-
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 4 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,96 @@ 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 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 }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,20 +88,28 @@ export class StreamingToolCallParser { | |||||||||||||||
| const existingDepth = this.depths.get(index)!; | ||||||||||||||||
| const existingMeta = this.toolCallMeta.get(index); | ||||||||||||||||
|
|
||||||||||||||||
| // Check if we have a complete tool call at this index | ||||||||||||||||
| // Check if we have a complete tool call at this index. Occupancy | ||||||||||||||||
| // is signaled by the name metadata, not the buffer: an empty | ||||||||||||||||
| // buffer with a name is a complete no-argument call. | ||||||||||||||||
| if ( | ||||||||||||||||
| existingBuffer.trim() && | ||||||||||||||||
| existingMeta?.name && | ||||||||||||||||
|
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 collision path was correctly updated to use The canonical streaming case (opener with empty args followed by fragments at the same index) is why this path wasn't simply switched to A proper fix would require tracking explicit state per index (e.g., — qwen3.7-max via Qwen Code /review |
||||||||||||||||
| existingDepth === 0 && | ||||||||||||||||
| existingMeta?.id && | ||||||||||||||||
| existingMeta.id !== id | ||||||||||||||||
| ) { | ||||||||||||||||
| try { | ||||||||||||||||
| JSON.parse(existingBuffer); | ||||||||||||||||
| let existingComplete = true; | ||||||||||||||||
| if (existingBuffer.trim()) { | ||||||||||||||||
| try { | ||||||||||||||||
| JSON.parse(existingBuffer); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // Existing buffer is not complete JSON, we can reuse this index | ||||||||||||||||
| existingComplete = false; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| if (existingComplete) { | ||||||||||||||||
| // We have a complete tool call with a different ID at this index | ||||||||||||||||
| // Find a new index for this tool call | ||||||||||||||||
| actualIndex = this.findNextAvailableIndex(); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // Existing buffer is not complete JSON, we can reuse this index | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -244,8 +252,10 @@ export class StreamingToolCallParser { | |||||||||||||||
| * 2. Auto-close unclosed strings and retry | ||||||||||||||||
| * 3. Fallback to safeJsonParse for malformed data | ||||||||||||||||
| * | ||||||||||||||||
| * Only returns tool calls with both name metadata and non-empty buffers. | ||||||||||||||||
| * Should be called when streaming is complete (finish_reason is present). | ||||||||||||||||
| * Only returns tool calls with name metadata. An empty buffer yields | ||||||||||||||||
| * empty arguments ({}), matching the non-streaming path for no-argument | ||||||||||||||||
| * tools. Should be called when streaming is complete (finish_reason is | ||||||||||||||||
| * present). | ||||||||||||||||
| * | ||||||||||||||||
| * @returns Array of completed tool calls with their metadata and parsed arguments | ||||||||||||||||
| */ | ||||||||||||||||
|
|
@@ -265,7 +275,7 @@ export class StreamingToolCallParser { | |||||||||||||||
|
|
||||||||||||||||
| for (const [index, buffer] of this.buffers.entries()) { | ||||||||||||||||
| const meta = this.toolCallMeta.get(index); | ||||||||||||||||
| if (meta?.name && buffer.trim()) { | ||||||||||||||||
| if (meta?.name) { | ||||||||||||||||
|
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] The JSDoc for
Suggested change
— qwen3.7-max via Qwen Code /review
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] The guard relaxation here (
None of these paths are covered by the new tests (they require multi-tool-call interleaving). The fix for all three is the same — replace — qwen3.7-max via Qwen Code /review |
||||||||||||||||
| if (meta.id) { | ||||||||||||||||
| if (emittedIds.has(meta.id)) { | ||||||||||||||||
| continue; | ||||||||||||||||
|
|
@@ -275,22 +285,45 @@ export class StreamingToolCallParser { | |||||||||||||||
|
|
||||||||||||||||
|
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. [Critical] The replay-detection guard at line 157 ( No existing test covers this scenario (the two replay-guard tests at lines ~629 and ~656 both use calls with non-empty argument buffers). Extend the replay guard to treat an empty buffer with name metadata as complete:
Suggested change
Inside the guard, the if (isKnownId && currentDepth === 0 && (currentBuffer.trim() || this.toolCallMeta.get(actualIndex)?.name)) {
let isComplete = !currentBuffer.trim(); // empty buffer + name = complete no-arg call
if (!isComplete) {
try {
JSON.parse(currentBuffer);
isComplete = true;
} catch {
// Not complete yet; append the incoming chunk below.
}
}
if (isComplete) {
debugLogger.debug(
`Ignoring replay chunk for completed toolCall id=${id}`,
);
return { complete: false };
}
}Add a regression test: it('should ignore replay chunks for a completed no-argument tool call', () => {
parser.addChunk(0, '', 'call_1', 'list_sessions');
// Provider replays the same ID with a different name
parser.addChunk(0, '', 'call_1', 'different_function');
const completed = parser.getCompletedToolCalls();
expect(completed).toHaveLength(1);
expect(completed[0].name).toBe('list_sessions');
});— qwen3.7-max via Qwen Code /review |
||||||||||||||||
| let args: Record<string, unknown> = {}; | ||||||||||||||||
|
|
||||||||||||||||
| // Try to parse the final buffer | ||||||||||||||||
| try { | ||||||||||||||||
| args = JSON.parse(buffer); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // Try with repair (auto-close strings) | ||||||||||||||||
| const inString = this.inStrings.get(index); | ||||||||||||||||
| if (inString) { | ||||||||||||||||
| try { | ||||||||||||||||
| args = JSON.parse(buffer + '"'); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // If all parsing fails, use safeJsonParse as fallback | ||||||||||||||||
| // An empty buffer is a legal end state: for no-argument tools some | ||||||||||||||||
| // providers stream `arguments: ""` (or omit the field entirely) and | ||||||||||||||||
| // never send an argument fragment. Keep args as {} to match the | ||||||||||||||||
| // non-streaming path in converter.ts. | ||||||||||||||||
| if (buffer.trim()) { | ||||||||||||||||
| // Try to parse the final buffer | ||||||||||||||||
| try { | ||||||||||||||||
| args = JSON.parse(buffer); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // Try with repair (auto-close strings) | ||||||||||||||||
| const inString = this.inStrings.get(index); | ||||||||||||||||
| if (inString) { | ||||||||||||||||
| try { | ||||||||||||||||
| args = JSON.parse(buffer + '"'); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // If all parsing fails, use safeJsonParse as fallback | ||||||||||||||||
| args = safeJsonParse(buffer, {}); | ||||||||||||||||
| } | ||||||||||||||||
| } else { | ||||||||||||||||
| args = safeJsonParse(buffer, {}); | ||||||||||||||||
| } | ||||||||||||||||
| } else { | ||||||||||||||||
| args = safeJsonParse(buffer, {}); | ||||||||||||||||
| } | ||||||||||||||||
| // Tool arguments are always JSON objects; a corrupted buffer can | ||||||||||||||||
| // parse or repair to a non-object value (e.g. a bare string), so | ||||||||||||||||
| // collapse anything else to {}. | ||||||||||||||||
| if ( | ||||||||||||||||
| typeof args !== 'object' || | ||||||||||||||||
| args === null || | ||||||||||||||||
| Array.isArray(args) | ||||||||||||||||
|
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] The non-object collapse guard has three branches (
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||||||||||||
| ) { | ||||||||||||||||
| debugLogger.debug( | ||||||||||||||||
| `Collapsing non-object arguments for tool call ${meta.name} (id=${meta.id}) at index ${index}; buffer likely polluted by a misrouted fragment`, | ||||||||||||||||
| ); | ||||||||||||||||
| args = {}; | ||||||||||||||||
| } | ||||||||||||||||
| } else { | ||||||||||||||||
| debugLogger.debug( | ||||||||||||||||
| `Emitting no-argument tool call ${meta.name} (id=${meta.id}) at index ${index} with empty buffer`, | ||||||||||||||||
| ); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| completed.push({ | ||||||||||||||||
|
|
@@ -309,8 +342,9 @@ export class StreamingToolCallParser { | |||||||||||||||
| * Finds the next available index for a new tool call | ||||||||||||||||
| * | ||||||||||||||||
| * Scans indices starting from nextAvailableIndex to find one that's safe to use. | ||||||||||||||||
| * Reuses indices with empty buffers or incomplete parsing states. | ||||||||||||||||
| * Skips indices with complete, parseable tool call data to prevent overwriting. | ||||||||||||||||
| * Reuses indices never claimed by a named tool call or with incomplete | ||||||||||||||||
| * parsing states. Skips indices with complete tool call data — including | ||||||||||||||||
| * no-argument calls with empty buffers — to prevent overwriting. | ||||||||||||||||
| * | ||||||||||||||||
| * @returns The next available index safe for storing a new tool call | ||||||||||||||||
| */ | ||||||||||||||||
|
|
@@ -321,22 +355,23 @@ export class StreamingToolCallParser { | |||||||||||||||
| const depth = this.depths.get(this.nextAvailableIndex)!; | ||||||||||||||||
| const meta = this.toolCallMeta.get(this.nextAvailableIndex); | ||||||||||||||||
|
|
||||||||||||||||
| // If buffer is empty or incomplete (depth > 0), this index is available | ||||||||||||||||
| if (!buffer.trim() || depth > 0 || !meta?.id) { | ||||||||||||||||
| // If the slot was never claimed by a named call or is incomplete | ||||||||||||||||
| // (depth > 0), this index is available. An empty buffer alone does | ||||||||||||||||
| // not mean available: with name metadata it is a complete | ||||||||||||||||
| // no-argument call. | ||||||||||||||||
| if (!meta?.name || depth > 0 || !meta?.id) { | ||||||||||||||||
| return this.nextAvailableIndex; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Try to parse the buffer to see if it's complete | ||||||||||||||||
| try { | ||||||||||||||||
| JSON.parse(buffer); | ||||||||||||||||
| // If parsing succeeds and depth is 0, this index has a complete tool call | ||||||||||||||||
| if (depth === 0) { | ||||||||||||||||
| this.nextAvailableIndex++; | ||||||||||||||||
| continue; | ||||||||||||||||
| // A non-empty buffer must parse as complete JSON for the slot to be | ||||||||||||||||
| // occupied; an empty buffer is already complete (no-argument call) | ||||||||||||||||
| if (buffer.trim()) { | ||||||||||||||||
| try { | ||||||||||||||||
|
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] The scanning loop's empty-buffer skip path (lines 384–393) is never tested with a scan that must advance past an occupied empty-buffer slot. Existing tests only trigger it('should scan past occupied no-argument slots when relocating', () => {
parser.addChunk(0, '', 'call_a', 'no_arg_a');
parser.addChunk(1, '', 'call_b', 'no_arg_b');
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 },
]);
});— qwen3.7-max via Qwen Code /review |
||||||||||||||||
| JSON.parse(buffer); | ||||||||||||||||
| } catch { | ||||||||||||||||
| // If parsing fails, this index is available for reuse | ||||||||||||||||
| return this.nextAvailableIndex; | ||||||||||||||||
| } | ||||||||||||||||
| } catch { | ||||||||||||||||
| // If parsing fails, this index is available for reuse | ||||||||||||||||
| return this.nextAvailableIndex; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| this.nextAvailableIndex++; | ||||||||||||||||
|
|
@@ -348,8 +383,9 @@ export class StreamingToolCallParser { | |||||||||||||||
| * Finds the most recent incomplete tool call index | ||||||||||||||||
| * | ||||||||||||||||
| * Used when continuation chunks arrive without IDs. Scans existing tool calls | ||||||||||||||||
| * to find the highest index with incomplete parsing state (depth > 0, empty buffer, | ||||||||||||||||
| * or unparseable JSON). Falls back to creating a new index if none found. | ||||||||||||||||
| * to find the highest index with incomplete parsing state (depth > 0, empty | ||||||||||||||||
| * buffer with no name metadata yet, or unparseable JSON). Falls back to | ||||||||||||||||
| * creating a new index if none found. | ||||||||||||||||
| * | ||||||||||||||||
| * @returns The index of the most recent incomplete tool call, or a new available index | ||||||||||||||||
| */ | ||||||||||||||||
|
|
@@ -360,8 +396,10 @@ export class StreamingToolCallParser { | |||||||||||||||
| const depth = this.depths.get(index)!; | ||||||||||||||||
| const meta = this.toolCallMeta.get(index); | ||||||||||||||||
|
|
||||||||||||||||
| // Check if this tool call is incomplete | ||||||||||||||||
| if (meta?.id && (depth > 0 || !buffer.trim())) { | ||||||||||||||||
| // Check if this tool call is incomplete. An empty buffer counts only | ||||||||||||||||
| // while no name has arrived: with name metadata it is a complete | ||||||||||||||||
| // no-argument call, and stray chunks must not be appended to it | ||||||||||||||||
| if (meta?.id && (depth > 0 || (!buffer.trim() && !meta?.name))) { | ||||||||||||||||
|
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. [Critical] Same The PR correctly updated three of four sites that used // Line 130 (unchanged by this PR, not in diff):
if (existingDepth > 0 || !existingBuffer.trim()) {
actualIndex = index;A completed no-argument call has The canonical-shape test ( Suggested fix (at line 130): const existingMeta = this.toolCallMeta.get(index);
if (existingDepth > 0 || (!existingBuffer.trim() && !existingMeta?.name)) {
actualIndex = index;— qwen3.7-max via Qwen Code /review |
||||||||||||||||
| maxIndex = Math.max(maxIndex, index); | ||||||||||||||||
| } else if (buffer.trim()) { | ||||||||||||||||
| // Check if buffer is parseable (complete) | ||||||||||||||||
|
|
||||||||||||||||
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