Files
hyperguild/ingestion/internal/mcp/tools_notes.go
T
mathiasandClaude Sonnet 5 442557ca31
CI / Lint / Test / Vet (pull_request) Successful in 33s
CI / Mirror to GitHub (pull_request) Skipped
docs(mcp): document checkClaimEnvelope's accepted v1 gap
Round 7's review found an 8th bypass in the same "value hidden from the
extractor" family as round 6, inside the fold logic round 7's own fix had
just added (a blank line interrupting a folded continuation) — the second
distinct gap found on the first alternate shape tried against that fix.

Decision (Mathias, via the running sweep session): ship this PR's rounds
1-7 as v1 rather than continue incremental patches past the point of
diminishing returns. Every bypass found across all 7 rounds 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 — narrow and low-likelihood next to another patch cycle. The real fix
(one shared, tested, encoding-normalized frontmatter parser, replacing the
three independent hand-rolled implementations this bug class keeps
surfacing in) is tracked separately as hyperguild#138.

Document the boundary directly on checkClaimEnvelope so the next person
reading this function knows what's covered and what's a deliberate,
recorded trade-off rather than an unnoticed gap.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-23 12:25:30 +02:00

406 lines
18 KiB
Go

package mcp
import (
"context"
"encoding/json"
"errors"
"fmt"
"log/slog"
"strings"
"time"
"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"
)
// noteToolDescriptors are the model-facing halves of brain#4.
//
// brain_edit_root is deliberately absent: it dispatches but is not listed, the
// same de-surfacing ADR-0012 rule 4 applies to brain_index (#78). Hiding the
// verb matters here for a specific reason — brain content is ingested from the
// outside world and reaches a model's context, so a listed governance-file
// editor is a durable prompt-injection path: one poisoned page could rewrite
// the CLAUDE.md that a .dispatch-allow agent later obeys. A human who knows
// the tool exists can still call it; a model reading tools/list cannot
// discover it.
func noteToolDescriptors() []map[string]any {
str := func(d string) map[string]any { return map[string]any{"type": "string", "description": d} }
schema := func(required []string, props map[string]any) json.RawMessage {
b, _ := json.Marshal(map[string]any{"type": "object", "required": required, "properties": props})
return b
}
return []map[string]any{
{
"name": "brain_read",
"description": "Return a note's raw markdown verbatim — no synthesis, no excerpting. " +
"Use this to obtain the exact text before brain_edit; never build old_str from a " +
"brain_query excerpt, which is truncated and normalised and will not match.",
"inputSchema": schema([]string{"path"}, map[string]any{
"path": str("brain-relative path under knowledge/ or wiki/, e.g. wiki/homelab/facts/x.md"),
}),
},
{
"name": "brain_edit",
"description": "Replace exactly one occurrence of old_str with new_str in a note, leaving " +
"every other byte identical. Fails loudly if old_str is absent or matches more than " +
"once — extend old_str until it is unique. Does NOT stamp updated_at or supersedes " +
"(brain_update does); it is a byte-level patch, not a metadata operation.",
"inputSchema": schema([]string{"path", "old_str", "new_str"}, map[string]any{
"path": str("brain-relative path under knowledge/ or wiki/"),
"old_str": str("exact text to replace; must occur exactly once"),
"new_str": str("replacement text; empty string deletes old_str"),
}),
},
}
}
// --- hyperguild#97: governance-rewrite observability -----------------------
// govNotifier raises an out-of-band alert. Declared here rather than importing
// internal/audit so the MCP package keeps depending on capture alone.
type govNotifier interface {
Notify(ctx context.Context, msg string) error
}
// WithGovernanceObserver attaches the audit sink and notifier that observe a
// successful brain_edit_root. Both are optional; either may be nil.
func (s *Server) WithGovernanceObserver(sink capture.AuditSink, n govNotifier) {
s.govAudit, s.govNotify = sink, n
}
type brainReadArgs struct {
Path string `json:"path"`
}
func (s *Server) brainRead(_ context.Context, args json.RawMessage) (json.RawMessage, error) {
var a brainReadArgs
if err := json.Unmarshal(args, &a); err != nil {
return nil, fmt.Errorf("parse args: %w", err)
}
content, err := notes.Read(s.brainDir, a.Path)
if err != nil {
return nil, err
}
return json.Marshal(map[string]any{"path": a.Path, "content": string(content)})
}
type brainEditArgs struct {
Path string `json:"path"`
OldStr string `json:"old_str"`
NewStr string `json:"new_str"`
}
// brainEdit is the model-facing editor: notes only.
//
// Deliberately NOT audited (#97). A note edit is already fully reconstructible
// from git history, which carries the same before/after bytes plus a diff.
// Governance files get the extra trail because they change what later agents
// obey — the asymmetry is about instruction-carrying content, not about how
// hard the change is to reconstruct. Auditing every note edit would also bury
// the rare governance record under routine traffic, which is how an alert
// stops being read.
func (s *Server) brainEdit(_ context.Context, args json.RawMessage) (json.RawMessage, error) {
_, out, err := s.editWithScope(args, notes.ScopeNotes)
return out, err
}
// brainEditRoot edits a governance file (CLAUDE.md). De-surfaced AND gated on
// the authenticated principal: only the static local-CLI token may call it, so
// a claude.ai-origin session — the one that may have just read an untrusted
// page — cannot rewrite the instructions later agents load.
func (s *Server) brainEditRoot(ctx context.Context, args json.RawMessage) (json.RawMessage, error) {
info, ok := ctx.Value(principalKey).(principalInfo)
if !ok || !info.viaStatic {
return nil, fmt.Errorf("brain_edit_root requires the local static principal; " +
"a governance file is not editable from a model-facing origin")
}
res, out, err := s.editWithScope(args, notes.ScopeRoot)
if err != nil {
// A refusal is not a rewrite. Recording or alerting on one would cry
// wolf, and the gate already logs its own denials.
return out, err
}
// A durable instruction rewrite should be loud, not silent.
slog.Warn("brain_edit_root applied", "principal", info.principal, "result", string(out))
s.observeGovernanceEdit(ctx, info.principal, res)
return out, err
}
// observeGovernanceEdit records and alerts on a rewrite that already landed.
//
// Best-effort by construction: the bytes are written and brain-sync will commit
// them within the quarter hour, so failing the call here would report a
// non-rewrite to the caller while the rewrite sits on disk. A failure to
// observe is therefore logged at ERROR — the one case where the trail itself is
// what broke, and the only remaining signal.
func (s *Server) observeGovernanceEdit(ctx context.Context, principal string, res notes.Result) {
// Path and hashes only. The notification leaves the cluster for a phone
// via a third-party topic; a hash identifies the rewrite, the bytes would
// leak whatever was written into an instruction file.
msg := fmt.Sprintf("brain_edit_root applied\npath=%s\nprincipal=%s\nbefore=%s\nafter=%s",
res.Path, principal, res.PrevContentHash, res.ContentHash)
if s.govNotify != nil {
if err := s.govNotify.Notify(ctx, msg); err != nil {
slog.Error("brain_edit_root notify failed", "path", res.Path, "err", err)
}
}
if s.govAudit == nil {
return
}
// Internal, not Confidential: a governance file is not client data, and
// Confidential would make DegradingSink REFUSE to record while loki is
// down rather than buffer — losing exactly the record that matters most.
outcome, err := s.govAudit.Reserve(ctx, classification.Internal)
if err != nil {
slog.Error("brain_edit_root audit reserve failed", "path", res.Path, "err", err)
return
}
entry := capture.AuditEntry{
Timestamp: time.Now().UTC(),
Principal: principal,
Actor: "brain_edit_root",
Harness: "mcp",
EffectiveClassification: classification.Internal.String(),
Items: []string{res.Path},
SecurityEvents: []string{fmt.Sprintf(
"governance file rewritten: %s before=%s after=%s",
res.Path, res.PrevContentHash, res.ContentHash)},
}
if err := s.govAudit.Record(ctx, entry, outcome); err != nil {
slog.Error("brain_edit_root audit record failed", "path", res.Path, "err", err)
}
}
func (s *Server) editWithScope(args json.RawMessage, scope notes.Scope) (notes.Result, json.RawMessage, error) {
var a brainEditArgs
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 == "" {
// hyperguild#115: "old_str is empty" alone doesn't say whether the
// caller sent it empty or a hop between the caller and here lost
// it. Naming the raw argument byte count makes that distinguishable
// without a second investigation.
return notes.Result{}, nil, fmt.Errorf("%w (raw arguments were %d bytes)", err, len(args))
}
return notes.Result{}, nil, err
}
// Reindex only when the edit could have changed what the index reads.
// BuildWingIndex consumes title/created_at from the frontmatter block;
// BM25 walks the filesystem live and embeddings are mtime-driven, so an
// unconditional rebuild would buy nothing and dirty a second file —
// _index.md being precisely the class brain-sync already needs a bespoke
// conflict resolver for.
if wing := wingOf(res.Path); wing != "" && touchesFrontmatter(a.OldStr, a.NewStr) {
if ierr := brain.BuildWingIndex(s.brainDir, wing); ierr != nil {
// Best-effort, matching every other index hook here: the write
// already landed and will be committed, so failing the call would
// make the caller retry an edit whose old_str no longer exists.
res.ReindexError = ierr.Error()
}
}
out, merr := json.Marshal(res)
return res, out, merr
}
// wingOf returns the wing for a wiki path, or "" for knowledge/ and root.
func wingOf(rel string) string {
parts := strings.Split(rel, "/")
if len(parts) < 3 || parts[0] != "wiki" {
return ""
}
return parts[1]
}
// touchesFrontmatter reports whether either side of the edit mentions a field
// the wing index reads.
func touchesFrontmatter(oldStr, newStr string) bool {
for _, s := range []string{oldStr, newStr} {
if strings.Contains(s, "title:") || strings.Contains(s, "created_at:") || strings.Contains(s, "---") {
return true
}
}
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
}