From 874206be9d4e2080be51822cc2400f0c65aa0c66 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 07:01:27 +0200 Subject: [PATCH] fix(wiki): rewrite refreshOwned on a parsed line list, not fence-index tracking Second review round on PR #126 found two more problems with the first append fix: 1. When a has no frontmatter fence at all, the fallback spliced raw, unfenced "key: value" text straight into the body -- worse than the original bug, since the next parseSections call wouldn't recognize it as frontmatter and would absorb it as preamble text, duplicating on every subsequent merge. 2. The regression test only asserted substrings (Contains), which doesn't prove structural position. Two independent mutations (closeIdx locking onto the wrong fence, a reversed sort) both survived the full suite undetected. Rewritten from string-index surgery (tracking a fence's position across two passes over split lines) to a small parsed representation: fmLine (key/value or raw), parseFrontmatterBlock, setLine (update in place or append -- the one behavior the original code got wrong), and renderFrontmatterBlock. Same shape as the sibling api package's own frontmatter type, reproduced locally rather than imported (it's unexported there, and this package's needs are smaller) because that set()-appends-if-absent behavior was already proven correct by that package's existing test suite -- this bug class shouldn't have existed here in the first place. A page with no fence gets one created to hold the new fields, instead of corrupting the body. Tests now assert exact frontmatter block output via assert.Equal, not Contains, and a new fixture (mixed replace+append in one call) covers both code paths together. Flip-tested: reversing the sort turns two of the new exact-match tests red (the Contains-based version of the same test missed this on the prior commit). The "wrong fence" bug class is now structurally impossible rather than merely untested -- there's no fence-position index left to get wrong. Also fixed a byte-stability regression the new render introduced and caught via the full pipeline suite: a key with no inline value (the YAML block-list shape, "aliases:" followed by indented "- item" lines) rendered as "aliases: " (trailing space) instead of "aliases:". Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/wiki/merge.go | 153 +++++++++++++++++--------- ingestion/internal/wiki/merge_test.go | 64 ++++++++++- 2 files changed, 159 insertions(+), 58 deletions(-) diff --git a/ingestion/internal/wiki/merge.go b/ingestion/internal/wiki/merge.go index 94baa7c..64fd226 100644 --- a/ingestion/internal/wiki/merge.go +++ b/ingestion/internal/wiki/merge.go @@ -82,75 +82,122 @@ var generatorOwned = map[string]bool{ "source_hash": true, } +// fmLine is one line of a frontmatter block: a parsed key/value pair, or a +// raw line (blank, comment, or anything else that isn't a simple "key: +// value") kept verbatim. Same shape as api.frontmatter's fmLine — this +// package doesn't import that one (it's unexported there, and a's/b's +// frontmatter here always arrives as a string, not a parsed struct), so the +// small, already-proven set()-appends-if-absent behavior is reproduced +// locally rather than string-index surgery reinvented per call site. +type fmLine struct { + key, value, raw string +} + +// parseFrontmatterBlock parses a "---\n...\n---\n" fenced block (the shape +// parseSections produces) into an ordered line list. A block with no +// opening/closing fence — including the empty string, a page with no +// frontmatter at all — returns nil, ok=false: there is nothing to update in +// place, only something to potentially create. +func parseFrontmatterBlock(block string) (lines []fmLine, ok bool) { + if !strings.HasPrefix(block, "---\n") { + return nil, false + } + rest := block[len("---\n"):] + end := strings.Index(rest, "\n---\n") + if end < 0 { + return nil, false + } + for _, line := range strings.Split(rest[:end], "\n") { + key, val, found := strings.Cut(line, ":") + key = strings.TrimSpace(key) + if !found || key == "" { + lines = append(lines, fmLine{raw: line}) + continue + } + lines = append(lines, fmLine{key: key, value: strings.TrimSpace(val)}) + } + return lines, true +} + +// setLine updates key's value in place if lines already has it, or appends +// a new entry if it doesn't — the one behavior the original refreshOwned +// got wrong (PR #126 review, round 2: it only ever updated in place). +func setLine(lines []fmLine, key, value string) []fmLine { + for i := range lines { + if lines[i].key == key { + lines[i].value = value + return lines + } + } + return append(lines, fmLine{key: key, value: value}) +} + +func renderFrontmatterBlock(lines []fmLine) string { + if len(lines) == 0 { + return "" + } + var sb strings.Builder + sb.WriteString("---\n") + for _, l := range lines { + switch { + case l.key == "": + sb.WriteString(l.raw) + case l.value == "": + // A key with nothing after the colon (the YAML block-list shape: + // "aliases:" followed by indented "- item" lines on their own + // raw lines) must stay bare — "key: " with a trailing space + // is a different, if invisible, byte sequence, and this file's + // whole purpose is byte-stability across an unrelated merge. + sb.WriteString(l.key) + sb.WriteByte(':') + default: + sb.WriteString(l.key) + sb.WriteString(": ") + sb.WriteString(l.value) + } + sb.WriteByte('\n') + } + sb.WriteString("---\n") + return sb.String() +} + // refreshOwned returns a's frontmatter with the generator-owned fields taken // from b where b supplies them. Fields absent from b keep a's value, so a // generator that stops emitting a field cannot blank it. // -// A field b supplies that a's frontmatter doesn't have yet — every page -// ingested before this mechanism existed has no source_hash line at all — -// is appended, not dropped. Dropping it silently defeats the whole reason a -// field is generator-owned: it would never reach the page it's meant to -// live on (found in review, PR #126: source_hash then never populates for -// the pre-existing corpus, so the skip-on-unchanged feature it exists to -// support never engages, permanently, with nothing to say so). +// A field b supplies that a doesn't have yet — every page ingested before +// this mechanism existed has no source_hash line at all — is appended, not +// dropped (round 1 of review: dropping it silently meant source_hash never +// populated for the pre-existing corpus). If a has no frontmatter fence at +// all, one is created to hold the new fields rather than splicing raw +// "key: value" text into the body (round 2: that corrupted the page and the +// fields would duplicate on every subsequent merge, since unfenced text +// isn't recognized as frontmatter by parseSections on the next pass). func refreshOwned(a, b string) string { + bLines, _ := parseFrontmatterBlock(b) bVals := make(map[string]string) - for _, line := range strings.Split(b, "\n") { - key, value, found := strings.Cut(line, ":") - if !found || !generatorOwned[strings.TrimSpace(key)] { - continue + for _, l := range bLines { + if l.key != "" && generatorOwned[l.key] { + bVals[l.key] = l.value } - bVals[strings.TrimSpace(key)] = strings.TrimSpace(value) } if len(bVals) == 0 { return a } - aLines := strings.Split(a, "\n") - out := make([]string, 0, len(aLines)+len(bVals)) - closeIdx := -1 - for _, line := range aLines { - key, _, found := strings.Cut(line, ":") - k := strings.TrimSpace(key) - if found && generatorOwned[k] { - if v, ok := bVals[k]; ok { - out = append(out, k+": "+v) - delete(bVals, k) - continue - } - } - if strings.TrimSpace(line) == "---" { - closeIdx = len(out) // the fence's position in the OUTPUT slice - } - out = append(out, line) - } + aLines, _ := parseFrontmatterBlock(a) // ok==false (no fence) -> aLines nil, same as "start fresh" - if len(bVals) == 0 { - return strings.Join(out, "\n") - } - - // Sorted for a deterministic, byte-stable result — map order is not. - remaining := make([]string, 0, len(bVals)) + // Sorted for a deterministic, byte-stable result when more than one + // field is appended in the same call — map iteration order isn't. + keys := make([]string, 0, len(bVals)) for k := range bVals { - remaining = append(remaining, k) + keys = append(keys, k) } - sort.Strings(remaining) - insert := make([]string, len(remaining)) - for i, k := range remaining { - insert[i] = k + ": " + bVals[k] + sort.Strings(keys) + for _, k := range keys { + aLines = setLine(aLines, k, bVals[k]) } - - if closeIdx < 0 { - // No closing fence found — malformed input for this well-formed-only - // helper; append at the end rather than lose the fields. - out = append(out, insert...) - return strings.Join(out, "\n") - } - final := make([]string, 0, len(out)+len(insert)) - final = append(final, out[:closeIdx]...) - final = append(final, insert...) - final = append(final, out[closeIdx:]...) - return strings.Join(final, "\n") + return renderFrontmatterBlock(aLines) } // parseSections splits markdown into its frontmatter, its preamble (content diff --git a/ingestion/internal/wiki/merge_test.go b/ingestion/internal/wiki/merge_test.go index 5ba8495..8f730cf 100644 --- a/ingestion/internal/wiki/merge_test.go +++ b/ingestion/internal/wiki/merge_test.go @@ -244,9 +244,63 @@ Rewritten by the model. got := Merge(existing, fresh).Content - assert.Contains(t, got, "source_hash: bbbb", "a generator-owned field a page never had before must be appended, not dropped") - assert.Contains(t, got, "last_updated: 2026-08-22") - assert.Contains(t, got, "title: 'X'", "a human-set title must still survive regeneration") - assert.Contains(t, got, "Original.") - assert.NotContains(t, got, "Renamed by the model") + // Exact frontmatter block, not Contains: the second review round found + // that a substring check doesn't prove the fields landed inside a valid + // fence, in the right order, or that the fence itself wasn't corrupted — + // two independent mutations (closeIdx targeting the wrong fence, reversed + // sort) both survived a Contains-only version of this test. + want := "---\ntitle: 'X'\nlast_updated: 2026-08-22\nsource_hash: bbbb\n---\n\n## Summary\n\nOriginal.\n" + assert.Equal(t, want, got) +} + +func TestMerge_GeneratorOwnedFieldsCreateAFrontmatterBlockWhenThePageHadNoneAtAll(t *testing.T) { + // Second review round on PR #126: when a has no frontmatter fence at all, + // the append fallback spliced raw, unfenced "key: value" lines straight + // into the body -- worse than the original bug, because the next + // parseSections call wouldn't recognize them as frontmatter (they don't + // start with `---`), so they'd be absorbed as ordinary preamble text and + // re-duplicate on every subsequent merge instead of updating in place. + existing := Page{Path: "wiki/sources/x.md", Content: "## Summary\n\nOriginal.\n"} + fresh := Page{Path: "wiki/sources/x.md", Content: `--- +last_updated: 2026-08-22 +source_hash: bbbb +--- + +## Summary + +Rewritten by the model. +`} + + got := Merge(existing, fresh).Content + + want := "---\nlast_updated: 2026-08-22\nsource_hash: bbbb\n---\n\n## Summary\n\nOriginal.\n" + assert.Equal(t, want, got) +} + +func TestMerge_GeneratorOwnedFieldsMixedReplaceAndAppend(t *testing.T) { + // a already has last_updated (replace in place) but not source_hash + // (append) -- the two code paths in the same call, together. + existing := Page{Path: "wiki/sources/x.md", Content: `--- +title: 'X' +last_updated: 2026-01-01 +--- + +## Summary + +Original. +`} + fresh := Page{Path: "wiki/sources/x.md", Content: `--- +last_updated: 2026-08-22 +source_hash: bbbb +--- + +## Summary + +Rewritten by the model. +`} + + got := Merge(existing, fresh).Content + + want := "---\ntitle: 'X'\nlast_updated: 2026-08-22\nsource_hash: bbbb\n---\n\n## Summary\n\nOriginal.\n" + assert.Equal(t, want, got) }