Fix streaming tool calls with tests for many variants. #1218
Conversation
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
This is the typical response, where "arguments":"" arrives
first in the stream, followed by "arguments":"{}"
The response data is a real capture from the OpenRouter API,
however some request and header data may be from other test fixtures.
This is a streaming response where the first arguments
you get is a fully formed "arguments":"{}"
The response data is a real capture from the OpenRouter API,
however some request and header data may be from other test fixtures.
Note that the replays are marked as "read-only", as they are variants seen in the wild where the streaming tool call argument fragments arrive in a specific order.
…at arrives first. The previous code erroneously caused the first "arguments" to be duplicated, by using "+=" even when being initially set. This went unnoticed as many models stream "arguments":"" first. When a more fully formed "arguments" fragment arrived first, it was causing "Error: Extra data: line 1 column 3 (char 2)"
This was failing with "Error: unsupported operand type(s) for +=: 'NoneType' and 'str'" The response data is a real capture from the OpenRouter API, however some request and header data may be from other test fixtures.
However, I'm not sure why arguments was initially not present or seen as None.
|
Here's output of these tests failing without the "fix" commits: https://gist.github.com/jamessanford/caff2c68f3be70aa31bbd023310339e3 |
|
This is excellent - thanks for being so thoughtful about the tests. |
|
Those test failures look unrelated to me. I'm going to land this. |
This was referenced Aug 11, 2025
Closed
simonw
added a commit
that referenced
this pull request
Aug 11, 2025
simonw
added a commit
that referenced
this pull request
Aug 11, 2025
|
@simonw Thanks for the merge, and for fixing the llm_version part! If you could peek at simonw/llm-openrouter#43 too that would be awesome |
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.
This adds test data and fixes for streaming tool call bugs as mentioned in:
As mentioned in #1130, the error was the function arguments from the first
streaming fragment being duplicated. This was often not seen as
many models emit an empty argument first.
The tests include three variants of streaming fragments that have been
observed using the OpenRouter API and non-OpenAI models.
The test data shows response fragments arriving as follows:
Variant "a", this worked before:
"function":{"name":"llm_version","arguments":""}
"function":{"name":"llm_version","arguments":"{}"}
Variant "b", this used to fail due to the bug that duplicated the initial arguments, causing
Error: Extra data: line 1 column 3 (char 2)"function":{"name":"llm_version","arguments":"{}"}
Variant "c", this used to fail with error
Error: unsupported operand type(s) for +=: 'NoneType' and 'str'This PR addresses these issues:
This PR includes the fixes from these PRs: