fix(mcp): keep required-only constraint fragments in tool schemas
_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.
This commit is contained in:
committed by
Teknium
parent
fca3221df4
commit
070c7cf00a
@@ -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))
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user