diff --git a/cmd/effective_dry_run_test.go b/cmd/effective_dry_run_test.go new file mode 100644 index 000000000..5e2195c77 --- /dev/null +++ b/cmd/effective_dry_run_test.go @@ -0,0 +1,43 @@ +package main + +import "testing" + +// 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 + want bool + }{ + {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}) + if got != tt.want { + t.Errorf("effectiveDryRun(ActualPurchase=%v) = %v, want %v", + tt.actualPurchase, got, tt.want) + } + }) + } +} + +// 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 118141e61..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", true, "Dry-run mode: show what would be purchased without actually buying") 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..b445632ee 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -71,11 +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; 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.