fix(arg-parsing): throw ValidationError (not raw Error) for format errors#793
Merged
fix(arg-parsing): throw ValidationError (not raw Error) for format errors#793
Conversation
…rors 9 sites in src/lib/arg-parsing.ts used `throw new Error(...)` for user-input validation errors (invalid issue format, invalid sort value, --limit out of range). These escape the error-reporting pipeline as plain `Error` instances, so: 1. They lack the `cli_error.class:"ValidationError"` tag, so the server-side fingerprint rule does not fire and they group as generic errors in Sentry. 2. They are not silenced by the 401-499 user-error filter (not that the filter would apply here — but the semantics are wrong). 3. The `CliError` formatting path does not render them the way ValidationErrors are rendered. Observed as CLI-1C9 today: a user-input 'Invalid issue format: "XQUIK-"' event created a new error group tagged `class=Error` instead of joining the canonical ValidationError parent. Fix: replace all 9 raw `throw new Error(...)` calls with `throw new ValidationError(...)` carrying a `field` label so the fingerprint rule's `{{ tags.cli_error.kind }}` slot is populated. Added a regression test in arg-parsing.test.ts that asserts `parseIssueArg` failure modes throw `ValidationError` instances (not just the message text).
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Contributor
|
Contributor
Codecov Results 📊✅ 138 passed | Total: 138 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ✅ Patch coverage is 88.46%. Project has 1705 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 95.59% 95.57% -0.02%
==========================================
Files 264 264 —
Lines 38445 38454 +9
Branches 0 0 —
==========================================
+ Hits 36748 36749 +1
- Misses 1697 1705 +8
- Partials 0 0 —Generated by Codecov Action |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
9 sites in
src/lib/arg-parsing.tsusedthrow new Error(...)for user-input validation errors. These escape the error-reporting pipeline as plainErrorinstances, so:cli_error.class:"ValidationError"tag the server-side fingerprint rule needs.Errorissues in Sentry instead of joining the canonicalValidationErrorparents.Observed
CLI-1C9today: a user ransentry issue view XQUIK-and created a new Sentry group taggedclass=Errorinstead of joining the canonical "Invalid issue format"ValidationErrorparent.Fix
Replace all 9 raw
throw new Error(...)calls withthrow new ValidationError(...)carrying afieldlabel so the fingerprint rule's{{ tags.cli_error.kind }}slot is populated:validateLimit"limit"parseLogSort"sort"parseOrgProjectArg(empty org with /)undefined(no field needed; falls back to message prefix)parseIssueArg× 6 (various format checks)"issue"Test
Added a regression test that asserts
parseIssueArgfailure modes throwValidationErrorinstances (not just the message text — existing tests only checked the message). If this had existed before, CLI-1C9 would have been caught pre-ship.5275 unit tests pass.