From a26f1806a4f4b8cfec2ea1be129963815a61e58c Mon Sep 17 00:00:00 2001 From: Abhinav Date: Wed, 26 Aug 2026 06:39:58 +0000 Subject: [PATCH] Use issuing step settings for approval decisions (#40821) ## Why Tool actions and approval retries can outlive the step that issued them. If approval handling reads newer turn settings, a delayed action can use the wrong approval policy or reviewer. ## What changed - Capture the approval policy and reviewer in `GuardianReviewContext` from the issuing step. - Use step settings for command, patch, permission, sandbox-retry, and remote-network approval decisions. - Use the active task's published settings for background network approvals and long-lived Unix shell interception. - Preserve explicit MCP reviewer overrides. ## Testing Added regression coverage for permission requests, same-turn network policy changes, delayed commands after a new turn starts, sandbox retries, patch policy selection, and MCP reviewer overrides. GitOrigin-RevId: b7e84a2e1e79a71502272b15b5ef5b0e5643f8b0 --- codex-rs/core/src/apply_patch.rs | 6 +- codex-rs/core/src/apply_patch_tests.rs | 48 ++ codex-rs/core/src/guardian/mod.rs | 48 +- .../src/session/elicitation_holders_tests.rs | 4 +- codex-rs/core/src/session/mod.rs | 23 +- codex-rs/core/src/session/tests.rs | 8 +- .../core/src/session/tests/guardian_tests.rs | 467 +++++++++++++++++- codex-rs/core/src/tools/approvals.rs | 10 +- codex-rs/core/src/tools/approvals_tests.rs | 58 +++ .../core/src/tools/handlers/apply_patch.rs | 10 +- .../handlers/unified_exec/exec_command.rs | 10 +- codex-rs/core/src/tools/network_approval.rs | 12 +- codex-rs/core/src/tools/orchestrator.rs | 6 +- .../core/src/tools/runtimes/unified_exec.rs | 8 +- .../runtimes/zsh_fork/unix_escalation.rs | 9 +- .../core/src/unified_exec/process_manager.rs | 4 +- 16 files changed, 671 insertions(+), 60 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 141581d0bc..1283d0cd9c 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,7 +1,7 @@ use crate::function_tool::FunctionCallError; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; -use crate::session::turn_context::TurnContext; +use crate::session::step_context::StepContext; use crate::session::turn_context::TurnEnvironment; use crate::tools::sandboxing::ExecApprovalRequirement; use codex_apply_patch::ApplyPatchAction; @@ -20,14 +20,14 @@ pub(crate) struct ApplyPatchRuntimeInvocation { } pub(crate) fn prepare_apply_patch( - turn_context: &TurnContext, + step_context: &StepContext, turn_environment: &TurnEnvironment, file_system_sandbox_policy: &FileSystemSandboxPolicy, action: ApplyPatchAction, ) -> Result { match assess_patch_safety( &action, - turn_context.approval_policy(), + step_context.settings.approval_policy(), turn_environment.permission_profile(), file_system_sandbox_policy, &action.cwd, diff --git a/codex-rs/core/src/apply_patch_tests.rs b/codex-rs/core/src/apply_patch_tests.rs index d845ca92b4..578873d3c6 100644 --- a/codex-rs/core/src/apply_patch_tests.rs +++ b/codex-rs/core/src/apply_patch_tests.rs @@ -1,6 +1,12 @@ use super::*; +use crate::session::tests::make_session_and_context; +use crate::session::tests::update_selected_settings_for_test; +use codex_protocol::models::PermissionProfile; +use codex_protocol::models::PermissionProfileSnapshot; +use codex_protocol::protocol::AskForApproval; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; +use std::sync::Arc; use tempfile::tempdir; @@ -20,3 +26,45 @@ fn convert_apply_patch_maps_add_variant() { }) ); } + +#[tokio::test] +async fn prepare_apply_patch_uses_action_policy_before_turn_policy() { + let (_, mut turn) = make_session_and_context().await; + Arc::make_mut(&mut turn.config) + .permissions + .approval_policy + .set(AskForApproval::Never) + .expect("set admitted turn approval policy"); + let mut step = StepContext::for_test(Arc::new(turn)); + let step_settings = &mut Arc::get_mut(&mut step) + .expect("unshared test step") + .settings; + update_selected_settings_for_test(Arc::make_mut(step_settings), |selected| { + selected + .approval_policy + .set(AskForApproval::OnRequest) + .expect("set captured approval policy"); + }); + let tmp = tempdir().expect("tmp"); + let path = tmp.path().join("outside.txt"); + let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); + let action = ApplyPatchAction::new_add_for_test(&path_uri, "hello".to_string()); + let permission_profile = PermissionProfile::read_only(); + let file_system_policy = permission_profile.file_system_sandbox_policy(); + let mut environment = step + .environments + .primary() + .expect("primary environment") + .clone(); + environment.config_mut().permission_profile = + PermissionProfileSnapshot::legacy(permission_profile); + + let prepared = prepare_apply_patch(&step, &environment, &file_system_policy, action) + .expect("issuing action policy should request approval"); + + assert!(!prepared.auto_approved); + assert!(matches!( + prepared.exec_approval_requirement, + ExecApprovalRequirement::NeedsApproval { .. } + )); +} diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 99bb8ae95a..6c63588a0b 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -20,12 +20,18 @@ mod review_session; use std::sync::Arc; use std::time::Duration; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::openai_models::ModelInfo; +use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GuardianAssessmentOutcome; use serde::Deserialize; use serde::Serialize; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::session::step_context::StepContext; +use crate::session::step_settings::ResolvedStepSettings; use crate::session::turn_context::TurnContext; use crate::tools::sandboxing::ApprovalRequestReasons; @@ -69,18 +75,44 @@ const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000; const GUARDIAN_RECENT_ENTRY_LIMIT: usize = 40; const TRUNCATION_TAG: &str = "truncated"; -/// Built from the originating StepContext when available. -/// There are currently two exceptions-- turn-only callers (background network approvals, reviewer -/// prewarming, etc.) and interactive Unix shells that can outlive the step that started them. +/// Captures review inputs from the issuing step without retaining its MCP bindings or tool router. +/// Background network approvals and Unix interception use the active task's resolved settings. +/// Startup reviewer prewarming intentionally uses turn-only inputs because it has no issuing step. +/// +/// MCP elicitation reviews still use turn-only inputs. /// TODO(sayan): See if we can find a way to model those as StepContext as well without holding /// step-scoped things past their lifetime (like MCP bindings) #[derive(Clone)] pub(crate) struct GuardianReviewContext { turn: Arc, environments: TurnEnvironmentSnapshot, + // Model and reasoning inputs are carried for the follow-up Guardian and V2 migrations. + #[expect(dead_code)] + pub(crate) model_info: Arc, + #[expect(dead_code)] + pub(crate) reasoning_effort: Option, + #[expect(dead_code)] + pub(crate) reasoning_summary: ReasoningSummary, + pub(crate) approval_policy: AskForApproval, + pub(crate) approvals_reviewer: ApprovalsReviewer, } impl GuardianReviewContext { + pub(crate) fn from_resolved_settings( + turn: Arc, + settings: &ResolvedStepSettings, + ) -> Self { + Self { + environments: turn.environments.clone(), + model_info: Arc::clone(&settings.model_info), + reasoning_effort: settings.reasoning_effort().cloned(), + reasoning_summary: settings.reasoning_summary, + approval_policy: settings.approval_policy(), + approvals_reviewer: settings.approvals_reviewer(), + turn, + } + } + pub(crate) fn turn(&self) -> &Arc { &self.turn } @@ -95,6 +127,11 @@ impl From<&Arc> for GuardianReviewContext { Self { turn: Arc::clone(&step.turn), environments: step.environments.clone(), + model_info: Arc::clone(&step.settings.model_info), + reasoning_effort: step.settings.reasoning_effort().cloned(), + reasoning_summary: step.settings.reasoning_summary, + approval_policy: step.settings.approval_policy(), + approvals_reviewer: step.settings.approvals_reviewer(), } } } @@ -103,6 +140,11 @@ impl From> for GuardianReviewContext { fn from(turn: Arc) -> Self { Self { environments: turn.environments.clone(), + model_info: Arc::clone(turn.model_info()), + reasoning_effort: turn.reasoning_effort().cloned(), + reasoning_summary: turn.reasoning_summary(), + approval_policy: turn.approval_policy(), + approvals_reviewer: turn.config.approvals_reviewer, turn, } } diff --git a/codex-rs/core/src/session/elicitation_holders_tests.rs b/codex-rs/core/src/session/elicitation_holders_tests.rs index d4061176e0..3d0b09cbf7 100644 --- a/codex-rs/core/src/session/elicitation_holders_tests.rs +++ b/codex-rs/core/src/session/elicitation_holders_tests.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::sync::Arc; use codex_core_plugins::PluginCommandAttribution; use codex_plugin::PluginId; @@ -13,6 +14,7 @@ use tokio::sync::watch; use tokio_util::sync::CancellationToken; use super::tests::make_session_and_context_with_rx; +use crate::session::step_context::StepContext; use crate::state::ActiveTurn; async fn wait_until_held(pause_state: &mut watch::Receiver) { @@ -126,7 +128,7 @@ async fn permission_request_holds_an_elicitation_until_response() { .selection(); session .request_permissions_for_environment( - &turn_context, + &StepContext::for_test(Arc::clone(&turn_context)), "call-1".to_string(), RequestPermissionsArgs { environment_id: None, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index a60ed4506d..d898ff969d 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -41,6 +41,7 @@ use crate::image_preparation::unified_image_budget_enabled; use crate::parse_turn_item; use crate::realtime_conversation::RealtimeConversationManager; use crate::session::step_context::StepContext; +use crate::session::step_settings::ResolvedStepSettings; use crate::session::step_settings::StepSettings; use crate::session::turn_context::TurnEnvironment; use crate::session_prefix::format_inter_agent_completion_message; @@ -2604,15 +2605,16 @@ impl Session { )] pub(crate) async fn request_permissions_for_environment( self: &Arc, - review_context: impl Into, + step_context: &Arc, call_id: String, args: RequestPermissionsArgs, environment: TurnEnvironmentSelection, cancellation_token: CancellationToken, ) -> Option { - let review_context = review_context.into(); - let turn_context = review_context.turn(); - match turn_context.as_ref().approval_policy() { + let turn_context = &step_context.turn; + let approval_policy = step_context.settings.approval_policy(); + let approvals_reviewer = step_context.settings.approvals_reviewer(); + match approval_policy { AskForApproval::Never => { return Some(RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), @@ -2648,7 +2650,8 @@ impl Session { }); }; - if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { + if crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) + { let originating_turn_state = { let active = self.active_turn.lock().await; active.as_ref().map(|active| Arc::clone(&active.turn_state)) @@ -2660,7 +2663,7 @@ impl Session { permissions: requested_permissions.clone(), }; let approval_context = ApprovalContext { - review_context: review_context.clone(), + review_context: crate::guardian::GuardianReviewContext::from(step_context), cancellation_token: Some(cancellation_token.clone()), call_id, tool_name: ToolName::plain("request_permissions"), @@ -2982,12 +2985,14 @@ impl Session { )] pub(crate) async fn active_turn_context_and_strict_auto_review( &self, - ) -> Option<(Arc, bool)> { + ) -> Option<(Arc, Arc, bool)> { let active = self.active_turn.lock().await; let active = active.as_ref()?; - let turn_context = Arc::clone(&active.task.as_ref()?.turn_context); + let task = active.task.as_ref()?; + let turn_context = Arc::clone(&task.turn_context); + let step_settings = turn_context.current_settings.load_full(); let ts = active.turn_state.lock().await; - Some((turn_context, ts.strict_auto_review_enabled())) + Some((turn_context, step_settings, ts.strict_auto_review_enabled())) } pub(crate) async fn granted_session_permissions( diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 8e6ebdb7ea..3b37ac4239 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1432,8 +1432,6 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an .primary() .expect("turn should have a primary environment"), &tool_ctx, - turn.as_ref(), - AskForApproval::Never, ) .await .expect("probe runtime should succeed"); @@ -6949,7 +6947,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() .selection(); session .request_permissions_for_environment( - turn_context.as_ref(), + &StepContext::for_test(Arc::clone(turn_context.as_ref())), call_id, codex_protocol::request_permissions::RequestPermissionsArgs { environment_id: None, @@ -7203,7 +7201,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec .selection(); session .request_permissions_for_environment( - turn_context.as_ref(), + &StepContext::for_test(Arc::clone(turn_context.as_ref())), call_id, codex_protocol::request_permissions::RequestPermissionsArgs { environment_id: None, @@ -7295,7 +7293,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req .selection(); let response = session .request_permissions_for_environment( - turn_context.as_ref(), + &StepContext::for_test(Arc::clone(turn_context.as_ref())), call_id, codex_protocol::request_permissions::RequestPermissionsArgs { environment_id: None, diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index eb9d4e9881..c2a04bf531 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -1,15 +1,26 @@ use super::*; use crate::compact::InitialContextInjection; +use crate::config::Constrained; use crate::exec_policy::ExecPolicyManager; use crate::guardian::GUARDIAN_REVIEWER_NAME; use crate::plugins::plugins_manager_for_config; use crate::sandboxing::SandboxPermissions; use crate::session::step_context::StepContext; use crate::session::tests::update_turn_settings_for_test; +use crate::session::turn_context::NewTurnContextOptions; use crate::test_support::models_manager_with_provider; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; +use crate::tools::orchestrator::ToolOrchestrator; +use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::ApprovalAction; +use crate::tools::sandboxing::ExecApprovalRequirement; +use crate::tools::sandboxing::SandboxAttempt; +use crate::tools::sandboxing::Sandboxable; +use crate::tools::sandboxing::ToolCtx; +use crate::tools::sandboxing::ToolError; +use crate::tools::sandboxing::ToolRuntime; use crate::turn_diff_tracker::TurnDiffTracker; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; @@ -18,10 +29,15 @@ use codex_config::ConfigRequirementsToml; use codex_exec_server::EnvironmentManager; use codex_execpolicy::Decision; use codex_execpolicy::Evaluation; +use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; use codex_features::Feature; use codex_model_provider::create_model_provider; +use codex_network_proxy::NetworkDecision; +use codex_network_proxy::NetworkPolicyRequest; +use codex_network_proxy::NetworkProtocol; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::error::SandboxErr; use codex_protocol::models::AdditionalPermissionProfile as PermissionProfile; use codex_protocol::models::ContentItem; use codex_protocol::models::ContentItemKind; @@ -50,6 +66,7 @@ use std::fs; use std::sync::Arc; use std::time::Duration; use tempfile::tempdir; +use test_case::test_case; use tokio::time::timeout; use tokio_util::sync::CancellationToken; @@ -72,6 +89,93 @@ where } } +async fn activate_turn_with_new_review_authority(session: &Arc) -> Arc { + let (current_turn, _) = session + .new_turn_with_sub_id( + "current-authority-turn".to_string(), + SessionSettingsUpdate { + step_settings: StepSettingsUpdate { + approval_policy: Some(AskForApproval::Never), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + ..Default::default() + }, + permission_profile: Some(codex_protocol::models::PermissionProfile::Disabled), + ..Default::default() + }, + NewTurnContextOptions::default(), + ) + .await + .expect("next turn should accept different approval authority"); + session + .start_task( + current_turn, + Vec::new(), + super::NeverEndingTask { + kind: crate::state::TaskKind::Regular, + listen_to_cancellation_token: true, + }, + ) + .await; + + let (active_turn, _, _) = session + .active_turn_context_and_strict_auto_review() + .await + .expect("next turn should have active review authority"); + assert_eq!( + ( + active_turn.approval_policy(), + active_turn.config.approvals_reviewer + ), + (AskForApproval::Never, ApprovalsReviewer::AutoReview) + ); + active_turn +} + +fn captured_step_with_user_reviewer( + turn: &mut Arc, + admitted_policy: AskForApproval, + captured_policy: AskForApproval, +) -> Arc { + let config = Arc::make_mut( + &mut Arc::get_mut(turn) + .expect("turn should not be shared") + .config, + ); + config + .permissions + .approval_policy + .set(admitted_policy) + .expect("set admitted turn approval policy"); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + + let mut step = StepContext::for_test(Arc::clone(turn)); + let captured = Arc::get_mut(&mut step).expect("step context should not be shared"); + update_selected_settings_for_test(Arc::make_mut(&mut captured.settings), |selected| { + selected + .approval_policy + .set(captured_policy) + .expect("set captured approval policy"); + selected.approvals_reviewer = ApprovalsReviewer::User; + }); + step +} + +async fn next_exec_approval( + events: &async_channel::Receiver, +) -> codex_protocol::protocol::ExecApprovalRequestEvent { + timeout(Duration::from_secs(5), async { + loop { + if let EventMsg::ExecApprovalRequest(approval) = + events.recv().await.expect("receive approval event").msg + { + break approval; + } + } + }) + .await + .expect("captured action should request user approval") +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { let server = start_mock_server().await; @@ -138,6 +242,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { evidence.record("js", "cell", "image", vec![image]); let session = Arc::new(session); let turn_context = Arc::new(turn_context_raw); + let step_context = StepContext::for_test(Arc::clone(&turn_context)); let requested_permissions = RequestPermissionProfile { network: Some(NetworkPermissions { @@ -153,7 +258,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { let response = tokio::time::timeout( Duration::from_secs(45), session.request_permissions_for_environment( - &turn_context, + &step_context, "perm-call-1".to_string(), RequestPermissionsArgs { environment_id: None, @@ -177,7 +282,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { ); let second_response = session .request_permissions_for_environment( - &turn_context, + &step_context, "perm-call-2".to_string(), RequestPermissionsArgs { environment_id: None, @@ -207,6 +312,84 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { assert!(guardian_request.body_contains_text("need network")); } +#[tokio::test] +async fn request_permissions_uses_issuing_step_policy_and_reviewer() { + let server = start_mock_server().await; + let guardian_requests = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("guardian", r#"{"outcome":"allow"}"#), + ev_completed("guardian-review"), + ]), + ) + .await; + let (session, turn, _) = make_session_and_context_with_auth_and_config_and_rx( + CodexAuth::from_api_key("Test API Key"), + Vec::new(), + |config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); + config.approvals_reviewer = ApprovalsReviewer::User; + config.model_provider.base_url = Some(format!("{}/v1", server.uri())); + config + .features + .enable(Feature::GuardianApproval) + .expect("enable Guardian"); + }, + ) + .await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let mut step = StepContext::for_test(turn); + let captured = Arc::get_mut(&mut step).expect("unshared step"); + // The issuing step differs from the admitted Never/User turn. + update_selected_settings_for_test(Arc::make_mut(&mut captured.settings), |selected| { + selected + .approval_policy + .set(AskForApproval::OnRequest) + .expect("set step policy"); + selected.approvals_reviewer = ApprovalsReviewer::AutoReview; + }); + let permissions = RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }; + + let response = timeout( + Duration::from_secs(5), + session.request_permissions_for_environment( + &step, + "step-permissions".to_string(), + RequestPermissionsArgs { + environment_id: None, + reason: Some("need network".to_string()), + permissions: permissions.clone(), + }, + step.environments + .primary() + .expect("primary environment") + .selection(), + CancellationToken::new(), + ), + ) + .await + .expect("issuing step should route to Guardian without a user approval"); + + assert_eq!( + response, + Some(RequestPermissionsResponse { + permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }) + ); + assert!( + guardian_requests + .single_request() + .body_contains_text("request_permissions") + ); +} + #[tokio::test] async fn request_permissions_guardian_review_stops_when_cancelled() { let server = start_mock_server().await; @@ -268,7 +451,7 @@ async fn request_permissions_guardian_review_stops_when_cancelled() { .selection(); session .request_permissions_for_environment( - &turn_context, + &StepContext::for_test(Arc::clone(&turn_context)), "perm-call-cancelled".to_string(), RequestPermissionsArgs { environment_id: None, @@ -534,6 +717,284 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_exec_command_policy_s assert!(guardian_request.body_contains_text("echo hi")); } +#[test_case(AskForApproval::Never; "policy_precheck")] +#[test_case(AskForApproval::OnRequest; "reviewer_routing")] +#[tokio::test] +async fn network_approval_uses_published_task_authority_within_same_turn( + admitted_policy: AskForApproval, +) { + let (session, turn, events) = make_session_and_context_with_auth_and_config_and_rx( + CodexAuth::from_api_key("Test API Key"), + Vec::new(), + move |config| { + config.permissions.approval_policy = Constrained::allow_any(admitted_policy); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .permissions + .set_permission_profile( + codex_protocol::models::PermissionProfile::workspace_write(), + ) + .expect("set managed permissions"); + }, + ) + .await; + session + .start_task( + Arc::clone(&turn), + Vec::new(), + super::NeverEndingTask { + kind: crate::state::TaskKind::Regular, + listen_to_cancellation_token: true, + }, + ) + .await; + // Inject later-step authority directly while live policy changes remain gated. + { + let active = session.active_turn.lock().await; + let task = active + .as_ref() + .expect("active turn") + .task + .as_ref() + .expect("active task"); + let mut settings = task.turn_context.current_settings.load_full(); + update_selected_settings_for_test(Arc::make_mut(&mut settings), |selected| { + selected + .approval_policy + .set(AskForApproval::OnRequest) + .expect("update policy"); + selected.approvals_reviewer = ApprovalsReviewer::User; + }); + task.turn_context.current_settings.store(settings); + } + let decision = session + .services + .network_approval + .handle_inline_policy_request( + Arc::clone(&session), + NetworkPolicyRequest { + protocol: NetworkProtocol::Http, + host: "example.com".to_string(), + port: 80, + environment_id: None, + client_addr: None, + method: None, + command: None, + exec_policy_hint: None, + execution_id: None, + disconnect: None, + }, + ); + tokio::pin!(decision); + let approval = timeout(Duration::from_secs(5), async { + loop { + tokio::select! { + result = &mut decision => panic!("expected user network approval, got {result:?}"), + event = events.recv() => { + match event.expect("approval event").msg { + EventMsg::ExecApprovalRequest(approval) => break approval, + EventMsg::GuardianAssessment(_) => panic!("expected the current user reviewer"), + _ => {} + } + } + } + } + }) + .await + .expect("network approval requested"); + assert_eq!(approval.turn_id, turn.sub_id); + session + .notify_approval(&approval.call_id, ReviewDecision::Approved) + .await; + assert_eq!( + timeout(Duration::from_secs(5), decision) + .await + .expect("network decision"), + NetworkDecision::Allow + ); + session.abort_all_tasks(TurnAbortReason::Interrupted).await; +} + +#[tokio::test] +async fn delayed_exec_command_uses_its_captured_authority_after_next_turn_starts() { + let (mut session, mut action_turn, events) = make_session_and_context_with_rx().await; + // Windows can allow safe echo commands without prompting when its sandbox is disabled. + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule( + &["echo".to_string(), "captured-action-authority".to_string()], + Decision::Prompt, + ) + .expect("test command should require approval"); + Arc::get_mut(&mut session) + .expect("session should not be shared") + .services + .exec_policy = Arc::new(ExecPolicyManager::new(Arc::new(exec_policy))); + let step_context = captured_step_with_user_reviewer( + &mut action_turn, + AskForApproval::Never, + AskForApproval::OnRequest, + ); + let current_turn = activate_turn_with_new_review_authority(&session).await; + assert_ne!(action_turn.sub_id, current_turn.sub_id); + + let call_id = "delayed-captured-authority-shell-command"; + let command = "echo captured-action-authority"; + let handler = crate::tools::handlers::ExecCommandHandler::default(); + let invocation = handler.handle(ToolInvocation { + session: Arc::clone(&session), + turn: Arc::clone(&action_turn), + step_context, + cancellation_token: CancellationToken::new(), + tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())), + call_id: call_id.to_string(), + tool_name: codex_tools::ToolName::plain("exec_command"), + source: ToolCallSource::Direct, + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "cmd": command, + "login": false, + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "verify captured action authority", + }) + .to_string(), + }, + }); + let approve = async { + let approval = next_exec_approval(&events).await; + assert_eq!(approval.call_id, call_id); + assert_eq!(approval.turn_id, action_turn.sub_id); + assert!(approval.command.join(" ").contains(command)); + session + .notify_approval(call_id, ReviewDecision::Approved) + .await; + }; + + let (output, ()) = tokio::join!(invocation, approve); + let output = output.expect("approved shell command should succeed"); + assert!(expect_text_output(output.as_ref()).contains("captured-action-authority")); +} + +#[tokio::test] +async fn sandbox_denied_retry_uses_the_action_policy_and_reviewer() { + #[derive(Default)] + struct DeniedOnceRuntime { + attempts: usize, + } + + impl Approvable for DeniedOnceRuntime { + fn exec_approval_requirement( + &self, + _request: &TurnEnvironment, + ) -> Option { + Some(ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }) + } + + fn approval_action( + &self, + request: &TurnEnvironment, + call_id: &str, + ) -> std::io::Result { + Ok(ApprovalAction::ExecCommand { + id: call_id.to_string(), + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + command: vec!["echo".to_string(), "sandbox-retry".to_string()], + hook_command: "echo sandbox-retry".to_string(), + cwd: request.cwd().clone(), + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + tty: false, + proposed_execpolicy_amendment: None, + }) + } + } + + impl Sandboxable for DeniedOnceRuntime { + fn sandbox_preference(&self) -> codex_sandboxing::SandboxablePreference { + codex_sandboxing::SandboxablePreference::Auto + } + } + + impl ToolRuntime for DeniedOnceRuntime { + fn turn_environment<'a>(&self, request: &'a TurnEnvironment) -> &'a TurnEnvironment { + request + } + + async fn run( + &mut self, + _request: &TurnEnvironment, + _attempt: &SandboxAttempt<'_>, + _context: &ToolCtx, + ) -> Result { + self.attempts += 1; + if self.attempts == 1 { + return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { + output: Box::new(ExecToolCallOutput { + exit_code: 1, + ..Default::default() + }), + network_policy_decision: None, + }))); + } + Ok("sandbox-retry-succeeded".to_string()) + } + } + + let (session, mut action_turn, events) = make_session_and_context_with_rx().await; + let step_context = captured_step_with_user_reviewer( + &mut action_turn, + AskForApproval::OnRequest, + AskForApproval::UnlessTrusted, + ); + + let current_turn = activate_turn_with_new_review_authority(&session).await; + assert_ne!(action_turn.sub_id, current_turn.sub_id); + + let call_id = "captured-action-sandbox-retry"; + let context = ToolCtx { + session: Arc::clone(&session), + step_context, + cancellation_token: CancellationToken::new(), + call_id: call_id.to_string(), + tool_name: codex_tools::ToolName::plain("exec_command"), + }; + let environment = context + .step_context + .environments + .primary() + .expect("primary environment"); + let mut orchestrator = ToolOrchestrator::new(); + let mut runtime = DeniedOnceRuntime::default(); + let approve = async { + let approval = next_exec_approval(&events).await; + assert_eq!(approval.call_id, call_id); + assert_eq!(approval.turn_id, action_turn.sub_id); + assert_eq!( + approval.reason.as_deref(), + Some("command failed; retry without sandbox?") + ); + session + .notify_approval(call_id, ReviewDecision::Approved) + .await; + }; + + let (output, ()) = tokio::join!( + orchestrator.run(&mut runtime, environment, &context), + approve + ); + assert_eq!( + output + .expect("approved sandbox retry should succeed") + .output, + "sandbox-retry-succeeded" + ); + assert_eq!(runtime.attempts, 2); +} + #[tokio::test] async fn guardian_allows_unified_exec_additional_permissions_requests_past_policy_validation() { let (mut session, mut turn_context_raw) = make_session_and_context().await; diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index d78e14f102..91a90cb6ca 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -14,7 +14,6 @@ use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_tool_call::request_mcp_tool_user_approval; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; -use crate::session::turn_context::TurnContext; use crate::tools::hook_names::HookToolName; use crate::tools::runtimes::apply_patch::ApplyPatchApprovalKey; use crate::tools::runtimes::unified_exec::UnifiedExecApprovalKey; @@ -377,10 +376,6 @@ enum ApprovalReviewer { } impl ApprovalReviewer { - fn for_turn(turn: &TurnContext) -> Self { - Self::for_policy(turn.approval_policy(), turn.config.approvals_reviewer) - } - fn for_policy(approval_policy: AskForApproval, reviewer: ApprovalsReviewer) -> Self { if routes_approval_policy_to_guardian(approval_policy, reviewer) { Self::Guardian @@ -515,7 +510,10 @@ impl Session { { ApprovalReviewer::for_policy(*approval_policy, *reviewer) } else { - ApprovalReviewer::for_turn(ctx.review_context.turn()) + ApprovalReviewer::for_policy( + ctx.review_context.approval_policy, + ctx.review_context.approvals_reviewer, + ) }; let decision = match reviewer { diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index 15a165dbb9..d5800e7846 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::session::tests::make_session_and_context_with_rx; use codex_models_manager::model_info::model_info_from_slug; use codex_protocol::approvals::NetworkPolicyAmendment; use pretty_assertions::assert_eq; @@ -94,3 +95,60 @@ fn guardian_cwd_rejects_foreign_remote_path() { assert!(guardian_cwd(codex_exec_server::REMOTE_ENVIRONMENT_ID, cwd).is_err()); } + +#[tokio::test] +async fn explicit_mcp_reviewer_override_takes_precedence_over_action_context() { + let (session, turn, events) = make_session_and_context_with_rx().await; + let action = ApprovalAction::McpToolCall { + id: "mcp-override".to_string(), + server: "example".to_string(), + tool_name: "dangerous".to_string(), + arguments: None, + connector_id: None, + connector_name: None, + connector_description: None, + connected_account_email: None, + tool_title: None, + tool_description: None, + annotations: None, + hook_tool_name: HookToolName::new("mcp__example__dangerous"), + approval_policy: AskForApproval::OnRequest, + reviewer: ApprovalsReviewer::User, + approval_mode: AppToolApproval::Prompt, + allow_session_remember: false, + allow_persistent_approval: false, + }; + let mut review_context = GuardianReviewContext::from(&turn); + review_context.approval_policy = AskForApproval::OnRequest; + review_context.approvals_reviewer = ApprovalsReviewer::AutoReview; + let context = ApprovalContext { + review_context, + cancellation_token: None, + call_id: "mcp-override".to_string(), + tool_name: ToolName::plain("dangerous"), + strict_auto_review: false, + approval_reason: None, + retry_reason: None, + network_approval_context: None, + }; + + tokio::select! { + resolution = session.request_reviewer_approval(action, &context) => { + panic!("expected a user approval request, got {resolution:?}"); + } + event = events.recv() => { + let codex_protocol::protocol::EventMsg::ElicitationRequest(request) = + event.expect("receive user approval request").msg + else { + panic!("expected an MCP user approval request"); + }; + assert_eq!(request.server_name, "example"); + assert_eq!( + request.id, + codex_protocol::mcp::RequestId::String( + "mcp_tool_call_approval_mcp-override".to_string() + ) + ); + } + } +} diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index b172281d92..4bfa2811c4 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -564,7 +564,7 @@ async fn execute_verified_patch( .await .unwrap_or_else(|_| patch_permissions_without_path_matching(&action)); let apply = apply_patch::prepare_apply_patch( - tool_ctx.step_context.turn.as_ref(), + &tool_ctx.step_context, &turn_environment, &file_system_sandbox_policy, action, @@ -595,13 +595,7 @@ async fn execute_verified_patch( let mut orchestrator = ToolOrchestrator::new(); let mut runtime = ApplyPatchRuntime::new(); let result = orchestrator - .run( - &mut runtime, - &request, - &tool_ctx, - tool_ctx.step_context.turn.as_ref(), - tool_ctx.step_context.turn.approval_policy(), - ) + .run(&mut runtime, &request, &tool_ctx) .await .map(|result| result.output); let (result, delta) = match result { diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index b832617c40..b3b600b549 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -276,17 +276,13 @@ impl ExecCommandHandler { // Sticky turn permissions have already been approved, so they should // continue through the normal exec approval flow for the command. + let approval_policy = context.step_context.settings.approval_policy(); if effective_additional_permissions .sandbox_permissions .requests_sandbox_override() && !effective_additional_permissions.permissions_preapproved - && prompt_is_rejected_by_policy( - context.step_context.turn.approval_policy(), - /*prompt_is_rule*/ false, - ) - .is_some() + && prompt_is_rejected_by_policy(approval_policy, /*prompt_is_rule*/ false).is_some() { - let approval_policy = context.step_context.turn.approval_policy(); manager.release_process_id(process_id).await; return Err(FunctionCallError::RespondToModel(format!( "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" @@ -302,7 +298,7 @@ impl ExecCommandHandler { || { normalize_and_validate_additional_permissions( additional_permissions_allowed, - context.step_context.turn.approval_policy(), + approval_policy, effective_additional_permissions.sandbox_permissions, effective_additional_permissions.additional_permissions, effective_additional_permissions.permissions_preapproved, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 13f8a2b375..e82780c863 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -621,7 +621,7 @@ impl NetworkApprovalService { let Some(environment_id) = active_environment_id.or_else(|| { active_turn .as_ref() - .and_then(|(turn_context, _)| turn_context.environments.primary()) + .and_then(|(turn_context, _, _)| turn_context.environments.primary()) .map(|environment| environment.selection.environment_id.clone()) }) else { return NetworkDecision::deny(REASON_NOT_ALLOWED); @@ -648,7 +648,7 @@ impl NetworkApprovalService { format!("Network access to \"{target}\" was blocked by policy."); let prompt_reason = format!("{} is not in the allowed_domains", request.host); - let Some((turn_context, strict_auto_review)) = active_turn else { + let Some((turn_context, step_settings, strict_auto_review)) = active_turn else { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome(&owner_call.registration_id, policy_denial_message); } @@ -694,7 +694,11 @@ impl NetworkApprovalService { pending_owner.complete(PendingApprovalDecision::Deny); return NetworkDecision::deny(REASON_NOT_ALLOWED); } - if !allows_network_approval_flow(turn_context.approval_policy()) { + let review_context = GuardianReviewContext::from_resolved_settings( + Arc::clone(&turn_context), + &step_settings, + ); + if !allows_network_approval_flow(review_context.approval_policy) { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome(&owner_call.registration_id, policy_denial_message); } @@ -755,7 +759,7 @@ impl NetworkApprovalService { cwd, }; let approval_context = ApprovalContext { - review_context: GuardianReviewContext::from(&turn_context), + review_context, cancellation_token: None, call_id: approval_call_id, tool_name: telemetry_tool_name.clone(), diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index b764d1cde5..3b3757171b 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -127,12 +127,12 @@ impl ToolOrchestrator { tool: &mut T, req: &Rq, tool_ctx: &ToolCtx, - turn_ctx: &crate::session::turn_context::TurnContext, - approval_policy: AskForApproval, ) -> Result, ToolError> where T: ToolRuntime, { + let turn_ctx = tool_ctx.step_context.turn.as_ref(); + let approval_policy = tool_ctx.step_context.settings.approval_policy(); let otel = turn_ctx.session_telemetry.clone(); let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned(); let otel_ci = &tool_ctx.call_id; @@ -140,7 +140,7 @@ impl ToolOrchestrator { .session .active_turn_context_and_strict_auto_review() .await - .is_some_and(|(_, strict_auto_review)| strict_auto_review); + .is_some_and(|(_, _, strict_auto_review)| strict_auto_review); // 1) Approval let mut already_approved = false; diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 67c2418b55..89ba4c734e 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -8,7 +8,7 @@ use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::guardian::GUARDIAN_REVIEW_TIMEOUT; use crate::guardian::GuardianNetworkAccessTrigger; -use crate::guardian::routes_approval_to_guardian; +use crate::guardian::routes_approval_policy_to_guardian; use crate::plugins::metrics::sidecar_for_command; use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecServerEnvConfig; @@ -289,8 +289,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt let mut launch = network.remote_launch_config().await.map_err(|err| { ToolError::Codex(CodexErr::Io(io::Error::other(err.to_string()))) })?; - if routes_approval_to_guardian(&ctx.step_context.turn) - && network.remote_policy_decider().is_some() + if routes_approval_policy_to_guardian( + ctx.step_context.settings.approval_policy(), + ctx.step_context.settings.approvals_reviewer(), + ) && network.remote_policy_decider().is_some() { let timeout = ctx .session diff --git a/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs index 7052db9165..0c3ae3dc4c 100644 --- a/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs @@ -150,7 +150,7 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( environment_id: req.turn_environment.selection.environment_id.clone(), source: GuardianCommandSource::UnifiedExec, tool_name: ctx.tool_name.clone(), - approval_policy: ctx.step_context.turn.approval_policy(), + approval_policy: ctx.step_context.settings.approval_policy(), permission_profile: exec_request.permission_profile.clone(), sandbox_permissions: req.sandbox_permissions, approval_sandbox_permissions: approval_sandbox_permissions( @@ -280,7 +280,7 @@ impl CoreShellActionProvider { }; match stopwatch .pause_for(async { - let (turn_context, strict_auto_review) = self + let (turn_context, step_settings, strict_auto_review) = self .session .active_turn_context_and_strict_auto_review() .await @@ -291,7 +291,10 @@ impl CoreShellActionProvider { ) })?; let approval_ctx = ApprovalContext { - review_context: GuardianReviewContext::from(turn_context), + review_context: GuardianReviewContext::from_resolved_settings( + turn_context, + &step_settings, + ), // The running process can outlive its launching tool or code-mode cell. cancellation_token: None, call_id: self.call_id.clone(), diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index a4f69ae869..1bbd73b7a2 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1266,7 +1266,7 @@ impl UnifiedExecProcessManager { .create_exec_approval_requirement_for_shell( ExecApprovalRequest { command: &request.command, - approval_policy: turn.approval_policy(), + approval_policy: context.step_context.settings.approval_policy(), permission_profile: request.turn_environment.permission_profile().clone(), environment_policy: request.turn_environment.config().exec_policy.as_ref(), windows_sandbox_level: request.turn_environment.config().windows_sandbox_level, @@ -1311,7 +1311,7 @@ impl UnifiedExecProcessManager { tool_name: ToolName::plain("exec_command"), }; orchestrator - .run(&mut runtime, &req, &tool_ctx, turn, turn.approval_policy()) + .run(&mut runtime, &req, &tool_ctx) .await .map(|result| (result.output, result.deferred_network_approval)) .map_err(|err| match err {