diff --git a/.gitignore b/.gitignore index 22b4725523a..03060404f8c 100644 --- a/.gitignore +++ b/.gitignore @@ -47,4 +47,10 @@ __pycache__/ cecli/website/_site/* cecli/website/.sass-cache/* cecli/website/.docmd-*/* -cecli/website/node_modules/* \ No newline at end of file +cecli/website/node_modules/* + +# Beads / Dolt files (added by bd init) +.dolt/ +*.db +.beads-credential-key +.beads/proxieddb/ diff --git a/cecli/coders/base_coder.py b/cecli/coders/base_coder.py index cc62c69493e..1851639471f 100755 --- a/cecli/coders/base_coder.py +++ b/cecli/coders/base_coder.py @@ -3193,7 +3193,8 @@ def _find_mcp_server_for_tool(self, tool_call): tool_name_from_schema = nested.getter(tool, "function.name") if ( tool_name_from_schema - and tool_name_from_schema.lower() == unprefixed_tool_name.lower() + and responses.sanitize_tool_name(tool_name_from_schema).lower() + == responses.sanitize_tool_name(unprefixed_tool_name).lower() ): # Find the McpServer instance that will be used for communication for server in self.mcp_manager: @@ -3415,6 +3416,10 @@ async def call_mcp_tool_from_session(self, session, tool_call): # with a "missing required parameter" error. arguments = responses.coerce_tool_structure(arguments) + # Providers require a provider-safe tool name, so the name coming back + # from the model may not be the name the MCP server advertises. + name = responses.original_tool_name(name, self.mcp_tools) + return await session.call_tool(name=name, arguments=arguments) async def process_tool_calls(self, tool_call_response): diff --git a/cecli/helpers/responses.py b/cecli/helpers/responses.py index ba1155a7ad9..41f0e921e8a 100644 --- a/cecli/helpers/responses.py +++ b/cecli/helpers/responses.py @@ -247,6 +247,32 @@ def extract_tools_from_pseudo_json(content: str) -> Optional[List[ChatCompletion return None +def sanitize_tool_name(name: str) -> str: + """Make a name acceptable in OpenAI-style ``tools[].function.name``. + + Providers enforce ``^[A-Za-z0-9_-]{1,64}$`` on the function name, which + rejects names an MCP server may legitimately advertise (``browser.fetch``). + Idempotent, so it is also safe on an incoming tool call before the name is + compared against, or mapped back to, the server's own name. + """ + return re.sub(r"[^A-Za-z0-9_-]", "_", name)[:64] + + +def original_tool_name(name: str, server_tools) -> str: + """Map a sanitized tool name back to the name the MCP server advertises. + + ``server_tools`` is an iterable of ``(server_name, tools)`` pairs. Returns + ``name`` unchanged when nothing matches, so an unsanitized name still + reaches the server as-is. + """ + for _server_name, tools in server_tools or []: + for tool in tools: + candidate = nested.getter(tool, "function.name", "") + if candidate and sanitize_tool_name(candidate) == name: + return candidate + return name + + def prefix_tool_name(server_name: str, tool_name: str) -> str: """ Prefix a tool name with the server name. @@ -256,9 +282,9 @@ def prefix_tool_name(server_name: str, tool_name: str) -> str: tool_name: Original tool name Returns: - Prefixed tool name in format "{server_name}--{tool_name}" + Prefixed, provider-safe tool name in format "{server_name}--{tool_name}" """ - return f"{server_name}--{tool_name}" + return sanitize_tool_name(f"{server_name}--{tool_name}") def unprefix_tool_name(prefixed_name: str) -> tuple[str, str]: diff --git a/tests/mcp/test_tool_name_sanitization.py b/tests/mcp/test_tool_name_sanitization.py new file mode 100644 index 00000000000..4c702ffce10 --- /dev/null +++ b/tests/mcp/test_tool_name_sanitization.py @@ -0,0 +1,119 @@ +"""Tests for provider-safe MCP tool names. + +Providers enforce ``^[A-Za-z0-9_-]{1,64}$`` on ``tools[].function.name``, so a +dotted MCP name such as ``browser.fetch`` is sanitized on the way to the model +and mapped back to the server's own name when the call comes back. +""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from cecli.coders import Coder +from cecli.helpers import responses + + +def _tool(name): + """Build a minimal OpenAI-style function tool dict.""" + return { + "type": "function", + "function": {"name": name, "description": "", "parameters": {}}, + } + + +def _call(name): + return { + "id": "call-1", + "type": "function", + "function": {"name": name, "arguments": "{}"}, + } + + +class TestSanitizeToolName: + def test_dots_become_underscores(self): + assert responses.sanitize_tool_name("browser.fetch") == "browser_fetch" + + def test_is_idempotent(self): + once = responses.sanitize_tool_name("browser.persisted_session.list") + + assert responses.sanitize_tool_name(once) == once + + def test_honors_provider_length_limit(self): + assert len(responses.sanitize_tool_name("a" * 200)) == 64 + + +class TestGetToolListSanitizesNames: + def _coder(self, tools): + return SimpleNamespace( + mcp_tools=[("browser", tools)], + registered_servers={"included": set(), "excluded": set()}, + registered_tools={"included": set(), "excluded": set()}, + ) + + def test_dotted_names_are_sanitized_for_the_model(self): + coder = self._coder([_tool("browser.fetch"), _tool("browser.persisted_session.list")]) + + names = [t["function"]["name"] for t in Coder.get_tool_list(coder)] + + assert names == [ + "browser--browser_fetch", + "browser--browser_persisted_session_list", + ] + + def test_underscore_name_is_unchanged(self): + coder = self._coder([_tool("brave_web_search")]) + + names = [t["function"]["name"] for t in Coder.get_tool_list(coder)] + + assert names == ["browser--brave_web_search"] + + +class TestFindMcpServerForSanitizedCall: + def test_sanitized_call_resolves_to_its_server(self): + server = SimpleNamespace(name="browser") + coder = SimpleNamespace( + mcp_tools=[("browser", [_tool("browser.fetch")])], + mcp_manager=[server], + ) + tool_call = SimpleNamespace( + id="call-1", + type="function", + function=SimpleNamespace(name="browser--browser_fetch"), + ) + + assert Coder._find_mcp_server_for_tool(coder, tool_call) is server + + +class TestCallToolMapsBackToAdvertisedName: + """The name the model calls must reach the server unsanitized.""" + + @pytest.mark.asyncio + async def test_sanitized_name_is_mapped_back(self): + coder = SimpleNamespace(mcp_tools=[("browser", [_tool("browser.fetch")])]) + session = MagicMock() + session.call_tool = AsyncMock(return_value="ok") + + await Coder.call_mcp_tool_from_session(coder, session, _call("browser_fetch")) + + assert session.call_tool.await_args.kwargs["name"] == "browser.fetch" + + @pytest.mark.asyncio + async def test_unsanitized_name_passes_through(self): + coder = SimpleNamespace(mcp_tools=[("browser", [_tool("browser.fetch")])]) + session = MagicMock() + session.call_tool = AsyncMock(return_value="ok") + + await Coder.call_mcp_tool_from_session(coder, session, _call("browser.fetch")) + + assert session.call_tool.await_args.kwargs["name"] == "browser.fetch" + + @pytest.mark.asyncio + async def test_unknown_name_is_not_rewritten(self): + coder = SimpleNamespace(mcp_tools=[("browser", [_tool("browser.fetch")])]) + session = MagicMock() + session.call_tool = AsyncMock(return_value="ok") + + await Coder.call_mcp_tool_from_session(coder, session, _call("read_file")) + + assert session.call_tool.await_args.kwargs["name"] == "read_file"