From 2ca29ccf73de6e09e16d84c99beec4695ce31ec4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 18:03:45 +0200 Subject: [PATCH] fix(cli): bound --min-savings-pct to 0-100 --min-savings-pct was only checked for being finite. A negative value behaved as no filter and a value above 100 dropped every recommendation, since savings percentages are measured against on-demand and cannot exceed 100. Reject both at validation, document the range, and rename the flag-parsing validation test to cover out-of-range as well as non-finite values. Closes #2161 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ cmd/validators.go | 12 +++++++++--- cmd/validators_test.go | 10 +++++++--- docs/cli/filtering.md | 2 +- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20baf8820..42151f622 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `--min-savings-pct` now rejects values below 0 or above 100 at validation. + Previously a negative value was accepted as no filter and a value above 100 + silently dropped every recommendation (#2161). - `configure-gcp`: skipping (or giving an unknown answer at) the "Create Service Account" step no longer continues with the default `cudly-service-account@` email that was never created. The wizard now diff --git a/cmd/validators.go b/cmd/validators.go index dda52a279..3ec27991d 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -145,8 +145,8 @@ func requireFinite(name string, v float64) error { return nil } -// validateCoverageAndSavingsPct checks --coverage (finite, 0-100) and that -// --min-savings-pct is finite. target-coverage and min-pool-size call +// validateCoverageAndSavingsPct checks that --coverage and --min-savings-pct +// are finite and within 0-100. target-coverage and min-pool-size call // requireFinite in their own validators. func validateCoverageAndSavingsPct() error { if err := requireFinite("coverage", toolCfg.Coverage); err != nil { @@ -155,7 +155,13 @@ func validateCoverageAndSavingsPct() error { if toolCfg.Coverage < 0 || toolCfg.Coverage > 100 { return fmt.Errorf("coverage percentage must be between 0 and 100, got: %.2f", toolCfg.Coverage) } - return requireFinite("min-savings-pct", toolCfg.MinSavingsPct) + if err := requireFinite("min-savings-pct", toolCfg.MinSavingsPct); err != nil { + return err + } + if toolCfg.MinSavingsPct < 0 || toolCfg.MinSavingsPct > 100 { + return fmt.Errorf("min-savings-pct must be between 0 and 100, got: %.2f", toolCfg.MinSavingsPct) + } + return nil } // validateMinPoolSize checks that --min-pool-size is a non-negative whole number. diff --git a/cmd/validators_test.go b/cmd/validators_test.go index cd548f16d..2bb7b8660 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -815,10 +815,11 @@ func TestCheckDuplicates_HonorsIdempotencyWindow(t *testing.T) { } } -// TestValidateNumericFlagsRejectNonFinite drives the real flag set through +// TestValidateNumericFlagsRejectInvalid drives the real flag set through // rootCmd.ParseFlags (cobra/pflag parse "NaN" and "Inf" as floats) and then -// the validators. Nothing here calls runTool, so no AWS client is built. -func TestValidateNumericFlagsRejectNonFinite(t *testing.T) { +// the validators, covering non-finite and out-of-range values. Nothing here +// calls runTool, so no AWS client is built. +func TestValidateNumericFlagsRejectInvalid(t *testing.T) { saved := toolCfg reset := func() { toolCfg = Config{} @@ -848,6 +849,9 @@ func TestValidateNumericFlagsRejectNonFinite(t *testing.T) { {name: "min-pool-size NaN", args: []string{"--min-pool-size", "NaN"}, wantErr: "min-pool-size must be a finite number, got: NaN"}, {name: "min-savings-pct NaN", args: []string{"--min-savings-pct", "NaN"}, wantErr: "min-savings-pct must be a finite number, got: NaN"}, {name: "min-savings-pct Inf", args: []string{"--min-savings-pct", "Inf"}, wantErr: "min-savings-pct must be a finite number, got: +Inf"}, + {name: "min-savings-pct negative", args: []string{"--min-savings-pct", "-1"}, wantErr: "min-savings-pct must be between 0 and 100, got: -1.00"}, + {name: "min-savings-pct above 100", args: []string{"--min-savings-pct", "100.5"}, wantErr: "min-savings-pct must be between 0 and 100, got: 100.50"}, + {name: "min-savings-pct bounds stay valid", args: []string{"--min-savings-pct", "100"}}, {name: "overflow rejected by the flag parser", args: []string{"--target-coverage", "1e999"}, parseErr: true}, {name: "explicit target 0 stays valid", args: []string{"--target-coverage", "0"}}, {name: "valid values pass", args: []string{"--target-coverage", "95", "--coverage", "50", "--min-pool-size", "2", "--min-savings-pct", "5"}}, diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 1cfbe9d3f..4008cc2e0 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -149,7 +149,7 @@ cudly --services rds --min-count 3 --min-savings-pct Minimum savings percentage (0 = no filter) ``` -Drop recommendations whose estimated savings percentage falls below this threshold. +Drop recommendations whose estimated savings percentage falls below this threshold. The value must be between 0 and 100. **Important naming distinction:** `--min-savings-pct` is a **percentage** value (e.g. `10` means "at least 10% savings"). This is different from the GUI and API `min_savings` parameter, which filters by **dollar amount**. A value of `30` in `--min-savings-pct` means "30% savings", but `min_savings=30` in the API means "$30 in savings". Mixing up the two by copying a CLI value into the GUI filter (or vice versa) produces silent, incorrect filtering.