fix: harden auth token refresh

src/cli.ts

@@ -1,6 +1,7 @@
 #!/usr/bin/env node
 import { Command } from "commander";
 import { version } from "../package.json";
+import { handleCliError } from "./cli/errors.js";
 import {
   enforceCachedRequiredUpdateForInvocation,
   runWithUpdateCheckFlush,
@@ -21,7 +22,6 @@ import {
   registerPkgCommandGroup,
   registerUnifiedSearchCommands,
 } from "./commands/index.js";
-import { AuthConfigError, AuthStoragePolicyError } from "./services/index.js";
 import {
   FileSystemServiceImpl,
   NpmRegistryUpdateCheckService,
@@ -158,17 +158,10 @@ try {
     { stderr: process.stderr, requiredUpdateRefreshTask },
   );
 } catch (error) {
-  if (isUserFacingError(error)) {
-    console.error(`${error.message}\n`);
-    process.exit(1);
-  }
-  throw error;
-}
-
-function isUserFacingError(error: unknown): error is Error {
-  return (
-    error instanceof AuthConfigError || error instanceof AuthStoragePolicyError
-  );
+  handleCliError(error, {
+    stderr: process.stderr,
+    exit: process.exit as (code: number) => never,
+  });
 }
 
 /**

src/cli/errors.test.ts

@@ -0,0 +1,52 @@
+import { describe, expect, it } from "bun:test";
+import { AuthStorageLockTimeoutError } from "../services/index.js";
+import { AuthRequiredError } from "../shared/require-auth.js";
+import { handleCliError } from "./errors.js";
+
+describe("handleCliError", () => {
+  it("exits without writing a stack trace for AuthRequiredError", () => {
+    const stderrWrites: string[] = [];
+    const exit = ((code: number) => {
+      throw new Error(`process.exit:${code}`);
+    }) as (code: number) => never;
+
+    expect(() =>
+      handleCliError(new AuthRequiredError("Authentication required"), {
+        stderr: {
+          write: (chunk: string | Uint8Array) => {
+            stderrWrites.push(String(chunk));
+            return true;
+          },
+        },
+        exit,
+      }),
+    ).toThrow("process.exit:1");
+
+    expect(stderrWrites.join("")).not.toContain("AuthRequiredError");
+    expect(stderrWrites.join("")).not.toContain("at ");
+  });
+
+  it("prints lock timeout errors without an uncaught stack trace", () => {
+    const stderrWrites: string[] = [];
+    const exit = ((code: number) => {
+      throw new Error(`process.exit:${code}`);
+    }) as (code: number) => never;
+
+    expect(() =>
+      handleCliError(new AuthStorageLockTimeoutError("lock timed out"), {
+        stderr: {
+          write: (chunk: string | Uint8Array) => {
+            stderrWrites.push(String(chunk));
+            return true;
+          },
+        },
+        exit,
+      }),
+    ).toThrow("process.exit:1");
+
+    const output = stderrWrites.join("");
+    expect(output).toContain("lock timed out");
+    expect(output).not.toContain("AuthStorageLockTimeoutError");
+    expect(output).not.toContain("at ");
+  });
+});

src/cli/errors.ts

@@ -0,0 +1,35 @@
+import {
+  AuthConfigError,
+  AuthStorageLockTimeoutError,
+  AuthStoragePolicyError,
+} from "../services/index.js";
+import { AuthRequiredError } from "../shared/require-auth.js";
+
+export interface CliErrorHandlerDeps {
+  stderr: Pick<NodeJS.WriteStream, "write">;
+  exit: (code: number) => never;
+}
+
+export function handleCliError(
+  error: unknown,
+  deps: CliErrorHandlerDeps,
+): never {
+  if (error instanceof AuthRequiredError) {
+    deps.exit(1);
+  }
+
+  if (isUserFacingError(error)) {
+    deps.stderr.write(`${error.message}\n\n`);
+    deps.exit(1);
+  }
+
+  throw error;
+}
+
+function isUserFacingError(error: unknown): error is Error {
+  return (
+    error instanceof AuthConfigError ||
+    error instanceof AuthStorageLockTimeoutError ||
+    error instanceof AuthStoragePolicyError
+  );
+}

src/commands/login.test.ts

@@ -29,10 +29,10 @@ describe("loginAction", () => {
     );
     expect(browserService.open).toHaveBeenCalled();
     expect(authService.exchangeCodeForTokens).toHaveBeenCalled();
-    expect(authStorage.saveTokens).toHaveBeenCalled();
-    expect(authStorage.saveClient).toHaveBeenCalledWith(
+    expect(authStorage.saveAuthSession).toHaveBeenCalledWith(
       expect.stringContaining("__githits_storage_probe__"),
       expect.any(Object),
+      expect.any(Object),
     );
 
     consoleSpy.mockRestore();
@@ -60,7 +60,11 @@ describe("loginAction", () => {
       },
     );
 
-    expect(authStorage.saveTokens).not.toHaveBeenCalled();
+    expect(authStorage.saveAuthSession).not.toHaveBeenCalledWith(
+      mcpUrl,
+      expect.any(Object),
+      expect.any(Object),
+    );
     consoleSpy.mockRestore();
   });
 
@@ -86,7 +90,11 @@ describe("loginAction", () => {
       },
     );
 
-    expect(authStorage.saveTokens).toHaveBeenCalled();
+    expect(authStorage.saveAuthSession).toHaveBeenCalledWith(
+      mcpUrl,
+      expect.any(Object),
+      expect.any(Object),
+    );
     consoleSpy.mockRestore();
   });
 
@@ -152,17 +160,18 @@ describe("loginAction", () => {
     );
 
     expect(authService.registerClient).toHaveBeenCalled();
-    expect(authStorage.saveClient).toHaveBeenCalledWith(
+    expect(authStorage.saveAuthSession).toHaveBeenCalledWith(
       mcpUrl,
       expect.any(Object),
+      expect.any(Object),
     );
     consoleSpy.mockRestore();
   });
 
   it("fails before remote registration when storage preflight fails", async () => {
     const consoleSpy = spyOn(console, "log").mockImplementation(() => {});
     const authStorage = createMockAuthStorage({
-      saveClient: mock(() => Promise.reject(new Error("keychain locked"))),
+      saveAuthSession: mock(() => Promise.reject(new Error("keychain locked"))),
     });
     const authService = createMockAuthService();
     const browserService = createMockBrowserService();
@@ -197,7 +206,11 @@ describe("loginAction", () => {
 
     expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl);
     expect(authService.registerClient).toHaveBeenCalled();
-    expect(authStorage.saveClient).toHaveBeenCalled();
+    expect(authStorage.saveAuthSession).toHaveBeenCalledWith(
+      mcpUrl,
+      expect.any(Object),
+      expect.any(Object),
+    );
     consoleSpy.mockRestore();
   });
 
@@ -307,7 +320,11 @@ describe("loginAction", () => {
     );
 
     expect(authStorage.clearClient).not.toHaveBeenCalledWith(mcpUrl);
-    expect(authStorage.saveTokens).not.toHaveBeenCalled();
+    expect(authStorage.saveAuthSession).not.toHaveBeenCalledWith(
+      mcpUrl,
+      expect.any(Object),
+      expect.any(Object),
+    );
     consoleSpy.mockRestore();
   });
 
@@ -334,7 +351,11 @@ describe("loginAction", () => {
     );
 
     expect(authStorage.clearClient).not.toHaveBeenCalledWith(mcpUrl);
-    expect(authStorage.saveTokens).toHaveBeenCalled();
+    expect(authStorage.saveAuthSession).toHaveBeenCalledWith(
+      mcpUrl,
+      expect.any(Object),
+      expect.any(Object),
+    );
     consoleSpy.mockRestore();
   });
 
@@ -360,7 +381,11 @@ describe("loginAction", () => {
       },
     );
 
-    expect(authStorage.saveTokens).toHaveBeenCalled();
+    expect(authStorage.saveAuthSession).toHaveBeenCalledWith(
+      mcpUrl,
+      expect.any(Object),
+      expect.any(Object),
+    );
     consoleSpy.mockRestore();
   });
 });

src/commands/login.ts

@@ -49,14 +49,11 @@ async function preflightAuthPersistence(
     createdAt: new Date(0).toISOString(),
   };
   try {
-    await authStorage.saveClient(probeUrl, probeClient);
-    await authStorage.saveTokens(probeUrl, probeTokens);
-    await authStorage.clearTokens(probeUrl);
-    await authStorage.clearClient(probeUrl);
+    await authStorage.saveAuthSession(probeUrl, probeClient, probeTokens);
+    await authStorage.clearAuthSession(probeUrl);
     return null;
   } catch (error) {
-    await authStorage.clearTokens(probeUrl).catch(() => {});
-    await authStorage.clearClient(probeUrl).catch(() => {});
+    await authStorage.clearAuthSession(probeUrl).catch(() => {});
     const message = error instanceof Error ? error.message : String(error);
     return {
       status: "failed",
@@ -135,7 +132,6 @@ export async function loginFlow(
           redirectUri,
           registeredAt: new Date().toISOString(),
         };
-        await authStorage.saveClient(mcpUrl, client);
       }
       port = options.port;
     } else {
@@ -158,7 +154,6 @@ export async function loginFlow(
       redirectUri,
       registeredAt: new Date().toISOString(),
     };
-    await authStorage.saveClient(mcpUrl, client);
   }
 
   // Step 3: Generate PKCE parameters
@@ -244,11 +239,11 @@ export async function loginFlow(
     };
   }
 
-  // Step 9: Save tokens
+  // Step 9: Save auth session
   const expiresAt = new Date(
     Date.now() + tokenResponse.expiresIn * 1000,
   ).toISOString();
-  await authStorage.saveTokens(mcpUrl, {
+  await authStorage.saveAuthSession(mcpUrl, client, {
     accessToken: tokenResponse.accessToken,
     refreshToken: tokenResponse.refreshToken,
     expiresAt,

src/commands/logout.test.ts

@@ -17,8 +17,7 @@ describe("logoutAction", () => {
 
     await logoutAction({ authStorage, mcpUrl });
 
-    expect(authStorage.clearTokens).toHaveBeenCalledWith(mcpUrl);
-    expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl);
+    expect(authStorage.clearAuthSession).toHaveBeenCalledWith(mcpUrl);
     consoleSpy.mockRestore();
   });
 
@@ -28,43 +27,26 @@ describe("logoutAction", () => {
 
     await logoutAction({ authStorage, mcpUrl });
 
-    // Idempotent cleanup removes orphaned client registrations
-    expect(authStorage.clearTokens).toHaveBeenCalledWith(mcpUrl);
-    expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl);
+    // Idempotent cleanup removes orphaned client registrations.
+    expect(authStorage.clearAuthSession).toHaveBeenCalledWith(mcpUrl);
     const output = consoleSpy.mock.calls.map((c) => c[0]).join("\n");
     expect(output).toContain("Not currently logged in");
     consoleSpy.mockRestore();
   });
 
-  it("clears client even when clearTokens throws", async () => {
+  it("propagates session clear failures", async () => {
     const consoleSpy = spyOn(console, "log").mockImplementation(() => {});
     const authStorage = createMockAuthStorage({
       loadTokens: mock(() => Promise.resolve(createValidTokenData())),
-      clearTokens: mock(() =>
+      clearAuthSession: mock(() =>
         Promise.reject(new KeychainUnavailableError("keychain locked")),
       ),
     });
 
     await expect(logoutAction({ authStorage, mcpUrl })).rejects.toThrow(
       KeychainUnavailableError,
     );
-    expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl);
-    consoleSpy.mockRestore();
-  });
-
-  it("clears tokens even when clearClient throws", async () => {
-    const consoleSpy = spyOn(console, "log").mockImplementation(() => {});
-    const authStorage = createMockAuthStorage({
-      loadTokens: mock(() => Promise.resolve(createValidTokenData())),
-      clearClient: mock(() =>
-        Promise.reject(new KeychainUnavailableError("keychain locked")),
-      ),
-    });
-
-    await expect(logoutAction({ authStorage, mcpUrl })).rejects.toThrow(
-      KeychainUnavailableError,
-    );
-    expect(authStorage.clearTokens).toHaveBeenCalledWith(mcpUrl);
+    expect(authStorage.clearAuthSession).toHaveBeenCalledWith(mcpUrl);
     consoleSpy.mockRestore();
   });
 });

src/commands/logout.ts

@@ -10,29 +10,14 @@ export interface LogoutDependencies {
 /**
  * Core logout logic, separated for testability.
  *
- * Always clears both tokens and client registration independently before
- * reporting status. This ensures orphaned client registrations are cleaned up
- * even when tokens are already absent (e.g. after a partial logout or expired
- * token clear). Both clear operations are idempotent and error-isolated so a
- * failure in one does not prevent the other from running.
+ * Clears both tokens and client registration as one auth-session update so
+ * concurrent MCP servers and login/logout commands cannot observe split state.
  */
 export async function logoutAction(deps: LogoutDependencies): Promise<void> {
   const { authStorage, mcpUrl } = deps;
 
   const auth = await authStorage.loadTokens(mcpUrl);
-
-  // Clear both independently — a failure in one must not prevent the other.
-  let firstError: unknown;
-  try {
-    await authStorage.clearTokens(mcpUrl);
-  } catch (error) {
-    firstError = error;
-  }
-  try {
-    await authStorage.clearClient(mcpUrl);
-  } catch (error) {
-    firstError ??= error;
-  }
+  await authStorage.clearAuthSession(mcpUrl);
 
   if (!auth) {
     console.log("Not currently logged in.\n");
@@ -41,8 +26,6 @@ export async function logoutAction(deps: LogoutDependencies): Promise<void> {
     console.log("Logged out.\n");
     console.log(`  Environment: ${mcpUrl}`);
   }
-
-  if (firstError) throw firstError;
 }
 
 const LOGOUT_DESCRIPTION = `Remove stored credentials.