mirror of
https://github.com/openai/codex.git
synced 2026-04-25 23:24:55 +00:00
feat: migrate to new constraint-based loading strategy (#8251)
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`. 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.
This commit is contained in:
@@ -66,13 +66,24 @@ async fn returns_empty_when_all_layers_missing() {
|
||||
let layers = load_config_layers_state(tmp.path(), &[] as &[(String, TomlValue)], overrides)
|
||||
.await
|
||||
.expect("load layers");
|
||||
let base_table = layers.user.config.as_table().expect("base table expected");
|
||||
assert!(
|
||||
layers.get_user_layer().is_none(),
|
||||
"no user layer when CODEX_HOME/config.toml does not exist"
|
||||
);
|
||||
|
||||
let binding = layers.effective_config();
|
||||
let base_table = binding.as_table().expect("base table expected");
|
||||
assert!(
|
||||
base_table.is_empty(),
|
||||
"expected empty base layer when configs missing"
|
||||
);
|
||||
assert!(
|
||||
layers.system.is_none(),
|
||||
let num_system_layers = layers
|
||||
.layers_high_to_low()
|
||||
.iter()
|
||||
.filter(|layer| matches!(layer.name, super::ConfigLayerSource::System { .. }))
|
||||
.count();
|
||||
assert_eq!(
|
||||
num_system_layers, 0,
|
||||
"managed config layer should be absent when file missing"
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user