Retract top-level MCP customization once its child id resolves - #328349
Draft
jruales wants to merge 1 commit into
Draft
Retract top-level MCP customization once its child id resolves#328349jruales wants to merge 1 commit into
jruales wants to merge 1 commit into
Conversation
`McpCustomizationController` publishes a bare top-level customization for MCP servers whose child customization cannot be resolved. Child customizations are registered asynchronously, so the first state update for a plugin-derived server can arrive before its child id exists. The controller then kept the server top-level for the rest of the session, so when the child id later resolved the same server was surfaced twice: once under the plugin-derived child id and once under the orphaned `mcp-top-level:<provider>:<session>:<name>` id. Start/stop on the orphan failed with "Cannot start/stop unknown MCP server customization". Retract the published top-level entry with `SessionCustomizationRemoved` once the child id resolves, then continue on the child path. The "stay top-level" rule existed to avoid orphaning the published id, which the explicit removal now handles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Retracts fallback top-level MCP customizations after their plugin child becomes resolvable.
Changes:
- Migrates MCP state updates to resolved child IDs.
- Adds a regression test for top-level retraction.
Show a summary per file
| File | Description |
|---|---|
mcpCustomizationController.ts |
Removes fallback entries during subsequent MCP updates. |
mcpCustomizationController.test.ts |
Tests migration from top-level to child customization. |
Review details
Suppressed comments (1)
src/vs/platform/agentHost/node/shared/mcpCustomizationController.ts:291
- This only notices a newly resolvable child during the next MCP state update, not when the child ID actually resolves.
SessionPluginControllerpublishes the asynchronously resolved plugin child independently (copilotAgent.ts:4355-4389), and that path neither mutates_livenor callsapplyOne; if the server has already settled (notablyauthRequired), no later state event is guaranteed, so the orphaned top-level entry can remain. The new test masks this by explicitly callingapplyOneafter adding the child. Please reconcile existing live entries when plugin customizations publish/settle (and test resolution without a subsequent server-state update).
const childId = this._options.resolveChildId(server.name);
if (childId !== undefined) {
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+283
to
+289
| // A top-level entry is only a fallback for servers whose child | ||
| // customization could not be resolved yet — child customizations are | ||
| // registered asynchronously, so the first state update for a | ||
| // plugin-derived server can arrive before its child id exists. Once the | ||
| // child id resolves, retract the previously published top-level entry | ||
| // and continue as a child, otherwise the same server stays surfaced | ||
| // twice for the rest of the session. |
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.
Authored by Copilot.
McpCustomizationControllerpublishes a bare top-level customization for MCP servers whose child customization cannot be resolved. Child customizations register asynchronously, so the first state update for a plugin-derived server can arrive before its child id exists.The controller then kept the server top-level for the rest of the session, so once the child id resolved the same server was surfaced twice — under the plugin child id and under the orphaned
mcp-top-level:<provider>:<session>:<name>id. Acting on the orphan failed withCannot start/stop unknown MCP server customization.Now the published top-level entry is retracted with
SessionCustomizationRemovedwhen the child id resolves. The "stay top-level" rule existed to avoid orphaning the published id, which the explicit removal handles.Repro
.mcp.json.authRequiredrather thanready, so the duplicate stays visible).Before: the server appears twice, once with a
plugin.file://...id and once withmcp-top-level:.... Start/stop on the second logsCannot start unknown MCP server customization mcp-top-level:....After: the top-level entry is retracted and the server appears once.
Validation
./scripts/test.sh --run src/vs/platform/agentHost/test/node/shared/mcpCustomizationController.test.ts(17 passing, incl. new retraction test)./scripts/test.sh --runGlob "**/agentHost/test/node/**/*.test.js"(3238 passing, 0 failing)npm run compile, hygiene, diagnostics clean