-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(agents): tolerate unparsable tool call arguments #4339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
BlueX888
wants to merge
2
commits into
camel-ai:master
Choose a base branch
from
BlueX888:fix/prep-nonstream-empty-tool-arguments-jsondecodeerror
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+82
−1
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this against
5b1cded65d7acda268599fb434f93ae55a4f8822in a clean container (python:3.12-slim,pip install -e .).The guard catches a decode error, but
argumentsis 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 reachesToolCallRequest(args=args)at :4046-4048, where pydantic raises instead, so it leavesstep()the same way the decode error did.Your test script, same
pingtool, varying only theargumentsstring, at the PR head: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_datadirectly 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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:I also measured the streaming side rather than assuming it, calling
_execute_tool_from_stream_datawith the same five strings: it returns without raising in every case (null,[],123,"ping"and""all come back as aNonerecord 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:The regression test is now parametrized over
["", '{"city": "San Fra', "null", "[]", "123", '"ping"'], so both theJSONDecodeErrorpath and the non-object path are pinned.test/agents/test_chat_agent.pyis 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 wholestep()today, and the streaming path already made the other choice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that closes it. At e97d759
ToolCallRequestis 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 reachargs.