Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/core/prompts/responses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ Otherwise, if you have not completed the task and do not need additional informa
invalidMcpToolArgumentError: (serverName: string, toolName: string) =>
`Invalid JSON argument used with ${serverName} for ${toolName}. Please retry with a properly formatted JSON argument.`,

unknownMcpToolError: (serverName: string, toolName: string, availableTools: string[]) => {
const toolsList = availableTools.length > 0 ? availableTools.join(", ") : "No tools available"
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.`
},

toolResult: (
text: string,
images?: string[],
Expand Down
154 changes: 154 additions & 0 deletions src/core/tools/__tests__/useMcpToolTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ vi.mock("../../prompts/responses", () => ({
toolResult: vi.fn((result: string) => `Tool result: ${result}`),
toolError: vi.fn((error: string) => `Tool error: ${error}`),
invalidMcpToolArgumentError: vi.fn((server: string, tool: string) => `Invalid args for ${server}:${tool}`),
unknownMcpToolError: vi.fn((server: string, tool: string, availableTools: string[]) => {
const toolsList = availableTools.length > 0 ? availableTools.join(", ") : "No tools available"
return `Tool '${tool}' does not exist on server '${server}'. Available tools: ${toolsList}`
}),
},
}))

Expand All @@ -18,6 +22,9 @@ vi.mock("../../../i18n", () => ({
if (key === "mcp:errors.invalidJsonArgument" && params?.toolName) {
return `Roo tried to use ${params.toolName} with an invalid JSON argument. Retrying...`
}
if (key === "mcp:errors.toolNotFound" && params) {
return `Tool '${params.toolName}' does not exist on server '${params.serverName}'. Available tools: ${params.availableTools}`
}
return key
}),
}))
Expand All @@ -40,6 +47,7 @@ describe("useMcpToolTool", () => {
deref: vi.fn().mockReturnValue({
getMcpHub: vi.fn().mockReturnValue({
callTool: vi.fn(),
getAllServers: vi.fn().mockReturnValue([]),
}),
postMessageToWebview: vi.fn(),
}),
Expand Down Expand Up @@ -266,5 +274,151 @@ describe("useMcpToolTool", () => {

expect(mockHandleError).toHaveBeenCalledWith("executing MCP tool", error)
})

it("should reject unknown tool names", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider adding a test case for when the MCP hub itself is unavailable to ensure the fallback behavior works correctly.

// Reset consecutiveMistakeCount for this test
mockTask.consecutiveMistakeCount = 0

const mockServers = [
{
name: "test-server",
tools: [
{ name: "existing-tool-1", description: "Tool 1" },
{ name: "existing-tool-2", description: "Tool 2" },
],
},
]

mockProviderRef.deref.mockReturnValue({
getMcpHub: () => ({
getAllServers: vi.fn().mockReturnValue(mockServers),
callTool: vi.fn(),
}),
postMessageToWebview: vi.fn(),
})

const block: ToolUse = {
type: "tool_use",
name: "use_mcp_tool",
params: {
server_name: "test-server",
tool_name: "non-existing-tool",
arguments: JSON.stringify({ test: "data" }),
},
partial: false,
}

await useMcpToolTool(
mockTask as Task,
block,
mockAskApproval,
mockHandleError,
mockPushToolResult,
mockRemoveClosingTag,
)

expect(mockTask.consecutiveMistakeCount).toBe(1)
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("does not exist"))
// Check that the error message contains available tools
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("existing-tool-1"))
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("existing-tool-2"))
})

it("should handle server with no tools", async () => {
// Reset consecutiveMistakeCount for this test
mockTask.consecutiveMistakeCount = 0

const mockServers = [
{
name: "test-server",
tools: [],
},
]

mockProviderRef.deref.mockReturnValue({
getMcpHub: () => ({
getAllServers: vi.fn().mockReturnValue(mockServers),
callTool: vi.fn(),
}),
postMessageToWebview: vi.fn(),
})

const block: ToolUse = {
type: "tool_use",
name: "use_mcp_tool",
params: {
server_name: "test-server",
tool_name: "any-tool",
arguments: JSON.stringify({ test: "data" }),
},
partial: false,
}

await useMcpToolTool(
mockTask as Task,
block,
mockAskApproval,
mockHandleError,
mockPushToolResult,
mockRemoveClosingTag,
)

expect(mockTask.consecutiveMistakeCount).toBe(1)
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("does not exist"))
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No tools available"))
})

it("should allow valid tool names", async () => {
// Reset consecutiveMistakeCount for this test
mockTask.consecutiveMistakeCount = 0

const mockServers = [
{
name: "test-server",
tools: [{ name: "valid-tool", description: "Valid Tool" }],
},
]

const mockToolResult = {
content: [{ type: "text", text: "Tool executed successfully" }],
}

mockProviderRef.deref.mockReturnValue({
getMcpHub: () => ({
getAllServers: vi.fn().mockReturnValue(mockServers),
callTool: vi.fn().mockResolvedValue(mockToolResult),
}),
postMessageToWebview: vi.fn(),
})

const block: ToolUse = {
type: "tool_use",
name: "use_mcp_tool",
params: {
server_name: "test-server",
tool_name: "valid-tool",
arguments: JSON.stringify({ test: "data" }),
},
partial: false,
}

mockAskApproval.mockResolvedValue(true)

await useMcpToolTool(
mockTask as Task,
block,
mockAskApproval,
mockHandleError,
mockPushToolResult,
mockRemoveClosingTag,
)

expect(mockTask.consecutiveMistakeCount).toBe(0)
expect(mockTask.recordToolError).not.toHaveBeenCalled()
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_request_started")
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_response", "Tool executed successfully")
})
})
})
81 changes: 81 additions & 0 deletions src/core/tools/useMcpToolTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,81 @@ async function validateParams(
}
}

async function validateToolExists(
cline: Task,
serverName: string,
toolName: string,
pushToolResult: PushToolResult,
): Promise<{ isValid: boolean; availableTools?: string[] }> {
try {
// Get the MCP hub to access server information
const provider = cline.providerRef.deref()
const mcpHub = provider?.getMcpHub()

if (!mcpHub) {
// If we can't get the MCP hub, we can't validate, so proceed with caution
return { isValid: true }
}

// Get all servers to find the specific one
const servers = mcpHub.getAllServers()
const server = servers.find((s) => s.name === serverName)

if (!server) {
// Server not found - this will be caught later in the flow
return { isValid: true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this intentional? When the server is not found, returning { isValid: true } allows execution to continue. Should we fail validation here instead to prevent unnecessary API calls?

}

// Check if the server has tools defined
if (!server.tools || server.tools.length === 0) {
// No tools available on this server
cline.consecutiveMistakeCount++
cline.recordToolError("use_mcp_tool")
await cline.say(
"error",
t("mcp:errors.toolNotFound", {
toolName,
serverName,
availableTools: "No tools available",
}),
)

pushToolResult(formatResponse.unknownMcpToolError(serverName, toolName, []))
return { isValid: false, availableTools: [] }
}

// Check if the requested tool exists
const toolExists = server.tools.some((tool) => tool.name === toolName)

if (!toolExists) {
// Tool not found - provide list of available tools
const availableToolNames = server.tools.map((tool) => tool.name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this take into account tools that the user has disabled?

Copy link
Member

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

take into account tools that the user has disabled

@mrubens

After the bug fix release, I see that disabled tools are being ignored, and the prompt mentions all tools, including the disabled ones.

I have additionally checked the system prompt, and everything is correct there - disabled tools are not included in the description.


cline.consecutiveMistakeCount++
cline.recordToolError("use_mcp_tool")
await cline.say(
"error",
t("mcp:errors.toolNotFound", {
toolName,
serverName,
availableTools: availableToolNames.join(", "),
}),
)

pushToolResult(formatResponse.unknownMcpToolError(serverName, toolName, availableToolNames))
return { isValid: false, availableTools: availableToolNames }
}

// Tool exists
return { isValid: true, availableTools: server.tools.map((tool) => tool.name) }
} catch (error) {
// If there's an error during validation, log it but don't block the tool execution
// The actual tool call might still fail with a proper error
console.error("Error validating MCP tool existence:", error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider surfacing validation errors to users for better debugging visibility. The error is only logged to console which might make troubleshooting harder.

return { isValid: true }
}
}

async function sendExecutionStatus(cline: Task, status: McpExecutionStatus): Promise<void> {
const clineProvider = await cline.providerRef.deref()
clineProvider?.postMessageToWebview({
Expand Down Expand Up @@ -193,6 +268,12 @@ export async function useMcpToolTool(

const { serverName, toolName, parsedArguments } = validation

// Validate that the tool exists on the server
const toolValidation = await validateToolExists(cline, serverName, toolName, pushToolResult)
if (!toolValidation.isValid) {
return
}

// Reset mistake count on successful validation
cline.consecutiveMistakeCount = 0

Expand Down
3 changes: 2 additions & 1 deletion src/i18n/locales/en/mcp.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"invalidJsonArgument": "Roo tried to use {{toolName}} with an invalid JSON argument. Retrying...",
"refresh_after_disable": "Failed to refresh MCP connections after disabling",
"refresh_after_enable": "Failed to refresh MCP connections after enabling",
"disconnect_servers_partial": "Failed to disconnect {{count}} MCP server(s). Check the output for details."
"disconnect_servers_partial": "Failed to disconnect {{count}} MCP server(s). Check the output for details.",
"toolNotFound": "Tool '{{toolName}}' does not exist on server '{{serverName}}'. Available tools: {{availableTools}}"
},
"info": {
"server_restarting": "Restarting {{serverName}} MCP server...",
Expand Down
Loading