diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index df99825..b2c5269 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -187,10 +187,9 @@ func (s *Server) editWithScope(args json.RawMessage, scope notes.Scope) (notes.R // 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 { + // 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) } } @@ -245,42 +244,59 @@ 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 +// 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. -func validateEnvelopeAfterEdit(brainDir, path, oldStr, newStr string) error { +func checkClaimEnvelope(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 } + block, ok := frontmatterBlock(string(before)) + if !ok || !strings.Contains(block, oldStr) { + return nil // old_str isn't in the frontmatter block at all + } after := strings.Replace(string(before), oldStr, newStr, 1) 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) { + 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, @@ -288,15 +304,11 @@ func validateEnvelopeAfterEdit(brainDir, path, oldStr, newStr string) error { // 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") { + block, ok := frontmatterBlock(doc) + if !ok { 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") { + for _, line := range strings.Split(block, "\n") { if line != "" && (line[0] == ' ' || line[0] == '\t') { continue } diff --git a/ingestion/internal/mcp/tools_notes_test.go b/ingestion/internal/mcp/tools_notes_test.go index dbe6fcc..5989cde 100644 --- a/ingestion/internal/mcp/tools_notes_test.go +++ b/ingestion/internal/mcp/tools_notes_test.go @@ -116,6 +116,31 @@ func TestBrainEdit_RejectsAnInvalidClaimEnvelopeWrittenViaStringReplace(t *testi 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_AllowsAValidClaimEnvelopeEdit(t *testing.T) { dir := newBrainRepo(t) note := filepath.Join(dir, "wiki/homelab/facts/n.md")