fix(auth): Ensure auth URL is always visible in headless environments #13867
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 @JuanCS-Dev, 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 resolves a critical regression in version 0.18.0 that prevented the authentication URL from being displayed in headless environments, leading to an indefinite hang during login. The fix ensures that the authentication URL and related messages are always visible by writing them directly to "stderr", bypasses fatal errors when the browser fails to launch, and improves user guidance, thereby restoring seamless authentication for all environments. 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
|
![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 effectively addresses the authentication issue in headless environments by ensuring the authentication URL is always visible. Writing auth-related messages directly to stderr is a good solution to bypass stdio patching. Removing the fatal error when the browser fails to open is a great improvement, as it allows for manual authentication and makes the CLI more robust in various environments. I've identified one area for improvement regarding code duplication in error handling, which I've detailed in a specific comment. Overall, these are solid changes that improve the user experience for authentication.
| childProcess.on('error', (error) => { | ||
| const errorMsg = | ||
| `Failed to open browser with error: ${getErrorMessage(error)}\n` + | ||
| `Please visit the URL above or run again with NO_BROWSER=true set.\n`; | ||
| writeToStderr(errorMsg); | ||
| coreEvents.emit(CoreEvent.UserFeedback, { | ||
| severity: 'error', | ||
| message: | ||
| `Failed to open browser with error: ${getErrorMessage(error)}\n` + | ||
| `Please try running again with NO_BROWSER=true set.`, | ||
| message: errorMsg, | ||
| }); | ||
| }); | ||
| } catch (err) { | ||
| const errorMsg = | ||
| `Failed to open browser with error: ${getErrorMessage(err)}\n` + | ||
| `Please visit the URL above or run again with NO_BROWSER=true set.\n`; | ||
| writeToStderr(errorMsg); | ||
| coreEvents.emit(CoreEvent.UserFeedback, { | ||
| severity: 'error', | ||
| message: | ||
| `Failed to open browser with error: ${getErrorMessage(err)}\n` + | ||
| `Please try running again with NO_BROWSER=true set.`, | ||
| message: errorMsg, | ||
| }); | ||
| throw new FatalAuthenticationError( | ||
| `Failed to open browser: ${getErrorMessage(err)}`, | ||
| ); | ||
| // Don't throw here - let the user complete auth manually via the URL | ||
| // The loginCompletePromise will handle timeout if auth never completes | ||
| } |
There was a problem hiding this comment.
The error handling logic for when open fails is duplicated in the childProcess.on('error', ...) handler and the catch (err) block. This can lead to maintenance issues where one is updated but the other is forgotten. To improve maintainability and ensure consistency, this logic should be extracted into a single helper function.
const handleBrowserOpenError = (error: unknown) => {
const errorMsg =
`Failed to open browser with error: ${getErrorMessage(error)}\n` +
`Please visit the URL above or run again with NO_BROWSER=true set.\n`;
writeToStderr(errorMsg);
coreEvents.emit(CoreEvent.UserFeedback, {
severity: 'error',
message: errorMsg,
});
};
childProcess.on('error', handleBrowserOpenError);
} catch (err) {
handleBrowserOpenError(err);
// Don't throw here - let the user complete auth manually via the URL
// The loginCompletePromise will handle timeout if auth never completes
}
- Write auth URL directly to stderr using writeToStderr to bypass stdio patching - Don't throw error when browser fails to open, allow manual auth completion - Improves error messages to reference the URL shown above - Fixes regression where URL was not visible in v0.18.0 headless environments Fixes google-gemini#13853
|
Thanks for the quick turnaround on this, @JuanCS-Dev !! Since I have the exact headless environment (Ubuntu VPS via SSH) where the regression was originally found, I'm available to verify/test this fix immediately to ensure it works as expected. Is there a preferred way you'd like me to test this PR (e.g., pulling this specific branch and building locally) or should I wait for a nightly build? |
JuanCS-Dev
force-pushed
the
fix-headless-auth-13853
branch
from
755c486 to
4798eca
Compare
|
This is already fixed in main by #13565 Fix multiple bugs with auth flow including using the implemented but unused restart support. Please let me know if it still happens. |
4 participants
Fixes #13853
Description
Fixes regression in v0.18.0 where authentication URL was not visible in headless environments (SSH/Docker), causing the CLI to hang indefinitely during login.
Root Cause
The commit bdf80ea (#13600) introduced stdout/stderr patching that prevented the auth URL from being displayed in non-interactive environments.
Changes
writeToStderr()to bypass stdio patchingTesting
npm run build✅Impact