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 <noreply@anthropic.com>
This commit is contained in:
@@ -65,29 +65,44 @@ func MergeReport(a, b Page) (Page, []string) {
|
|||||||
|
|
||||||
var conflicts []string
|
var conflicts []string
|
||||||
|
|
||||||
// First occurrence wins on a collision — a page can already carry two
|
// Sections in secsA sharing a normalized heading (pre-existing corpus
|
||||||
// on-disk sections that only differ by case or whitespace (the exact
|
// drift — "## Notes" and "## NOTES" both already on disk, the exact
|
||||||
// corpus drift this normalization exists to catch going forward, found
|
// pattern this normalization exists to catch going forward, found in
|
||||||
// pre-existing in review of PR #130): a plain last-write assignment
|
// review of PR #130/#132) are matched to b's occurrences of that same
|
||||||
// here would silently and deterministically route every future update
|
// heading POSITIONALLY: b's 1st occurrence of the key pairs with a's
|
||||||
// to whichever duplicate happens to sort last in the file, freezing
|
// 1st, b's 2nd with a's 2nd, and so on. A single "last write wins" (or
|
||||||
// the other one forever with nothing to say so.
|
// even "first write wins") map from key to one index would instead
|
||||||
idx := make(map[string]int, len(secsA))
|
// 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 {
|
for i, s := range secsA {
|
||||||
key := normalizeHeading(s.heading)
|
key := normalizeHeading(s.heading)
|
||||||
if _, exists := idx[key]; !exists {
|
byKey[key] = append(byKey[key], i)
|
||||||
idx[key] = i
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
cursor := make(map[string]int, len(byKey))
|
||||||
|
|
||||||
for _, sB := range secsB {
|
for _, sB := range secsB {
|
||||||
key := normalizeHeading(sB.heading)
|
key := normalizeHeading(sB.heading)
|
||||||
i, exists := idx[key]
|
indices := byKey[key]
|
||||||
if !exists {
|
pos := cursor[key]
|
||||||
idx[key] = len(secsA)
|
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)
|
secsA = append(secsA, sB)
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
i := indices[pos]
|
||||||
sA := secsA[i]
|
sA := secsA[i]
|
||||||
switch {
|
switch {
|
||||||
case bulletSectionsNorm[key]:
|
case bulletSectionsNorm[key]:
|
||||||
|
|||||||
@@ -367,6 +367,36 @@ func TestMerge_PreExistingCaseDuplicateHeadingsRouteToTheFirstOccurrence(t *test
|
|||||||
"the second duplicate is untouched by this update, not merged into or overwritten")
|
"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) {
|
func TestRefreshOwned_CommentLinesSurviveUnmangled(t *testing.T) {
|
||||||
// Third review round on PR #126: parseFrontmatterBlock dropped the
|
// Third review round on PR #126: parseFrontmatterBlock dropped the
|
||||||
// sibling api package's comment guard (strings.HasPrefix(trimmed, "#")).
|
// sibling api package's comment guard (strings.HasPrefix(trimmed, "#")).
|
||||||
|
|||||||
Reference in New Issue
Block a user