diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index 1d1d29bd7..823db8843 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -2750,3 +2750,153 @@ class TestMCPSelectiveToolLoading: assert connect_called == [] assert result == [] + + +# --------------------------------------------------------------------------- +# Tool name collision protection +# --------------------------------------------------------------------------- + +class TestRegistryCollisionWarning: + """registry.register() warns when a tool name is overwritten by a different toolset.""" + + def test_overwrite_different_toolset_logs_warning(self, caplog): + """Overwriting a tool from a different toolset emits a warning.""" + from tools.registry import ToolRegistry + import logging + + reg = ToolRegistry() + schema = {"name": "my_tool", "description": "test", "parameters": {"type": "object", "properties": {}}} + handler = lambda args, **kw: "{}" + + reg.register(name="my_tool", toolset="builtin", schema=schema, handler=handler) + + with caplog.at_level(logging.WARNING, logger="tools.registry"): + reg.register(name="my_tool", toolset="mcp-ext", schema=schema, handler=handler) + + assert any("collision" in r.message.lower() for r in caplog.records) + assert any("builtin" in r.message and "mcp-ext" in r.message for r in caplog.records) + + def test_overwrite_same_toolset_no_warning(self, caplog): + """Re-registering within the same toolset is silent (e.g. reconnect).""" + from tools.registry import ToolRegistry + import logging + + reg = ToolRegistry() + schema = {"name": "my_tool", "description": "test", "parameters": {"type": "object", "properties": {}}} + handler = lambda args, **kw: "{}" + + reg.register(name="my_tool", toolset="mcp-server", schema=schema, handler=handler) + + with caplog.at_level(logging.WARNING, logger="tools.registry"): + reg.register(name="my_tool", toolset="mcp-server", schema=schema, handler=handler) + + assert not any("collision" in r.message.lower() for r in caplog.records) + + +class TestMCPBuiltinCollisionGuard: + """MCP tools that collide with built-in tool names are skipped.""" + + def test_mcp_tool_skipped_when_builtin_exists(self): + """An MCP tool whose prefixed name collides with a built-in is skipped.""" + from tools.registry import ToolRegistry + from tools.mcp_tool import _discover_and_register_server, _servers, MCPServerTask + + mock_registry = ToolRegistry() + + # Pre-register a "built-in" tool with the name that the MCP tool would produce. + # Server "abc", tool "search" → mcp_abc_search + builtin_schema = { + "name": "mcp_abc_search", + "description": "A hypothetical built-in", + "parameters": {"type": "object", "properties": {}}, + } + mock_registry.register( + name="mcp_abc_search", toolset="web", + schema=builtin_schema, handler=lambda a, **k: "{}", + ) + + mock_tools = [_make_mcp_tool("search", "Search the web")] + mock_session = MagicMock() + + async def fake_connect(name, config): + server = MCPServerTask(name) + server.session = mock_session + server._tools = mock_tools + return server + + with patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \ + patch("tools.registry.registry", mock_registry): + registered = asyncio.run( + _discover_and_register_server("abc", {"command": "test", "args": []}) + ) + + # The MCP tool should have been skipped — built-in preserved. + assert "mcp_abc_search" not in registered + assert mock_registry.get_toolset_for_tool("mcp_abc_search") == "web" + + _servers.pop("abc", None) + + def test_mcp_tool_registered_when_no_builtin_collision(self): + """MCP tools register normally when there's no collision.""" + from tools.registry import ToolRegistry + from tools.mcp_tool import _discover_and_register_server, _servers, MCPServerTask + + mock_registry = ToolRegistry() + mock_tools = [_make_mcp_tool("web_search", "Search the web")] + mock_session = MagicMock() + + async def fake_connect(name, config): + server = MCPServerTask(name) + server.session = mock_session + server._tools = mock_tools + return server + + with patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \ + patch("tools.registry.registry", mock_registry): + registered = asyncio.run( + _discover_and_register_server("minimax", {"command": "test", "args": []}) + ) + + assert "mcp_minimax_web_search" in registered + assert mock_registry.get_toolset_for_tool("mcp_minimax_web_search") == "mcp-minimax" + + _servers.pop("minimax", None) + + def test_mcp_tool_allowed_when_collision_is_another_mcp(self): + """Collision between two MCP toolsets is allowed (last wins).""" + from tools.registry import ToolRegistry + from tools.mcp_tool import _discover_and_register_server, _servers, MCPServerTask + + mock_registry = ToolRegistry() + + # Pre-register an MCP tool from a different server. + mcp_schema = { + "name": "mcp_srv_do_thing", + "description": "From another MCP server", + "parameters": {"type": "object", "properties": {}}, + } + mock_registry.register( + name="mcp_srv_do_thing", toolset="mcp-old", + schema=mcp_schema, handler=lambda a, **k: "{}", + ) + + mock_tools = [_make_mcp_tool("do_thing", "Do a thing")] + mock_session = MagicMock() + + async def fake_connect(name, config): + server = MCPServerTask(name) + server.session = mock_session + server._tools = mock_tools + return server + + with patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \ + patch("tools.registry.registry", mock_registry): + registered = asyncio.run( + _discover_and_register_server("srv", {"command": "test", "args": []}) + ) + + # MCP-to-MCP collision is allowed — the new server wins. + assert "mcp_srv_do_thing" in registered + assert mock_registry.get_toolset_for_tool("mcp_srv_do_thing") == "mcp-srv" + + _servers.pop("srv", None) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 32da32476..f539586eb 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -1532,6 +1532,16 @@ async def _discover_and_register_server(name: str, config: dict) -> List[str]: schema = _convert_mcp_schema(name, mcp_tool) tool_name_prefixed = schema["name"] + # Guard against collisions with built-in (non-MCP) tools. + existing_toolset = registry.get_toolset_for_tool(tool_name_prefixed) + if existing_toolset and not existing_toolset.startswith("mcp-"): + logger.warning( + "MCP server '%s': tool '%s' (→ '%s') collides with built-in " + "tool in toolset '%s' — skipping to preserve built-in", + name, mcp_tool.name, tool_name_prefixed, existing_toolset, + ) + continue + registry.register( name=tool_name_prefixed, toolset=toolset_name, @@ -1556,9 +1566,20 @@ async def _discover_and_register_server(name: str, config: dict) -> List[str]: schema = entry["schema"] handler_key = entry["handler_key"] handler = _handler_factories[handler_key](name, server.tool_timeout) + util_name = schema["name"] + + # Same collision guard for utility tools. + existing_toolset = registry.get_toolset_for_tool(util_name) + if existing_toolset and not existing_toolset.startswith("mcp-"): + logger.warning( + "MCP server '%s': utility tool '%s' collides with built-in " + "tool in toolset '%s' — skipping to preserve built-in", + name, util_name, existing_toolset, + ) + continue registry.register( - name=schema["name"], + name=util_name, toolset=toolset_name, schema=schema, handler=handler, @@ -1566,7 +1587,7 @@ async def _discover_and_register_server(name: str, config: dict) -> List[str]: is_async=False, description=schema["description"], ) - registered_names.append(schema["name"]) + registered_names.append(util_name) server._registered_tool_names = list(registered_names) diff --git a/tools/registry.py b/tools/registry.py index b388761c9..c13d98502 100644 --- a/tools/registry.py +++ b/tools/registry.py @@ -66,6 +66,13 @@ class ToolRegistry: emoji: str = "", ): """Register a tool. Called at module-import time by each tool file.""" + existing = self._tools.get(name) + if existing and existing.toolset != toolset: + logger.warning( + "Tool name collision: '%s' (toolset '%s') is being " + "overwritten by toolset '%s'", + name, existing.toolset, toolset, + ) self._tools[name] = ToolEntry( name=name, toolset=toolset,