Skip to content

docs(migration): align four passages with the implementation #2388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
felixweinberger merged 1 commit into from
Jun 29, 2026
+21 −16
Merged

docs(migration): align four passages with the implementation #2388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions docs/migration/support-2026-07-28.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,13 @@
- **`resultType` is gone from every public result type** (`Result`, `CallToolResult`,
`GetPromptResult`, …). The wire schemas keep parsing it, and the protocol layer
consumes it before results reach your code.
- **`DiscoverResult` strips its cache fields too.** `ttlMs` / `cacheScope` on
`server/discover` are wire-only — consumed by the client's response-cache layer and
absent from the public `DiscoverResult` type returned by `getDiscoverResult()`.
Tooling that displays the server's advertised cache policy must parse raw frames.
- **`DiscoverResult` hides its cache fields at the type level only.** `ttlMs` /
`cacheScope` on `server/discover` are read by the client's response-cache layer and
are absent from the public `DiscoverResult` type returned by `getDiscoverResult()` —
but they are not removed at runtime: the returned object still carries both, readable
via a cast. The wire parse defaults absent or malformed hints to `0` / `'private'`,
so only tooling that must distinguish an omitted hint from an advertised default
needs raw frames.

Check warning on line 311 in docs/migration/support-2026-07-28.md

View check run for this annotation

Claude / Claude Code Review

DiscoverResult cache-hint claims only hold for the probe path, not client.discover() 

The new claims that the returned object "still carries both" cache fields, that "the wire parse defaults absent or malformed hints to 0/'private'", and that they are "read by the client's response-cache layer" only hold for the DiscoverResult populated by the connect-time version-negotiation probe — the public client.discover() path (which equally backs getDiscoverResult()) goes through codec.decodeResult(), which returns the raw object un-defaulted, and the response-cache layer never reads serv
Comment on lines +305 to +311

@claude claude Bot Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The new claims that the returned object "still carries both" cache fields, that "the wire parse defaults absent or malformed hints to 0/'private'", and that they are "read by the client's response-cache layer" only hold for the DiscoverResult populated by the connect-time version-negotiation probe — the public client.discover() path (which equally backs getDiscoverResult()) goes through codec.decodeResult(), which returns the raw object un-defaulted, and the response-cache layer never reads server/discover hints at all. Consider scoping the runtime-presence/defaulting claims to the probe-populated result and dropping or correcting the response-cache-layer clause.

Extended reasoning...

What the passage claims vs. what the code does

The rewritten bullet (docs/migration/support-2026-07-28.md:305-311) makes three runtime claims about the object behind getDiscoverResult():

  1. ttlMs / cacheScope "are read by the client's response-cache layer";
  2. "the returned object still carries both, readable via a cast";
  3. "The wire parse defaults absent or malformed hints to 0 / 'private', so only tooling that must distinguish an omitted hint from an advertised default needs raw frames."

Claims (2) and (3) are accurate only for one of the two sources of _discoverResult, and claim (1) is not backed by the implementation.

The probe path (where the claims hold). During connect-time version negotiation, probeClassifier.ts:143 routes the probe answer through codecForVersion(MODERN_WIRE_REVISION).validateResult('server/discover', ...). validateResult is triState() (core-internal/src/wire/rev2026-07-28/codec.ts:62-66), which returns parsed.data from the dispatch schema — and that schema's ttlMs/cacheScope carry .catch(0) / .catch('private') (rev2026-07-28/schemas.ts ~1119-1128). The parsed, defaults-applied object is what client.ts:1093 stores as _discoverResult. For this source, "still carries both" and "defaults absent or malformed hints" are true.

The discover() path (where they don't). client.discover() (client.ts:1447-1450) also populates _discoverResult and is one of the documented sources of getDiscoverResult(). It goes through _requestWithSchema → _requestWithSchemaViaCodec → codec.decodeResult(), which only safeParses the catch-defaulted wire schema as a validity check and then returns {...raw} with just resultType deleted (rev2026-07-28/codec.ts:247-261) — the defaulted parse output is discarded. The result is then validated against the public DiscoverResultSchema (types/schemas.ts:577), a looseObject with no cache fields, which neither adds absent keys nor normalizes malformed ones.

Step-by-step proof (discover() against a server that omits the hints):

  1. A non-SDK 2026-era server answers server/discover with a result that omits ttlMs and cacheScope.
  2. codec.decodeResult('server/discover', raw) safeParses the wire schema (the .catch defaults exist only inside that throwaway parse), then returns the raw object minus resultType — still with no ttlMs/cacheScope.
  3. The looseObject DiscoverResultSchema passes it through unchanged; _discoverResult is set to it.
  4. Tooling that follows the doc and casts getDiscoverResult() to read ttlMs/cacheScope gets undefined, not the documented 0/'private'. Likewise a malformed hint (e.g. ttlMs: "soon") passes through verbatim instead of being defaulted.

The response-cache clause. The "read by the client's response-cache layer" wording (carried over from the pre-PR text) is also not what the code does: the only freshness reads are the list verbs' aggregate write and resources/read (client.ts:1654, 1672-1680, 1789-1794), each reading hints off its own result body via _freshness(). 'server/discover' appears in responseCache.ts only inside the CAP_EXEMPT_METHODS set (lines 142-148); there is no cache write or read for it anywhere in the client, so its hints are never consumed by that layer.

Why this matters here. This PR's stated purpose is aligning these exact sentences with the implementation, with file:line verification, and the audience the sentence addresses is precisely the tooling that would rely on the cast/defaulting behavior. No verifier disputed the finding (4/4 confirmed).

Suggested fix. Scope the runtime-presence and 0/'private'-defaulting claims to the connect-probe-populated DiscoverResult (or describe the discover() behavior: omitted hints stay omitted, malformed hints pass through verbatim), and drop or correct the "read by the client's response-cache layer" clause — e.g. "appear in the response-cache store only as a cap-exempt method; the cache reads hints off the cacheable list/read results, not server/discover".

- **High-level methods return the named public types** (`client.callTool()` →
`Promise<CallToolResult>`, etc.). Handler return positions are unaffected.
- **Reserved envelope keys and retry fields appear in no public params/result type.**
Expand Down
26 changes: 14 additions & 12 deletions docs/migration/upgrade-to-v2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,11 @@ prefer wrapping explicitly. Zod v4, ArkType, and Valibot all implement the spec.
**Zod v3 is no longer supported** (v1 peer was `^3.25 || ^4.0`). Check the **declared
range** in your `package.json`, not just the installed version: a zod-3 range that
satisfied the v1 peer installs and typechecks cleanly under v2 and only fails at
runtime, when the first registration throws — under a spawning harness that surfaces
as an opaque child exit two hops from the cause. A Zod v3 schema
hard-errors with a pointer at `fromJsonSchema()`. Zod **≥4.2.0** self-converts via
runtime — and quietly: registration swallows the conversion failure, the server starts
and connects normally, and the first `tools/list` (so `client.listTools()`) answers
with an error pointing at `fromJsonSchema()` while the process keeps running. (Only the
deprecated unwrapped raw-shape form with zod-3 field values throws at registration,
with a message pointing at `zod/v4`.) Zod **≥4.2.0** self-converts via
`~standard.jsonSchema` — the supported path. Zod **4.0–4.1** lacks it, so the SDK falls
back to its bundled Zod's `z.toJSONSchema()` with a one-time `[mcp-sdk]` console
warning; and because `.describe()` field descriptions live in the _authoring_ Zod's
Expand Down Expand Up @@ -798,13 +800,12 @@ same handling as the POST send path.
methods plus `tokens()` / `clientInformation()`. On read, a stored value whose `issuer`
names a different AS is treated as `undefined` and the flow re-registers / re-authorizes.
**Round-trip the stored object verbatim and you're protected** — single-slot storage
works. The failure modes differ: a stamp naming a **different** AS reads back as
`undefined` and the flow re-registers / re-authorizes. A **missing** stamp (a
`saveTokens()` that rebuilds the object field-by-field and drops `issuer`, or
pre-upgrade storage) is used **as-is** with a `[mcp-sdk]` console warning — SEP-2352
isolation is silently inactive for that read; `auth()` re-stamps on first use where the
provider can persist it. If you see that warning repeatedly, your provider is not
round-tripping the stored object. To hold credentials for several authorization servers at once, key your storage
works. Dropping the stamp is easy to miss: a `saveTokens()` implementation that
rebuilds the object field-by-field and drops `issuer` leaves the value unstamped —
reads still succeed and refresh keeps working, the per-AS issuer check simply does not
apply to that credential, and every read logs an `[mcp-sdk]` warning (`auth()`
re-stamps on first use where the provider can persist it). If you see that warning
repeating after upgrading, check this first. To hold credentials for several authorization servers at once, key your storage
on `ctx.issuer` (treat **`ctx === undefined` as "return the most-recently-saved token
set"** — the transport's per-request `Authorization: Bearer` read calls `tokens()` with
no `ctx`). New TypeScript-only aliases `StoredOAuthTokens` / `StoredOAuthClientInformation`
Expand Down Expand Up @@ -998,8 +999,9 @@ rewrite required unless noted.
#### Client connection & dispatch

- **`connect()` skips the `initialize` handshake when the transport already exposes a
`sessionId`** — it assumes it is reconnecting to an existing session (v1 always
initialized). A custom or test transport that sets `sessionId` at construction
`sessionId`** — it assumes it is reconnecting to an existing session (unchanged from
v1.x, where the same guard has existed since 1.10.0; recorded here because the
far-away symptom keeps surprising migrators). A custom or test transport that sets `sessionId` at construction
silently skips initialization: `getServerCapabilities()` stays `undefined` and the
list verbs return empty results. Expose `sessionId` only after the first request has
been sent.
Expand Down
Loading