diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index aa88860ce..fc6fdafda 100644 --- a/src/lib/error-reporting.ts +++ b/src/lib/error-reporting.ts @@ -3,11 +3,11 @@ * * Provides two things: * - * 1. **Silencing rules** — `OutputError`, `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. + * 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. * * 2. **Grouping tags** — enriches every error event with `cli_error.*` tags * that Sentry's server-side fingerprint rules use for stable grouping. @@ -27,6 +27,7 @@ import { ContextError, DeviceFlowError, HostScopeError, + isNetworkError, isSearchQueryParseError, OutputError, ResolutionError, @@ -50,7 +51,8 @@ type SilenceReason = | "context_missing" | "auth_expected" | "api_user_error" - | "api_query_error"; + | "api_query_error" + | "network_error"; /** * Classify whether an error should be silenced. @@ -62,6 +64,14 @@ export function classifySilenced(error: unknown): SilenceReason | null { if (error instanceof OutputError) { return "output_error"; } + // A raw `TypeError: "fetch failed"` (CLI-16W) means the CLI could not reach + // Sentry at all (offline, DNS, connection refused/timeout). There is nothing + // actionable in a "user is offline" report, so drop it — same rationale as + // EPIPE/EBADF OS noise in `beforeSend`. Note: TLS cert errors are wrapped as + // ApiError(status 0), NOT matched here, so they stay captured/actionable. + if (isNetworkError(error)) { + return "network_error"; + } // A ContextError always means the user omitted a required value (no // org/project could be resolved, a required ID was not provided, etc.). It is // never a CLI bug — unlike ResolutionError, where a *provided* value that diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 386a9ba56..001ab80ea 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -780,6 +780,27 @@ export function isSearchQueryParseError(error: ApiError): boolean { ); } +/** + * Whether an error is a raw network-level fetch failure — the CLI could not + * reach the Sentry API at all (offline, DNS failure, connection + * refused/timeout). Node's `fetch` (undici) rejects with exactly + * `TypeError: "fetch failed"` for every such failure (the real errno is in + * `cause`). + * + * This reflects the user's environment, not a CLI bug, so it is treated as a + * user error (no upgrade nudge) and is not reported to Sentry — the same + * rationale as dropping EPIPE/EBADF OS noise in `beforeSend`. + * + * NOTE: This deliberately does NOT match `ApiError` with `status === 0`. That + * status is shared by genuine connectivity failures AND TLS certificate errors, + * and the latter are actionable user-configuration issues (e.g. a missing CA + * cert) that must stay visible. Callers that want the "user environment, not a + * CLI bug" treatment for `status === 0` check it explicitly. + */ +export function isNetworkError(error: unknown): boolean { + return error instanceof TypeError && error.message === "fetch failed"; +} + /** * Classify errors caused by user input, configuration, auth state, or account * settings. These errors already tell the user what to fix, so upgrade nudges @@ -789,14 +810,21 @@ export function isSearchQueryParseError(error: ApiError): boolean { * Explicit non-user subclasses must be checked before that fallback. */ export function isUserError(error: unknown): boolean { + // A raw "fetch failed" TypeError is a network-level failure — the user's + // environment, not a CLI bug. + if (isNetworkError(error)) { + return true; + } + if (error instanceof ApiError) { - // Status 0 = network-level failure (DNS, ECONNREFUSED) — user environment, - // not a CLI bug. 400 usually means the CLI constructed a bad request, so it - // is treated as a CLI bug — except when the server reports the user's search - // query was unparseable, which is a user input mistake. + // Status 0 = no HTTP response (network failure or TLS cert error) — the + // user's environment/config, not a CLI bug. if (error.status === 0) { return true; } + // 400 usually means the CLI constructed a bad request, so it is treated as + // a CLI bug — except when the server reports the user's search query was + // unparseable, which is a user input mistake. if (isSearchQueryParseError(error)) { return true; } diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index c89fcc66d..c3f253216 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -239,6 +239,24 @@ describe("classifySilenced", () => { expect(classifySilenced(new ApiError("bad", 400))).toBeNull(); }); + test("silences a raw 'fetch failed' TypeError (network failure)", () => { + expect(classifySilenced(new TypeError("fetch failed"))).toBe( + "network_error" + ); + }); + + test("does NOT silence a TLS cert error wrapped as ApiError(0)", () => { + // status 0 is shared by network failures and TLS cert errors; the latter + // are actionable (missing CA) and must stay captured. + expect( + classifySilenced(new ApiError("TLS certificate error", 0)) + ).toBeNull(); + }); + + test("does NOT silence an unrelated TypeError", () => { + expect(classifySilenced(new TypeError("x is not a function"))).toBeNull(); + }); + test("silences ApiError 400 with a search-query parse detail", () => { expect( classifySilenced( @@ -606,6 +624,18 @@ describe("reportCliError integration", () => { ); }); + test("silences a network error and emits metric", () => { + reportCliError(new TypeError("fetch failed")); + expect(captureSpy).not.toHaveBeenCalled(); + expect(metricSpy).toHaveBeenCalledWith( + "cli.error.silenced", + 1, + expect.objectContaining({ + attributes: expect.objectContaining({ reason: "network_error" }), + }) + ); + }); + test.each([ "not_authenticated", "expired", diff --git a/test/lib/errors.test.ts b/test/lib/errors.test.ts index bc7a4d05c..485235419 100644 --- a/test/lib/errors.test.ts +++ b/test/lib/errors.test.ts @@ -11,6 +11,7 @@ import { formatError, getExitCode, HostScopeError, + isNetworkError, isSearchQueryParseError, isUserError, OutputError, @@ -499,6 +500,11 @@ describe("isUserError", () => { ["SeerError", new SeerError("not_enabled"), true], ["WizardError", new WizardError("wizard failed"), true], ["ApiError 0 (network)", new ApiError("network error", 0), true], + [ + "raw fetch failed TypeError (network)", + new TypeError("fetch failed"), + true, + ], ["ApiError 400", new ApiError("bad request", 400), false], [ "ApiError 400 (search query parse)", @@ -520,6 +526,35 @@ describe("isUserError", () => { }); }); +describe("isNetworkError", () => { + test("true for a raw 'fetch failed' TypeError (undici network failure)", () => { + expect(isNetworkError(new TypeError("fetch failed"))).toBe(true); + }); + + test("false for ApiError status 0 (shared with TLS cert errors)", () => { + // status 0 is also used for TLS cert errors, which must stay actionable. + expect(isNetworkError(new ApiError("Network error", 0))).toBe(false); + expect(isNetworkError(new ApiError("TLS certificate error", 0))).toBe( + false + ); + }); + + test("false for an ApiError with an HTTP status", () => { + expect(isNetworkError(new ApiError("bad", 400))).toBe(false); + expect(isNetworkError(new ApiError("server", 500))).toBe(false); + }); + + test("false for an unrelated TypeError", () => { + expect(isNetworkError(new TypeError("x is not a function"))).toBe(false); + }); + + test("false for non-error values", () => { + expect(isNetworkError(new Error("boom"))).toBe(false); + expect(isNetworkError("fetch failed")).toBe(false); + expect(isNetworkError(null)).toBe(false); + }); +}); + describe("isSearchQueryParseError", () => { test("true for a 400 whose detail reports an unparseable query", () => { expect(