Conversation
…h __ Configuration was resolved by two overlapping mechanisms: ~40 `Default` impls that read the environment directly, and an `EnvOverride` trait applied after deserialization. Because every struct carries `#[serde(default)]`, the first of those baked the environment into the *defaults* layer, so an environment variable lost to the config file for any section the trait did not cover, and `pdf`, `pdf_handle_cache`, `scheduler`, `email` and `koreader_api` were not covered. Both are replaced by a single figment chain: struct defaults, then the config file, then a sibling `.local` overlay, then the environment. Environment variables now separate nesting levels with `__` and words within a key with `_`, so `rate_limit.anonymous_rps` is `CODEX_RATE_LIMIT__ANONYMOUS_RPS`. The old flat spelling was ambiguous: nothing in `CODEX_RATE_LIMIT_ANONYMOUS_RPS` distinguishes the section `rate_limit` from a section `rate`, which is why the previous implementation needed a hand-written override per key. A `<stem>.local.<ext>` file next to the config is merged on top when present, so secrets and per-host overrides no longer require editing the committed config. TOML is accepted alongside YAML, chosen by file extension. Rooting the data directories no longer guesses. It used to decide a path was "unset" by comparing it against the literal string `data/<subdir>`, which silently rewrote the path of anyone who wrote that value deliberately. The loader now builds its layer chain twice, once over the defaults and once without them, and asks the second whether any layer actually supplied the key. Paths keep their `String` type, so no call site changed. Two latent bugs go with it. The environment can now introduce a config subtree the file never mentioned, so switching to `db_type: postgres` by environment alone produces a working section instead of leaving `postgres` as `None` for `display_database_config` to unwrap. And `postgresql`, `http/protobuf` and `http/json` are accepted again as enum spellings, which the old override handled and plain serde did not. Adds tests for the precedence chain, the overlay, path rooting, and a registry-driven test that sets every scalar setting through its `__` name and checks it lands, replacing the per-key tests deleted with the override module. `Config::default()` is now pinned by a snapshot captured before the environment reads were lifted out, so a mistyped literal fails loudly rather than quietly changing a default. `serial_test` is no longer needed; the environment tests run under `figment::Jail`. The startup validator still carries the advisory wording it shipped with, which now reads backwards; switching it to reject the old names is separate. BREAKING CHANGE: every `CODEX_*` configuration environment variable is renamed to use `__` between nesting levels. The old flat names are no longer read. Run `codex config check` on the previous release to get the full list of replacements for the variables you set.
Deploying codex with
|
| Latest commit: |
058ce01
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00e425c6.codex-asm.pages.dev |
| Branch Preview URL: | https://v2-0.codex-asm.pages.dev |
…allowed Environment variables are strings, but the config tree holds bools, lists and maps. The deleted override layer hand-parsed each of those at its point of use, so plain serde would now reject input that used to work. These deserializers keep the accepted spellings: - bools take `true`/`false`, `1`/`0`, `yes`/`no` and `on`/`off`, in any case, which matters because figment parses `true` into a bool and `1` into a number before serde sees either - string lists take a YAML sequence or a comma-separated string, so `CODEX_API__CORS_ORIGINS=https://a,https://b` works - OTLP headers take a mapping or `k1=v1,k2=v2`, the form the whole header set is conventionally supplied in - OIDC role mapping takes one role per variable with a comma-separated group list, matching how it was set before - an empty optional string means absent, because blanking a variable and unsetting it are the same gesture in most deployment tooling and `env_string_opt` filtered empties Unrecognized input is now an error. The override layer used `if let Ok(v) = s.parse()`, so `CODEX_KOMGA_API_ENABLED=ture` quietly meant `false` and nothing said so. A typo in a deployment's environment should stop the process rather than flip a setting behind the operator's back. `OidcProviderConfig` gains struct-level defaults so a provider can again be introduced entirely from the environment, which the override layer supported and a missing-field parse error had broken. A provider that exists but is unusable is better reported by config validation, which names the actual problem, than by a parse error about `display_name`.
…lacks `db_type: postgres` with no `database.postgres` section parsed cleanly and then panicked, because `display_database_config` unwrapped the missing section while logging startup state. The same held for `sqlite`. Both are now caught by a `Config::validate` that runs at the end of loading, so the process stops with a message naming the missing section and how to supply it instead of aborting inside a log statement. The logging function no longer unwraps at all. Reaching it without a section would mean a caller bypassed the loader, and logging is not the place to end the process. Validation also rejects an OIDC provider with no `issuer_url` or `client_id`, but only while OIDC is enabled, so a half-written provider left in a disabled block does not stop the server. This is the check that makes struct-level defaults on the provider safe: an entry can be created from the environment alone, and if it is unusable the error names the field that is missing rather than blaming `display_name`.
Seven operator-tunable settings were read with `std::env::var` at their point of use rather than through `Config`, so they appeared in no config file, in no resolved-config dump, and in nothing that validates spelling. They are now ordinary keys: CODEX_COOKIE_SECURE -> auth.cookie_secure CODEX_DISABLE_WORKERS -> task.run_in_process (inverted) CODEX_IMAGE_DECODE_CONCURRENCY -> images.decode_concurrency CODEX_PLUGIN_ALLOWED_COMMANDS -> plugins.allowed_commands CODEX_SKIP_MIGRATIONS -> database.run_migrations (inverted) CODEX_MIGRATION_WAIT_TIMEOUT -> database.migration_wait_timeout_secs CODEX_MIGRATION_WAIT_INTERVAL -> database.migration_wait_interval_secs Both inverted keys default to the previous behaviour: workers run in process and migrations are applied unless something says otherwise. Three variables stay in the environment. The encryption key is read deep in codex-utils and codex-db, which have no configuration in scope, so moving it means threading config through those crates. The two `codex copy` database URLs are per-invocation arguments already backed by CLI flags. That leaves an allowlist of three, small enough that forgetting to update it is unlikely. Because two of these flip sense, a renamed variable cannot be derived by re-spelling the old name, and a deployment that kept `CODEX_DISABLE_WORKERS` and heard nothing would start task workers in a pod meant to serve web traffic only. A `REMOVED_VARS` table records each replacement and marks the inverted pair, so the validator can say what to set instead rather than guessing. The plugin allowlist keeps its process-wide `OnceLock`, now installed from config during startup instead of read from the environment on first use. `is_command_allowed` is reached from request handlers with no configuration in scope, and threading it down every call path would touch the whole plugin surface for a value fixed at boot. A second initialization is refused and logged rather than silently replacing the allowlist. Two `init_database` tests drove the old variables through `unsafe` env mutation and were caught by this change; they now set config fields and no longer need serialized execution. A third only checked that "1" was truthy, which the value deserializers already cover.
…hat is no longer read The environment audit shipped as advice, on the reasoning that the old names were still correct and 2.0 was not out. Both halves have now flipped: the nested spelling is what the loader reads, and a variable in the old form does nothing. Leaving that as a log line means a deployment carries on with default rate limits, or the wrong port, and the only evidence is one line in a startup log nobody reads. So `Config::load` now fails on it. Every offending variable appears in one message with its replacement, because an operator with a dozen of them should make one pass rather than discover them one restart at a time. Two severities, not one. A name that resolves to a real setting is fatal: the operator asked for something specific and is not getting it. A name that resolves to nothing stays a warning, because another tool may legitimately use the `CODEX_` prefix and guessing wrong should not take a deployment down. `Config::resolve` is the same resolution without the check, so `codex config check` can list every problem instead of stopping at the first. Its exit code now matches what the server would do: non-zero when a fatal finding is present, and `--strict` additionally fails on warnings. The `NotYetValid` finding is gone, since a nested name is now simply read, and the startup advisory line goes with it: enforcement says the same thing louder and at the right time.
…ig init` Starting with no config file wrote `Config::default()` to disk. That was already a poor introduction, since serializing the live defaults produces a file with no comments explaining any of it, but it was also a way to leak secrets: the defaults used to be read from the environment, so a container started once with a database password in its environment wrote that password into the generated YAML in plaintext, where it stayed. Loading now leaves no trace. A missing file is not an error and is not created, because defaults plus the environment are already a complete configuration, which is what a container with nothing mounted relies on. `codex config init` writes a commented starter template when an operator asks for one, and refuses to overwrite without `--force` so a stray invocation cannot clobber a tuned production config. The template documents the precedence chain, the `.local` overlay, and the `__` environment convention, and leaves optional sections commented so that nothing in it counts as an explicit setting. That last part matters for the paths: an uncommented `thumbnail_dir` would pin the path and stop it following `data_dir`. Two tests cover it: loading creates neither the file nor its parent directory, and a password in the environment never reaches disk. The template itself is parsed in a test, since a starter that does not load would hand the operator a broken server, and checked for a mention of every config section so a new one cannot go undocumented. `load_config` returns just the config now; the "did I create it?" flag it used to return has nothing left to report.
…s on odd passwords `ssl_mode` appeared in two YAML examples, an environment example and a Security Best Practices entry recommending `verify-full` in production, while no such field existed. Operators following that advice believed they had certificate verification and had none. The 1.44 docs corrected the claim and pointed at the libpq variables, which do work; this adds the setting the docs had been describing all along. database.postgres.ssl_mode disable|allow|prefer|require|verify-ca|verify-full database.postgres.ssl_root_cert database.postgres.ssl_client_cert database.postgres.ssl_client_key Leaving `ssl_mode` unset appends nothing to the connection URL, so the driver resolves it as before and `PGSSLMODE` keeps working for deployments configured that way. Setting it here wins. The default is deliberately unchanged. The driver resolves to `prefer`, which encrypts opportunistically, accepts any certificate and falls back to plaintext, all silently. Tightening that to `require` would break every deployment whose PostgreSQL has no TLS configured, which for a self-hosted product is most of them, and it would break them at the same upgrade that already renames every environment variable. The docs say plainly what `prefer` does and does not protect against. Building the URL properly also fixes a bug that had nothing to do with TLS: the connection string was assembled by interpolating the username and password directly, so a password containing `@`, `/`, `:`, `?` or `#` produced a URL pointing somewhere else entirely. Credentials are now percent-encoded, with a narrower escape set for query values so a certificate path stays readable. Two tests that asserted `ssl_mode` was not a real key have been inverted, which is the point of the change.
… scheme Adds an upgrade guide with the full rename table, generated from the configuration schema rather than written by hand so it cannot be partial. It covers the two settings whose sense inverted, the values that are now rejected instead of silently discarded, and the fact that startup no longer writes a config file. Sweeps every old variable name out of the documentation, the compose files, the Kubernetes manifests and the shipped example configs. The two inverted settings were excluded from the automated pass and rewritten by hand, since renaming `CODEX_DISABLE_WORKERS: "true"` to `CODEX_TASK__RUN_IN_PROCESS: "true"` would have silently reversed what those examples do. Verified by setting every variable the documentation mentions and running `codex config check`: nothing is flagged except the example `client_secret_env` target, which is correct, since that names an arbitrary variable rather than a setting. Two gaps surfaced while checking, both fixed: - A list field rejected an all-digit value, because figment parses one as a number before serde sees it, so `CODEX_API__CORS_ORIGINS=8080` failed. - `database.sqlite.pragmas` is a map the schema advertises as settable but had no lenient deserializer, so it could not actually be set from the environment. It now takes `k=v,k=v` like the other maps.
…ent parsers
figment already parses typed values out of an environment string: `true`,
`[a, b]`, `{k=v, k=v}`, numbers. The custom deserializers existed only to also
accept the pre-2.0 spellings the old override layer hand-parsed, so an entire
module was carrying compatibility with a scheme this release already breaks.
It is deleted.
What operators write changes with it:
boolean 1 / 0 / yes / no / on / off -> true / false
list a,b,c -> [a, b, c]
map k1=v1,k2=v2 -> {k1=v1, k2=v2}
Quote any entry containing a space or a comma, since those delimit entries.
Numbers and plain strings are unaffected.
One convenience survives, because losing it would be a regression rather than
a simplification: an empty value still means "unset", since blanking a
variable is how a setting gets switched off in compose files and Kubernetes
manifests. That no longer needs a deserializer on every optional field, only a
single normalization step after resolution.
`codex config check` now reports a bad value instead of dying on it. Parsing
failure used to abort the whole command, so one mistyped value hid every
misspelled variable beside it, and those are exactly the pair worth seeing
together. The resolved-config dump is suppressed when resolution failed, since
printing the defaults would misrepresent what the server would actually run
with. Serde stops at the first type error, so one is reported at a time, and
the output says so rather than leaving that to be inferred from a second
failure.
Also repairs the Docker build, which this branch had broken. The starter
template was pulled in with `include_str!` from `config/`, a directory
`.dockerignore` excludes deliberately so operator configs are never baked into
an image, so the crate failed to compile inside the image while every local
build succeeded. The template is program data emitted by `codex config init`
rather than an example to copy, so it moves inside the crate and the include
becomes crate-relative. Verified by building with `config/` absent, which is
what the image build sees.
The migration guide gains a before-and-after table for value syntax, and the
OIDC, rate-limit and OTLP examples are rewritten in bracket and brace form.
…onfig/ Two problems with files under config/, both introduced by this branch. `.gitignore` had no rule for `<stem>.local.<ext>`. That is the file the new overlay support tells operators to put secrets in, so the one file guaranteed to hold a credential was also the one guaranteed to be committed. Local overlays are now ignored. The starter template moves back to config/, where it sits beside the other examples and can simply be copied, which is the point of shipping an example at all. It was moved into the crate to unbreak the Docker build, but that fixed the symptom and gave up the property worth having. The build broke because `.dockerignore` excludes config/ so that operator configs are never baked into an image, and the template is pulled in with `include_str!` at compile time. That exclusion has been in place for months and was harmless until this branch added the repository's first compile-time dependency on a file, which happened to point into the excluded directory. Nothing local could catch it: every local build runs in a full checkout, and only the Docker build sees a trimmed context. The narrow fix is an explicit exception for that one file, so the template reaches the build context and every other config file stays out. Re-including a path inside an excluded directory is the sort of thing that silently does nothing, so it was verified with a throwaway image that copies the context and lists it: config/ arrives containing the template and nothing else.
The template was covered by two tests that together proved very little. One parsed it, but almost every key in a starter template is commented out, so it exercised a handful of lines. The other checked that section names appeared as substrings, which a mention in prose would satisfy. That left the case that actually bites unguarded: a typo or a stale name in a commented key. Those are precisely the lines an operator uncomments, and a key that does not exist is silently ignored by the loader, so the setting has no effect and nothing explains why. The new test un-comments as it walks, builds dotted paths from indentation, and requires each one to be a real setting. Reading a heavily commented file this way means telling YAML from prose, and the discriminator is that a section carries no value of its own while a sentence always does. That keeps `exception: a list in the overlay ...` out of the results while still catching a section that has been renamed or removed. It checks one direction deliberately. A setting missing from the template is harmless, since the reference documents the whole surface and `config check` prints the resolved config, and requiring every key would turn a starter into a reference and put a documentation chore on every new field. Writing it exposed a bug in the key registry: map containers were filed as exact paths even when the path already contained a wildcard, so `auth.oidc.providers.*.role_mapping` sat in the set the environment classifier normalizes over to suggest replacements, where it could only have produced a suggestion with a literal `*` in the variable name. Paths now go through one routine that files each by its own shape. Also documents `ssl_client_cert` and `ssl_client_key`, which were added with the other PostgreSQL TLS settings but never reached the template. Commented out like the rest of the postgres block: an explicitly written value is not the same as an absent one here, and shipping `ssl_mode` uncommented would demand certificate verification from every copy of the file.
AshDevFr
changed the title
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
1 participant
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 2.0 configuration migration. Every task in the plan is done except the version bump, which is the maintainer's release step.
What changed
Configuration was resolved by two overlapping mechanisms: ~40
Defaultimpls reading the environment directly, and anEnvOverridetrait applied after deserialization. Because every struct carries#[serde(default)], the first baked the environment into the defaults layer, so an environment variable lost to the config file for any section the trait did not cover, and five sections were not covered.Both are replaced by one figment chain:
env_override.rs(1,766 lines) is deleted.Breaking changes
Environment variable names. Nesting levels now separate with
__, words within a key with_. The old spelling was ambiguous: nothing inCODEX_RATE_LIMIT_ANONYMOUS_RPSdistinguishes sectionrate_limitfrom sectionrate, which is why the old code needed a hand-written rule per key, and why several documented variables never had one and did nothing.Codex refuses to start on an old name, listing every offender with its replacement. Ignoring them would mean running with default rate limits or the wrong port and no indication anything is wrong.
Seven settings moved from ad-hoc
env::varreads into real config keys. Two invert their sense, so they cannot be derived by re-spelling and are handled by an explicit table:CODEX_COOKIE_SECURECODEX_AUTH__COOKIE_SECURECODEX_DISABLE_WORKERSCODEX_TASK__RUN_IN_PROCESSCODEX_IMAGE_DECODE_CONCURRENCYCODEX_IMAGES__DECODE_CONCURRENCYCODEX_MIGRATION_WAIT_INTERVALCODEX_DATABASE__MIGRATION_WAIT_INTERVAL_SECSCODEX_MIGRATION_WAIT_TIMEOUTCODEX_DATABASE__MIGRATION_WAIT_TIMEOUT_SECSCODEX_PLUGIN_ALLOWED_COMMANDSCODEX_PLUGINS__ALLOWED_COMMANDSCODEX_SKIP_MIGRATIONSCODEX_DATABASE__RUN_MIGRATIONSEnvironment values are typed. figment parses the value itself, so the shapes the old layer hand-parsed are no longer accepted:
Quote any entry containing a space or a comma, since those delimit entries. Numbers and plain strings are unaffected. An empty value still means "unset", because blanking a variable is how a setting gets switched off in a compose file or a manifest.
This is the one break that a correctly renamed deployment can still trip over, so it is worth reading before the rename table is applied.
A bad value stops the server. The old layer discarded what it could not parse, so
CODEX_KOMGA_API_ENABLED=turequietly meantfalse.Startup no longer writes a config file. It used to serialize
Config::default()when the file was missing, which produced an uncommented dump and, since those defaults were read from the environment, could capture a database password in plaintext.codex config initwrites a commented starter instead.New
codex.local.yamloverlay, merged over the base file for secrets and per-host tweaks.database.postgres.ssl_modeand friends).ssl_modehas been documented since the project's first week without ever existing, including a Security Best Practices entry recommendingverify-full; operators following it had no verification. The default is deliberately unchanged, since tightening it would break every TLS-less deployment at the same upgrade that renames everything.codex config init, andconfig checkpromoted to enforcing. It also type-checks: a value that fails to parse is reported as a finding beside any misspelled names rather than aborting the command, since those are exactly the pair worth seeing together. The resolved-config dump is suppressed when resolution failed, so it never prints defaults the server would not actually run with.Bugs fixed along the way
db_type: postgreswith nopostgressection parsed fine and then panicked indisplay_database_config. Now rejected at load with a message naming the section.db_typeplus a host by environment alone silently did nothing.data/<subdir>, silently rewriting the path of anyone who wrote that value deliberately.@,/,:,?or#pointed the connection somewhere else.Testing
Config::default()is pinned by a snapshot captured before the environment reads were lifted out, verified identical to that baseline and identical again with all 82 documented variables poisoned. A registry-driven test sets every scalar setting through its__name and checks it lands, replacing the per-key tests deleted with the override module and asserting its own coverage so it cannot silently degrade.Every key named in the starter template is checked against the same registry, commented lines included, since a typo in a commented key is invisible until an operator uncomments it and the loader ignores the result in silence.
Full suite green throughout,
cargo clippy --workspace --all-targets -- -D warningsclean.Docs
New upgrade guide at
docs/docs/migration/v2-config.md, rename table generated from the schema rather than hand-written. Every old name swept from docs, compose files, manifests and example configs; verified by setting every documented variable and runningcodex config check.Not in this PR
The version bump.
make release-prepareregeneratesCHANGELOG.md, which is the maintainer's release-time step, and the version belongs onmainafter merge rather than on every commit of the branch. After merging:make release-prepare VERSION=2.0.0.