fix(config): converge env/DB precedence onto one rule with an explicit-vs-seeded marker (#794)

.version

@@ -1,6 +1,6 @@
 {
   "major": 1,
   "minor": 8,
-  "patch": 21,
+  "patch": 22,
   "prerelease": ""
 }

api-schema/tmi-openapi.json

@@ -3,7 +3,7 @@
   "info": {
     "title": "TMI (Threat Modeling Improved) API",
     "description": "A RESTful API for collaborative threat modeling with full X6 graph library compatibility. This API provides schemas that align with AntV X6 cell object models for seamless integration with modern diagramming libraries. Supports OAuth 2.0 authentication with client callback integration for seamless single-page application authentication flows.\n\n## API Design v1.1.0\n\n### Authorization Model\nTMI uses hierarchical authorization: access control is defined at the ThreatModel level via the authorization field (readers, writers, owners). All child resources (Assets, Diagrams, Documents, Notes, Repositories, Threats) inherit permissions from their parent ThreatModel. This simplifies permission management and ensures consistent access control.\n\n### Bulk Operations\nNotes and Diagrams do not support bulk operations due to their unique creation workflows and lack of valid bulk use cases. All other resources (Threats, Assets, Documents, Repositories) support full bulk operations: POST (create), PUT (upsert), PATCH (partial update), DELETE (batch delete).\n\nAll resources support bulk metadata operations regardless of resource-level bulk support.\n\n### List Response Strategy\n- ThreatModels return summary information (TMListItem) because they contain many child objects that can be large.\n- Diagrams return summary information (DiagramListItem) because diagram data (cells, images) can be large.\n- Notes return summary information (NoteListItem) because the content field can be large.\n- Threats, Assets, Documents, Repositories return full schemas as they are relatively small and static.\n\n### PATCH Support\nAll resources support PATCH for partial updates using JSON Patch (RFC 6902). This is particularly useful for:\n- Assets: Array field updates (affected_assets, trust_boundaries) ensuring no duplicates\n- Notes: Updating name/description without changing content field\n- All resources: Efficient updates without full object replacement\n",
-    "version": "1.8.21",
+    "version": "1.8.22",
     "contact": {
       "name": "TMI Development Team",
       "url": "https://github.com/ericfitz/tmi",

api/config_handlers.go

@@ -615,7 +615,7 @@ func (s *Server) GetSystemSetting(c *gin.Context, key string) {
 }
 
 // UpdateSystemSetting creates or updates a system setting (admin only)
-// SEM@5dfa9dcf64aa0662920dbbab3bca200db1b22c73: create or update a database system setting, validating provider enables and invalidating cache (reads DB)
+// SEM@0000000000000000000000000000000000000000: create or update a database system setting with explicit origin, validating provider enables and invalidating cache (reads DB)
 func (s *Server) UpdateSystemSetting(c *gin.Context, key string) {
 	logger := slogging.Get().WithContext(c)
 	ctx := c.Request.Context()
@@ -676,14 +676,17 @@ func (s *Server) UpdateSystemSetting(c *gin.Context, key string) {
 		}
 	}
 
-	// Convert to model
+	// Convert to model. Origin is stamped explicit here (an admin deliberately
+	// PUT this value) and again by SettingsService.Set below, which is the
+	// authoritative writer — belt and suspenders (#794).
 	setting := models.SystemSetting{
 		SettingKey:  models.DBVarchar(key),
 		Value:       models.DBText(req.Value),
 		SettingType: models.DBVarchar(string(req.Type)),
 		ModifiedAt:  time.Now(),
 		ModifiedBy:  models.NewNullableDBVarchar(modifiedBy),
 		Description: models.NewNullableDBText(req.Description),
+		Origin:      models.NullableDBVarchar{String: models.SystemSettingOriginExplicit, Valid: true},
 	}
 
 	// Enable-validation gate: validate required fields when enabling a provider

api/config_handlers_test.go

@@ -46,7 +46,7 @@ func (m *MockSettingsService) GetString(ctx context.Context, key string) (string
 	return "", nil
 }
 
-func (m *MockSettingsService) GetDatabaseString(ctx context.Context, key string) (string, bool, error) {
+func (m *MockSettingsService) GetResolvedString(ctx context.Context, key string) (string, bool, error) {
 	if setting, ok := m.settings[key]; ok {
 		return string(setting.Value), true, nil
 	}

api/models/system_setting.go

@@ -8,7 +8,7 @@ import (
 // SystemSetting represents a system-wide configuration setting stored in the database.
 // These settings can be modified at runtime without requiring server restart.
 // Settings are cached with short TTL for performance.
-// SEM@db6c3b75a42a48dd122e5984e9efdf0e6e15ca9d: GORM model for a runtime-configurable system setting with key, typed value, and audit fields (reads DB)
+// SEM@2daf3be663df9da54323f16d115f12d78d435c3f: GORM model for a runtime-configurable system setting with key, typed value, and audit fields (reads DB)
 type SystemSetting struct {
 	// SettingKey is the unique identifier for this setting (e.g., "rate_limit.requests_per_minute")
 	// Named SettingKey instead of Key to avoid Oracle reserved word conflict
@@ -20,6 +20,36 @@ type SystemSetting struct {
 	Description NullableDBText    `gorm:"" json:"description,omitempty"`
 	ModifiedAt  time.Time         `gorm:"not null;autoUpdateTime" json:"modified_at"`
 	ModifiedBy  NullableDBVarchar `gorm:"size:36" json:"modified_by,omitempty"` // User InternalUUID
+	// Origin records who wrote this row's current value: SystemSettingOriginSeeded
+	// when SeedDefaults inserted the registry default at first boot, or
+	// SystemSettingOriginExplicit when an operator deliberately set it (via the
+	// admin API, SettingsService.Set, or dbtool --import-config).
+	//
+	// NULL means SEEDED — the fail-safe direction. Every way an origin value
+	// can go missing (an empty string bound on Oracle, a future writer that
+	// forgets the stamp, a stale pre-upgrade Redis entry whose cached JSON has
+	// no origin key) therefore degrades to "seeded", which makes the config
+	// layer win. Config is the layer an operator can see and control, so
+	// losing this value must never hand authority to a database row nobody set
+	// — that is precisely the 2026-08-20 outage (oracle-db-admin review, #794).
+	//
+	// Rows that predate this column are stamped explicit where they show
+	// operator intent, by BackfillSystemSettingOrigin in internal/dbschema.
+	// The column itself is added with no `ALTER TABLE ... DEFAULT` (Oracle
+	// rejects unquoted string defaults — see the SettingType comment above).
+	// Never write "" here: Oracle binds an
+	// empty string as NULL, so an empty string and NULL would be
+	// indistinguishable there. Only NULL, SystemSettingOriginSeeded, or
+	// SystemSettingOriginExplicit are valid values.
+	// Not part of the wire API: modelToAPISystemSetting (api/config_handlers.go)
+	// builds the API SystemSetting field by field and never copies Origin
+	// across, so it never reaches a client regardless of this tag. The tag is
+	// a real json name (not "-") because the Redis cache tier round-trips a
+	// SystemSetting through json.Marshal/Unmarshal (setInRedisCache /
+	// getFromRedisCache) — "-" would silently drop Origin on every Redis
+	// cache hit and make every setting look explicit after a warm read
+	// (oracle-db-admin review, #794).
+	Origin NullableDBVarchar `gorm:"size:16" json:"origin,omitempty"`
 	// Source indicates where the effective value comes from: "database", "config", "environment", "vault"
 	// Computed at response time, not stored in the database.
 	Source string `gorm:"-" json:"source"`
@@ -47,6 +77,28 @@ const (
 	SystemSettingTypeFloat = "float"
 )
 
+// SystemSettingOrigin constants for the Origin field
+const (
+	// SystemSettingOriginSeeded marks a row inserted by SeedDefaults with a
+	// registry default value — no operator has ever set it.
+	SystemSettingOriginSeeded = "seeded"
+	// SystemSettingOriginExplicit marks a row an operator deliberately set,
+	// via the admin API, SettingsService.Set, or dbtool --import-config.
+	SystemSettingOriginExplicit = "explicit"
+)
+
+// IsExplicit reports whether an operator deliberately set this row's value,
+// as opposed to it having been seeded with a registry default at first boot.
+//
+// Only an explicit "explicit" counts. NULL, "seeded", and any unexpected value
+// all read as NOT explicit, so the database only outranks an explicitly
+// configured env/YAML value when something deliberately said so. See the
+// Origin field's comment for why that polarity is the safe one.
+// SEM@2daf3be663df9da54323f16d115f12d78d435c3f: report whether a setting's value was deliberately set rather than seeded (pure)
+func (s *SystemSetting) IsExplicit() bool {
+	return s.Origin.Valid && s.Origin.String == SystemSettingOriginExplicit
+}
+
 // DefaultSystemSettings returns the default system settings that should be seeded
 // when the database is initialized. These provide sensible defaults that can be
 // overridden by administrators.

api/models/system_setting_test.go

@@ -0,0 +1,94 @@
+package models
+
+import (
+	"encoding/json"
+	"testing"
+)
+
+// TestSystemSetting_IsExplicit pins the #794 precedence rule and, critically,
+// its polarity: "explicit" is the ONLY value that yields true. NULL, "seeded",
+// and anything unexpected all read as not-explicit, so the database outranks a
+// configured env/YAML value only when something deliberately said so.
+//
+// The polarity is the safety property. Every way an origin value can go
+// missing — an empty string bound on Oracle, a writer that forgets the stamp,
+// a stale pre-upgrade Redis entry with no origin key — lands on "not
+// explicit", which makes the config layer win. Config is what an operator can
+// see and control; a row nobody set winning is the 2026-08-20 outage.
+func TestSystemSetting_IsExplicit(t *testing.T) {
+	tests := []struct {
+		name   string
+		origin NullableDBVarchar
+		want   bool
+	}{
+		{
+			name:   "NULL origin is not explicit (fail-safe)",
+			origin: NullableDBVarchar{Valid: false},
+			want:   false,
+		},
+		{
+			name:   "seeded origin is not explicit",
+			origin: NullableDBVarchar{String: SystemSettingOriginSeeded, Valid: true},
+			want:   false,
+		},
+		{
+			name:   "explicit origin is explicit",
+			origin: NullableDBVarchar{String: SystemSettingOriginExplicit, Valid: true},
+			want:   true,
+		},
+		{
+			name:   "unexpected origin value is not explicit (fail-safe)",
+			origin: NullableDBVarchar{String: "some-future-value", Valid: true},
+			want:   false,
+		},
+		{
+			name:   "empty-string origin is not explicit (Oracle binds '' as NULL)",
+			origin: NullableDBVarchar{String: "", Valid: true},
+			want:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := &SystemSetting{Origin: tt.origin}
+			if got := s.IsExplicit(); got != tt.want {
+				t.Errorf("IsExplicit() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+// TestSystemSetting_OriginJSONRoundTrip guards against Origin regressing to
+// json:"-". The Redis cache tier (SettingsService.setInRedisCache /
+// getFromRedisCache) round-trips a SystemSetting through json.Marshal /
+// json.Unmarshal; "-" would silently drop Origin on every Redis cache hit,
+// making every seeded row look explicit after a warm read (oracle-db-admin
+// review, #794). This is orthogonal to the wire API, which never marshals
+// models.SystemSetting directly — modelToAPISystemSetting builds the API
+// type field by field and never copies Origin across.
+func TestSystemSetting_OriginJSONRoundTrip(t *testing.T) {
+	original := SystemSetting{
+		SettingKey:  "session.timeout_minutes",
+		Value:       "60",
+		SettingType: SystemSettingTypeInt,
+		Origin:      NullableDBVarchar{String: SystemSettingOriginSeeded, Valid: true},
+	}
+
+	data, err := json.Marshal(original)
+	if err != nil {
+		t.Fatalf("Marshal: %v", err)
+	}
+
+	var roundTripped SystemSetting
+	if err := json.Unmarshal(data, &roundTripped); err != nil {
+		t.Fatalf("Unmarshal: %v", err)
+	}
+
+	if !roundTripped.Origin.Valid || roundTripped.Origin.String != SystemSettingOriginSeeded {
+		t.Errorf("Origin after round-trip = (valid=%v, %q), want (valid=true, %q)",
+			roundTripped.Origin.Valid, roundTripped.Origin.String, SystemSettingOriginSeeded)
+	}
+	if roundTripped.IsExplicit() {
+		t.Error("round-tripped seeded row reports IsExplicit() == true; Origin was lost in JSON marshaling")
+	}
+}

api/runtime_config_reader_adapter.go

@@ -36,15 +36,17 @@ func NewRuntimeConfigReaderAdapter(settings SettingsServiceInterface) *RuntimeCo
 //   - exists=true, err!=nil: DB row present but unusable → caller MUST
 //     fail-closed to prevent open-redirect against a corrupt row.
 //
-// The read is database-only (GetDatabaseString): for #419 runtime keys the
-// DB row must win over the YAML config, which acts purely as a first-run
-// fallback. Reading through GetString would invert that — its
-// env/config-file priority silently shadows the DB row whenever the YAML
-// also carries the key (#767).
+// The read goes through GetResolvedString, which applies the converged
+// precedence rule (#794). It used to be database-only, because for #419
+// runtime keys the DB row must win over the YAML — and reading through
+// GetString would have inverted that, since its env/config-file priority
+// shadows the DB row whenever the YAML also carries the key (#767).
+// GetResolvedString keeps that property for any row an operator actually
+// set, while letting an explicit config value beat a merely-seeded default.
 //
-// SEM@08e19a77d4d2c499f116e1a1ee3c875c06407335: fetch the OAuth client callback allowlist from DB settings only; fail-closed on corrupt row (reads DB)
+// SEM@2daf3be663df9da54323f16d115f12d78d435c3f: fetch the OAuth client callback allowlist by resolved precedence; fail-closed on corrupt row (reads DB)
 func (a *RuntimeConfigReaderAdapter) GetClientCallbackAllowList(ctx context.Context) ([]string, bool, error) {
-	raw, exists, err := a.settings.GetDatabaseString(ctx, "auth.oauth.client_callback_allowlist")
+	raw, exists, err := a.settings.GetResolvedString(ctx, "auth.oauth.client_callback_allowlist")
 	if err != nil {
 		// A TRANSIENT DB fault (ADB idle-session kill, pool exhaustion,
 		// listener blip — ORA-02396/03113/12537 class) must NOT fail closed:
@@ -74,14 +76,19 @@ func (a *RuntimeConfigReaderAdapter) GetClientCallbackAllowList(ctx context.Cont
 	return list, true, nil
 }
 
-// IsSAMLEnabled reads features.saml_enabled, DB row first (#419/#767).
-// When no DB row exists it falls back to GetString (env/config file) —
+// IsSAMLEnabled reads features.saml_enabled by the converged precedence
+// rule (#794). When no layer supplies a value it falls back to GetString —
 // unlike the other two readers, the auth handler has no YAML fallback of
 // its own once a RuntimeConfigReader is wired. A read error or garbage
 // value returns false (fail-closed).
-// SEM@08e19a77d4d2c499f116e1a1ee3c875c06407335: check whether SAML login is enabled, DB-first with config fallback; fail-closed on error (reads DB)
+//
+// Note that TMI_SAML_ENABLED separately gates SAML manager construction at
+// startup (auth/service.go), so the env var stays load-bearing regardless of
+// what this returns, and enabling SAML via the database alone still needs a
+// restart. That asymmetry is called out in #794 and is not fixed here.
+// SEM@2daf3be663df9da54323f16d115f12d78d435c3f: check whether SAML login is enabled by resolved precedence; fail-closed on error (reads DB)
 func (a *RuntimeConfigReaderAdapter) IsSAMLEnabled(ctx context.Context) bool {
-	raw, exists, err := a.settings.GetDatabaseString(ctx, "features.saml_enabled")
+	raw, exists, err := a.settings.GetResolvedString(ctx, "features.saml_enabled")
 	if err != nil {
 		// Transient DB faults fall through to the config-layer fallback below
 		// (same reasoning as GetClientCallbackAllowList): an ADB blip must not
@@ -110,12 +117,20 @@ func (a *RuntimeConfigReaderAdapter) IsSAMLEnabled(ctx context.Context) bool {
 	return v
 }
 
-// GetOAuthCallbackURL reads auth.oauth_callback_url from the database only
-// (#419/#767). An empty string is returned on error/missing row; the
-// caller falls back to the YAML snapshot in h.config.OAuth.CallbackURL.
-// SEM@08e19a77d4d2c499f116e1a1ee3c875c06407335: fetch the OAuth callback URL from DB settings only; return empty string on error (reads DB)
+// GetOAuthCallbackURL reads auth.oauth_callback_url by the converged
+// precedence rule (#794). An empty string is returned on error or when no
+// layer supplies a value; the caller falls back to the YAML snapshot in
+// h.config.OAuth.CallbackURL.
+//
+// This is the key that broke production on 2026-08-20. RDS carried a
+// registry-seeded default of http://localhost:8080/oauth2/callback, the
+// database-only read let it outrank a correctly-set TMI_OAUTH_CALLBACK_URL,
+// and every provider rejected the resulting redirect_uri. Under the
+// converged rule a seeded row loses to an explicit env value, so that
+// specific failure cannot recur.
+// SEM@2daf3be663df9da54323f16d115f12d78d435c3f: fetch the OAuth callback URL by resolved precedence; return empty string on error (reads DB)
 func (a *RuntimeConfigReaderAdapter) GetOAuthCallbackURL(ctx context.Context) string {
-	raw, exists, err := a.settings.GetDatabaseString(ctx, "auth.oauth_callback_url")
+	raw, exists, err := a.settings.GetResolvedString(ctx, "auth.oauth_callback_url")
 	if err != nil {
 		slogging.Get().Warn("RuntimeConfigReader: failed to read auth.oauth_callback_url: %v", err)
 		return ""
@@ -131,5 +146,31 @@ func (a *RuntimeConfigReaderAdapter) GetOAuthCallbackURL(ctx context.Context) st
 	return raw
 }
 
+// IsEveryoneAReviewer reads auth.everyone_is_a_reviewer by the converged
+// precedence rule (#794). Fail-closed: any read error, missing value, or
+// unparseable value returns false.
+//
+// This key previously had no runtime reader at all — its only consumer read
+// the config struct field directly, so the database row was display-only
+// drift and editing it did nothing. It is the fifth and last of the keys
+// #794 catalogued as following inconsistent precedence.
+// SEM@2daf3be663df9da54323f16d115f12d78d435c3f: check whether all users are auto-added as security reviewers; fail-closed on error (reads DB)
+func (a *RuntimeConfigReaderAdapter) IsEveryoneAReviewer(ctx context.Context) bool {
+	raw, exists, err := a.settings.GetResolvedString(ctx, "auth.everyone_is_a_reviewer")
+	if err != nil {
+		slogging.Get().Warn("RuntimeConfigReader: failed to read auth.everyone_is_a_reviewer: %v", err)
+		return false
+	}
+	if !exists || raw == "" {
+		return false
+	}
+	v, parseErr := strconv.ParseBool(raw)
+	if parseErr != nil {
+		slogging.Get().Warn("RuntimeConfigReader: auth.everyone_is_a_reviewer is not a valid bool (%q): %v", raw, parseErr)
+		return false
+	}
+	return v
+}
+
 // Compile-time check that the adapter satisfies the auth interface.
 var _ auth.RuntimeConfigReader = (*RuntimeConfigReaderAdapter)(nil)