feat(pipeline): ingest revises the existing page instead of clobbering it (#103)

fix(wiki): give the preamble its own field instead of overloading heading=="" (#103)

Round 3 review: verified structurally that the preamble is no longer in the sections slice or the idx map, so it cannot collide with a degenerate `## ` heading. Flip-tested by reverting to the heading=="" model and confirming TestMerge_DegenerateEmptyHeadingDoesNotCollideWithPreamble goes red. Adversarial pass (multiple degenerate headings, frontmatter-only page, unicode around the ## boundary, 3x self-merge idempotency with preamble+degenerate) all held. CI green on 09aeaa9 (run 7820).

Co-authored-by: Mathias <mthbqv@gmail.com>

ingestion/internal/wiki/merge.go

@@ -29,12 +29,16 @@ type section struct {
 }
 
 // Merge combines two Page values with the same path.
-// Frontmatter is taken from a. Sections are merged by strategy:
-// bullet sections union unique lines, append sections concatenate,
-// all others keep a's version. Sections in b not present in a are appended.
+// Frontmatter is taken from a. The preamble (content before the first ##
+// heading) is taken from a — same "on-disk wins" default every unlisted
+// named section gets, kept as a separate field rather than a section with
+// an empty heading so it can never collide with one (see parseSections).
+// Sections are merged by strategy: bullet sections union unique lines,
+// append sections concatenate, all others keep a's version. Sections in b
+// not present in a are appended.
 func Merge(a, b Page) Page {
-	fmA, secsA := parseSections(a.Content)
-	_, secsB := parseSections(b.Content)
+	fmA, preambleA, secsA := parseSections(a.Content)
+	_, _, secsB := parseSections(b.Content)
 
 	idx := make(map[string]int, len(secsA))
 	for i, s := range secsA {
@@ -53,14 +57,24 @@ func Merge(a, b Page) Page {
 		case bulletSections[sB.heading]:
 			secsA[i].content = mergeBullets(sA.content, sB.content)
 		case appendSections[sB.heading]:
-			secsA[i].content = strings.TrimRight(sA.content, "\n") + "\n\n" + strings.TrimLeft(sB.content, "\n")
+			secsA[i].content = appendNew(sA.content, sB.content)
 		}
 	}
 
-	return Page{Path: a.Path, Content: rebuildContent(fmA, secsA)}
+	return Page{Path: a.Path, Content: rebuildContent(fmA, preambleA, secsA)}
 }
 
-func parseSections(markdown string) (frontmatter string, sections []section) {
+// parseSections splits markdown into its frontmatter, its preamble (content
+// before the first `## ` heading), and its `## `-headed sections.
+//
+// The preamble is returned separately rather than as a section with an
+// empty heading — a section keyed by heading=="" would collide with a
+// literal degenerate `## ` (marker, no title text; strings.TrimPrefix also
+// yields ""), which is a distinct, addressable section a page can genuinely
+// have, not the same thing as "no heading yet." Two entries sharing one key
+// in Merge's idx map means one of them silently loses lookup visibility —
+// found in review as real, reproducible data loss, not a hypothetical.
+func parseSections(markdown string) (frontmatter, preamble string, sections []section) {
 	lines := strings.Split(markdown, "\n")
 	i := 0
 
@@ -78,6 +92,22 @@ func parseSections(markdown string) (frontmatter string, sections []section) {
 		frontmatter = fmt.Sprintf("---\n%s\n---\n", strings.Join(fmLines, "\n"))
 	}
 
+	// Everything before the first `## ` heading — an intro paragraph, or, if
+	// the page has no headings at all, the entire body — is ordinary content,
+	// not something this parser is entitled to drop (hyperguild#103 review:
+	// dropping it meant Merge, one caller up, would silently replace the
+	// whole on-disk body with the fresh extraction whenever a page didn't
+	// follow the ## convention — exactly the class of bug this file exists
+	// to prevent).
+	var preambleLines []string
+	for ; i < len(lines); i++ {
+		if strings.HasPrefix(lines[i], "## ") {
+			break
+		}
+		preambleLines = append(preambleLines, lines[i])
+	}
+	preamble = strings.Join(preambleLines, "\n")
+
 	var cur *section
 	for ; i < len(lines); i++ {
 		line := lines[i]
@@ -96,24 +126,85 @@ func parseSections(markdown string) (frontmatter string, sections []section) {
 	return
 }
 
-func rebuildContent(frontmatter string, sections []section) string {
+// rebuildContent serialises sections back into a page.
+//
+// Each section body is normalised to exactly one trailing newline and no
+// leading blank line. Without that, a section keeps whatever blank lines it was
+// parsed with and this function adds its own, so every merge pass grows the page
+// by a blank line — invisible in a diff viewer, fatal to byte-stability. Compile-on-ingest
+// merges on every run, so "merging changes nothing when nothing changed" has to
+// hold at the byte level, not just the content level.
+func rebuildContent(frontmatter, preamble string, sections []section) string {
 	var sb strings.Builder
 	sb.WriteString(frontmatter)
+	if body := strings.Trim(preamble, "\n"); body != "" {
+		fmt.Fprintf(&sb, "\n%s\n", body)
+	}
 	for _, sec := range sections {
-		fmt.Fprintf(&sb, "\n## %s\n\n%s", sec.heading, sec.content)
+		body := strings.Trim(sec.content, "\n")
+		fmt.Fprintf(&sb, "\n## %s\n\n%s\n", sec.heading, body)
 	}
 	return sb.String()
 }
 
+// canonicalLine collapses a line to its comparison key: internal whitespace
+// runs (a re-extraction's bullet spacing, tabs vs spaces, trailing spaces)
+// must not make the same logical line look new. strings.Fields already
+// trims and splits on any whitespace run, so joining it back with single
+// spaces is exactly "same words, same order, whitespace-insensitive."
+func canonicalLine(line string) string {
+	return strings.Join(strings.Fields(line), " ")
+}
+
+// appendNew concatenates the lines of b that a does not already carry.
+//
+// Append sections used to concatenate wholesale, which is correct exactly once.
+// Compile-on-ingest merges a freshly extracted page into the one already on
+// disk, so an unchanged source re-ingested would grow "Open Questions" and
+// "Updates" on every run until the page is unreadable — and the drift guard
+// that would otherwise catch it (hashing the source body) does not exist yet,
+// so idempotency has to hold here rather than upstream.
+//
+// Line-level rather than block-level: the common case is a re-ingest carrying
+// one genuinely new line alongside several it already has. A block-level
+// "skip if identical" would duplicate the old lines every time that happened.
+//
+// The cost is that a line repeated verbatim on purpose collapses to one. For
+// these sections — questions, updates, evolving notes — a repeat is noise
+// rather than signal, and the failure is conservative: the page stops growing
+// instead of growing wrongly.
+func appendNew(a, b string) string {
+	seen := make(map[string]bool)
+	for _, line := range strings.Split(a, "\n") {
+		if c := canonicalLine(line); c != "" {
+			seen[c] = true
+		}
+	}
+
+	var add []string
+	for _, line := range strings.Split(b, "\n") {
+		c := canonicalLine(line)
+		if c == "" || seen[c] {
+			continue
+		}
+		seen[c] = true
+		add = append(add, line)
+	}
+	if len(add) == 0 {
+		return a // nothing new — leave the section byte-identical
+	}
+	return strings.TrimRight(a, "\n") + "\n\n" + strings.Join(add, "\n") + "\n"
+}
+
 func mergeBullets(a, b string) string {
 	seen := make(map[string]bool)
 	var lines []string
 	for _, line := range strings.Split(a+b, "\n") {
-		trimmed := strings.TrimSpace(line)
-		if trimmed == "" || seen[trimmed] {
+		c := canonicalLine(line)
+		if c == "" || seen[c] {
 			continue
 		}
-		seen[trimmed] = true
+		seen[c] = true
 		lines = append(lines, line)
 	}
 	return strings.Join(lines, "\n") + "\n"

ingestion/internal/wiki/merge_test.go

@@ -53,3 +53,124 @@ func TestMerge_KeepsFrontmatterFromA(t *testing.T) {
 	assert.Contains(t, got.Content, "title: A")
 	assert.NotContains(t, got.Content, "title: B")
 }
+
+// --- hyperguild#103 review: a page with content outside any `## ` section ---
+// must never lose that content on merge. parseSections used to only capture
+// text between `## ` headings — anything before the first heading (a common,
+// ordinary shape for a hand-written note: an intro paragraph before the first
+// section) or, worse, an entire page with no `## ` headings at all (a legacy
+// note, a stub, any hand-edit that doesn't follow the section convention)
+// vanished silently. Merge would then fill the page entirely from the fresh
+// extraction — the opposite of "never silently rewrite what a human put
+// there," the guarantee this whole file exists to provide.
+
+func TestMerge_PreservesOnDiskContentWithNoSectionHeadings(t *testing.T) {
+	a := Page{Path: "wiki/x.md", Content: "---\ntitle: X\n---\n\nJust some free text a human wrote. No section headings at all.\n"}
+	b := Page{Path: "wiki/x.md", Content: "---\ntitle: X\n---\n\n## Definition\n\nFresh extraction's take.\n"}
+
+	got := Merge(a, b)
+	assert.Contains(t, got.Content, "Just some free text a human wrote",
+		"on-disk content with no ## headings must survive the merge, not be replaced wholesale by the fresh extraction")
+}
+
+func TestMerge_DegenerateEmptyHeadingDoesNotCollideWithPreamble(t *testing.T) {
+	// Second review pass on #103: representing the preamble as a section with
+	// heading=="" collides with a literal `## ` (marker, no title text) —
+	// strings.TrimPrefix("## ", "## ") is also "". `a` has only a preamble
+	// (no degenerate heading of its own); `b` introduces a genuinely new
+	// degenerate `## ` section. Because both key to "", Merge's idx thinks
+	// b's section "already exists" in a (it's really just a's unrelated
+	// preamble) and never adds it — b's section vanishes from the output
+	// entirely, not merged, not kept, not appended.
+	a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\nAn intro paragraph.\n\n## Definition\n\nA thing.\n"}
+	b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## \n\nA genuinely new degenerate section's body.\n\n## Definition\n\nA thing.\n"}
+
+	got := Merge(a, b)
+	assert.Contains(t, got.Content, "An intro paragraph.", "a's own preamble must survive")
+	assert.Contains(t, got.Content, "A genuinely new degenerate section's body.",
+		"b's degenerate section is new content, not a collision with a's unrelated preamble — it must not be silently dropped")
+}
+
+func TestMerge_PreservesPreambleBeforeFirstHeading(t *testing.T) {
+	a := Page{Path: "wiki/x.md", Content: "---\ntitle: X\n---\n\nAn intro paragraph before the first heading.\n\n## Definition\n\nA thing.\n"}
+	b := Page{Path: "wiki/x.md", Content: "---\ntitle: X\n---\n\n## Definition\n\nA thing.\n"}
+
+	got := Merge(a, b)
+	assert.Contains(t, got.Content, "An intro paragraph before the first heading",
+		"text before the first ## heading is ordinary page content, not something the parser is entitled to drop")
+}
+
+// --- hyperguild#103 review: dedup must be whitespace-insensitive -----------
+// Page content is produced by an LLM per run, so byte-identical re-extraction
+// of "the same" logical line is not guaranteed — different bullet spacing or
+// trailing whitespace on an otherwise-identical line must still dedup, or
+// idempotency only holds when the extractor happens to reproduce whitespace
+// exactly, which is not a real guarantee.
+
+func TestMerge_AppendSectionDedupsAcrossWhitespaceVariants(t *testing.T) {
+	a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Open Questions\n\n- Does this converge?\n"}
+	b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Open Questions\n\n-   Does this converge?  \n"}
+
+	got := Merge(a, b)
+	assert.Equal(t, 1, strings.Count(got.Content, "Does this converge?"),
+		"same logical line with different whitespace must dedup, not duplicate")
+}
+
+func TestMerge_BulletSectionDedupsAcrossWhitespaceVariants(t *testing.T) {
+	a := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Related Concepts\n\n- [[bar|Bar]]\n"}
+	b := Page{Path: "p.md", Content: "---\ntitle: X\n---\n\n## Related Concepts\n\n-  [[bar|Bar]]\n"}
+
+	got := Merge(a, b)
+	assert.Equal(t, 1, strings.Count(got.Content, "[[bar|Bar]]"),
+		"same logical bullet with different whitespace must dedup, not duplicate")
+}
+
+// --- brain#21/B1: merging must be safe to run again -------------------------
+
+func TestMerge_AppendSectionDoesNotDuplicateContentAlreadyPresent(t *testing.T) {
+	// Compile-on-ingest merges a freshly extracted page into the one already
+	// on disk. Bullet sections union, so they are naturally idempotent — but
+	// append sections concatenate, so re-ingesting an unchanged source would
+	// grow "Open Questions" on every run until the page is unreadable.
+	page := Page{Path: "wiki/sources/x.md", Content: `---
+title: X
+---
+
+## Open Questions
+
+- Does this converge?
+`}
+
+	once := Merge(page, page)
+	twice := Merge(once, page)
+
+	assert.Equal(t, 1, strings.Count(once.Content, "Does this converge?"),
+		"merging a page with itself must not duplicate an append section")
+	assert.Equal(t, once.Content, twice.Content,
+		"merge must be idempotent — a third pass may not keep growing the page")
+}
+
+func TestMerge_AppendSectionStillAccumulatesGenuinelyNewContent(t *testing.T) {
+	// The dedup must not turn append sections into keep-a-only sections:
+	// new material is exactly what compile-on-ingest exists to add.
+	existing := Page{Path: "wiki/sources/x.md", Content: `---
+title: X
+---
+
+## Open Questions
+
+- Does this converge?
+`}
+	fresh := Page{Path: "wiki/sources/x.md", Content: `---
+title: X
+---
+
+## Open Questions
+
+- What happens under load?
+`}
+
+	got := Merge(existing, fresh).Content
+	assert.Contains(t, got, "Does this converge?", "existing question must survive")
+	assert.Contains(t, got, "What happens under load?", "new question must be added")
+}