feat: migrate to new constraint-based loading strategy #8251

bolinfest · 2025-12-18T08:09:48Z

This is a significant change to how layers of configuration are applied. In particular, the ConfigLayerStack now has two important fields:

layers: Vec<ConfigLayerEntry>
requirements: ConfigRequirements

We merge TomlValues across the layers, but they are subject to ConfigRequirements before creating a Config.

How I would review this PR:

start with codex-rs/app-server-protocol/src/protocol/v2.rs and note the new variants added to the ConfigLayerSource enum: LegacyManagedConfigTomlFromFile and LegacyManagedConfigTomlFromMdm
note that ConfigLayerSource now has a precedence() method and implements PartialOrd
codex-rs/core/src/config_loader/layer_io.rs is responsible for loading "admin" preferences from /etc/codex/managed_config.toml and MDM. Because /etc/codex/managed_config.toml is now deprecated in favor of /etc/codex/requirements.toml and /etc/codex/config.toml, we now include some extra information on the LoadedConfigLayers returned in layer_io.rs.
codex-rs/core/src/config_loader/mod.rs has major changes to load_config_layers_state(), which is what produces ConfigLayerStack. The docstring has the new specification and describes the various layers that will be loaded and the precedence order.
- It uses the information from LoaderOverrides "twice," both in the spirit of legacy support:
  - We use one instances to derive an instance of ConfigRequirements. Currently, the only field in managed_config.toml that contributes to ConfigRequirements is approval_policy. This PR introduces Constrained::allow_only() to support this.
  - We use a clone of LoaderOverrides to derive ConfigLayerSource::LegacyManagedConfigTomlFromFile and ConfigLayerSource::LegacyManagedConfigTomlFromMdm layers, as appropriate. As before, this ends up being a "best effort" at enterprise controls, but is enforcement is not guaranteed like it is for ConfigRequirements.
- Now we only create a "user" layer if $CODEX_HOME/config.toml exists. (Previously, a user layer was always created for ConfigLayerStack.)
- Similarly, we only add a "session flags" layer if there are CLI overrides.
config_loader/state.rs contains the updated implementation for ConfigLayerStack. Note the public API is largely the same as before, but the implementation is quite different. We leverage the fact that ConfigLayerSource is now PartialOrd to ensure layers are in the correct order.
A Config constructed via ConfigBuilder.build() will use load_config_layers_state() to create the ConfigLayerStack and use the associated ConfigRequirements when constructing the Config object.
That said, a Config constructed via Config::load_from_base_config_with_overrides() does not yet use ConfigBuilder, so it creates a ConfigRequirements::default() instead of loading a proper ConfigRequirements. I will fix this in a subsequent PR.

Then the following files are mostly test changes:

codex-rs/app-server/tests/suite/v2/config_rpc.rs
codex-rs/core/src/config/service.rs
codex-rs/core/src/config_loader/tests.rs

Again, because we do not always include "user" and "session flags" layers when the contents are empty, ConfigLayerStack sometimes has fewer layers than before (and the precedence order changed slightly), which is the main reason integration tests changed.

jif-oai · 2025-12-18T12:37:12Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

chatgpt-codex-connector · 2025-12-18T12:40:55Z

codex-rs/core/src/config_loader/mod.rs

+    match tokio::fs::read_to_string(&user_file).await {
+        Ok(contents) => {
+            let user_config: TomlValue =
+                toml::from_str(&contents).unwrap_or(TomlValue::Table(toml::map::Map::new()));


Propagate parse errors for user config

When $CODEX_HOME/config.toml exists but is malformed, the loader now parses with toml::from_str(...).unwrap_or(...), which silently falls back to an empty table. That bypasses the error propagation promised in the comment and means a user’s invalid config is ignored rather than surfaced, so their settings disappear without any feedback. This is a regression from the previous behavior where invalid TOML resulted in an error, and it can hide configuration mistakes in production.

Useful? React with 👍 / 👎.

Addressed locally: fix available when I update this PR.

chatgpt-codex-connector · 2025-12-18T12:40:55Z

codex-rs/core/src/config/service.rs

+        let user_layer = layers.get_user_layer().ok_or_else(|| {
+            // TODO(mbolin): Support creating the user layer if it does not exist.
+            ConfigServiceError::write(
+                ConfigWriteErrorCode::UserLayerNotFound,
+                "user layer not found and we do not support creating it yet",
+            )


Allow writes when no user config exists

With the new layering, load_config_layers_state no longer creates a user layer when config.toml is missing. apply_edits now treats a missing user layer as an error and returns UserLayerNotFound, which means config writes cannot create the initial file. This breaks first‑time configuration updates (e.g., UI settings on a fresh install) that previously succeeded by writing an empty base layer.

Useful? React with 👍 / 👎.

Addressed locally: fix available when I update this PR.

gt-oai

thanks for doing this and for the detailed 'how to review' guide!

gt-oai · 2025-12-18T12:37:51Z

codex-rs/app-server-protocol/src/protocol/v2.rs

+impl ConfigLayerSource {
+    pub fn precedence(&self) -> i16 {
+        match self {
+            ConfigLayerSource::Mdm { .. } => 0,


~~Perhaps start at 10, not 0, to allow for room below?~~ you can go negative, ignore me

gt-oai · 2025-12-18T12:40:35Z

codex-rs/app-server-protocol/src/protocol/v2.rs

+}
+
+impl ConfigLayerSource {
+    pub fn precedence(&self) -> i16 {


Suggest adding a small comment here about what lower / higher precedence means in practice. I'd probably explain it multiple ways that mean the same thing such that readers can pick the most understandable one.

"Higher precedence configs are applied last."
"Higher precedence configs overwrite values from lower precendence configs". etc.

gt-oai · 2025-12-18T12:53:39Z

codex-rs/core/src/config_loader/config_requirements.rs

+/// Base config deserialized from /etc/codex/requirements.toml or MDM.
+#[derive(Deserialize, Debug, Clone, Default, PartialEq)]
+pub struct ConfigRequirementsToml {
+    pub approval_policy: Option<Vec<AskForApproval>>,


approval_policy or allowed_approval_policies? I worry that reusing the same field name here will have the following undesirable effects:

Users may assume, because they have the same field names, that requirements.toml has the same shape as config.toml, or that all fields from config.toml are supported. I think it's confusing that the fields have the same name but different types.

It may set expectations that there should be a bijection between fields in config.toml and fields in requirements.toml. But I foresee a future where there are things you could set in requirements.toml that don't map cleanly to individual values in config.toml.

Name collisions make it harder to grep, for us and for Codex.

In summary: (1) users will expect things in config.toml to be in requirements.toml (2) users will expect things in requirements.toml to be in config.toml

Yep, happy to change!

gt-oai · 2025-12-18T13:00:11Z

codex-rs/core/src/config_loader/config_requirements.rs

+
+    fn try_from(toml: ConfigRequirementsToml) -> Result<Self, Self::Error> {
+        let approval_policy: Constrained<AskForApproval> = match toml.approval_policy {
+            Some(policies) => Constrained::allow_values(AskForApproval::default(), policies)?,


Does this imply that, if the default is not in the allowed policies, we just fail?

It looks like we aren't using this right now. But we should be wary of this when we start reading from requirements.toml. I expect this to look more like "pick a sensible allowed default for the user" rather than "crash" -- might be worth adding a todo here so codex can pick it up when we implement requirements.toml reading.

This is a good point. I'll change that so:

If AskForApproval::default() is in policies, make it the default (since that would be Codex's default even if requirements.toml did not exist).

Else, choose the first item in policies.

gt-oai · 2025-12-18T13:04:15Z

codex-rs/core/src/config_loader/mod.rs

+    // a config layer on top of everything else. Note users can still
+    // override these values on a per-turn basis in the TUI and VS Code.


I don't think "users can still override..." is strictly true because you're pulling out the approval_policy from the managed config and constraining it with Constrained::allow_only, so I expect that to actually take effect.

Clarified the comment. Currently, this is true for all fields except approval_policy.

Over in `config_loader/macos.rs`, we were doing this complicated `mod` thing to expose one version of `load_managed_admin_config_layer()` for Mac: https://github.com/openai/codex/blob/580c59aa9af61cb4bffb5b204bd16a5dcc4bc911/codex-rs/core/src/config_loader/macos.rs#L4-L5 While exposing a trivial implementation for non-Mac: https://github.com/openai/codex/blob/580c59aa9af61cb4bffb5b204bd16a5dcc4bc911/codex-rs/core/src/config_loader/macos.rs#L110-L117 That was being used like this: https://github.com/openai/codex/blob/580c59aa9af61cb4bffb5b204bd16a5dcc4bc911/codex-rs/core/src/config_loader/layer_io.rs#L47-L48 This PR simplifies that callsite in `layer_io.rs` to just be: ```rust #[cfg(not(target_os = "macos"))] let managed_preferences = None; ``` And updates `config_loader/mod.rs` so we only pull in `macos.rs` on Mac: ```rust #[cfg(target_os = "macos")] mod macos; ``` This simplifies `macos.rs` considerably, though it looks like a big change because everything gets unindented and reformatted because we can drop the whole `mod native` thing now. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/8248). * #8251 * #8249 * __->__ #8248

This is a significant change to how layers of configuration are applied. In particular, the `ConfigLayerStack` now has two important fields: - `layers: Vec<ConfigLayerEntry>` - `requirements: ConfigRequirements` We merge `TomlValue`s across the layers, but they are subject to `ConfigRequirements` before creating a `Config`.

bolinfest · 2025-12-18T16:56:45Z

@gt-oai I believe all comments have been addressed (including the ones from codexbot) if you want to take another look!

bolinfest changed the base branch from main to pr8249 December 18, 2025 08:09

This was referenced Dec 18, 2025

chore: prefer AsRef<Path> to &Path #8249

Merged

chore: simplify loading of Mac-specific logic in config_loader #8248

Merged

feat: migrate to new constraint-based loading strategy #8237

Closed

bolinfest force-pushed the pr8251 branch from b434970 to 892ba01 Compare December 18, 2025 08:13

bolinfest requested review from gt-oai, gverma-openai and jif-oai December 18, 2025 08:41

chatgpt-codex-connector bot reviewed Dec 18, 2025

View reviewed changes

gt-oai approved these changes Dec 18, 2025

View reviewed changes

bolinfest force-pushed the pr8251 branch from 892ba01 to f57c354 Compare December 18, 2025 15:36

bolinfest force-pushed the pr8249 branch 2 times, most recently from a271ed5 to d5a6b56 Compare December 18, 2025 15:49

Base automatically changed from pr8249 to main December 18, 2025 16:50

bolinfest force-pushed the pr8251 branch from f57c354 to c073963 Compare December 18, 2025 16:56

bolinfest merged commit b903285 into main Dec 18, 2025
50 of 52 checks passed

bolinfest deleted the pr8251 branch December 18, 2025 18:06

github-actions bot locked and limited conversation to collaborators Dec 18, 2025

		// a config layer on top of everything else. Note users can still
		// override these values on a per-turn basis in the TUI and VS Code.

feat: migrate to new constraint-based loading strategy #8251

feat: migrate to new constraint-based loading strategy #8251

Conversation

bolinfest commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jif-oai commented Dec 18, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gt-oai left a comment

Choose a reason for hiding this comment

Uh oh!

gt-oai Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bolinfest commented Dec 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bolinfest commented Dec 18, 2025 •

edited

Loading

gt-oai Dec 18, 2025 •

edited

Loading