Use map constraints for app approval reviewers

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()
                         },
                     )]),

codex-rs/config/src/config_requirements.rs

@@ -148,6 +148,7 @@ impl<T> std::ops::DerefMut for ConstrainedWithSource<T> {
 pub struct ConfigRequirements {
     pub approval_policy: ConstrainedWithSource<AskForApproval>,
     pub approvals_reviewer: ConstrainedWithSource<ApprovalsReviewer>,
+    pub app_approvals_reviewers: BTreeMap<String, ConstrainedWithSource<ApprovalsReviewer>>,
     pub permission_profile: ConstrainedWithSource<PermissionProfile>,
     pub windows_sandbox_mode: ConstrainedWithSource<Option<WindowsSandboxModeToml>>,
     pub web_search_mode: ConstrainedWithSource<WebSearchMode>,
@@ -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<ApprovalsReviewer> {
+        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<bool>,
-    pub allowed_approvals_reviewers: Option<Vec<ApprovalsReviewer>>,
+    pub allowed_approvals_reviewers: Option<BTreeMap<ApprovalsReviewer, bool>>,
     pub tools: Option<AppToolsRequirementsToml>,
 }
 
@@ -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<Vec<ApprovalsReviewer>>>,
     apps: Option<&Sourced<AppsRequirementsToml>>,
 ) -> 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<AppsRequirementsToml>>,
+    global_constraint: &ConstrainedWithSource<ApprovalsReviewer>,
+) -> Result<BTreeMap<String, ConstrainedWithSource<ApprovalsReviewer>>, 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::<Vec<_>>();
+        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<ConfigRequirementsWithSources> for ConfigRequirements {
     type Error = ConstraintError;
 
     fn try_from(toml: ConfigRequirementsWithSources) -> Result<Self, Self::Error> {
-        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<ConfigRequirementsWithSources> for ConfigRequirements {
             hooks,
             mcp_servers,
             plugins,
-            apps: _apps,
+            apps,
             rules,
             enforce_residency,
             network,
@@ -1272,6 +1351,8 @@ impl TryFrom<ConfigRequirementsWithSources> 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<ConfigRequirementsWithSources> 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#"

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"

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,

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<ApprovalsReviewer>,
-    requirement: Option<&AppRequirementToml>,
+    reviewer_constraint: &codex_config::ConstrainedWithSource<ApprovalsReviewer>,
 ) -> 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<AppsConfigToml> {

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()
                     },
                 )]),

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