fix(cmd): fail the run when recommendation fetching fails

[cristim]

Closes #2190

What

A recommendation fetch failure (expired SSO, denied Cost Explorer call, region discovery failure) used to be logged and turned into "No recommendations found" / "Nothing to purchase" with exit 0. It now aborts the run with exit 1 and the cause.

• fetchRecommendationsForRegion returns ([]Recommendation, error); provider errors are wrapped with errRecommendationFetch.
• fetchAllRecs returns the region-discovery and per-region fetch errors instead of continue/logging them.
• Unchanged: an empty answer (exit 0), IncompleteRecommendationsError and Azure partial-subscription results (warn and keep the usable rows), and interrupt handling (ctx cancel is not reported as a failure).
• Purchase-phase exit codes are fix(cmd): purchase run exits 0 and the summary ignores Savings Plan failures #2157 and are not touched.

Tests

• Replaced TestFetchAllRecs_ActiveContextProviderErrorStaysRecoverable (it pinned the old behaviour: a throttled region only lost that region) with TestFetchAllRecs_ProviderErrorFailsTheRun; added TestFetchAllRecs_EmptyResultIsNotAnError.
• The end-to-end TestRecommendationCompletenessCommand harness used to expect exit 0 for api-error and failed-type; it now expects exit 1, the cause in the output and no CSV.
• go vet ./cmd and go test ./cmd pass (72s). make lint could not run locally: golangci-lint panics on this toolchain (environmental); the pre-commit gocyclo, vet and gofmt hooks pass.

Live dry run (read-only; no purchase flags)

Fake credentials, so Cost Explorer rejects the call. cudly --services ec2 --regions us-east-1 --term 1 --payment no-upfront:

before (main 8ddb891):

ℹ️  No recommendations passed filters. Nothing to purchase.
exit=0

after:

Cannot complete the recommendation fetch: recommendation fetch failed: EC2 in us-east-1: ... UnrecognizedClientException: The security token included in the request is invalid.
exit=1

Overlap

cli#2186 touches cmd/audit_recurring_cost_test.go, go.mod/go.sum; cli#2145 touches cmd/multi_service_csv*.go. No shared files.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Recommendation retrieval failures now stop the run with a clear error instead of silently continuing with incomplete results.
  • Region-discovery failures now stop processing rather than allowing later services to continue.
  • Interrupted requests are handled separately from provider failures. Incomplete AWS or Azure results remain available with a warning.

[coderabbitai]

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: 570b9c3d-9c5d-4c68-b233-99403230353f

📥 Commits

Reviewing files that changed from the base of the PR and between de71edc and c969c5f.

📒 Files selected for processing: 7

• cmd/cancellation_prepurchase_test.go
• cmd/multi_service.go
• cmd/multi_service_helpers.go
• cmd/recommendation_completeness_test.go
• cmd/savings_plan_purchase_test.go
• cmd/savings_plan_region_test.go
• cmd/sp_account_scope_test.go

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

📝 Walkthrough

Recommendation fetch failures and region-discovery failures now abort the recommendation pipeline with errors. The command reports fetch failures separately from exclusion errors. Incomplete usable results remain available, while successful empty results remain error-free.

Changes: Recommendation fetch handling

Layer / File(s)	Summary
Fetch result contract cmd/multi_service_helpers.go	fetchRecommendationsForRegion now returns provider errors and context cancellation. It returns usable incomplete AWS and Azure results without an error. The legacy per-region processor logs fetch errors.
Pipeline error propagation cmd/multi_service_helpers.go, cmd/multi_service.go	fetchAllRecs now returns fetch and region-discovery errors. The multi-service command handles recommendation-fetch errors separately from exclusion errors.
Fetch error tests cmd/cancellation_prepurchase_test.go, cmd/recommendation_completeness_test.go, cmd/savings_plan_purchase_test.go, cmd/savings_plan_region_test.go, cmd/sp_account_scope_test.go	Tests now check fetch errors, cancellation, successful empty results, and command failure output. Existing recommendation scenarios also assert successful fetches.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Provider as AWS or Azure provider
  participant Fetch as fetchRecommendationsForRegion
  participant Pipeline as fetchAllRecs
  participant Command as runToolMultiService
  Provider->>Fetch: Return provider error
  Fetch->>Pipeline: Return wrapped errRecommendationFetch
  Pipeline->>Command: Propagate fetch error
  Command->>Command: Exit with fetch-specific message

Loading

Merge Risk: ⚪ Minimal · up to c969c

The reviewed error paths preserve the intended distinction between interruptions and fetch failures. No identified issue needs resolution before merge.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: recommendation-fetch failures now fail the run instead of being logged and ignored.
Linked Issues check		The changes satisfy #2190. fetchRecommendationsForRegion now returns provider errors wrapped with errRecommendationFetch. fetchAllRecs propagates region-discovery and per-region fetch errors. `r…
Out of Scope Changes check		The changes stay within #2190. Production changes update recommendation-fetch error propagation and command handling. Test changes update affected callers and verify failure, empty-result, cancellatio…

• 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

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review (claude-opus-5-5) of c969c5f: APPROVE

Error paths checked

• fetchRecommendationsForRegion (cmd/multi_service_helpers.go:599-661) handles each outcome as follows:
  • Library IncompleteRecommendationsError and Azure partial failures still warn and return the recs.
  • Context cancellation returns ctx.Err(), which fetchAllRecs treats as an interruption, not a failure (:955-957).
  • Any other error is wrapped in errRecommendationFetch.
  • An empty slice with a nil error is still "No recommendations found".
• In the pinned library (recommendations/client.go:428-454), a partial failure across services comes back as IncompleteRecommendationsError, and only an all-services failure is a plain error. So "incomplete but usable" still continues, and a total failure now fails the run.
• Region discovery failure (fetchAllRecs, :943-948) is now wrapped as a fetch error instead of log.Printf + continue.
• runToolMultiService -> exitOnFetchError (cmd/multi_service.go:192, :1055-1060) exits with "Cannot complete the recommendation fetch". The extended-support exclusion error still reaches exitOnExclusionError unchanged.
• processRegionRecommendations still logs and returns. Its only caller, processService, has no non-test caller, so the comment calling it test-only is accurate.

Verification

• Fresh clone at the head: gofmt and go vet ./... are clean, and go test -count=1 ./... passes.

• Head merged with current main de71edc: git merge-tree reports no conflicts, and ./cmd passes 1368 tests.

• Mutation (fixture): making the generic-error branch return nil, nil fails TestFetchAllRecs_ProviderErrorFailsTheRun.

• Real runs, comparing main de71edc with the PR head merged into main, using a fake static key (AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE=/dev/null):

  Run	main	PR
  --services savingsplans --include-sp-types Compute	rc=0, "Failed to fetch recommendations ... No recommendations found ... Nothing to purchase"	rc=1, Cannot complete the recommendation fetch: recommendation fetch failed: Compute Savings Plans in us-east-1: all 1 AWS recommendation services failed ...
  --services ec2 --regions us-east-1	rc=0, "No recommendations found"	rc=1, ... EC2 in us-east-1: failed to get RI recommendations ...
  --services ec2 (region discovery, no --regions)	not run	rc=1, ... determining regions for EC2: failed to discover regions ...; no CSV is written

• Real credentials, genuinely empty result: --include-sp-types Compute at linked scope on gives "No recommendations found ... Nothing to purchase" with rc=0.

Non-blocking findings

1. cmd/multi_service.go:1053-1055: the doc comment "exitOnExclusionError aborts the run..." now sits above exitOnFetchError, and exitOnExclusionError has lost its comment. Swap or add one line.
2. Behaviour change worth stating in the release notes: on a multi-region RI run, one region that fails with a non-partial error (for example throttling after retries) now aborts the whole run, where it used to lose only that region. This is what fix(cmd): recommendation fetch failures are reported as "No recommendations" and exit 0 #2190 asks for (the replaced test pinned the old behaviour), and it is the safe direction on a purchase path.
3. processService and processRegionRecommendations are reachable only from tests. They could be deleted in a follow-up rather than kept with a divergent error policy.

CI on c969c5f: all checks pass (Unit Tests, Integration Tests, CI Success among them); gosec and Trivy are skipped.