feat(auth): tolerate AUTH_STORE_READ_FAILED in logout --user (2/4)

README.md

@@ -213,7 +213,7 @@ attachTokenViewCommand<Account>(auth, {
 })
 ```
 
-`attachLogoutCommand` snapshots `store.active()` (only when `revokeToken` or `onCleared` is supplied — skipped otherwise to avoid keyring / file I/O), calls `store.clear()`, then awaits `revokeToken({ token, account, view, flags })` for best-effort server-side revocation, emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`), and finally fires `onCleared({ account, view, flags })`. Local clear runs first so logout always succeeds locally even if the snapshot read fails or the server is unreachable; both `store.active()` and `revokeToken` errors are swallowed (`revokeToken` is also skipped when no session was stored). The exported `AttachLogoutRevokeContext<TAccount>` is the ctx type for typing standalone revoke implementations.
+`attachLogoutCommand` snapshots `store.active(ref)` when either `--user <ref>` is supplied or one of the consumer hooks (`revokeToken` / `onCleared`) needs the prior account, calls `store.clear(ref)`, awaits `revokeToken({ token, account, ref, view, flags })` for best-effort server-side revocation, emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`), and finally fires `onCleared({ account, ref, view, flags })`. `ref` is the parsed `--user` argument (or `undefined`) so consumers can distinguish "nothing was stored" (`account: null`, `ref: undefined`) from "cleared an unreadable record by ref" (`account: null`, `ref: "me"`). `revokeToken` failures are always swallowed; the pre-flight snapshot's error contract is covered in the `--user <ref>` section below. The exported `AttachLogoutRevokeContext<TAccount>` is the ctx type for typing standalone revoke implementations.
 
 `attachStatusCommand` reads `store.active()`, optionally runs `fetchLive` (consumer translates 401 → `CliError('NO_TOKEN', …)`), then dispatches to `renderJson` (`--json` / `--ndjson` via `formatJson` / `formatNdjson`, defaults to the account itself, **only invoked in machine-output mode**) or `renderText` (human mode, string or array of lines). When the store is empty it throws `CliError('NOT_AUTHENTICATED', 'Not signed in.')` unless `onNotAuthenticated` is supplied.
 
@@ -338,6 +338,8 @@ Account-selection resolvers (env > `--user` > default > single-only > error), `a
 
 `ACCOUNT_NOT_FOUND` is thrown by the account-touching attachers when `--user <ref>` was supplied but `store.active(ref)` returned `null`. `NO_ACCOUNT_SELECTED` is reserved for consumer-thrown resolver failures (multiple accounts stored, no default, no `--user`); cli-core does not throw it itself.
 
+A `TokenStore` MAY throw `CliError('AUTH_STORE_READ_FAILED', …)` from `active(ref)` when a matching record exists but the token itself can't be read (e.g. an OS keyring backing the store is offline). `attachLogoutCommand` catches this specific code on the explicit-ref path and proceeds with `clear(ref)` — local logout doesn't need the token, and the `revokeToken` hook is skipped because there's no token to send. Every other error from `active(ref)` (notably `ACCOUNT_NOT_FOUND` from a genuine ref miss, plus any consumer-thrown code) still propagates so a real miss isn't masked. Without `--user`, the logout pre-flight swallows any snapshot read failure so the local clear always runs. `attachStatusCommand` and `attachTokenViewCommand` propagate `AUTH_STORE_READ_FAILED` since they have no way to render or print without the token.
+
 #### Custom `AuthProvider` (non-PKCE flows)
 
 Implement `AuthProvider` directly for Dynamic Client Registration, device code, magic-link, etc. The four hooks fire in this order during `runOAuthFlow`:

src/auth/errors.ts

@@ -9,6 +9,7 @@ export type AuthErrorCode =
     | 'AUTH_PORT_BIND_FAILED'
     | 'AUTH_TOKEN_EXCHANGE_FAILED'
     | 'AUTH_STORE_WRITE_FAILED'
+    | 'AUTH_STORE_READ_FAILED'
     | 'NOT_AUTHENTICATED'
     | 'TOKEN_FROM_ENV'
     | 'NO_ACCOUNT_SELECTED'

src/auth/logout.test.ts

@@ -74,6 +74,7 @@ describe('attachLogoutCommand', () => {
         expect(logSpy).toHaveBeenCalledWith('✓ Logged out')
         expect(onCleared).toHaveBeenCalledWith({
             account,
+            ref: undefined,
             view: { json: false, ndjson: false },
             flags: {},
         })
@@ -87,6 +88,7 @@ describe('attachLogoutCommand', () => {
         expect(logSpy).toHaveBeenCalledWith(formatJson({ ok: true }))
         expect(onCleared).toHaveBeenCalledWith({
             account,
+            ref: undefined,
             view: { json: true, ndjson: false },
             flags: {},
         })
@@ -100,6 +102,7 @@ describe('attachLogoutCommand', () => {
         expect(logSpy).not.toHaveBeenCalled()
         expect(onCleared).toHaveBeenCalledWith({
             account,
+            ref: undefined,
             view: { json: false, ndjson: true },
             flags: {},
         })
@@ -114,6 +117,7 @@ describe('attachLogoutCommand', () => {
         expect(built.clearSpy).toHaveBeenCalledTimes(1)
         expect(onCleared).toHaveBeenCalledWith({
             account: null,
+            ref: undefined,
             view: { json: false, ndjson: false },
             flags: {},
         })
@@ -149,6 +153,7 @@ describe('attachLogoutCommand', () => {
         expect(built.clearSpy).toHaveBeenCalledWith('me@example')
         expect(onCleared).toHaveBeenCalledWith({
             account,
+            ref: 'me@example',
             view: { json: true, ndjson: false },
             flags: { full: true },
         })
@@ -188,6 +193,7 @@ describe('attachLogoutCommand', () => {
         expect(revokeToken).toHaveBeenCalledWith({
             token: 'tok',
             account,
+            ref: undefined,
             view: { json: false, ndjson: false },
             flags: {},
         })
@@ -242,6 +248,7 @@ describe('attachLogoutCommand', () => {
         expect(built.clearSpy).toHaveBeenCalledTimes(1)
         expect(onCleared).toHaveBeenCalledWith({
             account,
+            ref: undefined,
             view: { json: false, ndjson: false },
             flags: {},
         })
@@ -259,6 +266,7 @@ describe('attachLogoutCommand', () => {
         expect(revokeToken).not.toHaveBeenCalled()
         expect(onCleared).toHaveBeenCalledWith({
             account: null,
+            ref: undefined,
             view: { json: false, ndjson: false },
             flags: {},
         })
@@ -297,6 +305,7 @@ describe('attachLogoutCommand', () => {
         expect(revokeToken).toHaveBeenCalledWith({
             token: 'tok',
             account,
+            ref: 'me@example',
             view: { json: true, ndjson: false },
             flags: { full: true },
         })
@@ -343,6 +352,7 @@ describe('attachLogoutCommand', () => {
         expect(built.clearSpy).toHaveBeenCalledWith('alice@example')
         expect(onCleared).toHaveBeenCalledWith({
             account,
+            ref: 'alice@example',
             view: { json: false, ndjson: false },
             flags: {},
         })
@@ -364,4 +374,64 @@ describe('attachLogoutCommand', () => {
         expect(built.clearSpy).not.toHaveBeenCalled()
         expect(logSpy).not.toHaveBeenCalled()
     })
+
+    it('proceeds with clear(ref) when active(ref) throws AUTH_STORE_READ_FAILED', async () => {
+        // A matching record exists but the keyring is offline, so the
+        // pre-flight can't return a snapshot. `logout --user me` should
+        // still clear the record (it doesn't need the token); only the
+        // optional `revokeToken` is skipped because there's no token to
+        // send to the server.
+        const built = buildStore()
+        built.activeSpy.mockRejectedValueOnce(
+            new CliError('AUTH_STORE_READ_FAILED', 'keyring offline'),
+        )
+        const revokeSpy = vi.fn()
+        const { program, onCleared } = build({ revokeToken: revokeSpy }, built.store)
+
+        await program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me'])
+
+        expect(built.clearSpy).toHaveBeenCalledWith('me')
+        expect(revokeSpy).not.toHaveBeenCalled()
+        expect(logSpy).toHaveBeenCalledWith('✓ Logged out')
+        // `account` is null (no readable snapshot) but `ref` is populated, so
+        // consumers can distinguish "nothing was stored" from "cleared an
+        // unreadable record".
+        expect(onCleared).toHaveBeenCalledWith({
+            account: null,
+            ref: 'me',
+            view: { json: false, ndjson: false },
+            flags: {},
+        })
+    })
+
+    it('tolerates AUTH_STORE_READ_FAILED when --user alone triggers the pre-flight (no hooks)', async () => {
+        // With neither `revokeToken` nor `onCleared` set, the snapshot only
+        // runs because `--user <ref>` was supplied. The recovery branch must
+        // still kick in there — otherwise `logout --user me` would abort with
+        // `AUTH_STORE_READ_FAILED` in the bare-config case.
+        const built = buildStore()
+        built.activeSpy.mockRejectedValueOnce(
+            new CliError('AUTH_STORE_READ_FAILED', 'keyring offline'),
+        )
+        const { program } = build({ onCleared: undefined }, built.store)
+
+        await program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me'])
+
+        expect(built.clearSpy).toHaveBeenCalledWith('me')
+        expect(logSpy).toHaveBeenCalledWith('✓ Logged out')
+    })
+
+    it('still propagates non-read errors from the snapshot pre-flight', async () => {
+        const thrown = new CliError('AUTH_STORE_WRITE_FAILED', 'something else')
+        const built = buildStore()
+        built.activeSpy.mockRejectedValueOnce(thrown)
+        const { program } = build({ onCleared: undefined }, built.store)
+
+        // Exact-instance match (`toBe`) — a wrap-and-rethrow with the same
+        // code would otherwise pass.
+        await expect(
+            program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']),
+        ).rejects.toBe(thrown)
+        expect(built.clearSpy).not.toHaveBeenCalled()
+    })
 })

src/auth/logout.ts

@@ -1,12 +1,20 @@
 import type { Command } from 'commander'
+import { CliError } from '../errors.js'
 import { formatJson } from '../json.js'
 import type { ViewOptions } from '../options.js'
-import type { AuthAccount, TokenStore } from './types.js'
+import type { AccountRef, AuthAccount, TokenStore } from './types.js'
 import { attachUserFlag, extractUserRef, requireSnapshotForRef } from './user-flag.js'
 
 export type AttachLogoutContext<TAccount extends AuthAccount> = {
-    /** The account that was active immediately before `clear()` ran, or `null` if nothing was stored. */
+    /**
+     * The account that was active immediately before `clear()` ran, or
+     * `null` if nothing was stored. Also `null` on the `AUTH_STORE_READ_FAILED`
+     * recovery path (matching record existed but the token wasn't readable);
+     * `ref` is still populated in that case so consumers can distinguish.
+     */
     account: TAccount | null
+    /** The `--user <ref>` value, or `undefined` when not supplied. Always present so consumers can tell "no stored account" from "cleared an unreadable account by ref". */
+    ref: AccountRef | undefined
     /** `--json` / `--ndjson` flag values, both present (defaulted to `false`). */
     view: Required<ViewOptions>
     /** Consumer-attached options. The registrar flags (`--json`, `--ndjson`, `--user`) are stripped. */
@@ -66,19 +74,31 @@ export function attachLogoutCommand<TAccount extends AuthAccount = AuthAccount>(
             ndjson: Boolean(ndjson),
         }
         const ref = extractUserRef(cmd)
-        // Explicit ref must surface a typed miss before `clear()` runs —
-        // `clear(ref)` is contractually a no-op on miss, so otherwise
-        // `logout --user mistake` would print `✓ Logged out`.
+        // Snapshot only when something downstream needs it:
+        //   - an explicit `--user <ref>` must surface a typed miss before
+        //     `clear()` runs (otherwise `logout --user mistake` would print
+        //     `✓ Logged out` after a no-op clear);
+        //   - either consumer hook is supplied and wants the prior account.
+        // `requireSnapshotForRef` handles both `ref === undefined` (returns
+        // the snapshot directly) and the explicit-ref path (throws
+        // `ACCOUNT_NOT_FOUND` on a null snapshot), so one call covers both.
         const needsSnapshot = ref !== undefined || Boolean(options.revokeToken || options.onCleared)
         let snapshot: { token: string; account: TAccount } | null = null
         if (needsSnapshot) {
-            if (ref !== undefined) {
+            try {
                 snapshot = await requireSnapshotForRef(options.store, ref)
-            } else {
-                try {
-                    snapshot = await options.store.active(ref)
-                } catch {
-                    // Snapshot lookup failures must not block local clear.
+            } catch (error) {
+                // Without an explicit ref, any snapshot read failure must
+                // not block local clear (we just lose the hooks' account
+                // context). With an explicit ref, `AUTH_STORE_READ_FAILED`
+                // is the only recoverable case — clear can still run
+                // without the token. Everything else (notably
+                // `ACCOUNT_NOT_FOUND` from a genuine ref miss) propagates.
+                if (
+                    ref !== undefined &&
+                    !(error instanceof CliError && error.code === 'AUTH_STORE_READ_FAILED')
+                ) {
+                    throw error
                 }
             }
         }
@@ -89,6 +109,7 @@ export function attachLogoutCommand<TAccount extends AuthAccount = AuthAccount>(
                 await options.revokeToken({
                     token: snapshot.token,
                     account: snapshot.account,
+                    ref,
                     view,
                     flags,
                 })
@@ -101,6 +122,6 @@ export function attachLogoutCommand<TAccount extends AuthAccount = AuthAccount>(
         } else if (!view.ndjson) {
             console.log('✓ Logged out')
         }
-        await options.onCleared?.({ account, view, flags })
+        await options.onCleared?.({ account, ref, view, flags })
     })
 }

src/auth/status.test.ts

@@ -246,4 +246,19 @@ describe('attachStatusCommand', () => {
             code: 'ACCOUNT_NOT_FOUND',
         })
     })
+
+    it('surfaces AUTH_STORE_READ_FAILED end-to-end (does not collapse to ACCOUNT_NOT_FOUND)', async () => {
+        const thrown = new CliError('AUTH_STORE_READ_FAILED', 'keyring offline')
+        const built = buildStore()
+        built.activeSpy.mockRejectedValueOnce(thrown)
+        const { program } = build({}, built.store)
+
+        // Assert the exact thrown instance bubbles through unchanged — a
+        // future refactor that recreates the error with the same code
+        // (losing the original `cause` / stack) would still satisfy an
+        // `objectContaining({ code })` check.
+        await expect(
+            program.parseAsync(['node', 'cli', 'auth', 'status', '--user', 'me']),
+        ).rejects.toBe(thrown)
+    })
 })

src/auth/types.ts

@@ -81,7 +81,15 @@ export type AccountRef = string
  * the README example).
  */
 export type TokenStore<TAccount extends AuthAccount = AuthAccount> = {
-    /** Active snapshot, or `null` when nothing matches (the attachers translate a ref miss into `ACCOUNT_NOT_FOUND`). */
+    /**
+     * Active snapshot, or `null` when nothing matches (the attachers translate
+     * a ref miss into `ACCOUNT_NOT_FOUND`). A store MAY throw
+     * `CliError('AUTH_STORE_READ_FAILED', …)` when a matching record exists
+     * but the token itself can't be read (e.g. an OS keyring backing the
+     * store is offline) — `attachLogoutCommand` catches that code on the
+     * explicit-ref path and proceeds with `clear(ref)`; `attachStatusCommand`
+     * and `attachTokenViewCommand` propagate it.
+     */
     active(ref?: AccountRef): Promise<{ token: string; account: TAccount } | null>
     /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */
     set(account: TAccount, token: string): Promise<void>