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

[hashbender]

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 uniformly, including the properties map — whose keys are user-facing parameter names, not 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, stamping a "type": "object" and an empty "properties" onto the map. Those surface to the model as parameters type / 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"]	nested → ["required", "type", "properties"]
["query"] (normal)	["query"] ✓ (unaffected)

The model can then fill those phantom params with junk arguments, and strict providers can 400 the malformed intermediate schema (a "type": "object" string value sitting inside the properties map) — 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 / patternProperties during descent (see its docstring and test_definitions_as_property_name_is_preserved), but the sibling _repair_object_shape was overlooked. Classic "fixed one traversal, missed the other."

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 / 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. Proof the fix is load-bearing: revert the tools/mcp_tool.py hunk and rerun -k property_named → the 3 new tests fail (phantom type / properties appear in the output properties map); restore → they pass.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(mcp):)
• I searched for existing PRs to make sure this isn't a duplicate (open PRs Fix MCP schema normalizer mangling parameter named 'definitions' into '$defs' NousResearch/hermes-agent#36955 / Keep definition-member names verbatim when promoting definitions to $defs NousResearch/hermes-agent#55082 fix the analogous definitions→$defs rename in _rewrite_local_refs; none touch _repair_object_shape)
• My PR contains only changes related to this fix
• I've run the test suite and all tests pass
• I've added tests for my changes
• I've tested on my platform: macOS 15

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

---

Mirror-of: NousResearch#56620
NousResearch#56620

[tenki-reviewer]

Review Complete

Files Reviewed: 2
Findings: 1

By Severity:

• 🟢 Low: 1

---

One low-severity finding about misleading comments in mcp_tool.py that falsely claim start_new_session=True exists in LSP and slash_worker spawn paths.

Files Reviewed (2 files)

tests/tools/test_mcp_tool.py
tools/mcp_tool.py

[tenki-reviewer]

Risk: 🟢 Low (18/100) — 1 low finding · 293 LOC across 2 files

---

PR #515 Review Summary

Scope: 2 files changed — and — adding PID filtering for non-MCP gateway child processes to the stdio transport cleanup sweep.

Risk score: 18 — the one finding is a comment-only issue with no functional impact; the core defense-in-depth PID filtering logic is sound.

Findings

• finding-001 (low/90): Misleading comments in claim that and now use , but neither file actually does. The mechanism works regardless, but the comments misdirect future maintainers.

Not flagged (below confidence threshold)

• Two additional verified findings (imprecise substring argv matching at confidence 75, missing gate at confidence 78) fell below the threshold and were excluded.

Assessment: Comment

No functional defects found. The PR correctly implements PID filtering for gateway child processes. The flagged issue is strictly about comment accuracy.

[tenki-reviewer]

🟢 Misleading cross-file comments claim start_new_session fix exists where it does not (bug)

tools/mcp_tool.py line 3017-3018 says 'LSP servers and slash_worker now use start_new_session=True too' and line 1875 says 'See agent/lsp/client.py for the complementary start_new_session fix.' Both comments were introduced in this PR. However, agent/lsp/client.py line 266 spawns LSP processes via asyncio.create_subprocess_exec without start_new_session=True, and tui_gateway/server.py line 287 spawns the slash_worker via subprocess.Popen without start_new_session=True. The comments are factually incorrect about the state of those files. The _filter_mcp_children defense-in-depth logic is not functionally broken by this — it works regardless — but the comments misdirect future maintainers who may rely on the claimed fix existing when it does not.

💡 Suggestion: Either add start_new_session=True to agent/lsp/client.py and tui_gateway/server.py spawn calls (matching the comment's claim), or update both comments in mcp_tool.py to accurately reflect the current state. Additionally, the comment at line 1875 should not claim a complementary fix exists in lsp/client.py when it doesn't.

Suggested change

	# during stdio MCP server spawn. LSP servers and slash_worker now use
	# start_new_session=True too; this remains defense-in-depth for any future
	# during stdio MCP server spawn. LSP servers and slash_worker are spawned
	# without start_new_session (see agent/lsp/client.py, tui_gateway/server.py);

📋 Prompt for AI Agents

In tools/mcp_tool.py around line 3017, the comment says 'LSP servers and slash_worker now use start_new_session=True too' but neither agent/lsp/client.py nor tui_gateway/server.py actually use start_new_session=True. Fix the comment to accurately state they are spawned WITHOUT start_new_session. Additionally, at line 1875, the comment 'See agent/lsp/client.py for the complementary start_new_session fix' is misleading since no such fix exists — either remove this reference or change it to a TODO.