From 3d5af857135642918d79297d6731746b9eead56f Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 13:07:32 +0000 Subject: [PATCH] fix(frontmatter): consolidate 3 independent fence-parsers into one (#139) Co-authored-by: Mathias --- ingestion/internal/api/frontmatter.go | 33 +++++++---- ingestion/internal/frontmatter/frontmatter.go | 38 ++++++++++--- .../internal/frontmatter/frontmatter_test.go | 56 ++++++++++++++++++- ingestion/internal/mcp/tools_notes.go | 32 +++++------ 4 files changed, 121 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..8548e33 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,14 +122,25 @@ 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 } @@ -139,5 +150,18 @@ func split(doc []byte) (block, rest []byte, ok bool) { 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 8f0c8a4..bda0ff0 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,55 @@ 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") +} + +// 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)) +} 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