mathias/hyperguild
1
0
Fork 0
Code Issues 30 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

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

Merged
mathias merged 3 commits from agent/b1-compile-on-ingest into main 2026-09-22 21:43:04 +00:00
Conversation 2 Commits 3 Files Changed 5
+349 -14
mathias commented 2026-08-22 11:19:43 +00:00
Owner
Copy Link
Copy Source

Compile-on-ingest, first half. Sprint B item 1.

The framing was too kind

The gap was described as "ingest still appends." It doesn't append — buildAndWrite did an unconditional os.WriteFile, so re-ingesting a source replaced its page wholesale, discarding everything accumulated since, human edits included. #43 recorded this from the human column as "re-ingest overwrites wholesale"; it's a data-loss path, not a tidiness problem.

What it does now

Reads what's on disk and merges the fresh extraction into it, deliberately asymmetric in the existing page's favour:

On the page In the extraction Result
a section the same section on-disk version wins
— a new section added
bullet list more bullets unioned, no duplicates
append section more lines only genuinely new lines
frontmatter frontmatter on-disk wins

An extraction can add to a note but never silently rewrite what a human put there — which is what makes this safe to run on every ingest rather than behind a review gate. That matters for the ADR-0009 authority split: this writes into curated space, so it earns that by being unable to overwrite curated content, not by asking permission.

Pleasant side effect: on-disk frontmatter winning means the claim-property envelope (half_life, applies, claim_provenance) survives re-ingest untouched.

Two supporting fixes, both load-bearing

Neither was in the plan; both are required for merge-on-every-ingest to be safe, and I found them by testing rather than reading.

Append sections concatenated wholesale — correct exactly once. Open Questions, Updates, Evolving Notes would have grown on every re-ingest until the page was unreadable. Now only genuinely new lines append. Line-level, not block-level: the common case is a re-ingest carrying one new line alongside several it already has, and a block-level "skip if identical" would duplicate the old ones every time that happened.

rebuildContent grew the page by a blank line per merge pass — it kept whatever blank lines a section was parsed with and added its own. Invisible in a diff viewer, fatal to byte-stability. Section bodies are now normalised.

That second one is why the idempotency test earns its place: without it, every ingest dirties the tree, brain-sync commits it every 15 minutes, and the git history fills with whitespace noise that hides real edits.

Note the sequencing: the drift guard that would otherwise catch repeated ingests (hashing the source body — B3) doesn't exist yet, so idempotency has to hold here rather than upstream. Building B1 on an unbuilt guard would have been the wrong dependency direction.

Tests

Red-first. TestRunRaw_MergesIntoAnExistingPageInsteadOfOverwritingIt failed by losing a hand-written ## My Take section. TestMerge_AppendSectionDoesNotDuplicateContentAlreadyPresent failed twice — first on duplication, then on the whitespace growth above.

TestRunRaw_ReIngestingUnchangedContentIsByteIdentical passed from the start — clobbering is trivially byte-stable — so it's a guard that the fix didn't regress the one property the old behaviour had. Labelled as such.

task check exit 0; full suite green including the pre-existing merge tests.

Known limitation, deliberate

On-disk frontmatter wins, so last_updated keeps its first-ingest date on a revised page. The alternative is this function editing frontmatter it doesn't own. No lint reads that field today (stale-date reads updated_at on hall: facts, which source pages don't carry), so nothing is misled — but if that changes, this is where to look.

Not in this PR

The LLM half of compile-on-ingest — feeding the existing page back into the extraction prompt so the model revises with knowledge of what's already there, rather than the deterministic section merge here. That's the larger, riskier half and it wants the contradiction detector (B2) alongside it, since "the model rewrote a claim" and "the model contradicted a claim" are the same event seen twice.

Risk: LOW–MEDIUM. Behaviour change on every ingest, but strictly less destructive than what it replaces, and idempotent by test.

Compile-on-ingest, first half. Sprint B item 1. ## The framing was too kind The gap was described as *"ingest still appends."* It doesn't append — `buildAndWrite` did an unconditional `os.WriteFile`, so re-ingesting a source **replaced its page wholesale**, discarding everything accumulated since, human edits included. #43 recorded this from the human column as *"re-ingest overwrites wholesale"*; it's a data-loss path, not a tidiness problem. ## What it does now Reads what's on disk and merges the fresh extraction into it, deliberately asymmetric in the existing page's favour: | On the page | In the extraction | Result | |---|---|---| | a section | the same section | **on-disk version wins** | | — | a new section | added | | bullet list | more bullets | unioned, no duplicates | | append section | more lines | only genuinely new lines | | frontmatter | frontmatter | **on-disk wins** | An extraction can add to a note but **never silently rewrite what a human put there** — which is what makes this safe to run on every ingest rather than behind a review gate. That matters for the ADR-0009 authority split: this writes into curated space, so it earns that by being unable to overwrite curated content, not by asking permission. Pleasant side effect: on-disk frontmatter winning means the claim-property envelope (`half_life`, `applies`, `claim_provenance`) survives re-ingest untouched. ## Two supporting fixes, both load-bearing Neither was in the plan; both are required for merge-on-every-ingest to be safe, and I found them by testing rather than reading. **Append sections concatenated wholesale** — correct exactly once. `Open Questions`, `Updates`, `Evolving Notes` would have grown on every re-ingest until the page was unreadable. Now only genuinely new lines append. **Line-level, not block-level**: the common case is a re-ingest carrying one new line alongside several it already has, and a block-level "skip if identical" would duplicate the old ones every time that happened. **`rebuildContent` grew the page by a blank line per merge pass** — it kept whatever blank lines a section was parsed with and added its own. Invisible in a diff viewer, fatal to byte-stability. Section bodies are now normalised. That second one is why the idempotency test earns its place: without it, every ingest dirties the tree, brain-sync commits it every 15 minutes, and the git history fills with whitespace noise that hides real edits. Note the sequencing: the drift guard that would otherwise catch repeated ingests (hashing the source body — B3) **doesn't exist yet**, so idempotency has to hold here rather than upstream. Building B1 on an unbuilt guard would have been the wrong dependency direction. ## Tests Red-first. `TestRunRaw_MergesIntoAnExistingPageInsteadOfOverwritingIt` failed by losing a hand-written `## My Take` section. `TestMerge_AppendSectionDoesNotDuplicateContentAlreadyPresent` failed twice — first on duplication, then on the whitespace growth above. `TestRunRaw_ReIngestingUnchangedContentIsByteIdentical` **passed from the start** — clobbering is trivially byte-stable — so it's a guard that the fix didn't regress the one property the old behaviour had. Labelled as such. `task check` exit 0; full suite green including the pre-existing merge tests. ## Known limitation, deliberate On-disk frontmatter wins, so `last_updated` keeps its first-ingest date on a revised page. The alternative is this function editing frontmatter it doesn't own. No lint reads that field today (`stale-date` reads `updated_at` on `hall: facts`, which source pages don't carry), so nothing is misled — but if that changes, this is where to look. ## Not in this PR The LLM half of compile-on-ingest — feeding the existing page back into the extraction prompt so the model revises *with knowledge of what's already there*, rather than the deterministic section merge here. That's the larger, riskier half and it wants the contradiction detector (B2) alongside it, since "the model rewrote a claim" and "the model contradicted a claim" are the same event seen twice. **Risk:** LOW–MEDIUM. Behaviour change on every ingest, but strictly less destructive than what it replaces, and idempotent by test.
mathias added 1 commit 2026-08-22 11:19:44 +00:00
feat(pipeline): ingest revises the existing page instead of clobbering it
CI / Lint / Test / Vet (pull_request) Successful in 38s
Details
CI / Mirror to GitHub (pull_request) Skipped
Details
60a8801a1d 
Compile-on-ingest, first half. buildAndWrite did an unconditional
os.WriteFile, so re-ingesting a source replaced its page wholesale and
discarded everything accumulated since — including human edits.
hyperguild#43 recorded the symptom as "re-ingest overwrites wholesale";
the framing that ingest "appends" understates it.

Now the write reads what is on disk and merges the fresh extraction into
it, asymmetrically in the existing page's favour: on-disk frontmatter and
on-disk sections win, extraction-only sections are added, bullet lists
union. An extraction can add to a note but never silently rewrite what a
human put there, which is what makes it safe on every ingest rather than
behind a review gate.

Two supporting fixes in wiki.Merge, both required for this to be safe to
run repeatedly:

- append sections (Open Questions, Updates, Evolving Notes) concatenated
  wholesale, which is correct exactly once. Re-ingesting an unchanged
  source would grow them every run until the page was unreadable. Now only
  genuinely new lines are appended. Line-level rather than block-level,
  because the common case is a re-ingest carrying one new line alongside
  several it already has, and a block-level check would duplicate the old
  ones every time.

- rebuildContent kept whatever blank lines a section was parsed with and
  added its own, so every merge pass grew the page by a blank line —
  invisible in a diff viewer, fatal to byte-stability. Section bodies are
  now normalised. Re-ingesting unchanged content is byte-identical, so
  brain-sync does not commit noise that hides real edits.

The drift guard that would otherwise catch repeated ingests (hashing the
source body, B3) does not exist yet, so idempotency holds here rather than
upstream.

Known limitation, deliberate: on-disk frontmatter wins, so last_updated
keeps its first-ingest date on a revised page. The alternative is this
function editing frontmatter it does not own, and no lint reads that field.

Refs hyperguild#43.
mathias added 1 commit 2026-09-22 21:34:15 +00:00
fix(wiki): stop Merge from silently deleting on-disk content it can't parse
CI / Lint / Test / Vet (pull_request) Successful in 32s
Details
CI / Mirror to GitHub (pull_request) Skipped
Details
9abe4369f2 
Review of PR #103 found a real data-loss bug: parseSections only captured
text between `## ` headings. Content before the first heading — an intro
paragraph, or, if the page had no headings at all, the entire body — was
dropped on the floor, not carried into any section. Merge then had
nothing of `a` to keep there, so the fresh extraction's sections silently
became the whole output wherever that content used to be. Exactly the
failure this file exists to prevent, just re-triggered by a page shape
the parser didn't expect instead of by every re-ingest.

Fixed by treating pre-first-heading content as a section with an empty
heading. Merge's switch never matches "", so it falls through to the
same on-disk-wins default every other unlisted section already gets —
no new merge path, just no longer discarding this one's input.

Second, separate finding from the same review: mergeBullets/appendNew
deduped on the raw trimmed line, so two re-extractions of the same
logical bullet with different internal whitespace (a real possibility —
the extractor is an LLM, not a fixed serializer) produced a duplicate
instead of a no-op, undermining the idempotency this file's own doc
comments claim. Fixed by comparing a whitespace-canonicalized key
(strings.Fields + single-space join) while still emitting the original
line's text.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mathias commented 2026-09-22 21:37:32 +00:00
Author
Owner
Copy Link
Copy Source

Re-review of 9abe436: both fixes from the first review hold up.

  • Preamble fix verified by flip test: reverting the started/empty-heading fallthrough in parseSections turns exactly TestMerge_PreservesOnDiskContentWithNoSectionHeadings and TestMerge_PreservesPreambleBeforeFirstHeading red, nothing else moves.
  • Whitespace-dedup fix verified by flip test: reverting canonicalLine to strings.TrimSpace turns exactly TestMerge_AppendSectionDedupsAcrossWhitespaceVariants and TestMerge_BulletSectionDedupsAcrossWhitespaceVariants red, nothing else moves.
  • Both new preamble-only and double-merge stability scenarios I probed manually (page with nothing but preamble on both sides; two extra merge passes over a page with a preamble) round-trip byte-identically. Not a problem.

Third problem found -- blocking on it:

parseSections now uses heading == "" as the sentinel for "this is the pre-first-heading preamble." But a literal markdown heading line ## (marker + trailing space, no title text) also produces heading == "" via strings.TrimPrefix(line, "## "). Repro (internal/wiki/merge.go unchanged, ad hoc test):

a.Content = "---\ntitle: X\n---\n\nIntro text.\n\n## \n\nMystery section body.\n\n## Definition\n\nReal def.\n"

parseSections yields two sections both with heading=="" (the real preamble, and the degenerate ## heading). rebuildContent treats every heading=="" section as the preamble case (no ## marker re-emitted), so the degenerate heading's ## marker vanishes on rebuild and its body silently fuses into the surrounding prose -- structural information ("this was its own section") is lost with no trace. This collision did not exist pre-fix (there was no preamble concept to collide with); it's introduced by this exact fix reusing "" as the sentinel. It also means Merge's idx map (idx[s.heading] = i) can only track the last heading=="" entry in secsA, so if a itself carries both a real preamble and a degenerate ## section, one silently loses its map slot.

Likelihood is low -- it needs the extractor to emit a heading marker with empty title text -- but it's real, reproducible, and in the same file/mechanism that already caused one block today. Fix is a judgment call (e.g. a separate isPreamble bool on section instead of overloading the empty string), not a one-line correction, so I'm not making it myself. Not merging. Escalating to Mathias.

CI on 9abe436: green (CI / Lint / Test / Vet success, run 7814).

Re-review of 9abe436: both fixes from the first review hold up. - Preamble fix verified by flip test: reverting the `started`/empty-heading fallthrough in `parseSections` turns exactly `TestMerge_PreservesOnDiskContentWithNoSectionHeadings` and `TestMerge_PreservesPreambleBeforeFirstHeading` red, nothing else moves. - Whitespace-dedup fix verified by flip test: reverting `canonicalLine` to `strings.TrimSpace` turns exactly `TestMerge_AppendSectionDedupsAcrossWhitespaceVariants` and `TestMerge_BulletSectionDedupsAcrossWhitespaceVariants` red, nothing else moves. - Both new preamble-only and double-merge stability scenarios I probed manually (page with nothing but preamble on both sides; two extra merge passes over a page with a preamble) round-trip byte-identically. Not a problem. Third problem found -- blocking on it: `parseSections` now uses `heading == ""` as the sentinel for "this is the pre-first-heading preamble." But a literal markdown heading line `## ` (marker + trailing space, no title text) also produces `heading == ""` via `strings.TrimPrefix(line, "## ")`. Repro (`internal/wiki/merge.go` unchanged, ad hoc test): ``` a.Content = "---\ntitle: X\n---\n\nIntro text.\n\n## \n\nMystery section body.\n\n## Definition\n\nReal def.\n" ``` `parseSections` yields two sections both with `heading==""` (the real preamble, and the degenerate `## ` heading). `rebuildContent` treats every `heading==""` section as the preamble case (no `## ` marker re-emitted), so the degenerate heading's `## ` marker vanishes on rebuild and its body silently fuses into the surrounding prose -- structural information ("this was its own section") is lost with no trace. This collision did not exist pre-fix (there was no preamble concept to collide with); it's introduced by this exact fix reusing `""` as the sentinel. It also means `Merge`'s `idx` map (`idx[s.heading] = i`) can only track the *last* `heading==""` entry in `secsA`, so if `a` itself carries both a real preamble and a degenerate `## ` section, one silently loses its map slot. Likelihood is low -- it needs the extractor to emit a heading marker with empty title text -- but it's real, reproducible, and in the same file/mechanism that already caused one block today. Fix is a judgment call (e.g. a separate `isPreamble bool` on `section` instead of overloading the empty string), not a one-line correction, so I'm not making it myself. Not merging. Escalating to Mathias. CI on 9abe436: green (`CI / Lint / Test / Vet` success, run 7814).
mathias added 1 commit 2026-09-22 21:40:32 +00:00
fix(wiki): give the preamble its own field instead of overloading heading==""
CI / Lint / Test / Vet (pull_request) Successful in 31s
Details
CI / Mirror to GitHub (pull_request) Skipped
Details
09aeaa91f5 
Second review pass on PR #103 found a third bug in the same file: the
preamble fix from the prior commit represented "content before the first
## heading" as a section with heading=="" — which collides with a literal
degenerate `## ` (marker, no title text; strings.TrimPrefix also yields
"").  Two entries sharing one key in Merge's idx map means one loses
lookup visibility. Reproduced: a has only a preamble, b introduces a
genuinely new degenerate `## ` section — Merge's idx wrongly reports it
as "already exists" (it's really a's unrelated preamble) and never adds
it, so b's section vanishes from the output entirely.

Fixed by giving parseSections a dedicated preamble return value instead
of folding it into the sections slice. It can no longer collide with
anything, degenerate or otherwise, because it's no longer keyed by
heading at all. Merge and rebuildContent updated to thread it through
directly. No other caller of parseSections/rebuildContent in this repo.

Also fixed a doc-comment left misplaced by the prior commit (appendNew's
comment had drifted onto canonicalLine, inserted between them).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mathias merged commit f450a6c84f into main 2026-09-22 21:43:04 +00:00
mathias commented 2026-09-22 21:43:20 +00:00
Author
Owner
Copy Link
Copy Source

Round 3 review of 09aeaa9 — MERGED (squash f450a6c8).

Structural check: confirmed preamble is a separate return value from parseSections, never appended to sections, never keyed into Merge's idx map. Read the full current file; there is no remaining path by which the preamble can collide with a section, including with itself.

b's preamble is discarded — checked, not a new bug: Merge uses preambleA only. This matches the existing "on-disk wins for anything unlisted" default every other unlisted section already gets (frontmatter, non-bullet/non-append sections) — it's documented as such in the new doc comment, and this exact discard-b's-preamble behavior was already present under round 2's heading=="" model (same default-wins branch), so it predates this commit and isn't introduced by it. Traced the two call sites: pipeline.go:149 never even calls Merge for a brand-new page (os.IsNotExist short-circuits to page.Content as-is, full preamble intact) — only mergeAll's same-run dedup path (pipeline.go:161) could theoretically drop a later-occurrence's preamble in favour of an earlier one. Pre-existing, documented, out of scope for this fix; not blocking.

Flip test: reverted merge.go to the round-2 (9abe436) heading=="" model and ran TestMerge_DegenerateEmptyHeadingDoesNotCollideWithPreamble — went red exactly as expected (b's degenerate section vanished). Restored the fix, full suite green again, working tree clean.

Adversarial pass (own tests, not committed): multiple degenerate ## headings in one page (known pre-existing limit: section identity is heading-text-only, not unique-ified — unrelated to preamble, unchanged by this commit), frontmatter-only page, unicode/multi-byte content straddling the ## boundary, and 3x self-merge idempotency on a page with both a preamble and a degenerate heading — all held, all byte-stable.

CI: green on 09aeaa9, run 7820 (CI / Lint / Test / Vet, success).

Pattern check: three review passes, three distinct real bugs in this file today (drop-all-preamble, heading=="" collision, and now nothing). The third pass found no fourth — the fix this time changed the data model (separate field) rather than patching the symptom, which is why it closed off the collision class structurally instead of moving it. Calling the pattern over for this file, not for this PR author's habit of introducing load-bearing fixes outside the stated plan — worth eyes on the next file this shape shows up in.

Round 3 review of 09aeaa9 — MERGED (squash f450a6c8). **Structural check**: confirmed preamble is a separate return value from `parseSections`, never appended to `sections`, never keyed into `Merge`'s `idx` map. Read the full current file; there is no remaining path by which the preamble can collide with a section, including with itself. **b's preamble is discarded — checked, not a new bug**: `Merge` uses `preambleA` only. This matches the *existing* "on-disk wins for anything unlisted" default every other unlisted section already gets (frontmatter, non-bullet/non-append sections) — it's documented as such in the new doc comment, and this exact discard-b's-preamble behavior was already present under round 2's heading=="" model (same default-wins branch), so it predates this commit and isn't introduced by it. Traced the two call sites: `pipeline.go:149` never even calls Merge for a brand-new page (os.IsNotExist short-circuits to `page.Content` as-is, full preamble intact) — only `mergeAll`'s same-run dedup path (pipeline.go:161) could theoretically drop a later-occurrence's preamble in favour of an earlier one. Pre-existing, documented, out of scope for this fix; not blocking. **Flip test**: reverted merge.go to the round-2 (9abe436) heading=="" model and ran `TestMerge_DegenerateEmptyHeadingDoesNotCollideWithPreamble` — went red exactly as expected (b's degenerate section vanished). Restored the fix, full suite green again, working tree clean. **Adversarial pass** (own tests, not committed): multiple degenerate `## ` headings in one page (known pre-existing limit: section identity is heading-text-only, not unique-ified — unrelated to preamble, unchanged by this commit), frontmatter-only page, unicode/multi-byte content straddling the `## ` boundary, and 3x self-merge idempotency on a page with both a preamble and a degenerate heading — all held, all byte-stable. **CI**: green on 09aeaa9, run 7820 (`CI / Lint / Test / Vet`, success). Pattern check: three review passes, three distinct real bugs in this file today (drop-all-preamble, heading=="" collision, and now nothing). The third pass found no fourth — the fix this time changed the *data model* (separate field) rather than patching the symptom, which is why it closed off the collision class structurally instead of moving it. Calling the pattern over for this file, not for this PR author's habit of introducing load-bearing fixes outside the stated plan — worth eyes on the next file this shape shows up in.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
agent/ready
Meets the agent-ready contract; authorizes unattended execution
 blocked/client
Waiting on the client
 blocked/supplier
Waiting on a supplier
 blocked/upstream
Waiting on an upstream project or vendor
 risk/high
Irreversible, security-relevant, or control plane
 risk/low
Reversible, local, test-covered
 risk/medium
Touches shared state, deploys or data
 size/l
Too big - split it
 size/m
Under two days
 size/s
Under half a day
 size/xs
Under an hour
 status:dispatch
status:dispatched
status:review
status:reviewing
type/chore
Maintenance, no behaviour change
 type/decision
Human decision required; never agent/ready
 type/docs
Documentation only
 type/feat
New capability
 type/fix
Corrects broken behaviour
 type/ops
Infrastructure, deploy, runtime
 type/research
Investigate; output is a finding, not code
 type/test
Test coverage only
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate-bot mathias
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/hyperguild#103