fix(wiki): first-occurrence wins when heading normalization collides pre-existing duplicates
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 <noreply@anthropic.com>
This commit is contained in:
@@ -41,9 +41,19 @@ func Merge(a, b Page) Page {
|
||||
fmA, preambleA, secsA := parseSections(a.Content)
|
||||
fmB, _, secsB := parseSections(b.Content)
|
||||
|
||||
// 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 {
|
||||
|
||||
@@ -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, "#")).
|
||||
|
||||
Reference in New Issue
Block a user