diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 507d49d7fa..3b809e57a9 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -69,7 +69,7 @@ pub(crate) struct NetworkApprovalSpec { pub(crate) struct DeferredNetworkApproval { registration_id: String, cancellation_token: CancellationToken, - finish_outcome: Arc>>, + finish_outcome: Arc>>, _execution_proxy: Option, } @@ -186,32 +186,17 @@ enum PendingApprovalDecision { Deny, } -#[derive(Clone, Debug, Eq, PartialEq)] -enum NetworkApprovalOutcome { - DeniedByApproval(String), - DeniedByPolicy(String), -} - -fn network_approval_outcome_to_result( - outcome: Option, -) -> Result<(), ToolError> { +fn network_approval_outcome_to_result(outcome: Option) -> Result<(), ToolError> { match outcome { - Some(NetworkApprovalOutcome::DeniedByApproval(rejection)) => { - Err(ToolError::Rejected(truncate_rejection_message(&rejection))) - } - Some(NetworkApprovalOutcome::DeniedByPolicy(message)) => { - Err(ToolError::Rejected(truncate_rejection_message(&message))) - } + Some(rejection) => Err(ToolError::Rejected(truncate_rejection_message(&rejection))), None => Ok(()), } } -fn abandoned_network_approval_outcome( - cancellation_token: &CancellationToken, -) -> Option { - cancellation_token.is_cancelled().then(|| { - NetworkApprovalOutcome::DeniedByPolicy(ABANDONED_NETWORK_APPROVAL_MESSAGE.to_string()) - }) +fn abandoned_network_approval_outcome(cancellation_token: &CancellationToken) -> Option { + cancellation_token + .is_cancelled() + .then(|| ABANDONED_NETWORK_APPROVAL_MESSAGE.to_string()) } /// Whether an allowlist miss may be reviewed instead of hard-denied. @@ -286,7 +271,7 @@ struct NetworkRequestAttribution { #[derive(Default)] struct NetworkApprovalCallState { active_calls: IndexMap>, - call_outcomes: HashMap, + call_outcomes: HashMap, } pub(crate) struct NetworkApprovalService { @@ -514,22 +499,17 @@ impl NetworkApprovalService { } #[cfg(test)] - async fn take_call_outcome(&self, registration_id: &str) -> Option { + async fn take_call_outcome(&self, registration_id: &str) -> Option { let mut calls = self.calls.lock().await; calls.call_outcomes.remove(registration_id) } - async fn record_call_outcome(&self, registration_id: &str, outcome: NetworkApprovalOutcome) { + async fn record_call_outcome(&self, registration_id: &str, outcome: String) { let mut calls = self.calls.lock().await; let Some(call) = calls.active_calls.get(registration_id).cloned() else { return; }; - if matches!( - calls.call_outcomes.get(registration_id), - Some(NetworkApprovalOutcome::DeniedByApproval(_)) - ) { - return; - } + // Explicit network-review outcomes replace generic blocked-request fallbacks. calls .call_outcomes .insert(registration_id.to_string(), outcome); @@ -538,13 +518,13 @@ impl NetworkApprovalService { call.cancellation_token.cancel(); } - async fn remove_call(&self, registration_id: &str) -> Option { + async fn remove_call(&self, registration_id: &str) -> Option { let mut calls = self.calls.lock().await; calls.active_calls.shift_remove(registration_id); calls.call_outcomes.remove(registration_id) } - async fn finish_call_outcome(&self, registration_id: &str) -> Option { + async fn finish_call_outcome(&self, registration_id: &str) -> Option { self.remove_call(registration_id).await } @@ -581,10 +561,9 @@ impl NetworkApprovalService { { return; } - calls.call_outcomes.insert( - owner_call.registration_id.clone(), - NetworkApprovalOutcome::DeniedByPolicy(message), - ); + calls + .call_outcomes + .insert(owner_call.registration_id.clone(), message); drop(calls); owner_call.cancellation_token.cancel(); @@ -668,11 +647,8 @@ impl NetworkApprovalService { let Some(turn_context) = turn_context else { if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), - ) - .await; + self.record_call_outcome(&owner_call.registration_id, policy_denial_message) + .await; } return NetworkDecision::deny(REASON_NOT_ALLOWED); }; @@ -710,22 +686,16 @@ impl NetworkApprovalService { }); if !permission_profile.is_some_and(permission_profile_allows_network_approval_flow) { if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), - ) - .await; + self.record_call_outcome(&owner_call.registration_id, policy_denial_message) + .await; } pending_owner.complete(PendingApprovalDecision::Deny); return NetworkDecision::deny(REASON_NOT_ALLOWED); } if !allows_network_approval_flow(turn_context.approval_policy()) { if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), - ) - .await; + self.record_call_outcome(&owner_call.registration_id, policy_denial_message) + .await; } pending_owner.complete(PendingApprovalDecision::Deny); return NetworkDecision::deny(REASON_NOT_ALLOWED); @@ -755,11 +725,8 @@ impl NetworkApprovalService { Some(PermissionRequestDecision::Allow) => Some(ReviewDecision::Approved), Some(PermissionRequestDecision::Deny { message }) => { if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(message), - ) - .await; + self.record_call_outcome(&owner_call.registration_id, message) + .await; } pending_owner.complete(PendingApprovalDecision::Deny); return NetworkDecision::deny(REASON_NOT_ALLOWED); @@ -859,7 +826,7 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message.clone()), + policy_denial_message.clone(), ) .await; } @@ -873,7 +840,7 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message.clone()), + policy_denial_message.clone(), ) .await; } @@ -929,9 +896,7 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy( - policy_denial_message.clone(), - ), + policy_denial_message.clone(), ) .await; } @@ -972,9 +937,7 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByApproval( - "rejected by user".to_string(), - ), + "rejected by user".to_string(), ) .await; } @@ -989,25 +952,17 @@ impl NetworkApprovalService { ReviewDecision::ApprovedMcpPolicyAmendment => { error!("Network approval received ApprovedMcpPolicyAmendment"); if let Some(owner_call) = owner_call.as_ref() { - let rejection = "Error while requesting approval".to_string(); - let outcome = if use_guardian { - NetworkApprovalOutcome::DeniedByPolicy(rejection) - } else { - NetworkApprovalOutcome::DeniedByApproval(rejection) - }; - self.record_call_outcome(&owner_call.registration_id, outcome) - .await; + self.record_call_outcome( + &owner_call.registration_id, + "Error while requesting approval".to_string(), + ) + .await; } PendingApprovalDecision::Deny } ReviewDecision::Denied { rejection } => { if let Some(owner_call) = owner_call.as_ref() { - let outcome = if use_guardian { - NetworkApprovalOutcome::DeniedByPolicy(rejection) - } else { - NetworkApprovalOutcome::DeniedByApproval(rejection) - }; - self.record_call_outcome(&owner_call.registration_id, outcome) + self.record_call_outcome(&owner_call.registration_id, rejection) .await; } PendingApprovalDecision::Deny @@ -1016,9 +971,7 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy( - crate::guardian::guardian_timeout_message(), - ), + crate::guardian::guardian_timeout_message(), ) .await; } @@ -1029,16 +982,14 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy( - "automatic approval review was cancelled".to_string(), - ), + "automatic approval review was cancelled".to_string(), ) .await; } } else if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByApproval("rejected by user".to_string()), + "rejected by user".to_string(), ) .await; } diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index afcdbecdda..b83e972485 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -559,9 +559,9 @@ async fn record_blocked_request_sets_policy_outcome_for_owner_call() { assert!(cancellation_token.is_cancelled()); assert_eq!( service.take_call_outcome("registration-1").await, - Some(NetworkApprovalOutcome::DeniedByPolicy( + Some( "Network access to \"example.com\" was blocked: domain is not on the allowlist for the current sandbox mode.".to_string() - )) + ) ); } @@ -572,10 +572,7 @@ async fn blocked_request_does_not_override_recorded_approval_outcome() { let rejection = "approval client unavailable"; service - .record_call_outcome( - "registration-1", - NetworkApprovalOutcome::DeniedByApproval(rejection.to_string()), - ) + .record_call_outcome("registration-1", rejection.to_string()) .await; service .record_blocked_request(denied_blocked_request("example.com")) @@ -597,10 +594,7 @@ async fn specific_approval_outcome_replaces_earlier_blocked_request() { .record_blocked_request(denied_blocked_request("example.com")) .await; service - .record_call_outcome( - "registration-1", - NetworkApprovalOutcome::DeniedByApproval(rejection.to_string()), - ) + .record_call_outcome("registration-1", rejection.to_string()) .await; let error = @@ -609,14 +603,33 @@ async fn specific_approval_outcome_replaces_earlier_blocked_request() { assert!(matches!(error, ToolError::Rejected(message) if message == rejection)); } +#[tokio::test] +async fn latest_specific_approval_outcome_replaces_earlier_specific_outcome() { + let service = NetworkApprovalService::default(); + register_call_with_default_shell_trigger(&service, "registration-1").await; + + service + .record_call_outcome("registration-1", "earlier approval rejection".to_string()) + .await; + service + .record_call_outcome("registration-1", "latest approval rejection".to_string()) + .await; + + let error = + network_approval_outcome_to_result(service.take_call_outcome("registration-1").await) + .expect_err("latest approval rejection should remain an error"); + assert!(matches!( + error, + ToolError::Rejected(message) if message == "latest approval rejection" + )); +} + #[test] fn approval_denial_messages_are_bounded_for_model_context() { let rejection = "x".repeat(40_000); - let error = network_approval_outcome_to_result(Some(NetworkApprovalOutcome::DeniedByApproval( - rejection, - ))) - .expect_err("approval denial should remain an error"); + let error = network_approval_outcome_to_result(Some(rejection)) + .expect_err("approval denial should remain an error"); let ToolError::Rejected(message) = error else { panic!("approval denial should produce a rejected tool error"); }; @@ -632,10 +645,7 @@ async fn finish_call_returns_denial_and_unregisters_active_call() { register_call_with_default_shell_trigger(&service, "registration-1").await; service - .record_call_outcome( - "registration-1", - NetworkApprovalOutcome::DeniedByPolicy("network denied".to_string()), - ) + .record_call_outcome("registration-1", "network denied".to_string()) .await; let err = service @@ -678,10 +688,7 @@ async fn deferred_finish_reuses_denial_result_after_first_consumer() { _execution_proxy: None, }; service - .record_call_outcome( - "registration-1", - NetworkApprovalOutcome::DeniedByPolicy("network denied".to_string()), - ) + .record_call_outcome("registration-1", "network denied".to_string()) .await; let first = deferred @@ -705,10 +712,7 @@ async fn record_call_outcome_ignores_inactive_call() { service.unregister_call("registration-1").await; service - .record_call_outcome( - "registration-1", - NetworkApprovalOutcome::DeniedByPolicy("network denied".to_string()), - ) + .record_call_outcome("registration-1", "network denied".to_string()) .await; assert!(!cancellation_token.is_cancelled()); @@ -747,8 +751,8 @@ async fn attributed_blocked_request_targets_one_of_multiple_active_calls() { assert_eq!(service.take_call_outcome("registration-1").await, None); assert_eq!( service.take_call_outcome("registration-2").await, - Some(NetworkApprovalOutcome::DeniedByPolicy( + Some( "Network access to \"example.com\" was blocked: domain is not on the allowlist for the current sandbox mode.".to_string() - )) + ) ); } diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index 5e2975fcad..5015dd9b79 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -594,6 +594,93 @@ async fn user_network_approval_once_session_and_denial_semantics() -> Result<()> Ok(()) } +#[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<()> { + 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(&server).await?; + let call_id = "network-multiple-reviews"; + 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]\"" + ); + 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?; + + submit_managed_network_turn( + &test, + "review both network requests from one execution", + vec![local(test.config.cwd.clone())], + ApprovalsReviewer::User, + AskForApproval::OnRequest, + ) + .await?; + + let mut approvals = Vec::new(); + for _ in 0..2 { + let event = wait_for_event_with_timeout( + &test.codex, + |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }, + Duration::from_secs(30), + ) + .await; + let EventMsg::ExecApprovalRequest(approval) = event else { + anyhow::bail!("execution completed before both network approvals were requested"); + }; + approvals.push(approval); + } + assert_eq!(approvals[0].turn_id, approvals[1].turn_id); + let mut 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]) + { + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: Some(approval.turn_id), + decision: ReviewDecision::denied(rejection), + }) + .await?; + } + wait_for_turn_complete(&test).await; + + let output = responses + .requests() + .iter() + .find_map(|request| request.function_call_output_text(call_id)) + .context("expected output from the execution with multiple network reviews")?; + assert!(output.contains(latest_rejection), "{output}"); + assert!(!output.contains(first_rejection), "{output}"); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg_attr( not(target_os = "linux"),