fix(execute_code): tolerate tool payloads with a trailing hint

[israellot]

What & why

The hermes_tools shim generated for execute_code sandboxes parses every RPC tool result with a strict json.loads():

# tools/code_execution_tool.py:479 (uds transport) and :545 (file transport)
result = json.loads(raw)

Several tools append human-readable text after the JSON payload. search_files appends a truncation hint (tools/file_tools.py:1928):

{"total_count": 50, "matches_text": "...", "truncated": true}

[Hint: Results truncated. Use offset=50 to see more, or narrow with a more specific pattern or file_glob.]

json.loads() raises JSONDecodeError: Extra data on that, so the sandbox call fails even though the tool call itself succeeded and the data is present. From inside execute_code the failure looks like the wrapper choking on large output, which sends you looking in the wrong place — the trigger is the appended hint, not the size.

Both transports had it; only the UDS path is commonly hit today, so the file-transport (remote backend) case was latent.

Reachability today, and why this gets worse soon

The hint currently only fires when context > 0. With context=0 the truncated flag can never become True, because fetch_limit is limit + offset while the flag is computed as total > offset + limit (tools/file_operations.py:2293, :2421) — the extra row that would prove truncation is never fetched. Measured on current main:

call	hint emitted	json.loads()
limit=3, context=0	no	ok
limit=3, context=2	yes	JSONDecodeError: Extra data
limit=100, context=0	no	ok

#41439 (open, approved) fixes that off-by-one with an n+1 sentinel so truncated reports correctly in the default context=0 path. Once it merges, every truncated search_files inside execute_code starts raising — this fix removes that latent regression ahead of it.

Fix

Add a shared _parse_tool_result() to the shim's _COMMON_HELPERS and use it from both transports. It raw_decode()s the first JSON value and preserves any trailing text under _hint rather than discarding it (_hint is only set when absent, so a tool's own key is never clobbered). The existing double-encoded-payload unwrap is kept, and genuinely malformed payloads still raise JSONDecodeError.

This is the same bug class and same fix shape as #33930, which handles the appended [Tool loop warning: ...] suffix in tools/delegate_tool.py. That PR fixes the delegate surface; this one fixes the sandbox shim. They touch different files and do not conflict.

How to test

Automated (CI-parity wrapper):

scripts/run_tests.sh tests/tools/test_code_execution.py \
  tests/tools/test_code_execution_modes.py \
  tests/tools/test_code_execution_windows_env.py

150 passed. The 7 new tests in TestGeneratedToolResultParsing exec() the generated shim and call the parser for real, rather than asserting on substrings of the generated source: plain payloads round-trip, a trailing hint is preserved under _hint instead of raising, an existing _hint is not clobbered, double-encoded payloads still unwrap, JSON arrays with trailing text work, and malformed input still raises.

Manual reproduction of the underlying failure on unpatched main:

printf 'MATCH %d\n' $(seq 0 9) > /tmp/hits.txt
python - <<'PY'
import json
from tools.file_tools import search_tool
s = search_tool(pattern="MATCH", path="/tmp/hits.txt", target="content",
                output_mode="content", limit=3, context=2)
print(json.loads(s))   # JSONDecodeError: Extra data  <-- shim does exactly this
PY

End to end, through a real sandbox (execute_code running search_files(..., limit=3, context=2) against that file). Before:

File "/tmp/hermes_sandbox_*/hermes_tools.py", line 90, in _call
    result = json.loads(raw)
json.decoder.JSONDecodeError: Extra data: line 3 column 1 (char 233)

After:

RESULT_TYPE dict
TOTAL 10
HINT [Hint: Results truncated. Use offset=3 to see ...

Windows footguns (change touches file I/O paths in the generated stub):

python scripts/check-windows-footguns.py tools/code_execution_tool.py tests/tools/test_code_execution.py
# ✓ No Windows footguns found (2 file(s) scanned).

Platforms tested

Linux (Ubuntu, Python 3.12) fully verified — full suite plus the manual repro above. Windows and macOS not manually tested; the change is pure stdlib json string parsing with no OS-dependent behaviour, and check-windows-footguns.py passes on both touched files.

Related

• fix(tools/delegate_tool): correctly classify error blobs with appended loop-warning text #33930 — same json.loads → raw_decode bug class for the [Tool loop warning: ...] suffix in delegate_tool.py. Different file, complementary; no conflict.
• fix(tools): detect content-search truncation so the model paginates #41439 — search_files truncation off-by-one. Independent, but merging it widens this bug's blast radius from a context>0 edge case to the default path, so landing this first (or together) avoids a regression.
• Feature: Structured Tool Result Hints (CTAs) — Next-Action Guidance to Reduce Agent Iterations (inspired by incur) #722 — structured tool-result hints/CTAs. The broader design question of whether hints should ride inside the payload rather than be appended as trailing prose. This PR deliberately only makes the consumer tolerant; other producers (web_tools.py, browser_tool.py, agent/subdirectory_hints.py) append similar footers and would be addressed by that design change, not here.

[teknium1]

Thanks for the focused fix. The premise is confirmed on current main: tools/file_tools.py:1914-1920 appends the truncation hint after JSON, while generated execute-code stubs strictly parse it at tools/code_execution_tool.py:493-500 and :560-566.

Problems

• In the proposed _parse_tool_result() (tools/code_execution_tool.py, PR diff), a double-encoded response with an outer trailing hint loses that hint: the isinstance(result, str) branch recursively returns the decoded inner payload and discards the already-captured outer trailing text. Please preserve the suffix through that unwrap and cover json.dumps(json.dumps({"ok": true})) + "\n\n[Hint: ...]".
• TestGeneratedToolResultParsing.test_both_transports_define_the_parser checks generated source substrings. AGENTS.md prohibits source-shape tests; the surrounding exec() tests can verify both generated transports behaviorally instead.

Suggested changes

• Carry outer trailing text into the final decoded dictionary after double-encoded unwrapping.
• Delete the source-inspection test and retain/add behavioral regression coverage.

Automated hermes-sweeper review.

[teknium1]

This recursive return drops trailing captured from the outer response. Please preserve it when an outer double-encoded JSON string has a suffix, and add a regression case for that combination.

[israellot]

Good catch — fixed in adc79ba.

The recursion discarded the outer trailing because it returned _parse_tool_result(result) directly, so a hint sitting after the outer JSON string was lost. The unwrap is now iterative and carries the suffix through it:

result, trailing = _decode(text)
while isinstance(result, str):
    try:
        inner, inner_trailing = _decode(result.strip())
    except (json.JSONDecodeError, TypeError):
        break
    result = inner
    trailing = inner_trailing or trailing

An inner hint wins when both levels carry one (it is the more specific attachment); otherwise the outer suffix survives. tools/code_execution_tool.py:436-455.

Two regression tests cover both placements — test_double_encoded_payload_keeps_outer_trailing_hint (tests/tools/test_code_execution.py:189) and test_double_encoded_payload_keeps_inner_trailing_hint (:203). Red-before-green on the outer case, with the source change stashed and the tests left in place:

$ git stash push -- tools/code_execution_tool.py
$ python -m pytest tests/tools/test_code_execution.py -q -k TestGeneratedToolResultParsing
8 passed, 1 failed
  tests/tools/test_code_execution.py:201: in test_double_encoded_payload_keeps_outer_trailing_hint
      self.assertEqual(result["_hint"], hint)
  E   KeyError: '_hint'

Green with the fix: scripts/run_tests.sh tests/tools/test_code_execution.py -q → 85 tests passed, 0 failed.

[teknium1]

Please avoid generated-source substring assertions here. The exec()-based tests below can exercise both transport parsers directly and comply with the repository's behavioral-test rule.

[israellot]

Agreed — that assertion tested the generator's source text rather than behavior, and assertNotIn("result = json.loads(raw)", src) would have gone green on a rename while the bug came back. Removed in adc79ba.

The behavioral coverage it was standing in for is now real: every case in TestGeneratedToolResultParsing (tests/tools/test_code_execution.py:143) loops over ("uds", "file") and exercises the parser compiled out of the generated module, so both transports are asserted by execution rather than by substring. That closes the gap the source assertion was hedging against — a transport regressing to a strict json.loads now fails test_trailing_hint_is_preserved_not_raised on that transport.

Cases in the class, all per-transport: plain object roundtrip, trailing hint preserved, existing _hint not clobbered, double-encoded unwrap, double-encoded with an outer trailing hint, double-encoded with an inner trailing hint, non-JSON trailing on a JSON list, plain string payload returned as-is, and truly malformed input still raising JSONDecodeError.

scripts/run_tests.sh tests/tools/test_code_execution.py -q → 85 tests passed, 0 failed.

[israellot]

Both review findings addressed in adc79ba.

1. Recursive unwrap dropped the outer trailing hint (tools/code_execution_tool.py) — the doubly-encoded branch returned _parse_tool_result(result) and threw away the trailing captured at the outer level, so a hint after the outer JSON string was silently lost. The unwrap is now an iterative loop that carries the suffix through it; an inner hint wins when both levels have one, otherwise the outer suffix survives (:436-455).

2. Source-substring assertion replaced with behavioral coverage (tests/tools/test_code_execution.py) — test_both_transports_define_the_parser asserted on the generated text (assertNotIn("result = json.loads(raw)", src)), which a rename would have satisfied while the bug returned. Deleted. Every case in TestGeneratedToolResultParsing (:143) now loops over ("uds", "file") and runs the parser compiled from the generated module, so both transports are covered by execution. Two new regression cases pin the hint placements: test_double_encoded_payload_keeps_outer_trailing_hint (:189) and test_double_encoded_payload_keeps_inner_trailing_hint (:203).

Verification

Red-before-green on the outer-hint case, source change stashed and tests left in place:

$ git stash push -- tools/code_execution_tool.py
$ python -m pytest tests/tools/test_code_execution.py -q -k TestGeneratedToolResultParsing
8 passed, 1 failed
  tests/tools/test_code_execution.py:201: in test_double_encoded_payload_keeps_outer_trailing_hint
      self.assertEqual(result["_hint"], hint)
  E   KeyError: '_hint'

Green with the fix:

$ scripts/run_tests.sh tests/tools/test_code_execution.py -q
=== Summary: 1 files, 85 tests passed, 0 failed (100% complete) ===

Two failures unrelated to this diff appear when the file is run in a single process under my local environment (TestExecuteCode.test_timeout_enforcement and TestInterruptHandling.test_interrupt_event_stops_execution, both 'error' != 'timeout'/'interrupted'). They reproduce identically with this branch's changes stashed, and they pass under the repo's own scripts/run_tests.sh runner — local-environment artifacts, not fallout from this PR.

[andrexibiza]

Interlock with #82243

#82243 expands execute_code RPC to arbitrary deferred plugin/MCP tools and adds _serialize_rpc_result(), but strings are still written verbatim while both generated clients start with json.loads(raw). The in-tree deferrable A2A handlers intentionally return plain/multiline text, so their new tool_call path fails after successful dispatch; multiline output can also split the NDJSON frame.

The fixes are complementary and touch the same template. #82243 should JSON-encode strings at the server boundary; this PR should retain ownership of tolerant client parsing for JSON plus trailing prose/double encoding. Please preserve the updated parser when rebasing either branch. #81622 owns the separate concatenated-frame case.

[israellot]

Agreed on the split, and confirmed against both diffs.

This PR keeps ownership of tolerant client-side parsing. The generated _parse_tool_result at tools/code_execution_tool.py:429-457 decodes the first JSON value, iteratively unwraps a double-encoded payload, and preserves the trailing text under _hint — inner hint wins when both levels carry one, otherwise the outer suffix survives. That behavior is pinned by execution, not by source-substring assertions: every case in TestGeneratedToolResultParsing compiles the generated module and loops over both the uds and file transports, so a rename cannot satisfy the test while the bug returns.

#82243 owning the server-boundary JSON encoding of strings is the right half. The two are complementary: the client stays tolerant of JSON with trailing prose and double encoding regardless, which is what keeps the in-tree deferrable A2A handlers working when they return plain or multiline text.

On the rebase interlock: we overlap on exactly two files, tools/code_execution_tool.py and tests/tools/test_code_execution.py. This PR is MERGEABLE / CLEAN against current main right now, and it is the far smaller diff of the two (2 files, versus 34 in #82243). Simplest ordering is to merge this one first and let #82243 rebase on top — then there is nothing for it to preserve by hand, because the parser is already on main underneath it. If #82243 lands first instead, I will rebase this branch and re-verify both hint-placement regression cases.

Either way I will keep watch on the parser hunk and re-run the red-before-green check after any rebase. #81622 owning the separate concatenated-frame case matches my read of the three diffs.