fix(errors): surface user search-query 400s as ValidationError, keep CLI-built 400s reported (CLI-FA)#1154
fix(errors): surface user search-query 400s as ValidationError, keep CLI-built 400s reported (CLI-FA)#1154BYK wants to merge 2 commits into
Conversation
…LI-built 400s reported (CLI-FA) Silencing every "Error parsing search query" 400 in the error classifiers papered over a real bug class: a 400 means the server rejected our request as malformed, and that has two distinct causes only distinguishable at the command boundary (where flags.query is in scope): - The user typed an invalid --query — a user input mistake. - The CLI built an invalid query itself — a genuine CLI defect. The blanket silence hid both. Now: - toSearchQueryError() converts a parse-400 into a clean, actionable ValidationError ONLY when the user supplied --query (issue/explore/trace list). With no user --query, the original ApiError(400) propagates and stays reported to Sentry as the bug it is. - Removed the isSearchQueryParseError special-case from all three classifiers (classifySilenced, isUserError, isUserApiError); a raw 400 is now always a reported CLI bug.
|
Codecov Results 📊❌ Patch coverage is 60.00%. Project has 5142 uncovered lines. Files with missing lines (9)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 81.47% 81.45% -0.02%
==========================================
Files 397 397 —
Lines 27702 27721 +19
Branches 17991 17996 +5
==========================================
+ Hits 22570 22579 +9
- Misses 5132 5142 +10
- Partials 1862 1863 +1Generated by Codecov Action |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| hasPreviousPage, | ||
| resolveCursor, | ||
| } from "../../lib/db/pagination.js"; | ||
| import { toSearchQueryError } from "../../lib/errors.js"; |
There was a problem hiding this comment.
User-query 400s in log/list, span/list, event/list, and issue/events now falsely reported to Sentry as CLI bugs
Removing isSearchQueryParseError from classifySilenced, isUserError, and isUserApiError requires every command with a user --query flag to call toSearchQueryError before re-throwing API errors. log/list.ts (passes flags.query to listLogs), span/list.ts (passes translateSpanQuery(flags.query) to listSpans), event/list.ts, and issue/events.ts were not updated, so a user typing an invalid --query in those commands will produce an ApiError(400) that is now captured by Sentry as a CLI defect instead of being surfaced as a ValidationError.
Evidence
classifySilencedinerror-reporting.tsnow has no special case for 400: onlyerror.status > 400 && error.status < 500is silenced, soApiError(400)with detail'Error parsing search query'reachescaptureException.isUserErrorinerrors.tssimilarly excludes 400 (error.status > 400 && error.status < 500), so the upgrade nudge fires.log/list.ts:178passesquery: flags.querytolistLogswith no surroundingtoSearchQueryErrorcall confirmed bygrepreturning no matches.span/list.ts:451passes the translated user query tolistSpanswith notoSearchQueryErrorcall.- The three commands that were updated (
issue/list.ts,explore.ts,trace/list.ts) all wrap their API.catchwiththrow toSearchQueryError(error, flags.query), confirming the required pattern.
Also found at 2 additional locations
src/lib/errors.ts:882-887src/lib/telemetry.ts:363-365
Identified by Warden find-bugs · DS5-SH4
There was a problem hiding this comment.
Good catch — fixed in a44094d. Wired toSearchQueryError(error, flags.query) into the remaining commands that hit the events/logs/spans search backend (which emits the Error parsing search query marker): log list, span list, event list, issue events, and trace logs. Other query-forwarding commands (replay/alert/dashboard) use different backends that don't emit that marker, so they were never silenced and toSearchQueryError is a no-op there.
Warden flagged that removing the search-query special-case from the classifiers requires every command forwarding a user --query to call toSearchQueryError, or its parse-400s get reported as CLI bugs. Wire it into the remaining commands that hit the events/logs/spans search backend (which emits "Error parsing search query"): log list, span list, event list, issue events, and trace logs.
github-actions
Bot
added
risk: high
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a44094d. Configure here.
| allProjects: true, | ||
| }).catch((error: unknown): never => { | ||
| // An unparseable user --query is a user input mistake, not a CLI bug. | ||
| throw toSearchQueryError(error, flags.query); |
There was a problem hiding this comment.
Span project mode missing handler
Medium Severity
handleTraceMode wraps listSpans with toSearchQueryError, but handleProjectMode does not. With --query in project mode, a server search-parse 400 stays a raw ApiError(400), so it is reported as a CLI defect and skips the ValidationError UX the PR adds elsewhere.
Reviewed by Cursor Bugbot for commit a44094d. Configure here.
| extraFields: flags.fields, | ||
| }).catch((error: unknown): never => { | ||
| // An unparseable user --query is a user input mistake, not a CLI bug. | ||
| throw toSearchQueryError(error, flags.query); |
There was a problem hiding this comment.
Log follow mode missing handler
Medium Severity
Non-follow fetches use toSearchQueryError on listLogs failures, but the --follow path calls listLogs with no conversion. A bad --query while streaming logs still surfaces as a reported ApiError(400) instead of a fielded ValidationError.
Reviewed by Cursor Bugbot for commit a44094d. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ...timeRangeToApiParams(timeRange), | ||
| extraFields: extraApiFields, | ||
| allProjects: true, | ||
| }).catch((error: unknown): never => { | ||
| // An unparseable user --query is a user input mistake, not a CLI bug. | ||
| throw toSearchQueryError(error, flags.query); | ||
| }) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Bug: The span list command in project mode is missing the toSearchQueryError() error handler, causing invalid user queries to be reported as CLI bugs instead of user errors.
Severity: MEDIUM
Suggested Fix
Wrap the listSpans() API call within handleProjectMode in src/commands/span/list.ts with a .catch() block that uses toSearchQueryError, similar to the implementation in handleTraceMode. This will ensure invalid search queries from users are correctly handled as input errors.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/commands/span/list.ts#L391-L399
Potential issue: The `span list` command has two modes: trace mode and project mode.
While an error handler using `toSearchQueryError` was added to `handleTraceMode` to
correctly handle invalid user search queries, the same handler is missing for the
`listSpans()` call in `handleProjectMode`. As a result, if a user runs `sentry span list
-q "invalid_query"`, the API's 400 error is not converted into a user-friendly
`ValidationError`. Instead, it's treated as an unhandled CLI bug and reported to Sentry,
contradicting the PR's goal of gracefully handling user query typos.
Did we get this right? 👍 / 👎 to inform future reviews.
| import { | ||
| AuthError, | ||
| stringifyUnknown, | ||
| toSearchQueryError, |
There was a problem hiding this comment.
executeTraceSingleFetch skips toSearchQueryError, leaving user-query 400s as raw ApiError in trace mode
In log/list.ts, executeSingleFetch wraps its listLogs call with .catch((error) => { throw toSearchQueryError(error, flags.query); }) so an invalid user --query surfaces as an actionable ValidationError instead of being reported to Sentry as a CLI bug. The parallel executeTraceSingleFetch (used by sentry log list <trace>) calls listTraceLogs at line 487 without this guard. A user-supplied --query rejected by the server with a 400 parse error therefore surfaces as a raw ApiError(400) and is misclassified as a CLI defect, getting reported to Sentry. The sibling command trace/logs.ts (line 213) wraps the identical listTraceLogs call with toSearchQueryError(error, flags.query), confirming the intended pattern. Fix: wrap the listTraceLogs call in executeTraceSingleFetch with .catch((error) => { throw toSearchQueryError(error, flags.query); }).
Evidence
executeSingleFetch(log/list.ts:183-188) catcheslistLogsfailures and rethrows viatoSearchQueryError(error, flags.query).executeTraceSingleFetch(log/list.ts:487) awaitslistTraceLogs(org, traceId, { query, ... })with no.catch, so a parse-400 propagates unchanged.queryis built from the user value:buildProjectQuery(flags.query, projectFilter)(line 486), so an invalid user--queryreaches the endpoint.trace/logs.ts:206-214wraps the samelistTraceLogscall withtoSearchQueryError(error, flags.query), showing the guard was intended for trace-logs queries.
Also found at 2 additional locations
src/commands/log/list.ts:184src/lib/telemetry.ts:364-366
Identified by Warden find-bugs · SWM-6YD
1 participant
Why
Follow-up to #1151 based on review feedback: silencing every search-query 400 papers over a real bug class.
HTTP 400 means the server rejected our request as malformed. That has two distinct causes, only distinguishable at the command boundary (where
flags.queryis in scope):--query→ a user input mistake.--query) → a genuine CLI defect.PR #1151 special-cased
isSearchQueryParseError()in all three classifiers (classifySilenced,isUserError,isUserApiError), which silenced both — so CLI-authored bad queries became invisible.What changed
toSearchQueryError(error, userQuery, extraSuggestions?)insrc/lib/errors.ts: converts a parse-400 into a clean, actionableValidationErroronly when the user supplied--query. When there's no user--query, the originalApiError(400)is returned untouched so it stays reported to Sentry as the CLI bug it is.--query:issue list(both the single- and multi-project 400 paths),explore, andtrace list.isSearchQueryParseErrorspecial-case fromclassifySilenced(+ dropped theapi_query_errorSilenceReason),isUserError, andisUserApiError. A rawApiError(400)is now always treated as a reported CLI bug.Result
ValidationErrorwith the server detail + search-syntax help (correct exit code, no "CLI bug" upgrade nudge).Note
A user-query
ValidationErroris still surfaced to the user and (like otherValidationErrors) reaches Sentry — that volume is now a useful signal for client-side query-linting UX rather than noise mislabeled as a 400 crash. Happy to silence queryValidationErrors separately if preferred.Tests
toSearchQueryErrorunit tests (convert vs passthrough, suggestions, non-ApiError).issue listintegration tests: user--query→ValidationError; no--query→ reportedApiError(400).classifySilenced/isUserError/isUserApiErrortests to assert search-query 400s are no longer silenced.