diff --git a/ingestion/internal/pipeline/pipeline.go b/ingestion/internal/pipeline/pipeline.go index 51d5dd3..f3ed9c4 100644 --- a/ingestion/internal/pipeline/pipeline.go +++ b/ingestion/internal/pipeline/pipeline.go @@ -372,15 +372,53 @@ func resolveSourceSlug(brainDir, wing, base string) (string, error) { } // dropSlugClashes removes, from the pages about to be written in a wing, any -// page whose slug already belongs to a different page elsewhere in the brain. -// The page's own path is not a clash (a shared entity merging into itself). -// A skipped page is a warning, not a failure: the ingest as a whole is fine, -// and one extracted page must not be able to lock a source out of re-ingest. +// page whose slug belongs to a different page. A slug is the graph identity, so +// two pages must not share one; and a page that shares one cannot be updated +// again, because every later ingest sees the other as its clash. +// +// Two kinds of clash, both by slug, neither failing the ingest (one extracted +// page must not be able to lock a source out of re-ingest): +// - inside the batch (a clip named notes.md whose model output also has an +// entity "Notes"): the source wins over an entity, an entity over a concept; +// - against the brain: an entity or concept whose slug is another page's. The +// SOURCE page is exempt: it is the note's own output, and its slug was +// already chosen free (or its own) by resolveSourceSlug. +// +// A page's own path is never a clash (a shared entity merging into itself). func dropSlugClashes(brainDir string, pages []wiki.Page) ([]wiki.Page, []string, error) { + rank := func(p wiki.Page) int { + switch { + case isSourcePath(p.Path): + return 0 + case strings.HasPrefix(p.Path, "wiki/entities/"): + return 1 + case strings.HasPrefix(p.Path, "wiki/concepts/"): + return 2 + } + return 3 + } + slugOf := func(p wiki.Page) string { return strings.TrimSuffix(filepath.Base(p.Path), ".md") } + + winner := map[string]wiki.Page{} + for _, p := range pages { + w, seen := winner[slugOf(p)] + if !seen || (w.Path != p.Path && rank(p) < rank(w)) { + winner[slugOf(p)] = p + } + } + var kept []wiki.Page var warnings []string for _, p := range pages { - slug := strings.TrimSuffix(filepath.Base(p.Path), ".md") + slug := slugOf(p) + if w := winner[slug]; w.Path != p.Path { + warnings = append(warnings, fmt.Sprintf("skipped %s: slug %q is also used by %s in this batch", p.Path, slug, w.Path)) + continue + } + if rank(p) == 0 { + kept = append(kept, p) + continue + } existing, err := wiki.FindSlugs(brainDir, slug) if err != nil { return nil, nil, fmt.Errorf("check slug %q: %w", slug, err) diff --git a/ingestion/internal/pipeline/wing_layout_test.go b/ingestion/internal/pipeline/wing_layout_test.go index 5664e4c..a881690 100644 --- a/ingestion/internal/pipeline/wing_layout_test.go +++ b/ingestion/internal/pipeline/wing_layout_test.go @@ -233,3 +233,62 @@ func TestResolveSourceSlug_AFurtherClashGetsANumber(t *testing.T) { require.NoError(t, err) assert.Equal(t, "talk-b-2", got) } + +// From the second review of #211: slug clashes INSIDE one batch. dropSlugClashes +// only looked at what was already on disk, so a source and an entity extracted +// together with one slug were both written, and neither could ever be updated. +func TestRun_ASourceAndAnEntityWithOneSlugInTheSameBatchWriteOnlyTheSource(t *testing.T) { + dir := t.TempDir() + pages := []RawPage{ + {Title: "Notes", Type: "source", Subtype: "note", Content: "## Summary\n\nfirst\n"}, + {Title: "Notes", Type: "entity", Subtype: "tool", Content: "## Definition\n\nan entity named like its source\n"}, + } + res, err := Run(context.Background(), wingCfg("homelab", pages...), dir, "body one", "notes", false) + require.NoError(t, err) + assert.True(t, exists(dir, "wiki/homelab/sources/notes.md")) + assert.False(t, exists(dir, "wiki/entities/notes.md"), "two pages must not share a slug") + assert.Contains(t, strings.Join(res.Warnings, "\n"), "notes") + + pages[0].Content = "## Summary\n\nfirst\n\n## Key Claims\n\n- second\n" + _, err = Run(context.Background(), wingCfg("homelab", pages...), dir, "body two, changed", "notes", false) + require.NoError(t, err) + assert.Contains(t, read(t, dir, "wiki/homelab/sources/notes.md"), "second", "the source can still be updated") +} + +func TestRun_AConceptAndAnEntityWithOneSlugInTheSameBatchWriteOne(t *testing.T) { + dir := t.TempDir() + pages := []RawPage{ + {Title: "Talk", Type: "source", Subtype: "video", Content: "## Summary\n\nx\n"}, + {Title: "Foo", Type: "concept", Content: "## Definition\n\na concept\n"}, + {Title: "Foo", Type: "entity", Subtype: "tool", Content: "## Definition\n\nan entity\n"}, + } + res, err := Run(context.Background(), wingCfg("homelab", pages...), dir, "body", "talk", false) + require.NoError(t, err) + written := 0 + for _, rel := range []string{"wiki/concepts/foo.md", "wiki/entities/foo.md"} { + if exists(dir, rel) { + written++ + } + } + assert.Equal(t, 1, written, "one slug, one page") + assert.Contains(t, strings.Join(res.Warnings, "\n"), "foo") +} + +// A brain that already holds the bad pair (written before this fix) must not +// lock the source out: the source page is the note's own output and is never +// the one skipped. +func TestRun_AnExistingSourceEntityPairDoesNotLockTheSourceOut(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "wiki/homelab/sources"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "wiki/entities"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "wiki/homelab/sources/notes.md"), + []byte("---\ntitle: 'Notes'\ntype: 'note'\nwing: homelab\nhall: sources\naliases:\n - 'Notes'\n---\n\n## Summary\n\nold\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "wiki/entities/notes.md"), + []byte("---\ntitle: 'Notes'\naliases:\n - 'Notes'\n---\n\n## Definition\n\nstale entity\n"), 0o644)) + + pages := []RawPage{{Title: "Notes", Type: "source", Subtype: "note", Content: "## Summary\n\nnew text\n\n## Key Claims\n\n- fresh\n"}} + _, err := Run(context.Background(), wingCfg("homelab", pages...), dir, "changed body", "notes", false) + require.NoError(t, err) + assert.Contains(t, read(t, dir, "wiki/homelab/sources/notes.md"), "fresh", "the source took the update") + assert.Contains(t, read(t, dir, "wiki/entities/notes.md"), "stale entity", "the entity is untouched") +}