-
-
Notifications
You must be signed in to change notification settings - Fork 10
fix(error-reporting): silence ValidationError for invalid trace IDs #1226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,12 +308,17 @@ | |
| expect(classifySilenced(err)).toBe("context_missing"); | ||
| }); | ||
|
|
||
| test("silences ValidationError (user-supplied malformed input)", () => { | ||
| expect(classifySilenced(new ValidationError("bad"))).toBe( | ||
| "validation_error", | ||
| ); | ||
| }); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale ValidationError capture testsMedium Severity Silencing Additional Locations (1)Reviewed by Cursor Bugbot for commit 6284299. Configure here. |
||
| test.each([ | ||
| [ | ||
| "ResolutionError", | ||
| new ResolutionError("Project 'x'", "not found", "sentry issue list"), | ||
| ], | ||
| ["ValidationError", new ValidationError("bad")], | ||
| ["SeerError", new SeerError("not_enabled")], | ||
| ["ConfigError", new ConfigError("bad")], | ||
| ["generic Error", new Error("boom")], | ||
|
|
@@ -485,7 +490,7 @@ | |
|
|
||
| test("ValidationError with field uses field as kind", () => { | ||
| const { tags } = capturedScopeTags(new ValidationError("Bad", "trace_id")); | ||
| expect(tags["cli_error.class"]).toBe("ValidationError"); | ||
|
Check failure on line 493 in test/lib/error-reporting.test.ts
|
||
| expect(tags["cli_error.kind"]).toBe("trace_id"); | ||
| }); | ||
|
|
||
|
|
@@ -496,7 +501,7 @@ | |
| 'Invalid trace ID "d2ad4a2d947b5983". Expected 32-char hex.' | ||
| ); | ||
| const { tags } = capturedScopeTags(err); | ||
| expect(tags["cli_error.class"]).toBe("ValidationError"); | ||
|
Check failure on line 504 in test/lib/error-reporting.test.ts
|
||
| expect(tags["cli_error.kind"]).toBe("Invalid trace ID"); | ||
| }); | ||
|
|
||
|
|
@@ -517,7 +522,7 @@ | |
| const eventErr = capturedScopeTags( | ||
| new ValidationError('Invalid event ID "abc"') | ||
| ).tags; | ||
| expect(traceErr["cli_error.kind"]).not.toBe(eventErr["cli_error.kind"]); | ||
|
Check failure on line 525 in test/lib/error-reporting.test.ts
|
||
| }); | ||
|
|
||
| test("captures ResolutionError", () => { | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Silencing
ValidationErrorcausesreportCliErrorto return early, which will break new tests in this PR that expect Sentry tags to be set for this error type.Severity: HIGH
Suggested Fix
The new tests for
ValidationErrortagging should be removed or modified. SinceValidationErroris now intentionally silenced and not reported to Sentry with scope, the tests that assert on its Sentry scope tags are no longer valid. Alternatively, if tagging is still desired, the early return inreportCliErrorfor silenced errors needs to be reconsidered.Prompt for AI Agent
Also affects:
test/lib/error-reporting.test.ts:492~518Did we get this right? 👍 / 👎 to inform future reviews.