From eb7dfe3ef65d5dc6fe7a36f193c605b5aed3ce49 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 26 Jun 2026 22:11:33 +0000 Subject: [PATCH 1/2] fix: handle network failures gracefully instead of crashing (CLI-16W) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the CLI cannot reach the Sentry API (offline, DNS failure, connection refused/timeout, proxy), Node's fetch rejects with a raw "TypeError: fetch failed". handleFetchError only special-cased abort, TLS, and timeout errors, so generic network failures propagated as an uncaught TypeError — surfaced to the user as a cryptic crash and reported to Sentry as a bug (CLI-16W and related DNS/connect-timeout issues). - Add isNetworkError() in errors.ts (ApiError status 0, or a raw "fetch failed" TypeError) as the single source of truth. - handleFetchError now wraps an exhausted network failure in a clear ApiError(status 0) with an actionable message, mirroring throwApiError. - isUserError treats network errors as user-environment (no upgrade nudge). - classifySilenced silences network errors (reason network_error): a "user is offline" report is not actionable — same rationale as dropping EPIPE/EBADF OS noise in beforeSend. --- src/lib/error-reporting.ts | 22 +++++++++++++------ src/lib/errors.ts | 37 ++++++++++++++++++++++++++------ src/lib/sentry-client.ts | 22 ++++++++++++++++++- test/lib/error-reporting.test.ts | 28 ++++++++++++++++++++++++ test/lib/errors.test.ts | 31 ++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 14 deletions(-) diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index aa88860ce..a0d2264f6 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"; } + // Network-level failures (offline, DNS, connection refused/timeout, proxy) + // mean the CLI could not reach Sentry at all. There is nothing actionable in + // a "user is offline" report, so drop it — same rationale as EPIPE/EBADF OS + // noise in `beforeSend`. Covers both the wrapped ApiError(status 0) and a raw + // `TypeError: "fetch failed"` that escaped before wrapping (CLI-16W). + 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..b89ec6873 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 network-level failure — the CLI could not reach the + * Sentry API at all (offline, DNS failure, connection refused/timeout, proxy). + * + * Two shapes occur: + * - An {@link ApiError} with `status === 0`, produced when the SDK/raw paths + * wrap a fetch rejection (see `throwApiError` / `handleFetchError`). + * - A raw `TypeError: "fetch failed"` — Node's `fetch` (undici) uses this exact + * message for every network-level failure (the real errno is in `cause`). + * + * These reflect the user's environment, not a CLI bug, so they are treated as + * user errors (no upgrade nudge) and are not reported to Sentry as issues — + * the same rationale as dropping EPIPE/EBADF OS noise in `beforeSend`. + */ +export function isNetworkError(error: unknown): boolean { + if (error instanceof ApiError) { + return error.status === 0; + } + 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,16 @@ export function isSearchQueryParseError(error: ApiError): boolean { * Explicit non-user subclasses must be checked before that fallback. */ export function isUserError(error: unknown): boolean { + // Network failures (ApiError status 0 or a raw "fetch failed" TypeError) are + // 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. - 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/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 3531f1d3f..6612ee57e 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -25,7 +25,12 @@ import { } from "./custom-ca.js"; import { applyCustomHeaders } from "./custom-headers.js"; import { getAuthToken, refreshToken } from "./db/auth.js"; -import { ApiError, HostScopeError, TimeoutError } from "./errors.js"; +import { + ApiError, + HostScopeError, + isNetworkError, + TimeoutError, +} from "./errors.js"; import { logger } from "./logger.js"; import { clearLastCacheHitAge, @@ -352,6 +357,21 @@ function handleFetchError( ), }; } + // Network-level failure (DNS, connection refused/timeout, offline, proxy): + // Node's fetch rejects with a raw `TypeError: "fetch failed"`. After retries + // are exhausted, wrap it in a clear ApiError(status 0) instead of surfacing + // the cryptic TypeError (CLI-16W). status 0 marks it as a non-CLI-bug network + // error for `isUserError`/`classifySilenced`, mirroring `throwApiError`. + if (isNetworkError(error) && isLastAttempt) { + return { + action: "throw", + error: new ApiError( + "Network error", + 0, + "Unable to reach the Sentry API. Check your internet connection (or proxy/VPN) and try again." + ), + }; + } if (isLastAttempt) { return { action: "throw", error }; } diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index c89fcc66d..3eed69309 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -239,6 +239,22 @@ describe("classifySilenced", () => { expect(classifySilenced(new ApiError("bad", 400))).toBeNull(); }); + test("silences ApiError with status 0 (network failure)", () => { + expect(classifySilenced(new ApiError("Network error", 0))).toBe( + "network_error" + ); + }); + + test("silences a raw 'fetch failed' TypeError (network failure)", () => { + expect(classifySilenced(new TypeError("fetch failed"))).toBe( + "network_error" + ); + }); + + 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 +622,18 @@ describe("reportCliError integration", () => { ); }); + test("silences a network error and emits metric", () => { + reportCliError(new ApiError("Network error", 0)); + 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..9d8eef4fc 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,31 @@ describe("isUserError", () => { }); }); +describe("isNetworkError", () => { + test("true for ApiError with status 0", () => { + expect(isNetworkError(new ApiError("Network error", 0))).toBe(true); + }); + + test("true for a raw 'fetch failed' TypeError (undici network failure)", () => { + expect(isNetworkError(new TypeError("fetch failed"))).toBe(true); + }); + + 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( From e41d3ca5c2b98ee8802b2b01536b8f23071b1b37 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 26 Jun 2026 22:47:05 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20Seer=20=E2=80=94=20don't?= =?UTF-8?q?=20silence=20TLS=20cert=20errors=20as=20network=20failures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seer (HIGH) flagged that isNetworkError matched ApiError(status 0), which is shared by genuine connectivity failures AND TLS certificate errors — the latter being actionable user-config issues (e.g. a missing CA cert) that must stay visible. isTlsCertError can't reliably re-detect TLS from the wrapped ApiError(0), so distinguishing by status is unsafe. Narrow isNetworkError to ONLY the raw `TypeError: "fetch failed"` (undici's unambiguous network-failure signature), which is exactly what escapes uncaught in CLI-16W. ApiError(status 0) — including TLS cert errors — is no longer treated as a network error and stays captured/actionable. Revert the handleFetchError conversion (it routed network failures through ApiError(0), reintroducing the TLS overload); isUserError keeps its explicit status-0 check so TLS/network ApiErrors still skip the upgrade nudge. Added a regression test proving TLS ApiError(0) is not silenced. --- src/lib/error-reporting.ts | 10 ++++---- src/lib/errors.ts | 39 ++++++++++++++++++-------------- src/lib/sentry-client.ts | 22 +----------------- test/lib/error-reporting.test.ts | 16 +++++++------ test/lib/errors.test.ts | 12 ++++++---- 5 files changed, 45 insertions(+), 54 deletions(-) diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index a0d2264f6..fc6fdafda 100644 --- a/src/lib/error-reporting.ts +++ b/src/lib/error-reporting.ts @@ -64,11 +64,11 @@ export function classifySilenced(error: unknown): SilenceReason | null { if (error instanceof OutputError) { return "output_error"; } - // Network-level failures (offline, DNS, connection refused/timeout, proxy) - // mean the CLI could not reach Sentry at all. There is nothing actionable in - // a "user is offline" report, so drop it — same rationale as EPIPE/EBADF OS - // noise in `beforeSend`. Covers both the wrapped ApiError(status 0) and a raw - // `TypeError: "fetch failed"` that escaped before wrapping (CLI-16W). + // 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"; } diff --git a/src/lib/errors.ts b/src/lib/errors.ts index b89ec6873..001ab80ea 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -781,23 +781,23 @@ export function isSearchQueryParseError(error: ApiError): boolean { } /** - * Whether an error is a network-level failure — the CLI could not reach the - * Sentry API at all (offline, DNS failure, connection refused/timeout, proxy). - * - * Two shapes occur: - * - An {@link ApiError} with `status === 0`, produced when the SDK/raw paths - * wrap a fetch rejection (see `throwApiError` / `handleFetchError`). - * - A raw `TypeError: "fetch failed"` — Node's `fetch` (undici) uses this exact - * message for every network-level failure (the real errno is in `cause`). - * - * These reflect the user's environment, not a CLI bug, so they are treated as - * user errors (no upgrade nudge) and are not reported to Sentry as issues — - * the same rationale as dropping EPIPE/EBADF OS noise in `beforeSend`. + * 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 { - if (error instanceof ApiError) { - return error.status === 0; - } return error instanceof TypeError && error.message === "fetch failed"; } @@ -810,13 +810,18 @@ export function isNetworkError(error: unknown): boolean { * Explicit non-user subclasses must be checked before that fallback. */ export function isUserError(error: unknown): boolean { - // Network failures (ApiError status 0 or a raw "fetch failed" TypeError) are - // the user's environment, not a CLI bug. + // 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 = 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. diff --git a/src/lib/sentry-client.ts b/src/lib/sentry-client.ts index 6612ee57e..3531f1d3f 100644 --- a/src/lib/sentry-client.ts +++ b/src/lib/sentry-client.ts @@ -25,12 +25,7 @@ import { } from "./custom-ca.js"; import { applyCustomHeaders } from "./custom-headers.js"; import { getAuthToken, refreshToken } from "./db/auth.js"; -import { - ApiError, - HostScopeError, - isNetworkError, - TimeoutError, -} from "./errors.js"; +import { ApiError, HostScopeError, TimeoutError } from "./errors.js"; import { logger } from "./logger.js"; import { clearLastCacheHitAge, @@ -357,21 +352,6 @@ function handleFetchError( ), }; } - // Network-level failure (DNS, connection refused/timeout, offline, proxy): - // Node's fetch rejects with a raw `TypeError: "fetch failed"`. After retries - // are exhausted, wrap it in a clear ApiError(status 0) instead of surfacing - // the cryptic TypeError (CLI-16W). status 0 marks it as a non-CLI-bug network - // error for `isUserError`/`classifySilenced`, mirroring `throwApiError`. - if (isNetworkError(error) && isLastAttempt) { - return { - action: "throw", - error: new ApiError( - "Network error", - 0, - "Unable to reach the Sentry API. Check your internet connection (or proxy/VPN) and try again." - ), - }; - } if (isLastAttempt) { return { action: "throw", error }; } diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index 3eed69309..c3f253216 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -239,18 +239,20 @@ describe("classifySilenced", () => { expect(classifySilenced(new ApiError("bad", 400))).toBeNull(); }); - test("silences ApiError with status 0 (network failure)", () => { - expect(classifySilenced(new ApiError("Network error", 0))).toBe( - "network_error" - ); - }); - 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(); }); @@ -623,7 +625,7 @@ describe("reportCliError integration", () => { }); test("silences a network error and emits metric", () => { - reportCliError(new ApiError("Network error", 0)); + reportCliError(new TypeError("fetch failed")); expect(captureSpy).not.toHaveBeenCalled(); expect(metricSpy).toHaveBeenCalledWith( "cli.error.silenced", diff --git a/test/lib/errors.test.ts b/test/lib/errors.test.ts index 9d8eef4fc..485235419 100644 --- a/test/lib/errors.test.ts +++ b/test/lib/errors.test.ts @@ -527,14 +527,18 @@ describe("isUserError", () => { }); describe("isNetworkError", () => { - test("true for ApiError with status 0", () => { - expect(isNetworkError(new ApiError("Network error", 0))).toBe(true); - }); - 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);