From 95aada11c4150e4ba28d6279c50f0995c1d93e5a Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 12 Aug 2026 15:25:56 +0000 Subject: [PATCH] Enforce non-interactive approval policy for Codex delegates (#38205) ## What changed - Require delegated Codex sessions to use the `never` approval policy and reject delegate creation with any prompt-capable policy. - Stop forwarding delegate approval and permission requests to the parent session. Approval-requiring commands and MCP tool calls are denied within the delegate instead. - Skip prompts for missing skill MCP dependencies when approvals are disabled. ## Testing Add coverage for review and Guardian delegates to verify that command escalations, MCP approvals, and skill dependency installation do not prompt or execute approval-gated actions. GitOrigin-RevId: fee96a97624352cd25c904592ba7eebe2ae3fd63 --- codex-rs/core/src/codex_delegate.rs | 643 +------------------- codex-rs/core/src/codex_delegate_tests.rs | 485 +-------------- codex-rs/core/src/connectors.rs | 15 - codex-rs/core/src/connectors_tests.rs | 30 +- codex-rs/core/src/guardian/mod.rs | 1 - codex-rs/core/src/guardian/tests.rs | 1 + codex-rs/core/src/mcp_skill_dependencies.rs | 5 + codex-rs/core/src/mcp_tool_call.rs | 37 +- codex-rs/core/src/mcp_tool_call_tests.rs | 20 - codex-rs/core/src/session/mod.rs | 34 -- codex-rs/core/tests/suite/codex_delegate.rs | 411 +++++++++++++ 11 files changed, 483 insertions(+), 1199 deletions(-) diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 5377f93da4..c779296ff8 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -1,55 +1,26 @@ -use std::collections::HashMap; use std::sync::Arc; use async_channel::Receiver; use async_channel::Sender; -use codex_analytics::GuardianApprovalRequestSource; use codex_async_utils::OrCancelExt; -use codex_core_plugins::PluginCommandAttribution; use codex_extension_api::LoadedUserInstructions; -use codex_plugin::PluginId; -use codex_protocol::items::is_safe_plugin_relative_path; -use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::ExecApprovalRequestEvent; -use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::Op; -use codex_protocol::protocol::RequestUserInputEvent; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::Submission; use codex_protocol::protocol::ThreadSource; -use codex_protocol::request_permissions::PermissionGrantScope; -use codex_protocol::request_permissions::RequestPermissionsArgs; -use codex_protocol::request_permissions::RequestPermissionsEvent; -use codex_protocol::request_permissions::RequestPermissionsResponse; -use codex_protocol::request_user_input::RequestUserInputArgs; -use codex_protocol::request_user_input::RequestUserInputResponse; use codex_protocol::user_input::UserInput; use serde_json::Value; use std::time::Duration; -use tokio::sync::Mutex; -use tokio::sync::oneshot; use tokio::time::timeout; use tokio_util::sync::CancellationToken; use crate::config::Config; +use crate::config::Constrained; use crate::environment_selection::TurnEnvironmentSnapshot; -use crate::guardian::GuardianApprovalRequest; -use crate::guardian::GuardianReviewOptions; -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; -use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; -use crate::mcp_tool_call::McpToolApprovalMetadata; -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::mcp_approvals_reviewer; use crate::session::ForkPersistence; use crate::session::GitEnrichmentPolicy; use crate::session::SUBMISSION_CHANNEL_CAPACITY; @@ -67,20 +38,13 @@ use codex_protocol::protocol::MultiAgentVersion; #[cfg(test)] use crate::session::completed_session_loop_termination; -#[derive(Clone)] -struct PendingMcpInvocation { - invocation: McpInvocation, - metadata: Option, -} - /// Start an interactive sub-Codex thread and return its runtime and IO channels. /// -/// The returned IO yields non-approval events emitted by the sub-agent. -/// Approval requests are handled via `parent_session` and are not surfaced. +/// Delegates never request approvals, and the returned IO yields their public events. /// Its submission channel accepts additional `Op`s for the sub-agent. #[allow(clippy::too_many_arguments)] pub(crate) async fn run_codex_thread_interactive( - config: Config, + mut config: Config, auth_manager: Arc, models_manager: SharedModelsManager, parent_session: Arc, @@ -92,6 +56,13 @@ pub(crate) async fn run_codex_thread_interactive( git_enrichment_policy: GitEnrichmentPolicy, windows_sandbox_proxy_settings_mode: codex_sandboxing::WindowsSandboxProxySettingsMode, ) -> Result<(Arc, SessionIo), CodexErr> { + if config.permissions.approval_policy.value() != AskForApproval::Never { + return Err(CodexErr::InvalidRequest( + "Codex delegates require approval policy `never`".to_string(), + )); + } + config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); + let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_ops, rx_ops) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let conversation_history = initial_history.unwrap_or(InitialHistory::New); @@ -160,16 +131,8 @@ pub(crate) async fn run_codex_thread_interactive( let cancel_token_events = cancel_token.child_token(); let cancel_token_ops = cancel_token.child_token(); - // Forward events from the sub-agent to the consumer, filtering approvals and - // routing them to the parent session for decisions. - let parent_session_clone = Arc::clone(&parent_session); - let parent_ctx_clone = Arc::clone(&parent_ctx); - let session_for_events = Arc::clone(&session); + // Forward public events from the sub-agent to the consumer. let io = Arc::new(io); - // Cache the child call's MCP metadata at begin time. The later legacy - // RequestUserInput approval event only carries a call_id and question metadata. - let pending_mcp_invocations = - Arc::new(Mutex::new(HashMap::::new())); let caller_io = SessionIo { tx_sub: tx_ops, rx_event: rx_sub, @@ -178,16 +141,7 @@ pub(crate) async fn run_codex_thread_interactive( }; let io_for_events = Arc::clone(&io); tokio::spawn(async move { - forward_events( - io_for_events, - session_for_events, - tx_sub, - parent_session_clone, - parent_ctx_clone, - pending_mcp_invocations, - cancel_token_events, - ) - .await; + forward_events(io_for_events, tx_sub, cancel_token_events).await; }); // Forward ops from the caller to the sub-agent. @@ -296,11 +250,7 @@ pub(crate) async fn run_codex_thread_one_shot( async fn forward_events( io: Arc, - session: Arc, tx_sub: Sender, - parent_session: Arc, - parent_ctx: Arc, - pending_mcp_invocations: Arc>>, cancel_token: CancellationToken, ) { let cancelled = cancel_token.cancelled(); @@ -326,117 +276,6 @@ async fn forward_events( | EventMsg::McpStartupUpdate(_) | EventMsg::McpStartupComplete(_), } => {} - Event { - id, - msg: EventMsg::ExecApprovalRequest(event), - } => { - // Initiate approval via parent session; do not surface to consumer. - handle_exec_approval( - &io, - id, - &parent_session, - &parent_ctx, - event, - &cancel_token, - ) - .await; - } - Event { - id, - msg: EventMsg::ApplyPatchApprovalRequest(event), - } => { - handle_patch_approval( - &io, - id, - &parent_session, - &parent_ctx, - event, - &cancel_token, - ) - .await; - } - Event { - msg: EventMsg::RequestPermissions(event), - .. - } => { - handle_request_permissions( - &io, - &parent_session, - &parent_ctx, - event, - &cancel_token, - ) - .await; - } - Event { - id, - msg: EventMsg::RequestUserInput(event), - } => { - handle_request_user_input( - &io, - id, - &parent_session, - &parent_ctx, - &pending_mcp_invocations, - event, - &cancel_token, - ) - .await; - } - Event { - id, - msg: EventMsg::McpToolCallBegin(event), - } => { - // The later approval event has only a call ID. Retain the exact facts - // captured before this begin event instead of consulting the latest - // runtime after a refresh. - let metadata = session - .mcp_tool_approval_metadata(&id, &event.call_id) - .await - .map(|(_, metadata)| metadata); - pending_mcp_invocations - .lock() - .await - .insert( - event.call_id.clone(), - PendingMcpInvocation { - invocation: event.invocation.clone(), - metadata, - }, - ); - if !forward_event_or_shutdown( - &io, - &tx_sub, - &cancel_token, - Event { - id, - msg: EventMsg::McpToolCallBegin(event), - }, - ) - .await - { - break; - } - } - Event { - id, - msg: EventMsg::McpToolCallEnd(event), - } => { - pending_mcp_invocations.lock().await.remove(&event.call_id); - if !forward_event_or_shutdown( - &io, - &tx_sub, - &cancel_token, - Event { - id, - msg: EventMsg::McpToolCallEnd(event), - }, - ) - .await - { - break; - } - } other => { if !forward_event_or_shutdown(&io, &tx_sub, &cancel_token, other).await { @@ -497,462 +336,6 @@ async fn forward_ops( } } -/// Handle an ExecApprovalRequest by consulting the parent session and replying. -async fn handle_exec_approval( - io: &SessionIo, - turn_id: String, - parent_session: &Arc, - parent_ctx: &Arc, - event: ExecApprovalRequestEvent, - cancel_token: &CancellationToken, -) { - let approval_id_for_op = event.effective_approval_id(); - let ExecApprovalRequestEvent { - call_id, - plugin_id, - script_path, - approval_id, - environment_id, - command, - cwd, - reason, - network_approval_context, - proposed_execpolicy_amendment, - additional_permissions, - available_decisions, - .. - } = event; - let plugin_attribution = plugin_id - .zip(script_path) - .and_then(|(plugin_id, script_path)| { - let plugin_id = PluginId::parse(&plugin_id).ok()?; - is_safe_plugin_relative_path(&script_path).then_some(PluginCommandAttribution { - plugin_id, - normalized_relative_path: script_path, - }) - }); - let decision = if routes_approval_to_guardian(parent_ctx) { - let review_cancel = cancel_token.child_token(); - let review_rx = spawn_approval_request_review( - Arc::clone(parent_session), - Arc::clone(parent_ctx), - new_guardian_review_id(), - GuardianApprovalRequest::Shell { - id: call_id.clone(), - command, - cwd, - sandbox_permissions: if additional_permissions.is_some() { - crate::sandboxing::SandboxPermissions::WithAdditionalPermissions - } else { - crate::sandboxing::SandboxPermissions::UseDefault - }, - additional_permissions, - justification: None, - }, - reason, - GuardianReviewOptions { - plugin_attribution_override: plugin_attribution.clone(), - approval_request_source: GuardianApprovalRequestSource::DelegatedSubagent, - external_cancel: Some(review_cancel.clone()), - }, - ); - await_approval_with_cancel( - receive_approval_review(review_rx), - parent_session, - &approval_id_for_op, - cancel_token, - Some(&review_cancel), - ) - .await - } else { - await_approval_with_cancel( - parent_session.request_command_approval( - parent_ctx, - call_id, - approval_id, - environment_id, - command, - cwd, - reason, - network_approval_context, - proposed_execpolicy_amendment, - additional_permissions, - available_decisions, - plugin_attribution, - ), - parent_session, - &approval_id_for_op, - cancel_token, - /*review_cancel_token*/ None, - ) - .await - }; - - let _ = io - .submit(Op::ExecApproval { - id: approval_id_for_op, - turn_id: Some(turn_id), - decision, - }) - .await; -} - -/// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying. -async fn handle_patch_approval( - io: &SessionIo, - _id: String, - parent_session: &Arc, - parent_ctx: &Arc, - event: ApplyPatchApprovalRequestEvent, - cancel_token: &CancellationToken, -) { - let ApplyPatchApprovalRequestEvent { - call_id, - changes, - reason, - grant_root, - .. - } = event; - let approval_id = call_id.clone(); - let guardian_decision = if routes_approval_to_guardian(parent_ctx) { - let files = changes - .keys() - .map(|path| { - #[allow(deprecated)] - parent_ctx.cwd.join(path) - }) - .collect::>(); - let review_cancel = cancel_token.child_token(); - let patch = changes - .iter() - .map(|(path, change)| match change { - codex_protocol::protocol::FileChange::Add { content } => { - format!("*** Add File: {}\n{}", path.display(), content) - } - codex_protocol::protocol::FileChange::Delete { content } => { - format!("*** Delete File: {}\n{}", path.display(), content) - } - codex_protocol::protocol::FileChange::Update { - unified_diff, - move_path, - } => { - if let Some(move_path) = move_path { - format!( - "*** Update File: {}\n*** Move to: {}\n{}", - path.display(), - move_path.display(), - unified_diff - ) - } else { - format!("*** Update File: {}\n{}", path.display(), unified_diff) - } - } - }) - .collect::>() - .join("\n"); - let review_rx = spawn_approval_request_review( - Arc::clone(parent_session), - Arc::clone(parent_ctx), - new_guardian_review_id(), - GuardianApprovalRequest::ApplyPatch { - id: approval_id.clone(), - #[allow(deprecated)] - cwd: parent_ctx.cwd.clone(), - files, - patch, - }, - reason.clone(), - GuardianReviewOptions { - plugin_attribution_override: None, - approval_request_source: GuardianApprovalRequestSource::DelegatedSubagent, - external_cancel: Some(review_cancel.clone()), - }, - ); - Some( - await_approval_with_cancel( - receive_approval_review(review_rx), - parent_session, - &approval_id, - cancel_token, - Some(&review_cancel), - ) - .await, - ) - } else { - None - }; - let decision = if let Some(decision) = guardian_decision { - decision - } else { - let decision = - parent_session.request_patch_approval(parent_ctx, call_id, changes, reason, grant_root); - await_approval_with_cancel( - decision, - parent_session, - &approval_id, - cancel_token, - /*review_cancel_token*/ None, - ) - .await - }; - let _ = io - .submit(Op::PatchApproval { - id: approval_id, - decision, - }) - .await; -} - -async fn handle_request_user_input( - io: &SessionIo, - id: String, - parent_session: &Arc, - parent_ctx: &Arc, - pending_mcp_invocations: &Arc>>, - event: RequestUserInputEvent, - cancel_token: &CancellationToken, -) { - if let Some(response) = maybe_auto_review_mcp_request_user_input( - parent_session, - parent_ctx, - pending_mcp_invocations, - &event, - cancel_token, - ) - .await - { - let _ = io.submit(Op::UserInputAnswer { id, response }).await; - return; - } - - let args = RequestUserInputArgs { - questions: event.questions, - is_blocking: event.is_blocking, - auto_resolution_ms: event.auto_resolution_ms, - }; - let response_fut = - parent_session.request_user_input(parent_ctx, parent_ctx.sub_id.clone(), args); - let response = await_user_input_with_cancel( - response_fut, - parent_session, - &parent_ctx.sub_id, - cancel_token, - ) - .await; - let _ = io.submit(Op::UserInputAnswer { id, response }).await; -} - -/// Intercepts delegated legacy MCP approval prompts on the RequestUserInput -/// compatibility path and, when guardian is active, answers them -/// programmatically after running the guardian review. -/// -/// The RequestUserInput event only carries `call_id` plus approval question -/// metadata, so this helper joins it back to the child runtime metadata cached at -/// `McpToolCallBegin` in order to rebuild the full guardian review request. -async fn maybe_auto_review_mcp_request_user_input( - parent_session: &Arc, - parent_ctx: &Arc, - pending_mcp_invocations: &Arc>>, - event: &RequestUserInputEvent, - cancel_token: &CancellationToken, -) -> Option { - // TODO(ccunningham): Support delegated MCP approval elicitations here too after - // coordinating with @fouad. Today guardian only auto-reviews the RequestUserInput - // compatibility path for delegated MCP approvals. - let question = event - .questions - .iter() - .find(|question| is_mcp_tool_approval_question_id(&question.id))?; - let pending = pending_mcp_invocations - .lock() - .await - .get(&event.call_id) - .cloned()?; - let invocation = pending.invocation; - let metadata = pending.metadata; - 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), - Arc::clone(parent_ctx), - new_guardian_review_id(), - build_guardian_mcp_tool_review_request(&event.call_id, &invocation, metadata.as_ref()), - /*retry_reason*/ None, - GuardianReviewOptions { - plugin_attribution_override: None, - approval_request_source: GuardianApprovalRequestSource::DelegatedSubagent, - external_cancel: Some(review_cancel.clone()), - }, - ); - let decision = await_approval_with_cancel( - receive_approval_review(review_rx), - parent_session, - &event.call_id, - cancel_token, - Some(&review_cancel), - ) - .await; - let selected_label = match decision { - ReviewDecision::ApprovedForSession => question - .options - .as_ref() - .and_then(|options| { - options - .iter() - .find(|option| option.label == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION) - }) - .map(|option| option.label.clone()) - .unwrap_or_else(|| MCP_TOOL_APPROVAL_ACCEPT.to_string()), - ReviewDecision::Approved - | ReviewDecision::ApprovedMcpPolicyAmendment - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::NetworkPolicyAmendment { .. } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), - ReviewDecision::Denied { .. } | ReviewDecision::TimedOut | ReviewDecision::Abort => { - MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string() - } - }; - Some(RequestUserInputResponse { - answers: HashMap::from([( - question.id.clone(), - codex_protocol::request_user_input::RequestUserInputAnswer { - answers: vec![selected_label], - }, - )]), - }) -} - -async fn handle_request_permissions( - io: &SessionIo, - parent_session: &Arc, - parent_ctx: &Arc, - event: RequestPermissionsEvent, - cancel_token: &CancellationToken, -) { - let call_id = event.call_id; - let args = RequestPermissionsArgs { - environment_id: event.environment_id, - reason: event.reason, - permissions: event.permissions, - }; - let cwd = event.cwd.unwrap_or_else(|| { - #[allow(deprecated)] - parent_ctx.cwd.clone() - }); - let response_fut = parent_session.request_permissions_for_cwd( - parent_ctx, - call_id.clone(), - args, - cwd, - cancel_token.clone(), - ); - let response = - await_request_permissions_with_cancel(response_fut, parent_session, &call_id, cancel_token) - .await; - let _ = io - .submit(Op::RequestPermissionsResponse { - id: call_id, - response, - }) - .await; -} - -async fn await_user_input_with_cancel( - fut: F, - parent_session: &Session, - sub_id: &str, - cancel_token: &CancellationToken, -) -> RequestUserInputResponse -where - F: core::future::Future>, -{ - tokio::select! { - biased; - _ = cancel_token.cancelled() => { - let empty = RequestUserInputResponse { - answers: HashMap::new(), - }; - parent_session - .notify_user_input_response(sub_id, empty.clone()) - .await; - empty - } - response = fut => response.unwrap_or_else(|| RequestUserInputResponse { - answers: HashMap::new(), - }), - } -} - -async fn await_request_permissions_with_cancel( - fut: F, - parent_session: &Session, - call_id: &str, - cancel_token: &CancellationToken, -) -> RequestPermissionsResponse -where - F: core::future::Future>, -{ - tokio::select! { - biased; - _ = cancel_token.cancelled() => { - let empty = RequestPermissionsResponse { - permissions: Default::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }; - parent_session - .notify_request_permissions_response(call_id, empty.clone()) - .await; - empty - } - response = fut => response.unwrap_or_else(|| RequestPermissionsResponse { - permissions: Default::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }), - } -} - -async fn receive_approval_review(review_rx: oneshot::Receiver) -> ReviewDecision { - review_rx - .await - .unwrap_or_else(|_| ReviewDecision::denied("automatic approval review could not complete")) -} - -/// Await an approval decision, aborting on cancellation. -async fn await_approval_with_cancel( - fut: F, - parent_session: &Session, - approval_id: &str, - cancel_token: &CancellationToken, - review_cancel_token: Option<&CancellationToken>, -) -> codex_protocol::protocol::ReviewDecision -where - F: core::future::Future, -{ - tokio::select! { - biased; - _ = cancel_token.cancelled() => { - if let Some(review_cancel_token) = review_cancel_token { - review_cancel_token.cancel(); - } - parent_session - .notify_approval( - approval_id, - codex_protocol::protocol::ReviewDecision::Abort, - ) - .await; - codex_protocol::protocol::ReviewDecision::Abort - } - decision = fut => { - decision - } - } -} - #[cfg(test)] #[path = "codex_delegate_tests.rs"] mod tests; diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 8a817934e8..159f026e05 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -1,61 +1,25 @@ use super::*; -use crate::environment_selection::TurnEnvironmentState; -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::error::CodexErrorDetails; -use codex_protocol::models::NetworkPermissions; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AgentStatus; -use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::ExecApprovalRequestEvent; -use codex_protocol::protocol::GuardianAssessmentAction; -use codex_protocol::protocol::GuardianAssessmentStatus; -use codex_protocol::protocol::GuardianCommandSource; -use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::McpStartupCompleteEvent; use codex_protocol::protocol::McpStartupStatus; use codex_protocol::protocol::McpStartupUpdateEvent; use codex_protocol::protocol::RawResponseItemEvent; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnAbortedEvent; -use codex_protocol::request_permissions::RequestPermissionProfile; -use codex_protocol::request_permissions::RequestPermissionsEvent; -use codex_protocol::request_permissions::RequestPermissionsResponse; -use codex_protocol::request_user_input::RequestUserInputAnswer; -use codex_protocol::request_user_input::RequestUserInputEvent; -use codex_protocol::request_user_input::RequestUserInputQuestion; -use core_test_support::PathBufExt; -use core_test_support::test_path_buf; use pretty_assertions::assert_eq; -use std::collections::HashMap; use std::sync::Arc; -use tokio::sync::Mutex; -use tokio::sync::oneshot; use tokio::sync::watch; use tokio::time::timeout; -#[tokio::test] -async fn dropped_approval_review_fails_closed() { - let (tx, rx) = oneshot::channel(); - drop(tx); - - assert_eq!( - receive_approval_review(rx).await, - ReviewDecision::denied("automatic approval review could not complete") - ); -} - #[tokio::test] async fn forward_events_filters_private_events_before_blocked_send_is_cancelled() { let (tx_events, rx_events) = bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); - let (session, ctx, _rx_evt) = crate::session::tests::make_session_and_context_with_rx().await; let io = Arc::new(SessionIo { tx_sub, rx_event: rx_events, @@ -81,11 +45,7 @@ async fn forward_events_filters_private_events_before_blocked_send_is_cancelled( let cancel = CancellationToken::new(); let forward = tokio::spawn(forward_events( Arc::clone(&io), - Arc::clone(&session), tx_out.clone(), - session, - ctx, - Arc::new(Mutex::new(HashMap::new())), cancel.clone(), )); @@ -207,6 +167,8 @@ async fn forward_ops_preserves_submission_trace_context() { async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() { let (parent_session, parent_ctx, _rx_events) = crate::session::tests::make_session_and_context_with_rx().await; + let mut config = parent_ctx.config.as_ref().clone(); + config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); let cancel_token = CancellationToken::new(); cancel_token.cancel(); let parent_environments = parent_ctx.environments.clone(); @@ -214,7 +176,7 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() { let result = timeout( Duration::from_secs(/*secs*/ 1), run_codex_thread_interactive( - parent_ctx.config.as_ref().clone(), + config, Arc::clone(&parent_session.services.auth_manager), Arc::clone(&parent_session.services.models_manager), parent_session, @@ -237,426 +199,35 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() { } #[tokio::test] -async fn handle_request_permissions_uses_tool_call_id_for_round_trip() { - let (parent_session, mut parent_ctx, rx_events) = - crate::session::tests::make_session_and_context_with_rx().await; - *parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); - let parent_ctx_mut = Arc::get_mut(&mut parent_ctx).expect("single turn context ref"); - let TurnEnvironmentState::Ready(environment) = &mut parent_ctx_mut.environments.environments[0] - else { - panic!("expected ready primary environment"); - }; - environment.environment_id = "remote".to_string(); - - let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); - let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY); - let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); - let io = Arc::new(SessionIo { - tx_sub, - rx_event: rx_events_child, - agent_status, - session_loop_termination: completed_session_loop_termination(), - }); - - let call_id = "tool-call-1".to_string(); - let expected_response = RequestPermissionsResponse { - permissions: RequestPermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - ..RequestPermissionProfile::default() - }, - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }; - #[allow(deprecated)] - let delegated_cwd = parent_ctx.cwd.join("delegated-cwd"); - let cancel_token = CancellationToken::new(); - let request_call_id = call_id.clone(); - let request_cwd = delegated_cwd.clone(); - - let handle = tokio::spawn({ - let io = Arc::clone(&io); - let parent_session = Arc::clone(&parent_session); - let parent_ctx = Arc::clone(&parent_ctx); - let cancel_token = cancel_token.clone(); - async move { - handle_request_permissions( - io.as_ref(), - &parent_session, - &parent_ctx, - RequestPermissionsEvent { - call_id: request_call_id, - turn_id: "child-turn-1".to_string(), - environment_id: Some("remote".to_string()), - started_at_ms: 0, - reason: Some("need access".to_string()), - permissions: RequestPermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - ..RequestPermissionProfile::default() - }, - cwd: Some(request_cwd), - }, - &cancel_token, - ) - .await; - } - }); - - let request_event = timeout(Duration::from_secs(1), rx_events.recv()) - .await - .expect("request_permissions event timed out") - .expect("request_permissions event missing"); - let EventMsg::RequestPermissions(request) = request_event.msg else { - panic!("expected RequestPermissions event"); - }; - assert_eq!(request.call_id, call_id.clone()); - assert_eq!(request.environment_id.as_deref(), Some("remote")); - assert_eq!(request.cwd, Some(delegated_cwd)); - - parent_session - .notify_request_permissions_response(&call_id, expected_response.clone()) - .await; - - timeout(Duration::from_secs(1), handle) - .await - .expect("handle_request_permissions hung") - .expect("handle_request_permissions join error"); - - let submission = timeout(Duration::from_secs(1), rx_sub.recv()) - .await - .expect("request_permissions response timed out") - .expect("request_permissions response missing"); - let Op::RequestPermissionsResponse { id, response } = submission.op else { - panic!("expected request permissions response"); - }; - assert_eq!(id, call_id); - assert_eq!(response, expected_response); -} - -#[tokio::test] -async fn handle_request_user_input_preserves_non_blocking_flag_for_round_trip() { - let (parent_session, parent_ctx, rx_events) = - crate::session::tests::make_session_and_context_with_rx().await; - *parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); - - let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); - let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY); - let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); - let io = Arc::new(SessionIo { - tx_sub, - rx_event: rx_events_child, - agent_status, - session_loop_termination: completed_session_loop_termination(), - }); - let pending_mcp_invocations = Arc::new(Mutex::new(HashMap::new())); - let cancel_token = CancellationToken::new(); - let child_event_id = "child-request-1".to_string(); - let parent_user_input_id = parent_ctx.sub_id.clone(); - let expected_response = RequestUserInputResponse { - answers: HashMap::from([( - "q1".to_string(), - RequestUserInputAnswer { - answers: vec!["yes".to_string()], - }, - )]), - }; - - let handle = tokio::spawn({ - let io = Arc::clone(&io); - let parent_session = Arc::clone(&parent_session); - let parent_ctx = Arc::clone(&parent_ctx); - let pending_mcp_invocations = Arc::clone(&pending_mcp_invocations); - let cancel_token = cancel_token.clone(); - let child_event_id = child_event_id.clone(); - async move { - handle_request_user_input( - io.as_ref(), - child_event_id, - &parent_session, - &parent_ctx, - &pending_mcp_invocations, - RequestUserInputEvent { - call_id: "child-call-1".to_string(), - turn_id: "child-turn-1".to_string(), - questions: vec![RequestUserInputQuestion { - id: "q1".to_string(), - header: "Confirm".to_string(), - question: "Continue?".to_string(), - is_other: false, - is_secret: false, - options: None, - }], - is_blocking: false, - auto_resolution_ms: None, - }, - &cancel_token, - ) - .await; - } - }); - - let request_event = timeout(Duration::from_secs(1), rx_events.recv()) - .await - .expect("request_user_input event timed out") - .expect("request_user_input event missing"); - let EventMsg::RequestUserInput(request) = request_event.msg else { - panic!("expected RequestUserInput event"); - }; - assert_eq!(request.call_id, parent_user_input_id); - assert_eq!(request.is_blocking, false); - - parent_session - .notify_user_input_response(&parent_user_input_id, expected_response.clone()) - .await; - - timeout(Duration::from_secs(1), handle) - .await - .expect("handle_request_user_input hung") - .expect("handle_request_user_input join error"); - - let submission = timeout(Duration::from_secs(1), rx_sub.recv()) - .await - .expect("request_user_input response timed out") - .expect("request_user_input response missing"); - let Op::UserInputAnswer { id, response } = submission.op else { - panic!("expected user input answer"); - }; - assert_eq!(id, child_event_id); - assert_eq!(response, expected_response); -} - -#[tokio::test] -async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_for_reply() { - let (parent_session, parent_ctx, rx_events) = - crate::session::tests::make_session_and_context_with_rx().await; - let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref"); - let mut config = (*parent_ctx.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::AutoReview; - parent_ctx.config = Arc::new(config); - Arc::make_mut(&mut parent_ctx.config) - .permissions - .approval_policy - .set(AskForApproval::OnRequest) - .expect("set on-request policy"); - let parent_ctx = Arc::new(parent_ctx); - - let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); - let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY); - let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); - let io = Arc::new(SessionIo { - tx_sub, - rx_event: rx_events_child, - agent_status, - session_loop_termination: completed_session_loop_termination(), - }); - - let cancel_token = CancellationToken::new(); - let handle = tokio::spawn({ - let io = Arc::clone(&io); - let parent_session = Arc::clone(&parent_session); - let parent_ctx = Arc::clone(&parent_ctx); - let cancel_token = cancel_token.clone(); - async move { - handle_exec_approval( - io.as_ref(), - "child-turn-1".to_string(), - &parent_session, - &parent_ctx, - ExecApprovalRequestEvent { - call_id: "command-item-1".to_string(), - plugin_id: Some("sample@openai-curated".to_string()), - script_path: Some("scripts/run.py".to_string()), - approval_id: Some("callback-approval-1".to_string()), - turn_id: "child-turn-1".to_string(), - environment_id: Some("remote".to_string()), - started_at_ms: 0, - command: vec!["rm".to_string(), "-rf".to_string(), "tmp".to_string()], - cwd: test_path_buf("/tmp").abs(), - reason: Some("unsafe subcommand".to_string()), - network_approval_context: None, - proposed_execpolicy_amendment: None, - proposed_network_policy_amendments: None, - additional_permissions: None, - available_decisions: Some(vec![ - ReviewDecision::Approved, - ReviewDecision::Abort, - ]), - parsed_cmd: Vec::new(), - }, - &cancel_token, - ) - .await; - } - }); - - let assessment_event = timeout(Duration::from_secs(2), async { - loop { - let event = rx_events.recv().await.expect("guardian assessment event"); - if let EventMsg::GuardianAssessment(assessment) = event.msg { - return assessment; - } - } - }) - .await - .expect("timed out waiting for guardian assessment"); - let expected_action = GuardianAssessmentAction::Command { - source: GuardianCommandSource::Shell, - command: "rm -rf tmp".to_string(), - cwd: test_path_buf("/tmp").abs(), - }; - assert!(!assessment_event.id.is_empty()); - assert_eq!( - assessment_event.target_item_id.as_deref(), - Some("command-item-1") - ); - assert_eq!( - assessment_event.plugin_id.as_deref(), - Some("sample@openai-curated") - ); - assert_eq!( - assessment_event.script_path.as_deref(), - Some("scripts/run.py") - ); - assert_eq!(assessment_event.turn_id, parent_ctx.sub_id); - assert_eq!( - assessment_event.status, - GuardianAssessmentStatus::InProgress - ); - assert_eq!(assessment_event.risk_level, None); - assert_eq!(assessment_event.user_authorization, None); - assert_eq!(assessment_event.rationale, None); - assert_eq!(assessment_event.decision_source, None); - assert_eq!(assessment_event.action, expected_action); - - cancel_token.cancel(); - - timeout(Duration::from_secs(2), handle) - .await - .expect("handle_exec_approval hung") - .expect("handle_exec_approval join error"); - - let submission = timeout(Duration::from_secs(2), rx_sub.recv()) - .await - .expect("exec approval response timed out") - .expect("exec approval response missing"); - let Op::ExecApproval { - id, - turn_id, - decision, - } = submission.op - else { - panic!("expected exec approval"); - }; - assert_eq!(id, "callback-approval-1"); - assert_eq!(turn_id, Some("child-turn-1".to_string())); - assert_eq!(decision, ReviewDecision::Abort); -} - -#[tokio::test] -async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() { +async fn run_codex_thread_interactive_rejects_approval_policy_that_can_prompt() { let (parent_session, parent_ctx, _rx_events) = crate::session::tests::make_session_and_context_with_rx().await; - let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref"); - let mut config = (*parent_ctx.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::AutoReview; - parent_ctx.config = Arc::new(config); - Arc::make_mut(&mut parent_ctx.config) - .permissions - .approval_policy - .set(AskForApproval::OnRequest) - .expect("set on-request policy"); - let parent_ctx = Arc::new(parent_ctx); + let mut config = parent_ctx.config.as_ref().clone(); + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + let parent_environments = parent_ctx.environments.clone(); - let pending_mcp_invocations = Arc::new(Mutex::new(HashMap::from([( - "call-1".to_string(), - PendingMcpInvocation { - invocation: McpInvocation { - server: "custom_server".to_string(), - tool: "dangerous_tool".to_string(), - arguments: None, - }, - metadata: None, - }, - )]))); - let cancel_token = CancellationToken::new(); - cancel_token.cancel(); - - let response = maybe_auto_review_mcp_request_user_input( - &parent_session, - &parent_ctx, - &pending_mcp_invocations, - &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, - }], - is_blocking: true, - auto_resolution_ms: None, - }, - &cancel_token, + let result = run_codex_thread_interactive( + config, + Arc::clone(&parent_session.services.auth_manager), + Arc::clone(&parent_session.services.models_manager), + parent_session, + parent_ctx, + parent_environments, + CancellationToken::new(), + SubAgentSource::Review, + /*initial_history*/ None, + crate::session::GitEnrichmentPolicy::Fresh, + codex_sandboxing::WindowsSandboxProxySettingsMode::Reconcile, ) .await; - assert_eq!( - response, - Some(RequestUserInputResponse { - answers: HashMap::from([( - format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_call-1"), - RequestUserInputAnswer { - answers: vec![MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string()], - }, - )]), - }) - ); -} - -#[tokio::test] -async fn delegated_mcp_user_reviewer_returns_none_without_metadata() { - 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(), - PendingMcpInvocation { - invocation: McpInvocation { - server: CODEX_APPS_MCP_SERVER_NAME.to_string(), - tool: "dangerous_tool".to_string(), - arguments: None, - }, - metadata: None, - }, - )]))); - let cancel_token = CancellationToken::new(); - - 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, - }], - is_blocking: true, - auto_resolution_ms: None, - }; - let response = maybe_auto_review_mcp_request_user_input( - &parent_session, - &parent_ctx, - &pending_mcp_invocations, - &event, - &cancel_token, - ) - .await; - assert_eq!(response, None); + assert!(matches!( + result, + Err(err) + if matches!( + err.details(), + CodexErrorDetails::InvalidRequest(message) + if message == "Codex delegates require approval policy `never`" + ) + )); } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 8a10228587..1b17e0e6c4 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -504,21 +504,6 @@ pub fn with_app_plugin_sources( connectors } -pub(crate) fn mcp_approvals_reviewer( - config: &Config, - model: Option<&str>, - server_name: &str, - connector_id: Option<&str>, -) -> ApprovalsReviewer { - mcp_approvals_reviewer_from_layers( - &config.config_layer_stack, - config.approvals_reviewer, - model, - server_name, - connector_id, - ) -} - pub(crate) fn mcp_approvals_reviewer_from_layers( config_layer_stack: &codex_config::ConfigLayerStack, default_reviewer: ApprovalsReviewer, diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index 44c8fbce82..8258f44763 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -345,8 +345,9 @@ approvals_reviewer = "{app}" .expect("config should build"); assert_eq!( - mcp_approvals_reviewer( - &config, + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, Some("calendar") @@ -354,8 +355,9 @@ approvals_reviewer = "{app}" expected_app ); assert_eq!( - mcp_approvals_reviewer( - &config, + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, Some("drive") @@ -363,8 +365,9 @@ approvals_reviewer = "{app}" expected_default ); assert_eq!( - mcp_approvals_reviewer( - &config, + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, /*connector_id*/ None @@ -372,8 +375,9 @@ approvals_reviewer = "{app}" expected_default ); assert_eq!( - mcp_approvals_reviewer( - &config, + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, config.model.as_deref(), "custom_server", Some("calendar") @@ -408,8 +412,9 @@ approvals_reviewer = "user" .expect("config should build"); assert_eq!( - mcp_approvals_reviewer( - &config, + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, Some("calendar") @@ -443,8 +448,9 @@ approvals_reviewer = "user" .expect("config should build"); assert_eq!( - mcp_approvals_reviewer( - &config, + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, Some("calendar") diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 42168e0cbf..bdfc77f2fe 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -44,7 +44,6 @@ pub(crate) use review::review_approval_request; pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::routes_approval_policy_to_guardian; 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/tests.rs b/codex-rs/core/src/guardian/tests.rs index 8aedea8fce..2c4ab5c5af 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -12,6 +12,7 @@ use crate::guardian::prompt::BUNDLED_GUARDIAN_POLICY; use crate::guardian::prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE; use crate::guardian::prompt::guardian_policy_prompt_with_config_and_template; use crate::guardian::review::guardian_review_session_config; +use crate::guardian::review::routes_approval_to_guardian_with_reviewer; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::session::turn_context::TurnEnvironment; diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index ce80db17db..b9c257b45c 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -7,6 +7,7 @@ use codex_config::McpServerTransportConfig; use codex_config::load_global_mcp_servers; use codex_login::default_client::is_first_party_originator; use codex_login::default_client::originator; +use codex_protocol::protocol::AskForApproval; use codex_protocol::request_user_input::RequestUserInputArgs; use codex_protocol::request_user_input::RequestUserInputQuestion; use codex_protocol::request_user_input::RequestUserInputQuestionOption; @@ -246,6 +247,10 @@ async fn should_install_mcp_dependencies( return true; } + if turn_context.approval_policy() == AskForApproval::Never { + return false; + } + let server_list = format_missing_mcp_dependencies(missing); let question = RequestUserInputQuestion { id: SKILL_MCP_DEPENDENCY_PROMPT_ID.to_string(), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 01c1e2bcbb..7e408810e3 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -26,7 +26,6 @@ use codex_analytics::build_track_events_context; use codex_api::HostedFileUploadContext; use codex_config::ConfigLayerSource; use codex_config::types::AppToolApproval; -use codex_config::types::ApprovalsReviewer; use codex_connectors::AppToolPolicy; use codex_connectors::AppToolPolicyEvaluator; use codex_connectors::AppToolPolicyInput; @@ -1264,20 +1263,9 @@ struct McpToolApprovalElicitationRequest<'a> { pub(crate) const MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX: &str = "mcp_tool_call_approval"; pub(crate) const MCP_TOOL_APPROVAL_ACCEPT: &str = "Allow"; pub(crate) const MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION: &str = "Allow for this session"; -// Internal-only token used when guardian auto-reviews delegated MCP approvals on the -// RequestUserInput compatibility path. That legacy MCP prompt has allow/cancel labels but no -// real "Decline" answer, so this lets guardian denials round-trip distinctly from user cancel. -// This is not a user-facing option. -pub(crate) const MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC: &str = "__codex_mcp_decline__"; const MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER: &str = "Allow and don't ask me again"; const MCP_TOOL_APPROVAL_CANCEL: &str = "Cancel"; -pub(crate) fn is_mcp_tool_approval_question_id(question_id: &str) -> bool { - question_id - .strip_prefix(MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX) - .is_some_and(|suffix| suffix.starts_with('_')) -} - #[derive(Clone, Debug, PartialEq, Eq, Serialize)] struct McpToolApprovalKey { server: String, @@ -1407,6 +1395,7 @@ pub(crate) async fn request_mcp_tool_user_approval( connected_account_email, tool_title, tool_description, + approval_policy, approval_mode, allow_session_remember, allow_persistent_approval, @@ -1416,6 +1405,12 @@ pub(crate) async fn request_mcp_tool_user_approval( unreachable!("only MCP actions can request MCP tool approval"); }; + if *approval_policy == AskForApproval::Never { + return ReviewDecision::denied( + "MCP tool call requires approval, but approval policy is never", + ); + } + let tool_call_mcp_elicitation_enabled = turn_context .config .features @@ -1501,19 +1496,6 @@ pub(crate) async fn request_mcp_tool_user_approval( ) } -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(), - Some(turn_context.model_info.slug.as_str()), - server_name, - metadata.and_then(|metadata| metadata.connector_id.as_deref()), - ) -} - fn session_mcp_tool_approval_key( invocation: &McpInvocation, metadata: Option<&McpToolApprovalMetadata>, @@ -1938,11 +1920,6 @@ fn parse_mcp_tool_approval_response( return ReviewDecision::Abort; }; if answers - .iter() - .any(|answer| answer == MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC) - { - ReviewDecision::denied("user rejected MCP tool call") - } else if answers .iter() .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION) { diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 427a8d2efc..cad340d7c0 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1822,26 +1822,6 @@ fn declined_elicitation_response_stays_decline() { ); } -#[test] -fn synthetic_decline_request_user_input_response_stays_decline() { - let response = parse_mcp_tool_approval_response( - Some(RequestUserInputResponse { - answers: HashMap::from([( - "approval".to_string(), - RequestUserInputAnswer { - answers: vec![MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string()], - }, - )]), - }), - "approval", - ); - - assert_eq!( - response, - ReviewDecision::denied("user rejected MCP tool call") - ); -} - #[test] fn accepted_elicitation_response_uses_always_persist_meta() { let response = parse_mcp_tool_approval_elicitation_response( diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 3b51352b72..5dd12bc91b 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2667,40 +2667,6 @@ impl Session { } } - pub(crate) async fn request_permissions_for_cwd( - self: &Arc, - turn_context: &Arc, - call_id: String, - args: RequestPermissionsArgs, - cwd: AbsolutePathBuf, - cancellation_token: CancellationToken, - ) -> Option { - let turn_environment = match args.environment_id.as_deref() { - Some(environment_id) => turn_context - .environments - .turn_environments() - .find(|environment| environment.environment_id == environment_id), - None => turn_context.environments.primary(), - }; - let Some(turn_environment) = turn_environment else { - return Some(RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }); - }; - let mut environment = turn_environment.selection(); - environment.cwd = PathUri::from_abs_path(&cwd); - self.request_permissions_for_environment( - turn_context, - call_id, - args, - environment, - cancellation_token, - ) - .await - } - #[expect( clippy::await_holding_invalid_type, reason = "active turn checks and turn state updates must remain atomic" diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index c0b437de53..63d6435c10 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -1,15 +1,34 @@ +use codex_core::config::Constrained; +use codex_exec_server::CreateDirectoryOptions; +use codex_features::Feature; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::ReviewTarget; +use codex_protocol::user_input::UserInput; +use codex_utils_path_uri::PathUri; +use core_test_support::PathExt; +use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL; +use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE; +use core_test_support::apps_test_server::recorded_apps_tool_calls; +use core_test_support::apps_test_server::search_capable_apps_builder; +use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_reasoning_item_added; use core_test_support::responses::ev_reasoning_summary_text_delta; use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; @@ -58,3 +77,395 @@ async fn codex_delegate_ignores_legacy_deltas() { assert_eq!(reasoning_delta_count, 1, "expected one new reasoning delta"); } + +#[cfg_attr(windows, ignore = "no exec_command on Windows")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn codex_delegate_rejects_escalation_requests_when_parent_can_prompt() { + skip_if_no_network!(); + + let call_id = "review-escalation-call"; + let command = serde_json::json!({ + "cmd": "echo review", + "sandbox_permissions": "require_escalated" + }); + let server = start_mock_server().await; + let response_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "exec_command", &command.to_string()), + ev_completed("resp-1"), + ]), + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ], + ) + .await; + + let test = test_codex() + .with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + }) + .build_with_auto_env(&server) + .await + .expect("build review delegate with escalation support"); + + test.codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Review without requesting approval".to_string(), + }, + user_facing_hint: None, + }, + }) + .await + .expect("submit review"); + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + matches!(event, EventMsg::TurnComplete(_)), + "review delegate should reject escalation requests without prompting: {event:?}" + ); + + let requests = response_mock.requests(); + let [_, completion_request] = requests.as_slice() else { + panic!("expected the model request and escalation-denial continuation"); + }; + let output = completion_request.function_call_output(call_id); + let response = output["output"] + .as_str() + .expect("command tool output should be a string"); + assert!( + response.contains("approval policy is Never"), + "escalation should be rejected by the delegate's approval policy: {response}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn codex_delegate_rejects_legacy_mcp_approvals_without_prompting() { + skip_if_no_network!(); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server) + .await + .expect("mount mock app server"); + let call_id = "review-calendar-call"; + let arguments = serde_json::json!({ + "title": "Review meeting", + "starts_at": "2026-08-12T12:00:00Z" + }); + let response_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call_with_namespace( + call_id, + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL, + &arguments.to_string(), + ), + ev_completed("resp-1"), + ]), + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ], + ) + .await; + + let test = search_capable_apps_builder(apps_server.chatgpt_base_url) + .with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("review delegate should inherit a restricted permission profile"); + let config_path = config.codex_home.join("config.toml").abs(); + let app_config = toml::from_str( + r#" +[apps.calendar] +default_tools_approval_mode = "prompt" +"#, + ) + .expect("app approval configuration should parse"); + config.config_layer_stack = config + .config_layer_stack + .with_user_config(&config_path, app_config) + .expect("app approval configuration should be valid"); + config + .features + .disable(Feature::ToolCallMcpElicitation) + .expect("legacy MCP approvals should be available"); + }) + .build_with_auto_env(&server) + .await + .expect("build review delegate with legacy MCP approvals"); + + test.codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Review the [$calendar](app://calendar) integration".to_string(), + }, + user_facing_hint: None, + }, + }) + .await + .expect("submit review"); + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::RequestUserInput(_) + | EventMsg::ElicitationRequest(_) + | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + matches!(event, EventMsg::TurnComplete(_)), + "review delegate should reject legacy MCP approvals without prompting: {event:?}" + ); + + let requests = response_mock.requests(); + let [_, completion_request] = requests.as_slice() else { + panic!("expected the model request and MCP-denial continuation"); + }; + let output = completion_request.function_call_output(call_id); + let response = output["output"] + .as_str() + .expect("MCP tool output should be a string"); + assert!( + response.contains("approval policy is never"), + "MCP tool should be rejected by the delegate's approval policy: {response}" + ); + assert!( + recorded_apps_tool_calls(&server).await.is_empty(), + "MCP tool requiring approval should never execute" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn codex_delegate_rejects_skill_mcp_dependency_installation_without_prompting() { + skip_if_wine_exec!("skill paths require matching host and executor path conventions"); + skip_if_no_network!(); + + let server = start_mock_server().await; + let response_mock = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + + let test = test_codex() + .with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("review delegate should inherit a restricted permission profile"); + config + .features + .enable(Feature::SkillMcpDependencyInstall) + .expect("skill MCP dependency installation should be available"); + }) + .with_workspace_setup(|cwd, fs| async move { + let skill_dir = cwd.join(".agents/skills/dependency-skill"); + let agents_dir = skill_dir.join("agents"); + fs.create_directory( + &PathUri::from_host_native_path(&agents_dir)?, + CreateDirectoryOptions { recursive: true }, + /*sandbox*/ None, + ) + .await?; + fs.write_file( + &PathUri::from_host_native_path(skill_dir.join("SKILL.md"))?, + b"---\nname: dependency-skill\ndescription: Requires an MCP server.\n---\n\nReview dependency instructions.\n" + .to_vec(), + /*sandbox*/ None, + ) + .await?; + fs.write_file( + &PathUri::from_host_native_path(agents_dir.join("openai.yaml"))?, + b"dependencies:\n tools:\n - type: mcp\n value: missing-review-server\n transport: streamable_http\n url: http://127.0.0.1:1/mcp\n" + .to_vec(), + /*sandbox*/ None, + ) + .await?; + Ok(()) + }) + .build_with_auto_env(&server) + .await + .expect("build review delegate with a missing skill MCP dependency"); + + test.codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Review with $dependency-skill".to_string(), + }, + user_facing_hint: None, + }, + }) + .await + .expect("submit review"); + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::RequestUserInput(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + matches!(event, EventMsg::TurnComplete(_)), + "review delegate should reject skill MCP dependency installation without prompting: {event:?}" + ); + + let request = response_mock.single_request(); + let user_texts = request.message_input_texts("user"); + assert!( + user_texts.iter().any(|text| { + text.contains("\ndependency-skill") + && text.contains("Review dependency instructions.") + }), + "review should continue with the selected skill after rejecting its MCP dependency: {user_texts:?}" + ); +} + +#[cfg_attr(windows, ignore = "no exec_command on Windows")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_delegate_rejects_escalation_requests_without_prompting() { + skip_if_wine_exec!("Guardian approval actions require host-native paths"); + skip_if_no_network!(); + + let server = start_mock_server().await; + let test = test_codex() + .with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("guardian delegate should inherit a restricted permission profile"); + }) + .build_with_auto_env(&server) + .await + .expect("build guardian delegate with escalation support"); + + let parent_call_id = "parent-escalation-call"; + let guardian_call_id = "guardian-escalation-call"; + let guardian_output_file = test.cwd.path().join("guardian-escalation-marker.txt"); + let parent_command = serde_json::json!({ + "cmd": "echo parent command", + "sandbox_permissions": "require_escalated", + "justification": "Trigger Guardian approval review." + }); + let guardian_command = serde_json::json!({ + "cmd": format!("echo guardian-ran > \"{}\"", guardian_output_file.display()), + "sandbox_permissions": "require_escalated", + "justification": "Guardian must not escalate its own commands." + }); + let assessment = serde_json::json!({ + "risk_level": "high", + "user_authorization": "low", + "outcome": "deny", + "rationale": "Guardian could not execute an escalated command." + }); + let response_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-parent-command"), + ev_function_call(parent_call_id, "exec_command", &parent_command.to_string()), + ev_completed("resp-parent-command"), + ]), + sse(vec![ + ev_response_created("resp-guardian-command"), + ev_function_call( + guardian_call_id, + "exec_command", + &guardian_command.to_string(), + ), + ev_completed("resp-guardian-command"), + ]), + sse(vec![ + ev_response_created("resp-guardian-assessment"), + ev_assistant_message("msg-guardian-assessment", &assessment.to_string()), + ev_completed("resp-guardian-assessment"), + ]), + sse(vec![ + ev_response_created("resp-parent-denied"), + ev_assistant_message("msg-parent-denied", "denied"), + ev_completed("resp-parent-denied"), + ]), + ], + ) + .await; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "Trigger Guardian review of an escalated command".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + ..Default::default() + }, + }) + .await + .expect("submit guardian-reviewed command"); + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + matches!(event, EventMsg::TurnComplete(_)), + "guardian delegate should reject escalation requests without prompting: {event:?}" + ); + + let requests = response_mock.requests(); + let guardian_requests = requests + .iter() + .filter(|request| { + request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian") + }) + .collect::>(); + assert_eq!(guardian_requests.len(), 2); + let guardian_output = guardian_requests + .iter() + .find_map(|request| request.function_call_output_text(guardian_call_id)) + .expect("guardian continuation should include the rejected command output"); + assert!( + guardian_output.contains("approval policy is Never"), + "guardian escalation should be rejected by its never approval policy: {guardian_output}" + ); + assert!( + !guardian_output_file.exists(), + "guardian command requiring approval should never execute" + ); + + let parent_output = requests + .iter() + .find_map(|request| request.function_call_output_text(parent_call_id)) + .expect("parent continuation should include the guardian denial"); + assert!( + parent_output.contains("Guardian could not execute an escalated command."), + "guardian denial rationale should reach the parent: {parent_output}" + ); +}