iframe-proxy

cristim · 2026-10-17T14:10:42Z

Summary

#2204 added purchaseSucceeded(result) (Success && Error == nil) and used it for the audit status and the purchase diagnostics. Some summary sites still read the raw result.Success. For a provider result of {Success: true, Error: non-nil}, those sites disagreed with the audit record for the same purchase. They now go through purchaseSucceeded:

cmd/interrupt.go printInterruptedSummary: the Ctrl-C summary now lists the result under "verify in AWS before re-running".
cmd/interrupt.go auditStopMessage: the message now reads "(errored: ...)" instead of "(commitment )".
cmd/multi_service_stats.go calculateServiceStats: the result now counts as failed, and the totals inherit that.
cmd/multi_service_stats.go purchaseFailureError (the exit-code check from fix(cmd): exit non-zero and count Savings Plan failures after a purchase run #2238/fix(cmd): repair red main after #2238 and drop the RI hint on SP-only runs #2243): the run now exits non-zero for such a result, using the same predicate as the printed summary.

The CSV Success column keeps the raw provider flag, as the issue allows. docs/cli/README.md now says to read the Error column, and that an errored row counts as failed in the summary and the exit code. cmd/multi_service_csv.go is unchanged; the open #2247 and #2250 touch it. CHANGELOG.md has a Fixed entry.

Evidence (local, macOS, `GOTOOLCHAIN=go1.26.9`, unit tests)

Fail before / pass after: cmd/purchase_succeeded_summaries_2205_test.go covers the interrupt summary, the audit-stop message, the stats and the exit-code check, plus ordinary success, Success=false and a separate Go error. With the two source files stashed, all four test functions fail. With the fix, they pass.
Mutation probes (reviewer): reverting calculateServiceStats, purchaseFailureError or interrupt.go:43 to .Success each fails a test.
Checks:
- make build: passes.
- go vet ./...: passes.
- go test -race ./cmd/...: passes.
- golangci-lint v2.10.1: 0 issues.
- scripts/test-git-secrets-allowlist.sh: 44/44.
Review: two passes by an independent non-author reviewer (sonnet, high) found no actionable findings at 441761f. The reviewer confirmed:
- no other .Success reads remain;
- the summary count and the exit code share one predicate;
- successful + failed still equals attempted;
- dry runs are unchanged.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Purchase results that include an error are now counted as failures in interrupted summaries, audit-stop messages, service statistics, and exit status—even when the provider reports success.
Documentation
- Clarified that the CSV Success column shows the provider’s raw result. Check the Error column for error details; results with errors count as failures in the run summary.

A result with Success=true and a non-nil Error is a failure. Route the Ctrl-C summary, the audit-stop message, the per-service counts and the non-zero exit check through purchaseSucceeded so they agree with the audit status. The CSV Success column stays the raw provider flag and is documented as such; the Error column carries the error. Closes #2205 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-17T14:11:20Z

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: c5bd1e6b-e763-4a07-997e-0d17a67e2eb8

📥 Commits

Reviewing files that changed from the base of the PR and between dcc8a1b and 441761f.

📒 Files selected for processing: 5

CHANGELOG.md
cmd/interrupt.go
cmd/multi_service_stats.go
cmd/purchase_succeeded_summaries_2205_test.go
docs/cli/README.md

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

Purchase results with Success=true and a non-nil Error now count as failures in interrupt reporting, service statistics, and failure status. The CSV Success column continues to show the provider’s raw flag.

Changes: Purchase result classification

Layer / File(s)	Summary
Classify purchase results consistently `cmd/interrupt.go`, `cmd/multi_service_stats.go`, `cmd/purchase_succeeded_summaries_2205_test.go`, `CHANGELOG.md`, `docs/cli/README.md`	Interrupt messages, service counts, and failure status use `purchaseSucceeded`. Tests cover success-with-error results. The changelog and CLI documentation describe this behavior and clarify that CSV `Success` retains the provider’s raw flag.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: iamjsd

Merge Risk: ⚪ Minimal · up to 44176

Purchase summaries and statistics now treat errored results as failures, while the CSV flag remains documented as the provider’s raw value. No actionable merge risk is identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. (2 skipped: …	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 identifies the main CLI fix: using purchaseSucceeded for interrupt summaries and run statistics. It accurately reflects the primary changes, although it does not mentio…
Linked Issues check		Issue `#2205` requires `purchaseSucceeded` for the interrupt summary, audit-stop message, and service statistics. The reviewed `cmd/interrupt.go` and `cmd/multi_service_stats.go` use that predicate. The…
Out of Scope Changes check		The changed tests verify issue `#2205` behavior. The documentation and changelog describe the changed result semantics. The exit-code change supports the same success predicate and failure reporting. No…

Full details: Title check

Explanation

The title clearly and concisely identifies the main CLI fix: using purchaseSucceeded for interrupt summaries and run statistics. It accurately reflects the primary changes, although it does not mention the related audit-stop and exit-status updates.

Full details: Linked Issues check

Explanation

Issue #2205 requires purchaseSucceeded for the interrupt summary, audit-stop message, and service statistics. The reviewed cmd/interrupt.go and cmd/multi_service_stats.go use that predicate. The tests cover Success=true with a non-nil Error, ordinary success, and failed results. The PR evidence states that the raw CSV Success value remains unchanged and that the documentation explains how to use the Error value. This is allowed by issue #2205. The exit-code check also uses the same predicate and keeps the summary and exit status consistent.

Full details: Out of Scope Changes check

Explanation

The changed tests verify issue #2205 behavior. The documentation and changelog describe the changed result semantics. The exit-code change supports the same success predicate and failure reporting. No unrelated change is demonstrated by the supplied evidence. The whole diff was unavailable, so this assessment relies on the supplied PR summary and the reviewed head file blobs.

Full details: Docstring Coverage

Explanation

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

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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 · 2026-10-17T14:19:03Z

Independent review verdict: CLEAN at `441761f`

Reviewer: fresh Claude agent (Sonnet 5.5, high effort), not the author, briefed cold. PR body and issue treated as claims. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Every success/failure decision in cmd/ audited:

purchaseSucceeded (cmd/multi_service.go:602) is Success && Error == nil.
It is now used everywhere a result is classified:
- audit status: multi_service.go:610
- diagnostics: :645 and :1054
- interrupt summary: interrupt.go:43 and :65
- exit code: multi_service_stats.go:23
- stats: :64
The only raw .Success uses left are not decisions:
- the CSV column (multi_service_csv.go:366), raw by design and documented;
- two writes, at multi_service_helpers.go:288 (dry run) and :305 (clears Success when the provider returns an error).
All the decision points agree.

How each result shape is now counted:

A real success counts as success.
Success=true with an error counts as failed. This is the fix.
An error counts as failed.
An unknown outcome carries a non-nil error, so it counts as failed and is listed under "verify in AWS before re-running". That is the safe direction against duplicate buys.
Dry-run handling is unchanged.

Interrupt path: shutdownRequested is atomic, and the summary reads the finished results on the same goroutine. -race is clean.

Local verification (unit and mock-based):

make build ok, go vet ./... clean, go test -race ./cmd/... -count=1 ok, and golangci-lint v2.10.1 reports 0 issues.
Fail before: the new test file run against origin/main's interrupt.go and stats code gives 4 failures. All pass at this SHA.
Mutation probes (each reverted):
- interrupt.go restored to main: 2 fail.
- stats restored to main: 2 fail.
- purchaseSucceeded ignoring the error: all 4 new tests fail.
Real-account gap: an offline run of the binary stops at the STS account lookup, before the summary or stats are printed, so those paths are covered only by the tests. No live provider was tested returning Success=true with an error.

Non-blocking notes (both predate this PR):

Dry-run results show as "Successful" in the per-service stats.
The CSV Success column can show true next to an error. This is documented.

Gate:

CI is green at this SHA: 10 pass, 2 skipped.
No main drift since the base.
Labels are mirrored from fix(cli): use purchaseSucceeded in interrupt summary, stats and CSV success column #2205.
CodeRabbit posted only a summary and no actionables. Exact-revision verification plus this review cover the SHA.

cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-quarter Within the quarter impact/few Limited audience effort/s Hours type/bug Defect triaged Item has been triaged labels Oct 17, 2026

cristim merged commit 7e885a2 into main Oct 17, 2026
12 checks passed

cristim deleted the fix/purchase-succeeded-summaries branch October 17, 2026 14:19

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

fix(cli): use purchaseSucceeded in the interrupt summary and run stats - #2253

fix(cli): use purchaseSucceeded in the interrupt summary and run stats#2253
cristim merged 1 commit into
mainfrom
fix/purchase-succeeded-summaries

cristim commented Oct 17, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 17, 2026 •

edited

Loading

Uh oh!

cristim commented Oct 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

cristim commented Oct 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Evidence (local, macOS, GOTOOLCHAIN=go1.26.9, unit tests)

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Oct 17, 2026

Independent review verdict: CLEAN at 441761f

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented Oct 17, 2026 •

edited by coderabbitai Bot

Loading

Evidence (local, macOS, `GOTOOLCHAIN=go1.26.9`, unit tests)

coderabbitai Bot commented Oct 17, 2026 •

edited

Loading

Independent review verdict: CLEAN at `441761f`