Fix terminal hang when user exits browser without logging in #15748
Conversation
Summary of ChangesHello @gundermanc, 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 significantly enhances the robustness and user experience of the CLI's OAuth authentication process. It introduces mechanisms to tolerate and differentiate between various authentication failure scenarios, such as when user interaction is required or when the process is explicitly cancelled. By implementing silent authentication attempts and refining the interactive login flow, the changes aim to make the CLI more resilient and user-friendly, especially in diverse operational environments. The refactoring also streamlines the initial application setup related to terminal interaction and authentication state management. 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
|
|
Size Change: +766 B (0%) Total Size: 22.2 MB ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the authentication flow, making it more robust by tolerating OAuth failures and allowing for user cancellation. The introduction of a silentAuthOnly mode is a great addition for non-interactive environments. The refactoring of authentication logic into the useAuth hook centralizes the responsibility and improves code organization. I've identified a couple of areas for improvement regarding type safety and code duplication that would further enhance the maintainability of these changes.
packages/cli/src/core/auth.ts
Outdated
|
|
||
| try { | ||
| if (silentAuthOnly) { | ||
| (config as any).silentAuthOnly = true; |
There was a problem hiding this comment.
Using as any to dynamically add a property to the config object circumvents TypeScript's type safety and can lead to maintainability issues. This creates an implicit contract between this function and the refreshAuth implementation that is not enforced by the type system.
A safer approach would be to introduce a mechanism for passing this option that is type-safe. For example, you could use a WeakMap to associate the silentAuthOnly flag with the config object without mutating it. Alternatively, if Config is a class you control, you could add a method to set temporary options, like config.setAuthOptions({ silent: true }), which would make the intent clearer and maintain type safety.
packages/cli/src/ui/auth/useAuth.ts
Outdated
| const { runExitCleanup } = await import('../../utils/cleanup.js'); | ||
| const { RELAUNCH_EXIT_CODE } = await import( | ||
| '../../utils/processUtils.js' | ||
| ); | ||
| const { writeToStdout } = await import('@google/gemini-cli-core'); | ||
|
|
||
| await runExitCleanup(); | ||
| writeToStdout(` | ||
| ---------------------------------------------------------------- | ||
| Logging in with Google... Restarting Gemini CLI to continue. | ||
| ---------------------------------------------------------------- | ||
| `); | ||
| process.exit(RELAUNCH_EXIT_CODE); |
There was a problem hiding this comment.
This block of code for relaunching the application duplicates the logic found in relaunchApp from ../../utils/processUtils.js. Calling process.exit() directly from within a React hook is generally discouraged as it can lead to an abrupt termination of the application, bypassing React's cleanup lifecycle. It's better to use the existing relaunchApp utility, which is designed to handle this gracefully by including the necessary cleanup steps.
const { relaunchApp } = await import('../../utils/processUtils.js');
const { writeToStdout } = await import('@google/gemini-cli-core');
writeToStdout(`
----------------------------------------------------------------
Logging in with Google... Restarting Gemini CLI to continue.
----------------------------------------------------------------
`);
await relaunchApp();
gundermanc
force-pushed
the
gundermanc/tolerate-oauth-failures
branch
from
f2d75cc to
3e9b687
Compare
gundermanc
changed the title
gundermanc
force-pushed
the
gundermanc/tolerate-oauth-failures
branch
from
3e9b687 to
65544f6
Compare
gundermanc
force-pushed
the
gundermanc/tolerate-oauth-failures
branch
from
65544f6 to
507ffd4
Compare
NTaylorMullen
left a comment
NTaylorMullen
left a comment
There was a problem hiding this comment.
Awesome catch! Few questions:
-
I noticed the use of
writeToStdoutto display the cancellation instructions. Since we’re using Ink for the TUI, do you think there’s a risk of UI flickering or text overlap when writing directly to stdout? Would it be safer to usecoreEvents.emitFeedback('info', ...)instead to ensure the message is correctly handled by the UI engine? -
Regarding the error handling: In the PR description, you mentioned that cancellation currently causes an error stack to be printed to the console. I noticed that
useAuthCommandhas atry-catchblock that handles errors fromrefreshAuth—is that stack trace still appearing with this latest version of the PR, or was it specifically tied to the experiment with disabling raw mode? I just want to make sure we don't accidentally show a crash report to the user when they cancel.
I think it was specifically related to disabling raw mode. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary
Fixes an annoying issue where not selecting a Google account in the browser when prompted causes the terminal to hang for 5 minutes. Even Ctrl+C doesn't work when in this state.
The problem appears to be that the specialized buffering logic we do redirects all input, including that which handles Ctrl+C -> SIGINT translation.
I tried disabling raw mode while waiting for the user to auth and that seems to work ok though it causes an error stack to be printed to the console because the SIGINT bubbles up somewhere else instead of canceling the existing task.
Details
Related Issues
Fixes #15742
How to Validate
rm ~/.gemini/oauth_creds.jsonto delete cached creds.Pre-Merge Checklist