diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 66bc7647b..1d58cd1da 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -208,6 +208,7 @@ v2_enum_from_core!( } ); +// TODO(mbolin): Support in-repo layer. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(tag = "type", rename_all = "camelCase")] #[ts(tag = "type")] @@ -216,17 +217,63 @@ pub enum ConfigLayerSource { /// Managed preferences layer delivered by MDM (macOS only). #[serde(rename_all = "camelCase")] #[ts(rename_all = "camelCase")] - Mdm { domain: String, key: String }, + Mdm { + domain: String, + key: String, + }, + /// Managed config layer from a file (usually `managed_config.toml`). #[serde(rename_all = "camelCase")] #[ts(rename_all = "camelCase")] - System { file: AbsolutePathBuf }, - /// Session-layer overrides supplied via `-c`/`--config`. - SessionFlags, - /// User config layer from a file (usually `config.toml`). + System { + file: AbsolutePathBuf, + }, + + /// User config layer from $CODEX_HOME/config.toml. This layer is special + /// in that it is expected to be: + /// - writable by the user + /// - generally outside the workspace directory #[serde(rename_all = "camelCase")] #[ts(rename_all = "camelCase")] - User { file: AbsolutePathBuf }, + User { + file: AbsolutePathBuf, + }, + + /// Session-layer overrides supplied via `-c`/`--config`. + SessionFlags, + + /// `managed_config.toml` was designed to be a config that was loaded + /// as the last layer on top of everything else. This scheme did not quite + /// work out as intended, but we keep this variant as a "best effort" while + /// we phase out `managed_config.toml` in favor of `requirements.toml`. + LegacyManagedConfigTomlFromFile { + file: AbsolutePathBuf, + }, + + LegacyManagedConfigTomlFromMdm, +} + +impl ConfigLayerSource { + /// A settings from a layer with a higher precedence will override a setting + /// from a layer with a lower precedence. + pub fn precedence(&self) -> i16 { + match self { + ConfigLayerSource::Mdm { .. } => 0, + ConfigLayerSource::System { .. } => 10, + ConfigLayerSource::User { .. } => 20, + ConfigLayerSource::SessionFlags => 30, + ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } => 40, + ConfigLayerSource::LegacyManagedConfigTomlFromMdm => 50, + } + } +} + +/// Compares [ConfigLayerSource] by precedence, so `A < B` means settings from +/// layer `A` will be overridden by settings from layer `B`. +impl PartialOrd for ConfigLayerSource { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.precedence().cmp(&other.precedence())) + } } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema, TS)] @@ -344,7 +391,7 @@ pub struct ConfigWriteResponse { pub status: WriteStatus, pub version: String, /// Canonical path to the config file that was written. - pub file_path: String, + pub file_path: AbsolutePathBuf, pub overridden_metadata: Option, } @@ -357,6 +404,7 @@ pub enum ConfigWriteErrorCode { ConfigValidationError, ConfigPathNotFound, ConfigSchemaUnknownKey, + UserLayerNotFound, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index d26f36fa3..805601d59 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -73,9 +73,8 @@ sandbox_mode = "workspace-write" } ); let layers = layers.expect("layers present"); - assert_eq!(layers.len(), 2); - assert_eq!(layers[0].name, ConfigLayerSource::SessionFlags); - assert_eq!(layers[1].name, ConfigLayerSource::User { file: user_file }); + assert_eq!(layers.len(), 1); + assert_eq!(layers[0].name, ConfigLayerSource::User { file: user_file }); Ok(()) } @@ -137,9 +136,8 @@ view_image = false ); let layers = layers.expect("layers present"); - assert_eq!(layers.len(), 2); - assert_eq!(layers[0].name, ConfigLayerSource::SessionFlags); - assert_eq!(layers[1].name, ConfigLayerSource::User { file: user_file }); + assert_eq!(layers.len(), 1); + assert_eq!(layers[0].name, ConfigLayerSource::User { file: user_file }); Ok(()) } @@ -211,7 +209,7 @@ writable_roots = [{}] assert_eq!(config.model.as_deref(), Some("gpt-system")); assert_eq!( origins.get("model").expect("origin").name, - ConfigLayerSource::System { + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file.clone(), } ); @@ -219,7 +217,7 @@ writable_roots = [{}] assert_eq!(config.approval_policy, Some(AskForApproval::Never)); assert_eq!( origins.get("approval_policy").expect("origin").name, - ConfigLayerSource::System { + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file.clone(), } ); @@ -242,7 +240,7 @@ writable_roots = [{}] .get("sandbox_workspace_write.writable_roots.0") .expect("origin") .name, - ConfigLayerSource::System { + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file.clone(), } ); @@ -259,28 +257,28 @@ writable_roots = [{}] ); let layers = layers.expect("layers present"); - assert_eq!(layers.len(), 3); + assert_eq!(layers.len(), 2); assert_eq!( layers[0].name, - ConfigLayerSource::System { file: managed_file } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } ); - assert_eq!(layers[1].name, ConfigLayerSource::SessionFlags); - assert_eq!(layers[2].name, ConfigLayerSource::User { file: user_file }); + assert_eq!(layers[1].name, ConfigLayerSource::User { file: user_file }); Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn config_value_write_replaces_value() -> Result<()> { - let codex_home = TempDir::new()?; + let temp_dir = TempDir::new()?; + let codex_home = temp_dir.path().canonicalize()?; write_config( - &codex_home, + &temp_dir, r#" model = "gpt-old" "#, )?; - let mut mcp = McpProcess::new(codex_home.path()).await?; + let mut mcp = McpProcess::new(&codex_home).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let read_id = mcp @@ -311,13 +309,7 @@ model = "gpt-old" ) .await??; let write: ConfigWriteResponse = to_response(write_resp)?; - let expected_file_path = codex_home - .path() - .join("config.toml") - .canonicalize() - .unwrap() - .display() - .to_string(); + let expected_file_path = AbsolutePathBuf::resolve_path_against_base("config.toml", codex_home)?; assert_eq!(write.status, WriteStatus::Ok); assert_eq!(write.file_path, expected_file_path); @@ -380,16 +372,17 @@ model = "gpt-old" #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn config_batch_write_applies_multiple_edits() -> Result<()> { - let codex_home = TempDir::new()?; - write_config(&codex_home, "")?; + let tmp_dir = TempDir::new()?; + let codex_home = tmp_dir.path().canonicalize()?; + write_config(&tmp_dir, "")?; - let mut mcp = McpProcess::new(codex_home.path()).await?; + let mut mcp = McpProcess::new(&codex_home).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let writable_root = test_tmp_path_buf(); let batch_id = mcp .send_config_batch_write_request(ConfigBatchWriteParams { - file_path: Some(codex_home.path().join("config.toml").display().to_string()), + file_path: Some(codex_home.join("config.toml").display().to_string()), edits: vec![ ConfigEdit { key_path: "sandbox_mode".to_string(), @@ -415,13 +408,7 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> { .await??; let batch_write: ConfigWriteResponse = to_response(batch_resp)?; assert_eq!(batch_write.status, WriteStatus::Ok); - let expected_file_path = codex_home - .path() - .join("config.toml") - .canonicalize() - .unwrap() - .display() - .to_string(); + let expected_file_path = AbsolutePathBuf::resolve_path_against_base("config.toml", codex_home)?; assert_eq!(batch_write.file_path, expected_file_path); let read_id = mcp diff --git a/codex-rs/core/src/config/constraint.rs b/codex-rs/core/src/config/constraint.rs index f10df09dd..d126b84a8 100644 --- a/codex-rs/core/src/config/constraint.rs +++ b/codex-rs/core/src/config/constraint.rs @@ -19,6 +19,12 @@ impl ConstraintError { ), } } + + pub fn empty_field(field_name: impl Into) -> Self { + Self { + message: format!("field `{}` cannot be empty", field_name.into()), + } + } } pub type ConstraintResult = Result; @@ -57,6 +63,32 @@ impl Constrained { } } + pub fn allow_only(value: T) -> Self + where + T: PartialEq + Send + Sync + fmt::Debug + Clone + 'static, + { + #[expect(clippy::expect_used)] + Self::new(value.clone(), move |candidate| { + if *candidate == value { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + format!("{candidate:?}"), + format!("{value:?}"), + )) + } + }) + .expect("initial value should always be valid") + } + + /// Allow any value of T, using T's Default as the initial value. + pub fn allow_any_from_default() -> Self + where + T: Default, + { + Self::allow_any(T::default()) + } + pub fn allow_values(initial_value: T, allowed: Vec) -> ConstraintResult where T: PartialEq + Send + Sync + fmt::Debug + 'static, @@ -129,6 +161,12 @@ mod tests { assert_eq!(constrained.value(), -10); } + #[test] + fn constrained_allow_any_default_uses_default_value() { + let constrained = Constrained::::allow_any_from_default(); + assert_eq!(constrained.value(), 0); + } + #[test] fn constrained_new_rejects_invalid_initial_value() { let result = Constrained::new(0, |value| { diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 64f1ca5c1..438e441b5 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -12,6 +12,7 @@ use crate::config::types::ShellEnvironmentPolicy; use crate::config::types::ShellEnvironmentPolicyToml; use crate::config::types::Tui; use crate::config::types::UriBasedFileOpener; +use crate::config_loader::ConfigRequirements; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::features::Feature; @@ -356,7 +357,12 @@ impl ConfigBuilder { let config_toml: ConfigToml = merged_toml .try_into() .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; - Config::load_from_base_config_with_overrides(config_toml, harness_overrides, codex_home) + Config::load_config_with_requirements( + config_toml, + harness_overrides, + codex_home, + config_layer_stack.requirements().clone(), + ) } } @@ -390,20 +396,18 @@ impl Config { } } -/// DEPRECATED: Use [Config::load_with_cli_overrides()] instead because this -/// codepath is not guaranteed to honor [ConfigRequirements]. +/// DEPRECATED: Use [Config::load_with_cli_overrides()] instead because working +/// with [ConfigToml] directly means that [ConfigRequirements] have not been +/// applied yet, which risks failing to enforce required constraints. pub async fn load_config_as_toml_with_cli_overrides( codex_home: &Path, cli_overrides: Vec<(String, TomlValue)>, ) -> std::io::Result { - let root_value = load_resolved_config( - codex_home, - cli_overrides, - crate::config_loader::LoaderOverrides::default(), - ) - .await?; + let config_layer_stack = + load_config_layers_state(codex_home, &cli_overrides, LoaderOverrides::default()).await?; - let cfg = deserialize_config_toml_with_base(root_value, codex_home).map_err(|e| { + let merged_toml = config_layer_stack.effective_config(); + let cfg = deserialize_config_toml_with_base(merged_toml, codex_home).map_err(|e| { tracing::error!("Failed to deserialize overridden config: {e}"); e })?; @@ -411,15 +415,6 @@ pub async fn load_config_as_toml_with_cli_overrides( Ok(cfg) } -async fn load_resolved_config( - codex_home: &Path, - cli_overrides: Vec<(String, TomlValue)>, - overrides: crate::config_loader::LoaderOverrides, -) -> std::io::Result { - let layers = load_config_layers_state(codex_home, &cli_overrides, overrides).await?; - Ok(layers.effective_config()) -} - fn deserialize_config_toml_with_base( root_value: TomlValue, config_base_dir: &Path, @@ -435,13 +430,18 @@ fn deserialize_config_toml_with_base( pub async fn load_global_mcp_servers( codex_home: &Path, ) -> std::io::Result> { - let root_value = load_resolved_config( - codex_home, - Vec::new(), - crate::config_loader::LoaderOverrides::default(), - ) - .await?; - let Some(servers_value) = root_value.get("mcp_servers") else { + // In general, Config::load_with_cli_overrides() should be used to load the + // full config with requirements.toml applied, but in this case, we need + // access to the raw TOML in order to warn the user about deprecated fields. + // + // Note that a more precise way to do this would be to audit the individual + // config layers for deprecated fields rather than reporting on the merged + // result. + let cli_overrides = Vec::<(String, TomlValue)>::new(); + let config_layer_stack = + load_config_layers_state(codex_home, &cli_overrides, LoaderOverrides::default()).await?; + let merged_toml = config_layer_stack.effective_config(); + let Some(servers_value) = merged_toml.get("mcp_servers") else { return Ok(BTreeMap::new()); }; @@ -999,6 +999,16 @@ impl Config { cfg: ConfigToml, overrides: ConfigOverrides, codex_home: PathBuf, + ) -> std::io::Result { + let requirements = ConfigRequirements::default(); + Self::load_config_with_requirements(cfg, overrides, codex_home, requirements) + } + + fn load_config_with_requirements( + cfg: ConfigToml, + overrides: ConfigOverrides, + codex_home: PathBuf, + requirements: ConfigRequirements, ) -> std::io::Result { let user_instructions = Self::load_instructions(Some(&codex_home)); @@ -1104,7 +1114,8 @@ impl Config { AskForApproval::default() } }); - let approval_policy = Constrained::allow_any(approval_policy); + // TODO(dylan): We should be able to leverage ConfigLayerStack so that + // we can reliably check this at every config level. let did_user_set_custom_approval_policy_or_sandbox_mode = approval_policy_override .is_some() || config_profile.approval_policy.is_some() @@ -1221,6 +1232,16 @@ impl Config { let check_for_update_on_startup = cfg.check_for_update_on_startup.unwrap_or(true); + // Ensure that every field of ConfigRequirements is applied to the final + // Config. + let ConfigRequirements { + approval_policy: mut constrained_approval_policy, + } = requirements; + + constrained_approval_policy + .set(approval_policy) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, format!("{e}")))?; + let config = Self { model, review_model, @@ -1229,7 +1250,7 @@ impl Config { model_provider_id, model_provider, cwd: resolved_cwd, - approval_policy, + approval_policy: constrained_approval_policy, sandbox_policy, did_user_set_custom_approval_policy_or_sandbox_mode, forced_auto_mode_downgraded_on_windows, @@ -1920,18 +1941,22 @@ trust_level = "trusted" std::fs::write(&config_path, "mcp_oauth_credentials_store = \"file\"\n")?; std::fs::write(&managed_path, "mcp_oauth_credentials_store = \"keyring\"\n")?; - let overrides = crate::config_loader::LoaderOverrides { + let overrides = LoaderOverrides { managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, }; - let root_value = load_resolved_config(codex_home.path(), Vec::new(), overrides).await?; - let cfg = - deserialize_config_toml_with_base(root_value, codex_home.path()).map_err(|e| { - tracing::error!("Failed to deserialize overridden config: {e}"); - e - })?; + let config_layer_stack = + load_config_layers_state(codex_home.path(), &Vec::new(), overrides).await?; + let cfg = deserialize_config_toml_with_base( + config_layer_stack.effective_config(), + codex_home.path(), + ) + .map_err(|e| { + tracing::error!("Failed to deserialize overridden config: {e}"); + e + })?; assert_eq!( cfg.mcp_oauth_credentials_store, Some(OAuthCredentialsStoreMode::Keyring), @@ -2035,24 +2060,27 @@ trust_level = "trusted" )?; std::fs::write(&managed_path, "model = \"managed_config\"\n")?; - let overrides = crate::config_loader::LoaderOverrides { + let overrides = LoaderOverrides { managed_config_path: Some(managed_path), #[cfg(target_os = "macos")] managed_preferences_base64: None, }; - let root_value = load_resolved_config( + let config_layer_stack = load_config_layers_state( codex_home.path(), - vec![("model".to_string(), TomlValue::String("cli".to_string()))], + &[("model".to_string(), TomlValue::String("cli".to_string()))], overrides, ) .await?; - let cfg = - deserialize_config_toml_with_base(root_value, codex_home.path()).map_err(|e| { - tracing::error!("Failed to deserialize overridden config: {e}"); - e - })?; + let cfg = deserialize_config_toml_with_base( + config_layer_stack.effective_config(), + codex_home.path(), + ) + .map_err(|e| { + tracing::error!("Failed to deserialize overridden config: {e}"); + e + })?; assert_eq!(cfg.model.as_deref(), Some("managed_config")); Ok(()) diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index 713d5c9e7..707936cb7 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -20,7 +20,9 @@ use codex_app_server_protocol::ConfigWriteResponse; use codex_app_server_protocol::MergeStrategy; use codex_app_server_protocol::OverriddenMetadata; use codex_app_server_protocol::WriteStatus; +use codex_utils_absolute_path::AbsolutePathBuf; use serde_json::Value as JsonValue; +use std::borrow::Cow; use std::path::Path; use std::path::PathBuf; use thiserror::Error; @@ -146,7 +148,13 @@ impl ConfigService { Ok(ConfigReadResponse { config, origins: layers.origins(), - layers: params.include_layers.then(|| layers.layers_high_to_low()), + layers: params.include_layers.then(|| { + layers + .layers_high_to_low() + .iter() + .map(|layer| layer.as_layer()) + .collect() + }), }) } @@ -194,11 +202,14 @@ impl ConfigService { expected_version: Option, edits: Vec<(String, JsonValue, MergeStrategy)>, ) -> Result { - let allowed_path = self.codex_home.join(CONFIG_TOML_FILE); - let provided_path = file_path - .as_ref() - .map(PathBuf::from) - .unwrap_or_else(|| allowed_path.clone()); + let allowed_path = + AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, &self.codex_home) + .map_err(|err| ConfigServiceError::io("failed to resolve user config path", err))?; + let provided_path = match file_path { + Some(path) => AbsolutePathBuf::from_absolute_path(PathBuf::from(path)) + .map_err(|err| ConfigServiceError::io("failed to resolve user config path", err))?, + None => allowed_path.clone(), + }; if !paths_match(&allowed_path, &provided_path) { return Err(ConfigServiceError::write( @@ -211,9 +222,13 @@ impl ConfigService { .load_layers_state() .await .map_err(|err| ConfigServiceError::io("failed to load configuration", err))?; + let user_layer = match layers.get_user_layer() { + Some(layer) => Cow::Borrowed(layer), + None => Cow::Owned(create_empty_user_layer(&allowed_path).await?), + }; if let Some(expected) = expected_version.as_deref() - && expected != layers.user.version + && expected != user_layer.version { return Err(ConfigServiceError::write( ConfigWriteErrorCode::ConfigVersionConflict, @@ -221,7 +236,7 @@ impl ConfigService { )); } - let mut user_config = layers.user.config.clone(); + let mut user_config = user_layer.config.clone(); let mut parsed_segments = Vec::new(); let mut config_edits = Vec::new(); @@ -273,7 +288,7 @@ impl ConfigService { ) })?; - let updated_layers = layers.with_user_config(user_config.clone()); + let updated_layers = layers.with_user_config(&provided_path, user_config.clone()); let effective = updated_layers.effective_config(); validate_config(&effective).map_err(|err| { ConfigServiceError::write( @@ -296,16 +311,19 @@ impl ConfigService { .map(|_| WriteStatus::OkOverridden) .unwrap_or(WriteStatus::Ok); - let file_path = provided_path - .canonicalize() - .unwrap_or(provided_path.clone()) - .display() - .to_string(); - Ok(ConfigWriteResponse { status, - version: updated_layers.user.version.clone(), - file_path, + version: updated_layers + .get_user_layer() + .ok_or_else(|| { + ConfigServiceError::write( + ConfigWriteErrorCode::UserLayerNotFound, + "user layer not found in updated layers", + ) + })? + .version + .clone(), + file_path: provided_path, overridden_metadata: overridden, }) } @@ -320,6 +338,32 @@ impl ConfigService { } } +async fn create_empty_user_layer( + config_toml: &AbsolutePathBuf, +) -> Result { + let toml_value = match tokio::fs::read_to_string(config_toml).await { + Ok(contents) => toml::from_str(&contents).map_err(|e| { + ConfigServiceError::toml("failed to parse existing user config.toml", e) + })?, + Err(e) => { + if e.kind() == std::io::ErrorKind::NotFound { + tokio::fs::write(config_toml, "").await.map_err(|e| { + ConfigServiceError::io("failed to create empty user config.toml", e) + })?; + TomlValue::Table(toml::map::Map::new()) + } else { + return Err(ConfigServiceError::io("failed to read user config.toml", e)); + } + } + }; + Ok(ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_toml.clone(), + }, + toml_value, + )) +} + fn parse_value(value: JsonValue) -> Result, String> { if value.is_null() { return Ok(None); @@ -501,10 +545,25 @@ fn value_at_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a Tom fn override_message(layer: &ConfigLayerSource) -> String { match layer { - ConfigLayerSource::Mdm { .. } => "Overridden by managed policy (mdm)".to_string(), - ConfigLayerSource::System { .. } => "Overridden by managed config (system)".to_string(), + ConfigLayerSource::Mdm { domain, key: _ } => { + format!("Overridden by managed policy (MDM): {domain}") + } + ConfigLayerSource::System { file } => { + format!("Overridden by managed config (system): {}", file.display()) + } ConfigLayerSource::SessionFlags => "Overridden by session flags".to_string(), - ConfigLayerSource::User { .. } => "Overridden by user config".to_string(), + ConfigLayerSource::User { file } => { + format!("Overridden by user config: {}", file.display()) + } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file } => { + format!( + "Overridden by legacy managed_config.toml: {}", + file.display() + ) + } + ConfigLayerSource::LegacyManagedConfigTomlFromMdm => { + "Overridden by legacy managed configuration from MDM".to_string() + } } } @@ -513,7 +572,10 @@ fn compute_override_metadata( effective: &TomlValue, segments: &[String], ) -> Option { - let user_value = value_at_path(&layers.user.config, segments); + let user_value = match layers.get_user_layer() { + Some(user_layer) => value_at_path(&user_layer.config, segments), + None => return None, + }; let effective_value = value_at_path(effective, segments); if user_value.is_some() && user_value == effective_value { @@ -524,8 +586,7 @@ fn compute_override_metadata( return None; } - let effective_layer = find_effective_layer(layers, segments); - let overriding_layer = effective_layer.unwrap_or_else(|| layers.user.metadata()); + let overriding_layer = find_effective_layer(layers, segments)?; let message = override_message(&overriding_layer.name); Some(OverriddenMetadata { @@ -554,23 +615,13 @@ fn find_effective_layer( layers: &ConfigLayerStack, segments: &[String], ) -> Option { - let check = - |state: &ConfigLayerEntry| value_at_path(&state.config, segments).map(|_| state.metadata()); + for layer in layers.layers_high_to_low() { + if let Some(meta) = value_at_path(&layer.config, segments).map(|_| layer.metadata()) { + return Some(meta); + } + } - if let Some(mdm) = &layers.mdm - && let Some(meta) = check(mdm) - { - return Some(meta); - } - if let Some(system) = &layers.system - && let Some(meta) = check(system) - { - return Some(meta); - } - if let Some(meta) = check(&layers.session_flags) { - return Some(meta); - } - check(&layers.user) + None } #[cfg(test)] @@ -713,18 +764,18 @@ remote_compaction = true .get("approval_policy") .expect("origin") .name, - ConfigLayerSource::System { - file: managed_file.clone(), - } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: managed_file.clone() + }, ); let layers = response.layers.expect("layers present"); + assert_eq!(layers.len(), 2, "expected two layers"); assert_eq!( layers.first().unwrap().name, - ConfigLayerSource::System { file: managed_file } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } ); - assert_eq!(layers.get(1).unwrap().name, ConfigLayerSource::SessionFlags); assert_eq!( - layers.last().unwrap().name, + layers.get(1).unwrap().name, ConfigLayerSource::User { file: user_file } ); } @@ -779,7 +830,9 @@ remote_compaction = true .get("approval_policy") .expect("origin") .name, - ConfigLayerSource::System { file: managed_file } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: managed_file.clone() + } ); assert_eq!(result.status, WriteStatus::Ok); assert!(result.overridden_metadata.is_none()); @@ -909,14 +962,14 @@ remote_compaction = true assert_eq!(response.config.model.as_deref(), Some("system")); assert_eq!( response.origins.get("model").expect("origin").name, - ConfigLayerSource::System { - file: managed_file.clone(), - } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: managed_file.clone() + }, ); let layers = response.layers.expect("layers"); assert_eq!( layers.first().unwrap().name, - ConfigLayerSource::System { file: managed_file } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } ); assert_eq!(layers.get(1).unwrap().name, ConfigLayerSource::SessionFlags); assert_eq!( @@ -959,7 +1012,7 @@ remote_compaction = true let overridden = result.overridden_metadata.expect("overridden metadata"); assert_eq!( overridden.overriding_layer.name, - ConfigLayerSource::System { file: managed_file } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } ); assert_eq!(overridden.effective_value, serde_json::json!("never")); } diff --git a/codex-rs/core/src/config_loader/config_requirements.rs b/codex-rs/core/src/config_loader/config_requirements.rs new file mode 100644 index 000000000..16fd9fcff --- /dev/null +++ b/codex-rs/core/src/config_loader/config_requirements.rs @@ -0,0 +1,47 @@ +use codex_protocol::protocol::AskForApproval; +use serde::Deserialize; + +use crate::config::Constrained; +use crate::config::ConstraintError; + +/// Normalized version of [`ConfigRequirementsToml`] after deserialization and +/// normalization. +#[derive(Debug, Clone, PartialEq)] +pub struct ConfigRequirements { + pub approval_policy: Constrained, +} + +impl Default for ConfigRequirements { + fn default() -> Self { + Self { + approval_policy: Constrained::allow_any_from_default(), + } + } +} + +/// Base config deserialized from /etc/codex/requirements.toml or MDM. +#[derive(Deserialize, Debug, Clone, Default, PartialEq)] +pub struct ConfigRequirementsToml { + pub allowed_approval_policies: Option>, +} + +impl TryFrom for ConfigRequirements { + type Error = ConstraintError; + + fn try_from(toml: ConfigRequirementsToml) -> Result { + let approval_policy: Constrained = match toml.allowed_approval_policies { + Some(policies) => { + let default_value = AskForApproval::default(); + if policies.contains(&default_value) { + Constrained::allow_values(default_value, policies)? + } else if let Some(first) = policies.first() { + Constrained::allow_values(*first, policies)? + } else { + return Err(ConstraintError::empty_field("allowed_approval_policies")); + } + } + None => Constrained::allow_any_from_default(), + }; + Ok(ConfigRequirements { approval_policy }) + } +} diff --git a/codex-rs/core/src/config_loader/layer_io.rs b/codex-rs/core/src/config_loader/layer_io.rs index d9dd449e8..d43127296 100644 --- a/codex-rs/core/src/config_loader/layer_io.rs +++ b/codex-rs/core/src/config_loader/layer_io.rs @@ -1,8 +1,7 @@ use super::LoaderOverrides; #[cfg(target_os = "macos")] use super::macos::load_managed_admin_config_layer; -use super::overrides::default_empty_table; -use crate::config::CONFIG_TOML_FILE; +use codex_utils_absolute_path::AbsolutePathBuf; use std::io; use std::path::Path; use std::path::PathBuf; @@ -12,11 +11,18 @@ use toml::Value as TomlValue; #[cfg(unix)] const CODEX_MANAGED_CONFIG_SYSTEM_PATH: &str = "/etc/codex/managed_config.toml"; +#[derive(Debug, Clone)] +pub(super) struct MangedConfigFromFile { + pub managed_config: TomlValue, + pub file: AbsolutePathBuf, +} + #[derive(Debug, Clone)] pub(super) struct LoadedConfigLayers { - pub base: TomlValue, - pub managed_config: Option, - pub managed_preferences: Option, + /// If present, data read from a file such as `/etc/codex/managed_config.toml`. + pub managed_config: Option, + /// If present, data read from managed preferences (macOS only). + pub managed_config_from_mdm: Option, } pub(super) async fn load_config_layers_internal( @@ -34,12 +40,16 @@ pub(super) async fn load_config_layers_internal( managed_config_path, } = overrides; - let managed_config_path = - managed_config_path.unwrap_or_else(|| managed_config_default_path(codex_home)); + let managed_config_path = AbsolutePathBuf::from_absolute_path( + managed_config_path.unwrap_or_else(|| managed_config_default_path(codex_home)), + )?; - let user_config_path = codex_home.join(CONFIG_TOML_FILE); - let user_config = read_config_from_path(&user_config_path, true).await?; - let managed_config = read_config_from_path(&managed_config_path, false).await?; + let managed_config = read_config_from_path(&managed_config_path, false) + .await? + .map(|managed_config| MangedConfigFromFile { + managed_config, + file: managed_config_path.clone(), + }); #[cfg(target_os = "macos")] let managed_preferences = @@ -49,9 +59,8 @@ pub(super) async fn load_config_layers_internal( let managed_preferences = None; Ok(LoadedConfigLayers { - base: user_config.unwrap_or_else(default_empty_table), managed_config, - managed_preferences, + managed_config_from_mdm: managed_preferences, }) } diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 19af4c729..04fc8d245 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -1,3 +1,4 @@ +mod config_requirements; mod fingerprint; mod layer_io; #[cfg(target_os = "macos")] @@ -10,74 +11,176 @@ mod state; mod tests; use crate::config::CONFIG_TOML_FILE; +use crate::config_loader::layer_io::LoadedConfigLayers; use codex_app_server_protocol::ConfigLayerSource; +use codex_protocol::protocol::AskForApproval; use codex_utils_absolute_path::AbsolutePathBuf; +use serde::Deserialize; use std::io; use std::path::Path; use toml::Value as TomlValue; +pub use config_requirements::ConfigRequirements; pub use merge::merge_toml_values; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::LoaderOverrides; -const MDM_PREFERENCES_DOMAIN: &str = "com.openai.codex"; -const MDM_PREFERENCES_KEY: &str = "config_toml_base64"; - -/// Configuration layering pipeline (top overrides bottom): +/// To build up the set of admin-enforced constraints, we build up from multiple +/// configuration layers in the following order, but a constraint defined in an +/// earlier layer cannot be overridden by a later layer: /// -/// +-------------------------+ -/// | Managed preferences (*) | -/// +-------------------------+ -/// ^ -/// | -/// +-------------------------+ -/// | managed_config.toml | -/// +-------------------------+ -/// ^ -/// | -/// +-------------------------+ -/// | config.toml (base) | -/// +-------------------------+ +/// - admin: managed preferences (*) +/// - system `/etc/codex/requirements.toml` +/// +/// For backwards compatibility, we also load from +/// `/etc/codex/managed_config.toml` and map it to +/// `/etc/codex/requirements.toml`. +/// +/// Configuration is built up from multiple layers in the following order: +/// +/// - admin: managed preferences (*) +/// - system `/etc/codex/config.toml` +/// - user `${CODEX_HOME}/config.toml` +/// - cwd `${PWD}/config.toml` +/// - tree parent directories up to root looking for `./.codex/config.toml` +/// - repo `$(git rev-parse --show-toplevel)/.codex/config.toml` +/// - runtime e.g., --config flags, model selector in UI /// /// (*) Only available on macOS via managed device profiles. +/// +/// See https://developers.openai.com/codex/security for details. pub async fn load_config_layers_state( codex_home: &Path, cli_overrides: &[(String, TomlValue)], overrides: LoaderOverrides, ) -> io::Result { - let managed_config_path = overrides - .managed_config_path - .clone() - .unwrap_or_else(|| layer_io::managed_config_default_path(codex_home)); + let loaded_config_layers = layer_io::load_config_layers_internal(codex_home, overrides).await?; + let requirements = load_requirements_from_legacy_scheme(loaded_config_layers.clone()).await?; - let layers = layer_io::load_config_layers_internal(codex_home, overrides).await?; - let cli_overrides_layer = overrides::build_cli_overrides_layer(cli_overrides); - let user_file = AbsolutePathBuf::from_absolute_path(codex_home.join(CONFIG_TOML_FILE))?; + // TODO(mbolin): Honor /etc/codex/requirements.toml. - let system = match layers.managed_config { - Some(cfg) => { - let system_file = AbsolutePathBuf::from_absolute_path(managed_config_path.clone())?; - Some(ConfigLayerEntry::new( - ConfigLayerSource::System { file: system_file }, - cfg, - )) + let mut layers = Vec::::new(); + + // TODO(mbolin): Honor managed preferences (macOS only). + // TODO(mbolin): Honor /etc/codex/config.toml. + + // Add a layer for $CODEX_HOME/config.toml if it exists. Note if the file + // exists, but is malformed, then this error should be propagated to the + // user. + let user_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home)?; + match tokio::fs::read_to_string(&user_file).await { + Ok(contents) => { + let user_config: TomlValue = toml::from_str(&contents).map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidData, + format!( + "Error parsing user config file {}: {e}", + user_file.as_path().display(), + ), + ) + })?; + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + user_config, + )); } - None => None, - }; + Err(e) => { + if e.kind() != io::ErrorKind::NotFound { + return Err(io::Error::new( + e.kind(), + format!( + "Failed to read user config file {}: {e}", + user_file.as_path().display(), + ), + )); + } + } + } - Ok(ConfigLayerStack { - user: ConfigLayerEntry::new(ConfigLayerSource::User { file: user_file }, layers.base), - session_flags: ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, cli_overrides_layer), - system, - mdm: layers.managed_preferences.map(|cfg| { - ConfigLayerEntry::new( - ConfigLayerSource::Mdm { - domain: MDM_PREFERENCES_DOMAIN.to_string(), - key: MDM_PREFERENCES_KEY.to_string(), - }, - cfg, - ) - }), - }) + // TODO(mbolin): Add layers for cwd, tree, and repo config files. + + // Add a layer for runtime overrides from the CLI or UI, if any exist. + if !cli_overrides.is_empty() { + let cli_overrides_layer = overrides::build_cli_overrides_layer(cli_overrides); + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + cli_overrides_layer, + )); + } + + // Make a best-effort to support the legacy `managed_config.toml` as a + // config layer on top of everything else. For fields in + // `managed_config.toml` that do not have an equivalent in + // `ConfigRequirements`, note users can still override these values on a + // per-turn basis in the TUI and VS Code. + let LoadedConfigLayers { + managed_config, + managed_config_from_mdm, + } = loaded_config_layers; + if let Some(config) = managed_config { + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: config.file.clone(), + }, + config.managed_config, + )); + } + if let Some(config) = managed_config_from_mdm { + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromMdm, + config, + )); + } + + ConfigLayerStack::new(layers, requirements) +} + +async fn load_requirements_from_legacy_scheme( + loaded_config_layers: LoadedConfigLayers, +) -> io::Result { + let mut config_requirements = ConfigRequirements::default(); + + // In this implementation, later layers override earlier layers, so list + // managed_config_from_mdm last because it has the highest precedence. + let LoadedConfigLayers { + managed_config, + managed_config_from_mdm, + } = loaded_config_layers; + for config in [ + managed_config.map(|c| c.managed_config), + managed_config_from_mdm, + ] + .into_iter() + .flatten() + { + let legacy_config: LegacyManagedConfigToml = + config.try_into().map_err(|err: toml::de::Error| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("Failed to parse config requirements as TOML: {err}"), + ) + })?; + + let LegacyManagedConfigToml { approval_policy } = legacy_config; + if let Some(approval_policy) = approval_policy { + config_requirements.approval_policy = + crate::config::Constrained::allow_only(approval_policy); + } + } + + Ok(config_requirements) +} + +/// The legacy mechanism for specifying admin-enforced configuration is to read +/// from a file like `/etc/codex/managed_config.toml` that has the same +/// structure as `config.toml` where fields like `approval_policy` can specify +/// exactly one value rather than a list of allowed values. +/// +/// If present, re-interpret `managed_config.toml` as a `requirements.toml` +/// where each specified field is treated as a constraint allowing only that +/// value. +#[derive(Deserialize, Debug, Clone, Default, PartialEq)] +struct LegacyManagedConfigToml { + approval_policy: Option, } diff --git a/codex-rs/core/src/config_loader/state.rs b/codex-rs/core/src/config_loader/state.rs index 4ae235b79..864a9c7ed 100644 --- a/codex-rs/core/src/config_loader/state.rs +++ b/codex-rs/core/src/config_loader/state.rs @@ -1,9 +1,12 @@ +use crate::config_loader::ConfigRequirements; + use super::fingerprint::record_origins; use super::fingerprint::version_for_toml; use super::merge::merge_toml_values; use codex_app_server_protocol::ConfigLayer; use codex_app_server_protocol::ConfigLayerMetadata; use codex_app_server_protocol::ConfigLayerSource; +use codex_utils_absolute_path::AbsolutePathBuf; use serde_json::Value as JsonValue; use std::collections::HashMap; use std::path::PathBuf; @@ -51,30 +54,90 @@ impl ConfigLayerEntry { #[derive(Debug, Clone)] pub struct ConfigLayerStack { - pub user: ConfigLayerEntry, - pub session_flags: ConfigLayerEntry, - pub system: Option, - pub mdm: Option, + /// Layers are listed from lowest precedence (base) to highest (top), so + /// later entries in the Vec override earlier ones. + layers: Vec, + + /// Index into [layers] of the user config layer, if any. + user_layer_index: Option, + + /// Constraints that must be enforced when deriving a [Config] from the + /// layers. + requirements: ConfigRequirements, } impl ConfigLayerStack { - pub fn with_user_config(&self, user_config: TomlValue) -> Self { - Self { - user: ConfigLayerEntry::new(self.user.name.clone(), user_config), - session_flags: self.session_flags.clone(), - system: self.system.clone(), - mdm: self.mdm.clone(), + pub fn new( + layers: Vec, + requirements: ConfigRequirements, + ) -> std::io::Result { + let user_layer_index = verify_layer_ordering(&layers)?; + Ok(Self { + layers, + user_layer_index, + requirements, + }) + } + + /// Returns the user config layer, if any. + pub fn get_user_layer(&self) -> Option<&ConfigLayerEntry> { + self.user_layer_index + .and_then(|index| self.layers.get(index)) + } + + pub fn requirements(&self) -> &ConfigRequirements { + &self.requirements + } + + /// Creates a new [ConfigLayerStack] using the specified values to inject a + /// "user layer" into the stack. If such a layer already exists, it is + /// replaced; otherwise, it is inserted into the stack at the appropriate + /// position based on precedence rules. + pub fn with_user_config(&self, config_toml: &AbsolutePathBuf, user_config: TomlValue) -> Self { + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_toml.clone(), + }, + user_config, + ); + + let mut layers = self.layers.clone(); + match self.user_layer_index { + Some(index) => { + layers[index] = user_layer; + Self { + layers, + user_layer_index: self.user_layer_index, + requirements: self.requirements.clone(), + } + } + None => { + let user_layer_index = match layers + .iter() + .position(|layer| layer.name.precedence() > user_layer.name.precedence()) + { + Some(index) => { + layers.insert(index, user_layer); + index + } + None => { + layers.push(user_layer); + layers.len() - 1 + } + }; + Self { + layers, + user_layer_index: Some(user_layer_index), + requirements: self.requirements.clone(), + } + } } } pub fn effective_config(&self) -> TomlValue { - let mut merged = self.user.config.clone(); - merge_toml_values(&mut merged, &self.session_flags.config); - if let Some(system) = &self.system { - merge_toml_values(&mut merged, &system.config); - } - if let Some(mdm) = &self.mdm { - merge_toml_values(&mut merged, &mdm.config); + let mut merged = TomlValue::Table(toml::map::Map::new()); + for layer in &self.layers { + merge_toml_values(&mut merged, &layer.config); } merged } @@ -83,38 +146,42 @@ impl ConfigLayerStack { let mut origins = HashMap::new(); let mut path = Vec::new(); - record_origins( - &self.user.config, - &self.user.metadata(), - &mut path, - &mut origins, - ); - record_origins( - &self.session_flags.config, - &self.session_flags.metadata(), - &mut path, - &mut origins, - ); - if let Some(system) = &self.system { - record_origins(&system.config, &system.metadata(), &mut path, &mut origins); - } - if let Some(mdm) = &self.mdm { - record_origins(&mdm.config, &mdm.metadata(), &mut path, &mut origins); + for layer in &self.layers { + record_origins(&layer.config, &layer.metadata(), &mut path, &mut origins); } origins } - pub fn layers_high_to_low(&self) -> Vec { - let mut layers = Vec::new(); - if let Some(mdm) = &self.mdm { - layers.push(mdm.as_layer()); - } - if let Some(system) = &self.system { - layers.push(system.as_layer()); - } - layers.push(self.session_flags.as_layer()); - layers.push(self.user.as_layer()); - layers + /// Returns the highest-precedence to lowest-precedence layers, so + /// `ConfigLayerSource::SessionFlags` would be first, if present. + pub fn layers_high_to_low(&self) -> Vec<&ConfigLayerEntry> { + self.layers.iter().rev().collect() } } + +/// Ensures precedence ordering of config layers is correct. Returns the index +/// of the user config layer, if any (at most one should exist). +fn verify_layer_ordering(layers: &[ConfigLayerEntry]) -> std::io::Result> { + if !layers.iter().map(|layer| &layer.name).is_sorted() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "config layers are not in correct precedence order", + )); + } + + let mut user_layer_index: Option = None; + for (index, layer) in layers.iter().enumerate() { + if matches!(layer.name, ConfigLayerSource::User { .. }) { + if user_layer_index.is_some() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "multiple user config layers found", + )); + } + user_layer_index = Some(index); + } + } + + Ok(user_layer_index) +} diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 121ef736d..15d457836 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -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" );