docs(migration): align four passages with the implementation#2388
Conversation
Corrections verified against source after the final automated review round landed post-merge on #2382: the SEP-2352 paragraph now describes what a missing issuer stamp actually does (reads succeed, refresh keeps working, the per-AS check does not apply to that credential, a warning is logged on each read) and drops a duplicated clause; the zod-3 paragraph now matches the real failure path (registration does not throw for z.object() schemas - the server starts normally and the first tools/list answers with an error pointing at fromJsonSchema(); only the deprecated raw-shape form errors at registration, pointing at zod/v4); the DiscoverResult bullet now says the cache fields are hidden at the type level only (the runtime object carries both, a cast suffices, raw frames only distinguish omitted hints from defaulted values); and the connect() sessionId-skip bullet notes the same guard exists in v1 since 1.10.0, so it is a gotcha rather than a migration delta.
|
@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}
| - **`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. |
There was a problem hiding this comment.
🟡 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():
ttlMs/cacheScope"are read by the client's response-cache layer";- "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."
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):
- A non-SDK 2026-era server answers
server/discoverwith a result that omitsttlMsandcacheScope. codec.decodeResult('server/discover', raw)safeParses the wire schema (the.catchdefaults exist only inside that throwaway parse), then returns the raw object minusresultType— still with nottlMs/cacheScope.- The looseObject
DiscoverResultSchemapasses it through unchanged;_discoverResultis set to it. - Tooling that follows the doc and casts
getDiscoverResult()to readttlMs/cacheScopegetsundefined, not the documented0/'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".
1 participant
Follow-up to #2382: the final automated review round landed after the merge, leaving four findings unanswered. Each was checked against the implementation before rewording (the other five findings from that round were already addressed by pre-merge pushes).
Motivation and Context
saveTokens(): the guide described the wrong consequence for a provider that drops theissuerfield. Actual behavior: reads still succeed and refresh keeps working; the per-AS issuer check does not apply to that credential and a[mcp-sdk]warning is logged on each read (with re-stamping on first use). Also removes a duplicated clause.z.object()schemas — the server starts and connects normally, and the firsttools/listanswers with an error pointing atfromJsonSchema(). Only the deprecated unwrapped raw-shape form errors at registration (pointing atzod/v4). The previous text described a registration-time error that does not occur on that path.DiscoverResultcache fields: hidden at the type level only — the runtime object still carriesttlMs/cacheScope(readable via a cast); raw frames are needed only to distinguish an omitted hint from the defaulted0/'private'.connect()sessionId skip: the same guard exists in v1 since 1.10.0, so the bullet now presents it as a long-standing gotcha rather than a v1→v2 behavior change.How Has This Been Tested?
Docs-only; each claim verified against the source with file:line evidence before rewording. Prettier clean.
Breaking Changes
None.
Types of changes
Checklist