Skip to content

fix(agent): close unclosed JSON delimiters in LIFO order when repairing truncated tool_call arguments - #42510

Closed
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/repair-tool-call-args-lifo-close
Closed

fix(agent): close unclosed JSON delimiters in LIFO order when repairing truncated tool_call arguments#42510
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/repair-tool-call-args-lifo-close

Conversation

@Frowtek

@Frowtek Frowtek commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

_repair_tool_call_arguments salvages malformed tool-call JSON from
local/quantized models. When closing unclosed structures it appended all
missing } first, then all missing ] — a fixed order that is wrong for
nested payloads. {"items": [1, 2 became {"items": [1, 2}] (invalid)
instead of {"items": [1, 2]}, every later repair pass then failed, and the
whole argument payload was silently dropped to {}. The tool then ran with
empty arguments.

The fix walks the text tracking string state (so braces/brackets inside
string values are ignored) and closes open delimiters in LIFO nesting order,
so nested truncations recover correctly. The flat-object case keeps working.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/message_sanitization.py — replace the count-and-append delimiter
    close in _repair_tool_call_arguments with a string-aware, stack-based
    LIFO close. Trailing-comma strip and the excess-closer pass are unchanged.
  • tests/run_agent/test_repair_tool_call_arguments.py — turn the misleading
    test_unclosed_bracket_and_brace (asserted only "valid JSON", so it passed
    on the dropped {}) into a real value assertion, plus 5 regression tests.

How to Test

pytest tests/run_agent/test_repair_tool_call_arguments.py -q
pytest tests/run_agent/test_streaming_tool_call_repair.py -q

Results:

  • test_repair_tool_call_arguments.py — 26 passed
  • test_streaming_tool_call_repair.py (uses the same repair path) — 12 passed

Before the fix, {"items": [1, 2{} (args lost); after, it recovers to
{"items": [1, 2]}.

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • All tests pass
  • I've added tests for my changes
  • I've updated relevant documentation — N/A (internal helper)

…ng truncated tool_call arguments

When _repair_tool_call_arguments closed unclosed structures it appended all missing '}' then all missing ']', a fixed order that is wrong for nested payloads: '{"items": [1, 2' became '{"items": [1, 2}]' (invalid) instead of '{"items": [1, 2]}'. Every later repair pass then failed and the whole argument payload was dropped to '{}', so a truncated tool call ran with empty arguments.

Walk the text tracking string state and close open delimiters in LIFO order so nested truncations recover correctly; braces/brackets inside string values are ignored.

Strengthen the previously-misleading test_unclosed_bracket_and_brace into a real value assertion and add regression tests for nested arrays/objects, the flat-object case, and a brace-inside-string case.
@Frowtek Frowtek closed this Jun 9, 2026
@Frowtek
Frowtek deleted the fix/repair-tool-call-args-lifo-close branch June 9, 2026 01:49
@Frowtek
Frowtek restored the fix/repair-tool-call-args-lifo-close branch June 9, 2026 01:51
@Frowtek Frowtek reopened this Jun 9, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Positive verification — LIFO JSON delimiter repair is correct and well-tested.

Reviewed the full diff (agent/message_sanitization.py + tests).

The replacement of the naive count-and-append approach ('}' * n then ']' * m) with a proper stack-based LIFO closer is correct. The key improvements:

  • String-awareness: The in_string / escaped state machine correctly ignores braces/brackets inside string values, so {"msg": "a { b" doesn't count the { as an open delimiter.
  • LIFO ordering: reversed(stack) produces closers in nesting order — {"k": [1, 2]} (valid) instead of }] (invalid).
  • No regression on flat cases: Simple {"a": 1, "b": 2 still closes correctly.

Test coverage is thorough: unclosed bracket+brace, array of strings, nested write_file args, object inside array, flat object, and brace-in-string-value. All 6 new test cases exercise distinct edge paths.

No issues found.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Correctness

  • Critical bug fix: the old count-based approach closed delimiters in wrong order (} then ]), which would corrupt nested JSON like {"k": [1, 2 -> invalid {"k": [1, 2]}.
  • The new LIFO stack-based approach correctly closes in reverse order of opening.
  • Properly handles string literals (ignores braces/brackets inside quoted strings).

Code Quality

  • Clean implementation using a stack with proper string/escape state tracking.
  • Comments clearly explain the edge case being addressed.

Testing

  • Excellent test coverage with 6 new test cases covering:
    • Unclosed bracket and brace (the original bug)
    • Unclosed array of strings
    • Unclosed nested write_file args
    • Object inside array closes LIFO
    • Flat object regression guard
    • Brace inside string value not treated as delimiter

Looks Good

  • Important bug fix with comprehensive test coverage. The fix is minimal and focused.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused investigation and regression coverage. The nested-delimiter case is real in the existing legacy repair path, but this PR takes a repair direction we do not accept.

  • Automated hermes-sweeper review: agent/message_sanitization.py:227-233 currently contains the legacy delimiter-repair logic, and this PR adds a new stack-based reconstruction pass for malformed/truncated model output.
  • This matches the standing model-output-repair policy: Hermes does not add new content-reconstruction passes for malformed tool-call JSON, including delimiter/boundary reconstruction.
  • Please keep model output valid upstream or focus a follow-up on a verified wire-transport defect rather than reconstructing malformed model arguments.

Closed as not-planned per standing maintainer policy (model-output-repair). This is a design-direction decision, not a code-quality judgment — see the Contribution Rubric in AGENTS.md for what the project is looking for. If you believe this policy was misapplied to your change, comment here and a maintainer will take a look.

@teknium1 teknium1 closed this Jul 14, 2026
@teknium1 teknium1 added the sweeper:not-planned Sweeper: closed per standing maintainer policy (design direction) label Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sweeper:not-planned Sweeper: closed per standing maintainer policy (design direction)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants