fix(cli): --override-count and --max-instances mutate Count without scaling costs, misstating the spend

[cristim]

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

ApplyCountOverride and ApplyInstanceLimit assign .Count directly and leave the cost fields describing the old quantity. The confirmation prompt, the CSV report and the audit record all then state the wrong money, on the exact screen the operator uses to approve the spend.

Where

• cmd/helpers.go:491-502 - ApplyCountOverride
• cmd/helpers.go:505-526 - ApplyInstanceLimit
• The correct idiom, in the same file: cmd/helpers.go:173-178 (applyCoverage, routed through common.ScaleRecommendationCosts, with a comment explaining why desynchronising count and cost is a bug) and cmd/helpers.go:396-403 (applyTargetCoverageRI)
• Consumers of the stale fields: ConfirmPurchase's savings figure (cmd/multi_service.go:157 -> sumPassedRecs over EstimatedSavings), the UpfrontPayment / EstimatedSavings CSV columns (cmd/multi_service_csv.go:272-274), the TOTAL row (cmd/multi_service_csv.go:307-321), and the audit record's EstimatedCost (pkg/common/audit.go:60-61)

What

After either helper runs, CommitmentCost, OnDemandCost, EstimatedSavings and RecurringMonthlyCost still describe the pre-override or pre-truncation quantity. Every other sizing path in the same file routes through common.ScaleRecommendationCosts precisely to keep the two in sync, and applyCoverage carries a comment saying why.

Failure scenario (override)

A recommendation with Count = 1, CommitmentCost = $4,000. The operator runs:

cudly ... --override-count 10 --purchase

ApplyCountOverride sets Count = 10 and leaves CommitmentCost = $4,000. The confirmation prompt and the CSV both report about $4,000 upfront while AWS is charged about $40,000. That is a 10x understatement on the approval screen.

Failure scenario (limit)

A recommendation with Count = 20, CommitmentCost = $100,000, run with --max-instances 5. Count becomes 5, the cost stays $100,000, so the CSV and the audit record overstate the purchase by 4x. The error runs in both directions depending on which helper fired, so an operator cannot even learn a consistent correction factor.

Fix direction

Route both helpers through common.ScaleRecommendationCosts(rec, newCount/oldCount) the way applyCoverage does, guarding oldCount == 0.

The regression test should assert the cost fields, not just Count. A test that only checks Count after the override passes today and would keep passing with the bug present.

Related

• --max-instances is separately applied per (service, region) rather than as the documented global cap, filed as its own issue from this review. The two interact: the per-region application means ApplyInstanceLimit runs many times per run, so the cost desync fires once per region as well.

[cristim]

Adding evidence from #1608's fix (PR #1725), because it widens this issue beyond the flags in the title.

ApplyInstanceLimit has the same defect as --override-count. When it truncates a recommendation to fit the remaining budget, the reduced recommendation keeps its pre-truncation CommitmentCost and EstimatedSavings. So a rec cut from 8 instances to 2 still displays the cost and savings of 8 on the confirmation screen the user approves from.

That makes this a money-display bug on the approval path, not only a --override-count bug: the number the user consents to is wrong whenever a cap truncates anything.

PR #1725 moves the cap from per-(service, region) to once per run. That reduces how often the stale figures appear — previously truncation could happen once per service/region pair, now at most once per run — but it does not fix the underlying issue. Deliberately out of scope there; #1608 was about the cap being multiplied, not about cost scaling.

Suggested addition to this issue's scope: whatever fixes --override-count's count/cost coupling should also cover ApplyInstanceLimit's truncation path, since both mutate Count without rescaling the derived money fields. One fix, applied at the point Count changes, rather than two.

Found while reviewing #1725.

[cristim]

Scope addition that settles where this fix belongs: ApplyCountOverride has the identical defect in the opposite direction.

• ApplyInstanceLimit lowers Count and leaves EstimatedSavings/CommitmentCost at their pre-truncation values -> a rec capped from 8 to 2 overstates by 4x.
• ApplyCountOverride raises Count and leaves the costs low -> --override-count 10 on a Count = 1 rec understates the upfront by 10x.

Understating an upfront commitment on the confirmation screen is the worse of the two: the operator approves a number smaller than what will actually be charged.

So this is not two adjacent bugs, it is one defect at one place — Count is mutated without rescaling the fields derived from it — reachable from two helpers. Fixing only one leaves the sibling broken and means touching the same lines twice. That is the concrete reason the fix belongs at the point Count changes rather than at either call site, beyond it merely being tidier.

Blast radius, enumerated: the confirmation prompt via sumPassedRecs, the UpfrontPayment and EstimatedSavings CSV columns, the TOTAL row, and pkg/common/audit.go's EstimatedCost — so the wrong figure reaches the audit record, not just the screen.

Note for whoever writes the test: a regression test asserting only Count passes today with the bug present. The assertion has to be on the derived money fields, or it proves nothing. This has been a recurring shape here — see #1595, where 77 assertions could not fail, and #1725, where a test pinning the cap total could not distinguish a correct fix from one that bought the worst commitments on offer.

CodeRabbit independently reported the ApplyInstanceLimit half on PR #1725 (cmd/multi_service.go:223), marked Heavy lift, and it was declined there with a written justification rather than folded into a cap-placement PR. Two independent finders reaching the same conclusion, plus this opposite-direction sibling, is good corroboration.

[cristim]

Verified resolved at 3c0f8ac: both helpers now rescale the count-derived money fields at the point Count changes. ApplyInstanceLimit calls common.ScaleRecommendationCosts(rec, remaining/rec.Count) before truncating, and ApplyCountOverride (now in cmd/helpers_count_override.go) scales by overrideCount/rec.Count before assigning Count, skipping Savings Plans and non-positive counts explicitly. Regression tests assert CommitmentCost/EstimatedSavings, not just Count (cmd/helpers_instance_limit_rescale_test.go, cmd/helpers_count_override_rescale_test.go). Evidence: cmd/helpers.go:188-190 (#1846), cmd/helpers_count_override.go:52-53 (#1847). Residual axes checked: confirmation prompt, CSV UpfrontPayment/EstimatedSavings columns, TOTAL row and audit EstimatedCost all read the now-scaled fields; ProjectedCoverage/ProjectedUtilization are not re-derived, tracked separately in #1845. Closing as completed; reopen if the behaviour recurs.