diff --git a/codex-rs/core/src/approval_coordinator.rs b/codex-rs/core/src/approval_coordinator.rs index dfef0b9738..6b0902fe18 100644 --- a/codex-rs/core/src/approval_coordinator.rs +++ b/codex-rs/core/src/approval_coordinator.rs @@ -104,6 +104,10 @@ impl ApprovalReviewer { Self::routes_to_guardian(turn, reviewer).then_some(Self::Guardian) } + pub(crate) fn automatic_for_turn(turn: &TurnContext) -> Option { + Self::automatic_for_reviewer(turn, turn.config.approvals_reviewer) + } + fn routes_to_guardian(turn: &TurnContext, reviewer: ApprovalsReviewer) -> bool { matches!( turn.approval_policy.value(), diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index b1923afdd9..54fb64ea56 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -38,6 +38,7 @@ pub(crate) use review::record_guardian_denial_for_test; pub(crate) use review::review_approval_request; #[cfg(test)] pub(crate) use review::review_approval_request_with_cancel; +#[cfg(test)] pub(crate) use review::routes_approval_to_guardian; #[cfg(test)] pub(crate) use review::routes_approval_to_guardian_with_reviewer; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 4756cc1fc4..c3198cb4b2 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -5,7 +5,9 @@ use codex_analytics::GuardianReviewFailureReason; use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewTrackContext; use codex_analytics::GuardianReviewedAction; +#[cfg(test)] use codex_protocol::config_types::ApprovalsReviewer; +#[cfg(test)] use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::EventMsg; @@ -165,11 +167,13 @@ fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str { /// Whether this turn should route allowed approval prompts through the guardian /// reviewer instead of surfacing them to the user. ARC may still block actions /// earlier in the flow. +#[cfg(test)] pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool { routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) } /// Whether an approval with its own reviewer selection should be routed through guardian. +#[cfg(test)] pub(crate) fn routes_approval_to_guardian_with_reviewer( turn: &TurnContext, approvals_reviewer: ApprovalsReviewer, diff --git a/codex-rs/core/src/session_startup_prewarm.rs b/codex-rs/core/src/session_startup_prewarm.rs index a5a4c5e437..b0333fc90f 100644 --- a/codex-rs/core/src/session_startup_prewarm.rs +++ b/codex-rs/core/src/session_startup_prewarm.rs @@ -9,8 +9,8 @@ use tracing::info; use tracing::instrument; use tracing::warn; +use crate::approval_coordinator::ApprovalReviewer; use crate::client::ModelClientSession; -use crate::guardian::routes_approval_to_guardian; use crate::responses_metadata::CodexResponsesRequestKind; use crate::session::INITIAL_SUBMIT_ID; use crate::session::session::Session; @@ -251,7 +251,7 @@ async fn schedule_startup_prewarm_inner( prewarm_started_at.elapsed(), /*status*/ None, ); - if routes_approval_to_guardian(&startup_turn_context) { + if ApprovalReviewer::automatic_for_turn(&startup_turn_context).is_some() { let guardian_session = Arc::clone(&session); let guardian_parent_turn = Arc::clone(&startup_turn_context); drop(tokio::spawn(async move { 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 10cb460205..106fdc8bf1 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -1,15 +1,16 @@ use super::ShellRequest; +use crate::approval_coordinator::ApprovalAction; +use crate::approval_coordinator::ApprovalCoordinator; +use crate::approval_coordinator::ApprovalHookRequest; +use crate::approval_coordinator::ApprovalResolution; +use crate::approval_coordinator::ApprovalResolutionSource; +use crate::approval_coordinator::ApprovalReview; +use crate::approval_coordinator::ApprovalReviewer; use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::exec::cancel_when_either; use crate::exec::is_likely_sandbox_denied; -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::review_approval_request; -use crate::guardian::routes_approval_to_guardian; -use crate::hook_runtime::run_permission_request_hooks; use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecRequest; use crate::sandboxing::SandboxPermissions; @@ -30,7 +31,6 @@ use codex_execpolicy::MatchOptions; use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; use codex_features::Feature; -use codex_hooks::PermissionRequestDecision; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; @@ -374,12 +374,6 @@ enum DecisionSource { UnmatchedCommandFallback, } -struct PromptDecision { - decision: ReviewDecision, - guardian_review_id: Option, - rejection_message: Option, -} - fn execve_prompt_is_rejected_by_policy( approval_policy: AskForApproval, decision_source: &DecisionSource, @@ -446,7 +440,7 @@ impl CoreShellActionProvider { workdir: &AbsolutePathBuf, stopwatch: &Stopwatch, additional_permissions: Option, - ) -> anyhow::Result { + ) -> anyhow::Result { let command = join_program_and_argv(program, argv); let workdir = workdir.clone(); let session = self.session.clone(); @@ -455,85 +449,52 @@ impl CoreShellActionProvider { let approval_id = Some(Uuid::new_v4().to_string()); let environment_id = Some(self.environment_id.clone()); let source = self.tool_name; - let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id); Ok(stopwatch .pause_for(async move { - // 1) Run PermissionRequest hooks let permission_request = PermissionRequestPayload::bash( codex_shell_command::parse_command::shlex_join(&command), /*description*/ None, ); let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); - match run_permission_request_hooks( + ApprovalCoordinator::resolve_event_cancellable( &session, &turn, - &effective_approval_id, - permission_request, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return PromptDecision { - decision: ReviewDecision::Approved, - guardian_review_id: None, - rejection_message: None, - }; - } - Some(PermissionRequestDecision::Deny { message }) => { - return PromptDecision { - decision: ReviewDecision::Denied, - guardian_review_id: None, - rejection_message: Some(message), - }; - } - None => {} - } - - // 2) Route to Guardian if configured - if let Some(review_id) = guardian_review_id.clone() { - let decision = review_approval_request( - &session, - &turn, - review_id.clone(), - GuardianApprovalRequest::Execve { + ApprovalReviewer::for_turn(&turn), + Some(ApprovalHookRequest { + run_id: &effective_approval_id, + payload: permission_request, + }), + ApprovalReview::main_turn_cancellable( + ApprovalAction::Execve { id: call_id.clone(), source, 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, - }; - } - - // 3) Fall back to regular user prompt - let decision = session - .request_command_approval( - &turn, - call_id, - approval_id, - environment_id, - command, - workdir.clone(), - /*reason*/ None, - /*network_approval_context*/ None, - /*proposed_execpolicy_amendment*/ None, - additional_permissions, - Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), - ) - .await; - PromptDecision { - decision, - guardian_review_id: None, - rejection_message: None, - } + CancellationToken::new(), + ), + || async { + session + .request_command_approval( + &turn, + call_id, + approval_id, + environment_id, + command, + workdir, + /*reason*/ None, + /*network_approval_context*/ None, + /*proposed_execpolicy_amendment*/ None, + additional_permissions, + Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), + ) + .await + }, + ) + .await }) .await) } @@ -588,22 +549,23 @@ impl CoreShellActionProvider { } }, ReviewDecision::Denied => { - let message = if let Some(message) = - prompt_decision.rejection_message.clone() - { - message - } else if let Some(review_id) = - prompt_decision.guardian_review_id.as_deref() - { - guardian_rejection_message(self.session.as_ref(), review_id).await - } else { - "User denied execution".to_string() - }; + let message = + if prompt_decision.source == ApprovalResolutionSource::User { + "User denied execution".to_string() + } else { + prompt_decision + .rejection + .clone() + .unwrap_or_else(|| "Execution denied by policy".to_string()) + }; EscalationDecision::deny(Some(message)) } - ReviewDecision::TimedOut => { - EscalationDecision::deny(Some(guardian_timeout_message())) - } + ReviewDecision::TimedOut => EscalationDecision::deny(Some( + prompt_decision + .rejection + .clone() + .unwrap_or_else(guardian_timeout_message), + )), ReviewDecision::Abort => { EscalationDecision::deny(Some("User cancelled execution".to_string())) }