feat(modes): per-mode MCP server restrictions (allowlist)#453
Conversation
Adds optional `allowedMcpServers: string[]` to ModeConfig. When defined, only listed MCP servers' schemas are injected into the system prompt and exposed as native tools, preventing context bloat in specialized modes. - Schema: packages/types ModeConfig + schemas/roomodes.json - Prompt: filter mcpHub.getServers() by allowlist in system.ts and native-tools/mcp_server.ts; wired via core/task/build-tools.ts - UI: new McpServerRestriction.tsx editor (cachedState + 150ms debounce, per AGENTS.md SettingsView pattern), integrated into ModesView for both edit and create flows - Tests: schema, system-prompt filtering, native-tool filtering, component behavior; vitest config + toolkit mock updates Ports upstream RooCodeInc/Roo-Code#12004 (fix from simurg79/Roo-Code#1).
…tion Test 3 `<Profiler onRender>` fires whenever the Profiler boundary commits, which happens whenever its parent re-renders — even when every child inside bails out via React.memo. The `=== 0` assertion in Test 3 part (b) therefore measured the Profiler's own commit cadence rather than the child's render work. Relax to `<= 1` and document the caveat in the test's JSDoc. The real anti-flicker guarantee is verified by Test 2 (DOM-node identity preserved across an equivalent `mcpServers` heartbeat). No production code changed.
…check Restricted modes could still read MCP resources from disallowed servers because hasAnyMcpResources() inspected the full hub. Forward allowedMcpServers from build-tools into filterNativeToolsForMode so the resource-availability check only considers allowed servers. Addresses review comment from PR Zoo-Code-Org#75 (RooCodeInc/Roo-Code -> Zoo-Code-Org/Zoo-Code).
…llowlist flush The 150ms debounced flush in McpServerRestriction captured the customMode from the scheduling render and spread it on commit, so an edit to another field within the debounce window was overwritten by the stale snapshot. Track the latest customMode/onCommit in refs and merge allowedMcpServers into the freshest snapshot at flush time. Adds Test 4 covering concurrent-edit safety. Addresses review comment from PR Zoo-Code-Org#75.
… mock shape
The shared toolkit mock now forwards data-testid to the inner <input type='checkbox'>, so getByTestId resolves to the checkbox input directly. The old queries re-derived the input via .querySelector("input[type='checkbox']") on the testid element, which now returns null (the testid element IS the input). Update the 5 affected tests to target the checkbox input directly for both the restrict toggle and per-server checkboxes. Meaningful assertions (allowlist enforcement, debounced-flush-merge) are unchanged.
…er-restrictions-review fix: honor per-mode MCP allowlist for access_mcp_resource and prevent debounced flush from clobbering concurrent mode edits
simurg79
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds per-mode MCP server allowlisting across types, prompt/tool filtering, task wiring, React UI, and tests so modes can restrict which MCP servers provide capabilities and tools. ChangesMCP Server Allowlisting Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/prompts/system.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/core/prompts/tools/filter-tools-for-mode.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webview-ui/src/components/modes/ModesView.tsx (1)
393-461:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
newModeAllowedMcpServersinhandleCreateModedependency array.The callback references
newModeAllowedMcpServers(line 411) but it's not listed in the dependency array. This can cause a stale closure where the mode is created with an outdatedallowedMcpServersvalue if the user toggles the MCP restriction and immediately clicks "Create".Proposed fix
}, [ newModeName, newModeSlug, newModeDescription, newModeRoleDefinition, newModeWhenToUse, // Add whenToUse dependency newModeCustomInstructions, newModeGroups, newModeSource, + newModeAllowedMcpServers, updateCustomMode, ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/modes/ModesView.tsx` around lines 393 - 461, handleCreateMode captures newModeAllowedMcpServers but it’s missing from the hook dependency array, which can produce a stale closure; add newModeAllowedMcpServers to the dependencies of the useCallback that defines handleCreateMode (alongside newModeName, newModeSlug, newModeDescription, newModeRoleDefinition, newModeWhenToUse, newModeCustomInstructions, newModeGroups, newModeSource, updateCustomMode) so the created ModeConfig uses the latest allowedMcpServers when calling updateCustomMode and then setVisualMode/switchMode.src/core/prompts/tools/filter-tools-for-mode.ts (1)
228-237: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDerive the MCP allowlist from
modeConfiginside this function.
filterNativeToolsForModealready resolves the activemodeConfig, butaccess_mcp_resourceenforcement still depends on callers passing a separateallowedMcpServersarg. If a caller forgets that new parameter, a restricted mode is treated as unrestricted here and can retainaccess_mcp_resourcebased on disallowed servers. UsemodeConfig.allowedMcpServersas the default source of truth and let the optional parameter only override it when explicitly needed.Proposed change
export function filterNativeToolsForMode( nativeTools: OpenAI.Chat.ChatCompletionTool[], mode: string | undefined, customModes: ModeConfig[] | undefined, experiments: Record<string, boolean> | undefined, codeIndexManager?: CodeIndexManager, settings?: Record<string, any>, mcpHub?: McpHub, allowedMcpServers?: string[], ): OpenAI.Chat.ChatCompletionTool[] { // Get mode configuration and all tools for this mode const modeSlug = mode ?? defaultModeSlug let modeConfig = getModeBySlug(modeSlug, customModes) // Fallback to default mode if current mode config is not found // This ensures the agent always has functional tools even if a custom mode is deleted // or configuration becomes corrupted if (!modeConfig) { modeConfig = getModeBySlug(defaultModeSlug, customModes)! } + + const effectiveAllowedMcpServers = allowedMcpServers ?? modeConfig.allowedMcpServers ... - if (!mcpHub || !hasAnyMcpResources(mcpHub, allowedMcpServers)) { + if (!mcpHub || !hasAnyMcpResources(mcpHub, effectiveAllowedMcpServers)) { allowedToolNames.delete("access_mcp_resource") }Also applies to: 239-247, 308-312
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/prompts/tools/filter-tools-for-mode.ts` around lines 228 - 237, In filterNativeToolsForMode, use the resolved modeConfig.allowedMcpServers as the default allowlist for enforcing access_mcp_resource and only use the incoming allowedMcpServers parameter when it is explicitly provided; specifically, after resolving modeConfig (the existing variable named modeConfig), set a local effectiveAllowedMcpServers = allowedMcpServers ?? modeConfig.allowedMcpServers and use that when checking/enforcing access_mcp_resource so callers that omit the param don’t bypass mode restrictions; apply the same pattern to the other analogous MCP-enforcement blocks in this file where access_mcp_resource gating occurs (the nearby occurrences that currently read allowedMcpServers directly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/prompts/system.ts`:
- Around line 69-77: The capability text is built from the unfiltered mcpHub
while tool exposure uses the filtered servers; update the call to
getCapabilitiesSection so it receives the same filtered set (or the
allowedMcpServers allowlist) used to compute hasMcpServers: compute the filtered
servers from mcpHub using allowedMcpServers (as done when setting hasMcpServers)
and pass that filtered servers array (or allowedMcpServers) into
getCapabilitiesSection instead of the raw mcpHub so shouldIncludeMcp and
getCapabilitiesSection render only allowed MCP servers.
---
Outside diff comments:
In `@src/core/prompts/tools/filter-tools-for-mode.ts`:
- Around line 228-237: In filterNativeToolsForMode, use the resolved
modeConfig.allowedMcpServers as the default allowlist for enforcing
access_mcp_resource and only use the incoming allowedMcpServers parameter when
it is explicitly provided; specifically, after resolving modeConfig (the
existing variable named modeConfig), set a local effectiveAllowedMcpServers =
allowedMcpServers ?? modeConfig.allowedMcpServers and use that when
checking/enforcing access_mcp_resource so callers that omit the param don’t
bypass mode restrictions; apply the same pattern to the other analogous
MCP-enforcement blocks in this file where access_mcp_resource gating occurs (the
nearby occurrences that currently read allowedMcpServers directly).
In `@webview-ui/src/components/modes/ModesView.tsx`:
- Around line 393-461: handleCreateMode captures newModeAllowedMcpServers but
it’s missing from the hook dependency array, which can produce a stale closure;
add newModeAllowedMcpServers to the dependencies of the useCallback that defines
handleCreateMode (alongside newModeName, newModeSlug, newModeDescription,
newModeRoleDefinition, newModeWhenToUse, newModeCustomInstructions,
newModeGroups, newModeSource, updateCustomMode) so the created ModeConfig uses
the latest allowedMcpServers when calling updateCustomMode and then
setVisualMode/switchMode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21da37d4-61c2-4648-bbd9-3b28e2f66d7a
📒 Files selected for processing (15)
packages/types/src/__tests__/mode-allowedMcpServers.spec.tspackages/types/src/mode.tsschemas/roomodes.jsonsrc/core/prompts/__tests__/system-prompt.spec.tssrc/core/prompts/system.tssrc/core/prompts/tools/__tests__/filter-tools-for-mode.spec.tssrc/core/prompts/tools/filter-tools-for-mode.tssrc/core/prompts/tools/native-tools/__tests__/mcp_server.spec.tssrc/core/prompts/tools/native-tools/mcp_server.tssrc/core/task/build-tools.tswebview-ui/src/__mocks__/@vscode/webview-ui-toolkit/react.tsxwebview-ui/src/components/modes/McpServerRestriction.tsxwebview-ui/src/components/modes/ModesView.tsxwebview-ui/src/components/modes/__tests__/McpServerRestriction.spec.tsxwebview-ui/vitest.config.ts
- filter-tools-for-mode: default access_mcp_resource gating to modeConfig.allowedMcpServers when the parameter is omitted (defense in depth), so a restricted mode can never retain the tool via a caller that forgets to thread the allowlist. Adds tests covering param-omitted fallback and explicit-param precedence. - ModesView: add newModeAllowedMcpServers (and switchMode/resetFormState) to handleCreateMode useCallback deps and drop the now-unnecessary react-hooks/exhaustive-deps disable, fixing a potential stale-closure on mode creation. - system.ts: document that the capabilities MCP line is already gated by the filtered allowlist via shouldIncludeMcp. Addresses review feedback on PR Zoo-Code-Org#453.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // When the mode restricts MCP servers via allowedMcpServers, only resources from allowed | ||
| // servers count — otherwise a restricted mode could still read resources from disallowed servers. | ||
| // Fall back to the mode config's own allowlist when the caller omits the parameter, so the | ||
| // restriction is enforced regardless of call site (defense in depth). |
There was a problem hiding this comment.
The "defense in depth" framing here is great, but right now the defense only has one layer — the listing/filtering layer. validateToolUse.ts:183-185 auto-allows any mcp_* tool if the mcp group is present, with no server-name check. And the mcp_tool_use path in presentAssistantMessage.ts bypasses validateToolUse entirely.
Had we considered adding an invocation-time guard as well? The existing fileRegex restriction already does this — it checks toolParams.path at execution time in isToolAllowedForMode:207. A similar check on toolParams.server_name against allowedMcpServers would close the gap. Could be a follow-up issue if it's too much for this PR.
| let hasMcpServers = false | ||
| if (mcpHub) { | ||
| const servers = allowedMcpServers | ||
| ? mcpHub.getServers().filter((s) => new Set(allowedMcpServers).has(s.name)) |
There was a problem hiding this comment.
nit: new Set(allowedMcpServers) is reconstructed for every server inside the .filter(). The two sibling call sites in this PR (filter-tools-for-mode.ts:351 and mcp_server.ts:23) both hoist the Set outside the lambda.
| ? mcpHub.getServers().filter((s) => new Set(allowedMcpServers).has(s.name)) | |
| const allowSet = new Set(allowedMcpServers) | |
| const servers = allowedMcpServers | |
| ? mcpHub.getServers().filter((s) => allowSet.has(s.name)) | |
| : mcpHub.getServers() |
| No MCP servers connected | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
The edit-panel McpServerRestriction component shows a warning for servers in the allowlist that aren't currently connected (McpServerRestriction.tsx:172-182). This inline copy is missing that — if a user creates a mode with a server name that later disconnects, there's no visual feedback here. Worth extracting a shared presentational component, or at least adding the warning?
| useEffect(() => { | ||
| if (lastSlugRef.current !== customMode.slug) { | ||
| lastSlugRef.current = customMode.slug | ||
| setCachedAllowedMcpServers(customMode.allowedMcpServers) | ||
| lastFlushedRef.current = customMode.allowedMcpServers | ||
| isInitialMountRef.current = true | ||
| } | ||
| }, [customMode.slug, customMode.allowedMcpServers]) |
There was a problem hiding this comment.
This reseed path is the most operationally critical reconciliation logic in the component — if it breaks, mode A's allowlist bleeds into mode B. I don't see a test covering the slug-change reseed in the spec file. The other paths (optimistic toggle, concurrent edit, heartbeat stability) are well covered — can you add a test for it.
There was a problem hiding this comment.
lets not update the global config, can you just mock @vscode/webview-ui-toolkt/react in vitest.setup.ts?
2 participants
Summary
This PR adds per-mode MCP server restrictions (allowlist), allowing each mode to declare an allowlist of MCP servers it is permitted to access. When a mode defines an allowlist, only the listed MCP servers are exposed to that mode, and tooling that interacts with MCP servers (including the availability check for
access_mcp_resource) is filtered accordingly.Supersedes #75
This PR supersedes #75, which was closed inadvertently (the source branch was force-pushed/rebased, which automatically closed the original PR). No behavior has been dropped — this PR carries the full feature plus the follow-up fixes requested in review.
Addresses reviewer feedback from #75
It also folds in the two fixes that address reviewer @edelauna's discussion on #75:
8eeaa2931—fix(mcp): enforce mode allowlist in access_mcp_resource availability check. Previously the allowlist was applied to tool listing but theaccess_mcp_resourceavailability check did not re-check the mode allowlist, so a restricted mode could still resolve a disallowed server. This fix enforces the allowlist consistently at the availability-check layer.948708ea4—fix(modes): avoid clobbering concurrent mode edits in debounced MCP allowlist flush. The debounced flush that persists the MCP allowlist could overwrite concurrent edits to other fields of the same mode. This fix merges against the latest mode state on flush so concurrent edits are no longer clobbered.Validation
I have been dogfooding this change on my fork build over several days.
Notes
simurg79/Zoo-Code.Summary by CodeRabbit
New Features
Behavior Changes
Tests
Chores