chore: update spec.types.ts from upstream#2027
Conversation
|
fe613a8 to
5f450ff
Compare
a64a23e to
af35bb5
Compare
| /* Request parameter type that includes input responses and request state. | ||
| * These parameters may be included in any client-initiated request. | ||
| */ | ||
| export interface InputResponseRequestParams extends RequestParams { | ||
| /* New field to carry the responses for the server's requests from the | ||
| * InputRequiredResult message. For each key in the response's inputRequests | ||
| * field, the same key must appear here with the associated response. | ||
| */ | ||
| inputResponses?: InputResponses; | ||
| /* Request state passed back to the server from the client. | ||
| */ | ||
| requestState?: string; | ||
| } |
There was a problem hiding this comment.
🟡 Nit (upstream): the field/interface comments here use plain /* ... */ instead of /** ... */ JSDoc, so the TS language service / TypeDoc will not surface them — and InputResponseRequestParams ends up with no doc comment and no @category Multi Round-Trip tag at all, unlike every sibling type in this section. Since spec.types.ts is not re-exported from this package the SDK's own docs are unaffected, but it's worth folding into the same upstream schema.ts fix as the resultType issue so the spec repo's generated docs and the forthcoming hand-written schemas.ts mirror get proper descriptions.
Extended reasoning...
What the issue is
In the new Multi Round-Trip block, several comments use plain block-comment syntax /* ... */ instead of JSDoc syntax /** ... */:
InputRequiredResult.inputRequests(lines 394-396)InputRequiredResult.requestState(lines 398-402)- the interface-level comment on
InputResponseRequestParams(lines 406-408) InputResponseRequestParams.inputResponses(lines 410-413)InputResponseRequestParams.requestState(lines 415-416)
Because the opening delimiter is /* (single asterisk) rather than /**, TypeScript's language service and TypeDoc treat these as ordinary comments, not documentation. Additionally, since InputResponseRequestParams has no /** */ block at all, it also has no @category Multi Round-Trip tag — every other exported type in this section (InputRequests, InputResponses, InputRequiredResult, TaskInputResponseRequest, etc.) carries that tag.
Why this is an upstream slip, not intentional
This is not the file's convention. Every surrounding declaration in the same diff hunk uses proper /** ... */ JSDoc: ResultType (148-156), InputRequests (354-362), InputResponses (366-375), InputRequiredResult itself (380-392), TaskInputResponseRequest (2031-2039), TaskInputResponseRequestParams (2046-2056). The pre-existing /* Empty result */ and /* Cancellation */ lines are one-line section dividers, not API documentation, so they are not precedent for multi-line field descriptions. The five blocks above are the only multi-line API descriptions in the file using /* — a clear authoring inconsistency in the upstream commit.
Addressing the "not SDK-public" objection
It is true that spec.types.ts is not part of this SDK's public surface — packages/core/src/types/index.ts re-exports constants/enums/errors/guards/schemas/specTypeSchema/types but not spec.types, and the only importer in the package is test/spec.types.test.ts. So this has zero effect on the typescript-sdk's generated docs or consumer .d.ts, and CLAUDE.md's "JSDoc for public APIs" rule does not directly apply to this file in this repo.
The reason it is still worth a (nit-level) mention is that this file is a verbatim mirror of the spec repo's schema/draft/schema.ts, which is the source for the protocol's own TypeDoc site. In the upstream output, InputResponseRequestParams will render with no description and will be uncategorised (it will not appear under the "Multi Round-Trip" group), and the normative "client must treat this as an opaque blob" guidance on requestState will be invisible in IDE hover for anyone consuming the spec types. The fix lives in the same upstream file that already needs editing for the resultType? optionality issue flagged elsewhere on this PR, so the marginal cost of including it in that upstream report is near zero.
Step-by-step proof
- Hover
InputResponseRequestParamsat line 409 in VS Code → tooltip shows onlyinterface InputResponseRequestParams extends RequestParamswith no description, because lines 406-408 start with/*not/**. - Hover
InputRequiredResultat line 393 → tooltip shows the full "An InputRequiredResult sent by the server…" text, because lines 380-392 start with/**. - Hover
requestStateat line 403 → no description; the "client must treat this as an opaque blob" note (398-402) is dropped. - Run TypeDoc over upstream
schema.ts→InputRequests,InputResponses,InputRequiredResultare grouped under Multi Round-Trip;InputResponseRequestParamsis not (no@categorytag, because there is no JSDoc block to carry one).
How to fix
In upstream schema.ts, change /* → /** on the five blocks listed above and add @category Multi Round-Trip to the InputResponseRequestParams doc comment, then re-run pnpm run fetch:spec-types. No change is appropriate in this repo directly (the file header says DO NOT EDIT). This is purely documentation rendering — it does not affect type-checking, the drift guard, or runtime behaviour — hence nit, raised only because an upstream schema.ts fix is already on the table for this sync.
| export interface InputRequiredResult extends Result { | ||
| /* Requests issued by the server that must be complete before the | ||
| * client can retry the original request. | ||
| */ | ||
| inputRequests?: InputRequests; | ||
| /* Request state to be passed back to the server when the client | ||
| * retries the original request. | ||
| * Note: The client must treat this as an opaque blob; it must not | ||
| * interpret it in any way. | ||
| */ | ||
| requestState?: string; | ||
| } |
There was a problem hiding this comment.
🟡 Upstream design gap worth flagging alongside the other schema.ts feedback: InputRequiredResult extends Result without redeclaring resultType: 'input_required', and none of the "complete" subtypes (CallToolResult/ReadResourceResult/GetPromptResult/GetTaskPayloadResult) narrow to resultType: 'complete' either — so the new result: CallToolResult | InputRequiredResult unions (lines 1122/1455/1650/2028) are not TypeScript discriminated unions and if (r.resultType === 'input_required') will not narrow. This is orthogonal to the "resultType is required" comment above (fixing one doesn't fix the other) and constrains the SDK from modeling these with z.discriminatedUnion('resultType', ...) while keeping the bidirectional spec↔SDK assignability check green.
Extended reasoning...
What the bug is
The spec introduces ResultType = 'complete' | 'input_required' and adds resultType: ResultType to the base Result interface (line 165), with the JSDoc explicitly stating its purpose is to "allow the client to determine how to parse the result object." It then defines InputRequiredResult extends Result (line 393) and unions it into four response envelopes — CallToolResultResponse.result: CallToolResult | InputRequiredResult (1650), and likewise for ReadResourceResultResponse (1122), GetPromptResultResponse (1455), and GetTaskPayloadResultResponse (2028).
However, InputRequiredResult does not redeclare resultType: 'input_required', and none of CallToolResult / ReadResourceResult / GetPromptResult / GetTaskPayloadResult redeclare resultType: 'complete'. A grep confirms resultType appears exactly once in spec.types.ts — only on the base Result. So every arm of every X | InputRequiredResult union has resultType typed as the full 'complete' | 'input_required', and TypeScript's discriminated-union narrowing does not engage.
Step-by-step proof
Result.resultType: 'complete' | 'input_required'(line 165).InputRequiredResult extends Result { inputRequests?: ...; requestState?: ... }(line 393) — inheritsresultType: 'complete' | 'input_required'unchanged.CallToolResult extends Result { content: ...; ... }— also inheritsresultType: 'complete' | 'input_required'unchanged.- Given
declare const r: CallToolResult | InputRequiredResult:TypeScript cannot eliminateif (r.resultType === 'input_required') { r.inputRequests; // ❌ TS error: Property 'inputRequests' does not exist on type 'CallToolResult | InputRequiredResult' }
CallToolResultfrom the union becauseCallToolResult['resultType']also includes'input_required'. Narrowing requires the discriminant property to have disjoint literal types across union members. - Additionally, since
Resultcarries[key: string]: unknownandInputRequiredResult's only additions (inputRequests?,requestState?) are optional,InputRequiredResultis structurally a subtype ofCallToolResult— the union is effectively degenerate at the type level.
Why this is distinct from the existing "resultType is required" comment
The comment on line 165 is about resultType being declared required despite @default "complete", which breaks SDK→spec assignability for every result. This finding is about resultType not being narrowed on subtypes, which breaks discriminated-union narrowing within the spec types themselves. They are orthogonal: making resultType optional on Result does not give InputRequiredResult a narrowed discriminant, and adding resultType: 'input_required' to InputRequiredResult does not make the base field optional. Both should be raised upstream together.
Impact on the SDK
The companion schemas.ts work (already requested in another comment on this PR) will need to define InputRequiredResultSchema and the four result: X | InputRequiredResult unions. The natural Zod encoding is z.discriminatedUnion('resultType', [...]), which requires each arm to declare a z.literal(...) discriminant. But if the SDK narrows InputRequiredResultSchema to resultType: z.literal('input_required'), the spec→SDK direction of the bidirectional assignability check in spec.types.test.ts fails: the spec's InputRequiredResult['resultType'] is 'complete' | 'input_required', which is not assignable to the SDK's 'input_required'. So the SDK is forced to either (a) use a non-discriminated z.union and lose narrowing, or (b) narrow anyway and add a carve-out in the bidirectional test — neither is great, and both go away if upstream narrows the discriminant.
At runtime the wire-level discriminator still works (a client can string-compare resultType and cast), so this is a type-ergonomics / SDK-modeling defect rather than a protocol-correctness one.
How to fix
Upstream in schema.ts: add resultType: 'input_required'; to InputRequiredResult, and add resultType?: 'complete'; (or required 'complete', depending on how the optionality question is resolved) to each concrete "complete" result that participates in an | InputRequiredResult union — at minimum CallToolResult, ReadResourceResult, GetPromptResult, GetTaskPayloadResult. Then re-run pnpm run fetch:spec-types. Batch this with the resultType? optionality fix and the other upstream feedback already noted on this PR.
af35bb5 to
879139a
Compare
| export type InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest; | ||
|
|
||
| /** @internal */ | ||
| export const URL_ELICITATION_REQUIRED = -32042; | ||
| export type InputResponse = CreateMessageResult | ListRootsResult | ElicitResult; |
There was a problem hiding this comment.
🟡 Upstream design gap to batch with the other schema.ts feedback: InputResponse = CreateMessageResult | ListRootsResult | ElicitResult admits only success payloads, and the InputResponseRequestParams.inputResponses comment says every inputRequests key "must appear here". In the pre-sync model these were full JSON-RPC requests so a client could return a JSONRPCErrorResponse for user-denied sampling / LLM provider error / no roots configured; that error channel is gone with no replacement (ElicitResult has action: 'decline'|'cancel', but CreateMessageResult and ListRootsResult have no refusal field). The SDK redesign called for in the earlier comment will have no spec-defined way to propagate per-request failures and would have to invent an out-of-spec workaround that breaks once upstream adds a real error variant — suggest upstream add an InputResponseError arm (mirroring the JSON-RPC {code, message, data} shape) or relax the must-appear rule.
Extended reasoning...
What the bug is
InputResponse (spec.types.ts:351) is defined as CreateMessageResult | ListRootsResult | ElicitResult — a union of three success payloads only. InputResponses (line 376-378) is { [key: string]: InputResponse }, and the normative-sounding comment on InputResponseRequestParams.inputResponses (lines 410-413) says: "For each key in the response's inputRequests field, the same key must appear here with the associated response." So per the spec text, the client must supply a value for every requested key, and that value must be one of the three success result shapes.
In the pre-sync model these three exchanges were ordinary server→client JSON-RPC requests (CreateMessageRequest extends JSONRPCRequest, etc.), so a client that could not or would not fulfil one returned a JSONRPCErrorResponse carrying { code: number; message: string; data?: unknown }. By stripping extends JSONRPCRequest / extends Result and embedding the exchange inside the InputRequests/InputResponses maps, the spec discarded that error channel without adding a replacement.
Why the existing types don't cover it
ElicitResulthappens to carryaction: 'accept' | 'decline' | 'cancel', so elicitation can encode refusal in-band.CreateMessageResult(line ~2335) isSamplingMessage & { model: string; stopReason?: ... }— requiredmodel/role/content, no refusal/error field, no index signature now thatextends Resultis dropped.ListRootsResult(line ~2826) is bare{ roots: Root[] }— same story.
The asymmetry (only ElicitResult has a decline arm) suggests this is an oversight rather than an intentional "refusal-via-abandonment" design — if abandoning the retry were the intended refusal signal, ElicitResult would not need action: 'decline' either.
Step-by-step proof
- Server returns
CallToolResultResponsewithresult: InputRequiredResult { inputRequests: { 's1': <CreateMessageRequest>, 'e1': <ElicitRequest> } }. - Client presents the elicitation; user accepts →
{ action: 'accept', content: {...} }. - Client attempts the sampling call; the LLM provider returns HTTP 429 / content-policy refusal / the user denies the sampling-consent prompt.
- Client must now construct
inputResponsesfor the retry. Per lines 410-413, both's1'and'e1'must appear.'e1'is fine. For's1'the only spec-permitted shapes areCreateMessageResult | ListRootsResult | ElicitResult— none of which can express "this request failed with code X / message Y". - The client's only options are: (a) omit
's1'— textually non-compliant with the must-appear rule; (b) abandon the whole retry / calltasks/cancel— loses the successful elicitation and gives the server no error code/message to act on; (c) stuff a fakeCreateMessageResultor an off-spec{ error: {...} }object into's1'— out of spec.
Why this matters for the SDK companion work
Today a client-side setRequestHandler(CreateMessageRequestSchema, …) can throw / reject and the SDK serialises a JSONRPCErrorResponse; the server's await ctx.sample() rejects with a ProtocolError carrying the code and message, and the tool handler can catch it and fall back. The redesign called for in comment #1 must preserve some equivalent of those semantics, but there is no spec-defined wire shape to carry them. Whatever workaround the SDK picks (drop the key, cancel the whole flow, invent an envelope) becomes public API that breaks once upstream adds a real error variant.
This is distinct from comment #1 (about the SDK still issuing these as wire requests), comment #4 (about tasks/input_response lacking taskId), and comment #6 (about the resultType discriminant) — none of those address the missing error arm in the InputResponse payload union itself.
Addressing "all-or-nothing is the intended design"
It is plausible the spec intends atomic fulfilment (client either satisfies every input or abandons). But (a) that loses the error code/message — the server cannot distinguish rate-limit vs. content-policy vs. user-denied, which is a real expressiveness regression vs. the JSON-RPC model; (b) it is contradicted by ElicitResult.action: 'decline'|'cancel' existing at all; and (c) even under that reading, the SDK still has to pick a public error-propagation contract for ctx.sample() rejection in the new flow, and it is better to do so against a spec-defined error variant than to guess.
How to fix
Raise upstream alongside the resultType? / taskId / discriminant fixes: either add an error arm, e.g.
export interface InputResponseError {
code: number;
message: string;
data?: unknown;
}
export type InputResponse = CreateMessageResult | ListRootsResult | ElicitResult | InputResponseError;(mirroring the removed JSON-RPC Error shape), or relax the "must appear" wording on inputResponses and define omission as refusal. Then re-run pnpm run fetch:spec-types. Don't encode InputResponsesSchema in schemas.ts until this is settled. Filed as a nit because it doesn't independently break CI and abandon-the-flow is a viable (if lossy) fallback — same tier as comments #5/#6.
879139a to
81ef4d8
Compare
81ef4d8 to
29aa626
Compare
| export interface RequestParams { | ||
| _meta?: RequestMetaObject; | ||
| _meta: RequestMetaObject; | ||
| } |
There was a problem hiding this comment.
🟡 Upstream spec self-inconsistency to batch with the other schema.ts feedback: RequestParams._meta is now required with three required client-identity keys (io.modelcontextprotocol/{protocolVersion,clientInfo,clientCapabilities}), but RequestParams is still referenced from contexts where that doesn't fit — DiscoverRequest.params? and PaginatedRequest.params? are still optional (so a client can type-validly omit the "Required" handshake metadata entirely, and discovery is circular: you must commit to a protocolVersion to ask which versions are supported); the server→client ListTasksRequest inherits PaginatedRequestParams → RequestParams, forcing a server sending tasks/list with a cursor to populate the client's clientInfo/clientCapabilities; and the server-authored ListRootsRequest (now only an InputRequest payload) kept params?: RequestParams while its siblings CreateMessageRequest/ElicitRequest had their RequestParams inheritance stripped. Suggest upstream make the three keys optional (or split RequestMetaObject by direction), make params non-optional on client wire requests, exempt server/discover, and drop the RequestParams reference from ListRootsRequest.
Extended reasoning...
What the issue is
This sync changes RequestParams (line 145) from _meta?: RequestMetaObject to _meta: RequestMetaObject and adds three required keys to RequestMetaObject (lines 82/89/97): 'io.modelcontextprotocol/protocolVersion', 'io.modelcontextprotocol/clientInfo', and 'io.modelcontextprotocol/clientCapabilities', each with JSDoc explicitly saying "Required." The intent — moving the handshake from a one-time initialize to per-request _meta — is clear, but RequestParams is still used in three places where mandatory client-identity metadata is either omittable, semantically wrong, or circular. This is the spec being internally inconsistent, distinct from comment #3223937258 which only notes that the SDK's RequestParamsSchema doesn't match the new required _meta.
Surface 1 — params? still optional on client wire requests
DiscoverRequest.params?: RequestParams (line 568) and PaginatedRequest.params?: PaginatedRequestParams (line 1014, inherited by ListResourcesRequest / ListResourceTemplatesRequest / ListPromptsRequest / ListToolsRequest / ListTasksRequest) declare params as optional. So { jsonrpc: '2.0', id: 1, method: 'tools/list' } is type-valid yet carries no _meta and therefore none of the "Required" handshake metadata — the spec contradicts itself. Pre-sync this was consistent because _meta was optional; the upstream commit tightened _meta without tightening params.
The DiscoverRequest case is additionally circular. Per its JSDoc, server/discover exists so the client can learn supportedVersions for use in subsequent requests, and per the JSDoc on protocolVersion (line 80) the server MUST return UnsupportedProtocolVersionError if the value is not supported. But if the client supplies params, it must include _meta['io.modelcontextprotocol/protocolVersion'] — i.e., commit to a version before learning which versions are supported. Either DiscoverRequest should not use RequestParams, or protocolVersion should be optional/exempt for discovery; the params? escape hatch is at best implicit and contradicts the unconditional "Required." prose.
Surface 2 — server→client wire request inherits client-direction keys
ServerRequest (line 3280) = GetTaskRequest | GetTaskPayloadRequest | ListTasksRequest | CancelTaskRequest. Three of these use inline params: { taskId: string } and escape, but ListTasksRequest (line 2155) extends PaginatedRequest → params?: PaginatedRequestParams (line 1004) extends RequestParams → _meta: RequestMetaObject with required clientInfo/clientCapabilities. The JSDoc on those keys is unambiguously client→server ("Identifies the client software making the request", "The client's capabilities for this specific request", "Servers MUST NOT infer capabilities from prior requests"). So per the type, a server sending tasks/list with a pagination cursor must fabricate the client's identity and capabilities — semantically nonsensical. Concrete SDK consequence: when the companion work adds per-request _meta injection to Protocol.request(), the server-side outbound path has no sensible value to put here.
Surface 3 — server-authored embedded ListRootsRequest payload
ListRootsRequest (line 2820) dropped extends JSONRPCRequest (it is now only an InputRequest payload embedded in server-emitted InputRequiredResult.inputRequests, per InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest at line 438), but unlike its two siblings it kept params?: RequestParams (line 2822). The siblings were cleaned: CreateMessageRequestParams (line 2257) and ElicitRequestFormParams/ElicitRequestURLParams (lines 2880/2913) all dropped extends TaskAugmentedRequestParams, so they no longer reference RequestParams/_meta at all. ListRootsRequest was missed — likely because it had no dedicated *Params wrapper to edit. params? is optional so a server can omit it, but a server wanting to attach e.g. _meta.progressToken would be type-forced to also fabricate clientInfo/clientCapabilities/protocolVersion for a server-authored embedded payload.
Step-by-step proof
RequestMetaObject(lines 74-97) declares'io.modelcontextprotocol/protocolVersion': string,'…/clientInfo': Implementation,'…/clientCapabilities': ClientCapabilities— none with?.RequestParams(line 145) declares_meta: RequestMetaObject— no?.PaginatedRequest(line 1014) declaresparams?: PaginatedRequestParams— with?.PaginatedRequestParams extends RequestParams(line 1004).ListToolsRequest extends PaginatedRequest→ a client may send{ jsonrpc:'2.0', id:1, method:'tools/list' }with noparams→ no_meta→ noprotocolVersion/clientInfo/clientCapabilities. Type-valid, but the JSDoc on each key says "Required."ListTasksRequest extends PaginatedRequestand is a member ofServerRequest(line 3280). A server paginatingtasks/listconstructsparams: { cursor: '…', _meta: { 'io.modelcontextprotocol/clientInfo': ???, 'io.modelcontextprotocol/clientCapabilities': ???, … } }— there is no value a server can sensibly put for the client's identity.ListRootsRequest(line 2822) keepsparams?: RequestParams, whileCreateMessageRequest.params: CreateMessageRequestParamsandElicitRequest.params: ElicitRequestParamsno longer extendRequestParams—ListRootsRequestis the odd one out among the threeInputRequestmembers.
Impact
None of these independently break CI beyond what comment #3223937258 already documents (the SDK↔spec RequestParams assignability failure). They are upstream design gaps in the same tier as comments #3214351591 (taskId missing), #3214351594 (discriminant not narrowed), #3216586352 (tasks.requests orphaned), and #3216586357 (InputResponse no error arm) — they don't add new red CI, but they identify shapes the SDK should not encode as-is. In particular, the server-side Protocol.request() _meta-injection redesign called for in #3223937258 cannot follow the same logic as the client side while RequestMetaObject is client-direction-only.
How to fix
Raise upstream alongside the other schema.ts feedback. Plausible fixes (any one resolves all three surfaces, or they can be combined):
- Make the three
io.modelcontextprotocol/*keys optional onRequestMetaObject(or split intoClientRequestMetaObject/ServerRequestMetaObject), and have client→server wire request types makeparamsnon-optional so the handshake metadata is actually carried. - Have
DiscoverRequestuse a params type that does not extendRequestParams(or document an explicit exemption from theprotocolVersionrejection rule forserver/discover). - Have server→client request params (concretely
ListTasksRequestviaPaginatedRequest) not extendRequestParams, or use a server-direction_metashape. - Change
ListRootsRequest.params?to a bare{ _meta?: MetaObject }or dropparamsentirely, matchingCreateMessageRequest/ElicitRequest.
Then re-run pnpm run fetch:spec-types.
There was a problem hiding this comment.
Partially superseded by the latest re-pull (c47bd846), which deletes the entire tasks subsystem.
Surface 2 is now moot: ListTasksRequest and ServerRequest no longer exist (the spec defines no server→client requests at all), so there is no longer a server→client wire request inheriting the client-direction _meta keys via PaginatedRequestParams → RequestParams.
Surfaces 1 and 3 remain fully applicable:
RequestParams._metais still required (line 128) with the three requiredio.modelcontextprotocol/*keys, whileDiscoverRequest.params?(line 549) andPaginatedRequest.params?(line 932) are still optional — a client can still type-validly omit the "Required" handshake metadata, and theserver/discovercircularity (must commit to aprotocolVersionto ask which versions are supported) is unchanged.ListRootsRequest(line 2452) still keepsparams?: RequestParamswhile itsInputRequestsiblingsCreateMessageRequest/ElicitRequesthave had theirRequestParamsinheritance stripped — still the odd one out among server-authored embedded payloads.
The remaining upstream feedback to batch: make the three keys optional (or split RequestMetaObject by direction), make params non-optional on client wire requests / exempt server/discover, and drop the RequestParams reference from ListRootsRequest.
There was a problem hiding this comment.
Updated against the latest re-pull (bbcfc6) — the prior follow-up is now partially stale.
params? optionality (Surface 1, sub-issue) — now resolved upstream: DiscoverRequest.params (line 558) and PaginatedRequest.params (line 941) are now declared required (no ?), so a client can no longer type-validly omit the handshake _meta on those wire requests. This was one of the suggested fixes ("make params non-optional on client wire requests").
Discover circularity (Surface 1, sub-issue) — still applicable, arguably tightened: with DiscoverRequest.params: RequestParams now required, the client must populate _meta['io.modelcontextprotocol/protocolVersion'] to call server/discover at all — i.e. it must commit to a version before learning which versions are supported. The escape hatch is that the server responds with UnsupportedProtocolVersionError.data.supported (now a structured array), so a client can recover by trying a guess and reading the 400 body. Whether that should require an explicit exemption for server/discover remains an upstream design question.
ListRootsRequest (Surface 3) — still applicable: ListRootsRequest (line 2459) still keeps params?: RequestParams while its InputRequest siblings CreateMessageRequest/ElicitRequest had their RequestParams inheritance stripped — still the odd one out among server-authored embedded payloads.
Surface 2 — moot (unchanged from prior follow-up; ListTasksRequest/ServerRequest are gone).
Remaining upstream feedback to batch: exempt server/discover from the protocolVersion requirement (or document the try-and-recover-via-data.supported pattern as the intended bootstrap), and drop the RequestParams reference from ListRootsRequest.
There was a problem hiding this comment.
Updated against the latest re-pull (91b403f8) — most of this comment is now addressed upstream; one sub-issue remains.
ListRootsRequest (Surface 3) — now resolved upstream: ListRootsRequest.params (line 2598-2603) has been changed from params?: RequestParams to a bare params?: { _meta?: MetaObject } — exactly the fix suggested in the original comment ("Change ListRootsRequest.params? to a bare { _meta?: MetaObject } … matching CreateMessageRequest/ElicitRequest"). The server-authored embedded payload no longer references the client-direction RequestParams/RequestMetaObject.
params? optionality (Surface 1, sub-issue) — remains resolved (unchanged from the prior follow-up): DiscoverRequest.params (line 612) and PaginatedRequest.params are required, so the handshake _meta cannot be type-validly omitted on client wire requests.
Discover circularity (Surface 1, sub-issue) — still applicable, now partially mitigated by prose: RequestParams._meta (line 156) still requires the three io.modelcontextprotocol/* keys (lines 83/90/98), and DiscoverRequest.params: RequestParams is required, so a client must still commit to a protocolVersion to call server/discover. The new DiscoverRequest JSDoc (lines 600-603) does soften this — "Clients MAY call it but are not required to — version negotiation can also happen inline via per-request _meta" — and UnsupportedProtocolVersionError.data.supported provides the try-and-recover path, but there is still no explicit exemption from the protocolVersion rejection rule for server/discover itself.
Surface 2 — moot (unchanged; ListTasksRequest/ServerRequest are gone).
Remaining upstream feedback to batch is now narrow: either exempt server/discover from the protocolVersion requirement or document the try-a-version-and-recover-via-data.supported pattern as the intended bootstrap.
29aa626 to
07e2103
Compare
| * Capabilities that a server may support. Known capabilities are defined here, in this schema, but this is not a closed set: any server can define its own, additional capabilities. | ||
| * | ||
| * @category `initialize` | ||
| * @category `server/discover` |
There was a problem hiding this comment.
🟡 🟡 Upstream spec self-inconsistency to batch with the other schema.ts feedback (server-side analogue of comment #3216586352): the same diff that deletes SubscribeRequest/UnsubscribeRequest and makes all list-changed notifications opt-in via SubscriptionFilter leaves ServerCapabilities.resources.subscribe (line 776) and {prompts,resources,tools}.listChanged (755/780/795) untouched, and adds no capability flag for subscriptions/listen itself. The flags are not dead — each maps 1:1 to a SubscriptionFilter field — but their documented semantics have silently shifted from "server will push unsolicited *_list_changed" / "server accepts the resources/subscribe RPC" to "server will honor the corresponding SubscriptionFilter opt-in on a subscriptions/listen stream", and the JSDoc/@example references still describe the old model. Suggest upstream either re-document these four flags for the opt-in model or fold them into a structured subscriptions: { listen?, resourceSubscriptions?, …ListChanged? } capability so the SDK's subscriptions/listen client API (replacing the client.ts:599 gate) has an unambiguous flag to check.
Extended reasoning...
What the issue is
This sync replaces the per-resource resources/subscribe / resources/unsubscribe RPCs and unsolicited *_list_changed pushes with a single opt-in subscriptions/listen stream: the client sends a SubscriptionFilter naming which notification types it wants, and the server echoes back what it will honor in SubscriptionsAcknowledgedNotification.params.notifications (the JSDoc on SubscriptionFilter says the server "MUST NOT send notification types the client has not explicitly requested"). But ServerCapabilities — structurally untouched by this diff beyond the @category initialize → server/discover rename at line 721 — still declares:
resources.subscribe?: boolean(line 776) — "Whether this server supports subscribing to resource updates"prompts.listChanged?: boolean/resources.listChanged?: boolean/tools.listChanged?: boolean(lines 755/780/795) — "Whether this server supports notifications for changes to the … list"
and adds no field advertising whether the server implements subscriptions/listen at all.
Addressing the "these flags map cleanly to SubscriptionFilter" objection
A reasonable counter-read is that these four flags are not orphaned: each corresponds 1:1 to a SubscriptionFilter field (resources.subscribe ↔ resourceSubscriptions, {prompts,resources,tools}.listChanged ↔ {prompts,resources,tools}ListChanged), the underlying notifications (ResourceUpdatedNotification, the three *ListChangedNotification types) all still exist in ServerNotification, and the JSDoc text on resources.subscribe ("subscribing to resource updates") is mechanism-agnostic. Under this reading, the static DiscoverResult.capabilities flags tell a client what to put in its SubscriptionFilter before opening a stream, and SubscriptionsAcknowledgedNotification confirms per-stream what was honored — two different purposes, not duplication. That counter-read is largely correct, and is why this is not the same as comment #3216586352's ClientCapabilities.tasks.requests case (where the advertised concept — task-augmented sampling/elicitation wire requests — was deleted entirely).
What that reading does not address is that the semantics of these flags changed without the spec saying so. Pre-sync, tools.listChanged: true meant "this server will send unsolicited notifications/tools/list_changed"; post-sync, per the SubscriptionFilter JSDoc, the server MUST NOT send that notification unless the client opts in on a subscriptions/listen stream — so the flag now means "this server will honor toolsListChanged: true in your filter." Pre-sync, resources.subscribe: true meant "this server accepts resources/subscribe requests" (and client.ts:599 gates on exactly that); post-sync, that RPC does not exist, so the flag must mean "this server will honor resourceSubscriptions: [...] in your filter." Neither shift is reflected in the JSDoc, and the @example {@includeCode ./examples/ServerCapabilities/resources-subscription-*.json} references (lines 763-770) still point at example files written for the old model. An implementer reading ServerCapabilities in isolation would reasonably conclude the server pushes list-changed notifications unprompted and accepts a resources/subscribe RPC — both now wrong.
Separately, there is no explicit capability for subscriptions/listen itself. Under the reinterpretation above, support is implied by any of the four sub-flags being present — but that is undocumented inference, and a server that implements subscriptions/listen for (say) LoggingMessageNotification only, with none of the four sub-flags, has no way to advertise the endpoint exists.
Step-by-step proof
- Diff deletes
SubscribeRequest/UnsubscribeRequest/SubscribeResultResponse/UnsubscribeResultResponse;ClientRequest(line 3256) no longer contains either. - Diff adds
SubscriptionFilter(line 1176) withtoolsListChanged?/promptsListChanged?/resourcesListChanged?/resourceSubscriptions?: string[]and JSDoc "the server MUST NOT send notification types the client has not explicitly requested here."SubscriptionFilter.resourceSubscriptionsJSDoc (line 1194) explicitly says "Replaces the formerresources/subscribeRPC." ServerCapabilitiesbody (lines 723-833) is byte-identical to pre-sync apart from the@categorytag.resources.subscribe(776),prompts.listChanged(755),resources.listChanged(780),tools.listChanged(795) all survive with pre-sync JSDoc. Grep forsubscriptionsunderServerCapabilities→ no match.- Pre-sync semantics of
prompts.listChanged: true: server will sendnotifications/prompts/list_changedwhenever its prompt set changes (no client opt-in required). Post-sync semantics per step 2: server MUST NOT send it unless the client includedpromptsListChanged: truein asubscriptions/listenfilter. The JSDoc at line 753 still reads "Whether this server supports notifications for changes to the prompt list" — silent on the new opt-in requirement. - SDK surface:
packages/client/src/client/client.ts:599doesif (method === 'resources/subscribe' && !this._serverCapabilities.resources.subscribe) throw …. After the redesign already requested in comment #3223937258 that branch is removed, and the newsubscriptions/listenclient API will need a capability gate — under the current spec it must either reuseresources.subscribe/*.listChangedunder their reinterpreted meaning, or ship ungated.
Why this is distinct from existing PR comments
Comment #3216586352 covers the client-side mirror (ClientCapabilities.tasks.requests orphaned by InputRequiredResult). Comment #3223937258 covers the SDK lifecycle redesign, including replacing subscribeResource()/unsubscribeResource() — but that comment is about what the SDK must change, not about the upstream schema.ts leaving ServerCapabilities documenting the old subscription model. Comment #3231781722 covers SubscriptionsListenRequest lacking a JSON-RPC response wrapper. None of #1-#13 mention ServerCapabilities.resources.subscribe / *.listChanged or a missing subscriptions capability.
Impact
No independent CI breakage — all four fields are optional, the ServerCapabilities body is untouched, so spec.types.test.ts's bidirectional check for ServerCapabilities still passes. This is upstream documentation/modeling feedback in the same tier as #3214351593 (JSDoc), #3216586352 (tasks.requests), #3231781731 (RequestParams direction). Filed as nit.
How to fix
Raise upstream alongside the other schema.ts feedback. Two coherent options:
- Re-document in place: keep the four flags, update their JSDoc to reference
SubscriptionFilter(e.g.,resources.subscribe→ "Whether this server will honorresourceSubscriptionson asubscriptions/listenstream";*.listChanged→ "Whether this server will honor*ListChangedon asubscriptions/listenstream"), and refresh the@exampleJSON files. Optionally add a top-levelsubscriptions?: JSONObjectto advertise the endpoint independently of which filter fields are honored. - Restructure: replace the four scattered flags with
subscriptions?: { listen?: JSONObject; resourceSubscriptions?: boolean; toolsListChanged?: boolean; promptsListChanged?: boolean; resourcesListChanged?: boolean }so the capability shape mirrorsSubscriptionFilter.
Then re-run pnpm run fetch:spec-types. Once re-pulled, ServerCapabilitiesSchema in schemas.ts and the client.ts capability gate for the new subscriptions/listen API can follow whichever shape upstream picks.
07e2103 to
0bcec2b
Compare
| export interface DiscoverRequest extends JSONRPCRequest { | ||
| method: 'server/discover'; | ||
| params?: RequestParams; | ||
| } |
There was a problem hiding this comment.
🔴 Another silent transport-layer divergence (same class as #3216586348/#3239359153, distinct surface): the Streamable HTTP server transport's entire Mcp-Session-Id lifecycle is hard-keyed on isInitializeRequest() (packages/server/src/server/streamableHttp.ts:666) — sessionId minting (678), _initialized = true (679), and the public onsessioninitialized hook (683-684) only fire for method: 'initialize'; any other first request hits validateSession() and 400s "Server not initialized" in stateful mode (847-856). After this sync InitializeRequest is gone — a spec-compliant client sends server/discover or just tools/list-with-_meta and never initialize, so a stateful SDK HTTP server 400s every request and never establishes a session. Lines 720-722 also read the deleted initRequest.params.protocolVersion (version now lives in _meta['io.modelcontextprotocol/protocolVersion']). And grepping post-sync spec.types.ts for "session" returns zero matches — the stateless overhaul appears to have dropped Mcp-Session-Id semantics entirely, so the redesign must explicitly decide remove-sessions vs. mint-on-first-request vs. re-key-on-server/discover, and update the public isInitializeRequest guard (guards.ts:93, exports/public/index.ts:111) and sessionIdGenerator/onsessioninitialized/onsessionclosed transport options (streamableHttp.ts:80/89/101) accordingly.
Extended reasoning...
What this finding covers
The Streamable HTTP server transport's session-ID bootstrap is hard-wired to the deleted initialize method, and the post-sync spec appears to have dropped Mcp-Session-Id session semantics entirely. This is a third silent transport-layer divergence in packages/server/src/server/streamableHttp.ts that neither comment #3223937258 (Protocol-layer server.ts/client.ts initialize handling) nor comment #3239359153 (streamableHttp.ts GET→subscriptions/listen + HTTP-400 MUSTs) covers.
The code path
handlePostRequest in packages/server/src/server/streamableHttp.ts:
- Line 666:
const isInitializationRequest = messages.some(element => isInitializeRequest(element))—isInitializeRequest(guards.ts:93) checks formethod: 'initialize'. - Lines 667-685 (the
if (isInitializationRequest)block):this.sessionId = this.sessionIdGenerator?.()(678),this._initialized = true(679), andawait this._onsessioninitialized(this.sessionId)(683-684). This is the only placesessionIdis minted and_initializedis flipped. - Lines 687-694 (the
elsepath): every non-initialize POST goes throughvalidateSession(req). validateSession()(847-856): in stateful mode (sessionIdGeneratorset),if (!this._initialized)→return this.createJsonErrorResponse(400, -32_000, 'Bad Request: Server not initialized').- Lines 720-722:
const initRequest = messages.find(m => isInitializeRequest(m)); const clientProtocolVersion = initRequest ? initRequest.params.protocolVersion : …— readsparams.protocolVersion, a field of the deletedInitializeRequestParams. In the new model the protocol version lives inparams._meta['io.modelcontextprotocol/protocolVersion'].
Why nothing in CI surfaces it
streamableHttp.ts imports isInitializeRequest from @modelcontextprotocol/core (guards.ts), not from spec.types.ts. InitializeRequestSchema still exists in schemas.ts, so guards.ts still typechecks; the transport never references spec.types.ts at all. Neither tsc --noEmit, spec.types.test.ts, nor the specTypeSchema allowlist guard sees this — same silent class as #3216586348 (-32042) and #3239359153 (GET endpoint).
Step-by-step proof
- User constructs the documented stateful server:
new StreamableHTTPServerTransport({ sessionIdGenerator: () => crypto.randomUUID(), onsessioninitialized: id => sessions.set(id, …) })(per the JSDoc example at streamableHttp.ts:195). - A spec-compliant client built against the post-sync schema connects. Per this diff there is no
initializemethod; the client either sends{ method: 'server/discover' }first or goes straight to{ method: 'tools/list', params: { _meta: { 'io.modelcontextprotocol/protocolVersion': …, 'io.modelcontextprotocol/clientInfo': …, 'io.modelcontextprotocol/clientCapabilities': … } } }. handlePostRequestparses the body.isInitializeRequest({ method: 'server/discover', … })→ false (it checks for'initialize').isInitializationRequest = false.- Control falls to line 687-691 →
validateSession(req).sessionIdGeneratoris set,this._initializedis stillfalse(never flipped) → returns 400 "Bad Request: Server not initialized". - The client retries; same result. No request ever succeeds,
sessionIdis never minted,onsessioninitializednever fires, the user's session map stays empty. - Separately, even if a legacy client sent
initialize, line 722 readsinitRequest.params.protocolVersion— fine for legacy clients, but once the redesign re-keys the bootstrap onserver/discover(whoseparams?: RequestParamshas noprotocolVersionfield), this read returnsundefinedand the priming-event protocol-version logic silently picks the wrong branch.
The bigger design question
grep -i session packages/core/src/types/spec.types.ts post-sync → zero matches. The stateless overhaul has apparently dropped Mcp-Session-Id from the protocol entirely (consistent with "every request carries its own _meta handshake"). That means the SDK's public stateful-mode surface — sessionIdGenerator (line 80), onsessioninitialized (89), onsessionclosed (101), the Mcp-Session-Id header read at 859, the DELETE-ends-session path at 838, plus the four middleware READMEs and streamableHttp.examples.ts that demonstrate per-session transport maps keyed on isInitializeRequest — is now unanchored from the spec. The redesign must explicitly choose one of:
- (a) Remove sessions from the transport (stateful mode goes away;
sessionIdGenerator/onsessioninitialized/onsessioncloseddeprecated → breaking change, migration.md entry). - (b) Mint on first request regardless of method (re-key the bootstrap on "first POST without
Mcp-Session-Idheader" instead of "isinitialize"). - (c) Re-key on
server/discover(closest 1:1 swap, butserver/discoveris optional per its JSDoc, so clients that skip it would still 400).
Whichever is chosen, the public isInitializeRequest guard (re-exported at exports/public/index.ts:111, used verbatim in the four middleware READMEs and streamableHttp.examples.ts for per-session routing) needs deprecation or replacement, and lines 720-722 need to read _meta['io.modelcontextprotocol/protocolVersion'] instead of params.protocolVersion.
Why this is distinct from existing comments
- #3223937258 (stateless overhaul) is scoped to the Protocol layer:
client.connect()sendinginitialize,server.tsregistering aninitializehandler /assertInitialized()gating, and the schemas/guards cleanup. Its remediation list never mentions the transport'sisInitializeRequest-gated session bootstrap,sessionIdGenerator,_initialized,onsessioninitialized,validateSession(), or theMcp-Session-Idlifecycle. - #3239359153 is the only existing comment touching
streamableHttp.ts, and it covers exactly two things: (1) GET→subscriptions/listen(handleGetRequest/_standaloneSseStreamId) and (2) the new HTTP-400 MUSTs (validateProtocolVersion()header↔body cross-check, error-code→status hook). It does not mention the session-ID bootstrap path,isInitializationRequest, or theparams.protocolVersionread at 720-722.
A reviewer following only those two comments would rip out assertInitialized() in server.ts and rewire the GET handler — and still ship a stateful HTTP transport that 400s every request from a spec-compliant client.
How to fix
Add to the companion-redesign scope:
- Decide (a)/(b)/(c) above for the
Mcp-Session-Idlifecycle and updatestreamableHttp.ts:664-700+validateSession()accordingly. - Replace the
initRequest.params.protocolVersionread (720-722) withparams._meta['io.modelcontextprotocol/protocolVersion'](or the header, since the bootstrap branch will no longer be exempt from header validation). - Deprecate or repurpose
isInitializeRequest(guards.ts:93, public re-export at exports/public/index.ts:111) and updatepackages/middleware/{node,hono,fastify,express}/README.md+streamableHttp.examples.tswhich use it for per-session transport routing. - If sessions are removed, deprecate
sessionIdGenerator/onsessioninitialized/onsessionclosed(breaking → changeset + migration.md).
0bcec2b to
57ccae0
Compare
e9de0a8 to
786d94c
Compare
786d94c to
edb9e09
Compare
edb9e09 to
1896892
Compare
1896892 to
b255ba4
Compare
b255ba4 to
da9d82e
Compare
ef5e88c to
bab2413
Compare
86d0536 to
fa89096
Compare
0435878 to
4c6b66f
Compare
4c6b66f to
20baa74
Compare
20baa74 to
234b0fc
Compare
| } | ||
|
|
||
| /** | ||
| * This notification can be sent by either side to indicate that it is cancelling a previously-issued request. | ||
| * This notification is sent by the client to indicate that it is cancelling a request it previously issued. | ||
| * | ||
| * On stdio, the server also sends this notification, solely to terminate a {@link SubscriptionsListenRequest | subscriptions/listen} stream: it references the ID of the `subscriptions/listen` request that opened the stream. Servers MUST NOT use this notification to cancel any other request. | ||
| * |
There was a problem hiding this comment.
🟡 Companion-work scoping note: the rewritten CancelledNotification JSDoc (lines 597–603) adds a normative server→client use on stdio — the server terminates a subscriptions/listen stream by sending notifications/cancelled referencing the ID of the listen request the client issued — but Protocol._oncancel (packages/core/src/shared/protocol.ts:345-352) only looks up _requestHandlerAbortControllers (incoming-request IDs) and never consults _responseHandlers/_progressHandlers, so a cancellation naming the receiver's own outgoing listen request is silently dropped and the pending request would hang until a misleading RequestTimeout. When the companion subscriptions/listen work lands, the cancelled-notification path needs a receiver-side route that settles/aborts the local outstanding listen request (gated per the spec's 'MUST NOT cancel any other request' constraint).
Extended reasoning...
What changed in the spec
This re-pull rewrites the CancelledNotification JSDoc (spec.types.2026-07-28.ts:597-603). The notification is now client→server for ordinary cancellation, and it gains one new normative server→client use: "On stdio, the server also sends this notification, solely to terminate a subscriptions/listen stream: it references the ID of the subscriptions/listen request that opened the stream. Servers MUST NOT use this notification to cancel any other request." In other words, the receiver of the cancellation is now expected to react to a cancellation that names a request the receiver itself issued (its own outgoing listen request) — not a request it is currently handling.
What the SDK does today
Protocol._oncancel (packages/core/src/shared/protocol.ts:345-352) handles every incoming notifications/cancelled by looking up this._requestHandlerAbortControllers.get(params.requestId) — a map keyed exclusively by incoming request IDs the local side is handling (populated in _onrequest at protocol.ts:512). It never consults _responseHandlers / _progressHandlers / _timeoutInfo (the maps tracking the local side's own outgoing requests, protocol.ts:289-291). A cancelled notification whose requestId names one of the receiver's own outstanding outgoing requests therefore finds no abort controller and silently returns: the pending Protocol.request() promise is never settled, no handler is informed, and nothing is cleaned up.
Step-by-step proof (once the companion subscriptions/listen work lands)
- An SDK client on stdio opens the notification stream by issuing
subscriptions/listen(the 2026-07-28 replacement for the standalone GET stream). The request ID lands in_responseHandlers/_progressHandlers. - The server later shuts the stream down using the only spec-defined mechanism on stdio: it sends
notifications/cancelledwith the listen request's ID. - The SDK client's default
notifications/cancelledhandler (registered in theProtocolconstructor, protocol.ts:323-325) routes to_oncancel. _oncancellooks up_requestHandlerAbortControllers.get(requestId)— the listen request is an outgoing request, so there is no entry — and returns.- The client never learns the stream was terminated: the listen request stays pending in
_responseHandlersuntil its timeout fires with a misleadingRequestTimeout, the subscription bookkeeping is never torn down, and there is no hook asubscriptions/listenimplementation could use to observe the termination.
Why nothing in CI surfaces it and why existing comments don't cover it
shared/protocol.ts does not import the spec snapshot, so the drift guard cannot see this — the same silent companion-work class as the other accepted transport/Protocol-layer findings on this PR. Status update #3433356219 records only the sender-side scoping of this change ('the server-side cancel path is now narrowly scoped to stream termination on stdio') for the ServerNotificationSchema work; #3440566896 covers the requestId optional-vs-required schema/test divergence and the absent-requestId no-op branch in _oncancel; the broader subscriptions/listen comments (#3239359153, #3223937258, #3433435417) cover transport routing, schemas, and subscriptionId stamping. None names the fact that _oncancel has no path for cancellations targeting the receiver's own outgoing requests — which is the mechanism the new normative sentence depends on.
Impact
Until addressed, the only spec-defined way to end a subscriptions/listen stream on stdio is a no-op against the SDK: the listen request hangs, then fails with a timeout error that misattributes the cause, and the listen-stream manager has no termination signal to clean up against.
How to fix (companion work, not this automated sync)
When implementing subscriptions/listen, extend the cancelled-notification handling so that a cancellation referencing one of the local side's outstanding outgoing requests settles/aborts that request (e.g. resolve or reject the _responseHandlers entry and clean up _progressHandlers/_timeoutInfo), or route it to the listen-stream manager — and gate the behavior to the listen request specifically, per the spec's 'Servers MUST NOT use this notification to cancel any other request' constraint.
2b8cddf to
ee274f4
Compare
ee274f4 to
4df8d55
Compare
|
|
||
| /** | ||
| * The response to a {@link SubscriptionsListenRequest | subscriptions/listen} | ||
| * request, signalling that the subscription has ended gracefully (for example, | ||
| * during server shutdown). Because the listen stream is long-lived, this result | ||
| * is sent only when the server tears the subscription down; an abrupt transport | ||
| * close carries no response. The result body is otherwise empty. | ||
| * | ||
| * @example Subscription closed gracefully | ||
| * {@includeCode ./examples/SubscriptionsListenResult/listen-closed.json} | ||
| * | ||
| * @category `subscriptions/listen` | ||
| */ | ||
| export interface SubscriptionsListenResult extends Result { | ||
| _meta: SubscriptionsListenResultMeta; | ||
| } |
There was a problem hiding this comment.
🟡 Upstream schema.ts underspecification to batch with the other accepted spec feedback: the new SubscriptionsListenResult JSDoc (lines 1304-1318) says, transport-unqualified, that this result "is sent only when the server tears the subscription down; an abrupt transport close carries no response" — but the CancelledNotification JSDoc (line 602, from the previous re-pull) says that on stdio the server terminates a subscriptions/listen stream by sending notifications/cancelled referencing the listen request's ID, and the spec never states how the two compose on stdio (cancellation only, result only, or both and in what order). The companion subscriptions/listen port must not bake either guess into wire behavior: the client side needs to know which signal settles the pending Protocol.request() listen request (cancelled-only requires the #3445564194 _oncancel routing fix; result-only makes the line-602 stdio sentence dead prose), and the server side needs to know what to emit on stdio teardown — raise upstream so the termination contract is stated explicitly before the port encodes it.
Extended reasoning...
What the issue is
This re-pull (2ffc3fa) adds SubscriptionsListenResult (spec.types.2026-07-28.ts:1303-1318), whose JSDoc is unqualified by transport: "The response to a subscriptions/listen request, signalling that the subscription has ended gracefully (for example, during server shutdown). Because the listen stream is long-lived, this result is sent only when the server tears the subscription down; an abrupt transport close carries no response."
But an earlier force-push of this same PR (91b403f) had already added a different server-side termination mechanism for the same stream, which this re-pull leaves untouched: the CancelledNotification JSDoc (line 602) says "On stdio, the server also sends this notification, solely to terminate a subscriptions/listen stream: it references the ID of the subscriptions/listen request that opened the stream. Servers MUST NOT use this notification to cancel any other request." The general CancelledNotification JSDoc also still says "the result will be unused, so any associated processing SHOULD cease" — which pulls directly against a result following the cancellation.
The two passages now both claim to be the way the server ends a listen stream, and the spec never says how they compose on stdio, where there is no per-request HTTP stream to close:
- Cancellation only: the listen request never receives a JSON-RPC response — directly contradicting the new result's "sent only when the server tears the subscription down" (server-initiated stdio teardown is the server tearing the subscription down), and re-creating the pending-request problem the result type was added to solve.
- Cancellation then result: an unusual JSON-RPC contract (the receiver of a request cancels it and then still responds to it) that the spec nowhere states — and that the surviving "the result will be unused" sentence argues against. An implementer reading only the result JSDoc would not emit the cancellation; one reading only the cancelled JSDoc would not emit the result.
- Result only: the stdio sentence at line 602 becomes dead prose left over from the revision that predates the result type.
This is the same partial-migration class already accepted repeatedly on this PR (#3246176905, #3368827092): the result type was added in the latest upstream commit without reconciling the stdio-cancellation sentence introduced by the previous one.
Step-by-step proof
- An SDK client on stdio opens the notification stream by issuing
subscriptions/listen(request ID7); the ID lands inProtocol's_responseHandlers. - The server later shuts down gracefully and wants to tear the subscription down. Per line 602 it sends
{ method: 'notifications/cancelled', params: { requestId: 7 } }; per lines 1304-1309 it sends{ id: 7, result: { resultType: 'complete', _meta: { 'io.modelcontextprotocol/subscriptionId': 7 } } }. - The spec gives no rule for which of these (or both, in which order) the server emits on stdio. All three behaviors are equally "compliant" and mutually incompatible on the wire.
- A client that follows the result JSDoc waits for the response; against a cancellation-only server the listen request hangs in
_responseHandlersuntil a misleadingRequestTimeout(and per #3445564194,Protocol._oncancelonly consults_requestHandlerAbortControllers, so the cancellation is silently dropped). A client that follows the cancelled JSDoc and tears down on the notification may then receive an unsolicited{ id: 7, result: ... }it no longer has a handler for.
Why existing comments don't cover it
- The companion bug filed on this run for
SubscriptionsListenResult/SubscriptionsListenResultMetacovers only the drift-guard/schema/export-count gap and the request-timeout interaction — not the stdio cancellation mechanism or the termination-contract ambiguity. - Prior comment #3445564194 covers only the
Protocol._oncancelrouting gap for the cancelled path and was written beforeSubscriptionsListenResultexisted; it assumes the cancellation is the sole termination signal. - Prior comment #3231781722 (no result wrapper at all) is superseded by the result's addition and does not discuss the cancelled mechanism.
- Status update #3433356219 records the stdio-cancellation scoping as the resolution of the
ServerNotificationunion question; it does not mention the result type (which did not exist yet).
Concrete SDK consequence (why this matters for the companion port)
The companion subscriptions/listen work (already mandated by #3223937253/#3239359153) has to pick a wire behavior on both sides:
- Client side: which signal settles the pending listen request created by
Protocol.request()? TheSubscriptionsListenResultis handled natively byProtocol's response path; a server-sentnotifications/cancellednaming the client's own outgoing request is NOT (the #3445564194 gap). If the spec's answer is "cancelled only on stdio", the #3445564194 routing fix is mandatory and the result schema is never exercised on stdio; if the answer is "result always", the client can ignore server-sent cancellations entirely. Different code either way. - Server side: on stdio teardown, must the SDK server emit the cancellation, the result, or both, and in which order? Whatever it encodes becomes wire-visible behavior that a strict 2026-07-28 client may or may not expect.
How to fix
No change to this automated sync itself. Raise upstream with the other batched schema.ts feedback: state explicitly how server-initiated termination of a subscriptions/listen stream works on stdio — e.g. "the server sends notifications/cancelled and MUST then send the SubscriptionsListenResult to complete the request", or drop the stdio cancellation sentence at line 602 now that a graceful-close response exists, or scope the cancellation to abrupt/non-graceful termination and say the result is omitted in that case. Until settled, the companion port should not bake either guess into the public client/server wire behavior.
9fb2071 to
613b17e
Compare
| * Source: https://github.com/modelcontextprotocol/modelcontextprotocol | ||
| * Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts | ||
| * Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63 | ||
| * Last updated from commit: 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 |
There was a problem hiding this comment.
🔴 This sync bumps the anchor's provenance header to upstream commit 2ffc3fa2, but the derived twins that the repo's OPERATIVE lifecycle rule 4 requires to be refreshed in the same commit still pin the old commit f68d864a: packages/core-internal/test/corpus/schema-twins/manifest.json and test/corpus/fixtures/2026-07-28/manifest.json. Please run pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 (and pnpm fetch:spec-examples if upstream examples changed) in this PR so both manifests record the same commit as the anchor — and longer-term, the nightly cron workflow should regenerate the twins alongside the anchor, since it currently only runs fetch:spec-types and only git adds the anchor file.
Extended reasoning...
What the bug is
This automated sync updates the 2026-07-28 anchor's provenance header to upstream commit 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 (packages/core-internal/src/types/spec.types.2026-07-28.ts:6), but it does not refresh the derived artifacts that the repo's own lifecycle policy ties to that anchor. packages/core-internal/src/types/README.md lifecycle rule 4 — explicitly marked OPERATIVE — states: "any refresh that changes the anchor must regenerate those artifacts in the same commit. The anchor and its derived twins must never be out of sync at any commit on main." It then names the governed artifacts: the vendored schema.json twins under packages/core-internal/test/corpus/schema-twins/ (whose manifest.json records the source commit and content hashes), and the spec example corpus manifest (test/corpus/fixtures/<revision>/manifest.json), which "follows the same atomicity rule when the examples are re-vendored."
The code path / state that triggers it
After this PR merges, the repository on main is in exactly the state rule 4 forbids:
- The anchor header (
spec.types.2026-07-28.ts:6) saysLast updated from commit: 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184. packages/core-internal/test/corpus/schema-twins/manifest.json:5still records\"commit\": \"f68d864a813754e188c6df52dcc5772a12f96c63\"— and its own header comment says refreshes must happen "ATOMICALLY with the matching spec.types anchor (see … lifecycle rule 4)".packages/core-internal/test/corpus/fixtures/2026-07-28/manifest.json:6likewise still recordsf68d864a.- So the anchor and its derived twins disagree about which upstream commit they were generated from, on every commit of
mainfrom the merge onward.
Why existing checks don't prevent it
This divergence lands silently. schemaTwinConformance.test.ts only hashes the vendored twin bytes against the manifest's own sha256/bytes entries and validates fixtures against the twin — it never cross-checks the manifest's source.commit against the anchor header's commit. Because the anchor's TypeScript body is byte-identical in this diff (only the provenance line changed), the content lock stays green and CI gives no signal. The README anticipates a content mismatch being "the desired loud signal of a missed twin update", but a header-only bump produces a provenance-only mismatch that nothing detects.
The root cause is structural: .github/workflows/update-spec-types.yml only runs pnpm run fetch:spec-types 2026-07-28 (line 48) and only git adds the anchor file (line 73). fetch:schema-twins and fetch:spec-examples are separate scripts that the cron job never invokes, so every nightly sync that bumps the anchor will recreate this violation.
Step-by-step proof
- Merge this PR as-is.
- On
main, rungrep 'Last updated from commit' packages/core-internal/src/types/spec.types.2026-07-28.ts→2ffc3fa2…. - Run
grep '\"commit\"' packages/core-internal/test/corpus/schema-twins/manifest.json packages/core-internal/test/corpus/fixtures/2026-07-28/manifest.json→ both sayf68d864a…. - This is the out-of-sync state rule 4 says "must never" exist at any commit on
main, and the schema-twins manifest's "ATOMICALLY with the matching spec.types anchor" instruction has been violated. pnpm vitest run test/wire/schemaTwinConformance.test.tsstays green, so nothing flags it.
Impact
Beyond the policy violation itself, the practical risk is that the vendored test oracles silently go stale: if upstream's generated schema.json or examples changed between f68d864a and 2ffc3fa2 (descriptions, required lists, examples), the conformance suites would be validating against an older oracle while the anchor claims a newer commit, and the manifests would be lying about their provenance. (Mitigating fact: the anchor's TS body is byte-identical here, so the twin content is likely unchanged — but the only way to know, and the thing the policy demands, is to regenerate and re-pin in the same commit.)
How to fix
In this PR: run pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 (and pnpm fetch:spec-examples if upstream examples changed) so the twin and both manifests record the same commit as the anchor — or, if the twins are intentionally being left at f68d864a, document why in the PR. Longer-term, update .github/workflows/update-spec-types.yml to run the twin/example fetch scripts and git add their outputs alongside the anchor, so the nightly sync can satisfy rule 4 on its own; optionally add a check in schemaTwinConformance.test.ts that compares the manifest's source.commit to the anchor header so a future provenance-only divergence fails loudly.
613b17e to
6d8fc68
Compare
| * Source: https://github.com/modelcontextprotocol/modelcontextprotocol | ||
| * Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts | ||
| * Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63 | ||
| * Last updated from commit: 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 |
There was a problem hiding this comment.
🟡 This sync is a verified content-noop — upstream schema/draft/schema.ts and schema.json are byte-identical between f68d864a and 2ffc3fa2, and the vendored twin's sha256 already matches upstream at the new commit — so the only effective change is the provenance line, yet it still re-creates the rule-4 anchor↔manifest skew flagged in #3485357818. Worse, following that comment's prescribed remediation as written misfires: fetch-schema-twins.ts applies one SHA to every twin and would re-fetch/re-stamp the frozen 2025-11-25 released-revision oracle from the floating draft commit (contrary to README rule 1 / RELEASED_REVISION_PINS), and fetch-spec-examples.ts run without a SHA defaults to the latest commit touching schema/draft/examples — a different path filter than the anchor — silently re-creating the provenance disagreement. Suggested fixes: make the workflow's drift check ignore the provenance header (or skip the write when the regenerated body is unchanged), give the twin/example fetch scripts per-revision pinning / anchor-aligned defaults, and for this PR either close it as a no-op or land it together with a 2026-07-28-only twin/fixtures refresh.
Extended reasoning...
This PR is a verified content-noop; only the provenance line moves
Both files the anchor mirrors are byte-identical between the previously recorded commit and the newly recorded one: fetching schema/draft/schema.ts from raw.githubusercontent.com at f68d864a813754e188c6df52dcc5772a12f96c63 and at 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 yields two 96,322-byte files with an empty diff, and schema/draft/schema.json at both commits is 178,613 bytes with sha256 14398c3d…ba5c4c83 — exactly the hash already recorded for the 2026-07-28 twin in packages/core-internal/test/corpus/schema-twins/manifest.json. Since scripts/fetch-spec-types.ts always regenerates the whole anchor from the fetched bytes, a byte-identical upstream file can only ever produce a header-only diff, which is exactly what this PR contains. The vendored conformance oracle is therefore not stale in content; the only divergence this PR creates is provenance metadata — yet merging it alone still leaves main in the rule-4-violating state already accepted as #3485357818 (anchor says 2ffc3fa2, both manifests still say f68d864a), for zero spec content change.
Why this churn will recur: the workflow's drift check counts header-only rewrites as drift
.github/workflows/update-spec-types.yml's Check for changes step runs git diff --quiet packages/core-internal/src/types/spec.types.2026-07-28.ts on the whole file, and fetchLatestSHA() in scripts/fetch-spec-types.ts (lines 51-65) picks the newest commit that touched schema/draft/schema.ts regardless of whether the content changed. Step-by-step: (1) upstream lands a content-neutral touch of the path (e.g. a change later reverted, with the revert being the newest path-touching commit); (2) the nightly job regenerates the anchor — the body is unchanged but the header SHA moves; (3) git diff --quiet sees the header line and reports drift; (4) a refresh PR like this one is opened, and per rule 4 it now demands a twin/fixtures refresh (or stands as a violation) despite mirroring identical bytes. This will repeat on every future content-neutral upstream touch.
The prescribed remediation in #3485357818 also misfires as written
That comment instructs running pnpm fetch:schema-twins 2ffc3fa2… (and pnpm fetch:spec-examples with no SHA). Two problems:
-
scripts/fetch-schema-twins.tshas no per-revision pinning. Line 53 takes a single SHA (process.argv[2] ?? manifest.source.commit), the loop at lines 55-63 iterates every entry inmanifest.files— both2026-07-28→schema/draft/schema.jsonand2025-11-25→schema/2025-11-25/schema.json— re-fetching each at that one SHA and rewriting bytes/sha256, and line 65 overwrites the single sharedmanifest.source.commit. Running the prescribed command therefore re-vendors and re-stamps the frozen 2025-11-25 released-revision oracle from the floating draft commit — exactly whatpackages/core-internal/src/types/README.mdrule 1 andRELEASED_REVISION_PINSinfetch-spec-types.ts(which pins 2025-11-25 at0168c57f…) forbid. The singlesource.commitfield structurally cannot represent the two lifecycles: today it already disagrees with both the 2025-11-25 anchor header (357adac4) and the released pin (0168c57f). Wiring the script into the nightly cron, as the comment also suggests, would institutionalize this every night. -
scripts/fetch-spec-examples.tsrun without a SHA stamps the wrong commit. Line 142 isconst sha = args[0] ?? (await fetchLatestSHA()), and that fallback queries the GitHub commits API filtered byschema/draft/examples— a different path filter than the anchor'sschema/draft/schema.ts. The resulting commit is in general not2ffc3fa2(it can be newer or older), andwriteCorpus()stamps it intotest/corpus/fixtures/2026-07-28/manifest.jsonsource.commit— silently re-creating the very anchor↔derived-artifact provenance disagreement rule 4 forbids, just between the anchor and the example corpus instead of the schema twins. No CI catches it:specCorpus.test.tsonly checks the revision string, directory listing, file count, and the 87/128 inventory pins, neversource.commitvs the anchor header;schemaTwinConformance.test.tshashes only the twin files.
How to fix
- Workflow / fetch script: make the drift check ignore the provenance header (e.g.
git diff -I 'Last updated from commit:' --quiet …, git ≥ 2.30), or havefetch-spec-types.tscompare the regenerated body (excluding the header) with the on-disk body and skip the write — keeping the previously recorded SHA — when they are identical, so the recorded provenance only moves when mirrored content actually changes. fetch-schema-twins.ts: accept a revision argument and/or mirrorRELEASED_REVISION_PINSso the 2025-11-25 entry is only ever fetched at its pinned commit, and record provenance per file (acommitfield inside eachmanifest.filesentry) instead of one sharedsource.commit.fetch-spec-examples.ts: pass the anchor commit explicitly (pnpm fetch:spec-examples 2ffc3fa2…) in the same commit that refreshes the twins; longer term, default its SHA to the anchor header'sLast updated from commitrather than the latest examples-path commit, and have the nightly workflow drive all three fetch scripts with one shared SHA. Optionally extendspecCorpus.test.tsto comparemanifest.source.commitagainst the anchor header so a future provenance-only skew fails loudly.- For this PR specifically: since the spec content is unchanged and the vendored twin hash already matches upstream at
2ffc3fa2, the lowest-cost resolution is to close it as a no-op, or land it together with the 2026-07-28-only twin/fixtures manifest refresh demanded by #3485357818 — merging the header bump alone buys nothing and leavesmainin the rule-4-violating state.
This PR updates
packages/core-internal/src/types/spec.types.2026-07-28.tsfrom the Model Context Protocol specification.Source file: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184/schema/draft/schema.ts
This is an automated update triggered by the nightly cron job.