Keep built-in wallet tools working with an MCP server connected

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");
       }

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,15 +739,15 @@ 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);
     process.exit(1);
   });
 }
 
-export { handleAction };
+export { handleAction, invokeMcpToolCall };

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,130 @@ 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. 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.
+ *
+ * @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) {
+      // 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");
+    }
+    // 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;
+}