feat(core): export Protocol class + ProtocolSpec generic for typed custom vocabularies#1917
Conversation
|
@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 |
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.