diff --git a/codex-rs/core/src/config_loader/config_requirements.rs b/codex-rs/core/src/config_loader/config_requirements.rs index 16fd9fcff..f611b31ff 100644 --- a/codex-rs/core/src/config_loader/config_requirements.rs +++ b/codex-rs/core/src/config_loader/config_requirements.rs @@ -25,6 +25,26 @@ pub struct ConfigRequirementsToml { pub allowed_approval_policies: Option>, } +impl ConfigRequirementsToml { + /// For every field in `other` that is `Some`, if the corresponding field in + /// `self` is `None`, copy the value from `other` into `self`. + pub fn merge_unset_fields(&mut self, mut other: ConfigRequirementsToml) { + macro_rules! fill_missing_take { + ($base:expr, $other:expr, { $($field:ident),+ $(,)? }) => { + $( + if $base.$field.is_none() { + if let Some(value) = $other.$field.take() { + $base.$field = Some(value); + } + } + )+ + }; + } + + fill_missing_take!(self, other, { allowed_approval_policies }); + } +} + impl TryFrom for ConfigRequirements { type Error = ConstraintError; @@ -45,3 +65,43 @@ impl TryFrom for ConfigRequirements { Ok(ConfigRequirements { approval_policy }) } } + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::Result; + use pretty_assertions::assert_eq; + use toml::from_str; + + #[test] + fn merge_unset_fields_only_fills_missing_values() -> Result<()> { + let source: ConfigRequirementsToml = from_str( + r#" + allowed_approval_policies = ["on-request"] + "#, + )?; + + let mut empty_target: ConfigRequirementsToml = from_str( + r#" + # intentionally left unset + "#, + )?; + empty_target.merge_unset_fields(source.clone()); + assert_eq!( + empty_target.allowed_approval_policies, + Some(vec![AskForApproval::OnRequest]) + ); + + let mut populated_target: ConfigRequirementsToml = from_str( + r#" + allowed_approval_policies = ["never"] + "#, + )?; + populated_target.merge_unset_fields(source); + assert_eq!( + populated_target.allowed_approval_policies, + Some(vec![AskForApproval::Never]) + ); + Ok(()) + } +} diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 04fc8d245..85d4014a6 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -11,6 +11,7 @@ mod state; mod tests; use crate::config::CONFIG_TOML_FILE; +use crate::config_loader::config_requirements::ConfigRequirementsToml; use crate::config_loader::layer_io::LoadedConfigLayers; use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::AskForApproval; @@ -26,6 +27,9 @@ pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::LoaderOverrides; +/// On Unix systems, load requirements from this file path, if present. +const DEFAULT_REQUIREMENTS_TOML_FILE_UNIX: &str = "/etc/codex/requirements.toml"; + /// 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: @@ -55,10 +59,28 @@ pub async fn load_config_layers_state( cli_overrides: &[(String, TomlValue)], overrides: LoaderOverrides, ) -> io::Result { - 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 mut config_requirements_toml = ConfigRequirementsToml::default(); - // TODO(mbolin): Honor /etc/codex/requirements.toml. + // TODO(mbolin): Support an entry in MDM for config requirements and use it + // with `config_requirements_toml.merge_unset_fields(...)`, if present. + + // Honor /etc/codex/requirements.toml. + if cfg!(unix) { + load_requirements_toml( + &mut config_requirements_toml, + DEFAULT_REQUIREMENTS_TOML_FILE_UNIX, + ) + .await?; + } + + // Make a best-effort to support the legacy `managed_config.toml` as a + // requirements specification. + let loaded_config_layers = layer_io::load_config_layers_internal(codex_home, overrides).await?; + load_requirements_from_legacy_scheme( + &mut config_requirements_toml, + loaded_config_layers.clone(), + ) + .await?; let mut layers = Vec::::new(); @@ -133,23 +155,59 @@ pub async fn load_config_layers_state( )); } - ConfigLayerStack::new(layers, requirements) + ConfigLayerStack::new(layers, config_requirements_toml.try_into()?) +} + +/// If available, apply requirements from `/etc/codex/requirements.toml` to +/// `config_requirements_toml` by filling in any unset fields. +async fn load_requirements_toml( + config_requirements_toml: &mut ConfigRequirementsToml, + requirements_toml_file: impl AsRef, +) -> io::Result<()> { + match tokio::fs::read_to_string(&requirements_toml_file).await { + Ok(contents) => { + let requirements_config: ConfigRequirementsToml = + toml::from_str(&contents).map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidData, + format!( + "Error parsing requirements file {}: {e}", + requirements_toml_file.as_ref().display(), + ), + ) + })?; + config_requirements_toml.merge_unset_fields(requirements_config); + } + Err(e) => { + if e.kind() != io::ErrorKind::NotFound { + return Err(io::Error::new( + e.kind(), + format!( + "Failed to read requirements file {}: {e}", + requirements_toml_file.as_ref().display(), + ), + )); + } + } + } + + Ok(()) } async fn load_requirements_from_legacy_scheme( + config_requirements_toml: &mut ConfigRequirementsToml, 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. +) -> io::Result<()> { + // In this implementation, earlier layers cannot be overwritten by later + // layers, so list managed_config_from_mdm first 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, + managed_config.map(|c| c.managed_config), ] .into_iter() .flatten() @@ -162,14 +220,11 @@ async fn load_requirements_from_legacy_scheme( ) })?; - let LegacyManagedConfigToml { approval_policy } = legacy_config; - if let Some(approval_policy) = approval_policy { - config_requirements.approval_policy = - crate::config::Constrained::allow_only(approval_policy); - } + let new_requirements_toml = ConfigRequirementsToml::from(legacy_config); + config_requirements_toml.merge_unset_fields(new_requirements_toml); } - Ok(config_requirements) + Ok(()) } /// The legacy mechanism for specifying admin-enforced configuration is to read @@ -184,3 +239,16 @@ async fn load_requirements_from_legacy_scheme( struct LegacyManagedConfigToml { approval_policy: Option, } + +impl From for ConfigRequirementsToml { + fn from(legacy: LegacyManagedConfigToml) -> Self { + let mut config_requirements_toml = ConfigRequirementsToml::default(); + + let LegacyManagedConfigToml { approval_policy } = legacy; + if let Some(approval_policy) = approval_policy { + config_requirements_toml.allowed_approval_policies = Some(vec![approval_policy]); + } + + config_requirements_toml + } +} diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 15d457836..fdd97eb67 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -1,6 +1,11 @@ use super::LoaderOverrides; use super::load_config_layers_state; use crate::config::CONFIG_TOML_FILE; +use crate::config_loader::ConfigRequirements; +use crate::config_loader::config_requirements::ConfigRequirementsToml; +use crate::config_loader::load_requirements_toml; +use codex_protocol::protocol::AskForApproval; +use pretty_assertions::assert_eq; use tempfile::tempdir; use toml::Value as TomlValue; @@ -147,3 +152,40 @@ flag = true ); assert_eq!(nested.get("flag"), Some(&TomlValue::Boolean(false))); } + +#[tokio::test(flavor = "current_thread")] +async fn load_requirements_toml_produces_expected_constraints() -> anyhow::Result<()> { + let tmp = tempdir()?; + let requirements_file = tmp.path().join("requirements.toml"); + tokio::fs::write( + &requirements_file, + r#" +allowed_approval_policies = ["never", "on-request"] +"#, + ) + .await?; + + let mut config_requirements_toml = ConfigRequirementsToml::default(); + load_requirements_toml(&mut config_requirements_toml, &requirements_file).await?; + + assert_eq!( + config_requirements_toml.allowed_approval_policies, + Some(vec![AskForApproval::Never, AskForApproval::OnRequest]) + ); + + let config_requirements: ConfigRequirements = config_requirements_toml.try_into()?; + assert_eq!( + config_requirements.approval_policy.value(), + AskForApproval::OnRequest + ); + config_requirements + .approval_policy + .can_set(&AskForApproval::Never)?; + assert!( + config_requirements + .approval_policy + .can_set(&AskForApproval::OnFailure) + .is_err() + ); + Ok(()) +}