From ce82004b69c65f3a55d8ce0fbb280e67c0172e94 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 26 Jun 2026 20:59:50 +0000 Subject: [PATCH 1/2] fix(error-reporting): silence search-query parse 400s (CLI-FA) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user passes invalid `--query` syntax (e.g. `is:403`, `lastSeen:>=-24h`, an empty `status:`), the Sentry search endpoint returns a 400 whose detail begins with "Error parsing search query: ...". classifySilenced captures all 400s by default (treating them as CLI-constructed malformed requests), so these user query-syntax mistakes were reported as crash reports — making CLI-FA one of the highest-volume issues (~450 users across issue/explore/trace list). Add a narrow exception: a 400 whose server detail reports an unparseable search query is a user input error, not a CLI bug. The API already returns an actionable message, so silence it (preserving volume via the cli.error.silenced metric). All other 400s remain captured. The match is a substring check on the server's own parser message, which is robust to the CLI re-wrapping list errors (enrichIssueListError prepends the original detail). --- src/lib/error-reporting.ts | 43 +++++++++++++++++++++++++++++--- test/lib/error-reporting.test.ts | 37 +++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index f3004786b..5867dbfaf 100644 --- a/src/lib/error-reporting.ts +++ b/src/lib/error-reporting.ts @@ -4,8 +4,9 @@ * Provides two things: * * 1. **Silencing rules** — `OutputError`, `ContextError` (a required value the - * user omitted), expected-state `AuthError`, and 401–499 `ApiError` are not - * sent to Sentry as issues. A `cli.error.silenced` metric preserves volume + + * 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 @@ -47,7 +48,35 @@ type SilenceReason = | "output_error" | "context_missing" | "auth_expected" - | "api_user_error"; + | "api_user_error" + | "api_query_error"; + +/** + * Sentry's search-query parser emits a 400 whose detail begins with this + * phrase when the user's `--query` cannot be parsed (e.g. `is:403`, + * `lastSeen:>=-24h`, an empty `status:`). The server is telling us the + * user-supplied query is malformed — definitionally a user input error, not + * the CLI constructing a bad request — so these 400s are silenced. + */ +const SEARCH_QUERY_PARSE_MARKER = "Error parsing search query"; + +/** + * Whether an `ApiError` is a Sentry search-query parse failure (HTTP 400 whose + * detail reports an unparseable query). Used to silence query-syntax mistakes + * that would otherwise be captured by the "400 = CLI bug" default. + * + * The CLI preserves the server's detail when re-wrapping list errors (see + * `enrichIssueListError` in `issue/list.ts`, which prepends the original + * detail), so a substring match is robust to that wrapping. + * + * @internal Exported for testing. + */ +export function isSearchQueryParseError(error: ApiError): boolean { + return ( + error.status === 400 && + (error.detail?.includes(SEARCH_QUERY_PARSE_MARKER) ?? false) + ); +} /** * Classify whether an error should be silenced. @@ -78,6 +107,14 @@ export function classifySilenced(error: unknown): SilenceReason | null { if (error instanceof ApiError && error.status > 400 && error.status < 500) { return "api_user_error"; } + // A 400 normally signals a malformed request the CLI built (a code defect), + // so it is captured by default. The exception: when the server reports it + // could not parse the user's search query, the `--query` syntax is wrong + // (CLI-FA: ~450 users across issue/explore/trace list). That is a user input + // error, and the API already returns an actionable message, so silence it. + if (error instanceof ApiError && isSearchQueryParseError(error)) { + return "api_query_error"; + } return null; } diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index 95db79506..f66316278 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -16,6 +16,7 @@ import { enrichEventWithGroupingTags, extractMessagePrefix, extractResourceKind, + isSearchQueryParseError, normalizeEndpoint, reportCliError, } from "../../src/lib/error-reporting.js"; @@ -238,6 +239,42 @@ describe("classifySilenced", () => { expect(classifySilenced(new ApiError("bad", 400))).toBeNull(); }); + test("silences ApiError 400 with a search-query parse detail", () => { + expect( + classifySilenced( + new ApiError( + "Failed to list issues: 400 Bad Request", + 400, + "Error parsing search query: invalid status value of '403'" + ) + ) + ).toBe("api_query_error"); + }); + + test("silences a wrapped issue-list query 400 (detail prepended)", () => { + // enrichIssueListError prepends the server detail, then appends CLI hints. + const detail = + "Error parsing search query: Empty string after 'status:'\n\n" + + "Suggestions:\n • Check your --query syntax"; + expect( + classifySilenced(new ApiError("Failed to fetch issues", 400, detail)) + ).toBe("api_query_error"); + }); + + test("does NOT silence a 400 whose detail is not a query parse error", () => { + expect( + classifySilenced( + new ApiError("bad", 400, "Invalid dashboard widget configuration") + ) + ).toBeNull(); + }); + + test("does NOT treat a 4xx-with-query-marker as a query 400", () => { + // status must be exactly 400; other 4xx already silence via api_user_error. + const err = new ApiError("x", 422, "Error parsing search query: ..."); + expect(isSearchQueryParseError(err)).toBe(false); + }); + test.each([ 500, 502, 503, ])("does NOT silence ApiError with 5xx status %i", (status) => { From aea33136b0765872b2240dfae7995d8bf011bbaa Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 26 Jun 2026 21:20:12 +0000 Subject: [PATCH 2/2] fix: treat search-query parse 400s as user errors in isUserError/isUserApiError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Seer review: the upgrade-nudge logic (isUserError → getErrorUpdateNotification) and span-attribution logic (isUserApiError) only treated status > 400 as user errors, so a silenced search-query parse 400 would still trigger a misleading "Upgrading may resolve this" banner and be mis-attributed. Move isSearchQueryParseError into errors.ts as the single source of truth and use it from all three 4xx classifiers (classifySilenced, isUserError, isUserApiError) so they agree on query-parse 400s. --- src/lib/error-reporting.ts | 28 +---------------------- src/lib/errors.ts | 34 ++++++++++++++++++++++++++-- src/lib/telemetry.ts | 15 ++++++++---- test/lib/error-reporting.test.ts | 2 +- test/lib/errors.test.ts | 39 ++++++++++++++++++++++++++++++++ test/lib/telemetry.test.ts | 8 +++++++ 6 files changed, 92 insertions(+), 34 deletions(-) diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index 5867dbfaf..aa88860ce 100644 --- a/src/lib/error-reporting.ts +++ b/src/lib/error-reporting.ts @@ -27,6 +27,7 @@ import { ContextError, DeviceFlowError, HostScopeError, + isSearchQueryParseError, OutputError, ResolutionError, SeerError, @@ -51,33 +52,6 @@ type SilenceReason = | "api_user_error" | "api_query_error"; -/** - * Sentry's search-query parser emits a 400 whose detail begins with this - * phrase when the user's `--query` cannot be parsed (e.g. `is:403`, - * `lastSeen:>=-24h`, an empty `status:`). The server is telling us the - * user-supplied query is malformed — definitionally a user input error, not - * the CLI constructing a bad request — so these 400s are silenced. - */ -const SEARCH_QUERY_PARSE_MARKER = "Error parsing search query"; - -/** - * Whether an `ApiError` is a Sentry search-query parse failure (HTTP 400 whose - * detail reports an unparseable query). Used to silence query-syntax mistakes - * that would otherwise be captured by the "400 = CLI bug" default. - * - * The CLI preserves the server's detail when re-wrapping list errors (see - * `enrichIssueListError` in `issue/list.ts`, which prepends the original - * detail), so a substring match is robust to that wrapping. - * - * @internal Exported for testing. - */ -export function isSearchQueryParseError(error: ApiError): boolean { - return ( - error.status === 400 && - (error.detail?.includes(SEARCH_QUERY_PARSE_MARKER) ?? false) - ); -} - /** * Classify whether an error should be silenced. * Returns the reason string when silenced, `null` when it should be captured. diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 8427cf14c..386a9ba56 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -754,6 +754,32 @@ export function getExitCode(error: unknown): number { return 1; } +/** + * Sentry's search-query parser emits a 400 whose detail begins with this phrase + * when the user's `--query` cannot be parsed (e.g. `is:403`, `lastSeen:>=-24h`, + * an empty `status:`). The server is telling us the user-supplied query is + * malformed — a user input error, not the CLI constructing a bad request. + */ +const SEARCH_QUERY_PARSE_MARKER = "Error parsing search query"; + +/** + * Whether an {@link ApiError} is a Sentry search-query parse failure (HTTP 400 + * whose detail reports an unparseable query). The CLI preserves the server's + * detail when re-wrapping list errors (`enrichIssueListError` in + * `commands/issue/list.ts` prepends the original detail), so a substring match + * is robust to that wrapping. + * + * Shared by the three 4xx classifiers so they agree on these 400s: + * `classifySilenced` (Sentry capture), {@link isUserError} (upgrade nudge), and + * `isUserApiError` (span attribution). + */ +export function isSearchQueryParseError(error: ApiError): boolean { + return ( + error.status === 400 && + (error.detail?.includes(SEARCH_QUERY_PARSE_MARKER) ?? false) + ); +} + /** * Classify errors caused by user input, configuration, auth state, or account * settings. These errors already tell the user what to fix, so upgrade nudges @@ -765,11 +791,15 @@ export function getExitCode(error: unknown): number { export function isUserError(error: unknown): boolean { 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. - // Other 4xx statuses are user/account/API-state problems. + // 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; } + if (isSearchQueryParseError(error)) { + return true; + } return error.status > 400 && error.status < 500; } diff --git a/src/lib/telemetry.ts b/src/lib/telemetry.ts index 9bdd1529b..e29fb3f0f 100644 --- a/src/lib/telemetry.ts +++ b/src/lib/telemetry.ts @@ -37,7 +37,7 @@ import { enrichEventWithGroupingTags, reportCliError, } from "./error-reporting.js"; -import { ApiError } from "./errors.js"; +import { ApiError, isSearchQueryParseError } from "./errors.js"; import { attachSentryReporter, logger } from "./logger.js"; import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js"; import { makeCompressedTransport } from "./telemetry/zstd-transport.js"; @@ -347,8 +347,10 @@ export function isEbadfError(event: Sentry.ErrorEvent): boolean { * Check if an error is a user-caused (401–499) API error. * * 401–499 errors are user errors — wrong issue IDs, no access, rate limited — - * not CLI bugs. 400 Bad Request is **excluded** because it indicates the CLI - * constructed a malformed API request, which is a code defect. + * not CLI bugs. 400 Bad Request is **excluded** because it usually indicates + * the CLI constructed a malformed API request, which is a code defect — except + * when the server reports the user's search query was unparseable, which is a + * user input mistake (kept in sync with `classifySilenced` / `isUserError`). * * These should be recorded as span attributes for volume-spike detection in * Discover, but should NOT be captured as Sentry exceptions. @@ -356,7 +358,12 @@ export function isEbadfError(event: Sentry.ErrorEvent): boolean { * @internal Exported for testing */ export function isUserApiError(error: unknown): boolean { - return error instanceof ApiError && error.status > 400 && error.status < 500; + if (!(error instanceof ApiError)) { + return false; + } + return ( + isSearchQueryParseError(error) || (error.status > 400 && error.status < 500) + ); } /** diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index f66316278..c89fcc66d 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -16,7 +16,6 @@ import { enrichEventWithGroupingTags, extractMessagePrefix, extractResourceKind, - isSearchQueryParseError, normalizeEndpoint, reportCliError, } from "../../src/lib/error-reporting.js"; @@ -27,6 +26,7 @@ import { ConfigError, ContextError, HostScopeError, + isSearchQueryParseError, OutputError, ResolutionError, SeerError, diff --git a/test/lib/errors.test.ts b/test/lib/errors.test.ts index 7f53e0aa7..bc7a4d05c 100644 --- a/test/lib/errors.test.ts +++ b/test/lib/errors.test.ts @@ -11,6 +11,7 @@ import { formatError, getExitCode, HostScopeError, + isSearchQueryParseError, isUserError, OutputError, ResolutionError, @@ -499,6 +500,11 @@ describe("isUserError", () => { ["WizardError", new WizardError("wizard failed"), true], ["ApiError 0 (network)", new ApiError("network error", 0), true], ["ApiError 400", new ApiError("bad request", 400), false], + [ + "ApiError 400 (search query parse)", + new ApiError("bad", 400, "Error parsing search query: invalid status"), + true, + ], ["ApiError 401", new ApiError("unauthorized", 401), true], ["ApiError 418", new ApiError("teapot", 418), true], ["ApiError 499", new ApiError("client closed", 499), true], @@ -514,6 +520,39 @@ describe("isUserError", () => { }); }); +describe("isSearchQueryParseError", () => { + test("true for a 400 whose detail reports an unparseable query", () => { + expect( + isSearchQueryParseError( + new ApiError("bad", 400, "Error parsing search query: invalid status") + ) + ).toBe(true); + }); + + test("true when the parser detail is prepended by error enrichment", () => { + const detail = + "Error parsing search query: Empty string after 'status:'\n\nSuggestions:"; + expect(isSearchQueryParseError(new ApiError("wrapped", 400, detail))).toBe( + true + ); + }); + + test("false for a 400 without a query parse detail", () => { + expect( + isSearchQueryParseError(new ApiError("bad", 400, "Other failure")) + ).toBe(false); + expect(isSearchQueryParseError(new ApiError("no detail", 400))).toBe(false); + }); + + test("false when status is not 400 (other 4xx handled elsewhere)", () => { + expect( + isSearchQueryParseError( + new ApiError("x", 422, "Error parsing search query: ...") + ) + ).toBe(false); + }); +}); + describe("exit codes", () => { test("CliError base defaults to EXIT.GENERAL (1)", () => { expect(new CliError("err").exitCode).toBe(EXIT.GENERAL); diff --git a/test/lib/telemetry.test.ts b/test/lib/telemetry.test.ts index c32271342..f493243f2 100644 --- a/test/lib/telemetry.test.ts +++ b/test/lib/telemetry.test.ts @@ -320,6 +320,14 @@ describe("isUserApiError", () => { expect(isUserApiError(new ApiError("Bad request", 400))).toBe(false); }); + test("returns true for 400 search-query parse error (user input)", () => { + expect( + isUserApiError( + new ApiError("bad", 400, "Error parsing search query: invalid status") + ) + ).toBe(true); + }); + test("returns true for 401 Unauthorized", () => { expect(isUserApiError(new ApiError("Unauthorized", 401))).toBe(true); });