mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-23 16:36:23 +00:00
fix(tools): rename provider-illegal property keys in tool schemas, reverse-map at dispatch
Cloudflare's flat API MCP ships 61 property keys that violate Anthropic's
^[a-zA-Z0-9_.-]{1,64}$ pattern (query-filter params like 'issue_class~neq'
and 'meta.<field>[<operator>]'). One bad key anywhere in the tools array
400s the ENTIRE request — measured live: Anthropic, Bedrock, Google Vertex,
and Azure all rejected an eager 3,320-tool Cloudflare request at validation,
before token limits even applied.
- schema_sanitizer: rename non-conforming property keys deterministically
(bad chars -> '_', 64-char truncation, collision dedup with numeric
suffixes), nested schemas included; required[] remapped alongside
- unrename_tool_args(): reverse map applied in coerce_tool_args at dispatch,
so the MCP server receives the original wire names; recurses into object
values and array items
- deterministic on both sides: the rename map is recomputed from the
registry's original schema at dispatch time, no state carried
E2E on the real capture: 61 -> 0 violations across 3,320 tools; round-trip
verified on get_accounts_intel_attacksurfacereport_issues (5 '~neq' keys).
This commit is contained in:
parent
c80ce5aae2
commit
91da5ca28e
3 changed files with 212 additions and 6 deletions
|
|
@ -715,6 +715,16 @@ def coerce_tool_args(tool_name: str, args: Dict[str, Any]) -> Dict[str, Any]:
|
|||
if not properties:
|
||||
return args
|
||||
|
||||
# The model saw the SANITIZED schema — property keys violating provider
|
||||
# patterns (e.g. Cloudflare's ``issue_class~neq``) were renamed before
|
||||
# the request. Map any sanitized keys back to the registry's original
|
||||
# wire names before schema lookup / dispatch.
|
||||
try:
|
||||
from tools.schema_sanitizer import unrename_tool_args
|
||||
args = unrename_tool_args(schema.get("parameters"), args)
|
||||
except Exception: # pragma: no cover — never break dispatch
|
||||
pass
|
||||
|
||||
for key, value in list(args.items()):
|
||||
prop_schema = properties.get(key)
|
||||
if not prop_schema:
|
||||
|
|
|
|||
|
|
@ -759,3 +759,111 @@ def test_strip_slash_enum_ignores_non_string_enum_values():
|
|||
props = tools[0]["function"]["parameters"]["properties"]
|
||||
assert props["level"]["enum"] == [1, 2, 3]
|
||||
assert props["flag"]["enum"] == [True, False]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Property-key renaming (provider ^[a-zA-Z0-9_.-]{1,64}$ pattern compat)
|
||||
# Real-world source: Cloudflare flat API MCP ships keys like
|
||||
# ``issue_class~neq`` and ``meta.<field>[<operator>]`` — one bad key anywhere
|
||||
# in the tools array 400s the whole request on Anthropic/Bedrock/Vertex/Azure.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
from tools.schema_sanitizer import sanitize_property_key, unrename_tool_args
|
||||
|
||||
|
||||
def test_bad_property_keys_renamed_to_conforming():
|
||||
tools = [_tool("cf_issues", {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"issue_class~neq": {"type": "string"},
|
||||
"meta.<field>[<operator>]": {"type": "string"},
|
||||
"normal_key": {"type": "string"},
|
||||
},
|
||||
"required": ["issue_class~neq"],
|
||||
})]
|
||||
out = sanitize_tool_schemas(tools)
|
||||
props = out[0]["function"]["parameters"]["properties"]
|
||||
import re
|
||||
pat = re.compile(r"^[a-zA-Z0-9_.-]{1,64}$")
|
||||
assert all(pat.match(k) for k in props), list(props)
|
||||
assert "normal_key" in props
|
||||
assert "issue_class_neq" in props
|
||||
# required remapped alongside
|
||||
assert out[0]["function"]["parameters"]["required"] == ["issue_class_neq"]
|
||||
|
||||
|
||||
def test_property_key_over_64_chars_truncated():
|
||||
long_key = "k" * 80
|
||||
tools = [_tool("t", {"type": "object", "properties": {long_key: {"type": "string"}}})]
|
||||
out = sanitize_tool_schemas(tools)
|
||||
props = out[0]["function"]["parameters"]["properties"]
|
||||
assert list(props) == ["k" * 64]
|
||||
|
||||
|
||||
def test_rename_collision_deduped_deterministically():
|
||||
tools = [_tool("t", {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"a~b": {"type": "string"},
|
||||
"a b": {"type": "integer"},
|
||||
"a_b": {"type": "boolean"}, # already owns the sanitized name
|
||||
},
|
||||
})]
|
||||
out = sanitize_tool_schemas(tools)
|
||||
props = out[0]["function"]["parameters"]["properties"]
|
||||
assert props["a_b"]["type"] == "boolean" # original conforming key untouched
|
||||
assert set(props) == {"a_b", "a_b_2", "a_b_3"}
|
||||
# deterministic across repeated runs
|
||||
out2 = sanitize_tool_schemas(tools)
|
||||
assert out2[0]["function"]["parameters"]["properties"].keys() == props.keys()
|
||||
|
||||
|
||||
def test_nested_bad_property_keys_renamed():
|
||||
tools = [_tool("t", {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"body": {
|
||||
"type": "object",
|
||||
"properties": {"filter~gte": {"type": "number"}},
|
||||
},
|
||||
},
|
||||
})]
|
||||
out = sanitize_tool_schemas(tools)
|
||||
body = out[0]["function"]["parameters"]["properties"]["body"]
|
||||
assert "filter_gte" in body["properties"]
|
||||
assert "filter~gte" not in body["properties"]
|
||||
|
||||
|
||||
def test_unrename_tool_args_maps_back_to_wire_names():
|
||||
original_params = {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"issue_class~neq": {"type": "string"},
|
||||
"normal": {"type": "string"},
|
||||
"body": {
|
||||
"type": "object",
|
||||
"properties": {"filter~gte": {"type": "number"}},
|
||||
},
|
||||
},
|
||||
}
|
||||
model_args = {
|
||||
"issue_class_neq": "spoofed_dns",
|
||||
"normal": "x",
|
||||
"body": {"filter_gte": 5},
|
||||
}
|
||||
restored = unrename_tool_args(original_params, model_args)
|
||||
assert restored == {
|
||||
"issue_class~neq": "spoofed_dns",
|
||||
"normal": "x",
|
||||
"body": {"filter~gte": 5},
|
||||
}
|
||||
|
||||
|
||||
def test_unrename_passes_unknown_keys_through():
|
||||
params = {"type": "object", "properties": {"a": {"type": "string"}}}
|
||||
assert unrename_tool_args(params, {"a": 1, "mystery": 2}) == {"a": 1, "mystery": 2}
|
||||
|
||||
|
||||
def test_sanitize_property_key_empty_falls_back():
|
||||
assert sanitize_property_key("~~~") == "___"
|
||||
assert sanitize_property_key("") == "param"
|
||||
|
|
|
|||
|
|
@ -38,11 +38,85 @@ from __future__ import annotations
|
|||
|
||||
import copy
|
||||
import logging
|
||||
import re
|
||||
from typing import Any
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# Anthropic (and Bedrock/Vertex/Azure fronting it) reject tool input schemas
|
||||
# whose property keys don't match this pattern. Cloudflare's flat API MCP
|
||||
# ships 61 such keys (query-filter params like ``issue_class~neq`` and
|
||||
# ``meta.<field>[<operator>]``) — one bad key anywhere in the tools array
|
||||
# 400s the entire request.
|
||||
_PROP_KEY_RE = re.compile(r"^[a-zA-Z0-9_.-]{1,64}$")
|
||||
_PROP_KEY_BAD_CHARS = re.compile(r"[^a-zA-Z0-9_.-]")
|
||||
|
||||
|
||||
def sanitize_property_key(key: str) -> str:
|
||||
"""Deterministically map an arbitrary property key to a conforming one."""
|
||||
new = _PROP_KEY_BAD_CHARS.sub("_", key)[:64]
|
||||
return new or "param"
|
||||
|
||||
|
||||
def _rename_property_keys(props: dict, path: str) -> dict[str, str]:
|
||||
"""Return {original_key: conforming_key} for one properties dict.
|
||||
|
||||
Identity entries are omitted. Deterministic: keys are processed in
|
||||
insertion order and collisions deduped with numeric suffixes, so the
|
||||
model-visible schema AND the dispatch-time reverse map (computed
|
||||
independently from the registry's original schema) always agree.
|
||||
"""
|
||||
renames: dict[str, str] = {}
|
||||
taken = {k for k in props if _PROP_KEY_RE.match(k)}
|
||||
for key in props:
|
||||
if _PROP_KEY_RE.match(key):
|
||||
continue
|
||||
base = sanitize_property_key(key)
|
||||
candidate, i = base, 2
|
||||
while candidate in taken:
|
||||
suffix = f"_{i}"
|
||||
candidate = base[: 64 - len(suffix)] + suffix
|
||||
i += 1
|
||||
taken.add(candidate)
|
||||
renames[key] = candidate
|
||||
logger.debug(
|
||||
"schema_sanitizer[%s]: renamed property key %r -> %r "
|
||||
"(provider key-pattern compat)", path, key, candidate,
|
||||
)
|
||||
return renames
|
||||
|
||||
|
||||
def unrename_tool_args(params_schema: Any, args: Any) -> Any:
|
||||
"""Map sanitized property keys in model-emitted args back to wire names.
|
||||
|
||||
``params_schema`` is the ORIGINAL (unsanitized) parameters schema from the
|
||||
registry. Recurses into object-typed values and array items so nested
|
||||
renamed keys are restored too. Unknown keys pass through untouched.
|
||||
"""
|
||||
if not isinstance(params_schema, dict) or not isinstance(args, dict):
|
||||
return args
|
||||
props = params_schema.get("properties")
|
||||
if not isinstance(props, dict):
|
||||
return args
|
||||
reverse = {v: k for k, v in _rename_property_keys(props, "<unrename>").items()}
|
||||
out = {}
|
||||
for key, value in args.items():
|
||||
orig = reverse.get(key, key)
|
||||
subschema = props.get(orig)
|
||||
if isinstance(subschema, dict):
|
||||
if isinstance(value, dict):
|
||||
value = unrename_tool_args(subschema, value)
|
||||
elif isinstance(value, list) and isinstance(subschema.get("items"), dict):
|
||||
value = [
|
||||
unrename_tool_args(subschema["items"], item)
|
||||
if isinstance(item, dict) else item
|
||||
for item in value
|
||||
]
|
||||
out[orig] = value
|
||||
return out
|
||||
|
||||
|
||||
def sanitize_tool_schemas(tools: list[dict]) -> list[dict]:
|
||||
"""Return a copy of ``tools`` with each tool's parameter schema sanitized.
|
||||
|
||||
|
|
@ -268,6 +342,13 @@ def _sanitize_node(node: Any, path: str) -> Any:
|
|||
if not isinstance(node, dict):
|
||||
return node
|
||||
|
||||
# Compute property-key renames up front so the ``required`` branch below
|
||||
# can remap regardless of dict iteration order (``required`` may precede
|
||||
# ``properties`` in the source dict).
|
||||
prop_renames: dict[str, str] = {}
|
||||
if isinstance(node.get("properties"), dict):
|
||||
prop_renames = _rename_property_keys(node["properties"], f"{path}.properties")
|
||||
|
||||
out: dict = {}
|
||||
for key, value in node.items():
|
||||
# JSON Schema ``type`` arrays (e.g. ``["number", "string"]``, common
|
||||
|
|
@ -306,10 +387,12 @@ def _sanitize_node(node: Any, path: str) -> Any:
|
|||
continue
|
||||
|
||||
if key in {"properties", "$defs", "definitions"} and isinstance(value, dict):
|
||||
out[key] = {
|
||||
sub_k: _sanitize_node(sub_v, f"{path}.{key}.{sub_k}")
|
||||
for sub_k, sub_v in value.items()
|
||||
}
|
||||
renames = prop_renames if key == "properties" else {}
|
||||
new_props = {}
|
||||
for sub_k, sub_v in value.items():
|
||||
out_k = renames.get(sub_k, sub_k)
|
||||
new_props[out_k] = _sanitize_node(sub_v, f"{path}.{key}.{out_k}")
|
||||
out[key] = new_props
|
||||
elif key in {"items", "additionalProperties"}:
|
||||
if isinstance(value, bool):
|
||||
# Keep bool ``additionalProperties`` as-is — it's a valid form
|
||||
|
|
@ -330,8 +413,13 @@ def _sanitize_node(node: Any, path: str) -> Any:
|
|||
# - ``examples``: list of example values (any JSON type)
|
||||
# Recursing into these with _sanitize_node() would mis-interpret
|
||||
# literal strings like "path" as bare-string schemas and replace
|
||||
# them with {"type": "object"} dicts. Pass through unchanged.
|
||||
out[key] = copy.deepcopy(value) if isinstance(value, (list, dict)) else value
|
||||
# them with {"type": "object"} dicts. Pass through unchanged
|
||||
# (remapping ``required`` entries through the property renames).
|
||||
if key == "required" and prop_renames and isinstance(value, list):
|
||||
out[key] = [prop_renames.get(r, r) if isinstance(r, str) else r
|
||||
for r in value]
|
||||
else:
|
||||
out[key] = copy.deepcopy(value) if isinstance(value, (list, dict)) else value
|
||||
else:
|
||||
out[key] = _sanitize_node(value, f"{path}.{key}") if isinstance(value, (dict, list)) else value
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue