fix(reconcile): honor product behavior, reject a credit amount without the credits behavior, and keep feature names verbatim on apply

billing/product/service.go

@@ -64,10 +64,21 @@ func (s *Service) Create(ctx context.Context, product Product) (Product, error)
 		product.ID = uuid.New().String()
 		product.ProviderID = product.ID
 	}
+	// Capture whether the caller stated a behavior before SetDefaults fills the
+	// "basic" default, so an explicit behavior is honored and only an omitted one
+	// on a credit product falls back to "credits".
+	statedBehavior := product.Behavior
 	defaults.SetDefaults(&product)
-	if product.Config.CreditAmount > 0 {
+	if statedBehavior == "" && product.Config.CreditAmount > 0 {
 		product.Behavior = CreditBehavior
 	}
+	// Credits are only granted for the credits behavior, so a positive credit
+	// amount on any other behavior would store credits that no path ever awards.
+	// Reject the contradiction instead of silently keeping a product whose credits
+	// are dead.
+	if product.Config.CreditAmount > 0 && product.Behavior != CreditBehavior {
+		return Product{}, fmt.Errorf("%w: credit_amount %d needs behavior %q, got %q", ErrInvalidDetail, product.Config.CreditAmount, CreditBehavior, product.Behavior)
+	}
 	product.Name = strings.ToLower(product.Name)
 
 	_, err := s.stripeClient.Products.New(&stripe.ProductParams{

billing/product/service_test.go

@@ -85,6 +85,129 @@ func TestService_Create(t *testing.T) {
 				return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo)
 			},
 		},
+		{
+			name: "honors an explicit credits behavior on a credit product",
+			args: args{
+				product: product.Product{
+					ID:          "creditprod",
+					Name:        "creditprod",
+					Description: "credit product",
+					Behavior:    product.CreditBehavior,
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				},
+			},
+			want: product.Product{
+				ID:          "creditprod",
+				Name:        "creditprod",
+				Description: "credit product",
+				Behavior:    product.CreditBehavior,
+				Config:      product.BehaviorConfig{CreditAmount: 5},
+			},
+			wantErr: false,
+			setup: func() *product.Service {
+				stripeClient, mockStripeBackend, mockProductRepo, mockPriceRepo, mockFeatureRepo := mockService(t)
+				mockProductRepo.EXPECT().Create(ctx, product.Product{
+					ID:          "creditprod",
+					Name:        "creditprod",
+					Description: "credit product",
+					Behavior:    product.CreditBehavior,
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				}).Return(product.Product{
+					ID:          "creditprod",
+					Name:        "creditprod",
+					Description: "credit product",
+					Behavior:    product.CreditBehavior,
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				}, nil)
+				mockStripeBackend.EXPECT().Call("POST", "/v1/products", "key_123", &stripe.ProductParams{
+					Params: stripe.Params{
+						Context: ctx,
+					},
+					ID:          new(""),
+					Name:        new(""),
+					Description: new("credit product"),
+					Metadata: map[string]string{
+						"behavior":      "credits",
+						"credit_amount": "5",
+						"managed_by":    "frontier",
+						"name":          "creditprod",
+						"product_id":    "creditprod",
+					},
+				}, &stripe.Product{}).Return(nil)
+				return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo)
+			},
+		},
+		{
+			name: "rejects a credit amount paired with a non-credit behavior",
+			args: args{
+				product: product.Product{
+					ID:          "creditprod",
+					Name:        "creditprod",
+					Description: "credit product",
+					Behavior:    product.PerSeatBehavior,
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				},
+			},
+			want:    product.Product{},
+			wantErr: true,
+			setup: func() *product.Service {
+				// The contradiction is rejected before any provider or repo call, so
+				// no expectations are set.
+				stripeClient, _, mockProductRepo, mockPriceRepo, mockFeatureRepo := mockService(t)
+				return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo)
+			},
+		},
+		{
+			name: "defaults an omitted behavior to credits on a credit product",
+			args: args{
+				product: product.Product{
+					ID:          "creditprod2",
+					Name:        "creditprod2",
+					Description: "credit product",
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				},
+			},
+			want: product.Product{
+				ID:          "creditprod2",
+				Name:        "creditprod2",
+				Description: "credit product",
+				Behavior:    product.CreditBehavior,
+				Config:      product.BehaviorConfig{CreditAmount: 5},
+			},
+			wantErr: false,
+			setup: func() *product.Service {
+				stripeClient, mockStripeBackend, mockProductRepo, mockPriceRepo, mockFeatureRepo := mockService(t)
+				mockProductRepo.EXPECT().Create(ctx, product.Product{
+					ID:          "creditprod2",
+					Name:        "creditprod2",
+					Description: "credit product",
+					Behavior:    product.CreditBehavior,
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				}).Return(product.Product{
+					ID:          "creditprod2",
+					Name:        "creditprod2",
+					Description: "credit product",
+					Behavior:    product.CreditBehavior,
+					Config:      product.BehaviorConfig{CreditAmount: 5},
+				}, nil)
+				mockStripeBackend.EXPECT().Call("POST", "/v1/products", "key_123", &stripe.ProductParams{
+					Params: stripe.Params{
+						Context: ctx,
+					},
+					ID:          new(""),
+					Name:        new(""),
+					Description: new("credit product"),
+					Metadata: map[string]string{
+						"behavior":      "credits",
+						"credit_amount": "5",
+						"managed_by":    "frontier",
+						"name":          "creditprod2",
+						"product_id":    "creditprod2",
+					},
+				}, &stripe.Product{}).Return(nil)
+				return product.NewService(stripeClient, mockProductRepo, mockPriceRepo, mockFeatureRepo)
+			},
+		},
 		{
 			name: "should create product in repo and billing provider with price and features",
 			args: args{

internal/reconcile/billingproduct.go

@@ -132,6 +132,16 @@ func validateBillingProductSpec(s BillingProductSpec) error {
 		return fmt.Errorf("product %q cannot be deleted: there is no product delete or deactivate API; remove the entry and archive the product by hand", s.Name)
 	}
 
+	// Credits are only granted for the credits behavior. A positive credit amount
+	// with any other stated behavior stores credits the server never awards, so
+	// reject it at plan time rather than apply a product whose credits are dead. An
+	// omitted behavior is fine: the server coerces it to credits when there is a
+	// credit amount.
+	behavior := strings.ToLower(strings.TrimSpace(s.Behavior))
+	if s.Config.CreditAmount > 0 && behavior != "" && behavior != string(product.CreditBehavior) {
+		return fmt.Errorf("product %q sets credit_amount %d with behavior %q: credits are only granted for the %q behavior", s.Name, s.Config.CreditAmount, behavior, product.CreditBehavior)
+	}
+
 	seenPrice := map[string]struct{}{}
 	for _, p := range s.Prices {
 		priceName := strings.ToLower(strings.TrimSpace(p.Name))
@@ -259,14 +269,14 @@ func billingProductChanges(s BillingProductSpec, cur currentBillingProduct) ([]s
 	if s.Description != cur.Description {
 		changes = append(changes, "description")
 	}
-	// behavior is create-only: the server sets it at create (rewriting it to
-	// "credits" when credit_amount > 0) and never changes it on update. A file
-	// that asks to change it to something the product is not cannot apply, so fail
-	// the plan rather than skip it in silence. A credit product's behavior is
-	// always "credits", so fold that in to avoid a false failure when the file
-	// still names a behavior alongside a credit amount.
+	// behavior is create-only: the server sets it at create and never changes it
+	// on update. The file's behavior is honored as written, so a file that names a
+	// behavior the product was not created with fails the plan rather than being
+	// silently overridden. Only an omitted behavior on a credit product falls back
+	// to "credits", matching the server's create-time default, so an omitted
+	// behavior does not read as a change against a credit product.
 	expectedBehavior := s.Behavior
-	if s.Config.CreditAmount > 0 {
+	if expectedBehavior == "" && s.Config.CreditAmount > 0 {
 		expectedBehavior = "credits"
 	}
 	if expectedBehavior != "" && expectedBehavior != cur.Behavior {
@@ -298,14 +308,24 @@ func billingConfigChanged(desired, cur BillingProductConfig) bool {
 		desired.MaxQuantity != cur.MaxQuantity
 }
 
+// normalizeFeatureName is the canonical form of a feature name, used only to
+// compare the desired and current feature sets so a case or whitespace difference
+// does not plan a spurious update. The apply does not use it: the server matches
+// and stores feature names verbatim, so sending a normalized name would miss a
+// differently cased stored feature and fork a duplicate. Making the two fully
+// agree needs the server to canonicalize feature names, which is a separate change.
+func normalizeFeatureName(name string) string {
+	return strings.ToLower(strings.TrimSpace(name))
+}
+
 func billingFeatureSetsEqual(desired []BillingFeatureRef, current []string) bool {
 	d := make([]string, 0, len(desired))
 	for _, f := range desired {
-		d = append(d, strings.ToLower(strings.TrimSpace(f.Name)))
+		d = append(d, normalizeFeatureName(f.Name))
 	}
 	c := make([]string, 0, len(current))
 	for _, name := range current {
-		c = append(c, strings.ToLower(strings.TrimSpace(name)))
+		c = append(c, normalizeFeatureName(name))
 	}
 	return stringSetsEqual(uniqueSorted(d), uniqueSorted(c))
 }

internal/reconcile/billingproduct_reconciler.go

@@ -288,6 +288,11 @@ func billingProductBody(s BillingProductSpec) *frontierv1beta1.ProductRequestBod
 	}
 	features := make([]*frontierv1beta1.Feature, 0, len(s.Features))
 	for _, f := range s.Features {
+		// Send the name verbatim. The server matches and stores feature names
+		// case-sensitively, so a normalized name here would miss a differently
+		// cased stored feature and fork a duplicate. The diff still compares
+		// normalized names so a case-only difference does not plan a spurious
+		// update.
 		features = append(features, &frontierv1beta1.Feature{Name: f.Name})
 	}
 	return &frontierv1beta1.ProductRequestBody{

internal/reconcile/billingproduct_test.go

@@ -53,6 +53,12 @@ func TestValidateBillingProductSpec(t *testing.T) {
 		{"duplicate feature", func(s *BillingProductSpec) {
 			s.Features = []BillingFeatureRef{{Name: "f"}, {Name: "f"}}
 		}, "more than once"},
+		{"credit amount with a non-credit behavior rejected", func(s *BillingProductSpec) {
+			s.Behavior = "per_seat" // newBillingProduct already sets CreditAmount 1
+		}, "credits are only granted"},
+		{"credit amount with an omitted behavior is allowed", func(s *BillingProductSpec) {
+			s.Behavior = "" // the server coerces an omitted behavior to credits
+		}, ""},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
@@ -149,14 +155,22 @@ func TestDiffBillingProducts(t *testing.T) {
 		}
 	})
 
-	t.Run("does not fail on a behavior a credit amount forces", func(t *testing.T) {
+	t.Run("defaults an omitted behavior to credits for a credit product", func(t *testing.T) {
 		s := newBillingProduct()
-		s.Behavior = "basic" // the file names basic, but credit_amount > 0 forces "credits"
+		s.Behavior = "" // omitted; credit_amount > 0 defaults it to the server's "credits"
 		ops, err := diffBillingProducts([]BillingProductSpec{s}, []currentBillingProduct{curToken()})
 		assert.NoError(t, err)
 		assert.Empty(t, ops)
 	})
 
+	t.Run("fails when the file names a behavior the credit product was not created with", func(t *testing.T) {
+		s := newBillingProduct()
+		s.Behavior = "basic"      // the file states basic, but the product was created as credits
+		s.Config.CreditAmount = 0 // basic carries no credit amount, so the spec is valid on its own
+		_, err := diffBillingProducts([]BillingProductSpec{s}, []currentBillingProduct{curToken()})
+		assert.ErrorContains(t, err, "behavior cannot change")
+	})
+
 	t.Run("fails the plan on a behavior change the server cannot apply", func(t *testing.T) {
 		cur := curToken()
 		cur.Behavior = "basic"
@@ -254,3 +268,18 @@ func TestDiffBillingProducts(t *testing.T) {
 		assert.Empty(t, ops)
 	})
 }
+
+func TestBillingProductBody_SendsFeatureNamesVerbatim(t *testing.T) {
+	// The server matches and stores feature names case-sensitively, so the apply
+	// body must send them exactly as written. Normalizing here would miss a
+	// differently cased stored feature and fork a duplicate. The diff still
+	// compares normalized names, so a case-only difference does not plan a change.
+	s := newBillingProduct()
+	s.Features = []BillingFeatureRef{{Name: "  Foo  "}, {Name: "BAR"}}
+	body := billingProductBody(s)
+	var got []string
+	for _, f := range body.GetFeatures() {
+		got = append(got, f.GetName())
+	}
+	assert.Equal(t, []string{"  Foo  ", "BAR"}, got)
+}