Auth Review Followups and Tests

I have a complete picture now. The branch builds clean, all tests pass, and there are no dangling references to the removed symbols. Here's my review.

Review: auth-context-consolidation

This is a well-executed PR. It pivots auth/logout away from data-API personal-access-token (PAT) management toward login-session management on entire-core (OAuth refresh-token families), removes the auth list/auth revoke commands, and reworks logout around a --everywhere / --all-contexts matrix. Build is clean, go test ./cmd/entire/cli/... ./internal/coreapi/... passes, no dangling refs to AuthTokensPath/ListTokens/RevokeToken/RemoveAllContexts.

🔴 Must verify (can't confirm from this repo)

1. The endpoint route changed, and unit tests can't catch a mismatch. The path moved from /api/v1/auth/tokens (routed at the data API, api.BaseURL()) to a hardcoded coreAuthSessionsPath = "/api/auth/tokens" on the auth host (auth.go:33). Note both the host and the v1 segment changed. Every test uses an httptest mock that echoes coreAuthSessionsPath, so a real route mismatch would pass CI and only fail at runtime against entire-core.

Before merging, confirm entire-core actually serves, with the CLI's entire:session-scoped JWT:

• GET /api/auth/tokens (list) returning the {"tokens":[…]} envelope
• DELETE /api/auth/tokens/current
• DELETE /api/auth/tokens/{id}

The backend repo isn't checked out here, so I couldn't statically verify it.

🟡 Should fix

2. CLAUDE.md:36 is now stale. It still documents:

This PR removes list and revoke and adds contexts/use. CLAUDE.md explicitly mandates keeping the command-layout section current — please update it (and the auth Short/Long already updated in code is a good reference for the new surface).

🟢 Minor / nits

3. Stale names in //nolint:wrapcheck comments. Several reference pre-rename methods — logout.go:945 says "RevokeCurrentSession", :957/:961 say "ListSessions"/"RevokeSession", auth.go:282 says "ListSessions". The methods are now RevokeCurrentAuthSession / ListAuthSessions / RevokeAuthSession. Cosmetic.

4. runLogout re-selects a revoke func the caller already chose. newLogoutCmd knows everywhere, yet passes both revokeCurrent and revokeAll plus the bool into runLogout, which then re-picks (logout.go:986-989). You could resolve a single revoke closure in the command and give runLogout one func — less surface, fewer params. (Not a bug; the matrix test confirms current behavior is correct.)

5. isKeychainTokenRejected string-matches "code 401" (auth.go:139). Fragile, but it's commented and a reasonable last-resort for non-JSON gateway 401s that bypass ogen's typed decode. Fine to keep; just noting it's a heuristic.

6. --everywhere can collaterally kill sibling contexts' sessions. It lists+revokes all sessions on the active core. If another saved context is the same user on the same core, those sessions die server-side too, but that context stays in contexts.json and promoteNextLogin may promote it with a now-dead token. Edge case — worth a sentence in the --everywhere help text or a follow-up.

What's good

• Test coverage is excellent. TestLogoutCommand_FlagMatrix pins all four --all-contexts×--everywhere quadrants end-to-end through the real cobra command against two fake cores, asserting exactly which revoke shape each core received. The soft-note path (auth status when session listing fails but /me succeeded) is covered too.
• Good TLS hygiene. Because the active context's CoreURL can differ from AuthBaseURL, both auth status and logout re-run api.RequireSecureURL(target.coreURL) before sending the bearer, and runLogoutAll checks each context's core individually — skipping the server revoke (but still removing locally) on a non-TLS core.
• Clean consolidation. RemoveCurrentContext/RemoveContext now share deleteContextKeychain; renderAlignedTable is extracted and reused by the sessions table and the contexts table; the ContextStore.GetToken seam (prefer active context, fall back to legacy keyring) keeps the gate in runLogout and the revoke closures consistent. The PAT machinery and Provider.AuthTokensPath are removed without leftovers.
• Sensible best-effort semantics in the sweep: per-context failures warn and continue, local removal always proceeds, and the legacy keyring entry is cleared at the end.

Net: solid, well-tested refactor. The one thing standing between this and a clean merge is runtime confirmation of the new core route (#1) — everything else is documentation/polish.

All green. Done — here's what I changed:

CLAUDE.md (#2) — cmd/entire/cli/CLAUDE.md... actually CLAUDE.md:36: replaced the stale auth: login, logout, status, list, revoke line with the real surface (login, logout, status, contexts, use) plus the logout --everywhere / --all-contexts flags.

Nit #3 — stale names in nolint/doc comments:

• auth.go:271 ListSessions → ListAuthSessions
• logout.go RevokeCurrentSession → RevokeCurrentAuthSession, and the ListSessions and RevokeSession block comment + nolint → ListAuthSessions/RevokeAuthSession.

Nit #4 — single resolved revoke func:

• runLogout now takes one revoke revokeCurrentFunc and drops the revokeAll param and the all bool; the internal re-selection is gone.
• newLogoutCmd selects via the existing revokeForTarget (set from everywhere) — that variable is now used by both the --all-contexts path and the single-target path, instead of being recomputed.
• Updated the 6 simple runLogout test callers to the new signature.
• Removed TestRunLogout_AllRevokesAllSessions — it pinned the selection logic that has moved up to the command, where it's already covered end-to-end by TestLogoutCommand_FlagMatrix's --everywhere quadrant.

go build ./..., the logout/auth tests, mise run fmt, and mise run lint (0 issues) all pass.

One thing I left for you: the broader mise run test:ci (full suite + integration + canary) — I ran the targeted package tests but not the full CI gate. Want me to kick that off before you commit?

Yes — that popup was real, and your instinct was right to stop and ask. Here's what was happening and what I changed.

Why the popup happened

The token store defaults to the real macOS keychain (keyringStore → keyring.Get/Set/Delete) unless a test swaps it out. Tests isolate it two ways:

• per-test tokenstore.UseFileBackendForTesting(...), or
• a TestMain that calls keyring.MockInit() to route the whole package to an in-memory mock.

The auth subpackage's TestMain does keyring.MockInit(). The cli package's TestMain (global_test.go) did not. So in the cli test binary, any credential access outside an explicit file-backend window lands on your real keychain — and you now have a real entry there (entire-core:https://us.auth.entire.io / Soph, from ~/.config/entire/contexts.json), so macOS prompts for the unlock password.

The trigger is structural, not one specific test: UseFileBackendForTesting swaps the global backend and, on cleanup, restores it to the real keychain. With the cli package's many t.Parallel() tests interleaving around the (newer) real-tokenstore tests in auth_context_test.go, a store read can fall on the restored keychain backend. This hazard predates your branch (those file-backend tests existed on main), but the branch widens the surface and your real us.auth.entire.io login is what makes it actually prompt now.

Fix

Added keyring.MockInit() to the cli package TestMain, mirroring the auth subpackage. The default backend now points at an in-memory mock, so the cli test binary can never touch your real keychain — regardless of which test forgot isolation or what the global backend gets restored to. Tests using UseFileBackendForTesting are unaffected (they replace the backend wholesale; after cleanup it falls back to the mock, not the OS keychain).

go test ./cmd/entire/cli/ passes, mise run lint is clean. This should be popup-free now — but since you're the one who can see the dialog, please run mise run test:ci and confirm no prompt appears. If one still does, it'd be coming from a different package's test binary (the auth subpackage and integration/canary subprocesses are already isolated, but if a prompt persists we can grep the remaining packages' TestMains the same way).

Want me to run the full mise run test:ci now, or do you want to run it yourself to watch for the dialog?

I have a definitive answer now. Let me state it with the evidence.

No — this branch did not introduce it.

What test:ci actually runs: core (in-process go test across all packages, incl. cmd/entire/cli) + integration (subprocess shards). The canary isn't part of it.

Evidence the branch is not the cause:

1. Isolation infra is byte-identical to main. git log main..HEAD shows the three files that govern keychain isolation — cli/global_test.go (the cli TestMain), integration_test/setup_test.go, and integration_test/login_test.go — were not touched on this branch.

2. No test reads the real keychain without a guard — on either branch. I scanned every cli test function for real-config/keychain readers (tokenstore.Get/Set/Delete, auth.Contexts, resolveStatusTarget, LoginTokenForContext, RecordLoginContext, …) lacking an isolation guard (ENTIRE_CONFIG_DIR / UseFileBackendForTesting / SetManagerForTest / mem store). Result: empty on both main and this branch.

3. The branch's new real-store tests are all guarded. auth_context_test.go (the 4th test added) and the logout flag-matrix both set ENTIRE_CONFIG_DIR + UseFileBackendForTesting (the matrix via seedTwoContexts). Integration login_test.go drives the subprocess with ENTIRE_TEST_AUTH_STORE_FILE + the authfilestore build tag — file store, never the keychain.

4. The gap that allows a popup is pre-existing. The cli package TestMain never called keyring.MockInit() on main either — only the auth subpackage did. So the latent "a cli test could reach the real keychain" hole has been there all along; the branch didn't open it.

So what was the popup? Almost certainly a pre-existing path, not branch code. The most plausible trigger is a command-.Execute() test (e.g. activity/search/dispatch) where token resolution happens inside production code — invisible to a line scan, and unchanged by this branch — landing on your real keychain while it was locked (a locked login keychain prompts for the password on the first access, even a write/miss). That, combined with you now having a real us.auth.entire.io login, is why you'd notice it "now."

My change still stands as the right fix. Adding keyring.MockInit() to the cli TestMain closes the latent gap for the whole package — matching the auth subpackage's existing precedent — so no cli test can ever reach the OS keychain regardless of which path it exercises. It's hardening, not a fix for a branch-introduced regression.

One caveat on honesty: I established this statically, not by reproducing on main (running main's suite could itself pop the dialog on your machine). If you want certainty, I can bisect by temporarily forcing the real backend under a controlled run — but I'd rather not trigger more prompts unless you want that.

Want me to also add the same keyring.MockInit() guard to the integration package's TestMain (its in-process code doesn't currently mock it, even though the subprocess is covered)? That'd make the whole test:ci chain keychain-proof.

Done. Committed as af7e7cd79 on auth-context-consolidation:

• CLAUDE.md — corrected the auth command surface + documented logout --everywhere / --all-contexts
• logout.go / logout_test.go — runLogout takes a single revoke func (nit #4); removed the obsolete selection test
• auth.go / logout.go — stale nolint comment names (nit #3)

The keyring test fix (cmd/entire/cli/global_test.go — the keyring.MockInit() in TestMain) is left unstaged in your working tree, as you asked, for you to carry to a fresh branch off main.

One heads-up for when you do that: the change is a one-line addition plus the github.com/zalando/go-keyring import. If you branch off main and git stash pop / cherry-pick the working-tree change there, it'll apply cleanly since global_test.go is unchanged on both main and this branch. If you'd rather I save the exact diff somewhere durable (e.g. a patch file) so it survives branch switching, say the word — otherwise the uncommitted edit will follow you across git checkout as long as you don't discard it.

Done. New branch auth-review-followups, based off the updated origin/main (which now includes the merged PR #1341), with two commits:

• The review-follow-ups commit was cherry-picked off the old branch onto fresh main (auto-merged cleanly — including over main's "say 'login server' not 'core'" wording change).
• The keyring test fix is now committed here too, as its own commit.
• Build clean, targeted cli tests pass, mise run fmt + mise run lint (0 issues), working tree clean.

One note on "move": the original commit af7e7cd79 still sits on the now-merged/stale auth-context-consolidation branch (local and origin/). I didn't rewrite that branch since it's tied to an already-merged PR and is presumably slated for deletion — the content now lives properly on auth-review-followups as 3eeb08cb6. If you'd like, I can delete the local auth-context-consolidation branch (and/or git push origin --delete the remote one) to tidy up. Want me to, or are you good to open the PR for auth-review-followups?