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

Compile-on-ingest, second half: let the extraction see the page it is revising #106

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

Follow-on from #103, which shipped the deterministic half. Filed so the remaining half is tracked rather than implied.

What #103 does and does not do

Does: ingest now merges into the page already on disk instead of clobbering it, asymmetrically in the existing page's favour — on-disk sections and frontmatter win, extraction-only sections are added, lists union. Re-ingest is byte-identical when nothing changed.

Does not: the extraction itself is still page-blind. The model produces a page as if writing it fresh, and wiki.Merge reconciles the two texts afterwards by heading. So:

  • the model cannot refine an existing claim — it can only restate it, and the on-disk version wins by design
  • it cannot notice that its new material contradicts what is already there (that is #104)
  • section-heading drift silently creates a near-duplicate section rather than updating the original ("Key Claims" vs "Key claims")

That is a real ceiling: the merge can only combine texts, it cannot compile them.

The second half

Feed the current page into the extraction prompt, so the model revises with knowledge of what exists:

  • Include the existing page body in BuildPrompt when the target path already exists
  • Ask for a revision rather than a fresh page — explicitly: what to add, what to strengthen, what is unchanged
  • Keep the deterministic merge as the backstop. The model proposing a revision does not mean it may overwrite; #103's asymmetry stays, so a model that ignores the instruction still cannot destroy human content
  • Normalise headings before matching, so casing/pluralisation drift stops creating parallel sections

Why it wants #104 alongside it

"The model rewrote this claim" and "the model contradicted this claim" are the same event observed twice. A revision pass without contradiction detection produces silent overwrites of meaning while preserving the text's shape — which is harder to notice than a clobber, not easier.

So the sequencing is #104 first or together, never this alone.

Also carried from #103

last_updated keeps its first-ingest date on a revised page, because the on-disk frontmatter wins. Deliberate for now — the alternative is the writer editing frontmatter it does not own — but if a generator-owned frontmatter block lands (see #105's ownership question), this is the natural field to put in it.

Risk: MEDIUM — larger prompt, more expensive extraction, and a model given the current page will sometimes echo it back verbatim. Needs a test that a revision pass on unchanged input still produces a byte-identical page.

Refs: #103, #104, #105, #43.

Follow-on from #103, which shipped the deterministic half. Filed so the remaining half is tracked rather than implied. ## What #103 does and does not do **Does:** ingest now merges into the page already on disk instead of clobbering it, asymmetrically in the existing page's favour — on-disk sections and frontmatter win, extraction-only sections are added, lists union. Re-ingest is byte-identical when nothing changed. **Does not:** the extraction itself is still page-blind. The model produces a page as if writing it fresh, and `wiki.Merge` reconciles the two texts afterwards by heading. So: - the model cannot *refine* an existing claim — it can only restate it, and the on-disk version wins by design - it cannot notice that its new material contradicts what is already there (that is #104) - section-heading drift silently creates a near-duplicate section rather than updating the original ("Key Claims" vs "Key claims") That is a real ceiling: the merge can only combine texts, it cannot *compile* them. ## The second half Feed the current page into the extraction prompt, so the model revises with knowledge of what exists: - [ ] Include the existing page body in `BuildPrompt` when the target path already exists - [ ] Ask for a revision rather than a fresh page — explicitly: what to add, what to strengthen, what is unchanged - [ ] Keep the deterministic merge as the backstop. The model proposing a revision does not mean it may overwrite; #103's asymmetry stays, so a model that ignores the instruction still cannot destroy human content - [ ] Normalise headings before matching, so casing/pluralisation drift stops creating parallel sections ## Why it wants #104 alongside it "The model rewrote this claim" and "the model contradicted this claim" are the same event observed twice. A revision pass without contradiction detection produces silent overwrites of meaning while preserving the text's shape — which is harder to notice than a clobber, not easier. So the sequencing is #104 first or together, never this alone. ## Also carried from #103 `last_updated` keeps its first-ingest date on a revised page, because the on-disk frontmatter wins. Deliberate for now — the alternative is the writer editing frontmatter it does not own — but if a generator-owned frontmatter block lands (see #105's ownership question), this is the natural field to put in it. **Risk:** MEDIUM — larger prompt, more expensive extraction, and a model given the current page will sometimes echo it back verbatim. Needs a test that a revision pass on unchanged input still produces a byte-identical page. Refs: #103, #104, #105, #43.
mathias added the risk/mediumsize/mtype/feat labels 2026-08-27 05:25:42 +00:00
mathias commented 2026-09-23 05:32:38 +00:00
Author
Owner
Copy Link
Copy Source

Review of PR #130 (feat/106-page-aware-extraction) — BLOCKED, not merged

What's solid: BuildPrompt's existing-page plumbing is real (empty string confirmed on a fresh source, non-empty only when wiki/sources/<slug>.md exists), the revision framing is scoped correctly to the source page only. TestRun_RevisionEchoingExistingPageStaysByteIdentical is genuinely end-to-end (Run → mocked Complete → Merge → written file) and uses assert.Equal, not Contains. Flip-tested normalizeHeading by making it a no-op: both TestMerge_HeadingCaseDriftMatchesSameSection and TestMerge_HeadingWhitespaceDriftMatchesSameSection went red as expected — not decoration. Full module suite (go build ./... && go vet ./... && go test ./... -race -count=1) green, 31 packages, no races — both on the PR branch tip and after merging current main (which now includes #104's cdafdf6, landed on main after this branch was cut) into it locally; the auto-merge is clean (no conflict markers) and stays green.

The blocking finding — normalizeHeading's idx map silently drops lookup visibility for pre-existing duplicate-by-case/whitespace headings in a.

Merge/MergeReport builds idx by iterating secsA and writing idx[normalizeHeading(s.heading)] = i — last write wins. Before this PR, idx was keyed by the exact heading string, so two on-disk sections differing only by case (a state the issue's own body cites as real: "Key Claims" vs "Key claims") never collided. After this PR, they do collide, and only the last one registered in idx is reachable for matching against b.

Reproduced directly (adversarial test, not in the PR's suite):

a := "## Key Claims\n\n- claim one\n- claim two\n## Other\n\nOther.\n## KEY CLAIMS\n\n- claim three\n"
b := "## Key Claims\n\n- claim one\n- brand new claim from extraction\n"

Result: the fresh bullet from b merges into the second, differently-cased duplicate (## KEY CLAIMS), not the first. - claim two in the original section never receives it — and won't on any future compile-on-ingest pass, since the same collision recurs every merge. No conflict is reported either (bullet/append sections don't go through MergeReport's disagreement path). This is exactly the bug class the file's own comment on parseSections warns about ("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") — reintroduced by this PR's own normalization, for any page with natural pre-existing heading-casing drift, which is precisely the failure mode #106 was filed to fix for newly arriving headings, not guard against for existing ones.

Not a hypothetical corpus state: any wiki page hand-edited or ingested before this normalization existed could carry such a duplicate today, and this PR silently starts routing all future updates to only one of them.

Not blocking, confirmed fine: bulletSectionsNorm/appendSectionsNorm are symmetric with the idx build/lookup sides (no naming/call-site asymmetry). Interaction with the preamble mechanism (heading "") is unaffected — preamble is never part of secsA. Composition with #104's MergeReport/canonicalBlock conflict-reporting (merged to main after this branch was cut) is textually clean and semantically consistent.

CI green: run 7858 (Lint/Test/Vet + Mirror), head SHA 3564498a1bddd65ce8546a2f3d4285b21614275b.

Recommendation: fix idx construction to detect same-a-side collisions (at minimum: keep the first match rather than silently overwriting, and ideally surface the pre-existing ambiguity rather than picking either silently) before merging. Reopening the loop on this file's own track record — this is bug #7 today, same class as #1–6.

## Review of PR #130 (feat/106-page-aware-extraction) — BLOCKED, not merged **What's solid:** `BuildPrompt`'s existing-page plumbing is real (empty string confirmed on a fresh source, non-empty only when `wiki/sources/<slug>.md` exists), the revision framing is scoped correctly to the source page only. `TestRun_RevisionEchoingExistingPageStaysByteIdentical` is genuinely end-to-end (Run → mocked `Complete` → `Merge` → written file) and uses `assert.Equal`, not `Contains`. Flip-tested `normalizeHeading` by making it a no-op: both `TestMerge_HeadingCaseDriftMatchesSameSection` and `TestMerge_HeadingWhitespaceDriftMatchesSameSection` went red as expected — not decoration. Full module suite (`go build ./... && go vet ./... && go test ./... -race -count=1`) green, 31 packages, no races — both on the PR branch tip and after merging current `main` (which now includes #104's `cdafdf6`, landed on main *after* this branch was cut) into it locally; the auto-merge is clean (no conflict markers) and stays green. **The blocking finding — normalizeHeading's `idx` map silently drops lookup visibility for pre-existing duplicate-by-case/whitespace headings in `a`.** `Merge`/`MergeReport` builds `idx` by iterating `secsA` and writing `idx[normalizeHeading(s.heading)] = i` — last write wins. Before this PR, `idx` was keyed by the exact heading string, so two on-disk sections differing only by case (a state the issue's own body cites as real: `"Key Claims"` vs `"Key claims"`) never collided. After this PR, they do collide, and only the *last* one registered in `idx` is reachable for matching against `b`. Reproduced directly (adversarial test, not in the PR's suite): ```go a := "## Key Claims\n\n- claim one\n- claim two\n## Other\n\nOther.\n## KEY CLAIMS\n\n- claim three\n" b := "## Key Claims\n\n- claim one\n- brand new claim from extraction\n" ``` Result: the fresh bullet from `b` merges into the **second**, differently-cased duplicate (`## KEY CLAIMS`), not the first. `- claim two` in the original section never receives it — and won't on any future compile-on-ingest pass, since the same collision recurs every merge. No conflict is reported either (bullet/append sections don't go through `MergeReport`'s disagreement path). This is exactly the bug class the file's own comment on `parseSections` warns about ("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") — reintroduced by this PR's own normalization, for any page with natural pre-existing heading-casing drift, which is precisely the failure mode #106 was filed to fix for *newly arriving* headings, not guard against for *existing* ones. Not a hypothetical corpus state: any wiki page hand-edited or ingested before this normalization existed could carry such a duplicate today, and this PR silently starts routing all future updates to only one of them. **Not blocking, confirmed fine:** `bulletSectionsNorm`/`appendSectionsNorm` are symmetric with the `idx` build/lookup sides (no naming/call-site asymmetry). Interaction with the preamble mechanism (heading `""`) is unaffected — preamble is never part of `secsA`. Composition with #104's `MergeReport`/`canonicalBlock` conflict-reporting (merged to main after this branch was cut) is textually clean and semantically consistent. CI green: run 7858 (Lint/Test/Vet + Mirror), head SHA `3564498a1bddd65ce8546a2f3d4285b21614275b`. **Recommendation:** fix `idx` construction to detect same-`a`-side collisions (at minimum: keep the first match rather than silently overwriting, and ideally surface the pre-existing ambiguity rather than picking either silently) before merging. Reopening the loop on this file's own track record — this is bug #7 today, same class as #1–6.
mathias commented 2026-09-23 05:42:01 +00:00
Author
Owner
Copy Link
Copy Source

Review of PR #132 — NOT merged, blocking finding

Rebase mechanics: clean. origin/main HEAD is cdafdf6 (#104's merge commit), exactly the PR's merge-base — no drift since opening. mergeable: true via API. var conflicts []string is declared before any append (only appended inside the sB loop), so its position relative to idx-building is provably inert — moving it earlier changes nothing observable. #104's canonicalBlock comparison and default: disagreement-report case are both present, unduplicated, unreordered in MergeReport. Confirmed via grep/git grep (not the PR body's claim) that the production write path is pipeline.go:230 reviseOrCreate → wiki.MergeReport directly — the fix is live on the real path, not the vestigial Merge wrapper (used elsewhere for pre-write raw-page dedup, pipeline.go:127, discards conflicts by design). CI green (run 7862, head 921bea8). go build/vet/test -race -count=1: 31 packages, all green, no races.

Blocking finding — MergeReport reports a false "source disagrees" conflict when merging a page against an unchanged copy of itself, if the page carries pre-existing duplicate-by-normalized-heading sections.

idx now keys by normalizeHeading, first occurrence wins, so a page with ## Notes and ## NOTES (different content — the exact drift #106 exists to handle) only exposes the first occurrence's index for lookup. When b's sections mirror a's structure (the "model echoes the existing page back verbatim" case #106's own body flags as expected and the review round required a byte-identical test for), b's second NOTES section gets compared against secsA[idx["notes"]] — the first occurrence — not its own counterpart. Since the two on-disk duplicates differ in content by construction, canonicalBlock sees a mismatch and appends a conflict, even though nothing changed between a and b.

This directly breaks the suite's own stated invariant, TestMerge_ReportsNothingWhenTheFreshVersionAgrees ("identical content is not a disagreement") — that test just never carries a pre-existing duplicate heading, so the gap wasn't visible. Reproduced live against MergeReport on the PR branch:

a := Page{Content: "## Notes\n\nSame text.\n\n## NOTES\n\nDifferent text (second dup).\n"}
MergeReport(a, a) // conflicts == ["NOTES"], should be empty

Effect: every wiki page with pre-existing case/whitespace-duplicate headings (confirmed real by prior review, cited in #106's own body) now emits a permanent, deterministic false-positive "source now disagrees on section... on-disk version kept" warning on every future ingest that echoes structure back — undermining #104's disagreement signal specifically for the corpus condition this PR was built to fix. Not data loss (content is unaffected), but it is new observable behavior on the production write path, since #130's version of this same commit fed the vestigial Merge wrapper that silently discarded conflicts — this bug was inert until this rebase wired it into MergeReport.

Other adversarial cases (3-way case collision with a genuine content disagreement, bullet-section union against a first-occurrence target with a pre-existing duplicate) behaved correctly — only the self-merge/no-op-echo case against a duplicate-heading target is wrong.

Not merged. Recommend: before matching a normalized idx collision, canonicalize/consolidate duplicate on-disk sections themselves (or compare sB against its own positional counterpart when b's section list itself contains the same duplicate structure) so an unchanged round-trip stays conflict-free even on already-drifted pages. #103/#105/#104 remain resolved; #106 is not closeable on this branch as-is — bug #8 on this file today.

## Review of PR #132 — NOT merged, blocking finding **Rebase mechanics: clean.** `origin/main` HEAD is `cdafdf6` (#104's merge commit), exactly the PR's merge-base — no drift since opening. `mergeable: true` via API. `var conflicts []string` is declared before any append (only appended inside the `sB` loop), so its position relative to `idx`-building is provably inert — moving it earlier changes nothing observable. #104's `canonicalBlock` comparison and `default:` disagreement-report case are both present, unduplicated, unreordered in `MergeReport`. Confirmed via `grep`/`git grep` (not the PR body's claim) that the production write path is `pipeline.go:230` `reviseOrCreate` → `wiki.MergeReport` directly — the fix is live on the real path, not the vestigial `Merge` wrapper (used elsewhere for pre-write raw-page dedup, `pipeline.go:127`, discards conflicts by design). CI green (run 7862, head `921bea8`). `go build/vet/test -race -count=1`: 31 packages, all green, no races. **Blocking finding — MergeReport reports a false "source disagrees" conflict when merging a page against an unchanged copy of itself, if the page carries pre-existing duplicate-by-normalized-heading sections.** `idx` now keys by `normalizeHeading`, first occurrence wins, so a page with `## Notes` and `## NOTES` (different content — the exact drift #106 exists to handle) only exposes the *first* occurrence's index for lookup. When `b`'s sections mirror `a`'s structure (the "model echoes the existing page back verbatim" case #106's own body flags as expected and the review round required a byte-identical test for), `b`'s second `NOTES` section gets compared against `secsA[idx["notes"]]` — the *first* occurrence — not its own counterpart. Since the two on-disk duplicates differ in content by construction, `canonicalBlock` sees a mismatch and appends a conflict, even though nothing changed between `a` and `b`. This directly breaks the suite's own stated invariant, `TestMerge_ReportsNothingWhenTheFreshVersionAgrees` ("identical content is not a disagreement") — that test just never carries a pre-existing duplicate heading, so the gap wasn't visible. Reproduced live against `MergeReport` on the PR branch: ```go a := Page{Content: "## Notes\n\nSame text.\n\n## NOTES\n\nDifferent text (second dup).\n"} MergeReport(a, a) // conflicts == ["NOTES"], should be empty ``` Effect: every wiki page with pre-existing case/whitespace-duplicate headings (confirmed real by prior review, cited in #106's own body) now emits a permanent, deterministic false-positive "source now disagrees on section... on-disk version kept" warning on every future ingest that echoes structure back — undermining #104's disagreement signal specifically for the corpus condition this PR was built to fix. Not data loss (content is unaffected), but it is new observable behavior on the production write path, since #130's version of this same commit fed the vestigial `Merge` wrapper that silently discarded `conflicts` — this bug was inert until this rebase wired it into `MergeReport`. Other adversarial cases (3-way case collision with a genuine content disagreement, bullet-section union against a first-occurrence target with a pre-existing duplicate) behaved correctly — only the self-merge/no-op-echo case against a duplicate-heading target is wrong. **Not merged.** Recommend: before matching a normalized `idx` collision, canonicalize/consolidate duplicate on-disk sections themselves (or compare `sB` against *its own* positional counterpart when `b`'s section list itself contains the same duplicate structure) so an unchanged round-trip stays conflict-free even on already-drifted pages. #103/#105/#104 remain resolved; #106 is not closeable on this branch as-is — bug #8 on this file today.
mathias commented 2026-09-23 05:46:16 +00:00
Author
Owner
Copy Link
Copy Source

Round 3 review of PR #132, head bdba49b — merged as 9a37881441.

What changed since round 2: MergeReport now pairs duplicate-heading occurrences positionally (byKey map[string][]int + cursor per key) instead of routing every occurrence of a normalized heading to a single index. This fixes the round-2 bug where merging a self-echoed page with pre-existing case-duplicate headings (e.g. "## Notes" / "## NOTES") falsely reported a conflict.

Independent verification performed:

  • Read full diff of merge.go + merge_test.go against prior head 921bea8.
  • Flip-tested the claim myself: forced pos := cursor[key] to pos := 0 (reverting to round-2 behavior). Result: TestMergeReport_SelfMergeOfPreExistingDuplicateHeadingsReportsNoConflict went red (reported a false [NOTES] conflict), while TestMerge_PreExistingCaseDuplicateHeadingsRouteToTheFirstOccurrence (the #130 regression test) stayed green — confirms first-occurrence-wins is the pos==0 special case, not a conflicting behavior.
  • Probed boundary conditions beyond the PR's own tests: 3 occurrences in a / 2 in b (a's untouched 3rd survives verbatim, no orphaning); 1 in a / 3 in b (b's extras append correctly without creating spurious duplicates, index bookkeeping around byKey[key]=append(...,len(secsA)) before secsA=append(secsA,sB) is correct); 2 in a / 0 in b (both survive verbatim); bullet-strategy heading with 2 dupes on each side (unions into the correct respective duplicate, no cross-leak). All four held — no ninth bug found.
  • go build ./... && go vet ./... && go test ./... -race -count=1 from ingestion/: all green.
  • Verified mergeable via API (not assumed): mergeable: true against current main at merge time.
  • Verified CI green on head bdba49b via commit status API: combined state success (CI / Lint / Test / Vet, run 7865).

The #103/#104/#105/#106 arc touching wiki/merge.go is now resolved: 8 real bugs found across 7 review rounds today, all fixed and verified, full test suite + race detector green, no regressions reintroduced by the fix-for-the-fix.

Round 3 review of PR #132, head bdba49b — merged as 9a37881441d54ea042cdf9f83a08f32146c620d1. **What changed since round 2**: MergeReport now pairs duplicate-heading occurrences positionally (byKey map[string][]int + cursor per key) instead of routing every occurrence of a normalized heading to a single index. This fixes the round-2 bug where merging a self-echoed page with pre-existing case-duplicate headings (e.g. "## Notes" / "## NOTES") falsely reported a conflict. **Independent verification performed:** - Read full diff of merge.go + merge_test.go against prior head 921bea8. - Flip-tested the claim myself: forced `pos := cursor[key]` to `pos := 0` (reverting to round-2 behavior). Result: TestMergeReport_SelfMergeOfPreExistingDuplicateHeadingsReportsNoConflict went red (reported a false [NOTES] conflict), while TestMerge_PreExistingCaseDuplicateHeadingsRouteToTheFirstOccurrence (the #130 regression test) stayed green — confirms first-occurrence-wins is the pos==0 special case, not a conflicting behavior. - Probed boundary conditions beyond the PR's own tests: 3 occurrences in a / 2 in b (a's untouched 3rd survives verbatim, no orphaning); 1 in a / 3 in b (b's extras append correctly without creating spurious duplicates, index bookkeeping around `byKey[key]=append(...,len(secsA))` before `secsA=append(secsA,sB)` is correct); 2 in a / 0 in b (both survive verbatim); bullet-strategy heading with 2 dupes on each side (unions into the correct respective duplicate, no cross-leak). All four held — no ninth bug found. - `go build ./... && go vet ./... && go test ./... -race -count=1` from ingestion/: all green. - Verified mergeable via API (not assumed): `mergeable: true` against current main at merge time. - Verified CI green on head bdba49b via commit status API: combined state `success` (CI / Lint / Test / Vet, run 7865). The #103/#104/#105/#106 arc touching wiki/merge.go is now resolved: 8 real bugs found across 7 review rounds today, all fixed and verified, full test suite + race detector green, no regressions reintroduced by the fix-for-the-fix.
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/m 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#106