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

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