Skip to content
56 changes: 56 additions & 0 deletions packages/types/src/__tests__/mode-allowedMcpServers.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { modeConfigSchema } from "../mode.js"

describe("modeConfigSchema allowedMcpServers", () => {
const baseModeConfig = {
slug: "test-mode",
name: "Test Mode",
roleDefinition: "A test mode",
groups: ["read" as const],
}

it("should accept valid allowedMcpServers array of strings", () => {
const result = modeConfigSchema.safeParse({
...baseModeConfig,
allowedMcpServers: ["server1", "server2"],
})
expect(result.success).toBe(true)
if (result.success) {
expect(result.data.allowedMcpServers).toEqual(["server1", "server2"])
}
})

it("should accept missing/undefined allowedMcpServers", () => {
const result = modeConfigSchema.safeParse(baseModeConfig)
expect(result.success).toBe(true)
if (result.success) {
expect(result.data.allowedMcpServers).toBeUndefined()
}
})

it("should accept empty allowedMcpServers array", () => {
const result = modeConfigSchema.safeParse({
...baseModeConfig,
allowedMcpServers: [],
})
expect(result.success).toBe(true)
if (result.success) {
expect(result.data.allowedMcpServers).toEqual([])
}
})

it("should reject non-string array items", () => {
const result = modeConfigSchema.safeParse({
...baseModeConfig,
allowedMcpServers: [123, 456],
})
expect(result.success).toBe(false)
})

it("should reject non-array value", () => {
const result = modeConfigSchema.safeParse({
...baseModeConfig,
allowedMcpServers: "server1",
})
expect(result.success).toBe(false)
})
})
1 change: 1 addition & 0 deletions packages/types/src/mode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export const modeConfigSchema = z.object({
customInstructions: z.string().optional(),
groups: groupEntryArraySchema,
source: z.enum(["global", "project"]).optional(),
allowedMcpServers: z.array(z.string()).optional(),
})

export type ModeConfig = z.infer<typeof modeConfigSchema>
Expand Down
7 changes: 7 additions & 0 deletions schemas/roomodes.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@
"required": ["relativePath"],
"additionalProperties": false
}
},
"allowedMcpServers": {
"type": "array",
"items": {
"type": "string"
},
"description": "Optional list of MCP server names to include. When omitted, all servers are available. When set, only the listed servers are injected."
}
},
"required": ["slug", "name", "roleDefinition", "groups"],
Expand Down
64 changes: 64 additions & 0 deletions src/core/prompts/__tests__/system-prompt.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,70 @@ describe("SYSTEM_PROMPT", () => {
expect(prompt).toContain("OBJECTIVE")
})

describe("allowedMcpServers filtering in system prompt", () => {
it("should exclude MCP capability text when allowedMcpServers is empty array", async () => {
mockMcpHub = createMockMcpHub(true)

const customModes: ModeConfig[] = [
{
slug: "filtered-mode",
name: "Filtered Mode",
roleDefinition: "A filtered mode",
groups: ["read", "mcp"] as const,
allowedMcpServers: [],
},
]

const prompt = await SYSTEM_PROMPT(
mockContext,
"/test/path",
false,
mockMcpHub, // mcpHub with servers
undefined, // diffStrategy
"filtered-mode", // mode
undefined, // customModePrompts
customModes, // customModes
undefined, // globalCustomInstructions
experiments,
undefined, // language
undefined, // rooIgnoreInstructions
)

expect(prompt).not.toContain("MCP servers")
})

it("should include MCP capability text when allowedMcpServers matches connected servers", async () => {
mockMcpHub = createMockMcpHub(true) // has "test-server"

const customModes: ModeConfig[] = [
{
slug: "mcp-mode",
name: "MCP Mode",
roleDefinition: "A mode with MCP",
groups: ["read", "mcp"] as const,
allowedMcpServers: ["test-server"],
},
]

const prompt = await SYSTEM_PROMPT(
mockContext,
"/test/path",
false,
mockMcpHub,
undefined,
"mcp-mode",
undefined,
customModes,
undefined,
experiments,
undefined,
undefined,
)

expect(prompt).toContain("MCP servers")
})
})

afterAll(() => {
vi.restoreAllMocks()
})
Expand Down
20 changes: 18 additions & 2 deletions src/core/prompts/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,15 @@ async function generatePrompt(

// Check if MCP functionality should be included
const hasMcpGroup = modeConfig.groups.some((groupEntry) => getGroupName(groupEntry) === "mcp")
const hasMcpServers = mcpHub && mcpHub.getServers().length > 0
const allowedMcpServers = modeConfig.allowedMcpServers

let hasMcpServers = false
if (mcpHub) {
const servers = allowedMcpServers
? mcpHub.getServers().filter((s) => new Set(allowedMcpServers).has(s.name))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
? 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()

: mcpHub.getServers()
hasMcpServers = servers.length > 0
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const shouldIncludeMcp = hasMcpGroup && hasMcpServers

const codeIndexManager = CodeIndexManager.getInstance(context, cwd)
Expand All @@ -90,7 +98,15 @@ ${getSharedToolUseSection()}${toolsCatalog}

${getToolUseGuidelinesSection()}

${getCapabilitiesSection(cwd, shouldIncludeMcp ? mcpHub : undefined)}
${
// `shouldIncludeMcp` already accounts for the mode's allowedMcpServers allowlist
// (see hasMcpServers above), so a mode that restricts MCP servers down to none will
// pass `undefined` here and omit the MCP capabilities line entirely. The capabilities
// section only emits a generic MCP availability statement (it does not enumerate
// individual servers), so forwarding the hub when at least one allowed server exists
// is consistent with the per-mode tool exposure.
getCapabilitiesSection(cwd, shouldIncludeMcp ? mcpHub : undefined)
}

${modesSection}
${skillsSection ? `\n${skillsSection}` : ""}
Expand Down
138 changes: 138 additions & 0 deletions src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,141 @@ describe("filterNativeToolsForMode - disabledTools", () => {
expect(resultNames).not.toContain("edit")
})
})

describe("filterNativeToolsForMode - access_mcp_resource allowlist", () => {
const nativeTools: OpenAI.Chat.ChatCompletionTool[] = [makeTool("read_file"), makeTool("access_mcp_resource")]

// Minimal McpHub stub exposing only getServers(), which is all the resource
// availability check uses.
function makeMcpHub(servers: Array<{ name: string; resources?: unknown[] }>): any {
return {
getServers: () => servers,
}
}

it("keeps access_mcp_resource when an allowed server has resources", () => {
const mcpHub = makeMcpHub([{ name: "allowed-server", resources: [{ uri: "res://x" }] }])

const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub, [
"allowed-server",
])

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).toContain("access_mcp_resource")
})

it("removes access_mcp_resource when only a disallowed server has resources", () => {
// The server with resources is NOT in the allowlist, so the restricted
// mode must not retain access_mcp_resource.
const mcpHub = makeMcpHub([
{ name: "allowed-server", resources: [] },
{ name: "blocked-server", resources: [{ uri: "res://secret" }] },
])

const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub, [
"allowed-server",
])

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).not.toContain("access_mcp_resource")
expect(resultNames).toContain("read_file")
})

it("considers all servers when no allowlist is provided (unrestricted mode)", () => {
const mcpHub = makeMcpHub([{ name: "any-server", resources: [{ uri: "res://y" }] }])

const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub)

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).toContain("access_mcp_resource")
})

it("removes access_mcp_resource when the allowlist is empty", () => {
const mcpHub = makeMcpHub([{ name: "some-server", resources: [{ uri: "res://z" }] }])

const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub, [])

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).not.toContain("access_mcp_resource")
})

// Defense in depth: even if a caller forgets to thread `allowedMcpServers`, the
// function must fall back to the mode config's own allowlist so a restricted mode
// can never retain access_mcp_resource based on resources from disallowed servers.
describe("falls back to modeConfig.allowedMcpServers when the parameter is omitted", () => {
const restrictedMode = {
slug: "restricted",
name: "Restricted",
roleDefinition: "restricted role",
groups: ["read", "mcp"],
allowedMcpServers: ["allowed-server"],
} as any

it("removes access_mcp_resource when only a disallowed server has resources (param omitted)", () => {
const mcpHub = makeMcpHub([
{ name: "allowed-server", resources: [] },
{ name: "blocked-server", resources: [{ uri: "res://secret" }] },
])

// Note: the 8th argument (allowedMcpServers) is intentionally omitted to
// simulate a caller that does not thread the allowlist through.
const result = filterNativeToolsForMode(
nativeTools,
"restricted",
[restrictedMode],
undefined,
undefined,
{},
mcpHub,
)

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).not.toContain("access_mcp_resource")
expect(resultNames).toContain("read_file")
})

it("keeps access_mcp_resource when an allowed server has resources (param omitted)", () => {
const mcpHub = makeMcpHub([
{ name: "allowed-server", resources: [{ uri: "res://x" }] },
{ name: "blocked-server", resources: [{ uri: "res://secret" }] },
])

const result = filterNativeToolsForMode(
nativeTools,
"restricted",
[restrictedMode],
undefined,
undefined,
{},
mcpHub,
)

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).toContain("access_mcp_resource")
})

it("prefers the explicit parameter over the mode config allowlist when both are provided", () => {
// The mode config allows "allowed-server", but the explicit parameter
// allows only "blocked-server" (which has the resources), so the explicit
// parameter must win and access_mcp_resource is retained.
const mcpHub = makeMcpHub([
{ name: "allowed-server", resources: [] },
{ name: "blocked-server", resources: [{ uri: "res://secret" }] },
])

const result = filterNativeToolsForMode(
nativeTools,
"restricted",
[restrictedMode],
undefined,
undefined,
{},
mcpHub,
["blocked-server"],
)

const resultNames = result.map((t) => (t as any).function.name)
expect(resultNames).toContain("access_mcp_resource")
})
})
})
27 changes: 22 additions & 5 deletions src/core/prompts/tools/filter-tools-for-mode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ export function applyModelToolCustomization(
* @param codeIndexManager - Code index manager for codebase_search feature check
* @param settings - Additional settings for tool filtering (includes modelInfo for model-specific customization)
* @param mcpHub - MCP hub for checking available resources
* @param allowedMcpServers - Optional allowlist of MCP server names for the current mode. When
* provided, the resource-availability check only considers servers in this list, so a mode that
* restricts MCP servers cannot retain `access_mcp_resource` based on resources from disallowed servers.
* @returns Filtered array of tools allowed for the mode
*/
export function filterNativeToolsForMode(
Expand All @@ -230,6 +233,7 @@ export function filterNativeToolsForMode(
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
Expand Down Expand Up @@ -301,8 +305,13 @@ export function filterNativeToolsForMode(
}
}

// Conditionally exclude access_mcp_resource if MCP is not enabled or there are no resources
if (!mcpHub || !hasAnyMcpResources(mcpHub)) {
// Conditionally exclude access_mcp_resource if MCP is not enabled or there are no resources.
// 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).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const effectiveAllowedMcpServers = allowedMcpServers ?? modeConfig.allowedMcpServers
if (!mcpHub || !hasAnyMcpResources(mcpHub, effectiveAllowedMcpServers)) {
allowedToolNames.delete("access_mcp_resource")
}

Expand Down Expand Up @@ -330,10 +339,18 @@ export function filterNativeToolsForMode(
}

/**
* Helper function to check if any MCP server has resources available
* Helper function to check if any MCP server has resources available.
*
* When `allowedServers` is provided, only servers whose name is in the allowlist are considered.
* This keeps the `access_mcp_resource` availability check consistent with the mode's MCP server
* allowlist so a restricted mode cannot retain the tool based on resources from disallowed servers.
*/
function hasAnyMcpResources(mcpHub: McpHub): boolean {
const servers = mcpHub.getServers()
function hasAnyMcpResources(mcpHub: McpHub, allowedServers?: string[]): boolean {
let servers = mcpHub.getServers()
if (allowedServers) {
const allowSet = new Set(allowedServers)
servers = servers.filter((server) => allowSet.has(server.name))
}
return servers.some((server) => server.resources && server.resources.length > 0)
}

Expand Down
Loading
Loading