fix(gcp): take the compute CUD term from the recommendation plan (experimental, opt-in)

known_issues/01_gcp_compute_nonroot_term.md

@@ -0,0 +1,21 @@
+# Known Issues: GCP Compute Engine CUD term on the non-root payload path
+
+> **Audit status (2026-10-09):** `1 needs triage · 0 resolved`
+
+## MEDIUM: Non-root recommendation payloads still guess the term
+
+**Files:** `providers/gcp/services/computeengine/commitment_type.go`
+(`resolveRecTerm`, `populateRecommendationResources`)
+
+**Description:** The fix for #291 reads the commitment `plan` from the root
+Commitment operation. A recommendation with no root Commitment op has no plan
+to read, so its term still comes from `params.Term`, defaulting to "1yr".
+
+**Why deferred:** The Recommender payload shape for non-root operations is
+unverified, and #291 was scoped to the root path. Tracked in #304.
+
+**Status:** Open.
+
+**Experimental gate:** the plan-derived term path is EXPERIMENTAL and off by
+default (`RecommendationParams.ExperimentalGCPPlanTerm`). The root `plan`
+payload shape is unverified against a live Recommender.

providers/gcp/go.mod

@@ -8,7 +8,7 @@ require (
 	cloud.google.com/go/redis v1.18.3
 	cloud.google.com/go/resourcemanager v1.10.7
 	cloud.google.com/go/storage v1.59.2
-	github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012192523-3838efd56668
+	github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261013221254-77bc8a993eb8
 	github.com/googleapis/gax-go/v2 v2.21.0
 	github.com/stretchr/testify v1.11.1
 	golang.org/x/oauth2 v0.36.0

providers/gcp/go.sum

@@ -36,8 +36,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0
 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA=
 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI=
 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc=
-github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012192523-3838efd56668 h1:/m3hxkSHGd8AJlhBaPTttRDwtwC1ycYH7IbRkRKR9bM=
-github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012192523-3838efd56668/go.mod h1:8RTmzPazA8IeVZfn7wNaU7ILx0VK5eHZQozO21Mvtq8=
+github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261013221254-77bc8a993eb8 h1:0EMP5PHw8RnQ5Xl8B2firZ8CwRSpmxitMZ4cdYuL8p4=
+github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261013221254-77bc8a993eb8/go.mod h1:8RTmzPazA8IeVZfn7wNaU7ILx0VK5eHZQozO21Mvtq8=
 github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY=
 github.com/aws/aws-sdk-go-v2 v1.41.5/go.mod h1:mwsPRE8ceUUpiTgF7QmQIJ7lgsKUPQOUl3o72QBrE1o=
 github.com/aws/smithy-go v1.24.2 h1:FzA3bu/nt/vDvmnkg+R8Xl46gmzEDam6mZ1hzmwXFng=

providers/gcp/recommendations_sdk_test.go

@@ -248,7 +248,7 @@ func sdkCostRecommendation(savings int64, state recommenderpb.RecommendationStat
 // Schema root fixture, not a captured service response.
 func sdkRootCommitment(kind, region string) *recommenderpb.RecommendationContent {
 	value, _ := structpb.NewStruct(map[string]any{
-		"type": kind, "resources": []any{
+		"type": kind, "plan": "TWELVE_MONTH", "resources": []any{
 			map[string]any{"type": "MEMORY", "amount": "6291456"},
 			map[string]any{"type": "VCPU", "amount": "480"},
 		},

providers/gcp/services/computeengine/client.go

@@ -482,7 +482,7 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio
 // Structural payload errors are still returned and abort the batch.
 func (c *Client) convertOrSkipBadAmount(ctx context.Context, rec *recommenderpb.Recommendation, params common.RecommendationParams) (*common.Recommendation, error) {
 	converted, err := c.convertGCPRecommendation(ctx, rec, params)
-	if errors.Is(err, errBadAmount) {
+	if errors.Is(err, errBadAmount) || errors.Is(err, errBadPlan) {
 		log.Printf("computeengine: skipping recommendation %q: %v", rec.GetName(), err)
 		return nil, nil
 	}
@@ -1082,8 +1082,11 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService,
 // 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 unrecognized so the caller skips an
-// unroutable recommendation rather than queuing a purchase with an invalid plan.
+// The term comes from the root Commitment plan when present (issue #291); an
+// explicit params.Term that disagrees, or a root without a plan and without
+// params.Term, returns errBadPlan so the caller skips the recommendation.
+// Returns nil when the term is unrecognized so the caller skips an unroutable
+// recommendation rather than queuing a purchase with an invalid plan.
 func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (*common.Recommendation, error) {
 	// GCP CUDs are billed monthly with no upfront option; force "monthly"
 	// unconditionally and log any non-monthly input so scheduler
@@ -1095,11 +1098,15 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend
 	}
 	paymentOption := "monthly"
 
-	// H-3: propagate params.Term (default "1yr") and validate it so an
-	// unrecognized term fails loud here rather than reaching buildInsertRequest.
-	term := params.Term
-	if term == "" {
-		term = "1yr"
+	// The root Commitment plan is authoritative (issue #291); resolveRecTerm
+	// reconciles it with params.Term and validates the result so an unrecognized
+	// term never reaches buildInsertRequest.
+	term, err := resolveRecTerm(gcpRec, params)
+	if err != nil {
+		return nil, err
+	}
+	if params.ExperimentalGCPPlanTerm {
+		log.Printf("computeengine: EXPERIMENTAL plan-term path: term %q for %q; root plan payload shape is unverified", term, gcpRec.GetName())
 	}
 	if _, err := termPlan(term); err != nil {
 		log.Printf("computeengine: skipping recommendation with unrecognized term %q: %v", term, err)

providers/gcp/services/computeengine/commitment_type.go

@@ -1,6 +1,7 @@
 package computeengine
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -157,3 +158,107 @@ func populateRecommendationResources(gcpRec *recommenderpb.Recommendation, rec *
 	rec.Count = count
 	return extractMemoryMBFromRecommendation(gcpRec, rec)
 }
+
+// errBadPlan (EXPERIMENTAL plan-term path, see resolveRecTerm) marks a recommendation whose commitment plan is malformed,
+// missing where required, or in conflict with the caller's term. The
+// recommendation is skipped rather than purchased with a guessed term.
+var errBadPlan = errors.New("bad recommendation plan")
+
+// rootCommitmentTerm (EXPERIMENTAL, unverified payload shape) reads the root Commitment's plan and returns the
+// canonical term ("1yr" or "3yr"). found is false when the root has no plan
+// key. A present but malformed plan returns an error wrapping errBadPlan.
+func rootCommitmentTerm(content *recommenderpb.RecommendationContent) (term string, found bool, err error) {
+	root, err := selectRootCommitment(content)
+	if err != nil {
+		return "", false, err
+	}
+	return rootPlanTerm(root)
+}
+
+func rootPlanTerm(root *recommenderpb.Operation) (term string, found bool, err error) {
+	if root == nil {
+		return "", false, nil
+	}
+	object := root.GetValue().GetStructValue()
+	if object == nil {
+		return "", false, fmt.Errorf("root Commitment value must be an object")
+	}
+	value, present := object.Fields["plan"]
+	if !present {
+		return "", false, nil
+	}
+	name, ok := value.GetKind().(*structpb.Value_StringValue)
+	if !ok {
+		return "", false, fmt.Errorf("%w: root Commitment plan must be a string", errBadPlan)
+	}
+	switch name.StringValue {
+	case computepb.Commitment_TWELVE_MONTH.String():
+		return "1yr", true, nil
+	case computepb.Commitment_THIRTY_SIX_MONTH.String():
+		return "3yr", true, nil
+	default:
+		return "", false, fmt.Errorf("%w: unsupported root Commitment plan %q", errBadPlan, name.StringValue)
+	}
+}
+
+// canonicalTerm maps any accepted term spelling to "1yr" or "3yr". An
+// unrecognized term is returned unchanged so the caller skips it, as before.
+func canonicalTerm(term string) string {
+	plan, err := termPlan(term)
+	if err != nil {
+		return term
+	}
+	if plan == computepb.Commitment_THIRTY_SIX_MONTH.String() {
+		return "3yr"
+	}
+	return "1yr"
+}
+
+// resolveRecTerm picks the canonical term ("1yr"/"3yr") for a recommendation.
+// A root Commitment plan is authoritative: an explicit params.Term must agree
+// with it. A root Commitment without a plan needs an explicit params.Term. A
+// payload with no root Commitment keeps the params.Term / "1yr" fallback
+// (issue #291 follow-up).
+//
+// EXPERIMENTAL: the plan path runs only when params.ExperimentalGCPPlanTerm is
+// set. The root-plan payload shape is unverified against a live Recommender.
+// When unset, the pre-#291 behavior applies: params.Term, default "1yr".
+func resolveRecTerm(gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (string, error) {
+	if !params.ExperimentalGCPPlanTerm {
+		if params.Term == "" {
+			return "1yr", nil
+		}
+		return params.Term, nil
+	}
+	root, err := selectRootCommitment(gcpRec.GetContent())
+	if err != nil {
+		return "", err
+	}
+	planTerm, found, err := rootPlanTerm(root)
+	if err != nil {
+		return "", err
+	}
+	if !found {
+		return termWithoutPlan(root != nil, params.Term)
+	}
+	if params.Term == "" {
+		return planTerm, nil
+	}
+	if want := canonicalTerm(params.Term); want != "1yr" && want != "3yr" {
+		return want, nil
+	} else if want != planTerm {
+		return "", fmt.Errorf("%w: params term %q conflicts with recommendation plan term %s", errBadPlan, params.Term, planTerm)
+	}
+	return planTerm, nil
+}
+
+// termWithoutPlan resolves the term when the payload carries no plan.
+func termWithoutPlan(hasRoot bool, paramTerm string) (string, error) {
+	if paramTerm != "" {
+		return canonicalTerm(paramTerm), nil
+	}
+	if hasRoot {
+		return "", fmt.Errorf("%w: root Commitment has no plan and no explicit term was given", errBadPlan)
+	}
+	return "1yr", nil
+}

providers/gcp/services/computeengine/dedupe_test.go

@@ -18,12 +18,22 @@ import (
 
 // Schema fixture, not a captured service response.
 func rootCUDRecommendation(kind string) *recommenderpb.Recommendation {
-	value, _ := structpb.NewStruct(map[string]any{
+	return rootCUDRecommendationWithPlan(kind, "TWELVE_MONTH")
+}
+
+// rootCUDRecommendationWithPlan builds the fixture with the given root plan;
+// an empty plan omits the key.
+func rootCUDRecommendationWithPlan(kind, plan string) *recommenderpb.Recommendation {
+	fields := map[string]any{
 		"type": kind, "resources": []any{
 			map[string]any{"type": "MEMORY", "amount": "6291456"},
 			map[string]any{"type": "VCPU", "amount": "480"},
 		},
-	})
+	}
+	if plan != "" {
+		fields["plan"] = plan
+	}
+	value, _ := structpb.NewStruct(fields)
 	return &recommenderpb.Recommendation{
 		StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE},
 		Content: &recommenderpb.RecommendationContent{OperationGroups: []*recommenderpb.OperationGroup{{Operations: []*recommenderpb.Operation{{

providers/gcp/services/computeengine/plan_test.go

@@ -0,0 +1,145 @@
+package computeengine
+
+import (
+	"context"
+	"testing"
+
+	"cloud.google.com/go/recommender/apiv1/recommenderpb"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/types/known/structpb"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+)
+
+func planClient(t *testing.T, recs ...*recommenderpb.Recommendation) (*Client, *MockCommitmentsService) {
+	t.Helper()
+	client, err := NewClient(context.Background(), "test-project", "us-central1")
+	require.NoError(t, err)
+	client.SetBillingService(&MockBillingService{err: assert.AnError})
+	client.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: recs}})
+	service := &MockCommitmentsService{operation: &MockOperation{}}
+	client.SetCommitmentsService(service)
+	return client, service
+}
+
+// Issue #291: a THIRTY_SIX_MONTH recommendation with no caller term must be
+// bought as THIRTY_SIX_MONTH. Pre-fix it was labeled 1yr and bought TWELVE_MONTH.
+func TestRecommendationPlanDrivesPurchaseTerm(t *testing.T) {
+	cases := []struct {
+		name, plan, paramTerm, wantTerm, wantPlan string
+	}{
+		{"36mo plan, empty term", "THIRTY_SIX_MONTH", "", "3yr", "THIRTY_SIX_MONTH"},
+		{"12mo plan, empty term", "TWELVE_MONTH", "", "1yr", "TWELVE_MONTH"},
+		{"36mo plan, 3yr", "THIRTY_SIX_MONTH", "3yr", "3yr", "THIRTY_SIX_MONTH"},
+		{"36mo plan, 36mo spelling", "THIRTY_SIX_MONTH", "36mo", "3yr", "THIRTY_SIX_MONTH"},
+		{"36mo plan, bare 3", "THIRTY_SIX_MONTH", " 3 ", "3yr", "THIRTY_SIX_MONTH"},
+		{"no plan, explicit 3yr", "", "3yr", "3yr", "THIRTY_SIX_MONTH"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			client, service := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", tc.plan))
+			recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.paramTerm, ExperimentalGCPPlanTerm: true})
+			require.NoError(t, err)
+			require.Len(t, recs, 1)
+			assert.Equal(t, tc.wantTerm, recs[0].Term)
+			_, err = client.PurchaseCommitment(context.Background(), recs[0], common.PurchaseOptions{})
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantPlan, service.lastInsertReq.CommitmentResource.GetPlan())
+		})
+	}
+}
+
+func TestRecommendationPlanSkips(t *testing.T) {
+	malformed := func(v *structpb.Value) *recommenderpb.Recommendation {
+		rec := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "")
+		rec.Content.OperationGroups[0].Operations[0].GetValue().GetStructValue().Fields["plan"] = v
+		return rec
+	}
+	cases := map[string]struct {
+		rec  *recommenderpb.Recommendation
+		term string
+	}{
+		"conflict 12mo vs 3yr":  {rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "TWELVE_MONTH"), "3yr"},
+		"conflict 36mo vs 1yr":  {rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "THIRTY_SIX_MONTH"), "1yr"},
+		"missing plan, no term": {rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", ""), ""},
+		"UNDEFINED_PLAN":        {malformed(structpb.NewStringValue("UNDEFINED_PLAN")), ""},
+		"unknown plan":          {malformed(structpb.NewStringValue("FIVE_YEAR")), ""},
+		"empty plan":            {malformed(structpb.NewStringValue("")), ""},
+		"numeric plan":          {malformed(structpb.NewNumberValue(36)), ""},
+		"null plan":             {malformed(structpb.NewNullValue()), ""},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			client, service := planClient(t, tc.rec)
+			recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.term, ExperimentalGCPPlanTerm: true})
+			require.NoError(t, err)
+			assert.Empty(t, recs)
+			assert.Empty(t, service.insertReqs)
+		})
+	}
+}
+
+// A conflicting recommendation is skipped without dropping its good neighbor.
+func TestRecommendationPlanConflictKeepsGoodRecommendation(t *testing.T) {
+	bad := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "TWELVE_MONTH")
+	bad.Name = "bad"
+	good := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "THIRTY_SIX_MONTH")
+	good.Name = "good"
+	client, service := planClient(t, bad, good)
+	recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr", ExperimentalGCPPlanTerm: true})
+	require.NoError(t, err)
+	require.Len(t, recs, 1)
+	assert.Equal(t, "3yr", recs[0].Term)
+	assert.Empty(t, service.insertReqs, "listing must not insert")
+}
+
+// Mutation probe: the plan-to-term mapping must not be swappable.
+func TestRootCommitmentTermMapping(t *testing.T) {
+	for plan, want := range map[string]string{"TWELVE_MONTH": "1yr", "THIRTY_SIX_MONTH": "3yr"} {
+		got, found, err := rootCommitmentTerm(rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", plan).GetContent())
+		require.NoError(t, err)
+		assert.True(t, found)
+		assert.Equal(t, want, got)
+	}
+}
+
+// An explicit non-canonical Term must be canonicalized so pricing and grouping
+// see "3yr", not "36mo" (which GetOfferingDetails would price as one year).
+func TestExplicitTermIsCanonicalized(t *testing.T) {
+	for _, raw := range []string{"36mo", " 3 ", "3yr"} {
+		for _, plan := range []string{"", "THIRTY_SIX_MONTH"} {
+			client, _ := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", plan))
+			recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: raw, ExperimentalGCPPlanTerm: true})
+			require.NoError(t, err)
+			require.Len(t, recs, 1)
+			assert.Equal(t, "3yr", recs[0].Term, "raw %q plan %q", raw, plan)
+			assert.Equal(t, 3, termYearsFromTerm(recs[0].Term))
+		}
+	}
+	assert.Equal(t, "3yr", canonicalTerm("36mo"))
+}
+
+// Default (flag off) keeps the pre-#291 behavior: Term or "1yr", plan ignored,
+// nothing skipped.
+func TestPlanTermOffByDefault(t *testing.T) {
+	cases := []struct {
+		name, plan, paramTerm, wantTerm, wantPlan string
+	}{
+		{"36mo plan, empty term", "THIRTY_SIX_MONTH", "", "1yr", "TWELVE_MONTH"},
+		{"12mo plan, 3yr term (conflict kept)", "TWELVE_MONTH", "3yr", "3yr", "THIRTY_SIX_MONTH"},
+		{"missing plan, no term", "", "", "1yr", "TWELVE_MONTH"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			client, service := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", tc.plan))
+			recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.paramTerm})
+			require.NoError(t, err)
+			require.Len(t, recs, 1)
+			assert.Equal(t, tc.wantTerm, recs[0].Term)
+			_, err = client.PurchaseCommitment(context.Background(), recs[0], common.PurchaseOptions{})
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantPlan, service.lastInsertReq.CommitmentResource.GetPlan())
+		})
+	}
+}