diff --git a/agent/transports/anthropic.py b/agent/transports/anthropic.py index 72024ac20f3..d77ae63ef32 100644 --- a/agent/transports/anthropic.py +++ b/agent/transports/anthropic.py @@ -106,7 +106,17 @@ class AnthropicTransport(ProviderTransport): elif block.type == "tool_use": name = block.name if strip_tool_prefix and name.startswith(_MCP_PREFIX): - name = name[len(_MCP_PREFIX):] + stripped = name[len(_MCP_PREFIX):] + # Only strip the mcp_ prefix for OAuth-injected tools + # (where Hermes adds the prefix when sending to Anthropic + # and must remove it on the way back). Native MCP server + # tools (from mcp_servers: in config.yaml) are registered + # in the tool registry under their FULL mcp__ + # name and must NOT be stripped. GH-25255. + from tools.registry import registry as _tool_registry + if (_tool_registry.get_entry(stripped) + and not _tool_registry.get_entry(name)): + name = stripped tool_calls.append( ToolCall( id=block.id, diff --git a/tests/agent/test_anthropic_mcp_prefix_strip.py b/tests/agent/test_anthropic_mcp_prefix_strip.py new file mode 100644 index 00000000000..52e55561418 --- /dev/null +++ b/tests/agent/test_anthropic_mcp_prefix_strip.py @@ -0,0 +1,185 @@ +"""Tests for GH-25255: Anthropic OAuth mcp_ prefix stripping. + +When strip_tool_prefix=True (Anthropic OAuth path), the transport must only +strip the ``mcp_`` prefix from OAuth-injected tools, NOT from Hermes-native +MCP server tools that are registered under their full ``mcp__`` +name in the tool registry. +""" + +from __future__ import annotations + +import json +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_tool_use_block(name: str, block_id: str = "tc_1", input_data: dict | None = None): + """Create a fake Anthropic tool_use content block.""" + return SimpleNamespace( + type="tool_use", + id=block_id, + name=name, + input=input_data or {"query": "test"}, + ) + + +def _make_response(*blocks, stop_reason="end_turn"): + """Create a fake Anthropic Messages response.""" + return SimpleNamespace( + content=list(blocks), + stop_reason=stop_reason, + model="claude-sonnet-4", + usage=SimpleNamespace(input_tokens=100, output_tokens=50), + ) + + +class _FakeRegistry: + """Minimal fake tool registry for testing prefix stripping logic.""" + + def __init__(self, registered_names: set[str]): + self._names = registered_names + + def get_entry(self, name: str): + if name in self._names: + return SimpleNamespace(name=name) # truthy = tool exists + return None + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + +class TestAnthropicMcpPrefixStrip: + """Verify that strip_tool_prefix only strips OAuth-injected prefixes.""" + + def _get_transport(self): + from agent.transports.anthropic import AnthropicTransport + return AnthropicTransport() + + def test_strips_prefix_for_oauth_injected_tool(self): + """OAuth tools: mcp_read_file -> read_file (stripped). + + The tool was registered as 'read_file' in the registry. + Anthropic sees 'mcp_read_file' because Hermes adds the prefix. + On response, we must strip it back to 'read_file'. + """ + transport = self._get_transport() + block = _make_tool_use_block("mcp_read_file") + response = _make_response(block) + + registry = _FakeRegistry({"read_file", "terminal", "web_search"}) + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=True) + + assert len(result.tool_calls) == 1 + assert result.tool_calls[0].name == "read_file" + + def test_preserves_native_mcp_server_tool_name(self): + """Native MCP tools: mcp_composio_SEARCH -> mcp_composio_SEARCH (kept). + + The tool is registered with the full mcp_ prefix in the registry. + Stripping would break registry lookup. + """ + transport = self._get_transport() + block = _make_tool_use_block("mcp_composio_COMPOSIO_SEARCH_TOOLS") + response = _make_response(block) + + registry = _FakeRegistry({ + "mcp_composio_COMPOSIO_SEARCH_TOOLS", + "mcp_composio_COMPOSIO_GET_TOOL_SCHEMAS", + "read_file", + }) + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=True) + + assert len(result.tool_calls) == 1 + assert result.tool_calls[0].name == "mcp_composio_COMPOSIO_SEARCH_TOOLS" + + def test_no_strip_when_flag_false(self): + """When strip_tool_prefix=False, names are never modified.""" + transport = self._get_transport() + block = _make_tool_use_block("mcp_read_file") + response = _make_response(block) + + registry = _FakeRegistry({"read_file"}) + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=False) + + assert len(result.tool_calls) == 1 + assert result.tool_calls[0].name == "mcp_read_file" + + def test_no_strip_when_not_mcp_prefixed(self): + """Non-mcp_ names are untouched regardless of strip flag.""" + transport = self._get_transport() + block = _make_tool_use_block("web_search") + response = _make_response(block) + + registry = _FakeRegistry({"web_search"}) + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=True) + + assert len(result.tool_calls) == 1 + assert result.tool_calls[0].name == "web_search" + + def test_preserves_name_when_neither_in_registry(self): + """When neither stripped nor full name is in registry, keep full name. + + Safety fallback: if we can't determine the type, prefer the full name + since it's what the LLM was told about. + """ + transport = self._get_transport() + block = _make_tool_use_block("mcp_unknown_tool") + response = _make_response(block) + + registry = _FakeRegistry({"read_file"}) # neither name registered + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=True) + + assert len(result.tool_calls) == 1 + assert result.tool_calls[0].name == "mcp_unknown_tool" + + def test_mixed_tools_same_response(self): + """Both OAuth and native MCP tools in the same response.""" + transport = self._get_transport() + block1 = _make_tool_use_block("mcp_read_file", block_id="tc_1") + block2 = _make_tool_use_block("mcp_composio_SEARCH", block_id="tc_2") + block3 = _make_tool_use_block("mcp_composio_SEARCH", block_id="tc_3") # also registered natively + response = _make_response(block1, block2, block3) + + registry = _FakeRegistry({ + "read_file", # OAuth-injected + "mcp_composio_SEARCH", # native MCP + }) + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=True) + + assert len(result.tool_calls) == 3 + # OAuth tool: stripped + assert result.tool_calls[0].name == "read_file" + # Native MCP: preserved (both stripped and full are registered, full wins) + assert result.tool_calls[1].name == "mcp_composio_SEARCH" + assert result.tool_calls[2].name == "mcp_composio_SEARCH" + + def test_both_stripped_and_full_registered_prefers_full(self): + """Edge case: both 'foo' and 'mcp_foo' exist in registry. + + Keep 'mcp_foo' (the original name) since it's what the LLM requested. + """ + transport = self._get_transport() + block = _make_tool_use_block("mcp_foo") + response = _make_response(block) + + registry = _FakeRegistry({"foo", "mcp_foo"}) + with patch("tools.registry.registry", registry): + result = transport.normalize_response(response, strip_tool_prefix=True) + + assert len(result.tool_calls) == 1 + # Both exist — the condition `get_entry(stripped) and not get_entry(name)` + # is False because get_entry(name) IS truthy, so we keep the full name. + assert result.tool_calls[0].name == "mcp_foo"