Skip to content

fix: repair broken tool_call arguments at the recovery write site - #16505

Open
guigui0711 wants to merge 1 commit into
NousResearch:mainfrom
guigui0711:fix/sanitize-recovery-tool-calls-at-write-site
Open

fix: repair broken tool_call arguments at the recovery write site#16505
guigui0711 wants to merge 1 commit into
NousResearch:mainfrom
guigui0711:fix/sanitize-recovery-tool-calls-at-write-site

Conversation

@guigui0711

Copy link
Copy Markdown

Summary

Follow-up micro-improvement to #15348 (7a192b12), as suggested by @teknium1 in #14518:

Your proposed approach of sanitizing at the write site is slightly cleaner as defense-in-depth. If you'd like to land that as a follow-up micro-improvement on top of the existing fix, a new PR narrowly scoped to the recovery append site at line 11711 would be a good candidate.

The loop-top _sanitize_tool_call_arguments() pass (added in #15348) repairs corrupted entries on the next iteration, but the recovery append site still writes broken JSON into messages[] first. This creates a one-iteration window where corrupted data exists in the list and can be persisted to session state.

This PR repairs arguments at the write site using the existing _repair_tool_call_arguments() helper, so corrupted JSON never enters the message history at all.

Changes

  • 1 file changed, 11 insertions, 1 deletion
  • Narrowly scoped to the recovery append site at run_agent.py:11911

Ref: #4662, #14518

cc @alt-glitch

🤖 Generated with Claude Code

Follow-up to NousResearch#15348 / 7a192b1. The loop-top _sanitize_tool_call_arguments
pass repairs corrupted entries on the *next* iteration, but the recovery
append at line 11911 still writes broken JSON into messages[] first.

Repair at the write site so corrupted arguments never enter the persistent
message history, eliminating the one-iteration window where broken data
exists in the list.

Ref: NousResearch#4662, NousResearch#14518 (prior PR, closed as covered by NousResearch#15348)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@guigui0711

Copy link
Copy Markdown
Author

Hi @teknium1 @alt-glitch — this is the follow-up micro-improvement suggested in #14518, narrowly scoped to the recovery append site. Would appreciate a review when you have a chance 🙏

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Apr 27, 2026

@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 narrowly scoped defense-in-depth follow-up. The underlying write-time gap still exists on current main: agent/conversation_loop.py:4519-4522 detects invalid arguments, and agent/conversation_loop.py:4571-4572 appends the built assistant message before recovery tool results are added.

Problems

  • The live loop no longer resides in run_agent.py. Commit 053025238434cfbf121873977b39888d7f27d1c1 extracted it to agent/conversation_loop.py; this PR's run_agent.py hunk will not affect the active recovery path.
  • The diff adds no regression coverage for the third-invalid-response recovery branch.

Suggested changes

  • Apply the same repair to recovery_assistant at agent/conversation_loop.py:4571, before messages.append() at line 4572.
  • Add an end-to-end loop test that reaches this branch and asserts the recovery assistant's persisted/replayed tool_calls[*].function.arguments are valid JSON while tool-call/result pairing remains intact.

Automated hermes-sweeper review.

Comment thread run_agent.py

# Append the assistant message with its (broken) tool_calls
# Append the assistant message, repairing broken tool_call
# arguments at write time so corrupted JSON never enters

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.

Current main extracted the live run_conversation loop into agent/conversation_loop.py; the active recovery branch is now at agent/conversation_loop.py:4571 (commit 053025238). Salvage this write-site repair there, before the assistant message is appended.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 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 P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants