feat(core): add extension() registrar for SEP-2133 capability-aware custom methods#1868
Conversation
🦋 Changeset detectedLatest commit: 9434b46 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
Note: this PR depends on #1846 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/extension-registrar
branch
4 times, most recently
from
ef775f7 to
63335f1
Compare
|
@claude review |
1 similar comment
|
@claude review |
felixweinberger
force-pushed
the
fweinberger/extension-registrar
branch
from
ef775f7 to
63335f1
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
packages/client/test/client/barrelClean.test.ts:10-13— The 'chunks transitively imported by dist/index.mjs' test only performs a one-level-deep import scan —chunkImportsOf()reads and regex-matches the entry file's direct.mjsimports but never recurses into those chunks to check their own imports. The test description claims transitive coverage but the implementation is shallow; if a shared chunk directly reachable from index.mjs itself imported another chunk containingchild_processorcross-spawn, the test would pass silently. The same issue exists in the identical helper inpackages/server/test/server/barrelClean.test.ts.Extended reasoning...
What the bug is and how it manifests
The
chunkImportsOf()helper (packages/client/test/client/barrelClean.test.ts lines 10-13) reads a single file and extracts relative.mjsimports via a regex:function chunkImportsOf(entryPath: string): string[] { const src = readFileSync(entryPath, 'utf8'); return [...src.matchAll(/from\s+["']\.\/(.+?\.mjs)["']/g)].map(m => join(distDir, m[1]\!)); }
The caller feeds it
dist/index.mjsand iterates over the result to check each chunk forNODE_ONLYpatterns. The function is never called recursively on the discovered chunks. Despite this, the test is named "chunks transitively imported by dist/index.mjs contain no process-spawning runtime imports" — a description that implies multi-level traversal.The specific code path that triggers it
If the bundler produces a two-level chain — index.mjs → chunk-A.mjs → chunk-B.mjs — where chunk-B.mjs contains
node:child_processorcross-spawn, then:chunkImportsOf('\''dist/index.mjs'\'')returns['\''dist/chunk-A.mjs'\'']- The loop checks chunk-A.mjs — passes (no banned patterns there)
- chunk-B.mjs is never read or checked
- The test reports success while the banned dependency is reachable from index.mjs
Why the refutation does not fully resolve this
One verifier argued that tsdown/rollup in this configuration produces flat shared chunks that are leaves and do not import other chunk files, so the one-level check is functionally adequate. This is plausible for the current output. However:
- The claim is a property of the current build output, not enforced by the test or the bundler config. With 6 entry points and
noExternal: ['\''@modelcontextprotocol/core'\''], the bundler may produce multi-level chunk chains if the module graph changes (new entry points, deeper shared dependencies, changed split heuristics). - The test description actively misleads future maintainers: a developer who trusts the label "transitively imported" will not know the guard has a depth of 1.
- Both packages share identical shallow implementations, doubling the surface where this assumption could silently break.
What the impact would be
If the bundle structure changes in a future PR and a multi-level chunk chain forms that includes a Node-only import, this test would continue to pass, defeating the entire purpose of the browser-safety guard.
How to fix it
Replace the single-level helper with a BFS/DFS traversal:
function chunksTransitivelyImportedBy(entryPath: string): string[] { const visited = new Set<string>(); const queue = [entryPath]; while (queue.length > 0) { const file = queue.shift()\!; if (visited.has(file)) continue; visited.add(file); const src = readFileSync(file, '\''utf8'\''); for (const m of src.matchAll(/from\s+["']\.\/(.+?\.mjs)["']/g)) { queue.push(join(distDir, m[1]\!)); } } visited.delete(entryPath); return [...visited]; }
This is a small, mechanical change that makes the test match its documented behavior and is resilient to future bundler output changes.
Step-by-step proof
- Bundler emits:
index.mjs(imports./chunk-A.mjs),chunk-A.mjs(imports./chunk-B.mjs),chunk-B.mjs(containsimport '\''node:child_process'\''). chunkImportsOf('\''dist/index.mjs'\'')regex-matches index.mjs and returns['\''dist/chunk-A.mjs'\''].- Loop reads chunk-A.mjs, finds no
NODE_ONLYmatch — assertion passes. - chunk-B.mjs is never opened.
node:child_processis reachable from the browser-targeted root entry but the test reports green.
-
🔴
packages/core/src/exports/public/index.ts:77— The top commit (ef775f7) is explicitly labeled DO NOT PUSH and re-addsexport { InMemoryTransport } from '../../util/inMemory.js'topackages/core/src/exports/public/index.ts(line 77). Because both@modelcontextprotocol/clientand@modelcontextprotocol/serverre-export this file viaexport * from '@modelcontextprotocol/core/public', merging as-is would silently re-exposeInMemoryTransportin the stable public API of both packages — directly contradictingdocs/migration.mdin this very PR, which states it was intentionally removed.Extended reasoning...
What the bug is and how it manifests
The HEAD commit ef775f7 ('chore(local): re-add InMemoryTransport to public for ext-apps test linking (DO NOT PUSH)') adds one line to
packages/core/src/exports/public/index.tsat line 77:export { InMemoryTransport } from '../../util/inMemory.js';
This line was added as a local workaround to make the integration test files in
packages/client/test/client/extension.test.tsandpackages/server/test/server/extension.test.tsable to importInMemoryTransportvia@modelcontextprotocol/core. The commit message explicitly warns it must not be pushed.The specific code path that triggers it
packages/core/src/exports/public/index.tsis the file pointed to by the@modelcontextprotocol/core/publicsubpath export. Bothpackages/client/src/index.tsandpackages/server/src/index.tsend with:export * from '@modelcontextprotocol/core/public';
So any symbol added to
public/index.tsimmediately becomes part of the stable public API of both user-facing packages. The accidental line at line 77 ofpublic/index.tstherefore re-exportsInMemoryTransportfrom both@modelcontextprotocol/clientand@modelcontextprotocol/server.Why existing guards do not catch it
The
barrelClean.test.tstests in client and server only check for Node.js process-spawning imports (child_process,node:stream, etc.) — they do not verify the absence ofInMemoryTransport. The integration tests that needInMemoryTransportimport it from@modelcontextprotocol/core(the internal barrel), not from the public surface, so the DO NOT PUSH line was added purely to satisfy a TypeScript module resolution issue during development.What the impact would be
This PR's own
docs/migration.md(added in this same PR) explicitly states:InMemoryTransporthas been removed from the public API surface. ...@modelcontextprotocol/coreis internal, not for production use.Merging this commit would directly contradict that documentation and the design decision to keep
InMemoryTransportinternal. Any downstream package that imports it from@modelcontextprotocol/clientor@modelcontextprotocol/serverwould take a hard dependency on an internal API, and the SDK team would be unable to remove it later without a semver major bump.Step-by-step proof
packages/core/src/exports/public/index.tsline 77:export { InMemoryTransport } from '../../util/inMemory.js';— present in the repo at HEAD.packages/client/src/index.tslast line:export * from '@modelcontextprotocol/core/public';— re-exports everything including line 77.packages/server/src/index.tslast line:export * from '@modelcontextprotocol/core/public';— same.- Result:
import { InMemoryTransport } from '@modelcontextprotocol/client'andimport { InMemoryTransport } from '@modelcontextprotocol/server'both resolve — the symbol is part of the stable public API. docs/migration.md(this PR): 'InMemoryTransport has been removed from the public API surface' — direct contradiction.
How to fix it
Remove the line
export { InMemoryTransport } from '../../util/inMemory.js';frompackages/core/src/exports/public/index.tsbefore merging. The integration tests inextension.test.tsalready importInMemoryTransportfrom@modelcontextprotocol/core(the internal barrel, not the public path), so removing this line does not break any test.
|
@claude review |
There was a problem hiding this comment.
Thanks for addressing the two previous findings — both the async-throw fix and the removal of the stale cache are confirmed in the current commit. This introduces a new public API surface stacked on #1846, so it merits a human look before merging.
Extended reasoning...
Overview\nThis PR adds / returning an that merges settings into , exposes , and gates / on peer capability under . It touches (new), , , , and public export barrel files — 10 files in total, all additive.\n\n### Prior findings resolved\nBoth 🔴 issues from the first review are confirmed fixed in commit :\n- and are now declared , so capability errors reject through the Promise chain instead of throwing synchronously.\n- calls on every invocation with no intermediate caching; reconnect reflectivity is verified by a dedicated test.\n\n### Remaining bug (nit, posted as inline comment)\nA new nit has been flagged: silently overwrites the wire capabilities for an extension previously registered via , leaving stale relative to what the peer receives. The asymmetry with the duplicate-ID guard in (which throws) makes this a mild API footgun. Severity is nit — the scenario requires an unusual two-step setup — but the JSDoc invariant is technically violated.\n\n### Security risks\nNone. The changes are additive capability negotiation helpers with no auth, crypto, or permission paths involved.\n\n### Level of scrutiny\nModerate-high: this is a new, stable public API (, , ) that will be hard to change once released. The stacked dependency on #1846 means reviewers should verify the custom-method foundation is also in good shape before landing. Test coverage is solid (unit + integration), but the design choices (overload signatures, wiring, validation semantics) benefit from a human second opinion.\n\n### Other factors\nThe PR is additive with no breaking changes. Documentation and changeset are included. The author has been responsive to review feedback.
|
On the two "Additional findings (outside current diff)" items above:
|
|
@claude review |
|
@claude review |
…ustom methods
Adds Client.extension(id, settings, {peerSchema?}) and Server.extension(...)
returning an ExtensionHandle that:
- merges settings into capabilities.extensions[id] (advertised in initialize)
- exposes getPeerSettings() with optional schema validation of the peer blob
- wraps setCustom*/sendCustom* with peer-capability gating under
enforceStrictCapabilities
Connects the SEP-2133 capabilities.extensions field to the custom-method API
from #1846. Declare-before-register is structural (you cannot get a handle
without declaring); peer-gating on send mirrors assertCapabilityForMethod.
Stacked on #1846.
…ion in ExtensionHandle JSDoc
- 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
…rCapabilities/extension ordering
ExtensionHandle now reads enforceStrictCapabilities via a getter (matching
the existing _getPeerExtensionSettings pattern), so a future setter on the
host would be observed. JSDoc on extension() notes that a later
registerCapabilities({extensions:{id:...}}) overwrites the wire value while
handle.settings reflects the original call.
…ck pre-connect - duplicate-ID guard now throws SdkError(ExtensionAlreadyRegistered) instead of plain Error - _assertPeerCapability defers when peer capabilities are not yet populated, so strict-mode sendRequest before connect() surfaces NotConnected (from the underlying send) rather than a misleading CapabilityNotSupported
Calls peerSchema['~standard'].validate directly (sync path); falls back to raw blob with a warning if the schema's validation is async, since getPeerSettings() is a synchronous read of already-received capabilities.
felixweinberger
force-pushed
the
fweinberger/extension-registrar
branch
from
9a22a16 to
641cc7a
Compare
|
@claude review |
| private _assertPeerCapability(method: string): void { | ||
| // If peer capabilities are not yet populated (pre-connect), defer to the | ||
| // NotConnected error from the underlying send path rather than misreporting | ||
| // CapabilityNotSupported. | ||
| if (!this._getPeerCapabilitiesPresent()) return; | ||
| if (this._getEnforceStrictCapabilities() && this._getPeerExtensionSettings() === undefined) { | ||
| throw new SdkError( | ||
| SdkErrorCode.CapabilityNotSupported, | ||
| `Peer does not support extension "${this.id}" (required for ${method})` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 nit: The 9a22a16 fix only covers the pre-first-connect path — _getPeerCapabilitiesPresent() keys off this._serverCapabilities \!== undefined (and _clientCapabilities on Server), but neither field is cleared by close()/_onclose(). So after await client.close(), in strict mode handle.sendRequest() still throws CapabilityNotSupported against the previous peer's caps instead of surfacing NotConnected. Consider keying the getter off live transport state (() => this.transport \!== undefined) to mirror the ordering _requestWithSchema already uses.
Extended reasoning...
What the bug is
Commit 9a22a16 added _getPeerCapabilitiesPresent() so that, before the first connect(), _assertPeerCapability defers to the underlying NotConnected error rather than misreporting CapabilityNotSupported. The getter is wired (client.ts:351, server.ts:265) as () => this._serverCapabilities \!== undefined / () => this._clientCapabilities \!== undefined. However, Protocol._onclose() clears _transport, response handlers, etc. but does not clear _serverCapabilities/_clientCapabilities (those live on Client/Server and are only ever assigned during initialize — never reset). Neither Client nor Server overrides close(). So after close(), _getPeerCapabilitiesPresent() continues to return true and _assertPeerCapability runs the capability check against stale data from the previous connection.
The specific code path that triggers it
In ExtensionHandle._assertPeerCapability (extensionHandle.ts:158–168):
if (\!this._getPeerCapabilitiesPresent()) return; // ← still true after close()
if (this._getEnforceStrictCapabilities() && this._getPeerExtensionSettings() === undefined) {
throw new SdkError(SdkErrorCode.CapabilityNotSupported, ...);
}After close(), the first guard is bypassed because _serverCapabilities still holds the previous peer's caps. If the previous peer did not advertise the extension, _getPeerExtensionSettings() is undefined and the strict-mode throw fires before the call ever reaches sendCustomRequest → _requestWithSchema, which would otherwise surface NotConnected.
Why existing code doesn't prevent it
For standard MCP methods, Protocol._requestWithSchema checks \!this._transport before assertCapabilityForMethod (protocol.ts ~813–820), so post-close standard requests always reject with NotConnected regardless of stale caps. ExtensionHandle inverts this ordering by running its capability check first. The 9a22a16 short-circuit only restores the standard ordering for the never-connected case; it leaves the post-close case behaving like the pre-fix code.
Impact
In strict mode, calling handle.sendRequest()/sendNotification() after close() (and before any reconnect) when the previous peer did not advertise the extension yields SdkError(CapabilityNotSupported, 'Peer does not support extension ...') instead of NotConnected. This is the exact diagnostic confusion the fix was meant to eliminate, just on the post-close path. It's a narrow edge case (strict mode + previous-peer-didn't-advertise + send-while-disconnected) and only affects error diagnostics, not functional correctness — hence nit severity.
How to fix it
Key the presence check off live transport state, which _onclose() does clear:
// client.ts / server.ts
() => this.transport \!== undefined,This exactly mirrors the \!this._transport check that _requestWithSchema performs first, so ExtensionHandle's capability gate runs only when (and against the peer for which) a live connection exists. Alternatively, clear _serverCapabilities/_clientCapabilities in an _onclose() override, but that's a wider behavior change.
Step-by-step proof
const client = new Client({name:'c',version:'1'}, {enforceStrictCapabilities:true})const handle = client.extension('io.x/ext', {})await client.connect(transportA)— server A responds withcapabilities: {}(noextensionskey)._serverCapabilities = {}.await client.close()—Protocol._onclose()sets_transport = undefined;_serverCapabilitiesis still{}.await handle.sendRequest('x/do', {}, Result):_assertPeerCapability('x/do')→_getPeerCapabilitiesPresent()=({}) \!== undefined= true → guard not taken._getEnforceStrictCapabilities()= true;_getPeerExtensionSettings()={}?.extensions?.['io.x/ext']=undefined.- Throws
SdkError(CapabilityNotSupported, 'Peer does not support extension "io.x/ext" ...').
- Expected (and what standard methods do): rejection with
NotConnected, since_transport === undefined. - With the proposed fix
() => this.transport \!== undefined, step 5's first guard evaluates tofalse,_assertPeerCapabilityreturns, andsendCustomRequest → _requestWithSchemarejects withNotConnected✓.
| // State errors | ||
| /** Transport is not connected */ | ||
| NotConnected = 'NOT_CONNECTED', | ||
| ExtensionAlreadyRegistered = 'EXTENSION_ALREADY_REGISTERED', |
There was a problem hiding this comment.
🟡 The new ExtensionAlreadyRegistered member is missing a JSDoc comment and is wedged between NotConnected and the /** Transport is already connected */ comment for AlreadyConnected, breaking up an obvious pair. Consider adding a JSDoc line (e.g. /** Extension ID is already registered via Client.extension()/Server.extension() */) and moving it after AlreadyConnected.
Extended reasoning...
What the issue is
The new enum member at line 13 is inserted without a JSDoc comment, while every other member in the // State errors group (NotConnected, AlreadyConnected, NotInitialized) has one:
// State errors
/** Transport is not connected */
NotConnected = 'NOT_CONNECTED',
ExtensionAlreadyRegistered = 'EXTENSION_ALREADY_REGISTERED',
/** Transport is already connected */
AlreadyConnected = 'ALREADY_CONNECTED',
/** Protocol is not initialized */
NotInitialized = 'NOT_INITIALIZED',The placement also splits the obvious NotConnected/AlreadyConnected pair, which reads oddly when scanning the source.
Why this is only a nit
There is no functional or tooling impact: the /** Transport is already connected */ comment still sits directly above AlreadyConnected, so TypeDoc/IDE hover correctly attributes it. Also, the file is not perfectly uniform — the ClientHttp* members at lines 32–37 also lack JSDoc — so the original claim that every member is documented overstates things. But within the State-errors group the pattern is consistent, and this PR also documents ExtensionAlreadyRegistered in docs/migration.md's SdkErrorCode table only by implication (it's actually not listed there either), so a one-liner here is the only place a reader will find what it means.
Step-by-step proof
- Open
packages/core/src/errors/sdkErrors.tsat the// State errorsgroup. - Line 11–12:
/** Transport is not connected */→NotConnected. Documented. - Line 13:
ExtensionAlreadyRegistered = 'EXTENSION_ALREADY_REGISTERED',— no preceding/** ... */. Undocumented. - Line 14–15:
/** Transport is already connected */→AlreadyConnected. Documented, and still correctly attached (comment is immediately above its target). - Line 16–17:
/** Protocol is not initialized */→NotInitialized. Documented. - Result: 3/4 members in the group documented; the new one is the odd one out, and it sits between two transport-connection-state siblings rather than after them.
How to fix
// State errors
/** Transport is not connected */
NotConnected = 'NOT_CONNECTED',
/** Transport is already connected */
AlreadyConnected = 'ALREADY_CONNECTED',
/** Protocol is not initialized */
NotInitialized = 'NOT_INITIALIZED',
/** Extension ID is already registered via Client.extension()/Server.extension() */
ExtensionAlreadyRegistered = 'EXTENSION_ALREADY_REGISTERED',(Or keep it in the State-errors block but after AlreadyConnected — either way, add the comment.)
…ion() and custom methods
1 participant
Adds
Client.extension(id, settings, {peerSchema?})andServer.extension(...)returning anExtensionHandlethat mergessettingsintocapabilities.extensions[id], exposesgetPeerSettings()(optionally schema-validated), and wraps the*Custom*methods from #1846 with peer-capability gating underenforceStrictCapabilities.Stacked on #1846.
Motivation and Context
SEP-2133 added
capabilities.extensionsfor negotiating protocol extensions, but nothing in the SDK reads it. This connects that field to the custom-method API so extension authors declare once and the SDK handles advertisement and peer checks.How Has This Been Tested?
Unit tests (
extensionHandle.test.ts, 11), integration tests overInMemoryTransport(server/extension.test.ts, 4), andexamples/server/src/customMethodExample.ts (server) and examples/client/src/customMethodExample.ts (client)runs end-to-end showing both sides reading the other'sextensions[id].Breaking Changes
None — additive.
Types of changes
Checklist
Additional context
Flat
setCustom*/sendCustom*from #1846 remain available as the ungated path. Method-ownership tracking,removeExtension, and a typed method registry are intentionally deferred.