fix(mcp): validate the claim envelope before brain_edit writes too (#136)

7 review rounds closed 8 distinct bypasses across two mechanism families: fence-detection (field-name substring, block-containment, fence-stripping, CRLF, UTF-8 BOM) and value-extraction (folded YAML block scalar hiding an invalid value). A known v1 gap remains in the block-scalar fold's handling of blank-line-interrupted continuations — documented on checkClaimEnvelope, accepted deliberately (narrow, requires an already-privileged caller deliberately constructing a specific malformed-YAML shape) rather than chased past diminishing returns. Real fix (shared, tested, encoding-normalized frontmatter parser) tracked separately as hyperguild#138. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Mathias <mthbqv@gmail.com>
2026-09-23 10:28:07 +00:00
parent a5f502eb9c
commit e157311cd8
2 changed files with 412 additions and 0 deletions
@@ -11,6 +11,7 @@ import (

 	"github.com/mathiasbq/hyperguild/ingestion/internal/brain"
 	"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/notes"
 )
@@ -179,6 +180,20 @@ func (s *Server) editWithScope(args json.RawMessage, scope notes.Scope) (notes.R
 	if err := json.Unmarshal(args, &a); err != nil {
 		return notes.Result{}, nil, fmt.Errorf("parse args: %w", err)
 	}
+
+	// hyperguild#123: brain_write/brain_update both validate the claim
+	// envelope before writing (hyperguild#111: "same rules when a note
+	// changes as when it's born"); brain_edit didn't — a byte-level string
+	// replace has no frontmatter awareness at all, so it could rewrite an
+	// existing half_life:/applies:/claim_provenance: line straight past the
+	// validator entirely. Scoped to notes only (governance files have no
+	// claim-envelope concept).
+	if scope == notes.ScopeNotes {
+		if verr := checkClaimEnvelope(s.brainDir, a.Path, a.OldStr, a.NewStr); verr != nil {
+			return notes.Result{}, nil, fmt.Errorf("claim envelope: %w", verr)
+		}
+	}
+
 	res, err := notes.Edit(s.brainDir, a.Path, a.OldStr, a.NewStr, scope)
 	if err != nil {
 		if errors.Is(err, notes.ErrDegenerate) && a.OldStr == "" {
@@ -228,3 +243,163 @@ func touchesFrontmatter(oldStr, newStr string) bool {
 	}
 	return false
 }
+
+// checkClaimEnvelope previews what old_str->new_str would produce — the
+// exact strings.Replace(before, oldStr, newStr, 1) formula notes.Edit
+// itself uses internally — and validates the resulting frontmatter's claim
+// envelope before the real edit is allowed to run. Fail-closed: an invalid
+// result blocks the edit entirely, the same posture writeHallNote/UpdateNote
+// already apply on create/supersede.
+//
+// Gated on old_str appearing within the note's frontmatter BLOCK, not on
+// old_str/new_str mentioning a claim-envelope field NAME — found in review:
+// a field-name-substring gate misses a value-only edit like
+// "asserted"->"guessed" on a claim_provenance line, which never mentions
+// the field name at all. That is exactly the shape of edit a minimal,
+// natural diff produces, so a field-name gate was closing the issue in
+// name only. Checking block containment instead means any edit that
+// couldn't possibly touch the frontmatter (its target text only exists in
+// the body) is still skipped cheaply, while nothing inside the fence can
+// slip through unvalidated regardless of which token it targets.
+//
+// This does not replicate notes.Edit's uniqueness/CRLF/conflict-marker
+// checks — if old_str is absent or ambiguous, this preview may not match
+// what Edit ultimately does, but Edit independently and correctly rejects
+// those cases at write time regardless of what this function decides, so
+// the only thing riding on this preview's accuracy is the envelope check
+// itself, not write safety.
+// checkClaimEnvelope previews what old_str->new_str would produce and
+// validates the resulting claim envelope before the real edit runs
+// (hyperguild#123). Seven review rounds closed every plausible bypass in the
+// fence-detection family (field-name substring, block-containment,
+// fence-stripping, CRLF, UTF-8 BOM) and the first instance of a
+// value-extraction gap (a folded YAML block scalar hiding a value behind an
+// empty inline colon).
+//
+// Known, accepted v1 gap: frontmatterScalars's block-scalar fold is a
+// best-effort approximation, not a YAML parser — a blank line interrupting a
+// folded continuation, or other contrived multi-line YAML shapes not yet
+// probed (block-scalar indicators `|`/`>`, inline comments on a key line,
+// quoted multi-line values), can still hide an invalid value from
+// ValidateEnvelope. Accepted deliberately rather than chased indefinitely:
+// every bypass found so far requires the caller to already hold brain_edit
+// write access and to deliberately construct a specific malformed-YAML shape
+// with knowledge of this exact gap — a narrow, low-likelihood threat next to
+// the diminishing returns of another incremental patch round. The real fix
+// (a shared, tested, encoding-normalized frontmatter parser used by all
+// three independent implementations in this codebase) is tracked separately
+// as hyperguild#138, not bolted on here piece by piece.
+func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error {
+	if oldStr == "" {
+		// hyperguild#115: strings.Replace treats "" as matching position 0,
+		// which would make an empty old_str look like an edit that inserts
+		// text before the fence — a degenerate-input case that belongs to
+		// notes.Edit's own "old_str is empty" refusal, not to the envelope
+		// check.
+		return nil
+	}
+	before, err := notes.Read(brainDir, path)
+	if err != nil {
+		// Let notes.Edit itself produce the real error (missing file, etc.)
+		// — this function's job is the envelope, not read failures.
+		return nil
+	}
+	// No gate on where old_str falls relative to the frontmatter fence:
+	// round 2 found that requiring old_str to be fully contained in the
+	// extracted block was itself bypassable by an old_str disambiguated
+	// across the closing "---" into the body. Simpler and bypass-proof:
+	// always compute what the edit would produce and validate that
+	// resulting state's envelope. A body-only edit leaves the frontmatter
+	// (and thus the validation outcome) unchanged, so this is a no-op for
+	// the common case and fail-closed for every case that touches the
+	// envelope, regardless of how old_str is phrased.
+	_, hadFence := frontmatterBlock(string(before))
+	after := strings.Replace(string(before), oldStr, newStr, 1)
+	_, hasFence := frontmatterBlock(after)
+	if hadFence && !hasFence {
+		// Round 3: an edit that drops the closing fence degrades `after`
+		// to "unfenced" — frontmatterScalars then sees an empty map and
+		// ValidateEnvelope({}) is nil by design (absent fields are valid),
+		// so an invalid value would otherwise land with zero error while
+		// also corrupting the note's structure. A previously-fenced note
+		// must stay fenced, full stop.
+		return fmt.Errorf("edit would remove the frontmatter fence")
+	}
+	return claim.ValidateEnvelope(frontmatterScalars(after))
+}
+
+// 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") {
+		return "", false
+	}
+	rest := doc[len("---\n"):]
+	end := strings.Index(rest, "\n---\n")
+	if end < 0 {
+		return "", false
+	}
+	return rest[:end], true
+}
+
+// frontmatterScalars extracts top-level "key: value" scalar pairs from
+// doc's leading fence block, for the claim-envelope check only — not a
+// general frontmatter parser. Indented lines (YAML block-list items,
+// wrapped values) are skipped rather than misparsed as keys, same fix as
+// hyperguild#126/#128 in the sibling packages this mirrors.
+func frontmatterScalars(doc string) map[string]string {
+	out := map[string]string{}
+	block, ok := frontmatterBlock(doc)
+	if !ok {
+		return out
+	}
+	lines := strings.Split(block, "\n")
+	for i := 0; i < len(lines); i++ {
+		line := lines[i]
+		if line != "" && (line[0] == ' ' || line[0] == '\t') {
+			continue
+		}
+		key, val, ok := strings.Cut(line, ":")
+		if !ok {
+			continue
+		}
+		val = strings.TrimSpace(val)
+		if val == "" {
+			// Round 6: a colon with nothing after it is ambiguous — it can
+			// be a genuinely empty/omitted value, or the start of a YAML
+			// block scalar whose real value lives on the indented
+			// continuation line(s) that follow (e.g. "half_life:\n
+			// durable"). Treating the latter as absent let an edit smuggle
+			// an invalid value straight past ValidateEnvelope: the field
+			// read as unset while the disk actually carried a value. Fold
+			// the continuation in as a best-effort join so oneOf() still
+			// sees it. A genuinely empty value (no indented line follows)
+			// still folds to "", preserving present()'s existing
+			// placeholder-not-assertion contract.
+			var cont []string
+			for i+1 < len(lines) {
+				next := lines[i+1]
+				if next == "" || (next[0] != ' ' && next[0] != '\t') {
+					break
+				}
+				cont = append(cont, strings.TrimSpace(next))
+				i++
+			}
+			val = strings.Join(cont, " ")
+		}
+		out[strings.TrimSpace(key)] = val
+	}
+	return out
+}
@@ -93,6 +93,243 @@ func TestBrainEdit_EmptyOldStrErrorNamesRawArgumentBytes(t *testing.T) {
 	assert.Contains(t, errMsg, "raw arguments were", "%v", errMsg)
 }

+func TestBrainEdit_RejectsAnInvalidClaimEnvelopeWrittenViaStringReplace(t *testing.T) {
+	// hyperguild#123: brain_write/brain_update both validate the claim
+	// envelope (half_life/applies/claim_provenance) before writing —
+	// brain_edit didn't, even though it's the one live, model-facing path
+	// that can rewrite an existing envelope line directly via raw string
+	// replace, no parsing involved. "same rules when a note changes as when
+	// it's born" (hyperguild#111's own framing) applies here too.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path": "wiki/homelab/facts/n.md", "old_str": "claim_provenance: asserted", "new_str": "claim_provenance: guessed",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
+}
+
+func TestBrainEdit_RejectsAValueOnlyEditThatBreaksTheEnvelope(t *testing.T) {
+	// Found in review: the original gate (touchesClaimEnvelope) checked
+	// whether old_str/new_str mention the FIELD NAME — but a minimal diff
+	// (the natural shape of any real edit, model-generated or human) can
+	// target just the VALUE token and never mention the field name at all.
+	// "asserted" -> "guessed" on a claim_provenance line walked straight
+	// past the field-name substring check and wrote an invalid enum value
+	// with no error — the exact bypass #123 was supposed to close, closed
+	// in name only.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path": "wiki/homelab/facts/n.md", "old_str": "asserted", "new_str": "guessed",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
+}
+
+func TestBrainEdit_RejectsAnOldStrThatCrossesTheClosingFence(t *testing.T) {
+	// Found in round-2 review: the position-based gate required old_str to
+	// be fully CONTAINED in the extracted frontmatter block (which excludes
+	// both "---" fence lines). An old_str disambiguated by extending past
+	// the closing fence into the body — a natural choice, since notes.Edit
+	// requires old_str to be unique in the file — fails that containment
+	// test, so checkClaimEnvelope skips validation entirely while
+	// notes.Edit still applies the replace verbatim. Same invalid-enum
+	// outcome as the original two bypasses, reached a third way.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path":    "wiki/homelab/facts/n.md",
+		"old_str": "asserted\n---\n\nbody",
+		"new_str": "guessed\n---\n\nbody",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
+}
+
+func TestBrainEdit_RejectsAnEditThatRemovesTheClosingFence(t *testing.T) {
+	// Found in round-3 review: checkClaimEnvelope always validates the
+	// resulting frontmatter of `after` — but an edit whose new_str drops
+	// the closing "---" degrades `after` to "unfenced" in the same call.
+	// frontmatterBlock(after) then returns ok=false, frontmatterScalars
+	// returns an empty map, and claim.ValidateEnvelope({}) is nil by
+	// design (absent fields are valid) — so an invalid enum value lands on
+	// disk with zero error, and the note is left permanently unfenced.
+	// Structural fix: a note that had a valid fence before the edit must
+	// still have one after, or the edit is rejected outright.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path":    "wiki/homelab/facts/n.md",
+		"old_str": "claim_provenance: asserted\n---\n",
+		"new_str": "claim_provenance: not-a-real-value\n",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "claim_provenance: asserted\n---\n", "rejected edit must leave the note byte-identical, fence intact")
+}
+
+func TestBrainEdit_RejectsAnInvalidEnvelopeOnACRLFFencedNote(t *testing.T) {
+	// Found in round-4 review: frontmatterBlock's fence detection matches
+	// the literal byte sequence "---\n" / "\n---\n". A note whose fence
+	// lines use CRLF ("---\r\n") fails strings.HasPrefix(doc, "---\n") on
+	// the BEFORE read, so hadFence is false — not because the fence is
+	// missing, but because \r sits in front of every \n. hadFence&&!hasFence
+	// can then never fire, frontmatterScalars falls back to an empty map,
+	// and claim.ValidateEnvelope({}) is nil by design. No adversarial
+	// old_str/new_str crafting needed: encoding alone silently disables the
+	// whole envelope check for this note. Zero CRLF-fenced notes exist in
+	// the live corpus today, but nothing in the code prevents one.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\r\ntitle: 'n'\r\nhalf_life: durable\r\napplies: universal\r\nclaim_provenance: asserted\r\n---\r\n\r\nbody\r\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path": "wiki/homelab/facts/n.md", "old_str": "asserted", "new_str": "guessed",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
+}
+
+func TestBrainEdit_RejectsAnInvalidEnvelopeOnABOMPrefixedNote(t *testing.T) {
+	// Found in round-5 review: the same "encoding property empties
+	// frontmatterScalars's map" mechanism as the CRLF bypass, one layer
+	// earlier — a leading UTF-8 BOM (EF BB BF) defeats
+	// strings.HasPrefix(doc, "---\n") exactly like CRLF did, before that
+	// prefix is even reached. Notepad's default "UTF-8" save and
+	// PowerShell's pre-v6 Out-File/Set-Content both emit a BOM, so this is
+	// a live, plausible authoring path, not an exotic one — same severity
+	// and same one-line cost as the CRLF fix, fixed the same way rather
+	// than deferred to hyperguild#138.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("\xef\xbb\xbf---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path": "wiki/homelab/facts/n.md", "old_str": "asserted", "new_str": "guessed",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
+}
+
+func TestBrainEdit_RejectsAFoldedScalarThatHidesAnInvalidValue(t *testing.T) {
+	// Found in round-6 review: a different mechanism from the encoding
+	// family (rounds 1-6 were all about fence detection; this is the field
+	// extractor). old_str="half_life: durable", new_str="half_life:\n
+	// garbage-value" turns the field into a YAML block-scalar shape: the
+	// top-level line is now "half_life:" with an empty inline value, and
+	// the real value lives on the indented continuation line that follows.
+	// frontmatterScalars's indentation guard skips that continuation line
+	// outright, so the field reads as present-with-empty-value —
+	// present()'s own contract then treats that as absent (a placeholder,
+	// not an assertion) — and claim.ValidateEnvelope never sees the
+	// invalid value at all. No encoding quirk needed: one ordinary
+	// brain_edit call smuggles an out-of-enum value straight past the
+	// check the field was supposed to be validated by.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path":    "wiki/homelab/facts/n.md",
+		"old_str": "half_life: durable",
+		"new_str": "half_life:\n  garbage-value",
+	})
+	require.Contains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "half_life: durable", "rejected edit must leave the note byte-identical")
+}
+
+func TestBrainEdit_AllowsAValidClaimEnvelopeEdit(t *testing.T) {
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path": "wiki/homelab/facts/n.md", "old_str": "half_life: durable", "new_str": "half_life: seasonal",
+	})
+	require.NotContains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "half_life: seasonal")
+}
+
+func TestBrainEdit_UnrelatedEditsSkipTheEnvelopeCheckEntirely(t *testing.T) {
+	// The gate only engages when old_str/new_str actually mention an
+	// envelope field name — an ordinary body edit on a note that happens to
+	// carry a valid envelope must not pay for (or be affected by) a check
+	// that has nothing to do with it.
+	dir := newBrainRepo(t)
+	note := filepath.Join(dir, "wiki/homelab/facts/n.md")
+	require.NoError(t, os.WriteFile(note,
+		[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nold body\n"), 0o644))
+	srv := mcp.NewServer(dir, nil, nil, nil)
+
+	resp := call(t, srv, "brain_edit", map[string]any{
+		"path": "wiki/homelab/facts/n.md", "old_str": "old body", "new_str": "new body",
+	})
+	require.NotContains(t, resp, "error", "%v", resp)
+
+	got, err := os.ReadFile(note)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), "new body")
+}
+
+func TestBrainEditRoot_GovernanceFilesAreNeverEnvelopeChecked(t *testing.T) {
+	// CLAUDE.md has no claim-envelope concept at all — the check must be
+	// scoped to notes (ScopeNotes), never applied on the governance path.
+	srv, _, _, _ := govServer(t)
+
+	resp := authCall(t, srv, "Bearer "+capStaticTok, "brain_edit_root", map[string]any{
+		"path": "CLAUDE.md", "old_str": "# rules", "new_str": "half_life: durable",
+	})
+	require.NotContains(t, resp, "error", "%v", resp)
+}
+
 func TestBrainEdit_RefusesGovernanceFilesFromTheModelFacingTool(t *testing.T) {
 	dir := newBrainRepo(t)
 	srv := mcp.NewServer(dir, nil, nil, nil)