Skip to content

Commit 7935c94

Browse files
roomote[bot]roomotedaniel-lxs
authored
fix: validate MCP tool exists before execution (#7632)
Co-authored-by: Roo Code <[email protected]> Co-authored-by: Daniel Riccio <[email protected]>
1 parent 72502d8 commit 7935c94

File tree

21 files changed

+460
-18
lines changed

21 files changed

+460
-18
lines changed

‎src/core/prompts/responses.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,16 @@ Otherwise, if you have not completed the task and do not need additional informa
7272
invalidMcpToolArgumentError: (serverName: string, toolName: string) =>
7373
`Invalid JSON argument used with ${serverName} for ${toolName}. Please retry with a properly formatted JSON argument.`,
7474

75+
unknownMcpToolError: (serverName: string, toolName: string, availableTools: string[]) => {
76+
const toolsList = availableTools.length > 0 ? availableTools.join(", ") : "No tools available"
77+
return `Tool '${toolName}' does not exist on server '${serverName}'.\n\nAvailable tools on this server: ${toolsList}\n\nPlease use one of the available tools or check if the server is properly configured.`
78+
},
79+
80+
unknownMcpServerError: (serverName: string, availableServers: string[]) => {
81+
const serversList = availableServers.length > 0 ? availableServers.join(", ") : "No servers available"
82+
return `Server '${serverName}' is not configured. Available servers: ${serversList}`
83+
},
84+
7585
toolResult: (
7686
text: string,
7787
images?: string[],

‎src/core/tools/__tests__/useMcpToolTool.spec.ts‎

Lines changed: 266 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,14 @@ vi.mock("../../prompts/responses", () => ({
1010
toolResult: vi.fn((result: string) => `Tool result: ${result}`),
1111
toolError: vi.fn((error: string) => `Tool error: ${error}`),
1212
invalidMcpToolArgumentError: vi.fn((server: string, tool: string) => `Invalid args for ${server}:${tool}`),
13+
unknownMcpToolError: vi.fn((server: string, tool: string, availableTools: string[]) => {
14+
const toolsList = availableTools.length > 0 ? availableTools.join(", ") : "No tools available"
15+
return `Tool '${tool}' does not exist on server '${server}'. Available tools: ${toolsList}`
16+
}),
17+
unknownMcpServerError: vi.fn((server: string, availableServers: string[]) => {
18+
const list = availableServers.length > 0 ? availableServers.join(", ") : "No servers available"
19+
return `Server '${server}' is not configured. Available servers: ${list}`
20+
}),
1321
},
1422
}))
1523

@@ -18,6 +26,12 @@ vi.mock("../../../i18n", () => ({
1826
if (key === "mcp:errors.invalidJsonArgument" && params?.toolName) {
1927
return `Roo tried to use ${params.toolName} with an invalid JSON argument. Retrying...`
2028
}
29+
if (key === "mcp:errors.toolNotFound" && params) {
30+
return `Tool '${params.toolName}' does not exist on server '${params.serverName}'. Available tools: ${params.availableTools}`
31+
}
32+
if (key === "mcp:errors.serverNotFound" && params) {
33+
return `MCP server '${params.serverName}' is not configured. Available servers: ${params.availableServers}`
34+
}
2135
return key
2236
}),
2337
}))
@@ -40,6 +54,7 @@ describe("useMcpToolTool", () => {
4054
deref: vi.fn().mockReturnValue({
4155
getMcpHub: vi.fn().mockReturnValue({
4256
callTool: vi.fn(),
57+
getAllServers: vi.fn().mockReturnValue([]),
4358
}),
4459
postMessageToWebview: vi.fn(),
4560
}),
@@ -224,6 +239,10 @@ describe("useMcpToolTool", () => {
224239
partial: false,
225240
}
226241

242+
// Ensure validation does not fail due to unknown server by returning no provider once
243+
// This makes validateToolExists return isValid: true and proceed to askApproval
244+
mockProviderRef.deref.mockReturnValueOnce(undefined as any)
245+
227246
mockAskApproval.mockResolvedValue(false)
228247

229248
await useMcpToolTool(
@@ -252,6 +271,19 @@ describe("useMcpToolTool", () => {
252271
partial: false,
253272
}
254273

274+
// Ensure validation passes so askApproval is reached and throws
275+
mockProviderRef.deref.mockReturnValueOnce({
276+
getMcpHub: () => ({
277+
getAllServers: vi
278+
.fn()
279+
.mockReturnValue([
280+
{ name: "test_server", tools: [{ name: "test_tool", description: "desc" }] },
281+
]),
282+
callTool: vi.fn(),
283+
}),
284+
postMessageToWebview: vi.fn(),
285+
})
286+
255287
const error = new Error("Unexpected error")
256288
mockAskApproval.mockRejectedValue(error)
257289

@@ -266,5 +298,239 @@ describe("useMcpToolTool", () => {
266298

267299
expect(mockHandleError).toHaveBeenCalledWith("executing MCP tool", error)
268300
})
301+
302+
it("should reject unknown tool names", async () => {
303+
// Reset consecutiveMistakeCount for this test
304+
mockTask.consecutiveMistakeCount = 0
305+
306+
const mockServers = [
307+
{
308+
name: "test-server",
309+
tools: [
310+
{ name: "existing-tool-1", description: "Tool 1" },
311+
{ name: "existing-tool-2", description: "Tool 2" },
312+
],
313+
},
314+
]
315+
316+
mockProviderRef.deref.mockReturnValue({
317+
getMcpHub: () => ({
318+
getAllServers: vi.fn().mockReturnValue(mockServers),
319+
callTool: vi.fn(),
320+
}),
321+
postMessageToWebview: vi.fn(),
322+
})
323+
324+
const block: ToolUse = {
325+
type: "tool_use",
326+
name: "use_mcp_tool",
327+
params: {
328+
server_name: "test-server",
329+
tool_name: "non-existing-tool",
330+
arguments: JSON.stringify({ test: "data" }),
331+
},
332+
partial: false,
333+
}
334+
335+
await useMcpToolTool(
336+
mockTask as Task,
337+
block,
338+
mockAskApproval,
339+
mockHandleError,
340+
mockPushToolResult,
341+
mockRemoveClosingTag,
342+
)
343+
344+
expect(mockTask.consecutiveMistakeCount).toBe(1)
345+
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
346+
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("does not exist"))
347+
// Check that the error message contains available tools
348+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("existing-tool-1"))
349+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("existing-tool-2"))
350+
})
351+
352+
it("should handle server with no tools", async () => {
353+
// Reset consecutiveMistakeCount for this test
354+
mockTask.consecutiveMistakeCount = 0
355+
356+
const mockServers = [
357+
{
358+
name: "test-server",
359+
tools: [],
360+
},
361+
]
362+
363+
mockProviderRef.deref.mockReturnValue({
364+
getMcpHub: () => ({
365+
getAllServers: vi.fn().mockReturnValue(mockServers),
366+
callTool: vi.fn(),
367+
}),
368+
postMessageToWebview: vi.fn(),
369+
})
370+
371+
const block: ToolUse = {
372+
type: "tool_use",
373+
name: "use_mcp_tool",
374+
params: {
375+
server_name: "test-server",
376+
tool_name: "any-tool",
377+
arguments: JSON.stringify({ test: "data" }),
378+
},
379+
partial: false,
380+
}
381+
382+
await useMcpToolTool(
383+
mockTask as Task,
384+
block,
385+
mockAskApproval,
386+
mockHandleError,
387+
mockPushToolResult,
388+
mockRemoveClosingTag,
389+
)
390+
391+
expect(mockTask.consecutiveMistakeCount).toBe(1)
392+
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
393+
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("does not exist"))
394+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No tools available"))
395+
})
396+
397+
it("should allow valid tool names", async () => {
398+
// Reset consecutiveMistakeCount for this test
399+
mockTask.consecutiveMistakeCount = 0
400+
401+
const mockServers = [
402+
{
403+
name: "test-server",
404+
tools: [{ name: "valid-tool", description: "Valid Tool" }],
405+
},
406+
]
407+
408+
const mockToolResult = {
409+
content: [{ type: "text", text: "Tool executed successfully" }],
410+
}
411+
412+
mockProviderRef.deref.mockReturnValue({
413+
getMcpHub: () => ({
414+
getAllServers: vi.fn().mockReturnValue(mockServers),
415+
callTool: vi.fn().mockResolvedValue(mockToolResult),
416+
}),
417+
postMessageToWebview: vi.fn(),
418+
})
419+
420+
const block: ToolUse = {
421+
type: "tool_use",
422+
name: "use_mcp_tool",
423+
params: {
424+
server_name: "test-server",
425+
tool_name: "valid-tool",
426+
arguments: JSON.stringify({ test: "data" }),
427+
},
428+
partial: false,
429+
}
430+
431+
mockAskApproval.mockResolvedValue(true)
432+
433+
await useMcpToolTool(
434+
mockTask as Task,
435+
block,
436+
mockAskApproval,
437+
mockHandleError,
438+
mockPushToolResult,
439+
mockRemoveClosingTag,
440+
)
441+
442+
expect(mockTask.consecutiveMistakeCount).toBe(0)
443+
expect(mockTask.recordToolError).not.toHaveBeenCalled()
444+
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_request_started")
445+
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_response", "Tool executed successfully")
446+
})
447+
448+
it("should reject unknown server names with available servers listed", async () => {
449+
// Arrange
450+
mockTask.consecutiveMistakeCount = 0
451+
452+
const mockServers = [{ name: "s1", tools: [] }]
453+
const callToolMock = vi.fn()
454+
455+
mockProviderRef.deref.mockReturnValue({
456+
getMcpHub: () => ({
457+
getAllServers: vi.fn().mockReturnValue(mockServers),
458+
callTool: callToolMock,
459+
}),
460+
postMessageToWebview: vi.fn(),
461+
})
462+
463+
const block: ToolUse = {
464+
type: "tool_use",
465+
name: "use_mcp_tool",
466+
params: {
467+
server_name: "unknown",
468+
tool_name: "any-tool",
469+
arguments: "{}",
470+
},
471+
partial: false,
472+
}
473+
474+
// Act
475+
await useMcpToolTool(
476+
mockTask as Task,
477+
block,
478+
mockAskApproval,
479+
mockHandleError,
480+
mockPushToolResult,
481+
mockRemoveClosingTag,
482+
)
483+
484+
// Assert
485+
expect(mockTask.consecutiveMistakeCount).toBe(1)
486+
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
487+
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("not configured"))
488+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("s1"))
489+
expect(callToolMock).not.toHaveBeenCalled()
490+
expect(mockAskApproval).not.toHaveBeenCalled()
491+
})
492+
493+
it("should reject unknown server names when no servers are available", async () => {
494+
// Arrange
495+
mockTask.consecutiveMistakeCount = 0
496+
497+
const callToolMock = vi.fn()
498+
mockProviderRef.deref.mockReturnValue({
499+
getMcpHub: () => ({
500+
getAllServers: vi.fn().mockReturnValue([]),
501+
callTool: callToolMock,
502+
}),
503+
postMessageToWebview: vi.fn(),
504+
})
505+
506+
const block: ToolUse = {
507+
type: "tool_use",
508+
name: "use_mcp_tool",
509+
params: {
510+
server_name: "unknown",
511+
tool_name: "any-tool",
512+
arguments: "{}",
513+
},
514+
partial: false,
515+
}
516+
517+
// Act
518+
await useMcpToolTool(
519+
mockTask as Task,
520+
block,
521+
mockAskApproval,
522+
mockHandleError,
523+
mockPushToolResult,
524+
mockRemoveClosingTag,
525+
)
526+
527+
// Assert
528+
expect(mockTask.consecutiveMistakeCount).toBe(1)
529+
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
530+
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("not configured"))
531+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No servers available"))
532+
expect(callToolMock).not.toHaveBeenCalled()
533+
expect(mockAskApproval).not.toHaveBeenCalled()
534+
})
269535
})
270536
})

0 commit comments

Comments
 (0)