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/"):]