feat(tools): validate required params before dispatch - #61550
feat(tools): validate required params before dispatch#61550John-Lussier wants to merge 2 commits into
Conversation
After coerce_tool_args normalizes types, add a lightweight check that all JSON Schema 'required' parameters are actually present in the arguments dict before dispatching to the tool handler. Currently, when a model omits a required param (e.g. calling terminal without 'command'), the call reaches the handler which either crashes with a KeyError, silently uses a None default, or produces a confusing partial result. This surfaces an actionable error to the model instead: 'Missing required parameter(s): X. Please retry with all required parameters.' The check is schema-driven (reads the tool's registered 'required' array from the registry) and runs after coercion so type-mismatched values are already normalized. Returns early for tools with no schema, no required array, or unknown tools — zero overhead for unaffected paths. 10 unit tests covering: all-present, one-missing, multi-missing, no-required-array, empty-required, unknown-tool, non-dict-args, empty-dict, extra-args, null-value-present.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real dispatch gap: current model_tools.py:1065-1068 has no generic check of schema required fields before registry execution.
Problems
- The new check at
model_tools.py:1103runs before tool-request middleware. That middleware is explicitly allowed to replace effective arguments before execution (hermes_cli/middleware.py:125-155; current dispatcher applies it atmodel_tools.py:1146-1162). A middleware that supplies a required field will be rejected too early; later execution middleware can also alter the arguments passed toregistry.dispatch(). - The added tests call only
validate_required_params()directly. They do not coverhandle_function_call()or either middleware boundary.
Suggested changes
- Validate the final arguments immediately before
registry.dispatch()and add integration tests for request middleware adding a required field and execution middleware removing one. - Clarify whether agent-level dispatches are in scope:
agent/tool_executor.py:1215-1248bypasseshandle_function_call()even thoughtools/memory_tool.py:1127has a root required field.
Automated hermes-sweeper review.
| # coerce_tool_args handles *type* mismatches; this catches genuinely | ||
| # *missing* required params that would otherwise surface as confusing | ||
| # KeyError or None-default behaviour deep inside the tool handler. | ||
| _missing = validate_required_params(function_name, function_args) |
There was a problem hiding this comment.
Blocking: this validates before tool_request middleware, but that middleware may replace effective arguments before execution. Validate at the final dispatch boundary instead, so middleware can supply required fields and later execution middleware cannot remove them undetected.
There was a problem hiding this comment.
Fixed in e039765. Required-param validation now runs at the final execution boundary immediately before registry dispatch, after tool-request middleware and inside the tool-execution middleware chain. Request middleware can supply a required field; execution middleware removing one now returns the actionable validation error without calling the handler. I also included agent-level tools in scope for both sequential and concurrent paths (including memory) and added integration coverage for all four boundaries. Verification: 155 focused tests passed; git diff --check passed.
Structured stats collection for the existing tool-call repair pipeline. Records RepairEvent (pattern, tool, model, timestamp) at each repair pass in message_sanitization.py and model_tools.py coerce_tool_args. New module: agent/tool_repair_stats.py - RepairPattern enum (20 known failure patterns) - ToolRepairStats singleton: thread-safe, ring-buffer (10k events) - record_repair() convenience function - summary() for CLI display Hooks added (1-2 lines each, zero-overhead when unused): - message_sanitization.py: 6 hooks in _repair_tool_call_arguments (empty_args, none_literal, control_char_escape, trailing_comma, unrepairable) - model_tools.py: 2 hooks in coerce_tool_args (bare_string_wrap, bare_object_wrap) Design constraints: - No new model tools (zero API cost impact) - No prompt caching impact - No new config keys - Import failure → no-op (never breaks repair pipeline) - Thread-safe with threading.Lock - Bounded memory (ring buffer caps at 10k events) Tests: 19 new tests (stats, thread-safety, ring-buffer, resilience) Regression: 82 existing repair/coercion tests still pass Complementary to existing repair PRs (NousResearch#62578, NousResearch#56399, NousResearch#61550, NousResearch#59267, NousResearch#52747, NousResearch#55620, NousResearch#56557, NousResearch#21696) — adds observability, not repairs.
|
Review feedback is addressed in the current head
Tests: |
What
Adds schema-driven required-parameter validation to the tool dispatch pipeline. When a model omits a required parameter from its tool call, the agent now receives an actionable error:
"Missing required parameter(s): X. Please retry with all required parameters."instead of a confusingKeyError, silentNonedefault, or partial result deep inside the tool handler.Why
AIRTBench (Dreadnode, arXiv:2506.14682) found that ~21.7% of all agent execution failures are syntax/format errors — the single largest category. Failed runs cost 10× more than successful runs ($8.91 vs $0.89).
Hermes already has excellent type-level repair (trailing commas, Python
None, unclosed braces via_repair_tool_call_arguments; string→int/bool/array coercion viacoerce_tool_args; fuzzy tool-name matching viarepair_tool_call). But there's no presence-level check — the schema'srequiredarray is never consulted before dispatch.This fills that gap with a 15-line function (
validate_required_params) that reads the tool's registeredrequiredarray and returns any missing param names. It runs aftercoerce_tool_argsand before the tool handler.Where
Design notes
[]immediately for tools with no schema, norequiredarray, or unknown tools.coerce_tool_argshas already fixed type mismatches (string→int, etc.) before this check runs.Nonecounts as "present" — this is presence validation, not nullability validation.{"error": "..."}JSON string, same as other pre-dispatch rejections.ToolRegistryin tests to avoid polluting the global registry.Verification