fix(cli): validate min-pool-size, remove duplicate filter call, lint fixes

- Reject fractional and negative --min-pool-size values in validateNumericRanges
  (instance counts are integers; 1.5 is nonsensical as a threshold)
- Add test cases for negative, fractional, zero, and whole-number inputs
- Remove redundant shouldIncludePoolSize call from passesDimensionFilters;
  applyFilters already handles it with logging, so the silent re-check was
  dead code that obscured the observable path
- Replace []string accumulator (poolDrops) with an int counter since labels
  are already logged per-rec and the summary only needs a count
- Fix rangeValCopy: use index loop to avoid copying 360-byte Recommendation
- Fix equalFold: use strings.EqualFold instead of ToLower+== in engine checks
- Fix godot: add missing periods to exported comments in filters and validators

Author: cristim

cmd/multi_service_filters.go

@@ -9,19 +9,21 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/common"
 )
 
-// applyFilters applies region, instance type, engine, and engine version filters to recommendations
-// currentRegion is the region being processed in the current loop iteration - if non-empty, only recommendations for that region are included
+// applyFilters applies region, instance type, engine, and engine version filters to recommendations.
+// currentRegion is the region being processed in the current loop iteration; if non-empty, only
+// recommendations for that region are included.
 func applyFilters(recs []common.Recommendation, cfg Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) []common.Recommendation {
 	var filtered []common.Recommendation
-	var poolDrops []string
+	var poolDropCount int
 	var poolDropInstances float64
 
-	for _, rec := range recs {
+	for i := range recs {
+		rec := recs[i]
 		if cfg.MinPoolSize > 0 && !shouldIncludePoolSize(rec, cfg) {
 			poolDropInstances += rec.AverageInstancesUsedPerHour
 			label := fmt.Sprintf("%s/%s/%s", rec.Service, rec.Region, rec.ResourceType)
 			log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, rec.AverageInstancesUsedPerHour)
-			poolDrops = append(poolDrops, label)
+			poolDropCount++
 			continue
 		}
 		adjusted, include := processRecommendation(rec, cfg, instanceVersions, versionInfo, currentRegion)
@@ -30,8 +32,8 @@ func applyFilters(recs []common.Recommendation, cfg Config, instanceVersions map
 		}
 	}
 
-	if len(poolDrops) > 0 {
-		log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", len(poolDrops), poolDropInstances)
+	if poolDropCount > 0 {
+		log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances)
 	}
 
 	return filtered
@@ -42,9 +44,9 @@ func applyFilters(recs []common.Recommendation, cfg Config, instanceVersions map
 // boolean-filter checks are delegated to passesDimensionFilters to keep
 // this function under gocyclo's complexity threshold.
 func processRecommendation(rec common.Recommendation, cfg Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool) {
-	// Filter to only recommendations for the current region being processed
-	// This prevents duplicating recommendations across all regions
-	// Skip this filter for Savings Plans as they are account-level, not regional
+	// Filter to only recommendations for the current region being processed.
+	// This prevents duplicating recommendations across all regions.
+	// Skip this filter for Savings Plans as they are account-level, not regional.
 	if currentRegion != "" && rec.Region != currentRegion && !common.IsSavingsPlan(rec.Service) {
 		return rec, false
 	}
@@ -53,10 +55,10 @@ func processRecommendation(rec common.Recommendation, cfg Config, instanceVersio
 		return rec, false
 	}
 
-	// Apply engine version filters - adjust instance count by subtracting extended support versions
+	// Apply engine version filters - adjust instance count by subtracting extended support versions.
 	if !cfg.IncludeExtendedSupport {
 		rec = adjustRecommendationForExcludedVersions(rec, instanceVersions, versionInfo)
-		// Skip if all instances were excluded (count reduced to 0)
+		// Skip if all instances were excluded (count reduced to 0).
 		if rec.Count <= 0 {
 			return rec, false
 		}
@@ -66,11 +68,11 @@ func processRecommendation(rec common.Recommendation, cfg Config, instanceVersio
 }
 
 // passesDimensionFilters runs the stateless include/exclude checks on
-// region, instance type, engine, account, and pool size. Returns false on
+// region, instance type, engine, and account. Returns false on
 // the first failing filter. Split out of processRecommendation to keep
 // each function's cyclomatic complexity under the gocyclo limit; the
 // dimension filters here are pure functions of rec + cfg with no side
-// effects.
+// effects. Pool-size filtering is handled with logging in applyFilters.
 func passesDimensionFilters(rec common.Recommendation, cfg Config) bool {
 	if !shouldIncludeRegion(rec.Region, cfg) {
 		return false
@@ -84,21 +86,18 @@ func passesDimensionFilters(rec common.Recommendation, cfg Config) bool {
 	if !shouldIncludeAccount(rec.AccountName, cfg) {
 		return false
 	}
-	if !shouldIncludePoolSize(rec, cfg) {
-		return false
-	}
 	return true
 }
 
 // shouldIncludePoolSize filters out RI recommendations for pools whose
 // AverageInstancesUsedPerHour is below cfg.MinPoolSize. The purpose is to
 // drop tiny pools where integer-arithmetic sizing forces 100% coverage
-// regardless of --target-coverage (e.g. avg=1 with target=80% → floor(0.8)=0
+// regardless of --target-coverage (e.g. avg=1 with target=80% -> floor(0.8)=0
 // drops, ceil(0.8)=1 over-covers). Setting --min-pool-size=2 keeps pools
 // where target can be meaningfully approximated.
 //
 // Pass-through cases: filter disabled (MinPoolSize<=0), or rec has no
-// per-hour signal (avg<=0 — SPs and recs CE didn't return usage for).
+// per-hour signal (avg<=0 -- SPs and recs CE didn't return usage for).
 // Those pools aren't sized via the per-hour formula so the filter doesn't
 // apply to them.
 func shouldIncludePoolSize(rec common.Recommendation, cfg Config) bool {
@@ -111,53 +110,53 @@ func shouldIncludePoolSize(rec common.Recommendation, cfg Config) bool {
 	return rec.AverageInstancesUsedPerHour >= cfg.MinPoolSize
 }
 
-// shouldIncludeRegion checks if a region should be included based on filters
+// shouldIncludeRegion checks if a region should be included based on filters.
 func shouldIncludeRegion(region string, cfg Config) bool {
-	// If include list is specified, region must be in it
+	// If include list is specified, region must be in it.
 	if len(cfg.IncludeRegions) > 0 && !slices.Contains(cfg.IncludeRegions, region) {
 		return false
 	}
 
-	// If exclude list is specified, region must not be in it
+	// If exclude list is specified, region must not be in it.
 	if slices.Contains(cfg.ExcludeRegions, region) {
 		return false
 	}
 
 	return true
 }
 
-// shouldIncludeInstanceType checks if an instance type should be included based on filters
+// shouldIncludeInstanceType checks if an instance type should be included based on filters.
 func shouldIncludeInstanceType(instanceType string, cfg Config) bool {
-	// If include list is specified, instance type must be in it
+	// If include list is specified, instance type must be in it.
 	if len(cfg.IncludeInstanceTypes) > 0 && !slices.Contains(cfg.IncludeInstanceTypes, instanceType) {
 		return false
 	}
 
-	// If exclude list is specified, instance type must not be in it
+	// If exclude list is specified, instance type must not be in it.
 	if slices.Contains(cfg.ExcludeInstanceTypes, instanceType) {
 		return false
 	}
 
 	return true
 }
 
-// shouldIncludeEngine checks if a recommendation should be included based on engine filters
+// shouldIncludeEngine checks if a recommendation should be included based on engine filters.
 func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool {
-	// Extract engine from recommendation
+	// Extract engine from recommendation.
 	engine := getEngineFromRecommendation(rec)
 	if engine == "" {
-		// If no engine info, include by default unless there's an include list
+		// If no engine info, include by default unless there's an include list.
 		return len(cfg.IncludeEngines) == 0
 	}
 
-	// Normalize engine name to lowercase for comparison
+	// Normalize engine name to lowercase for comparison.
 	engine = strings.ToLower(engine)
 
-	// If include list is specified, engine must be in it
+	// If include list is specified, engine must be in it.
 	if len(cfg.IncludeEngines) > 0 {
 		found := false
 		for _, e := range cfg.IncludeEngines {
-			if strings.ToLower(e) == engine {
+			if strings.EqualFold(e, engine) {
 				found = true
 				break
 			}
@@ -167,10 +166,10 @@ func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool {
 		}
 	}
 
-	// If exclude list is specified, engine must not be in it
+	// If exclude list is specified, engine must not be in it.
 	if len(cfg.ExcludeEngines) > 0 {
 		for _, e := range cfg.ExcludeEngines {
-			if strings.ToLower(e) == engine {
+			if strings.EqualFold(e, engine) {
 				return false
 			}
 		}
@@ -179,29 +178,29 @@ func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool {
 	return true
 }
 
-// shouldIncludeAccount checks if an account should be included based on filters
+// shouldIncludeAccount checks if an account should be included based on filters.
 func shouldIncludeAccount(accountName string, cfg Config) bool {
-	// If account name is empty and there are filters, skip it (unless include list is empty)
+	// If account name is empty and there are filters, skip it (unless include list is empty).
 	if accountName == "" {
 		return len(cfg.IncludeAccounts) == 0 && len(cfg.ExcludeAccounts) == 0
 	}
 
 	accountLower := strings.ToLower(accountName)
 
-	// Check include list
+	// Check include list.
 	if !checkIncludeList(accountLower, cfg.IncludeAccounts) {
 		return false
 	}
 
-	// Check exclude list
+	// Check exclude list.
 	if checkExcludeList(accountLower, cfg.ExcludeAccounts) {
 		return false
 	}
 
 	return true
 }
 
-// checkIncludeList checks if an account matches the include filters
+// checkIncludeList checks if an account matches the include filters.
 func checkIncludeList(accountLower string, includeAccounts []string) bool {
 	if len(includeAccounts) == 0 {
 		return true
@@ -216,7 +215,7 @@ func checkIncludeList(accountLower string, includeAccounts []string) bool {
 	return false
 }
 
-// checkExcludeList checks if an account matches any exclude filters
+// checkExcludeList checks if an account matches any exclude filters.
 func checkExcludeList(accountLower string, excludeAccounts []string) bool {
 	for _, filter := range excludeAccounts {
 		if accountMatchesFilter(accountLower, filter) {
@@ -226,7 +225,7 @@ func checkExcludeList(accountLower string, excludeAccounts []string) bool {
 	return false
 }
 
-// accountMatchesFilter checks if an account matches a filter pattern (exact or substring match)
+// accountMatchesFilter checks if an account matches a filter pattern (exact or substring match).
 func accountMatchesFilter(accountLower, filter string) bool {
 	filterLower := strings.ToLower(filter)
 	return filterLower == accountLower || strings.Contains(accountLower, filterLower)

cmd/validators.go

@@ -11,7 +11,7 @@ import (
 	"github.com/spf13/cobra"
 )
 
-// validateFlags performs validation on command line flags before execution
+// validateFlags performs validation on command line flags before execution.
 func validateFlags(cmd *cobra.Command, args []string) error {
 	if err := validateNumericRanges(cmd); err != nil {
 		return err
@@ -69,6 +69,22 @@ func validateNumericRanges(cmd *cobra.Command) error {
 		return fmt.Errorf("override-count (%d) exceeds reasonable limit of %d", toolCfg.OverrideCount, MaxReasonableInstances)
 	}
 
+	if err := validateMinPoolSize(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateMinPoolSize checks that --min-pool-size is a non-negative whole number.
+// Instance counts are integers, so a fractional threshold is nonsensical.
+func validateMinPoolSize() error {
+	if toolCfg.MinPoolSize < 0 {
+		return fmt.Errorf("min-pool-size must be 0 (disabled) or a positive whole number, got: %.2f", toolCfg.MinPoolSize)
+	}
+	if toolCfg.MinPoolSize > 0 && toolCfg.MinPoolSize != float64(int64(toolCfg.MinPoolSize)) {
+		return fmt.Errorf("min-pool-size must be a whole number (instance counts are integers), got: %.2f", toolCfg.MinPoolSize)
+	}
 	return nil
 }
 
@@ -92,7 +108,7 @@ func validateTargetCoverage(cmd *cobra.Command) error {
 	return nil
 }
 
-// validatePaymentAndTerm validates payment options and term configuration
+// validatePaymentAndTerm validates payment options and term configuration.
 func validatePaymentAndTerm() error {
 	// Validate payment option
 	validPaymentOptions := map[string]bool{
@@ -113,7 +129,7 @@ func validatePaymentAndTerm() error {
 	return warnRDS3YearNoUpfront()
 }
 
-// warnRDS3YearNoUpfront warns if RDS service is selected with 3-year no-upfront
+// warnRDS3YearNoUpfront warns if RDS service is selected with 3-year no-upfront.
 func warnRDS3YearNoUpfront() error {
 	// In CSV-input mode the payment option comes from each row, not the
 	// --payment flag (which keeps its no-upfront default), so this flag-based
@@ -138,7 +154,7 @@ func warnRDS3YearNoUpfront() error {
 	return nil
 }
 
-// containsService checks if a service exists in the slice
+// containsService checks if a service exists in the slice.
 func containsService(services []common.ServiceType, service common.ServiceType) bool {
 	for _, svc := range services {
 		if svc == service {
@@ -148,7 +164,7 @@ func containsService(services []common.ServiceType, service common.ServiceType)
 	return false
 }
 
-// validateFilePaths validates CSV input/output paths
+// validateFilePaths validates CSV input/output paths.
 func validateFilePaths() error {
 	// Validate CSV output path if provided
 	if toolCfg.CSVOutput != "" {
@@ -173,7 +189,7 @@ func validateFilePaths() error {
 	return nil
 }
 
-// validateFilterFlags validates filter configuration flags
+// validateFilterFlags validates filter configuration flags.
 func validateFilterFlags() error {
 	// Check for region conflicts
 	if err := validateNoConflicts(toolCfg.IncludeRegions, toolCfg.ExcludeRegions, "region"); err != nil {
@@ -201,7 +217,7 @@ func validateFilterFlags() error {
 	return nil
 }
 
-// validateNoConflicts checks that include and exclude lists don't overlap
+// validateNoConflicts checks that include and exclude lists don't overlap.
 func validateNoConflicts(include, exclude []string, itemType string) error {
 	if len(include) == 0 || len(exclude) == 0 {
 		return nil
@@ -218,7 +234,7 @@ func validateNoConflicts(include, exclude []string, itemType string) error {
 	return nil
 }
 
-// validateInstanceTypes performs basic validation on instance type names
+// validateInstanceTypes performs basic validation on instance type names.
 func validateInstanceTypes(instanceTypes []string) error {
 	if len(instanceTypes) == 0 {
 		return nil

cmd/validators_test.go

@@ -87,6 +87,52 @@ func TestValidateNumericRanges(t *testing.T) {
 			wantErr: true,
 			errMsg:  "override-count",
 		},
+		{
+			name: "min-pool-size negative",
+			setupFunc: func() {
+				toolCfg.Coverage = 80.0
+				toolCfg.MaxInstances = 0
+				toolCfg.OverrideCount = 0
+				toolCfg.CoverageLookbackDays = 30
+				toolCfg.MinPoolSize = -1.0
+			},
+			wantErr: true,
+			errMsg:  "min-pool-size must be 0 (disabled) or a positive whole number",
+		},
+		{
+			name: "min-pool-size fractional rejected",
+			setupFunc: func() {
+				toolCfg.Coverage = 80.0
+				toolCfg.MaxInstances = 0
+				toolCfg.OverrideCount = 0
+				toolCfg.CoverageLookbackDays = 30
+				toolCfg.MinPoolSize = 1.5
+			},
+			wantErr: true,
+			errMsg:  "min-pool-size must be a whole number",
+		},
+		{
+			name: "min-pool-size disabled (zero)",
+			setupFunc: func() {
+				toolCfg.Coverage = 80.0
+				toolCfg.MaxInstances = 0
+				toolCfg.OverrideCount = 0
+				toolCfg.CoverageLookbackDays = 30
+				toolCfg.MinPoolSize = 0
+			},
+			wantErr: false,
+		},
+		{
+			name: "min-pool-size whole number accepted",
+			setupFunc: func() {
+				toolCfg.Coverage = 80.0
+				toolCfg.MaxInstances = 0
+				toolCfg.OverrideCount = 0
+				toolCfg.CoverageLookbackDays = 30
+				toolCfg.MinPoolSize = 2.0
+			},
+			wantErr: false,
+		},
 	}
 
 	for _, tt := range tests {