Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions codex-rs/core/src/guardian/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ pub(crate) use review::is_guardian_reviewer_source;
pub(crate) use review::new_guardian_review_id;
#[cfg(test)]
pub(crate) use review::record_guardian_denial_for_test;
pub(crate) use review::reset_auto_review_rejection_circuit_breaker;
pub(crate) use review::review_approval_request;
#[cfg(test)]
pub(crate) use review::review_approval_request_with_cancel;
pub(crate) use review::routes_approval_to_guardian;
pub(crate) use review::spawn_approval_request_review;
pub(crate) use review::take_pending_auto_review_escalation;
pub(crate) use review_session::GuardianReviewSessionManager;

const GUARDIAN_PREFERRED_MODEL: &str = "codex-auto-review";
Expand Down Expand Up @@ -118,6 +120,12 @@ impl GuardianRejectionCircuitBreaker {
let turn = self.turns.entry(turn_id.to_string()).or_default();
turn.consecutive_denials = 0;
}

pub(crate) fn take_pending_auto_review_escalation(&mut self, turn_id: &str) -> bool {
self.turns
.get_mut(turn_id)
.is_some_and(|turn| std::mem::take(&mut turn.interrupt_triggered))
}
}

#[cfg(test)]
Expand Down
34 changes: 24 additions & 10 deletions codex-rs/core/src/guardian/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use codex_protocol::protocol::GuardianRiskLevel;
use codex_protocol::protocol::GuardianUserAuthorization;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::SubAgentSource;
use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::WarningEvent;
use tokio::sync::oneshot;
use tokio_util::sync::CancellationToken;
Expand Down Expand Up @@ -202,20 +201,35 @@ async fn record_guardian_denial(session: &Arc<Session>, turn: &Arc<TurnContext>,
turn.as_ref(),
EventMsg::GuardianWarning(WarningEvent {
message: format!(
"Automatic approval review rejected too many approval requests for this turn ({consecutive_denials} consecutive, {total_denials} total); interrupting the turn."
"Automatic approval review rejected too many approval requests for this turn ({consecutive_denials} consecutive, {total_denials} total); requesting manual approval for the current request."
),
}),
)
.await;
}

let runtime_handle = session.services.runtime_handle.clone();
let session = Arc::clone(session);
let turn_id = turn_id.to_string();
let _abort_task = runtime_handle.spawn(async move {
session
.abort_turn_if_active(&turn_id, TurnAbortReason::Interrupted)
.await;
});
pub(crate) async fn reset_auto_review_rejection_circuit_breaker(
session: &Arc<Session>,
turn_id: &str,
) {
session
.services
.guardian_rejection_circuit_breaker
.lock()
.await
.clear_turn(turn_id);
}

pub(crate) async fn take_pending_auto_review_escalation(
session: &Arc<Session>,
turn_id: &str,
) -> bool {
session
.services
.guardian_rejection_circuit_breaker
.lock()
.await
.take_pending_auto_review_escalation(turn_id)
}

#[cfg(test)]
Expand Down
34 changes: 24 additions & 10 deletions codex-rs/core/src/mcp_tool_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ use crate::guardian::guardian_approval_request_to_json;
use crate::guardian::guardian_rejection_message;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::reset_auto_review_rejection_circuit_breaker;
use crate::guardian::review_approval_request;
use crate::guardian::routes_approval_to_guardian;
use crate::guardian::take_pending_auto_review_escalation;
use crate::hook_runtime::run_permission_request_hooks;
use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files;
use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam;
Expand Down Expand Up @@ -958,6 +960,7 @@ async fn maybe_request_mcp_tool_approval(
.features
.enabled(Feature::ToolCallMcpElicitation);

let mut escalated_from_guardian = false;
if routes_approval_to_guardian(turn_context) {
let review_id = new_guardian_review_id();
let decision = review_approval_request(
Expand All @@ -968,16 +971,21 @@ async fn maybe_request_mcp_tool_approval(
monitor_reason.clone(),
)
.await;
let decision = mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await;
apply_mcp_tool_approval_decision(
sess,
turn_context,
&decision,
session_approval_key,
persistent_approval_key,
)
.await;
return Some(decision);
escalated_from_guardian = matches!(decision, ReviewDecision::Denied)
&& take_pending_auto_review_escalation(sess, &turn_context.sub_id).await;
if !escalated_from_guardian {
let decision =
mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await;
apply_mcp_tool_approval_decision(
sess,
turn_context,
&decision,
session_approval_key,
persistent_approval_key,
)
.await;
return Some(decision);
}
}

let prompt_options = mcp_tool_approval_prompt_options(
Expand Down Expand Up @@ -1039,6 +1047,9 @@ async fn maybe_request_mcp_tool_approval(
&question_id,
);
let decision = normalize_approval_decision_for_mode(decision, approval_mode);
if escalated_from_guardian {
reset_auto_review_rejection_circuit_breaker(sess, &turn_context.sub_id).await;
}
apply_mcp_tool_approval_decision(
sess,
turn_context,
Expand All @@ -1060,6 +1071,9 @@ async fn maybe_request_mcp_tool_approval(
parse_mcp_tool_approval_response(response, &question_id),
approval_mode,
);
if escalated_from_guardian {
reset_auto_review_rejection_circuit_breaker(sess, &turn_context.sub_id).await;
}
apply_mcp_tool_approval_decision(
sess,
turn_context,
Comment thread
won-openai marked this conversation as resolved.
Expand Down
104 changes: 60 additions & 44 deletions codex-rs/core/src/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1988,7 +1988,9 @@ impl Session {
}

let requested_permissions = args.permissions;
let request_reason = args.reason;

let mut escalated_from_guardian = false;
if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) {
let originating_turn_state = {
let active = self.active_turn.lock().await;
Expand All @@ -1998,9 +2000,9 @@ impl Session {
let session = Arc::clone(self);
let turn = Arc::clone(turn_context);
let request = crate::guardian::GuardianApprovalRequest::RequestPermissions {
id: call_id,
id: call_id.clone(),
turn_id: turn_context.sub_id.clone(),
reason: args.reason,
reason: request_reason.clone(),
permissions: requested_permissions.clone(),
};
let review_rx = crate::guardian::spawn_approval_request_review(
Expand All @@ -2017,52 +2019,58 @@ impl Session {
_ = cancellation_token.cancelled() => return None,
decision = review_rx => decision.unwrap_or(ReviewDecision::Denied),
};
let response = match decision {
ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
escalated_from_guardian = matches!(decision, ReviewDecision::Denied)
&& crate::guardian::take_pending_auto_review_escalation(self, &turn_context.sub_id)
.await;
if !escalated_from_guardian {
let response = match decision {
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
}
}
ReviewDecision::ApprovedForSession => RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Session,
strict_auto_review: false,
},
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => RequestPermissionsResponse {
ReviewDecision::ApprovedForSession => RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
scope: PermissionGrantScope::Session,
strict_auto_review: false,
},
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
},
},
ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => {
RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => {
RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
}
}
};
let response = Self::normalize_request_permissions_response(
requested_permissions,
response,
cwd.as_path(),
);
self.record_granted_request_permissions_for_turn(
&response,
originating_turn_state.as_ref(),
)
.await;
return Some(response);
};
let response = Self::normalize_request_permissions_response(
requested_permissions,
response,
cwd.as_path(),
);
self.record_granted_request_permissions_for_turn(
&response,
originating_turn_state.as_ref(),
)
.await;
return Some(response);
}
}

let (tx_response, rx_response) = oneshot::channel();
Expand Down Expand Up @@ -2090,12 +2098,12 @@ impl Session {
let event = EventMsg::RequestPermissions(RequestPermissionsEvent {
call_id: call_id.clone(),
turn_id: turn_context.sub_id.clone(),
reason: args.reason,
reason: request_reason,
permissions: requested_permissions,
cwd: Some(cwd),
});
self.send_event(turn_context.as_ref(), event).await;
tokio::select! {
let response = tokio::select! {
biased;
_ = cancellation_token.cancelled() => {
let mut active = self.active_turn.lock().await;
Expand All @@ -2106,7 +2114,15 @@ impl Session {
None
}
response = rx_response => response.ok(),
};
if escalated_from_guardian {
crate::guardian::reset_auto_review_rejection_circuit_breaker(
self,
&turn_context.sub_id,
)
.await;
}
response
}

#[expect(
Expand Down
1 change: 0 additions & 1 deletion codex-rs/core/src/session/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,6 @@ impl Session {
tool_approvals: Mutex::new(ApprovalStore::default()),
guardian_rejections: Mutex::new(HashMap::new()),
guardian_rejection_circuit_breaker: Mutex::new(Default::default()),
runtime_handle: tokio::runtime::Handle::current(),
skills_manager,
plugins_manager: Arc::clone(&plugins_manager),
mcp_manager: Arc::clone(&mcp_manager),
Expand Down
Loading
Loading