Repository navigation
Conversation
One-shot jsonschema.validate() re-selects the validator class, checks the schema against the meta-schema, and resolves $refs on every tools/call, then discards the result. Compile per-tool input/output validators once per tool listing and reuse them across calls, mirroring the client-side fix from modelcontextprotocol#3134 (explicit empty referencing registry, SchemaError surfaced with tool context). Fixes modelcontextprotocol#3676
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
the
missing-issue-link
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3676. If a maintainer assigns you to #3676, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
There was a problem hiding this comment.
🟡 Changes recommended
Mutable schemas can retain stale validators, and unresolved references bypass the intended error contract.
2 open findings
What changed in this PR
Caches compiled server-side tool schema validators to avoid repeated JSON Schema compilation.
Changes:
- Adds lazy input/output validator caches.
- Invalidates caches when tool listings refresh.
- Adds validation-cache regression tests.
| File | Description |
|---|---|
src/mcp/server/lowlevel/server.py |
Implements compiled validator caching. |
tests/server/test_lowlevel_validator_cache.py |
Tests reuse, relisting, errors, and disabled validation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| validator_cls.check_schema(schema) | ||
| except jsonschema.SchemaError as e: | ||
| raise RuntimeError(f"Invalid schema for tool {tool_name}: {e}") | ||
| return validator_cls(schema, registry=Registry()) |
| cached = self._tool_input_validators.get(tool_name) | ||
| if cached is not None and cached[0] is tool: | ||
| return cached[1] |
Fixes #3676.
Problem
Server.call_tool()on v1.x calls one-shotjsonschema.validate()twice pertools/call(input at former line 536, structured output at former line 573). Each call re-selects the validator class, runscheck_schema()against the meta-schema, and resolves$refs, then throws the result away. Tool schemas do not change between calls. This is the server-side counterpart of #3133, fixed forClientSessionin #3134.Change (
src/mcp/server/lowlevel/server.py)_tool_input_validators/_tool_output_validators, identity-bound to the cachedToolso re-listed schemas recompile; both cleared with_tool_cache).referencing.Registry(resolution stays within the schema document and bundled meta-schemas) and invalid schemas surfaced asRuntimeError("Invalid schema for tool …"), which the existing generic handler turns into an error result.validate_input=Falsestill skips input validation (and its compile) entirely; unlisted tools still skip validation with the existing warning. Error message contract (Input validation error: …/Output validation error: …) unchanged.Verification
tests/server/test_lowlevel_validator_cache.py(5 tests): compile-once-reuse (spy onvalidator_for), preserved input/output error contracts, re-list recompiles and enforces the new schema,validate_input=Falseskips input compile.tests/server/test_lowlevel_{validator_cache,input_validation,output_validation}.py: 20 passed.test_lowlevel_exception_handling,test_session,test_stdio,tests/server/fastmcp: 353 passed.ruff check+ruff format --checkclean;pyright(strict) 0 errors on both touched files.Note: drafted with AI assistance; I ran the suites above locally, reviewed the diff, and will answer questions on it.