fix(auth): prevent re-triggering Google login after ESC cancellation - #2082
fix(auth): prevent re-triggering Google login after ESC cancellation#2082Red-Asuka wants to merge 1 commit into
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request aims to fix a bug where cancelling Google authentication could lead to an infinite loop of auth prompts. The proposed change correctly clears the selected auth type from user settings. My review identifies a scenario where this fix is incomplete—if the auth type is set in workspace settings—and suggests a more robust state-based approach to fully resolve the issue.
|
I think there's probably a more elegant way to do this that doesn't involve introducing new state to pass between files. The whole problem stems from the fact that we are saving the auth method to the settings before we know that they work. We shouldn't do that. Instead of handleAuthSelect just saving changes to settings we should change it to be a useState that simply stores the user's selection. then have the Effect only save that selection to the settings if performAuthFlow succeeds. You can make the changes or I can get to that later today. |
|
Actually. I realize I have a few bugs to fix in this area anyways. I'll take care of this today. |
|
@scidomino Thanks for the feedback! I've updated the PR to only save auth settings after successful authentication and removed the wasCancelled logic as suggested. Please take another look. |
|
This issue has been fixed in another PR so I will close this. |
Two reliability follow-ups mirroring the kimi-cli PR google-gemini#2082 review: - writeRuntimeStatus: switch the temp-file open from 'w' to 'wx' so the open succeeds only when the path does not already exist. The random UUID suffix already makes collision astronomically unlikely; the exclusive open (O_CREAT | O_EXCL) adds defense-in-depth against a pre-placed regular file or symlink at the temp path that 'w' would silently follow and overwrite. - tests: add a regression test asserting that no .tmp.* file is left behind when the underlying rename raises. We force the failure by pre-placing a non-empty directory at the target path, which makes fs.renameSync fail on every supported platform and exercises the catch-block tmp cleanup path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…witch Mirrors kimi-cli PR google-gemini#2082 commit 0f79e348. When the same PID switches to serving a different session id mid-flight, the previous session's runtime.json must be dropped before the new session's record is written; otherwise an external observer running a PID-liveness check would see this PID mapped to BOTH sessions and treat both as live. Gemini CLI has two such in-process session switches: - '/clear' (clearCommand) -> resetNewSessionState(<freshUUID>) - session-browser resume (useSessionBrowser) -> setSessionId(<resumedId>) Both paths now: 1. Capture the OLD session dir before changing the session id. 2. Switch the session id (resetNewSessionState / setSessionId). 3. clearRuntimeStatus(oldSessionDir) to drop the stale claim. 4. writeRuntimeStatus(newSessionDir, ...) so the PID is observable under the new session id from this point on. All four steps are best-effort; the runtime-status I/O is wrapped in try/catch so a write failure on slow/network/read-only filesystems never blocks the user-visible operation. Adds clearRuntimeStatus() to the runtime-status module: synchronous, swallows ENOENT/ENOTDIR (idempotent), all other I/O errors silenced. Three new tests: removes existing file, idempotent on missing file, no-op on missing dir. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two reliability follow-ups mirroring the kimi-cli PR google-gemini#2082 review: - writeRuntimeStatus: switch the temp-file open from 'w' to 'wx' so the open succeeds only when the path does not already exist. The random UUID suffix already makes collision astronomically unlikely; the exclusive open (O_CREAT | O_EXCL) adds defense-in-depth against a pre-placed regular file or symlink at the temp path that 'w' would silently follow and overwrite. - tests: add a regression test asserting that no .tmp.* file is left behind when the underlying rename raises. We force the failure by pre-placing a non-empty directory at the target path, which makes fs.renameSync fail on every supported platform and exercises the catch-block tmp cleanup path.
…witch Mirrors kimi-cli PR google-gemini#2082 commit 0f79e348. When the same PID switches to serving a different session id mid-flight, the previous session's runtime.json must be dropped before the new session's record is written; otherwise an external observer running a PID-liveness check would see this PID mapped to BOTH sessions and treat both as live. Gemini CLI has two such in-process session switches: - '/clear' (clearCommand) -> resetNewSessionState(<freshUUID>) - session-browser resume (useSessionBrowser) -> setSessionId(<resumedId>) Both paths now: 1. Capture the OLD session dir before changing the session id. 2. Switch the session id (resetNewSessionState / setSessionId). 3. clearRuntimeStatus(oldSessionDir) to drop the stale claim. 4. writeRuntimeStatus(newSessionDir, ...) so the PID is observable under the new session id from this point on. All four steps are best-effort; the runtime-status I/O is wrapped in try/catch so a write failure on slow/network/read-only filesystems never blocks the user-visible operation. Adds clearRuntimeStatus() to the runtime-status module: synchronous, swallows ENOENT/ENOTDIR (idempotent), all other I/O errors silenced. Three new tests: removes existing file, idempotent on missing file, no-op on missing dir.
TLDR
Fixed authentication dialog ESC key handling issues including Google login re-triggering and infinite retry loops. Implemented deferred auth settings persistence to preserve user configuration during cancellations, and enhanced error message handling to prevent ESC exit when errors are present.
Dive Deeper
Multiple ESC key handling issues were identified and resolved:
Auth cancellation bug: When user cancels Google authentication with ESC, subsequent ESC presses would incorrectly re-trigger the auth flow instead of showing proper error message.
Configuration corruption: The original approach of immediately saving selectedAuthType would break users' existing auth configurations in their settings.json when authentication fails or gets cancelled.
Infinite retry loops: When authentication fails with errors (e.g., missing environment variables), pressing ESC would cause infinite loops between "Waiting for auth" and error dialog.
The solution implements a deferred authentication pattern with the following improvements:
pendingAuthSelectionstate to track auth choices without immediately persisting them to settings|| errorMessagecondition), requiring users to resolve issues before proceedingReviewer Test Plan
Test Case 1: Auth Cancellation (Updated)
Test Case 2: Configuration Preservation (Enhanced)
Test Case 3: Error Message Handling (Enhanced)
Test Case 4: Deferred Persistence (New)
Testing Matrix
Linked issues / bugs
#1735 (comment)