fix(providers): correct Azure resourceType filters + fail-loud SP money parsing + nil covered-cost (adversarial-review)

providers/aws/recommendations/parser_sp.go

@@ -167,7 +167,15 @@ func (c *Client) parseSavingsPlansRecommendations(
 	var recommendations []common.Recommendation
 
 	for _, detail := range spRec.SavingsPlansPurchaseRecommendationDetails {
-		rec := c.parseSavingsPlanDetail(&detail, params, planType)
+		rec, err := c.parseSavingsPlanDetail(&detail, params, planType)
+		if err != nil {
+			// present-but-unparseable money field: drop this recommendation
+			// rather than forwarding a corrupt $0 to the scheduler/frontend.
+			// Mirrors the RI path (parseAWSCostDetails) which errors on the
+			// same class. Log so operators can detect malformed CE responses.
+			log.Printf("WARNING: skipping SP recommendation (planType=%s): %v", planType, err)
+			continue
+		}
 		if rec != nil {
 			recommendations = append(recommendations, *rec)
 		}
@@ -176,16 +184,34 @@ func (c *Client) parseSavingsPlansRecommendations(
 	return recommendations
 }
 
-// parseSavingsPlanDetail converts a single Savings Plan recommendation detail
-// parseOptionalFloat parses a string pointer as float64, logging a warning on failure.
-// Returns 0 if the pointer is nil.
-func parseOptionalFloat(field string, s *string) float64 {
+// parseOptionalFloat parses a *string pointer as float64.
+//   - nil pointer (field absent from API response): returns (0, nil) — absent
+//     is acceptable; callers treat 0 as "not available".
+//   - non-nil pointer that fails ParseFloat (present but unparseable): returns
+//     (0, error) — mirrors the RI path (parseAWSCostDetails) which errors on
+//     the same class rather than silently substituting 0. Callers must propagate
+//     this error and drop the recommendation so a corrupt money figure never
+//     reaches the scheduler or frontend.
+func parseOptionalFloat(field string, s *string) (float64, error) {
 	if s == nil {
-		return 0
+		return 0, nil
 	}
 	val, err := strconv.ParseFloat(*s, 64)
 	if err != nil {
-		log.Printf("WARNING: failed to parse %s: %v", field, err)
+		return 0, fmt.Errorf("failed to parse %s %q: %w", field, *s, err)
+	}
+	return val, nil
+}
+
+// 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.
+func parseOptionalFloatOrWarn(field string, s *string) float64 {
+	val, err := parseOptionalFloat(field, s)
+	if err != nil {
+		log.Printf("WARNING: %v — treating as 0", err)
 		return 0
 	}
 	return val
@@ -218,20 +244,57 @@ func extractEC2SPFields(planType types.SupportedSavingsPlansType, detail *types.
 	}
 }
 
+// spPlanTypeDisplayString converts a SupportedSavingsPlansType to a
+// human-readable plan-type label used in SavingsPlanDetails.PlanType.
+// Returns the raw SDK string for unrecognised types (forward-compat).
+func spPlanTypeDisplayString(pt types.SupportedSavingsPlansType) string {
+	switch pt {
+	case types.SupportedSavingsPlansTypeComputeSp:
+		return "Compute"
+	case types.SupportedSavingsPlansTypeEc2InstanceSp:
+		return "EC2Instance"
+	case types.SupportedSavingsPlansTypeSagemakerSp:
+		return "SageMaker"
+	case types.SupportedSavingsPlansTypeDatabaseSp:
+		return "Database"
+	}
+	return string(pt)
+}
+
+// parseSavingsPlanDetail converts a single Savings Plan recommendation detail
+// into a *common.Recommendation. Returns (nil, error) when any money field
+// (HourlyCommitmentToPurchase, EstimatedMonthlySavingsAmount, UpfrontCost) is
+// present in the API response but cannot be parsed as float64 — mirroring the
+// RI path (parseAWSCostDetails) which errors on the same class rather than
+// silently substituting 0. Non-money fields (percentages, utilization) degrade
+// to 0 with a log warning.
 func (c *Client) parseSavingsPlanDetail(
 	detail *types.SavingsPlansPurchaseRecommendationDetail,
 	params *common.RecommendationParams,
 	planType types.SupportedSavingsPlansType,
-) *common.Recommendation {
-	hourlyCommitment := parseOptionalFloat("HourlyCommitmentToPurchase", detail.HourlyCommitmentToPurchase)
-	monthlySavings := parseOptionalFloat("EstimatedMonthlySavingsAmount", detail.EstimatedMonthlySavingsAmount)
-	savingsPercent := parseOptionalFloat("EstimatedSavingsPercentage", detail.EstimatedSavingsPercentage)
-	upfrontCost := parseOptionalFloat("UpfrontCost", detail.UpfrontCost)
+) (*common.Recommendation, error) {
+	hourlyCommitment, err := parseOptionalFloat("HourlyCommitmentToPurchase", detail.HourlyCommitmentToPurchase)
+	if err != nil {
+		return nil, err
+	}
+	monthlySavings, err := parseOptionalFloat("EstimatedMonthlySavingsAmount", detail.EstimatedMonthlySavingsAmount)
+	if err != nil {
+		return nil, err
+	}
+	upfrontCost, err := parseOptionalFloat("UpfrontCost", detail.UpfrontCost)
+	if err != nil {
+		return nil, err
+	}
+
+	// Non-money fields: present-but-unparseable degrades to 0 with a warning
+	// rather than dropping the whole recommendation. The RI path uses the same
+	// 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 := parseOptionalFloat("EstimatedAverageUtilization", detail.EstimatedAverageUtilization)
+	recommendedUtilization := parseOptionalFloatOrWarn("EstimatedAverageUtilization", detail.EstimatedAverageUtilization)
 	// 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;
@@ -242,7 +305,7 @@ func (c *Client) parseSavingsPlanDetail(
 	// monthly_cost + savings + amortized (which is less accurate for SP rows
 	// where monthly_cost reflects only the no-upfront recurring charge, not
 	// the full on-demand baseline). See #303.
-	onDemandCost := parseOptionalFloat("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth
+	onDemandCost := parseOptionalFloatOrWarn("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth
 	if detail.CurrentAverageHourlyOnDemandSpend == nil {
 		// CurrentAverageHourlyOnDemandSpend absent from AWS CE response — onDemandCost
 		// will be 0 and the scheduler's nonZeroPtr will store nil, causing the
@@ -251,17 +314,7 @@ func (c *Client) parseSavingsPlanDetail(
 		log.Printf("WARNING: CurrentAverageHourlyOnDemandSpend is nil for SP recommendation (planType=%s, account=%s) — Effective %% will use reconstruction fallback", planType, aws.ToString(detail.AccountId))
 	}
 
-	planTypeStr := string(planType)
-	switch planType {
-	case types.SupportedSavingsPlansTypeComputeSp:
-		planTypeStr = "Compute"
-	case types.SupportedSavingsPlansTypeEc2InstanceSp:
-		planTypeStr = "EC2Instance"
-	case types.SupportedSavingsPlansTypeSagemakerSp:
-		planTypeStr = "SageMaker"
-	case types.SupportedSavingsPlansTypeDatabaseSp:
-		planTypeStr = "Database"
-	}
+	planTypeStr := spPlanTypeDisplayString(planType)
 
 	accountID := ""
 	if detail.AccountId != nil {
@@ -318,7 +371,7 @@ func (c *Client) parseSavingsPlanDetail(
 			Region:           ec2Fields.region,
 			OfferingID:       ec2Fields.offeringID,
 		},
-	}
+	}, nil
 }
 
 // planTypesForParams resolves which AWS Cost Explorer plan types to query

providers/aws/recommendations/parser_sp_additional_test.go

@@ -149,7 +149,8 @@ func TestParseSavingsPlanDetail(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			rec := client.parseSavingsPlanDetail(tt.detail, &tt.params, tt.planType)
+			rec, err := client.parseSavingsPlanDetail(tt.detail, &tt.params, tt.planType)
+			require.NoError(t, err)
 			require.NotNil(t, rec)
 			if tt.validate != nil {
 				tt.validate(t, rec)
@@ -367,7 +368,8 @@ func TestParseSavingsPlanDetail_OnDemandCost(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			rec := client.parseSavingsPlanDetail(tt.detail, &params, types.SupportedSavingsPlansTypeComputeSp)
+			rec, err := client.parseSavingsPlanDetail(tt.detail, &params, types.SupportedSavingsPlansTypeComputeSp)
+			require.NoError(t, err)
 			require.NotNil(t, rec)
 			assert.InDelta(t, tt.wantOnDemand, rec.OnDemandCost, 0.001,
 				"OnDemandCost should equal CurrentAverageHourlyOnDemandSpend × 730")

providers/aws/recommendations/parser_sp_test.go

@@ -194,7 +194,9 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) {
 				HourlyCommitmentToPurchase:  aws.String("1.0"),
 				EstimatedAverageUtilization: tt.utilizationStr,
 			}
-			rec := client.parseSavingsPlanDetail(detail, &params, types.SupportedSavingsPlansTypeComputeSp)
+			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")
 			require.NotNil(t, rec)
 			assert.Equal(t, tt.wantUtilization, rec.RecommendedUtilization,
 				"SP utilization should be parsed into rec.RecommendedUtilization")
@@ -293,7 +295,8 @@ func TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			rec := client.parseSavingsPlanDetail(tt.detail, params, tt.planType)
+			rec, err := client.parseSavingsPlanDetail(tt.detail, &params, tt.planType)
+			require.NoError(t, err)
 			require.NotNil(t, rec)
 
 			spDetails, ok := rec.Details.(*common.SavingsPlanDetails)
@@ -308,3 +311,54 @@ func TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured(t *testing.T) {
 		})
 	}
 }
+
+// TestParseSavingsPlanDetail_MoneyFieldUnparseable is the M2 regression test:
+// a present-but-unparseable money field (HourlyCommitmentToPurchase,
+// EstimatedMonthlySavingsAmount, UpfrontCost) must return an error, NOT a
+// silently-fabricated $0. Pre-fix, parseOptionalFloat swallowed parse errors
+// for all fields and substituted 0; the fix errors on money fields and
+// uses warn+0 only for non-money fields (percentages/averages).
+func TestParseSavingsPlanDetail_MoneyFieldUnparseable(t *testing.T) {
+	client := &Client{}
+	params := common.RecommendationParams{
+		Service:       common.ServiceSavingsPlansCompute,
+		PaymentOption: "no-upfront",
+		Term:          "1yr",
+	}
+
+	tests := []struct {
+		name   string
+		detail *types.SavingsPlansPurchaseRecommendationDetail
+	}{
+		{
+			name: "unparseable HourlyCommitmentToPurchase",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase: aws.String("not-a-number"),
+			},
+		},
+		{
+			name: "unparseable EstimatedMonthlySavingsAmount",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase:    aws.String("1.0"),
+				EstimatedMonthlySavingsAmount: aws.String("bad-value"),
+			},
+		},
+		{
+			name: "unparseable UpfrontCost",
+			detail: &types.SavingsPlansPurchaseRecommendationDetail{
+				HourlyCommitmentToPurchase: aws.String("1.0"),
+				UpfrontCost:                aws.String("not-a-float"),
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rec, err := client.parseSavingsPlanDetail(tt.detail, &params, types.SupportedSavingsPlansTypeComputeSp)
+			require.Error(t, err,
+				"present-but-unparseable money field must return an error, not a silently-fabricated $0")
+			assert.Nil(t, rec,
+				"rec must be nil when a money field is unparseable")
+		})
+	}
+}

providers/azure/internal/recommendations/converter.go

@@ -367,19 +367,37 @@ func ExpandPaymentVariants(base common.Recommendation) []common.Recommendation {
 	}
 
 	months := termToMonths(base.Term)
-	recurringMonthly := totalReservation / float64(months)
+
+	// RecurringMonthlyCost semantics:
+	//   - nil   : CommitmentCost was absent from the provider response; the
+	//             frontend renders "-" (data not available) rather than "$0".
+	//   - &0.0  : all-upfront variant with a known non-zero CommitmentCost; the
+	//             full charge was already paid upfront, so the recurring charge
+	//             is a known zero.
+	//   - &N    : monthly variant; CommitmentCost spread evenly over term months.
+	//
+	// When CommitmentCost is 0 it means data was absent from the provider, NOT
+	// that the reservation is free. Using float64Ptr(0) in that case fabricates
+	// a non-nil &0.0 that the frontend renders as "$0" instead of "-", which is
+	// incorrect. Guard: only set non-nil pointers when we have real cost data.
+	var upfrontRecurring, monthlyRecurring *float64
+	if totalReservation != 0 {
+		upfrontRecurring = float64Ptr(0)
+		monthly := totalReservation / float64(months)
+		monthlyRecurring = float64Ptr(monthly)
+	}
 
 	allUpfront := base
 	allUpfront.PaymentOption = "upfront"
 	allUpfront.EstimatedSavings = savings
 	allUpfront.SavingsPercentage = savingsPct
-	allUpfront.RecurringMonthlyCost = float64Ptr(0)
+	allUpfront.RecurringMonthlyCost = upfrontRecurring
 
 	noUpfront := base
 	noUpfront.PaymentOption = "monthly"
 	noUpfront.EstimatedSavings = savings
 	noUpfront.SavingsPercentage = savingsPct
-	noUpfront.RecurringMonthlyCost = float64Ptr(recurringMonthly)
+	noUpfront.RecurringMonthlyCost = monthlyRecurring
 
 	return []common.Recommendation{allUpfront, noUpfront}
 }

providers/azure/internal/recommendations/converter_test.go

@@ -397,11 +397,17 @@ func TestExpandPaymentVariants_ZeroOnDemand_NonZeroCommitment(t *testing.T) {
 }
 
 func TestExpandPaymentVariants_ZeroCommitmentCost(t *testing.T) {
-	// Zero reservation total: both variants still emitted; no-upfront monthly = 0.
+	// M1 regression: when CommitmentCost is 0 (absent from provider response),
+	// both variants must carry nil RecurringMonthlyCost so the frontend renders
+	// "-" (data absent) rather than "$0" (known-zero). The pre-fix code
+	// unconditionally set RecurringMonthlyCost = &0.0 for both variants,
+	// fabricating a non-nil pointer even when no cost data was available.
 	variants := ExpandPaymentVariants(baseRec(common.ServiceCompute, "1yr", 50, 0))
 	require.Len(t, variants, 2)
-	require.NotNil(t, variants[1].RecurringMonthlyCost)
-	assert.InDelta(t, 0.0, *variants[1].RecurringMonthlyCost, 1e-9)
+	assert.Nil(t, variants[0].RecurringMonthlyCost,
+		"all-upfront variant: RecurringMonthlyCost must be nil when CommitmentCost is absent")
+	assert.Nil(t, variants[1].RecurringMonthlyCost,
+		"no-upfront variant: RecurringMonthlyCost must be nil when CommitmentCost is absent")
 }
 
 func TestExpandPaymentVariants_SharedFieldsCarriedThrough(t *testing.T) {

providers/azure/services/cosmosdb/client.go

@@ -27,6 +27,13 @@ import (
 	"github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations"
 )
 
+// reservationResourceTypeCosmosDB is the canonical resourceType value for
+// Azure Cosmos DB in the Consumption API $filter.
+// Source: Azure REST API spec for Microsoft.Consumption/reservationRecommendations
+// (2021-10-01 stable). The previous hand-written value "CosmosDb" has wrong
+// case; the correct value is "CosmosDB".
+const reservationResourceTypeCosmosDB = "CosmosDB"
+
 // maxRecsPages caps Consumption API recommendation pagination.
 const maxRecsPages = 10
 
@@ -166,7 +173,7 @@ func (c *CosmosDBClient) GetRecommendations(ctx context.Context, params common.R
 		// filter — see the parallel comment in compute/client.go for the
 		// failure mode that the wrong shape produced.
 		scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID)
-		filter := "properties/scope eq 'Shared' and properties/resourceType eq 'CosmosDb'"
+		filter := "properties/scope eq 'Shared' and properties/resourceType eq '" + reservationResourceTypeCosmosDB + "'"
 		pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter})
 	}
 

providers/azure/services/cosmosdb/client_test.go

@@ -1317,3 +1317,15 @@ func TestCosmosDBClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t
 	assert.Regexp(t, `^cosmos-`, capturedDisplayName)
 	assert.Contains(t, capturedDisplayName, "EnableCassandra")
 }
+
+// TestReservationResourceTypeCosmosDB_IsCanonical is the H2c regression test.
+//
+// The Consumption ReservationRecommendations API $filter for Azure Cosmos DB
+// requires exactly "CosmosDB" (uppercase DB). The pre-fix code used the
+// hand-written string "CosmosDb" (lowercase d) which is not a valid resourceType
+// enum member, causing the API to return no recommendations.
+func TestReservationResourceTypeCosmosDB_IsCanonical(t *testing.T) {
+	assert.Equal(t, "CosmosDB", reservationResourceTypeCosmosDB,
+		"Cosmos DB filter value must be the Azure REST API canonical enum \"CosmosDB\"; "+
+			"\"CosmosDb\" (pre-fix) is not a valid resourceType")
+}

providers/azure/services/database/client.go

@@ -26,6 +26,13 @@ import (
 	"github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations"
 )
 
+// reservationResourceTypeSQLDB is the canonical resourceType value for Azure
+// SQL Databases in the Consumption API $filter.
+// Source: Azure REST API spec for Microsoft.Consumption/reservationRecommendations
+// (2021-10-01 stable). The previous hand-written value "SqlDatabase" is not a
+// valid enum member; the correct case is "SQLDatabases".
+const reservationResourceTypeSQLDB = "SQLDatabases"
+
 // maxRecsPages caps Consumption API recommendation pagination.
 const maxRecsPages = 10
 
@@ -171,7 +178,7 @@ func (c *DatabaseClient) GetRecommendations(ctx context.Context, params common.R
 		// filter — see the parallel comment in compute/client.go for the
 		// failure mode that the wrong shape produced.
 		scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID)
-		filter := "properties/scope eq 'Shared' and properties/resourceType eq 'SqlDatabase'"
+		filter := "properties/scope eq 'Shared' and properties/resourceType eq '" + reservationResourceTypeSQLDB + "'"
 		pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter})
 	}
 

providers/azure/services/database/client_test.go

@@ -1298,3 +1298,15 @@ func TestDatabaseClient_PurchaseCommitment_CanonicalReservedResourceType(t *test
 		"reservedResourceType %q must be a member of armreservations.PossibleReservedResourceTypeValues()", capturedRRT)
 	mockHTTP.AssertExpectations(t)
 }
+
+// TestReservationResourceTypeSQLDB_IsCanonical is the H2b regression test.
+//
+// The Consumption ReservationRecommendations API $filter for Azure SQL Databases
+// requires exactly "SQLDatabases" (uppercase SQL, plural). The pre-fix code used
+// the hand-written string "SqlDatabase" (lowercase SQL, singular) which is not a
+// valid resourceType enum member, causing the API to return no recommendations.
+func TestReservationResourceTypeSQLDB_IsCanonical(t *testing.T) {
+	assert.Equal(t, "SQLDatabases", reservationResourceTypeSQLDB,
+		"SQL Database filter value must be the Azure REST API canonical enum \"SQLDatabases\"; "+
+			"\"SqlDatabase\" (pre-fix) is not a valid resourceType")
+}