diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index f3004786b..aa88860ce 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 @@ -26,6 +27,7 @@ import { ContextError, DeviceFlowError, HostScopeError, + isSearchQueryParseError, OutputError, ResolutionError, SeerError, @@ -47,7 +49,8 @@ type SilenceReason = | "output_error" | "context_missing" | "auth_expected" - | "api_user_error"; + | "api_user_error" + | "api_query_error"; /** * Classify whether an error should be silenced. @@ -78,6 +81,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/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 95db79506..c89fcc66d 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -26,6 +26,7 @@ import { ConfigError, ContextError, HostScopeError, + isSearchQueryParseError, OutputError, ResolutionError, SeerError, @@ -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) => { 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); });