mcp: convert tool/prompt schemas eagerly at registration time#1861
mcp: convert tool/prompt schemas eagerly at registration time#1861ravyg wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
Currently `standardSchemaToJsonSchema()` is called lazily inside the `tools/list` request handler, re-converting every tool's schema on every list request. The same applies to prompts via `promptArgumentsFromStandardSchema()` in the `prompts/list` handler. Move the conversion to `_createRegisteredTool()` / `_createRegisteredPrompt()` and cache the result on `RegisteredTool` (`inputJsonSchema`, `outputJsonSchema`) and `RegisteredPrompt` (`cachedArguments`). The list handlers now read from these cached fields. The `update()` methods recompute the cache when schemas change. This: - Surfaces schema conversion errors (e.g. cycle detection from modelcontextprotocol#1563) at dev time when the tool is registered, not at runtime when a client first calls `tools/list` - Avoids re-converting identical schemas on every `tools/list` / `prompts/list` call - Matches the Go SDK and FastMCP, which both process schemas at registration time Includes regression tests verifying eager conversion at registration, cached reuse across list calls, and cache invalidation on `update()` for both tools and prompts. Fixes modelcontextprotocol#1847
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let needsExecutorRegen = false; | ||
| if (updates.paramsSchema !== undefined) { | ||
| registeredTool.inputSchema = updates.paramsSchema; | ||
| // Re-cache the JSON Schema; surfaces conversion errors | ||
| // synchronously like the initial registration does. | ||
| registeredTool.inputJsonSchema = standardSchemaToJsonSchema(updates.paramsSchema, 'input') as Tool['inputSchema']; | ||
| needsExecutorRegen = true; | ||
| } | ||
| if (updates.callback !== undefined) { |
There was a problem hiding this comment.
🔴 Both tool.update() and prompt.update() mutate the registered object's schema field before calling the potentially-throwing conversion function, leaving the object in an inconsistent state if conversion fails. After a failed update, argsSchema/inputSchema holds the new broken schema while cachedArguments/inputJsonSchema remain stale, needsExecutorRegen/needsHandlerRegen are never set (executor/handler not regenerated), and any concurrent callback change is silently dropped; fix by computing the new JSON schema/arguments before mutating state.
Extended reasoning...
What the bug is and how it manifests
In both _createRegisteredTool.update() and _createRegisteredPrompt.update(), this PR introduces a pattern where the registered object's schema field is mutated before the conversion function that can throw is called. If the conversion function throws (e.g., cycle detection from #1563, or any other schema validation error), the object is left in a partially-updated, inconsistent state.
The specific code paths that trigger it
For tools (lines 829–835 in the new code):
if (updates.paramsSchema \!== undefined) {
registeredTool.inputSchema = updates.paramsSchema; // (1) mutated
registeredTool.inputJsonSchema = standardSchemaToJsonSchema(updates.paramsSchema, 'input') // (2) can throw
needsExecutorRegen = true; // (3) never reached
}
if (updates.callback \!== undefined) { ... } // (4) never reachedFor prompts (lines 738–744 in the new code):
if (updates.argsSchema \!== undefined) {
registeredPrompt.argsSchema = updates.argsSchema; // (1) mutated
registeredPrompt.cachedArguments = promptArgumentsFromStandardSchema(updates.argsSchema); // (2) can throw
currentArgsSchema = updates.argsSchema; // (3) never reached
needsHandlerRegen = true; // (4) never reached
}Why existing code doesn't prevent it
Before this PR, update() never called standardSchemaToJsonSchema or promptArgumentsFromStandardSchema — those conversions happened lazily in the list handlers. This PR moves the conversion into update() (correctly) but forgets to guard the mutation against conversion failure.
Impact
After a failed tool.update({ paramsSchema: cycleSchema, callback: newCb }):
inputSchema= new cyclic schema (broken)inputJsonSchema= stale old JSON schema →tools/listserves wrong schemaneedsExecutorRegennever set → executor NOT regeneratedcallbackupdate silently dropped (theif (updates.callback)…block is after the throw and never reached)validateToolInputreadstool.inputSchemadirectly, so subsequenttools/callrequests will attempt to validate against the new cyclic/broken schema
After a failed prompt.update({ argsSchema: badSchema }):
argsSchema= new broken schemacachedArguments= stale old arguments →prompts/listserves wrong argumentscurrentArgsSchema(closure variable used for future handler regeneration, line 743) is never updated, so any laterupdate({ callback })will regenerate the handler with the old schema whileregisteredPrompt.argsSchemasays the new one — a persistent inconsistency that outlives the failing call
The same mutation-before-throw pattern at lines 845–848 for outputSchema leaves outputSchema updated while outputJsonSchema remains stale.
Step-by-step proof
const tool = server.registerTool('foo', { inputSchema: zodGoodSchema }, cb)
→tool.inputSchema= goodSchema,tool.inputJsonSchema= good JSON, executor uses goodSchematool.update({ paramsSchema: zodCycleSchema, callback: newCb })
→ line 830:tool.inputSchema= cycleSchema ✓ (mutated)
→ line 833:standardSchemaToJsonSchema(cycleSchema, 'input')throws (cycle detection)
→ exception propagates; lines 834+ never execute- State after throw:
inputSchema= cycleSchema,inputJsonSchema= good JSON (stale), executor still uses goodSchema,newCbnever installed tools/list→ returns good JSON schema (stale) — appears finetools/call→validateToolInputreadstool.inputSchema(cycleSchema) → crash or unexpected validation behavior
How to fix it
Compute the new value before mutating state:
if (updates.paramsSchema \!== undefined) {
const newInputJsonSchema = standardSchemaToJsonSchema(updates.paramsSchema, 'input'); // may throw — state untouched
registeredTool.inputSchema = updates.paramsSchema;
registeredTool.inputJsonSchema = newInputJsonSchema as Tool['inputSchema'];
needsExecutorRegen = true;
}Apply the same pattern for outputSchema and for registeredPrompt.argsSchema / cachedArguments.
2 participants
Currently
standardSchemaToJsonSchema()is called lazily inside thetools/listrequest handler, re-converting every tool's schema on every list request. The same applies to prompts viapromptArgumentsFromStandardSchema()in theprompts/listhandler.Move the conversion to
_createRegisteredTool()/_createRegisteredPrompt()and cache the result onRegisteredTool(inputJsonSchema,outputJsonSchema) andRegisteredPrompt(cachedArguments). The list handlers now read from these cached fields. Theupdate()methods recompute the cache when schemas change.This:
tools/listtools/list/prompts/listcallIncludes regression tests verifying eager conversion at registration, cached reuse across list calls, and cache invalidation on
update()for both tools and prompts.Fixes #1847
Motivation and Context
Quoting the issue (filed by @felixweinberger and labeled
ready for work,P2):The current lazy behavior wastes CPU on every
tools/listcall (agents poll this frequently) and delays the discovery of schema-conversion bugs until a client first connects, which makes the bug look like a runtime crash instead of a registration error.How Has This Been Tested?
Six new regression tests were added to
test/integration/test/server/mcp.test.ts:should convert tool schemas eagerly at registration time— assertstool.inputJsonSchemaandtool.outputJsonSchemaare populated immediately afterregisterTool(), with no client connection required.should reuse cached JSON Schema across tools/list calls— asserts two consecutivetools/listrequests return identical JSON Schema content for the same tool.should re-cache JSON Schema when paramsSchema is updated— assertstool.update({ paramsSchema })invalidates and recomputes the cache.should re-cache JSON Schema when outputSchema is updated— same foroutputSchema.should compute prompt arguments eagerly at registration time— assertsprompt.cachedArgumentsis populated immediately afterregisterPrompt().should re-cache prompt arguments when argsSchema is updated— assertsprompt.update({ argsSchema })invalidates and recomputes the cache.The tests were verified to fail on unfixed code (5 of 6 fail — the 6th doesn't exercise the bug path) and pass with the fix.
Full repo verification on the final diff:
pnpm typecheck:all— cleanpnpm lint:all— cleanpnpm build:all— cleanpnpm test:all— 1,438 tests pass across all packages (core: 489,client: 350,server: 55,middleware/*: 114,examples/shared: 2,test/integration: 428)Breaking Changes
None. This is a purely internal optimization:
registerTool()/registerPrompt()public signatures and behavior are unchanged.RegisteredTool(inputJsonSchema,outputJsonSchema) andRegisteredPrompt(cachedArguments) are marked@hiddenin JSDoc and are additive.tools/listandprompts/listresponses remain byte-identical for valid schemas.tools/listnow throw synchronously insideregisterTool()/registerPrompt(). Any code that today registers a tool with a knowingly-broken schema and only fails when a client connects will now fail at registration. We believe this is a net win — it's the explicitly-stated goal in the issue.Types of changes
Checklist
(Documentation: no doc updates needed. The
RegisteredTool/RegisteredPromptcached fields are@hidden. PublicregisterTool/registerPromptexamples indocs/server.mdcontinue to work without modification, anddocs/migration.mddoesn't apply because this is a backwards-compatible internal change.)Additional context
Related: #1563 (where this came up) — the eager-conversion approach surfaces cycle-detection errors at registration time.
The implementation follows the pattern already used elsewhere in this file: cached state on the
Registered*object, recomputed inside the existingupdate()methods alongside the related fields. No new abstractions, no new dependencies.