From 9a31c8c5923950b3edf88849b5105ae1b3d2ba8a Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Tue, 18 Aug 2026 10:36:52 +0200
Subject: [PATCH 1/5] fix(cli): rescale count-derived money when
 --max-instances truncates a row

ApplyInstanceLimit cut a recommendation's Count to fit the --max-instances
budget but copied the struct wholesale, so every whole-row money figure
survived at its full pre-cap value. A row entering with Count=100 and
EstimatedSavings=600 left as Count=10 with EstimatedSavings=600, a 10x
overstatement for that row.

The figure flows into the run summary, the purchase report and the CSV, so
an operator saw a capped run promising savings it cannot deliver. Affects
both the default and the --input-csv path.

Truncated rows are now scaled by the discrete count ratio via
common.ScaleRecommendationCosts, the helper every other sizing path
(ApplyCoverage, ApplyTargetCoverage, family-NU) already used. Only the
extensive fields scale: SavingsPercentage and BreakEvenMonths are ratios of
figures that scale together, RecommendedCount is a frozen record of the
provider's proposal, and the usage signals describe observed demand rather
than what we buy. A nil RecurringMonthlyCost stays nil, since absent is not
the same claim as zero. A non-positive Count can never enter the truncation
branch, so the ratio never divides by a zero or negative denominator.

ScaleRecommendationCosts now also scales SavingsPlanDetails.HourlyCommitment,
which was duplicated at its two existing call sites and missing at this new
third one. An SP commitment is dollar-denominated, and --override-count sets
Count on SP recs without discriminating by commitment type, so an SP can
reach the cap at a truncatable count; scaling its costs while leaving its
commitment whole would produce an internally inconsistent row.

Closes #1830
---
 cmd/helpers.go                             |  30 ++-
 cmd/helpers_instance_limit_rescale_test.go | 220 +++++++++++++++++++++
 cmd/multi_service_csv_cap.go               |  12 +-
 cmd/multi_service_max_instances_test.go    |   6 +-
 pkg/common/types.go                        |  36 +++-
 5 files changed, 284 insertions(+), 20 deletions(-)
 create mode 100644 cmd/helpers_instance_limit_rescale_test.go

diff --git a/cmd/helpers.go b/cmd/helpers.go
index 0aae0c12b..762211ddd 100644
--- a/cmd/helpers.go
+++ b/cmd/helpers.go
@@ -149,11 +149,10 @@ func applyCoverage(recs []common.Recommendation, coverage float64, drops *common
 		// missing-Details record is a logged anomaly, not a reason to
 		// erase coverage from the run.
 		if common.IsSavingsPlan(rec.Service) {
-			if details, ok := rec.Details.(*common.SavingsPlanDetails); ok {
-				newDetails := *details // Copy the struct
-				newDetails.HourlyCommitment *= ratio
+			if _, ok := rec.Details.(*common.SavingsPlanDetails); ok {
+				// ScaleRecommendationCosts scales HourlyCommitment along with
+				// the cost fields and replaces Details with a scaled copy.
 				adjusted = common.ScaleRecommendationCosts(adjusted, ratio)
-				adjusted.Details = &newDetails
 			} else {
 				AppLogger.Printf("WARNING: SP recommendation for service %q has unexpected Details type %T; passing through unscaled\n", rec.Service, rec.Details)
 			}
@@ -448,16 +447,14 @@ func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common
 	// leaving ProjectedUtilization at zero. Setting projection fields on a
 	// rec whose commitment fields couldn't be scaled would produce a
 	// misleading row (projection=target%, savings=full-unscaled).
-	details, ok := rec.Details.(*common.SavingsPlanDetails)
-	if !ok {
+	if _, ok := rec.Details.(*common.SavingsPlanDetails); !ok {
 		AppLogger.Printf("WARNING: SP recommendation for service %q has unexpected Details type %T; passing through unscaled\n", rec.Service, rec.Details)
 		return rec, true
 	}
 	ratio := targetPct / 100.0
-	newDetails := *details // copy
-	newDetails.HourlyCommitment *= ratio
+	// ScaleRecommendationCosts scales HourlyCommitment along with the cost
+	// fields and replaces Details with a scaled copy.
 	adjusted := common.ScaleRecommendationCosts(rec, ratio)
-	adjusted.Details = &newDetails
 	// Shrinking commitment raises projected utilization by 1/ratio
 	// (used is fixed = orig_commit * RecUtil, bought is orig_commit * ratio).
 	// Clamp to 100 since utilization caps at full use.
@@ -511,6 +508,15 @@ func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []com
 // Recommendations are consumed in slice order, so the caller controls which
 // ones survive by ordering the slice (the main path caps the scorer's
 // savings-sorted output, keeping the highest-value commitments).
+//
+// A truncated recommendation has its extensive money fields scaled by the
+// discrete count ratio, like every other sizing path (see
+// common.ScaleRecommendationCosts). EstimatedSavings and friends are
+// whole-row totals for the count the provider proposed, so cutting Count
+// alone leaves the row claiming the savings of instances the run will not
+// buy. The run summary and the purchase report then overstate the benefit
+// of a capped run, which is the wrong direction to be wrong in on a money
+// path (#1830).
 func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation {
 	if maxInstances <= 0 {
 		return recs
@@ -525,7 +531,13 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm
 			break
 		}
 		adjusted := rec
+		// rec.Count > remaining and remaining >= 1 together imply rec.Count
+		// >= 2, so the denominator is always positive here. A non-positive
+		// Count can never enter this branch and so is never rescaled: it
+		// buys nothing, there is nothing to scale down to, and a zero or
+		// negative denominator would produce NaN or a sign flip.
 		if rec.Count > remaining {
+			adjusted = common.ScaleRecommendationCosts(rec, float64(remaining)/float64(rec.Count))
 			adjusted.Count = remaining
 		}
 		result = append(result, adjusted)
diff --git a/cmd/helpers_instance_limit_rescale_test.go b/cmd/helpers_instance_limit_rescale_test.go
new file mode 100644
index 000000000..d95fbbd9a
--- /dev/null
+++ b/cmd/helpers_instance_limit_rescale_test.go
@@ -0,0 +1,220 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/LeanerCloud/CUDly/pkg/common"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// float64Ptr is local to this file so the rescale tests can express "the
+// provider returned a monthly breakdown" without borrowing a helper whose
+// nil-vs-zero semantics might change elsewhere.
+func float64Ptr(v float64) *float64 { return &v }
+
+// TestApplyInstanceLimitRescalesTruncatedRow pins the #1830 invariant: a row
+// the cap truncates from N to M carries M/N of the money it entered with.
+//
+// The assertion is the ratio, not a literal figure, so the test keeps
+// meaning if the fixture's dollar values are ever changed.
+func TestApplyInstanceLimitRescalesTruncatedRow(t *testing.T) {
+	const (
+		origCount = 100
+		maxKept   = 10
+		ratio     = float64(maxKept) / float64(origCount)
+	)
+
+	rec := common.Recommendation{
+		Service:              common.ServiceEC2,
+		Region:               "us-east-1",
+		ResourceType:         "m5.large",
+		Count:                origCount,
+		EstimatedSavings:     600,
+		CommitmentCost:       2400,
+		OnDemandCost:         3000,
+		SavingsPercentage:    20,
+		RecurringMonthlyCost: float64Ptr(200),
+	}
+
+	got := ApplyInstanceLimit([]common.Recommendation{rec}, maxKept)
+
+	require.Len(t, got, 1)
+	require.Equal(t, maxKept, got[0].Count, "the cap must truncate Count to the budget")
+
+	assert.InDelta(t, rec.EstimatedSavings*ratio, got[0].EstimatedSavings, 0.0001,
+		"EstimatedSavings is a whole-row total and must scale with the truncated Count")
+	assert.InDelta(t, rec.CommitmentCost*ratio, got[0].CommitmentCost, 0.0001,
+		"CommitmentCost is a whole-row total and must scale with the truncated Count")
+	assert.InDelta(t, rec.OnDemandCost*ratio, got[0].OnDemandCost, 0.0001,
+		"OnDemandCost is a whole-row total and must scale with the truncated Count")
+
+	require.NotNil(t, got[0].RecurringMonthlyCost,
+		"a present monthly breakdown must stay present after truncation")
+	assert.InDelta(t, *rec.RecurringMonthlyCost*ratio, *got[0].RecurringMonthlyCost, 0.0001,
+		"RecurringMonthlyCost is a whole-row total and must scale with the truncated Count")
+
+	// SavingsPercentage is a ratio of two figures that scale together, so it
+	// is invariant under truncation. Scaling it would be the mirror-image bug.
+	assert.InDelta(t, rec.SavingsPercentage, got[0].SavingsPercentage, 0.0001,
+		"SavingsPercentage is intensive and must not be scaled")
+
+	// The caller's rec must not be mutated: RecurringMonthlyCost is a pointer
+	// and a shared target would corrupt the pre-cap slice the reporter diffs
+	// against.
+	assert.Equal(t, origCount, rec.Count, "the input rec must not be mutated")
+	assert.InDelta(t, 600.0, rec.EstimatedSavings, 0.0001, "the input rec must not be mutated")
+	require.NotNil(t, rec.RecurringMonthlyCost)
+	assert.InDelta(t, 200.0, *rec.RecurringMonthlyCost, 0.0001,
+		"the input rec's pointer target must not be mutated")
+}
+
+// TestApplyInstanceLimitLeavesUntruncatedRowsUntouched is the other direction:
+// a fix that rescaled every row would satisfy the truncation test above while
+// silently shrinking rows that fit inside the budget.
+func TestApplyInstanceLimitLeavesUntruncatedRowsUntouched(t *testing.T) {
+	monthly := float64Ptr(200)
+	rec := common.Recommendation{
+		Service:              common.ServiceEC2,
+		Region:               "us-east-1",
+		ResourceType:         "m5.large",
+		Count:                4,
+		EstimatedSavings:     600,
+		CommitmentCost:       2400,
+		OnDemandCost:         3000,
+		SavingsPercentage:    20,
+		RecurringMonthlyCost: monthly,
+	}
+
+	// A budget strictly larger than the row's Count, so the row fits whole.
+	got := ApplyInstanceLimit([]common.Recommendation{rec}, 10)
+
+	require.Len(t, got, 1)
+	assert.Equal(t, 4, got[0].Count, "a row that fits the budget keeps its Count")
+	assert.InDelta(t, 600.0, got[0].EstimatedSavings, 0.0001,
+		"an untruncated row must keep its savings; rescaling everything is the mirror-image bug")
+	assert.InDelta(t, 2400.0, got[0].CommitmentCost, 0.0001, "an untruncated row must keep its commitment cost")
+	assert.InDelta(t, 3000.0, got[0].OnDemandCost, 0.0001, "an untruncated row must keep its on-demand cost")
+	require.NotNil(t, got[0].RecurringMonthlyCost)
+	assert.InDelta(t, 200.0, *got[0].RecurringMonthlyCost, 0.0001,
+		"an untruncated row must keep its monthly cost")
+}
+
+// TestApplyInstanceLimitPreservesNilRecurringMonthlyCost pins the
+// absent-versus-zero rule: nil means "the provider returned no monthly
+// breakdown" and the frontend renders it as "—". Truncation must not turn
+// that into a confident $0.
+func TestApplyInstanceLimitPreservesNilRecurringMonthlyCost(t *testing.T) {
+	recs := []common.Recommendation{
+		{
+			Service:              common.ServiceEC2,
+			Region:               "us-east-1",
+			ResourceType:         "truncated",
+			Count:                100,
+			EstimatedSavings:     600,
+			RecurringMonthlyCost: nil,
+		},
+	}
+
+	got := ApplyInstanceLimit(recs, 10)
+
+	require.Len(t, got, 1)
+	require.Equal(t, 10, got[0].Count)
+	assert.Nil(t, got[0].RecurringMonthlyCost,
+		"a nil monthly cost means absent, not zero, and must survive truncation as nil")
+}
+
+// TestApplyInstanceLimitNonPositiveCountIsNotRescaled guards the divide-by-zero
+// denominator. A Count of 0 or below buys nothing and cannot be truncated, so
+// it must pass through with its money untouched rather than through a ratio
+// computed from a zero denominator (NaN) or a negative one (sign flip).
+func TestApplyInstanceLimitNonPositiveCountIsNotRescaled(t *testing.T) {
+	recs := []common.Recommendation{
+		{ResourceType: "zero-count", Count: 0, EstimatedSavings: 600, OnDemandCost: 100},
+		{ResourceType: "negative-count", Count: -5, EstimatedSavings: 300, OnDemandCost: 50},
+	}
+
+	got := ApplyInstanceLimit(recs, 10)
+
+	require.Len(t, got, 2)
+	for i := range got {
+		assert.False(t, isNaNOrInf(got[i].EstimatedSavings),
+			"%s: savings must not become NaN/Inf via a non-positive denominator", got[i].ResourceType)
+	}
+	assert.Equal(t, 0, got[0].Count)
+	assert.InDelta(t, 600.0, got[0].EstimatedSavings, 0.0001, "a zero-count row is not truncated, so it is not rescaled")
+	assert.InDelta(t, 100.0, got[0].OnDemandCost, 0.0001, "a zero-count row is not truncated, so it is not rescaled")
+	assert.Equal(t, -5, got[1].Count)
+	assert.InDelta(t, 300.0, got[1].EstimatedSavings, 0.0001, "a negative-count row is not truncated, so it is not rescaled")
+	assert.InDelta(t, 50.0, got[1].OnDemandCost, 0.0001, "a negative-count row is not truncated, so it is not rescaled")
+}
+
+// TestApplyInstanceLimitRescalesSavingsPlanHourlyCommitment covers the Savings
+// Plan case. SP recs are parsed at Count 1 and so are normally undivisible,
+// but --override-count sets Count on every rec without discriminating by
+// commitment type (ApplyCountOverride), which lets an SP reach the cap at a
+// count the budget can truncate. HourlyCommitment is the SP's actual money
+// quantity, so leaving it whole while the cost fields shrink produces exactly
+// the internally-inconsistent row #1830 warns about.
+func TestApplyInstanceLimitRescalesSavingsPlanHourlyCommitment(t *testing.T) {
+	const ratio = 2.0 / 5.0
+
+	rec := common.Recommendation{
+		Service:          common.ServiceSavingsPlansCompute,
+		Region:           "us-east-1",
+		Count:            5,
+		EstimatedSavings: 500,
+		Details: &common.SavingsPlanDetails{
+			PlanType:         "Compute",
+			HourlyCommitment: 10,
+		},
+	}
+
+	got := ApplyInstanceLimit([]common.Recommendation{rec}, 2)
+
+	require.Len(t, got, 1)
+	require.Equal(t, 2, got[0].Count)
+	assert.InDelta(t, 500*ratio, got[0].EstimatedSavings, 0.0001)
+
+	details, ok := got[0].Details.(*common.SavingsPlanDetails)
+	require.True(t, ok, "SP details must survive truncation with their concrete type")
+	assert.InDelta(t, 10*ratio, details.HourlyCommitment, 0.0001,
+		"an SP's hourly commitment must scale with the truncated Count, like every other extensive figure")
+
+	// The caller's Details must not be mutated through the shared pointer.
+	orig, ok := rec.Details.(*common.SavingsPlanDetails)
+	require.True(t, ok)
+	assert.InDelta(t, 10.0, orig.HourlyCommitment, 0.0001,
+		"the input rec's Details pointer target must not be mutated")
+}
+
+// TestApplyInstanceLimitTotalMatchesSumOfKeptRows is the run-summary invariant:
+// what the summary adds up must equal what the run will actually buy. It is
+// asserted against an independently computed expectation rather than against
+// the function's own output, so it cannot agree with a wrong implementation.
+func TestApplyInstanceLimitTotalMatchesSumOfKeptRows(t *testing.T) {
+	recs := []common.Recommendation{
+		{ResourceType: "whole", Count: 6, EstimatedSavings: 500},     // fits whole
+		{ResourceType: "truncated", Count: 6, EstimatedSavings: 100}, // truncated 6 -> 4
+		{ResourceType: "dropped", Count: 6, EstimatedSavings: 900},   // budget exhausted
+	}
+
+	got := ApplyInstanceLimit(recs, 10)
+
+	require.Len(t, got, 2, "the third row must be dropped once the budget is spent")
+	assert.Equal(t, 10, CalculateTotalInstances(got), "the cap is a hard budget")
+
+	var total float64
+	for i := range got {
+		total += got[i].EstimatedSavings
+	}
+	// 500 for the whole row + 100*(4/6) for the truncated one. The dropped
+	// row contributes nothing.
+	want := 500.0 + 100.0*(4.0/6.0)
+	assert.InDelta(t, want, total, 0.0001,
+		"the reported total must equal the savings of the instances the run will actually buy")
+}
+
+func isNaNOrInf(f float64) bool {
+	return f != f || f > 1e300 || f < -1e300
+}
diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go
index 76d360a9c..81092a2bc 100644
--- a/cmd/multi_service_csv_cap.go
+++ b/cmd/multi_service_csv_cap.go
@@ -92,9 +92,15 @@ func savingsPerInstance(rec common.Recommendation) float64 {
 // entirely, which would otherwise leave file order deciding between equals on
 // exactly the path #1741 is about.
 //
-// It must run before the cap, never after: ApplyInstanceLimit truncates Count
-// without rescaling EstimatedSavings (#1830), so a post-cap row's rate is
-// inflated by exactly the amount the cap removed.
+// It must run before the cap, never after: ApplyInstanceLimit consumes its
+// input in slice order, so by the time it returns the selection has already
+// been made and re-ordering the survivors decides nothing.
+//
+// The rate itself is now invariant across the cap. ApplyInstanceLimit scales
+// a truncated row's EstimatedSavings by the same ratio it cuts Count by
+// (#1830), and savings/count is unchanged by scaling both, so ranking is
+// unaffected by where the cap runs. It was not always so: before #1830 a
+// post-cap row's rate was inflated by exactly the amount the cap removed.
 func sortBySavingsPerInstance(recs []common.Recommendation) {
 	sort.SliceStable(recs, func(i, j int) bool {
 		a, b := recs[i], recs[j]
diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go
index f0befa28c..2b113a9ae 100644
--- a/cmd/multi_service_max_instances_test.go
+++ b/cmd/multi_service_max_instances_test.go
@@ -511,8 +511,12 @@ rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012
 	assert.InDelta(t, 500.00, got[0].EstimatedSavings, 0.001)
 	assert.Equal(t, countPerRow, got[0].Count)
 	assert.Equal(t, "db.t3.large", got[1].ResourceType)
-	assert.InDelta(t, 100.00, got[1].EstimatedSavings, 0.001)
 	assert.Equal(t, maxInstances-countPerRow, got[1].Count)
+	// This row is truncated, so its savings are the file's figure scaled by
+	// the share of the row the budget actually buys (#1830). Asserting the
+	// unscaled 100.00 here is what the bug looked like.
+	assert.InDelta(t, 100.00*float64(maxInstances-countPerRow)/float64(countPerRow),
+		got[1].EstimatedSavings, 0.001)
 
 	// Nothing shrinks silently: the row the cap dropped is named.
 	assert.Contains(t, out, "db.t3.small")
diff --git a/pkg/common/types.go b/pkg/common/types.go
index 56d4e9b12..760e31947 100644
--- a/pkg/common/types.go
+++ b/pkg/common/types.go
@@ -277,13 +277,30 @@ type ServiceDetails interface {
 	GetDetailDescription() string
 }
 
-// ScaleRecommendationCosts multiplies all cost-bearing fields of rec by
-// ratio and returns the result. RecurringMonthlyCost is allocated as a
-// new pointer when present so callers don't mutate the upstream rec's
-// pointer target. Used by sizing paths (ApplyCoverage, ApplyTargetCoverage,
-// family-NU) to keep Count and cost in sync when a recommendation is
-// sized down (or up) from AWS's proposal — without this helper the same
-// four-field scaling pattern was duplicated at every sizing site.
+// ScaleRecommendationCosts multiplies every extensive (whole-row) money field
+// of rec by ratio and returns the result. Used by sizing paths (ApplyCoverage,
+// ApplyTargetCoverage, ApplyInstanceLimit, family-NU) to keep Count and cost in
+// sync when a recommendation is sized down (or up) from AWS's proposal.
+// Without this helper the same scaling pattern was duplicated at every sizing
+// site, and #1830 was a sizing site that forgot it entirely.
+//
+// Extensive fields scale here; intensive ones deliberately do not.
+// SavingsPercentage and BreakEvenMonths are ratios of two figures that scale
+// together, so they are invariant. RecommendedCount is a frozen record of the
+// provider's pre-sizing proposal. AverageInstancesUsedPerHour and UsageHistory
+// describe observed demand, which does not change with what we choose to buy.
+//
+// Pointer and pointed-to state is copied rather than mutated: callers hold the
+// pre-sizing slice (the reporter diffs against it), so writing through a
+// shared pointer would corrupt their copy.
+//
+//   - RecurringMonthlyCost is allocated as a new pointer when present. A nil
+//     stays nil: nil means "the provider returned no monthly breakdown" and
+//     renders as "—", which is not the same claim as $0.
+//   - SavingsPlanDetails.HourlyCommitment is the SP's own money quantity (an
+//     SP commitment is dollar-denominated, not count-denominated), so it
+//     scales with the rest. Details is replaced with a scaled copy. A rec
+//     whose Details is any other type is left alone.
 func ScaleRecommendationCosts(rec Recommendation, ratio float64) Recommendation {
 	rec.CommitmentCost *= ratio
 	rec.OnDemandCost *= ratio
@@ -292,6 +309,11 @@ func ScaleRecommendationCosts(rec Recommendation, ratio float64) Recommendation
 		scaled := *rec.RecurringMonthlyCost * ratio
 		rec.RecurringMonthlyCost = &scaled
 	}
+	if details, ok := rec.Details.(*SavingsPlanDetails); ok {
+		scaled := *details
+		scaled.HourlyCommitment *= ratio
+		rec.Details = &scaled
+	}
 	return rec
 }
 

From 8c33a15113183e9a67507bef17a486d9bf0f7d93 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Tue, 18 Aug 2026 11:57:59 +0200
Subject: [PATCH 2/5] refactor(cli): address review findings on the truncation
 rescale

- Use math.IsNaN/math.IsInf instead of a hand-rolled float classifier that
  misreported large-but-finite values as infinite.
- Hoist the *SavingsPlanDetails assertion in applyTargetCoverageSP so the
  type is asserted once rather than three times. The zero-commitment and
  wrong-type guards are reordered, which is behaviour-equivalent: a non-SP
  Details reached the warning either way, and an SP with a non-positive
  commitment reached the skip either way.
- Enumerate the count-linear non-money fields ScaleRecommendationCosts
  deliberately leaves alone (ProjectedCoverage / ProjectedUtilization,
  DataWarehouseDetails.NumberOfNodes) so the docstring's completeness claim
  is honest.
- Drop the self-contradicting half of the sortBySavingsPerInstance comment:
  rate invariance makes a post-cap sort harmless, not useful, and the
  ordering requirement is unchanged.
---
 cmd/helpers.go                             | 22 +++++++++++-----------
 cmd/helpers_instance_limit_rescale_test.go |  7 ++-----
 cmd/multi_service_csv_cap.go               | 12 +++++-------
 pkg/common/types.go                        | 10 +++++++++-
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/cmd/helpers.go b/cmd/helpers.go
index 762211ddd..0316953cb 100644
--- a/cmd/helpers.go
+++ b/cmd/helpers.go
@@ -425,12 +425,22 @@ func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common
 	if rec.RecommendedUtilization <= 0 {
 		return rec, false
 	}
+	// If Details isn't a *SavingsPlanDetails (defensive — should always be
+	// for SP recs), log a warning and pass through UNCHANGED — including
+	// leaving ProjectedUtilization at zero. Setting projection fields on a
+	// rec whose commitment fields couldn't be scaled would produce a
+	// misleading row (projection=target%, savings=full-unscaled).
+	details, ok := rec.Details.(*common.SavingsPlanDetails)
+	if !ok {
+		AppLogger.Printf("WARNING: SP recommendation for service %q has unexpected Details type %T; passing through unscaled\n", rec.Service, rec.Details)
+		return rec, true
+	}
 	// Also treat a $0 HourlyCommitment as "no signal" — CE occasionally
 	// returns placeholder recs with zero commitment. Sizing such a rec
 	// would produce nonsense ($0 commitment * ratio = $0) while still
 	// claiming the target coverage is achieved, which is incoherent.
 	// Pass through unchanged and count in the skip summary.
-	if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details.HourlyCommitment <= 0 {
+	if details.HourlyCommitment <= 0 {
 		return rec, false
 	}
 
@@ -441,16 +451,6 @@ func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common
 	// zero value means we can't sanity-check the result); the scaling itself
 	// uses targetPct directly rather than a recUtil/target ratio so the flag's
 	// intent is honored even when AWS already projects above target.
-	//
-	// If Details isn't a *SavingsPlanDetails (defensive — should always be
-	// for SP recs), log a warning and pass through UNCHANGED — including
-	// leaving ProjectedUtilization at zero. Setting projection fields on a
-	// rec whose commitment fields couldn't be scaled would produce a
-	// misleading row (projection=target%, savings=full-unscaled).
-	if _, ok := rec.Details.(*common.SavingsPlanDetails); !ok {
-		AppLogger.Printf("WARNING: SP recommendation for service %q has unexpected Details type %T; passing through unscaled\n", rec.Service, rec.Details)
-		return rec, true
-	}
 	ratio := targetPct / 100.0
 	// ScaleRecommendationCosts scales HourlyCommitment along with the cost
 	// fields and replaces Details with a scaled copy.
diff --git a/cmd/helpers_instance_limit_rescale_test.go b/cmd/helpers_instance_limit_rescale_test.go
index d95fbbd9a..9348aef71 100644
--- a/cmd/helpers_instance_limit_rescale_test.go
+++ b/cmd/helpers_instance_limit_rescale_test.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"math"
 	"testing"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
@@ -138,7 +139,7 @@ func TestApplyInstanceLimitNonPositiveCountIsNotRescaled(t *testing.T) {
 
 	require.Len(t, got, 2)
 	for i := range got {
-		assert.False(t, isNaNOrInf(got[i].EstimatedSavings),
+		assert.False(t, math.IsNaN(got[i].EstimatedSavings) || math.IsInf(got[i].EstimatedSavings, 0),
 			"%s: savings must not become NaN/Inf via a non-positive denominator", got[i].ResourceType)
 	}
 	assert.Equal(t, 0, got[0].Count)
@@ -214,7 +215,3 @@ func TestApplyInstanceLimitTotalMatchesSumOfKeptRows(t *testing.T) {
 	assert.InDelta(t, want, total, 0.0001,
 		"the reported total must equal the savings of the instances the run will actually buy")
 }
-
-func isNaNOrInf(f float64) bool {
-	return f != f || f > 1e300 || f < -1e300
-}
diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go
index 81092a2bc..163c0a69e 100644
--- a/cmd/multi_service_csv_cap.go
+++ b/cmd/multi_service_csv_cap.go
@@ -94,13 +94,11 @@ func savingsPerInstance(rec common.Recommendation) float64 {
 //
 // It must run before the cap, never after: ApplyInstanceLimit consumes its
 // input in slice order, so by the time it returns the selection has already
-// been made and re-ordering the survivors decides nothing.
-//
-// The rate itself is now invariant across the cap. ApplyInstanceLimit scales
-// a truncated row's EstimatedSavings by the same ratio it cuts Count by
-// (#1830), and savings/count is unchanged by scaling both, so ranking is
-// unaffected by where the cap runs. It was not always so: before #1830 a
-// post-cap row's rate was inflated by exactly the amount the cap removed.
+// been made and re-ordering the survivors decides nothing. The rate values
+// themselves are invariant across the cap since #1830 (a truncated row's
+// savings and Count are scaled by the same ratio, and savings/count is
+// unchanged by scaling both), but that only means a post-cap sort would be
+// harmless rather than useful. Ordering still has to happen first.
 func sortBySavingsPerInstance(recs []common.Recommendation) {
 	sort.SliceStable(recs, func(i, j int) bool {
 		a, b := recs[i], recs[j]
diff --git a/pkg/common/types.go b/pkg/common/types.go
index 760e31947..f48cf3e9a 100644
--- a/pkg/common/types.go
+++ b/pkg/common/types.go
@@ -284,12 +284,20 @@ type ServiceDetails interface {
 // Without this helper the same scaling pattern was duplicated at every sizing
 // site, and #1830 was a sizing site that forgot it entirely.
 //
-// Extensive fields scale here; intensive ones deliberately do not.
+// Extensive MONEY fields scale here; intensive ones deliberately do not.
 // SavingsPercentage and BreakEvenMonths are ratios of two figures that scale
 // together, so they are invariant. RecommendedCount is a frozen record of the
 // provider's pre-sizing proposal. AverageInstancesUsedPerHour and UsageHistory
 // describe observed demand, which does not change with what we choose to buy.
 //
+// Two count-linear NON-money fields are deliberately out of scope here and are
+// the caller's problem: ProjectedCoverage / ProjectedUtilization (recomputed
+// from the chosen quantity by the target-coverage callers, which is why
+// scaling them here would be wrong) and DataWarehouseDetails.NumberOfNodes
+// (a Redshift count mirror set at parse time). Neither is re-derived by
+// ApplyInstanceLimit, so a capped run can still report a projection sized for
+// the pre-cap count. Tracked separately; purchases read Count, not these.
+//
 // Pointer and pointed-to state is copied rather than mutated: callers hold the
 // pre-sizing slice (the reporter diffs against it), so writing through a
 // shared pointer would corrupt their copy.

From 475d243ec28f84cc17fcc0ba8b4b6c6568d2eeca Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Tue, 18 Aug 2026 12:19:23 +0200
Subject: [PATCH 3/5] docs(cli): cite the follow-up issues for the
 count-derived fields left unscaled

#1844 tracks ApplyCountOverride, which replaces Count without rescaling and
runs upstream of the cap. #1845 tracks the projection and node-count mirrors
that no sizing path re-derives.
---
 cmd/helpers.go      | 5 +++++
 pkg/common/types.go | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cmd/helpers.go b/cmd/helpers.go
index 0316953cb..7b0b5cbad 100644
--- a/cmd/helpers.go
+++ b/cmd/helpers.go
@@ -485,6 +485,11 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro
 }
 
 // ApplyCountOverride overrides the count for all recommendations.
+//
+// It does NOT rescale the count-derived money fields, so an overridden row
+// still carries the savings of the count the provider proposed. That is the
+// same defect ApplyInstanceLimit had before #1830, on a neighbouring flag,
+// and it runs upstream of the cap. Tracked by #1844.
 func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []common.Recommendation {
 	if overrideCount <= 0 {
 		return recs
diff --git a/pkg/common/types.go b/pkg/common/types.go
index f48cf3e9a..eed294d71 100644
--- a/pkg/common/types.go
+++ b/pkg/common/types.go
@@ -296,7 +296,7 @@ type ServiceDetails interface {
 // scaling them here would be wrong) and DataWarehouseDetails.NumberOfNodes
 // (a Redshift count mirror set at parse time). Neither is re-derived by
 // ApplyInstanceLimit, so a capped run can still report a projection sized for
-// the pre-cap count. Tracked separately; purchases read Count, not these.
+// the pre-cap count. Tracked by #1845; purchases read Count, not these.
 //
 // Pointer and pointed-to state is copied rather than mutated: callers hold the
 // pre-sizing slice (the reporter diffs against it), so writing through a

From 274313312edc06815b1f8d0ae63646df39e6d993 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Tue, 18 Aug 2026 13:01:29 +0200
Subject: [PATCH 4/5] style(cli): use American spelling in the
 ApplyCountOverride doc comment

The misspell linter enforces American spelling and reddened the Lint Code
job on "neighbouring".
---
 cmd/helpers.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/helpers.go b/cmd/helpers.go
index 7b0b5cbad..f97168566 100644
--- a/cmd/helpers.go
+++ b/cmd/helpers.go
@@ -488,7 +488,7 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro
 //
 // It does NOT rescale the count-derived money fields, so an overridden row
 // still carries the savings of the count the provider proposed. That is the
-// same defect ApplyInstanceLimit had before #1830, on a neighbouring flag,
+// same defect ApplyInstanceLimit had before #1830, on a neighboring flag,
 // and it runs upstream of the cap. Tracked by #1844.
 func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []common.Recommendation {
 	if overrideCount <= 0 {

From e819361d3d2a0b1f67a497f746a3bb661340abdd Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Tue, 18 Aug 2026 13:46:44 +0200
Subject: [PATCH 5/5] fix(cli): degrade instead of panicking on a typed-nil
 SavingsPlanDetails

An interface holding (*SavingsPlanDetails)(nil) satisfies a type assertion
to *SavingsPlanDetails with ok == true and yields a nil pointer, so the
comma-ok alone was not a guard. Three sizing sites then dereferenced it:

- ScaleRecommendationCosts copied *details
- applyCoverage's SP branch discarded the pointer and called the helper,
  which panicked inside
- applyTargetCoverageSP read details.HourlyCommitment for its no-signal
  guard, before any nil check

A malformed recommendation must degrade and warn, not crash. Panicking
partway through a capped run leaves the operator with no report at all
rather than a degraded one, which is the opposite of what the rest of this
change is careful about on a money path.

The two cmd sites now take the existing warn-and-pass-through-unscaled path,
which is what a rec whose commitment cannot be scaled should do. The helper
leaves a nil Details exactly as it found it rather than substituting a
fabricated zero-value commitment, and still scales the cost fields, which do
not depend on Details. The warning text now says "missing or unexpected"
since a typed nil is neither an absent Details nor a wrong type.

Regression tests use an interface explicitly holding a typed nil; a plain
nil interface fails the assertion and takes a different path, so only the
typed form reproduces the crash. A sentinel test pins that Go semantic so
the guards cannot start passing vacuously.
---
 cmd/helpers.go                        |  36 ++++---
 cmd/helpers_typed_nil_details_test.go | 144 ++++++++++++++++++++++++++
 pkg/common/types.go                   |   7 +-
 3 files changed, 172 insertions(+), 15 deletions(-)
 create mode 100644 cmd/helpers_typed_nil_details_test.go

diff --git a/cmd/helpers.go b/cmd/helpers.go
index f97168566..de861a554 100644
--- a/cmd/helpers.go
+++ b/cmd/helpers.go
@@ -143,18 +143,22 @@ func applyCoverage(recs []common.Recommendation, coverage float64, drops *common
 		adjusted := rec
 
 		// For Savings Plans, reduce the hourly commitment instead of count.
-		// If the type assertion fails (defensive — Details should always
-		// be *SavingsPlanDetails for SP recs), preserve the recommendation
-		// at its original values rather than silently dropping it. A
-		// missing-Details record is a logged anomaly, not a reason to
-		// erase coverage from the run.
+		// If Details is the wrong type or a nil pointer (defensive — it
+		// should always be a non-nil *SavingsPlanDetails for SP recs),
+		// preserve the recommendation at its original values rather than
+		// silently dropping it. A missing-Details record is a logged
+		// anomaly, not a reason to erase coverage from the run.
+		//
+		// The nil check matters: an interface holding a typed nil satisfies
+		// the assertion, so testing ok alone would send an unscalable rec
+		// down the scaling path.
 		if common.IsSavingsPlan(rec.Service) {
-			if _, ok := rec.Details.(*common.SavingsPlanDetails); ok {
+			if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details != nil {
 				// ScaleRecommendationCosts scales HourlyCommitment along with
 				// the cost fields and replaces Details with a scaled copy.
 				adjusted = common.ScaleRecommendationCosts(adjusted, ratio)
 			} else {
-				AppLogger.Printf("WARNING: SP recommendation for service %q has unexpected Details type %T; passing through unscaled\n", rec.Service, rec.Details)
+				AppLogger.Printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details)
 			}
 			result = append(result, adjusted)
 			continue
@@ -425,14 +429,18 @@ func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common
 	if rec.RecommendedUtilization <= 0 {
 		return rec, false
 	}
-	// If Details isn't a *SavingsPlanDetails (defensive — should always be
-	// for SP recs), log a warning and pass through UNCHANGED — including
-	// leaving ProjectedUtilization at zero. Setting projection fields on a
-	// rec whose commitment fields couldn't be scaled would produce a
-	// misleading row (projection=target%, savings=full-unscaled).
+	// If Details isn't a non-nil *SavingsPlanDetails (defensive — it should
+	// always be one for SP recs), log a warning and pass through UNCHANGED —
+	// including leaving ProjectedUtilization at zero. Setting projection
+	// fields on a rec whose commitment fields couldn't be scaled would
+	// produce a misleading row (projection=target%, savings=full-unscaled).
+	//
+	// The nil check must precede the HourlyCommitment read below: an
+	// interface holding a typed nil satisfies the assertion, so reading the
+	// field off it would dereference nil.
 	details, ok := rec.Details.(*common.SavingsPlanDetails)
-	if !ok {
-		AppLogger.Printf("WARNING: SP recommendation for service %q has unexpected Details type %T; passing through unscaled\n", rec.Service, rec.Details)
+	if !ok || details == nil {
+		AppLogger.Printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details)
 		return rec, true
 	}
 	// Also treat a $0 HourlyCommitment as "no signal" — CE occasionally
diff --git a/cmd/helpers_typed_nil_details_test.go b/cmd/helpers_typed_nil_details_test.go
new file mode 100644
index 000000000..b45214fc0
--- /dev/null
+++ b/cmd/helpers_typed_nil_details_test.go
@@ -0,0 +1,144 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/LeanerCloud/CUDly/pkg/common"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// typedNilSPDetails returns a ServiceDetails interface holding a TYPED nil
+// *SavingsPlanDetails. This is not the same value as a plain nil interface:
+// a type assertion to *SavingsPlanDetails succeeds on this one (ok == true)
+// and yields a nil pointer, so any unguarded dereference panics. A plain nil
+// interface fails the assertion and takes the warning path instead, which is
+// why only the typed form reproduces the crash.
+func typedNilSPDetails() common.ServiceDetails {
+	var d *common.SavingsPlanDetails // nil pointer of concrete type
+	return d
+}
+
+// TestTypedNilSPDetailsAssertionSucceeds pins the Go semantic the other tests
+// in this file depend on. If this ever stops holding, the guards below are
+// guarding nothing and the tests would pass vacuously.
+func TestTypedNilSPDetailsAssertionSucceeds(t *testing.T) {
+	details, ok := typedNilSPDetails().(*common.SavingsPlanDetails)
+	require.True(t, ok, "a typed nil must still satisfy the type assertion")
+	require.Nil(t, details, "and must yield a nil pointer, which is what makes an unguarded deref panic")
+
+	var plain common.ServiceDetails
+	_, plainOK := plain.(*common.SavingsPlanDetails)
+	require.False(t, plainOK, "a plain nil interface must NOT satisfy it; the two values differ")
+}
+
+// TestScaleRecommendationCostsSurvivesTypedNilDetails covers the helper
+// itself. A malformed recommendation must degrade, not crash: the cost fields
+// still scale (they do not depend on Details) and Details is left exactly as
+// it was rather than being replaced with a fabricated zero-value struct.
+func TestScaleRecommendationCostsSurvivesTypedNilDetails(t *testing.T) {
+	rec := common.Recommendation{
+		Service:          common.ServiceSavingsPlansCompute,
+		Count:            4,
+		EstimatedSavings: 500,
+		CommitmentCost:   200,
+		OnDemandCost:     700,
+		Details:          typedNilSPDetails(),
+	}
+
+	var got common.Recommendation
+	require.NotPanics(t, func() {
+		got = common.ScaleRecommendationCosts(rec, 0.5)
+	}, "a typed nil *SavingsPlanDetails must not panic the sizing helper")
+
+	assert.InDelta(t, 250.0, got.EstimatedSavings, 0.0001, "cost fields do not depend on Details and must still scale")
+	assert.InDelta(t, 100.0, got.CommitmentCost, 0.0001)
+	assert.InDelta(t, 350.0, got.OnDemandCost, 0.0001)
+
+	details, ok := got.Details.(*common.SavingsPlanDetails)
+	require.True(t, ok)
+	assert.Nil(t, details,
+		"a nil commitment must stay nil rather than becoming a fabricated zero-value struct")
+}
+
+// TestApplyInstanceLimitSurvivesTypedNilSPDetails is the #1830 path. A capped
+// run must still produce a report when one recommendation is malformed;
+// panicking mid-run leaves the operator with no report at all rather than a
+// degraded one.
+func TestApplyInstanceLimitSurvivesTypedNilSPDetails(t *testing.T) {
+	recs := []common.Recommendation{
+		{
+			Service:          common.ServiceSavingsPlansCompute,
+			ResourceType:     "malformed",
+			Count:            10,
+			EstimatedSavings: 500,
+			Details:          typedNilSPDetails(),
+		},
+		{
+			Service:          common.ServiceEC2,
+			ResourceType:     "healthy",
+			Count:            4,
+			EstimatedSavings: 100,
+		},
+	}
+
+	var got []common.Recommendation
+	require.NotPanics(t, func() {
+		got = ApplyInstanceLimit(recs, 4)
+	}, "a malformed row must not crash the whole capped run")
+
+	require.Len(t, got, 1, "the budget is spent by the first row")
+	assert.Equal(t, 4, got[0].Count)
+	assert.InDelta(t, 500.0*4.0/10.0, got[0].EstimatedSavings, 0.0001,
+		"the truncated row's money still rescales even though its Details are malformed")
+}
+
+// TestApplyCoverageSurvivesTypedNilSPDetails covers the applyCoverage SP
+// branch, which discards the asserted pointer and so cannot see the nil
+// itself. A typed nil must take the same warning-and-pass-through path as a
+// wrong Details type, since neither can have its commitment scaled.
+func TestApplyCoverageSurvivesTypedNilSPDetails(t *testing.T) {
+	rec := common.Recommendation{
+		Service:          common.ServiceSavingsPlansCompute,
+		Count:            1,
+		EstimatedSavings: 500,
+		CommitmentCost:   200,
+		Details:          typedNilSPDetails(),
+	}
+
+	var got []common.Recommendation
+	require.NotPanics(t, func() {
+		got = ApplyCoverage([]common.Recommendation{rec}, 50)
+	}, "a typed nil must not panic the coverage sizing path")
+
+	require.Len(t, got, 1, "a malformed rec is preserved, not dropped")
+	assert.InDelta(t, 500.0, got[0].EstimatedSavings, 0.0001,
+		"an unscalable commitment must pass through UNSCALED rather than scaling costs it cannot match")
+	assert.InDelta(t, 200.0, got[0].CommitmentCost, 0.0001)
+}
+
+// TestApplyTargetCoverageSurvivesTypedNilSPDetails covers applyTargetCoverageSP,
+// which reads HourlyCommitment off the asserted pointer for its no-signal
+// guard and so dereferences before any nil check.
+func TestApplyTargetCoverageSurvivesTypedNilSPDetails(t *testing.T) {
+	rec := common.Recommendation{
+		Service:                common.ServiceSavingsPlansCompute,
+		Count:                  1,
+		RecommendedUtilization: 90,
+		EstimatedSavings:       500,
+		CommitmentCost:         200,
+		Details:                typedNilSPDetails(),
+	}
+
+	var got []common.Recommendation
+	require.NotPanics(t, func() {
+		got = ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil)
+	}, "a typed nil must not panic the target-coverage sizing path")
+
+	require.Len(t, got, 1, "a malformed rec is preserved, not dropped")
+	assert.InDelta(t, 500.0, got[0].EstimatedSavings, 0.0001,
+		"an unscalable commitment must pass through UNSCALED")
+	assert.InDelta(t, 200.0, got[0].CommitmentCost, 0.0001)
+	assert.InDelta(t, 0.0, got[0].ProjectedUtilization, 0.0001,
+		"projection fields stay zero on a rec whose commitment could not be scaled")
+}
diff --git a/pkg/common/types.go b/pkg/common/types.go
index eed294d71..c0f3ce8ce 100644
--- a/pkg/common/types.go
+++ b/pkg/common/types.go
@@ -317,7 +317,12 @@ func ScaleRecommendationCosts(rec Recommendation, ratio float64) Recommendation
 		scaled := *rec.RecurringMonthlyCost * ratio
 		rec.RecurringMonthlyCost = &scaled
 	}
-	if details, ok := rec.Details.(*SavingsPlanDetails); ok {
+	// The nil check is not redundant with the comma-ok: an interface holding a
+	// typed nil (*SavingsPlanDetails)(nil) satisfies the assertion with
+	// ok == true, so copying without it dereferences nil. A malformed rec is
+	// left exactly as it was rather than gaining a fabricated zero-value
+	// commitment.
+	if details, ok := rec.Details.(*SavingsPlanDetails); ok && details != nil {
 		scaled := *details
 		scaled.HourlyCommitment *= ratio
 		rec.Details = &scaled