## 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.