mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-11 22:51:28 +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 * review: strengthen the suffix-fallback tie-breaker and the deduplication regression test (Greptile comments on #26117) - test_same_tool_not_returned_twice now passes two distinct canonicals ("read_file" and "file") that both suffix-match the same incoming tool, rather than the same canonical twice, so the assertion actually exercises the used_ids dedup path instead of the duplicate-input-list path. - The suffix fallback in _get_tools_by_names now prefers the shortest incoming name that still qualifies under the separator-anchored match. In the one-prefix-per-client opencode scenario this is a no-op, but in multi-namespace configurations the shortest qualifying name is the least-wrapped one and is the most defensible deterministic choice, replacing the dict-insertion-order fallback. - Adds test_suffix_fallback_prefers_shortest_candidate covering the new tie-breaker directly. Still 15 tests passing locally (was 14). * review(#26117): gate suffix-matching on canonical containing MCP_TOOL_PREFIX_SEPARATOR @krrish-berri-2 flagged a possible collision in the suffix fallback: a local user function whose name happens to end in a bare canonical substring (e.g. my_firecrawl_scrape vs canonical firecrawl_scrape) would be spuriously selected. Server-registered MCP tools are always emitted as <server_name><MCP_TOOL_PREFIX_SEPARATOR><tool_name> via add_server_prefix_to_name, so a canonical without the separator is not a namespaced MCP tool and does not warrant suffix matching. Added that guard to _name_matches_canonical with a regression test (test_does_not_collide_with_local_function_on_unprefixed_canonical) that reproduces the collision before the fix and is pinned after. Pre-existing TestGetToolsByNames fixtures that relied on bare canonicals (get_weather, search, read_file, write/delete/read) were switched to realistic server-prefixed ones so they continue to exercise the suffix-fallback path under the new guard. The opencode scenario (client prefix on already-server-prefixed canonical) is unchanged. --------- Co-authored-by: sakenuGOD <sakenuGOD@users.noreply.github.com> Co-authored-by: Krrish Dholakia <krrish+github@berri.ai>
This commit is contained in:
sakenuGOD
GitHub
parent
09cd7e383e
commit
034f4fdef2
2 changed files with 270 additions and 10 deletions
|
|
@ -7,6 +7,7 @@ Filters MCP tools semantically for /chat/completions and /responses endpoints.
|
|||
from typing import TYPE_CHECKING, Any, Dict, List, Optional
|
||||
|
||||
from litellm._logging import verbose_logger
|
||||
from litellm.proxy._experimental.mcp_server.utils import MCP_TOOL_PREFIX_SEPARATOR
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from semantic_router.routers import SemanticRouter
|
||||
|
|
@ -214,20 +215,89 @@ 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``.
|
||||
|
||||
Suffix matching is additionally gated on ``canonical`` itself
|
||||
containing ``MCP_TOOL_PREFIX_SEPARATOR``. Server-registered MCP
|
||||
tools are always emitted as
|
||||
``<server_name><MCP_TOOL_PREFIX_SEPARATOR><tool_name>`` (see
|
||||
``add_server_prefix_to_name``), so a canonical without the
|
||||
separator is not a namespaced MCP tool and falling back to
|
||||
suffix matching would spuriously collide with unrelated local
|
||||
user functions whose names end in the same characters.
|
||||
"""
|
||||
if client_name == canonical:
|
||||
return True
|
||||
if MCP_TOOL_PREFIX_SEPARATOR not in canonical:
|
||||
return False
|
||||
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:
|
||||
# Prefer the shortest qualifying name. When several
|
||||
# incoming tools suffix-match the same canonical (e.g.
|
||||
# "my_search" and "my_tag_search" both end in "search"),
|
||||
# the one closest in length to the canonical is the
|
||||
# least-wrapped and most likely the intended target.
|
||||
best_name: Optional[str] = None
|
||||
for client_name in available_by_name:
|
||||
if not self._name_matches_canonical(client_name, canonical):
|
||||
continue
|
||||
if best_name is None or len(client_name) < len(best_name):
|
||||
best_name = client_name
|
||||
if best_name is not None:
|
||||
tool = available_by_name[best_name]
|
||||
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:
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -450,3 +450,193 @@ async def test_semantic_filter_hook_skips_no_tools():
|
|||
# Should return None (no modification)
|
||||
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 = "weather_svc-get_weather"
|
||||
available_tools = [
|
||||
{"name": "mcp-" + canonical, "description": "weather"}
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
[canonical], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
assert matched[0]["name"] == "mcp-" + canonical
|
||||
|
||||
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 distinct canonicals that both suffix-match the same incoming
|
||||
tool must not produce a duplicate in the output list.
|
||||
``fs-read_file`` and ``api-fs-read_file`` are both valid
|
||||
separator-anchored suffixes of ``litellm_api-fs-read_file``.
|
||||
"""
|
||||
filter_instance = self._make_filter()
|
||||
available_tools = [
|
||||
{"name": "litellm_api-fs-read_file", "description": "read"}
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
["fs-read_file", "api-fs-read_file"], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
|
||||
def test_suffix_fallback_prefers_shortest_candidate(self):
|
||||
"""
|
||||
When no exact match exists and several incoming tools
|
||||
suffix-match the same canonical, the one closest in length to
|
||||
the canonical (i.e. the least-wrapped) should be chosen.
|
||||
"""
|
||||
filter_instance = self._make_filter()
|
||||
canonical = "svc-search"
|
||||
available_tools = [
|
||||
{"name": "my_tag_" + canonical, "description": "tag search"},
|
||||
{"name": "my_" + canonical, "description": "plain search"},
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
[canonical], available_tools
|
||||
)
|
||||
|
||||
assert len(matched) == 1
|
||||
assert matched[0]["name"] == "my_" + canonical
|
||||
|
||||
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_fs-read", "description": "read"},
|
||||
{"name": "litellm_fs-write", "description": "write"},
|
||||
{"name": "litellm_fs-delete", "description": "delete"},
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
["fs-write", "fs-delete", "fs-read"], available_tools
|
||||
)
|
||||
|
||||
names = [t["name"] for t in matched]
|
||||
assert names == [
|
||||
"litellm_fs-write",
|
||||
"litellm_fs-delete",
|
||||
"litellm_fs-read",
|
||||
]
|
||||
|
||||
def test_does_not_collide_with_local_function_on_unprefixed_canonical(self):
|
||||
"""
|
||||
Guard against the collision @krrish-berri-2 flagged on #26117:
|
||||
if the canonical name from the router is not server-prefixed
|
||||
(i.e. does not contain ``MCP_TOOL_PREFIX_SEPARATOR``), suffix
|
||||
matching must not kick in. Otherwise an unrelated local user
|
||||
function whose name happens to end in the canonical substring
|
||||
would be spuriously selected.
|
||||
"""
|
||||
filter_instance = self._make_filter()
|
||||
available_tools = [
|
||||
{
|
||||
"name": "my_firecrawl_scrape",
|
||||
"description": "unrelated local function",
|
||||
},
|
||||
]
|
||||
|
||||
matched = filter_instance._get_tools_by_names(
|
||||
["firecrawl_scrape"], # no MCP_TOOL_PREFIX_SEPARATOR in canonical
|
||||
available_tools,
|
||||
)
|
||||
|
||||
assert matched == []
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue