[v1.x] fix: allow registering after connect() when the capability was declared - #2958
Conversation
McpServer installed the tools, resources and prompts handlers on the first registration, and installing them registers the capability, which throws once a transport is connected. So a server that declared a capability in its options and registered its first tool after connect() got 'Cannot register capabilities after connecting to transport'. A capability declared in the constructor options now gets its handlers in the constructor, as on main (#2269). Registering after connect() then works, and a declared capability with nothing registered answers its list request with an empty list instead of 'Method not found'. A capability declared as {} is advertised with listChanged: true. Registering after connect() without having declared the capability still throws.
🦋 Changeset detectedLatest commit: 20cb3c5 The changes in this PR will be included in the next version bump. 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 |
commit: |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012VRbFCp41otcScXE1YY3es
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/server/mcp.ts— Servers that declare prompts (or resources) but not completions get a ghost prompt or template after a failed late registration: registerPrompt throws, yet clients see the item in prompts/list and get Method not found on completion/complete. src/server/mcp.ts:923 stores the prompt before src/server/mcp.ts:932 calls setCompletionRequestHandler, which throws after connect() via registerCapabilities. The handlers installed by src/server/mcp.ts:143-145 now serve that leftover entry; on the base branch nothing was installed, so the entry stayed invisible. Fix: store the prompt/template only after the completion handler is installed, or fail before mutating _registeredPrompts/_registeredResourceTemplates, so a thrown registration leaves no served item.Why this was flagged
A server built with
capabilities: { prompts: {} }(nocompletions) calls registerPrompt after connect() with an argsSchema containing a completable field, which is the late-registration case this PR enables. _createRegisteredPrompt writesthis._registeredPrompts[name]at src/server/mcp.ts:923, then src/server/mcp.ts:932 calls setCompletionRequestHandler, whose registerCapabilities at src/server/index.ts:201-202 throwsCannot register capabilities after connecting to transport. The caller sees a failure, but the constructor already installed the prompts/list and prompts/get handlers at src/server/mcp.ts:143-145, so clients now list and can get that prompt while completion/complete answers Method not found; a retry with the same name throwsPrompt ... is already registeredat src/server/mcp.ts:1216-1218. On the base branch, with no prompt registered before connect, the prompt handlers were never installed, so the leftover entry was never served.Verification: src/server/mcp.ts:923
this._registeredPrompts[name] = registeredPrompt;runs before line 932this.setCompletionRequestHandler(), which at line 477 callsthis.server.registerCapabilities({ completions: {} }); src/server/index.ts:201-202 throwsCannot register capabilities after connecting to transportonce a transport is attached. Nothing removes the stored entry on that throw. -
🟡
src/server/mcp.ts— Clients of servers that declare a capability and register their first item right after connect() now receive notifications/tools/list_changed before the initialize handshake has finished. src/server/mcp.ts:987-988 installs nothing new (handlers already exist from src/server/mcp.ts:137-145) and immediately calls sendToolListChanged, which src/server/mcp.ts:1266-1268 sends as soon as a transport is attached, with no check that the client has sent notifications/initialized. On the base branch this population got a throw instead of a pre-initialization notification. Fix: defer list_changed notifications until initialization completes (queue them and flush on oninitialized), for tools, resources and prompts alike.Why this was flagged
Trigger:
new McpServer(info, { capabilities: { tools: {} } }),await connect(stdioTransport), then registerTool in the same tick or before the client's initialize round-trip completes, the exact pattern this PR advertises as now working. src/server/mcp.ts:985-988 stores the tool and calls sendToolListChanged; src/server/mcp.ts:1267 only checksthis.server.transport !== undefined, and src/server/index.ts:643-644 plus src/shared/protocol.ts:1336-1443 write the notification straight to the transport. Nothing in Server tracks whether notifications/initialized has arrived, so a stdio client readsnotifications/tools/list_changedon stdout before the initialize result. On the base branch this population never reached src/server/mcp.ts:988: the first registration threw at setToolRequestHandlers, so no pre-initialization notification was emitted. Strict clients that reject messages before initialization fail the session; permissive ones re-list tools before they can. Remedy: queue list_changed notifications until oninitialized fires and flush them then.Verification: Trigger:
registerToolright afterawait connect(stdioTransport), before the client's initialize/initialized round-trip completes (on base the same call threw atregisterCapabilities). src/server/mcp.ts:987-988 runsthis.sendToolListChanged(); src/server/mcp.ts:1266-1269 gates only onisConnected(); src/shared/protocol.ts:1336-1341 never checks whethernotifications/initializedhas arrived. -
🟣
src/server/mcp.ts— Operators of servers that declare a capability and register items after connect() can now get an unhandled promise rejection, which terminates a Node process, when the transport's send rejects. src/server/mcp.ts:1268 calls this.server.sendToolListChanged() without await or catch; src/shared/protocol.ts:1443 awaitsthis._transport.sendon the non-debounced path, so a rejected send (an eventStore.storeEvent failure at src/server/webStandardStreamableHttp.ts:1143, or a stdout error) surfaces as an unhandled rejection. On the base branch the declared-capability late registration threw synchronously before reaching this line. Fix: attach a catch that routes the error to server.onerror at all three send*ListChanged sites (src/server/mcp.ts:1259, 1268, 1277).Why this was flagged
Trigger: a server declared with
capabilities: { tools: {} }connected over Streamable HTTP with an eventStore, registering a tool after connect() (the path this PR enables at src/server/mcp.ts:137-145). src/server/mcp.ts:987-988 runs sendToolListChanged; src/server/mcp.ts:1267 sees a transport and calls src/server/index.ts:643-644, which returns the promise from Protocol.notification. With no debouncedNotificationMethods configured, src/shared/protocol.ts:1443 doesawait this._transport.send(...); in src/server/webStandardStreamableHttp.ts:1141-1143storeEventis awaited before the no-stream early return at line 1146-1149, so a failing event store rejects the send. The promise is dropped at src/server/mcp.ts:1268, so Node raises an unhandled rejection and, by default, exits. On the base branch this population threw synchronously at setToolRequestHandlers and never reached the floating promise. Remedy:.catch(error => this.server.onerror?.(error))at src/server/mcp.ts:1259, 1268 and 1277.Verification: pre-existing. The defective code is untouched by this PR and the base already fails the same way by the same route. src/server/mcp.ts:1268
this.server.sendToolListChanged();(no await, no.catch) -> src/shared/protocol.ts:1443 on the non-debounced path. A rejected send therefore becomes an unhandled promise rejection, which by default terminates a Node process.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012VRbFCp41otcScXE1YY3es
There was a problem hiding this comment.
LGTM — the changeset rewording in the latest commit now matches what the constructor does (any declared tools/resources/prompts capability is advertised with listChanged: true), and the code is unchanged from the version already reviewed. The two remaining inline notes (declared completions left on the lazy path, e2e coverage) are optional and do not block this fix.
Extended reasoning...
The diff adds nine lines to the McpServer constructor in src/server/mcp.ts that call the existing idempotent setToolRequestHandlers/setResourceRequestHandlers/setPromptRequestHandlers helpers when the matching capability is declared, plus three end-to-end InMemoryTransport tests and a patch changeset. It touches no auth, crypto, or input-parsing surface; the behavioural change (empty lists and listChanged: true for declared-but-empty capabilities) is documented in the changeset and mirrors main. The only change since the prior review is a one-line changeset wording fix that addresses the nit I raised about it, and the remaining open inline notes were marked optional. A human reviewer has already approved, the change is small and self-contained on the v1.x maintenance line, and the hunt ran dry, so approval is warranted.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
Requested by Felix Weinberger · Slack thread
Before: A server that declares
tools,resourcesorpromptsin theMcpServeroptions and registers its first item afterconnect()getsCannot register capabilities after connecting to transport, because the handlers (and with them the capability) were only installed on the first registration. This is the problem reported in #893.After: A capability declared in the constructor options gets its handlers in the constructor, as on
main(#2269), so registering afterconnect()works. Two visible side effects, both matching v2: a declared capability with nothing registered answers its list request with an empty list instead ofMethod not found(the server-side behaviour #181 asks for), and a capability declared as{}is advertised withlistChanged: true. Registering afterconnect()without having declared the capability still throws.How it was verified: Three new tests in
test/server/mcp.test.ts. Onv1.xas it is, the first two fail with exactlyCannot register capabilities after connecting to transportandMCP error -32601: Method not found, and the third passes; with the change all three pass. Unit suite 1858 of 1858,tsc --noEmit0 errors, Prettier clean, ESLint clean onsrc/server/mcp.ts.🤖 Generated with Claude Code
https://claude.ai/code/session_012VRbFCp41otcScXE1YY3es
Generated by Claude Code