From 034f4fdef20fb9b8ab5c63787e2ba764ad4661cc Mon Sep 17 00:00:00 2001 From: sakenuGOD Date: Thu, 23 Apr 2026 05:06:34 +0300 Subject: [PATCH] fix(mcp_semantic_tool_filter): match tools with client-side namespace prefix (#26078) (#26117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 (``) and the name in the incoming `tools[]` list. MCP clients such as opencode wrap every tool name with their own additive alias prefix (`_`), 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 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 Co-authored-by: Krrish Dholakia --- .../mcp_server/semantic_tool_filter.py | 90 ++++++++- .../mcp_server/test_semantic_tool_filter.py | 190 ++++++++++++++++++ 2 files changed, 270 insertions(+), 10 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/semantic_tool_filter.py b/litellm/proxy/_experimental/mcp_server/semantic_tool_filter.py index 0e32bfd7026..a9c4d2ece46 100644 --- a/litellm/proxy/_experimental/mcp_server/semantic_tool_filter.py +++ b/litellm/proxy/_experimental/mcp_server/semantic_tool_filter.py @@ -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 + (````). 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 + ```` (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 ``_`` while the + router returned ```` (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: """ diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_semantic_tool_filter.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_semantic_tool_filter.py index 3acd5c112e3..2558df8533b 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_semantic_tool_filter.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_semantic_tool_filter.py @@ -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 ``_`` (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 == []