agentHost: Clean up disconnected client tool ownership - #336826
Draft
roblourens wants to merge 3 commits into
Draft
roblourens wants to merge 3 commits into
roblourens wants to merge 3 commits into
Conversation
Remove disconnected active clients from provider-owned tool and customization registries when the host emits its automatic removal action. Preserve the authoring client when Copilot promotes a steering message so duplicate client tools remain routed to the correct window. Add unit, real-SDK, and deterministic full-host coverage for automatic cleanup, steering attribution, and replacement tool ownership. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The functional changes are well-covered and coherent; the only finding is a minor JSDoc placement issue.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes stale provider-owned client registrations after disconnects and preserves steering-message ownership for correct tool routing.
Changes:
- Cleans up provider tools and customizations on automatic client removal.
- Propagates steering sender identity into Copilot turns.
- Adds unit, integration, and deterministic E2E coverage.
| File | Description |
|---|---|
common/agent.ts |
Extends pending-message API with sender metadata. |
node/agentSideEffects.ts |
Handles server-originated client removal. |
node/chatContributions/queueDrain/queueDrainContribution.ts |
Tracks steering-message sender ownership. |
node/copilot/copilotAgent.ts |
Forwards sender metadata to Copilot sessions. |
node/copilot/copilotAgentSession.ts |
Preserves sender identity during steering promotion. |
test/node/mockAgent.ts |
Records steering sender in the mock provider. |
test/node/agentSideEffects.test.ts |
Tests automatic provider cleanup and sender propagation. |
test/node/chatContributions.test.ts |
Tests steering sender replacement and removal. |
test/node/copilotAgentSession.test.ts |
Tests preferred steering-client tool routing. |
test/node/providerIntegration/copilotMockLlm.integrationTest.ts |
Covers steering ownership with the real Copilot SDK. |
test/node/e2e/suites/mcpPluginSuite.ts |
Covers disconnected-client customization cleanup. |
test/node/e2e/suites/copilotRuntimeToolsSuite.ts |
Covers duplicate tool ownership transfer. |
test/node/e2e/captures/copilotcli-runtime-tools-removing-a-client-transfers-duplicate-tool-ownership-to-the-surviving-client.yaml |
Records deterministic model traffic for the ownership test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Move the pending-message sender type ahead of the provider-interface JSDoc so editor and generated documentation remain attached to IAgent. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
(Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> # Conflicts: # src/vs/platform/agentHost/test/node/e2e/suites/copilotRuntimeToolsSuite.ts
Contributor
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
2 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details.
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
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.
Summary
Root cause
The protocol server removed disconnected clients from synchronized session state with a server-originated
session/activeClientRemovedaction, but provider cleanup only ran for client-originated actions. Copilot therefore retained stale client tools and customizations internally. Steering-created turns also discarded their sender client ID, allowing duplicate client tools to fall back to the stale first-inserted owner.Validation
npm run typecheck-clientnpm run valid-layers-check(Written by Copilot)