Skip to content

amico-run(#112): B5 — real MCP stdio facade for amico mcp-serve - #126

Open
aarontrowbridge wants to merge 1 commit into
mainfrom
b5-mcp-facade
Open

amico-run(#112): B5 — real MCP stdio facade for amico mcp-serve#126
aarontrowbridge wants to merge 1 commit into
mainfrom
b5-mcp-facade

Conversation

@aarontrowbridge

Copy link
Copy Markdown
Member

Closes #112. Stacks on #118 (B1, b1-amico-cli-router) — base this PR's diff on that branch; merge #118 first.

What this lands

The OPTIONAL MCP facade (spec-20260708-112732 §7.3). B1 shipped amico mcp-serve as an SDK-free stub; B5 wires the real @modelcontextprotocol/sdk stdio transport. The spine verbs (catalog/vault/device/note) are exposed as MCP tools, and a tools/call dispatches to the same verb function the CLI uses — one impl, two transports, no fork.

Verb ↔ MCP-tool mapping

direction mapping
discovery (tools/list) each Verb in SPINE_VERBS → one MCP tool amico_<name>, description = verb.summary, inputSchema = { argv: string[] } (via verbToMcpTool / listMcpTools)
invocation (tools/call) amico_<name>callMcpTool(name, argv)the same verb.run(argv) the CLI dispatches; the verb's JSON result is returned as an MCP text block, non-zero exit → isError
transport serve() attaches StdioServerTransport; --list still renders the mapping transport-free

createMcpServer() registers both request handlers against the shared spine and attaches no transport, so it's exercised over an in-memory ClientServer pair in tests; serve() adds stdio and shuts down cleanly on client disconnect / stdin EOF (the SDK's StdioServerTransport watches stdin data/error only, not EOF, so serve() closes the server on stdin end to avoid a hang).

dist/amico.js bundles the SDK (620 kb); dist/amico-run.js stays SDK-free (340 kb) — cli.ts never imports the facade.

⚠️ S31 amendment (governance — needs Jack review)

This is the key review item. The fork's S31 guard (packages/amico-run/test/s31.test.ts) bans the literal modelcontextprotocol (plus HTTP/fetch) anywhere in src/. Landing a real MCP transport requires lifting that ban — done here as a narrow, named carve-out, mirroring the precedent where spec C amended the S31 SolveSpec ban with a single named carve-out (amico-run = launch gate) rather than a blanket removal.

Exactly what changed in s31.test.ts:

  • the /modelcontextprotocol/i pattern is pulled into a named MCP_SDK_PATTERN const and an allowlist MCP_SDK_ALLOWED = new Set(["mcp_serve.ts"]);
  • the grep loop skips only MCP_SDK_PATTERN for only mcp_serve.ts (if (re === MCP_SDK_PATTERN && MCP_SDK_ALLOWED.has(f)) continue;);
  • everything else stays fully banned: mcp_serve.ts is still subject to the HTTP (node:https?) and fetch( bans (the transport is stdio, never network) and the physics-flag bans; every other src/ file stays under the full ban including MCP;
  • two new assertions pin the carve-out so it can't silently widen: "the MCP-SDK carve-out is exactly one file" and "every non-carved-out src file is still MCP-SDK-free";
  • a loud amendment comment block documents what/why in-file.

No carve-out for orchestrator/harness code. The no-MCP/HTTP guard remains in full force everywhere except the one facade module.

Tests

pnpm --filter @amicode/amico-run test18 files, 128 tests pass; pnpm typecheck clean. New test/mcp_serve.test.ts (6 tests): pure mapping, direct callMcpTool dispatch, an in-memory ClientServer round-trip, and a real stdio round-trip spawning the built amico mcp-serve bundle via StdioClientTransport (proves tools/list + tools/call end-to-end over actual stdio). Amended s31.test.ts (3 tests) and updated amico.test.ts (14 tests, no-flag test now drives the real EOF-terminated server).

Note: fork CI may be RED due to the known private-fork vendored-binary token issue — unrelated to this change.

Do not merge

Awaiting Jack's governance sign-off on the S31 amendment, and #118 (B1) first.

🤖 Generated with Claude Code

Land the OPTIONAL MCP facade (spec §7.3): `amico mcp-serve` now stands up a
real @modelcontextprotocol/sdk stdio transport that exposes the spine verbs as
MCP tools. One impl, two transports — tools/call dispatches to the SAME verb
function (verbs.ts) the CLI uses; the CLI and MCP paths never fork.

- add @modelcontextprotocol/sdk@^1.29.0 to packages/amico-run
- mcp_serve.ts: createMcpServer() wires list-tools → verb↔tool mapping and
  call-tool → callMcpTool (the shared verb spine); serve() attaches the stdio
  transport and shuts down cleanly on client disconnect / stdin EOF
- amico.ts: mcp-serve is real (usage + header updated); --list still renders the
  mapping transport-free
- tests: mcp_serve.test.ts — pure mapping, direct dispatch, an in-memory
  Client↔Server round-trip, and a REAL stdio round-trip against the built bundle;
  amico.test.ts no-flag test updated to the real (EOF-terminated) server

S31 AMENDMENT (governance — needs Jack review): test/s31.test.ts carves the
`modelcontextprotocol` ban out for EXACTLY ONE file (src/mcp_serve.ts), mirroring
spec C's single-file SolveSpec carve-out. Only the MCP-SDK pattern is lifted, and
only for that file — mcp_serve.ts stays under the HTTP/fetch bans (stdio, never
network), and every other src file stays under the full ban. Two new assertions
pin the carve-out to one file so it cannot silently widen.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jack-champagne
jack-champagne changed the base branch from b1-amico-cli-router to main July 28, 2026 14:13
@jack-champagne

Copy link
Copy Markdown
Member

Retargeted from b1-amico-cli-router to main.

The old base is now fully merged into main, so the PR was comparing against a dead branch. Against main the real remaining change is 7 files, +747/-41 — that's what actually needs review here.

(Part of a sweep over the stacked b*/aaron/* chains: #119, #125 and #131 turned out to be entirely in main already and were closed; this one has real work left.)

@jack-champagne

Copy link
Copy Markdown
Member

@aarontrowbridge — rebased this onto current main to check landability. The conflicts themselves are mechanical (a dependency line, and prose in amico.ts that needed combining), and the S31 amendment is well done — single named carve-out, scoped to mcp_serve.ts, lifting only /modelcontextprotocol/i while leaving the HTTP and fetch bans on that file. That mirrors spec C's precedent exactly.

But the rebase surfaced something that isn't in this PR's design — it appeared because SPINE_VERBS grew underneath it.

listMcpTools() exposes every spine verb, and the spine now includes ledger

export function listMcpTools(): McpToolDescriptor[] {
  return SPINE_VERBS.map(verbToMcpTool);
}

SPINE_VERBS was 4 verbs when B5 was written. On current main it is 9:

catalog  vault  device  note  ledger  profile  fleet  spec  plan

ledger's verb surface (ledger_verb.ts:31):

amico ledger approve --plan-hash <h> [--max-solves <n>] [--tier <t>]

So as rebased, this registers amico_ledger as an MCP tool the agent can callapprove included.

That contradicts the capability-warrant provenance argument. From opencode#75's own header:

approve reaches amico ledger approve instead — the ledger's single writer. The separation is the point.

and the amicode_request_approval tool description already shipped on main:

This tool NEITHER records nor grants anything — pressing the button is what mints the warrant. … never approve on the user's behalf, and never shell amico ledger approve yourself.

The MCP facade would hand the agent that capability through a different door.

The way it failed is the part worth noting

The only symptom was a stale expectation in mcp_serve.test.ts:

AssertionError: expected [ 'amico_catalog', …(8) ] to deeply equal [ 'amico_catalog', …(3) ]
+   "amico_fleet"
+   "amico_ledger"
+   "amico_plan"
+   "amico_profile"
+   "amico_spec"

Update TOOL_NAMES to make that pass and you ship a self-approving agent with green CI. The test pinned the count, not the policy.

Suggestion

An explicit allowlist of MCP-exposed verbs rather than SPINE_VERBS.map(...), with the same named-carve-out discipline you applied to S31 — so adding a spine verb never silently widens the MCP surface. amico_spec, amico_plan and amico_fleet want the same look: plan/spec feed the launch gate, fleet touches remote execution.

I have not pushed the rebase — the branch is untouched at its original commit, so nothing here is lost. Everything else checks out: typecheck clean, and 942 pass / 12 skipped across the amico-run suite with only the 4 mcp_serve assertions failing on that name list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

B5: MCP facade (amico mcp-serve, optional)

2 participants