From 0f932d8f774d62562369fcf0a59f9244c3dc2212 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 10:27:30 +0200 Subject: [PATCH] fix(mcp): drop the containment gate, always validate the post-edit envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review found a third bypass: checkClaimEnvelope required old_str to be fully contained within the extracted frontmatter block (which excludes both "---" fence lines). An old_str disambiguated across the closing fence into the body — a natural choice, since notes.Edit requires old_str to be unique — failed that containment test, so the gate skipped validation entirely while notes.Edit applied the replace verbatim, reaching the same invalid-enum outcome as the two bypasses already closed. Remove the containment gate structurally instead of patching another edge: checkClaimEnvelope now always computes the post-edit doc and validates its resulting frontmatter, regardless of old_str's textual relationship to the fence. A body-only edit leaves the frontmatter unchanged (still valid, since the note started valid), so this is a no-op for the common case and fail-closed for every case that touches the envelope, however old_str is phrased. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/mcp/tools_notes.go | 13 +++++++---- ingestion/internal/mcp/tools_notes_test.go | 27 ++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index b2c5269..72ad383 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -275,10 +275,15 @@ func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error { // — this function's job is the envelope, not read failures. return nil } - block, ok := frontmatterBlock(string(before)) - if !ok || !strings.Contains(block, oldStr) { - return nil // old_str isn't in the frontmatter block at all - } + // 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. after := strings.Replace(string(before), oldStr, newStr, 1) return claim.ValidateEnvelope(frontmatterScalars(after)) } diff --git a/ingestion/internal/mcp/tools_notes_test.go b/ingestion/internal/mcp/tools_notes_test.go index 5989cde..b32ae14 100644 --- a/ingestion/internal/mcp/tools_notes_test.go +++ b/ingestion/internal/mcp/tools_notes_test.go @@ -141,6 +141,33 @@ func TestBrainEdit_RejectsAValueOnlyEditThatBreaksTheEnvelope(t *testing.T) { 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_AllowsAValidClaimEnvelopeEdit(t *testing.T) { dir := newBrainRepo(t) note := filepath.Join(dir, "wiki/homelab/facts/n.md")