Skip to content

Commit ea92c10

Browse files
committed
Merge pull request #694 from chrisnestrud/fix/mcp-tool-name-sanitization
2 parents 7ecceea + baaecd2 commit ea92c10

4 files changed

Lines changed: 160 additions & 4 deletions

File tree

‎.gitignore‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,10 @@ __pycache__/
4747
cecli/website/_site/*
4848
cecli/website/.sass-cache/*
4949
cecli/website/.docmd-*/*
50-
cecli/website/node_modules/*
50+
cecli/website/node_modules/*
51+
52+
# Beads / Dolt files (added by bd init)
53+
.dolt/
54+
*.db
55+
.beads-credential-key
56+
.beads/proxieddb/

‎cecli/coders/base_coder.py‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3187,7 +3187,8 @@ def _find_mcp_server_for_tool(self, tool_call):
31873187
tool_name_from_schema = nested.getter(tool, "function.name")
31883188
if (
31893189
tool_name_from_schema
3190-
and tool_name_from_schema.lower() == unprefixed_tool_name.lower()
3190+
and responses.sanitize_tool_name(tool_name_from_schema).lower()
3191+
== responses.sanitize_tool_name(unprefixed_tool_name).lower()
31913192
):
31923193
# Find the McpServer instance that will be used for communication
31933194
for server in self.mcp_manager:
@@ -3409,6 +3410,10 @@ async def call_mcp_tool_from_session(self, session, tool_call):
34093410
# with a "missing required parameter" error.
34103411
arguments = responses.coerce_tool_structure(arguments)
34113412

3413+
# Providers require a provider-safe tool name, so the name coming back
3414+
# from the model may not be the name the MCP server advertises.
3415+
name = responses.original_tool_name(name, self.mcp_tools)
3416+
34123417
return await session.call_tool(name=name, arguments=arguments)
34133418

34143419
async def process_tool_calls(self, tool_call_response):

‎cecli/helpers/responses.py‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,32 @@ def extract_tools_from_pseudo_json(content: str) -> Optional[List[ChatCompletion
247247
return None
248248

249249

250+
def sanitize_tool_name(name: str) -> str:
251+
"""Make a name acceptable in OpenAI-style ``tools[].function.name``.
252+
253+
Providers enforce ``^[A-Za-z0-9_-]{1,64}$`` on the function name, which
254+
rejects names an MCP server may legitimately advertise (``browser.fetch``).
255+
Idempotent, so it is also safe on an incoming tool call before the name is
256+
compared against, or mapped back to, the server's own name.
257+
"""
258+
return re.sub(r"[^A-Za-z0-9_-]", "_", name)[:64]
259+
260+
261+
def original_tool_name(name: str, server_tools) -> str:
262+
"""Map a sanitized tool name back to the name the MCP server advertises.
263+
264+
``server_tools`` is an iterable of ``(server_name, tools)`` pairs. Returns
265+
``name`` unchanged when nothing matches, so an unsanitized name still
266+
reaches the server as-is.
267+
"""
268+
for _server_name, tools in server_tools or []:
269+
for tool in tools:
270+
candidate = nested.getter(tool, "function.name", "")
271+
if candidate and sanitize_tool_name(candidate) == name:
272+
return candidate
273+
return name
274+
275+
250276
def prefix_tool_name(server_name: str, tool_name: str) -> str:
251277
"""
252278
Prefix a tool name with the server name.
@@ -256,9 +282,9 @@ def prefix_tool_name(server_name: str, tool_name: str) -> str:
256282
tool_name: Original tool name
257283
258284
Returns:
259-
Prefixed tool name in format "{server_name}--{tool_name}"
285+
Prefixed, provider-safe tool name in format "{server_name}--{tool_name}"
260286
"""
261-
return f"{server_name}--{tool_name}"
287+
return sanitize_tool_name(f"{server_name}--{tool_name}")
262288

263289

264290
def unprefix_tool_name(prefixed_name: str) -> tuple[str, str]:
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
"""Tests for provider-safe MCP tool names.
2+
3+
Providers enforce ``^[A-Za-z0-9_-]{1,64}$`` on ``tools[].function.name``, so a
4+
dotted MCP name such as ``browser.fetch`` is sanitized on the way to the model
5+
and mapped back to the server's own name when the call comes back.
6+
"""
7+
8+
from types import SimpleNamespace
9+
from unittest.mock import AsyncMock, MagicMock
10+
11+
import pytest
12+
13+
from cecli.coders import Coder
14+
from cecli.helpers import responses
15+
16+
17+
def _tool(name):
18+
"""Build a minimal OpenAI-style function tool dict."""
19+
return {
20+
"type": "function",
21+
"function": {"name": name, "description": "", "parameters": {}},
22+
}
23+
24+
25+
def _call(name):
26+
return {
27+
"id": "call-1",
28+
"type": "function",
29+
"function": {"name": name, "arguments": "{}"},
30+
}
31+
32+
33+
class TestSanitizeToolName:
34+
def test_dots_become_underscores(self):
35+
assert responses.sanitize_tool_name("browser.fetch") == "browser_fetch"
36+
37+
def test_is_idempotent(self):
38+
once = responses.sanitize_tool_name("browser.persisted_session.list")
39+
40+
assert responses.sanitize_tool_name(once) == once
41+
42+
def test_honors_provider_length_limit(self):
43+
assert len(responses.sanitize_tool_name("a" * 200)) == 64
44+
45+
46+
class TestGetToolListSanitizesNames:
47+
def _coder(self, tools):
48+
return SimpleNamespace(
49+
mcp_tools=[("browser", tools)],
50+
registered_servers={"included": set(), "excluded": set()},
51+
registered_tools={"included": set(), "excluded": set()},
52+
)
53+
54+
def test_dotted_names_are_sanitized_for_the_model(self):
55+
coder = self._coder([_tool("browser.fetch"), _tool("browser.persisted_session.list")])
56+
57+
names = [t["function"]["name"] for t in Coder.get_tool_list(coder)]
58+
59+
assert names == [
60+
"browser--browser_fetch",
61+
"browser--browser_persisted_session_list",
62+
]
63+
64+
def test_underscore_name_is_unchanged(self):
65+
coder = self._coder([_tool("brave_web_search")])
66+
67+
names = [t["function"]["name"] for t in Coder.get_tool_list(coder)]
68+
69+
assert names == ["browser--brave_web_search"]
70+
71+
72+
class TestFindMcpServerForSanitizedCall:
73+
def test_sanitized_call_resolves_to_its_server(self):
74+
server = SimpleNamespace(name="browser")
75+
coder = SimpleNamespace(
76+
mcp_tools=[("browser", [_tool("browser.fetch")])],
77+
mcp_manager=[server],
78+
)
79+
tool_call = SimpleNamespace(
80+
id="call-1",
81+
type="function",
82+
function=SimpleNamespace(name="browser--browser_fetch"),
83+
)
84+
85+
assert Coder._find_mcp_server_for_tool(coder, tool_call) is server
86+
87+
88+
class TestCallToolMapsBackToAdvertisedName:
89+
"""The name the model calls must reach the server unsanitized."""
90+
91+
@pytest.mark.asyncio
92+
async def test_sanitized_name_is_mapped_back(self):
93+
coder = SimpleNamespace(mcp_tools=[("browser", [_tool("browser.fetch")])])
94+
session = MagicMock()
95+
session.call_tool = AsyncMock(return_value="ok")
96+
97+
await Coder.call_mcp_tool_from_session(coder, session, _call("browser_fetch"))
98+
99+
assert session.call_tool.await_args.kwargs["name"] == "browser.fetch"
100+
101+
@pytest.mark.asyncio
102+
async def test_unsanitized_name_passes_through(self):
103+
coder = SimpleNamespace(mcp_tools=[("browser", [_tool("browser.fetch")])])
104+
session = MagicMock()
105+
session.call_tool = AsyncMock(return_value="ok")
106+
107+
await Coder.call_mcp_tool_from_session(coder, session, _call("browser.fetch"))
108+
109+
assert session.call_tool.await_args.kwargs["name"] == "browser.fetch"
110+
111+
@pytest.mark.asyncio
112+
async def test_unknown_name_is_not_rewritten(self):
113+
coder = SimpleNamespace(mcp_tools=[("browser", [_tool("browser.fetch")])])
114+
session = MagicMock()
115+
session.call_tool = AsyncMock(return_value="ok")
116+
117+
await Coder.call_mcp_tool_from_session(coder, session, _call("read_file"))
118+
119+
assert session.call_tool.await_args.kwargs["name"] == "read_file"

0 commit comments

Comments
 (0)