From 6e83303752619ecc64f558f10bbeef14210565be Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 07:27:57 +0200 Subject: [PATCH 1/4] feat(pipeline): feed the current source page into the extraction prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #103 made ingest merge into the page already on disk instead of clobbering it, but the extraction itself stayed page-blind: the model wrote every page as if fresh, and Merge reconciled the two texts afterwards by heading. It could restate a claim but never refine it, and had no way to know what already existed. BuildPrompt now accepts the source page's current on-disk content (when one exists) and asks the model to revise it explicitly: add what's missing, strengthen what the source now supports, leave the rest alone and reuse its wording verbatim so unchanged claims are recognized as unchanged. Scoped to the source page specifically — its path (wiki/sources/.md) is deterministic before extraction runs, unlike a concept/entity page's path, which only exists once the model has produced a title. The deterministic merge in wiki.Merge is unchanged and stays the backstop: an on-disk section the model doesn't touch survives untouched regardless of what the prompt asks for, so a model that ignores the instruction still cannot destroy human content. Closes hyperguild#106 (first half; heading normalization follows). Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/pipeline/pipeline.go | 25 +++-- ingestion/internal/pipeline/pipeline_test.go | 98 ++++++++++++++++++++ ingestion/internal/pipeline/prompt.go | 26 +++++- 3 files changed, 141 insertions(+), 8 deletions(-) diff --git a/ingestion/internal/pipeline/pipeline.go b/ingestion/internal/pipeline/pipeline.go index ac63c52..1007729 100644 --- a/ingestion/internal/pipeline/pipeline.go +++ b/ingestion/internal/pipeline/pipeline.go @@ -62,13 +62,20 @@ func Run(ctx context.Context, cfg Config, brainDir, content, source string, dryR }}, nil } + // The source's own page path is deterministic (sourceSlug, computed above) + // even before extraction runs, unlike a concept/entity page's path, which + // only exists once the model has produced a title. So page-aware + // extraction (#106) is scoped to the source page: it's the one page every + // chunk's prompt can be given in advance. + existingPage := readSourcePage(brainDir, sourceSlug) + chunks := Chunk(content, cfg.ChunkSize) var allRaw []RawPage var allWarnings []string for _, chunk := range chunks { - userPrompt := BuildPrompt(schema, source, chunk, inventory) + userPrompt := BuildPrompt(schema, source, chunk, inventory, existingPage) output, err := cfg.Complete(ctx, systemPrompt, userPrompt) if err != nil { return Result{}, fmt.Errorf("LLM call: %w", err) @@ -168,6 +175,16 @@ func hashBody(content string) string { return hex.EncodeToString(sum[:]) } +// readSourcePage returns the full on-disk content of the source page at +// wiki/sources/.md, or "" when it doesn't exist yet. +func readSourcePage(brainDir, sourceSlug string) string { + raw, err := os.ReadFile(filepath.Join(brainDir, "wiki", "sources", sourceSlug+".md")) + if err != nil { + return "" + } + return string(raw) +} + // storedSourceHash reads source_hash from the source page a previous ingest // wrote, or "" when there is no such page or no such field. // @@ -175,11 +192,7 @@ func hashBody(content string) string { // skip costs one extraction, while a wrong skip loses an update silently — and // silence is the failure mode this repo keeps paying for. func storedSourceHash(brainDir, sourceSlug string) string { - raw, err := os.ReadFile(filepath.Join(brainDir, "wiki", "sources", sourceSlug+".md")) - if err != nil { - return "" - } - for _, line := range strings.Split(string(raw), "\n") { + for _, line := range strings.Split(readSourcePage(brainDir, sourceSlug), "\n") { if key, value, found := strings.Cut(line, ":"); found && strings.TrimSpace(key) == "source_hash" { return strings.TrimSpace(value) } diff --git a/ingestion/internal/pipeline/pipeline_test.go b/ingestion/internal/pipeline/pipeline_test.go index 208f913..5c6d048 100644 --- a/ingestion/internal/pipeline/pipeline_test.go +++ b/ingestion/internal/pipeline/pipeline_test.go @@ -277,6 +277,104 @@ func TestRun_ReExtractsWhenTheSourceBodyChanged(t *testing.T) { assert.Equal(t, 2, calls, "changed content must be re-extracted") } +// --- #106: page-aware extraction — the model sees the page it is revising -- + +func TestRun_ExistingSourcePageIsIncludedInTheExtractionPrompt(t *testing.T) { + // #103 made the merge asymmetric (on-disk wins); #106 is the other half — + // the extraction itself gets to see what's already there instead of + // producing a page blind and letting the merge reconcile it afterwards. + // This only proves the existing page reaches the prompt; the byte-identical + // guarantee for an echoed-back revision is a separate test below. + dir := t.TempDir() + for _, sub := range []string{"wiki/concepts", "wiki/entities", "wiki/sources"} { + require.NoError(t, os.MkdirAll(filepath.Join(dir, sub), 0o755)) + } + + seeded := "---\ntitle: 'Shape Up'\n---\n\n## Summary\n\nA book about shaping product work.\n\n## Key Claims\n\n- Appetite beats estimation.\n" + page := filepath.Join(dir, "wiki", "sources", "shape-up.md") + require.NoError(t, os.WriteFile(page, []byte(seeded), 0o644)) + + var gotPrompt string + cfg := Config{Complete: func(_ context.Context, _, user string) (string, error) { + gotPrompt = user + return mustJSON([]RawPage{{ + Title: "Shape Up", Type: "source", Subtype: "book", + Content: "## Summary\n\nA book about shaping product work.\n", + }}), nil + }} + + // Deliberately does not mention "Appetite beats estimation." — that phrase + // can only reach the prompt via the existing on-disk page, not the source + // content, so its presence proves the page was threaded through. + content := "A short editorial update to the book's listing. No new claims here." + _, err := Run(context.Background(), cfg, dir, content, "shape-up", false) + require.NoError(t, err) + + assert.Contains(t, gotPrompt, "Appetite beats estimation.", + "the existing page's content must be visible to the extraction prompt") + assert.Contains(t, gotPrompt, "revis", "the prompt must ask for a revision, not a fresh page") +} + +func TestRun_NewSourceHasNoExistingPageInThePrompt(t *testing.T) { + // The other side of the same behavior: a source with no page on disk yet + // must not get an "existing page" section injected into its prompt at all. + dir := t.TempDir() + for _, sub := range []string{"wiki/concepts", "wiki/entities", "wiki/sources"} { + require.NoError(t, os.MkdirAll(filepath.Join(dir, sub), 0o755)) + } + + var gotPrompt string + cfg := Config{Complete: func(_ context.Context, _, user string) (string, error) { + gotPrompt = user + return mustJSON([]RawPage{{ + Title: "New Thing", Type: "source", Subtype: "article", + Content: "## Summary\n\nBrand new.\n", + }}), nil + }} + + _, err := Run(context.Background(), cfg, dir, "Brand new content.", "new-thing", false) + require.NoError(t, err) + + assert.NotContains(t, gotPrompt, "Current page", + "a source with no page on disk yet must not get a revision section in its prompt") +} + +func TestRun_RevisionEchoingExistingPageStaysByteIdentical(t *testing.T) { + // The issue's explicit risk: a model given the current page will sometimes + // echo it back verbatim. That must not duplicate content or otherwise + // perturb the page — the deterministic merge is still the backstop. + dir := t.TempDir() + for _, sub := range []string{"wiki/concepts", "wiki/entities", "wiki/sources"} { + require.NoError(t, os.MkdirAll(filepath.Join(dir, sub), 0o755)) + } + + seeded := "---\ntitle: 'Shape Up'\n---\n\n## Summary\n\nA book about shaping product work.\n\n## Key Claims\n\n- Appetite beats estimation.\n" + page := filepath.Join(dir, "wiki", "sources", "shape-up.md") + require.NoError(t, os.WriteFile(page, []byte(seeded), 0o644)) + + cfg := Config{Complete: func(_ context.Context, _, _ string) (string, error) { + return mustJSON([]RawPage{{ + Title: "Shape Up", Type: "source", Subtype: "book", + Content: "## Summary\n\nA book about shaping product work.\n\n## Key Claims\n\n- Appetite beats estimation.\n", + }}), nil + }} + + content := "A short editorial update to the book's listing. No new claims here." + result, err := Run(context.Background(), cfg, dir, content, "shape-up", false) + require.NoError(t, err) + require.False(t, result.Skipped) + + got, err := os.ReadFile(page) + require.NoError(t, err) + + want := "---\ntitle: 'Shape Up'\nlast_updated: " + time.Now().UTC().Format("2006-01-02") + + "\nsource_hash: " + hashBody(content) + "\n---\n\n" + + "## Summary\n\nA book about shaping product work.\n\n" + + "## Key Claims\n\n- Appetite beats estimation.\n" + assert.Equal(t, want, string(got), + "a revision pass that echoes the existing page back must not duplicate or alter its content") +} + func TestRun_FrontmatterOnlyChangeIsNotDrift(t *testing.T) { // The hash covers the body, so re-fetching a page whose metadata moved // (a re-crawl stamping a new date) is not treated as the source changing. diff --git a/ingestion/internal/pipeline/prompt.go b/ingestion/internal/pipeline/prompt.go index 00b65c7..bdfa2e5 100644 --- a/ingestion/internal/pipeline/prompt.go +++ b/ingestion/internal/pipeline/prompt.go @@ -22,8 +22,18 @@ Each element must have exactly these fields: Wikilinks in content: [[Display Name]] — just the display name, no slug, no pipe separator. Only link to pages listed in the inventory or pages you are creating in this response.` -// BuildPrompt constructs the user prompt for a single chunk. -func BuildPrompt(schema, source, content string, inventory map[wiki.PageType][]wiki.Entry) string { +// BuildPrompt constructs the user prompt for a single chunk. existingPage is +// the full current content (frontmatter + body) of the source's own wiki page +// when one already exists on disk, or "" for a source being ingested for the +// first time. When non-empty, the model is asked to revise that page rather +// than write a fresh one — see hyperguild#106. The deterministic merge in +// wiki.Merge remains the backstop regardless of what the model returns: an +// on-disk section the model doesn't touch survives untouched, and a section +// it does touch only takes effect where Merge's strategy allows the fresh +// side to change anything (bullet union, append) — never for an ordinary +// section, so the model proposing a revision is never how a page gets +// overwritten. +func BuildPrompt(schema, source, content string, inventory map[wiki.PageType][]wiki.Entry, existingPage string) string { var sb strings.Builder fmt.Fprintf(&sb, "Today's date is %s.\n\n", time.Now().UTC().Format("2006-01-02")) @@ -49,6 +59,18 @@ func BuildPrompt(schema, source, content string, inventory map[wiki.PageType][]w sb.WriteString("\n") } + if existingPage != "" { + sb.WriteString("## Current page for this source — revise it, don't restart\n\n") + sb.WriteString("This source has already been ingested and has the wiki page shown below.\n") + sb.WriteString("You are revising that page, not writing a fresh one:\n") + sb.WriteString(" - Add sections or claims the source below supports that this page doesn't have yet.\n") + sb.WriteString(" - Strengthen a claim where the source gives more detail, evidence, or nuance than what's already there.\n") + sb.WriteString(" - Leave unchanged whatever the source doesn't affect — reuse the existing wording verbatim rather than paraphrasing it, so an unchanged claim is recognized as unchanged.\n") + sb.WriteString(" - A merge step keeps this page's on-disk content for any section you don't return, so it's fine to omit a section you aren't revising.\n\n") + sb.WriteString(existingPage) + sb.WriteString("\n\n") + } + sb.WriteString("## Non-negotiable rules\n\n") sb.WriteString("1. Output ONLY a valid JSON array — no prose, no fences.\n") sb.WriteString("2. Fields: title, type, subtype (if applicable), domain (if applicable), content.\n") -- 2.54.0 From bf935d89bf6574dd4297dc5bc5a1076b93b2a486 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 07:28:11 +0200 Subject: [PATCH 2/4] fix(wiki): normalise heading casing/whitespace before matching in Merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Page-aware extraction (previous commit) asks the model to revise the page it's given, but nothing guarantees it reproduces a heading's exact on-disk text. Before this, Merge matched sections by exact string equality, so "Key Claims" vs "Key claims" (or trailing whitespace) silently created a parallel section instead of updating the original — the drift #106 calls out by name. Merge's idx map and the bulletSections/appendSections strategy lookup now key on normalizeHeading (trim + lowercase), so casing/whitespace drift matches the same section. The on-disk heading's original text (secsA[i].heading) is untouched and is what gets written out — the normalized form is used only to decide "is this the same section," never to rewrite a human's heading casing. Touches wiki/merge.go, which is explicitly out of casual-edit scope per today's review history on this file: justified because heading normalization is #106's own checklist item and has to hook into exactly this matching logic — there's nowhere else to put it. Scope kept minimal: the two lookup sites only, no other behavior in this file changed. bulletSectionsNorm/appendSectionsNorm exist because matching a section by its normalized key but then classifying its merge strategy by exact-string lookup would silently fall through to "keep a's version" for a case-drifted bullet/append heading — dropping b's content instead of unioning or appending it. Closes hyperguild#106. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/wiki/merge.go | 39 +++++++++++++++++++++++---- ingestion/internal/wiki/merge_test.go | 36 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/ingestion/internal/wiki/merge.go b/ingestion/internal/wiki/merge.go index 644035a..f41b53b 100644 --- a/ingestion/internal/wiki/merge.go +++ b/ingestion/internal/wiki/merge.go @@ -66,21 +66,22 @@ func MergeReport(a, b Page) (Page, []string) { var conflicts []string idx := make(map[string]int, len(secsA)) for i, s := range secsA { - idx[s.heading] = i + idx[normalizeHeading(s.heading)] = i } for _, sB := range secsB { - i, exists := idx[sB.heading] + key := normalizeHeading(sB.heading) + i, exists := idx[key] if !exists { - idx[sB.heading] = len(secsA) + idx[key] = len(secsA) secsA = append(secsA, sB) continue } sA := secsA[i] switch { - case bulletSections[sB.heading]: + case bulletSectionsNorm[key]: secsA[i].content = mergeBullets(sA.content, sB.content) - case appendSections[sB.heading]: + case appendSectionsNorm[key]: secsA[i].content = appendNew(sA.content, sB.content) default: // a wins — but say that b disagreed, rather than dropping it mute. @@ -93,6 +94,34 @@ func MergeReport(a, b Page) (Page, []string) { return Page{Path: a.Path, Content: rebuildContent(refreshOwned(fmA, fmB), preambleA, secsA)}, conflicts } +// normalizeHeading returns the key Merge uses to decide "is this the same +// section" — trimmed and case-folded, so "Key Claims" and "Key claims" match +// (hyperguild#106: heading casing drift must not fork a section). It is used +// only for that comparison; the on-disk section's original heading text +// (secsA[i].heading, untouched here) is what actually gets written out, so a +// human's heading casing is never silently rewritten by a merge. +func normalizeHeading(h string) string { + return strings.ToLower(strings.TrimSpace(h)) +} + +// bulletSectionsNorm and appendSectionsNorm are bulletSections/appendSections +// keyed by normalizeHeading instead of exact text, so a section's merge +// strategy (union bullets, append) is chosen by the same normalized identity +// Merge uses to match it — otherwise a heading that matched an on-disk +// section (case-insensitively) could still fail the exact-string strategy +// lookup and silently fall through to "keep a's version," dropping b's +// content instead of unioning or appending it. +var bulletSectionsNorm = normalizeSet(bulletSections) +var appendSectionsNorm = normalizeSet(appendSections) + +func normalizeSet(names map[string]bool) map[string]bool { + out := make(map[string]bool, len(names)) + for k := range names { + out[normalizeHeading(k)] = true + } + return out +} + // generatorOwned names the frontmatter fields the ingest pipeline may refresh // on a page it is revising. Everything else on disk wins. // diff --git a/ingestion/internal/wiki/merge_test.go b/ingestion/internal/wiki/merge_test.go index 4286ff3..8386815 100644 --- a/ingestion/internal/wiki/merge_test.go +++ b/ingestion/internal/wiki/merge_test.go @@ -305,6 +305,42 @@ Rewritten by the model. assert.Equal(t, want, got) } +// --- hyperguild#106: heading casing drift must not fork a section ---------- +// Page-aware extraction asks the model to revise the page it's given, but +// nothing guarantees the model reproduces a heading's exact on-disk casing +// ("Key Claims" vs "Key claims"). Without normalizing the match key, that +// drift silently creates a near-duplicate section instead of updating the +// original — the failure mode #106 exists to close. + +func TestMerge_HeadingCaseDriftMatchesSameSection(t *testing.T) { + a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Key Claims\n\n- Appetite beats estimation.\n"} + b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Key claims\n\n- New pattern holds under load.\n"} + + got := Merge(a, b).Content + + assert.Equal(t, 1, strings.Count(got, "## Key"), + "casing drift in the heading must not create a second section") + assert.Contains(t, got, "Appetite beats estimation.") + assert.Contains(t, got, "New pattern holds under load.", + "the differently-cased section's content must still be unioned in") + assert.Contains(t, got, "## Key Claims", + "the on-disk heading's original casing must be preserved in the output") + assert.NotContains(t, got, "## Key claims", + "the fresh extraction's heading casing must not overwrite the on-disk display text") +} + +func TestMerge_HeadingWhitespaceDriftMatchesSameSection(t *testing.T) { + a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Open Questions \n\n- Does appetite scale?\n"} + b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Open Questions\n\n- What about remote teams?\n"} + + got := Merge(a, b).Content + + assert.Equal(t, 1, strings.Count(got, "## Open Questions"), + "surrounding whitespace drift in the heading must not create a second section") + assert.Contains(t, got, "Does appetite scale?") + assert.Contains(t, got, "What about remote teams?") +} + func TestRefreshOwned_CommentLinesSurviveUnmangled(t *testing.T) { // Third review round on PR #126: parseFrontmatterBlock dropped the // sibling api package's comment guard (strings.HasPrefix(trimmed, "#")). -- 2.54.0 From 921bea8738c7cb2d45a4fab64a409886d5618385 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 07:34:49 +0200 Subject: [PATCH 3/4] fix(wiki): first-occurrence wins when heading normalization collides pre-existing duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of PR #130 found a real bug in the new normalizeHeading matching: idx was built with idx[normalizeHeading(s.heading)] = i, plain map assignment while iterating secsA — last write wins. A page that already carries two on-disk sections differing only by case or whitespace (real, pre-existing corpus drift, the exact pattern #106 itself cites) never collided in idx before this normalization existed, since it keyed on the exact string. After normalization they collide, and every future update silently, deterministically routes to whichever duplicate happens to sort last in the file — the first one is frozen forever, never receiving an update again, with nothing to say so. Same bug class parseSections's own doc comment already warns about (two keys sharing one map slot means one loses lookup visibility), reintroduced here. Fixed by keeping only the first occurrence's index when a collision happens while building idx — deterministic, and matches "earliest is canonical," the same reasoning behind every other on-disk-wins default in this file. New test reproduces the exact review finding with a positional assertion (the fresh bullet must land adjacent to the first occurrence's own content, not the second) — a Contains-only version of this test would have passed even with the bug present, since nothing is literally deleted, a bullet just silently routes to the wrong of two still-present sections. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/wiki/merge.go | 13 ++++++++++++- ingestion/internal/wiki/merge_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/ingestion/internal/wiki/merge.go b/ingestion/internal/wiki/merge.go index f41b53b..119a11f 100644 --- a/ingestion/internal/wiki/merge.go +++ b/ingestion/internal/wiki/merge.go @@ -64,9 +64,20 @@ func MergeReport(a, b Page) (Page, []string) { fmB, _, secsB := parseSections(b.Content) var conflicts []string + + // First occurrence wins on a collision — a page can already carry two + // on-disk sections that only differ by case or whitespace (the exact + // corpus drift this normalization exists to catch going forward, found + // pre-existing in review of PR #130): a plain last-write assignment + // here would silently and deterministically route every future update + // to whichever duplicate happens to sort last in the file, freezing + // the other one forever with nothing to say so. idx := make(map[string]int, len(secsA)) for i, s := range secsA { - idx[normalizeHeading(s.heading)] = i + key := normalizeHeading(s.heading) + if _, exists := idx[key]; !exists { + idx[key] = i + } } for _, sB := range secsB { diff --git a/ingestion/internal/wiki/merge_test.go b/ingestion/internal/wiki/merge_test.go index 8386815..2cb135e 100644 --- a/ingestion/internal/wiki/merge_test.go +++ b/ingestion/internal/wiki/merge_test.go @@ -341,6 +341,32 @@ func TestMerge_HeadingWhitespaceDriftMatchesSameSection(t *testing.T) { assert.Contains(t, got, "What about remote teams?") } +func TestMerge_PreExistingCaseDuplicateHeadingsRouteToTheFirstOccurrence(t *testing.T) { + // Found in review of PR #130: normalizeHeading's idx map is built by + // iterating secsA with idx[normalizeHeading(s.heading)] = i -- last + // write wins. Before heading normalization existed, two on-disk + // sections differing only by case (real, pre-existing corpus drift -- + // the exact pattern #106 itself cites) never collided, since idx keyed + // on the exact string. After normalization they collide, and every + // fresh update silently, deterministically routes to whichever + // duplicate happens to appear LAST in the file -- the first one is + // frozen forever, never receiving updates again, with nothing to say + // so. A Contains-only check doesn't catch this: nothing is literally + // deleted, a bullet just lands under the wrong of two still-present + // headings. Assert it lands under the FIRST occurrence instead — + // deterministic and matches "earliest is canonical," same reasoning + // as every other on-disk-wins default in this file. + a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Key Claims\n\n- claim one\n\n## KEY CLAIMS\n\n- claim two\n"} + b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Key Claims\n\n- claim one\n- fresh claim\n"} + + got := Merge(a, b).Content + + assert.Contains(t, got, "## Key Claims\n\n- claim one\n- fresh claim\n", + "the fresh bullet must land in the first-occurrence section, not silently route to the last duplicate") + assert.Contains(t, got, "## KEY CLAIMS\n\n- claim two\n", + "the second duplicate is untouched by this update, not merged into or overwritten") +} + func TestRefreshOwned_CommentLinesSurviveUnmangled(t *testing.T) { // Third review round on PR #126: parseFrontmatterBlock dropped the // sibling api package's comment guard (strings.HasPrefix(trimmed, "#")). -- 2.54.0 From bdba49b7561d5fcad1ced1a6d05277611bf77a25 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 07:43:50 +0200 Subject: [PATCH 4/4] fix(wiki): pair duplicate-heading occurrences positionally, not all to one index Review of PR #132 found a real bug in the first-occurrence-wins fix: routing every one of b's occurrences of a normalized heading to a's single first occurrence means a self-merge (or a revision pass echoing a page back unchanged, exactly what #106 can produce) pairs b's SECOND "## NOTES" against a's FIRST "## Notes" -- different content, same normalized key -- and MergeReport wrongly reports a disagreement where none exists. Inert in #130 (fed the vestigial Merge wrapper, which discards conflicts) and only became visible once correctly wired into MergeReport, the function production actually calls. Fixed by pairing occurrences positionally: a's on-disk sections sharing a normalized heading are collected into an ordered list per key: b's 1st occurrence of that key pairs with a's 1st, b's 2nd with a's 2nd, and so on. b having MORE occurrences than a appends the extra as new content and registers it, so a third occurrence in b pairs with that newly appended one rather than creating yet another duplicate. Flip-tested: forcing every pairing back to position 0 (the prior fix's actual behavior) turns the new self-merge test red; the positional version stays green, and the original PR #130 regression test (fresh content must route to the first occurrence when b has only one occurrence) still passes unchanged -- the two fixes aren't in tension, positional pairing subsumes first-occurrence-wins as the pos==0 case. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/wiki/merge.go | 43 ++++++++++++++++++--------- ingestion/internal/wiki/merge_test.go | 30 +++++++++++++++++++ 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/ingestion/internal/wiki/merge.go b/ingestion/internal/wiki/merge.go index 119a11f..e80be52 100644 --- a/ingestion/internal/wiki/merge.go +++ b/ingestion/internal/wiki/merge.go @@ -65,29 +65,44 @@ func MergeReport(a, b Page) (Page, []string) { var conflicts []string - // First occurrence wins on a collision — a page can already carry two - // on-disk sections that only differ by case or whitespace (the exact - // corpus drift this normalization exists to catch going forward, found - // pre-existing in review of PR #130): a plain last-write assignment - // here would silently and deterministically route every future update - // to whichever duplicate happens to sort last in the file, freezing - // the other one forever with nothing to say so. - idx := make(map[string]int, len(secsA)) + // Sections in secsA sharing a normalized heading (pre-existing corpus + // drift — "## Notes" and "## NOTES" both already on disk, the exact + // pattern this normalization exists to catch going forward, found in + // review of PR #130/#132) are matched to b's occurrences of that same + // heading POSITIONALLY: b's 1st occurrence of the key pairs with a's + // 1st, b's 2nd with a's 2nd, and so on. A single "last write wins" (or + // even "first write wins") map from key to one index would instead + // route every one of b's occurrences to the SAME on-disk section — + // which either freezes every duplicate but one (never receiving + // updates again) or, worse, compares mismatched pairs and reports a + // disagreement between two sections that were never supposed to be + // compared, false-positiving on the plain self-merge case a revision + // pass produces when the model echoes a page back unchanged (found in + // review of PR #132). + byKey := make(map[string][]int, len(secsA)) for i, s := range secsA { key := normalizeHeading(s.heading) - if _, exists := idx[key]; !exists { - idx[key] = i - } + byKey[key] = append(byKey[key], i) } + cursor := make(map[string]int, len(byKey)) for _, sB := range secsB { key := normalizeHeading(sB.heading) - i, exists := idx[key] - if !exists { - idx[key] = len(secsA) + indices := byKey[key] + pos := cursor[key] + cursor[key] = pos + 1 + + if pos >= len(indices) { + // b has more occurrences of this heading than a does (including + // the case where a has none at all) — the extra is new content, + // appended as its own section and registered so a THIRD + // occurrence of the same heading in b pairs with this one + // rather than appending yet another duplicate. + byKey[key] = append(byKey[key], len(secsA)) secsA = append(secsA, sB) continue } + i := indices[pos] sA := secsA[i] switch { case bulletSectionsNorm[key]: diff --git a/ingestion/internal/wiki/merge_test.go b/ingestion/internal/wiki/merge_test.go index 2cb135e..9a8464b 100644 --- a/ingestion/internal/wiki/merge_test.go +++ b/ingestion/internal/wiki/merge_test.go @@ -367,6 +367,36 @@ func TestMerge_PreExistingCaseDuplicateHeadingsRouteToTheFirstOccurrence(t *test "the second duplicate is untouched by this update, not merged into or overwritten") } +func TestMergeReport_SelfMergeOfPreExistingDuplicateHeadingsReportsNoConflict(t *testing.T) { + // Found in review of PR #132: first-occurrence-wins routes ALL of b's + // occurrences of a normalized heading to a's FIRST occurrence, not to + // their own positional counterpart. So merging a page against itself + // (or an echoed-back copy, exactly what #106's revision prompt can + // produce) pairs b's second "## NOTES" against a's first "## Notes" — + // different content, same normalized key — and MergeReport wrongly + // reports a disagreement where none exists. Fixed by pairing + // positionally: b's Nth occurrence of a normalized heading matches a's + // Nth occurrence, not always the first. + page := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Notes\n\nFirst note.\n\n## NOTES\n\nSecond note.\n"} + + _, conflicts := MergeReport(page, page) + assert.Empty(t, conflicts, "merging a page with pre-existing duplicate headings against itself must report no conflicts") +} + +func TestMergeReport_DuplicateHeadingsPairPositionallyNotAllToFirst(t *testing.T) { + // Companion case: b has genuinely different content in its second + // occurrence of the normalized heading — that must compare against a's + // second occurrence (and report if they disagree), not against a's + // first (which would either false-positive if a's first differs, or + // false-negative-mask a real disagreement in the second pair). + a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Notes\n\nFirst note.\n\n## NOTES\n\nSecond note.\n"} + b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Notes\n\nFirst note.\n\n## NOTES\n\nSecond note, revised.\n"} + + _, conflicts := MergeReport(a, b) + assert.Equal(t, []string{"NOTES"}, conflicts, + "only the second occurrence, which actually disagrees, is reported") +} + func TestRefreshOwned_CommentLinesSurviveUnmangled(t *testing.T) { // Third review round on PR #126: parseFrontmatterBlock dropped the // sibling api package's comment guard (strings.HasPrefix(trimmed, "#")). -- 2.54.0