Skip to content

Commit 63335f1

Browse files
fix(core): address review findings on ExtensionHandle
- sendRequest/sendNotification are now async so the strict-mode CapabilityNotSupported throw surfaces as a rejection (catchable via .catch()) instead of escaping synchronously - getPeerSettings() no longer caches; reads current peer capabilities on each call so it reflects close()/connect() to a different peer - Client.extension()/Server.extension() throw on duplicate id - add packages/client/test/client/extension.test.ts covering the client-side initialize round-trip and reconnect behavior
1 parent e7743b0 commit 63335f1

6 files changed

Lines changed: 155 additions & 49 deletions

File tree

packages/client/src/client/client.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,9 @@ export class Client extends Protocol<ClientContext> {
335335
if (this.transport) {
336336
throw new SdkError(SdkErrorCode.AlreadyConnected, 'Cannot register extension after connecting to transport');
337337
}
338+
if (this._capabilities.extensions && Object.hasOwn(this._capabilities.extensions, id)) {
339+
throw new Error(`Extension "${id}" is already registered`);
340+
}
338341
this._capabilities.extensions = { ...this._capabilities.extensions, [id]: settings };
339342
return new ExtensionHandle(
340343
this,
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
import { InMemoryTransport, type JSONRPCMessage, SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
2+
import { describe, expect, test } from 'vitest';
3+
import * as z from 'zod/v4';
4+
5+
import { Client } from '../../src/client/client.js';
6+
7+
/**
8+
* These tests exercise the `Client.extension()` factory and the client side of the
9+
* `capabilities.extensions` round-trip via `initialize`. The `ExtensionHandle` class itself is
10+
* unit-tested in `@modelcontextprotocol/core/test/shared/extensionHandle.test.ts`.
11+
*/
12+
13+
interface RawServerHarness {
14+
serverSide: InMemoryTransport;
15+
capturedInitParams: Promise<Record<string, unknown>>;
16+
}
17+
18+
function rawServer(serverCapabilities: Record<string, unknown> = {}): RawServerHarness {
19+
const [clientSide, serverSide] = InMemoryTransport.createLinkedPair();
20+
let resolveInit: (p: Record<string, unknown>) => void;
21+
const capturedInitParams = new Promise<Record<string, unknown>>(r => {
22+
resolveInit = r;
23+
});
24+
serverSide.onmessage = (msg: JSONRPCMessage) => {
25+
if ('method' in msg && msg.method === 'initialize' && 'id' in msg) {
26+
resolveInit((msg.params ?? {}) as Record<string, unknown>);
27+
void serverSide.send({
28+
jsonrpc: '2.0',
29+
id: msg.id,
30+
result: {
31+
protocolVersion: '2025-11-25',
32+
capabilities: serverCapabilities,
33+
serverInfo: { name: 'raw-server', version: '0.0.0' }
34+
}
35+
});
36+
}
37+
};
38+
void serverSide.start();
39+
// Expose clientSide via the harness's serverSide.peer for the test to connect to.
40+
return { serverSide: clientSide, capturedInitParams };
41+
}
42+
43+
describe('Client.extension()', () => {
44+
test('merges settings into capabilities.extensions and advertises them in initialize request', async () => {
45+
const client = new Client({ name: 'c', version: '1.0.0' }, { capabilities: {} });
46+
client.extension('io.example/ui', { contentTypes: ['text/html'] });
47+
client.extension('com.acme/widgets', { v: 2 });
48+
49+
const harness = rawServer();
50+
await client.connect(harness.serverSide);
51+
const initParams = await harness.capturedInitParams;
52+
53+
const caps = initParams.capabilities as Record<string, unknown>;
54+
expect(caps.extensions).toEqual({
55+
'io.example/ui': { contentTypes: ['text/html'] },
56+
'com.acme/widgets': { v: 2 }
57+
});
58+
});
59+
60+
test('throws AlreadyConnected after connect()', async () => {
61+
const client = new Client({ name: 'c', version: '1.0.0' });
62+
const harness = rawServer();
63+
await client.connect(harness.serverSide);
64+
65+
expect(() => client.extension('io.example/ui', {})).toThrow(SdkError);
66+
try {
67+
client.extension('io.example/ui', {});
68+
expect.fail('should have thrown');
69+
} catch (e) {
70+
expect(e).toBeInstanceOf(SdkError);
71+
expect((e as SdkError).code).toBe(SdkErrorCode.AlreadyConnected);
72+
}
73+
});
74+
75+
test('throws on duplicate extension id', () => {
76+
const client = new Client({ name: 'c', version: '1.0.0' });
77+
client.extension('io.example/ui', { v: 1 });
78+
expect(() => client.extension('io.example/ui', { v: 2 })).toThrow(/already registered/);
79+
expect(() => client.extension('com.other/thing', {})).not.toThrow();
80+
});
81+
82+
test("getPeerSettings() reads the server's capabilities.extensions[id] from initialize result", async () => {
83+
const PeerSchema = z.object({ availableDisplayModes: z.array(z.string()) });
84+
const client = new Client({ name: 'c', version: '1.0.0' });
85+
const handle = client.extension('io.example/ui', { clientSide: true }, { peerSchema: PeerSchema });
86+
87+
expect(handle.getPeerSettings()).toBeUndefined();
88+
89+
const harness = rawServer({
90+
extensions: { 'io.example/ui': { availableDisplayModes: ['inline', 'fullscreen'] } }
91+
});
92+
await client.connect(harness.serverSide);
93+
94+
expect(handle.getPeerSettings()).toEqual({ availableDisplayModes: ['inline', 'fullscreen'] });
95+
});
96+
97+
test('getPeerSettings() reflects reconnect to a different server', async () => {
98+
const client = new Client({ name: 'c', version: '1.0.0' });
99+
const handle = client.extension('io.example/ui', {});
100+
101+
const harnessA = rawServer({ extensions: { 'io.example/ui': { v: 1 } } });
102+
await client.connect(harnessA.serverSide);
103+
expect(handle.getPeerSettings()).toEqual({ v: 1 });
104+
105+
await client.close();
106+
107+
const harnessB = rawServer({ extensions: { 'io.example/ui': { v: 2 } } });
108+
await client.connect(harnessB.serverSide);
109+
expect(handle.getPeerSettings()).toEqual({ v: 2 });
110+
});
111+
});

packages/core/src/shared/extensionHandle.ts

Lines changed: 13 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,6 @@ export interface ExtensionOptions<P extends AnySchema> {
5757
* {@linkcode getPeerSettings} returns `undefined`.
5858
*/
5959
export class ExtensionHandle<Local extends JSONObject, Peer = JSONObject, ContextT extends BaseContext = BaseContext> {
60-
private _peerSettingsCache?: { value: Peer | undefined };
61-
6260
/**
6361
* @internal Use `Client.extension()` or `Server.extension()` to construct.
6462
*/
@@ -76,33 +74,25 @@ export class ExtensionHandle<Local extends JSONObject, Peer = JSONObject, Contex
7674
/**
7775
* Returns the peer's `capabilities.extensions[id]` settings, or `undefined` if the peer did not
7876
* advertise this extension or (when `peerSchema` was provided) if the peer's blob fails
79-
* validation. The result is parsed once and cached.
77+
* validation. Reads the current peer capabilities on each call (no caching), so it reflects
78+
* reconnects.
8079
*/
8180
getPeerSettings(): Peer | undefined {
82-
if (this._peerSettingsCache) {
83-
return this._peerSettingsCache.value;
84-
}
8581
const raw = this._getPeerExtensionSettings();
8682
if (raw === undefined) {
87-
// Don't cache: peer may not have connected yet.
8883
return undefined;
8984
}
90-
let value: Peer | undefined;
9185
if (this._peerSchema === undefined) {
92-
value = raw as Peer;
93-
} else {
94-
const parsed = parseSchema(this._peerSchema, raw);
95-
if (parsed.success) {
96-
value = parsed.data as Peer;
97-
} else {
98-
console.warn(
99-
`[ExtensionHandle] Peer's capabilities.extensions["${this.id}"] failed schema validation: ${parsed.error.message}`
100-
);
101-
value = undefined;
102-
}
86+
return raw as Peer;
87+
}
88+
const parsed = parseSchema(this._peerSchema, raw);
89+
if (!parsed.success) {
90+
console.warn(
91+
`[ExtensionHandle] Peer's capabilities.extensions["${this.id}"] failed schema validation: ${parsed.error.message}`
92+
);
93+
return undefined;
10394
}
104-
this._peerSettingsCache = { value };
105-
return value;
95+
return parsed.data as Peer;
10696
}
10797

10898
/**
@@ -137,7 +127,7 @@ export class ExtensionHandle<Local extends JSONObject, Peer = JSONObject, Contex
137127
* `capabilities.extensions[id]`, throws {@linkcode SdkError} with
138128
* {@linkcode SdkErrorCode.CapabilityNotSupported}.
139129
*/
140-
sendRequest<R extends AnySchema>(
130+
async sendRequest<R extends AnySchema>(
141131
method: string,
142132
params: Record<string, unknown> | undefined,
143133
resultSchema: R,
@@ -154,7 +144,7 @@ export class ExtensionHandle<Local extends JSONObject, Peer = JSONObject, Contex
154144
* `capabilities.extensions[id]`, throws {@linkcode SdkError} with
155145
* {@linkcode SdkErrorCode.CapabilityNotSupported}.
156146
*/
157-
sendNotification(method: string, params?: Record<string, unknown>, options?: NotificationOptions): Promise<void> {
147+
async sendNotification(method: string, params?: Record<string, unknown>, options?: NotificationOptions): Promise<void> {
158148
this._assertPeerCapability(method);
159149
return this._host.sendCustomNotification(method, params, options);
160150
}

packages/core/test/shared/extensionHandle.test.ts

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -66,26 +66,20 @@ describe('ExtensionHandle.getPeerSettings', () => {
6666
warn.mockRestore();
6767
});
6868

69-
test('caches the parsed result once peer has advertised', () => {
70-
const getter = vi.fn().mockReturnValue({ a: 1 });
71-
const host = makeMockHost() as unknown as ExtensionHost<BaseContext>;
72-
const handle = new ExtensionHandle(host, 'io.example/ui', {}, getter, false);
73-
handle.getPeerSettings();
74-
handle.getPeerSettings();
75-
handle.getPeerSettings();
76-
expect(getter).toHaveBeenCalledTimes(1);
77-
});
78-
79-
test('does not cache undefined (so a later-connecting peer is observable)', () => {
69+
test('reflects current peer settings on each call (no caching across reconnects)', () => {
8070
let peer: JSONObject | undefined;
8171
const getter = vi.fn(() => peer);
8272
const host = makeMockHost() as unknown as ExtensionHost<BaseContext>;
8373
const handle = new ExtensionHandle(host, 'io.example/ui', {}, getter, false);
74+
75+
expect(handle.getPeerSettings()).toBeUndefined();
76+
peer = { v: 1 };
77+
expect(handle.getPeerSettings()).toEqual({ v: 1 });
78+
peer = { v: 2 };
79+
expect(handle.getPeerSettings()).toEqual({ v: 2 });
80+
peer = undefined;
8481
expect(handle.getPeerSettings()).toBeUndefined();
85-
peer = { now: 'connected' };
86-
expect(handle.getPeerSettings()).toEqual({ now: 'connected' });
87-
expect(handle.getPeerSettings()).toEqual({ now: 'connected' });
88-
expect(getter).toHaveBeenCalledTimes(2);
82+
expect(getter).toHaveBeenCalledTimes(4);
8983
});
9084
});
9185

@@ -115,19 +109,17 @@ describe('ExtensionHandle.sendRequest / sendNotification — peer gating', () =>
115109
expect(host.sendCustomNotification).toHaveBeenCalledWith('ui/ping', {}, undefined);
116110
});
117111

118-
test('strict mode: throws CapabilityNotSupported when peer did not advertise', () => {
112+
test('strict mode: rejects with CapabilityNotSupported when peer did not advertise', async () => {
119113
const { host, handle } = makeHandle({ peer: undefined, strict: true });
120-
let thrown: unknown;
121-
try {
122-
void handle.sendRequest('ui/do', {}, Result);
123-
} catch (e) {
124-
thrown = e;
125-
}
126-
expect(thrown).toBeInstanceOf(SdkError);
127-
expect((thrown as SdkError).code).toBe(SdkErrorCode.CapabilityNotSupported);
128-
expect((thrown as SdkError).message).toMatch(/io\.example\/ui.*ui\/do/);
114+
await expect(handle.sendRequest('ui/do', {}, Result)).rejects.toSatisfy(
115+
(e: unknown) =>
116+
e instanceof SdkError && e.code === SdkErrorCode.CapabilityNotSupported && /io\.example\/ui.*ui\/do/.test(e.message)
117+
);
129118
expect(host.sendCustomRequest).not.toHaveBeenCalled();
130-
expect(() => handle.sendNotification('ui/ping')).toThrow(SdkError);
119+
await expect(handle.sendNotification('ui/ping')).rejects.toSatisfy(
120+
(e: unknown) => e instanceof SdkError && e.code === SdkErrorCode.CapabilityNotSupported
121+
);
122+
expect(host.sendCustomNotification).not.toHaveBeenCalled();
131123
});
132124

133125
test('strict mode: sends when peer did advertise', async () => {

packages/server/src/server/server.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,9 @@ export class Server extends Protocol<ServerContext> {
249249
if (this.transport) {
250250
throw new SdkError(SdkErrorCode.AlreadyConnected, 'Cannot register extension after connecting to transport');
251251
}
252+
if (this._capabilities.extensions && Object.hasOwn(this._capabilities.extensions, id)) {
253+
throw new Error(`Extension "${id}" is already registered`);
254+
}
252255
this._capabilities.extensions = { ...this._capabilities.extensions, [id]: settings };
253256
return new ExtensionHandle(
254257
this,

packages/server/test/server/extension.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,13 @@ describe('Server.extension()', () => {
6868
}
6969
});
7070

71+
test('throws on duplicate extension id', () => {
72+
const server = new Server({ name: 's', version: '1.0.0' });
73+
server.extension('io.example/ui', { v: 1 });
74+
expect(() => server.extension('io.example/ui', { v: 2 })).toThrow(/already registered/);
75+
expect(() => server.extension('com.other/thing', {})).not.toThrow();
76+
});
77+
7178
test("getPeerSettings() reads the client's capabilities.extensions[id] after initialize", async () => {
7279
const PeerSchema = z.object({ availableDisplayModes: z.array(z.string()) });
7380
const server = new Server({ name: 's', version: '1.0.0' });

0 commit comments

Comments
 (0)