From a75fd95ab84a21a83779c114d384f35a02e5e968 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 14:57:34 +0200 Subject: [PATCH 1/2] fix(frontmatter): consolidate 3 independent fence-parsers into one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hyperguild#138: internal/mcp/tools_notes.go's frontmatterBlock, internal/api/frontmatter.go's parseFrontmatter, and internal/frontmatter/frontmatter.go's split each independently detected a note's --- fence, converging on the same shape through parallel bug-fixing across today's #123/#126/#128 sagas. Each carried a subtly different gap: internal/frontmatter's closing-fence match had no trailing-newline requirement (a document ending "\n---extra" was treated as validly closed); internal/mcp needed CRLF/BOM guards bolted on across two separate review rounds after both silently defeated its own HasPrefix check; internal/api never gained either fix, so the same encoding-dependent bypass was live there too, just never triggered by a test. internal/frontmatter is the leaf package (imports nothing internal), so it becomes the canonical fence detector: exported Split(doc []byte) (block, rest []byte, ok bool), BOM-stripped and CRLF-normalized, with the stricter trailing-newline-required close. internal/api and internal/mcp now call it instead of re-detecting the fence themselves, keeping only what's genuinely package-specific (api's ordered fmLine round-trip shape, mcp's map + block- scalar-fold logic). Found two real regressions while wiring the callers, caught by each package's own pre-existing byte-exactness tests going red immediately: Split's block/rest boundary sits one byte differently than either caller's prior contract (it keeps the fence-adjacent newline on each side, by design for Fix's own reconstruction) — both callers now TrimSuffix/TrimPrefix exactly that one byte to restore their exact prior semantics. Neither regression would have been visible without those tests already existing. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/api/frontmatter.go | 33 ++++++++++----- ingestion/internal/frontmatter/frontmatter.go | 25 +++++++---- .../internal/frontmatter/frontmatter_test.go | 41 ++++++++++++++++++- ingestion/internal/mcp/tools_notes.go | 32 ++++++--------- 4 files changed, 93 insertions(+), 38 deletions(-) diff --git a/ingestion/internal/api/frontmatter.go b/ingestion/internal/api/frontmatter.go index 6177161..f0614b4 100644 --- a/ingestion/internal/api/frontmatter.go +++ b/ingestion/internal/api/frontmatter.go @@ -1,6 +1,12 @@ package api -import "strings" +import ( + "strings" + + // Aliased: this package's own frontmatter type (below) would collide + // with the imported package's name otherwise. + sharedfm "github.com/mathiasbq/hyperguild/ingestion/internal/frontmatter" +) // frontmatter is an ordered, line-preserving view of a note's YAML // frontmatter block. It deliberately avoids a full YAML round-trip: the @@ -25,19 +31,26 @@ type fmLine struct { // frontmatter block is recognised only when the file opens with a `---` // fence and a closing `---` fence follows. Otherwise the whole input is // the body and the returned frontmatter is empty. +// +// Fence detection itself is frontmatter.Split (hyperguild#138) — this +// package's own contribution is purely the ordered, line-preserving fmLine +// shape below, which a shared parser has no business owning since it's +// specific to how brain_update does a surgical, byte-round-trip edit. func parseFrontmatter(src string) (frontmatter, string) { var fm frontmatter - if !strings.HasPrefix(src, "---\n") { + blockBytes, restBytes, ok := sharedfm.Split([]byte(src)) + if !ok { return fm, src } - rest := src[len("---\n"):] - end := strings.Index(rest, "\n---\n") - if end < 0 { - // Opening fence with no closing fence — treat as bodyless content. - return fm, src - } - block := rest[:end] - body := rest[end+len("\n---\n"):] + // sharedfm.Split's block/rest boundary sits one byte earlier than this + // package's always has: block keeps the "\n" immediately before the + // closing fence, and rest keeps the "\n" immediately after it (both by + // design, for Fix's own reconstruction). This package's block/body + // never included either — TrimSuffix/TrimPrefix restore that exact + // boundary. Split guarantees ok=true only when "\n---\n" matched, so + // both trims are always exactly one byte, never ambiguous. + block := strings.TrimSuffix(string(blockBytes), "\n") + body := strings.TrimPrefix(string(restBytes), "\n") for _, line := range strings.Split(block, "\n") { // An indented line is a continuation (a YAML block-list item, a diff --git a/ingestion/internal/frontmatter/frontmatter.go b/ingestion/internal/frontmatter/frontmatter.go index e18079a..254d9db 100644 --- a/ingestion/internal/frontmatter/frontmatter.go +++ b/ingestion/internal/frontmatter/frontmatter.go @@ -66,7 +66,7 @@ func Fix(doc []byte) ([]byte, bool, error) { return doc, false, fmt.Errorf("frontmatter: quoting did not repair the block: %w", err) } - _, rest, _ := split(doc) + _, rest, _ := Split(doc) out := []byte(fence + "\n") out = append(out, fixed...) out = append(out, []byte(fence)...) @@ -122,20 +122,31 @@ func needsQuoting(value string) bool { // block returns the raw YAML between the opening and closing fences. func block(doc []byte) ([]byte, bool) { - b, _, ok := split(doc) + b, _, ok := Split(doc) return b, ok } -// split returns the frontmatter block and everything from the closing fence -// onward (the closing fence itself excluded, its trailing newline included). -func split(doc []byte) (block, rest []byte, ok bool) { - s := string(doc) +// Split returns doc's frontmatter block and everything from the closing +// fence onward (the closing fence itself excluded, its trailing newline +// included), or ok=false if doc has no properly-fenced frontmatter. +// +// The canonical fence detector for this codebase (hyperguild#138): three +// independent reimplementations of this same logic each grew their own gap +// through separate bug-fixing before being consolidated here — +// internal/mcp's needed a CRLF/BOM guard added after both silently defeated +// its own HasPrefix check, and internal/api's closing-fence match had no +// trailing-newline requirement, so a document ending in "\n---extra" with no +// real newline after the fence was treated as validly closed. Both classes +// are closed here, once, for every caller. +func Split(doc []byte) (block, rest []byte, ok bool) { + s := strings.TrimPrefix(string(doc), "\xef\xbb\xbf") + s = strings.ReplaceAll(s, "\r\n", "\n") if !strings.HasPrefix(s, fence+"\n") { return nil, nil, false } body := s[len(fence)+1:] - idx := strings.Index(body, "\n"+fence) + idx := strings.Index(body, "\n"+fence+"\n") if idx < 0 { return nil, nil, false } diff --git a/ingestion/internal/frontmatter/frontmatter_test.go b/ingestion/internal/frontmatter/frontmatter_test.go index 8f0c8a4..4945d57 100644 --- a/ingestion/internal/frontmatter/frontmatter_test.go +++ b/ingestion/internal/frontmatter/frontmatter_test.go @@ -168,9 +168,9 @@ func TestFix_PreservesBodyExactly(t *testing.T) { require.True(t, changed) assert.True(t, len(got) > 0) - _, rest, ok := split([]byte(doc)) + _, rest, ok := Split([]byte(doc)) require.True(t, ok) - _, gotRest, ok := split(got) + _, gotRest, ok := Split(got) require.True(t, ok) assert.Equal(t, string(rest), string(gotRest), "the body must not be touched") } @@ -186,3 +186,40 @@ func TestFix_RefusesToReturnAnUnparseableResult(t *testing.T) { assert.False(t, changed) assert.Equal(t, doc, string(got), "an unfixable document must come back byte-identical") } + +// TestSplit_NormalizesCRLFBeforeFenceDetection is hyperguild#138: a note +// whose fence lines use CRLF ("---\r\n") failed strings.HasPrefix(doc, +// "---\n") on the precondition read in internal/mcp's independent +// reimplementation -- encoding alone, no adversarial input, silently +// defeated fence detection there before this consolidation. Split must +// never repeat that. +func TestSplit_NormalizesCRLFBeforeFenceDetection(t *testing.T) { + doc := "---\r\ntitle: 'n'\r\nhalf_life: durable\r\n---\r\n\r\nbody\r\n" + block, rest, ok := Split([]byte(doc)) + require.True(t, ok) + assert.Contains(t, string(block), "half_life: durable") + assert.Contains(t, string(rest), "body") +} + +// TestSplit_StripsALeadingUTF8BOM is the same mechanism one layer earlier: +// a leading BOM (Notepad's default "UTF-8" save, PowerShell's pre-v6 +// Out-File/Set-Content) defeats the same HasPrefix check before CRLF +// normalization even applies. +func TestSplit_StripsALeadingUTF8BOM(t *testing.T) { + doc := "\xef\xbb\xbf---\ntitle: 'n'\n---\n\nbody\n" + block, rest, ok := Split([]byte(doc)) + require.True(t, ok) + assert.Contains(t, string(block), "title: 'n'") + assert.Contains(t, string(rest), "body") +} + +// TestSplit_RequiresANewlineAfterTheClosingFence closes the looser match +// internal/api's independent reimplementation carried (strings.Index(body, +// "\n"+fence) with no trailing-newline requirement): a document ending in +// "\n---extra" with no newline separating the fence from what follows it is +// not a valid closing fence and must not be treated as one. +func TestSplit_RequiresANewlineAfterTheClosingFence(t *testing.T) { + doc := "---\ntitle: 'n'\n---extra-not-a-real-close" + _, _, ok := Split([]byte(doc)) + assert.False(t, ok, "a fence line not followed by a newline is not a valid closing fence") +} diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index 0f9ba0c..36ce113 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -13,6 +13,7 @@ import ( "github.com/mathiasbq/hyperguild/ingestion/internal/capture" "github.com/mathiasbq/hyperguild/ingestion/internal/claim" "github.com/mathiasbq/hyperguild/ingestion/internal/classification" + "github.com/mathiasbq/hyperguild/ingestion/internal/frontmatter" "github.com/mathiasbq/hyperguild/ingestion/internal/notes" ) @@ -331,27 +332,20 @@ func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error { // frontmatterBlock returns the raw text between doc's opening and closing // `---` fences, or ok=false if doc has no (properly closed) fence. func frontmatterBlock(doc string) (block string, ok bool) { - // Round 4 found a note with CRLF fence lines ("---\r\n") silently - // defeats HasPrefix(doc, "---\n") on the precondition read — not an - // adversarial old_str/new_str, just an encoding difference — which - // makes checkClaimEnvelope no-op entirely. Round 5 found the same - // mechanism one layer earlier: a leading UTF-8 BOM (a live, plausible - // byte sequence — Notepad's default "UTF-8" save and PowerShell's - // pre-v6 Out-File/Set-Content both emit one) defeats the same prefix - // check before CRLF normalization even applies. Strip both, here, so - // every caller (hadFence/hasFence detection and frontmatterScalars) - // agrees regardless of encoding. - doc = strings.TrimPrefix(doc, "\xef\xbb\xbf") - doc = strings.ReplaceAll(doc, "\r\n", "\n") - if !strings.HasPrefix(doc, "---\n") { + // hyperguild#138: fence detection itself is frontmatter.Split, shared + // with internal/api and internal/frontmatter — the CRLF/BOM guards that + // used to live here (added piecemeal across rounds 4 and 5 of #123's + // review) are now the shared parser's job, so every caller inherits a + // fix instead of needing bug-for-bug parity edits. + b, _, ok := frontmatter.Split([]byte(doc)) + if !ok { return "", false } - rest := doc[len("---\n"):] - end := strings.Index(rest, "\n---\n") - if end < 0 { - return "", false - } - return rest[:end], true + // Split's block keeps the "\n" immediately before the closing fence + // (by design — see its own doc comment); this function never included + // it. Split guarantees ok=true only when "\n---\n" matched, so this is + // always exactly one byte, never ambiguous. + return strings.TrimSuffix(string(b), "\n"), true } // frontmatterScalars extracts top-level "key: value" scalar pairs from -- 2.54.0 From 625955e9442542e40f9e68e04a869e234f3e1d33 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 15:04:59 +0200 Subject: [PATCH 2/2] fix(frontmatter): accept a closing fence at literal end-of-file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review found a real regression: tightening Split's closing-fence match to require a trailing newline (closing the "\n---extra" bypass) also silently narrowed internal/frontmatter's own Lint/Fix behavior — a document whose closing fence is literally the last bytes of the file ("---\ntitle: x\n---", no body at all) used to parse fine via this package's private split, and now returns ok=false, contradicting this PR's own "zero behavior change for Lint/Fix" claim. internal/api and internal/mcp were unaffected (both already required the trailing newline pre-consolidation), but cmd/frontmatterlint's Lint/Fix relied on the old, looser acceptance. Split now distinguishes "fence followed by a newline" (real body follows) and "fence is the last bytes of the document" (no body, not a bypass attempt) — both valid — from "fence followed by anything else" (the actual bypass shape, still rejected). Regression test pins the literal-EOF case; flip-tested to confirm it's load-bearing. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/frontmatter/frontmatter.go | 17 +++++++++++++++-- .../internal/frontmatter/frontmatter_test.go | 15 +++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/ingestion/internal/frontmatter/frontmatter.go b/ingestion/internal/frontmatter/frontmatter.go index 254d9db..8548e33 100644 --- a/ingestion/internal/frontmatter/frontmatter.go +++ b/ingestion/internal/frontmatter/frontmatter.go @@ -146,9 +146,22 @@ func Split(doc []byte) (block, rest []byte, ok bool) { } body := s[len(fence)+1:] - idx := strings.Index(body, "\n"+fence+"\n") + idx := strings.Index(body, "\n"+fence) if idx < 0 { return nil, nil, false } - return []byte(body[:idx+1]), []byte(body[idx+1+len(fence):]), true + // What follows the closing fence must be either a newline (a real body + // follows) or nothing at all (the fence is the literal end of the + // document — no body, not a bypass attempt). Anything else, e.g. + // "\n---extra", is not a valid close: an earlier version of this check + // accepted it, which is exactly the looser match hyperguild#138 closes. + after := idx + 1 + len(fence) + switch { + case after == len(body): + return []byte(body[:idx+1]), nil, true + case body[after] == '\n': + return []byte(body[:idx+1]), []byte(body[after:]), true + default: + return nil, nil, false + } } diff --git a/ingestion/internal/frontmatter/frontmatter_test.go b/ingestion/internal/frontmatter/frontmatter_test.go index 4945d57..bda0ff0 100644 --- a/ingestion/internal/frontmatter/frontmatter_test.go +++ b/ingestion/internal/frontmatter/frontmatter_test.go @@ -223,3 +223,18 @@ func TestSplit_RequiresANewlineAfterTheClosingFence(t *testing.T) { _, _, ok := Split([]byte(doc)) assert.False(t, ok, "a fence line not followed by a newline is not a valid closing fence") } + +// TestSplit_AcceptsAClosingFenceThatIsTheLastBytesOfTheDocument is a +// regression pin found in review of hyperguild#138: the fix for the +// "\n---extra" bypass above must not also reject the legitimate case of a +// closing fence with nothing after it at all (no trailing newline because +// there's no body, not because a bypass is being attempted) -- this +// package's own private split() accepted that shape before the +// consolidation, and cmd/frontmatterlint's Lint/Fix relied on it. +func TestSplit_AcceptsAClosingFenceThatIsTheLastBytesOfTheDocument(t *testing.T) { + doc := "---\ntitle: 'n'\n---" + block, rest, ok := Split([]byte(doc)) + require.True(t, ok, "a closing fence at literal end-of-file, with nothing after it, is valid -- there is no body to require a newline before") + assert.Contains(t, string(block), "title: 'n'") + assert.Equal(t, "", string(rest)) +} -- 2.54.0