feat(mcp): per-server outcomes for aggregate tools/list and truthful single-server REST statuses - #33153
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryPhase 2 of the MCP error-handling framework replaces silent empty-list absorbs with classified per-server outcomes, so a broken upstream is never indistinguishable from a healthy server with no tools. Single-server REST routes now relay truthful HTTP statuses (502/504/500/403) instead of returning
Confidence Score: 5/5Safe to merge; the fan-out aggregate and single-server REST paths are both thoroughly tested, the exception traversal ordering is pinned by dedicated tests, and callers that only needed the tool list are correctly adapted to the new AggregateToolListing wrapper. The classification logic is total (every exception maps to exactly one outcome), the dcr_bridge challenge suppression is correctly threaded through both fetch-time and client-build-time error paths, and all test changes reflect intentional behavior improvements rather than masking existing failures. The MCP protocol and REST paths each apply the correct policy at their boundary. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/faults/list_outcomes.py | New module providing total, centralized classification of per-server listing failures into typed outcomes; traversal order (cause before ExceptionGroup members before context) and wire representation are both correct and well-tested. |
| litellm/proxy/_experimental/mcp_server/exceptions.py | Adds MCPServerListError as a typed carrier for classified faults; typing fault as object to break the circular import with faults/ is clearly documented. |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | _fetch_tools_with_timeout and _get_tools_from_server now raise typed exceptions instead of silently returning []; _extract_upstream_auth_failure delegates to the shared traversal in faults/list_outcomes.py; suppress_challenge for dcr_bridge servers is correctly applied in the _get_tools_from_server generic arm. |
| litellm/proxy/_experimental/mcp_server/rest_endpoints.py | Single-server REST requests now propagate truthful HTTP statuses (502/504/500/403) via the new MCPServerListError handler; the HTTPException re-raise condition correctly gates which path relays the real status vs the legacy error-dict shape. |
| litellm/proxy/_experimental/mcp_server/server.py | _get_tools_from_mcp_servers, _list_mcp_tools, and handle_list_tools updated to return AggregateToolListing; _aggregate_server_key correctly keys outcomes by display prefix (alias) rather than canonical server name. |
| tests/test_litellm/proxy/_experimental/mcp_server/faults/test_list_outcomes.py | New test file comprehensively covers the classification matrix, traversal ordering, wire value format, and HTTP status mapping for every fault category. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Existing tests updated to match the new raise-instead-of-return-[] contract; all changes reflect intentional behavior improvements, not regression masking. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py | Access-denied and upstream-fault tests correctly updated from 200+error-dict expectations to HTTPException with real status codes. |
| litellm/proxy/_experimental/mcp_server/tool_search.py | Correctly unwraps AggregateToolListing.tools before processing. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Correctly adapts to the new AggregateToolListing return type by extracting .tools before serialization. |
| litellm/responses/mcp/litellm_proxy_mcp_handler.py | Correctly updated to extract .tools from the AggregateToolListing; test mocks updated to return the new type. |
Reviews (9): Last reviewed commit: "fix(mcp): key every caller-visible listi..." | Re-trigger Greptile
|
bugbot run |
|
@greptileai rereview |
|
bugbot run |
…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
…a healthy empty server A cancelled fetch absorbed to [] made that server contribute ServerListOk(tool_count=0), the exact healthy-but-empty impostor this change removes. Cancellation stays suppressed (the pre-existing choice); it now carries an internal fault so outcomes stay truthful
…ding the upstream response
…m listing failures Both review findings shared one root cause: two exception-tree walkers with drifted semantics. _extract_upstream_auth_failure walked the incidental __context__ chain before explicit causes, so a 403 raised while handling the causal 401 could shadow it; and the generic _get_tools_from_server arm classified without extracting the challenge, so a nested 401 at client-build time surfaced without the WWW-Authenticate the client needs. upstream_auth_challenge and raise_classified_list_failure in faults/list_outcomes.py are now the single traversal and the single choice-point; both fetch arms and _extract_upstream_auth_failure (also serving tool calls and the connect-time probe) delegate to them, with dcr_bridge challenge suppression as a parameter so it holds on every path. The stale _fetch_tools_with_timeout docstring describing the pre-change 403 absorb is rewritten to the actual contract: 403 relays with its own status, an upstream-sent challenge relays verbatim per RFC 6750 insufficient_scope, and a challenge is only ever fabricated for a challenge-less 401
192b502 to
c6d6567
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c6d6567. Configure here.
The consolidation regressed the pre-existing walker semantics: _extract_upstream_auth_failure used to keep scanning until it found a 401/403, while the consolidated helper took the first response of any status and then tested it, so a causal 401 sitting behind an unrelated 5xx (retry attempts, multi-stream task groups) was misclassified as upstream_error and its challenge lost on the listing, tool-call, and probe paths. The traversal is now an iterator in deliberate order and each consumer applies its predicate over the stream: the auth scan takes the first 401/403 even behind non-auth responses, generic classification takes the first response, and classify_list_exception derives its auth arm from the same scan so the carrier choice and the classification can never disagree
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d0ee110. Configure here.
…omes Append-append conflict at the end of test_mcp_server.py between this branch's aggregate-outcome tests and the mode-aware preemptive-401 tests from staging; both kept
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 98bf25e. Configure here.
…omes Conflict in _list_mcp_tools: staging (#33612) moved toolset-grant expansion into the shared permission primitives and removed the _merge_toolset_permissions call; resolution applies that removal to this branch's AggregateToolListing structure
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5de0340. Configure here.
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
…fix, never canonical names Outcome keys in the tools/list _meta, the spend-log outcome and count maps, and the REST error messages now all use get_server_prefix (alias, or the short prefix when that mode is enabled), the same naming the caller already sees on tool names. Keying them by canonical server_name let an authenticated caller enumerate internal server names and their health or auth state that the alias and short-prefix schemes deliberately hide (Veria finding). One helper decides the key for every surface; exception messages reaching the multi-server REST error list are mapped to their fault tag with the display prefix instead of relaying exception text carrying canonical names. Server-side logs keep the real names
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cf08c07. Configure here.
…dation Staging now contains #33153, whose final rounds made _extract_upstream_auth_failure a thin delegate to upstream_auth_challenge and introduced the response-level iterator this branch predates. The resolution completes the consolidation both branches were converging on: iter_exception_tree (faults/traversal.py) is the one tree walk, _iter_upstream_responses is rebuilt on top of it instead of carrying a second copy of the traversal, the manager keeps the delegate, and the semantic filter port from this branch stands. Test conflicts were append-append and both sides are kept

Relevant issues
Builds on #33113 (phase 1 of the MCP error-handling framework, merged)
Behavior changes at a glance
_meta["litellm.ai/server_outcomes"]carries each server's outcome, e.g.{"healthy_wiki": {"status": "ok", "tool_count": 3}, "broken_box": {"status": "unreachable"}}auth_required/forbiddenwith the statusper_server_tool_countsonly; a failed server logs0like an empty oneper_server_list_outcomeswith the classified outcome per server200 {"tools": [], "error": null, "message": "Successfully retrieved tools"}{"error": <category>, "message": ...}200 {"tools": [], "error": "unexpected_error", "message": "... access_denied ..."}{"error": "access_denied", ...}MCPUpstreamAuthErrorwith its own status (forbiddenoutcome in the aggregate, real 403 on single-server routes). An upstream-sentWWW-Authenticaterelays verbatim since a 403 challenge is the RFC 6750 insufficient_scope scope-step-up; a challenge is never fabricated for a 403, only for a challenge-less 401Outcome wire values carry only a category and HTTP status; upstream prose and URLs never cross (same trust-boundary rule as phase 1). The multi-server REST aggregate keeps its legacy
{"tools", "error", "message"}dict shapeLinear ticket
Resolves LIT-4421
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Rig: proxy on localhost:4000 backed by Postgres with two registered servers;
healthy_wikipoints at the real public deepwiki MCP server (https://mcp.deepwiki.com/mcp),broken_boxpoints at an unreachable address. The aggregate call is a real streamable-HTTP handshake (initialize, notifications/initialized, tools/list) via curlBefore (at b70de76, the pre-change state including phase 1): the aggregate lists the 3 deepwiki tools and nothing else; the broken server leaves no trace
and the single-server REST list on the broken server reports success
After (at dbcc52e): same healthy tools, and the listing now says what happened to every server
and the broken server's single-server REST list answers truthfully
The healthy server's single-server REST list still returns HTTP 200 with its tools
Type
🆕 New Feature
🐛 Bug Fix
Changes
Phase 2 of the MCP error-handling framework (LIT-4419; phase 1 is #33113). The framework rule applied here: a fan-out read may drop a member's data only if it emits that member's outcome somewhere machine-readable, and single-upstream operations relay truthfully
New module
mcp_server/faults/list_outcomes.py: frozenServerListOk/ServerListFaultoutcome values on a category literal (auth_required,forbidden,timeout,unreachable,upstream_error,internal), a totalclassify_list_exception(walks exception trees for embedded upstream responses; anything unrecognized is the gateway's owninternalfault, never a re-raise), the wire form (outcome_wire_value, category plus status only), and the truthful status map for single-upstream requests.MCPServerListError(exceptions.py) carries a classified fault through the existing exception channel, the same pattern asMCPUpstreamAuthErrorMCPServerManager._fetch_tools_with_timeoutand_get_tools_from_serverstop absorbing failures into empty lists; they raise the classified carrier (upstream 401 and now also 403 keep raisingMCPUpstreamAuthErrorwith the challenge). Every boundary then chooses per the policy matrix: the aggregate_fetch_and_filter_server_toolsabsorbs into a per-server outcome;_get_tools_from_mcp_serversand_list_mcp_toolsreturn anAggregateToolListing(tools plus outcomes);handle_list_toolsreturns aListToolsResultwhose_metacarries the outcomes (the MCP SDK passes a ListToolsResult through unwrapped, which is what lets_metareach the client; verified against mcp 1.26.0); spend logs gainper_server_list_outcomes; the single-server REST path relays the truthful status; the legacylist_tools/get_tools_for_server/startup-mapping callers keep absorbing at their own levelThe
_metakey islitellm.ai/server_outcomes, following the MCP spec's prefixed_metakey format. The MCP spec (through the 2026-07-28 draft) defines no aggregator partial-failure reporting; the design note for this choice, with the GraphQL partial-data precedent, is in the framework design docDeliberately deferred, tracked on LIT-4421: recording the last outcome onto the server table's health fields (a DB write per tools/list is not acceptable; needs a cached map) and the OBO classifier backport
Review findings were folded in as structure rather than spot patches. Both bugbot findings shared one root cause, two exception-tree walkers with drifted semantics, so the fix is consolidation:
upstream_auth_challengeandraise_classified_list_failurein faults/list_outcomes.py are now the single traversal and the single carrier choice-point (status and challenge always read from the same response, cause-first order so an incidental 403 raised while handling the causal 401 can never shadow it), and both fetch arms plus_extract_upstream_auth_failure(which also serves the tool-call path and the connect-time probe) delegate to them. The dcr_bridge challenge suppression is a parameter of the choice-point so it holds on every path. The stale_fetch_tools_with_timeoutdocstring describing the pre-change 403 absorb was rewritten to the actual contractTest updates that pin the new contract rather than the old absorb behavior:
test_fetch_tools_with_timeout_absorbs_upstream_403became..._surfaces_upstream_403,..._returns_empty_on_non_auth_errorbecame..._raises_classified_fault_on_non_auth_error, the REST access-denied tests now expect the real 403, and the aggregate tests assert outcomes alongside the healthy subset. Suite result: 2227 passed, 0 failed across the MCP test treeFinal Attestation
Note
Medium Risk
Changes observable MCP and REST listing contracts (status codes,
_meta, and error types) across aggregate and single-server paths, including auth challenge relay; behavior is heavily tested but clients depending on silent empty lists or legacy 200 error bodies may need updates.Overview
MCP tools/list no longer treats a failed upstream as an empty tool list. Listing failures raise
MCPServerListError/MCPUpstreamAuthErrorwith classified faults (auth_required,forbidden,timeout,unreachable,upstream_error,internal), centralized in newfaults/list_outcomes.py(shared exception-tree auth scanning andraise_classified_list_failure).On the aggregate MCP path, healthy servers still contribute tools; each server’s outcome is exposed in
_meta["litellm.ai/server_outcomes"]viaListToolsResult, and spend logs addper_server_list_outcomes. Single-server REST/mcp-rest/tools/listreturns matching HTTP statuses (e.g. 502/504/403) instead of200with empty tools; access denials propagate as real 403s.Upstream 403 during listing now follows the auth channel (with challenge relay where applicable) rather than being absorbed silently. Callers that aggregated tools now use
AggregateToolListing.toolsinstead of a bare list.Reviewed by Cursor Bugbot for commit cf08c07. Bugbot is set up for automated code reviews on this repo. Configure here.