Address PR #1404 Login Review Comments
have a look at the PR comments on #1404. let's work through them in turn, the bot might not always be right.
I'll fetch the PR and its review comments.
have a look at the PR comments on #1404. let's work through them in turn, the bot might not always be right.
Done — committed as 4a292044fd4 (not pushed). Here's the verdict on each comment, plus one thing the bots missed:
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. But 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 (the context is the only record, so warn-and-continue would print "Login complete." while storing nothing usable).
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 (context recording needs claims — an opaque token can't be a usable non-default login anymore), and sandbox ENTIRE_CONFIG_DIR/token store so the spawned binary can't touch your real ~/.config/entire or keychain.
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.
push
Pushed — 4a292044fd4 is on the PR branch; CI should go green now that the integration tests use --server.
respond to the reviewer comments as appropriate. just one sentence each, with commit referenced as appropriate.
All three replies posted:
- Bugbot logout-key — acknowledged as real, explained the write-side fix in
4a292044fd4and why revocation was already safe. - Bugbot userinfo leak — fixed in
4a292044fd4, noted all post-parse branches now useu.Redacted(). - Copilot loop-var — dismissed: per-iteration loop variables since Go 1.22, repo is on 1.26.4.
oh no, more review comments on https://github.com/entireio/cli/pull/1404!
Two genuinely new comments (the first three are the ones already handled). Let me look at the flagged test.
wait wait why are we hard-unsetting ENTIRE_AUTH_BASE_URL? Surely it's not being set anywhere? Taht seems superstitious.
Done — committed 1c64a23005d (not pushed). Where the two new comments landed:
- Bugbot "unset env test skips wrongly" (low) — right, fixed. The skip guard now uses
os.LookupEnv, matching the gate's present-even-if-empty check. - 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
entireinvocation already fails loudly with the remediation message — that's the PR's deliberate behavior, and the harness masking it would undercut it. I had started addingos.Unsetenvto both TestMains and reverted that; the commit message records the rationale.
Say the word and I'll push and reply to both comments (fixed-in-1c64a23 for the skip guard; working-as-designed for the env inheritance).
are there any other occurrences of ENTIRE_AUTH_BASE_URL in the codebase, and can we remove them?
Yes — there were quite a few, and I've removed every reference that was wrong or misleading, in commit f41d01eb86d (not pushed). Breakdown:
Fixed now (was actively broken by this PR):
README.md— told devs toexport ENTIRE_AUTH_BASE_URL=...for local auth testing, which now bricks every command; rewritten aroundentire login --server.scripts/local-device-auth-smoke.sh— exported the var before running login (dies at the gate). It was stale in two more ways: it grepped forApproval URL:which login hasn't printed in a while (Login URL:now), and it verified the legacyauth.jsonentry, which a--serverlogin correctly no longer writes. It now uses--server, the right output strings, and verifies the contexts.json entry instead.internal/coreapi/client_test.go— a test sentinel still advisedrun ENTIRE_AUTH_BASE_URL=... entire login; now saysentire login --server ....
Fixed now (stale comments/docs describing it as a live mechanism): control_plane.go, exchange.go, data_api.go, api/client.go, coreapi/client.go, one test comment, and docs/architecture/upstream-host-resolution.md (which also referenced the already-removed AuthBaseURLOverridden).
Deliberately left for part 2: the env read inside api.AuthBaseURL(), the AuthBaseURLEnvVar const (the gate needs it for its error message), and the ~6 test files that t.Setenv it (base_url_test.go, provider_test.go, store_test.go, data_api_test.go, control_plane_test.go). Those are exactly the "static fallbacks / v1 provider / legacy-token migration" demolition the PR description promises as the follow-up, and pulling them forward would mean rewriting all the env-driven provider/fallback tests in this PR.
Two commits are now sitting unpushed (1c64a23 skip-guard fix, f41d01e doc/script cleanup), plus the pending replies to the two newest Bugbot comments — ready to push and respond whenever you say.