From e9fa0ad47bcc7bbaa1fff40467644b9da973383c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 26 Oct 2026 13:06:35 +0000 Subject: [PATCH] [ENG-6795] Skip Sentry capture for CLI flag validation errors Malformed flags such as `--tags bad` already print a usage error and exit non-zero. Treat cmd.flagError as an expected exit path so it is not reported as a high-priority Sentry issue. Fixes CLI-Q Co-authored-by: TP Honey --- cmd/changes_submit_plan.go | 6 +-- cmd/flags.go | 2 +- cmd/flags_test.go | 76 ++++++++++++++++++++++++++++++ cmd/root.go | 54 ++++++++++++++------- cmd/root_error_test.go | 96 ++++++++++++++++++++++++++++++++++++++ cmd/terraform_plan.go | 2 +- 6 files changed, 211 insertions(+), 25 deletions(-) create mode 100644 cmd/root_error_test.go diff --git a/cmd/changes_submit_plan.go b/cmd/changes_submit_plan.go index 29a0f60b..5d611f89 100644 --- a/cmd/changes_submit_plan.go +++ b/cmd/changes_submit_plan.go @@ -182,11 +182,7 @@ func SubmitPlan(cmd *cobra.Command, args []string) error { enrichedTags, err := parseTagsArgument() if err != nil { - return loggedError{ - err: err, - fields: lf, - message: "Failed to parse tags", - } + return err } labels, err := parseLabelsArgument() diff --git a/cmd/flags.go b/cmd/flags.go index b45c857f..222c86b7 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -48,7 +48,7 @@ func parseTagsArgument() (*sdp.EnrichedTags, error) { for _, tag := range viper.GetStringSlice("tags") { parts := strings.SplitN(tag, "=", 2) if len(parts) != 2 { - return nil, fmt.Errorf("invalid tag format: %s", tag) + return nil, flagError{usage: fmt.Sprintf("invalid --tags value '%s', expected format 'key=value'", tag)} } tags[parts[0]] = parts[1] } diff --git a/cmd/flags_test.go b/cmd/flags_test.go index 5521757a..51b8fbc4 100644 --- a/cmd/flags_test.go +++ b/cmd/flags_test.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "strings" "testing" @@ -8,6 +9,81 @@ import ( "github.com/spf13/viper" ) +func TestParseTagsArgument(t *testing.T) { + tests := []struct { + name string + tags []string + want map[string]string + errorContains string + flagError bool + }{ + { + name: "empty tags", + tags: []string{}, + want: map[string]string{}, + }, + { + name: "single tag", + tags: []string{"env=prod"}, + want: map[string]string{"env": "prod"}, + }, + { + name: "multiple tags", + tags: []string{"env=prod", "team=platform"}, + want: map[string]string{"env": "prod", "team": "platform"}, + }, + { + name: "missing equals sign", + tags: []string{"bad"}, + errorContains: "invalid --tags value 'bad', expected format 'key=value'", + flagError: true, + }, + { + name: "empty value is allowed", + tags: []string{"key="}, + want: map[string]string{"key": ""}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + viper.Reset() + viper.Set("tags", tt.tags) + + got, err := parseTagsArgument() + if tt.errorContains != "" { + if err == nil { + t.Fatalf("parseTagsArgument() expected error containing %q, got nil", tt.errorContains) + } + if !strings.Contains(err.Error(), tt.errorContains) { + t.Fatalf("parseTagsArgument() error = %v, want containing %q", err, tt.errorContains) + } + if tt.flagError { + if _, ok := errors.AsType[flagError](err); !ok { + t.Fatalf("parseTagsArgument() expected flagError, got %T", err) + } + } + return + } + if err != nil { + t.Fatalf("parseTagsArgument() unexpected error: %v", err) + } + if len(got.GetTagValue()) != len(tt.want) { + t.Fatalf("parseTagsArgument() returned %d tags, want %d", len(got.GetTagValue()), len(tt.want)) + } + for key, value := range tt.want { + tag, ok := got.GetTagValue()[key] + if !ok { + t.Fatalf("parseTagsArgument() missing tag %q", key) + } + if tag.GetUserTagValue().GetValue() != value { + t.Fatalf("parseTagsArgument() tag %q = %q, want %q", key, tag.GetUserTagValue().GetValue(), value) + } + } + }) + } +} + func TestParseLabelsArgument(t *testing.T) { tests := []struct { name string diff --git a/cmd/root.go b/cmd/root.go index 07eeab2c..14a4ee4e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -149,24 +149,7 @@ func Execute() { err := rootCmd.ExecuteContext(ctx) if err != nil { - switch err := err.(type) { //nolint:errorlint // the selected error types are all top-level wrappers used by the CLI implementation - case flagError: - // print errors from viper with usage to stderr - fmt.Fprintln(os.Stderr, err) - case loggedError: - log.WithContext(ctx).WithError(err.err).WithFields(err.fields).Error(err.message) - } - if cmdSpan != nil { - // if printing the error was not requested by the appropriate - // wrapper, only record the data to honeycomb and sentry, the - // command already has handled logging - cmdSpan.SetAttributes( - attribute.Bool("ovm.cli.fatalError", true), - attribute.String("ovm.cli.fatalError.msg", err.Error()), - ) - cmdSpan.RecordError(err) - } - sentry.CaptureException(err) + reportCommandError(ctx, err, sentry.CaptureException) } return err @@ -402,6 +385,41 @@ func (f flagError) Error() string { return f.usage } +// captureExceptionFunc is the Sentry capture hook used by reportCommandError. +type captureExceptionFunc func(error) *sentry.EventID + +// reportCommandError prints expected CLI errors, records them on the command +// span for Honeycomb, and reports unexpected failures to Sentry. +// +// flagError is returned for malformed user input (invalid flags, missing +// required arguments, and similar validation). Those are expected exit paths: +// the user already sees the validation message and a non-zero exit. Sending +// them to Sentry creates high-priority noise (ENG-6795 / CLI-Q). +func reportCommandError(ctx context.Context, err error, capture captureExceptionFunc) { + reportToSentry := true + switch e := err.(type) { //nolint:errorlint // the selected error types are all top-level wrappers used by the CLI implementation + case flagError: + // print errors from viper with usage to stderr + fmt.Fprintln(os.Stderr, e) + reportToSentry = false + case loggedError: + log.WithContext(ctx).WithError(e.err).WithFields(e.fields).Error(e.message) + } + if cmdSpan != nil { + // if printing the error was not requested by the appropriate + // wrapper, only record the data to honeycomb and sentry, the + // command already has handled logging + cmdSpan.SetAttributes( + attribute.Bool("ovm.cli.fatalError", true), + attribute.String("ovm.cli.fatalError.msg", err.Error()), + ) + cmdSpan.RecordError(err) + } + if reportToSentry && capture != nil { + capture(err) + } +} + type loggedError struct { err error fields log.Fields diff --git a/cmd/root_error_test.go b/cmd/root_error_test.go new file mode 100644 index 00000000..8b1c0d3c --- /dev/null +++ b/cmd/root_error_test.go @@ -0,0 +1,96 @@ +package cmd + +import ( + "bytes" + "context" + "errors" + "io" + "os" + "strings" + "testing" + + "github.com/getsentry/sentry-go" + log "github.com/sirupsen/logrus" +) + +func TestReportCommandErrorSkipsSentryForFlagError(t *testing.T) { + readStderr := captureStderr(t) + var captured []error + capture := func(err error) *sentry.EventID { + captured = append(captured, err) + return nil + } + + err := flagError{usage: "invalid --tags value 'bad', expected format 'key=value'"} + reportCommandError(context.Background(), err, capture) + + if len(captured) != 0 { + t.Fatalf("flagError should not be sent to Sentry, got %d events: %v", len(captured), captured) + } + got := readStderr() + if !strings.Contains(got, "invalid --tags value 'bad', expected format 'key=value'") { + t.Fatalf("flagError should still be printed to stderr, got %q", got) + } +} + +func TestReportCommandErrorSendsUnexpectedErrorsToSentry(t *testing.T) { + var captured []error + capture := func(err error) *sentry.EventID { + captured = append(captured, err) + return nil + } + + unexpected := errors.New("backend unavailable") + reportCommandError(context.Background(), unexpected, capture) + if len(captured) != 1 || captured[0] != unexpected { + t.Fatalf("unexpected errors should be sent to Sentry, got %v", captured) + } + + logged := loggedError{ + err: errors.New("dial tcp: lookup api.overmind.tech"), + fields: log.Fields{"app": "https://app.overmind.tech"}, + message: "Failed searching for existing changes", + } + reportCommandError(context.Background(), logged, capture) + if len(captured) != 2 { + t.Fatalf("loggedError should still be sent to Sentry, got %d events", len(captured)) + } + if _, ok := captured[1].(loggedError); !ok { + t.Fatalf("expected loggedError to be captured, got %T", captured[1]) + } +} + +func TestReportCommandErrorNilCaptureIsSafe(t *testing.T) { + // Should not panic when Sentry is not configured. + reportCommandError(context.Background(), errors.New("boom"), nil) + reportCommandError(context.Background(), flagError{usage: "invalid --timeout value 'nope'"}, nil) +} + +func captureStderr(t *testing.T) func() string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + old := os.Stderr + os.Stderr = w + buf := &bytes.Buffer{} + done := make(chan struct{}) + go func() { + _, _ = io.Copy(buf, r) + close(done) + }() + restore := func() string { + os.Stderr = old + _ = w.Close() + <-done + _ = r.Close() + return buf.String() + } + t.Cleanup(func() { + if os.Stderr == w { + _ = restore() + } + }) + return restore +} diff --git a/cmd/terraform_plan.go b/cmd/terraform_plan.go index 62d51ec1..3eaebdea 100644 --- a/cmd/terraform_plan.go +++ b/cmd/terraform_plan.go @@ -245,7 +245,7 @@ func TerraformPlanImpl(ctx context.Context, cmd *cobra.Command, oi sdp.OvermindI enrichedTags, err := parseTagsArgument() if err != nil { uploadChangesSpinner.Fail(fmt.Sprintf("Uploading planned changes: failed to parse tags: %v", err)) - return nil + return err } labels, err := parseLabelsArgument()