chore: update spec.types.ts from upstream#2027
Conversation
|
fe613a8 to
5f450ff
Compare
| | TaskStatusNotification; | ||
|
|
||
| /** @internal */ | ||
| export type ClientResult = | ||
| | EmptyResult | ||
| | CreateMessageResult | ||
| | ListRootsResult | ||
| | ElicitResult | ||
| | GetTaskResult | ||
| | GetTaskPayloadResult | ||
| | ListTasksResult | ||
| | CancelTaskResult; | ||
| export type ClientResult = EmptyResult | GetTaskResult | GetTaskPayloadResult | ListTasksResult | CancelTaskResult; | ||
|
|
||
| /* Server messages */ | ||
| /** @internal */ | ||
| export type ServerRequest = | ||
| | PingRequest | ||
| | CreateMessageRequest | ||
| | ListRootsRequest | ||
| | ElicitRequest | ||
| | GetTaskRequest | ||
| | GetTaskPayloadRequest | ||
| | ListTasksRequest | ||
| | CancelTaskRequest; | ||
| export type ServerRequest = PingRequest | GetTaskRequest | GetTaskPayloadRequest | ListTasksRequest | CancelTaskRequest; |
There was a problem hiding this comment.
🔴 The spec has restructured sampling/elicitation/roots out of the JSON-RPC request/response model: CreateMessageRequest/ElicitRequest/ListRootsRequest no longer extend JSONRPCRequest, their results no longer extend Result, the *ResultResponse wrappers are deleted, and all six are dropped from ServerRequest/ClientResult — they are now payloads embedded inside the new InputRequiredResult flow. The SDK still models these as standalone server→client wire requests (ServerRequestSchema/ClientResultSchema in schemas.ts:2107-2123, plus the server.createMessage()/elicitInput()/listRoots() APIs), so beyond the immediate spec.types.test.ts compile failures (lines 156/160/168/172/225/301/476/499 and hard references to the deleted SpecTypes.*ResultResponse types at 745-763 & 1019-1024), this sync leaves the SDK architecturally divergent from the protocol. Unlike the new-type additions, this isn't a mechanical "add schemas" fix — it requires redesigning the SDK's server-initiated-request APIs around InputRequiredResult/inputResponses, so this PR should be held until that companion work is ready.
Extended reasoning...
What changed
The upstream spec has fundamentally changed how a server obtains sampling, elicitation, and roots from the client. Previously these were ordinary server→client JSON-RPC requests: CreateMessageRequest, ElicitRequest, and ListRootsRequest each extended JSONRPCRequest, their results extended Result, each had a *ResultResponse envelope, and they were members of the ServerRequest / ClientResult unions. In this diff:
CreateMessageRequest/ElicitRequest/ListRootsRequestdropextends JSONRPCRequest(and their*ParamsdropTaskAugmentedRequestParams/RequestParams).CreateMessageResult/ElicitResult/ListRootsResultdropextends Result.CreateMessageResultResponse,ElicitResultResponse,ListRootsResultResponseare deleted.- All six are removed from the
ServerRequestandClientResultunions (lines 3284-3291). - They are now referenced only via the new
InputRequest/InputResponsealiases, which are carried insideInputRequiredResult.inputRequestsand echoed back viaInputResponseRequestParams.inputResponses(ortasks/input_response).
In other words, the spec no longer models "server asks client for sampling" as a standalone JSON-RPC request — it is now an embedded sub-request inside a resultType: 'input_required' result that the client must fulfill and replay on the next call.
Immediate breakage in this PR
packages/core/test/spec.types.test.ts fails to compile in two distinct ways:
- Bidirectional assignability checks at lines 155-172, 223-225, 301, 476, 499 fail because the spec types no longer carry
jsonrpc/id(requests) or theResultindex signature (results), and theServerRequest/ClientResultunions no longer line up with the SDK's. - Hard references to deleted types: lines 745-763 and 1019-1024 reference
SpecTypes.CreateMessageResultResponse,SpecTypes.ListRootsResultResponse, andSpecTypes.ElicitResultResponse, which no longer exist — these are unconditionalTS2694/TS2339-class errors, not just assignability mismatches.
So the package will not build / CI will be red on merge.
Architectural divergence (why this isn't just a test fix)
The SDK runtime does not import spec.types.ts (only the test does), so src/ still typechecks — but that is precisely the problem the drift guard exists to surface. schemas.ts:2107 (ClientResultSchema) and schemas.ts:2119 (ServerRequestSchema) still include CreateMessageRequestSchema / ElicitRequestSchema / ListRootsRequestSchema and their results, and the public SDK surface (server.createMessage(), ctx.elicitInput(), roots listing) still issues these as wire-level JSON-RPC requests. After this sync, an SDK server speaking to a spec-compliant client would send a sampling/createMessage JSON-RPC request that the client is no longer obligated to handle as a top-level request; conversely, the SDK has no machinery to emit InputRequiredResult from a tool/prompt/resource handler or to accept inputResponses on the retry.
Step-by-step proof
- Before:
spec.types.tshadinterface CreateMessageRequest extends JSONRPCRequest { method: 'sampling/createMessage'; ... }andServerRequest = PingRequest | CreateMessageRequest | ListRootsRequest | ElicitRequest | .... - After (this diff, ~line 2311):
interface CreateMessageRequest { method: 'sampling/createMessage'; params: ... }— nojsonrpc, noid.ServerRequest(line 3291) is nowPingRequest | GetTaskRequest | GetTaskPayloadRequest | ListTasksRequest | CancelTaskRequestonly. spec.types.test.ts:476does(sdk: WithJSONRPCRequest<SDKTypes.CreateMessageRequest>, spec: SpecTypes.CreateMessageRequest) => { sdk = spec; }.spechas nojsonrpc/id, sosdk = specfails: Type 'CreateMessageRequest' is missing the following properties: jsonrpc, id.spec.types.test.ts:756doesspec: SpecTypes.ListRootsResultResponse— that export no longer exists → Namespace has no exported member 'ListRootsResultResponse'.- Meanwhile
schemas.ts:2119-2123still buildsServerRequestSchemafromCreateMessageRequestSchema | ElicitRequestSchema | ListRootsRequestSchema | ..., soSDKTypes.ServerRequest⊋SpecTypes.ServerRequestand the union check at ~499 fails too.
Why this is distinct from the "new types need schemas" comment
A separate comment notes that ResultType, InputRequiredResult, InputResponseRequestParams, TaskInputResponseRequest, etc. need new Zod schemas and specTypeSchema allowlist entries. That is additive, mechanical work. This finding is about the removal/restructuring of existing types and unions, whose remediation is not mechanical: you cannot simply add a schema — you have to (a) decide whether the SDK keeps server.createMessage()/elicitInput()/listRoots() as a compatibility shim or replaces them with an InputRequiredResult-returning API, (b) remove these from ServerRequestSchema/ClientResultSchema, and (c) rewrite the bidirectional test entries (drop the WithJSONRPCRequest<...> wrapper for these three, delete the *ResultResponse checks). The refutation that this is "the same finding" conflates detection mechanism (both trip spec.types.test.ts) with root cause and fix; reviewers need to see this called out separately because it is the part that requires API design, not a 10-line schema patch.
Recommended action
Hold this automated sync until a companion PR implements the InputRequiredResult flow in the SDK (new result/param schemas, updated ServerRequestSchema/ClientResultSchema, reworked createMessage/elicitInput/listRoots plumbing, and updated spec.types.test.ts entries for these six types plus removal of the three deleted *ResultResponse checks). Merging this alone breaks the build and publishes a spec snapshot the SDK cannot honor.
| 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; | ||
| } | ||
|
|
||
| /* 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.
🔴 This spec sync adds 10 new Multi Round-Trip types (ResultType, InputRequest/InputResponse/InputRequests/InputResponses, InputRequiredResult, InputResponseRequestParams, TaskInputResponseRequest/TaskInputResponseRequestParams/TaskInputResponseResultResponse) and adds tasks/input_response to ClientRequest, but none of these have corresponding entries in schemas.ts, types.ts, or the sdkTypeChecks map — so spec.types.test.ts fails on the hardcoded type count (176 vs 182), the "comprehensive compatibility tests" coverage check, and the ClientRequest bidirectional assignability check. This automated cron PR needs companion SDK schema/type/test updates before it can land.
Extended reasoning...
What the bug is
This PR is the nightly automated sync of spec.types.ts from upstream. The upstream commit it pulls in introduces the new Multi Round-Trip machinery: ResultType, InputRequest, InputResponse, InputRequests, InputResponses, InputRequiredResult, InputResponseRequestParams, TaskInputResponseRequest, TaskInputResponseRequestParams, and TaskInputResponseResultResponse, plus a new tasks/input_response member of the ClientRequest union. None of these names appear anywhere else in packages/core/src — schemas.ts has no Zod schemas for them, types.ts has no SDK-side type aliases, and test/spec.types.test.ts has no sdkTypeChecks entries for them. The repo's spec↔SDK drift guard catches exactly this situation and fails the build.
Code path that triggers it
packages/core/test/spec.types.test.ts enforces three invariants that all break:
- Type count — line ~1089 hardcodes
expect(specTypes).toHaveLength(176). After this sync the file exports 182 types (10 added, 4 removed:URLElicitationRequiredError,CreateMessageResultResponse,ListRootsResultResponse,ElicitResultResponse), so this assertion fails. - Coverage check — the
should have comprehensive compatibility teststest (lines ~1098-1108) parses every exported name fromspec.types.ts, skips theMISSING_SDK_TYPESallowlist (currently only['Error', 'URLElicitationRequiredError']), and asserts each remaining name has an entry insdkTypeChecks. The 10 new types have no entry, so 10 assertions fail. Additionally,URLElicitationRequiredErrorwas removed from the spec but is still inMISSING_SDK_TYPES, which will fail the "Missing SDK Types" suite that asserts every allowlisted name actually exists in the spec file. ClientRequestbidirectional check —tscshows the assignability check at ~line 496 failing because the specClientRequestunion now containsTaskInputResponseRequestwhile the SDKClientRequestunion does not.
Why existing code doesn't prevent it
That's by design — spec.types.test.ts is the guard that turns an automated spec pull into a forcing function for SDK updates. The cron job only updates spec.types.ts; it does not (and cannot) generate the matching Zod schemas, SDK type aliases, or sdkTypeChecks entries. Note: the specTypeSchemas allowlist drift guard from #1993 (specTypeSchema.test.ts:162) does not trip here — that guard compares schemas.ts exports against the SPEC_SCHEMA_KEYS allowlist, not against spec.types.ts. The guard that trips is the runtime coverage check in spec.types.test.ts.
Step-by-step proof
Running cd packages/core && pnpm vitest run test/spec.types.test.ts on this branch:
should define some expected types→ FAIL:expected 182 to be 176should have comprehensive compatibility tests→ FAIL:ResultType,InputRequest,InputResponse,InputRequests,InputResponses,InputRequiredResult,InputResponseRequestParams,TaskInputResponseRequest,TaskInputResponseRequestParams,TaskInputResponseResultResponseeach produceexpected undefined to be defined
And tsc --noEmit on packages/core fails because spec.types.test.ts still references the now-removed CreateMessageResultResponse / ListRootsResultResponse / ElicitResultResponse / URLElicitationRequiredError (lines ~745-760, ~1019-1024, ~1069), and because spec.ClientRequest is no longer assignable to sdk.ClientRequest once TaskInputResponseRequest is in the union.
Impact
CI is red on this PR. Merging it as-is would break main. Beyond the test failures, downstream consumers using isSpecType / specTypeSchemas (#1887) would have no runtime validators for any of the new Multi Round-Trip messages, and the SDK ClientRequest union would be out of sync with the wire protocol.
How to fix
This needs a companion commit on top of the cron sync:
- Add Zod schemas for
ResultType,InputRequests,InputResponses,InputRequiredResult,InputResponseRequestParams,TaskInputResponseRequest,TaskInputResponseRequestParams,TaskInputResponseResultResponse(andInputRequest/InputResponsealiases) inschemas.ts, and register them inspecTypeSchemas. - Add the corresponding SDK types in
types.tsand addTaskInputResponseRequestto the SDKClientRequestunion. - In
spec.types.test.ts: bump thetoHaveLength(176)count, addsdkTypeChecksentries for all 10 new types, remove the entries/references forCreateMessageResultResponse/ListRootsResultResponse/ElicitResultResponse/URLElicitationRequiredError, and dropURLElicitationRequiredErrorfromMISSING_SDK_TYPES.
(This is separate from the resultType required-field assignability breakage tracked elsewhere — even if resultType is made optional upstream, every failure listed here still occurs.)
| export interface Result { | ||
| _meta?: MetaObject; | ||
| /** | ||
| * Indicates the type of the result, which allows the client to determine | ||
| * how to parse the result object. | ||
| * | ||
| * @default "complete" | ||
| */ | ||
| resultType: ResultType; | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
🔴 The new resultType: ResultType field on the base Result interface is declared as required even though the JSDoc says @default "complete" — this makes every Result subtype (InitializeResult, CallToolResult, ReadResourceResult, GetPromptResult, EmptyResult, all the Paginated/Task results, etc.) require resultType. The SDK's ResultSchema in schemas.ts does not produce this field, so tsc --noEmit on @modelcontextprotocol/core now fails with 30+ TS2741 errors in spec.types.test.ts and CI will not pass. Given the @default annotation, the upstream spec almost certainly intended resultType?: ResultType; either the upstream schema.ts needs to be fixed before this is re-pulled, or this PR needs an accompanying SDK-side ResultSchema change before it can land.
Extended reasoning...
What the bug is
This automated spec sync adds a new field to the base Result interface in packages/core/src/types/spec.types.ts:
export interface Result {
_meta?: MetaObject;
/**
* Indicates the type of the result, which allows the client to determine
* how to parse the result object.
*
* @default "complete"
*/
resultType: ResultType;
[key: string]: unknown;
}The field is declared without a ?, making it required on Result and on every interface that extends Result. The @default "complete" JSDoc strongly implies the spec author intended it to be optional and defaulted server-side, but the TypeScript declaration does not reflect that.
How it manifests / proof
The SDK maintains bidirectional-assignability checks between the spec types and the SDK's Zod-inferred types in packages/core/test/spec.types.test.ts (e.g. Result: (sdk, spec) => { sdk = spec; spec = sdk; }). The SDK's ResultSchema is defined in packages/core/src/types/schemas.ts:111 as:
export const ResultSchema = z.looseObject({
_meta: ...
});It has no resultType field, and a grep for resultType under packages/core/src finds it only in spec.types.ts — no SDK schema, handler, or result-construction site sets it.
Running pnpm tsc --noEmit -p packages/core/tsconfig.json on this branch produces 30+ errors of the form:
test/spec.types.test.ts: error TS2741: Property 'resultType' is missing in type
'{ _meta?: ...; }' but required in type 'Result'.
repeated for Result, EmptyResult, PaginatedResult, CompleteResult, ListToolsResult, CallToolResult, ListResourcesResult, ListResourceTemplatesResult, ReadResourceResult, ListPromptsResult, GetPromptResult, InitializeResult, CreateTaskResult, GetTaskResult, ListTasksResult, CancelTaskResult, GetTaskPayloadResult, JSONRPCResultResponse, JSONRPCMessage, ServerResult, ClientResult, and more.
Step-by-step
spec.types.tsnow saysResultrequiresresultType: 'complete' | 'input_required'.schemas.tsinfers the SDKResulttype fromResultSchema = z.looseObject({ _meta: ... }), which has noresultTypekey.spec.types.test.tsdoesspec = sdk;forResultand every derived result type.- TypeScript rejects the assignment because
{ _meta?: ... }lacks the requiredresultTypeproperty → TS2741. - The same failure cascades to every
extends Resultinterface and toJSONRPCResultResponse(whoseresult: Resultfield inherits the requirement).
Why existing code doesn't prevent it
Result has an index signature [key: string]: unknown, but index signatures do not satisfy explicitly-declared required properties — TypeScript still demands resultType be present by name. Nothing in this PR touches schemas.ts or any handler to add the field on the SDK side.
Impact
- CI typecheck fails, so this PR cannot merge as-is.
- Even if the typecheck were bypassed, every SDK consumer constructing a result literal (e.g.
return { content: [...] }from a tool handler) would no longer satisfySpec.CallToolResult, and downstream consumers typing against the spec types would see the same breakage.
How to fix
The cleanest fix is upstream: change the spec's schema.ts to resultType?: ResultType; (matching the @default "complete" semantics) and re-run pnpm run fetch:spec-types. If the spec genuinely intends the field to be required on the wire, then this PR must be accompanied by an SDK change that adds resultType: z.enum(['complete', 'input_required']).default('complete') (or equivalent) to ResultSchema and updates all result-construction sites — but that is a much larger, non-mechanical change that should not land in an automated nightly sync.
There was a problem hiding this comment.
Partially superseded by the latest re-pull (c47bd846). The @default "complete" JSDoc that this comment used to argue resultType should be optional has been removed upstream and replaced with explicit normative text:
Servers implementing this protocol version MUST include this field. For backward compatibility, when a client receives a result from a server implementing an earlier protocol version (which does not include
resultType), the client MUST treat the absent field as"complete".
So the "upstream almost certainly intended resultType?" reading no longer holds — upstream has clarified the field is intentionally required on the wire, with the default applying only when receiving from older servers. The first remediation path in this comment (fix upstream to make it optional and re-pull) is no longer viable.
What is still unaddressed: ResultSchema in schemas.ts still has no resultType field, so the 30+ TS2741 errors in spec.types.test.ts (and the cascading extends Result failures across ListToolsResult/CallToolResult/etc.) remain. The only path now is the SDK-side companion change: add resultType: z.enum(['complete', 'input_required']).default('complete') to ResultSchema, thread it through every *Result schema and result-construction site, and update spec.types.test.ts. That's the larger non-mechanical change the original comment flagged as inappropriate for an automated nightly sync — it should still be done in a companion PR rather than landing this raw.
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
| /* Empty result */ | ||
| /** | ||
| * A result that indicates success but carries no data. | ||
| * | ||
| * @category Common Types | ||
| */ | ||
| export type EmptyResult = Result; | ||
|
|
||
| /** @internal */ | ||
| export type InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest; | ||
|
|
There was a problem hiding this comment.
🔴 🔴 The spec deletes URL_ELICITATION_REQUIRED = -32042 / URLElicitationRequiredError (replaced by the InputRequiredResult flow), but the SDK still ships this mechanism as public API: ProtocolErrorCode.UrlElicitationRequired (enums.ts:15), the UrlElicitationRequiredError class + ProtocolError.fromError() special-case (errors.ts:23-48), the public re-export (exports/public/index.ts:103), the tool-handler rethrow at packages/server/src/server/mcp.ts:211, and the examples/{client,server}/src/elicitationUrlExample.ts apps (plus docs/README links and test/integration/test/server/mcp.test.ts:1892-1937). None of these import spec.types.ts, so unlike the other findings on this PR they are silent — no typecheck or drift-guard failure surfaces them. The companion work needs to deprecate/remove the public error class + enum member (breaking change → migration.md), strip the mcp.ts special-case, and rewrite/remove the elicitationUrlExample apps.
Extended reasoning...
What changed in the spec
This sync deletes the entire -32042 error-response mechanism from the protocol. Before, the spec defined an implementation-specific JSON-RPC error code URL_ELICITATION_REQUIRED = -32042 and a URLElicitationRequiredError interface (a JSONRPCErrorResponse whose error.data.elicitations carried ElicitRequestURLParams[]). A server could respond to tools/call with this error to demand that the client open a browser URL before retrying. The diff removes both the constant and the interface; the replacement is the new InputRequiredResult result (with resultType: 'input_required' and inputRequests), which is delivered as a successful response, not an error. The error path is gone from the protocol entirely.
What the SDK still ships
The SDK implemented the -32042 flow end-to-end and exported it publicly. All of the following survive untouched after this PR:
packages/core/src/types/enums.ts:15—ProtocolErrorCode.UrlElicitationRequired = -32_042, an error code the spec no longer defines.packages/core/src/types/errors.ts:21-48—ProtocolError.fromError()special-cases code-32042and constructs aUrlElicitationRequiredError; theUrlElicitationRequiredErrorclass itself wrapselicitations: ElicitRequestURLParams[]intoerror.data.packages/core/src/exports/public/index.ts:103—export { ProtocolError, UrlElicitationRequiredError }puts the class on the package's public surface.packages/server/src/server/mcp.ts:211-213— thetools/callhandler catches thrown errors and, iferror.code === ProtocolErrorCode.UrlElicitationRequired, rethrows so the framework serialises it onto the wire as a JSON-RPC error response instead of wrapping it into aCallToolResultwithisError: true. This is runtime behaviour that emits a non-spec error code.examples/server/src/elicitationUrlExample.ts:58,102— tool handlersthrow new UrlElicitationRequiredError([...]).examples/client/src/elicitationUrlExample.ts:26,741— client catchesUrlElicitationRequiredErrorand drives the browser flow.test/integration/test/server/mcp.test.ts:1892-1937— integration test asserting the round-trip.docs/client.md:471,626,docs/server.md:477,examples/{client,server}/README.md— documentation linking to the example apps.
Why this is silent (and distinct from the other comments)
Every other finding on this PR is surfaced by spec.types.test.ts or tsc because the affected code references spec.types.ts. This one is not: enums.ts, errors.ts, mcp.ts, and the example apps do not import spec.types.ts. The enum member is a hand-written numeric literal (-32_042); the error class is a hand-written subclass of ProtocolError; the mcp.ts rethrow checks the enum, not the spec interface. Nothing in CI fails when URL_ELICITATION_REQUIRED and URLElicitationRequiredError are removed from spec.types.ts. A reviewer addressing only the existing comments would fix the schemas, the unions, and the test allowlists — and ship a release that still publicly exports an error class for a protocol mechanism that no longer exists.
This is also not a duplicate of the existing comments. Comment #3206453743 (line 3291) covers the server→client request restructuring (createMessage/elicitInput/listRoots becoming InputRequiredResult payloads) and its remediation list is scoped to ServerRequestSchema/ClientResultSchema and the server.createMessage()/elicitInput()/listRoots() APIs — it never mentions the -32042 error-response path, which is a completely separate code path (a tool handler throws, mcp.ts rethrows onto the wire as a JSON-RPC error). Comment #3206453749 (line 418) mentions URLElicitationRequiredError only as a stale entry in the MISSING_SDK_TYPES allowlist affecting the spec.types.test.ts type count; it does not mention enums.ts, errors.ts, the public re-export, mcp.ts:211, or the example apps.
Step-by-step proof
- Before (spec.types.ts pre-diff):
export const URL_ELICITATION_REQUIRED = -32042;andexport interface URLElicitationRequiredError extends Omit<JSONRPCErrorResponse, 'error'> { error: Error & { code: typeof URL_ELICITATION_REQUIRED; data: { elicitations: ElicitRequestURLParams[]; ... } } }. - After (this diff): both are deleted; the only remaining server-side mechanism for "need browser-based input before continuing" is to return
{ resultType: 'input_required', inputRequests: { ... } }as a successful result. - SDK runtime (
mcp.ts:200-215): a tool handler runs, throwsnew UrlElicitationRequiredError([...]). The catch block at line 211 seeserror.code === ProtocolErrorCode.UrlElicitationRequired(i.e.,-32_042) and rethrows.Protocolthen serialises this into a JSON-RPC error response witherror.code = -32042anderror.data.elicitations = [...]. - Wire: the SDK is now emitting an error code and error-data shape that the protocol no longer defines. A spec-compliant client built against the post-sync schema has no
URLElicitationRequiredErrortype to deserialise this into and no-32042constant to switch on. - Public surface:
import { UrlElicitationRequiredError } from '@modelcontextprotocol/sdk'still works (exports/public/index.ts:103), andexamples/server/src/elicitationUrlExample.tsactively demonstrates throwing it from a tool handler — teaching consumers a pattern the protocol just removed. - No CI signal:
grep -r 'spec.types' packages/core/src/types/{enums,errors}.ts packages/server/src/server/mcp.tsreturns nothing — these files have no compile-time link to the spec snapshot, so neithertscnorspec.types.test.tsflags them.
Impact
After this sync the SDK publicly exports, documents, demonstrates in two example apps, and special-cases at runtime a protocol mechanism that the spec has removed. New consumers following docs/server.md and elicitationUrlExample.ts will build servers that emit non-spec error responses; spec-compliant clients will see an unrecognised -32042 error instead of an InputRequiredResult they know how to fulfil.
How to fix
This is non-mechanical, public-API companion work that must accompany the sync (or its companion PR):
- Decide deprecate-vs-remove for
UrlElicitationRequiredErrorandProtocolErrorCode.UrlElicitationRequired. Either remove them outright (breaking → add amigration.mdentry and a major changeset) or mark them@deprecatedwith JSDoc pointing at theInputRequiredResultreplacement and schedule removal. - Strip the
mcp.ts:211-213rethrow special-case (and theProtocolError.fromError()branch aterrors.ts:23-28). - Rewrite or remove
examples/{client,server}/src/elicitationUrlExample.tsto use theInputRequiredResult/inputResponsesflow (this dovetails with the redesign already requested in comment #3206453743), and update the four README/docs links that point at them. - Delete
test/integration/test/server/mcp.test.ts:1892-1937(or rewrite it for the new flow).
| 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
| export const MISSING_REQUIRED_CLIENT_CAPABILITY = -32003; | ||
|
|
||
| /** | ||
| * An error response that indicates that the server requires the client to provide additional information via an elicitation request. | ||
| * Returned when the request's protocol version is unknown to the server or | ||
| * unsupported (e.g., a known experimental or draft version the server has | ||
| * chosen not to implement). For HTTP, the response status code MUST be | ||
| * `400 Bad Request`. | ||
| * | ||
| * @example Authorization required | ||
| * {@includeCode ./examples/URLElicitationRequiredError/authorization-required.json} | ||
| * @example Unsupported protocol version | ||
| * {@includeCode ./examples/UnsupportedProtocolVersionError/unsupported-version.json} | ||
| * | ||
| * @internal | ||
| * @category Errors | ||
| */ | ||
| export interface URLElicitationRequiredError extends Omit<JSONRPCErrorResponse, 'error'> { | ||
| export interface UnsupportedProtocolVersionError extends Omit<JSONRPCErrorResponse, 'error'> { | ||
| error: Error & { | ||
| code: typeof URL_ELICITATION_REQUIRED; | ||
| code: typeof INVALID_PARAMS; | ||
| data: { | ||
| elicitations: ElicitRequestURLParams[]; | ||
| [key: string]: unknown; | ||
| /** | ||
| * Protocol versions the server supports. The client should choose a | ||
| * mutually supported version from this list and retry. | ||
| */ | ||
| supported: string[]; | ||
| /** | ||
| * The protocol version that was requested by the client. | ||
| */ | ||
| requested: string; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔴 Beyond the 10 Multi Round-Trip types already flagged in the earlier comment, this updated sync brings in a second, disjoint batch of new exports with no SDK counterparts: DiscoverRequest/DiscoverResult/DiscoverResultResponse, SubscriptionFilter, SubscriptionsListenRequest/SubscriptionsListenRequestParams, SubscriptionsAcknowledgedNotification/SubscriptionsAcknowledgedNotificationParams, UnsupportedProtocolVersionError, MissingRequiredClientCapabilityError, and the MISSING_REQUIRED_CLIENT_CAPABILITY = -32003 constant. None of these appear in schemas.ts/types.ts/specTypeSchema.ts/enums.ts, so each one trips the spec.types.test.ts coverage check and widens the toHaveLength(176) delta further; ServerNotification now includes SubscriptionsAcknowledgedNotification and ServerResult now includes DiscoverResult, so those bidirectional union checks fail too. This is the additive half of the stateless-protocol overhaul — the subtractive half (deleted Initialize*/Ping*/SetLevel*/Subscribe*/Unsubscribe*/RootsListChangedNotification) is filed separately.
Extended reasoning...
What changed
The latest force-push to this PR layers a stateless-protocol overhaul on top of the Multi Round-Trip changes that the earlier comment (#3206453749) already enumerates. That earlier comment lists exactly 10 types (ResultType, InputRequest/InputResponse/InputRequests/InputResponses, InputRequiredResult, InputResponseRequestParams, TaskInputResponseRequest/TaskInputResponseRequestParams/TaskInputResponseResultResponse). This finding covers a second, fully disjoint batch of new exports introduced by the same diff:
| New export | Replaces / purpose |
|---|---|
DiscoverRequest / DiscoverResult / DiscoverResultResponse |
replacement for initialize |
SubscriptionFilter |
opt-in notification selector |
SubscriptionsListenRequest / SubscriptionsListenRequestParams |
replacement for resources/subscribe/unsubscribe + the HTTP GET stream |
SubscriptionsAcknowledgedNotification / SubscriptionsAcknowledgedNotificationParams |
first message on a subscriptions/listen stream |
UnsupportedProtocolVersionError |
new error envelope (code: INVALID_PARAMS, data.supported/data.requested) |
MissingRequiredClientCapabilityError |
new error envelope (code: -32003, data.requiredCapabilities) |
MISSING_REQUIRED_CLIENT_CAPABILITY = -32003 |
new error-code constant |
A grep for DiscoverRequest|DiscoverResult|SubscriptionFilter|SubscriptionsListen|SubscriptionsAcknowledged|UnsupportedProtocolVersionError|MissingRequiredClientCapability|MISSING_REQUIRED_CLIENT_CAPABILITY|-32003|32_003 across packages/core/src matches only spec.types.ts — no schemas.ts Zod schemas, no types.ts aliases, no specTypeSchema.ts registrations, no guards.ts predicates, and enums.ts has no ProtocolErrorCode member for -32003.
How it manifests in CI
packages/core/test/spec.types.test.ts enforces three invariants that this batch breaks (independently of the 10 Multi Round-Trip types and independently of the deletions filed in the sibling comment):
- Coverage check —
should have comprehensive compatibility tests(lines ~1098-1107) parses everyexport (interface|type) <Name>fromspec.types.ts, skips theMISSING_SDK_TYPESallowlist (['Error', 'URLElicitationRequiredError']), and assertssdkTypeChecks[<Name>]is defined.DiscoverRequest,DiscoverResult,DiscoverResultResponse,SubscriptionFilter,SubscriptionsListenRequest,SubscriptionsListenRequestParams,SubscriptionsAcknowledgedNotification,SubscriptionsAcknowledgedNotificationParams,UnsupportedProtocolVersionError, andMissingRequiredClientCapabilityErroreach produceexpected undefined to be defined. (MISSING_REQUIRED_CLIENT_CAPABILITYisexport const, so theextractExportedTypesregex does not pick it up — but the lack of aProtocolErrorCodeenum member for-32003is still a real SDK gap that needs addressing alongside the schema work.) - Type count —
expect(specTypes).toHaveLength(176)(line ~1089) was already off by the Multi Round-Trip delta; this batch widens it by another +10 exported interfaces/types on the additive side. - Union assignability — the diff adds
SubscriptionsAcknowledgedNotificationtoServerNotification(line 3289) andDiscoverResulttoServerResult(line 3308). The bidirectionalspec = sdk; sdk = spec;checks forServerNotification/ServerResultfail because the SDK unions inschemas.tscontain neither.
Why existing code doesn't prevent it
By design — spec.types.test.ts is the drift guard whose purpose is to turn the automated cron sync into a forcing function for SDK updates. The cron job updates only spec.types.ts; nothing generates the matching Zod schemas, SDK type aliases, specTypeSchemas registrations, ProtocolErrorCode enum members, or sdkTypeChecks entries.
Step-by-step proof
- After this diff,
spec.types.ts:377-405exportsUnsupportedProtocolVersionErrorandMissingRequiredClientCapabilityError; lines ~554-619 exportDiscoverRequest/DiscoverResult/DiscoverResultResponse; lines ~1170-1257 exportSubscriptionFilter/SubscriptionsListenRequest/SubscriptionsListenRequestParams/SubscriptionsAcknowledgedNotification/SubscriptionsAcknowledgedNotificationParams. spec.types.test.ts'sextractExportedTypesregex (/^export (interface|type) (\w+)/gm) picks up all 10 names.- None of those 10 names appears in
MISSING_SDK_TYPES(only'Error'and'URLElicitationRequiredError'). - None has an
sdkTypeChecksentry → 10 ×expect(sdkTypeChecks[name]).toBeDefined()failures. SDKTypes.ServerNotification(fromServerNotificationSchemainschemas.ts) has nonotifications/subscriptions/acknowledgedarm, sosdk = specfails forServerNotification; likewiseSDKTypes.ServerResulthas noDiscoverResultarm, sosdk = specfails forServerResult.grep -r '32003' packages/core/src/types/enums.ts→ no match; the SDK has no name for the new error code.
Why this is distinct from the existing PR comments
Comment #3206453749 enumerates only the 10 Multi Round-Trip types — it does not mention any of the Discover*/Subscription* types, the two error envelopes, or -32003. The sibling "deletions" comment covers the removal of Initialize*/Ping*/SetLevel*/Subscribe*/Unsubscribe*/RootsListChangedNotification and the lifecycle redesign that forces; this comment covers the additions whose remediation is mechanical schema work plus a new ProtocolErrorCode enum member. The split mirrors the PR's existing comment-#1/#2 structure for the InputRequiredResult flow (architectural removal vs. mechanical additions).
How to fix
Companion commit on top of the cron sync:
schemas.ts: addDiscoverRequestSchema/DiscoverResultSchema/DiscoverResultResponseSchema,SubscriptionFilterSchema,SubscriptionsListenRequestSchema/...ParamsSchema,SubscriptionsAcknowledgedNotificationSchema/...ParamsSchema,UnsupportedProtocolVersionErrorSchema,MissingRequiredClientCapabilityErrorSchema; addSubscriptionsAcknowledgedNotificationSchematoServerNotificationSchemaandDiscoverResultSchematoServerResultSchema; addDiscoverRequestSchema/SubscriptionsListenRequestSchematoClientRequestSchema.enums.ts: addProtocolErrorCode.MissingRequiredClientCapability = -32_003.types.ts/specTypeSchema.ts: add the corresponding SDK aliases andspecTypeSchemasregistrations.spec.types.test.ts: addsdkTypeChecksentries for all 10 new names (or add the two error envelopes toMISSING_SDK_TYPESif the SDK chooses to model them asProtocolErrorsubclasses rather than schemas, mirroring the priorURLElicitationRequiredErrortreatment), and update thetoHaveLength(...)count once both the additive and subtractive halves are reconciled.
| export interface DiscoverRequest extends JSONRPCRequest { | ||
| method: 'server/discover'; | ||
| params?: RequestParams; | ||
| } |
There was a problem hiding this comment.
🔴 🔴 The cron commit now pulled into this PR is the stateless-protocol overhaul, which goes far beyond the InputRequiredResult/Multi-Round-Trip changes the existing nine comments cover: InitializeRequest/InitializeResult/InitializeResultResponse/InitializedNotification are deleted (replaced by DiscoverRequest/DiscoverResult, method server/discover), PingRequest/PingResultResponse are deleted, SetLevelRequest/SetLevelResultResponse are deleted (replaced by per-request _meta['io.modelcontextprotocol/logLevel']), SubscribeRequest/UnsubscribeRequest are deleted (replaced by SubscriptionsListenRequest + SubscriptionsAcknowledgedNotification), RootsListChangedNotification is deleted, RequestParams._meta becomes required with three mandatory io.modelcontextprotocol/* keys, and ClientRequest/ClientNotification/ServerRequest are rewritten accordingly. The SDK's entire connection lifecycle is built on the deleted types — client.connect() sends initialize/notifications/initialized, server.ts registers an initialize handler and gates on it, Protocol.ping(), setLoggingLevel()/subscribeResource()/unsubscribeResource()/sendRootsListChanged(), plus InitializeRequestSchema/PingRequestSchema/SetLevelRequestSchema/SubscribeRequestSchema/UnsubscribeRequestSchema/InitializedNotificationSchema/RootsListChangedNotificationSchema in schemas.ts/types.ts/guards.ts/specTypeSchema.ts — and spec.types.test.ts hard-references all ~16 deleted SpecTypes.* names so tsc fails outright. None of the prior comments mention initialize→discover, ping removal, setLevel→_meta logLevel, subscribe/unsubscribe→subscriptions/listen, or RootsListChangedNotification removal; this is a separate, larger architectural divergence requiring an SDK handshake/keep-alive/log-level/subscription/roots-signalling redesign — not a mechanical schema add — so this PR must be held until that companion redesign exists.
Extended reasoning...
What changed
The latest commit pulled by this cron sync (8e192a22…, replacing 5c252088…) is the upstream stateless-protocol overhaul. The protocol no longer has a connection-lifecycle handshake; instead every request carries the handshake metadata in _meta. Concretely the diff:
- Deletes
InitializeRequest/InitializeRequestParams/InitializeResult/InitializeResultResponse/InitializedNotificationand replaces them withDiscoverRequest/DiscoverResult/DiscoverResultResponse(methodserver/discover, optional, returnssupportedVersions: string[]instead of a single negotiatedprotocolVersion). - Deletes
PingRequest/PingResultResponseoutright — no replacement;ServerRequestno longer contains a ping at all. - Deletes
SetLevelRequest/SetLevelRequestParams/SetLevelResultResponse— replaced by an optional per-request_meta['io.modelcontextprotocol/logLevel']key. - Deletes
SubscribeRequest/SubscribeRequestParams/SubscribeResultResponse/UnsubscribeRequest/UnsubscribeRequestParams/UnsubscribeResultResponse— replaced by a single long-livedSubscriptionsListenRequest(methodsubscriptions/listen) carrying aSubscriptionFilter, plus a server→clientSubscriptionsAcknowledgedNotification. - Deletes
RootsListChangedNotificationand reducesClientCapabilities.rootsto{}(thelistChangedflag is gone). - Makes
RequestParams._metarequired and adds three required keys toRequestMetaObject:'io.modelcontextprotocol/protocolVersion','io.modelcontextprotocol/clientInfo','io.modelcontextprotocol/clientCapabilities'(plus optional'…/logLevel'). - Adds
UnsupportedProtocolVersionError/MissingRequiredClientCapabilityError/MISSING_REQUIRED_CLIENT_CAPABILITY = -32003. - Rewrites the unions:
ClientRequestdropsPingRequest/InitializeRequest/SetLevelRequest/SubscribeRequest/UnsubscribeRequestand addsDiscoverRequest/SubscriptionsListenRequest;ClientNotificationdropsInitializedNotification/RootsListChangedNotification;ServerRequestdropsPingRequest;ServerNotificationaddsSubscriptionsAcknowledgedNotification;ServerResultswapsInitializeResult→DiscoverResult.
Immediate breakage
packages/core/test/spec.types.test.ts hard-references the deleted names — e.g. SpecTypes.InitializeRequest/InitializeRequestParams/InitializeResult/InitializeResultResponse/InitializedNotification/PingRequest/PingResultResponse/SetLevelRequest/SetLevelRequestParams/SetLevelResultResponse/SubscribeRequest/SubscribeRequestParams/SubscribeResultResponse/UnsubscribeRequest/UnsubscribeRequestParams/UnsubscribeResultResponse/RootsListChangedNotification at lines ~42/58/62/81/139/143/287/304/308/312/479/483/671/675/812/816-817/823/886/940/948/955-956/963-964/973/987-989. Each becomes an unconditional TS2694: Namespace '…spec.types' has no exported member '…' — the package will not compile. The hardcoded type-count assertion and the ClientRequest/ClientNotification/ServerRequest/ServerResult bidirectional checks all fail too. (The earlier comments' "176 vs 182" / "10 added, 4 removed" numbers were written against a smaller previous revision; the current diff removes ~17 and adds ~20 types, so even those comments' counts are stale.)
Architectural divergence (the part that matters)
The SDK's runtime is built around the very flows this commit deletes:
- Handshake:
packages/client/src/client/client.ts:connect()sends{ method: 'initialize', params: { protocolVersion, capabilities, clientInfo } }and thennotifications/initialized;packages/server/src/server/server.tsregisters an'initialize'handler and a'notifications/initialized'handler and gates every other request behindassertInitialized(). - Keep-alive:
Protocol.ping()sends{ method: 'ping' }. - Logging level:
client.setLoggingLevel()sendslogging/setLevel;server.tsregisters a'logging/setLevel'handler. - Resource subscriptions:
client.subscribeResource()/unsubscribeResource()sendresources/subscribe/resources/unsubscribe. - Roots change signalling:
client.sendRootsListChanged()sendsnotifications/roots/list_changed. - Schemas/types/guards:
schemas.tsdefinesInitializeRequestSchema/InitializedNotificationSchema/PingRequestSchema/SetLevelRequestSchema/SubscribeRequestSchema/UnsubscribeRequestSchema/RootsListChangedNotificationSchema, all referenced fromtypes.ts,guards.ts,specTypeSchema.ts, and theClientRequestSchema/ServerRequestSchema/ClientNotificationSchemaunions.
After this sync, an SDK client talking to a spec-compliant server would send an initialize request the server is not obligated to handle, would never populate the now-required _meta.{protocolVersion,clientInfo,clientCapabilities} on each request, would attempt ping/logging/setLevel/resources/subscribe calls that no longer exist, and has no implementation of server/discover or subscriptions/listen. Conversely, an SDK server would reject every request from a spec-compliant client because assertInitialized() never sees an initialize.
Step-by-step proof
- Before (spec.types.ts @
5c252088):export interface InitializeRequest extends JSONRPCRequest { method: 'initialize'; params: InitializeRequestParams }, andClientRequest = PingRequest | InitializeRequest | … | SetLevelRequest | … | SubscribeRequest | UnsubscribeRequest | …. - After (this diff, lines 566-569):
export interface DiscoverRequest extends JSONRPCRequest { method: 'server/discover'; params?: RequestParams }. Grepping the new file forInitializeRequest/'initialize'/PingRequest/'ping'/SetLevelRequest/'logging/setLevel'/SubscribeRequest/'resources/subscribe'/'resources/unsubscribe'/RootsListChangedNotification/'notifications/roots/list_changed'returns nothing. spec.types.test.ts:139does(sdk: WithJSONRPCRequest<SDKTypes.InitializeRequest>, spec: SpecTypes.InitializeRequest) => { … }→ TS2694 "no exported member 'InitializeRequest'". Same for ~16 other deleted names →tsc --noEmit -p packages/corefails before any test runs.packages/server/src/server/server.tsregistersthis.setRequestHandler(InitializeRequestSchema, …)andthis.setNotificationHandler(InitializedNotificationSchema, …);packages/client/src/client/client.ts:connect()doesawait this.request({ method: 'initialize', … }, InitializeResultSchema)thenthis.notification({ method: 'notifications/initialized' }). None of these have a spec counterpart anymore.RequestParams._metais now non-optional with three requiredio.modelcontextprotocol/*keys, butschemas.ts'sRequestParamsSchemadeclares_metaoptional with no such keys, so theRequest/RequestParams/RequestMetaObjectbidirectional checks inspec.types.test.tsalso fail.
Why this is distinct from the existing nine comments
- #3206453743 covers only sampling/elicitation/roots moving into
InputRequiredResult. - #3206453749 covers only the 10 new Multi-Round-Trip type additions.
- #3206453753 / #3214351594 cover
resultTyperequired/discriminant. - #3214351591 covers
tasks/input_responselackingtaskId. - #3214351593 covers JSDoc
/* */vs/** */. - #3216586348 covers
-32042/URLElicitationRequiredErrorremoval. - #3216586352 / #3216586357 cover
tasks.requestscapability andInputResponseerror arm.
None of them mention initialize→discover, ping removal, setLevel→_meta logLevel, subscribe/unsubscribe→subscriptions/listen, RootsListChangedNotification removal, the required _meta keys, or the new UnsupportedProtocolVersionError/MissingRequiredClientCapabilityError types. Their type-count arithmetic ("182") confirms they were written against an earlier, smaller revision of this PR.
Recommended action
Hold this automated sync until a companion redesign lands. That work is not "add schemas" — it is:
- Replace the
connect()-timeinitialize/initializedhandshake with per-request_metainjection (protocolVersion/clientInfo/clientCapabilities) and an optionalserver/discovercall; rip outassertInitialized()gating on the server. - Remove
Protocol.ping()(or repoint it at a transport-level liveness check). - Replace
setLoggingLevel()with a per-request_meta['io.modelcontextprotocol/logLevel']setter. - Replace
subscribeResource()/unsubscribeResource()with asubscriptions/listenstream +SubscriptionsAcknowledgedNotificationhandling. - Remove
sendRootsListChanged()and theroots.listChangedcapability plumbing. - Add schemas for
DiscoverRequest/DiscoverResult/DiscoverResultResponse/SubscriptionFilter/SubscriptionsListenRequest/SubscriptionsListenRequestParams/SubscriptionsAcknowledgedNotification/SubscriptionsAcknowledgedNotificationParams/UnsupportedProtocolVersionError/MissingRequiredClientCapabilityError/MISSING_REQUIRED_CLIENT_CAPABILITY; delete the seven now-orphaned*Schemadefinitions and their references intypes.ts/guards.ts/specTypeSchema.tsand theClientRequestSchema/ServerRequestSchema/ClientNotificationSchemaunions. - Update
spec.types.test.ts: remove the ~17 deletedsdkTypeChecksentries, add the new ones, and fix the type-count assertion.
81ef4d8 to
29aa626
Compare
| export interface SubscriptionsListenRequest extends JSONRPCRequest { | ||
| method: 'subscriptions/listen'; | ||
| params: SubscriptionsListenRequestParams; | ||
| } |
There was a problem hiding this comment.
🟡 🟡 Upstream spec gap to batch with the other schema.ts feedback: SubscriptionsListenRequest extends JSONRPCRequest (so it carries an id and per JSON-RPC 2.0 must eventually receive exactly one response object), but unlike every other *Request extends JSONRPCRequest in this file it has no SubscriptionsListenResult / SubscriptionsListenResultResponse, and ServerResult includes none. The spec instead has the server send SubscriptionsAcknowledgedNotification "as the first message on the stream" — a notification (no id), not a response — so the wire contract for completing/closing the subscriptions/listen request is undefined, and when the SDK adds SubscriptionsListenRequestSchema it will have no spec-defined result schema to pair with Protocol.request() (the pending-request map would leak the id if the server never responds). Suggest upstream either add SubscriptionsListenResultResponse { result: EmptyResult } (sent on stream close) or document that this request is intentionally never JSON-RPC-responded so the SDK can special-case its id bookkeeping.
Extended reasoning...
What the bug is
SubscriptionsListenRequest (spec.types.ts:1223) is declared as:
export interface SubscriptionsListenRequest extends JSONRPCRequest {
method: 'subscriptions/listen';
params: SubscriptionsListenRequestParams;
}Because it extends JSONRPCRequest, it carries jsonrpc: '2.0' and id: RequestId. Per JSON-RPC 2.0 §4.2, a request object that includes an id MUST eventually receive exactly one response object (either a result or an error) carrying that same id. But the spec defines no SubscriptionsListenResult or SubscriptionsListenResultResponse, and ServerResult (line 3296+) has no member for it. The only server reaction the spec defines is SubscriptionsAcknowledgedNotification — "sent by the server as the first message on a subscriptions/listen stream" — which extends JSONRPCNotification and therefore has no id and cannot satisfy the JSON-RPC response requirement.
Why this is uniquely inconsistent
Grepping the file shows 11 concrete interfaces that extends JSONRPCRequest (DiscoverRequest, PaginatedRequest and its derivatives, ReadResourceRequest, SubscriptionsListenRequest, GetPromptRequest, CallToolRequest, GetTaskRequest, GetTaskPayloadRequest, TaskInputResponseRequest, CancelTaskRequest, CompleteRequest) and 15 *ResultResponse extends JSONRPCResultResponse wrappers. Every JSONRPCRequest subtype has a paired *ResultResponse — except SubscriptionsListenRequest. The deleted predecessors it replaces (SubscribeRequest / UnsubscribeRequest) both had explicit { result: EmptyResult } wrappers (SubscribeResultResponse / UnsubscribeResultResponse), so this is a regression in spec completeness, not an established convention for "streaming" requests. ServerResult does include EmptyResult, so a server could respond with that on stream close, but the spec does not say so and there is no SubscriptionsListenResultResponse documenting it.
Step-by-step proof
- Client sends
{ jsonrpc: '2.0', id: 7, method: 'subscriptions/listen', params: { notifications: {...} } }(this is a JSON-RPC request becauseSubscriptionsListenRequest extends JSONRPCRequest, line 1223). - Server replies with
{ jsonrpc: '2.0', method: 'notifications/subscriptions/acknowledged', params: { notifications: {...} } }(line 1255 —extends JSONRPCNotification, noid). - Per JSON-RPC 2.0, message (2) is a notification, not a response to
id: 7. The client's pending-request entry forid: 7is still outstanding. - The spec defines no message that ever carries
id: 7back. There is noSubscriptionsListenResultResponse;ServerResulthas no dedicated arm; the JSDoc onSubscriptionsListenRequestdoes not say "the server never responds" or "the server responds withEmptyResulton stream close". - Three behaviours are therefore equally spec-compliant and mutually incompatible: (a) server sends
{ id: 7, result: {} }immediately and then streams notifications; (b) server sends{ id: 7, result: {} }only when the stream closes; (c) server never responds and the client must special-case theid. The wire contract is undefined.
Concrete SDK impact
The SDK's Protocol.request() API takes a request and a result schema, stores the id in a pending-request map, and resolves the returned promise when a matching response arrives. When the companion work (already requested in comment #3223937253) adds SubscriptionsListenRequestSchema, there is no spec-defined result schema to pass as the second argument. If the SDK uses EmptyResultSchema and a server follows interpretation (c), the promise never resolves and the pending-request map leaks id: 7 for the lifetime of the connection. If the SDK special-cases this method to not await a response and a server follows interpretation (a), the SDK will receive an unsolicited { id: 7, result: {} } it has no handler for. Either way, encoding SubscriptionsListenRequestSchema before this is settled bakes a guess into the public surface that breaks once upstream picks one.
Why this is distinct from existing PR comments
Comment #3223937253 lists SubscriptionsListenRequest/SubscriptionsListenRequestParams among the "new types needing SDK schemas" but does not note that there is no response type to pair them with — it assumes mechanical schema work, which is precisely what this gap blocks. Comment #3223937258 covers the SDK lifecycle redesign (subscribeResource()/unsubscribeResource() → subscriptions/listen) but does not address the spec's own JSON-RPC response-contract gap. This is the same class of upstream-schema.ts design feedback as #3214351591 (taskId missing), #3214351594 (discriminant not narrowed), and #3216586357 (InputResponse error arm) — concrete enough to block correct SDK implementation, but doesn't independently break CI (the drift guard checks for types that exist, not types that should exist), hence nit.
How to fix
Raise upstream alongside the other schema.ts feedback. Two options:
- Add a response wrapper:
export interface SubscriptionsListenResultResponse extends JSONRPCResultResponse { result: EmptyResult; }and document that the server sends it when the stream closes (or immediately after the ack notification, depending on intended semantics). This matches the deletedSubscribeResultResponse/UnsubscribeResultResponseprecedent. - Document the no-response contract: state explicitly in the
SubscriptionsListenRequestJSDoc that the server MUST NOT send a JSON-RPC response for this request and that clients MUST NOT track itsidin their pending-request map (or, more cleanly, change it to extendJSONRPCNotificationso it carries noidat all — though that loses the ability to error-respond).
Then re-run pnpm run fetch:spec-types. Don't add SubscriptionsListenRequestSchema to schemas.ts until this is settled.
| 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.
29aa626 to
07e2103
Compare
| /** | ||
| * A successful response from the server for a {@link SubscribeRequest | resources/subscribe} request. | ||
| * Sent from the client to open a long-lived channel for receiving notifications | ||
| * outside the context of a specific request. Replaces the previous HTTP GET | ||
| * endpoint and ensures consistent behavior between HTTP and STDIO. | ||
| * | ||
| * @example Subscribe result response | ||
| * {@includeCode ./examples/SubscribeResultResponse/subscribe-result-response.json} | ||
| * @example Listen for tools and resource list changes | ||
| * {@includeCode ./examples/SubscriptionsListenRequest/listen-for-list-changes.json} | ||
| * | ||
| * @category `resources/subscribe` | ||
| * @category `subscriptions/listen` | ||
| */ | ||
| export interface SubscribeResultResponse extends JSONRPCResultResponse { | ||
| result: EmptyResult; | ||
| export interface SubscriptionsListenRequest extends JSONRPCRequest { | ||
| method: 'subscriptions/listen'; | ||
| params: SubscriptionsListenRequestParams; | ||
| } |
There was a problem hiding this comment.
🟡 🟡 The companion redesign also has transport-layer work in packages/{server,client}/src/**/streamableHttp.ts that none of the existing comments mention and that nothing in CI surfaces (those files don't import spec.types.ts — same silent class as the -32042 finding). (1) This JSDoc says subscriptions/listen "Replaces the previous HTTP GET endpoint" — the server transport still implements that endpoint (handleGetRequest / _standaloneSseStreamId='_GET_stream', streamableHttp.ts:234/358/404/433/937/964) and the client transport still opens a bare GET stream for unsolicited notifications (_startOrAuthSse, streamableHttp.ts:233/251/647) instead of issuing subscriptions/listen. (2) The new RequestMetaObject JSDoc (lines 77-80) and the UnsupportedProtocolVersionError/MissingRequiredClientCapabilityError JSDoc add three HTTP MUSTs — _meta protocolVersion MUST match the MCP-Protocol-Version header else 400, and both error envelopes MUST be HTTP 400 — but validateProtocolVersion() (streamableHttp.ts:889) checks only the header (never cross-checks body _meta), and Protocol-layer JSON-RPC errors flow through send() into a hardcoded HTTP 200 (lines 817/1022/1024) with no error-code→HTTP-status hook. The redesign needs to: route subscriptions/listen to the long-lived SSE stream (and decide GET back-compat vs 405); have the client transport send subscriptions/listen instead of opening GET; add header↔_meta cross-validation; and add a Protocol→transport hook so these two error envelopes surface as HTTP 400.
Extended reasoning...
What this finding covers
Two transport-layer normative changes in this sync land in JSDoc prose only and affect packages/server/src/server/streamableHttp.ts and packages/client/src/client/streamableHttp.ts. Neither file imports spec.types.ts, so — like the -32042 / UrlElicitationRequiredError finding (#3216586348) — there is zero CI signal: tsc, spec.types.test.ts, and the specTypeSchema allowlist guard all stay green for these files. None of the 13 existing PR comments mention streamableHttp.ts, the GET handler, or HTTP-status-code mapping; comments #3223937253/#3223937258 cover the schema/lifecycle/subscribeResource() side of subscriptions/listen, and #3231781722 covers its missing JSON-RPC result type, but the transport plumbing is a separate code path.
(1) GET standalone-SSE endpoint replaced by subscriptions/listen
The SubscriptionsListenRequest JSDoc (this line) explicitly says it "Replaces the previous HTTP GET endpoint and ensures consistent behavior between HTTP and STDIO." The Streamable HTTP transport implements that GET endpoint as core functionality:
- Server (
packages/server/src/server/streamableHttp.ts):case 'GET'→handleGetRequest()at lines 358-359/404; the standalone notification stream is keyed by_standaloneSseStreamId = '_GET_stream'(line 234) with 409-conflict handling (line 433), close/cleanup (449/469), and the unsolicited-message send path / event-store replay routed through it (lines 937/964/967). - Client (
packages/client/src/client/streamableHttp.ts):_startOrAuthSse()issuesmethod: 'GET'(lines 233/251) to open the unsolicited-notification stream, kicked off afternotifications/initialized(line 647) and on resume (lines 535/756), with reconnect at 347.
After this sync the spec no longer defines a GET endpoint for the standalone notification stream; a spec-compliant server is not obligated to accept GET. The client transport already tolerates a 405 on GET (lines 285-288), so it would not crash against such a server — but it would silently degrade to never receiving any unsolicited server notifications, because nothing in the client transport sends subscriptions/listen. Conversely, the server transport has no path that routes an incoming subscriptions/listen JSON-RPC request to the long-lived SSE stream — the only way to open that stream is GET.
(2) New HTTP-400 MUSTs with no transport hook
This sync adds three HTTP-transport-specific MUST clauses in JSDoc:
RequestMetaObject['io.modelcontextprotocol/protocolVersion'](lines 77-80): "For the HTTP transport, this value MUST match theMCP-Protocol-Versionheader; otherwise the server MUST return a400 Bad Request."UnsupportedProtocolVersionError(line ~383): "For HTTP, the response status code MUST be400 Bad Request."MissingRequiredClientCapabilityError(line ~413): "For HTTP, the response status code MUST be400 Bad Request."
The server transport's validateProtocolVersion() (streamableHttp.ts:889-898) reads the MCP-Protocol-Version header on every request and 400s if it's not in _supportedProtocolVersions, but it never cross-checks the header against the body params._meta['io.modelcontextprotocol/protocolVersion'] (which the SDK doesn't populate yet — that's the per-request _meta injection work in #3223937258). And Protocol-layer JSON-RPC errors generated by handlers flow back through the transport's send() into an already-opened HTTP 200 SSE/JSON body (lines 817/1022/1024) — there is no hook by which Protocol can tell the transport "this particular JSONRPCErrorResponse should be delivered as HTTP 400 instead of inside a 200." So once the redesign starts emitting UnsupportedProtocolVersionError (INVALID_PARAMS + data.supported/requested) or MissingRequiredClientCapabilityError (-32003), they will go out as HTTP 200, violating the new MUSTs.
Step-by-step proof
GET → subscriptions/listen:
- Spec post-sync:
SubscriptionsListenRequestJSDoc says it "Replaces the previous HTTP GET endpoint"; the GET endpoint is no longer part of the transport spec. - SDK client connects, completes handshake, and at
streamableHttp.ts:647calls_startOrAuthSse({}), which issuesfetch(url, { method: 'GET', headers: { Accept: 'text/event-stream', … } })(line 251). - A spec-compliant server with no GET handler returns 405. Client hits line 287 and silently returns — no error, no notification stream, no
subscriptions/listensent. The user'sToolListChangedNotification/ResourceUpdatedNotificationhandlers never fire. - Conversely, an SDK server receiving a POST
{ method: 'subscriptions/listen', … }would dispatch it throughhandlePostRequest→Protocol._onrequestlike any RPC; nothing wires it to_standaloneSseStreamId, so unsolicitedsend()calls (line 937/967) still look up'_GET_stream'and find nothing.
HTTP-400 MUSTs:
- After the redesign, client sends POST with header
MCP-Protocol-Version: 2026-03-01and bodyparams._meta['io.modelcontextprotocol/protocolVersion'] = '2025-11-07'. validateProtocolVersion()(line 889) reads onlyreq.headers.get('mcp-protocol-version'), sees'2026-03-01'∈_supportedProtocolVersions, returnsundefined. The header↔body mismatch is never checked → no 400, contrary to lines 77-80.- Separately, a tool handler determines the request needs
elicitationbut_meta.clientCapabilities.elicitationis absent, and (per the redesign) emits aMissingRequiredClientCapabilityError(code: -32003).Protocolserialises it and callstransport.send(errorResponse, { relatedRequestId }). send()finds the pending stream for that request and either writes an SSE event into the already-200 stream or callsstream.resolveJson(Response.json(responses[0], { status: 200, … }))(lines 1022/1024). The-32003error reaches the client inside an HTTP 200 body — violating the "MUST be 400" clause.
Why existing code doesn't prevent it
streamableHttp.ts (both packages) does not import spec.types.ts, so the drift guard / spec.types.test.ts cannot see this. The new requirements are JSDoc prose, not types, so even the bidirectional-assignability checks would not catch them. And the existing comments' remediation lists are scoped to schemas.ts/types.ts/client.ts/server.ts — a reviewer following only those could replace subscribeResource() with a subscriptions/listen API and add the two error-envelope schemas without ever touching the transport's GET handler or its hardcoded-200 send path.
Impact
This is forward-looking scoping for the companion redesign rather than an independent blocker (the SDK doesn't yet emit -32003 or populate _meta.protocolVersion, and keeping GET as a back-compat shim is a viable transitional state). But it identifies two non-obvious design decisions the redesign must make at the transport layer — (a) how subscriptions/listen is wired to the long-lived SSE stream and what happens to handleGetRequest, and (b) a new Protocol→transport hook to map specific JSON-RPC error envelopes to non-200 HTTP status — that someone doing pure schemas.ts/server.ts work would plausibly miss.
How to fix
In the companion redesign:
- Server transport: route
method: 'subscriptions/listen'to the long-lived SSE stream (taking over the role of_standaloneSseStreamId/event-store replay); decide whetherhandleGetRequeststays as a back-compat shim or returns 405. ExtendvalidateProtocolVersion()(or add a sibling) to compare theMCP-Protocol-Versionheader against bodyparams._meta['io.modelcontextprotocol/protocolVersion']and 400 on mismatch. Add an error-code→HTTP-status hook sosend()(or the pre-stream path) can emit HTTP 400 when the outgoingJSONRPCErrorResponseis anUnsupportedProtocolVersionError(INVALID_PARAMS+data.supported/requested) orMissingRequiredClientCapabilityError(-32003). - Client transport: replace
_startOrAuthSse's bare GET with a POSTsubscriptions/listencarrying aSubscriptionFilter, and treat the resulting SSE stream (andSubscriptionsAcknowledgedNotification) as the unsolicited-notification channel; keep a GET fallback only if back-compat with pre-spec servers is desired.
| export interface SubscriptionFilter { | ||
| /** | ||
| * If true, receive {@link ToolListChangedNotification | notifications/tools/list_changed}. | ||
| */ | ||
| toolsListChanged?: boolean; | ||
| /** | ||
| * If true, receive {@link PromptListChangedNotification | notifications/prompts/list_changed}. | ||
| */ | ||
| promptsListChanged?: boolean; | ||
| /** | ||
| * If true, receive {@link ResourceListChangedNotification | notifications/resources/list_changed}. | ||
| */ | ||
| resourcesListChanged?: boolean; | ||
| /** | ||
| * Subscribe to {@link ResourceUpdatedNotification | notifications/resources/updated} for these resource URIs. | ||
| * Replaces the former `resources/subscribe` RPC. | ||
| */ | ||
| resourceSubscriptions?: string[]; | ||
| } |
There was a problem hiding this comment.
🟡 Upstream spec gap to batch with the other schema.ts feedback: ElicitationCompleteNotification (line 3243, "informing it of a completion of a out-of-band elicitation request") survives in ServerNotification (line 3291), but after this sync there is no spec-defined channel to deliver it. ElicitRequest is now only an InputRequest payload inside InputRequiredResult (so the originating request's stream is closed when the result arrives), and the standalone GET stream is replaced by subscriptions/listen — whose SubscriptionFilter is an explicit opt-in allowlist with only four fields (toolsListChanged/promptsListChanged/resourcesListChanged/resourceSubscriptions) and JSDoc saying the server "MUST NOT send notification types the client has not explicitly requested here." So per the spec's own MUST NOT, a server cannot push elicitation-complete on the only out-of-band channel; a client doing a URL-mode elicitation can only poll by blindly retrying. (TaskStatusNotification, also in ServerNotification and absent from SubscriptionFilter, arguably has the same gap.) Same tier as comments #9/#12/#13 — suggest upstream either add elicitationComplete?: boolean (and taskStatus?: boolean) to SubscriptionFilter, or remove ElicitationCompleteNotification from ServerNotification if poll-via-retry is the intended model.
Extended reasoning...
What the bug is
ElicitationCompleteNotification (spec.types.ts:3243, method notifications/elicitation/complete, JSDoc: "informing it of a completion of a out-of-band elicitation request") survives this diff and remains a member of ServerNotification (line 3291). But after the stateless overhaul there is no spec-defined channel on which a server can actually deliver it.
The pre-sync flow was: a server emitted URLElicitationRequiredError (-32042) or sent an ElicitRequest JSON-RPC; the client opened the URL in a browser; the server detected completion out-of-band and pushed ElicitationCompleteNotification on the standalone HTTP GET/SSE stream so the client knew when to retry.
Post-sync, both halves of that delivery path are gone:
- (a)
ElicitRequestno longer extendsJSONRPCRequest— it is only anInputRequestpayload embedded insideInputRequiredResult(line 438). TheInputRequiredResultis the terminal result of the originatingtools/call/prompts/get/resources/read/tasks/resultrequest, so that request's response stream is closed once the result is delivered. There is no still-open per-request stream to carry a later notification. - (b) The standalone HTTP GET stream is replaced by
subscriptions/listen(line 1223; its JSDoc at 1214-1216 says it "Replaces the previous HTTP GET endpoint"). That stream'sSubscriptionFilter(lines 1179-1197) is a closed, explicit opt-in allowlist with exactly four fields —toolsListChanged,promptsListChanged,resourcesListChanged,resourceSubscriptions— and the JSDoc at lines 1174-1175 (and again at 1206-1208) is normative: "Each notification type is opt-in; the server MUST NOT send notification types the client has not explicitly requested here." There is no field for elicitation-complete.
So per the spec's own MUST NOT, ElicitationCompleteNotification is undeliverable on the only out-of-request channel, and there is no in-request channel left. The notification type is in ServerNotification but unreachable.
Step-by-step proof
- Client sends
tools/call. Server returnsCallToolResultResponse { result: InputRequiredResult { inputRequests: { 'e1': { method: 'elicitation/create', params: { mode: 'url', url: 'https://…', elicitationId: 'abc' } } } } }. Thetools/callrequest is complete; its response stream closes. - Client opens
https://…in a browser. The user completes the OAuth/consent flow there. The server learns this out-of-band (callback, webhook, etc.). - The server now wants to send
{ method: 'notifications/elicitation/complete', params: { elicitationId: 'abc' } }so the client knows to retry thetools/callwithinputResponses. - Per-request stream: gone — the originating
tools/callalready returned in step 1. subscriptions/listenstream: the client could only have requested{ toolsListChanged?, promptsListChanged?, resourcesListChanged?, resourceSubscriptions? }— there is noelicitationCompletefield inSubscriptionFilter. Per lines 1174-1175 the server "MUST NOT send notification types the client has not explicitly requested", so sendingnotifications/elicitation/completeon this stream violates the spec's own normative text.- There is no third channel. The client's only option is to poll: blindly retry
tools/call(withrequestState) on a timer until the server stops returninginput_required.
Cross-checking the other ServerNotification members
This isn't a general "SubscriptionFilter is incomplete for everything" complaint — most ServerNotification members do have a defined channel:
| Member | Delivery channel |
|---|---|
CancelledNotification / ProgressNotification / LoggingMessageNotification |
per-request stream (request-scoped) |
SubscriptionsAcknowledgedNotification |
first message on the subscriptions/listen stream itself |
ResourceListChangedNotification / ResourceUpdatedNotification / ToolListChangedNotification / PromptListChangedNotification |
subscriptions/listen, gated by the four SubscriptionFilter fields |
ElicitationCompleteNotification |
none — by definition out-of-band, no SubscriptionFilter field |
TaskStatusNotification |
arguably none — also absent from SubscriptionFilter; tasks at least have tasks/get polling as a documented fallback, whereas URL elicitation has none |
ElicitationCompleteNotification is the clear odd-one-out (and TaskStatusNotification strengthens the case that the closed allowlist is incomplete rather than that elicitation-complete was intentionally dropped).
Why this is distinct from existing PR comments
None of comments #1-#13 mention ElicitationCompleteNotification. Comment #7 (#3216586348) covers the -32042/UrlElicitationRequiredError SDK-side removal (the error path), not this notification's reachability. Comments #11/#12 cover subscriptions/listen (the missing result wrapper and the new types needing schemas) but not the gap in SubscriptionFilter's allowlist. This is the same tier as #9 (InputResponse no error arm), #12 (SubscriptionsListenRequest no result), and #13 (RequestParams._meta direction mismatch): an upstream schema.ts self-inconsistency that doesn't independently break CI but identifies a shape the SDK should not encode as-is.
Impact
Doesn't independently break CI — ElicitationCompleteNotification itself is unchanged by the diff, so no assignability/coverage check trips on it specifically. But when the companion work adds SubscriptionFilterSchema to schemas.ts (per comment #10), encoding only the four fields bakes in a filter that can never permit elicitation-complete, and when it rewrites the URL-elicitation flow (per comments #1/#7) it will have no spec-defined replacement for the "server pushes when browser flow completes" signal. The SDK would have to either (a) ship a poll-on-timer client API, or (b) invent an off-spec extra SubscriptionFilter field — both break once upstream picks the real answer.
How to fix
Raise upstream alongside the other schema.ts feedback. Two options:
- Extend
SubscriptionFilter: addelicitationComplete?: boolean(orelicitationIds?: string[]for per-elicitation granularity), and probablytaskStatus?: booleanfor symmetry. This keepsElicitationCompleteNotificationdeliverable on thesubscriptions/listenstream and matches the four existing list-changed fields. - Remove
ElicitationCompleteNotificationfromServerNotificationif poll-via-retry is the intended model — i.e., the client just re-issues the original request withrequestStateuntilresultType !== 'input_required'. In that case the notification type is dead and should be deleted alongside the other elicitation cleanup.
Then re-run pnpm run fetch:spec-types. Don't add SubscriptionFilterSchema to schemas.ts until this is settled. Filed as a nit because it doesn't add new red CI and poll-via-retry is a viable (if ugly) fallback — same tier as comments #5/#6/#8/#9/#12/#13.
| * 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).
| * Each notification type is **opt-in**; the server **MUST NOT** send | ||
| * notification types the client has not explicitly requested here. |
There was a problem hiding this comment.
🟡 Nit (upstream): the JSDoc on ResourceListChangedNotification (line 1158), PromptListChangedNotification (1602), and ToolListChangedNotification (1741) still say "This may be issued by servers without any previous subscription from the client", which now directly contradicts the new SubscriptionFilter JSDoc here ("the server MUST NOT send notification types the client has not explicitly requested"). The same diff did update ResourceUpdatedNotification's JSDoc (line 1277) to the opt-in wording, so this looks like a partial migration that missed the three *ListChanged siblings — worth replacing the offending sentence on all three with wording mirroring the updated ResourceUpdatedNotification text and re-pulling. (Distinct from comment #3239359160, which covers the stale ServerCapabilities.{*.listChanged,resources.subscribe} JSDoc, and from #3239359156, which covers SubscriptionFilter's allowlist gaps.)
Extended reasoning...
What the issue is
Three notification-type JSDoc blocks — all untouched by this diff — still describe the pre-opt-in delivery model:
ResourceListChangedNotification(spec.types.ts:1158): "This may be issued by servers without any previous subscription from the client."PromptListChangedNotification(spec.types.ts:1602): identical sentence.ToolListChangedNotification(spec.types.ts:1741): identical sentence.
The new SubscriptionFilter JSDoc introduced by this same sync (lines 1174-1175, repeated at 1206-1208 on SubscriptionsListenRequestParams.notifications) is normative the opposite way: "Each notification type is opt-in; the server MUST NOT send notification types the client has not explicitly requested here." And SubscriptionFilter explicitly enumerates toolsListChanged / promptsListChanged / resourcesListChanged as the gates for exactly these three notifications. So the spec file now contains a direct internal contradiction: the type-level JSDoc says "may be issued without any previous subscription"; the channel-level JSDoc says "MUST NOT be sent unless explicitly subscribed."
Why this is a partial-migration slip, not intentional
This is not a case of the upstream author leaving older prose alone wholesale. The same diff hunk did update the fourth sibling, ResourceUpdatedNotification (spec.types.ts:1277), from:
"This should only be sent if the client previously sent a resources/subscribe request."
to:
"This is only sent for resources the client opted in to via the
resourceSubscriptionsfield of a subscriptions/listen request."
So the upstream author migrated one of four notification descriptions to the new opt-in model and missed the other three. A grep for "may be issued by servers without any previous subscription" finds exactly three surviving instances — the classic partial-migration signature.
Step-by-step proof
- After this sync, the only out-of-request channel for unsolicited server notifications is
subscriptions/listen(line 1223; its JSDoc at 1214-1216 says it "Replaces the previous HTTP GET endpoint"). SubscriptionsListenRequestParams.notifications: SubscriptionFilter(line 1209) is the opt-in selector; the JSDoc at 1174-1175 says the server "MUST NOT send notification types the client has not explicitly requested here."SubscriptionFilter.toolsListChanged(line 1182) gatesnotifications/tools/list_changed;.promptsListChanged(1186) gatesnotifications/prompts/list_changed;.resourcesListChanged(1190) gatesnotifications/resources/list_changed.- Therefore, per the spec's own MUST NOT, none of these three notifications can be "issued by servers without any previous subscription from the client." The sentence at lines 1158/1602/1741 is now affirmatively wrong, not merely stale.
- An implementer hovering
ToolListChangedNotificationin their IDE sees only the type's own JSDoc — "may be issued without any previous subscription" — and would reasonably build a server that pushes it unprompted, violating the new MUST NOT.
Why this is distinct from the existing PR comments
- Comment #3239359160 covers stale
ServerCapabilities.{prompts,resources,tools}.listChanged/resources.subscribeJSDoc (lines 755/776/780/795) — different location, different fix (capability-flag semantics vs. notification-type delivery semantics). - Comment #3239359156 covers
SubscriptionFilter's allowlist being incomplete forElicitationCompleteNotification/TaskStatusNotification— about missing filter fields, not about contradictory prose on the three notifications that do have filter fields. - None of the 16 prior comments mention lines 1158/1602/1741 or the "may be issued … without any previous subscription" sentence.
Impact
Same tier as comments #3214351593 (JSDoc /* vs /**) and #3239359160 — pure upstream prose, no CI/type impact, spec.types.ts is not re-exported from this package. Filed as a nit to fold into the same upstream schema.ts batch already on the table.
How to fix
In upstream schema.ts, replace "This may be issued by servers without any previous subscription from the client." on all three with wording mirroring the already-updated ResourceUpdatedNotification JSDoc, e.g.:
ResourceListChangedNotification: "This is only sent if the client opted in via theresourcesListChangedfield of a {@link SubscriptionsListenRequest | subscriptions/listen} request."PromptListChangedNotification: "…via thepromptsListChangedfield…"ToolListChangedNotification: "…via thetoolsListChangedfield…"
Then re-run pnpm run fetch:spec-types. No change is appropriate in this repo directly (the file header says DO NOT EDIT).
0bcec2b to
57ccae0
Compare
| params: TaskStatusNotificationParams; | ||
| } | ||
|
|
||
| /* Logging */ |
There was a problem hiding this comment.
🔴 🔴 The c47bd846 revision layers a third architectural removal on top of the Multi-Round-Trip (#3206453743/49) and stateless (#3223937258) overhauls: the entire tasks subsystem is deleted — TaskAugmentedRequestParams, TaskStatus/TaskMetadata/Task, CreateTaskResult*/GetTask*/GetTaskPayload*/ListTasks*/CancelTask*, TaskStatusNotification, ToolExecution/Tool.execution, {Server,Client}Capabilities.tasks are all gone, and export type ServerRequest is removed entirely (the protocol now defines no server→client requests; ClientResult = EmptyResult only). This supersedes/obsoletes comments #3214351591, #3216586352, #3246176908, #3246176911 (which discuss task types that no longer exist) and the 3 TaskInputResponse* entries in #3206453749's list. Beyond ~24 more TS2694 errors in spec.types.test.ts, the SDK still ships tasks as silent public API — 9 files under packages/{core,client,server}/src/experimental/tasks/**, ~20 task schemas in schemas.ts (TaskSchema 628, ToolExecutionSchema 1288, ToolSchema.execution 1341, CallToolRequestParamsSchema = TaskAugmentedRequestParamsSchema.extend 1412, ServerRequestSchema 2119, 'tools/call' → CallToolResult|CreateTaskResult 2171), shared/taskManager.ts, {Server,Client}TasksCapabilityWithRuntime, assert{Client,Server}RequestTaskCapability — none of which import spec.types.ts. The companion work must deprecate/remove the experimental/tasks surface, strip Tool.execution/task: TaskMetadata from CallTool, delete ServerRequestSchema and rework Protocol's server→client request typing, and add migration.md entries.
Extended reasoning...
What changed in c47bd846
The latest cron force-push layers a third wholesale architectural removal on top of the two already documented (Multi-Round-Trip in #3206453743/#3206453749, stateless handshake in #3223937258): the entire tasks subsystem is deleted from the protocol. The diff removes the full /* Tasks */ block (~280 lines, ~24 exports): TaskAugmentedRequestParams, TaskStatus, TaskMetadata, RelatedTaskMetadata, Task, CreateTaskResult/CreateTaskResultResponse, GetTaskRequest/GetTaskResult/GetTaskResultResponse, GetTaskPayloadRequest/GetTaskPayloadResult/GetTaskPayloadResultResponse, CancelTaskRequest/CancelTaskResult/CancelTaskResultResponse, ListTasksRequest/ListTasksResult/ListTasksResultResponse, TaskStatusNotification/TaskStatusNotificationParams, ToolExecution, plus Tool.execution, ServerCapabilities.tasks, ClientCapabilities.tasks, the InvalidParamsError Tasks JSDoc bullet, and the CancelledNotification task-related JSDoc. CallToolRequestParams now extends InputResponseRequestParams instead of TaskAugmentedRequestParams. Most consequentially, export type ServerRequest is deleted with no replacement — the /* Server messages */ block now contains only ServerNotification and ServerResult; the protocol defines no server→client JSON-RPC requests at all, and ClientResult collapses to bare EmptyResult.
Existing comments are now stale
This supersedes or obsoletes four existing PR comments that were written against the prior revision where task types still existed:
- #3214351591 (
TaskInputResponseRequestParamslackstaskId) — that type no longer exists. - #3216586352 (
ClientCapabilities.tasks.requestsorphaned) —ClientCapabilities.tasksis gone entirely. - #3246176908 (
ServerRequestis exclusively task members → unreachable) — written assumingServerRequeststill exists; it has now been deleted, which is the more aggressive resolution upstream chose. - #3246176911 (
TaskInputResponseResultResponse.resultun-narrowed) — that type no longer exists. - #3206453749's list of "10 Multi Round-Trip types" includes 3
TaskInputResponse*types that never landed in c47bd846.
None of the 20 existing comments say "the entire tasks subsystem is deleted", "ServerRequest itself is removed", "ToolExecution/Tool.execution is removed", or "the experimental/tasks package surface needs deprecation". Comment #3223937258 explicitly describes ServerRequest as "rewritten" to drop PingRequest while keeping the four task members — that description is now wrong.
Immediate CI breakage (beyond what existing comments document)
packages/core/test/spec.types.test.ts hard-references SpecTypes.ServerRequest (~line 499), SpecTypes.ToolExecution (559/899), SpecTypes.TaskStatus (563), SpecTypes.TaskMetadata (567/900), SpecTypes.Task (575/902), SpecTypes.CreateTaskResult (579/903/1008), SpecTypes.GetTaskRequest (607/979), SpecTypes.TaskStatusNotification[Params] (615/619/909/949), SpecTypes.CreateTaskResultResponse (718), and the rest of the GetTask*/ListTasks*/CancelTask*/GetTaskPayload* family — each is an unconditional TS2694: Namespace has no exported member error (~24 additional, on top of those already documented). The hardcoded toHaveLength(176) is now off by an additional ~−24 on the subtractive side. And the ClientRequest/ClientNotification/ServerResult bidirectional checks fail in new ways because the spec dropped GetTaskRequest|GetTaskPayloadRequest|ListTasksRequest|CancelTaskRequest from ClientRequest, TaskStatusNotification from ClientNotification/ServerNotification, and CreateTaskResult|GetTaskResult|GetTaskPayloadResult|ListTasksResult|CancelTaskResult from ServerResult.
Silent SDK divergence (no CI signal — same class as #3216586348/#3239359153/#3246176899)
The SDK ships tasks as public experimental API with no compile-time link to spec.types.ts:
- 9 files under
packages/{core,client,server}/src/experimental/tasks/**—TaskManager,InMemoryTaskStore, helpers, server/client task façades. schemas.ts:TaskMetadataSchema(45),RelatedTaskMetadataSchema(53),TaskAugmentedRequestParamsSchema(81),TaskStatusSchema(622),TaskSchema(628),CreateTaskResultSchema(654),GetTaskRequestSchema(674) and the rest of theGetTask*/ListTasks*/CancelTask*/GetTaskPayload*schemas,ToolExecutionSchema(1288),ToolSchema.shape.execution(1341),CallToolRequestParamsSchema = TaskAugmentedRequestParamsSchema.extend(...)(1412),ServerRequestSchema(2119), and the method-result map entry'tools/call' → z.union([CallToolResultSchema, CreateTaskResultSchema])(2171).packages/core/src/shared/taskManager.ts,Protocoltask plumbing,{Server,Client}TasksCapabilityWithRuntime,assertClientRequestTaskCapability/assertServerRequestTaskCapability, plus the correspondingtypes.ts/guards.ts/specTypeSchema.tsentries.
None of these import spec.types.ts, so neither tsc --noEmit on src/ nor the specTypeSchema allowlist guard flags any of it. A reviewer addressing only the existing 20 comments would fix the schemas/unions/test allowlists for the Multi-Round-Trip and stateless changes — and ship a release that still publicly exports InMemoryTaskStore, callTool({ task: { ttl } }), Tool.execution.taskSupport, and a ServerRequestSchema for a request direction the protocol no longer defines.
Step-by-step proof
- Before (
spec.types.ts@ 8e192a22, the revision comments #3223937258/#3246176908 were written against):export type ServerRequest = GetTaskRequest | GetTaskPayloadRequest | ListTasksRequest | CancelTaskRequest;export interface Task { taskId; status; … };Tool.execution?: ToolExecution;ServerCapabilities.tasks?: { list?, cancel?, requests? }. - After (this diff, c47bd846):
grep -E '^export (type|interface) (ServerRequest|Task|TaskStatus|TaskMetadata|GetTaskRequest|ListTasksRequest|CancelTaskRequest|CreateTaskResult|ToolExecution|TaskAugmentedRequestParams)' packages/core/src/types/spec.types.ts→ zero matches. The/* Server messages */block contains onlyServerNotificationandServerResult;ClientResult = EmptyResultonly;ClientRequestends atListToolsRequest. spec.types.test.ts:499does(sdk: SDKTypes.ServerRequest, spec: SpecTypes.ServerRequest) => { sdk = spec; spec = sdk; }→ TS2694 "Namespace '…/spec.types' has no exported member 'ServerRequest'". Same for ~24 other deleted task names →tsc --noEmit -p packages/corefails before any test runs.- SDK runtime still exposes the task flow end-to-end:
new McpServer({ capabilities: { tasks: { requests: { tools: { call: {} } } } } })advertises a capability the spec no longer defines;server.registerTool('foo', { execution: { taskSupport: 'optional' } }, …)populatesToolSchema.executionfor a field the spec deleted;client.callTool({ name: 'foo', task: { ttl: 60_000 } })buildsCallToolRequestParamsSchemafromTaskAugmentedRequestParamsSchema.extend(...)and emitsparams.taskon the wire — a field a c47bd846-compliant server has no schema for. Protocol's generic typing is parameterised over<SendRequestT, SendNotificationT, SendResultT>withServerRequestSchema(schemas.ts:2119) feeding the server-side request union. With the spec defining no server→client requests, that schema and theProtocol.request()server-side outbound path have no spec counterpart — the SDK must decide whetherServerRequestbecomesnever, is removed from the generic signature, or is kept as a back-compat shim.- No CI signal:
grep -r 'spec.types' packages/core/src/experimental packages/core/src/shared/taskManager.ts packages/{client,server}/src/experimental→ nothing. The 9experimental/tasks/**files,taskManager.ts, and the ~20 task schemas all typecheck cleanly.
Impact
This is a fourth non-mechanical redesign axis (after #3206453743 InputRequiredResult, #3223937258 stateless handshake, and the subscriptions/listen transport work). It is not "add/remove schemas" — it requires:
- Deprecate/remove the entire
packages/{core,client,server}/src/experimental/tasks/**public surface (TaskManager,InMemoryTaskStore,{Server,Client}TasksCapabilityWithRuntime,assert{Client,Server}RequestTaskCapability, thecapabilities.tasksconstructor handling) — breaking change →migration.mdentry + major changeset. - Strip
task: TaskMetadataSchemafromCallToolRequestParamsSchema(it must extend the newInputResponseRequestParamsSchemainstead ofTaskAugmentedRequestParamsSchema); removeToolExecutionSchemaandToolSchema.shape.execution; removeCreateTaskResultSchemafrom the'tools/call'result-map union (2171). - Delete
ServerRequestSchema(2119) and reworkProtocol's server→client request typing — the spec now defines no such requests, so the SDK must decide whether the server-siderequest()outbound path is removed, narrowed tonever, or kept as a deprecated shim. - Remove ~20 task schemas from
schemas.ts, theirtypes.tsaliases,guards.tspredicates, andspecTypeSchema.tsregistrations; droptasksfrom{Server,Client}CapabilitiesSchema; deleteshared/taskManager.ts. - In
spec.types.test.ts: delete the ~24sdkTypeChecksentries for task types andServerRequest, and update thetoHaveLength(...)count once all four axes are reconciled.
None of the existing comments' remediation lists mention any task-type deletion, ToolExecution removal, ServerRequestSchema deletion, or experimental/tasks deprecation, so this needs to be tracked as a separate companion-work axis.
| export interface CacheableResult extends Result { | ||
| /** | ||
| * A hint from the server indicating how long (in milliseconds) the | ||
| * client MAY cache this response before re-fetching. Semantics are | ||
| * analogous to HTTP Cache-Control max-age. | ||
| * | ||
| * - If 0, The response SHOULD be considered immediately stale, | ||
| * The client MAY re-fetch every time the result is needed. | ||
| * - If positive, the client SHOULD consider the result fresh for this many | ||
| * milliseconds after receiving the response. | ||
| * | ||
| * @minimum 0 | ||
| */ | ||
| ttlMs: number; | ||
|
|
||
| /** | ||
| * Indicates the intended scope of the cached response, analogous to HTTP | ||
| * `Cache-Control: public` vs `Cache-Control: private`. | ||
| * | ||
| * - `"public"`: Any client or intermediary (e.g., shared gateway, proxy) | ||
| * MAY cache the response and serve it to any user. | ||
| * - `"private"`: Only the requesting user's client MAY cache the response. | ||
| * Shared caches (e.g., multi-tenant gateways) MUST NOT serve a cached | ||
| * copy to a different user. | ||
| * | ||
| */ | ||
| cacheScope: 'public' | 'private'; | ||
| } |
There was a problem hiding this comment.
🔴 🔴 The latest c47bd846 revision adds a new CacheableResult mixin (lines 949-976) with required ttlMs: number and cacheScope: 'public' | 'private', and makes ListResourcesResult/ListResourceTemplatesResult/ReadResourceResult/ListPromptsResult/ListToolsResult all extend it — none of the existing comments cover this. A grep across packages/core matches only spec.types.ts (no Zod schema, no sdkTypeChecks entry, no specTypeSchemas registration), so this independently breaks the spec.types.test.ts coverage check (CacheableResult → expected undefined to be defined) and adds TS2741 "Property 'ttlMs' is missing" / "'cacheScope' is missing" errors to the bidirectional/AssertExactKeys checks for all five result types and ServerResult, separate from the resultType errors in comment #3206453753. Same upstream-optionality class as that comment: the JSDoc calls ttlMs a "hint" the client "MAY cache" on, and the HTTP Cache-Control analogy is opt-in, so both fields almost certainly should be ttlMs? / cacheScope? upstream before the SDK adds a CacheableResultSchema and threads required cache fields through every list/read result-construction site in mcp.ts.
Extended reasoning...
What the bug is
The latest force-push (upstream commit c47bd846) adds a new mixin interface to packages/core/src/types/spec.types.ts (lines 949-976):
export interface CacheableResult extends Result {
/**
* A hint from the server indicating how long (in milliseconds) the
* client MAY cache this response before re-fetching. Semantics are
* analogous to HTTP Cache-Control max-age.
* ...
*/
ttlMs: number;
/**
* Indicates the intended scope of the cached response, analogous to HTTP
* `Cache-Control: public` vs `Cache-Control: private`.
* ...
*/
cacheScope: 'public' | 'private';
}Both fields are declared without ?, so they are required. The same diff then changes five result types to extends CacheableResult: ListResourcesResult (line 999), ListResourceTemplatesResult (1035), ReadResourceResult (1096), ListPromptsResult (1392), and ListToolsResult (1593). None of the 20 existing PR comments mention CacheableResult, ttlMs, or cacheScope — comments #3206453749/#3223937253 enumerate the Multi Round-Trip and Discover/Subscription additions, and #3206453753 covers resultType-required on the base Result, but this is a fully disjoint addition from the latest revision.
How it manifests in CI
A grep for CacheableResult|ttlMs|cacheScope across packages/core/src and packages/core/test matches only spec.types.ts — no Zod schema in schemas.ts, no SDK type alias in types.ts, no specTypeSchemas registration, and no sdkTypeChecks entry in spec.types.test.ts. So the drift guard fails in two ways that are independent of the resultType errors already documented:
- Coverage check —
should have comprehensive compatibility testsparses everyexport (interface|type) <Name>fromspec.types.ts, skipsMISSING_SDK_TYPES, and assertssdkTypeChecks[<Name>]is defined.CacheableResultis a new exported interface with no entry →expected undefined to be defined. (And the hardcodedtoHaveLength(176)count is off by one more.) - Bidirectional / exact-keys checks —
spec.types.test.tsdoesspec = sdkforListToolsResult/ListResourcesResult(lines ~255/320) andAssertExactKeysfor all five affected result types (lines ~850-862). The SDK'sListToolsResultSchemaetc. inschemas.tsare stillPaginatedResultSchema.extend({ tools: ... })with no cache fields, so each produces TS2741 "Property 'ttlMs' is missing in type '{ ... }' but required in type 'ListToolsResult'" (and likewise forcacheScope).ServerResult(which unions all five) gets the same error.
These errors occur on top of the resultType ones — even if resultType were made optional upstream, all of the above still fail.
Why existing code doesn't prevent it
By design — the cron job updates only spec.types.ts; the drift guard exists precisely to surface this. schemas.ts's ListToolsResultSchema/ListPromptsResultSchema/ListResourcesResultSchema/ListResourceTemplatesResultSchema/ReadResourceResultSchema have no ttlMs/cacheScope fields, and nothing in mcp.ts or any handler sets them when constructing list/read results.
Step-by-step proof
- After this diff,
spec.types.ts:949-976definesCacheableResultwithttlMs: numberandcacheScope: 'public' | 'private'— neither has a?. ListToolsResult(line 1593) is changed fromextends PaginatedResulttoextends PaginatedResult, CacheableResult. SoSpecTypes.ListToolsResultnow requires{ tools: Tool[]; nextCursor?: Cursor; ttlMs: number; cacheScope: 'public'|'private'; resultType: ResultType; _meta?: ... }.schemas.ts'sListToolsResultSchema(line ~1360) is unchanged:PaginatedResultSchema.extend({ tools: z.array(ToolSchema) }). The inferred SDK type has nottlMsorcacheScope.spec.types.test.tsdoes(sdk: SDKTypes.ListToolsResult, spec: SpecTypes.ListToolsResult) => { spec = sdk; }. TypeScript rejects: TS2741 "Property 'ttlMs' is missing in type '{ tools: ...; nextCursor?: ...; _meta?: ... }' but required in type 'ListToolsResult'", then the same forcacheScope.- The same failure repeats for
ListResourcesResult,ListResourceTemplatesResult,ReadResourceResult,ListPromptsResult, and theServerResultunion check; theAssertExactKeys<SpecTypes.ListXxxResult, SDKTypes.ListXxxResult>checks at lines ~850-862 fail for the same reason. - Independently,
extractExportedTypespicks up'CacheableResult'; it is not inMISSING_SDK_TYPESand not insdkTypeChecks→ coverage assertion fails.
Impact
CI is red on this PR for an additional, distinct reason. Beyond the test failures, every SDK consumer's server.tool(...) / listTools handler that returns { tools: [...] } would no longer satisfy Spec.ListToolsResult, and once the companion work mirrors this into schemas.ts, every list/read result-construction site in mcp.ts would have to supply ttlMs/cacheScope even when the server has no caching opinion — a substantial, non-mechanical change for fields the JSDoc itself describes as a "hint" the client "MAY" honor.
Upstream-design observation (same class as comment #3206453753)
Declaring ttlMs/cacheScope as required contradicts the JSDoc's own framing: ttlMs is "a hint … the client MAY cache", and the explicit analogy is HTTP Cache-Control, which is opt-in — an HTTP server omits the header when it has no caching policy. Forcing every server to emit caching directives on every list/read result (including ones backed by dynamic per-request data where caching is meaningless) is the same defect class as resultType being required despite @default "complete". Both fields almost certainly should be ttlMs?: number / cacheScope?: 'public' | 'private' in upstream schema.ts.
How to fix
Two parts, mirroring the structure of comments #3206453749/#3206453753:
- Upstream (preferred): change
schema.tstottlMs?: numberandcacheScope?: 'public' | 'private'(matching the "hint"/"MAY cache"/Cache-Controlopt-in semantics), then re-runpnpm run fetch:spec-types. Batch with theresultType?(#3206453753) and other upstream feedback already on the table. - SDK companion work (regardless of optionality): add
CacheableResultSchematoschemas.tsand haveListResourcesResultSchema/ListResourceTemplatesResultSchema/ReadResourceResultSchema/ListPromptsResultSchema/ListToolsResultSchemaextend it; registerCacheableResultinspecTypeSchemas; add asdkTypeChecks.CacheableResultentry inspec.types.test.tsand bump the type count. If upstream keeps the fields required, also threadttlMs/cacheScopethrough every list/read result-construction site inmcp.ts— but that should not land in an automated nightly sync.
57ccae0 to
e9de0a8
Compare
e9de0a8 to
786d94c
Compare
| * The desired log level for this request. Optional. | ||
| * | ||
| * If absent, the server MUST NOT send any {@link LoggingMessageNotification | notifications/message} | ||
| * notifications for this request. The client opts in to log messages by | ||
| * explicitly setting a level. Replaces the former `logging/setLevel` RPC. | ||
| */ | ||
| 'io.modelcontextprotocol/logLevel'?: LoggingLevel; |
There was a problem hiding this comment.
🔴 Two new normative MUST-NOT clauses in this sync invert the unsolicited-notification default from opt-out to opt-in — _meta['io.modelcontextprotocol/logLevel'] for notifications/message, and SubscriptionFilter for the four list-changed/resource-updated notifications — and the SDK's server-side default-emit paths now actively violate both, silently (neither server.ts nor mcp.ts imports spec.types.ts, so no CI signal). isMessageIgnored() defaults to send when no level is set, and McpServer auto-fires send*ListChanged() on every registerTool/registerPrompt/registerResource/update gated only on isConnected(), never on a client-supplied filter. The companion redesign needs to invert both defaults (absent level/filter → suppress, not send) and add per-session SubscriptionFilter tracking — neither of which is captured by the existing porting checklists.
Extended reasoning...
What changed in the spec
Two separate JSDoc clauses in this sync flip the protocol's unsolicited-notification model from opt-out to opt-in with normative MUST NOT text:
- Logging (
RequestMetaObject['io.modelcontextprotocol/logLevel'], lines ~99-105): "If absent, the server MUST NOT send anynotifications/messagenotifications for this request. The client opts in to log messages by explicitly setting a level." This replaces the pre-syncLoggingMessageNotificationJSDoc — "If nologging/setLevelrequest has been sent from the client, the server MAY decide which messages to send automatically." MAY-decide → MUST-NOT-without-opt-in. - List-changed / ResourceUpdated (
SubscriptionFilterJSDoc, lines ~1129-1135 and ~1163-1166): "Each notification type is opt-in; the server MUST NOT send notification types the client has not explicitly requested here." The fourSubscriptionFilterfields gate exactlynotifications/{tools,prompts,resources}/list_changedandnotifications/resources/updated. Pre-sync the protocol's model was opt-out (the old*ListChangedNotificationJSDoc says "may be issued by servers without any previous subscription from the client").
What the SDK does — both defaults are still opt-out
Logging (packages/server/src/server/server.ts):
server.ts:194—private _loggingLevels = new Map<string | undefined, LoggingLevel>(), populated only by thelogging/setLevelhandler atserver.ts:142-153. After this sync that RPC is deleted, so a spec-compliant client never populates the map.server.ts:200-203—isMessageIgnored()returnscurrentLevel ? <severity check> : false. With the map empty,currentLevelisundefined, so it returnsfalse("not ignored" = send) for every level on every session.server.ts:646-650—sendLoggingMessage()gates only onthis._capabilities.logging && !isMessageIgnored(...). WithisMessageIgnored()alwaysfalseand the server advertising theloggingcapability, every call emits anotifications/message.server.ts:162—RequestHandlerExtra.log()(the per-request logging helper every tool/prompt/resource handler receives) wires straight through tosendLoggingMessage()with no_meta.logLevelcheck at all.mcp.ts:1024(McpServer.sendLoggingMessage()) is the same path.
List-changed / ResourceUpdated (packages/server/src/server/mcp.ts + server.ts):
mcp.ts:605/621/652/685/747/830/836/996—McpServercallssendResourceListChanged()/sendToolListChanged()/sendPromptListChanged()unconditionally on every tool/prompt/resource register, update, enable, disable, and remove.mcp.ts:1030-1052— those wrappers gate only onisConnected(), nothing else.server.ts:302-340—assertNotificationCapability()checks only the server's ownthis._capabilities.{tools,resources,prompts}advertisement, never the client's per-streamSubscriptionFilteropt-in.grep -rn 'SubscriptionFilter\|subscriptions/listen\|resourceSubscriptions' packages/server/srcreturns zero matches — there is no per-session filter tracking anywhere in the package.
Step-by-step proof
Logging:
- A spec-compliant client built against c47bd846 sends
tools/callwith_meta = { protocolVersion, clientInfo, clientCapabilities }(the three required keys) and nologLevel. - The tool handler calls
extra.log('info', 'starting work'). server.ts:162→sendLoggingMessage()→isMessageIgnored('info', sessionId)→_loggingLevels.get(sessionId)isundefined→ returnsfalse.- The notification is emitted. The client receives an unsolicited
notifications/messagefor a request where it never opted in — violating the new MUST NOT.
List-changed:
- A spec-compliant client connects, never sends
subscriptions/listen, never setstoolsListChanged: true. - Server code does
mcpServer.registerTool('foo', { ... }, handler)— a routine post-connect registration. mcp.ts:836unconditionally callsthis.sendToolListChanged();mcp.ts:1039-1041seesisConnected() === trueand forwards toserver.sendToolListChanged().assertNotificationCapability('notifications/tools/list_changed')atserver.ts:302+passes because the server advertisescapabilities.tools— it never consults a client filter.- The transport emits
notifications/tools/list_changedto a client that never requested it — violating the new MUST NOT. Same for prompts/resources/list_changed and resources/updated.
Why no CI signal & why the existing comments don't catch this
Neither server.ts nor mcp.ts imports spec.types.ts, so the drift guard never trips. This is the same silent-divergence class as #3216586348 (-32042 removal), #3246176899 (session bootstrap), and #3239359153 (GET endpoint) — distinct surface, same blind spot.
The existing comments cover adjacent surfaces but not these defaults: #3223937258 lists logging only as "Replace setLoggingLevel() with a per-request _meta logLevel setter" — a literal port (delete the setLevel handler, read _meta instead) could leave the : false default in isMessageIgnored() untouched and still spam every client. #3239359160 covers stale ServerCapabilities flag JSDoc, #3239359153 covers streamableHttp.ts GET routing, #3246176905 covers the upstream JSDoc contradiction in the *ListChangedNotification types — none mention McpServer's auto-fire-on-registration behavior or that assertNotificationCapability() gates on the wrong side of the connection.
How to fix (companion-redesign scope)
- Logging: Invert
isMessageIgnored()'s default — absent level → suppress. The level source must be the originating request's_meta['io.modelcontextprotocol/logLevel'](threaded throughRequestHandlerExtra.log()), not a per-session map. Delete_loggingLevelsand thelogging/setLevelhandler. Decide whether the un-scopedserver.sendLoggingMessage()/mcpServer.sendLoggingMessage()APIs survive at all — there is no spec-defined channel for unsolicited log notifications post-sync. - List-changed: Add per-session
SubscriptionFiltertracking toServer(or the transport). Havemcp.ts:1030-1052andserver.ts:652-671no-op when the active filter lacks the corresponding field, instead of gating only onisConnected(). Or, transitionally, drop the auto-fire-on-registration behavior and makesendXxxListChanged()an explicit user-called API gated on the discovered filter.
This is forward-looking scoping for the already-required companion redesign on an automated cron PR, not a defect this PR introduces — but it identifies two non-obvious default-direction flips that the existing 22-comment porting checklist would not catch.
This PR updates
packages/core/src/types/spec.types.tsfrom the Model Context Protocol specification.Source file: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/c47bd846ac3a38e262080e197d28dcecb847a279/schema/draft/schema.ts
This is an automated update triggered by the nightly cron job.