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.
a64a23e to
af35bb5
Compare
| export interface TaskInputResponseRequestParams extends RequestParams { | ||
| /** | ||
| * The client's responses to the server's input requests from | ||
| * the {@link InputRequiredResult} returned by {@link GetTaskPayloadRequest | tasks/result}. | ||
| */ | ||
| inputResponses: InputResponses; | ||
| } |
There was a problem hiding this comment.
🔴 TaskInputResponseRequestParams carries only inputResponses — it has no taskId (and no requestState, since it extends bare RequestParams rather than InputResponseRequestParams), unlike every other tasks/* method (tasks/get, tasks/result, tasks/cancel), which all take { taskId: string }. A server with more than one task in input_required status has no spec-defined way to route the responses to the correct task. This is the same class of upstream schema.ts defect as the resultType-required finding — it should be fixed upstream (add taskId: string) and re-pulled before the SDK encodes this shape into a Zod schema.
Extended reasoning...
What the bug is
The new tasks/input_response request's params type is:
export interface TaskInputResponseRequestParams extends RequestParams {
inputResponses: InputResponses;
}RequestParams (spec.types.ts:112) contains only _meta?: RequestMetaObject, so the only data this request carries on the wire is { inputResponses, _meta? }. There is no taskId, and because it extends bare RequestParams rather than InputResponseRequestParams, there is also no requestState — so neither of the two routing mechanisms available elsewhere in the Multi Round-Trip flow is present here.
Why this is inconsistent with the rest of tasks/*
Every other task-targeting method in the spec identifies the task explicitly:
| method | params | line |
|---|---|---|
tasks/get |
{ taskId: string } |
spec.types.ts:1957 |
tasks/result |
{ taskId: string } |
spec.types.ts:1992 |
tasks/cancel |
{ taskId: string } |
spec.types.ts:2078 |
tasks/input_response |
{ inputResponses } — no taskId |
spec.types.ts:2050 |
The InputRequests JSDoc says only that keys are "server-assigned identifiers"; it does not require them to be globally unique across tasks. Relying on the server to embed task identity in those keys would be an undocumented implicit contract that contradicts the explicit-taskId convention used by every sibling method.
Step-by-step proof
- Client creates two tasks; both reach
status: 'input_required'. - Client calls
tasks/resultwith{ taskId: 'task-A' }→ server returnsInputRequiredResult { inputRequests: { 'req-1': <ElicitRequest> } }. - Client calls
tasks/resultwith{ taskId: 'task-B' }→ server returnsInputRequiredResult { inputRequests: { 'req-1': <ElicitRequest> } }(nothing in the spec forbids reusing'req-1'per-task). - Client now sends
tasks/input_responsewithparams: { inputResponses: { 'req-1': <ElicitResult> } }. - The server receives
{ inputResponses: { 'req-1': … } }and has no field telling it whether this fulfills task-A or task-B.InputRequiredResultitself carries notaskId, andTaskInputResponseRequestParamscarries notaskIdand norequestState. The request is unroutable by spec.
Why existing code doesn't prevent it
The synchronous path is fine: CallToolRequestParams / GetPromptRequestParams / ReadResourceRequestParams now extend InputResponseRequestParams, which carries requestState — the server can stash routing info there. But TaskInputResponseRequestParams extends RequestParams, not InputResponseRequestParams, so it gets neither requestState nor a taskId. And the SDK's task plumbing (packages/core/src/shared/taskManager.ts) keys everything by taskId, so when the companion work from comment #2 adds a tasks/input_response handler, it will have nothing to look up.
Impact
This doesn't independently break CI (unlike resultType-required), but it blocks correct implementation of the companion SDK work this PR demands. If the SDK adds TaskInputResponseRequestSchema matching this shape, it bakes an unroutable request into the public surface and will need a breaking change once upstream adds taskId.
How to fix
Same remediation path as the resultType-required finding: file upstream against modelcontextprotocol/schema/draft/schema.ts to add taskId: string to TaskInputResponseRequestParams (and likely requestState?: string, or have it extend InputResponseRequestParams), then re-run pnpm run fetch:spec-types. Don't add a Zod schema for this type in its current shape.
| /* 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).
| * @category `sampling/createMessage` | ||
| */ | ||
| export interface CreateMessageRequestParams extends TaskAugmentedRequestParams { | ||
| export interface CreateMessageRequestParams { |
There was a problem hiding this comment.
🟡 Upstream spec inconsistency to batch with the other schema.ts feedback: this sync strips extends TaskAugmentedRequestParams from CreateMessageRequestParams/ElicitRequestFormParams/ElicitRequestURLParams (and extends JSONRPCRequest from their requests), so task-augmented sampling/elicitation no longer exists in the protocol — yet ClientCapabilities.tasks.requests.{sampling.createMessage, elicitation.create} (spec.types.ts:628-647, untouched by this diff) still advertise support for it. The SDK mirrors these flags in ClientTasksCapabilitySchema (schemas.ts:349-365) and gates on them at runtime in assertClientRequestTaskCapability (experimental/tasks/helpers.ts:78-96), so after the InputRequiredResult redesign those schema fields and switch arms become unreachable. Should be raised upstream so schema.ts drops tasks.requests.{sampling,elicitation}, after which the SDK can prune the dead capability schema and helper branches.
Extended reasoning...
What the bug is
This sync removes the mechanism for task-augmented sampling/elicitation but leaves the capability advertisement for it in place. Concretely, the diff drops extends TaskAugmentedRequestParams from CreateMessageRequestParams (line 2245), ElicitRequestFormParams (2883) and ElicitRequestURLParams (2916), and drops extends JSONRPCRequest from CreateMessageRequest/ElicitRequest/ListRootsRequest — sampling and elicitation are no longer wire-level requests at all, and can no longer carry a params.task field. They now exist solely as payloads embedded inside InputRequiredResult.inputRequests (per comment #1).
But ClientCapabilities.tasks.requests at spec.types.ts:628-647 — not touched by this diff — still defines:
requests?: {
sampling?: {
/** Whether the client supports task-augmented `sampling/createMessage` requests. */
createMessage?: JSONObject;
};
elicitation?: {
/** Whether the client supports task-augmented elicitation/create requests. */
create?: JSONObject;
};
};These two are the only members of tasks.requests, and their JSDoc explicitly references "task-augmented sampling/elicitation requests" — a flow this same upstream commit just deleted. The spec is now internally inconsistent: it advertises a capability for a mechanism it no longer defines.
Why this isn't just dead prose — concrete SDK surface affected
The SDK mirrors and acts on these flags:
ClientTasksCapabilitySchema(schemas.ts:349-365) definesrequests.sampling.createMessage/requests.elicitation.createas Zod schema fields.assertClientRequestTaskCapability(experimental/tasks/helpers.ts:69-97) switches onmethod, with arms for'sampling/createMessage'(checksrequests.sampling?.createMessage, line 79-87) and'elicitation/create'(checksrequests.elicitation?.create, line 89-96), throwingSdkErrorCode.CapabilityNotSupportedif absent.- That helper is called from
server.ts:414andclient.ts:709, and re-exported from the public surface (exports/public/index.ts:122).
After the InputRequiredResult redesign, no caller can ever reach those switch arms with a task-augmented request — CreateMessageRequest/ElicitRequest are no longer JSON-RPC requests, so the SDK will never dispatch them through the path that calls assertClientRequestTaskCapability. The capability flags, the schema fields, and the two switch arms all become unreachable protocol surface.
Step-by-step proof
- Before this sync:
CreateMessageRequestParams extends TaskAugmentedRequestParams→ a server could send{ method: 'sampling/createMessage', params: { task: {...}, messages: [...] } }as a wire request. The client advertises support viacapabilities.tasks.requests.sampling.createMessage = {}, andassertClientRequestTaskCapability(caps.tasks?.requests, 'sampling/createMessage', ...)validates it before sending. - After this sync:
CreateMessageRequestParamshas noextendsclause (line 2245) → notaskfield.CreateMessageRequesthas noextends JSONRPCRequest(line 2314) → not a wire request. It is now only reachable as a value insideInputRequests(line 348:InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest), which is embedded inInputRequiredResult.inputRequests. - Yet
ClientCapabilities.tasks.requests.sampling.createMessage(line 636) is unchanged. There is no longer any request shape in the spec for which this flag could meaningfully gate behaviour. - Therefore
assertClientRequestTaskCapability'scase 'sampling/createMessage'andcase 'elicitation/create'arms (helpers.ts:79-96) can never be reached by a spec-compliant flow, andClientTasksCapabilitySchema.requests(schemas.ts:349-365) validates fields the protocol no longer uses.
Why this is distinct from the existing comments
Comment #1 covers the SDK's request/result-handler divergence (ServerRequestSchema/ClientResultSchema, server.createMessage()/elicitInput() APIs) — i.e., the SDK still treating these as wire requests. This finding is about the spec being internally inconsistent with itself: the same upstream commit that removed task-augmented sampling/elicitation left the capability advertisement for it in ClientCapabilities. None of comments #1-#6 mention ClientCapabilities.tasks.requests or assertClientRequestTaskCapability. It's the same class of upstream-defect feedback as comment #4 (taskId missing) and #6 (discriminant not narrowed): doesn't break CI (all fields optional), but identifies something schema.ts should fix before the SDK encodes the new shape.
Impact
No CI breakage — tasks.requests and its children are all optional, and this diff doesn't touch ClientCapabilities, so the bidirectional assignability check still passes for that type. But it leaves orphaned protocol surface that will confuse implementers (a client setting tasks.requests.sampling.createMessage is now a no-op), and it identifies concrete SDK code that the companion work should prune rather than carry forward.
How to fix
Upstream in schema.ts: drop requests.sampling and requests.elicitation from ClientCapabilities.tasks (which empties tasks.requests entirely — it can be removed too unless other request types are planned). Batch with the resultType? (#3), taskId (#4), JSDoc (#5), and discriminant-narrowing (#6) feedback. Once re-pulled, the SDK companion work can delete ClientTasksCapabilitySchema.shape.requests (schemas.ts:349-365) and the 'sampling/createMessage' / 'elicitation/create' arms of assertClientRequestTaskCapability (helpers.ts:79-96).
| 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.
This PR updates
packages/core/src/types/spec.types.tsfrom the Model Context Protocol specification.Source file: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/8e192a2277251483768fb254fee4fc3b8da3944f/schema/draft/schema.ts
This is an automated update triggered by the nightly cron job.