diff --git a/codex-rs/cloud-config/src/lib.rs b/codex-rs/cloud-config/src/lib.rs index cda4db44fe..dc9a3689d1 100644 --- a/codex-rs/cloud-config/src/lib.rs +++ b/codex-rs/cloud-config/src/lib.rs @@ -1452,7 +1452,9 @@ deny_read = [{deny_read:?}] r#" [apps.connector_5f3c8c41a1e54ad7a76272c89e2554fa] enabled = false -allowed_approvals_reviewers = ["user"] + +[apps.connector_5f3c8c41a1e54ad7a76272c89e2554fa.allowed_approvals_reviewers] +user = true "#, )); @@ -1464,9 +1466,10 @@ allowed_approvals_reviewers = ["user"] "connector_5f3c8c41a1e54ad7a76272c89e2554fa".to_string(), codex_config::AppRequirementToml { enabled: Some(false), - allowed_approvals_reviewers: Some(vec![ + allowed_approvals_reviewers: Some(BTreeMap::from([( codex_protocol::config_types::ApprovalsReviewer::User, - ]), + true, + )])), ..Default::default() }, )]), diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 7c844dc17b..c69170b6e7 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -148,6 +148,7 @@ impl std::ops::DerefMut for ConstrainedWithSource { pub struct ConfigRequirements { pub approval_policy: ConstrainedWithSource, pub approvals_reviewer: ConstrainedWithSource, + pub app_approvals_reviewers: BTreeMap>, pub permission_profile: ConstrainedWithSource, pub windows_sandbox_mode: ConstrainedWithSource>, pub web_search_mode: ConstrainedWithSource, @@ -179,6 +180,7 @@ impl Default for ConfigRequirements { Constrained::allow_any_from_default(), /*source*/ None, ), + app_approvals_reviewers: BTreeMap::new(), permission_profile: ConstrainedWithSource::new( Constrained::allow_any(PermissionProfile::read_only()), /*source*/ None, @@ -214,6 +216,15 @@ impl ConfigRequirements { pub fn exec_policy_source(&self) -> Option<&RequirementSource> { self.exec_policy.as_ref().map(|policy| &policy.source) } + + pub fn approvals_reviewer_constraint_for_app( + &self, + connector_id: Option<&str>, + ) -> &ConstrainedWithSource { + connector_id + .and_then(|connector_id| self.app_approvals_reviewers.get(connector_id)) + .unwrap_or(&self.approvals_reviewer) + } } #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] @@ -764,7 +775,7 @@ impl AppToolsRequirementsToml { #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] pub struct AppRequirementToml { pub enabled: Option, - pub allowed_approvals_reviewers: Option>, + pub allowed_approvals_reviewers: Option>, pub tools: Option, } @@ -793,8 +804,8 @@ impl AppsRequirementsToml { /// Merge app requirements from a lower-precedence source into an existing higher-precedence set. /// This lets managed sources (for example Cloud/MDM) enforce setting disablement across layers, -/// while app reviewer constraints and exact tool approval settings keep the higher-precedence -/// value when present. +/// while app reviewer constraints merge by reviewer and exact tool approval settings keep the +/// higher-precedence value when present. pub(crate) fn merge_app_requirements_descending( base: &mut AppsRequirementsToml, incoming: AppsRequirementsToml, @@ -810,9 +821,13 @@ pub(crate) fn merge_app_requirements_descending( higher_precedence.or(lower_precedence) }; - if base_requirement.allowed_approvals_reviewers.is_none() { - base_requirement.allowed_approvals_reviewers = - incoming_requirement.allowed_approvals_reviewers; + if let Some(incoming_reviewers) = incoming_requirement.allowed_approvals_reviewers { + let base_reviewers = base_requirement + .allowed_approvals_reviewers + .get_or_insert_with(Default::default); + for (reviewer, allowed) in incoming_reviewers { + base_reviewers.entry(reviewer).or_insert(allowed); + } } let Some(incoming_tools) = incoming_requirement.tools else { continue; @@ -1144,28 +1159,39 @@ impl ConfigRequirementsToml { } } -fn validate_app_approvals_reviewers_are_subsets( +fn validate_app_approvals_reviewers( allowed_approvals_reviewers: Option<&Sourced>>, apps: Option<&Sourced>, ) -> Result<(), ConstraintError> { - let (Some(allowed_approvals_reviewers), Some(apps)) = (allowed_approvals_reviewers, apps) - else { + let Some(apps) = apps else { return Ok(()); }; - if allowed_approvals_reviewers.value.is_empty() { - return Ok(()); - } for (app_id, app_requirement) in &apps.value.apps { let Some(app_allowed_approvals_reviewers) = - app_requirement.allowed_approvals_reviewers.as_deref() + app_requirement.allowed_approvals_reviewers.as_ref() else { continue; }; - if let Some(reviewer) = app_allowed_approvals_reviewers - .iter() - .find(|reviewer| !allowed_approvals_reviewers.value.contains(reviewer)) + if !app_allowed_approvals_reviewers + .values() + .any(|allowed| *allowed) + { + return Err(ConstraintError::empty_field(format!( + "apps.{app_id}.allowed_approvals_reviewers" + ))); + } + + if let Some(allowed_approvals_reviewers) = + allowed_approvals_reviewers.filter(|reviewers| !reviewers.value.is_empty()) + && let Some(reviewer) = + app_allowed_approvals_reviewers + .iter() + .find_map(|(reviewer, allowed)| { + (*allowed && !allowed_approvals_reviewers.value.contains(reviewer)) + .then_some(reviewer) + }) { return Err(ConstraintError::InvalidValue { field_name: "apps.*.allowed_approvals_reviewers", @@ -1179,11 +1205,64 @@ fn validate_app_approvals_reviewers_are_subsets( Ok(()) } +fn build_app_approvals_reviewer_constraints( + apps: Option<&Sourced>, + global_constraint: &ConstrainedWithSource, +) -> Result>, ConstraintError> { + let Some(apps) = apps else { + return Ok(BTreeMap::new()); + }; + + let mut constraints = BTreeMap::new(); + for (app_id, app_requirement) in &apps.value.apps { + let Some(reviewer_map) = app_requirement.allowed_approvals_reviewers.as_ref() else { + continue; + }; + + let allowed = reviewer_map + .iter() + .filter_map(|(reviewer, allowed)| allowed.then_some(*reviewer)) + .collect::>(); + let field_name = format!("apps.{app_id}.allowed_approvals_reviewers"); + let Some(initial_value) = allowed + .iter() + .copied() + .find(|reviewer| global_constraint.can_set(reviewer).is_ok()) + else { + return Err(ConstraintError::empty_field(field_name)); + }; + + let allowed_for_validator = allowed.clone(); + let global_constraint_for_validator = global_constraint.clone(); + let requirement_source_for_error = apps.source.clone(); + let app_id_for_error = app_id.clone(); + let constrained = Constrained::new(initial_value, move |candidate| { + global_constraint_for_validator.can_set(candidate)?; + if allowed_for_validator.contains(candidate) { + Ok(()) + } else { + Err(ConstraintError::InvalidValue { + field_name: "apps.*.allowed_approvals_reviewers", + candidate: format!("{candidate:?} for app {app_id_for_error:?}"), + allowed: format!("{allowed_for_validator:?}"), + requirement_source: requirement_source_for_error.clone(), + }) + } + })?; + constraints.insert( + app_id.clone(), + ConstrainedWithSource::new(constrained, Some(apps.source.clone())), + ); + } + + Ok(constraints) +} + impl TryFrom for ConfigRequirements { type Error = ConstraintError; fn try_from(toml: ConfigRequirementsWithSources) -> Result { - validate_app_approvals_reviewers_are_subsets( + validate_app_approvals_reviewers( toml.allowed_approvals_reviewers.as_ref(), toml.apps.as_ref(), )?; @@ -1205,7 +1284,7 @@ impl TryFrom for ConfigRequirements { hooks, mcp_servers, plugins, - apps: _apps, + apps, rules, enforce_residency, network, @@ -1272,6 +1351,8 @@ impl TryFrom for ConfigRequirements { /*source*/ None, ), }; + let app_approvals_reviewers = + build_app_approvals_reviewer_constraints(apps.as_ref(), &approvals_reviewer)?; let default_permission_profile = PermissionProfile::read_only(); let permission_profile = match allowed_sandbox_modes { @@ -1472,6 +1553,7 @@ impl TryFrom for ConfigRequirements { Ok(ConfigRequirements { approval_policy, approvals_reviewer, + app_approvals_reviewers, permission_profile, windows_sandbox_mode, web_search_mode, @@ -2095,7 +2177,10 @@ allowed_approvals_reviewers = ["user"] let toml_str = r#" [apps.connector_123123] enabled = false - allowed_approvals_reviewers = ["user"] + + [apps.connector_123123.allowed_approvals_reviewers] + user = true + auto_review = false "#; let requirements: ConfigRequirementsToml = from_str(toml_str)?; @@ -2106,7 +2191,10 @@ allowed_approvals_reviewers = ["user"] "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + allowed_approvals_reviewers: Some(BTreeMap::from([ + (ApprovalsReviewer::User, true), + (ApprovalsReviewer::AutoReview, false), + ])), ..Default::default() }, )]), @@ -2300,12 +2388,15 @@ allowed_approvals_reviewers = ["user"] } #[test] - fn merge_app_requirements_descending_preserves_higher_app_reviewer_constraint() { + fn merge_app_requirements_descending_merges_app_reviewer_constraints_by_reviewer() { let mut merged = AppsRequirementsToml { apps: BTreeMap::from([( "connector_123123".to_string(), AppRequirementToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + allowed_approvals_reviewers: Some(BTreeMap::from([( + ApprovalsReviewer::User, + true, + )])), ..Default::default() }, )]), @@ -2314,7 +2405,10 @@ allowed_approvals_reviewers = ["user"] apps: BTreeMap::from([( "connector_123123".to_string(), AppRequirementToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), + allowed_approvals_reviewers: Some(BTreeMap::from([ + (ApprovalsReviewer::User, false), + (ApprovalsReviewer::AutoReview, false), + ])), ..Default::default() }, )]), @@ -2328,7 +2422,10 @@ allowed_approvals_reviewers = ["user"] apps: BTreeMap::from([( "connector_123123".to_string(), AppRequirementToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + allowed_approvals_reviewers: Some(BTreeMap::from([ + (ApprovalsReviewer::User, true), + (ApprovalsReviewer::AutoReview, false), + ])), ..Default::default() }, )]), @@ -2623,8 +2720,8 @@ allowed_approvals_reviewers = ["user"] let toml_str = r#" allowed_approvals_reviewers = ["auto_review"] - [apps.calendar] - allowed_approvals_reviewers = ["user"] + [apps.calendar.allowed_approvals_reviewers] + user = true "#; let config: ConfigRequirementsToml = from_str(toml_str)?; let err = ConfigRequirements::try_from(with_unknown_source(config)) @@ -2643,6 +2740,60 @@ allowed_approvals_reviewers = ["user"] Ok(()) } + #[test] + fn app_allowed_approvals_reviewers_requires_an_explicit_allowed_reviewer() -> Result<()> { + for toml_str in [ + r#" + allowed_approvals_reviewers = ["auto_review"] + + [apps.calendar.allowed_approvals_reviewers] + user = false + "#, + r#" + [apps.calendar.allowed_approvals_reviewers] + "#, + ] { + let config: ConfigRequirementsToml = from_str(toml_str)?; + let err = ConfigRequirements::try_from(with_unknown_source(config)) + .expect_err("app reviewer map should explicitly allow at least one reviewer"); + + assert_eq!( + err, + ConstraintError::EmptyField { + field_name: "apps.calendar.allowed_approvals_reviewers".to_string(), + } + ); + } + + Ok(()) + } + + #[test] + fn app_allowed_approvals_reviewers_builds_normalized_constraint() -> Result<()> { + let toml_str = r#" + allowed_approvals_reviewers = ["user", "auto_review"] + + [apps.calendar.allowed_approvals_reviewers] + user = false + auto_review = true + "#; + let config: ConfigRequirementsToml = from_str(toml_str)?; + let requirements = ConfigRequirements::try_from(with_unknown_source(config))?; + let constraint = requirements.approvals_reviewer_constraint_for_app(Some("calendar")); + + assert_eq!(constraint.value(), ApprovalsReviewer::AutoReview); + assert!(constraint.can_set(&ApprovalsReviewer::AutoReview).is_ok()); + assert!(constraint.can_set(&ApprovalsReviewer::User).is_err()); + assert!( + requirements + .approvals_reviewer_constraint_for_app(Some("drive")) + .can_set(&ApprovalsReviewer::User) + .is_ok() + ); + + Ok(()) + } + #[test] fn deserialize_allowed_windows_sandbox_implementations() -> Result<()> { let toml_str = r#" diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index eac8ce2336..7339220d7d 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -226,7 +226,10 @@ shared = false [apps.connector_1] enabled = false -allowed_approvals_reviewers = ["auto_review", "user"] + +[apps.connector_1.allowed_approvals_reviewers] +user = true +auto_review = true [apps.connector_1.tools.search] approval_mode = "prompt" @@ -245,7 +248,9 @@ shared = true [apps.connector_1] enabled = true -allowed_approvals_reviewers = ["user"] + +[apps.connector_1.allowed_approvals_reviewers] +auto_review = false [apps.connector_1.tools.search] approval_mode = "approve" @@ -266,7 +271,10 @@ shared = true [apps.connector_1] enabled = true -allowed_approvals_reviewers = ["user"] + +[apps.connector_1.allowed_approvals_reviewers] +user = true +auto_review = false [apps.connector_1.tools.list] approval_mode = "prompt" diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 051064f279..797db9cb75 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2555,6 +2555,7 @@ impl Config { let ConfigRequirements { approval_policy: mut constrained_approval_policy, approvals_reviewer: mut constrained_approvals_reviewer, + app_approvals_reviewers: _, permission_profile: mut constrained_permission_profile, windows_sandbox_mode: mut constrained_windows_sandbox_mode, web_search_mode: mut constrained_web_search_mode, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 2ba2c92653..4998929495 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -24,7 +24,6 @@ use crate::config::Config; use crate::mcp::McpManager; use crate::plugins::list_tool_suggest_discoverable_plugins; use crate::session::INITIAL_SUBMIT_ID; -use codex_config::AppRequirementToml; use codex_config::AppsRequirementsToml; use codex_config::types::AppToolApproval; use codex_config::types::ApprovalsReviewer; @@ -587,52 +586,24 @@ pub(crate) fn mcp_approvals_reviewer( .and_then(|connector_id| apps_config.apps.get(connector_id)) .and_then(|app| app.approvals_reviewer) }); - let app_requirement = config + let reviewer_constraint = config .config_layer_stack - .requirements_toml() - .apps - .as_ref() - .and_then(|apps| connector_id.and_then(|connector_id| apps.apps.get(connector_id))); + .requirements() + .approvals_reviewer_constraint_for_app(connector_id); - app_approvals_reviewer(config, app_reviewer, app_requirement) + app_approvals_reviewer(config, app_reviewer, reviewer_constraint) } fn app_approvals_reviewer( config: &Config, configured: Option, - requirement: Option<&AppRequirementToml>, + reviewer_constraint: &codex_config::ConstrainedWithSource, ) -> ApprovalsReviewer { - let global_requirement = &config.config_layer_stack.requirements().approvals_reviewer; - let resolved_app_allowed = - requirement.and_then(|requirement| requirement.allowed_approvals_reviewers.as_deref()); - - if let Some(resolved_app_allowed) = resolved_app_allowed { - return configured - .into_iter() - .chain([config.approvals_reviewer]) - .find(|reviewer| { - resolved_app_allowed.contains(reviewer) - && global_requirement.can_set(reviewer).is_ok() - }) - .or_else(|| { - resolved_app_allowed - .iter() - .copied() - .find(|reviewer| global_requirement.can_set(reviewer).is_ok()) - }) - // Invalid or stale requirements must not widen the global managed constraint. - .unwrap_or_else(|| { - if global_requirement.can_set(&ApprovalsReviewer::User).is_ok() { - ApprovalsReviewer::User - } else { - config.approvals_reviewer - } - }); - } - configured - .filter(|reviewer| global_requirement.can_set(reviewer).is_ok()) - .unwrap_or(config.approvals_reviewer) + .into_iter() + .chain([config.approvals_reviewer]) + .find(|reviewer| reviewer_constraint.can_set(reviewer).is_ok()) + .unwrap_or_else(|| reviewer_constraint.value()) } fn read_apps_config(config: &Config) -> Option { diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index d71db848c6..fd4b53dc1b 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -475,7 +475,7 @@ approvals_reviewer = "user" } #[tokio::test] -async fn app_approvals_reviewer_uses_first_resolved_app_requirement_within_global_constraint() { +async fn app_approvals_reviewer_uses_allowed_fallback_within_global_constraint() { let codex_home = tempdir().expect("tempdir should succeed"); std::fs::write( codex_home.path().join(CONFIG_TOML_FILE), @@ -493,7 +493,10 @@ approvals_reviewer = "user" apps: BTreeMap::from([( "calendar".to_string(), AppRequirementToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), + allowed_approvals_reviewers: Some(BTreeMap::from([ + (ApprovalsReviewer::User, false), + (ApprovalsReviewer::AutoReview, true), + ])), ..Default::default() }, )]), @@ -515,71 +518,29 @@ approvals_reviewer = "user" ); } -#[tokio::test] -async fn app_approvals_reviewer_does_not_widen_global_reviewer_requirement() { - let codex_home = tempdir().expect("tempdir should succeed"); - std::fs::write( - codex_home.path().join(CONFIG_TOML_FILE), - r#" -approvals_reviewer = "auto_review" -"#, - ) - .expect("write config"); - let requirements = ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), - ..Default::default() - }; - let config = ConfigBuilder::default() - .codex_home(codex_home.path().to_path_buf()) - .cloud_requirements(CloudRequirementsLoader::new(async move { - Ok(Some(requirements)) - })) - .build() - .await - .expect("config should build"); - let invalid_app_requirement = AppRequirementToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), - ..Default::default() - }; - let empty_app_requirement = AppRequirementToml { - allowed_approvals_reviewers: Some(Vec::new()), - ..Default::default() - }; - - assert_eq!( - app_approvals_reviewer( - &config, - Some(ApprovalsReviewer::User), - Some(&invalid_app_requirement), - ), - ApprovalsReviewer::AutoReview - ); - assert_eq!( - app_approvals_reviewer(&config, None, Some(&empty_app_requirement)), - ApprovalsReviewer::AutoReview - ); -} - #[tokio::test] async fn app_approvals_reviewer_respects_app_reviewer_requirements() { - for (global, allowed, expected_global, expected_app) in [ + for (global, overrides, expected_global, expected_app) in [ ( "auto_review", - vec![ApprovalsReviewer::User], + BTreeMap::from([(ApprovalsReviewer::User, true)]), ApprovalsReviewer::AutoReview, ApprovalsReviewer::User, ), ( "user", - vec![ApprovalsReviewer::AutoReview], + BTreeMap::from([(ApprovalsReviewer::AutoReview, true)]), ApprovalsReviewer::User, ApprovalsReviewer::AutoReview, ), ( "auto_review", - Vec::new(), + BTreeMap::from([ + (ApprovalsReviewer::User, true), + (ApprovalsReviewer::AutoReview, true), + ]), + ApprovalsReviewer::AutoReview, ApprovalsReviewer::AutoReview, - ApprovalsReviewer::User, ), ] { let codex_home = tempdir().expect("tempdir should succeed"); @@ -597,7 +558,7 @@ approvals_reviewer = "{global}" apps: BTreeMap::from([( "calendar".to_string(), AppRequirementToml { - allowed_approvals_reviewers: Some(allowed), + allowed_approvals_reviewers: Some(overrides), ..Default::default() }, )]), diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index 9e5c47a5c1..d6b2b3bbb9 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/codex-rs/protocol/src/config_types.rs @@ -154,7 +154,9 @@ impl fmt::Display for ProfileV2Name { } } -#[derive(Deserialize, Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Display, TS)] +#[derive( + Deserialize, Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Default, Serialize, Display, TS, +)] #[strum(serialize_all = "snake_case")] #[ts(type = r#""user" | "auto_review" | "guardian_subagent""#)] /// Configures who approval requests are routed to for review. Examples