From dd22c2f5333eff799c670f64b9d494594fe5831b Mon Sep 17 00:00:00 2001 From: Matt Kotsenas <51421+MattKotsenas@users.noreply.github.com> Date: Fri, 22 May 2026 08:03:55 -0700 Subject: [PATCH] fix(mcp): preserve 'definitions' as a property name in tool schemas The MCP input-schema normalizer in _normalize_mcp_input_schema promotes the legacy JSON Schema 'definitions' meta-keyword to '$defs' (draft 2019-09+) so local '$ref' resolution works downstream. The previous walk renamed *any* key named 'definitions' anywhere in the tree, including inside 'properties' dicts. That turned user-facing parameter names into '$defs', producing property keys that contain '$', which Anthropic and OpenAI both reject with HTTP 400 (pattern '^[a-zA-Z0-9_.-]{1,64}$'). Real-world repro: an MCP server that exposes a CI/pipelines tool whose 'definitions' parameter is an array of pipeline-definition IDs. Such a tool is enough on its own to break every conversation, because the full tools array is sent on every request. Fix: when descending into a 'properties' or 'patternProperties' mapping, iterate property-name -> schema pairs directly, leaving the property names verbatim. Ordinary JSON Schema semantics resume inside each property's schema, so a legitimately nested 'definitions' meta-keyword inside a property's schema is still promoted. Adds two regression tests: - test_definitions_as_property_name_is_preserved (the property-name case) - test_definitions_property_and_meta_keyword_coexist (both forms in one schema; the property name stays, the meta-keyword promotes) --- tests/tools/test_mcp_tool.py | 83 ++++++++++++++++++++++++++++++++++++ tools/mcp_tool.py | 36 +++++++++++++++- 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index c299e506d1a..3fafaf67101 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -235,6 +235,89 @@ class TestSchemaConversion: assert schema["parameters"]["properties"]["items"]["items"]["$ref"] == "#/$defs/Entry" assert schema["parameters"]["$defs"]["Entry"]["properties"]["child"]["$ref"] == "#/$defs/Child" + def test_definitions_as_property_name_is_preserved(self): + """A tool parameter literally named ``definitions`` must not be renamed. + + Regression: the rewrite that promotes the legacy ``definitions`` + meta-keyword to ``$defs`` used to fire for *any* key named + ``definitions`` anywhere in the tree, including inside ``properties`` + dicts. That turned user-facing parameter names into ``$defs``, which + Anthropic and OpenAI both reject because ``$`` is not in the + ``^[a-zA-Z0-9_.-]{1,64}$`` property-name pattern. Real-world repro: a + CI/pipelines MCP tool whose ``definitions`` parameter is an array of + pipeline-definition IDs. + """ + from tools.mcp_tool import _convert_mcp_schema + + mcp_tool = _make_mcp_tool( + name="pipelines_build", + description="List pipeline builds", + input_schema={ + "type": "object", + "properties": { + "action": {"type": "string"}, + "definitions": { + "description": "Array of build definition IDs to filter builds.", + }, + "top": {"type": "integer"}, + }, + }, + ) + + schema = _convert_mcp_schema("pipelines", mcp_tool) + + props = schema["parameters"]["properties"] + assert "definitions" in props, "user-facing property name was renamed away" + assert "$defs" not in props, "user-facing property name was rewritten to $defs" + # And the meta-keyword promotion didn't happen at the root either, + # because there was no `definitions` meta-keyword to promote. + assert "$defs" not in schema["parameters"] + assert "definitions" not in schema["parameters"] + + def test_definitions_property_and_meta_keyword_coexist(self): + """``definitions`` as both a property name AND a meta-keyword in the + same schema. The property name stays; the meta-keyword is promoted. + + Note: Python source can't express both keys as literals (the second + would clobber the first), so build the dict explicitly. + """ + from tools.mcp_tool import _convert_mcp_schema + + input_schema = { + "type": "object", + "properties": { + # User-facing parameter literally named "definitions". + "definitions": { + "description": "Array of build definition IDs.", + }, + "payload": {"$ref": "#/definitions/Payload"}, + }, + } + # Meta-keyword (legacy draft-07 reusable defs), set after the literal. + input_schema["definitions"] = { + "Payload": { + "type": "object", + "properties": {"q": {"type": "string"}}, + }, + } + + mcp_tool = _make_mcp_tool( + name="mixed", + description="Schema with both forms of `definitions`", + input_schema=input_schema, + ) + + schema = _convert_mcp_schema("mixed", mcp_tool) + + # Property name preserved. + assert "definitions" in schema["parameters"]["properties"] + assert "$defs" not in schema["parameters"]["properties"] + # Meta-keyword promoted at the root. + assert "$defs" in schema["parameters"] + assert "definitions" not in schema["parameters"] + # The $ref into the legacy location was rewritten too. + assert schema["parameters"]["properties"]["payload"]["$ref"] == "#/$defs/Payload" + def test_missing_type_on_object_is_coerced(self): """Schemas that describe an object but omit ``type`` get type='object'.""" from tools.mcp_tool import _normalize_mcp_input_schema diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index c125db62a11..211d5ea65a7 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3731,11 +3731,43 @@ def _normalize_mcp_input_schema(schema: dict | None) -> dict: return {"type": "object", "properties": {}} def _rewrite_local_refs(node): + """Walk the schema, promoting legacy ``definitions`` to ``$defs``. + + The promotion is contextual: ``definitions`` is renamed only when it + appears as a JSON Schema *meta-keyword* (sibling of ``properties`` / + ``$ref`` at a schema node), never when it appears as the *name of a + property* (i.e., as a key inside a ``properties`` dict). + + Without this gate, MCP servers that legitimately expose a tool + parameter named ``definitions`` (e.g. a CI/pipelines tool that uses + ``definitions`` for an array of pipeline-definition IDs) would have + that user-facing property name silently rewritten to ``$defs``. + Anthropic and OpenAI both reject ``$`` in property names + (``^[a-zA-Z0-9_.-]{1,64}$``), so the whole tool array gets a 400 and + every conversation breaks. + + The gate works by treating ``properties`` and ``patternProperties`` + specially during descent: we iterate the property-name -> schema map + directly, leaving the property names verbatim, then recurse into each + property's schema where ordinary JSON Schema semantics resume (so any + legitimately-nested ``definitions`` meta-keyword inside a property's + schema is still promoted). + """ if isinstance(node, dict): normalized = {} for key, value in node.items(): - out_key = "$defs" if key == "definitions" else key - normalized[out_key] = _rewrite_local_refs(value) + if key in ("properties", "patternProperties") and isinstance(value, dict): + # Keys of this dict are user-facing property names, not + # meta-keywords. Preserve them verbatim; recurse only into + # each property's schema, where ``definitions`` again has + # its JSON Schema meaning. + normalized[key] = { + prop_name: _rewrite_local_refs(prop_schema) + for prop_name, prop_schema in value.items() + } + else: + out_key = "$defs" if key == "definitions" else key + normalized[out_key] = _rewrite_local_refs(value) ref = normalized.get("$ref") if isinstance(ref, str) and ref.startswith("#/definitions/"): normalized["$ref"] = "#/$defs/" + ref[len("#/definitions/"):]