Conversation
`_handle_batch_response` parsed `tool_call.function.arguments` with a bare
`json.loads`, so a tool call whose arguments string is empty or truncated
raised `json.JSONDecodeError` out of `ChatAgent.step()` before any tool ran.
OpenAI-compatible servers (vLLM, Ollama, Together, ...) send
`arguments: ""` for zero-argument tools and can truncate arguments, so a
parse failure is an expected input rather than an invariant violation. The
streaming accumulators and `_utils.extract_tool_call` already catch
`json.JSONDecodeError` for the same field.
`ToolCallRequest.args` is typed `Dict[str, Any]`, so unlike the streaming
accumulators the batch path cannot keep the raw string; fall back to `{}`
and let the tool call proceed.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| args = json.loads(tool_call.function.arguments) # type: ignore[union-attr] | ||
| except json.JSONDecodeError: | ||
| # `arguments` may be empty or truncated on the wire. | ||
| args = {} |
There was a problem hiding this comment.
I ran this against 5b1cded65d7acda268599fb434f93ae55a4f8822 in a clean container (python:3.12-slim, pip install -e .).
The guard catches a decode error, but arguments is only required to be a string, not an object. A value that parses as valid JSON that is not a dict skips the except and reaches ToolCallRequest(args=args) at :4046-4048, where pydantic raises instead, so it leaves step() the same way the decode error did.
Your test script, same ping tool, varying only the arguments string, at the PR head:
empty string (your case 1) -> OK args={} result='pong'
truncated (your case 2) -> OK args={} result='pong'
valid JSON null -> ValidationError out of step()
valid JSON list -> ValidationError out of step()
valid JSON scalar 123 -> ValidationError out of step()
valid JSON string "ping" -> ValidationError out of step()
normal object {} -> OK args={} result='pong'
The last four behave identically with your change reverted, so the PR does not introduce this.
It does bear on the consistency point in the description. I called _execute_tool_from_stream_data directly with those same five strings, not through a real stream, and it returned a record every time and logged the failure rather than raising. So for this input class the two paths still disagree.
One line if you want it in scope:
if not isinstance(args, dict):
args = {}I have not seen a provider send arguments: "null", so that part is argued from the spec wording you quote rather than observed.
There was a problem hiding this comment.
Confirmed, and fixed in e97d759.
I re-ran your matrix from this PR's own test harness (ChatAgent.step() driven by a stub model), at the head you tested:
empty string -> OK args={} result='pong'
truncated -> OK args={} result='pong'
valid JSON null -> ValidationError out of step()
valid JSON list -> ValidationError out of step()
valid JSON list [1, 2] -> ValidationError out of step()
valid JSON number 123 -> ValidationError out of step()
valid JSON true -> ValidationError out of step()
valid JSON "ping" -> ValidationError out of step()
I also measured the streaming side rather than assuming it, calling _execute_tool_from_stream_data with the same five strings: it returns without raising in every case (null, [], 123, "ping" and "" all come back as a None record after the logged failure). So the two paths disagreed exactly where you said they did.
The guard now covers both classes, since neither payload can fill ToolCallRequest.args:
try:
args = json.loads(tool_call.function.arguments)
except json.JSONDecodeError:
# `arguments` may be empty or truncated on the wire.
args = {}
if not isinstance(args, dict):
# `arguments` may also be valid JSON that is not an
# object (`null`, `[]`, a bare scalar), which `args`
# cannot hold any more than a malformed payload.
args = {}The regression test is now parametrized over ["", '{"city": "San Fra', "null", "[]", "123", '"ping"'], so both the JSONDecodeError path and the non-object path are pinned. test/agents/test_chat_agent.py is 40 passed / 4 failed on this head, with the same 4 failures on master (they need live API credentials), and ruff reports the same three pre-existing RUF059 findings on master and here.
For the record, I agree with the caveat you flagged: I have not seen a provider send arguments: "null" either, so this is argued from the contract rather than observed. The reason I took the one-liner is that the failure mode is asymmetric - a stray non-object payload costs the whole step() today, and the streaming path already made the other choice.
There was a problem hiding this comment.
Thanks, that closes it. At e97d759 ToolCallRequest is built in exactly one place, chat_agent.py:4051, inside the function you guarded, so there is no second path left where a non-object payload can reach args.
A payload that decodes to valid JSON that is not an object (`null`, `[]`, a bare scalar) clears the JSONDecodeError guard but cannot fill `ToolCallRequest.args`, so pydantic raises and the error still escapes `ChatAgent.step()`. Verified against the PR head: `null`, `[]`, `[1, 2]`, `123`, `true` and `"ping"` all raise ValidationError out of step(), while `_execute_tool_from_stream_data` handles every one of them without raising. Extends the regression test to cover both classes.
2 participants
Related Issue
Closes #4338
Description
ChatAgent._handle_batch_responseparsedtool_call.function.argumentswith a barejson.loadsatcamel/agents/chat_agent.py:4039. A tool call whoseargumentsstring is empty or truncated raisedjson.JSONDecodeErrorstraight out ofChatAgent.step(): the tool was never executed, noToolCallingRecordwas recorded, and the caller got a decoder error with no indication that it came from a model tool call.This is reachable in the default (non-streaming) configuration. OpenAI-compatible servers (vLLM, Ollama, Together, and others) send
arguments: ""for zero-argument tools, and the Chat Completions specification typesfunction.argumentsas a plainstringwith no JSON guarantee.Every other site that parses this same field already treats a parse failure as expected input — the streaming accumulators at
camel/agents/chat_agent.py:5049-5051and6047-6049catchjson.JSONDecodeError, andcamel/agents/_utils.py:130-134does too. The batch path was the only unguarded one, which is why sync-non-streaming and sync-streaming disagreed on identical input.What is the purpose of this pull request?
Changes
camel/agents/chat_agent.py: guard the parse in_handle_batch_responsewithtry/except json.JSONDecodeErrorand fall back to{}.ToolCallRequest.argsis typedDict[str, Any], so unlike the streaming accumulators the batch path cannot keep the raw string; with{}a zero-argument tool executes normally, and a tool that requires arguments fails inside the existing_execute_toolhandler (which already converts tool errors into aTool execution failed: ...result) instead of aborting the step.test/agents/test_chat_agent.py: regression test coveringarguments=""and a truncated arguments string.Testing
Red — the new test on
master(unpatched), failing atcamel/agents/chat_agent.py:4039:Green — same test with the fix applied:
Module suite (no regression — the same 4 failures occur on unpatched
master; they need real API keys and network):Lint / format / typecheck:
Checklist
pyproject.tomland runuv lock