-
Notifications
You must be signed in to change notification settings - Fork 10.9k
fix(core): binary content handling in parallel function responses #17058
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
Conversation
Summary of ChangesHello @jw409, 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 refactors the handling of binary content, specifically Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request correctly refactors the handling of binary content in function responses to support parallel function calls. The core change properly nests fileData and inlineData for multimodal-capable models (like Gemini 3) and drops them for older models to prevent API errors, which is a solid improvement.
I've identified one high-severity issue where a misleading descriptive text ('Binary content provided...') is added to the response for older models, even though the binary content itself is dropped. This could confuse the model. My review includes a specific suggestion to fix this by ensuring the text is only added when the binary content is actually sent.
Overall, the changes are well-structured and address the stated problem effectively, pending the fix for the identified issue.
| if (siblingParts.length > 0) { | ||
| return [part, ...siblingParts]; | ||
| debugLogger.warn( | ||
| `Model ${model} does not support multimodal function responses. Sibling parts will be omitted to prevent API errors in parallel function calling.`, | ||
| ); | ||
| } |
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 logic correctly warns and drops sibling parts for non-multimodal models. However, there's a related issue: the code at lines 115-123 has already added a descriptive text like output: 'Binary content provided (...)' to the response. This message becomes misleading because the binary content is now being dropped. The model will be told binary content is provided when it has actually been omitted.
To fix this, we should clear this misleading message from the response object when we drop the parts and there was no original text content. This will also require updating the test 'should handle llmContent with fileData for non-Gemini 3 models (should omit siblings)' to expect response: {}.
| if (siblingParts.length > 0) { | |
| return [part, ...siblingParts]; | |
| debugLogger.warn( | |
| `Model ${model} does not support multimodal function responses. Sibling parts will be omitted to prevent API errors in parallel function calling.`, | |
| ); | |
| } | |
| if (siblingParts.length > 0) { | |
| debugLogger.warn( | |
| `Model ${model} does not support multimodal function responses. Sibling parts will be omitted to prevent API errors in parallel function calling.`, | |
| ); | |
| // If there was no original text, the response object may contain a misleading | |
| // "Binary content provided" message from the dropped parts. Clear it. | |
| if (textParts.length === 0) { | |
| part.functionResponse!.response = {}; | |
| } | |
| } |
Address review feedback: when binary content is dropped for non-multimodal models (Gemini 2.x), the response message now accurately states "Tool executed but binary output was omitted" instead of the misleading "Binary content provided". This prevents confusing the model about what content is available.
|
Addressed in 0e4cf4e:
This prevents misleading the model about what content is available. |
|
Thanks for confirming, @jw409! It's great to hear that the binary content handling is now accurate for both Gemini 3+ (nested) and Gemini 2.x (omitted with warning) models. This ensures correct API behavior and clear communication about content availability. The solution looks robust. |
|
Fixes #17068 |
|
Hi there! Thank you for your contribution to Gemini CLI. We really appreciate the time and effort you've put into this pull request. To keep our backlog manageable and ensure we're focusing on current priorities, we are closing pull requests that haven't seen maintainer activity for 30 days. Currently, the team is prioritizing work associated with 🔒 maintainer only or help wanted issues. If you believe this change is still critical, please feel free to comment with updated details. Otherwise, we encourage contributors to focus on open issues labeled as help wanted. Thank you for your understanding! |
Fixes #17068