fix(frontmatter): consolidate 3 independent fence-parsers into one
CI / Lint / Test / Vet (pull_request) Successful in 34s
CI / Mirror to GitHub (pull_request) Skipped

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 <noreply@anthropic.com>
This commit is contained in:
2026-09-23 14:57:34 +02:00
co-authored by Claude Sonnet 5
parent e157311cd8
commit a75fd95ab8
4 changed files with 93 additions and 38 deletions
+18 -7
View File
@@ -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
}
@@ -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")
}