fix(aws/recommendations): fail the detail on an invalid average-instances or SP utilization

providers/aws/recommendations/parser_ri.go

@@ -92,7 +92,9 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R
 	}
 
 	// Parse RI utilization signals used by --target-coverage sizing
-	c.parseRIUtilizationSignals(rec, details)
+	if err := c.parseRIUtilizationSignals(rec, details); err != nil {
+		return nil, fmt.Errorf("failed to parse RI utilization signals: %w", err)
+	}
 
 	// Parse service-specific details
 	if err := c.parseServiceSpecificDetails(ctx, rec, details, params.Service); err != nil {
@@ -108,23 +110,26 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R
 }
 
 // parseRIUtilizationSignals populates AverageInstancesUsedPerHour and
-// RecommendedUtilization from the CE response. Both fields are *string in the
-// SDK; nil or unparseable values leave the destination at zero, which the
-// --target-coverage sizing path treats as "no signal" and skips.
-func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) {
-	// Route through parseOptionalFloatOrWarn so a non-finite/negative value
-	// (which strconv.ParseFloat accepts / passes through) degrades to 0 rather
-	// than being stored as a live signal. The downstream --target-coverage
-	// guards are all `<= 0`, and NaN <= 0 is false, so a stored NaN would be
-	// treated as a real signal and produce NaN purchase counts.
-	//
-	// The field label carries service/account context so a warning still
-	// identifies which row was corrupt (the pre-refactor inline logs did).
+// RecommendedUtilization from the CE response.
+//
+// AverageNumberOfInstancesUsedPerHour is a sizing input: --target-coverage
+// treats zero as "no signal" and passes the rec through at AWS's full count
+// (and --min-pool-size keeps avg<=0 recs), so a present-but-invalid value
+// (unparsable, non-finite, negative, empty) must fail the detail instead of
+// degrading to 0. A nil field is genuinely absent and stays 0. AverageUtilization
+// is display-only here and degrades to 0 with a warning.
+func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error {
+	// The field label carries service/account context so an error still
+	// identifies which row was corrupt.
 	ctx := fmt.Sprintf("service=%s account=%s", rec.Service, rec.Account)
-	rec.AverageInstancesUsedPerHour = parseOptionalFloatOrWarn(
-		"AverageNumberOfInstancesUsedPerHour ("+ctx+")", details.AverageNumberOfInstancesUsedPerHour)
+	avg, err := parseOptionalFloat("AverageNumberOfInstancesUsedPerHour ("+ctx+")", details.AverageNumberOfInstancesUsedPerHour)
+	if err != nil {
+		return err
+	}
+	rec.AverageInstancesUsedPerHour = avg
 	rec.RecommendedUtilization = parseOptionalFloatOrWarn(
 		"AverageUtilization ("+ctx+")", details.AverageUtilization)
+	return nil
 }
 
 // parseRecommendedQuantity extracts the recommended quantity from details. The

providers/aws/recommendations/parser_ri_test.go

@@ -586,9 +586,10 @@ func TestParseRecommendations_EmptyInput(t *testing.T) {
 
 // TestParseRIUtilizationSignals covers the AverageNumberOfInstancesUsedPerHour
 // and AverageUtilization fields added for issue #338 (--target-coverage).
-// Verifies both successful parses, nil-pointer fallback to zero, and
-// parse-failure fallback to zero — the sizing path in cmd/helpers.go treats
-// zero as "no signal" so the fallback behavior matters.
+// Verifies successful parses and nil-pointer fallback to zero. A present but
+// invalid average-instances value is an error (zero means "no signal" to the
+// sizing path, so it must not be fabricated); an invalid AverageUtilization is
+// display-only and degrades to zero.
 func TestParseRIUtilizationSignals(t *testing.T) {
 	client := &Client{}
 
@@ -597,6 +598,7 @@ func TestParseRIUtilizationSignals(t *testing.T) {
 		details          *types.ReservationPurchaseRecommendationDetail
 		wantAvgInstances float64
 		wantUtilization  float64
+		wantErr          bool
 	}{
 		{
 			name: "both fields parsed",
@@ -614,13 +616,12 @@ func TestParseRIUtilizationSignals(t *testing.T) {
 			wantUtilization:  0,
 		},
 		{
-			name: "unparseable AverageNumberOfInstancesUsedPerHour → that field zero, other still parses",
+			name: "unparseable AverageNumberOfInstancesUsedPerHour is an error",
 			details: &types.ReservationPurchaseRecommendationDetail{
 				AverageNumberOfInstancesUsedPerHour: aws.String("not-a-number"),
 				AverageUtilization:                  aws.String("90.0"),
 			},
-			wantAvgInstances: 0,
-			wantUtilization:  90.0,
+			wantErr: true,
 		},
 		{
 			name: "unparseable AverageUtilization → that field zero, other still parses",
@@ -641,23 +642,35 @@ func TestParseRIUtilizationSignals(t *testing.T) {
 			wantUtilization:  0,
 		},
 		{
-			// NaN/Inf parse to a nil error under strconv.ParseFloat; they must
-			// degrade to 0, not be stored as a live signal (NaN <= 0 is false,
-			// so a stored NaN would drive NaN purchase counts in sizing).
-			name: "non-finite values degrade to zero",
+			// NaN parses to a nil error under strconv.ParseFloat; it must be
+			// rejected, not stored as a live signal (NaN <= 0 is false, so a
+			// stored NaN would drive NaN purchase counts in sizing).
+			name: "non-finite AverageNumberOfInstancesUsedPerHour is an error",
 			details: &types.ReservationPurchaseRecommendationDetail{
 				AverageNumberOfInstancesUsedPerHour: aws.String("NaN"),
+			},
+			wantErr: true,
+		},
+		{
+			name: "non-finite AverageUtilization degrades to zero (display only)",
+			details: &types.ReservationPurchaseRecommendationDetail{
+				AverageNumberOfInstancesUsedPerHour: aws.String("3"),
 				AverageUtilization:                  aws.String("+Inf"),
 			},
-			wantAvgInstances: 0,
+			wantAvgInstances: 3,
 			wantUtilization:  0,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			rec := &common.Recommendation{}
-			client.parseRIUtilizationSignals(rec, tt.details)
+			err := client.parseRIUtilizationSignals(rec, tt.details)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
 			assert.Equal(t, tt.wantAvgInstances, rec.AverageInstancesUsedPerHour)
 			assert.Equal(t, tt.wantUtilization, rec.RecommendedUtilization)
 		})
@@ -767,3 +780,41 @@ func captureStdoutAndLog(t *testing.T, fn func()) (stdout, logged string) {
 	require.NoError(t, r.Close())
 	return stdout, logBuf.String()
 }
+
+// FIXTURE-BASED (CE-shaped ReservationPurchaseRecommendationDetail, not live AWS).
+// go#54: a present-but-invalid AverageNumberOfInstancesUsedPerHour used to
+// become 0 = "no signal", so --target-coverage passed the rec through unsized
+// and --min-pool-size kept it. It must now drop the detail and report it.
+func TestParseRecommendations_InvalidAverageInstancesIsReportedNotZeroed(t *testing.T) {
+	detail := func(avg *string) types.ReservationPurchaseRecommendationDetail {
+		return types.ReservationPurchaseRecommendationDetail{
+			RecommendedNumberOfInstancesToPurchase: aws.String("5"),
+			EstimatedMonthlySavingsAmount:          aws.String("100.00"),
+			AverageNumberOfInstancesUsedPerHour:    avg,
+			InstanceDetails: &types.InstanceDetails{EC2InstanceDetails: &types.EC2InstanceDetails{
+				InstanceType: aws.String("m5.large"), Platform: aws.String("Linux/UNIX"), Region: aws.String("us-east-1"),
+			}},
+		}
+	}
+	params := common.RecommendationParams{Service: common.ServiceEC2, Term: "1yr", PaymentOption: "no-upfront"}
+	for _, bad := range []string{"abc", "NaN", "-1", "Inf", ""} {
+		t.Run("invalid "+bad, func(t *testing.T) {
+			awsRecs := []types.ReservationPurchaseRecommendation{{RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{
+				detail(aws.String("10")), detail(aws.String(bad)),
+			}}}
+			recs, err := (&Client{}).parseRecommendations(context.Background(), awsRecs, params)
+			var incomplete *IncompleteRecommendationsError
+			require.ErrorAs(t, err, &incomplete)
+			assert.Equal(t, 1, incomplete.FailedDetails)
+			require.Len(t, recs, 1, "the valid sibling survives")
+			assert.Equal(t, 10.0, recs[0].AverageInstancesUsedPerHour)
+		})
+	}
+	t.Run("nil stays a kept rec with no signal", func(t *testing.T) {
+		awsRecs := []types.ReservationPurchaseRecommendation{{RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{detail(nil)}}}
+		recs, err := (&Client{}).parseRecommendations(context.Background(), awsRecs, params)
+		require.NoError(t, err)
+		require.Len(t, recs, 1)
+		assert.Zero(t, recs[0].AverageInstancesUsedPerHour)
+	})
+}

providers/aws/recommendations/parser_sp.go

@@ -81,9 +81,10 @@ func parseOptionalFloat(field string, s *string) (float64, error) {
 
 // parseOptionalFloatOrWarn parses a *string as float64 but treats present-but-
 // unparseable as a non-fatal warning (logs and returns 0). Use ONLY for
-// non-money fields (utilization percentages, averages) where a bad value
-// degrades gracefully. For money fields use parseOptionalFloat and propagate
-// the error.
+// display-only fields (savings percentage, RI AverageUtilization, SP on-demand
+// spend) where a bad value degrades gracefully. Money fields and any value a
+// sizing path reads (average instances used, SP estimated utilization) must use
+// parseOptionalFloat and propagate the error: 0 means "no signal" to sizing.
 func parseOptionalFloatOrWarn(field string, s *string) float64 {
 	val, err := parseOptionalFloat(field, s)
 	if err != nil {
@@ -171,10 +172,14 @@ func (c *Client) parseSavingsPlanDetail(
 	// two-tier treatment (hard error on cost, warn-and-continue on percentages).
 	savingsPercent := parseOptionalFloatOrWarn("EstimatedSavingsPercentage", detail.EstimatedSavingsPercentage)
 	// EstimatedAverageUtilization carries the "if you buy exactly this commitment,
-	// what % of it will AWS expect to be used" signal. Used by --target-coverage
-	// sizing in cmd/helpers.go; zero (nil pointer or parse failure) means "no signal"
-	// and the sizing path leaves the recommendation unchanged.
-	recommendedUtilization := parseOptionalFloatOrWarn("EstimatedAverageUtilization", detail.EstimatedAverageUtilization)
+	// what % of it will AWS expect to be used" signal. It is a sizing input:
+	// --target-coverage treats zero as "no signal" and leaves the recommendation
+	// unsized, so a nil pointer stays 0 but a present-but-invalid value fails the
+	// detail rather than degrading to 0.
+	recommendedUtilization, err := parseOptionalFloat("EstimatedAverageUtilization", detail.EstimatedAverageUtilization)
+	if err != nil {
+		return nil, err
+	}
 	// onDemandCost is the canonical monthly on-demand baseline for this SP
 	// recommendation. AWS Cost Explorer returns the average hourly on-demand
 	// spend over the lookback period in CurrentAverageHourlyOnDemandSpend;

providers/aws/recommendations/parser_sp_test.go

@@ -167,6 +167,7 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) {
 		utilizationStr     *string
 		wantUtilization    float64
 		wantAvgInstancesIs float64
+		wantErr            bool
 	}{
 		{
 			name:               "field present and parseable",
@@ -181,10 +182,9 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) {
 			wantAvgInstancesIs: 0,
 		},
 		{
-			name:               "field unparseable → zero (parseOptionalFloat logs warn)",
-			utilizationStr:     aws.String("not-a-number"),
-			wantUtilization:    0,
-			wantAvgInstancesIs: 0,
+			name:           "field unparseable is an error (sizing input, go#54)",
+			utilizationStr: aws.String("not-a-number"),
+			wantErr:        true,
 		},
 	}
 
@@ -195,8 +195,11 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) {
 				EstimatedAverageUtilization: tt.utilizationStr,
 			}
 			rec, err := client.parseSavingsPlanDetail(detail, &params, types.SupportedSavingsPlansTypeComputeSp)
-			require.NoError(t, err,
-				"EstimatedAverageUtilization is a non-money field; parse failures must not propagate as errors")
+			if tt.wantErr {
+				require.Error(t, err, "EstimatedAverageUtilization is a sizing input; zero means no signal, so a bad value must fail the detail")
+				return
+			}
+			require.NoError(t, err)
 			require.NotNil(t, rec)
 			assert.Equal(t, tt.wantUtilization, rec.RecommendedUtilization,
 				"SP utilization should be parsed into rec.RecommendedUtilization")
@@ -503,3 +506,39 @@ func TestExtractEC2SPFieldsNormalizesRegion(t *testing.T) {
 		assert.Empty(t, got.instanceFamily)
 	})
 }
+
+// FIXTURE-BASED (CE-shaped SavingsPlansPurchaseRecommendationDetail, not live AWS).
+// go#54: a present-but-invalid EstimatedAverageUtilization used to become 0 =
+// "no signal", so --target-coverage left the SP unsized at AWS's commitment.
+func TestParseSavingsPlansRecommendations_InvalidEstimatedUtilizationIsReported(t *testing.T) {
+	detail := func(util *string) types.SavingsPlansPurchaseRecommendationDetail {
+		return types.SavingsPlansPurchaseRecommendationDetail{
+			HourlyCommitmentToPurchase:    aws.String("1.5"),
+			EstimatedMonthlySavingsAmount: aws.String("100"),
+			UpfrontCost:                   aws.String("0"),
+			EstimatedAverageUtilization:   util,
+		}
+	}
+	params := &common.RecommendationParams{Service: common.ServiceSavingsPlansCompute, Term: "1yr", PaymentOption: "no-upfront"}
+	planType := types.SupportedSavingsPlansTypeComputeSp
+	for _, bad := range []string{"abc", "NaN", "-1", "Inf", ""} {
+		t.Run("invalid "+bad, func(t *testing.T) {
+			spRec := &types.SavingsPlansPurchaseRecommendation{SavingsPlansPurchaseRecommendationDetails: []types.SavingsPlansPurchaseRecommendationDetail{
+				detail(aws.String("90")), detail(aws.String(bad)),
+			}}
+			recs, err := (&Client{}).parseSavingsPlansRecommendations(spRec, params, planType, 0)
+			var incomplete *IncompleteRecommendationsError
+			require.ErrorAs(t, err, &incomplete)
+			assert.Equal(t, 1, incomplete.FailedDetails)
+			require.Len(t, recs, 1)
+			assert.Equal(t, 90.0, recs[0].RecommendedUtilization)
+		})
+	}
+	t.Run("nil stays a kept rec with no signal", func(t *testing.T) {
+		spRec := &types.SavingsPlansPurchaseRecommendation{SavingsPlansPurchaseRecommendationDetails: []types.SavingsPlansPurchaseRecommendationDetail{detail(nil)}}
+		recs, err := (&Client{}).parseSavingsPlansRecommendations(spRec, params, planType, 0)
+		require.NoError(t, err)
+		require.Len(t, recs, 1)
+		assert.Zero(t, recs[0].RecommendedUtilization)
+	})
+}