Do not run a client tool twice when a tool call is readied twice

[Ryan Ewen (RyanEwen)]

Problem

A client tool call can receive two ChatToolCallReady actions: one from the permission flow (carrying confirmation options, preApproved unset) and one from the agent's stream mapper on content_block_stop (confirmed: not-needed, pre-approved). _executeClientTool is driven once per ready, so the tool runs twice.

The existing startedClientToolCalls guard cannot prevent this. It is armed from markInvocationStarted, which _executeClientTool only calls after awaiting resolveToolInput, so both readies pass the started check before either arms it. The two requests also differ (only in preApproved), so the equals(observedRequest, request) check treats the second as a new request rather than a duplicate.

Symptom

The turn hangs indefinitely. The tool visibly succeeds and chat/toolCallComplete is reported, but only one of the two invocations is tracked, so the turn is never closed and nothing is logged.

User-visible variants of the same double dispatch: a browser page opening before the approval prompt is answered, and answering it then opening a second one.

Evidence

Captured by logging a stack at LanguageModelToolsService#invokeTool in a dev container window:

11:20:23.064  callId=toolu_01VfWfPn…  preApproved=undefined    at WX._executeClientTool
11:20:23.064  callId=toolu_01VfWfPn…  preApproved={"type":1}   at WX._executeClientTool

Same call id, same call site, same millisecond, differing only in approval state.

Fix

Arm a separate in-flight set synchronously when execution starts, and clear it when the execution settles. A second ready for a call that is already running is ignored, while genuine retries still work because the set is cleared on completion.

Verification

With the change applied, each tool call is invoked exactly once and turns complete:

open_browser_page   invoked 1x
type_in_page        invoked 1x
navigate_page       invoked 1x

Reproduced originally in a compose dev container on Windows (VS Code 1.133.0). Local windows were unaffected.

[Copilot]

Pull request overview

Prevents duplicate client-tool execution when concurrent ready actions arrive.

Changes:

• Tracks in-flight client-tool calls synchronously.
• Suppresses execution while the call remains active.
• Clears tracking after execution settles.

Suppressed comments (2)

src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts:2333

• This call-wide guard also suppresses legitimate superseding attempts while resolveToolInput is still pending. The existing supersedes a hung referenced input read when the request changes test updates the same call from a never-resolving input URI to a second URI; the first attempt adds this key, so the update now returns here and the second input is never read or invoked. Track the active request/attempt and coalesce only duplicate ready variants (for example, equivalent execution payloads ignoring approval metadata) instead of blocking every request sharing the call key.

					if (startedClientToolCalls.has(key) || inFlightClientToolCalls.has(key)) {

src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts:2356

• Add a regression test for the pre-markInvocationStarted race: keep referenced input resolution pending, deliver the two ready variants with different approval metadata, and assert exactly one invocation while preserving a genuine retry after settlement. The existing tests do not exercise this timing window.

						inFlightClientToolCalls.add(key);

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Ryan Ewen (RyanEwen)]

@microsoft-github-policy-service agree

[Ryan Ewen (RyanEwen)]

Related: these three share a root cause. Noting it so they can be triaged together rather than as three unrelated patches.

The workbench executes a client tool off the stream-mapper chat/toolCallReady (emitted at content_block_stop with confirmed: "not-needed"), independently of whether the SDK has invoked that tool through its in-process MCP server. Three defects fall out of that one seam:

• Do not run a client tool twice when a tool call is readied twice #330683 — the call is readied twice (stream mapper, then the permission flow), so the tool runs twice.
• Validate client tool input against the tool's schema #330684 — the disconnect path synthesises a ready with no toolInput, so the tool is invoked with {} and throws during preparation.
• Buffer a client tool result that arrives before the SDK asks for it #330730 — execution finishes before the SDK registers its handler, so the result is dropped and the turn stalls.

If client tools executed only on the SDK-driven invocation, with the stream-mapper ready used for rendering only, all three would be structurally impossible: one authoritative trigger, never without real input, and registration ordered before execution.

I have not proposed that as a PR because I do not know whether the stream-mapper ready is deliberately actionable. Starting the tool as soon as its arguments finish streaming, instead of waiting for the SDK round trip, is a plausible latency win, and the permission flow may already depend on the current ordering. That is a call for whoever owns the design.

Each of the three fixes a reproducible bug on its own and carries a test that fails without it, so they are safe to land in the meantime. If the structural fix is preferred, #330683 and #330684 can reasonably be closed in its favour; #330730 is worth keeping either way, since buffering a result that arrives before its handler is the same defensive behaviour CopilotAgentSession already has on this path.

[Ryan Ewen (RyanEwen)]

Root cause filed as #330899, per the request on #330684. This PR still stands on its own, with a test that fails without it.