fix(agent): coordinate truncated tool call argument repair - #56399
konsisumer wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Improves tool call argument repair by distinguishing successful normalization from unrepairable failures. Truncated args now return tool errors instead of empty-object execution.
Looks Good
- ToolCallArgumentsRepair dataclass cleanly separates repair status
- Truncated args now return model-visible errors instead of silent empty execution
- Repairable truncations still get one attempt before fallback
- Comprehensive tests covering repair success, failure, and truncation paths
Reviewed by Hermes Agent
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.
|
Thanks for the careful investigation and focused tests. This automated hermes-sweeper review is closing the PR as not planned under the standing
This is a design-direction decision, not a judgment on the quality of the contribution. Thanks again for the clear root-cause analysis and regression coverage. Automated hermes-sweeper review. Closed as not-planned per standing maintainer policy ( |
What does this PR do?
Coordinates truncated tool-call argument handling around one repair contract: repairable malformed JSON continues normally, while unrepairable truncated JSON is reported back to the model as a tool error instead of executing with
{}or aborting prematurely.Related Issue
Type of Change
Shared root cause
agent/message_sanitization.pyreturned only repaired argument text, so callers could not distinguish a legitimate empty{}from the last-resort{}used after failed repair.agent/conversation_loop.pyhad a separate invalid-JSON truncation branch that rejected router-mislabeled truncated tool calls before trying the shared repair routine.How this fixes each issue
Changes Made
repair_tool_call_arguments_with_status()so callers can tell whether repair succeeded even when the returned JSON is{}.How to Test
HERMES_HOME=/private/tmp/hermes-test-home /opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/run_agent/test_repair_tool_call_arguments.py tests/run_agent/test_tool_call_args_sanitizer.py tests/run_agent/test_run_agent.py -q -x --timeout=60'BASE=$(git merge-base origin/main HEAD); { git diff --name-only --diff-filter=d "$BASE"; git ls-files --others --exclude-standard; } | grep -E '\.pyi?$' | sort -u | xargs ruff checkpython scripts/check-windows-footguns.py run_agent.py agent/message_sanitization.py agent/agent_runtime_helpers.py agent/chat_completion_helpers.py agent/conversation_loop.py tests/run_agent/test_repair_tool_call_arguments.py tests/run_agent/test_tool_call_args_sanitizer.py tests/run_agent/test_run_agent.py/opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' sh; it aborted during collection becausefastapi/uvicornare not installed and this Homebrew Python is externally managed, so the lazy dependency installer cannot install them.What platforms tested on
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass; broad collection is blocked locally by missing dashboard dependencies as noted aboveDocumentation & Housekeeping
cli-config.yaml.exampleupdate N/ACONTRIBUTING.md/AGENTS.mdupdate N/AScreenshots / Logs
run_agent.py:107# noqadirective format)Refs #35151
Refs #35574