Skip to content

fix(aws): floor Savings Plan commitments to API precision - #403

Merged
cristim merged 1 commit into
mainfrom
fix/savings-plan-commitment-precision
Oct 14, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/savings-plan-commitment-precision

Conversation

@cristim

@cristim cristim commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Savings Plan purchases currently serialize hourly commitments to two decimal places, which can exceed the approved amount or send a valid small commitment as zero. This floors shortest-decimal commitment amounts to the AWS API five-place precision and rejects nonfinite or out-of-range inputs before lookup or purchase.

The client and ladder share one provider-internal rule. SDK request, returned recommendation, and offering preview agree; copied details preserve caller authorization data. Explicit idempotency tokens and authoritative post-purchase cost lookup remain unchanged.

Refs #354, #53. This provider leaf leaves #354 open for platform storage precision and old-tranche CommitmentRate requirements. No consumer pin updates or MCP whole-cent guard changes.

Reviewed head: d7dfc8428f1ccaa76e6b6c06a6dba169d72b63f9, tree 3bc791e21e42917342d3e067c03ed682c4544172, parent ce96fafce117c3b132013655b5fc21d58d44f404 after #352/#353. Rebase preserved the precision patch and all five owned source/test blobs; original bdec9f94 is retained in a backup ref.

  • Three clean independent refresh-plan passes, two clean full-source passes, and a fresh cold committed review with no actionable findings.
  • Fresh author Go 1.26.9 race tests, all five module builds, standalone AWS build, six public SDK consumer cases, and explicit normal pinned precommit hooks passed.
  • Fresh independent offline macOS race tests passed AWS Savings Plans, AWS ladder, and shared ladder; standalone AWS build and six public-client cases passed. New allocation/rate contracts were checked for exactly one conversion followed by precision flooring.
  • Prior fail-before and scratch five-to-two-digit mutation proof remains relevant because owned blobs and old/new parent purchase files are identical.

The independent broader build passed pkg and AWS, then Azure compilation ran out of disk space; full-build success is from the author run, not that independent attempt. Runtime evidence uses realistic fixtures and a fake SDK boundary. No live AWS acceptance or purchase was performed.

Required CI passed on the reviewed refreshed head. CodeRabbit recovered and completed an included review covering that exact head with no actionable comments and no inline findings; no quota waiver is used for the final head. Normal squash merge completed as 29adce1c3883bfb71a926451c323eafea601e87f; the merged tree exactly matches the reviewed tree. Both post-merge workflows completed successfully.

@cristim cristim added severity/medium Moderate harm triaged Item has been triaged urgency/this-sprint Within the current sprint priority/p1 Next up; this sprint impact/few Limited audience effort/s Hours type/bug Defect labels Oct 14, 2026
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 11076e0b-9e99-483b-8127-e7ef80811233
📥 Commits

Reviewing files that changed from the base of the PR and between ce96faf and d7dfc84.

📒 Files selected for processing: 5
  • providers/aws/internal/spcommitment/commitment.go
  • providers/aws/ladder/precision_test.go
  • providers/aws/ladder/purchase.go
  • providers/aws/services/savingsplans/client.go
  • providers/aws/services/savingsplans/precision_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

AWS Savings Plan commitment amounts now pass through a shared normalizer. Ladder purchases and Savings Plans service operations use normalized amounts, with validation for missing details and values outside the supported range.

Changes: AWS Savings Plan commitment precision

Layer / File(s) Summary
Normalize commitments and validate ladder purchases
providers/aws/internal/spcommitment/commitment.go, providers/aws/ladder/purchase.go, providers/aws/ladder/precision_test.go
Normalize checks commitment details and limits, truncates valid amounts to five decimal places, and copies the normalized value into the recommendation. Ladder purchase validation uses it. Tests cover valid precision, invalid amounts, and missing details.
Normalize service purchases and offering lookups
providers/aws/services/savingsplans/client.go, providers/aws/services/savingsplans/precision_test.go
Purchase and offering operations normalize recommendations before proceeding. Purchase requests use the normalized commitment. Tests cover precision, invalid inputs, and offering validation.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to d7dfc

No actionable issue is established in the AWS Savings Plan precision change. It is ready to merge after the stated CI and unchanged-head checks.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Linked Issues check Warning The AWS provider portion of #354 is implemented. spcommitment.Normalize rejects nonfinite and out-of-range values, truncates to five decimal places, rejects values below 0.001, and runs before off… Complete the #354 platform requirements for storage precision and the old-tranche CommitmentRate gate. Add the required per-provider precision documentation. Then verify the linked issue requirements together with the AWS implementation.
Docstring Coverage Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Out of Scope Changes check Passed The changed files implement AWS Savings Plan commitment normalization at the client and ladder purchase boundaries. The added tests verify precision, bounds, copied details, idempotency, preview behav…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: flooring AWS Savings Plan commitments to the API's required precision.
Full details: Linked Issues check

Explanation

The AWS provider portion of #354 is implemented. spcommitment.Normalize rejects nonfinite and out-of-range values, truncates to five decimal places, rejects values below 0.001, and runs before offering lookup or purchase. The ladder and client tests cover both Savings Plan layers, request/preview/result agreement, copied details, and no SDK calls for invalid input. However, #354 also requires platform storage precision and refusal of old tranches without CommitmentRate. The current diff changes only AWS provider code and tests, and the PR description states that these requirements remain open.

  • 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

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Independent committed verdict: no actionable findings at bdec9f9455e46492011b3adc04a6c402db1872b2, tree 6f4b6e84bc0d06038b52f59d4e749798c381329e. Separate non-author reviews also completed two clean full-source passes.

The cold verifier checked the current AWS CreateSavingsPlan contract (0.001 through 1 million; at most five fractional digits), raw range checks before truncation, adjacent float boundaries, cent preservation, repeated normalization, typed nil rejection before SDK calls, copied authorization details, both ladder Savings Plan layers, preview/request/returned amount agreement, unchanged ClientToken, and authoritative post-purchase Cost. Source traced through internal/spcommitment/commitment.go:20-46, ladder/purchase.go:139, and services/savingsplans/client.go:222-267,652-681.

Fresh independent macOS Go 1.26.9 verification at that SHA:

  • Offline make build passed all five modules.
  • go test -race -count=1 -short ./ladder ./services/savingsplans passed.
  • A public NewClient/SetSavingsPlansAPI consumer probe passed .29, 10, .001, .00499, .005, and 1.234567, asserting wire/preview/result agreement, token/cost preservation, and caller immutability.
  • In a scratch copy, changing precision from five digits to two made both regression suites fail; restoring five made both pass.

Evidence uses realistic fixtures and a fake SDK boundary. No live AWS acceptance or purchase, consumer pin update, or platform storage/gate verification occurred. The platform portion remains open in #354; the separate offering-ID scope bypass is already tracked in #53.

CodeRabbit posted Review limit reached for this head and submitted no review. Its green status is not review evidence. No paid review is requested; the project independent-review path applies once required CI is green. Merge remains blocked until fresh CI and unchanged-head checks cover this exact SHA.

@cristim
cristim force-pushed the fix/savings-plan-commitment-precision branch from bdec9f9 to d7dfc84 Compare October 14, 2026 21:07
@cristim

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Refreshed exact-head verdict: no actionable findings at d7dfc8428f1ccaa76e6b6c06a6dba169d72b63f9, tree 3bc791e21e42917342d3e067c03ed682c4544172, parent ce96fafce117c3b132013655b5fc21d58d44f404.

Rebased after #352/#353 under an explicit old-head lease. The single precision patch and all five owned blobs are unchanged; original bdec9f9 remains backed up. Two independent full-source passes and a fresh cold committed review found no actionable findings.

The verifier checked AWS range/five-digit API and pinned SDK contracts, typed nil/nonfinite/raw-boundary refusal before SDK calls, decimal flooring and repeat stability, cents preservation, copied caller details, both AWS Savings Plan layers, request/preview/returned recommendation agreement, unchanged supplied token, and authoritative Cost. New-base compatibility was traced through pkg/ladder/allocate.go:78-82, pkg/ladder/allocate_units.go:113-114, and providers/aws/recommendations/sp_commitment_rate.go:41-55: allocation converts to commitment dollars once; this boundary only floors HourlyCommitment.

Fresh independent macOS arm64 Go 1.26.9 offline proof: go test -race -count=1 -short ./providers/aws/services/savingsplans ./providers/aws/ladder ./pkg/ladder, standalone AWS GOWORK=off go build ./..., and inspected public NewClient/SetSavingsPlansAPI six-case probe all exited zero. Prior mutation proof was inspected and reused only after confirming identical blobs. The independent broader build passed pkg/AWS before Azure disk exhaustion; no independent full-build success is claimed. Author all-five-module build and explicit normal hooks passed separately.

Evidence uses a fake external SDK boundary. No live AWS purchase or acceptance was checked. No source changes, consumer pins or security-guard changes were introduced by the refresh. New-head required CI remains pending; normal merge waits for green checks and unchanged exact head.

@cristim

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Final reviewed head: d7dfc8428f1ccaa76e6b6c06a6dba169d72b63f9, tree 3bc791e21e42917342d3e067c03ed682c4544172. Independent full-source and cold committed verification are recorded above. Required CI, including both workflow conclusions and all constituent jobs, is green on this head.

CodeRabbit completed an included review with coverage metadata naming this exact SHA and kind=reviewed. It reports no actionable comments; the inline-comments API is empty. No quota waiver or paid request is used for this final head.

Disposition of its two informational pre-merge warnings: #354 intentionally remains open for platform storage/old-tranche gates and documentation, as this approved PR covers only the AWS provider boundary. The docstring coverage suggestion is declined under repository conventions favoring concise code and test names over comments restating behavior; the shared normalizer retains its necessary caller-immutability rationale. Neither warning is a required GitHub check or an unresolved inline finding.

Normal squash merge will use an exact-head match after fresh head, base, mergeability and check-state verification. No admin bypass, live purchase, deployment, consumer pin or security-guard change is authorized by this merge.

@cristim
cristim merged commit 29adce1 into main Oct 14, 2026
15 checks passed
@cristim

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Normally squash merged as 29adce1c3883bfb71a926451c323eafea601e87f after explicit exact-merge approval and fresh green unchanged-head checks. The merge tree 3bc791e21e42917342d3e067c03ed682c4544172 exactly matches independently reviewed d7dfc842.

Both post-merge workflows completed successfully; the originating watcher owner reported exit zero and reaped each watcher:

  • CI Build & Test: run 38642001903.
  • Pre-commit: run 38642002131.

CodeRabbit completed an included review of the final head with no actionable findings; no quota waiver was needed. Existing #354 remains open for the platform portion, and #53 tracks the separate offering-ID scope issue. No additional follow-up defect was found.

Local runtime proof used a fake external SDK boundary. No live AWS purchase, consumer pin change, MCP security-guard change or deployment was performed.

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

Labels

effort/s Hours impact/few Limited audience priority/p1 Next up; this sprint severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant