-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(web): allow session resume without new message (#4100) #4185
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?
Conversation
Summary of ChangesHello @Akshat8510, 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 enhances the flexibility of agent session management by addressing a limitation in 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. 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-code-assist/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 correctly addresses the issue of being unable to resume an agent session without a new message. By changing RunAgentRequest.new_message to be optional (Optional[types.Content] = None), the API now properly supports "resume-only" workflows. The change is well-contained and aligns the web server's request model with the existing logic in runner.run_async, which already handles cases where new_message is None for session resumption. The implementation is correct. I have one suggestion to improve test coverage for this new capability.
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 addresses the issue by making the new_message field optional in the RunAgentRequest model. This change allows for agent session resumption without requiring a new message, which is a valuable improvement for 'resume-only' workflows. The implementation is clean and direct. I've added one comment regarding the addition of a test case to ensure this fix is validated and to prevent future regressions.
|
@gemini-code-assist/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 correctly makes the new_message field optional in RunAgentRequest to resolve a 422 Unprocessable Entity error when resuming a session. The inclusion of a new unit test to verify this is a good practice. However, the fix is incomplete as it introduces a new runtime error. The new test provides a false positive because it mocks the Runner.run_async method, bypassing a validation check that would cause a ValueError and a 500 error in a real-world scenario. To be complete, the runner logic should also be updated to handle this new 'resume-only' case gracefully.
|
@gemini-code-assist/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 correctly addresses the issue of allowing agent session resumption without requiring a new_message. The change to make RunAgentRequest.new_message optional is well-implemented. The corresponding adjustment in runners.py to handle a "no-op resume" by returning early instead of raising an error is a sensible approach. The new test case effectively verifies that the API now accepts requests without a new message. I have a couple of minor suggestions in the test file to improve code cleanliness.
|
@gemini-code-assist/gemini review |
|
/gemini summary |
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 makes the new_message field optional in RunAgentRequest to support 'resume-only' workflows, which is a good enhancement. The changes in the web server model, the runner logic, and the addition of a new test case are all well-aligned with this goal. I have one suggestion to improve observability for the new no-op resume behavior.
Summary of ChangesThis pull request enhances the flexibility of agent session management by enabling "resume-only" workflows. It addresses a previous limitation where the FastAPI web server would reject requests to resume an agent session if no 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
|
|
@gemini-code-assist/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 correctly makes the new_message field optional in the RunAgentRequest to allow for "resume-only" workflows, which is a great enhancement. The changes are well-implemented, including the logic to handle this case as a no-op in the runner and the addition of a new test case.
I have a couple of suggestions to further improve the changes:
- Update the
run_asyncdocstring to reflect the new behavior. - Enhance the new test to cover the full end-to-end logic instead of mocking the runner's behavior.
Overall, this is a solid contribution.
|
@gemini-code-assist/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 correctly addresses the issue of allowing session resumption without a new message by making the new_message field optional. The changes to the RunAgentRequest model and the corresponding logic in runners.py are appropriate and well-implemented. The addition of a new test case to verify this behavior is also a great inclusion. I have a couple of minor suggestions to improve the clarity of the documentation and a test comment.
|
@gemini-code-assist/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 resolves an issue where resuming an agent session without a new message would fail. The changes make the new_message field in RunAgentRequest optional and update the runner logic to handle this case as a no-op resume, which is a clean solution. The docstrings have been updated to reflect the new behavior, and a new test case has been added to verify the fix. The changes are well-implemented and improve the flexibility of the web API. The code quality is high, and I have no further suggestions for improvement.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
@gemini-code-assist/gemini review |
|
/gemini summary |
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 implements the ability to resume a session without providing a new message by making new_message optional in RunAgentRequest. The changes are well-contained, touching the web server model, the runner logic, and adding a corresponding test case. My main feedback is to improve the handling of state_delta in the new no-op resume path to avoid silently ignoring user input.
Summary of ChangesThis pull request enhances the flexibility of agent session management by enabling 'resume-only' workflows. It addresses a previous limitation where the FastAPI web server would reject requests to resume an agent session if no 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
|
|
@gemini-code-assist/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 is a well-executed fix. Making the new_message field optional in RunAgentRequest is the correct approach to allow for "resume-only" workflows. The changes in runners.py to gracefully handle the absence of both new_message and invocation_id by performing a no-op are clean and logical. I appreciate the addition of a new test case in test_fast_api.py, which effectively verifies the fix and prevents future regressions. Overall, the changes are clear, concise, and well-tested.
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 addresses the issue of allowing agent session resumption without a new message. The changes in adk_web_server.py make the new_message field optional in the RunAgentRequest model, and the logic in runners.py is updated to handle this as a no-op resume, which is a clean solution. The associated test updates, including a new test case to verify this specific workflow, are well-implemented and ensure the fix is robust. Overall, the changes are of high quality and I have no concerns.
|
@gemini-code-assist/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 correctly addresses the issue of allowing agent session resumption without requiring a new message. The changes are well-implemented across the API model, the runner logic, and the tests. By making new_message optional in RunAgentRequest and handling the "no-op resume" case gracefully in the runner, the fix is both robust and clean. The addition of a dedicated unit test and updates to the mock implementation ensure the new behavior is verified. The docstring update also accurately reflects the change in the run_async method's behavior. Overall, this is a solid contribution that improves the flexibility of the web server API.
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 addresses the issue of allowing session resumes without a new message by making new_message optional in RunAgentRequest. The logic in runners.py to handle this as a no-op resume is well-implemented, including the warning for an ignored state_delta. The addition of corresponding unit tests is also a great improvement. I have a couple of suggestions to improve the docstring accuracy and test robustness.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 correctly makes the new_message field optional in RunAgentRequest to support session resume workflows without a new message. The changes in runners.py to handle this as a no-op instead of an error are well-implemented, including the warning for an ignored state_delta. The new tests effectively verify this behavior. I have one suggestion to improve the robustness of the new test.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 correctly addresses the issue of allowing agent session resumption without a new message. The change to make new_message optional in RunAgentRequest is appropriate, and the logic in runners.py to handle this "no-op resume" case is well-implemented. The addition of new tests to verify this behavior is also a great inclusion. I have one suggestion in the test file to improve the accuracy of a mock function, ensuring it fully aligns with the behavior of the real implementation.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 correctly implements the ability to resume a session without providing a new message, which is a valuable fix for "resume-only" workflows. The changes are well-contained, making RunAgentRequest.new_message optional and updating the runner to handle this case as a no-op. The addition of new tests to cover this scenario is also appreciated.
My main feedback is on the implementation of one of the new tests, which currently tests a mock's behavior rather than the production code's logging. I've left a detailed comment with a suggestion to improve the test's reliability and maintainability. Overall, this is a good change.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 allowing agent session resumption without a new message. The changes are logical and well-implemented: new_message is now optional in the RunAgentRequest, and the runner logic correctly handles this as a no-op resume. The addition of a warning when state_delta is ignored in this scenario is a thoughtful touch. The new unit tests in both test_fast_api.py and test_runners.py provide good coverage for the new functionality. I have one suggestion regarding code duplication in a test mock to improve long-term maintainability. Overall, this is a solid contribution.
…dback Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 correctly makes the new_message field optional in RunAgentRequest to allow for "resume-only" agent runs. The implementation handles the new no-op case gracefully by logging informative messages and ignoring state_delta when appropriate. The accompanying tests are thorough, covering the API change, the new no-op logic, and the specific warning for ignored state deltas. My only suggestion is to address some code duplication in the test suite to improve long-term maintainability.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 correctly addresses the issue of resuming an agent session without a new message by making the new_message field optional in the RunAgentRequest. The changes are well-implemented, with the runner now handling this as a "no-op resume" and logging a warning if an unhandled state_delta is provided. The accompanying tests are thorough, covering both the API layer and the runner's internal logic.
I have one suggestion to improve the maintainability of the new tests by reducing code duplication. Overall, this is a solid fix.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 correctly implements the ability to resume a session without providing a new message by making new_message optional in RunAgentRequest. The core logic in runners.py is updated to handle this as a no-op resume, including appropriate logging for an ignored state_delta. The changes are well-supported by new unit tests that verify the behavior at both the web server and runner levels. My feedback includes a minor suggestion to improve code clarity in one of the new test mocks.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@gemini-code-assist/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 the issue of being unable to resume a session without providing a new message. The change to make new_message optional in RunAgentRequest is the correct fix for the web server API. The underlying runner logic has also been thoughtfully updated to handle this "no-op resume" case by returning early instead of raising an error, and it correctly warns when state_delta is ignored in this context. The new behavior is well-covered by new tests for both the API and the runner logic, ensuring the solution is robust. This is a high-quality contribution that improves the flexibility of session management.
|
/gemini summary |
Summary of ChangesThis pull request enhances the flexibility of agent session management by resolving an issue where the FastAPI web server previously rejected requests to resume an agent session if no 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
|
|
Hi @ryanaiagent, I’ve addressed all feedback from the manual + AI reviews and updated the implementation + tests accordingly. The gemini-code-assist bot also reviewed it and has no more concerns. |
|
Ready for review |
Description
This PR resolves #4100 by making the
new_messagefield optional in theRunAgentRequestmodel.The Problem:
When attempting to resume an agent session via the FastAPI web server, the request would fail with a
422 Unprocessable Entityifnew_messagewas omitted. This prevented "resume-only" workflows where a user just wants to wake up an existing session.The Solution:
Updated
RunAgentRequest.new_messageto beOptional[types.Content] = None. The underlyingrunner.run_asynclogic already supportsNonefor resuming purposes, so no further logic changes were required.Verification:
Verified that
RunAgentRequestnow validates successfully whennew_messageis missing, defaulting the field toNone.