Repository navigation
fix(cli): --override-count and --max-instances mutate Count without scaling costs, misstating the spend #1611
Description
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-ApplyCountOverridecmd/helpers.go:505-526-ApplyInstanceLimit- The correct idiom, in the same file:
cmd/helpers.go:173-178(applyCoverage, routed throughcommon.ScaleRecommendationCosts, with a comment explaining why desynchronising count and cost is a bug) andcmd/helpers.go:396-403(applyTargetCoverageRI) - Consumers of the stale fields:
ConfirmPurchase's savings figure (cmd/multi_service.go:157->sumPassedRecsoverEstimatedSavings), theUpfrontPayment/EstimatedSavingsCSV columns (cmd/multi_service_csv.go:272-274), theTOTALrow (cmd/multi_service_csv.go:307-321), and the audit record'sEstimatedCost(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-instancesis 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 meansApplyInstanceLimitruns many times per run, so the cost desync fires once per region as well.
Activity
cristim commentedon Aug 7, 2026
Adding evidence from #1608's fix (PR #1725), because it widens this issue beyond the flags in the title.
ApplyInstanceLimithas the same defect as--override-count. When it truncates a recommendation to fit the remaining budget, the reduced recommendation keeps its pre-truncationCommitmentCostandEstimatedSavings. 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-countbug: 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 coverApplyInstanceLimit's truncation path, since both mutateCountwithout rescaling the derived money fields. One fix, applied at the pointCountchanges, rather than two.Found while reviewing #1725.
cristim commentedon Aug 8, 2026
Scope addition that settles where this fix belongs:
ApplyCountOverridehas the identical defect in the opposite direction.ApplyInstanceLimitlowersCountand leavesEstimatedSavings/CommitmentCostat their pre-truncation values -> a rec capped from 8 to 2 overstates by 4x.ApplyCountOverrideraisesCountand leaves the costs low ->--override-count 10on aCount = 1rec 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 —
Countis 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 pointCountchanges rather than at either call site, beyond it merely being tidier.Blast radius, enumerated: the confirmation prompt via
sumPassedRecs, theUpfrontPaymentandEstimatedSavingsCSV columns, theTOTALrow, andpkg/common/audit.go'sEstimatedCost— so the wrong figure reaches the audit record, not just the screen.Note for whoever writes the test: a regression test asserting only
Countpasses 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
ApplyInstanceLimithalf 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 commentedon Sep 2, 2026
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.