From 070c7cf00a14cffafad20c46d6c6e17c0191817e Mon Sep 17 00:00:00 2001 From: Rob Christiansen Date: Sun, 20 Sep 2026 22:41:55 -0600 Subject: [PATCH] fix(mcp): keep required-only constraint fragments in tool schemas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _repair_object_shape() treated every dict carrying `required` as an object declaration. A constraint fragment — {"required": ["chain"]} inside an allOf/oneOf/anyOf/if branch, with no properties and no type — is not one. Repairing it synthesised `properties: {}` and then pruned every name out of `required`, so sibling branches collapsed into identical always-true schemas and the enclosing oneOf had two matches: the tool appeared in the catalog and every call failed validation. Only the parameters-schema root still receives the dangling-`required` repair, which is the single node handed to providers as the argument object. --- tests/tools/test_schema_sanitizer.py | 93 ++++++++++++++++++++++++++++ tools/mcp_tool_schema.py | 41 +++++++----- 2 files changed, 120 insertions(+), 14 deletions(-) diff --git a/tests/tools/test_schema_sanitizer.py b/tests/tools/test_schema_sanitizer.py index e685a5b0b8..a5d5ff370c 100644 --- a/tests/tools/test_schema_sanitizer.py +++ b/tests/tools/test_schema_sanitizer.py @@ -526,6 +526,99 @@ def test_collapse_const_unions_does_not_mutate_input(): assert schema == snapshot +def test_normalize_mcp_input_schema_preserves_required_only_constraint_fragments(): + """``allOf``/``oneOf``/``if`` branches carrying only ``required`` must survive intact. + + Regression: ``_repair_object_shape`` treated every dict with a ``required`` list as an + object declaration, so a fragment like ``{"required": ["chain"]}`` got + ``type: object`` + ``properties: {}``, and the pruning step then deleted ``chain`` from + ``required`` — turning the branch into an always-true ``{"type": "object", + "properties": {}}``. Sibling branches collapsed into identical schemas, and the enclosing + ``oneOf`` (which demands exactly one match) could never be satisfied, so every call to that + tool failed client-side argument validation while the server-side tool was perfectly + healthy. Real-world repro: Morpho's MCP ``morpho_query_markets``, whose + ``allOf: [{oneOf: [{required: [chain]}, {required: [chainId]}]}]`` made the tool + permanently un-dispatchable. + """ + from tools.mcp_tool_schema import _normalize_mcp_input_schema + + schema = { + "type": "object", + "properties": { + "chain": {"type": "string"}, + "chainId": {"type": "integer"}, + "limit": {"type": "integer"}, + }, + "allOf": [{"oneOf": [{"required": ["chain"]}, {"required": ["chainId"]}]}], + } + out = _normalize_mcp_input_schema(schema) + + assert out["allOf"] == [{"oneOf": [{"required": ["chain"]}, {"required": ["chainId"]}]}] + + # Exactly-one semantics must be preserved, not just the branch text. jsonschema's + # Draft202012Validator is the same validator tools.tool_search_validation selects for a + # schema with no `$schema` key, so this exercises the real local-validation semantics. + from jsonschema.validators import Draft202012Validator + + validator_cls = Draft202012Validator + + def valid(args): + return validator_cls(out).is_valid(args) + + assert valid({"chain": "ethereum", "limit": 3}) + assert valid({"chainId": 1}) + assert not valid({}) + assert not valid({"chain": "ethereum", "chainId": 1}) + + +def test_normalize_mcp_input_schema_preserves_if_then_fragments(): + """``if``/``then`` fragments are constraint-only dicts and must not be object-repaired.""" + from tools.mcp_tool_schema import _normalize_mcp_input_schema + + out = _normalize_mcp_input_schema({ + "type": "object", + "properties": {"mode": {"type": "string"}, "id": {"type": "integer"}}, + "if": {"required": ["mode"]}, + "then": {"required": ["id"]}, + }) + assert out["if"] == {"required": ["mode"]} + assert out["then"] == {"required": ["id"]} + + +def test_normalize_mcp_input_schema_still_repairs_root_dangling_required(): + """The root dangling-``required`` repair (PR #4651) must keep firing. + + Only *nested* constraint fragments are exempt; the parameters-schema root is the single + node providers receive as the function's argument object, so a name in ``required`` that + has no matching ``properties`` entry still has to be pruned (Gemini 400s otherwise). + """ + from tools.mcp_tool_schema import _normalize_mcp_input_schema + + out = _normalize_mcp_input_schema({ + "type": "object", + "properties": {"a": {"type": "string"}}, + "required": ["a", "ghost"], + }) + assert out["required"] == ["a"] + + # Root typed object, required present, properties absent -> properties:{} still added. + bare = _normalize_mcp_input_schema({"type": "object", "required": ["a"]}) + assert bare["type"] == "object" + assert bare["properties"] == {} + + +def test_normalize_mcp_input_schema_still_repairs_nested_object_literals(): + """A nested dict that *does* declare ``properties`` is still a literal object schema.""" + from tools.mcp_tool_schema import _normalize_mcp_input_schema + + out = _normalize_mcp_input_schema({ + "type": "object", + "properties": {"opts": {"properties": {"x": {"type": "string"}}}}, + }) + assert out["properties"]["opts"]["type"] == "object" + assert out["properties"]["opts"]["properties"] == {"x": {"type": "string"}} + + def test_collapse_is_deterministic(): schema = {"anyOf": [{"const": "b"}, {"const": "a"}]} first = collapse_const_unions(copy.deepcopy(schema)) diff --git a/tools/mcp_tool_schema.py b/tools/mcp_tool_schema.py index d31d3387ff..249382f175 100644 --- a/tools/mcp_tool_schema.py +++ b/tools/mcp_tool_schema.py @@ -71,10 +71,21 @@ def _rewrite_local_refs(node): _SCHEMA_MAP_KEYS = ("properties", "patternProperties", "$defs", "definitions", "dependentSchemas") -def _repair_object_shape(node): +def _repair_object_shape(node, *, _object_root=False): """Recursively fill a missing object ``type``, ensure ``properties`` (so ``required`` can't dangle) and prune ``required`` to names present in ``properties`` (Gemini 400s - otherwise).""" + otherwise). + + A dict carrying ``required`` but declaring no ``properties`` and no ``type`` is NOT an + object declaration — it is a *constraint fragment*, the JSON Schema idiom for "at least + one of these keys must be present" (``allOf``/``oneOf``/``anyOf``/``if`` branches). + Synthesising ``properties: {}`` for it prunes every name out of ``required``, so sibling + branches collapse into identical always-true schemas; the enclosing ``oneOf`` then has + two matches and can never be satisfied, making the tool permanently un-dispatchable while + the server-side tool itself is fine. Only the parameters-schema ROOT still gets the + dangling-``required`` repair (``_object_root``), because that is the single node handed + to providers as the function's argument object. + """ if isinstance(node, list): return [_repair_object_shape(item) for item in node] if not isinstance(node, dict): @@ -88,17 +99,19 @@ def _repair_object_shape(node): repaired[key] = {name: _repair_object_shape(schema) for name, schema in value.items()} else: repaired[key] = _repair_object_shape(value) - if not repaired.get("type") and ("properties" in repaired or "required" in repaired): - repaired["type"] = "object" - if repaired.get("type") == "object": - if not isinstance(repaired.get("properties"), dict): - repaired["properties"] = {} - # Always a list: a missing/non-list ``required`` reads as ``null`` on strict - # OpenAI-compatible backends (#56123); ``[]`` is valid everywhere (Gemini included). - required = repaired.get("required") - props = repaired.get("properties") or {} - repaired["required"] = ([r for r in required if isinstance(r, str) and r in props] - if isinstance(required, list) else []) + # Constraint fragments are returned untouched: repairing them is what destroys them. + if _object_root or "properties" in repaired or "type" in node: + if not repaired.get("type") and ("properties" in repaired or "required" in repaired): + repaired["type"] = "object" + if repaired.get("type") == "object": + if not isinstance(repaired.get("properties"), dict): + repaired["properties"] = {} + # Always a list: a missing/non-list ``required`` reads as ``null`` on strict + # OpenAI-compatible backends (#56123); ``[]`` is valid everywhere (Gemini included). + required = repaired.get("required") + props = repaired.get("properties") or {} + repaired["required"] = ([r for r in required if isinstance(r, str) and r in props] + if isinstance(required, list) else []) return repaired @@ -123,7 +136,7 @@ def _normalize_mcp_input_schema(schema: dict | None) -> dict: normalized = _rewrite_local_refs(schema) normalized = strip_nullable_unions(normalized, keep_nullable_hint=True) normalized = collapse_const_unions(normalized) - normalized = _repair_object_shape(normalized) + normalized = _repair_object_shape(normalized, _object_root=True) if not isinstance(normalized, dict): return dict(_EMPTY_OBJECT_SCHEMA) if normalized.get("type") == "object" and "properties" not in normalized: