feat(client): accept a cached era verdict on ConnectOptions.prior#2511
Conversation
🦋 Changeset detectedLatest commit: 23e22de The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, a few other angles were examined and ruled out: (1) a legacy verdict overriding versionNegotiation: { pin } — deliberate and documented in the new ConnectOptions JSDoc ("including { pin }"), since prior is checked before mode resolution; (2) the _connectPlainLegacy extraction — faithful to the old inline legacy body, preserving the session-resume branch, the fresh-connect state reset, and options threading into _legacyHandshake; (3) a { kind: 'legacy' } verdict on a modern-only supportedProtocolVersions list — fails with a typed SdkError(EraNegotiationFailed) inside _legacyHandshake, not silently.
Extended reasoning...
Two nit-level findings are posted inline (stale prose after the widening; a corrupt-blob shape that slips past normalizePrior and throws a raw TypeError after transport start). Neither blocks the feature, but this PR adds new public API surface (exported PriorDiscovery, widened ConnectOptions.prior), so per the repo's addition-scrutiny conventions a human should still make the design call — this note only records what was already examined so it is not re-explored.
a831d11 to
c14b7a9
Compare
There was a problem hiding this comment.
Beyond the inline finding, I also examined whether prior: { kind: 'legacy' } silently overriding a versionNegotiation: { mode: { pin } } client is a bug — it is documented intent (the new ConnectOptions.prior JSDoc states a supplied verdict takes precedence over the configured mode, including { pin }), so it is not flagged.
Extended reasoning...
Bugs were found and are posted as inline comments (the validatePrior kind-only discrimination breaking the published bare DiscoverResult form — verified against the parent commit e81758c, which shipped prior?: DiscoverResult in @modelcontextprotocol/client 2.0.0-beta.4). This note only records that the verdict-overrides-pin concern raised by finders was examined and ruled out as documented behavior, so a later pass need not re-explore it.
| /** | ||
| * Discriminating on `kind` alone is safe here: `prior` is exclusively | ||
| * host-constructed (`PriorDiscovery`), never raw server data. The `discover` | ||
| * payload is the part that still guards a corrupt blob — it is validated with | ||
| * the same schema the `server/discover` wire path uses, so a corrupt or | ||
| * partially-corrupt blob fails typed before any connection state changes. | ||
| */ | ||
| function validatePrior(prior: PriorDiscovery): PriorDiscovery { | ||
| // `prior` typically arrives via JSON.parse of a persisted blob: a corrupt | ||
| // primitive, an unrecognized kind, or a corrupt `discover` payload must | ||
| // all fail typed here — not as a TypeError mid-adopt. | ||
| if (typeof prior === 'object' && prior !== null) { | ||
| if (prior.kind === 'legacy') { | ||
| return prior; | ||
| } | ||
| // Validation only — the blob itself is adopted verbatim (not the | ||
| // parsed clone), so unknown nested members a stricter schema would | ||
| // strip survive re-persistence via getDiscoverResult(). | ||
| if (prior.kind === 'modern' && DiscoverResultSchema.safeParse(prior.discover).success) { | ||
| return prior; | ||
| } | ||
| } | ||
| throw new SdkError( | ||
| SdkErrorCode.EraNegotiationFailed, | ||
| "connect({ prior }): unrecognized prior — expected { kind: 'modern', discover } or { kind: 'legacy' }" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 validatePrior() discriminates on kind alone and the type narrows to PriorDiscovery only, so the bare DiscoverResult form published in 2.0.0-beta.4 (and documented in docs/advanced/gateway.md, examples/gateway, and the e2e manifest) now fails at compile time and throws SdkError(EraNegotiationFailed) at runtime — an unflagged breaking change under a 'minor' changeset, while the PR description promises the opposite ("prior?: DiscoverResult widens to prior?: DiscoverResult | PriorDiscovery; existing callers are unaffected"). The same kind-only check also lets a persisted bare blob carrying a stray top-level kind: 'legacy' member (which survives the loose wire schemas) silently run the legacy initialize handshake against a modern server — the exact misclassification the description says supportedVersions-keyed discrimination prevents, except that discrimination was never implemented. Either implement the promised widening — discriminate on supportedVersions (required on every DiscoverResult, absent from both verdict arms), which fixes both issues — or, if dropping the bare form is intentional, correct the description/changeset, add a migration note for existing beta callers, and reject legacy-arm objects that also carry supportedVersions.
Extended reasoning...
The implementation contradicts the PR description on both of its prior-compatibility claims
The PR description states: "Breaking Changes: None. prior?: DiscoverResult widens to prior?: DiscoverResult | PriorDiscovery; existing callers are unaffected and the bare form's semantics are unchanged", and under Additional context: "Bare-vs-verdict discrimination keys on supportedVersions (required on every DiscoverResult, absent from both verdict arms), so a server result carrying a stray extra kind member cannot be misclassified." Neither is what the code does. ConnectOptions.prior is narrowed to prior?: PriorDiscovery (no union arm), and validatePrior() discriminates on prior.kind alone — there is no supportedVersions-keyed discrimination anywhere in the diff. The PR's own test pins the opposite of the described behavior: "object without a kind (e.g. a raw DiscoverResult) → typed SdkError, nothing sent".
Problem 1: the bare form is published API, and this PR breaks it unflagged
prior?: DiscoverResult is not an unreleased shape. git show e81758c:packages/client/src/client/client.ts (the parent commit, the "Version Packages (beta)" release of @modelcontextprotocol/client 2.0.0-beta.4) shows prior?: DiscoverResult; with JSDoc documenting direct adoption — and the bare form prior: JSON.parse(persisted) was the documented pattern in docs/advanced/gateway.md, examples/gateway/client.ts, and the typescript:client:connect:prior-zero-roundtrip e2e requirement, all of which this PR rewrites to the wrapped form. A beta.4 consumer following those shipped docs now fails to compile, and — since persisted blobs are typically JSON.parsed and any-typed — throws SdkError(EraNegotiationFailed, 'unrecognized prior…') at runtime.
Step-by-step proof (break):
- Host on beta.4 follows the shipped gateway recipe:
await worker.connect(transport, { prior: JSON.parse(persisted) })wherepersistedis aDiscoverResultfromgetDiscoverResult(). - Host upgrades to this PR's release.
connect()atclient.ts:971-976seesprior != nulland callsvalidatePrior(). - The blob has no
kindmember —prior.kind === 'legacy'andprior.kind === 'modern'both fail — sovalidatePrior()falls through tothrow new SdkError(EraNegotiationFailed, \"connect({ prior }): unrecognized prior…\")(client.ts:372-375). - The connect that worked yesterday throws today. The changeset is
minor, its text never mentions removing the bare form, the breaking-change checkbox is unchecked,docs/migration/upgrade-to-v2.mdis rewritten as ifpriorwas alwaysPriorDiscovery(no "if you were on an earlier v2 beta" note, the patternsupport-2026-07-28.mduses for alpha→beta shape changes), and there is no codemod mapping.
This hits two repo-instruction requirements directly: the checklist's "Backwards compat: public-interface changes, default changes, removed exports — flagged and justified" and the Recurring Catch "prose that promises behavior the code no longer ships" (#1718, #1838).
Problem 2: kind-only discrimination creates the exact misclassification the description claims is impossible
validatePrior's doc comment asserts "prior is exclusively host-constructed (PriorDiscovery), never raw server data" — but the misuse the function's own tests harden against (a host passing a persisted raw DiscoverResult blob) is verbatim-preserved server data. The 2026 wire codec parses server/discover results with liftedResult() = z.looseObject (packages/core-internal/src/wire/rev2026-07-28/buildSchemas.ts), and Zod's looseObject retains unknown keys — so a server extension member literally named kind survives into _discoverResult, out through getDiscoverResult(), and through JSON persistence (DiscoverResultSchema extends the loose ResultSchema, so validatePrior's safeParse would not strip it either).
Step-by-step proof (misclassification): (1) A modern server's discover result carries a stray top-level kind: 'legacy' member — spec-legal under the loose schemas. (2) A host on the pre-PR calling convention passes the bare persisted blob (any-typed, no compile error). (3) validatePrior() at client.ts:363 hits prior.kind === 'legacy' → returns the blob as a legacy verdict. (4) _connectFromPrior() routes it to _connectPlainLegacy(), which runs the legacy initialize handshake against a modern server — silent wrong-era connection, not the typed error the hardening layer promises for unrecognized shapes, and not the modern adoption the blob actually encodes. Only the legacy arm is exposed (a stray kind: 'modern' fails safely because discover is undefined). The description explicitly claims a pinning test for "a bare DiscoverResult carrying a stray extra kind member" — no such test exists in connectPrior.test.ts.
Fix
One change resolves both problems and makes the description true: implement the discrimination the description already specifies. In validatePrior/normalizePrior, treat an object with an array supportedVersions as the bare DiscoverResult form (wrap it as { kind: 'modern', discover }), and only then discriminate the verdict arms on kind — supportedVersions is required on every DiscoverResult and absent from both verdict arms, so the split is unambiguous, restores the beta.4 form, and makes a stray kind member on a bare blob harmless. Alternatively, if dropping the bare form is a deliberate design decision: check the breaking-change box, fix the description and changeset text, add the migration note to upgrade-to-v2.md for earlier-beta callers, and have the legacy arm reject objects that also carry supportedVersions so a raw blob can never silently connect on the wrong era. Merging as-is ships a runtime break to published-beta consumers under release prose that affirmatively denies it.
There was a problem hiding this comment.
The amended head (7f7e7d9, committed after this comment) addresses both substantive halves of this finding on the intentional-drop path:
- Misclassification fixed:
validatePrior()(client.ts:366) now accepts the legacy arm only when the blob carries neithersupportedVersionsnordiscover, so a persisted bareDiscoverResultwith a straykind: 'legacy'fails with the typedSdkError(EraNegotiationFailed)instead of silently runninginitializeagainst a modern server's advertisement — and the previously missing pinning test now exists ("DiscoverResult-shaped blob with a stray kind: 'legacy' → typed SdkError, never era-chooses" in connectPrior.test.ts). - Description corrected: the PR description no longer promises the
DiscoverResult | PriorDiscoverywidening orsupportedVersions-keyed discrimination; it now discloses the shape finalization as a beta-only surface change.
One piece of the migration package is still missing: neither the changeset nor docs/migration/upgrade-to-v2.md says the bare DiscoverResult form shipped in 2.0.0-beta.4 (and documented in the beta.4 gateway recipe) stops working. The changeset text describes only the new shape, and the rewritten Client.connect(transport, { prior }) section in upgrade-to-v2.md describes the end state with no "if you were on an earlier v2 beta" note (the pattern support-2026-07-28.md uses for alpha→beta shape changes). A beta.4 consumer following the previously shipped docs upgrades into a runtime SdkError(EraNegotiationFailed) with no changelog or migration entry explaining it. One line in each — e.g. "earlier v2 betas accepted a bare DiscoverResult; wrap it as { kind: 'modern', discover }" — closes the gap.
There was a problem hiding this comment.
Re-checked at the amended head (23e22de, pushed 16:15Z): the two substantive halves remain fixed — validatePrior()'s legacy arm rejects DiscoverResult-shaped blobs (with the pinning test), and the description discloses the shape finalization — and this push also incorporated other review feedback (the validatePrior JSDoc rationale, the gateway.md discover() tip). The one remaining item from this thread is still open, though: neither .changeset/prior-legacy-verdict.md nor docs/migration/upgrade-to-v2.md says the bare DiscoverResult form shipped in 2.0.0-beta.4 (and documented in the beta.4 gateway recipe) stops working. A beta.4 consumer upgrading into this release still hits a runtime SdkError(EraNegotiationFailed) with no changelog or migration entry explaining it. One line in each closes it — e.g. "earlier v2 betas accepted a bare DiscoverResult as prior; wrap it as { kind: 'modern', discover }".
felixweinberger
changed the title
c14b7a9 to
7f7e7d9
Compare
There was a problem hiding this comment.
Beyond the two inline doc/comment nits, I also examined whether prior: { kind: 'legacy' } silently bypassing versionNegotiation: { mode: { pin } } is a bug — it is intended and explicitly documented on the ConnectOptions.prior JSDoc ("takes precedence over the configured versionNegotiation mode entirely — including { pin }"), so it is not a mode-safety hole.
Extended reasoning...
This run's findings are two documentation nits (the validatePrior JSDoc rationale contradicting its own decoy guard, and three doc locations not qualified for the legacy verdict arm). A finder also raised the pin-override concern; I verified connect() routes any non-null prior to _connectFromPrior before resolveVersionNegotiation runs, and the new ConnectOptions JSDoc documents that precedence explicitly, so it was ruled out as intended behavior. Prior review rounds' concerns (stale zero-round-trip prose, the normalizePrior TypeError gap, the bare-DiscoverResult break) are all addressed at HEAD via the reworded JSDoc summary, schema validation in validatePrior with test-pinned no-state-change semantics, and the description/changeset reframing prior as a beta-only surface.
7f7e7d9 to
a39585e
Compare
There was a problem hiding this comment.
Beyond the inline doc nit, this run also examined whether prior: { kind: 'legacy' } silently overriding versionNegotiation: { mode: { pin: '2026-07-28' } } is a bug — it is not: connect() checks prior before resolving versionNegotiation by design, and the ConnectOptions.prior JSDoc explicitly documents the verdict as "taking precedence over versionNegotiation", with corrupt blobs rejected typed by validatePrior() before any era choice.
Extended reasoning...
One candidate issue was raised and refuted this run: a stale/corrupt cached legacy verdict bypassing a pinned modern mode. The precedence is intentional and documented in the ConnectOptions.prior JSDoc ('taking precedence over versionNegotiation'), the pin still governs whenever no prior is supplied, and validatePrior() ensures a corrupt blob throws SdkError(EraNegotiationFailed) rather than era-choosing — so a decoy blob cannot silently downgrade a pinned client. Recording this here so a later review pass does not re-explore it; this is informational, not a correctness guarantee.
Hosts that already know a server's era from out-of-band metadata shouldn't pay a server/discover probe round trip that fails against every legacy server — and mode: 'legacy' pins the client to initialize forever. `prior` takes a discriminated PriorDiscovery verdict: the modern arm wraps a previously obtained DiscoverResult (zero round trips), the legacy arm skips the probe and runs the plain legacy initialize handshake. Freshness is the supplying host's responsibility — a stale legacy verdict succeeds silently against an upgraded server, so hosts must date cached legacy verdicts in their own storage and stop supplying them past their policy horizon; with no prior, the configured versionNegotiation mode decides (an 'auto' client re-probes). Persisted-blob plumbing fails typed: null priors are treated as absent, the modern arm's discover payload is schema-validated at the seam (validation only — the blob is adopted verbatim), and corrupt or unrecognized shapes reject with SdkError(EraNegotiationFailed) before anything reaches the wire.
a39585e to
23e22de
Compare
1 participant
ConnectOptions.priortakes a cached era verdict (PriorDiscovery):{ kind: 'modern', discover }adopts a previously obtainedDiscoverResultwith zero round trips;{ kind: 'legacy' }skips theserver/discoverprobe and runs the plaininitializehandshake directly.Motivation and Context
Hosts that already know a server's era from out-of-band metadata (a registry entry, an earlier connection's outcome) shouldn't pay a probe round-trip that fails against every legacy server. Today the only ways to express that knowledge are
versionNegotiation: { mode: 'legacy' }, which pins the client toinitializeforever (a server upgrade is never discovered), orconnect({ prior }), which throwsEraNegotiationFailedon anything without a 2026-07-28+ overlap.The fix models a prior as what it is — a cached probe verdict.
prioris a discriminatedPriorDiscovery: themodernarm wraps aDiscoverResult(adopted with zero round trips), thelegacyarm goes straight toinitialize. The verdict is exclusively host-constructed, never raw server data, so the discriminant iskinddirectly; the modern arm'sdiscoverpayload is schema-validated at the seam (validation only — the blob is adopted verbatim), so corrupt persisted blobs fail typed before any connection state changes.Freshness is deliberately the supplying host's authority, not the SDK's — the SDK enforces only server-declared lifetimes (like
DiscoverResult.ttlMson the response cache), and a host-side cache policy doesn't belong in the connect signature. The staleness asymmetry is documented as a host contract on the type: a stale modern verdict fails loudly at the first request, but a stale legacy verdict succeeds silently forever (an upgraded server still answersinitialize), so hosts must date cached legacy verdicts in their own storage and stop supplying them past their policy horizon — supplying nopriorlets the configuredversionNegotiationmode decide (under'auto', it re-probes).How Has This Been Tested?
packages/client/test/client/connectPrior.test.ts): legacy verdict sends noserver/discover(transport-level message capture — the only request on the wire isinitialize), even undermode: 'auto'; modern verdict is zero-round-trip;prior: nulltreated as absent; malformed blobs (object withoutkind, lostdiscover, unrecognizedkind, partially-corruptdiscoverpayload, corrupt primitive) reject with a typedSdkError, nothing sent, and no connection state touched.Clientover the public package exports connected to a legacy-only server with a legacy prior; the captured message trace showsinitializewith no probe.docs/advanced/gateway.mdruns as part of the guide-examples CI leg — a fetch-layer probe counter asserts one probe on the cold connect, zero while the cached verdict is fresh, and one more after the host's horizon passes (the re-probe re-populating the cache is asserted too).Breaking Changes
None against any stable release.
prioris a beta-only surface; this PR finalizes its shape asPriorDiscovery.Types of changes
Checklist
Additional context
PriorDiscoveryis the persistable subset of the internal probe classifier's verdict vocabulary — only the two era verdicts can be cached;correctiveanderrorare transient negotiation states.kinddirectly — safe becauseprioris exclusively host-constructed, never raw server data; thediscoverpayload is the part that guards corrupt blobs, validated with the same schema theserver/discoverwire path uses.mode: 'legacy'connect body (extracted, not duplicated), so the handshake stays byte-identical to a plain legacy connect, including session-resume handling.mode: 'auto'.docs/advanced/gateway.mdgains a "Caching discovery verdicts" section teaching the full host-side loop: probe once undermode: 'auto', read the outcome (getDiscoverResult()present = modern; absent on a connected client = legacy), store it with the host's own timestamp, and gate each connect on a host freshness predicate — supplying nopriorunder'auto'is the re-probe that re-populates the cache.docs/protocol-versions.md("Skip the probe with a cached verdict", after the mode list),docs/clients/connect.md(getDiscoverResult()in the connect-time accessors, with an executed snippet showingundefinedon a legacy connect), anddocs/clients/caching.md("Two caches, two owners": server-declaredttlMsthe SDK enforces vs the host-owned verdict cache). All link to the gateway recipe rather than duplicating it.