Skip to content

fix(cli): --input-csv enforces neither --min-count nor savings-ordered capping, silently #1741

Description

@cristim

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 after scorer.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.
  • If CSV is meant to be "the same pipeline, different input", route it through the same scoring and floor gates the default path uses, and the cap moves after scoring for the same reason it did in fix(cli): apply --max-instances once run-wide, not per service and region #1725.

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.

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions