Skip to content

Fix MCP schemas for dynamically dispatched Hermes tools - #52493

Closed
Juddd wants to merge 1 commit into
NousResearch:mainfrom
Juddd:fix-mcp-dynamic-tool-schemas
Closed

Fix MCP schemas for dynamically dispatched Hermes tools#52493
Juddd wants to merge 1 commit into
NousResearch:mainfrom
Juddd:fix-mcp-dynamic-tool-schemas

Conversation

@Juddd

@Juddd Juddd commented Jun 25, 2026

Copy link
Copy Markdown

Summary

  • override FastMCP's reflected **kwargs schema with each Hermes tool's authoritative parameters schema
  • route dynamic MCP tool calls through the raw argument dict so structured arguments that match the advertised schema reach handle_function_call
  • add regression coverage for schema replacement and structured argument passthrough

Why

The hermes-tools MCP server registers dynamic trampoline functions (**kwargs) for Hermes tools. FastMCP reflects that signature as a kwargs parameter, so MCP clients see schemas like {"kwargs": "string"} instead of the real tool inputs such as image_url and question for vision_analyze. This makes standards-compliant MCP clients generate the wrong tool arguments even though Hermes already has the correct schema in its tool registry.

Testing

  • uv run --extra dev python -m pytest tests/agent/transports/test_hermes_tools_mcp_server.py -q
  • manual FastMCP build/call smoke test confirming vision_analyze exposes image_url/question and forwards structured args to handle_function_call

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/mcp MCP client and OAuth P3 Low — cosmetic, nice to have labels Jun 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #49800 (same root cause: FastMCP reflects the **kwargs trampoline as a single kwargs schema). Different mechanism though: #49800 unwraps the {"kwargs": {...}} envelope at dispatch time, while this PR replaces the exposed schema with Hermes' authoritative parameters (so MCP clients generate correct args in the first place) and routes structured args through a raw dict. This PR is the more complete fix; maintainer should pick one. Also related: #46109, #31279.

@John-Lussier

John-Lussier commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

We independently reproduced this exact defect from recent Codex/gpt-5.6-sol sessions: FastMCP list_tools() advertised real tools such as web_extract as { "properties": { "kwargs": {} }, "required": ["kwargs"] }. Calls matching that advertised schema lost the real fields; calls matching Hermes’ authoritative schema failed before dispatch.

I opened #62762 before this older PR surfaced in the phrase search. I agree #52493 is the canonical earlier report for the core schema replacement. One concrete gap to preserve when landing a canonical fix: this patch replaces call_fn_with_arg_validation with raw pass-through, so the newly advertised required/enum/nested/additionalProperties constraints are not enforced server-side. #62762 adds protocol-level negative tests proving invalid calls never dispatch, plus full Hermes structured-error → MCP error semantics and a fail-closed FastMCP 1.26 compatibility boundary.

Happy for maintainers to choose #52493 as canonical and fold those validation/error tests in; I can close #62762 once the complete behavior is represented.

@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 addressing the dynamic-tool schema defect. The premise is confirmed on current upstream main: agent/transports/hermes_tools_mcp_server.py:154-185 reads Hermes' params_schema but registers only a **kwargs callable, so the schema is not used by FastMCP.

Problems

  • agent/transports/hermes_tools_mcp_server.py:152-156 replaces call_fn_with_arg_validation with direct fn(**kwargs) execution. This leaves the advertised Hermes schema unenforced before dispatch; current model_tools.py:1065-1068 performs coercion, not required/enum/nested/additional-properties validation. The July 11 discussion correctly calls out this protocol-contract gap.
  • agent/transports/hermes_tools_mcp_server.py:236 ignores a failed private-schema installation. The server can then continue with FastMCP's reflected kwargs contract. The new test uses SimpleNamespace (tests/agent/transports/test_hermes_tools_mcp_server.py:120-156), so it does not prove compatibility with the pinned FastMCP implementation.

Suggested changes

  • Preserve exact server-side schema validation and add real FastMCP negative tests showing invalid calls never dispatch.
  • Fail closed if the required FastMCP internals cannot be replaced, and exercise a real tools/list plus structured-call round trip.

Automated hermes-sweeper review.


object.__setattr__(
fn_metadata,
"call_fn_with_arg_validation",

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.

This replaces FastMCP's validator with raw dispatch, so the newly advertised required, enum, nested, and additionalProperties constraints are never enforced before a Hermes tool runs. Please preserve equivalent validation and add protocol tests proving invalid calls do not reach handle_function_call.

handler = _make_handler(name)
handler = mcp.tool(name=name, description=description)(handler)

_install_tool_schema_override(mcp, name, params_schema)

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 handle a false result here. _install_tool_schema_override() can fail when FastMCP's private shape differs; ignoring that failure silently restores the broken reflected-kwargs schema instead of surfacing an incompatible server.

@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
@Juddd Juddd closed this by deleting the head repository Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have 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.

4 participants