From 63cb11d665134f1e1a64cda6cce9ff8b593ef124 Mon Sep 17 00:00:00 2001 From: Yash_10 Date: Sun, 4 Oct 2026 08:40:45 +0000 Subject: [PATCH 1/2] Keep built-in wallet tools working with an MCP server connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With mcp.json present, processLine sent every line to completeWithMcp, which completed with { mcp } alone: the nine built-in actions the system prompt advertises were never offered to the model, so a wallet request was a no-op and a scripted run still exited 0. A built-in call the model did make was answered by the server's catalog instead of our safety boundary. completeWithMcp now takes the agent's own tool definitions alongside mcp (an empty list keeps the request byte-identical to the MCP-only shape) and routes by name: built-in names go to invokeNativeToolCall, server names to the existing invokeToolCall gate. Both turn drivers share dispatchNativeCall, so a write and an account switch reach the same handleAction boundary — resolve, policy, preview, mainnet ack, y/N — that a slash command uses, while reads go through dispatchToolCall and are printed once. Refusals, tool errors and a hit round limit fold into hadFailure, so a scripted run still exits 1. The MCP confirmation gate is untouched: a server tool still asks, and a declined call never reaches the server. The turn body lives in src/nativeToolLoop.mjs beside the native loop and is reachable from tests; cli.mjs only passes in the handles it needs, and exports invokeMcpToolCall next to handleAction so the gate is testable. Tested: npm run build, npm test (573 pass), npm run smoke (SMOKE_OK). --- src/agent.mjs | 37 +++- src/cli.mjs | 63 +++---- src/nativeToolLoop.mjs | 185 ++++++++++++++++++-- test/agent-mcp.test.mjs | 80 +++++++++ test/helpers/mcp-stdio-server.mjs | 70 ++++++++ test/helpers/shim-qvac.mjs | 93 ++++++++-- test/mcp-cli.test.mjs | 167 ++++++++++++++++++ test/mcp-native-dispatch.test.mjs | 278 ++++++++++++++++++++++++++++++ 8 files changed, 910 insertions(+), 63 deletions(-) create mode 100644 test/helpers/mcp-stdio-server.mjs create mode 100644 test/mcp-cli.test.mjs create mode 100644 test/mcp-native-dispatch.test.mjs diff --git a/src/agent.mjs b/src/agent.mjs index bff9790..c71b699 100644 --- a/src/agent.mjs +++ b/src/agent.mjs @@ -77,9 +77,19 @@ export async function complete(history, onToken) { * the model stops calling tools — or `maxToolRounds` is hit, so a runaway * tool-call loop cannot hang the agent forever. * - * `invokeToolCall(call)` executes one call and must resolve to a string for - * history; this function has no UI, so confirmation/refusal is entirely the - * caller's decision (cli.mjs gates every call behind a y/N prompt). + * `tools` are this agent's OWN built-in tools, advertised to the same model in + * the same request as the MCP servers' (the SDK merges both lists). Advertised + * is not enough on its own: a call whose name is one of ours is a wallet action + * and must go to `invokeNativeToolCall`, which routes writes through + * handleAction's resolve → policy → preview → confirm boundary. MCP tools have + * no such boundary, so they go to `invokeToolCall` instead — cli.mjs gates that + * one behind its own y/N prompt. Routing by name is what keeps an MCP server + * from ever being handed a `send_mon`, and keeps a wallet action from being + * answered by an MCP handler. + * + * `invokeToolCall(call)` / `invokeNativeToolCall(call)` execute one call and + * must resolve to a string for history; this function has no UI, so + * confirmation/refusal is entirely the caller's decision. * * `runCompletion` is a test seam — it defaults to QVAC's own `completion()`, * lazily imported like every other QVAC call in this file, but a caller can @@ -92,16 +102,22 @@ export async function complete(history, onToken) { */ export async function completeWithMcp( history, - { mcpClients = [], onToken, invokeToolCall, maxToolRounds = 8, runCompletion } = {}, + { mcpClients = [], tools = [], onToken, invokeToolCall, invokeNativeToolCall, maxToolRounds = 8, runCompletion } = {}, ) { const doCompletion = runCompletion ?? (await qvac()).completion; const mcp = mcpClients.map((c) => ({ client: c.client, includeResources: false })); + // Membership decides the handler. An empty list keeps the completion request + // byte-identical to the MCP-only behaviour it replaces. + const nativeToolNames = new Set(tools.map((t) => t.name)); const toolErrors = []; let text = ""; let rounds = 0; for (;;) { - const run = doCompletion({ modelId, history, mcp, stream: true }, { timeout: 300_000 }); + const run = doCompletion( + { modelId, history, ...(tools.length ? { tools } : {}), mcp, stream: true }, + { timeout: 300_000 }, + ); try { for await (const event of run.events) { if (event.type === "contentDelta") { @@ -132,6 +148,17 @@ export async function completeWithMcp( // caller to push, matching how complete() leaves that to its caller too. history.push({ role: "assistant", content: text }); for (const call of toolCalls) { + const isNative = nativeToolNames.has(call.name); + if (isNative) { + if (!invokeNativeToolCall) { + throw new Error( + `completeWithMcp: invokeNativeToolCall is required when the model calls the built-in tool "${call.name}"`, + ); + } + const result = await invokeNativeToolCall(call); + history.push({ role: "tool", content: String(result ?? "") }); + continue; + } if (!invokeToolCall) { throw new Error("completeWithMcp: invokeToolCall is required when the model calls a tool"); } diff --git a/src/cli.mjs b/src/cli.mjs index 5703fb6..238bd1b 100644 --- a/src/cli.mjs +++ b/src/cli.mjs @@ -95,7 +95,7 @@ import { getToolDefinitions, dispatchToolCall, } from "./tools.mjs"; -import { runNativeToolLoop } from "./nativeToolLoop.mjs"; +import { runNativeToolLoop, runMcpTurn } from "./nativeToolLoop.mjs"; /** * Build the REPL's initial history with the prompt matching the enabled @@ -241,6 +241,9 @@ async function confirm(question) { * MCP server exposes, discovered at runtime, so there is no reliable way to tell * a read apart from a write here the way isWrite() does for the built-in actions * — asking every time is the safe default, not a guess. + * + * Exported alongside handleAction so a test can prove the gate still refuses a + * call the operator has not approved, now that built-in tools share the turn. */ async function invokeMcpToolCall(call) { println("\n " + c.yellow(`MCP tool: ${call.name}(${JSON.stringify(call.arguments ?? {})})`)); @@ -607,39 +610,41 @@ async function main() { printw(" " + DIM); if (mcpClients.length) { - // MCP servers are connected: let QVAC call their tools directly, feeding - // each result back for a follow-up turn (completeWithMcp owns that loop). - // The model's raw output streams dimmed; tool-call confirmations and - // results print bright, same as everything else that needs an operator. - let result; + // MCP servers are connected, but so is our own toolset: the prompt below + // promises send_mon/get_balance/… and the model must reach the SAME + // handleAction boundary a slash command would, while each server's tools + // keep their own confirmation gate. The turn body — tools advertised + // alongside mcp, writes → handleAction, reads → dispatchToolCall, refusals + // and tool errors folded into hadFailure — lives in src/nativeToolLoop.mjs + // beside the native loop that shares its boundary; here we just pass in the + // handles it needs, exactly like that loop. + const hadFailureRef = { value: hadFailure }; try { - result = await brain.completeWithMcp(history, { + await runMcpTurn({ + history, mcpClients, + completeWithMcp: brain.completeWithMcp, + getToolDefinitions, + handleAction, + dispatchToolCall, + isWrite, onToken: (t) => printw(t), - invokeToolCall: async (call) => { - printw(RST + "\n"); - const out = await invokeMcpToolCall(call); - printw(DIM); - return out; - }, + invokeToolCall: invokeMcpToolCall, + printw, + DIM, + RST, + println, + c, + SCRIPTED, + hadFailure: hadFailureRef, }); - printw(RST + "\n"); + hadFailure = propagateHadFailure(hadFailureRef, hadFailure); } catch (err) { printw(RST); println(c.red(` model error: ${err.message}`) + "\n"); if (SCRIPTED) hadFailure = true; return true; } - history.push({ role: "assistant", content: result.text }); - for (const e of result.toolErrors) { - println(c.red(` tool error [${e.code}]: ${e.message}`)); - if (SCRIPTED) hadFailure = true; - } - if (result.limitReached) { - println(c.yellow(` (stopped after ${result.rounds} tool-call rounds — the model kept calling tools)`)); - if (SCRIPTED) hadFailure = true; - } - println(""); return true; } @@ -734,10 +739,10 @@ async function main() { } // Test seam: importing this module with NAD_CLI_NO_RUN=1 loads its exports -// (handleAction, buildInitialHistory, propagateHadFailure) without starting the -// REPL — which would otherwise drain stdin, load the model, and call -// process.exit. Production entry points (npm start, node dist/cli.mjs) never set -// it, so runtime behavior is unchanged. +// (handleAction, invokeMcpToolCall, buildInitialHistory, propagateHadFailure) +// without starting the REPL — which would otherwise drain stdin, load the +// model, and call process.exit. Production entry points (npm start, node +// dist/cli.mjs) never set it, so runtime behavior is unchanged. if (!process.env.NAD_CLI_NO_RUN) { main().catch((err) => { console.error(err); @@ -745,4 +750,4 @@ if (!process.env.NAD_CLI_NO_RUN) { }); } -export { handleAction }; +export { handleAction, invokeMcpToolCall }; diff --git a/src/nativeToolLoop.mjs b/src/nativeToolLoop.mjs index 0bc5754..631361a 100644 --- a/src/nativeToolLoop.mjs +++ b/src/nativeToolLoop.mjs @@ -8,6 +8,13 @@ * from tests. The full REPL also imports this: every dependency (model, dispatch, * print helpers, mode flags) is passed in, so the same code drives both the * interactive REPL and a scripted test. + * + * This file hosts BOTH turn drivers and the one boundary they share: + * - runNativeToolLoop — MCP not connected: `completeWithTools` drives the turn. + * - runMcpTurn — MCP connected: `completeWithMcp` drives the turn with + * the built-in tools advertised alongside the servers', + * and every call routed through dispatchNativeCall below. + * Both exist so a native write cannot reach the wallet by any other road. */ import { isRefusal } from "./tools.mjs"; @@ -32,6 +39,45 @@ export function formatToolError(toolErr) { return code ? `[${code}]: ${text}` : text; } +/** + * Route ONE native tool call through the safety boundary. This is the seam both + * turn drivers share, and the reason it exists is a write: + * + * ROUTE THROUGH THE SAFETY BOUNDARY. + * + * Writes (send_mon, send_token, transfer_nft, swap) and account-switch (account + * with an index) MUST go through handleAction so they share the recipient + * resolution, spend policy, preview, mainnet ack, and y/N confirmation the v0 + * path and the slash commands already use. Anything else is a read — + * dispatchToolCall is fine and stays snappy. + * + * Returns `{ result, boundary }`. `boundary` says who printed it: handleAction + * prints its own results and refusals as it produces them, dispatchToolCall + * prints nothing — so a caller that also printed every result would show writes + * twice and reads never. + * + * Throws are left to the caller: both turn drivers turn them into an + * `Error: …` result and a scripted failure. + * + * @param call { name, arguments } — one tool call from the model. + * @param deps { handleAction, dispatchToolCall, isWrite } — injected by cli.mjs + * so tests can prove which side of the seam answered. + */ +export async function dispatchNativeCall( + { name, arguments: args = {} }, + { handleAction, dispatchToolCall, isWrite }, +) { + const isAccountSwitch = + name === "account" && + args.index !== undefined && + args.index !== null && + args.index !== ""; + if (isWrite(name) || isAccountSwitch) { + return { result: await handleAction({ action: name, ...args }), boundary: "handleAction" }; + } + return { result: await dispatchToolCall(name, args), boundary: "dispatchToolCall" }; +} + /** * Run one full tool-turn loop against `history` (mutated in place: assistant and * tool messages are appended). The loop: @@ -118,26 +164,13 @@ export async function runNativeToolLoop({ // Dispatch each tool call and collect results for history. const toolResults = []; for (const toolCall of result.toolCalls) { - const action = { action: toolCall.name, ...toolCall.arguments }; let execResult = null; try { - // ROUTE THROUGH THE SAFETY BOUNDARY. - // - // Writes (send_mon, send_token, transfer_nft, swap) and account-switch - // (account with an index) MUST go through handleAction so they share the - // recipient resolution, spend policy, preview, mainnet ack, and y/N - // confirmation the v0 path and the slash commands already use. Anything - // else is a read — dispatchToolCall is fine and stays snappy. - const isAccountSwitch = toolCall.name === "account" && - toolCall.arguments.index !== undefined && - toolCall.arguments.index !== null && - toolCall.arguments.index !== ""; - if (isWrite(toolCall.name) || isAccountSwitch) { - execResult = await handleAction(action); - } else { - execResult = await dispatchToolCall(toolCall.name, toolCall.arguments); - } + // Through the shared boundary: writes → handleAction, reads → + // dispatchToolCall (see dispatchNativeCall above). + const routed = await dispatchNativeCall(toolCall, { handleAction, dispatchToolCall, isWrite }); + execResult = routed.result; } catch (err) { execResult = `Error: ${err.message || String(err)}`; if (SCRIPTED) hadFailure.value = true; @@ -190,3 +223,121 @@ export async function runNativeToolLoop({ if (SCRIPTED) hadFailure.value = true; } } + +/** + * One turn with MCP servers connected — the counterpart of runNativeToolLoop. + * + * Advertising the built-in tools is only half the fix for "an MCP server + * disables every wallet tool": the model can now emit BOTH kinds of call in one + * turn, and each has to reach the right handler. So this driver: + * + * 1. Completes with `completeWithMcp({ mcpClients, tools })` — the SDK merges + * our tool definitions and the servers' into one request, so the prompt's + * promises and the model's catalogue match again. + * 2. Routes every built-in call through dispatchNativeCall — the SAME + * handleAction boundary (resolve → policy → preview → mainnet ack → y/N) + * the slash commands, the v0 path and runNativeToolLoop use. An MCP server + * never answers a wallet action. + * 3. Leaves MCP calls to `invokeToolCall`, which cli.mjs gates behind its own + * confirmation before any server sees the call. + * 4. Folds refusals, thrown errors, SDK toolErrors and a hit round limit into + * `hadFailure`, so a scripted run still exits non-zero — the same + * propagateHadFailure mapping the native loop feeds. + * + * Printing follows each handler's own contract: handleAction prints its write + * result itself (a second copy would be shown otherwise), while a read from + * dispatchToolCall is printed here. Every call sits in the same RST/DIM frame + * cli.mjs used around the MCP gate, so a result is never written while the + * model's dimmed stream is still open. + * + * Returns whatever `completeWithMcp` resolved to, so the caller can log the + * round count or surface a limit. + * + * @param ctx + * @param ctx.history mutated in place: user/assistant/tool messages are appended. + * @param ctx.mcpClients connected clients (cli.mjs module state). + * @param ctx.completeWithMcp the real loop from agent.mjs (injectable seam). + * @param ctx.getToolDefinitions our built-in tools, advertised alongside MCP's. + * @param ctx.handleAction the REAL CLI boundary — writes go here. + * @param ctx.dispatchToolCall read-only fast path. + * @param ctx.isWrite toolName -> boolean. + * @param ctx.onToken streams the model's own tokens. + * @param ctx.invokeToolCall the MCP gate (cli.mjs: confirm + invoke). + * @param ctx.printw / ctx.DIM / ctx.RST presentation frame. + * @param ctx.println line output (stdout interactive, stderr scripted). + * @param ctx.c color helpers. + * @param ctx.SCRIPTED boolean — when true, failures must set hadFailure. + * @param ctx.hadFailure mutable { value: boolean } shared with the REPL. + */ +export async function runMcpTurn({ + history, + mcpClients, + completeWithMcp, + getToolDefinitions, + handleAction, + dispatchToolCall, + isWrite, + onToken, + invokeToolCall, + printw, + DIM, + RST, + println, + c, + SCRIPTED, + hadFailure, +}) { + // One frame per call: the model's tokens stream dimmed, so anything printed + // while that is open would be dim too. RST before the result, DIM after. + const framed = async (fn) => { + printw(RST + "\n"); + try { + return await fn(); + } finally { + printw(DIM); + } + }; + + const invokeNativeToolCall = async (call) => { + let execResult = null; + let boundary = "handleAction"; + try { + const routed = await framed(() => dispatchNativeCall(call, { handleAction, dispatchToolCall, isWrite })); + execResult = routed.result; + boundary = routed.boundary; + } catch (err) { + execResult = `Error: ${err.message || String(err)}`; + if (SCRIPTED) hadFailure.value = true; + } + if (boundary === "dispatchToolCall" && execResult) { + println(" " + c.cyan(String(execResult).replace(/\n/g, "\n ")) + "\n"); + } + // A returned refusal is a failure, same rule as the native loop and the + // v0 path — so a refused write over MCP still exits non-zero in a script. + if (execResult != null && isRefusal(execResult) && SCRIPTED) { + hadFailure.value = true; + } + return execResult; + }; + + const result = await completeWithMcp(history, { + mcpClients, + tools: getToolDefinitions(), + onToken, + invokeToolCall: (call) => framed(() => invokeToolCall(call)), + invokeNativeToolCall, + }); + printw(RST + "\n"); + + history.push({ role: "assistant", content: result.text }); + for (const e of result.toolErrors) { + println(c.red(` tool error [${e.code}]: ${e.message}`)); + if (SCRIPTED) hadFailure.value = true; + } + if (result.limitReached) { + println(c.yellow(` (stopped after ${result.rounds} tool-call rounds — the model kept calling tools)`)); + if (SCRIPTED) hadFailure.value = true; + } + println(""); + return result; +} diff --git a/test/agent-mcp.test.mjs b/test/agent-mcp.test.mjs index 31631b4..185cd06 100644 --- a/test/agent-mcp.test.mjs +++ b/test/agent-mcp.test.mjs @@ -177,6 +177,86 @@ describe("completeWithMcp — round limit", () => { }); }); +describe("completeWithMcp — built-in tools alongside MCP", () => { + const BUILTIN = [ + { type: "function", name: "send_mon", description: "d", parameters: { type: "object", properties: {}, required: [] } }, + { type: "function", name: "get_balance", description: "d", parameters: { type: "object", properties: {}, required: [] } }, + ]; + + test("advertises our tools in the same completion request as mcp", async () => { + const seen = []; + const runCompletion = (params) => { + seen.push(params); + return fakeRun({ events: [], final: { contentText: "hi", toolCalls: [] } }); + }; + await completeWithMcp([{ role: "user", content: "hello" }], { + mcpClients: [{ name: "s", client: {} }], + tools: BUILTIN, + runCompletion, + }); + assert.equal(seen.length, 1); + assert.deepEqual(seen[0].tools, BUILTIN); + assert.equal(seen[0].mcp.length, 1); + }); + + test("no tools → the request is the MCP-only shape it replaces", async () => { + const seen = []; + const runCompletion = (params) => { + seen.push(params); + return fakeRun({ events: [], final: { contentText: "hi", toolCalls: [] } }); + }; + await completeWithMcp([], { mcpClients: [{ name: "s", client: {} }], runCompletion }); + assert.equal("tools" in seen[0], false, "an empty tool list must not add a key to the request"); + }); + + test("a built-in name goes to invokeNativeToolCall, an MCP name to invokeToolCall", async () => { + let calls = 0; + const runCompletion = () => { + calls++; + if (calls === 1) { + return fakeRun({ + events: [], + final: { + contentText: "", + toolCalls: [ + { id: "1", name: "send_mon", arguments: { amountMon: "1" } }, + { id: "2", name: "srv_tool", arguments: {} }, + ], + }, + }); + } + return fakeRun({ events: [], final: { contentText: "done", toolCalls: [] } }); + }; + const native = []; + const mcp = []; + const history = []; + await completeWithMcp(history, { + mcpClients: [{ name: "s", client: {} }], + tools: BUILTIN, + runCompletion, + invokeNativeToolCall: async (call) => { native.push(call); return "native result"; }, + invokeToolCall: async (call) => { mcp.push(call); return "mcp result"; }, + }); + + assert.deepEqual(native.map((c) => c.name), ["send_mon"]); + assert.deepEqual(mcp.map((c) => c.name), ["srv_tool"]); + // Both results are fed back so the model can react to either. + assert.deepEqual(history.filter((m) => m.role === "tool"), [ + { role: "tool", content: "native result" }, + { role: "tool", content: "mcp result" }, + ]); + }); + + test("throws when the model calls a built-in tool with no invokeNativeToolCall", async () => { + const runCompletion = () => + fakeRun({ events: [], final: { contentText: "", toolCalls: [{ id: "1", name: "send_mon", arguments: {} }] } }); + await assert.rejects( + () => completeWithMcp([], { mcpClients: [{ name: "s", client: {} }], tools: BUILTIN, runCompletion }), + /invokeNativeToolCall is required/, + ); + }); +}); + describe("completeWithMcp — model stream error", () => { test("does not throw when the event stream errors, and stops the loop", async () => { const runCompletion = () => ({ diff --git a/test/helpers/mcp-stdio-server.mjs b/test/helpers/mcp-stdio-server.mjs new file mode 100644 index 0000000..ed4d755 --- /dev/null +++ b/test/helpers/mcp-stdio-server.mjs @@ -0,0 +1,70 @@ +/** + * A minimal MCP server over stdio, for tests: answers `initialize`, + * `tools/list` and `tools/call` so the real @modelcontextprotocol/sdk client + * can connect, list one tool and call it — no network, no third-party package. + * + * One tool, `srv_do_thing`, whose name deliberately does NOT collide with any + * built-in (dispatch routes by name, and a test needs both kinds in one turn). + * + * Anything else with an id gets an empty result, so a protocol addition on the + * client side degrades to "empty" instead of hanging the handshake. + * + * NAD_MCP_CALL_LOG, when set, names a file that gets one line per `tools/call` + * — a test can assert a declined call never reached the server. + * + * `node --test` executes every file under test/ (its default globs cover + * **\/test/**), and with no client on stdin this loop would wait forever for a + * handshake that never comes — wedging the whole suite. A real spawn (the CLI, + * or a test connecting through connectMcpServers) has no NODE_TEST_CONTEXT in + * the environment the stdio transport forwards, so it is only the discovery + * pass that trips this. + */ +import { appendFileSync } from "node:fs"; +import { createInterface } from "node:readline"; + +if (process.env.NODE_TEST_CONTEXT) process.exit(0); + +const TOOL = { + name: "srv_do_thing", + description: "Test tool exposed by the fake MCP server.", + inputSchema: { type: "object", properties: {}, required: [] }, +}; + +const send = (msg) => process.stdout.write(JSON.stringify(msg) + "\n"); + +const rl = createInterface({ input: process.stdin }); +for await (const line of rl) { + let msg; + try { + msg = JSON.parse(line); + } catch { + continue; + } + const { id, method, params } = msg; + if (method === "initialize") { + send({ + jsonrpc: "2.0", + id, + result: { + protocolVersion: params?.protocolVersion ?? "2024-11-05", + capabilities: { tools: {} }, + serverInfo: { name: "nad-test-mcp", version: "1.0.0" }, + }, + }); + } else if (method === "tools/list") { + send({ jsonrpc: "2.0", id, result: { tools: [TOOL] } }); + } else if (method === "tools/call") { + // A test can set NAD_MCP_CALL_LOG to prove the server was (or was NOT) + // reached — the confirmation gate must never let a declined call through. + if (process.env.NAD_MCP_CALL_LOG) { + appendFileSync(process.env.NAD_MCP_CALL_LOG, `${params?.name ?? ""}\n`); + } + send({ + jsonrpc: "2.0", + id, + result: { content: [{ type: "text", text: `MCP TOOL RAN: ${params?.name ?? ""}` }] }, + }); + } else if (id !== undefined && id !== null) { + send({ jsonrpc: "2.0", id, result: {} }); + } +} diff --git a/test/helpers/shim-qvac.mjs b/test/helpers/shim-qvac.mjs index 8a6e35f..cf5f060 100644 --- a/test/helpers/shim-qvac.mjs +++ b/test/helpers/shim-qvac.mjs @@ -4,9 +4,9 @@ * `node --import dist/cli.mjs`). * * Only the model is faked — wallet init (dry-run), policy, history, the native - * tool loop, confirm prompts and `process.exit` are all production code, so a - * scripted run through this shim verifies the actual CLI exit path with no live - * model, no GPU and no funded wallet. + * tool loop, the MCP turn, confirm prompts and `process.exit` are all + * production code, so a scripted run through this shim verifies the actual CLI + * exit path with no live model, no GPU and no funded wallet. * * One scenario per process, selected via NAD_SHIM_SCENARIO (read lazily at each * completion call so the turn counter drives multi-turn cases): @@ -16,6 +16,29 @@ * sdk-error — a toolError event with a fixed message (exit 1) * turn-exhaust — a get_address tool call on EVERY turn (exit 1 via turn limit) * + * The mcp-* scenarios drive the MCP branch of processLine (an mcp.json with a + * connected server is the harness's job, not this file's): + * mcp-chat — text only; the turn must complete with exit 0 + * mcp-read — calls the BUILT-IN get_address while MCP is connected; the + * read must answer from our own dispatch (issue #118: with a + * server connected, built-in tools used to vanish, so the + * model could not reach them at all) + * mcp-refusal — calls the BUILT-IN send_mon to an unknown recipient; the + * real handleAction boundary refuses it (exit 1) + * mcp-gate — calls the server's srv_do_thing; the confirmation gate has + * no scripted answer line left, so it must decline WITHOUT + * invoking the server (exit 1) + * mcp-accept — same call, but the scripted `y` approves it: the gate must + * reach the real server (exit 0) + * + * Every mcp-* response reports what the request actually carried + * (`… tools= mcp=`), so a test can prove from inside the real process + * that the built-in definitions and the servers were offered together. + * + * Unlike completeWithTools, completeWithMcp reads `run.final` (contentText + + * toolCalls) instead of collecting tool calls from the event stream, so every + * completion returns both halves. + * * NAD_SHIM_CALL_LOG, when set, names a file that gets one line per completion() call, so a test * can prove a command never reached the model (slash commands must not). */ @@ -23,8 +46,14 @@ import { appendFileSync } from "node:fs"; let calls = 0; -function eventsFor(scenario) { - calls++; +const DELTA = (text) => ({ type: "contentDelta", seq: 0, text }); +const TEXT_ONLY = (text) => ({ + events: [DELTA(text)], + final: { contentText: text, toolCalls: [] }, +}); + +/** Events for the non-MCP scenarios (`n` is 1-based, one per completion call). */ +function nativeEvents(scenario, n) { switch (scenario) { case "sdk-error": return [ @@ -39,11 +68,11 @@ function eventsFor(scenario) { { type: "toolCall", seq: 0, - call: { id: `shim_${calls}`, name: "get_address", arguments: {} }, + call: { id: `shim_${n}`, name: "get_address", arguments: {} }, }, ]; case "refusal": - if (calls === 1) { + if (n === 1) { return [ { type: "toolCall", @@ -52,10 +81,10 @@ function eventsFor(scenario) { }, ]; } - return [{ type: "contentDelta", seq: 0, text: "Noted." }]; + return [DELTA("Noted.")]; case "ok": default: - if (calls === 1) { + if (n === 1) { return [ { type: "toolCall", @@ -64,10 +93,46 @@ function eventsFor(scenario) { }, ]; } - return [{ type: "contentDelta", seq: 0, text: "Done." }]; + return [DELTA("Done.")]; } } +/** One completion for the MCP scenarios — see the header for each contract. */ +function mcpCompletion(scenario, n, params) { + // Report what this request actually carried, so a test can prove (from + // inside the real process) that our tools and the servers' were offered + // together — the whole of issue #118. + const counts = `tools=${params.tools?.length ?? 0} mcp=${params.mcp?.length ?? 0}`; + if (scenario === "mcp-chat") return TEXT_ONLY(`Hello from the MCP branch. ${counts}`); + if (n > 1) return TEXT_ONLY(`Done. ${counts}`); + const firstCalls = { + "mcp-read": { id: "shim_mread", name: "get_address", arguments: {} }, + "mcp-refusal": { id: "shim_mref", name: "send_mon", arguments: { to: "nobody-in-the-book", amountMon: "1" } }, + "mcp-gate": { id: "shim_mgate", name: "srv_do_thing", arguments: { x: 1 } }, + "mcp-accept": { id: "shim_macc", name: "srv_do_thing", arguments: { x: 1 } }, + }; + const call = { ...firstCalls[scenario] }; + if (scenario === "mcp-accept") { + // The real SDK attaches `invoke` to a server's tool call (mcp-adapter's + // handler is `client.callTool`). Our fake replaces that stream, so attach + // the same contract here — otherwise the gate would find no handler and + // the approve branch could never be exercised. It really does dial the + // server (same config, same stdio transport), so a test can assert the + // round trip happened. + call.invoke = async () => { + const { loadMcpConfig, connectMcpServers, disconnectMcpServers } = await import("../../src/mcp.mjs"); + const cfg = loadMcpConfig(); + const connected = await connectMcpServers(cfg.servers, { onWarn: () => {} }); + try { + return await connected[0].client.callTool({ name: call.name, arguments: call.arguments }); + } finally { + await disconnectMcpServers(connected); + } + }; + } + return { events: [], final: { contentText: "", toolCalls: [call] } }; +} + export async function loadModel(_params, _opts) { return "shim-model-id"; } @@ -76,13 +141,17 @@ export async function unloadModel(_params) { return null; } -export function completion(_params, _opts) { +export function completion(params, _opts) { if (process.env.NAD_SHIM_CALL_LOG) appendFileSync(process.env.NAD_SHIM_CALL_LOG, "completion\n"); const scenario = process.env.NAD_SHIM_SCENARIO || "ok"; - const events = eventsFor(scenario); + calls++; + const { events, final } = scenario.startsWith("mcp-") + ? mcpCompletion(scenario, calls, params) + : { events: nativeEvents(scenario, calls), final: { contentText: "", toolCalls: [] } }; return { events: (async function* () { for (const e of events) yield e; })(), + final: Promise.resolve(final), }; } diff --git a/test/mcp-cli.test.mjs b/test/mcp-cli.test.mjs new file mode 100644 index 0000000..3f0149f --- /dev/null +++ b/test/mcp-cli.test.mjs @@ -0,0 +1,167 @@ +/** + * Real CLI subprocess with an MCP server connected (issue #118). + * + * The bug was reachable only through THIS configuration: mcp.json present, so + * processLine took the MCP branch, and every line went to completeWithMcp — + * which completed with `{ mcp }` alone. The nine built-in wallet tools the + * system prompt advertises were never offered, a wallet request became a + * no-op, and a scripted run still exited 0. + * + * The loop-level file (mcp-native-dispatch.test.mjs) pins the routing in + * process; these run dist/cli.mjs end to end — real MCP handshake with a real + * stdio server (test/helpers/mcp-stdio-server.mjs), real handleAction, real + * confirmation gate, real `process.exit(hadFailure ? 1 : 0)` — and so cover the + * wiring in src/cli.mjs itself: + * + * mcp-chat — server connects, a plain turn completes, exit 0, and the + * request carried tools AND mcp together (reported by the shim). + * mcp-read — a built-in read answers from our own dispatch, not the gate. + * mcp-refusal — a built-in write is refused by handleAction → exit 1. + * mcp-gate — a server tool is declined by the confirmation gate before + * the server is ever called → exit 1. + * mcp-accept — the same server tool, approved by a scripted `y`, reaches the + * real server → exit 0 (the positive control for the refusals). + * + * Offline: only the model is faked (loader hook → test/helpers/shim-qvac.mjs), + * no live model, no GPU, no funded wallet, no network beyond localhost. + */ + +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const SHIM_REGISTER = fileURLToPath(new URL("./helpers/shim-register.mjs", import.meta.url)); +const MCP_SERVER = fileURLToPath(new URL("./helpers/mcp-stdio-server.mjs", import.meta.url)); +const DIST_CLI = fileURLToPath(new URL("../dist/cli.mjs", import.meta.url)); +// --import must be a file:// URL: a bare absolute path is rejected by the ESM +// loader on Windows (ERR_UNSUPPORTED_ESM_URL_SCHEME). +const SHIM_REGISTER_URL = pathToFileURL(SHIM_REGISTER).href; + +// Standard BIP-39 test vector — valid checksum, never funded. initWallet only +// derives locally from it (dry-run); the startup balance read is best-effort. +const TEST_SEED = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + +const stateDir = mkdtempSync(join(tmpdir(), "nad-mcp-cli-")); +const EMPTY_POLICY = join(stateDir, "empty-policy.json"); +writeFileSync(EMPTY_POLICY, "{}"); + +function runMcpCli(scenario, { input = "hello\n" } = {}) { + assert.ok(existsSync(DIST_CLI), "dist/cli.mjs missing — run npm run build first (CI builds before testing)"); + const callLog = join(stateDir, `server-calls-${scenario}.log`); + rmSync(callLog, { force: true }); + // The call-log path goes in the SERVER's env map, not the CLI's: the stdio + // transport only forwards a whitelist of the parent's environment. + const mcpConfig = join(stateDir, `mcp-${scenario}.json`); + writeFileSync( + mcpConfig, + JSON.stringify({ + servers: [ + { name: "t", command: process.execPath, args: [MCP_SERVER], env: { NAD_MCP_CALL_LOG: callLog } }, + ], + }), + ); + const env = { + ...process.env, + WDK_SEED: TEST_SEED, + QVAC_MODEL_PATH: "shim-model.gguf", // consumed by the shim, never read + USE_NATIVE_TOOLS: "true", + MONAD_NETWORK: "testnet", + NAD_STATE_PATH: join(stateDir, `${scenario}.json`), + NAD_POLICY: EMPTY_POLICY, + NAD_MCP_CONFIG: mcpConfig, + NAD_SHIM_SCENARIO: scenario, + NO_COLOR: "1", + }; + delete env.NAD_CLI_NO_RUN; // this file never sets it; the child must REALLY run + delete env.PIMLICO_API_KEY; // force dry-run regardless of the developer shell + delete env.PIMLICO_SPONSORSHIP_POLICY_ID; + // spawnSync, not execFileSync: stderr holds the whole scripted transcript and + // must be captured on success too, not only when the child throws. + const res = spawnSync(process.execPath, ["--import", SHIM_REGISTER_URL, DIST_CLI], { + env, + input, // scripted lines; the shimmed model ignores their content + encoding: "utf8", + timeout: 120000, + stdio: ["pipe", "pipe", "pipe"], + }); + assert.equal(res.error, undefined, `child failed to run: ${res.error?.message}`); + return { + status: res.status, + stdout: res.stdout ?? "", + stderr: res.stderr ?? "", + log: `${res.stdout ?? ""}${res.stderr ?? ""}`, + callLog, + }; +} + +/** Lines the fake MCP server actually executed (absent → never called). */ +function serverCalls(callLog) { + if (!existsSync(callLog)) return []; + return readFileSync(callLog, "utf8").split("\n").filter(Boolean); +} + +describe("real CLI subprocess — MCP server connected (issue #118)", () => { + test("the server connects and a plain turn exits 0 with tools AND mcp in the request", () => { + const r = runMcpCli("mcp-chat"); + const log = r.log; + assert.equal(r.status, 0, `expected exit 0, transcript:\n${log}`); + assert.match(log, /ok \(1\/1\)/, "the configured server must have connected"); + // The shim reports what the completion request actually carried — the whole + // of the bug: the built-in definitions and the servers, together, every time. + assert.match(log, /Hello from the MCP branch\. tools=[1-9][0-9]* mcp=1/, `built-in tools must be offered alongside mcp:\n${log}`); + }); + + test("a built-in read is answered by our dispatch while MCP is connected", () => { + const r = runMcpCli("mcp-read"); + const log = r.log; + assert.equal(r.status, 0, `expected exit 0, transcript:\n${log}`); + assert.match(log, /Done\. tools=[1-9][0-9]* mcp=1/, log); + // Our boundary answered: the read result was printed, and the model's + // request went nowhere near the confirmation gate. + assert.ok(!log.includes("MCP tool: get_address"), "a built-in read must not hit the MCP gate"); + assert.ok( + !log.includes("no handler is registered"), + "the built-in tool must not be looked up on the server:\n" + log, + ); + assert.match(log, /Read: your wallet address|0x[0-9a-fA-F]{40}|\(wallet not initialized\)/, log); + }); + + test("a built-in write is refused by handleAction → exit 1, server untouched", () => { + const r = runMcpCli("mcp-refusal"); + const log = r.log; + assert.equal(r.status, 1, `expected exit 1, transcript:\n${log}`); + assert.match(log, /Refused:/, log); + assert.match(log, /nobody-in-the-book/, "the recipient from the model's arguments must surface"); + // The write went through OUR boundary, not the server's tool catalog. + assert.ok(!log.includes("MCP tool: send_mon"), "a built-in write must not hit the MCP gate"); + assert.deepEqual(serverCalls(r.callLog), [], "the server must not be called for a built-in action"); + }); + + test("a server tool call is declined by the gate before the server runs → exit 1", () => { + const r = runMcpCli("mcp-gate"); + const log = r.log; + assert.equal(r.status, 1, `expected exit 1, transcript:\n${log}`); + assert.match(log, /MCP tool: srv_do_thing/, "the server's tool must still reach the gate"); + // The gate asks, gets no answer line, cancels — the refusal is returned to + // the model as a tool result (never printed), so the transcript shows the + // cancel and the exit code carries the failure. + assert.match(log, /no answer line for the confirmation/, log); + assert.match(log, /cancelling this action/, log); + // Scripted mode has no `y` left to give: the gate must cancel, and the + // confirmation it never got must not be worth a single server round trip. + assert.deepEqual(serverCalls(r.callLog), [], "a declined call must never reach the MCP server"); + }); + + test("the same server tool, approved by a scripted y, reaches the real server → exit 0", () => { + const r = runMcpCli("mcp-accept", { input: "hello\ny\n" }); + const log = r.log; + assert.equal(r.status, 0, `expected exit 0, transcript:\n${log}`); + assert.match(log, /allow this tool call\? \[y\/N\] y/, log); + assert.match(log, /MCP TOOL RAN: srv_do_thing/, "the approved call must actually run on the server"); + assert.deepEqual(serverCalls(r.callLog), ["srv_do_thing"]); + }); +}); diff --git a/test/mcp-native-dispatch.test.mjs b/test/mcp-native-dispatch.test.mjs new file mode 100644 index 0000000..ac5800d --- /dev/null +++ b/test/mcp-native-dispatch.test.mjs @@ -0,0 +1,278 @@ +/** + * Combined MCP + built-in dispatch (issue #118). + * + * The bug: with an mcp.json present, processLine handed every line to + * completeWithMcp, which completed with `{ mcp }` alone — so the nine built-in + * wallet tools the system prompt advertises were never offered to the model, + * and a wallet request became a no-op (scripted runs still exiting 0). + * + * Passing the definitions is only half of it, which is what these tests pin: + * + * 1. the completion receives `tools` AND `mcp` in one request; + * 2. a built-in WRITE from that request reaches the REAL handleAction + * boundary (resolve → policy → confirm) and never the MCP gate; + * 3. a built-in READ reaches dispatchToolCall, is printed once, and a + * refusal still fails a scripted run; + * 4. an MCP tool call still reaches the confirmation gate — the gate is + * unchanged and refuses a call the operator has not approved; + * 5. tool errors and a hit round limit fold into hadFailure, so the CLI's + * `process.exit(hadFailure ? 1 : 0)` mapping still sees them. + * + * Everything runs offline: only the model is faked (the runCompletion seam + * the other native-tool tests use) — the loop, the boundary, the gate and the + * exit mapping are production code. + */ + +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; + +// Set BEFORE the dynamic cli.mjs import: importing the CLI without this flag +// starts the REPL (drains stdin, loads the model, calls process.exit). +process.env.NAD_CLI_NO_RUN = "1"; +const cli = await import("../src/cli.mjs"); +const { runMcpTurn, dispatchNativeCall } = await import("../src/nativeToolLoop.mjs"); +const { completeWithMcp } = await import("../src/agent.mjs"); +const { getToolDefinitions, dispatchToolCall, isWrite } = await import("../src/tools.mjs"); + +const noColor = { + red: (s) => s, cyan: (s) => s, yellow: (s) => s, dim: (s) => s, bold: (s) => s, + green: (s) => s, prompt: (s) => s, violet: (s) => s, +}; + +/** One fake completion(): replays `events`, then resolves `final`. */ +function fakeRun({ events = [], final }) { + return { + events: (async function* () { + for (const e of events) yield e; + })(), + final: Promise.resolve(final), + }; +} + +/** + * The exact production composition processLine now builds, with only the model + * faked: real completeWithMcp → real handleAction → real dispatchToolCall → + * real isWrite. `turns` replays in order (the last one repeats), and every + * completion() params object is captured so a test can assert what the model + * was offered. + */ +function makeTurn(turns, { handleAction, dispatchToolCall: dispatch, gate, printed }) { + const seen = []; + let i = 0; + const runCompletion = (params) => { + seen.push(params); + const turn = turns[Math.min(i, turns.length - 1)]; + i++; + return fakeRun(turn); + }; + const hadFailure = { value: false }; + const history = [ + { role: "system", content: "system prompt" }, + { role: "user", content: "the user's line" }, + ]; + const gateCalls = []; + const run = () => + runMcpTurn({ + history, + mcpClients: [{ name: "srv", client: {} }], + completeWithMcp: (h, opts) => completeWithMcp(h, { ...opts, runCompletion }), + getToolDefinitions, + handleAction: handleAction ?? cli.handleAction, + dispatchToolCall: dispatch ?? dispatchToolCall, + isWrite, + onToken: () => {}, + invokeToolCall: gate ?? (async (call) => { gateCalls.push(call); return "mcp result"; }), + printw: () => {}, + DIM: "", + RST: "", + println: (...a) => printed.push(a.join("")), + c: noColor, + SCRIPTED: true, + hadFailure, + }); + return { seen, gateCalls, hadFailure, history, run }; +} + +const nativeToolCallTurn = (call) => ({ + events: [], + final: { contentText: "", toolCalls: [call] }, +}); +const chatTurn = (text) => ({ events: [{ type: "contentDelta", text }], final: { contentText: text, toolCalls: [] } }); + +describe("MCP connected — the built-in tools are advertised alongside mcp", () => { + test("every completion of the turn carries our tool definitions and the servers", async () => { + const printed = []; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "get_address", arguments: {} }), chatTurn("done.")], + { printed }, + ); + await t.run(); + + assert.ok(t.seen.length >= 2, "the model must be re-completed after the tool result"); + for (const params of t.seen) { + assert.ok(Array.isArray(params.tools), "tools must be passed to every completion"); + assert.deepEqual( + params.tools.map((x) => x.name), + getToolDefinitions().map((x) => x.name), + "the advertised list must be the agent's own getToolDefinitions()", + ); + assert.equal(params.mcp.length, 1, "the MCP server must stay in the same request"); + } + // The prompt's promises and the model's catalogue agree again. + for (const name of ["get_balance", "send_mon", "swap", "account", "transfer_nft"]) { + assert.ok(getToolDefinitions().some((d) => d.name === name), `${name} missing from the advertised tools`); + } + }); +}); + +describe("MCP connected — combined dispatch", () => { + test("a built-in write reaches the REAL handleAction boundary, not the MCP gate", async () => { + const printed = []; + const t = makeTurn( + [ + nativeToolCallTurn({ id: "1", name: "send_mon", arguments: { to: "nobody-in-the-book", amountMon: "1" } }), + chatTurn("ok."), + ], + { printed }, + ); + await t.run(); + + // The gate saw nothing: an MCP server must never answer a wallet action. + assert.deepEqual(t.gateCalls, [], "send_mon must not be routed to the MCP gate"); + // Real boundary, real refusal (unknown recipient — refused before any prompt). + const toolMsg = t.history.findLast((m) => m.role === "tool"); + assert.ok(toolMsg, "the tool result must be fed back into history"); + assert.match(toolMsg.content, /^Refused: unknown recipient/); + // Refusal → scripted failure → the CLI exits 1 through this exact mapping. + assert.equal(t.hadFailure.value, true, "a refused write must fail a scripted run"); + assert.equal(cli.propagateHadFailure(t.hadFailure, false), true); + }); + + test("a built-in read goes through dispatchToolCall, prints once, and never touches handleAction", async () => { + const printed = []; + const handled = []; + const handleStub = async (action) => { handled.push(action); return "SHOULD NOT HAPPEN"; }; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "get_address", arguments: {} }), chatTurn("ok.")], + { handleAction: handleStub, printed }, + ); + await t.run(); + + assert.deepEqual(handled, [], "a read must not go through handleAction"); + assert.deepEqual(t.gateCalls, [], "a read must not go through the MCP gate either"); + const toolMsg = t.history.findLast((m) => m.role === "tool"); + assert.ok(printed.some((p) => p.includes(toolMsg.content)), "the read result must be printed for the operator"); + assert.equal(t.hadFailure.value, false); + }); + + test("a read refusal still fails the scripted run", async () => { + const printed = []; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "get_nfts", arguments: { address: "not-an-address" } }), chatTurn("ok.")], + { printed }, + ); + await t.run(); + + const toolMsg = t.history.findLast((m) => m.role === "tool"); + assert.match(toolMsg.content, /^Refused:/); + assert.equal(t.hadFailure.value, true); + assert.equal(cli.propagateHadFailure(t.hadFailure, false), true); + }); + + test("an MCP tool call goes to the gate and nowhere else", async () => { + const printed = []; + const handled = []; + const handleStub = async (action) => { handled.push(action); return "SHOULD NOT HAPPEN"; }; + const dispatched = []; + const dispatchStub = async (name, args) => { dispatched.push(name); return "SHOULD NOT HAPPEN"; }; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "srv_do_thing", arguments: { x: 1 } }), chatTurn("ok.")], + { handleAction: handleStub, dispatchToolCall: dispatchStub, printed }, + ); + await t.run(); + + assert.equal(t.gateCalls.length, 1, "the server's tool must reach the confirmation gate"); + assert.equal(t.gateCalls[0].name, "srv_do_thing"); + assert.deepEqual(handled, []); + assert.deepEqual(dispatched, []); + assert.equal(t.history.findLast((m) => m.role === "tool").content, "mcp result"); + }); + + test("the MCP confirmation gate refuses a call the operator never approved", async () => { + let invoked = 0; + const out = await cli.invokeMcpToolCall({ + name: "srv_do_thing", + arguments: { x: 1 }, + invoke: async () => { invoked++; return "server result"; }, + }); + + // Scripted mode has no answer line left: the gate cancels instead of running. + assert.match(String(out), /^Refused: the operator declined this tool call\./); + assert.equal(invoked, 0, "the server must not be called before the operator says yes"); + }); +}); + +describe("MCP connected — scripted failure handling", () => { + test("an SDK toolError is reported and fails the run", async () => { + const printed = []; + const t = makeTurn( + [{ + events: [{ type: "toolError", error: { code: "VALIDATION_ERROR", message: "bad arguments" } }], + final: { contentText: "sorry", toolCalls: [] }, + }], + { printed }, + ); + await t.run(); + + assert.ok(printed.some((p) => p.includes("tool error [VALIDATION_ERROR]: bad arguments"))); + assert.equal(t.hadFailure.value, true); + assert.equal(cli.propagateHadFailure(t.hadFailure, false), true); + }); + + test("a hit round limit is reported and fails the run", async () => { + const printed = []; + // Same shape every round → completeWithMcp runs past maxToolRounds (8). + const runaway = nativeToolCallTurn({ id: "x", name: "srv_forever", arguments: {} }); + const t = makeTurn([runaway], { printed }); + const result = await t.run(); + + assert.equal(result.limitReached, true); + assert.ok(printed.some((p) => p.includes("stopped after 8 tool-call rounds"))); + assert.equal(t.hadFailure.value, true); + }); +}); + +describe("dispatchNativeCall — the shared seam", () => { + const routed = async (call) => { + const seen = { handle: [], dispatch: [] }; + const out = await dispatchNativeCall(call, { + handleAction: async (action) => { seen.handle.push(action); return "via handleAction"; }, + dispatchToolCall: async (name, args) => { seen.dispatch.push({ name, args }); return "via dispatch"; }, + isWrite, + }); + return { seen, out }; + }; + + test("writes route to handleAction with the action object the boundary expects", async () => { + const { seen, out } = await routed({ name: "send_mon", arguments: { to: "0x1", amountMon: "1" } }); + assert.equal(out.boundary, "handleAction"); + assert.equal(out.result, "via handleAction"); + assert.deepEqual(seen.handle, [{ action: "send_mon", to: "0x1", amountMon: "1" }]); + assert.deepEqual(seen.dispatch, []); + }); + + test("account WITH an index is a switch (handleAction); without one it is a read", async () => { + const withIndex = await routed({ name: "account", arguments: { index: "1" } }); + assert.equal(withIndex.out.boundary, "handleAction"); + const list = await routed({ name: "account", arguments: {} }); + assert.equal(list.out.boundary, "dispatchToolCall"); + assert.deepEqual(list.seen.dispatch, [{ name: "account", args: {} }]); + }); + + test("reads route to dispatchToolCall with the raw arguments", async () => { + const { seen, out } = await routed({ name: "get_nfts", arguments: { address: "0x1" } }); + assert.equal(out.boundary, "dispatchToolCall"); + assert.deepEqual(seen.dispatch, [{ name: "get_nfts", args: { address: "0x1" } }]); + assert.deepEqual(seen.handle, []); + }); +}); From 3d9eabc1ae2029cf536bba6c61718697a792fc76 Mon Sep 17 00:00:00 2001 From: Yash_10 Date: Sun, 4 Oct 2026 16:22:59 +0000 Subject: [PATCH 2/2] Print a thrown tool call instead of leaving it to the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runMcpTurn's catch built an `Error: …` result but `boundary` still read "handleAction", so the read-only print branch skipped it: get_balance failing with "RPC unavailable" left the operator watching blank lines while a scripted run failed, and an interactive one depended on the model choosing to repeat the cause. Print the caught exception in the catch — nobody else has printed it, so there is no duplicate — and keep the failure flag. handleAction's own writes still print themselves exactly once, reads still print through dispatchToolCall, and the result still reaches the model either way. Tested: npm run build, npm test (576 pass), npm run smoke (SMOKE_OK). The two new regressions fail on the reviewed commit and pass with this one. --- src/nativeToolLoop.mjs | 15 +++++-- test/mcp-native-dispatch.test.mjs | 72 ++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/nativeToolLoop.mjs b/src/nativeToolLoop.mjs index 631361a..665c193 100644 --- a/src/nativeToolLoop.mjs +++ b/src/nativeToolLoop.mjs @@ -246,9 +246,11 @@ export async function runNativeToolLoop({ * * Printing follows each handler's own contract: handleAction prints its write * result itself (a second copy would be shown otherwise), while a read from - * dispatchToolCall is printed here. Every call sits in the same RST/DIM frame - * cli.mjs used around the MCP gate, so a result is never written while the - * model's dimmed stream is still open. + * dispatchToolCall is printed here. A call that THREW was printed by neither — + * the catch above does it, in red, once — so an operator always sees the cause + * and does not have to wait for the model to repeat it. Every call sits in the + * same RST/DIM frame cli.mjs used around the MCP gate, so a result is never + * written while the model's dimmed stream is still open. * * Returns whatever `completeWithMcp` resolved to, so the caller can log the * round count or surface a limit. @@ -306,8 +308,15 @@ export async function runMcpTurn({ execResult = routed.result; boundary = routed.boundary; } catch (err) { + // Nobody has printed this. The boundary that would have is the one that + // threw, so `boundary` below still reads "handleAction" and the read-only + // print would skip it: a scripted run would fail with only blank lines + // for a cause, and an interactive one would depend on the model choosing + // to repeat it. Say it here, once — the model still gets it as a result. execResult = `Error: ${err.message || String(err)}`; + println(" " + c.red(String(execResult).replace(/\n/g, "\n ")) + "\n"); if (SCRIPTED) hadFailure.value = true; + return execResult; } if (boundary === "dispatchToolCall" && execResult) { println(" " + c.cyan(String(execResult).replace(/\n/g, "\n ")) + "\n"); diff --git a/test/mcp-native-dispatch.test.mjs b/test/mcp-native-dispatch.test.mjs index ac5800d..10118d9 100644 --- a/test/mcp-native-dispatch.test.mjs +++ b/test/mcp-native-dispatch.test.mjs @@ -16,7 +16,10 @@ * 4. an MCP tool call still reaches the confirmation gate — the gate is * unchanged and refuses a call the operator has not approved; * 5. tool errors and a hit round limit fold into hadFailure, so the CLI's - * `process.exit(hadFailure ? 1 : 0)` mapping still sees them. + * `process.exit(hadFailure ? 1 : 0)` mapping still sees them; + * 6. a call that THROWS is printed once by the catch (the boundary that + * would have printed it is the one that threw), reaches the model as a + * result, and still fails a scripted run. * * Everything runs offline: only the model is faked (the runCompletion seam * the other native-tool tests use) — the loop, the boundary, the gate and the @@ -82,7 +85,9 @@ function makeTurn(turns, { handleAction, dispatchToolCall: dispatch, gate, print isWrite, onToken: () => {}, invokeToolCall: gate ?? (async (call) => { gateCalls.push(call); return "mcp result"; }), - printw: () => {}, + // Both channels land in `printed`: an assertion about what the operator + // saw has to cover printw and println alike, not just one of them. + printw: (s) => { if (s) printed.push(s); }, DIM: "", RST: "", println: (...a) => printed.push(a.join("")), @@ -212,6 +217,69 @@ describe("MCP connected — combined dispatch", () => { }); }); +describe("MCP connected — a call that throws (PR review follow-up)", () => { + /** How many times `needle` reached the operator, over both print channels. */ + const shown = (printed, needle) => printed.join("\n").split(needle).length - 1; + + test("a throwing read shows its error once, hands it to the model, and fails the run", async () => { + const printed = []; + const handled = []; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "get_balance", arguments: {} }), chatTurn("ok.")], + { + handleAction: async (action) => { handled.push(action); return "SHOULD NOT HAPPEN"; }, + dispatchToolCall: async () => { throw new Error("RPC unavailable"); }, + printed, + }, + ); + await t.run(); + + // Printed by the catch — the read boundary that would have printed it is + // the thing that threw, so the operator must not be left guessing. + assert.equal(shown(printed, "Error: RPC unavailable"), 1, `expected the error exactly once:\n${printed.join("")}`); + // …and handed to the model as the tool result, so it can react. + const toolMsg = t.history.findLast((m) => m.role === "tool"); + assert.equal(toolMsg?.content, "Error: RPC unavailable"); + assert.deepEqual(handled, [], "a read must not be routed to handleAction"); + // Still a scripted failure: exit 1 through the real mapping. + assert.equal(t.hadFailure.value, true); + assert.equal(cli.propagateHadFailure(t.hadFailure, false), true); + }); + + test("a throwing write boundary shows its error once, hands it to the model, and fails the run", async () => { + const printed = []; + const dispatched = []; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "send_mon", arguments: { to: "0x1", amountMon: "1" } }), chatTurn("ok.")], + { + handleAction: async () => { throw new Error("signer unavailable"); }, + dispatchToolCall: async (name) => { dispatched.push(name); return "SHOULD NOT HAPPEN"; }, + printed, + }, + ); + await t.run(); + + assert.equal(shown(printed, "Error: signer unavailable"), 1, `expected the error exactly once:\n${printed.join("")}`); + const toolMsg = t.history.findLast((m) => m.role === "tool"); + assert.equal(toolMsg?.content, "Error: signer unavailable"); + assert.deepEqual(dispatched, [], "a write must never reach the read fast path"); + assert.equal(t.hadFailure.value, true); + assert.equal(cli.propagateHadFailure(t.hadFailure, false), true); + }); + + test("a successful read is still printed exactly once, not twice", async () => { + const printed = []; + const t = makeTurn( + [nativeToolCallTurn({ id: "1", name: "get_address", arguments: {} }), chatTurn("ok.")], + { printed }, + ); + await t.run(); + + const toolMsg = t.history.findLast((m) => m.role === "tool"); + assert.equal(shown(printed, toolMsg.content), 1, "the no-duplicate contract must survive the fix"); + }); +}); + describe("MCP connected — scripted failure handling", () => { test("an SDK toolError is reported and fails the run", async () => { const printed = [];