Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/lib/error-reporting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ type SilenceReason =
| "output_error"
| "auth_expected"
| "api_user_error"
| "network_error";
| "network_error"
| "validation_error";

/**
* Classify whether an error should be silenced.
Expand Down Expand Up @@ -88,6 +89,13 @@ export function classifySilenced(error: unknown): SilenceReason | null {
// is now only thrown for a genuine 401/403 (see auth/login.ts) — transient
// network/server failures no longer masquerade as it — so it is safe to
// silence alongside the others (CLI-19).
// A ValidationError means the user supplied invalid input (e.g. combining
// --follow with an absolute date range). These are user-input mistakes, not
// CLI bugs, so silencing them keeps Sentry noise-free while still showing
// the user an actionable error message (CLI-296).
if (error instanceof ValidationError) {
return "validation_error";
}
if (error instanceof AuthError) {
return "auth_expected";
}
Expand Down
7 changes: 6 additions & 1 deletion test/lib/error-reporting.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,17 @@
expect(classifySilenced(err)).toBeNull();
});

test("silences ValidationError (user input mistake)", () => {
expect(classifySilenced(new ValidationError("bad"))).toBe(
"validation_error"
);
Comment on lines 298 to +304

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Integration tests for ValidationError will fail because they were not updated to reflect that this error is now silenced in reportCliError, which now returns early before calling Sentry.withScope.
Severity: LOW

Suggested Fix

Update the four integration tests for ValidationError in test/lib/error-reporting.test.ts (starting around line 479) to align with the new behavior. These tests should either be removed or modified to assert that ValidationError is correctly silenced and that Sentry.withScope is not called.

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: test/lib/error-reporting.test.ts#L298-L304

Potential issue: The function `reportCliError` was modified to silence `ValidationError`
instances by returning early. However, four integration tests in
`test/lib/error-reporting.test.ts` were not updated to reflect this new behavior. These
tests mock `Sentry.withScope` and expect it to be called to capture error tags. Because
`reportCliError` now returns early for a `ValidationError`, `Sentry.withScope` is never
invoked. Consequently, the expected tags are not set, and assertions within these tests
will fail, breaking the test suite.

Did we get this right? 👍 / 👎 to inform future reviews.

});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken ValidationError tagging tests

Medium Severity

classifySilenced now returns early for ValidationError, so reportCliError never calls withScope or sets grouping tags. The reportCliError integration tests still use capturedScopeTags with ValidationError and assert cli_error.class / cli_error.kind, which will fail because those tags stay empty.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a8418f9. 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")],
Expand Down Expand Up @@ -473,7 +478,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 481 in test/lib/error-reporting.test.ts

View workflow job for this annotation

GitHub Actions / Unit Tests

test/lib/error-reporting.test.ts > reportCliError integration > ValidationError with field uses field as kind

AssertionError: expected undefined to be 'ValidationError' // Object.is equality - Expected: "ValidationError" + Received: undefined ❯ test/lib/error-reporting.test.ts:481:37
expect(tags["cli_error.kind"]).toBe("trace_id");
});

Expand All @@ -484,7 +489,7 @@
'Invalid trace ID "d2ad4a2d947b5983". Expected 32-char hex.'
);
const { tags } = capturedScopeTags(err);
expect(tags["cli_error.class"]).toBe("ValidationError");

Check failure on line 492 in test/lib/error-reporting.test.ts

View workflow job for this annotation

GitHub Actions / Unit Tests

test/lib/error-reporting.test.ts > reportCliError integration > ValidationError without field falls back to message prefix

AssertionError: expected undefined to be 'ValidationError' // Object.is equality - Expected: "ValidationError" + Received: undefined ❯ test/lib/error-reporting.test.ts:492:37
expect(tags["cli_error.kind"]).toBe("Invalid trace ID");
});

Expand All @@ -505,7 +510,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 513 in test/lib/error-reporting.test.ts

View workflow job for this annotation

GitHub Actions / Unit Tests

test/lib/error-reporting.test.ts > reportCliError integration > ValidationError kind differentiates by validator

AssertionError: expected undefined not to be undefined // Object.is equality ❯ test/lib/error-reporting.test.ts:513:44
});

test("captures ResolutionError", () => {
Expand Down
Loading