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

Source drift: hash the ingested body, skip an unchanged re-ingest, flag a changed one #105

New Issue
Closed
opened 2026-08-22 11:59:04 +00:00 by mathias · 3 comments
mathias commented 2026-08-22 11:59:04 +00:00
Owner
Copy Link
Copy Source

Sprint B item 3. Filed so it stops living only in a conversation.

What

On ingest, hash the source body. Re-ingesting the same URL:

  • unchanged → skip the extraction entirely
  • changed → flag it, and let the merge update the page

content_hash already exists in the codebase (brain_get/brain_read return it, brain_update stamps prior-hash on supersede) — reuse the concept rather than inventing a second hashing scheme.

Why it is now an optimisation, not a correctness fix

Worth stating plainly, because the plan had this the other way round.

The reason to want a skip-if-unchanged guard was that re-ingesting grew pages without bound. #103 fixed that in the write itself: the merge is idempotent, and re-ingesting unchanged content is byte-identical by test. So this issue no longer carries correctness — it carries cost and signal:

  • cost: an unchanged re-ingest currently still runs the full LLM extraction over every chunk, for a result that merges to nothing. That is the real saving.
  • signal: "this source changed since we ingested it" is information the corpus does not have today, and it is the natural input to half_life (brain#21) — a volatile note whose source has changed is the highest-value re-verification candidate in the corpus.

Building it before #103 would have put idempotency in the guard rather than the write, which is the wrong dependency direction — the write must be safe on its own.

Scope

  • Hash the raw body (post-frontmatter, so a metadata-only change is not drift) at ingest
  • Store it where the page can be compared against it on the next run — frontmatter field, or a sidecar index; decide which, noting that frontmatter is what survives a corpus move
  • Unchanged → skip extraction, log it, and make the skip visible rather than silent (a run that does nothing should say so)
  • Changed → proceed, and record that drift happened
  • Test: same content twice → one extraction, not two

Interaction to decide

Where does the hash live if the on-disk frontmatter wins on merge (#103)? If ingest writes the hash into frontmatter, the merge must let that specific field be updated by the generator while everything else stays human-owned — i.e. one field the generator owns. Cleanest is probably a generator-owned block rather than a loose key, so the ownership boundary is explicit.

Risk: LOW — additive, and the failure mode of a wrong hash is an unnecessary re-extraction, not data loss.

Refs: #103, brain#21 (half_life), #43.

Sprint B item 3. Filed so it stops living only in a conversation. ## What On ingest, hash the source body. Re-ingesting the same URL: - **unchanged** → skip the extraction entirely - **changed** → flag it, and let the merge update the page `content_hash` already exists in the codebase (`brain_get`/`brain_read` return it, `brain_update` stamps prior-hash on supersede) — reuse the concept rather than inventing a second hashing scheme. ## Why it is now an optimisation, not a correctness fix Worth stating plainly, because the plan had this the other way round. The reason to want a skip-if-unchanged guard was that re-ingesting grew pages without bound. **#103 fixed that in the write itself**: the merge is idempotent, and re-ingesting unchanged content is byte-identical by test. So this issue no longer carries correctness — it carries cost and signal: - **cost**: an unchanged re-ingest currently still runs the full LLM extraction over every chunk, for a result that merges to nothing. That is the real saving. - **signal**: "this source changed since we ingested it" is information the corpus does not have today, and it is the natural input to `half_life` (brain#21) — a `volatile` note whose source has changed is the highest-value re-verification candidate in the corpus. Building it before #103 would have put idempotency in the guard rather than the write, which is the wrong dependency direction — the write must be safe on its own. ## Scope - [ ] Hash the raw body (post-frontmatter, so a metadata-only change is not drift) at ingest - [ ] Store it where the page can be compared against it on the next run — frontmatter field, or a sidecar index; decide which, noting that frontmatter is what survives a corpus move - [ ] Unchanged → skip extraction, log it, and **make the skip visible** rather than silent (a run that does nothing should say so) - [ ] Changed → proceed, and record that drift happened - [ ] Test: same content twice → one extraction, not two ## Interaction to decide Where does the hash live if the on-disk frontmatter wins on merge (#103)? If ingest writes the hash into frontmatter, the merge must let *that specific field* be updated by the generator while everything else stays human-owned — i.e. one field the generator owns. Cleanest is probably a generator-owned block rather than a loose key, so the ownership boundary is explicit. **Risk:** LOW — additive, and the failure mode of a wrong hash is an unnecessary re-extraction, not data loss. Refs: #103, brain#21 (`half_life`), #43.
mathias added the risk/mediumsize/stype/feat labels 2026-08-27 05:25:42 +00:00
mathias commented 2026-09-23 04:54:26 +00:00
Author
Owner
Copy Link
Copy Source

PR #126 review: BLOCKED, not merged.

refreshOwned in ingestion/internal/wiki/merge.go only replaces an existing generator-owned frontmatter line in the on-disk page; it cannot append one that is not already present. Every wiki/sources/*.md page that existed before this PR ships has no source_hash key at all. On its next re-ingest, Merge(existing, fresh) calls refreshOwned(fmA, fmB): fmA has no source_hash line to replace, so bVals["source_hash"] is silently discarded and the field never lands on disk. storedSourceHash then keeps reading "" forever, so bodyHash == storedSourceHash never matches for any pre-existing source page — the skip-on-unchanged feature this issue asks for never engages for the entire existing corpus, permanently, with no warning (fails safe — always re-extracts — but silently defeats the issue's stated purpose for everything except brand-new sources ingested after this PR lands).

Verified directly: a standalone refreshOwned test with a lacking source_hash and b supplying it returns a unchanged (no source_hash line added). The PR's own TestMerge_GeneratorOwnedFrontmatterFieldsAreRefreshed never covers this — its existing fixture already carries source_hash: aaaa, so the append-vs-replace gap is untested. pipeline_test.go's skip test only exercises a page created fresh within the test's own tempdir, same gap.

This is exactly the interaction the issue itself flagged as the hard part ("the merge must let that specific field be updated by the generator") and the implementation gets wrong for the pre-existing corpus. Fix: refreshOwned must append a missing generator-owned key, not only replace an existing one.

CI was green (run 7838, CI / Lint / Test / Vet success) — green because no test exercises the missing-key case. Not merged; PR #126 branch fix/105-source-drift-on-main left open. #127 should not stack until this is fixed.

**PR #126 review: BLOCKED, not merged.** `refreshOwned` in `ingestion/internal/wiki/merge.go` only *replaces* an existing generator-owned frontmatter line in the on-disk page; it cannot *append* one that is not already present. Every `wiki/sources/*.md` page that existed before this PR ships has no `source_hash` key at all. On its next re-ingest, `Merge(existing, fresh)` calls `refreshOwned(fmA, fmB)`: `fmA` has no `source_hash` line to replace, so `bVals["source_hash"]` is silently discarded and the field never lands on disk. `storedSourceHash` then keeps reading `""` forever, so `bodyHash == storedSourceHash` never matches for any pre-existing source page — the skip-on-unchanged feature this issue asks for never engages for the entire existing corpus, permanently, with no warning (fails safe — always re-extracts — but silently defeats the issue's stated purpose for everything except brand-new sources ingested after this PR lands). Verified directly: a standalone `refreshOwned` test with `a` lacking `source_hash` and `b` supplying it returns `a` unchanged (no `source_hash` line added). The PR's own `TestMerge_GeneratorOwnedFrontmatterFieldsAreRefreshed` never covers this — its `existing` fixture already carries `source_hash: aaaa`, so the append-vs-replace gap is untested. `pipeline_test.go`'s skip test only exercises a page created fresh within the test's own tempdir, same gap. This is exactly the interaction the issue itself flagged as the hard part ("the merge must let that specific field be updated by the generator") and the implementation gets wrong for the pre-existing corpus. Fix: `refreshOwned` must append a missing generator-owned key, not only replace an existing one. CI was green (run 7838, `CI / Lint / Test / Vet` success) — green because no test exercises the missing-key case. Not merged; PR #126 branch `fix/105-source-drift-on-main` left open. #127 should not stack until this is fixed.
mathias commented 2026-09-23 05:05:30 +00:00
Author
Owner
Copy Link
Copy Source

PR #126 review (round 3): BLOCKED, not merged.

Rounds 1 and 2 are fixed (verified directly, not just via the suite): refreshOwned now appends a missing generator-owned key (round 1) and never splices unfenced text into the body (round 2) — the rewrite onto a parsed fmLine list plus a canonical renderFrontmatterBlock genuinely retires the fence-position-index bug class; confirmed by mutating parseFrontmatterBlock's strings.Index(rest, "\n---\n") fence search, which a real test in merge_test.go catches and turns red.

A 4th bug, in the same function, introduced by this rewrite: parseFrontmatterBlock's line classifier (merge.go) does key, val, found := strings.Cut(line, ":") with no guard against the line being a non-key raw line that merely contains a colon. The sibling it claims to mirror — internal/api/frontmatter.go's parseFrontmatter — explicitly guards this: !ok || key == "" || strings.HasPrefix(strings.TrimSpace(line), "#"). The new code dropped that clause. Two concrete, verified repros:

  1. A frontmatter comment with a colon and irregular spacing (#see also: https://x/y ) gets misparsed as key=# see also, value=https://x/y, then re-rendered with collapsed whitespace — byte drift on every subsequent ingest, which is exactly the invariant TestRunRaw_ReIngestingUnchangedContentIsByteIdentical exists to guard, just not for this fixture shape.
  2. Worse: a YAML block-list item whose text contains a colon — e.g. aliases:\n - 'Chapter 3: The Reckoning', an entirely plausible value for this corpus (book/chapter titles) — gets misparsed as key=- 'Chapter 3, value=The Reckoning', and re-renders without its 2-space indentation: - 'Chapter 3: The Reckoning' instead of - 'Chapter 3: The Reckoning'. That's not cosmetic — it changes the line's YAML structural meaning (de-indents it out of the list) and reproduces the same class of damage round 2 already found ("corrupted the page"), just via a different code path (parse-side misclassification instead of fence-position tracking).

Both repros verified locally against 874206b: go test on a hand-built fixture round-trips to a different byte string than the input, and for the list-item case the structural indentation is lost. Neither is covered by merge_test.go's existing fixtures (none contain a colon inside a comment or a list-item value) or by the pipeline suite.

This triggers on the normal path, not an obscure one: refreshOwned re-renders the whole frontmatter block through renderFrontmatterBlock whenever source_hash/last_updated change, i.e. on effectively every non-skipped ingest — this PR's own stated purpose. Any existing page with a #-comment or a colon-bearing list item in its frontmatter will silently degrade on its next ingest.

Fix: reproduce the sibling's #-comment guard (and more generally, don't treat a colon anywhere in a non-key raw line as a key separator) in parseFrontmatterBlock.

Separately, confirmed not a blocker but worth recording: the empty-value render branch (the aliases: no-trailing-space fix) has no direct unit test in internal/wiki itself — disabling that branch entirely (case l.value == "": mutated to always-false) leaves go test ./internal/wiki/... green; it is only caught by TestRunRaw_ReIngestingUnchangedContentIsByteIdentical in internal/pipeline. Same pattern as round 2: the wiki package's own tests are not self-sufficient proof for this function.

CI on head 874206b is green (run 7842) and the other three "done when" clauses of #105 (hash post-frontmatter body, skip+log+visible, changed→proceed+re-stamp hash, one-extraction test) are genuinely met — this block is narrowly about the new frontmatter parser's colon-handling, not the drift-skip feature itself.

Not merged. Branch fix/105-source-drift-on-main left open. #127 should continue to wait, per its own stacking note.

**PR #126 review (round 3): BLOCKED, not merged.** Rounds 1 and 2 are fixed (verified directly, not just via the suite): `refreshOwned` now appends a missing generator-owned key (round 1) and never splices unfenced text into the body (round 2) — the rewrite onto a parsed `fmLine` list plus a canonical `renderFrontmatterBlock` genuinely retires the fence-position-index bug class; confirmed by mutating `parseFrontmatterBlock`'s `strings.Index(rest, "\n---\n")` fence search, which a real test in `merge_test.go` catches and turns red. **A 4th bug, in the same function, introduced by this rewrite:** `parseFrontmatterBlock`'s line classifier (`merge.go`) does `key, val, found := strings.Cut(line, ":")` with no guard against the line being a non-key raw line that merely *contains* a colon. The sibling it claims to mirror — `internal/api/frontmatter.go`'s `parseFrontmatter` — explicitly guards this: `!ok || key == "" || strings.HasPrefix(strings.TrimSpace(line), "#")`. The new code dropped that clause. Two concrete, verified repros: 1. A frontmatter comment with a colon and irregular spacing (`#see also: https://x/y `) gets misparsed as key=`# see also`, value=`https://x/y`, then re-rendered with collapsed whitespace — byte drift on every subsequent ingest, which is exactly the invariant `TestRunRaw_ReIngestingUnchangedContentIsByteIdentical` exists to guard, just not for this fixture shape. 2. Worse: a YAML block-list item whose text contains a colon — e.g. `aliases:\n - 'Chapter 3: The Reckoning'`, an entirely plausible value for this corpus (book/chapter titles) — gets misparsed as key=`- 'Chapter 3`, value=`The Reckoning'`, and re-renders **without its 2-space indentation**: `- 'Chapter 3: The Reckoning'` instead of ` - 'Chapter 3: The Reckoning'`. That's not cosmetic — it changes the line's YAML structural meaning (de-indents it out of the list) and reproduces the same class of damage round 2 already found ("corrupted the page"), just via a different code path (parse-side misclassification instead of fence-position tracking). Both repros verified locally against `874206b`: `go test` on a hand-built fixture round-trips to a different byte string than the input, and for the list-item case the structural indentation is lost. Neither is covered by `merge_test.go`'s existing fixtures (none contain a colon inside a comment or a list-item value) or by the pipeline suite. This triggers on the *normal* path, not an obscure one: `refreshOwned` re-renders the whole frontmatter block through `renderFrontmatterBlock` whenever `source_hash`/`last_updated` change, i.e. on effectively every non-skipped ingest — this PR's own stated purpose. Any existing page with a `#`-comment or a colon-bearing list item in its frontmatter will silently degrade on its next ingest. Fix: reproduce the sibling's `#`-comment guard (and more generally, don't treat a colon anywhere in a non-key raw line as a key separator) in `parseFrontmatterBlock`. Separately, confirmed not a blocker but worth recording: the empty-value render branch (the `aliases:` no-trailing-space fix) has **no direct unit test in `internal/wiki` itself** — disabling that branch entirely (`case l.value == "":` mutated to always-false) leaves `go test ./internal/wiki/...` green; it is only caught by `TestRunRaw_ReIngestingUnchangedContentIsByteIdentical` in `internal/pipeline`. Same pattern as round 2: the wiki package's own tests are not self-sufficient proof for this function. CI on head `874206b` is green (run 7842) and the other three "done when" clauses of #105 (hash post-frontmatter body, skip+log+visible, changed→proceed+re-stamp hash, one-extraction test) are genuinely met — this block is narrowly about the new frontmatter parser's colon-handling, not the drift-skip feature itself. Not merged. Branch `fix/105-source-drift-on-main` left open. #127 should continue to wait, per its own stacking note.
mathias commented 2026-09-23 05:09:50 +00:00
Author
Owner
Copy Link
Copy Source

PR #126 merged (round 4, final): merge commit b340f86.

Round 4 fixed the last bug: parseFrontmatterBlock dropped the sibling api/frontmatter.go package's comment guard AND misparsed an indented YAML block-list item containing a colon as a top-level key. Commit 43f43e4 restored the comment guard verbatim and added an indentation check (any line starting with space/tab is a raw continuation line, checked before strings.Cut runs at all), with two new exact-match regression tests.

Verification performed before merge:

  • Read the full diff at head 43f43e4 against main.
  • Stress-tested edge cases beyond the PR's own tests: a comment line and an indented list item on adjacent lines together; a top-level value starting with # (title: '#1 in the series' — confirmed the comment guard checks the whole trimmed line, not just the post-colon value, so this is correctly NOT treated as a comment); a whitespace-only line (single tab, no crash on line[0]); a multi-byte-first-rune line (日本語: value, correctly falls through to the key/value path with no panic). All five held.
  • Flip-tested the fix two ways: (1) removed the indentation guard entirely — both the PR's own TestRefreshOwned_IndentedListItemsWithColonsAreNotMisparsedAsKeys and a scratch test went red; (2) narrowed the comment guard to check only the post-colon value instead of the whole line — TestRefreshOwned_CommentLinesSurviveUnmangled went red. Both mutations were caught.
  • Full module suite from ingestion/: go build ./..., go vet ./..., go test ./... -race -count=1 — all green, checked by exit code not grep.
  • Confirmed issue #128's framing is accurate: internal/api/frontmatter.go's parseFrontmatter still lacks the indentation guard and has the identical list-item-colon bug, pre-existing and correctly out of scope for this PR.
  • CI green at head 43f43e4 (Gitea Actions run 7843, conclusion success), PR was mergeable: true with no conflicts.

Scope note (not blocking): four review rounds all landed inside refreshOwned/frontmatter parsing, which is orthogonal to this issue's actual feature (skip-extraction-when-unchanged). In hindsight that mechanism earned its own PR, reviewed independently of the skip-on-unchanged feature it happened to be built to support — the feature diff itself was never what needed four passes. Not acted on here since the code is now correct and tested; noting it for next time a similarly load-bearing helper rides in on a feature PR.

Done-when clauses verified: hash of raw post-frontmatter body computed at ingest; stored in a generator-owned frontmatter field (source_hash, alongside last_updated) that survives append/replace correctly for both pre-existing and fresh pages; unchanged source skips extraction (logged, visible per #105's own scope item) and changed source proceeds; TestRunRaw_ReIngestingUnchangedContentIsByteIdentical-class tests cover byte-stability. PR #127 is stacked on top and correctly retargeted onto main.

**PR #126 merged (round 4, final): merge commit `b340f86`.** Round 4 fixed the last bug: `parseFrontmatterBlock` dropped the sibling `api/frontmatter.go` package's comment guard AND misparsed an indented YAML block-list item containing a colon as a top-level key. Commit `43f43e4` restored the comment guard verbatim and added an indentation check (any line starting with space/tab is a raw continuation line, checked before `strings.Cut` runs at all), with two new exact-match regression tests. Verification performed before merge: - Read the full diff at head `43f43e4` against `main`. - Stress-tested edge cases beyond the PR's own tests: a comment line and an indented list item on adjacent lines together; a top-level value starting with `#` (`title: '#1 in the series'` — confirmed the comment guard checks the whole trimmed line, not just the post-colon value, so this is correctly NOT treated as a comment); a whitespace-only line (single tab, no crash on `line[0]`); a multi-byte-first-rune line (`日本語: value`, correctly falls through to the key/value path with no panic). All five held. - Flip-tested the fix two ways: (1) removed the indentation guard entirely — both the PR's own `TestRefreshOwned_IndentedListItemsWithColonsAreNotMisparsedAsKeys` and a scratch test went red; (2) narrowed the comment guard to check only the post-colon value instead of the whole line — `TestRefreshOwned_CommentLinesSurviveUnmangled` went red. Both mutations were caught. - Full module suite from `ingestion/`: `go build ./...`, `go vet ./...`, `go test ./... -race -count=1` — all green, checked by exit code not grep. - Confirmed issue #128's framing is accurate: `internal/api/frontmatter.go`'s `parseFrontmatter` still lacks the indentation guard and has the identical list-item-colon bug, pre-existing and correctly out of scope for this PR. - CI green at head `43f43e4` (Gitea Actions run 7843, conclusion `success`), PR was `mergeable: true` with no conflicts. Scope note (not blocking): four review rounds all landed inside `refreshOwned`/frontmatter parsing, which is orthogonal to this issue's actual feature (skip-extraction-when-unchanged). In hindsight that mechanism earned its own PR, reviewed independently of the skip-on-unchanged feature it happened to be built to support — the feature diff itself was never what needed four passes. Not acted on here since the code is now correct and tested; noting it for next time a similarly load-bearing helper rides in on a feature PR. Done-when clauses verified: hash of raw post-frontmatter body computed at ingest; stored in a generator-owned frontmatter field (`source_hash`, alongside `last_updated`) that survives append/replace correctly for both pre-existing and fresh pages; unchanged source skips extraction (logged, visible per #105's own scope item) and changed source proceeds; `TestRunRaw_ReIngestingUnchangedContentIsByteIdentical`-class tests cover byte-stability. PR #127 is stacked on top and correctly retargeted onto `main`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 risk/medium size/s type/feat
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#105