diff --git a/src/adcp/server/mcp_tools.py b/src/adcp/server/mcp_tools.py index 369a4891..5c8fbbad 100644 --- a/src/adcp/server/mcp_tools.py +++ b/src/adcp/server/mcp_tools.py @@ -18,7 +18,7 @@ from __future__ import annotations -import json +import copy import logging from collections.abc import Callable, Iterable from typing import Any @@ -1033,6 +1033,93 @@ def validate_discovery_set(tools: Iterable[str]) -> None: # ============================================================================ +def _inline_refs(schema: dict[str, Any]) -> dict[str, Any]: + """Resolve every local ``$ref`` into the referenced ``$defs`` body. + + Pydantic emits nested models as ``{"$ref": "#/$defs/Name"}`` with the + actual shape under ``$defs``. That's spec-valid JSON Schema, but the + MCP client ecosystem is mixed — several popular consumers (including + some of the cheaper agent runtimes we see in validation runs) don't + implement ``$ref`` resolution. Tool discovery that looks correct in + MCP Inspector shows up as ``{}`` to those clients, producing silent + "this tool takes no params" confusion. + + The inliner walks the schema tree and replaces each ``$ref`` with a + deep copy of the referenced definition. Sibling keys on the ``$ref`` + node (``description``, ``title``) are merged on top of the resolved + body. Note: this is an annotation-level override that matches what + Pydantic actually emits at reference sites — it is NOT spec §8.2 + merge semantics (which would evaluate siblings as an implicit + ``allOf``). If a future Pydantic version starts emitting + assertion-level siblings (``type``, ``enum``, etc.) the merge + would silently change validation; today it doesn't. + + Only handles local refs (``#/$defs/X``). External refs are left in + place — Pydantic doesn't emit them for our request models, but if + one ever appears it surfaces to the caller rather than being + silently stripped. + + Cycles are protected by a ``seen`` set threaded through recursion. + Pydantic request models don't generate cyclic refs today; the guard + exists so a future schema shape can't turn inlining into a + RecursionError. When the walk leaves at least one ``$ref`` + unresolved (cycle or dangling), ``$defs`` is kept in place so a + spec-compliant client can still resolve what we couldn't. + """ + defs = schema.get("$defs", {}) + # Track whether we emitted any $ref in the output — tells the + # caller whether it's safe to drop $defs. Avoids a + # stringify-the-whole-tree scan post-walk, and sidesteps false + # positives from legitimate ``"$ref"`` values inside enum / const + # / description strings. + unresolved = [False] + + def _resolve(node: Any, seen: frozenset[str]) -> Any: + if isinstance(node, dict): + ref = node.get("$ref") + if isinstance(ref, str): + if not ref.startswith("#/$defs/"): + # External ref (http://…, relative path). Pydantic + # doesn't emit these for our request models; leave + # untouched rather than risk silent corruption. + unresolved[0] = True + return {k: _resolve(v, seen) for k, v in node.items()} + def_name = ref[len("#/$defs/") :] + if def_name in seen: + # Cycle — leave the $ref intact so a spec-compliant + # client can still resolve via $defs. + unresolved[0] = True + return {k: _resolve(v, seen) for k, v in node.items()} + body = defs.get(def_name) + if body is None: + # Dangling ref — nothing in $defs matches. Leave + # the $ref for consumers to error on; preserving + # the shape is safer than silently stripping. + unresolved[0] = True + return {k: _resolve(v, seen) for k, v in node.items()} + resolved = _resolve(copy.deepcopy(body), seen | {def_name}) + # Annotation-level merge — sibling description/title + # on the $ref node wins over the resolved body's + # same-named keys. + merged = dict(resolved) if isinstance(resolved, dict) else resolved + if isinstance(merged, dict): + for k, v in node.items(): + if k == "$ref": + continue + merged[k] = _resolve(v, seen) + return merged + return {k: _resolve(v, seen) for k, v in node.items()} + if isinstance(node, list): + return [_resolve(item, seen) for item in node] + return node + + result = _resolve(schema, frozenset()) + if isinstance(result, dict) and not unresolved[0]: + result.pop("$defs", None) + assert isinstance(result, dict) + return result + + def _generate_pydantic_schemas() -> dict[str, dict[str, Any]]: """Generate JSON schemas from Pydantic request models. @@ -1207,11 +1294,11 @@ def _generate_pydantic_schemas() -> dict[str, dict[str, Any]]: if "anyOf" in schema or "$ref" in schema: continue - # Only strip $defs if no $ref references exist in the schema. - # If nested properties use $ref, keep $defs so references resolve. - schema_str = json.dumps(schema) - if '"$ref"' not in schema_str: - schema.pop("$defs", None) + # Inline every $ref into its $defs body so MCP clients that + # don't resolve JSON-Schema references (a surprisingly large + # slice of the ecosystem) still see the full tool surface. + # Spec-wise the schema is equivalent — just flat. + schema = _inline_refs(schema) schemas[tool_name] = schema except Exception: diff --git a/tests/test_mcp_schema_drift.py b/tests/test_mcp_schema_drift.py index 143860c5..38aaf166 100644 --- a/tests/test_mcp_schema_drift.py +++ b/tests/test_mcp_schema_drift.py @@ -27,6 +27,7 @@ _PYDANTIC_SCHEMAS, ADCP_TOOL_DEFINITIONS, _generate_pydantic_schemas, + _inline_refs, ) @@ -140,3 +141,343 @@ def test_spot_check_real_fields_reach_clients() -> None: assert req in create_media_buy.get( "required", [] ), f"create_media_buy should require {req!r}" + + +# --------------------------------------------------------------------------- +# $defs inlining invariants (closes #208) +# --------------------------------------------------------------------------- + + +def test_no_dollar_ref_in_any_advertised_schema() -> None: + """Every tool's inputSchema must be ``$ref``-free. MCP clients that + don't implement JSON Schema reference resolution (a surprisingly + large slice of the ecosystem) see ``{"$ref": ...}`` as an empty + object — which means "this tool takes no params" in their + interpretation. Inlining is the only way to give those clients the + full tool surface. Regression here silently re-breaks discovery for + those clients.""" + for tool in ADCP_TOOL_DEFINITIONS: + serialized = json.dumps(tool["inputSchema"]) + assert '"$ref"' not in serialized, ( + f"tool {tool['name']!r} inputSchema contains unresolved $ref. " + "Check _inline_refs in adcp.server.mcp_tools." + ) + + +def test_no_dollar_defs_in_any_advertised_schema() -> None: + """After inlining, ``$defs`` serves no purpose and is noise on the + wire. Drop it so the advertised schema is minimal.""" + for tool in ADCP_TOOL_DEFINITIONS: + serialized = json.dumps(tool["inputSchema"]) + assert '"$defs"' not in serialized, ( + f"tool {tool['name']!r} inputSchema retains $defs block after " + "inlining. Check _inline_refs drop-when-resolved path." + ) + + +# --------------------------------------------------------------------------- +# _inline_refs unit tests — behavior-level guarantees +# --------------------------------------------------------------------------- + + +def test_inline_refs_replaces_local_ref_with_body() -> None: + """The core transform: ``{"$ref": "#/$defs/X"}`` becomes the body + of ``$defs["X"]``.""" + schema = { + "type": "object", + "properties": {"user": {"$ref": "#/$defs/User"}}, + "$defs": { + "User": { + "type": "object", + "properties": {"name": {"type": "string"}}, + } + }, + } + result = _inline_refs(schema) + assert result["properties"]["user"] == { + "type": "object", + "properties": {"name": {"type": "string"}}, + } + assert "$defs" not in result + + +def test_inline_refs_resolves_nested_refs() -> None: + """A $def that itself references another $def must be fully + resolved in one pass. Without recursion, the second level stays + as a $ref.""" + schema = { + "type": "object", + "properties": {"order": {"$ref": "#/$defs/Order"}}, + "$defs": { + "Order": { + "type": "object", + "properties": {"customer": {"$ref": "#/$defs/Customer"}}, + }, + "Customer": { + "type": "object", + "properties": {"id": {"type": "string"}}, + }, + }, + } + result = _inline_refs(schema) + # Both levels resolved in the output. + assert result["properties"]["order"]["properties"]["customer"]["properties"]["id"] == { + "type": "string" + } + assert '"$ref"' not in json.dumps(result) + + +def test_inline_refs_sibling_annotations_override_resolved_body() -> None: + """Annotation-level merge: sibling ``description`` / ``title`` on + the $ref node win over the resolved body's same-named keys. This + is what Pydantic emits at ref sites in practice (a field-level + description on top of a nested model). + + Note: this is NOT JSON Schema 2020-12 §8.2 merge semantics (which + would evaluate siblings as an implicit ``allOf``). The inliner's + override semantics match Pydantic's actual output, not the spec's + general-case composition rule. If a future Pydantic version + emits assertion-level siblings at ref sites (``type``, ``enum``, + etc.), this merge would silently clobber them — today it doesn't.""" + schema = { + "properties": { + "account": { + "$ref": "#/$defs/Account", + "description": "This one is special — overrides the generic description.", + } + }, + "$defs": { + "Account": { + "type": "object", + "description": "Generic account description.", + "properties": {"id": {"type": "string"}}, + } + }, + } + result = _inline_refs(schema) + assert ( + result["properties"]["account"]["description"] + == "This one is special — overrides the generic description." + ) + assert result["properties"]["account"]["properties"] == {"id": {"type": "string"}} + + +def test_inline_refs_resolves_inside_anyof() -> None: + """Pydantic emits ``anyOf: [{"$ref": "..."}, {"type": "null"}]`` + for ``Optional[Model]`` on request types. The inliner MUST recurse + into composition keywords (``anyOf`` / ``oneOf`` / ``allOf``) so + optional nested models still flatten correctly. Real production + case — regression here silently breaks optional-model properties + for non-ref-resolving clients.""" + schema = { + "type": "object", + "properties": {"maybe_account": {"anyOf": [{"$ref": "#/$defs/Account"}, {"type": "null"}]}}, + "$defs": { + "Account": { + "type": "object", + "properties": {"id": {"type": "string"}}, + } + }, + } + result = _inline_refs(schema) + any_of = result["properties"]["maybe_account"]["anyOf"] + # First branch — Account resolved inline. + assert any_of[0] == { + "type": "object", + "properties": {"id": {"type": "string"}}, + } + # Second branch — unchanged. + assert any_of[1] == {"type": "null"} + assert "$defs" not in result + + +def test_inline_refs_resolves_inside_additional_properties() -> None: + """``additionalProperties: {"$ref": "..."}`` is another shape + Pydantic emits — e.g. ``dict[str, NestedModel]`` on a request + field. Must resolve like any other $ref.""" + schema = { + "type": "object", + "properties": { + "accounts_by_id": { + "type": "object", + "additionalProperties": {"$ref": "#/$defs/Account"}, + } + }, + "$defs": { + "Account": { + "type": "object", + "properties": {"name": {"type": "string"}}, + } + }, + } + result = _inline_refs(schema) + assert result["properties"]["accounts_by_id"]["additionalProperties"] == { + "type": "object", + "properties": {"name": {"type": "string"}}, + } + assert "$defs" not in result + + +def test_inline_refs_does_not_false_positive_on_ref_as_value() -> None: + """The ``$defs``-drop decision must not treat a legitimate + ``"$ref"`` value inside an enum / const / description as an + unresolved reference. A description that mentions the word + ``"$ref"`` in prose, or an enum with ``"$ref"`` as a literal + string, would be a false positive under a naive substring check.""" + schema = { + "type": "object", + "properties": { + "keyword": { + "type": "string", + "description": 'Must be a JSON Schema keyword like "$ref" or "$id".', + } + }, + } + result = _inline_refs(schema) + # $defs was never present — no issue here specifically — but + # verify the description survived and the result is otherwise + # unchanged. + assert ( + result["properties"]["keyword"]["description"] + == 'Must be a JSON Schema keyword like "$ref" or "$id".' + ) + + +def test_inline_refs_protects_against_cycles() -> None: + """Pydantic doesn't emit cyclic refs today, but a future request + model could. Cycle protection must leave the original ``$ref`` + intact (and keep $defs) so the caller still has resolvable data.""" + schema = { + "type": "object", + "properties": {"node": {"$ref": "#/$defs/Node"}}, + "$defs": { + "Node": { + "type": "object", + "properties": {"child": {"$ref": "#/$defs/Node"}}, + } + }, + } + result = _inline_refs(schema) + # Cycle detected — the inner $ref survives because it'd recurse + # forever. $defs stays so it's still resolvable for spec-compliant + # clients. + assert '"$ref"' in json.dumps(result) + assert "$defs" in result + + +def test_inline_refs_dangling_ref_leaves_schema_alone() -> None: + """A $ref pointing at a non-existent $def is a spec error, but the + inliner shouldn't crash — leave both the $ref and the (empty) + $defs intact so a spec-compliant client's error surface fires + with the right shape.""" + schema = { + "type": "object", + "properties": {"bad": {"$ref": "#/$defs/NotThere"}}, + "$defs": {}, + } + result = _inline_refs(schema) + # Dangling $ref survives; $defs kept because the ref didn't resolve. + assert '"$ref"' in json.dumps(result) + + +def test_inline_refs_ignores_external_refs() -> None: + """External $refs (``http://…``, relative paths) are spec-valid + but aren't something Pydantic emits for our request models. If + one ever shows up, leave it alone — silently stripping it would + corrupt the schema.""" + schema = { + "type": "object", + "properties": {"x": {"$ref": "https://example.com/schema.json"}}, + } + result = _inline_refs(schema) + assert result["properties"]["x"] == {"$ref": "https://example.com/schema.json"} + + +def test_inline_refs_preserves_required_arrays() -> None: + """Inlining must not lose the ``required`` array on a resolved + definition. Agents constructing payloads read this to know which + fields are mandatory.""" + schema = { + "type": "object", + "properties": {"user": {"$ref": "#/$defs/User"}}, + "$defs": { + "User": { + "type": "object", + "required": ["name", "email"], + "properties": { + "name": {"type": "string"}, + "email": {"type": "string"}, + }, + } + }, + } + result = _inline_refs(schema) + assert result["properties"]["user"]["required"] == ["name", "email"] + + +def test_inline_refs_handles_arrays_of_refs() -> None: + """``items: {"$ref": "..."}`` must resolve just like top-level + property refs.""" + schema = { + "type": "object", + "properties": { + "users": { + "type": "array", + "items": {"$ref": "#/$defs/User"}, + } + }, + "$defs": { + "User": { + "type": "object", + "properties": {"id": {"type": "string"}}, + } + }, + } + result = _inline_refs(schema) + assert result["properties"]["users"]["items"]["properties"] == {"id": {"type": "string"}} + assert "$defs" not in result + + +def test_inline_refs_does_not_mutate_input() -> None: + """The inliner must return a new object — callers may want to keep + the pre-inline form (e.g. for comparison against Pydantic's fresh + output). Mutating the input silently breaks that contract.""" + schema = { + "properties": {"user": {"$ref": "#/$defs/User"}}, + "$defs": {"User": {"type": "object"}}, + } + before = json.dumps(schema, sort_keys=True) + _inline_refs(schema) + after = json.dumps(schema, sort_keys=True) + assert before == after, "inliner must not mutate its input" + + +# --------------------------------------------------------------------------- +# End-to-end — inlined schema accepts the same valid input as Pydantic +# --------------------------------------------------------------------------- + + +def test_inlined_schema_still_validates_real_request() -> None: + """The inlined schema must accept every payload the original + Pydantic model accepts. Structural equivalence — the shape is + preserved, just flattened. Round-trip via jsonschema's validator + against a concrete minimal valid payload.""" + import jsonschema + import pytest + + from adcp.types import GetProductsRequest + + tool_schemas = {t["name"]: t["inputSchema"] for t in ADCP_TOOL_DEFINITIONS} + get_products = tool_schemas["get_products"] + + # Minimal valid payload per the model. + payload = {"buying_mode": "brief"} + + # Build validator from the inlined schema; payload must pass. + try: + jsonschema.validate(payload, get_products) + except jsonschema.ValidationError as exc: # pragma: no cover + pytest.fail(f"inlined schema rejected a valid payload: {exc}") + + # And Pydantic still accepts it — both sides of the equivalence. + GetProductsRequest.model_validate(payload)