diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index e25756647487..7cb3f7391f83 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -185,6 +185,113 @@ def test_nested_definition_refs_are_rewritten_recursively(self): assert schema["parameters"]["properties"]["items"]["items"]["$ref"] == "#/$defs/Entry" assert schema["parameters"]["$defs"]["Entry"]["properties"]["child"]["$ref"] == "#/$defs/Child" + def test_property_named_definitions_is_not_renamed(self): + """A user parameter named ``definitions`` must NOT be renamed to ``$defs``. + + Regression for the Azure DevOps MCP ``pipelines_get_builds`` tool, + whose Zod schema legitimately exposes ``definitions: z.array(...)`` as + a parameter name. Previously ``_rewrite_local_refs`` blindly renamed + every dict key named ``definitions`` (including parameter names inside + ``properties``), producing ``properties.$defs`` which then failed + Anthropic's ``^[a-zA-Z0-9_.-]{1,64}$`` validator with HTTP 400. + """ + from tools.mcp_tool import _normalize_mcp_input_schema + + schema = _normalize_mcp_input_schema({ + "type": "object", + "properties": { + "project": {"type": "string"}, + "definitions": { + "type": "array", + "items": {"type": "number", "minimum": 1}, + "description": "Array of build definition IDs to filter builds", + }, + }, + "required": ["project"], + }) + + assert "definitions" in schema["properties"] + assert "$defs" not in schema["properties"] + assert schema["properties"]["definitions"]["type"] == "array" + # And the schema-keyword form is still rewritten correctly when present + # as a sibling of properties (covered by other tests). + + def test_property_named_definitions_alongside_schema_definitions(self): + """Both forms can coexist: keyword renamed, property kept verbatim.""" + from tools.mcp_tool import _normalize_mcp_input_schema + + schema = _normalize_mcp_input_schema({ + "type": "object", + "properties": { + "definitions": {"type": "array", "items": {"type": "string"}}, + "payload": {"$ref": "#/definitions/P"}, + }, + "required": ["definitions"], + "definitions": { + "P": {"type": "object", "properties": {"k": {"type": "string"}}}, + }, + }) + + # Schema-keyword form was rewritten + assert "$defs" in schema + assert "definitions" not in {k for k in schema.keys() if k != "properties"} - {"properties"} + assert "P" in schema["$defs"] + assert schema["properties"]["payload"]["$ref"] == "#/$defs/P" + # Property-name form preserved verbatim + assert "definitions" in schema["properties"] + assert schema["properties"]["definitions"]["type"] == "array" + # Required still references the original parameter name + assert "definitions" in schema["required"] + + def test_invalid_property_keys_are_sanitized(self): + """Property keys outside ^[a-zA-Z0-9_.-]{1,64}$ are renamed defensively. + + Belt-and-suspenders pass: if an MCP server emits a property key with + characters Anthropic rejects (``$``, ``@``, spaces, unicode, …), we + rename it so the whole tool list still validates. + """ + from tools.mcp_tool import _normalize_mcp_input_schema + + schema = _normalize_mcp_input_schema({ + "type": "object", + "properties": { + "ok": {"type": "string"}, + "$weird": {"type": "string"}, + "has spaces": {"type": "integer"}, + }, + "required": ["$weird"], + }) + + keys = list(schema["properties"].keys()) + import re as _re + regex = _re.compile(r"^[a-zA-Z0-9_.-]{1,64}$") + for k in keys: + assert regex.match(k), f"key {k!r} still fails Anthropic regex" + assert "ok" in schema["properties"] + # ``$weird`` -> ``_weird``; required list is rewritten in lockstep + assert "_weird" in schema["properties"] + assert "_weird" in schema["required"] + # ``has spaces`` -> ``has_spaces`` + assert "has_spaces" in schema["properties"] + + def test_valid_property_keys_are_unchanged(self): + """Sanitizer must be a no-op on clean schemas.""" + from tools.mcp_tool import _normalize_mcp_input_schema + + inp = { + "type": "object", + "properties": { + "project": {"type": "string"}, + "x.y": {"type": "string"}, + "snake_case": {"type": "integer"}, + "kebab-case": {"type": "boolean"}, + }, + "required": ["project"], + } + schema = _normalize_mcp_input_schema(inp) + assert set(schema["properties"].keys()) == {"project", "x.y", "snake_case", "kebab-case"} + assert schema["required"] == ["project"] + 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 b3a7cd2d5ce6..a5b2f4a83fe9 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -2911,18 +2911,41 @@ def _normalize_mcp_input_schema(schema: dict | None) -> dict: if not schema: return {"type": "object", "properties": {}} - def _rewrite_local_refs(node): + def _rewrite_local_refs(node, *, inside_properties=False): + """Rewrite draft-07 ``definitions`` to draft-2020 ``$defs``. + + IMPORTANT: only the *schema keyword* ``definitions`` is renamed. When + the recursion is descending through a ``properties`` dict, its keys + are *user-defined parameter names* — never JSON-Schema keywords — and + must be preserved verbatim. Renaming them produces ``properties.$defs`` + (a top-level reserved keyword) and breaks downstream validators that + forbid ``$`` in property names (e.g. Anthropic's + ``^[a-zA-Z0-9_.-]{1,64}$`` regex). See: Azure DevOps MCP's + ``pipelines_get_builds`` tool, whose Zod schema legitimately exposes a + parameter named ``definitions``. + """ if isinstance(node, dict): normalized = {} for key, value in node.items(): + if inside_properties: + # Inside a properties dict: keys are user parameter names, + # not schema keywords. Never rewrite. Recurse into each + # value as a fresh schema node. + normalized[key] = _rewrite_local_refs(value, inside_properties=False) + continue out_key = "$defs" if key == "definitions" else key - normalized[out_key] = _rewrite_local_refs(value) + # The value of a ``properties`` key is itself a dict whose own + # keys are parameter names. Mark that boundary for the recursion. + child_inside_properties = (key == "properties") and isinstance(value, dict) + normalized[out_key] = _rewrite_local_refs( + value, inside_properties=child_inside_properties + ) ref = normalized.get("$ref") if isinstance(ref, str) and ref.startswith("#/definitions/"): normalized["$ref"] = "#/$defs/" + ref[len("#/definitions/"):] return normalized if isinstance(node, list): - return [_rewrite_local_refs(item) for item in node] + return [_rewrite_local_refs(item, inside_properties=False) for item in node] return node def _strip_nullable_union(node): @@ -2976,9 +2999,70 @@ def _repair_object_shape(node): return repaired + # Anthropic's tool-schema validator rejects ``properties`` keys that don't + # match ``^[a-zA-Z0-9_.-]{1,64}$``. Most other providers are looser, but + # the strictest cross-provider intersection is roughly this regex, so we + # apply it as a final belt-and-suspenders pass. Sanitization replaces + # disallowed characters with ``_`` and truncates over-long keys; collisions + # get an integer suffix. A warning is logged so server authors can fix the + # upstream Zod / Pydantic schema. + _SAFE_PROP_KEY = re.compile(r"^[a-zA-Z0-9_.-]{1,64}$") + + def _sanitize_property_keys(node, *, path="$", tool_hint=""): + if isinstance(node, list): + return [_sanitize_property_keys(item, path=f"{path}[{i}]", tool_hint=tool_hint) for i, item in enumerate(node)] + if not isinstance(node, dict): + return node + out = {} + for k, v in node.items(): + if k == "properties" and isinstance(v, dict): + new_props: dict = {} + for pk, pv in v.items(): + safe_pk = pk + if not isinstance(pk, str) or not _SAFE_PROP_KEY.match(pk): + raw = pk if isinstance(pk, str) else str(pk) + safe_pk = re.sub(r"[^a-zA-Z0-9_.-]", "_", raw)[:64] or "_" + # Disambiguate against any existing/sibling key + base = safe_pk + suffix = 1 + while safe_pk in new_props: + safe_pk = f"{base[:60]}_{suffix}" + suffix += 1 + try: + logger.warning( + "MCP schema: renamed invalid property key %r -> %r " + "at %s.properties (tool=%s) to satisfy provider " + "regex ^[a-zA-Z0-9_.-]{1,64}$", + raw, safe_pk, path, tool_hint or "?", + ) + except Exception: + pass + new_props[safe_pk] = _sanitize_property_keys( + pv, path=f"{path}.properties.{safe_pk}", tool_hint=tool_hint + ) + # Update ``required`` to use the renamed keys when applicable + req = node.get("required") + if isinstance(req, list): + # We can only safely rename in ``required`` if we have a 1:1 map, + # which we do because pk -> safe_pk is built in order. + rename = {} + for (orig_pk, _), (new_pk, _) in zip(v.items(), new_props.items()): + if orig_pk != new_pk: + rename[orig_pk] = new_pk + if rename: + out["required"] = [rename.get(r, r) for r in req] + out[k] = new_props + elif k == "required" and "required" in out: + # Already handled above when we rewrote ``properties``. + continue + else: + out[k] = _sanitize_property_keys(v, path=f"{path}.{k}", tool_hint=tool_hint) + return out + normalized = _rewrite_local_refs(schema) normalized = _strip_nullable_union(normalized) normalized = _repair_object_shape(normalized) + normalized = _sanitize_property_keys(normalized) # Ensure top-level is a well-formed object schema if not isinstance(normalized, dict):