fix(recfilter): coverage drop reason, EC2 platform engine filter, money scaling on dedupe

pkg/common/drop_summary.go

@@ -11,6 +11,7 @@ import (
 const (
 	DropMinPoolSize           = "--min-pool-size"
 	DropExtendedSupport       = "--include-extended-support"
+	DropCoverageSizedToZero   = "--coverage-sized-to-zero"
 	DropTargetAlreadyMet      = "target-already-met"
 	DropTargetSizedToZero     = "target-sized-to-zero"
 	DropTargetInputInvalid    = "target-input-invalid"

pkg/common/engine.go

@@ -66,3 +66,43 @@ func EngineFromDetails(details ServiceDetails) string {
 	}
 	return NormalizeEngineName(engine)
 }
+
+// platformAliases maps the spellings operators type to the normalized EC2
+// platform. Normalization lowercases and drops the " (amazon vpc)" suffix that
+// Cost Explorer appends, so "Windows (Amazon VPC)" and "windows" agree.
+var platformAliases = map[string]string{
+	"linux": "linux/unix",
+	"unix":  "linux/unix",
+}
+
+// NormalizePlatformName normalizes an EC2 platform for filter comparison.
+func NormalizePlatformName(platform string) string {
+	p := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(platform)), " (amazon vpc)")
+	if alias, ok := platformAliases[p]; ok {
+		return alias
+	}
+	return p
+}
+
+// EngineFilterKey returns the value the --include-engines/--exclude-engines
+// filters compare against, and the normalizer to apply to each filter entry.
+// Databases and caches use their engine; compute (EC2) uses its platform,
+// which the CSV and table label "Engine". EngineFromDetails is deliberately
+// left alone: it feeds the dedupe key and Matches, and existing EC2
+// commitments carry no engine to match a platform against.
+// An empty key means the recommendation has no engine or platform.
+func EngineFilterKey(details ServiceDetails) (key string, normalize func(string) string) {
+	var platform string
+	switch d := details.(type) {
+	case *ComputeDetails:
+		if d == nil {
+			return "", NormalizeEngineName
+		}
+		platform = d.Platform
+	case ComputeDetails:
+		platform = d.Platform
+	default:
+		return EngineFromDetails(details), NormalizeEngineName
+	}
+	return NormalizePlatformName(platform), NormalizePlatformName
+}

pkg/common/resize.go

@@ -0,0 +1,27 @@
+package common //nolint:revive // pkg/common is the module's core domain package (Recommendation, ProviderType, ...), imported by name (common.X) throughout this repo and by consumer-mcp; renaming would break every consumer import
+
+import "fmt"
+
+// ResizeCount returns rec sized to newCount with every money field scaled by
+// newCount/rec.Count (see ScaleRecommendationCosts for what scales). Use it
+// wherever a filter shrinks a recommendation's count, so Count and cost
+// cannot drift apart.
+//
+// It fails loud rather than inventing a per-unit cost: rec.Count must be
+// positive, and newCount must be in [1, rec.Count]. Growing a count is the
+// sizing paths' job (they derive the target from demand) and dropping to zero
+// is the caller's drop decision, so neither is accepted here.
+func ResizeCount(rec Recommendation, newCount int) (Recommendation, error) {
+	if rec.Count <= 0 {
+		return rec, fmt.Errorf("cannot resize %s/%s/%s: source count %d has no per-unit cost", rec.Service, rec.Region, rec.ResourceType, rec.Count)
+	}
+	if newCount < 1 || newCount > rec.Count {
+		return rec, fmt.Errorf("cannot resize %s/%s/%s from %d to %d: new count must be in [1, %d]", rec.Service, rec.Region, rec.ResourceType, rec.Count, newCount, rec.Count)
+	}
+	if newCount == rec.Count {
+		return rec, nil
+	}
+	resized := ScaleRecommendationCosts(rec, float64(newCount)/float64(rec.Count))
+	resized.Count = newCount
+	return resized, nil
+}

pkg/common/resize_test.go

@@ -0,0 +1,65 @@
+package common //nolint:revive // see resize.go
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestResizeCount(t *testing.T) {
+	t.Parallel()
+	monthly := 30.0
+	rec := Recommendation{
+		Count: 3, CommitmentCost: 300, OnDemandCost: 600, EstimatedSavings: 150,
+		SavingsPercentage: 25, BreakEvenMonths: 4, RecommendedCount: 3, RecurringMonthlyCost: &monthly,
+	}
+
+	got, err := ResizeCount(rec, 1)
+
+	require.NoError(t, err)
+	assert.Equal(t, 1, got.Count)
+	assert.InDelta(t, 100.0, got.CommitmentCost, 1e-9)
+	assert.InDelta(t, 200.0, got.OnDemandCost, 1e-9)
+	assert.InDelta(t, 50.0, got.EstimatedSavings, 1e-9)
+	require.NotNil(t, got.RecurringMonthlyCost)
+	assert.InDelta(t, 10.0, *got.RecurringMonthlyCost, 1e-9)
+	assert.Equal(t, 25.0, got.SavingsPercentage)
+	assert.Equal(t, 4.0, got.BreakEvenMonths)
+	assert.Equal(t, 3, got.RecommendedCount, "frozen pre-sizing record")
+	assert.Equal(t, 30.0, monthly, "input pointer untouched")
+}
+
+func TestResizeCount_NilRecurringStaysNil(t *testing.T) {
+	t.Parallel()
+	got, err := ResizeCount(Recommendation{Count: 2, CommitmentCost: 10}, 1)
+	require.NoError(t, err)
+	assert.Nil(t, got.RecurringMonthlyCost)
+}
+
+func TestResizeCount_SameCountIsIdentity(t *testing.T) {
+	t.Parallel()
+	rec := Recommendation{Count: 2, CommitmentCost: 10}
+	got, err := ResizeCount(rec, 2)
+	require.NoError(t, err)
+	assert.Equal(t, rec, got)
+}
+
+func TestResizeCount_RejectsInvalid(t *testing.T) {
+	t.Parallel()
+	for name, tc := range map[string]struct {
+		rec Recommendation
+		n   int
+	}{
+		"zero source count": {Recommendation{Count: 0, CommitmentCost: 10}, 1},
+		"zero target":       {Recommendation{Count: 2}, 0},
+		"negative target":   {Recommendation{Count: 2}, -1},
+		"growing":           {Recommendation{Count: 2}, 3},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			_, err := ResizeCount(tc.rec, tc.n)
+			assert.Error(t, err)
+		})
+	}
+}

pkg/recfilter/dedupe.go

@@ -65,7 +65,10 @@ func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context,
 	existingMap := buildExistingCommitmentsMap(recentExisting, d.Logf)
 	d.Logf.printf("    [DuplicateChecker] Existing map has %d unique keys", len(existingMap))
 
-	passed, filtered = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf)
+	passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf)
+	if err != nil {
+		return nil, nil, err
+	}
 
 	if len(filtered) > 0 {
 		d.Logf.printf("    [DuplicateChecker] Result: %d recommendations kept out of %d (avoided %d duplicates)",
@@ -212,27 +215,31 @@ func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map
 
 // adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments.
 // Returns (passed, filtered) where filtered contains recs whose count was reduced to zero.
-func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, logf Logf) (passed, filtered []common.Recommendation) {
+func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, logf Logf) (passed, filtered []common.Recommendation, err error) {
 	passed = make([]common.Recommendation, 0, len(recs))
 	filtered = make([]common.Recommendation, 0)
 
 	for _rvc := range recs {
 		rec := recs[_rvc]
-		adjusted := adjustSingleRecommendation(rec, existingMap, logf)
+		adjusted, err := adjustSingleRecommendation(rec, existingMap, logf)
+		if err != nil {
+			return nil, nil, err
+		}
 		if adjusted.Count > 0 {
 			passed = append(passed, adjusted)
 		} else {
 			filtered = append(filtered, rec)
 		}
 	}
 
-	return passed, filtered
+	return passed, filtered, nil
 }
 
-// adjustSingleRecommendation adjusts a single recommendation based on existing commitments.
-func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) common.Recommendation {
+// adjustSingleRecommendation adjusts a single recommendation based on existing
+// commitments. A reduced count scales the money fields with it.
+func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) (common.Recommendation, error) {
 	if rec.Count <= 0 {
-		return common.Recommendation{Count: 0}
+		return common.Recommendation{Count: 0}, nil
 	}
 	engine, isElastiCache := dedupeEngine(rec.Provider, rec.Service, common.EngineFromDetails(rec.Details))
 	deployment := common.NormalizeDeploymentName(common.DeploymentFromDetails(rec.Details))
@@ -254,16 +261,18 @@ func adjustSingleRecommendation(rec common.Recommendation, existingMap map[strin
 
 	if remaining <= 0 {
 		logf.printf("    [DuplicateChecker] SKIP %s: recent commitments cover recommended %d", key, rec.Count)
-		return common.Recommendation{Count: 0}
+		return common.Recommendation{Count: 0}, nil
 	}
 
-	adjusted := rec
+	adjusted, err := common.ResizeCount(rec, remaining)
+	if err != nil {
+		return common.Recommendation{}, err
+	}
 	if remaining < rec.Count {
-		adjusted.Count = remaining
 		logf.printf("    [DuplicateChecker] PARTIAL %s: adjusted count from %d to %d", key, rec.Count, adjusted.Count)
 	}
 
-	return adjusted
+	return adjusted, nil
 }
 
 // AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting.

pkg/recfilter/dedupe_test.go

@@ -601,3 +601,31 @@ func TestFilterRecentCommitments_EndedStatesIgnored(t *testing.T) {
 
 	assert.Empty(t, NewDuplicateChecker(DefaultDuplicateCheckLookbackHours).filterRecentCommitments(commitments))
 }
+
+// A partially covered recommendation keeps its money in step with Count.
+func TestAdjustRecommendationsForExisting_PartialCoverageScalesMoney(t *testing.T) {
+	t.Parallel()
+	client := &fakeServiceClient{commitments: []common.Commitment{
+		{ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 6, State: "active", StartDate: time.Now().Add(-1 * time.Hour)},
+	}}
+	monthly := 100.0
+	recs := []common.Recommendation{{
+		ResourceType: "db.t3.small", Region: "us-east-1", Count: 10,
+		CommitmentCost: 1000, OnDemandCost: 2000, EstimatedSavings: 500, SavingsPercentage: 25,
+		RecurringMonthlyCost: &monthly,
+		Details:              &common.DatabaseDetails{Engine: "mysql"},
+	}}
+
+	passed, _, err := NewDuplicateChecker(0).AdjustRecommendationsForExisting(context.Background(), recs, client)
+
+	require.NoError(t, err)
+	require.Len(t, passed, 1)
+	assert.Equal(t, 4, passed[0].Count)
+	assert.InDelta(t, 400.0, passed[0].CommitmentCost, 1e-9)
+	assert.InDelta(t, 800.0, passed[0].OnDemandCost, 1e-9)
+	assert.InDelta(t, 200.0, passed[0].EstimatedSavings, 1e-9)
+	assert.Equal(t, 25.0, passed[0].SavingsPercentage, "ratio is invariant")
+	require.NotNil(t, passed[0].RecurringMonthlyCost)
+	assert.InDelta(t, 40.0, *passed[0].RecurringMonthlyCost, 1e-9)
+	assert.Equal(t, 100.0, monthly, "caller's pointer is not mutated")
+}

pkg/recfilter/filters.go

@@ -75,25 +75,29 @@ func (f Filters) IncludesInstanceType(instanceType string) bool {
 // recommendation whose engine normalizes to "postgresql". NormalizeEngineName
 // lowercases anything it does not recognize, so unknown engines keep the
 // case-insensitive matching this had before.
+//
+// EC2 recommendations have no database engine; the filters match their
+// Platform (the value the CSV shows as "Engine"), so "Windows (Amazon VPC)",
+// "windows" and "WINDOWS" are equivalent (see common.EngineFilterKey).
 func (f Filters) IncludesEngine(rec *common.Recommendation) bool {
-	engine := common.EngineFromDetails(rec.Details)
+	engine, normalize := common.EngineFilterKey(rec.Details)
 	if engine == "" {
 		// No engine info: include by default unless there's an include list.
 		return len(f.IncludeEngines) == 0
 	}
 	engine = strings.ToLower(engine)
 
-	if len(f.IncludeEngines) > 0 && !matchesEngine(f.IncludeEngines, engine) {
+	if len(f.IncludeEngines) > 0 && !matchesEngine(f.IncludeEngines, engine, normalize) {
 		return false
 	}
-	return !matchesEngine(f.ExcludeEngines, engine)
+	return !matchesEngine(f.ExcludeEngines, engine, normalize)
 }
 
 // matchesEngine reports whether any filter entry normalizes to engine, which
 // the caller has already normalized.
-func matchesEngine(filters []string, engine string) bool {
+func matchesEngine(filters []string, engine string, normalize func(string) string) bool {
 	for _, e := range filters {
-		if common.NormalizeEngineName(e) == engine {
+		if normalize(e) == engine {
 			return true
 		}
 	}

pkg/recfilter/filters_test.go

@@ -215,3 +215,45 @@ func TestDimensionPredicates_IgnoreAccount(t *testing.T) {
 	assert.True(t, f.IncludesInstanceType(rec.ResourceType))
 	assert.True(t, f.IncludesEngine(&rec))
 }
+
+func TestIncludesEngine_EC2PlatformIsTheEngine(t *testing.T) {
+	t.Parallel()
+	rec := func(platform string) *common.Recommendation {
+		return &common.Recommendation{Details: &common.ComputeDetails{Platform: platform}}
+	}
+	tests := []struct {
+		name     string
+		filters  Filters
+		platform string
+		want     bool
+	}{
+		{"include displayed value", Filters{IncludeEngines: []string{"Windows (Amazon VPC)"}}, "Windows (Amazon VPC)", true},
+		{"include bare name", Filters{IncludeEngines: []string{"windows"}}, "Windows (Amazon VPC)", true},
+		{"include is case-insensitive", Filters{IncludeEngines: []string{"WINDOWS"}}, "Windows", true},
+		{"include other platform", Filters{IncludeEngines: []string{"windows"}}, "Linux/UNIX", false},
+		{"exclude windows", Filters{ExcludeEngines: []string{"windows"}}, "Windows (Amazon VPC)", false},
+		{"exclude windows keeps linux", Filters{ExcludeEngines: []string{"windows"}}, "Linux/UNIX", true},
+		{"linux alias", Filters{IncludeEngines: []string{"linux"}}, "Linux/UNIX (Amazon VPC)", true},
+		{"windows does not match SQL Server variant", Filters{ExcludeEngines: []string{"windows"}}, "Windows with SQL Server", true},
+		{"empty platform with include list", Filters{IncludeEngines: []string{"windows"}}, "", false},
+		{"empty platform with exclude list", Filters{ExcludeEngines: []string{"windows"}}, "", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tt.want, tt.filters.IncludesEngine(rec(tt.platform)))
+		})
+	}
+
+	t.Run("value-typed ComputeDetails", func(t *testing.T) {
+		t.Parallel()
+		r := &common.Recommendation{Details: common.ComputeDetails{Platform: "Windows"}}
+		assert.False(t, Filters{ExcludeEngines: []string{"windows"}}.IncludesEngine(r))
+	})
+	t.Run("typed nil ComputeDetails does not panic", func(t *testing.T) {
+		t.Parallel()
+		var d *common.ComputeDetails
+		r := &common.Recommendation{Details: d}
+		assert.NotPanics(t, func() { Filters{ExcludeEngines: []string{"windows"}}.IncludesEngine(r) })
+	})
+}

pkg/recfilter/sizing.go

@@ -20,11 +20,17 @@ import (
 // drops accumulates per-reason drop counts for the end-of-run summary; pass
 // nil to skip tracking. logf receives WARNING lines for anomalous recs
 // (nil-safe; pass nil to disable logging).
+//
+// RI counts are rounded DOWN (int(Count*coverage/100)): coverage is an upper
+// bound on what is bought, and the discrete ratio keeps Count and money in
+// step. A rec whose count rounds to zero is dropped under
+// common.DropCoverageSizedToZero, distinct from the --target-coverage reasons.
 func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, drops *common.DropSummary) []common.Recommendation {
 	if coverage >= 100 {
 		return recs
 	}
 	if coverage <= 0 {
+		drops.Add(common.DropCoverageSizedToZero, len(recs))
 		return []common.Recommendation{}
 	}
 
@@ -73,8 +79,9 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, dr
 			result = append(result, adjusted)
 		} else {
 			// No nil guard: DropSummary.Add is nil-receiver safe, and every
-			// other drop site in this package relies on that.
-			drops.Add(common.DropTargetSizedToZero, 1)
+			// other drop site in this package relies on that. This is the
+			// --coverage percentage floor, not --target-coverage sizing.
+			drops.Add(common.DropCoverageSizedToZero, 1)
 		}
 	}
 	return result

pkg/recfilter/sizing_test.go

@@ -137,7 +137,18 @@ func TestApplyCoverage_RISizedToZero_DropsAndRecords(t *testing.T) {
 		out := ApplyCoverage([]common.Recommendation{rec}, 10, nil, d)
 		assert.Empty(t, out)
 		assert.Equal(t, 1, d.Total())
-		assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero)
+		assert.Contains(t, d.FormatOneLine(), common.DropCoverageSizedToZero)
+		assert.NotContains(t, d.FormatOneLine(), common.DropTargetSizedToZero,
+			"a --coverage drop must not be attributed to --target-coverage")
+	})
+
+	t.Run("coverage zero drops all and records them", func(t *testing.T) {
+		t.Parallel()
+		d := common.NewDropSummary()
+		out := ApplyCoverage([]common.Recommendation{rec, rec}, 0, nil, d)
+		assert.Empty(t, out)
+		assert.Equal(t, 2, d.Total())
+		assert.Contains(t, d.FormatOneLine(), common.DropCoverageSizedToZero)
 	})
 
 	t.Run("with nil drops", func(t *testing.T) {