From 73f7480bc8c02971fe7f52ad3025e6e0b827c84c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 05:41:06 +0200 Subject: [PATCH 1/4] fix(api): derive the purchase spend cap from the stored recommendation price POST /api/purchases/execute enforced MaxPurchaseAmount, and stamped the execution row and the approval email, from the upfront_cost, monthly_cost and savings the client sent. A purchaser holding execute:purchases with a $1,000 cap could submit upfront_cost: 1 for 100 three-year m5.24xlarge reservations and the provider bought them at list price (audit A01-001). Every rec in the request is now matched against the stored recommendation set on (provider, account, service, region, resource_type, engine, term, payment). Its id, details and cost fields are replaced by the stored row's values scaled by count before the constraint check, the execution row, the approval email and the idempotency key read them. A rec that matches no stored recommendation, or whose stored row carries no usable price, is refused with 409. Retry re-checks the cap against the persisted row, which is now written only from store-derived values. Closes #1905 Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- .../api/handler_per_account_perms_test.go | 5 + internal/api/handler_purchases.go | 45 +- internal/api/handler_purchases_guards_test.go | 9 + internal/api/handler_purchases_test.go | 58 +- internal/api/openapi.yaml | 9 + internal/api/purchase_pricing.go | 149 +++++ internal/api/purchase_pricing_test.go | 534 ++++++++++++++++++ 7 files changed, 791 insertions(+), 18 deletions(-) create mode 100644 internal/api/purchase_pricing.go create mode 100644 internal/api/purchase_pricing_test.go diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index e016f58a4..ded493a06 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -711,6 +711,11 @@ func TestPerAccountPerms_ExecutePurchase_AllowedAccountAccepted(t *testing.T) { mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } + // #1905: the rec is now priced from the stored recommendation set. + accA := permsAccA + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", CloudAccountID: &accA, Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 10, + }) handler := &Handler{ auth: scopedAuthMock(ctx), diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 26a25deb2..9d0adf1c6 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1626,7 +1626,10 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU // relaunch another user's over-cap failed purchase that the retrying // session's OWN constraints would deny, and constraints tightened after // the original submission would never apply on replay (adversarial - // review follow-up to #1210). + // review follow-up to #1210). failedExec.Recommendations carry the + // store-derived prices stamped at submit time + // (priceRecommendationsFromStore), so this re-check never sees + // client-supplied numbers. if constraintErr := h.enforcePurchaseConstraints(ctx, session, failedExec.Recommendations); constraintErr != nil { return nil, constraintErr } @@ -2100,6 +2103,11 @@ func (h *Handler) getPurchaseDetails(ctx context.Context, req *events.LambdaFunc // ExecutePurchaseRequest represents the request to execute purchases. type ExecutePurchaseRequest struct { + // Recommendations are matched against the stored recommendation set on + // (provider, cloud_account_id, service, region, resource_type, engine, + // term, payment). Only count, recommended_count and selected are taken + // from the request; id, details and every cost field are replaced from + // the stored row scaled by count (priceRecommendationsFromStore, #1905). Recommendations []config.RecommendationRecord `json:"recommendations"` // CapacityPercent is what fraction (1..100) of the originally- // recommended counts the user chose in the bulk Purchase flow. @@ -2169,16 +2177,17 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event if err := validateCapacityConsistency(execReq.Recommendations, execReq.CapacityPercent); err != nil { return ExecutePurchaseRequest{}, nil, nil, err } - // Enforce the per-permission Constraints (MaxPurchaseAmount, Providers, - // Services, Regions, AccountIDs) configured on the granting - // execute:purchases permission (SEC-01, issue #1141). Runs after the - // per-rec validation above so provider tokens are already normalized. - // Each recommendation must individually be granted by a permission; the - // amount cap is checked against the batch's total commitment (upfront - // plus recurring, see recTotalCommitment) so it cannot be evaded by - // splitting a large purchase across recs or by a no-upfront commitment - // whose real cost is entirely recurring. - if err := h.enforcePurchaseConstraints(ctx, session, execReq.Recommendations); err != nil { + // Price every rec from the stored recommendation set, then enforce the + // per-permission Constraints (MaxPurchaseAmount, Providers, Services, + // Regions, AccountIDs) on the granting execute:purchases permission + // (SEC-01, issue #1141) against those store-derived values. Runs after + // the per-rec validation above so provider and payment tokens are + // canonical before the identity match, and after the scope check so an + // out-of-scope account is refused as 403 before any pricing lookup. The + // client's own upfront_cost / monthly_cost / savings / details are never + // used (issue #1905): a purchaser under a MaxPurchaseAmount cap could + // otherwise state any number and buy at list price. + if err := h.priceAndEnforcePurchaseConstraints(ctx, session, execReq.Recommendations); err != nil { return ExecutePurchaseRequest{}, nil, nil, err } return execReq, session, adjustments, nil @@ -2191,7 +2200,10 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // Shared by the direct-execute request validation and the retry path // (retryPurchase) so both re-derive and check the exact same Constraints // (SEC-01, issue #1141; adversarial review follow-up to #1210 -- the retry -// path previously skipped this check entirely). +// path previously skipped this check entirely). The web execute path +// reaches this through priceAndEnforcePurchaseConstraints, so recs carry +// store-derived costs; the retry path enforces against the persisted row, +// which was priced when it was written. func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Session, recs []config.RecommendationRecord) error { constraintSets := purchaseConstraintSets(recs) if err := requireNonZeroCommitment(constraintSets); err != nil { @@ -2251,7 +2263,9 @@ func purchaseConstraintSets(recs []config.RecommendationRecord) []auth.Permissio // same *12 convention used throughout this package. A nil MonthlyCost // (all-upfront commitments have no recurring charge) or a non-positive // Term (can't be converted to months) contribute nothing to the recurring -// leg, matching the Term<=0 handling elsewhere in this package. +// leg, matching the Term<=0 handling elsewhere in this package. On the web +// execute path the two cost fields are the store-derived values written by +// priceRecommendationsFromStore. func recTotalCommitment(rec *config.RecommendationRecord) float64 { total := rec.UpfrontCost if rec.Term > 0 && rec.MonthlyCost != nil { @@ -2341,7 +2355,10 @@ func (h *Handler) finalizePurchaseStatus(ctx context.Context, execution *config. return "failed" } -// validateAndTotalRecommendations validates each recommendation and returns totals. +// validateAndTotalRecommendations validates each recommendation and returns +// totals. Runs on store-derived recs for the web path and on the persisted +// row for retry; the negative and $10M guards stay as the sanity check for +// both. func validateAndTotalRecommendations(recs []config.RecommendationRecord) (upfront, savings float64, err error) { const maxAmount = 10_000_000 // $10M sanity cap for i := range recs { diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index 8ff602897..f6113a785 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -363,6 +363,12 @@ func TestHandler_executePurchase_SurfacesPaymentAdjustments(t *testing.T) { mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + // #1905: both recs canonicalise to azure/vm/monthly before pricing, so + // they both match this one stored row; the second rec's client + // upfront_cost is ignored. + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "azure", Service: "vm", Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 100, Savings: 50, + }) handler := &Handler{config: mockStore, auth: mockAuth} @@ -413,6 +419,9 @@ func TestHandler_executePurchase_NoAdjustmentsWhenCanonical(t *testing.T) { mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "azure", Service: "vm", Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 100, Savings: 50, + }) handler := &Handler{config: mockStore, auth: mockAuth} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index e4a1a80e4..ae3c345f8 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2202,6 +2202,11 @@ func TestHandler_executePurchase_Success(t *testing.T) { // The #644 idempotency lookup queries pending executions before creating. // No prior pending row → not a duplicate → proceeds to create. mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + // #1905: recs are now priced from the stored recommendation set. + expectStoredRecs(mockStore, + config.RecommendationRecord{Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50}, + config.RecommendationRecord{Provider: "aws", Service: "ec2", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 200, Savings: 100}, + ) handler := &Handler{config: mockStore, auth: mockAuth} @@ -2283,6 +2288,7 @@ func TestHandler_executePurchase_EmptyRecommendations(t *testing.T) { func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) { ctx := context.Background() + mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) adminSession := &Session{ @@ -2292,8 +2298,14 @@ func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdminPurchaser() + // #1905: the client's upfront_cost is now ignored; a negative cost on + // the STORED row is what must be refused (the client value can no + // longer poison the check). + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: -100, + }) - handler := &Handler{auth: mockAuth} + handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{ @@ -2304,11 +2316,12 @@ func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) { result, err := handler.executePurchase(ctx, req) assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "negative upfront cost") + assert.Contains(t, err.Error(), "no usable price") } func TestHandler_executePurchase_NegativeSavings(t *testing.T) { ctx := context.Background() + mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) adminSession := &Session{ @@ -2318,8 +2331,14 @@ func TestHandler_executePurchase_NegativeSavings(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdminPurchaser() + // #1905: pricing passes (stored upfront/count are usable); the negative + // savings on the stored row still trips the validateAndTotalRecommendations + // guard the same as before. + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: -50, + }) - handler := &Handler{auth: mockAuth} + handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{ @@ -2372,6 +2391,7 @@ func TestHandler_executePurchase_TooManyRecommendations(t *testing.T) { func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) { ctx := context.Background() + mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) adminSession := &Session{ @@ -2381,8 +2401,12 @@ func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdminPurchaser() + // #1905: the $10M sanity guard now fires against the stored cost. + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 15_000_000, Savings: 50, + }) - handler := &Handler{auth: mockAuth} + handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{ @@ -2411,6 +2435,9 @@ func TestHandler_executePurchase_SaveError(t *testing.T) { mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(errors.New("database error")) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50, + }) handler := &Handler{config: mockStore, auth: mockAuth} @@ -3674,6 +3701,10 @@ func setupDirectExecMocks(ctx context.Context, store *MockConfigStore) { store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) store.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + // #1905: directExecRecBody's rec-1 is now priced from the stored set. + expectStoredRecs(store, config.RecommendationRecord{ + ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + }) } // TestHandler_executePurchase_DirectExec_NoPermission verifies the fail-closed @@ -3767,6 +3798,11 @@ func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) { assert.ObjectsAreEqual([]string{"ec2"}, sets[1].Services) && assert.ObjectsAreEqual([]string{"eu-west-1"}, sets[1].Regions) })).Return(false, nil) + // #1905: constraint sets are now built from the stored costs. + expectStoredRecs(mockStore, + config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 50}, + config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "eu-west-1", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 2500, Savings: 100}, + ) handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ @@ -3823,6 +3859,12 @@ func TestHandler_executePurchase_NoUpfrontBatch_TotalCommitmentEnforced(t *testi mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { return len(sets) == 1 && sets[0].MaxPurchaseAmount == wantTotalCommitment })).Return(false, nil) + // #1905: the total-commitment constraint is now built from the stored + // upfront/monthly cost. + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 3, Payment: "no-upfront", + UpfrontCost: 0, MonthlyCost: float64Ptr(600), Savings: 50, + }) handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ @@ -3881,6 +3923,10 @@ func TestHandler_executePurchase_UserAPIKeyConstraintsDenied(t *testing.T) { } return sets[0].MaxPurchaseAmount == 500.0 })).Return(false, nil) + // #1905: the constraint set is now built from the stored cost. + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 50, + }) handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ @@ -4107,6 +4153,10 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute(t *test mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) + // #1905: directExecRecBody's rec-1 is now priced from the stored set. + expectStoredRecs(mockStore, config.RecommendationRecord{ + ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + }) // enforceFourEyesPolicy re-loads the (freshly-created, randomly-ID'd) // execution to read CreatedByUserID; the real one always carries the // direct-executor's own UUID (see comment above), which is what makes diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index e3529bd54..487af99d2 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -466,6 +466,8 @@ paths: $ref: '#/components/responses/Unauthorized' '403': $ref: '#/components/responses/Forbidden' + '409': + $ref: '#/components/responses/Conflict' /api/purchases/approve/{id}: parameters: @@ -2848,6 +2850,13 @@ components: $ref: '#/components/schemas/RecommendationRecord' minItems: 1 maxItems: 1000 + description: > + Each item must match a recommendation in the current recommendation set on + (provider, cloud_account_id, service, region, resource_type, engine, term, + payment). Only count, recommended_count and selected are honoured; id, details, + upfront_cost, monthly_cost and savings are replaced server-side from the stored + recommendation scaled by count, and the spend-cap check, execution record and + approval email use those values. Savings plan items must keep the recommended count. ExecutePurchaseResponse: type: object diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go new file mode 100644 index 000000000..9b1298834 --- /dev/null +++ b/internal/api/purchase_pricing.go @@ -0,0 +1,149 @@ +package api + +import ( + "context" + "fmt" + "strconv" + "strings" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// priceAndEnforcePurchaseConstraints replaces every rec's cost fields with +// values derived from the stored recommendation that matches it, then +// enforces the execute:purchases Constraints against those values. The +// client-supplied upfront_cost / monthly_cost / savings / details are never +// consulted for the spend cap, the execution row or the approval email +// (issue #1905, audit A01-001). Wrapped in one call so +// validateExecutePurchaseRequest keeps its gocyclo score (it is at the +// pre-commit ceiling of 10). +func (h *Handler) priceAndEnforcePurchaseConstraints(ctx context.Context, session *Session, recs []config.RecommendationRecord) error { + if err := h.priceRecommendationsFromStore(ctx, recs); err != nil { + return err + } + return h.enforcePurchaseConstraints(ctx, session, recs) +} + +// priceRecommendationsFromStore rewrites recs in place. Each rec must match +// a row of the stored recommendation set on its identity tuple +// (recIdentityKey); unmatched recs are refused rather than priced from the +// request. Matching by tuple instead of by rec.ID lets the purchase modal's +// term/payment change (#111, #197, #1903) resolve to the stored variant the +// user actually chose: AWS stores every (term, payment) combination and +// Azure stores both payment variants, each under its own id. +func (h *Handler) priceRecommendationsFromStore(ctx context.Context, recs []config.RecommendationRecord) error { + stored, err := h.loadStoredRecommendationIndex(ctx, recs) + if err != nil { + return err + } + for i := range recs { + match, ok := stored[recIdentityKey(&recs[i])] + if !ok { + return NewClientError(409, fmt.Sprintf( + "recommendation %d (%s) is not in the current recommendation set; refresh the recommendations and try again", + i, describeRec(&recs[i]))) + } + priced, err := priceFromStored(&recs[i], &match, i) + if err != nil { + return err + } + recs[i] = priced + } + return nil +} + +// loadStoredRecommendationIndex reads the stored rows for every provider in +// the batch (one query per distinct provider, at most three) and indexes +// them by identity tuple. The store's unique index on the same tuple +// (migration 000043) guarantees one row per key. +func (h *Handler) loadStoredRecommendationIndex(ctx context.Context, recs []config.RecommendationRecord) (map[string]config.RecommendationRecord, error) { + index := make(map[string]config.RecommendationRecord) + seen := make(map[string]bool) + for i := range recs { + provider := recs[i].Provider + if seen[provider] { + continue + } + seen[provider] = true + rows, err := h.config.ListStoredRecommendations(ctx, config.RecommendationFilter{Provider: provider}) + if err != nil { + return nil, fmt.Errorf("load stored recommendations for %s: %w", provider, err) + } + for j := range rows { + index[recIdentityKey(&rows[j])] = rows[j] + } + } + return index, nil +} + +// recIdentityKey is the tuple a price is a function of: the same eight +// components the scheduler encodes into RecommendationRecord.ID, with a nil +// and an empty CloudAccountID collapsed together (as purchaseConstraintSets +// and the store's account_key both do). Provider and payment are lowercased +// because validatePurchaseRecommendation canonicalises the request side. +func recIdentityKey(rec *config.RecommendationRecord) string { + account := "" + if rec.CloudAccountID != nil { + account = *rec.CloudAccountID + } + return strings.Join([]string{ + strings.ToLower(rec.Provider), account, rec.Service, rec.Region, rec.ResourceType, + rec.Engine, strconv.Itoa(rec.Term), strings.ToLower(rec.Payment), + }, "\x1f") +} + +// priceFromStored returns a copy of stored carrying the request's Count, +// RecommendedCount and Selected, with UpfrontCost, MonthlyCost, Savings and +// OnDemandCost scaled by Count/stored.Count (stored costs are totals for +// stored.Count; RI/CUD/Azure prices are linear in count). Savings Plans are +// not count-denominated, so their Count must equal the stored placeholder. +// A stored row that cannot yield a positive price is refused: the cap must +// never be evaluated against a zero or negative commitment. +func priceFromStored(req, stored *config.RecommendationRecord, idx int) (config.RecommendationRecord, error) { + if stored.Count <= 0 { + return config.RecommendationRecord{}, NewClientError(409, fmt.Sprintf( + "recommendation %d (%s): stored recommendation has count %d, cannot derive a per-unit price", + idx, describeRec(req), stored.Count)) + } + if stored.UpfrontCost < 0 || (stored.MonthlyCost != nil && *stored.MonthlyCost < 0) || recTotalCommitment(stored) <= 0 { + return config.RecommendationRecord{}, NewClientError(409, fmt.Sprintf( + "recommendation %d (%s): stored recommendation carries no usable price (upfront %.2f, monthly %s); the spend cap cannot be enforced against it", + idx, describeRec(req), stored.UpfrontCost, formatOptionalCost(stored.MonthlyCost))) + } + if common.IsSavingsPlan(common.ServiceType(stored.Service)) && req.Count != stored.Count { + return config.RecommendationRecord{}, NewClientError(409, fmt.Sprintf( + "recommendation %d (%s): savings plan count %d must equal the recommended count %d; savings plans are priced by hourly commitment, not by count", + idx, describeRec(req), req.Count, stored.Count)) + } + ratio := float64(req.Count) / float64(stored.Count) + out := *stored + out.Count = req.Count + out.RecommendedCount = req.RecommendedCount + out.Selected = req.Selected + out.UpfrontCost = stored.UpfrontCost * ratio + out.Savings = stored.Savings * ratio + out.MonthlyCost = scaledCost(stored.MonthlyCost, ratio) + out.OnDemandCost = scaledCost(stored.OnDemandCost, ratio) + return out, nil +} + +func scaledCost(v *float64, ratio float64) *float64 { + if v == nil { + return nil + } + s := *v * ratio + return &s +} + +func formatOptionalCost(v *float64) string { + if v == nil { + return "absent" + } + return fmt.Sprintf("%.2f", *v) +} + +func describeRec(rec *config.RecommendationRecord) string { + return fmt.Sprintf("%s/%s %s %s engine=%q term=%d payment=%s account=%s", + rec.Provider, rec.Service, rec.Region, rec.ResourceType, rec.Engine, rec.Term, rec.Payment, derefStringOrEmpty(rec.CloudAccountID)) +} diff --git a/internal/api/purchase_pricing_test.go b/internal/api/purchase_pricing_test.go new file mode 100644 index 000000000..1ada67dbe --- /dev/null +++ b/internal/api/purchase_pricing_test.go @@ -0,0 +1,534 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// expectStoredRecs registers the ListStoredRecommendations expectation the +// pricing lookup issues: one call per distinct provider, filtered by provider +// only. Registering the exact filter pins that contract. +func expectStoredRecs(store *MockConfigStore, recs ...config.RecommendationRecord) { + byProvider := map[string][]config.RecommendationRecord{} + for _, r := range recs { + byProvider[r.Provider] = append(byProvider[r.Provider], r) + } + for provider, rows := range byProvider { + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: provider}).Return(rows, nil) + } +} + +// TestHandler_executePurchase_CapUsesStoredPriceNotClientPrice is the issue's +// reproduction (audit A01-001): a purchaser holding execute:purchases with a +// $1,000 MaxPurchaseAmount cap submits upfront_cost: 1 for 100 x m5.24xlarge +// x 3yr all-upfront reservations whose real stored price is $1,000,000. The +// cap must be enforced against the stored price, not the client's number. +func TestHandler_executePurchase_CapUsesStoredPriceNotClientPrice(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockPurchase.AssertExpectations(t) }) + + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"} + mockAuth.On("ValidateSession", ctx, "cap-token").Return(session, nil) + mockAuth.grantPermissions([]auth.Permission{ + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}}, + {Action: auth.ActionExecuteAny, Resource: auth.ResourcePurchases}, + }) + + expectStoredRecs(mockStore, config.RecommendationRecord{ + ID: "aws|123456789012|ec2|us-east-1|m5.24xlarge||3|all-upfront", + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.24xlarge", + Count: 100, Term: 3, Payment: "all-upfront", UpfrontCost: 1_000_000, Savings: 40_000, + }) + // Registered .Maybe(): pre-fix these are reached, post-fix they must not + // be. Not requiring GetGlobalConfig/GetPendingExecutions here because the + // mock's isExpected guard on GetGlobalConfig defaults gracefully, and + // this test is not asserting on the persisted row's suppression window; + // GetPendingExecutions has no such guard, so it must be stubbed to avoid + // a panic on the pre-fix run, which does reach persistExecutionAndSuppressions. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer cap-token"}, + Body: `{"recommendations":[{"id":"aws|123456789012|ec2|us-east-1|m5.24xlarge||3|all-upfront","provider":"aws","service":"ec2","region":"us-east-1","resource_type":"m5.24xlarge","count":100,"term":3,"payment":"all-upfront","upfront_cost":1,"monthly_cost":null,"savings":40000,"selected":true}],"execute_mode":"direct"}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// TestHandler_executePurchase_StoredPriceUnderCapProceeds is the no-over- +// rejection companion to T1: the client overstates upfront_cost far beyond +// the cap, but the STORED price is well under it, so the purchase must +// proceed and the response must reflect the stored numbers. +func TestHandler_executePurchase_StoredPriceUnderCapProceeds(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockPurchase.AssertExpectations(t) }) + + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"} + mockAuth.On("ValidateSession", ctx, "cap-token").Return(session, nil) + mockAuth.grantPermissions([]auth.Permission{ + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}}, + {Action: auth.ActionExecuteAny, Resource: auth.ResourcePurchases}, + }) + + expectStoredRecs(mockStore, config.RecommendationRecord{ + ID: "aws|123456789012|ec2|us-east-1|m5.24xlarge||3|all-upfront", + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.24xlarge", + Count: 1, Term: 3, Payment: "all-upfront", UpfrontCost: 900, Savings: 30, + }) + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil) + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer cap-token"}, + Body: `{"recommendations":[{"id":"aws|123456789012|ec2|us-east-1|m5.24xlarge||3|all-upfront","provider":"aws","service":"ec2","region":"us-east-1","resource_type":"m5.24xlarge","count":1,"term":3,"payment":"all-upfront","upfront_cost":5000,"savings":30,"selected":true}],"execute_mode":"direct"}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + resultMap := result.(map[string]any) + assert.Equal(t, "completed", resultMap["status"]) + assert.Equal(t, 900.0, resultMap["total_upfront_cost"]) + assert.Equal(t, 30.0, resultMap["estimated_savings"]) +} + +// TestHandler_executePurchase_PersistsStoredCostsNotClientCosts pins the +// second half of #1905: the execution row and the approval email must carry +// the store-derived costs, not the client's. +func TestHandler_executePurchase_PersistsStoredCostsNotClientCosts(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + notify := "notify@example.com" + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{NotificationEmail: ¬ify}, nil) + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil) + + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }). + Return(nil) + + expectStoredRecs(mockStore, config.RecommendationRecord{ + ID: "stored-id", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.24xlarge", + Count: 4, Term: 3, Payment: "all-upfront", + UpfrontCost: 4000, MonthlyCost: float64Ptr(40), Savings: 400, OnDemandCost: float64Ptr(1000), + Details: json.RawMessage(`{"platform":"Linux/UNIX"}`), + }) + + notifier := &recordingEmailNotifier{} + handler := &Handler{config: mockStore, auth: mockAuth, emailNotifier: notifier} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"client-id","provider":"aws","service":"ec2","region":"us-east-1","resource_type":"m5.24xlarge","count":2,"term":3,"payment":"all-upfront","recommended_count":4,"upfront_cost":1,"monthly_cost":1,"savings":999,"on_demand_cost":1,"details":{"platform":"Windows"},"selected":true}],"capacity_percent":50}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + + require.NotNil(t, saved) + require.Len(t, saved.Recommendations, 1) + rec := saved.Recommendations[0] + assert.Equal(t, "stored-id", rec.ID) + assert.Equal(t, 2, rec.Count) + assert.Equal(t, 4, rec.RecommendedCount) + assert.Equal(t, 2000.0, rec.UpfrontCost) + require.NotNil(t, rec.MonthlyCost) + assert.Equal(t, 20.0, *rec.MonthlyCost) + assert.Equal(t, 200.0, rec.Savings) + require.NotNil(t, rec.OnDemandCost) + assert.Equal(t, 500.0, *rec.OnDemandCost) + assert.Equal(t, `{"platform":"Linux/UNIX"}`, string(rec.Details)) + assert.True(t, rec.Selected) + + assert.Equal(t, 2000.0, saved.TotalUpfrontCost) + assert.Equal(t, 200.0, saved.EstimatedSavings) + + assert.Equal(t, 2000.0, notifier.captured.TotalUpfrontCost) + assert.Equal(t, 200.0, notifier.captured.TotalSavings) + + resultMap := result.(map[string]any) + assert.Equal(t, 2000.0, resultMap["total_upfront_cost"]) + assert.Equal(t, 200.0, resultMap["estimated_savings"]) + assert.Equal(t, true, resultMap["email_sent"]) +} + +// TestHandler_executePurchase_UnknownRecommendationRefused: a rec that +// matches no stored recommendation is refused with 409, before anything is +// persisted. +func TestHandler_executePurchase_UnknownRecommendationRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + // Registered .Maybe(): grantAdminPurchaser carries no MaxPurchaseAmount + // cap, so pre-fix the (unpriced) constraint check trivially passes and + // the flow reaches persistExecutionAndSuppressions, which needs these to + // avoid a panic on the unstubbed mock; post-fix they are never reached. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, + }) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","resource_type":"m5.xlarge","count":1,"term":1,"payment":"all-upfront","upfront_cost":100,"savings":10}]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "not in the current recommendation set") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestHandler_executePurchase_StoredRecWithoutPriceRefused: a matched stored +// row that carries no usable price is refused with 409. +func TestHandler_executePurchase_StoredRecWithoutPriceRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + // Registered .Maybe(): see TestHandler_executePurchase_UnknownRecommendationRefused. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", + UpfrontCost: 0, MonthlyCost: nil, + }) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","resource_type":"m5.large","count":1,"term":1,"payment":"all-upfront","upfront_cost":100,"savings":10}]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "no usable price") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestHandler_executePurchase_StoredRecWithZeroCountRefused: a matched +// stored row with a non-positive Count cannot yield a per-unit price and +// must be refused with 409 rather than dividing by zero or by a negative +// count (section 4 of the #1905 plan). +func TestHandler_executePurchase_StoredRecWithZeroCountRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + // Registered .Maybe(): see TestHandler_executePurchase_UnknownRecommendationRefused. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 0, Term: 1, Payment: "all-upfront", UpfrontCost: 100, + }) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","resource_type":"m5.large","count":1,"term":1,"payment":"all-upfront","upfront_cost":100,"savings":10}]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "cannot derive a per-unit price") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestHandler_executePurchase_StoreErrorFailsClosed: a store read error is a +// 500, never a client-value fallback. +func TestHandler_executePurchase_StoreErrorFailsClosed(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + // Registered .Maybe(): see TestHandler_executePurchase_UnknownRecommendationRefused. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + mockStore.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "aws"}). + Return(nil, errors.New("pg down")) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","resource_type":"m5.large","count":1,"term":1,"payment":"all-upfront","upfront_cost":100,"savings":10}]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a store read failure must not be reported as a client error") + assert.ErrorContains(t, err, "load stored recommendations for aws") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestHandler_executePurchase_PaymentChangePricedFromStoredVariant: the +// purchase modal's term/payment change (#111, #197, #1903) resolves to the +// stored variant the user actually chose, not the id's original variant. +func TestHandler_executePurchase_PaymentChangePricedFromStoredVariant(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"} + mockAuth.On("ValidateSession", ctx, "cap-token").Return(session, nil) + mockAuth.grantPermissions([]auth.Permission{ + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1500}}, + }) + + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }). + Return(nil) + + expectStoredRecs(mockStore, + config.RecommendationRecord{ + ID: "aws|acct|ec2|us-east-1|m5.large||1|all-upfront", + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 10, + }, + config.RecommendationRecord{ + ID: "aws|acct|ec2|us-east-1|m5.large||1|no-upfront", + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Count: 1, Term: 1, Payment: "no-upfront", UpfrontCost: 0, MonthlyCost: float64Ptr(100), Savings: 10, + }, + ) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer cap-token"}, + // Client sends variant A's id, but the NEW payment "no-upfront" with + // stale variant-A costs. + Body: `{"recommendations":[{"id":"aws|acct|ec2|us-east-1|m5.large||1|all-upfront","provider":"aws","service":"ec2","region":"us-east-1","resource_type":"m5.large","count":1,"term":1,"payment":"no-upfront","upfront_cost":3000,"savings":10}]}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + require.NotNil(t, saved) + require.Len(t, saved.Recommendations, 1) + assert.Equal(t, "aws|acct|ec2|us-east-1|m5.large||1|no-upfront", saved.Recommendations[0].ID) + assert.Equal(t, 0.0, saved.Recommendations[0].UpfrontCost) + require.NotNil(t, saved.Recommendations[0].MonthlyCost) + assert.Equal(t, 100.0, *saved.Recommendations[0].MonthlyCost) + resultMap := result.(map[string]any) + assert.Equal(t, 0.0, resultMap["total_upfront_cost"]) +} + +// TestHandler_executePurchase_SavingsPlanCountMustMatchStored: a Savings Plan +// rec is priced by hourly commitment, not by count; a count mismatch is +// refused rather than scaled. +func TestHandler_executePurchase_SavingsPlanCountMustMatchStored(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + // Registered .Maybe(): see TestHandler_executePurchase_UnknownRecommendationRefused. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: string(common.ServiceSavingsPlansCompute), Count: 1, Term: 1, Payment: "all-upfront", + UpfrontCost: 0, MonthlyCost: float64Ptr(200), + }) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"rec-1","provider":"aws","service":"savings-plans-compute","count":2,"term":1,"payment":"all-upfront","upfront_cost":0,"savings":10}]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "savings plan count") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestPriceFromStored_ScalesByCountAndKeepsStoredIdentity is a pure unit +// test of priceFromStored's scaling and field-provenance contract. +func TestPriceFromStored_ScalesByCountAndKeepsStoredIdentity(t *testing.T) { + stored := config.RecommendationRecord{ + ID: "stored-id", Provider: "aws", Service: "ec2", Count: 4, + UpfrontCost: 1000, Savings: 100, MonthlyCost: nil, OnDemandCost: float64Ptr(2000), + Details: json.RawMessage(`{"platform":"Linux/UNIX"}`), + } + + t.Run("ratio 0.5 with nil MonthlyCost stays nil", func(t *testing.T) { + req := config.RecommendationRecord{Count: 2, RecommendedCount: 4, Selected: true} + out, err := priceFromStored(&req, &stored, 0) + require.NoError(t, err) + assert.Equal(t, 500.0, out.UpfrontCost) + assert.Equal(t, 50.0, out.Savings) + assert.Nil(t, out.MonthlyCost) + require.NotNil(t, out.OnDemandCost) + assert.Equal(t, 1000.0, *out.OnDemandCost) + }) + + t.Run("ratio 1 reproduces stored values exactly", func(t *testing.T) { + req := config.RecommendationRecord{Count: 4, RecommendedCount: 4, Selected: true} + out, err := priceFromStored(&req, &stored, 0) + require.NoError(t, err) + assert.Equal(t, stored.UpfrontCost, out.UpfrontCost) + assert.Equal(t, stored.Savings, out.Savings) + require.NotNil(t, out.OnDemandCost) + assert.Equal(t, *stored.OnDemandCost, *out.OnDemandCost) + }) + + t.Run("Selected and RecommendedCount come from the request", func(t *testing.T) { + req := config.RecommendationRecord{Count: 4, RecommendedCount: 8, Selected: false} + out, err := priceFromStored(&req, &stored, 0) + require.NoError(t, err) + assert.Equal(t, 8, out.RecommendedCount) + assert.False(t, out.Selected) + }) + + t.Run("ID, Details, Purchased, PurchaseID and Error come from stored even when the request sets Purchased", func(t *testing.T) { + req := config.RecommendationRecord{ + Count: 4, RecommendedCount: 4, Selected: true, + Purchased: true, PurchaseID: "req-purchase-id", Error: "req-error", + } + out, err := priceFromStored(&req, &stored, 0) + require.NoError(t, err) + assert.Equal(t, "stored-id", out.ID) + assert.Equal(t, string(stored.Details), string(out.Details)) + assert.False(t, out.Purchased, "Purchased must come from the stored row, not the request") + assert.Empty(t, out.PurchaseID) + assert.Empty(t, out.Error) + }) +} + +// TestRecIdentityKey_TupleSemantics pins the identity-tuple contract: +// nil and "" CloudAccountID collapse to the same key, provider/payment case +// is folded, and every other component change yields a different key. +func TestRecIdentityKey_TupleSemantics(t *testing.T) { + base := config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Engine: "mysql", Term: 3, Payment: "all-upfront", + } + + t.Run("nil and empty CloudAccountID collapse to the same key", func(t *testing.T) { + empty := "" + withNil := base + withEmpty := base + withEmpty.CloudAccountID = &empty + assert.Equal(t, recIdentityKey(&withNil), recIdentityKey(&withEmpty)) + }) + + t.Run("provider and payment case is folded", func(t *testing.T) { + upper := base + upper.Provider = "AWS" + upper.Payment = "ALL-UPFRONT" + assert.Equal(t, recIdentityKey(&base), recIdentityKey(&upper)) + }) + + t.Run("a different payment yields a different key", func(t *testing.T) { + other := base + other.Payment = "no-upfront" + assert.NotEqual(t, recIdentityKey(&base), recIdentityKey(&other)) + }) + + t.Run("a different term yields a different key", func(t *testing.T) { + other := base + other.Term = 1 + assert.NotEqual(t, recIdentityKey(&base), recIdentityKey(&other)) + }) + + t.Run("a different engine yields a different key", func(t *testing.T) { + other := base + other.Engine = "postgres" + assert.NotEqual(t, recIdentityKey(&base), recIdentityKey(&other)) + }) + + t.Run("a different account yields a different key", func(t *testing.T) { + acctA := "acct-a" + acctB := "acct-b" + withA := base + withA.CloudAccountID = &acctA + withB := base + withB.CloudAccountID = &acctB + assert.NotEqual(t, recIdentityKey(&withA), recIdentityKey(&withB)) + }) +} From 4ce750f7efb2658d031491acb1274ec184d4647d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 06:16:50 +0200 Subject: [PATCH 2/4] fix(api): close #1905 review gaps - handler test, dup-key guard, doc accuracy An adversarial review of the stored-price purchase-cap fix found three low-severity gaps, all addressed here without changing the fix's behaviour: - The account component of recIdentityKey was only proven by a pure unit test, not by any handler test, so a regression there could still pass end to end through the real request/scope/pricing pipeline. Added TestHandler_executePurchase_CrossAccountMismatchRefused: stored recommendations exist under one cloud account, the request claims the same resource under a different account, and the handler must refuse with 409 before persisting or contacting the provider. - loadStoredRecommendationIndex silently kept whichever stored row won a map-key collision. The comment claimed migration 000043's unique index rules this out, but that index is case-sensitive on provider and payment while recIdentityKey folds their case, so two rows differing only in case would collide (unreachable today since the scheduler always writes lowercase, but not guaranteed by the schema). This is a money path, so a collision is now refused with an error naming the key instead of picking a row. TestHandler_executePurchase_Success needed a fixture fix: its two stored rows shared one identity tuple and relied on the prior silent-overwrite behaviour to add up to the right total, which the new guard correctly rejects; giving each row a distinct resource_type keeps the same expected totals under a fixture the real store could actually hold. - The OpenAPI description under-listed the fields replaced from the stored recommendation (missing on_demand_cost, purchased, purchase_id and error), reworded to match what priceFromStored actually does. Closes #1905 Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- internal/api/handler_purchases_test.go | 12 ++-- internal/api/openapi.yaml | 10 ++-- internal/api/purchase_pricing.go | 15 ++++- internal/api/purchase_pricing_test.go | 78 ++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 10 deletions(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index ae3c345f8..08c3e32e6 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2202,10 +2202,14 @@ func TestHandler_executePurchase_Success(t *testing.T) { // The #644 idempotency lookup queries pending executions before creating. // No prior pending row → not a duplicate → proceeds to create. mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) - // #1905: recs are now priced from the stored recommendation set. + // #1905: recs are now priced from the stored recommendation set. The two + // rows need distinct identity tuples (ResourceType here) — the real store + // can never hold two rows under the same tuple (migration 000043's unique + // index), and an identical-tuple fixture would trip the loadStoredRecommendationIndex + // duplicate-key guard. expectStoredRecs(mockStore, - config.RecommendationRecord{Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50}, - config.RecommendationRecord{Provider: "aws", Service: "ec2", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 200, Savings: 100}, + config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50}, + config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.xlarge", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 200, Savings: 100}, ) handler := &Handler{config: mockStore, auth: mockAuth} @@ -2214,7 +2218,7 @@ func TestHandler_executePurchase_Success(t *testing.T) { Headers: map[string]string{ "Authorization": "Bearer admin-token", }, - Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}, {"id": "rec-2", "provider": "aws", "service": "ec2", "count": 2, "term": 1, "payment": "all-upfront", "upfront_cost": 200.0, "savings": 100.0}]}`, + Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "resource_type": "m5.large", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}, {"id": "rec-2", "provider": "aws", "service": "ec2", "resource_type": "m5.xlarge", "count": 2, "term": 1, "payment": "all-upfront", "upfront_cost": 200.0, "savings": 100.0}]}`, } result, err := handler.executePurchase(ctx, req) require.NoError(t, err) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 487af99d2..9e6f85e49 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2853,10 +2853,12 @@ components: description: > Each item must match a recommendation in the current recommendation set on (provider, cloud_account_id, service, region, resource_type, engine, term, - payment). Only count, recommended_count and selected are honoured; id, details, - upfront_cost, monthly_cost and savings are replaced server-side from the stored - recommendation scaled by count, and the spend-cap check, execution record and - approval email use those values. Savings plan items must keep the recommended count. + payment). Only count, recommended_count and selected are honoured; every other + field (including id, details, upfront_cost, monthly_cost, savings, + on_demand_cost, purchased, purchase_id and error) is replaced server-side from + the stored recommendation, with the cost fields scaled by count. The spend-cap + check, execution record and approval email use those replaced values. Savings + plan items must keep the recommended count. ExecutePurchaseResponse: type: object diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 9b1298834..c32089a2d 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -56,7 +56,14 @@ func (h *Handler) priceRecommendationsFromStore(ctx context.Context, recs []conf // loadStoredRecommendationIndex reads the stored rows for every provider in // the batch (one query per distinct provider, at most three) and indexes // them by identity tuple. The store's unique index on the same tuple -// (migration 000043) guarantees one row per key. +// (migration 000043) guarantees one row per key only when provider and +// payment are byte-identical: the index is case-sensitive on both columns, +// while recIdentityKey folds their case, so two rows differing only in case +// would collide here (unreachable today because the scheduler always writes +// lowercase, but not guaranteed by the index itself). Rather than silently +// picking whichever row wins the map insert, a collision is refused: this is +// a money path, and the caller (priceRecommendationsFromStore) must never +// price a purchase off an arbitrarily chosen row. func (h *Handler) loadStoredRecommendationIndex(ctx context.Context, recs []config.RecommendationRecord) (map[string]config.RecommendationRecord, error) { index := make(map[string]config.RecommendationRecord) seen := make(map[string]bool) @@ -71,7 +78,11 @@ func (h *Handler) loadStoredRecommendationIndex(ctx context.Context, recs []conf return nil, fmt.Errorf("load stored recommendations for %s: %w", provider, err) } for j := range rows { - index[recIdentityKey(&rows[j])] = rows[j] + key := recIdentityKey(&rows[j]) + if _, dup := index[key]; dup { + return nil, fmt.Errorf("stored recommendations for %s contain more than one row for identity key %q", provider, key) + } + index[key] = rows[j] } } return index, nil diff --git a/internal/api/purchase_pricing_test.go b/internal/api/purchase_pricing_test.go index 1ada67dbe..d43fed38d 100644 --- a/internal/api/purchase_pricing_test.go +++ b/internal/api/purchase_pricing_test.go @@ -229,6 +229,56 @@ func TestHandler_executePurchase_UnknownRecommendationRefused(t *testing.T) { mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } +// TestHandler_executePurchase_CrossAccountMismatchRefused is a handler-level +// companion to TestRecIdentityKey_TupleSemantics' "a different account +// yields a different key" subtest: that pure unit test proves recIdentityKey +// includes the account, but not that the scope check and the key agree end +// to end, through the real handler, on what "account" means (a mutation +// that dropped CloudAccountID from recIdentityKey would still pass an +// unrestricted admin session's scope check). Stored recommendations exist +// only under account A; the request claims the identical resource under +// account B. The mismatch must be refused with 409 before anything is +// persisted or the provider is contacted. +func TestHandler_executePurchase_CrossAccountMismatchRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockPurchase.AssertExpectations(t) }) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + + // Registered .Maybe(): see TestHandler_executePurchase_UnknownRecommendationRefused. + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + accountA := "111111111111" + expectStoredRecs(mockStore, config.RecommendationRecord{ + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + CloudAccountID: &accountA, Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, + }) + + handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations":[{"id":"rec-1","provider":"aws","service":"ec2","region":"us-east-1","resource_type":"m5.large","cloud_account_id":"222222222222","count":1,"term":1,"payment":"all-upfront","upfront_cost":100,"savings":10}]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), "not in the current recommendation set") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + // TestHandler_executePurchase_StoredRecWithoutPriceRefused: a matched stored // row that carries no usable price is refused with 409. func TestHandler_executePurchase_StoredRecWithoutPriceRefused(t *testing.T) { @@ -480,6 +530,34 @@ func TestPriceFromStored_ScalesByCountAndKeepsStoredIdentity(t *testing.T) { }) } +// TestLoadStoredRecommendationIndex_CaseFoldCollisionRefused: the store's +// unique index on the identity tuple (migration 000043) is case-sensitive on +// provider and payment, while recIdentityKey folds their case. Two stored +// rows differing only in case therefore collide under the fold even though +// the index allowed both rows to exist; the index build must refuse rather +// than silently keep whichever row inserted last (unreachable today because +// the scheduler always writes lowercase, but this is a money path and must +// fail loud rather than pick a row on a broken assumption). +func TestLoadStoredRecommendationIndex_CaseFoldCollisionRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockStore.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "aws"}).Return([]config.RecommendationRecord{ + {ID: "lower-id", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: 100}, + {ID: "upper-id", Provider: "AWS", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Term: 1, Payment: "ALL-UPFRONT", Count: 1, UpfrontCost: 200}, + }, nil) + + handler := &Handler{config: mockStore} + _, err := handler.loadStoredRecommendationIndex(ctx, []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Term: 1, Payment: "all-upfront"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "more than one row for identity key") + _, isClient := IsClientError(err) + assert.False(t, isClient, "an index invariant violation is a server-side bug, not a client error") +} + // TestRecIdentityKey_TupleSemantics pins the identity-tuple contract: // nil and "" CloudAccountID collapse to the same key, provider/payment case // is folded, and every other component change yields a different key. From 78358f39c25f5102c2cf7023802744c742d365d8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 06:45:41 +0200 Subject: [PATCH 3/4] fix(api): address CodeRabbit findings on #2073 CodeRabbit flagged three gaps in the #1905 stored-price purchase cap fix: - TestHandler_executePurchase_NegativeSavings and TestHandler_executePurchase_ExceedsMaxAmount submitted client values that were themselves invalid (negative savings, over-cap upfront cost), so both tests would still pass if the handler validated the client's numbers instead of the stored recommendation's. Making the client's values valid while the stored row keeps the invalid value proves the stored value is what actually governs. - The execute-purchase 409 response referenced the shared Conflict component, which documents only the idempotency-claim case. This PR added four more 409 causes (unmatched recommendation, duplicate identity key, unpriceable stored row, Savings Plan count mismatch), so the operation now gets its own 409 description covering both conflict families. - RecommendationRecord was missing cloud_account_id and recommended_count, both of which the stored-recommendation match now depends on, so a generated client could never construct a valid account-scoped request. Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- internal/api/handler_purchases_test.go | 15 +++++++--- internal/api/openapi.yaml | 40 +++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 08c3e32e6..8d96ae34d 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2337,7 +2337,10 @@ func TestHandler_executePurchase_NegativeSavings(t *testing.T) { mockAuth.grantAdminPurchaser() // #1905: pricing passes (stored upfront/count are usable); the negative // savings on the stored row still trips the validateAndTotalRecommendations - // guard the same as before. + // guard the same as before. The request itself carries a VALID savings + // value (50.0) so this only fails because the stored row's savings (-50) + // governs, not because the client's own number happens to be negative + // too (CodeRabbit finding on #2073). expectStoredRecs(mockStore, config.RecommendationRecord{ Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: -50, }) @@ -2348,7 +2351,7 @@ func TestHandler_executePurchase_NegativeSavings(t *testing.T) { Headers: map[string]string{ "Authorization": "Bearer admin-token", }, - Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": -50.0}]}`, + Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`, } result, err := handler.executePurchase(ctx, req) assert.Error(t, err) @@ -2405,7 +2408,11 @@ func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdminPurchaser() - // #1905: the $10M sanity guard now fires against the stored cost. + // #1905: the $10M sanity guard now fires against the stored cost. The + // request itself carries a VALID upfront_cost (100.0, well under the + // sanity limit) so this only fails because the stored row's cost + // ($15M) governs, not because the client's own number also exceeds + // the limit (CodeRabbit finding on #2073). expectStoredRecs(mockStore, config.RecommendationRecord{ Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 15_000_000, Savings: 50, }) @@ -2416,7 +2423,7 @@ func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) { Headers: map[string]string{ "Authorization": "Bearer admin-token", }, - Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 15000000.0, "savings": 50.0}]}`, + Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`, } result, err := handler.executePurchase(ctx, req) assert.Error(t, err) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 9e6f85e49..c35323808 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -467,7 +467,25 @@ paths: '403': $ref: '#/components/responses/Forbidden' '409': - $ref: '#/components/responses/Conflict' + description: > + One of two conflict families. Idempotency: an identical submit + already holds the idempotency claim, so THIS request was not + executed (see the shared Conflict response for how to interpret + that case). Stored-recommendation conflict (#1905): a + recommendation in the request does not match any row in the + current stored recommendation set (it was purchased, superseded, + or never existed), two requested recommendations resolve to the + same identity key, a matched stored row has no usable price to + derive the spend cap from, or a Savings Plan's requested count + does not equal the stored recommended count. Recovery differs by + cause: for the idempotency case, verify the earlier submit's + outcome with the provider before resubmitting; for a + stored-recommendation conflict, refresh the recommendation set + and resubmit with the current data. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' /api/purchases/approve/{id}: parameters: @@ -2622,8 +2640,28 @@ components: type: string engine: type: string + cloud_account_id: + type: string + nullable: true + description: > + The cloud account this recommendation belongs to. Matched + against the stored recommendation set together with provider, + service, region, resource_type, engine, term and payment + (#1905); an account-scoped recommendation cannot be resolved + without it. Null or omitted only for recommendations that are + not tied to a specific account. count: type: integer + recommended_count: + type: integer + description: > + The pre-scaling count originally recommended, before any + Capacity % scaling was applied to produce count. Optional: + omitted or 0 means "not supplied" and the capacity-consistency + check is skipped for this recommendation. For a Savings Plan, + the stored value of this field is also the count that count + must equal, since Savings Plans are priced by hourly commitment + rather than by count. term: type: integer payment: From 98217391f7594eacb1d460f30265cc55c63eb8df Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 07:42:57 +0200 Subject: [PATCH 4/4] docs(api): note that a duplicate stored identity key also returns 409 The 409 description listed duplicate identities in the request but not in the stored set. loadStoredRecommendationIndex refuses two stored rows that share an identity key rather than picking one arbitrarily, and that fires independently of how many recommendations the request carries, including for a single one. The distinction matters to a client: a duplicate in the request is fixed by changing the selection, while a duplicate in the stored set is not the caller's to fix and calls for refreshing the recommendation set or an operator repairing the store. Found by CodeRabbit reviewing 78358f39c. Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- internal/api/openapi.yaml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index c35323808..ca2dcde5f 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -475,9 +475,12 @@ paths: recommendation in the request does not match any row in the current stored recommendation set (it was purchased, superseded, or never existed), two requested recommendations resolve to the - same identity key, a matched stored row has no usable price to - derive the spend cap from, or a Savings Plan's requested count - does not equal the stored recommended count. Recovery differs by + same identity key, two rows in the stored set share one identity + key (which is refused rather than resolved arbitrarily, and can + occur even for a single-recommendation request), a matched stored + row has no usable price to derive the spend cap from, or a + Savings Plan's requested count does not equal the stored + recommended count. Recovery differs by cause: for the idempotency case, verify the earlier submit's outcome with the provider before resubmitting; for a stored-recommendation conflict, refresh the recommendation set