Skip to content

Commit b8f00cc

Browse files
roomoteroomote
committed
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
1 parent 62d8cc0 commit b8f00cc

File tree

2 files changed

+301
-3
lines changed

2 files changed

+301
-3
lines changed

‎src/core/auto-approval/__tests__/checkAutoApproval.spec.ts‎

Lines changed: 288 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,288 @@
1+
import { describe, it, expect } from "vitest"
2+
import { checkAutoApproval } from "../index"
3+
import type { ExtensionState } from "../../../shared/ExtensionMessage"
4+
import type { McpServer } from "../../../shared/mcp"
5+
6+
describe("checkAutoApproval", () => {
7+
describe("MCP tool auto-approval", () => {
8+
it("should auto-approve MCP tools when alwaysAllowMcp is true and tool has no explicit alwaysAllow", async () => {
9+
const state: Partial<ExtensionState> = {
10+
autoApprovalEnabled: true,
11+
alwaysAllowMcp: true,
12+
mcpServers: [
13+
{
14+
name: "test-server",
15+
config: "test",
16+
status: "connected",
17+
tools: [
18+
{
19+
name: "test-tool",
20+
description: "Test tool",
21+
// No alwaysAllow property - should default to auto-approve
22+
},
23+
],
24+
} as McpServer,
25+
],
26+
}
27+
28+
const mcpServerUse = JSON.stringify({
29+
type: "use_mcp_tool",
30+
serverName: "test-server",
31+
toolName: "test-tool",
32+
})
33+
34+
const result = await checkAutoApproval({
35+
state: state as any,
36+
ask: "use_mcp_server",
37+
text: mcpServerUse,
38+
})
39+
40+
expect(result.decision).toBe("approve")
41+
})
42+
43+
it("should auto-approve MCP tools when alwaysAllowMcp is true and tool has alwaysAllow=true", async () => {
44+
const state: Partial<ExtensionState> = {
45+
autoApprovalEnabled: true,
46+
alwaysAllowMcp: true,
47+
mcpServers: [
48+
{
49+
name: "test-server",
50+
config: "test",
51+
status: "connected",
52+
tools: [
53+
{
54+
name: "test-tool",
55+
description: "Test tool",
56+
alwaysAllow: true,
57+
},
58+
],
59+
} as McpServer,
60+
],
61+
}
62+
63+
const mcpServerUse = JSON.stringify({
64+
type: "use_mcp_tool",
65+
serverName: "test-server",
66+
toolName: "test-tool",
67+
})
68+
69+
const result = await checkAutoApproval({
70+
state: state as any,
71+
ask: "use_mcp_server",
72+
text: mcpServerUse,
73+
})
74+
75+
expect(result.decision).toBe("approve")
76+
})
77+
78+
it("should NOT auto-approve MCP tools when tool explicitly has alwaysAllow=false", async () => {
79+
const state: Partial<ExtensionState> = {
80+
autoApprovalEnabled: true,
81+
alwaysAllowMcp: true,
82+
mcpServers: [
83+
{
84+
name: "test-server",
85+
config: "test",
86+
status: "connected",
87+
tools: [
88+
{
89+
name: "test-tool",
90+
description: "Test tool",
91+
alwaysAllow: false, // Explicitly disabled
92+
},
93+
],
94+
} as McpServer,
95+
],
96+
}
97+
98+
const mcpServerUse = JSON.stringify({
99+
type: "use_mcp_tool",
100+
serverName: "test-server",
101+
toolName: "test-tool",
102+
})
103+
104+
const result = await checkAutoApproval({
105+
state: state as any,
106+
ask: "use_mcp_server",
107+
text: mcpServerUse,
108+
})
109+
110+
expect(result.decision).toBe("ask")
111+
})
112+
113+
it("should NOT auto-approve MCP tools when alwaysAllowMcp is false", async () => {
114+
const state: Partial<ExtensionState> = {
115+
autoApprovalEnabled: true,
116+
alwaysAllowMcp: false, // Global MCP auto-approval disabled
117+
mcpServers: [
118+
{
119+
name: "test-server",
120+
config: "test",
121+
status: "connected",
122+
tools: [
123+
{
124+
name: "test-tool",
125+
description: "Test tool",
126+
},
127+
],
128+
} as McpServer,
129+
],
130+
}
131+
132+
const mcpServerUse = JSON.stringify({
133+
type: "use_mcp_tool",
134+
serverName: "test-server",
135+
toolName: "test-tool",
136+
})
137+
138+
const result = await checkAutoApproval({
139+
state: state as any,
140+
ask: "use_mcp_server",
141+
text: mcpServerUse,
142+
})
143+
144+
expect(result.decision).toBe("ask")
145+
})
146+
147+
it("should NOT auto-approve when autoApprovalEnabled is false", async () => {
148+
const state: Partial<ExtensionState> = {
149+
autoApprovalEnabled: false, // Auto-approval completely disabled
150+
alwaysAllowMcp: true,
151+
mcpServers: [],
152+
}
153+
154+
const mcpServerUse = JSON.stringify({
155+
type: "use_mcp_tool",
156+
serverName: "test-server",
157+
toolName: "test-tool",
158+
})
159+
160+
const result = await checkAutoApproval({
161+
state: state as any,
162+
ask: "use_mcp_server",
163+
text: mcpServerUse,
164+
})
165+
166+
expect(result.decision).toBe("ask")
167+
})
168+
169+
it("should auto-approve MCP resources when alwaysAllowMcp is true", async () => {
170+
const state: Partial<ExtensionState> = {
171+
autoApprovalEnabled: true,
172+
alwaysAllowMcp: true,
173+
mcpServers: [],
174+
}
175+
176+
const mcpServerUse = JSON.stringify({
177+
type: "access_mcp_resource",
178+
serverName: "test-server",
179+
resourceUri: "test://resource",
180+
})
181+
182+
const result = await checkAutoApproval({
183+
state: state as any,
184+
ask: "use_mcp_server",
185+
text: mcpServerUse,
186+
})
187+
188+
expect(result.decision).toBe("approve")
189+
})
190+
191+
it("should handle missing or unknown servers gracefully", async () => {
192+
const state: Partial<ExtensionState> = {
193+
autoApprovalEnabled: true,
194+
alwaysAllowMcp: true,
195+
mcpServers: [
196+
{
197+
name: "different-server",
198+
config: "test",
199+
status: "connected",
200+
tools: [],
201+
} as McpServer,
202+
],
203+
}
204+
205+
const mcpServerUse = JSON.stringify({
206+
type: "use_mcp_tool",
207+
serverName: "unknown-server",
208+
toolName: "test-tool",
209+
})
210+
211+
const result = await checkAutoApproval({
212+
state: state as any,
213+
ask: "use_mcp_server",
214+
text: mcpServerUse,
215+
})
216+
217+
// Should still auto-approve since alwaysAllowMcp is true and tool doesn't exist to have alwaysAllow=false
218+
expect(result.decision).toBe("approve")
219+
})
220+
221+
it("should handle missing tools gracefully", async () => {
222+
const state: Partial<ExtensionState> = {
223+
autoApprovalEnabled: true,
224+
alwaysAllowMcp: true,
225+
mcpServers: [
226+
{
227+
name: "test-server",
228+
config: "test",
229+
status: "connected",
230+
tools: [
231+
{
232+
name: "different-tool",
233+
description: "Different tool",
234+
},
235+
],
236+
} as McpServer,
237+
],
238+
}
239+
240+
const mcpServerUse = JSON.stringify({
241+
type: "use_mcp_tool",
242+
serverName: "test-server",
243+
toolName: "unknown-tool",
244+
})
245+
246+
const result = await checkAutoApproval({
247+
state: state as any,
248+
ask: "use_mcp_server",
249+
text: mcpServerUse,
250+
})
251+
252+
// Should still auto-approve since alwaysAllowMcp is true and tool doesn't exist to have alwaysAllow=false
253+
expect(result.decision).toBe("approve")
254+
})
255+
256+
it("should handle invalid JSON gracefully", async () => {
257+
const state: Partial<ExtensionState> = {
258+
autoApprovalEnabled: true,
259+
alwaysAllowMcp: true,
260+
mcpServers: [],
261+
}
262+
263+
const result = await checkAutoApproval({
264+
state: state as any,
265+
ask: "use_mcp_server",
266+
text: "invalid json",
267+
})
268+
269+
expect(result.decision).toBe("ask")
270+
})
271+
272+
it("should handle missing text gracefully", async () => {
273+
const state: Partial<ExtensionState> = {
274+
autoApprovalEnabled: true,
275+
alwaysAllowMcp: true,
276+
mcpServers: [],
277+
}
278+
279+
const result = await checkAutoApproval({
280+
state: state as any,
281+
ask: "use_mcp_server",
282+
text: undefined,
283+
})
284+
285+
expect(result.decision).toBe("ask")
286+
})
287+
})
288+
})

‎src/core/auto-approval/index.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { type ClineAsk, type McpServerUse, type FollowUpData, isNonBlockingAsk }
22

33
import type { ClineSayTool, ExtensionState } from "../../shared/ExtensionMessage"
44
import { ClineAskResponse } from "../../shared/WebviewMessage"
5+
import type { McpServer, McpTool } from "../../shared/mcp"
56

67
import { isWriteToolAction, isReadOnlyToolAction } from "./tools"
78
import { isMcpToolAlwaysAllowed } from "./mcp"
@@ -99,9 +100,18 @@ export async function checkAutoApproval({
99100
const mcpServerUse = JSON.parse(text) as McpServerUse
100101

101102
if (mcpServerUse.type === "use_mcp_tool") {
102-
return state.alwaysAllowMcp === true && isMcpToolAlwaysAllowed(mcpServerUse, state.mcpServers)
103-
? { decision: "approve" }
104-
: { decision: "ask" }
103+
// Check if global MCP auto-approval is enabled
104+
if (state.alwaysAllowMcp === true) {
105+
// If the tool has explicit alwaysAllow set to false, respect that
106+
const server = state.mcpServers?.find((s) => s.name === mcpServerUse.serverName)
107+
const tool = server?.tools?.find((t) => t.name === mcpServerUse.toolName)
108+
if (tool?.alwaysAllow === false) {
109+
return { decision: "ask" }
110+
}
111+
// Otherwise auto-approve (including when alwaysAllow is true or undefined)
112+
return { decision: "approve" }
113+
}
114+
return { decision: "ask" }
105115
} else if (mcpServerUse.type === "access_mcp_resource") {
106116
return state.alwaysAllowMcp === true ? { decision: "approve" } : { decision: "ask" }
107117
}

0 commit comments

Comments
 (0)