fix(sanitization): close truncated tool-call args by nesting, not by count

[mohamedelrefaiy]

What

_repair_tool_call_arguments decided where JSON structures open and close by counting delimiters. Counting cannot see nesting order, and counts every { or ] inside a string value as structure. Both make the repair emit invalid JSON for payloads it could otherwise recover, which then degrade to the "{}" last resort — dropping the model's arguments entirely.

_repair_tool_call_arguments('{"items": [1, 2, 3')
# before: '{"items": [1, 2, 3}]'   -> still invalid -> "{}"
#  after: '{"items": [1, 2, 3]}'

All three passes that reasoned about delimiters now share one string- and escape-aware scan:

pass	was	now
strip trailing commas	re.sub(r',\s*([}\]])', ...)	string-aware scan
close unclosed structures	str.count deficit, all } then all ]	LIFO stack, innermost first
trim excess closers	str.count deficit	string-aware, trailing token only

Two ways the function could break its documented never-raises contract are also fixed. Since CPython 3.11, json.loads raises a bare ValueError on an integer literal past sys.get_int_max_str_digits(); two of four guards caught only JSONDecodeError, and — as review correctly pointed out — the two callers pre-validate with json.loads before ever reaching the helper, so guarding the helper alone was not enough. Both call sites now catch it. json.loads also recurses per nesting level, so "[" * 100000 (a repetition-loop payload from exactly the local models this function serves) raised RecursionError straight out; that is now contained.

What this deliberately refuses to repair

Repaired arguments may be executed, so anything that would invent a value fails closed to "{}" and routes to the existing truncation path instead.

• Truncation inside a string value. {"path":"x.txt","content":"hel — closing the quote yields well-formed JSON carrying a silently incomplete value. The existing test asserting this (test_unrepairable_partial_returns_empty_object) and TestStreamingApiCall::test_truncated_tool_call_args_no_finish_reason_routes_to_stub are unchanged.
• Truncation immediately after a comma. {"a": [1, 2, — the distinction is whether the last value is complete. [1, 2 has a complete final value and only needs its container closed. [1, 2, does not: the comma is the model promising a next value that never arrived, so completing it would present a short list as a whole one. This is narrower than the previous behaviour on that input, and intentionally so.
• An excess closer that is not the last token. {"a": [1]]} — trimming works from the tail and would remove a legitimate closer. Unchanged from before.
• JSON-illegal characters posing as whitespace. JSON permits only space, tab, LF and CR between tokens (RFC 8259 §2). str.isspace(), bare str.rstrip() and a regex \s on a str pattern all additionally match U+001C–U+001F, U+0085, U+00A0 and U+2028. Treating those as skippable deleted them and returned a payload that parses, so {"command":"echo X",\x1c} became an executable call. All whitespace tests in the repair path now use an explicit _JSON_WHITESPACE.

Behaviour

input	before	after
{"items": [1, 2, 3	{}	{"items": [1, 2, 3]}
{"edits": [{"line": 1	{}	{"edits": [{"line": 1}]}
{"a": {"b": [{"c": [1, 2	{}	{"a": {"b": [{"c": [1, 2]}]}}
{"sep": ", ]", "files": ["a"	{}	{"sep": ", ]", "files": ["a"]}
{"s":"{","x":[1]}}	{}	{"s":"{","x":[1]}
{"n": + 9×5000	raises ValueError	{}
"[" × 100000	raises RecursionError	{}
{"command":"echo X",\x1c}	executed	{}
{"a": [1, 2,	{"a": [1, 2]}	{}
{"path":"x.txt","content":"hel	{}	{}
balanced input	unchanged	unchanged

Tests

17 cases added to tests/run_agent/test_repair_tool_call_arguments.py and 3 to tests/run_agent/test_run_agent.py covering the caller boundaries. Each fails without its corresponding change. tests/run_agent: 2131 passed, 2 failed — both test_provider_parity failures reproduce on an unmodified checkout of this commit's parent. ruff clean.

Related

Refs #35151, #62948. Those report the symptom: arguments replaced with {}. The mitigation on main — treating a {} result as truncated rather than forwarding it — stopped the silent execution but left the repair itself wrong, so recoverable payloads still fail. This fixes the repair underneath that mitigation; the two compose. Note agent/conversation_loop.py:1107 assigns the repair result directly with no {} check, unlike the chat_completion_helpers.py site; that path is history normalisation rather than execution, so it is not addressed here.

Non-finite numbers (1e999, NaN, Infinity) still reach tools through pass 3's plain json.loads and tool_executor._parse_tool_arguments, and lone surrogate escapes still pass through a module that carries _sanitize_surrogates for that hazard. Both predate this change and need a policy applied at every parse boundary rather than inside the repair passes; happy to follow up with those separately.

[Copilot]

Pull request overview

This PR improves Hermes’s tool-call argument sanitization by making JSON “closing” repairs nesting-aware (rather than delimiter-count-based) and by ensuring _repair_tool_call_arguments never raises when json.loads triggers a ValueError (e.g., from overly long numeric literals). This lives in the agent’s message/tool-call repair path, so it directly reduces cases where otherwise-repairable tool arguments degrade to the "{}" last resort.

Changes:

• Replace count-based “close unclosed JSON” logic with a string/escape-aware stack that appends closers in correct nesting order and ignores delimiters inside string values.
• Widen two json.loads(...) exception handlers to catch (json.JSONDecodeError, ValueError) so the sanitizer preserves its “never-raises” contract under Python’s max-int-digit guard.
• Add targeted regression tests covering nesting-aware closing, delimiters-in-strings, dangling commas before appended closers, and long-digit-run ValueError behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
agent/message_sanitization.py	Introduces _close_unclosed_json_structures() and uses it in _repair_tool_call_arguments; widens exception handling to include ValueError for json.loads.
tests/run_agent/test_repair_tool_call_arguments.py	Adds new test cases validating correct nesting closure, delimiter handling inside strings, intentional non-repair when truncated mid-string, and “never-raises” behavior for long numeric literals.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[teknium1]

Thanks for the focused repair and regression coverage. The delimiter-count premise is present on current main at agent/message_sanitization.py:228-233, and the nesting-aware replacement is appropriately scoped.

Problems

• The ValueError fix is bypassed in streaming: agent/chat_completion_helpers.py:3373-3375 catches only JSONDecodeError before calling _repair_tool_call_arguments. A >4300-digit literal therefore raises before this PR's helper guards run.
• The executor's fail-closed parser has the same gap at agent/tool_executor.py:102-107; it catches JSONDecodeError and TypeError, not ValueError.

Suggested changes

• Catch ValueError at both parsing boundaries and add an actual streaming-path regression test, rather than testing only the helper. The existing real-path truncated-call test is at tests/run_agent/test_run_agent.py:8294-8313.

Automated hermes-sweeper review.

[teknium1]

This makes the helper safe when called, but streaming parses first at agent/chat_completion_helpers.py:3373-3375 and catches only JSONDecodeError; a long numeric literal still raises ValueError before reaching here. Please widen that boundary and add a real streaming-path regression test.

[mohamedelrefaiy]

You're right, and thanks for catching it. Guarding the helper alone was not enough, since both callers parse before they ever reach it. Fixed in 312b349:

• agent/chat_completion_helpers.py:2943 now catches ValueError instead of JSONDecodeError before calling _repair_tool_call_arguments.
• agent/tool_executor.py:105 now catches (ValueError, TypeError).

json.JSONDecodeError subclasses ValueError, so both boundaries still catch everything they caught before.

Three regression tests exercise the real paths rather than the helper, in tests/run_agent/test_run_agent.py:

• test_long_numeric_literal_does_not_escape_streaming_builder
• test_concurrent_long_numeric_literal_rejected_without_crash
• test_nested_array_truncation_is_repaired_in_streaming_path

The first two fail without the boundary change — the streaming builder aborts with Exceeds the limit (4300 digits) for integer string conversion before the repair helper runs, which is exactly the bypass you described.

[mohamedelrefaiy]

Both problems are addressed, plus three more that surfaced while auditing the rest of the pipeline. Four commits pushed.

The ValueError bypass you flagged (312b349). Guarding the helper was not enough because both callers parse first. agent/chat_completion_helpers.py:2943 now catches ValueError; agent/tool_executor.py:105 now catches (ValueError, TypeError). JSONDecodeError subclasses ValueError, so coverage is a strict superset. Three tests in tests/run_agent/test_run_agent.py exercise the streaming and executor paths rather than the helper, two of which fail without the change.

Three further fixes, all found by testing the passes against each other:

• 73e8f09 — the trailing-comma strip used re.sub(r',\s*([}\]])', ...), which has no notion of string boundaries and rewrote a comma inside a string value. On its own that degraded to "{}"; paired with a nesting-aware closer it parsed cleanly and delivered a corrupted value: {"sep": ", ]", "files": ["a" became {"sep": "]", ...}. Same commit adds RecursionError to the never-raises guards — json.loads recurses per nesting level, so "[" * 100000 raised straight out of a function documented never to raise.
• 0d649be — str.isspace() and bare str.rstrip() are Unicode-aware and match U+001C–U+001F, U+0085, U+00A0 and U+2028, none of which JSON permits outside a string. Treating them as skippable deleted them and returned a payload that parses, so {"command":"echo X",\x1c} became an executable call. Note the pre-existing \s regex had the same hole. Also stops trimming a dangling comma before appending closers, since {"a": [1, 2, is the model promising a value that never arrived — completing it presents a short list as a whole one. That input now fails closed, which is narrower than before and deliberate.
• 7998eb4 — the excess-closer trim still used str.count, the same technique this PR replaces elsewhere and for the same reason. {"s":"{","x":[1]}} counted as balanced, so a payload one character from valid degraded to "{}". Trimming only removes a closer that is the payload's last token; an excess closer mid-payload ({"a": [1]]}) is left alone rather than taking a legitimate closer with it.

Net effect: the branch is now stricter than main on inputs it used to execute, while recovering the shapes it used to silently drop. tests/run_agent: 2131 passed, 2 failed, both reproducing on an unmodified checkout of the base commit.

Two gaps I did not fix here, both pre-existing and verified against an unmodified checkout:

1. Non-finite numbers still reach tools. {"c":"echo X","timeout":1e999,} survives as 1e999 and parses to float('inf'); NaN, Infinity and -Infinity behave the same. The allow_nan=False in 73e8f09 constrains only pass 0's re-serialisation — pass 3's plain json.loads and _parse_tool_arguments both accept these Python extensions. A real fix needs a finite-number policy at every parse boundary.
2. Lone surrogate escapes pass through: {"path":"\uD800",} yields a Python string carrying a lone surrogate, which can fail at a UTF-8 boundary. Worth noting because this module already carries _sanitize_surrogates for that hazard, and a valid surrogate pair is preserved as two units that the later sanitizer replaces independently.

Happy to send either as its own PR if you want them.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the delimiter-count failure in tool-call argument repair: #42510 narrowly replaces fixed-order closing with a string-aware LIFO stack, while #68612 incorporates that core fix and extends it across related sanitization passes, exception boundaries, and real streaming/executor paths. The latter also deliberately leaves truncation inside strings or after dangling commas unrepairable to avoid executing silently incomplete arguments.

Related pull requests

• #42510 [closed] duplicate — (+61/-11) — leave closed as superseded by #68612: its focused string-aware LIFO delimiter repair and regression tests correctly expose the nesting bug, so it remains relevant as the narrower reference implementation. The maintainer-bot close verdict rejects that reconstruction direction under the standing policy; reopening is unnecessary because #68612 contains the same core change and has a later contributor keep-open review.
• #68612 related — (+397/-23) — keep open with a salvage path: retain the nesting-aware closer, string-aware comma/excess-closer handling, fail-closed treatment of incomplete values, and real-path regressions. The contributor keep_open review identified uncaught ValueError boundaries at agent/chat_completion_helpers.py:3373-3375 and agent/tool_executor.py:102-107; the diff addresses both boundaries and adds streaming/executor tests, while also supplying broader safeguards that should be reviewed independently of the core LIFO fix.

Duplicates

#42510 and #68612 substantially duplicate the string-aware LIFO replacement for count-based delimiter closing; #68612 is the broader successor, so #42510 can remain closed as superseded by #68612.

Suggested consolidation

Keep open with a salvage path for #68612: preserve the focused LIFO repair and its end-to-end regression coverage, then review the additional sanitizer and exception-boundary changes as separable parts because they materially expand scope and risk. Leave #42510 closed as superseded by #68612; do not reopen it over the maintainer-bot policy verdict.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup42510 ["PRs duplicating each other"]
        P42510["PR #42510 (closed)"]
        P68612["PR #68612 (open)"]
    end
    class P42510 closed
    class P68612 open
    class P68612 target
    click P42510 "https://github.com/NousResearch/hermes-agent/pull/42510"
    click P68612 "https://github.com/NousResearch/hermes-agent/pull/68612"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 29 kB of PR diffs, 7 kB of issue/PR text, 10 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.