-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): add extension() registrar for SEP-2133 capability-aware custom methods #1868
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
Draft
felixweinberger
wants to merge
3
commits into
fweinberger/custom-method-handlers
Choose a base branch
from
fweinberger/extension-registrar
base: fweinberger/custom-method-handlers
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.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
f33255c
feat(core): add extension() registrar for SEP-2133 capability-aware c…
felixweinberger e7743b0
docs(core): fix typedoc cross-package and internal-class link resolut…
felixweinberger 63335f1
fix(core): address review findings on ExtensionHandle
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
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 | ||
| --- | ||
|
|
||
| Add `Client.extension()` / `Server.extension()` registrar for SEP-2133 capability-aware custom methods. Declares an extension in `capabilities.extensions[id]` and returns an `ExtensionHandle` whose `setRequestHandler`/`sendRequest`/`setNotificationHandler`/`sendNotification` calls are tied to that declared capability. `getPeerSettings()` returns the peer's extension settings, optionally validated against a `peerSchema`. |
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,111 @@ | ||
| import { InMemoryTransport, type JSONRPCMessage, SdkError, SdkErrorCode } from '@modelcontextprotocol/core'; | ||
| import { describe, expect, test } from 'vitest'; | ||
| import * as z from 'zod/v4'; | ||
|
|
||
| import { Client } from '../../src/client/client.js'; | ||
|
|
||
| /** | ||
| * These tests exercise the `Client.extension()` factory and the client side of the | ||
| * `capabilities.extensions` round-trip via `initialize`. The `ExtensionHandle` class itself is | ||
| * unit-tested in `@modelcontextprotocol/core/test/shared/extensionHandle.test.ts`. | ||
| */ | ||
|
|
||
| interface RawServerHarness { | ||
| serverSide: InMemoryTransport; | ||
| capturedInitParams: Promise<Record<string, unknown>>; | ||
| } | ||
|
|
||
| function rawServer(serverCapabilities: Record<string, unknown> = {}): RawServerHarness { | ||
| const [clientSide, serverSide] = InMemoryTransport.createLinkedPair(); | ||
| let resolveInit: (p: Record<string, unknown>) => void; | ||
| const capturedInitParams = new Promise<Record<string, unknown>>(r => { | ||
| resolveInit = r; | ||
| }); | ||
| serverSide.onmessage = (msg: JSONRPCMessage) => { | ||
| if ('method' in msg && msg.method === 'initialize' && 'id' in msg) { | ||
| resolveInit((msg.params ?? {}) as Record<string, unknown>); | ||
| void serverSide.send({ | ||
| jsonrpc: '2.0', | ||
| id: msg.id, | ||
| result: { | ||
| protocolVersion: '2025-11-25', | ||
| capabilities: serverCapabilities, | ||
| serverInfo: { name: 'raw-server', version: '0.0.0' } | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
| void serverSide.start(); | ||
| // Expose clientSide via the harness's serverSide.peer for the test to connect to. | ||
| return { serverSide: clientSide, capturedInitParams }; | ||
| } | ||
|
|
||
| describe('Client.extension()', () => { | ||
| test('merges settings into capabilities.extensions and advertises them in initialize request', async () => { | ||
| const client = new Client({ name: 'c', version: '1.0.0' }, { capabilities: {} }); | ||
| client.extension('io.example/ui', { contentTypes: ['text/html'] }); | ||
| client.extension('com.acme/widgets', { v: 2 }); | ||
|
|
||
| const harness = rawServer(); | ||
| await client.connect(harness.serverSide); | ||
| const initParams = await harness.capturedInitParams; | ||
|
|
||
| const caps = initParams.capabilities as Record<string, unknown>; | ||
| expect(caps.extensions).toEqual({ | ||
| 'io.example/ui': { contentTypes: ['text/html'] }, | ||
| 'com.acme/widgets': { v: 2 } | ||
| }); | ||
| }); | ||
|
|
||
| test('throws AlreadyConnected after connect()', async () => { | ||
| const client = new Client({ name: 'c', version: '1.0.0' }); | ||
| const harness = rawServer(); | ||
| await client.connect(harness.serverSide); | ||
|
|
||
| expect(() => client.extension('io.example/ui', {})).toThrow(SdkError); | ||
| try { | ||
| client.extension('io.example/ui', {}); | ||
| expect.fail('should have thrown'); | ||
| } catch (e) { | ||
| expect(e).toBeInstanceOf(SdkError); | ||
| expect((e as SdkError).code).toBe(SdkErrorCode.AlreadyConnected); | ||
| } | ||
| }); | ||
|
|
||
| test('throws on duplicate extension id', () => { | ||
| const client = new Client({ name: 'c', version: '1.0.0' }); | ||
| client.extension('io.example/ui', { v: 1 }); | ||
| expect(() => client.extension('io.example/ui', { v: 2 })).toThrow(/already registered/); | ||
| expect(() => client.extension('com.other/thing', {})).not.toThrow(); | ||
| }); | ||
|
|
||
| test("getPeerSettings() reads the server's capabilities.extensions[id] from initialize result", async () => { | ||
| const PeerSchema = z.object({ availableDisplayModes: z.array(z.string()) }); | ||
| const client = new Client({ name: 'c', version: '1.0.0' }); | ||
| const handle = client.extension('io.example/ui', { clientSide: true }, { peerSchema: PeerSchema }); | ||
|
|
||
| expect(handle.getPeerSettings()).toBeUndefined(); | ||
|
|
||
| const harness = rawServer({ | ||
| extensions: { 'io.example/ui': { availableDisplayModes: ['inline', 'fullscreen'] } } | ||
| }); | ||
| await client.connect(harness.serverSide); | ||
|
|
||
| expect(handle.getPeerSettings()).toEqual({ availableDisplayModes: ['inline', 'fullscreen'] }); | ||
| }); | ||
|
|
||
| test('getPeerSettings() reflects reconnect to a different server', async () => { | ||
| const client = new Client({ name: 'c', version: '1.0.0' }); | ||
| const handle = client.extension('io.example/ui', {}); | ||
|
|
||
| const harnessA = rawServer({ extensions: { 'io.example/ui': { v: 1 } } }); | ||
| await client.connect(harnessA.serverSide); | ||
| expect(handle.getPeerSettings()).toEqual({ v: 1 }); | ||
|
|
||
| await client.close(); | ||
|
|
||
| const harnessB = rawServer({ extensions: { 'io.example/ui': { v: 2 } } }); | ||
| await client.connect(harnessB.serverSide); | ||
| expect(handle.getPeerSettings()).toEqual({ v: 2 }); | ||
| }); | ||
| }); |
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.
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.
🟡 After calling extension('io.a', {v:1}), a subsequent registerCapabilities({extensions: {'io.a': {different: true}}}) silently overwrites the wire capabilities for that extension, while handle.settings still holds the original {v:1} — violating the documented invariant that handle.settings is 'The local settings object advertised in capabilities.extensions[id]'. The inconsistency is compounded by an API asymmetry: calling extension() twice for the same ID throws 'already registered', but registerCapabilities() can silently overwrite it.
Extended reasoning...
What the bug is and how it manifests
extension(id, settings) creates an ExtensionHandle and sets _capabilities.extensions[id] = settings. The returned handle stores settings as a readonly field, with the JSDoc contract: 'The local settings object advertised in capabilities.extensions[id]'. However, registerCapabilities() delegates to mergeCapabilities(), which does a one-level-deep object spread: { ...base.extensions, ...additional.extensions }. This means any extension ID present in both base and additional is overwritten at the entry level, not merged.
The specific code path that triggers it
Why existing code does not prevent it
The extension() method guards against duplicate registrations with Object.hasOwn(this._capabilities.extensions, id) and throws 'already registered'. But this guard only covers the extension() to extension() path. The registerCapabilities() path has no such guard and calls mergeCapabilities() unconditionally, which can silently stomp any previously-registered extension entry.
Addressing the refutation
The refutation notes the scenario is implausible in normal usage — no sane caller would call extension('io.a', {v:1}) and then immediately call registerCapabilities({extensions: {'io.a': {v:2}}}). This is a fair point for the common case. However: (a) the API contract expressed in JSDoc is unambiguously violated, (b) the asymmetry where extension() throws on duplicates while registerCapabilities() silently overwrites is a genuine footgun for callers who compose multiple libraries that each call registerCapabilities(), and (c) the failure is silent with no warning. The scenario is unusual but the contract violation is real. Severity: nit.
Impact
Any caller reading handle.settings after a registerCapabilities() call that overlapped the extension ID will see stale local data while the peer was advertised something different. Extension-aware logic relying on handle.settings as the source of truth would be silently wrong.
How to fix it
The simplest fix is to add an overlap check in registerCapabilities() for IDs already registered via extension(). A cleaner approach is to maintain a _registeredExtensionIds Set and throw (or at minimum warn) in registerCapabilities() when the incoming extensions object overlaps that set. Alternatively, document clearly that registerCapabilities() can override extension settings registered via extension(), downgrading the JSDoc invariant so callers are not misled.
Step-by-step proof