fix(agent): parse Qwen/Hermes <tool_call> bare-JSON tool calls in text mode

[harshit-ojha0324]

Summary

parse_tool_blocks matched the <tool_call> wrapper but only looked for <invoke>/<parameter> XML inside it. Qwen and Hermes/NousResearch models are trained to emit the OpenAI function-call object as JSON directly inside the wrapper:

<tool_call>
{"name": "bash", "arguments": {"command": "mkdir -p agent-test"}}
</tool_call>

In text mode — a manually added Ollama endpoint, native_tools=False — the block matched no inner format and fell through as inert text, so the agent talked but never acted. Since Qwen is one of the most common local families on Ollama, this hit a lot of "the agent narrates but nothing runs" reports. This adds a JSON-body fallback that runs only after the existing <invoke>/<direct> XML parses come up empty, reusing function_call_to_tool_block — the same converter used for native structured tool calls. It handles arguments as an object or a JSON-encoded string (a Hermes variant), tool-name aliases (shell → bash), and multiple <tool_call> blocks in one turn (tracked per-wrapper, not globally, since Qwen commonly emits several). Bare JSON outside a <tool_call> wrapper is deliberately not parsed, so example JSON in prose can't execute — the wrapper is the disambiguating signal. Stripping already removed the <tool_call> wrapper regardless of its contents, so no strip change is needed.

Scope: the canonical <tool_call> JSON format from the issue title. The [bash]…[/bash] square-bracket drift the same report mentions is intentionally out of scope — it has no wrapper to disambiguate it from prose, so parsing it globally would risk executing example text; native tool calling via the per-endpoint supports_tools flag (filed separately as #5192) is the right fix for those finetune-drift cases.

Target branch

• This PR targets dev, not main.

Linked Issue

Fixes #5187

Type of Change

• Bug fix (non-breaking — fixes a confirmed issue)
• New feature (non-breaking — adds new behaviour)
• Breaking change (changes or removes existing behaviour)
• Refactor / cleanup (behaviour unchanged)
• Documentation only
• CI / tooling / configuration

Checklist

• I searched open issues and open PRs — this is not a duplicate.
• This PR targets dev
• My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
• I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.

How to Test

Fed the issue's exact <tool_call> payload through the public parser (from src.agent_tools import parse_tool_blocks, strip_tool_blocks) — the path src/agent_loop.py calls on every agent round:

raw = '<tool_call>\n{"name": "bash", "arguments": {"command": "mkdir -p agent-test"}}\n</tool_call>'
parse_tool_blocks(raw)          # before: []   after: [ToolBlock("bash", "mkdir -p agent-test")]
strip_tool_blocks(raw).strip()  # ""  (wrapper never shown to the user)

Verified through parse_tool_blocks(resp, skip_fenced=…) with both skip_fenced=False and True (Pattern 3 runs regardless of the fenced-skip mode the agent loop chooses per model), and confirmed the issue's two-call turn (mkdir + python script) yields both blocks in order.

Behaviours covered by the new tests/test_qwen_tool_call_parsing.py (9 cases):

• canonical <tool_call> JSON parses and strips clean
• multiple <tool_call> JSON blocks in one turn all parse, in order
• arguments as a JSON-encoded string (Hermes variant)
• tool-name alias normalisation (shell → bash)
• missing </tool_call> closer (streamed truncation) still parses
• regression: existing <invoke>/<parameter> XML still parses
• safety: bare JSON in prose is not executed; non-object JSON and name-less objects inside the wrapper are ignored

Checks run:

./venv/bin/python -m pytest tests/test_qwen_tool_call_parsing.py \
  tests/test_gemma_tool_call_parsing.py tests/test_redos_xml_tool_parsers.py \
  tests/test_fenced_invoke_no_raw_xml.py tests/test_web_search_raw_json_tool_call.py \
  tests/test_unknown_tool_calls.py tests/test_fenced_example_not_executed_for_native_models.py \
  tests/test_redos_llm_parsers.py tests/test_fenced_inline_args.py \
  tests/test_misfenced_read_file_tool_call.py tests/test_plain_ui_control_open_panel.py \
  tests/test_tool_parsing_nonstring.py -q
# 118 passed
./venv/bin/python -m py_compile src/tool_parsing.py

ReDoS check on the new path: parse_tool_blocks('<tool_call>{"name":"bash",' * 20000) returns in ~0.02 s — the JSON fallback runs inside the existing forward-only _iter_delimited scan, so it doesn't reintroduce the O(n²) lazy-rescan the surrounding code guards against.

Visual / UI changes — REQUIRED if you touched anything that renders

Not applicable — backend parser change (src/tool_parsing.py + tests); nothing that renders was touched.

• I am not an LLM agent submitting a bulk PR.

[alteixeira20]

The focused Qwen/Hermes wrapped-JSON parser looks good, but the branch currently conflicts with dev in src/tool_parsing.py, so the guarded audit cannot validate the actual merge result.

I transplanted the PR's focused changes onto the current dev head locally and verified:

• git diff --check passes;
• the changed parser and test file compile;
• the PR tests and adjacent parser regression suites pass;
• 130 tests passed in total;
• multiple wrapped JSON calls remain ordered;
• mixed XML and JSON wrappers parse correctly;
• bare JSON outside <tool_call> remains inert;
• malformed and unknown calls are ignored;
• the malformed-opener stress case remains linear and completed in approximately 0.03 seconds.

Please rebase the branch onto the latest dev and resolve the src/tool_parsing.py conflict while preserving the per-wrapper fallback logic. Once the PR is mergeable again, I will rerun the guarded audit against the new pinned head.

Non-blocking note: a complete wrapper followed by an unclosed final wrapper currently recovers only the complete call. The standalone unclosed-wrapper case works as described. This combination is outside the reported reproduction, but it may be worth documenting or adding coverage for later.

[harshit-ojha0324]

Rebased onto the latest dev (now at 30a5cc8, mergeable). The only conflict was positional in src/tool_parsing.py — dev added _parse_function_model_call in the same spot as this PR's _parse_json_tool_call_body; I kept both, and the per-wrapper fallback in parse_tool_blocks (the before = len(blocks) per-body tracking, so multiple <tool_call> JSON blocks stay independent) is preserved unchanged.

Re-verified on the new head: focused + adjacent parser regression suites pass, ordered multi-call and mixed XML+JSON wrappers parse correctly, bare JSON outside a wrapper stays inert, and the 20k-opener stress case is still linear (~0.02s).

On the non-blocking note — good catch, you're right: a complete wrapper followed by an unclosed final wrapper recovers only the complete call, because the closed-wrapper pass finds a block and the unclosed-wrapper fallback is gated on if not blocks. It's outside the reported repro, so I left this PR focused on the rebase. Happy to follow up with a separate PR that runs the unclosed-wrapper recovery on the trailing remainder after the last closed wrapper (plus coverage), if you'd like — or fold it in here, your call.

[RaresKeY]

Thanks for rebasing. I rechecked the current head (30a5cc8); the conflict is resolved and the text-mode compatibility goal is reasonable, but two blocking parser issues remain.

Findings

Do not reinterpret JSON argument data as another tool call

The wrapper parser scans the complete body as direct XML before trying the JSON fallback. Consequently, XML-like text inside a valid JSON argument can select a different tool.

For example, a wrapped write_file call whose content contains <bash>echo unsafe</bash> is parsed as a Bash call, and the intended write is dropped.

Please classify JSON-looking wrapper bodies as JSON before direct-XML scanning and fail closed if that JSON cannot be safely converted. Add a regression proving that XML-like strings inside JSON arguments remain data for the intended tool.

Reject non-string command fields before constructing a ToolBlock

A payload such as:

<tool_call>{"name":"bash","arguments":{"command":1}}</tool_call>

produces a ToolBlock with integer content. The agent loop later calls .strip() on that value and aborts the turn.

Please reject non-string scalar fields—or enforce that every converter result has string-compatible ToolBlock.content—and add regressions for numeric command and code values.

Safety boundary

This should remain a compatibility fallback for explicitly enabled agent text-tool mode; native structured message.tool_calls should remain preferred. Please also add coverage confirming that wrapped examples inside fenced/quoted prose remain inert and that this parsing cannot activate in ordinary non-agent chat.

[harshit-ojha0324]

Both good catches, confirmed and fixed in edda743.

P1: the wrapper loop now classifies a JSON-looking body ({...} / [...]) as JSON before any direct-XML scan and fails closed, so it never falls through to XML. A wrapped write_file whose content is <bash>echo unsafe</bash> now stays a single write_file with the tag kept as data, and a JSON body that can't be safely converted is dropped rather than reinterpreted. Regression added.

P2: _parse_json_tool_call_body now rejects a converted block whose content isn't a string, so {"command": 1} and {"code": 3.14} fail closed instead of building a non-string ToolBlock that later breaks .strip(). Regressions added for numeric command and code.

On the safety boundary: parse_tool_blocks is only invoked from the agent loop (agent_loop.py), never in ordinary chat, and bare JSON without a <tool_call> wrapper stays inert (tested). One note for transparency: wrapped markup inside fenced or quoted prose is currently treated as live by the existing parser for every <tool_call> format (<invoke>, direct-XML, DSML), not just this JSON path. Gating that inside fences would change behavior for those formats too, so I kept it out of this PR. Happy to take it as a separate change if you want that hardening globally.

New head is the P1/P2 fixes on top of the rebased branch; the focused and adjacent parser suites are green and the 20k-opener stress stays linear (~0.01s).