Skip to content

Commit 3521270

Browse files
authored
feat: sanitize MCP server/tool names for API compatibility (#10054)
1 parent f60c14e commit 3521270

File tree

6 files changed

+359
-20
lines changed

6 files changed

+359
-20
lines changed

src/core/assistant-message/NativeToolCallParser.ts

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type {
1313
ApiStreamToolCallDeltaChunk,
1414
ApiStreamToolCallEndChunk,
1515
} from "../../api/transform/stream"
16+
import { MCP_TOOL_PREFIX, MCP_TOOL_SEPARATOR, parseMcpToolName } from "../../utils/mcp-name"
1617

1718
/**
1819
* Helper type to extract properly typed native arguments for a given tool.
@@ -238,7 +239,8 @@ export class NativeToolCallParser {
238239
toolCall.argumentsAccumulator += chunk
239240

240241
// For dynamic MCP tools, we don't return partial updates - wait for final
241-
if (toolCall.name.startsWith("mcp_")) {
242+
const mcpPrefix = MCP_TOOL_PREFIX + MCP_TOOL_SEPARATOR
243+
if (toolCall.name.startsWith(mcpPrefix)) {
242244
return null
243245
}
244246

@@ -554,8 +556,9 @@ export class NativeToolCallParser {
554556
name: TName
555557
arguments: string
556558
}): ToolUse<TName> | McpToolUse | null {
557-
// Check if this is a dynamic MCP tool (mcp_serverName_toolName)
558-
if (typeof toolCall.name === "string" && toolCall.name.startsWith("mcp_")) {
559+
// Check if this is a dynamic MCP tool (mcp--serverName--toolName)
560+
const mcpPrefix = MCP_TOOL_PREFIX + MCP_TOOL_SEPARATOR
561+
if (typeof toolCall.name === "string" && toolCall.name.startsWith(mcpPrefix)) {
559562
return this.parseDynamicMcpTool(toolCall)
560563
}
561564

@@ -811,7 +814,7 @@ export class NativeToolCallParser {
811814
}
812815

813816
/**
814-
* Parse dynamic MCP tools (named mcp_serverName_toolName).
817+
* Parse dynamic MCP tools (named mcp--serverName--toolName).
815818
* These are generated dynamically by getMcpServerTools() and are returned
816819
* as McpToolUse objects that preserve the original tool name.
817820
*
@@ -825,26 +828,19 @@ export class NativeToolCallParser {
825828
const args = JSON.parse(toolCall.arguments || "{}")
826829

827830
// Extract server_name and tool_name from the tool name itself
828-
// Format: mcp_serverName_toolName
829-
const nameParts = toolCall.name.split("_")
830-
if (nameParts.length < 3 || nameParts[0] !== "mcp") {
831+
// Format: mcp--serverName--toolName (using -- separator)
832+
const parsed = parseMcpToolName(toolCall.name)
833+
if (!parsed) {
831834
console.error(`Invalid dynamic MCP tool name format: ${toolCall.name}`)
832835
return null
833836
}
834837

835-
// Server name is the second part, tool name is everything after
836-
const serverName = nameParts[1]
837-
const toolName = nameParts.slice(2).join("_")
838-
839-
if (!serverName || !toolName) {
840-
console.error(`Could not extract server_name or tool_name from: ${toolCall.name}`)
841-
return null
842-
}
838+
const { serverName, toolName } = parsed
843839

844840
const result: McpToolUse = {
845841
type: "mcp_tool_use" as const,
846842
id: toolCall.id,
847-
// Keep the original tool name (e.g., "mcp_serverName_toolName") for API history
843+
// Keep the original tool name (e.g., "mcp--serverName--toolName") for API history
848844
name: toolCall.name,
849845
serverName,
850846
toolName,

src/core/assistant-message/presentAssistantMessage.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -241,20 +241,32 @@ export async function presentAssistantMessage(cline: Task) {
241241
TelemetryService.instance.captureToolUsage(cline.taskId, "use_mcp_tool", toolProtocol)
242242
}
243243

244+
// Resolve sanitized server name back to original server name
245+
// The serverName from parsing is sanitized (e.g., "my_server" from "my server")
246+
// We need the original name to find the actual MCP connection
247+
const mcpHub = cline.providerRef.deref()?.getMcpHub()
248+
let resolvedServerName = mcpBlock.serverName
249+
if (mcpHub) {
250+
const originalName = mcpHub.findServerNameBySanitizedName(mcpBlock.serverName)
251+
if (originalName) {
252+
resolvedServerName = originalName
253+
}
254+
}
255+
244256
// Execute the MCP tool using the same handler as use_mcp_tool
245257
// Create a synthetic ToolUse block that the useMcpToolTool can handle
246258
const syntheticToolUse: ToolUse<"use_mcp_tool"> = {
247259
type: "tool_use",
248260
id: mcpBlock.id,
249261
name: "use_mcp_tool",
250262
params: {
251-
server_name: mcpBlock.serverName,
263+
server_name: resolvedServerName,
252264
tool_name: mcpBlock.toolName,
253265
arguments: JSON.stringify(mcpBlock.arguments),
254266
},
255267
partial: mcpBlock.partial,
256268
nativeArgs: {
257-
server_name: mcpBlock.serverName,
269+
server_name: resolvedServerName,
258270
tool_name: mcpBlock.toolName,
259271
arguments: mcpBlock.arguments,
260272
},

src/core/prompts/tools/native-tools/mcp_server.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type OpenAI from "openai"
22
import { McpHub } from "../../../../services/mcp/McpHub"
3+
import { buildMcpToolName } from "../../../../utils/mcp-name"
34

45
/**
56
* Dynamically generates native tool definitions for all enabled tools across connected MCP servers.
@@ -43,11 +44,14 @@ export function getMcpServerTools(mcpHub?: McpHub): OpenAI.Chat.ChatCompletionTo
4344
parameters.required = toolInputRequired
4445
}
4546

46-
// Use mcp_ prefix to identify dynamic MCP tools
47+
// Build sanitized tool name for API compliance
48+
// The name is sanitized to conform to API requirements (e.g., Gemini's function name restrictions)
49+
const toolName = buildMcpToolName(server.name, tool.name)
50+
4751
const toolDefinition: OpenAI.Chat.ChatCompletionTool = {
4852
type: "function",
4953
function: {
50-
name: `mcp_${server.name}_${tool.name}`,
54+
name: toolName,
5155
description: tool.description,
5256
parameters: parameters,
5357
},

src/services/mcp/McpHub.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import { fileExistsAtPath } from "../../utils/fs"
3333
import { arePathsEqual, getWorkspacePath } from "../../utils/path"
3434
import { injectVariables } from "../../utils/config"
3535
import { safeWriteJson } from "../../utils/safeWriteJson"
36+
import { sanitizeMcpName } from "../../utils/mcp-name"
3637

3738
// Discriminated union for connection states
3839
export type ConnectedMcpConnection = {
@@ -154,6 +155,7 @@ export class McpHub {
154155
private configChangeDebounceTimers: Map<string, NodeJS.Timeout> = new Map()
155156
private isProgrammaticUpdate: boolean = false
156157
private flagResetTimer?: NodeJS.Timeout
158+
private sanitizedNameRegistry: Map<string, string> = new Map()
157159

158160
constructor(provider: ClineProvider) {
159161
this.providerRef = new WeakRef(provider)
@@ -627,6 +629,10 @@ export class McpHub {
627629
// Remove existing connection if it exists with the same source
628630
await this.deleteConnection(name, source)
629631

632+
// Register the sanitized name for O(1) lookup
633+
const sanitizedName = sanitizeMcpName(name)
634+
this.sanitizedNameRegistry.set(sanitizedName, name)
635+
630636
// Check if MCP is globally enabled
631637
const mcpEnabled = await this.isMcpEnabled()
632638
if (!mcpEnabled) {
@@ -910,6 +916,22 @@ export class McpHub {
910916
)
911917
}
912918

919+
/**
920+
* Find a connection by sanitized server name.
921+
* This is used when parsing MCP tool responses where the server name has been
922+
* sanitized (e.g., hyphens replaced with underscores) for API compliance.
923+
* @param sanitizedServerName The sanitized server name from the API tool call
924+
* @returns The original server name if found, or null if no match
925+
*/
926+
public findServerNameBySanitizedName(sanitizedServerName: string): string | null {
927+
const exactMatch = this.connections.find((conn) => conn.server.name === sanitizedServerName)
928+
if (exactMatch) {
929+
return exactMatch.server.name
930+
}
931+
932+
return this.sanitizedNameRegistry.get(sanitizedServerName) ?? null
933+
}
934+
913935
private async fetchToolsList(serverName: string, source?: "global" | "project"): Promise<McpTool[]> {
914936
try {
915937
// Use the helper method to find the connection
@@ -1027,6 +1049,13 @@ export class McpHub {
10271049
if (source && conn.server.source !== source) return true
10281050
return false
10291051
})
1052+
1053+
// Remove from sanitized name registry if no more connections with this name exist
1054+
const remainingConnections = this.connections.filter((conn) => conn.server.name === name)
1055+
if (remainingConnections.length === 0) {
1056+
const sanitizedName = sanitizeMcpName(name)
1057+
this.sanitizedNameRegistry.delete(sanitizedName)
1058+
}
10301059
}
10311060

10321061
async updateServerConnections(
Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
import { sanitizeMcpName, buildMcpToolName, parseMcpToolName, MCP_TOOL_SEPARATOR, MCP_TOOL_PREFIX } from "../mcp-name"
2+
3+
describe("mcp-name utilities", () => {
4+
describe("constants", () => {
5+
it("should have correct separator and prefix", () => {
6+
expect(MCP_TOOL_SEPARATOR).toBe("--")
7+
expect(MCP_TOOL_PREFIX).toBe("mcp")
8+
})
9+
})
10+
11+
describe("sanitizeMcpName", () => {
12+
it("should return underscore placeholder for empty input", () => {
13+
expect(sanitizeMcpName("")).toBe("_")
14+
})
15+
16+
it("should replace spaces with underscores", () => {
17+
expect(sanitizeMcpName("my server")).toBe("my_server")
18+
expect(sanitizeMcpName("server name here")).toBe("server_name_here")
19+
})
20+
21+
it("should remove invalid characters", () => {
22+
expect(sanitizeMcpName("server@name!")).toBe("servername")
23+
expect(sanitizeMcpName("test#$%^&*()")).toBe("test")
24+
})
25+
26+
it("should keep valid characters (alphanumeric, underscore, dot, colon, dash)", () => {
27+
expect(sanitizeMcpName("server_name")).toBe("server_name")
28+
expect(sanitizeMcpName("server.name")).toBe("server.name")
29+
expect(sanitizeMcpName("server:name")).toBe("server:name")
30+
expect(sanitizeMcpName("server-name")).toBe("server-name")
31+
expect(sanitizeMcpName("Server123")).toBe("Server123")
32+
})
33+
34+
it("should prepend underscore if name starts with non-letter/underscore", () => {
35+
expect(sanitizeMcpName("123server")).toBe("_123server")
36+
expect(sanitizeMcpName("-server")).toBe("_-server")
37+
expect(sanitizeMcpName(".server")).toBe("_.server")
38+
})
39+
40+
it("should not modify names that start with letter or underscore", () => {
41+
expect(sanitizeMcpName("server")).toBe("server")
42+
expect(sanitizeMcpName("_server")).toBe("_server")
43+
expect(sanitizeMcpName("Server")).toBe("Server")
44+
})
45+
46+
it("should replace double-hyphen sequences with single hyphen to avoid separator conflicts", () => {
47+
expect(sanitizeMcpName("server--name")).toBe("server-name")
48+
expect(sanitizeMcpName("test---server")).toBe("test-server")
49+
expect(sanitizeMcpName("my----tool")).toBe("my-tool")
50+
})
51+
52+
it("should handle complex names with multiple issues", () => {
53+
expect(sanitizeMcpName("My Server @ Home!")).toBe("My_Server__Home")
54+
expect(sanitizeMcpName("123-test server")).toBe("_123-test_server")
55+
})
56+
57+
it("should return placeholder for names that become empty after sanitization", () => {
58+
expect(sanitizeMcpName("@#$%")).toBe("_unnamed")
59+
// Spaces become underscores, which is a valid character, so it returns "_"
60+
expect(sanitizeMcpName(" ")).toBe("_")
61+
})
62+
})
63+
64+
describe("buildMcpToolName", () => {
65+
it("should build tool name with mcp-- prefix and -- separators", () => {
66+
expect(buildMcpToolName("server", "tool")).toBe("mcp--server--tool")
67+
})
68+
69+
it("should sanitize both server and tool names", () => {
70+
expect(buildMcpToolName("my server", "my tool")).toBe("mcp--my_server--my_tool")
71+
})
72+
73+
it("should handle names with special characters", () => {
74+
expect(buildMcpToolName("server@name", "tool!name")).toBe("mcp--servername--toolname")
75+
})
76+
77+
it("should truncate long names to 64 characters", () => {
78+
const longServer = "a".repeat(50)
79+
const longTool = "b".repeat(50)
80+
const result = buildMcpToolName(longServer, longTool)
81+
expect(result.length).toBeLessThanOrEqual(64)
82+
expect(result.startsWith("mcp--")).toBe(true)
83+
})
84+
85+
it("should handle names starting with numbers", () => {
86+
expect(buildMcpToolName("123server", "456tool")).toBe("mcp--_123server--_456tool")
87+
})
88+
89+
it("should preserve underscores in server and tool names", () => {
90+
expect(buildMcpToolName("my_server", "my_tool")).toBe("mcp--my_server--my_tool")
91+
})
92+
})
93+
94+
describe("parseMcpToolName", () => {
95+
it("should parse valid mcp tool names", () => {
96+
expect(parseMcpToolName("mcp--server--tool")).toEqual({
97+
serverName: "server",
98+
toolName: "tool",
99+
})
100+
})
101+
102+
it("should return null for non-mcp tool names", () => {
103+
expect(parseMcpToolName("server--tool")).toBeNull()
104+
expect(parseMcpToolName("tool")).toBeNull()
105+
})
106+
107+
it("should return null for old underscore format", () => {
108+
expect(parseMcpToolName("mcp_server_tool")).toBeNull()
109+
})
110+
111+
it("should handle tool names with underscores", () => {
112+
expect(parseMcpToolName("mcp--server--tool_name")).toEqual({
113+
serverName: "server",
114+
toolName: "tool_name",
115+
})
116+
})
117+
118+
it("should correctly handle server names with underscores (fixed from old behavior)", () => {
119+
// With the new -- separator, server names with underscores work correctly
120+
expect(parseMcpToolName("mcp--my_server--tool")).toEqual({
121+
serverName: "my_server",
122+
toolName: "tool",
123+
})
124+
})
125+
126+
it("should handle both server and tool names with underscores", () => {
127+
expect(parseMcpToolName("mcp--my_server--get_forecast")).toEqual({
128+
serverName: "my_server",
129+
toolName: "get_forecast",
130+
})
131+
})
132+
133+
it("should return null for malformed names", () => {
134+
expect(parseMcpToolName("mcp--")).toBeNull()
135+
expect(parseMcpToolName("mcp--server")).toBeNull()
136+
})
137+
})
138+
139+
describe("roundtrip behavior", () => {
140+
it("should be able to parse names that were built", () => {
141+
const toolName = buildMcpToolName("server", "tool")
142+
const parsed = parseMcpToolName(toolName)
143+
expect(parsed).toEqual({
144+
serverName: "server",
145+
toolName: "tool",
146+
})
147+
})
148+
149+
it("should preserve sanitized names through roundtrip with underscores", () => {
150+
// Names with underscores now work correctly through roundtrip
151+
const toolName = buildMcpToolName("my_server", "my_tool")
152+
const parsed = parseMcpToolName(toolName)
153+
expect(parsed).toEqual({
154+
serverName: "my_server",
155+
toolName: "my_tool",
156+
})
157+
})
158+
159+
it("should handle spaces that get converted to underscores", () => {
160+
// "my server" becomes "my_server" after sanitization
161+
const toolName = buildMcpToolName("my server", "get tool")
162+
const parsed = parseMcpToolName(toolName)
163+
expect(parsed).toEqual({
164+
serverName: "my_server",
165+
toolName: "get_tool",
166+
})
167+
})
168+
169+
it("should handle complex server and tool names", () => {
170+
const toolName = buildMcpToolName("Weather API", "get_current_forecast")
171+
const parsed = parseMcpToolName(toolName)
172+
expect(parsed).toEqual({
173+
serverName: "Weather_API",
174+
toolName: "get_current_forecast",
175+
})
176+
})
177+
})
178+
})

0 commit comments

Comments
 (0)