-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[v1.x] Resolve tool output-schema references within the schema document only #3396
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 |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| import logging | ||
| from contextlib import contextmanager | ||
| from pathlib import Path | ||
| from typing import Any | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
| from referencing.exceptions import Unresolvable | ||
|
|
||
| from mcp.server.lowlevel import Server | ||
| from mcp.shared.memory import ( | ||
|
|
@@ -215,3 +217,34 @@ async def call_tool(name: str, arguments: dict[str, Any]) -> dict[str, Any]: | |
|
|
||
| # Check that warning was logged | ||
| assert "Tool mystery_tool not listed" in caplog.text | ||
|
|
||
|
|
||
| # jsonschema's fallback retriever emits this DeprecationWarning; keep it a plain warning so the | ||
| # assertions below decide the outcome rather than the suite's warnings-as-errors filter. | ||
| @pytest.mark.filterwarnings("default:Automatically retrieving remote references:DeprecationWarning") | ||
|
Comment on lines
+222
to
+224
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. 🟡 nit: This comment and filterwarnings mark claim jsonschema's fallback retriever emits the "Automatically retrieving remote references" DeprecationWarning during this test, but that deprecated auto-retrieval only exists on the default registry jsonschema uses when no Extended reasoning...Path: the client calls jsonschema.validate(..., registry=Registry()) (src/mcp/client/session.py:439-441). In jsonschema 4.25.1, Validator.attrs_post_init checks Verification: nit. The filterwarnings mark and its comment at tests/client/test_output_schema_validation.py:221-223 ("jsonschema's fallback retriever emits this DeprecationWarning; keep it a plain warning...") describe a warning that cannot fire on this test's code path. The client validates via src/mcp/client/session.py: |
||
| @pytest.mark.anyio | ||
| async def test_output_schema_ref_outside_the_document_is_rejected(tmp_path: Path): | ||
| """A `$ref` to a URI outside the output schema is not resolved, and a result whose validation | ||
| reaches one fails as an invalid schema (spec `$ref` resolution; applying it to `file:` URIs too | ||
| is SDK-defined).""" | ||
| target = tmp_path / "schema.json" | ||
| target.write_text("{}", encoding="utf-8") | ||
| server = Server("test-server") | ||
|
|
||
| @server.list_tools() | ||
| async def list_tools(): | ||
| return [ | ||
| Tool(name="probe", description="", inputSchema={"type": "object"}, outputSchema={"$ref": target.as_uri()}) | ||
| ] | ||
|
|
||
| @server.call_tool() | ||
| async def call_tool(name: str, arguments: dict[str, Any]): | ||
| return {"v": 1} | ||
|
|
||
| with bypass_server_output_validation(): | ||
| async with client_session(server) as client: | ||
| with pytest.raises(RuntimeError) as exc_info: | ||
| await client.call_tool("probe", {}) | ||
| # SDK-authored prefix only; the tail is `referencing`'s text. | ||
| assert str(exc_info.value).startswith("Invalid schema for tool probe: ") | ||
| assert isinstance(exc_info.value.__cause__, Unresolvable) | ||
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.
🟡 nit:
referencingis now imported at runtime (here and in tests/client/test_output_schema_validation.py) but is not declared in pyproject.tomldependencies— it is only available transitively throughjsonschema>=4.20.0, so a future jsonschema release that drops or vendors it would breakcall_tooloutput validation with an ImportError. Declarereferencingas a direct dependency (e.g.uv add referencing).Extended reasoning...
src/mcp/client/session.py:431-432 does
from referencing import Registry/from referencing.exceptions import Unresolvableinside_validate_tool_result, executed on every call_tool for a tool with an outputSchema. pyproject.toml lists onlyjsonschema>=4.20.0(line 37);referencingappears nowhere as a direct dependency. Today jsonschema 4.18+ depends on referencing so the import resolves, which is why tests pass — but the package now has a direct code dependency it does not declare, violating packaging hygiene: if jsonschema pins change (vendoring, extras split) within the allowed>=4.20.0range, or a downstream resolver installs jsonschema without referencing,_validate_tool_resultraises ImportError at call time instead of validating. On the base branch neither src nor tests imported referencing, so this undeclared dependency is introduced by this PR. Fix: addreferencingto[project.dependencies](uv add referencing).Verification: nit. The factual claims all check out. The diff adds runtime imports of an undeclared package at src/mcp/client/session.py:431-432 (
from referencing import Registry/from referencing.exceptions import Unresolvable), executed inside_validate_tool_resulton everycall_toolfor a tool with an output schema, and tests/client/test_output_schema_validation.py:8 also imports it at module level.