diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index ee1a49069..cdb8f9553 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -5952,6 +5952,16 @@ }, "AppConfig": { "properties": { + "approvals_reviewer": { + "anyOf": [ + { + "$ref": "#/definitions/v2/ApprovalsReviewer" + }, + { + "type": "null" + } + ] + }, "default_tools_approval_mode": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 4d219f383..6d859ee5f 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -324,6 +324,16 @@ }, "AppConfig": { "properties": { + "approvals_reviewer": { + "anyOf": [ + { + "$ref": "#/definitions/ApprovalsReviewer" + }, + { + "type": "null" + } + ] + }, "default_tools_approval_mode": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json index e714c0975..09cf89789 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json @@ -19,6 +19,16 @@ }, "AppConfig": { "properties": { + "approvals_reviewer": { + "anyOf": [ + { + "$ref": "#/definitions/ApprovalsReviewer" + }, + { + "type": "null" + } + ] + }, "default_tools_approval_mode": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AppsConfig.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AppsConfig.ts index b22997de9..a4a0f2208 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AppsConfig.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AppsConfig.ts @@ -3,6 +3,7 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AppToolApproval } from "./AppToolApproval"; import type { AppToolsConfig } from "./AppToolsConfig"; +import type { ApprovalsReviewer } from "./ApprovalsReviewer"; import type { AppsDefaultConfig } from "./AppsDefaultConfig"; -export type AppsConfig = { _default: AppsDefaultConfig | null, } & ({ [key in string]?: { enabled: boolean, destructive_enabled: boolean | null, open_world_enabled: boolean | null, default_tools_approval_mode: AppToolApproval | null, default_tools_enabled: boolean | null, tools: AppToolsConfig | null, } }); +export type AppsConfig = { _default: AppsDefaultConfig | null, } & ({ [key in string]?: { enabled: boolean, approvals_reviewer: ApprovalsReviewer | null, destructive_enabled: boolean | null, open_world_enabled: boolean | null, default_tools_approval_mode: AppToolApproval | null, default_tools_enabled: boolean | null, tools: AppToolsConfig | null, } }); diff --git a/codex-rs/app-server-protocol/src/protocol/v2/config.rs b/codex-rs/app-server-protocol/src/protocol/v2/config.rs index 473bb1e03..f40192ddc 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/config.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/config.rs @@ -200,6 +200,7 @@ pub struct AppToolsConfig { pub struct AppConfig { #[serde(default = "default_enabled")] pub enabled: bool, + pub approvals_reviewer: Option, pub destructive_enabled: Option, pub open_world_enabled: Option, pub default_tools_approval_mode: Option, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index c853eb50f..f3b55337c 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1716,6 +1716,20 @@ The server also emits `app/list/updated` notifications whenever either source (a } ``` +Connected apps may override the thread's approval reviewer in `config.toml`. +When omitted, the app inherits the top-level `approvals_reviewer` value: + +```toml +approvals_reviewer = "auto_review" + +[apps.demo-app] +approvals_reviewer = "user" +``` + +Setting the app value to `"user"` routes its approval prompts to the user +instead of Guardian; setting it to `"auto_review"` opts that app into Guardian +review when allowed by configuration requirements. + Invoke an app by inserting `$` in the text input. The slug is derived from the app name and lowercased with non-alphanumeric characters replaced by `-` (for example, "Demo App" becomes `$demo-app`). Add a `mention` input item (recommended) so the server uses the exact `app://` path rather than guessing by name. Plugins use the same `mention` item shape, but with `plugin://@` paths from `plugin/installed` or `plugin/list`. Example: diff --git a/codex-rs/app-server/src/config_manager_service_tests.rs b/codex-rs/app-server/src/config_manager_service_tests.rs index 5c6e3de5a..da2437ca1 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -284,6 +284,7 @@ async fn write_value_supports_nested_app_paths() -> Result<()> { "app1".to_string(), AppConfig { enabled: false, + approvals_reviewer: None, destructive_enabled: None, open_world_enabled: None, default_tools_approval_mode: Some(AppToolApproval::Prompt), diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index dbcc12084..649c4fe8e 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -5,6 +5,7 @@ use app_test_support::test_tmp_path_buf; use app_test_support::to_response; use codex_app_server_protocol::AppConfig; use codex_app_server_protocol::AppToolApproval; +use codex_app_server_protocol::ApprovalsReviewer; use codex_app_server_protocol::AppsConfig; use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::ConfigBatchWriteParams; @@ -333,6 +334,7 @@ async fn config_read_includes_apps() -> Result<()> { r#" [apps.app1] enabled = false +approvals_reviewer = "user" destructive_enabled = false default_tools_approval_mode = "prompt" "#, @@ -368,6 +370,7 @@ default_tools_approval_mode = "prompt" "app1".to_string(), AppConfig { enabled: false, + approvals_reviewer: Some(ApprovalsReviewer::User), destructive_enabled: Some(false), open_world_enabled: None, default_tools_approval_mode: Some(AppToolApproval::Prompt), @@ -384,6 +387,16 @@ default_tools_approval_mode = "prompt" profile: None, } ); + assert_eq!( + origins + .get("apps.app1.approvals_reviewer") + .expect("origin") + .name, + ConfigLayerSource::User { + file: user_file.clone(), + profile: None, + } + ); assert_eq!( origins .get("apps.app1.destructive_enabled") diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 51a6f1868..963365584 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -21,7 +21,6 @@ use codex_config::Constrained; use codex_config::McpServerConfig; use codex_config::McpServerTransportConfig; use codex_config::types::AppToolApproval; -use codex_config::types::ApprovalsReviewer; use codex_config::types::OAuthCredentialsStoreMode; use codex_login::CodexAuth; use codex_plugin::PluginCapabilitySummary; @@ -91,7 +90,6 @@ pub fn mcp_permission_prompt_is_auto_approved( #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub struct McpPermissionPromptAutoApproveContext { - pub approvals_reviewer: Option, pub tool_approval_mode: Option, } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index d4d888f20..b29f7a9e5 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -1,7 +1,6 @@ use super::*; use codex_config::Constrained; use codex_config::types::AppToolApproval; -use codex_config::types::ApprovalsReviewer; use codex_login::CodexAuth; use codex_plugin::AppConnectorId; use codex_plugin::PluginCapabilitySummary; @@ -96,7 +95,6 @@ fn mcp_prompt_auto_approval_honors_approved_tools_in_all_permission_modes() { approval_policy, &PermissionProfile::read_only(), McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(ApprovalsReviewer::User), tool_approval_mode: Some(AppToolApproval::Approve), }, )); @@ -106,7 +104,6 @@ fn mcp_prompt_auto_approval_honors_approved_tools_in_all_permission_modes() { AskForApproval::OnRequest, &PermissionProfile::read_only(), McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), tool_approval_mode: Some(AppToolApproval::Auto), }, )); @@ -118,7 +115,6 @@ fn mcp_prompt_auto_approval_rejects_auto_mode_in_default_permission_mode() { AskForApproval::OnRequest, &PermissionProfile::read_only(), McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(ApprovalsReviewer::User), tool_approval_mode: Some(AppToolApproval::Auto), }, )); diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 598b7fdee..7b634a518 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -425,6 +425,10 @@ pub struct AppConfig { #[serde(default = "default_enabled")] pub enabled: bool, + /// Reviewer for approval prompts from this app, overriding the thread default. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub approvals_reviewer: Option, + /// Whether tools with `destructive_hint = true` are allowed for this app. #[serde(default, skip_serializing_if = "Option::is_none")] pub destructive_enabled: Option, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 6dd85dffc..b281b44bb 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -102,6 +102,14 @@ "additionalProperties": false, "description": "Config values for a single app/connector.", "properties": { + "approvals_reviewer": { + "allOf": [ + { + "$ref": "#/definitions/ApprovalsReviewer" + } + ], + "description": "Reviewer for approval prompts from this app, overriding the thread default." + }, "default_tools_approval_mode": { "allOf": [ { diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 646d5969e..3132420e4 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -34,6 +34,7 @@ use crate::config::Config; use crate::guardian::GuardianApprovalRequest; use crate::guardian::new_guardian_review_id; use crate::guardian::routes_approval_to_guardian; +use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::guardian::spawn_approval_request_review; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION; @@ -41,6 +42,7 @@ use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; use crate::mcp_tool_call::build_guardian_mcp_tool_review_request; use crate::mcp_tool_call::is_mcp_tool_approval_question_id; use crate::mcp_tool_call::lookup_mcp_tool_metadata; +use crate::mcp_tool_call::mcp_approvals_reviewer; use crate::session::Codex; use crate::session::CodexSpawnArgs; use crate::session::CodexSpawnOk; @@ -632,15 +634,14 @@ async fn handle_request_user_input( event: RequestUserInputEvent, cancel_token: &CancellationToken, ) { - if routes_approval_to_guardian(parent_ctx) - && let Some(response) = maybe_auto_review_mcp_request_user_input( - parent_session, - parent_ctx, - pending_mcp_invocations, - &event, - cancel_token, - ) - .await + if let Some(response) = maybe_auto_review_mcp_request_user_input( + parent_session, + parent_ctx, + pending_mcp_invocations, + &event, + cancel_token, + ) + .await { let _ = codex.submit(Op::UserInputAnswer { id, response }).await; return; @@ -694,6 +695,11 @@ async fn maybe_auto_review_mcp_request_user_input( &invocation.tool, ) .await; + let approvals_reviewer = + mcp_approvals_reviewer(parent_ctx, &invocation.server, metadata.as_ref()); + if !routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) { + return None; + } let review_cancel = cancel_token.child_token(); let review_rx = spawn_approval_request_review( Arc::clone(parent_session), diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 04f6ae396..64e0c1641 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -2,6 +2,7 @@ use super::*; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX; use async_channel::bounded; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::ResponseItem; @@ -445,3 +446,72 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() { }) ); } + +#[tokio::test] +async fn delegated_mcp_user_reviewer_waits_for_metadata_lookup() { + let (parent_session, parent_ctx, _rx_events) = + crate::session::tests::make_session_and_context_with_rx().await; + let pending_mcp_invocations = Arc::new(Mutex::new(HashMap::from([( + "call-1".to_string(), + McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "dangerous_tool".to_string(), + arguments: None, + }, + )]))); + let cancel_token = CancellationToken::new(); + let manager = Arc::clone(&parent_session.services.mcp_connection_manager); + let (manager_locked_tx, manager_locked_rx) = std::sync::mpsc::sync_channel(0); + let (release_manager_tx, release_manager_rx) = std::sync::mpsc::sync_channel(0); + let manager_lock = tokio::task::spawn_blocking(move || { + let _manager_guard = manager.blocking_write(); + manager_locked_tx + .send(()) + .expect("manager lock receiver should remain open"); + release_manager_rx + .recv() + .expect("manager lock release sender should remain open"); + }); + manager_locked_rx + .recv_timeout(Duration::from_secs(1)) + .expect("manager write lock should be acquired"); + + let event = RequestUserInputEvent { + call_id: "call-1".to_string(), + turn_id: "child-turn-1".to_string(), + questions: vec![RequestUserInputQuestion { + id: format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_call-1"), + header: "Approve app tool call?".to_string(), + question: "Allow this app tool?".to_string(), + is_other: false, + is_secret: false, + options: None, + }], + }; + let response = maybe_auto_review_mcp_request_user_input( + &parent_session, + &parent_ctx, + &pending_mcp_invocations, + &event, + &cancel_token, + ); + tokio::pin!(response); + assert!( + timeout(Duration::from_millis(100), &mut response) + .await + .is_err(), + "manual reviewer should wait for MCP metadata" + ); + release_manager_tx + .send(()) + .expect("manager lock holder should remain open"); + manager_lock + .await + .expect("manager lock task should not panic"); + assert_eq!( + timeout(Duration::from_secs(1), response) + .await + .expect("manual reviewer should finish after MCP metadata lookup"), + None + ); +} diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index c0b49cb63..7a8344b3d 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -26,6 +26,7 @@ use crate::plugins::list_tool_suggest_discoverable_plugins; use crate::session::INITIAL_SUBMIT_ID; use codex_config::AppsRequirementsToml; use codex_config::types::AppToolApproval; +use codex_config::types::ApprovalsReviewer; use codex_config::types::AppsConfigToml; use codex_config::types::ToolSuggestDiscoverableType; use codex_core_plugins::PluginsManager; @@ -571,6 +572,35 @@ pub(crate) fn codex_app_tool_is_enabled(config: &Config, tool_info: &ToolInfo) - .enabled } +pub(crate) fn mcp_approvals_reviewer( + config: &Config, + 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; + } + + config.approvals_reviewer +} + fn read_apps_config(config: &Config) -> Option { let apps_config = read_user_apps_config(config); let had_apps_config = apps_config.is_some(); diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index 5db026c3b..c01fd5467 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -12,6 +12,7 @@ use codex_config::ConfigRequirementsToml; use codex_config::types::AppConfig; use codex_config::types::AppToolConfig; use codex_config::types::AppToolsConfig; +use codex_config::types::ApprovalsReviewer; use codex_config::types::AppsDefaultConfig; use codex_connectors::merge::plugin_connector_to_app_info; use codex_connectors::metadata::connector_install_url; @@ -377,6 +378,7 @@ fn app_is_enabled_prefers_per_app_override_over_default() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: None, open_world_enabled: None, default_tools_approval_mode: None, @@ -390,6 +392,88 @@ fn app_is_enabled_prefers_per_app_override_over_default() { assert!(!app_is_enabled(&apps_config, Some("drive"))); } +#[tokio::test] +async fn app_approvals_reviewer_overrides_global_reviewer() { + for (global, app, expected_global, expected_app) in [ + ( + "user", + "auto_review", + ApprovalsReviewer::User, + ApprovalsReviewer::AutoReview, + ), + ( + "auto_review", + "user", + 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}" + +[apps.calendar] +approvals_reviewer = "{app}" +"# + ), + ) + .expect("write config"); + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .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 + ); + assert_eq!( + mcp_approvals_reviewer(&config, "custom_server", Some("calendar")), + expected_global + ); + } +} + +#[tokio::test] +async fn app_approvals_reviewer_respects_global_reviewer_requirements() { + let codex_home = tempdir().expect("tempdir should succeed"); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#" +approvals_reviewer = "auto_review" + +[apps.calendar] +approvals_reviewer = "user" +"#, + ) + .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"); + + assert_eq!( + mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")), + ApprovalsReviewer::AutoReview + ); +} + #[test] fn requirements_disabled_connector_overrides_enabled_connector() { let mut effective_apps = AppsConfigToml { @@ -932,6 +1016,7 @@ fn app_tool_policy_allows_per_app_enable_when_default_is_disabled() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: None, open_world_enabled: None, default_tools_approval_mode: None, @@ -969,6 +1054,7 @@ fn app_tool_policy_per_tool_enabled_true_overrides_app_level_disable_flags() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: Some(false), open_world_enabled: Some(false), default_tools_approval_mode: None, @@ -1012,6 +1098,7 @@ fn app_tool_policy_default_tools_enabled_true_overrides_app_level_tool_hints() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: Some(false), open_world_enabled: Some(false), default_tools_approval_mode: None, @@ -1047,6 +1134,7 @@ fn app_tool_policy_default_tools_enabled_false_overrides_app_level_tool_hints() "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: Some(true), open_world_enabled: Some(true), default_tools_approval_mode: Some(AppToolApproval::Approve), @@ -1084,6 +1172,7 @@ fn app_tool_policy_uses_default_tools_approval_mode() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: None, open_world_enabled: None, default_tools_approval_mode: Some(AppToolApproval::Prompt), @@ -1123,6 +1212,7 @@ fn app_tool_policy_matches_prefix_stripped_tool_name_for_tool_config() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: Some(false), open_world_enabled: Some(false), default_tools_approval_mode: Some(AppToolApproval::Auto), diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 8cc04e5c4..b4920f1ff 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -39,6 +39,7 @@ pub(crate) use review::review_approval_request; #[cfg(test)] pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::routes_approval_to_guardian; +pub(crate) use review::routes_approval_to_guardian_with_reviewer; pub(crate) use review::spawn_approval_request_review; pub(crate) use review_session::GuardianReviewSessionManager; pub(crate) use review_session::prompt_cache_key_override_for_review_session; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 244d6e09e..167b153e2 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -145,10 +145,18 @@ fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str { /// reviewer instead of surfacing them to the user. ARC may still block actions /// earlier in the flow. pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool { + routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) +} + +/// Whether an approval with its own reviewer selection should be routed through guardian. +pub(crate) fn routes_approval_to_guardian_with_reviewer( + turn: &TurnContext, + approvals_reviewer: ApprovalsReviewer, +) -> bool { matches!( turn.approval_policy.value(), AskForApproval::OnRequest | AskForApproval::Granular(_) - ) && turn.config.approvals_reviewer == ApprovalsReviewer::AutoReview + ) && approvals_reviewer == ApprovalsReviewer::AutoReview } pub(crate) fn is_guardian_reviewer_source( diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 40a07d380..aba67d850 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1099,6 +1099,20 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() { assert!(routes_approval_to_guardian(&turn)); } +#[tokio::test] +async fn routes_approval_to_guardian_can_use_app_reviewer_override() { + let (_session, turn) = crate::session::tests::make_session_and_context().await; + + assert!(!routes_approval_to_guardian_with_reviewer( + &turn, + ApprovalsReviewer::User + )); + assert!(routes_approval_to_guardian_with_reviewer( + &turn, + ApprovalsReviewer::AutoReview + )); +} + #[tokio::test] async fn routes_approval_to_guardian_allows_granular_review_policy() { let (_session, mut turn) = crate::session::tests::make_session_and_context().await; diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e514fa0d0..9c00ba2be 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -13,7 +13,7 @@ use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian; +use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; @@ -32,6 +32,7 @@ use codex_app_server_protocol::McpElicitationSchema; use codex_app_server_protocol::McpServerElicitationRequest; use codex_app_server_protocol::McpServerElicitationRequestParams; use codex_config::types::AppToolApproval; +use codex_config::types::ApprovalsReviewer; use codex_features::Feature; use codex_hooks::PermissionRequestDecision; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; @@ -1162,11 +1163,11 @@ async fn maybe_request_mcp_tool_approval( metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, ) -> Option { + let approvals_reviewer = mcp_approvals_reviewer(turn_context, &invocation.server, metadata); if mcp_permission_prompt_is_auto_approved( turn_context.approval_policy.value(), &turn_context.permission_profile(), McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(turn_context.config.approvals_reviewer), tool_approval_mode: Some(approval_mode), }, ) { @@ -1218,7 +1219,7 @@ async fn maybe_request_mcp_tool_approval( .features .enabled(Feature::ToolCallMcpElicitation); - if routes_approval_to_guardian(turn_context) { + if routes_approval_to_guardian_with_reviewer(turn_context, approvals_reviewer) { let review_id = new_guardian_review_id(); let decision = review_approval_request( sess, @@ -1328,6 +1329,18 @@ async fn maybe_request_mcp_tool_approval( Some(decision) } +pub(crate) fn mcp_approvals_reviewer( + turn_context: &TurnContext, + server_name: &str, + metadata: Option<&McpToolApprovalMetadata>, +) -> ApprovalsReviewer { + connectors::mcp_approvals_reviewer( + turn_context.config.as_ref(), + server_name, + metadata.and_then(|metadata| metadata.connector_id.as_deref()), + ) +} + fn session_mcp_tool_approval_key( invocation: &McpInvocation, metadata: Option<&McpToolApprovalMetadata>, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 534c2f011..c9259b8f2 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1886,6 +1886,7 @@ async fn persist_codex_app_tool_approval_writes_tool_override() { "calendar".to_string(), AppConfig { enabled: true, + approvals_reviewer: None, destructive_enabled: None, open_world_enabled: None, default_tools_approval_mode: None, diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index a775c8b83..cc018343e 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -455,7 +455,15 @@ async fn review_guardian_mcp_elicitation( return Ok(None); }; - if !crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { + let approvals_reviewer = crate::connectors::mcp_approvals_reviewer( + turn_context.config.as_ref(), + request.server_name.as_str(), + elicitation_connector_id(&request.elicitation), + ); + if !crate::guardian::routes_approval_to_guardian_with_reviewer( + turn_context.as_ref(), + approvals_reviewer, + ) { return Ok(None); } @@ -567,6 +575,15 @@ fn guardian_elicitation_review_request( )) } +fn elicitation_connector_id(elicitation: &CreateElicitationRequestParams) -> Option<&str> { + match elicitation { + CreateElicitationRequestParams::FormElicitationParams { meta, .. } + | CreateElicitationRequestParams::UrlElicitationParams { meta, .. } => meta + .as_ref() + .and_then(|meta| metadata_str(&meta.0, MCP_ELICITATION_CONNECTOR_ID_KEY)), + } +} + fn meta_requests_approval_request(meta: &Option) -> bool { meta.as_ref() .and_then(|meta| metadata_str(&meta.0, MCP_ELICITATION_REQUEST_TYPE_KEY))