-
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
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
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.
…equests for session resumption without providing new content. 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 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.
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 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.
Signed-off-by: Akshat Kumar <akshat230405@gmail.com>
|
@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
|
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 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.
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 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.
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 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! |
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.