mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-13 23:11:40 +00:00
fix(mcp_semantic_tool_filter): match canonical tools that arrive with
a client-side namespace prefix. `SemanticMCPToolFilter._get_tools_by_names` matched by exact equality between the canonical name stored in the router (`<server><MCP_TOOL_PREFIX_SEPARATOR><tool>`) and the name in the incoming `tools[]` list. MCP clients such as opencode wrap every tool name with their own additive alias prefix (`<client_alias>_<canonical>`), so the two never matched, the filter dropped every tool to zero, and the proxy forwarded `tools: []` with `tool_choice: auto` — which strict upstream providers reject with a 400. The fix adds anchored suffix matching with a separator check: the canonical must form the complete tail of the incoming name and be preceded by `_` or `-`. Exact matches still win over suffix matches, incoming tools are returned at most once, and the original tool object is passed through unchanged so the client-facing name survives for tool-call round-trips. Seven unit tests in a new TestGetToolsByNames class cover exact match, underscore- and dash-prefixed variants, non-separator-anchored suffixes (which must not match), exact-wins-over-prefixed precedence, deduplication when two canonicals suffix-match the same incoming tool, and ordering-follows-router-output. Fixes #26078
This commit is contained in:
parent
b8f7d61400
commit
981bc3614a
2 changed files with 196 additions and 10 deletions
|
|
@ -213,20 +213,69 @@ class SemanticMCPToolFilter:
|
|||
|
||||
return []
|
||||
|
||||
@staticmethod
|
||||
def _name_matches_canonical(client_name: str, canonical: str) -> bool:
|
||||
"""
|
||||
Return True if a client-side tool name refers to the given canonical
|
||||
MCP tool name.
|
||||
|
||||
MCP clients (e.g. opencode) commonly wrap the proxy's canonical tool
|
||||
name with an additive namespace prefix of their own
|
||||
(``<client_alias><sep><canonical>``). The prefix can use either a
|
||||
dash or an underscore as separator regardless of what
|
||||
``MCP_TOOL_PREFIX_SEPARATOR`` is set to on the proxy, because the
|
||||
client doesn't know the proxy's separator.
|
||||
|
||||
The match is anchored: ``canonical`` must form the complete suffix
|
||||
of ``client_name`` and be preceded by a separator character, so
|
||||
``rain_gear`` does not match canonical ``ear``.
|
||||
"""
|
||||
if client_name == canonical:
|
||||
return True
|
||||
if len(client_name) <= len(canonical):
|
||||
return False
|
||||
if not client_name.endswith(canonical):
|
||||
return False
|
||||
separator = client_name[-len(canonical) - 1]
|
||||
return separator in ("_", "-")
|
||||
|
||||
def _get_tools_by_names(
|
||||
self, tool_names: List[str], available_tools: List[Any]
|
||||
) -> List[Any]:
|
||||
"""Get tools from available_tools by their names, preserving order."""
|
||||
# Match tools from available_tools (preserves format - dict or MCPTool)
|
||||
matched_tools = []
|
||||
for tool in available_tools:
|
||||
tool_name, _ = self._extract_tool_info(tool)
|
||||
if tool_name in tool_names:
|
||||
matched_tools.append(tool)
|
||||
"""
|
||||
Get tools from available_tools by their names, preserving the
|
||||
semantic router's ordering.
|
||||
|
||||
# Reorder to match semantic router's ordering
|
||||
tool_map = {self._extract_tool_info(t)[0]: t for t in matched_tools}
|
||||
return [tool_map[name] for name in tool_names if name in tool_map]
|
||||
Matching is tolerant of client-side namespace prefixes: if an
|
||||
incoming tool arrived as ``<client_alias>_<canonical>`` while the
|
||||
router returned ``<canonical>`` (see
|
||||
``_name_matches_canonical``), that tool is still selected. The
|
||||
returned tool object is the original from ``available_tools``, so
|
||||
the client-facing name is preserved for tool-call round-trips.
|
||||
"""
|
||||
# Build an index of incoming tools by their client-facing name.
|
||||
# Exact matches win over suffix matches when both are present, and
|
||||
# each incoming tool is returned at most once even if two canonical
|
||||
# names happen to be tail-compatible with the same incoming name.
|
||||
available_by_name: Dict[str, Any] = {}
|
||||
for tool in available_tools:
|
||||
client_name, _ = self._extract_tool_info(tool)
|
||||
if client_name and client_name not in available_by_name:
|
||||
available_by_name[client_name] = tool
|
||||
|
||||
matched: List[Any] = []
|
||||
used_ids: set = set()
|
||||
for canonical in tool_names:
|
||||
tool = available_by_name.get(canonical)
|
||||
if tool is None:
|
||||
for client_name, candidate in available_by_name.items():
|
||||
if self._name_matches_canonical(client_name, canonical):
|
||||
tool = candidate
|
||||
break
|
||||
if tool is not None and id(tool) not in used_ids:
|
||||
matched.append(tool)
|
||||
used_ids.add(id(tool))
|
||||
return matched
|
||||
|
||||
def extract_user_query(self, messages: List[Dict[str, Any]]) -> str:
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -392,3 +392,140 @@ async def test_semantic_filter_hook_skips_no_tools():
|
|||
assert result is None, "Hook should skip requests without tools"
|
||||
print("✅ Hook correctly skips requests without tools")
|
||||
|
||||
|
||||
class TestGetToolsByNames:
|
||||
"""
|
||||
Regression coverage for SemanticMCPToolFilter._get_tools_by_names
|
||||
name-matching behavior (issue #26078).
|
||||
|
||||
The canonical name stored in the router is what the proxy's MCP
|
||||
registry emits (e.g. ``fc_web_search-firecrawl_scrape``). Some MCP
|
||||
clients — notably opencode — wrap every tool name with their own
|
||||
additive namespace prefix before sending it back in ``tools[]``, so
|
||||
the incoming name is ``litellm_fc_web_search-firecrawl_scrape``.
|
||||
|
||||
Exact-equality matching against the canonical dropped every such
|
||||
tool, the proxy forwarded ``tools: []`` with ``tool_choice: auto``,
|
||||
and strict upstream providers returned 400.
|
||||
"""
|
||||
|
||||
def _make_filter(self):
|
||||
from litellm.proxy._experimental.mcp_server.semantic_tool_filter import (
|
||||
SemanticMCPToolFilter,
|
||||
)
|
||||
|
||||
return SemanticMCPToolFilter(
|
||||
embedding_model="text-embedding-3-small",
|
||||
litellm_router_instance=Mock(),
|
||||
top_k=5,
|
||||
similarity_threshold=0.3,
|
||||
enabled=True,
|
||||
)
|
||||
|
||||
def test_exact_match_unchanged(self):
|
||||
"""Incoming name equals canonical — the historical path still works."""
|
||||
filter_instance = self._make_filter()
|
||||
available_tools = [
|
||||
{"name": "get_weather", "description": "fetch weather"},
|
||||
{"name": "send_email", "description": "send mail"},
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
["send_email"], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
assert matched[0]["name"] == "send_email"
|
||||
|
||||
def test_client_prefix_with_underscore_separator(self):
|
||||
"""Client wraps canonical with ``<alias>_`` (opencode pattern)."""
|
||||
filter_instance = self._make_filter()
|
||||
canonical = "fc_web_search-firecrawl_scrape"
|
||||
client_name = "litellm_" + canonical
|
||||
available_tools = [{"name": client_name, "description": "scrape"}]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
[canonical], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
# Must return the incoming tool unchanged so the client-facing
|
||||
# name survives, otherwise tool-call round-trips break client-side.
|
||||
assert matched[0]["name"] == client_name
|
||||
|
||||
def test_client_prefix_with_dash_separator(self):
|
||||
"""Some clients use dash as alias separator; accept that too."""
|
||||
filter_instance = self._make_filter()
|
||||
canonical = "get_weather"
|
||||
available_tools = [{"name": "mcp-get_weather", "description": "weather"}]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
[canonical], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
assert matched[0]["name"] == "mcp-get_weather"
|
||||
|
||||
def test_suffix_without_separator_does_not_match(self):
|
||||
"""
|
||||
A bare-substring suffix must not match — ``rain_gear`` is not a
|
||||
namespaced version of canonical ``ear`` and the user would be
|
||||
surprised to see it selected.
|
||||
"""
|
||||
filter_instance = self._make_filter()
|
||||
available_tools = [{"name": "rain_gear", "description": "raincoat"}]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(["ear"], available_tools)
|
||||
|
||||
assert matched == []
|
||||
|
||||
def test_exact_match_preferred_over_prefixed(self):
|
||||
"""
|
||||
When both a bare canonical and a client-prefixed variant are
|
||||
present, the bare one wins so ordering is stable.
|
||||
"""
|
||||
filter_instance = self._make_filter()
|
||||
canonical = "search"
|
||||
available_tools = [
|
||||
{"name": canonical, "description": "plain"},
|
||||
{"name": "litellm_" + canonical, "description": "wrapped"},
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
[canonical], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
assert matched[0]["name"] == canonical
|
||||
|
||||
def test_same_tool_not_returned_twice(self):
|
||||
"""
|
||||
Two canonicals that both suffix-match the same incoming tool
|
||||
must not produce a duplicate in the output list.
|
||||
"""
|
||||
filter_instance = self._make_filter()
|
||||
available_tools = [
|
||||
{"name": "srv_read_file", "description": "read"}
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
["read_file", "read_file"], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
|
||||
def test_ordering_follows_router_output(self):
|
||||
"""Returned tools follow the order the semantic router chose."""
|
||||
filter_instance = self._make_filter()
|
||||
available_tools = [
|
||||
{"name": "litellm_read", "description": "read"},
|
||||
{"name": "litellm_write", "description": "write"},
|
||||
{"name": "litellm_delete", "description": "delete"},
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
["write", "delete", "read"], available_tools
|
||||
)
|
||||
|
||||
names = [t["name"] for t in matched]
|
||||
assert names == ["litellm_write", "litellm_delete", "litellm_read"]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue