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, "#")).