Repository navigation
feat(coding-agent): path-scoped disabledExtensions - #15716
chrisalexandrepena wants to merge 7 commits into
Conversation
disabledExtensions now accepts the same {path|paths, extensions} entries as disabledProviders, so an MCP server, skill, rule or context file can be turned off only under chosen directories. The /extensions dashboard writes one id at a time so toggling keeps those entries.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
P2 — scoped extension disables are a useful, focused feature, but the MCP isolation contract needs a live-session fix before merge.
Blocking: moving a connected session into a blocked directory re-scopes settings without disconnecting that MCP server or its tools. Should-fix: cover actual MCP discovery/cwd transitions in a regression test and add the TUI package changelog entry.
Verified the scoped-settings test (1 pass), native dashboard tests (2 pass), and a throwaway MCP discovery scenario in the scoped root, child, and unrelated directory; no live /move transition was exercised.
Maintainer call: is this path-scoped blocklist intended to apply immediately to existing MCP connections on /move? Thank you for the scoped-setting work.
| fs.mkdirSync(nestedDir, { recursive: true }); | ||
| fs.mkdirSync(otherDir, { recursive: true }); | ||
| const scopedEntry = { paths: [projectDir], extensions: ["mcp:work-server"] }; | ||
| await writeSettings({ disabledExtensions: ["skill:always-off", scopedEntry] }); |
There was a problem hiding this comment.
should-fix — test the MCP isolation contract. This test checks only cfgDisabledExtensions.get() and the saved YAML; it passes even if loadAllMCPConfigs()/loadCapability() ignores the resolved mcp:work-server id and connects the server in nestedDir. Add a consumer-level regression with a real discovered MCP config: assert the scoped server is excluded inside the tree (including a child) and present in otherDir. The feature's central safety claim is that the wrong-context server never loads, not just that its id resolves.
There was a problem hiding this comment.
Added in 96440f7: test/mcp-path-scoped-disabled-extensions.test.ts writes a real user mcp.json with two stdio servers plus a scoped disabledExtensions entry in config.yml, then asserts loadAllMCPConfigs drops work-server in the project dir and a child dir and keeps it in otherDir. The same file covers live connections across a cwd change.
| * Adds or removes one id in the persisted disabled list, leaving every other entry | ||
| * (including path-scoped ones) untouched. | ||
| */ | ||
| setExtensionDisabled(id: string, disabled: boolean): void; |
There was a problem hiding this comment.
should-fix — add packages/tui/CHANGELOG.md under [Unreleased]. This changes the exported ExtensionDashboardRuntime contract from whole-list setDisabledExtensions to per-id setExtensionDisabled and changes dashboard toggle behavior, but the PR updates only packages/coding-agent/CHANGELOG.md. Repository convention requires an entry in every touched package's changelog; describe the dashboard's user-visible scoped-entry preservation there.
There was a problem hiding this comment.
Added in 9299209: a packages/tui/CHANGELOG.md [Unreleased] Changed entry for setDisabledExtensions(ids) → setExtensionDisabled(id, disabled) and the dashboard keeping path-scoped entries on toggle.
| id: "disabledExtensions", | ||
| type: "array", | ||
| default: EMPTY_STRING_ARRAY, | ||
| pathScoped: { valuesKey: "extensions" }, |
There was a problem hiding this comment.
blocking — rescope live MCP connections on cwd changes. pathScoped makes cfgDisabledExtensions.get(settings) change when settings.reloadForCwd() runs, but InteractiveMode.applyCwdChange() (interactive-mode.ts:2750-2779) only refreshes skills/commands after that reload; it never disconnects/reloads mcpManager or removes its tools. Start in an unscoped directory with mcp:personal-mail connected, then /move the same session into ~/work: the newly excluded server stays connected and its tools remain callable until an explicit /mcp reload. The same setting now promises an MCP blocklist for that tree, so the move must reconcile live MCP connections (and the inverse move should rediscover allowed servers). Add a transition regression, not only the startup getter assertion.
There was a problem hiding this comment.
Fixed in 96440f7. InteractiveMode.applyCwdChange now snapshots the resolved disabledExtensions before reloadForCwd and, once the move commits, calls the new reconcileMcpDisabledExtensions (modes/components/extensions/mcp-runtime.ts). It disconnects servers whose mcp:<name> became disabled and rediscovers and connects ones that became allowed, reusing applyMcpToggleRuntime and its refreshMCPTools. Transition regression: test/mcp-path-scoped-disabled-extensions.test.ts uses a real MCPManager with real stdio servers to move into the tree (server disconnected, tools gone) and back out (reconnected); it fails with the reconcile disabled. I also checked it end to end with /move in tmux.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86bdb9a6ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id: "disabledExtensions", | ||
| type: "array", | ||
| default: EMPTY_STRING_ARRAY, | ||
| pathScoped: { valuesKey: "extensions" }, |
There was a problem hiding this comment.
When a session starts outside a matching prefix and then /moves into it, Settings.reloadForCwd() changes this handle's effective value, but the cfgExtensionSources listener in sdk.ts:4961-4965 only reconciles extension modules and skills, while InteractiveMode.applyCwdChange (interactive-mode.ts:2767-2778) never asks MCPManager to rediscover or disconnect. Consequently an already connected mcp:<name> tool remains registered and callable inside the directory this new entry is supposed to block (and moving out of a blocked prefix does not connect it) until a manual MCP reload or restart; reconcile MCP connections and tools when this scoped value changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 96440f7. Same fix as the thread above: a cwd change now reconciles live MCP connections and tools against the re-resolved disabledExtensions, both moving into a blocked prefix and moving out of one.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9299209266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cwd: newCwd, | ||
| manager: this.mcpManager, |
There was a problem hiding this comment.
When a session starts inside a scoped-blocked directory, then /moves out, and the newly connected server calls roots/list, the helper loads configuration from newCwd but passes the original manager unchanged. MCPManager captures its cwd only in the constructor (mcp/manager.ts:354-359) and answers roots/list from that fixed value (mcp/manager.ts:1164-1170), so the newly allowed server is advertised the blocked source directory as its workspace root; retarget the manager as part of the move before connecting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 946dff0. The reconcile now calls the new MCPManager.setCwd with the session cwd before connecting anything, so a server that becomes allowed after /move gets the new directory from roots/list. Covered in test/mcp-path-scoped-disabled-extensions.test.ts: the fixture server (new --roots-echo mode) asks roots/list while handling a tool call and echoes the root; after moving out of the scoped tree it reports the new directory, and the assertion fails without setCwd.
| ); | ||
| return false; | ||
| } | ||
| await this.#reconcileMcpForCwd(newCwd, disabledExtensionsBefore); |
There was a problem hiding this comment.
The TUI fix does not cover RPC: rpc-mode.ts:1799-1807 dispatches /move through the shared headless handler, whose rescopeHeadlessToCwd only reloads settings, skills, and plugins (slash-commands/builtin-lifecycle.ts:847-858). Consequently, a long-lived RPC session that starts outside a matching prefix with an MCP connected and moves into it leaves that now-blocked server's tools connected and callable. This is the fresh remaining path after the earlier TUI-only report; route the same before/after MCP reconciliation through the headless move path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 946dff0. The reconcile moved out of the TUI into the main session in sdk.ts: a cfgDisabledExtensions listener (serialized like the extension-source reconcile) runs reconcileMcpDisabledExtensions from the new mcp/disabled-extensions.ts whenever the resolved list changes. reloadForCwd fires it, so TUI /move, RPC/headless /move via rescopeHeadlessToCwd, cross-project resume and live config edits all go through one path; the TUI-only code from round 1 is gone. Verified with a real --mode rpc process: /move into the scoped tree stopped the server, /move out started it again.
… retarget the manager cwd
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 946dff0a01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| if (blocked.length === 0 && allowed.length === 0) return new Map(); | ||
|
|
||
| for (const name of blocked) await manager.disconnectServer(name); |
There was a problem hiding this comment.
When a hasUI session changes cwd or reloads config after deferred startup discovery has captured the old policy but before loadConfigs returns, this loop runs before discovery has populated any pending connection, so disconnectServer is a no-op; MCPManager.#discoverAndConnect then calls connectServers with the stale result and re-registers the now-blocked server and its tools, with no later settings change to remove them. The fresh remaining race after the earlier cwd reconciliation fixes is that #discoveryInFlight is not joined here (unlike the manager's project-config reconciler), so serialize or cancel startup discovery and re-evaluate the blocked set afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eac2223. The reconcile moved into MCPManager.reconcileDisabledExtensions, which runs on the same serialized tail as reconcileProjectConfig and awaits #discoveryInFlight first. It also blocks by current state rather than by diff: every disabled mcp: server that is still connected or connecting disconnects, so a discovery that loaded the old list is corrected once it settles. New manager-level test: a gated config loader holds discovery mid-load while a list change blocks one server; after both settle only the other server's tools remain. It fails when the discovery join is removed.
|
|
||
| let errors = new Map<string, string>(); | ||
| if (allowed.length > 0) { | ||
| const loaded = await loadAllMCPConfigs(cwd, discovery); |
There was a problem hiding this comment.
When a session starts in a subtree that scopes mcp:exa off and later moves out, this rediscovery runs with filterExa: true; loadAllMCPConfigs therefore removes the Exa server and returns its configured credential only in loaded.exaApiKeys, but this helper drops that field and connects no server. Unlike initial discovery, the SDK never calls applyMCPEnvironment, so the newly allowed configuration still does not provide its key to the native Exa integration and searches that rely on that configured key remain unauthenticated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eac2223. The reconcile now returns the rediscovery's exaApiKeys with the connect result, and the sdk listener passes it through applyMCPEnvironment, the same as initial discovery. Covered by a session test: start where mcp:exa is scoped off, move out, and EXA_API_KEY is set from the exa server's config. It fails without the applyMCPEnvironment call.
…artup discovery and apply rediscovered Exa keys
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac22239e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const disabled = new Set<string>(); | ||
| for (const id of after) | ||
| if (id.startsWith(MCP_EXTENSION_PREFIX)) disabled.add(id.slice(MCP_EXTENSION_PREFIX.length)); | ||
| const blocked = [...disabled].filter(name => this.getConnectionStatus(name) !== "disconnected"); |
There was a problem hiding this comment.
When a previously connected MCP server has lost its transport and its reconnect attempt has settled, getConnectionStatus() returns disconnected, but the manager intentionally retains its stale tools and, for remote servers, a scheduled retry. Entering a path that adds mcp:<name> therefore omits it from blocked, so disconnectServer() never clears the tools, preserved config, or retry; the tool remains exposed and the retry can reconnect the server inside the forbidden scope. Treat a newly blocked id as requiring teardown regardless of current transport status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5efb087. The reconcile now tears down every server in the new disabled set whatever its status. disconnectServer clears the tools, the config and the lost-remote retry, and it is a no-op for unknown names. New test: a remote server whose transport dropped (failed ladder, retry scheduled) is blocked, its tools disappear, and it stays disconnected after the remote comes back. It fails if teardown skips disconnected servers.
| if (!manager) return; | ||
| const result = await manager.reconcileDisabledExtensions(sessionManager.getCwd(), before, after); | ||
| if (!result || session.isDisposed) return; | ||
| applyMCPEnvironment(result); |
There was a problem hiding this comment.
When a session starts outside the scoped block, filterExa: true discovery extracts the Exa MCP key and this path sets Bun.env.EXA_API_KEY; the Exa server itself is filtered out, so moving into a scope that adds mcp:exa has no connection to disconnect and produces no cleanup result. The additive applyMCPEnvironment call never restores or clears that injected key, leaving native Exa authenticated from a now-blocked MCP definition even though a cold start in the same directory leaves the key unset. Track the provenance of the injected key and remove or restore it when that Exa definition becomes blocked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5efb087. The session now tracks the EXA_API_KEY it injected from an MCP definition. Every mcp: list change rediscovers, so exaApiKeys reflects only the definitions the new list allows. When the injected key is no longer among them, it is removed (only if the env still holds that value, so a user-set key is never touched). The Exa test now also moves back into the blocking scope and asserts the key is gone; it fails without the clearing step.
| const manager = mcpManager; | ||
| if (!manager) return; | ||
| const result = await manager.reconcileDisabledExtensions(sessionManager.getCwd(), before, after); | ||
| if (!result || session.isDisposed) return; |
There was a problem hiding this comment.
When a change allows a server and the session is disposed while reconciliation is still queued behind discovery or waiting in loadConfigs, disposal's single disconnectAll() can finish before the later connectServers() starts. The callback then reaches this post-result check and returns, leaving the newly connected transport or stdio child owned by an already disposed session. Cancel the reconciliation with disposal or disconnect the manager when a result arrives after isDisposed, as the deferred-startup path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5efb087. When the reconcile result arrives after session.isDisposed, the listener disconnects the servers this change allowed before returning, mirroring the deferred-startup handling. New test: move to a scope that allows a server, dispose immediately, and the server ends up disconnected. It fails without the post-disposal disconnect.
…ort state, clear blocked Exa keys, disconnect after disposal
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5efb087fa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Before the first discovery there is nothing to reconcile: it reads the list itself. | ||
| const options = this.#discoverOptions; | ||
| if (!options) return undefined; |
There was a problem hiding this comment.
When a caller constructs a manager with the public default createMCPManager(cwd) / discoverAndConnect() path, #discoverOptions remains undefined even after servers are connected. This new guard therefore treats the completed discovery as “before first discovery” and returns before tearing down IDs added to disabledExtensions, leaving a blocked server and its tools callable; retain an explicit discovered flag or store {} for the default options.
Useful? React with 👍 / 👎.
| enableProjectConfig: options.enableProjectConfig, | ||
| filterExa: options.filterExa, | ||
| filterBrowser: options.filterBrowser, | ||
| extensionRoots: options.extensionRoots, |
There was a problem hiding this comment.
When a /move changes both the scoped blocklist and the project extensions list, options.extensionRoots here is still the object materialized once at session startup. For example, start in A with an absolute configured plugin root whose mcp:x is path-blocked, then move to B where that root is not configured and x becomes allowed: rediscovery still scans A's root and connects x in B (the inverse misses a newly configured server). Recompute roots from current settings before this load.
Useful? React with 👍 / 👎.
| cfgDisabledExtensions.listen(session, async (after, before) => { | ||
| const manager = mcpManager; | ||
| if (!manager) return; | ||
| const result = await manager.reconcileDisabledExtensions(sessionManager.getCwd(), before, after); |
There was a problem hiding this comment.
When two top-level SDK sessions reuse the same options.mcpManager, only one may move into a directory that adds mcp:x to its effective list. This unconditional call mutates the shared manager, so it disconnects x and removes its tools for the other session that is still in an allowed directory (and an allow transition also retargets the shared roots/list cwd); only reconcile managers owned by the session or add per-session filtering.
Useful? React with 👍 / 👎.
| ); | ||
| // Always rediscover: besides the configs of newly allowed servers, it | ||
| // yields the Exa keys of the definitions the new list still allows. | ||
| const loaded = await this.loadConfigs(this.cwd, { |
There was a problem hiding this comment.
When two top-level sessions have separate managers, capability discovery remains bound to the newest outstanding initializeWithSettings hold. If older session A moves out of a scope blocking x while newer session B remains inside it, allowed contains x but this loadConfigs call passes no A-specific disabled list, so loadCapability filters x using B's settings and A never reconnects it. Pass after through LoadMCPConfigsOptions rather than relying on process-bound settings.
Useful? React with 👍 / 👎.
| } | ||
| // A key injected from an Exa definition the new list blocks no longer applies. | ||
| if (mcpExaApiKey !== undefined && !result.exaApiKeys.includes(mcpExaApiKey)) { | ||
| if ($env.EXA_API_KEY === mcpExaApiKey) delete Bun.env.EXA_API_KEY; |
There was a problem hiding this comment.
When session A injected key K, a later session B with an allowed Exa definition sees K already set, so applyMCPEnvironment records no ownership even though B's native searches use that process-global value. If A then enters a scope blocking Exa, this deletion removes K out from under B and subsequent searches become unauthenticated with no event to restore it; use process-level ownership/reference counting (or restore another live session's key) before deleting.
Useful? React with 👍 / 👎.
2 participants
What
disabledExtensionsnow accepts path-scoped entries, the same shapedisabledProvidersalready takes, with the ids underextensions:The
/extensionsdashboard now adds or removes one id at a time (setMember) instead of rewriting the whole list, so toggling an item no longer drops the scoped entries.Why
I have mcp servers configured for my professional and personal work and I don't want them called by mistake in the wrong context so I needed a way to add a per-directory blocklist. This change makes disabledExtensions work per directory, like disabledProviders already does. I tried it in a scoped folder, a subfolder and an unrelated folder and it works as intended
disabledServersinmcp.jsonis global, and project.omp/mcp.jsononly applies at the exact cwd (#10607), so there was no way to say "this MCP server never loads under this directory tree" from one config.disabledExtensionsalready covers MCP servers, skills, rules and context files by id, and the path-scope resolver already exists, so this reuses both. Related: #5589, #8174, #7181.Before this change, an object entry in
disabledExtensionscrashed startup on 18.10.4 (q.startsWith is not a function).Testing
bun run checkpasses inpackages/coding-agentandpackages/tui.test/settings-manager.test.ts: a scoped entry applies in a subdirectory, a dashboard-stylesetMemberadd/remove keeps the scoped object in the savedconfig.yml, andreloadForCwdto another directory drops it. Fails without the registry change, passes with it.test/mcp-path-scoped-disabled-extensions.test.tswith a real usermcp.jsonand real stdio servers:loadAllMCPConfigsexcludes the scoped server in the tree and a child dir and keeps it in another dir; a realcreateAgentSessionstarted inside the tree and moved out viareloadForCwdconnects the server, which answers a tool call with the root it gets fromroots/list(the new directory), and moving back in disconnects it and drops its tools. The transition fails with the session listener disabled, and the root assertion fails without retargeting the manager's directory. A manager-level test holds startup discovery mid-load, applies a list change that blocks one server, and asserts only the other server's tools remain (fails without joining in-flight discovery). A session started where anexaserver is scoped off, moved out, setsEXA_API_KEYfrom that server's config, and moving back in clears it (fails without applying the rediscovered keys, or without clearing the key the session injected). Round 4: a remote server whose transport already dropped (retry scheduled) is torn down when the list blocks it, and does not come back when the remote does (fails if teardown skipsdisconnectedservers); a session disposed while a move is still connecting a server leaves it disconnected (fails without the post-disposal disconnect). Each new assertion was mutation-checked./moveand cwd reload, dashboard parity, discoverydisabledExtensions, SDK MCP instructions, ACP MCP isolation), and the fullpackages/tuisuite (3711;packages/tuiis untouched since round 1).PI_CODING_AGENT_DIR, two real stdio MCP servers (test/fixtures/instructions-mcp.ts) anddisabledExtensions: [{paths: [<dir>/work], extensions: [mcp:work-hidden]}]:/mcp listinwork/andwork/sub/:work-hiddennot connected,always-onconnected./mcp listinother/: both connected./extensionsinwork/: disabling then re-enabling a skill wrote and removed onlyskill:capture; the scoped entry stayed inconfig.yml. Toggling the scope-disabledwork-hiddenon did not connect it.other/with both connected:/move work/subdisconnectedwork-hiddenand unmounted its tool (always-onstayed connected);/move otherreconnected it.work/sub/(round 2: the reconcile now lives in the session, not the TUI):/move otherconnectedwork-hidden,/move work/subdisconnected it.--mode rpc), started inother/: a/move work/subprompt stopped thework-hiddenserver process,/move otherstarted it again.work/sub/:--roots-echoserver processes 0, then 1 after/move other, then 0 after/move work/sub.Checklist
bun checkpasses