fix(client): fall back from discover for any non-modern error - #1133
Conversation
github-actions
Bot
added
T-test
| // Fall back to the legacy `initialize` handshake. The 2026-07-28 | ||
| // backward-compatibility guidance is explicit that this MUST NOT | ||
| // be keyed to one specific error code. | ||
| Err(_) => { |
There was a problem hiding this comment.
Err(_) also catches 401 and 403, as well as client-side failures like NoPreferredProtocolVersion. Neither indicates that the peer is legacy, but Auto now attempts initialize and may hide the original actionable error.
There was a problem hiding this comment.
Fixed in the new push — no more Err(_). indicates_legacy_server
classifies each variant, so 401/403/missing-OAuth (new
is_authorization_failure) and NoPreferredProtocolVersion / Cancelled
all surface instead of falling back. Match is exhaustive, so a new variant
has to be classified explicitly.
| // must not trigger a legacy fallback. Surface the error. | ||
| Err(error) if is_modern_server_error(&error) => return Err(error), | ||
| // Any other outcome — `-32601`, `-32602`, `-32600`, another | ||
| // implementation-defined error, or no response at all — means |
There was a problem hiding this comment.
discover_startup waits in expect_response until a message comes in or the transport closes. So, if a legacy server is open but silently ignores server/discover, it never reaches this branch.
There was a problem hiding this comment.
Right, didn't fix that here. expect_response blocks until the transport
closes or a message arrives, so a server that ignores server/discover
never reaches the fallback. Needs a discover probe timeout, which changes
discover_startup / expect_response — I'll open a separate issue so it
stays out of this PR.
330109a to
1e29393
Compare
`ClientLifecycleMode::Auto` only fell back from `server/discover` on `-32601`, so legacy servers that reject the probe with other codes (`-32600`, `-32602`, session-middleware errors) failed to connect even though `initialize` would have succeeded. The 2026-07-28 backward-compatibility guidance is explicit that the fallback MUST NOT be keyed to one specific error code, but it also only applies to failures that signal a legacy server. Add `ClientInitializeError::indicates_legacy_server`, which classifies a discover failure by what it says about the peer: - a JSON-RPC error is legacy unless it is a modern-era rejection (`MISSING_REQUIRED_CLIENT_CAPABILITY` or `HEADER_MISMATCH`; version negotiation is already handled inside `discover_startup`, so `UNSUPPORTED_PROTOCOL_VERSION` never reaches here); - a closed connection, an unexpected response shape, or a non-authorization transport failure is treated as a legacy server that did not engage the probe; - an authorization or scope gate (HTTP 401/403, missing local OAuth), a modern version mismatch, or a client-side condition (`NoPreferredProtocolVersion`, `Cancelled`) is surfaced, because an `initialize` retry cannot resolve it and would mask the actionable error. The fallback is keyed on this classification, and the new `is_authorization_failure` helper fills the 403 gap left by the existing 401-only `is_authorization_required`. The classification match is exhaustive on `ClientInitializeError`, so adding a variant forces a decision instead of being silently swept into the fallback. A silently legacy server that ignores the probe entirely still stalls `expect_response`; that requires a discover timeout and is out of scope here. Fixes modelcontextprotocol#1040.
1e29393 to
2af076e
Compare
|
Redesigned per your feedback. Replaced the Three calls I'd like your input on:
|
2 participants
Fixes #1040.
Problem
ClientLifecycleMode::Autoonly fell back to the legacyinitializehandshake when
server/discoverfailed with-32601(METHOD_NOT_FOUND).Legacy servers commonly reject an unknown pre-
initializerequest withother implementation-defined errors (
-32600,-32602, session-middlewareerrors), so Auto broke against servers
Initializecould have reached.Approach
Replace the
Err(_)+ modern-code carve-out with a single classification,ClientInitializeError::indicates_legacy_server, placed next to theexisting
auth_challenge/is_authorization_required:connection, an unexpected response shape, or a non-authorization transport
failure.
initializeretry cannot resolve: modern-erarejections (
MISSING_REQUIRED_CLIENT_CAPABILITY,HEADER_MISMATCH), anegotiated version mismatch, an authorization or scope gate, and
client-side state (
NoPreferredProtocolVersion,Cancelled).The match is exhaustive, so a new
ClientInitializeErrorvariant forces adecision instead of being swept into the fallback.
is_authorization_failurefills the 403 gap left by the 401-only
is_authorization_required.A silently legacy server that ignores the probe and hangs
expect_responseis a separate concern (needs a discover timeout); tracked in #1142.
Tests
test_legacy_server_classification.rspins the classification per variant(legacy JSON-RPC, modern rejection, 401/403/local-OAuth, client state,
closed/unexpected response, version mismatch). Existing lifecycle and auth
classification tests stay green.