Conversation
9475d37 to
d4d74a4
Compare
bbec96c to
999cbdb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 999cbdb177
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jif-oai
left a comment
There was a problem hiding this comment.
I think this is doing too much plumbing by hand for a config-loading mode
b909ad9 to
7726a0d
Compare
|
@jif-oai I addressed the config-loading plumbing concern in the latest pushes. Strict mode now flows through ConfigBuilder::strict_config(...) into ConfigLoadOptions, and the command crates use CliConfigOverrides::prepend_root_overrides(...) so strict config is applied before config loading instead of each caller hand-plumbing LoaderOverrides-style state. I also rebased again onto current main; the PR head is now 7726a0d. |
jif-oai
left a comment
There was a problem hiding this comment.
TBH I'm still not convinced by all this wiring. It makes it super easy to forget one entry-point and have some blind spots
Maybe the proper solution is a cleaning of the config system though which would make it out of scope of this PR
I can approve to un-block but I think we need to re-design something here tbh
| let config = ConfigBuilder::default() | ||
| .cli_overrides(cli_kv_overrides) | ||
| .harness_overrides(overrides) | ||
| .strict_config(root_config_overrides.strict_config) |
There was a problem hiding this comment.
This only makes features list strict. features enable/disable --strict-config still bypass config loading..
| let strict_config = config_overrides.strict_config; | ||
|
|
||
| match subcommand { | ||
| MarketplaceSubcommand::Add(args) => run_add(args).await?, |
There was a problem hiding this comment.
OOC, why don't we have strict for add/remove?
Would be good to add a small comment in the code to justify
| Ok(()) | ||
| } | ||
|
|
||
| async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs) -> Result<()> { |
There was a problem hiding this comment.
What about run_remove for example?
4d1ad0b to
fe3a730
Compare
b9e8ed6 to
97f3820
Compare
|
@jif-oai I addressed your above comments by changing things so that subcommands have to declare explicit support for |
jif-oai
left a comment
There was a problem hiding this comment.
Ok for me but one point to note:
It does not hard fail the app-server. Is this expected? I just see the following but the app-server still starts and work
2026-05-13T08:55:41.283598Z ERROR codex_app_server: Invalid configuration; using defaults. com.openai.codex:config_toml_base64:11:10: unknown configuration field `features.general_analytics`
Good catch! I wasn't accounting for this fallback login in codex/codex-rs/app-server/src/lib.rs Lines 503 to 513 in 2edae8d I added the following to the top of that block: Then I also discovered that And then I added the following integration tests: |
Why
Codex intentionally ignores unknown
config.tomlfields by default so older and newer config files keep working across versions. That leniency also makes typo detection hard because misspelled or misplaced keys disappear silently.This change adds an opt-in strict config mode so users and tooling can fail fast on unrecognized config fields without changing the default permissive behavior.
This feature is possible because
serde_ignoredexposes the exact signal Codex needs: it lets Codex run ordinary Serde deserialization while recording fields Serde would otherwise ignore. That avoids requiring#[serde(deny_unknown_fields)]across every config type and keeps strict validation opt-in around the existing config model.What Changed
Added strict config validation
serde_ignored-based validation forConfigTomlincodex-rs/config/src/strict_config.rs.serde_ignoredwithserde_path_to_errorso strict mode preserves typed config error paths while also collecting fields Serde would otherwise ignore.[features]keys, including keys that would otherwise be accepted byFeaturesToml's flattened boolean map.Kept parsing single-pass per source
TomlValuefor that source.-c/--configoverride layers with the same base-directory context used for normal relative-path resolution, so unknown override keys are still reported when another override contains a relative path.Scoped
--strict-configto config-heavy entry points--strict-configon the main config-loading entry points where it is most useful:codexcodex resumecodex forkcodex execcodex reviewcodex mcp-servercodex app-serverwhen running the server itselfcodex-app-serverbinarycodex-execbinary--strict-configearly with targeted errors instead of accepting it everywhere through shared CLI plumbing.codex app-serversubcommands such asproxy,daemon, andgenerate-*are intentionally excluded from the first rollout.ReviewCommandwrapper incodex-rs/cliinstead of extending sharedReviewArgs, so--strict-configstays on the outer config-loading command surface and does not become part of the reusable review payload used bycodex exec review.Coverage
[features]keys, typed-error precedence, source-location reporting, and non-file managed preference source names.--enable, invalid--disable, and unknown-coverrides still error when--strict-configis present, including compound-looking feature names such asmulti_agent_v2.subagent_usage_hint_text.codex app-server --strict-configand standalonecodex-app-server --strict-configexit with an error for unknown config fields instead of starting with fallback defaults.--strict-configwith explicit errors.Example Usage
Run Codex with strict config validation enabled:
Strict config mode is also available on the supported config-heavy subcommands:
For example, if
~/.codex/config.tomlcontains a typo in a key name:then
codex --strict-configreports the misspelled key instead of silently ignoring it. The path is shortened to~here for readability:Without
--strict-config, Codex keeps the existing permissive behavior and ignores the unknown key.Strict config mode also validates ad-hoc
-c/--configoverrides:Invalid feature toggles are rejected too, including values that look like nested config paths:
Unsupported commands reject the flag explicitly:
Verification
The
codex-clistrict_configtests cover invalid--enable, invalid--disable, the compoundmulti_agent_v2.subagent_usage_hint_textcase, unknown-coverrides, app-server strict startup failure throughcodex app-server, and rejection for unsupported commands such ascodex cloud,codex mcp,codex remote-control, andcodex app-server proxy.The config and config-loader tests cover unknown top-level fields, unknown nested fields, unknown
[features]keys, source-location reporting, non-file managed config sources, and-cvalidation for keys such asfeatures.foo.The app-server test suite covers standalone
codex-app-server --strict-configstartup failure for an unknown config field.Documentation
The Codex CLI docs on developers.openai.com/codex should mention
--strict-configas an opt-in validation mode for supported config-heavy entry points once this ships.