Repository navigation
fix(server): reflect resource title updates in list responses - #3005
felixweinberger merged 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 142c861 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 |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
0c352e0 to
142c861
Compare
chaeyn
changed the title
claude
Bot
added
the
v2
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Beyond the inline findings, I also checked whether prompts and tools have the same title/metadata divergence — their list handlers (mcp.ts:272, :644) read title straight from the registered object, so they are not affected. I also checked the update({ title: null }) schema-invalid case — the update types declare title?: string, so null is rejected at the type level.
Extended reasoning...
The change moves title handling after metadata assignment in the resource and resource-template update callbacks in packages/server/src/server/mcp.ts and mirrors the title into metadata so resources/list and resources/templates/list reflect it; it touches no security-sensitive surface. Two inline findings (title lost on a later metadata-only update, and the pre-existing stale-URI key on repeated uri updates) were posted, so a human should weigh those before merging.
| if (updates.metadata !== undefined) registeredResource.metadata = updates.metadata; | ||
| if (updates.title !== undefined) { | ||
| registeredResource.title = updates.title; | ||
| registeredResource.metadata = { ...registeredResource.metadata, title: updates.title }; | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Server authors who update a title and later replace metadata will see the title vanish from resources/list while the handle still reports the new title. The sync at packages/server/src/server/mcp.ts:783-787 only runs inside the call that carries title; a following update({ metadata }) at mcp.ts:783 overwrites metadata wholesale and drops the synced title key, and mcp.ts:542 spreads only metadata. The PR makes title the authoritative field but only for one call. Fix: treat registeredResource.title as the source of truth in list handlers (or re-apply it whenever metadata is replaced), in both the resource and template update paths (mcp.ts:824-828).
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Sequence: resource.update({ title: 'New' }) then resource.update({ metadata: { description: 'd' } }). After the first call mcp.ts:786 stores metadata.title = 'New' and the wire shows it. The second call at mcp.ts:783 assigns metadata = updates.metadata, discarding the title key; mcp.ts:784 does not run because updates.title is undefined. resources/list at mcp.ts:542 and the template path at mcp.ts:554/568 spread only metadata, so clients now get no title at all, while registeredResource.title still reads 'New'. The base branch also lost the title in this sequence, but the base never surfaced handle.title on the wire at all; this PR introduces the contract that update({ title }) is reflected in list responses and its changeset says other metadata is preserved, so users will now rely on the handle title persisting. Remedy: have the list handlers emit title from the handle or re-merge the stored title whenever updates.metadata replaces the object, for both resources and templates.
Verification: pre-existing — triggered when a server calls update({ title }) and then, in a separate call, update({ metadata }) without a title key. packages/server/src/server/mcp.ts:783 replaces the metadata object wholesale, and the title mirror at mcp.ts:784-787 only runs when updates.title !== undefined; resources/list at mcp.ts:542 spreads only resource.metadata. The base branch ends in the same state.
| if (updates.uri) this._registeredResources[updates.uri] = registeredResource; | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Server authors who move a resource with update({ uri }) and then call remove() or a second update({ uri }) leave the resource registered under the previous URI, so resources/list keeps advertising it and resources/read still serves it. The update callback at mcp.ts:778-781 compares against and deletes the closure-captured original uri, never the current key. The same pattern exists for template names at mcp.ts:819-822. Fix: track the current uri (and template name) in a mutable variable updated on each rename, and delete that key, so remove() and chained renames act on the live registration.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: resource.update({ uri: 'b' }) then resource.remove() (which calls update({ uri: null }) at mcp.ts:776), or a second update({ uri: 'c' }). mcp.ts:778 tests updates.uri !== uri where uri is the registration-time parameter captured at mcp.ts:762, and mcp.ts:779 deletes this._registeredResources[uri], which no longer holds the resource. The entry under 'b' survives, so resources/list at mcp.ts:537 still lists it and resources/read at mcp.ts:586 still serves it; after a second rename the resource is registered twice under 'b' and 'c'. The template path at mcp.ts:819-822 has the identical flaw with the captured name. The base branch behaves the same, which is why it was dismissed, but this PR edits exactly this update callback in both places and adds a changeset promising update() is reflected in list responses, so the review had the function open and the defect sits in the lines directly above the hunk. Remedy: keep a let currentUri / currentName that each rename updates and delete that key.
Verification: pre-existing — the base at b022522 fails by the same route; this PR does not touch the rename branch. Trigger: a server author calls resource.update({ uri: 'b' }) and later resource.remove(). Mechanism at packages/server/src/server/mcp.ts:778-781: uri is the registration-time parameter never reassigned, so remove() is a no-op and the 'b' entry survives.
2 participants
Motivation and Context
update({ title: 'New title' })changes a registered resource or resource template's handle, but list responses still use the originalmetadata.title. Clients therefore receive the old title even when bypassing their response cache. If registration supplied no title, the new title is missing from the response.Fixes #3004.
What changed
packages/server/src/server/mcp.ts: synchronize the metadata title in both resource update paths, including when other metadata is replaced in the same call.test/integration/test/server/resourceTitleUpdate.test.ts: add seven client/server regression cases for initial title presence/absence, simultaneous metadata updates, and template title inheritance. Individual resource titles retain precedence over template metadata.How Has This Been Tested?
pnpm build:all,pnpm typecheck:all, andpnpm lint:allpass, including pre-push hooks.142c861, including Node 20/22/24 E2E, Bun/Deno, and client/server conformance.pnpm test:allpasses the non-E2E packages but exits nonzero on two unhandledREQUEST_TIMEOUTrejections inprotocol.test.ts. Both reproduce with the unmodifiedmcp.tsfromb022522. A standalone protocol-file rerun also had amax-totaltiming assertion failure; isolatedmax-totalcases pass on both base and patched source.Local environment: Node v24.19.0, pnpm 10.26.1, macOS. Reproduced with SDK client/server instances over InMemoryTransport; no production-application validation.
Breaking Changes
None. No API or configuration changes are required.
Types of changes
Checklist
AI assistance: Codex was used for the investigation, implementation, tests, and PR description.