From f7fe00cecf5ec05e0ee43ea4d898d44258504542 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 10:13:52 +0200 Subject: [PATCH] fix(mcp): validate the claim envelope before brain_edit writes too hyperguild#123, the audit follow-up from #111: writeHallNote and UpdateNote both validate the claim envelope (half_life/applies/ claim_provenance) before writing; brain_edit didn't -- a byte-level string replace has no frontmatter awareness at all, so a caller could rewrite an existing envelope line (e.g. flip claim_provenance from asserted to guessed, or set half_life to a value outside the enum) straight past the validator. This was the one live, model-facing gap #123 identified; the ingestion pipeline write paths audited alongside it turned out not to touch these fields at all, so this closes the issue. Fixed by previewing what old_str->new_str would produce -- the exact strings.Replace(before, old_str, new_str, 1) formula notes.Edit itself uses internally -- and validating the envelope on that preview before the real edit is allowed to run. Fail-closed: an invalid result blocks the edit entirely, matching create/supersede's existing posture. Scoped to notes only (scope == notes.ScopeNotes) since governance files have no claim-envelope concept, and gated on a cheap substring check so an ordinary body edit that never mentions an envelope field name pays nothing extra. The preview doesn't replicate notes.Edit's uniqueness/CRLF/conflict- marker checks -- if old_str is absent or ambiguous, the preview may not match what Edit ultimately does, but Edit independently and correctly rejects those cases at write time regardless, so nothing about write safety rides on this preview's accuracy, only the envelope check itself. frontmatterScalars is a third, minimal implementation of the same indented-line-safe scalar extraction as the sibling packages (hyperguild#126/#128) -- deliberately not shared/exported across packages (each package's frontmatter handling has slightly different needs and none currently import each other), but built the same way from the start this time rather than needing a follow-up fix. Flip-tested: disabling the pre-check gate lets the invalid-envelope test fail (the edit silently succeeds); restoring it passes. Full module suite green. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/mcp/tools_notes.go | 80 ++++++++++++++++++++++ ingestion/internal/mcp/tools_notes_test.go | 72 +++++++++++++++++++ 2 files changed, 152 insertions(+) diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index 866ffde..df99825 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,21 @@ 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) and gated on a cheap substring check so an + // edit that never touches these field names pays nothing extra. + if scope == notes.ScopeNotes && touchesClaimEnvelope(a.OldStr, a.NewStr) { + if verr := validateEnvelopeAfterEdit(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 +244,67 @@ func touchesFrontmatter(oldStr, newStr string) bool { } return false } + +// touchesClaimEnvelope reports whether oldStr/newStr mention any of the +// three claim-envelope field names — the cheap gate that lets an ordinary +// body edit skip the read-and-validate round trip entirely. +func touchesClaimEnvelope(oldStr, newStr string) bool { + for _, s := range []string{oldStr, newStr} { + if strings.Contains(s, claim.FieldHalfLife) || strings.Contains(s, claim.FieldApplies) || strings.Contains(s, claim.FieldProvenance) { + return true + } + } + return false +} + +// validateEnvelopeAfterEdit previews what oldStr->newStr 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. +// +// 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. +func validateEnvelopeAfterEdit(brainDir, path, oldStr, newStr string) error { + 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 + } + after := strings.Replace(string(before), oldStr, newStr, 1) + return claim.ValidateEnvelope(frontmatterScalars(after)) +} + +// 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{} + if !strings.HasPrefix(doc, "---\n") { + return out + } + rest := doc[len("---\n"):] + end := strings.Index(rest, "\n---\n") + if end < 0 { + return out + } + for _, line := range strings.Split(rest[:end], "\n") { + if line != "" && (line[0] == ' ' || line[0] == '\t') { + continue + } + key, val, ok := strings.Cut(line, ":") + if !ok { + continue + } + out[strings.TrimSpace(key)] = strings.TrimSpace(val) + } + return out +} diff --git a/ingestion/internal/mcp/tools_notes_test.go b/ingestion/internal/mcp/tools_notes_test.go index b936e75..dbe6fcc 100644 --- a/ingestion/internal/mcp/tools_notes_test.go +++ b/ingestion/internal/mcp/tools_notes_test.go @@ -93,6 +93,78 @@ 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_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)