[IAmJSD]

Closes #392.

Root cause

common.BreakEven returned (0, true) for any CommitmentCost == 0, so a missing upfront price on an upfront payment option counted as a known zero upfront: a break-even of 0 months. That's wrong for both shapes in the issue: the Azure Advisor "upfront" variant, and AWS recs with a nil UpfrontCost.

Fix (pkg only; providers don't need to set anything)

• New helper: common.UpfrontPriceUnknown(rec) in pkg/common/breakeven.go returns true when CommitmentCost == 0 and the payment option is all-upfront, partial-upfront, upfront or partial. Matching ignores case, -, _ and spaces. PaymentOption already tells "no upfront" apart from "price missing".
• BreakEven: returns (0, false) in that case. A positive provider BreakEvenMonths, which go#452 fills in for AWS RIs, is still checked first and wins.
• Scorer (pkg/scorer/scorer.go): when a maximum is set, these recs are dropped with a reason starting price-unknown:, exported as scorer.FilterReasonPriceUnknown. With no maximum set they pass, as the issue asks. The break-even check moved into breakEvenReason so filterReason stays under the gocyclo limit.
• Unchanged: no-upfront, monthly, empty and unrecognized payment options still read 0 as a real zero.

Behavior for callers

Rec shape	Max set: before	Max set: after	No max set
Upfront option, no price	passes	dropped as price-unknown	passes
No-upfront, monthly or empty, cost 0	passes	passes	passes
Provider BreakEvenMonths > 0	compared to max	compared to max	passes

An unknown break-even is never written onto recs that pass. The GCP memorystore and cloudsql tests that call BreakEven (monthly, cost 0) are unaffected.

Merge with go#452 (#390)

git merge-tree --write-tree HEAD fork/fix/issue-390 is clean, and so is the merge with current origin/main. When AWS gives a positive break-even, BreakEven returns before the changed code.

Verification

All runs used AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. These are unit tests with no cloud calls.

• Fail-before / pass-after: the new tests fail against the old breakeven.go and scorer.go, using a temporary test-only stub for the two new names. They pass after the change.
• Mutation probes: 8 run, all 8 caught.
• Build and tests: go build, go vet and go test -short pass in all 5 workspace modules. GOWORK=off go test -race -short passes in pkg, aws, azure and gcp.
• Lint and complexity: golangci-lint v2.10.1 reports 0 issues in pkg. Nothing is over gocyclo 10; BreakEven is exactly 10.
• Secrets: git-secrets is clean, run on a temp copy with an isolated HOME.

Out of scope / follow-ups

• The CommitmentCostUnknown field: the issue's plan names one for Advisor rows (G6a-3), and it isn't added here. Its only remaining use would be recs with an empty or unrecognized payment option. That belongs with feat(common): distinguish a known zero cost from unknown on Recommendation cost fields #326 and the fix(money): zero-vs-absent conflation in providers/azure and pkg/config,pkg/scorer [go part] #62 epic. If you want the field anyway, this should be Refs #392.
• Docs: docs/cli/filtering.md in the CLI could mention price-unknown under --max-break-even-months.

Maintainer: please mirror #392's labels (priority/p2, severity/medium, urgency/this-sprint, impact/few, effort/s, type/bug, triaged).

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 31 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: 2292e0f9-621d-4d8c-9956-21f3207b680c

📥 Commits

Reviewing files that changed from the base of the PR and between caf1d45 and 5f83d85.

📒 Files selected for processing: 4

• pkg/common/breakeven.go
• pkg/common/breakeven_test.go
• pkg/scorer/scorer.go
• pkg/scorer/scorer_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 c2c4444

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.

Payment options. UpfrontPriceUnknown (pkg/common/breakeven.go:68) lowercases, strips -/_/spaces, then exact-matches {allupfront, upfront, partialupfront, partial}. It is not a substring match.

Provider value	Caught as upfront
AWS offerings "All Upfront" / "Partial Upfront" (ec2 client.go:685, rds :598, elasticache :427, memorydb :477, opensearch :558)	Yes
AWS recs, ladder and redshift "all-upfront" / "partial-upfront" (redshift/client.go:666-670)	Yes
Azure "upfront" (converter.go:426, consumption.go:274)	Yes
"No Upfront" / "no-upfront" / "no_upfront" (normalizes to noupfront), Azure/GCP "monthly" (converter.go:432, gcp computeengine:1099), redshift "unknown", empty	No. These stay (0, true).

Scorer:

• Under MaxBreakEvenMonths, an unknown break-even is filtered (fail-closed, scorer.go:113-118), now with a "price-unknown:" reason.
• With no max set, nothing is filtered.
• The only other BreakEven consumer is scorer.go:62, which writes BreakEvenMonths only when known and > 0.

Interplay with #452:

• git merge-tree of the two heads has no conflicts.
• A positive Cost Explorer BreakEvenMonths still wins (fix(aws): parse Cost Explorer EstimatedBreakEvenInMonths into BreakEvenMonths #452). When CE sends 0 or nothing, this PR marks the rec unknown. The two are compatible.
• Suggested order: this PR first (the safety fix), then fix(aws): parse Cost Explorer EstimatedBreakEvenInMonths into BreakEvenMonths #452.

What was attacked and how (pkg, GOWORK=off):

• go vet ./... is clean.
• go test -race -count=1 ./... passes 15 packages, and ./common ./scorer passes 362.
• Mutation probes, all killed:
  • The pre-fix (0, true) fails TestBreakEven and TestScore_MaxBreakEvenMonths_PriceUnknown.
  • A substring match on "upfront" fails TestUpfrontPriceUnknown, TestBreakEven and the scorer test, so a no-upfront regression is caught.
  • Disabling the scorer reason branch fails the scorer test.
• Fail-before: this is a compile failure, because the tests use the new symbols. Mutation A covers the behaviour.

Gate:

• Labels are mirrored from fix(common): BreakEven treats CommitmentCost 0 as a known zero upfront #392.
• Needs the author to mark the draft ready, green CI, and its turn in the go merge order.

[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: this one before #452.

[IAmJSD]

Brought up to date with main in df4defb 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> df4defb contains only main's changes.

[cristim]

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

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

CI is re-running on df4defb1. Still a draft: @IAmJSD, please mark it ready for review (merges before #452).

[IAmJSD]

Merged main again in 8a41241 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 8a412411, 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 8a412411 (pre-commit and Build & Test). Merge order: before #452. 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 256b1f4 (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 5f83d85a2bd49a8f5498e6641328e2823cbb518e (merge driver)

• Independent review CLEAN at 256b1f40 (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): go test -race -short ./... && go vet ./... in pkg, plus go test -short ./... in providers/aws, providers/azure, providers/gcp and ci_cd_sanity_tests (pkg is shared), all pass.
• CI green on this head: CI - Build & Test 39048102116, pre-commit 39048102098. mergeState CLEAN, no unresolved threads, labels mirrored from fix(common): BreakEven treats CommitmentCost 0 as a known zero upfront #392. CR waived (quota): adversarial review + local verification + green CI.

Squash-merging with --match-head-commit 5f83d85a.