From 1acdd72d231b989d9274509b4e6f990605cc239d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 12:56:23 +0100 Subject: [PATCH 1/4] feat(planstep): resolve a bound group from its selected recommendations Slice A, steps A2 and A3 of #1746. - DerivePlanServices drops a bound entry's "#" suffix, so the arming checks and grant constraints see "ec2" for "aws/ec2#g1". - resolveService keeps only a bound group's own identities from the strict read, before the plan match, suppression and account checks that still apply. A matching recommendation that was not selected is never bought and never enters the frozen baseline. - Bound identities the strict read didn't show are returned as Result.MissingBound and logged at warn with plan and service. Some missing: the rest are sized. All missing: the empty result takes the existing ErrNoMatch grace path. - On a bound plan, planServiceOf matches a baselined identity to the enabled group that bound it, so targetMet judges each group against its own coverage, including groups that differ only in payment. A key no group bound is not judged. Unmarked plans keep today's first-entry match (a regression test covers the payment-split case). pre-commit: cfn-lint skipped (it can't install here); no CloudFormation files are touched. Part of #1746 Co-Authored-By: Claude Opus 5.5 --- internal/config/purchase_plan_providers.go | 2 + .../purchase_plan_services_bound_test.go | 22 ++++ internal/planstep/bound_resolve_test.go | 102 +++++++++++++++++ internal/planstep/resolve.go | 103 ++++++++++++++++-- 4 files changed, 219 insertions(+), 10 deletions(-) create mode 100644 internal/config/purchase_plan_services_bound_test.go create mode 100644 internal/planstep/bound_resolve_test.go diff --git a/internal/config/purchase_plan_providers.go b/internal/config/purchase_plan_providers.go index bd4af2829..35882a919 100644 --- a/internal/config/purchase_plan_providers.go +++ b/internal/config/purchase_plan_providers.go @@ -53,12 +53,14 @@ func serviceEntryProvider(key, fieldProvider string) string { // DerivePlanServices extracts the distinct service names a plan targets from // the "provider/service" and "provider:service" keys of plan.Services, sorted. +// A bound entry's "#" suffix is not part of its service name. func DerivePlanServices(plan *PurchasePlan) []string { if plan == nil { return nil } seen := make(map[string]struct{}, len(plan.Services)) for k := range plan.Services { + k, _, _ = strings.Cut(k, BoundKeySeparator) if i := strings.IndexAny(k, "/:"); i > 0 && i < len(k)-1 { seen[k[i+1:]] = struct{}{} } diff --git a/internal/config/purchase_plan_services_bound_test.go b/internal/config/purchase_plan_services_bound_test.go new file mode 100644 index 000000000..fafe5f088 --- /dev/null +++ b/internal/config/purchase_plan_services_bound_test.go @@ -0,0 +1,22 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +func TestDerivePlanServices_CutsBoundGroupSuffix(t *testing.T) { + p := &PurchasePlan{Services: map[string]ServiceConfig{ + "aws/ec2#g1": {}, "aws/ec2#g2": {}, "aws/rds#g3": {}, + }} + assert.Equal(t, []string{"ec2", "rds"}, DerivePlanServices(p)) + providers, err := DerivePlanProviders(p) + assert.NoError(t, err) + assert.Equal(t, []string{"aws"}, providers) + // (guard) legacy keys are unchanged. + legacy := &PurchasePlan{Services: map[string]ServiceConfig{"aws/ec2": {}, "aws:savings-plans-compute": {}}} + assert.Equal(t, []string{"ec2", "savings-plans-compute"}, DerivePlanServices(legacy)) +} diff --git a/internal/planstep/bound_resolve_test.go b/internal/planstep/bound_resolve_test.go new file mode 100644 index 000000000..c6dbcdb03 --- /dev/null +++ b/internal/planstep/bound_resolve_test.go @@ -0,0 +1,102 @@ +package planstep + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are in-memory fakes, not a real database or cloud (issue #1746). + +func azureRecIn(region, payment string) config.RecommendationRecord { + r := azureRec(10, 100) + r.Region, r.Payment = region, payment + r.ID = "azure|acct-T|compute|" + region + "|Standard_D2s_v3||1|" + payment + return r +} + +func boundAzurePlan(entries map[string]config.ServiceConfig) *config.PurchasePlan { + return &config.PurchasePlan{ID: "plan-b", Services: entries, RampSchedule: config.RampSchedule{ + Type: "custom", PercentPerStep: 50, TotalSteps: 2, AllocationMode: config.RampAllocationBoundGroups, + }} +} + +func azureGroup(payment string, coverage float64, ids ...string) config.ServiceConfig { + return config.ServiceConfig{Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: payment, + Coverage: coverage, MinCount: 1, BoundIdentities: ids} +} + +func TestResolve_BoundGroupBuysOnlyItsSelection(t *testing.T) { + west, north := azureRecIn("westeurope", "monthly"), azureRecIn("northeurope", "monthly") + rows := []config.RecommendationRecord{west, north} + plan := boundAzurePlan(map[string]config.ServiceConfig{"azure/compute#g1": azureGroup("monthly", 100, cumKey(t, west))}) + res, err := Resolve(t.Context(), fakeSource{shown: rows}, fakeStore{rows: rows}, plan, accts, 1, nil, nil) + require.NoError(t, err) + require.Len(t, res.Recs, 1, "the matching but unselected northeurope rec is not bought") + assert.Equal(t, "westeurope", res.Recs[0].Region) + assert.Empty(t, res.MissingBound) + assert.Contains(t, res.Baseline, cumKey(t, west)) + assert.NotContains(t, res.Baseline, cumKey(t, north), "an unselected rec never enters the baseline") +} + +func TestResolve_BoundIdentitiesMissingFromTheStrictRead(t *testing.T) { + west, north := azureRecIn("westeurope", "monthly"), azureRecIn("northeurope", "monthly") + gone := azureRecIn("uksouth", "monthly") + // Some missing: the present one is sized, the missing one is reported. + plan := boundAzurePlan(map[string]config.ServiceConfig{"azure/compute#g1": azureGroup("monthly", 100, cumKey(t, west), cumKey(t, gone))}) + rows := []config.RecommendationRecord{west, north} + res, err := Resolve(t.Context(), fakeSource{shown: rows}, fakeStore{rows: rows}, plan, accts, 1, nil, nil) + require.NoError(t, err) + require.Len(t, res.Recs, 1) + assert.Equal(t, []string{cumKey(t, gone)}, res.MissingBound) + // All missing: nothing to buy takes the ErrNoMatch grace path. + plan = boundAzurePlan(map[string]config.ServiceConfig{"azure/compute#g1": azureGroup("monthly", 100, cumKey(t, gone))}) + res, err = Resolve(t.Context(), fakeSource{shown: rows}, fakeStore{rows: rows}, plan, accts, 1, nil, nil) + require.ErrorIs(t, err, ErrNoMatch) + assert.Equal(t, []string{cumKey(t, gone)}, res.MissingBound) +} + +// targetMet judges a baselined identity only against the group that bound it. +func TestResolve_TargetMetJudgesByBoundGroup(t *testing.T) { + monthly, upfront := azureRecIn("westeurope", "monthly"), azureRecIn("westeurope", "upfront") + km, ku := cumKey(t, monthly), cumKey(t, upfront) + baseline := config.PlanRampBaseline{km: {Count: 10}, ku: {Count: 10}} + committed := map[string]config.PlanRampQuantity{km: {Count: 10}, ku: {Count: 5}} + // Step 2 at 50%/step is cumulative 100%: g1 (coverage 100) needs 10, g2 (coverage 50) needs 5. + bound := boundAzurePlan(map[string]config.ServiceConfig{ + "azure/compute#g1": azureGroup("monthly", 100, km), + "azure/compute#g2": azureGroup("upfront", 50, ku), + }) + _, err := Resolve(t.Context(), fakeSource{}, fakeStore{}, bound, accts, 2, baseline, committed) + require.ErrorIs(t, err, ErrTargetMet, "each group is judged against its own coverage") + + // (guard) an unmarked plan with the same payment split keeps today's + // first-entry match: the upfront key is judged at coverage 100 and is short. + legacy := &config.PurchasePlan{ID: "plan-l", RampSchedule: config.RampSchedule{Type: "custom", PercentPerStep: 50, TotalSteps: 2}, + Services: map[string]config.ServiceConfig{ + "azure:compute": {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "monthly", Coverage: 100}, + "azure:compute-u": {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "upfront", Coverage: 50}, + }} + _, err = Resolve(t.Context(), fakeSource{}, fakeStore{}, legacy, accts, 2, baseline, committed) + require.ErrorIs(t, err, ErrNoMatch) + + // A disabled group's identities are not judged, as for any disabled entry. + g2 := bound.Services["azure/compute#g2"] + g2.Enabled = false + bound.Services["azure/compute#g2"] = g2 + short := map[string]config.PlanRampQuantity{km: {Count: 10}, ku: {Count: 0}} + _, err = Resolve(t.Context(), fakeSource{}, fakeStore{}, bound, accts, 2, baseline, short) + require.ErrorIs(t, err, ErrTargetMet) + g2.Enabled = true + bound.Services["azure/compute#g2"] = g2 + + // A baselined key that no group bound is not judged, even though its + // attributes match g1: with nothing judged the step is not met. + other := azureRecIn("northeurope", "monthly") + ko := cumKey(t, other) + onlyOther := config.PlanRampBaseline{ko: {Count: 10}} + _, err = Resolve(t.Context(), fakeSource{}, fakeStore{}, bound, accts, 2, onlyOther, map[string]config.PlanRampQuantity{ko: {Count: 10}}) + require.ErrorIs(t, err, ErrNoMatch) +} diff --git a/internal/planstep/resolve.go b/internal/planstep/resolve.go index b0ca1ca2a..55b33ea47 100644 --- a/internal/planstep/resolve.go +++ b/internal/planstep/resolve.go @@ -10,11 +10,13 @@ import ( "fmt" "math" "math/big" + "slices" "sort" "strconv" "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -61,6 +63,9 @@ type Result struct { // what the plan already holds. Set only with ErrTargetMet, for the no-op // row's note. Target, Committed int + // MissingBound lists bound identities (issue #1746) the strict read did + // not show, through churn or suppression. They are not bought this step. + MissingBound []string } // ramp carries the cumulative-sizing inputs and accumulates its outputs. @@ -146,7 +151,11 @@ func (r *ramp) resolveServices(ctx context.Context, src Source, store Store, pla if !svc.Enabled { continue } - recs, err := resolveService(ctx, src, store, &svc, plan.RampSchedule.PercentPerStep, r, accountIDs, allowed, enabledProviders) + recs, missing, err := resolveService(ctx, src, store, &svc, plan.RampSchedule.PercentPerStep, r, accountIDs, allowed, enabledProviders) + if len(missing) > 0 { + logging.Warnf("plan %s service %q: %d bound recommendation(s) not shown, not bought this step: %v", plan.ID, k, len(missing), missing) + r.out.MissingBound = append(r.out.MissingBound, missing...) + } if err == nil { err = claimIdentities(seen, recs) } @@ -190,9 +199,14 @@ func (r *ramp) targetMet(plan *config.PurchasePlan) (bool, error) { return judged > 0, nil } -// planServiceOf finds the enabled plan service an identity key belongs to -// (provider, service and term are fields 0, 2 and 6 of the key). +// planServiceOf finds the enabled plan service an identity key belongs to. On +// a bound plan (issue #1746) that is the group whose bound identities hold the +// key, and a key no group bound is not judged. Otherwise it is the first entry +// with the key's provider, service and term (fields 0, 2 and 6), as before. func planServiceOf(plan *config.PurchasePlan, key string) (config.ServiceConfig, bool) { + if plan.RampSchedule.AllocationMode == config.RampAllocationBoundGroups { + return boundServiceOf(plan, key) + } f := strings.Split(key, "\x1f") if len(f) < 7 { return config.ServiceConfig{}, false @@ -206,6 +220,18 @@ func planServiceOf(plan *config.PurchasePlan, key string) (config.ServiceConfig, return config.ServiceConfig{}, false } +// boundServiceOf is planServiceOf for a bound plan: the enabled group whose +// bound identities hold key. +func boundServiceOf(plan *config.PurchasePlan, key string) (config.ServiceConfig, bool) { + for _, k := range sortedKeys(plan.Services) { + svc := plan.Services[k] + if svc.Enabled && slices.Contains(svc.BoundIdentities, key) { + return svc, true + } + } + return config.ServiceConfig{}, false +} + // Legacy targets multiply coverage*cum first; marked targets use exact shares. func (r *ramp) target(base int, coverage float64) (int, error) { if r.mode == config.RampAllocationEqualCount { @@ -246,41 +272,98 @@ func claimIdentities(seen map[string]struct{}, recs []config.RecommendationRecor return nil } +// resolveService sizes one plan service. A bound group (issue #1746) only +// considers its bound identities, and returns those the strict read did not +// show as missing. func resolveService(ctx context.Context, src Source, store Store, svc *config.ServiceConfig, percentPerStep float64, r *ramp, - accountIDs []string, allowed map[string]struct{}, enabledProviders []string) ([]config.RecommendationRecord, error) { + accountIDs []string, allowed map[string]struct{}, enabledProviders []string) ([]config.RecommendationRecord, []string, error) { wantPayment, err := r.validateInputs(svc, percentPerStep) if err != nil { - return nil, err + return nil, nil, err } filter := config.RecommendationFilter{Provider: svc.Provider, Service: svc.Service, AccountIDs: accountIDs} shown, err := src.ListRecommendationsStrict(ctx, filter) if err != nil { - return nil, err + return nil, nil, err } shown = config.FilterRecommendationsByEnabledProviders(shown, enabledProviders) stored, err := storedByIdentity(ctx, store, filter) if err != nil { - return nil, err + return nil, nil, err + } + bound := newBoundSet(svc.BoundIdentities) + if shown, err = bound.filter(shown); err != nil { + return nil, nil, err } var out []config.RecommendationRecord for i := range shown { rec := shown[i] match, err := matchesPlan(&rec, svc.Term, wantPayment, allowed) if err != nil { - return nil, err + return nil, nil, err } if !match { continue } sized, keep, err := r.sizeFromStored(&rec, stored, svc.Coverage, percentPerStep) if err != nil { - return nil, fmt.Errorf("recommendation %s: %w", rec.ID, err) + return nil, nil, fmt.Errorf("recommendation %s: %w", rec.ID, err) } if keep { out = append(out, sized) } } - return out, nil + return out, bound.missing(), nil +} + +// boundSet restricts a bound group to its bound identities and records which +// of them the strict read showed. A nil set (an unbound entry) admits every rec. +type boundSet struct { + ids []string + want map[string]bool // identity -> shown +} + +func newBoundSet(ids []string) *boundSet { + if len(ids) == 0 { + return nil + } + b := &boundSet{ids: ids, want: make(map[string]bool, len(ids))} + for _, id := range ids { + b.want[id] = false + } + return b +} + +// filter keeps the recs whose identity is bound and marks them shown. +func (b *boundSet) filter(recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { + if b == nil { + return recs, nil + } + var kept []config.RecommendationRecord + for i := range recs { + key, err := config.RecIdentityKey(&recs[i]) + if err != nil { + return nil, err + } + if _, ok := b.want[key]; ok { + b.want[key] = true + kept = append(kept, recs[i]) + } + } + return kept, nil +} + +func (b *boundSet) missing() []string { + if b == nil { + return nil + } + var out []string + for _, id := range b.ids { + if !b.want[id] { + out = append(out, id) + } + } + return out } // validateInputs rejects sizing inputs outside (0,100] (custom ramps too) and From 737c9b712a1d8bb15c106ed5a4d96cd83ffc2b5a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:51:25 +0100 Subject: [PATCH 2/4] feat(api): re-check the arming grant when a bound plan's groups widen Slice A, step A5 of #1746. When the stored or the new plan carries the bound_groups marker, these writes to an armed plan also count as escalations, so the execute:purchases grant's constraints are checked again: - a new effective group id; - a bound set that isn't a subset of the old one, including bound to unbound (an empty set means filter re-match); - a group cap raised or removed (a nil group cap is unbounded, unlike capCents(nil)); - a group's term or normalized payment changing; - a disabled group re-enabled; - the bound_groups marker removed. The provider and service sets can't show any of these. Unmarked plans keep today's rules exactly; a guard test pins that term-only and payment-only edits don't escalate. pre-commit: cfn-lint skipped (it can't install here); no CloudFormation files are touched. Part of #1746 Co-Authored-By: Claude Opus 5.5 --- .../api/auto_purchase_bound_escalation.go | 77 ++++++++++++++++++ .../auto_purchase_bound_escalation_test.go | 78 +++++++++++++++++++ internal/api/auto_purchase_grant.go | 3 +- 3 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 internal/api/auto_purchase_bound_escalation.go create mode 100644 internal/api/auto_purchase_bound_escalation_test.go diff --git a/internal/api/auto_purchase_bound_escalation.go b/internal/api/auto_purchase_bound_escalation.go new file mode 100644 index 000000000..4e30c0a0b --- /dev/null +++ b/internal/api/auto_purchase_bound_escalation.go @@ -0,0 +1,77 @@ +package api + +import ( + "slices" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// boundGroupsEscalate reports whether a write widens what an armed bound plan +// (issue #1746) buys in a way the provider and service sets cannot show, so +// the arming grant's constraints must be checked again. It applies when the +// stored or the new plan carries the bound_groups marker; unmarked plans keep +// today's rules. +func boundGroupsEscalate(before, after *config.PurchasePlan) bool { + beforeBound := isBoundPlan(before) + afterBound := isBoundPlan(after) + if !beforeBound && !afterBound { + return false + } + if beforeBound && !afterBound { + return true // the marker was removed + } + old := make(map[string]config.ServiceConfig, len(before.Services)) + for k, svc := range before.Services { + old[svc.EffectiveGroupID(k)] = svc + } + for k := range after.Services { + svc := after.Services[k] + prev, ok := old[svc.EffectiveGroupID(k)] + if !ok || groupWidens(&prev, &svc) { + return true + } + } + return false +} + +func isBoundPlan(p *config.PurchasePlan) bool { + return p != nil && p.RampSchedule.AllocationMode == config.RampAllocationBoundGroups +} + +// groupWidens compares one group before and after a write. +func groupWidens(prev, next *config.ServiceConfig) bool { + if next.Enabled && !prev.Enabled { + return true + } + // An empty set means filter re-match, the widest binding there is. + if len(prev.BoundIdentities) > 0 && len(next.BoundIdentities) == 0 { + return true + } + for _, id := range next.BoundIdentities { + if !slices.Contains(prev.BoundIdentities, id) { + return true + } + } + if groupCapRaised(prev.MaxStepCommitment, next.MaxStepCommitment) { + return true + } + return next.Term != prev.Term || canonicalPayment(next) != canonicalPayment(prev) +} + +// groupCapRaised treats a nil group cap as unbounded, unlike capCents(nil). +func groupCapRaised(prev, next *float64) bool { + if next == nil { + return prev != nil + } + if prev == nil { + return false + } + return capCents(next) > capCents(prev) +} + +func canonicalPayment(svc *config.ServiceConfig) string { + if canon, ok := config.NormalizePaymentOption(svc.Provider, svc.Payment); ok { + return canon + } + return svc.Payment +} diff --git a/internal/api/auto_purchase_bound_escalation_test.go b/internal/api/auto_purchase_bound_escalation_test.go new file mode 100644 index 000000000..7bc6980c1 --- /dev/null +++ b/internal/api/auto_purchase_bound_escalation_test.go @@ -0,0 +1,78 @@ +package api + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +// armedBoundPlan is an armed bound plan with one group g1. +func armedBoundPlan() *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + p.Services = map[string]config.ServiceConfig{"aws/ec2#g1": { + Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80, + MinCount: 1, GroupID: "g1", BoundIdentities: []string{"k1", "k2"}, MaxStepCommitment: apgCap(100), + }} + return p +} + +// edited returns a copy of the armed bound plan with f applied to group g1. +func edited(f func(p *config.PurchasePlan, g *config.ServiceConfig)) *config.PurchasePlan { + p := armedBoundPlan() + g := p.Services["aws/ec2#g1"] + f(p, &g) + p.Services["aws/ec2#g1"] = g + return p +} + +func TestPlanArmEscalates_BoundGroups(t *testing.T) { + for _, tc := range []struct { + name string + after *config.PurchasePlan + want bool + }{ + {"unchanged", armedBoundPlan(), false}, + {"narrowed bound set", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = []string{"k1"} }), false}, + {"lowered group cap", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = apgCap(50) }), false}, + {"coverage edit", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Coverage = 60 }), false}, + {"new group", edited(func(p *config.PurchasePlan, _ *config.ServiceConfig) { + p.Services["aws/ec2#g2"] = config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, Term: 3, Payment: "all-upfront", GroupID: "g2", BoundIdentities: []string{"k9"}} + }), true}, + {"added bound identity", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = []string{"k1", "k2", "k3"} }), true}, + {"bound to unbound", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = nil }), true}, + {"raised group cap", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = apgCap(200) }), true}, + {"removed group cap (nil is unbounded)", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = nil }), true}, + {"term changed", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Term = 3 }), true}, + {"payment changed", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Payment = "all-upfront" }), true}, + {"marker removed", edited(func(p *config.PurchasePlan, _ *config.ServiceConfig) { p.RampSchedule.AllocationMode = "" }), true}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, planArmEscalates(armedBoundPlan(), tc.after)) + }) + } + t.Run("re-enabling a disabled group", func(t *testing.T) { + before := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Enabled = false }) + assert.True(t, planArmEscalates(before, armedBoundPlan())) + assert.False(t, planArmEscalates(armedBoundPlan(), before), "disabling a group narrows") + }) + t.Run("a group cap added where none was set narrows", func(t *testing.T) { + before := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = nil }) + assert.False(t, planArmEscalates(before, armedBoundPlan())) + }) +} + +// (guard) Unmarked armed plans keep today's rules: a term- or payment-only +// edit is not an escalation, so admin-key callers are unaffected. +func TestPlanArmEscalates_UnmarkedTermAndPaymentEditsDoNotEscalate(t *testing.T) { + plan := func(term int, payment string) *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.Services = map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Enabled: true, Term: term, Payment: payment}} + return p + } + assert.False(t, planArmEscalates(plan(1, "no-upfront"), plan(3, "no-upfront"))) + assert.False(t, planArmEscalates(plan(1, "no-upfront"), plan(1, "all-upfront"))) +} diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index dfc6952a2..46c972d12 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -72,7 +72,8 @@ func planArmEscalates(before, after *config.PurchasePlan) bool { return true } return !subsetOf(afterProviders, beforeProviders) || - !subsetOf(config.DerivePlanServices(after), config.DerivePlanServices(before)) + !subsetOf(config.DerivePlanServices(after), config.DerivePlanServices(before)) || + boundGroupsEscalate(before, after) } func subsetOf(sub, super []string) bool { From fea2b01fdadc8edf61e65a102e320df632737816 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 01:38:21 +0100 Subject: [PATCH 3/4] fix(api): avoid copying service configs when comparing bound groups Part of #1746. Co-Authored-By: Claude Opus 5.5 --- internal/api/auto_purchase_bound_escalation.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/api/auto_purchase_bound_escalation.go b/internal/api/auto_purchase_bound_escalation.go index 4e30c0a0b..56462279a 100644 --- a/internal/api/auto_purchase_bound_escalation.go +++ b/internal/api/auto_purchase_bound_escalation.go @@ -21,7 +21,8 @@ func boundGroupsEscalate(before, after *config.PurchasePlan) bool { return true // the marker was removed } old := make(map[string]config.ServiceConfig, len(before.Services)) - for k, svc := range before.Services { + for k := range before.Services { + svc := before.Services[k] old[svc.EffectiveGroupID(k)] = svc } for k := range after.Services { From 56c54f8f146ec6aa79afe208ef6157ab19406e90 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 04:59:28 +0100 Subject: [PATCH 4/4] fix(api): ignore disabled bound groups when checking arming escalation A disabled group buys nothing, so adding or widening one no longer counts as an escalation; enabling it still does. Tests also pin that arming a disarmed bound plan escalates even when the write narrows it. Part of #1746. Co-Authored-By: Claude Opus 5.5 --- internal/api/auto_purchase_bound_escalation.go | 4 ++++ .../api/auto_purchase_bound_escalation_test.go | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/internal/api/auto_purchase_bound_escalation.go b/internal/api/auto_purchase_bound_escalation.go index 56462279a..1c251a85f 100644 --- a/internal/api/auto_purchase_bound_escalation.go +++ b/internal/api/auto_purchase_bound_escalation.go @@ -27,6 +27,10 @@ func boundGroupsEscalate(before, after *config.PurchasePlan) bool { } for k := range after.Services { svc := after.Services[k] + // A disabled group buys nothing; enabling it later is checked then. + if !svc.Enabled { + continue + } prev, ok := old[svc.EffectiveGroupID(k)] if !ok || groupWidens(&prev, &svc) { return true diff --git a/internal/api/auto_purchase_bound_escalation_test.go b/internal/api/auto_purchase_bound_escalation_test.go index 7bc6980c1..a2c5c19a6 100644 --- a/internal/api/auto_purchase_bound_escalation_test.go +++ b/internal/api/auto_purchase_bound_escalation_test.go @@ -59,6 +59,24 @@ func TestPlanArmEscalates_BoundGroups(t *testing.T) { assert.True(t, planArmEscalates(before, armedBoundPlan())) assert.False(t, planArmEscalates(armedBoundPlan(), before), "disabling a group narrows") }) + t.Run("arming a disarmed bound plan escalates even when it narrows", func(t *testing.T) { + disarmed := armedBoundPlan() + disarmed.AutoPurchase = false + assert.True(t, planArmEscalates(disarmed, armedBoundPlan()), "unchanged") + narrowed := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = []string{"k1"} }) + assert.True(t, planArmEscalates(disarmed, narrowed), "narrowed") + }) + t.Run("a disabled group buys nothing, so its edits wait for re-enabling", func(t *testing.T) { + withG2 := func(enabled bool, ids ...string) *config.PurchasePlan { + return edited(func(p *config.PurchasePlan, _ *config.ServiceConfig) { + p.Services["aws/ec2#g2"] = config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: enabled, Term: 1, Payment: "no-upfront", GroupID: "g2", BoundIdentities: ids} + }) + } + assert.False(t, planArmEscalates(armedBoundPlan(), withG2(false, "k9")), "new disabled group") + assert.False(t, planArmEscalates(withG2(false, "k9"), withG2(false, "k9", "k8")), "disabled group widened") + assert.True(t, planArmEscalates(withG2(false, "k9"), withG2(true, "k9")), "enabling it") + assert.True(t, planArmEscalates(armedBoundPlan(), withG2(true, "k9")), "new enabled group") + }) t.Run("a group cap added where none was set narrows", func(t *testing.T) { before := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = nil }) assert.False(t, planArmEscalates(before, armedBoundPlan()))