feat(recs): plumb provider SavingsPercentage to GUI for CLI/GUI parity (closes #1106)

frontend/src/__tests__/recommendations.test.ts

@@ -1,7 +1,7 @@
 /**
  * Recommendations module tests
  */
-import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, pickBestVariantPerCell, seedGlobalDefaults, effectiveMonthlySavings, effectiveSavingsPct, onDemandMonthly, groupRecsByCell, cellSummary, pageLevelRange, resetExpandedCells, resetAutoRefreshInFlight, scaleCost, formatCostForPeriod, periodSuffix, loadColumnVisibility, saveColumnVisibility, resetColumnVisibilityState, TOGGLEABLE_COLUMNS, COLUMN_DEFS, isHomogeneousSelection, renderUsageSparkline, loadColumnFilters, saveColumnFilters, resetColumnFiltersState, spGroupKey, groupSpCellKeys, formatCapacity } from '../recommendations';
+import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, pickBestVariantPerCell, seedGlobalDefaults, effectiveMonthlySavings, effectiveSavingsPct, displaySavingsPct, onDemandMonthly, groupRecsByCell, cellSummary, pageLevelRange, resetExpandedCells, resetAutoRefreshInFlight, scaleCost, formatCostForPeriod, periodSuffix, loadColumnVisibility, saveColumnVisibility, resetColumnVisibilityState, TOGGLEABLE_COLUMNS, COLUMN_DEFS, isHomogeneousSelection, renderUsageSparkline, loadColumnFilters, saveColumnFilters, resetColumnFiltersState, spGroupKey, groupSpCellKeys, formatCapacity } from '../recommendations';
 import type { CostPeriod } from '../state';
 
 // Mock the api module
@@ -4240,6 +4240,79 @@ describe('effectiveSavingsPct', () => {
   });
 });
 
+// CLI/GUI parity: displaySavingsPct prefers the provider-authoritative
+// `savings_percentage` (the same figure the CLI/reporter prints) over the
+// client-side reconstruction (effectiveSavingsPct), falling back only when no
+// provider % is present. This closes the gap where the GUI re-derived the %
+// (could drift, and returned an em-dash for AWS recs missing on_demand_cost despite
+// the provider reporting a real %, the #323 case).
+describe('displaySavingsPct (CLI/GUI parity)', () => {
+  const mk = (overrides: Partial<LocalRecommendation>): LocalRecommendation => ({
+    id: 'r',
+    provider: 'aws',
+    service: 'ec2',
+    resource_type: 'm5.large',
+    region: 'us-east-1',
+    count: 1,
+    term: 1,
+    savings: 45,
+    upfront_cost: 120,
+    monthly_cost: 60,
+    on_demand_cost: 150,
+    ...overrides,
+  } as unknown as LocalRecommendation);
+
+  test('(a) uses provider savings_percentage verbatim and ignores the computed value', () => {
+    // Provider reports 30%. The reconstruction would compute a DIFFERENT
+    // number (savings 45 / on_demand 150 = 30% here would coincide, so skew
+    // on_demand to force divergence): with on_demand_cost=300 the computed
+    // value is 45/300=15%, but the provider % (30) must win verbatim.
+    const r = mk({ savings_percentage: 30, savings: 45, on_demand_cost: 300 });
+    // Sanity: the computed fallback would have been 15%, proving the provider
+    // value is used, not the reconstruction.
+    expect(effectiveSavingsPct(r)).toBeCloseTo(15, 1);
+    expect(displaySavingsPct(r)).toBe(30);
+  });
+
+  test('(b) falls back to effectiveSavingsPct when savings_percentage is null', () => {
+    // No provider %, so the displayed value is the net-savings reconstruction.
+    // savings (net) = 45, on_demand_cost = 150 => 45 / 150 = 30%.
+    const r = mk({ savings_percentage: null, savings: 45, on_demand_cost: 150 });
+    expect(displaySavingsPct(r)).toBeCloseTo(30, 1);
+    expect(displaySavingsPct(r)).toBe(effectiveSavingsPct(r));
+  });
+
+  test('(b2) falls back when savings_percentage is undefined (pre-plumbing cached recs)', () => {
+    const r = mk({ savings: 48, on_demand_cost: 200 });
+    delete (r as { savings_percentage?: unknown }).savings_percentage;
+    expect(displaySavingsPct(r)).toBeCloseTo(24, 1); // 48 / 200
+    expect(displaySavingsPct(r)).toBe(effectiveSavingsPct(r));
+  });
+
+  test('(c) AWS rec missing on_demand_cost still shows the provider % (not an em-dash)', () => {
+    // Pre-parity: effectiveSavingsPct returns null for an AWS rec without
+    // on_demand_cost (#323), so the column rendered an em-dash. With the provider %
+    // plumbed through, displaySavingsPct returns the real number.
+    const r = mk({ provider: 'aws', savings_percentage: 22, on_demand_cost: null, monthly_cost: 60 });
+    // Sanity: the reconstruction fallback alone WOULD be null (the bug).
+    expect(effectiveSavingsPct(r)).toBeNull();
+    // With the provider %, the displayed value is the authoritative 22%.
+    expect(displaySavingsPct(r)).toBe(22);
+  });
+
+  test('ignores a non-finite savings_percentage and falls back', () => {
+    const r = mk({ savings_percentage: Number.NaN, savings: 45, on_demand_cost: 150 });
+    expect(displaySavingsPct(r)).toBeCloseTo(30, 1);
+  });
+
+  test('returns null when neither provider % nor a computable fallback exists', () => {
+    // No provider %, AWS rec with no on_demand_cost => effectiveSavingsPct is
+    // null (#323) => displaySavingsPct is null => column renders an em-dash.
+    const r = mk({ provider: 'aws', savings_percentage: null, on_demand_cost: null });
+    expect(displaySavingsPct(r)).toBeNull();
+  });
+});
+
 describe('onDemandMonthly', () => {
   // #330: onDemandMonthly returns the provider-supplied on_demand_cost
   // directly. No reconstruction fallback — when the provider didn't

frontend/src/recommendations.ts

@@ -808,11 +808,14 @@ const SORTABLE_NUMERIC_COLUMNS: Record<string, (r: LocalRecommendation) => numbe
     const period = state.getCostPeriod();
     return scaleCost(onDemandMonthly(r), period) ?? Number.POSITIVE_INFINITY;
   },
-  // effectiveSavingsPct returns null for term=0 / on_demand=0 / null monthly_cost.
-  // POSITIVE_INFINITY places null rows at the bottom in ascending order and
-  // at the top in descending — the least surprising behaviour for a savings
-  // column where "no data" rows should be de-emphasised.
-  effective_savings_pct: (r) => effectiveSavingsPct(r) ?? Number.POSITIVE_INFINITY,
+  // displaySavingsPct prefers the provider-authoritative % and falls back to
+  // effectiveSavingsPct; it returns null for term=0 / on_demand=0 / null
+  // monthly_cost when no provider % is present. POSITIVE_INFINITY places null
+  // rows at the bottom in ascending order and at the top in descending: the
+  // least surprising behaviour for a savings column where "no data" rows
+  // should be de-emphasised. Sorting on the displayed value keeps the order
+  // consistent with what the column shows.
+  effective_savings_pct: (r) => displaySavingsPct(r) ?? Number.POSITIVE_INFINITY,
   count: (r) => r.count,
   term: (r) => r.term,
 };
@@ -1159,7 +1162,9 @@ function cellScoreFor(
   if (column === 'effective_savings_pct') {
     const finite: number[] = [];
     for (const v of variants) {
-      const pct = effectiveSavingsPct(v);
+      // Prefer the provider-authoritative % (parity with the column render);
+      // fall back to the reconstruction for variants without one.
+      const pct = displaySavingsPct(v);
       if (pct != null) finite.push(pct);
     }
     // Best-case framing: highest savings pct is the cell's best pitch.
@@ -1330,6 +1335,32 @@ export function effectiveSavingsPct(r: LocalRecommendation): number | null {
   return (r.savings / onDemand) * 100;
 }
 
+/**
+ * Returns the effective savings percentage to DISPLAY (and sort) for a
+ * recommendation, preferring the provider-authoritative value over the
+ * client-side reconstruction.
+ *
+ * CLI/GUI parity: the CLI/reporter prints `rec.SavingsPercentage` straight
+ * from the provider (AWS EstimatedMonthlySavingsPercentage, Azure/GCP
+ * converter SavingsPercentage), but that figure used to be dropped at the API
+ * boundary, forcing the GUI to re-derive the % via effectiveSavingsPct. The
+ * recomputation could drift from the authoritative number and returned null
+ * (an em-dash) whenever on_demand_cost was missing even though the provider reported
+ * a real % (the #323 case). Now that `savings_percentage` is plumbed through,
+ * the GUI shows the same number the CLI shows.
+ *
+ * Preference rule: use `r.savings_percentage` when it is a finite, non-null
+ * number; otherwise fall back to the (fixed, net-savings) effectiveSavingsPct.
+ * Returns null only when both are unavailable (renders as an em-dash).
+ */
+export function displaySavingsPct(r: LocalRecommendation): number | null {
+  const provided = r.savings_percentage;
+  if (provided != null && Number.isFinite(provided)) {
+    return provided;
+  }
+  return effectiveSavingsPct(r);
+}
+
 /**
  * Returns the provider-reported on-demand monthly cost (`r.on_demand_cost`)
  * directly. Issue #330 — earlier behaviour reconstructed the value from
@@ -1685,8 +1716,10 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu
     // numeric filter targets what the user sees.
     case 'on_demand_monthly':    return scaleCost(onDemandMonthly(r), period) ?? Number.NaN;
     // Return NaN for null effective_savings_pct so any numeric predicate
-    // returns false rather than coincidentally matching 0.
-    case 'effective_savings_pct': return effectiveSavingsPct(r) ?? Number.NaN;
+    // returns false rather than coincidentally matching 0. Filter on the
+    // displayed value (provider % preferred) so typing the visible number
+    // matches the row.
+    case 'effective_savings_pct': return displaySavingsPct(r) ?? Number.NaN;
     // Categorical / visual columns shouldn't reach this branch; return NaN so any
     // numeric predicate returns false rather than coincidentally matching 0.
     case 'provider':
@@ -2816,7 +2849,7 @@ function buildVariantRowMarkup(
   const accountName = rec.cloud_account_id ? (accountNamesCache.get(rec.cloud_account_id) || rec.cloud_account_id) : '\u2014';
   const badge = renderSuppressionBadge(rec);
   const recId = escapeHtml(rec.id);
-  const pct = effectiveSavingsPct(rec);
+  const pct = displaySavingsPct(rec);
   const pctClass = pct !== null && pct < 0 ? ' class="effective-pct-negative"' : '';
   const pctText = pct === null ? '\u2014' : pct.toFixed(1) + '%';
   const nestedClass = isNested ? ' rec-variant-row' : '';
@@ -5063,9 +5096,10 @@ function renderPurchaseModalRow(idx: number, paymentSource: 'override' | 'rec' |
   effSavTd.appendChild(document.createTextNode(formatCurrency(effectiveMonthlySavings(rec))));
   tr.appendChild(effSavTd);
 
-  // Effective % (col 8): reuses effectiveSavingsPct helper.
+  // Effective % (col 8): reuses displaySavingsPct (provider % preferred,
+  // falls back to effectiveSavingsPct).
   const effPctTd = document.createElement('td');
-  const pct = effectiveSavingsPct(rec);
+  const pct = displaySavingsPct(rec);
   if (pct !== null && pct < 0) effPctTd.className = 'effective-pct-negative';
   effPctTd.appendChild(document.createTextNode(pct !== null ? pct.toFixed(1) + '%' : '—'));
   tr.appendChild(effPctTd);

frontend/src/types.ts

@@ -99,6 +99,16 @@ export interface LocalRecommendation {
   // `monthly_cost + savings + amortized_upfront`. When populated it's
   // preferred — see #274.
   on_demand_cost?: number | null;
+  // Provider-authoritative effective savings % reported directly by the cloud
+  // provider (AWS EstimatedMonthlySavingsPercentage, Azure/GCP converter
+  // SavingsPercentage), the same figure the CLI/reporter prints. When it is
+  // a finite, non-null number the effective-% column and its sort prefer it
+  // over the client-side reconstruction (effectiveSavingsPct), so the GUI
+  // shows the identical value the CLI shows and AWS recs missing
+  // on_demand_cost still render a real % instead of an em-dash (see #323).
+  // Optional/null when the provider did not report a percentage; the column
+  // then falls back to effectiveSavingsPct.
+  savings_percentage?: number | null;
   cloud_account_id?: string;
   // Populated by the scheduler when any active purchase_suppression
   // matches this rec's 6-tuple. The three fields drive the "recently

internal/config/types.go

@@ -371,12 +371,27 @@ type RecommendationRecord struct {
 	// denominator (e.g. Azure all-upfront recs where monthly_cost=$0
 	// collapses the denominator) don't inflate the displayed effective
 	// savings %. See #274.
-	OnDemandCost   *float64 `json:"on_demand_cost,omitempty" dynamodbav:"on_demand_cost,omitempty"`
-	Selected       bool     `json:"selected" dynamodbav:"selected"`
-	Purchased      bool     `json:"purchased" dynamodbav:"purchased"`
-	PurchaseID     string   `json:"purchase_id,omitempty" dynamodbav:"purchase_id,omitempty"`
-	Error          string   `json:"error,omitempty" dynamodbav:"error,omitempty"`
-	CloudAccountID *string  `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"`
+	OnDemandCost *float64 `json:"on_demand_cost,omitempty" dynamodbav:"on_demand_cost,omitempty"`
+	// SavingsPercentage is the provider-authoritative effective savings %
+	// reported directly by the cloud provider (AWS Cost Explorer
+	// `EstimatedMonthlySavingsPercentage`, Azure / GCP converters' computed
+	// SavingsPercentage). It is the same figure the CLI/reporter prints
+	// verbatim (internal/reporter/reporter.go); persisting it lets the GUI
+	// show the identical number instead of re-deriving it client-side from
+	// savings / on-demand. Persisted via the recommendations row's JSONB
+	// `payload` column; no DDL needed.
+	//
+	// nil means the provider did not report a percentage; the frontend then
+	// falls back to the client-side reconstruction (effectiveSavingsPct).
+	// When non-nil, the frontend prefers this value so the displayed % cannot
+	// drift from the provider's authoritative number and AWS recs missing
+	// on_demand_cost still render a real % rather than an em-dash (see #323).
+	SavingsPercentage *float64 `json:"savings_percentage" dynamodbav:"savings_percentage,omitempty"`
+	Selected          bool     `json:"selected" dynamodbav:"selected"`
+	Purchased         bool     `json:"purchased" dynamodbav:"purchased"`
+	PurchaseID        string   `json:"purchase_id,omitempty" dynamodbav:"purchase_id,omitempty"`
+	Error             string   `json:"error,omitempty" dynamodbav:"error,omitempty"`
+	CloudAccountID    *string  `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"`
 	// SuppressedCount is the cumulative count already committed against
 	// this recommendation's 6-tuple (account, provider, service, region,
 	// resource_type, engine) within the active grace window. The

internal/scheduler/scheduler.go

@@ -1351,23 +1351,24 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide
 			rec.ResourceType, engine, term, rec.PaymentOption)
 
 		records = append(records, config.RecommendationRecord{
-			ID:           recordID,
-			Provider:     providerName,
-			Service:      string(rec.Service),
-			Region:       rec.Region,
-			ResourceType: rec.ResourceType,
-			Engine:       engine,
-			Details:      detailsBlob, // full ServiceDetails payload (issue #453)
-			Count:        rec.Count,
-			Term:         term,
-			Payment:      rec.PaymentOption,
-			UpfrontCost:  rec.CommitmentCost,
-			MonthlyCost:  rec.RecurringMonthlyCost, // nil when provider API didn't return a monthly breakdown
-			Savings:      rec.EstimatedSavings,
-			OnDemandCost: nonZeroPtr(rec.OnDemandCost), // nil when provider API didn't return a baseline; frontend falls back to reconstruction (#274)
-			UsageHistory: rec.UsageHistory,             // daily coverage pcts (nil when provider not yet wired; see #239)
-			Selected:     true,                         // Default to selected
-			Purchased:    false,
+			ID:                recordID,
+			Provider:          providerName,
+			Service:           string(rec.Service),
+			Region:            rec.Region,
+			ResourceType:      rec.ResourceType,
+			Engine:            engine,
+			Details:           detailsBlob, // full ServiceDetails payload (issue #453)
+			Count:             rec.Count,
+			Term:              term,
+			Payment:           rec.PaymentOption,
+			UpfrontCost:       rec.CommitmentCost,
+			MonthlyCost:       rec.RecurringMonthlyCost, // nil when provider API didn't return a monthly breakdown
+			Savings:           rec.EstimatedSavings,
+			OnDemandCost:      nonZeroPtr(rec.OnDemandCost),      // nil when provider API didn't return a baseline; frontend falls back to reconstruction (#274)
+			SavingsPercentage: nonZeroPtr(rec.SavingsPercentage), // provider-authoritative %; nil when not reported, frontend falls back to effectiveSavingsPct
+			UsageHistory:      rec.UsageHistory,                  // daily coverage pcts (nil when provider not yet wired; see #239)
+			Selected:          true,                              // Default to selected
+			Purchased:         false,
 		})
 	}
 

internal/scheduler/scheduler_test.go

@@ -1608,6 +1608,56 @@ func TestScheduler_ConvertRecommendations_OnDemandCost(t *testing.T) {
 		"OnDemandCost=0 from the provider must round-trip as nil, not as a pointer to 0.0")
 }
 
+// TestScheduler_ConvertRecommendations_SavingsPercentage pins the CLI/GUI
+// parity fix: the provider-authoritative SavingsPercentage (the same figure
+// the CLI/reporter prints) must be carried onto the RecommendationRecord so
+// the frontend can display it verbatim instead of re-deriving it. A provider
+// value of 0 (not reported) must round-trip as nil, never as a pointer to
+// 0.0, mirroring OnDemandCost so the frontend's "is this populated?" branch
+// stays accurate and falls back to the client-side reconstruction.
+func TestScheduler_ConvertRecommendations_SavingsPercentage(t *testing.T) {
+	scheduler := &Scheduler{}
+
+	recommendations := []common.Recommendation{
+		// AWS rec with a provider-reported percentage in the real data band.
+		{
+			Provider: common.ProviderAWS, Service: common.ServiceEC2,
+			Region: "us-east-1", ResourceType: "m5.large",
+			Count: 2, Term: "1yr", PaymentOption: "partial-upfront",
+			OnDemandCost: 150.0, CommitmentCost: 120.0, EstimatedSavings: 45.0,
+			SavingsPercentage: 30.0,
+		},
+		// Azure rec with a higher provider-reported percentage.
+		{
+			Provider: common.ProviderAzure, Service: common.ServiceCompute,
+			Region: "eastus", ResourceType: "Standard_D11_v2",
+			Count: 1, Term: "3yr", PaymentOption: "all-upfront",
+			OnDemandCost: 122.64, CommitmentCost: 1050.0, EstimatedSavings: 58.0,
+			SavingsPercentage: 47.3,
+		},
+		// Provider did not report a percentage (0); must round-trip as nil.
+		{
+			Provider: common.ProviderGCP, Service: common.ServiceCompute,
+			Region: "us-central1", ResourceType: "n2-standard-4",
+			Count: 1, Term: "3yr", PaymentOption: "no-upfront",
+			OnDemandCost: 200.0, CommitmentCost: 0, EstimatedSavings: 44.0,
+			SavingsPercentage: 0,
+		},
+	}
+
+	records := scheduler.convertRecommendations(recommendations, "aws")
+	require.Len(t, records, 3)
+
+	require.NotNil(t, records[0].SavingsPercentage)
+	assert.InDelta(t, 30.0, *records[0].SavingsPercentage, 0.001)
+
+	require.NotNil(t, records[1].SavingsPercentage)
+	assert.InDelta(t, 47.3, *records[1].SavingsPercentage, 0.001)
+
+	assert.Nil(t, records[2].SavingsPercentage,
+		"SavingsPercentage=0 from the provider must round-trip as nil, not as a pointer to 0.0")
+}
+
 // TestScheduler_ConvertRecommendations_IDUniqueness pins issue #187 +
 // #188: the rec ID must include term, account, and engine — not just
 // (provider, service, region, resource_type, payment) — otherwise