-
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
Closed
felixweinberger
wants to merge
7
commits into
fweinberger/v2-bc-3arg-custom-methods
from
fweinberger/v2-bc-export-protocol
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6936d7b
feat(core): export Protocol class + ProtocolSpec generic for typed cu…
felixweinberger f5f58d5
docs: update prose now that Protocol is concrete and public
felixweinberger c0baba4
docs(core): public/index.ts section comment — Protocol is abstract, n…
felixweinberger 7824061
fix(core): enforce SpecT result type on setRequestHandler (no loose-o…
felixweinberger 8a26ebc
fix: enforce result type on Client/Server 3-arg setRequestHandler; cl…
felixweinberger 702fdfd
docs(core): add @remarks on Protocol subclassing stability
felixweinberger a6a9a52
docs(core): drop result-type-correlation claim from setNotificationHa…
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 on the typed `setRequestHandler`/`setNotificationHandler` overloads, and result-type correlation on `setRequestHandler` (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: 8a26ebc applied the same never-guard +
ResultTypeMap[M]3-arg overload to bothServer.setRequestHandlerandClient.setRequestHandler, but only Server got a@ts-expect-errortype-regression test (packages/server/test/server/setRequestHandlerSchemaParity.test.ts:106-112). The parallelpackages/client/test/client/setRequestHandlerSchemaParity.test.tsalready 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 shadowProtocol's, so neitherprotocolSpec.test.tsnor the Server test coversClient).Extended reasoning...
What's missing. Commit 8a26ebc propagated the never-guard fix to both subclass overrides:
Server.setRequestHandler(server.ts:235-244) andClient.setRequestHandler(client.ts:348-357) each gained anM extends RequestMethod3-arg overload returningResultTypeMap[M], plus themethod: M extends RequestMethod ? never : Mguard on the loose overload. The same commit added a type-level regression test for Server atpackages/server/test/server/setRequestHandlerSchemaParity.test.ts:106-112— a@ts-expect-errorasserting thats.setRequestHandler('ping', z.object({}), () => ({ ok: 'wrong-type' }))fails to compile. No equivalent test was added forClient, even though a directly parallel filepackages/client/test/client/setRequestHandlerSchemaParity.test.tsalready 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.tsandserver.tsat 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.() => 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.tsthat dropped the never-guard or theResultTypeMap[M]overload would therefore passpnpm typecheck:allandpnpm test:allcleanly.Step-by-step proof.
packages/client/src/client/client.ts:353-357: the loose 3-arg overload's first parameter ismethod: M extends RequestMethod ? never : M.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.as neveris assignable to anything). Lines 87-89 ('acme/echo', non-spec method) still compile against plainmethod: string.protocolSpec.test.ts:69-76callsnew TestProtocol<AppSpec>().setRequestHandler(...), which resolves againstProtocol's overload set, notClient's — still passes.setRequestHandlerSchemaParity.test.ts:108callsnew Server(...).setRequestHandler(...)— unaffected by theClientedit.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
Serverbut skippedClientfor an identical change in the same commit. The parallel test file already exists, already importsClientandz, 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.:Nit-level — no runtime or correctness impact; this is purely about locking in the type-level guarantee on both subclasses.