From 039de8d072717fa25a65876947ad6eaa18e9f317 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 09:44:34 +0200 Subject: [PATCH 1/2] fix(cli): write audit records and check writability on the CSV purchase path cudly --input-csv X --purchase moved real money but wrote zero audit records: processPurchaseLoop (the CSV path's purchase loop) never called common.WriteAuditRecord, unlike executePurchasePipeline on the non-CSV path. The CSV path also never called CheckAuditLogWritable, so even the pre-flight guarantee that a record could have been written was absent. On a partial failure there was no durable, per-recommendation record of which rows succeeded, so an operator could only re-run the whole file -- a double purchase for the rows that already succeeded. Extracted writePurchaseAuditRecord out of executePurchasePipeline so both purchase entry points (the main pipeline and processPurchaseLoop) share it: every recommendation that reaches a purchase attempt, dry run or real, is now recorded to cfg.AuditLog regardless of which path produced it. processPurchaseLoop and the legacy processRegionRecommendations path now thread a runID through so every recommendation carries a run ID grouping it with its invocation, matching the non-CSV path. CheckAuditLogWritable now runs at the top of runToolFromCSV, before the CSV is even read, matching the non-CSV path's pre-flight check. Extracted prepareCSVPurchaseRun out of runToolFromCSV, and added a purchaseAuditStatus helper, to keep both functions under the project's gocyclo budget after adding the new branches. Regression tests: - TestRunToolFromCSV_WritesAuditRecordsOnPurchaseRun runs runToolFromCSV with ActualPurchase=true and invalid AWS credentials (so the purchase call fails the same way a real AWS error would) and asserts the audit log contains one record per recommendation with a run ID, source=cli, and a success/error status. - TestRunToolFromCSV_ChecksAuditLogWritability asserts an unwritable audit-log directory is rejected before the CSV is even read. Proved both fail on the pre-fix code: built a detached worktree of origin/main at this branch's base (12fe8389) with only the two new test functions applied (as a standalone file, since the full test-file diff also carries unrelated processPurchaseLoop signature-adaptation edits that don't compile against the pre-fix signature). Both fail as expected: no audit file is created, and the writability check never runs. Every existing test that reaches processPurchaseLoop now needs its own AuditLog, or it will write real records into wherever the default resolves to. Set toolCfg.AuditLog on every existing test call site that reaches the purchase loop, and added a TestMain that points the shared toolCfg.AuditLog default at a process-scoped temp file as a safety net so a test that reaches the loop without an explicit override does not silently create a stray cmd/cudly-audit.jsonl file in the repo working directory. Verification: go build ./cmd, go vet ./cmd/..., golangci-lint v2.10.1 (the exact CI pin) run --timeout=10m clean, gocyclo -over 10 clean, go mod tidy -diff empty, go test -race -short ./... all green, and no stray files left in the working tree after the full run. Closes #1609 --- cmd/main_test.go | 19 +++++ cmd/multi_service.go | 109 ++++++++++++++++++------ cmd/multi_service_helpers.go | 10 ++- cmd/multi_service_max_instances_test.go | 1 + cmd/multi_service_test.go | 96 +++++++++++++++++++-- 5 files changed, 199 insertions(+), 36 deletions(-) diff --git a/cmd/main_test.go b/cmd/main_test.go index c685a034c..3473675da 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -2,6 +2,8 @@ package main import ( "fmt" + "os" + "path/filepath" "strings" "testing" @@ -10,6 +12,23 @@ import ( "github.com/stretchr/testify/assert" ) +// TestMain overrides toolCfg.AuditLog's cobra-registered default +// ("./cudly-audit.jsonl", relative to the process working directory) before +// any test in this package runs. Since #1609, processPurchaseLoop (shared by +// the --input-csv path and the legacy per-region purchase path) writes a real +// audit record via cfg.AuditLog on every dry-run and real purchase attempt. +// Without this override, any test that reaches that loop without setting its +// own AuditLog would silently create/append to a stray cmd/cudly-audit.jsonl +// file in the repo working directory on every `go test` run. Tests that need +// to assert on audit-log contents still set their own t.TempDir()-scoped +// AuditLog, which takes precedence within that test. +func TestMain(m *testing.M) { + toolCfg.AuditLog = filepath.Join(os.TempDir(), fmt.Sprintf("cudly-test-audit-%d.jsonl", os.Getpid())) + code := m.Run() + _ = os.Remove(toolCfg.AuditLog) + os.Exit(code) +} + func TestParseServices(t *testing.T) { tests := []struct { name string diff --git a/cmd/multi_service.go b/cmd/multi_service.go index c3ed439eb..91ee7aeff 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -421,10 +421,7 @@ func executePurchasePipeline(ctx context.Context, awsCfg aws.Config, recs []comm } result, status := purchaseSingleRec(ctx, awsCfg, rec, i+1, isDryRun, cfg) results = append(results, result) - auditRec := common.NewAuditRecord(runID, rec, result, status, isDryRun, common.PurchaseSourceCLI) - if err := common.WriteAuditRecord(auditRec, cfg.AuditLog); err != nil { - log.Printf("Warning: failed to write audit record: %v", err) - } + writePurchaseAuditRecord(runID, rec, result, status, isDryRun, cfg.AuditLog) if !isDryRun && i < len(recs)-1 && os.Getenv("DISABLE_PURCHASE_DELAY") != "true" { time.Sleep(PurchaseDelaySeconds * time.Second) } @@ -432,6 +429,32 @@ func executePurchasePipeline(ctx context.Context, awsCfg aws.Config, recs []comm return results } +// writePurchaseAuditRecord writes a single purchase's audit record. Shared by +// both purchase entry points -- executePurchasePipeline (the main pipeline) +// and processPurchaseLoop (the --input-csv path) -- so every recommendation +// that reaches a purchase attempt, dry-run or real, is recorded to +// cfg.AuditLog regardless of which one produced it. Before #1609, +// processPurchaseLoop never wrote a record at all, so CSV-mode purchases left +// no audit trail: on a partial failure there was no durable, per-recommendation +// record of which rows succeeded, so an operator could only re-run the whole +// file, which is a double purchase for the rows that already succeeded. +func writePurchaseAuditRecord(runID string, rec common.Recommendation, result common.PurchaseResult, status string, isDryRun bool, auditLogPath string) { + auditRec := common.NewAuditRecord(runID, rec, result, status, isDryRun, common.PurchaseSourceCLI) + if err := common.WriteAuditRecord(auditRec, auditLogPath); err != nil { + log.Printf("Warning: failed to write audit record: %v", err) + } +} + +// purchaseAuditStatus derives the audit status for a completed (non-dry-run) +// purchase attempt. Callers handle the dry-run ("skipped") case separately, +// since that never reaches a PurchaseResult from an actual API call. +func purchaseAuditStatus(result common.PurchaseResult) string { + if result.Success { + return "success" + } + return "error" +} + // purchaseSingleRec executes or dry-runs a single purchase and returns the result + audit status. func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recommendation, index int, isDryRun bool, cfg Config) (purchaseResult common.PurchaseResult, auditStatus string) { AppLogger.Printf(" [%d] %s %s %s (count=%d)\n", index, rec.Service, rec.Region, rec.ResourceType, rec.Count) @@ -489,6 +512,47 @@ func runCSVPathOrFatal(ctx context.Context, cfg Config) { } } +// prepareCSVPurchaseRun validates and loads everything runToolFromCSV needs +// before the per-service purchase loop: the audit log writability, the CSV +// file, filtering/sizing, and the AWS config. Extracted to keep +// runToolFromCSV under the project's gocyclo budget. +// +// The audit-log check runs first and before any cloud API call, matching the +// non-CSV path (CheckAuditLogWritable in runToolMultiService). Before #1609 +// this check ran only on the non-CSV path, so a CSV-mode purchase run could +// reach real purchase calls with no way to have written a durable, +// per-recommendation audit record even in principle. +// +// A nil recs with a nil error means "nothing to process after filtering", +// which the caller treats as success rather than an error. +func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage float64) (recs []common.Recommendation, awsCfg aws.Config, runID string, err error) { + if err = CheckAuditLogWritable(cfg.AuditLog); err != nil { + return nil, aws.Config{}, "", fmt.Errorf("cannot write audit log: %w", err) + } + + AppLogger.Printf("📄 Reading recommendations from CSV: %s\n", cfg.CSVInput) + recs, err = loadRecommendationsFromCSV(cfg.CSVInput) + if err != nil { + return nil, aws.Config{}, "", fmt.Errorf("failed to read CSV file: %w", err) + } + AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs)) + + recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg) + if err != nil { + return nil, aws.Config{}, "", err + } + if len(recs) == 0 { + return nil, aws.Config{}, "", nil + } + + awsCfg, err = loadAWSConfig(ctx, cfg) + if err != nil { + return nil, aws.Config{}, "", fmt.Errorf("failed to load AWS config: %w", err) + } + + return recs, awsCfg, uuid.New().String(), nil +} + // runToolFromCSV processes recommendations from a CSV input file. // It returns an error instead of exiting so the orchestration glue is // unit-testable; the caller (runCSVPathOrFatal) turns errors fatal. @@ -498,32 +562,15 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { csvModeCoverage := determineCSVCoverage(cfg) - AppLogger.Printf("📄 Reading recommendations from CSV: %s\n", cfg.CSVInput) - - // Read recommendations from CSV - recs, err := loadRecommendationsFromCSV(cfg.CSVInput) - if err != nil { - return fmt.Errorf("failed to read CSV file: %w", err) - } - - AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs)) - - // Filter and adjust recommendations - recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg) + recs, awsCfg, runID, err := prepareCSVPurchaseRun(ctx, cfg, csvModeCoverage) if err != nil { return err } - if len(recs) == 0 { AppLogger.Println("⚠️ No recommendations to process after filtering") return nil } - awsCfg, err := loadAWSConfig(ctx, cfg) - if err != nil { - return fmt.Errorf("failed to load AWS config: %w", err) - } - // Create account alias cache for lookup accountCache := NewAccountAliasCache(awsCfg) @@ -581,7 +628,7 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { allAdjustedRecs = append(allAdjustedRecs, recs...) // Process purchases for this region - regionResults := processPurchaseLoop(ctx, recs, region, isDryRun, serviceClient, cfg) + regionResults := processPurchaseLoop(ctx, recs, region, isDryRun, serviceClient, cfg, runID) serviceResults = append(serviceResults, regionResults...) } @@ -721,8 +768,11 @@ func processService(ctx context.Context, awsCfg aws.Config, recClient provider.R return serviceRecs, serviceResults } -// processPurchaseLoop processes purchases for a single region (used by CSV mode). -func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, region string, isDryRun bool, serviceClient provider.ServiceClient, cfg Config) []common.PurchaseResult { +// processPurchaseLoop processes purchases for a single region (used by CSV +// mode). runID groups every recommendation processed across the whole CSV +// run into one audit trail, matching how executePurchasePipeline (the main +// pipeline) generates one runID per invocation. +func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, region string, isDryRun bool, serviceClient provider.ServiceClient, cfg Config, runID string) []common.PurchaseResult { results := make([]common.PurchaseResult, 0, len(recs)) for j := range recs { @@ -731,8 +781,10 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi AppLogger.Printf(" 💳 Purchasing %d instances\n", rec.Count) var result common.PurchaseResult + var status string if isDryRun { result = createDryRunResult(rec, region, j+1, cfg) + status = "skipped" } else { // Ask for confirmation before proceeding with purchases (only on first item) if j == 0 { @@ -744,13 +796,17 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi } if !ConfirmPurchase(totalInstances, totalSavings, cfg.SkipConfirmation) { - // User canceled - return canceled results for all + // User canceled - return canceled results for all. No audit + // record is written for a declined run, matching the + // non-CSV path: runPurchaseAndReport returns before ever + // calling executePurchasePipeline when the user declines. return createCancelledResults(recs, region, cfg) } } // Execute actual purchase result = executePurchase(ctx, rec, region, j+1, serviceClient, cfg) + status = purchaseAuditStatus(result) // Add delay between purchases to avoid rate limiting if j < len(recs)-1 && os.Getenv("DISABLE_PURCHASE_DELAY") != "true" { @@ -758,6 +814,7 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi } } + writePurchaseAuditRecord(runID, rec, result, status, isDryRun, cfg.AuditLog) results = append(results, result) if result.Success { diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index fe45c6509..b537fdfcb 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -14,6 +14,7 @@ import ( azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" "github.com/aws/aws-sdk-go-v2/aws" awsec2 "github.com/aws/aws-sdk-go-v2/service/ec2" + "github.com/google/uuid" ) // EC2ClientInterface defines the interface for EC2 operations. @@ -436,8 +437,13 @@ func processRegionRecommendations( // Check for duplicate RIs. Drop tracking skipped (nil). adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, isDryRun, nil) - // Process purchases - regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg) + // Process purchases. This legacy per-region entry point has no run-wide + // runID of its own (unlike runToolMultiService/runToolFromCSV, which mint + // one per invocation), so each call gets its own -- every recommendation + // it processes still ends up in cfg.AuditLog with a durable, groupable + // record; it is simply not grouped with a sibling region's run. + runID := uuid.New().String() + regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg, runID) result.results = regionResults return result diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 7c11bd54d..0da06c8f1 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -638,6 +638,7 @@ rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 reportPath := filepath.Join(t.TempDir(), "report.csv") toolCfg.CSVInput = csvPath toolCfg.CSVOutput = reportPath + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.ActualPurchase = false toolCfg.Coverage = 100.0 toolCfg.TargetCoverage = 0 diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index e12a60208..c7a8fe5af 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/csv" + "encoding/json" "fmt" "log" "os" @@ -1261,6 +1262,7 @@ func TestProcessPurchaseLoopPurchaseFailure(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 80.0 toolCfg.SkipConfirmation = true @@ -1281,7 +1283,7 @@ func TestProcessPurchaseLoopPurchaseFailure(t *testing.T) { t.Setenv("DISABLE_PURCHASE_DELAY", "true") - results := processPurchaseLoop(ctx, recs, "ap-south-1", false, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "ap-south-1", false, mockClient, toolCfg, "test-run") assert.Len(t, results, 1) assert.False(t, results[0].Success) @@ -1296,6 +1298,7 @@ func TestProcessPurchaseLoopUserCancellation(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 90.0 toolCfg.SkipConfirmation = false // User will be prompted @@ -1324,7 +1327,7 @@ func TestProcessPurchaseLoopUserCancellation(t *testing.T) { t.Setenv("DISABLE_PURCHASE_DELAY", "true") - results := processPurchaseLoop(ctx, recs, "eu-central-1", false, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "eu-central-1", false, mockClient, toolCfg, "test-run") assert.Len(t, results, 2) for _, result := range results { @@ -1343,7 +1346,7 @@ func TestProcessPurchaseLoopEmptyRecommendations(t *testing.T) { mockClient := &MockServiceClient{} - results := processPurchaseLoop(ctx, []common.Recommendation{}, "us-east-1", false, mockClient, toolCfg) + results := processPurchaseLoop(ctx, []common.Recommendation{}, "us-east-1", false, mockClient, toolCfg, "test-run") assert.Empty(t, results) mockClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) @@ -1354,6 +1357,7 @@ func TestProcessServicePurchasesUserCancellation(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 85.0 toolCfg.SkipConfirmation = true // Skip for testing @@ -1372,7 +1376,7 @@ func TestProcessServicePurchasesUserCancellation(t *testing.T) { t.Setenv("DISABLE_PURCHASE_DELAY", "true") - results := processPurchaseLoop(ctx, recs, "us-west-1", false, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "us-west-1", false, mockClient, toolCfg, "test-run") assert.Len(t, results, 1) assert.True(t, results[0].Success) @@ -1386,6 +1390,7 @@ func TestProcessServicePurchasesDryRunMultiple(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 100.0 recs := []common.Recommendation{ @@ -1397,7 +1402,7 @@ func TestProcessServicePurchasesDryRunMultiple(t *testing.T) { mockClient := &MockServiceClient{} // Dry run should not call PurchaseCommitment - results := processPurchaseLoop(ctx, recs, "ap-northeast-1", true, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "ap-northeast-1", true, mockClient, toolCfg, "test-run") assert.Len(t, results, 3) for i, result := range results { @@ -1461,6 +1466,7 @@ func TestProcessPurchaseLoopDryRun(t *testing.T) { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 75.0 recs := []common.Recommendation{ @@ -1472,7 +1478,7 @@ func TestProcessPurchaseLoopDryRun(t *testing.T) { // Logger output disabled for testing - results := processPurchaseLoop(ctx, recs, "us-east-1", true, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "us-east-1", true, mockClient, toolCfg, "test-run") assert.Len(t, results, 2) for _, result := range results { @@ -1495,6 +1501,7 @@ func TestProcessPurchaseLoopActualPurchase(t *testing.T) { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 80.0 toolCfg.SkipConfirmation = true // Skip confirmation for testing @@ -1520,7 +1527,7 @@ func TestProcessPurchaseLoopActualPurchase(t *testing.T) { // Disable purchase delay for testing t.Setenv("DISABLE_PURCHASE_DELAY", "true") - results := processPurchaseLoop(ctx, recs, "eu-west-1", false, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "eu-west-1", false, mockClient, toolCfg, "test-run") assert.Len(t, results, 2) for i, result := range results { @@ -1540,6 +1547,7 @@ func TestProcessPurchaseLoopWithConfirmation(t *testing.T) { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.Coverage = 80.0 toolCfg.SkipConfirmation = true // Skip confirmation to proceed with purchase @@ -1563,7 +1571,7 @@ func TestProcessPurchaseLoopWithConfirmation(t *testing.T) { // Disable purchase delay for testing t.Setenv("DISABLE_PURCHASE_DELAY", "true") - results := processPurchaseLoop(ctx, recs, "us-west-2", false, mockClient, toolCfg) + results := processPurchaseLoop(ctx, recs, "us-west-2", false, mockClient, toolCfg, "test-run") assert.Len(t, results, 1) assert.True(t, results[0].Success) @@ -1763,6 +1771,7 @@ elasticache,us-west-2,cache.t3.micro,redis,1,1yr,All Upfront,123456789012 reportPath := filepath.Join(t.TempDir(), "report.csv") toolCfg.CSVInput = csvPath toolCfg.CSVOutput = reportPath + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.ActualPurchase = false toolCfg.Coverage = tt.coverage toolCfg.TargetCoverage = 0 @@ -1831,6 +1840,7 @@ func TestRunToolFromCSV_NonExistentFile(t *testing.T) { defer func() { toolCfg = origCfg }() toolCfg.CSVInput = filepath.Join(t.TempDir(), "does-not-exist.csv") + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.ActualPurchase = false err := runToolFromCSV(context.Background(), toolCfg) @@ -1846,6 +1856,7 @@ func TestRunToolFromCSV_EmptyFile(t *testing.T) { defer func() { toolCfg = origCfg }() toolCfg.CSVInput = writeTestRecommendationsCSV(t, "") + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.ActualPurchase = false err := runToolFromCSV(context.Background(), toolCfg) @@ -1872,6 +1883,7 @@ rds,us-east-1,db.t3.large,postgres,10,300.00,1yr,All Upfront,123456789012 reportPath := filepath.Join(t.TempDir(), "report.csv") toolCfg.CSVInput = csvPath toolCfg.CSVOutput = reportPath + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.ActualPurchase = false toolCfg.Coverage = 100.0 toolCfg.TargetCoverage = 0 @@ -1905,6 +1917,7 @@ rds,us-east-1,db.t3.medium,mysql,5,1yr,All Upfront,123456789012 reportPath := filepath.Join(t.TempDir(), "report.csv") toolCfg.CSVInput = csvPath toolCfg.CSVOutput = reportPath + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") toolCfg.ActualPurchase = false toolCfg.Coverage = 100.0 toolCfg.TargetCoverage = 0 @@ -1922,6 +1935,73 @@ rds,us-east-1,db.t3.medium,mysql,5,1yr,All Upfront,123456789012 } } +// TestRunToolFromCSV_WritesAuditRecordsOnPurchaseRun reproduces #1609: a real +// purchase run through --input-csv wrote zero audit records, unlike the +// non-CSV path. isolateAWSEnv points the SDK at invalid credentials so the +// real purchase call fails the same way a real AWS error would; the property +// under test is that a durable, per-recommendation audit record is written +// regardless of whether the purchase attempt itself succeeds. +func TestRunToolFromCSV_WritesAuditRecordsOnPurchaseRun(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + isolateAWSEnv(t) + + csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account +rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 +`) + auditPath := filepath.Join(t.TempDir(), "audit.jsonl") + + toolCfg.CSVInput = csvPath + toolCfg.CSVOutput = filepath.Join(t.TempDir(), "report.csv") + toolCfg.AuditLog = auditPath + toolCfg.ActualPurchase = true + toolCfg.SkipConfirmation = true + toolCfg.Coverage = 100.0 + toolCfg.TargetCoverage = 0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + + err := runToolFromCSV(context.Background(), toolCfg) + require.NoError(t, err) + + data, readErr := os.ReadFile(auditPath) // #nosec G304 -- test-owned tempdir path + require.NoError(t, readErr, "a real purchase run through --input-csv must write an audit log") + + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + require.Len(t, lines, 1, "one audit record per recommendation") + + var rec map[string]any + require.NoError(t, json.Unmarshal([]byte(lines[0]), &rec)) + assert.NotEmpty(t, rec["run_id"], "the audit record must carry a run ID grouping the CSV run") + assert.Equal(t, common.PurchaseSourceCLI, rec["source"]) + assert.Equal(t, "db.t3.small", rec["resource_type"]) + assert.Equal(t, false, rec["dry_run"]) + assert.Contains(t, []any{"success", "error"}, rec["status"], + "a real purchase attempt is audited as success or error, never silently dropped") +} + +// TestRunToolFromCSV_ChecksAuditLogWritability reproduces the second half of +// #1609: the --input-csv path never verified the audit log was writable +// before touching AWS, unlike the non-CSV path (CheckAuditLogWritable in +// runToolMultiService). An unwritable audit-log directory must be rejected +// before the CSV is even read, on both dry-run and purchase invocations. +func TestRunToolFromCSV_ChecksAuditLogWritability(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + + csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account +rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 +`) + + toolCfg.CSVInput = csvPath + toolCfg.AuditLog = filepath.Join(t.TempDir(), "does-not-exist-dir", "audit.jsonl") + toolCfg.ActualPurchase = false + + err := runToolFromCSV(context.Background(), toolCfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "audit log") +} + // ==================== Tests for adjustRecommendationForExcludedVersions ==================== // Helper to create test version info with extended support dates. From 917be03302f61db50c469db462d255a445cfb3a9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 13:28:21 +0200 Subject: [PATCH 2/2] fix(cli): rebase onto #1941, retest the purchase-run audit path at the loop level Rebased onto origin/main, which merged #1941 (fail-closed duplicate check) and #1942 (fail-closed --target-coverage) after this branch's base. #1941 changed CI's outcome for this PR's purchase-run test: TestRunToolFromCSV_WritesAuditRecordsOnPurchaseRun drove a real purchase through runToolFromCSV with invalid AWS credentials, relying on the purchase call itself failing. Post-#1941, the duplicate check now fails closed first on those same invalid credentials and refuses the region before ever reaching a purchase attempt, correctly writing no audit record by design -- so the test's audit file was empty and json.Unmarshal("") failed with "unexpected end of JSON input" in CI (the PR's base predated #1941 locally, so the old behavior still ran there, which is why it passed locally and failed in CI). Replaced that test with two more targeted ones: - TestRunToolFromCSV_WritesAuditRecordsOnDryRun exercises the full --input-csv entry point (prepareCSVPurchaseRun's runID -> processPurchaseLoop -> writePurchaseAuditRecord) via a dry run, which #1941 does not affect (the duplicate check only warns and continues on a dry run). - TestProcessPurchaseLoop_WritesAuditRecordForRealPurchase exercises processPurchaseLoop directly with a mocked provider.ServiceClient (table-driven over PurchaseCommitment success and error), the same technique TestProcessPurchaseLoopActualPurchase already uses to test real-purchase behavior without live AWS credentials -- createServiceClient is not injectable, so this is the only way to observe a real purchase attempt's audit status deterministically. Both new tests assert the audit file is non-empty before splitting it into lines, so an empty-file bug fails loudly at that assertion rather than making the length check pass vacuously and failing confusingly at json.Unmarshal. Also had purchaseSingleRec call the purchaseAuditStatus helper instead of duplicating its two-line success/error branch inline. Filed #2103 (p2) for a related gap surfaced while reviewing this: a failed audit-record write mid-run currently logs a warning and lets the purchase loop continue rather than stopping further purchases once the audit trail can no longer be trusted -- out of scope for this fix, which is about the audit trail existing on the CSV path at all. Verification: go build ./cmd, go vet ./cmd/..., golangci-lint v2.10.1 (the exact CI pin) run --timeout=10m clean, gocyclo -over 10 clean, go mod tidy -diff empty, go test -race -short ./... all green (437s), no stray files in the working tree afterward. Proved the new dry-run CSV test fails on the pre-#1609 code (built a worktree of origin/main, which already includes #1941+#1942, with only that test applied): it fails with "a purchase run through --input-csv must write an audit log" since no audit record was written pre-fix. The processPurchaseLoop test fails to compile against the pre-#1609 signature (which has no runID parameter), which is itself evidence of the same gap. --- cmd/multi_service.go | 9 ++-- cmd/multi_service_test.go | 100 +++++++++++++++++++++++++++++++++----- 2 files changed, 91 insertions(+), 18 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 91ee7aeff..9cd3cbb82 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -473,12 +473,11 @@ func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recomm } result := executePurchase(ctx, rec, rec.Region, index, serviceClient, cfg) - status := "success" - if !result.Success { - status = "error" - AppLogger.Printf(" ❌ %v\n", result.Error) - } else { + status := purchaseAuditStatus(result) + if result.Success { AppLogger.Printf(" ✅ %s\n", result.CommitmentID) + } else { + AppLogger.Printf(" ❌ %v\n", result.Error) } return result, status } diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index c7a8fe5af..c774ad032 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1935,13 +1935,22 @@ rds,us-east-1,db.t3.medium,mysql,5,1yr,All Upfront,123456789012 } } -// TestRunToolFromCSV_WritesAuditRecordsOnPurchaseRun reproduces #1609: a real -// purchase run through --input-csv wrote zero audit records, unlike the -// non-CSV path. isolateAWSEnv points the SDK at invalid credentials so the -// real purchase call fails the same way a real AWS error would; the property -// under test is that a durable, per-recommendation audit record is written -// regardless of whether the purchase attempt itself succeeds. -func TestRunToolFromCSV_WritesAuditRecordsOnPurchaseRun(t *testing.T) { +// TestRunToolFromCSV_WritesAuditRecordsOnDryRun reproduces #1609 through the +// public --input-csv entry point: before the fix, processPurchaseLoop never +// wrote an audit record at all, on a dry run or a real purchase. A dry run is +// used here (rather than ActualPurchase=true with invalid credentials) +// because #1941 made the duplicate check fail closed on a real purchase run: +// with no valid AWS credentials the check now refuses the region before ever +// reaching a purchase attempt, and writes no audit record at all by design +// (see TestCheckDuplicates_ErrorOnPurchaseRun_DropsRecsRatherThanFallingBack) +// -- so there is no way to drive an un-mocked real-purchase attempt through +// this entry point in a test. A dry run still exercises the exact wiring +// #1609 fixes (prepareCSVPurchaseRun's runID -> processPurchaseLoop -> +// writePurchaseAuditRecord), since the duplicate check only warns and +// continues on a dry run rather than refusing. +// TestProcessPurchaseLoop_WritesAuditRecordForRealPurchase below covers the +// "success"/"error" statuses a real purchase attempt gets audited with. +func TestRunToolFromCSV_WritesAuditRecordsOnDryRun(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() isolateAWSEnv(t) @@ -1954,8 +1963,7 @@ rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 toolCfg.CSVInput = csvPath toolCfg.CSVOutput = filepath.Join(t.TempDir(), "report.csv") toolCfg.AuditLog = auditPath - toolCfg.ActualPurchase = true - toolCfg.SkipConfirmation = true + toolCfg.ActualPurchase = false toolCfg.Coverage = 100.0 toolCfg.TargetCoverage = 0 toolCfg.MaxInstances = 0 @@ -1965,7 +1973,8 @@ rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 require.NoError(t, err) data, readErr := os.ReadFile(auditPath) // #nosec G304 -- test-owned tempdir path - require.NoError(t, readErr, "a real purchase run through --input-csv must write an audit log") + require.NoError(t, readErr, "a purchase run through --input-csv must write an audit log") + require.NotEmpty(t, data, "the audit log must not be empty -- an empty file would make the next length check pass vacuously") lines := strings.Split(strings.TrimSpace(string(data)), "\n") require.Len(t, lines, 1, "one audit record per recommendation") @@ -1975,9 +1984,74 @@ rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 assert.NotEmpty(t, rec["run_id"], "the audit record must carry a run ID grouping the CSV run") assert.Equal(t, common.PurchaseSourceCLI, rec["source"]) assert.Equal(t, "db.t3.small", rec["resource_type"]) - assert.Equal(t, false, rec["dry_run"]) - assert.Contains(t, []any{"success", "error"}, rec["status"], - "a real purchase attempt is audited as success or error, never silently dropped") + assert.Equal(t, true, rec["dry_run"]) + assert.Equal(t, "skipped", rec["status"]) +} + +// TestProcessPurchaseLoop_WritesAuditRecordForRealPurchase reproduces the +// other half of #1609 at the processPurchaseLoop level: a real (non-dry-run) +// purchase attempt must be audited as "success" or "error", never silently +// dropped. createServiceClient is not injectable (see the comment on +// TestApplyMinCountFloorAfterDuplicateAdjustment), so this exercises +// processPurchaseLoop directly with a mocked provider.ServiceClient rather +// than through runToolFromCSV -- the same technique +// TestProcessPurchaseLoopActualPurchase already uses to test this loop's +// purchase behavior without live AWS credentials. +func TestProcessPurchaseLoop_WritesAuditRecordForRealPurchase(t *testing.T) { + tests := []struct { + result common.PurchaseResult + name string + wantStatus string + }{ + { + name: "success", + result: common.PurchaseResult{Success: true, CommitmentID: "test-purchase-id", Timestamp: time.Now()}, + wantStatus: "success", + }, + { + name: "error", + result: common.PurchaseResult{Success: false, Error: fmt.Errorf("API error: quota exceeded"), Timestamp: time.Now()}, + wantStatus: "error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg.AuditLog = filepath.Join(t.TempDir(), "audit.jsonl") + toolCfg.SkipConfirmation = true + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + + recs := []common.Recommendation{ + {Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 2, EstimatedSavings: 100}, + } + result := tt.result + result.Recommendation = recs[0] + + mockClient := &MockServiceClient{} + mockClient.On("PurchaseCommitment", ctx, recs[0], mock.MatchedBy(func(o common.PurchaseOptions) bool { return o.Source == common.PurchaseSourceCLI })).Return(result, nil) + + processPurchaseLoop(ctx, recs, "us-east-1", false /* isDryRun */, mockClient, toolCfg, "test-run-id") + + data, readErr := os.ReadFile(toolCfg.AuditLog) // #nosec G304 -- test-owned tempdir path + require.NoError(t, readErr, "a real purchase attempt must write an audit log") + require.NotEmpty(t, data, "the audit log must not be empty -- an empty file would make the next length check pass vacuously") + + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + require.Len(t, lines, 1, "one audit record per recommendation") + + var rec map[string]any + require.NoError(t, json.Unmarshal([]byte(lines[0]), &rec)) + assert.Equal(t, "test-run-id", rec["run_id"]) + assert.Equal(t, common.PurchaseSourceCLI, rec["source"]) + assert.Equal(t, false, rec["dry_run"]) + assert.Equal(t, tt.wantStatus, rec["status"]) + + mockClient.AssertExpectations(t) + }) + } } // TestRunToolFromCSV_ChecksAuditLogWritability reproduces the second half of