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) }