fix(cli): fail closed when the duplicate-purchase check errors

[cristim]

Summary

A failed duplicate-purchase check (throttling, a DescribeReservedDBInstances
AccessDenied, a transient 5xx) was logged as a warning and the run proceeded
with the pre-dedup counts, so an error on this check caused a real-purchase
run to buy reserved capacity the account already owns. This was the only
guard between a re-run and a double purchase, and it failed open.

Root cause

Three call sites shared the fail-open shape:

• checkDuplicates (cmd/multi_service_helpers.go), used by the main
  non-CSV pipeline (fetchAndFilterRegionRecs) and the legacy
  processRegionRecommendations.
• The --input-csv path in runToolFromCSV (cmd/multi_service.go), which
  called adjustRecsForDuplicates and reset adjustedRecs = recs on error.

All three logged a warning on error and continued with the original,
un-deduplicated recommendation counts, regardless of whether the run was a
dry run or a real purchase.

Fix

All three now fail closed on a real purchase run (!isDryRun): a failed
duplicate check drops that (service, region)'s recommendations entirely
rather than falling back to the un-deduplicated counts, mirroring the
existing "refuse to spend rather than purchase uncapped" stance already
taken for --max-instances in processRegionRecommendations. A dry run
keeps the previous behavior (a loud warning, continuing with the
un-deduplicated counts) since nothing is bought and reporting fidelity wins.

checkDuplicatesForCSVRegion was extracted out of runToolFromCSV to keep
its cyclomatic complexity at 10 (the project's gocyclo gate) after adding the
new branch.

Regression test

• TestCheckDuplicates_ErrorOnPurchaseRun_DropsRecsRatherThanFallingBack and
  TestCheckDuplicates_ErrorOnDryRun_ContinuesUnadjusted exercise
  checkDuplicates directly with a mocked GetExistingCommitments error.
• TestRunToolFromCSV_DuplicateCheckFailureRefusesToPurchase runs
  runToolFromCSV with ActualPurchase=true and invalid AWS credentials (so
  GetExistingCommitments fails the same way a real AWS error would) and
  asserts no purchase report is produced (the region is skipped before
  processPurchaseLoop runs).

Proved the new CSV-level test fails on the pre-fix code: built a detached
worktree of origin/main (parent commit de556ad1), applied only the new
test, and ran it. Pre-fix it proceeds into the purchase loop with the
un-deduplicated recommendation (1 rec, 2 instances) and writes a purchase
report row; post-fix the region is skipped and no report file is written.

Verification

• go build ./cmd - clean
• go vet ./cmd/... - clean
• golangci-lint v2.10.1 (the exact CI pin, not the newer local default) run
  --timeout=10m from the repo root - 0 issues
• gocyclo -over 10 -ignore "_test.go" cmd/ - clean
• go mod tidy -diff - empty
• go test -race -short ./... - all green (493s)

Closes #1941

Summary by CodeRabbit

• Bug Fixes
  • Purchase runs now skip the affected service and region when the existing-commitments check fails, preventing purchases without a successful duplicate check. Dry runs continue with the original recommendations and display a warning.
• Documentation
  • Clarified how dry runs and purchase runs handle failed commitment lookups and Cost Explorer coverage checks, including the purchase safety checklist.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: c27dad71-ee80-4d13-bb6e-d8a767ceb8e8

📥 Commits

Reviewing files that changed from the base of the PR and between caa83c6 and afcc2af.

📒 Files selected for processing (6)

• README.md
• cmd/multi_service.go
• cmd/multi_service_helpers.go
• cmd/multi_service_helpers_test.go
• cmd/multi_service_test.go
• docs/cli/purchase-safety.md

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

📝 Walkthrough

Walkthrough

A failed existing-commitments check now prevents purchases for the affected service and region. Dry runs continue with a warning. CSV processing follows the same mode-specific behavior, with tests and documentation updated.

Changes

Duplicate-check failure handling

Layer / File(s)	Summary
Mode-aware duplicate checking cmd/multi_service_helpers.go, cmd/multi_service_helpers_test.go, README.md, docs/cli/purchase-safety.md	checkDuplicates now receives the run mode. When the check fails, purchase runs drop the recommendations and record a distinct drop reason. Dry runs retain the original recommendations and warn. Tests and documentation describe these outcomes.
CSV region failure handling cmd/multi_service.go, cmd/multi_service_test.go	CSV processing skips a region when duplicate checking fails during a purchase run. Dry runs warn and continue with the original recommendations. A test verifies that the failed purchase check does not create a report.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant runToolFromCSV
  participant checkDuplicatesForCSVRegion
  participant checkDuplicates
  runToolFromCSV->>checkDuplicatesForCSVRegion: check recommendations for region
  checkDuplicatesForCSVRegion->>checkDuplicates: check existing commitments
  checkDuplicates-->>checkDuplicatesForCSVRegion: return no recommendations on purchase-run failure
  checkDuplicatesForCSVRegion-->>runToolFromCSV: skip region

Loading

Merge Risk: ⚪ Minimal · up to afcc2

No confirmed merge-blocking issue remains. The CSV regression test’s ability to catch the purchase-path regression is still unverified.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The duplicate-check implementation, tests, and related documentation are in scope for [#1941]. cmd/multi_service.go also changes only the punctuation of the coverageFetchFailure warning, while its…	Remove the unrelated coverageFetchFailure warning-string change, or provide a direct coding connection to [#1941].
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: purchase runs now fail closed when the duplicate-purchase check errors.
Linked Issues check	✅ Passed	Issue [#1941] requires fail-closed duplicate checks for purchase runs and warning-and-continue behavior for dry runs. The summary shows this behavior in the main pipeline, legacy recommendation path, …

Full details: Out of Scope Changes check

Explanation

The duplicate-check implementation, tests, and related documentation are in scope for [#1941]. cmd/multi_service.go also changes only the punctuation of the coverageFetchFailure warning, while its behavior remains unchanged. This coverage-warning change has no demonstrated connection to the duplicate-check objective.

Full details: Docstring Coverage

Explanation

Docstring coverage is 78.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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

[cristim]

Independent adversarial review (two rounds) + local verification: MERGE at afcc2af. A failed existing-commitments check now fails closed on purchase runs on both paths (--services: checkDuplicates drops the region's recs and records duplicate-check-failed; --input-csv: the region is skipped before the purchase loop); dry runs warn and continue. Docs rewritten to match; #2100's behavior intact after rebase. Regression tests fail on the pre-fix parent and pass on head under -race; 7/7 checks.