feat(core): export Protocol class + ProtocolSpec generic for typed custom vocabularies#1917
Conversation
🦋 Changeset detectedLatest commit: a6a9a52 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.
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
e1af6ec to
ccb49d3
Compare
|
@claude review |
2a23915 to
59f44bb
Compare
b7402d8 to
a0158a2
Compare
59f44bb to
c61962c
Compare
a0158a2 to
829ed7f
Compare
c61962c to
d3f9f5a
Compare
829ed7f to
eca508d
Compare
00306b5 to
99794a8
Compare
|
@claude review |
54a89b3 to
e042fae
Compare
|
@claude review |
cdc022b to
d353e17
Compare
e042fae to
56ce347
Compare
d353e17 to
ddd6776
Compare
56ce347 to
7051b7c
Compare
8eab636 to
1c4736b
Compare
b141b90 to
04d9133
Compare
1c4736b to
5f3dbdb
Compare
04d9133 to
e25c005
Compare
5f3dbdb to
4572d4d
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.
…verload fallthrough) The SpecT-typed overload's P constraint (StandardSchemaV1<SpecParams>) was too tight to ever match real schemas, so calls always fell through to the loose (method: string, schema, h: => Result) overload, silently accepting any return type. Loosen overload 1's P to bare StandardSchemaV1 (params type still inferred from the schema; only the result type is SpecT-constrained) and never-guard the loose overload's method param against SpecRequests<SpecT> so spec-method calls that don't satisfy the typed overload error instead of falling through. Same guard on setNotificationHandler for symmetry. JSDoc updated; type tests added.
…arify ProtocolSpec.params is informational
…ndler (returns void)
e25c005 to
a6a9a52
Compare
| public override setRequestHandler<M extends RequestMethod, P extends StandardSchemaV1>( | ||
| method: M, | ||
| paramsSchema: P, | ||
| handler: (params: StandardSchemaV1.InferOutput<P>, ctx: ClientContext) => ResultTypeMap[M] | Promise<ResultTypeMap[M]> | ||
| ): void; | ||
| public override setRequestHandler<M extends string, P extends StandardSchemaV1>( | ||
| method: M extends RequestMethod ? never : M, | ||
| paramsSchema: P, | ||
| handler: (params: StandardSchemaV1.InferOutput<P>, ctx: ClientContext) => Result | Promise<Result> | ||
| ): void; |
There was a problem hiding this comment.
🟡 Nit: 8a26ebc applied the same never-guard + ResultTypeMap[M] 3-arg overload to both Server.setRequestHandler and Client.setRequestHandler, but only Server got a @ts-expect-error type-regression test (packages/server/test/server/setRequestHandlerSchemaParity.test.ts:106-112). The parallel packages/client/test/client/setRequestHandlerSchemaParity.test.ts already exists and mirrors the other Server cases — worth dropping the same ~6-line test in there so a future edit to client.ts can't silently drop the guard (subclass overload sets fully shadow Protocol's, so neither protocolSpec.test.ts nor the Server test covers Client).
Extended reasoning...
What's missing. Commit 8a26ebc propagated the never-guard fix to both subclass overrides: Server.setRequestHandler (server.ts:235-244) and Client.setRequestHandler (client.ts:348-357) each gained an M extends RequestMethod 3-arg overload returning ResultTypeMap[M], plus the method: M extends RequestMethod ? never : M guard on the loose overload. The same commit added a type-level regression test for Server at packages/server/test/server/setRequestHandlerSchemaParity.test.ts:106-112 — a @ts-expect-error asserting that s.setRequestHandler('ping', z.object({}), () => ({ ok: 'wrong-type' })) fails to compile. No equivalent test was added for Client, even though a directly parallel file packages/client/test/client/setRequestHandlerSchemaParity.test.ts already exists (91 lines) and otherwise mirrors the Server parity tests case-for-case.
Why the existing tests don't cover it. The whole reason 8a26ebc had to touch client.ts and server.ts at all (per resolved inline comment 3100080308) is that a subclass which redeclares an overloaded method's signature set fully shadows the base class's overloads at call sites. So:
packages/core/test/shared/protocolSpec.test.ts:69-76exercises the guard on a directProtocolsubclass — it never seesClient's overload set.packages/server/.../setRequestHandlerSchemaParity.test.ts:106-112exercisesServer's overload set — it never seesClient's.- The client parity file's existing 3-arg test at line 80 uses
() => invalidElicitResult as neverto bypass the new return-type constraint for a runtime assertion. Anas nevercast compiles whether or not the guard exists, so it is not a regression test for the type-level change.
A future edit to client.ts that dropped the never-guard or the ResultTypeMap[M] overload would therefore pass pnpm typecheck:all and pnpm test:all cleanly.
Step-by-step proof.
- Open
packages/client/src/client/client.ts:353-357: the loose 3-arg overload's first parameter ismethod: M extends RequestMethod ? never : M. - Hypothetically revert that line to
method: string(the pre-8a26ebc form) and delete theResultTypeMap[M]overload at lines 348-352. grep -r '@ts-expect-error' packages/client/test/→ no matches. There is no type-level assertion in the client package that depends on the guard.- The client parity file's line 80 still compiles (
as neveris assignable to anything). Lines 87-89 ('acme/echo', non-spec method) still compile against plainmethod: string. - The core test at
protocolSpec.test.ts:69-76callsnew TestProtocol<AppSpec>().setRequestHandler(...), which resolves againstProtocol's overload set, notClient's — still passes. - The server test at
setRequestHandlerSchemaParity.test.ts:108callsnew Server(...).setRequestHandler(...)— unaffected by theClientedit. - Result: the regression ships with green CI.
Why it's worth flagging. Per REVIEW.md §Tests ("New behavior has vitest coverage") and §Completeness ("when a PR replaces a pattern, grep the package for surviving instances"), this is the textbook asymmetric-coverage shape: the PR established the test-the-override pattern for Server but skipped Client for an identical change in the same commit. The parallel test file already exists, already imports Client and z, and already has 3-arg-form cases — so the marginal cost is ~6 lines.
Fix. Mirror the Server case into packages/client/test/client/setRequestHandlerSchemaParity.test.ts, e.g.:
it('three-arg form on Client enforces spec-method result type (no fallthrough to loose overload)', () => {
const c = new Client({ name: 't', version: '1.0' }, { capabilities: {} });
// @ts-expect-error -- result for 'ping' must be EmptyResult-compatible; loose overload is never-guarded for spec methods
c.setRequestHandler('ping', z.object({}), () => ({ ok: 'wrong-type' }) as { ok: string });
// non-spec methods still allow loose Result
c.setRequestHandler('acme/custom', z.object({}), () => ({ anything: 1 }));
});Nit-level — no runtime or correctness impact; this is purely about locking in the type-level guarantee on both subclasses.
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.