diff --git a/CLAUDE.md b/CLAUDE.md index 7e29a7e3..ecf5f187 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -423,6 +423,7 @@ GoBricks breaks its own API surface when justified. Greenfield work uses the new - **keystore.secretminlength tri-state (ADR-065):** `KeyStoreConfig.SecretMinLength` is `*int` (`new(n)` in Go literals; nil = 32, `0` = off, deprecated); a hand-built config that left it unset now enforces the 32-byte floor. - **Dead app lifecycle surface removed (ADR-067):** `MessagingInitializer` and `ConnectionPreWarmer` (constructors and methods included), `Options.Database` and `Options.MessagingClient` are gone; the eight debug response types are unexported with their JSON unchanged. - **One delivery pipeline (ADR-068):** `messaging.StartConsumeSpan` is removed — a service driving its own consume loop starts its own span — and the AMQP `messaging.client.consumed.messages` counter is recorded at completion with `error.type` instead of at receive without it. +- **Delivered-empty numeric config (ADR-074):** an empty string bound to a numeric key (`FOO=`, empty `secretKeyRef`) fails startup naming the key instead of decoding as `0`; `FOO=0`, unset, YAML omission and YAML null are unchanged. - **Scheduler timeouts normalize (ADR-075):** `scheduler.timeout.shutdown`/`slowjob` default in `config.Validate` (30s/25s — hand-built configs move 30s → 25s), negatives fail validation, and `scheduler.Module.Init` requires a NORMALIZED `deps.Config` — non-nil, both timeouts positive — so a config assembled outside app construction must go through `config.Validate` first. - **Section-qualified config errors (ADR-076):** a non-root database section's `ConfigError.Field` names that section (`databases.reporting.host`, `multitenant.tenants.acme.database.host`); match with a database-scoped predicate, not equality and not a bare suffix — `cache.redis.host` ends in `.host` too. Root and the connect door keep the root spelling. diff --git a/app/bootstrap.go b/app/bootstrap.go index 51f09c4e..f060370d 100644 --- a/app/bootstrap.go +++ b/app/bootstrap.go @@ -2,6 +2,7 @@ package app import ( "context" + "fmt" "strconv" "github.com/gaborage/go-bricks/config" @@ -130,7 +131,18 @@ func (b *appBootstrap) dependencies(startupCtx context.Context) (*dependencyBund } // Initialize observability provider (no-op if disabled) - obsProvider := b.initializeObservability(startupCtx) + obsProvider, err := b.initializeObservability(startupCtx) + if err != nil { + if cacheManager != nil { + _ = cacheManager.Close() + } + closeManagers := b.closeManagers + if closeManagers == nil { + closeManagers = closeManagersOnDependencyError + } + closeManagers(dbManager, messagingManager) + return nil, err + } // Enhance logger with OTLP export if enabled // This upgrades the bootstrap logger so all subsequent components share a single @@ -213,19 +225,34 @@ func (b *appBootstrap) warnIfDatabaseAbsent() { "if this service expects a database, its configuration did not reach the process") } +// observabilityConfigKey is the koanf section initializeObservability decodes; its presence +// separates "no observability configured" from "configured, but undecodable". +const observabilityConfigKey = "observability" + // initializeObservability creates and configures the observability provider. -// Returns a no-op provider if observability is disabled or configuration is missing. -func (b *appBootstrap) initializeObservability(startupCtx context.Context) observability.Provider { +// Returns a no-op provider when the observability section is absent. +// +// A section that IS present but cannot be decoded aborts startup instead: degrading to +// the no-op provider there turns one bad key into total telemetry loss — no traces, no +// metrics, no OTLP logs, no migration audit events — announced by a single WARN on the +// way past. That is exactly the shape a delivered-empty numeric now produces (ADR-074), +// so the distinction has to be drawn rather than assumed. +func (b *appBootstrap) initializeObservability(startupCtx context.Context) (observability.Provider, error) { b.log.Debug().Msg("Starting observability initialization") // Create observability config var obsCfg observability.Config - // Try to unmarshal configuration from the "observability" key - if err := b.cfg.Unmarshal("observability", &obsCfg); err != nil { - // Configuration missing or invalid - use defaults (observability disabled) - b.log.Warn().Err(err).Msg("Observability configuration not found or invalid, using no-op provider") - return observability.MustNewProvider(&observability.Config{Enabled: false}) + // Absence is decided BEFORE decoding, not inferred from a decode error: koanf returns no + // error for a missing key, so an absent section would otherwise decode to a zero Config + // and fall through to construction, leaving the documented no-op branch unreachable. + if !b.cfg.Exists(observabilityConfigKey) { + b.log.Debug().Msg("No observability configuration, using no-op provider") + return observability.MustNewProvider(&observability.Config{Enabled: false}), nil + } + + if err := b.cfg.Unmarshal(observabilityConfigKey, &obsCfg); err != nil { + return nil, fmt.Errorf("observability configuration is present but invalid: %w", err) } b.log.Debug(). @@ -287,8 +314,10 @@ func (b *appBootstrap) initializeObservability(startupCtx context.Context) obser provider, err := construct(ctx, &obsCfg) if err != nil { + // Construction failure is an environment problem (unreachable collector, bad + // resource probe), not a malformed config: it stays non-fatal, as before. b.log.Warn().Err(err).Msg("Failed to initialize observability, using no-op provider") - return observability.MustNewProvider(&observability.Config{Enabled: false}) + return observability.MustNewProvider(&observability.Config{Enabled: false}), nil } if obsCfg.Enabled { @@ -303,7 +332,7 @@ func (b *appBootstrap) initializeObservability(startupCtx context.Context) obser b.log.Debug().Msg("Observability disabled by configuration") } - return provider + return provider, nil } // enhanceLoggerWithOTel attaches OTLP log export to the logger if observability is enabled. diff --git a/app/bootstrap_test.go b/app/bootstrap_test.go index 54db43dc..4f5dde5a 100644 --- a/app/bootstrap_test.go +++ b/app/bootstrap_test.go @@ -310,7 +310,8 @@ observability: bootstrap := newAppBootstrap(cfg, log, &Options{}) // Initialize observability - obsProvider := bootstrap.initializeObservability(context.Background()) + obsProvider, err := bootstrap.initializeObservability(context.Background()) + require.NoError(t, err) require.NotNil(t, obsProvider) // Verify tracer provider is initialized @@ -415,7 +416,8 @@ observability: bootstrap := newAppBootstrap(cfg, log, &Options{}) // Initialize observability - obsProvider := bootstrap.initializeObservability(context.Background()) + obsProvider, err := bootstrap.initializeObservability(context.Background()) + require.NoError(t, err) require.NotNil(t, obsProvider) // Verify provider is functional (indicates config was loaded successfully) @@ -497,7 +499,8 @@ observability: bootstrap := newAppBootstrap(cfg, log, &Options{}) // Initialize observability - obsProvider := bootstrap.initializeObservability(context.Background()) + obsProvider, err := bootstrap.initializeObservability(context.Background()) + require.NoError(t, err) require.NotNil(t, obsProvider) // Should return noop providers @@ -556,7 +559,8 @@ debug: bootstrap := newAppBootstrap(cfg, log, &Options{}) // Initialize observability (should fallback to noop provider) - obsProvider := bootstrap.initializeObservability(context.Background()) + obsProvider, err := bootstrap.initializeObservability(context.Background()) + require.NoError(t, err) require.NotNil(t, obsProvider, "Should return noop provider when config is missing") // Should return noop providers @@ -571,6 +575,77 @@ debug: assert.NoError(t, err) } +// TestInitializeObservabilityFailsClosedOnUndecodableSection pins the distinction the +// no-op fallback used to blur. observability.* is decoded separately from config.Config, +// so a delivered-empty numeric there passes config.Load and lands here (ADR-074) — and +// swallowing it took every trace, metric, OTLP log and migration audit event with it, +// announced by one WARN. A present-but-undecodable section now aborts startup; an ABSENT +// section keeps the documented no-op posture. +func TestInitializeObservabilityFailsClosedOnUndecodableSection(t *testing.T) { + t.Run("present_but_undecodable_aborts", func(t *testing.T) { + cfg := loadConfigFromYAML(t, ` +app: + name: "test" + version: "1.0.0" +server: + port: 8080 +observability: + enabled: true + trace: + batch: + size: "" +`) + bootstrap := newAppBootstrap(cfg, logger.New("info", false), &Options{}) + + provider, err := bootstrap.initializeObservability(context.Background()) + + require.Error(t, err) + assert.ErrorContains(t, err, "present but invalid") + assert.ErrorContains(t, err, "delivered empty") + assert.Nil(t, provider, "a caller must not receive a silently degraded provider") + }) + + t.Run("absent_section_never_reaches_construction", func(t *testing.T) { + cfg := loadConfigFromYAML(t, ` +app: + name: "test" + version: "1.0.0" +server: + port: 8080 +`) + bootstrap := newAppBootstrap(cfg, logger.New("info", false), &Options{}) + constructed := false + bootstrap.newProvider = func(context.Context, *observability.Config) (observability.Provider, error) { + constructed = true + return observability.MustNewProvider(&observability.Config{Enabled: false}), nil + } + + provider, err := bootstrap.initializeObservability(context.Background()) + + require.NoError(t, err) + require.NotNil(t, provider) + assert.False(t, constructed, + "an absent section takes the documented no-op branch; koanf returns no error for a "+ + "missing key, so inferring absence from a decode failure never fires") + }) + + t.Run("absent_section_stays_no_op", func(t *testing.T) { + cfg := loadConfigFromYAML(t, ` +app: + name: "test" + version: "1.0.0" +server: + port: 8080 +`) + bootstrap := newAppBootstrap(cfg, logger.New("info", false), &Options{}) + + provider, err := bootstrap.initializeObservability(context.Background()) + + require.NoError(t, err) + require.NotNil(t, provider) + }) +} + // TestInitializeObservabilityThreadsBudgetContext verifies that when // app.startup.observability is positive, the construction seam receives a // non-nil context whose deadline is set roughly the budget into the future. @@ -619,7 +694,8 @@ observability: } start := time.Now() - got := bootstrap.initializeObservability(context.Background()) + got, err := bootstrap.initializeObservability(context.Background()) + require.NoError(t, err) assert.Same(t, want, got, "the provider returned by the seam should be installed verbatim") require.NotNil(t, gotCtx, "seam must receive a non-nil context") @@ -672,7 +748,8 @@ observability: parent, cancel := context.WithCancel(context.Background()) cancel() // cancel the parent BEFORE construction - bootstrap.initializeObservability(parent) + _, err := bootstrap.initializeObservability(parent) + require.NoError(t, err) require.NotNil(t, gotCtx, "seam must receive a non-nil context") require.ErrorIs(t, gotCtx.Err(), context.Canceled, @@ -715,7 +792,8 @@ observability: return nil, errors.New("exporter dial failed") } - got := bootstrap.initializeObservability(context.Background()) + got, err := bootstrap.initializeObservability(context.Background()) + require.NoError(t, err) require.NotNil(t, got, "a no-op provider must be installed on constructor error") assert.NotNil(t, got.TracerProvider(), "no-op provider must expose a tracer provider") diff --git a/config/config.go b/config/config.go index 6a491ece..4686007d 100644 --- a/config/config.go +++ b/config/config.go @@ -281,12 +281,14 @@ func tryLoadYAMLFile(k *koanf.Koanf, baseName string) error { } // buildDecoderConfig is the decoder for Load: it replicates koanf's default Unmarshal -// decoder (knadh/koanf/v2 koanf.go:265-272) plus the numeric-duration guard and the -// comma-split slice hook (so a single env var can express a []string). koanf fills in +// decoder (knadh/koanf/v2 koanf.go:265-272) plus the delivered-empty numeric guard, the +// numeric-duration guard and the comma-split slice hook (so a single env var can express +// a []string). koanf fills in // Result and TagName at unmarshal time. func buildDecoderConfig() *mapstructure.DecoderConfig { return &mapstructure.DecoderConfig{ DecodeHook: mapstructure.ComposeDecodeHookFunc( + configdecode.EmptyStringToNumericGuardHookFunc(), configdecode.NumericToDurationGuardHookFunc(), stringToTrimmedSliceHookFunc(","), mapstructure.StringToTimeDurationHookFunc(), @@ -298,7 +300,8 @@ func buildDecoderConfig() *mapstructure.DecoderConfig { // unmarshalDecoderConfig is the decoder for the public Config.Unmarshal. It mirrors koanf's // default Unmarshal decoder (StringToTimeDurationHookFunc + text-unmarshaler + WeaklyTypedInput) -// plus only the numeric-duration guard — deliberately WITHOUT the comma-split slice hook, so +// plus the delivered-empty numeric guard and the numeric-duration guard — deliberately +// WITHOUT the comma-split slice hook, so // string -> []string keeps koanf's default single-element wrap on this public seam instead of // silently comma-splitting. (koanf's default uses its own unexported textUnmarshalerHookFunc; // the exported mapstructure.TextUnmarshallerHookFunc is the closest public equivalent and @@ -306,6 +309,7 @@ func buildDecoderConfig() *mapstructure.DecoderConfig { func unmarshalDecoderConfig() *mapstructure.DecoderConfig { return &mapstructure.DecoderConfig{ DecodeHook: mapstructure.ComposeDecodeHookFunc( + configdecode.EmptyStringToNumericGuardHookFunc(), configdecode.NumericToDurationGuardHookFunc(), mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), diff --git a/config/config_test.go b/config/config_test.go index 39f26b18..96f29bab 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -185,6 +185,74 @@ func TestConfigPerTenantJobKeys(t *testing.T) { }) } +// TestLoadRejectsEmptyNumericEnv pins the delivered-empty rule for numeric keys: a +// set-but-empty variable used to decode as a legal 0 (defeating ADR-065's tri-state and +// silently zeroing byte limits), and now fails Load naming the key. +func TestLoadRejectsEmptyNumericEnv(t *testing.T) { + tests := []struct { + name string + envVar string + wantKey string + }{ + {name: "keystore_secretminlength", envVar: "KEYSTORE_SECRETMINLENGTH", wantKey: "keystore.secretminlength"}, + {name: "server_bodylimit", envVar: "SERVER_BODYLIMIT", wantKey: "server.bodylimit"}, + {name: "server_port", envVar: "SERVER_PORT", wantKey: "server.port"}, + // database.port is an ADR-051 identity key AND numeric, so the numeric guard + // reaches it first: it now fails at decode rather than with the identity error. + {name: "database_port_changes_error_class", envVar: "DATABASE_PORT", wantKey: "database.port"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearEnvironmentVariables() + t.Setenv(tt.envVar, "") + + _, err := Load() + + require.Error(t, err) + assert.ErrorContains(t, err, tt.wantKey) + assert.ErrorContains(t, err, "delivered empty") + }) + } +} + +// TestLoadEmptyDurationEnvKeepsItsOwnError pins the guard's one exemption: time.Duration +// targets fall through to the duration parser, so an empty duration still fails with the +// parse error rather than the delivered-empty one. Both are loud; this pins which. +func TestLoadEmptyDurationEnvKeepsItsOwnError(t *testing.T) { + clearEnvironmentVariables() + t.Setenv("SERVER_TIMEOUT_READ", "") + + _, err := Load() + + require.Error(t, err) + assert.ErrorContains(t, err, "invalid duration") + assert.NotContains(t, err.Error(), "delivered empty", + "the duration parser owns this target; guarding it here would only change the message") +} + +// TestLoadEmptyNumericYAMLStringRejected covers the same rule arriving through YAML: an +// empty string takes the identical decode path an empty env var does. +func TestLoadEmptyNumericYAMLStringRejected(t *testing.T) { + _, err := loadDeliveredEmptyFixture(t, "keystore:\n secretminlength: \"\"\n", nil) + + require.Error(t, err) + assert.ErrorContains(t, err, "secretminlength") + assert.ErrorContains(t, err, "delivered empty") +} + +// TestLoadYAMLNullNumericKeepsTodaysDecode pins the boundary the guard deliberately does +// NOT cover: a YAML null is different plumbing — koanf delivers a nil value, not the "" +// the guard judges — so a null pointer key still decodes as absent and takes its default. +// Documented in ADR-074; this test exists so the boundary cannot drift unnoticed. +func TestLoadYAMLNullNumericKeepsTodaysDecode(t *testing.T) { + cfg, err := loadDeliveredEmptyFixture(t, "keystore:\n secretminlength:\n", nil) + + require.NoError(t, err) + require.NotNil(t, cfg.KeyStore.SecretMinLength) + assert.Equal(t, 32, *cfg.KeyStore.SecretMinLength, "a null key is absence, so the floor still applies") +} + func TestLoadMultiElementStringSliceEnv(t *testing.T) { clearEnvironmentVariables() t.Setenv("SCHEDULER_SECURITY_CIDRALLOWLIST", "10.0.0.0/8,192.168.0.0/16") diff --git a/internal/configdecode/configdecode.go b/internal/configdecode/configdecode.go index 9a6b59fa..0cf27332 100644 --- a/internal/configdecode/configdecode.go +++ b/internal/configdecode/configdecode.go @@ -5,8 +5,10 @@ package configdecode import ( + "errors" "fmt" "reflect" + "strings" "time" "github.com/go-viper/mapstructure/v2" @@ -57,3 +59,49 @@ func NumericToDurationGuardHookFunc() mapstructure.DecodeHookFunc { ) } } + +// EmptyStringToNumericGuardHookFunc rejects an empty (or whitespace-only) string bound to a +// numeric field. WeaklyTypedInput would otherwise coerce it to 0, so a set-but-empty +// environment variable (FOO=) or an empty YAML string decodes as a legal zero and boots a +// config nobody wrote — the tri-state in ADR-065 is defeated exactly this way. Pointer +// targets are guarded too: the zero arrives as a non-nil *0, which normalization reads as +// "operator set it". +// +// time.Duration is exempt: StringToTimeDurationHookFunc owns that target and already fails +// loudly on an empty string. Non-numeric targets are untouched — an empty string is a legal +// string, and the database-identity subset is ADR-051's to judge. +func EmptyStringToNumericGuardHookFunc() mapstructure.DecodeHookFunc { + return func(f, t reflect.Type, data any) (any, error) { + if f.Kind() != reflect.String { + return data, nil + } + // No pointer walk: mapstructure recurses into a pointer target and re-runs the + // hook chain against the element type, so *int arrives here as int. + if t == durationType || !isNumericKind(t.Kind()) { + return data, nil + } + // reflect rather than a concrete type assertion: a named string type (type Env + // string) has Kind String but fails data.(string), and passing it through hands it + // straight to the weak "" -> 0 conversion this guard exists to stop. + if strings.TrimSpace(reflect.ValueOf(data).String()) != "" { + return data, nil + } + return nil, errors.New( + "numeric value delivered empty — set an explicit value (empty secretKeyRef / unset envsubst variable?) " + + "or remove the key entirely to take its default", + ) + } +} + +// isNumericKind reports whether k is one mapstructure's WeaklyTypedInput would fill from a +// string, i.e. exactly the kinds where "" silently becomes 0. +func isNumericKind(k reflect.Kind) bool { + switch k { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return true + default: + return false + } +} diff --git a/internal/configdecode/empty_numeric_test.go b/internal/configdecode/empty_numeric_test.go new file mode 100644 index 00000000..01814013 --- /dev/null +++ b/internal/configdecode/empty_numeric_test.go @@ -0,0 +1,125 @@ +package configdecode + +import ( + "testing" + "time" + + "github.com/go-viper/mapstructure/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEmptyStringToNumericGuardHookFunc pins the guard's decision table: an empty or +// whitespace-only string targeting any numeric field is rejected, pointer targets +// included; every other source/target pair decodes as before. +func TestEmptyStringToNumericGuardHookFunc(t *testing.T) { + type target struct { + Count int `mapstructure:"count"` + Ratio float64 `mapstructure:"ratio"` + Size uint `mapstructure:"size"` + MinLen *int `mapstructure:"minlen"` + Name string `mapstructure:"name"` + Interval time.Duration `mapstructure:"interval"` + } + + tests := []struct { + name string + input map[string]any + wantErr bool + errSubstr string + assert func(t *testing.T, got target) + }{ + {name: "empty_int_rejected", input: map[string]any{"count": ""}, wantErr: true, errSubstr: "delivered empty"}, + {name: "empty_float_rejected", input: map[string]any{"ratio": ""}, wantErr: true, errSubstr: "delivered empty"}, + {name: "empty_uint_rejected", input: map[string]any{"size": ""}, wantErr: true, errSubstr: "delivered empty"}, + {name: "empty_int_pointer_rejected", input: map[string]any{"minlen": ""}, wantErr: true, errSubstr: "delivered empty"}, + {name: "whitespace_int_rejected", input: map[string]any{"count": " "}, wantErr: true, errSubstr: "delivered empty"}, + { + // A named string type has Kind String but is not a string: a concrete type + // assertion drops it, and the weak conversion then makes it a zero. + name: "named_string_type_rejected", + input: map[string]any{"count": envName("")}, + wantErr: true, + errSubstr: "delivered empty", + }, + { + name: "empty_string_target_passes", + input: map[string]any{"name": ""}, + assert: func(t *testing.T, got target) { assert.Empty(t, got.Name) }, + }, + { + name: "explicit_zero_passes", + input: map[string]any{"count": "0"}, + assert: func(t *testing.T, got target) { assert.Equal(t, 0, got.Count) }, + }, + { + name: "explicit_value_passes", + input: map[string]any{"count": "7"}, + assert: func(t *testing.T, got target) { assert.Equal(t, 7, got.Count) }, + }, + { + name: "explicit_pointer_zero_passes", + input: map[string]any{"minlen": "0"}, + assert: func(t *testing.T, got target) { require.NotNil(t, got.MinLen); assert.Equal(t, 0, *got.MinLen) }, + }, + { + name: "numeric_source_passes", + input: map[string]any{"count": 5}, + assert: func(t *testing.T, got target) { assert.Equal(t, 5, got.Count) }, + }, + { + name: "duration_string_passes", + input: map[string]any{"interval": "5s"}, + assert: func(t *testing.T, got target) { assert.Equal(t, 5*time.Second, got.Interval) }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var out target + dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: mapstructure.ComposeDecodeHookFunc( + EmptyStringToNumericGuardHookFunc(), + NumericToDurationGuardHookFunc(), + mapstructure.StringToTimeDurationHookFunc(), + ), + WeaklyTypedInput: true, + Result: &out, + }) + require.NoError(t, err) + + err = dec.Decode(tt.input) + if tt.wantErr { + require.Error(t, err) + assert.ErrorContains(t, err, tt.errSubstr) + return + } + require.NoError(t, err) + tt.assert(t, out) + }) + } +} + +// TestEmptyStringToNumericGuardNamesTheField pins that the rejection reaches the operator +// with the field path attached — mapstructure wraps hook errors with the key it was +// decoding, which is the only place the key name exists at this seam. +func TestEmptyStringToNumericGuardNamesTheField(t *testing.T) { + var out struct { + SecretMinLength *int `mapstructure:"secretminlength"` + } + dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: EmptyStringToNumericGuardHookFunc(), + WeaklyTypedInput: true, + Result: &out, + }) + require.NoError(t, err) + + err = dec.Decode(map[string]any{"secretminlength": ""}) + + require.Error(t, err) + assert.ErrorContains(t, err, "secretminlength") +} + +// envName is a named string type, the shape a consumer config field takes when it wants a +// domain type rather than a bare string. +type envName string diff --git a/internal/configdecode/mirror_drift_test.go b/internal/configdecode/mirror_drift_test.go new file mode 100644 index 00000000..b8f18e56 --- /dev/null +++ b/internal/configdecode/mirror_drift_test.go @@ -0,0 +1,109 @@ +package configdecode + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The tools/migration CLI is a separate module that keeps byte-identical copies of these +// hooks rather than importing this package (see #1109 for whether that stays). Nothing +// gated the promise: the two test suites exercise each copy separately, so a change here +// leaves the CLI's copy green while the two behave differently — and a tenants.yaml would +// then decode under different rules than a framework config. +// +// This compares the bodies as source text, which needs no import and therefore does not +// prejudge #1109. It reads the mirror by path; the two live in one repository. +const mirrorPath = "../../tools/migration/internal/commands/common.go" + +func TestMirroredHooksHaveNotDrifted(t *testing.T) { + canonical := readSource(t, "configdecode.go") + mirror := readSource(t, mirrorPath) + + tests := []struct { + name string + canonicalFunc string + mirrorFunc string + }{ + { + name: "empty_string_to_numeric_guard", + canonicalFunc: "EmptyStringToNumericGuardHookFunc", + mirrorFunc: "emptyStringToNumericGuardHookFunc", + }, + { + name: "numeric_to_duration_guard", + canonicalFunc: "NumericToDurationGuardHookFunc", + mirrorFunc: "numericToDurationGuardHookFunc", + }, + { + name: "numeric_kind_predicate", + canonicalFunc: "isNumericKind", + mirrorFunc: "isNumericKind", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + want := funcBody(t, canonical, tt.canonicalFunc) + got := funcBody(t, mirror, tt.mirrorFunc) + + require.Equal(t, want, got, + "%s has drifted from its mirror in %s — change both, or resolve #1109", + tt.canonicalFunc, mirrorPath) + }) + } +} + +// TestReadSourceNormalizesCRLF pins the normalization at its own seam — reading a genuinely +// CRLF-terminated file — because without it the drift test passes on Linux and fails on +// Windows, which is a worse failure than the drift it looks for. +func TestReadSourceNormalizesCRLF(t *testing.T) { + path := filepath.Join(t.TempDir(), "sample.go") + require.NoError(t, os.WriteFile(path, + []byte("package x\r\n\r\nfunc sample() bool {\r\n\treturn true\r\n}\r\n"), 0o600)) + + source := readSource(t, path) + + assert.NotContains(t, source, "\r") + assert.Equal(t, "return true", funcBody(t, source, "sample")) +} + +func readSource(t *testing.T, path string) string { + t.Helper() + raw, err := os.ReadFile(filepath.Clean(path)) + require.NoError(t, err, "the mirror moved; update mirrorPath or resolve #1109") + // Normalize line endings: a Windows checkout hands these files back with CRLF, and a + // parser that looks for "\n}\n" finds nothing there — the comparison would be + // platform-dependent rather than drift-dependent. + return strings.ReplaceAll(string(raw), "\r\n", "\n") +} + +// funcBody returns everything between a function's signature and its closing brace, with +// comments and blank lines dropped: the mirror renames the exported hooks and carries its +// own doc comments, so only the statements are comparable. +func funcBody(t *testing.T, source, name string) string { + t.Helper() + + start := regexp.MustCompile(`(?m)^func ` + regexp.QuoteMeta(name) + `\(`) + loc := start.FindStringIndex(source) + require.NotNil(t, loc, "func %s not found", name) + + rest := source[loc[0]:] + end := strings.Index(rest, "\n}\n") + require.NotEqual(t, -1, end, "func %s is not terminated", name) + + var kept []string + for _, line := range strings.Split(rest[:end], "\n")[1:] { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "//") { + continue + } + kept = append(kept, trimmed) + } + return strings.Join(kept, "\n") +} diff --git a/migration/secrets.go b/migration/secrets.go index a1462c9a..94d098f5 100644 --- a/migration/secrets.go +++ b/migration/secrets.go @@ -209,12 +209,15 @@ func parseSecretPayload(raw []byte) (*config.DatabaseConfig, error) { // decodeSecretConfig maps a JSON-decoded payload into a DatabaseConfig via mapstructure, // routing numeric time.Duration fields through the shared guard so a bare JSON number -// (e.g. keepalive.interval: 60) is rejected, not silently coerced to nanoseconds. TagName +// (e.g. keepalive.interval: 60) is rejected, not silently coerced to nanoseconds, and empty +// numeric fields through the delivered-empty guard — a rotated secret rendering +// {"port": ""} would otherwise dial port 0 (ADR-074). TagName // "json" matches the struct's json tags; WeaklyTypedInput handles float64 (JSON's numeric // type) -> int and preserves the prior encoding/json numeric-coercion behavior. func decodeSecretConfig(payload map[string]any, cfg *config.DatabaseConfig) error { dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ DecodeHook: mapstructure.ComposeDecodeHookFunc( + configdecode.EmptyStringToNumericGuardHookFunc(), configdecode.NumericToDurationGuardHookFunc(), mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), diff --git a/migration/secrets_test.go b/migration/secrets_test.go index 60e8729a..b80faefb 100644 --- a/migration/secrets_test.go +++ b/migration/secrets_test.go @@ -236,6 +236,56 @@ func TestSecretsProviderDBConfigNumericDurationGuard(t *testing.T) { }) } +// TestSecretsProviderDBConfigEmptyNumericGuard pins the delivered-empty rule at the seam +// that literally reads secrets: a rotated secret rendering a numeric field as "" used to +// decode as 0 — port 0, or a tuned pool silently reverted to the framework default — +// because this provider builds its own decoder (ADR-074). +func TestSecretsProviderDBConfigEmptyNumericGuard(t *testing.T) { + tests := []struct { + name string + payload string + wantErr bool + assert func(t *testing.T, got *config.DatabaseConfig) + }{ + { + name: "empty_port_rejected", + payload: `{"type":"postgresql","host":"h","username":"u","password":"S3cr3t-P@ss","port":""}`, + wantErr: true, + }, + { + name: "empty_pool_max_connections_rejected", + payload: `{"type":"postgresql","host":"h","username":"u","pool":{"max":{"connections":""}}}`, + wantErr: true, + }, + { + name: "explicit_port_binds", + payload: `{"type":"postgresql","host":"h","username":"u","port":5432}`, + assert: func(t *testing.T, got *config.DatabaseConfig) { assert.Equal(t, 5432, got.Port) }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + payload := []byte(tt.payload) + p := &SecretsProvider{Fetch: func(context.Context, string) ([]byte, error) { return payload, nil }} + + got, err := p.DBConfig(context.Background(), "x") + + if tt.wantErr { + require.Error(t, err) + assert.ErrorIs(t, err, ErrSecretMalformed) + assert.ErrorContains(t, err, "delivered empty") + // The error travels to logs and operator consoles; the payload it was + // decoding is secret material, so no value from it may ride along. + assert.NotContains(t, err.Error(), "S3cr3t-P@ss") + return + } + require.NoError(t, err) + tt.assert(t, got) + }) + } +} + func TestSecretsProviderContextCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() diff --git a/tools/migration/internal/commands/builders_test.go b/tools/migration/internal/commands/builders_test.go index 0aaffa0b..5a9fee66 100644 --- a/tools/migration/internal/commands/builders_test.go +++ b/tools/migration/internal/commands/builders_test.go @@ -98,6 +98,57 @@ func TestTenantDecoderConfigCommaSplitsStringSlice(t *testing.T) { assert.Equal(t, []string{"a", "b"}, out.Allow) } +// TestTenantDecoderConfigRejectsEmptyNumeric proves the CLI decoder carries the framework's +// delivered-empty numeric guard, so a tenants.yaml key set to "" fails here exactly as it +// fails a framework Load instead of decoding as a legal 0. +func TestTenantDecoderConfigRejectsEmptyNumeric(t *testing.T) { + type target struct { + Port int `mapstructure:"port"` + MinLen *int `mapstructure:"minlen"` + Comment string `mapstructure:"comment"` + } + + tests := []struct { + name string + input map[string]any + wantErr bool + assert func(t *testing.T, got target) + }{ + {name: "empty_int_rejected", input: map[string]any{"port": ""}, wantErr: true}, + {name: "empty_int_pointer_rejected", input: map[string]any{"minlen": ""}, wantErr: true}, + {name: "whitespace_int_rejected", input: map[string]any{"port": " "}, wantErr: true}, + { + name: "empty_string_target_passes", + input: map[string]any{"comment": ""}, + assert: func(t *testing.T, got target) { assert.Empty(t, got.Comment) }, + }, + { + name: "explicit_value_passes", + input: map[string]any{"port": "5432"}, + assert: func(t *testing.T, got target) { assert.Equal(t, 5432, got.Port) }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var out target + dc := tenantDecoderConfig() + dc.Result = &out + dec, err := mapstructure.NewDecoder(dc) + require.NoError(t, err) + + err = dec.Decode(tt.input) + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), "delivered empty") + return + } + require.NoError(t, err) + tt.assert(t, out) + }) + } +} + func TestBuildListerSingleTenantPath(t *testing.T) { lister, err := buildLister(&CommonFlags{Tenant: "only"}, nil) require.NoError(t, err) diff --git a/tools/migration/internal/commands/common.go b/tools/migration/internal/commands/common.go index f4a54029..18a0438d 100644 --- a/tools/migration/internal/commands/common.go +++ b/tools/migration/internal/commands/common.go @@ -216,6 +216,7 @@ var durationType = reflect.TypeOf(time.Duration(0)) func tenantDecoderConfig() *mapstructure.DecoderConfig { return &mapstructure.DecoderConfig{ DecodeHook: mapstructure.ComposeDecodeHookFunc( + emptyStringToNumericGuardHookFunc(), numericToDurationGuardHookFunc(), stringToTrimmedSliceHookFunc(","), mapstructure.StringToTimeDurationHookFunc(), @@ -226,9 +227,11 @@ func tenantDecoderConfig() *mapstructure.DecoderConfig { } // numericToDurationGuardHookFunc mirrors -// github.com/gaborage/go-bricks/internal/configdecode.NumericToDurationGuardHookFunc — this is -// a separate module that cannot import go-bricks/internal, so this is a byte-identical local -// copy. Keep the two in sync (bool rejection, typed-Duration pass-through, grouped zero test, +// github.com/gaborage/go-bricks/internal/configdecode.NumericToDurationGuardHookFunc as a +// byte-identical local copy. The import would in fact compile — Go's internal rule is +// import-path-prefix based and this module sits under github.com/gaborage/go-bricks/ — but the +// copy is kept deliberately so the CLI does not bind to a framework package that carries no +// compatibility guarantee across releases. Keep the two in sync (bool rejection, typed-Duration pass-through, grouped zero test, // message). Reject a bare non-zero numeric bound to time.Duration (WeaklyTypedInput would coerce // 300 -> 300ns); an explicit zero (incl. -0.0) is the "unset -> use default" idiom and stays // exempt; a bool is never a duration; a source already time.Duration passes untouched. @@ -267,6 +270,50 @@ func numericToDurationGuardHookFunc() mapstructure.DecodeHookFunc { } } +// emptyStringToNumericGuardHookFunc mirrors +// github.com/gaborage/go-bricks/internal/configdecode.EmptyStringToNumericGuardHookFunc as a +// byte-identical local copy, for the reason given on numericToDurationGuardHookFunc above. +// Keep the two in sync (time.Duration exemption, whitespace trim, message). +// Reject an empty or whitespace-only string bound to a numeric field: WeaklyTypedInput would +// coerce it to 0, so a set-but-empty variable decodes as a legal zero and boots a config nobody +// wrote. time.Duration is exempt (StringToTimeDurationHookFunc already fails loudly on it) and +// non-numeric targets are untouched. +func emptyStringToNumericGuardHookFunc() mapstructure.DecodeHookFunc { + return func(f, t reflect.Type, data any) (any, error) { + if f.Kind() != reflect.String { + return data, nil + } + // No pointer walk: mapstructure recurses into a pointer target and re-runs the + // hook chain against the element type, so *int arrives here as int. + if t == durationType || !isNumericKind(t.Kind()) { + return data, nil + } + // reflect rather than a concrete type assertion: a named string type (type Env + // string) has Kind String but fails data.(string), and passing it through hands it + // straight to the weak "" -> 0 conversion this guard exists to stop. + if strings.TrimSpace(reflect.ValueOf(data).String()) != "" { + return data, nil + } + return nil, errors.New( + "numeric value delivered empty — set an explicit value (empty secretKeyRef / unset envsubst variable?) " + + "or remove the key entirely to take its default", + ) + } +} + +// isNumericKind reports whether k is one mapstructure's WeaklyTypedInput would fill from a +// string, i.e. exactly the kinds where "" silently becomes 0. +func isNumericKind(k reflect.Kind) bool { + switch k { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return true + default: + return false + } +} + // stringToTrimmedSliceHookFunc splits a scalar string into []string on sep, trimming each // element and dropping empties. Scoped to string -> []string only, so []byte, other slices, // and YAML sequences are untouched. Local copy of config.stringToTrimmedSliceHookFunc / diff --git a/wiki/adr_074_delivered_empty_numeric_config.md b/wiki/adr_074_delivered_empty_numeric_config.md new file mode 100644 index 00000000..953756b3 --- /dev/null +++ b/wiki/adr_074_delivered_empty_numeric_config.md @@ -0,0 +1,127 @@ +# ADR-074: A delivered-empty numeric config value fails startup + +- **Status**: Accepted +- **Date**: 2026-08-20 +- **Related**: [ADR-051](adr_051_delivered_empty_database_identity.md) (the delivered-empty rule this extends from database identity keys to numeric keys) · [ADR-065](adr_065_keystore_secretminlength_tristate.md) (the tri-state this silently defeated) + +## Context + +`FOO=` in a Kubernetes manifest, a `secretKeyRef` whose stored value is empty (an +ABSENT key leaves the variable unset instead), an `envsubst` over an unset +variable — every one of them delivers a set-but-empty string. koanf keeps the key +(with its empty value), and mapstructure's `WeaklyTypedInput` rewrites `""` to `0` +for any numeric target. The result decodes as a legal zero. + +Measured against `main` before this change: + +- `KEYSTORE_SECRETMINLENGTH=` produced `SecretMinLength = *0`, which **disables the + secret-length floor**. ADR-065's tri-state cannot defend itself here: normalization + fills a *nil* pointer, and the decoder handed it a non-nil pointer to zero, which + reads as "the operator chose 0". +- `SERVER_BODYLIMIT=` produced `0`, rescued only by a downstream `<= 0` fallback. + +The damage is confined to numeric keys where `0` is legal. Range-validated ints +(`SERVER_PORT=`), enums (`LOG_LEVEL=`), and durations (`SERVER_TIMEOUT_READ=`) already +fail loudly, because zero is rejected downstream or the string will not parse. + +Worth stating plainly, because it sets the cost of this change: for MOST of those keys +the old behaviour was not broken. Across the framework `0` conventionally means +"unset — use the default", so an empty value landed on the documented default and the +deployment was genuinely healthy — `cache.redis.port` resolved to 6379, `outbox.batchsize` +and `messaging.reconnect.maxpublishattempts` to their defaults, `database.manager.maxsize` +likewise. Only where `0` is a legal, meaningful value — the secret-length floor, the body +limit — did an empty value quietly change behaviour. This ADR therefore turns a set of +working deployments into failing ones, deliberately: the framework cannot tell "I meant +the default" from "my template rendered nothing", and guessing has already produced one +security-relevant silent flip. + +One constraint fell out of verification and shapes the whole decision: the fix must +NOT prune empty keys from the koanf tree. ADR-051's database-identity check reads key +*presence* (`Config.Exists`), so dropping a delivered-empty key would boot the exact +misconfiguration ADR-051 exists to catch — reading it as "no database configured". + +## Decision + +The rejection lives at the decode layer, not in the tree. A mapstructure hook +(`configdecode.EmptyStringToNumericGuardHookFunc`) runs ahead of the weak `""` → `0` +coercion in both decoder seams — `buildDecoderConfig` (used by `config.Load`) and +`unmarshalDecoderConfig` (the public `Config.Unmarshal`) — and fails the decode when +an empty or whitespace-only string is bound to a numeric field. Pointer targets are +included: `*0` is precisely the shape that defeats a tri-state. mapstructure attaches +the key it was decoding, so the operator gets the key name with the message. + +`WeaklyTypedInput` stays: string → bool and the other coercions are still wanted. +`time.Duration` is exempt, because `StringToTimeDurationHookFunc` owns that target and +already fails loudly on an empty string — guarding it here would only change which +loud error appears. Non-numeric targets are untouched: an empty string is a legal +string, and the identity subset of them belongs to ADR-051, which still fires first +for `DATABASE_HOST=` because those keys are string-typed. + +YAML **null** (`secretminlength:` with no value) is deliberately out of scope. It is +different plumbing — koanf delivers a nil value, not `""` — and it already behaves as +absence: the key takes its default rather than a silent zero. A test pins that +boundary so it cannot drift unnoticed. + +Four seams carry the hook: the two above, `migration.decodeSecretConfig`, and the +`tools/migration` CLI's `tenantDecoderConfig`. The third builds its own decoder for a +dynamic `DBConfigProvider` payload — the seam that +literally reads secrets, where a rotated secret rendering `{"port": ""}` would dial +port 0 and `{"pool":{"max":{"connections":""}}}` would silently revert a tuned pool to +the framework default. The fourth is the CLI's, which keeps a byte-identical local copy of +the hook rather than importing it. The import would in fact compile — Go's internal +rule is import-path-prefix based and the CLI sits under `github.com/gaborage/go-bricks/` +— but a copy keeps that separately-released binary off a package that carries no +compatibility guarantee. The copies must be kept in sync by hand, and a source-comparison test in +`internal/configdecode` now fails when they diverge — a gate that holds whichever way #1109 +decides, since it neither imports the package nor assumes the copy stays. + +An empty string is judged after trimming, so a whitespace-only value is rejected too. +That one is a message change rather than a new failure — `" "` already failed to +parse — but it keeps one rule for what "delivered empty" means. + +## Consequences + +- `FOO=` no longer means `0` for a numeric key. A deployment that relied on it — or + whose `secretKeyRef` resolves to an empty value it never noticed — fails startup naming + the key. + For keys where `0` resolves to a default this ends a posture that was working; the + trade is a loud failure now against an unnoticed one later, and the operator is the + only one who knows which value was intended. +- `observability.*` decodes through the public `Config.Unmarshal` seam, not through + `config.Config`, and `app` used to swallow a decode failure there with one WARN and + fall back to the no-op provider — trading a single bad key for total telemetry loss + (no traces, no metrics, no OTLP logs, no `migration.applied` audit events). This guard + makes that shape reachable from a rendered-empty value, so the seam now separates the + two cases: an ABSENT section keeps the no-op posture, a section that is present but + undecodable aborts startup. +- `KEYSTORE_SECRETMINLENGTH=` is now a startup error rather than a WARN plus a + disabled floor. `KEYSTORE_SECRETMINLENGTH=0` is unchanged — the explicit, + deprecated opt-out still works. +- Unset variables, YAML omission, YAML null, and explicit values behave exactly as + before. +- The public `Config.Unmarshal` seam enforces the same rule, so a consumer's own + config struct gets it without opting in — including its slice and map ELEMENTS, where + an empty element used to decode as a zero entry. +- Deliberately not covered, and worth naming so the sweep is not read as complete: + `""` bound to a `*bool` still decodes as a non-nil `false`, which defeats + `cache.critical`'s tri-state the same way `*0` defeated the secret floor (#1110); and + the typed getters `Config.Int`/`Int64`/`Float64`/`Bool` still return their default for + a present-but-empty key rather than reporting it (#1111). Both are the same defect + class at different seams; neither is numeric-decode, which is what this ADR closes. + +Migration: [C60.15](migrations.md). + +## Alternatives considered + +**Drop empty values in the env `TransformFunc`.** The obvious fix, and it breaks +ADR-051: with the key gone from the tree, a delivered-empty `DATABASE_HOST` reads as +a database-free deployment and boots green — the failure ADR-051 was written to stop. + +**Turn `WeaklyTypedInput` off.** It would take the coercion away along with string → +bool and every other conversion the framework's env-var surface depends on. The +problem is one source/target pair, not weak typing. + +**Let each key defend itself downstream.** That is the status quo, and it is why the +bug is uneven: `SERVER_BODYLIMIT` happened to have a `<= 0` fallback and +`KEYSTORE_SECRETMINLENGTH` did not. A rule that every numeric key has to remember is +a rule that some numeric key will forget. diff --git a/wiki/architecture_decisions.md b/wiki/architecture_decisions.md index 65ee1e58..a648aee9 100644 --- a/wiki/architecture_decisions.md +++ b/wiki/architecture_decisions.md @@ -1479,6 +1479,26 @@ already covers it. See [migrations.md](migrations.md) `[C60.13]`. --- +### [ADR-074: A Delivered-Empty Numeric Config Value Fails Startup](adr_074_delivered_empty_numeric_config.md) + +**Date:** 2026-08-20 | **Status:** Accepted + +`FOO=` — an empty `secretKeyRef`, an `envsubst` over an unset variable — delivers a set-but-empty +string that koanf keeps and mapstructure's `WeaklyTypedInput` rewrites to `0` for any numeric +target. Measured on `main`: `KEYSTORE_SECRETMINLENGTH=` decoded as `*0` and DISABLED the secret +floor, defeating ADR-065's tri-state (normalization fills a nil pointer; the decoder handed it a +non-nil pointer to zero). A decode hook now rejects an empty or whitespace-only string bound to a +numeric field — pointer targets included — in both decoder seams, so the failure names the key +instead of booting a config nobody wrote. Pruning empty keys from the koanf tree was rejected: the +ADR-051 identity check reads key presence, so dropping them boots the very misconfiguration it +exists to catch. `time.Duration` is exempt (it already fails loudly) and YAML null stays absence. + +**Key Benefits:** one rule for every numeric key, instead of a `<= 0` fallback each key must +remember. +**Watch:** `FOO=` no longer means `0` — a deployment relying on it, or carrying an empty +`secretKeyRef` it never noticed, now fails startup; `FOO=0` is unchanged. See `[C60.15]` +in [migrations.md](migrations.md). + ### [ADR-071: Upsert Column Sets Name Each Column Once, in a Form the Vendor Can Name](adr_071_upsert_column_sets_name_each_column_once.md) **Date:** 2026-08-20 | **Status:** Accepted diff --git a/wiki/migrations.md b/wiki/migrations.md index 14016107..a6c2e761 100644 --- a/wiki/migrations.md +++ b/wiki/migrations.md @@ -45,7 +45,7 @@ v0.39.1 ─E40─ v0.40.0 ─E401─ v0.40.1 ─E41─ v0.41.0 ─E42─ v0.42.0 | E58 | v0.57.0 → v0.58.0 | compile-break + breaking (C58.3 aborts startup) + behavior (C58.4, C58.5) | 5 | C58.1 C58.2 C58.3 | check every environment for a **negative** `cache.manager.maxsize` or `cache.manager.idlettl`, and — in multi-tenant mode with `cache.manager.maxsize` unset — a negative `multitenant.limits.tenants`, which becomes the pool size. Under `cache.enabled: false` such a value used to be inert and now aborts startup (C58.3); and if any dashboard, alert, or saved query reads OTLP-exported log records by a `service.*`, `telemetry.sdk.*`, or `deployment.environment.name` **record** attribute your code sets as a log field, re-key it to the `app.`-prefixed name (C58.4); and audit the log backend for dashboards, alerts, or saved queries filtering log records by **any** record-level resource attribute — the framework's `service.*` / `telemetry.sdk.*` / `deployment.environment.name` plus every key your deployment injects via `OTEL_RESOURCE_ATTRIBUTES` (`k8s.pod.name`, …) — since all of them move to resource level only; no code grep finds these (C58.5) | | E581 | v0.58.0 → v0.58.1 | silent-behavior | 3 | none | if you cache any type carrying a time.Time, decide before the bump whether a compare-and-set on a sub-second timestamp may fail during the rolling deploy (C581.1); and if `observability.logs.samplingrate` is set to any value strictly between 0.0 and 1.0, expect the exported INFO/DEBUG log volume AND the membership of the sampled set to change: a rate at or above 0.00005 and below 0.01 exported nothing before the bump and starts exporting its configured fraction after it, a rate that is not a whole percent stops flooring (0.999 was 99%, now 99.9%), and every fractional rate redraws which traces land in the sample; a rate below 0.00005, plus 0.0 and 1.0, are unaffected (C581.2); and if you call `Close()` directly on a `DbManager`/`CacheManager`/`messaging.Manager`, know that a handle still borrowed by in-flight work now stays open until its final release instead of closing immediately (C581.3) | | E59 | v0.58.1 → v0.59.0 | compile-break (C59.2, C59.3, C59.13) + silent-behavior (C59.1, C59.4, C59.14 turns a hand-built config's silently-off secret floor into a startup abort, C59.9 lets a valid Oracle case or whitespace variant build and write, while other newly-accepted spellings still fail later) + breaking (C59.5 rejects a password, C59.6 rejects a dynamic config's TLS material, C59.7 rejects an upsert call, C59.8 rejects another, C59.10 rejects a duplicated conflict column, C59.11 rejects a `database.tls` shape at startup, C59.12 rejects a hand-built config at construction) | 14 | C59.2 C59.3 C59.13 (only partially — a hand-built config that never set the field still compiles; see C59.14) | if your service sits behind a proxy on a **public** address (CloudFront, a partner edge), set `server.trustedproxies` to its CIDR range before the bump — otherwise that proxy is itself returned as the client and every caller behind it collapses into a single rate-limit bucket; and check the load balancer for any mode that writes a non-IP `X-Forwarded-For` entry — on AWS ALB that is `routing.http.xff_client_port.enabled` (appends `client_ip:port`) and, separately, `routing.http.xff_header_processing.mode = remove` — since either keys the entire fleet on the load balancer's own address after the bump and `server.trustedproxies` cannot fix it; the remedy is deployment-side (C59.1); and if any of your code — **including test files**, which `go build` does not compile — implements `cache.Cache`, add the new `CompareAndDelete` method, and before swapping a lock's `Delete` release for it make sure the lock is acquired with a **positive** TTL, since a `ttl == 0` lock that a token-verified release declines to remove is held forever (C59.3); and grep your **test** files for a `cache/testing.MockCache` handed a context that is already canceled or expired while the call is expected to succeed — the mock's cancellation check no longer depends on a configured `WithDelay`, so that call now returns the context's error (C59.4); and if you call `ProvisionPGRoles` or `PGRoleProvisioningSQL` with a `PGRoleSpec` whose `MigratorPassword` or `RuntimePassword` is read from a file, a mounted secret, an environment read, or a command substitution, `strings.TrimSpace` it before the bump — a password containing CR, LF, or NUL is now rejected by `Validate` instead of provisioning, and any credential whose provisioning failure was logged while its password contained a newline should be rotated, since the first line of that secret reached the error string (C59.5); and hand-read every `BuildUpsert` call for a column key present in BOTH `conflictColumns` and `updateColumns` — grep finds the calls but not the overlap, since both maps are usually built dynamically — because on PostgreSQL such a call built and ran before the bump and now returns an error from the builder, while on Oracle it already failed and now fails earlier, at build time with the builder's message rather than at execution with ORA-38104; match keys the way each vendor does, since Oracle folds the unquoted identifiers it emits to upper case (so `id` and `ID` are one column there, and are now rejected) while PostgreSQL quotes every identifier and keeps them distinct; then compare that column's update value against its insert value before remediating — equal means dropping it from `updateColumns` changes no column value — though if it is the column's **only** entry the set empties, which builds `DO NOTHING` on PostgreSQL and drops Oracle's `WHEN MATCHED` arm, so a matched row stops being updated at all, its UPDATE triggers stop firing and `RETURNING` yields no row; keep a real non-conflict column or issue an explicit `UPDATE` — under the same transaction and locking rule as below — where that matters — but differing means the call was rewriting the conflict column on a matched row, which no vendor-portable upsert can express, so those need a separate `UPDATE` rather than a dropped column — run it in the same transaction as the insert, keyed on the conflict columns, and holding the row lock the single statement took for you (`SELECT … FOR UPDATE` or equivalent), because splitting one atomic upsert into two statements lets a concurrent writer interleave and under READ COMMITTED a shared transaction alone does not stop it (C59.7); and if a dynamic multi-tenant `DBConfigProvider` returns a `database.connectionstring` with no `type`, that tenant now dials a real database at first use instead of failing every request with `unsupported database type: ""`, and if you supply `Options.DatabaseConnector` it now receives an inferred `type` for a recognized scheme instead of an empty one — inference is unconditional, since that option's exemption only ever covered the startup guard; then enumerate the same source for any tenant carrying `database.tls.cert`/`database.tls.key`/`database.tls.ca` next to an Oracle type or `oracle://` DSN, or exactly one of `database.tls.cert`/`database.tls.key` on a PostgreSQL one, since those connect today with the TLS material silently dropped and stop connecting after the bump, typed configs included (C59.6); and hand-read the `BuildUpsert` shortlist again for a conflict column that names no column of `insertColumns` by vendor identity — Oracle already rejected those, PostgreSQL let the column fall to its table default, which was inert where the default was absent but is a working pattern where it is a sequence, a `current_setting(...)` or a generated column, and after the bump both vendors refuse it, so a sequence or `current_setting(...)` default means passing the value in `insertColumns` while a generated column — which PostgreSQL forbids writing directly — means `database.Raw` or a schema change; drop any match on the precondition texts too, since `conflict columns required for Oracle MERGE` and `conflict columns required for PostgreSQL upsert` both become `conflict columns required for upsert` (C59.8); and check whether any `BuildUpsert` call can pass the same column twice in `conflictColumns` — duplicates are judged by vendor identity, so an exact repeat on either vendor, or on Oracle a case variant of a non-reserved identifier, which Oracle emits unquoted and folds (its reserved words are quoted and stay case-sensitive, so `["level", "LEVEL"]` is still accepted; on PostgreSQL no case variant is a duplicate) — because both vendors now refuse it where PostgreSQL previously failed only at execution (`42P10`) and Oracle accepted it outright, so de-duplicate at the call site by the vendor's own rules rather than by lower-casing, which is wrong on PostgreSQL (C59.10); and read that shortlist once more for an Oracle call whose conflict column and insert key spell the same column differently — a case variant or a whitespace-padded key — since those were refused at build time and now build and write, with no signal after the bump, so any validation you were getting from that rejection has to move into your own code before it (C59.9); and grep every environment for a `database.tls` block, because four shapes that booted green now abort startup — static configuration at boot, while a dynamic `DBConfigProvider` record with the same shape boots green and fails at first connection acquisition, so check dynamic tenants by acquiring a connection: a PostgreSQL `mode` outside the sslmode allowlist, `cert`/`key`/`ca` under an empty/`disable`/`allow`/`prefer` mode (pgx was discarding the material, allowing a plaintext downgrade, or — for `ca: system` — silently upgrading the mode to `verify-full`; an unset mode defaults to `prefer`, so this is the common case), any `database.tls.*` alongside a `connectionstring`, and — extending C42.1, which said mode alone still passed — `database.tls.mode` on Oracle; setting `verify-ca`/`verify-full` — or `require` plus a `ca` — also makes that connection verify the server for the first time, so confirm the CA chain before the bump, while bare `require` encrypts without authenticating the server (C59.11); and if any code hands `app.NewWithConfig` or a direct `app.Builder` chain a hand-built config, run it against `config.Validate`'s rules before the bump — missing `app.name`/`app.version`, zero server timeouts and invalid vendors now fail construction (C59.12); and if any Go code sets `config.KeyStoreConfig.SecretMinLength`, write `new(0)` / `new(N)` — the field is now `*int` (C59.13); and if a hand-built config with symmetric secrets never set it, it relied on the Go zero silently disabling the floor and now gets 32 bytes, so any secret shorter than that fails startup — grep finds nothing for this shape, read C59.14 | -| E60 | v0.59.0 → v0.60.0 | compile-break (C60.4 — internal helpers nothing outside app/ used; C60.14 — the 33 `config.TestKey*` constants are deleted) + breaking (C60.1 fails a migrate run; C60.16 re-addresses a non-root database section's ConfigError.Field; C60.11 rejects an upsert call whose column keys name one Oracle column twice, that names an Oracle conflict column it also updates under another spelling, or that Oracle's MERGE cannot name — and, on BOTH vendors, one whose key carries an unescaped interior quote; C60.12 rejects a negative scheduler timeout and a module Init with no config) + silent-behavior (C60.2 forwards `database.tls` to Flyway, where nothing reached it before; C60.3 changes strings on `/ready`'s 200 body — including the `db_stats` → `database_stats` rename — and folds two entries out of the debug health view; no status code moves; C60.5 — idle-cleanup sweeps start at manager construction; five startup/shutdown log lines retire; C60.7 — the AMQP failure and panic lines' second `correlation_id` stamp becomes `amqp_correlation_id`; C60.8 — inbound trace identifiers failing validation are discarded and regenerated; C60.9 — the streams lane's consume telemetry and log lines change shape; C60.10 — a published message's `CorrelationId` carries the same id as its own `X-Request-ID` header, or nothing at all where that id fails validation; C60.13 — the default log filter stops masking a field matched only by the removed bare `key` needle; the named ones, `api_key`, `private_key`, `signing_key`, `encryption_key`, still mask) + compile-break (C60.6 — `messaging.StartConsumeSpan` removed; the consumed-messages counter now counts at completion with `error.type`) | 15 | C60.4; C60.14; C60.6 (only partially — the code door is compiler-caught; the telemetry door is silent, see its gate) | if you run the `go-bricks-migrate` CLI, resolve every tenant it reads before the bump — `go-bricks-migrate info` per tenant is the cheap check — because configs it accepted are now validated the way the framework validates them before dialing, which covers the `database.tls` shapes of [C59.11] plus a missing Oracle connection identifier and an unloadable `database.timezone`; the per-tenant AWS Secrets Manager payloads cannot be grepped from your repo, so enumerate the prefix in each account rather than trusting a config-file sweep (C60.1); and grep dashboards, alerts, synthetic checks and contract tests for `db_stats`, `not_ready`, `connection_failed`, `no_active_connections`, `database_manager`/`messaging_manager`, for `messaging_stats`/`cache_stats` pinned to `{}` on a service without that kind, and for `overall_status == unknown` on `/_sys/health-debug` — every kind now speaks one vocabulary (`healthy`, `unhealthy`, `not_configured`, `disabled`, `per_tenant`), a disabled kind's stats read `{"status":"disabled"}`, messaging and cache read `per_tenant` in multi-tenant deployments where they read `not_configured`, and the debug view lists every classic kind; the 200 body's `db_stats` key is now `database_stats`, the debug view's `database_manager`/`messaging_manager` entries are gone (their statistics are the `database`/`messaging` entries' details), and `overall_status` reads `degraded` where it read `unknown` for a non-critical kind that is not live; a critical kind that is down still answers 503 exactly as before (C60.3); and grep your Go code — test files included — for the sixteen removed app helpers and the eight unexported debug response types (C60.4). And grep log-based alerts and saved queries for the five retired cleanup-loop lines (`Starting/Stopping database manager cleanup loop`, `Starting/Stopping messaging manager cleanup loop`, `Manager cleanup loops stopped`) — they have no renamed equivalent (C60.5). And grep your Go code for `StartConsumeSpan` and your dashboards for `messaging.client.consumed.messages` — the call is a build break, the counter now increments at completion and carries `error.type` on failure (C60.6). And grep log-based alerts, saved queries and log-parsing tests for `correlation_id` on the AMQP consumer's failure and panic lines — the delivery's own AMQP `CorrelationId` is stamped as `amqp_correlation_id` there now, and `correlation_id` carries only the framework trace ID (C60.7). And search your log backend for `correlation_id` values that are empty, longer than 128 characters, or contain anything outside `A-Za-z0-9_-` — those identifiers are now discarded at ingress and replaced by a framework-minted one, so correlation with the upstream that emitted them breaks; that query covers the X-Request-ID half only, so also check the gateway emitting your `traceparent`/`tracestate`, which are now validated too and show up in no log field (C60.8). And if you consume native streams, grep dashboards and log queries for the streams consumer's failure and panic lines and for its span attributes — the lane now runs on the shared delivery pipeline, so its lines gain the spine, its panic line's wording changes, and its span gains the four shared attributes (C60.9). And if anything downstream reads a message's AMQP `CorrelationId` property rather than its `X-Request-ID` header, that property now carries the same aligned trace id the header carries — a 32-hex traceparent trace-id where an HTTP-originated publish used to put the request's UUID (C60.10). And if you build `config.Config` in Go or call `scheduler.Module.Init` / `app.NewModuleRegistry` yourself, check both `scheduler.timeout.*` keys: zero now normalizes (30s/25s, where the module used to fall back to 30s for slowjob), a negative value fails validation, and a module handed no config fails Init (C60.12). And if you build Oracle upserts, grep `BuildUpsert` call sites for column maps assembled dynamically — two keys that fold to one Oracle column, a conflict column spelled differently from the insert or update key naming that same column, and dotted or function-shaped keys, are now build-time errors rather than SQL Oracle rejects at parse or at execution (C60.11); on PostgreSQL the only new rejection is a key carrying a quote that is not doubled, which used to leave the identifier and become SQL, and no identity rule changes there. And grep your Go code for `ConfigError.Field` compared to a literal `database.*` key: a non-root database section now reports `databases..host` / `multitenant.tenants..database.host` there instead of the root spelling, and its message loses the `databases.: ` prefix (C60.16). And read C60.13 before upgrading if you log any field whose name contains `key`: the bare `key` needle is gone from the default log filter, so a field matched only by it — an identifier like `tenant_key`, but also a secret the new list does not name, such as `license_key` — logs in clear until you add a needle via `log.sensitivefields`; the named needles (`api_key`, `private_key`, `signing_key`, `encryption_key`, each in three spellings) still mask. And grep your Go code for the `TestKey` identifier itself — not for a `config.` qualifier, which an aliased or dot-imported reference does not carry; C60.14 has the exact pattern, whose alternation cannot live in this cell. Those 33 constants are deleted, and five of them named keys the loader never read, so C60.14's table tells you which value to inline instead of the one they held. | +| E60 | v0.59.0 → v0.60.0 | compile-break (C60.4 — internal helpers nothing outside app/ used; C60.14 — the 33 `config.TestKey*` constants are deleted) + breaking (C60.1 fails a migrate run; C60.15 fails configuration resolution on a numeric key delivered empty; C60.16 re-addresses a non-root database section's ConfigError.Field; C60.11 rejects an upsert call whose column keys name one Oracle column twice, that names an Oracle conflict column it also updates under another spelling, or that Oracle's MERGE cannot name — and, on BOTH vendors, one whose key carries an unescaped interior quote; C60.12 rejects a negative scheduler timeout and a module Init with no config) + silent-behavior (C60.2 forwards `database.tls` to Flyway, where nothing reached it before; C60.3 changes strings on `/ready`'s 200 body — including the `db_stats` → `database_stats` rename — and folds two entries out of the debug health view; no status code moves; C60.5 — idle-cleanup sweeps start at manager construction; five startup/shutdown log lines retire; C60.7 — the AMQP failure and panic lines' second `correlation_id` stamp becomes `amqp_correlation_id`; C60.8 — inbound trace identifiers failing validation are discarded and regenerated; C60.9 — the streams lane's consume telemetry and log lines change shape; C60.10 — a published message's `CorrelationId` carries the same id as its own `X-Request-ID` header, or nothing at all where that id fails validation; C60.13 — the default log filter stops masking a field matched only by the removed bare `key` needle; the named ones, `api_key`, `private_key`, `signing_key`, `encryption_key`, still mask) + compile-break (C60.6 — `messaging.StartConsumeSpan` removed; the consumed-messages counter now counts at completion with `error.type`) | 16 | C60.4; C60.14; C60.6 (only partially — the code door is compiler-caught; the telemetry door is silent, see its gate) | if you run the `go-bricks-migrate` CLI, resolve every tenant it reads before the bump — `go-bricks-migrate info` per tenant is the cheap check — because configs it accepted are now validated the way the framework validates them before dialing, which covers the `database.tls` shapes of [C59.11] plus a missing Oracle connection identifier and an unloadable `database.timezone`; the per-tenant AWS Secrets Manager payloads cannot be grepped from your repo, so enumerate the prefix in each account rather than trusting a config-file sweep (C60.1); and grep dashboards, alerts, synthetic checks and contract tests for `db_stats`, `not_ready`, `connection_failed`, `no_active_connections`, `database_manager`/`messaging_manager`, for `messaging_stats`/`cache_stats` pinned to `{}` on a service without that kind, and for `overall_status == unknown` on `/_sys/health-debug` — every kind now speaks one vocabulary (`healthy`, `unhealthy`, `not_configured`, `disabled`, `per_tenant`), a disabled kind's stats read `{"status":"disabled"}`, messaging and cache read `per_tenant` in multi-tenant deployments where they read `not_configured`, and the debug view lists every classic kind; the 200 body's `db_stats` key is now `database_stats`, the debug view's `database_manager`/`messaging_manager` entries are gone (their statistics are the `database`/`messaging` entries' details), and `overall_status` reads `degraded` where it read `unknown` for a non-critical kind that is not live; a critical kind that is down still answers 503 exactly as before (C60.3); and grep your Go code — test files included — for the sixteen removed app helpers and the eight unexported debug response types (C60.4). And grep log-based alerts and saved queries for the five retired cleanup-loop lines (`Starting/Stopping database manager cleanup loop`, `Starting/Stopping messaging manager cleanup loop`, `Manager cleanup loops stopped`) — they have no renamed equivalent (C60.5). And grep your Go code for `StartConsumeSpan` and your dashboards for `messaging.client.consumed.messages` — the call is a build break, the counter now increments at completion and carries `error.type` on failure (C60.6). And grep log-based alerts, saved queries and log-parsing tests for `correlation_id` on the AMQP consumer's failure and panic lines — the delivery's own AMQP `CorrelationId` is stamped as `amqp_correlation_id` there now, and `correlation_id` carries only the framework trace ID (C60.7). And search your log backend for `correlation_id` values that are empty, longer than 128 characters, or contain anything outside `A-Za-z0-9_-` — those identifiers are now discarded at ingress and replaced by a framework-minted one, so correlation with the upstream that emitted them breaks; that query covers the X-Request-ID half only, so also check the gateway emitting your `traceparent`/`tracestate`, which are now validated too and show up in no log field (C60.8). And if you consume native streams, grep dashboards and log queries for the streams consumer's failure and panic lines and for its span attributes — the lane now runs on the shared delivery pipeline, so its lines gain the spine, its panic line's wording changes, and its span gains the four shared attributes (C60.9). And if anything downstream reads a message's AMQP `CorrelationId` property rather than its `X-Request-ID` header, that property now carries the same aligned trace id the header carries — a 32-hex traceparent trace-id where an HTTP-originated publish used to put the request's UUID (C60.10). And if you build `config.Config` in Go or call `scheduler.Module.Init` / `app.NewModuleRegistry` yourself, check both `scheduler.timeout.*` keys: zero now normalizes (30s/25s, where the module used to fall back to 30s for slowjob), a negative value fails validation, and a module handed no config fails Init (C60.12). And if you build Oracle upserts, grep `BuildUpsert` call sites for column maps assembled dynamically — two keys that fold to one Oracle column, a conflict column spelled differently from the insert or update key naming that same column, and dotted or function-shaped keys, are now build-time errors rather than SQL Oracle rejects at parse or at execution (C60.11); on PostgreSQL the only new rejection is a key carrying a quote that is not doubled, which used to leave the identifier and become SQL, and no identity rule changes there. And grep your Go code for `ConfigError.Field` compared to a literal `database.*` key: a non-root database section now reports `databases..host` / `multitenant.tenants..database.host` there instead of the root spelling, and its message loses the `databases.: ` prefix (C60.16). And read C60.13 before upgrading if you log any field whose name contains `key`: the bare `key` needle is gone from the default log filter, so a field matched only by it — an identifier like `tenant_key`, but also a secret the new list does not name, such as `license_key` — logs in clear until you add a needle via `log.sensitivefields`; the named needles (`api_key`, `private_key`, `signing_key`, `encryption_key`, each in three spellings) still mask. And grep your Go code for the `TestKey` identifier itself — not for a `config.` qualifier, which an aliased or dot-imported reference does not carry; C60.14 has the exact pattern, whose alternation cannot live in this cell. Those 33 constants are deleted, and five of them named keys the loader never read, so C60.14's table tells you which value to inline instead of the one they held. And grep every deployment surface — Helm values, Kustomize overlays, `.env` files, rendered manifests — for a go-bricks variable set to nothing (`FOO=`, `FOO=""`, a structured `value: ""`, a `secretKeyRef` whose stored value is empty, an `envsubst` over an unset variable): a numeric key delivered empty used to decode as `0` and now fails naming the key, which is how `KEYSTORE_SECRETMINLENGTH=` silently disabled the secret-length floor; then resolve the two seams no environment sweep reaches — the CLI's `tenants.yaml` and every stored `DBConfigProvider` payload — because those decode at first use, so a clean environment and a green startup say nothing about them (C60.15). | **4 — Read each atom's gate before acting.** Every atom carries `when: match | no-match | always`: @@ -3238,7 +3238,11 @@ None of them is exhaustive — all three are line-oriented and blind to an impor (C60.16); and grep your log calls for field names containing `key`, since the default filter stops masking them (C60.13); and `git grep -nE '(^|[^[:alnum:]_])TestKey[A-Za-z0-9_]*([^[:alnum:]_]|$)' -- '*.go'`, since those constants are gone — match the identifier, not the `config.` qualifier, so an aliased or - dot-imported reference cannot hide from the sweep (C60.14) + dot-imported reference cannot hide from the sweep (C60.14); and grep every deployment surface for a go-bricks variable + set to nothing (`FOO=`, `FOO=""`, a structured `value: ""`, a `secretKeyRef` whose stored value + is empty, an `envsubst` over an unset variable), because a numeric key delivered empty now fails + configuration resolution instead of decoding as `0` — at startup for the service's own config, + at first use for the CLI's `tenants.yaml` and a dynamic `DBConfigProvider` payload (C60.15) - exit: `go get github.com/gaborage/go-bricks@v0.60.0 && go mod tidy && go build ./... && go test ./...` ### [C60.1] `go-bricks-migrate` validates every resolved database config · breaking · when: match @@ -3823,6 +3827,92 @@ None of them is exhaustive — all three are line-oriented and blind to an impor (`normalizeScheduler`) · `scheduler/module.go` (`Init`, `Shutdown`, `determineJobSeverity`) +### [C60.15] a numeric config key delivered empty fails configuration resolution · breaking · when: match + +- detect: grep every deployment surface — Helm values, Kustomize overlays, `.env` files, + Task/Compose definitions, CI secrets — for a go-bricks variable **set to nothing**: + `grep -rnE "^[[:space:]]*[A-Z0-9_]+=[[:space:]]*(\"[[:space:]]*\"|'[[:space:]]*')?[[:space:]]*(#.*)?$"` + over env files — written to catch the QUOTED spellings too, since `FOO=""`, `FOO=''` and + `FOO=" "` all deliver what this rule rejects: the guard TRIMS, so quoted whitespace is as + empty as nothing at all — plus + `grep -rnE ":[[:space:]]*(\"[[:space:]]*\"|'[[:space:]]*')[[:space:]]*(#.*)?$"` over Helm + values, Kustomize overlays and Compose files, where the empty value is structured + (`value: ""`, `value: " "`, `FOO: ''`) and no `=` appears at all. Both tolerate a trailing + comment, which is where a deliberately-blanked value tends to be documented + (`value: "" # intentionally blank`). Neither matches a bare `key:` with nothing after it — + that is YAML **null**, which is absence and still takes the default. Plus any + `secretKeyRef`/`configMapKeyRef` whose source key holds an EMPTY value, and any `envsubst` + template over a variable that can be unset. Note which secret shape actually bites: a + MISSING key does not produce `FOO=` — with `optional: true` Kubernetes leaves the variable + unset, and with `optional: false` the container will not start — so only a present key with + an empty payload reaches this rule. A repo grep will not find those last two: read the + rendered manifest, or `kubectl exec … env | grep -E '=[[:space:]]*$'` in a running pod + (whitespace-aware, because a whitespace-only value is rejected too). +- scope: a set-but-empty string bound to a NUMERIC config key used to decode as `0` + (koanf keeps the key; mapstructure's `WeaklyTypedInput` rewrote `""` to `0`). It now + fails `config.Load` — and the public `Config.Unmarshal` — with an error naming the key + and reporting it was `delivered empty`. Pointer targets are included, which is the + damaging case: `KEYSTORE_SECRETMINLENGTH=` decoded as `*0` and DISABLED the secret-length + floor rather than taking ADR-065's 32-byte default, because a non-nil pointer to zero + reads as an operator choice. `SERVER_BODYLIMIT=` decoded as `0` and was rescued only by a + downstream fallback. Unchanged: an unset variable, an omitted YAML key, a YAML **null** + (different plumbing — still absence, still takes the default), an explicit value + including an explicit `0`, and every non-numeric key — `DATABASE_HOST=` still produces + ADR-051's database-identity error, not this one — with ONE exception: `database.port` is + both an identity key and numeric, so `DATABASE_PORT=` now fails at decode with this + message instead of ADR-051's, which means it no longer lists every other offending + identity key alongside it. `time.Duration` keys are exempt: an empty string already + failed loudly there. A whitespace-only value is rejected too (it already failed to parse; + the message is now the same one). Through the public `Config.Unmarshal`, slice and map + ELEMENTS are judged as well — `["1","","3"]` into a `[]int` used to decode `[1 0 3]` and + now fails naming the index. Two other seams carry the rule: the `go-bricks-migrate` CLI + applies it to `tenants.yaml`, and a dynamic `DBConfigProvider` payload read through + `migration.SecretsProvider` applies it to the secret's own numeric fields, so a rotated + secret rendering `{"port": ""}` fails instead of dialing port 0. +- gate: match = an empty value can reach a numeric key through ANY of the four seams — an + environment variable or YAML key in the service's own configuration, the public + `Config.Unmarshal`, the CLI's `tenants.yaml`, or a stored `DBConfigProvider` payload. Read + that wider than it sounds: for most numeric keys `0` means "use the default", so an empty + value was BENIGN before this change (`CACHE_REDIS_PORT=` resolved to 6379, + `OUTBOX_BATCHSIZE=` to its default) and those deployments were healthy. They now fail — + at startup for the first two seams, at first use for the last two. no-match = every one of + those sources is either absent or carries a real value; a clean environment ALONE is not + no-match, since the CLI and dynamic seams resolve payloads no env sweep sees. +- apply: unset the variable, or give it an explicit value. For a `secretKeyRef`, fix the + stored VALUE rather than the reference: an absent key leaves the variable unset (or blocks + the pod), so what reaches this rule is a key whose payload is empty. For a dynamic + `DBConfigProvider`, check the stored secret payload itself: a rotation that writes `""` + for a numeric field now fails that tenant's config resolution. In a multi-tenant + deployment sweep EVERY tenant record (`multitenant.tenants..*`, `databases..*`), + not just top-level keys: one bad record now fails `config.Load` for the whole process, + where before it produced a zero for that tenant alone. `FOO=0` is + still the way to say zero where zero is legal (`KEYSTORE_SECRETMINLENGTH=0` keeps + disabling the floor, deprecated but unchanged). +- verify: boot the service in staging with its real manifest. A startup failure naming a + key with `delivered empty` is this rule; fix the source and re-boot. Two naming + caveats: the message carries the koanf key (`keystore.secretminlength`), not the + environment variable, and on the `Config.Unmarshal` seam it carries the Go field path + of the target struct instead (`Trace.Batch.Size`), so grep your manifest for the key's + env spelling rather than the string in the error. `observability.*` decodes through + that seam, and a delivered-empty value there now ABORTS startup where it previously + fell back to a no-op provider — telemetry silently off — so that section is worth + checking first. To confirm the + posture before deploying, run `kubectl exec -- env | grep -E '=[[:space:]]*$'` + against the current release — every line it prints is a candidate, whitespace-only values + included, since those are rejected too. + + Booting the service exercises only the two seams that resolve at startup — `config.Load` + and the `Config.Unmarshal` calls the framework makes for you. Check the other two: run `go-bricks-migrate info` per tenant to put the CLI's + `tenants.yaml` through the same guard, and for a dynamic `DBConfigProvider`, resolve every + stored secret payload (acquire a connection per tenant, or call your provider's resolve + path directly) — those decode at first use rather than at boot, so a green startup says + nothing about them. +- ref: [ADR-074](adr_074_delivered_empty_numeric_config.md) · + [ADR-051](adr_051_delivered_empty_database_identity.md) · + [ADR-065](adr_065_keystore_secretminlength_tristate.md) · + `internal/configdecode/configdecode.go` (`EmptyStringToNumericGuardHookFunc`) · + `config/config.go` (`buildDecoderConfig`, `unmarshalDecoderConfig`) + ### [C60.16] a database section's `ConfigError.Field` names that section · breaking · when: match - detect: `git grep -nE 'ConfigError|errors\.As' -- '*.go'` in your own code, then read every