From 41a9112860902506b2a0276963a62fd65339b5ba Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 06:56:41 +0200 Subject: [PATCH] fix(wiki): append generator-owned frontmatter fields a page never had, not just replace Review of PR #126 found refreshOwned only replaced a generator-owned line that already existed in a's frontmatter -- it never appended one a didn't have. Every page ingested before this feature shipped has no source_hash line at all, so on its first re-ingest after deploy, the fresh hash was silently dropped: storedSourceHash reads "" forever for that page, the hash never matches, and the skip-on-unchanged feature this whole PR/issue exists to build never engages for the entire pre-existing corpus, permanently, with nothing telling anyone. Fixed by tracking the closing frontmatter fence's position while scanning a's lines, then inserting any generator-owned fields b supplied that a didn't have just before that fence -- sorted for a deterministic, byte-stable result, since map iteration order isn't. Flip-tested: forcing the old drop-silently return turns the new test red; restoring the fix turns it green. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/wiki/merge.go | 46 +++++++++++++++++++++++++-- ingestion/internal/wiki/merge_test.go | 36 +++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/ingestion/internal/wiki/merge.go b/ingestion/internal/wiki/merge.go index 31e9835..94baa7c 100644 --- a/ingestion/internal/wiki/merge.go +++ b/ingestion/internal/wiki/merge.go @@ -3,6 +3,7 @@ package wiki import ( "fmt" + "sort" "strings" ) @@ -84,6 +85,14 @@ var generatorOwned = map[string]bool{ // 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). func refreshOwned(a, b string) string { bVals := make(map[string]string) for _, line := range strings.Split(b, "\n") { @@ -97,8 +106,10 @@ func refreshOwned(a, b string) string { return a } - out := make([]string, 0, len(strings.Split(a, "\n"))) - for _, line := range strings.Split(a, "\n") { + 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] { @@ -108,9 +119,38 @@ func refreshOwned(a, b string) string { continue } } + if strings.TrimSpace(line) == "---" { + closeIdx = len(out) // the fence's position in the OUTPUT slice + } out = append(out, line) } - return strings.Join(out, "\n") + + 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)) + for k := range bVals { + remaining = append(remaining, k) + } + sort.Strings(remaining) + insert := make([]string, len(remaining)) + for i, k := range remaining { + insert[i] = 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") } // 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 1f91d62..5ba8495 100644 --- a/ingestion/internal/wiki/merge_test.go +++ b/ingestion/internal/wiki/merge_test.go @@ -214,3 +214,39 @@ Rewritten by the model. assert.Contains(t, got, "Original.", "an existing section still wins over the model's rewrite") assert.NotContains(t, got, "Renamed by the model") } + +func TestMerge_GeneratorOwnedFieldsAreAppendedWhenAPageNeverHadThemBefore(t *testing.T) { + // Found in review of PR #126: refreshOwned only replaced a generator-owned + // line that already existed in a's frontmatter — it never appended one a + // didn't have. Every page ingested before this feature shipped has no + // source_hash line at all, so on its first re-ingest after deploy, the + // fresh hash was silently dropped and the skip-on-unchanged feature this + // PR exists to build would never engage for the entire pre-existing + // corpus, permanently, with nothing telling anyone. + existing := Page{Path: "wiki/sources/x.md", Content: `--- +title: 'X' +--- + +## Summary + +Original. +`} + fresh := Page{Path: "wiki/sources/x.md", Content: `--- +title: 'Renamed by the model' +last_updated: 2026-08-22 +source_hash: bbbb +--- + +## Summary + +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") +}