refactor(mcp): extract CLI filter/sizing/dedupe logic into pkg/recfilter (T1,T3-T6)

cmd/helpers_test.go

@@ -583,7 +583,7 @@ func TestNormalizeEngineName(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.input, func(t *testing.T) {
-			result := normalizeEngineName(tt.input)
+			result := common.NormalizeEngineName(tt.input)
 			assert.Equal(t, tt.expected, result)
 		})
 	}
@@ -649,7 +649,7 @@ func TestGetEngineFromRecommendation(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := getEngineFromRecommendation(tt.rec)
+			result := common.EngineFromDetails(tt.rec.Details)
 			assert.Equal(t, tt.expected, result)
 		})
 	}

cmd/multi_service_filters.go

@@ -1,45 +1,46 @@
 package main
 
 import (
-	"fmt"
 	"log"
-	"slices"
 	"strings"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
+	"github.com/LeanerCloud/CUDly/pkg/recfilter"
 	awsprovider "github.com/LeanerCloud/CUDly/providers/aws"
 )
 
+// filtersFromConfig maps the CLI Config's dimension-filter and min-pool-size
+// fields onto a recfilter.Filters value. Account filtering stays cmd-only
+// (see shouldIncludeAccount) so it is not part of recfilter.Filters.
+func filtersFromConfig(cfg *Config) recfilter.Filters {
+	return recfilter.Filters{
+		IncludeRegions:       cfg.IncludeRegions,
+		ExcludeRegions:       cfg.ExcludeRegions,
+		IncludeInstanceTypes: cfg.IncludeInstanceTypes,
+		ExcludeInstanceTypes: cfg.ExcludeInstanceTypes,
+		IncludeEngines:       cfg.IncludeEngines,
+		ExcludeEngines:       cfg.ExcludeEngines,
+		MinPoolSize:          cfg.MinPoolSize,
+	}
+}
+
 // 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.
 // drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip tracking.
 func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []common.Recommendation {
-	var filtered []common.Recommendation
-	var poolDropCount int
-	var poolDropInstances float64
+	survivors := filtersFromConfig(cfg).ApplyMinPoolSize(recs, log.Printf, drops)
 
-	for i := range recs {
-		if cfg.MinPoolSize > 0 && !shouldIncludePoolSize(&recs[i], cfg) {
-			poolDropInstances += recs[i].AverageInstancesUsedPerHour
-			label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType)
-			log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour)
-			poolDropCount++
-			drops.Add(common.DropMinPoolSize, 1)
-			continue
-		}
-		adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion)
+	var filtered []common.Recommendation
+	for i := range survivors {
+		adjusted, include, dropReason := processRecommendation(&survivors[i], cfg, instanceVersions, versionInfo, currentRegion)
 		if include {
 			filtered = append(filtered, adjusted)
 		} else if dropReason != "" {
 			drops.Add(dropReason, 1)
 		}
 	}
 
-	if poolDropCount > 0 {
-		log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances)
-	}
-
 	return filtered
 }
 
@@ -86,6 +87,11 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers
 // each function's cyclomatic complexity under the gocyclo limit; the
 // dimension filters here are pure functions of rec + cfg with no side
 // effects. Pool-size filtering is handled with logging in applyFilters.
+//
+// Region and account stay here rather than moving into recfilter: the
+// region-agnostic handling (#1881) needs providers/aws, which the pkg module
+// cannot import, and account filtering is name-substring matching backed by
+// AccountAliasCache. Only the instance-type and engine checks are portable.
 func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool {
 	if !shouldIncludeRecommendationRegion(rec, cfg) {
 		return false
@@ -96,31 +102,14 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool {
 	if !shouldIncludeEngine(rec, cfg) {
 		return false
 	}
-	if !shouldIncludeAccount(rec.AccountName, cfg) {
-		return false
-	}
-	return true
+	return shouldIncludeAccount(rec.AccountName, cfg)
 }
 
-// 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
-// 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).
-// Those pools aren't sized via the per-hour formula so the filter doesn't
-// apply to them.
+// shouldIncludePoolSize checks if a recommendation's pool size meets cfg.MinPoolSize.
+// Thin wrapper over recfilter.Filters.IncludesPoolSize; kept so cmd's existing call sites
+// and tests are unchanged.
 func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool {
-	if cfg.MinPoolSize <= 0 {
-		return true
-	}
-	if rec.AverageInstancesUsedPerHour <= 0 {
-		return true
-	}
-	return rec.AverageInstancesUsedPerHour >= cfg.MinPoolSize
+	return filtersFromConfig(cfg).IncludesPoolSize(rec)
 }
 
 // shouldIncludeRecommendationRegion applies the region filters to a whole
@@ -143,71 +132,24 @@ func shouldIncludeRecommendationRegion(rec *common.Recommendation, cfg *Config)
 }
 
 // shouldIncludeRegion checks if a region should be included based on filters.
+// Thin wrapper over recfilter.Filters.IncludesRegion; kept so cmd's existing call sites
+// and tests are unchanged.
 func shouldIncludeRegion(region string, cfg *Config) bool {
-	// 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 slices.Contains(cfg.ExcludeRegions, region) {
-		return false
-	}
-
-	return true
+	return filtersFromConfig(cfg).IncludesRegion(region)
 }
 
 // shouldIncludeInstanceType checks if an instance type should be included based on filters.
+// Thin wrapper over recfilter.Filters.IncludesInstanceType; kept so cmd's existing call sites
+// and tests are unchanged.
 func shouldIncludeInstanceType(instanceType string, cfg *Config) bool {
-	// 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 slices.Contains(cfg.ExcludeInstanceTypes, instanceType) {
-		return false
-	}
-
-	return true
+	return filtersFromConfig(cfg).IncludesInstanceType(instanceType)
 }
 
 // shouldIncludeEngine checks if a recommendation should be included based on engine filters.
+// Thin wrapper over recfilter.Filters.IncludesEngine; kept so cmd's existing call sites
+// and tests are unchanged.
 func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool {
-	// Extract engine from recommendation.
-	engine := getEngineFromRecommendation(*rec)
-	if engine == "" {
-		// 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.
-	engine = strings.ToLower(engine)
-
-	// If include list is specified, engine must be in it.
-	if len(cfg.IncludeEngines) > 0 {
-		found := false
-		for _, e := range cfg.IncludeEngines {
-			if strings.EqualFold(e, engine) {
-				found = true
-				break
-			}
-		}
-		if !found {
-			return false
-		}
-	}
-
-	// If exclude list is specified, engine must not be in it.
-	if len(cfg.ExcludeEngines) > 0 {
-		for _, e := range cfg.ExcludeEngines {
-			if strings.EqualFold(e, engine) {
-				return false
-			}
-		}
-	}
-
-	return true
+	return filtersFromConfig(cfg).IncludesEngine(rec)
 }
 
 // shouldIncludeAccount checks if an account should be included based on filters.

cmd/multi_service_filters_test.go

@@ -1,11 +1,14 @@
 package main
 
 import (
+	"fmt"
+	"log"
 	"testing"
 	"time"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestApplyFilters(t *testing.T) {
@@ -629,3 +632,124 @@ func TestApplyFilters_SavingsPlansRegionFilters(t *testing.T) {
 		})
 	}
 }
+
+// applyFiltersPreExtraction is the pre-refactor applyFilters implementation,
+// kept verbatim (from origin/main, before the recfilter.Filters.ApplyMinPoolSize
+// extraction) as a differential oracle. It applies the --min-pool-size check
+// inline, in the same loop and same order as processRecommendation, rather
+// than as a separate pass over the full slice.
+func applyFiltersPreExtraction(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []common.Recommendation {
+	var filtered []common.Recommendation
+	var poolDropCount int
+	var poolDropInstances float64
+
+	for i := range recs {
+		if cfg.MinPoolSize > 0 && !shouldIncludePoolSize(&recs[i], cfg) {
+			poolDropInstances += recs[i].AverageInstancesUsedPerHour
+			label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType)
+			log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour)
+			poolDropCount++
+			drops.Add(common.DropMinPoolSize, 1)
+			continue
+		}
+		adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion)
+		if include {
+			filtered = append(filtered, adjusted)
+		} else if dropReason != "" {
+			drops.Add(dropReason, 1)
+		}
+	}
+
+	if poolDropCount > 0 {
+		log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances)
+	}
+
+	return filtered
+}
+
+// TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour is a
+// differential regression test against a reviewer's claim that extracting
+// the --min-pool-size check into recfilter.Filters.ApplyMinPoolSize changed
+// its ordering relative to the currentRegion guard in processRecommendation
+// (inflating common.DropMinPoolSize in multi-region runs). It runs the
+// current applyFilters and the pre-extraction oracle above side by side,
+// once per region, over the same multi-region recommendation set, and
+// asserts both the survivors and the drop accounting match exactly.
+func TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour(t *testing.T) {
+	origCfg := toolCfg
+	defer func() { toolCfg = origCfg }()
+
+	toolCfg = Config{MinPoolSize: 2.0}
+
+	regions := []string{"us-east-1", "eu-west-1", "ap-southeast-1"}
+	const distinctBelowThreshold = 3 // one below-threshold rec per region below
+
+	// Fresh copies per call: applyFilters mutates nothing in place today, but
+	// the oracle and the refactored code must each see their own slice so a
+	// hypothetical future in-place adjustment on one side can't leak into the
+	// other's input and mask a real divergence.
+	makeRecs := func() []common.Recommendation {
+		return []common.Recommendation{
+			{Region: "us-east-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0},
+			{Region: "us-east-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold
+			{Region: "eu-west-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0},
+			{Region: "eu-west-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold
+			{Region: "ap-southeast-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0},
+			{Region: "ap-southeast-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold
+			{Region: "us-east-1", ResourceType: "db.t3.large", Count: 3, AverageInstancesUsedPerHour: 0},        // no-signal passthrough
+			{
+				Service:                     common.ServiceSavingsPlansCompute,
+				ResourceType:                "ec2-instance",
+				Count:                       10,
+				AverageInstancesUsedPerHour: 5.0, // account-level: bypasses the currentRegion guard
+			},
+		}
+	}
+
+	var totalDropMinPoolSizeNew, totalDropMinPoolSizeOld int
+
+	for _, region := range regions {
+		t.Run(region, func(t *testing.T) {
+			dropsNew := common.NewDropSummary()
+			dropsOld := common.NewDropSummary()
+
+			resultNew := applyFilters(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsNew)
+			resultOld := applyFiltersPreExtraction(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsOld)
+
+			assert.Equal(t, resultOld, resultNew,
+				"region %s: refactored applyFilters diverged from the pre-extraction oracle -- the ApplyMinPoolSize extraction changed observable CLI filtering behavior", region)
+			assert.Equal(t, dropsOld.FormatOneLine(), dropsNew.FormatOneLine(),
+				"region %s: drop summaries diverged between pre- and post-extraction implementations", region)
+			assert.Equal(t, dropsOld.Total(), dropsNew.Total(),
+				"region %s: total drop counts diverged between pre- and post-extraction implementations", region)
+
+			// The fixture is built so --min-pool-size is the ONLY drop reason
+			// either implementation can record, which is what lets the summed
+			// Total() below stand in for the min-pool count specifically.
+			// Asserting the exact per-pass count keeps that guarantee honest:
+			// if some other filter started dropping rows, Total() would exceed
+			// the below-threshold count and this would fail.
+			require.Contains(t, dropsNew.FormatOneLine(), common.DropMinPoolSize,
+				"region %s: fixture must exercise the --min-pool-size drop path", region)
+			require.Equal(t, distinctBelowThreshold, dropsNew.Total(),
+				"region %s: --min-pool-size must be the only drop reason this fixture records", region)
+
+			totalDropMinPoolSizeNew += dropsNew.Total()
+			totalDropMinPoolSizeOld += dropsOld.Total()
+		})
+	}
+
+	require.Equal(t, totalDropMinPoolSizeOld, totalDropMinPoolSizeNew,
+		"old and new implementations must agree on the summed multi-region --min-pool-size drop count")
+
+	// Pre-existing behavior, identical in both implementations (not introduced
+	// by the ApplyMinPoolSize extraction): each per-region call re-scans the
+	// FULL recommendation set passed to it, not a per-region subset, so a
+	// below-threshold recommendation from one region is re-counted as dropped
+	// during every other region's pass too. Summed across N region passes this
+	// is distinctBelowThreshold * N, not distinctBelowThreshold. This assertion
+	// pins today's (inflated) value rather than an aspirational deduplicated
+	// one -- see the test's summary report for the actual vs. distinct counts.
+	assert.Equal(t, distinctBelowThreshold*len(regions), totalDropMinPoolSizeNew,
+		"summed multi-region --min-pool-size drop count should match today's pre-existing inflation factor")
+}

pkg/common/audit.go

@@ -37,6 +37,16 @@ func WriteAuditRecord(record AuditRecord, path string) error {
 	return nil
 }
 
+// CheckAuditLogWritable opens the audit log file in append mode to verify it is writable.
+// Returns an error if the path cannot be opened for writing.
+func CheckAuditLogWritable(path string) error {
+	f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) // #nosec G302,G304 -- 0644 intentionally matches WriteAuditRecord; path is operator-configured.
+	if err != nil {
+		return fmt.Errorf("audit log %q not writable: %w", path, err)
+	}
+	return f.Close()
+}
+
 // NewAuditRecord constructs an AuditRecord from a Recommendation and a PurchaseResult.
 // status must be one of: "success", "error", "skipped" (dry-run), "skipped_covered" (idempotency).
 // source is the CUDly surface that triggered the run — copied into the JSONL so CLI