Repository navigation
fix(openai): keep streamed tool calls without an index apart - #2028
Open
pcbeingused333 wants to merge 2 commits into
Open
pcbeingused333 wants to merge 2 commits into
pcbeingused333 wants to merge 2 commits into
Conversation
_extract_streamed_openai_response put every tool-call delta without an index into the latest call. Gemini's OpenAI-compatible API streams each parallel call whole in its own chunk and leaves index unset, so two calls were merged into one: the second id overwrote the first, the first name was kept and the arguments were concatenated into invalid JSON. A delta without an index now starts a new call when it brings a new id, or a name without an id; otherwise it continues the latest one, so argument fragments still merge. Deltas with an index are grouped as before. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Comment on lines
+430
to
+445
| def test_streaming_chat_completion_keeps_parallel_tool_calls_without_index_apart(): | ||
| _model, completion, _usage, _metadata, _service_tier = ( | ||
| lf_openai_module._extract_streamed_openai_response( | ||
| SimpleNamespace(type="chat"), | ||
| [ | ||
| _tool_call_chunk( | ||
| id="call_paris", | ||
| name="get_weather", | ||
| arguments='{"city": "Paris"}', | ||
| ), | ||
| _tool_call_chunk( | ||
| id="call_tokyo", | ||
| name="get_weather", | ||
| arguments='{"city": "Tokyo"}', | ||
| finish_reason="stop", | ||
| ), |
Contributor
There was a problem hiding this comment.
Name-only calls lack test coverage
Neither new test exercises the rule that a name without an ID starts another call. Both parallel calls have IDs, and the fragment without an ID also has no name. Removing this new branch would therefore leave the tests passing.
Add a test with two calls that omit both index and id, share a function name, and have different arguments. Assert that their arguments stay separate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/test_openai.py
Line: 430-445
Comment:
**Name-only calls lack test coverage**
Neither new test exercises the rule that a name without an ID starts another call. Both parallel calls have IDs, and the fragment without an ID also has no name. Removing this new branch would therefore leave the tests passing.
Add a test with two calls that omit both `index` and `id`, share a function name, and have different arguments. Assert that their arguments stay separate.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
… call The rule that a delta without an index, id or continuation starts a new call when it carries a function name had no test: removing that branch left the suite green. Two calls with the same function name and different arguments, both without index and id, must stay separate; the test fails without the name branch. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With the OpenAI integration, a streamed chat completion whose tool-call deltas have no
indexis traced as a single tool call. Gemini's OpenAI-compatible API (https://generativelanguage.googleapis.com/v1beta/openai/) streams parallel calls this way: each call arrives whole (id, name and arguments) in its own chunk, withouttool_calls[].index._extract_streamed_openai_responsesends a delta without an index to the latest call, so two parallelget_weathercalls end up as one generation output with the second call's id, the first call's name and the arguments concatenated ({"city": "Paris"}{"city": "Tokyo"}), which is not valid JSON.A delta without an index now starts a new call when it brings a new id, or a name without an id; anything else continues the latest call, so argument fragments of one call still merge, including a fragment that repeats the call's id. Deltas with an index are grouped exactly as before. This is the same rule as in langfuse-js #1040 and langsmith-python #65.
Tests in
tests/unit/test_openai.py: two whole calls without index (fails onmain, passes here) and one call without index split into fragments (passes before and after).tests/unit/test_openai.pyandtest_openai_prompt_extraction.pypass (62), andruff check/ruff format --checkare clean. No live API call was made.This change was made with Claude Code.
The PR appears safe to merge, with a non-blocking gap in test coverage.
Summary
Updates streamed OpenAI tool-call grouping for providers that omit
index.Reviews (1) · Last reviewed commit: "fix(openai): keep streamed tool calls wi..." · Reviewed by Greptile