Skip to content

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

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

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

Conversation

@github-actions

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

Copy link
Copy Markdown
Contributor

This PR updates packages/core-internal/src/types/spec.types.2026-07-28.ts from the Model Context Protocol specification.

Source file: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/93671a3f2bac3bc11b0eb6327c2d029e272b2871/schema/draft/schema.ts

This is an automated update triggered by the nightly cron job.

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

changeset-bot Bot commented May 7, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 751db78

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

This PR includes no changesets

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

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

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

@github-actions github-actions Bot force-pushed the update-spec-types branch from fe613a8 to 5f450ff Compare May 8, 2026 05:01
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch 2 times, most recently from a64a23e to af35bb5 Compare May 10, 2026 05:12
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment on lines +406 to +418
/* Request parameter type that includes input responses and request state.
* These parameters may be included in any client-initiated request.
*/
export interface InputResponseRequestParams extends RequestParams {
/* New field to carry the responses for the server's requests from the
* InputRequiredResult message. For each key in the response's inputRequests
* field, the same key must appear here with the associated response.
*/
inputResponses?: InputResponses;
/* Request state passed back to the server from the client.
*/
requestState?: string;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Nit (upstream): the field/interface comments here use plain /* ... */ instead of /** ... */ JSDoc, so the TS language service / TypeDoc will not surface them — and InputResponseRequestParams ends up with no doc comment and no @category Multi Round-Trip tag at all, unlike every sibling type in this section. Since spec.types.ts is not re-exported from this package the SDK's own docs are unaffected, but it's worth folding into the same upstream schema.ts fix as the resultType issue so the spec repo's generated docs and the forthcoming hand-written schemas.ts mirror get proper descriptions.

Extended reasoning...

What the issue is

In the new Multi Round-Trip block, several comments use plain block-comment syntax /* ... */ instead of JSDoc syntax /** ... */:

  • InputRequiredResult.inputRequests (lines 394-396)
  • InputRequiredResult.requestState (lines 398-402)
  • the interface-level comment on InputResponseRequestParams (lines 406-408)
  • InputResponseRequestParams.inputResponses (lines 410-413)
  • InputResponseRequestParams.requestState (lines 415-416)

Because the opening delimiter is /* (single asterisk) rather than /**, TypeScript's language service and TypeDoc treat these as ordinary comments, not documentation. Additionally, since InputResponseRequestParams has no /** */ block at all, it also has no @category Multi Round-Trip tag — every other exported type in this section (InputRequests, InputResponses, InputRequiredResult, TaskInputResponseRequest, etc.) carries that tag.

Why this is an upstream slip, not intentional

This is not the file's convention. Every surrounding declaration in the same diff hunk uses proper /** ... */ JSDoc: ResultType (148-156), InputRequests (354-362), InputResponses (366-375), InputRequiredResult itself (380-392), TaskInputResponseRequest (2031-2039), TaskInputResponseRequestParams (2046-2056). The pre-existing /* Empty result */ and /* Cancellation */ lines are one-line section dividers, not API documentation, so they are not precedent for multi-line field descriptions. The five blocks above are the only multi-line API descriptions in the file using /* — a clear authoring inconsistency in the upstream commit.

Addressing the "not SDK-public" objection

It is true that spec.types.ts is not part of this SDK's public surface — packages/core/src/types/index.ts re-exports constants/enums/errors/guards/schemas/specTypeSchema/types but not spec.types, and the only importer in the package is test/spec.types.test.ts. So this has zero effect on the typescript-sdk's generated docs or consumer .d.ts, and CLAUDE.md's "JSDoc for public APIs" rule does not directly apply to this file in this repo.

The reason it is still worth a (nit-level) mention is that this file is a verbatim mirror of the spec repo's schema/draft/schema.ts, which is the source for the protocol's own TypeDoc site. In the upstream output, InputResponseRequestParams will render with no description and will be uncategorised (it will not appear under the "Multi Round-Trip" group), and the normative "client must treat this as an opaque blob" guidance on requestState will be invisible in IDE hover for anyone consuming the spec types. The fix lives in the same upstream file that already needs editing for the resultType? optionality issue flagged elsewhere on this PR, so the marginal cost of including it in that upstream report is near zero.

Step-by-step proof

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

How to fix

In upstream schema.ts, change /*/** on the five blocks listed above and add @category Multi Round-Trip to the InputResponseRequestParams doc comment, then re-run pnpm run fetch:spec-types. No change is appropriate in this repo directly (the file header says DO NOT EDIT). This is purely documentation rendering — it does not affect type-checking, the drift guard, or runtime behaviour — hence nit, raised only because an upstream schema.ts fix is already on the table for this sync.

Comment on lines +393 to +404
export interface InputRequiredResult extends Result {
/* Requests issued by the server that must be complete before the
* client can retry the original request.
*/
inputRequests?: InputRequests;
/* Request state to be passed back to the server when the client
* retries the original request.
* Note: The client must treat this as an opaque blob; it must not
* interpret it in any way.
*/
requestState?: string;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Upstream design gap worth flagging alongside the other schema.ts feedback: InputRequiredResult extends Result without redeclaring resultType: 'input_required', and none of the "complete" subtypes (CallToolResult/ReadResourceResult/GetPromptResult/GetTaskPayloadResult) narrow to resultType: 'complete' either — so the new result: CallToolResult | InputRequiredResult unions (lines 1122/1455/1650/2028) are not TypeScript discriminated unions and if (r.resultType === 'input_required') will not narrow. This is orthogonal to the "resultType is required" comment above (fixing one doesn't fix the other) and constrains the SDK from modeling these with z.discriminatedUnion('resultType', ...) while keeping the bidirectional spec↔SDK assignability check green.

Extended reasoning...

What the bug is

The spec introduces ResultType = 'complete' | 'input_required' and adds resultType: ResultType to the base Result interface (line 165), with the JSDoc explicitly stating its purpose is to "allow the client to determine how to parse the result object." It then defines InputRequiredResult extends Result (line 393) and unions it into four response envelopes — CallToolResultResponse.result: CallToolResult | InputRequiredResult (1650), and likewise for ReadResourceResultResponse (1122), GetPromptResultResponse (1455), and GetTaskPayloadResultResponse (2028).

However, InputRequiredResult does not redeclare resultType: 'input_required', and none of CallToolResult / ReadResourceResult / GetPromptResult / GetTaskPayloadResult redeclare resultType: 'complete'. A grep confirms resultType appears exactly once in spec.types.ts — only on the base Result. So every arm of every X | InputRequiredResult union has resultType typed as the full 'complete' | 'input_required', and TypeScript's discriminated-union narrowing does not engage.

Step-by-step proof

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

Why this is distinct from the existing "resultType is required" comment

The comment on line 165 is about resultType being declared required despite @default "complete", which breaks SDK→spec assignability for every result. This finding is about resultType not being narrowed on subtypes, which breaks discriminated-union narrowing within the spec types themselves. They are orthogonal: making resultType optional on Result does not give InputRequiredResult a narrowed discriminant, and adding resultType: 'input_required' to InputRequiredResult does not make the base field optional. Both should be raised upstream together.

Impact on the SDK

The companion schemas.ts work (already requested in another comment on this PR) will need to define InputRequiredResultSchema and the four result: X | InputRequiredResult unions. The natural Zod encoding is z.discriminatedUnion('resultType', [...]), which requires each arm to declare a z.literal(...) discriminant. But if the SDK narrows InputRequiredResultSchema to resultType: z.literal('input_required'), the spec→SDK direction of the bidirectional assignability check in spec.types.test.ts fails: the spec's InputRequiredResult['resultType'] is 'complete' | 'input_required', which is not assignable to the SDK's 'input_required'. So the SDK is forced to either (a) use a non-discriminated z.union and lose narrowing, or (b) narrow anyway and add a carve-out in the bidirectional test — neither is great, and both go away if upstream narrows the discriminant.

At runtime the wire-level discriminator still works (a client can string-compare resultType and cast), so this is a type-ergonomics / SDK-modeling defect rather than a protocol-correctness one.

How to fix

Upstream in schema.ts: add resultType: 'input_required'; to InputRequiredResult, and add resultType?: 'complete'; (or required 'complete', depending on how the optionality question is resolved) to each concrete "complete" result that participates in an | InputRequiredResult union — at minimum CallToolResult, ReadResourceResult, GetPromptResult, GetTaskPayloadResult. Then re-run pnpm run fetch:spec-types. Batch this with the resultType? optionality fix and the other upstream feedback already noted on this PR.

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Upstream design gap to batch with the other schema.ts feedback: InputResponse = CreateMessageResult | ListRootsResult | ElicitResult admits only success payloads, and the InputResponseRequestParams.inputResponses comment says every inputRequests key "must appear here". In the pre-sync model these were full JSON-RPC requests so a client could return a JSONRPCErrorResponse for user-denied sampling / LLM provider error / no roots configured; that error channel is gone with no replacement (ElicitResult has action: 'decline'|'cancel', but CreateMessageResult and ListRootsResult have no refusal field). The SDK redesign called for in the earlier comment will have no spec-defined way to propagate per-request failures and would have to invent an out-of-spec workaround that breaks once upstream adds a real error variant — suggest upstream add an InputResponseError arm (mirroring the JSON-RPC {code, message, data} shape) or relax the must-appear rule.

Extended reasoning...

What the bug is

InputResponse (spec.types.ts:351) is defined as CreateMessageResult | ListRootsResult | ElicitResult — a union of three success payloads only. InputResponses (line 376-378) is { [key: string]: InputResponse }, and the normative-sounding comment on InputResponseRequestParams.inputResponses (lines 410-413) says: "For each key in the response's inputRequests field, the same key must appear here with the associated response." So per the spec text, the client must supply a value for every requested key, and that value must be one of the three success result shapes.

In the pre-sync model these three exchanges were ordinary server→client JSON-RPC requests (CreateMessageRequest extends JSONRPCRequest, etc.), so a client that could not or would not fulfil one returned a JSONRPCErrorResponse carrying { code: number; message: string; data?: unknown }. By stripping extends JSONRPCRequest / extends Result and embedding the exchange inside the InputRequests/InputResponses maps, the spec discarded that error channel without adding a replacement.

Why the existing types don't cover it

  • ElicitResult happens to carry action: 'accept' | 'decline' | 'cancel', so elicitation can encode refusal in-band.
  • CreateMessageResult (line ~2335) is SamplingMessage & { model: string; stopReason?: ... } — required model/role/content, no refusal/error field, no index signature now that extends Result is dropped.
  • ListRootsResult (line ~2826) is bare { roots: Root[] } — same story.

The asymmetry (only ElicitResult has a decline arm) suggests this is an oversight rather than an intentional "refusal-via-abandonment" design — if abandoning the retry were the intended refusal signal, ElicitResult would not need action: 'decline' either.

Step-by-step proof

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

Why this matters for the SDK companion work

Today a client-side setRequestHandler(CreateMessageRequestSchema, …) can throw / reject and the SDK serialises a JSONRPCErrorResponse; the server's await ctx.sample() rejects with a ProtocolError carrying the code and message, and the tool handler can catch it and fall back. The redesign called for in comment #1 must preserve some equivalent of those semantics, but there is no spec-defined wire shape to carry them. Whatever workaround the SDK picks (drop the key, cancel the whole flow, invent an envelope) becomes public API that breaks once upstream adds a real error variant.

This is distinct from comment #1 (about the SDK still issuing these as wire requests), comment #4 (about tasks/input_response lacking taskId), and comment #6 (about the resultType discriminant) — none of those address the missing error arm in the InputResponse payload union itself.

Addressing "all-or-nothing is the intended design"

It is plausible the spec intends atomic fulfilment (client either satisfies every input or abandons). But (a) that loses the error code/message — the server cannot distinguish rate-limit vs. content-policy vs. user-denied, which is a real expressiveness regression vs. the JSON-RPC model; (b) it is contradicted by ElicitResult.action: 'decline'|'cancel' existing at all; and (c) even under that reading, the SDK still has to pick a public error-propagation contract for ctx.sample() rejection in the new flow, and it is better to do so against a spec-defined error variant than to guess.

How to fix

Raise upstream alongside the resultType? / taskId / discriminant fixes: either add an error arm, e.g.

export interface InputResponseError {
    code: number;
    message: string;
    data?: unknown;
}
export type InputResponse = CreateMessageResult | ListRootsResult | ElicitResult | InputResponseError;

(mirroring the removed JSON-RPC Error shape), or relax the "must appear" wording on inputResponses and define omission as refusal. Then re-run pnpm run fetch:spec-types. Don't encode InputResponsesSchema in schemas.ts until this is settled. Filed as a nit because it doesn't independently break CI and abandon-the-flow is a viable (if lossy) fallback — same tier as comments #5/#6.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Upstream spec self-inconsistency to batch with the other schema.ts feedback: RequestParams._meta is now required with three required client-identity keys (io.modelcontextprotocol/{protocolVersion,clientInfo,clientCapabilities}), but RequestParams is still referenced from contexts where that doesn't fit — DiscoverRequest.params? and PaginatedRequest.params? are still optional (so a client can type-validly omit the "Required" handshake metadata entirely, and discovery is circular: you must commit to a protocolVersion to ask which versions are supported); the server→client ListTasksRequest inherits PaginatedRequestParams → RequestParams, forcing a server sending tasks/list with a cursor to populate the client's clientInfo/clientCapabilities; and the server-authored ListRootsRequest (now only an InputRequest payload) kept params?: RequestParams while its siblings CreateMessageRequest/ElicitRequest had their RequestParams inheritance stripped. Suggest upstream make the three keys optional (or split RequestMetaObject by direction), make params non-optional on client wire requests, exempt server/discover, and drop the RequestParams reference from ListRootsRequest.

Extended reasoning...

What the issue is

This sync changes RequestParams (line 145) from _meta?: RequestMetaObject to _meta: RequestMetaObject and adds three required keys to RequestMetaObject (lines 82/89/97): 'io.modelcontextprotocol/protocolVersion', 'io.modelcontextprotocol/clientInfo', and 'io.modelcontextprotocol/clientCapabilities', each with JSDoc explicitly saying "Required." The intent — moving the handshake from a one-time initialize to per-request _meta — is clear, but RequestParams is still used in three places where mandatory client-identity metadata is either omittable, semantically wrong, or circular. This is the spec being internally inconsistent, distinct from comment #3223937258 which only notes that the SDK's RequestParamsSchema doesn't match the new required _meta.

Surface 1 — params? still optional on client wire requests

DiscoverRequest.params?: RequestParams (line 568) and PaginatedRequest.params?: PaginatedRequestParams (line 1014, inherited by ListResourcesRequest / ListResourceTemplatesRequest / ListPromptsRequest / ListToolsRequest / ListTasksRequest) declare params as optional. So { jsonrpc: '2.0', id: 1, method: 'tools/list' } is type-valid yet carries no _meta and therefore none of the "Required" handshake metadata — the spec contradicts itself. Pre-sync this was consistent because _meta was optional; the upstream commit tightened _meta without tightening params.

The DiscoverRequest case is additionally circular. Per its JSDoc, server/discover exists so the client can learn supportedVersions for use in subsequent requests, and per the JSDoc on protocolVersion (line 80) the server MUST return UnsupportedProtocolVersionError if the value is not supported. But if the client supplies params, it must include _meta['io.modelcontextprotocol/protocolVersion'] — i.e., commit to a version before learning which versions are supported. Either DiscoverRequest should not use RequestParams, or protocolVersion should be optional/exempt for discovery; the params? escape hatch is at best implicit and contradicts the unconditional "Required." prose.

Surface 2 — server→client wire request inherits client-direction keys

ServerRequest (line 3280) = GetTaskRequest | GetTaskPayloadRequest | ListTasksRequest | CancelTaskRequest. Three of these use inline params: { taskId: string } and escape, but ListTasksRequest (line 2155) extends PaginatedRequestparams?: PaginatedRequestParams (line 1004) extends RequestParams_meta: RequestMetaObject with required clientInfo/clientCapabilities. The JSDoc on those keys is unambiguously client→server ("Identifies the client software making the request", "The client's capabilities for this specific request", "Servers MUST NOT infer capabilities from prior requests"). So per the type, a server sending tasks/list with a pagination cursor must fabricate the client's identity and capabilities — semantically nonsensical. Concrete SDK consequence: when the companion work adds per-request _meta injection to Protocol.request(), the server-side outbound path has no sensible value to put here.

Surface 3 — server-authored embedded ListRootsRequest payload

ListRootsRequest (line 2820) dropped extends JSONRPCRequest (it is now only an InputRequest payload embedded in server-emitted InputRequiredResult.inputRequests, per InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest at line 438), but unlike its two siblings it kept params?: RequestParams (line 2822). The siblings were cleaned: CreateMessageRequestParams (line 2257) and ElicitRequestFormParams/ElicitRequestURLParams (lines 2880/2913) all dropped extends TaskAugmentedRequestParams, so they no longer reference RequestParams/_meta at all. ListRootsRequest was missed — likely because it had no dedicated *Params wrapper to edit. params? is optional so a server can omit it, but a server wanting to attach e.g. _meta.progressToken would be type-forced to also fabricate clientInfo/clientCapabilities/protocolVersion for a server-authored embedded payload.

Step-by-step proof

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

Impact

None of these independently break CI beyond what comment #3223937258 already documents (the SDK↔spec RequestParams assignability failure). They are upstream design gaps in the same tier as comments #3214351591 (taskId missing), #3214351594 (discriminant not narrowed), #3216586352 (tasks.requests orphaned), and #3216586357 (InputResponse no error arm) — they don't add new red CI, but they identify shapes the SDK should not encode as-is. In particular, the server-side Protocol.request() _meta-injection redesign called for in #3223937258 cannot follow the same logic as the client side while RequestMetaObject is client-direction-only.

How to fix

Raise upstream alongside the other schema.ts feedback. Plausible fixes (any one resolves all three surfaces, or they can be combined):

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Partially superseded by the latest re-pull (c47bd846), which deletes the entire tasks subsystem.

Surface 2 is now moot: ListTasksRequest and ServerRequest no longer exist (the spec defines no server→client requests at all), so there is no longer a server→client wire request inheriting the client-direction _meta keys via PaginatedRequestParams → RequestParams.

Surfaces 1 and 3 remain fully applicable:

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

The remaining upstream feedback to batch: make the three keys optional (or split RequestMetaObject by direction), make params non-optional on client wire requests / exempt server/discover, and drop the RequestParams reference from ListRootsRequest.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Updated against the latest re-pull (bbcfc6) — the prior follow-up is now partially stale.

params? optionality (Surface 1, sub-issue) — now resolved upstream: DiscoverRequest.params (line 558) and PaginatedRequest.params (line 941) are now declared required (no ?), so a client can no longer type-validly omit the handshake _meta on those wire requests. This was one of the suggested fixes ("make params non-optional on client wire requests").

Discover circularity (Surface 1, sub-issue) — still applicable, arguably tightened: with DiscoverRequest.params: RequestParams now required, the client must populate _meta['io.modelcontextprotocol/protocolVersion'] to call server/discover at all — i.e. it must commit to a version before learning which versions are supported. The escape hatch is that the server responds with UnsupportedProtocolVersionError.data.supported (now a structured array), so a client can recover by trying a guess and reading the 400 body. Whether that should require an explicit exemption for server/discover remains an upstream design question.

ListRootsRequest (Surface 3) — still applicable: ListRootsRequest (line 2459) still keeps params?: RequestParams while its InputRequest siblings CreateMessageRequest/ElicitRequest had their RequestParams inheritance stripped — still the odd one out among server-authored embedded payloads.

Surface 2 — moot (unchanged from prior follow-up; ListTasksRequest/ServerRequest are gone).

Remaining upstream feedback to batch: exempt server/discover from the protocolVersion requirement (or document the try-and-recover-via-data.supported pattern as the intended bootstrap), and drop the RequestParams reference from ListRootsRequest.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Updated against the latest re-pull (91b403f8) — most of this comment is now addressed upstream; one sub-issue remains.

ListRootsRequest (Surface 3) — now resolved upstream: ListRootsRequest.params (line 2598-2603) has been changed from params?: RequestParams to a bare params?: { _meta?: MetaObject } — exactly the fix suggested in the original comment ("Change ListRootsRequest.params? to a bare { _meta?: MetaObject } … matching CreateMessageRequest/ElicitRequest"). The server-authored embedded payload no longer references the client-direction RequestParams/RequestMetaObject.

params? optionality (Surface 1, sub-issue) — remains resolved (unchanged from the prior follow-up): DiscoverRequest.params (line 612) and PaginatedRequest.params are required, so the handshake _meta cannot be type-validly omitted on client wire requests.

Discover circularity (Surface 1, sub-issue) — still applicable, now partially mitigated by prose: RequestParams._meta (line 156) still requires the three io.modelcontextprotocol/* keys (lines 83/90/98), and DiscoverRequest.params: RequestParams is required, so a client must still commit to a protocolVersion to call server/discover. The new DiscoverRequest JSDoc (lines 600-603) does soften this — "Clients MAY call it but are not required to — version negotiation can also happen inline via per-request _meta" — and UnsupportedProtocolVersionError.data.supported provides the try-and-recover path, but there is still no explicit exemption from the protocolVersion rejection rule for server/discover itself.

Surface 2 — moot (unchanged; ListTasksRequest/ServerRequest are gone).

Remaining upstream feedback to batch is now narrow: either exempt server/discover from the protocolVersion requirement or document the try-a-version-and-recover-via-data.supported pattern as the intended bootstrap.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 29aa626 to 07e2103 Compare May 14, 2026 05:17
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
* Capabilities that a server may support. Known capabilities are defined here, in this schema, but this is not a closed set: any server can define its own, additional capabilities.
*
* @category `initialize`
* @category `server/discover`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 🟡 Upstream spec self-inconsistency to batch with the other schema.ts feedback (server-side analogue of comment #3216586352): the same diff that deletes SubscribeRequest/UnsubscribeRequest and makes all list-changed notifications opt-in via SubscriptionFilter leaves ServerCapabilities.resources.subscribe (line 776) and {prompts,resources,tools}.listChanged (755/780/795) untouched, and adds no capability flag for subscriptions/listen itself. The flags are not dead — each maps 1:1 to a SubscriptionFilter field — but their documented semantics have silently shifted from "server will push unsolicited *_list_changed" / "server accepts the resources/subscribe RPC" to "server will honor the corresponding SubscriptionFilter opt-in on a subscriptions/listen stream", and the JSDoc/@example references still describe the old model. Suggest upstream either re-document these four flags for the opt-in model or fold them into a structured subscriptions: { listen?, resourceSubscriptions?, …ListChanged? } capability so the SDK's subscriptions/listen client API (replacing the client.ts:599 gate) has an unambiguous flag to check.

Extended reasoning...

What the issue is

This sync replaces the per-resource resources/subscribe / resources/unsubscribe RPCs and unsolicited *_list_changed pushes with a single opt-in subscriptions/listen stream: the client sends a SubscriptionFilter naming which notification types it wants, and the server echoes back what it will honor in SubscriptionsAcknowledgedNotification.params.notifications (the JSDoc on SubscriptionFilter says the server "MUST NOT send notification types the client has not explicitly requested"). But ServerCapabilities — structurally untouched by this diff beyond the @category initialize → server/discover rename at line 721 — still declares:

  • resources.subscribe?: boolean (line 776) — "Whether this server supports subscribing to resource updates"
  • prompts.listChanged?: boolean / resources.listChanged?: boolean / tools.listChanged?: boolean (lines 755/780/795) — "Whether this server supports notifications for changes to the … list"

and adds no field advertising whether the server implements subscriptions/listen at all.

Addressing the "these flags map cleanly to SubscriptionFilter" objection

A reasonable counter-read is that these four flags are not orphaned: each corresponds 1:1 to a SubscriptionFilter field (resources.subscriberesourceSubscriptions, {prompts,resources,tools}.listChanged{prompts,resources,tools}ListChanged), the underlying notifications (ResourceUpdatedNotification, the three *ListChangedNotification types) all still exist in ServerNotification, and the JSDoc text on resources.subscribe ("subscribing to resource updates") is mechanism-agnostic. Under this reading, the static DiscoverResult.capabilities flags tell a client what to put in its SubscriptionFilter before opening a stream, and SubscriptionsAcknowledgedNotification confirms per-stream what was honored — two different purposes, not duplication. That counter-read is largely correct, and is why this is not the same as comment #3216586352's ClientCapabilities.tasks.requests case (where the advertised concept — task-augmented sampling/elicitation wire requests — was deleted entirely).

What that reading does not address is that the semantics of these flags changed without the spec saying so. Pre-sync, tools.listChanged: true meant "this server will send unsolicited notifications/tools/list_changed"; post-sync, per the SubscriptionFilter JSDoc, the server MUST NOT send that notification unless the client opts in on a subscriptions/listen stream — so the flag now means "this server will honor toolsListChanged: true in your filter." Pre-sync, resources.subscribe: true meant "this server accepts resources/subscribe requests" (and client.ts:599 gates on exactly that); post-sync, that RPC does not exist, so the flag must mean "this server will honor resourceSubscriptions: [...] in your filter." Neither shift is reflected in the JSDoc, and the @example {@includeCode ./examples/ServerCapabilities/resources-subscription-*.json} references (lines 763-770) still point at example files written for the old model. An implementer reading ServerCapabilities in isolation would reasonably conclude the server pushes list-changed notifications unprompted and accepts a resources/subscribe RPC — both now wrong.

Separately, there is no explicit capability for subscriptions/listen itself. Under the reinterpretation above, support is implied by any of the four sub-flags being present — but that is undocumented inference, and a server that implements subscriptions/listen for (say) LoggingMessageNotification only, with none of the four sub-flags, has no way to advertise the endpoint exists.

Step-by-step proof

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

Why this is distinct from existing PR comments

Comment #3216586352 covers the client-side mirror (ClientCapabilities.tasks.requests orphaned by InputRequiredResult). Comment #3223937258 covers the SDK lifecycle redesign, including replacing subscribeResource()/unsubscribeResource() — but that comment is about what the SDK must change, not about the upstream schema.ts leaving ServerCapabilities documenting the old subscription model. Comment #3231781722 covers SubscriptionsListenRequest lacking a JSON-RPC response wrapper. None of #1-#13 mention ServerCapabilities.resources.subscribe / *.listChanged or a missing subscriptions capability.

Impact

No independent CI breakage — all four fields are optional, the ServerCapabilities body is untouched, so spec.types.test.ts's bidirectional check for ServerCapabilities still passes. This is upstream documentation/modeling feedback in the same tier as #3214351593 (JSDoc), #3216586352 (tasks.requests), #3231781731 (RequestParams direction). Filed as nit.

How to fix

Raise upstream alongside the other schema.ts feedback. Two coherent options:

  • Re-document in place: keep the four flags, update their JSDoc to reference SubscriptionFilter (e.g., resources.subscribe → "Whether this server will honor resourceSubscriptions on a subscriptions/listen stream"; *.listChanged → "Whether this server will honor *ListChanged on a subscriptions/listen stream"), and refresh the @example JSON files. Optionally add a top-level subscriptions?: JSONObject to advertise the endpoint independently of which filter fields are honored.
  • Restructure: replace the four scattered flags with subscriptions?: { listen?: JSONObject; resourceSubscriptions?: boolean; toolsListChanged?: boolean; promptsListChanged?: boolean; resourcesListChanged?: boolean } so the capability shape mirrors SubscriptionFilter.

Then re-run pnpm run fetch:spec-types. Once re-pulled, ServerCapabilitiesSchema in schemas.ts and the client.ts capability gate for the new subscriptions/listen API can follow whichever shape upstream picks.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 07e2103 to 0bcec2b Compare May 15, 2026 05:18
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts Outdated
Comment thread packages/core/src/types/spec.types.ts Outdated
@github-actions github-actions Bot force-pushed the update-spec-types branch from 0bcec2b to 57ccae0 Compare May 16, 2026 05:05
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch 2 times, most recently from e9de0a8 to 786d94c Compare May 18, 2026 05:23
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 786d94c to edb9e09 Compare May 19, 2026 05:20
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from edb9e09 to 1896892 Compare May 20, 2026 05:21
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 1896892 to b255ba4 Compare May 21, 2026 05:24
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core/src/types/spec.types.ts Outdated
@github-actions github-actions Bot force-pushed the update-spec-types branch from b255ba4 to da9d82e Compare May 22, 2026 05:20
Comment thread packages/core/src/types/spec.types.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from 20baa74 to 234b0fc Compare June 20, 2026 05:29
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch 3 times, most recently from 2b8cddf to ee274f4 Compare June 23, 2026 05:20
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
@github-actions github-actions Bot force-pushed the update-spec-types branch from ee274f4 to 4df8d55 Compare June 24, 2026 05:19
Comment thread packages/core-internal/src/types/spec.types.2026-07-28.ts
Comment on lines +1303 to +1318

/**
* The response to a {@link SubscriptionsListenRequest | subscriptions/listen}
* request, signalling that the subscription has ended gracefully (for example,
* during server shutdown). Because the listen stream is long-lived, this result
* is sent only when the server tears the subscription down; an abrupt transport
* close carries no response. The result body is otherwise empty.
*
* @example Subscription closed gracefully
* {@includeCode ./examples/SubscriptionsListenResult/listen-closed.json}
*
* @category `subscriptions/listen`
*/
export interface SubscriptionsListenResult extends Result {
_meta: SubscriptionsListenResultMeta;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Upstream schema.ts underspecification to batch with the other accepted spec feedback: the new SubscriptionsListenResult JSDoc (lines 1304-1318) says, transport-unqualified, that this result "is sent only when the server tears the subscription down; an abrupt transport close carries no response" — but the CancelledNotification JSDoc (line 602, from the previous re-pull) says that on stdio the server terminates a subscriptions/listen stream by sending notifications/cancelled referencing the listen request's ID, and the spec never states how the two compose on stdio (cancellation only, result only, or both and in what order). The companion subscriptions/listen port must not bake either guess into wire behavior: the client side needs to know which signal settles the pending Protocol.request() listen request (cancelled-only requires the #3445564194 _oncancel routing fix; result-only makes the line-602 stdio sentence dead prose), and the server side needs to know what to emit on stdio teardown — raise upstream so the termination contract is stated explicitly before the port encodes it.

Extended reasoning...

What the issue is

This re-pull (2ffc3fa) adds SubscriptionsListenResult (spec.types.2026-07-28.ts:1303-1318), whose JSDoc is unqualified by transport: "The response to a subscriptions/listen request, signalling that the subscription has ended gracefully (for example, during server shutdown). Because the listen stream is long-lived, this result is sent only when the server tears the subscription down; an abrupt transport close carries no response."

But an earlier force-push of this same PR (91b403f) had already added a different server-side termination mechanism for the same stream, which this re-pull leaves untouched: the CancelledNotification JSDoc (line 602) says "On stdio, the server also sends this notification, solely to terminate a subscriptions/listen stream: it references the ID of the subscriptions/listen request that opened the stream. Servers MUST NOT use this notification to cancel any other request." The general CancelledNotification JSDoc also still says "the result will be unused, so any associated processing SHOULD cease" — which pulls directly against a result following the cancellation.

The two passages now both claim to be the way the server ends a listen stream, and the spec never says how they compose on stdio, where there is no per-request HTTP stream to close:

  • Cancellation only: the listen request never receives a JSON-RPC response — directly contradicting the new result's "sent only when the server tears the subscription down" (server-initiated stdio teardown is the server tearing the subscription down), and re-creating the pending-request problem the result type was added to solve.
  • Cancellation then result: an unusual JSON-RPC contract (the receiver of a request cancels it and then still responds to it) that the spec nowhere states — and that the surviving "the result will be unused" sentence argues against. An implementer reading only the result JSDoc would not emit the cancellation; one reading only the cancelled JSDoc would not emit the result.
  • Result only: the stdio sentence at line 602 becomes dead prose left over from the revision that predates the result type.

This is the same partial-migration class already accepted repeatedly on this PR (#3246176905, #3368827092): the result type was added in the latest upstream commit without reconciling the stdio-cancellation sentence introduced by the previous one.

Step-by-step proof

  1. An SDK client on stdio opens the notification stream by issuing subscriptions/listen (request ID 7); the ID lands in Protocol's _responseHandlers.
  2. The server later shuts down gracefully and wants to tear the subscription down. Per line 602 it sends { method: 'notifications/cancelled', params: { requestId: 7 } }; per lines 1304-1309 it sends { id: 7, result: { resultType: 'complete', _meta: { 'io.modelcontextprotocol/subscriptionId': 7 } } }.
  3. The spec gives no rule for which of these (or both, in which order) the server emits on stdio. All three behaviors are equally "compliant" and mutually incompatible on the wire.
  4. A client that follows the result JSDoc waits for the response; against a cancellation-only server the listen request hangs in _responseHandlers until a misleading RequestTimeout (and per #3445564194, Protocol._oncancel only consults _requestHandlerAbortControllers, so the cancellation is silently dropped). A client that follows the cancelled JSDoc and tears down on the notification may then receive an unsolicited { id: 7, result: ... } it no longer has a handler for.

Why existing comments don't cover it

  • The companion bug filed on this run for SubscriptionsListenResult/SubscriptionsListenResultMeta covers only the drift-guard/schema/export-count gap and the request-timeout interaction — not the stdio cancellation mechanism or the termination-contract ambiguity.
  • Prior comment #3445564194 covers only the Protocol._oncancel routing gap for the cancelled path and was written before SubscriptionsListenResult existed; it assumes the cancellation is the sole termination signal.
  • Prior comment #3231781722 (no result wrapper at all) is superseded by the result's addition and does not discuss the cancelled mechanism.
  • Status update #3433356219 records the stdio-cancellation scoping as the resolution of the ServerNotification union question; it does not mention the result type (which did not exist yet).

Concrete SDK consequence (why this matters for the companion port)

The companion subscriptions/listen work (already mandated by #3223937253/#3239359153) has to pick a wire behavior on both sides:

  • Client side: which signal settles the pending listen request created by Protocol.request()? The SubscriptionsListenResult is handled natively by Protocol's response path; a server-sent notifications/cancelled naming the client's own outgoing request is NOT (the #3445564194 gap). If the spec's answer is "cancelled only on stdio", the #3445564194 routing fix is mandatory and the result schema is never exercised on stdio; if the answer is "result always", the client can ignore server-sent cancellations entirely. Different code either way.
  • Server side: on stdio teardown, must the SDK server emit the cancellation, the result, or both, and in which order? Whatever it encodes becomes wire-visible behavior that a strict 2026-07-28 client may or may not expect.

How to fix

No change to this automated sync itself. Raise upstream with the other batched schema.ts feedback: state explicitly how server-initiated termination of a subscriptions/listen stream works on stdio — e.g. "the server sends notifications/cancelled and MUST then send the SubscriptionsListenResult to complete the request", or drop the stdio cancellation sentence at line 602 now that a graceful-close response exists, or scope the cancellation to abrupt/non-graceful termination and say the result is omitted in that case. Until settled, the companion port should not bake either guess into the public client/server wire behavior.

@github-actions github-actions Bot force-pushed the update-spec-types branch 2 times, most recently from 9fb2071 to 613b17e Compare June 27, 2026 05:17
* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 This sync bumps the anchor's provenance header to upstream commit 2ffc3fa2, but the derived twins that the repo's OPERATIVE lifecycle rule 4 requires to be refreshed in the same commit still pin the old commit f68d864a: packages/core-internal/test/corpus/schema-twins/manifest.json and test/corpus/fixtures/2026-07-28/manifest.json. Please run pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 (and pnpm fetch:spec-examples if upstream examples changed) in this PR so both manifests record the same commit as the anchor — and longer-term, the nightly cron workflow should regenerate the twins alongside the anchor, since it currently only runs fetch:spec-types and only git adds the anchor file.

Extended reasoning...

What the bug is

This automated sync updates the 2026-07-28 anchor's provenance header to upstream commit 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 (packages/core-internal/src/types/spec.types.2026-07-28.ts:6), but it does not refresh the derived artifacts that the repo's own lifecycle policy ties to that anchor. packages/core-internal/src/types/README.md lifecycle rule 4 — explicitly marked OPERATIVE — states: "any refresh that changes the anchor must regenerate those artifacts in the same commit. The anchor and its derived twins must never be out of sync at any commit on main." It then names the governed artifacts: the vendored schema.json twins under packages/core-internal/test/corpus/schema-twins/ (whose manifest.json records the source commit and content hashes), and the spec example corpus manifest (test/corpus/fixtures/<revision>/manifest.json), which "follows the same atomicity rule when the examples are re-vendored."

The code path / state that triggers it

After this PR merges, the repository on main is in exactly the state rule 4 forbids:

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

Why existing checks don't prevent it

This divergence lands silently. schemaTwinConformance.test.ts only hashes the vendored twin bytes against the manifest's own sha256/bytes entries and validates fixtures against the twin — it never cross-checks the manifest's source.commit against the anchor header's commit. Because the anchor's TypeScript body is byte-identical in this diff (only the provenance line changed), the content lock stays green and CI gives no signal. The README anticipates a content mismatch being "the desired loud signal of a missed twin update", but a header-only bump produces a provenance-only mismatch that nothing detects.

The root cause is structural: .github/workflows/update-spec-types.yml only runs pnpm run fetch:spec-types 2026-07-28 (line 48) and only git adds the anchor file (line 73). fetch:schema-twins and fetch:spec-examples are separate scripts that the cron job never invokes, so every nightly sync that bumps the anchor will recreate this violation.

Step-by-step proof

  1. Merge this PR as-is.
  2. On main, run grep 'Last updated from commit' packages/core-internal/src/types/spec.types.2026-07-28.ts2ffc3fa2….
  3. Run grep '\"commit\"' packages/core-internal/test/corpus/schema-twins/manifest.json packages/core-internal/test/corpus/fixtures/2026-07-28/manifest.json → both say f68d864a….
  4. This is the out-of-sync state rule 4 says "must never" exist at any commit on main, and the schema-twins manifest's "ATOMICALLY with the matching spec.types anchor" instruction has been violated.
  5. pnpm vitest run test/wire/schemaTwinConformance.test.ts stays green, so nothing flags it.

Impact

Beyond the policy violation itself, the practical risk is that the vendored test oracles silently go stale: if upstream's generated schema.json or examples changed between f68d864a and 2ffc3fa2 (descriptions, required lists, examples), the conformance suites would be validating against an older oracle while the anchor claims a newer commit, and the manifests would be lying about their provenance. (Mitigating fact: the anchor's TS body is byte-identical here, so the twin content is likely unchanged — but the only way to know, and the thing the policy demands, is to regenerate and re-pin in the same commit.)

How to fix

In this PR: run pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 (and pnpm fetch:spec-examples if upstream examples changed) so the twin and both manifests record the same commit as the anchor — or, if the twins are intentionally being left at f68d864a, document why in the PR. Longer-term, update .github/workflows/update-spec-types.yml to run the twin/example fetch scripts and git add their outputs alongside the anchor, so the nightly sync can satisfy rule 4 on its own; optionally add a check in schemaTwinConformance.test.ts that compares the manifest's source.commit to the anchor header so a future provenance-only divergence fails loudly.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 613b17e to 6d8fc68 Compare June 28, 2026 05:30
* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 This sync is a verified content-noop — upstream schema/draft/schema.ts and schema.json are byte-identical between f68d864a and 2ffc3fa2, and the vendored twin's sha256 already matches upstream at the new commit — so the only effective change is the provenance line, yet it still re-creates the rule-4 anchor↔manifest skew flagged in #3485357818. Worse, following that comment's prescribed remediation as written misfires: fetch-schema-twins.ts applies one SHA to every twin and would re-fetch/re-stamp the frozen 2025-11-25 released-revision oracle from the floating draft commit (contrary to README rule 1 / RELEASED_REVISION_PINS), and fetch-spec-examples.ts run without a SHA defaults to the latest commit touching schema/draft/examples — a different path filter than the anchor — silently re-creating the provenance disagreement. Suggested fixes: make the workflow's drift check ignore the provenance header (or skip the write when the regenerated body is unchanged), give the twin/example fetch scripts per-revision pinning / anchor-aligned defaults, and for this PR either close it as a no-op or land it together with a 2026-07-28-only twin/fixtures refresh.

Extended reasoning...

This PR is a verified content-noop; only the provenance line moves

Both files the anchor mirrors are byte-identical between the previously recorded commit and the newly recorded one: fetching schema/draft/schema.ts from raw.githubusercontent.com at f68d864a813754e188c6df52dcc5772a12f96c63 and at 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 yields two 96,322-byte files with an empty diff, and schema/draft/schema.json at both commits is 178,613 bytes with sha256 14398c3d…ba5c4c83 — exactly the hash already recorded for the 2026-07-28 twin in packages/core-internal/test/corpus/schema-twins/manifest.json. Since scripts/fetch-spec-types.ts always regenerates the whole anchor from the fetched bytes, a byte-identical upstream file can only ever produce a header-only diff, which is exactly what this PR contains. The vendored conformance oracle is therefore not stale in content; the only divergence this PR creates is provenance metadata — yet merging it alone still leaves main in the rule-4-violating state already accepted as #3485357818 (anchor says 2ffc3fa2, both manifests still say f68d864a), for zero spec content change.

Why this churn will recur: the workflow's drift check counts header-only rewrites as drift

.github/workflows/update-spec-types.yml's Check for changes step runs git diff --quiet packages/core-internal/src/types/spec.types.2026-07-28.ts on the whole file, and fetchLatestSHA() in scripts/fetch-spec-types.ts (lines 51-65) picks the newest commit that touched schema/draft/schema.ts regardless of whether the content changed. Step-by-step: (1) upstream lands a content-neutral touch of the path (e.g. a change later reverted, with the revert being the newest path-touching commit); (2) the nightly job regenerates the anchor — the body is unchanged but the header SHA moves; (3) git diff --quiet sees the header line and reports drift; (4) a refresh PR like this one is opened, and per rule 4 it now demands a twin/fixtures refresh (or stands as a violation) despite mirroring identical bytes. This will repeat on every future content-neutral upstream touch.

The prescribed remediation in #3485357818 also misfires as written

That comment instructs running pnpm fetch:schema-twins 2ffc3fa2… (and pnpm fetch:spec-examples with no SHA). Two problems:

  1. scripts/fetch-schema-twins.ts has no per-revision pinning. Line 53 takes a single SHA (process.argv[2] ?? manifest.source.commit), the loop at lines 55-63 iterates every entry in manifest.files — both 2026-07-28schema/draft/schema.json and 2025-11-25schema/2025-11-25/schema.json — re-fetching each at that one SHA and rewriting bytes/sha256, and line 65 overwrites the single shared manifest.source.commit. Running the prescribed command therefore re-vendors and re-stamps the frozen 2025-11-25 released-revision oracle from the floating draft commit — exactly what packages/core-internal/src/types/README.md rule 1 and RELEASED_REVISION_PINS in fetch-spec-types.ts (which pins 2025-11-25 at 0168c57f…) forbid. The single source.commit field structurally cannot represent the two lifecycles: today it already disagrees with both the 2025-11-25 anchor header (357adac4) and the released pin (0168c57f). Wiring the script into the nightly cron, as the comment also suggests, would institutionalize this every night.

  2. scripts/fetch-spec-examples.ts run without a SHA stamps the wrong commit. Line 142 is const sha = args[0] ?? (await fetchLatestSHA()), and that fallback queries the GitHub commits API filtered by schema/draft/examples — a different path filter than the anchor's schema/draft/schema.ts. The resulting commit is in general not 2ffc3fa2 (it can be newer or older), and writeCorpus() stamps it into test/corpus/fixtures/2026-07-28/manifest.json source.commit — silently re-creating the very anchor↔derived-artifact provenance disagreement rule 4 forbids, just between the anchor and the example corpus instead of the schema twins. No CI catches it: specCorpus.test.ts only checks the revision string, directory listing, file count, and the 87/128 inventory pins, never source.commit vs the anchor header; schemaTwinConformance.test.ts hashes only the twin files.

How to fix

  • Workflow / fetch script: make the drift check ignore the provenance header (e.g. git diff -I 'Last updated from commit:' --quiet …, git ≥ 2.30), or have fetch-spec-types.ts compare the regenerated body (excluding the header) with the on-disk body and skip the write — keeping the previously recorded SHA — when they are identical, so the recorded provenance only moves when mirrored content actually changes.
  • fetch-schema-twins.ts: accept a revision argument and/or mirror RELEASED_REVISION_PINS so the 2025-11-25 entry is only ever fetched at its pinned commit, and record provenance per file (a commit field inside each manifest.files entry) instead of one shared source.commit.
  • fetch-spec-examples.ts: pass the anchor commit explicitly (pnpm fetch:spec-examples 2ffc3fa2…) in the same commit that refreshes the twins; longer term, default its SHA to the anchor header's Last updated from commit rather than the latest examples-path commit, and have the nightly workflow drive all three fetch scripts with one shared SHA. Optionally extend specCorpus.test.ts to compare manifest.source.commit against the anchor header so a future provenance-only skew fails loudly.
  • For this PR specifically: since the spec content is unchanged and the vendored twin hash already matches upstream at 2ffc3fa2, the lowest-cost resolution is to close it as a no-op, or land it together with the 2026-07-28-only twin/fixtures manifest refresh demanded by #3485357818 — merging the header bump alone buys nothing and leaves main in the rule-4-violating state.

@github-actions github-actions Bot force-pushed the update-spec-types branch 6 times, most recently from 3664505 to f4f316e Compare July 5, 2026 05:19
* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The two prior comments covering this exact situation (#3485357818's rule-4 remediation and #3487174588's content-noop verification) were written against the previous re-pull (2ffc3fa2), but this PR now records upstream commit 93671a3f in the anchor header — so following the earlier comment's command verbatim (pnpm fetch:schema-twins 2ffc3fa2...) would stamp the manifests with a third commit and recreate the rule-4 anchor↔manifest provenance skew. Any twin/fixtures manifest refresh landed with this PR must pin 93671a3f (or whatever the anchor records at merge time) instead; re-verified against the new head, the upstream delta between f68d864a and 93671a3f is formatting-only (schema.json hash unchanged, no examples touched), so the close-as-no-op / land-with-manifest-refresh options from the prior comments remain valid.

Extended reasoning...

What this corrects

This re-pull moves the anchor's provenance header (packages/core-internal/src/types/spec.types.2026-07-28.ts:6) from f68d864a813754e188c6df52dcc5772a12f96c63 to 93671a3f2bac3bc11b0eb6327c2d029e272b2871. The two most recent review comments covering exactly this situation were written against the previous re-pull (2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184) and are now stale in ways that matter for whoever acts on them:

  1. #3485357818 (rule-4 anchor↔manifest skew) instructs: "run pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184 … so both manifests record the same commit as the anchor." Following that command verbatim against the current head would stamp packages/core-internal/test/corpus/schema-twins/manifest.json (and, via fetch:spec-examples, test/corpus/fixtures/2026-07-28/manifest.json) with 2ffc3fa2 while the anchor header says 93671a3f — recreating the very anchor↔derived-artifact provenance disagreement that lifecycle rule 4 (packages/core-internal/src/types/README.md, marked OPERATIVE) forbids, just at a different pair of commits.
  2. #3487174588 (verified content-noop) based its close-as-no-op recommendation on upstream schema/draft/schema.ts and schema.json being byte-identical between f68d864a and 2ffc3fa2. That specific verification does not automatically carry over to the new head.

Re-verification against the new head (93671a3f)

  • Between f68d864a and 93671a3f, upstream schema/draft/schema.ts does change (96,322 → 96,256 bytes, 63 diff lines) — but every changed hunk is pure union-type re-wrapping (multi-line | A | B | C; collapsed to a single line) with zero semantic or JSDoc change. Prettier in scripts/fetch-spec-types.ts normalizes this away, which is why the regenerated anchor body is byte-identical and this PR's diff is still header-only.
  • schema/draft/schema.json at 93671a3f is unchanged: 178,613 bytes, sha256 14398c3dd2c66b9c3f6661fc7a7eaa24174952ed1598d0b7f011b686ba5c4c83, exactly matching the hash pinned for the 2026-07-28 twin in schema-twins/manifest.json — so the vendored twin oracle remains content-current.
  • The upstream compare f68d864a…93671a3f (16 commits, 27 files) touches no file under schema/draft/examples, so the fixtures corpus at test/corpus/fixtures/2026-07-28/ is also content-current.

Concrete step-by-step consequence of following the stale command

  1. A maintainer decides to satisfy rule 4 by landing a manifest refresh with this PR and copies the command from #3485357818: pnpm fetch:schema-twins 2ffc3fa2d00e637ae7c3d1438aaa0561ddfda184.
  2. Both manifests now record source.commit = 2ffc3fa2.
  3. The anchor header on this PR's head says 93671a3f.
  4. main after merge is again in the state rule 4 forbids — anchor and derived twins disagreeing on their upstream commit — only now across three different SHAs (f68d864a in history, 2ffc3fa2 in the manifests, 93671a3f in the anchor). Nothing in CI catches it: schemaTwinConformance.test.ts hashes only twin bytes and never cross-checks source.commit against the anchor header.

What remains valid, and the recurring-churn point

The prior comments' bottom-line options remain valid for the current head — the content is verified unchanged, so the PR can either be closed as a no-op or landed together with a 2026-07-28-only twin/fixtures manifest refresh, pinned to 93671a3f (or whatever commit the anchor records at merge time), not the SHA quoted in the earlier comment. This is now the second consecutive header-only refresh PR (f68d864a → 2ffc3fa2 → 93671a3f), which reinforces the already-suggested workflow fixes: make the drift check ignore the provenance line (git diff -I 'Last updated from commit:') or have fetch-spec-types.ts skip the write when the regenerated body is unchanged, so content-neutral upstream touches of schema/draft/schema.ts stop generating these PRs.

Why nit

The PR is already held and both prior findings are already on record; nothing newly breaks. This comment corrects the SHA in the porting/remediation instructions and re-establishes the content-noop verification against the new revision so the maintainer acting on the earlier comments doesn't recreate the provenance skew — the same stale-checklist-correction tier already established on this PR.

@github-actions github-actions Bot force-pushed the update-spec-types branch 3 times, most recently from 8b2eb7b to 6ebb2f5 Compare July 8, 2026 05:03
* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The nightly workflow that maintains this PR rebuilds its head branch from scratch on every drift detection (git checkout -B update-spec-types from a fresh checkout of main, commit only the regenerated anchor, then git push -f --no-verify), so any other commit pushed to this branch — in particular the twin/fixtures manifest refresh that #3485357818 and the 2026-07-05 SHA-correction prescribe landing in this PR to fix the rule-4 anchor↔manifest skew — is silently wiped at the next 04:00 UTC run, with no CI signal that the remediation regressed. This is pre-existing behavior in .github/workflows/update-spec-types.yml (untouched by this diff), but it invalidates the accepted in-PR remediation plan: either the workflow should preserve/rebase existing branch commits (or regenerate the twins/manifests itself with per-revision pinning), or the manifest refresh must be merged the same day / with the cron paused.

Extended reasoning...

What the bug is

The nightly cron in .github/workflows/update-spec-types.yml does not update the existing update-spec-types branch — it recreates it from scratch every time drift is detected. The "Create Pull Request" step (lines 72–75) runs on a fresh actions/checkout of main (the remote branch is never fetched), then:

git checkout -B update-spec-types      # recreate branch from main HEAD + regenerated working tree
git add packages/core-internal/src/types/spec.types.2026-07-28.ts   # ONLY the anchor file
git commit -m "chore: update spec.types.2026-07-28.ts from upstream"
git push -f --no-verify origin update-spec-types

Standard git semantics: the branch is rebuilt as main + exactly one commit containing exactly one file, and the force-push discards any other commit present on the remote PR branch — companion fixes, test carve-outs, and specifically the rule-4 manifest refresh that the standing review comments direct maintainers to land here.

Why the trigger is guaranteed for the life of this PR

The drift gate (line 53) runs git diff --quiet on the regenerated anchor against main's copy. While this PR is unmerged, main's anchor header still records f68d864a, and the regenerated file records the latest upstream SHA (currently 93671a3f) — so the header line differs every night (even when upstream content is byte-identical, per the header-only-churn finding in #3487174588), has_changes=true fires, and the branch is force-recreated. This isn't hypothetical: this PR's own head has already been rewritten repeatedly (f68d864a2ffc3fa293671a3f) by exactly this mechanism, invalidating review state each time.

Step-by-step failure scenario

  1. A maintainer follows #3485357818 (with the corrected SHA from the 2026-07-05 follow-up) and pushes a commit to update-spec-types refreshing packages/core-internal/test/corpus/schema-twins/manifest.json and test/corpus/fixtures/2026-07-28/manifest.json to pin 93671a3f. Rule 4 is now satisfied on the PR branch — anchor and manifests agree.
  2. At the next 04:00 UTC run, the workflow checks out main, regenerates the anchor, and the drift check fails (main's copy still says f68d864a), so has_changes=true.
  3. git checkout -B update-spec-types recreates the branch from main + the single anchor commit; git push -f overwrites the remote. The maintainer's manifest commit vanishes with no notification.
  4. Nothing in CI catches the regression: schemaTwinConformance.test.ts only hashes twin bytes against the manifest's own sha256 entries and never cross-checks manifest.source.commit against the anchor header (a grep of the test tree for Last updated from commit returns only scripts/fetch-spec-types.ts). The PR later merges in exactly the rule-4-violating anchor↔manifest provenance-skew state the remediation was supposed to prevent.

Why existing comments don't cover this

#3485357818 asks for the manifest refresh to land in this PR and separately suggests the cron should regenerate the twins alongside the anchor; #3487174588 covers the header-only drift churn and the fetch scripts' single-SHA/path-filter defects; the 2026-07-05 comment corrects the target SHA to 93671a3f. All of them implicitly assume a commit pushed to this branch would stick — none observes that the workflow's branch-recreation + force-push makes any human commit on this branch non-durable across nightly runs, which is the piece that defeats the prescribed remediation path.

How to fix

Options, roughly in order of robustness: (a) have the workflow fetch the existing remote branch, regenerate the anchor on top of it, and push only if the regenerated anchor differs from the branch's copy (rather than recreating from main) — preserving extra commits; (b) have the workflow itself run fetch:schema-twins / fetch:spec-examples with the same SHA and git add the manifests+twins so the branch is always rule-4-consistent without human commits (dovetails with #3485357818's longer-term suggestion, but must use per-revision pinning per #3487174588's caveats about the fetch scripts); or (c) at minimum, document that the manifest refresh must be merged the same day as it's pushed, or with the cron paused. Option (b) combined with a header-aware drift check (git diff -I 'Last updated from commit:') resolves both this finding and the recurring header-only churn.

Severity

Filed as a nit: the defective mechanism lives in the pre-existing workflow file, which this diff does not touch, and merging this one-line sync breaks nothing by itself. But it materially corrects the remediation plan two standing review comments prescribe, so it belongs on record as companion/process scoping alongside the other accepted workflow findings on this PR.

* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟣 Two pre-existing gaps in the fetch-spec-types.ts pipeline that stamps this provenance line (neither introduced by this PR): (1) fetchLatestSHA() resolves the draft anchor's commit via an unauthenticated api.github.com call (60 req/hr shared per runner-IP) and the nightly workflow passes no GITHUB_TOKEN to that step, so intermittent 403 rate-limits fail the job before the drift check and silently skip that night's refresh; (2) the sibling frozen anchor spec.types.2025-11-25.ts:6 records commit 357adac4 while RELEASED_REVISION_PINS pins 0168c57f, so the documented no-arg 'pnpm run fetch:spec-types' produces an unexpected header-only dirty diff on the frozen anchor, contrary to README lifecycle rule 1. Fix by sending 'Authorization: Bearer $GITHUB_TOKEN' when set (and exposing github.token to the fetch step), and by realigning the 2025-11-25 header/pin (content at both commits is verified byte-identical, so a header-only commit suffices).

Extended reasoning...

Gap 1 — unauthenticated SHA lookup silently skips nightly refreshes

fetchLatestSHA() (scripts/fetch-spec-types.ts:51-65) resolves the floating draft anchor's target commit — the SHA recorded on the header line this PR changes — via a bare fetch() to https://api.github.com/repos/modelcontextprotocol/modelcontextprotocol/commits?path=schema/draft/schema.ts, with no Authorization header (a repo-wide grep for Authorization/GITHUB_TOKEN under scripts/ returns nothing). The nightly workflow's "Fetch latest spec types" step (.github/workflows/update-spec-types.yml:47-48) sets no env: block, so GITHUB_TOKEN is never available to the script even if it tried — only the later "Create Pull Request" step gets GH_TOKEN (line 64).

Why it fails: unauthenticated GitHub REST calls are limited to 60 requests/hour per origin IP, and GitHub-hosted runners share a small egress IP pool across all tenants, so the quota is routinely exhausted by other workloads on the same IP. When that happens fetchLatestSHA() gets HTTP 403, throws Failed to fetch commit info: 403, and main()'s catch does process.exit(1) — the job fails before the "Check for changes" step, so no refresh PR is opened or updated that night. This failure class was reproduced during review: an unauthenticated api.github.com call from a shared runner IP returned "API rate limit exceeded". Note the subsequent content fetch (raw.githubusercontent.com, fetch-spec-types.ts:68) is not subject to this API quota — the SHA lookup is the only fragile call in the pipeline.

Impact: fail-safe but silent staleness — no wrong content is ever written, but the nightly refresh is skipped, and cron-failure notifications go only to whoever last touched the workflow file, so repeated skips can go unnoticed while the draft anchor drifts behind upstream (contrary to lifecycle rule 2 in packages/core-internal/src/types/README.md, which says the anchor floats via refresh PRs). Contributors running pnpm run fetch:spec-types from shared/corporate/CI egress IPs hit the same 403s.

Step-by-step proof:

  1. Nightly cron fires; the runner's shared egress IP has already burned its 60 unauthenticated requests/hour on other tenants' jobs.
  2. fetchLatestSHA() issues fetch('https://api.github.com/repos/.../commits?path=schema/draft/schema.ts') with no auth header → HTTP 403 API rate limit exceeded.
  3. response.ok is false → throw new Error('Failed to fetch commit info: 403 ...').
  4. main()'s catch logs and process.exit(1) → the step fails; "Check for changes" and "Create Pull Request" never run.
  5. No refresh PR that night; the failure email goes only to the last workflow-file editor. Next successful night self-heals — but nothing surfaces the skipped nights.

Fix: in fetchLatestSHA() (and the identical pattern in fetch-spec-examples.ts's fallback, plus fetch-schema-twins.ts), send Authorization: Bearer ${process.env.GITHUB_TOKEN} when set — authenticated Actions calls get 1,000 req/hr/repo — and add env: { GITHUB_TOKEN: ${{ github.token }} } to the "Fetch latest spec types" step; optionally retry with backoff on 403. This folds naturally into the workflow hardening already proposed in #3485357818/#3487174588.

Gap 2 — frozen 2025-11-25 anchor header disagrees with its governing pin

packages/core-internal/src/types/spec.types.2025-11-25.ts:6 records Last updated from commit: 357adac47ab2654b64799f994e6db8d3df4ee19d, but scripts/fetch-spec-types.ts:43-45 pins RELEASED_REVISION_PINS['2025-11-25'] = '0168c57fc74aba6e6dcf8f0b7191db3caaa5ad65'. README lifecycle rule 1 says released anchors regenerate only from the pinned commit "so the anchor and the automation that maintains it can never disagree about the source of truth" — yet on main today they already disagree.

Upstream schema/2025-11-25/schema.ts was fetched at both SHAs during verification: byte-identical (66,611 bytes each, empty diff), so the divergence is provenance-only — the anchor content is correct. But the consequence is real: updateSpecTypes() uses the pin whenever no explicit SHA is given, and main() with no args iterates all SUPPORTED_VERSIONS, so both the documented no-arg pnpm run fetch:spec-types and the per-file instruction printed in the frozen anchor's own header (pnpm run fetch:spec-types 2025-11-25) regenerate the frozen anchor and flip its header 357adac4 → 0168c57f — an unexpected dirty diff on a file the lifecycle policy says only changes through a deliberate, reviewed repin. A contributor doing an unrelated draft refresh with the no-arg command would see (and could accidentally commit) a change to the frozen anchor.

Why it's silent: no test cross-checks any anchor header commit against RELEASED_REVISION_PINS or the schema-twins manifest (grep for "Last updated from commit" under packages/core-internal/test/ returns nothing) — the same silent-provenance class as the rule-4 anchor/manifest skew already reported in #3485357818.

Fix: a one-line alignment — regenerate the 2025-11-25 anchor from the pin so its header records 0168c57f (content unchanged, so this is a header-only commit), or move the pin to 357adac4 if that is the intended freeze point. Optionally add a test asserting each released anchor header's recorded commit equals its pin (and the draft anchor's equals the twins-manifest commit), so provenance-only divergences fail loudly.

Why pre-existing

This PR's diff touches only the 2026-07-28 anchor's provenance header. Neither fetch-spec-types.ts, the workflow, nor the 2025-11-25 anchor/pin are modified here — this PR is the pipeline's output, and the findings concern the mechanism that produced it and stamps the exact line the diff changes (the same in-scope basis as prior accepted pipeline findings on this PR). Both issues are fail-safe: the rate-limit failure never writes wrong content, and the header/pin skew is provenance-only with verified-identical content. Nothing here should block this merge; both belong in the already-planned pipeline hardening.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 6ebb2f5 to 71e93d3 Compare July 9, 2026 05:19
* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟣 Pre-existing pipeline gap (in .github/workflows/update-spec-types.yml, not this diff): the nightly refresh branch is pushed and the PR created/updated with the default GITHUB_TOKEN, and GitHub suppresses workflow runs for events created by that token — so the pull_request-triggered gate workflows (main.yml, conformance.yml, examples.yml) never run on this or any refresh PR, and the drift-guard comparison suites structurally cannot produce the red-CI signal that README lifecycle rule 3 relies on. Fix by pushing/creating the PR with a GitHub App installation token or PAT (or explicitly dispatching the gate workflows after pr create/edit), or at minimum document that refresh PRs carry no CI verdict and must be validated locally before merge.

Extended reasoning...

What the bug is

The nightly cron that produces and maintains this PR authenticates every write with the repository's default GITHUB_TOKEN: the branch push (git push -f --no-verify origin update-spec-types, update-spec-types.yml:75) uses the credentials persisted by actions/checkout@v7 (no token: input, default persist-credentials: true), and the PR is created/updated via gh with GH_TOKEN: ${{ github.token }} (line 64). GitHub's documented platform behavior ("Automatic token authentication") is: "events triggered by the GITHUB_TOKEN, with the exception of workflow_dispatch and repository_dispatch, will not create a new workflow run" — a deliberate, long-standing guard against recursive workflow runs (the same limitation peter-evans/create-pull-request prominently documents under "Triggering further workflow runs"). So the pull_request opened event from gh pr create and every subsequent synchronize event from the nightly force-pushes never trigger any workflow.

The code path

  1. Cron fires at 04:00 UTC; drift is detected; the workflow force-pushes update-spec-types using the checkout-persisted GITHUB_TOKEN and runs gh pr create/gh pr edit with github.token.
  2. GitHub attributes the resulting pull_request opened/synchronize events to the GITHUB_TOKEN actor and, per the rule above, creates zero workflow runs from them.
  3. All three per-PR gate workflows — main.yml (the check:all/test matrix where the drift-guard suites, incl. spec.types.2026-07-28.test.ts, run), conformance.yml, and examples.yml — trigger only on pull_request, push filtered to main/v2-2026-07-28, and workflow_dispatch (verified by reading their on: blocks; no merge_group or schedule anywhere in .github/workflows/). Nothing compensates.
  4. Result: no refresh PR produced by this cron ever gets a pull_request-triggered CI verdict. The permissions: block (contents: write, pull-requests: write) is irrelevant — the suppression keys on token identity, not scopes.

Why this defeats the repo's own design

packages/core-internal/src/types/README.md lifecycle rule 3 states: "A refresh PR that breaks the comparison suites is the desired signal — it is fixed in that PR, not bypassed." That signal structurally cannot appear: the comparison suites are pull_request-triggered and never execute on the bot's PR. The workflow even sets LEFTHOOK: 0 / --no-verify with the comment "spec drift that breaks typecheck should still open a PR so it can be fixed there" — explicitly deferring validation to PR CI that never runs. It also reframes this PR's review record: the many prior comments describing red CI reflect local reproduction only; this PR has never had a pull_request-triggered CI verdict of any kind.

Concrete failure scenario

  1. Upstream lands a semantic schema.ts change; the nightly job opens a refresh PR whose anchor change breaks spec.types.2026-07-28.test.ts (exactly the situation documented at length in this PR's history).
  2. The PR shows no checks at all — not red, absent. A maintainer merging on the strength of an apparently unobstructed PR breaks main; the breakage only surfaces afterwards in main.yml's push-triggered run on main.
  3. Alternatively, if branch protection marks these workflows as required checks, every cron refresh PR is permanently stuck at "Expected — waiting for status". A human push to unstick it does not stick either: per the already-filed branch-recreation finding, the next 04:00 UTC run force-recreates the branch from main and wipes the human commit, re-suppressing the synchronize event.

Why nothing prevents it

No other trigger covers the update-spec-types branch: the gate workflows' push triggers are filtered to main/v2-2026-07-28, and there is no merge_group in the repo. And nothing surfaces the gap — a PR with zero checks looks clean, not broken. (Per the repo's recurring-catch on unverified CI claims: a live check-runs API probe for this PR's head was rate-limited from the runner IP — itself corroborating the previously filed fetchLatestSHA rate-limit finding — so this claim rests on GitHub's first-party documented GITHUB_TOKEN event semantics, not on inference about third-party actions.)

How to fix

In order of robustness: (a) push the branch and create the PR with a GitHub App installation token or a PAT — pass it as actions/checkout's token: input so the persisted push credential is the app/PAT, and set GH_TOKEN accordingly; events from those tokens trigger workflows normally. This folds naturally into the token-hardening already proposed for the rate-limit finding. (b) Keep github.token but explicitly kick the gates after pr create/edit — all three gate workflows already declare workflow_dispatch (e.g. gh workflow run main.yml --ref update-spec-types), accepting those runs won't attach to the PR as pull_request checks. (c) At minimum, document in the workflow header and README rule 3 that refresh PRs carry no CI verdict and must be validated locally before merge.

This is pre-existing: the entire mechanism lives in update-spec-types.yml, which this diff (a one-line provenance-header bump) does not touch — the same pipeline tier as the force-push branch-recreation and rate-limit findings already on this PR.

@github-actions github-actions Bot force-pushed the update-spec-types branch from 71e93d3 to 751db78 Compare July 10, 2026 05:18
* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The nightly workflow refreshes this PR with gh pr edit $EXISTING_PR --body ... (no --title), so this PR — created before #2252 split the spec snapshot into per-revision files — is permanently titled "chore: update spec.types.ts from upstream", referencing a file that no longer exists anywhere in the tree, even though the body and HEAD commit message already use the new spec.types.2026-07-28.ts name. One-line fix in .github/workflows/update-spec-types.yml: pass the same --title on the edit branch as on the create branch. (Pre-existing workflow behavior, not introduced by this diff.)

Extended reasoning...

What the bug is

The nightly refresh workflow maintains this PR asymmetrically:

  • On first creation it runs gh pr create --title "chore: update spec.types.2026-07-28.ts from upstream" --body "$PR_BODY" ... — the title matching the per-revision anchor filename introduced when feat(core): per-revision spec reference types and the 2026-07-28 wire contract surface #2252 split the single spec.types.ts into spec.types.<revision>.ts snapshots.
  • On every subsequent nightly refresh of an already-open PR it runs gh pr edit "$EXISTING_PR" --body "$PR_BODY" — body only, never --title.

gh pr edit updates only the fields it is explicitly given, so the title can never self-heal. This PR (#2027) was created 2026-05-07 by a pre-#2252 revision of the workflow, so its title is chore: update spec.types.ts from upstream — a path that no longer exists anywhere in the repository (the only spec snapshot files today are packages/core-internal/src/types/spec.types.2025-11-25.ts and spec.types.2026-07-28.ts).

Step-by-step proof

  1. The old (pre-feat(core): per-revision spec reference types and the 2026-07-28 wire contract surface #2252) workflow creates PR chore: update spec.types.ts from upstream #2027 titled chore: update spec.types.ts from upstream (visible in the PR metadata, created 2026-05-07).
  2. feat(core): per-revision spec reference types and the 2026-07-28 wire contract surface #2252 renames the anchor to spec.types.2026-07-28.ts; the workflow's title and commit-message strings are updated to match (current update-spec-types.yml: commit message at ~line 74, --title at ~line 91).
  3. Every nightly run since then detects drift, force-pushes the branch, finds the open PR via gh pr list --head update-spec-types --state open, and takes the gh pr edit "$EXISTING_PR" --body "$PR_BODY" branch at ~line 88 — the title argument is simply never passed.
  4. Result: a permanent three-way divergence — the PR title carries the old filename, while the PR body (rewritten nightly: "This PR updates packages/core-internal/src/types/spec.types.2026-07-28.ts...") and the HEAD commit message (751db78: chore: update spec.types.2026-07-28.ts from upstream) both carry the new one.

Why nothing prevents it

gh pr edit only modifies explicitly-passed fields, and no automation or check compares the PR title against the workflow's canonical title string. The same divergence will recur for any future title-format change in the workflow while a refresh PR is open.

Impact

Cosmetic but user-facing and un-self-healing: the PR list, notifications, and any changelog/merge tooling that consumes PR titles record a filename that does not exist in the tree — misleading for anyone searching history for changes to spec.types.2026-07-28.ts by PR title. If the repository's squash-merge default uses the pull request title, the stale filename would also land in the merge-commit headline.

How to fix

Factor the title into a variable shared by both branches and pass it on the edit path, e.g. in .github/workflows/update-spec-types.yml:

gh pr edit "$EXISTING_PR" --title "$PR_TITLE" --body "$PR_BODY"

One-line change, no behavior risk.

Note: the defective mechanism lives in .github/workflows/update-spec-types.yml, which this diff does not touch — this is a pre-existing pipeline issue (same tier as the force-push branch-recreation, GITHUB_TOKEN CI-suppression, and rate-limit findings already on this PR) and should not block the merge.

* Source: https://github.com/modelcontextprotocol/modelcontextprotocol
* Pulled from: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
* Last updated from commit: f68d864a813754e188c6df52dcc5772a12f96c63
* Last updated from commit: 93671a3f2bac3bc11b0eb6327c2d029e272b2871

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟣 Pre-existing pipeline gap, time-bound (the 2026-07-28 publication date is 18 days out): the 2026-07-28 anchor is draft-tracked (UPSTREAM_SCHEMA_DIRS maps it to schema/draft/, no RELEASED_REVISION_PINS entry) and fetch-spec-types.ts writes fetched bytes verbatim with no check that the content matches the revision label — so once upstream publishes schema/2026-07-28/ and schema/draft/ rolls over to the next revision's working directory, the nightly cron will silently stamp next-revision draft content into spec.types.2026-07-28.ts and force-push it into this open PR. Suggested guards: parse the fetched content's LATEST_PROTOCOL_VERSION and abort on mismatch for draft-tracked revisions, and have the nightly job probe upstream for schema/2026-07-28/schema.ts and fail with a "revision published — freeze the pin and retarget" message.

Extended reasoning...

The mechanism

The 2026-07-28 anchor is draft-tracked: UPSTREAM_SCHEMA_DIRS maps '2026-07-28''draft' (scripts/fetch-spec-types.ts:25), RELEASED_REVISION_PINS (line 43) has only a 2025-11-25 entry, and the nightly workflow unconditionally runs pnpm run fetch:spec-types 2026-07-28 with no SHA argument (.github/workflows/update-spec-types.yml:48). That means every night the pipeline resolves fetchLatestSHA() against the latest commit touching schema/draft/schema.ts and updateSpecTypes() writes whatever it finds verbatim under the 2026-07-28 label — nothing in the script verifies that the fetched draft bytes actually correspond to the revision named in the file.

The revision's namesake publication date is 2026-07-28 — 18 days from this re-pull. When upstream publishes, schema/2026-07-28/ appears in the spec repo and schema/draft/ immediately becomes the working directory for the next protocol revision. This is exactly how the current cycle worked: schema/2025-11-25/ froze and schema/draft/ accumulated the 2026-07-28 changes this PR has been tracking since May. From that moment, every nightly run fetches next-revision draft content, relabels it as 2026-07-28, and — via the branch-recreation behavior already on record — force-pushes it into this PR, whose title, body, and two months of review record all describe it as the 2026-07-28 revision.

Why the existing guards don't close this

  1. The documented process is purely manual. README lifecycle rule 1 and the workflow header say the freeze ('when its schema moves out of schema/draft/') 'must land in the same commit that retargets this workflow' — but nothing detects that schema/2026-07-28/ now exists upstream. This PR has been open since 2026-05-07 without merging; assuming a same-day manual repin is not safe.
  2. The only in-repo tripwire is narrow. spec.types.2026-07-28.test.ts:851 asserts expect(LATEST_PROTOCOL_VERSION).toBe('2026-07-28'), where the constant is imported from the anchor itself. It only trips if upstream bumps the draft's version constant — and historically the constant lags the release cut (the draft carried 'DRAFT-2026-v1' for months before being stamped '2026-07-28' mid-June, with draft edits continuing to land afterwards). Any post-release straggler or next-revision draft change that doesn't touch the constant is invisible to this assertion.
  3. Even when the tripwire fires, it can't fire on this PR. Per the already-recorded GITHUB_TOKEN finding, no pull_request-triggered workflow ever runs on the bot's refresh PR, so the mislabeled content arrives with no CI verdict; the assertion first runs post-merge on main.
  4. The blast radius extends to the whole revision-anchored oracle chain. fetch-spec-examples.ts:35 hardcodes EXAMPLES_PATH = 'schema/draft/examples' with no per-revision mapping, and the schema-twins manifest's 2026-07-28 entry pins upstreamPath: 'schema/draft/schema.json' — so a maintainer dutifully performing the rule-4 atomic manifest refresh prescribed by the standing review comments would vendor next-revision schema twins and fixtures under the frozen 2026-07-28 label, corrupting the conformance oracle for a released revision in the same commit.

Concrete step-by-step scenario

  1. 2026-07-28: upstream publishes schema/2026-07-28/ (copying the draft) and merges the first post-release change into schema/draft/ — next-revision work or a straggler — without yet bumping the draft's LATEST_PROTOCOL_VERSION constant.
  2. 04:00 UTC next day: the cron fetches the draft at the new SHA, regenerates spec.types.2026-07-28.ts with next-revision content, sees drift vs main, force-recreates update-spec-types, and overwrites this PR's head. Title and body still say 2026-07-28; no CI runs.
  3. A maintainer — possibly acting on the standing 'close as no-op or land with a manifest refresh' guidance written against the current content-noop state — merges, or first refreshes the twins/fixtures from the same draft SHA per rule 4.
  4. main now carries next-revision spec content (and possibly twins/fixtures) labeled 2026-07-28. The vitest version pin catches it only if/when upstream has bumped the draft constant; otherwise the frozen-revision oracle is silently wrong and the SDK's 2026-07-28 wire drift guard starts enforcing against the wrong revision.

How to fix (folds into the already-planned pipeline hardening)

  • Cheap deterministic guard in fetch-spec-types.ts: for draft-tracked revisions, parse the fetched content's LATEST_PROTOCOL_VERSION and abort loudly when it does not equal the target revision (catches the constant-bumped case before writing anything).
  • Release detection in the nightly workflow: probe for the existence of schema/2026-07-28/schema.ts upstream (one raw.githubusercontent HEAD request) and, when found, fail the job with a 'revision published — freeze the pin and retarget this workflow' message instead of opening/updating refresh PRs.
  • On publication: add RELEASED_REVISION_PINS['2026-07-28'], flip UPSTREAM_SCHEMA_DIRS to schema/2026-07-28, update fetch-spec-examples.ts's EXAMPLES_PATH and the twins-manifest upstreamPath, and retarget the workflow — in one commit, per lifecycle rule 1.
  • Give fetch-spec-examples.ts a per-revision path mapping like fetch-spec-types.ts instead of the hardcoded draft path.

This is a pre-existing gap — the entire mechanism lives in .github/workflows/update-spec-types.yml and scripts/, which this one-line provenance-header diff does not touch — so it should not block this merge. It is recorded because it directly determines this open PR's future head content, the deadline is concrete, and the guards are cheap.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants