feat(core): export Protocol class + ProtocolSpec generic for typed custom vocabularies#1917
Conversation
🦋 Changeset detectedLatest commit: ccb49d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
2f34630 to
daab2e2
Compare
33209d0 to
ae50e77
Compare
|
@claude review |
There was a problem hiding this comment.
No correctness issues spotted beyond the doc-drift nit, but this promotes Protocol (plus four new types) into the curated public surface and adds a second class-level generic — that's exactly the kind of API-surface commitment REVIEW.md says needs a human maintainer's sign-off, so deferring.
Extended reasoning...
Overview
This PR (third in the v2-bc stack) does two things: (1) re-exports the abstract Protocol class from core/public along with four new type-level helpers (ProtocolSpec, McpSpec, SpecRequests, SpecNotifications), and (2) extends Protocol with a second generic parameter SpecT extends ProtocolSpec = McpSpec plus a new spec-typed overload at the front of the setRequestHandler / setNotificationHandler overload sets. ContextT also gains a = BaseContext default. A new vitest file covers the type inference, and a changeset is included.
Security risks
None identified — purely type-level additions and an export-barrel change; no new runtime code paths, I/O, auth, or parsing logic.
Level of scrutiny
High, on API-surface grounds rather than correctness. REVIEW.md is explicit that the burden of proof is on addition and that every new public export must be intentional; CLAUDE.md §Public API Exports until now used Protocol as the canonical internal-only example. Promoting it (and committing to ProtocolSpec as a public extension contract) is a deliberate policy reversal that a maintainer should explicitly approve — even if the motivation (v1 deep-import parity, ext-apps subclassing) is sound.
Other factors
- I verified
ClientandServerboth re-declare their ownsetRequestHandleroverload sets, so the new SpecT-typed overload on the base class is shadowed there and shouldn't perturb existing user-facing inference; the change is effectively scoped to directProtocolsubclasses. - The defaults (
ContextT = BaseContext,SpecT = McpSpec) keep existingextends Protocol<X>references compiling, andSpecRequests<ProtocolSpec> = nevercorrectly disables the typed overload for the unconstrained default — both are exercised in the new test. - One nit (posted inline): the file-header JSDoc in
exports/public/index.tsand CLAUDE.md still citeProtocolas internal-only, contradicting the new export. - Stacked on #1916 → #1891; reviewer should confirm this lands in the intended order.
| SpecRequests | ||
| } from '../../shared/protocol.js'; | ||
| export { DEFAULT_REQUEST_TIMEOUT_MSEC } from '../../shared/protocol.js'; | ||
| export { DEFAULT_REQUEST_TIMEOUT_MSEC, Protocol } from '../../shared/protocol.js'; |
There was a problem hiding this comment.
🟡 Nit: now that Protocol is exported here, two pieces of contributor-facing prose still call it out as internal-only — the file-header JSDoc in this file (lines 7-9: "Internal utilities (Protocol class, stdio parsing, …)") and CLAUDE.md §Public API Exports (lines 69-70, which uses "Protocol class" as the canonical example of an internal-barrel-only symbol). The inline section comment on line 41 was updated, but these two were missed; drop "Protocol class" from both example lists.
Extended reasoning...
What's stale and where. This PR promotes the abstract Protocol class to the public surface (packages/core/src/exports/public/index.ts:55) and correctly updates the inline section comment on line 41 from "NOT the Protocol class itself or mergeCapabilities" to "Protocol class (abstract — subclass for custom vocabularies) + types. NOT mergeCapabilities." However, two other contributor-facing docs still use Protocol as the example of an internal-only symbol:
packages/core/src/exports/public/index.ts:7-9— the module-level JSDoc reads: "Internal utilities (Protocol class, stdio parsing, schema helpers, etc.) remain available via the internal barrel". This file now exportsProtocol48 lines below that comment.CLAUDE.md:69-70— line 69 lists "Protocol class" as an example of what the internal barrel exports, and line 70 describescore/publicas "Exports only TypeScript types, error classes, constants, and guards", with the contrast implyingProtocollives only in the internal barrel.
Why it matters. REVIEW.md's "Documentation & Changesets" item asks us to flag prose that now contradicts the implementation, and its API-surface checklist explicitly references "CLAUDE.md § Public API Exports" — so this section is load-bearing for future reviewers deciding what's allowed in core/public. A contributor reading either doc today would conclude that exporting Protocol from core/public is a policy violation, when in fact this PR is the deliberate policy change.
Step-by-step proof.
- Open
packages/core/src/exports/public/index.tsat HEAD: line 55 readsexport { DEFAULT_REQUEST_TIMEOUT_MSEC, Protocol } from '../../shared/protocol.js';. - Scroll to lines 7-9 of the same file: the JSDoc still parenthetically lists "Protocol class" as an internal utility not exported here. Direct contradiction within one file.
- Open
CLAUDE.md:69: "Internal barrel. Exports everything (including Zod schemas, Protocol class, stdio utils)." Line 70: "Curated public API. Exports only TypeScript types, error classes, constants, and guards." The juxtaposition usesProtocolas the example of something the internal barrel has andcore/publicdoes not. - The PR diff updates line 41's section comment to acknowledge the new export but touches neither the file header nor CLAUDE.md.
Scope note. CLAUDE.md:70's "only types/errors/constants/guards" wording was already loose before this PR (UriTemplate, ReadBuffer, AjvJsonSchemaValidator, InMemoryTaskStore are already exported), so that clause is partly pre-existing drift. But the explicit "Protocol class" call-out in line 69 and in the file header are new contradictions introduced specifically by this PR's export change.
Fix. Trivial doc edits: drop "Protocol class, " from the parenthetical at public/index.ts:7, and update CLAUDE.md:69-70 to stop using Protocol as the internal-only example (and optionally relax line 70's "only types/errors/constants/guards" phrasing while you're there).
daab2e2 to
324ed12
Compare
ae50e77 to
b25d3a5
Compare
324ed12 to
3961553
Compare
b25d3a5 to
5fce199
Compare
e75e0b0 to
9da988e
Compare
5fce199 to
9da988e
Compare
|
@claude review |
9da988e to
05a01a1
Compare
…stom vocabularies Exports the abstract Protocol class (was reachable in v1 via deep imports) and adds Protocol<ContextT, SpecT extends ProtocolSpec = McpSpec>. Subclasses supplying a concrete ProtocolSpec get method-name autocomplete and params/result correlation on the typed setRequestHandler/setNotificationHandler overloads.
e1af6ec to
ccb49d3
Compare
|
@claude review |
| * Valibot, ArkType, etc.). When `method` is listed in this instance's | ||
| * {@linkcode ProtocolSpec}, params and result types are inferred from `SpecT`. |
There was a problem hiding this comment.
🟡 Nit: this JSDoc says "params and result types are inferred from SpecT" (and setNotificationHandler at lines 1191-1193 says "the params type is inferred from SpecT"), but the overload right below types the handler's params as StandardSchemaV1.InferOutput<P> — the passed schema's output. Only the result type comes from SpecT; this PR's own regression test (protocolSpec.test.ts:55-66, "types handler from passed schema, not SpecT") asserts via @ts-expect-error that params.b is absent even though Spec declares it. Suggest rewording to e.g. "the result type is inferred from SpecT; params is typed by the passed paramsSchema (which is constrained to accept SpecT's params shape)".
Extended reasoning...
What's wrong. The new JSDoc on setRequestHandler (protocol.ts:1072-1073) reads: "When method is listed in this instance's ProtocolSpec, params and result types are inferred from SpecT." The mirrored JSDoc on setNotificationHandler (protocol.ts:1191-1193) reads: "the params type is inferred from SpecT." But the overload signatures added immediately below each comment type the handler's params argument as StandardSchemaV1.InferOutput<P> — i.e., the output type of the passed paramsSchema, not _Requests<SpecT>[K]['params']. SpecT only enters the picture as a constraint on the schema's input (P extends StandardSchemaV1<_Requests<SpecT>[K]['params']>) and as the source of the result type (_Requests<SpecT>[K]['result']). For setNotificationHandler there is no result type, so the only claim the JSDoc makes — "params type inferred from SpecT" — is the one that's wrong.
The PR's own test contradicts the prose. packages/core/test/shared/protocolSpec.test.ts:55-66 is literally titled "typed-SpecT overload types handler from passed schema, not SpecT (regression)" and uses @ts-expect-error to assert that params.b does not exist even though SpecT declares { a: string; b: string }. The inline comment says: "params is InferOutput, has no 'b' even though Spec does". So the diff adds a doc claim and a test that directly disagree on the same point.
Step-by-step proof.
- Define
type Spec = { requests: { 'x/y': { params: { a: string; b: string }; result: { ok: boolean } } } }and instantiatenew TestProtocol<Spec>(). - Call
p.setRequestHandler('x/y', z.object({ a: z.string() }), params => …). The JSDoc claimsparamsshould be{ a: string; b: string }("inferred fromSpecT"). - The overload resolves
K = 'x/y',P = ZodObject<{ a: ZodString }>; the handler arg is typedStandardSchemaV1.InferOutput<P> = { a: string }. - Accessing
params.bis a type error — exactly what the regression test asserts at line 62 with@ts-expect-error. - Meanwhile the return type is correctly constrained to
{ ok: boolean }fromSpecT— so the "result" half of the JSDoc claim is accurate, the "params" half is not.
Why it matters. This is the public-facing JSDoc for a newly-exported class; users hovering setRequestHandler in their editor will be told that supplying a ProtocolSpec infers their handler's params type, then be confused when a narrower paramsSchema silently narrows the handler signature instead. The changeset and the ProtocolSpec type-level JSDoc both correctly say "params/result correlation" rather than "inferred from", which makes this block the inconsistent outlier. Per REVIEW.md §Documentation & Changesets ("flag prose the diff doesn't back"), this is worth fixing.
Fix. Doc-only reword, e.g.:
setRequestHandler: "Whenmethodis listed in this instance's ProtocolSpec, the result type is inferred fromSpecT;paramsis typed by the passedparamsSchema(which is constrained to acceptSpecT's params shape)."setNotificationHandler: "When the three-arg form'smethodis listed in this instance's ProtocolSpec, the passedparamsSchemais constrained to acceptSpecT's params shape (the handler'sparamsis typed by the schema's output)."
No runtime impact — nit.
| /** | ||
| * Declares the request and notification vocabulary a `Protocol` subclass speaks. | ||
| * | ||
| * Supplying a concrete `ProtocolSpec` as `Protocol`'s second type argument gives method-name | ||
| * autocomplete and params/result correlation on the typed overloads of `setRequestHandler` | ||
| * and `setNotificationHandler`. The default leaves them string-keyed and untyped. | ||
| */ | ||
| export type ProtocolSpec = { | ||
| requests?: Record<string, { params?: unknown; result: unknown }>; | ||
| notifications?: Record<string, { params?: unknown }>; | ||
| }; |
There was a problem hiding this comment.
🟡 Nit: "The default leaves them string-keyed and untyped" reads as describing Protocol's default SpecT argument — but that default is McpSpec (line 342), which is concrete and does give typed autocomplete. Consider rewording to e.g. "Using the bare ProtocolSpec type leaves them string-keyed and untyped; Protocol itself defaults to McpSpec."
Extended reasoning...
What's ambiguous. The ProtocolSpec JSDoc (protocol.ts:301-303) reads:
Supplying a concrete
ProtocolSpecasProtocol's second type argument gives method-name autocomplete and params/result correlation on the typed overloads… The default leaves them string-keyed and untyped.
The immediately-preceding subject is "Protocol's second type argument", so the natural parse of "the default" is "the default of Protocol's second type argument". But line 342 declares Protocol<ContextT extends BaseContext = BaseContext, SpecT extends ProtocolSpec = McpSpec> — the default is McpSpec, and SpecRequests<McpSpec> resolves to the concrete RequestMethod union. So under that reading, the sentence is wrong: omitting SpecT gives MCP-method autocomplete, not string-keyed/untyped overloads.
Addressing the alternative reading. One could argue "the default" means "the bare ProtocolSpec type being documented" (contrasted with "a concrete ProtocolSpec"), and that the SpecRequests JSDoc at lines 322-326 uses "default" the same way ("never for the unconstrained default ProtocolSpec"). Under that reading the sentence is technically accurate — passing the unconstrained ProtocolSpec yields SpecRequests<ProtocolSpec> = never, disabling the spec-typed overload and falling through to the string-keyed one. That is almost certainly the author's intent. But the SpecRequests JSDoc disambiguates by saying "the unconstrained default ProtocolSpec"; this JSDoc just says "the default", and its preceding sentence has already shifted the topic to "Protocol's second type argument", so a reader landing here in isolation will likely take the wrong meaning.
Step-by-step proof of the misleading reading.
- A user reads this JSDoc and writes
class MyProto extends Protocol<MyCtx> { … }— omitting the second type argument, expecting (per "the default leaves them string-keyed and untyped") thatsetRequestHandlerwill accept anystringmethod without spec-typed constraints. SpecTdefaults toMcpSpec(line 342).SpecRequests<McpSpec>=keyof McpSpec['requests'] & string= theRequestMethodunion (sincestring extends RequestMethodis false, line 327).- The first
setRequestHandleroverload (lines 1079-1086) is therefore active withK extends RequestMethod, giving MCP autocomplete and result-type correlation — the opposite of "string-keyed and untyped". - The user's expectation from the JSDoc does not match observed IDE behavior.
Why this matters / fix. REVIEW.md §Documentation & Changesets asks us to flag prose that contradicts (or appears to contradict) the implementation. This is now public API surface (ProtocolSpec is exported from core/public), so the JSDoc is what downstream consumers will read. The fix is a one-line clarification, e.g.:
Using the bare
ProtocolSpectype leaves them string-keyed and untyped;Protocolitself defaults toMcpSpec.
Optionally apply the same "bare/unconstrained" qualifier wherever "the default" is used in this sense. Nit — wording clarity, not a behavioral bug.
Part of the v2 BC series — see reviewer guide. Stacks on #1916 (which stacks on #1891).
Exports the abstract
Protocolclass (was reachable in v1 via deep imports; ext-apps subclasses it) and addsProtocol<ContextT, SpecT extends ProtocolSpec = McpSpec>. Supplying a concreteProtocolSpecgives method-name autocomplete and params/result correlation on the typedsetRequestHandler/setNotificationHandleroverloads.Motivation and Context
v1 exposed
Protocolvia deep imports (sdk/shared/protocol.js). MCP-dialect protocols (e.g., MCP Apps) subclass it. v2 omitted it from the public surface — this restores it and adds aProtocolSpectype-level vocabulary.How Has This Been Tested?
packages/core/test/shared/protocolSpec.test.ts— typed-SpecT inference,SpecRequests<ProtocolSpec> = never, params typed by passed schema (not SpecT).Breaking Changes
None — additive.
Protocolstays abstract; the second generic defaults so existingProtocol<ContextT>references work unchanged.Types of changes
Additional context
Stacks on #1916 → #1891.