Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Hi, @yuzhouo7 Verified by reading the code and reproducing the issue on
Because of this, the old The proposed fix is correct: at that branch, The issue only affects streaming tool calls. Scalar, array, and no-argument cases remain unchanged, while the non-streaming path is unaffected because it rebuilds the arguments from the raw XML. Thx for pointing this issue! |
The finalizer owns the outer `}` of the argument object but skipped it whenever the accumulated text already ended in `}` -- which is also the case when the final argument is itself an object, so those calls emitted invalid JSON. Adds the first tests for this detector; 6 of the 9 fail without the fix.
e8acc44 to
c870488
Compare
|
Closing as superseded: the production fix for the glm47 parser dropping the outer Rebase onto current |
Motivation
_process_xml_to_json_streaming()emits the outer{of the argument object on the first parameter, but it never emits a matching}. Every other brace it writes comes fromjson.dumps()of an argument value. Closing the outer object is therefore solely_finalize_tool_call()'s job.Today the finalizer skips that close whenever the accumulated argument text already ends in
}:That suffix test conflates the close of an object-valued final argument with the close of the outer argument object. When the last argument is itself an object, the trailing
}belongs to that argument, so the finalizer concludes the outer object is already closed and skips it:main{"mode": "fast", "options": {"topic": "t"}— outer}missing, invalid JSON{"mode": "fast", "options": {"topic": "t"}}{"options": {"topic": "t"}, "mode": "fast"}— validAs a result, agent frameworks can reject otherwise-correct tool calls whenever the final argument is object-valued.
Modifications
Drop the suffix heuristic; close the outer object whenever it is still open.
not self._sent_empty_objectis sufficient here. Reaching this branch already excludes the "no parameters seen yet" case, so if_sent_empty_objectis false at this point, at least one parameter has been streamed and the outer object is open. Equivalently, at this branchnot _sent_empty_objectimpliesnot _is_first_param— which is why the latter is not spelled out in the condition.The state invariants this rests on:
_is_first_paramis cleared exactly when the outer{is appended (json_output += "{" if self._is_first_param else ", ")._sent_empty_objectis set only on paths that have already emitted a complete empty{}object, so if it is false in this branch, the non-empty outer object still needs its closing}._reset_streaming_state()resets these flags together after each finalized tool call, so they cannot drift apart between calls.Scope. Only final arguments whose serialization ends in
}are affected — object values, including empty and nested ones. Arrays, numbers and strings (which end in"even when the text inside ends in}) are byte-identical before and after, and the no-argument{}path is untouched.This PR extends the existing
TestGlm47MoeDetectorsuite intest/registered/unit/function_call/test_function_call_parser.py. Three grouped regression tests cover the nine scenarios below; six of them fail onmainand pass with this PR:main{}as final value{}Note: #24147 is open against the same file. It fixes a different bug in a different function, where a closing tag is dropped when a value ends with
<, and does not overlap textually with this change.Accuracy Tests
Not applicable — this changes only the JSON framing of streamed tool-call arguments, never model outputs. No kernel or model forward code is touched.
Speed Tests and Profiling
Not applicable — the change removes one string comparison from a path that runs once per tool call.
Checklist
CI States
Latest PR Test (Base): ❌ Run #30505650747
Latest PR Test (Extra): ❌ Run #30505650600