Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions tests/tools/test_code_execution.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,94 @@ def test_file_transport_serializes_seq_allocation(self):
self.assertIn("with _seq_lock:", src)


class TestGeneratedToolResultParsing(unittest.TestCase):
"""Regression: tool payloads with a trailing human-readable hint.

Several tools (search_files when results are truncated, notably) emit a
JSON object followed by a blank line and a "[Hint: ...]" string. A strict
json.loads() on that raises JSONDecodeError("Extra data"), which used to
surface inside execute_code as a spurious parse failure even though the
underlying tool call succeeded.
"""

def _parser_for(self, transport):
src = generate_hermes_tools_module(["search_files"], transport=transport)
ns = {}
exec(compile(src, "hermes_tools", "exec"), ns)
self.assertIn("_parse_tool_result", ns)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

return ns["_parse_tool_result"]

def test_plain_json_object_roundtrips(self):
for transport in ("uds", "file"):
parse = self._parser_for(transport)
result = parse(json.dumps({"total_count": 2, "matches_text": "a\nb"}))
self.assertEqual(result["total_count"], 2)
self.assertNotIn("_hint", result)

def test_trailing_hint_is_preserved_not_raised(self):
hint = "[Hint: Results truncated. Use offset=50 to see more.]"
payload = json.dumps({"total_count": 50, "truncated": True}) + "\n\n" + hint
for transport in ("uds", "file"):
parse = self._parser_for(transport)
result = parse(payload)
self.assertEqual(result["total_count"], 50)
self.assertTrue(result["truncated"])
self.assertEqual(result["_hint"], hint)

def test_existing_hint_key_is_not_clobbered(self):
payload = json.dumps({"ok": True, "_hint": "from tool"}) + "\n\ntrailing"
for transport in ("uds", "file"):
parse = self._parser_for(transport)
self.assertEqual(parse(payload)["_hint"], "from tool")

def test_double_encoded_payload_still_unwraps(self):
payload = json.dumps(json.dumps({"output": "hi", "exit_code": 0}))
for transport in ("uds", "file"):
parse = self._parser_for(transport)
self.assertEqual(parse(payload)["output"], "hi")

def test_double_encoded_payload_keeps_outer_trailing_hint(self):
"""The suffix survives the double-encoded unwrap.

The outer value is a JSON string holding the real payload, and the hint
sits after that outer string. Unwrapping must not discard it.
"""
hint = "[Hint: Results truncated. Use offset=50 to see more.]"
payload = json.dumps(json.dumps({"ok": True})) + "\n\n" + hint
for transport in ("uds", "file"):
parse = self._parser_for(transport)
result = parse(payload)
self.assertTrue(result["ok"])
self.assertEqual(result["_hint"], hint)

def test_double_encoded_payload_keeps_inner_trailing_hint(self):
"""A hint after the inner payload is preserved too."""
hint = "[Hint: inner]"
payload = json.dumps(json.dumps({"ok": True}) + "\n\n" + hint)
for transport in ("uds", "file"):
parse = self._parser_for(transport)
result = parse(payload)
self.assertTrue(result["ok"])
self.assertEqual(result["_hint"], hint)

def test_non_json_trailing_on_json_list_does_not_raise(self):
payload = "[1, 2, 3]\n\n[Hint: more]"
for transport in ("uds", "file"):
parse = self._parser_for(transport)
self.assertEqual(parse(payload), [1, 2, 3])

def test_plain_string_payload_returned_as_is(self):
for transport in ("uds", "file"):
parse = self._parser_for(transport)
self.assertEqual(parse(json.dumps("just a message")), "just a message")

def test_truly_malformed_payload_still_raises(self):
for transport in ("uds", "file"):
parse = self._parser_for(transport)
with self.assertRaises(json.JSONDecodeError):
parse("not json at all")


class TestExecuteCodeRemoteTempDir(unittest.TestCase):
def test_execute_remote_uses_backend_temp_dir_for_sandbox(self):
class FakeEnv:
Expand Down
56 changes: 43 additions & 13 deletions tools/code_execution_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,47 @@ def retry(fn, max_attempts=3, delay=2):
time.sleep(delay * (2 ** attempt))
raise last_err


def _parse_tool_result(raw):
"""Parse an RPC tool-result payload into Python data.

Some Hermes tools append a human-readable hint AFTER the JSON payload --
e.g. search_files emits a JSON object, a blank line, then
"[Hint: Results truncated. Use offset=50 ...]" whenever results were
truncated. A strict json.loads() raises JSONDecodeError("Extra data") on
those, which used to surface inside execute_code as a spurious parse
failure even though the tool call itself succeeded. Decode the first JSON
value and keep any trailing text under the "_hint" key so nothing is lost.

A payload can also be doubly encoded (the outer JSON value is itself a JSON
string), and the trailing text can sit after either the outer or the inner
payload. Unwrap iteratively and carry the suffix through the unwrap instead
of dropping it.
"""
def _decode(text):
"""Decode the first JSON value in `text` -> (value, trailing_text)."""
try:
return json.loads(text), ""
except json.JSONDecodeError:
value, end = json.JSONDecoder().raw_decode(text)
return value, text[end:].strip()

text = raw.strip() if isinstance(raw, str) else raw
result, trailing = _decode(text)
while isinstance(result, str):
# Doubly-encoded payload: the outer JSON value is itself a JSON string.
try:
inner, inner_trailing = _decode(result.strip())
except (json.JSONDecodeError, TypeError):
break
result = inner
# A hint attached to the inner payload is the more specific one; keep
# the outer suffix when the inner level had none.
trailing = inner_trailing or trailing
if trailing and isinstance(result, dict) and "_hint" not in result:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

result["_hint"] = trailing
return result

'''

# ---- UDS transport (local backend) ---------------------------------------
Expand Down Expand Up @@ -476,13 +517,7 @@ def _call(tool_name, args):
if buf.endswith(b"\\n"):
break
raw = buf.decode().strip()
result = json.loads(raw)
if isinstance(result, str):
try:
return json.loads(result)
except (json.JSONDecodeError, TypeError):
return result
return result
return _parse_tool_result(raw)

'''

Expand Down Expand Up @@ -542,12 +577,7 @@ def _call(tool_name, args):
except OSError:
pass

result = json.loads(raw)
if isinstance(result, str):
try:
return json.loads(result)
except (json.JSONDecodeError, TypeError):
return result
result = _parse_tool_result(raw)
return result

'''
Expand Down
Loading