feat(store/mcp): #7 — mcp_servers proxy substrate (mcp_register + mcp-query, leashed, creds-by-reference)#58
Conversation
…-query) The credential-proxy piece of the agent-state-store epic (#7, PR D): present registered downstream MCP servers as a single modulex surface so a hotseat agent points its harness at modulex and never touches enterprise credentials. Store (modulex-core/store.rs): - McpServer struct + mcp_register/mcp_servers/mcp_server/mcp_unregister over the already-reserved mcp_servers table (name PK, command, args_json, note, created_gen — generation-stamped, never wall-clock). Upsert by name. - Wired into StoreDump export/import (sovereignty); the export carries the invocation shape only — never a credential. Tool (modulex-mcp/tools.rs): - mcp_register MCP tool (action add|list|remove) — a mutation, so a real tool. Lands in a new opt-in `mcp` facet: NOT on the default ≤12 tools/list budget (DEFAULT_TOOL_BUDGET stays 12, CI-pinned), discoverable + callable via the tool_search/tool_describe/tool_invoke trio, exactly like the board facet. Step (modulex-core/steps/mcp_query.rs): - mcp-query step: spawn a registered downstream as a stdio MCP client (initialize → notifications/initialized → tools/call over the existing single-shot Spawner seam — no new deps, lean build unaffected), embed ONLY the tool result as a report section. Security (this is the credential-proxy surface): - Leashed spawn: the downstream command goes through ExecGate::spawn → check_exec BEFORE any process; an ungranted command is DENIED, not run. The store cannot silently widen the leash — a registered server's command must be granted (inline `command` joins the declared-default grant via required_programs, or via [caveats] exec). - Credentials by reference: the downstream's secrets are resolved at spawn time into unserializable Secrets injected only into the child env; they never reach the store, an export, the report data/markdown, or error text. stderr is never surfaced (only parsed JSON-RPC error messages are). - Regression test asserts a resolved {cmd} credential value never appears in the serialized step result. Coverage: cargo llvm-cov on the new modules — mcp_query.rs 96.46% lines, store.rs 96.73%, tools.rs 93.79%, server.rs 96.89%; library total 95.03% lines (binary/pyo3 entrypoints excluded). Adds `just cov`/`cov-ci` and a CI `coverage` job enforcing an 80% floor, mirroring the test gate (hook parity note updated — coverage is CI-only, too slow pre-push). 3-tier tests: pure wire-shape (build_stdin/parse_call_response/render), mocked-spawner logic (happy path, denial, timeout, missing store/credential, downstream error, credential-leak regression), and server-level round-trip via tool_invoke. Golden schema pinned for the new step. Disclosure tiers: capability = mcp-query STEP (zero tool surface) + mcp_register MUTATION tool in the opt-in `mcp` facet (discovered, not listed). Refs #7. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stderr/stdout body
Adversarial review (DO-NOT-MERGE-AS-IS) found a real credential leak: a
{cmd} credential helper (gh auth token, vault, pass, custom wrappers) that
exits non-zero routinely echoes the very token it tried — on stdout AND
stderr. credentials.rs surfaced the trimmed stderr verbatim into
CredentialError::CommandFailed, which mcp_query embeds into the
agent-visible step result. The exec-gate scrub can't catch it: during a
credential's own resolution the secret is the command's OUTPUT, not in the
child env, so the scrub loop is empty.
Fix is in the SHARED resolver, so it closes the leak for all seven call
sites at once (project/gitlab/script/python/github/board_ingest + the new
mcp-query). Surface only the exit status, never the stream body. Adds the
adversarial regression (failing gh-auth helper echoing a token → token
absent from the error, exit status still reported).
Everything else in #58 held under review: the exec leash (argv exec, no
shell — injection-proof), store-can't-widen-the-grant, the result-echo
scrub, Secret unserializability, the ≤12 tool budget, generation stamps.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Adversarial security review — verdict: DO-NOT-MERGE-AS-IS → one fix applied, now MERGE. The review attacked credential-leak and exec-leash-bypass across every path. Everything held except one real leak, now fixed in commit The leak (Medium-High): a failing Held under attack (evidence in the review): exec leash runs One pre-existing note (not a #58 blocker): agent-bridle's exec scope matches by basename, so a bare grant Merging on green. |
Refs #7 (PR D of the agent-state-store epic — the mcp_servers proxy piece only).
Presents registered downstream MCP servers as a single modulex surface, so a hotseat agent points its harness at modulex and never touches enterprise credentials.
What this PR does
Store (
modulex-core/src/store.rs)McpServerstruct +mcp_register/mcp_servers/mcp_server/mcp_unregisterover the already-reservedmcp_serverstable (namePK,command,args_json,note,created_gen— generation-stamped, never wall-clock). Upsert by name.StoreDumpexport/import (sovereignty). The export carries the invocation shape only — never a credential.Tool (
modulex-mcp/src/tools.rs)mcp_registerMCP tool (action=add|list|remove). It is a mutation, so it is a legitimate tool (per modulex's rule: tools for mutations, step types for capability).mcpfacet — NOT on the defaulttools/list, soDEFAULT_TOOL_BUDGETstays 12 (CI-pinned). It is discoverable + callable via thetool_search/tool_describe/tool_invoketrio, exactly like the existingboardfacet.Step (
modulex-core/src/steps/mcp_query.rs)mcp-querystep: spawns a registered downstream as a stdio MCP client (initialize→notifications/initialized→tools/call, newline-delimited JSON-RPC 2.0) over the existing single-shotSpawnerseam — no new dependencies, lean (--no-default-features) build unaffected. Embeds only the tool result as a report section.Security / trust model (this is the credential-proxy surface)
ExecGate::spawn, which calls agent-bridle'scheck_execbefore any process exists (mcp_query.rsquery()→cx.exec.spawn, denial arm atsteps/mcp_query.rs:301-306). Anmcp-queryto a server whose command is not in the run's exec grant is denied, not run (state: "denied"). The store cannot silently widen the leash — a registered server's command must be granted: either a configmcp-querystep declarescommandinline (so it joins the declared-default grant viarequired_programs,steps/mcp_query.rs:167-170+:222-225) or it is listed in[caveats] exec.env = { NAME = {env|file|cmd} }references, resolved at spawn time intoSecrets (resolve_step_env,steps/mcp_query.rs:274) injected only into the child environment (ExecRequest.env).Secretis unserializable by construction, so it cannot enter the reportdata. Secrets never reach the store, an export, the report markdown/data, or error text.resultis embedded (steps/mcp_query.rs:320-331).stderris never surfaced (it could echo a credential); only parsed JSON-RPC error messages are reported. The exec gate also scrubs any injected secret from captured stdout/stderr as defense-in-depth before this code sees the output (exec.rs:258-264).argumentsback inside its own tool-result content. Thoseargumentscome from the calling step config, not from stored credentials, so no modulex-held secret is involved.Test plan
Coverage measured with
cargo llvm-cov(just cov-ci), library code (binary/pyo3 entrypoints excluded): 95.03% lines total; on the new modules:steps/mcp_query.rs(the step)store.rs(incl. mcp_servers accessors)tools.rs(incl.h_mcp_register)server.rs(new integration tests)Tests across modulex's three tiers:
build_stdin,parse_call_response(result / JSON-RPC error / missing / chatty stdout),render_result; happy path, ungranted-command denial (asserts never spawned), timeout, missing store, missing credential (names the var, not the value), downstream error, and a credential-leak regression asserting a resolved{cmd}secret never appears in the serialized step result.ExecGate::spawn, whose live leash + secret-scrub behavior is proven end-to-end intests/gated_exec.rs.mcp-query(tests/golden/mcp-query.json); the data-contract conformance harness validates executed-stepdataagainst it.tool_invoke(register → list → remove) + an unlisted-but-discoverable assertion for themcpfacet.Gates:
just check(fmt + clippy-D warnings+ tests) green; lean--no-default-featuresbuild green. Addsjust cov/cov-ciand a CIcoveragejob enforcing an 80% line floor (mirrors the test gate; the pre-push hook note records that coverage is CI-only — full instrumentation is too slow per push). Example config gains a commentedmcp-queryblock with the leash/credentials note.Disclosure tiers: capability =
mcp-querySTEP (zero tool surface) +mcp_registerMUTATION tool in the opt-inmcpfacet (discovered, not listed; budget stays 12).Out of scope
The rest of issue #7: reminders, countdowns, watches, iCal feeds, and the
url-watch/ical-searchsteps (PRs A–C). This PR is only themcp_serversproxy substrate (PR D). Per-generation result caching for deterministic replays (an open question on #7) is not implemented.🤖 Generated with Claude Code