fix: handle OpenAI responses tool call indices#2245
fix: handle OpenAI responses tool call indices#2245Long-louis wants to merge 2 commits intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug in the OpenAI response translator where streaming tool calls with multiple indices were not correctly processed. The changes ensure that each tool call, identified by its index, is handled independently, leading to accurate event emission for function calls and proper reconstruction of their arguments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue in the OpenAI responses translator where streamed tool calls with multiple indices were not handled correctly. The change replaces the logic that was hardcoded to the first tool call with an iteration over all tool calls, correctly handling state and event emission for each one. The implementation is sound and addresses the described problem. I have one minor suggestion to improve the readability of the logic that determines the tool call ID.
| callID := tc.Get("id").String() | ||
| if callID == "" { | ||
| callID = st.FuncCallIDs[toolIdx] | ||
| } | ||
| if callID == "" { | ||
| callID = fmt.Sprintf("call_%s_%d", st.ResponseID, toolIdx) | ||
| } |
There was a problem hiding this comment.
The logic for determining the callID is spread across two separate if statements. While functionally correct, this can be made more readable and the logic clearer by nesting the checks. This would make it more explicit that you first try to get the ID from the chunk, then from state, and only then synthesize a new one if both are missing.
| callID := tc.Get("id").String() | |
| if callID == "" { | |
| callID = st.FuncCallIDs[toolIdx] | |
| } | |
| if callID == "" { | |
| callID = fmt.Sprintf("call_%s_%d", st.ResponseID, toolIdx) | |
| } | |
| callID := tc.Get("id").String() | |
| if callID == "" { | |
| callID = st.FuncCallIDs[toolIdx] | |
| if callID == "" { | |
| callID = fmt.Sprintf("call_%s_%d", st.ResponseID, toolIdx) | |
| } | |
| } |
There was a problem hiding this comment.
Moved the stream override out of internal/translator/** and registered it from sdk/translator/builtin in 2108f7c so the PR no longer needs to touch the guarded translator path. That also made this local readability suggestion obsolete on the original hunk.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1024e71e94
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| toolIdx := idx | ||
| if tcIdx := tc.Get("index"); tcIdx.Exists() { | ||
| toolIdx = int(tcIdx.Int()) | ||
| } |
There was a problem hiding this comment.
toolIdx is reassigned from tc.index and then used as the sole key for FuncCallIDs, FuncNames, and FuncArgsBuf. Because tool_calls[*].index is local to each choice, a stream with multiple choices can have both choices writing to the same key (for example, both using index 0), which suppresses the second response.output_item.added and merges argument deltas across choices. Key this state by both choice index and tool index (or nest per choice) to avoid cross-choice corruption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2108f7c. The stream override now keys function-call state by (choice index, tool_calls index) and assigns a stable output slot per logical call, so parallel choices that both emit tool_calls[0] no longer collide. Regression coverage was added in sdk/api/handlers/openai/openai_responses_websocket_test.go for the multi-choice case.
| if st.FuncCallIDs[toolIdx] == "" { | ||
| st.FuncCallIDs[toolIdx] = callID |
There was a problem hiding this comment.
This branch permanently locks the first seen callID for a tool index because st.FuncCallIDs[toolIdx] is only assigned when empty. In this commit, missing ids are now synthesized (call_<response>_<idx>), so if a later chunk provides the real provider tool_calls[*].id, it is ignored and all emitted item_id/call_id values remain synthetic. In streams where id arrives after initial deltas, clients that expect the provider id will get a mismatched call id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2108f7c. The override now buffers tool-call arguments until a real upstream id is available and only emits response.output_item.added and argument deltas once the provider call_id is known. Regression coverage was added for the id-arrives-later stream shape.
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Summary: Refactors tool call handling to properly iterate over the tool_calls array using ForEach instead of hardcoding index 0. This is a critical fix — the previous code would silently drop all but the first concurrent tool call.
✅ Positives
- Correctly iterates all tool call chunks via
tcs.ForEach - Properly handles
indexfield from each tool call to dispatch to the correct slot - Synthetic call_id fallback (
call_{responseID}_{toolIdx}) prevents NPEs when providers omit ids
⚠️ Concerns
-
Map bounds safety:
st.FuncCallIDs[toolIdx],st.FuncNames[toolIdx], andst.FuncArgsBuf[toolIdx]— if a provider sends"index": 99, this will panic on index-out-of-range if these are fixed-size slices. Consider adding a bounds check or using a map instead of a slice. -
Synthetic call_id collision risk: The fallback
call_{responseID}_{toolIdx}is deterministic per (response, index) pair. If the realidarrives on a later chunk, the synthetic id will already be emitted inresponse.output_item.added. Consumers may see a mismatch between the added item id and what the model references. Worth documenting this as a known limitation. -
No tests included. This touches the core streaming translation path. A test case with 2+ parallel tool calls in a single chunk would validate the fix and prevent regression.
Verdict
The fix is directionally correct and addresses a real bug. The bounds-check concern should be resolved before merge to avoid panics in production.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
This PR fixes an important issue for streamed tool calls with multiple indices, but it introduces two correctness regressions in state keying and call_id lifecycle handling.
Blocking findings:
internal/translator/openai/openai/responses/openai_openai-responses_response.go:301-303
toolIdxis derived fromtool_calls[].indexand used as the global output/state key.
In multi-choice streams, different choices can both emittool_calls[0], causing collisions and mixed state (arguments/name/call_id) across choices.internal/translator/openai/openai/responses/openai_openai-responses_response.go:306-320
A synthetic fallbackcallIDis persisted on first sight when upstreamidis absent, and later real IDs are never adopted.
This can produce response events with non-upstreamcall_idvalues.
Suggested fix direction:
- Keep state partitioned by choice scope (or another collision-free composite key), not raw
tool_calls[].indexalone. - Allow upgrading from synthesized call IDs to real upstream IDs when the real ID appears in later chunks.
Test plan:
- Add streaming tests for:
- multi-choice + same
tool_calls[].indexvalues (no state collision), - first chunk without
id, later chunk with realid(ID is upgraded and emitted consistently).
- multi-choice + same
|
Updated in 2108f7c. Changes applied:
One review note did not apply as written: the map-bounds concern assumed fixed-size slices, but this translator state was already using maps, so |
3 participants
Summary
Verification