Skip to content

fix(cli): bound --min-savings-pct to 0-100 - #2231

Merged
cristim merged 1 commit into
mainfrom
fix/min-savings-pct-range
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/min-savings-pct-range

Conversation

@cristim

@cristim cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Closes #2161

Summary

--min-savings-pct was only checked for being finite. That allowed two silent mistakes:

  • A negative value behaved as "no filter", because the scorer only filters when MinSavingsPct > 0.
  • A value above 100 dropped every recommendation. SavingsPercentage is AWS's EstimatedMonthlySavingsPercentage against on-demand (the comparison is at pkg/scorer/scorer.go:88), so it can never exceed 100.

This PR adds the following:

  • validateCoverageAndSavingsPct now rejects values outside [0, 100] with min-savings-pct must be between 0 and 100, got: <v>. Both ends are inclusive, so 0 keeps today's "no filter" default.
  • docs/cli/filtering.md documents the range, and CHANGELOG.md has an entry under Fixed.
  • The flag-parsing validation table is renamed TestValidateNumericFlagsRejectInvalid, since it now covers out-of-range values as well as non-finite ones.

Validation ordering: the range check runs before the CSV-mode refusal, so --min-savings-pct 150 --input-csv x reports the range error. Both reject the run.

Evidence (local, macOS, GOTOOLCHAIN=go1.26.9; unit tests through the real flag set)

  • Fail before / pass after: with the validators.go change stashed, the new min-savings-pct negative and min-savings-pct above 100 cases fail. With the change, they pass, and 100 stays valid.
  • Checks:
    • make build passes.
    • go vet ./... passes.
    • go test -race ./cmd/...: 1472 passed.
    • golangci-lint v2.10.1 (the CI pin): 0 issues.
    • scripts/test-git-secrets-allowlist.sh: 44/44.
    • Pre-commit hooks pass.
  • Review: an independent non-author reviewer (sonnet, high) did two passes. It confirmed the [0, 100] bound from source, confirmed the CHANGELOG claims, and found no bypass: the flag is the only writer and is validated in PreRunE. Its one low finding, about where the tests sit and what the table is named, is applied.

🤖 Generated with Claude Code

--min-savings-pct was only checked for being finite. A negative value
behaved as no filter and a value above 100 dropped every recommendation,
since savings percentages are measured against on-demand and cannot
exceed 100. Reject both at validation, document the range, and rename the
flag-parsing validation test to cover out-of-range as well as non-finite
values.

Closes #2161

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/xs Trivial / one-liner triaged Item has been triaged labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 4 billable files and costs up to $1.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 27 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: fcfb4900-ea66-4393-b94d-f9656611adc9
📥 Commits

Reviewing files that changed from the base of the PR and between e0a1db6 and 2ca29cc.

📒 Files selected for processing: 4
  • CHANGELOG.md
  • cmd/validators.go
  • cmd/validators_test.go
  • docs/cli/filtering.md
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at 2ca29cc

Reviewer: fresh Claude agent (Sonnet, high effort), not the author. Coordinator: claude-cc-reviewer.

What was checked:

  • No sentinels: every MinSavingsPct use is in cmd/: main.go:74 (field), main.go:146 (flag, default 0), multi_service.go:357 (passed to scorer.Config), validators.go:271 (CSV-mode > 0). Tests use 0, 5 and 20, and there are no config-file, env or programmatic setters.
  • Wiring: PreRunE: validateFlags (main.go:100) → validateNumericRanges → validateCoverageAndSavingsPct. It runs for every use of the flag and before any network call, and NaN/Inf are rejected first.
  • Semantics: checked by the coordinator in the pinned go module. scorer.go:80-81 compares rec.SavingsPercentage < cfg.MinSavingsPct and formats both as %.1f%%, so the value is a 0-100 percentage and the bound matches.
  • go vet ./cmd/... is clean, and go test -race -count=1 ./cmd/... passes 1472.

Gate:

@cristim
cristim merged commit 700ff43 into main Oct 16, 2026
12 checks passed
@cristim
cristim deleted the fix/min-savings-pct-range branch October 16, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cmd): --min-savings-pct has no range validation

1 participant