fix(auth): only treat 401/403 as an invalid token on login (CLI-19)

src/commands/auth/login.ts

@@ -20,6 +20,7 @@ import { getDbPath } from "../../lib/db/index.js";
 import { getUserInfo, setUserInfo } from "../../lib/db/user.js";
 import { getEnv } from "../../lib/env.js";
 import {
+  ApiError,
   AuthError,
   HostScopeError,
   ValidationError,
@@ -356,6 +357,32 @@ async function handleExistingAuth(force: boolean): Promise<boolean> {
   return true;
 }
 
+/**
+ * Handle a failure while validating a user-supplied `--token` via
+ * {@link getUserRegions}. A 401/403 means the token itself is rejected (invalid
+ * or insufficiently scoped) — clear it and raise a user-facing {@link AuthError}.
+ * Any other failure (network, 5xx, parse) is NOT a token problem, so the token
+ * is kept and the original error is re-thrown: surfacing "Invalid API token"
+ * would mislead the user and discard the real cause, and clearing a
+ * possibly-valid token on a transient blip is hostile (CLI-19).
+ *
+ * @throws {AuthError} for a 401/403 (after clearing the stored token)
+ * @throws the original error for any other failure
+ */
+async function handleTokenValidationError(error: unknown): Promise<never> {
+  if (
+    error instanceof ApiError &&
+    (error.status === 401 || error.status === 403)
+  ) {
+    await clearAuth();
+    throw new AuthError(
+      "invalid",
+      "Invalid API token. Please check your token and try again."
+    );
+  }
+  throw error;
+}
+
 export const loginCommand = buildCommand({
   auth: false,
   skipRcUrlCheck: true,
@@ -461,12 +488,8 @@ export const loginCommand = buildCommand({
 
       try {
         await getUserRegions();
-      } catch {
-        await clearAuth();
-        throw new AuthError(
-          "invalid",
-          "Invalid API token. Please check your token and try again."
-        );
+      } catch (error) {
+        await handleTokenValidationError(error);
       }
 
       persistLoginUrlAsDefault(flags.url, effectiveHost);

src/lib/error-reporting.ts

@@ -4,10 +4,10 @@
  * Provides two things:
  *
  * 1. **Silencing rules** — `OutputError`, network failures (offline/DNS/proxy),
- *    `ContextError` (a required value the user omitted), expected-state
- *    `AuthError`, 401–499 `ApiError`, and 400 `ApiError`s that report an
- *    unparseable user search query are not sent to Sentry as issues. A
- *    `cli.error.silenced` metric preserves volume + user/org context.
+ *    `ContextError` (a required value the user omitted), `AuthError` (expected
+ *    auth states the user must act on), 401–499 `ApiError`, and 400 `ApiError`s
+ *    that report an unparseable user search query are not sent to Sentry as
+ *    issues. A `cli.error.silenced` metric preserves volume + user/org context.
  *
  * 2. **Grouping tags** — enriches every error event with `cli_error.*` tags
  *    that Sentry's server-side fingerprint rules use for stable grouping.
@@ -82,10 +82,13 @@ export function classifySilenced(error: unknown): SilenceReason | null {
   if (error instanceof ContextError) {
     return "context_missing";
   }
-  if (
-    error instanceof AuthError &&
-    (error.reason === "not_authenticated" || error.reason === "expired")
-  ) {
+  // All AuthError reasons are expected auth states the user must act on, not
+  // CLI bugs: `not_authenticated` (no token), `expired` (token aged out), and
+  // `invalid` (a bad/insufficiently-scoped token the user supplied). `invalid`
+  // is now only thrown for a genuine 401/403 (see auth/login.ts) — transient
+  // network/server failures no longer masquerade as it — so it is safe to
+  // silence alongside the others (CLI-19).
+  if (error instanceof AuthError) {
     return "auth_expected";
   }
   if (error instanceof ApiError && error.status > 400 && error.status < 500) {

test/commands/auth/login.test.ts

@@ -128,7 +128,11 @@ vi.mock("../../../src/lib/db/user.js", async (importOriginal) => {
 
 // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
 import * as dbUser from "../../../src/lib/db/user.js";
-import { AuthError, ValidationError } from "../../../src/lib/errors.js";
+import {
+  ApiError,
+  AuthError,
+  ValidationError,
+} from "../../../src/lib/errors.js";
 
 vi.mock("../../../src/lib/interactive-login.js", async (importOriginal) => {
   const actual =
@@ -361,10 +365,10 @@ describe("loginCommand.func --token path", () => {
     expect(out).toContain("x@y.com");
   });
 
-  test("--token: invalid token clears auth and throws AuthError", async () => {
+  test("--token: 401 from validation clears auth and throws AuthError", async () => {
     isAuthenticatedSpy.mockReturnValue(false);
     setAuthTokenSpy.mockReturnValue(undefined);
-    getUserRegionsSpy.mockRejectedValue(new Error("401 Unauthorized"));
+    getUserRegionsSpy.mockRejectedValue(new ApiError("Unauthorized", 401));
     clearAuthSpy.mockResolvedValue(undefined);
 
     const { context } = createContext();
@@ -376,6 +380,24 @@ describe("loginCommand.func --token path", () => {
     expect(getCurrentUserSpy).not.toHaveBeenCalled();
   });
 
+  test("--token: non-401 validation failure keeps token and re-throws cause", async () => {
+    // A network/server failure during validation is not a token problem:
+    // surface the real error and do NOT clear a possibly-valid token (CLI-19).
+    isAuthenticatedSpy.mockReturnValue(false);
+    setAuthTokenSpy.mockReturnValue(undefined);
+    const cause = new ApiError("Server error", 503);
+    getUserRegionsSpy.mockRejectedValue(cause);
+    clearAuthSpy.mockResolvedValue(undefined);
+
+    const { context } = createContext();
+    await expect(
+      func.call(context, { token: "maybe-good", force: false, timeout: 900 })
+    ).rejects.toBe(cause);
+
+    expect(clearAuthSpy).not.toHaveBeenCalled();
+    expect(getCurrentUserSpy).not.toHaveBeenCalled();
+  });
+
   test("--token: shows 'Logged in as' when user info fetch succeeds", async () => {
     isAuthenticatedSpy.mockReturnValue(false);
     setAuthTokenSpy.mockReturnValue(undefined);

test/lib/error-reporting.test.ts

@@ -225,8 +225,10 @@ describe("classifySilenced", () => {
     expect(classifySilenced(new AuthError("expired"))).toBe("auth_expected");
   });
 
-  test("does NOT silence AuthError(invalid)", () => {
-    expect(classifySilenced(new AuthError("invalid"))).toBeNull();
+  test("silences AuthError(invalid)", () => {
+    // `invalid` is only thrown for a genuine 401/403 (a bad/insufficient token
+    // the user supplied), so it is an expected auth state like the others.
+    expect(classifySilenced(new AuthError("invalid"))).toBe("auth_expected");
   });
 
   test.each([
@@ -534,9 +536,19 @@ describe("reportCliError integration", () => {
     expect(metricSpy).not.toHaveBeenCalled();
   });
 
-  test("captures AuthError(invalid)", () => {
+  test("silences AuthError(invalid) and emits metric", () => {
     reportCliError(new AuthError("invalid"));
-    expect(captureSpy).toHaveBeenCalled();
+    expect(captureSpy).not.toHaveBeenCalled();
+    expect(metricSpy).toHaveBeenCalledWith(
+      "cli.error.silenced",
+      1,
+      expect.objectContaining({
+        attributes: expect.objectContaining({
+          reason: "auth_expected",
+          auth_reason: "invalid",
+        }),
+      })
+    );
   });
 
   test("captures ApiError(400) with normalized endpoint tag", () => {