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

Consolidate the three independent frontmatter-fence parsers #138

New Issue
Closed
opened 2026-09-23 08:39:52 +00:00 by mathias · 2 comments
mathias commented 2026-09-23 08:39:52 +00:00
Owner
Copy Link
Copy Source

Context

ingestion/internal/mcp/tools_notes.go#123's claim-envelope check for brain_edit (issue #123, PR #136) took 4 review rounds to close, each finding a structurally distinct bypass:

  1. Field-name substring gate — bypassed by a value-only diff.
  2. Position-based containment gate — bypassed by an old_str crossing the closing fence.
  3. Always-validate-after — bypassed by an edit that strips the closing fence, degrading to "unfenced" (empty map validates trivially).
  4. Fence-integrity check (hadFence && !hasFence) — bypassed by CRLF-fenced notes, where strings.HasPrefix(doc, "---\n") false-negatives on the precondition read purely from line-ending encoding, no adversarial crafting needed.

Round 4's reviewer's explicit judgment: "The fence-based, byte/string-matching strategy has now failed four structurally distinct ways across four rounds... continuing to patch checkClaimEnvelope in isolation is likely to keep producing new shapes of the same problem."

The actual root cause

Three independent reimplementations of "find the frontmatter fence" exist in this codebase, each with subtly different matching rules:

  • internal/mcp/tools_notes.go's frontmatterBlock — strings.HasPrefix(doc, "---\n") + strings.Index(rest, "\n---\n"), now CRLF-normalized (round 5's fix, this PR).
  • internal/api/frontmatter.go's parseFrontmatter/split.
  • internal/frontmatter/frontmatter.go's split — uses a looser "\n"+fence substring match with no trailing-newline requirement.

Nothing keeps them in lockstep. The CRLF bug in round 5 is a direct symptom: fixing it in one implementation doesn't fix it in the other two, and any future encoding/whitespace edge case has to be independently rediscovered and independently patched in up to three places (this has literally already happened once this session, for the indented-list-item / indentation-guard fix across internal/api/frontmatter.go, internal/wiki/merge.go, and this file's frontmatterScalars — hyperguild#126/#128/#123).

Recommendation

Consolidate into one shared, tested fence/frontmatter parser (normalizing line endings once at the boundary), imported by all three call sites, so this class of bug is closed by construction instead of by the next reviewer finding the next byte sequence that breaks a hand-rolled HasPrefix/Index pair.

Deliberately not done as part of #123/PR #136: that PR's scope is "brain_edit validates the claim envelope," and this is a larger cross-package refactor (internal/api, internal/wiki, internal/mcp currently deliberately don't import each other for this). Filing separately per the surgical-changes principle — fix the reported bug narrowly, track the structural improvement on its own ticket.

Source

Found during round 4 of pr-gatekeeper review on PR #136 (branch fix/123-brain-edit-envelope-check). Full round-4 finding: issue #123 comment.

Created via git-mcp on behalf of @mathias

## Context `ingestion/internal/mcp/tools_notes.go#123`'s claim-envelope check for `brain_edit` (issue #123, PR #136) took 4 review rounds to close, each finding a structurally distinct bypass: 1. Field-name substring gate — bypassed by a value-only diff. 2. Position-based containment gate — bypassed by an old_str crossing the closing fence. 3. Always-validate-`after` — bypassed by an edit that strips the closing fence, degrading to "unfenced" (empty map validates trivially). 4. Fence-integrity check (`hadFence && !hasFence`) — bypassed by CRLF-fenced notes, where `strings.HasPrefix(doc, "---\n")` false-negatives on the precondition read purely from line-ending encoding, no adversarial crafting needed. Round 4's reviewer's explicit judgment: *"The fence-based, byte/string-matching strategy has now failed four structurally distinct ways across four rounds... continuing to patch `checkClaimEnvelope` in isolation is likely to keep producing new shapes of the same problem."* ## The actual root cause Three independent reimplementations of "find the frontmatter fence" exist in this codebase, each with subtly different matching rules: - `internal/mcp/tools_notes.go`'s `frontmatterBlock` — `strings.HasPrefix(doc, "---\n")` + `strings.Index(rest, "\n---\n")`, now CRLF-normalized (round 5's fix, this PR). - `internal/api/frontmatter.go`'s `parseFrontmatter`/`split`. - `internal/frontmatter/frontmatter.go`'s `split` — uses a looser `"\n"+fence` substring match with no trailing-newline requirement. Nothing keeps them in lockstep. The CRLF bug in round 5 is a direct symptom: fixing it in one implementation doesn't fix it in the other two, and any future encoding/whitespace edge case has to be independently rediscovered and independently patched in up to three places (this has literally already happened once this session, for the indented-list-item / indentation-guard fix across `internal/api/frontmatter.go`, `internal/wiki/merge.go`, and this file's `frontmatterScalars` — hyperguild#126/#128/#123). ## Recommendation Consolidate into one shared, tested fence/frontmatter parser (normalizing line endings once at the boundary), imported by all three call sites, so this class of bug is closed by construction instead of by the next reviewer finding the next byte sequence that breaks a hand-rolled `HasPrefix`/`Index` pair. Deliberately not done as part of #123/PR #136: that PR's scope is "brain_edit validates the claim envelope," and this is a larger cross-package refactor (`internal/api`, `internal/wiki`, `internal/mcp` currently deliberately don't import each other for this). Filing separately per the surgical-changes principle — fix the reported bug narrowly, track the structural improvement on its own ticket. ## Source Found during round 4 of `pr-gatekeeper` review on PR #136 (branch `fix/123-brain-edit-envelope-check`). Full round-4 finding: issue #123 comment. --- _Created via git-mcp on behalf of @mathias_
mathias commented 2026-09-23 13:03:20 +00:00
Author
Owner
Copy Link
Copy Source

PR #139 review — not merged. Found a real, code-confirmed regression in internal/frontmatter.Split's tightened closing-fence match, in a category the review brief explicitly called out as disqualifying ("a behavior change in Lint/Fix").

What I independently verified

  1. Byte-offset claim (block/rest TrimSuffix/TrimPrefix in internal/api and internal/mcp) — traced by hand and confirmed with a standalone Go repro against the exact Split body: block always ends with exactly one \n (the separator's leading newline, regardless of how many blank lines precede the fence — verified with a doc containing a blank line directly before the closing fence), and rest always begins with exactly one \n (the fence's mandatory trailing newline). TrimSuffix/TrimPrefix remove exactly that one byte in every ok=true case and reproduce the old internal/api/internal/mcp contracts byte-for-byte. This part of the claim holds.

  2. Flip test on internal/api/frontmatter.go: removed the TrimSuffix/TrimPrefix compensation, ran TestParseFrontmatterSplitsHeaderAndBody, TestFrontmatterRenderRoundTrips, TestFrontmatterRenderEmptyValueKeyStaysBareNoTrailingSpace — all three went red for the exact claimed reason (spurious blank line before/after the fence, e.g. "---\nwing: a\nhall: facts\n\n---\n" vs expected "---\nwing: a\nhall: facts\n---\n"). Restored the compensation, all three green again. Claim holds.

  3. All six of #123's regression tests (CRLF fence, BOM prefix, old_str crossing the closing fence, edit removing the closing fence, folded scalar hiding an invalid value, plus the envelope tests) pass under the consolidated parser. go build ./... && go vet ./... && go test ./... -race -count=1 — all 31 packages green.

The regression

Split's closing-fence match was tightened from strings.Index(body, "\n"+fence) to strings.Index(body, "\n"+fence+"\n") (require a newline after the closing fence too, not just before it). For internal/api and internal/mcp, this is a no-op — both of their pre-existing hand-rolled parsers already required a trailing newline after the close, so no behavior changes there.

But internal/frontmatter's own private split (now Split) did not require that trailing newline before this PR. Consolidating onto the tightened match silently changes internal/frontmatter.Lint/Fix's own behavior — the thing the PR's own comments claim is "zero behavior change" ("Split replaces private split with identical external behavior for Lint/Fix's own call sites, just tightened for the closing-fence edge case").

Confirmed against the real package (not a reimplementation) with:

doc := []byte("---
title: x
---")  // closing fence is the last byte of the file, no trailing 
 at all
b, ok := block(doc)
// before this PR: ok=true, b="title: x
" — Lint/Fix would validate/repair it
// after this PR:  ok=false            — Lint returns nil (silently treated as "no frontmatter"), Fix no-ops

Impact: internal/frontmatter.Scan (wired to cmd/frontmatterlint, the repo-hygiene linter over brain/) will silently stop flagging a bodyless note whose frontmatter closing fence is the literal last bytes of the file (no trailing newline) as broken, even if its YAML doesn't parse — a false negative in the linter, not a crash. This does not touch the security-critical path: internal/mcp's checkClaimEnvelope and internal/api's parseFrontmatter both already required the trailing newline in their pre-PR implementations, so #123's claim-envelope validation is unaffected by this specific edge case.

Also noted, out of blocking scope but worth a follow-up issue: internal/wiki/merge.go's parseFrontmatterBlock (line 184) is a fourth hand-rolled HasPrefix/Index-based fence parser, not touched by this consolidation. It isn't named as one of the "three independent reimplementations" in #138's own root-cause section, so it's not a #138 acceptance-criteria violation — but it's the same class of drift #138 exists to close, and root_causing this fully would eventually want it folded into frontmatter.Split too.

Verdict: BLOCKED, not merged. The fix is small (add a no-newline-at-EOF test to frontmatter_test.go covering the bodyless/no-trailing-newline case, and either special-case it in Split or accept the narrowing explicitly with a comment + a Scan-level regression test) — recommend the author add the missing coverage and a one-line acknowledgment of the narrowed edge case before re-requesting review, or explicitly accept the narrowing as intentional with a comment explaining why it's safe for Scan's purposes.

CI: green (run 7946, job "Lint / Test / Vet", head sha a75fd95a, matches PR #139's exact head commit).

PR #139 review — **not merged**. Found a real, code-confirmed regression in `internal/frontmatter.Split`'s tightened closing-fence match, in a category the review brief explicitly called out as disqualifying ("a behavior change in Lint/Fix"). **What I independently verified** 1. Byte-offset claim (block/rest TrimSuffix/TrimPrefix in `internal/api` and `internal/mcp`) — traced by hand and confirmed with a standalone Go repro against the exact `Split` body: `block` always ends with exactly one `\n` (the separator's leading newline, regardless of how many blank lines precede the fence — verified with a doc containing a blank line directly before the closing fence), and `rest` always begins with exactly one `\n` (the fence's mandatory trailing newline). `TrimSuffix`/`TrimPrefix` remove exactly that one byte in every `ok=true` case and reproduce the old `internal/api`/`internal/mcp` contracts byte-for-byte. This part of the claim holds. 2. Flip test on `internal/api/frontmatter.go`: removed the `TrimSuffix`/`TrimPrefix` compensation, ran `TestParseFrontmatterSplitsHeaderAndBody`, `TestFrontmatterRenderRoundTrips`, `TestFrontmatterRenderEmptyValueKeyStaysBareNoTrailingSpace` — all three went red for the exact claimed reason (spurious blank line before/after the fence, e.g. `"---\nwing: a\nhall: facts\n\n---\n"` vs expected `"---\nwing: a\nhall: facts\n---\n"`). Restored the compensation, all three green again. Claim holds. 3. All six of #123's regression tests (CRLF fence, BOM prefix, old_str crossing the closing fence, edit removing the closing fence, folded scalar hiding an invalid value, plus the envelope tests) pass under the consolidated parser. `go build ./... && go vet ./... && go test ./... -race -count=1` — all 31 packages green. **The regression** `Split`'s closing-fence match was tightened from `strings.Index(body, "\n"+fence)` to `strings.Index(body, "\n"+fence+"\n")` (require a newline *after* the closing fence too, not just before it). For `internal/api` and `internal/mcp`, this is a no-op — both of their **pre-existing** hand-rolled parsers already required a trailing newline after the close, so no behavior changes there. But `internal/frontmatter`'s own private `split` (now `Split`) did **not** require that trailing newline before this PR. Consolidating onto the tightened match silently changes `internal/frontmatter.Lint`/`Fix`'s own behavior — the thing the PR's own comments claim is "zero behavior change" ("Split replaces private split with identical external behavior for Lint/Fix's own call sites, just tightened for the closing-fence edge case"). Confirmed against the real package (not a reimplementation) with: ```go doc := []byte("--- title: x ---") // closing fence is the last byte of the file, no trailing at all b, ok := block(doc) // before this PR: ok=true, b="title: x " — Lint/Fix would validate/repair it // after this PR: ok=false — Lint returns nil (silently treated as "no frontmatter"), Fix no-ops ``` Impact: `internal/frontmatter.Scan` (wired to `cmd/frontmatterlint`, the repo-hygiene linter over `brain/`) will silently stop flagging a bodyless note whose frontmatter closing fence is the literal last bytes of the file (no trailing newline) as broken, even if its YAML doesn't parse — a false negative in the linter, not a crash. This does **not** touch the security-critical path: `internal/mcp`'s `checkClaimEnvelope` and `internal/api`'s `parseFrontmatter` both already required the trailing newline in their pre-PR implementations, so #123's claim-envelope validation is unaffected by this specific edge case. **Also noted, out of blocking scope but worth a follow-up issue**: `internal/wiki/merge.go`'s `parseFrontmatterBlock` (line 184) is a fourth hand-rolled `HasPrefix`/`Index`-based fence parser, not touched by this consolidation. It isn't named as one of the "three independent reimplementations" in #138's own root-cause section, so it's not a #138 acceptance-criteria violation — but it's the same class of drift #138 exists to close, and root_causing this fully would eventually want it folded into `frontmatter.Split` too. **Verdict: BLOCKED, not merged.** The fix is small (add a no-newline-at-EOF test to `frontmatter_test.go` covering the bodyless/no-trailing-newline case, and either special-case it in `Split` or accept the narrowing explicitly with a comment + a `Scan`-level regression test) — recommend the author add the missing coverage and a one-line acknowledgment of the narrowed edge case before re-requesting review, or explicitly accept the narrowing as intentional with a comment explaining why it's safe for `Scan`'s purposes. CI: green (run 7946, job "Lint / Test / Vet", head sha a75fd95a, matches PR #139's exact head commit).
mathias commented 2026-09-23 13:08:22 +00:00
Author
Owner
Copy Link
Copy Source

PR #139 merged (squash commit 3d5af85713). Round-2 verification, independent of the author's claims.

Round 1 recap (previously posted here): found a real regression in Split's tightened closing-fence match — a document whose closing fence is the literal last bytes of the file ("---\ntitle: x\n---", no body at all) went from ok=true to ok=false, silently narrowing internal/frontmatter.Lint/Fix and contradicting the PR's "zero behavior change" claim. Blocked, not merged.

Round 2 — commit 625955e fix verified:

  1. Read the actual diff. Split now branches on three cases after the closing fence: after == len(body) (fence is EOF, valid), body[after] == '\n' (real body follows, valid), default (anything else, e.g. "\n---extra", invalid). Confirmed by reading the code directly — the after == len(body) bounds check is evaluated in the switch before body[after] is ever indexed, so there is no out-of-bounds read possible via case ordering.

  2. Flip-tested independently (not just trusting the new test):

    • Forced the after == len(body) case to return nil, nil, false → TestSplit_AcceptsAClosingFenceThatIsTheLastBytesOfTheDocument went red, TestSplit_RequiresANewlineAfterTheClosingFence stayed green. Restored, both green again.
    • Forced the default case to accept instead of reject → TestSplit_RequiresANewlineAfterTheClosingFence went red (the original round-1 bypass reopened), TestSplit_AcceptsAClosingFenceThatIsTheLastBytesOfTheDocument stayed green. Restored, both green again.
    • Both tests hold simultaneously at baseline and each is independently load-bearing for its own regression — neither is decoration.

  3. Reproduced round 1's exact repro by hand, independently, against the real package (not the new test): doc := []byte("---\ntitle: x\n---") → Split(doc) returns ok=true, block="title: x\n", rest="". The specific regression is gone.

  4. Adversarial pass for a fourth shape: probed "---\n" (opening fence only, empty body), "---\n---" and "---\n---\n" (fence-adjacent with no gap), bare "---" with no newline, empty doc "", and "---\n---extra" — all correctly return ok=false, no panic, no out-of-bounds access. Also checked the minimal legitimate empty-block case "---\n\n---" → ok=true, block="\n". No fourth gap found.

  5. Full gate re-run: go build ./..., go vet ./... both clean; go test ./... -race -count=1 — checked actual exit code (0) and grep -c FAIL (0) on the full log, not just tail output. All 31 packages (? on two with no test files) pass.

  6. CI: commit 625955e, run 7954, "CI / Lint / Test / Vet (pull_request)" = success. PR was mergeable, no conflicts.

Filed as follow-up, not blocking: #141 — internal/wiki/merge.go:184's parseFrontmatterBlock is a fourth hand-rolled fence parser, same drift class as #138, not named in #138's own scope and therefore not touched here. Tracked separately per round 1's note.

Verdict: MERGED.

PR #139 merged (squash commit 3d5af857135642918d79297d6731746b9eead56f). Round-2 verification, independent of the author's claims. **Round 1 recap (previously posted here):** found a real regression in `Split`'s tightened closing-fence match — a document whose closing fence is the literal last bytes of the file (`"---\ntitle: x\n---"`, no body at all) went from `ok=true` to `ok=false`, silently narrowing `internal/frontmatter.Lint`/`Fix` and contradicting the PR's "zero behavior change" claim. Blocked, not merged. **Round 2 — commit 625955e fix verified:** 1. Read the actual diff. `Split` now branches on three cases after the closing fence: `after == len(body)` (fence is EOF, valid), `body[after] == '\n'` (real body follows, valid), default (anything else, e.g. `"\n---extra"`, invalid). Confirmed by reading the code directly — the `after == len(body)` bounds check is evaluated in the switch *before* `body[after]` is ever indexed, so there is no out-of-bounds read possible via case ordering. 2. Flip-tested independently (not just trusting the new test): - Forced the `after == len(body)` case to `return nil, nil, false` → `TestSplit_AcceptsAClosingFenceThatIsTheLastBytesOfTheDocument` went red, `TestSplit_RequiresANewlineAfterTheClosingFence` stayed green. Restored, both green again. - Forced the `default` case to accept instead of reject → `TestSplit_RequiresANewlineAfterTheClosingFence` went red (the original round-1 bypass reopened), `TestSplit_AcceptsAClosingFenceThatIsTheLastBytesOfTheDocument` stayed green. Restored, both green again. - Both tests hold simultaneously at baseline and each is independently load-bearing for its own regression — neither is decoration. 3. Reproduced round 1's exact repro by hand, independently, against the real package (not the new test): `doc := []byte("---\ntitle: x\n---")` → `Split(doc)` returns `ok=true`, `block="title: x\n"`, `rest=""`. The specific regression is gone. 4. Adversarial pass for a fourth shape: probed `"---\n"` (opening fence only, empty body), `"---\n---"` and `"---\n---\n"` (fence-adjacent with no gap), bare `"---"` with no newline, empty doc `""`, and `"---\n---extra"` — all correctly return `ok=false`, no panic, no out-of-bounds access. Also checked the minimal legitimate empty-block case `"---\n\n---"` → `ok=true, block="\n"`. No fourth gap found. 5. Full gate re-run: `go build ./...`, `go vet ./...` both clean; `go test ./... -race -count=1` — checked actual exit code (0) and `grep -c FAIL` (0) on the full log, not just tail output. All 31 packages (`?` on two with no test files) pass. 6. CI: commit 625955e, run 7954, "CI / Lint / Test / Vet (pull_request)" = success. PR was mergeable, no conflicts. **Filed as follow-up, not blocking:** #141 — `internal/wiki/merge.go:184`'s `parseFrontmatterBlock` is a fourth hand-rolled fence parser, same drift class as #138, not named in #138's own scope and therefore not touched here. Tracked separately per round 1's note. Verdict: MERGED.
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
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#138