mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-29 06:33:28 +00:00
fix(mcp): resolve a bare tool name on a single-server scope against that server (#46225)
A JSON-RPC tools/call on /{server_name}/mcp (or /mcp with x-mcp-servers naming one server) that uses the upstream's bare tool name answered "Tool not found" on every proxy process that had not yet listed that server, because the per-process tool-name map stays empty for a server whose upstream only lists tools with the caller's own credentials, and mcp SDK 2 no longer lists before an uncached call. call_mcp_tool now hands the single scoped server to execute_mcp_tool as requested_server_id, so the existing REST contract lists the server with the caller's headers and routes the bare name to it, and a bare name shared by two servers goes to the one the path names
Co-authored-by: aayushbaluni <73417844+aayushbaluni@users.noreply.github.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
(cherry picked from commit ec28acf322)
This commit is contained in:
parent
35f52bf3e8
commit
1c6e82a60e
3 changed files with 112 additions and 27 deletions
|
|
@ -2450,6 +2450,20 @@ async def fire_mcp_tool_call_failure_logging(
|
|||
)
|
||||
|
||||
|
||||
def _single_scoped_server(
|
||||
mcp_servers: Sequence[str] | None, allowed_mcp_servers: Sequence[MCPServer]
|
||||
) -> MCPServer | None:
|
||||
"""The one server a ``/{server_name}/mcp`` path or ``x-mcp-servers`` header scoped this call to.
|
||||
|
||||
A bare tool name on such a request resolves against that server, the way a REST ``server_id``
|
||||
does, instead of against the per-process tool-name map, which stays empty for a server whose
|
||||
upstream only lists tools with the caller's own credentials until this process has listed it.
|
||||
"""
|
||||
if mcp_servers is None or len(mcp_servers) != 1 or len(allowed_mcp_servers) != 1:
|
||||
return None
|
||||
return allowed_mcp_servers[0]
|
||||
|
||||
|
||||
@client
|
||||
@catalog_operation(lambda: global_mcp_server_manager)
|
||||
async def call_mcp_tool(
|
||||
|
|
@ -2511,6 +2525,7 @@ async def call_mcp_tool(
|
|||
detail="User not allowed to call this tool.",
|
||||
)
|
||||
|
||||
scoped_server: Final = _single_scoped_server(mcp_servers, allowed_mcp_servers)
|
||||
# Delegate to execute_mcp_tool for execution
|
||||
response = await execute_mcp_tool(
|
||||
name=name,
|
||||
|
|
@ -2527,6 +2542,7 @@ async def call_mcp_tool(
|
|||
input_responses=input_responses,
|
||||
request_state=request_state,
|
||||
incoming_bearer_token=incoming_bearer_token,
|
||||
requested_server_id=scoped_server.server_id if scoped_server is not None else None,
|
||||
**kwargs,
|
||||
)
|
||||
except Exception as e:
|
||||
|
|
|
|||
|
|
@ -2994,11 +2994,7 @@ async def test_call_mcp_tool_uses_manager_permission_lookup():
|
|||
"get_mcp_server_by_id",
|
||||
return_value=mock_server,
|
||||
),
|
||||
patch.object(
|
||||
global_mcp_server_manager,
|
||||
"get_mcp_server_from_tool_name",
|
||||
return_value=mock_server,
|
||||
) as mock_get_server,
|
||||
patch.object(global_mcp_server_manager, "server_exposes_tool", return_value=True),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.operations.global_mcp_tool_registry"
|
||||
) as mock_tool_registry,
|
||||
|
|
@ -3023,24 +3019,18 @@ async def test_call_mcp_tool_uses_manager_permission_lookup():
|
|||
|
||||
assert result == expected_response
|
||||
mock_get_allowed.assert_awaited_once()
|
||||
# We call `_get_mcp_server_from_tool_name` multiple times:
|
||||
# - for logging/metadata
|
||||
# - for resolving the server during dispatch
|
||||
# - and inside `call_tool` for guardrails/hooks
|
||||
# The exact count isn't important, only that it is used.
|
||||
assert mock_get_server.call_count >= 2
|
||||
# First call should use the prefixed tool name
|
||||
assert (
|
||||
mock_get_server.call_args_list[0][0][0]
|
||||
== f"{mock_server.name}/gmail_send_email"
|
||||
)
|
||||
# The request names exactly one server, so that server is the dispatch target
|
||||
# and the tool name reaches it untouched (the "/" is not a LiteLLM prefix).
|
||||
mock_handle_managed.assert_awaited_once()
|
||||
assert mock_handle_managed.await_args.kwargs["server_name"] == mock_server.name
|
||||
assert mock_handle_managed.await_args.kwargs["name"] == f"{mock_server.name}/gmail_send_email"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_call_mcp_tool_resolves_unprefixed_tool_name_and_checks_permissions():
|
||||
"""
|
||||
Ensure `call_mcp_tool` correctly resolves the MCP server for an unprefixed tool
|
||||
name and enforces server-level permissions using that resolved server.
|
||||
Ensure `call_mcp_tool` resolves an unprefixed tool name on a request scoped to
|
||||
one server against that server and enforces server-level permissions with it.
|
||||
"""
|
||||
from litellm.proxy._experimental.mcp_server.server import (
|
||||
call_mcp_tool,
|
||||
|
|
@ -3070,11 +3060,7 @@ async def test_call_mcp_tool_resolves_unprefixed_tool_name_and_checks_permission
|
|||
"get_mcp_server_by_id",
|
||||
return_value=mock_server,
|
||||
),
|
||||
patch.object(
|
||||
global_mcp_server_manager,
|
||||
"get_mcp_server_from_tool_name",
|
||||
return_value=mock_server,
|
||||
) as mock_get_server,
|
||||
patch.object(global_mcp_server_manager, "server_exposes_tool", return_value=True),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.operations.global_mcp_tool_registry"
|
||||
) as mock_tool_registry,
|
||||
|
|
@ -3091,7 +3077,7 @@ async def test_call_mcp_tool_resolves_unprefixed_tool_name_and_checks_permission
|
|||
mock_tool_registry.get_tool.return_value = None
|
||||
mock_handle_managed.return_value = expected_response
|
||||
|
||||
# Call with UNPREFIXED tool name; server should be resolved via mapping
|
||||
# Call with UNPREFIXED tool name; the single server the request names is the target
|
||||
result = await call_mcp_tool(
|
||||
name="gmail_send_email",
|
||||
arguments={"body": "hello"},
|
||||
|
|
@ -3100,8 +3086,9 @@ async def test_call_mcp_tool_resolves_unprefixed_tool_name_and_checks_permission
|
|||
|
||||
assert result == expected_response
|
||||
mock_get_allowed.assert_awaited_once()
|
||||
# We should resolve the server at least once using the unprefixed name
|
||||
assert mock_get_server.call_count >= 1
|
||||
assert mock_get_server.call_args_list[0][0][0] == "gmail_send_email"
|
||||
mock_handle_managed.assert_awaited_once()
|
||||
assert mock_handle_managed.await_args.kwargs["server_name"] == mock_server.name
|
||||
assert mock_handle_managed.await_args.kwargs["name"] == "gmail_send_email"
|
||||
# Permissions check should be invoked with the resolved server name
|
||||
mock_is_allowed.assert_called_once()
|
||||
assert mock_is_allowed.call_args.kwargs["server_name"] == mock_server.name
|
||||
|
|
|
|||
|
|
@ -565,6 +565,88 @@ async def test_tools_call_warmup_does_not_consume_mcp_server_rpm() -> None:
|
|||
assert [tool.name for tool in listing.tools] == ["echo"]
|
||||
|
||||
|
||||
|
||||
def _scoped_rig(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> tuple[MCPServer, MCPServer, AsyncMock, AsyncMock]:
|
||||
"""Two config servers that both expose a bare ``list_rules`` on a manager that has listed neither."""
|
||||
from mcp.types import CallToolResult, TextContent
|
||||
|
||||
from litellm.proxy import proxy_server
|
||||
from litellm.proxy._experimental.mcp_server.mcp_server_manager import MCPServerManager
|
||||
|
||||
manager: Final = MCPServerManager()
|
||||
memory: Final = MCPServer(
|
||||
server_id="id-memory", name="memory_server", server_name="memory_server", transport=MCPTransport.http
|
||||
)
|
||||
other: Final = MCPServer(
|
||||
server_id="id-other", name="other_server", server_name="other_server", transport=MCPTransport.http
|
||||
)
|
||||
manager.config_mcp_servers = {server.server_id: server for server in (memory, other)}
|
||||
monkeypatch.setattr(proxy_server, "prisma_client", None)
|
||||
monkeypatch.setattr(operations, "global_mcp_server_manager", manager)
|
||||
monkeypatch.setattr(
|
||||
manager, "get_allowed_mcp_servers", AsyncMock(return_value=[memory.server_id, other.server_id])
|
||||
)
|
||||
monkeypatch.setattr(global_mcp_tool_registry, "published_tools", {})
|
||||
listing: Final = AsyncMock()
|
||||
monkeypatch.setattr(operations, "_get_tools_from_mcp_servers", listing)
|
||||
handled: Final = AsyncMock(
|
||||
return_value=CallToolResult(content=[TextContent(type="text", text="ok")], isError=False)
|
||||
)
|
||||
monkeypatch.setattr(operations, "_handle_managed_mcp_tool", handled)
|
||||
return memory, other, listing, handled
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("scope", ["memory_server", "other_server"])
|
||||
async def test_bare_tool_name_on_a_single_server_scope_routes_to_that_server_cold(
|
||||
monkeypatch: pytest.MonkeyPatch, scope: str
|
||||
) -> None:
|
||||
"""``/{server_name}/mcp`` (or ``x-mcp-servers`` naming one server) with the upstream's bare tool name
|
||||
must list and call the scoped server on a process whose tool-name map has never seen it, and when two
|
||||
servers share the bare name the scope decides (BerriAI/litellm#44831)."""
|
||||
memory, other, listing, handled = _scoped_rig(monkeypatch)
|
||||
expected: Final = memory if scope == "memory_server" else other
|
||||
|
||||
async with operations.global_mcp_server_manager.catalog.operation():
|
||||
result: Final = await operations.call_mcp_tool(
|
||||
name="list_rules",
|
||||
arguments={},
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-scoped"),
|
||||
mcp_servers=[scope],
|
||||
)
|
||||
|
||||
assert result.is_error is False
|
||||
assert listing.await_args.kwargs["mcp_servers"] == [expected.server_id]
|
||||
assert handled.await_args.kwargs["server_name"] == expected.name
|
||||
assert handled.await_args.kwargs["name"] == "list_rules"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("mcp_servers", [None, ["memory_server", "other_server"]])
|
||||
async def test_bare_tool_name_without_a_single_server_scope_is_still_unresolved_cold(
|
||||
monkeypatch: pytest.MonkeyPatch, mcp_servers: list[str] | None
|
||||
) -> None:
|
||||
"""The scope shortcut only applies when the request names exactly one server; an ambiguous bare name
|
||||
keeps today's answer rather than being guessed onto the first allowed server."""
|
||||
from fastapi import HTTPException
|
||||
|
||||
_memory, _other, listing, handled = _scoped_rig(monkeypatch)
|
||||
|
||||
async with operations.global_mcp_server_manager.catalog.operation():
|
||||
with pytest.raises(HTTPException) as unresolved:
|
||||
await operations.call_mcp_tool(
|
||||
name="list_rules",
|
||||
arguments={},
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-scoped"),
|
||||
mcp_servers=mcp_servers,
|
||||
)
|
||||
|
||||
assert unresolved.value.status_code == 404
|
||||
listing.assert_not_awaited()
|
||||
handled.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_tools_call_pre_call_check_enforces_mcp_server_rpm() -> None:
|
||||
from litellm.proxy._experimental.mcp_server.mcp_server_manager import MCPServerManager
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue