fix: auth credential storage and transient error handling (#81)

* fix: make credential and config stores keychain-primary with file fallback

Both stores now follow the same pattern: write to system keychain first,
only write to file when keychain is unavailable, never proactively delete
existing file backups.

config-store: removed deleteFile() calls from saveConfig() and getConfig()
migration path that caused environment configs to vanish after addon
rebuilds or keychain access failures.

credential-store: stopped always writing to file alongside keychain.
File is now only written as fallback when keychain write fails.

* feat: add cross-process file locking for token refresh

Prevents race condition where two concurrent CLI processes both attempt
to refresh the same token, causing one to get invalid_grant when the
server rotates the refresh token.

New file-lock utility uses atomic file creation (O_EXCL) with stale
lock detection (30s timeout). New refreshAccessTokenSafe() wraps the
existing refresh with lock-acquire-then-recheck pattern: after acquiring
the lock, re-reads credentials in case another process already refreshed.

Falls back to unlocked refresh if lock acquisition fails (degraded but
functional). All callers (ensure-auth, background-refresh, credential-proxy)
now use the safe variant.

* Revert "feat: add cross-process file locking for token refresh"

This reverts commit 98095ab.

* fix: don't clear credentials on transient refresh errors

Only clear credentials on invalid_grant (refresh token permanently dead).
Network and server errors are transient — preserve credentials so the
user can retry without being forced to re-login.

Previously, any refresh failure wiped the keychain, destroying a
potentially valid refresh token over a temporary server hiccup.

* fix: eliminate redundant keyring reads and migration retries

- ensure-auth: collapse hasCredentials() + getCredentials() two-step
  into single getCredentials() call, eliminating double keyring read
  and fixing semantic mismatch (hasCredentials returns true for corrupt
  files, getCredentials returns null)

- credential-store/config-store: add migrationAttempted flag to prevent
  repeated failed keyring writes on every read when keyring is
  unavailable (e.g., headless Linux with no keyring daemon)

* fix: move session refresh message to debug log

The "(Session refreshed)" message is implementation detail noise for
users. Moved to debug log, keeping only "Already logged in as ..."
in user-facing output.

Author: nicknisi

src/commands/login.ts

@@ -3,6 +3,7 @@ import clack from '../utils/clack.js';
 import { saveCredentials, getCredentials, getAccessToken, isTokenExpired, updateTokens } from '../lib/credentials.js';
 import { getCliAuthClientId, getAuthkitDomain } from '../lib/settings.js';
 import { refreshAccessToken } from '../lib/token-refresh-client.js';
+import { logInfo } from '../utils/debug.js';
 
 /**
  * Parse JWT payload
@@ -88,8 +89,8 @@ export async function runLogin(): Promise<void> {
       const result = await refreshAccessToken(authkitDomain, clientId);
       if (result.accessToken && result.expiresAt) {
         updateTokens(result.accessToken, result.expiresAt, result.refreshToken);
+        logInfo('[login] Session refreshed via refresh token');
         clack.log.info(`Already logged in as ${existingCreds.email ?? 'unknown'}`);
-        clack.log.info('(Session refreshed)');
         clack.log.info('Run `workos logout` to log out');
         return;
       }

src/lib/config-store.spec.ts

@@ -1,5 +1,14 @@
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
-import { existsSync, readFileSync, unlinkSync, mkdtempSync, rmdirSync, statSync, writeFileSync } from 'node:fs';
+import {
+  existsSync,
+  readFileSync,
+  unlinkSync,
+  mkdtempSync,
+  rmdirSync,
+  statSync,
+  writeFileSync,
+  mkdirSync,
+} from 'node:fs';
 import { join } from 'node:path';
 import { tmpdir } from 'node:os';
 
@@ -13,6 +22,44 @@ let testDir: string;
 let workosDir: string;
 let configFile: string;
 
+// Mock keyring storage
+const mockKeyring = new Map<string, string>();
+let keyringAvailable = true;
+
+vi.mock('@napi-rs/keyring', () => ({
+  Entry: class MockEntry {
+    private key: string;
+
+    constructor(
+      service: string,
+      private account: string,
+    ) {
+      this.key = `${service}:${account}`;
+    }
+
+    getPassword(): string | null {
+      if (!keyringAvailable) {
+        throw new Error('Keyring not available');
+      }
+      return mockKeyring.get(this.key) ?? null;
+    }
+
+    setPassword(password: string): void {
+      if (!keyringAvailable) {
+        throw new Error('Keyring not available');
+      }
+      mockKeyring.set(this.key, password);
+    }
+
+    deletePassword(): void {
+      if (!keyringAvailable && mockKeyring.has(this.key)) {
+        throw new Error('Keyring not available');
+      }
+      mockKeyring.delete(this.key);
+    }
+  },
+}));
+
 // Mock os.homedir BEFORE importing config-store module
 vi.mock('node:os', async (importOriginal) => {
   const original = await importOriginal<typeof import('node:os')>();
@@ -36,7 +83,11 @@ describe('config-store', () => {
     testDir = mkdtempSync(join(tmpdir(), 'config-store-test-'));
     workosDir = join(testDir, '.workos');
     configFile = join(workosDir, 'config.json');
-    // Force file-based storage for tests
+
+    // Reset state
+    mockKeyring.clear();
+    keyringAvailable = true;
+    // Force file-based storage for most tests
     setInsecureConfigStorage(true);
   });
 
@@ -199,4 +250,97 @@ describe('config-store', () => {
       expect(env?.endpoint).toBe('http://localhost:8001');
     });
   });
+
+  describe('keyring storage (default)', () => {
+    beforeEach(() => {
+      setInsecureConfigStorage(false);
+    });
+
+    it('saves config to keyring', () => {
+      saveConfig(sampleConfig);
+      expect(mockKeyring.has('workos-cli:config')).toBe(true);
+    });
+
+    it('retrieves config from keyring', () => {
+      saveConfig(sampleConfig);
+      const config = getConfig();
+      expect(config?.activeEnvironment).toBe('production');
+      expect(config?.environments.production.apiKey).toBe('sk_test_abc123');
+    });
+
+    it('does not write file when keyring succeeds', () => {
+      saveConfig(sampleConfig);
+      expect(mockKeyring.has('workos-cli:config')).toBe(true);
+      expect(existsSync(configFile)).toBe(false);
+    });
+
+    it('does not delete existing file on keyring success', () => {
+      // Create a pre-existing file (from a prior fallback write)
+      mkdirSync(workosDir, { recursive: true });
+      writeFileSync(configFile, JSON.stringify(sampleConfig));
+
+      saveConfig({ ...sampleConfig, activeEnvironment: 'staging' });
+
+      expect(mockKeyring.has('workos-cli:config')).toBe(true);
+      expect(existsSync(configFile)).toBe(true);
+    });
+
+    it('clears from both keyring and file', () => {
+      saveConfig(sampleConfig);
+      mkdirSync(workosDir, { recursive: true });
+      writeFileSync(configFile, JSON.stringify(sampleConfig));
+
+      clearConfig();
+
+      expect(mockKeyring.has('workos-cli:config')).toBe(false);
+      expect(existsSync(configFile)).toBe(false);
+    });
+  });
+
+  describe('file fallback (keyring unavailable)', () => {
+    beforeEach(() => {
+      setInsecureConfigStorage(false);
+      keyringAvailable = false;
+    });
+
+    it('falls back to file when keyring unavailable', () => {
+      saveConfig(sampleConfig);
+      expect(existsSync(configFile)).toBe(true);
+      expect(mockKeyring.has('workos-cli:config')).toBe(false);
+    });
+
+    it('reads from file when keyring unavailable', () => {
+      saveConfig(sampleConfig);
+      const config = getConfig();
+      expect(config?.activeEnvironment).toBe('production');
+    });
+  });
+
+  describe('migration (file to keyring)', () => {
+    beforeEach(() => {
+      setInsecureConfigStorage(false);
+    });
+
+    it('migrates file config to keyring on read but keeps file', () => {
+      mkdirSync(workosDir, { recursive: true });
+      writeFileSync(configFile, JSON.stringify(sampleConfig));
+
+      const config = getConfig();
+
+      expect(config?.activeEnvironment).toBe('production');
+      expect(mockKeyring.has('workos-cli:config')).toBe(true);
+      expect(existsSync(configFile)).toBe(true);
+    });
+
+    it('keeps file if keyring unavailable during migration', () => {
+      mkdirSync(workosDir, { recursive: true });
+      writeFileSync(configFile, JSON.stringify(sampleConfig));
+
+      keyringAvailable = false;
+      const config = getConfig();
+
+      expect(config?.activeEnvironment).toBe('production');
+      expect(existsSync(configFile)).toBe(true);
+    });
+  });
 });

src/lib/config-store.ts

@@ -33,9 +33,11 @@ const ACCOUNT_NAME = 'config';
 
 let fallbackWarningShown = false;
 let forceInsecureStorage = false;
+let migrationAttempted = false;
 
 export function setInsecureConfigStorage(value: boolean): void {
   forceInsecureStorage = value;
+  migrationAttempted = false;
 }
 
 function getConfigDir(): string {
@@ -134,8 +136,10 @@ export function getConfig(): CliConfig | null {
 
   const fileConfig = readFromFile();
   if (fileConfig) {
-    // Migrate file config to keyring if possible
-    if (writeToKeyring(fileConfig)) deleteFile();
+    if (!migrationAttempted) {
+      migrationAttempted = true;
+      writeToKeyring(fileConfig);
+    }
     return fileConfig;
   }
 
@@ -145,9 +149,7 @@ export function getConfig(): CliConfig | null {
 export function saveConfig(config: CliConfig): void {
   if (forceInsecureStorage) return writeToFile(config);
 
-  if (writeToKeyring(config)) {
-    deleteFile();
-  } else {
+  if (!writeToKeyring(config)) {
     showFallbackWarning();
     writeToFile(config);
   }
@@ -156,6 +158,7 @@ export function saveConfig(config: CliConfig): void {
 export function clearConfig(): void {
   deleteFromKeyring();
   deleteFile();
+  migrationAttempted = false;
 }
 
 export function getActiveEnvironment(): EnvironmentConfig | null {

src/lib/credential-store.spec.ts

@@ -124,10 +124,22 @@ describe('credential-store', () => {
       expect(creds?.userId).toBe(validCreds.userId);
     });
 
-    it('keeps file as durable fallback alongside keyring', () => {
+    it('does not write file when keyring succeeds', () => {
       saveCredentials(validCreds);
 
-      // Both keyring and file should have credentials
+      // Keyring has credentials, file should NOT be written
+      expect(mockKeyring.has('workos-cli:credentials')).toBe(true);
+      expect(existsSync(credentialsFile)).toBe(false);
+    });
+
+    it('does not delete existing file on keyring success', () => {
+      // Create a pre-existing file (from a prior fallback write)
+      mkdirSync(installerDir, { recursive: true });
+      writeFileSync(credentialsFile, JSON.stringify(validCreds));
+
+      // Save with keyring available — file should remain untouched
+      saveCredentials({ ...validCreds, accessToken: 'new_token' });
+
       expect(mockKeyring.has('workos-cli:credentials')).toBe(true);
       expect(existsSync(credentialsFile)).toBe(true);
     });
@@ -236,11 +248,10 @@ describe('credential-store', () => {
     });
 
     it('hasCredentials only checks file when flag is set', () => {
-      // Save with default mode (writes to both keyring + file)
+      // Save with insecure mode (writes to file)
+      setInsecureStorage(true);
       saveCredentials(validCreds);
 
-      // Enable insecure storage — should still find the file
-      setInsecureStorage(true);
       expect(hasCredentials()).toBe(true);
     });
   });

src/lib/credential-store.ts

@@ -32,9 +32,11 @@ const ACCOUNT_NAME = 'credentials';
 
 let fallbackWarningShown = false;
 let forceInsecureStorage = false;
+let migrationAttempted = false;
 
 export function setInsecureStorage(value: boolean): void {
   forceInsecureStorage = value;
+  migrationAttempted = false;
 }
 
 function getCredentialsDir(): string {
@@ -145,7 +147,10 @@ export function getCredentials(): Credentials | null {
 
   const fileCreds = readFromFile();
   if (fileCreds) {
-    writeToKeyring(fileCreds);
+    if (!migrationAttempted) {
+      migrationAttempted = true;
+      writeToKeyring(fileCreds);
+    }
     return fileCreds;
   }
 
@@ -155,15 +160,16 @@ export function getCredentials(): Credentials | null {
 export function saveCredentials(creds: Credentials): void {
   if (forceInsecureStorage) return writeToFile(creds);
 
-  writeToFile(creds);
   if (!writeToKeyring(creds)) {
     showFallbackWarning();
+    writeToFile(creds);
   }
 }
 
 export function clearCredentials(): void {
   deleteFromKeyring();
   deleteFile();
+  migrationAttempted = false;
 }
 
 export function updateTokens(accessToken: string, expiresAt: number, refreshToken?: string): void {

src/lib/ensure-auth.spec.ts

@@ -304,7 +304,7 @@ describe('ensure-auth', () => {
         expect(mockRunLogin).toHaveBeenCalledOnce();
       });
 
-      it('clears credentials when refresh fails with network error', async () => {
+      it('preserves credentials when refresh fails with network error', async () => {
         saveCredentials(expiredAccessCreds);
 
         mockRefreshAccessToken.mockResolvedValue({
@@ -313,13 +313,12 @@ describe('ensure-auth', () => {
           error: 'Network error',
         });
 
-        // Don't save new creds in login — verify old ones were cleared
         mockRunLogin.mockImplementation(() => {});
 
         await ensureAuthenticated();
 
-        // Old stale credentials should be gone
-        expect(hasCredentials()).toBe(false);
+        // Credentials should be preserved — transient errors shouldn't nuke the session
+        expect(hasCredentials()).toBe(true);
       });
 
       it('clears credentials when no refresh token available', async () => {