From d7c2b4b2978346a0d793e09c06b79da27b1f10ce Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 07:55:11 +0000 Subject: [PATCH 1/5] fix(scheduler): report min_count in a rec's hidden_by reasons The list filter drops a rec on five override dimensions, but the rec-detail explainer checked only four, so a rec hidden by a min_count floor opened with an empty hidden_by. Both now walk one overrideDimensions table, so a new dimension cannot reach only one. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/api/types.go | 2 +- internal/scheduler/scheduler.go | 22 +----- internal/scheduler/scheduler_overrides.go | 75 +++++++++++++++---- .../scheduler/scheduler_overrides_test.go | 74 ++++++++++++++++++ 4 files changed, 137 insertions(+), 36 deletions(-) diff --git a/internal/api/types.go b/internal/api/types.go index 9ff2a91f1..17734625a 100644 --- a/internal/api/types.go +++ b/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"` diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index c7514c959..f6786d03f 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1275,7 +1275,8 @@ func (s *Scheduler) ListRecommendationsStrict(ctx context.Context, filter config // hiddenBy is non-nil (one or more reason strings) when the rec exists and // passes the suppression check, but would be dropped by the account-override // filter. Possible reasons: "enabled=false", "engine", "region", -// "resource_type". The caller renders a "hidden by your override" banner. +// "resource_type", "min_count" (overrideDimensions). The caller renders a +// "hidden by your override" banner. // // Returns (nil, nil, nil) when the rec is genuinely absent or fully suppressed. func (s *Scheduler) GetRecommendationByID(ctx context.Context, id string) (rec *config.RecommendationRecord, hiddenBy []string, err error) { @@ -1320,25 +1321,6 @@ func (s *Scheduler) GetRecommendationByID(ctx context.Context, id string) (rec * return found, hiddenBy, nil } -// overrideHiddenReasons returns a non-empty slice when rec would be dropped by -// the given resolved ServiceConfig, naming each failing dimension. -func overrideHiddenReasons(rec *config.RecommendationRecord, cfg *config.ServiceConfig) []string { - var reasons []string - if !cfg.Enabled { - reasons = append(reasons, "enabled=false") - } - if !engineMatches(rec.Engine, cfg) { - reasons = append(reasons, "engine") - } - if !inListRule(rec.Region, cfg.IncludeRegions, cfg.ExcludeRegions) { - reasons = append(reasons, "region") - } - if !inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) { - reasons = append(reasons, "resource_type") - } - return reasons -} - // resolveEffectiveCacheTTL returns the effective stale-while-revalidate TTL // and whether background auto-refresh has been explicitly disabled (value 0). // It prefers the DB-configured RecommendationsCacheStaleHours; falls back to diff --git a/internal/scheduler/scheduler_overrides.go b/internal/scheduler/scheduler_overrides.go index d3d3a7993..dcfb7af08 100644 --- a/internal/scheduler/scheduler_overrides.go +++ b/internal/scheduler/scheduler_overrides.go @@ -42,8 +42,9 @@ func (s *Scheduler) applyAccountOverrides(ctx context.Context, recs []config.Rec } // filterRecsByResolvedConfigs drops recs whose resolved per-account -// ServiceConfig says enabled=false, or whose engine / region / resource_type -// is rejected by the resolved include/exclude lists. +// ServiceConfig says enabled=false, whose engine / region / resource_type +// is rejected by the resolved include/exclude lists, or whose count is below +// the min_count floor (see overrideDimensions). // // Recs without a CloudAccountID use global configuration. Recs whose triple // has no resolved entry pass through unfiltered. @@ -65,19 +66,7 @@ func filterRecsByResolvedConfigs( out = append(out, rec) continue } - if !cfg.Enabled { - continue - } - if !engineMatches(rec.Engine, cfg) { - continue - } - if !inListRule(rec.Region, cfg.IncludeRegions, cfg.ExcludeRegions) { - continue - } - if !inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) { - continue - } - if !common.IsSavingsPlan(common.ServiceType(rec.Service)) && !meetsMinCount(rec.Count, cfg) { + if !passesOverrideDimensions(&rec, cfg) { continue } out = append(out, rec) @@ -85,6 +74,62 @@ func filterRecsByResolvedConfigs( return out } +// overrideDimension is one axis of the account-service override filter: +// passes reports whether rec survives it under cfg, and reason is the +// hidden_by string the rec-detail view reports when it does not. +type overrideDimension struct { + passes func(rec *config.RecommendationRecord, cfg *config.ServiceConfig) bool + reason string +} + +// overrideDimensions is the single list both the list filter +// (filterRecsByResolvedConfigs) and the rec-detail explainer +// (overrideHiddenReasons) walk, so a rec the list hides always carries a +// reason in hidden_by. Add a new filter dimension here, never to only one of +// the two callers: min_count was once added to the filter alone, and a +// min-count-filtered rec then opened with an empty hidden_by (#120). +var overrideDimensions = []overrideDimension{ + {reason: "enabled=false", passes: func(_ *config.RecommendationRecord, cfg *config.ServiceConfig) bool { + return cfg.Enabled + }}, + {reason: "engine", passes: func(rec *config.RecommendationRecord, cfg *config.ServiceConfig) bool { + return engineMatches(rec.Engine, cfg) + }}, + {reason: "region", passes: func(rec *config.RecommendationRecord, cfg *config.ServiceConfig) bool { + return inListRule(rec.Region, cfg.IncludeRegions, cfg.ExcludeRegions) + }}, + {reason: "resource_type", passes: func(rec *config.RecommendationRecord, cfg *config.ServiceConfig) bool { + return inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) + }}, + {reason: "min_count", passes: func(rec *config.RecommendationRecord, cfg *config.ServiceConfig) bool { + return common.IsSavingsPlan(common.ServiceType(rec.Service)) || meetsMinCount(rec.Count, cfg) + }}, +} + +// passesOverrideDimensions reports whether rec survives every override +// dimension under cfg, stopping at the first failure. +func passesOverrideDimensions(rec *config.RecommendationRecord, cfg *config.ServiceConfig) bool { + for _, d := range overrideDimensions { + if !d.passes(rec, cfg) { + return false + } + } + return true +} + +// overrideHiddenReasons returns a non-empty slice when rec would be dropped by +// the given resolved ServiceConfig, naming each failing dimension in +// overrideDimensions order. +func overrideHiddenReasons(rec *config.RecommendationRecord, cfg *config.ServiceConfig) []string { + var reasons []string + for _, d := range overrideDimensions { + if !d.passes(rec, cfg) { + reasons = append(reasons, d.reason) + } + } + return reasons +} + // meetsMinCount implements the read-time half of the GUI/CLI --min-count // filter: a rec is dropped when its count is below cfg.MinCount. A MinCount // of 0 (the default) disables the filter, matching the CLI flag's diff --git a/internal/scheduler/scheduler_overrides_test.go b/internal/scheduler/scheduler_overrides_test.go index a340839ef..f1fdeec0a 100644 --- a/internal/scheduler/scheduler_overrides_test.go +++ b/internal/scheduler/scheduler_overrides_test.go @@ -545,3 +545,77 @@ func TestFilterRecsByResolvedConfigs_MinCount_ExemptsSavingsPlans(t *testing.T) }) } } + +// TestGetRecommendationByID_HiddenByMinCount covers #120: a rec the list +// drops for being under the min_count floor must open with a "min_count" +// hidden_by reason, not an empty one that contradicts the list view. +func TestGetRecommendationByID_HiddenByMinCount(t *testing.T) { + ctx := context.Background() + rec := rdsRec("acct-A", "us-east-1", "mysql") + rec.Count = 3 + store := &mockOverrideStore{ + recs: []config.RecommendationRecord{rec}, + globals: map[string]*config.ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: true, MinCount: 5}, + }, + } + s := &Scheduler{config: store} + + listed, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Empty(t, listed, "count 3 is under min_count 5, so the list hides it") + + got, hiddenBy, err := s.GetRecommendationByID(ctx, rec.ID) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, []string{"min_count"}, hiddenBy) +} + +// TestOverrideHiddenReasons_AgreesWithFilter checks, over every combination +// of failing dimensions, that the list filter drops a rec exactly when +// overrideHiddenReasons names a reason, and names every failing dimension. +func TestOverrideHiddenReasons_AgreesWithFilter(t *testing.T) { + a := "acct-A" + for mask := 0; mask < 1<<5; mask++ { + rec := config.RecommendationRecord{ + ID: "r", Provider: "aws", Service: "rds", Region: "us-east-1", + ResourceType: "db.t3.medium", Engine: "mysql", Count: 3, CloudAccountID: &a, + } + cfg := &config.ServiceConfig{Provider: "aws", Service: "rds", Enabled: true} + var want []string + if mask&1 != 0 { + cfg.Enabled = false + want = append(want, "enabled=false") + } + if mask&2 != 0 { + cfg.ExcludeEngines = []string{"mysql"} + want = append(want, "engine") + } + if mask&4 != 0 { + cfg.ExcludeRegions = []string{"us-east-1"} + want = append(want, "region") + } + if mask&8 != 0 { + cfg.IncludeTypes = []string{"db.r5.large"} + want = append(want, "resource_type") + } + if mask&16 != 0 { + cfg.MinCount = 4 + want = append(want, "min_count") + } + resolved := map[string]*config.ServiceConfig{config.AccountConfigKey(a, "aws", "rds"): cfg} + kept := len(filterRecsByResolvedConfigs([]config.RecommendationRecord{rec}, resolved)) == 1 + got := overrideHiddenReasons(&rec, cfg) + assert.Equal(t, want, got, "mask %05b", mask) + assert.Equal(t, kept, len(got) == 0, "mask %05b: filter and hidden_by disagree", mask) + } +} + +// TestOverrideHiddenReasons_MinCountExemptsSavingsPlans keeps the explainer in +// line with the filter's Savings Plans carve-out (#1146): an SP rec is never +// reported as hidden by min_count. +func TestOverrideHiddenReasons_MinCountExemptsSavingsPlans(t *testing.T) { + rec := config.RecommendationRecord{ID: "r", Provider: "aws", Service: "savingsplans", Count: 1} + cfg := &config.ServiceConfig{Provider: "aws", Service: "savingsplans", Enabled: true, MinCount: 5} + assert.Empty(t, overrideHiddenReasons(&rec, cfg)) +} From d716b6b60988278588badb02f1ed60be1a41bb77 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 07:55:11 +0000 Subject: [PATCH 2/5] fix(config): validate RI-exchange caps and threshold on PUT /api/config GlobalConfig.Validate never checked ri_exchange_max_per_exchange_usd, ri_exchange_max_daily_usd or ri_exchange_utilization_threshold, so a negative money cap or an out-of-range threshold sent to PUT /api/config was stored as is. Reject negative or non-finite caps and a threshold outside [0, 100], the bounds PUT /api/ri-exchange/config already applies. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_config_test.go | 27 ++++++++++++++++++++ internal/config/validation.go | 35 ++++++++++++++++++++++++++ internal/config/validation_test.go | 38 +++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+) diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 44c388030..277f87eb0 100644 --- a/internal/api/handler_config_test.go +++ b/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) diff --git a/internal/config/validation.go b/internal/config/validation.go index e8f3ab455..3b65bd89e 100644 --- a/internal/config/validation.go +++ b/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. diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 2b9301b13..f7718ad08 100644 --- a/internal/config/validation_test.go +++ b/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 From 2ad49ecb884d5d2ca75212a1531890a01e416a35 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 07:55:11 +0000 Subject: [PATCH 3/5] fix(config): return ErrNotFound from GetAccountCredential for a missing row A missing credential returned ("", nil), the same pair a stored empty blob returns. Return ErrNotFound instead, before a caller reads the empty result as "use the ambient identity". Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/config/interfaces.go | 2 + internal/config/store_postgres.go | 5 +- ...res_account_credential_integration_test.go | 51 +++++++++++++++++++ .../config/store_postgres_pgxmock_test.go | 3 +- 4 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 internal/config/store_postgres_account_credential_integration_test.go diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 9a078709a..e7234af55 100644 --- a/internal/config/interfaces.go +++ b/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) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 668d1bd27..e7dfd41a8 100644 --- a/internal/config/store_postgres.go +++ b/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) } diff --git a/internal/config/store_postgres_account_credential_integration_test.go b/internal/config/store_postgres_account_credential_integration_test.go new file mode 100644 index 000000000..609bd15cc --- /dev/null +++ b/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") +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 25a76d932..8280d5bb1 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/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) } From 1eb95c5fdc47d9839ee721417240437a64cf6e29 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 07:55:11 +0000 Subject: [PATCH 4/5] fix(database): refuse malformed DB_* tuning env vars instead of using defaults getEnvInt, getEnvBool and getEnvDuration discarded parse errors, so a typo such as DB_MAX_CONNECTIONS="100 " silently became the default and passed Validate. LoadFromEnv now fails and names every set but unparseable variable. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/database/config.go | 86 ++++++++++++++++++++++---------- internal/database/config_test.go | 53 ++++++++++++++++++-- 2 files changed, 109 insertions(+), 30 deletions(-) diff --git a/internal/database/config.go b/internal/database/config.go index 8f0fc61a5..a1c42fe94 100644 --- a/internal/database/config.go +++ b/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 } diff --git a/internal/database/config_test.go b/internal/database/config_test.go index 3e701dd0d..5ba1525dc 100644 --- a/internal/database/config_test.go +++ b/internal/database/config_test.go @@ -64,6 +64,7 @@ func TestGetEnvInt(t *testing.T) { envValue string defaultValue int expected int + wantErr bool }{ { name: "returns parsed int when valid", @@ -85,6 +86,7 @@ func TestGetEnvInt(t *testing.T) { envValue: "not_a_number", defaultValue: 10, expected: 10, + wantErr: true, }, { name: "handles negative numbers", @@ -111,8 +113,15 @@ func TestGetEnvInt(t *testing.T) { defer os.Unsetenv(tt.key) } - result := getEnvInt(tt.key, tt.defaultValue) + env := &envParser{} + result := env.getEnvInt(tt.key, tt.defaultValue) assert.Equal(t, tt.expected, result) + if tt.wantErr { + require.Len(t, env.errs, 1, "a malformed value must be reported (#120)") + assert.Contains(t, env.errs[0].Error(), tt.key) + } else { + assert.Empty(t, env.errs) + } }) } } @@ -124,6 +133,7 @@ func TestGetEnvBool(t *testing.T) { envValue string defaultValue bool expected bool + wantErr bool }{ { name: "returns true for 'true'", @@ -166,6 +176,7 @@ func TestGetEnvBool(t *testing.T) { envValue: "not_a_bool", defaultValue: true, expected: true, + wantErr: true, }, } @@ -178,8 +189,15 @@ func TestGetEnvBool(t *testing.T) { defer os.Unsetenv(tt.key) } - result := getEnvBool(tt.key, tt.defaultValue) + env := &envParser{} + result := env.getEnvBool(tt.key, tt.defaultValue) assert.Equal(t, tt.expected, result) + if tt.wantErr { + require.Len(t, env.errs, 1, "a malformed value must be reported (#120)") + assert.Contains(t, env.errs[0].Error(), tt.key) + } else { + assert.Empty(t, env.errs) + } }) } } @@ -191,6 +209,7 @@ func TestGetEnvDuration(t *testing.T) { envValue string defaultValue time.Duration expected time.Duration + wantErr bool }{ { name: "parses seconds", @@ -233,6 +252,7 @@ func TestGetEnvDuration(t *testing.T) { envValue: "invalid", defaultValue: time.Hour, expected: time.Hour, + wantErr: true, }, } @@ -245,8 +265,15 @@ func TestGetEnvDuration(t *testing.T) { defer os.Unsetenv(tt.key) } - result := getEnvDuration(tt.key, tt.defaultValue) + env := &envParser{} + result := env.getEnvDuration(tt.key, tt.defaultValue) assert.Equal(t, tt.expected, result) + if tt.wantErr { + require.Len(t, env.errs, 1, "a malformed value must be reported (#120)") + assert.Contains(t, env.errs[0].Error(), tt.key) + } else { + assert.Empty(t, env.errs) + } }) } } @@ -651,6 +678,26 @@ func TestLoadFromEnv(t *testing.T) { _, err := LoadFromEnv() require.Error(t, err) }) + + // #120: a set but malformed tuning variable fails the load instead of + // silently becoming the default, which Validate would then accept. + t.Run("returns error naming every malformed variable", func(t *testing.T) { + cleanup := setEnvVars(map[string]string{ + "DB_HOST": "db.example.com", + "DB_PASSWORD": "testpass", + "DB_MAX_CONNECTIONS": "100 ", + "DB_MAX_CONN_LIFETIME": "1h30", + "DB_AUTO_MIGRATE": "yes", + }) + defer cleanup() + + config, err := LoadFromEnv() + require.Error(t, err) + assert.Nil(t, config) + for _, key := range []string{"DB_MAX_CONNECTIONS", "DB_MAX_CONN_LIFETIME", "DB_AUTO_MIGRATE"} { + assert.Contains(t, err.Error(), key) + } + }) } func TestConfigStruct(t *testing.T) { From b4587190c104e249eaf39ba73002588e2f2d5294 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 09:33:47 +0000 Subject: [PATCH 5/5] test(scheduler): expect min_count in hidden_by for the global minimum case TestGlobalFiltersPersistedAmbientAndRegistered/minimum asserted an empty hidden_by for count-1 recs that the same subtest shows the list hiding. Every other row in the table, all global service-config policies, expects its dimension in hidden_by. The empty expectation dates from before the explainer knew about min_count, and min_count can only come from the service config (AccountServiceOverride has no MinCount), so this row encoded the list/detail contradiction the fix removes. Refs #120 Co-Authored-By: Claude Opus 5.5 --- .../scheduler/scheduler_global_filters_integration_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/scheduler/scheduler_global_filters_integration_test.go b/internal/scheduler/scheduler_global_filters_integration_test.go index c12703268..3a9df5360 100644 --- a/internal/scheduler/scheduler_global_filters_integration_test.go +++ b/internal/scheduler/scheduler_global_filters_integration_test.go @@ -51,7 +51,10 @@ func TestGlobalFiltersPersistedAmbientAndRegistered(t *testing.T) { }{ {"disabled", config.ServiceConfig{}, nil, "enabled=false"}, {"enabled", config.ServiceConfig{Enabled: true}, []string{"1", "10", "11", "sibling"}, ""}, - {"minimum", config.ServiceConfig{Enabled: true, MinCount: 10}, []string{"10", "11", "sibling"}, ""}, + // The list drops count-1 recs under the floor, so their detail must say + // why, like every other dimension in this table (#120). min_count is + // only ever set on the service config (overrides have no MinCount). + {"minimum", config.ServiceConfig{Enabled: true, MinCount: 10}, []string{"10", "11", "sibling"}, "min_count"}, {"include engine", config.ServiceConfig{Enabled: true, IncludeEngines: []string{"postgres"}}, []string{"sibling"}, "engine"}, {"exclude engine", config.ServiceConfig{Enabled: true, ExcludeEngines: []string{"mysql"}}, []string{"sibling"}, "engine"}, {"include region", config.ServiceConfig{Enabled: true, IncludeRegions: []string{"us-west-2"}}, []string{"sibling"}, "region"},