-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix OpenAI empty choices panic #5248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix OpenAI empty choices panic #5248
Conversation
…ponses Signed-off-by: Arya Pratap Singh <[email protected]>
Signed-off-by: Arya Pratap Singh <[email protected]>
DOsinga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing this. if you want I saw that databricks has a similar way of doing things.
|
|
||
| if choices.is_empty() { | ||
| return Err(anyhow!("Response contains empty choices array")); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably good enough, but it feels more in line with the existing code to do something like:
let Some(original) = response
.get("choices")
.and_then(|c| c.get(0))
.and_then(|m| m.get("message"))
else {
return Ok(Message::with_content(Vec::new()));
};
i.e. if there is no content, we just return an empty message, same as if it is empty for other reasons below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and the great suggestion, i've implemented your recommended change to use the more standard pattern that returns an empty message instead of an error when choices are missing or empty. This is indeed more consistent with how the rest of the codebase handles empty content cases.
lgtm now @DOsinga
Signed-off-by: Arya Pratap Singh <[email protected]>
|
this is completed i think |
| if chunk.choices.is_empty() { | ||
| yield (None, usage) | ||
| } else if let Some(tool_calls) = &chunk.choices[0].delta.tool_calls { | ||
| } else if !chunk.choices.is_empty() && chunk.choices[0].delta.tool_calls.as_ref().is_some_and(|tc| !tc.is_empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we aleady guard for empty though, don't we? on line 473?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just for safety, should not impact
|
just checking the build failure |
Signed-off-by: Arya Pratap Singh <[email protected]>
|
@DOsinga please rerun ci pipelines for this, it should works now mostly edit:
|
|
there is still some of that checking going on I think |
I think all have completed successfully now. Thanks, glad I was able to fix this |
* main: (32 commits) turn off WAL (#5203) Skip subagents for gemini (#5257) Revert "Standardize Session Name Attribute" (#5250) improve provider request logging a bit (#5236) Fix OpenAI empty choices panic (#5248) Revert "Rewrite extension management tools" (#5243) Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) Release/1.11.0 (#5224) Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229) Break compaction back into check_ and do_ compaction (#5212) fix: revert built app name to uppercase Goose (#5206) ...
* 'main' of github.com:block/goose: docs: provide more clarity in tagging step of releases (#5269) docs: add GOOSE_DEBUG environment variable (#5265) Lifei/UI parameter input (#5222) turn off WAL (#5203) Skip subagents for gemini (#5257) Revert "Standardize Session Name Attribute" (#5250) improve provider request logging a bit (#5236) Fix OpenAI empty choices panic (#5248) Revert "Rewrite extension management tools" (#5243) Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) Release/1.11.0 (#5224)
|
@taniandjerry can you please add Hacktoberfest and Large label to this. Thanks, I'll see this week's issues in a while |
* main: (40 commits) Fix extension manager resource deadlock (#5066) add new prompt for memory extension (#4945) feat: Stable system prompt and tool ordering for more cache hits (#5192) Remove gtag analytics error (#5268) Feat/smart task organizer recipe (#5032) docs: `goose recipe open` command (#5264) docs: provide more clarity in tagging step of releases (#5269) docs: add GOOSE_DEBUG environment variable (#5265) Lifei/UI parameter input (#5222) turn off WAL (#5203) Skip subagents for gemini (#5257) Revert "Standardize Session Name Attribute" (#5250) improve provider request logging a bit (#5236) Fix OpenAI empty choices panic (#5248) Revert "Rewrite extension management tools" (#5243) Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) ...
Summary
Fixed a panic error ("index out of bounds") that occurred when Goose interacted with OpenAI-compatible servers (like
mlx_lm.serverorLM Studio). The error was triggered when these servers sent response chunks with an emptychoicesarray, as the code accessed elements likeresponse["choices"][0]andchunk.choices[0]without checking if the array was empty first. The issue occurs rarely and is now almost guranteed to not occur.Root Cause Analysis (RCA, perhaps)
The panic was caused by unsafe array access in the OpenAI response parsing logic:
response_to_streaming_message()function directly accessedchunk.choices[0]without performing a bounds check.response_to_message()function directly accessedresponse["choices"][0]["message"]without validating the array's existence or length.While an empty
choicesarray is valid according to the OpenAI specification, the existing code did not handle this case, leading to a crash.Technical Solution
Defensive bounds checking was implemented in both response parsing pathways to prevent the crash:
Streaming Response Handler (
response_to_streaming_message)!chunk.choices.is_empty()guards before all accesses tochoices[0].!tool_chunk.choices.is_empty()checks within tool call processing loops.Non-Streaming Response Handler (
response_to_message)response.get("choices").and_then(|c| c.as_array()).Type of Change
Testing
The fix was verified through the following steps:
cargo build.choicesarrays no longer cause panics.Manual testing confirmed that Goose now gracefully handles empty
choicesarrays, ensuring compatibility with a wider range of OpenAI-compatible servers.I can't actually test mlx server based but reverified with everything else. (needs apple chip)
Related Issues
Fixes #4276