Skip to content

fix(server): keep the registry key current when a handle is renamed - #2724

Open
YatsukBogdan1 wants to merge 1 commit into
modelcontextprotocol:mainfrom
YatsukBogdan1:fix/registry-key-on-rename
Open

fix(server): keep the registry key current when a handle is renamed#2724
YatsukBogdan1 wants to merge 1 commit into
modelcontextprotocol:mainfrom
YatsukBogdan1:fix/registry-key-on-rename

Conversation

@YatsukBogdan1

Copy link
Copy Markdown

Fixes #2723

Summary

The update closure for prompts, resources and resource templates captured the registration key and never reassigned it, so after one rename it pointed at a key the entry no longer occupied. delete this._registeredX[key] then hit a vacant slot and left the live entry alone.

RegisteredTool already tracks the current key (L871); this brings the other three in line.

Changes

Three sites in packages/server/src/server/mcp.ts reassign the key binding after moving the entry — the same shape as the existing tool path.

Testing

Three tests in test/integration/test/server/mcp.test.ts, one per affected primitive, asserting through a connected client. Each renames twice, checks only the current key is listed, then calls remove() and checks the list is empty.

With the fix reverted, all three fail:

× should not leave the intermediate uri registered after two uri changes
× should not leave the intermediate name registered after two template renames
× should not leave the intermediate name registered after two renames

Also run green: @modelcontextprotocol/server (481), integration (374), e2e (2639 + 147 expected-fail), lint and typecheck.

Notes for review

  • One behaviour change beyond the bookkeeping: createPromptHandler(name, …) at L788 is re-invoked when a schema or callback update regenerates the handler, and uses name in its validation error text. A prompt renamed and then given a new schema now reports the current name instead of the registration name.
  • Not addressed, pre-existing on the tool path too: after remove(), the binding still holds the last live key, so update({ name }) on a removed handle resurrects it. Left alone to keep this focused.
  • I did not touch the e2e scenarios. They are the reason this survived — both update non-key fields only — so tightening them may be worth a follow-up.

v1.x has the same defect on all four primitives including tools; happy to open a backport once this lands.

The `update` closure for prompts, resources and resource templates
captured the registration key and never reassigned it, so after one
rename it pointed at a key the entry no longer occupied.

- `remove()` became a silent no-op: it deletes the captured key, which
  is already vacant, leaving the entry listed and callable while
  `list_changed` still fires.
- A second rename left the intermediate key registered, aliasing one
  entry under two live names.
- Renaming back to the original name silently did nothing, because the
  `updates.name !== name` guard compared against the stale value.

`RegisteredTool` already tracked the current key and is unchanged.
@YatsukBogdan1
YatsukBogdan1 requested a review from a team as a code owner August 26, 2026 11:57
@changeset-bot

changeset-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1a8b5d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/server Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/client Patch
@modelcontextprotocol/server-legacy Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core-internal Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Aug 26, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2724

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2724

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2724

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2724

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2724

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2724

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2724

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2724

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2724

commit: 1a8b5d2

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the exact head locally: the integration suite passes 374/374. The mutable key now follows each resource URI / template name / prompt name change, so repeated renames (including renaming back) and remove() target the live registry entry.

One coordination note: #2114 appears to cover the same registry-key bookkeeping on an older branch, including tools. This PR is narrower and has current-main integration coverage, but the overlap is worth resolving before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remove() is a no-op after a handle is renamed, and the entry stays callable

2 participants