Skip to content

fix(cmd): reject Savings Plan hourly commitments that round to 0.00 - #2209

Open
IAmJSD wants to merge 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-2163
Open

IAmJSD wants to merge 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-2163

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Root cause

validateSavingsPlanPreconditions (cmd/savings_plan_purchase.go) only rejected HourlyCommitment <= 0. The library formats the commitment for CreateSavingsPlan with fmt.Sprintf("%.2f", ...) (cloud-commitments-go/providers/aws/services/savingsplans/client.go:235 at the pinned version), so a positive commitment below 0.005 (e.g. 0.004) passed validation, was reported as a success by the dry run, and was sent to AWS as "0.00".

Fix

Add a spCommitmentFormat = "%.2f" constant that mirrors the library's formatting and reject any commitment that formats as "0.00", with an error that says so and names the 0.005 minimum. validatePurchasePreconditions runs on both the main path and the --input-csv path, dry run and real run, so both now reject such rows before any API call.

Closes #2163

Verification

All evidence is unit-level (no AWS calls). Run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid, which otherwise breaks some unrelated tests.

New test TestValidatePurchasePreconditions_RejectsCommitmentThatRoundsToZero: 0.001, 0.004, 0.00499 must be rejected (error mentions 0.00); 0.005, 0.01, 1.5 must still be accepted.

  • Fail before (test added, fix not yet applied):
    $ go test ./cmd -run TestValidatePurchasePreconditions -count=1
    --- FAIL: TestValidatePurchasePreconditions_RejectsCommitmentThatRoundsToZero
        Error: An error is expected but got nil.
        Messages: commitment 0.001 rounds to 0.00 and must be rejected
    
  • Pass after:
    --- PASS: TestValidatePurchasePreconditions
    --- PASS: TestValidatePurchasePreconditions_RejectsCommitmentThatRoundsToZero
    
  • go build -o cudly ./cmd: OK
  • go test -short -race -count=1 ./...: ok github.com/LeanerCloud/cloud-commitments-cli/cmd
  • go vet ./...: clean
  • golangci-lint run (v2.10.1): 0 issues
  • gocyclo -over 10 cmd/savings_plan_purchase.go: silent
  • bash scripts/gosec-hook.sh cmd/savings_plan_purchase.go: 0 issues; gofmt -l cmd: clean; make tidy-check: clean

Mutation note: if the check is removed, or the format constant drifts to a finer precision (e.g. %.3f), the new test fails on 0.001.

Real-cloud gap: not exercised against a real CreateSavingsPlan call. The rejection depends only on the pinned library's %.2f formatting, which was checked in the module source. If the library's formatting changes, spCommitmentFormat must change with it.

Out of scope

  • A non-finite commitment (NaN) still passes <= 0 and the new check, because it formats as "NaN". CSV parsing already validates numeric bounds (fix(cli): validate CSV headers, encoding and numeric bounds #2145), so this was left alone here.
  • No library change. Sending more precision, or rejecting at the library boundary, would belong in cloud-commitments-go.

Maintainer: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/xs, type/bug.

🤖 Generated with Claude Code

The library sends HourlyCommitment to CreateSavingsPlan formatted with
"%.2f", so a positive commitment below 0.005 passed the `> 0` precondition
and was submitted as "0.00". The precondition now rejects any commitment
that formats as 0.00, on both the dry-run and real purchase paths.

Closes LeanerCloud#2163

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

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

Or wait 57 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: dc8c91cd-bcf2-4252-8bd7-75c746dac3bf
📥 Commits

Reviewing files that changed from the base of the PR and between 817ab4b and 69a97e4.

📒 Files selected for processing: 2
  • cmd/savings_plan_purchase.go
  • cmd/savings_plan_purchase_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 cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/xs Trivial / one-liner type/bug Defect triaged Item has been triaged labels Oct 16, 2026
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Merge-gate review verdict: CLEAN at 69a97e4

Independent adversarial review by a non-author agent (Sonnet high, cold brief, full diff 817ab4bc...69a97e45): no blocking findings.

Checked against the source of truth:

  • Rounding parity. The provider sends fmt.Sprintf("%.2f", spDetails.HourlyCommitment) (cloud-commitments-go providers/aws/services/savingsplans/client.go:235, the version pinned in go.mod). The new check at cmd/savings_plan_purchase.go:151 uses the same verb on the same float64, so the two cannot disagree. 0.005 formats as 0.01 and is accepted.
  • Placement and entry points. The check is in validateSavingsPlanPreconditions, which validatePurchasePreconditions runs before the single PurchaseCommitment call site (multi_service_helpers.go:302). Both callers, the flag/recommendation path (multi_service.go:636) and the CSV path (:1046), validate first (:613, :1039). Dry-run validates too. Nothing mutates HourlyCommitment after validation.
  • Conflicts. The PR does not overlap the commits that landed on main since its base.

Non-blocking notes:

Local verification on macOS at 69a97e4 (fixture-based unit tests, no AWS calls):

  • go test ./... -count=1 (go1.26.9, AWS config and credentials set to /dev/null): ok cmd 81.4s.
  • go build ./cmd OK; go vet ./... clean.
  • Deletion probe: neutralising the new check fails TestValidatePurchasePreconditions_RejectsCommitmentThatRoundsToZero. This is equivalent to fail-before, since the base has no check.
  • Mutation probe: changing %.2f to %.1f fails the same test. Both probes were reverted.
  • Not run: the built binary against a 0.004 input, because the command needs AWS config. Real-account coverage gap noted.

Merge blockers (not code): the fork CI runs 38838595062 and 38838595305 need maintainer approval, and the PR is a draft for the author to mark ready. CodeRabbit will be run, or waived on quota, once the PR is ready.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cmd): Savings Plan hourly commitment below 0.005 passes validation and is sent as 0.00

2 participants