fix(server): throw when resource subscribe capability is missing#2550
fix(server): throw when resource subscribe capability is missing#2550arimu1 wants to merge 2 commits into
Conversation
…ribe Resource update notifications only reach clients that opted in via resources/subscribe, which is gated on the advertised subscribe capability. Warn once on sendResourceUpdated when that bit is missing so missing capabilities are obvious during development. Closes modelcontextprotocol#2545
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
Nice, this covers the notification side. One thing it doesn't touch: registering the handler is silent too. The other thing worth a maintainer call is warn vs throw. Every local capability mistake in this file throws I'd gone down the throwing route with tests for both halves before I spotted your PR. Happy to hand that over or to open a follow-up for the handler gate once this lands, whichever works best. |
Align with other local capability checks: sendResourceUpdated throws SdkError(CapabilityNotSupported) via assertNotificationCapability when resources.subscribe is missing, and setRequestHandler gates resources/subscribe|unsubscribe the same way. Covers both halves of modelcontextprotocol#2545 (notification send + handler registration).
|
@ondraulehla Thanks — took both halves of the suggestion.
Tests cover send + both handler registrations. Still draft while the fork's ready-for-review slot is held by #2549; happy to mark ready once that clears (or if a maintainer wants it earlier). |
Summary
Fixes #2545 by making missing
resources.subscribea hard local capability error on both halves of the resource-subscription path:sendResourceUpdated→assertNotificationCapability('notifications/resources/updated')now requirescapabilities.resources.subscribeand throwsSdkError(CapabilityNotSupported), matching every other local capability mistake in this file (and the client, which already throws forresources/subscribe).assertRequestHandlerCapabilitynow gatesresources/subscribeandresources/unsubscribethe same way, sosetRequestHandler('resources/subscribe', …)without the advertised bit no longer succeeds silently.notifications/resources/list_changedstill only requiresresources(not subscribe).Test plan
packages/serverunit tests: throw without subscribe; succeed with subscribe;setRequestHandlerreject/allow for subscribe & unsubscribe@modelcontextprotocol/serversuite: 453/453Notes
Switched from the initial warn-once approach to throw after review feedback: warn left the server half inconsistent with the client and with every other local capability check in
server.ts.