openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-30 19:18:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Report the latest rejection from multiple network reviews (#38256)

Browse Source 
## Why

A single execution can trigger multiple network approval reviews. Its tool
result should reflect the latest specific rejection.

## What changed

- Let each explicit network review outcome replace the previously recorded
  outcome for the execution.
- Keep generic blocked-request messages as fallbacks that cannot overwrite an
  explicit review outcome.

## Testing

- Add unit coverage for outcome precedence.
- Add an integration test that rejects two network requests from one execution
  and verifies that only the latest rejection is returned.

GitOrigin-RevId: 644a01e5c49855adcdc48ba8f91baededce26875
This commit is contained in:
Dylan Hurd
2026-08-12 21:55:31 +00:00
committed by copyberry
parent 6e7daed1e9
commit 020f6c963e
3 changed files with 156 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/tools/network_approval.rs
+37 -86
View File
@@ -69,7 +69,7 @@ pub(crate) struct NetworkApprovalSpec {
pub(crate) struct DeferredNetworkApproval {
registration_id: String,
cancellation_token: CancellationToken,
finish_outcome: Arc<OnceCell<Option<NetworkApprovalOutcome>>>,
finish_outcome: Arc<OnceCell<Option<String>>>,
_execution_proxy: Option<NetworkProxy>,
}
@@ -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<NetworkApprovalOutcome>,
) -> Result<(), ToolError> {
fn network_approval_outcome_to_result(outcome: Option<String>) -> 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<NetworkApprovalOutcome> {
cancellation_token.is_cancelled().then(|| {
NetworkApprovalOutcome::DeniedByPolicy(ABANDONED_NETWORK_APPROVAL_MESSAGE.to_string())
})
fn abandoned_network_approval_outcome(cancellation_token: &CancellationToken) -> Option<String> {
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<String, Arc<ActiveNetworkApprovalCall>>,
call_outcomes: HashMap<String, NetworkApprovalOutcome>,
call_outcomes: HashMap<String, String>,
}
pub(crate) struct NetworkApprovalService {
@@ -514,22 +499,17 @@ impl NetworkApprovalService {
}
#[cfg(test)]
async fn take_call_outcome(&self, registration_id: &str) -> Option<NetworkApprovalOutcome> {
async fn take_call_outcome(&self, registration_id: &str) -> Option<String> {
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<NetworkApprovalOutcome> {
async fn remove_call(&self, registration_id: &str) -> Option<String> {
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<NetworkApprovalOutcome> {
async fn finish_call_outcome(&self, registration_id: &str) -> Option<String> {
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;
}
codex-rs/core/src/tools/network_approval_tests.rs
+32 -28
View File
@@ -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()
))
)
);
}