mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: improve MCP tool error handling with retry logic and better messages
- Add retry logic with exponential backoff (up to 3 retries) for transient failures - Implement comprehensive error handling with user-friendly messages - Add parameter validation before MCP tool execution - Enhance error messages to provide actionable guidance - Check server connection status before attempting tool calls - Add localized error messages for better user experience Fixes #6189
This commit is contained in:
parent
2170c61e3c
commit
1a1fc31eb1
5 changed files with 171 additions and 54 deletions
|
|
@ -136,8 +136,13 @@ describe("useMcpToolTool", () => {
|
|||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.recordToolError).toHaveBeenCalledWith("use_mcp_tool")
|
||||
expect(mockTask.say).toHaveBeenCalledWith("error", expect.stringContaining("invalid JSON argument"))
|
||||
expect(mockPushToolResult).toHaveBeenCalledWith("Tool error: Invalid args for test_server:test_tool")
|
||||
expect(mockTask.say).toHaveBeenCalledWith(
|
||||
"error",
|
||||
expect.stringContaining("Invalid JSON arguments for tool 'test_tool'"),
|
||||
)
|
||||
expect(mockPushToolResult).toHaveBeenCalledWith(
|
||||
expect.stringContaining("Invalid JSON arguments for test_server.test_tool"),
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -54,20 +54,45 @@ async function validateParams(
|
|||
return { isValid: false }
|
||||
}
|
||||
|
||||
// Validate server name format
|
||||
const serverName = params.server_name.trim()
|
||||
if (!serverName) {
|
||||
cline.consecutiveMistakeCount++
|
||||
cline.recordToolError("use_mcp_tool")
|
||||
await cline.say("error", "Server name cannot be empty or contain only whitespace")
|
||||
pushToolResult(formatResponse.toolError("Invalid server name: cannot be empty"))
|
||||
return { isValid: false }
|
||||
}
|
||||
|
||||
// Validate tool name format
|
||||
const toolName = params.tool_name.trim()
|
||||
if (!toolName) {
|
||||
cline.consecutiveMistakeCount++
|
||||
cline.recordToolError("use_mcp_tool")
|
||||
await cline.say("error", "Tool name cannot be empty or contain only whitespace")
|
||||
pushToolResult(formatResponse.toolError("Invalid tool name: cannot be empty"))
|
||||
return { isValid: false }
|
||||
}
|
||||
|
||||
let parsedArguments: Record<string, unknown> | undefined
|
||||
|
||||
if (params.arguments) {
|
||||
try {
|
||||
parsedArguments = JSON.parse(params.arguments)
|
||||
|
||||
// Validate that arguments is an object (not array or primitive)
|
||||
if ((parsedArguments !== null && typeof parsedArguments !== "object") || Array.isArray(parsedArguments)) {
|
||||
throw new Error("Arguments must be a JSON object, not an array or primitive value")
|
||||
}
|
||||
} catch (error) {
|
||||
cline.consecutiveMistakeCount++
|
||||
cline.recordToolError("use_mcp_tool")
|
||||
await cline.say("error", t("mcp:errors.invalidJsonArgument", { toolName: params.tool_name }))
|
||||
|
||||
const errorMessage = error instanceof Error ? error.message : "Invalid JSON"
|
||||
await cline.say("error", `Invalid JSON arguments for tool '${toolName}': ${errorMessage}`)
|
||||
|
||||
pushToolResult(
|
||||
formatResponse.toolError(
|
||||
formatResponse.invalidMcpToolArgumentError(params.server_name, params.tool_name),
|
||||
),
|
||||
formatResponse.toolError(`Invalid JSON arguments for ${serverName}.${toolName}: ${errorMessage}`),
|
||||
)
|
||||
return { isValid: false }
|
||||
}
|
||||
|
|
@ -75,8 +100,8 @@ async function validateParams(
|
|||
|
||||
return {
|
||||
isValid: true,
|
||||
serverName: params.server_name,
|
||||
toolName: params.tool_name,
|
||||
serverName,
|
||||
toolName,
|
||||
parsedArguments,
|
||||
}
|
||||
}
|
||||
|
|
@ -127,41 +152,74 @@ async function executeToolAndProcessResult(
|
|||
toolName,
|
||||
})
|
||||
|
||||
const toolResult = await cline.providerRef.deref()?.getMcpHub()?.callTool(serverName, toolName, parsedArguments)
|
||||
let retryCount = 0
|
||||
const maxRetries = 3
|
||||
let lastError: Error | null = null
|
||||
|
||||
let toolResultPretty = "(No response)"
|
||||
while (retryCount <= maxRetries) {
|
||||
try {
|
||||
const mcpHub = cline.providerRef.deref()?.getMcpHub()
|
||||
if (!mcpHub) {
|
||||
throw new Error("MCP Hub is not available. Please ensure MCP servers are properly configured.")
|
||||
}
|
||||
|
||||
if (toolResult) {
|
||||
const outputText = processToolContent(toolResult)
|
||||
const toolResult = await mcpHub.callTool(serverName, toolName, parsedArguments)
|
||||
|
||||
if (outputText) {
|
||||
await sendExecutionStatus(cline, {
|
||||
executionId,
|
||||
status: "output",
|
||||
response: outputText,
|
||||
})
|
||||
if (toolResult) {
|
||||
const outputText = processToolContent(toolResult)
|
||||
|
||||
toolResultPretty = (toolResult.isError ? "Error:\n" : "") + outputText
|
||||
if (outputText) {
|
||||
await sendExecutionStatus(cline, {
|
||||
executionId,
|
||||
status: "output",
|
||||
response: outputText,
|
||||
})
|
||||
|
||||
const toolResultPretty = (toolResult.isError ? "Error:\n" : "") + outputText
|
||||
|
||||
// Send completion status
|
||||
await sendExecutionStatus(cline, {
|
||||
executionId,
|
||||
status: toolResult.isError ? "error" : "completed",
|
||||
response: toolResultPretty,
|
||||
error: toolResult.isError ? outputText : undefined,
|
||||
})
|
||||
|
||||
await cline.say("mcp_server_response", toolResultPretty)
|
||||
pushToolResult(formatResponse.toolResult(toolResultPretty))
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// If we get here, toolResult was null/undefined
|
||||
throw new Error(`No response received from MCP server '${serverName}' for tool '${toolName}'`)
|
||||
} catch (error) {
|
||||
lastError = error instanceof Error ? error : new Error(String(error))
|
||||
retryCount++
|
||||
|
||||
if (retryCount <= maxRetries) {
|
||||
const delay = Math.min(1000 * Math.pow(2, retryCount - 1), 5000) // Exponential backoff with max 5s
|
||||
await cline.say(
|
||||
"error",
|
||||
`MCP tool execution failed (attempt ${retryCount}/${maxRetries}). Retrying in ${delay / 1000}s...`,
|
||||
)
|
||||
await new Promise((resolve) => setTimeout(resolve, delay))
|
||||
}
|
||||
}
|
||||
|
||||
// Send completion status
|
||||
await sendExecutionStatus(cline, {
|
||||
executionId,
|
||||
status: toolResult.isError ? "error" : "completed",
|
||||
response: toolResultPretty,
|
||||
error: toolResult.isError ? "Error executing MCP tool" : undefined,
|
||||
})
|
||||
} else {
|
||||
// Send error status if no result
|
||||
await sendExecutionStatus(cline, {
|
||||
executionId,
|
||||
status: "error",
|
||||
error: "No response from MCP server",
|
||||
})
|
||||
}
|
||||
|
||||
await cline.say("mcp_server_response", toolResultPretty)
|
||||
pushToolResult(formatResponse.toolResult(toolResultPretty))
|
||||
// All retries failed
|
||||
const errorMessage = lastError?.message || "Unknown error occurred"
|
||||
const userFriendlyError = `Failed to execute MCP tool '${toolName}' on server '${serverName}' after ${maxRetries} attempts. ${errorMessage}`
|
||||
|
||||
await sendExecutionStatus(cline, {
|
||||
executionId,
|
||||
status: "error",
|
||||
error: userFriendlyError,
|
||||
})
|
||||
|
||||
await cline.say("mcp_server_response", `Error: ${userFriendlyError}`)
|
||||
pushToolResult(formatResponse.toolError(userFriendlyError))
|
||||
}
|
||||
|
||||
export async function useMcpToolTool(
|
||||
|
|
|
|||
|
|
@ -5,7 +5,9 @@
|
|||
"invalid_settings_validation": "Invalid MCP settings format: {{errorMessages}}",
|
||||
"create_json": "Failed to create or open .roo/mcp.json: {{error}}",
|
||||
"failed_update_project": "Failed to update project MCP servers",
|
||||
"invalidJsonArgument": "Roo tried to use {{toolName}} with an invalid JSON argument. Retrying..."
|
||||
"invalidJsonArgument": "Roo tried to use {{toolName}} with an invalid JSON argument. Retrying...",
|
||||
"connection_failed": "Failed to connect to MCP server '{{serverName}}'. Please check your server configuration and ensure the server is running.",
|
||||
"tool_execution_failed": "Failed to execute MCP tool '{{toolName}}' on server '{{serverName}}': {{error}}"
|
||||
},
|
||||
"info": {
|
||||
"server_restarting": "Restarting {{serverName}} MCP server...",
|
||||
|
|
|
|||
|
|
@ -1464,12 +1464,39 @@ export class McpHub {
|
|||
): Promise<McpToolCallResponse> {
|
||||
const connection = this.findConnection(serverName, source)
|
||||
if (!connection) {
|
||||
// Provide helpful suggestions
|
||||
const availableServers = this.getAllServers()
|
||||
.filter((s) => !s.disabled)
|
||||
.map((s) => s.name)
|
||||
.join(", ")
|
||||
|
||||
const errorMsg =
|
||||
availableServers.length > 0
|
||||
? `MCP server '${serverName}' not found. Available servers: ${availableServers}`
|
||||
: `No MCP servers are currently connected. Please configure MCP servers in your settings.`
|
||||
|
||||
throw new Error(errorMsg)
|
||||
}
|
||||
|
||||
if (connection.server.disabled) {
|
||||
throw new Error(
|
||||
`No connection found for server: ${serverName}${source ? ` with source ${source}` : ""}. Please make sure to use MCP servers available under 'Connected MCP Servers'.`,
|
||||
`MCP server '${serverName}' is currently disabled. Please enable it in the MCP settings to use it.`,
|
||||
)
|
||||
}
|
||||
if (connection.server.disabled) {
|
||||
throw new Error(`Server "${serverName}" is disabled and cannot be used`)
|
||||
|
||||
// Check if server is connected
|
||||
if (connection.server.status !== "connected") {
|
||||
throw new Error(
|
||||
`MCP server '${serverName}' is not connected (status: ${connection.server.status}). Please check the server configuration and logs.`,
|
||||
)
|
||||
}
|
||||
|
||||
// Validate tool exists
|
||||
const availableTools = connection.server.tools || []
|
||||
const toolExists = availableTools.some((t) => t.name === toolName)
|
||||
if (!toolExists && availableTools.length > 0) {
|
||||
const toolNames = availableTools.map((t) => t.name).join(", ")
|
||||
throw new Error(`Tool '${toolName}' not found on server '${serverName}'. Available tools: ${toolNames}`)
|
||||
}
|
||||
|
||||
let timeout: number
|
||||
|
|
@ -1482,19 +1509,44 @@ export class McpHub {
|
|||
timeout = 60 * 1000
|
||||
}
|
||||
|
||||
return await connection.client.request(
|
||||
{
|
||||
method: "tools/call",
|
||||
params: {
|
||||
name: toolName,
|
||||
arguments: toolArguments,
|
||||
try {
|
||||
const result = await connection.client.request(
|
||||
{
|
||||
method: "tools/call",
|
||||
params: {
|
||||
name: toolName,
|
||||
arguments: toolArguments,
|
||||
},
|
||||
},
|
||||
},
|
||||
CallToolResultSchema,
|
||||
{
|
||||
timeout,
|
||||
},
|
||||
)
|
||||
CallToolResultSchema,
|
||||
{
|
||||
timeout,
|
||||
},
|
||||
)
|
||||
|
||||
return result
|
||||
} catch (error) {
|
||||
// Enhance error messages for common issues
|
||||
if (error instanceof Error) {
|
||||
if (error.message.includes("timeout")) {
|
||||
throw new Error(
|
||||
`MCP tool '${toolName}' on server '${serverName}' timed out after ${timeout / 1000} seconds. The tool may be taking too long to respond or the server may be unresponsive.`,
|
||||
)
|
||||
}
|
||||
if (error.message.includes("ECONNREFUSED")) {
|
||||
throw new Error(
|
||||
`Cannot connect to MCP server '${serverName}'. The server may not be running or may be configured incorrectly.`,
|
||||
)
|
||||
}
|
||||
if (error.message.includes("EPIPE") || error.message.includes("ENOTCONN")) {
|
||||
throw new Error(
|
||||
`Lost connection to MCP server '${serverName}'. The server may have crashed or been terminated.`,
|
||||
)
|
||||
}
|
||||
}
|
||||
// Re-throw with original error if not a known issue
|
||||
throw error
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -545,7 +545,7 @@ describe("McpHub", () => {
|
|||
mcpHub.connections = [mockConnection]
|
||||
|
||||
await expect(mcpHub.callTool("disabled-server", "some-tool", {})).rejects.toThrow(
|
||||
'Server "disabled-server" is disabled and cannot be used',
|
||||
"MCP server 'disabled-server' is currently disabled. Please enable it in the MCP settings to use it.",
|
||||
)
|
||||
})
|
||||
|
||||
|
|
@ -610,7 +610,7 @@ describe("McpHub", () => {
|
|||
|
||||
it("should throw error if server not found", async () => {
|
||||
await expect(mcpHub.callTool("non-existent-server", "some-tool", {})).rejects.toThrow(
|
||||
"No connection found for server: non-existent-server",
|
||||
"No MCP servers are currently connected. Please configure MCP servers in your settings.",
|
||||
)
|
||||
})
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue