From bfca0335fabd27b5f9907acc286349988473e309 Mon Sep 17 00:00:00 2001 From: Abhinav Date: Thu, 10 Sep 2026 16:08:24 +0000 Subject: [PATCH] Tie network approval reviews to their originating execution (#44575) ## Why Background commands can outlive their launching turn. Their network reviews need the original execution environment even when current turn settings change, and pending reviews must stop when the execution is cancelled. ## What changed - Capture the execution's environment snapshot for Guardian network reviews while continuing to use current review settings. - Deny requests from already cancelled executions before checking session host grants, and resolve pending approvals as denied when execution cancellation interrupts review. - Preserve accepted review results and session grants when cancellation happens after acceptance. ## Testing Add regression coverage for same-turn and later-turn settings changes, the original review working directory, cancellation before and after review acceptance, late callbacks, background process termination, and reuse of accepted session grants by new executions. GitOrigin-RevId: 29487488da9ab43700ba6684c61eea86f30a03a8 --- codex-rs/core/src/guardian/mod.rs | 3 +- codex-rs/core/src/tools/network_approval.rs | 32 +- .../core/src/tools/network_approval_tests.rs | 148 +++++ codex-rs/core/src/tools/orchestrator.rs | 1 + .../runtimes/zsh_fork/unix_escalation.rs | 3 +- codex-rs/core/tests/suite/guardian_review.rs | 40 +- codex-rs/core/tests/suite/network_approval.rs | 531 +++++++++++++----- 7 files changed, 586 insertions(+), 172 deletions(-) diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 13e2ba9d24..48789d3549 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -94,13 +94,14 @@ impl GuardianReviewContext { pub(crate) fn from_resolved_settings( turn: Arc, settings: &ResolvedStepSettings, + environments: &TurnEnvironmentSnapshot, ) -> Self { Self { parent_response_id: turn .extension_data .get::() .map(|id| id.0.clone()), - environments: turn.environments.clone(), + environments: environments.clone(), model_info: Arc::clone(&settings.model_info), reasoning_effort: settings.reasoning_effort().cloned(), reasoning_summary: settings.reasoning_summary, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index a3e870a92b..9ea9089e98 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -1,4 +1,5 @@ use crate::config::NetworkProxySpec; +use crate::environment_selection::TurnEnvironmentSnapshot; use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::GuardianReviewContext; use crate::network_policy_decision::denied_network_policy_message; @@ -241,6 +242,7 @@ struct ActiveNetworkApprovalCall { command: String, environment_id: String, permission_profile: PermissionProfile, + environments: TurnEnvironmentSnapshot, cancellation_token: CancellationToken, } @@ -639,6 +641,12 @@ impl NetworkApprovalService { else { return NetworkDecision::deny(REASON_NOT_ALLOWED); }; + if owner_call + .as_ref() + .is_some_and(|call| call.cancellation_token.is_cancelled()) + { + return NetworkDecision::deny(REASON_NOT_ALLOWED); + } let active_turn = session.active_turn_context_and_strict_auto_review().await; let Some(environment_id) = active_environment_id.or_else(|| { active_turn @@ -720,6 +728,11 @@ impl NetworkApprovalService { let review_context = GuardianReviewContext::from_resolved_settings( Arc::clone(&turn_context), &step_settings, + // Review this new request under current settings, in its originating execution's + // environments. Completing the original turn does not transfer process ownership. + owner_call + .as_ref() + .map_or(&turn_context.environments, |call| &call.environments), ); if !allows_network_approval_flow(review_context.approval_policy) { if let Some(owner_call) = owner_call.as_ref() { @@ -791,7 +804,22 @@ impl NetworkApprovalService { retry_reason: Some(policy_denial_message.clone()), network_approval_context: Some(network_approval_context.clone()), }; - let approval_decision = match session.request_approval(action, approval_context).await { + let approval = session.request_approval(action, approval_context); + let approval_result = if let Some(owner_call) = &owner_call { + // A ready review result takes precedence over concurrent cancellation. + let Some(result) = owner_call + .cancellation_token + .run_until_cancelled(approval) + .await + else { + pending_owner.complete(PendingApprovalDecision::Deny); + return NetworkDecision::deny(REASON_NOT_ALLOWED); + }; + result + } else { + approval.await + }; + let approval_decision = match approval_result { Ok(decision) => decision, Err(ToolError::Rejected(rejection)) => { if let Some(owner_call) = owner_call.as_ref() { @@ -1067,6 +1095,7 @@ pub(crate) fn build_network_policy_decider( pub(crate) async fn begin_network_approval( session: &Arc, turn: &TurnContext, + environments: &TurnEnvironmentSnapshot, managed_network_active: bool, spec: Option, ) -> Result, ToolError> { @@ -1181,6 +1210,7 @@ pub(crate) async fn begin_network_approval( command, environment_id, permission_profile, + environments: environments.clone(), cancellation_token: cancellation_token.clone(), }) .await; diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index 95ac30a406..139cff008e 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -10,9 +10,155 @@ use core_test_support::test_path_buf; use futures::poll; use pretty_assertions::assert_eq; use std::time::Duration; +use test_case::test_case; use tokio::time::timeout; use tokio_util::sync::CancellationToken; +enum ExecutionCancellationTiming { + PendingReview, + AcceptedReview, +} + +#[test_case(ExecutionCancellationTiming::PendingReview; "pending_review")] +#[test_case(ExecutionCancellationTiming::AcceptedReview; "accepted_review")] +#[tokio::test] +#[allow( + clippy::await_holding_invalid_type, + reason = "hold the commit lock to cancel execution after the review decision is accepted" +)] +async fn execution_cancellation_respects_network_approval_boundary( + timing: ExecutionCancellationTiming, +) { + use crate::session::tests::make_session_and_context_with_rx; + use crate::tasks::SessionTask; + + struct PendingTask; + impl SessionTask for PendingTask { + fn kind(&self) -> crate::state::TaskKind { + crate::state::TaskKind::Regular + } + + fn span_name(&self) -> &'static str { + "network_approval_test" + } + + async fn run( + self: Arc, + _session: Arc, + _ctx: Arc, + _input: Vec, + cancellation_token: CancellationToken, + ) -> crate::tasks::SessionTaskResult { + cancellation_token.cancelled().await; + Ok(None) + } + } + + let (session, _, events) = make_session_and_context_with_rx().await; + let (turn, _) = session + .new_turn_with_sub_id( + "active-turn".to_string(), + crate::session::SessionSettingsUpdate { + step_settings: crate::session::step_settings::StepSettingsUpdate { + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::User), + ..Default::default() + }, + ..Default::default() + }, + crate::session::turn_context::NewTurnContextOptions::default(), + ) + .await + .unwrap(); + session.start_task(turn, Vec::new(), PendingTask).await; + let service = &session.services.network_approval; + register_call_with_default_shell_trigger(service, "execution-1").await; + let request = NetworkPolicyRequest { + protocol: NetworkProtocol::Http, + host: "example.com".to_string(), + port: 80, + environment_id: Some("local".to_string()), + execution_id: Some("execution-1".to_string()), + client_addr: None, + method: None, + command: None, + exec_policy_hint: None, + disconnect: None, + cancellation: None, + }; + let decision = service.handle_inline_policy_request(Arc::clone(&session), request.clone()); + tokio::pin!(decision); + let approval = timeout(Duration::from_secs(5), async { + loop { + tokio::select! { + result = &mut decision => panic!("expected pending approval, got {result:?}"), + event = events.recv() => { + if let EventMsg::ExecApprovalRequest(approval) = event.unwrap().msg { + break approval; + } + } + } + } + }) + .await + .unwrap(); + let expected_decision = match timing { + ExecutionCancellationTiming::PendingReview => { + service.record_call_outcome("execution-1", "original execution denial".to_string()); + NetworkDecision::deny("not_allowed") + } + ExecutionCancellationTiming::AcceptedReview => { + let commit_guard = service.session_policy_commit_lock.lock().await; + session + .notify_approval( + &approval.effective_approval_id(), + ReviewDecision::ApprovedForSession, + ) + .await; + // Consume the approval and stop at the held policy commit lock. + assert!(poll!(decision.as_mut()).is_pending()); + service.record_call_outcome("execution-1", "original execution denial".to_string()); + assert!(poll!(decision.as_mut()).is_pending()); + drop(commit_guard); + NetworkDecision::Allow + } + }; + assert_eq!( + timeout(Duration::from_secs(5), &mut decision) + .await + .expect("finish cancelled or accepted review"), + expected_decision, + ); + let key = + HostApprovalKey::from_request(&request, NetworkApprovalProtocol::Http, "local".to_string()); + assert_eq!( + service.session_approved_hosts.lock().await.contains(&key), + expected_decision == NetworkDecision::Allow, + ); + // A late callback must not resurrect the cancelled execution, even with a host grant. + service.session_approved_hosts.lock().await.insert(key); + assert_eq!( + service + .handle_inline_policy_request(Arc::clone(&session), request) + .await, + NetworkDecision::deny("not_allowed"), + ); + assert_eq!( + service.take_call_outcome("execution-1").await, + Some("original execution denial".to_string()) + ); + assert!(service.pending_host_approvals.lock().unwrap().is_empty()); + while let Ok(event) = events.try_recv() { + assert!(!matches!( + event.msg, + EventMsg::ExecApprovalRequest(_) | EventMsg::GuardianAssessment(_) + )); + } + session + .abort_all_tasks(codex_protocol::protocol::TurnAbortReason::Interrupted) + .await; +} + fn pending_key(host: HostApprovalKey, turn_id: &str, execution_id: &str) -> PendingHostApprovalKey { PendingHostApprovalKey { host, @@ -492,6 +638,7 @@ async fn register_call_with_default_shell_trigger( command: "curl https://example.com".to_string(), environment_id: "local".to_string(), permission_profile: PermissionProfile::workspace_write(), + environments: TurnEnvironmentSnapshot::default(), cancellation_token: cancellation_token.clone(), }) .await; @@ -522,6 +669,7 @@ async fn active_call_preserves_triggering_command_context() { command: "curl https://example.com".to_string(), environment_id: "remote".to_string(), permission_profile: PermissionProfile::workspace_write(), + environments: TurnEnvironmentSnapshot::default(), cancellation_token: CancellationToken::new(), }) .await; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index ad5086f500..ec5be46125 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -62,6 +62,7 @@ impl ToolOrchestrator { let network_approval = match begin_network_approval( &tool_ctx.session, &tool_ctx.step_context.turn, + &tool_ctx.step_context.environments, attempt.enforce_managed_network, network_approval_spec, ) 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 dfd66f583c..d0be8e1f9b 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 @@ -293,8 +293,9 @@ impl CoreShellActionProvider { })?; let approval_ctx = ApprovalContext { review_context: GuardianReviewContext::from_resolved_settings( - turn_context, + Arc::clone(&turn_context), &step_settings, + &turn_context.environments, ), // The running process can outlive its launching tool or code-mode cell. cancellation_token: None, diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 994fbb5f3b..245fba5db8 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -34,7 +34,6 @@ use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::AutoReviewMessages; use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER; use codex_protocol::openai_models::ModelTokenBudgetConfig; -use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -92,6 +91,7 @@ use wiremock::http::Method; use wiremock::matchers::method; use wiremock::matchers::path_regex; +use super::network_approval::guardian_parent_catalog; use super::rmcp_client::remote_aware_environment_id; use super::rmcp_client::remote_aware_stdio_server_bin; @@ -951,44 +951,6 @@ async fn guardian_node_repl_policy_follows_production_approval_path( Ok(()) } -fn guardian_parent_catalog() -> ModelsResponse { - let template = codex_models_manager::bundled_models_response() - .expect("bundled model catalog") - .models - .into_iter() - .find(|model| model.slug == "gpt-5.4") - .expect("gpt-5.4 in bundled catalog"); - // Keep safety settings compatible so active publication can switch A to B. - ModelsResponse { - models: ["guardian-parent-a", "guardian-parent-b"] - .into_iter() - .map(|slug| { - let mut model = template.clone(); - model.slug = slug.to_string(); - model.visibility = ModelVisibility::List; - model.auto_review_model_override = None; - model.supported_reasoning_levels.retain(|level| { - level.effort != codex_protocol::openai_models::ReasoningEffort::Low - }); - model - .model_messages - .as_mut() - .expect("model messages") - .auto_review = Some(AutoReviewMessages { - policy: Some("captured policy".to_string()), - policy_template: Some( - "captured template: {{ tenant_policy_config }}".to_string(), - ), - node_repl_policy: None, - rejection_instructions: None, - timeout_instructions: None, - }); - model - }) - .collect(), - } -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_reviews_delayed_and_new_actions_after_catalog_refresh() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index 4d6f92e966..283a5724d2 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -27,6 +27,9 @@ use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::models::PermissionProfileSnapshot; +use codex_protocol::openai_models::AutoReviewMessages; +use codex_protocol::openai_models::ModelVisibility; +use codex_protocol::openai_models::ModelsResponse; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EnvironmentConfigState; @@ -64,6 +67,7 @@ use core_test_support::skip_if_no_remote_env; use core_test_support::skip_if_sandbox; use core_test_support::skip_if_target_windows; use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::TestCodexBuilder; use core_test_support::test_codex::local; use core_test_support::test_codex::test_codex; use core_test_support::test_codex::turn_permission_fields; @@ -661,23 +665,31 @@ async fn timed_out_guardian_network_review_uses_timeout_outcome_without_user_fal Ok(()) } +#[test_case(true; "same_turn")] +#[test_case(false; "later_turn")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg_attr( not(target_os = "linux"), ignore = "requires the trusted Linux proxy bridge" )] -async fn background_network_approval_uses_active_turn_after_original_turn_completes() -> Result<()> -{ +async fn background_network_approval_uses_current_review_settings_and_original_execution( + same_turn: bool, +) -> Result<()> { skip_if_target_windows!(Ok(()), "uses the POSIX/Python network fixture"); skip_if_host_windows!(Ok(())); skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); let server = start_mock_server().await; - let test = managed_network_unified_exec_test_with_features( - &server, - &[Feature::RequestPermissionsTool], - ) + let test = managed_network_unified_exec_builder(&[ + Feature::RequestPermissionsTool, + Feature::StepModelSwitching, + ])? + .with_model("guardian-parent-a") + .with_config(|config| { + config.model_catalog = Some(guardian_parent_catalog()); + }) + .build_with_remote_and_local_env(&server) .await?; let start_call_id = "cross-turn-network-start"; let permission_call_id = "cross-turn-network-permissions"; @@ -689,73 +701,84 @@ async fn background_network_approval_uses_active_turn_after_original_turn_comple ..Default::default() }; let command = format!( - "read _; python3 -c \"import urllib.request; urllib.request.build_opener(urllib.request.ProxyHandler()).open('{NETWORK_TEST_TARGET}', timeout=2).read()\"; echo CROSS-TURN-NETWORK-COMPLETE; read _" + "read _; {}; echo CROSS-TURN-NETWORK-COMPLETE; read _", + remote_network_proxy_request_command("NETWORK-RESULT") ); let mut start_args = network_exec_args(&command); start_args["environment_id"] = json!(LOCAL_ENVIRONMENT_ID); start_args["tty"] = json!(true); start_args["yield_time_ms"] = json!(250); - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-cross-turn-network-start"), - ev_function_call( - start_call_id, - "exec_command", - &serde_json::to_string(&start_args)?, - ), - ev_completed("resp-cross-turn-network-start"), - ]), - sse(vec![ - ev_response_created("resp-cross-turn-network-first-complete"), - ev_assistant_message("msg-cross-turn-network-first-complete", "terminal started"), - ev_completed("resp-cross-turn-network-first-complete"), - ]), - sse(vec![ - ev_response_created("resp-cross-turn-network-permissions"), - ev_function_call( - permission_call_id, - "request_permissions", - &serde_json::to_string(&json!({ - "reason": "Automatically review the existing terminal's network access", - "permissions": requested_permissions, - }))?, - ), - ev_completed("resp-cross-turn-network-permissions"), - ]), - sse(vec![ - ev_response_created("resp-cross-turn-network-stdin"), - ev_function_call( - stdin_call_id, - "write_stdin", - &serde_json::to_string(&json!({ - "session_id": 1000, - "chars": "continue\n", - "yield_time_ms": 1_000, - }))?, - ), - ev_completed("resp-cross-turn-network-stdin"), - ]), - sse(vec![ - ev_response_created("resp-cross-turn-network-guardian"), - ev_assistant_message( - "msg-cross-turn-network-guardian", - r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The existing terminal's network request is safe."}"#, - ), - ev_completed("resp-cross-turn-network-guardian"), - ]), - sse(vec![ - ev_response_created("resp-cross-turn-network-second-complete"), - ev_assistant_message( - "msg-cross-turn-network-second-complete", - "network request approved", - ), - ev_completed("resp-cross-turn-network-second-complete"), - ]), - ], - ) - .await; + let mut events = vec![ + sse(vec![ + ev_response_created("resp-cross-turn-network-start"), + ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-cross-turn-network-start"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-first-complete"), + ev_assistant_message("msg-cross-turn-network-first-complete", "terminal started"), + ev_completed("resp-cross-turn-network-first-complete"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-permissions"), + ev_function_call( + permission_call_id, + "request_permissions", + &serde_json::to_string(&json!({ + "reason": "Automatically review the existing terminal's network access", + "permissions": requested_permissions, + }))?, + ), + ev_completed("resp-cross-turn-network-permissions"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-stdin"), + ev_function_call( + stdin_call_id, + "write_stdin", + &serde_json::to_string(&json!({ + "session_id": 1000, + "chars": "continue\n", + "yield_time_ms": 10_000, + }))?, + ), + ev_completed("resp-cross-turn-network-stdin"), + ]), + sse(vec![ + ev_response_created("resp-network-guardian-environment"), + ev_function_call( + "guardian-pwd", + "exec_command", + &network_exec_args("pwd").to_string(), + ), + ev_completed("resp-network-guardian-environment"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-guardian"), + ev_assistant_message( + "msg-cross-turn-network-guardian", + r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The existing terminal's network request is safe."}"#, + ), + ev_completed("resp-cross-turn-network-guardian"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-second-complete"), + ev_assistant_message( + "msg-cross-turn-network-second-complete", + "network request approved", + ), + ev_completed("resp-cross-turn-network-second-complete"), + ]), + ]; + if same_turn { + // Omit the first completion so the original turn stays active. + events.remove(/*index*/ 1); + } + let responses = mount_sse_sequence(&server, events).await; submit_managed_network_turn( &test, @@ -765,29 +788,54 @@ async fn background_network_approval_uses_active_turn_after_original_turn_comple AskForApproval::OnRequest, ) .await?; - let EventMsg::TurnComplete(first_turn) = wait_for_event(&test.codex, |event| { - matches!(event, EventMsg::TurnComplete(_)) - }) - .await - else { - unreachable!("matched first turn completion") - }; - assert_eq!(test.codex.list_background_terminals().await.len(), 1); - - submit_managed_network_turn( - &test, - "allow the existing background terminal to request network access", - vec![local(test.config.cwd.clone())], - ApprovalsReviewer::User, - AskForApproval::OnRequest, - ) - .await?; - let EventMsg::TurnStarted(active_turn) = wait_for_event(&test.codex, |event| { + let EventMsg::TurnStarted(first_turn) = wait_for_event(&test.codex, |event| { matches!(event, EventMsg::TurnStarted(_)) }) .await else { - unreachable!("matched second turn start") + unreachable!("matched first turn start") + }; + let current_cwd = test.cwd.path().join("later-turn"); + fs::create_dir(¤t_cwd)?; + let active_turn_id = if same_turn { + first_turn.turn_id.clone() + } else { + wait_for_turn_complete(&test).await; + assert_eq!(test.codex.list_background_terminals().await.len(), 1); + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "review the existing terminal under B".to_string(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + environments: Some(TurnEnvironmentSelections::new( + current_cwd.abs(), + vec![local(current_cwd.abs())], + )), + collaboration_mode: Some(CollaborationMode { + mode: ModeKind::Default, + settings: Settings { + model: "guardian-parent-b".to_string(), + reasoning_effort: Some( + codex_protocol::openai_models::ReasoningEffort::Medium, + ), + developer_instructions: None, + }, + }), + ..Default::default() + }), + ) + .await?; + let EventMsg::TurnStarted(active_turn) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnStarted(_)) + }) + .await + else { + unreachable!("matched second turn start") + }; + assert_ne!(active_turn.turn_id, first_turn.turn_id); + active_turn.turn_id }; let EventMsg::RequestPermissions(request) = wait_for_event(&test.codex, |event| { matches!(event, EventMsg::RequestPermissions(_)) @@ -797,6 +845,24 @@ async fn background_network_approval_uses_active_turn_after_original_turn_comple unreachable!("matched request permissions event") }; assert_eq!(request.call_id, permission_call_id); + if same_turn { + let (reply, outcome) = tokio::sync::oneshot::channel(); + test.codex + .submit(Op::TurnSettings { + turn_id: active_turn_id.clone(), + update: codex_protocol::protocol::TurnSettingsUpdate { + model: Some("guardian-parent-b".to_string()), + effort: Some(Some(codex_protocol::openai_models::ReasoningEffort::Medium)), + ..Default::default() + }, + reply, + }) + .await?; + assert_eq!( + tokio::time::timeout(Duration::from_secs(10), outcome).await??, + codex_protocol::protocol::TurnSettingsUpdateOutcome::Applied, + ); + } test.codex .submit(Op::RequestPermissionsResponse { id: permission_call_id.to_string(), @@ -819,26 +885,77 @@ async fn background_network_approval_uses_active_turn_after_original_turn_comple }) .await; let EventMsg::GuardianAssessment(assessment) = assessment else { - panic!("expected Guardian to approve the background terminal's network request"); + panic!("expected Guardian approval, got {assessment:?}"); }; - assert_eq!(assessment.turn_id, active_turn.turn_id); - assert_ne!(assessment.turn_id, first_turn.turn_id); + assert_eq!(assessment.turn_id, active_turn_id); wait_for_turn_complete(&test).await; let actions = guardian_network_actions(&responses)?; - assert_eq!(actions.len(), 1); + assert_eq!(actions.len(), 2); + assert_eq!(actions[0], actions[1]); + let original_command = get_shell(ShellType::Sh) + .context("expected local sh")? + .derive_exec_args(&command, /*use_login_shell*/ false); assert_eq!( - actions[0] - .pointer("/trigger/callId") - .and_then(Value::as_str), - Some(start_call_id) + actions[0]["trigger"], + json!({ + "callId": start_call_id, + "toolName": "exec_command", + "command": original_command, + "cwd": test.config.cwd, + "sandboxPermissions": "use_default", + "tty": true, + }) ); + let requests = responses.requests(); + let reviews = requests + .iter() + .filter(|request| request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian") + .collect::>(); + assert_eq!(reviews.len(), 2); + for review in &reviews { + assert_eq!(review.body_json()["model"], "guardian-parent-b"); + assert_eq!(review.body_json()["reasoning"]["effort"], "medium"); + } + let parent_requests = requests + .iter() + .filter(|request| request.body_json()["client_metadata"]["x-openai-subagent"] != "guardian") + .collect::>(); + assert_eq!(parent_requests[0].body_json()["model"], "guardian-parent-a"); + let final_parent = parent_requests + .last() + .context("parent completion request")?; + assert_eq!(final_parent.body_json()["model"], "guardian-parent-b"); + let metadata: Value = serde_json::from_str( + final_parent.body_json()["client_metadata"]["x-codex-turn-metadata"] + .as_str() + .context("Responses turn metadata")?, + )?; + // Strict review does not change this field's policy-routing meaning. + assert_eq!(metadata["auto_review_enabled"], false); + let pwd_output = reviews[1] + .function_call_output_text("guardian-pwd") + .context("Guardian pwd output")?; + let original_cwd = test + .config + .cwd + .as_path() + .to_str() + .context("UTF-8 original cwd")?; + let current_cwd = current_cwd.to_str().context("UTF-8 current cwd")?; + assert!(pwd_output.contains(original_cwd)); + assert!(!pwd_output.contains(current_cwd)); let stdin_output = responses .requests() .iter() .find_map(|request| request.function_call_output_text(stdin_call_id)) .context("expected background terminal network request output")?; assert!(!stdin_output.contains("blocked by policy")); + assert!( + stdin_output.contains("NETWORK-RESULT:HTTP/1.1 502") + || stdin_output.contains("NETWORK-RESULT:HTTP/1.1 200") + ); + assert!(stdin_output.contains("CROSS-TURN-NETWORK-COMPLETE")); assert_eq!( test.codex.list_background_terminals().await.len(), 1, @@ -1036,12 +1153,24 @@ async fn user_network_approval_once_session_and_denial_semantics() -> Result<()> Ok(()) } +#[derive(Clone, Copy)] +enum OtherNetworkReview { + Denied, + Pending, + ApprovedForSession, +} + +#[test_case(OtherNetworkReview::Denied; "latest_rejection")] +#[test_case(OtherNetworkReview::Pending; "pending_review")] +#[test_case(OtherNetworkReview::ApprovedForSession; "accepted_review")] #[tokio::test(flavor = "current_thread")] #[cfg_attr( not(target_os = "linux"), ignore = "requires the trusted Linux proxy bridge" )] -async fn latest_network_rejection_wins_for_multiple_reviews_of_one_execution() -> Result<()> { +async fn network_rejection_preserves_execution_and_review_outcomes( + other_review: OtherNetworkReview, +) -> Result<()> { skip_if_target_windows!(Ok(()), "uses the POSIX/Python network fixture"); skip_if_host_windows!(Ok(())); skip_if_no_network!(Ok(())); @@ -1049,20 +1178,62 @@ async fn latest_network_rejection_wins_for_multiple_reviews_of_one_execution() - let server = start_mock_server().await; let test = managed_network_unified_exec_test(&server).await?; + let local_cwd = test.cwd.path().abs(); let call_id = "network-multiple-reviews"; + let poll_call_id = "network-multiple-reviews-poll"; let second_target = format!("http://{NETWORK_TEST_HOST}:81"); let command = format!( - "python3 -c \"import threading,urllib.request; threads=[threading.Thread(target=urllib.request.build_opener(urllib.request.ProxyHandler()).open,args=(url,),kwargs={{'timeout': 10}}) for url in ('{NETWORK_TEST_TARGET}','{second_target}')]; [thread.start() for thread in threads]; [thread.join() for thread in threads]\"" + r#"read _; python3 - <<'PY' +import fcntl, os, pathlib, socket, threading, urllib.parse +process_lock = open('network-process-lock', 'w') +fcntl.flock(process_lock, fcntl.LOCK_EX) +proxy = urllib.parse.urlparse(os.environ['HTTP_PROXY']) +def fetch(port): + with socket.create_connection((proxy.hostname, proxy.port), timeout=30) as sock: + sock.sendall(f'GET http://{NETWORK_TEST_HOST}:{{port}}/ HTTP/1.1\r\nHost: {NETWORK_TEST_HOST}:{{port}}\r\nConnection: close\r\n\r\n'.encode()) + status = sock.makefile('rb').readline().decode().strip() + result = pathlib.Path(f'network-result-{{port}}') + result.with_suffix('.tmp').write_text(status) + result.with_suffix('.tmp').replace(result) +threads = [threading.Thread(target=fetch, args=(port,)) for port in (80, 81)] +for thread in threads: + thread.start() +for thread in threads: + thread.join() +threading.Event().wait() +PY"# ); let mut args = network_exec_args(&command); - args["yield_time_ms"] = json!(10_000); - let responses = - mount_exec_network_turn(&server, "resp-network-multiple-reviews", call_id, args).await?; + args["yield_time_ms"] = json!(250); + args["tty"] = json!(true); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_function_call(call_id, "exec_command", &args.to_string()), + ev_completed("network-start"), + ]), + sse(vec![ + ev_function_call( + poll_call_id, + "write_stdin", + &json!({"session_id": 1000, "chars": "start\n", "yield_time_ms": 10_000}) + .to_string(), + ), + ev_completed("network-poll"), + ]), + sse(vec![ + ev_assistant_message("network-done", "done"), + ev_completed("network-done"), + ]), + ], + ) + .await; submit_managed_network_turn( &test, "review both network requests from one execution", - vec![local(test.config.cwd.clone())], + vec![local(local_cwd.clone())], ApprovalsReviewer::User, AskForApproval::OnRequest, ) @@ -1086,40 +1257,107 @@ async fn latest_network_rejection_wins_for_multiple_reviews_of_one_execution() - }; approvals.push(approval); } + assert_eq!(test.codex.list_background_terminals().await.len(), 1); + // The child holds this lock until exit, including across the sandbox's PID namespace. + let process_lock = fs::File::open(local_cwd.join("network-process-lock"))?; + assert!(matches!( + process_lock.try_lock(), + Err(fs::TryLockError::WouldBlock) + )); assert_eq!(approvals[0].turn_id, approvals[1].turn_id); - let mut actual_targets = approvals + approvals.sort_by_key(|approval| approval.command[1].clone()); + let actual_targets = approvals .iter() .map(|approval| approval.command[1].clone()) .collect::>(); - actual_targets.sort(); let mut expected_targets = vec![NETWORK_TEST_TARGET.to_string(), second_target]; expected_targets.sort(); assert_eq!(actual_targets, expected_targets); let first_rejection = "first network approval was rejected"; let latest_rejection = "latest network approval was rejected"; - for (approval, rejection) in approvals - .into_iter() - .zip([first_rejection, latest_rejection]) - { + let first_approval = &approvals[0]; + let first_decision = match other_review { + OtherNetworkReview::Denied => Some(ReviewDecision::denied(first_rejection)), + OtherNetworkReview::Pending => None, + OtherNetworkReview::ApprovedForSession => Some(ReviewDecision::ApprovedForSession), + }; + if let Some(decision) = first_decision { test.codex .submit(Op::ExecApproval { - id: approval.effective_approval_id(), - turn_id: Some(approval.turn_id), - decision: ReviewDecision::denied(rejection), + id: first_approval.effective_approval_id(), + turn_id: Some(first_approval.turn_id.clone()), + decision, }) .await?; } - wait_for_turn_complete(&test).await; + if matches!(other_review, OtherNetworkReview::ApprovedForSession) { + // Wait for the process to observe the proxy response before cancelling its execution. + let result_path = local_cwd.join("network-result-80"); + wait_for_paths(&[&result_path]).await?; + let result = fs::read_to_string(result_path)?; + assert!(result.starts_with("HTTP/1.1 200") || result.starts_with("HTTP/1.1 502")); + } + let final_approval = &approvals[1]; + test.codex + .submit(Op::ExecApproval { + id: final_approval.effective_approval_id(), + turn_id: Some(final_approval.turn_id.clone()), + decision: ReviewDecision::denied(latest_rejection), + }) + .await?; + wait_for_completion_without_network_prompt(&test).await; + tokio::time::timeout(Duration::from_secs(5), async { + loop { + match process_lock.try_lock() { + Ok(()) => return Ok(()), + Err(fs::TryLockError::WouldBlock) => { + tokio::time::sleep(Duration::from_millis(10)).await; + } + Err(fs::TryLockError::Error(error)) => return Err(error), + } + } + }) + .await + .context("background process remained alive after network rejection")??; + assert!(test.codex.list_background_terminals().await.is_empty()); let output = responses .requests() .iter() - .find_map(|request| request.function_call_output_text(call_id)) + .find_map(|request| request.function_call_output_text(poll_call_id)) .context("expected output from the execution with multiple network reviews")?; assert!(output.contains(latest_rejection), "{output}"); assert!(!output.contains(first_rejection), "{output}"); + if matches!(other_review, OtherNetworkReview::ApprovedForSession) { + let probe_call_id = "network-session-grant-probe"; + let probe = mount_exec_network_turn( + &server, + "network-session-grant-probe", + probe_call_id, + network_exec_args(&remote_network_proxy_request_command("REUSED-GRANT")), + ) + .await?; + submit_managed_network_turn( + &test, + "reuse the accepted host grant after the original execution was cancelled", + vec![local(local_cwd.clone())], + ApprovalsReviewer::User, + AskForApproval::OnRequest, + ) + .await?; + wait_for_completion_without_network_prompt(&test).await; + let output = probe + .function_call_output_text(probe_call_id) + .context("expected network output from the new execution")?; + assert!( + output.contains("REUSED-GRANT:HTTP/1.1 200") + || output.contains("REUSED-GRANT:HTTP/1.1 502"), + "{output}", + ); + } + Ok(()) } @@ -2349,6 +2587,44 @@ async fn approved_network_host_for_one_environment_still_prompts_in_another() -> Ok(()) } +pub(super) fn guardian_parent_catalog() -> ModelsResponse { + let template = codex_models_manager::bundled_models_response() + .expect("bundled model catalog") + .models + .into_iter() + .find(|model| model.slug == "gpt-5.4") + .expect("gpt-5.4 in bundled catalog"); + // Keep safety settings compatible so active publication can switch A to B. + ModelsResponse { + models: ["guardian-parent-a", "guardian-parent-b"] + .into_iter() + .map(|slug| { + let mut model = template.clone(); + model.slug = slug.to_string(); + model.visibility = ModelVisibility::List; + model.auto_review_model_override = None; + model.supported_reasoning_levels.retain(|level| { + level.effort != codex_protocol::openai_models::ReasoningEffort::Low + }); + model + .model_messages + .as_mut() + .expect("model messages") + .auto_review = Some(AutoReviewMessages { + policy: Some("captured policy".to_string()), + policy_template: Some( + "captured template: {{ tenant_policy_config }}".to_string(), + ), + node_repl_policy: None, + rejection_instructions: None, + timeout_instructions: None, + }); + model + }) + .collect(), + } +} + async fn managed_network_unified_exec_test(server: &wiremock::MockServer) -> Result { managed_network_unified_exec_test_with_features(server, &[]).await } @@ -2357,6 +2633,16 @@ async fn managed_network_unified_exec_test_with_features( server: &wiremock::MockServer, features: &[Feature], ) -> Result { + let test = managed_network_unified_exec_builder(features)? + .build_with_remote_and_local_env(server) + .await?; + assert!(test.config.managed_network_requirements_enabled()); + assert!(test.config.permissions.network.is_some()); + assert!(test.session_configured.network_proxy.is_some()); + Ok(test) +} + +fn managed_network_unified_exec_builder(features: &[Feature]) -> Result { let home = Arc::new(TempDir::new()?); fs::write( home.path().join("config.toml"), @@ -2378,9 +2664,9 @@ allow_local_binding = true /*exclude_tmpdir_env_var*/ false, /*exclude_slash_tmp*/ false, ); - let permission_profile_for_config = permission_profile.clone(); + let permission_profile_for_config = permission_profile; let features = features.to_vec(); - let mut builder = test_codex() + Ok(test_codex() .with_home(home) .with_cloud_config_bundle(managed_network_requirements_loader()) .with_config(move |config| { @@ -2395,22 +2681,7 @@ allow_local_binding = true .permissions .set_permission_profile(permission_profile_for_config) .expect("set permission profile"); - }); - let test = builder.build_with_remote_and_local_env(server).await?; - assert!( - test.config.managed_network_requirements_enabled(), - "expected managed network requirements to be enabled" - ); - assert!( - test.config.permissions.network.is_some(), - "expected managed network proxy config to be present" - ); - test.session_configured - .network_proxy - .as_ref() - .expect("expected runtime managed network proxy addresses"); - - Ok(test) + })) } async fn mount_exec_network_turn(