-
Notifications
You must be signed in to change notification settings - Fork 10.5k
feat(cli): Add /auth logout command to clear credentials and auth state
#13383
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
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @CN-Scars, 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 implements a /logout slash command within the CLI, designed to streamline the user authentication experience. By clearing cached credentials, resetting the authentication type, and wiping conversation history, it provides a robust and user-friendly mechanism for users to log out and switch accounts. This enhancement addresses a significant UX gap, simplifying what was previously a manual and cumbersome process. 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 introduces a /logout command to clear credentials and authentication state, which is a valuable UX improvement. The implementation is well-structured, correctly registering the new command and handling the state changes across different parts of the application. My review identifies a point of duplicated logic where the authentication setting is cleared in two different places. I've provided suggestions to centralize this logic, which will improve code maintainability and clarity. Overall, this is a solid contribution.
2c533d1 to
2ef3308
Compare
|
@CN-Scars , thank you for the PR! This is indeed a good idea, and it looks like it's going in the right direction. I left some feedback already, but at a high level one big change will be needed. Specifically, lets rename this slash command Please incorporate these changes, and then I will be happy to take a second look! |
2ef3308 to
54435f0
Compare
Thanks for the review! I've updated the PR with the following changes:
Ready for another look! |
/logout command to clear credentials and auth state/auth logout command to clear credentials and auth state
|
@CN-Scars , thanks! This PR is looking very close, just one comment to work through and I think we'll be there. |
Thank you for pointing that out. There shouldn't be any problems now! |
|
@joshualitt I have completed these changes, please check them again. |
| ); | ||
| // Strip thoughts from history instead of clearing completely | ||
| context.services.config?.getGeminiClient()?.stripThoughtsFromHistory(); | ||
| // Return dialog action to show auth selection menu |
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.
are we missing context.commands.logout() here? I just realized the existing logout command is deadcode perhaps?
If so, let's update this, update the tests, and then I think this looks good.
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.
You're right - that was dead code. I've removed it.
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.
Hmm, but now who is doing this setAuthState(AuthState.Unauthenticated);? The code may work fine, but regardless, I think it's good for the logout command to explicitly set the state to unauthenticated?
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.
I've refactored the flow: the command now returns { type: 'logout' }, which the processor handles by calling actions.logout() (explicitly sets AuthState.Unauthenticated) then actions.openAuthDialog(). This keeps the state transition explicit while eliminating the dead code issue. Tests updated accordingly.
|
@CN-Scars , thanks for your patience! This looks great now, modulo my question on |
|
@joshualitt , thank you for your review. I have now resolved the issue. Please review it again to see if there are any other problems and test it again. |
| return { type: 'handled' }; | ||
| case 'logout': | ||
| actions.logout(); | ||
| actions.openAuthDialog(); |
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.
Apologies for the delay, I have been testing this, and so far it does work which is great!
However, I'm not sure we want to open the auth dialog automatically when a user logs out? While it does seem very likely this is what the user will do after logging out, alternatively they may exit or wish to confirm they are in fact logged out. Unfortunately, they cannot do this if they are forced back into the login menu. Another problem is that this makes /auth logout look the exact same as /auth login and /auth, and then why have 3 ways to do the same thing?
Taking a step back, I think the value of this command is that it logs you out. It's good for troubleshooting, yes, but users may also wish to logout in contexts when their next action is to quit, i.e. using Gemini CLI in semi-trusted environments, where you never know who is on the computer next.
Another option is to have logout print "You are now logged out, login again to continue using Gemini CLI." or some variant thereof. Thoughts? Also @jacob314 who may have some insights.
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.
It's alright, a slight delay won't have any impact.
Now, after thinking about it, I tend to agree with your point of view. @jacob314 Any thoughts on this approach before I make the change?
|
@joshualitt, I used an alternative approach, you can consider whether it's suitable.: I added a dialog box after the user executes
This gives users clear confirmation of their logged-out state and explicit control over their next action, rather than automatically opening the login menu. |
|
@jacob314 Thanks for the UX feedback! I've addressed all points:
|
|
@CN-Scars , hmm, I think we may want to add a right margin or somethin? The right edge of the box seems to touch the scrollbar. |
|
@joshualitt I've added right margin to logout dialog, and it should look perfect now! |
|
We'll need to cleanup the margin after this lands. Notice the margins are better but not quite right. while there is a margin 1 on the right side it is not rendering as there is an issue in the width calculation. This is fine to land and I'll cleanup with a quick followup PR to make the UI the correct width. Easiest way to fix is to look at what other dialogs do and align with their styles. the root of the problem is that width 100% shouldn't be used quite like this in Ink and there is an extra layer of boxes adding complexity. |
jacob314
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.
joshualitt
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, looks great now!!
80c4225



Summary
Implements a
/logoutslash command to clear cached credentials and reset authentication state. After logout, users see the authentication selection menu instead of automatically re-authenticating, enabling easy account switching and providing a cleaner logout experience.This addresses a common UX gap where users had to manually delete credential files or use workarounds to switch between different authentication methods or accounts.
Details
Implementation:
Command Registration:
packages/cli/src/ui/commands/logoutCommand.ts: New slash command that clears cached credentials and resetsselectedAuthTypesettingpackages/cli/src/services/BuiltinCommandLoader.ts: Registered the logout commandType System:
packages/cli/src/ui/commands/types.ts: AddedLogoutActionReturninterfacepackages/cli/src/ui/types.ts: ExtendedSlashCommandProcessorResultunion type withlogoutState Management:
packages/cli/src/ui/AppContainer.tsx:logoutaction toslashCommandActionsselectedAuthTypesetting viasettings.setValue()Unauthenticatedpackages/cli/src/ui/hooks/slashCommandProcessor.ts: Integrated logout action handlingpackages/cli/src/ui/hooks/useGeminiStream.ts: Added logout case handlerTest Coverage:
packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx: Added logout mock to test suiteBehavior:
/logout:selectedAuthTypesetting removedDesign Decisions:
selectedAuthTypeto prevent auto-login/logoutcommand without parametersRelated Issues
How to Validate
1. Basic Logout Flow:
2. Verify Credential Clearing:
3. Test Account Switching:
4. Test State Reset:
5. Preflight Check:
npm run preflight # Expected: All checks pass6. Test Command Availability:
Pre-Merge Checklist
/helpslashCommandProcessor.test.tsxTesting Matrix
Notes: