diff --git a/README.md b/README.md index ebb6265..1c1d545 100644 --- a/README.md +++ b/README.md @@ -76,6 +76,46 @@ that prompted the question is answered by naming it here instead. Full reasoning, including the reversal trigger: brain `wiki/hyperguild/decisions/brain-mcp-stays-in-ingestion-fix-discoverability-by-name`. +## Ingest revises, it does not overwrite + +Re-ingesting a source **merges into the page already on disk** rather than +replacing it. Previously the write was unconditional, so a second ingest of the +same source discarded everything accumulated since — including hand-written +sections. + +The merge is deliberately asymmetric in the existing page's favour: + +| On the page | In the fresh extraction | Result | +|---|---|---| +| a section | the same section | **the on-disk version wins** | +| — | a new section | added | +| a bullet list (`Sources`, `Key Claims`, …) | more bullets | unioned, no duplicates | +| an append section (`Open Questions`, `Updates`, …) | more lines | only genuinely new lines appended | +| frontmatter | frontmatter | **the on-disk frontmatter wins** | + +So an extraction can add to a note but never silently rewrite what a human put +there, which is what makes it safe to run on every ingest rather than behind a +review gate. + +Re-ingesting unchanged content is **byte-identical** — no diff, so brain-sync +does not commit noise that hides real edits. + +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. + +## Re-ingesting an unchanged source is skipped + +Ingest stores `source_hash` — sha256 of the ingested **body**, frontmatter +excluded — on the source page. A later ingest of the same source compares it +first and skips extraction entirely when it matches, reporting the skip rather +than doing it silently. + +Body-only is deliberate: a re-crawl that stamps a new fetch date has not changed +the source, and treating that as drift would re-extract the 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. + ## Phase 1 tools (available now) | Tool | What it does | diff --git a/ingestion/internal/pipeline/build.go b/ingestion/internal/pipeline/build.go index c59ba8e..4ff005a 100644 --- a/ingestion/internal/pipeline/build.go +++ b/ingestion/internal/pipeline/build.go @@ -76,6 +76,9 @@ func buildFrontmatter(rp RawPage, date string) string { } fmt.Fprintf(&sb, "date_ingested: %s\n", date) fmt.Fprintf(&sb, "last_updated: %s\n", date) + if rp.SourceHash != "" { + fmt.Fprintf(&sb, "source_hash: %s\n", rp.SourceHash) + } if rp.Source != "" { fmt.Fprintf(&sb, "source: %s\n", yamlScalar(rp.Source)) } diff --git a/ingestion/internal/pipeline/parse.go b/ingestion/internal/pipeline/parse.go index 6b2afc5..43615e1 100644 --- a/ingestion/internal/pipeline/parse.go +++ b/ingestion/internal/pipeline/parse.go @@ -20,9 +20,13 @@ type RawPage struct { // ingested content's own frontmatter (see parseContentFrontmatter) — never // set by the LLM. json:"-" keeps them immune to same-named keys the LLM // might emit. Only meaningful for Type == "source". - Source string `json:"-"` - Author string `json:"-"` - Published string `json:"-"` + // SourceHash is sha256 of the ingested BODY (frontmatter excluded), stamped + // so the next ingest of the same source can tell "unchanged" from "edited" + // without re-running extraction (#105). + SourceHash string `json:"-"` + Source string `json:"-"` + Author string `json:"-"` + Published string `json:"-"` } // ParseRawPages parses LLM output as a JSON array of RawPage objects. @@ -112,6 +116,11 @@ type sourceMeta struct { Source string Author string Published string + // BodyHash is sha256 of the content with any leading frontmatter removed. + // Body-only 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 (#105). + BodyHash string } // parseContentFrontmatter extracts source/author/published from a leading @@ -154,6 +163,7 @@ func applySourceMeta(pages []RawPage, meta sourceMeta) { if pages[i].Type != "source" { continue } + pages[i].SourceHash = meta.BodyHash pages[i].Source = meta.Source pages[i].Author = meta.Author pages[i].Published = meta.Published diff --git a/ingestion/internal/pipeline/pipeline.go b/ingestion/internal/pipeline/pipeline.go index 0385e7a..f7c3487 100644 --- a/ingestion/internal/pipeline/pipeline.go +++ b/ingestion/internal/pipeline/pipeline.go @@ -3,6 +3,8 @@ package pipeline import ( "context" + "crypto/sha256" + "encoding/hex" "fmt" "os" "path/filepath" @@ -26,6 +28,11 @@ type Config struct { type Result struct { Pages []string // relative paths written (or would-be written in dry-run) Warnings []string + // Skipped reports that the source was byte-identical to the last ingest, + // so no extraction ran and nothing was written. Reported rather than + // silent: a run that does nothing must say so, or "the pipeline is fine" + // and "the pipeline stopped working" look the same (#105, #107). + Skipped bool } // Run ingests content and writes structured wiki pages to brainDir/wiki/. @@ -43,6 +50,18 @@ func Run(ctx context.Context, cfg Config, brainDir, content, source string, dryR sourceSlug := wiki.Slug(source) date := time.Now().UTC().Format("2006-01-02") + + // Drift check BEFORE any extraction: an unchanged source costs a full LLM + // pass over every chunk to produce a result that merges to nothing (#105). + // The merge is already idempotent (#103), so this is cost and signal, not + // correctness — which is why it is safe for it to be an early return. + bodyHash := hashBody(content) + if !dryRun && bodyHash != "" && bodyHash == storedSourceHash(brainDir, sourceSlug) { + return Result{Skipped: true, Warnings: []string{ + fmt.Sprintf("source %q unchanged since last ingest (hash %s) — extraction skipped", source, bodyHash[:12]), + }}, nil + } + chunks := Chunk(content, cfg.ChunkSize) var allRaw []RawPage @@ -59,7 +78,9 @@ func Run(ctx context.Context, cfg Config, brainDir, content, source string, dryR allWarnings = append(allWarnings, warnings...) } - applySourceMeta(allRaw, parseContentFrontmatter(content)) + meta := parseContentFrontmatter(content) + meta.BodyHash = bodyHash + applySourceMeta(allRaw, meta) return buildAndWrite(allRaw, sourceSlug, date, brainDir, source, inventory, allWarnings, dryRun) } @@ -96,7 +117,11 @@ 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) } - if err := os.WriteFile(dest, []byte(page.Content), 0o644); err != nil { + content, err := reviseOrCreate(dest, page) + if err != nil { + return Result{}, err + } + if err := os.WriteFile(dest, []byte(content), 0o644); err != nil { return Result{}, fmt.Errorf("write %s: %w", page.Path, err) } } @@ -115,6 +140,75 @@ func buildAndWrite(rawPages []RawPage, sourceSlug, date, brainDir, source string return Result{Pages: written, Warnings: warnings}, nil } +// hashBody returns sha256 of content with any leading frontmatter block +// removed, or "" for empty content. +// +// Body-only 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 corpus every time +// a crawler ran (#105). +func hashBody(content string) string { + body := content + if strings.HasPrefix(body, "---\n") { + if _, rest, found := strings.Cut(body[4:], "\n---"); found { + body = strings.TrimPrefix(rest, "\n") + } + } + if strings.TrimSpace(body) == "" { + return "" + } + sum := sha256.Sum256([]byte(body)) + return hex.EncodeToString(sum[:]) +} + +// storedSourceHash reads source_hash from the source page a previous ingest +// wrote, or "" when there is no such page or no such field. +// +// Absence always means "extract". That is the fail-safe direction: a missed +// skip costs one extraction, while a wrong skip loses an update silently — and +// silence is the failure mode this repo keeps paying for. +func storedSourceHash(brainDir, sourceSlug string) string { + raw, err := os.ReadFile(filepath.Join(brainDir, "wiki", "sources", sourceSlug+".md")) + if err != nil { + return "" + } + for _, line := range strings.Split(string(raw), "\n") { + if key, value, found := strings.Cut(line, ":"); found && strings.TrimSpace(key) == "source_hash" { + return strings.TrimSpace(value) + } + } + return "" +} + +// reviseOrCreate returns the bytes to write for one extracted page: merged into +// whatever is already on disk, or the page as-is when the path is new. +// +// This is the compile half of compile-on-ingest. Before it, the write was an +// unconditional os.WriteFile, so re-ingesting a source replaced its page +// wholesale and discarded everything accumulated since — including human edits +// (hyperguild#43: "re-ingest overwrites wholesale"). +// +// The merge is deliberately asymmetric in the existing page's favour: +// wiki.Merge keeps the on-disk frontmatter and the on-disk version of any +// section both sides carry, adds sections only the extraction produced, and +// unions bullet lists. So a freshly extracted page can add to a note but never +// silently rewrite what a human put there — which is what makes it safe to run +// this on every ingest rather than behind a review gate. +// +// Known limitation: because the on-disk frontmatter wins, `last_updated` keeps +// 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, error) { + existing, err := os.ReadFile(dest) + switch { + case os.IsNotExist(err): + return page.Content, nil + case err != nil: + return "", fmt.Errorf("read existing %s: %w", page.Path, err) + } + return wiki.Merge(wiki.Page{Path: page.Path, Content: string(existing)}, page).Content, nil +} + // mergeAll deduplicates pages by path, merging content from later occurrences. func mergeAll(pages []wiki.Page) []wiki.Page { order := make([]string, 0, len(pages)) diff --git a/ingestion/internal/pipeline/pipeline_test.go b/ingestion/internal/pipeline/pipeline_test.go index 92c0be7..d208fe3 100644 --- a/ingestion/internal/pipeline/pipeline_test.go +++ b/ingestion/internal/pipeline/pipeline_test.go @@ -171,3 +171,125 @@ func mustJSON(v any) string { } return string(b) } + +// --- B1: compile-on-ingest — revise the existing page, never clobber it ----- + +func TestRunRaw_MergesIntoAnExistingPageInsteadOfOverwritingIt(t *testing.T) { + // Before this, buildAndWrite did an unconditional os.WriteFile: re-ingesting + // a source replaced its page wholesale, discarding anything accumulated + // since — including human edits. hyperguild#43 recorded the symptom as + // "re-ingest overwrites wholesale". + 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 + +A book about shaping product work. + +## My Take + +Read this before the next planning cycle. +`), 0o644)) + + _, err := RunRaw(dir, "shape-up", []RawPage{{ + Title: "Shape Up", Type: "source", Subtype: "book", Domain: "product-strategy", + Content: "## Summary\n\nA book about shaping product work.\n\n## Key Claims\n\n- Appetite beats estimation.\n", + }}, false) + require.NoError(t, err) + + got, err := os.ReadFile(page) + require.NoError(t, err) + s := string(got) + + assert.Contains(t, s, "## My Take", "a section the extraction never saw must survive the merge") + assert.Contains(t, s, "Read this before the next planning cycle.") + assert.Contains(t, s, "Appetite beats estimation.", "newly extracted material must still land") +} + +func TestRunRaw_ReIngestingUnchangedContentIsByteIdentical(t *testing.T) { + // Compile-on-ingest runs on every ingest, so "nothing changed" must produce + // no diff at all. Otherwise every re-ingest dirties the tree, brain-sync + // commits it, and the git history fills with noise that hides real edits. + dir := t.TempDir() + raw := []RawPage{{ + Title: "Shape Up", Type: "source", Subtype: "book", Domain: "product-strategy", + Content: "## Summary\n\nA book about shaping product work.\n\n## Open Questions\n\n- Does appetite scale?\n", + }} + + _, err := RunRaw(dir, "shape-up", raw, false) + require.NoError(t, err) + first, err := os.ReadFile(filepath.Join(dir, "wiki", "sources", "shape-up.md")) + require.NoError(t, err) + + _, err = RunRaw(dir, "shape-up", raw, false) + require.NoError(t, err) + second, err := os.ReadFile(filepath.Join(dir, "wiki", "sources", "shape-up.md")) + require.NoError(t, err) + + assert.Equal(t, string(first), string(second), + "re-ingesting unchanged content must leave the page byte-identical") +} + +// --- #105: source drift — an unchanged re-ingest must not re-extract -------- + +func TestRun_SkipsExtractionWhenTheSourceIsUnchanged(t *testing.T) { + // #103 made a re-ingest harmless (byte-identical merge). It is still not + // free: the full LLM extraction runs over every chunk to produce a result + // that merges to nothing. This is the cost half. + dir := t.TempDir() + var calls int + cfg := Config{Complete: func(_ context.Context, _, _ string) (string, error) { + calls++ + return `[{"title":"Shape Up","type":"source","subtype":"book","domain":"product","content":"## Summary\n\nA book.\n"}]`, nil + }} + + content := "---\nsource: https://example.com/shape-up\n---\n\nThe body of the article.\n" + + first, err := Run(context.Background(), cfg, dir, content, "shape-up", false) + require.NoError(t, err) + require.Equal(t, 1, calls, "first ingest must extract") + assert.False(t, first.Skipped) + + second, err := Run(context.Background(), cfg, dir, content, "shape-up", false) + require.NoError(t, err) + assert.Equal(t, 1, calls, "unchanged content must not be extracted again") + assert.True(t, second.Skipped, "the skip must be reported, not silent") +} + +func TestRun_ReExtractsWhenTheSourceBodyChanged(t *testing.T) { + dir := t.TempDir() + var calls int + cfg := Config{Complete: func(_ context.Context, _, _ string) (string, error) { + calls++ + return `[{"title":"Shape Up","type":"source","subtype":"book","domain":"product","content":"## Summary\n\nA book.\n"}]`, nil + }} + + _, err := Run(context.Background(), cfg, dir, "---\nsource: x\n---\n\nversion one\n", "shape-up", false) + require.NoError(t, err) + _, err = Run(context.Background(), cfg, dir, "---\nsource: x\n---\n\nversion two — the source was edited\n", "shape-up", false) + require.NoError(t, err) + + assert.Equal(t, 2, calls, "changed content must be re-extracted") +} + +func TestRun_FrontmatterOnlyChangeIsNotDrift(t *testing.T) { + // The hash covers the body, so re-fetching a page whose metadata moved + // (a re-crawl stamping a new date) is not treated as the source changing. + dir := t.TempDir() + var calls int + cfg := Config{Complete: func(_ context.Context, _, _ string) (string, error) { + calls++ + return `[{"title":"Shape Up","type":"source","subtype":"book","domain":"product","content":"## Summary\n\nA book.\n"}]`, nil + }} + + _, err := Run(context.Background(), cfg, dir, "---\nsource: x\nfetched: monday\n---\n\nsame body\n", "shape-up", false) + require.NoError(t, err) + _, err = Run(context.Background(), cfg, dir, "---\nsource: x\nfetched: tuesday\n---\n\nsame body\n", "shape-up", false) + require.NoError(t, err) + + assert.Equal(t, 1, calls, "only the body counts as the source") +} diff --git a/ingestion/internal/wiki/merge.go b/ingestion/internal/wiki/merge.go index 578bf72..20de1ba 100644 --- a/ingestion/internal/wiki/merge.go +++ b/ingestion/internal/wiki/merge.go @@ -34,7 +34,7 @@ type section struct { // 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) + fmB, secsB := parseSections(b.Content) idx := make(map[string]int, len(secsA)) for i, s := range secsA { @@ -53,11 +53,60 @@ 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(refreshOwned(fmA, fmB), secsA)} +} + +// generatorOwned names the frontmatter fields the ingest pipeline may refresh +// on a page it is revising. Everything else on disk wins. +// +// #103 made the on-disk frontmatter win wholesale, which is what stops a +// regenerating writer from overwriting human-set metadata — a title someone +// fixed, or the claim-property envelope (brain#21). The cost was that fields +// the generator legitimately owns froze at their first-ingest values: when it +// last ran, and what it last saw. +// +// An explicit allowlist rather than a carve-out: the ownership boundary is +// nameable, and a field nobody has thought about stays human-owned by default, +// which is the safe direction. +var generatorOwned = map[string]bool{ + "last_updated": true, + "source_hash": true, +} + +// 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. +func refreshOwned(a, b string) string { + 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 + } + bVals[strings.TrimSpace(key)] = strings.TrimSpace(value) + } + if len(bVals) == 0 { + return a + } + + out := make([]string, 0, len(strings.Split(a, "\n"))) + for _, line := range strings.Split(a, "\n") { + 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 + } + } + out = append(out, line) + } + return strings.Join(out, "\n") } func parseSections(markdown string) (frontmatter string, sections []section) { @@ -96,15 +145,64 @@ func parseSections(markdown string) (frontmatter string, sections []section) { return } +// 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 string, sections []section) string { var sb strings.Builder sb.WriteString(frontmatter) 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() } +// 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 t := strings.TrimSpace(line); t != "" { + seen[t] = true + } + } + + var add []string + for _, line := range strings.Split(b, "\n") { + t := strings.TrimSpace(line) + if t == "" || seen[t] { + continue + } + seen[t] = 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 diff --git a/ingestion/internal/wiki/merge_test.go b/ingestion/internal/wiki/merge_test.go index d69480d..9ea466f 100644 --- a/ingestion/internal/wiki/merge_test.go +++ b/ingestion/internal/wiki/merge_test.go @@ -53,3 +53,93 @@ func TestMerge_KeepsFrontmatterFromA(t *testing.T) { assert.Contains(t, got.Content, "title: A") assert.NotContains(t, got.Content, "title: B") } + +// --- 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") +} + +func TestMerge_GeneratorOwnedFrontmatterFieldsAreRefreshed(t *testing.T) { + // #103 made on-disk frontmatter win wholesale, which is what protects + // human-set fields from a regenerating writer. The cost was that fields the + // generator legitimately owns — when it last ran, what it last saw — froze + // at their first-ingest values. An explicit owned-set is the boundary: + // named fields the generator may refresh, everything else human-owned. + existing := Page{Path: "wiki/sources/x.md", Content: `--- +title: 'X' +last_updated: 2026-01-01 +source_hash: aaaa +half_life: durable +claim_provenance: asserted +--- + +## 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, "last_updated: 2026-08-22", "generator owns last_updated") + assert.Contains(t, got, "source_hash: bbbb", "generator owns source_hash") + assert.Contains(t, got, "title: 'X'", "a human-set title must survive regeneration") + assert.Contains(t, got, "half_life: durable", "the claim envelope must survive regeneration") + assert.Contains(t, got, "claim_provenance: asserted") + assert.Contains(t, got, "Original.", "an existing section still wins over the model's rewrite") + assert.NotContains(t, got, "Renamed by the model") +}