agent host: scoped enablement for MCP servers and plugins - #330566
agent host: scoped enablement for MCP servers and plugins#330566Connor Peet (connor4312) wants to merge 16 commits into
Conversation
Replace the single `enabled` boolean on plugins and MCP servers with a list of explicit, scoped enablement decisions. A decision is recorded at one of three scopes -- global, workspace or session -- and the list is published sorted most-specific-first, so `enablement[0]` is always the winning decision and no consumer implements precedence itself. An absent or empty list means no explicit decision exists, so the customization is enabled. `session/customizationToggled` now carries the complete decision set and replaces it wholesale rather than patching one scope, so a caller changing one scope must include every decision it means to keep. Only plugins and MCP servers carry `enablement`, since they are the only things that can be keyed durably. Directories keep their plain `enabled` flag and container children are untouched. The separate `enabled` boolean is removed from the two types that gained `enablement` so the same fact cannot be recorded twice and disagree; consumers derive it through `isCustomizationEnabled()`. `getEffectiveMcpServerCustomizations()` now returns each server with its effective value, applying the container gate at the point of use rather than by overwriting a child's stored decision, so re-enabling a plugin restores each child to whatever the user chose for it. No storage, resolution, launch filtering or UI behaviour changes here. Refs: ENABLEMENT-SPEC §3 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Give the host somewhere to keep enablement decisions and one place that resolves them. Global and workspace decisions go in a new host key/value store at globalStorage/agent-host-storage.json. Session decisions go in that session's own database, so "session" means this session permanently rather than until the app closes. Keeping global on the host too means a global decision survives even when the client that made it is not connected. Decisions are keyed by durable identity -- a plugin's source URI, and `<plugin source URI>#mcp=<name>` for a server it contributes -- never by customization id. A plugin child's id contains the materialized path and a hash of the plugin's contents, so keying on it would silently forget the user's choice on every plugin edit and the server would quietly come back. Session scope is the exception: its key only has to be stable within one session, so the id is enough there. Only decisions that differ from what would be inherited are stored, so setting a scope to match its inherited value clears the entry instead of pinning it. Stored decisions are capped by a 512-entry LRU, so decisions belonging to plugins that are gone age out on their own. Resolution never computes from state that has not arrived yet. A session's working directory is modelled as directory, workspace-less or pending, so a directory that is merely unregistered can no longer be mistaken for a session that has none. Resolution reports pending rather than claiming no decision exists, and the service announces the transitions -- session decisions finishing loading, and a working directory becoming known -- so nothing is left displaying an answer resolved under stale inputs. Session decisions are cached in memory at session open and read synchronously, because an async read during publishing would reintroduce that same absent-state problem. Nothing consumes this yet: no publishing, launch filtering or UI changes. Refs: ENABLEMENT-SPEC §2, §5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The client stops deciding enablement and starts reporting it. Everything the client knows about is now published, including things that are turned off. Each plugin and MCP server carries an explicit global decision, and that entry is always present -- even when the answer is "enabled" -- so the host never has to infer intent from missing data. The global entry is VS Code's profile-level value only. VS Code's own workspace scope is deliberately not published: the host owns workspace and session scope, and its "workspace" means a working directory, which is a different axis. Reading it needs a new `readProfileEnabled()`, because `readEnabled()` collapses profile and workspace with workspace winning, and publishing that would have leaked one model into the other. Legacy sessions keep using `readEnabled()` and are unaffected. MCP servers configured in VS Code reach the host inside a synthetic plugin's `.mcp.json` rather than as published customizations, so they cannot carry `enablement` themselves. `ClientPluginCustomization` gains `childEnablement` to carry their decisions alongside the bundle. The per-plugin sync checkbox is gone. It controlled whether a plugin was sent to the host at all, which was a second way of saying "off". The same provider also backs per-file opt-out, which is unrelated and stays. Also gone is the reconciliation that pushed client-owned MCP enablement at the start of every turn. That raced the host's asynchronous discovery of MCP servers, and is the reason neither side reliably knew the truth. One consequence is deliberate and not yet complete: disabled servers now remain in the synthetic bundle's `.mcp.json`, because removing them would make it impossible to enable a server for a single workspace after disabling it globally. Nothing consumes `childEnablement` yet, so such a server still starts. Closing that is the job of the single enforcement gate, and the bundler says so at the point it happens. Refs: ENABLEMENT-SPEC §4, §9 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Give enablement exactly one enforcement point: the place the host builds what it hands to a provider SDK. Everything is still published and still materialised, so a disabled thing appears in the UI with a reason rather than silently vanishing. The gate is shared by all three providers and derives from a single resolution call, so Claude, Copilot and Codex cannot drift apart. A disabled plugin's directory is never passed, which matters because the SDK discovers `.mcp.json` inside any directory it is given. That last point needed more than skipping disabled directories. MCP servers configured in VS Code are bundled into a synthetic plugin that is itself always enabled, and disabled servers have to stay in its `.mcp.json` -- removing them would make it impossible to enable a server for one workspace after disabling it globally. So the host now consumes `childEnablement`, recording the client's decision under each child's durable key and replacing only the global scope, leaving workspace and session decisions alone. Toggling a customization now records the decision in the enablement service rather than only mutating published state. Without this a session-scoped decision lived in the reducer while resolution read from the service, and the two disagreed -- the same fact in two places, which is the problem this feature exists to remove. A decision made before its session's working directory or stored decisions have arrived is queued and applied once they do, instead of being dropped. Resolution that has not settled yet is published truthfully, carrying no invented decision, and fails closed only at the SDK boundary. Attributing it to a scope nobody chose would have shown every customization as disabled at session scope during startup and offered to undo a decision that did not exist. Some MCP servers are discovered by the SDK from a directory and cannot be prevented from starting until the session-scoped `disabledMcpServers` option is available to us. Until then a disabled server is reported as disabled and its authentication prompts are suppressed, driven by the same resolved value as the gate. Refs: ENABLEMENT-SPEC §7 Refs: github/copilot-sdk#2260 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A decision that is stored but not republished leaves the UI showing state resolved under the old policy, which looks exactly like the change not working. The enablement service already announced every change, but nothing listened, so nothing was ever refreshed. Side effects now subscribe and republish through the existing path, so the state-based deduplication and its guard against envelope storms still apply. Which sessions are affected depends on the scope. A session decision touches only its own session. A workspace decision touches every session whose primary working directory matches. A global decision can touch any open session, since the same plugin or MCP server is commonly open in several at once and the host holds global decisions on behalf of clients that may not be connected. Sessions are collected into a set, so one change refreshes each affected session at most once, and the affected set is computed by comparing the decision before and after the write so a no-op change fans out to nobody. Replacing a customization's decisions did not announce anything at all, which is the path a toggle takes, so toggles were silently failing to reach the UI. Refreshes are serialized per session. Two overlapping refreshes would each read the same pre-dispatch state, both conclude they had something new, and both dispatch. A refresh whose snapshot is overtaken by an agent's own publish is retried rather than dropped, since dropping it loses the very change that triggered it. A session whose agent has not registered yet is remembered and refreshed once it appears. Refs: ENABLEMENT-SPEC §12 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A row that only says "Disabled" leaves the user guessing which scope decided it, and makes the menu carry that burden instead. The row now says why: "Disabled (Workspace)", "Disabled (Session)", or plain "Disabled" for a global decision. The client computes none of this. The host publishes decisions sorted most specific first, and a single accessor owns the rule that the first entry wins; the effective boolean is now derived through it too. Widgets never index the list themselves, so precedence cannot drift into the view layer. The reason is a tagged shape rather than a bare scope, so the container cascade can add its own case later without reworking what is here. The label switch is exhaustive over the known scopes, so a new one becomes a compile error rather than silently falling back to a bare "Disabled". Rows backed by VS Code's own enablement are untouched and continue to read plainly, since their state does not come from the host and has no scope to report. Screen readers get the same reason as the visible label. Refs: ENABLEMENT-SPEC §8 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A row now offers a way back at every scope that applies to it. Each action shows the inverse of what the state would be if nothing more specific were set, so the workspace action reflects workspace-or-global and the session action reflects the current effective value. A globally disabled row therefore offers to enable it for this workspace rather than pointlessly offering to disable it again. Two actions per scope is enough only because setting a scope to match what it would inherit clears the entry instead of pinning it, which is what removes the need for a third "Inherit" action. The per-scope values come from one shared derivation, so no widget ranks scopes itself. Changes are dispatched as a complete replacement built from the customization's published decisions, so changing one scope cannot silently drop another. Actions that cannot take effect are hidden rather than greyed, since the row label already carries the reason. A session with no working directory offers global and session only, and starting, stopping and authenticating disappear from a disabled row because the server is not running. A built-in MCP server that is also present in the active session used to offer VS Code's own scopes plus an agent-host session toggle, which left no way to undo an agent workspace decision from that row. Its workspace and session actions now act on the agent host when an agent session is the current context, while its global action continues through VS Code's profile enablement, which is the shared global truth. Rows with no such context are untouched. Both paths share one action builder so they cannot drift. Refs: ENABLEMENT-SPEC §8 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Complete the container cascade and fix enablement for MCP servers a plugin contributes. A child that is off only because its plugin is off now says so, keeps its row rather than vanishing, and offers a single "Enable Plugin" action, since none of its own actions could take effect. The plugin reason wins over the child's own decisions for display and actions, while those decisions stay recorded underneath and re-surface when the plugin comes back. Enabling a plugin follows who owns the decision: global on a plugin the client published is written through the client's own enablement, and everything else goes to the host. Writing global to the host for a client-published plugin looked like it worked and was then overwritten by the client's next publish, so the action silently did nothing. A disabled plugin's agents are now published like everything else and filtered where they are selected, so the container gate applies when the data is used rather than by dropping it at publish time. Two enablement bugs found by hand: A client republish carried no opinion about a plugin's own MCP children, since the client only knows about servers it bundled itself, and that absent opinion was applied as "enabled" -- erasing the user's decision on every republish. Absence now means no opinion; only a real global entry is treated as the client's. Enablement was only reconciled with a provider SDK when a session bound or sent, so a decision taken mid-session did not reach a running session until the next message: the server stayed callable while the UI showed it disabled. Claude and Copilot now reconcile when customizations change, serialized against the existing reconciliation and skipped when nothing differs. Codex supplies its servers only at thread start and has no live path, which is noted where they are supplied. Refs: ENABLEMENT-SPEC §6, §8 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…blement-2 # Conflicts: # src/vs/platform/agentHost/node/agentHostChangesetFileMonitorCoordinator.ts # src/vs/platform/agentHost/node/agentHostMain.ts # src/vs/platform/agentHost/node/agentHostServerMain.ts # src/vs/platform/agentHost/node/agentService.ts # src/vs/platform/agentHost/node/codex/codexAgent.ts # src/vs/platform/agentHost/node/copilot/copilotAgent.ts # src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts # src/vs/platform/agentHost/test/node/agentHostChangesetService.test.ts # src/vs/platform/agentHost/test/node/agentHostSkillCompletionProvider.test.ts # src/vs/platform/agentHost/test/node/agentService.test.ts # src/vs/platform/agentHost/test/node/claudeAgent.test.ts # src/vs/sessions/contrib/providers/agentHost/test/browser/localAgentHostSessionsProvider.test.ts # src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLocalCustomizations.ts # src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts # src/vs/workbench/contrib/chat/test/browser/agentSessions/resolveCustomizationRefs.test.ts
Enablement was stored correctly but kept coming back on, so a disabled MCP server still reached the model. Several distinct faults produced that one symptom. A decision erased itself. Recording one fired a republish, which re-applied the client's stale global assertion, and because that value matched the default it deleted the entry that had just been written. Enablement is now an overlay: the client's published value is a base and host decisions sit on top of it, so the client can no longer write into host storage at all. Whether a decision differs from what it inherits is measured against that base rather than a fixed default, so a host decision matching the default is no longer silently dropped. A server's durable key depended on where it happened to appear. Nested under its plugin it keyed one way, published on its own another, so a fresh session looked under a key nothing had ever been written to. A server now carries the source of the plugin that contributes it, and its key is derived from that alone. Several publications skipped resolution entirely and emitted a customization with no decisions, which reads as enabled. Those paths now resolve first, and reconciliation with the provider SDK derives from the same gated view as the launch handoff, so it can no longer re-enable a server that startup deliberately skipped. A server the client contributes has its global decision owned by the client, so sending that decision to the host did nothing at all. It now follows the same ownership rule as everything else: global through the client for what the client bundles, workspace and session always through the host. Rows take their state from the session's own view when there is one, so a row and its menu can no longer disagree. Refs: ENABLEMENT-SPEC §2, §4, §5, §7 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…blement-2 # Conflicts: # src/vs/platform/agentHost/node/agentHostChangesetService.ts # src/vs/platform/agentHost/node/agentHostMain.ts # src/vs/platform/agentHost/node/agentHostServerMain.ts # src/vs/platform/agentHost/node/agentHostSkillCompletionProvider.ts # src/vs/platform/agentHost/node/agentService.ts # src/vs/platform/agentHost/node/agentSideEffects.ts # src/vs/platform/agentHost/node/claude/claudeAgent.ts # src/vs/platform/agentHost/node/claude/claudeAgentSession.ts # src/vs/platform/agentHost/node/codex/codexAgent.ts # src/vs/platform/agentHost/node/copilot/copilotAgent.ts # src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts # src/vs/platform/agentHost/test/node/agentConfigurationService.test.ts # src/vs/platform/agentHost/test/node/agentService.test.ts # src/vs/platform/agentHost/test/node/claudeAgent.test.ts # src/vs/platform/agentHost/test/node/copilotAgent.test.ts # src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts # src/vs/platform/agentHost/test/node/shared/mcpCustomizationController.test.ts # src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts # src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostActiveClientService.ts # src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLocalCustomizations.ts # src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/syncedCustomizationBundler.ts # src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts
There was a problem hiding this comment.
Pull request overview
Centralizes scoped plugin and MCP enablement in the agent host, persists decisions, exposes disabled reasons, and gates provider SDK configuration.
Changes:
- Adds global, workspace, and session enablement resolution and persistence.
- Propagates scoped state through providers, UI actions, and protocol models.
- Updates provider integrations and tests for the new enablement model.
Four blocking correctness issues were identified, including launch-time filtering and stale Claude/Copilot state.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Ignores local validation artifacts. |
src/vs/platform/agentHost/common/agentHostCustomizationConfig.ts |
Removes legacy enabled flag. |
src/vs/platform/agentHost/common/customAgents.ts |
Filters agents by plugin enablement. |
src/vs/platform/agentHost/common/customizationEnablement.ts |
Adds shared enablement utilities. |
src/vs/platform/agentHost/common/state/protocol/channels-session/actions.ts |
Updates enablement action contract. |
src/vs/platform/agentHost/common/state/protocol/channels-session/reducer.ts |
Applies scoped decisions. |
src/vs/platform/agentHost/common/state/protocol/channels-session/state.ts |
Adds protocol enablement types. |
src/vs/platform/agentHost/node/agentConfigurationService.ts |
Refactors working-directory resolution. |
src/vs/platform/agentHost/node/agentHostChangesetFileMonitorCoordinator.ts |
Uses shared directory helpers. |
src/vs/platform/agentHost/node/agentHostChangesetService.ts |
Updates directory lookup. |
src/vs/platform/agentHost/node/agentHostCustomizationEnablementService.ts |
Owns scoped persistence and resolution. |
src/vs/platform/agentHost/node/agentHostMain.ts |
Registers new host services. |
src/vs/platform/agentHost/node/agentHostServerMain.ts |
Wires persistence and shutdown flushing. |
src/vs/platform/agentHost/node/agentHostSkillCompletionProvider.ts |
Honors plugin enablement. |
src/vs/platform/agentHost/node/agentHostStorageService.ts |
Adds persistent host storage. |
src/vs/platform/agentHost/node/agentService.ts |
Instantiates enablement infrastructure. |
src/vs/platform/agentHost/node/claude/claudeAgent.ts |
Serializes customization synchronization. |
src/vs/platform/agentHost/node/claude/claudeAgentSession.ts |
Resolves Claude customization enablement. |
src/vs/platform/agentHost/node/claude/customizations/claudeSessionClientCustomizationsModel.ts |
Updates snapshot equality. |
src/vs/platform/agentHost/node/claude/customizations/claudeSessionCustomizationDiscovery.ts |
Removes legacy defaults. |
src/vs/platform/agentHost/node/codex/codexAgent.ts |
Gates Codex MCP configuration. |
src/vs/platform/agentHost/node/codex/codexClientCustomizations.ts |
Filters enabled Codex plugins and servers. |
src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts |
Reconciles live MCP enablement. |
src/vs/platform/agentHost/node/copilot/copilotSessionLauncher.ts |
Supplies disabled servers to the SDK. |
src/vs/platform/agentHost/node/copilot/copilotSlashCommandCompletionProvider.ts |
Filters disabled plugin commands. |
src/vs/platform/agentHost/node/sessionPermissions.ts |
Uses shared directory resolution. |
src/vs/platform/agentHost/node/shared/customizationEnablementGate.ts |
Adds the provider SDK gate. |
src/vs/platform/agentHost/node/shared/sessionPluginBundler.ts |
Removes legacy enablement state. |
src/vs/platform/agentHost/node/shared/worktreeIsolation.ts |
Publishes pending-directory changes. |
src/vs/platform/agentHost/test/electron-browser/remoteAgentHostProtocolClient.test.ts |
Updates protocol fixtures. |
src/vs/platform/agentHost/test/node/agentConfigurationService.test.ts |
Tests directory helpers. |
src/vs/platform/agentHost/test/node/agentHostChangesetOperationService.test.ts |
Updates service stub. |
src/vs/platform/agentHost/test/node/agentHostSeams.test.ts |
Updates customization assertions. |
src/vs/platform/agentHost/test/node/agentHostSkillCompletionProvider.test.ts |
Tests disabled plugin filtering. |
src/vs/platform/agentHost/test/node/agentHostStorageService.test.ts |
Tests host persistence. |
src/vs/platform/agentHost/test/node/agentHostToolCallTelemetry.test.ts |
Updates enablement dependencies. |
src/vs/platform/agentHost/test/node/agentHostTurnHangTelemetry.test.ts |
Updates enablement dependencies. |
src/vs/platform/agentHost/test/node/agentHostTurnTelemetry.test.ts |
Updates enablement dependencies. |
src/vs/platform/agentHost/test/node/agentPluginManager.test.ts |
Updates plugin fixtures. |
src/vs/platform/agentHost/test/node/agentService.test.ts |
Updates service and protocol tests. |
src/vs/platform/agentHost/test/node/codex/codexClientCustomizations.test.ts |
Tests Codex enablement. |
src/vs/platform/agentHost/test/node/codex/codexPrewarmEviction.test.ts |
Updates fixtures. |
src/vs/platform/agentHost/test/node/copilotAgent.test.ts |
Tests scoped Copilot behavior. |
src/vs/platform/agentHost/test/node/copilotPluginConverters.test.ts |
Updates MCP fixtures. |
src/vs/platform/agentHost/test/node/copilotSessionLauncher.test.ts |
Tests disabled server options. |
src/vs/platform/agentHost/test/node/copilotShellTools.test.ts |
Updates configuration stub. |
src/vs/platform/agentHost/test/node/copilotSlashCommandCompletionProvider.test.ts |
Tests plugin filtering. |
src/vs/platform/agentHost/test/node/customizations/claudeSessionClientCustomizationsModel.test.ts |
Updates scoped fixtures. |
src/vs/platform/agentHost/test/node/customizations/claudeSessionCustomizationDiscovery.test.ts |
Updates discovery fixtures. |
src/vs/platform/agentHost/test/node/customizations/scan/claudeMcpScan.test.ts |
Tests derived enablement. |
src/vs/platform/agentHost/test/node/e2e/suites/clientFilesystemSuite.ts |
Updates E2E fixtures. |
src/vs/platform/agentHost/test/node/e2e/suites/customizationDiscoverySuite.ts |
Updates E2E fixtures. |
src/vs/platform/agentHost/test/node/e2e/suites/mcpPluginSuite.ts |
Tests scoped toggle actions. |
src/vs/platform/agentHost/test/node/providerIntegration/codexCustomizations.integrationTest.ts |
Updates Codex integration fixtures. |
src/vs/platform/agentHost/test/node/providerIntegration/copilotCustomizations.integrationTest.ts |
Updates Copilot integration fixtures. |
src/vs/platform/agentHost/test/node/reducers.test.ts |
Tests enablement replacement. |
src/vs/platform/agentHost/test/node/shared/editArcReporter.test.ts |
Updates configuration stub. |
src/vs/platform/agentHost/test/node/shared/mcpCustomizationController.test.ts |
Tests ownership and masking. |
src/vs/platform/agentPlugins/common/pluginParsers.ts |
Removes legacy MCP defaults. |
src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts |
Updates parser expectations. |
src/vs/sessions/AI_CUSTOMIZATIONS.md |
Documents host-owned enablement. |
src/vs/sessions/SESSIONS.md |
Documents provider metadata. |
src/vs/sessions/common/agentHostSessionsProvider.ts |
Extends provider enablement API. |
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts |
Publishes and dispatches scoped state. |
src/vs/sessions/contrib/providers/agentHost/test/browser/agentHostAgents.test.ts |
Tests effective client agents. |
src/vs/sessions/contrib/providers/agentHost/test/browser/localAgentHostSessionsProvider.test.ts |
Updates provider fixtures. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHostCustomizationHarness.ts |
Removes legacy default. |
src/vs/sessions/services/agentHost/browser/agentHostCustomizationService.ts |
Routes enablement to providers. |
src/vs/workbench/contrib/chat/browser/agentPluginActions.ts |
Adds scoped plugin actions. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentCustomizationItemProvider.ts |
Publishes disabled reasons and actions. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLocalCustomizations.ts |
Mirrors disabled customizations. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts |
Uses derived MCP state. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/syncedCustomizationBundler.ts |
Bundles child enablement. |
src/vs/workbench/contrib/chat/browser/aiCustomization/pluginListWidget.ts |
Displays scoped disabled labels. |
src/vs/workbench/contrib/chat/browser/chatDebug/agentHostChatDebugProvider.ts |
Reports derived enablement. |
src/vs/workbench/contrib/chat/common/customizationHarnessService.ts |
Adds disabled-reason presentation. |
src/vs/workbench/contrib/chat/common/enablement.ts |
Adds profile-only lookup. |
src/vs/workbench/contrib/chat/test/browser/agentHostChatDebugProvider.test.ts |
Updates debug fixtures. |
src/vs/workbench/contrib/chat/test/browser/agentPluginActions.test.ts |
Tests scoped plugin actions. |
src/vs/workbench/contrib/chat/test/browser/agentSessions/agentCustomizationItemProvider.test.ts |
Tests disabled reasons. |
src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostUntitledProvisionalSessionService.test.ts |
Updates plugin fixtures. |
src/vs/workbench/contrib/chat/test/browser/agentSessions/syncedCustomizationBundler.test.ts |
Tests child enablement bundling. |
src/vs/workbench/contrib/chat/test/browser/aiCustomization/aiCustomizationItemsModel.test.ts |
Updates enablement stubs. |
src/vs/workbench/contrib/chat/test/browser/aiCustomization/aiCustomizationListWidget.test.ts |
Updates enablement stubs. |
src/vs/workbench/contrib/chat/test/browser/aiCustomization/pluginListWidget.test.ts |
Tests disabled labels. |
src/vs/workbench/contrib/chat/test/common/plugins/agentPluginEnablement.test.ts |
Updates profile-state model. |
src/vs/workbench/contrib/chat/test/common/plugins/agentPluginFormatDetection.test.ts |
Updates enablement stub. |
src/vs/workbench/contrib/chat/test/common/plugins/convertBareEnvVarsToVsCodeSyntax.test.ts |
Updates MCP fixture. |
src/vs/workbench/contrib/chat/test/common/promptSyntax/computeAutomaticInstructions.test.ts |
Updates enablement stub. |
src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts |
Updates enablement stub. |
src/vs/workbench/contrib/mcp/browser/mcpCommands.ts |
Adds scoped MCP controls. |
src/vs/workbench/contrib/mcp/browser/mcpLanguageFeatures.ts |
Uses derived enablement. |
src/vs/workbench/contrib/mcp/test/common/testMcpService.ts |
Updates test model. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 107/108 changed files
- Comments generated: 4
- Review effort level: Balanced
Fixes the 11 integration test failures this branch introduced, plus four review findings. CI regressions: - Restore `setWorktreeIsolation` (and the `CopilotApiService` / `WorktreeIsolation` construction it depends on) inside `if (!options.quiet)` in agentHostServerMain. Integration test servers run with `--quiet`, so hoisting this out gave them host-owned isolation schema for the first time, resolving a non-git working directory to `isolation: 'folder'` and dropping `branch`. Broke 7 sessionConfig and toolApproval tests. - Register `IAgentHostCustomizationEnablementService` in the Claude integration test's own service collection via a new shared `createNoopCustomizationEnablementService()` helper. - Codex cached a transient `workingDirectory`-pending verdict at plugin parse time, permanently excluding the plugin from the SDK projection. Re-resolve enablement at each projection instead. Review findings: - Gate root-configured MCP servers at Copilot launch. Only plugin children were filtered, so a globally or workspace-disabled root server could start and request authentication before the turn-start reconcile ran. - Resolve MCP auth enablement live rather than from the immutable launch snapshot, suppressing only on a definitive disabled decision so servers we have no opinion about still authenticate. - Compare Claude client customizations structurally, covering `childEnablement`, so identical snapshots stop marking the plugin set dirty and child-only toggles trigger reconciliation. - Track Claude client enablement per client and rebuild the merged maps on snapshot replace or client removal, so decisions omitted by a new snapshot no longer leak onto a newly parsed plugin. Also adds a `customizations-in-the-agent-host` skill documenting the overlay model, host-vs-client global ownership, position-independent identity, and the "every publication path goes through resolution" rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…blement-2 # Conflicts: # src/vs/platform/agentHost/node/copilot/copilotAgent.ts # src/vs/platform/agentHost/test/node/agentSideEffects.test.ts # src/vs/platform/agentHost/test/node/copilotAgent.test.ts # src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/vs/platform/agentHost/node/agentHostCustomizationEnablementService.ts:206
- Queued workspace/session changes are never replayed when worktree resolution is the event that clears the pending state.
_applyPendingReplacementsis called for session-load and working-directory actions, but this callback only republishes the old decision; if no later directory action occurs, the user's change remains queued indefinitely. Replay the session's pending replacements here before notifying listeners.
this._notifyDecisionChanged([session]);
src/vs/workbench/contrib/mcp/browser/mcpCommands.ts:530
- This treats every server that happens to match a local definition as client-owned. A host-discovered plugin server can have the same raw ID or unique display name, in which case its global action is written to the local enablement model and cannot affect the host-owned decision. Match
getBuiltinMcpServerEnablementActionsand route all scopes through the host whenserver.isPluginProvided && !server.isClientBundled; apply that ownership check both while building items and while executing the profile action.
src/vs/platform/agentHost/common/customAgents.ts:68 - This filter has no production caller:
NewSession.getClientCustomAgents()still returns_activeClientScope.customAgentsdirectly, andBaseAgentHostSessionsProvider.getCustomAgents()merges that complete list into the draft picker. Consequently an agent contributed by a disabled client plugin remains selectable before host materialization, despite this helper's test passing. Apply the filter at that draft-agent boundary using the scope'scustomizationsobservable.
export function getEffectiveClientAgents(
clientCustomizations: readonly ClientPluginCustomization[] | undefined,
clientAgents: readonly AgentCustomization[],
): readonly AgentCustomization[] {
if (!clientCustomizations || clientCustomizations.length === 0) {
return clientAgents;
}
return clientAgents.filter(agent => {
const agentUri = URI.parse(agent.uri);
const plugin = clientCustomizations.find(candidate => isEqualOrParent(agentUri, URI.parse(candidate.uri)));
return !plugin || isCustomizationEnabled(plugin);
- Files reviewed: 110/111 changed files
- Comments generated: 1
- Review effort level: Balanced
| export function getCustomizationEnablementKey(target: ICustomizationEnablementTarget, kind: CustomizationEnablementKind): string { | ||
| if (kind === CustomizationEnablementKind.Session) { | ||
| return target.id; | ||
| } | ||
|
|
||
| switch (target.type) { | ||
| case CustomizationType.Plugin: | ||
| return target.source.toString(); | ||
| case CustomizationType.McpServer: | ||
| return target.owningPluginSource | ||
| ? `${target.owningPluginSource.toString()}#mcp=${target.name}` | ||
| : `mcpServers#${target.name}`; | ||
| default: | ||
| throw new Error(`Enablement is only supported for plugins and MCP servers, not ${target.type}`); | ||
| } | ||
| } |
The Copilot session's first customization sync awaited `_enablementReady` and then `_publish` added a further `.then()` microtask hop. Whether that continuation ran before or after the session's startup actions was down to microtask scheduling, so `session/customizationsChanged` landed at a nondeterministic point relative to `session/chatUpdated` and `session/titleChanged`. The agent host E2E traffic snapshot failed roughly two runs in three. Track whether enablement has resolved and, once it has, run the sync and the publish synchronously rather than scheduling another microtask. Before resolution both still await, so customizations are never published while the enablement decision is pending, and publication continues to precede `session/ready`. Verified with 8 consecutive runs of the previously flaky test, all passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Tyler James Leonhardt (@TylerLeonhardt)Matched files:
|
`McpServerCustomization.owningPluginUri` had no consumers outside the host's own `node/` layer: it was written by `McpCustomizationController` from an in-memory map and read only by `targetForMcpServer` to derive a durable enablement key. Publishing it made host bookkeeping part of the protocol surface, which the Agent Host Protocol doctrine keeps behind the host boundary, so it is not part of the spec. Pass ownership explicitly instead. Nested MCP servers already have their plugin in hand and pass its URI directly, so `withOwningPluginUri` is gone. Top-level servers — which exist because a server the SDK reports before its plugin's children are published is promoted to top-level for the rest of the session — resolve their owner through a host-only map threaded from the controller's existing `pluginMcpServerSources`. `getCustomizationEnablementKey` is unchanged, so stored decisions keep resolving: a server published top-level and the same server published under its plugin still derive the same durable key. The gate test that pins that equivalence now supplies ownership through the map instead of the published field. `isClientBundled` stays on the protocol type; it is genuinely client-facing and routes the Disable action in the customizations UI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Connor Peet (@connor4312) Would this address #329475 as well? |
| switch (customization.type) { | ||
| case CustomizationType.Plugin: | ||
| case CustomizationType.McpServer: { | ||
| if (enablement.length > 0) { |
There was a problem hiding this comment.
AI Review: This now dereferences enablement unconditionally, but SessionCustomizationToggled is still version 1 and is still registered as introduced in 0.1.0 even though its payload changed from { enabled } to { enablement }. An older client can therefore dispatch the old shape and crash this reducer at enablement.length. Please version/capability-gate the new action or accept both shapes during the mixed-version compatibility window.
| /** Whether to enable or disable the targeted customization. */ | ||
| enabled: boolean; | ||
| /** Explicit enablement decisions, replacing the previous list entirely. */ | ||
| enablement: CustomizationEnablement[]; |
There was a problem hiding this comment.
AI Review: A UI gesture changes one scope, but this payload replaces every explicit decision. Two clients starting from the same snapshot can independently change Workspace and Session; whichever full array arrives last silently clears the other scope. Since the host service already has scoped set semantics, could this action encode the changed { kind, enabled } (or merge against current host state) instead of trusting a client snapshot?
| // reads the current pending state, and any later clear is observed. | ||
| return; | ||
| } | ||
| this._notifyDecisionChanged([session]); |
There was a problem hiding this comment.
AI Review: When worktree pending clears, this listener only republishes. A Workspace/Session replacement queued while resolve() returned pending remains in _pendingReplacements; replay currently happens only on session load or SessionWorkingDirectorySet/Removed. SessionConfigChanged from worktree to folder and failed first-send worktree resolution can clear pending without either action, so the user's toggle can remain unapplied indefinitely. Please replay _applyPendingReplacements(session) here before notifying.
| decisions = new Map(); | ||
| this._clientGlobalEnablement.set(session, decisions); | ||
| } | ||
| decisions.set(key, enabled); |
There was a problem hiding this comment.
AI Review: _clientGlobalEnablement is append-only. Removing an active client, plugin, or bundled child never removes its durable key, so a later host-discovered/unbundled customization with the same URI inherits the stale client base. This is the append-only-map failure described in the new architecture guide and it undermines the new replacement/removal scenarios. Please replace each session's client-base snapshot or explicitly clear keys that disappeared.
| const global = this._persistent.global ?? {}; | ||
| this._setPersistentDecision('global', global, key, enabled, this._clientGlobalEnablement.get(session)?.get(key) ?? DEFAULT_CUSTOMIZATION_ENABLED); | ||
| this._persistent = { ...this._persistent, global }; | ||
| if (removeRedundantWorkspaceDecisions && global[key] !== undefined) { |
There was a problem hiding this comment.
AI Review: Pruning is skipped when _setPersistentDecision deletes the Global key because the requested value matches the client base. In that transition, an existing Workspace override can become equal to inherited Global and should be removed too. The new retains and clears host decisions relative to the client base test expects persisted state to become empty, but this guard leaves the workspace entry. Please prune against the effective Global value even when no host Global key remains.
|
|
||
| private _setWorkspace(session: string, target: ICustomizationEnablementTarget, workingDirectory: URI, enabled: boolean | undefined): void { | ||
| const key = this._persistentKey(target); | ||
| const directoryKey = workingDirectory.toString(); |
There was a problem hiding this comment.
AI Review: Workspace persistence uses raw URI.toString() keys. Equivalent file URIs with path-case or separator differences resolve to different buckets, so a durable Workspace decision can disappear across session recreation/restart, especially on Windows. Please use a filesystem-aware ExtUri comparison key/normalization consistently for lookup, write, and LRU bookkeeping.
|
|
||
| private _persistent: IPersistedEnablement; | ||
| private _lru: ILruEntry[]; | ||
| private readonly _clientGlobalEnablement = new Map<string, Map<string, boolean>>(); |
There was a problem hiding this comment.
AI Review: The new per-session state has no definitive deletion cleanup: _clientGlobalEnablement, _sessionEnablement, _sessionLoads, _sessionsById, and queued replacements retain deleted session URIs for the host lifetime. Besides unbounded growth, reusing a URI would skip the database reload and resurrect stale decisions. Please clear all per-session state from the definitive session-deletion path (distinct from temporary list eviction).
| } | ||
| this._onDidCustomizationsChange.fire(); | ||
| if (this._pipeline) { | ||
| this._reconcileMcpServerEnablement(true).catch(error => this._logService.error(error, `[Claude:${this.sessionId}] Failed to reconcile MCP enablement after customizations changed`)); |
There was a problem hiding this comment.
AI Review: Reconciliation only starts once _pipeline already exists, after _sdkService.startup() (initial startup at line 604; rebuild at line 709). Startup wiring does not apply child/settings MCP enablement, so an individually disabled plugin child or settings-discovered server can start and request authentication before this disables it. Effective enablement needs to be applied before both SDK startup paths, not only reconciled afterward.
| return getAgentHostMcpServerEnablementActions(agentHostCustomizations, agentPluginService, sessionResource, activeSessionServer); | ||
| } | ||
| return [ | ||
| ...getLocalMcpServerEnablementActions(mcpService, serverId, isEmptyWorkbench, { includeWorkspace: false, activeSessionServer }), |
There was a problem hiding this comment.
AI Review: For a locally backed row whose active server was discovered by the host, this sends Global changes only to VS Code and reserves host actions for Workspace/Session. Workspace .mcp.json servers are intentionally excluded from client sync, so that local Global write never updates the host-owned decision or the published row. Please use host Global actions whenever activeSessionServer.isClientBundled is false, including non-plugin host discovery, and reserve local Global for client-bundled servers.
| .filter(id => id !== undefined)); | ||
| return servers | ||
| .filter(server => server.enabled && server.state.kind === McpServerStatus.AuthRequired && !toolAuthServerIds.has(server.id)) | ||
| .filter(server => isCustomizationEnabled(server) && server.state.kind === McpServerStatus.AuthRequired && !toolAuthServerIds.has(server.id)) |
There was a problem hiding this comment.
AI Review: Flattening children here drops the containing plugin's enablement. A child can be individually enabled while its plugin is disabled, then survive this filter and appear in the authentication-required UI; mcpStarting$ repeats the same pattern around line 2955. This contradicts the effective parent && child gate and can prompt for a server that cannot run. Please carry container enablement into both projections.
| userInvocable: undefined, | ||
| actions: this._getItemActions?.(customization, clientId), | ||
| actions: [ | ||
| ...(clientId === undefined ? getAgentHostPluginEnablementActions(this._customAgentsService, undefined, sessionResource, customization, this._customAgentsService.getWorkingDirectories(sessionResource).length > 0) : []), |
There was a problem hiding this comment.
AI Review: The clientId === undefined branch withholds all host scoped actions from client-published plugins. In the local Agent Host construction _getItemActions is undefined, and the remote construction explicitly returns no actions for client reflections, so a plugin disabled at Workspace or Session has no recovery action in this view. Client ownership should restrict Global writeback, not host-owned Workspace/Session overrides; please expose equivalent scoped actions here.
Scoped enablement for MCP servers and plugins, so turning one off actually
turns it off — everywhere, and for as long as you meant it.
The problem
You could right-click an MCP server or plugin and disable it, but the row
kept showing its old state, the decision did not survive into new sessions,
and the server still started and still prompted for authentication.
Enablement was owned by the client and pushed to the agent host at the start
of a turn, so it raced the host's asynchronous discovery of MCP servers.
Neither side reliably knew the truth. A thing could also be turned off in two
different places by two different mechanisms, and they could disagree.
The model
The agent host owns enablement. There is one place decisions live and one
place they are resolved.
A decision is recorded at global, workspace, or session scope, and resolution
is always
session > workspace > global > enabled— the most specificdecision that exists wins, and absent means enabled. Each customization
publishes its decisions sorted most-specific-first, so the first entry is
decisive and nobody reimplements precedence. The client displays what the
host publishes and computes nothing.
Decisions are keyed by durable identity — a plugin's source URI, and
<plugin source URI>#mcp=<name>for a server it contributes — never bycustomization id, whose materialized path contains a content hash that
changes whenever the plugin is edited.
Only decisions that differ from what they inherit are stored, so setting a
scope back to its inherited value clears it. That is what makes two actions
per scope sufficient and removes the need for a third "Inherit" action.
What this changes for you
"Disabled (Plugin)" when its container is off.
applicable scope.
restores each child to whatever you chose for it rather than switching
everything on.
disabled, so you can disable one globally and still enable it for a single
agent workspace.
Enforcement
There is one gate: where the host builds what it hands to a provider SDK.
Everything is still published and materialized, so disabled things appear in
the UI with a reason instead of vanishing. A disabled plugin's directory is
never passed, because the SDK discovers
.mcp.jsoninside any directory itis given.
Notes for review
McpServerCustomizationgainsowningPluginUriandenablement, andClientPluginCustomizationgainschildEnablement. These are protocoladditions and need backporting to
agent-host-protocol.change does not reach a running thread. Noted at the supply site.
confirmed to persist into newly created sessions, with the decision visible
in host storage, on the wire, in the UI, and absent from the model's tools.