From f776ea7f9bf491f458dcf5a570599d0e544ff4d3 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Mon, 13 Jul 2026 19:25:37 -0700 Subject: [PATCH] feat(mcp): per-server outcomes for aggregate tools/list and truthful single-server REST statuses The aggregate MCP tools/list absorbed every per-server failure (upstream 401/403/5xx, timeouts, network errors) into that server contributing zero tools, making a broken upstream indistinguishable from a healthy server with no tools; the single-server REST list masked the same failures as {"tools": [], "error": null, "message": "Successfully retrieved tools"} Phase 2 of the MCP error-handling framework (LIT-4419): the manager fetch hops now raise a classified MCPServerListError (faults/list_outcomes.py: total classifier, frozen outcome values) instead of returning [], and each boundary applies the relay-vs-absorb policy matrix. The aggregate keeps serving the healthy subset but records each server's outcome, surfaced on the tools/list result _meta under litellm.ai/server_outcomes (the SDK passes a ListToolsResult through unwrapped) and in spend logs as per_server_list_outcomes. Single-server REST requests relay truthful statuses (unreachable/upstream_error 502, timeout 504, internal 500) and access denials now surface as real 403s instead of 200 unexpected_error bodies; upstream 403s surface through MCPUpstreamAuthError like 401s. Outcome wire values carry category and status code only, never upstream prose Resolves LIT-4421 --- .../_experimental/mcp_server/exceptions.py | 16 ++ .../mcp_server/faults/list_outcomes.py | 143 ++++++++++++++ .../mcp_server/mcp_server_manager.py | 35 ++-- .../mcp_server/rest_endpoints.py | 24 ++- .../proxy/_experimental/mcp_server/server.py | 107 +++++++---- .../_experimental/mcp_server/tool_search.py | 3 +- .../mcp_management_endpoints.py | 3 +- .../mcp/litellm_proxy_mcp_handler.py | 3 +- tests/mcp_tests/test_mcp_server.py | 21 +- .../mcp_server/faults/test_list_outcomes.py | 76 ++++++++ .../test_mcp_oauth_passthrough_tools.py | 27 +-- .../mcp_server/test_mcp_server.py | 181 +++++++++++++++--- .../mcp_server/test_mcp_server_manager.py | 77 +++++--- .../mcp_server/test_mcp_tool_search.py | 5 +- .../mcp_server/test_rest_endpoints.py | 91 +++++++-- .../mcp/test_litellm_proxy_mcp_handler.py | 5 +- 16 files changed, 669 insertions(+), 148 deletions(-) create mode 100644 litellm/proxy/_experimental/mcp_server/faults/list_outcomes.py create mode 100644 tests/test_litellm/proxy/_experimental/mcp_server/faults/test_list_outcomes.py diff --git a/litellm/proxy/_experimental/mcp_server/exceptions.py b/litellm/proxy/_experimental/mcp_server/exceptions.py index 3e3e549008d..74752809e86 100644 --- a/litellm/proxy/_experimental/mcp_server/exceptions.py +++ b/litellm/proxy/_experimental/mcp_server/exceptions.py @@ -88,3 +88,19 @@ class MCPToolResultError(Exception): into two identities, breaking ``isinstance`` checks against instances created before the reload. """ + + +class MCPServerListError(Exception): + """Carrier for a classified per-server listing fault (``faults.list_outcomes.ServerListFault``). + + Raised where a server fetch used to silently return an empty tool list, so each boundary can + apply its own policy: the aggregate listing absorbs it into that server's outcome, while + single-server routes relay a truthful HTTP status instead of empty-success. The fault value is + typed as ``object`` here only to avoid a circular import with the faults package; construction + sites always pass a ``ServerListFault``. + """ + + def __init__(self, fault: object, server_name: str) -> None: + self.fault = fault + self.server_name = server_name + super().__init__(f"Listing tools from MCP server {server_name!r} failed") diff --git a/litellm/proxy/_experimental/mcp_server/faults/list_outcomes.py b/litellm/proxy/_experimental/mcp_server/faults/list_outcomes.py new file mode 100644 index 00000000000..c8cf0821428 --- /dev/null +++ b/litellm/proxy/_experimental/mcp_server/faults/list_outcomes.py @@ -0,0 +1,143 @@ +"""Per-server outcomes for the aggregate MCP tools/list fan-out. + +The aggregate listing deliberately keeps serving the healthy subset when one server fails, but a +failed server must contribute a classified outcome instead of silently shrinking the list: an empty +contribution with no signal makes a broken upstream indistinguishable from a healthy server with no +tools. Outcomes carry only machine fields (category and status code) so nothing from an upstream +body crosses the trust boundary; classification is total, so any exception out of a server fetch +becomes an outcome, never a second failure. +""" + +from __future__ import annotations + +from typing import Literal, NamedTuple, TypeAlias + +import httpx +from mcp.types import Tool as MCPTool +from pydantic import BaseModel, ConfigDict +from typing_extensions import assert_never + +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPServerListError, + MCPUpstreamAuthError, +) + +ListFaultCategory: TypeAlias = Literal[ + "auth_required", + "forbidden", + "timeout", + "unreachable", + "upstream_error", + "internal", +] + + +class ServerListOk(BaseModel): + model_config = ConfigDict(frozen=True) + tag: Literal["ok"] = "ok" + tool_count: int + + +class ServerListFault(BaseModel): + """Why a server contributed nothing to a listing: the caller must authenticate upstream + (``auth_required``/``forbidden``), the upstream did not answer (``timeout``/``unreachable``), + the upstream answered outside its contract (``upstream_error``), or the gateway itself failed + (``internal``). ``status_code`` is the upstream HTTP status when one exists.""" + + model_config = ConfigDict(frozen=True) + tag: ListFaultCategory + status_code: int | None = None + + +ServerOutcome: TypeAlias = ServerListOk | ServerListFault + +SERVER_OUTCOMES_META_KEY = "litellm.ai/server_outcomes" +"""The tools/list result ``_meta`` key carrying per-server outcomes. Prefixed with the litellm.ai +domain per the MCP spec's ``_meta`` key format so it cannot collide with spec-reserved names.""" + + +class AggregateToolListing(NamedTuple): + tools: list[MCPTool] + outcomes: dict[str, ServerOutcome] + + +def _find_upstream_response(exc: BaseException) -> httpx.Response | None: + """Walk the exception tree (``__cause__``/``__context__``/ExceptionGroup members) for an + ``httpx.Response``, mirroring how upstream failures surface through the MCP SDK's task groups.""" + seen: set[int] = set() + stack = [exc] + while stack: + current = stack.pop() + if id(current) in seen: + continue + seen.add(id(current)) + response = getattr(current, "response", None) + if isinstance(response, httpx.Response): + return response + exceptions = getattr(current, "exceptions", None) + if isinstance(exceptions, tuple): + stack.extend(exceptions) + for link in (current.__cause__, current.__context__): + if link is not None: + stack.append(link) + return None + + +def classify_list_exception(exc: BaseException) -> ServerListFault: + """Classify a per-server listing failure into exactly one outcome. Total: an exception this + function cannot recognize is the gateway's own fault (``internal``), never a re-raise.""" + if isinstance(exc, MCPServerListError) and isinstance(exc.fault, ServerListFault): + return exc.fault + if isinstance(exc, MCPUpstreamAuthError): + tag = "forbidden" if exc.status_code == 403 else "auth_required" + return ServerListFault(tag=tag, status_code=exc.status_code) + if isinstance(exc, TimeoutError): + return ServerListFault(tag="timeout") + if isinstance(exc, ConnectionError): + return ServerListFault(tag="unreachable") + response = _find_upstream_response(exc) + if response is not None: + if response.status_code == 401: + return ServerListFault(tag="auth_required", status_code=401) + if response.status_code == 403: + return ServerListFault(tag="forbidden", status_code=403) + return ServerListFault(tag="upstream_error", status_code=response.status_code) + if isinstance(exc, (httpx.TimeoutException,)): + return ServerListFault(tag="timeout") + if isinstance(exc, httpx.TransportError): + return ServerListFault(tag="unreachable") + return ServerListFault(tag="internal") + + +def outcome_wire_value(outcome: ServerOutcome) -> dict[str, object]: + """The client-visible form of one outcome, for the tools/list result ``_meta`` and the REST + response: category plus status code only, never upstream prose or URLs.""" + match outcome.tag: + case "ok": + return {"status": "ok", "tool_count": outcome.tool_count} + case "auth_required" | "forbidden" | "timeout" | "unreachable" | "upstream_error" | "internal": + return { + "status": outcome.tag, + **({"http_status": outcome.status_code} if outcome.status_code is not None else {}), + } + case _: + assert_never(outcome.tag) + + +def list_fault_http_status(fault: ServerListFault) -> int: + """The truthful HTTP status for a single-upstream listing fault per RFC 9110: the upstream's own + 401/403 for auth, 504 for a timeout, 502 for an unreachable or misbehaving upstream, and 500 only + for the gateway's own failure.""" + match fault.tag: + case "auth_required": + return fault.status_code or 401 + case "forbidden": + return 403 + case "timeout": + return 504 + case "unreachable" | "upstream_error": + return 502 + case "internal": + return 500 + case _: + assert_never(fault.tag) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index e6e265abb61..6fa0232b4c8 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -50,7 +50,14 @@ from litellm.llms.custom_httpx.http_handler import get_async_httpx_client from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import ( MCPRequestHandler, ) -from litellm.proxy._experimental.mcp_server.exceptions import MCPUpstreamAuthError +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPServerListError, + MCPUpstreamAuthError, +) +from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ( + ServerListFault, + classify_list_exception, +) from litellm.proxy._experimental.mcp_server.elicitation_handler import ( MCP_ELICITATION_AVAILABLE, ) @@ -2767,10 +2774,12 @@ class MCPServerManager: server_name=server.name, ) from e verbose_logger.warning(f"Failed to get tools from server {server.name}: {str(e)}") - return [] + raise MCPServerListError(ServerListFault(tag="internal", status_code=e.status_code), server.name) from e + except MCPServerListError: + raise except Exception as e: verbose_logger.warning(f"Failed to get tools from server {server.name}: {str(e)}") - return [] + raise MCPServerListError(classify_list_exception(e), server.name) from e async def get_prompts_from_server( self, @@ -3372,34 +3381,36 @@ class MCPServerManager: server_name: Name of the server for logging Returns: - List of tools from the server + List of tools from the server. Failures never return an empty list: an upstream 401/403 + raises MCPUpstreamAuthError and everything else raises MCPServerListError carrying a + classified fault, so each boundary applies its own absorb-or-relay policy. """ try: with anyio.fail_after(MCP_TOOL_LISTING_TIMEOUT): tools = await client.list_tools(raise_on_error=True) verbose_logger.debug(f"Tools from {server_name}: {tools}") return tools - except TimeoutError: + except TimeoutError as e: verbose_logger.warning(f"Timeout while listing tools from {server_name}") - return [] + raise MCPServerListError(ServerListFault(tag="timeout"), server_name) from e except asyncio.CancelledError: verbose_logger.warning(f"Task cancelled while listing tools from {server_name}") return [] except ConnectionError as e: verbose_logger.warning(f"Connection error while listing tools from {server_name}: {str(e)}") - return [] + raise MCPServerListError(ServerListFault(tag="unreachable"), server_name) from e except Exception as e: auth_info = _extract_upstream_auth_failure(e) - if auth_info is not None and auth_info[0] == 401: - _, www_authenticate = auth_info - verbose_logger.info(f"Upstream auth failure from MCP server {server_name}: HTTP 401") + if auth_info is not None and auth_info[0] in (401, 403): + status_code, www_authenticate = auth_info + verbose_logger.info(f"Upstream auth failure from MCP server {server_name}: HTTP {status_code}") raise MCPUpstreamAuthError( - status_code=401, + status_code=status_code, www_authenticate=www_authenticate, server_name=server_name, ) from e verbose_logger.warning(f"Error listing tools from {server_name}: {str(e)}") - return [] + raise MCPServerListError(classify_list_exception(e), server_name) from e _SHORT_PREFIX_MAX_REHASH_ATTEMPTS = 1024 diff --git a/litellm/proxy/_experimental/mcp_server/rest_endpoints.py b/litellm/proxy/_experimental/mcp_server/rest_endpoints.py index 111fde86ea0..caca63a9d35 100644 --- a/litellm/proxy/_experimental/mcp_server/rest_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/rest_endpoints.py @@ -19,7 +19,14 @@ import httpx from fastapi import APIRouter, Depends, HTTPException, Query, Request, status from litellm._logging import verbose_logger -from litellm.proxy._experimental.mcp_server.exceptions import MCPUpstreamAuthError +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPServerListError, + MCPUpstreamAuthError, +) +from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ( + classify_list_exception, + list_fault_http_status, +) from litellm.proxy._experimental.mcp_server.ui_session_utils import ( build_effective_auth_contexts, ) @@ -627,6 +634,16 @@ if MCP_AVAILABLE: # matching status code and WWW-Authenticate challenge; that is what # lets standards-compliant MCP clients run the upstream OAuth flow. raise + except MCPServerListError as e: + fault = classify_list_exception(e) + verbose_logger.info(f"Listing tools from {server.name} failed with a {fault.tag} fault") + raise HTTPException( + status_code=list_fault_http_status(fault), + detail={ + "error": fault.tag, + "message": f"Failed to list tools from server {server.name}", + }, + ) from e except Exception as e: verbose_logger.exception(f"Error getting tools from {server.name}: {e}") return { @@ -858,7 +875,10 @@ if MCP_AVAILABLE: request_path=request.scope.get("_original_path") or request.url.path, ) except HTTPException as http_exc: - if http_exc.status_code == status.HTTP_404_NOT_FOUND: + if http_exc.status_code == status.HTTP_404_NOT_FOUND or server_id: + # Single-server requests relay the truthful status (a 502/504 upstream fault must + # not masquerade as a 200 empty-success body); only the multi-server aggregate + # keeps the legacy error-dict response shape below. raise # Internal access/IP 403s keep the legacy error-dict response shape # so the existing contract stays intact. diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index 68a61b85175..2c2c77bd254 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -348,6 +348,7 @@ if MCP_AVAILABLE: CallToolResult, EmbeddedResource, ImageContent, + ListToolsResult, Prompt, TextContent, ) @@ -356,6 +357,14 @@ if MCP_AVAILABLE: from litellm.proxy._experimental.mcp_server.auth.litellm_auth_handler import ( MCPAuthenticatedUser, ) + from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ( + SERVER_OUTCOMES_META_KEY, + AggregateToolListing, + ServerListOk, + ServerOutcome, + classify_list_exception, + outcome_wire_value, + ) from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( MCPServerManager, _caller_authorization_fans_out, @@ -664,9 +673,12 @@ if MCP_AVAILABLE: ######################################################## @server.list_tools() - async def handle_list_tools() -> List[Tool]: + async def handle_list_tools() -> "ListToolsResult | List[Tool]": """ - List all available tools. + List all available tools, with each server's listing outcome attached to the result's + ``_meta`` (SERVER_OUTCOMES_META_KEY) so a broken upstream is distinguishable from a healthy + server with no tools. Returning a ListToolsResult (rather than a bare list) makes the MCP SDK + pass the result through unwrapped, which is what lets the ``_meta`` survive to the client. Also captures the active session for propagation to callbacks. """ from mcp.server.lowlevel.server import request_ctx @@ -709,7 +721,7 @@ if MCP_AVAILABLE: # Get mcp_servers from context variable verbose_logger.debug("MCP list_tools - Calling _list_mcp_tools") - tools = await _list_mcp_tools( + listing = await _list_mcp_tools( user_api_key_auth=user_api_key_auth, mcp_auth_header=mcp_auth_header, mcp_servers=mcp_servers, @@ -719,8 +731,15 @@ if MCP_AVAILABLE: log_list_tools_to_spendlogs=True, list_tools_log_source="mcp_protocol", ) - verbose_logger.info(f"MCP list_tools - Successfully returned {len(tools)} tools") - return tools + verbose_logger.info(f"MCP list_tools - Successfully returned {len(listing.tools)} tools") + if not listing.outcomes: + return listing.tools + outcome_meta = { + SERVER_OUTCOMES_META_KEY: { + key: outcome_wire_value(outcome) for key, outcome in listing.outcomes.items() + } + } + return ListToolsResult.model_validate({"tools": listing.tools, "_meta": outcome_meta}) except Exception as e: verbose_logger.exception(f"Error in list_tools endpoint: {str(e)}") # Return empty list instead of failing completely @@ -1746,6 +1765,14 @@ if MCP_AVAILABLE: _mcp_gateway_initialize_instructions.reset(instructions_token) _mcp_gateway_server_name.reset(server_name_token) + def _aggregate_server_key(server: MCPServer) -> str: + return str( + getattr(server, "server_name", None) + or getattr(server, "alias", None) + or getattr(server, "name", None) + or "unknown" + ) + async def _get_tools_from_mcp_servers( user_api_key_auth: Optional[UserAPIKeyAuth], mcp_auth_header: Optional[str], @@ -1758,7 +1785,7 @@ if MCP_AVAILABLE: litellm_trace_id: Optional[str] = None, request_tags: Optional[list[str]] = None, client_ip: Optional[str] = None, - ) -> List[MCPTool]: + ) -> AggregateToolListing: """ Helper method to fetch tools from MCP servers based on server filtering criteria. @@ -1770,10 +1797,11 @@ if MCP_AVAILABLE: oauth2_headers: Optional dict of oauth2 headers Returns: - List[MCPTool]: Combined list of tools from filtered servers + AggregateToolListing: Combined tools from filtered servers plus each server's + classified listing outcome """ if not MCP_AVAILABLE: - return [] + return AggregateToolListing(tools=[], outcomes={}) list_tools_start_time = datetime.now() litellm_logging_obj: Optional[LiteLLMLoggingObj] = None @@ -1858,10 +1886,12 @@ if MCP_AVAILABLE: async def _fetch_and_filter_server_tools( server: MCPServer, - ) -> List[MCPTool]: - """Fetch and filter tools from a single server with error handling.""" + ) -> "tuple[List[MCPTool], ServerOutcome]": + """Fetch and filter tools from a single server, classifying any failure into that + server's outcome so the aggregate can keep serving the healthy subset without a + broken server masquerading as an empty one.""" if server is None: - return [] + return [], ServerListOk(tool_count=0) server_auth_header, extra_headers = _prepare_mcp_server_headers( server=server, @@ -1931,8 +1961,8 @@ if MCP_AVAILABLE: verbose_logger.debug( f"Successfully fetched {len(tools)} tools from server {server.name}, {len(filtered_tools)} after filtering" ) - return filtered_tools - except MCPUpstreamAuthError: + return filtered_tools, ServerListOk(tool_count=len(filtered_tools)) + except MCPUpstreamAuthError as e: # Absorb so one unauthenticated server does not empty every other server's # tools. Surfacing the upstream 401 to the client as a re-auth challenge is # intentionally not done here: raising from this list handler cannot produce a @@ -1940,31 +1970,30 @@ if MCP_AVAILABLE: # error). Single-server routes surface it via the request-scope preemptive # check in _raise_preemptive_401_for_unauthenticated_servers instead. verbose_logger.debug(f"MCP list_tools: omitting {server.name}; it needs upstream auth") - return [] + return [], classify_list_exception(e) except Exception as e: verbose_logger.exception(f"Error getting tools from server {server.name}: {str(e)}") - return [] + return [], classify_list_exception(e) # Fetch tools from all servers in parallel tasks = [_fetch_and_filter_server_tools(server) for server in allowed_mcp_servers] results = await asyncio.gather(*tasks) # Flatten results into single list - all_tools: List[MCPTool] = [tool for tools in results for tool in tools] + all_tools: List[MCPTool] = [tool for tools, _ in results for tool in tools] + server_outcomes: Dict[str, ServerOutcome] = { + _aggregate_server_key(server): outcome + for server, (_, outcome) in zip(allowed_mcp_servers, results) + if server is not None + } # If logging is enabled, enrich spend_logs_metadata with counts if litellm_logging_obj: - per_server_tool_counts: Dict[str, int] = {} - for server, server_tools in zip(allowed_mcp_servers, results): - if server is None: - continue - server_key = ( - getattr(server, "server_name", None) - or getattr(server, "alias", None) - or getattr(server, "name", None) - or "unknown" - ) - per_server_tool_counts[str(server_key)] = len(server_tools) + per_server_tool_counts: Dict[str, int] = { + _aggregate_server_key(server): len(server_tools) + for server, (server_tools, _) in zip(allowed_mcp_servers, results) + if server is not None + } metadata_dict = litellm_logging_obj.model_call_details.get("metadata") if isinstance(metadata_dict, dict): @@ -1975,6 +2004,9 @@ if MCP_AVAILABLE: spend_meta["allowed_server_count"] = len(allowed_mcp_servers) spend_meta["tool_count_total"] = len(all_tools) spend_meta["per_server_tool_counts"] = per_server_tool_counts + spend_meta["per_server_list_outcomes"] = { + key: outcome_wire_value(outcome) for key, outcome in server_outcomes.items() + } end_time = datetime.now() try: @@ -1995,7 +2027,7 @@ if MCP_AVAILABLE: verbose_logger.info(f"Successfully fetched {len(all_tools)} tools total from all MCP servers") - return all_tools + return AggregateToolListing(tools=all_tools, outcomes=server_outcomes) except Exception as e: # Only fire failure hook if logging was requested for this list-tools execution if log_list_tools_to_spendlogs and user_api_key_auth is not None: @@ -2265,7 +2297,7 @@ if MCP_AVAILABLE: log_list_tools_to_spendlogs: bool = False, list_tools_log_source: Optional[str] = None, client_ip: Optional[str] = None, - ) -> List[MCPTool]: + ) -> AggregateToolListing: """ List all available MCP tools. @@ -2277,19 +2309,18 @@ if MCP_AVAILABLE: client_ip: Client IP for IP-based server access control Returns: - List[MCPTool]: Combined list of tools from all accessible servers + AggregateToolListing: Combined tools from all accessible servers plus each server's + classified listing outcome """ if not MCP_AVAILABLE: - return [] + return AggregateToolListing(tools=[], outcomes={}) # Resolve toolset permissions and merge into the key's object_permission # so that the existing filter_tools_by_key_team_permissions logic picks them up. user_api_key_auth = await _merge_toolset_permissions(user_api_key_auth) - # Get tools from managed MCP servers with error handling - managed_tools = [] try: - managed_tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=mcp_auth_header, mcp_servers=mcp_servers, @@ -2300,12 +2331,12 @@ if MCP_AVAILABLE: list_tools_log_source=list_tools_log_source, client_ip=client_ip, ) - verbose_logger.debug(f"Successfully fetched {len(managed_tools)} tools from managed MCP servers") + verbose_logger.debug(f"Successfully fetched {len(listing.tools)} tools from managed MCP servers") + return listing except Exception as e: verbose_logger.exception(f"Error getting tools from managed MCP servers: {str(e)}") - # Continue with empty managed tools list instead of failing completely - - return managed_tools + # Continue with an empty listing instead of failing completely + return AggregateToolListing(tools=[], outcomes={}) async def _list_mcp_prompts( user_api_key_auth: Optional[UserAPIKeyAuth] = None, diff --git a/litellm/proxy/_experimental/mcp_server/tool_search.py b/litellm/proxy/_experimental/mcp_server/tool_search.py index fa57a2b3eb2..2f6b54a264a 100644 --- a/litellm/proxy/_experimental/mcp_server/tool_search.py +++ b/litellm/proxy/_experimental/mcp_server/tool_search.py @@ -91,7 +91,7 @@ async def handle_mcp_tool_search( from litellm.proxy._experimental.mcp_server.server import _list_mcp_tools - mcp_tools = await _list_mcp_tools( + mcp_listing = await _list_mcp_tools( user_api_key_auth=user_api_key_dict, mcp_servers=mcp_servers, client_ip=client_ip, @@ -100,6 +100,7 @@ async def handle_mcp_tool_search( oauth2_headers=oauth2_headers, raw_headers=raw_headers, ) + mcp_tools = mcp_listing.tools tools = [ { "name": t.name, diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 288282dd08b..c5401bb88cf 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -720,12 +720,13 @@ if MCP_AVAILABLE: """ from litellm.proxy._experimental.mcp_server.server import _list_mcp_tools - tools = await _list_mcp_tools( + listing = await _list_mcp_tools( user_api_key_auth=user_api_key_dict, mcp_auth_header=None, mcp_servers=None, mcp_server_auth_headers=None, ) + tools = listing.tools dumped_tools = [dict(tool) for tool in tools] return {"tools": dumped_tools} diff --git a/litellm/responses/mcp/litellm_proxy_mcp_handler.py b/litellm/responses/mcp/litellm_proxy_mcp_handler.py index e03f0296109..392bb7bcab2 100644 --- a/litellm/responses/mcp/litellm_proxy_mcp_handler.py +++ b/litellm/responses/mcp/litellm_proxy_mcp_handler.py @@ -260,7 +260,7 @@ class LiteLLM_Proxy_MCP_Handler: # names), so use None and let the auth object's mcp_servers do the filtering. effective_server_filter = None if resolved_toolset_ids else (resolved_mcp_servers or None) - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=mcp_auth_header, mcp_servers=effective_server_filter, @@ -270,6 +270,7 @@ class LiteLLM_Proxy_MCP_Handler: litellm_trace_id=litellm_trace_id, request_tags=request_tags, ) + tools = listing.tools allowed_mcp_server_ids = await global_mcp_server_manager.get_allowed_mcp_servers(user_api_key_auth) allowed_mcp_servers = global_mcp_server_manager.get_mcp_servers_from_ids( # type: ignore[attr-defined] diff --git a/tests/mcp_tests/test_mcp_server.py b/tests/mcp_tests/test_mcp_server.py index 515bf1233aa..f1e6539439a 100644 --- a/tests/mcp_tests/test_mcp_server.py +++ b/tests/mcp_tests/test_mcp_server.py @@ -935,8 +935,8 @@ async def test_get_tools_from_mcp_servers(): mcp_auth_header=mock_auth_header, mcp_servers=["server1"], ) - assert len(result) == 1, "Should only return tools from server1" - assert result[0].name == "tool1", "Should return tool from server1" + assert len(result.tools) == 1, "Should only return tools from server1" + assert result.tools[0].name == "tool1", "Should return tool from server1" # Test Case 2: Without specific MCP servers # Create a different mock manager for the second test case @@ -978,9 +978,9 @@ async def test_get_tools_from_mcp_servers(): mcp_auth_header=mock_auth_header, mcp_servers=None, ) - assert len(result) == 2, "Should return tools from all servers" + assert len(result.tools) == 2, "Should return tools from all servers" assert ( - result[0].name == "tool1" and result[1].name == "tool2" + result.tools[0].name == "tool1" and result.tools[1].name == "tool2" ), "Should return tools from all servers" # @@ -1015,8 +1015,8 @@ async def test_get_tools_from_mcp_servers(): mcp_auth_header=mock_auth_header, mcp_servers=["group-a"], ) - assert len(result) == 1, "Should only return tools from server3" - assert result[0].name == "tool1", "Should return tool from server1" + assert len(result.tools) == 1, "Should only return tools from server3" + assert result.tools[0].name == "tool1", "Should return tool from server1" except AssertionError as e: pytest.fail(f"Test failed: {str(e)}") @@ -2436,11 +2436,12 @@ async def test_filter_tools_by_allowed_tools_integration(): mock_client_constructor, ): # Call _get_tools_from_mcp_servers which should apply the filtering - filtered_tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=mock_user_auth, mcp_auth_header="Bearer test_token", mcp_servers=None, # Get from all servers ) + filtered_tools = listing.tools # Verify that only allowed tools are returned assert ( @@ -2549,11 +2550,12 @@ async def test_filter_tools_by_disallowed_tools_integration(): mock_client_constructor, ): # Call _get_tools_from_mcp_servers which should apply the filtering - filtered_tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=mock_user_auth, mcp_auth_header="Bearer test_token", mcp_servers=None, # Get from all servers ) + filtered_tools = listing.tools # Verify that only safe tools are returned (dangerous tools filtered out) assert ( @@ -2650,11 +2652,12 @@ async def test_filter_tools_no_restrictions_integration(): mock_client_constructor, ): # Call _get_tools_from_mcp_servers which should apply the filtering - filtered_tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=mock_user_auth, mcp_auth_header="Bearer test_token", mcp_servers=None, # Get from all servers ) + filtered_tools = listing.tools # Should return all tools when no restrictions assert ( diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/faults/test_list_outcomes.py b/tests/test_litellm/proxy/_experimental/mcp_server/faults/test_list_outcomes.py new file mode 100644 index 00000000000..4c2a307566f --- /dev/null +++ b/tests/test_litellm/proxy/_experimental/mcp_server/faults/test_list_outcomes.py @@ -0,0 +1,76 @@ +"""Classification and rendering matrix for per-server tools/list outcomes: every failure mode maps +to exactly one category, wire values never carry upstream prose, and single-upstream HTTP statuses +stay truthful to who failed.""" + +import httpx +import pytest + +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPServerListError, + MCPUpstreamAuthError, +) +from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ( + ServerListFault, + ServerListOk, + classify_list_exception, + list_fault_http_status, + outcome_wire_value, +) + + +def test_carried_fault_passes_through(): + fault = ServerListFault(tag="timeout") + assert classify_list_exception(MCPServerListError(fault, "srv")) is fault + + +def test_upstream_auth_error_maps_to_auth_required_and_forbidden(): + assert classify_list_exception(MCPUpstreamAuthError(401, None, "srv")).tag == "auth_required" + assert classify_list_exception(MCPUpstreamAuthError(403, None, "srv")).tag == "forbidden" + + +def test_timeout_and_connection_errors_classify_without_status(): + assert classify_list_exception(TimeoutError()).tag == "timeout" + assert classify_list_exception(ConnectionError()).tag == "unreachable" + + +def test_embedded_upstream_response_status_wins(): + response = httpx.Response(503, request=httpx.Request("POST", "https://mcp.example.com/mcp")) + exc = httpx.HTTPStatusError("boom", request=response.request, response=response) + wrapped = RuntimeError("wrapper") + wrapped.__cause__ = exc + fault = classify_list_exception(wrapped) + assert fault.tag == "upstream_error" + assert fault.status_code == 503 + + +def test_embedded_401_classifies_auth_required(): + response = httpx.Response(401, request=httpx.Request("POST", "https://mcp.example.com/mcp")) + exc = httpx.HTTPStatusError("no", request=response.request, response=response) + assert classify_list_exception(exc).tag == "auth_required" + + +def test_unknown_exception_is_internal(): + assert classify_list_exception(ValueError("who knows")).tag == "internal" + + +def test_wire_value_carries_no_prose(): + fault = ServerListFault(tag="upstream_error", status_code=500) + assert outcome_wire_value(fault) == {"status": "upstream_error", "http_status": 500} + assert outcome_wire_value(ServerListOk(tool_count=7)) == {"status": "ok", "tool_count": 7} + assert outcome_wire_value(ServerListFault(tag="timeout")) == {"status": "timeout"} + + +@pytest.mark.parametrize( + "tag,status_code,expected", + [ + ("auth_required", 401, 401), + ("auth_required", None, 401), + ("forbidden", 403, 403), + ("timeout", None, 504), + ("unreachable", None, 502), + ("upstream_error", 500, 502), + ("internal", None, 500), + ], +) +def test_single_upstream_http_status_is_truthful(tag, status_code, expected): + assert list_fault_http_status(ServerListFault(tag=tag, status_code=status_code)) == expected diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py index 16c36af5156..2d56680b64d 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_oauth_passthrough_tools.py @@ -284,7 +284,8 @@ async def test_aggregate_list_tools_absorbs_one_unauthenticated_server(): """Regression: across the aggregate (/mcp), a delegate/passthrough server that raises MCPUpstreamAuthError must not empty every other server's tools. Re-raising it on the aggregate path (introduced with the passthrough feature) zeroed the whole list because the - fan-out gather propagated it.""" + fan-out gather propagated it. The failed server now contributes an "auth_required" outcome + instead of vanishing, so it stays distinguishable from a healthy server with no tools.""" from unittest.mock import patch from mcp.types import Tool as MCPTool @@ -312,22 +313,24 @@ async def test_aggregate_list_tools_absorbs_one_unauthenticated_server(): ), patch.object( mcp_server.global_mcp_server_manager, "_get_tools_from_server", AsyncMock(side_effect=fake_get_tools) ): - tools = await mcp_server._get_tools_from_mcp_servers( + listing = await mcp_server._get_tools_from_mcp_servers( user_api_key_auth=UserAPIKeyAuth(token="h", user_id="u1"), mcp_auth_header=None, mcp_servers=None, ) - assert [t.name for t in tools] == ["working_docs-read"] + assert [t.name for t in listing.tools] == ["working_docs-read"] + assert listing.outcomes["delegate_docs"].tag == "auth_required" + assert listing.outcomes["working_docs"].tag == "ok" @pytest.mark.asyncio async def test_single_server_route_also_absorbs_upstream_auth_error(): """A single-server route (//mcp) absorbs an upstream-auth error just like the aggregate: - the failing server is omitted (empty list) rather than re-raised. Surfacing it to the client as a - 401 + WWW-Authenticate challenge cannot be done from this list handler — the MCP session manager - serializes a raise into a JSON-RPC error, not an HTTP 401 — so re-auth surfacing is handled by a - request-scope preemptive check, tracked separately.""" + the failing server contributes no tools and an "auth_required" outcome rather than re-raising. + Surfacing it to the client as a 401 + WWW-Authenticate challenge cannot be done from this list + handler — the MCP session manager serializes a raise into a JSON-RPC error, not an HTTP 401 — so + re-auth surfacing is handled by a request-scope preemptive check, tracked separately.""" from unittest.mock import patch from litellm.proxy._experimental.mcp_server import server as mcp_server @@ -351,12 +354,13 @@ async def test_single_server_route_also_absorbs_upstream_auth_error(): ), patch.object( mcp_server.global_mcp_server_manager, "_get_tools_from_server", AsyncMock(side_effect=fake_get_tools) ): - tools = await mcp_server._get_tools_from_mcp_servers( + listing = await mcp_server._get_tools_from_mcp_servers( user_api_key_auth=UserAPIKeyAuth(token="h", user_id="u1"), mcp_auth_header=None, mcp_servers=["delegate_docs"], ) - assert tools == [] + assert listing.tools == [] + assert listing.outcomes["delegate_docs"].tag == "auth_required" finally: _mcp_gateway_server_name.reset(token) @@ -388,10 +392,11 @@ async def test_aggregate_with_single_accessible_server_still_absorbs(): mcp_server.global_mcp_server_manager, "_get_tools_from_server", AsyncMock(side_effect=fake_get_tools) ): # Aggregate route: no explicit server filter, even though only one server is accessible. - tools = await mcp_server._get_tools_from_mcp_servers( + listing = await mcp_server._get_tools_from_mcp_servers( user_api_key_auth=UserAPIKeyAuth(token="h", user_id="u1"), mcp_auth_header=None, mcp_servers=None, ) - assert tools == [] + assert listing.tools == [] + assert listing.outcomes["delegate_docs"].tag == "auth_required" diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py index a983ac3ff48..099350d2e78 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py @@ -1094,8 +1094,10 @@ async def test_get_tools_from_mcp_servers_continues_when_one_server_fails(): ) # Verify that tools from the working server are returned - assert len(result) == 1 - assert result[0].name == "working_tool_1" + assert len(result.tools) == 1 + assert result.tools[0].name == "working_tool_1" + assert result.outcomes["working_server"].tag == "ok" + assert result.outcomes["failing_server"].tag == "internal" # Verify failure logging mock_logger.exception.assert_any_call( @@ -1188,7 +1190,9 @@ async def test_get_tools_from_mcp_servers_handles_all_servers_failing(): ) # Verify that empty list is returned - assert len(result) == 0 + assert len(result.tools) == 0 + assert result.outcomes["failing_server1"].tag == "internal" + assert result.outcomes["failing_server2"].tag == "internal" # Verify failure logging for both servers mock_logger.exception.assert_any_call( @@ -3074,7 +3078,7 @@ async def test_list_tools_single_server_unprefixed_names(): "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager", mock_manager, ): - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=None, mcp_servers=None, @@ -3082,8 +3086,8 @@ async def test_list_tools_single_server_unprefixed_names(): ) # Server prefix is always added regardless of number of allowed servers - assert len(tools) == 1 - assert tools[0].name == "zapier-toolA" + assert len(listing.tools) == 1 + assert listing.tools[0].name == "zapier-toolA" @pytest.mark.asyncio @@ -3153,7 +3157,7 @@ async def test_list_tools_multiple_servers_prefixed_names(): "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager", mock_manager, ): - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=None, mcp_servers=None, @@ -3161,7 +3165,7 @@ async def test_list_tools_multiple_servers_prefixed_names(): ) # Should be prefixed since multiple servers are allowed - names = sorted([t.name for t in tools]) + names = sorted([t.name for t in listing.tools]) assert names == ["jira-toolA", "zapier-toolA"] @@ -3437,7 +3441,7 @@ async def test_list_tools_filters_by_key_team_permissions(): "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager", mock_manager, ): - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=None, mcp_servers=None, @@ -3445,8 +3449,8 @@ async def test_list_tools_filters_by_key_team_permissions(): ) # Should only return tool1 and tool2 - assert len(tools) == 2 - tool_names = sorted([t.name for t in tools]) + assert len(listing.tools) == 2 + tool_names = sorted([t.name for t in listing.tools]) assert tool_names == ["tool1", "tool2"] @@ -3553,7 +3557,7 @@ async def test_list_tools_with_team_tool_permissions_inheritance(): "litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp.MCPRequestHandler._get_team_object_permission", AsyncMock(return_value=team_object_permission), ): - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=None, mcp_servers=None, @@ -3561,8 +3565,8 @@ async def test_list_tools_with_team_tool_permissions_inheritance(): ) # Should only return tool2 and tool3 (intersection of key and team permissions) - assert len(tools) == 2 - tool_names = sorted([t.name for t in tools]) + assert len(listing.tools) == 2 + tool_names = sorted([t.name for t in listing.tools]) assert tool_names == ["tool2", "tool3"] @@ -3640,7 +3644,7 @@ async def test_list_tools_with_no_tool_permissions_shows_all(): "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager", mock_manager, ): - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=None, mcp_servers=None, @@ -3648,8 +3652,8 @@ async def test_list_tools_with_no_tool_permissions_shows_all(): ) # Should return all tools when no restrictions - assert len(tools) == 3 - tool_names = sorted([t.name for t in tools]) + assert len(listing.tools) == 3 + tool_names = sorted([t.name for t in listing.tools]) assert tool_names == ["tool1", "tool2", "tool3"] @@ -3746,7 +3750,7 @@ async def test_list_tools_strips_prefix_when_matching_permissions(): "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager", mock_manager, ): - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_api_key_auth, mcp_auth_header=None, mcp_servers=None, @@ -3754,8 +3758,8 @@ async def test_list_tools_strips_prefix_when_matching_permissions(): ) # Should only return the 2 tools that match (after stripping prefix) - assert len(tools) == 2 - tool_names = sorted([t.name for t in tools]) + assert len(listing.tools) == 2 + tool_names = sorted([t.name for t in listing.tools]) # Tools still have prefixes in the output, but were filtered correctly assert tool_names == [ "GITMCP-fetch_litellm_documentation", @@ -4278,7 +4282,7 @@ async def test_get_tools_from_mcp_servers_logs_list_tools_to_spendlogs_when_enab ): mock_manager._get_tools_from_server = AsyncMock(return_value=[tool_1]) - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_auth, mcp_auth_header=None, mcp_servers=["server_a"], @@ -4288,7 +4292,7 @@ async def test_get_tools_from_mcp_servers_logs_list_tools_to_spendlogs_when_enab request_tags=["team-a"], ) - assert tools == [tool_1] + assert listing.tools == [tool_1] dummy_logging_obj.async_success_handler.assert_awaited_once() assert dummy_logging_obj.async_success_handler.await_args.kwargs["result"] == [tool_1.model_dump(mode="json")] assert function_setup_kwargs["metadata"]["tags"] == ["team-a"] @@ -4297,6 +4301,7 @@ async def test_get_tools_from_mcp_servers_logs_list_tools_to_spendlogs_when_enab assert spend_meta["tool_count_total"] == 1 assert spend_meta["allowed_server_count"] == 1 assert spend_meta["per_server_tool_counts"]["server_a"] == 1 + assert spend_meta["per_server_list_outcomes"] == {"server_a": {"status": "ok", "tool_count": 1}} @pytest.mark.asyncio @@ -4359,7 +4364,7 @@ async def test_get_tools_from_mcp_servers_returns_tools_when_success_logging_fai ): mock_manager._get_tools_from_server = AsyncMock(return_value=[tool_1]) - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_auth, mcp_auth_header=None, mcp_servers=["server_a"], @@ -4368,7 +4373,7 @@ async def test_get_tools_from_mcp_servers_returns_tools_when_success_logging_fai list_tools_log_source="mcp_protocol", ) - assert tools == [tool_1] + assert listing.tools == [tool_1] dummy_logging_obj.async_success_handler.assert_awaited_once() @@ -4665,7 +4670,7 @@ async def test_get_tools_from_mcp_servers_injects_stored_oauth2_token(): ): mock_manager._get_tools_from_server = AsyncMock(return_value=[tool_1]) - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_auth, mcp_auth_header=None, mcp_servers=["atlassian_test"], @@ -4681,7 +4686,7 @@ async def test_get_tools_from_mcp_servers_injects_stored_oauth2_token(): call_kwargs = mock_manager._get_tools_from_server.await_args.kwargs assert call_kwargs["extra_headers"] == {"Authorization": f"Bearer {STORED_TOKEN}"} - assert tools == [tool_1] + assert listing.tools == [tool_1] # --------------------------------------------------------------------------- @@ -5207,7 +5212,7 @@ async def test_list_tools_with_legacy_db_m2m_server_resolves_oauth2_flow(): mock_manager.filter_server_ids_by_ip_with_info = MagicMock(return_value=(["legacy-m2m-id"], 0)) mock_manager._get_tools_from_server = AsyncMock(side_effect=capture_extra_headers) - tools = await _get_tools_from_mcp_servers( + listing = await _get_tools_from_mcp_servers( user_api_key_auth=user_auth, mcp_auth_header=None, mcp_servers=["legacy_m2m"], @@ -5222,7 +5227,7 @@ async def test_list_tools_with_legacy_db_m2m_server_resolves_oauth2_flow(): "P1 security issue: caller's Authorization header was forwarded to M2M server. " "Expected None, got: " + str(captured_extra_headers) ) - assert tools == [tool_1] + assert listing.tools == [tool_1] @pytest.mark.asyncio @@ -7437,3 +7442,123 @@ async def test_call_mcp_tool_skips_failure_hook_for_upstream_auth_error(): ) proxy_logging_mock.post_call_failure_hook.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_aggregate_listing_reports_per_server_outcomes(): + """A failed server must contribute a classified outcome, not just silently shrink the list: + without the outcome a broken upstream is indistinguishable from a healthy server with no tools.""" + try: + from litellm.proxy._experimental.mcp_server.server import ( + _get_tools_from_mcp_servers, + set_auth_context, + ) + except ImportError: + pytest.skip("MCP server not available") + + from litellm.proxy._experimental.mcp_server.exceptions import MCPServerListError + from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ServerListFault + + user_api_key_auth = UserAPIKeyAuth(api_key="test_key", user_id="test_user") + set_auth_context(user_api_key_auth) + + working_server = MagicMock() + working_server.name = "working_server" + working_server.alias = "working" + working_server.allowed_tools = None + working_server.disallowed_tools = None + working_server.server_id = "working_server" + working_server.server_name = "working_server" + working_server.auth_type = None + working_server.extra_headers = None + + broken_server = MagicMock() + broken_server.name = "broken_server" + broken_server.alias = "broken" + broken_server.allowed_tools = None + broken_server.disallowed_tools = None + broken_server.server_id = "broken_server" + broken_server.server_name = "broken_server" + broken_server.auth_type = None + broken_server.extra_headers = None + + mock_manager = MagicMock() + mock_manager.get_allowed_mcp_servers = AsyncMock(return_value=["working_server", "broken_server"]) + mock_manager.get_mcp_server_by_id = lambda server_id: ( + working_server if server_id == "working_server" else broken_server + ) + mock_manager.filter_server_ids_by_ip_with_info = lambda server_ids, client_ip: (server_ids, 0) + + async def mock_get_tools_from_server(server, **kwargs): + if server.name == "working_server": + tool1 = MagicMock() + tool1.name = "working_tool_1" + tool1.description = "Working tool 1" + tool1.inputSchema = {} + return [tool1] + raise MCPServerListError(ServerListFault(tag="upstream_error", status_code=500), server.name) + + mock_manager._get_tools_from_server = mock_get_tools_from_server + + with patch( + "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager", + mock_manager, + ): + listing = await _get_tools_from_mcp_servers( + user_api_key_auth=user_api_key_auth, + mcp_auth_header=None, + mcp_servers=["working_server", "broken_server"], + mcp_server_auth_headers=None, + ) + + assert [tool.name for tool in listing.tools] == ["working_tool_1"] + assert listing.outcomes["working_server"].tag == "ok" + assert listing.outcomes["working_server"].tool_count == 1 + assert listing.outcomes["broken_server"].tag == "upstream_error" + assert listing.outcomes["broken_server"].status_code == 500 + + +@pytest.mark.asyncio +async def test_handle_list_tools_attaches_outcome_meta(): + """The protocol handler returns a ListToolsResult whose _meta carries the per-server outcomes, + so MCP clients can tell a degraded listing from a genuinely empty one.""" + try: + from litellm.proxy._experimental.mcp_server.server import handle_list_tools + except ImportError: + pytest.skip("MCP server not available") + + from mcp.types import ListToolsResult, Tool + + from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ( + SERVER_OUTCOMES_META_KEY, + AggregateToolListing, + ServerListFault, + ServerListOk, + ) + + tool = Tool(name="t1", inputSchema={"type": "object"}) + listing = AggregateToolListing( + tools=[tool], + outcomes={"healthy": ServerListOk(tool_count=1), "broken": ServerListFault(tag="unreachable")}, + ) + + async def fake_auth_context(): + return (None, None, None, None, None, None, None) + + with ( + patch( + "litellm.proxy._experimental.mcp_server.server.get_or_extract_auth_context", + new=AsyncMock(return_value=(None, None, None, None, None, None, None)), + ), + patch( + "litellm.proxy._experimental.mcp_server.server._list_mcp_tools", + new=AsyncMock(return_value=listing), + ), + ): + result = await handle_list_tools() + + assert isinstance(result, ListToolsResult) + wire = result.model_dump(by_alias=True) + outcomes_meta = wire["_meta"][SERVER_OUTCOMES_META_KEY] + assert outcomes_meta["healthy"] == {"status": "ok", "tool_count": 1} + assert outcomes_meta["broken"] == {"status": "unreachable"} diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index adcfff6fe9d..dbad00d7baf 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -11,7 +11,11 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from fastapi import HTTPException -from litellm.proxy._experimental.mcp_server.exceptions import MCPUpstreamAuthError +from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPServerListError, + MCPUpstreamAuthError, +) +from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ServerListFault # Add the parent directory to the path so we can import litellm sys.path.insert(0, "../../../../../") @@ -824,9 +828,12 @@ class TestMCPServerManager: assert exc_info.value.www_authenticate == challenge @pytest.mark.asyncio - async def test_list_absorbs_non_auth_httpexception(self): - """A non-auth HTTP error (e.g. 412 no endpoint, 503 IdP down) must stay absorbed to [] so one - misconfigured/unavailable server does not blank the whole aggregate listing.""" + async def test_list_surfaces_non_auth_httpexception_as_internal_fault(self): + """A non-auth HTTP error (e.g. 412 no endpoint, 503 IdP down) now raises MCPServerListError + with an "internal" fault carrying the status code instead of absorbing to []: the silent + empty list made a misconfigured/unavailable server indistinguishable from a healthy server + with no tools. The aggregate absorbs it into that server's outcome, so one broken server + still does not blank the whole aggregate listing.""" server = MCPServer( server_id="te-412", name="te-412-server", @@ -841,10 +848,10 @@ class TestMCPServerManager: manager._create_mcp_client = AsyncMock( side_effect=HTTPException(status_code=412, detail="token exchange endpoint is not configured") ) - result = await manager._get_tools_from_server( - server=server, oauth2_headers={"Authorization": "Bearer subj-jwt"} - ) - assert result == [] + with pytest.raises(MCPServerListError) as exc_info: + await manager._get_tools_from_server(server=server, oauth2_headers={"Authorization": "Bearer subj-jwt"}) + assert exc_info.value.fault == ServerListFault(tag="internal", status_code=412) + assert exc_info.value.server_name == "te-412-server" def _upstream_status_error(self, status_code: int, www_authenticate: Optional[str] = None) -> httpx.HTTPStatusError: """Build an httpx.HTTPStatusError shaped like the one the MCP SDK surfaces for an upstream @@ -6862,14 +6869,16 @@ def _upstream_status_error(status_code: int, challenge: str) -> httpx.HTTPStatus class TestMCPToolsListAuthSurfacing: - """Regression: MCP tools/list 401 auth failures must surface as MCPUpstreamAuthError. + """Regression: MCP tools/list failures must surface as typed exceptions, never a silent []. Previously a missing/expired per-user OAuth token, or an upstream 401 for any non-carveout auth_type, was swallowed to an empty tool list, so a single-server client saw a 200 with no tools instead of a 401 challenge. The listing helpers - now raise MCPUpstreamAuthError on a 401 regardless of auth_type; the single-server - routes turn it into a 401 + WWW-Authenticate while the aggregator absorbs it to an - empty list. Only a 401 challenges; a 403 (forbidden) degrades like any other error. + now raise MCPUpstreamAuthError on an upstream 401 or 403 and MCPServerListError + with a classified fault for every other failure; single-server routes relay a + truthful HTTP status while the aggregator absorbs each failure into that + server's outcome, so a broken upstream is never indistinguishable from a + healthy server with no tools. """ @pytest.mark.asyncio @@ -6891,25 +6900,38 @@ class TestMCPToolsListAuthSurfacing: assert exc_info.value.server_name == "static-key-server" @pytest.mark.asyncio - async def test_fetch_tools_with_timeout_absorbs_upstream_403(self): - """Only a 401 drives the re-auth challenge. A 403 (authenticated but - forbidden, e.g. insufficient scope) is not a re-auth signal, so even - with a WWW-Authenticate header it degrades to an empty list rather than - surfacing a challenge.""" + async def test_fetch_tools_with_timeout_surfaces_upstream_403(self): + """An upstream 403 (authenticated but forbidden, e.g. insufficient scope) now raises + MCPUpstreamAuthError instead of absorbing to []: the silent empty list made a forbidden + upstream indistinguishable from a healthy server with no tools. The upstream + WWW-Authenticate is preserved so single-server routes can relay the real challenge.""" manager = MCPServerManager() challenge = 'Bearer error="insufficient_scope", scope="read:tools"' client = MagicMock() client.list_tools = AsyncMock(side_effect=_upstream_status_error(403, challenge)) - assert await manager._fetch_tools_with_timeout(client, "forbidden-server") == [] + with pytest.raises(MCPUpstreamAuthError) as exc_info: + await manager._fetch_tools_with_timeout(client, "forbidden-server") + + assert exc_info.value.status_code == 403 + assert exc_info.value.www_authenticate == challenge + assert exc_info.value.server_name == "forbidden-server" @pytest.mark.asyncio - async def test_fetch_tools_with_timeout_returns_empty_on_non_auth_error(self): + async def test_fetch_tools_with_timeout_raises_classified_fault_on_non_auth_error(self): + """A non-auth listing failure now raises MCPServerListError carrying a classified fault + instead of absorbing to []: the silent empty list made a broken upstream indistinguishable + from a healthy server with no tools. An unrecognized exception classifies as the gateway's + own fault ("internal").""" manager = MCPServerManager() client = MagicMock() client.list_tools = AsyncMock(side_effect=RuntimeError("upstream 500")) - assert await manager._fetch_tools_with_timeout(client, "srv") == [] + with pytest.raises(MCPServerListError) as exc_info: + await manager._fetch_tools_with_timeout(client, "srv") + + assert exc_info.value.fault == ServerListFault(tag="internal") + assert exc_info.value.server_name == "srv" @pytest.mark.asyncio async def test_get_tools_from_server_surfaces_unusable_user_token(self): @@ -6936,9 +6958,12 @@ class TestMCPToolsListAuthSurfacing: assert exc_info.value.server_name == "oauth-srv" @pytest.mark.asyncio - async def test_get_tools_from_server_absorbs_non_challenge_http_error(self): - """A non-auth HTTPException (500) stays absorbed so one misconfigured server cannot blank - the listing; 401/403 are the challenge-class statuses routed to MCPUpstreamAuthError.""" + async def test_get_tools_from_server_surfaces_non_challenge_http_error_as_internal_fault(self): + """A non-auth HTTPException (500) now raises MCPServerListError with an "internal" fault + carrying the status code instead of absorbing to []: the silent empty list made a + misconfigured server indistinguishable from a healthy server with no tools. The aggregate + absorbs it into that server's outcome; single-server routes relay a truthful status. + 401/403 remain the challenge-class statuses routed to MCPUpstreamAuthError.""" manager = MCPServerManager() server = MCPServer(server_id="stdio-srv", name="stdio-srv", transport=MCPTransport.http) manager._create_mcp_client = AsyncMock( @@ -6948,7 +6973,11 @@ class TestMCPToolsListAuthSurfacing: ) ) - assert await manager._get_tools_from_server(server) == [] + with pytest.raises(MCPServerListError) as exc_info: + await manager._get_tools_from_server(server) + + assert exc_info.value.fault == ServerListFault(tag="internal", status_code=500) + assert exc_info.value.server_name == "stdio-srv" @pytest.mark.asyncio async def test_get_tools_from_server_suppresses_upstream_challenge_for_dcr_bridge(self): diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_tool_search.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_tool_search.py index b8f0b205831..1da44029b5c 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_tool_search.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_tool_search.py @@ -16,6 +16,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from litellm.models.object_permission import LiteLLM_ObjectPermissionTable +from litellm.proxy._experimental.mcp_server.faults.list_outcomes import AggregateToolListing from litellm.proxy._experimental.mcp_server.tool_search import ( MCP_TOOL_CALL_TOOL_NAME, MCP_TOOL_SEARCH_TOOL_NAME, @@ -381,7 +382,7 @@ class TestCallToolRestApiVirtualTools: with patch( "litellm.proxy._experimental.mcp_server.server._list_mcp_tools", new_callable=AsyncMock, - return_value=[mock_tool], + return_value=AggregateToolListing(tools=[mock_tool], outcomes={}), ): result = await self._get_call_fn()( request=request, @@ -508,7 +509,7 @@ class TestCallToolRestApiVirtualTools: patch( "litellm.proxy._experimental.mcp_server.server._list_mcp_tools", new_callable=AsyncMock, - return_value=[], + return_value=AggregateToolListing(tools=[], outcomes={}), ) as mock_list, ): await self._get_call_fn()(request=request, user_api_key_dict=user_api_key_dict) diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py index 99e05182361..010ab614421 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py @@ -690,16 +690,16 @@ class TestListToolsRestAPI: ) request = _build_request(path="/mcp-rest/tools/list", method="GET") - result = await rest_endpoints.list_tool_rest_api( - request, - server_id="server-1", - user_api_key_dict=UserAPIKeyAuth(), - ) + with pytest.raises(HTTPException) as exc_info: + await rest_endpoints.list_tool_rest_api( + request, + server_id="server-1", + user_api_key_dict=UserAPIKeyAuth(), + ) - assert result["tools"] == [] - assert result["error"] == "unexpected_error" - assert "access_denied" in result["message"] - assert "server server-1" in result["message"] + assert exc_info.value.status_code == 403 + assert exc_info.value.detail["error"] == "access_denied" + assert "server-1" in exc_info.value.detail["message"] async def test_lists_tools_for_allowed_server(self, monkeypatch): async def fake_contexts(user_api_key_auth): @@ -911,6 +911,63 @@ class TestListToolsRestAPI: assert exc_info.value.status_code == upstream_status assert exc_info.value.headers == {"www-authenticate": challenge} + async def test_single_server_upstream_fault_surfaces_truthful_status(self, monkeypatch): + """A single-server listing whose upstream breaks (5xx, timeout, unreachable) must answer + with the truthful gateway status instead of masking the failure as an empty-success + {"tools": [], "error": null} body a caller cannot distinguish from a toolless server.""" + from litellm.proxy._experimental.mcp_server.exceptions import ( + MCPServerListError, + ) + from litellm.proxy._experimental.mcp_server.faults.list_outcomes import ( + ServerListFault, + ) + + class StubServer: + alias = "server-1" + server_name = "server-1" + name = "flaky" + allowed_tools = None + mcp_info = {"server_name": "flaky"} + available_on_public_internet = True + + stub_server = StubServer() + + async def fake_contexts(user_api_key_auth): + return [user_api_key_auth] + + async def fake_get_allowed_mcp_servers(*args, **kwargs): + return ["server-1"] + + async def fake_get_tools(*args, **kwargs): + raise MCPServerListError(ServerListFault(tag="upstream_error", status_code=503), "flaky") + + monkeypatch.setattr(rest_endpoints, "build_effective_auth_contexts", fake_contexts, raising=False) + monkeypatch.setattr( + rest_endpoints.global_mcp_server_manager, + "get_allowed_mcp_servers", + fake_get_allowed_mcp_servers, + raising=False, + ) + monkeypatch.setattr( + rest_endpoints.global_mcp_server_manager, + "get_mcp_server_by_id", + lambda server_id: stub_server if server_id == "server-1" else None, + raising=False, + ) + monkeypatch.setattr(rest_endpoints, "_get_tools_for_single_server", fake_get_tools, raising=False) + + request = _build_request(path="/mcp-rest/tools/list", method="GET") + with pytest.raises(HTTPException) as exc_info: + await rest_endpoints.list_tool_rest_api( + request, + server_id="server-1", + user_api_key_dict=UserAPIKeyAuth(), + ) + + assert exc_info.value.status_code == 502 + assert exc_info.value.detail["error"] == "upstream_error" + assert "flaky" in exc_info.value.detail["message"] + 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 @@ -1108,15 +1165,15 @@ class TestListToolsRestAPI: ) request = _build_request(path="/mcp-rest/tools/list", method="GET") - result = await rest_endpoints.list_tool_rest_api( - request, - server_id="restricted-server", - user_api_key_dict=UserAPIKeyAuth(), - ) + with pytest.raises(HTTPException) as exc_info: + await rest_endpoints.list_tool_rest_api( + request, + server_id="restricted-server", + user_api_key_dict=UserAPIKeyAuth(), + ) - assert result["tools"] == [] - assert result["error"] == "unexpected_error" - assert "access_denied" in result["message"] + assert exc_info.value.status_code == 403 + assert exc_info.value.detail["error"] == "access_denied" async def test_mcp_server_name_query_param_resolves_to_server(self, monkeypatch): """mcp_server_name is a name-based alias for server_id: it should diff --git a/tests/test_litellm/responses/mcp/test_litellm_proxy_mcp_handler.py b/tests/test_litellm/responses/mcp/test_litellm_proxy_mcp_handler.py index 6fdbb0741aa..a1347aa111c 100644 --- a/tests/test_litellm/responses/mcp/test_litellm_proxy_mcp_handler.py +++ b/tests/test_litellm/responses/mcp/test_litellm_proxy_mcp_handler.py @@ -8,6 +8,7 @@ import pytest from fastapi import HTTPException import importlib +from litellm.proxy._experimental.mcp_server.faults.list_outcomes import AggregateToolListing from litellm.responses.mcp.litellm_proxy_mcp_handler import ( LiteLLM_Proxy_MCP_Handler, ) @@ -455,7 +456,7 @@ async def test_get_mcp_tools_from_manager_enables_list_tools_logging(monkeypatch Regression test for 872e5b98...: Ensure responses-side tool discovery enables list-tools SpendLogs logging flags. """ - mock_get_tools = AsyncMock(return_value=[]) + mock_get_tools = AsyncMock(return_value=AggregateToolListing(tools=[], outcomes={})) monkeypatch.setattr( "litellm.proxy._experimental.mcp_server.server._get_tools_from_mcp_servers", mock_get_tools, @@ -509,7 +510,7 @@ def test_get_parent_request_tags_from_nested_litellm_params(): @pytest.mark.asyncio async def test_get_mcp_tools_from_manager_forwards_request_tags(monkeypatch): - mock_get_tools = AsyncMock(return_value=[]) + mock_get_tools = AsyncMock(return_value=AggregateToolListing(tools=[], outcomes={})) monkeypatch.setattr( "litellm.proxy._experimental.mcp_server.server._get_tools_from_mcp_servers", mock_get_tools,