Skip to content

feat(core): export Protocol class + ProtocolSpec generic for typed custom vocabularies#1917

Open
felixweinberger wants to merge 7 commits intofweinberger/v2-bc-3arg-custom-methodsfrom
fweinberger/v2-bc-export-protocol
Open

feat(core): export Protocol class + ProtocolSpec generic for typed custom vocabularies#1917
felixweinberger wants to merge 7 commits intofweinberger/v2-bc-3arg-custom-methodsfrom
fweinberger/v2-bc-export-protocol

Conversation

@felixweinberger
Copy link
Copy Markdown
Contributor

Part of the v2 BC series — see reviewer guide. Stacks on #1916 (which stacks on #1891).

Exports the abstract Protocol class (was reachable in v1 via deep imports; ext-apps subclasses it) and adds Protocol<ContextT, SpecT extends ProtocolSpec = McpSpec>. Supplying a concrete ProtocolSpec gives method-name autocomplete and params/result correlation on the typed setRequestHandler/setNotificationHandler overloads.

Motivation and Context

v1 exposed Protocol via 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 a ProtocolSpec type-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. Protocol stays abstract; the second generic defaults so existing Protocol<ContextT> references work unchanged.

Types of changes

  • New feature (non-breaking change which adds functionality)

Additional context

Stacks on #1916#1891.

@felixweinberger felixweinberger added this to the v2.0.0-bc milestone Apr 16, 2026
@felixweinberger felixweinberger added the v2-bc v2 backwards-compatibility series label Apr 16, 2026
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 16, 2026

🦋 Changeset detected

Latest commit: a6a9a52

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/client Minor
@modelcontextprotocol/server Minor
@modelcontextprotocol/express Major
@modelcontextprotocol/fastify Major
@modelcontextprotocol/hono Major
@modelcontextprotocol/node Major

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

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 16, 2026

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@1917

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@1917

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@1917

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@1917

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@1917

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@1917

commit: a6a9a52

@felixweinberger
Copy link
Copy Markdown
Contributor Author

@claude review

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

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 Client and Server both re-declare their own setRequestHandler overload 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 direct Protocol subclasses.
  • The defaults (ContextT = BaseContext, SpecT = McpSpec) keep existing extends Protocol<X> references compiling, and SpecRequests<ProtocolSpec> = never correctly 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.ts and CLAUDE.md still cite Protocol as internal-only, contradicting the new export.
  • Stacked on #1916#1891; reviewer should confirm this lands in the intended order.

Comment thread packages/core/src/exports/public/index.ts
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from daab2e2 to 324ed12 Compare April 16, 2026 19:38
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from ae50e77 to b25d3a5 Compare April 16, 2026 19:41
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 324ed12 to 3961553 Compare April 16, 2026 20:04
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from b25d3a5 to 5fce199 Compare April 16, 2026 20:06
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch 2 times, most recently from e75e0b0 to 9da988e Compare April 16, 2026 20:20
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from 5fce199 to 9da988e Compare April 16, 2026 20:21
@felixweinberger
Copy link
Copy Markdown
Contributor Author

@claude review

@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 9da988e to 05a01a1 Compare April 16, 2026 22:23
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from e1af6ec to ccb49d3 Compare April 16, 2026 22:25
@felixweinberger
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/core/src/shared/protocol.ts Outdated
Comment thread packages/core/src/shared/protocol.ts
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 2a23915 to 59f44bb Compare April 16, 2026 23:45
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from b7402d8 to a0158a2 Compare April 16, 2026 23:46
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 59f44bb to c61962c Compare April 17, 2026 08:14
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from a0158a2 to 829ed7f Compare April 17, 2026 08:15
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from c61962c to d3f9f5a Compare April 17, 2026 08:30
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from 829ed7f to eca508d Compare April 17, 2026 08:32
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from 00306b5 to 99794a8 Compare April 17, 2026 10:30
@felixweinberger
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/core/src/shared/protocol.ts Outdated
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from 54a89b3 to e042fae Compare April 17, 2026 11:39
@felixweinberger
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/core/src/shared/protocol.ts
Comment thread packages/core/src/shared/protocol.ts
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from cdc022b to d353e17 Compare April 17, 2026 12:18
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from e042fae to 56ce347 Compare April 17, 2026 12:27
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from d353e17 to ddd6776 Compare April 17, 2026 12:36
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from 56ce347 to 7051b7c Compare April 17, 2026 12:42
@felixweinberger felixweinberger marked this pull request as ready for review April 17, 2026 12:55
@felixweinberger felixweinberger requested a review from a team as a code owner April 17, 2026 12:55
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 8eab636 to 1c4736b Compare April 17, 2026 14:01
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from b141b90 to 04d9133 Compare April 17, 2026 14:01
Comment thread packages/core/src/shared/protocol.ts Outdated
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 1c4736b to 5f3dbdb Compare April 27, 2026 13:47
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from 04d9133 to e25c005 Compare April 27, 2026 14:00
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-3arg-custom-methods branch from 5f3dbdb to 4572d4d Compare April 27, 2026 15:00
…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.
@felixweinberger felixweinberger force-pushed the fweinberger/v2-bc-export-protocol branch from e25c005 to a6a9a52 Compare April 27, 2026 15:13
Comment on lines +348 to 357
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;
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: 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-76 exercises the guard on a direct Protocol subclass — it never sees Client's overload set.
  • packages/server/.../setRequestHandlerSchemaParity.test.ts:106-112 exercises Server's overload set — it never sees Client's.
  • The client parity file's existing 3-arg test at line 80 uses () => invalidElicitResult as never to bypass the new return-type constraint for a runtime assertion. An as never cast 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.

  1. Open packages/client/src/client/client.ts:353-357: the loose 3-arg overload's first parameter is method: M extends RequestMethod ? never : M.
  2. Hypothetically revert that line to method: string (the pre-8a26ebc form) and delete the ResultTypeMap[M] overload at lines 348-352.
  3. 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.
  4. The client parity file's line 80 still compiles (as never is assignable to anything). Lines 87-89 ('acme/echo', non-spec method) still compile against plain method: string.
  5. The core test at protocolSpec.test.ts:69-76 calls new TestProtocol<AppSpec>().setRequestHandler(...), which resolves against Protocol's overload set, not Client's — still passes.
  6. The server test at setRequestHandlerSchemaParity.test.ts:108 calls new Server(...).setRequestHandler(...) — unaffected by the Client edit.
  7. 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.

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

Labels

v2-bc v2 backwards-compatibility series

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant