Skip to content

Commit 3a30a21

Browse files
roomoteroomote
committed
fix: validate MCP tool exists before execution
- Add validation to check if requested tool exists on the MCP server - Provide helpful error messages with list of available tools - Prevent sending requests for non-existent tools to MCP servers - Add comprehensive tests for the new validation logic Fixes #7631
1 parent 130ce29 commit 3a30a21

File tree

4 files changed

+242
-1
lines changed

4 files changed

+242
-1
lines changed

‎src/core/prompts/responses.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,11 @@ 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+
7580
toolResult: (
7681
text: string,
7782
images?: string[],

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

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ 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+
}),
1317
},
1418
}))
1519

@@ -18,6 +22,9 @@ vi.mock("../../../i18n", () => ({
1822
if (key === "mcp:errors.invalidJsonArgument" && params?.toolName) {
1923
return `Roo tried to use ${params.toolName} with an invalid JSON argument. Retrying...`
2024
}
25+
if (key === "mcp:errors.toolNotFound" && params) {
26+
return `Tool '${params.toolName}' does not exist on server '${params.serverName}'. Available tools: ${params.availableTools}`
27+
}
2128
return key
2229
}),
2330
}))
@@ -40,6 +47,7 @@ describe("useMcpToolTool", () => {
4047
deref: vi.fn().mockReturnValue({
4148
getMcpHub: vi.fn().mockReturnValue({
4249
callTool: vi.fn(),
50+
getAllServers: vi.fn().mockReturnValue([]),
4351
}),
4452
postMessageToWebview: vi.fn(),
4553
}),
@@ -266,5 +274,151 @@ describe("useMcpToolTool", () => {
266274

267275
expect(mockHandleError).toHaveBeenCalledWith("executing MCP tool", error)
268276
})
277+
278+
it("should reject unknown tool names", async () => {
279+
// Reset consecutiveMistakeCount for this test
280+
mockTask.consecutiveMistakeCount = 0
281+
282+
const mockServers = [
283+
{
284+
name: "test-server",
285+
tools: [
286+
{ name: "existing-tool-1", description: "Tool 1" },
287+
{ name: "existing-tool-2", description: "Tool 2" },
288+
],
289+
},
290+
]
291+
292+
mockProviderRef.deref.mockReturnValue({
293+
getMcpHub: () => ({
294+
getAllServers: vi.fn().mockReturnValue(mockServers),
295+
callTool: vi.fn(),
296+
}),
297+
postMessageToWebview: vi.fn(),
298+
})
299+
300+
const block: ToolUse = {
301+
type: "tool_use",
302+
name: "use_mcp_tool",
303+
params: {
304+
server_name: "test-server",
305+
tool_name: "non-existing-tool",
306+
arguments: JSON.stringify({ test: "data" }),
307+
},
308+
partial: false,
309+
}
310+
311+
await useMcpToolTool(
312+
mockTask as Task,
313+
block,
314+
mockAskApproval,
315+
mockHandleError,
316+
mockPushToolResult,
317+
mockRemoveClosingTag,
318+
)
319+
320+
expect(mockTask.consecutiveMistakeCount).toBe(1)
321+
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
322+
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("does not exist"))
323+
// Check that the error message contains available tools
324+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("existing-tool-1"))
325+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("existing-tool-2"))
326+
})
327+
328+
it("should handle server with no tools", async () => {
329+
// Reset consecutiveMistakeCount for this test
330+
mockTask.consecutiveMistakeCount = 0
331+
332+
const mockServers = [
333+
{
334+
name: "test-server",
335+
tools: [],
336+
},
337+
]
338+
339+
mockProviderRef.deref.mockReturnValue({
340+
getMcpHub: () => ({
341+
getAllServers: vi.fn().mockReturnValue(mockServers),
342+
callTool: vi.fn(),
343+
}),
344+
postMessageToWebview: vi.fn(),
345+
})
346+
347+
const block: ToolUse = {
348+
type: "tool_use",
349+
name: "use_mcp_tool",
350+
params: {
351+
server_name: "test-server",
352+
tool_name: "any-tool",
353+
arguments: JSON.stringify({ test: "data" }),
354+
},
355+
partial: false,
356+
}
357+
358+
await useMcpToolTool(
359+
mockTask as Task,
360+
block,
361+
mockAskApproval,
362+
mockHandleError,
363+
mockPushToolResult,
364+
mockRemoveClosingTag,
365+
)
366+
367+
expect(mockTask.consecutiveMistakeCount).toBe(1)
368+
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
369+
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("does not exist"))
370+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No tools available"))
371+
})
372+
373+
it("should allow valid tool names", async () => {
374+
// Reset consecutiveMistakeCount for this test
375+
mockTask.consecutiveMistakeCount = 0
376+
377+
const mockServers = [
378+
{
379+
name: "test-server",
380+
tools: [{ name: "valid-tool", description: "Valid Tool" }],
381+
},
382+
]
383+
384+
const mockToolResult = {
385+
content: [{ type: "text", text: "Tool executed successfully" }],
386+
}
387+
388+
mockProviderRef.deref.mockReturnValue({
389+
getMcpHub: () => ({
390+
getAllServers: vi.fn().mockReturnValue(mockServers),
391+
callTool: vi.fn().mockResolvedValue(mockToolResult),
392+
}),
393+
postMessageToWebview: vi.fn(),
394+
})
395+
396+
const block: ToolUse = {
397+
type: "tool_use",
398+
name: "use_mcp_tool",
399+
params: {
400+
server_name: "test-server",
401+
tool_name: "valid-tool",
402+
arguments: JSON.stringify({ test: "data" }),
403+
},
404+
partial: false,
405+
}
406+
407+
mockAskApproval.mockResolvedValue(true)
408+
409+
await useMcpToolTool(
410+
mockTask as Task,
411+
block,
412+
mockAskApproval,
413+
mockHandleError,
414+
mockPushToolResult,
415+
mockRemoveClosingTag,
416+
)
417+
418+
expect(mockTask.consecutiveMistakeCount).toBe(0)
419+
expect(mockTask.recordToolError).not.toHaveBeenCalled()
420+
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_request_started")
421+
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_response", "Tool executed successfully")
422+
})
269423
})
270424
})

‎src/core/tools/useMcpToolTool.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,81 @@ async function validateParams(
8181
}
8282
}
8383

84+
async function validateToolExists(
85+
cline: Task,
86+
serverName: string,
87+
toolName: string,
88+
pushToolResult: PushToolResult,
89+
): Promise<{ isValid: boolean; availableTools?: string[] }> {
90+
try {
91+
// Get the MCP hub to access server information
92+
const provider = cline.providerRef.deref()
93+
const mcpHub = provider?.getMcpHub()
94+
95+
if (!mcpHub) {
96+
// If we can't get the MCP hub, we can't validate, so proceed with caution
97+
return { isValid: true }
98+
}
99+
100+
// Get all servers to find the specific one
101+
const servers = mcpHub.getAllServers()
102+
const server = servers.find((s) => s.name === serverName)
103+
104+
if (!server) {
105+
// Server not found - this will be caught later in the flow
106+
return { isValid: true }
107+
}
108+
109+
// Check if the server has tools defined
110+
if (!server.tools || server.tools.length === 0) {
111+
// No tools available on this server
112+
cline.consecutiveMistakeCount++
113+
cline.recordToolError("use_mcp_tool")
114+
await cline.say(
115+
"error",
116+
t("mcp:errors.toolNotFound", {
117+
toolName,
118+
serverName,
119+
availableTools: "No tools available",
120+
}),
121+
)
122+
123+
pushToolResult(formatResponse.unknownMcpToolError(serverName, toolName, []))
124+
return { isValid: false, availableTools: [] }
125+
}
126+
127+
// Check if the requested tool exists
128+
const toolExists = server.tools.some((tool) => tool.name === toolName)
129+
130+
if (!toolExists) {
131+
// Tool not found - provide list of available tools
132+
const availableToolNames = server.tools.map((tool) => tool.name)
133+
134+
cline.consecutiveMistakeCount++
135+
cline.recordToolError("use_mcp_tool")
136+
await cline.say(
137+
"error",
138+
t("mcp:errors.toolNotFound", {
139+
toolName,
140+
serverName,
141+
availableTools: availableToolNames.join(", "),
142+
}),
143+
)
144+
145+
pushToolResult(formatResponse.unknownMcpToolError(serverName, toolName, availableToolNames))
146+
return { isValid: false, availableTools: availableToolNames }
147+
}
148+
149+
// Tool exists
150+
return { isValid: true, availableTools: server.tools.map((tool) => tool.name) }
151+
} catch (error) {
152+
// If there's an error during validation, log it but don't block the tool execution
153+
// The actual tool call might still fail with a proper error
154+
console.error("Error validating MCP tool existence:", error)
155+
return { isValid: true }
156+
}
157+
}
158+
84159
async function sendExecutionStatus(cline: Task, status: McpExecutionStatus): Promise<void> {
85160
const clineProvider = await cline.providerRef.deref()
86161
clineProvider?.postMessageToWebview({
@@ -193,6 +268,12 @@ export async function useMcpToolTool(
193268

194269
const { serverName, toolName, parsedArguments } = validation
195270

271+
// Validate that the tool exists on the server
272+
const toolValidation = await validateToolExists(cline, serverName, toolName, pushToolResult)
273+
if (!toolValidation.isValid) {
274+
return
275+
}
276+
196277
// Reset mistake count on successful validation
197278
cline.consecutiveMistakeCount = 0
198279

‎src/i18n/locales/en/mcp.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88
"invalidJsonArgument": "Roo tried to use {{toolName}} with an invalid JSON argument. Retrying...",
99
"refresh_after_disable": "Failed to refresh MCP connections after disabling",
1010
"refresh_after_enable": "Failed to refresh MCP connections after enabling",
11-
"disconnect_servers_partial": "Failed to disconnect {{count}} MCP server(s). Check the output for details."
11+
"disconnect_servers_partial": "Failed to disconnect {{count}} MCP server(s). Check the output for details.",
12+
"toolNotFound": "Tool '{{toolName}}' does not exist on server '{{serverName}}'. Available tools: {{availableTools}}"
1213
},
1314
"info": {
1415
"server_restarting": "Restarting {{serverName}} MCP server...",

0 commit comments

Comments
 (0)