From 932a5a446f42e566c0dbd3004ae2cee50cdcc0ce Mon Sep 17 00:00:00 2001 From: gt-oai Date: Thu, 8 Jan 2026 16:11:14 +0000 Subject: [PATCH] config requirements: improve requirement error messages (#8843) **Before:** ``` Error loading configuration: value `Never` is not in the allowed set [OnRequest] ``` **After:** ``` Error loading configuration: invalid value for `approval_policy`: `Never` is not in the allowed set [OnRequest] (set by MDM com.openai.codex:requirements_toml_base64) ``` Done by introducing a new struct `ConfigRequirementsWithSources` onto which we `merge_unset_fields` now. Also introduces a pair of requirement value and its `RequirementSource` (inspired by `ConfigLayerSource`): ```rust pub struct Sourced { pub value: T, pub source: RequirementSource, } ``` --- codex-rs/core/src/config/constraint.rs | 86 ++--- .../src/config_loader/config_requirements.rs | 341 +++++++++++++++--- codex-rs/core/src/config_loader/macos.rs | 22 +- codex-rs/core/src/config_loader/mod.rs | 43 ++- codex-rs/core/src/config_loader/tests.rs | 9 +- codex-rs/tui/src/chatwidget/tests.rs | 17 +- 6 files changed, 378 insertions(+), 140 deletions(-) diff --git a/codex-rs/core/src/config/constraint.rs b/codex-rs/core/src/config/constraint.rs index 795a8d568..5a412a0d0 100644 --- a/codex-rs/core/src/config/constraint.rs +++ b/codex-rs/core/src/config/constraint.rs @@ -1,25 +1,26 @@ use std::fmt; use std::sync::Arc; +use crate::config_loader::RequirementSource; use thiserror::Error; #[derive(Debug, Error, PartialEq, Eq)] pub enum ConstraintError { - #[error("value `{candidate}` is not in the allowed set {allowed}")] - InvalidValue { candidate: String, allowed: String }, + #[error( + "invalid value for `{field_name}`: `{candidate}` is not in the allowed set {allowed} (set by {requirement_source})" + )] + InvalidValue { + field_name: &'static str, + candidate: String, + allowed: String, + requirement_source: RequirementSource, + }, #[error("field `{field_name}` cannot be empty")] EmptyField { field_name: String }, } impl ConstraintError { - pub fn invalid_value(candidate: impl Into, allowed: impl Into) -> Self { - Self::InvalidValue { - candidate: candidate.into(), - allowed: allowed.into(), - } - } - pub fn empty_field(field_name: impl Into) -> Self { Self::EmptyField { field_name: field_name.into(), @@ -63,24 +64,6 @@ 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 @@ -89,22 +72,6 @@ impl Constrained { Self::allow_any(T::default()) } - pub fn allow_values(initial_value: T, allowed: Vec) -> ConstraintResult - where - T: PartialEq + Send + Sync + fmt::Debug + 'static, - { - Self::new(initial_value, move |candidate| { - if allowed.contains(candidate) { - Ok(()) - } else { - Err(ConstraintError::invalid_value( - format!("{candidate:?}"), - format!("{allowed:?}"), - )) - } - }) - } - pub fn get(&self) -> &T { &self.value } @@ -154,6 +121,15 @@ mod tests { use super::*; use pretty_assertions::assert_eq; + fn invalid_value(candidate: impl Into, allowed: impl Into) -> ConstraintError { + ConstraintError::InvalidValue { + field_name: "", + candidate: candidate.into(), + allowed: allowed.into(), + requirement_source: RequirementSource::Unknown, + } + } + #[test] fn constrained_allow_any_accepts_any_value() { let mut constrained = Constrained::allow_any(5); @@ -173,17 +149,11 @@ mod tests { if *value > 0 { Ok(()) } else { - Err(ConstraintError::invalid_value( - value.to_string(), - "positive values", - )) + Err(invalid_value(value.to_string(), "positive values")) } }); - assert_eq!( - result, - Err(ConstraintError::invalid_value("0", "positive values")) - ); + assert_eq!(result, Err(invalid_value("0", "positive values"))); } #[test] @@ -192,10 +162,7 @@ mod tests { if *value > 0 { Ok(()) } else { - Err(ConstraintError::invalid_value( - value.to_string(), - "positive values", - )) + Err(invalid_value(value.to_string(), "positive values")) } }) .expect("initial value should be accepted"); @@ -203,7 +170,7 @@ mod tests { let err = constrained .set(-5) .expect_err("negative values should be rejected"); - assert_eq!(err, ConstraintError::invalid_value("-5", "positive values")); + assert_eq!(err, invalid_value("-5", "positive values")); assert_eq!(constrained.value(), 1); } @@ -213,10 +180,7 @@ mod tests { if *value > 0 { Ok(()) } else { - Err(ConstraintError::invalid_value( - value.to_string(), - "positive values", - )) + Err(invalid_value(value.to_string(), "positive values")) } }) .expect("initial value should be accepted"); @@ -227,7 +191,7 @@ mod tests { let err = constrained .can_set(&-1) .expect_err("can_set should reject negative value"); - assert_eq!(err, ConstraintError::invalid_value("-1", "positive values")); + assert_eq!(err, invalid_value("-1", "positive values")); assert_eq!(constrained.value(), 1); } } diff --git a/codex-rs/core/src/config_loader/config_requirements.rs b/codex-rs/core/src/config_loader/config_requirements.rs index efbf9d61e..dd001e417 100644 --- a/codex-rs/core/src/config_loader/config_requirements.rs +++ b/codex-rs/core/src/config_loader/config_requirements.rs @@ -1,11 +1,42 @@ use codex_protocol::config_types::SandboxMode; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; +use std::fmt; use crate::config::Constrained; use crate::config::ConstraintError; +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RequirementSource { + Unknown, + MdmManagedPreferences { domain: String, key: String }, + SystemRequirementsToml { file: AbsolutePathBuf }, + LegacyManagedConfigTomlFromFile { file: AbsolutePathBuf }, + LegacyManagedConfigTomlFromMdm, +} + +impl fmt::Display for RequirementSource { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + RequirementSource::Unknown => write!(f, ""), + RequirementSource::MdmManagedPreferences { domain, key } => { + write!(f, "MDM {domain}:{key}") + } + RequirementSource::SystemRequirementsToml { file } => { + write!(f, "{}", file.as_path().display()) + } + RequirementSource::LegacyManagedConfigTomlFromFile { file } => { + write!(f, "{}", file.as_path().display()) + } + RequirementSource::LegacyManagedConfigTomlFromMdm => { + write!(f, "MDM managed_config.toml (legacy)") + } + } + } +} + /// Normalized version of [`ConfigRequirementsToml`] after deserialization and /// normalization. #[derive(Debug, Clone, PartialEq)] @@ -30,6 +61,75 @@ pub struct ConfigRequirementsToml { pub allowed_sandbox_modes: Option>, } +/// Value paired with the requirement source it came from, for better error +/// messages. +#[derive(Debug, Clone, PartialEq)] +pub struct Sourced { + pub value: T, + pub source: RequirementSource, +} + +impl Sourced { + pub fn new(value: T, source: RequirementSource) -> Self { + Self { value, source } + } +} + +impl std::ops::Deref for Sourced { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.value + } +} + +#[derive(Debug, Clone, Default, PartialEq)] +pub struct ConfigRequirementsWithSources { + pub allowed_approval_policies: Option>>, + pub allowed_sandbox_modes: Option>>, +} + +impl ConfigRequirementsWithSources { + pub fn merge_unset_fields(&mut self, source: RequirementSource, other: ConfigRequirementsToml) { + // For every field in `other` that is `Some`, if the corresponding field + // in `self` is `None`, copy the value from `other` into `self`. + macro_rules! fill_missing_take { + ($base:expr, $other:expr, $source:expr, { $($field:ident),+ $(,)? }) => { + // Destructure without `..` so adding fields to `ConfigRequirementsToml` + // forces this merge logic to be updated. + let ConfigRequirementsToml { $($field: _,)+ } = &$other; + + $( + if $base.$field.is_none() + && let Some(value) = $other.$field.take() + { + $base.$field = Some(Sourced::new(value, $source.clone())); + } + )+ + }; + } + + let mut other = other; + fill_missing_take!( + self, + other, + source, + { allowed_approval_policies, allowed_sandbox_modes } + ); + } + + pub fn into_toml(self) -> ConfigRequirementsToml { + let ConfigRequirementsWithSources { + allowed_approval_policies, + allowed_sandbox_modes, + } = self; + ConfigRequirementsToml { + allowed_approval_policies: allowed_approval_policies.map(|sourced| sourced.value), + allowed_sandbox_modes: allowed_sandbox_modes.map(|sourced| sourced.value), + } + } +} + /// Currently, `external-sandbox` is not supported in config.toml, but it is /// supported through programmatic use. #[derive(Deserialize, Debug, Clone, Copy, PartialEq)] @@ -61,41 +161,38 @@ impl ConfigRequirementsToml { pub fn is_empty(&self) -> bool { self.allowed_approval_policies.is_none() && self.allowed_sandbox_modes.is_none() } - - /// 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, allowed_sandbox_modes }); - } } -impl TryFrom for ConfigRequirements { +impl TryFrom for ConfigRequirements { type Error = ConstraintError; - fn try_from(toml: ConfigRequirementsToml) -> Result { - let ConfigRequirementsToml { + fn try_from(toml: ConfigRequirementsWithSources) -> Result { + let ConfigRequirementsWithSources { allowed_approval_policies, allowed_sandbox_modes, } = toml; + let approval_policy: Constrained = match allowed_approval_policies { - Some(policies) => { - if let Some(first) = policies.first() { - Constrained::allow_values(*first, policies)? - } else { + Some(Sourced { + value: policies, + source: requirement_source, + }) => { + let Some(initial_value) = policies.first().copied() else { return Err(ConstraintError::empty_field("allowed_approval_policies")); - } + }; + + Constrained::new(initial_value, move |candidate| { + if policies.contains(candidate) { + Ok(()) + } else { + Err(ConstraintError::InvalidValue { + field_name: "approval_policy", + candidate: format!("{candidate:?}"), + allowed: format!("{policies:?}"), + requirement_source: requirement_source.clone(), + }) + } + })? } None => Constrained::allow_any_from_default(), }; @@ -109,12 +206,17 @@ impl TryFrom for ConfigRequirements { // format to allow specifying those parameters. let default_sandbox_policy = SandboxPolicy::ReadOnly; let sandbox_policy: Constrained = match allowed_sandbox_modes { - Some(modes) => { + Some(Sourced { + value: modes, + source: requirement_source, + }) => { if !modes.contains(&SandboxModeRequirement::ReadOnly) { - return Err(ConstraintError::invalid_value( - "allowed_sandbox_modes", - "must include 'read-only' to allow any SandboxPolicy", - )); + return Err(ConstraintError::InvalidValue { + field_name: "allowed_sandbox_modes", + candidate: format!("{modes:?}"), + allowed: "must include 'read-only' to allow any SandboxPolicy".to_string(), + requirement_source, + }); }; Constrained::new(default_sandbox_policy, move |candidate| { @@ -131,10 +233,12 @@ impl TryFrom for ConfigRequirements { if modes.contains(&mode) { Ok(()) } else { - Err(ConstraintError::invalid_value( - format!("{candidate:?}"), - format!("{modes:?}"), - )) + Err(ConstraintError::InvalidValue { + field_name: "sandbox_mode", + candidate: format!("{mode:?}"), + allowed: format!("{modes:?}"), + requirement_source: requirement_source.clone(), + }) } })? } @@ -156,45 +260,168 @@ mod tests { use pretty_assertions::assert_eq; use toml::from_str; + fn with_unknown_source(toml: ConfigRequirementsToml) -> ConfigRequirementsWithSources { + let ConfigRequirementsToml { + allowed_approval_policies, + allowed_sandbox_modes, + } = toml; + ConfigRequirementsWithSources { + allowed_approval_policies: allowed_approval_policies + .map(|value| Sourced::new(value, RequirementSource::Unknown)), + allowed_sandbox_modes: allowed_sandbox_modes + .map(|value| Sourced::new(value, RequirementSource::Unknown)), + } + } + #[test] - fn merge_unset_fields_only_fills_missing_values() -> Result<()> { + fn merge_unset_fields_copies_every_field_and_sets_sources() { + let mut target = ConfigRequirementsWithSources::default(); + let source = RequirementSource::LegacyManagedConfigTomlFromMdm; + + let allowed_approval_policies = vec![AskForApproval::UnlessTrusted, AskForApproval::Never]; + let allowed_sandbox_modes = vec![ + SandboxModeRequirement::WorkspaceWrite, + SandboxModeRequirement::DangerFullAccess, + ]; + + // Intentionally constructed without `..Default::default()` so adding a new field to + // `ConfigRequirementsToml` forces this test to be updated. + let other = ConfigRequirementsToml { + allowed_approval_policies: Some(allowed_approval_policies.clone()), + allowed_sandbox_modes: Some(allowed_sandbox_modes.clone()), + }; + + target.merge_unset_fields(source.clone(), other); + + assert_eq!( + target, + ConfigRequirementsWithSources { + allowed_approval_policies: Some(Sourced::new( + allowed_approval_policies, + source.clone() + )), + allowed_sandbox_modes: Some(Sourced::new(allowed_sandbox_modes, source)), + } + ); + } + + #[test] + fn merge_unset_fields_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 source_location = RequirementSource::MdmManagedPreferences { + domain: "com.codex".to_string(), + key: "allowed_approval_policies".to_string(), + }; - let mut populated_target: ConfigRequirementsToml = from_str( + let mut empty_target = ConfigRequirementsWithSources::default(); + empty_target.merge_unset_fields(source_location.clone(), source); + assert_eq!( + empty_target, + ConfigRequirementsWithSources { + allowed_approval_policies: Some(Sourced::new( + vec![AskForApproval::OnRequest], + source_location, + )), + allowed_sandbox_modes: None, + } + ); + Ok(()) + } + + #[test] + fn merge_unset_fields_does_not_overwrite_existing_values() -> Result<()> { + let existing_source = RequirementSource::LegacyManagedConfigTomlFromMdm; + let mut populated_target = ConfigRequirementsWithSources::default(); + let populated_requirements: ConfigRequirementsToml = from_str( r#" allowed_approval_policies = ["never"] "#, )?; - populated_target.merge_unset_fields(source); + populated_target.merge_unset_fields(existing_source.clone(), populated_requirements); + + let source: ConfigRequirementsToml = from_str( + r#" + allowed_approval_policies = ["on-request"] + "#, + )?; + let source_location = RequirementSource::MdmManagedPreferences { + domain: "com.codex".to_string(), + key: "allowed_approval_policies".to_string(), + }; + populated_target.merge_unset_fields(source_location, source); + assert_eq!( - populated_target.allowed_approval_policies, - Some(vec![AskForApproval::Never]) + populated_target, + ConfigRequirementsWithSources { + allowed_approval_policies: Some(Sourced::new( + vec![AskForApproval::Never], + existing_source, + )), + allowed_sandbox_modes: None, + } ); Ok(()) } + #[test] + fn constraint_error_includes_requirement_source() -> Result<()> { + let source: ConfigRequirementsToml = from_str( + r#" + allowed_approval_policies = ["on-request"] + allowed_sandbox_modes = ["read-only"] + "#, + )?; + + let requirements_toml_file = if cfg!(windows) { + "C:\\etc\\codex\\requirements.toml" + } else { + "/etc/codex/requirements.toml" + }; + let requirements_toml_file = AbsolutePathBuf::from_absolute_path(requirements_toml_file)?; + let source_location = RequirementSource::SystemRequirementsToml { + file: requirements_toml_file, + }; + + let mut target = ConfigRequirementsWithSources::default(); + target.merge_unset_fields(source_location.clone(), source); + let requirements = ConfigRequirements::try_from(target)?; + + assert_eq!( + requirements.approval_policy.can_set(&AskForApproval::Never), + Err(ConstraintError::InvalidValue { + field_name: "approval_policy", + candidate: "Never".into(), + allowed: "[OnRequest]".into(), + requirement_source: source_location.clone(), + }) + ); + assert_eq!( + requirements + .sandbox_policy + .can_set(&SandboxPolicy::DangerFullAccess), + Err(ConstraintError::InvalidValue { + field_name: "sandbox_mode", + candidate: "DangerFullAccess".into(), + allowed: "[ReadOnly]".into(), + requirement_source: source_location, + }) + ); + + Ok(()) + } + #[test] fn deserialize_allowed_approval_policies() -> Result<()> { let toml_str = r#" allowed_approval_policies = ["untrusted", "on-request"] "#; let config: ConfigRequirementsToml = from_str(toml_str)?; - let requirements = ConfigRequirements::try_from(config)?; + let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; assert_eq!( requirements.approval_policy.value(), @@ -212,8 +439,10 @@ mod tests { .approval_policy .can_set(&AskForApproval::OnFailure), Err(ConstraintError::InvalidValue { + field_name: "approval_policy", candidate: "OnFailure".into(), allowed: "[UnlessTrusted, OnRequest]".into(), + requirement_source: RequirementSource::Unknown, }) ); assert!( @@ -225,8 +454,10 @@ mod tests { assert_eq!( requirements.approval_policy.can_set(&AskForApproval::Never), Err(ConstraintError::InvalidValue { + field_name: "approval_policy", candidate: "Never".into(), allowed: "[UnlessTrusted, OnRequest]".into(), + requirement_source: RequirementSource::Unknown, }) ); assert!( @@ -245,7 +476,7 @@ mod tests { allowed_sandbox_modes = ["read-only", "workspace-write"] "#; let config: ConfigRequirementsToml = from_str(toml_str)?; - let requirements = ConfigRequirements::try_from(config)?; + let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; let root = if cfg!(windows) { "C:\\repo" } else { "/repo" }; assert!( @@ -270,8 +501,10 @@ mod tests { .sandbox_policy .can_set(&SandboxPolicy::DangerFullAccess), Err(ConstraintError::InvalidValue { + field_name: "sandbox_mode", candidate: "DangerFullAccess".into(), allowed: "[ReadOnly, WorkspaceWrite]".into(), + requirement_source: RequirementSource::Unknown, }) ); assert_eq!( @@ -281,8 +514,10 @@ mod tests { network_access: NetworkAccess::Restricted, }), Err(ConstraintError::InvalidValue { - candidate: "ExternalSandbox { network_access: Restricted }".into(), + field_name: "sandbox_mode", + candidate: "ExternalSandbox".into(), allowed: "[ReadOnly, WorkspaceWrite]".into(), + requirement_source: RequirementSource::Unknown, }) ); diff --git a/codex-rs/core/src/config_loader/macos.rs b/codex-rs/core/src/config_loader/macos.rs index 8d2289e91..e3bbcc07e 100644 --- a/codex-rs/core/src/config_loader/macos.rs +++ b/codex-rs/core/src/config_loader/macos.rs @@ -1,4 +1,6 @@ use super::config_requirements::ConfigRequirementsToml; +use super::config_requirements::ConfigRequirementsWithSources; +use super::config_requirements::RequirementSource; use base64::Engine; use base64::prelude::BASE64_STANDARD; use core_foundation::base::TCFType; @@ -13,6 +15,13 @@ const MANAGED_PREFERENCES_APPLICATION_ID: &str = "com.openai.codex"; const MANAGED_PREFERENCES_CONFIG_KEY: &str = "config_toml_base64"; const MANAGED_PREFERENCES_REQUIREMENTS_KEY: &str = "requirements_toml_base64"; +pub(super) fn managed_preferences_requirements_source() -> RequirementSource { + RequirementSource::MdmManagedPreferences { + domain: MANAGED_PREFERENCES_APPLICATION_ID.to_string(), + key: MANAGED_PREFERENCES_REQUIREMENTS_KEY.to_string(), + } +} + pub(crate) async fn load_managed_admin_config_layer( override_base64: Option<&str>, ) -> io::Result> { @@ -47,21 +56,26 @@ fn load_managed_admin_config() -> io::Result> { } pub(crate) async fn load_managed_admin_requirements_toml( - target: &mut ConfigRequirementsToml, + target: &mut ConfigRequirementsWithSources, override_base64: Option<&str>, ) -> io::Result<()> { if let Some(encoded) = override_base64 { let trimmed = encoded.trim(); - if !trimmed.is_empty() { - target.merge_unset_fields(parse_managed_requirements_base64(trimmed)?); + if trimmed.is_empty() { + return Ok(()); } + + target.merge_unset_fields( + managed_preferences_requirements_source(), + parse_managed_requirements_base64(trimmed)?, + ); return Ok(()); } match task::spawn_blocking(load_managed_admin_requirements).await { Ok(result) => { if let Some(requirements) = result? { - target.merge_unset_fields(requirements); + target.merge_unset_fields(managed_preferences_requirements_source(), requirements); } Ok(()) } diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index bb995fda2..9cc437d1f 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -12,6 +12,7 @@ mod tests; use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigToml; +use crate::config_loader::config_requirements::ConfigRequirementsWithSources; use crate::config_loader::layer_io::LoadedConfigLayers; use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::config_types::SandboxMode; @@ -25,6 +26,7 @@ use toml::Value as TomlValue; pub use config_requirements::ConfigRequirements; pub use config_requirements::ConfigRequirementsToml; +pub use config_requirements::RequirementSource; pub use config_requirements::SandboxModeRequirement; pub use merge::merge_toml_values; pub use state::ConfigLayerEntry; @@ -77,7 +79,7 @@ pub async fn load_config_layers_state( cli_overrides: &[(String, TomlValue)], overrides: LoaderOverrides, ) -> io::Result { - let mut config_requirements_toml = ConfigRequirementsToml::default(); + let mut config_requirements_toml = ConfigRequirementsWithSources::default(); #[cfg(target_os = "macos")] macos::load_managed_admin_requirements_toml( @@ -202,9 +204,11 @@ pub async fn load_config_layers_state( )); } - let requirements_toml = config_requirements_toml.clone(); - let requirements = config_requirements_toml.try_into()?; - ConfigLayerStack::new(layers, requirements, requirements_toml) + ConfigLayerStack::new( + layers, + config_requirements_toml.clone().try_into()?, + config_requirements_toml.into_toml(), + ) } /// Attempts to load a config.toml file from `config_toml`. @@ -256,9 +260,11 @@ async fn load_config_toml_for_required_layer( /// 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, + config_requirements_toml: &mut ConfigRequirementsWithSources, requirements_toml_file: impl AsRef, ) -> io::Result<()> { + let requirements_toml_file = + AbsolutePathBuf::from_absolute_path(requirements_toml_file.as_ref())?; match tokio::fs::read_to_string(&requirements_toml_file).await { Ok(contents) => { let requirements_config: ConfigRequirementsToml = @@ -271,7 +277,12 @@ async fn load_requirements_toml( ), ) })?; - config_requirements_toml.merge_unset_fields(requirements_config); + config_requirements_toml.merge_unset_fields( + RequirementSource::SystemRequirementsToml { + file: requirements_toml_file.clone(), + }, + requirements_config, + ); } Err(e) => { if e.kind() != io::ErrorKind::NotFound { @@ -290,7 +301,7 @@ async fn load_requirements_toml( } async fn load_requirements_from_legacy_scheme( - config_requirements_toml: &mut ConfigRequirementsToml, + config_requirements_toml: &mut ConfigRequirementsWithSources, loaded_config_layers: LoadedConfigLayers, ) -> io::Result<()> { // In this implementation, earlier layers cannot be overwritten by later @@ -300,12 +311,16 @@ async fn load_requirements_from_legacy_scheme( managed_config, managed_config_from_mdm, } = loaded_config_layers; - for config in [ - managed_config_from_mdm, - managed_config.map(|c| c.managed_config), - ] - .into_iter() - .flatten() + + for (source, config) in managed_config_from_mdm + .map(|config| (RequirementSource::LegacyManagedConfigTomlFromMdm, config)) + .into_iter() + .chain(managed_config.map(|c| { + ( + RequirementSource::LegacyManagedConfigTomlFromFile { file: c.file }, + c.managed_config, + ) + })) { let legacy_config: LegacyManagedConfigToml = config.try_into().map_err(|err: toml::de::Error| { @@ -316,7 +331,7 @@ async fn load_requirements_from_legacy_scheme( })?; let new_requirements_toml = ConfigRequirementsToml::from(legacy_config); - config_requirements_toml.merge_unset_fields(new_requirements_toml); + config_requirements_toml.merge_unset_fields(source, new_requirements_toml); } Ok(()) diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index b80f00c71..3738f95b2 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -5,7 +5,7 @@ use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigRequirements; -use crate::config_loader::config_requirements::ConfigRequirementsToml; +use crate::config_loader::config_requirements::ConfigRequirementsWithSources; use crate::config_loader::fingerprint::version_for_toml; use crate::config_loader::load_requirements_toml; use codex_protocol::protocol::AskForApproval; @@ -315,11 +315,14 @@ allowed_approval_policies = ["never", "on-request"] ) .await?; - let mut config_requirements_toml = ConfigRequirementsToml::default(); + let mut config_requirements_toml = ConfigRequirementsWithSources::default(); load_requirements_toml(&mut config_requirements_toml, &requirements_file).await?; assert_eq!( - config_requirements_toml.allowed_approval_policies, + config_requirements_toml + .allowed_approval_policies + .as_deref() + .cloned(), Some(vec![AskForApproval::Never, AskForApproval::OnRequest]) ); diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index cf53b7fac..edfb4e1d4 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -11,6 +11,7 @@ use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config::Constrained; use codex_core::config::ConstraintError; +use codex_core::config_loader::RequirementSource; use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; use codex_core::protocol::AgentMessageDeltaEvent; @@ -85,6 +86,15 @@ async fn test_config() -> Config { .expect("config") } +fn invalid_value(candidate: impl Into, allowed: impl Into) -> ConstraintError { + ConstraintError::InvalidValue { + field_name: "", + candidate: candidate.into(), + allowed: allowed.into(), + requirement_source: RequirementSource::Unknown, + } +} + fn snapshot(percent: f64) -> RateLimitSnapshot { RateLimitSnapshot { primary: Some(RateLimitWindow { @@ -2303,7 +2313,7 @@ async fn approvals_popup_shows_disabled_presets() { chat.config.approval_policy = Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { AskForApproval::OnRequest => Ok(()), - _ => Err(ConstraintError::invalid_value( + _ => Err(invalid_value( candidate.to_string(), "this message should be printed in the description", )), @@ -2339,10 +2349,7 @@ async fn approvals_popup_navigation_skips_disabled() { chat.config.approval_policy = Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { AskForApproval::OnRequest => Ok(()), - _ => Err(ConstraintError::invalid_value( - candidate.to_string(), - "[on-request]", - )), + _ => Err(invalid_value(candidate.to_string(), "[on-request]")), }) .expect("construct constrained approval policy"); chat.open_approvals_popup();