Skip to content
Closed
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
256 changes: 256 additions & 0 deletions packages/core/src/core/openaiContentGenerator/converter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ import {
import type OpenAI from 'openai';
import { convertToFunctionResponse } from '../coreToolScheduler.js';

function legacyFunctionCallPart(
name = 'read_file',
args: Record<string, unknown> = { path: 'README.md' },
) {
return {
functionCall: {
id: expect.any(String),
name,
args,
},
};
}

describe('OpenAIContentConverter', () => {
let converter: typeof OpenAIContentConverter;
let requestContext: RequestContext;
Expand Down Expand Up @@ -2677,6 +2690,13 @@ describe('OpenAIContentConverter', () => {
});

describe('convertOpenAIResponseToGemini', () => {
type LegacyFunctionCallMessage = Omit<
Partial<OpenAI.Chat.ChatCompletionMessage>,
'function_call'
> & {
function_call?: { name: string; arguments?: string };
};

it('should handle empty choices array without crashing', () => {
const response = converter.convertOpenAIResponseToGemini(
{
Expand All @@ -2692,6 +2712,105 @@ describe('OpenAIContentConverter', () => {
expect(response.candidates).toEqual([]);
});

Comment thread
VectorPeak marked this conversation as resolved.
Resolved
function convertLegacyFunctionCall(message: LegacyFunctionCallMessage) {
return converter.convertOpenAIResponseToGemini(
{
object: 'chat.completion',
id: 'chatcmpl-legacy-function-call',
created: 123,
model: 'test-model',
choices: [
{
index: 0,
message: { role: 'assistant', content: null, ...message },
finish_reason: 'function_call',
logprobs: null,
},
],
} as unknown as OpenAI.Chat.ChatCompletion,
requestContext,
);
}

it.each([
{
name: 'legacy function_call responses',
message: {
function_call: {
name: 'read_file',
arguments: '{"path":"README.md"}',
},
},
expectedParts: [legacyFunctionCallPart()],
},
{
name: 'legacy function_call when tool_calls is empty',
message: {
tool_calls: [],
function_call: {
name: 'read_file',
arguments: '{"path":"README.md"}',
},
},
expectedParts: [legacyFunctionCallPart()],
},
{
name: 'modern tool_calls over legacy function_call',
message: {
tool_calls: [
{
id: 'call_modern',
type: 'function' as const,
function: {
name: 'read_file',
arguments: '{"path":"modern.md"}',
},
},
],
function_call: {
name: 'read_file',
arguments: '{"path":"legacy.md"}',
},
},
expectedParts: [
legacyFunctionCallPart('read_file', { path: 'modern.md' }),
],
},
{
name: 'invalid legacy function_call arguments',
message: {
function_call: { name: 'read_file', arguments: 'not-json' },
},
expectedParts: [legacyFunctionCallPart('read_file', {})],
},
{
name: 'legacy function_call without arguments',
message: {
function_call: { name: 'ping' },
},
expectedParts: [legacyFunctionCallPart('ping', {})],
},
{
name: 'text content plus legacy function_call',
message: {
content: 'Let me read that file.',
function_call: {
name: 'read_file',
arguments: '{"path":"README.md"}',
},
},
expectedParts: [
{ text: 'Let me read that file.' },
legacyFunctionCallPart(),
],
},
])('should convert $name', ({ message, expectedParts }) => {
const response = convertLegacyFunctionCall(message);

expect(response.candidates?.[0]?.content?.parts).toEqual(expectedParts);
expect(response.candidates?.[0]?.finishReason).toBe(FinishReason.STOP);
});

it('keeps the estimated prompt/completion split summing to total tokens', () => {
// When a provider reports only total_tokens, the 70/30 estimate must

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in dfde5a0. I added explicit non-streaming and streaming regression coverage for the precedence invariant: when modern tool_calls is non-empty and legacy function_call is also present, only the modern tool_calls payload is converted.

// still add back up to the total instead of rounding each half on its
Expand Down Expand Up @@ -5205,6 +5324,143 @@ describe('Truncated tool call detection in streaming', () => {

expect(result.candidates?.[0]?.finishReason).toBe(FinishReason.MAX_TOKENS);
});

function feedLegacyFunctionCallChunks({
chunks,
finishReason = 'function_call',
includeEmptyToolCallsOnFirstChunk = false,
includeModernToolCallOnFirstChunk = false,
}: {
chunks: Array<{ name?: string; arguments?: string }>;
finishReason?: string;
includeEmptyToolCallsOnFirstChunk?: boolean;
includeModernToolCallOnFirstChunk?: boolean;
}) {
const ctx = createStreamingRequestContext();

for (const [index, functionCall] of chunks.entries()) {
converter.convertOpenAIChunkToGemini(
{
object: 'chat.completion.chunk',
id: `legacy-c${index + 1}`,
created: 100,
model: 'test-model',
choices: [
{
index: 0,
delta: {
...(index === 0 && includeEmptyToolCallsOnFirstChunk
? { tool_calls: [] }
: {}),
...(index === 0 && includeModernToolCallOnFirstChunk
? {
tool_calls: [
{
index: 0,
id: 'call_modern',
type: 'function' as const,
function: {
name: 'read_file',
arguments: '{"path":"modern.md"}',
},
},
],
}
: {}),
function_call: functionCall,
},
finish_reason: null,
logprobs: null,
},
],
} as unknown as OpenAI.Chat.ChatCompletionChunk,
ctx,
);
}

return converter.convertOpenAIChunkToGemini(
{
object: 'chat.completion.chunk',
id: 'legacy-final',
created: 101,
model: 'test-model',
choices: [
{
index: 0,
delta: {},
finish_reason: finishReason,
logprobs: null,
},
],
} as unknown as OpenAI.Chat.ChatCompletionChunk,
ctx,
);
}

it.each([
{
name: 'legacy streaming function_call chunks',
chunks: [
{ name: 'read_file', arguments: '{"path"' },
{ arguments: ':"README.md"}' },
],
expectedFinishReason: FinishReason.STOP,
expectedParts: [legacyFunctionCallPart()],
},
{
name: 'legacy streaming function_call when tool_calls is empty',
chunks: [
{ name: 'read_file', arguments: '{"path"' },
{ arguments: ':"README.md"}' },
],
includeEmptyToolCallsOnFirstChunk: true,
expectedFinishReason: FinishReason.STOP,
expectedParts: [legacyFunctionCallPart()],
},
{
name: 'modern streaming tool_calls over legacy function_call',
chunks: [{ name: 'read_file', arguments: '{"path":"legacy.md"}' }],
includeModernToolCallOnFirstChunk: true,
finishReason: 'tool_calls',
expectedFinishReason: FinishReason.STOP,
expectedParts: [
legacyFunctionCallPart('read_file', { path: 'modern.md' }),
],
},
{
name: 'truncated legacy streaming function_call',
chunks: [{ name: 'read_file', arguments: '{"path"' }],
expectedFinishReason: FinishReason.MAX_TOKENS,
},
{
name: 'legacy streaming function_call without arguments',
chunks: [{ name: 'ping' }],
expectedFinishReason: FinishReason.STOP,
expectedParts: [legacyFunctionCallPart('ping', {})],
},
])(
'should convert $name',
({
chunks,
includeEmptyToolCallsOnFirstChunk,
includeModernToolCallOnFirstChunk,
finishReason,
expectedFinishReason,
expectedParts,
}) => {
const result = feedLegacyFunctionCallChunks({
chunks,
finishReason,
includeEmptyToolCallsOnFirstChunk,
includeModernToolCallOnFirstChunk,
});

expect(result.candidates?.[0]?.finishReason).toBe(expectedFinishReason);
if (expectedParts) {
expect(result.candidates?.[0]?.content?.parts).toEqual(expectedParts);
}
},
);
});

describe('mapGeminiFinishReasonToOpenAI', () => {
Expand Down
81 changes: 67 additions & 14 deletions packages/core/src/core/openaiContentGenerator/converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,33 @@ function convertOpenAITextToParts(
return parseTaggedThinkingText(text);
}

function parseToolCallArgs(argsJson?: string | null): Record<string, unknown> {
const parsed = safeJsonParse<unknown>(argsJson ?? '', {});
return parsed && typeof parsed === 'object' && !Array.isArray(parsed)
? (parsed as Record<string, unknown>)
: {};
}

function createFunctionCallPart(
name: string,
argsJson?: string | null,
id = `call_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4129bad4f. I extracted the repeated fallback ID expression into generateToolCallId() and now use it from both createFunctionCallPart() and the streaming emission fallback, so non-streaming and streaming legacy paths stay in sync if the generated ID format changes later.

): Part {
return {
functionCall: {
id,
name,
args: parseToolCallArgs(argsJson),
},
};
}

function getLegacyFunctionCallChunk(
functionCall: OpenAI.Chat.ChatCompletionChunk.Choice.Delta.FunctionCall,
): string {
return functionCall.arguments ?? (functionCall.name ? '{}' : '');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the careful catch. Addressed in 4129bad4f: getLegacyFunctionCallChunk() now appends only real argument deltas (functionCall.arguments ?? ''), so a name-only opener no longer corrupts later argument chunks into {} + payload.

I also updated the streaming regression matrix to use the realistic shape: first chunk carries only name, followed by argument-only chunks. Zero-argument legacy streaming calls are preserved separately at finalization, so we avoid pre-filling the shared parser buffer while still emitting {} for true no-argument calls.

Validation:

wsl -e bash -lc 'cd /mnt/d/ZXY/Github/qwen-code && npm exec -- vitest run src/core/openaiContentGenerator/converter.test.ts'
# 146 passed

wsl -e bash -lc 'cd /mnt/d/ZXY/Github/qwen-code && npm exec -- vitest run src/core/openaiContentGenerator/streamingToolCallParser.test.ts'
# 60 passed

wsl -e bash -lc 'cd /mnt/d/ZXY/Github/qwen-code && npm run build --workspace=packages/core'
# passed

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.

[Critical] getLegacyFunctionCallChunk() still injects '{}' when the first legacy streaming chunk carries only function_call.name. Real OpenAI-compatible streams commonly send the name in one delta and arguments in later deltas; with the current code the parser buffer becomes '{}{"path":"README.md"}', parsing falls back to {}, and the tool executes with empty arguments instead of the streamed payload. The new tests cover name+arguments in the same chunk and name-only with no later arguments, but not the name-only opener followed by argument chunks.

Suggested change
return functionCall.arguments ?? (functionCall.name ? '{}' : '');
return functionCall.arguments ?? '';

-- GPT-5 via Codex /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4129bad4f. The first legacy streaming test case now uses the real split shape: a name-only opener followed by argument-only chunks. This specifically verifies that the parser buffer receives '' for metadata-only legacy chunks and then accumulates the real arguments without corrupting them.

}

/**
* Convert OpenAI response to Gemini format.
*/
Expand Down Expand Up @@ -1115,23 +1142,32 @@ export function convertOpenAIResponseToGemini(
}

// Handle tool calls
if (choice.message.tool_calls) {
if (choice.message.tool_calls?.length) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d0ced77. I kept modern tool_calls as the authoritative path when non-empty, and added a debug log for the mixed tool_calls + legacy function_call case so the ignored legacy field is visible during troubleshooting without changing conversion semantics.

if (choice.message.function_call) {
debugLogger.debug(
`Ignoring legacy function_call "${choice.message.function_call.name}" because tool_calls is non-empty`,
);
}

for (const toolCall of choice.message.tool_calls) {
if (toolCall.function) {
let args: Record<string, unknown> = {};
if (toolCall.function.arguments) {
args = safeJsonParse(toolCall.function.arguments, {});
}

parts.push({
functionCall: {
id: toolCall.id,
name: toolCall.function.name,
args,
},
});
parts.push(
createFunctionCallPart(
toolCall.function.name,
toolCall.function.arguments,
toolCall.id,
),
);
}
}
} else if (choice.message.function_call) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e230917. I changed both modern tool-call guards to require a non-empty tool_calls array, so an empty tool_calls: [] no longer blocks the legacy function_call fallback. I also added regression coverage for both non-streaming and streaming empty-array cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d0ced77. I added narrow debug logs at both legacy fallback entry points: non-streaming message.function_call and streaming delta.function_call. The fallback behavior remains unchanged apart from the diagnostic logging.

const functionCall = choice.message.function_call;
debugLogger.debug(
`Using legacy function_call fallback (non-streaming): ${functionCall.name}`,
);
parts.push(
createFunctionCallPart(functionCall.name, functionCall.arguments),
);
}

response.candidates = [
Expand Down Expand Up @@ -1275,7 +1311,13 @@ export function convertOpenAIChunkToGemini(
}

// Handle tool calls using the stream-local parser
if (choice.delta?.tool_calls) {
if (choice.delta?.tool_calls?.length) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e519feb. I added the matching streaming diagnostic log when a delta contains non-empty modern tool_calls plus legacy function_call, preserving the same precedence semantics while making the ignored legacy field visible during debugging.

if (choice.delta.function_call) {
debugLogger.debug(
`Ignoring legacy function_call "${choice.delta.function_call.name ?? '<pending>'}" because tool_calls is non-empty`,
);
}

for (const toolCall of choice.delta.tool_calls) {
const index = toolCall.index ?? 0;

Expand All @@ -1297,6 +1339,17 @@ export function convertOpenAIChunkToGemini(
);
}
}
} else if (choice.delta?.function_call) {
const functionCall = choice.delta.function_call;
debugLogger.debug(
`Using legacy function_call fallback (streaming): ${functionCall.name ?? '<pending>'}`,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4129bad4f. The streaming legacy fallback log now only fires on name-bearing chunks:

if (functionCall.name) {
  debugLogger.debug(
    `Using legacy function_call fallback (streaming): ${functionCall.name}`,
  );
}

Continuation chunks that only carry fragmented arguments no longer produce repetitive <pending> debug lines.

);
toolCallParser.addChunk(
0,
getLegacyFunctionCallChunk(functionCall),
undefined,
functionCall.name,
);
}

// Only emit function calls when streaming is complete (finish_reason is present)
Expand Down
Loading