Skip to content

Commit 326268a

Browse files
committed
fix(mcp): fail fast on unknown server; add i18n keys/translations; add tests
1 parent 3a30a21 commit 326268a

File tree

21 files changed

+181
-20
lines changed

21 files changed

+181
-20
lines changed

src/core/prompts/responses.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@ Otherwise, if you have not completed the task and do not need additional informa
7777
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.`
7878
},
7979

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+
8085
toolResult: (
8186
text: string,
8287
images?: string[],

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

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ vi.mock("../../prompts/responses", () => ({
1414
const toolsList = availableTools.length > 0 ? availableTools.join(", ") : "No tools available"
1515
return `Tool '${tool}' does not exist on server '${server}'. Available tools: ${toolsList}`
1616
}),
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+
}),
1721
},
1822
}))
1923

@@ -25,6 +29,9 @@ vi.mock("../../../i18n", () => ({
2529
if (key === "mcp:errors.toolNotFound" && params) {
2630
return `Tool '${params.toolName}' does not exist on server '${params.serverName}'. Available tools: ${params.availableTools}`
2731
}
32+
if (key === "mcp:errors.serverNotFound" && params) {
33+
return `MCP server '${params.serverName}' is not configured. Available servers: ${params.availableServers}`
34+
}
2835
return key
2936
}),
3037
}))
@@ -232,6 +239,10 @@ describe("useMcpToolTool", () => {
232239
partial: false,
233240
}
234241

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+
235246
mockAskApproval.mockResolvedValue(false)
236247

237248
await useMcpToolTool(
@@ -260,6 +271,19 @@ describe("useMcpToolTool", () => {
260271
partial: false,
261272
}
262273

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+
263287
const error = new Error("Unexpected error")
264288
mockAskApproval.mockRejectedValue(error)
265289

@@ -420,5 +444,93 @@ describe("useMcpToolTool", () => {
420444
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_request_started")
421445
expect(mockTask.say).toHaveBeenCalledWith("mcp_server_response", "Tool executed successfully")
422446
})
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+
})
423535
})
424536
})

src/core/tools/useMcpToolTool.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,17 @@ async function validateToolExists(
102102
const server = servers.find((s) => s.name === serverName)
103103

104104
if (!server) {
105-
// Server not found - this will be caught later in the flow
106-
return { isValid: true }
105+
// Fail fast when server is unknown
106+
const availableServersArray = servers.map((s) => s.name)
107+
const availableServers =
108+
availableServersArray.length > 0 ? availableServersArray.join(", ") : "No servers available"
109+
110+
cline.consecutiveMistakeCount++
111+
cline.recordToolError("use_mcp_tool")
112+
await cline.say("error", t("mcp:errors.serverNotFound", { serverName, availableServers }))
113+
114+
pushToolResult(formatResponse.unknownMcpServerError(serverName, availableServersArray))
115+
return { isValid: false, availableTools: [] }
107116
}
108117

109118
// Check if the server has tools defined

src/i18n/locales/ca/mcp.json

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/i18n/locales/de/mcp.json

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/i18n/locales/en/mcp.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@
99
"refresh_after_disable": "Failed to refresh MCP connections after disabling",
1010
"refresh_after_enable": "Failed to refresh MCP connections after enabling",
1111
"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}}"
12+
"toolNotFound": "Tool '{{toolName}}' does not exist on server '{{serverName}}'. Available tools: {{availableTools}}",
13+
"serverNotFound": "MCP server '{{serverName}}' is not configured. Available servers: {{availableServers}}"
1314
},
1415
"info": {
1516
"server_restarting": "Restarting {{serverName}} MCP server...",

src/i18n/locales/es/mcp.json

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/i18n/locales/fr/mcp.json

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/i18n/locales/hi/mcp.json

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/i18n/locales/id/mcp.json

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)