feat(config): make one registry the authoritative declaration of every setting - #812
Merged
Conversation
…operational settings Approved design for the config model redesign. Two categories with exactly one delivery path each: bootstrap from file/env (precedence env > file > default), everything else database-only. One authoritative registry replaces the four declaration sites that let #793 and the rate_limit.* 404 drift apart. Adds a template mechanism whose per-environment files carry secret *references* resolved at import through internal/secrets, never secret material, so the templates for project-owned environments can be tracked. Closes #793 as obsolete: the export/import mismatch it describes cannot exist once DefaultSystemSettings() is a projection of the registry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…ing registry Ten TDD tasks taking internal/config from four parallel declaration sites to one registry, with no runtime behavior change. The bijection and total-coverage tests are the specification for the bulk ports: they enumerate exactly which keys are missing. Includes the transitional ratchet, whose golden list may only shrink and whose emptiness is the Phase E completion gate, and fixes #809 structurally by making DefaultSystemSettings() a projection of the registry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…ansition marker Adds ValidateSettingDefs, the enforcement point that makes an unclassified or malformed SettingDef impossible. Carries forward every rule ValidateClassifications enforces (including two the task brief's reference implementation omitted: operational settings must carry a Delivery, and SharedInvariant requires the monolith as a Consumer), adds the YAMLPath/EnvVar/Get category-legality rules for the new config-path fields, and adds Seeded => CategoryOperational.
…jection test Declares the remaining sections not covered by the three sibling declaration files (websocket.*, webhooks.*, operator.*, secrets.*, alerting.*, ssrf.*, the five DB-only client-config keys, and the derived session.timeout_minutes key) in a new setting_defs_misc.go, then rebinds settingDefs in setting_def.go to concatDefs(serverSettingDefs, authSettingDefs, contentSettingDefs, miscSettingDefs) — 159 defs total. Relaxes two setting_def_validation.go rules per controller ruling: a transitional operational setting now only requires Get != nil (YAMLPath and EnvVar are each independently optional — administrators has no env: tag and session.timeout_minutes has no YAMLPath), and Default is no longer required for a Class.Secret operational setting. Also exempts Type "string" from the Default requirement: Default is carried as a plain string, so there's no way to distinguish a genuinely empty compiled-in default (auth.cookie.domain, unconfigured Timmy/content-source fields, the fail-closed ssrf.* allowlists) from an omitted one, while every other type's zero value still serializes to a non-empty string. Each rule change has a new covering test. Corrects two sibling declarations found during integration: administrators and every ssrf.* key get YAMLPath: "" (their underlying Config fields have a yaml tag but no env tag, so a non-empty YAMLPath makes them "extra" against the bijection test's env-tag-only struct walk); content_token_encryption_key is now Secret: true; and the administrators / client_callback_allowlist Get closures return "[]" for an empty slice instead of json.Marshal's "null". Adds setting_defs_bijection_test.go, which walks every env-tagged Config struct field via reflection and asserts it's bijective with the registry's declared YAMLPaths — the acceptance test for this phase of the config registry consolidation (see .superpowers/sdd/2026-08-22-config-registry-phase-a). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…audit Mutability Fix round 1 on the SettingDef registry wiring, addressing four spec-review findings: 1. Reverts a prior deviation that set YAMLPath: "" on administrators and all 10 ssrf.*.allowlist/schemes keys to dodge a bijection-test defect — those yaml paths are real. Re-keys setting_defs_bijection_test.go's registry bijection on EnvVar instead of YAMLPath, compared as sets (so TMI_JWT_EXPIRATION_SECONDS legitimately backing both auth.jwt.expiration_seconds and the derived session.timeout_minutes no longer collides), and adds TestSettingDefs_YAMLPathsAreReal, which walks Config for every yaml-tagged path regardless of env tag and asserts every declared YAMLPath is real — closing the gap that let a false-empty YAMLPath through undetected. 2. Adds two setting_def_validation.go rules: a Seeded setting must declare a Default (only Seeded keys are ever written into system_settings at DB init, so Default is load-bearing there regardless of the separate Type=="string"/Secret exemptions on the general operational-Default rule), and a Seeded setting must not be Secret (a secret must never be seeded into the database with a compiled-in value). 3. Adds the previously-missing setting_defs_operational_test.go from the merged brief's Task 4, with one ruled amendment: Default is asserted non-empty only when Seeded, since several sampled keys (operator.name, ssrf.webhook.allowlist) have a genuinely empty compiled-in default. 4. Audits Mutability on every operational SettingDef by reading its actual consumer, rather than leaving the operationalClass/classificationFor default of Hot everywhere. Adds a withMutability() override helper and applies it to 89 entries that are captured once at server construction with no live re-read (server.disable_rate_limiting/ratelimit_public_rpm/ require_if_match, observability.*, most auth.* JWT/cookie/step-up settings, features.saml_enabled — which gates SAML manager construction at startup — administrators, websocket.inactivity_timeout_seconds, webhooks.allow_http_targets, all ssrf.*, session.timeout_minutes, and nearly all Timmy/content_extractors/content_sources/content_oauth keys). Settings with a confirmed live per-request reader (operator.*, upload.max_file_size_mb, websocket.max_participants, ui.default_theme, extraction.async_enabled, timmy.enabled, features.webhooks_enabled/ websocket_enabled, auth.everyone_is_a_reviewer, auth.oauth_callback_url, auth.oauth.client_callback_allowlist) stay Hot. make lint: 0 issues. make test-unit: 2719 passed, 0 failed, 10 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…from the registry
The seed list was a hand-kept parallel list, so rate_limit.requests_per_minute
and rate_limit.requests_per_hour were seeded into system_settings while having
no classification entry. Unclassified resolves to VisibilityInternal, so
GET/DELETE /admin/settings/{key} returned 404 for keys the LIST endpoint
displayed.
DefaultSystemSettings() now projects config.SeedableOperationalDefs() instead
of maintaining its own copy of the 9 seeded rows, so the two lists cannot
drift again.
Fixes #809
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
Round 0 (62e82fc) declared rate_limit.requests_per_minute and rate_limit.requests_per_hour in the SettingDef registry, but that registry is not what api/config_handlers.go's GET/DELETE /admin/settings/{key} consults — it calls config.ClassificationFor(key), which resolves only through classification_registry.go's exactClassifications table. Without an entry there, both keys still resolved to the zero ConfigClass (VisibilityInternal) and still 404'd, unchanged from before round 0. Add both keys to exactClassifications as operationalClass(VisibilityAdminOnly, false), the same shape used for the five sibling DB-only client-config keys. Their SettingDef.Class now builds via classificationFor(key) instead of an inline operationalClass() call, so there is one source of truth instead of two hand-kept ones. Add TestClassificationFor_SeededKeysAreNotInternal, which asserts ClassificationFor(key).Visibility != VisibilityInternal for every key in SeedableOperationalDefs() — the test that would have caught round 0's gap, since a SettingDef-level assertion alone cannot see the endpoint's actual resolution path. Verified directly: ClassificationFor("rate_limit.requests_per_minute") now returns Category=operational Visibility=admin-only, not the zero value. Fixes #809 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
Adds the enforcement point for the config-registry refactor: a coverage test that would have caught #809 (rate_limit.* seeded but unclassified) and the list/get 404 inconsistency it caused. TestRegistry_CoversEveryReachableKey cross-checks the SettingDef registry against every key reachable via the Config struct's env tags, the classification registry, and the database seed list. Also closes two gaps found during review: a golden-list test pinning SeedableOperationalDefs() to the exact 9 keys written into system_settings at database init (nothing previously stopped a new Seeded:true flag from silently adding a seed row to every new database), and a test asserting only session.timeout_minutes may have a non-nil Get with an empty YAMLPath (closing the hole that let a real config path be silently blanked out). All three guardrails were verified to actually fail under the mutation they exist to catch, then reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
TestRegistry_CoversEveryReachableKey's reachable set is built from Config struct env tags, which yield raw struct yaml paths — not settings keys. Three defs deliberately declare a settings Key that differs from their struct path (e.g. auth.saml.enabled is declared as features.saml_enabled), so reusing ExpectedMigratableKeysSkipped() to paper over the mismatch was wrong: that allowlist has two more entries beyond the three renames, and both of those (content_token_encryption_key, database.oracle_wallet_location) already have real SettingDefs — reusing the list silently excluded them from ever being checked by this test. Fix: build the declared set from both SettingDef.Key and (when non-empty) SettingDef.YAMLPath, and drop the ExpectedMigratableKeysSkipped() reference entirely. TestSettingDefs_YAMLPathsAreReal already guarantees a non-empty YAMLPath names a real Config field, so this can't be gamed with a fabricated path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
GetMigratableSettings() is now a projection over AllSettingDefs() instead of 15 hand-written per-section builders. Emission stays conditional (unlike the original plan's "make it unconditional", which the controller's Ruling 16 rejected: DefaultOperationalSettings() feeds both SeedDefaults and #794's origin backfill, and unconditional emission would seed a new row per previously-omitted operational key on every fresh database, and reclassify existing rows' seeded-vs-explicit origin). Conditional emission is now declared per-setting via a new SettingDef.OmitWhenEmpty field (45 defs), with a type-aware emptiness test ("" for string, "0" for the two int keys that used ">0" as their old guard, "[]"/"null" for the three json/slice keys). server.tls_cert_file and server.tls_key_file are special-cased directly in GetMigratableSettings instead, since their old guard was server.tls_enabled — a different field than the one being emitted, which OmitWhenEmpty cannot express. DefaultOperationalSettings() output is pinned byte-for-byte against a captured pre-refactor baseline (70 keys) in the new migratable_settings_equivalence_test.go. Deleted the 15 obsolete per-section builders; kept and trimmed the two provider-wrapper functions (getMigratableOAuthSettings, getMigratableContentOAuthSettings) down to just their per-provider loops, now that their static keys are registry-projected; the SAML equivalents and all three per-provider settings builders are unchanged. Known follow-up (left for Task 8, which owns config-example.yml/ config-reference.md regeneration): three Bootstrap-category keys (content_token_encryption_key, database.oracle_wallet_location, server.trusted_proxies) now appear in GetMigratableSettings() output because their SettingDefs have real Get accessors, even though ExpectedMigratableKeysSkipped() documents them as excluded. This only affects the two doc-generation tests (config-example.yml/config-reference.md are now stale) — DefaultOperationalSettings() is unaffected since all three are Bootstrap, not Operational. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…green Fix round 1 on the GetMigratableSettings registry projection (71b4a22): - GetMigratableSettings() now skips any def whose Key appears in ExpectedMigratableKeysSkipped(), matched on Key (not YAMLPath) so the three rename entries (auth.oauth.callback_url, auth.saml.enabled, content_extractors.async_enabled) stay correctly emitted under their renamed keys, while content_token_encryption_key and database.oracle_wallet_location — both flagged there as intentionally excluded, and both Secret in the first case — are no longer emitted. - server.trusted_proxies is also excluded (documented inline): its SettingDef has a real Get and a real Bootstrap Class, but emitting it requires resolving two out-of-scope problems first — classificationFor(key) doesn't recognize it (would need either a registry-Class-direct design Ruling 15 deferred to Phase E, or a one-off exactClassifications entry), and GenerateExampleConfig's JSON-value coercion mishandles a nil-slice value ("null" is written as a literal YAML string, not decoded), breaking auth's TestYAMLConfigsPassOAuthValidation. Both belong to Task 8, which owns rewriting these generators. - Reverted Class assignment back to classificationFor(key) (matching the pre-registry implementation exactly) rather than SettingDef.Class directly: the latter would surface ~40 keys' deliberate Mutability overrides (hot -> static) into config-reference.md for the first time — a real behavior change Ruling 15 explicitly deferred to Phase E. - Fixed one unrelated one-character description mismatch (observability.sampling_rate: hyphen -> en dash) so the regenerated config-example.yml/config-reference.md are byte-identical to committed content (verified; not committed, since the only diff was the generation timestamp). make test-unit: 2735 passed, 0 failed. make lint: 0 issues. DefaultOperationalSettings() still byte-identical to its pinned baseline — none of the three newly-skipped/excluded keys are CategoryOperational. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…ed_proxies
GenerateExampleConfig dumped a JSON-typed setting's already-marshaled string
through to YAML verbatim, so a nil-slice default ("null") became the literal
YAML string "null" instead of an empty list. Decode json-typed values into
native Go values before marshaling, normalizing "null"/"[]"/"" to an empty
list, matching the convention this file already uses for empty collections.
Enable emission of server.trusted_proxies (TMI_TRUSTED_PROXIES), which bound
an env var but appeared in no generated documentation because no builder
ever emitted it — an omission blocked on the coercion bug above, not a
deliberate exclusion.
Add two generator guardrail tests: every env var actually emitted by
GetMigratableSettings must be named in the generated config reference (it
doubles as the TMI_* allowlist), and no operational, non-Transitional
(database-only) setting may appear in the config-file template. The first
test exposed that the operational settings table never rendered an Env var
column at all, hiding every Transitional operational setting's override
variable; add that column to close the gap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
The substring check against GenerateExampleConfig's nested YAML output could never fail: a dotted key like "timmy.enabled" never appears as literal text once setNested + yaml.Marshal render it as nested mappings. Parse the generated document back into a tree and check reconstructed dotted paths instead, and prove the fix catches a real leak (deliberate drift experiment, reverted) before landing it. Also drop a stale SEM@ marker on coerceJSONSettingValue — its anchor commit predated the function it labeled, and this task chain deliberately adds no SEM markers (generated later, once the code is in HEAD). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
…d by config/env Adds a golden-list test over AllSettingDefs() Transitional keys that can only shrink as Phase E removes config/env delivery paths for operational settings; adding a key fails the test by name. Also fixes TestGetMigratableSettings_ExplicitTracksEnvAndFile, which asserted inside an if s.Key == "server.port" branch with nothing proving the branch ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
Markers were suppressed across the config-registry-phase-a task chain because sem blame cannot anchor a file not yet in HEAD. Now that the registry code is committed, add/refresh SEM@<sha> intent markers for the SettingDef registry, its validation, the seed/example/reference projections, and the migratable-settings listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
mergeSettingsWithConfig's database-only branch lists every seeded row
from GET /admin/settings with no visibility filter, while GET/DELETE
/admin/settings/{key} both 404 on VisibilityInternal. A seeded def with
internal visibility would recreate #809: a key the list endpoint shows
but the single-item endpoints refuse. All nine currently-seeded defs are
public or admin-only, so this was latent, not live; ValidateSettingDefs
now makes it structurally impossible per the Oracle review note on #809's
class not being closed.
Re-anchor after adding the Seeded/VisibilityInternal rule.
…shipped The committed plan still described emission becoming unconditional and never mentioned the Seeded flag, OmitWhenEmpty, or that Tasks 3/4 were merged. Add an "Execution deviations" section after Global Constraints recording the four real departures, and point the three Task 7 "unconditional" passages at it. Also amend the doc comment on TestRegistry_CoversEveryReachableKey: the database-seed-list leg is tautological now that SeedableOperationalDefs() is derived from the registry it's being checked against, so it can no longer catch what the comment claimed. State which legs actually bite and name the three mechanisms (TestSeedableOperationalDefs_MatchesGoldenList, TestClassificationFor_SeededKeysAreNotInternal, the four Seeded validation rules) that now hold the seeded-set invariant instead. Comment/markdown only, no logic changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase A of the config model redesign. Spec:
docs/superpowers/specs/2026-08-22-config-model-redesign-design.md.Plan:
docs/superpowers/plans/2026-08-22-config-registry-phase-a.md.Why
A setting could be declared in up to four places — the
Configstruct'syaml/env tags,
GetMigratableSettings(),classification_registry.go, andDefaultSystemSettings()— with nothing structural keeping them in agreement.Two production bugs came from that drift, and the guardrail that should have
caught them structurally could not:
ValidateClassificationsvalidates whateverslice it is handed, in practice
GetMigratableSettings(), which by constructioncontains only keys that already have a struct field — and which emitted
conditionally, so the validated set changed with runtime values.
What
161 settings now have exactly one declaration each, as
SettingDefvalues ininternal/config/setting_defs_*.go.DefaultSystemSettings()andGetMigratableSettings()became projections of it; the hand-written per-sectionbuilders are deleted.
Guardrails, each with a demonstrated failing case:
Configstruct or theclassification registry has a declaration.
every declared
YAMLPathis a real yaml path.that still have a config/env path. It may only shrink; at zero, the cutover is
done.
ClassificationFor()— the path the endpoint actually resolves through.Seededimplies operational, requires a non-empty default, forbids secret,and forbids internal visibility.
Fixes #809
rate_limit.requests_per_minute/_per_hourwere seeded intosystem_settingsand listed by
GET /admin/settings, but 404'd onGET/DELETEbecause they hadno classification and unclassified resolves to
VisibilityInternal.Worth knowing before closing it: both keys are dead. Nothing reads either;
real rate limiting runs off
server.disable_rate_limiting/server.ratelimit_public_rpm. This PR classifies them, which is thebehavior-preserving fix. Deleting them or wiring them up are both behavior
changes — see the discussion on the issue.
Verification
make lint0 ·make build-serverclean ·make test-unit2743 / 0 failed·
make test-integration85 / 0 failed ·oracle-db-adminAPPROVED WITHNOTES.
The Oracle review verified byte-identity empirically rather than trusting the
in-repo pins — worktree at the base commit, identical dumper test in both trees,
field-by-field diff of runtime output — confirming the 9 seeded rows and the 70
DefaultOperationalSettings()entries are unchanged. That set matters beyondseeding:
internal/dbschema/system_setting_origin_backfill.gouses it to decideseeded-vs-explicit, which drives env-vs-database precedence on live databases.
What this found
rate_limit.*keysplus
server.trusted_proxies,database.oracle_wallet_location, andcontent_token_encryption_key, the last an encryption key defaulting tonon-secret.
Mutabilitywas decorative; the audit set it deliberately per entry against consuming code.
features.saml_enabledis the traced case:NewServiceonly builds the SAMLmanager when the boot flag is true, so the DB-backed reader gates handlers that
a nil manager already sank.
SeedDefaults' empty-value skip covers only one of itstwo loops, so a seeded setting with an empty default reaches
Create(), andOracle binds
''as NULL into aNOT NULLCLOB. TheSeededrequires-defaultrule prevents it.
over-exempted two keys, a test asserting over declarations instead of the
resolution path,
strings.Containsagainst nested YAML, a loop-guardedassertion with no found-flag, and a coverage leg that derives from the registry
it checks.
Not in scope
Phases B–E: the template tool with
!secretreferences, the per-environmentcutover, and pointing
classificationFor()itself at the registry. Operationalsettings still have config/env paths — that is what the ratchet measures.
Execution deviated from the plan in five recorded ways (emission stayed
conditional, chiefly); see Execution deviations at the top of the plan
document.
Follow-ups filed
#808 · #810 · #811, and a dead-setting comment on #809.
Before merge
Check AWS/RDS for a stray
server.trusted_proxiesrow:SELECT setting_key FROM system_settings WHERE setting_key = 'server.trusted_proxies';Its
exactClassificationsentry meansPUT /admin/settings/server.trusted_proxiesnow 409s instead of silently writing a bootstrap-key row — an improvement, but a
deployment that exercised the old path has a row that lists yet 404s. k3s is
verified clean; RDS was not checked here because it needs credentials.
🤖 Generated with Claude Code
https://claude.ai/code/session_011gCPZA9iBjL4ugHHXuHj6t