Compare commits

..
Author SHA1 Message Date
mathiasandClaude Sonnet 5 43f43e4621 fix(wiki): parseFrontmatterBlock must not treat comments or indented lines as keys
CI / Lint / Test / Vet (pull_request) Successful in 40s
CI / Mirror to GitHub (pull_request) Skipped
Third review round on PR #126. Two real bugs in the rewrite from the
previous commit:

1. The comment guard the sibling api package's frontmatter parser has
   (strings.HasPrefix(trimmed, "#")) was dropped in this reimplementation.
   A frontmatter comment containing a colon got cut at the colon and
   re-rendered with collapsed internal whitespace.
2. Worse: an indented YAML block-list item containing a colon (e.g. a
   title with a subtitle inside an aliases: list) was misparsed as a
   top-level key/value pair -- strings.Cut doesn't know a line is a
   continuation just because TrimSpace(key) happens to be non-empty.
   Re-rendering then stripped the line's indentation, de-indenting it
   out of the list it belonged to. refreshOwned re-renders this block on
   essentially every non-skipped ingest, so this was the normal path,
   not an edge case.

Checked whether the sibling api/frontmatter.go has the same list-item gap:
it does (same strings.Cut-then-TrimSpace shape, no indentation check) --
not a regression introduced here, a pre-existing latent defect shared
with the package this was modeled on. Filed separately rather than fixed
here (out of scope for this PR, needs its own review).

Fixed by checking indentation BEFORE calling Cut at all -- a line
starting with a space or tab is a continuation line, full stop,
regardless of what it contains -- and restoring the comment guard
verbatim from the sibling.

Flip-tested both guards independently: removing either one turns its
corresponding new test red in isolation; the other stays green,
confirming they're independent fixes, not one masking the other.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-23 07:07:21 +02:00
mathiasandClaude Sonnet 5 874206be9d fix(wiki): rewrite refreshOwned on a parsed line list, not fence-index tracking
CI / Lint / Test / Vet (pull_request) Successful in 33s
CI / Mirror to GitHub (pull_request) Skipped
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 <noreply@anthropic.com>
2026-09-23 07:01:27 +02:00
mathiasandClaude Sonnet 5 41a9112860 fix(wiki): append generator-owned frontmatter fields a page never had, not just replace
CI / Lint / Test / Vet (pull_request) Successful in 38s
CI / Mirror to GitHub (pull_request) Skipped
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 <noreply@anthropic.com>
2026-09-23 06:56:41 +02:00
mathias a5a328fea5 feat(pipeline): skip extraction when the source has not changed (#105)
CI / Lint / Test / Vet (pull_request) Successful in 34s
CI / Mirror to GitHub (pull_request) Skipped
Ingest now stamps source_hash — sha256 of the ingested body, frontmatter
excluded — onto the source page, and compares it before doing any work. An
unchanged source returns Skipped with no LLM call at all.

This is cost and signal, not correctness: #103 already made a re-ingest
byte-identical, so a repeated run was harmless but not free — a full
extraction over every chunk to produce a result that merges to nothing.
Building this first would have put idempotency in the guard rather than in
the write, which is the wrong dependency direction.

Body-only hashing on purpose. A re-crawl that stamps a new fetch date has
not changed the source, and treating that as drift would re-extract the
whole corpus every time a crawler ran. A missing or unreadable hash always
means "extract": a missed skip costs one extraction, a wrong skip loses an
update silently, and silence is the failure mode this repo keeps paying
for.

The skip is reported (Result.Skipped plus a warning naming the hash), not
silent — a run that does nothing must say so, or "the pipeline is fine" and
"the pipeline stopped working" look identical (#107).

Storing the hash required settling who owns which frontmatter field. #103
made the on-disk block win wholesale, which protects human-set metadata
from a regenerating writer but froze fields the generator legitimately
owns. wiki.Merge now refreshes an explicit generatorOwned allowlist —
last_updated and source_hash — and keeps everything else on disk. An
allowlist rather than a carve-out, so a field nobody has thought about
stays human-owned by default. That also retires the last_updated
limitation #103 shipped with.

Refs #105, #103, #107, brain#21.
2026-09-22 23:45:25 +02:00
5 changed files with 6 additions and 190 deletions
-18
View File
@@ -104,24 +104,6 @@ Two fields are **generator-owned** and do get refreshed on a revise:
`last_updated` and `source_hash`. Everything else on disk wins. The boundary is
an explicit allowlist, so a field nobody has thought about stays human-owned.
### Disagreement is reported, not resolved
When the fresh extraction produces a *different version* of a section that
already exists, the on-disk version still wins — but the run now says so:
```
wiki/sources/shape-up.md: source now disagrees on section "Summary" — on-disk version kept
```
That line lands in `log.md` with the rest of the run's warnings. Without it, "the
source is unchanged" and "the source changed its mind and we ignored it" look
identical from the outside.
This is deliberately **textual** disagreement, not semantic contradiction: cheap,
deterministic, no model on the write path. Deciding whether a disagreement is a
genuine contradiction stays with the Judge-gated `brain-enrich` pass, which is
approval-gated by design. Detection here, adjudication there.
## Re-ingesting an unchanged source is skipped
Ingest stores `source_hash` — sha256 of the ingested **body**, frontmatter
+5 -14
View File
@@ -117,18 +117,10 @@ func buildAndWrite(rawPages []RawPage, sourceSlug, date, brainDir, source string
if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil {
return Result{}, fmt.Errorf("mkdir for %s: %w", page.Path, err)
}
content, conflicts, err := reviseOrCreate(dest, page)
content, err := reviseOrCreate(dest, page)
if err != nil {
return Result{}, err
}
for _, heading := range conflicts {
// Reported, not resolved. The on-disk version stands; whether
// the disagreement is a genuine contradiction is the
// Judge-gated enrich pass's call, not the write path's.
warnings = append(warnings, fmt.Sprintf(
"%s: source now disagrees on section %q — on-disk version kept",
page.Path, heading))
}
if err := os.WriteFile(dest, []byte(content), 0o644); err != nil {
return Result{}, fmt.Errorf("write %s: %w", page.Path, err)
}
@@ -206,16 +198,15 @@ func storedSourceHash(brainDir, sourceSlug string) string {
// its first-ingest date on a revised page. Left deliberately — the alternative
// is this function editing frontmatter it does not own, and no lint reads that
// field today.
func reviseOrCreate(dest string, page wiki.Page) (string, []string, error) {
func reviseOrCreate(dest string, page wiki.Page) (string, error) {
existing, err := os.ReadFile(dest)
switch {
case os.IsNotExist(err):
return page.Content, nil, nil
return page.Content, nil
case err != nil:
return "", nil, fmt.Errorf("read existing %s: %w", page.Path, err)
return "", fmt.Errorf("read existing %s: %w", page.Path, err)
}
merged, conflicts := wiki.MergeReport(wiki.Page{Path: page.Path, Content: string(existing)}, page)
return merged.Content, conflicts, nil
return wiki.Merge(wiki.Page{Path: page.Path, Content: string(existing)}, page).Content, nil
}
// mergeAll deduplicates pages by path, merging content from later occurrences.
@@ -8,7 +8,6 @@ import (
"net/http/httptest"
"os"
"path/filepath"
"strings"
"testing"
"time"
@@ -294,34 +293,3 @@ func TestRun_FrontmatterOnlyChangeIsNotDrift(t *testing.T) {
assert.Equal(t, 1, calls, "only the body counts as the source")
}
func TestRunRaw_ReportsWhenTheSourceDisagreesWithWhatIsOnDisk(t *testing.T) {
// The on-disk version still wins — that is what makes unattended ingest
// safe. But the run must say that the source now claims something else,
// or a changed source is indistinguishable from an unchanged one.
dir := t.TempDir()
page := filepath.Join(dir, "wiki", "sources", "shape-up.md")
require.NoError(t, os.MkdirAll(filepath.Dir(page), 0o755))
require.NoError(t, os.WriteFile(page, []byte(`---
title: 'Shape Up'
---
## Summary
Appetite is fixed, scope is variable.
`), 0o644))
res, err := RunRaw(dir, "shape-up", []RawPage{{
Title: "Shape Up", Type: "source", Subtype: "book", Domain: "product",
Content: "## Summary\n\nAppetite is negotiable and scope is fixed.\n",
}}, false)
require.NoError(t, err)
got, err := os.ReadFile(page)
require.NoError(t, err)
assert.Contains(t, string(got), "Appetite is fixed", "on-disk version wins")
joined := strings.Join(res.Warnings, " ")
assert.Contains(t, joined, "Summary", "the disagreeing section must be named")
assert.Contains(t, joined, "shape-up", "and the page it happened on")
}
+1 -44
View File
@@ -37,33 +37,10 @@ type section struct {
// 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.
// Merge discards any disagreement silently; prefer MergeReport where the
// caller can act on it.
func Merge(a, b Page) Page {
page, _ := MergeReport(a, b)
return page
}
// MergeReport is Merge, plus the headings of sections where b carried a
// different version of a section a already had — and lost.
//
// That loss is the property which makes ingest safe to run unattended (#103):
// a regenerating writer can add to a note but never rewrite it. But the
// discarded text is also a signal, and until now it went in the bin: the source
// now says something different about a section we already hold. Nobody could
// tell "the source is unchanged" from "the source changed its mind and we
// ignored it".
//
// Deliberately NOT a semantic contradiction check. This reports textual
// disagreement, which is cheap, deterministic, and needs no model on the write
// path. Deciding whether a disagreement is a genuine contradiction stays with
// the Judge-gated enrich pass that already exists (brain-gardener#175), which
// is approval-gated by design — detection here, adjudication there.
func MergeReport(a, b Page) (Page, []string) {
fmA, preambleA, secsA := parseSections(a.Content)
fmB, _, secsB := parseSections(b.Content)
var conflicts []string
idx := make(map[string]int, len(secsA))
for i, s := range secsA {
idx[s.heading] = i
@@ -82,15 +59,10 @@ func MergeReport(a, b Page) (Page, []string) {
secsA[i].content = mergeBullets(sA.content, sB.content)
case appendSections[sB.heading]:
secsA[i].content = appendNew(sA.content, sB.content)
default:
// a wins — but say that b disagreed, rather than dropping it mute.
if canonicalBlock(sA.content) != canonicalBlock(sB.content) {
conflicts = append(conflicts, sB.heading)
}
}
}
return Page{Path: a.Path, Content: rebuildContent(refreshOwned(fmA, fmB), preambleA, secsA)}, conflicts
return Page{Path: a.Path, Content: rebuildContent(refreshOwned(fmA, fmB), preambleA, secsA)}
}
// generatorOwned names the frontmatter fields the ingest pipeline may refresh
@@ -329,21 +301,6 @@ func canonicalLine(line string) string {
return strings.Join(strings.Fields(line), " ")
}
// canonicalBlock applies canonicalLine per line of a multi-line section body
// — the same whitespace-insensitivity appendNew/mergeBullets already give a
// single line, extended to a whole section for MergeReport's disagreement
// check (found in review, PR #129: comparing raw strings.TrimSpace only
// tolerated leading/trailing whitespace, not a re-extraction's mid-line
// spacing, which every other comparison in this file already tolerates).
func canonicalBlock(block string) string {
lines := strings.Split(block, "\n")
out := make([]string, len(lines))
for i, l := range lines {
out[i] = canonicalLine(l)
}
return strings.Join(out, "\n")
}
// appendNew concatenates the lines of b that a does not already carry.
//
// Append sections used to concatenate wholesale, which is correct exactly once.
-82
View File
@@ -332,85 +332,3 @@ func TestRefreshOwned_IndentedListItemsWithColonsAreNotMisparsedAsKeys(t *testin
got := refreshOwned(a, b)
assert.Equal(t, "---\naliases:\n - 'Chapter 3: The Reckoning'\ntitle: 'X'\nsource_hash: bbbb\n---\n", got)
}
func TestMerge_ReportsSectionsWhereTheFreshVersionWasDiscarded(t *testing.T) {
// The asymmetry that makes ingest safe (on-disk wins) also throws away a
// real signal: the source now says something different about a section we
// already have. Today that disagreement is silently discarded, so nobody
// can tell "the source is unchanged" from "the source changed its mind and
// we ignored it".
existing := Page{Path: "wiki/sources/x.md", Content: `---
title: 'X'
---
## Summary
Appetite is fixed, scope is variable.
## Method
Six-week cycles.
`}
fresh := Page{Path: "wiki/sources/x.md", Content: `---
title: 'X'
---
## Summary
Appetite is negotiable and scope is fixed.
## Method
Six-week cycles.
`}
got, conflicts := MergeReport(existing, fresh)
assert.Contains(t, got.Content, "Appetite is fixed", "on-disk version still wins")
assert.NotContains(t, got.Content, "negotiable")
assert.Equal(t, []string{"Summary"}, conflicts,
"only the section that actually disagreed is reported")
}
func TestMerge_ReportsNothingWhenTheFreshVersionAgrees(t *testing.T) {
page := Page{Path: "wiki/sources/x.md", Content: `---
title: 'X'
---
## Summary
Appetite is fixed, scope is variable.
`}
_, conflicts := MergeReport(page, page)
assert.Empty(t, conflicts, "identical content is not a disagreement")
}
func TestMerge_ReportsNothingForInternalWhitespaceVariantsOnly(t *testing.T) {
// Found in review of PR #129: the conflict comparison used raw
// strings.TrimSpace, which only tolerates LEADING/TRAILING whitespace —
// unlike appendNew/mergeBullets, which already canonicalize internal
// whitespace runs too (#103: a re-extraction's spacing isn't guaranteed
// byte-stable). A mid-line double-space from re-extraction would
// false-positive as a "disagreement" here even though every other
// whitespace-sensitive comparison in this file already tolerates it.
existing := Page{Path: "wiki/sources/x.md", Content: `---
title: 'X'
---
## Summary
Appetite is fixed, scope is variable.
`}
fresh := Page{Path: "wiki/sources/x.md", Content: `---
title: 'X'
---
## Summary
Appetite is fixed, scope is variable.
`}
_, conflicts := MergeReport(existing, fresh)
assert.Empty(t, conflicts, "internal whitespace differences alone are not a disagreement")
}