Login Flow Improvements and Fallback Handling · Entire

Review: PR #1366 — login: default to browser sign-in, add --device fallback

Verdict: Approve. Solid PR — clean structure, good test coverage at both unit and integration levels, careful comments, and the security fundamentals (PKCE S256, loopback-only listener, URL scheme validation, token validation before persist) are all in place. Unit tests pass locally. Two real-world UX gaps worth considering before or shortly after merge.

What it does

entire login now defaults to the RFC 8252 loopback authorization-code flow with PKCE (via auth-go v0.5.0's authcode package) when an interactive terminal is present, falling back to the device-code flow otherwise. --device forces the old flow. Both flows converge on a shared persistLogin tail (validate → keyring → contexts.json).

Strengths

• Good factoring. Extracting persistLogin instead of duplicating the validate/save/record tail, and the browserAuthFlow interface mirroring the existing deviceAuthClient pattern, keeps login.go consistent with itself. shouldUseBrowserLogin as a pure function with a table test is the right shape.
• Test design. The integration test's trick — openBrowser reports failure under test so the fallback URL lands on stdout, then the test plays the browser against the loopback callback — exercises the real listener, real PKCE (code_verifier asserted at the token endpoint), and real state round-trip without spawning anything. The interactive.UnderTest() guards in waitForEnter and openBrowser follow the documented project pattern.
• Housekeeping done. The "before merge" item (re-pin from the auth-go branch pseudo-version to tagged v0.5.0) is already done in go.mod/go.sum. Error wrapping preserves sentinels for errors.Is (verified by TestRunBrowserLogin_WaitError), and the //nolint:wrapcheck comments explain why.

Issues worth considering

1. SSH with a TTY gets a broken default (cmd/entire/cli/login.go:79). CanPromptInteractively() is true over ssh -t, so the browser flow is chosen — but the loopback listener binds 127.0.0.1 on the remote host, which the user's local browser can't reach. Even the printed fallback URL can't complete. The user's only recourse is Ctrl-C and --device. gh and gcloud detect this (e.g. SSH_CONNECTION/SSH_TTY env vars) and route to the code flow. Suggest adding an SSH check to shouldUseBrowserLogin, or at minimum mentioning --device in the "Waiting for sign-in..." context so a stuck user knows the escape hatch.

2. Wait has no deadline (login.go:184). The device flow bounds waiting via expires_in (capped at 15 min); the browser flow waits on the command context, which has no timeout — if the user closes the browser tab without completing, entire login hangs until Ctrl-C. A context.WithTimeout around flow.Wait (5–15 min) would match the device flow's behavior and most peer CLIs.

Minor / non-blocking

• No fallback when the listener can't start (login.go:80-83). If StartBrowserAuth fails (port bind, firewall), the command errors out rather than falling back to the device flow. Falling back here would be friendlier and costs little.
• TestLogin_BrowserFlow_SavesToken doesn't assert the saved token — it checks "Login complete." only. This matches the existing TestLogin_SavesTokenAfterApproval convention exactly, so it's fine as-is, but both names slightly overpromise; asserting the ENTIRE_TEST_AUTH_STORE_FILE contents would make them honest. Could be a follow-up touching both.
• waitForBrowserPrompt's 10s deadline only fires between reads (a ReadString block ignores it) — but this mirrors the pre-existing waitForLoginPrompt, and the pipe closes when the process exits, so it's not a new hazard.

Security notes

All good: PKCE with S256 (verified against prod discovery doc per the PR body), state round-tripped and validated by the authcode lib, openBrowser still rejects non-HTTP(S) URLs before the test guard, token iss/exp cross-checked before persisting, and the loopback redirect was verified registered server-side for any-port 127.0.0.1/callback. The full authorize URL (with PKCE challenge) deliberately isn't printed on the happy path — nice touch.