Skip to content

auth: review follow-ups + keyring-safe cli TestMain - #1360

Merged
Soph merged 3 commits into
mainfrom
auth-review-followups
Jun 4, 2026
Merged

auth: review follow-ups + keyring-safe cli TestMain#1360
Soph merged 3 commits into
mainfrom
auth-review-followups

Conversation

@Soph

@Soph Soph commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/511

Follow-ups from review of the (now-merged) auth-context-consolidation work, plus a test-isolation fix.

Changes

auth: review follow-ups (3eeb08c)

  • CLAUDE.md: correct the auth command surface — login/logout/status/contexts/use (drop the removed list/revoke) and document logout's --everywhere / --all-contexts flags.
  • logout: runLogout now takes a single caller-selected revoke func instead of both revokeCurrent+revokeAll plus an all bool — the command already knows --everywhere. Dropped the now-obsolete TestRunLogout_AllRevokesAllSessions (selection is covered end-to-end by TestLogoutCommand_FlagMatrix) and updated the simple callers.
  • Fixed stale method names in //nolint:wrapcheck comments left over from the SessionAuthSession rename.

test: mock the OS keyring in the cli package TestMain (1ea54e2)

  • The cli package's TestMain never routed go-keyring to its in-memory mock (unlike the auth subpackage). The default tokenstore backend is the real OS keychain, so a cli test reaching a credential path without UseFileBackendForTesting — or running in the window after such a test restores the global backend — could read the developer's real keychain and trigger a macOS unlock prompt during mise run test:ci.
  • Calls keyring.MockInit() once in TestMain, mirroring cmd/entire/cli/auth's TestMain, so no cli test can touch the real keychain. This closes a latent, pre-existing gap (not introduced by the auth branch).

Testing

  • go build ./..., targeted cli tests pass
  • mise run fmt + mise run lint — 0 issues

🤖 Generated with Claude Code


Note

Low Risk
Local CLI auth/logout and test isolation only; behavior for --everywhere is unchanged and still covered by the flag-matrix e2e test.

Overview
Docs: CLAUDE.md now documents the consolidated auth surface (contexts, use; no list/revoke) and logout’s --everywhere / --all-contexts flags.

Logout refactor: runLogout takes one caller-chosen revoke func instead of revokeCurrent + revokeAll and an all bool—the Cobra handler already selects current vs all sessions via --everywhere. Unit tests were updated; TestRunLogout_AllRevokesAllSessions was removed in favor of TestLogoutCommand_FlagMatrix.

Tests: cli package TestMain calls keyring.MockInit() so tests never touch the real OS keychain (avoids macOS unlock prompts). Stale //nolint:wrapcheck comments were aligned with AuthSession naming.

Reviewed by Cursor Bugbot for commit 1ea54e2. Configure here.

Soph and others added 2 commits June 4, 2026 11:25
- CLAUDE.md: correct the `auth` command surface (login/logout/status/
  contexts/use; drop the removed list/revoke) and document logout's
  --everywhere / --all-contexts flags.
- logout: have runLogout take a single caller-selected revoke func
  instead of both revokeCurrent+revokeAll plus an `all` bool; the
  command already knows --everywhere. Drop the now-obsolete
  TestRunLogout_AllRevokesAllSessions (selection is covered end-to-end
  by TestLogoutCommand_FlagMatrix) and update the simple callers.
- Fix stale method names in //nolint:wrapcheck comments left over from
  the Session -> AuthSession rename.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 7bbdd7fd89c1
The cli package's TestMain never routed go-keyring to its in-memory mock,
unlike the auth subpackage. The default tokenstore backend is the real OS
keychain, so any cli test that reaches a credential path without
UseFileBackendForTesting — or runs in the window after such a test restores
the global backend — would read the developer's real keychain and trigger a
macOS unlock prompt during `mise run test:ci`.

Call keyring.MockInit() once in TestMain so no cli test can touch the real
keychain, mirroring cmd/entire/cli/auth's TestMain.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: eb9e9029effe
@Soph
Soph requested a review from a team as a code owner June 4, 2026 09:40
Copilot AI review requested due to automatic review settings June 4, 2026 09:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request applies follow-up refinements to the auth/logout surface and hardens test isolation by ensuring the CLI package never hits the real OS keychain during unit tests.

Changes:

  • Refactors logout internals so runLogout takes a single caller-selected revoke function (current-session vs all-sessions selection remains in the Cobra command).
  • Updates //nolint:wrapcheck comments to match AuthSession-renamed method names and removes an obsolete unit test now covered by the command flag-matrix test.
  • Adds keyring.MockInit() to the cli package TestMain to prevent any tests from accessing the developer’s real keychain.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cmd/entire/cli/logout.go Simplifies logout’s revocation injection and refreshes wrapcheck annotations.
cmd/entire/cli/logout_test.go Updates unit tests for the new runLogout signature and removes a redundant test.
cmd/entire/cli/global_test.go Mocks go-keyring globally for the cli package test suite to avoid OS keychain access.
cmd/entire/cli/auth.go Fixes stale wrapcheck comment text after method renames.
CLAUDE.md Updates documented auth command surface and logout flags to match current behavior.

Comment thread cmd/entire/cli/logout.go Outdated
nodo
nodo previously approved these changes Jun 4, 2026
The single revoke func runLogout takes can now represent either the
current session or every session on the core (--everywhere), so the
old "Current" name was misleading. Rename to boundRevokeFunc and note
the current-or-all semantics — it contrasts with the unbound
revokeTargetFunc (coreURL + token still open) used by the --all-contexts sweep.

Addresses PR #1360 review feedback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 1e0358381bae

@toothbrush toothbrush left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the cleanup 🫡

@Soph
Soph enabled auto-merge June 4, 2026 09:59
@Soph
Soph merged commit e3104a6 into main Jun 4, 2026
9 checks passed
@Soph
Soph deleted the auth-review-followups branch June 4, 2026 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants