Uh oh!
There was an error while loading. Please reload this page.
fix(client): fall back to initialize when the discover probe gets a completed non-modern answer - #2571
fix(client): fall back to initialize when the discover probe gets a completed non-modern answer#2571claude[bot] wants to merge 6 commits into
Conversation
…ompleted non-modern answer Under versionNegotiation mode 'auto', a server/discover probe answered by a completed, non-auth HTTP exchange that is not a valid modern reply now selects the legacy fallback instead of failing connect(): - 2xx replies whose JSON body fails strict JSON-RPC validation (e.g. the JSON-RPC 2.0 parse-error shape -32700 with id: null — anomalyco/opencode#39354, or error replies with extra/unknown members), stamped at the transport's parse boundary and classified by the probe classifier - 2xx application/json replies with an empty or unparseable body - 2xx replies in a non-MCP content type (HTML error pages, text/plain, missing content-type) - 202 Accepted answers to the probe: immediate legacy evidence instead of waiting out the full probe timeout - any 5xx, with or without a JSON-RPC error body (hosts caching era verdicts should date legacy verdicts; the SDK never persists one) Unchanged: 401/403 stay typed auth failures, network failures and the HTTP probe timeout still reject, and pin mode never falls back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EfMZraQUoWyeSBT2HaQ5v
🦋 Changeset detectedLatest commit: 066bbe1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client@modelcontextprotocol/codemod@modelcontextprotocol/core@modelcontextprotocol/server@modelcontextprotocol/server-legacy@modelcontextprotocol/express@modelcontextprotocol/fastify@modelcontextprotocol/hono@modelcontextprotocol/nodecommit: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…fallback
Address review on the completed-exchange fallback:
- Read the 2xx application/json body via text() before parsing: a body-read
rejection is a network failure mid-transfer (undici TypeError('terminated')
after a connection reset behind 2xx headers) and now propagates unstamped
to the typed EraNegotiationFailed rejection instead of classifying as
legacy evidence. Only a JSON.parse failure of the fully received text is
stamped for the probe classifier, so the empty-body and invalid-JSON
fallbacks are unchanged.
- Wire-real regression test: 200 + application/json headers followed by a
mid-body socket destroy rejects connect() typed and never sends initialize.
- Docs: qualify the completed-exchange rule to answers delivered as a
completed HTTP body — a probe answered 200 + text/event-stream is
classified by the reply the stream delivers, and an SSE stream carrying
only a schema-invalid reply (or none) still runs out the probe timeout,
as before.
- docs/troubleshooting.md: replace the stale "HTTP 5xx -> no legacy fallback"
bullet — a 5xx probe answer now selects the legacy initialize fallback,
with the date-your-cached-legacy-verdicts caveat.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EfMZraQUoWyeSBT2HaQ5vThere was a problem hiding this comment.
Beyond the inline findings, this run also re-examined the switch from response.json() to an unguarded response.text() read in the 2xx JSON branch and confirmed it is correct: a text() rejection (mid-body reset) propagates unstamped to the network-error row, so the prior review round's truncated-body-classifies-as-legacy concern is resolved — backed by the new mid-body socket-destroy integration test. Also checked schema-valid-but-probe-unmatched direct JSON answers (wrong/fixed id, empty batch): they time out today exactly as before this PR, so no runtime regression — only the docs-overbreadth nit noted inline.
Extended reasoning...
Bugs (three nits) were found and are posted as inline comments, so no approval/deferral verdict is given. This note records only what was additionally examined and ruled out this run: (1) the unguarded response.text() read — verified that mid-body network failures stay unstamped and reject typed, confirming the fix for the previous round's blocking finding; (2) schema-valid replies with non-matching ids and empty batch bodies — pre-existing timeout behavior, not a regression introduced here. Informational only; not a guarantee of correctness.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…on pin/modern-only rejections
Address review round 2 on the completed-exchange fallback:
- 202 gate: key the accepted-without-reply row on the probe's reserved
server-discover-probe- string id prefix (shared constant in
invalidReplySeam), so the public post-connect Client.discover() — a
Protocol request with a numeric id — can never surface
EraNegotiationFailed mid-session: a 202 answering it stays pending until
the ordinary RequestTimeout, as before. The inline comment and the
sdkErrors.ts docstring now state the same scoping; wire-real regression
test added.
- Diagnostic fidelity: the pin and modern-only rejections for outcomes
routed to the legacy verdict without a server/discover answer now name
the cause, mirroring closedCause ("the server answered the probe with
HTTP 503" for http-error outcomes; "the probe reply was not a valid
JSON-RPC message" for invalid-reply), and carry the evidence on
error.data (status/statusText/response text, or the stamped original
error and offending body — the invalid-reply outcome now keeps the
original error instead of discarding it). Message content and data
carriage pinned by tests; troubleshooting.md remediations updated,
including that a 5xx CAN produce ERA_NEGOTIATION_FAILED in pin and
modern-only modes.
- Docs overbreadth: qualify the completed-exchange rule on
protocol-versions.md and support-2026-07-28.md to answers the client can
read as a failure, dropping the unconditional "however broken" — a
direct 2xx JSON answer that parses and validates cleanly yet carries no
reply to the probe (an empty batch, or a schema-valid reply under a
non-matching id) reads as silence and runs out the probe timeout.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EfMZraQUoWyeSBT2HaQ5vThere was a problem hiding this comment.
Warning
This review may be incomplete: some analysis steps could not run due to a temporary API capacity limit.
All findings from my earlier review rounds are now addressed in the diff (the mid-body text()-first read, the probe-id-prefix scoping on the 202 gate, the pin/modern-only cause fidelity, and the docs qualifications), and this pass found no new issues — but this changes connect-time negotiation semantics and the PR itself poses an open design question (5xx-as-legacy-evidence vs. cached era verdicts), so it warrants a human decision.
What was reviewed this pass:
- the invalid-reply seam end to end (stamp sites in
streamableHttp.ts,normalizeReply, the new classifier row) against the documented rule and its stated exceptions (auth, network, silence) - the
answerCauseparenthetical on pin/modern-only throws — a candidate contradiction with the new troubleshooting prose was investigated and ruled out - the wire-real test matrix covers every newly-falling-back class plus controls for network failure, probe timeout, and post-connect
discover()
Extended reasoning...
Overview
The PR reroutes five classes of completed-but-broken server/discover probe answers (schema-invalid 2xx bodies, empty/unparseable JSON, non-MCP content types, 202-to-the-probe, and all non-auth 5xx) from hard connect failures into the legacy initialize fallback. It adds an internal provenance-stamp module (invalidReplySeam.ts), a new invalid-reply classifier outcome, cause-carrying diagnostics on the pin/modern-only rejection paths, and updates four docs pages plus a changeset. Roughly 340 lines of new unit tests and an integration test for the mid-body-reset control accompany it.
Security risks
Low. No auth, crypto, or permission logic is touched; the 401/403 typed-auth-failure paths are explicitly preserved and pinned by tests. The main risk class is misclassification — a transient server failure being handshaken as legacy — which is a robustness/fleet-caching concern rather than a security one, and the PR documents the mitigation (dated cached legacy verdicts) in three places.
Level of scrutiny
High. This is client connect-path logic in a published SDK, and it inverts a previously deliberate rule (5xx was 'never era evidence'; it now selects the legacy fallback). The PR description itself flags the 5xx/cached-verdict tradeoff as an open question for reviewers, and the repo's own review conventions put design justification first — that call (made per maintainer direction on the linked issue, but still a semantic inversion) belongs to a human maintainer, not a bot approval.
Other factors
My three earlier review rounds surfaced one red finding (mid-body network failures being stamped as legacy evidence) and several yellow ones (202 gate matching post-connect discover(), missing diagnostics on pin-mode throws, stale troubleshooting.md 5xx bullet, over-categorical docs prose); the current diff addresses all of them, each with a pinning test or docs fix — including an integration test that destroys the socket mid-body to prove the network path stays typed. This pass found nothing new; the one finder candidate (the answerCause parenthetical firing for ordinary 4xx declines) was examined and refuted. Test coverage is thorough and includes negative controls, so the remaining open item is purely the design question above.
…rror-fallback # Conflicts: # packages/client/src/client/streamableHttp.ts
Uh oh!
There was an error while loading. Please reload this page.
…-JSON branch Read the 2xx application/json body via text() only when the method is present, matching the file's existing response.text?.() tolerance for partial response-likes from a custom fetch. When only json() exists, fall back to await response.json(); its rejection propagates unstamped (base behavior), since json() cannot distinguish a parse failure from a mid-body transport failure. The invalid-reply stamp stays on the text() path's JSON.parse failure only. Adds a regression test with a custom fetch response-like implementing only json().
| try { | ||
| data = JSON.parse(bodyText); | ||
| } catch (error) { | ||
| throw markInvalidReplyEscape(error, undefined); |
There was a problem hiding this comment.
🟡 nit (optional): The JSON.parse-failure stamp discards the already-read bodyText (markInvalidReplyEscape(error, undefined)), so for a 2xx application/json probe answer with an empty or non-JSON body, pin-mode and modern-only clients get error.data = { cause, body: undefined } — while the troubleshooting doc and changeset added in this PR promise "the offending body and original validation error" / "response text" on error.data. Fix: stamp the raw text — markInvalidReplyEscape(error, bodyText) — which is verdict-neutral, since parseJsonRpcErrorMember ignores non-object bodies and classifyInvalidReply still returns legacy.
Extended reasoning...
Path: pin mode (versionNegotiation: { mode: { pin: '2026-07-28' } }) against a proxy that answers the server/discover probe 200 + content-type: application/json with an HTML error page (or an empty body). In _send (streamableHttp.ts:1214-1221) response.text() resolves the full text, JSON.parse(bodyText) throws SyntaxError, and the catch stamps it with body: undefined — discarding bodyText, which is in scope one line up. normalizeReply (versionNegotiation.ts:414-423) produces { kind: 'invalid-reply', body: undefined, cause: SyntaxError }, classifier returns legacy, and the pin/modern-only throw (versionNegotiation.ts:542-571) attaches causeData = { cause: SyntaxError, body: undefined }. The user-visible result: connect() rejects with EraNegotiationFailed whose data contains only a SyntaxError ("Unexpected token '<'...") — no response text, no offending body, no status (it was a 2xx). Yet docs/troubleshooting.md (edited in this PR) instructs for exactly this parenthetical — (the probe reply was not a valid JSON-RPC message; no fallback in pin mode) —…
Verification: nit — trigger: a pin-mode or modern-only client whose server/proxy answers the server/discover probe 200 + content-type application/json with an empty or non-JSON body. Mechanism verified: streamableHttp.ts:1216-1220 reads const bodyText = await response.text();, then on JSON.parse(bodyText) failure throws markInvalidReplyEscape(error, undefined) — discarding bodyText, in scope one line up…
The direct-JSON branch read the 2xx body as text before parsing but stamped the parse failure bodiless, so pin-mode and modern-only clients got error.data.body === undefined for empty/non-JSON 2xx bodies instead of the offending response text the troubleshooting doc promises. Stamp the already-read bodyText instead. Verdict-neutral: the probe classifier ignores non-object bodies, so the invalid reply still classifies legacy.
Requested byFelix Weinberger
Before
A v2 client with
versionNegotiation: { mode: 'auto' }hard-failedconnect()withSdkError: Version negotiation probe failed(or burned the full probe timeout) whenever the server answered theserver/discoverprobe with a completed HTTP exchange that wasn't a valid modern reply — even though the server had just demonstrated it doesn't speakserver/discoverand a plaininitializewould have succeeded. Concretely:server/discoverprobe rejected by server (-32700), no fallback to classicinitializeanomalyco/opencode#39354: a vendor server answers unknown methods with{"error":{"code":-32700,"message":"Parse Error"},"id":null}(a legal JSON-RPC 2.0 parse-error reply, but not representable by the SDK's strict message schema because ofid: null). The probe died in schema validation and connect rejected with the raw zod issues — no fallback. Note the asymmetry: the same body on a 404 already fell back via the classifier's lenient 4xx body parse; only the 200 path hard-stopped.text/plain, missing content-type) — same hard stop, surfaced as an opaque "network" failure.RequestTimeout, despite the 202 being a definitive "no reply is coming".-32601body (some frameworks map JSON-RPC errors to 500).After
Per Felix's direction ("always try
initializerather than cut short after discover" for completed exchanges; 5xx included — "lots of poorly behaved weird servers out there"), the rule is now:All five classes above now fall back and connect against a working legacy server, matching the long-standing unparseable-4xx rule. The 202 case falls back immediately — no probe-timeout wait. Repro'd end-to-end against the exact opencode#39354 wire shape (200 +
-32700+id:null→ fallbackinitialize→ connected, eralegacy).How
packages/client/src/client/invalidReplySeam.ts(new, internal): an identity-preserving provenance stamp in the style of the fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence #2564 auth seam (Symbol.for, survives bundler double-installs). The transport stamps errors born between fetch resolution andonmessage— previously indistinguishable from network errors by the time the classifier saw them — carrying the offending parsed body when there is one.streamableHttp.ts: stamps the strictJSONRPCMessageSchemavalidation failure with the parsed body (2xx JSON branch), stampsresponse.json()parse failures (empty/invalid JSON), and turns a 202-to-the-probe-request into an immediate stamped throw (scoped tomethod === 'server/discover'; every other flow through the 202 branch is unchanged). Wrong/missing content-type needed no transport change — it already throws typedSdkError(ClientHttpUnexpectedContent).versionNegotiation.tsnormalizeReply: reads the stamp (and matchesClientHttpUnexpectedContent) into a new normalized outcome,{ kind: 'invalid-reply', body? }, instead ofnetwork-error.probeClassifier.ts: newinvalid-replyrow — a leniently readable JSON-RPCerrormember classifies like an in-band error first (so the-32022corrective/select-and-continue rows keep working even off a malformed reply), everything else is the conservative legacy fallback. The 5xx special case from fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence #2564 is removed: all non-auth HTTP rejections (remaining 4xx and 5xx) now flow through the same body-parse-then-legacy path. 401/403 handling is untouched and still ranks above the body parse.docs/protocol-versions.mdanddocs/migration/support-2026-07-28.mdupdated with the completed-exchange rule (and the kept exceptions); changeset added (patch,@modelcontextprotocol/client).-32022-via-malformed-reply, bodiless invalid replies; wire-real regressions through the realStreamableHTTPClientTransportwith a scripted fetch for every class (oc#39354 body, empty body, invalid JSON, HTML, wrong CT, missing CT, 202-immediacy with a timing assertion, bare 500, 500+-32601body, 502 HTML); pin-mode and modern-only gates still refuse to fall back; controls pinning that network failures and the HTTP probe timeout still reject.Open question for review: 5xx and cached era verdicts
The one argument against 5xx-fallback was fleet-level caching: a modern server having a transient 5xx moment gets handshaken as legacy for that connection, and a host using the gateway guide's
connect({ prior })recipe could persist that verdict. The SDK itself never emits a cacheable verdict object — hosts synthesize{ kind: 'legacy' }from the absence ofgetDiscoverResult()— so marking the 5xx path non-cacheable would require new public API (verdict provenance onClient). This PR ships the plain fallback and leans on the guide's existing mitigation (date cached legacy verdicts and let them expire), with notes added in the classifier comment and both docs pages. If reviewers want a first-class guard (e.g. exposing why the era is legacy), happy to follow up.Fixes the SDK side of anomalyco/opencode#39354.
Generated by Claude Code