-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: validate MCP tool exists before execution #7632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed my own code. Found it disturbingly functional. Clearly a glitch in the matrix.
src/core/tools/useMcpToolTool.ts
Outdated
|
|
||
| if (!server) { | ||
| // Server not found - this will be caught later in the flow | ||
| return { isValid: true } |
There was a problem hiding this comment.
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?
| } 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) |
There was a problem hiding this comment.
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.
| expect(mockHandleError).toHaveBeenCalledWith("executing MCP tool", error) | ||
| }) | ||
|
|
||
| it("should reject unknown tool names", async () => { |
There was a problem hiding this comment.
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.
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| if (!toolExists) { | ||
| // Tool not found - provide list of available tools | ||
| const availableToolNames = server.tools.map((tool) => tool.name) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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
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.
- Check enabledForPrompt property in validateToolExists function - Reject disabled tools with appropriate error message - Show only enabled tools in error message when tool is disabled Addresses PR feedback about checking for disabled tools
src/core/tools/useMcpToolTool.ts
Outdated
| cline.recordToolError("use_mcp_tool") | ||
| await cline.say( | ||
| "error", | ||
| `Tool '${toolName}' on server '${serverName}' is disabled. Available enabled tools: ${enabledToolNames.length > 0 ? enabledToolNames.join(", ") : "No enabled tools available"}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using the i18n translation function (t()) for the disabled tool error message instead of an inline string. For consistency with the tool-not-found error (which uses t('mcp:errors.toolNotFound')), refactor this error message (and the "No enabled tools available" text) to use a translation key (e.g., t('mcp:errors.toolDisabled', { toolName, serverName, enabledTools: enabledToolNames.join(", ") })) so that the message supports multiple languages.
| `Tool '${toolName}' on server '${serverName}' is disabled. Available enabled tools: ${enabledToolNames.length > 0 ? enabledToolNames.join(", ") : "No enabled tools available"}`, | |
| t('mcp:errors.toolDisabled', { toolName, serverName, enabledTools: enabledToolNames.length > 0 ? enabledToolNames.join(", ") : "No enabled tools available" }), |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
- Replace hardcoded error message with t() translation function - Add toolDisabled translation key to all 17 locale files - Maintains proper placeholder variables for code integration
Summary
This PR addresses Issue #7631 by adding validation to prevent Roo from calling MCP tools that don't exist on a server.
Problem
Previously, when the model requested a non-existent tool on an MCP server, the request would be sent to the server anyway, resulting in a raw error response being displayed to the user.
Solution
Added validation that checks if the requested tool exists on the MCP server before attempting to execute it. If the tool doesn't exist:
Changes
validateToolExistsfunction inuseMcpToolTool.tsto check tool availabilityTesting
Fixes #7631
Important
Adds validation in
useMcpToolTool.tsto check MCP tool existence before execution, with error handling and i18n support.validateToolExistsinuseMcpToolTool.tsto check tool existence on MCP server before execution.responses.ts.useMcpToolTool.spec.tsfor tool validation scenarios (unknown tools, no tools, valid tools).This description was created by
for 97945bd. You can customize this summary. It will automatically update as commits are pushed.