Repository navigation
Keep built-in wallet tools working with an MCP server connected - #120
Conversation
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).
portdeveloper
left a comment
There was a problem hiding this comment.
The combined native/MCP routing and policy coverage look good. One error path still needs a fix in runMcpTurn (src/nativeToolLoop.mjs:301-313). If dispatchNativeCall throws, boundary stays "handleAction", so the catch creates an error result but the following print branch skips it.
I reproduced this with get_balance throwing Error("RPC unavailable"): the model receives Error: RPC unavailable, hadFailure is true, and the operator output is only newlines. A scripted run can therefore fail without showing the cause; an interactive run depends on the model choosing to repeat it.
Please print caught exceptions directly and keep the no-duplicate behavior for results already printed by handleAction. Add a regression for a throwing read (and a throwing write boundary) asserting the error appears once, reaches the model, and still fails scripted runs. The existing 573 tests pass locally.
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.
|
Thanks — fixed in 3d9eabc. The bug: in The fix ( Regressions added (
Both throwing tests fail on the reviewed commit (12 pass / 2 fail) and pass with the fix. |
portdeveloper
left a comment
There was a problem hiding this comment.
lgtm, thanks for fixing the missing error output
Closes #118
What was wrong
With an
mcp.jsonpresent,processLinesent every line tocompleteWithMcp, 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 tool catalog instead of our safety boundary (Refused: no handler is registered for MCP tool "get_address").What changed
src/agent.mjs—completeWithMcpnow takes the agent's owntoolsalongsidemcp(the SDK merges both lists into one request; an empty list keeps the request byte-identical to the MCP-only shape it replaces) and routes by name: built-in names go toinvokeNativeToolCall, server names to the existinginvokeToolCallgate.src/nativeToolLoop.mjs—dispatchNativeCallis the seam both turn drivers share. Writes (send_mon,send_token,transfer_nft,swap) andaccount-with-index go through the samehandleActiona slash command uses (resolve → policy → preview → mainnet ack → y/N); reads go throughdispatchToolCall.boundarysays who printed, so results are shown once, not twice or never. Refusals, tool errors and a hit round limit all fold intohadFailure.src/nativeToolLoop.mjs—runMcpTurnis the new MCP turn driver, living besiderunNativeToolLoop;src/cli.mjsonly passes in the handles it needs, same as the native loop.invokeMcpToolCallis exported next tohandleActionso the gate is testable.The MCP confirmation gate is untouched: every server tool still asks, and a declined call never reaches the server.
Tests
Coverage of the combined CLI dispatch offline (only the model is faked — no live model, no GPU, no funded wallet):
test/mcp-native-dispatch.test.mjs(13) — completion receivestoolsandmcpon every round; a built-in write reaches the realhandleAction(refused, exit 1) and never the gate; a read goes throughdispatchToolCall, prints once, and a refusal still fails the run; MCP calls route to the gate and nowhere else; the gate refuses an unapproved call; toolError and round limit fail the run; routing unit tests fordispatchNativeCall.test/mcp-cli.test.mjs(5) —dist/cli.mjsend to end with a real stdio MCP server (test/helpers/mcp-stdio-server.mjs): server connects,tools=9 mcp=1in the actual request, built-in read/write answered by our boundary (exit 1 on refusal, server untouched), a declined server call never reaches the server (exit 1), and the same call approved by a scriptedyreally runs on it (exit 0).test/agent-mcp.test.mjs(+4) — request shape (toolsabsent when empty), name-based routing, and the missing-handler error.Verified the new tests fail against the current code:
tools=0 mcp=1and the built-in read/write misroute (3 of 5 subprocess tests fail onmain).Scope: no new surface — this restores the built-ins the prompt already promised alongside MCP, on the existing safety boundary.