fix(config): stop struct defaults shadowing database settings; configure Timmy durably - #621
Merged
Merged
Conversation
The DB-backed settings service was unreachable for every operational key. Found in production: timmy.enabled was true in the settings table while the server answered "Timmy AI assistant is not enabled" on every Timmy endpoint. GetMigratableSettings emits *every* setting, including ones nobody configured — those carry the struct default. getConfigSetting returned whatever it found, and cmd/server/main.go wires that config layer ahead of the database. So the default false for timmy.enabled shadowed the database row, and since every operational key has a default, none of them could ever be set from the database. #415 makes the database the source of truth for operational config; in practice it was the one source that could not win. Source could not distinguish the two cases: it reports "config" both for a value written in the YAML file and for a value nobody ever set. So this adds Explicit, stamped centrally in GetMigratableSettings from two signals already available — Source == "environment", and a new KeysInFile() that records the dotted keys the YAML actually named (the sibling of the existing OperationalKeysInFile, which already walks the same tree). getConfigSetting now yields to the database for operational settings that are not Explicit. Bootstrap settings (database URL, JWT secret, listen port) always resolve from config — they are never database-backed. Deliberately NOT a blanket "database always wins for operational". That would have been closer to #415's letter and would have broken both live deployments: on AWS the tmi-server-config ConfigMap arrives via envFrom, so every key in it is an environment variable that must keep winning; and config-development.yml sets server.disable_rate_limiting=true while the dev database holds false, so dev would have silently started rate-limiting — which also breaks CATS fuzzing. Keeping the Explicit escape hatch preserves every value an operator actually configured and changes behaviour only for keys that were pure defaults, which is exactly the broken case. The api-layer MigratableSetting had to carry Explicit and Class too; the adapter was dropping both on conversion. TestSettingsService_ConfigPriority's stubs now set Explicit: true. They stand for values an operator supplied, which is that test's premise — its intent is unchanged, it just predates the distinction. Verified the new guard fails against the old logic (TestGetConfigSetting_ DefaultDoesNotShadowDatabase) rather than passing vacuously. lint 0 · 2428 unit tests Refs #415 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc
Timmy was disabled on every deployment: no TMI_TIMMY_* value existed in Terraform, the manifests, or either live cluster, so the server ran DefaultTimmyConfig — Enabled false, every provider empty — and answered 404 on all /chat/sessions and /admin/timmy paths. This makes it configured in the manifests and Terraform, so it survives the next deploy rather than living only as a live patch. Verified working end to end on k3s-rp first: a real session streamed snapshot -> entity scan -> vector index -> ready, and gpt-5.5 answered a live question. Values are environment variables, not database settings. That was forced by the companion fix in this branch: the config layer only defers to the database for values nobody configured (#415), so a settings row alone could not switch Timmy on — proven the hard way, with timmy.enabled=true in the table and the server still reporting Timmy disabled. The two credentials are deliberately NOT in Terraform. They live in an out-of-band `tmi-timmy` Secret created by the new scripts/set-timmy-secret.sh, following the existing tmi-embedding precedent: Terraform-managed secrets land in remote state, and the provider key is operator-rotated. The script takes the key only as a file, never argv or an env var, and shreds it afterwards. It grew a --context flag because the embedding one hardcodes EKS and the local cluster needs the same secret. Two values are load-bearing and were established by measurement, not assumption: - text_embedding_model MUST equal TMI_EMBEDDING_MODEL on tmi-chunk-embed. The worker reads that env var directly (cmd/chunkembed/embedder.go MustEnv), not the stamped envelope, so nothing keeps the two in sync. The worker embeds documents at ingest and the server embeds the query; vectors of different dimension score 0 against each other and retrieval returns nothing, with no error anywhere. Both clusters and the component manifest move to text-embedding-3-large together. - embedding_dimension is 3072 because that is what text-embedding-3-large actually returned from a live probe — the repo's examples show 3072 and 768 for other models and pin nothing for this one. It must be > 0 or EmbeddingProfile.Validate rejects every job envelope the monolith stamps. The dev manifest marks both secretKeyRefs optional, so a cluster without the secret gets a 503 from Timmy rather than a crashlooping server. NOT APPLIED to AWS by this commit. `terraform plan` shows 4 changes: this ConfigMap, plus pre-existing drift that would upgrade the coredns, kube-proxy and vpc-cni EKS addons. Applying the ConfigMap alone needs `-target=module.kubernetes.kubernetes_config_map_v1.tmi`, or the addon upgrades ride along with it. lint 0 · terraform validate clean · overlay renders both secret refs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc
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.
The problem
The DB-backed settings service was unreachable for every operational key. Found in production:
timmy.enabledwastruein the settings table while the server answered "Timmy AI assistant is not enabled" on every Timmy endpoint.GetMigratableSettings()emits every setting, including ones nobody configured — those carry the struct default.getConfigSettingreturned whatever it found, andcmd/server/main.go:750wires that config layer ahead of the database. So the defaultfalseshadowed the database row, and since every operational key has a default, none of them could ever be set from the database. #415 makes the database the source of truth for operational config; in practice it was the one source that could not win.The fix
Sourcecould not distinguish the two cases — it reports"config"both for a value written in the YAML and for a value nobody ever set. So this addsExplicit, stamped centrally from two signals already available:Source == "environment", and a newKeysInFile()recording the dotted keys the YAML actually named (sibling of the existingOperationalKeysInFile, walking the same tree).getConfigSettingnow yields to the database for operational settings that are notExplicit. Bootstrap settings (database URL, JWT secret, listen port) always resolve from config — they are never database-backed.Deliberately NOT a blanket "database always wins for operational". That would be closer to #415's letter and would have broken both live deployments:
tmi-server-configConfigMap arrives viaenvFrom, so every key in it is an environment variable that must keep winning.config-development.ymlsetsserver.disable_rate_limiting=truewhile the dev database holdsfalse— dev would have silently started rate-limiting, which also breaks CATS fuzzing.Keeping the
Explicitescape hatch preserves every value an operator actually configured and changes behaviour only for keys that were pure defaults, which is exactly the broken case.Timmy configuration (second commit)
Timmy was disabled on every deployment — no
TMI_TIMMY_*value existed anywhere, so the server ranDefaultTimmyConfig(disabled, providers empty) and 404'd all Timmy paths. Now configured in Terraform and the manifests so it survives the next deploy. Verified working end to end on k3s-rp: a real session streamed snapshot → entity scan → vector index → ready, andgpt-5.5answered a live question.Two values are load-bearing and were established by measurement, not assumption:
text_embedding_modelMUST equalTMI_EMBEDDING_MODELontmi-chunk-embed. The worker reads that env var directly (cmd/chunkembed/embedder.goMustEnv), not the stamped envelope, so nothing keeps the two in sync. Vectors of different dimension score 0 against each other and retrieval returns nothing, with no error anywhere. Both clusters and the component manifest move totext-embedding-3-largetogether.embedding_dimensionis 3072 because that is whattext-embedding-3-largeactually returned from a live probe. It must be > 0 orEmbeddingProfile.Validaterejects every job envelope.Credentials are deliberately not in Terraform — they live in an out-of-band
tmi-timmySecret created by the newscripts/set-timmy-secret.sh, following the existingtmi-embeddingprecedent (Terraform-managed secrets land in remote state).Deploy note
terraform planshows 4 changes: this ConfigMap, plus pre-existing drift that would upgrade thecoredns,kube-proxyandvpc-cniEKS addons. Applying the ConfigMap alone needs-target=module.kubernetes.kubernetes_config_map_v1.tmi.lint 0 · 2428 unit tests · terraform validate clean
🤖 Generated with Claude Code
https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc