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): --input-csv enforces neither --min-count nor savings-ordered capping, silently #1741
The --input-csv purchase path enforces neither --min-count nor savings-ordered selection. Both are enforced on the default path.
Found while reviewing PR #1725, which cites the CSV path as the reference for correct global cap semantics. That citation is wrong in a way worth correcting: the CSV path is global, but it is global in load order.
Two distinct defects
1. --min-count is silently unenforced in CSV mode.
There is no scorer and no --min-count gate anywhere on the CSV path. Confirmed by exhaustive grep, not inference: scoreLimitAndDisplay is called only at cmd/multi_service.go:140 (the default path), and cfg.MinCount appears only at :217 and in the new floor-drop code #1725 added to the default path.
Consequence: a recommendation truncated by --max-instances to below the --min-count floor is purchased short in CSV mode. On the default path #1725 now drops it. An operator setting --min-count 5 and getting a 2-instance commitment is the exact defect #1608/#1725 fixed for the default path, still live here.
2. The CSV cap selects in load order, not savings order.
filterAndAdjustRecommendations applies ApplyInstanceLimit at cmd/multi_service.go:610, before any scoring. ApplyInstanceLimit consumes its input in slice order and drops the tail, so whichever rows appear first in the file consume the budget.
That is precisely the selection defect PR #1725 established as the important correctness property on the default path — where the cap now runs afterscorer.Score so the survivors are the highest-savings recommendations run-wide. In CSV mode the survivors are whatever the file happened to list first.
Why this is not simply "the CSV path is different"
--max-instances and --min-count are documented in docs/cli/filtering.md as flags of the tool, not of a mode. An operator who reads that page and passes --input-csv gets neither behaviour, with nothing indicating the flags were ignored. Silent partial enforcement of a spend guard is worse than not offering it.
Suggested approach
Decide first, then implement — the two ends are genuinely different products:
If CSV is meant to be "purchase exactly what I listed", then --min-count and savings-ordered capping arguably should not apply at all — but the tool must then refuse those flags in CSV mode with a clear error rather than accepting and ignoring them.
Do not split the difference by applying one and not the other.
Tests must cover the real scenario either way: a CSV whose best-value rows are not first, a cap that binds, and a --min-count that a truncation would breach. A test asserting only the total passes today with both defects present — see #1725, where exactly that gap let a pre-scoring cap look correct.
Note #1609, #1610 and this issue all live in filterAndAdjustRecommendations / runToolFromCSV, and #1719 records that both of those functions sit at exactly the gocyclo ceiling of 10. Whoever takes any of the three inherits that constraint.
The
--input-csvpurchase path enforces neither--min-countnor savings-ordered selection. Both are enforced on the default path.Found while reviewing PR #1725, which cites the CSV path as the reference for correct global cap semantics. That citation is wrong in a way worth correcting: the CSV path is global, but it is global in load order.
Two distinct defects
1.
--min-countis silently unenforced in CSV mode.There is no scorer and no
--min-countgate anywhere on the CSV path. Confirmed by exhaustive grep, not inference:scoreLimitAndDisplayis called only atcmd/multi_service.go:140(the default path), andcfg.MinCountappears only at:217and in the new floor-drop code #1725 added to the default path.Consequence: a recommendation truncated by
--max-instancesto below the--min-countfloor is purchased short in CSV mode. On the default path #1725 now drops it. An operator setting--min-count 5and getting a 2-instance commitment is the exact defect #1608/#1725 fixed for the default path, still live here.2. The CSV cap selects in load order, not savings order.
filterAndAdjustRecommendationsappliesApplyInstanceLimitatcmd/multi_service.go:610, before any scoring.ApplyInstanceLimitconsumes its input in slice order and drops the tail, so whichever rows appear first in the file consume the budget.That is precisely the selection defect PR #1725 established as the important correctness property on the default path — where the cap now runs after
scorer.Scoreso the survivors are the highest-savings recommendations run-wide. In CSV mode the survivors are whatever the file happened to list first.Why this is not simply "the CSV path is different"
--max-instancesand--min-countare documented indocs/cli/filtering.mdas flags of the tool, not of a mode. An operator who reads that page and passes--input-csvgets neither behaviour, with nothing indicating the flags were ignored. Silent partial enforcement of a spend guard is worse than not offering it.Suggested approach
Decide first, then implement — the two ends are genuinely different products:
--min-countand savings-ordered capping arguably should not apply at all — but the tool must then refuse those flags in CSV mode with a clear error rather than accepting and ignoring them.Do not split the difference by applying one and not the other.
Tests must cover the real scenario either way: a CSV whose best-value rows are not first, a cap that binds, and a
--min-countthat a truncation would breach. A test asserting only the total passes today with both defects present — see #1725, where exactly that gap let a pre-scoring cap look correct.Related
Note #1609, #1610 and this issue all live in
filterAndAdjustRecommendations/runToolFromCSV, and #1719 records that both of those functions sit at exactly the gocyclo ceiling of 10. Whoever takes any of the three inherits that constraint.