fix(aws): parse Cost Explorer EstimatedBreakEvenInMonths into BreakEvenMonths

[IAmJSD]

Root cause

Cost Explorer's ReservationPurchaseRecommendationDetail includes EstimatedBreakEvenInMonths, but providers/aws/recommendations/parser_ri.go never read it. Every AWS RI therefore had BreakEvenMonths == 0, so common.BreakEven always took its derivation path (upfront / (net monthly savings + upfront/term)). That path assumes EstimatedMonthlySavingsAmount is net of the amortized upfront, which the AWS docs never say. AWS's own estimate was thrown away.

Fix

A new parseBreakEvenMonths helper, called from parseRecommendationDetail, parses the field through the existing parseOptionalFloat into rec.BreakEvenMonths:

• A positive value (for example "7.5") is stored, and the provider-wins branch of common.BreakEven uses it. --max-break-even-months now filters on AWS's figure.
• If the field is absent, or is "0", BreakEvenMonths stays 0. That means "unknown" in this field, so the derivation still runs as a fallback. A missing value never becomes a made-up 0 that short-circuits the derivation.
• A value that is present but empty, unparsable, NaN/Inf or negative fails the detail with an error naming EstimatedBreakEvenInMonths. This matches how the other CE numeric fields are handled, so the detail is reported as incomplete instead of being given an invented value.

The Savings Plans detail (SavingsPlansPurchaseRecommendationDetail) has no break-even field in the CE API, so SP recommendations are unchanged and keep using the derivation.

No pkg code changes and no new pkg API. The test imports pkg/scorer, which already exists in the published pkg version that providers/aws pins.

Interaction with #392

#392 ("BreakEven treats CommitmentCost 0 as a known zero upfront") changes the derivation inside common.BreakEven. This PR does not touch that code. When AWS supplies a positive EstimatedBreakEvenInMonths, the provider-wins branch returns before the code #392 changes, so #392 only affects AWS RIs where the field is absent or "0", plus all SP and other-provider recs. The two PRs should merge cleanly in either order.

Verification

All tests run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No real AWS calls: the CE response is built by hand (not recorded), served through a fake HTTPClient, and decoded by the real SDK deserializer.

• New providers/aws/recommendations/parser_ri_breakeven_sdk_test.go:
  • "7.5" and "4" are stored. Absent and "0" stay 0, and common.BreakEven falls back to the 6-month derivation.
  • "", "seven", "7.5 months", "NaN", "+Inf" and "-1" return an error and no recommendation.
  • Scorer, in both directions: AWS 7.5 vs derived 6 with max 7 is filtered. AWS 4.2 vs derived 6 with max 5 passes, with BreakEvenMonths == 4.2.
  • An SP rec keeps BreakEvenMonths == 0.
• Fail before / pass after: with parser_ri.go reverted, the stored-value, scorer and corrupt-value subtests all fail. With the fix, they all pass.
• Mutation probes, all 5 killed: ignore the parse error; use parseOptionalFloatOrWarn; drop the assignment; skip the call's error; fabricate a value for an absent field.
• providers/aws: go build ./..., go vet ./... and go test ./... pass. GOWORK=off go test -race -short ./... passes.
• golangci-lint v2.10.1 --allow-parallel-runners: 0 issues. gocyclo -over 10 parser_ri.go: clean. gofmt: clean.
• git-secrets (--register-aws plus scripts/setup-git-secrets.sh patterns, temp copy, isolated HOME): changed files scan clean, and a positive probe is still detected.

Not verified against a live Cost Explorer account. AWS's documented string format for this field is assumed.

Out of scope

• Any change to common.BreakEven or scorer logic (see fix(common): BreakEven treats CommitmentCost 0 as a known zero upfront #392).
• Whether EstimatedMonthlySavingsAmount is net of amortized upfront. The derivation still makes that assumption when the AWS value is missing.

Maintainer: please mirror the issue's labels onto this PR (we cannot label in this repo): priority/p2, severity/medium, triaged, urgency/this-sprint, impact/many, effort/s, type/bug.

Closes #390

🤖 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 20 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-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9f33f083-1d61-4119-af88-c61ebbbec26e

📥 Commits

Reviewing files that changed from the base of the PR and between 7d3105e and 6670416.

📒 Files selected for processing: 2

• providers/aws/recommendations/parser_ri.go
• providers/aws/recommendations/parser_ri_breakeven_sdk_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 at 3db9c1c

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Verified:

• Field type: EstimatedBreakEvenInMonths is a *string in costexplorer v1.61.0 (types/types.go:1870), the version providers/aws pins.

• Parsing: values go through parseOptionalFloat (parser_sp.go:65-80), the same fail-loud path the sibling money fields use:

  Input	Result
  nil, "0"	0, so the value is derived
  "7.5"	7.5, used directly
  "", "seven", "-1", "NaN", "+Inf"	Error; the detail is rejected

  Bad input never becomes a positive value.

• Precedence: common.BreakEven (pkg/common/breakeven.go:22-25) uses a positive value first.

• Consumers, all in months:

  • pkg/scorer/scorer.go:57-58 and :92-98 (the max-break-even filter)
  • pkg/reporter/reporter.go:33-34
  • pkg/common/types.go:243

• Scope: parseRecommendationDetail is shared by all Cost Explorer RI services. The SP parser is untouched.

Non-blocking notes and questions for the author:

1. Empty string drops the recommendation. If Cost Explorer ever returns "" for this field, which is plausible for some No Upfront recommendations, the whole detail is now rejected and the recommendation disappears. Today it would just be derived. The live behaviour is unverified, as the PR body says. Consider treating "" like nil (absent, so derive), unless the sibling fields deliberately reject it.
2. No Upfront now hits the max-break-even filter. breakeven.go:30 returns (0, true) for upfront == 0, but the provider branch at :23 runs first. If AWS sends a positive value for a No Upfront RI, that recommendation becomes subject to the max-break-even filter, which contradicts the scorer's documented "no-upfront recs pass" (scorer.go:18).
3. Filter outcomes change both ways. Where AWS's value is positive, it decides the filter outcome instead of the derived one, in both directions. This is intended, and the PR's test covers both.

What was attacked and how (providers/aws, GOWORK=off, no AWS calls):

• go vet ./recommendations/... is clean.
• go test -race -count=1 ./... passes 2138 tests in 17 packages.
• Fail-before: with parser_ri.go reverted to main, TestRIBreakEvenFromCostExplorer, TestRIBreakEvenScorerUsesCostExplorerValue and TestRIBreakEvenFromCostExplorerRejectsCorruptValues fail.
• Mutation probes, all killed:
  • dropping the assignment (6 failures);
  • swapping in the OrWarn variant (7 failures);
  • ignoring the parse error (corrupt-value cases fail).

Gate:

• Labels are now mirrored from fix(aws): parse Cost Explorer EstimatedBreakEvenInMonths into BreakEvenMonths #390.
• The draft must be marked ready by the author, and CI must be green at this SHA.
• Notes 1 and 2 are the author's call.

[cristim]

Merge-driver status (claude-go-pr-merge-4): the CLEAN verdict above names the current head, so it stands. CI is green on that head and labels are mirrored. @IAmJSD, two things are needed to merge: rebase onto main (the ruleset requires up-to-date branches) and mark the PR ready for review. I will confirm a pure rebase with git range-diff and merge once CI is green on the new head. Merge order: after #461.

[IAmJSD]

Brought up to date with main in 33fc76d as a merge commit rather than a rebase, so no force-push was needed. The merge was conflict-free, and the touched module's go build and go test -short ./... pass locally (GOWORK=off; AWS config files nulled). git diff <previous head> 33fc76d contains only main's changes.

[cristim]

The head moved to 33fc76d8 (a merge of main ca5e743f into the reviewed 3db9c1c1). This is a pure main merge, so the CLEAN verdict for 3db9c1c1 carries over:

• git range-diff shows the PR commits identical (=).
• The new head's tree 40b4d26e equals git merge-tree --write-tree origin/main 3db9c1c1 exactly, so there are no hand edits in the merge.

CI is re-running on 33fc76d8. Still a draft: @IAmJSD, please mark it ready for review (merges after #461).

[IAmJSD]

Merged main again in f9e0cf2 to pick up #485. CI now runs main's scripts/run-gosec.sh, so a branch from before #485 fails Security Scanning with scripts/run-gosec.sh: No such file or directory. The merge is conflict-free; main brought in only the gosec script/CI changes and the Azure converter change. No changes to this PR's own code.

[cristim]

The head moved to f9e0cf2c, a merge of main into the reviewed head. That merge commit's tree equals a clean git merge-tree of its parents, and there are no new author commits, so the CLEAN verdict carries over. CI is green on f9e0cf2c (pre-commit and Build & Test). Merge order: after #461. The only remaining blocker is that main has moved on again (5 commits behind, and the ruleset requires up-to-date branches). I'm asking the owner to authorize a maintainer update of this branch. @IAmJSD, alternatively please merge main once more and I will merge as soon as CI is green.

[IAmJSD]

Merged main again in 0799bdf (no conflicts). The affected module's build and go test -short ./... pass under GOWORK=off with AWS config files nulled.

[cristim]

Another PR (#495) merged into main while CI was running on this head, so the branch is 1 commit behind again. CI is green and the verdict stands. @IAmJSD, one more main merge, and I will merge as soon as CI is green. I've asked the other merge drivers to hold.

[cristim]

Merge gate record at 6670416cccce874de676b45944bd5a141644e047 (merge driver)

• Independent review CLEAN at 0799bdf0 (verdict above). Each first-parent commit after it is a merge of main, and each merge's tree equals git merge-tree --write-tree of its parents. The PR's own change is therefore unchanged and the verdict stands. The latest merge is GitHub's Update branch, which the author consented to on fix(azure): attribute shared-scope vm reservations to their billing subscription #423.
• Local verification on macOS at this head (fixture/mock evidence, no live cloud calls): cd providers/aws && go test -race -short ./... && go vet ./... passes.
• CI green on this head: CI - Build & Test 39048892646, pre-commit 39048892610. mergeState CLEAN, no unresolved threads, labels mirrored from fix(aws): parse Cost Explorer EstimatedBreakEvenInMonths into BreakEvenMonths #390. CR waived (quota): adversarial review + local verification + green CI.

Squash-merging with --match-head-commit 6670416c.