diff --git a/codex-rs/cloud-config/src/lib.rs b/codex-rs/cloud-config/src/lib.rs index 0beb91780c..cda4db44fe 100644 --- a/codex-rs/cloud-config/src/lib.rs +++ b/codex-rs/cloud-config/src/lib.rs @@ -1452,6 +1452,7 @@ deny_read = [{deny_read:?}] r#" [apps.connector_5f3c8c41a1e54ad7a76272c89e2554fa] enabled = false +allowed_approvals_reviewers = ["user"] "#, )); @@ -1463,7 +1464,10 @@ enabled = false "connector_5f3c8c41a1e54ad7a76272c89e2554fa".to_string(), codex_config::AppRequirementToml { enabled: Some(false), - tools: None, + allowed_approvals_reviewers: Some(vec![ + codex_protocol::config_types::ApprovalsReviewer::User, + ]), + ..Default::default() }, )]), }), @@ -1488,7 +1492,6 @@ approval_mode = "approve" apps: BTreeMap::from([( "connector_5f3c8c41a1e54ad7a76272c89e2554fa".to_string(), codex_config::AppRequirementToml { - enabled: None, tools: Some(codex_config::AppToolsRequirementsToml { tools: BTreeMap::from([( "calendar/list_events".to_string(), @@ -1497,6 +1500,7 @@ approval_mode = "approve" }, )]), }), + ..Default::default() }, )]), }), diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 86d64e4153..f1b5fe349e 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -764,12 +764,14 @@ impl AppToolsRequirementsToml { #[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)] pub struct AppRequirementToml { pub enabled: Option, + pub allowed_approvals_reviewers: Option>, pub tools: Option, } impl AppRequirementToml { pub fn is_empty(&self) -> bool { self.enabled.is_none() + && self.allowed_approvals_reviewers.is_none() && self .tools .as_ref() @@ -791,7 +793,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 exact tool approval settings keep the higher-precedence value when present. +/// while app reviewer constraints and exact tool approval settings keep the higher-precedence +/// value when present. pub(crate) fn merge_app_requirements_descending( base: &mut AppsRequirementsToml, incoming: AppsRequirementsToml, @@ -807,6 +810,10 @@ 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; + } let Some(incoming_tools) = incoming_requirement.tools else { continue; }; @@ -2048,6 +2055,7 @@ allowed_approvals_reviewers = ["user"] let toml_str = r#" [apps.connector_123123] enabled = false + allowed_approvals_reviewers = ["user"] "#; let requirements: ConfigRequirementsToml = from_str(toml_str)?; @@ -2058,7 +2066,8 @@ allowed_approvals_reviewers = ["user"] "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + ..Default::default() }, )]), }) @@ -2080,7 +2089,6 @@ allowed_approvals_reviewers = ["user"] apps: BTreeMap::from([( "connector_123123".to_string(), AppRequirementToml { - enabled: None, tools: Some(AppToolsRequirementsToml { tools: BTreeMap::from([( "calendar/list_events".to_string(), @@ -2089,6 +2097,7 @@ allowed_approvals_reviewers = ["user"] }, )]), }), + ..Default::default() }, )]), }) @@ -2105,7 +2114,7 @@ allowed_approvals_reviewers = ["user"] (*app_id).to_string(), AppRequirementToml { enabled: *enabled, - tools: None, + ..Default::default() }, ) }) @@ -2122,7 +2131,6 @@ allowed_approvals_reviewers = ["user"] apps: BTreeMap::from([( app_id.to_string(), AppRequirementToml { - enabled: None, tools: Some(AppToolsRequirementsToml { tools: BTreeMap::from([( tool_name.to_string(), @@ -2131,6 +2139,7 @@ allowed_approvals_reviewers = ["user"] }, )]), }), + ..Default::default() }, )]), } @@ -2250,6 +2259,43 @@ allowed_approvals_reviewers = ["user"] ); } + #[test] + fn merge_app_requirements_descending_preserves_higher_app_reviewer_constraint() { + let mut merged = AppsRequirementsToml { + apps: BTreeMap::from([( + "connector_123123".to_string(), + AppRequirementToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + ..Default::default() + }, + )]), + }; + let lower = AppsRequirementsToml { + apps: BTreeMap::from([( + "connector_123123".to_string(), + AppRequirementToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), + ..Default::default() + }, + )]), + }; + + merge_app_requirements_descending(&mut merged, lower); + + assert_eq!( + merged, + AppsRequirementsToml { + apps: BTreeMap::from([( + "connector_123123".to_string(), + AppRequirementToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + ..Default::default() + }, + )]), + } + ); + } + #[test] fn merge_unset_fields_merges_apps_across_sources_with_enabled_evaluation() { let higher_source = RequirementSource::CloudRequirements; diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index 8acfd5f0d5..eac8ce2336 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -226,6 +226,7 @@ shared = false [apps.connector_1] enabled = false +allowed_approvals_reviewers = ["auto_review", "user"] [apps.connector_1.tools.search] approval_mode = "prompt" @@ -244,6 +245,7 @@ shared = true [apps.connector_1] enabled = true +allowed_approvals_reviewers = ["user"] [apps.connector_1.tools.search] approval_mode = "approve" @@ -264,6 +266,7 @@ shared = true [apps.connector_1] enabled = true +allowed_approvals_reviewers = ["user"] [apps.connector_1.tools.list] approval_mode = "prompt" diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 7a8344b3d7..31f28380fa 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -24,6 +24,7 @@ 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; @@ -577,28 +578,53 @@ pub(crate) fn mcp_approvals_reviewer( server_name: &str, connector_id: Option<&str>, ) -> ApprovalsReviewer { - let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME { - read_user_apps_config(config).and_then(|apps_config| { - connector_id - .and_then(|connector_id| apps_config.apps.get(connector_id)) - .and_then(|app| app.approvals_reviewer) - }) - } else { - None - }; - - if let Some(reviewer) = app_reviewer - && config - .config_layer_stack - .requirements() - .approvals_reviewer - .can_set(&reviewer) - .is_ok() - { - return reviewer; + if server_name != CODEX_APPS_MCP_SERVER_NAME { + return config.approvals_reviewer; } - config.approvals_reviewer + let app_reviewer = read_user_apps_config(config).and_then(|apps_config| { + connector_id + .and_then(|connector_id| apps_config.apps.get(connector_id)) + .and_then(|app| app.approvals_reviewer) + }); + let app_requirement = config + .config_layer_stack + .requirements_toml() + .apps + .as_ref() + .and_then(|apps| connector_id.and_then(|connector_id| apps.apps.get(connector_id))); + + app_approvals_reviewer(config, app_reviewer, app_requirement) +} + +fn app_approvals_reviewer( + config: &Config, + configured: Option, + requirement: Option<&AppRequirementToml>, +) -> ApprovalsReviewer { + 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)) + .or_else(|| resolved_app_allowed.first().copied()) + // An empty managed allow-list must not route through Guardian. + .unwrap_or(ApprovalsReviewer::User); + } + + configured + .filter(|reviewer| { + config + .config_layer_stack + .requirements() + .approvals_reviewer + .can_set(reviewer) + .is_ok() + }) + .unwrap_or(config.approvals_reviewer) } fn read_apps_config(config: &Config) -> Option { @@ -648,14 +674,12 @@ fn managed_app_tool_approval( tool_name: &str, ) -> Option { let connector_id = connector_id?; - requirements_apps_config? - .apps - .get(connector_id)? + let requirement = requirements_apps_config?.apps.get(connector_id)?; + requirement .tools - .as_ref()? - .tools - .get(tool_name)? - .approval_mode + .as_ref() + .and_then(|tools| tools.tools.get(tool_name)) + .and_then(|tool| tool.approval_mode) } fn app_is_enabled(apps_config: &AppsConfigToml, connector_id: Option<&str>) -> bool { diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index c01fd5467a..eb4100cb3a 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -474,6 +474,108 @@ approvals_reviewer = "user" ); } +#[tokio::test] +async fn app_approvals_reviewer_uses_first_resolved_app_requirement_when_constraints_conflict() { + let codex_home = tempdir().expect("tempdir should succeed"); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#" +approvals_reviewer = "user" +"#, + ) + .expect("write config"); + let requirements = ConfigRequirementsToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + apps: Some(AppsRequirementsToml { + apps: BTreeMap::from([( + "calendar".to_string(), + AppRequirementToml { + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), + ..Default::default() + }, + )]), + }), + ..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"); + + assert_eq!( + mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")), + ApprovalsReviewer::AutoReview + ); +} + +#[tokio::test] +async fn app_approvals_reviewer_respects_app_reviewer_requirements() { + for (global, allowed, expected_global, expected_app) in [ + ( + "auto_review", + vec![ApprovalsReviewer::User], + ApprovalsReviewer::AutoReview, + ApprovalsReviewer::User, + ), + ( + "user", + vec![ApprovalsReviewer::AutoReview], + ApprovalsReviewer::User, + ApprovalsReviewer::AutoReview, + ), + ( + "auto_review", + Vec::new(), + ApprovalsReviewer::AutoReview, + ApprovalsReviewer::User, + ), + ] { + let codex_home = tempdir().expect("tempdir should succeed"); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + format!( + r#" +approvals_reviewer = "{global}" +"# + ), + ) + .expect("write config"); + let requirements = ConfigRequirementsToml { + apps: Some(AppsRequirementsToml { + apps: BTreeMap::from([( + "calendar".to_string(), + AppRequirementToml { + allowed_approvals_reviewers: Some(allowed), + ..Default::default() + }, + )]), + }), + ..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"); + + assert_eq!( + mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")), + expected_app + ); + assert_eq!( + mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("drive")), + expected_global + ); + } +} + #[test] fn requirements_disabled_connector_overrides_enabled_connector() { let mut effective_apps = AppsConfigToml { @@ -491,7 +593,7 @@ fn requirements_disabled_connector_overrides_enabled_connector() { "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + ..Default::default() }, )]), }; @@ -524,7 +626,7 @@ fn requirements_enabled_does_not_override_disabled_connector() { "connector_123123".to_string(), AppRequirementToml { enabled: Some(true), - tools: None, + ..Default::default() }, )]), }; @@ -558,7 +660,7 @@ enabled = true "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + ..Default::default() }, )]), }), @@ -602,7 +704,7 @@ async fn cloud_requirements_disable_connector_applies_without_user_apps_table() "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + ..Default::default() }, )]), }), @@ -653,7 +755,7 @@ async fn local_requirements_disable_connector_overrides_user_apps_config() { "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + ..Default::default() }, )]), }), @@ -705,7 +807,7 @@ async fn local_requirements_disable_connector_applies_without_user_apps_table() "connector_123123".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + ..Default::default() }, )]), }), @@ -747,7 +849,7 @@ async fn with_app_enabled_state_preserves_unrelated_disabled_connector() { "connector_drive".to_string(), AppRequirementToml { enabled: Some(false), - tools: None, + ..Default::default() }, )]), }), @@ -829,7 +931,6 @@ fn app_tool_requirements( apps: BTreeMap::from([( app_id.to_string(), AppRequirementToml { - enabled: None, tools: Some(AppToolsRequirementsToml { tools: BTreeMap::from([( tool_name.to_string(), @@ -838,6 +939,7 @@ fn app_tool_requirements( }, )]), }), + ..Default::default() }, )]), }