Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions packages/core/src/core/openaiContentGenerator/converter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,64 @@ describe('OpenAIContentConverter', () => {
expect(fnB?.args).toEqual({ file_path: '/b/y.ts' });
expect(fnB?.id).toBe('call_B');
});

it('emits no-argument tool calls that stream an empty arguments string', () => {
// Providers may finish a no-argument tool call with `arguments: ""`
// and no follow-up fragment (e.g. llama.cpp-style servers). The call
// must reach the caller with empty args instead of being dropped,
// which would make the whole turn look empty and trigger retries.
const stream = withStreamParser(new StreamingToolCallParser());

const opener = {
object: 'chat.completion.chunk',
id: 'noargs-open',
created: 1,
model: 'test',
choices: [
{
index: 0,
delta: {
tool_calls: [
{
index: 0,
id: 'call_noargs',
type: 'function' as const,
function: { name: 'list_sessions', arguments: '' },
},
],
},
finish_reason: null,
logprobs: null,
},
],
} as unknown as OpenAI.Chat.ChatCompletionChunk;

const finisher = {
object: 'chat.completion.chunk',
id: 'noargs-finish',
created: 2,
model: 'test',
choices: [
{
index: 0,
delta: {},
finish_reason: 'tool_calls',
logprobs: null,
},
],
} as unknown as OpenAI.Chat.ChatCompletionChunk;

converter.convertOpenAIChunkToGemini(opener, stream);
const result = converter.convertOpenAIChunkToGemini(finisher, stream);

const fn = result.candidates?.[0]?.content?.parts?.find(
(p: Part) => p.functionCall,
)?.functionCall;

expect(fn?.name).toBe('list_sessions');
expect(fn?.args).toEqual({});
expect(fn?.id).toBe('call_noargs');
});
});

describe('convertGeminiRequestToOpenAI', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,11 +391,25 @@ 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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 expect(completed).toEqual([{ id: 'call_1', name: 'function1', args: {}, index: 0 }]) (full shape), while this one only checks toHaveLength(1) and completed[0].args. It would not catch metadata corruption or wrong-index routing.

Suggested change
expect(completed).toHaveLength(1);
it('should return empty args for whitespace-only argument buffers', () => {
parser.addChunk(0, ' ', 'call_1', 'function1');
const completed = parser.getCompletedToolCalls();
expect(completed).toEqual([
{ id: 'call_1', name: 'function1', args: {}, index: 0 },
]);
});

— qwen3.7-max via Qwen Code /review

expect(completed[0].args).toEqual({});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The guard relaxation here (meta?.name only) correctly preserves empty-buffer no-arg tool calls at emission time, but three buffer.trim() checks in unchanged addChunk() code still use "has buffer content" as a proxy for "this slot is occupied by a real tool call." Now that empty buffer is a valid terminal state, those proxies break in multi-tool-call scenarios:

  1. Line 93 (collision detection): existingBuffer.trim() is falsy for an empty-buffer entry, so when a second tool call arrives at the same index, the collision guard is skipped and the first call's metadata is silently overwritten.
  2. Line 149 (replay guard): currentBuffer.trim() is falsy, so a replayed chunk for a completed no-arg call bypasses the guard and corrupts the buffer.
  3. Lines 121 & 345 (continuation routing + findMostRecentIncompleteIndex): !existingBuffer.trim() classifies a completed no-arg call as "incomplete," so continuation chunks from other tool calls get misrouted to it.

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 buffer.trim() with a completion signal that accounts for empty-buffer entries, e.g. meta?.name && meta?.id && depth === 0.

— qwen3.7-max via Qwen Code /review

if (meta.id) {
if (emittedIds.has(meta.id)) {
continue;
Expand All @@ -275,21 +275,27 @@ export class StreamingToolCallParser {

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, {});
}
}

Expand Down
Loading