feat(core): add extension() registrar for SEP-2133 capability-aware custom methods#1868
Conversation
🦋 Changeset detectedLatest commit: 63335f1 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
2 times, most recently
from
714c236 to
3539ddd
Compare
…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
felixweinberger
force-pushed
the
fweinberger/extension-registrar
branch
2 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.
| } | ||
| if (this._capabilities.extensions && Object.hasOwn(this._capabilities.extensions, id)) { | ||
| throw new Error(`Extension "${id}" is already registered`); |
There was a problem hiding this comment.
🟡 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
- client.extension('io.a', {v:1}) sets _capabilities.extensions = {'io.a': {v:1}} and handle.settings = {v:1}
- client.registerCapabilities({extensions: {'io.a': {different: true}}}) calls mergeCapabilities which spreads extensions, resulting in _capabilities.extensions = {'io.a': {different: true}}
- handle.settings still returns {v:1} (captured at construction time)
- On connect(), the server receives {different: true} for 'io.a' in the initialize request, not {v:1}
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
- const client = new Client({name: 'c', version: '1'})
- const handle = client.extension('io.a', {v: 1}) — _capabilities.extensions = {'io.a': {v:1}}, handle.settings = {v:1}
- client.registerCapabilities({extensions: {'io.a': {different: true}}}) — mergeCapabilities spreads extensions at the top level, result: _capabilities.extensions = {'io.a': {different: true}}
- handle.settings still returns {v:1} — stale
- await client.connect(transport) — initialize params contain capabilities.extensions = {'io.a': {different: true}}
- Server receives {different: true}; handle.settings still says {v:1}. JSDoc invariant broken.
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.