Auth Review Followups and Tests · Entire

can you review the changes in this branch / PR

1mo ago·41m·18 calls

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:

- `auth`: `login`, `logout`, `status`, `list`, `revoke`

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.