mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: restore MCP tool auto-approval behavior when alwaysAllowMcp is enabled
- Fixed regression introduced in #9157 where MCP tools required individual alwaysAllow property - When alwaysAllowMcp is true, all MCP tools are now auto-approved by default - Tools can still opt-out by explicitly setting alwaysAllow=false - Added comprehensive tests for MCP auto-approval scenarios Fixes #9204
This commit is contained in:
parent
62d8cc0d66
commit
b8f00cc6bb
2 changed files with 301 additions and 3 deletions
288
src/core/auto-approval/__tests__/checkAutoApproval.spec.ts
Normal file
288
src/core/auto-approval/__tests__/checkAutoApproval.spec.ts
Normal file
|
|
@ -0,0 +1,288 @@
|
|||
import { describe, it, expect } from "vitest"
|
||||
import { checkAutoApproval } from "../index"
|
||||
import type { ExtensionState } from "../../../shared/ExtensionMessage"
|
||||
import type { McpServer } from "../../../shared/mcp"
|
||||
|
||||
describe("checkAutoApproval", () => {
|
||||
describe("MCP tool auto-approval", () => {
|
||||
it("should auto-approve MCP tools when alwaysAllowMcp is true and tool has no explicit alwaysAllow", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [
|
||||
{
|
||||
name: "test-server",
|
||||
config: "test",
|
||||
status: "connected",
|
||||
tools: [
|
||||
{
|
||||
name: "test-tool",
|
||||
description: "Test tool",
|
||||
// No alwaysAllow property - should default to auto-approve
|
||||
},
|
||||
],
|
||||
} as McpServer,
|
||||
],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "test-server",
|
||||
toolName: "test-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("approve")
|
||||
})
|
||||
|
||||
it("should auto-approve MCP tools when alwaysAllowMcp is true and tool has alwaysAllow=true", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [
|
||||
{
|
||||
name: "test-server",
|
||||
config: "test",
|
||||
status: "connected",
|
||||
tools: [
|
||||
{
|
||||
name: "test-tool",
|
||||
description: "Test tool",
|
||||
alwaysAllow: true,
|
||||
},
|
||||
],
|
||||
} as McpServer,
|
||||
],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "test-server",
|
||||
toolName: "test-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("approve")
|
||||
})
|
||||
|
||||
it("should NOT auto-approve MCP tools when tool explicitly has alwaysAllow=false", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [
|
||||
{
|
||||
name: "test-server",
|
||||
config: "test",
|
||||
status: "connected",
|
||||
tools: [
|
||||
{
|
||||
name: "test-tool",
|
||||
description: "Test tool",
|
||||
alwaysAllow: false, // Explicitly disabled
|
||||
},
|
||||
],
|
||||
} as McpServer,
|
||||
],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "test-server",
|
||||
toolName: "test-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
|
||||
it("should NOT auto-approve MCP tools when alwaysAllowMcp is false", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: false, // Global MCP auto-approval disabled
|
||||
mcpServers: [
|
||||
{
|
||||
name: "test-server",
|
||||
config: "test",
|
||||
status: "connected",
|
||||
tools: [
|
||||
{
|
||||
name: "test-tool",
|
||||
description: "Test tool",
|
||||
},
|
||||
],
|
||||
} as McpServer,
|
||||
],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "test-server",
|
||||
toolName: "test-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
|
||||
it("should NOT auto-approve when autoApprovalEnabled is false", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: false, // Auto-approval completely disabled
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "test-server",
|
||||
toolName: "test-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
|
||||
it("should auto-approve MCP resources when alwaysAllowMcp is true", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "access_mcp_resource",
|
||||
serverName: "test-server",
|
||||
resourceUri: "test://resource",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("approve")
|
||||
})
|
||||
|
||||
it("should handle missing or unknown servers gracefully", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [
|
||||
{
|
||||
name: "different-server",
|
||||
config: "test",
|
||||
status: "connected",
|
||||
tools: [],
|
||||
} as McpServer,
|
||||
],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "unknown-server",
|
||||
toolName: "test-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
// Should still auto-approve since alwaysAllowMcp is true and tool doesn't exist to have alwaysAllow=false
|
||||
expect(result.decision).toBe("approve")
|
||||
})
|
||||
|
||||
it("should handle missing tools gracefully", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [
|
||||
{
|
||||
name: "test-server",
|
||||
config: "test",
|
||||
status: "connected",
|
||||
tools: [
|
||||
{
|
||||
name: "different-tool",
|
||||
description: "Different tool",
|
||||
},
|
||||
],
|
||||
} as McpServer,
|
||||
],
|
||||
}
|
||||
|
||||
const mcpServerUse = JSON.stringify({
|
||||
type: "use_mcp_tool",
|
||||
serverName: "test-server",
|
||||
toolName: "unknown-tool",
|
||||
})
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: mcpServerUse,
|
||||
})
|
||||
|
||||
// Should still auto-approve since alwaysAllowMcp is true and tool doesn't exist to have alwaysAllow=false
|
||||
expect(result.decision).toBe("approve")
|
||||
})
|
||||
|
||||
it("should handle invalid JSON gracefully", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [],
|
||||
}
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: "invalid json",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
|
||||
it("should handle missing text gracefully", async () => {
|
||||
const state: Partial<ExtensionState> = {
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowMcp: true,
|
||||
mcpServers: [],
|
||||
}
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: state as any,
|
||||
ask: "use_mcp_server",
|
||||
text: undefined,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -2,6 +2,7 @@ import { type ClineAsk, type McpServerUse, type FollowUpData, isNonBlockingAsk }
|
|||
|
||||
import type { ClineSayTool, ExtensionState } from "../../shared/ExtensionMessage"
|
||||
import { ClineAskResponse } from "../../shared/WebviewMessage"
|
||||
import type { McpServer, McpTool } from "../../shared/mcp"
|
||||
|
||||
import { isWriteToolAction, isReadOnlyToolAction } from "./tools"
|
||||
import { isMcpToolAlwaysAllowed } from "./mcp"
|
||||
|
|
@ -99,9 +100,18 @@ export async function checkAutoApproval({
|
|||
const mcpServerUse = JSON.parse(text) as McpServerUse
|
||||
|
||||
if (mcpServerUse.type === "use_mcp_tool") {
|
||||
return state.alwaysAllowMcp === true && isMcpToolAlwaysAllowed(mcpServerUse, state.mcpServers)
|
||||
? { decision: "approve" }
|
||||
: { decision: "ask" }
|
||||
// Check if global MCP auto-approval is enabled
|
||||
if (state.alwaysAllowMcp === true) {
|
||||
// If the tool has explicit alwaysAllow set to false, respect that
|
||||
const server = state.mcpServers?.find((s) => s.name === mcpServerUse.serverName)
|
||||
const tool = server?.tools?.find((t) => t.name === mcpServerUse.toolName)
|
||||
if (tool?.alwaysAllow === false) {
|
||||
return { decision: "ask" }
|
||||
}
|
||||
// Otherwise auto-approve (including when alwaysAllow is true or undefined)
|
||||
return { decision: "approve" }
|
||||
}
|
||||
return { decision: "ask" }
|
||||
} else if (mcpServerUse.type === "access_mcp_resource") {
|
||||
return state.alwaysAllowMcp === true ? { decision: "approve" } : { decision: "ask" }
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue