test(cmd): make CSV purchase-path tests assert real behavior

[cristim]

Problem

Closes #1154 (review finding TEST-02, P2).

The CLI purchase-path tests in cmd/ were exercise-only or tautological:

• TestRunToolFromCSV drove the CSV purchase entry point and asserted nothing (assert.NotPanics). Worse, its fixture used CSV headers the parser does not recognize (Instance Type, Instance Count), so every row loaded with Count=0, the entire run processed zero recommendations, and the test stayed green, exactly the silent-regression mode the finding warned about.
• TestRunToolFromCSV_NonExistentFile and _EmptyFile were t.Skip-ped, citing integration coverage that does not exist; runToolFromCSV called log.Fatalf, making the branches structurally untestable.
• TestQueryMajorEngineVersions_Success and _ProfileHandling were tautological and non-hermetic: they loaded live AWS config and asserted on whichever branch the environment produced, issuing real AWS API calls on credentialed machines with no testing.Short() guard, so they ran under CI's go test -short ./....

Fix

• runToolFromCSV now returns an error instead of calling log.Fatalf; runToolMultiService keeps the fail-loud CLI behavior by turning the returned error fatal. No behavior change for users.
• TestRunToolFromCSV asserts observable output: the written purchase report's resource types, post-sizing counts, and dry-run success flags, for both the 100% and 50% coverage paths, using the real round-trip column names (ResourceType, Count, ...).
• The two skipped error-path tests are un-skipped with real assertions on the wrapped error chain.
• _WithMaxInstances / _WithOverrideCount assert the instance cap and count override against the report rows instead of NotPanics.
• The live-AWS engine-version tests are replaced by hermetic stubbed-client tests of queryMajorEngineVersionsWithClient (full engine fan-out, result merging, warn-and-continue on per-engine failure) plus a deterministic profile-precedence test that fails at config load time against a redirected empty shared config, so no network or credentials are involved.
• New isolateAWSEnv helper pins invalid static credentials so the CSV-path orchestration tests behave identically on credentialed developer machines and bare CI runners, and never touch a real account.

Test evidence

• go build ./... clean; go vet ./cmd/ clean; gofmt -l cmd/ clean.
• go test ./cmd/ -run 'TestRunToolFromCSV|TestQueryMajorEngineVersions': 16 passed.
• go test -short ./cmd/...: 752 passed in 7 packages.
• Regression-test validity: the error-path tests could not exist pre-fix (log.Fatalf exits the process), and the happy-path assertions were observed failing against the old broken fixture shape (report never written because the run processed zero recommendations) before the fixture was corrected, demonstrating they catch the silent-orchestration-regression mode described in the finding.

Out of scope (sibling occurrences, same defect shape)

TestQueryMajorEngineVersions_ErrorHandling and TestQueryRunningInstanceEngineVersions_ErrorHandling in cmd/multi_service_engine_versions_test.go have the same live-config tautological pattern but were not cited by TEST-02; left untouched to keep the diff focused.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7135b828-f94c-48e3-b3b2-8d2e542f1554

📥 Commits

Reviewing files that changed from the base of the PR and between 81cd1f4 and 12e80c5.

📒 Files selected for processing (3)

• cmd/multi_service.go
• cmd/multi_service_coverage_test.go
• cmd/multi_service_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/test-02-cli-purchase-path-tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial review

What this PR does well:

• Correctly diagnoses the silent-fixture bug. The old fixture used CSV headers (Instance Type, Instance Count) that buildColumnIndexMap does not recognize, so getCSVField returned "" and parseCSVRecord parsed Count=0; the whole run processed zero recommendations while assert.NotPanics stayed green. Replacing the headers with the round-trip column names (ResourceType, Count, ...) is the right call.
• Expected values are derived from the algorithm, not "whatever the current code outputs": with 50% coverage int(2 * 0.5) = 1 (kept), int(1 * 0.5) = 0 (dropped by newCount > 0). That matches ApplyCoverage in cmd/helpers.go:165-167 and would catch a regression that broke the discrete-count scaling.
• isolateAWSEnv is a clean pattern: pinning invalid static creds + AWS_CONFIG_FILE=os.DevNull makes the orchestration tests deterministic on credentialed laptops and bare CI runners alike.
• Un-skipping _NonExistentFile and _EmptyFile against the wrapped error chain is exactly what TEST-02 asked for. The error matching (failed to read CSV file + failed to open/read CSV header) anchors on the wrapping runToolFromCSV adds plus the underlying loadRecommendationsFromCSV error, so a future refactor that drops the outer wrap or stops wrapping fails the test.

Findings:

1. TestQueryMajorEngineVersionsWithClient_EngineErrorContinues locks in a memory-violating contract.
The test comment says "per-engine API failures are warn-and-continue" and asserts require.NoError(t, err) for ANY error returned by the per-engine call. But fetchMajorEngineVersionsForEngine returns ctx.Err() (line 236-237 of cmd/multi_service_engine_versions.go) on context cancellation, and queryMajorEngineVersionsWithClient (line 220-224) treats that the same as a transient throttling error: log a warning and move to the next engine. This is the exact pattern flagged by feedback_ctx_cancel_terminal.md ("Context cancellation is terminal in loops -- treat context.Canceled/DeadlineExceeded as hard stops in API fan-out loops; never accumulate as lastErr and continue").

The test as written makes this bug harder to fix because the comment legitimizes the behavior as a contract. The underlying fix is small (~5 LOC: if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return nil, err } in queryMajorEngineVersionsWithClient) plus a paired test asserting ctx.Canceled propagates. Filed as #1325.

2. "Real behavior" only covers the dry-run path.
All four runToolFromCSV tests pin ActualPurchase=false. The PR title is "make CSV purchase-path tests assert real behavior" but the actual-purchase orchestration (real processPurchaseLoop, adjustRecsForDuplicates hitting DescribeReservedDBInstances / DescribeReservedCacheNodes / DescribeSavingsPlans, real CommitmentID flow) is unverified. A regression that quietly bypassed the purchase-loop on the real path while still writing a dry-run-shaped report would not be caught. Filed as #1326.

3. CSV parser still silently tolerates unknown / missing required columns -- the root cause TEST-02 worked around.
With wrong column names, getCSVField returns "" for the missing columns and parseCSVRecord happily builds a Recommendation{Count: 0, ResourceType: "", ...}. loadRecommendationsFromCSV prints "Loaded N recommendations" and returns success. The fix in this PR corrects the FIXTURE, not the PARSER. Per feedback_no_silent_fallbacks.md (money path), the parser should fail loudly when required columns (Service, ResourceType, Count, Term, PaymentOption) are missing, instead of producing Count=0 rows that silently no-op the entire run. The PR's test is now a witness for the parser regressing back into silence -- but only because the test ASSERTS report rows; nothing in the parser itself errors on the wrong-header case. Filed as #1327.

4. CSV edge-case coverage is thin.
_EmptyFile only covers the 0-byte case. Headers-only-no-rows would surface loadRecommendationsFromCSV returning an empty slice and runToolFromCSV printing "No recommendations to process" with no error -- not currently tested. Also untested: BOM-prefixed file, CRLF vs LF line endings, UTF-16/Latin-1, quoted commas, quoted newlines, mismatched field counts (encoding/csv would error with csv.ErrFieldCount), numeric overflow on Count (strconv.Atoi -> overflow path). Folded into #1327.

5. Idempotency not tested.
Feeding the same CSV twice into a real-purchase run isn't covered. Memory feedback_upsert_key_matches_hash.md is adjacent: the report's CommitmentID should change OR a duplicate-suppression path should fire on the second run, depending on how adjustRecsForDuplicates interacts with newly-purchased RIs. Folded into #1326.

6. PR-acknowledged sibling occurrences.
TestQueryMajorEngineVersions_ErrorHandling and TestQueryRunningInstanceEngineVersions_ErrorHandling in cmd/multi_service_engine_versions_test.go have the same live-AWS + tautological pattern (test passes whether AWS creds are present or not, comment "important thing is that the function doesn't panic"). PR body explicitly leaves them for a follow-up. Filed as #1328.

Notes on the CI failures (UNSTABLE state):

• Lint Code, Run pre-commit hooks, Security Scanning, Integration Tests, CI Success are all pre-existing failures on main (errcheck across cmd/configure_azure.go, cmd/configure_gcp.go, cmd/cleanup-lambda/main.go, cmd/rekey/main.go, internal/api/handler.go; TestPostgresStore_UpsertRecommendations_* integration failures; govulncheck HIGH/CRITICAL). None are caused by this PR's three-file diff (cmd/multi_service.go, cmd/multi_service_coverage_test.go, cmd/multi_service_test.go) -- the Unit Tests job that exercises the actual changes is GREEN. Will not block merge on these.

Verification done locally:

• go build ./... clean against this branch.
• go vet ./cmd/ clean.
• gofmt -l cmd/ clean.
• go test -short ./cmd/ -run 'TestRunToolFromCSV|TestQueryMajorEngineVersions' -- 16/16 pass.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto origin/main (73166c2). Clean rebase (conflict in cmd/multi_service.go resolved by keeping main's helper functions + PR's return nil / error-returning signatures, and PR's test struct fields). Gates: go build, go vet, gocyclo -over 10, golangci-lint (0 new issues). Tests: cmd (739 passed).

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto current origin/main (81cd1f4) to pick up the flaky-test fix from #1399 (TestVerifyRetry_RebuildRateLimited is now deterministic). The two PR-owned commits are the only ones on this branch; validator_test.go now matches main's version. All gates passed: go build, go vet, scheduledauth -count=5 (exit 0), cmd package tests, gocyclo -over 10 (clean), golangci-lint full run (0 issues).

[cristim]

Merged to main (rebased onto current main, all CI green) closes #1154. CSV purchase-path tests now assert real behavior instead of always-passing stubs.