Skip to content

fix(error-reporting): silence ValidationError from user input #1225

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.

Sign up for GitHub

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

Open
sentry wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(error-reporting): silence ValidationError from user input #1225

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion src/lib/error-reporting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ type SilenceReason =
| "auth_expected"
| "api_user_error"
| "api_query_error"
| "network_error";
| "network_error"
| "validation_error";

/**
* Classify whether an error should be silenced.
Expand Down Expand Up @@ -82,6 +83,12 @@ export function classifySilenced(error: unknown): SilenceReason | null {
if (error instanceof ContextError) {
return "context_missing";
}
// A ValidationError always means the user supplied an invalid argument value
// (e.g. an empty issue identifier). It is never a CLI bug, so silence the
// whole class to avoid noise from AI-agent callers passing bad input.
if (error instanceof ValidationError) {
return "validation_error";
}
Comment on lines +89 to +91

@sentry sentry Bot Jul 10, 2026

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: When a ValidationError is silenced, the recorded metric in recordSilencedError omits the field property, losing context about which validation rule failed.
Severity: LOW

Suggested Fix

In recordSilencedError(), add a condition to include the field attribute in the metric when the error is an instance of ValidationError and the field property is present. This should follow the existing pattern for ContextError.resource. For example: if (error instanceof ValidationError && error.field) { attributes.field = error.field; }.

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/lib/error-reporting.ts#L89-L91

Potential issue: The pull request silences `ValidationError` instances, causing them to
be processed by the `recordSilencedError` function. However, this function fails to
include the `field` property from the error object when creating the
`cli.error.silenced` metric. This is inconsistent with how analogous errors like
`ContextError` are handled, where the `resource` property is explicitly preserved to
maintain context in metrics. This change results in a loss of observability, as it
becomes impossible to distinguish which specific validation rule failed based on the
silenced error metrics, whereas previously the `field` was used for grouping in Sentry.

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

@cursor cursor Bot Jul 10, 2026

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.

Over-broad ValidationError silencing

Medium Severity

The new branch treats every ValidationError as user input, but some throws come from API or internal state rather than caller arguments. buildFormatFromUrl raises ValidationError when a server install URL has an unrecognized response_format, so those product/API mismatches would now be silenced instead of captured.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f48ea79. Configure here.

@cursor cursor Bot Jul 10, 2026

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.

Outdated silence classification test

Medium Severity

classifySilenced now returns validation_error for ValidationError, but the existing classifySilenced suite still expects ValidationError under "does NOT silence". That assertion will fail, and there is no positive coverage for the new silence reason.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f48ea79. Configure here.

@cursor cursor Bot Jul 10, 2026

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.

Missing validation field metric

Low Severity

Silenced ValidationErrors emit cli.error.silenced without the structured field attribute. recordSilencedError already preserves resource for ContextError and auth_reason for AuthError, so validation volume cannot be broken down by which input failed after capture is dropped.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f48ea79. Configure here.

// All AuthError reasons are expected auth states the user must act on, not
// CLI bugs: `not_authenticated` (no token), `expired` (token aged out), and
// `invalid` (a bad/insufficiently-scoped token the user supplied). `invalid`
Expand Down
Loading