Repository navigation
fix(server): reflect resource title updates in list responses #3005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/server': patch | ||
| --- | ||
|
|
||
| Reflect resource and resource template `update({ title })` changes in list responses. Preserve other metadata and individual resource titles when a template supplies list entries. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -780,8 +780,11 @@ export class McpServer { | |
| if (updates.uri) this._registeredResources[updates.uri] = registeredResource; | ||
| } | ||
| if (updates.name !== undefined) registeredResource.name = updates.name; | ||
| if (updates.title !== undefined) registeredResource.title = updates.title; | ||
| if (updates.metadata !== undefined) registeredResource.metadata = updates.metadata; | ||
| if (updates.title !== undefined) { | ||
| registeredResource.title = updates.title; | ||
| registeredResource.metadata = { ...registeredResource.metadata, title: updates.title }; | ||
| } | ||
|
Comment on lines
783
to
+787
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 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 Why this was flaggedSequence: 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 |
||
| if (updates.callback !== undefined) registeredResource.readCallback = updates.callback; | ||
| if (updates.scopeChallenge !== undefined) { | ||
| registeredResource.scopeChallenge = updates.scopeChallenge === null ? undefined : updates.scopeChallenge; | ||
|
|
@@ -817,9 +820,12 @@ export class McpServer { | |
| delete this._registeredResourceTemplates[name]; | ||
| if (updates.name) this._registeredResourceTemplates[updates.name] = registeredResourceTemplate; | ||
| } | ||
| if (updates.title !== undefined) registeredResourceTemplate.title = updates.title; | ||
| if (updates.template !== undefined) registeredResourceTemplate.resourceTemplate = updates.template; | ||
| if (updates.metadata !== undefined) registeredResourceTemplate.metadata = updates.metadata; | ||
| if (updates.title !== undefined) { | ||
| registeredResourceTemplate.title = updates.title; | ||
| registeredResourceTemplate.metadata = { ...registeredResourceTemplate.metadata, title: updates.title }; | ||
| } | ||
| if (updates.callback !== undefined) registeredResourceTemplate.readCallback = updates.callback; | ||
| if (updates.scopeChallenge !== undefined) { | ||
| registeredResourceTemplate.scopeChallenge = updates.scopeChallenge === null ? undefined : updates.scopeChallenge; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { Client } from '@modelcontextprotocol/client'; | ||
| import { InMemoryTransport } from '@modelcontextprotocol/core-internal'; | ||
| import { McpServer, ResourceTemplate } from '@modelcontextprotocol/server'; | ||
| import { afterEach, beforeEach, describe, expect, test } from 'vitest'; | ||
|
|
||
| describe('resource title updates', () => { | ||
| let server: McpServer; | ||
| let client: Client; | ||
|
|
||
| beforeEach(() => { | ||
| server = new McpServer({ name: 'test-server', version: '1' }); | ||
| client = new Client({ name: 'test-client', version: '1' }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await client.close(); | ||
| await server.close(); | ||
| }); | ||
|
|
||
| async function connect() { | ||
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | ||
| await server.connect(serverTransport); | ||
| await client.connect(clientTransport); | ||
| } | ||
|
|
||
| async function list(kind: 'static' | 'template') { | ||
| if (kind === 'static') { | ||
| const result = await client.listResources(undefined, { cacheMode: 'bypass' }); | ||
| return result.resources; | ||
| } | ||
| const result = await client.listResourceTemplates(undefined, { cacheMode: 'bypass' }); | ||
| return result.resourceTemplates; | ||
| } | ||
|
|
||
| for (const kind of ['static', 'template'] as const) { | ||
| for (const initialTitle of [undefined, 'Old title']) { | ||
| test(`${kind}: advertises an updated title when the initial title is ${initialTitle}`, async () => { | ||
| const target = kind === 'static' ? 'docs://one' : new ResourceTemplate('docs://{id}', { list: undefined }); | ||
| const metadata = { title: initialTitle, description: 'Keep this description', mimeType: 'text/plain' }; | ||
| const read = async (uri: URL) => ({ contents: [{ uri: uri.href, text: 'document' }] }); | ||
| const resource = | ||
| typeof target === 'string' | ||
| ? server.registerResource('docs', target, metadata, read) | ||
| : server.registerResource('docs', target, metadata, read); | ||
| await connect(); | ||
|
|
||
| resource.update({ title: 'New title' }); | ||
|
|
||
| const entries = await list(kind); | ||
| expect(entries).toEqual([expect.objectContaining({ ...metadata, title: 'New title' })]); | ||
| }); | ||
| } | ||
|
|
||
| test(`${kind}: keeps an explicit title update when replacing other metadata`, async () => { | ||
| const read = async (uri: URL) => ({ contents: [{ uri: uri.href, text: 'document' }] }); | ||
| const resource = | ||
| kind === 'static' | ||
| ? server.registerResource('docs', 'docs://one', { title: 'Old title' }, read) | ||
| : server.registerResource( | ||
| 'docs', | ||
| new ResourceTemplate('docs://{id}', { list: undefined }), | ||
| { title: 'Old title' }, | ||
| read | ||
| ); | ||
| await connect(); | ||
|
|
||
| resource.update({ title: 'New title', metadata: { description: 'New description' } }); | ||
|
|
||
| const entries = await list(kind); | ||
| expect(entries).toEqual([expect.objectContaining({ title: 'New title', description: 'New description' })]); | ||
| }); | ||
| } | ||
|
|
||
| test('updates inherited template titles while preserving individual resource titles', async () => { | ||
| const resource = server.registerResource( | ||
| 'docs', | ||
| new ResourceTemplate('docs://{id}', { | ||
| list: async () => ({ | ||
| resources: [ | ||
| { uri: 'docs://one', name: 'one' }, | ||
| { uri: 'docs://two', name: 'two', title: 'Individual title' } | ||
| ] | ||
| }) | ||
| }), | ||
| { title: 'Old title', mimeType: 'text/plain' }, | ||
| async uri => ({ contents: [{ uri: uri.href, text: 'document' }] }) | ||
| ); | ||
| await connect(); | ||
|
|
||
| resource.update({ title: 'New title' }); | ||
|
|
||
| const { resources } = await client.listResources(undefined, { cacheMode: 'bypass' }); | ||
| expect(resources).toEqual([ | ||
| { uri: 'docs://one', name: 'one', title: 'New title', mimeType: 'text/plain' }, | ||
| { uri: 'docs://two', name: 'two', title: 'Individual title', mimeType: 'text/plain' } | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 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 laterresource.remove(). Mechanism at packages/server/src/server/mcp.ts:778-781:uriis the registration-time parameter never reassigned, soremove()is a no-op and the 'b' entry survives.