From e157311cd8524aa34c816692a840620137699e37 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 10:28:07 +0000 Subject: [PATCH] fix(mcp): validate the claim envelope before brain_edit writes too (#136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Mathias --- ingestion/internal/mcp/tools_notes.go | 175 +++++++++++++++ ingestion/internal/mcp/tools_notes_test.go | 237 +++++++++++++++++++++ 2 files changed, 412 insertions(+) diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index 866ffde..0f9ba0c 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -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 +} diff --git a/ingestion/internal/mcp/tools_notes_test.go b/ingestion/internal/mcp/tools_notes_test.go index b936e75..37dbf79 100644 --- a/ingestion/internal/mcp/tools_notes_test.go +++ b/ingestion/internal/mcp/tools_notes_test.go @@ -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)