Address PR #1404 Login Review Comments · Entire

PR Comments on #1404

Let's work through them in turn, the bot might not always be right.

1. Bugbot "logout deletes wrong legacy key" (medium) — half right, fixed differently. Its revocation claim was wrong: logout's ContextStore.GetToken prefers the active context's token, and the revoke closure uses the context-resolved target, so server-side revocation works fine. It found a real defect: login --server <non-default> wrote a legacy keyring entry under that origin, a key no reader ever consults (every legacy read keys by api.AuthBaseURL(), which is always the default now) and which logout could never delete. Rather than re-keying logout, I fixed the write side: the legacy entry is only written for the default server, and for a non-default server RecordLoginContext failure now fails the login.

2. Bugbot userinfo leak (low) — right, applied. All post-parse parseLoginServer errors now echo u.Redacted() instead of the raw value — including the scheme/host branches, which could also carry a password (e.g. REDACTED hits the scheme check first).

3. Copilot loop-var capture — wrong, no change. Go ≥1.22 has per-iteration loop variables and this repo is on 1.26.4; adding tc := tc would itself get flagged by copyloopvar. Worth dismissing with that note.

Bonus: the branch's CI was already red (test-integration (c)), unrelated to the bot comments. The login integration tests still exported the retired ENTIRE_AUTH_BASE_URL (so the binary died at the new gate), and their fake server served the v1 /oauth/device/code path while the now-default split-host v2 provider posts to /device_authorization. The tests now pass --server, serve the v2 path, mint an iss-bearing fake JWT and sandbox ENTIRE_CONFIG_DIR/token store.

mise run check is fully green (lint, unit, integration, canary). I haven't replied to the comments on GitHub — say the word if you want me to post responses to the two bots.

New Commit: Pushed — 4a292044fd4 is on the PR branch; CI should go green now that the integration tests use --server.

Responding to Reviewer Comments: All three replies posted:

• Bugbot logout-key — acknowledged as real, explained the write-side fix in 4a292044fd4 and why revocation was already safe.
• Bugbot userinfo leak — fixed in 4a292044fd4, noted all post-parse branches now use u.Redacted().
• Copilot loop-var — dismissed: per-iteration loop variables since Go 1.22, repo is on 1.26.4.

Additional Review Comments

Oh no, more review comments on the PR! Two genuinely new comments:

• Bugbot "unset env test skips wrongly" (low) — right, fixed. The skip guard now uses os.LookupEnv.
• Bugbot "login integration inherits retired env" (medium) — declining, per your point. The only way the spawned binary inherits that var is a shell that still exports it, and in that shell every entire invocation already fails loudly with the remediation message.

Commit on New Comments: Done — committed 1c64a23005d. Fixed where necessary:

• README.md was updated to not tell devs to export ENTIRE_AUTH_BASE_URL=... for local auth testing.
• The smoke script has been corrected to match updated outputs and verification paths.

Final Notes: Two commits are now sitting unpushed (1c64a23 skip-guard fix, f41d01eb86d doc/script cleanup), plus pending replies to the two Bugbot comments.