fix(frontmatter): consolidate 3 independent fence-parsers into one (#139)

Co-authored-by: Mathias <mthbqv@gmail.com>

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"):]
+	// sharedfm.Split's block/rest boundary sits one byte earlier than this
-	end := strings.Index(rest, "\n---\n")
+	// package's always has: block keeps the "\n" immediately before the
-	if end < 0 {
+	// closing fence, and rest keeps the "\n" immediately after it (both by
-		// Opening fence with no closing fence — treat as bodyless content.
+	// design, for Fix's own reconstruction). This package's block/body
-		return fm, src
+	// never included either — TrimSuffix/TrimPrefix restore that exact
-	}
+	// boundary. Split guarantees ok=true only when "\n---\n" matched, so
-	block := rest[:end]
+	// both trims are always exactly one byte, never ambiguous.
-	body := rest[end+len("\n---\n"):]
+	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

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
+// Split returns doc's frontmatter block and everything from the closing
-// onward (the closing fence itself excluded, its trailing newline included).
+// fence onward (the closing fence itself excluded, its trailing newline
-func split(doc []byte) (block, rest []byte, ok bool) {
+// included), or ok=false if doc has no properly-fenced frontmatter.
-	s := string(doc)
+//
+// 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
+	}
 }

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))
+}

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
+	// hyperguild#138: fence detection itself is frontmatter.Split, shared
-	// defeats HasPrefix(doc, "---\n") on the precondition read — not an
+	// with internal/api and internal/frontmatter — the CRLF/BOM guards that
-	// adversarial old_str/new_str, just an encoding difference — which
+	// used to live here (added piecemeal across rounds 4 and 5 of #123's
-	// makes checkClaimEnvelope no-op entirely. Round 5 found the same
+	// review) are now the shared parser's job, so every caller inherits a
-	// mechanism one layer earlier: a leading UTF-8 BOM (a live, plausible
+	// fix instead of needing bug-for-bug parity edits.
-	// byte sequence — Notepad's default "UTF-8" save and PowerShell's
+	b, _, ok := frontmatter.Split([]byte(doc))
-	// pre-v6 Out-File/Set-Content both emit one) defeats the same prefix
+	if !ok {
-	// 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") {
 		return "", false
 	}
-	rest := doc[len("---\n"):]
+	// Split's block keeps the "\n" immediately before the closing fence
-	end := strings.Index(rest, "\n---\n")
+	// (by design — see its own doc comment); this function never included
-	if end < 0 {
+	// it. Split guarantees ok=true only when "\n---\n" matched, so this is
-		return "", false
+	// always exactly one byte, never ambiguous.
-	}
+	return strings.TrimSuffix(string(b), "\n"), true
-	return rest[:end], true
 }
 
 // frontmatterScalars extracts top-level "key: value" scalar pairs from