From b93a02a06d40fdd17d128c55de19011f9a95a0d4 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 03:10:57 -0700
Subject: [PATCH 01/10] fix(gcp): wire Count/pricing into GCP converters;
 refuse fabricated prices

Closes #1022. Addresses the GCP portion of #1020.

**C1 (issue #1022):** `convertGCPRecommendation` (all 4 services) never set
`Count`, causing `buildInsertRequest` to request a 0-vCPU / 0-MB commitment.
Now extracts the vCPU count from the Recommender operation's structpb numeric
value (or falls back to the overview `numericValue`). `buildInsertRequest` and
`PurchaseCommitment` now refuse `rec.Count <= 0` with an explicit error before
reaching the GCP API.

**C2 (issue #1022):** All four `convertGCPRecommendation` implementations now
call the service-specific pricing function to populate `CommitmentCost`,
`OnDemandCost`, `SavingsPercentage`, and `BreakEvenMonths`. A scorer with any
`MinSavingsPct` filter will no longer silently drop all GCP recommendations.
Pricing failures are logged and the recommendation is still returned (the
Recommender-derived `EstimatedSavings` is the authoritative figure).

**M4 (issue #1020, GCP side):** All four `get*Pricing` functions previously
fabricated a commitment price from hardcoded discount multipliers when no
commitment SKU existed in the catalog, surfacing it as a real `OfferingDetails`
figure. Following the `managedredis` precedent, these now return an error
("no commitment pricing found") instead of a fabricated number, consistent with
`feedback_nullable_not_zero` and `feedback_empty_string_vs_error`.

**M5 (issue #1022):** `PaymentOption` is now derived from
`RecommendationParams.PaymentOption` (with a service-appropriate default when
empty) rather than being hardcoded per service.

**H2 (cloudstorage):** Fixed the iterator error swallow (`break` on error ->
`return nil, err`) to match the other three service clients.

Regression tests added in `computeengine/client_test.go`:
- `TestConverterToInsert_CountNonZero_VCPUAmountSet`: realistic Recommender
  payload -> converter -> buildInsertRequest -> VCPU Amount > 0.
- `TestConverterFillsPricingForScorer`: converter fills SavingsPercentage;
  scorer with MinSavingsPct=5 passes the recommendation.
- `TestBuildInsertRequest_RefusesZeroCount`: Count <= 0 -> error, no Insert.
- `TestGetComputePricing_NoCommitmentSKUReturnsError`: missing commitment SKU
  -> error (no fabricated price).

Related: #718, #829 (payment-option monthly stamp).
---
 providers/gcp/services/cloudsql/client.go     |  73 +++--
 .../gcp/services/cloudsql/client_test.go      |  96 ++++---
 providers/gcp/services/cloudstorage/client.go |  75 +++--
 .../gcp/services/cloudstorage/client_test.go  | 156 +++++-----
 .../gcp/services/computeengine/client.go      | 154 ++++++++--
 .../gcp/services/computeengine/client_test.go | 271 +++++++++++++++++-
 providers/gcp/services/memorystore/client.go  |  58 ++--
 .../gcp/services/memorystore/client_test.go   |  43 ++-
 8 files changed, 704 insertions(+), 222 deletions(-)

diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go
index b164f0310..ed3de8fff 100644
--- a/providers/gcp/services/cloudsql/client.go
+++ b/providers/gcp/services/cloudsql/client.go
@@ -4,6 +4,7 @@ package cloudsql
 import (
 	"context"
 	"fmt"
+	"log"
 	"strings"
 	"time"
 
@@ -181,7 +182,7 @@ func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.R
 			return nil, fmt.Errorf("cloudsql: iterate recommendations: %w", err)
 		}
 
-		converted := c.convertGCPRecommendation(ctx, rec)
+		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
 		}
@@ -362,7 +363,9 @@ type SQLPricing struct {
 	SavingsPercentage float64
 }
 
-// getSQLPricing gets pricing from GCP Cloud Billing Catalog API
+// getSQLPricing gets pricing from GCP Cloud Billing Catalog API.
+// It returns an error when commitment pricing is absent from the catalog rather
+// than fabricating a price from a hardcoded discount factor (issue #1020).
 func (c *CloudSQLClient) getSQLPricing(ctx context.Context, tier, region string, termYears int) (*SQLPricing, error) {
 	svc, err := c.getOrCreateBillingService(ctx)
 	if err != nil {
@@ -374,13 +377,15 @@ func (c *CloudSQLClient) getSQLPricing(ctx context.Context, tier, region string,
 		return nil, fmt.Errorf("failed to list SKUs: %w", err)
 	}
 
-	onDemandPrice, currency := extractSQLPricingFromSKUs(skus.Skus, tier, region)
+	onDemandPrice, commitmentPrice, currency := extractSQLPricingFromSKUs(skus.Skus, tier, region)
 	if onDemandPrice == 0 {
 		return nil, fmt.Errorf("no pricing found for Cloud SQL tier %s", tier)
 	}
+	if commitmentPrice == 0 {
+		return nil, fmt.Errorf("no commitment pricing found for Cloud SQL tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region)
+	}
 
 	hoursInTerm := 8760.0 * float64(termYears)
-	commitmentPrice := estimateSQLCommitmentPrice(onDemandPrice, hoursInTerm, termYears)
 	savingsPercentage := calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
 
 	return &SQLPricing{
@@ -406,8 +411,9 @@ func (c *CloudSQLClient) getOrCreateBillingService(ctx context.Context) (Billing
 	return &realBillingService{service: service}, nil
 }
 
-// extractSQLPricingFromSKUs extracts on-demand pricing from SKU list
-func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDemand float64, currency string) {
+// extractSQLPricingFromSKUs extracts on-demand and commitment pricing from the SKU list.
+// Cloud SQL committed-use discounts are surfaced as "commitment" SKUs in the billing catalog.
+func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDemand, commitment float64, currency string) {
 	currency = "USD"
 
 	for _, sku := range skus {
@@ -424,11 +430,14 @@ func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (o
 			currency = curr
 		}
 
-		// Cloud SQL doesn't have separate commitment pricing in the API
-		onDemand = price
+		if strings.Contains(strings.ToLower(sku.Description), "commitment") {
+			commitment = price
+		} else {
+			onDemand = price
+		}
 	}
 
-	return onDemand, currency
+	return onDemand, commitment, currency
 }
 
 // extractSQLPriceFromSKU extracts the unit price from a SKU
@@ -451,15 +460,6 @@ func extractSQLPriceFromSKU(sku *cloudbilling.Sku) (float64, string) {
 	return price, rate.UnitPrice.CurrencyCode
 }
 
-// estimateSQLCommitmentPrice estimates commitment price based on GCP SQL package savings
-func estimateSQLCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 {
-	discount := 0.85 // 15% savings for 1 year
-	if termYears == 3 {
-		discount = 0.80 // 20% savings for 3 years
-	}
-	return onDemandPrice * hoursInTerm * discount
-}
-
 // calculateSQLSavingsPercentage calculates the savings percentage
 func calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 {
 	onDemandTotal := onDemandPrice * hoursInTerm
@@ -486,8 +486,16 @@ func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool {
 	return true
 }
 
-// convertGCPRecommendation converts a GCP Recommender recommendation to common format
-func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation {
+// convertGCPRecommendation converts a GCP Recommender recommendation to common format.
+// It also calls getSQLPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
+// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
+// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation.
+func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
+	paymentOption := params.PaymentOption
+	if paymentOption == "" {
+		paymentOption = "monthly"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceRelationalDB,
@@ -496,7 +504,7 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 		CommitmentType: common.CommitmentCUD,
 		Timestamp:      time.Now(),
 		Term:           "1yr",
-		PaymentOption:  "monthly",
+		PaymentOption:  paymentOption,
 	}
 
 	// Extract resource type from recommendation content
@@ -517,7 +525,6 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 
 	// Extract cost impact
 	if gcpRec.PrimaryImpact != nil {
-		// Use GetCostProjection() method to access the cost projection
 		if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil {
 			cost := costProj.Cost
 			savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9)
@@ -525,6 +532,28 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 		}
 	}
 
+	// Thread pricing into the converter so the scorer can rank/filter GCP recs
+	// correctly (issue #1022 C2).
+	if rec.ResourceType != "" {
+		termYears := 1
+		if rec.Term == "3yr" || rec.Term == "3" {
+			termYears = 3
+		}
+		if pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
+			log.Printf("cloudsql: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		} else {
+			rec.CommitmentCost = pricing.CommitmentPrice
+			rec.OnDemandCost = pricing.OnDemandPrice
+			rec.SavingsPercentage = pricing.SavingsPercentage
+			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+				if monthlySavings > 0 {
+					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+				}
+			}
+		}
+	}
+
 	return rec
 }
 
diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go
index 1a6bff726..991a5df40 100644
--- a/providers/gcp/services/cloudsql/client_test.go
+++ b/providers/gcp/services/cloudsql/client_test.go
@@ -500,28 +500,42 @@ func TestCloudSQLClient_PurchaseCommitment_NotSupported(t *testing.T) {
 	assert.False(t, mockService.insertCalled, "PurchaseCommitment must not call InsertInstance")
 }
 
-func TestCloudSQLClient_GetOfferingDetails_WithMock(t *testing.T) {
-	ctx := context.Background()
-	client, _ := NewClient(ctx, "test-project", "us-central1")
-
-	mockService := &MockBillingService{
-		skus: &cloudbilling.ListSkusResponse{
-			Skus: []*cloudbilling.Sku{
-				{
-					Description:    "db-n1-standard-1 Cloud SQL",
-					ServiceRegions: []string{"us-central1"},
-					PricingInfo: []*cloudbilling.PricingInfo{
+// sqlMockSkus returns a slice with both an on-demand and a commitment SKU for
+// the given tier and region. Required by tests that exercise GetOfferingDetails
+// after the issue #1020 fix (fabricated commitment prices are no longer allowed;
+// both SKUs must be present for getSQLPricing to succeed).
+func sqlMockSkus(tier, region string) []*cloudbilling.Sku {
+	onDemandSKU := &cloudbilling.Sku{
+		Description:    tier + " Cloud SQL",
+		ServiceRegions: []string{region},
+		PricingInfo: []*cloudbilling.PricingInfo{
+			{
+				PricingExpression: &cloudbilling.PricingExpression{
+					TieredRates: []*cloudbilling.TierRate{
 						{
-							PricingExpression: &cloudbilling.PricingExpression{
-								TieredRates: []*cloudbilling.TierRate{
-									{
-										UnitPrice: &cloudbilling.Money{
-											Units:        0,
-											Nanos:        50000000, // 0.05 per hour
-											CurrencyCode: "USD",
-										},
-									},
-								},
+							UnitPrice: &cloudbilling.Money{
+								Units:        0,
+								Nanos:        50000000,
+								CurrencyCode: "USD",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	commitmentSKU := &cloudbilling.Sku{
+		Description:    tier + " Cloud SQL commitment 1yr",
+		ServiceRegions: []string{region},
+		PricingInfo: []*cloudbilling.PricingInfo{
+			{
+				PricingExpression: &cloudbilling.PricingExpression{
+					TieredRates: []*cloudbilling.TierRate{
+						{
+							UnitPrice: &cloudbilling.Money{
+								Units:        0,
+								Nanos:        42000000,
+								CurrencyCode: "USD",
 							},
 						},
 					},
@@ -529,6 +543,17 @@ func TestCloudSQLClient_GetOfferingDetails_WithMock(t *testing.T) {
 			},
 		},
 	}
+	return []*cloudbilling.Sku{onDemandSKU, commitmentSKU}
+}
+
+func TestCloudSQLClient_GetOfferingDetails_WithMock(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	// Both on-demand and commitment SKUs are required after the issue #1020 fix.
+	mockService := &MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{Skus: sqlMockSkus("db-n1-standard-1", "us-central1")},
+	}
 	client.SetBillingService(mockService)
 
 	rec := common.Recommendation{
@@ -549,30 +574,9 @@ func TestCloudSQLClient_GetOfferingDetails_3Year(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
+	// Both on-demand and commitment SKUs are required after the issue #1020 fix.
 	mockService := &MockBillingService{
-		skus: &cloudbilling.ListSkusResponse{
-			Skus: []*cloudbilling.Sku{
-				{
-					Description:    "db-n1-standard-1 Cloud SQL",
-					ServiceRegions: []string{"us-central1"},
-					PricingInfo: []*cloudbilling.PricingInfo{
-						{
-							PricingExpression: &cloudbilling.PricingExpression{
-								TieredRates: []*cloudbilling.TierRate{
-									{
-										UnitPrice: &cloudbilling.Money{
-											Units:        0,
-											Nanos:        50000000,
-											CurrencyCode: "USD",
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-		},
+		skus: &cloudbilling.ListSkusResponse{Skus: sqlMockSkus("db-n1-standard-1", "us-central1")},
 	}
 	client.SetBillingService(mockService)
 
@@ -762,7 +766,7 @@ func TestCloudSQLClient_ConvertGCPRecommendation(t *testing.T) {
 		},
 	}
 
-	rec := client.convertGCPRecommendation(ctx, gcpRec)
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 	assert.Equal(t, common.ProviderGCP, rec.Provider)
 	assert.Equal(t, common.ServiceRelationalDB, rec.Service)
@@ -781,7 +785,7 @@ func TestCloudSQLClient_ConvertGCPRecommendation_NilContent(t *testing.T) {
 		Content: nil,
 	}
 
-	rec := client.convertGCPRecommendation(ctx, gcpRec)
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 	assert.Equal(t, common.ProviderGCP, rec.Provider)
 }
diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go
index 0248a6c4f..e4a377136 100644
--- a/providers/gcp/services/cloudstorage/client.go
+++ b/providers/gcp/services/cloudstorage/client.go
@@ -4,6 +4,7 @@ package cloudstorage
 import (
 	"context"
 	"fmt"
+	"log"
 	"strings"
 	"time"
 
@@ -17,6 +18,10 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/common"
 )
 
+// maxRecsPages caps GCP Recommender API iteration to avoid looping forever on a
+// stalled or unexpectedly large result set.
+const maxRecsPages = 20
+
 // StorageService interface for storage operations (enables mocking)
 type StorageService interface {
 	Buckets(ctx context.Context, projectID string) BucketIterator
@@ -179,16 +184,25 @@ func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params comm
 	}
 
 	it := recClient.ListRecommendations(ctx, req)
-	for {
+	for pageIdx := 0; ; pageIdx++ {
+		if err := ctx.Err(); err != nil {
+			return nil, fmt.Errorf("context cancelled during pagination: %w", err)
+		}
+		if pageIdx >= maxRecsPages {
+			return nil, fmt.Errorf("cloudstorage: GetRecommendations iteration cap (%d items) reached", maxRecsPages)
+		}
 		rec, err := it.Next()
 		if err == iterator.Done {
 			break
 		}
 		if err != nil {
-			break
+			// Iterator errors must propagate so callers don't silently act on a
+			// partial recommendation list -- see the computeengine client for the
+			// full rationale (issue #1022 H2 fix).
+			return nil, fmt.Errorf("cloudstorage: iterate recommendations: %w", err)
 		}
 
-		converted := c.convertGCPRecommendation(ctx, rec)
+		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
 		}
@@ -340,7 +354,9 @@ type StoragePricing struct {
 	SavingsPercentage float64
 }
 
-// getStoragePricing gets pricing from GCP Cloud Billing Catalog API
+// getStoragePricing gets pricing from GCP Cloud Billing Catalog API.
+// It returns an error when commitment pricing is absent from the catalog rather
+// than fabricating a price from a hardcoded discount factor (issue #1020).
 func (c *CloudStorageClient) getStoragePricing(ctx context.Context, storageClass, region string, termYears int) (*StoragePricing, error) {
 	svc, err := c.getOrCreateBillingService(ctx)
 	if err != nil {
@@ -356,12 +372,11 @@ func (c *CloudStorageClient) getStoragePricing(ctx context.Context, storageClass
 	if onDemandPrice == 0 {
 		return nil, fmt.Errorf("no pricing found for Cloud Storage class %s", storageClass)
 	}
-
-	hoursInTerm := 8760.0 * float64(termYears)
 	if commitmentPrice == 0 {
-		commitmentPrice = estimateStorageCommitmentPrice(onDemandPrice, hoursInTerm, termYears)
+		return nil, fmt.Errorf("no commitment pricing found for Cloud Storage class %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", storageClass, region)
 	}
 
+	hoursInTerm := 8760.0 * float64(termYears)
 	savingsPercentage := calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
 
 	return &StoragePricing{
@@ -435,15 +450,6 @@ func extractStoragePriceFromSKU(sku *cloudbilling.Sku) (float64, string) {
 	return price, rate.UnitPrice.CurrencyCode
 }
 
-// estimateStorageCommitmentPrice estimates commitment price based on GCP storage savings
-func estimateStorageCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 {
-	discount := 0.75 // 25% savings for 1 year
-	if termYears == 3 {
-		discount = 0.70 // 30% savings for 3 years
-	}
-	return onDemandPrice * hoursInTerm * discount
-}
-
 // calculateStorageSavingsPercentage calculates the savings percentage
 func calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 {
 	onDemandTotal := onDemandPrice * hoursInTerm
@@ -470,8 +476,16 @@ func skuMatchesStorageClass(sku *cloudbilling.Sku, storageClass, region string)
 	return true
 }
 
-// convertGCPRecommendation converts a GCP Recommender recommendation to common format
-func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation {
+// convertGCPRecommendation converts a GCP Recommender recommendation to common format.
+// It also calls getStoragePricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
+// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
+// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation.
+func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
+	paymentOption := params.PaymentOption
+	if paymentOption == "" {
+		paymentOption = "monthly"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceStorage,
@@ -480,7 +494,7 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 		CommitmentType: common.CommitmentReservedCapacity,
 		Timestamp:      time.Now(),
 		Term:           "1yr",
-		PaymentOption:  "monthly",
+		PaymentOption:  paymentOption,
 	}
 
 	// Extract resource type from recommendation content
@@ -501,7 +515,6 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 
 	// Extract cost impact
 	if gcpRec.PrimaryImpact != nil {
-		// Use GetCostProjection() method to access the cost projection
 		if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil {
 			cost := costProj.Cost
 			savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9)
@@ -509,5 +522,27 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 		}
 	}
 
+	// Thread pricing into the converter so the scorer can rank/filter GCP recs
+	// correctly (issue #1022 C2).
+	if rec.ResourceType != "" {
+		termYears := 1
+		if rec.Term == "3yr" || rec.Term == "3" {
+			termYears = 3
+		}
+		if pricing, err := c.getStoragePricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
+			log.Printf("cloudstorage: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		} else {
+			rec.CommitmentCost = pricing.CommitmentPrice
+			rec.OnDemandCost = pricing.OnDemandPrice
+			rec.SavingsPercentage = pricing.SavingsPercentage
+			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+				if monthlySavings > 0 {
+					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+				}
+			}
+		}
+	}
+
 	return rec
 }
diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go
index 9ee3c648b..0b1ebc400 100644
--- a/providers/gcp/services/cloudstorage/client_test.go
+++ b/providers/gcp/services/cloudstorage/client_test.go
@@ -460,32 +460,51 @@ func TestCloudStorageClient_GetRecommendations_IteratorError(t *testing.T) {
 	}
 	client.SetRecommenderClient(mockClient)
 
+	// Iterator errors now propagate (issue #1022 H2 fix) -- they must not be
+	// silently swallowed, as that would mask auth/quota failures and cause callers
+	// to act on a partial (empty) recommendation list.
 	recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{})
-	require.NoError(t, err) // Error during iteration is handled gracefully
-	assert.Empty(t, recommendations)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "cloudstorage: iterate recommendations")
+	assert.Nil(t, recommendations)
 }
 
-func TestCloudStorageClient_GetOfferingDetails_WithMock(t *testing.T) {
-	ctx := context.Background()
-	client, _ := NewClient(ctx, "test-project", "us-central1")
-
-	mockService := &MockBillingService{
-		skus: &cloudbilling.ListSkusResponse{
-			Skus: []*cloudbilling.Sku{
+// storageMockSkus returns a slice with both an on-demand and a commitment SKU for
+// the given storage class and region. Required by tests that exercise GetOfferingDetails
+// after the issue #1020 fix (fabricated commitment prices are no longer allowed).
+func storageMockSkus(storageClass, region string, onDemandNanos, commitmentNanos int64) []*cloudbilling.Sku {
+	return []*cloudbilling.Sku{
+		{
+			Description:    storageClass + " Storage in " + region,
+			ServiceRegions: []string{region},
+			PricingInfo: []*cloudbilling.PricingInfo{
 				{
-					Description:    "Standard Storage in us-central1",
-					ServiceRegions: []string{"us-central1"},
-					PricingInfo: []*cloudbilling.PricingInfo{
-						{
-							PricingExpression: &cloudbilling.PricingExpression{
-								TieredRates: []*cloudbilling.TierRate{
-									{
-										UnitPrice: &cloudbilling.Money{
-											Units:        0,
-											Nanos:        26000000,
-											CurrencyCode: "USD",
-										},
-									},
+					PricingExpression: &cloudbilling.PricingExpression{
+						TieredRates: []*cloudbilling.TierRate{
+							{
+								UnitPrice: &cloudbilling.Money{
+									Units:        0,
+									Nanos:        onDemandNanos,
+									CurrencyCode: "USD",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			Description:    storageClass + " Storage commitment in " + region,
+			ServiceRegions: []string{region},
+			PricingInfo: []*cloudbilling.PricingInfo{
+				{
+					PricingExpression: &cloudbilling.PricingExpression{
+						TieredRates: []*cloudbilling.TierRate{
+							{
+								UnitPrice: &cloudbilling.Money{
+									Units:        0,
+									Nanos:        commitmentNanos,
+									CurrencyCode: "USD",
 								},
 							},
 						},
@@ -494,6 +513,18 @@ func TestCloudStorageClient_GetOfferingDetails_WithMock(t *testing.T) {
 			},
 		},
 	}
+}
+
+func TestCloudStorageClient_GetOfferingDetails_WithMock(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	// Both on-demand and commitment SKUs required after the issue #1020 fix.
+	mockService := &MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{
+			Skus: storageMockSkus("STANDARD", "us-central1", 26000000, 19500000),
+		},
+	}
 	client.SetBillingService(mockService)
 
 	rec := common.Recommendation{
@@ -516,29 +547,10 @@ func TestCloudStorageClient_GetOfferingDetails_3yr(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
+	// Both on-demand and commitment SKUs required after the issue #1020 fix.
 	mockService := &MockBillingService{
 		skus: &cloudbilling.ListSkusResponse{
-			Skus: []*cloudbilling.Sku{
-				{
-					Description:    "Nearline Storage in us-central1",
-					ServiceRegions: []string{"us-central1"},
-					PricingInfo: []*cloudbilling.PricingInfo{
-						{
-							PricingExpression: &cloudbilling.PricingExpression{
-								TieredRates: []*cloudbilling.TierRate{
-									{
-										UnitPrice: &cloudbilling.Money{
-											Units:        0,
-											Nanos:        10000000,
-											CurrencyCode: "USD",
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
+			Skus: storageMockSkus("NEARLINE", "us-central1", 10000000, 7000000),
 		},
 	}
 	client.SetBillingService(mockService)
@@ -599,29 +611,10 @@ func TestCloudStorageClient_GetOfferingDetails_DefaultPaymentOption(t *testing.T
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
+	// Both on-demand and commitment SKUs required after the issue #1020 fix.
 	mockService := &MockBillingService{
 		skus: &cloudbilling.ListSkusResponse{
-			Skus: []*cloudbilling.Sku{
-				{
-					Description:    "Standard Storage in us-central1",
-					ServiceRegions: []string{"us-central1"},
-					PricingInfo: []*cloudbilling.PricingInfo{
-						{
-							PricingExpression: &cloudbilling.PricingExpression{
-								TieredRates: []*cloudbilling.TierRate{
-									{
-										UnitPrice: &cloudbilling.Money{
-											Units:        0,
-											Nanos:        26000000,
-											CurrencyCode: "USD",
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
+			Skus: storageMockSkus("STANDARD", "us-central1", 26000000, 19500000),
 		},
 	}
 	client.SetBillingService(mockService)
@@ -666,7 +659,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation(t *testing.T) {
 		},
 	}
 
-	rec := client.convertGCPRecommendation(ctx, gcpRec)
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 	assert.Equal(t, common.ProviderGCP, rec.Provider)
 	assert.Equal(t, common.ServiceStorage, rec.Service)
@@ -686,7 +679,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation_NilContent(t *testing.T) {
 		Content: nil,
 	}
 
-	rec := client.convertGCPRecommendation(ctx, gcpRec)
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 	assert.Equal(t, common.ProviderGCP, rec.Provider)
 	assert.Empty(t, rec.ResourceType)
@@ -710,7 +703,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation_NilPrimaryImpact(t *testing
 		},
 	}
 
-	rec := client.convertGCPRecommendation(ctx, gcpRec)
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 	assert.Equal(t, float64(0), rec.EstimatedSavings)
 }
@@ -777,37 +770,20 @@ func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
+	// Both on-demand and commitment SKUs required after the issue #1020 fix:
+	// without a commitment SKU, getStoragePricing returns an error.
 	mockService := &MockBillingService{
 		skus: &cloudbilling.ListSkusResponse{
-			Skus: []*cloudbilling.Sku{
-				{
-					Description:    "Standard Storage in us-central1",
-					ServiceRegions: []string{"us-central1"},
-					PricingInfo: []*cloudbilling.PricingInfo{
-						{
-							PricingExpression: &cloudbilling.PricingExpression{
-								TieredRates: []*cloudbilling.TierRate{
-									{
-										UnitPrice: &cloudbilling.Money{
-											Units:        0,
-											Nanos:        26000000,
-											CurrencyCode: "USD",
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
+			Skus: storageMockSkus("STANDARD", "us-central1", 26000000, 18200000),
 		},
 	}
 	client.SetBillingService(mockService)
 
 	pricing, err := client.getStoragePricing(ctx, "STANDARD", "us-central1", 3)
 	require.NoError(t, err)
-	// 3-year should have 30% savings vs 25% for 1-year
-	assert.Greater(t, pricing.SavingsPercentage, float64(25))
+	assert.Greater(t, pricing.SavingsPercentage, float64(0))
+	assert.Greater(t, pricing.OnDemandPrice, float64(0))
+	assert.Greater(t, pricing.CommitmentPrice, float64(0))
 }
 
 func TestSkuMatchesStorageClass_CaseInsensitive(t *testing.T) {
diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index 7e598af71..47a305ffd 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -17,6 +17,7 @@ import (
 	"google.golang.org/api/cloudbilling/v1"
 	"google.golang.org/api/iterator"
 	"google.golang.org/api/option"
+	"google.golang.org/protobuf/types/known/structpb"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
 	"github.com/LeanerCloud/CUDly/pkg/retry"
@@ -234,7 +235,7 @@ func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, params com
 			return nil, fmt.Errorf("computeengine: iterate recommendations: %w", err)
 		}
 
-		converted := c.convertGCPRecommendation(ctx, rec)
+		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
 		}
@@ -464,7 +465,11 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common
 	}
 	defer svc.Close()
 
-	insertReq, commitmentName := c.buildInsertRequest(rec, opts)
+	insertReq, commitmentName, buildErr := c.buildInsertRequest(rec, opts)
+	if buildErr != nil {
+		result.Error = buildErr
+		return result, buildErr
+	}
 
 	// Exponential backoff on RESOURCE_EXHAUSTED: BaseDelay 1s with 2× growth
 	// capped at MaxDelay 4s gives the same 1s/2s/4s sequence the open-coded
@@ -508,8 +513,9 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common
 // buildInsertRequest assembles the RegionCommitments.Insert request for a
 // purchase, threading opts.IdempotencyToken into both GCP idempotency levers so
 // a re-drive of the same execution cannot create a second CUD (issue #654, the
-// financial double-buy). It returns the request and the commitment name (used as
-// the resulting CommitmentID).
+// financial double-buy). It returns the request, the commitment name (used as
+// the resulting CommitmentID), and an error when rec.Count <= 0 (which would
+// produce a zero-vCPU / zero-MB commitment that GCP rejects or silently wastes).
 //
 //   - RequestId is GCP's native server-side idempotency key on Insert, which the
 //     API documents as preventing clients from accidentally creating duplicate
@@ -526,7 +532,11 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common
 // An empty token preserves the prior non-idempotent timestamp-based name (the
 // CLI path, which has no owning execution). The token is masked in logs via
 // common.MaskToken and never logged verbatim.
-func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts common.PurchaseOptions) (*computepb.InsertRegionCommitmentRequest, string) {
+func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts common.PurchaseOptions) (*computepb.InsertRegionCommitmentRequest, string, error) {
+	if rec.Count <= 0 {
+		return nil, "", fmt.Errorf("buildInsertRequest: rec.Count must be > 0 (got %d); a zero-vCPU commitment is invalid (issue #1022)", rec.Count)
+	}
+
 	plan := "TWELVE_MONTH"
 	if rec.Term == "3yr" || rec.Term == "3" {
 		plan = "THIRTY_SIX_MONTH"
@@ -572,7 +582,7 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 			common.MaskToken(opts.IdempotencyToken), commitmentName)
 	}
 
-	return insertReq, commitmentName
+	return insertReq, commitmentName, nil
 }
 
 // ValidateOffering validates that a machine type exists
@@ -689,7 +699,9 @@ type ComputePricing struct {
 	SavingsPercentage float64
 }
 
-// getComputePricing gets pricing from GCP Cloud Billing Catalog API
+// getComputePricing gets pricing from GCP Cloud Billing Catalog API.
+// It returns an error when commitment pricing is absent from the catalog rather
+// than fabricating a price from a hardcoded discount factor (issue #1020).
 func (c *ComputeEngineClient) getComputePricing(ctx context.Context, machineType, region string, termYears int) (*ComputePricing, error) {
 	svc, err := c.getOrCreateBillingService(ctx)
 	if err != nil {
@@ -705,12 +717,11 @@ func (c *ComputeEngineClient) getComputePricing(ctx context.Context, machineType
 	if onDemandPrice == 0 {
 		return nil, fmt.Errorf("no on-demand pricing found for machine type %s", machineType)
 	}
-
-	hoursInTerm := 8760.0 * float64(termYears)
 	if commitmentPrice == 0 {
-		commitmentPrice = estimateComputeCommitmentPrice(onDemandPrice, hoursInTerm, termYears)
+		return nil, fmt.Errorf("no commitment pricing found for machine type %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", machineType, region)
 	}
 
+	hoursInTerm := 8760.0 * float64(termYears)
 	savingsPercentage := calculateComputeSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
 
 	return &ComputePricing{
@@ -784,15 +795,6 @@ func extractComputePriceFromSKU(sku *cloudbilling.Sku) (float64, string) {
 	return price, rate.UnitPrice.CurrencyCode
 }
 
-// estimateComputeCommitmentPrice estimates commitment price based on GCP CUD discounts
-func estimateComputeCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 {
-	discount := 0.63 // 37% savings for 1 year
-	if termYears == 3 {
-		discount = 0.45 // 55% savings for 3 years
-	}
-	return onDemandPrice * hoursInTerm * discount
-}
-
 // calculateComputeSavingsPercentage calculates the savings percentage
 func calculateComputeSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 {
 	onDemandTotal := onDemandPrice * hoursInTerm
@@ -819,8 +821,17 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo
 	return true
 }
 
-// convertGCPRecommendation converts a GCP Recommender recommendation to common format
-func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation {
+// convertGCPRecommendation converts a GCP Recommender recommendation to common format.
+// It also calls getComputePricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
+// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
+// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation:
+// EstimatedSavings from the Recommender payload is the authoritative savings signal.
+func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
+	paymentOption := params.PaymentOption
+	if paymentOption == "" {
+		paymentOption = "upfront"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceCompute,
@@ -829,11 +840,40 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR
 		CommitmentType: common.CommitmentCUD,
 		Timestamp:      time.Now(),
 		Term:           "1yr",
-		PaymentOption:  "upfront",
+		PaymentOption:  paymentOption,
 	}
 
 	extractResourceTypeFromRecommendation(gcpRec, rec)
 	extractCostImpactFromRecommendation(gcpRec, rec)
+	extractVCPUCountFromRecommendation(gcpRec, rec)
+
+	// Thread pricing into the converter so the scorer can rank/filter GCP recs
+	// correctly (issue #1022 C2). If the billing catalog lacks a commitment SKU
+	// we propagate the error as a log line rather than dropping the rec entirely
+	// -- the Recommender-derived EstimatedSavings is still valid.
+	if rec.ResourceType != "" {
+		termYears := 1
+		if rec.Term == "3yr" || rec.Term == "3" {
+			termYears = 3
+		}
+		if pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
+			log.Printf("computeengine: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		} else {
+			rec.CommitmentCost = pricing.CommitmentPrice
+			rec.OnDemandCost = pricing.OnDemandPrice
+			rec.SavingsPercentage = pricing.SavingsPercentage
+			// BreakEvenMonths: months of accrued savings required to cover the
+			// commitment cost (conservative: treats the whole period cost as sunk,
+			// which is accurate for all-upfront and overestimates for monthly CUDs).
+			// monthlySavings = monthly spend difference between on-demand and CUD.
+			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+				if monthlySavings > 0 {
+					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+				}
+			}
+		}
+	}
 
 	return rec
 }
@@ -889,6 +929,76 @@ func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, r
 	}
 }
 
+// extractVCPUCountFromRecommendation extracts the recommended vCPU count from a
+// GCP Commitment Recommender response (issue #1022 C1).
+//
+// The Recommender encodes the commitment resource amounts in two places:
+//   - Operation.Value: a structpb.Value whose numeric value is the amount, with
+//     Operation.Path indicating the resource type (e.g. "/resources/0/amount").
+//     Operations with ResourceType "compute.googleapis.com/Commitment" and a path
+//     containing "amount" carry the VCPU count; the sibling MEMORY_MB amount is
+//     always 4096 * vcpuCount for general-purpose commitments.
+//   - RecommendationContent.Overview: a JSON struct with a "numericValue" field.
+//
+// We prefer the operation-value path because it is structured and unambiguous.
+// If no VCPU operation is found we fall back to the overview's numericValue.
+func extractVCPUCountFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) {
+	if gcpRec.Content == nil {
+		return
+	}
+
+	// Walk operations looking for a VCPU resource amount.
+	for _, opGroup := range gcpRec.Content.GetOperationGroups() {
+		for _, op := range opGroup.GetOperations() {
+			// The commitment recommender uses ResourceType
+			// "compute.googleapis.com/Commitment" with paths like
+			// "/resources/0/amount" (VCPU) and "/resources/1/amount" (MEMORY_MB).
+			// We detect VCPU by checking that the path does NOT mention "memory"
+			// and that a sibling "/resources/0/type" is "VCPU" (encoded in pathFilters).
+			if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") {
+				continue
+			}
+			path := strings.ToLower(op.GetPath())
+			if !strings.Contains(path, "amount") {
+				continue
+			}
+			// Skip the memory amount: the path filter or a nearby type operation
+			// distinguishes VCPU from MEMORY_MB. The VCPU amount is always first
+			// ("/resources/0/amount") in the canonical recommender output; if
+			// path_filters carry a "type":"VCPU" entry we can check that directly.
+			isMemory := false
+			for filterKey, filterVal := range op.GetPathFilters() {
+				if strings.Contains(strings.ToLower(filterKey), "type") {
+					if sv, ok := filterVal.GetKind().(*structpb.Value_StringValue); ok {
+						if strings.EqualFold(sv.StringValue, "MEMORY_MB") {
+							isMemory = true
+							break
+						}
+					}
+				}
+			}
+			if isMemory {
+				continue
+			}
+			if v := op.GetValue(); v != nil {
+				if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 {
+					rec.Count = int(nv.NumberValue)
+					return
+				}
+			}
+		}
+	}
+
+	// Fallback: overview numericValue (used by older recommender versions).
+	if gcpRec.Content.GetOverview() != nil {
+		if nv := gcpRec.Content.GetOverview().GetFields()["numericValue"]; nv != nil {
+			if count := nv.GetNumberValue(); count > 0 {
+				rec.Count = int(count)
+			}
+		}
+	}
+}
+
 // idempotentNameTokenLen is how many leading hex characters of the idempotency
 // token are folded into the derived commitment name. A GCP commitment name must
 // match RFC1035 (1-63 chars, lowercase [a-z]([-a-z0-9]*[a-z0-9])?); the "cud-"
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index 74ce48896..fdf2ea098 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -13,8 +13,10 @@ import (
 	"google.golang.org/api/cloudbilling/v1"
 	"google.golang.org/api/iterator"
 	"google.golang.org/genproto/googleapis/type/money"
+	"google.golang.org/protobuf/types/known/structpb"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
+	"github.com/LeanerCloud/CUDly/pkg/scorer"
 )
 
 // MockCommitmentsService mocks the CommitmentsService interface
@@ -590,6 +592,7 @@ func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) {
 	rec := common.Recommendation{
 		ResourceType: "n1-standard-1",
 		Term:         "3yr",
+		Count:        4, // must be > 0 after issue #1022 guard
 	}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
@@ -606,7 +609,8 @@ func TestComputeEngineClient_PurchaseCommitment_InsertError(t *testing.T) {
 	}
 	client.SetCommitmentsService(mockService)
 
-	rec := common.Recommendation{ResourceType: "n1-standard-1"}
+	// Count must be > 0 so the guard passes and the insert error is exercised.
+	rec := common.Recommendation{ResourceType: "n1-standard-1", Count: 2}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
 	assert.Error(t, err)
@@ -623,7 +627,8 @@ func TestComputeEngineClient_PurchaseCommitment_WaitError(t *testing.T) {
 	}
 	client.SetCommitmentsService(mockService)
 
-	rec := common.Recommendation{ResourceType: "n1-standard-1"}
+	// Count must be > 0 so the guard passes and the wait error is exercised.
+	rec := common.Recommendation{ResourceType: "n1-standard-1", Count: 2}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
 	assert.Error(t, err)
@@ -635,6 +640,8 @@ func TestComputeEngineClient_GetOfferingDetails_WithMock(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
+	// Both on-demand and commitment SKUs are required; without a commitment SKU
+	// getComputePricing now returns an error (issue #1020 fix).
 	mockService := &MockBillingService{
 		skus: &cloudbilling.ListSkusResponse{
 			Skus: []*cloudbilling.Sku{
@@ -657,6 +664,25 @@ func TestComputeEngineClient_GetOfferingDetails_WithMock(t *testing.T) {
 						},
 					},
 				},
+				{
+					Description:    "n1-standard-1 commitment 1yr in Americas",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        32000000,
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
 			},
 		},
 	}
@@ -831,7 +857,7 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) {
 		},
 	}
 
-	rec := client.convertGCPRecommendation(ctx, gcpRec)
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 	assert.Equal(t, common.ProviderGCP, rec.Provider)
 	assert.Equal(t, common.ServiceCompute, rec.Service)
@@ -883,3 +909,242 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) {
 	_, err = client.GetRecommendations(context.Background(), common.RecommendationParams{})
 	require.Error(t, err, "page cap must surface an error when the iterator never terminates")
 }
+
+// realisticCUDRecommendation builds a realistic GCP Commitment Recommender payload
+// for a 4-vCPU n1-standard-4 commitment in us-central1. The operation group has two ops:
+//  1. A machine-type op whose resource path ends in the machine type (n1-standard-4) --
+//     used by extractResourceTypeFromOperations to set rec.ResourceType.
+//  2. A commitment resource op whose numeric Value is the VCPU count --
+//     used by extractVCPUCountFromRecommendation to set rec.Count.
+//
+// This mirrors the GCP CUD Recommender format (issue #1022 C1).
+func realisticCUDRecommendation() *recommenderpb.Recommendation {
+	vcpuVal, _ := structpb.NewValue(4.0)
+	return &recommenderpb.Recommendation{
+		Name: "projects/test/locations/us-central1/recommenders/google.billing.CostInsight.commitmentRecommender/recommendations/rec-001",
+		PrimaryImpact: &recommenderpb.Impact{
+			Category: recommenderpb.Impact_COST,
+			Projection: &recommenderpb.Impact_CostProjection{
+				CostProjection: &recommenderpb.CostProjection{
+					Cost: &money.Money{
+						Units:        -200,
+						Nanos:        0,
+						CurrencyCode: "USD",
+					},
+				},
+			},
+		},
+		Content: &recommenderpb.RecommendationContent{
+			OperationGroups: []*recommenderpb.OperationGroup{
+				{
+					Operations: []*recommenderpb.Operation{
+						{
+							// Machine type op: resource path ends in the machine type name,
+							// so extractResourceTypeFromOperations sets rec.ResourceType = "n1-standard-4".
+							Action:   "add",
+							Resource: "projects/test/zones/us-central1-a/machineTypes/n1-standard-4",
+						},
+					},
+				},
+				{
+					Operations: []*recommenderpb.Operation{
+						{
+							// Commitment resource op: carries the VCPU amount as a numeric value.
+							// extractVCPUCountFromRecommendation reads this and sets rec.Count = 4.
+							Action:       "add",
+							ResourceType: "compute.googleapis.com/Commitment",
+							Resource:     "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001",
+							Path:         "/resources/0/amount",
+							PathValue:    &recommenderpb.Operation_Value{Value: vcpuVal},
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+// mockBillingWithCommitment returns a MockBillingService that has both an
+// on-demand SKU and a commitment SKU for n1-standard machines in us-central1.
+func mockBillingWithCommitment() *MockBillingService {
+	return &MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{
+			Skus: []*cloudbilling.Sku{
+				{
+					Description:    "n1-standard-4 VM running in Americas",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        190000000,
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+				{
+					Description:    "n1-standard-4 commitment 1yr in Americas",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        120000000,
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+// TestConverterToInsert_CountNonZero_VCPUAmountSet is the primary regression test
+// for issue #1022 C1. It exercises the full converter->purchase path with a
+// realistic Recommender payload and asserts:
+//   - converter sets Count > 0 (extracted from the operation's numeric value)
+//   - buildInsertRequest produces a non-zero VCPU Amount in the resulting commitment
+//
+// This test FAILS on the pre-fix code where convertGCPRecommendation never set Count.
+func TestConverterToInsert_CountNonZero_VCPUAmountSet(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+	client.SetBillingService(mockBillingWithCommitment())
+
+	gcpRec := realisticCUDRecommendation()
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
+	require.NotNil(t, rec)
+
+	// C1: Count must be > 0 so that buildInsertRequest produces non-zero VCPU Amount.
+	require.Greater(t, rec.Count, 0, "convertGCPRecommendation must set Count > 0 from the Recommender payload (issue #1022 C1)")
+
+	// Verify the insert request carries the correct VCPU count.
+	insertReq, _, buildErr := client.buildInsertRequest(*rec, common.PurchaseOptions{})
+	require.NoError(t, buildErr, "buildInsertRequest must not error when Count > 0")
+	require.NotNil(t, insertReq)
+	require.NotNil(t, insertReq.CommitmentResource)
+
+	var vcpuAmount, memoryAmount int64
+	for _, r := range insertReq.CommitmentResource.Resources {
+		switch r.GetType() {
+		case "VCPU":
+			vcpuAmount = r.GetAmount()
+		case "MEMORY_MB":
+			memoryAmount = r.GetAmount()
+		}
+	}
+	assert.Greater(t, vcpuAmount, int64(0), "VCPU Amount in the insert request must be > 0 (issue #1022 C1)")
+	assert.Greater(t, memoryAmount, int64(0), "MEMORY_MB Amount in the insert request must be > 0 (issue #1022 C1)")
+}
+
+// TestConverterFillsPricingForScorer is the regression test for issue #1022 C2.
+// It exercises the full converter path with a realistic Recommender payload and a
+// billing mock that returns both on-demand and commitment SKUs, and then asserts
+// that a scorer with MinSavingsPct set does NOT drop the GCP recommendation.
+//
+// This test FAILS on the pre-fix code where convertGCPRecommendation never
+// called getComputePricing, leaving SavingsPercentage at 0.
+func TestConverterFillsPricingForScorer(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+	client.SetBillingService(mockBillingWithCommitment())
+
+	gcpRec := realisticCUDRecommendation()
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
+	require.NotNil(t, rec)
+
+	// C2: SavingsPercentage must be > 0 so a MinSavingsPct filter doesn't silently drop the rec.
+	require.Greater(t, rec.SavingsPercentage, float64(0),
+		"convertGCPRecommendation must set SavingsPercentage > 0 from billing pricing (issue #1022 C2)")
+	assert.Greater(t, rec.CommitmentCost, float64(0), "CommitmentCost must be > 0")
+	assert.Greater(t, rec.OnDemandCost, float64(0), "OnDemandCost must be > 0")
+
+	// Apply a non-trivial MinSavingsPct filter and confirm the rec passes.
+	result := scorer.Score([]common.Recommendation{*rec}, scorer.Config{MinSavingsPct: 5.0})
+	require.Len(t, result.Passed, 1, "GCP recommendation must pass a MinSavingsPct=5 scorer filter after pricing is wired (issue #1022 C2)")
+	assert.Empty(t, result.Filtered, "no GCP recommendations should be filtered by MinSavingsPct when pricing is set")
+}
+
+// TestBuildInsertRequest_RefusesZeroCount is the regression test for the
+// issue #1022 guard in buildInsertRequest. A recommendation with Count == 0
+// must return an error rather than sending a zero-vCPU commitment to GCP.
+//
+// This test FAILS on the pre-fix code (which had no such guard).
+func TestBuildInsertRequest_RefusesZeroCount(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-4",
+		Term:         "1yr",
+		Count:        0, // zero -- should be refused
+	}
+
+	_, _, err := client.buildInsertRequest(rec, common.PurchaseOptions{})
+	require.Error(t, err, "buildInsertRequest must refuse Count <= 0 (issue #1022 guard)")
+	assert.Contains(t, err.Error(), "rec.Count must be > 0")
+
+	// PurchaseCommitment must also surface the error and not call Insert.
+	mockSvc := &MockCommitmentsService{operation: &MockOperation{}}
+	client.SetCommitmentsService(mockSvc)
+	result, purchaseErr := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
+	require.Error(t, purchaseErr)
+	assert.False(t, result.Success)
+	assert.Empty(t, mockSvc.insertReqs, "Insert must not be called when Count <= 0")
+}
+
+// TestGetComputePricing_NoCommitmentSKUReturnsError is the regression test for
+// issue #1020 (GCP). When the billing catalog has no commitment SKU, getComputePricing
+// must return an error rather than fabricating a price from a hardcoded discount factor.
+//
+// This test FAILS on the pre-fix code where estimateComputeCommitmentPrice was called.
+func TestGetComputePricing_NoCommitmentSKUReturnsError(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	// Only on-demand SKU -- no commitment SKU.
+	client.SetBillingService(&MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{
+			Skus: []*cloudbilling.Sku{
+				{
+					Description:    "n1-standard-4 VM running in Americas",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        190000000,
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+
+	pricing, err := client.getComputePricing(ctx, "n1-standard-4", "us-central1", 1)
+	require.Error(t, err, "getComputePricing must return an error when no commitment SKU exists (issue #1020)")
+	assert.Contains(t, err.Error(), "no commitment pricing found")
+	assert.Nil(t, pricing, "no pricing struct must be returned when commitment SKU is missing")
+}
diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go
index 2407b77c8..982228a90 100644
--- a/providers/gcp/services/memorystore/client.go
+++ b/providers/gcp/services/memorystore/client.go
@@ -4,6 +4,7 @@ package memorystore
 import (
 	"context"
 	"fmt"
+	"log"
 	"strings"
 	"time"
 
@@ -181,7 +182,7 @@ func (c *MemorystoreClient) GetRecommendations(ctx context.Context, params commo
 			return nil, fmt.Errorf("memorystore: iterate recommendations: %w", err)
 		}
 
-		converted := c.convertGCPRecommendation(ctx, rec)
+		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
 		}
@@ -295,7 +296,9 @@ type RedisPricing struct {
 	SavingsPercentage float64
 }
 
-// getRedisPricing gets pricing from GCP Cloud Billing Catalog API
+// getRedisPricing gets pricing from GCP Cloud Billing Catalog API.
+// It returns an error when commitment pricing is absent from the catalog rather
+// than fabricating a price from a hardcoded discount factor (issue #1020).
 func (c *MemorystoreClient) getRedisPricing(ctx context.Context, tier, region string, termYears int) (*RedisPricing, error) {
 	billingSvc, err := c.getOrCreateBillingService(ctx)
 	if err != nil {
@@ -311,12 +314,11 @@ func (c *MemorystoreClient) getRedisPricing(ctx context.Context, tier, region st
 	if onDemandPrice == 0 {
 		return nil, fmt.Errorf("no pricing found for Memorystore tier %s", tier)
 	}
-
-	hoursInTerm := 8760.0 * float64(termYears)
 	if commitmentPrice == 0 {
-		commitmentPrice = estimateCommitmentPrice(onDemandPrice, hoursInTerm, termYears)
+		return nil, fmt.Errorf("no commitment pricing found for Memorystore tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region)
 	}
 
+	hoursInTerm := 8760.0 * float64(termYears)
 	savingsPercentage := calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
 
 	return &RedisPricing{
@@ -390,15 +392,6 @@ func extractPriceFromSKU(sku *cloudbilling.Sku) (float64, string) {
 	return price, rate.UnitPrice.CurrencyCode
 }
 
-// estimateCommitmentPrice estimates commitment price based on typical GCP savings
-func estimateCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 {
-	discount := 0.70 // 30% savings for 1 year
-	if termYears == 3 {
-		discount = 0.65 // 35% savings for 3 years
-	}
-	return onDemandPrice * hoursInTerm * discount
-}
-
 // calculateSavingsPercentage calculates the savings percentage
 func calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 {
 	onDemandTotal := onDemandPrice * hoursInTerm
@@ -425,8 +418,16 @@ func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool {
 	return true
 }
 
-// convertGCPRecommendation converts a GCP Recommender recommendation to common format
-func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation {
+// convertGCPRecommendation converts a GCP Recommender recommendation to common format.
+// It also calls getRedisPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
+// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
+// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation.
+func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
+	paymentOption := params.PaymentOption
+	if paymentOption == "" {
+		paymentOption = "monthly"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceCache,
@@ -435,7 +436,7 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 		CommitmentType: common.CommitmentCUD,
 		Timestamp:      time.Now(),
 		Term:           "1yr",
-		PaymentOption:  "monthly",
+		PaymentOption:  paymentOption,
 	}
 
 	// Extract resource type from recommendation content
@@ -456,7 +457,6 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 
 	// Extract cost impact
 	if gcpRec.PrimaryImpact != nil {
-		// Use GetCostProjection() method to access the cost projection
 		if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil {
 			cost := costProj.Cost
 			savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9)
@@ -464,5 +464,27 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 		}
 	}
 
+	// Thread pricing into the converter so the scorer can rank/filter GCP recs
+	// correctly (issue #1022 C2).
+	if rec.ResourceType != "" {
+		termYears := 1
+		if rec.Term == "3yr" || rec.Term == "3" {
+			termYears = 3
+		}
+		if pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
+			log.Printf("memorystore: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		} else {
+			rec.CommitmentCost = pricing.CommitmentPrice
+			rec.OnDemandCost = pricing.OnDemandPrice
+			rec.SavingsPercentage = pricing.SavingsPercentage
+			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+				if monthlySavings > 0 {
+					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+				}
+			}
+		}
+	}
+
 	return rec
 }
diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go
index 4f543b865..d97330cf8 100644
--- a/providers/gcp/services/memorystore/client_test.go
+++ b/providers/gcp/services/memorystore/client_test.go
@@ -364,6 +364,8 @@ func TestMemorystoreClient_GetOfferingDetails_WithMockService(t *testing.T) {
 		errContains string
 	}{
 		{
+			// Both on-demand and commitment SKUs are required after the issue #1020 fix:
+			// getRedisPricing now returns an error when no commitment SKU is found.
 			name: "successful 1yr offering details",
 			rec: common.Recommendation{
 				ResourceType:  "STANDARD_HA",
@@ -391,11 +393,31 @@ func TestMemorystoreClient_GetOfferingDetails_WithMockService(t *testing.T) {
 							},
 						},
 					},
+					{
+						Description:    "Memorystore Redis STANDARD_HA commitment 1yr",
+						ServiceRegions: []string{"us-central1"},
+						PricingInfo: []*cloudbilling.PricingInfo{
+							{
+								PricingExpression: &cloudbilling.PricingExpression{
+									TieredRates: []*cloudbilling.TierRate{
+										{
+											UnitPrice: &cloudbilling.Money{
+												CurrencyCode: "USD",
+												Units:        0,
+												Nanos:        35000000, // $0.035 per hour
+											},
+										},
+									},
+								},
+							},
+						},
+					},
 				},
 			},
 			wantErr: false,
 		},
 		{
+			// Both on-demand and commitment SKUs are required after the issue #1020 fix.
 			name: "successful 3yr offering details",
 			rec: common.Recommendation{
 				ResourceType:  "BASIC",
@@ -423,6 +445,25 @@ func TestMemorystoreClient_GetOfferingDetails_WithMockService(t *testing.T) {
 							},
 						},
 					},
+					{
+						Description:    "Memorystore Redis BASIC commitment 3yr",
+						ServiceRegions: []string{"us-central1"},
+						PricingInfo: []*cloudbilling.PricingInfo{
+							{
+								PricingExpression: &cloudbilling.PricingExpression{
+									TieredRates: []*cloudbilling.TierRate{
+										{
+											UnitPrice: &cloudbilling.Money{
+												CurrencyCode: "USD",
+												Units:        0,
+												Nanos:        19500000, // $0.0195 per hour
+											},
+										},
+									},
+								},
+							},
+						},
+					},
 				},
 			},
 			wantErr: false,
@@ -592,7 +633,7 @@ func TestMemorystoreClient_ConvertGCPRecommendation(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := client.convertGCPRecommendation(ctx, tt.rec)
+			result := client.convertGCPRecommendation(ctx, tt.rec, common.RecommendationParams{})
 
 			require.NotNil(t, result)
 			assert.Equal(t, common.ProviderGCP, result.Provider)

From 7dc3f9ad859b011e226a14e237cc717a79da5e58 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 04:19:40 -0700
Subject: [PATCH 02/10] fix(gcp/computeengine): default PaymentOption to
 "monthly" (10-M5)

GCP CUDs have no upfront-payment option; the previous default of "upfront"
caused downstream purchase-body construction to silently use a mode that
does not exist for CUDs. Changes to align with cloudsql/memorystore/
cloudstorage which already default to "monthly" and with the monthly stamp
introduced by PRs #718/#829 (which this supersedes in the converter layer).

Regression tests:
- TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly: FAILS on
  pre-fix code (paymentOption == "upfront"); PASSES after.
- TestConvertGCPRecommendation_ParamPaymentOptionRespected: caller-supplied
  value is forwarded unchanged.

Closes #1062
---
 .../gcp/services/computeengine/client.go      | 93 +++++++++++--------
 .../gcp/services/computeengine/client_test.go | 43 +++++++++
 2 files changed, 95 insertions(+), 41 deletions(-)

diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index 47a305ffd..6b3f51e45 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -829,7 +829,12 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo
 func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
 	paymentOption := params.PaymentOption
 	if paymentOption == "" {
-		paymentOption = "upfront"
+		// GCP CUDs are billed monthly with no upfront option; "monthly" is
+		// the only valid default. Aligns with cloudsql/memorystore/cloudstorage
+		// and supersedes the monthly stamp introduced in PR #829 (which
+		// stamped "monthly" in the purchase body but not in the converter
+		// default -- see PR #1047 for the supersession note).
+		paymentOption = "monthly"
 	}
 
 	rec := &common.Recommendation{
@@ -929,6 +934,49 @@ func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, r
 	}
 }
 
+// isMemoryAmountOp returns true when op's path_filters indicate a MEMORY_MB
+// resource type. Used by extractVCPUCountFromRecommendation to skip the memory
+// sibling of the VCPU operation in a GCP commitment resource operation group.
+func isMemoryAmountOp(op *recommenderpb.Operation) bool {
+	for filterKey, filterVal := range op.GetPathFilters() {
+		if !strings.Contains(strings.ToLower(filterKey), "type") {
+			continue
+		}
+		if sv, ok := filterVal.GetKind().(*structpb.Value_StringValue); ok {
+			if strings.EqualFold(sv.StringValue, "MEMORY_MB") {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// vcpuCountFromOperationGroups walks the commitment operation groups and
+// returns the VCPU count encoded in the operation's numeric value, or 0 if
+// none is found. Extracted from extractVCPUCountFromRecommendation to keep
+// cyclomatic complexity in check.
+func vcpuCountFromOperationGroups(content *recommenderpb.RecommendationContent) int {
+	for _, opGroup := range content.GetOperationGroups() {
+		for _, op := range opGroup.GetOperations() {
+			if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") {
+				continue
+			}
+			if !strings.Contains(strings.ToLower(op.GetPath()), "amount") {
+				continue
+			}
+			if isMemoryAmountOp(op) {
+				continue
+			}
+			if v := op.GetValue(); v != nil {
+				if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 {
+					return int(nv.NumberValue)
+				}
+			}
+		}
+	}
+	return 0
+}
+
 // extractVCPUCountFromRecommendation extracts the recommended vCPU count from a
 // GCP Commitment Recommender response (issue #1022 C1).
 //
@@ -947,46 +995,9 @@ func extractVCPUCountFromRecommendation(gcpRec *recommenderpb.Recommendation, re
 		return
 	}
 
-	// Walk operations looking for a VCPU resource amount.
-	for _, opGroup := range gcpRec.Content.GetOperationGroups() {
-		for _, op := range opGroup.GetOperations() {
-			// The commitment recommender uses ResourceType
-			// "compute.googleapis.com/Commitment" with paths like
-			// "/resources/0/amount" (VCPU) and "/resources/1/amount" (MEMORY_MB).
-			// We detect VCPU by checking that the path does NOT mention "memory"
-			// and that a sibling "/resources/0/type" is "VCPU" (encoded in pathFilters).
-			if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") {
-				continue
-			}
-			path := strings.ToLower(op.GetPath())
-			if !strings.Contains(path, "amount") {
-				continue
-			}
-			// Skip the memory amount: the path filter or a nearby type operation
-			// distinguishes VCPU from MEMORY_MB. The VCPU amount is always first
-			// ("/resources/0/amount") in the canonical recommender output; if
-			// path_filters carry a "type":"VCPU" entry we can check that directly.
-			isMemory := false
-			for filterKey, filterVal := range op.GetPathFilters() {
-				if strings.Contains(strings.ToLower(filterKey), "type") {
-					if sv, ok := filterVal.GetKind().(*structpb.Value_StringValue); ok {
-						if strings.EqualFold(sv.StringValue, "MEMORY_MB") {
-							isMemory = true
-							break
-						}
-					}
-				}
-			}
-			if isMemory {
-				continue
-			}
-			if v := op.GetValue(); v != nil {
-				if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 {
-					rec.Count = int(nv.NumberValue)
-					return
-				}
-			}
-		}
+	if count := vcpuCountFromOperationGroups(gcpRec.Content); count > 0 {
+		rec.Count = count
+		return
 	}
 
 	// Fallback: overview numericValue (used by older recommender versions).
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index fdf2ea098..336ebf135 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -1148,3 +1148,46 @@ func TestGetComputePricing_NoCommitmentSKUReturnsError(t *testing.T) {
 	assert.Contains(t, err.Error(), "no commitment pricing found")
 	assert.Nil(t, pricing, "no pricing struct must be returned when commitment SKU is missing")
 }
+
+// TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly asserts that when
+// RecommendationParams.PaymentOption is empty the converter defaults to "monthly".
+// GCP CUDs have no upfront-payment option; defaulting to "upfront" caused incorrect
+// purchase-body construction downstream.
+//
+// This test FAILS on the pre-fix code that defaulted to "upfront" (10-M5 / auditor finding).
+func TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{
+		Name: "test-rec",
+		PrimaryImpact: &recommenderpb.Impact{
+			Category: recommenderpb.Impact_COST,
+			Projection: &recommenderpb.Impact_CostProjection{
+				CostProjection: &recommenderpb.CostProjection{
+					Cost: &money.Money{Units: -100, CurrencyCode: "USD"},
+				},
+			},
+		},
+	}
+
+	// Empty params: no caller-supplied PaymentOption.
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
+	require.NotNil(t, rec)
+	assert.Equal(t, "monthly", rec.PaymentOption,
+		"GCP CUDs have no upfront option; empty PaymentOption must default to \"monthly\" (10-M5)")
+}
+
+// TestConvertGCPRecommendation_ParamPaymentOptionRespected asserts that an
+// explicit PaymentOption in RecommendationParams is forwarded to the recommendation.
+func TestConvertGCPRecommendation_ParamPaymentOptionRespected(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+	params := common.RecommendationParams{PaymentOption: "monthly"}
+
+	rec := client.convertGCPRecommendation(ctx, gcpRec, params)
+	require.NotNil(t, rec)
+	assert.Equal(t, "monthly", rec.PaymentOption)
+}

From 7198bdcbe338c9b0bd65525f99d257427c2a6a2c Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 04:19:51 -0700
Subject: [PATCH 03/10] refactor(gcp): extract helpers to fix cyclomatic
 complexity in converters

The four convertGCPRecommendation implementations (cloudsql, cloudstorage,
memorystore) scored 18 and extractVCPUCountFromRecommendation scored 17 on
gocyclo, failing the pre-commit hook. Refactor:

- Add extractResourceTypeFromContent/extractEstimatedSavings helpers shared
  by all three service converters; reduce each converter body.
- Add fillRedisPricing/fillSQLPricing/fillStoragePricing per-service helpers
  that isolate the if-pricing-err-ok branching.
- Add isMemoryAmountOp + vcpuCountFromOperationGroups to split the nested
  path-filter scan in extractVCPUCountFromRecommendation.

All 235 GCP tests pass; gocyclo --over 10 finds no violations.
---
 providers/gcp/services/cloudsql/client.go     | 95 +++++++++++--------
 providers/gcp/services/cloudstorage/client.go | 95 +++++++++++--------
 providers/gcp/services/memorystore/client.go  | 95 +++++++++++--------
 3 files changed, 174 insertions(+), 111 deletions(-)

diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go
index ed3de8fff..539e418f9 100644
--- a/providers/gcp/services/cloudsql/client.go
+++ b/providers/gcp/services/cloudsql/client.go
@@ -486,6 +486,61 @@ func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool {
 	return true
 }
 
+// extractResourceTypeFromContent extracts the last path segment of the first
+// non-empty Operation.Resource across all operation groups. Used by all four
+// GCP service converters to set rec.ResourceType from Recommender payloads.
+func extractResourceTypeFromContent(content *recommenderpb.RecommendationContent) string {
+	if content == nil || content.OperationGroups == nil {
+		return ""
+	}
+	for _, opGroup := range content.OperationGroups {
+		for _, op := range opGroup.Operations {
+			if op.Resource == "" {
+				continue
+			}
+			parts := strings.Split(op.Resource, "/")
+			if len(parts) > 0 {
+				return parts[len(parts)-1]
+			}
+		}
+	}
+	return ""
+}
+
+// extractEstimatedSavings returns the negative of the PrimaryImpact cost
+// projection (GCP encodes savings as a negative cost delta).
+func extractEstimatedSavings(gcpRec *recommenderpb.Recommendation) float64 {
+	if gcpRec.PrimaryImpact == nil {
+		return 0
+	}
+	costProj := gcpRec.PrimaryImpact.GetCostProjection()
+	if costProj == nil || costProj.Cost == nil {
+		return 0
+	}
+	cost := costProj.Cost
+	return -(float64(cost.Units) + float64(cost.Nanos)/1e9)
+}
+
+// fillSQLPricing calls getSQLPricing and, on success, writes CommitmentCost,
+// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing
+// failures are logged and do not discard the recommendation.
+func (c *CloudSQLClient) fillSQLPricing(ctx context.Context, rec *common.Recommendation, termYears int) {
+	pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears)
+	if err != nil {
+		log.Printf("cloudsql: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		return
+	}
+	rec.CommitmentCost = pricing.CommitmentPrice
+	rec.OnDemandCost = pricing.OnDemandPrice
+	rec.SavingsPercentage = pricing.SavingsPercentage
+	if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+		monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+		if monthlySavings > 0 {
+			rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+		}
+	}
+}
+
 // convertGCPRecommendation converts a GCP Recommender recommendation to common format.
 // It also calls getSQLPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
 // BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
@@ -507,30 +562,8 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 		PaymentOption:  paymentOption,
 	}
 
-	// Extract resource type from recommendation content
-	if gcpRec.Content != nil {
-		if gcpRec.Content.OperationGroups != nil {
-			for _, opGroup := range gcpRec.Content.OperationGroups {
-				for _, op := range opGroup.Operations {
-					if op.Resource != "" {
-						parts := strings.Split(op.Resource, "/")
-						if len(parts) > 0 {
-							rec.ResourceType = parts[len(parts)-1]
-						}
-					}
-				}
-			}
-		}
-	}
-
-	// Extract cost impact
-	if gcpRec.PrimaryImpact != nil {
-		if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil {
-			cost := costProj.Cost
-			savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9)
-			rec.EstimatedSavings = savings
-		}
-	}
+	rec.ResourceType = extractResourceTypeFromContent(gcpRec.Content)
+	rec.EstimatedSavings = extractEstimatedSavings(gcpRec)
 
 	// Thread pricing into the converter so the scorer can rank/filter GCP recs
 	// correctly (issue #1022 C2).
@@ -539,19 +572,7 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 		if rec.Term == "3yr" || rec.Term == "3" {
 			termYears = 3
 		}
-		if pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
-			log.Printf("cloudsql: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
-		} else {
-			rec.CommitmentCost = pricing.CommitmentPrice
-			rec.OnDemandCost = pricing.OnDemandPrice
-			rec.SavingsPercentage = pricing.SavingsPercentage
-			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
-				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
-				if monthlySavings > 0 {
-					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
-				}
-			}
-		}
+		c.fillSQLPricing(ctx, rec, termYears)
 	}
 
 	return rec
diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go
index e4a377136..d65aa3c19 100644
--- a/providers/gcp/services/cloudstorage/client.go
+++ b/providers/gcp/services/cloudstorage/client.go
@@ -476,6 +476,61 @@ func skuMatchesStorageClass(sku *cloudbilling.Sku, storageClass, region string)
 	return true
 }
 
+// extractResourceTypeFromContent extracts the last path segment of the first
+// non-empty Operation.Resource across all operation groups. Used by all four
+// GCP service converters to set rec.ResourceType from Recommender payloads.
+func extractResourceTypeFromContent(content *recommenderpb.RecommendationContent) string {
+	if content == nil || content.OperationGroups == nil {
+		return ""
+	}
+	for _, opGroup := range content.OperationGroups {
+		for _, op := range opGroup.Operations {
+			if op.Resource == "" {
+				continue
+			}
+			parts := strings.Split(op.Resource, "/")
+			if len(parts) > 0 {
+				return parts[len(parts)-1]
+			}
+		}
+	}
+	return ""
+}
+
+// extractEstimatedSavings returns the negative of the PrimaryImpact cost
+// projection (GCP encodes savings as a negative cost delta).
+func extractEstimatedSavings(gcpRec *recommenderpb.Recommendation) float64 {
+	if gcpRec.PrimaryImpact == nil {
+		return 0
+	}
+	costProj := gcpRec.PrimaryImpact.GetCostProjection()
+	if costProj == nil || costProj.Cost == nil {
+		return 0
+	}
+	cost := costProj.Cost
+	return -(float64(cost.Units) + float64(cost.Nanos)/1e9)
+}
+
+// fillStoragePricing calls getStoragePricing and, on success, writes CommitmentCost,
+// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing
+// failures are logged and do not discard the recommendation.
+func (c *CloudStorageClient) fillStoragePricing(ctx context.Context, rec *common.Recommendation, termYears int) {
+	pricing, err := c.getStoragePricing(ctx, rec.ResourceType, c.region, termYears)
+	if err != nil {
+		log.Printf("cloudstorage: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		return
+	}
+	rec.CommitmentCost = pricing.CommitmentPrice
+	rec.OnDemandCost = pricing.OnDemandPrice
+	rec.SavingsPercentage = pricing.SavingsPercentage
+	if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+		monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+		if monthlySavings > 0 {
+			rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+		}
+	}
+}
+
 // convertGCPRecommendation converts a GCP Recommender recommendation to common format.
 // It also calls getStoragePricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
 // BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
@@ -497,30 +552,8 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 		PaymentOption:  paymentOption,
 	}
 
-	// Extract resource type from recommendation content
-	if gcpRec.Content != nil {
-		if gcpRec.Content.OperationGroups != nil {
-			for _, opGroup := range gcpRec.Content.OperationGroups {
-				for _, op := range opGroup.Operations {
-					if op.Resource != "" {
-						parts := strings.Split(op.Resource, "/")
-						if len(parts) > 0 {
-							rec.ResourceType = parts[len(parts)-1]
-						}
-					}
-				}
-			}
-		}
-	}
-
-	// Extract cost impact
-	if gcpRec.PrimaryImpact != nil {
-		if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil {
-			cost := costProj.Cost
-			savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9)
-			rec.EstimatedSavings = savings
-		}
-	}
+	rec.ResourceType = extractResourceTypeFromContent(gcpRec.Content)
+	rec.EstimatedSavings = extractEstimatedSavings(gcpRec)
 
 	// Thread pricing into the converter so the scorer can rank/filter GCP recs
 	// correctly (issue #1022 C2).
@@ -529,19 +562,7 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 		if rec.Term == "3yr" || rec.Term == "3" {
 			termYears = 3
 		}
-		if pricing, err := c.getStoragePricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
-			log.Printf("cloudstorage: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
-		} else {
-			rec.CommitmentCost = pricing.CommitmentPrice
-			rec.OnDemandCost = pricing.OnDemandPrice
-			rec.SavingsPercentage = pricing.SavingsPercentage
-			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
-				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
-				if monthlySavings > 0 {
-					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
-				}
-			}
-		}
+		c.fillStoragePricing(ctx, rec, termYears)
 	}
 
 	return rec
diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go
index 982228a90..73ffac2ce 100644
--- a/providers/gcp/services/memorystore/client.go
+++ b/providers/gcp/services/memorystore/client.go
@@ -418,6 +418,61 @@ func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool {
 	return true
 }
 
+// extractResourceTypeFromContent extracts the last path segment of the first
+// non-empty Operation.Resource across all operation groups. Used by all four
+// GCP service converters to set rec.ResourceType from Recommender payloads.
+func extractResourceTypeFromContent(content *recommenderpb.RecommendationContent) string {
+	if content == nil || content.OperationGroups == nil {
+		return ""
+	}
+	for _, opGroup := range content.OperationGroups {
+		for _, op := range opGroup.Operations {
+			if op.Resource == "" {
+				continue
+			}
+			parts := strings.Split(op.Resource, "/")
+			if len(parts) > 0 {
+				return parts[len(parts)-1]
+			}
+		}
+	}
+	return ""
+}
+
+// extractEstimatedSavings returns the negative of the PrimaryImpact cost
+// projection (GCP encodes savings as a negative cost delta).
+func extractEstimatedSavings(gcpRec *recommenderpb.Recommendation) float64 {
+	if gcpRec.PrimaryImpact == nil {
+		return 0
+	}
+	costProj := gcpRec.PrimaryImpact.GetCostProjection()
+	if costProj == nil || costProj.Cost == nil {
+		return 0
+	}
+	cost := costProj.Cost
+	return -(float64(cost.Units) + float64(cost.Nanos)/1e9)
+}
+
+// fillRedisPricing calls getRedisPricing and, on success, writes CommitmentCost,
+// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing
+// failures are logged and do not discard the recommendation.
+func (c *MemorystoreClient) fillRedisPricing(ctx context.Context, rec *common.Recommendation, termYears int) {
+	pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears)
+	if err != nil {
+		log.Printf("memorystore: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		return
+	}
+	rec.CommitmentCost = pricing.CommitmentPrice
+	rec.OnDemandCost = pricing.OnDemandPrice
+	rec.SavingsPercentage = pricing.SavingsPercentage
+	if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+		monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+		if monthlySavings > 0 {
+			rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+		}
+	}
+}
+
 // convertGCPRecommendation converts a GCP Recommender recommendation to common format.
 // It also calls getRedisPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/
 // BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
@@ -439,30 +494,8 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 		PaymentOption:  paymentOption,
 	}
 
-	// Extract resource type from recommendation content
-	if gcpRec.Content != nil {
-		if gcpRec.Content.OperationGroups != nil {
-			for _, opGroup := range gcpRec.Content.OperationGroups {
-				for _, op := range opGroup.Operations {
-					if op.Resource != "" {
-						parts := strings.Split(op.Resource, "/")
-						if len(parts) > 0 {
-							rec.ResourceType = parts[len(parts)-1]
-						}
-					}
-				}
-			}
-		}
-	}
-
-	// Extract cost impact
-	if gcpRec.PrimaryImpact != nil {
-		if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil {
-			cost := costProj.Cost
-			savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9)
-			rec.EstimatedSavings = savings
-		}
-	}
+	rec.ResourceType = extractResourceTypeFromContent(gcpRec.Content)
+	rec.EstimatedSavings = extractEstimatedSavings(gcpRec)
 
 	// Thread pricing into the converter so the scorer can rank/filter GCP recs
 	// correctly (issue #1022 C2).
@@ -471,19 +504,7 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 		if rec.Term == "3yr" || rec.Term == "3" {
 			termYears = 3
 		}
-		if pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
-			log.Printf("memorystore: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
-		} else {
-			rec.CommitmentCost = pricing.CommitmentPrice
-			rec.OnDemandCost = pricing.OnDemandPrice
-			rec.SavingsPercentage = pricing.SavingsPercentage
-			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
-				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
-				if monthlySavings > 0 {
-					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
-				}
-			}
-		}
+		c.fillRedisPricing(ctx, rec, termYears)
 	}
 
 	return rec

From 769513ef8296339c37656d33426a30940f859a75 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 04:28:49 -0700
Subject: [PATCH 04/10] fix(gcp): FOLD-1047 correctness findings
 (10-M2,M3,M7,L1,L2,L3,L4,N3)

10-M2: Add termPlan(term) helper and memMBPerVCPU const in computeengine;
       dedup the "TWELVE_MONTH"/"THIRTY_SIX_MONTH" and 4096 literals.

10-M3: Replace substring-match isResourceExhausted with typed checks:
       errors.As(*googleapi.Error, Code==429) for REST, status.FromError
       codes.ResourceExhausted for gRPC; substring match kept as fallback
       for non-standard wrapped errors.

10-M7: GetAccounts returns empty slice when no ACTIVE projects are visible
       instead of synthesising a fallback account from p.projectID, which
       would silently hide credential/permission errors from callers. Test
       updated to assert empty (not 1-element) on zero-projects input.

10-L1: Remove the no-op commitmentType if-branch in convertGCPCommitmentToCommon
       (both arms assigned CommitmentCUD).

10-L2: CloudStorage GetExistingCommitments returns empty; enumerating
       regional buckets is not a commitment indicator (GCS has no CUD API).
       Test updated accordingly.

10-L3: CloudSQL GetExistingCommitments returns empty; PricingPlan "PACKAGE"
       is a legacy per-instance billing mode, not a spend-based CUD. Test
       updated accordingly.

10-L4: Wrap memorystore realRecommenderClient.ListRecommendations in
       realRecommenderIterator for diffability with the other 3 service
       clients.

10-N3: Rename local var 'provider' in recommendations.getRegions to 'p' to
       avoid shadowing the imported 'provider' package.

Deferred (logged to docs/code-review/open-questions/fold-1047.md):
- 10-M1: dead GroupCommitments types -- no-delete: pending owner confirmation
- 10-M6: struct-stored ctx removal -- tests assert on ctx field; needs test
         update before removal (correctness impact: none, field is never read)
- 10-N1: common.Ptr[T] -- helper does not yet exist; lands with #1035

Closes #1062
---
 providers/gcp/provider.go                     | 14 ++--
 providers/gcp/provider_test.go                |  8 ++-
 providers/gcp/recommendations.go              |  7 +-
 providers/gcp/services/cloudsql/client.go     | 59 +++-------------
 .../gcp/services/cloudsql/client_test.go      | 70 ++-----------------
 providers/gcp/services/cloudstorage/client.go | 53 ++------------
 .../gcp/services/cloudstorage/client_test.go  | 63 ++---------------
 .../gcp/services/computeengine/client.go      | 58 +++++++++++----
 providers/gcp/services/memorystore/client.go  | 12 +++-
 9 files changed, 95 insertions(+), 249 deletions(-)

diff --git a/providers/gcp/provider.go b/providers/gcp/provider.go
index ad3f77ca9..56d4d75c0 100644
--- a/providers/gcp/provider.go
+++ b/providers/gcp/provider.go
@@ -333,16 +333,10 @@ func (p *GCPProvider) GetAccounts(ctx context.Context) ([]common.Account, error)
 		}
 	}
 
-	// If no projects found, return at least the default project
-	if len(accounts) == 0 {
-		accounts = append(accounts, common.Account{
-			Provider:  common.ProviderGCP,
-			ID:        p.projectID,
-			Name:      p.projectID,
-			IsDefault: true,
-		})
-	}
-
+	// Return empty if no ACTIVE projects are visible to the credentials (10-M7).
+	// Synthesising a fallback account from p.projectID can silently succeed
+	// when the account has no accessible projects, hiding auth / permission
+	// errors from callers. Return empty so the caller can surface the gap.
 	return accounts, nil
 }
 
diff --git a/providers/gcp/provider_test.go b/providers/gcp/provider_test.go
index d6b4f90d4..254bf47c4 100644
--- a/providers/gcp/provider_test.go
+++ b/providers/gcp/provider_test.go
@@ -562,6 +562,10 @@ func TestGCPProvider_GetAccounts_WithMock(t *testing.T) {
 	assert.Equal(t, "project-2", accounts[1].ID)
 }
 
+// TestGCPProvider_GetAccounts_Empty asserts that GetAccounts returns an empty
+// slice (not a synthesised fallback project) when no ACTIVE projects are
+// visible to the credentials (10-M7). A synthesised account would hide
+// permission / auth errors from callers.
 func TestGCPProvider_GetAccounts_Empty(t *testing.T) {
 	ctx := context.Background()
 	p := NewProviderWithProject(ctx, "default-project")
@@ -573,9 +577,7 @@ func TestGCPProvider_GetAccounts_Empty(t *testing.T) {
 
 	accounts, err := p.GetAccounts(ctx)
 	require.NoError(t, err)
-	// Should return the default project when no projects found
-	assert.Len(t, accounts, 1)
-	assert.Equal(t, "default-project", accounts[0].ID)
+	assert.Empty(t, accounts, "GetAccounts must return empty when no ACTIVE projects are visible (10-M7)")
 }
 
 func TestGCPProvider_GetAccounts_Error(t *testing.T) {
diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go
index b33027d8f..3168480a3 100644
--- a/providers/gcp/recommendations.go
+++ b/providers/gcp/recommendations.go
@@ -235,10 +235,11 @@ func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context
 
 // getRegions retrieves available GCP regions for the project
 func (r *RecommendationsClientAdapter) getRegions(ctx context.Context) ([]string, error) {
-	// Create a temporary provider to get regions
-	provider := NewProviderWithProject(ctx, r.projectID, r.clientOpts...)
+	// Create a temporary provider to get regions. The local variable is named
+	// p (not provider) to avoid shadowing the imported provider package (10-N3).
+	p := NewProviderWithProject(ctx, r.projectID, r.clientOpts...)
 
-	regions, err := provider.GetRegions(ctx)
+	regions, err := p.GetRegions(ctx)
 	if err != nil {
 		return nil, err
 	}
diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go
index 539e418f9..dd803f67a 100644
--- a/providers/gcp/services/cloudsql/client.go
+++ b/providers/gcp/services/cloudsql/client.go
@@ -191,56 +191,15 @@ func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.R
 	return recommendations, nil
 }
 
-// GetExistingCommitments retrieves existing Cloud SQL commitments
-func (c *CloudSQLClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) {
-	commitments := make([]common.Commitment, 0)
-
-	// Use injected service if available (for testing)
-	var svc SQLAdminService
-	if c.sqlAdminService != nil {
-		svc = c.sqlAdminService
-	} else {
-		service, err := sqladmin.NewService(ctx, c.clientOpts...)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create SQL admin service: %w", err)
-		}
-		svc = &realSQLAdminService{service: service}
-	}
-
-	// List all SQL instances in the project
-	instances, err := svc.ListInstances(c.projectID)
-	if err != nil {
-		return nil, fmt.Errorf("failed to list SQL instances: %w", err)
-	}
-
-	if instances == nil {
-		return commitments, nil
-	}
-
-	for _, instance := range instances.Items {
-		// Check if instance has a commitment (long-term pricing plan)
-		if instance.Settings != nil && instance.Settings.PricingPlan == "PACKAGE" {
-			commitment := common.Commitment{
-				Provider:       common.ProviderGCP,
-				Account:        c.projectID,
-				CommitmentType: common.CommitmentCUD,
-				Service:        common.ServiceRelationalDB,
-				Region:         instance.Region,
-				CommitmentID:   instance.Name,
-				State:          strings.ToLower(instance.State),
-				ResourceType:   instance.DatabaseVersion,
-			}
-
-			// Extract tier (machine type)
-			if instance.Settings.Tier != "" {
-				commitment.ResourceType = instance.Settings.Tier
-			}
-
-			commitments = append(commitments, commitment)
-		}
-	}
-
-	return commitments, nil
+// GetExistingCommitments returns an empty slice for Cloud SQL. Cloud SQL
+// spend-based CUDs are purchased via the Cloud Billing console / Cloud Commerce
+// Consumer Procurement API, not exposed through the sqladmin API. The legacy
+// PricingPlan "PACKAGE" is a per-instance billing mode (non-commitment,
+// deprecated) -- treating it as a commitment caused double-counting against
+// real spend-based CUDs (10-L3). Return empty until a proper commitment-
+// detection path is available.
+func (c *CloudSQLClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) {
+	return nil, nil
 }
 
 // PurchaseCommitment is intentionally a no-op for Cloud SQL: GCP exposes no
diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go
index 991a5df40..42b1a0600 100644
--- a/providers/gcp/services/cloudsql/client_test.go
+++ b/providers/gcp/services/cloudsql/client_test.go
@@ -300,74 +300,18 @@ func TestCloudSQLClient_ValidateOffering_TierListError(t *testing.T) {
 	assert.Contains(t, err.Error(), "failed to list SQL tiers")
 }
 
-func TestCloudSQLClient_GetExistingCommitments_WithMock(t *testing.T) {
+// TestCloudSQLClient_GetExistingCommitments_ReturnsEmpty asserts that
+// GetExistingCommitments always returns an empty slice regardless of the
+// sqladmin instance list. Cloud SQL spend-based CUDs are not detectable via
+// the sqladmin API; PricingPlan "PACKAGE" is a legacy billing mode, not a
+// commitment indicator (10-L3).
+func TestCloudSQLClient_GetExistingCommitments_ReturnsEmpty(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
-	mockService := &MockSQLAdminService{
-		instances: &sqladmin.InstancesListResponse{
-			Items: []*sqladmin.DatabaseInstance{
-				{
-					Name:            "instance-1",
-					Region:          "us-central1",
-					State:           "RUNNABLE",
-					DatabaseVersion: "MYSQL_8_0",
-					Settings: &sqladmin.Settings{
-						Tier:        "db-n1-standard-1",
-						PricingPlan: "PACKAGE",
-					},
-				},
-				{
-					Name:            "instance-2",
-					Region:          "us-central1",
-					State:           "RUNNABLE",
-					DatabaseVersion: "POSTGRES_14",
-					Settings: &sqladmin.Settings{
-						Tier:        "db-n1-standard-2",
-						PricingPlan: "PER_USE", // Not a commitment
-					},
-				},
-			},
-		},
-	}
-	client.SetSQLAdminService(mockService)
-
-	commitments, err := client.GetExistingCommitments(ctx)
-	require.NoError(t, err)
-	require.Len(t, commitments, 1)
-	assert.Equal(t, "instance-1", commitments[0].CommitmentID)
-	assert.Equal(t, "db-n1-standard-1", commitments[0].ResourceType)
-	assert.Equal(t, common.ServiceRelationalDB, commitments[0].Service)
-}
-
-func TestCloudSQLClient_GetExistingCommitments_Error(t *testing.T) {
-	ctx := context.Background()
-	client, _ := NewClient(ctx, "test-project", "us-central1")
-
-	mockService := &MockSQLAdminService{
-		err: errors.New("API error"),
-	}
-	client.SetSQLAdminService(mockService)
-
-	_, err := client.GetExistingCommitments(ctx)
-	assert.Error(t, err)
-	assert.Contains(t, err.Error(), "failed to list SQL instances")
-}
-
-func TestCloudSQLClient_GetExistingCommitments_Empty(t *testing.T) {
-	ctx := context.Background()
-	client, _ := NewClient(ctx, "test-project", "us-central1")
-
-	mockService := &MockSQLAdminService{
-		instances: &sqladmin.InstancesListResponse{
-			Items: []*sqladmin.DatabaseInstance{},
-		},
-	}
-	client.SetSQLAdminService(mockService)
-
 	commitments, err := client.GetExistingCommitments(ctx)
 	require.NoError(t, err)
-	assert.Empty(t, commitments)
+	assert.Empty(t, commitments, "Cloud SQL GetExistingCommitments must return empty (10-L3)")
 }
 
 func TestCloudSQLClient_GetValidResourceTypes_WithMock(t *testing.T) {
diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go
index d65aa3c19..8caafd85a 100644
--- a/providers/gcp/services/cloudstorage/client.go
+++ b/providers/gcp/services/cloudstorage/client.go
@@ -211,52 +211,13 @@ func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params comm
 	return recommendations, nil
 }
 
-// GetExistingCommitments retrieves existing Cloud Storage commitments
-func (c *CloudStorageClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) {
-	commitments := make([]common.Commitment, 0)
-
-	// Use injected service if available (for testing)
-	var svc StorageService
-	if c.storageService != nil {
-		svc = c.storageService
-	} else {
-		client, err := storage.NewClient(ctx, c.clientOpts...)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create storage client: %w", err)
-		}
-		svc = &realStorageService{client: client}
-	}
-	defer svc.Close()
-
-	// List all buckets in the project
-	it := svc.Buckets(ctx, c.projectID)
-	for {
-		bucket, err := it.Next()
-		if err == iterator.Done {
-			break
-		}
-		if err != nil {
-			return nil, fmt.Errorf("failed to list buckets: %w", err)
-		}
-
-		// Check if bucket has committed storage
-		if bucket.Location == c.region {
-			commitment := common.Commitment{
-				Provider:       common.ProviderGCP,
-				Account:        c.projectID,
-				CommitmentType: common.CommitmentReservedCapacity,
-				Service:        common.ServiceStorage,
-				Region:         c.region,
-				CommitmentID:   bucket.Name,
-				State:          "active",
-				ResourceType:   bucket.StorageClass,
-			}
-
-			commitments = append(commitments, commitment)
-		}
-	}
-
-	return commitments, nil
+// GetExistingCommitments returns an empty slice for Cloud Storage. GCP Cloud
+// Storage has no commitment API: there is no committed-use discount purchase
+// for GCS, and enumerating regional buckets does not represent a commitment --
+// it caused every bucket in a region to appear as a "commitment" in the UI
+// (10-L2). Return empty until a proper commitment-detection path is available.
+func (c *CloudStorageClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) {
+	return nil, nil
 }
 
 // PurchaseCommitment is intentionally a no-op for Cloud Storage: GCP has no CUD
diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go
index 0b1ebc400..0988acf5b 100644
--- a/providers/gcp/services/cloudstorage/client_test.go
+++ b/providers/gcp/services/cloudstorage/client_test.go
@@ -301,69 +301,16 @@ func TestCloudStorageClient_SetterMethods(t *testing.T) {
 	assert.Equal(t, mockBilling, client.billingService)
 }
 
-func TestCloudStorageClient_GetExistingCommitments_WithMock(t *testing.T) {
+// TestCloudStorageClient_GetExistingCommitments_ReturnsEmpty asserts that
+// GetExistingCommitments always returns an empty slice. GCS has no commitment
+// API; enumerating regional buckets does not represent a commitment (10-L2).
+func TestCloudStorageClient_GetExistingCommitments_ReturnsEmpty(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 
-	mockService := &MockStorageService{
-		buckets: []*storage.BucketAttrs{
-			{
-				Name:         "bucket-1",
-				Location:     "us-central1",
-				StorageClass: "STANDARD",
-			},
-			{
-				Name:         "bucket-2",
-				Location:     "us-central1",
-				StorageClass: "NEARLINE",
-			},
-			{
-				Name:         "bucket-other-region",
-				Location:     "europe-west1",
-				StorageClass: "COLDLINE",
-			},
-		},
-	}
-	client.SetStorageService(mockService)
-
-	commitments, err := client.GetExistingCommitments(ctx)
-	require.NoError(t, err)
-	// Only buckets in the matching region should be returned
-	assert.Len(t, commitments, 2)
-	assert.Equal(t, "bucket-1", commitments[0].CommitmentID)
-	assert.Equal(t, "STANDARD", commitments[0].ResourceType)
-	assert.Equal(t, common.ProviderGCP, commitments[0].Provider)
-	assert.Equal(t, common.ServiceStorage, commitments[0].Service)
-	assert.Equal(t, "bucket-2", commitments[1].CommitmentID)
-	assert.Equal(t, "NEARLINE", commitments[1].ResourceType)
-}
-
-func TestCloudStorageClient_GetExistingCommitments_Error(t *testing.T) {
-	ctx := context.Background()
-	client, _ := NewClient(ctx, "test-project", "us-central1")
-
-	mockService := &MockStorageService{
-		listErr: errors.New("API error"),
-	}
-	client.SetStorageService(mockService)
-
-	_, err := client.GetExistingCommitments(ctx)
-	assert.Error(t, err)
-	assert.Contains(t, err.Error(), "failed to list buckets")
-}
-
-func TestCloudStorageClient_GetExistingCommitments_Empty(t *testing.T) {
-	ctx := context.Background()
-	client, _ := NewClient(ctx, "test-project", "us-central1")
-
-	mockService := &MockStorageService{
-		buckets: []*storage.BucketAttrs{},
-	}
-	client.SetStorageService(mockService)
-
 	commitments, err := client.GetExistingCommitments(ctx)
 	require.NoError(t, err)
-	assert.Empty(t, commitments)
+	assert.Empty(t, commitments, "Cloud Storage GetExistingCommitments must return empty (10-L2)")
 }
 
 // TestCloudStorageClient_PurchaseCommitment_NotSupported is the regression test for
diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index 6b3f51e45..4c823f840 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -15,8 +15,11 @@ import (
 	"cloud.google.com/go/recommender/apiv1/recommenderpb"
 	gax "github.com/googleapis/gax-go/v2"
 	"google.golang.org/api/cloudbilling/v1"
+	"google.golang.org/api/googleapi"
 	"google.golang.org/api/iterator"
 	"google.golang.org/api/option"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/structpb"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
@@ -33,6 +36,22 @@ const maxCommitmentsPages = 50
 // maxMachineTypeItems caps GCP machine types iteration (one item per Next() call).
 const maxMachineTypeItems = 20
 
+// memMBPerVCPU is the memory-per-vCPU ratio (in MB) for GCP general-purpose
+// committed-use discounts. GCP requires both VCPU and MEMORY_MB in a single
+// commitments.insert call; the ratio is fixed at 4096 MB per vCPU for
+// GENERAL_PURPOSE commitments.
+const memMBPerVCPU = 4096
+
+// termPlan converts a term string ("1yr"/"3yr"/"3") to the GCP Compute API
+// commitment plan value ("TWELVE_MONTH" or "THIRTY_SIX_MONTH"). Unrecognised
+// terms default to TWELVE_MONTH.
+func termPlan(term string) string {
+	if term == "3yr" || term == "3" {
+		return "THIRTY_SIX_MONTH"
+	}
+	return "TWELVE_MONTH"
+}
+
 // CommitmentsService interface for commitments operations (enables mocking)
 type CommitmentsService interface {
 	List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator
@@ -313,10 +332,11 @@ func (c *ComputeEngineClient) convertGCPCommitmentToCommon(commitment *computepb
 		status = strings.ToLower(*commitment.Status)
 	}
 
+	// All commitment types (GENERAL_PURPOSE, ACCELERATOR) map to CommitmentCUD
+	// for the purposes of the common layer. The if-branch checking for
+	// "GENERAL_PURPOSE" was a no-op (both arms assigned CommitmentCUD) and
+	// is removed (10-L1).
 	commitmentType := common.CommitmentCUD
-	if commitment.Type != nil && *commitment.Type == "GENERAL_PURPOSE" {
-		commitmentType = common.CommitmentCUD
-	}
 
 	com := common.Commitment{
 		Provider:       common.ProviderGCP,
@@ -370,11 +390,7 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 		}
 		k := key{account: rec.Account, region: rec.Region, term: rec.Term}
 		if _, ok := groups[k]; !ok {
-			plan := "TWELVE_MONTH"
-			if rec.Term == "3yr" || rec.Term == "3" {
-				plan = "THIRTY_SIX_MONTH"
-			}
-			groups[k] = &agg{plan: plan}
+			groups[k] = &agg{plan: termPlan(rec.Term)}
 		}
 		groups[k].vcpus += int64(rec.Count)
 	}
@@ -389,7 +405,7 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 			Region: k.region,
 			Resources: []ResourceCommitment{
 				{Type: "VCPU", Amount: a.vcpus},
-				{Type: "MEMORY_MB", Amount: a.vcpus * 4096},
+				{Type: "MEMORY_MB", Amount: a.vcpus * memMBPerVCPU},
 			},
 		})
 		counter++
@@ -397,11 +413,26 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 	return result
 }
 
-// isResourceExhausted reports whether the error represents a RESOURCE_EXHAUSTED (429) response.
+// isResourceExhausted reports whether the error represents a RESOURCE_EXHAUSTED
+// (quota / 429) response. Uses typed checks first: errors.As for REST API errors
+// (*googleapi.Error with Code 429) and status.FromError for gRPC errors
+// (codes.ResourceExhausted). Falls back to string matching only for error types
+// that neither unwraps as a *googleapi.Error nor as a gRPC status (10-M3).
 func isResourceExhausted(err error) bool {
 	if err == nil {
 		return false
 	}
+	// REST path: *googleapi.Error wraps HTTP 429 Too Many Requests.
+	var gapiErr *googleapi.Error
+	if errors.As(err, &gapiErr) {
+		return gapiErr.Code == 429
+	}
+	// gRPC path: RESOURCE_EXHAUSTED maps to quota-exceeded responses from
+	// Google Cloud APIs served over gRPC (e.g. quota for Recommender).
+	if st, ok := status.FromError(err); ok {
+		return st.Code() == codes.ResourceExhausted
+	}
+	// Fallback for wrapped/non-standard errors that carry the signal as text.
 	s := err.Error()
 	return strings.Contains(s, "ResourceExhausted") || strings.Contains(s, "RESOURCE_EXHAUSTED") || strings.Contains(s, "429")
 }
@@ -537,10 +568,7 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 		return nil, "", fmt.Errorf("buildInsertRequest: rec.Count must be > 0 (got %d); a zero-vCPU commitment is invalid (issue #1022)", rec.Count)
 	}
 
-	plan := "TWELVE_MONTH"
-	if rec.Term == "3yr" || rec.Term == "3" {
-		plan = "THIRTY_SIX_MONTH"
-	}
+	plan := termPlan(rec.Term)
 
 	// GCP's computepb.Commitment has no Labels field, and the RegionCommitments
 	// client exposes no SetLabels call — CUDs cannot be tagged or labeled via
@@ -566,7 +594,7 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 			},
 			{
 				Type:   stringPtr("MEMORY_MB"),
-				Amount: int64Ptr(int64(rec.Count) * 4096),
+				Amount: int64Ptr(int64(rec.Count) * memMBPerVCPU),
 			},
 		},
 	}
diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go
index 73ffac2ce..81f930c5d 100644
--- a/providers/gcp/services/memorystore/client.go
+++ b/providers/gcp/services/memorystore/client.go
@@ -118,13 +118,23 @@ func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusR
 	return r.service.Services.Skus.List(serviceID).Do()
 }
 
+// realRecommenderIterator wraps the real recommender iterator (10-L4: makes
+// the memorystore client diffable against the other three service clients).
+type realRecommenderIterator struct {
+	it *recommender.RecommendationIterator
+}
+
+func (r *realRecommenderIterator) Next() (*recommenderpb.Recommendation, error) {
+	return r.it.Next()
+}
+
 // realRecommenderClient wraps the actual recommender client
 type realRecommenderClient struct {
 	client *recommender.Client
 }
 
 func (r *realRecommenderClient) ListRecommendations(ctx context.Context, req *recommenderpb.ListRecommendationsRequest) RecommenderIterator {
-	return r.client.ListRecommendations(ctx, req)
+	return &realRecommenderIterator{it: r.client.ListRecommendations(ctx, req)}
 }
 
 func (r *realRecommenderClient) Close() error {

From 3491d789731716f37f681b77c494bd931c8dcfd3 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 09:34:14 -0700
Subject: [PATCH 05/10] fix(gcp/computeengine): use valid MEMORY resource enum
 on CUD insert (#1022)

GCP's ResourceCommitment.Type enum is VCPU/MEMORY/LOCAL_SSD/ACCELERATOR.
buildInsertRequest and the public GroupCommitments helper emitted the
invalid type string "MEMORY_MB" for the memory member, so every Compute
Engine CUD purchase would be rejected by RegionCommitments.Insert. The
Amount unit (MB) was already correct; only the enum string was wrong.

Switch the outbound type to "MEMORY", make the inbound Recommender memory
detector (isMemoryAmountOp) tolerate both "MEMORY" and the legacy
"MEMORY_MB" spelling since the path_filter wire format is not documented,
and update the resource-pair doc comments.

Regression tests: TestGroupCommitments_UsesValidMemoryEnum and
TestIsMemoryAmountOp_MatchesBothSpellings added; the converter->insert
test now rejects any invalid ResourceCommitment.Type. All fail on the
pre-fix "MEMORY_MB" code and pass after.
---
 .../gcp/services/computeengine/client.go      | 30 +++++---
 .../gcp/services/computeengine/client_test.go | 72 ++++++++++++++++++-
 2 files changed, 91 insertions(+), 11 deletions(-)

diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index 4c823f840..9fe65f114 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -37,8 +37,9 @@ const maxCommitmentsPages = 50
 const maxMachineTypeItems = 20
 
 // memMBPerVCPU is the memory-per-vCPU ratio (in MB) for GCP general-purpose
-// committed-use discounts. GCP requires both VCPU and MEMORY_MB in a single
-// commitments.insert call; the ratio is fixed at 4096 MB per vCPU for
+// committed-use discounts. GCP requires both a VCPU and a MEMORY resource (the
+// memory Amount is expressed in MB) in a single commitments.insert call; the
+// ratio is fixed at 4096 MB per vCPU for
 // GENERAL_PURPOSE commitments.
 const memMBPerVCPU = 4096
 
@@ -362,7 +363,7 @@ func (c *ComputeEngineClient) convertGCPCommitmentToCommon(commitment *computepb
 // ResourceCommitment represents a single resource within a GCP commitment.
 type ResourceCommitment struct {
 	Amount int64  // number of vCPUs or memory in MB
-	Type   string // "VCPU" or "MEMORY_MB"
+	Type   string // GCP ResourceCommitment.Type enum: "VCPU" or "MEMORY"
 }
 
 // CommitmentRequest represents a single GCP commitment to create.
@@ -374,7 +375,8 @@ type CommitmentRequest struct {
 }
 
 // GroupCommitments groups recommendations by project+region+term into CommitmentRequests.
-// GCP requires both VCPU and MEMORY_MB in a single commitments.insert call.
+// GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a
+// single commitments.insert call.
 // Each recommendation's Count is treated as vCPU count; memory is estimated at 4 GB per vCPU.
 func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 	type key struct{ account, region, term string }
@@ -405,7 +407,9 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 			Region: k.region,
 			Resources: []ResourceCommitment{
 				{Type: "VCPU", Amount: a.vcpus},
-				{Type: "MEMORY_MB", Amount: a.vcpus * memMBPerVCPU},
+				// "MEMORY" is the GCP ResourceCommitment.Type enum member; the
+				// Amount is in MB (see buildInsertRequest, issue #1022).
+				{Type: "MEMORY", Amount: a.vcpus * memMBPerVCPU},
 			},
 		})
 		counter++
@@ -581,7 +585,8 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 
 	commitmentName := idempotentCommitmentName(opts.IdempotencyToken)
 
-	// GCP requires both VCPU and MEMORY_MB in a single commitment insert.
+	// GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a
+	// single commitment insert.
 	commitment := &computepb.Commitment{
 		Name:        stringPtr(commitmentName),
 		Plan:        stringPtr(plan),
@@ -593,7 +598,11 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 				Amount: int64Ptr(int64(rec.Count)),
 			},
 			{
-				Type:   stringPtr("MEMORY_MB"),
+				// GCP's ResourceCommitment.Type enum is VCPU/MEMORY/LOCAL_SSD/
+				// ACCELERATOR; the memory member is "MEMORY" (the Amount is still in
+				// MB). Sending "MEMORY_MB" is an invalid enum value and GCP rejects
+				// the commitments.insert, failing the purchase (issue #1022).
+				Type:   stringPtr("MEMORY"),
 				Amount: int64Ptr(int64(rec.Count) * memMBPerVCPU),
 			},
 		},
@@ -962,16 +971,19 @@ func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, r
 	}
 }
 
-// isMemoryAmountOp returns true when op's path_filters indicate a MEMORY_MB
+// isMemoryAmountOp returns true when op's path_filters indicate a memory
 // resource type. Used by extractVCPUCountFromRecommendation to skip the memory
 // sibling of the VCPU operation in a GCP commitment resource operation group.
+// Matches both "MEMORY" (the canonical ResourceCommitment.Type enum member) and
+// the legacy "MEMORY_MB" spelling, since the Recommender's path_filter encoding
+// is not contractually documented (issue #1022).
 func isMemoryAmountOp(op *recommenderpb.Operation) bool {
 	for filterKey, filterVal := range op.GetPathFilters() {
 		if !strings.Contains(strings.ToLower(filterKey), "type") {
 			continue
 		}
 		if sv, ok := filterVal.GetKind().(*structpb.Value_StringValue); ok {
-			if strings.EqualFold(sv.StringValue, "MEMORY_MB") {
+			if strings.EqualFold(sv.StringValue, "MEMORY") || strings.EqualFold(sv.StringValue, "MEMORY_MB") {
 				return true
 			}
 		}
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index 336ebf135..ce9a88fd8 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -1038,17 +1038,25 @@ func TestConverterToInsert_CountNonZero_VCPUAmountSet(t *testing.T) {
 	require.NotNil(t, insertReq)
 	require.NotNil(t, insertReq.CommitmentResource)
 
+	// The resource Type strings MUST be valid GCP ResourceCommitment.Type enum
+	// members (VCPU/MEMORY/LOCAL_SSD/ACCELERATOR). "MEMORY_MB" is NOT a valid
+	// enum value and GCP rejects the commitments.insert, failing the purchase
+	// (issue #1022). This loop asserts on the canonical "MEMORY" spelling.
 	var vcpuAmount, memoryAmount int64
+	var sawInvalidType string
 	for _, r := range insertReq.CommitmentResource.Resources {
 		switch r.GetType() {
 		case "VCPU":
 			vcpuAmount = r.GetAmount()
-		case "MEMORY_MB":
+		case "MEMORY":
 			memoryAmount = r.GetAmount()
+		default:
+			sawInvalidType = r.GetType()
 		}
 	}
+	assert.Empty(t, sawInvalidType, "insert request must use only valid ResourceCommitment.Type enum members; got %q (issue #1022)", sawInvalidType)
 	assert.Greater(t, vcpuAmount, int64(0), "VCPU Amount in the insert request must be > 0 (issue #1022 C1)")
-	assert.Greater(t, memoryAmount, int64(0), "MEMORY_MB Amount in the insert request must be > 0 (issue #1022 C1)")
+	assert.Greater(t, memoryAmount, int64(0), "MEMORY Amount in the insert request must be > 0 (issue #1022 C1)")
 }
 
 // TestConverterFillsPricingForScorer is the regression test for issue #1022 C2.
@@ -1191,3 +1199,63 @@ func TestConvertGCPRecommendation_ParamPaymentOptionRespected(t *testing.T) {
 	require.NotNil(t, rec)
 	assert.Equal(t, "monthly", rec.PaymentOption)
 }
+
+// TestGroupCommitments_UsesValidMemoryEnum is a regression test for issue #1022:
+// GroupCommitments must emit the canonical GCP ResourceCommitment.Type enum
+// member "MEMORY" (Amount in MB), not the invalid "MEMORY_MB" string that GCP
+// rejects on commitments.insert. Fails on the pre-fix code that emitted
+// "MEMORY_MB".
+func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) {
+	recs := []common.Recommendation{
+		{
+			Provider: common.ProviderGCP,
+			Service:  common.ServiceCompute,
+			Account:  "test-project",
+			Region:   "us-central1",
+			Term:     "1yr",
+			Count:    4,
+		},
+	}
+
+	groups := GroupCommitments(recs)
+	require.Len(t, groups, 1, "one project+region+term group expected")
+
+	var sawVCPU, sawMemory bool
+	for _, r := range groups[0].Resources {
+		switch r.Type {
+		case "VCPU":
+			sawVCPU = true
+			assert.Equal(t, int64(4), r.Amount, "VCPU amount must equal the summed Count")
+		case "MEMORY":
+			sawMemory = true
+			assert.Equal(t, int64(4*memMBPerVCPU), r.Amount, "MEMORY amount must be 4096 MB per vCPU")
+		default:
+			t.Fatalf("invalid ResourceCommitment.Type %q (issue #1022): must be VCPU/MEMORY/LOCAL_SSD/ACCELERATOR", r.Type)
+		}
+	}
+	assert.True(t, sawVCPU, "GroupCommitments must include a VCPU resource")
+	assert.True(t, sawMemory, "GroupCommitments must include a MEMORY resource (issue #1022)")
+}
+
+// TestIsMemoryAmountOp_MatchesBothSpellings asserts that the inbound Recommender
+// memory-op detector skips both the canonical "MEMORY" and legacy "MEMORY_MB"
+// path_filter spellings, so the VCPU extractor never mistakes the memory sibling
+// for the vCPU amount (issue #1022).
+func TestIsMemoryAmountOp_MatchesBothSpellings(t *testing.T) {
+	for _, typeVal := range []string{"MEMORY", "MEMORY_MB", "memory"} {
+		sv, _ := structpb.NewValue(typeVal)
+		op := &recommenderpb.Operation{
+			PathFilters: map[string]*structpb.Value{
+				"/resources/*/type": sv,
+			},
+		}
+		assert.True(t, isMemoryAmountOp(op), "isMemoryAmountOp must skip memory op with type %q", typeVal)
+	}
+
+	// A VCPU op (or an op with no type filter) must NOT be treated as memory.
+	svVCPU, _ := structpb.NewValue("VCPU")
+	vcpuOp := &recommenderpb.Operation{
+		PathFilters: map[string]*structpb.Value{"/resources/*/type": svVCPU},
+	}
+	assert.False(t, isMemoryAmountOp(vcpuOp), "isMemoryAmountOp must not skip a VCPU op")
+}

From c6280c390088e273e11720db3260358b034c4ce7 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 10:36:15 -0700
Subject: [PATCH 06/10] fix(gcp): address CR post-commit findings on payment
 option, term, and pricing math

- computeengine: force paymentOption="monthly" unconditionally; log non-monthly
  inputs instead of passing them through (GCP CUDs are monthly-only).
- memorystore/cloudstorage: propagate params.Term to rec.Term in
  convertGCPRecommendation; hardcoded "1yr" dropped 3yr caller requests.
- memorystore/cloudstorage: fix unit-mismatch in getRedisPricing/getStoragePricing
  where per-hour commitmentPrice was passed to calculateSavingsPercentage as a
  term total, producing ~99.99% savings. Multiply by hoursInTerm before math;
  HourlyRate is now the raw per-hour SKU rate.

Regression tests: 5 new tests that fail on pre-fix code.
Follow-up for CloudSQL identical issue: closes #1078.
---
 providers/gcp/services/cloudstorage/client.go |  17 ++-
 .../gcp/services/cloudstorage/client_test.go  | 119 +++++++++++++++++-
 .../gcp/services/computeengine/client.go      |  18 +--
 .../gcp/services/computeengine/client_test.go |  22 ++++
 providers/gcp/services/memorystore/client.go  |  17 ++-
 .../gcp/services/memorystore/client_test.go   | 109 ++++++++++++++++
 6 files changed, 283 insertions(+), 19 deletions(-)

diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go
index 8caafd85a..c32e44ceb 100644
--- a/providers/gcp/services/cloudstorage/client.go
+++ b/providers/gcp/services/cloudstorage/client.go
@@ -338,11 +338,15 @@ func (c *CloudStorageClient) getStoragePricing(ctx context.Context, storageClass
 	}
 
 	hoursInTerm := 8760.0 * float64(termYears)
-	savingsPercentage := calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
+	// Scale the per-unit commitment price to a term total so it is on the
+	// same basis as onDemandPrice * hoursInTerm. Without this, the savings
+	// percentage would be nearly 100% (per-unit price vs term total).
+	commitmentPriceTerm := commitmentPrice * hoursInTerm
+	savingsPercentage := calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm)
 
 	return &StoragePricing{
-		HourlyRate:        commitmentPrice / hoursInTerm,
-		CommitmentPrice:   commitmentPrice,
+		HourlyRate:        commitmentPrice,
+		CommitmentPrice:   commitmentPriceTerm,
 		OnDemandPrice:     onDemandPrice * hoursInTerm,
 		Currency:          currency,
 		SavingsPercentage: savingsPercentage,
@@ -502,6 +506,11 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 		paymentOption = "monthly"
 	}
 
+	term := params.Term
+	if term == "" {
+		term = "1yr"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceStorage,
@@ -509,7 +518,7 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe
 		Region:         c.region,
 		CommitmentType: common.CommitmentReservedCapacity,
 		Timestamp:      time.Now(),
-		Term:           "1yr",
+		Term:           term,
 		PaymentOption:  paymentOption,
 	}
 
diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go
index 0988acf5b..0a9b586af 100644
--- a/providers/gcp/services/cloudstorage/client_test.go
+++ b/providers/gcp/services/cloudstorage/client_test.go
@@ -709,8 +709,14 @@ func TestCloudStorageClient_GetStoragePricing_WithCommitmentPrice(t *testing.T)
 	require.NoError(t, err)
 	assert.Equal(t, "USD", pricing.Currency)
 	assert.Greater(t, pricing.OnDemandPrice, float64(0))
-	// When commitment price is found, it should be used
-	assert.Equal(t, float64(0.02), pricing.CommitmentPrice)
+	// CommitmentPrice is the term total (per-unit SKU price * hoursInTerm).
+	// onDemand unit = 0.026, commitment unit = 0.020, hoursInTerm = 8760.
+	assert.InDelta(t, 0.02*8760, pricing.CommitmentPrice, 0.01)
+	// HourlyRate is the per-unit commitment price (not divided by hoursInTerm).
+	assert.InDelta(t, 0.02, pricing.HourlyRate, 0.0001)
+	// SavingsPercentage should be positive and less than 100.
+	assert.Greater(t, pricing.SavingsPercentage, float64(0))
+	assert.Less(t, pricing.SavingsPercentage, float64(100))
 }
 
 func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) {
@@ -733,6 +739,115 @@ func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) {
 	assert.Greater(t, pricing.CommitmentPrice, float64(0))
 }
 
+// TestConvertGCPRecommendation_PropagatesTermFromParams is a regression test for
+// the finding that convertGCPRecommendation hardcoded rec.Term = "1yr",
+// ignoring params.Term. A caller requesting "3yr" must get "3yr" in the output.
+//
+// This test FAILS on the pre-fix code that always set Term = "1yr".
+func TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+
+	tests := []struct {
+		inputTerm string
+		wantTerm  string
+	}{
+		{"3yr", "3yr"},
+		{"1yr", "1yr"},
+		{"", "1yr"}, // empty defaults to "1yr"
+	}
+
+	for _, tt := range tests {
+		params := common.RecommendationParams{Term: tt.inputTerm}
+		rec := client.convertGCPRecommendation(ctx, gcpRec, params)
+		require.NotNil(t, rec)
+		assert.Equal(t, tt.wantTerm, rec.Term,
+			"params.Term %q must produce rec.Term %q", tt.inputTerm, tt.wantTerm)
+	}
+}
+
+// TestGetStoragePricing_CommitmentPriceIsTermTotal is a regression test for the
+// unit-mismatch bug where commitmentPrice (per-unit from SKU) was passed
+// directly to calculateStorageSavingsPercentage which expects a term total,
+// producing a ~99.99% savings percentage. After the fix, CommitmentPrice must
+// equal unitRate * hoursInTerm and SavingsPercentage must be realistic.
+//
+// This test FAILS on the pre-fix code where CommitmentPrice == 0.02 (per-unit).
+func TestGetStoragePricing_CommitmentPriceIsTermTotal(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	// onDemand = $0.026/unit, commitment = $0.020/unit
+	// hoursInTerm (1yr) = 8760
+	// Expected: CommitmentPrice = 0.020*8760 = 175.2, OnDemandPrice = 0.026*8760 = 227.76
+	// Expected savings ~= (227.76-175.2)/227.76*100 ~= 23%
+	mockService := &MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{
+			Skus: []*cloudbilling.Sku{
+				{
+					Description:    "Standard Storage in us-central1",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        26000000, // $0.026/unit
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+				{
+					Description:    "Standard Storage Commitment in us-central1",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        20000000, // $0.020/unit
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	client.SetBillingService(mockService)
+
+	pricing, err := client.getStoragePricing(ctx, "STANDARD", "us-central1", 1)
+	require.NoError(t, err)
+
+	const hoursInYear = 8760.0
+	// CommitmentPrice must be a term total, not the raw per-unit SKU rate.
+	assert.InDelta(t, 0.02*hoursInYear, pricing.CommitmentPrice, 0.01,
+		"CommitmentPrice must be commitment SKU unit rate * hoursInTerm")
+	// HourlyRate must be the per-unit commitment rate.
+	assert.InDelta(t, 0.02, pricing.HourlyRate, 0.0001,
+		"HourlyRate must be the per-unit commitment SKU rate")
+	// OnDemandPrice stays a term total.
+	assert.InDelta(t, 0.026*hoursInYear, pricing.OnDemandPrice, 0.01)
+	// SavingsPercentage must be ~23%, not ~99.99%.
+	assert.Greater(t, pricing.SavingsPercentage, float64(1),
+		"SavingsPercentage must not be ~99.99% (unit-mismatch bug)")
+	assert.Less(t, pricing.SavingsPercentage, float64(60),
+		"SavingsPercentage must be a realistic storage commitment discount")
+}
+
 func TestSkuMatchesStorageClass_CaseInsensitive(t *testing.T) {
 	sku := &cloudbilling.Sku{
 		Description:    "STANDARD Storage in Americas",
diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index 9fe65f114..b6a1c5d1b 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -864,15 +864,15 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo
 // (issue #1022 C2). Pricing failures are logged but do not discard the recommendation:
 // EstimatedSavings from the Recommender payload is the authoritative savings signal.
 func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
-	paymentOption := params.PaymentOption
-	if paymentOption == "" {
-		// GCP CUDs are billed monthly with no upfront option; "monthly" is
-		// the only valid default. Aligns with cloudsql/memorystore/cloudstorage
-		// and supersedes the monthly stamp introduced in PR #829 (which
-		// stamped "monthly" in the purchase body but not in the converter
-		// default -- see PR #1047 for the supersession note).
-		paymentOption = "monthly"
-	}
+	// GCP CUDs are billed monthly with no upfront option; force "monthly"
+	// unconditionally and log any non-monthly input so scheduler
+	// misconfiguration is visible. Supersedes the monthly stamp introduced
+	// in PR #829 (which stamped "monthly" in the purchase body but not in
+	// the converter default -- see PR #1047 for the supersession note).
+	if params.PaymentOption != "" && !strings.EqualFold(params.PaymentOption, "monthly") {
+		log.Printf("computeengine: unsupported GCP payment option %q; forcing monthly", params.PaymentOption)
+	}
+	paymentOption := "monthly"
 
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index ce9a88fd8..705bf96ed 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -1237,6 +1237,28 @@ func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) {
 	assert.True(t, sawMemory, "GroupCommitments must include a MEMORY resource (issue #1022)")
 }
 
+// TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly is a
+// regression test for the CR finding that non-monthly values (e.g. "upfront")
+// are silently propagated to the recommendation, which is invalid for GCP CUDs
+// that only support monthly billing. The converter must always emit "monthly".
+//
+// This test FAILS on the pre-fix code that passed params.PaymentOption through
+// unchanged when it was non-empty.
+func TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+
+	for _, input := range []string{"upfront", "all-upfront", "UPFRONT", "partial-upfront"} {
+		params := common.RecommendationParams{PaymentOption: input}
+		rec := client.convertGCPRecommendation(ctx, gcpRec, params)
+		require.NotNil(t, rec)
+		assert.Equal(t, "monthly", rec.PaymentOption,
+			"GCP CUDs are monthly-only; input %q must be forced to \"monthly\"", input)
+	}
+}
+
 // TestIsMemoryAmountOp_MatchesBothSpellings asserts that the inbound Recommender
 // memory-op detector skips both the canonical "MEMORY" and legacy "MEMORY_MB"
 // path_filter spellings, so the VCPU extractor never mistakes the memory sibling
diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go
index 81f930c5d..327f4f1b9 100644
--- a/providers/gcp/services/memorystore/client.go
+++ b/providers/gcp/services/memorystore/client.go
@@ -329,11 +329,15 @@ func (c *MemorystoreClient) getRedisPricing(ctx context.Context, tier, region st
 	}
 
 	hoursInTerm := 8760.0 * float64(termYears)
-	savingsPercentage := calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
+	// Scale the per-hour commitment price to a term total so it is on the
+	// same basis as onDemandPrice * hoursInTerm. Without this, the savings
+	// percentage would be nearly 100% (per-hour price vs term total).
+	commitmentPriceTerm := commitmentPrice * hoursInTerm
+	savingsPercentage := calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm)
 
 	return &RedisPricing{
-		HourlyRate:        commitmentPrice / hoursInTerm,
-		CommitmentPrice:   commitmentPrice,
+		HourlyRate:        commitmentPrice,
+		CommitmentPrice:   commitmentPriceTerm,
 		OnDemandPrice:     onDemandPrice * hoursInTerm,
 		Currency:          currency,
 		SavingsPercentage: savingsPercentage,
@@ -493,6 +497,11 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 		paymentOption = "monthly"
 	}
 
+	term := params.Term
+	if term == "" {
+		term = "1yr"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceCache,
@@ -500,7 +509,7 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec
 		Region:         c.region,
 		CommitmentType: common.CommitmentCUD,
 		Timestamp:      time.Now(),
-		Term:           "1yr",
+		Term:           term,
 		PaymentOption:  paymentOption,
 	}
 
diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go
index d97330cf8..4ce0213f0 100644
--- a/providers/gcp/services/memorystore/client_test.go
+++ b/providers/gcp/services/memorystore/client_test.go
@@ -646,6 +646,115 @@ func TestMemorystoreClient_ConvertGCPRecommendation(t *testing.T) {
 	}
 }
 
+// TestConvertGCPRecommendation_PropagatesTermFromParams is a regression test for
+// the finding that convertGCPRecommendation hardcoded rec.Term = "1yr",
+// ignoring params.Term. A caller requesting "3yr" must get "3yr" in the output.
+//
+// This test FAILS on the pre-fix code that always set Term = "1yr".
+func TestConvertGCPRecommendation_PropagatesTermFromParams(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+
+	tests := []struct {
+		inputTerm string
+		wantTerm  string
+	}{
+		{"3yr", "3yr"},
+		{"1yr", "1yr"},
+		{"", "1yr"}, // empty defaults to "1yr"
+	}
+
+	for _, tt := range tests {
+		params := common.RecommendationParams{Term: tt.inputTerm}
+		rec := client.convertGCPRecommendation(ctx, gcpRec, params)
+		require.NotNil(t, rec)
+		assert.Equal(t, tt.wantTerm, rec.Term,
+			"params.Term %q must produce rec.Term %q", tt.inputTerm, tt.wantTerm)
+	}
+}
+
+// TestGetRedisPricing_CommitmentPriceIsTermTotal is a regression test for the
+// unit-mismatch bug where commitmentPrice (per-hour from SKU) was passed
+// directly to calculateSavingsPercentage which expects a term total, producing
+// a ~99.99% savings percentage. After the fix, CommitmentPrice must equal
+// hourlyRate * hoursInTerm and SavingsPercentage must be a realistic fraction.
+//
+// This test FAILS on the pre-fix code where CommitmentPrice == 0.035 (per-hour).
+func TestGetRedisPricing_CommitmentPriceIsTermTotal(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	// onDemand = $0.05/hr, commitment = $0.035/hr
+	// hoursInTerm (1yr) = 8760
+	// Expected: CommitmentPrice = 0.035*8760 = 306.6, OnDemandPrice = 0.05*8760 = 438
+	// Expected savings ~= (438-306.6)/438*100 ~= 30%
+	mockBilling := &MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{
+			Skus: []*cloudbilling.Sku{
+				{
+					Description:    "Memorystore Redis STANDARD_HA instance",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											CurrencyCode: "USD",
+											Units:        0,
+											Nanos:        50000000, // $0.05/hr
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+				{
+					Description:    "Memorystore Redis STANDARD_HA commitment 1yr",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											CurrencyCode: "USD",
+											Units:        0,
+											Nanos:        35000000, // $0.035/hr
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	client.SetBillingService(mockBilling)
+
+	pricing, err := client.getRedisPricing(ctx, "STANDARD_HA", "us-central1", 1)
+	require.NoError(t, err)
+
+	const hoursInYear = 8760.0
+	// CommitmentPrice must be a term total, not the raw per-hour SKU rate.
+	assert.InDelta(t, 0.035*hoursInYear, pricing.CommitmentPrice, 0.01,
+		"CommitmentPrice must be commitment SKU hourly rate * hoursInTerm")
+	// HourlyRate must be the per-hour commitment rate.
+	assert.InDelta(t, 0.035, pricing.HourlyRate, 0.0001,
+		"HourlyRate must be the per-hour commitment SKU rate")
+	// OnDemandPrice stays a term total.
+	assert.InDelta(t, 0.05*hoursInYear, pricing.OnDemandPrice, 0.01)
+	// SavingsPercentage must be ~30%, not ~99.99%.
+	assert.Greater(t, pricing.SavingsPercentage, float64(1),
+		"SavingsPercentage must not be ~99.99% (unit-mismatch bug)")
+	assert.Less(t, pricing.SavingsPercentage, float64(60),
+		"SavingsPercentage must be a realistic 1yr CUD discount")
+}
+
 func TestMemorystoreClient_SetterMethods(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")

From 20590c6b4b261e4a5d805acf85192f47fd8dfb1d Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 10:42:30 -0700
Subject: [PATCH 07/10] fix(gcp/cloudsql): correct savings unit, term
 propagation (closes #1078)

Two bugs mirroring the Memorystore/CloudStorage fixes from PR #1047:

1. Unit mismatch in getSQLPricing: commitmentPrice (per-hour SKU rate) was
   passed directly to calculateSQLSavingsPercentage which expects a term
   total, producing ~99.99% savings. Now multiplies by hoursInTerm first
   (commitmentPriceTerm = commitmentPrice * hoursInTerm). HourlyRate is now
   the raw per-hour rate instead of per-hour / hours (near-zero).

2. Term propagation in convertGCPRecommendation: rec.Term was hardcoded to
   "1yr", ignoring params.Term. A caller requesting "3yr" now gets "3yr".

PaymentOption was not affected: CloudSQL already defaults to "monthly" when
params.PaymentOption is empty, matching the Memorystore/CloudStorage pattern.

Regression tests: 2 new tests that fail pre-fix and pass after.
---
 providers/gcp/services/cloudsql/client.go     |  17 ++-
 .../gcp/services/cloudsql/client_test.go      | 107 ++++++++++++++++++
 2 files changed, 120 insertions(+), 4 deletions(-)

diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go
index dd803f67a..571842b10 100644
--- a/providers/gcp/services/cloudsql/client.go
+++ b/providers/gcp/services/cloudsql/client.go
@@ -345,11 +345,15 @@ func (c *CloudSQLClient) getSQLPricing(ctx context.Context, tier, region string,
 	}
 
 	hoursInTerm := 8760.0 * float64(termYears)
-	savingsPercentage := calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice)
+	// Scale the per-hour commitment price to a term total so it is on the
+	// same basis as onDemandPrice * hoursInTerm. Without this, the savings
+	// percentage would be nearly 100% (per-hour price vs term total).
+	commitmentPriceTerm := commitmentPrice * hoursInTerm
+	savingsPercentage := calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm)
 
 	return &SQLPricing{
-		HourlyRate:        commitmentPrice / hoursInTerm,
-		CommitmentPrice:   commitmentPrice,
+		HourlyRate:        commitmentPrice,
+		CommitmentPrice:   commitmentPriceTerm,
 		OnDemandPrice:     onDemandPrice * hoursInTerm,
 		Currency:          currency,
 		SavingsPercentage: savingsPercentage,
@@ -510,6 +514,11 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 		paymentOption = "monthly"
 	}
 
+	term := params.Term
+	if term == "" {
+		term = "1yr"
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceRelationalDB,
@@ -517,7 +526,7 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r
 		Region:         c.region,
 		CommitmentType: common.CommitmentCUD,
 		Timestamp:      time.Now(),
-		Term:           "1yr",
+		Term:           term,
 		PaymentOption:  paymentOption,
 	}
 
diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go
index 42b1a0600..9a047808d 100644
--- a/providers/gcp/services/cloudsql/client_test.go
+++ b/providers/gcp/services/cloudsql/client_test.go
@@ -733,3 +733,110 @@ func TestCloudSQLClient_ConvertGCPRecommendation_NilContent(t *testing.T) {
 	require.NotNil(t, rec)
 	assert.Equal(t, common.ProviderGCP, rec.Provider)
 }
+
+// TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams is a regression
+// test for the finding that convertGCPRecommendation hardcoded rec.Term = "1yr",
+// ignoring params.Term. A caller requesting "3yr" must get "3yr" in the output.
+//
+// This test FAILS on the pre-fix code that always set Term = "1yr".
+func TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+
+	tests := []struct {
+		inputTerm string
+		wantTerm  string
+	}{
+		{"3yr", "3yr"},
+		{"1yr", "1yr"},
+		{"", "1yr"}, // empty defaults to "1yr"
+	}
+
+	for _, tt := range tests {
+		params := common.RecommendationParams{Term: tt.inputTerm}
+		rec := client.convertGCPRecommendation(ctx, gcpRec, params)
+		require.NotNil(t, rec)
+		assert.Equal(t, tt.wantTerm, rec.Term,
+			"params.Term %q must produce rec.Term %q", tt.inputTerm, tt.wantTerm)
+	}
+}
+
+// TestGetSQLPricing_CommitmentPriceIsTermTotal is a regression test for the
+// unit-mismatch bug where commitmentPrice (per-hour from SKU) was passed
+// directly to calculateSQLSavingsPercentage which expects a term total,
+// producing a ~99.99% savings percentage. After the fix, CommitmentPrice must
+// equal hourlyRate * hoursInTerm and SavingsPercentage must be realistic.
+//
+// This test FAILS on the pre-fix code where CommitmentPrice == 0.042 (per-hour).
+func TestGetSQLPricing_CommitmentPriceIsTermTotal(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	// onDemand = $0.05/hr, commitment = $0.042/hr
+	// hoursInTerm (1yr) = 8760
+	// Expected: CommitmentPrice = 0.042*8760 = 367.92, OnDemandPrice = 0.05*8760 = 438
+	// Expected savings ~= (438-367.92)/438*100 ~= 16%
+	mockService := &MockBillingService{
+		skus: &cloudbilling.ListSkusResponse{
+			Skus: []*cloudbilling.Sku{
+				{
+					Description:    "db-n1-standard-1 Cloud SQL",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        50000000, // $0.05/hr
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+				{
+					Description:    "db-n1-standard-1 Cloud SQL commitment",
+					ServiceRegions: []string{"us-central1"},
+					PricingInfo: []*cloudbilling.PricingInfo{
+						{
+							PricingExpression: &cloudbilling.PricingExpression{
+								TieredRates: []*cloudbilling.TierRate{
+									{
+										UnitPrice: &cloudbilling.Money{
+											Units:        0,
+											Nanos:        42000000, // $0.042/hr
+											CurrencyCode: "USD",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	client.SetBillingService(mockService)
+
+	pricing, err := client.getSQLPricing(ctx, "db-n1-standard-1", "us-central1", 1)
+	require.NoError(t, err)
+
+	const hoursInYear = 8760.0
+	// CommitmentPrice must be a term total, not the raw per-hour SKU rate.
+	assert.InDelta(t, 0.042*hoursInYear, pricing.CommitmentPrice, 0.01,
+		"CommitmentPrice must be commitment SKU hourly rate * hoursInTerm")
+	// HourlyRate must be the per-hour commitment rate.
+	assert.InDelta(t, 0.042, pricing.HourlyRate, 0.0001,
+		"HourlyRate must be the per-hour commitment SKU rate")
+	// SavingsPercentage must be realistic (not ~99.99%).
+	assert.Greater(t, pricing.SavingsPercentage, float64(0),
+		"SavingsPercentage must be positive")
+	assert.Less(t, pricing.SavingsPercentage, float64(50),
+		"SavingsPercentage must be realistic (not ~100%)")
+}

From 8f9a787098403856870374fc0b007d1bd7c18112 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 11:10:28 -0700
Subject: [PATCH 08/10] refactor(gcp): extract memory from payload and use
 Commitment_Plan enum (de-hardcode)

Target 1 - MEMORY amount from Recommender payload:
- Add memoryMBFromOperationGroups (parallel to vcpuCountFromOperationGroups)
  to extract the MEMORY resource amount from the commitment operation group.
- Add extractMemoryMBFromRecommendation to store the payload memory in
  ComputeDetails.MemoryGB on the Recommendation (called from convertGCPRecommendation).
- Add memoryMBFromDetails to read the value back; returns an error (fail-loud)
  when absent -- no silent fallback to the 4096 MB/vCPU ratio.
- Remove the memMBPerVCPU const (no longer used).
- buildInsertRequest and GroupCommitments now use memoryMBFromDetails; both
  return/log an error for recs that lack the payload memory field.
- Regression tests: TestBuildInsertRequest_RefusesMissingMemory,
  TestGroupCommitments_SkipsRecsWithoutMemory, and strengthened
  TestConverterToInsert_CountNonZero_VCPUAmountSet (asserts 6144 MB from
  payload, not ratio-derived 16384 MB).

Target 2 - termPlan SDK enum constants:
- termPlan now returns (string, error) using
  computepb.Commitment_TWELVE_MONTH.String() and
  computepb.Commitment_THIRTY_SIX_MONTH.String().
- Accepts 1yr/1/12mo (TWELVE_MONTH) and 3yr/3/36mo (THIRTY_SIX_MONTH).
- Returns an error on unrecognised/empty term (no silent 12-month default).
- Error threaded through buildInsertRequest, GroupCommitments, and
  convertGCPRecommendation (which returns nil on bad term).
- Regression tests: TestTermPlan_UsesSdkEnumConstants,
  TestTermPlan_RejectsUnknownTerm, TestTermPlan_AcceptsAllDocumentedForms.

H-3 audit finding - convertGCPRecommendation propagates params.Term:
- No longer hardcodes Term: "1yr"; propagates params.Term (default "1yr").
- Validates the term via termPlan and returns nil (skips rec) when invalid.
- Regression tests: TestConvertGCPRecommendation_PropagatesParamsTerm,
  TestConvertGCPRecommendation_RejectsUnknownTerm.

All existing 247 GCP tests pass; 57 computeengine tests pass under -race.
---
 .../gcp/services/computeengine/client.go      | 151 ++++++++--
 .../gcp/services/computeengine/client_test.go | 262 ++++++++++++++++--
 2 files changed, 368 insertions(+), 45 deletions(-)

diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index b6a1c5d1b..daad5169c 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -36,21 +36,25 @@ const maxCommitmentsPages = 50
 // maxMachineTypeItems caps GCP machine types iteration (one item per Next() call).
 const maxMachineTypeItems = 20
 
-// memMBPerVCPU is the memory-per-vCPU ratio (in MB) for GCP general-purpose
-// committed-use discounts. GCP requires both a VCPU and a MEMORY resource (the
-// memory Amount is expressed in MB) in a single commitments.insert call; the
-// ratio is fixed at 4096 MB per vCPU for
-// GENERAL_PURPOSE commitments.
-const memMBPerVCPU = 4096
 
-// termPlan converts a term string ("1yr"/"3yr"/"3") to the GCP Compute API
-// commitment plan value ("TWELVE_MONTH" or "THIRTY_SIX_MONTH"). Unrecognised
-// terms default to TWELVE_MONTH.
-func termPlan(term string) string {
-	if term == "3yr" || term == "3" {
-		return "THIRTY_SIX_MONTH"
+// termPlan converts a commitment term string to the canonical GCP Compute API
+// commitment plan value derived from the SDK enum constants.
+//
+// Accepted forms:
+//   - 1-year: "1yr", "1", "12mo"
+//   - 3-year: "3yr", "3", "36mo"
+//
+// An empty or unrecognised term returns an error rather than silently defaulting
+// to 12 months; a silent mis-default can purchase the wrong term and waste money.
+func termPlan(term string) (string, error) {
+	switch strings.ToLower(strings.TrimSpace(term)) {
+	case "3yr", "3", "36mo":
+		return computepb.Commitment_THIRTY_SIX_MONTH.String(), nil
+	case "1yr", "1", "12mo":
+		return computepb.Commitment_TWELVE_MONTH.String(), nil
+	default:
+		return "", fmt.Errorf("termPlan: unrecognised commitment term %q (accepted: 1yr/1/12mo or 3yr/3/36mo)", term)
 	}
-	return "TWELVE_MONTH"
 }
 
 // CommitmentsService interface for commitments operations (enables mocking)
@@ -377,12 +381,16 @@ type CommitmentRequest struct {
 // GroupCommitments groups recommendations by project+region+term into CommitmentRequests.
 // GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a
 // single commitments.insert call.
-// Each recommendation's Count is treated as vCPU count; memory is estimated at 4 GB per vCPU.
+// Each recommendation's Count is treated as vCPU count; the memory Amount is
+// read from ComputeDetails.MemoryGB (populated by extractMemoryMBFromRecommendation).
+// Recommendations with an unrecognised term or missing memory are skipped with a
+// log warning so a bad rec never contaminates an otherwise valid group.
 func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 	type key struct{ account, region, term string }
 	type agg struct {
-		vcpus int64
-		plan  string
+		vcpus    int64
+		memoryMB int64
+		plan     string
 	}
 	groups := make(map[key]*agg)
 
@@ -390,11 +398,22 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 		if rec.Service != common.ServiceCompute || rec.Provider != common.ProviderGCP {
 			continue
 		}
+		plan, err := termPlan(rec.Term)
+		if err != nil {
+			log.Printf("GroupCommitments: skipping recommendation with unrecognised term %q: %v", rec.Term, err)
+			continue
+		}
+		recMemMB, err := memoryMBFromDetails(rec)
+		if err != nil {
+			log.Printf("GroupCommitments: skipping recommendation missing memory amount: %v", err)
+			continue
+		}
 		k := key{account: rec.Account, region: rec.Region, term: rec.Term}
 		if _, ok := groups[k]; !ok {
-			groups[k] = &agg{plan: termPlan(rec.Term)}
+			groups[k] = &agg{plan: plan}
 		}
 		groups[k].vcpus += int64(rec.Count)
+		groups[k].memoryMB += recMemMB
 	}
 
 	result := make([]CommitmentRequest, 0, len(groups))
@@ -409,7 +428,7 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
 				{Type: "VCPU", Amount: a.vcpus},
 				// "MEMORY" is the GCP ResourceCommitment.Type enum member; the
 				// Amount is in MB (see buildInsertRequest, issue #1022).
-				{Type: "MEMORY", Amount: a.vcpus * memMBPerVCPU},
+				{Type: "MEMORY", Amount: a.memoryMB},
 			},
 		})
 		counter++
@@ -572,10 +591,13 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 		return nil, "", fmt.Errorf("buildInsertRequest: rec.Count must be > 0 (got %d); a zero-vCPU commitment is invalid (issue #1022)", rec.Count)
 	}
 
-	plan := termPlan(rec.Term)
+	plan, err := termPlan(rec.Term)
+	if err != nil {
+		return nil, "", fmt.Errorf("buildInsertRequest: %w", err)
+	}
 
 	// GCP's computepb.Commitment has no Labels field, and the RegionCommitments
-	// client exposes no SetLabels call — CUDs cannot be tagged or labeled via
+	// client exposes no SetLabels call -- CUDs cannot be tagged or labeled via
 	// the API. Encode the source into Description so customers can still filter
 	// with `gcloud compute commitments list --filter="description:..."`.
 	description := fmt.Sprintf("CUD for %s", rec.ResourceType)
@@ -585,6 +607,17 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 
 	commitmentName := idempotentCommitmentName(opts.IdempotencyToken)
 
+	// Derive the memory Amount from the Recommender payload (stored in
+	// ComputeDetails.MemoryGB by extractMemoryMBFromRecommendation). Using the
+	// real payload value is required for non-standard families (e.g. high-memory
+	// N2 uses 6 GB/vCPU rather than the GENERAL_PURPOSE 4 GB). We fail loud when
+	// the payload omits memory rather than silently falling back to a ratio that
+	// would produce an incorrect commitment for any non-standard family.
+	memMB, err := memoryMBFromDetails(rec)
+	if err != nil {
+		return nil, "", err
+	}
+
 	// GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a
 	// single commitment insert.
 	commitment := &computepb.Commitment{
@@ -603,7 +636,7 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts
 				// MB). Sending "MEMORY_MB" is an invalid enum value and GCP rejects
 				// the commitments.insert, failing the purchase (issue #1022).
 				Type:   stringPtr("MEMORY"),
-				Amount: int64Ptr(int64(rec.Count) * memMBPerVCPU),
+				Amount: int64Ptr(memMB),
 			},
 		},
 	}
@@ -863,6 +896,8 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo
 // BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
 // (issue #1022 C2). Pricing failures are logged but do not discard the recommendation:
 // EstimatedSavings from the Recommender payload is the authoritative savings signal.
+// Returns nil when the params.Term is unrecognised so the caller skips an
+// unroutable recommendation rather than queuing a purchase with an invalid plan.
 func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
 	// GCP CUDs are billed monthly with no upfront option; force "monthly"
 	// unconditionally and log any non-monthly input so scheduler
@@ -874,6 +909,17 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR
 	}
 	paymentOption := "monthly"
 
+	// H-3: propagate params.Term (default "1yr") and validate it so an
+	// unrecognised term fails loud here rather than reaching buildInsertRequest.
+	term := params.Term
+	if term == "" {
+		term = "1yr"
+	}
+	if _, err := termPlan(term); err != nil {
+		log.Printf("computeengine: skipping recommendation with unrecognised term %q: %v", term, err)
+		return nil
+	}
+
 	rec := &common.Recommendation{
 		Provider:       common.ProviderGCP,
 		Service:        common.ServiceCompute,
@@ -881,13 +927,14 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR
 		Region:         c.region,
 		CommitmentType: common.CommitmentCUD,
 		Timestamp:      time.Now(),
-		Term:           "1yr",
+		Term:           term,
 		PaymentOption:  paymentOption,
 	}
 
 	extractResourceTypeFromRecommendation(gcpRec, rec)
 	extractCostImpactFromRecommendation(gcpRec, rec)
 	extractVCPUCountFromRecommendation(gcpRec, rec)
+	extractMemoryMBFromRecommendation(gcpRec, rec)
 
 	// Thread pricing into the converter so the scorer can rank/filter GCP recs
 	// correctly (issue #1022 C2). If the billing catalog lacks a commitment SKU
@@ -1024,8 +1071,8 @@ func vcpuCountFromOperationGroups(content *recommenderpb.RecommendationContent)
 //   - Operation.Value: a structpb.Value whose numeric value is the amount, with
 //     Operation.Path indicating the resource type (e.g. "/resources/0/amount").
 //     Operations with ResourceType "compute.googleapis.com/Commitment" and a path
-//     containing "amount" carry the VCPU count; the sibling MEMORY_MB amount is
-//     always 4096 * vcpuCount for general-purpose commitments.
+//     containing "amount" carry the VCPU count; the sibling MEMORY amount is
+//     extracted by extractMemoryMBFromRecommendation.
 //   - RecommendationContent.Overview: a JSON struct with a "numericValue" field.
 //
 // We prefer the operation-value path because it is structured and unambiguous.
@@ -1050,6 +1097,62 @@ func extractVCPUCountFromRecommendation(gcpRec *recommenderpb.Recommendation, re
 	}
 }
 
+// memoryMBFromOperationGroups walks the commitment operation groups and returns
+// the MEMORY resource amount (in MB) encoded in the operation's numeric value,
+// or 0 if none is found. It is the sibling of vcpuCountFromOperationGroups:
+// both look at "compute.googleapis.com/Commitment" operations whose Path
+// contains "amount", but this function selects only those where isMemoryAmountOp
+// returns true (i.e. the path_filter type is MEMORY or MEMORY_MB).
+func memoryMBFromOperationGroups(content *recommenderpb.RecommendationContent) int64 {
+	for _, opGroup := range content.GetOperationGroups() {
+		for _, op := range opGroup.GetOperations() {
+			if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") {
+				continue
+			}
+			if !strings.Contains(strings.ToLower(op.GetPath()), "amount") {
+				continue
+			}
+			if !isMemoryAmountOp(op) {
+				continue
+			}
+			if v := op.GetValue(); v != nil {
+				if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 {
+					return int64(nv.NumberValue)
+				}
+			}
+		}
+	}
+	return 0
+}
+
+// extractMemoryMBFromRecommendation extracts the MEMORY resource amount (in MB)
+// from the Recommender payload and stores it in rec.Details as ComputeDetails.MemoryGB.
+// This allows buildInsertRequest to use the payload-sourced amount rather than the
+// general-purpose ratio approximation. No-op when the payload carries no MEMORY op.
+func extractMemoryMBFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) {
+	if gcpRec.Content == nil {
+		return
+	}
+	memMB := memoryMBFromOperationGroups(gcpRec.Content)
+	if memMB <= 0 {
+		return
+	}
+	// Store as ComputeDetails.MemoryGB (MB -> GB conversion). GCP memory amounts
+	// are always whole multiples of at least 256 MB, so the float64 roundtrip is
+	// exact: memMB / 1024 * 1024 == memMB for any value divisible by 1.
+	rec.Details = common.ComputeDetails{MemoryGB: float64(memMB) / 1024.0}
+}
+
+// memoryMBFromDetails reads the MEMORY amount (in MB) from rec.Details when it
+// was populated by extractMemoryMBFromRecommendation. Returns an error when the
+// field is absent or zero -- callers must not silently fall back to a ratio.
+func memoryMBFromDetails(rec common.Recommendation) (int64, error) {
+	if cd, ok := rec.Details.(common.ComputeDetails); ok && cd.MemoryGB > 0 {
+		return int64(cd.MemoryGB * 1024), nil
+	}
+	return 0, fmt.Errorf("memoryMBFromDetails: MEMORY resource amount absent from recommendation Details (no MEMORY op in Recommender payload); cannot build CUD insert without explicit memory")
+}
+
 // idempotentNameTokenLen is how many leading hex characters of the idempotency
 // token are folded into the derived commitment name. A GCP commitment name must
 // match RFC1035 (1-63 chars, lowercase [a-z]([-a-z0-9]*[a-z0-9])?); the "cud-"
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index 705bf96ed..5a560aefa 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -473,6 +473,7 @@ func TestComputeEngineClient_PurchaseCommitment_WithMock(t *testing.T) {
 		Term:           "1yr",
 		CommitmentCost: 1000.0,
 		Count:          5,
+		Details:        common.ComputeDetails{MemoryGB: 20.0}, // 5 vCPU * 4 GB
 	}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
@@ -489,7 +490,12 @@ func TestComputeEngineClient_PurchaseCommitment_EncodesSourceInDescription(t *te
 	mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}}
 	client.SetCommitmentsService(mockService)
 
-	rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1}
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-1",
+		Term:         "1yr",
+		Count:        1,
+		Details:      common.ComputeDetails{MemoryGB: 4.0}, // 1 vCPU * 4 GB
+	}
 
 	_, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceWeb})
 	require.NoError(t, err)
@@ -506,7 +512,12 @@ func TestComputeEngineClient_PurchaseCommitment_OmitsTagWhenSourceEmpty(t *testi
 	mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}}
 	client.SetCommitmentsService(mockService)
 
-	rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1}
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-1",
+		Term:         "1yr",
+		Count:        1,
+		Details:      common.ComputeDetails{MemoryGB: 4.0},
+	}
 
 	_, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
 	require.NoError(t, err)
@@ -528,7 +539,12 @@ func TestComputeEngineClient_PurchaseCommitment_IdempotentReDrive(t *testing.T)
 	mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}}
 	client.SetCommitmentsService(mockService)
 
-	rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 5}
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-1",
+		Term:         "1yr",
+		Count:        5,
+		Details:      common.ComputeDetails{MemoryGB: 20.0}, // 5 vCPU * 4 GB
+	}
 	token := common.DeriveIdempotencyToken("exec-654", 0)
 	opts := common.PurchaseOptions{Source: common.PurchaseSourceWeb, IdempotencyToken: token}
 
@@ -569,7 +585,12 @@ func TestComputeEngineClient_PurchaseCommitment_EmptyTokenNoRequestID(t *testing
 	mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}}
 	client.SetCommitmentsService(mockService)
 
-	rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1}
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-1",
+		Term:         "1yr",
+		Count:        1,
+		Details:      common.ComputeDetails{MemoryGB: 4.0},
+	}
 
 	_, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
 	require.NoError(t, err)
@@ -593,6 +614,7 @@ func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) {
 		ResourceType: "n1-standard-1",
 		Term:         "3yr",
 		Count:        4, // must be > 0 after issue #1022 guard
+		Details:      common.ComputeDetails{MemoryGB: 16.0}, // 4 vCPU * 4 GB
 	}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
@@ -609,8 +631,14 @@ func TestComputeEngineClient_PurchaseCommitment_InsertError(t *testing.T) {
 	}
 	client.SetCommitmentsService(mockService)
 
-	// Count must be > 0 so the guard passes and the insert error is exercised.
-	rec := common.Recommendation{ResourceType: "n1-standard-1", Count: 2}
+	// Count must be > 0, Term must be valid, and Details.MemoryGB must be set so
+	// all pre-insert guards pass and the insert error is exercised.
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-1",
+		Term:         "1yr",
+		Count:        2,
+		Details:      common.ComputeDetails{MemoryGB: 8.0},
+	}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
 	assert.Error(t, err)
@@ -627,8 +655,14 @@ func TestComputeEngineClient_PurchaseCommitment_WaitError(t *testing.T) {
 	}
 	client.SetCommitmentsService(mockService)
 
-	// Count must be > 0 so the guard passes and the wait error is exercised.
-	rec := common.Recommendation{ResourceType: "n1-standard-1", Count: 2}
+	// Count must be > 0, Term must be valid, and Details.MemoryGB must be set so
+	// all pre-insert guards pass and the wait error is exercised.
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-1",
+		Term:         "1yr",
+		Count:        2,
+		Details:      common.ComputeDetails{MemoryGB: 8.0},
+	}
 
 	result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
 	assert.Error(t, err)
@@ -911,15 +945,22 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) {
 }
 
 // realisticCUDRecommendation builds a realistic GCP Commitment Recommender payload
-// for a 4-vCPU n1-standard-4 commitment in us-central1. The operation group has two ops:
+// for a 4-vCPU n1-standard-4 commitment in us-central1. The operation groups have
+// three ops:
 //  1. A machine-type op whose resource path ends in the machine type (n1-standard-4) --
 //     used by extractResourceTypeFromOperations to set rec.ResourceType.
-//  2. A commitment resource op whose numeric Value is the VCPU count --
-//     used by extractVCPUCountFromRecommendation to set rec.Count.
+//  2. A VCPU commitment resource op whose numeric Value is the VCPU count --
+//     used by extractVCPUCountFromRecommendation to set rec.Count = 4.
+//  3. A MEMORY commitment resource op whose numeric Value is 6144 MB --
+//     used by extractMemoryMBFromRecommendation to set rec.Details.MemoryGB = 6.
+//     Using 6144 MB (1536 MB/vCPU) intentionally tests a non-4096-ratio case to
+//     confirm the value is read from the payload rather than computed from a ratio.
 //
-// This mirrors the GCP CUD Recommender format (issue #1022 C1).
+// This mirrors the GCP CUD Recommender format (issue #1022 C1 + memory fix).
 func realisticCUDRecommendation() *recommenderpb.Recommendation {
 	vcpuVal, _ := structpb.NewValue(4.0)
+	memVal, _ := structpb.NewValue(6144.0) // 6144 MB = 6 GB; ratio is 1536 MB/vCPU, NOT 4096
+	memTypeFilter, _ := structpb.NewValue("MEMORY")
 	return &recommenderpb.Recommendation{
 		Name: "projects/test/locations/us-central1/recommenders/google.billing.CostInsight.commitmentRecommender/recommendations/rec-001",
 		PrimaryImpact: &recommenderpb.Impact{
@@ -949,7 +990,7 @@ func realisticCUDRecommendation() *recommenderpb.Recommendation {
 				{
 					Operations: []*recommenderpb.Operation{
 						{
-							// Commitment resource op: carries the VCPU amount as a numeric value.
+							// VCPU op: carries the vCPU count as a numeric value.
 							// extractVCPUCountFromRecommendation reads this and sets rec.Count = 4.
 							Action:       "add",
 							ResourceType: "compute.googleapis.com/Commitment",
@@ -957,6 +998,19 @@ func realisticCUDRecommendation() *recommenderpb.Recommendation {
 							Path:         "/resources/0/amount",
 							PathValue:    &recommenderpb.Operation_Value{Value: vcpuVal},
 						},
+						{
+							// MEMORY op: carries the memory amount in MB as a numeric value.
+							// extractMemoryMBFromRecommendation reads this (path_filter type=MEMORY)
+							// and sets rec.Details = ComputeDetails{MemoryGB: 6.0}.
+							Action:       "add",
+							ResourceType: "compute.googleapis.com/Commitment",
+							Resource:     "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001",
+							Path:         "/resources/1/amount",
+							PathValue:    &recommenderpb.Operation_Value{Value: memVal},
+							PathFilters: map[string]*structpb.Value{
+								"/resources/1/type": memTypeFilter,
+							},
+						},
 					},
 				},
 			},
@@ -1017,24 +1071,33 @@ func mockBillingWithCommitment() *MockBillingService {
 // for issue #1022 C1. It exercises the full converter->purchase path with a
 // realistic Recommender payload and asserts:
 //   - converter sets Count > 0 (extracted from the operation's numeric value)
-//   - buildInsertRequest produces a non-zero VCPU Amount in the resulting commitment
+//   - converter extracts the MEMORY amount from the payload (not a ratio)
+//   - buildInsertRequest produces the exact VCPU and MEMORY Amounts
 //
-// This test FAILS on the pre-fix code where convertGCPRecommendation never set Count.
+// The payload uses 6144 MB for 4 vCPUs (1536 MB/vCPU, NOT 4096). An old ratio-
+// based implementation would produce 4*4096 = 16384 MB; the correct value is
+// 6144 MB. This assertion fails on pre-fix ratio-based code.
 func TestConverterToInsert_CountNonZero_VCPUAmountSet(t *testing.T) {
 	ctx := context.Background()
 	client, _ := NewClient(ctx, "test-project", "us-central1")
 	client.SetBillingService(mockBillingWithCommitment())
 
-	gcpRec := realisticCUDRecommendation()
+	gcpRec := realisticCUDRecommendation() // 4 vCPU, 6144 MB (from payload)
 	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
 	require.NotNil(t, rec)
 
 	// C1: Count must be > 0 so that buildInsertRequest produces non-zero VCPU Amount.
 	require.Greater(t, rec.Count, 0, "convertGCPRecommendation must set Count > 0 from the Recommender payload (issue #1022 C1)")
 
-	// Verify the insert request carries the correct VCPU count.
+	// Memory must be extracted from the payload and stored in Details.
+	cd, ok := rec.Details.(common.ComputeDetails)
+	require.True(t, ok, "convertGCPRecommendation must populate ComputeDetails (memory extracted from payload)")
+	assert.Equal(t, 6.0, cd.MemoryGB,
+		"MEMORY amount from payload is 6144 MB = 6 GB; ratio-based code would produce 16 GB (4*4096 MB)")
+
+	// Verify the insert request carries the correct VCPU and MEMORY amounts.
 	insertReq, _, buildErr := client.buildInsertRequest(*rec, common.PurchaseOptions{})
-	require.NoError(t, buildErr, "buildInsertRequest must not error when Count > 0")
+	require.NoError(t, buildErr, "buildInsertRequest must not error when Count > 0 and memory is present")
 	require.NotNil(t, insertReq)
 	require.NotNil(t, insertReq.CommitmentResource)
 
@@ -1055,8 +1118,10 @@ func TestConverterToInsert_CountNonZero_VCPUAmountSet(t *testing.T) {
 		}
 	}
 	assert.Empty(t, sawInvalidType, "insert request must use only valid ResourceCommitment.Type enum members; got %q (issue #1022)", sawInvalidType)
-	assert.Greater(t, vcpuAmount, int64(0), "VCPU Amount in the insert request must be > 0 (issue #1022 C1)")
-	assert.Greater(t, memoryAmount, int64(0), "MEMORY Amount in the insert request must be > 0 (issue #1022 C1)")
+	assert.Equal(t, int64(4), vcpuAmount, "VCPU Amount in the insert request must equal the extracted count")
+	// Exact memory check: payload says 6144 MB; old ratio (4*4096=16384 MB) would fail.
+	assert.Equal(t, int64(6144), memoryAmount,
+		"MEMORY Amount must match the payload's 6144 MB (not ratio-derived 4*4096=16384 MB)")
 }
 
 // TestConverterFillsPricingForScorer is the regression test for issue #1022 C2.
@@ -1205,7 +1270,14 @@ func TestConvertGCPRecommendation_ParamPaymentOptionRespected(t *testing.T) {
 // member "MEMORY" (Amount in MB), not the invalid "MEMORY_MB" string that GCP
 // rejects on commitments.insert. Fails on the pre-fix code that emitted
 // "MEMORY_MB".
+// The memory amount is read from ComputeDetails.MemoryGB (set by the converter);
+// recommendations without Details are skipped.
 func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) {
+	// 4 vCPU, 24 GB memory (non-4096-ratio to confirm the value is read from
+	// Details, not computed from the old ratio).
+	const wantMemoryGB = 24.0
+	const wantMemoryMB = int64(wantMemoryGB * 1024)
+
 	recs := []common.Recommendation{
 		{
 			Provider: common.ProviderGCP,
@@ -1214,6 +1286,7 @@ func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) {
 			Region:   "us-central1",
 			Term:     "1yr",
 			Count:    4,
+			Details:  common.ComputeDetails{MemoryGB: wantMemoryGB},
 		},
 	}
 
@@ -1228,7 +1301,7 @@ func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) {
 			assert.Equal(t, int64(4), r.Amount, "VCPU amount must equal the summed Count")
 		case "MEMORY":
 			sawMemory = true
-			assert.Equal(t, int64(4*memMBPerVCPU), r.Amount, "MEMORY amount must be 4096 MB per vCPU")
+			assert.Equal(t, wantMemoryMB, r.Amount, "MEMORY amount must equal the payload-sourced MB (issue #1022)")
 		default:
 			t.Fatalf("invalid ResourceCommitment.Type %q (issue #1022): must be VCPU/MEMORY/LOCAL_SSD/ACCELERATOR", r.Type)
 		}
@@ -1237,6 +1310,26 @@ func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) {
 	assert.True(t, sawMemory, "GroupCommitments must include a MEMORY resource (issue #1022)")
 }
 
+// TestGroupCommitments_SkipsRecsWithoutMemory verifies that GroupCommitments skips
+// recommendations that have no Details.MemoryGB (payload omitted memory) rather
+// than producing a zero-memory commitment that GCP would reject.
+func TestGroupCommitments_SkipsRecsWithoutMemory(t *testing.T) {
+	recs := []common.Recommendation{
+		{
+			Provider: common.ProviderGCP,
+			Service:  common.ServiceCompute,
+			Account:  "test-project",
+			Region:   "us-central1",
+			Term:     "1yr",
+			Count:    4,
+			// No Details: memory is absent from the payload.
+		},
+	}
+
+	groups := GroupCommitments(recs)
+	assert.Empty(t, groups, "GroupCommitments must skip recs without payload memory, not produce a zero-memory commitment")
+}
+
 // TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly is a
 // regression test for the CR finding that non-monthly values (e.g. "upfront")
 // are silently propagated to the recommendation, which is invalid for GCP CUDs
@@ -1281,3 +1374,130 @@ func TestIsMemoryAmountOp_MatchesBothSpellings(t *testing.T) {
 	}
 	assert.False(t, isMemoryAmountOp(vcpuOp), "isMemoryAmountOp must not skip a VCPU op")
 }
+
+// TestBuildInsertRequest_RefusesMissingMemory is the regression test for the
+// "fail loud" policy on missing memory: buildInsertRequest must return an error
+// when rec.Details does not carry a ComputeDetails.MemoryGB value (i.e. the
+// Recommender payload omitted the MEMORY op). A silent fallback to a fixed ratio
+// would produce incorrect commitments for non-standard machine families.
+//
+// This test fails on pre-fix code that silently used memMBPerVCPU as a fallback.
+func TestBuildInsertRequest_RefusesMissingMemory(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	rec := common.Recommendation{
+		ResourceType: "n1-standard-4",
+		Term:         "1yr",
+		Count:        4,
+		// No Details: memory absent from Recommender payload.
+	}
+
+	_, _, err := client.buildInsertRequest(rec, common.PurchaseOptions{})
+	require.Error(t, err, "buildInsertRequest must refuse when memory is absent from the payload")
+	assert.Contains(t, err.Error(), "MEMORY resource amount absent",
+		"error must name the cause so operators can diagnose the missing Recommender field")
+
+	// PurchaseCommitment must surface the error and not call Insert.
+	mockSvc := &MockCommitmentsService{operation: &MockOperation{}}
+	client.SetCommitmentsService(mockSvc)
+	result, purchaseErr := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})
+	require.Error(t, purchaseErr)
+	assert.False(t, result.Success)
+	assert.Empty(t, mockSvc.insertReqs, "Insert must not be called when memory is absent")
+}
+
+// TestTermPlan_UsesSdkEnumConstants verifies that termPlan returns the exact string
+// produced by the SDK enum's String() method (not a hand-written literal), so any
+// future SDK rename propagates automatically.
+func TestTermPlan_UsesSdkEnumConstants(t *testing.T) {
+	plan12, err := termPlan("1yr")
+	require.NoError(t, err)
+	assert.Equal(t, computepb.Commitment_TWELVE_MONTH.String(), plan12,
+		"1yr must map to the SDK TWELVE_MONTH constant, not a string literal")
+
+	plan36, err := termPlan("3yr")
+	require.NoError(t, err)
+	assert.Equal(t, computepb.Commitment_THIRTY_SIX_MONTH.String(), plan36,
+		"3yr must map to the SDK THIRTY_SIX_MONTH constant, not a string literal")
+}
+
+// TestTermPlan_RejectsUnknownTerm is the regression test for the "fail loud" policy:
+// termPlan must return an error rather than silently defaulting to 12 months when
+// given an unrecognised or empty term. A silent mis-default can purchase the wrong
+// duration and waste money.
+//
+// This test fails on pre-fix code that silently returned TWELVE_MONTH for any
+// unrecognised input.
+func TestTermPlan_RejectsUnknownTerm(t *testing.T) {
+	for _, badTerm := range []string{"", "2yr", "invalid", "24mo", "forever"} {
+		_, err := termPlan(badTerm)
+		require.Error(t, err, "termPlan must reject unrecognised term %q (no silent 12-month default)", badTerm)
+		assert.Contains(t, err.Error(), "unrecognised commitment term",
+			"error for term %q must identify the bad input", badTerm)
+	}
+}
+
+// TestTermPlan_AcceptsAllDocumentedForms asserts that termPlan accepts all
+// documented 1-year and 3-year input forms.
+func TestTermPlan_AcceptsAllDocumentedForms(t *testing.T) {
+	for _, form := range []string{"1yr", "1", "12mo"} {
+		got, err := termPlan(form)
+		require.NoError(t, err, "termPlan must accept 1-year form %q", form)
+		assert.Equal(t, computepb.Commitment_TWELVE_MONTH.String(), got,
+			"1-year form %q must map to TWELVE_MONTH", form)
+	}
+	for _, form := range []string{"3yr", "3", "36mo"} {
+		got, err := termPlan(form)
+		require.NoError(t, err, "termPlan must accept 3-year form %q", form)
+		assert.Equal(t, computepb.Commitment_THIRTY_SIX_MONTH.String(), got,
+			"3-year form %q must map to THIRTY_SIX_MONTH", form)
+	}
+}
+
+// TestConvertGCPRecommendation_PropagatesParamsTerm asserts that convertGCPRecommendation
+// propagates params.Term to rec.Term (H-3 audit finding). Pre-fix code hardcoded
+// "1yr" regardless of params.Term, so callers requesting "3yr" silently received
+// 1-year recommendations and 1-year pricing.
+//
+// This test FAILS on the pre-fix code that hardcoded Term: "1yr".
+func TestConvertGCPRecommendation_PropagatesParamsTerm(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+
+	// 3yr must be propagated.
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "3yr"})
+	require.NotNil(t, rec)
+	assert.Equal(t, "3yr", rec.Term,
+		"params.Term=3yr must be propagated to rec.Term (H-3 fix); pre-fix code hardcoded 1yr")
+
+	// 1yr explicit must be propagated.
+	rec = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "1yr"})
+	require.NotNil(t, rec)
+	assert.Equal(t, "1yr", rec.Term)
+
+	// Empty term must default to "1yr".
+	rec = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{})
+	require.NotNil(t, rec)
+	assert.Equal(t, "1yr", rec.Term,
+		"empty params.Term must default to 1yr")
+}
+
+// TestConvertGCPRecommendation_RejectsUnknownTerm asserts that convertGCPRecommendation
+// returns nil when given an unrecognised params.Term (e.g. "5yr"). An unroutable
+// recommendation must be dropped before it can reach buildInsertRequest and attempt
+// to insert a commitment with an invalid plan.
+//
+// This test FAILS on the pre-fix code that either silently used "1yr" or panicked.
+func TestConvertGCPRecommendation_RejectsUnknownTerm(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	gcpRec := &recommenderpb.Recommendation{Name: "test-rec"}
+
+	rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "5yr"})
+	assert.Nil(t, rec,
+		"convertGCPRecommendation must return nil for unrecognised term (not silently default to 12 months)")
+}

From 95a390f3856ca38271e97a3be2b6b25e6c176b3c Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 11:26:32 -0700
Subject: [PATCH 09/10] fix(gcp): filter non-ACTIVE recs and wire cache/storage
 into fan-out

H-1 (rec state never filtered): GetRecommendations in all four GCP
service clients (computeengine, cloudsql, memorystore, cloudstorage)
now skip recommendations whose StateInfo.State is not ACTIVE. The GCP
Recommender returns recommendations in ACTIVE/CLAIMED/SUCCEEDED/FAILED/
DISMISSED states; acting on a non-ACTIVE rec is a cross-run double-
purchase vector and inflates actionable rec counts. Uses typed enum
constant recommenderpb.RecommendationStateInfo_ACTIVE (not a string).
The proto GetStateInfo() getter is nil-safe so nil-StateInfo recs
(STATE_UNSPECIFIED) are also filtered.

H-2 (cache/storage advertised but produce zero recs): collectRegion in
recommendations.go now fans out to memorystore and cloudstorage in
addition to compute and cloudsql, guarded by shouldIncludeService.
Both services have complete GetRecommendations implementations; their
PurchaseCommitment paths are advisory-only no-ops (documented) which is
orthogonal to surfacing recommendations. regionResult gains cache and
storage slices; GetRecommendations merge appends them per region.
Decision log: docs/code-review/open-questions/fix-1047-h1h2.md.

Regression tests:
- TestGetRecommendations_FiltersNonActiveStates: five-rec mock (one
  ACTIVE, four non-ACTIVE); asserts only ACTIVE is returned. Fails
  pre-fix.
- TestGetRecommendations_ActiveRecIncluded: positive case.
- TestRegionResult_HasCacheAndStorageFields: compile-time + runtime
  check that regionResult carries all four service slices (H-2).
- TestShouldIncludeService_Cache_Storage: routing logic for cache/
  storage service types.
- Existing WithMock tests in all four services updated to set
  StateInfo=ACTIVE (they failed post-fix; pre-fix they passed against
  STATE_UNSPECIFIED=0 which was not filtered).

Follow-up: remaining Medium/Low/Nit findings from 10-gcp-and-pkg.md
tracked in issue #1083.

257 GCP tests pass under -race. go build/vet ./... clean.
---
 providers/gcp/recommendations.go              | 98 +++++++++++++++----
 providers/gcp/recommendations_test.go         | 75 ++++++++++++++
 providers/gcp/services/cloudsql/client.go     |  6 ++
 .../gcp/services/cloudsql/client_test.go      |  3 +-
 providers/gcp/services/cloudstorage/client.go |  6 ++
 .../gcp/services/cloudstorage/client_test.go  |  3 +-
 .../gcp/services/computeengine/client.go      |  8 ++
 .../gcp/services/computeengine/client_test.go | 94 +++++++++++++++++-
 providers/gcp/services/memorystore/client.go  |  6 ++
 .../gcp/services/memorystore/client_test.go   |  3 +-
 10 files changed, 277 insertions(+), 25 deletions(-)

diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go
index 3168480a3..dd96ec17a 100644
--- a/providers/gcp/recommendations.go
+++ b/providers/gcp/recommendations.go
@@ -18,7 +18,9 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/concurrency"
 	"github.com/LeanerCloud/CUDly/pkg/logging"
 	"github.com/LeanerCloud/CUDly/providers/gcp/services/cloudsql"
+	"github.com/LeanerCloud/CUDly/providers/gcp/services/cloudstorage"
 	"github.com/LeanerCloud/CUDly/providers/gcp/services/computeengine"
+	"github.com/LeanerCloud/CUDly/providers/gcp/services/memorystore"
 )
 
 // defaultGCPRegionConcurrency caps the parallel per-region goroutines inside
@@ -42,13 +44,21 @@ func gcpRegionConcurrency() int {
 	return defaultGCPRegionConcurrency
 }
 
-// regionResult bundles the Compute Engine and Cloud SQL recommendation slices
-// returned for a single GCP region. The merge in GetRecommendations walks
-// regions in sorted order and appends compute then sql per region so output
-// is deterministic independent of goroutine completion order.
+// regionResult bundles per-service recommendation slices returned for a single
+// GCP region. The merge in GetRecommendations walks regions in sorted order
+// and appends compute, sql, cache, storage per region so output is
+// deterministic independent of goroutine completion order.
+//
+// All four GCP service clients (computeengine, cloudsql, memorystore,
+// cloudstorage) implement GetRecommendations and are fanned out concurrently
+// when shouldIncludeService permits. Note that cache and storage purchase paths
+// are advisory-only (no-op PurchaseCommitment); their recommendations are still
+// surfaced so operators can see spend-optimisation signals.
 type regionResult struct {
 	compute []common.Recommendation
 	sql     []common.Recommendation
+	cache   []common.Recommendation
+	storage []common.Recommendation
 }
 
 // RecommendationsClientAdapter aggregates GCP CUD and commitment recommendations across all services
@@ -65,9 +75,10 @@ type RecommendationsClientAdapter struct {
 //   - Outer: errgroup over regions, capped at gcpRegionConcurrency()
 //     (CUDLY_GCP_REGION_PARALLELISM, default 10) to stay polite to the
 //     project-scoped Recommender API quota.
-//   - Inner: within each region's goroutine, the (compute, cloud-sql) calls
-//     run as two further goroutines under a per-region sub-errgroup, so the
-//     per-region cost is max(compute, sql) rather than compute + sql.
+//   - Inner: within each region's goroutine, the four service calls
+//     (compute, cloud-sql, memorystore, cloudstorage) run as concurrent
+//     goroutines under a per-region sub-errgroup, so the per-region cost is
+//     max(service latencies) rather than their sum.
 //
 // Behaviour change vs the previous nested for-loops: per-(region, service)
 // errors that were previously silently swallowed (`if err == nil { ... }`
@@ -132,9 +143,9 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
 		return nil, err
 	}
 
-	// Deterministic merge: walk regions in sorted order, append compute then
-	// sql per region. Output is stable regardless of GCP API region-list
-	// ordering or goroutine completion order.
+	// Deterministic merge: walk regions in sorted order, append compute, sql,
+	// cache, storage per region. Output is stable regardless of GCP API
+	// region-list ordering or goroutine completion order.
 	sortedRegions := make([]string, 0, len(results))
 	for region := range results {
 		sortedRegions = append(sortedRegions, region)
@@ -146,22 +157,29 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
 		res := results[region]
 		allRecommendations = append(allRecommendations, res.compute...)
 		allRecommendations = append(allRecommendations, res.sql...)
+		allRecommendations = append(allRecommendations, res.cache...)
+		allRecommendations = append(allRecommendations, res.storage...)
 	}
 	return allRecommendations, nil
 }
 
-// collectRegion fetches Compute Engine and Cloud SQL recommendations for a
-// single region concurrently. Per-service errors are logged at WARN with the
-// region+service tag and never propagate — the previous silent-skip-on-err
-// shape is preserved (so a misconfigured project doesn't error out the whole
-// recommendations refresh) but errors are now observable in logs. Extracted
-// from GetRecommendations to keep that function under the gocyclo gate
+// collectRegion fetches recommendations for all four GCP services
+// (Compute Engine, Cloud SQL, Memorystore, Cloud Storage) for a single region
+// concurrently. Per-service errors are logged at WARN with the region+service
+// tag and never propagate — the previous silent-skip-on-err shape is preserved
+// (so a misconfigured project doesn't error out the whole recommendations
+// refresh) but errors are now observable in logs. Extracted from
+// GetRecommendations to keep that function under the gocyclo gate
 // (.golangci.yml min-complexity: 15) after the post-Wait ctx.Err() block was
 // added.
+//
+// Note: memorystore and cloudstorage PurchaseCommitment paths are advisory-only
+// (no programmatic purchase API exists for either); their recommendations are
+// surfaced so operators can see spend-optimisation signals (H-2 fix).
 func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params common.RecommendationParams, region string) regionResult {
 	var (
-		computeRecs, sqlRecs []common.Recommendation
-		computeErr, sqlErr   error
+		computeRecs, sqlRecs, cacheRecs, storageRecs []common.Recommendation
+		computeErr, sqlErr, cacheErr, storageErr     error
 	)
 
 	g, gctx := errgroup.WithContext(ctx)
@@ -170,8 +188,8 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params
 	// Recommender API call. Acquire bounds aggregate concurrent IO across
 	// the whole recommendations-collection fan-out tree at the shared
 	// semaphore's cap (CUDLY_MAX_PARALLELISM, default 20); Release returns
-	// the slot. Without this bound the per-region fan-out (cap 10) ×
-	// per-service sub-fan-out (2) × accounts × providers can produce
+	// the slot. Without this bound the per-region fan-out (cap 10) x
+	// per-service sub-fan-out (4) x accounts x providers can produce
 	// hundreds of concurrent gRPC clients that exhaust Lambda memory. If
 	// no semaphore is on ctx (CLI tools, unit tests), Acquire/Release are
 	// no-ops. See pkg/concurrency.
@@ -207,6 +225,38 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params
 			return nil
 		})
 	}
+	if shouldIncludeService(params, common.ServiceCache) {
+		g.Go(func() error {
+			if err := concurrency.Acquire(gctx); err != nil {
+				cacheErr = err
+				return nil
+			}
+			defer concurrency.Release(gctx)
+			client, err := memorystore.NewClient(gctx, r.projectID, region, r.clientOpts...)
+			if err != nil {
+				cacheErr = err
+				return nil
+			}
+			cacheRecs, cacheErr = client.GetRecommendations(gctx, params)
+			return nil
+		})
+	}
+	if shouldIncludeService(params, common.ServiceStorage) {
+		g.Go(func() error {
+			if err := concurrency.Acquire(gctx); err != nil {
+				storageErr = err
+				return nil
+			}
+			defer concurrency.Release(gctx)
+			client, err := cloudstorage.NewClient(gctx, r.projectID, region, r.clientOpts...)
+			if err != nil {
+				storageErr = err
+				return nil
+			}
+			storageRecs, storageErr = client.GetRecommendations(gctx, params)
+			return nil
+		})
+	}
 	_ = g.Wait()
 
 	if computeErr != nil {
@@ -215,8 +265,14 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params
 	if sqlErr != nil {
 		logging.Warnf("GCP %s cloudsql recommendations: %v", region, sqlErr)
 	}
+	if cacheErr != nil {
+		logging.Warnf("GCP %s memorystore recommendations: %v", region, cacheErr)
+	}
+	if storageErr != nil {
+		logging.Warnf("GCP %s cloudstorage recommendations: %v", region, storageErr)
+	}
 
-	return regionResult{compute: computeRecs, sql: sqlRecs}
+	return regionResult{compute: computeRecs, sql: sqlRecs, cache: cacheRecs, storage: storageRecs}
 }
 
 // GetRecommendationsForService retrieves GCP commitment recommendations for a specific service
diff --git a/providers/gcp/recommendations_test.go b/providers/gcp/recommendations_test.go
index a32eb8073..d0fe486b6 100644
--- a/providers/gcp/recommendations_test.go
+++ b/providers/gcp/recommendations_test.go
@@ -160,6 +160,81 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel
 		"GetRecommendations must propagate the parent ctx error")
 }
 
+// TestRegionResult_HasCacheAndStorageFields is a compile-time regression test
+// for H-2 (GCP broad audit): regionResult must carry cache and storage slices
+// so collectRegion can fan out to memorystore and cloudstorage in addition to
+// compute and cloudsql. If this test stops compiling, the wiring was reverted.
+func TestRegionResult_HasCacheAndStorageFields(t *testing.T) {
+	recs := []common.Recommendation{{Provider: common.ProviderGCP}}
+
+	// Field access verifies that regionResult has the full four-service shape.
+	result := regionResult{
+		compute: recs,
+		sql:     recs,
+		cache:   recs,
+		storage: recs,
+	}
+
+	assert.Len(t, result.compute, 1, "compute field must be present on regionResult")
+	assert.Len(t, result.sql, 1, "sql field must be present on regionResult")
+	assert.Len(t, result.cache, 1, "cache field must be present on regionResult (H-2)")
+	assert.Len(t, result.storage, 1, "storage field must be present on regionResult (H-2)")
+}
+
+// TestShouldIncludeService_Cache_Storage verifies that shouldIncludeService
+// correctly routes ServiceCache and ServiceStorage requests, which is a
+// prerequisite for H-2 (wiring them into collectRegion).
+func TestShouldIncludeService_Cache_Storage(t *testing.T) {
+	tests := []struct {
+		name     string
+		params   common.RecommendationParams
+		service  common.ServiceType
+		expected bool
+	}{
+		{
+			name:     "all services includes Cache",
+			params:   common.RecommendationParams{},
+			service:  common.ServiceCache,
+			expected: true,
+		},
+		{
+			name:     "all services includes Storage",
+			params:   common.RecommendationParams{},
+			service:  common.ServiceStorage,
+			expected: true,
+		},
+		{
+			name:     "Cache-scoped request includes Cache",
+			params:   common.RecommendationParams{Service: common.ServiceCache},
+			service:  common.ServiceCache,
+			expected: true,
+		},
+		{
+			name:     "Cache-scoped request excludes Storage",
+			params:   common.RecommendationParams{Service: common.ServiceCache},
+			service:  common.ServiceStorage,
+			expected: false,
+		},
+		{
+			name:     "Storage-scoped request includes Storage",
+			params:   common.RecommendationParams{Service: common.ServiceStorage},
+			service:  common.ServiceStorage,
+			expected: true,
+		},
+		{
+			name:     "Storage-scoped request excludes Compute",
+			params:   common.RecommendationParams{Service: common.ServiceStorage},
+			service:  common.ServiceCompute,
+			expected: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, shouldIncludeService(tt.params, tt.service))
+		})
+	}
+}
+
 // TestGCPRegionConcurrency pins the env-knob parsing for
 // CUDLY_GCP_REGION_PARALLELISM.
 func TestGCPRegionConcurrency(t *testing.T) {
diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go
index 571842b10..8f443e3eb 100644
--- a/providers/gcp/services/cloudsql/client.go
+++ b/providers/gcp/services/cloudsql/client.go
@@ -182,6 +182,12 @@ func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.R
 			return nil, fmt.Errorf("cloudsql: iterate recommendations: %w", err)
 		}
 
+		// Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED).
+		// See computeengine.GetRecommendations for the full rationale.
+		if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE {
+			continue
+		}
+
 		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go
index 9a047808d..6a93b81ae 100644
--- a/providers/gcp/services/cloudsql/client_test.go
+++ b/providers/gcp/services/cloudsql/client_test.go
@@ -583,7 +583,8 @@ func TestCloudSQLClient_GetRecommendations_WithMock(t *testing.T) {
 	mockIterator := &MockRecommenderIterator{
 		recommendations: []*recommenderpb.Recommendation{
 			{
-				Name: "recommendation-1",
+				Name:      "recommendation-1",
+				StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE},
 				PrimaryImpact: &recommenderpb.Impact{
 					Category: recommenderpb.Impact_COST,
 					Projection: &recommenderpb.Impact_CostProjection{
diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go
index c32e44ceb..bcae9ae1a 100644
--- a/providers/gcp/services/cloudstorage/client.go
+++ b/providers/gcp/services/cloudstorage/client.go
@@ -202,6 +202,12 @@ func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params comm
 			return nil, fmt.Errorf("cloudstorage: iterate recommendations: %w", err)
 		}
 
+		// Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED).
+		// See computeengine.GetRecommendations for the full rationale.
+		if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE {
+			continue
+		}
+
 		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go
index 0a9b586af..78b874d0b 100644
--- a/providers/gcp/services/cloudstorage/client_test.go
+++ b/providers/gcp/services/cloudstorage/client_test.go
@@ -348,7 +348,8 @@ func TestCloudStorageClient_GetRecommendations_WithMock(t *testing.T) {
 	mockClient := &MockRecommenderClient{
 		recommendations: []*recommenderpb.Recommendation{
 			{
-				Name: "recommendation-1",
+				Name:      "recommendation-1",
+				StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE},
 				PrimaryImpact: &recommenderpb.Impact{
 					Category: recommenderpb.Impact_COST,
 					Projection: &recommenderpb.Impact_CostProjection{
diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index daad5169c..76b175d21 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -259,6 +259,14 @@ func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, params com
 			return nil, fmt.Errorf("computeengine: iterate recommendations: %w", err)
 		}
 
+		// Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED).
+		// The GCP Recommender returns all states unless filtered at the API layer;
+		// acting on an already-CLAIMED or SUCCEEDED recommendation is a
+		// cross-run double-purchase vector and inflates actionable rec counts.
+		if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE {
+			continue
+		}
+
 		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index 5a560aefa..8b231aa4c 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -761,7 +761,8 @@ func TestComputeEngineClient_GetRecommendations_WithMock(t *testing.T) {
 	mockIterator := &MockRecommenderIterator{
 		recommendations: []*recommenderpb.Recommendation{
 			{
-				Name: "recommendation-1",
+				Name:      "recommendation-1",
+				StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE},
 				PrimaryImpact: &recommenderpb.Impact{
 					Category: recommenderpb.Impact_COST,
 					Projection: &recommenderpb.Impact_CostProjection{
@@ -1501,3 +1502,94 @@ func TestConvertGCPRecommendation_RejectsUnknownTerm(t *testing.T) {
 	assert.Nil(t, rec,
 		"convertGCPRecommendation must return nil for unrecognised term (not silently default to 12 months)")
 }
+
+// TestGetRecommendations_FiltersNonActiveStates is a regression test for H-1
+// (GCP broad audit): the Recommender API returns recommendations in all states
+// (ACTIVE/CLAIMED/SUCCEEDED/FAILED/DISMISSED); only ACTIVE ones must be surfaced
+// as actionable. Acting on an already-CLAIMED or SUCCEEDED recommendation is a
+// cross-run double-purchase vector and inflates actionable rec counts.
+//
+// This test FAILS on the pre-fix code that converted all recommendations
+// regardless of state.
+func TestGetRecommendations_FiltersNonActiveStates(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	stateActive := recommenderpb.RecommendationStateInfo_ACTIVE
+	stateClaimed := recommenderpb.RecommendationStateInfo_CLAIMED
+	stateSucceeded := recommenderpb.RecommendationStateInfo_SUCCEEDED
+	stateFailed := recommenderpb.RecommendationStateInfo_FAILED
+	stateDismissed := recommenderpb.RecommendationStateInfo_DISMISSED
+
+	recs := []*recommenderpb.Recommendation{
+		{
+			Name:      "active-rec",
+			StateInfo: &recommenderpb.RecommendationStateInfo{State: stateActive},
+			PrimaryImpact: &recommenderpb.Impact{
+				Category: recommenderpb.Impact_COST,
+				Projection: &recommenderpb.Impact_CostProjection{
+					CostProjection: &recommenderpb.CostProjection{
+						Cost: &money.Money{Units: -100, CurrencyCode: "USD"},
+					},
+				},
+			},
+		},
+		{
+			Name:      "claimed-rec",
+			StateInfo: &recommenderpb.RecommendationStateInfo{State: stateClaimed},
+		},
+		{
+			Name:      "succeeded-rec",
+			StateInfo: &recommenderpb.RecommendationStateInfo{State: stateSucceeded},
+		},
+		{
+			Name:      "failed-rec",
+			StateInfo: &recommenderpb.RecommendationStateInfo{State: stateFailed},
+		},
+		{
+			Name:      "dismissed-rec",
+			StateInfo: &recommenderpb.RecommendationStateInfo{State: stateDismissed},
+		},
+	}
+
+	mockIterator := &MockRecommenderIterator{recommendations: recs}
+	mockClient := &MockRecommenderClient{iterator: mockIterator}
+	client.SetRecommenderClient(mockClient)
+
+	results, err := client.GetRecommendations(ctx, common.RecommendationParams{})
+	require.NoError(t, err)
+	require.Len(t, results, 1,
+		"only the ACTIVE recommendation must be returned; CLAIMED/SUCCEEDED/FAILED/DISMISSED must be filtered (H-1)")
+	assert.Equal(t, common.ProviderGCP, results[0].Provider)
+}
+
+// TestGetRecommendations_ActiveRecIncluded is the positive-case complement of
+// TestGetRecommendations_FiltersNonActiveStates: a recommendation explicitly
+// marked ACTIVE must pass the state filter.
+func TestGetRecommendations_ActiveRecIncluded(t *testing.T) {
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", "us-central1")
+
+	mockIterator := &MockRecommenderIterator{
+		recommendations: []*recommenderpb.Recommendation{
+			{
+				Name:      "active-only",
+				StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE},
+				PrimaryImpact: &recommenderpb.Impact{
+					Category: recommenderpb.Impact_COST,
+					Projection: &recommenderpb.Impact_CostProjection{
+						CostProjection: &recommenderpb.CostProjection{
+							Cost: &money.Money{Units: -50, CurrencyCode: "USD"},
+						},
+					},
+				},
+			},
+		},
+	}
+	mockClient := &MockRecommenderClient{iterator: mockIterator}
+	client.SetRecommenderClient(mockClient)
+
+	results, err := client.GetRecommendations(ctx, common.RecommendationParams{})
+	require.NoError(t, err)
+	require.Len(t, results, 1, "an ACTIVE recommendation must be included")
+}
diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go
index 327f4f1b9..ba6a2946a 100644
--- a/providers/gcp/services/memorystore/client.go
+++ b/providers/gcp/services/memorystore/client.go
@@ -192,6 +192,12 @@ func (c *MemorystoreClient) GetRecommendations(ctx context.Context, params commo
 			return nil, fmt.Errorf("memorystore: iterate recommendations: %w", err)
 		}
 
+		// Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED).
+		// See computeengine.GetRecommendations for the full rationale.
+		if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE {
+			continue
+		}
+
 		converted := c.convertGCPRecommendation(ctx, rec, params)
 		if converted != nil {
 			recommendations = append(recommendations, *converted)
diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go
index 4ce0213f0..81eff3f53 100644
--- a/providers/gcp/services/memorystore/client_test.go
+++ b/providers/gcp/services/memorystore/client_test.go
@@ -531,7 +531,8 @@ func TestMemorystoreClient_GetRecommendations_WithMockClient(t *testing.T) {
 			name: "returns recommendations successfully",
 			recommendations: []*recommenderpb.Recommendation{
 				{
-					Name: "projects/test/locations/us-central1/recommenders/google.memorystore.redis.PerformanceRecommender/recommendations/rec-1",
+					Name:      "projects/test/locations/us-central1/recommenders/google.memorystore.redis.PerformanceRecommender/recommendations/rec-1",
+					StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE},
 					Content: &recommenderpb.RecommendationContent{
 						OperationGroups: []*recommenderpb.OperationGroup{
 							{

From 33c0c7d8b275a1f53e8d1f6f94ab0d6f45d2fe83 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Sun, 7 Jun 2026 11:45:29 -0700
Subject: [PATCH 10/10] fix(gcp): reduce gocyclo complexity on collectRegion
 and convertGCPRecommendation

Extract per-service fetch helpers (collectComputeRecs, collectSQLRecs,
collectCacheRecs, collectStorageRecs) from collectRegion so the
Acquire/Release and client-construction branches are in named functions
rather than anonymous closures, bringing collectRegion from 17 to 9.

Extract enrichRecWithPricing from convertGCPRecommendation to move the
pricing lookup and BreakEvenMonths calculation into a dedicated method,
bringing convertGCPRecommendation from 12 to 5.

Apply gofmt to client.go (trailing blank line) and client_test.go
(comment alignment). No logic changes.

Fixes the failing "Check cyclomatic complexity" and "Run gofmt"
pre-commit hooks in CI for PR #1047.
---
 providers/gcp/recommendations.go              | 122 +++++++++---------
 .../gcp/services/computeengine/client.go      |  63 +++++----
 .../gcp/services/computeengine/client_test.go |   2 +-
 3 files changed, 100 insertions(+), 87 deletions(-)

diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go
index dd96ec17a..2a8c1cecc 100644
--- a/providers/gcp/recommendations.go
+++ b/providers/gcp/recommendations.go
@@ -163,15 +163,70 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
 	return allRecommendations, nil
 }
 
+// collectComputeRecs fetches Compute Engine CUD recommendations for one region.
+// Handles semaphore acquire/release and client construction so these branches
+// are not counted toward collectRegion's cyclomatic complexity.
+func (r *RecommendationsClientAdapter) collectComputeRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) {
+	if err := concurrency.Acquire(ctx); err != nil {
+		return nil, err
+	}
+	defer concurrency.Release(ctx)
+	client, err := computeengine.NewClient(ctx, r.projectID, region, r.clientOpts...)
+	if err != nil {
+		return nil, err
+	}
+	return client.GetRecommendations(ctx, params)
+}
+
+// collectSQLRecs fetches Cloud SQL CUD recommendations for one region.
+func (r *RecommendationsClientAdapter) collectSQLRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) {
+	if err := concurrency.Acquire(ctx); err != nil {
+		return nil, err
+	}
+	defer concurrency.Release(ctx)
+	client, err := cloudsql.NewClient(ctx, r.projectID, region, r.clientOpts...)
+	if err != nil {
+		return nil, err
+	}
+	return client.GetRecommendations(ctx, params)
+}
+
+// collectCacheRecs fetches Memorystore recommendations for one region.
+func (r *RecommendationsClientAdapter) collectCacheRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) {
+	if err := concurrency.Acquire(ctx); err != nil {
+		return nil, err
+	}
+	defer concurrency.Release(ctx)
+	client, err := memorystore.NewClient(ctx, r.projectID, region, r.clientOpts...)
+	if err != nil {
+		return nil, err
+	}
+	return client.GetRecommendations(ctx, params)
+}
+
+// collectStorageRecs fetches Cloud Storage recommendations for one region.
+func (r *RecommendationsClientAdapter) collectStorageRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) {
+	if err := concurrency.Acquire(ctx); err != nil {
+		return nil, err
+	}
+	defer concurrency.Release(ctx)
+	client, err := cloudstorage.NewClient(ctx, r.projectID, region, r.clientOpts...)
+	if err != nil {
+		return nil, err
+	}
+	return client.GetRecommendations(ctx, params)
+}
+
 // collectRegion fetches recommendations for all four GCP services
 // (Compute Engine, Cloud SQL, Memorystore, Cloud Storage) for a single region
 // concurrently. Per-service errors are logged at WARN with the region+service
-// tag and never propagate — the previous silent-skip-on-err shape is preserved
+// tag and never propagate -- the previous silent-skip-on-err shape is preserved
 // (so a misconfigured project doesn't error out the whole recommendations
-// refresh) but errors are now observable in logs. Extracted from
-// GetRecommendations to keep that function under the gocyclo gate
-// (.golangci.yml min-complexity: 15) after the post-Wait ctx.Err() block was
-// added.
+// refresh) but errors are now observable in logs. The per-service fetch logic
+// (semaphore, client construction, GetRecommendations call) is delegated to
+// dedicated helpers (collectComputeRecs, collectSQLRecs, collectCacheRecs,
+// collectStorageRecs) to keep this function's cyclomatic complexity under the
+// gocyclo gate.
 //
 // Note: memorystore and cloudstorage PurchaseCommitment paths are advisory-only
 // (no programmatic purchase API exists for either); their recommendations are
@@ -184,76 +239,27 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params
 
 	g, gctx := errgroup.WithContext(ctx)
 
-	// Per-(region, service) goroutines are leaves — they issue the actual
-	// Recommender API call. Acquire bounds aggregate concurrent IO across
-	// the whole recommendations-collection fan-out tree at the shared
-	// semaphore's cap (CUDLY_MAX_PARALLELISM, default 20); Release returns
-	// the slot. Without this bound the per-region fan-out (cap 10) x
-	// per-service sub-fan-out (4) x accounts x providers can produce
-	// hundreds of concurrent gRPC clients that exhaust Lambda memory. If
-	// no semaphore is on ctx (CLI tools, unit tests), Acquire/Release are
-	// no-ops. See pkg/concurrency.
 	if shouldIncludeService(params, common.ServiceCompute) {
 		g.Go(func() error {
-			if err := concurrency.Acquire(gctx); err != nil {
-				computeErr = err
-				return nil
-			}
-			defer concurrency.Release(gctx)
-			client, err := computeengine.NewClient(gctx, r.projectID, region, r.clientOpts...)
-			if err != nil {
-				computeErr = err
-				return nil
-			}
-			computeRecs, computeErr = client.GetRecommendations(gctx, params)
+			computeRecs, computeErr = r.collectComputeRecs(gctx, params, region)
 			return nil
 		})
 	}
 	if shouldIncludeService(params, common.ServiceRelationalDB) {
 		g.Go(func() error {
-			if err := concurrency.Acquire(gctx); err != nil {
-				sqlErr = err
-				return nil
-			}
-			defer concurrency.Release(gctx)
-			client, err := cloudsql.NewClient(gctx, r.projectID, region, r.clientOpts...)
-			if err != nil {
-				sqlErr = err
-				return nil
-			}
-			sqlRecs, sqlErr = client.GetRecommendations(gctx, params)
+			sqlRecs, sqlErr = r.collectSQLRecs(gctx, params, region)
 			return nil
 		})
 	}
 	if shouldIncludeService(params, common.ServiceCache) {
 		g.Go(func() error {
-			if err := concurrency.Acquire(gctx); err != nil {
-				cacheErr = err
-				return nil
-			}
-			defer concurrency.Release(gctx)
-			client, err := memorystore.NewClient(gctx, r.projectID, region, r.clientOpts...)
-			if err != nil {
-				cacheErr = err
-				return nil
-			}
-			cacheRecs, cacheErr = client.GetRecommendations(gctx, params)
+			cacheRecs, cacheErr = r.collectCacheRecs(gctx, params, region)
 			return nil
 		})
 	}
 	if shouldIncludeService(params, common.ServiceStorage) {
 		g.Go(func() error {
-			if err := concurrency.Acquire(gctx); err != nil {
-				storageErr = err
-				return nil
-			}
-			defer concurrency.Release(gctx)
-			client, err := cloudstorage.NewClient(gctx, r.projectID, region, r.clientOpts...)
-			if err != nil {
-				storageErr = err
-				return nil
-			}
-			storageRecs, storageErr = client.GetRecommendations(gctx, params)
+			storageRecs, storageErr = r.collectStorageRecs(gctx, params, region)
 			return nil
 		})
 	}
diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go
index 76b175d21..2af9cfa83 100644
--- a/providers/gcp/services/computeengine/client.go
+++ b/providers/gcp/services/computeengine/client.go
@@ -36,7 +36,6 @@ const maxCommitmentsPages = 50
 // maxMachineTypeItems caps GCP machine types iteration (one item per Next() call).
 const maxMachineTypeItems = 20
 
-
 // termPlan converts a commitment term string to the canonical GCP Compute API
 // commitment plan value derived from the SDK enum constants.
 //
@@ -944,37 +943,45 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR
 	extractVCPUCountFromRecommendation(gcpRec, rec)
 	extractMemoryMBFromRecommendation(gcpRec, rec)
 
-	// Thread pricing into the converter so the scorer can rank/filter GCP recs
-	// correctly (issue #1022 C2). If the billing catalog lacks a commitment SKU
-	// we propagate the error as a log line rather than dropping the rec entirely
-	// -- the Recommender-derived EstimatedSavings is still valid.
-	if rec.ResourceType != "" {
-		termYears := 1
-		if rec.Term == "3yr" || rec.Term == "3" {
-			termYears = 3
-		}
-		if pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears); err != nil {
-			log.Printf("computeengine: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
-		} else {
-			rec.CommitmentCost = pricing.CommitmentPrice
-			rec.OnDemandCost = pricing.OnDemandPrice
-			rec.SavingsPercentage = pricing.SavingsPercentage
-			// BreakEvenMonths: months of accrued savings required to cover the
-			// commitment cost (conservative: treats the whole period cost as sunk,
-			// which is accurate for all-upfront and overestimates for monthly CUDs).
-			// monthlySavings = monthly spend difference between on-demand and CUD.
-			if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
-				monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
-				if monthlySavings > 0 {
-					rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
-				}
-			}
-		}
-	}
+	c.enrichRecWithPricing(ctx, rec)
 
 	return rec
 }
 
+// enrichRecWithPricing fills CommitmentCost, OnDemandCost, SavingsPercentage,
+// and BreakEvenMonths on rec by querying the billing catalog. Extracted from
+// convertGCPRecommendation to keep that function's cyclomatic complexity under
+// the gocyclo gate. A missing or invalid pricing entry is logged but does not
+// discard the recommendation -- the Recommender-derived EstimatedSavings is
+// still the authoritative savings signal (issue #1022 C2).
+func (c *ComputeEngineClient) enrichRecWithPricing(ctx context.Context, rec *common.Recommendation) {
+	if rec.ResourceType == "" {
+		return
+	}
+	termYears := 1
+	if rec.Term == "3yr" || rec.Term == "3" {
+		termYears = 3
+	}
+	pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears)
+	if err != nil {
+		log.Printf("computeengine: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err)
+		return
+	}
+	rec.CommitmentCost = pricing.CommitmentPrice
+	rec.OnDemandCost = pricing.OnDemandPrice
+	rec.SavingsPercentage = pricing.SavingsPercentage
+	// BreakEvenMonths: months of accrued savings required to cover the
+	// commitment cost (conservative: treats the whole period cost as sunk,
+	// which is accurate for all-upfront and overestimates for monthly CUDs).
+	// monthlySavings = monthly spend difference between on-demand and CUD.
+	if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 {
+		monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12)
+		if monthlySavings > 0 {
+			rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings
+		}
+	}
+}
+
 // extractResourceTypeFromRecommendation extracts the resource type from a GCP recommendation
 func extractResourceTypeFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) {
 	if gcpRec.Content == nil || gcpRec.Content.OperationGroups == nil {
diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go
index 8b231aa4c..bb3e0d534 100644
--- a/providers/gcp/services/computeengine/client_test.go
+++ b/providers/gcp/services/computeengine/client_test.go
@@ -613,7 +613,7 @@ func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) {
 	rec := common.Recommendation{
 		ResourceType: "n1-standard-1",
 		Term:         "3yr",
-		Count:        4, // must be > 0 after issue #1022 guard
+		Count:        4,                                     // must be > 0 after issue #1022 guard
 		Details:      common.ComputeDetails{MemoryGB: 16.0}, // 4 vCPU * 4 GB
 	}