fix(recs): disambiguate min-savings dollar (API) vs percent (CLI) filters

frontend/src/__tests__/api.test.ts

@@ -709,19 +709,69 @@ describe('Recommendations API', () => {
       expect(fetchMock).toHaveBeenCalledWith('/api/recommendations', expect.anything());
     });
 
-    test('applies filters to query string', async () => {
+    // Regression test for issue #1089: minSavingsUsd (dollar) and minSavingsPct
+    // (percentage) must map to distinct query params -- min_savings_usd and
+    // min_savings_pct -- and must NEVER both map to the same legacy `min_savings`
+    // key. Conflating them would silently treat a percentage as a dollar amount
+    // or vice versa.
+    test('minSavingsUsd maps to min_savings_usd (dollar) query param', async () => {
+      await getRecommendations({
+        provider: 'aws',
+        service: 'ec2',
+        region: 'us-east-1',
+        minSavingsUsd: 100
+      });
+
+      const url = fetchMock.mock.calls[0][0] as string;
+      expect(url).toContain('provider=aws');
+      expect(url).toContain('service=ec2');
+      expect(url).toContain('region=us-east-1');
+      // Dollar filter uses the distinct usd-suffixed key.
+      expect(url).toContain('min_savings_usd=100');
+      // Must NOT appear as the bare (ambiguous) key or as the pct key.
+      expect(url).not.toContain('min_savings=');
+      expect(url).not.toContain('min_savings_pct=');
+    });
+
+    test('minSavingsPct maps to min_savings_pct (percentage) query param', async () => {
+      await getRecommendations({
+        minSavingsPct: 30
+      });
+
+      const url = fetchMock.mock.calls[0][0] as string;
+      // Percentage filter uses the distinct pct-suffixed key.
+      expect(url).toContain('min_savings_pct=30');
+      // Must NOT appear as the bare (ambiguous) key or as the usd key.
+      expect(url).not.toContain('min_savings=');
+      expect(url).not.toContain('min_savings_usd=');
+    });
+
+    test('usd and pct filters can be combined and remain distinct', async () => {
+      await getRecommendations({
+        minSavingsUsd: 50,
+        minSavingsPct: 20
+      });
+
+      const url = fetchMock.mock.calls[0][0] as string;
+      expect(url).toContain('min_savings_usd=50');
+      expect(url).toContain('min_savings_pct=20');
+      // The same value "50" for usd must not appear on the pct key and vice versa.
+      expect(url).not.toContain('min_savings_pct=50');
+      expect(url).not.toContain('min_savings_usd=20');
+      expect(url).not.toContain('min_savings=');
+    });
+
+    test('applies other filters to query string', async () => {
       await getRecommendations({
         provider: 'aws',
         service: 'ec2',
         region: 'us-east-1',
-        minSavings: 100
       });
 
       const url = fetchMock.mock.calls[0][0] as string;
       expect(url).toContain('provider=aws');
       expect(url).toContain('service=ec2');
       expect(url).toContain('region=us-east-1');
-      expect(url).toContain('min_savings=100');
     });
   });
 

frontend/src/api/recommendations.ts

@@ -13,7 +13,10 @@ export async function getRecommendations(filters: RecommendationFilters = {}): P
   if (filters.provider) params.set('provider', filters.provider);
   if (filters.service) params.set('service', filters.service);
   if (filters.region) params.set('region', filters.region);
-  if (filters.minSavings) params.set('min_savings', String(filters.minSavings));
+  // min_savings_usd: absolute dollar floor (distinct from the CLI --min-savings-pct percentage filter)
+  if (filters.minSavingsUsd) params.set('min_savings_usd', String(filters.minSavingsUsd));
+  // min_savings_pct: percentage floor (mirrors the CLI --min-savings-pct semantics)
+  if (filters.minSavingsPct) params.set('min_savings_pct', String(filters.minSavingsPct));
   if (filters.account_ids && filters.account_ids.length > 0) params.set('account_ids', filters.account_ids.join(','));
 
   const queryString = params.toString();

frontend/src/api/types.ts

@@ -175,7 +175,24 @@ export interface RecommendationFilters {
   provider?: Provider | '';
   service?: string;
   region?: string;
-  minSavings?: number;
+  /**
+   * Minimum absolute dollar savings per month. Sent to the API as
+   * `min_savings_usd`. 0 or absent means no floor.
+   *
+   * IMPORTANT: this is a DOLLAR amount, not a percentage. The CLI equivalent
+   * is `--min-savings-pct` which filters by percentage -- these are two
+   * distinct concepts. Use `minSavingsPct` to filter by percentage.
+   */
+  minSavingsUsd?: number;
+  /**
+   * Minimum effective savings percentage (0-100 scale). Sent to the API as
+   * `min_savings_pct`. 0 or absent means no floor.
+   *
+   * IMPORTANT: this is a PERCENTAGE (0-100), not a dollar amount. The CLI
+   * `--min-savings-pct` flag uses the same semantics. Use `minSavingsUsd`
+   * to filter by an absolute dollar floor.
+   */
+  minSavingsPct?: number;
   account_ids?: string[];
 }
 

internal/api/handler_recommendations.go

@@ -12,23 +12,41 @@ import (
 	"github.com/aws/aws-lambda-go/events"
 )
 
-// Recommendations handlers
-func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*RecommendationsResponse, error) {
-	// Require view:recommendations permission
-	session, err := h.requirePermission(ctx, req, "view", "recommendations")
-	if err != nil {
-		return nil, err
-	}
-
-	// Validate input parameters to prevent injection attacks
+// parseRecommendationFilter validates all query parameters and builds the DB
+// filter. It centralises the parameter-validation logic so getRecommendations
+// stays below the cyclomatic-complexity threshold.
+//
+// account_ids filter semantics (issue #211):
+//
+//	account_ids param  | cloud_account_id row | Result
+//	-------------------|----------------------|----------------------------
+//	absent             | any (incl. NULL)     | included (no SQL filter)
+//	non-empty          | NULL                 | excluded (legacy rows are
+//	                   |                      |   not "in any account")
+//	non-empty          | matches one of IDs   | included
+//	non-empty          | doesn't match        | excluded
+//	non-empty, contains| matches (account is  | excluded — enforced by
+//	  a disabled ID    |   disabled)          |   filterRecommendationsByAllowedAccounts
+//	                   |                      |   below, NOT by the SQL filter
+//
+// The SQL contract lives in config.buildRecommendationFilter
+// (store_postgres_recommendations.go). The disabled-account case is enforced by
+// filterRecommendationsByAllowedAccounts after the DB read.
+//
+// min_savings_usd: absolute dollar floor on monthly savings.
+// min_savings_pct: effective savings percentage floor (0-100 scale).
+// Both are optional; absent or "0" means no floor. Fractions are rejected (a
+// user typing "30%" expects a percentage, not $30.5).
+func parseRecommendationFilter(params map[string]string) (config.RecommendationFilter, error) {
+	// Validate input parameters to prevent injection attacks.
 	if err := validateProvider(params["provider"]); err != nil {
-		return nil, err
+		return config.RecommendationFilter{}, err
 	}
 	if err := validateServiceName(params["service"]); err != nil {
-		return nil, err
+		return config.RecommendationFilter{}, err
 	}
 	if err := validateRegion(params["region"]); err != nil {
-		return nil, err
+		return config.RecommendationFilter{}, err
 	}
 
 	// parseAccountIDs splits, trims, and UUID-validates the comma-separated
@@ -37,33 +55,44 @@ func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunc
 	// MaxAccountIDsPerRequest (200). See validation.go::parseAccountIDs.
 	accountIDs, err := parseAccountIDs(params["account_ids"])
 	if err != nil {
-		return nil, NewClientError(400, err.Error())
+		return config.RecommendationFilter{}, NewClientError(400, err.Error())
+	}
+
+	minSavingsUSD, err := parseMinSavingsParam(params["min_savings_usd"], "min_savings_usd")
+	if err != nil {
+		return config.RecommendationFilter{}, err
+	}
+	minSavingsPct, err := parseMinSavingsParam(params["min_savings_pct"], "min_savings_pct")
+	if err != nil {
+		return config.RecommendationFilter{}, err
 	}
+	if minSavingsPct < 0 || minSavingsPct > 100 {
+		return config.RecommendationFilter{}, NewClientError(400, "min_savings_pct must be between 0 and 100")
+	}
+
+	return config.RecommendationFilter{
+		Provider:      params["provider"],
+		Service:       params["service"],
+		Region:        params["region"],
+		AccountIDs:    accountIDs,
+		MinSavingsUSD: minSavingsUSD,
+		MinSavingsPct: minSavingsPct,
+	}, nil
+}
 
-	// account_ids filter semantics (issue #211):
-	//
-	//   account_ids param  | cloud_account_id row | Result
-	//   -------------------|----------------------|----------------------------
-	//   absent             | any (incl. NULL)     | included (no SQL filter)
-	//   non-empty          | NULL                 | excluded (legacy rows are
-	//                      |                      |   not "in any account")
-	//   non-empty          | matches one of IDs   | included
-	//   non-empty          | doesn't match        | excluded
-	//   non-empty, contains| matches (account is  | excluded — enforced by
-	//     a disabled ID    |   disabled)          |   filterRecommendationsByAllowedAccounts
-	//                      |                      |   below, NOT by the SQL filter
-	//
-	// The SQL contract lives in config.buildRecommendationFilter
-	// (store_postgres_recommendations.go). The disabled-account case is
-	// enforced by filterRecommendationsByAllowedAccounts after the DB read.
+// Recommendations handlers
+func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*RecommendationsResponse, error) {
+	// Require view:recommendations permission
+	session, err := h.requirePermission(ctx, req, "view", "recommendations")
+	if err != nil {
+		return nil, err
+	}
 
-	// Translate query string → DB-level filter. ListRecommendations
-	// pushes these into the WHERE clause so the cache does the pruning.
-	filter := config.RecommendationFilter{
-		Provider:   params["provider"],
-		Service:    params["service"],
-		Region:     params["region"],
-		AccountIDs: accountIDs,
+	// Translate query string to DB-level filter. ListRecommendations pushes
+	// these into the WHERE clause so the cache does the pruning.
+	filter, err := parseRecommendationFilter(params)
+	if err != nil {
+		return nil, err
 	}
 
 	recommendations, err := h.scheduler.ListRecommendations(ctx, filter)

internal/api/handler_recommendations_test.go

@@ -454,6 +454,126 @@ func TestBuildRecommendationsResponse_CapacityFields(t *testing.T) {
 	}
 }
 
+// TestGetRecommendations_MinSavingsFilters is the regression test for issue #1089:
+// it asserts that min_savings_usd (dollar floor) and min_savings_pct (percentage
+// floor) are distinct parameters with separate semantics and cannot be confused.
+//
+// Key invariants proven:
+//   - A dollar value passed as min_savings_usd does NOT conflate with a percentage
+//     (e.g. min_savings_usd=30 means "$30", not "30%").
+//   - A percentage passed as min_savings_pct does NOT conflate with a dollar amount.
+//   - Both can be specified independently and are applied independently.
+//   - min_savings_pct validates the range 0-100.
+//   - Non-numeric values are rejected with 400.
+func TestGetRecommendations_MinSavingsFilters(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("min_savings_usd filter is wired to RecommendationFilter.MinSavingsUSD not MinSavingsPct", func(t *testing.T) {
+		mockScheduler := new(MockScheduler)
+		mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool {
+			// Dollar filter must be set; percentage filter must be zero.
+			return f.MinSavingsUSD == 30 && f.MinSavingsPct == 0
+		})).Return([]config.RecommendationRecord{}, nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
+
+		handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		params := map[string]string{"min_savings_usd": "30"}
+
+		result, err := handler.getRecommendations(ctx, req, params)
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+	})
+
+	t.Run("min_savings_pct filter is wired to RecommendationFilter.MinSavingsPct not MinSavingsUSD", func(t *testing.T) {
+		mockScheduler := new(MockScheduler)
+		mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool {
+			// Percentage filter must be set; dollar filter must be zero.
+			return f.MinSavingsPct == 30 && f.MinSavingsUSD == 0
+		})).Return([]config.RecommendationRecord{}, nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
+
+		handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		// Sending "30" as min_savings_pct must NOT be treated as $30 by the server.
+		params := map[string]string{"min_savings_pct": "30"}
+
+		result, err := handler.getRecommendations(ctx, req, params)
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+	})
+
+	t.Run("both filters can be combined independently", func(t *testing.T) {
+		mockScheduler := new(MockScheduler)
+		mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool {
+			return f.MinSavingsUSD == 50 && f.MinSavingsPct == 20
+		})).Return([]config.RecommendationRecord{}, nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
+
+		handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		params := map[string]string{"min_savings_usd": "50", "min_savings_pct": "20"}
+
+		result, err := handler.getRecommendations(ctx, req, params)
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+	})
+
+	t.Run("min_savings_pct above 100 returns 400", func(t *testing.T) {
+		handler := &Handler{apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		params := map[string]string{"min_savings_pct": "101"}
+
+		result, err := handler.getRecommendations(ctx, req, params)
+		require.Error(t, err)
+		assert.Nil(t, result)
+		ce, ok := IsClientError(err)
+		require.True(t, ok)
+		assert.Equal(t, 400, ce.code)
+	})
+
+	t.Run("min_savings_usd with non-numeric value returns 400", func(t *testing.T) {
+		handler := &Handler{apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		params := map[string]string{"min_savings_usd": "thirty"}
+
+		result, err := handler.getRecommendations(ctx, req, params)
+		require.Error(t, err)
+		assert.Nil(t, result)
+		ce, ok := IsClientError(err)
+		require.True(t, ok)
+		assert.Equal(t, 400, ce.code)
+	})
+
+	t.Run("min_savings_pct with non-numeric value returns 400", func(t *testing.T) {
+		handler := &Handler{apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		params := map[string]string{"min_savings_pct": "thirty"}
+
+		result, err := handler.getRecommendations(ctx, req, params)
+		require.Error(t, err)
+		assert.Nil(t, result)
+		ce, ok := IsClientError(err)
+		require.True(t, ok)
+		assert.Equal(t, 400, ce.code)
+	})
+
+	t.Run("absent filters pass through zero values in RecommendationFilter", func(t *testing.T) {
+		mockScheduler := new(MockScheduler)
+		mockScheduler.On("ListRecommendations", ctx, mock.MatchedBy(func(f config.RecommendationFilter) bool {
+			return f.MinSavingsUSD == 0 && f.MinSavingsPct == 0
+		})).Return([]config.RecommendationRecord{}, nil)
+		t.Cleanup(func() { mockScheduler.AssertExpectations(t) })
+
+		handler := &Handler{scheduler: mockScheduler, apiKey: "test-key"}
+		req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}
+		// No min_savings params at all.
+		result, err := handler.getRecommendations(ctx, req, map[string]string{})
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+	})
+}
+
 func TestConfidenceBucketFor(t *testing.T) {
 	cases := []struct {
 		name    string

internal/api/validation.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/mail"
 	"regexp"
+	"strconv"
 	"strings"
 	"unicode"
 
@@ -587,3 +588,26 @@ func decodeBase64Password(encoded string) (string, error) {
 	}
 	return string(decoded), nil
 }
+
+// parseMinSavingsParam parses a numeric savings-floor query parameter.
+// Returns (0, nil) when the parameter is absent or empty (no floor).
+// Returns 400 when the value is present but not a valid non-negative
+// integer or float. Fractional values are allowed (e.g. "12.5") since
+// savings floors can be sub-dollar amounts.
+//
+// paramName is included in the error message so callers can distinguish
+// min_savings_usd vs min_savings_pct errors in client logs.
+func parseMinSavingsParam(raw string, paramName string) (float64, error) {
+	raw = strings.TrimSpace(raw)
+	if raw == "" || raw == "0" {
+		return 0, nil
+	}
+	v, err := strconv.ParseFloat(raw, 64)
+	if err != nil {
+		return 0, NewClientError(400, fmt.Sprintf("%s must be a non-negative number", paramName))
+	}
+	if v < 0 {
+		return 0, NewClientError(400, fmt.Sprintf("%s must be non-negative", paramName))
+	}
+	return v, nil
+}