From fcf50b2cfea139f05de714bc2009ded63a9fac6d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 22 Jul 2026 22:57:09 +0200 Subject: [PATCH 1/2] fix(cli): make --purchase execute real purchases (--dry-run defaults false) The --dry-run flag defaulted to true, so effectiveDryRun (!ActualPurchase || DryRun) stayed true even when --purchase was given -- meaning --purchase alone never executed real purchases, defeating the flag. Default --dry-run to false; the safe default is preserved because ActualPurchase defaults to false (a bare invocation is still dry-run). Passing --dry-run alongside --purchase still forces dry-run as an explicit safety override. Adds a regression test that fails on the prior true default and covers all four flag combinations. --- cmd/effective_dry_run_test.go | 47 +++++++++++++++++++++++++++++++++++ cmd/main.go | 2 +- cmd/multi_service.go | 11 +++++--- 3 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 cmd/effective_dry_run_test.go diff --git a/cmd/effective_dry_run_test.go b/cmd/effective_dry_run_test.go new file mode 100644 index 000000000..535b62ae7 --- /dev/null +++ b/cmd/effective_dry_run_test.go @@ -0,0 +1,47 @@ +package main + +import "testing" + +// TestEffectiveDryRun documents the flag contract for real purchases and guards +// the regression where `--purchase` alone silently stayed in dry-run because +// `--dry-run` defaulted to true (effectiveDryRun = !ActualPurchase || DryRun, +// so !true || true == true). The default for --dry-run is now false; safety is +// preserved by ActualPurchase defaulting to false (a bare run is still dry-run). +func TestEffectiveDryRun(t *testing.T) { + tests := []struct { + name string + actualPurchase bool // --purchase + dryRun bool // --dry-run (default false) + want bool + }{ + {name: "bare invocation is dry-run", actualPurchase: false, dryRun: false, want: true}, + {name: "--purchase executes real purchases", actualPurchase: true, dryRun: false, want: false}, + {name: "--purchase --dry-run forces dry-run (explicit safety override)", actualPurchase: true, dryRun: true, want: true}, + {name: "--dry-run without --purchase stays dry-run", actualPurchase: false, dryRun: true, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := effectiveDryRun(Config{ActualPurchase: tt.actualPurchase, DryRun: tt.dryRun}) + if got != tt.want { + t.Errorf("effectiveDryRun(ActualPurchase=%v, DryRun=%v) = %v, want %v", + tt.actualPurchase, tt.dryRun, got, tt.want) + } + }) + } +} + +// TestDryRunFlagDefaultIsFalse guards the actual fix: the --dry-run flag must +// default to false. When it defaulted to true, `--purchase` alone resolved to +// effectiveDryRun == true (!ActualPurchase || DryRun => !true || true), so real +// purchases never executed. A bare run stays dry-run via ActualPurchase's own +// false default, so flipping this default is safe. +func TestDryRunFlagDefaultIsFalse(t *testing.T) { + f := rootCmd.Flags().Lookup("dry-run") + if f == nil { + t.Fatal("--dry-run flag not registered") + } + if f.DefValue != "false" { + t.Errorf("--dry-run default = %q, want \"false\" (so --purchase alone executes real purchases)", f.DefValue) + } +} diff --git a/cmd/main.go b/cmd/main.go index 118141e61..3f1bbc64c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -134,7 +134,7 @@ func init() { // Purchase pipeline flags rootCmd.Flags().StringVar(&toolCfg.AuditLog, "audit-log", "./cudly-audit.jsonl", "Path to JSONL audit log file") - rootCmd.Flags().BoolVar(&toolCfg.DryRun, "dry-run", true, "Dry-run mode: show what would be purchased without actually buying") + rootCmd.Flags().BoolVar(&toolCfg.DryRun, "dry-run", false, "Force dry-run even when --purchase is set (runs are already dry-run unless --purchase is given)") rootCmd.Flags().StringVar(&toolCfg.IdempotencyWindow, "idempotency-window", "24h", "Lookback window for duplicate purchase detection") rootCmd.Flags().Float64Var(&toolCfg.MinSavingsPct, "min-savings-pct", 0, "Minimum savings percentage to include a recommendation (0 = no filter)") rootCmd.Flags().IntVar(&toolCfg.MaxBreakEvenMonths, "max-break-even-months", 0, "Maximum break-even period in months (0 = no filter)") diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 7c540fa66..2926f3831 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -71,9 +71,14 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro // shutdownRequested is set to true when SIGINT is received during a purchase run. var shutdownRequested atomic.Bool -// effectiveDryRun returns true when either DryRun is explicitly set or -// ActualPurchase is not enabled. Both the non-CSV and CSV code paths use -// this helper so the guard is consistent and defined in one place. +// effectiveDryRun reports whether the run must stay in dry-run mode. A run is +// dry-run unless the user opts into real purchases with --purchase; passing +// --dry-run additionally forces dry-run even alongside --purchase (an explicit +// safety override). --dry-run defaults to false so that --purchase alone +// actually executes purchases; the safe default is preserved because +// ActualPurchase defaults to false, so a bare invocation is still dry-run. +// Both the non-CSV and CSV code paths use this helper so the guard is +// consistent and defined in one place. func effectiveDryRun(cfg Config) bool { return !cfg.ActualPurchase || cfg.DryRun } From 238812fdc3ac137ef0d229da9d70116f4a193004 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 17:32:52 +0200 Subject: [PATCH 2/2] fix(cli): remove --dry-run flag; --purchase alone executes real purchases The `--purchase` flag never bought anything on its own because `--dry-run` defaulted to true and the guard was `!ActualPurchase || DryRun`, so `--purchase` gave `!true || true == true` (a dry run). Users had to pass `--purchase --dry-run=false` to actually buy. (Surfaced by CodeRabbit on #1364.) Rather than flip `--dry-run`'s default to false (which leaves a confusing flag named `--dry-run` that defaults off and only acts as a redundant "force dry-run even with --purchase" override), remove the flag entirely. `--purchase` is now the single purchase control: effectiveDryRun(cfg) = !cfg.ActualPurchase A bare run is always a dry run; `--purchase` is the one opt-in that moves money (still gated by the `--yes` / interactive confirmation prompt). This also unifies the two code paths: cloud-fetch and `--input-csv` mode now behave identically, matching what the CSV path already documented. Docs (purchase-safety.md, cli/README.md) are updated to the single-flag model with a History note explaining the removal. Tests replace the default-value guard with `TestDryRunFlagRemoved`, which fails if the flag is ever reintroduced. --- cmd/effective_dry_run_test.go | 46 +++++++++++++++---------------- cmd/main.go | 2 -- cmd/multi_service.go | 14 +++++----- docs/cli/README.md | 3 +-- docs/cli/purchase-safety.md | 51 +++++++++++++---------------------- 5 files changed, 46 insertions(+), 70 deletions(-) diff --git a/cmd/effective_dry_run_test.go b/cmd/effective_dry_run_test.go index 535b62ae7..5e2195c77 100644 --- a/cmd/effective_dry_run_test.go +++ b/cmd/effective_dry_run_test.go @@ -2,46 +2,42 @@ package main import "testing" -// TestEffectiveDryRun documents the flag contract for real purchases and guards -// the regression where `--purchase` alone silently stayed in dry-run because -// `--dry-run` defaulted to true (effectiveDryRun = !ActualPurchase || DryRun, -// so !true || true == true). The default for --dry-run is now false; safety is -// preserved by ActualPurchase defaulting to false (a bare run is still dry-run). +// TestEffectiveDryRun documents the single-flag purchase contract: a run is a +// dry run unless the user opts into real purchases with --purchase. This guards +// the original regression (issue surfaced on #1364) where --purchase alone +// silently stayed in dry-run because a separate --dry-run flag defaulted to +// true. That flag has since been removed; --purchase is now the only control, +// so moving money is always an explicit opt-in and a bare run is always safe. func TestEffectiveDryRun(t *testing.T) { tests := []struct { name string actualPurchase bool // --purchase - dryRun bool // --dry-run (default false) want bool }{ - {name: "bare invocation is dry-run", actualPurchase: false, dryRun: false, want: true}, - {name: "--purchase executes real purchases", actualPurchase: true, dryRun: false, want: false}, - {name: "--purchase --dry-run forces dry-run (explicit safety override)", actualPurchase: true, dryRun: true, want: true}, - {name: "--dry-run without --purchase stays dry-run", actualPurchase: false, dryRun: true, want: true}, + {name: "bare invocation is dry-run", actualPurchase: false, want: true}, + {name: "--purchase executes real purchases", actualPurchase: true, want: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := effectiveDryRun(Config{ActualPurchase: tt.actualPurchase, DryRun: tt.dryRun}) + got := effectiveDryRun(Config{ActualPurchase: tt.actualPurchase}) if got != tt.want { - t.Errorf("effectiveDryRun(ActualPurchase=%v, DryRun=%v) = %v, want %v", - tt.actualPurchase, tt.dryRun, got, tt.want) + t.Errorf("effectiveDryRun(ActualPurchase=%v) = %v, want %v", + tt.actualPurchase, got, tt.want) } }) } } -// TestDryRunFlagDefaultIsFalse guards the actual fix: the --dry-run flag must -// default to false. When it defaulted to true, `--purchase` alone resolved to -// effectiveDryRun == true (!ActualPurchase || DryRun => !true || true), so real -// purchases never executed. A bare run stays dry-run via ActualPurchase's own -// false default, so flipping this default is safe. -func TestDryRunFlagDefaultIsFalse(t *testing.T) { - f := rootCmd.Flags().Lookup("dry-run") - if f == nil { - t.Fatal("--dry-run flag not registered") - } - if f.DefValue != "false" { - t.Errorf("--dry-run default = %q, want \"false\" (so --purchase alone executes real purchases)", f.DefValue) +// TestDryRunFlagRemoved guards against reintroducing the --dry-run flag. It was +// removed because it was a footgun: as a default-true flag it silently +// suppressed real purchases even with --purchase (the #1364 regression), and +// once its default was flipped to false it became a redundant "force dry-run +// even with --purchase" override that only muddied the single-flag contract. +// --purchase is now the sole purchase control; a bare run is always a dry run. +func TestDryRunFlagRemoved(t *testing.T) { + if f := rootCmd.Flags().Lookup("dry-run"); f != nil { + t.Errorf("--dry-run flag is registered again (default %q); it was intentionally removed. "+ + "--purchase is the only purchase control; a bare run is always a dry run.", f.DefValue) } } diff --git a/cmd/main.go b/cmd/main.go index 3f1bbc64c..2d3ec4fbd 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -68,7 +68,6 @@ type Config struct { IncludeExtendedSupport bool AllServices bool ActualPurchase bool - DryRun bool SkipConfirmation bool // RecLookbackPeriod controls the LookbackPeriodInDays passed to // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d" @@ -134,7 +133,6 @@ func init() { // Purchase pipeline flags rootCmd.Flags().StringVar(&toolCfg.AuditLog, "audit-log", "./cudly-audit.jsonl", "Path to JSONL audit log file") - rootCmd.Flags().BoolVar(&toolCfg.DryRun, "dry-run", false, "Force dry-run even when --purchase is set (runs are already dry-run unless --purchase is given)") rootCmd.Flags().StringVar(&toolCfg.IdempotencyWindow, "idempotency-window", "24h", "Lookback window for duplicate purchase detection") rootCmd.Flags().Float64Var(&toolCfg.MinSavingsPct, "min-savings-pct", 0, "Minimum savings percentage to include a recommendation (0 = no filter)") rootCmd.Flags().IntVar(&toolCfg.MaxBreakEvenMonths, "max-break-even-months", 0, "Maximum break-even period in months (0 = no filter)") diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 2926f3831..b445632ee 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -72,15 +72,13 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro var shutdownRequested atomic.Bool // effectiveDryRun reports whether the run must stay in dry-run mode. A run is -// dry-run unless the user opts into real purchases with --purchase; passing -// --dry-run additionally forces dry-run even alongside --purchase (an explicit -// safety override). --dry-run defaults to false so that --purchase alone -// actually executes purchases; the safe default is preserved because -// ActualPurchase defaults to false, so a bare invocation is still dry-run. -// Both the non-CSV and CSV code paths use this helper so the guard is -// consistent and defined in one place. +// dry-run unless the user opts into real purchases with --purchase; that single +// flag is the only control. It defaults to false, so a bare invocation is a +// dry run and moving money is always an explicit opt-in. Both the non-CSV and +// CSV code paths use this helper so the guard is consistent and defined in one +// place. func effectiveDryRun(cfg Config) bool { - return !cfg.ActualPurchase || cfg.DryRun + return !cfg.ActualPurchase } // runToolMultiService is the main entry point for processing multiple services. diff --git a/docs/cli/README.md b/docs/cli/README.md index 4ec53063c..30a83af09 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -49,8 +49,7 @@ All flags belong to the root command unless noted otherwise. | Flag | Short | Default | Description | |------|-------|---------|-------------| -| `--dry-run` | | `true` | Show what would be purchased without buying. **Ignored in `--input-csv` mode**, where the dry-run decision is `!ActualPurchase` (i.e. only `--purchase` matters). See [purchase-safety.md](purchase-safety.md) for the interaction with `--purchase`. | -| `--purchase` | | `false` | Execute real purchases. In the normal cloud-fetch path it must be combined with `--dry-run=false` (`--purchase` alone sets `ActualPurchase=true` but `DryRun` stays `true`, so the run remains a dry run). **Exception:** in `--input-csv` mode `--dry-run` is ignored and `--purchase` alone executes real purchases. See [purchase-safety.md](purchase-safety.md). | +| `--purchase` | | `false` | Execute real purchases. This is the only purchase control: a bare run is always a dry run, and `--purchase` alone executes real purchases (identically in cloud-fetch and `--input-csv` modes). Still gated by the `--yes` / interactive confirmation prompt. See [purchase-safety.md](purchase-safety.md). | | `--yes` | | `false` | Skip the interactive confirmation prompt. Use with caution in automation. | | `--audit-log` | | `./cudly-audit.jsonl` | Path to the JSONL audit log file. Written for every recommendation (dry-run and real). See [purchase-safety.md](purchase-safety.md). | | `--idempotency-window` | | `24h` | Lookback window for duplicate purchase detection. Accepted as a Go duration string (not validated by the CLI; currently has no effect on CLI runs). See [purchase-safety.md](purchase-safety.md). | diff --git a/docs/cli/purchase-safety.md b/docs/cli/purchase-safety.md index 505cce1a3..b81dd289a 100644 --- a/docs/cli/purchase-safety.md +++ b/docs/cli/purchase-safety.md @@ -2,50 +2,35 @@ CUDly is designed to be safe by default. Real purchases require multiple explicit opt-ins, and several mechanisms prevent duplicate or unintended buys. -## The purchase decision: --dry-run and --purchase +## The purchase decision: --purchase ```text ---dry-run bool default: true --purchase bool default: false ``` -There are two code paths with different rules: +Whether a run executes real purchases is controlled by a single flag: -- **Cloud-fetch mode** (the default, recommendations fetched from the cloud APIs): - - ```text - isDryRun = !ActualPurchase || DryRun - ``` - -- **CSV input mode** (`--input-csv`): `--dry-run` is **ignored**: - - ```text - isDryRun = !ActualPurchase - ``` - - In CSV mode, `--purchase` alone is enough to execute real purchases. - -In plain terms, for cloud-fetch mode: +```text +isDryRun = !ActualPurchase +``` -| `--purchase` | `--dry-run` | Result | -|---|---|---| -| (not set / false) | (not set / true) | Dry run - nothing purchased | -| `true` | (not set / true) | **Dry run** - `DryRun` default overrides `--purchase` | -| `true` | `false` | Real purchases | -| (not set / false) | `false` | Dry run - `ActualPurchase` is false, so `isDryRun` stays true | +A bare invocation is always a dry run; passing `--purchase` is the one and only opt-in that moves money. This rule is identical in both cloud-fetch mode (the default) and CSV input mode (`--input-csv`). -The footgun: in cloud-fetch mode, passing `--purchase` alone is not sufficient - you must also pass `--dry-run=false`. This is intentional: two flags must be flipped to move money. +| `--purchase` | Result | +|---|---| +| (not set / false) | Dry run - nothing purchased | +| `true` | Real purchases | -The reverse footgun: in `--input-csv` mode the two-flag protection does not apply. `--purchase` alone performs real purchases, and `--dry-run` has no effect. Treat any CSV-mode invocation that includes `--purchase` as a real purchase run. +> **History:** earlier versions had a separate `--dry-run` flag. As a default-true flag it silently suppressed purchases even when `--purchase` was set (you had to pass `--purchase --dry-run=false` to actually buy - a footgun surfaced on #1364), and once its default was flipped to false it became a redundant "force dry-run even with `--purchase`" override that only muddied the contract. It has been removed in favour of the single `--purchase` control. Real purchases still require the `--yes` confirmation (or the interactive prompt) below, so moving money remains a deliberate act. ```bash -# Correct way to execute real purchases (cloud-fetch mode): -cudly --services rds --purchase --dry-run=false +# Dry run (the default - nothing is purchased): +cudly --services rds -# Still a dry run (--dry-run defaults to true): +# Execute real purchases (prompts for confirmation unless --yes is given): cudly --services rds --purchase -# CSV mode: this executes REAL purchases (--dry-run is ignored): +# CSV mode behaves identically: cudly --input-csv recs.csv --purchase ``` @@ -59,7 +44,7 @@ When running in purchase mode (`isDryRun=false`), cudly prints a summary of the ```bash # Unattended purchase (use with care): -cudly --services rds --purchase --dry-run=false --yes +cudly --services rds --purchase --yes ``` ## Audit log: --audit-log @@ -81,7 +66,7 @@ cudly verifies that the audit log path is writable before making any cloud API c ```bash # Write audit records to a shared directory -cudly --services rds --purchase --dry-run=false \ +cudly --services rds --purchase \ --audit-log /var/log/cudly/audit.jsonl ``` @@ -142,4 +127,4 @@ Before any real purchase run: 3. If using `--target-coverage`, verify `--rebuy-window-days` is set appropriately for your RI renewal cadence. 4. Narrow the scope with `--include-regions`, `--include-accounts`, or `--min-savings-pct` before buying across all services. 5. Consider `--max-instances` as a final safety cap for a first run. -6. Note that `--idempotency-window` does not prevent double-buying in the CLI path; use `--dry-run` review and audit-log inspection to guard against retried runs. +6. Note that `--idempotency-window` does not prevent double-buying in the CLI path; use a dry-run review (run without `--purchase`) and audit-log inspection to guard against retried runs.