Skip to content

fix(tool-calls): repair concatenated JSON, extract from noise, scrub nonfinite, coerce booleans, port content-channel promotion - #38042

Open
benclawbot wants to merge 1 commit into
NousResearch:mainfrom
benclawbot:fix/tool-call-repair-pass
Open

fix(tool-calls): repair concatenated JSON, extract from noise, scrub nonfinite, coerce booleans, port content-channel promotion#38042
benclawbot wants to merge 1 commit into
NousResearch:mainfrom
benclawbot:fix/tool-call-repair-pass

Conversation

@benclawbot

Copy link
Copy Markdown

Summary

Six pre-existing tool-call argument failure modes in the Hermes dispatch pipeline, surfaced by adversarial probing and cross-referenced against open upstream PRs. 241/241 tests passing in the repair+coerce+transport surface.

This work is the local-install complement to three open upstream PRs (the design mirrors their contracts so a future merge is near-zero-conflict):

What's fixed

  1. Concatenated streamed tool-call args ({"a":1}{"b":2}) — {} fallback was dropping every call. Added _split_concatenated_tool_call_arguments using json.JSONDecoder().raw_decode(pos=...) to walk left-to-right and peel complete top-level dicts. Matches the upstream PR fix(run_agent): split concatenated streamed tool-call args #25346 contract verbatim.

  2. Valid JSON buried in noise ({"a":1}garbage, {"a":1}\u2028{"b":2}, BOM markers, model preamble/postscript) — {} fallback. Added _extract_first_json_object which uses raw_decode from every { position and rescues the first complete dict.

  3. NaN / Infinity pass-throughjson.loads(strict=False) accepts the literal tokens; re-emits as NaN/Infinity literals; strict-validating providers (Anthropic, AWS Bedrock, Google Vertex) reject with HTTP 400. Added _scrub_nonfinite_numbers which recursively replaces with None.

  4. Integer overflow silent corruption in _coerce_number'99999999999999999999' was going float(value) → int(f), producing 100000000000000000000 (off by ~9 orders of magnitude). Fixed by trying int(value) first for integer_only fields; the float path is the fallback for decimal strings like "3.0".

  5. _coerce_boolean accepted only "true"/"false" while M3, DeepSeek, Qwen, and GLM routinely emit "1"/"0"/"yes"/"no"/"on"/"off". Extended the match set. Two pre-existing tests asserted the old behaviour; updated with comments explaining the change.

  6. Content-channel tool calls (M3 / MiniMax, Kimi K2, Ollama qwen2.5-coder, GLM, Gemma) — models emit tool calls in content as <invoke name="…">…</invoke> or <tool_call>{…}</tool_call> instead of the structured tool_calls field, causing the call to leak as chat text. Ported the leaf parser from PR feat(transports): execute tool calls models emit in content (Ollama/Kimi/MiniMax/Gemma) #35129 into a new module agent/transports/content_tool_calls.py (309 lines, 6 parsers, exact-name gate, fail-closed, env kill-switch HERMES_PROMOTE_TOOLCALLS) and added a single promotion seam in agent/conversation_loop.py. The seam is a strict no-op when structured tool_calls already exist, so native tool-calling paths are untouched.

Design decisions

  • Content-channel promotion is exact-name match only — no fuzzy name repair for content-extracted calls. A name lifted from free-text is lower-trust than one from a structured field; fuzzy-repairing risks executing the wrong tool from prose. Native structured calls still get the fuzzy repair at the loop seam.
  • Pre-pass ordering matters: the {"a":1}{"b":2} split MUST run before the "valid + garbage" extraction, otherwise the extractor returns {"a":1} and the second call is lost.
  • Verified the 4 things I initially claimed were bugs but weren't via direct probe — string "null" in non-nullable string field (passes through correctly, the _schema_allows_null check handles the nullable case), lone surrogate pair (Python's json handles emoji round-trips correctly), empty dict {} for array field (logged and falls through gracefully), hex numbers (correctly rejected).

Tests

  • 4 new classes in test_repair_tool_call_arguments.py (53 tests) for the split/extract/scrub helpers and end-to-end coverage of the new repair stages.
  • 1 new class in test_tool_arg_coercion.py (8 tests) for integer overflow precision.
  • 2 pre-existing tests in test_tool_arg_coercion.py updated to assert the new correct boolean coercion behaviour (with comments explaining the change).
  • New file tests/agent/transports/test_content_tool_calls.py (17 tests) covering all 6 content-channel parsers, dedup, env gates, and fail-closed behaviour on unknown tool names.

Test totals: 241/241 passing across test_content_tool_calls.py (17) + test_transport.py (23) + test_chat_completions.py (75) + test_tool_arg_coercion.py (65) + test_repair_tool_call_arguments.py (61).

Files changed

6 modified, 2 new. +1035 / -13.

  • agent/message_sanitization.py — three new helpers, two new repair stages
  • run_agent.py — re-exports
  • model_tools.py_coerce_number int-first, _coerce_boolean extended
  • agent/transports/content_tool_calls.py (NEW, 309 lines) — port from PR feat(transports): execute tool calls models emit in content (Ollama/Kimi/MiniMax/Gemma) #35129
  • agent/conversation_loop.py — promotion seam
  • tests/run_agent/test_repair_tool_call_arguments.py — 53 new tests
  • tests/run_agent/test_tool_arg_coercion.py — 8 new tests + 2 updated
  • tests/agent/transports/test_content_tool_calls.py (NEW, 17 tests)

Links

Test plan

cd /home/thomas/.hermes/hermes-agent  # or your normal hermes-agent checkout
venv/bin/python -m pytest \
    tests/agent/transports/test_content_tool_calls.py \
    tests/agent/transports/test_transport.py \
    tests/agent/transports/test_chat_completions.py \
    tests/run_agent/test_tool_arg_coercion.py \
    tests/run_agent/test_repair_tool_call_arguments.py
# Expected: 241 passed

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists labels Jun 3, 2026
…nonfinite, coerce booleans, port content-channel promotion

Six pre-existing failure modes in the tool-call argument pipeline, all
surfaced by adversarial probing and online research into upstream
Hermes PRs. 241/241 tests passing in the repair+coerce+transport surface.

A. Concatenated streamed tool-call args — Gemini-3-flash-preview and
   some Ollama routes emit ``{"a":1}{"b":2}`` with no delimiter. The
   repair layer was falling through to ``{}`` and losing every call.
   Fixed by adding ``_split_concatenated_tool_call_arguments`` (matches
   upstream PR NousResearch#25346/NousResearch#36039 verbatim so a future merge is conflict-
   free), which uses ``json.JSONDecoder().raw_decode(pos=...)`` to
   walk left-to-right and peel off complete top-level dicts.

B. JSON buried in surrounding noise — U+2028/U+2029 line separators,
   BOM markers, model preamble/postscript, extra ``true``/``null``/
   ``"excess"`` after valid JSON, etc. All made ``json.loads`` reject
   the whole payload. Fixed by adding ``_extract_first_json_object``,
   which uses ``raw_decode`` from every ``{`` position in the string
   and rescues the first complete dict it finds.

C. NaN / Infinity pass-through — ``json.loads(strict=False)`` accepts
   the literal tokens, but re-serialising produces ``NaN``/``Infinity``
   literals that strict-validating providers (Anthropic, AWS Bedrock,
   Google Vertex) reject with HTTP 400. Fixed by adding
   ``_scrub_nonfinite_numbers`` which recursively replaces them with
   ``None``.

D. Integer overflow silent corruption in ``_coerce_number`` —
   ``'99999999999999999999'`` was being routed through ``float()`` then
   ``int()``, producing ``100000000000000000000`` (off by ~9 orders of
   magnitude). Python's ``int()`` has arbitrary precision, so for
   ``integer_only`` fields we now try ``int(value)`` first; the float
   path is the fallback for decimal strings like ``"3.0"``.

E. ``_coerce_boolean`` accepted only ``"true"`` / ``"false"`` while
   M3, DeepSeek, Qwen, and GLM routinely emit ``"1"`` / ``"0"`` /
   ``"yes"`` / ``"no"`` / ``"on"`` / ``"off"`` for boolean fields.
   Extended the match set. Two pre-existing tests asserted the old
   wrong behaviour; updated with comments explaining the change.

F. Content-channel tool calls (M3 / MiniMax, Kimi K2, Ollama
   qwen2.5-coder, GLM, Gemma) — models emit tool calls in the
   response ``content`` field as ``<invoke name="…">…</invoke>`` or
   ``<tool_call>{…}</tool_call>`` instead of the structured
   ``tool_calls`` field, causing the call to leak as chat text. Ported
   the leaf parser from upstream Hermes PR NousResearch#35129 into a new module
   ``agent/transports/content_tool_calls.py`` (309 lines, 6 parsers,
   exact-name gate, fail-closed, env kill-switch
   ``HERMES_PROMOTE_TOOLCALLS``) and added a single promotion seam
   in ``agent/conversation_loop.py`` at the post-normalization point
   in the response flow. The seam is a strict no-op when structured
   ``tool_calls`` already exist, so native tool-calling paths are
   untouched.

Tests:
- 4 new classes in test_repair_tool_call_arguments.py (53 tests) for
  the split/extract/scrub helpers and end-to-end coverage of the new
  repair stages.
- 1 new class in test_tool_arg_coercion.py (8 tests) for integer
  overflow precision.
- 2 pre-existing tests in test_tool_arg_coercion.py updated to assert
  the new correct boolean coercion behaviour.
- New file tests/agent/transports/test_content_tool_calls.py (17 tests)
  covering all 6 content-channel parsers, dedup, env gates, and
  fail-closed behaviour on unknown tool names.

Files changed: 6 modified, 2 new. +506/-13.

Upstream: PRs NousResearch#25346, NousResearch#36039, NousResearch#35129 are still open. If the user
wants to push these locally, the diffs are designed to merge near-zero-
conflict with those PRs.
@benclawbot
benclawbot force-pushed the fix/tool-call-repair-pass branch from cda1285 to f847aa9 Compare June 3, 2026 10:51

@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 detailed investigation and regression coverage. The integer precision issue is real on current main: model_tools.py:936-947 still takes integer-schema strings through float() before int().

Problems

  • agent/message_sanitization.py:302-313 detects concatenated objects but returns only split[0]. Current streaming assembly emits one call per accumulated slot (agent/chat_completion_helpers.py:2473-2503), and this PR does not add the required fan-out path, so later calls remain lost.
  • agent/conversation_loop.py:3503-3521 adds shared-loop parsing/execution of calls from free-text content. Maintainer review on PR #35129 explicitly rejected this compatibility layer in core; unsupported wire formats must be handled at provider/capability level.
  • The new JSON split/extract/scrub stages are model-output reconstruction, which conflicts with the repository's standing policy against adding repair passes for malformed model output.

Suggested changes

  • Salvage the int-first integer coercion change and regression tests as a focused PR.
  • Keep any boolean-coercion proposal separate.
  • Remove the content-field promotion and new generic reconstruction passes.

Automated hermes-sweeper review.

"Concatenated tool_call arguments for %s — keeping first of %d",
tool_name, len(split),
)
return split[0]

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.

This only returns the first decoded object. The actual streaming builder still emits exactly one call per tool_calls_acc slot (agent/chat_completion_helpers.py:2473-2503), and this PR does not consume the rest of split, so the claimed parallel-call recovery still drops every later object. Either implement fan-out at the assembly layer with end-to-end coverage or remove this repair.

extract_content_tool_calls,
)
valid = getattr(agent, "valid_tool_names", None) or set()
promoted, residual = extract_content_tool_calls(

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.

Maintainer review on #35129 explicitly rejected shared-loop promotion of JSON/XML-like calls from free-text content; unsupported model wire formats must be handled by a provider/capability integration. Please remove this core promotion seam rather than adding another content parser.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 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 comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants