mcp: convert tool/prompt schemas eagerly at registration time#1861
Open
ravyg wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
mcp: convert tool/prompt schemas eagerly at registration time#1861ravyg wants to merge 1 commit intomodelcontextprotocol:mainfrom
ravyg 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: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.