fix(cli): validate --input-csv Service cells and bound Count

CHANGELOG.md

@@ -41,6 +41,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 - `configure-azure` no longer creates the Entra app, client secret and role
   assignment when you press Enter at the create-service-principal prompt; Enter
   now skips, as in the GCP wizard (#1613).
+- `--input-csv` now rejects a row whose `Service` cell is not one of the service
+  names the CSV report writes (for example the `--services` alias
+  `elasticsearch`, `RDS` or `rds` with a trailing space), and a `Count` above
+  10000, failing the load with the row and column. Such a row used to load and
+  then be skipped mid-run with "Service client not yet implemented" (#379).
 - Savings Plans purchases now send the hourly commitment truncated to 5 decimals
   (at least 2 shown) instead of rounded to cents, and accept amounts from 0.001
   to 1,000,000, so a 0.001 to 0.004 commitment is no longer rejected (library

cmd/multi_service_csv.go

@@ -83,12 +83,26 @@ func loadCSVRecommendations(csvPath string, need csvThresholdNeeds) ([]common.Re
 var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count", "Term", "PaymentOption"}
 
 // Accepted Term values match fetchRecommendationsForRegion; accepted
-// PaymentOption values match validatePaymentAndTerm.
+// PaymentOption values match validatePaymentAndTerm. Accepted Service values
+// are the canonical slugs writeMultiServiceCSV emits and createServiceClient
+// builds a client for; --services flag aliases (elasticsearch, savingsplans,
+// savingsplans-compute, ...) are not accepted, since a row the purchase loop
+// cannot route would otherwise be skipped mid-run instead of failing the load.
 var (
 	validCSVTerms          = []string{"1yr", "3yr"}
 	validCSVPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"}
+	validCSVServices       = csvServiceNames()
 )
 
+func csvServiceNames() []string {
+	services := getAllServices()
+	names := make([]string, len(services))
+	for i, s := range services {
+		names[i] = string(s)
+	}
+	return names
+}
+
 // parseCSVEnum requires the cell to be exactly one of allowed.
 func parseCSVEnum(record []string, colIdx map[string]int, fieldName string, allowed []string) (string, error) {
 	col := colIdx[fieldName] + 1
@@ -174,8 +188,13 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int, need csvThreshol
 func parseCSVRecord(record []string, colIdx map[string]int, need csvThresholdNeeds) (common.Recommendation, error) {
 	rec := common.Recommendation{Provider: common.ProviderAWS}
 
+	service, err := parseCSVEnum(record, colIdx, "Service", validCSVServices)
+	if err != nil {
+		return rec, err
+	}
+	rec.Service = common.ServiceType(service)
+
 	// Parse string fields
-	rec.Service = common.ServiceType(getCSVField(record, colIdx, "Service"))
 	rec.Region = getCSVField(record, colIdx, "Region")
 	rec.ResourceType = getCSVField(record, colIdx, "ResourceType")
 	rec.Account = getCSVField(record, colIdx, "Account")
@@ -243,6 +262,10 @@ func parseCSVCount(record []string, colIdx map[string]int, target *int) error {
 	if n < 1 {
 		return fmt.Errorf("column %d %q: must be at least 1, got %d", idx+1, fieldName, n)
 	}
+	// Same ceiling validateFlags puts on --max-instances and --override-count.
+	if n > MaxReasonableInstances {
+		return fmt.Errorf("column %d %q: must be at most %d, got %d", idx+1, fieldName, MaxReasonableInstances, n)
+	}
 	*target = n
 	return nil
 }

cmd/multi_service_csv_strict_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"path/filepath"
+	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -179,3 +180,92 @@ func TestLoadRecommendationsFromCSV_TermAndPaymentAccepted(t *testing.T) {
 		}
 	}
 }
+
+// Service cells must be one of the canonical service types the purchase loop
+// builds a client for (#379, audit A10-007). A flag alias such as
+// "elasticsearch", a padded or capitalized slug, or the "savingsplans"
+// umbrella used to load as an unknown ServiceType, and the row was then
+// skipped with "Service client not yet implemented" instead of failing.
+func TestLoadRecommendationsFromCSV_ServiceValues(t *testing.T) {
+	tests := []struct {
+		name    string
+		service string
+	}{
+		{"empty", ""},
+		{"whitespace only", " "},
+		{"flag alias", "elasticsearch"},
+		{"trailing space", "rds "},
+		{"uppercase", "RDS"},
+		{"savings plans umbrella", "savingsplans"},
+		{"flag-only savings plans spelling", "savingsplans-compute"},
+		{"unknown", "dynamodb"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			content := "Service,Region,ResourceType,Count,Term,PaymentOption\n" +
+				"rds,us-east-1,db.t3.micro,1,1yr,all-upfront\n" +
+				"\"" + tt.service + "\",us-east-1,db.t3.small,2,1yr,all-upfront\n"
+			err := loadCSVContent(t, content)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "line 3")
+			assert.Contains(t, err.Error(), `column 1 "Service"`)
+		})
+	}
+}
+
+// Every service the tool supports still loads, so the report a run writes can
+// be fed back through --input-csv.
+func TestLoadRecommendationsFromCSV_ServiceAccepted(t *testing.T) {
+	for _, service := range getAllServices() {
+		t.Run(string(service), func(t *testing.T) {
+			recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t,
+				"Service,Region,ResourceType,Count,Term,PaymentOption\n"+
+					string(service)+",us-east-1,m5.large,1,1yr,no-upfront\n"))
+			require.NoError(t, err)
+			require.Len(t, recs, 1)
+			assert.Equal(t, service, recs[0].Service)
+		})
+	}
+}
+
+// Count is bounded by MaxReasonableInstances, the same ceiling validateFlags
+// puts on --max-instances and --override-count (#379).
+func TestLoadRecommendationsFromCSV_CountUpperBound(t *testing.T) {
+	row := func(count int) string {
+		return "Service,Region,ResourceType,Count,Term,PaymentOption\n" +
+			"rds,us-east-1,db.t3.micro," + strconv.Itoa(count) + ",3yr,partial-upfront\n"
+	}
+
+	recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, row(MaxReasonableInstances)))
+	require.NoError(t, err)
+	require.Len(t, recs, 1)
+	assert.Equal(t, MaxReasonableInstances, recs[0].Count)
+
+	err = loadCSVContent(t, row(MaxReasonableInstances+1))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "line 2")
+	assert.Contains(t, err.Error(), `column 4 "Count"`)
+	assert.Contains(t, err.Error(), strconv.Itoa(MaxReasonableInstances))
+}
+
+// End to end: a row with a bad Service cell stops the run before any AWS call
+// and before a report is written, rather than being dropped mid-run.
+func TestRunToolFromCSV_RejectsUnknownService(t *testing.T) {
+	awsHits := isolateAWSEnv(t)
+	cfg := Config{
+		CSVInput: writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption\n"+
+			"rds,us-east-1,db.t3.micro,1,100,1yr,all-upfront\n"+
+			"elasticsearch,us-east-1,m5.large.search,1,100,1yr,all-upfront\n"),
+		CSVOutput:      filepath.Join(t.TempDir(), "report.csv"),
+		AuditLog:       filepath.Join(t.TempDir(), "audit.jsonl"),
+		ActualPurchase: false,
+		Coverage:       100,
+	}
+	err := runToolFromCSV(context.Background(), cfg)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to read CSV file: CSV line 3:")
+	assert.Contains(t, err.Error(), `column 1 "Service"`)
+	assert.Contains(t, err.Error(), `"elasticsearch"`)
+	assert.NoFileExists(t, cfg.CSVOutput)
+	assert.Zero(t, awsHits.Load())
+}

docs/cli/README.md

@@ -91,7 +91,7 @@ All flags belong to the root command unless noted otherwise.
 | Flag | Short | Default | Description |
 |------|-------|---------|-------------|
 | `--output` | `-o` | auto-generated | Output CSV file path. Auto-name format: `ri-helper-dryrun-YYYYMMDD-HHMMSS.csv` or `ri-helper-purchase-YYYYMMDD-HHMMSS.csv`. An explicit path that already exists (file, directory or symlink) is refused instead of overwritten; choose a new path or remove the old file. The `Success` column is the raw flag the provider returned; a provider can report `true` together with an error, so read the `Error` column too (a row with an error counts as failed in the run summary). |
-| `--input-csv` | `-i` | (none) | Read recommendations from an existing CSV instead of fetching from cloud APIs. The file must have a `.csv` extension and exist at the path given. The header must contain `Service`, `Region`, `ResourceType`, `Count`, `Term` and `PaymentOption`; a missing column is a load error that names it. `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` or `no-upfront` on every row; a blank or unknown value fails the load with the row number. CSVs with only the older four columns are rejected. |
+| `--input-csv` | `-i` | (none) | Read recommendations from an existing CSV instead of fetching from cloud APIs. The file must have a `.csv` extension and exist at the path given. The header must contain `Service`, `Region`, `ResourceType`, `Count`, `Term` and `PaymentOption`; a missing column is a load error that names it. `Service` must be one of `rds`, `elasticache`, `ec2`, `opensearch`, `redshift`, `memorydb`, `savings-plans-compute`, `savings-plans-ec2instance`, `savings-plans-sagemaker` or `savings-plans-database` (the names the CSV report writes; `--services` aliases such as `elasticsearch` or `savingsplans` are rejected), `Count` must be a whole number from 1 to 10000, `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` or `no-upfront` on every row; a blank or invalid value fails the load with the row number. CSVs with only the older four columns are rejected. |
 
 ### Authentication