-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(agent_tool): ensure text output when skip_summarization is True #4390
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
base: main
Are you sure you want to change the base?
fix(agent_tool): ensure text output when skip_summarization is True #4390
Conversation
When skip_summarization=True, AgentTool previously terminated the flow with a FunctionResponse event that contained no text, causing UIs to appear stuck. This change appends a Text part to the response event when skipping summarization so tool output is displayed. The implementation avoids duplication, safely serializes non-string results, and preserves existing behavior for normal flows. Added regression tests to verify text output is present for both string and JSON-like tool results.
Summary of ChangesHello @ItsMacto, 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 critical user experience issue where the 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. Changelog
Activity
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
|
|
/gemini review |
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 addresses an issue where UIs could get stuck when skip_summarization=True by ensuring a text part is always included in the final event. The implementation correctly adds a text representation of the tool's output, and includes new regression tests.
My review includes a suggestion to refine the serialization logic for non-string results to provide a cleaner output. Additionally, I've recommended strengthening the new test assertions and expanding test coverage to better validate the fix.
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 addresses an issue where AgentTool would not produce a displayable text output when skip_summarization is enabled, causing UIs to appear stuck. The fix correctly appends a Text part to the response event, serializing non-string results to ensure there's always a visible output. The added regression tests effectively cover the new functionality. My review includes suggestions to strengthen the assertions in these new tests to make them more precise and less prone to false positives.
…the 'result' key for JSON display. - assert the exact string content - new test case for a non-string dictionary - more precise assertion checking for the exact text content
|
/gemini review |
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 fixes an issue where the UI would hang when skip_summarization=True by ensuring a text part is always included in the final event. The implementation correctly handles both string and non-string results from the tool. The accompanying tests are thorough and cover the key scenarios. My review includes a suggestion to refactor the implementation for better readability and another to reduce code duplication in the new tests using a pytest fixture.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
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 effectively addresses the issue of UIs hanging when skip_summarization=True by ensuring a text part is always included in the function response event. The implementation correctly handles both string and non-string (JSON-serializable) tool outputs. The new unit tests are comprehensive and validate the fix across different scenarios. The changes are well-implemented and solve the problem described. I have one minor suggestion to improve code clarity and maintainability.
…zation is skipped.
|
Hi @ItsMacto , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the formatting errors. You can run autoformat.sh. Also can you also fix the failing unit tests. |
|
/gemini review |
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 fixes an issue where the UI would hang when skip_summarization=True by ensuring a text part is always included in the final event. The implementation correctly handles both string and non-string tool outputs, and includes a good set of regression tests to prevent this issue from recurring.
My review includes one suggestion to improve the robustness of the logic that unwraps tool results. The current implementation could lead to data loss if a tool returns a dictionary that happens to contain a result key. The suggested change makes this logic more precise to avoid this potential bug.
Overall, this is a solid fix for an important usability issue.
|
/gemini review |
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 effectively resolves an issue where the UI would hang when skip_summarization=True by ensuring a text part is always included in the final response event. The implementation is robust, correctly handling different types of tool results (strings, JSON, and other objects) and safely serializing them. The logic also correctly avoids adding a text part for error responses. The addition of comprehensive regression tests covering various output scenarios is excellent and ensures the fix is effective and won't be broken by future changes. Overall, this is a high-quality contribution that improves the user experience and stability of the agent tool.
|
Hi @ryanaiagent , I believe I made the changes correctly. One thing when running |
When skip_summarization=True, AgentTool previously terminated the flow with a FunctionResponse event that contained no text, leaving UIs stuck.
This change appends a Text part to the response event when summarization is skipped, so the tool's output is displayed. The implementation avoids duplication, safely serializes non-string results, and preserves existing behavior for normal flows.
Added regression tests to verify text output is present for both string and JSON-like tool results.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
When
skip_summarization=True, the final response event emitted by AgentTool contains only a FunctionResponse and no displayable text. Most UIs do not render function responses, making the agent appear to hang or return an empty result.Solution:
When skip summarization is enabled, the function result is additionally included as a Text part in the response event. If the result is a string, it is used directly; otherwise, it is safely serialized to JSON for display. This ensures the final event remains displayable while preserving existing behavior for non-skip paths.
Testing Plan
Unit Tests:
Passed tests:
pytest tests/unittests/tools/test_agent_tool.pyManual End-to-End (E2E) Tests:
Not required. The change affects event construction and is fully covered by unit tests validating the final response content.
Checklist