fix(cli): classify errored provider purchase results as failures

[IAmJSD]

Root cause

The CLI decided whether a purchase succeeded using only PurchaseResult.Success. executePurchase keeps the provider's result as-is when the separate Go error is nil, so a provider result of {Success: true, Error: non-nil} was audited as success and printed with ✅, even though the provider had reported an error.

Fix

• Add purchaseSucceeded(result), which is Success && Error == nil.
• Use it in purchaseAuditStatus, which both paths share, and in the success/failure diagnostics in purchaseSingleRec (the recommendation path) and logPurchaseOutcome (the CSV path).
• The returned PurchaseResult is not mutated, so provider data stays the same (per the issue).
• Add a newServiceClient package-var seam (defaults to createServiceClient) so the test can drive the real purchaseSingleRec path with a fake provider. Nothing changes at runtime.

Closes #1900

Verification

New test file cmd/purchase_result_classification_test.go covers four cases on both purchaseSingleRec and runOnePurchase + logPurchaseOutcome: ordinary success, Success=true with a provider error, Success=false, and a separate Go error. Each case checks the audit status and the ✅/❌ diagnostic.

Fail-before: the seam was added, the fix was not yet applied, and the commands were run on macOS.

AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./cmd -count=1 -run 'ClassifiesErroredProviderResult' -v
--- FAIL: TestPurchaseSingleRec_ClassifiesErroredProviderResultAsFailure/success_flag_with_provider_error
    expected: "error"  actual: "success";  output: "✅ ri-err"
--- FAIL: TestRunOnePurchase_ClassifiesErroredProviderResultAsFailure/success_flag_with_provider_error
    expected: "error"  actual: "success";  output: "✅ Success: ri-err"
(the other three cases PASS: existing behavior is preserved)

Pass-after, with the fix applied:

go build -o /dev/null ./cmd                      # OK
go vet ./...                                     # OK
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./... -count=1   # ok .../cmd
golangci-lint run --timeout=5m ./...             # v2.10.1: 0 issues

AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null is set only because the dev machine's ~/.aws/config default profile is invalid, which breaks some existing tests. It is unrelated to this change.

Coverage gap: this was verified only with a mocked provider (MockServiceClient). No real purchase was made and the CLI was never run with --yes. None of the pinned cloud-commitments-go AWS providers is known to return Success=true with an error today, so this is a defensive fix.

Out of scope (not changed here)

Other places still read only result.Success. They would still treat such a result as successful:

• cmd/multi_service_stats.go (calculateServiceStats successful/failed counts)
• cmd/interrupt.go (printInterruptedSummary skips it from the "verify in AWS" list; auditStopMessage labels it with a commitment ID)
• cmd/multi_service_csv.go (the CSV Success column prints the raw flag)

These could switch to purchaseSucceeded in a follow-up if maintainers want summaries to match the audit status.

Maintainer: please mirror the issue's triage labels on this PR (the author lacks label permissions): triaged, priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 589cdb28-8c21-42fa-a4ed-cc06b673985d

📥 Commits

Reviewing files that changed from the base of the PR and between 817ab4b and 55249c3.

📒 Files selected for processing: 2

• cmd/multi_service.go
• cmd/purchase_result_classification_test.go

• 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 verdict: CLEAN for #1900 scope, head 55249c34fbb55eddbfb85b85b04d9f10eb23d83a

An independent adversarial reviewer (did not author the change) verified:

• purchaseSucceeded is nil-safe and doesn't mutate provider data.
• It is used on all three status/diagnostic sites: purchaseSingleRec, runOnePurchase and logPurchaseOutcome.
• executePurchase can't misclassify a real success.
• The newServiceClient seam is needed to drive the real purchaseSingleRec. It is safe today because no cmd/ test uses t.Parallel.
• Merges cleanly with main and with fix(cli): only print "CSV report written to" after a complete CSV write #2203.

Local verification on macOS (mock evidence: MockServiceClient, no real purchase, no --yes):

• make build, go test ./... -count=1 and go vet ./... all pass.
• Mutation: changing purchaseSucceeded to return result.Success makes both *_ClassifiesErroredProviderResultAsFailure/success_flag_with_provider_error subtests FAIL.
• With the fix restored, 10 tests pass.

The reviewer raised one more finding. The four sites the PR body lists as out of scope still read the raw .Success:

• the interrupt.go Ctrl-C summary and auditStopMessage;
• the multi_service_stats.go counts;
• the CSV column.

For a Success=true, Error!=nil result, these now disagree with the audit record. The #1900 acceptance criteria cover only audit status and diagnostics, and no provider returns this shape today, so this is tracked as non-blocking follow-up #2205.

Labels mirrored from #1900.

Not yet mergeable:

• (1) Fork CI runs 38822167183 and 38822167189 are awaiting maintainer approval.
• (2) The PR is still a draft. @IAmJSD, mark it ready when you're done.
• (3) CodeRabbit hasn't reviewed it because it's a draft.

[cristim]

@coderabbitai full review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

[cristim]

Merge gate met at 55249c34fbb55eddbfb85b85b04d9f10eb23d83a

• Independent adversarial review: CLEAN for the fix(cli): classify errored provider purchase results as failures #1900 scope at this SHA (see the verdict above). The remaining raw .Success sites are tracked in fix(cli): use purchaseSucceeded in interrupt summary, stats and CSV success column #2205.
• CI: green at this SHA after maintainer approval of the fork runs: pre-commit 38822167183 and CI - Build & Test 38822167189.
• CodeRabbit: the full review was rate-limited. CR waived: quota, adversarial review + local verification + green CI.
• Local verification on macOS: recorded in the verdict. It is mock evidence (MockServiceClient, no real purchase, no --yes), with fail-before/pass-after via mutation.
• Combination with fix(cli): only print "CSV report written to" after a complete CSV write #2203: fix(cli): only print "CSV report written to" after a complete CSV write #2203 merged just before this and also touches cmd/multi_service.go, so I re-verified this PR on top of current main 0e337474 (local macOS build):
  • go build ./cmd, go vet ./... and go test ./... -count=1 all pass.
  • The tests from both PRs (*_ClassifiesErroredProviderResultAsFailure, TestWriteMultiServiceCSV_FlushErrorIsReturned, TestWriteReportAndSummary_NoResultsDoesNotClaimCSVWritten, TestWriteCSVReport_FailureDoesNotClaimCSVWritten) pass.
• Labels: mirrored from fix(cli): classify errored provider purchase results as failures #1900.
• Mergeability: clean.

Squash-merging with --match-head-commit 55249c34.