fix(cli): apply --min-savings-pct and --max-break-even-months on --input-csv

CHANGELOG.md

@@ -20,6 +20,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 - `--target-coverage` with `--rebuy-window-days` no longer buys replacement EC2 and
   other non-RDS RIs again when they were already bought inside the coverage
   lookback: the expiring RIs are netted against those replacements (#2235).
+- `--min-savings-pct` and `--max-break-even-months` now apply on `--input-csv`
+  runs instead of being refused. The CSV report gains `SavingsPercentage` and
+  `BreakEvenMonths` columns; replaying a report that lacks a column a set flag
+  needs (or has it blank) fails with an error naming the flag, never reading the
+  missing figure as 0. When present, the `SavingsPercentage` and
+  `BreakEvenMonths` columns are now parsed strictly (an invalid value fails the
+  load) even when the threshold flags are unset. `--max-instances` still ranks
+  on savings per instance (#1819).
 - Recommendations with no account ID or name are now reported in the drop summary
   when an account filter (`--include-accounts` or `--exclude-accounts`) removes
   them (#1613).

cmd/multi_service.go

@@ -373,10 +373,11 @@ const (
 	// rankBySavingsPercentage is the recommendation-driven path: scorer.Score
 	// sorts on SavingsPercentage, an intensive rate, before the cap runs.
 	rankBySavingsPercentage rankingRule = "highest savings-percentage recommendations first"
-	// rankBySavingsPerInstance is the --input-csv path: a CSV row carries no
-	// savings percentage, so sortBySavingsPerInstance derives the rate from
-	// the EstimatedSavings and Count columns it does carry.
-	rankBySavingsPerInstance rankingRule = "highest savings-per-instance rows first (a CSV row carries no savings percentage)"
+	// rankBySavingsPerInstance is the --input-csv path: the cap is a budget in
+	// instances, and a CSV may lack the savings percentage (older reports), so
+	// sortBySavingsPerInstance derives the rate from the EstimatedSavings and
+	// Count columns every CSV carries.
+	rankBySavingsPerInstance rankingRule = "highest savings-per-instance rows first (EstimatedSavings / Count, not savings percentage)"
 )
 
 // capBinds reports whether --max-instances will actually remove instances from
@@ -704,7 +705,7 @@ func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage floa
 	}
 
 	AppLogger.Printf("📄 Reading recommendations from CSV: %s\n", cfg.CSVInput)
-	recs, err = loadRecommendationsFromCSV(cfg.CSVInput)
+	recs, err = loadCSVRecommendations(cfg.CSVInput, csvThresholdNeedsFor(cfg))
 	if err != nil {
 		return nil, aws.Config{}, "", fmt.Errorf("failed to read CSV file: %w", err)
 	}

cmd/multi_service_csv.go

@@ -33,6 +33,12 @@ func determineCSVCoverage(cfg Config) float64 {
 
 // loadRecommendationsFromCSV reads and returns recommendations from a CSV file.
 func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) {
+	return loadCSVRecommendations(csvPath, csvThresholdNeeds{})
+}
+
+// loadCSVRecommendations is loadRecommendationsFromCSV for a run whose flags
+// need threshold figures from the file (see csvThresholdNeeds).
+func loadCSVRecommendations(csvPath string, need csvThresholdNeeds) ([]common.Recommendation, error) {
 	file, err := os.Open(csvPath) // #nosec G304 -- CLI tool: csvPath is an operator-supplied command-line argument
 	if err != nil {
 		return nil, fmt.Errorf("failed to open CSV file: %w", err)
@@ -58,9 +64,12 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error)
 	if err != nil {
 		return nil, err
 	}
+	if colErr := requireThresholdColumns(colIdx, need); colErr != nil {
+		return nil, colErr
+	}
 
 	// Parse all records
-	parsed, err := parseCSVRecords(reader, colIdx)
+	parsed, err := parseCSVRecords(reader, colIdx, need)
 	if err != nil {
 		return nil, err
 	}
@@ -125,7 +134,7 @@ func buildColumnIndexMap(header []string) (map[string]int, error) {
 }
 
 // parseCSVRecords reads and parses all CSV records.
-func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recommendation, error) {
+func parseCSVRecords(reader *csv.Reader, colIdx map[string]int, need csvThresholdNeeds) ([]common.Recommendation, error) {
 	var recs []common.Recommendation
 
 	for {
@@ -149,7 +158,7 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm
 			continue
 		}
 
-		rec, err := parseCSVRecord(record, colIdx)
+		rec, err := parseCSVRecord(record, colIdx, need)
 		if err != nil {
 			line, _ := reader.FieldPos(0)
 			return nil, fmt.Errorf("CSV line %d: %w", line, err)
@@ -162,7 +171,7 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm
 }
 
 // parseCSVRecord parses a single CSV record into a Recommendation.
-func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendation, error) {
+func parseCSVRecord(record []string, colIdx map[string]int, need csvThresholdNeeds) (common.Recommendation, error) {
 	rec := common.Recommendation{Provider: common.ProviderAWS}
 
 	// Parse string fields
@@ -193,6 +202,10 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati
 		return rec, err
 	}
 
+	if cellErr := parseCSVThresholds(record, colIdx, need, &rec); cellErr != nil {
+		return rec, cellErr
+	}
+
 	// Details come from the Engine/Deployment columns and the appended detail
 	// columns (see csvDetails).
 	details, err := csvDetails(rec, record, colIdx)
@@ -320,7 +333,7 @@ func writeMultiServiceCSV(w io.Writer, results []common.PurchaseResult) error {
 		"ExistingCoverage", "ProjectedCoverage",
 	}
 	// Appended, never reordered: existing readers look columns up by name.
-	header := slices.Concat(baseHeader, csvDetailColumns)
+	header := slices.Concat(baseHeader, csvDetailColumns, csvThresholdColumns)
 	if err := writer.Write(header); err != nil {
 		return fmt.Errorf("failed to write CSV header: %w", err)
 	}
@@ -370,7 +383,7 @@ func writeMultiServiceCSV(w io.Writer, results []common.PurchaseResult) error {
 			formatExistingCoverage(rec),
 			formatPercentOrBlank(rec.ProjectedCoverage),
 		}
-		row := slices.Concat(baseRow, detailCells(rec))
+		row := slices.Concat(baseRow, detailCells(rec), thresholdCells(rec))
 		if err := writer.Write(row); err != nil {
 			return fmt.Errorf("failed to write CSV row: %w", err)
 		}
@@ -432,7 +445,7 @@ func buildTotalRow(results []common.PurchaseResult) []string {
 		"", "", "", "", // CommitmentID, Success, Error, Timestamp
 		"", "", // ExistingCoverage, ProjectedCoverage
 	}
-	return slices.Concat(totals, make([]string, len(csvDetailColumns))) // detail columns do not aggregate
+	return slices.Concat(totals, make([]string, len(csvDetailColumns)+len(csvThresholdColumns))) // detail and threshold columns do not aggregate
 }
 
 // formatIntOrBlank renders an int as its decimal string when non-zero, ""

cmd/multi_service_csv_cap.go

@@ -20,26 +20,23 @@ import (
 // --min-count was never consulted, so a row the cap truncated below the floor
 // was purchased short.
 //
-// Only MinCount is enforced here. A CSV row carries no savings percentage and
-// no break-even figure (writeMultiServiceCSVReport emits neither column and
-// parseCSVRecord reads neither), so both fields load as zero and gating on
-// them would reject every row of every file. --min-savings-pct and
-// --max-break-even-months are therefore refused up front by
-// validateCSVModeFilterFlags rather than accepted and ignored here; #1819
-// tracks teaching the CSV format to carry those columns.
+// --min-savings-pct and --max-break-even-months are enforced here too, on the
+// SavingsPercentage and BreakEvenMonths columns the report writer emits. The
+// loader refuses a file that lacks a column a set flag needs (see
+// csvThresholdNeeds), so a gate never reads a coerced zero.
 //
-// scorer.Score's own ordering is useless here: SavingsPercentage is uniformly
-// zero on loaded rows, so it resolves on EstimatedSavings, a whole-row dollar
-// total, while --max-instances is a budget in instances. Ranking a total
-// against a per-instance budget spends the whole budget on whichever row is
-// merely biggest, not on the rows that return the most per instance bought.
+// scorer.Score's own ordering is useless here: it sorts on SavingsPercentage and
+// then EstimatedSavings, a whole-row dollar total, while --max-instances is a
+// budget in instances. Ranking a total against a per-instance budget spends the
+// whole budget on whichever row is merely biggest, not on the rows that return
+// the most per instance bought.
 // sortBySavingsPerInstance therefore re-orders the survivors on the rate
 // derived from the two columns a CSV does carry, which is the greedy the flag
 // implies, and applyGlobalInstanceLimit consumes that order: the cap keeps the
 // best-value rows, names every row it reduces or drops, and drops rather than
 // shortens anything truncated below --min-count.
 func scoreAndLimitCSVRecs(recs []common.Recommendation, cfg Config) ([]common.Recommendation, error) {
-	passed := applyMinCountFloor(recs, cfg.MinCount)
+	passed := applyThresholdGates(applyMinCountFloor(recs, cfg.MinCount), cfg)
 	if err := requireRankingSignal(passed, cfg); err != nil {
 		return nil, err
 	}
@@ -68,6 +65,23 @@ func applyMinCountFloor(recs []common.Recommendation, minCount int) []common.Rec
 	return scored.Passed
 }
 
+// applyThresholdGates drops recommendations that fail --min-savings-pct or
+// --max-break-even-months and names each drop on stdout with scorer.Score's own
+// reason, so the CSV path and the recommendation-driven path reject on the
+// identical predicates. With both at 0 it returns recs untouched.
+func applyThresholdGates(recs []common.Recommendation, cfg Config) []common.Recommendation {
+	if cfg.MinSavingsPct <= 0 && cfg.MaxBreakEvenMonths <= 0 {
+		return recs
+	}
+	scored := scorer.Score(recs, scorer.Config{MinSavingsPct: cfg.MinSavingsPct, MaxBreakEvenMonths: cfg.MaxBreakEvenMonths})
+	for i := range scored.Filtered {
+		f := scored.Filtered[i]
+		AppLogger.Printf("🔒 --min-savings-pct/--max-break-even-months dropped %s %s %s: %s\n",
+			f.Recommendation.Service, f.Recommendation.Region, f.Recommendation.ResourceType, f.FilterReason)
+	}
+	return scored.Passed
+}
+
 // savingsPerInstance is the ranking key for CSV rows: the row's monthly
 // savings divided by the instances it would buy. --max-instances is a budget
 // in instances, so the rows worth keeping are the ones returning the most per

cmd/multi_service_csv_thresholds.go

@@ -0,0 +1,90 @@
+package main
+
+import (
+	"fmt"
+	"math"
+	"strconv"
+	"strings"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+)
+
+// csvThresholdColumns carry the two figures --min-savings-pct and
+// --max-break-even-months gate on, so a report replayed with --input-csv can
+// be filtered the way a live run is. They are appended after csvDetailColumns,
+// never reordered: readers look columns up by name.
+//
+// SavingsPercentage is a percent (25.5 means 25.5%), the unit of the flag and
+// of common.Recommendation.SavingsPercentage. BreakEvenMonths is months.
+// Both are written with the shortest exact decimal, so a write then a read
+// yields the same float64.
+const (
+	csvSavingsPctColumn = "SavingsPercentage"
+	csvBreakEvenColumn  = "BreakEvenMonths"
+)
+
+var csvThresholdColumns = []string{csvSavingsPctColumn, csvBreakEvenColumn}
+
+// csvThresholdNeeds names the threshold figures a run reads from the CSV.
+// A figure that is needed but absent fails the load: common.Recommendation
+// holds both as plain float64, so an absent cell would read as 0, which the
+// savings gate rejects and the break-even gate passes.
+type csvThresholdNeeds struct {
+	savingsPct bool
+	breakEven  bool
+}
+
+func csvThresholdNeedsFor(cfg Config) csvThresholdNeeds {
+	return csvThresholdNeeds{savingsPct: cfg.MinSavingsPct > 0, breakEven: cfg.MaxBreakEvenMonths > 0}
+}
+
+// thresholdCells renders rec for csvThresholdColumns, in that order. The
+// break-even cell is blank when it is unknown or never pays back; "0" is a
+// real value (no upfront payment).
+func thresholdCells(rec common.Recommendation) []string {
+	cells := []string{strconv.FormatFloat(rec.SavingsPercentage, 'f', -1, 64), ""}
+	if months, known := common.BreakEven(rec); known && !math.IsInf(months, 0) {
+		cells[1] = strconv.FormatFloat(months, 'f', -1, 64)
+	}
+	return cells
+}
+
+// requireThresholdColumns refuses a CSV whose header lacks a column a set flag
+// needs (a report written before the columns existed).
+func requireThresholdColumns(colIdx map[string]int, need csvThresholdNeeds) error {
+	if need.savingsPct {
+		if _, ok := colIdx[csvSavingsPctColumn]; !ok {
+			return missingThresholdColumn(csvSavingsPctColumn, "--min-savings-pct")
+		}
+	}
+	if need.breakEven {
+		if _, ok := colIdx[csvBreakEvenColumn]; !ok {
+			return missingThresholdColumn(csvBreakEvenColumn, "--max-break-even-months")
+		}
+	}
+	return nil
+}
+
+func missingThresholdColumn(column, flag string) error {
+	return fmt.Errorf("CSV has no %q column, so %s cannot be applied to it (reports written before #1819 lack it). Regenerate the report with this version, add the column, or drop %s", column, flag, flag)
+}
+
+// parseCSVThresholds reads the threshold cells of one row into rec. A cell is
+// parsed whenever it holds a value, and a blank one is an error only when the
+// matching flag needs it.
+func parseCSVThresholds(record []string, colIdx map[string]int, need csvThresholdNeeds, rec *common.Recommendation) error {
+	if err := parseThresholdCell(record, colIdx, csvSavingsPctColumn, need.savingsPct, "--min-savings-pct", &rec.SavingsPercentage); err != nil {
+		return err
+	}
+	return parseThresholdCell(record, colIdx, csvBreakEvenColumn, need.breakEven, "--max-break-even-months", &rec.BreakEvenMonths)
+}
+
+func parseThresholdCell(record []string, colIdx map[string]int, column string, required bool, flag string, target *float64) error {
+	if strings.TrimSpace(getCSVField(record, colIdx, column)) == "" {
+		if required {
+			return fmt.Errorf("column %q is blank, so %s cannot be applied to this row", column, flag)
+		}
+		return nil
+	}
+	return parseCSVFloat(record, colIdx, column, target)
+}