fix(telemetry): skip Sentry reporting for 4xx API errors

src/lib/telemetry.ts

@@ -13,7 +13,7 @@
 import * as Sentry from "@sentry/bun";
 import { CLI_VERSION, SENTRY_CLI_DSN } from "./constants.js";
 import { tryRepairAndRetry } from "./db/schema.js";
-import { AuthError } from "./errors.js";
+import { ApiError, AuthError } from "./errors.js";
 import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js";
 
 export type { Span } from "@sentry/bun";
@@ -102,17 +102,25 @@ export async function withTelemetry<T>(
       async (span) => {
         try {
           return await callback(span);
+        } catch (e) {
+          // Record 4xx API errors as span attributes instead of exceptions.
+          // These are user errors (wrong ID, no access) not CLI bugs, but
+          // recording on the span lets us detect volume spikes in Discover.
+          if (isClientApiError(e)) {
+            recordApiErrorOnSpan(span, e as ApiError);
+          }
+          throw e;
         } finally {
           span.end();
         }
       }
     );
   } catch (e) {
-    // Don't capture or mark as crashed for expected auth state
-    // AuthError("not_authenticated") is re-thrown from app.ts for auto-login flow
     const isExpectedAuthState =
       e instanceof AuthError && e.reason === "not_authenticated";
-    if (!isExpectedAuthState) {
+    // 4xx API errors are user errors (wrong ID, no access), not CLI bugs.
+    // They're recorded as span attributes above for volume-spike detection.
+    if (!(isExpectedAuthState || isClientApiError(e))) {
       Sentry.captureException(e);
       markSessionCrashed();
     }
@@ -190,6 +198,35 @@ export function isEpipeError(event: Sentry.ErrorEvent): boolean {
   return false;
 }
 
+/**
+ * Check if an error is a client-side (4xx) API error.
+ *
+ * 4xx errors are user errors — wrong issue IDs, no access, invalid input —
+ * not CLI bugs. These should be recorded as span attributes for volume-spike
+ * detection in Discover, but should NOT be captured as Sentry exceptions.
+ *
+ * @internal Exported for testing
+ */
+export function isClientApiError(error: unknown): boolean {
+  return error instanceof ApiError && error.status >= 400 && error.status < 500;
+}
+
+/**
+ * Record a client API error as span attributes for Discover queryability.
+ *
+ * Sets `api_error.status`, `api_error.message`, and optionally `api_error.detail`
+ * on the span. Must be called before `span.end()`.
+ *
+ * @internal Exported for testing
+ */
+export function recordApiErrorOnSpan(span: Span, error: ApiError): void {
+  span.setAttribute("api_error.status", error.status);
+  span.setAttribute("api_error.message", error.message);
+  if (error.detail) {
+    span.setAttribute("api_error.detail", error.detail);
+  }
+}
+
 /**
  * Integrations to exclude for CLI.
  * These add overhead without benefit for short-lived CLI processes.

test/lib/telemetry.test.ts

@@ -8,9 +8,12 @@ import { Database } from "bun:sqlite";
 import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
 // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
 import * as Sentry from "@sentry/bun";
+import { ApiError, AuthError } from "../../src/lib/errors.js";
 import {
   createTracedDatabase,
   initSentry,
+  isClientApiError,
+  recordApiErrorOnSpan,
   setArgsContext,
   setCommandSpanName,
   setFlagContext,
@@ -126,6 +129,158 @@ describe("withTelemetry", () => {
     });
     expect(executed).toBe(true);
   });
+
+  test("propagates 4xx ApiError to caller", async () => {
+    const error = new ApiError("Not found", 404, "Issue not found");
+    await expect(
+      withTelemetry(() => {
+        throw error;
+      })
+    ).rejects.toThrow(error);
+  });
+
+  describe("with telemetry enabled", () => {
+    beforeEach(() => {
+      delete process.env[ENV_VAR];
+    });
+
+    afterEach(() => {
+      // Re-init with enabled=false to reset global SDK state.
+      // Without this, Sentry.isEnabled() returns true for all
+      // subsequent test files (e.g. feedbackCommand checks it).
+      initSentry(false);
+    });
+
+    test("propagates 4xx ApiError through enabled SDK path", async () => {
+      const error = new ApiError("Not found", 404, "Issue not found");
+      await expect(
+        withTelemetry(() => {
+          throw error;
+        })
+      ).rejects.toThrow(error);
+    });
+
+    test("propagates 5xx ApiError through enabled SDK path", async () => {
+      const error = new ApiError("Server error", 500, "Internal error");
+      await expect(
+        withTelemetry(() => {
+          throw error;
+        })
+      ).rejects.toThrow(error);
+    });
+
+    test("propagates generic Error through enabled SDK path", async () => {
+      await expect(
+        withTelemetry(() => {
+          throw new Error("unexpected bug");
+        })
+      ).rejects.toThrow("unexpected bug");
+    });
+
+    test("returns result through enabled SDK path", async () => {
+      const result = await withTelemetry(() => 42);
+      expect(result).toBe(42);
+    });
+  });
+});
+
+describe("isClientApiError", () => {
+  test("returns true for 400 Bad Request", () => {
+    expect(isClientApiError(new ApiError("Bad request", 400))).toBe(true);
+  });
+
+  test("returns true for 403 Forbidden", () => {
+    expect(isClientApiError(new ApiError("Forbidden", 403, "No access"))).toBe(
+      true
+    );
+  });
+
+  test("returns true for 404 Not Found", () => {
+    expect(
+      isClientApiError(new ApiError("Not found", 404, "Issue not found"))
+    ).toBe(true);
+  });
+
+  test("returns true for 429 Too Many Requests", () => {
+    expect(isClientApiError(new ApiError("Rate limited", 429))).toBe(true);
+  });
+
+  test("returns false for 500 Internal Server Error", () => {
+    expect(isClientApiError(new ApiError("Server error", 500))).toBe(false);
+  });
+
+  test("returns false for 502 Bad Gateway", () => {
+    expect(isClientApiError(new ApiError("Bad gateway", 502))).toBe(false);
+  });
+
+  test("returns false for non-ApiError", () => {
+    expect(isClientApiError(new Error("generic error"))).toBe(false);
+  });
+
+  test("returns false for AuthError", () => {
+    expect(isClientApiError(new AuthError("not_authenticated"))).toBe(false);
+  });
+
+  test("returns false for null/undefined", () => {
+    expect(isClientApiError(null)).toBe(false);
+    expect(isClientApiError(undefined)).toBe(false);
+  });
+
+  test("returns false for non-Error objects", () => {
+    expect(isClientApiError({ status: 404 })).toBe(false);
+    expect(isClientApiError("404")).toBe(false);
+  });
+});
+
+describe("recordApiErrorOnSpan", () => {
+  function createMockSpan() {
+    const attributes: Record<string, string | number> = {};
+    return {
+      attributes,
+      setAttribute(key: string, value: string | number) {
+        attributes[key] = value;
+      },
+    };
+  }
+
+  test("sets status and message attributes", () => {
+    const span = createMockSpan();
+    const error = new ApiError("Not found", 404);
+    recordApiErrorOnSpan(span as never, error);
+
+    expect(span.attributes["api_error.status"]).toBe(404);
+    expect(span.attributes["api_error.message"]).toBe("Not found");
+    expect(span.attributes["api_error.detail"]).toBeUndefined();
+  });
+
+  test("sets detail attribute when present", () => {
+    const span = createMockSpan();
+    const error = new ApiError("Not found", 404, "Issue not found");
+    recordApiErrorOnSpan(span as never, error);
+
+    expect(span.attributes["api_error.status"]).toBe(404);
+    expect(span.attributes["api_error.message"]).toBe("Not found");
+    expect(span.attributes["api_error.detail"]).toBe("Issue not found");
+  });
+
+  test("omits detail attribute when empty string", () => {
+    const span = createMockSpan();
+    const error = new ApiError("Bad request", 400, "");
+    recordApiErrorOnSpan(span as never, error);
+
+    expect(span.attributes["api_error.status"]).toBe(400);
+    expect(span.attributes["api_error.detail"]).toBeUndefined();
+  });
+
+  test("handles different 4xx status codes", () => {
+    const span = createMockSpan();
+    const error = new ApiError("Forbidden", 403, "No access");
+    recordApiErrorOnSpan(span as never, error);
+
+    expect(span.attributes["api_error.status"]).toBe(403);
+    expect(span.attributes["api_error.message"]).toBe("Forbidden");
+    expect(span.attributes["api_error.detail"]).toBe("No access");
+  });
 });
 
 describe("setCommandSpanName", () => {