Skip to content

chore: update spec.types.ts from upstream#2027

Open
github-actions[bot] wants to merge 1 commit into
mainfrom
update-spec-types
Open

chore: update spec.types.ts from upstream#2027
github-actions[bot] wants to merge 1 commit into
mainfrom
update-spec-types

Conversation

@github-actions

@github-actions github-actions Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

This PR updates packages/core-internal/src/types/spec.types.2026-07-28.ts from 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.

@github-actions github-actions Bot requested a review from a team as a code owner May 7, 2026 05:08
@changeset-bot

changeset-bot Bot commented May 7, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6d8fc68

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions Bot force-pushed the update-spec-types branch from fe613a8 to 5f450ff Compare May 8, 2026 05:01
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch 2 times, most recently from a64a23e to af35bb5 Compare May 10, 2026 05:12
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment on lines +406 to +418
/* 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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  1. Hover InputResponseRequestParams at line 409 in VS Code → tooltip shows only interface InputResponseRequestParams extends RequestParams with no description, because lines 406-408 start with /* not /**.
  2. Hover InputRequiredResult at line 393 → tooltip shows the full "An InputRequiredResult sent by the server…" text, because lines 380-392 start with /**.
  3. Hover requestState at line 403 → no description; the "client must treat this as an opaque blob" note (398-402) is dropped.
  4. Run TypeDoc over upstream schema.tsInputRequests, InputResponses, InputRequiredResult are grouped under Multi Round-Trip; InputResponseRequestParams is not (no @category tag, 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.

Comment on lines +393 to +404
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  1. Result.resultType: 'complete' | 'input_required' (line 165).
  2. InputRequiredResult extends Result { inputRequests?: ...; requestState?: ... } (line 393) — inherits resultType: 'complete' | 'input_required' unchanged.
  3. CallToolResult extends Result { content: ...; ... } — also inherits resultType: 'complete' | 'input_required' unchanged.
  4. Given declare const r: CallToolResult | InputRequiredResult:
    if (r.resultType === 'input_required') {
      r.inputRequests; // ❌ TS error: Property 'inputRequests' does not exist on type 'CallToolResult | InputRequiredResult'
    }
    TypeScript cannot eliminate CallToolResult from the union because CallToolResult['resultType'] also includes 'input_required'. Narrowing requires the discriminant property to have disjoint literal types across union members.
  5. Additionally, since Result carries [key: string]: unknown and InputRequiredResult's only additions (inputRequests?, requestState?) are optional, InputRequiredResult is structurally a subtype of CallToolResult — 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.

@github-actions github-actions Bot force-pushed the update-spec-types branch from af35bb5 to 879139a Compare May 11, 2026 05:19
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
Comment on lines +348 to +351
export type InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest;

/** @internal */
export const URL_ELICITATION_REQUIRED = -32042;
export type InputResponse = CreateMessageResult | ListRootsResult | ElicitResult;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  • ElicitResult happens to carry action: 'accept' | 'decline' | 'cancel', so elicitation can encode refusal in-band.
  • CreateMessageResult (line ~2335) is SamplingMessage & { model: string; stopReason?: ... } — required model/role/content, no refusal/error field, no index signature now that extends Result is 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

  1. Server returns CallToolResultResponse with result: InputRequiredResult { inputRequests: { 's1': <CreateMessageRequest>, 'e1': <ElicitRequest> } }.
  2. Client presents the elicitation; user accepts → { action: 'accept', content: {...} }.
  3. Client attempts the sampling call; the LLM provider returns HTTP 429 / content-policy refusal / the user denies the sampling-consent prompt.
  4. Client must now construct inputResponses for the retry. Per lines 410-413, both 's1' and 'e1' must appear. 'e1' is fine. For 's1' the only spec-permitted shapes are CreateMessageResult | ListRootsResult | ElicitResult — none of which can express "this request failed with code X / message Y".
  5. The client's only options are: (a) omit 's1' — textually non-compliant with the must-appear rule; (b) abandon the whole retry / call tasks/cancel — loses the successful elicitation and gives the server no error code/message to act on; (c) stuff a fake CreateMessageResult or 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.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 879139a to 81ef4d8 Compare May 12, 2026 05:13
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 81ef4d8 to 29aa626 Compare May 13, 2026 05:17
Comment thread packages/core/src/types/spec.types.ts
Comment on lines 144 to 146
export interface RequestParams {
_meta?: RequestMetaObject;
_meta: RequestMetaObject;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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 PaginatedRequestparams?: 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

  1. RequestMetaObject (lines 74-97) declares 'io.modelcontextprotocol/protocolVersion': string, '…/clientInfo': Implementation, '…/clientCapabilities': ClientCapabilities — none with ?.
  2. RequestParams (line 145) declares _meta: RequestMetaObject — no ?.
  3. PaginatedRequest (line 1014) declares params?: PaginatedRequestParamswith ?. PaginatedRequestParams extends RequestParams (line 1004).
  4. ListToolsRequest extends PaginatedRequest → a client may send { jsonrpc:'2.0', id:1, method:'tools/list' } with no params → no _meta → no protocolVersion/clientInfo/clientCapabilities. Type-valid, but the JSDoc on each key says "Required."
  5. ListTasksRequest extends PaginatedRequest and is a member of ServerRequest (line 3280). A server paginating tasks/list constructs params: { cursor: '…', _meta: { 'io.modelcontextprotocol/clientInfo': ???, 'io.modelcontextprotocol/clientCapabilities': ???, … } } — there is no value a server can sensibly put for the client's identity.
  6. ListRootsRequest (line 2822) keeps params?: RequestParams, while CreateMessageRequest.params: CreateMessageRequestParams and ElicitRequest.params: ElicitRequestParams no longer extend RequestParamsListRootsRequest is the odd one out among the three InputRequest members.

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 on RequestMetaObject (or split into ClientRequestMetaObject / ServerRequestMetaObject), and have client→server wire request types make params non-optional so the handshake metadata is actually carried.
  • Have DiscoverRequest use a params type that does not extend RequestParams (or document an explicit exemption from the protocolVersion rejection rule for server/discover).
  • Have server→client request params (concretely ListTasksRequest via PaginatedRequest) not extend RequestParams, or use a server-direction _meta shape.
  • Change ListRootsRequest.params? to a bare { _meta?: MetaObject } or drop params entirely, matching CreateMessageRequest/ElicitRequest.

Then re-run pnpm run fetch:spec-types.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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._meta is still required (line 128) with the three required io.modelcontextprotocol/* keys, while DiscoverRequest.params? (line 549) and PaginatedRequest.params? (line 932) are still optional — a client can still type-validly omit the "Required" handshake metadata, and the server/discover circularity (must commit to a protocolVersion to ask which versions are supported) is unchanged.
  • ListRootsRequest (line 2452) still keeps params?: RequestParams while its InputRequest siblings CreateMessageRequest/ElicitRequest have had their RequestParams inheritance 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 29aa626 to 07e2103 Compare May 14, 2026 05:17
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
* 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`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 🟡 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.subscriberesourceSubscriptions, {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

  1. Diff deletes SubscribeRequest / UnsubscribeRequest / SubscribeResultResponse / UnsubscribeResultResponse; ClientRequest (line 3256) no longer contains either.
  2. Diff adds SubscriptionFilter (line 1176) with toolsListChanged? / promptsListChanged? / resourcesListChanged? / resourceSubscriptions?: string[] and JSDoc "the server MUST NOT send notification types the client has not explicitly requested here." SubscriptionFilter.resourceSubscriptions JSDoc (line 1194) explicitly says "Replaces the former resources/subscribe RPC."
  3. ServerCapabilities body (lines 723-833) is byte-identical to pre-sync apart from the @category tag. resources.subscribe (776), prompts.listChanged (755), resources.listChanged (780), tools.listChanged (795) all survive with pre-sync JSDoc. Grep for subscriptions under ServerCapabilities → no match.
  4. Pre-sync semantics of prompts.listChanged: true: server will send notifications/prompts/list_changed whenever its prompt set changes (no client opt-in required). Post-sync semantics per step 2: server MUST NOT send it unless the client included promptsListChanged: true in a subscriptions/listen filter. 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.
  5. SDK surface: packages/client/src/client/client.ts:599 does if (method === 'resources/subscribe' && !this._serverCapabilities.resources.subscribe) throw …. After the redesign already requested in comment #3223937258 that branch is removed, and the new subscriptions/listen client API will need a capability gate — under the current spec it must either reuse resources.subscribe/*.listChanged under 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 honor resourceSubscriptions on a subscriptions/listen stream"; *.listChanged → "Whether this server will honor *ListChanged on a subscriptions/listen stream"), and refresh the @example JSON files. Optionally add a top-level subscriptions?: JSONObject to 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 mirrors SubscriptionFilter.

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.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 07e2103 to 0bcec2b Compare May 15, 2026 05:18
Comment on lines +566 to 569
export interface DiscoverRequest extends JSONRPCRequest {
method: 'server/discover';
params?: RequestParams;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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 for method: 'initialize'.
  • Lines 667-685 (the if (isInitializationRequest) block): this.sessionId = this.sessionIdGenerator?.() (678), this._initialized = true (679), and await this._onsessioninitialized(this.sessionId) (683-684). This is the only place sessionId is minted and _initialized is flipped.
  • Lines 687-694 (the else path): every non-initialize POST goes through validateSession(req).
  • validateSession() (847-856): in stateful mode (sessionIdGenerator set), 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 : … — reads params.protocolVersion, a field of the deleted InitializeRequestParams. In the new model the protocol version lives in params._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

  1. User constructs the documented stateful server: new StreamableHTTPServerTransport({ sessionIdGenerator: () => crypto.randomUUID(), onsessioninitialized: id => sessions.set(id, …) }) (per the JSDoc example at streamableHttp.ts:195).
  2. A spec-compliant client built against the post-sync schema connects. Per this diff there is no initialize method; 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': … } } }.
  3. handlePostRequest parses the body. isInitializeRequest({ method: 'server/discover', … })false (it checks for 'initialize'). isInitializationRequest = false.
  4. Control falls to line 687-691 → validateSession(req). sessionIdGenerator is set, this._initialized is still false (never flipped) → returns 400 "Bad Request: Server not initialized".
  5. The client retries; same result. No request ever succeeds, sessionId is never minted, onsessioninitialized never fires, the user's session map stays empty.
  6. Separately, even if a legacy client sent initialize, line 722 reads initRequest.params.protocolVersion — fine for legacy clients, but once the redesign re-keys the bootstrap on server/discover (whose params?: RequestParams has no protocolVersion field), this read returns undefined and 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/onsessionclosed deprecated → breaking change, migration.md entry).
  • (b) Mint on first request regardless of method (re-key the bootstrap on "first POST without Mcp-Session-Id header" instead of "is initialize").
  • (c) Re-key on server/discover (closest 1:1 swap, but server/discover is 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() sending initialize, server.ts registering an initialize handler / assertInitialized() gating, and the schemas/guards cleanup. Its remediation list never mentions the transport's isInitializeRequest-gated session bootstrap, sessionIdGenerator, _initialized, onsessioninitialized, validateSession(), or the Mcp-Session-Id lifecycle.
  • #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 the params.protocolVersion read 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-Id lifecycle and update streamableHttp.ts:664-700 + validateSession() accordingly.
  • Replace the initRequest.params.protocolVersion read (720-722) with params._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 update packages/middleware/{node,hono,fastify,express}/README.md + streamableHttp.examples.ts which use it for per-session transport routing.
  • If sessions are removed, deprecate sessionIdGenerator/onsessioninitialized/onsessionclosed (breaking → changeset + migration.md).

Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment thread packages/core/src/types/spec.types.ts Outdated
@github-actions github-actions Bot force-pushed the update-spec-types branch from 0bcec2b to 57ccae0 Compare May 16, 2026 05:05
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch 2 times, most recently from e9de0a8 to 786d94c Compare May 18, 2026 05:23
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 786d94c to edb9e09 Compare May 19, 2026 05:20
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from edb9e09 to 1896892 Compare May 20, 2026 05:21
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 1896892 to b255ba4 Compare May 21, 2026 05:24
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts Outdated
@github-actions github-actions Bot force-pushed the update-spec-types branch from b255ba4 to da9d82e Compare May 22, 2026 05:20
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from ef5e88c to bab2413 Compare June 8, 2026 05:34
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.2026-07-28.ts Outdated
@github-actions github-actions Bot force-pushed the update-spec-types branch 7 times, most recently from 86d0536 to fa89096 Compare June 15, 2026 05:46
@github-actions github-actions Bot force-pushed the update-spec-types branch 3 times, most recently from 0435878 to 4c6b66f Compare June 18, 2026 05:35
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 20baa74 to 234b0fc Compare June 20, 2026 05:29
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment on lines 597 to 603
}

/**
* 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.
*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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)

  1. 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.
  2. The server later shuts the stream down using the only spec-defined mechanism on stdio: it sends notifications/cancelled with the listen request's ID.
  3. The SDK client's default notifications/cancelled handler (registered in the Protocol constructor, protocol.ts:323-325) routes to _oncancel.
  4. _oncancel looks up _requestHandlerAbortControllers.get(requestId) — the listen request is an outgoing request, so there is no entry — and returns.
  5. The client never learns the stream was terminated: the listen request stays pending in _responseHandlers until its timeout fires with a misleading RequestTimeout, the subscription bookkeeping is never torn down, and there is no hook a subscriptions/listen implementation 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.

Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch 3 times, most recently from 2b8cddf to ee274f4 Compare June 23, 2026 05:20
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from ee274f4 to 4df8d55 Compare June 24, 2026 05:19
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment on lines +1303 to +1318

/**
* 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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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

  1. An SDK client on stdio opens the notification stream by issuing subscriptions/listen (request ID 7); the ID lands in Protocol's _responseHandlers.
  2. 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 } } }.
  3. 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.
  4. A client that follows the result JSDoc waits for the response; against a cancellation-only server the listen request hangs in _responseHandlers until a misleading RequestTimeout (and per #3445564194, Protocol._oncancel only 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/SubscriptionsListenResultMeta covers 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._oncancel routing gap for the cancelled path and was written before SubscriptionsListenResult existed; 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 ServerNotification union 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()? The SubscriptionsListenResult is handled natively by Protocol's response path; a server-sent notifications/cancelled naming 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.

@github-actions github-actions Bot force-pushed the update-spec-types branch 2 times, most recently from 9fb2071 to 613b17e Compare June 27, 2026 05:17
* 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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:

  1. The anchor header (spec.types.2026-07-28.ts:6) says Last updated from commit: 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184.
  2. packages/core-internal/test/corpus/schema-twins/manifest.json:5 still records \"commit\": \"f68d864a813754e188c6df52dcc5772a12f96c63\" — and its own header comment says refreshes must happen "ATOMICALLY with the matching spec.types anchor (see … lifecycle rule 4)".
  3. packages/core-internal/test/corpus/fixtures/2026-07-28/manifest.json:6 likewise still records f68d864a.
  4. So the anchor and its derived twins disagree about which upstream commit they were generated from, on every commit of main from 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

  1. Merge this PR as-is.
  2. On main, run grep 'Last updated from commit' packages/core-internal/src/types/spec.types.2026-07-28.ts2ffc3fa2….
  3. Run grep '\"commit\"' packages/core-internal/test/corpus/schema-twins/manifest.json packages/core-internal/test/corpus/fixtures/2026-07-28/manifest.json → both say f68d864a….
  4. 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.
  5. pnpm vitest run test/wire/schemaTwinConformance.test.ts stays 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.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 613b17e to 6d8fc68 Compare June 28, 2026 05:30
* 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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:

  1. scripts/fetch-schema-twins.ts has 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 in manifest.files — both 2026-07-28schema/draft/schema.json and 2025-11-25schema/2025-11-25/schema.json — re-fetching each at that one SHA and rewriting bytes/sha256, and line 65 overwrites the single shared manifest.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 what packages/core-internal/src/types/README.md rule 1 and RELEASED_REVISION_PINS in fetch-spec-types.ts (which pins 2025-11-25 at 0168c57f…) forbid. The single source.commit field 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.

  2. scripts/fetch-spec-examples.ts run without a SHA stamps the wrong commit. Line 142 is const sha = args[0] ?? (await fetchLatestSHA()), and that fallback queries the GitHub commits API filtered by schema/draft/examples — a different path filter than the anchor's schema/draft/schema.ts. The resulting commit is in general not 2ffc3fa2 (it can be newer or older), and writeCorpus() stamps it into test/corpus/fixtures/2026-07-28/manifest.json source.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.ts only checks the revision string, directory listing, file count, and the 87/128 inventory pins, never source.commit vs the anchor header; schemaTwinConformance.test.ts hashes 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 have fetch-spec-types.ts compare 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 mirror RELEASED_REVISION_PINS so the 2025-11-25 entry is only ever fetched at its pinned commit, and record provenance per file (a commit field inside each manifest.files entry) instead of one shared source.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's Last updated from commit rather than the latest examples-path commit, and have the nightly workflow drive all three fetch scripts with one shared SHA. Optionally extend specCorpus.test.ts to compare manifest.source.commit against 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 leaves main in the rule-4-violating state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants