From 3e5266aeb6dfc6133ae15afefb6703712aa60f7a Mon Sep 17 00:00:00 2001 From: won Date: Fri, 1 May 2026 12:28:53 -0700 Subject: [PATCH] first draft --- codex-rs/core/src/guardian/mod.rs | 8 ++ codex-rs/core/src/guardian/review.rs | 57 ++++++++-- codex-rs/core/src/mcp_tool_call.rs | 34 ++++-- codex-rs/core/src/session/mod.rs | 104 ++++++++++-------- codex-rs/core/src/session/session.rs | 1 - codex-rs/core/src/session/tests.rs | 34 +++--- codex-rs/core/src/state/service.rs | 2 - codex-rs/core/src/tasks/mod.rs | 45 -------- codex-rs/core/src/tools/network_approval.rs | 31 +++++- .../core/src/tools/runtimes/apply_patch.rs | 32 ++++-- codex-rs/core/src/tools/runtimes/shell.rs | 64 ++++++----- .../tools/runtimes/shell/unix_escalation.rs | 21 +++- .../core/src/tools/runtimes/unified_exec.rs | 64 ++++++----- 13 files changed, 306 insertions(+), 191 deletions(-) diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 256a616b97..bcb42c7e7a 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -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"; @@ -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)] diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 850d84dd2a..4880dc4503 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -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; @@ -55,6 +54,9 @@ const GUARDIAN_TIMEOUT_INSTRUCTIONS: &str = concat!( "You may retry once, or ask the user for guidance or explicit approval.", ); +const FORCE_GUARDIAN_DENY_FOR_LOCAL_TESTING: bool = true; +const FORCED_GUARDIAN_DENIAL_RATIONALE: &str = "Auto-review forced denial for local testing."; + pub(crate) fn new_guardian_review_id() -> String { uuid::Uuid::new_v4().to_string() } @@ -71,6 +73,14 @@ pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &st rationale: "Auto-reviewer denied the action without a specific rationale.".to_string(), source: GuardianAssessmentDecisionSource::Agent, }); + if FORCE_GUARDIAN_DENY_FOR_LOCAL_TESTING + && rejection.rationale == FORCED_GUARDIAN_DENIAL_RATIONALE + { + return format!( + "This action was rejected by forced local auto-review denial.\nReason: {}\nRetry.", + rejection.rationale + ); + } match rejection.source { GuardianAssessmentDecisionSource::Agent => format!( "This action was rejected due to unacceptable risk.\nReason: {}\n{}", @@ -202,20 +212,35 @@ async fn record_guardian_denial(session: &Arc, turn: &Arc, 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." + "Auto-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, + 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, + turn_id: &str, +) -> bool { + session + .services + .guardian_rejection_circuit_breaker + .lock() + .await + .take_pending_auto_review_escalation(turn_id) } #[cfg(test)] @@ -615,6 +640,18 @@ pub(super) async fn run_guardian_review_session( schema: serde_json::Value, external_cancel: Option, ) -> (GuardianReviewOutcome, GuardianReviewAnalyticsResult) { + if FORCE_GUARDIAN_DENY_FOR_LOCAL_TESTING { + return ( + GuardianReviewOutcome::Completed(GuardianAssessment { + risk_level: GuardianRiskLevel::Low, + user_authorization: GuardianUserAuthorization::Unknown, + outcome: GuardianAssessmentOutcome::Deny, + rationale: FORCED_GUARDIAN_DENIAL_RATIONALE.to_string(), + }), + GuardianReviewAnalyticsResult::without_session(), + ); + } + let live_network_config = match session.services.network_proxy.as_ref() { Some(network_proxy) => match network_proxy.proxy().current_cfg().await { Ok(config) => Some(config), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index c93ab296b1..ee428534bd 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -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; @@ -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( @@ -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( @@ -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, @@ -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, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 0c5af3fc5e..ef086f4e16 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -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; @@ -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( @@ -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(); @@ -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; @@ -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( diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 03849f7e07..bd9fb30223 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -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), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index e7484e43b7..15539cb20c 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -3486,7 +3486,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { tool_approvals: Mutex::new(ApprovalStore::default()), guardian_rejections: Mutex::new(std::collections::HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), - runtime_handle: tokio::runtime::Handle::current(), skills_manager, plugins_manager, mcp_manager, @@ -4912,7 +4911,6 @@ where tool_approvals: Mutex::new(ApprovalStore::default()), guardian_rejections: Mutex::new(std::collections::HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), - runtime_handle: tokio::runtime::Handle::current(), skills_manager, plugins_manager, mcp_manager, @@ -6315,7 +6313,7 @@ impl SessionTask for GuardianDeniedApprovalTask { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn guardian_auto_review_interrupts_after_three_consecutive_denials() { +async fn guardian_auto_review_warns_after_three_consecutive_denials() { let (sess, tc, rx) = make_session_and_context_with_rx().await; let input = vec![UserInput::Text { text: "trigger guardian denials".to_string(), @@ -6325,12 +6323,12 @@ async fn guardian_auto_review_interrupts_after_three_consecutive_denials() { .await; let mut observed = Vec::new(); - let aborted = tokio::time::timeout(std::time::Duration::from_secs(5), async { + let warning = tokio::time::timeout(std::time::Duration::from_secs(5), async { loop { let event = rx.recv().await.expect("event"); - if let EventMsg::TurnAborted(event) = &event.msg { + if let EventMsg::GuardianWarning(event) = &event.msg { let event = event.clone(); - observed.push(EventMsg::TurnAborted(event.clone())); + observed.push(EventMsg::GuardianWarning(event.clone())); break event; } observed.push(event.msg); @@ -6339,14 +6337,18 @@ async fn guardian_auto_review_interrupts_after_three_consecutive_denials() { .await .unwrap_or_else(|_| { panic!( - "guardian denial circuit breaker should interrupt the turn; observed events: {observed:?}" + "guardian denial circuit breaker should warn about escalation; observed events: {observed:?}" ) }); - assert_eq!(aborted.reason, TurnAbortReason::Interrupted); + assert!( + warning + .message + .contains("requesting manual approval for the current request") + ); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn guardian_helper_review_interrupts_after_three_consecutive_denials() { +async fn guardian_helper_review_warns_after_three_consecutive_denials() { let (sess, tc, rx) = make_session_and_context_with_rx().await; let input = vec![UserInput::Text { text: "keep turn active for helper reviews".to_string(), @@ -6384,12 +6386,12 @@ async fn guardian_helper_review_interrupts_after_three_consecutive_denials() { review_thread.join().expect("helper review thread"); let mut observed = Vec::new(); - let aborted = timeout(StdDuration::from_secs(5), async { + let warning = timeout(StdDuration::from_secs(5), async { loop { let event = rx.recv().await.expect("event"); - if let EventMsg::TurnAborted(event) = &event.msg { + if let EventMsg::GuardianWarning(event) = &event.msg { let event = event.clone(); - observed.push(EventMsg::TurnAborted(event.clone())); + observed.push(EventMsg::GuardianWarning(event.clone())); break event; } observed.push(event.msg); @@ -6398,10 +6400,14 @@ async fn guardian_helper_review_interrupts_after_three_consecutive_denials() { .await .unwrap_or_else(|_| { panic!( - "helper review circuit breaker should interrupt the turn; observed events: {observed:?}" + "helper review circuit breaker should warn about escalation; observed events: {observed:?}" ) }); - assert_eq!(aborted.reason, TurnAbortReason::Interrupted); + assert!( + warning + .message + .contains("escalating the current request to the user") + ); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index fe27d89ae0..798be8bd63 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -27,7 +27,6 @@ use codex_rollout_trace::ThreadTraceContext; use codex_thread_store::LiveThread; use codex_thread_store::ThreadStore; use std::path::PathBuf; -use tokio::runtime::Handle; use tokio::sync::Mutex; use tokio::sync::RwLock; use tokio::sync::watch; @@ -54,7 +53,6 @@ pub(crate) struct SessionServices { pub(crate) tool_approvals: Mutex, pub(crate) guardian_rejections: Mutex>, pub(crate) guardian_rejection_circuit_breaker: Mutex, - pub(crate) runtime_handle: Handle, pub(crate) skills_manager: Arc, pub(crate) plugins_manager: Arc, pub(crate) mcp_manager: Arc, diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index 91078c50ce..4c4df15317 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -508,51 +508,6 @@ impl Session { } } - pub(crate) async fn abort_turn_if_active( - self: &Arc, - turn_id: &str, - reason: TurnAbortReason, - ) -> bool { - let active_turn = { - let mut active = self.active_turn.lock().await; - if active - .as_ref() - .is_some_and(|active_turn| active_turn.tasks.contains_key(turn_id)) - { - active.take() - } else { - None - } - }; - let Some(mut active_turn) = active_turn else { - return false; - }; - - let tasks = active_turn.drain_tasks(); - let turn_context = tasks.first().map(|task| Arc::clone(&task.turn_context)); - for task in tasks { - self.handle_task_abort(task, reason.clone()).await; - } - if let Err(err) = self - .goal_runtime_apply(GoalRuntimeEvent::TaskAborted { - turn_context: turn_context.as_deref(), - reason: reason.clone(), - }) - .await - { - warn!("failed to apply goal runtime abort event: {err}"); - } - // Let interrupted tasks observe cancellation before dropping pending approvals, or an - // in-flight approval wait can surface as a model-visible rejection before TurnAborted. - active_turn.clear_pending().await; - - if reason == TurnAbortReason::Interrupted { - self.maybe_start_turn_for_pending_work().await; - } - - true - } - pub async fn on_task_finished( self: &Arc, turn_context: Arc, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 14af2c9c5f..ab7a2bbf4d 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -3,8 +3,10 @@ use crate::guardian::GuardianNetworkAccessTrigger; 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::network_policy_decision::denied_network_policy_message; use crate::session::session::Session; @@ -498,8 +500,9 @@ impl NetworkApprovalService { } let use_guardian = routes_approval_to_guardian(&turn_context); let guardian_review_id = use_guardian.then(new_guardian_review_id); + let mut escalated_from_guardian = false; let approval_decision = if let Some(review_id) = guardian_review_id.clone() { - review_approval_request( + let decision = review_approval_request( &session, &turn_context, review_id, @@ -516,7 +519,28 @@ impl NetworkApprovalService { }, Some(policy_denial_message.clone()), ) - .await + .await; + escalated_from_guardian = matches!(decision, ReviewDecision::Denied) + && take_pending_auto_review_escalation(&session, &turn_context.sub_id).await; + if escalated_from_guardian { + let available_decisions = None; + session + .request_command_approval( + turn_context.as_ref(), + guardian_approval_id, + /*approval_id*/ None, + prompt_command, + turn_context.cwd.clone(), + Some(prompt_reason), + Some(network_approval_context.clone()), + /*proposed_execpolicy_amendment*/ None, + /*additional_permissions*/ None, + available_decisions, + ) + .await + } else { + decision + } } else { let available_decisions = None; session @@ -534,6 +558,9 @@ impl NetworkApprovalService { ) .await }; + if escalated_from_guardian { + reset_auto_review_rejection_circuit_breaker(&session, &turn_context.sub_id).await; + } let mut cache_session_deny = false; let resolved = match approval_decision { diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 7f218a629a..e83b78ff5a 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -5,7 +5,9 @@ //! sandboxing enforced by the explicit filesystem sandbox context. use crate::exec::is_likely_sandbox_denied; use crate::guardian::GuardianApprovalRequest; +use crate::guardian::reset_auto_review_rejection_circuit_breaker; use crate::guardian::review_approval_request; +use crate::guardian::take_pending_auto_review_escalation; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; @@ -130,16 +132,24 @@ impl Approvable for ApplyPatchRuntime { let turn = ctx.turn; let call_id = ctx.call_id.to_string(); let retry_reason = ctx.retry_reason.clone(); - let approval_keys = self.approval_keys(req); + let mut approval_keys = self.approval_keys(req); let changes = req.changes.clone(); let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { + let mut escalated_from_guardian = false; if let Some(review_id) = guardian_review_id { let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); - return review_approval_request(session, turn, review_id, action, retry_reason) - .await; + let decision = + review_approval_request(session, turn, review_id, action, retry_reason.clone()) + .await; + escalated_from_guardian = matches!(decision, ReviewDecision::Denied) + && take_pending_auto_review_escalation(session, &turn.sub_id).await; + if !escalated_from_guardian { + return decision; + } + approval_keys.clear(); } - if req.permissions_preapproved && retry_reason.is_none() { + if req.permissions_preapproved && retry_reason.is_none() && !escalated_from_guardian { return ReviewDecision::Approved; } if let Some(reason) = retry_reason { @@ -152,10 +162,14 @@ impl Approvable for ApplyPatchRuntime { /*grant_root*/ None, ) .await; - return rx_approve.await.unwrap_or_default(); + let decision = rx_approve.await.unwrap_or_default(); + if escalated_from_guardian { + reset_auto_review_rejection_circuit_breaker(session, &turn.sub_id).await; + } + return decision; } - with_cached_approval( + let decision = with_cached_approval( &session.services, "apply_patch", approval_keys, @@ -168,7 +182,11 @@ impl Approvable for ApplyPatchRuntime { rx_approve.await.unwrap_or_default() }, ) - .await + .await; + if escalated_from_guardian { + reset_auto_review_rejection_circuit_breaker(session, &turn.sub_id).await; + } + decision }) } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 7f17285db9..afcd956407 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -12,7 +12,9 @@ use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; +use crate::guardian::reset_auto_review_rejection_circuit_breaker; use crate::guardian::review_approval_request; +use crate::guardian::take_pending_auto_review_escalation; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; @@ -148,7 +150,7 @@ impl Approvable for ShellRuntime { req: &'a ShellRequest, ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { - let keys = self.approval_keys(req); + let mut keys = self.approval_keys(req); let command = req.command.clone(); let cwd = req.cwd.clone(); let retry_reason = ctx.retry_reason.clone(); @@ -158,43 +160,55 @@ impl Approvable for ShellRuntime { let call_id = ctx.call_id.to_string(); let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { + let mut escalated_from_guardian = false; if let Some(review_id) = guardian_review_id { - return review_approval_request( + let decision = review_approval_request( session, turn, review_id, GuardianApprovalRequest::Shell { - id: call_id, - command, + id: call_id.clone(), + command: command.clone(), cwd: cwd.clone(), sandbox_permissions: req.sandbox_permissions, additional_permissions: req.additional_permissions.clone(), justification: req.justification.clone(), }, - retry_reason, + retry_reason.clone(), ) .await; + escalated_from_guardian = matches!(decision, ReviewDecision::Denied) + && take_pending_auto_review_escalation(session, &turn.sub_id).await; + if !escalated_from_guardian { + return decision; + } + keys.clear(); } - with_cached_approval(&session.services, "shell", keys, move || async move { - let available_decisions = None; - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, - command, - cwd, - reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement - .proposed_execpolicy_amendment() - .cloned(), - req.additional_permissions.clone(), - available_decisions, - ) - .await - }) - .await + let decision = + with_cached_approval(&session.services, "shell", keys, move || async move { + let available_decisions = None; + session + .request_command_approval( + turn, + call_id, + /*approval_id*/ None, + command, + cwd, + reason, + ctx.network_approval_context.clone(), + req.exec_approval_requirement + .proposed_execpolicy_amendment() + .cloned(), + req.additional_permissions.clone(), + available_decisions, + ) + .await + }) + .await; + if escalated_from_guardian { + reset_auto_review_rejection_circuit_breaker(session, &turn.sub_id).await; + } + decision }) } diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 41aa552d89..546abb742d 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -7,8 +7,10 @@ use crate::guardian::GuardianApprovalRequest; 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::sandboxing::ExecOptions; use crate::sandboxing::ExecRequest; @@ -449,16 +451,20 @@ impl CoreShellActionProvider { program: program.to_string_lossy().into_owned(), argv: argv.to_vec(), cwd: workdir.clone(), - additional_permissions, + additional_permissions: additional_permissions.clone(), }, /*retry_reason*/ None, ) .await; - return PromptDecision { - decision, - guardian_review_id, - rejection_message: None, - }; + let escalated_from_guardian = matches!(decision, ReviewDecision::Denied) + && take_pending_auto_review_escalation(&session, &turn.sub_id).await; + if !escalated_from_guardian { + return PromptDecision { + decision, + guardian_review_id, + rejection_message: None, + }; + } } // 3) Fall back to regular user prompt @@ -476,6 +482,9 @@ impl CoreShellActionProvider { Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), ) .await; + if guardian_review_id.is_some() { + reset_auto_review_rejection_circuit_breaker(&session, &turn.sub_id).await; + } PromptDecision { decision, guardian_review_id: None, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index dbdd6efb51..d6f02f1a22 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -9,7 +9,9 @@ use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; +use crate::guardian::reset_auto_review_rejection_circuit_breaker; use crate::guardian::review_approval_request; +use crate::guardian::take_pending_auto_review_escalation; use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecServerEnvConfig; use crate::sandboxing::SandboxPermissions; @@ -140,7 +142,7 @@ impl Approvable for UnifiedExecRuntime<'_> { req: &'b UnifiedExecRequest, ctx: ApprovalCtx<'b>, ) -> BoxFuture<'b, ReviewDecision> { - let keys = self.approval_keys(req); + let mut keys = self.approval_keys(req); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); @@ -150,44 +152,56 @@ impl Approvable for UnifiedExecRuntime<'_> { let reason = retry_reason.clone().or_else(|| req.justification.clone()); let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { + let mut escalated_from_guardian = false; if let Some(review_id) = guardian_review_id { - return review_approval_request( + let decision = review_approval_request( session, turn, review_id, GuardianApprovalRequest::ExecCommand { - id: call_id, - command, + id: call_id.clone(), + command: command.clone(), cwd: cwd.clone(), sandbox_permissions: req.sandbox_permissions, additional_permissions: req.additional_permissions.clone(), justification: req.justification.clone(), tty: req.tty, }, - retry_reason, + retry_reason.clone(), ) .await; + escalated_from_guardian = matches!(decision, ReviewDecision::Denied) + && take_pending_auto_review_escalation(session, &turn.sub_id).await; + if !escalated_from_guardian { + return decision; + } + keys.clear(); } - with_cached_approval(&session.services, "unified_exec", keys, || async move { - let available_decisions = None; - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, - command, - cwd.clone(), - reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement - .proposed_execpolicy_amendment() - .cloned(), - req.additional_permissions.clone(), - available_decisions, - ) - .await - }) - .await + let decision = + with_cached_approval(&session.services, "unified_exec", keys, || async move { + let available_decisions = None; + session + .request_command_approval( + turn, + call_id, + /*approval_id*/ None, + command, + cwd.clone(), + reason, + ctx.network_approval_context.clone(), + req.exec_approval_requirement + .proposed_execpolicy_amendment() + .cloned(), + req.additional_permissions.clone(), + available_decisions, + ) + .await + }) + .await; + if escalated_from_guardian { + reset_auto_review_rejection_circuit_breaker(session, &turn.sub_id).await; + } + decision }) }