mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
fix(mcp): report per-server outcomes in aggregate REST tools/list (#39232)
GET /mcp-rest/tools/list without server_id returned only the tools of the servers that answered and silently dropped any server whose listing failed (for example an OAuth-protected server without credentials), so clients could not tell a partial listing from a complete one. The aggregate response now carries a server_outcomes map keyed by server alias with the same classified outcome (ok/auth_required/forbidden/...) that the MCP protocol path already puts in _meta. Healthy tools and the HTTP 200 status are unchanged. Co-authored-by: yassin <yassin@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
b0041f32a2
commit
69029c139e
2 changed files with 51 additions and 31 deletions
|
|
@ -1,7 +1,8 @@
|
|||
import asyncio
|
||||
import importlib
|
||||
from collections.abc import Awaitable, Callable, Mapping
|
||||
from collections.abc import Awaitable, Callable, Mapping, Sequence
|
||||
from datetime import datetime
|
||||
from types import MappingProxyType
|
||||
from typing import TYPE_CHECKING, Any, Final, Literal
|
||||
|
||||
import anyio
|
||||
|
|
@ -20,8 +21,11 @@ from litellm.proxy._experimental.mcp_server.exceptions import (
|
|||
MCPUpstreamAuthError,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.faults.list_outcomes import (
|
||||
ServerListOk,
|
||||
ServerOutcome,
|
||||
classify_list_exception,
|
||||
list_fault_http_status,
|
||||
outcome_wire_value,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.ui_session_utils import (
|
||||
acting_user_auth,
|
||||
|
|
@ -99,6 +103,7 @@ if MCP_AVAILABLE:
|
|||
ListMCPToolsRestAPIResponseObject,
|
||||
MCPInfo,
|
||||
MCPServer,
|
||||
_aggregate_server_key, # pyright: ignore[reportPrivateUsage] # same per-server key as the tools/list _meta outcomes
|
||||
_apply_toolset_scope,
|
||||
_fire_mcp_tool_call_logging,
|
||||
execute_mcp_tool,
|
||||
|
|
@ -803,9 +808,6 @@ if MCP_AVAILABLE:
|
|||
list(allowed_server_ids_set), _rest_client_ip
|
||||
)
|
||||
|
||||
list_tools_result: Final = []
|
||||
error_message = None
|
||||
|
||||
# If server_id is specified, only query that specific server
|
||||
if server_id:
|
||||
return await _list_tools_for_single_server(
|
||||
|
|
@ -849,22 +851,19 @@ if MCP_AVAILABLE:
|
|||
else {}
|
||||
)
|
||||
|
||||
# Query all servers the user has access to
|
||||
errors: Final = []
|
||||
for allowed_server_id in allowed_server_ids:
|
||||
server = global_mcp_server_manager.get_mcp_server_by_id(allowed_server_id)
|
||||
if server is None:
|
||||
continue
|
||||
|
||||
server_auth_header = _get_server_auth_header(server, mcp_server_auth_headers, mcp_auth_header)
|
||||
user_oauth_extra_headers = await _get_user_oauth_extra_headers(
|
||||
async def list_server(
|
||||
server: MCPServer,
|
||||
) -> tuple[Sequence[ListMCPToolsRestAPIResponseObject], ServerOutcome]:
|
||||
server_auth_header: Final = _get_server_auth_header(
|
||||
server, mcp_server_auth_headers, mcp_auth_header
|
||||
)
|
||||
user_oauth_extra_headers: Final = await _get_user_oauth_extra_headers(
|
||||
server,
|
||||
user_api_key_dict,
|
||||
prefetched_creds=prefetched_oauth_creds,
|
||||
)
|
||||
|
||||
try:
|
||||
tools_result = await _get_tools_for_single_server(
|
||||
tools_result: Final = await _get_tools_for_single_server(
|
||||
server,
|
||||
server_auth_header,
|
||||
raw_headers_from_request,
|
||||
|
|
@ -872,24 +871,36 @@ if MCP_AVAILABLE:
|
|||
extra_headers=user_oauth_extra_headers,
|
||||
apply_tool_filters=apply_tool_filters,
|
||||
)
|
||||
list_tools_result.extend(tools_result)
|
||||
except Exception as e:
|
||||
verbose_logger.exception("Error getting tools from %s: %s", server.name, e)
|
||||
errors.append(
|
||||
f"{get_server_prefix(server)}: {classify_list_exception(e).tag}"
|
||||
if isinstance(e, (MCPServerListError, MCPUpstreamAuthError))
|
||||
else f"{get_server_prefix(server)}: {e}"
|
||||
)
|
||||
continue
|
||||
return (), classify_list_exception(e)
|
||||
return tools_result, ServerListOk(tool_count=len(tools_result))
|
||||
|
||||
if errors and not list_tools_result:
|
||||
error_message = "Failed to get tools from servers: " + "; ".join(errors)
|
||||
|
||||
return {
|
||||
"tools": list_tools_result,
|
||||
"error": "partial_failure" if error_message else None,
|
||||
"message": (error_message if error_message else "Successfully retrieved tools"),
|
||||
}
|
||||
# Query all servers the user has access to
|
||||
queried_servers: Final = tuple(
|
||||
server
|
||||
for server in map(global_mcp_server_manager.get_mcp_server_by_id, allowed_server_ids)
|
||||
if server is not None
|
||||
)
|
||||
listings: Final = tuple([await list_server(server) for server in queried_servers])
|
||||
list_tools_result: Final = [tool for tools, _ in listings for tool in tools]
|
||||
server_outcomes: Final = MappingProxyType(
|
||||
{_aggregate_server_key(server): outcome for server, (_, outcome) in zip(queried_servers, listings)}
|
||||
)
|
||||
errors: Final = tuple(
|
||||
f"{key}: {outcome.tag}" for key, outcome in server_outcomes.items() if outcome.tag != "ok"
|
||||
)
|
||||
error_message: Final = (
|
||||
"Failed to get tools from servers: " + "; ".join(errors)
|
||||
if errors and not list_tools_result
|
||||
else None
|
||||
)
|
||||
return {
|
||||
"tools": list_tools_result,
|
||||
"error": "partial_failure" if error_message else None,
|
||||
"message": (error_message if error_message else "Successfully retrieved tools"),
|
||||
"server_outcomes": {key: outcome_wire_value(outcome) for key, outcome in server_outcomes.items()},
|
||||
}
|
||||
|
||||
except MCPUpstreamAuthError as e:
|
||||
# Surface upstream pass-through 401/403 challenges to the client so
|
||||
|
|
|
|||
|
|
@ -1431,7 +1431,11 @@ class TestListToolsRestAPI:
|
|||
async def test_aggregate_list_absorbs_one_server_auth_failure(self, monkeypatch):
|
||||
"""The multi-server aggregate listing degrades a server whose upstream
|
||||
rejects auth to an empty contribution and still returns the healthy
|
||||
server's tools with a 200, rather than surfacing a 401."""
|
||||
server's tools with a 200, rather than surfacing a 401. The absorbed
|
||||
server must still show up as a classified per-server outcome so a REST
|
||||
caller can tell "needs upstream auth" apart from "has no tools"."""
|
||||
from pydantic import TypeAdapter
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.exceptions import (
|
||||
MCPUpstreamAuthError,
|
||||
)
|
||||
|
|
@ -1497,6 +1501,11 @@ class TestListToolsRestAPI:
|
|||
|
||||
assert result["tools"] == ["good-tool"]
|
||||
assert result["error"] is None
|
||||
wire_body = json.loads(TypeAdapter(dict).dump_json(result))
|
||||
assert wire_body["server_outcomes"] == {
|
||||
"good": {"status": "ok", "tool_count": 1},
|
||||
"bad": {"status": "auth_required", "http_status": 401},
|
||||
}
|
||||
|
||||
async def test_name_resolution_finds_server_by_uuid(self, monkeypatch):
|
||||
"""When server_id is a name string, it should be resolved to its UUID
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue