fix(codex): set finish_reason to "tool_calls" in non-streaming response when tool calls are present#2444
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the OpenAI response conversion logic to correctly set the finish_reason to 'tool_calls' when tool calls are present. A critical issue was identified where the 'toolCalls' variable is used outside of its scope, which will cause a compilation error. It is suggested to verify the presence of tool calls by checking the 'template' object directly using gjson.
| template, _ = sjson.SetBytes(template, "choices.0.finish_reason", "stop") | ||
| template, _ = sjson.SetBytes(template, "choices.0.native_finish_reason", "stop") | ||
| finishReason := "stop" | ||
| if len(toolCalls) > 0 { |
There was a problem hiding this comment.
The variable toolCalls is defined inside the if outputResult.IsArray() block (lines 288-364) and is not accessible here. This will cause a compilation error.
To fix this, you should move the declaration of toolCalls (line 292) outside of the if block. Alternatively, you can check the template for the presence of tool calls as suggested below.
| if len(toolCalls) > 0 { | |
| if gjson.GetBytes(template, "choices.0.message.tool_calls").IsArray() { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 532c3aa409
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| template, _ = sjson.SetBytes(template, "choices.0.finish_reason", "stop") | ||
| template, _ = sjson.SetBytes(template, "choices.0.native_finish_reason", "stop") | ||
| finishReason := "stop" | ||
| if len(toolCalls) > 0 { |
There was a problem hiding this comment.
Move toolCalls state outside output block
toolCalls is declared inside if outputResult.IsArray() but then referenced later when setting finish_reason. In Go this identifier is out of scope at that point, so this change makes the package fail to compile (undefined toolCalls) in any build that includes this file. Please hoist the variable (or a boolean like hasToolCalls) to the outer function scope before using it in the status block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adjusts the Codex→OpenAI non-streaming chat-completions translator so OpenAI-compatible clients don’t prematurely exit agent loops when the Codex response includes tool calls.
Changes:
- Derives
choices[0].finish_reason/native_finish_reasonfrom presence of tool calls (tool_callsvsstop) inConvertCodexResponseToOpenAINonStream.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| finishReason := "stop" | ||
| if len(toolCalls) > 0 { | ||
| finishReason = "tool_calls" | ||
| } | ||
| template, _ = sjson.SetBytes(template, "choices.0.finish_reason", finishReason) | ||
| template, _ = sjson.SetBytes(template, "choices.0.native_finish_reason", finishReason) |
There was a problem hiding this comment.
toolCalls is declared inside the if outputResult.IsArray() block earlier in this function, but it’s referenced here outside that block. In Go this will not compile (undefined: toolCalls). Move the declaration to the outer function scope (or track a separate hasToolCalls boolean) so it’s available when computing finishReason.
| finishReason := "stop" | ||
| if len(toolCalls) > 0 { | ||
| finishReason = "tool_calls" | ||
| } | ||
| template, _ = sjson.SetBytes(template, "choices.0.finish_reason", finishReason) | ||
| template, _ = sjson.SetBytes(template, "choices.0.native_finish_reason", finishReason) |
There was a problem hiding this comment.
This change alters non-streaming finish_reason behavior for completed responses with tool calls, but there are currently no unit tests covering ConvertCodexResponseToOpenAINonStream. Please add tests asserting choices[0].finish_reason is tool_calls when a function_call output item is present and stop when it isn’t, to prevent regressions and keep parity with the streaming path.
… when tool calls are present
532c3aa to
ec77f4a
Compare
xkonjin
left a comment
There was a problem hiding this comment.
Code Review — fix(codex): set finish_reason to "tool_calls" in non-streaming response
Overall: Good fix for a subtle OpenAI compatibility bug 🟢
The changes correctly address an issue where non-streaming Codex responses with tool calls would report finish_reason: "stop" instead of "tool_calls", which could break agent loop detection in OpenAI-compatible clients.
Positives
- Scope fix: Moving
var toolCallsdeclaration outside theoutputResult.IsArray()block ensures the variable is accessible when determining the finish reason. This is the correct fix. - Clean finish_reason logic: The conditional check
if len(toolCalls) > 0properly differentiates between content-only responses and tool call responses.
Minor Concerns
1. Test coverage (important)
This PR lacks any test coverage for the fix. Consider adding:
- A test case where the Codex response contains tool calls and verifying
finish_reasonequals"tool_calls" - A test case with mixed content + tool calls
- A regression test to prevent this from breaking in the future
2. Edge case consideration
What happens if outputResult is not an array but the Codex response still contains tool calls via another path? The current logic would result in finish_reason: "stop". Is this the correct fallback behavior? A comment explaining this assumption would help future maintainers.
Verdict
LGTM as a correctness fix, but recommend adding test coverage before merging to prevent regression.
luispater
left a comment
There was a problem hiding this comment.
Summary
This fixes ConvertCodexResponseToOpenAINonStream always setting choices[0].finish_reason to "stop" for status=="completed", even when the response includes tool calls. The change mirrors the streaming path behavior and prevents OpenAI-compatible clients from incorrectly terminating tool/agent loops.
Key findings
- Correctly sets
finish_reason(andnative_finish_reason) to"tool_calls"whentoolCallsis non-empty; otherwise keeps"stop". - Scope change for
toolCallsis minimal and only enables the finish-reason decision after output parsing.
Tests
- CI
buildis green. - Suggest adding a focused unit test for the non-streaming tool-call finish reason to prevent regressions.
This is an automated Codex review result and still requires manual verification by a human reviewer.
Fixes #2443
Problem
ConvertCodexResponseToOpenAINonStreamunconditionally setsfinish_reason = "stop"when the Codex response status is"completed", even when the response containstool_calls. This causes any OpenAI-compatible client that inspectsfinish_reasonto incorrectly terminate its agentic loop.The streaming path (
ConvertCodexResponseToOpenAI) already handles this correctly by checkingFunctionCallIndex != -1.Fix
Check whether
toolCallsis non-empty before decidingfinish_reason, mirroring the streaming path:Testing