Fix agent host MCP server state and add per-server diagnostics - #326384
Merged
Connor Peet (connor4312) merged 3 commits intoJul 17, 2026
Conversation
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Tyler James Leonhardt (@TylerLeonhardt)Matched files:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Corrects MCP lifecycle state reporting and adds structured, per-server diagnostics.
Changes:
- Seeds unstarted servers as
Stopped; marks real restart triggers asStarting. - Adds structured lifecycle logging and tests.
- Adds hidden per-server Output channels and encapsulates channel access.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/contrib/mcp/browser/mcpCommands.ts |
Routes Show Output through the customization service. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostCustomizationService.ts |
Implements per-server diagnostic channels. |
src/vs/sessions/services/agentHost/browser/agentHostCustomizationService.ts |
Removes host-level channel plumbing. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostCustomizationHarness.test.ts |
Updates the service test stub. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHostSessionsProvider.ts |
Removes remote channel lookup. |
src/vs/sessions/contrib/providers/agentHost/browser/localAgentHostSessionsProvider.ts |
Removes local channel lookup. |
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts |
Removes channel IDs from server projections. |
src/vs/sessions/common/agentHostSessionsProvider.ts |
Simplifies the MCP server interface. |
src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts |
Expects Stopped seed state. |
src/vs/platform/agentPlugins/common/pluginParsers.ts |
Seeds declared servers as Stopped. |
src/vs/platform/agentHost/test/node/shared/mcpCustomizationController.test.ts |
Tests optimistic Starting transitions. |
src/vs/platform/agentHost/test/node/customizations/scan/claudeMcpScan.test.ts |
Updates Claude scan expectations. |
src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts |
Tests starting state and lifecycle logs. |
src/vs/platform/agentHost/node/shared/mcpCustomizationController.ts |
Adds markStarting. |
src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts |
Adds optimistic state and structured logging. |
src/vs/platform/agentHost/node/copilot/copilotAgent.ts |
Updates seed-state documentation. |
src/vs/platform/agentHost/node/claude/customizations/scan/claudeMcpScan.ts |
Updates scan-state documentation. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 6
- Review effort level: Medium
Comment on lines
+156
to
+161
| // Record into the per-server diagnostics channel on every | ||
| // projection (which fires on each state change the UI observes) | ||
| // so the channel captures the observable transition history. The | ||
| // channel id itself stays internal — reveal it via | ||
| // {@link showMcpServerLog}. | ||
| this._mcpLogRegistry.record({ id, name: c.name, enabled: c.enabled, state: c.state }); |
Comment on lines
+424
to
+425
| record(server: { readonly id: string; readonly name: string; readonly enabled: boolean; readonly state: McpServerState }): string { | ||
| const channelId = channelIdForMcpServer(server.id); |
Comment on lines
+467
to
+469
| function channelIdForMcpServer(serverId: string): string { | ||
| return `agentHostMcpServer.${serverId.replace(/[\\/:*?"<>|]/g, '-')}`; | ||
| } |
Comment on lines
+1853
to
1857
| // Re-enabling restarts the server. The SDK connects it in the | ||
| // background with no live "starting" event, so surface Starting | ||
| // optimistically until the trailing refresh settles it. | ||
| this._mcpCustomizations.markStarting([server.serverName]); | ||
| await this._wrapper.session.rpc.mcp.enable({ serverName: server.serverName }); |
Comment on lines
1487
to
1489
| await this._reconcileMcpServerEnablement(); | ||
| this._markEnabledMcpServersStarting(); | ||
| await this._wrapper.session.send({ prompt, attachments: sdkAttachments?.length ? sdkAttachments : undefined }); |
| */ | ||
| class AgentHostMcpServerLogRegistry extends Disposable { | ||
|
|
||
| private readonly _entries = new Map<string, { readonly logger: ILogger; lastSignature: string | undefined }>(); |
Discovered MCP servers were seeded as `Starting` before any SDK had attempted to start them, and failures were only visible as a coarse status. This corrects the lifecycle state and adds diagnostics. - Seed `makeMcpServerCustomization` as `Stopped` instead of `Starting`; a declared-but-unstarted server has not begun connecting. - The Copilot SDK emits no live "starting" signal on initial connect (servers settle inside a blocking init phase and `rpc.mcp.list` blocks until settled), so drive `Starting` optimistically from the workbench at the two real start triggers: sending a message (`_markEnabledMcpServersStarting`) and the disable->enable flip. Added `McpCustomizationController.markStarting` (skips Ready/AuthRequired/Starting). - Log structured MCP lifecycle records (loaded/statusChanged/inventory) through the agent host's existing OTLP log stream with `OtelData` attributes, deduplicated by SDK status; failures log at error with the failure detail preserved in `McpServerErrorState`. - Give each `(session, MCP server)` its own hidden Output channel that records lifecycle transitions; "Show Output" reveals it. - Encapsulate the channel: `showMcpServerLog(sessionResource, serverId)` resolves and reveals internally; removed `logOutputChannelId` from the public `IAgentHostMcpServer` surface and the now-dead `getLogOutputChannelId` provider plumbing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Connor Peet (connor4312)
force-pushed
the
connor/mcp-server-state-diagnostics
branch
from
July 17, 2026 18:11
6d8af8d to
c4610dd
Compare
Connor Peet (connor4312)
marked this pull request as draft
July 17, 2026 19:47
auto-merge was automatically disabled
July 17, 2026 19:47
Pull request was converted to draft
- Record MCP diagnostics from state-change events over a tracked set of sessions rather than as a side effect of `getMcpServers`, so a failure and a later recovery are both captured even without a UI re-query. - Key the per-server log registry by the full session resource plus the raw customization id (not the UI-facing scoped id, whose authority is empty for Agents-window resources and would mix histories across sessions). - Use an injective, filesystem-safe channel id (SHA1 hex of the composite key) so distinct servers can't collapse onto one logger/dedup entry. - Dispose each session's loggers and Output channel registrations when the session goes away (session dispose / removal / provisional backend swap), fixing the logger/file-handle leak. - Route the explicit Start/Restart (`startMcpServer`) through `markStarting` so it shows Starting during the SDK's blocking reconnect, and always reconcile via a trailing inventory refresh (try/finally) so a rejected enable can't leave the UI stuck at Starting -- same fix applied to the enable path in `_reconcileMcpServerEnablement`. - Add coverage for the explicit-start optimistic Starting behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Connor Peet (connor4312)
marked this pull request as ready for review
July 17, 2026 20:08
Member
Author
|
Thanks for the review — addressed all six in 42dc9e0:
|
Ben Villalobos (benvillalobos)
previously approved these changes
Jul 17, 2026
The MCP diagnostics registry now resolves `ILoggerService`/`IOutputService` lazily when `record` runs (via the new eager state recording). The enablement unit test instantiated the abstract service against a bare `TestInstantiationService`, so `getMcpServers` threw `this._loggerService.createLogger is not a function`. Stub both services (NullLoggerService + a minimal IOutputService) in the test harness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Connor Peet (connor4312)
enabled auto-merge (squash)
July 17, 2026 20:28
Tyler James Leonhardt (TylerLeonhardt)
approved these changes
Jul 17, 2026
Megan Rogge (meganrogge)
added a commit
that referenced
this pull request
Jul 18, 2026
Restores the optional readonly logOutputChannelId property on IAgentHostMcpServer. #326384 removed it from the interface but left consumers (mcpListWidget, aiCustomizationManagementEditor fixture, mcpListWidget.test) still referencing it, breaking tsc in the PR merge. This non-behavioral change re-declares the optional property those consumers already expect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d527ed3c-29a9-435f-8772-c8117077d394
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.
Problem
Agent-host MCP servers were seeded as
Startingbefore any SDK had attempted to start them, and connection failures were only visible as a coarse status badge with no detail.Changes
Correct the seed state.
makeMcpServerCustomizationnow producesStoppedinstead ofStarting-- a declared-but-unstarted server has not begun connecting. Comments and parser/scan/provider tests updated accordingly.Optimistic
Startingat the real triggers. The Copilot SDK gives no live "starting" signal on initial connect (servers settle inside a blocking init phase, andrpc.mcp.listblocks until settled -- confirmed by an SDK probe and the runtime source). So the workbench drivesStartingitself at the two moments the SDK actually (re)starts a server:_markEnabledMcpServersStarting(), andAdded
McpCustomizationController.markStarting(names)which skips servers alreadyReady/AuthRequired/Starting. It is not run on every_reconcileMcpServerEnablementpass (that only syncs enablement).Structured lifecycle logging. MCP
loaded/statusChanged/inventoryevents are logged through the agent host's existing OTLP log stream withOtelDataattributes (server, status, state, source/transport/plugin metadata, error type), deduplicated by SDK status. Failures log aterrorwith the failure text, which is also preserved inMcpServerErrorState.Per-server diagnostics channel. Each
(session, MCP server)gets its own hidden Output channel recording lifecycle transitions (deduplicated). "Show Output" reveals it.Encapsulated the channel.
showMcpServerLog(sessionResource, serverId)resolves and reveals the channel internally (mirroringauthenticateMcpServer);logOutputChannelIdwas removed from the publicIAgentHostMcpServersurface, along with the now-deadgetLogOutputChannelIdprovider plumbing.No AHP schema/version change: errored state already carries structured
ErrorInfo.Testing
npm run typecheck-client,npm run valid-layers-check, and hygiene all clean.markStarting(controller), optimistic-Starting on send (session), the three lifecycle-log tests, theStoppedseed assertions (parser + claude scan), plus existing reconcile/auth-preservation coverage still green.