mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
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)
This commit is contained in:
parent
bc6cd46925
commit
dd22c2f533
2 changed files with 117 additions and 2 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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/"):]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue