fix(cmd): refuse an EC2 Instance Savings Plan without a family in the dry run too (#2261)

* fix(cmd): refuse an EC2 Instance Savings Plan without a family in the dry run too

validatePurchasePreconditions required only a region for an EC2 Instance
Savings Plan. With no instance family and no offering ID, the real run
refuses in the offering lookup (ambiguous or missing offerings) while the
dry run reported success. Require the family unless an offering ID is set.
The completeness fixture now carries a family, as a real Cost Explorer
EC2 Instance recommendation does.

Closes #2164

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(cmd): reject whitespace-only EC2 Instance family in SP precondition

validateEC2InstanceSPOffering checked InstanceFamily == "", so a family of spaces passed the dry run when OfferingID was empty; the real lookup then sent it as the instanceFamily filter. Trim before the empty check and cover it in TestValidatePurchasePreconditions.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: cristim

CHANGELOG.md

@@ -16,6 +16,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 - Recommendations with no account ID or name are now reported in the drop summary
   when an account filter (`--include-accounts` or `--exclude-accounts`) removes
   them (#1613).
+- A dry run now refuses an EC2 Instance Savings Plan that has no instance family
+  and no offering ID, the same way the real run does, instead of reporting
+  success for a purchase the offering lookup would reject (#2164).
 - A purchase result with `Success=true` and a non-nil `Error` is now treated as
   a failure in the Ctrl-C summary, the audit-stop message, the per-service
   success/failed counts and the non-zero exit. The CSV `Success` column keeps the

cmd/recommendation_completeness_proxy_test.go

@@ -305,7 +305,7 @@ func (p *completenessProxy) spRecommendation(body []byte) (int, string, string)
 	const invalid = `{"HourlyCommitmentToPurchase":"not-a-number","EstimatedMonthlySavingsAmount":"10","UpfrontCost":"3"}`
 	details := valid
 	if request.SavingsPlansType == "EC2_INSTANCE_SP" {
-		details = strings.TrimSuffix(valid, "}") + `,"SavingsPlansDetails":{"Region":"us-east-1"}}`
+		details = strings.TrimSuffix(valid, "}") + `,"SavingsPlansDetails":{"InstanceFamily":"m5","Region":"us-east-1"}}`
 	}
 	switch p.details {
 	case "mixed":

cmd/savings_plan_purchase.go

@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
@@ -151,9 +152,22 @@ func validateSavingsPlanPreconditions(rec common.Recommendation) error {
 	if fmt.Sprintf(spCommitmentFormat, d.HourlyCommitment) == "0.00" {
 		return fmt.Errorf("%s recommendation's hourly commitment %v would be sent as 0.00; it must be at least 0.005", rec.Service, d.HourlyCommitment)
 	}
-	if rec.Service == common.ServiceSavingsPlansEC2Instance && d.Region == "" {
+	if rec.Service == common.ServiceSavingsPlansEC2Instance {
+		return validateEC2InstanceSPOffering(d)
+	}
+	return nil
+}
+
+// validateEC2InstanceSPOffering mirrors the library's offering lookup: an
+// explicit offering ID skips it, otherwise it needs the region and family to
+// pick exactly one offering.
+func validateEC2InstanceSPOffering(d *common.SavingsPlanDetails) error {
+	if d.Region == "" {
 		return fmt.Errorf("EC2 Instance Savings Plan needs a region to pick the offering")
 	}
+	if d.OfferingID == "" && strings.TrimSpace(d.InstanceFamily) == "" {
+		return fmt.Errorf("EC2 Instance Savings Plan needs an instance family or an offering ID to pick the offering")
+	}
 	return nil
 }
 

cmd/savings_plan_purchase_test.go

@@ -258,8 +258,14 @@ func TestValidatePurchasePreconditions(t *testing.T) {
 	ok := map[string]common.Recommendation{
 		"compute sp":      parserShapedSP(),
 		"ec2 instance sp": validEC2InstanceSP(),
-		"ec2":             validEC2Rec(),
-		"rds":             {Service: common.ServiceRDS, Region: "us-east-1"},
+		"ec2 instance sp offering id no family": mut(validEC2InstanceSP(), func(r *common.Recommendation) {
+			r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1", OfferingID: "o-1"}
+		}),
+		"compute sp no family": mut(parserShapedSP(), func(r *common.Recommendation) {
+			r.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1}
+		}),
+		"ec2": validEC2Rec(),
+		"rds": {Service: common.ServiceRDS, Region: "us-east-1"},
 	}
 	for name, rec := range ok {
 		assert.NoError(t, validatePurchasePreconditions(rec, rec.Region, defaultTestAccount), name)
@@ -274,6 +280,12 @@ func TestValidatePurchasePreconditions(t *testing.T) {
 		"ec2 instance sp no region": mut(validEC2InstanceSP(), func(r *common.Recommendation) {
 			r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, InstanceFamily: "m5"}
 		}),
+		"ec2 instance sp no family no offering": mut(validEC2InstanceSP(), func(r *common.Recommendation) {
+			r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1"}
+		}),
+		"ec2 instance sp whitespace family no offering": mut(validEC2InstanceSP(), func(r *common.Recommendation) {
+			r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1", InstanceFamily: "   "}
+		}),
 		"sp all":        mut(parserShapedSP(), func(r *common.Recommendation) { r.Service = common.ServiceSavingsPlansAll }),
 		"ec2 nil":       mut(validEC2Rec(), func(r *common.Recommendation) { r.Details = nil }),
 		"ec2 tenancy":   mut(validEC2Rec(), func(r *common.Recommendation) { r.Details.(*common.ComputeDetails).Tenancy = "" }),