Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion agent/anthropic_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -2122,9 +2122,13 @@ def build_anthropic_kwargs(
block["text"] = text

# 3. Prefix tool names with mcp_ (Claude Code convention)
# Skip names that already begin with the marker — native MCP server
# tools (from mcp_servers: in config.yaml) are registered under their
# full mcp_<server>_<tool> name and would double-prefix otherwise,
# breaking round-trip registry lookup in normalize_response. GH-25255.
if anthropic_tools:
for tool in anthropic_tools:
if "name" in tool:
if "name" in tool and not tool["name"].startswith(_MCP_TOOL_PREFIX):
tool["name"] = _MCP_TOOL_PREFIX + tool["name"]

# 4. Prefix tool names in message history (tool_use and tool_result blocks)
Expand Down
12 changes: 11 additions & 1 deletion agent/transports/anthropic.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,17 @@ def normalize_response(self, response: Any, **kwargs) -> NormalizedResponse:
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_<server>_<tool>
# 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,
Expand Down
1 change: 1 addition & 0 deletions scripts/release.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@
"gonzes7@gmail.com": "aqilaziz", # PR #26406 salvage (preserve native audio outside Telegram)
"karthikeyann@users.noreply.github.com": "karthikeyann", # PR #26609 salvage (DM-topic routing pin)
"rino.alpin@gmail.com": "kunci115", # PR #27098 salvage (thread-not-found retry)
"hayka-pacha@users.noreply.github.com": "hayka-pacha", # PR #25270 salvage (registry-aware mcp_ prefix strip)
"237601532+chromalinx@users.noreply.github.com": "chromalinx", # PR #27014 salvage (commands for groups+DM)
"booker1207@gmail.com": "booker1207", # PR #25132 salvage (gate profile bots by allowed topics)
"kiranvk2011@gmail.com": "kiranvk-2011", # PR #24815 salvage (image documents → vision)
Expand Down
250 changes: 250 additions & 0 deletions tests/agent/test_anthropic_mcp_prefix_strip.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
"""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_<server>_<tool>``
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"


class TestAnthropicOAuthOutgoingPrefix:
"""Verify the outgoing-side companion fix: build_anthropic_kwargs must not
double-prefix tool names that already start with ``mcp_`` (native MCP server
tools registered as ``mcp_<server>_<tool>``). GH-25255."""

def _build(self, tools, is_oauth=True):
from agent.anthropic_adapter import build_anthropic_kwargs
return build_anthropic_kwargs(
model="claude-sonnet-4-6",
messages=[{"role": "user", "content": "Hi"}],
tools=tools,
max_tokens=4096,
reasoning_config=None,
is_oauth=is_oauth,
)

def test_oauth_adds_prefix_to_bare_tool_name(self):
"""OAuth + bare name → prefix added (existing Claude Code convention)."""
kwargs = self._build([{
"type": "function",
"function": {"name": "read_file", "description": "x", "parameters": {}},
}])
names = [t["name"] for t in kwargs["tools"]]
assert names == ["mcp_read_file"]

def test_oauth_does_not_double_prefix_native_mcp_tool(self):
"""OAuth + already-prefixed native MCP name → left alone."""
kwargs = self._build([{
"type": "function",
"function": {
"name": "mcp_composio_COMPOSIO_SEARCH_TOOLS",
"description": "x",
"parameters": {},
},
}])
names = [t["name"] for t in kwargs["tools"]]
# Must NOT become "mcp_mcp_composio_..." — that breaks the round-trip
# because normalize_response only strips ONE mcp_ prefix.
assert names == ["mcp_composio_COMPOSIO_SEARCH_TOOLS"]

def test_oauth_mixed_native_and_bare_tools(self):
"""Mixed: native MCP preserved, bare names prefixed."""
kwargs = self._build([
{"type": "function", "function": {"name": "read_file",
"description": "x", "parameters": {}}},
{"type": "function", "function": {"name": "mcp_composio_SEARCH",
"description": "y", "parameters": {}}},
{"type": "function", "function": {"name": "terminal",
"description": "z", "parameters": {}}},
])
names = sorted(t["name"] for t in kwargs["tools"])
assert names == ["mcp_composio_SEARCH", "mcp_read_file", "mcp_terminal"]

def test_non_oauth_path_untouched(self):
"""Non-OAuth requests never get the prefix — schemas pass through as-is."""
kwargs = self._build([
{"type": "function", "function": {"name": "read_file",
"description": "x", "parameters": {}}},
{"type": "function", "function": {"name": "mcp_composio_SEARCH",
"description": "y", "parameters": {}}},
], is_oauth=False)
names = sorted(t["name"] for t in kwargs["tools"])
assert names == ["mcp_composio_SEARCH", "read_file"]
Loading