You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(cli): --max-instances truncation does not re-derive ProjectedCoverage/ProjectedUtilization or NumberOfNodes #1845
Surfaced during the adversarial review of the #1830 fix. Pre-existing, and left out of that PR because the fields are projections and node counts rather than money, so rescaling them does not belong in common.ScaleRecommendationCosts.
What
#1830 established that every quantity derived from Recommendation.Count must be re-derived when a sizing path changes Count. Two count-linear fields are still not re-derived when --max-instances truncates a row:
1. ProjectedCoverage / ProjectedUtilization
applyTargetCoverageRI computes ProjectedCoverage = ExistingCoveragePct + nTarget/avg*100 and a matching ProjectedUtilization, both linear in the quantity purchased. ApplyInstanceLimit truncates Count afterwards and recomputes neither, so a run combining --target-coverage with --max-instances emits a row claiming the coverage it was sized for rather than the coverage the truncated purchase will actually reach.
Concretely, --target-coverage 80 --max-instances 10 against a row sized to 100 instances writes Count=10 next to ProjectedCoverage=80.0. The CSV carries both columns, so the report is internally inconsistent in the same way #1830 described: a capped run overstating what it achieves, just denominated in percent rather than dollars.
2. DataWarehouseDetails.NumberOfNodes
Set to rec.Count at parse time in providers/aws/recommendations/parser_services.go and never re-derived by any sizing path (ApplyCoverage and ApplyTargetCoverage leave it stale too, so this predates the cap). After any Count change the mirror disagrees with the field it mirrors.
Why it matters
Neither field currently causes a wrong purchase. The Redshift purchase path reads rec.Count, not NumberOfNodes, and the projections are advisory. This is a reporting correctness issue, not a spend issue, which is why it is filed separately from #1830 rather than folded into it.
It still matters: --target-coverage exists so an operator can deliberately under-buy to a chosen coverage figure, and the projection column is the only feedback that the chosen figure was hit. A projection that silently ignores the cap tells the operator they reached 80% coverage on a run that bought a tenth of the instances.
Fix direction
For the projections, recompute in ApplyInstanceLimit for truncated rows from the fields already on the rec (AverageInstancesUsedPerHour, ExistingCoveragePct), mirroring the formulas in applyTargetCoverageRI / applyTargetCoverageSP and keeping the existing clamps to 100. Note the two formulas differ between RI and SP, so the recompute has to branch the same way the sizing does. Extracting the projection math into one helper shared by the sizing path and the cap is preferable to a third copy.
Where no signal exists (AverageInstancesUsedPerHour == 0), leave the projections rather than fabricating one, consistent with the project rule that absent is not zero.
For NumberOfNodes, decide whether the field should exist at all given it duplicates Count; removing a redundant mirror is likely better than teaching every sizing path to maintain it.
Context
common.ScaleRecommendationCosts documents both of these as deliberately out of scope, so the docstring should be updated to point at this issue once it is fixed.
Findings from the 2026-09-02 codebase audit
Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.
A10-001 (high)
A fourth count-reducing path, and this one drops the money fields too. adjustRecommendationForExcludedVersions (cmd/multi_service_engine_versions.go:479-488) sets rec.Count = newCount when instances sit on an extended-support engine version and leaves EstimatedSavings, CommitmentCost and UpfrontPayment at their pre-truncation values. The three sibling paths this issue's premise rests on all route through common.ScaleRecommendationCosts (cmd/helpers.go:189, cmd/helpers_count_override.go:56, pkg/recfilter/sizing.go:303); this one does not. A 10-instance RDS rec with 3 excluded versions buys 7 while the confirmation prompt, the CSV UpfrontPayment/EstimatedSavings columns and the TOTAL row all describe the 10-instance purchase, overstating spend and benefit by 30%. The adjusted rec reaches the purchase set via cmd/multi_service_filters.go:73. Finding A10-001.
A10-025 (medium)
A third count-mutating site for the invariant this issue states. adjustRecommendationForExcludedVersions reduces Count on the extended-support path, and its tests assert only result.Count -- cmd/multi_service_engine_versions_test.go:180, :185, :238, :255, :282 and cmd/multi_service_coverage_test.go:416 -- while no fixture sets EstimatedSavings or CommitmentCost at all. So whether the money fields are rescaled there is not merely unasserted, it is untestable with the current fixtures. The sibling flags already have exactly the coverage this path lacks (cmd/helpers_instance_limit_rescale_test.go, cmd/helpers_count_override_rescale_test.go), so the fix is a rescale test in that shape: a rec with non-zero EstimatedSavings/CommitmentCost, two of ten instances excluded, asserting both land at 0.8x. (audit finding A10-025)
Activity
fix(cli): rescale count-derived money when --max-instances truncates …
fix(cli): rescale count-derived money when --override-count replaces …
fix(cli): rescale count-derived money when --max-instances truncates …
fix(cli): rescale count-derived money when --override-count replaces …
cristim commentedon Oct 16, 2026
Claiming: working on this [claude-cli-impl]
fix(cli): re-derive count-linear projections after Count changes (#2206)