From a0b20bd4611d67c51d687ad4431f77fdecfbf65a Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Tue, 7 Jul 2026 10:49:02 -0700 Subject: [PATCH] refactor(approvals): centralize permission reviews --- codex-rs/core/src/approval_coordinator.rs | 179 ++++++++++++++++------ codex-rs/core/src/session/mod.rs | 102 ++++++++---- 2 files changed, 205 insertions(+), 76 deletions(-) diff --git a/codex-rs/core/src/approval_coordinator.rs b/codex-rs/core/src/approval_coordinator.rs index d0b74555b8..1a939f5bdc 100644 --- a/codex-rs/core/src/approval_coordinator.rs +++ b/codex-rs/core/src/approval_coordinator.rs @@ -7,6 +7,7 @@ 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::spawn_approval_request_review; use crate::hook_runtime::run_permission_request_hooks; use crate::session::session::Session; use crate::session::turn_context::TurnContext; @@ -16,12 +17,14 @@ use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; +use codex_analytics::GuardianApprovalRequestSource; use codex_config::types::ApprovalsReviewer; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; +use tokio_util::sync::CancellationToken; pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest; @@ -29,6 +32,8 @@ pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest; pub(crate) struct ApprovalReview { pub(crate) action: ApprovalAction, pub(crate) retry_reason: Option, + request_source: GuardianApprovalRequestSource, + cancellation: Option, } impl ApprovalReview { @@ -36,6 +41,34 @@ impl ApprovalReview { Self { action, retry_reason, + request_source: GuardianApprovalRequestSource::MainTurn, + cancellation: None, + } + } + + pub(crate) fn main_turn_cancellable( + action: ApprovalAction, + retry_reason: Option, + cancellation: CancellationToken, + ) -> Self { + Self { + action, + retry_reason, + request_source: GuardianApprovalRequestSource::MainTurn, + cancellation: Some(cancellation), + } + } + + pub(crate) fn delegated( + action: ApprovalAction, + retry_reason: Option, + cancellation: CancellationToken, + ) -> Self { + Self { + action, + retry_reason, + request_source: GuardianApprovalRequestSource::DelegatedSubagent, + cancellation: Some(cancellation), } } } @@ -218,11 +251,11 @@ impl ApprovalCoordinator { hook_request, review, || async { - ApprovalUserReview { - decision: user_review().await, - output: (), - } - }, + ApprovalUserReview { + decision: user_review().await, + output: (), + } + }, ) .await .resolution @@ -236,14 +269,96 @@ impl ApprovalCoordinator { review: ApprovalReview, ) -> ApprovalResolution { debug_assert_eq!(reviewer, ApprovalReviewer::Guardian); - Self::resolve_event_with_user_output(session, turn, reviewer, hook_request, review, || async { - ApprovalUserReview { + Self::try_resolve_automatic(session, turn, reviewer, hook_request, review) + .await + .unwrap_or(ApprovalResolution { decision: ReviewDecision::Denied, - output: (), + rejection: Some("automatic approval reviewer routed to the user".to_string()), + source: ApprovalResolutionSource::User, + }) + } + + pub(crate) async fn try_resolve_automatic( + session: &Arc, + turn: &Arc, + reviewer: ApprovalReviewer, + hook_request: Option>, + review: ApprovalReview, + ) -> Option { + if let Some(hook_request) = hook_request + && let Some(resolution) = + Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await + { + return Some(resolution); + } + + match reviewer { + ApprovalReviewer::Guardian => { + debug_assert!(review.cancellation.is_none()); + debug_assert!(matches!( + review.request_source, + GuardianApprovalRequestSource::MainTurn + )); + let review_id = new_guardian_review_id(); + let decision = review_approval_request( + session, + turn, + review_id.clone(), + review.action, + review.retry_reason, + ) + .await; + Some(Self::normalize_guardian(session, review_id, decision).await) } - }) - .await - .resolution + ApprovalReviewer::User => None, + } + } + + pub(crate) async fn try_resolve_automatic_cancellable( + session: &Arc, + turn: &Arc, + reviewer: ApprovalReviewer, + hook_request: Option>, + review: ApprovalReview, + ) -> Option { + if let Some(hook_request) = hook_request + && let Some(resolution) = + Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await + { + return Some(resolution); + } + + match reviewer { + ApprovalReviewer::Guardian => { + let Some(cancellation) = review.cancellation else { + tracing::error!( + "cancellable approval review is missing its cancellation token" + ); + return Some(ApprovalResolution { + decision: ReviewDecision::Abort, + rejection: Some( + "automatic approval review is missing its cancellation token" + .to_string(), + ), + source: ApprovalResolutionSource::Guardian, + }); + }; + let review_id = new_guardian_review_id(); + let decision = spawn_approval_request_review( + Arc::clone(session), + Arc::clone(turn), + review_id.clone(), + review.action, + review.retry_reason, + review.request_source, + cancellation, + ) + .await + .unwrap_or(ReviewDecision::Denied); + Some(Self::normalize_guardian(session, review_id, decision).await) + } + ApprovalReviewer::User => None, + } } pub(crate) async fn resolve_event_with_user_output( @@ -258,9 +373,8 @@ impl ApprovalCoordinator { F: FnOnce() -> Fut, Fut: Future>, { - if let Some(hook_request) = hook_request - && let Some(resolution) = - Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await + if let Some(resolution) = + Self::try_resolve_automatic(session, turn, reviewer, hook_request, review).await { return ApprovalEventResolution { resolution, @@ -268,37 +382,14 @@ impl ApprovalCoordinator { }; } - let (resolution, user_output) = match reviewer { - ApprovalReviewer::Guardian => { - let review_id = new_guardian_review_id(); - let decision = review_approval_request( - session, - turn, - review_id.clone(), - review.action.clone(), - review.retry_reason.clone(), - ) - .await; - ( - Self::normalize_guardian(session, review_id, decision).await, - None, - ) - } - ApprovalReviewer::User => { - let user_review = user_review().await; - ( - ApprovalResolution { - decision: user_review.decision, - rejection: None, - source: ApprovalResolutionSource::User, - }, - Some(user_review.output), - ) - } - }; + let user_review = user_review().await; ApprovalEventResolution { - resolution: Self::normalize_user_rejection(resolution), - user_output, + resolution: Self::normalize_user_rejection(ApprovalResolution { + decision: user_review.decision, + rejection: None, + source: ApprovalResolutionSource::User, + }), + user_output: Some(user_review.output), } } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index ba8eacb395..22dd07c9af 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -16,6 +16,10 @@ use crate::agent::agent_status_from_event; use crate::agent::status::is_final; use crate::agent_communication::AgentCommunicationContext; use crate::agent_communication::AgentCommunicationKind; +use crate::approval_coordinator::ApprovalAction; +use crate::approval_coordinator::ApprovalCoordinator; +use crate::approval_coordinator::ApprovalReview; +use crate::approval_coordinator::ApprovalReviewer; use crate::attestation::AttestationProvider; use crate::build_available_skills; use crate::compact; @@ -977,6 +981,12 @@ fn push_prompt_fragment( } } +#[derive(Clone, Copy, Debug)] +enum RequestPermissionsReviewOrigin { + MainTurn, + DelegatedSubagent, +} + impl Session { pub(crate) async fn app_server_client_metadata(&self) -> AppServerClientMetadata { let state = self.state.lock().await; @@ -2228,10 +2238,6 @@ impl Session { rx_approve } - #[expect( - clippy::await_holding_invalid_type, - reason = "active turn checks and turn state updates must remain atomic" - )] pub(crate) async fn request_permissions_for_environment( self: &Arc, turn_context: &Arc, @@ -2239,6 +2245,30 @@ impl Session { args: RequestPermissionsArgs, environment: TurnEnvironmentSelection, cancellation_token: CancellationToken, + ) -> Option { + self.request_permissions_for_environment_with_origin( + turn_context, + call_id, + args, + environment, + cancellation_token, + RequestPermissionsReviewOrigin::MainTurn, + ) + .await + } + + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] + async fn request_permissions_for_environment_with_origin( + self: &Arc, + turn_context: &Arc, + call_id: String, + args: RequestPermissionsArgs, + environment: TurnEnvironmentSelection, + cancellation_token: CancellationToken, + review_origin: RequestPermissionsReviewOrigin, ) -> Option { match turn_context.as_ref().approval_policy.value() { AskForApproval::Never => { @@ -2276,35 +2306,42 @@ impl Session { }); }; - if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { - let originating_turn_state = { - let active = self.active_turn.lock().await; - active.as_ref().map(|active| Arc::clone(&active.turn_state)) - }; - let review_id = crate::guardian::new_guardian_review_id(); - let session = Arc::clone(self); - let turn = Arc::clone(turn_context); - let request = crate::guardian::GuardianApprovalRequest::RequestPermissions { - id: call_id, - turn_id: turn_context.sub_id.clone(), - reason: args.reason, - permissions: requested_permissions.clone(), - }; - let review_rx = crate::guardian::spawn_approval_request_review( - session, - turn, - review_id, - request, + let originating_turn_state = { + let active = self.active_turn.lock().await; + active.as_ref().map(|active| Arc::clone(&active.turn_state)) + }; + let action = ApprovalAction::RequestPermissions { + id: call_id.clone(), + turn_id: turn_context.sub_id.clone(), + reason: args.reason.clone(), + permissions: requested_permissions.clone(), + }; + let review = match review_origin { + RequestPermissionsReviewOrigin::MainTurn => ApprovalReview::main_turn_cancellable( + action, /*retry_reason*/ None, - codex_analytics::GuardianApprovalRequestSource::MainTurn, cancellation_token.clone(), - ); - let decision = tokio::select! { - biased; - _ = cancellation_token.cancelled() => return None, - decision = review_rx => decision.unwrap_or(ReviewDecision::Denied), - }; - let response = match decision { + ), + RequestPermissionsReviewOrigin::DelegatedSubagent => ApprovalReview::delegated( + action, + /*retry_reason*/ None, + cancellation_token.clone(), + ), + }; + let automatic_review = ApprovalCoordinator::try_resolve_automatic_cancellable( + self, + turn_context, + ApprovalReviewer::for_turn(turn_context), + /*hook_request*/ None, + review, + ); + let automatic_resolution = tokio::select! { + biased; + _ = cancellation_token.cancelled() => return None, + resolution = automatic_review => resolution, + }; + if let Some(resolution) = automatic_resolution { + let response = match resolution.decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { RequestPermissionsResponse { permissions: requested_permissions.clone(), @@ -2424,12 +2461,13 @@ impl Session { }; let mut environment = turn_environment.selection(); environment.cwd = PathUri::from_abs_path(&cwd); - self.request_permissions_for_environment( + self.request_permissions_for_environment_with_origin( turn_context, call_id, args, environment, cancellation_token, + RequestPermissionsReviewOrigin::DelegatedSubagent, ) .await }