Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/mcp/client/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -428,17 +428,24 @@ async def _validate_tool_result(self, name: str, result: types.CallToolResult) -

if output_schema is not None:
from jsonschema import SchemaError, ValidationError, validate
from referencing import Registry

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 nit: referencing is now imported at runtime (here and in tests/client/test_output_schema_validation.py) but is not declared in pyproject.toml dependencies — it is only available transitively through jsonschema>=4.20.0, so a future jsonschema release that drops or vendors it would break call_tool output validation with an ImportError. Declare referencing as 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 Unresolvable inside _validate_tool_result, executed on every call_tool for a tool with an outputSchema. pyproject.toml lists only jsonschema>=4.20.0 (line 37); referencing appears 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.0 range, or a downstream resolver installs jsonschema without referencing, _validate_tool_result raises 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: add referencing to [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_result on every call_tool for a tool with an output schema, and tests/client/test_output_schema_validation.py:8 also imports it at module level.

from referencing.exceptions import Unresolvable

if result.structuredContent is None:
raise RuntimeError(
f"Tool {name} has an output schema but did not return structured content"
) # pragma: no cover
# An explicit empty registry: `$ref`s resolve within the schema document and the bundled metaschemas.
registry: Registry[Any] = Registry()
try:
validate(result.structuredContent, output_schema)
validate(result.structuredContent, output_schema, registry=registry)
except ValidationError as e:
raise RuntimeError(f"Invalid structured content returned by tool {name}: {e}") # pragma: no cover
except SchemaError as e: # pragma: no cover
raise RuntimeError(f"Invalid schema for tool {name}: {e}") # pragma: no cover
except Unresolvable as e:
# A `$ref` did not resolve within the schema document.
raise RuntimeError(f"Invalid schema for tool {name}: {e}") from e

@overload
@deprecated("Use list_prompts(params=PaginatedRequestParams(...)) instead")
Expand Down
33 changes: 33 additions & 0 deletions tests/client/test_output_schema_validation.py
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 (
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 registry= is passed; with the explicit empty Registry() the validator combines it with the retrieve-less bundled SPECIFICATIONS, so lookup raises Unresolvable without ever invoking a retriever and the warning can never fire — the mark is dead and the comment falsely implies retrieval is still attempted under the fix. Drop both, or reword them as purely defensive.

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 if registry is not <default>: registry = SPECIFICATIONS.combine(registry); the warning-emitting retrieve function is attached only to the default registry used when no registry kwarg is given (that is how the base branch triggered the warning), and SPECIFICATIONS carries no retrieve function — it cannot, since referencing's Registry.combine raises "conflicting retrieval functions" when both sides define one, which would break the documented pattern of passing a registry with a custom retrieve. So for the file: $ref here, Resolver.lookup -> crawl -> get_or_retrieve hits referencing's _fail_to_retrieve, raising NoSuchResource, wrapped into Unresolvable and then jsonschema's _WrappedReferencingError; no retriever runs and no DeprecationWarning is emitted at any point (check_schema validates the schema as an instance and follows no instance-position refs either). pytest's default:... filterwarnings mark does not require

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: registry: Registry[Any] = Registry() then `validate(result.structuredContent, output_s

@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)
Loading