Surfaced during the adversarial review of #1825 (which closes #1741). Pre-existing, affects both the default and the --input-csv path, and deliberately not fixed there to keep that PR to one concern.
What
ApplyInstanceLimit (cmd/helpers.go:514-536) truncates a recommendation's Count to fit the --max-instances budget but does not rescale the quantities derived from that count:
adjusted := rec // full struct copy, EstimatedSavings included
if rec.Count > remaining {
adjusted.Count = remaining // only Count is reduced
}
result = append(result, adjusted)EstimatedSavings (pkg/common/types.go:212) is an absolute figure for the whole row. Cut a row from 100 instances to 10 and it keeps the savings of all 100.
Reproduced during the #1825 review: a probe row entered with Count=100, EstimatedSavings=600 and left as count=10 savings=600, so the reported saving is 10x the truth for that row.
Other extensive fields on the same struct almost certainly need the same treatment. RecurringMonthlyCost (:219), and on the sibling types UpfrontCost (:414) and TotalCost (:416), are all per-quantity figures. Enumerate every field that scales with Count before fixing, rather than patching EstimatedSavings alone. Fixing one and missing the others would leave the report internally inconsistent, which is harder to notice than a uniformly wrong number.
Why it matters
The truncated figure flows into the run summary and the purchase report, so an operator sees a savings number for a capped run that is larger than what the run can deliver. On a money path, a report that overstates the benefit of a purchase is the wrong direction to be wrong in.
It also compounds the ranking defect found in #1825: when a cap binds, the ordering key is itself un-rescaled, so rows are both ranked and reported on quantities that do not correspond to what will actually be bought.
Fix direction
Scale every count-derived field by adjusted.Count / rec.Count when the row is truncated. Guard rec.Count <= 0 so the ratio is never computed from a zero or negative denominator.
Where a field is a pointer (RecurringMonthlyCost is *float64), preserve the distinction between absent and zero: a nil stays nil, it does not become 0. Coercing absent to zero is its own defect on this path.
Verification bar
Both directions, and assert the invariant rather than a single number:
- a row truncated from N to M has its savings scaled by M/N, not left at the N value and not zeroed
- a row that is not truncated is unchanged, including its pointer fields
- the run summary total equals the sum of the post-truncation rows
Mutation-verify each test individually (go test ./cmd/ -run '^Name$' -count=1); an aggregate run misreports because a panic kills the binary. Every failure must be by assertion, not panic. Confirm the regression test fails against the current code before the fix.
Surfaced during the adversarial review of #1825 (which closes #1741). Pre-existing, affects both the default and the
--input-csvpath, and deliberately not fixed there to keep that PR to one concern.What
ApplyInstanceLimit(cmd/helpers.go:514-536) truncates a recommendation'sCountto fit the--max-instancesbudget but does not rescale the quantities derived from that count:EstimatedSavings(pkg/common/types.go:212) is an absolute figure for the whole row. Cut a row from 100 instances to 10 and it keeps the savings of all 100.Reproduced during the #1825 review: a probe row entered with
Count=100, EstimatedSavings=600and left ascount=10 savings=600, so the reported saving is 10x the truth for that row.Other extensive fields on the same struct almost certainly need the same treatment.
RecurringMonthlyCost(:219), and on the sibling typesUpfrontCost(:414) andTotalCost(:416), are all per-quantity figures. Enumerate every field that scales withCountbefore fixing, rather than patchingEstimatedSavingsalone. Fixing one and missing the others would leave the report internally inconsistent, which is harder to notice than a uniformly wrong number.Why it matters
The truncated figure flows into the run summary and the purchase report, so an operator sees a savings number for a capped run that is larger than what the run can deliver. On a money path, a report that overstates the benefit of a purchase is the wrong direction to be wrong in.
It also compounds the ranking defect found in #1825: when a cap binds, the ordering key is itself un-rescaled, so rows are both ranked and reported on quantities that do not correspond to what will actually be bought.
Fix direction
Scale every count-derived field by
adjusted.Count / rec.Countwhen the row is truncated. Guardrec.Count <= 0so the ratio is never computed from a zero or negative denominator.Where a field is a pointer (
RecurringMonthlyCostis*float64), preserve the distinction between absent and zero: a nil stays nil, it does not become0. Coercing absent to zero is its own defect on this path.Verification bar
Both directions, and assert the invariant rather than a single number:
Mutation-verify each test individually (
go test ./cmd/ -run '^Name$' -count=1); an aggregate run misreports because a panic kills the binary. Every failure must be by assertion, not panic. Confirm the regression test fails against the current code before the fix.