-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): export Protocol class + ProtocolSpec generic for typed custom vocabularies #1917
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
felixweinberger
wants to merge
6
commits into
fweinberger/v2-bc-3arg-custom-methods
Choose a base branch
from
fweinberger/v2-bc-export-protocol
base: fweinberger/v2-bc-3arg-custom-methods
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6f699ea
feat(core): export Protocol class + ProtocolSpec generic for typed cu…
felixweinberger 7401e8b
docs: update prose now that Protocol is concrete and public
felixweinberger 441a762
docs(core): public/index.ts section comment — Protocol is abstract, n…
felixweinberger cca9964
fix(core): enforce SpecT result type on setRequestHandler (no loose-o…
felixweinberger 5707536
fix: enforce result type on Client/Server 3-arg setRequestHandler; cl…
felixweinberger 04d9133
docs(core): add @remarks on Protocol subclassing stability
felixweinberger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| '@modelcontextprotocol/server': minor | ||
| --- | ||
|
|
||
| Export the abstract `Protocol` class (was reachable in v1 via deep imports) and add `Protocol<ContextT, SpecT extends ProtocolSpec = McpSpec>` for typed custom-method vocabularies. Subclasses supplying a concrete `ProtocolSpec` get method-name autocomplete and result-type correlation on the typed `setRequestHandler`/`setNotificationHandler` overloads (handler param types come from the `paramsSchema` argument; `ProtocolSpec['params']` is informational). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { z } from 'zod'; | ||
|
|
||
| import type { BaseContext, ProtocolSpec, SpecRequests } from '../../src/shared/protocol.js'; | ||
| import { Protocol } from '../../src/shared/protocol.js'; | ||
| import { InMemoryTransport } from '../../src/util/inMemory.js'; | ||
|
|
||
| class TestProtocol<SpecT extends ProtocolSpec = ProtocolSpec> extends Protocol<BaseContext, SpecT> { | ||
| protected assertCapabilityForMethod(): void {} | ||
| protected assertNotificationCapability(): void {} | ||
| protected assertRequestHandlerCapability(): void {} | ||
| protected assertTaskCapability(): void {} | ||
| protected assertTaskHandlerCapability(): void {} | ||
| protected buildContext(ctx: BaseContext): BaseContext { | ||
| return ctx; | ||
| } | ||
| } | ||
|
|
||
| describe('ProtocolSpec typing', () => { | ||
| type AppSpec = { | ||
| requests: { | ||
| 'ui/open-link': { params: { url: string }; result: { opened: boolean } }; | ||
| }; | ||
| notifications: { | ||
| 'ui/size-changed': { params: { width: number; height: number } }; | ||
| }; | ||
| }; | ||
|
|
||
| type _Assert<T extends true> = T; | ||
| type _Eq<A, B> = [A] extends [B] ? ([B] extends [A] ? true : false) : false; | ||
| type _t1 = _Assert<_Eq<SpecRequests<AppSpec>, 'ui/open-link'>>; | ||
| type _t2 = _Assert<_Eq<SpecRequests<ProtocolSpec>, never>>; | ||
| void (undefined as unknown as [_t1, _t2]); | ||
|
|
||
| it('typed-SpecT overload infers params/result; string fallback still works', async () => { | ||
| const [t1, t2] = InMemoryTransport.createLinkedPair(); | ||
| const app = new TestProtocol<AppSpec>(); | ||
| const host = new TestProtocol<AppSpec>(); | ||
| await app.connect(t1); | ||
| await host.connect(t2); | ||
|
|
||
| host.setRequestHandler('ui/open-link', z.object({ url: z.string() }), p => { | ||
| const _typed: string = p.url; | ||
| void _typed; | ||
| return { opened: true }; | ||
| }); | ||
| const r = await app.request({ method: 'ui/open-link', params: { url: 'https://x' } }, z.object({ opened: z.boolean() })); | ||
| expect(r.opened).toBe(true); | ||
|
|
||
| host.setRequestHandler('not/in-spec', z.object({ n: z.number() }), p => ({ doubled: p.n * 2 })); | ||
| const r2 = await app.request({ method: 'not/in-spec', params: { n: 3 } }, z.object({ doubled: z.number() })); | ||
| expect(r2.doubled).toBe(6); | ||
| }); | ||
|
|
||
| it('typed-SpecT overload types handler from passed schema, not SpecT (regression)', () => { | ||
| type Spec = { requests: { 'x/y': { params: { a: string; b: string }; result: { ok: boolean } } } }; | ||
| const p = new TestProtocol<Spec>(); | ||
| const Narrow = z.object({ a: z.string() }); | ||
| p.setRequestHandler('x/y', Narrow, params => { | ||
| const _a: string = params.a; | ||
| // @ts-expect-error -- params is InferOutput<Narrow>, has no 'b' even though Spec does | ||
| const _b: string = params.b; | ||
| void _a; | ||
| void _b; | ||
| return { ok: true }; | ||
| }); | ||
| }); | ||
|
|
||
| it('typed-SpecT setRequestHandler enforces result type (no fallthrough to loose string overload)', () => { | ||
| const p = new TestProtocol<AppSpec>(); | ||
| // @ts-expect-error -- result must be { opened: boolean }; string overload is `never`-guarded for spec methods | ||
| p.setRequestHandler('ui/open-link', z.object({ url: z.string() }), () => ({ ok: 'wrong-type' })); | ||
| // @ts-expect-error -- empty object doesn't satisfy { opened: boolean } | ||
| p.setRequestHandler('ui/open-link', z.object({ url: z.string() }), () => ({})); | ||
| // non-spec methods still allow loose Result | ||
| p.setRequestHandler('not/in-spec', z.object({}), () => ({ anything: 1 })); | ||
| // notifications: spec and non-spec both allow any schema and return void | ||
| p.setNotificationHandler('ui/size-changed', z.object({ width: z.number(), height: z.number() }), () => {}); | ||
| p.setNotificationHandler('not/in-spec', z.object({ x: z.number() }), () => {}); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Nit: after 7051b7c changed "params/result correlation" → "result-type correlation", this JSDoc (and
.changeset/export-protocol-spec.md:6) still says SpecT gives "method-name autocomplete and result-type correlation on the typed overloads ofsetRequestHandlerandsetNotificationHandler". Notification handlers returnvoid— there's no result type to correlate, so forsetNotificationHandlerSpecT only contributes method-name autocomplete. Suggest: "method-name autocomplete on both, plus result-type correlation onsetRequestHandler" (or just dropsetNotificationHandlerfrom the result-correlation clause).Extended reasoning...
What's overstated. The
ProtocolSpecJSDoc at protocol.ts:311-313 and the changeset at.changeset/export-protocol-spec.md:6both read: "method-name autocomplete and result-type correlation on the typed overloads ofsetRequestHandlerandsetNotificationHandler". The natural English parse distributes both features ("method-name autocomplete" and "result-type correlation") across both methods. But notification handlers have no result type — the SpecT-typedsetNotificationHandleroverload (protocol.ts:1209-1213) declares the handler as(params: StandardSchemaV1.InferOutput<P>) => void | Promise<void>, identical to the never-guarded string overload below it. There is nothing for "result-type correlation" to correlate.How it got here. Commit 7051b7c addressed the prior review comment (inline 3100080328) that flagged "params/result correlation" as overstating what SpecT does, since e042fae had dropped the params constraint. The fix changed "params/result" → "result-type" in both locations — correct for
setRequestHandler— but leftsetNotificationHandlerin the conjunction. After dropping "params/",setNotificationHandler's only remaining SpecT contribution is theK extends SpecNotifications<SpecT>method-name constraint; the "result-type" half no longer has anything to point at for notifications.Step-by-step proof.
type Spec = { notifications: { 'ui/ping': { params: { n: number } } } }andconst p = new TestProtocol<Spec>().K = 'ui/ping'(method-name autocomplete ✓).(params: InferOutput<P>) => void | Promise<void>— no_Notifications<SpecT>[K]['result']lookup exists (notifications have noresultfield inProtocolSpec, line 321:notifications?: Record<string, { params?: unknown }>).setRequestHandler's SpecT overload at protocol.ts:1100-1107, which types the return as_Requests<SpecT>[K]['result'] | Promise<…>— that's the result-type correlation the prose describes.setNotificationHandler, SpecT contributes only step 2 (autocomplete); the "result-type correlation" claim is vacuous there.Partial mitigation, and why it's still worth fixing. The very next paragraph (protocol.ts:316-317: "Only
requests[K].resultis enforced by the type system") implicitly scopes result enforcement to requests, so a careful reader can reconcile. But the headline sentence is what readers skim, and the changeset prose lands verbatim in published CHANGELOGs where the mitigating paragraph won't follow it. Per REVIEW.md §Documentation & Changesets ("flag any claim the diff doesn't back"), this is the textbook case.Fix. One-clause reword in both locations, e.g.: "gives method-name autocomplete on the typed overloads of
setRequestHandlerandsetNotificationHandler, plus result-type correlation onsetRequestHandler" — or simply dropsetNotificationHandlerfrom the sentence and let the next paragraph cover it. Doc-only; no runtime or type-level impact.