-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(core): preserve no-argument tool calls that stream an empty arguments string #6250
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
Changes from all commits
2a02b5d
0d2b91d
a5db830
f1efd7f
096eea5
852e763
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -391,11 +391,124 @@ describe('StreamingToolCallParser', () => { | |||||||||||||||||||
| expect(completed).toHaveLength(0); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should not return tool calls with empty buffer', () => { | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'function1'); // Empty buffer | ||||||||||||||||||||
| it('should return no-argument tool calls with empty args when buffer is empty', () => { | ||||||||||||||||||||
| // For tools without parameters, some providers stream | ||||||||||||||||||||
| // `arguments: ""` (or omit the field) and never send an argument | ||||||||||||||||||||
| // fragment. The call must survive with empty args, matching the | ||||||||||||||||||||
| // non-streaming path. | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'function1'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toHaveLength(0); | ||||||||||||||||||||
| expect(completed).toEqual([ | ||||||||||||||||||||
| { id: 'call_1', name: 'function1', args: {}, index: 0 }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should return empty args for whitespace-only argument buffers', () => { | ||||||||||||||||||||
| parser.addChunk(0, ' ', 'call_1', 'function1'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toHaveLength(1); | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This test uses weaker assertions than the empty-buffer test above (line 398). The empty-buffer test uses
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||||||||||||||||
| expect(completed[0].args).toEqual({}); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should not overwrite a completed no-argument tool call when a new call reuses its index', () => { | ||||||||||||||||||||
| // First tool call: no arguments, provider never sends a fragment | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'no_arg_function'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Second tool call arrives at the same index with a different ID | ||||||||||||||||||||
| parser.addChunk(0, '{"param": "value"}', 'call_2', 'function2'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Both calls must survive: the second is relocated to a new index | ||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toEqual([ | ||||||||||||||||||||
| { id: 'call_1', name: 'no_arg_function', args: {}, index: 0 }, | ||||||||||||||||||||
| { | ||||||||||||||||||||
| id: 'call_2', | ||||||||||||||||||||
| name: 'function2', | ||||||||||||||||||||
| args: { param: 'value' }, | ||||||||||||||||||||
| index: 1, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should route ID-less argument fragments to a call whose opener streamed empty arguments', () => { | ||||||||||||||||||||
| // Canonical OpenAI-compatible streaming shape: the opener carries | ||||||||||||||||||||
| // id + name + `arguments: ""`, then argument fragments follow at the | ||||||||||||||||||||
| // same index without an ID. Mid-stream, an empty buffer with name | ||||||||||||||||||||
| // metadata must therefore stay continuable at its own index — it is | ||||||||||||||||||||
| // indistinguishable from a completed no-argument call until stream end. | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'function1'); | ||||||||||||||||||||
| parser.addChunk(0, '{"x":'); | ||||||||||||||||||||
| parser.addChunk(0, '1}'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toEqual([ | ||||||||||||||||||||
| { id: 'call_1', name: 'function1', args: { x: 1 }, index: 0 }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should emit empty args for a no-argument call polluted by a stray fragment at its index', () => { | ||||||||||||||||||||
| // If a misbehaving provider reuses a completed no-argument call's | ||||||||||||||||||||
| // index for another call's ID-less fragment, the fragment cannot be | ||||||||||||||||||||
| // re-routed (see canonical-shape test above). The damage must stay | ||||||||||||||||||||
| // bounded: the polluted buffer repairs to a non-object value, which | ||||||||||||||||||||
| // collapses to {} at emit time. | ||||||||||||||||||||
| parser.addChunk(0, '{"key":', 'call_1', 'function1'); | ||||||||||||||||||||
| parser.addChunk(1, '', 'call_2', 'no_arg_function'); | ||||||||||||||||||||
| parser.addChunk(1, '"value"}'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| const noArg = completed.find((c) => c.id === 'call_2'); | ||||||||||||||||||||
| expect(noArg?.args).toEqual({}); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should collapse null argument buffers to empty args', () => { | ||||||||||||||||||||
| parser.addChunk(0, 'null', 'call_1', 'function1'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed[0].args).toEqual({}); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should collapse array argument buffers to empty args', () => { | ||||||||||||||||||||
| parser.addChunk(0, '[1,2,3]', 'call_1', 'function1'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed[0].args).toEqual({}); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should scan past occupied no-argument slots when relocating a colliding call', () => { | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_a', 'no_arg_a'); | ||||||||||||||||||||
| parser.addChunk(1, '', 'call_b', 'no_arg_b'); | ||||||||||||||||||||
| // Collision at index 0 must relocate past both occupied no-arg slots | ||||||||||||||||||||
| parser.addChunk(0, '{"x": 1}', 'call_c', 'fn_c'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toEqual([ | ||||||||||||||||||||
| { id: 'call_a', name: 'no_arg_a', args: {}, index: 0 }, | ||||||||||||||||||||
| { id: 'call_b', name: 'no_arg_b', args: {}, index: 1 }, | ||||||||||||||||||||
| { id: 'call_c', name: 'fn_c', args: { x: 1 }, index: 2 }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should not route continuation chunks to a completed no-argument tool call', () => { | ||||||||||||||||||||
| // Incomplete tool call accumulating arguments at index 0 | ||||||||||||||||||||
| parser.addChunk(0, '{"key":', 'call_1', 'function1'); | ||||||||||||||||||||
| // Completed no-argument tool call at the higher index 1 | ||||||||||||||||||||
| parser.addChunk(1, '', 'call_2', 'no_arg_function'); | ||||||||||||||||||||
| // Completed tool call at index 2 | ||||||||||||||||||||
| parser.addChunk(2, '{"x": 1}', 'call_3', 'function3'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Continuation chunk without an ID arriving at a completed index must | ||||||||||||||||||||
| // be routed to the incomplete call_1, not to the no-argument call_2 | ||||||||||||||||||||
| parser.addChunk(2, '"value"}'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toEqual([ | ||||||||||||||||||||
| { id: 'call_1', name: 'function1', args: { key: 'value' }, index: 0 }, | ||||||||||||||||||||
| { id: 'call_2', name: 'no_arg_function', args: {}, index: 1 }, | ||||||||||||||||||||
| { id: 'call_3', name: 'function3', args: { x: 1 }, index: 2 }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -568,6 +681,31 @@ describe('StreamingToolCallParser', () => { | |||||||||||||||||||
| expect(parser.getBuffer(0)).toBe('{"param1": "value1"}'); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should ignore replayed openers for a completed no-argument tool call', () => { | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'list_sessions'); | ||||||||||||||||||||
| // Provider replays the same ID's opener with a different name; the | ||||||||||||||||||||
| // surviving call must not be mutated | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'different_function'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const completed = parser.getCompletedToolCalls(); | ||||||||||||||||||||
| expect(completed).toHaveLength(1); | ||||||||||||||||||||
| expect(completed[0].name).toBe('list_sessions'); | ||||||||||||||||||||
| expect(completed[0].args).toEqual({}); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should append ID-bearing argument fragments after an empty opener', () => { | ||||||||||||||||||||
| // Some providers repeat the tool call ID on argument fragments. A | ||||||||||||||||||||
| // known-ID chunk carrying argument content is a continuation, not a | ||||||||||||||||||||
| // replay, and must not be swallowed by the replay guard. | ||||||||||||||||||||
| parser.addChunk(0, '', 'call_1', 'function1'); | ||||||||||||||||||||
| const result = parser.addChunk(0, '{"x":1}', 'call_1'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.complete).toBe(true); | ||||||||||||||||||||
| expect(parser.getCompletedToolCalls()).toEqual([ | ||||||||||||||||||||
| { id: 'call_1', name: 'function1', args: { x: 1 }, index: 0 }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('should ignore metadata-only replay chunks after a tool call ID completes', () => { | ||||||||||||||||||||
| parser.addChunk(0, '{"file_path": "a.ts"}', 'call_1', 'read_file'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
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.
[Suggestion] No test covers a no-argument tool call coexisting with a regular (argument-bearing) tool call in the same parser instance. This mixed case is where the internal routing/collision heuristics (index-collision guard, continuation-chunk routing,
findMostRecentIncompleteIndex) are most likely to surface edge cases. Consider adding a test like:— qwen3.7-max via Qwen Code /review