feat(mcp): graft v2 resolver onto _create_mcp_client (none + api_key static family) - #31058
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR grafts the v2 credential resolver onto
Confidence Score: 5/5Safe to merge — the live modes are parity-verified against real upstreams, all deferred modes fall through to v1 unchanged, and the completeness guard is restored. The graft is narrow and well-tested: migrated modes resolve through the injected provider and are covered end-to-end; deferred modes, stdio, and the per-request-override path each have dedicated tests that confirm v1 fallback is intact. The one defensive gap (resolved_auth potentially unbound if a non-conforming provider returns an unexpected type) cannot be triggered by any current production or test code path, as every existing provider always returns Ok or Error. mcp_server_manager.py — the resolved_auth variable could be unbound if a custom UpstreamCredentialProvider returns something other than Ok or Error.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/outbound_credentials/adapter.py | New v1↔v2 bridge: exhaustive match over MCPAuth with assert_never tail, correct base64 encoding for Basic auth, early BYOK/passthrough deferral guards, and raise_public mapping CredError tags to HTTP statuses. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/resolver.py | Fills none (→ NoOpAuth) and api_key shared-key (→ StaticHeaderAuth) arms; Byok and every other arm remain not_implemented stubs with correct assert_never tail. |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Grafts v2 resolver onto HTTP/SSE branch of _create_mcp_client; resolved_auth variable is only assigned inside case Ok, leaving it potentially unbound if a non-conforming provider returns an unexpected value. |
| litellm/experimental_mcp_client/client.py | Adds resolved_auth parameter; fallback chain resolved_auth → aws_auth is correct, and _get_auth_headers() (v1 path) is unchanged. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_resolver.py | Restored test_every_auth_spec_kind_is_exercised completeness guard: live_kinds |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Comprehensive graft tests: migrated modes attach resolved_auth, deferred modes fall back to v1 auth_value, stdio stays on v1, resolver error maps to 401, per-request override and extra-header conflict are both covered. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_adapter.py | New adapter tests cover all migrated modes, the full defer table (including BYOK across all auth_types), to_subject mapping, and raise_public status codes for all six CredError tags. |
| tests/mcp_tests/test_mcp_auth_priority.py | Updated to verify the auth header via auth_flow (v2 path) rather than _get_auth_headers(); correctly checks the emitted Authorization header for the bearer_token mode. |
| tests/test_litellm/experimental_mcp_client/test_mcp_client.py | New TestMCPClientResolvedAuth class covers the resolved_auth→aws_auth precedence and the fallback-to-aws_auth case. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/init.py | Docstring updated to reflect that none and api_key modes are now live on the request path; public exports unchanged. |
| tests/mcp_tests/test_mcp_server.py | Replaces MagicMock() with a real UserAPIKeyAuth() instance for list_tools, fixing an attribute-access issue exposed by the v2 adapter's to_subject call. |
Reviews (7): Last reviewed commit: "test(mcp): restore the AuthSpecKind comp..." | Re-trigger Greptile
6444639 to
39a8799
Compare
e57309b to
4a56c5c
Compare
|
Addressed: removed |
4921883 to
d166fa2
Compare
|
Updated since the last review: removed the redundant |
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 |
b9200be to
ed4c55d
Compare
Note: deleting the v1 static-credential code is gated by the override-on-static residual, not just per-mode migrationThis came up reviewing what PR4 can delete. Recording it so the final v1 retirement isn't a surprise. PR4 deletes zero v1 code, by design. The graft routes the
The two precedence facets, and why they differ:
When the static v1 code becomes deletable. The
Point 2 is the key correction: finishing the per-mode PRs does not auto-delete the override pre-check, because a client can send Unchanged from the plan: per-mode v1 branches (token_exchange, client_credentials, etc.) still delete incrementally in their own PR. stdio's |
|
@greptileai please re-review the latest commit. Since the last review this now covers the graft into Generated by Claude Code |
|
@greptileai please re-review the latest commit. Fixed the stale module docstring in On the other nit (the Generated by Claude Code |
|
@greptileai please re-review the latest commit. Updated the Generated by Claude Code |
|
@greptileai please re-review the latest commit. Restored the Generated by Claude Code |
|
bugbot run Generated by Claude Code |
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 7c11148. Configure here.
mateo-berri
left a comment
There was a problem hiding this comment.
One comment but otherwise lgtm! Great stuff 💯
PR4a of the MCP v2 outbound-credential migration, stacked on the resolver skeleton. Builds the bridge for the first live modes without wiring it onto the request path: - resolver.py: the none arm (NoOpAuth) and the api_key shared-key arm (StaticHeaderAuth from the config); the BYOK source and the other five arms stay not_implemented. - adapter.py: the v1 <-> v2 edge (to_subject, to_server_spec, raise_public, should_defer). to_server_spec maps only none + the static-header family and returns None to defer every other mode to v1. Imports v1, kept out of the package __init__ so the resolver core stays v1-free. - MCPClient gains an optional resolved_auth that feeds the factory's auth= slot, taking precedence over the SigV4 aws_auth; default None keeps current behavior. Nothing calls these from _create_mcp_client yet, so production behavior is unchanged; the graft lands in PR4b. Unit tests cover the two arms, the full mapping table, and the auth plumbing.
Wire the none + api_key static-family resolver arms from PR4a onto v1's live request path. In _create_mcp_client's HTTP/SSE branch, to_server_spec decides per mode: a migrated mode resolves through the injected UpstreamCredentialProvider and feeds the resulting httpx.Auth into the new resolved_auth slot; every other mode returns None and falls through to the unchanged v1 construction. resolve_mcp_auth now runs only when the mode defers, so a migrated server skips the v1 token-exchange / M2M I/O. stdio is untouched: auth_type/auth_value never reach the upstream on the stdio path (_get_auth_headers is HTTP/SSE only), so there is nothing to graft there. No v1 code is deleted yet; resolve_mcp_auth's static return still backs stdio and the not-yet-migrated modes until later PRs retire it.
Regression tests for the PR4 graft. Migrated HTTP modes resolve through the provider into resolved_auth: none -> NoOpAuth, and the static api_key family emits the right header per scheme (X-API-Key, Bearer, token, raw authorization, base64 basic). Deferred modes (oauth2) and a missing static token fall back to v1's auth_value. A stdio server with a migrated auth_type still defers to v1, since httpx.Auth never reaches the subprocess. A resolver Error is mapped to the public HTTP contract (401) via an injected provider, exercising the DI seam.
The graft attaches the resolved static credential as an httpx.Auth, whose auth flow writes its header after extra_headers. That silently overrode an inbound Authorization: a per-request mcp_auth_header override, or a header supplied via a guardrail hook / static_headers / forwarded caller header. v1 lets those win, so the graft had inverted the credential precedence for the migrated static modes. Mirror the v2 egress credential-isolation invariant: defer the request to v1 when mcp_auth_header is set, or when the header the resolved credential would write is already present in extra_headers. none writes no header, so it never defers.
Regression tests for the precedence fix. A per-request mcp_auth_header override and an Authorization already present in extra_headers (guardrail hook like the JWT signer, static_headers, or a forwarded caller header) both defer a migrated static server to v1 so the inbound credential wins; none stays on v2 and does not clobber an inbound Authorization since NoOpAuth writes nothing. The deferred cases assert resolved_auth is None, which fails if the guard is removed.
…ring For an Authorization already supplied via extra_headers (a guardrail hook such as the JWT signer, static_headers, or a forwarded caller header), keep the request on the v2 path and skip resolved_auth rather than deferring to v1. The inbound header still wins since nothing overwrites it, but hooks no longer pin a v1 fallback, which is what lets resolve_mcp_auth be retired once the remaining modes migrate. The mcp_auth_header per-request override still defers to v1, since that value becomes the upstream credential rather than sitting in extra_headers; that defer falls away once the per-user modes stop writing mcp_auth_header.
…e graft adapter.py uses `from __future__ import annotations`, so the quoted "UserAPIKeyAuth" / "MCPServer" annotations in to_subject/to_server_spec/_shared_key_spec were unnecessary and pushed UP037 over the strict-rule budget; drop the quotes. test_list_tools_only_returns_allowed_servers passed a MagicMock as user_api_key_auth. The graft now builds a Subject from the principal, and the MagicMock's non-string org_id/user_id fail Subject validation, so the listing came back empty. Use a real UserAPIKeyAuth instead (MagicMock for an injected dependency was the anti-pattern here).
test_mcp_server_config_auth_value_header_used inspected _get_auth_headers(), but the graft now carries the static credential on the client's httpx.Auth (resolved_auth) and writes the header at send time, so that dict is empty. Assert the header the StaticHeaderAuth emits onto the request instead. Both config keys (authentication_token, auth_value) stay covered.
63cb433 to
50bd8b5
Compare
The previous slack of 3 put the ceiling at baseline + slack = 4, so a newly non-exhaustive match (for instance dropping an Error arm off a Result match) could land without tripping the gate. Setting slack to 0 pins the ceiling at the current baseline of 1, so any added non-exhaustive match now fails CI while the one pre-existing violation in router.py stays within budget
Relevant issues
PR4 of the MCP v2 outbound-credential migration (the lean "Mini PR" track). Stacked on PR #31056 (base
litellm_mcp_v2_resolver_skeleton) so this diff shows only the bridge, the first two live arms, and the graft that puts them on the request path. This lands the first live modes end to end: it builds the bridge, fills thenoneand shared-keyapi_keyarms, and grafts the resolver into_create_mcp_clientso those modes resolve through v2 while every other mode defers to v1 unchangedLinear ticket
N/A (groundwork for MCP V2)
Pre-Submission checklist
make test-unitScreenshots / Proof of Fix
The graft is live (no flag), so it is curl-able on a running proxy through the REST surface, which routes through
_create_mcp_client. Real upstream MCP servers were configured (through the UI, plus one inconfig.yamlfor theauthorizationmode the UI does not surface), one per migrated arm plus two oauth2 servers that must defer to v1:dw->https://mcp.deepwiki.com/mcp,auth_type: none(thenone/NoOpAutharm)github->https://api.githubcopilot.com/mcp/,auth_type: bearer_token(staticapi_keyfamily;StaticHeaderAuthwritingAuthorization: Bearer <token>)github_authz-> same upstream,auth_type: authorization(staticapi_keyfamily;StaticHeaderAuthwriting the token verbatim, no prefix)linear,slack->auth_type: oauth2(not migrated; defer to v1 unchanged)none arm (NoOpAuth) against a real no-auth upstream:
static bearer arm (StaticHeaderAuth) against a real authenticated upstream:
authorization arm (StaticHeaderAuth writing the value verbatim, no prefix) against the same upstream, declared in
config.yamlsince the UI does not expose this mode:The authenticated GitHub calls returning their full tool catalogs are the load-bearing proof: the resolver built the
StaticHeaderAuthand attached the credential (Bearer-prefixed forbearer_token, verbatim forauthorization), and a real upstream accepted both. A broken graft would surface the upstream 401 as an empty list with an error. Thetokenscheme (Authorization: token <PAT>) was also exercised and correctly emitted; GitHub rejects that scheme with a 400, which confirms the resolver sends exactly what the mode specifies rather than silently falling back.Note on why this is functional rather than a header byte-diff: on v1 a static credential is built into the request
headersdict; on v2 it rides as anhttpx.Auththat writes the header at send time, so it does not appear in the--detailed_debug"litellm headers" line. The upstream accepting the call is the observable proof the right credential went out.Type
🆕 New Feature
Changes
This wires the first two live credential modes onto v1's request path through a typed resolver:
noneand the shared-keyapi_keystatic-header family.resolver.pyfills in two of the seven arms.nonereturns aNoOpAuth;api_keyreads the shared key straight from its config and returns aStaticHeaderAuthwith the configured header name and prefix. Theapi_keyBYOK source and the other five arms staynot_implemented, so an unbuilt mode still fails closed with a typed error. These arms read entirely from the config, so the provider still needs no injected collaborators.adapter.pyis the v1 to v2 edge.to_subjectmaps v1's principal onto the resolver'sSubject.to_server_specmaps a v1 server onto aServerSpecfor a migrated mode and returnsNonefor every other mode so the caller defers to v1; here it maps onlynoneand the static-header family (api_keyonX-API-Key, andbearer_token/token/authorization/basiconAuthorizationwith their scheme prefix,basicbase64-encoded), all shared-key, and defers BYOK, OAuth, token-exchange, client-credentials, and SigV4.raise_publicmaps aCredErroronto the proxy's public HTTP contract and is the one edge allowed to raise.adapter.pyimports v1 and is deliberately kept out of the package__init__, so the resolver core (resolver.py/types.py) stays free of v1 imports.MCPClientgains an optionalresolved_auththat the client factory attaches to itsauth=slot, taking precedence over the SigV4aws_auth. The graft in_create_mcp_client's HTTP/SSE branch decides per mode viato_server_spec: a migrated mode resolves through an injectedUpstreamCredentialProviderand feeds the resultinghttpx.Authintoresolved_auth, mapping a resolver error onto the public contract viaraise_public; every other mode returnsNoneand falls through to the unchanged v1 construction. The provider is injected into the manager at construction (default-constructed for production, a fake in tests), so the resolver is exercised through real DI rather than monkeypatching.resolve_mcp_authnow runs only when the mode defers, so a migrated server skips v1's token-exchange and M2M token fetches.stdio is untouched.
auth_type/auth_valuenever reach the upstream on the stdio path (_get_auth_headersis HTTP/SSE only), and anhttpx.Authis meaningless to a subprocess, so a stdio server with a migratedauth_typestill defers to v1. No v1 code is deleted in this PR;resolve_mcp_auth's static return still backs stdio and the not-yet-migrated modes until later PRs in the sequence retire it.Tests cover the two live arms (including the emitted header per scheme), the full
to_server_specmapping and defer table,to_subject,raise_public's status mapping, theMCPClientauth-slot precedence, and the graft itself: migrated HTTP modes attachresolved_auth, deferred modes and a missing static token fall back to v1'sauth_value, a stdio server with a migratedauth_typestays on v1, and a resolver error maps to a 401 through the injected providerNote
Medium Risk
Changes live upstream authentication for common static MCP server configs on every HTTP/SSE client build, though unmigrated modes and overrides are explicitly deferred to preserve v1 behavior.
Overview
Wires the v2
UpstreamCredentialProviderinto_create_mcp_clientfor HTTP/SSE transports sononeand shared static-key modes (api_key, bearer/token/authorization/basic) resolve tohttpx.Auth(NoOpAuth/StaticHeaderAuth) instead of v1’sresolve_mcp_auth+ header dict. OAuth, SigV4, BYOK, passthrough, missing tokens,mcp_auth_headeroverrides, and stdio still defer to v1 unchanged.Adds
adapter.py(to_server_spec,to_subject,raise_public) as the v1↔v2 bridge andMCPClient.resolved_auth, which the httpx factory prefers over SigV4aws_auth. Resolvernoneand shared-keyapi_keyarms are implemented; other modes remainnot_implemented. InboundAuthorizationinextra_headersskips attaching resolved auth so hooks/caller headers keep winning.Reviewed by Cursor Bugbot for commit 7c11148. Bugbot is set up for automated code reviews on this repo. Configure here.