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
⚠️ No Changeset foundLatest commit: 010294f Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Sorry, something went wrong.
|
@modelcontextprotocol/client
npm i https://pkg.pr.new/@modelcontextprotocol/client@2550
npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2550
npm i https://pkg.pr.new/@modelcontextprotocol/core@2550
npm i https://pkg.pr.new/@modelcontextprotocol/server@2550
npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2550
npm i https://pkg.pr.new/@modelcontextprotocol/express@2550
npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2550
npm i https://pkg.pr.new/@modelcontextprotocol/hono@2550
npm i https://pkg.pr.new/@modelcontextprotocol/node@2550 commit: 010294f |
Sorry, something went wrong.
|
Nice, this covers the notification side. One thing it doesn't touch: registering the handler is silent too. assertRequestHandlerCapability gates resources/list, resources/read and resources/templates/list on resources, but resources/subscribe and resources/unsubscribe aren't in that switch at all, so setRequestHandler('resources/subscribe', ...) works fine without the bit. That's the other half of #2545, the one where the client's subscribe request is what goes nowhere. The other thing worth a maintainer call is warn vs throw. Every local capability mistake in this file throws SdkError(CapabilityNotSupported), and those checks aren't behind enforceStrictCapabilities, since Protocol.notification() calls assertNotificationCapability unconditionally and that option's docs call a mis-declared local capability a logic error. The client throws for resources/subscribe as well, so warning on the server side leaves the two halves inconsistent. 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. |
Sorry, something went wrong.
|
@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). |
Sorry, something went wrong.
…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
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).
Summary
Fixes #2545 by making missing resources.subscribe a hard local capability error on both halves of the resource-subscription path:
notifications/resources/list_changed still only requires resources (not subscribe).
Test plan
Notes
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.