refactor: Extract shared remote MCP tool-call helper - #1291
Open
MQ37 wants to merge 7 commits into
Open
Conversation
connectMCPClient -> callTool -> close was duplicated in tool_dispatch.ts's ACTOR_MCP case and call_actor.ts's MCP passthrough, including a copy-pasted comment for an abort-race workaround. Extracted callRemoteMcpTool() into a new src/mcp/remote_tool_call.ts (kept out of client.ts on purpose: tests mock connectMCPClient via vi.spyOn on that module's namespace, which only intercepts a genuine cross-module import, not a same-module direct call). Pure refactor, no behavior change. Verified: type-check, lint, format, check:agents clean; full unit suite green (96 files, 1415 passed, 1 skipped).
Two JSDoc blocks in remote_tool_call.ts were multi-sentence where one sufficed; tightened both and the matching AGENTS.md entry. No logic change.
…hrough outcomes Verification (subagent) found two real gaps: - tool_dispatch.ts's ACTOR_MCP case moved the 'Calling Actor-MCP' info log to fire unconditionally, including on a connect failure. Original code only logged it after a successful connect. Moved it into onConnected so it (and notification-handler setup) run in the same order and only on success, matching pre-extraction behavior exactly. - call_actor.ts's handleMcpToolCall had no direct test for the 'connect-failed' or non-abort 'error' outcomes (tool_dispatch.ts's ACTOR_MCP case already covered its own). Added both. pnpm run type-check / lint / format / check:agents clean. pnpm run test:unit: 96 files, 1417 passed, 1 skipped.
github-actions
Bot
added
the
tested
MQ37
marked this pull request as draft
August 20, 2026 14:06
callRemoteMcpTool() owned callTool() itself (meta + onConnected hooks for tool_dispatch.ts's notification forwarding). Replaced with withRemoteMcpClient(), generic over a caller-supplied run(client) callback that does the actual callTool — each caller already knows its own tool name/args, no need for the helper to own that shape too. Net LOC: +76 across the branch vs +80 before (measured, not estimated) — the two are close because callTool's own shape (schema/timeout/signal) has to live somewhere; pushing it back to callers costs almost exactly what centralizing it saved. Kept mainly for the simpler, symmetric interface (one callback for both callers, no special-cased meta/onConnected split). pnpm run type-check / lint / format / check:agents clean. pnpm run test:unit: 96 files, 1417 passed, 1 skipped (unchanged).
MQ37
marked this pull request as ready for review
August 21, 2026 09:35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Extracts
withRemoteMcpClient()intosrc/mcp/remote_tool_call.ts: connect, hand the live client to a caller-suppliedrun(client)callback, always close, return a closed outcome union (connect-failed/aborted/error/success). Replaces duplicated connect/callTool/close try-catch-finally (including the same abort-race macrotask-yield workaround) intool_dispatch.ts'sACTOR_MCPcase andcall_actor.ts's MCP passthrough. Each caller keeps its owncallToolshape and response handling.Why
Two copies of a non-obvious workaround drift the moment one is fixed and the other isn't.
One incidental behavior delta: the
ACTOR_MCP_meta.progressTokengate now checksprogressToken != null(previously only checkedshouldForwardNotifications). Old code sent_meta: {}on the wire when a sync call had no progress token; new code omits_metaentirely. Same semantics for any spec-conformant server, confirmed via the SDK's request/serialization path.Testing
pnpm run type-check/lint/format/check:agentsclean.pnpm run test:unit: 97 files, 1457 passed, 1 skipped — includes 2 new tests forcall_actor.ts'sconnect-failed/erroroutcomes.