From ded559680df03ebf944d01d8fd19be1debb51d04 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 6 Apr 2026 11:11:44 -0700 Subject: [PATCH] feat(requirements): support allowed_approval_reviewers (#16701) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Add requirements.toml support for `allowed_approvals_reviewers = ["user", "guardian_subagent"]`, so admins can now restrict the use of guardian mode. Note: If a user sets a reviewer that isn’t allowed by requirements.toml, config loading falls back to the first allowed reviewer and emits a startup warning. The table below describes the possible admin controls. | Admin intent | `requirements.toml` | User `config.toml` | End result | |---|---|---|---| | Leave Guardian optional | omit `allowed_approvals_reviewers` or set `["user", "guardian_subagent"]` | user chooses `approvals_reviewer = "user"` or `"guardian_subagent"` | Guardian off for `user`, on for `guardian_subagent` + `approval_policy = "on-request"` | | Force Guardian off | `allowed_approvals_reviewers = ["user"]` | any user value | Effective reviewer is `user`; Guardian off | | Force Guardian on | `allowed_approvals_reviewers = ["guardian_subagent"]` and usually `allowed_approval_policies = ["on-request"]` | any user reviewer value; user should also have `approval_policy = "on-request"` unless policy is forced | Effective reviewer is `guardian_subagent`; Guardian on when effective approval policy is `on-request` | | Allow both, but default to manual if user does nothing | `allowed_approvals_reviewers = ["user", "guardian_subagent"]` | omit `approvals_reviewer` | Effective reviewer is `user`; Guardian off | | Allow both, and user explicitly opts into Guardian | `allowed_approvals_reviewers = ["user", "guardian_subagent"]` | `approvals_reviewer = "guardian_subagent"` and `approval_policy = "on-request"` | Guardian on | | Invalid admin config | `allowed_approvals_reviewers = []` | anything | Config load error | --- .../v2/ConfigRequirementsReadResponse.json | 8 + .../app-server-protocol/src/protocol/v2.rs | 3 + codex-rs/app-server/src/config_api.rs | 20 ++ codex-rs/cloud-requirements/src/lib.rs | 16 ++ codex-rs/config/src/config_requirements.rs | 134 ++++++++++++ codex-rs/config/src/state.rs | 25 +++ codex-rs/core/src/agent/control_tests.rs | 9 +- codex-rs/core/src/codex_tests.rs | 2 +- codex-rs/core/src/config/config_tests.rs | 198 ++++++++++++++---- codex-rs/core/src/config/mod.rs | 28 ++- codex-rs/core/src/config/service.rs | 10 + codex-rs/core/src/config/service_tests.rs | 87 +++----- codex-rs/core/src/config_loader/mod.rs | 23 ++ codex-rs/core/src/config_loader/tests.rs | 130 +++++------- codex-rs/core/src/exec_policy_tests.rs | 9 +- codex-rs/tui/src/debug_config.rs | 2 + 16 files changed, 513 insertions(+), 191 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json index de5eca1d3..614575a95 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json @@ -1,6 +1,14 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "ApprovalsReviewer": { + "description": "Configures who approval requests are routed to for review. Examples include sandbox escapes, blocked network access, MCP approval prompts, and ARC escalations. Defaults to `user`. `guardian_subagent` uses a carefully prompted subagent to gather relevant context and apply a risk-based decision framework before approving or denying the request.", + "enum": [ + "user", + "guardian_subagent" + ], + "type": "string" + }, "AskForApproval": { "oneOf": [ { diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 82b568515..cdc78647a 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -850,6 +850,8 @@ pub struct ConfigReadResponse { pub struct ConfigRequirements { #[experimental(nested)] pub allowed_approval_policies: Option>, + #[experimental("configRequirements/read.allowedApprovalsReviewers")] + pub allowed_approvals_reviewers: Option>, pub allowed_sandbox_modes: Option>, pub allowed_web_search_modes: Option>, pub feature_requirements: Option>, @@ -7315,6 +7317,7 @@ mod tests { request_permissions: false, mcp_elicitations: false, }]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index c36d406d8..ec2cf82cc 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -366,6 +366,12 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR .map(codex_app_server_protocol::AskForApproval::from) .collect() }), + allowed_approvals_reviewers: requirements.allowed_approvals_reviewers.map(|reviewers| { + reviewers + .into_iter() + .map(codex_app_server_protocol::ApprovalsReviewer::from) + .collect() + }), allowed_sandbox_modes: requirements.allowed_sandbox_modes.map(|modes| { modes .into_iter() @@ -519,6 +525,7 @@ mod tests { use codex_features::Feature; use codex_login::AuthManager; use codex_login::CodexAuth; + use codex_protocol::config_types::ApprovalsReviewer as CoreApprovalsReviewer; use codex_protocol::protocol::AskForApproval as CoreAskForApproval; use pretty_assertions::assert_eq; use serde_json::json; @@ -545,6 +552,10 @@ mod tests { CoreAskForApproval::Never, CoreAskForApproval::OnRequest, ]), + allowed_approvals_reviewers: Some(vec![ + CoreApprovalsReviewer::User, + CoreApprovalsReviewer::GuardianSubagent, + ]), allowed_sandbox_modes: Some(vec![ CoreSandboxModeRequirement::ReadOnly, CoreSandboxModeRequirement::ExternalSandbox, @@ -602,6 +613,13 @@ mod tests { codex_app_server_protocol::AskForApproval::OnRequest, ]) ); + assert_eq!( + mapped.allowed_approvals_reviewers, + Some(vec![ + codex_app_server_protocol::ApprovalsReviewer::User, + codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent, + ]) + ); assert_eq!( mapped.allowed_sandbox_modes, Some(vec![SandboxMode::ReadOnly]), @@ -651,6 +669,7 @@ mod tests { fn map_requirements_toml_to_api_omits_unix_socket_none_entries_from_legacy_network_fields() { let requirements = ConfigRequirementsToml { allowed_approval_policies: None, + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -707,6 +726,7 @@ mod tests { fn map_requirements_toml_to_api_normalizes_allowed_web_search_modes() { let requirements = ConfigRequirementsToml { allowed_approval_policies: None, + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: Some(Vec::new()), guardian_developer_instructions: None, diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index 1befb936c..f2916c2e9 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -1154,6 +1154,7 @@ mod tests { service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1182,6 +1183,7 @@ mod tests { service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1210,6 +1212,7 @@ mod tests { service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1255,6 +1258,7 @@ mod tests { result, Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1336,6 +1340,7 @@ enabled = false handle.await.expect("cloud requirements task"), Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1407,6 +1412,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1476,6 +1482,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1639,6 +1646,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1673,6 +1681,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1727,6 +1736,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::OnRequest]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1776,6 +1786,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::OnRequest]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1829,6 +1840,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1883,6 +1895,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -1937,6 +1950,7 @@ enabled = false .and_then(|contents| parse_cloud_requirements(contents).ok().flatten()), Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -2024,6 +2038,7 @@ enabled = false service.fetch().await, Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, @@ -2050,6 +2065,7 @@ enabled = false .and_then(|contents| parse_cloud_requirements(contents).ok().flatten()), Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::OnRequest]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, guardian_developer_instructions: None, diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index d63fa1e8b..7f6b5bc1e 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -1,3 +1,4 @@ +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::WebSearchMode; use codex_protocol::protocol::AskForApproval; @@ -78,6 +79,7 @@ impl std::ops::DerefMut for ConstrainedWithSource { #[derive(Debug, Clone, PartialEq)] pub struct ConfigRequirements { pub approval_policy: ConstrainedWithSource, + pub approvals_reviewer: ConstrainedWithSource, pub sandbox_policy: ConstrainedWithSource, pub web_search_mode: ConstrainedWithSource, pub feature_requirements: Option>, @@ -95,6 +97,10 @@ impl Default for ConfigRequirements { Constrained::allow_any_from_default(), /*source*/ None, ), + approvals_reviewer: ConstrainedWithSource::new( + Constrained::allow_any_from_default(), + /*source*/ None, + ), sandbox_policy: ConstrainedWithSource::new( Constrained::allow_any(SandboxPolicy::new_read_only_policy()), /*source*/ None, @@ -487,6 +493,7 @@ pub(crate) fn merge_enablement_settings_descending( #[derive(Deserialize, Debug, Clone, Default, PartialEq)] pub struct ConfigRequirementsToml { pub allowed_approval_policies: Option>, + pub allowed_approvals_reviewers: Option>, pub allowed_sandbox_modes: Option>, pub allowed_web_search_modes: Option>, #[serde(rename = "features", alias = "feature_requirements")] @@ -525,6 +532,7 @@ impl std::ops::Deref for Sourced { #[derive(Debug, Clone, Default, PartialEq)] pub struct ConfigRequirementsWithSources { pub allowed_approval_policies: Option>>, + pub allowed_approvals_reviewers: Option>>, pub allowed_sandbox_modes: Option>>, pub allowed_web_search_modes: Option>>, pub feature_requirements: Option>, @@ -556,6 +564,7 @@ impl ConfigRequirementsWithSources { // forces this merge logic to be updated. let ConfigRequirementsToml { allowed_approval_policies: _, + allowed_approvals_reviewers: _, allowed_sandbox_modes: _, allowed_web_search_modes: _, feature_requirements: _, @@ -581,6 +590,7 @@ impl ConfigRequirementsWithSources { source, { allowed_approval_policies, + allowed_approvals_reviewers, allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, @@ -604,6 +614,7 @@ impl ConfigRequirementsWithSources { pub fn into_toml(self) -> ConfigRequirementsToml { let ConfigRequirementsWithSources { allowed_approval_policies, + allowed_approvals_reviewers, allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, @@ -616,6 +627,7 @@ impl ConfigRequirementsWithSources { } = self; ConfigRequirementsToml { allowed_approval_policies: allowed_approval_policies.map(|sourced| sourced.value), + allowed_approvals_reviewers: allowed_approvals_reviewers.map(|sourced| sourced.value), allowed_sandbox_modes: allowed_sandbox_modes.map(|sourced| sourced.value), allowed_web_search_modes: allowed_web_search_modes.map(|sourced| sourced.value), feature_requirements: feature_requirements.map(|sourced| sourced.value), @@ -666,6 +678,7 @@ pub enum ResidencyRequirement { impl ConfigRequirementsToml { pub fn is_empty(&self) -> bool { self.allowed_approval_policies.is_none() + && self.allowed_approvals_reviewers.is_none() && self.allowed_sandbox_modes.is_none() && self.allowed_web_search_modes.is_none() && self @@ -693,6 +706,7 @@ impl TryFrom for ConfigRequirements { fn try_from(toml: ConfigRequirementsWithSources) -> Result { let ConfigRequirementsWithSources { allowed_approval_policies, + allowed_approvals_reviewers, allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, @@ -734,6 +748,36 @@ impl TryFrom for ConfigRequirements { ), }; + let approvals_reviewer = match allowed_approvals_reviewers { + Some(Sourced { + value: reviewers, + source: requirement_source, + }) => { + let Some(initial_value) = reviewers.first().copied() else { + return Err(ConstraintError::empty_field("allowed_approvals_reviewers")); + }; + + let requirement_source_for_error = requirement_source.clone(); + let constrained = Constrained::new(initial_value, move |candidate| { + if reviewers.contains(candidate) { + Ok(()) + } else { + Err(ConstraintError::InvalidValue { + field_name: "approvals_reviewer", + candidate: format!("{candidate:?}"), + allowed: format!("{reviewers:?}"), + requirement_source: requirement_source_for_error.clone(), + }) + } + })?; + ConstrainedWithSource::new(constrained, Some(requirement_source)) + } + None => ConstrainedWithSource::new( + Constrained::allow_any_from_default(), + /*source*/ None, + ), + }; + // TODO(gt): `ConfigRequirementsToml` should let the author specify the // default `SandboxPolicy`? Should do this for `AskForApproval` too? // @@ -878,6 +922,7 @@ impl TryFrom for ConfigRequirements { }); Ok(ConfigRequirements { approval_policy, + approvals_reviewer, sandbox_policy, web_search_mode, feature_requirements, @@ -914,6 +959,7 @@ mod tests { fn with_unknown_source(toml: ConfigRequirementsToml) -> ConfigRequirementsWithSources { let ConfigRequirementsToml { allowed_approval_policies, + allowed_approvals_reviewers, allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, @@ -927,6 +973,8 @@ mod tests { ConfigRequirementsWithSources { allowed_approval_policies: allowed_approval_policies .map(|value| Sourced::new(value, RequirementSource::Unknown)), + allowed_approvals_reviewers: allowed_approvals_reviewers + .map(|value| Sourced::new(value, RequirementSource::Unknown)), allowed_sandbox_modes: allowed_sandbox_modes .map(|value| Sourced::new(value, RequirementSource::Unknown)), allowed_web_search_modes: allowed_web_search_modes @@ -950,6 +998,8 @@ mod tests { let source = RequirementSource::LegacyManagedConfigTomlFromMdm; let allowed_approval_policies = vec![AskForApproval::UnlessTrusted, AskForApproval::Never]; + let allowed_approvals_reviewers = + vec![ApprovalsReviewer::GuardianSubagent, ApprovalsReviewer::User]; let allowed_sandbox_modes = vec![ SandboxModeRequirement::WorkspaceWrite, SandboxModeRequirement::DangerFullAccess, @@ -970,6 +1020,7 @@ mod tests { // `ConfigRequirementsToml` forces this test to be updated. let other = ConfigRequirementsToml { allowed_approval_policies: Some(allowed_approval_policies.clone()), + allowed_approvals_reviewers: Some(allowed_approvals_reviewers.clone()), allowed_sandbox_modes: Some(allowed_sandbox_modes.clone()), allowed_web_search_modes: Some(allowed_web_search_modes.clone()), feature_requirements: Some(feature_requirements.clone()), @@ -990,6 +1041,10 @@ mod tests { allowed_approval_policies, source.clone() )), + allowed_approvals_reviewers: Some(Sourced::new( + allowed_approvals_reviewers, + source.clone(), + )), allowed_sandbox_modes: Some(Sourced::new(allowed_sandbox_modes, source.clone(),)), allowed_web_search_modes: Some(Sourced::new( allowed_web_search_modes, @@ -1034,6 +1089,7 @@ mod tests { vec![AskForApproval::OnRequest], source_location, )), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, @@ -1077,6 +1133,7 @@ mod tests { vec![AskForApproval::Never], existing_source, )), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, @@ -1157,6 +1214,18 @@ guardian_developer_instructions = """ Ok(()) } + #[test] + fn allowed_approvals_reviewers_is_not_empty() -> Result<()> { + let requirements: ConfigRequirementsToml = from_str( + r#" +allowed_approvals_reviewers = ["user"] +"#, + )?; + + assert!(!requirements.is_empty()); + Ok(()) + } + #[test] fn deserialize_apps_requirements() -> Result<()> { let toml_str = r#" @@ -1330,6 +1399,7 @@ guardian_developer_instructions = """ let source: ConfigRequirementsToml = from_str( r#" allowed_approval_policies = ["on-request"] + allowed_approvals_reviewers = ["guardian_subagent"] allowed_sandbox_modes = ["read-only"] "#, )?; @@ -1360,6 +1430,17 @@ guardian_developer_instructions = """ field_name: "sandbox_mode", candidate: "DangerFullAccess".into(), allowed: "[ReadOnly]".into(), + requirement_source: source_location.clone(), + }) + ); + assert_eq!( + requirements + .approvals_reviewer + .can_set(&ApprovalsReviewer::User), + Err(ConstraintError::InvalidValue { + field_name: "approvals_reviewer", + candidate: "User".into(), + allowed: "[GuardianSubagent]".into(), requirement_source: source_location, }) ); @@ -1399,6 +1480,7 @@ guardian_developer_instructions = """ let source: ConfigRequirementsToml = from_str( r#" allowed_approval_policies = ["on-request"] + allowed_approvals_reviewers = ["guardian_subagent"] allowed_sandbox_modes = ["read-only"] allowed_web_search_modes = ["cached"] enforce_residency = "us" @@ -1416,6 +1498,10 @@ guardian_developer_instructions = """ requirements.approval_policy.source, Some(source_location.clone()) ); + assert_eq!( + requirements.approvals_reviewer.source, + Some(source_location.clone()) + ); assert_eq!( requirements.sandbox_policy.source, Some(source_location.clone()) @@ -1491,6 +1577,54 @@ guardian_developer_instructions = """ Ok(()) } + #[test] + fn deserialize_allowed_approvals_reviewers() -> Result<()> { + let toml_str = r#" + allowed_approvals_reviewers = ["guardian_subagent", "user"] + "#; + let config: ConfigRequirementsToml = from_str(toml_str)?; + let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; + + assert_eq!( + requirements.approvals_reviewer.value(), + ApprovalsReviewer::GuardianSubagent, + "currently, there is no way to specify the default value for approvals reviewer in the toml, so it picks the first allowed value" + ); + assert!( + requirements + .approvals_reviewer + .can_set(&ApprovalsReviewer::GuardianSubagent) + .is_ok() + ); + assert!( + requirements + .approvals_reviewer + .can_set(&ApprovalsReviewer::User) + .is_ok() + ); + + Ok(()) + } + + #[test] + fn empty_allowed_approvals_reviewers_is_rejected() -> Result<()> { + let toml_str = r#" + allowed_approvals_reviewers = [] + "#; + let config: ConfigRequirementsToml = from_str(toml_str)?; + let err = ConfigRequirements::try_from(with_unknown_source(config)) + .expect_err("empty approvals reviewer allow-list should be rejected"); + + assert_eq!( + err, + ConstraintError::EmptyField { + field_name: "allowed_approvals_reviewers".to_string(), + } + ); + + Ok(()) + } + #[test] fn deserialize_allowed_sandbox_modes() -> Result<()> { let toml_str = r#" diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index f6899651b..a34ccc6cd 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -23,6 +23,31 @@ pub struct LoaderOverrides { pub macos_managed_config_requirements_base64: Option, } +impl LoaderOverrides { + /// Returns overrides that ignore host-managed configuration. + /// + /// This is intended for tests that should load only repo-controlled config fixtures. + pub fn without_managed_config_for_tests() -> Self { + Self::with_managed_config_path_for_tests( + std::env::temp_dir() + .join("codex-config-tests") + .join("managed_config.toml"), + ) + } + + /// Returns overrides with host MDM disabled and managed config loaded from `managed_config_path`. + /// + /// This is intended for tests that supply an explicit managed config fixture. + pub fn with_managed_config_path_for_tests(managed_config_path: PathBuf) -> Self { + Self { + managed_config_path: Some(managed_config_path), + #[cfg(target_os = "macos")] + managed_preferences_base64: Some(String::new()), + macos_managed_config_requirements_base64: Some(String::new()), + } + } +} + #[derive(Debug, Clone, PartialEq)] pub struct ConfigLayerEntry { pub name: ConfigLayerSource, diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index b24cc977a..d254a4934 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -5,7 +5,6 @@ use crate::agent::agent_status_from_event; use crate::config::AgentRoleConfig; use crate::config::Config; use crate::config::ConfigBuilder; -use crate::config_loader::LoaderOverrides; use crate::contextual_user_message::SUBAGENT_NOTIFICATION_OPEN_TAG; use assert_matches::assert_matches; use chrono::Utc; @@ -36,15 +35,9 @@ async fn test_config_with_cli_overrides( cli_overrides: Vec<(String, TomlValue)>, ) -> (TempDir, Config) { let home = TempDir::new().expect("create temp dir"); - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(home.path().to_path_buf()) .cli_overrides(cli_overrides) - .loader_overrides(LoaderOverrides { - #[cfg(target_os = "macos")] - managed_preferences_base64: Some(String::new()), - macos_managed_config_requirements_base64: Some(String::new()), - ..LoaderOverrides::default() - }) .build() .await .expect("load default test config"); diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 274607095..85404d4d5 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2216,7 +2216,7 @@ fn text_block(s: &str) -> serde_json::Value { } async fn build_test_config(codex_home: &Path) -> Config { - ConfigBuilder::default() + ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.to_path_buf()) .build() .await diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 384458a69..13e60ecb7 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -1626,7 +1626,7 @@ profile = "project" "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { cwd: Some(workspace.path().to_path_buf()), @@ -1817,12 +1817,7 @@ async fn managed_config_overrides_oauth_store_mode() -> anyhow::Result<()> { std::fs::write(&config_path, "mcp_oauth_credentials_store = \"file\"\n")?; std::fs::write(&managed_path, "mcp_oauth_credentials_store = \"keyring\"\n")?; - let overrides = LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }; + let overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()); let cwd = codex_home.path().abs(); let config_layer_stack = load_config_layers_state( @@ -1947,12 +1942,7 @@ async fn managed_config_wins_over_cli_overrides() -> anyhow::Result<()> { )?; std::fs::write(&managed_path, "model = \"managed_config\"\n")?; - let overrides = LoaderOverrides { - managed_config_path: Some(managed_path), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }; + let overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path); let cwd = codex_home.path().abs(); let config_layer_stack = load_config_layers_state( @@ -3286,7 +3276,7 @@ nickname_candidates = ["Hypatia", "Noether"] ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -3340,7 +3330,7 @@ nickname_candidates = ["Noether"] ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -3403,7 +3393,7 @@ model = "gpt-5" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { cwd: Some(nested_cwd), @@ -3457,7 +3447,7 @@ config_file = "./agents/researcher.toml" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -3510,7 +3500,7 @@ description = "Review role" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -3572,7 +3562,7 @@ developer_instructions = "Review carefully" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { cwd: Some(nested_cwd), @@ -3627,7 +3617,7 @@ config_file = "./agents/researcher.toml" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -3679,7 +3669,7 @@ nickname_candidates = ["Atlas"] ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -3813,7 +3803,7 @@ developer_instructions = "Write carefully" "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { cwd: Some(nested_cwd), @@ -3937,7 +3927,7 @@ model = "gpt-5" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { cwd: Some(nested_cwd), @@ -4057,7 +4047,7 @@ model = "gpt-5-mini" ) .await?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { cwd: Some(nested_cwd), @@ -4957,6 +4947,7 @@ fn test_requirements_web_search_mode_allowlist_does_not_warn_when_unset() -> any let requirements_toml = crate::config_loader::ConfigRequirementsToml { allowed_approval_policies: None, + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: Some(vec![ crate::config_loader::WebSearchModeRequirement::Cached, @@ -5549,7 +5540,7 @@ async fn requirements_disallowing_default_sandbox_falls_back_to_required_default -> std::io::Result<()> { let codex_home = TempDir::new()?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .cloud_requirements(CloudRequirementsLoader::new(async { Ok(Some(crate::config_loader::ConfigRequirementsToml { @@ -5579,6 +5570,7 @@ async fn explicit_sandbox_mode_falls_back_when_disallowed_by_requirements() -> s let requirements = crate::config_loader::ConfigRequirementsToml { allowed_approval_policies: None, + allowed_approvals_reviewers: None, allowed_sandbox_modes: Some(vec![crate::config_loader::SandboxModeRequirement::ReadOnly]), allowed_web_search_modes: None, feature_requirements: None, @@ -5590,7 +5582,7 @@ async fn explicit_sandbox_mode_falls_back_when_disallowed_by_requirements() -> s guardian_developer_instructions: None, }; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async move { @@ -5615,7 +5607,7 @@ async fn requirements_web_search_mode_overrides_danger_full_access_default() -> "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async { @@ -5656,7 +5648,7 @@ trust_level = "untrusted" ), )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(workspace.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async { @@ -5685,7 +5677,7 @@ async fn explicit_approval_policy_falls_back_when_disallowed_by_requirements() - "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async { @@ -5707,7 +5699,7 @@ async fn explicit_approval_policy_falls_back_when_disallowed_by_requirements() - async fn feature_requirements_normalize_effective_feature_values() -> std::io::Result<()> { let codex_home = TempDir::new()?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .cloud_requirements(CloudRequirementsLoader::new(async { Ok(Some(crate::config_loader::ConfigRequirementsToml { @@ -5749,7 +5741,7 @@ shell_tool = true "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async { @@ -5785,7 +5777,7 @@ async fn approvals_reviewer_defaults_to_manual_only_without_guardian_feature() - { let codex_home = TempDir::new()?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -5835,7 +5827,7 @@ guardian_approval = true "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -5855,7 +5847,7 @@ async fn approvals_reviewer_can_be_set_in_config_without_guardian_approval() -> "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -5878,7 +5870,7 @@ approvals_reviewer = "guardian_subagent" "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -5891,6 +5883,138 @@ approvals_reviewer = "guardian_subagent" Ok(()) } +#[tokio::test] +async fn requirements_disallowing_default_approvals_reviewer_falls_back_to_required_default() +-> std::io::Result<()> { + let codex_home = TempDir::new()?; + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .cloud_requirements(CloudRequirementsLoader::new(async { + Ok(Some(crate::config_loader::ConfigRequirementsToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + ..Default::default() + })) + })) + .build() + .await?; + + assert_eq!( + config.approvals_reviewer, + ApprovalsReviewer::GuardianSubagent + ); + Ok(()) +} + +#[tokio::test] +async fn root_approvals_reviewer_falls_back_when_disallowed_by_requirements() -> std::io::Result<()> +{ + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"approvals_reviewer = "user" +"#, + )?; + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new(async { + Ok(Some(crate::config_loader::ConfigRequirementsToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + ..Default::default() + })) + })) + .build() + .await?; + + assert_eq!( + config.approvals_reviewer, + ApprovalsReviewer::GuardianSubagent + ); + assert!( + config.startup_warnings.iter().any(|warning| { + warning + .contains("Configured value for `approvals_reviewer` is disallowed by requirements") + }), + "{:?}", + config.startup_warnings + ); + Ok(()) +} + +#[tokio::test] +async fn profile_approvals_reviewer_falls_back_when_disallowed_by_requirements() +-> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"profile = "default" + +[profiles.default] +approvals_reviewer = "user" +"#, + )?; + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new(async { + Ok(Some(crate::config_loader::ConfigRequirementsToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + ..Default::default() + })) + })) + .build() + .await?; + + assert_eq!( + config.approvals_reviewer, + ApprovalsReviewer::GuardianSubagent + ); + Ok(()) +} + +#[tokio::test] +async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirements() +-> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"approvals_reviewer = "guardian_subagent" +"#, + )?; + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new(async { + Ok(Some(crate::config_loader::ConfigRequirementsToml { + allowed_approvals_reviewers: Some(vec![ + ApprovalsReviewer::User, + ApprovalsReviewer::GuardianSubagent, + ]), + ..Default::default() + })) + })) + .build() + .await?; + + assert_eq!( + config.approvals_reviewer, + ApprovalsReviewer::GuardianSubagent + ); + assert!( + config + .startup_warnings + .iter() + .all(|warning| !warning.contains("approvals_reviewer")), + "{:?}", + config.startup_warnings + ); + Ok(()) +} + #[tokio::test] async fn smart_approvals_alias_is_ignored() -> std::io::Result<()> { let codex_home = TempDir::new()?; @@ -5901,7 +6025,7 @@ smart_approvals = true "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() @@ -5930,7 +6054,7 @@ smart_approvals = true "#, )?; - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) .build() diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 8eb069133..d83fc8737 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -689,6 +689,11 @@ impl ConfigBuilder { config_layer_stack, ) } + + #[cfg(test)] + pub(crate) fn without_managed_config_for_tests() -> Self { + Self::default().loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + } } impl Config { @@ -2060,6 +2065,7 @@ impl Config { // Config. let ConfigRequirements { approval_policy: mut constrained_approval_policy, + approvals_reviewer: mut constrained_approvals_reviewer, sandbox_policy: mut constrained_sandbox_policy, web_search_mode: mut constrained_web_search_mode, feature_requirements, @@ -2308,10 +2314,22 @@ impl Config { ); approval_policy = constrained_approval_policy.value(); } - let approvals_reviewer = approvals_reviewer_override + let approvals_reviewer_was_explicit = approvals_reviewer_override.is_some() + || config_profile.approvals_reviewer.is_some() + || cfg.approvals_reviewer.is_some(); + let mut approvals_reviewer = approvals_reviewer_override .or(config_profile.approvals_reviewer) .or(cfg.approvals_reviewer) .unwrap_or(ApprovalsReviewer::User); + if !approvals_reviewer_was_explicit + && let Err(err) = constrained_approvals_reviewer.can_set(&approvals_reviewer) + { + tracing::warn!( + error = %err, + "default approvals reviewer is disallowed by requirements; falling back to required default" + ); + approvals_reviewer = constrained_approvals_reviewer.value(); + } let web_search_mode = resolve_web_search_mode(&cfg, &config_profile, &features) .unwrap_or(WebSearchMode::Cached); let web_search_config = resolve_web_search_config(&cfg, &config_profile); @@ -2554,6 +2572,12 @@ impl Config { &mut constrained_approval_policy, &mut startup_warnings, )?; + apply_requirement_constrained_value( + "approvals_reviewer", + approvals_reviewer, + &mut constrained_approvals_reviewer, + &mut startup_warnings, + )?; apply_requirement_constrained_value( "sandbox_mode", sandbox_policy, @@ -2639,7 +2663,7 @@ impl Config { windows_sandbox_mode, windows_sandbox_private_desktop, }, - approvals_reviewer, + approvals_reviewer: constrained_approvals_reviewer.value(), enforce_residency: enforce_residency.value, notify: cfg.notify, user_instructions, diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index 279af666c..e878270b8 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -140,6 +140,16 @@ impl ConfigService { } } + #[cfg(test)] + pub(crate) fn without_managed_config_for_tests(codex_home: PathBuf) -> Self { + Self::new( + codex_home, + Vec::new(), + LoaderOverrides::without_managed_config_for_tests(), + CloudRequirementsLoader::default(), + ) + } + pub async fn read( &self, params: ConfigReadParams, diff --git a/codex-rs/core/src/config/service_tests.rs b/codex-rs/core/src/config/service_tests.rs index a1897c9dd..ae6232660 100644 --- a/codex-rs/core/src/config/service_tests.rs +++ b/codex-rs/core/src/config/service_tests.rs @@ -74,7 +74,7 @@ unified_exec = true "#; std::fs::write(tmp.path().join(CONFIG_TOML_FILE), original)?; - let service = ConfigService::new_with_defaults(tmp.path().to_path_buf()); + let service = ConfigService::without_managed_config_for_tests(tmp.path().to_path_buf()); service .write_value(ConfigValueWriteParams { file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()), @@ -108,7 +108,7 @@ async fn write_value_supports_nested_app_paths() -> Result<()> { let tmp = tempdir().expect("tempdir"); std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "")?; - let service = ConfigService::new_with_defaults(tmp.path().to_path_buf()); + let service = ConfigService::without_managed_config_for_tests(tmp.path().to_path_buf()); service .write_value(ConfigValueWriteParams { file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()), @@ -178,12 +178,7 @@ async fn read_includes_origins_and_layers() { let service = ConfigService::new( tmp.path().to_path_buf(), vec![], - LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()), CloudRequirementsLoader::default(), ); @@ -245,23 +240,23 @@ async fn write_value_succeeds_when_managed_preferences_expand_home_directory_pat let tmp = tempdir().expect("tempdir"); std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"\n")?; - let service = ConfigService::new( - tmp.path().to_path_buf(), - vec![], - LoaderOverrides { - managed_config_path: Some(tmp.path().join("managed_config.toml")), - managed_preferences_base64: Some( - base64::prelude::BASE64_STANDARD.encode( - r#" + let mut loader_overrides = + LoaderOverrides::with_managed_config_path_for_tests(tmp.path().join("managed_config.toml")); + loader_overrides.managed_preferences_base64 = Some( + base64::prelude::BASE64_STANDARD.encode( + r#" sandbox_mode = "workspace-write" [sandbox_workspace_write] writable_roots = ["~/code"] "# - .as_bytes(), - ), - ), - macos_managed_config_requirements_base64: None, - }, + .as_bytes(), + ), + ); + + let service = ConfigService::new( + tmp.path().to_path_buf(), + vec![], + loader_overrides, CloudRequirementsLoader::default(), ); @@ -301,12 +296,7 @@ async fn write_value_reports_override() { let service = ConfigService::new( tmp.path().to_path_buf(), vec![], - LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()), CloudRequirementsLoader::default(), ); @@ -352,7 +342,7 @@ async fn version_conflict_rejected() { let user_path = tmp.path().join(CONFIG_TOML_FILE); std::fs::write(&user_path, "model = \"user\"").unwrap(); - let service = ConfigService::new_with_defaults(tmp.path().to_path_buf()); + let service = ConfigService::without_managed_config_for_tests(tmp.path().to_path_buf()); let error = service .write_value(ConfigValueWriteParams { file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()), @@ -375,7 +365,7 @@ async fn write_value_defaults_to_user_config_path() { let tmp = tempdir().expect("tempdir"); std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "").unwrap(); - let service = ConfigService::new_with_defaults(tmp.path().to_path_buf()); + let service = ConfigService::without_managed_config_for_tests(tmp.path().to_path_buf()); service .write_value(ConfigValueWriteParams { file_path: None, @@ -405,12 +395,7 @@ async fn invalid_user_value_rejected_even_if_overridden_by_managed() { let service = ConfigService::new( tmp.path().to_path_buf(), vec![], - LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()), CloudRequirementsLoader::default(), ); @@ -439,7 +424,7 @@ async fn reserved_builtin_provider_override_rejected() { let tmp = tempdir().expect("tempdir"); std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"\n").unwrap(); - let service = ConfigService::new_with_defaults(tmp.path().to_path_buf()); + let service = ConfigService::without_managed_config_for_tests(tmp.path().to_path_buf()); let error = service .write_value(ConfigValueWriteParams { file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()), @@ -470,12 +455,7 @@ async fn write_value_rejects_feature_requirement_conflict() { let service = ConfigService::new( tmp.path().to_path_buf(), vec![], - LoaderOverrides { - managed_config_path: None, - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::without_managed_config_for_tests(), CloudRequirementsLoader::new(async { Ok(Some(ConfigRequirementsToml { feature_requirements: Some(crate::config_loader::FeatureRequirementsToml { @@ -521,12 +501,7 @@ async fn write_value_rejects_profile_feature_requirement_conflict() { let service = ConfigService::new( tmp.path().to_path_buf(), vec![], - LoaderOverrides { - managed_config_path: None, - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::without_managed_config_for_tests(), CloudRequirementsLoader::new(async { Ok(Some(ConfigRequirementsToml { feature_requirements: Some(crate::config_loader::FeatureRequirementsToml { @@ -583,12 +558,7 @@ async fn read_reports_managed_overrides_user_and_session_flags() { let service = ConfigService::new( tmp.path().to_path_buf(), cli_overrides, - LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()), CloudRequirementsLoader::default(), ); @@ -641,12 +611,7 @@ async fn write_value_reports_managed_override() { let service = ConfigService::new( tmp.path().to_path_buf(), vec![], - LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }, + LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()), CloudRequirementsLoader::default(), ); @@ -698,7 +663,7 @@ alpha = "a" std::fs::write(&path, base)?; - let service = ConfigService::new_with_defaults(tmp.path().to_path_buf()); + let service = ConfigService::without_managed_config_for_tests(tmp.path().to_path_buf()); service .write_value(ConfigValueWriteParams { file_path: Some(path.display().to_string()), diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index a00512967..48555f0b9 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -11,6 +11,7 @@ use codex_app_server_protocol::ConfigLayerSource; use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigRequirementsWithSources; use codex_git_utils::resolve_root_git_project_for_trust; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::TrustLevel; use codex_protocol::protocol::AskForApproval; @@ -880,6 +881,7 @@ async fn load_project_layers( #[derive(Deserialize, Debug, Clone, Default, PartialEq)] struct LegacyManagedConfigToml { approval_policy: Option, + approvals_reviewer: Option, sandbox_mode: Option, } @@ -889,11 +891,15 @@ impl From for ConfigRequirementsToml { let LegacyManagedConfigToml { approval_policy, + approvals_reviewer, sandbox_mode, } = legacy; if let Some(approval_policy) = approval_policy { config_requirements_toml.allowed_approval_policies = Some(vec![approval_policy]); } + if let Some(approvals_reviewer) = approvals_reviewer { + config_requirements_toml.allowed_approvals_reviewers = Some(vec![approvals_reviewer]); + } if let Some(sandbox_mode) = sandbox_mode { let required_mode: SandboxModeRequirement = sandbox_mode.into(); // Allowing read-only is a requirement for Codex to function correctly. @@ -957,6 +963,7 @@ foo = "xyzzy" fn legacy_managed_config_backfill_includes_read_only_sandbox_mode() { let legacy = LegacyManagedConfigToml { approval_policy: None, + approvals_reviewer: None, sandbox_mode: Some(SandboxMode::WorkspaceWrite), }; @@ -971,6 +978,22 @@ foo = "xyzzy" ); } + #[test] + fn legacy_managed_config_backfill_includes_approvals_reviewer() { + let legacy = LegacyManagedConfigToml { + approval_policy: None, + approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent), + sandbox_mode: None, + }; + + let requirements = ConfigRequirementsToml::from(legacy); + + assert_eq!( + requirements.allowed_approvals_reviewers, + Some(vec![ApprovalsReviewer::GuardianSubagent]) + ); + } + #[cfg(windows)] #[test] fn windows_system_requirements_toml_file_uses_expected_suffix() { diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 77e69a53c..ea782922b 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -115,10 +115,7 @@ async fn returns_config_error_for_invalid_managed_config_toml() { let contents = "model = \"gpt-4\"\ninvalid = ["; std::fs::write(&managed_path, contents).expect("write managed config"); - let overrides = LoaderOverrides { - managed_config_path: Some(managed_path.clone()), - ..Default::default() - }; + let overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()); let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); let err = load_config_layers_state( @@ -202,12 +199,7 @@ extra = true ) .expect("write managed config"); - let overrides = LoaderOverrides { - managed_config_path: Some(managed_path), - #[cfg(target_os = "macos")] - managed_preferences_base64: None, - macos_managed_config_requirements_base64: None, - }; + let overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path); let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); let state = load_config_layers_state( @@ -239,14 +231,7 @@ async fn returns_empty_when_all_layers_missing() { let tmp = tempdir().expect("tempdir"); let managed_path = tmp.path().join("managed_config.toml"); - let overrides = LoaderOverrides { - managed_config_path: Some(managed_path), - #[cfg(target_os = "macos")] - // Force managed preferences to resolve as empty so this test does not - // inherit non-empty machine-specific managed state. - managed_preferences_base64: Some(String::new()), - macos_managed_config_requirements_base64: None, - }; + let overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path); let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); let layers = load_config_layers_state( @@ -337,13 +322,9 @@ value = "managed" flag = false "#; - let overrides = LoaderOverrides { - managed_config_path: Some(managed_path), - managed_preferences_base64: Some( - base64::prelude::BASE64_STANDARD.encode(raw_managed_preferences.as_bytes()), - ), - macos_managed_config_requirements_base64: None, - }; + let mut overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path); + overrides.managed_preferences_base64 = + Some(base64::prelude::BASE64_STANDARD.encode(raw_managed_preferences.as_bytes())); let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); let state = load_config_layers_state( @@ -391,23 +372,23 @@ async fn managed_preferences_expand_home_directory_in_workspace_write_roots() -> }; let tmp = tempdir()?; - let config = ConfigBuilder::default() - .codex_home(tmp.path().to_path_buf()) - .fallback_cwd(Some(tmp.path().to_path_buf())) - .loader_overrides(LoaderOverrides { - managed_config_path: Some(tmp.path().join("managed_config.toml")), - managed_preferences_base64: Some( - base64::prelude::BASE64_STANDARD.encode( - r#" + let mut loader_overrides = + LoaderOverrides::with_managed_config_path_for_tests(tmp.path().join("managed_config.toml")); + loader_overrides.managed_preferences_base64 = Some( + base64::prelude::BASE64_STANDARD.encode( + r#" sandbox_mode = "workspace-write" [sandbox_workspace_write] writable_roots = ["~/code"] "# - .as_bytes(), - ), - ), - macos_managed_config_requirements_base64: None, - }) + .as_bytes(), + ), + ); + + let config = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(loader_overrides) .build() .await?; @@ -435,23 +416,23 @@ async fn managed_preferences_requirements_are_applied() -> anyhow::Result<()> { let tmp = tempdir()?; + let mut loader_overrides = + LoaderOverrides::with_managed_config_path_for_tests(tmp.path().join("managed_config.toml")); + loader_overrides.macos_managed_config_requirements_base64 = Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +allowed_approval_policies = ["never"] +allowed_sandbox_modes = ["read-only"] +"# + .as_bytes(), + ), + ); + let state = load_config_layers_state( tmp.path(), Some(AbsolutePathBuf::try_from(tmp.path())?), &[] as &[(String, TomlValue)], - LoaderOverrides { - managed_config_path: Some(tmp.path().join("managed_config.toml")), - managed_preferences_base64: Some(String::new()), - macos_managed_config_requirements_base64: Some( - base64::prelude::BASE64_STANDARD.encode( - r#" -allowed_approval_policies = ["never"] -allowed_sandbox_modes = ["read-only"] -"# - .as_bytes(), - ), - ), - }, + loader_overrides, CloudRequirementsLoader::default(), ) .await?; @@ -498,22 +479,21 @@ async fn managed_preferences_requirements_take_precedence() -> anyhow::Result<() tokio::fs::write(&managed_path, "approval_policy = \"on-request\"\n").await?; + let mut loader_overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path); + loader_overrides.macos_managed_config_requirements_base64 = Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +allowed_approval_policies = ["never"] +"# + .as_bytes(), + ), + ); + let state = load_config_layers_state( tmp.path(), Some(AbsolutePathBuf::try_from(tmp.path())?), &[] as &[(String, TomlValue)], - LoaderOverrides { - managed_config_path: Some(managed_path), - managed_preferences_base64: Some(String::new()), - macos_managed_config_requirements_base64: Some( - base64::prelude::BASE64_STANDARD.encode( - r#" -allowed_approval_policies = ["never"] -"# - .as_bytes(), - ), - ), - }, + loader_overrides, CloudRequirementsLoader::default(), ) .await?; @@ -631,24 +611,24 @@ async fn cloud_requirements_take_precedence_over_mdm_requirements() -> anyhow::R use base64::Engine; let tmp = tempdir()?; + let mut loader_overrides = LoaderOverrides::without_managed_config_for_tests(); + loader_overrides.macos_managed_config_requirements_base64 = Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +allowed_approval_policies = ["on-request"] +"# + .as_bytes(), + ), + ); let state = load_config_layers_state( tmp.path(), Some(AbsolutePathBuf::try_from(tmp.path())?), &[] as &[(String, TomlValue)], - LoaderOverrides { - macos_managed_config_requirements_base64: Some( - base64::prelude::BASE64_STANDARD.encode( - r#" -allowed_approval_policies = ["on-request"] -"# - .as_bytes(), - ), - ), - ..LoaderOverrides::default() - }, + loader_overrides, CloudRequirementsLoader::new(async { Ok(Some(ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, @@ -700,6 +680,7 @@ allowed_approval_policies = ["on-request"] RequirementSource::CloudRequirements, ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, @@ -740,6 +721,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> let requirements = ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::Never]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 03a586214..3c05bed90 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -6,7 +6,6 @@ use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigLayerStackOrdering; use crate::config_loader::ConfigRequirements; use crate::config_loader::ConfigRequirementsToml; -use crate::config_loader::LoaderOverrides; use crate::config_loader::RequirementSource; use crate::config_loader::Sourced; use codex_app_server_protocol::ConfigLayerSource; @@ -87,14 +86,8 @@ fn external_file_system_sandbox_policy() -> FileSystemSandboxPolicy { async fn test_config() -> (TempDir, Config) { let home = TempDir::new().expect("create temp dir"); - let config = ConfigBuilder::default() + let config = ConfigBuilder::without_managed_config_for_tests() .codex_home(home.path().to_path_buf()) - .loader_overrides(LoaderOverrides { - #[cfg(target_os = "macos")] - managed_preferences_base64: Some(String::new()), - macos_managed_config_requirements_base64: Some(String::new()), - ..LoaderOverrides::default() - }) .build() .await .expect("load default test config"); diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index a6e652f79..86d9949c5 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -566,6 +566,7 @@ mod tests { let requirements_toml = ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::OnRequest]), + allowed_approvals_reviewers: None, allowed_sandbox_modes: Some(vec![SandboxModeRequirement::ReadOnly]), allowed_web_search_modes: Some(vec![WebSearchModeRequirement::Cached]), guardian_developer_instructions: None, @@ -729,6 +730,7 @@ approval_policy = "never" let requirements_toml = ConfigRequirementsToml { allowed_approval_policies: None, + allowed_approvals_reviewers: None, allowed_sandbox_modes: None, allowed_web_search_modes: Some(Vec::new()), guardian_developer_instructions: None,