fix(config/db): first slice of the 2026-07-28 review findings (hidden_by min_count, RI-exchange limits, credential not-found, DB env parsing)

internal/api/handler_config_test.go

@@ -1007,6 +1007,33 @@ func TestHandler_updateConfig_ValidationError(t *testing.T) {
 	assert.Contains(t, err.Error(), "validation error")
 }
 
+// TestUpdateConfig_RejectsOutOfRangeRIExchangeLimits covers #120: PUT
+// /api/config must refuse a negative RI-exchange cap or an out-of-range
+// utilization threshold instead of storing it. The principal holds
+// execute:ri-exchange, so the #1765 auto-mode gate never refuses these writes
+// and only validation can.
+func TestUpdateConfig_RejectsOutOfRangeRIExchangeLimits(t *testing.T) {
+	for body, field := range map[string]string{
+		`{"ri_exchange_max_daily_usd": -1}`:           "ri_exchange_max_daily_usd",
+		`{"ri_exchange_max_per_exchange_usd": -250}`:  "ri_exchange_max_per_exchange_usd",
+		`{"ri_exchange_utilization_threshold": 101}`:  "ri_exchange_utilization_threshold",
+		`{"ri_exchange_utilization_threshold": -0.5}`: "ri_exchange_utilization_threshold",
+	} {
+		t.Run(body, func(t *testing.T) {
+			r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, disarmedConfig())
+
+			_, err := r.Route(context.Background(), "PUT", genericPath, autoGateTokenReq(body))
+
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected a client error, got %v", err)
+			assert.Equal(t, 400, ce.code)
+			assert.Contains(t, err.Error(), field)
+			assert.False(t, wrote(store), "a rejected write must not reach the store")
+		})
+	}
+}
+
 func TestHandler_updateConfig_SaveError(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)

internal/api/types.go

@@ -498,7 +498,7 @@ type RecommendationDetailResponse struct {
 	ProvenanceNote   string       `json:"provenance_note"`
 	// HiddenBy is non-nil when the rec is filtered out by an account-service
 	// override (issue #214). Each element names one failing dimension:
-	// "enabled=false", "engine", "region", or "resource_type". The frontend
+	// "enabled=false", "engine", "region", "resource_type" or "min_count". The frontend
 	// renders a "hidden by your override" banner when this field is present.
 	// Absent (null) means the rec is fully visible.
 	HiddenBy []string `json:"hidden_by,omitempty"`

internal/config/interfaces.go

@@ -531,6 +531,8 @@ type StoreInterface interface {
 
 	// Account credentials (encrypted blobs; never returned via API)
 	SaveAccountCredential(ctx context.Context, accountID, credentialType, encryptedBlob string) error
+	// GetAccountCredential returns ErrNotFound when the account has no
+	// credential of credentialType.
 	GetAccountCredential(ctx context.Context, accountID, credentialType string) (string, error)
 	DeleteAccountCredentials(ctx context.Context, accountID string) error
 	HasAccountCredentials(ctx context.Context, accountID string) (bool, error)

internal/config/store_postgres.go

@@ -4201,6 +4201,9 @@ func (s *PostgresStore) SaveAccountCredential(ctx context.Context, accountID, cr
 }
 
 // GetAccountCredential returns the encrypted blob for an account credential.
+// A missing row returns ErrNotFound, never ("", nil): that is what an empty
+// stored blob returns, and a caller that read an empty result as "no
+// credential, use the ambient identity" would act in the wrong account (#120).
 func (s *PostgresStore) GetAccountCredential(ctx context.Context, accountID, credentialType string) (string, error) {
 	var blob string
 	err := s.db.QueryRow(ctx,
@@ -4209,7 +4212,7 @@ func (s *PostgresStore) GetAccountCredential(ctx context.Context, accountID, cre
 	).Scan(&blob)
 	if err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {
-			return "", nil
+			return "", ErrNotFound
 		}
 		return "", fmt.Errorf("failed to get account credential: %w", err)
 	}

internal/config/store_postgres_account_credential_integration_test.go

@@ -0,0 +1,51 @@
+//go:build integration
+// +build integration
+
+package config
+
+// Real-DB test for GetAccountCredential's missing-row contract (#120): a
+// missing credential is ErrNotFound, and only a stored empty blob reads back
+// as ("", nil).
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+)
+
+func TestGetAccountCredential_MissingRowIsNotFoundPostgres(t *testing.T) {
+	ctx := context.Background()
+	container := testhelpers.RequireMigratedDatabase(ctx, t, getTestMigrationsPath())
+	store := NewPostgresStore(container.DB)
+
+	account := &CloudAccount{Name: "credential fixture", Provider: "aws", ExternalID: "111111111111", Enabled: true}
+	require.NoError(t, store.CreateCloudAccount(ctx, account))
+
+	blob, err := store.GetAccountCredential(ctx, account.ID, "aws_access_keys")
+	require.ErrorIs(t, err, ErrNotFound, "no row for the account must be ErrNotFound")
+	assert.Empty(t, blob)
+
+	// A stored empty blob is a row: it reads back as ("", nil), which is
+	// exactly why a missing row must not return the same pair.
+	require.NoError(t, store.SaveAccountCredential(ctx, account.ID, "aws_role_arn", ""))
+	blob, err = store.GetAccountCredential(ctx, account.ID, "aws_role_arn")
+	require.NoError(t, err)
+	assert.Empty(t, blob)
+
+	// Another type's row does not satisfy the lookup.
+	_, err = store.GetAccountCredential(ctx, account.ID, "aws_access_keys")
+	require.ErrorIs(t, err, ErrNotFound)
+
+	require.NoError(t, store.SaveAccountCredential(ctx, account.ID, "aws_access_keys", "ciphertext"))
+	blob, err = store.GetAccountCredential(ctx, account.ID, "aws_access_keys")
+	require.NoError(t, err)
+	assert.Equal(t, "ciphertext", blob)
+
+	require.NoError(t, store.DeleteAccountCredentials(ctx, account.ID))
+	_, err = store.GetAccountCredential(ctx, account.ID, "aws_access_keys")
+	require.ErrorIs(t, err, ErrNotFound, "a deleted credential must read back as ErrNotFound")
+}

internal/config/store_postgres_pgxmock_test.go

@@ -2075,8 +2075,9 @@ func TestPGXMock_GetAccountCredential_NotFound(t *testing.T) {
 	mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).
 		WillReturnError(errNoRows())
 
+	// #120: a missing row is ErrNotFound, distinct from an empty stored blob.
 	blob, err := store.GetAccountCredential(ctx, "acct-1", "aws_access_keys")
-	require.NoError(t, err)
+	require.ErrorIs(t, err, ErrNotFound)
 	assert.Equal(t, "", blob)
 }
 

internal/config/validation.go

@@ -302,9 +302,44 @@ func (c *GlobalConfig) Validate() error {
 	if err := validateOfferingClass(c.OfferingClass); err != nil {
 		return err
 	}
+	if err := c.validateRIExchangeLimits(); err != nil {
+		return err
+	}
 	return c.validateRecommendationsFields()
 }
 
+// MaxRIExchangeUtilizationThreshold is the upper bound of
+// ri_exchange_utilization_threshold, a percentage.
+const MaxRIExchangeUtilizationThreshold = 100.0
+
+// validateRIExchangeLimits rejects an RI-exchange money cap that is negative
+// or not finite, and a utilization threshold outside
+// [0, MaxRIExchangeUtilizationThreshold] (#120). PUT /api/config unmarshals
+// onto the whole GlobalConfig, so without this a negative daily cap reached
+// the store unchecked; the bounds match the ones the dedicated
+// PUT /api/ri-exchange/config body already enforces, so both routes accept
+// the same values. 0 stays valid for every field: a 0 cap is the "not
+// configured" state the exchange guard refuses to run under.
+func (c *GlobalConfig) validateRIExchangeLimits() error {
+	caps := []struct {
+		name  string
+		value float64
+	}{
+		{"ri_exchange_max_per_exchange_usd", c.RIExchangeMaxPerExchangeUSD},
+		{"ri_exchange_max_daily_usd", c.RIExchangeMaxDailyUSD},
+	}
+	for _, cp := range caps {
+		if math.IsNaN(cp.value) || math.IsInf(cp.value, 0) || cp.value < 0 {
+			return fmt.Errorf("%s must be a finite amount >= 0, got: %v", cp.name, cp.value)
+		}
+	}
+	t := c.RIExchangeUtilizationThreshold
+	if math.IsNaN(t) || t < 0 || t > MaxRIExchangeUtilizationThreshold {
+		return fmt.Errorf("ri_exchange_utilization_threshold must be between 0 and %v, got: %v", MaxRIExchangeUtilizationThreshold, t)
+	}
+	return nil
+}
+
 // validateScheduleAndNotifications validates collection_schedule and
 // notification_days_before. Extracted to keep Validate's cyclomatic
 // complexity under the project limit.

internal/config/validation_test.go

@@ -331,6 +331,44 @@ func TestGlobalConfig_Validate(t *testing.T) {
 	}
 }
 
+// TestGlobalConfig_Validate_RIExchangeLimits covers #120: PUT /api/config
+// reaches the store through GlobalConfig.Validate, which used to accept a
+// negative or non-finite RI-exchange cap and an out-of-range threshold.
+func TestGlobalConfig_Validate_RIExchangeLimits(t *testing.T) {
+	tests := []struct {
+		name   string
+		mutate func(c *GlobalConfig)
+		errMsg string
+	}{
+		{"zero caps and threshold are valid", func(c *GlobalConfig) {}, ""},
+		{"positive caps and threshold 100 are valid", func(c *GlobalConfig) {
+			c.RIExchangeMaxPerExchangeUSD = 500
+			c.RIExchangeMaxDailyUSD = 2000
+			c.RIExchangeUtilizationThreshold = 100
+		}, ""},
+		{"negative daily cap", func(c *GlobalConfig) { c.RIExchangeMaxDailyUSD = -1 }, "ri_exchange_max_daily_usd"},
+		{"negative per-exchange cap", func(c *GlobalConfig) { c.RIExchangeMaxPerExchangeUSD = -0.01 }, "ri_exchange_max_per_exchange_usd"},
+		{"NaN daily cap", func(c *GlobalConfig) { c.RIExchangeMaxDailyUSD = math.NaN() }, "ri_exchange_max_daily_usd"},
+		{"infinite per-exchange cap", func(c *GlobalConfig) { c.RIExchangeMaxPerExchangeUSD = math.Inf(1) }, "ri_exchange_max_per_exchange_usd"},
+		{"negative threshold", func(c *GlobalConfig) { c.RIExchangeUtilizationThreshold = -5 }, "ri_exchange_utilization_threshold"},
+		{"threshold over 100", func(c *GlobalConfig) { c.RIExchangeUtilizationThreshold = 100.5 }, "ri_exchange_utilization_threshold"},
+		{"NaN threshold", func(c *GlobalConfig) { c.RIExchangeUtilizationThreshold = math.NaN() }, "ri_exchange_utilization_threshold"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := GlobalConfig{DefaultTerm: 3}
+			tt.mutate(&cfg)
+			err := cfg.Validate()
+			if tt.errMsg == "" {
+				assert.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), tt.errMsg)
+		})
+	}
+}
+
 func TestServiceConfig_Validate(t *testing.T) {
 	tests := []struct {
 		name    string

internal/database/config.go

@@ -1,6 +1,7 @@
 package database
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"strconv"
@@ -28,12 +29,17 @@ type Config struct {
 	AutoMigrate       bool
 }
 
-// LoadFromEnv loads database configuration from environment variables.
+// LoadFromEnv loads database configuration from environment variables. A set
+// but malformed numeric, boolean or duration variable is an error, like an
+// out-of-range one: falling back to the default would let a typo such as
+// DB_MAX_CONNECTIONS="100 " run the pool at a quarter of its intended size
+// with nothing in the logs (#120).
 func LoadFromEnv() (*Config, error) {
+	env := &envParser{}
 	config := &Config{
 		// Required fields
 		Host:     getEnv("DB_HOST", "localhost"),
-		Port:     getEnvInt("DB_PORT", 5432),
+		Port:     env.getEnvInt("DB_PORT", 5432),
 		Database: getEnv("DB_NAME", "cudly"),
 		User:     getEnv("DB_USER", "cudly"),
 
@@ -45,20 +51,23 @@ func LoadFromEnv() (*Config, error) {
 		SSLMode: getEnv("DB_SSL_MODE", "require"),
 
 		// Connection pool defaults
-		MaxConnections:    getEnvInt("DB_MAX_CONNECTIONS", 25),
-		MinConnections:    getEnvInt("DB_MIN_CONNECTIONS", 2),
-		MaxConnLifetime:   getEnvDuration("DB_MAX_CONN_LIFETIME", time.Hour),
-		MaxConnIdleTime:   getEnvDuration("DB_MAX_CONN_IDLE_TIME", 30*time.Minute),
-		HealthCheckPeriod: getEnvDuration("DB_HEALTH_CHECK_PERIOD", time.Minute),
-		ConnectTimeout:    getEnvDuration("DB_CONNECT_TIMEOUT", 10*time.Second),
+		MaxConnections:    env.getEnvInt("DB_MAX_CONNECTIONS", 25),
+		MinConnections:    env.getEnvInt("DB_MIN_CONNECTIONS", 2),
+		MaxConnLifetime:   env.getEnvDuration("DB_MAX_CONN_LIFETIME", time.Hour),
+		MaxConnIdleTime:   env.getEnvDuration("DB_MAX_CONN_IDLE_TIME", 30*time.Minute),
+		HealthCheckPeriod: env.getEnvDuration("DB_HEALTH_CHECK_PERIOD", time.Minute),
+		ConnectTimeout:    env.getEnvDuration("DB_CONNECT_TIMEOUT", 10*time.Second),
 
 		// Migrations
-		AutoMigrate:    getEnvBool("DB_AUTO_MIGRATE", false),
+		AutoMigrate:    env.getEnvBool("DB_AUTO_MIGRATE", false),
 		MigrationsPath: getEnv("DB_MIGRATIONS_PATH", "internal/database/postgres/migrations"),
 
 		// Logging
 		LogLevel: getEnv("DB_LOG_LEVEL", "info"),
 	}
+	if err := errors.Join(env.errs...); err != nil {
+		return nil, err
+	}
 
 	// Validate configuration
 	if err := config.Validate(); err != nil {
@@ -192,29 +201,52 @@ func getEnv(key, defaultValue string) string {
 	return defaultValue
 }
 
-func getEnvInt(key string, defaultValue int) int {
-	if value := os.Getenv(key); value != "" {
-		if intVal, err := strconv.Atoi(value); err == nil {
-			return intVal
-		}
+// envParser reads typed environment variables. An unset variable yields the
+// default; a set one that does not parse also yields the default but records
+// an error, so LoadFromEnv can refuse to start and name every bad variable.
+type envParser struct {
+	errs []error
+}
+
+func (p *envParser) invalid(key, value string, err error) {
+	p.errs = append(p.errs, fmt.Errorf("invalid %s %q: %w", key, value, err))
+}
+
+func (p *envParser) getEnvInt(key string, defaultValue int) int {
+	value := os.Getenv(key)
+	if value == "" {
+		return defaultValue
 	}
-	return defaultValue
+	intVal, err := strconv.Atoi(value)
+	if err != nil {
+		p.invalid(key, value, err)
+		return defaultValue
+	}
+	return intVal
 }
 
-func getEnvBool(key string, defaultValue bool) bool {
-	if value := os.Getenv(key); value != "" {
-		if boolVal, err := strconv.ParseBool(value); err == nil {
-			return boolVal
-		}
+func (p *envParser) getEnvBool(key string, defaultValue bool) bool {
+	value := os.Getenv(key)
+	if value == "" {
+		return defaultValue
 	}
-	return defaultValue
+	boolVal, err := strconv.ParseBool(value)
+	if err != nil {
+		p.invalid(key, value, err)
+		return defaultValue
+	}
+	return boolVal
 }
 
-func getEnvDuration(key string, defaultValue time.Duration) time.Duration {
-	if value := os.Getenv(key); value != "" {
-		if duration, err := time.ParseDuration(value); err == nil {
-			return duration
-		}
+func (p *envParser) getEnvDuration(key string, defaultValue time.Duration) time.Duration {
+	value := os.Getenv(key)
+	if value == "" {
+		return defaultValue
 	}
-	return defaultValue
+	duration, err := time.ParseDuration(value)
+	if err != nil {
+		p.invalid(key, value, err)
+		return defaultValue
+	}
+	return duration
 }