Let the workbench own confirmation for Claude client tools - #334037
Let the workbench own confirmation for Claude client tools#334037Ryan Ewen (RyanEwen) wants to merge 2 commits into
Conversation
The Claude SDK gates in-process MCP tools through canUseTool, so a workbench client tool (mcp__client__*) was routed through the generic pending_confirmation path even though the stream mapper had already readied it as a Running client execution. The extra host-side prompt regressed the tool call from Running back to PendingConfirmation, and the workbench rewrote that into a second ToolClientExecution request with no pre-approval, which is how a client tool could be readied and run twice (microsoft#330683). dispatchCanUseTool now allows client tools outright, after the interactive built-ins and before the server-tool auto-allow: the workbench confirms its own tools through requestConfirmation and preApproved, matching how the Copilot session skips the host permission for client tools. allowedTools is left alone because its semantics for in-process MCP tools under the plan and dontAsk modes are unverified. Adds a canUseTool test asserting that a client tool returns allow and produces no pending_confirmation signal.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: TylerLeonhardtMatched files:
|
There was a problem hiding this comment.
🟡 Changes recommended
Prefix-only auto-approval can unintentionally bypass confirmation for an external MCP server named client.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves Claude client-tool confirmation ownership to the workbench to prevent duplicate execution.
Changes:
- Auto-allows client-prefixed tools in Claude’s permission dispatcher.
- Adds regression coverage for avoiding host-side confirmation.
File summaries
| File | Description |
|---|---|
claudeCanUseTool.ts |
Bypasses host confirmation for client-prefixed tools. |
claudeAgent.test.ts |
Tests the client-tool permission path. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (hasClientToolNamePrefix(toolName)) { | ||
| // The workbench owns confirmation for its own client tools; a host-side prompt here would surface the call twice (#330683). | ||
| return { behavior: 'allow', updatedInput: input }; | ||
| } |
There was a problem hiding this comment.
Correct, and verified: the SDK server map is built as { ...externalServers.servers, ...(clientServers ?? {}) } with no reserved-name check, so a configured server named client survives whenever the window contributes no client tools, and its tools carry the same prefix. Fixed in 687dc83: the early allow now also requires session.hasClientTool(strippedName), which asks the session's client-tool model for an owner, so a prefixed name nobody owns falls through to the existing confirmation path exactly as before. The existing test now registers the tool on an active client first, and an mcp__client__ tool no window owns still prompts asserts one pending confirmation and a deny when nobody answers; it fails against the prefix-only version. Reserving the server name in _buildExternalMcpServers would be a further hardening and is left for a separate change.
AI disclosure: this comment and the related code were written with the assistance of AI.
The early allow added for microsoft#330683 keyed on the mcp__client__ name prefix alone. Copilot review on microsoft#334037 pointed out that the SDK server map is built from the external MCP configuration and the in-process client server without a reserved-name check, so a workspace or user MCP server named client also emits mcp__client__* tools. When the window contributes no client tools that external server survives, and the prefix-only allow would skip confirmation for tools no window owns. dispatchCanUseTool now allows early only when the stripped tool name has an owner in the session's client-tool model, through a new hasClientTool accessor beside the other client-tool wrappers on ClaudeAgentSession. Anything carrying the prefix without an owner falls through to the existing MCP confirmation path unchanged. The mapper's own owner resolution is untouched. Reserving the server name client in _buildExternalMcpServers would be a further hardening and is left for a separate change. The positive test now registers the tool on an active client, and a negative test asserts that an unowned mcp__client__ tool still produces one pending_confirmation and denies when nobody approves it.
Lets the workbench own confirmation for its own client tools in Claude sessions, so a client tool call is surfaced and executed once rather than twice.
Defect
A client tool (an
mcp__client__tool the window executes) is driven by two independent triggers. The stream mapper emitsChatToolCallStartwith a Client contributor andChatToolCallReadywithconfirmed: NotNeeded, the reducer puts the call in Running, and the workbench takes over: it asks the tool's own confirmation and passespreApprovedwhen it invokes. Concurrently the SDK gates the same tool throughcanUseTool, because only server tools are allow-listed, anddispatchCanUseTooltreated it like any MCP tool: it built a PendingConfirmation state and asked the host to confirm. The reducer accepts a ready action on a Running call and, with noconfirmed, rewrites it back to PendingConfirmation. The workbench then rewrites that confirmation into a second client-execution request withpreApprovedunset. That is the "readied twice runs twice" behaviour behind #330683, and the Running to PendingConfirmation regression seen in the transcript.Copilot's harness skips the host-side permission for client tools (
skipPermission); Claude's did not.Fix
dispatchCanUseToolallows a client tool immediately, after the interactive tools that must always reach the user and beside the existing server-tool auto-allow, since both are host-owned exemptions with disjoint name prefixes. The outerhandleCanUseToolchecks still apply.allowedToolsis untouched: the SDK's behaviour for in-process MCP tools underplananddontAskis not established, so thecanUseToolpath is the right place for this.Test
a client tool never reaches the pending_confirmation channeldrives the capturedcanUseToolwithmcp__client__editFileand asserts the result is allow and that nopending_confirmationsignal was produced. Without the fix it fails with one pending confirmation and a denied result.claudeAgent.test: 289 passing, 0 failing. No new type errors.Part of #333174.
AI disclosure: this comment and the related code were written with the assistance of AI.