From e393052b3b276f8ef41b4a0cdab8a7e5feb9a5f7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 23 Jul 2026 20:37:09 +0200 Subject: [PATCH 1/9] fix(azure): coerce web partial-upfront to monthly, not upfront The web/API purchase path coerced Azure partial-upfront payment options to upfront before validation. Since PR #1495 wires Azure billingPlan directly from the normalized payment option, that coercion silently billed an all-upfront schedule the caller never chose. Coerce to monthly (no-upfront, CUDly's default Azure schedule) instead, so the rec still survives validation without flipping the billing schedule. Closes #1503 --- internal/api/handler_purchases_guards_test.go | 73 ++++++++++--------- internal/config/validation.go | 29 +++++--- internal/config/validation_test.go | 2 +- 3 files changed, 61 insertions(+), 43 deletions(-) diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index 400af8ce5..3d4be7d91 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -39,75 +39,79 @@ func TestValidatePurchaseRecommendation(t *testing.T) { return r } tests := []struct { - name string - rec config.RecommendationRecord - wantError bool + name string + rec config.RecommendationRecord + wantError bool + wantPayment string // when non-empty, asserted against rec.Payment after a successful call }{ // --- AWS canonical set --- - {"valid aws all-upfront 3y", validRec(), false}, - {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false}, - {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false}, - {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true}, - {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true}, + {"valid aws all-upfront 3y", validRec(), false, ""}, + {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false, ""}, + {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false, ""}, + {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true, ""}, + {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true, ""}, // --- Azure canonical set --- - {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false}, - {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false}, + {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false, ""}, + {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false, ""}, // Legacy AWS-style tokens on Azure are normalized to Azure-canonical before validation. {"azure accepts legacy all-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "all-upfront" - }), false}, + }), false, "upfront"}, {"azure accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "no-upfront" - }), false}, - {"azure accepts legacy partial-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { + }), false, "monthly"}, + // partial-upfront has no Azure equivalent; it coerces to monthly (the + // no-upfront default), never to upfront, so the caller never gets + // silently billed an all-upfront schedule it did not choose (#1503). + {"azure accepts legacy partial-upfront (coerced to monthly, not upfront)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "partial-upfront" - }), false}, + }), false, "monthly"}, {"azure rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "foo" - }), true}, + }), true, ""}, // --- GCP canonical set (monthly-only) --- - {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false}, + {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false, ""}, // Legacy tokens on GCP are all normalized to monthly. {"gcp accepts legacy upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "upfront" - }), false}, + }), false, ""}, {"gcp accepts legacy all-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "all-upfront" - }), false}, + }), false, ""}, {"gcp accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "no-upfront" - }), false}, + }), false, ""}, {"gcp rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "foo" - }), true}, + }), true, ""}, // --- General --- - {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false}, - {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true}, - {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true}, - {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true}, - {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true}, + {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false, ""}, + {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true, ""}, + {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true, ""}, + {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true, ""}, + {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true, ""}, {"negative monthly cost rejected", mutate(func(r *config.RecommendationRecord) { m := -1.0 r.MonthlyCost = &m - }), true}, - {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false}, + }), true, ""}, + {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false, ""}, {"zero monthly cost accepted", mutate(func(r *config.RecommendationRecord) { m := 0.0 r.MonthlyCost = &m - }), false}, - {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true}, - {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true}, - {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true}, - {"all provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "all" }), true}, - {"unknown provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "ibm" }), true}, + }), false, ""}, + {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true, ""}, + {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true, ""}, + {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true, ""}, + {"all provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "all" }), true, ""}, + {"unknown provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "ibm" }), true, ""}, } for _, tt := range tests { tt := tt @@ -119,6 +123,9 @@ func TestValidatePurchaseRecommendation(t *testing.T) { require.Error(t, err) } else { require.NoError(t, err) + if tt.wantPayment != "" { + assert.Equal(t, tt.wantPayment, rec.Payment) + } } }) } diff --git a/internal/config/validation.go b/internal/config/validation.go index 2f5d80677..8aecad761 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -93,8 +93,13 @@ func validPaymentOptionsFor(provider string) []string { // // - AWS : passthrough (AWS already speaks the three-tier set). // - Azure: all-upfront → upfront, no-upfront → monthly, -// partial-upfront → upfront (no semantic equivalent — coerce to the -// all-upfront tier rather than drop the rec; caller may log). +// partial-upfront → monthly (no semantic equivalent — coerce to the +// no-upfront tier, CUDly's default billing schedule for Azure, rather +// than drop the rec; caller may log). Coercing to upfront here would +// silently bill an all-upfront schedule the caller never chose, since +// the web/API path wires billingPlan directly from this normalized +// payment option (see providers/azure billingPlan construction); +// landing on monthly keeps the rec on the default schedule instead. // - GCP : all-upfront → monthly, no-upfront → monthly, // partial-upfront → monthly, upfront → monthly (GCP CUDs are // inherently monthly-billed — every non-monthly token collapses to the @@ -106,9 +111,11 @@ func validPaymentOptionsFor(provider string) []string { // // The partial-upfront coercion is deliberate on both Azure and GCP: dropping // the rec would be a silent data loss for the user, while coercing to the -// closest billing model the provider offers preserves the rec. The caller is -// expected to log a warning when raw != canonical so an operator notices the -// upstream input bug. +// closest billing model the provider offers preserves the rec without +// changing the money the caller committed to (Azure's no-upfront/monthly +// schedule bills the same total as upfront, just spread monthly). The caller +// is expected to log a warning when raw != canonical so an operator notices +// the upstream input bug. // // Empty raw passes through as ("", true) — callers that distinguish "unset" // from "invalid" can check the returned bool only when raw is non-empty. @@ -146,12 +153,16 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { case "azure": // Azure reservations model both billing plans; coerce AWS-style tokens // to the Azure-canonical spelling. partial-upfront has no Azure - // equivalent — coerce to the all-upfront tier so the rec survives - // validation rather than dropping silently (caller WARN-logs). + // equivalent — coerce to the no-upfront (monthly) tier, CUDly's + // default billing schedule, so the rec survives validation rather + // than dropping silently (caller WARN-logs). Coercing to "upfront" + // would silently bill an upfront schedule the caller never chose, + // since the web/API path wires Azure's billingPlan straight from + // this normalized payment option. switch raw { - case "all-upfront", "partial-upfront": + case "all-upfront": return "upfront", true - case "no-upfront": + case "no-upfront", "partial-upfront": return "monthly", true } case "gcp": diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 94aa7ad02..0237a9bfd 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -986,7 +986,7 @@ func TestNormalizePaymentOption(t *testing.T) { // Azure: AWS-style aliases coerced to canonical. {"azure all-upfront → upfront", "azure", "all-upfront", "upfront", true}, {"azure no-upfront → monthly", "azure", "no-upfront", "monthly", true}, - {"azure partial-upfront → upfront (nearest)", "azure", "partial-upfront", "upfront", true}, + {"azure partial-upfront → monthly (default, not upfront)", "azure", "partial-upfront", "monthly", true}, // GCP: canonical passthrough (monthly-only — every non-monthly token // collapses to monthly because GCP CUDs only model one billing plan). From ca41328b23ffdf3c85b85f446dc6e573b12d67c0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 26 Jul 2026 01:43:08 +0200 Subject: [PATCH 2/9] docs(azure): correct payment-option mechanism comment (cost split, not billingPlan) The comment on NormalizePaymentOption/crossProviderPaymentAlias said the web/API path wires Azure's billingPlan directly from the normalized payment option. buildReservationBody in providers/azure/services/compute/client.go never emits a billingPlan field for VM reservations; the payment option actually drives the upfront-vs-monthly cost split in that file's GetOfferingDetails. No behavior change, comment accuracy only. --- internal/config/validation.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index 8aecad761..23af36e2f 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -96,10 +96,12 @@ func validPaymentOptionsFor(provider string) []string { // partial-upfront → monthly (no semantic equivalent — coerce to the // no-upfront tier, CUDly's default billing schedule for Azure, rather // than drop the rec; caller may log). Coercing to upfront here would -// silently bill an all-upfront schedule the caller never chose, since -// the web/API path wires billingPlan directly from this normalized -// payment option (see providers/azure billingPlan construction); -// landing on monthly keeps the rec on the default schedule instead. +// silently bill an all-upfront schedule the caller never chose: this +// normalized payment option drives the upfront-vs-monthly cost split in +// providers/azure/services/compute/client.go's GetOfferingDetails +// (the PaymentOption switch that allocates upfrontCost vs +// recurringCost), not a billingPlan request field; landing on monthly +// keeps the rec on the default schedule instead. // - GCP : all-upfront → monthly, no-upfront → monthly, // partial-upfront → monthly, upfront → monthly (GCP CUDs are // inherently monthly-billed — every non-monthly token collapses to the @@ -156,9 +158,12 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { // equivalent — coerce to the no-upfront (monthly) tier, CUDly's // default billing schedule, so the rec survives validation rather // than dropping silently (caller WARN-logs). Coercing to "upfront" - // would silently bill an upfront schedule the caller never chose, - // since the web/API path wires Azure's billingPlan straight from - // this normalized payment option. + // would silently bill an upfront schedule the caller never chose: + // the web/API path's normalized payment option drives the + // upfront-vs-monthly cost split in + // providers/azure/services/compute/client.go's + // GetOfferingDetails (the PaymentOption switch), not a + // billingPlan request field. switch raw { case "all-upfront": return "upfront", true From 3f150ab84beaebffa05bdcb6e3dc4b741814e8b1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 26 Jul 2026 02:20:17 +0200 Subject: [PATCH 3/9] fix(api): warn on payment-option normalization coercion validatePurchaseRecommendation silently overwrote rec.Payment with the NormalizePaymentOption result, contradicting the doc comment that callers are expected to WARN when a raw payment option is coerced to its canonical form (e.g. azure partial-upfront -> monthly). Log the transition via pkg/logging, matching the pattern already used by scheduler.convertRecommendations, so the coercion is auditable on this money-affecting field. The coercion itself is unchanged. --- internal/api/handler_purchases_guards_test.go | 54 +++++++++++++++++++ internal/api/validation.go | 12 ++++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index 3d4be7d91..10fac4720 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -147,6 +147,60 @@ func TestValidatePurchaseRecommendation_ErrorMessage(t *testing.T) { assert.Contains(t, err.Error(), "monthly") } +// TestValidatePurchaseRecommendation_NormalizationWarning is a regression test +// for #1504: the doc comment on config.NormalizePaymentOption +// (internal/config/validation.go) states that the caller is expected to WARN +// when a raw payment-option token differs from its normalized canonical form, +// so an operator can audit a money-affecting coercion. validatePurchaseRecommendation +// is that caller; it must emit a WARN log when normalization actually changes +// the value (raw != canonical). This test captures the default logger's +// output via captureDefaultLog (defined in handler_accounts_test.go, package +// api) and asserts the log line is emitted with the provider, service, raw +// and canonical values, and rec index. +// +// Not run with t.Parallel(): captureDefaultLog mutates the shared default +// logger's output, which would race against other SetOutput-using tests if +// parallelized (see TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak). +func TestValidatePurchaseRecommendation_NormalizationWarning(t *testing.T) { + logBuf := captureDefaultLog(t) + + // Azure partial-upfront has no direct Azure equivalent; NormalizePaymentOption + // coerces it to "monthly" (#1503). This is a real raw != canonical + // transition and must be logged. + rec := validRec() + rec.Provider = "azure" + rec.Payment = "partial-upfront" + err := validatePurchaseRecommendation(&rec, 7) + require.NoError(t, err) + // The money-affecting coercion itself must be unchanged by adding the log. + assert.Equal(t, "monthly", rec.Payment, "coercion behavior must stay partial-upfront -> monthly, not upfront") + + logged := logBuf.String() + assert.Contains(t, logged, "[WARN]") + assert.Contains(t, logged, "rec 7 (azure/ec2)", "warning must identify the rec index, provider and service for audit") + assert.Contains(t, logged, `raw="partial-upfront"`) + assert.Contains(t, logged, `canonical="monthly"`) +} + +// TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical guards that +// an already-canonical payment option (no real normalization) does not emit a +// WARN log: only an actual raw->canonical transition should be observable, so +// operators aren't flooded with noise on the common case. +// +// Not run with t.Parallel(); see TestValidatePurchaseRecommendation_NormalizationWarning. +func TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical(t *testing.T) { + logBuf := captureDefaultLog(t) + + rec := validRec() + rec.Provider = "azure" + rec.Payment = "monthly" // already canonical for azure; no normalization occurs + err := validatePurchaseRecommendation(&rec, 0) + require.NoError(t, err) + assert.Equal(t, "monthly", rec.Payment) + + assert.Empty(t, logBuf.String(), "no normalization occurred, so nothing should be logged") +} + // The per-rec #643 validation is wired into the web execute boundary // (validateExecutePurchaseRequest), NOT the shared validateAndTotalRecommendations // which the retry path also calls with replayed recs. This test pins that diff --git a/internal/api/validation.go b/internal/api/validation.go index 9b5fdf34b..6f5b52b9b 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-lambda-go/events" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/logging" ) // Security constants. @@ -564,8 +565,17 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e payment := strings.ToLower(strings.TrimSpace(rec.Payment)) // Coerce any legacy/cross-provider alias before the whitelist check so // that callers using old AWS-style tokens are transparently redirected to - // the canonical token for the target provider. + // the canonical token for the target provider. WARN when a real + // normalization occurs (raw != canonical) so an operator can audit the + // coercion of this money-affecting field, matching the WARN contract + // documented on config.NormalizePaymentOption and mirroring the same + // coerced/uncoerced logging convertRecommendations does at the + // scheduler's emission boundary (internal/scheduler/scheduler.go). if normalized, ok := config.NormalizePaymentOption(provider, payment); ok { + if normalized != payment { + logging.Warnf("validatePurchaseRecommendation: rec %d (%s/%s) payment option normalized: raw=%q canonical=%q", + idx, provider, rec.Service, payment, normalized) + } payment = normalized } if !payments[payment] { From d784a5644046049f49836d8061b5404a69022844 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 12:47:04 +0200 Subject: [PATCH 4/9] docs(config): future-proof Azure payment-coercion comments for #1495 The coercion-rationale comments assert the normalized payment option is "not a billingPlan request field". That is accurate on main today (the reservation purchase body sends no billingPlan), but becomes false once the billingPlan wiring lands (#1495/#1502), and that wiring does not touch this file, so the comment would silently go stale on a money-path rationale. Rephrase to state the forward-compatible fact instead: the wiring maps only upfront/monthly tokens and hard-errors on partial-upfront, which is exactly why normalizing to monthly here keeps web-path Azure purchases valid. Comment-only change; no behavior difference. --- internal/config/validation.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index 23af36e2f..56f94b4ff 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -100,8 +100,10 @@ func validPaymentOptionsFor(provider string) []string { // normalized payment option drives the upfront-vs-monthly cost split in // providers/azure/services/compute/client.go's GetOfferingDetails // (the PaymentOption switch that allocates upfrontCost vs -// recurringCost), not a billingPlan request field; landing on monthly -// keeps the rec on the default schedule instead. +// recurringCost); the reservation-purchase billingPlan wiring being +// added in #1495/#1502 likewise maps only upfront/monthly tokens and +// hard-errors on partial-upfront, so landing on monthly keeps the rec +// on the default schedule instead. // - GCP : all-upfront → monthly, no-upfront → monthly, // partial-upfront → monthly, upfront → monthly (GCP CUDs are // inherently monthly-billed — every non-monthly token collapses to the @@ -162,8 +164,10 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { // the web/API path's normalized payment option drives the // upfront-vs-monthly cost split in // providers/azure/services/compute/client.go's - // GetOfferingDetails (the PaymentOption switch), not a - // billingPlan request field. + // GetOfferingDetails (the PaymentOption switch), and the + // reservation-purchase billingPlan wiring being added in + // #1495/#1502 hard-errors on partial-upfront, so only the + // canonical upfront/monthly tokens survive to the purchase body. switch raw { case "all-upfront": return "upfront", true From 53cbaa1c3db7ab060bacdc1ba34b6e9f236a8f0c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 14:35:17 +0200 Subject: [PATCH 5/9] feat(api): surface payment-option coercions in the purchase response The web execute path silently normalizes cross-provider payment tokens (most notably Azure partial-upfront -> monthly, #1503); until now only operators saw the WARN log while the caller's response gave no hint the billing schedule they requested was changed. Return the coercion to the caller: validatePurchaseRecommendation now also yields a PaymentAdjustment (rec index, provider, service, requested token, applied token, reason) whenever normalization changes the token, and executePurchase attaches the collected list to all three response bodies (approval-pending, duplicate-collapse, direct-execute) under payment_adjustments. The key is omitted entirely when every option was already canonical, so existing clients are unaffected on the common case, and the coercion policy itself (config.NormalizePaymentOption) plus the operator WARN log are unchanged. Tests: the guards table now cross-checks every coerced row surfaces a matching adjustment and every canonical row surfaces none; new field-level unit test for the Azure partial-upfront case; new handler-level tests assert the response body carries the adjustment for a mixed canonical+coerced batch (verified to fail with the attach stubbed out) and omits the key for a canonical-only batch, both with mock.AssertExpectations registered. frontend PurchaseResult gains the matching optional payment_adjustments field. --- frontend/src/api/types.ts | 16 ++ .../api/executed_notification_flow_test.go | 4 +- internal/api/handler_purchases.go | 78 ++++++--- internal/api/handler_purchases_guards_test.go | 157 ++++++++++++++++-- internal/api/handler_purchases_test.go | 2 +- internal/api/validation.go | 57 ++++++- 6 files changed, 264 insertions(+), 50 deletions(-) diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index ec7dd3786..befcbf653 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -378,6 +378,22 @@ export interface PurchaseResult { // True when the request was handled via the direct-execute path (issue // #289). Absent (undefined) on the standard approval-required flow. direct_execute?: boolean; + // Per-rec payment-option coercion notices (#1503 follow-up). Present only + // when the backend normalized a requested payment option onto a DIFFERENT + // provider-canonical token (e.g. Azure "partial-upfront" -> "monthly"): + // each entry names what was requested, what was actually applied, and why, + // so the UI can tell the user instead of silently changing the billing + // schedule. Absent (never an empty array) when every option was already + // canonical. Mirrors the backend PaymentAdjustment struct + // (internal/api/validation.go). + payment_adjustments?: Array<{ + rec_index: number; + provider: string; + service: string; + requested_payment_option: string; + applied_payment_option: string; + reason: string; + }>; results?: Array<{ recommendation_id: string; status: string; diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 136ffaac9..2d59b2e44 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -295,7 +295,7 @@ func TestExecutedNotification_DirectExecutePath(t *testing.T) { } session := &Session{Email: adminEmail, UserID: "admin-uid"} - result, err := handler.directExecutePurchase(ctx, req, exec, session) + result, err := handler.directExecutePurchase(ctx, req, exec, session, nil) require.NoError(t, err) resultMap := result.(map[string]any) assert.Equal(t, "completed", resultMap["status"]) @@ -336,7 +336,7 @@ func TestExecutedNotification_DirectExecute_NilNotifierNoPanic(t *testing.T) { req := &events.LambdaFunctionURLRequest{} session := &Session{Email: adminEmail, UserID: "admin-uid"} - result, err := handler.directExecutePurchase(ctx, req, exec, session) + result, err := handler.directExecutePurchase(ctx, req, exec, session, nil) require.NoError(t, err) assert.Equal(t, "completed", result.(map[string]any)["status"]) mockPurchase.AssertExpectations(t) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4b9e4efca..2ea40a328 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2025,25 +2025,28 @@ type ExecutePurchaseRequest struct { // validateExecutePurchaseRequest handles the permission check, body parse, // and recommendation-list bounds + scope checks. Extracted so executePurchase -// itself stays linear and under the gocyclo threshold. -func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *events.LambdaFunctionURLRequest) (ExecutePurchaseRequest, *Session, error) { +// itself stays linear and under the gocyclo threshold. The returned +// PaymentAdjustment slice carries any payment-option coercions performed by +// the per-rec validation, for the response to surface to the caller (#1503 +// follow-up); it is empty/nil when every rec was already canonical. +func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *events.LambdaFunctionURLRequest) (ExecutePurchaseRequest, *Session, []PaymentAdjustment, error) { session, err := h.requirePermission(ctx, req, "execute", "purchases") if err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } var execReq ExecutePurchaseRequest if err := json.Unmarshal([]byte(req.Body), &execReq); err != nil { - return ExecutePurchaseRequest{}, nil, NewClientError(400, "invalid request body") + return ExecutePurchaseRequest{}, nil, nil, NewClientError(400, "invalid request body") } const maxRecommendations = 1000 if len(execReq.Recommendations) == 0 { - return ExecutePurchaseRequest{}, nil, NewClientError(400, "no recommendations provided") + return ExecutePurchaseRequest{}, nil, nil, NewClientError(400, "no recommendations provided") } if len(execReq.Recommendations) > maxRecommendations { - return ExecutePurchaseRequest{}, nil, NewClientError(400, fmt.Sprintf("too many recommendations: %d (max %d)", len(execReq.Recommendations), maxRecommendations)) + return ExecutePurchaseRequest{}, nil, nil, NewClientError(400, fmt.Sprintf("too many recommendations: %d (max %d)", len(execReq.Recommendations), maxRecommendations)) } if err := normalizeCapacityPercent(&execReq); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } // Scope: reject the whole request if any recommendation targets an // account outside the session's allowed_accounts. Safer than silently @@ -2052,7 +2055,7 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // Runs before per-rec content validation so an out-of-scope request is // rejected as 403 regardless of the rec's Term/Payment/Count contents. if err := h.validatePurchaseRecommendationScope(ctx, session, execReq.Recommendations); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } // Per-rec Provider/Service/Term/Payment/Count validation at the API // boundary so a malformed client-supplied rec (e.g. Term:7, Payment:"foo", @@ -2060,15 +2063,16 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // execute time (#643). This is scoped to the web execute path only — the // retry path replays recs from an already-validated execution and must // not be re-gated by the same rules. - if err := validateExecutePurchaseRecommendations(execReq.Recommendations); err != nil { - return ExecutePurchaseRequest{}, nil, err + adjustments, recErr := validateExecutePurchaseRecommendations(execReq.Recommendations) + if recErr != nil { + return ExecutePurchaseRequest{}, nil, nil, recErr } // Cross-check the audit-only capacity_percent against the scaled rec // counts so the persisted execution can't claim a capacity that // disagrees with what was actually purchased (#647). Skipped per-rec // when the rec carries no recommended_count. if err := validateCapacityConsistency(execReq.Recommendations, execReq.CapacityPercent); err != nil { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } // Enforce the per-permission Constraints (MaxPurchaseAmount, Providers, // Services, Regions, AccountIDs) configured on the granting @@ -2080,9 +2084,9 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // 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 { - return ExecutePurchaseRequest{}, nil, err + return ExecutePurchaseRequest{}, nil, nil, err } - return execReq, session, nil + return execReq, session, adjustments, nil } // enforcePurchaseConstraints builds the per-recommendation @@ -2199,15 +2203,22 @@ func normalizeCapacityPercent(execReq *ExecutePurchaseRequest) error { // validateExecutePurchaseRecommendations runs the per-rec #643 boundary // validation over every rec in a web execute request, returning the first -// failure. Extracted so validateExecutePurchaseRequest stays under the -// gocyclo threshold. -func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) error { +// failure. On success it also returns the payment-option coercions that +// occurred (nil-free, in rec order) so the response can surface them to the +// caller (#1503 follow-up). Extracted so validateExecutePurchaseRequest stays +// under the gocyclo threshold. +func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) ([]PaymentAdjustment, error) { + var adjustments []PaymentAdjustment for i := range recs { - if err := validatePurchaseRecommendation(&recs[i], i); err != nil { - return err + adjustment, err := validatePurchaseRecommendation(&recs[i], i) + if err != nil { + return nil, err + } + if adjustment != nil { + adjustments = append(adjustments, *adjustment) } } - return nil + return adjustments, nil } // finalizePurchaseStatus flips an execution's stored status to "failed" if @@ -2482,7 +2493,7 @@ func newPendingExecution(req *ExecutePurchaseRequest, totalUpfront, totalSavings } func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - execReq, session, err := h.validateExecutePurchaseRequest(ctx, req) + execReq, session, paymentAdjustments, err := h.validateExecutePurchaseRequest(ctx, req) if err != nil { return nil, err } @@ -2531,7 +2542,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio } if dupExec != nil { logging.Infof("concurrent duplicate purchase submit collapsed to existing execution %s", dupExec.ExecutionID) - return buildDuplicatePurchaseResponse(dupExec), nil + return withPaymentAdjustments(buildDuplicatePurchaseResponse(dupExec), paymentAdjustments), nil } // Direct-execute path (issue #289): a session with execute-any or @@ -2546,7 +2557,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio if err := h.authorizeSessionExecuteDirect(ctx, session, creatorID); err != nil { return nil, err } - return h.directExecutePurchase(ctx, req, execution, session) + return h.directExecutePurchase(ctx, req, execution, session, paymentAdjustments) } // Send approval email synchronously so the response can surface the @@ -2557,7 +2568,18 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio emailSent, emailReason, recipient := h.sendPurchaseApprovalEmail(ctx, req, execution, execReq.Recommendations, totalUpfront, totalSavings) status := h.finalizePurchaseStatus(ctx, execution, emailSent, emailReason) - return buildApprovalPendingResponse(executionID, status, len(execReq.Recommendations), totalUpfront, totalSavings, emailSent, emailReason, recipient), nil + return withPaymentAdjustments(buildApprovalPendingResponse(executionID, status, len(execReq.Recommendations), totalUpfront, totalSavings, emailSent, emailReason, recipient), paymentAdjustments), nil +} + +// withPaymentAdjustments attaches the payment-option coercion notices to an +// executePurchase response body under "payment_adjustments" (#1503 follow-up). +// The key is omitted entirely when no coercion happened, so existing clients +// see an unchanged response on the common canonical-input case. +func withPaymentAdjustments(resp map[string]any, adjustments []PaymentAdjustment) map[string]any { + if len(adjustments) > 0 { + resp["payment_adjustments"] = adjustments + } + return resp } // buildApprovalPendingResponse assembles the JSON-serialisable response body @@ -2612,7 +2634,9 @@ func buildApprovalPendingResponse( // This is the only email the direct-execute flow emits -- no approval // email precedes it -- so it is the path where the executed-notification // matters most. -// 4. Return a "completed" status to the caller. +// 4. Return a "completed" status to the caller, carrying any payment-option +// coercion notices (paymentAdjustments, from the request validation) so +// the direct-execute response surfaces them like the approval path does. // // The audit fields are best-effort if ApproveAndExecute's SavePurchaseExecution // races with our pre-call stamp -- but in practice ApproveAndExecute calls @@ -2621,7 +2645,7 @@ func buildApprovalPendingResponse( // TransitionExecutionStatus. The critical audit invariant is that a non-nil // executed_by_user_id always co-occurs with a non-nil pre_approval_skip_reason, // and both are set atomically in the same SavePurchaseExecution call here. -func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session) (any, error) { +func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session, paymentAdjustments []PaymentAdjustment) (any, error) { t0 := time.Now() executionID := execution.ExecutionID logging.Infof("purchase[%s]: directExecutePurchase entry (auth=session)", executionID) @@ -2661,7 +2685,7 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF // guard live inside sendPurchaseExecutedEmail. session.Email is the actor // who direct-executed, matching the actor passed to ApproveAndExecute above. h.sendPurchaseExecutedEmail(ctx, req, execution, session.Email) - return map[string]any{ + return withPaymentAdjustments(map[string]any{ "execution_id": executionID, "status": "completed", "recommendation_count": len(execution.Recommendations), @@ -2669,7 +2693,7 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF "estimated_savings": execution.EstimatedSavings, "direct_execute": true, "message": "Purchase executed immediately (direct-execute permission).", - }, nil + }, paymentAdjustments), nil } // archeraEducationURL returns dashboardBase + "/archera-insurance", or "" when diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index 10fac4720..f130d65d3 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -6,7 +6,9 @@ import ( "testing" "time" + "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/LeanerCloud/CUDly/internal/config" @@ -39,10 +41,14 @@ func TestValidatePurchaseRecommendation(t *testing.T) { return r } tests := []struct { - name string - rec config.RecommendationRecord - wantError bool - wantPayment string // when non-empty, asserted against rec.Payment after a successful call + name string + rec config.RecommendationRecord + wantError bool + // wantPayment is set ONLY on rows whose input token gets coerced: it is + // asserted against rec.Payment after a successful call, and the row must + // also surface a matching PaymentAdjustment. Rows with "" must NOT + // surface an adjustment (canonical passthrough, incl. case-only changes). + wantPayment string }{ // --- AWS canonical set --- {"valid aws all-upfront 3y", validRec(), false, ""}, @@ -79,15 +85,15 @@ func TestValidatePurchaseRecommendation(t *testing.T) { {"gcp accepts legacy upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "upfront" - }), false, ""}, + }), false, "monthly"}, {"gcp accepts legacy all-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "all-upfront" - }), false, ""}, + }), false, "monthly"}, {"gcp accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "no-upfront" - }), false, ""}, + }), false, "monthly"}, {"gcp rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "foo" @@ -118,13 +124,22 @@ func TestValidatePurchaseRecommendation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() rec := tt.rec - err := validatePurchaseRecommendation(&rec, 0) + adjustment, err := validatePurchaseRecommendation(&rec, 0) if tt.wantError { require.Error(t, err) } else { require.NoError(t, err) if tt.wantPayment != "" { assert.Equal(t, tt.wantPayment, rec.Payment) + // wantPayment is only set on rows whose input token gets + // coerced, so each of those must also surface a caller- + // visible PaymentAdjustment consistent with the mutation + // (#1503 follow-up). Field-level assertions live in + // TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment. + require.NotNil(t, adjustment, "coerced payment option must surface a PaymentAdjustment") + assert.Equal(t, tt.wantPayment, adjustment.AppliedPaymentOption) + } else { + assert.Nil(t, adjustment, "no coercion occurred, so no adjustment should be surfaced") } } }) @@ -140,7 +155,7 @@ func TestValidatePurchaseRecommendation_ErrorMessage(t *testing.T) { rec := validRec() rec.Provider = "azure" rec.Payment = "foo" // not in azure canonical set and has no normalization alias - err := validatePurchaseRecommendation(&rec, 0) + _, err := validatePurchaseRecommendation(&rec, 0) require.Error(t, err) assert.Contains(t, err.Error(), "azure") assert.Contains(t, err.Error(), "upfront") @@ -170,7 +185,7 @@ func TestValidatePurchaseRecommendation_NormalizationWarning(t *testing.T) { rec := validRec() rec.Provider = "azure" rec.Payment = "partial-upfront" - err := validatePurchaseRecommendation(&rec, 7) + _, err := validatePurchaseRecommendation(&rec, 7) require.NoError(t, err) // The money-affecting coercion itself must be unchanged by adding the log. assert.Equal(t, "monthly", rec.Payment, "coercion behavior must stay partial-upfront -> monthly, not upfront") @@ -194,13 +209,133 @@ func TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical(t *testing rec := validRec() rec.Provider = "azure" rec.Payment = "monthly" // already canonical for azure; no normalization occurs - err := validatePurchaseRecommendation(&rec, 0) + adjustment, err := validatePurchaseRecommendation(&rec, 0) require.NoError(t, err) + assert.Nil(t, adjustment, "already-canonical payment option must not surface an adjustment") assert.Equal(t, "monthly", rec.Payment) assert.Empty(t, logBuf.String(), "no normalization occurred, so nothing should be logged") } +// TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment pins the +// caller-facing coercion notice (#1503 follow-up): when the web execute path +// normalizes a payment option onto a DIFFERENT provider-canonical token, the +// returned PaymentAdjustment must identify the rec and carry the requested +// token, the applied token, and a reason naming both, so the API response can +// tell the caller what actually happened to this money-affecting field instead +// of only WARN-logging it for operators. +func TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment(t *testing.T) { + t.Parallel() + rec := validRec() + rec.Provider = "azure" + rec.Payment = "partial-upfront" + adjustment, err := validatePurchaseRecommendation(&rec, 3) + require.NoError(t, err) + require.NotNil(t, adjustment, "azure partial-upfront is coerced and must surface an adjustment") + assert.Equal(t, 3, adjustment.RecIndex) + assert.Equal(t, "azure", adjustment.Provider) + assert.Equal(t, rec.Service, adjustment.Service) + assert.Equal(t, "partial-upfront", adjustment.RequestedPaymentOption) + assert.Equal(t, "monthly", adjustment.AppliedPaymentOption) + // The reason must be self-explanatory to the caller: it names the rejected + // token, the applied token, and the provider whose billing model forced it. + assert.Contains(t, adjustment.Reason, `"partial-upfront"`) + assert.Contains(t, adjustment.Reason, `"monthly"`) + assert.Contains(t, adjustment.Reason, "azure") + // The adjustment must agree with the actual mutation applied to the rec: + // applied means applied, not merely advertised. + assert.Equal(t, adjustment.AppliedPaymentOption, rec.Payment) +} + +// TestHandler_executePurchase_SurfacesPaymentAdjustments is the response-level +// regression test for the #1503 follow-up: an Azure partial-upfront purchase +// submitted through the real executePurchase handler must return a +// payment_adjustments entry telling the caller the request was applied as +// monthly; a green validator-level test alone could not prove the notice +// survives to the response body the client actually sees. +func TestHandler_executePurchase_SurfacesPaymentAdjustments(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) + }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + 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) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + // rec 0 is already canonical (azure/monthly); rec 1 carries the #1503 + // partial-upfront token and must be the only rec surfaced. + Body: `{"recommendations": [` + + `{"id": "rec-1", "provider": "azure", "service": "vm", "count": 1, "term": 1, "payment": "monthly", "upfront_cost": 100.0, "savings": 50.0},` + + `{"id": "rec-2", "provider": "azure", "service": "vm", "count": 1, "term": 1, "payment": "partial-upfront", "upfront_cost": 200.0, "savings": 25.0}]}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + + resultMap := result.(map[string]any) + adjustments, ok := resultMap["payment_adjustments"].([]PaymentAdjustment) + require.True(t, ok, "response must carry payment_adjustments when a coercion occurred") + require.Len(t, adjustments, 1, "only the coerced rec must be surfaced") + adj := adjustments[0] + assert.Equal(t, 1, adj.RecIndex, "adjustment must point at the coerced rec's position") + assert.Equal(t, "azure", adj.Provider) + assert.Equal(t, "vm", adj.Service) + assert.Equal(t, "partial-upfront", adj.RequestedPaymentOption) + assert.Equal(t, "monthly", adj.AppliedPaymentOption) + assert.NotEmpty(t, adj.Reason) +} + +// TestHandler_executePurchase_NoAdjustmentsWhenCanonical guards the +// backward-compat contract of the #1503 follow-up: when every payment option +// is already provider-canonical, the payment_adjustments key must be ABSENT +// (not an empty array), so existing clients see a byte-identical response on +// the common case. +func TestHandler_executePurchase_NoAdjustmentsWhenCanonical(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) + }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + 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) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations": [{"id": "rec-1", "provider": "azure", "service": "vm", "count": 1, "term": 1, "payment": "monthly", "upfront_cost": 100.0, "savings": 50.0}]}`, + } + result, err := handler.executePurchase(ctx, req) + require.NoError(t, err) + + resultMap := result.(map[string]any) + assert.NotContains(t, resultMap, "payment_adjustments", + "canonical-only request must not carry the payment_adjustments key at all") +} + // The per-rec #643 validation is wired into the web execute boundary // (validateExecutePurchaseRequest), NOT the shared validateAndTotalRecommendations // which the retry path also calls with replayed recs. This test pins that diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 56da6b542..5878c99c2 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4110,7 +4110,7 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfE realManager := purchase.NewManager(purchase.ManagerConfig{ConfigStore: mockStore}) handler := &Handler{config: mockStore, purchase: realManager} - _, err := handler.directExecutePurchase(ctx, &events.LambdaFunctionURLRequest{}, execution, ownerSession) + _, err := handler.directExecutePurchase(ctx, &events.LambdaFunctionURLRequest{}, execution, ownerSession, nil) require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "expected a clientError") diff --git a/internal/api/validation.go b/internal/api/validation.go index 6f5b52b9b..db7db52ef 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -522,6 +522,29 @@ var purchaseTermWhitelist = map[string]map[int]bool{ "gcp": {1: true, 3: true}, } +// PaymentAdjustment surfaces a payment-option coercion to the API caller +// (follow-up to #1503): when the web execute path normalizes a caller-supplied +// payment option onto a provider-canonical token that DIFFERS from what was +// requested (e.g. Azure "partial-upfront" -> "monthly"), the response carries +// one of these per adjusted rec so the caller sees what they requested, what +// was actually applied, and why, not just the operator WARN log. Purely +// additive visibility: the coercion policy itself lives in +// config.NormalizePaymentOption and is unchanged. +type PaymentAdjustment struct { + // RecIndex is the rec's position in the request's recommendations slice. + RecIndex int `json:"rec_index"` + Provider string `json:"provider"` + Service string `json:"service"` + // RequestedPaymentOption is the caller's token after trim/lowercase only + // (case-only differences are not surfaced as adjustments, matching the + // operator WARN-log semantics). + RequestedPaymentOption string `json:"requested_payment_option"` + // AppliedPaymentOption is the provider-canonical token the purchase will + // actually carry. + AppliedPaymentOption string `json:"applied_payment_option"` + Reason string `json:"reason"` +} + // validatePurchaseRecommendation validates a single client-supplied // recommendation before it reaches the cloud purchase SDK. Unlike the // query-time validateProvider (which permits ""/"all"), the execute path @@ -537,22 +560,26 @@ var purchaseTermWhitelist = map[string]map[int]bool{ // 2. The provider-canonical set (from config.ValidPaymentOptionsByProvider) // rejects anything that has no canonical mapping, with an error that // names the provider and lists the accepted tokens. -func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) error { +// +// When step 1 actually changes the token (raw != canonical), the returned +// *PaymentAdjustment describes the coercion so the response can surface it to +// the caller; nil means the payment option was already canonical. +func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) (*PaymentAdjustment, error) { provider := strings.ToLower(strings.TrimSpace(rec.Provider)) payments := purchasePaymentSet(provider) if payments == nil { - return NewClientError(400, fmt.Sprintf("recommendation %d has invalid provider %q: must be one of aws, azure, gcp", idx, rec.Provider)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has invalid provider %q: must be one of aws, azure, gcp", idx, rec.Provider)) } rec.Provider = provider rec.Service = strings.TrimSpace(rec.Service) if rec.Service == "" { - return NewClientError(400, fmt.Sprintf("recommendation %d is missing a service", idx)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d is missing a service", idx)) } if rec.Count <= 0 { - return NewClientError(400, fmt.Sprintf("recommendation %d has non-positive count: %d", idx, rec.Count)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has non-positive count: %d", idx, rec.Count)) } if !purchaseTermWhitelist[provider][rec.Term] { - return NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider)) } // A negative MonthlyCost would let recTotalCommitment's recurring leg // subtract from the batch's total commitment, offsetting or masking a @@ -560,7 +587,7 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e // review follow-up to #1210). Nil is fine (no recurring charge); only a // present-and-negative value is rejected. if rec.MonthlyCost != nil && *rec.MonthlyCost < 0 { - return NewClientError(400, fmt.Sprintf("recommendation %d has negative monthly cost: %.2f", idx, *rec.MonthlyCost)) + return nil, NewClientError(400, fmt.Sprintf("recommendation %d has negative monthly cost: %.2f", idx, *rec.MonthlyCost)) } payment := strings.ToLower(strings.TrimSpace(rec.Payment)) // Coerce any legacy/cross-provider alias before the whitelist check so @@ -570,23 +597,35 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e // coercion of this money-affecting field, matching the WARN contract // documented on config.NormalizePaymentOption and mirroring the same // coerced/uncoerced logging convertRecommendations does at the - // scheduler's emission boundary (internal/scheduler/scheduler.go). + // scheduler's emission boundary (internal/scheduler/scheduler.go). The + // same transition is also returned as a PaymentAdjustment so the API + // response surfaces it to the caller, not just the operator log. + var adjustment *PaymentAdjustment if normalized, ok := config.NormalizePaymentOption(provider, payment); ok { if normalized != payment { logging.Warnf("validatePurchaseRecommendation: rec %d (%s/%s) payment option normalized: raw=%q canonical=%q", idx, provider, rec.Service, payment, normalized) + adjustment = &PaymentAdjustment{ + RecIndex: idx, + Provider: provider, + Service: rec.Service, + RequestedPaymentOption: payment, + AppliedPaymentOption: normalized, + Reason: fmt.Sprintf("payment option %q is not in %s's supported set (%s); the closest supported option %q was applied", + payment, provider, strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), normalized), + } } payment = normalized } if !payments[payment] { - return NewClientError(400, fmt.Sprintf( + return nil, NewClientError(400, fmt.Sprintf( "invalid payment option for %s service: %q (valid for %s: %s)", provider, rec.Payment, provider, strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), )) } rec.Payment = payment - return nil + return adjustment, nil } // validateCapacityConsistency cross-checks the client-supplied capacity_percent From ac05be990c17ac7f0e40a38a2b3de487301e2779 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 14:35:46 +0200 Subject: [PATCH 6/9] fix(frontend): coerce azure partial-upfront to monthly, not upfront The Go side of #1503 was fixed to map Azure partial-upfront onto "monthly", but normalizePaymentValue in the frontend still mapped it onto "upfront", so the two halves of the web path disagreed. That function decides which option the plan and purchase dropdowns pre-select (plans.ts:1166 on plan edit, plans.ts:1562 when prefilling from a selected commitment), so a rec carrying the legacy partial-upfront token pre-selected "Pay Upfront". Saving from that state submits an already-canonical "upfront", which the backend accepts verbatim, so the backend coercion never gets a chance to run and the user is committed to a full upfront charge they never chose. Azure offers exactly two billing plans and the total cost is identical either way, but the plan cannot be changed after purchase, so landing on monthly is the only direction that cannot surprise the user with an irreversible upfront charge. The jest case that pinned the old expectation is updated and now documents why monthly is required, mirroring the Go-side assertion in TestNormalizePaymentOption. Verified failing before this change and passing after. Refs #1503 --- .../src/__tests__/commitmentOptions.test.ts | 10 ++++++-- frontend/src/commitmentOptions.ts | 23 ++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/commitmentOptions.test.ts b/frontend/src/__tests__/commitmentOptions.test.ts index b9501eede..37ba9c9b1 100644 --- a/frontend/src/__tests__/commitmentOptions.test.ts +++ b/frontend/src/__tests__/commitmentOptions.test.ts @@ -633,8 +633,14 @@ describe('commitmentOptions', () => { expect(normalizePaymentValue('all-upfront', 'azure')).toBe('upfront'); }); - it('should convert partial-upfront to upfront for Azure', () => { - expect(normalizePaymentValue('partial-upfront', 'azure')).toBe('upfront'); + // Regression test for #1503: partial-upfront has no Azure equivalent + // and must land on 'monthly', never 'upfront'. Azure's billing plan is + // immutable after purchase and both plans cost the same total, so + // pre-selecting 'upfront' would hand the user an irreversible full + // upfront charge they never chose. Mirrors the Go-side assertion in + // internal/config/validation_test.go:TestNormalizePaymentOption. + it('should convert partial-upfront to monthly (not upfront) for Azure', () => { + expect(normalizePaymentValue('partial-upfront', 'azure')).toBe('monthly'); }); it('should convert no-upfront to monthly for Azure', () => { diff --git a/frontend/src/commitmentOptions.ts b/frontend/src/commitmentOptions.ts index abdc8c87d..299490678 100644 --- a/frontend/src/commitmentOptions.ts +++ b/frontend/src/commitmentOptions.ts @@ -292,15 +292,32 @@ export async function fetchAndPopulateCommitmentOptions(fetchFn?: FetchLike): Pr } /** - * Map legacy AWS payment values to display labels + * Map legacy AWS payment values onto the provider-canonical token. + * + * MUST stay in lockstep with the Go-side mapping in + * internal/config/validation.go:crossProviderPaymentAlias — this function + * decides which option the plan/purchase dropdowns pre-select, so a + * disagreement means the UI shows one billing schedule while the backend + * canonicalizes to another. See #1503. + * + * Azure reservations offer exactly two billing plans, Upfront and Monthly, + * and the total cost is identical either way ("The total cost of up-front and + * monthly reservations is the same and you don't pay any extra fees when you + * choose to pay monthly" — + * https://learn.microsoft.com/en-us/azure/cost-management-billing/reservations/prepare-buy-reservation). + * There is no partial-upfront equivalent, so that token has to land on one of + * the two. It lands on 'monthly': the billing plan cannot be changed after + * purchase (same doc set, "Manage Azure Reservations"), so pre-selecting + * 'upfront' would put a full, irreversible upfront charge in front of a user + * who never asked for one. */ export function normalizePaymentValue(value: string, provider: string): string { // Handle legacy values or cross-provider values if (provider === 'azure') { - if (value === 'all-upfront' || value === 'partial-upfront') { + if (value === 'all-upfront') { return 'upfront'; } - if (value === 'no-upfront') { + if (value === 'no-upfront' || value === 'partial-upfront') { return 'monthly'; } } else if (provider === 'gcp') { From 2acccae827dfa02153146fc7d795edd0037a4c3f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 14:36:12 +0200 Subject: [PATCH 7/9] docs(config): correct azure payment-coercion rationale The doc comments justifying the partial-upfront to monthly coercion cited providers/azure/services/compute/client.go's GetOfferingDetails as the consumer that "drives the upfront-vs-monthly cost split". GetOfferingDetails has no non-test callers anywhere in the repo; it exists only to satisfy the ServiceClient interface, so the stated mechanism does not run. On a money path a wrong rationale is worse than none, since the next reader reasons from it. Replace it with what actually consumes the canonical token today (persisted on the execution, then copied into common.Recommendation.PaymentOption in internal/purchase/execution.go) and the billingPlan wiring arriving in #1495/#1502. Also record the two Microsoft-documented facts the decision rests on: upfront and monthly cost the same total, and the billing frequency cannot be changed after purchase. Note the documented exception that monthly is unavailable for SUSE Linux, Red Hat, Azure Red Hat OpenShift and pre-purchase plans, where the coerced token makes Azure reject the purchase; that loud rejection is the intended outcome. Adds the cross-reference to the frontend mirror of this mapping so the two stay in lockstep. Comments only, no behaviour change. Refs #1503 --- internal/config/validation.go | 51 ++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index 56f94b4ff..dacce9dc0 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -96,14 +96,15 @@ func validPaymentOptionsFor(provider string) []string { // partial-upfront → monthly (no semantic equivalent — coerce to the // no-upfront tier, CUDly's default billing schedule for Azure, rather // than drop the rec; caller may log). Coercing to upfront here would -// silently bill an all-upfront schedule the caller never chose: this -// normalized payment option drives the upfront-vs-monthly cost split in -// providers/azure/services/compute/client.go's GetOfferingDetails -// (the PaymentOption switch that allocates upfrontCost vs -// recurringCost); the reservation-purchase billingPlan wiring being -// added in #1495/#1502 likewise maps only upfront/monthly tokens and -// hard-errors on partial-upfront, so landing on monthly keeps the rec -// on the default schedule instead. +// silently bill an all-upfront schedule the caller never chose: the +// canonical token is persisted onto the execution and copied into +// common.Recommendation.PaymentOption (internal/purchase/execution.go), +// and the reservation-purchase billingPlan wiring being added in +// #1495/#1502 maps only upfront/monthly tokens and hard-errors on +// partial-upfront, so landing on monthly keeps the rec on the +// no-upfront schedule instead of an irreversible upfront charge — +// Azure does not allow changing a reservation's billing frequency +// after purchase. // - GCP : all-upfront → monthly, no-upfront → monthly, // partial-upfront → monthly, upfront → monthly (GCP CUDs are // inherently monthly-billed — every non-monthly token collapses to the @@ -116,10 +117,25 @@ func validPaymentOptionsFor(provider string) []string { // The partial-upfront coercion is deliberate on both Azure and GCP: dropping // the rec would be a silent data loss for the user, while coercing to the // closest billing model the provider offers preserves the rec without -// changing the money the caller committed to (Azure's no-upfront/monthly -// schedule bills the same total as upfront, just spread monthly). The caller -// is expected to log a warning when raw != canonical so an operator notices -// the upstream input bug. +// changing the total the caller committed to. Microsoft documents that +// choice as cost-neutral: "The total cost of up-front and monthly +// reservations is the same and you don't pay any extra fees when you choose +// to pay monthly" +// (https://learn.microsoft.com/en-us/azure/cost-management-billing/reservations/prepare-buy-reservation). +// Caveat from the same page: monthly payments are NOT offered for SUSE Linux +// reservations, Red Hat plans, Azure Red Hat OpenShift licenses, or +// pre-purchase plans, so for those products the coerced token can make Azure +// reject the purchase. That is the intended failure mode — a loud purchase- +// time rejection is preferable to silently committing the caller to an +// upfront charge they never chose and cannot undo. +// +// The caller is expected to log a warning when raw != canonical so an +// operator notices the upstream input bug. +// +// The frontend mirrors this mapping in +// frontend/src/commitmentOptions.ts:normalizePaymentValue (it decides which +// option the plan/purchase dropdowns pre-select); the two must stay in +// lockstep. // // Empty raw passes through as ("", true) — callers that distinguish "unset" // from "invalid" can check the returned bool only when raw is non-empty. @@ -160,14 +176,13 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { // equivalent — coerce to the no-upfront (monthly) tier, CUDly's // default billing schedule, so the rec survives validation rather // than dropping silently (caller WARN-logs). Coercing to "upfront" - // would silently bill an upfront schedule the caller never chose: - // the web/API path's normalized payment option drives the - // upfront-vs-monthly cost split in - // providers/azure/services/compute/client.go's - // GetOfferingDetails (the PaymentOption switch), and the - // reservation-purchase billingPlan wiring being added in + // would silently bill an upfront schedule the caller never chose + // and cannot undo (Azure forbids changing a reservation's billing + // frequency after purchase), whereas monthly costs the same total. + // The reservation-purchase billingPlan wiring being added in // #1495/#1502 hard-errors on partial-upfront, so only the // canonical upfront/monthly tokens survive to the purchase body. + // Rationale and doc citations: see NormalizePaymentOption above. switch raw { case "all-upfront": return "upfront", true From bc461e999eebf115f5c7d72c534defa415c875fb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 18:09:37 +0200 Subject: [PATCH 8/9] fix(frontend): surface azure payment coercion to the user (#1503) The backend already returns `payment_adjustments` when it normalizes a requested payment option onto a different provider-canonical token (Azure has exactly two billing plans, Upfront and Monthly, so an inherited AWS-style `partial-upfront` has nowhere to land and is coerced to monthly). The field was declared in `frontend/src/api/types.ts` and read by nothing, so the coercion was disclosed to API clients and to the operator WARN log but never to the web user whose billing schedule actually changed. Coercing instead of rejecting is only defensible if the user is told, so this closes that gap: - Add `formatPaymentAdjustmentNotice` next to `normalizePaymentValue`, the mapping it discloses. Reuses the existing `getPaymentLabel` so the copy shows "Partial Upfront"/"Pay Monthly" rather than raw API tokens, and collapses a batch to its distinct requested -> applied pairs. - Render it as a separate non-expiring warning toast on both purchase submit paths (single and fan-out) so an irreversible billing-schedule change cannot scroll away inside a success message. The fan-out path collects from every fulfilled response, including buckets whose approval email failed, since those still created a pending execution carrying the coerced schedule. - Extract the inline `payment_adjustments` element type into a named `PaymentAdjustment` interface so the formatter is typed against the same shape the API returns. Regression coverage: the two disclosure tests in purchase-execution-toast.test.ts fail with the wiring removed and pass with it, exercising the real handler through to the rendered toast; a formatter-only unit test could not prove the notice reaches the user. --- .../src/__tests__/commitmentOptions.test.ts | 60 +++++++ .../purchase-execution-toast.test.ts | 148 ++++++++++++++++++ frontend/src/api/types.ts | 32 ++-- frontend/src/app.ts | 23 +++ frontend/src/commitmentOptions.ts | 51 ++++++ 5 files changed, 304 insertions(+), 10 deletions(-) diff --git a/frontend/src/__tests__/commitmentOptions.test.ts b/frontend/src/__tests__/commitmentOptions.test.ts index 37ba9c9b1..7b5d9d008 100644 --- a/frontend/src/__tests__/commitmentOptions.test.ts +++ b/frontend/src/__tests__/commitmentOptions.test.ts @@ -11,10 +11,12 @@ import { populatePaymentSelect, getPaymentLabel, normalizePaymentValue, + formatPaymentAdjustmentNotice, CommitmentConfig, PaymentOption, TermOption } from '../commitmentOptions'; +import type { PaymentAdjustment } from '../api/types'; describe('commitmentOptions', () => { describe('getCommitmentConfig', () => { @@ -821,4 +823,62 @@ describe('commitmentOptions', () => { expect(isValidCombination('aws', 'rds', 1, 'partial-upfront')).toBe(true); }); }); + + // #1503: the backend coerces a payment option the target provider cannot + // express (Azure has only Upfront/Monthly) instead of rejecting it. That is + // only acceptable if the change is disclosed, so this formatter is the copy + // every purchase-submit path renders. A null return means "nothing to say". + describe('formatPaymentAdjustmentNotice', () => { + const adj = (over: Partial = {}): PaymentAdjustment => ({ + rec_index: 0, + provider: 'azure', + service: 'vm', + requested_payment_option: 'partial-upfront', + applied_payment_option: 'monthly', + reason: 'no azure equivalent', + ...over, + }); + + it('returns null when there is nothing to disclose', () => { + expect(formatPaymentAdjustmentNotice(undefined)).toBeNull(); + expect(formatPaymentAdjustmentNotice([])).toBeNull(); + }); + + it('names both the requested and the applied schedule for a single rec', () => { + const msg = formatPaymentAdjustmentNotice([adj()]); + // Human labels, not raw API tokens -- the user picked "Partial Upfront" + // in the UI, so that is the wording they will recognise. + expect(msg).toContain('Partial Upfront'); + expect(msg).toContain('Pay Monthly'); + expect(msg).toContain('AZURE'); + expect(msg).toContain('vm'); + // The raw tokens must not leak into user-facing copy. + expect(msg).not.toContain('partial-upfront'); + }); + + it('collapses a batch to the distinct requested -> applied pairs', () => { + const msg = formatPaymentAdjustmentNotice([ + adj({ rec_index: 0 }), + adj({ rec_index: 1 }), + adj({ rec_index: 2 }), + ]); + // Count reflects every affected rec... + expect(msg).toContain('3 recommendations'); + // ...but the identical mapping is stated once, not three times. + expect(msg?.match(/Partial Upfront/g)).toHaveLength(1); + }); + + it('lists every distinct mapping when a batch was coerced differently', () => { + const msg = formatPaymentAdjustmentNotice([ + adj({ requested_payment_option: 'partial-upfront', applied_payment_option: 'monthly' }), + adj({ requested_payment_option: 'all-upfront', applied_payment_option: 'upfront' }), + ]); + expect(msg).toContain('2 recommendations'); + expect(msg).toContain('Partial Upfront'); + expect(msg).toContain('All Upfront'); + expect(msg).toContain('Pay Monthly'); + expect(msg).toContain('Pay Upfront'); + }); + }); + }); diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 356cc39de..f35f6dd5d 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -173,6 +173,15 @@ function lastToastMessage(): string | null { return last?.querySelector('.toast-message')?.textContent ?? last?.textContent ?? null; } +/** Return the text of every rendered toast, oldest first. */ +function allToastMessages(): string[] { + const container = document.getElementById('toast-container'); + if (!container) return []; + return Array.from(container.querySelectorAll('.toast')).map( + (t) => t.querySelector('.toast-message')?.textContent ?? t.textContent ?? '', + ); +} + /** Return the kind class of the most recently rendered toast (success/error/warning). */ function lastToastKind(): string | null { const container = document.getElementById('toast-container'); @@ -842,3 +851,142 @@ describe('handleExecutePurchase — double-submit guard (#644)', () => { expect(btn.textContent).toBe('Send for Approval'); }); }); + +// ── #1503: payment-option coercion must be disclosed to the user ───────────── +// +// The backend does not reject a payment option the target provider cannot +// express (Azure has exactly two billing plans, so an inherited AWS-style +// 'partial-upfront' token has nowhere to land). It coerces to the closest +// supported schedule and reports it in `payment_adjustments`. That coercion is +// only defensible if the user is actually TOLD -- otherwise their billing +// schedule changes behind their back, which is the failure #1503 reported. +// +// These are the end-to-end guards: a green unit test on the formatter alone +// could not prove the notice survives to a toast the user actually sees. +describe('#1503 — payment-option coercion is disclosed in the purchase toast', () => { + function buildAzureBucket(id: string) { + return { + key: `key-${id}`, + label: `Bucket ${id}`, + provider: 'azure', + service: `svc-${id}`, + recs: [buildMinimalRec()], + payment: 'partial-upfront', + capacityPercent: 100, + }; + } + + beforeEach(() => { + jest.clearAllMocks(); + (recs.getFanOutBuckets as jest.Mock).mockReturnValue([]); + (recs.getPurchaseModalRecommendations as jest.Mock).mockReturnValue([buildMinimalRec()]); + (plans.closePurchaseModal as jest.Mock).mockImplementation(() => undefined); + }); + + afterEach(() => { + document.body.textContent = ''; + }); + + test('single path — partial-upfront coerced to monthly raises a warning toast', async () => { + (api.executePurchase as jest.Mock).mockResolvedValue({ + execution_id: 'exec-adj-1', + status: 'queued', + email_sent: true, + approval_recipient: 'approver@example.com', + payment_adjustments: [ + { + rec_index: 0, + provider: 'azure', + service: 'vm', + requested_payment_option: 'partial-upfront', + applied_payment_option: 'monthly', + reason: 'payment option "partial-upfront" is not in azure\'s supported set', + }, + ], + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const messages = allToastMessages(); + // The success toast must still be shown -- disclosure is additive. + expect(messages.some((m) => m.includes('Approval request sent to'))).toBe(true); + // ...and the coercion must be surfaced, naming both schedules so the user + // can tell what they asked for from what they are actually getting. + const notice = messages.find((m) => m.includes('Billing schedule adjusted')); + expect(notice).toBeDefined(); + expect(notice).toContain('Partial Upfront'); + expect(notice).toContain('Pay Monthly'); + expect(notice).toContain('AZURE'); + // Warning, and non-expiring: an irreversible billing-schedule change must + // not scroll away before the user reads it. + expect(lastToastKind()).toBe('warning'); + }); + + test('single path — no adjustments means no extra toast (no noise on the common case)', async () => { + (api.executePurchase as jest.Mock).mockResolvedValue({ + execution_id: 'exec-adj-2', + status: 'queued', + email_sent: true, + approval_recipient: 'approver@example.com', + // payment_adjustments intentionally absent: everything was canonical. + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const messages = allToastMessages(); + expect(messages.some((m) => m.includes('Billing schedule adjusted'))).toBe(false); + expect(lastToastKind()).toBe('success'); + }); + + test('fan-out path — adjustments from every bucket are disclosed, incl. email-failed buckets', async () => { + (recs.getPurchaseModalRecommendations as jest.Mock).mockReturnValue([]); + (recs.getFanOutBuckets as jest.Mock).mockReturnValue([ + buildAzureBucket('a'), + buildAzureBucket('b'), + ]); + + const adjustment = (recIndex: number) => ({ + rec_index: recIndex, + provider: 'azure', + service: 'vm', + requested_payment_option: 'partial-upfront', + applied_payment_option: 'monthly', + reason: 'no azure equivalent', + }); + + (api.executePurchase as jest.Mock) + .mockResolvedValueOnce({ + execution_id: 'exec-a', + status: 'queued', + email_sent: true, + approval_recipient: 'alice@example.com', + payment_adjustments: [adjustment(0)], + }) + // Bucket b's approval email failed to send, but the pending execution + // still exists carrying the coerced schedule, so its adjustment must + // be disclosed too rather than dropped with the "failed" bucket. + .mockResolvedValueOnce({ + execution_id: 'exec-b', + status: 'queued', + email_sent: false, + email_reason: 'SMTP timeout', + payment_adjustments: [adjustment(0)], + }); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const notice = allToastMessages().find((m) => m.includes('Billing schedule adjusted')); + expect(notice).toBeDefined(); + // Both buckets contributed, so the count must be 2 -- proving the + // email-failed bucket's coercion was not silently dropped. + expect(notice).toContain('2 recommendations'); + expect(notice).toContain('Partial Upfront'); + expect(notice).toContain('Pay Monthly'); + }); +}); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index befcbf653..2584e2ef4 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -357,6 +357,26 @@ export interface DeploymentInfo { } // Purchase types + +/** + * PaymentAdjustment is one payment-option coercion notice returned by the + * purchase-execute endpoint (#1503 follow-up). Mirrors the backend + * PaymentAdjustment struct (internal/api/validation.go) field for field. + * + * Named (rather than inlined into PurchaseResult) so the presentation helper + * that turns these into user-facing copy + * (commitmentOptions.ts:formatPaymentAdjustmentNotice) can be typed and unit + * tested against the same shape the API actually returns. + */ +export interface PaymentAdjustment { + rec_index: number; + provider: string; + service: string; + requested_payment_option: string; + applied_payment_option: string; + reason: string; +} + export interface PurchaseResult { execution_id: string; status: string; @@ -384,16 +404,8 @@ export interface PurchaseResult { // each entry names what was requested, what was actually applied, and why, // so the UI can tell the user instead of silently changing the billing // schedule. Absent (never an empty array) when every option was already - // canonical. Mirrors the backend PaymentAdjustment struct - // (internal/api/validation.go). - payment_adjustments?: Array<{ - rec_index: number; - provider: string; - service: string; - requested_payment_option: string; - applied_payment_option: string; - reason: string; - }>; + // canonical. + payment_adjustments?: PaymentAdjustment[]; results?: Array<{ recommendation_id: string; status: string; diff --git a/frontend/src/app.ts b/frontend/src/app.ts index caf9a372b..27b449543 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -16,6 +16,7 @@ import { loadHistory, setupHistoryHandlers } from './history'; import { initSavingsHistory } from './modules/savings-history'; import { setupRIExchangeHandlers, saveAutomationSettings } from './riexchange'; import { showToast } from './toast'; +import { formatPaymentAdjustmentNotice } from './commitmentOptions'; import { confirmDialog } from './confirmDialog'; import { handlePurchaseDeeplink } from './purchases-deeplink'; import { handleArcheraDeeplink, openArcheraOfferModal } from './archera'; @@ -438,6 +439,16 @@ async function handleExecutePurchase(): Promise { timeout: 10_000, }); } + // Disclose any payment-option coercion the backend applied (#1503). This + // is a SEPARATE, non-expiring warning toast rather than an addition to the + // success copy above: the billing schedule changing is the one thing the + // user did not ask for, and on Azure it cannot be changed after purchase, + // so it must not scroll away inside a success message. + const paymentNotice = formatPaymentAdjustmentNotice(result.payment_adjustments); + if (paymentNotice) { + showToast({ message: paymentNotice, kind: 'warning', timeout: null }); + } + // Offer Archera Insurance immediately after the user approves the // pre-purchase confirmation and the approval-submission call succeeds // (issue #499 follow-up). Firing here, rather than after the async @@ -550,6 +561,18 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise { clearFanOutBuckets(); clearPurchaseModalRecommendations(); + // Disclose payment-option coercions across every bucket that reached the + // backend (#1503). Collected from all fulfilled responses, not just the + // truly-succeeded ones: a bucket whose approval email failed to send still + // created a pending execution carrying the coerced billing schedule, so the + // user needs to know before they approve it from History. + const fanOutNotice = formatPaymentAdjustmentNotice( + fulfilled.flatMap((r) => r.value.payment_adjustments ?? []), + ); + if (fanOutNotice) { + showToast({ message: fanOutNotice, kind: 'warning', timeout: null }); + } + if (failed === 0) { // Collect the unique approval-recipient set from truly-succeeded responses // only (email_sent !== false and status !== 'failed') so the toast doesn't diff --git a/frontend/src/commitmentOptions.ts b/frontend/src/commitmentOptions.ts index 299490678..a37898de3 100644 --- a/frontend/src/commitmentOptions.ts +++ b/frontend/src/commitmentOptions.ts @@ -5,6 +5,8 @@ * for each cloud provider and service combination. */ +import type { PaymentAdjustment } from './api/types'; + export interface PaymentOption { value: string; label: string; @@ -326,3 +328,52 @@ export function normalizePaymentValue(value: string, provider: string): string { } return value; } + +/** + * Turn the backend's payment-option coercion notices into one line of + * user-facing copy, or null when there is nothing to disclose. + * + * Why this exists (#1503): the backend does not reject a payment option that + * the target provider cannot express (e.g. Azure has exactly two billing + * plans, Upfront and Monthly, so an inherited AWS-style 'partial-upfront' + * token has nowhere to land). It coerces to the closest supported schedule + * and reports the change in `payment_adjustments`. Coercing is only + * defensible if the user is actually told, so every purchase-submit path + * MUST render this notice -- otherwise the user's billing schedule changes + * behind their back, which is exactly the failure #1503 reported. + * + * The applied value, not the requested one, is what the purchase carries; + * the copy leads with that because it is the fact the user has to act on. + * Azure's billing plan cannot be changed after purchase. + */ +export function formatPaymentAdjustmentNotice( + adjustments: PaymentAdjustment[] | undefined, +): string | null { + if (!adjustments || adjustments.length === 0) return null; + + const only = adjustments.length === 1 ? adjustments[0] : undefined; + if (only) { + return ( + `Billing schedule adjusted: ${only.provider.toUpperCase()} ${only.service} does not offer ` + + `"${getPaymentLabel(only.requested_payment_option)}", so this purchase was applied as ` + + `"${getPaymentLabel(only.applied_payment_option)}".` + ); + } + + // Multiple recs: collapse to the distinct requested -> applied pairs so the + // toast stays short when a whole batch inherited the same legacy token, + // while still naming every distinct change that was made. + const pairs = [ + ...new Set( + adjustments.map( + a => + `"${getPaymentLabel(a.requested_payment_option)}" applied as ` + + `"${getPaymentLabel(a.applied_payment_option)}"`, + ), + ), + ]; + return ( + `Billing schedule adjusted on ${adjustments.length} recommendations ` + + `(unsupported payment option for the target provider): ${pairs.join('; ')}.` + ); +} From 84a55ac228a2a5451c20900c6b01b05ee5ff7095 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 19:28:35 +0200 Subject: [PATCH 9/9] fix(api): disclose payment coercion only when the schedule changes The #1503 disclosure fired on every raw != canonical rewrite, including the cross-provider renames that leave the customer's cash flow untouched. Azure spells AWS's all-upfront "upfront" and AWS's no-upfront "monthly", so both of those rewrites are bookkeeping, not a billing change. That mattered on the ordinary path, not an edge case: the fan-out purchase modal builds its per-bucket Payment dropdown from paymentOptionsFor (frontend/src/lib/purchase-compatibility.ts), whose candidate list has no "upfront" entry. An Azure bucket the user chooses to pay upfront therefore ALWAYS submits "all-upfront", so every ordinary Azure upfront purchase raised a sticky "Billing schedule adjusted" warning claiming Azure "does not offer All Upfront" and that the purchase was applied as something else. Both claims are false, and the noise trains users to dismiss the one notice that is real. Classify a payment token by the cash flow it implies (config.PaymentScheduleFor) and surface a PaymentAdjustment only when the rewrite crosses schedules (config.PaymentCoercionChangesSchedule). Azure partial-upfront -> monthly and GCP upfront -> monthly still disclose; Azure all-upfront -> upfront and no-upfront -> monthly no longer do. The operator WARN keeps firing on every rewrite: a non-canonical token on the wire is an upstream input bug worth auditing even when it costs the customer nothing. Only the user-facing notice is narrowed. Adjustment construction moves into paymentAdjustmentFor so validatePurchaseRecommendation stays at gocyclo 10 (the pre-commit threshold), not 11. Regression coverage: the three rename cases assert nil adjustment plus a still-emitted WARN, and the GCP upfront-shaped tokens assert a surfaced adjustment. Both new tests fail against the previous raw != canonical condition and pass after (verified by reverting the condition). --- frontend/src/api/types.ts | 9 +- internal/api/handler_purchases_guards_test.go | 194 +++++++++++++----- internal/api/validation.go | 70 +++++-- internal/config/validation.go | 64 ++++++ internal/config/validation_test.go | 62 ++++++ 5 files changed, 325 insertions(+), 74 deletions(-) diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 2584e2ef4..ca89e1411 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -399,12 +399,15 @@ export interface PurchaseResult { // #289). Absent (undefined) on the standard approval-required flow. direct_execute?: boolean; // Per-rec payment-option coercion notices (#1503 follow-up). Present only - // when the backend normalized a requested payment option onto a DIFFERENT - // provider-canonical token (e.g. Azure "partial-upfront" -> "monthly"): + // when the backend normalized a requested payment option onto a token that + // bills on a DIFFERENT schedule (e.g. Azure "partial-upfront" -> "monthly"): // each entry names what was requested, what was actually applied, and why, // so the UI can tell the user instead of silently changing the billing // schedule. Absent (never an empty array) when every option was already - // canonical. + // canonical, and also when the backend only respelled a token into the + // provider's own vocabulary for the same schedule (Azure "all-upfront" -> + // "upfront") — that changes nothing the user pays, so warning about it + // would just teach them to dismiss the notice that matters. payment_adjustments?: PaymentAdjustment[]; results?: Array<{ recommendation_id: string; diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index f130d65d3..c15185d7a 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -44,80 +44,96 @@ func TestValidatePurchaseRecommendation(t *testing.T) { name string rec config.RecommendationRecord wantError bool - // wantPayment is set ONLY on rows whose input token gets coerced: it is - // asserted against rec.Payment after a successful call, and the row must - // also surface a matching PaymentAdjustment. Rows with "" must NOT - // surface an adjustment (canonical passthrough, incl. case-only changes). + // wantPayment is set ONLY on rows whose input token gets rewritten: it + // is asserted against rec.Payment after a successful call. "" means the + // token was already canonical for the provider (case-only differences + // included). wantPayment string + // wantAdjustment marks the rows whose rewrite changes the BILLING + // SCHEDULE rather than just the spelling, and so must surface a + // caller-visible PaymentAdjustment (#1503 follow-up). A rewrite between + // two spellings of the same schedule (Azure "all-upfront" -> "upfront") + // costs the customer nothing and must NOT be disclosed. Field-level + // assertions live in + // TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment and + // TestValidatePurchaseRecommendation_NoAdjustmentForScheduleEquivalentRename. + wantAdjustment bool }{ // --- AWS canonical set --- - {"valid aws all-upfront 3y", validRec(), false, ""}, - {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false, ""}, - {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false, ""}, - {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true, ""}, - {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true, ""}, + {"valid aws all-upfront 3y", validRec(), false, "", false}, + {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false, "", false}, + {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false, "", false}, + {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true, "", false}, + {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true, "", false}, // --- Azure canonical set --- - {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false, ""}, - {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false, ""}, - // Legacy AWS-style tokens on Azure are normalized to Azure-canonical before validation. - {"azure accepts legacy all-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { + {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false, "", false}, + {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false, "", false}, + // Legacy AWS-style tokens on Azure are normalized to Azure-canonical + // before validation. Both of these are pure respellings -- Azure's + // "upfront" IS all-upfront and its "monthly" IS no-upfront -- so the + // customer's cash flow is untouched and no adjustment is surfaced. + {"azure accepts legacy all-upfront (respelled upfront, no adjustment)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "all-upfront" - }), false, "upfront"}, - {"azure accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + }), false, "upfront", false}, + {"azure accepts legacy no-upfront (respelled monthly, no adjustment)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "no-upfront" - }), false, "monthly"}, + }), false, "monthly", false}, // partial-upfront has no Azure equivalent; it coerces to monthly (the // no-upfront default), never to upfront, so the caller never gets // silently billed an all-upfront schedule it did not choose (#1503). + // This one genuinely changes the schedule, so it must be disclosed. {"azure accepts legacy partial-upfront (coerced to monthly, not upfront)", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "partial-upfront" - }), false, "monthly"}, + }), false, "monthly", true}, {"azure rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure" r.Payment = "foo" - }), true, ""}, + }), true, "", false}, // --- GCP canonical set (monthly-only) --- - {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false, ""}, - // Legacy tokens on GCP are all normalized to monthly. + {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false, "", false}, + // Legacy tokens on GCP are all normalized to monthly. GCP has no + // upfront tier at all, so every upfront-shaped token really does move + // the customer onto a different schedule; only no-upfront is a + // respelling of the schedule they already asked for. {"gcp accepts legacy upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "upfront" - }), false, "monthly"}, + }), false, "monthly", true}, {"gcp accepts legacy all-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "all-upfront" - }), false, "monthly"}, - {"gcp accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + }), false, "monthly", true}, + {"gcp accepts legacy no-upfront (respelled monthly, no adjustment)", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "no-upfront" - }), false, "monthly"}, + }), false, "monthly", false}, {"gcp rejects unknown token", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp" r.Payment = "foo" - }), true, ""}, + }), true, "", false}, // --- General --- - {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false, ""}, - {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true, ""}, - {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true, ""}, - {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true, ""}, - {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true, ""}, + {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false, "", false}, + {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true, "", false}, + {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true, "", false}, + {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true, "", false}, + {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true, "", false}, {"negative monthly cost rejected", mutate(func(r *config.RecommendationRecord) { m := -1.0 r.MonthlyCost = &m - }), true, ""}, - {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false, ""}, + }), true, "", false}, + {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false, "", false}, {"zero monthly cost accepted", mutate(func(r *config.RecommendationRecord) { m := 0.0 r.MonthlyCost = &m - }), false, ""}, - {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true, ""}, - {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true, ""}, - {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true, ""}, - {"all provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "all" }), true, ""}, - {"unknown provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "ibm" }), true, ""}, + }), false, "", false}, + {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true, "", false}, + {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true, "", false}, + {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true, "", false}, + {"all provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "all" }), true, "", false}, + {"unknown provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "ibm" }), true, "", false}, } for _, tt := range tests { tt := tt @@ -127,20 +143,17 @@ func TestValidatePurchaseRecommendation(t *testing.T) { adjustment, err := validatePurchaseRecommendation(&rec, 0) if tt.wantError { require.Error(t, err) + return + } + require.NoError(t, err) + if tt.wantPayment != "" { + assert.Equal(t, tt.wantPayment, rec.Payment) + } + if tt.wantAdjustment { + require.NotNil(t, adjustment, "a billing-schedule change must surface a PaymentAdjustment") + assert.Equal(t, tt.wantPayment, adjustment.AppliedPaymentOption) } else { - require.NoError(t, err) - if tt.wantPayment != "" { - assert.Equal(t, tt.wantPayment, rec.Payment) - // wantPayment is only set on rows whose input token gets - // coerced, so each of those must also surface a caller- - // visible PaymentAdjustment consistent with the mutation - // (#1503 follow-up). Field-level assertions live in - // TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment. - require.NotNil(t, adjustment, "coerced payment option must surface a PaymentAdjustment") - assert.Equal(t, tt.wantPayment, adjustment.AppliedPaymentOption) - } else { - assert.Nil(t, adjustment, "no coercion occurred, so no adjustment should be surfaced") - } + assert.Nil(t, adjustment, "the billing schedule did not change, so nothing should be disclosed") } }) } @@ -247,6 +260,85 @@ func TestValidatePurchaseRecommendation_SurfacesPaymentAdjustment(t *testing.T) assert.Equal(t, adjustment.AppliedPaymentOption, rec.Payment) } +// TestValidatePurchaseRecommendation_NoAdjustmentForScheduleEquivalentRename +// guards the other half of the #1503 disclosure contract: a coercion that only +// respells a token in the target provider's vocabulary must NOT be reported as +// an adjustment, because nothing about the customer's cash flow changed. +// +// This is not hypothetical noise-avoidance. The fan-out purchase modal's +// per-bucket Payment dropdown is populated from paymentOptionsFor +// (frontend/src/lib/purchase-compatibility.ts), whose candidate list has no +// "upfront" entry — so an Azure bucket the user pays upfront ALWAYS submits +// the AWS-style "all-upfront". Reporting that rename would put a sticky +// "Billing schedule adjusted" warning on every ordinary Azure upfront +// purchase, claiming a change that did not happen and drowning out the +// partial-upfront case that did. +// +// The operator WARN is deliberately still emitted for these: a non-canonical +// token on the wire is an upstream input bug worth auditing even when it costs +// the customer nothing. +// +// Not run with t.Parallel(): captureDefaultLog mutates the shared default +// logger's output (see TestValidatePurchaseRecommendation_NormalizationWarning). +func TestValidatePurchaseRecommendation_NoAdjustmentForScheduleEquivalentRename(t *testing.T) { + tests := []struct { + name string + provider string + payment string + wantPayment string + }{ + // Azure spells AWS's all-upfront "upfront": same single charge at + // purchase, different word. + {"azure all-upfront is upfront respelled", "azure", "all-upfront", "upfront"}, + // Azure spells AWS's no-upfront "monthly": same per-period billing. + {"azure no-upfront is monthly respelled", "azure", "no-upfront", "monthly"}, + // GCP likewise has only the recurring schedule spelled "monthly". + {"gcp no-upfront is monthly respelled", "gcp", "no-upfront", "monthly"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + logBuf := captureDefaultLog(t) + + rec := validRec() + rec.Provider = tc.provider + rec.Payment = tc.payment + adjustment, err := validatePurchaseRecommendation(&rec, 0) + require.NoError(t, err) + assert.Nil(t, adjustment, + "%s bills identically to %s; a rename must not be disclosed as a billing-schedule change", + tc.payment, tc.wantPayment) + // The canonicalization itself must still happen. + assert.Equal(t, tc.wantPayment, rec.Payment) + // ...and the operator-facing audit log must still fire. + assert.Contains(t, logBuf.String(), "[WARN]", + "a non-canonical token on the wire is still an upstream input bug worth logging") + }) + } +} + +// TestValidatePurchaseRecommendation_AdjustmentWhenScheduleChanges covers the +// non-Azure half of the same contract: GCP commitments are monthly-only, so an +// upfront-shaped token really does move the customer onto a different billing +// schedule and MUST be disclosed. +func TestValidatePurchaseRecommendation_AdjustmentWhenScheduleChanges(t *testing.T) { + t.Parallel() + for _, payment := range []string{"all-upfront", "upfront", "partial-upfront"} { + t.Run(payment, func(t *testing.T) { + t.Parallel() + rec := validRec() + rec.Provider = "gcp" + rec.Payment = payment + adjustment, err := validatePurchaseRecommendation(&rec, 0) + require.NoError(t, err) + require.NotNil(t, adjustment, + "gcp has no upfront billing tier, so %q lands on a different schedule and must be disclosed", payment) + assert.Equal(t, payment, adjustment.RequestedPaymentOption) + assert.Equal(t, "monthly", adjustment.AppliedPaymentOption) + assert.Equal(t, "monthly", rec.Payment) + }) + } +} + // TestHandler_executePurchase_SurfacesPaymentAdjustments is the response-level // regression test for the #1503 follow-up: an Azure partial-upfront purchase // submitted through the real executePurchase handler must return a diff --git a/internal/api/validation.go b/internal/api/validation.go index db7db52ef..e9d080a64 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -524,12 +524,17 @@ var purchaseTermWhitelist = map[string]map[int]bool{ // PaymentAdjustment surfaces a payment-option coercion to the API caller // (follow-up to #1503): when the web execute path normalizes a caller-supplied -// payment option onto a provider-canonical token that DIFFERS from what was -// requested (e.g. Azure "partial-upfront" -> "monthly"), the response carries -// one of these per adjusted rec so the caller sees what they requested, what -// was actually applied, and why, not just the operator WARN log. Purely -// additive visibility: the coercion policy itself lives in +// payment option onto a provider-canonical token that bills on a DIFFERENT +// schedule than what was requested (e.g. Azure "partial-upfront" -> "monthly"), +// the response carries one of these per adjusted rec so the caller sees what +// they requested, what was actually applied, and why, not just the operator +// WARN log. Purely additive visibility: the coercion policy itself lives in // config.NormalizePaymentOption and is unchanged. +// +// Cross-provider renames of the SAME schedule (Azure "all-upfront" -> +// "upfront", "no-upfront" -> "monthly") are not adjustments: nothing about +// the customer's cash flow changed, so there is nothing to disclose. See +// config.PaymentCoercionChangesSchedule. type PaymentAdjustment struct { // RecIndex is the rec's position in the request's recommendations slice. RecIndex int `json:"rec_index"` @@ -561,9 +566,10 @@ type PaymentAdjustment struct { // rejects anything that has no canonical mapping, with an error that // names the provider and lists the accepted tokens. // -// When step 1 actually changes the token (raw != canonical), the returned -// *PaymentAdjustment describes the coercion so the response can surface it to -// the caller; nil means the payment option was already canonical. +// When step 1 changes the BILLING SCHEDULE (not merely the spelling), the +// returned *PaymentAdjustment describes the coercion so the response can +// surface it to the caller; nil means the payment option was already canonical +// or was respelled onto an equivalent schedule. func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) (*PaymentAdjustment, error) { provider := strings.ToLower(strings.TrimSpace(rec.Provider)) payments := purchasePaymentSet(provider) @@ -597,24 +603,26 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) ( // coercion of this money-affecting field, matching the WARN contract // documented on config.NormalizePaymentOption and mirroring the same // coerced/uncoerced logging convertRecommendations does at the - // scheduler's emission boundary (internal/scheduler/scheduler.go). The - // same transition is also returned as a PaymentAdjustment so the API - // response surfaces it to the caller, not just the operator log. + // scheduler's emission boundary (internal/scheduler/scheduler.go). + // + // The caller-facing PaymentAdjustment is deliberately NARROWER than the + // WARN: it is returned only when the rewrite changes the billing schedule + // itself, not when it merely respells the same schedule in the target + // provider's vocabulary (Azure "all-upfront" -> "upfront", "no-upfront" -> + // "monthly"). Every WARN is worth an operator's attention because it means + // an upstream caller sent a non-canonical token; only a schedule change is + // worth interrupting the user, whose money is what actually moved. The + // fan-out purchase modal submits "all-upfront" for Azure buckets by + // construction (frontend/src/lib/purchase-compatibility.ts), so surfacing + // renames too would warn on the ordinary Azure upfront purchase and teach + // users to dismiss the notice that matters. var adjustment *PaymentAdjustment if normalized, ok := config.NormalizePaymentOption(provider, payment); ok { if normalized != payment { logging.Warnf("validatePurchaseRecommendation: rec %d (%s/%s) payment option normalized: raw=%q canonical=%q", idx, provider, rec.Service, payment, normalized) - adjustment = &PaymentAdjustment{ - RecIndex: idx, - Provider: provider, - Service: rec.Service, - RequestedPaymentOption: payment, - AppliedPaymentOption: normalized, - Reason: fmt.Sprintf("payment option %q is not in %s's supported set (%s); the closest supported option %q was applied", - payment, provider, strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), normalized), - } } + adjustment = paymentAdjustmentFor(idx, provider, rec.Service, payment, normalized) payment = normalized } if !payments[payment] { @@ -628,6 +636,28 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) ( return adjustment, nil } +// paymentAdjustmentFor builds the caller-facing coercion notice for a single +// rec, or returns nil when there is nothing to disclose: either the token was +// left alone, or it was only respelled into the target provider's vocabulary +// for the same billing schedule (see config.PaymentCoercionChangesSchedule). +// +// requested is the caller's token after trim/lowercase; applied is the +// provider-canonical token the purchase will actually carry. +func paymentAdjustmentFor(idx int, provider, service, requested, applied string) *PaymentAdjustment { + if !config.PaymentCoercionChangesSchedule(requested, applied) { + return nil + } + return &PaymentAdjustment{ + RecIndex: idx, + Provider: provider, + Service: service, + RequestedPaymentOption: requested, + AppliedPaymentOption: applied, + Reason: fmt.Sprintf("payment option %q is not in %s's supported set (%s); the closest supported option %q was applied", + requested, provider, strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), applied), + } +} + // validateCapacityConsistency cross-checks the client-supplied capacity_percent // against the scaled rec counts so the audit record can't claim a capacity that // disagrees with what was actually purchased (#647). The frontend scales each diff --git a/internal/config/validation.go b/internal/config/validation.go index dacce9dc0..409902d76 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -201,6 +201,70 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { return "", false } +// PaymentSchedule is the cash-flow shape a payment-option token implies, +// stripped of the provider-specific spelling of that token. Two tokens that +// map to the same PaymentSchedule bill the customer identically; only the +// word differs (AWS spells all-upfront what Azure spells upfront, and AWS +// spells no-upfront what Azure and GCP spell monthly). +// +// It exists so callers can tell a rename apart from a real change when +// NormalizePaymentOption rewrites a token: a rename is bookkeeping, a real +// change moves the customer's money and has to be disclosed (#1503). +type PaymentSchedule string + +const ( + // PaymentScheduleUpfront: the whole commitment is charged once, at purchase. + PaymentScheduleUpfront PaymentSchedule = "upfront" + // PaymentSchedulePartialUpfront: part is charged at purchase, the rest recurs. + PaymentSchedulePartialUpfront PaymentSchedule = "partial-upfront" + // PaymentScheduleRecurring: nothing is charged at purchase; the commitment + // is billed per period across the term. + PaymentScheduleRecurring PaymentSchedule = "recurring" + // PaymentScheduleUnknown: the token is not one of the modeled schedules. + // Two unrecognized tokens compare equal under this classification, so + // callers that must distinguish them have to compare the raw tokens too. + // In practice unmapped tokens never reach a coercion comparison: + // NormalizePaymentOption returns ok=false for them and the validator + // rejects them at the next boundary. + PaymentScheduleUnknown PaymentSchedule = "unknown" +) + +// PaymentScheduleFor classifies a payment-option token by the billing +// schedule it implies. The token must already be lowercased and trimmed +// (validatePurchaseRecommendation and NormalizePaymentOption both work on +// such tokens). +func PaymentScheduleFor(token string) PaymentSchedule { + switch token { + case "all-upfront", "upfront": + return PaymentScheduleUpfront + case "partial-upfront": + return PaymentSchedulePartialUpfront + case "no-upfront", "monthly": + return PaymentScheduleRecurring + default: + return PaymentScheduleUnknown + } +} + +// PaymentCoercionChangesSchedule reports whether rewriting raw to canonical +// actually changes what the customer pays and when, as opposed to merely +// renaming the same schedule into the target provider's vocabulary. +// +// Azure "all-upfront" -> "upfront" and Azure/GCP "no-upfront" -> "monthly" +// are renames: identical cash flow, different spelling. Azure +// "partial-upfront" -> "monthly" and GCP "upfront" -> "monthly" are real +// changes: the customer is billed on a schedule they did not ask for. +// +// Only real changes are worth putting in front of a user (#1503). The +// fan-out purchase modal submits the AWS-style "all-upfront" for Azure +// buckets by construction (frontend/src/lib/purchase-compatibility.ts: +// paymentOptionsFor), so treating every rewrite as a change would fire a +// "billing schedule adjusted" warning on the ordinary Azure upfront +// purchase and train users to dismiss the one notice that matters. +func PaymentCoercionChangesSchedule(raw, canonical string) bool { + return PaymentScheduleFor(raw) != PaymentScheduleFor(canonical) +} + // ValidOfferingClasses lists the accepted EC2 RI offering class values for // GlobalConfig. The empty string is also accepted (maps to "convertible" at // purchase time to preserve pre-694 behavior). diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 0237a9bfd..118d5c710 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -1021,6 +1021,68 @@ func TestNormalizePaymentOption(t *testing.T) { } } +// TestPaymentScheduleFor pins the token -> cash-flow classification that lets +// callers tell a cross-provider RENAME apart from a real billing-schedule +// change (#1503). Getting this wrong in either direction is a user-visible +// defect: over-classifying warns on ordinary purchases until users learn to +// dismiss the warning, under-classifying hides a schedule change the customer +// never chose. +func TestPaymentScheduleFor(t *testing.T) { + tests := []struct { + token string + want PaymentSchedule + }{ + // One charge at purchase, spelled two ways. + {"all-upfront", PaymentScheduleUpfront}, + {"upfront", PaymentScheduleUpfront}, + // AWS's middle tier; no Azure or GCP equivalent. + {"partial-upfront", PaymentSchedulePartialUpfront}, + // Nothing at purchase, billed per period; spelled two ways. + {"no-upfront", PaymentScheduleRecurring}, + {"monthly", PaymentScheduleRecurring}, + // Not a modeled schedule. + {"", PaymentScheduleUnknown}, + {"ohai", PaymentScheduleUnknown}, + } + for _, tt := range tests { + t.Run(tt.token, func(t *testing.T) { + assert.Equal(t, tt.want, PaymentScheduleFor(tt.token)) + }) + } +} + +// TestPaymentCoercionChangesSchedule walks every rewrite crossProviderPaymentAlias +// can actually perform and pins whether it is a disclosable change. The pairs +// are the ones NormalizePaymentOption produces, so this test breaks if a future +// mapping change silently flips a rename into a schedule change or vice versa. +func TestPaymentCoercionChangesSchedule(t *testing.T) { + tests := []struct { + name string + raw string + canonical string + want bool + }{ + // Azure vocabulary: same cash flow, different word. + {"azure all-upfront respelled upfront", "all-upfront", "upfront", false}, + {"azure no-upfront respelled monthly", "no-upfront", "monthly", false}, + // Azure has no partial tier, so this really moves the money. + {"azure partial-upfront becomes monthly", "partial-upfront", "monthly", true}, + // GCP is monthly-only: upfront-shaped tokens all change the schedule. + {"gcp upfront becomes monthly", "upfront", "monthly", true}, + {"gcp all-upfront becomes monthly", "all-upfront", "monthly", true}, + {"gcp partial-upfront becomes monthly", "partial-upfront", "monthly", true}, + {"gcp no-upfront respelled monthly", "no-upfront", "monthly", false}, + // No rewrite at all. + {"identity", "monthly", "monthly", false}, + {"empty identity", "", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, PaymentCoercionChangesSchedule(tt.raw, tt.canonical)) + }) + } +} + func TestIsValidRampScheduleType(t *testing.T) { assert.True(t, isValidRampScheduleType("immediate")) assert.True(t, isValidRampScheduleType("weekly"))