Skip to content

fix(mcp): don't inject phantom params for tools named required/properties - #56620

Open
golldyck wants to merge 1 commit into
NousResearch:mainfrom
golldyck:fix/mcp-schema-repair-property-names
Open

fix(mcp): don't inject phantom params for tools named required/properties#56620
golldyck wants to merge 1 commit into
NousResearch:mainfrom
golldyck:fix/mcp-schema-repair-property-names

Conversation

@golldyck

@golldyck golldyck commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

_normalize_mcp_input_schema (via its inner _repair_object_shape) can inject phantom parameters into an MCP tool's schema when the tool declares a parameter literally named required or properties.

_repair_object_shape recurses into every dict the same way, including the properties map, whose keys are user-facing parameter names rather than JSON Schema meta-keywords. The object-shape repair right below it (fill a missing type, ensure a properties dict exists) then fires on the properties map itself whenever it contains a key named required or properties. That stamps a "type": "object" and an empty "properties" onto the map. Both then surface to the model as parameters type and properties that the MCP server never declared.

Concrete reproduction (running the actual function):

input properties keys output properties keys (before)
["required"] ["required", "type", "properties"]
["properties"] ["properties", "type"]
nested config.properties = ["required"] ["required", "type", "properties"] (nested)
["query"] (normal) ["query"] (unaffected)

The model can then fill those phantom params with junk arguments. Strict providers can also 400 the malformed intermediate schema, because a "type": "object" string value ends up inside the properties map. That is the same failure mode the sibling _rewrite_local_refs docstring already warns about.

Root cause: _rewrite_local_refs was deliberately fixed to special-case properties and patternProperties during descent (see its docstring and test_definitions_as_property_name_is_preserved), but the sibling _repair_object_shape was overlooked. One traversal got fixed and the other was missed.

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/mcp_tool.py: in _repair_object_shape, gate properties and patternProperties during descent so their keys are treated as user-facing parameter names (iterate the map, recurse only into each property's schema), mirroring the existing gate in _rewrite_local_refs. All legitimate repairs (type inference, required pruning, nested object coercion) are unchanged.
  • tests/tools/test_mcp_tool.py: 3 regression tests (test_property_named_required_does_not_inject_phantom_params, test_property_named_properties_does_not_inject_phantom_type, test_property_named_required_is_preserved_when_nested).

How to Test

  1. scripts/run_tests.sh tests/tools/test_mcp_tool.py -q: 206 tests pass.
  2. To confirm the fix is load-bearing, revert the tools/mcp_tool.py hunk and rerun -k property_named. The 3 new tests fail, with phantom type and properties appearing in the output properties map. Restore the hunk and they pass.

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings): the new gate is documented inline
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md / AGENTS.md: N/A
  • Cross-platform impact: N/A (pure schema logic; scripts/check-windows-footguns.py clean)
  • Tool descriptions/schemas: N/A

…ties

_normalize_mcp_input_schema's _repair_object_shape recursed into the
properties map the same way it recurses into a schema node. When an MCP
tool declared a parameter literally named "required" or "properties",
the object-shape repair (fill missing type, add empty properties) fired
on the properties map itself and stamped a "type": "object" plus an
empty "properties" onto it. Those surfaced to the model as phantom
parameters the server never declared, which the model could fill with
junk args (and strict providers can 400 the malformed intermediate).

Gate properties/patternProperties during descent so their keys are
treated as user-facing parameter names, not JSON Schema meta-keywords —
mirroring the existing gate in the sibling _rewrite_local_refs. All
legitimate object-shape repairs (type inference, required pruning,
nested coercion) are unchanged.

Adds regression tests in tests/tools/test_mcp_tool.py.
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists labels Jul 1, 2026
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
Telegram API returns HTTP 400 when sent whitespace-only or empty
text. Add a guard at the top of send() to silently succeed on
blank content instead of crashing.

Equivalent to OpenClaw NousResearch#56620.
liuchanchen pushed a commit to liuchanchen/hermes-agent that referenced this pull request Jul 3, 2026
Telegram API returns HTTP 400 when sent whitespace-only or empty
text. Add a guard at the top of send() to silently succeed on
blank content instead of crashing.

Equivalent to OpenClaw NousResearch#56620.
liuchanchen pushed a commit to liuchanchen/hermes-agent that referenced this pull request Jul 3, 2026
Telegram API returns HTTP 400 when sent whitespace-only or empty
text. Add a guard at the top of send() to silently succeed on
blank content instead of crashing.

Equivalent to OpenClaw NousResearch#56620.
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
Telegram API returns HTTP 400 when sent whitespace-only or empty
text. Add a guard at the top of send() to silently succeed on
blank content instead of crashing.

Equivalent to OpenClaw NousResearch#56620.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused regression coverage. The property-map premise is present on current main: tools/mcp_tool.py:4617-4633 recursively repairs a properties map and can inject type/properties when that map contains those names.

Problems

  • The new guard only covers properties and patternProperties. Legacy definitions is rewritten to $defs before repair (tools/mcp_tool.py:4587, tools/mcp_tool.py:4648-4650), but $defs still reaches the generic recursive branch. A $defs entry named required or properties will be corrupted by the same object-shape logic. The shared sanitizer explicitly treats $defs as a name-to-schema map (tools/schema_sanitizer.py:308-312).

Suggested changes

  • Include $defs in the map-aware traversal and add a regression for a definition with a reserved name. The existing property-map tests should remain.

Automated hermes-sweeper review.

Comment thread tools/mcp_tool.py
repaired[k] = {
prop_name: _repair_object_shape(prop_schema)
for prop_name, prop_schema in v.items()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extend this schema-map branch to $defs: _rewrite_local_refs has already converted legacy definitions to $defs before this repair runs, so a definition named required or properties still reaches the generic recursion and receives the same phantom entries.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
Telegram API returns HTTP 400 when sent whitespace-only or empty
text. Add a guard at the top of send() to silently succeed on
blank content instead of crashing.

Equivalent to OpenClaw NousResearch#56620.
jh1nresh pushed a commit to jh1nresh/hermes-agent that referenced this pull request Aug 25, 2026
Telegram API returns HTTP 400 when sent whitespace-only or empty
text. Add a guard at the top of send() to silently succeed on
blank content instead of crashing.

Equivalent to OpenClaw NousResearch#56620.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants