From f7cd9e84973d1a77964aa07da19981a154677b05 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Thu, 11 Jun 2026 22:48:27 -0700 Subject: [PATCH] core: dispatch delegated approval reviews through extensions --- codex-rs/core/src/codex_delegate.rs | 103 ++++++++++-------- codex-rs/core/src/codex_delegate_tests.rs | 68 ++++++++++++ .../tools/runtimes/shell/unix_escalation.rs | 22 +++- 3 files changed, 142 insertions(+), 51 deletions(-) diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 6ee0328463..e09a820f06 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -3,8 +3,8 @@ use std::sync::Arc; use async_channel::Receiver; use async_channel::Sender; -use codex_analytics::GuardianApprovalRequestSource; use codex_async_utils::OrCancelExt; +use codex_extension_api::ApprovalReviewSource; use codex_extension_api::LoadedUserInstructions; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::Event; @@ -36,7 +36,6 @@ use crate::guardian::GuardianApprovalRequest; use crate::guardian::new_guardian_review_id; use crate::guardian::routes_approval_to_guardian; use crate::guardian::routes_approval_to_guardian_with_reviewer; -use crate::guardian::spawn_approval_request_review; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; @@ -51,6 +50,7 @@ use crate::session::SUBMISSION_CHANNEL_CAPACITY; use crate::session::emit_subagent_session_started; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::tools::approval_dispatch::request_automated_approval_with_cancel; use codex_login::AuthManager; use codex_models_manager::manager::SharedModelsManager; use codex_protocol::error::CodexErr; @@ -445,6 +445,39 @@ async fn forward_ops( } /// Handle an ExecApprovalRequest by consulting the parent session and replying. +async fn request_delegated_automated_approval( + parent_session: &Arc, + parent_ctx: &Arc, + approval_id: &str, + request: GuardianApprovalRequest, + retry_reason: Option, + reviewer: codex_protocol::config_types::ApprovalsReviewer, + cancel_token: &CancellationToken, +) -> ReviewDecision { + let review_cancel = cancel_token.child_token(); + match request_automated_approval_with_cancel( + parent_session, + parent_ctx, + new_guardian_review_id(), + request, + reviewer, + retry_reason, + ApprovalReviewSource::DelegatedSubagent, + review_cancel, + ) + .await + { + Some(Ok(automated)) => automated.decision, + Some(Err(_)) => ReviewDecision::Denied, + None => { + parent_session + .notify_approval(approval_id, ReviewDecision::Abort) + .await; + ReviewDecision::Abort + } + } +} + async fn handle_exec_approval( codex: &Codex, turn_id: String, @@ -467,11 +500,10 @@ async fn handle_exec_approval( .. } = event; let decision = if routes_approval_to_guardian(parent_ctx) { - let review_cancel = cancel_token.child_token(); - let review_rx = spawn_approval_request_review( - Arc::clone(parent_session), - Arc::clone(parent_ctx), - new_guardian_review_id(), + request_delegated_automated_approval( + parent_session, + parent_ctx, + &approval_id_for_op, GuardianApprovalRequest::Shell { id: call_id.clone(), command, @@ -485,15 +517,8 @@ async fn handle_exec_approval( justification: None, }, reason, - GuardianApprovalRequestSource::DelegatedSubagent, - review_cancel.clone(), - ); - await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, - parent_session, - &approval_id_for_op, + parent_ctx.config.approvals_reviewer, cancel_token, - Some(&review_cancel), ) .await } else { @@ -552,7 +577,6 @@ async fn handle_patch_approval( parent_ctx.cwd.join(path) }) .collect::>(); - let review_cancel = cancel_token.child_token(); let patch = changes .iter() .map(|(path, change)| match change { @@ -580,28 +604,21 @@ async fn handle_patch_approval( }) .collect::>() .join("\n"); - let review_rx = spawn_approval_request_review( - Arc::clone(parent_session), - Arc::clone(parent_ctx), - new_guardian_review_id(), - GuardianApprovalRequest::ApplyPatch { - id: approval_id.clone(), - #[allow(deprecated)] - cwd: parent_ctx.cwd.clone(), - files, - patch, - }, - reason.clone(), - GuardianApprovalRequestSource::DelegatedSubagent, - review_cancel.clone(), - ); Some( - await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + request_delegated_automated_approval( parent_session, + parent_ctx, &approval_id, + GuardianApprovalRequest::ApplyPatch { + id: approval_id.clone(), + #[allow(deprecated)] + cwd: parent_ctx.cwd.clone(), + files, + patch, + }, + reason.clone(), + parent_ctx.config.approvals_reviewer, cancel_token, - Some(&review_cancel), ) .await, ) @@ -706,22 +723,14 @@ async fn maybe_auto_review_mcp_request_user_input( if !routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) { return None; } - let review_cancel = cancel_token.child_token(); - let review_rx = spawn_approval_request_review( - Arc::clone(parent_session), - Arc::clone(parent_ctx), - new_guardian_review_id(), + let decision = request_delegated_automated_approval( + parent_session, + parent_ctx, + &event.call_id, build_guardian_mcp_tool_review_request(&event.call_id, &invocation, metadata.as_ref()), /*retry_reason*/ None, - GuardianApprovalRequestSource::DelegatedSubagent, - review_cancel.clone(), - ); - let decision = await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, - parent_session, - &event.call_id, + approvals_reviewer, cancel_token, - Some(&review_cancel), ) .await; let selected_label = match decision { diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index cb7767a16c..8d8d4074fc 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -2,6 +2,12 @@ use super::*; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX; use async_channel::bounded; +use codex_extension_api::ApprovalReviewContributor; +use codex_extension_api::ApprovalReviewError; +use codex_extension_api::ApprovalReviewInput; +use codex_extension_api::ApprovalReviewOutcome; +use codex_extension_api::ExtensionFuture; +use codex_extension_api::ExtensionRegistryBuilder; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::NetworkPermissions; @@ -33,6 +39,20 @@ use tokio::sync::Mutex; use tokio::sync::watch; use tokio::time::timeout; +struct ApprovingDelegatedReviewContributor { + source: Arc>>, +} + +impl ApprovalReviewContributor for ApprovingDelegatedReviewContributor { + fn review<'a>( + &'a self, + input: ApprovalReviewInput<'a>, + ) -> ExtensionFuture<'a, Result> { + *self.source.lock().expect("source lock") = Some(input.source); + Box::pin(async { Ok(ApprovalReviewOutcome::decision(ReviewDecision::Approved)) }) + } +} + #[tokio::test] async fn forward_events_cancelled_while_send_blocked_shuts_down_delegate() { let (tx_events, rx_events) = bounded(1); @@ -280,6 +300,54 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() { ); } +#[tokio::test] +async fn delegated_automated_approval_uses_extension_with_delegated_source() { + let (mut session, mut parent_ctx, _rx_events) = + crate::session::tests::make_session_and_context_with_rx().await; + let parent_ctx_mut = Arc::get_mut(&mut parent_ctx).expect("single turn context ref"); + parent_ctx_mut + .approval_policy + .set(AskForApproval::OnRequest) + .expect("set on-request policy"); + let mut config = (*parent_ctx_mut.config).clone(); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + parent_ctx_mut.config = Arc::new(config); + + let source = Arc::new(std::sync::Mutex::new(None)); + let mut extensions = ExtensionRegistryBuilder::::new(); + extensions.approval_review_contributor(Arc::new(ApprovingDelegatedReviewContributor { + source: Arc::clone(&source), + })); + Arc::get_mut(&mut session) + .expect("single session ref") + .services + .extensions = Arc::new(extensions.build()); + + let decision = request_delegated_automated_approval( + &session, + &parent_ctx, + "callback-approval-1", + GuardianApprovalRequest::Shell { + id: "command-item-1".to_string(), + command: vec!["echo".to_string(), "hello".to_string()], + cwd: test_path_buf("/tmp").abs(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + }, + /*retry_reason*/ None, + ApprovalsReviewer::AutoReview, + &CancellationToken::new(), + ) + .await; + + assert_eq!(decision, ReviewDecision::Approved); + assert_eq!( + *source.lock().expect("source lock"), + Some(ApprovalReviewSource::DelegatedSubagent) + ); +} + #[tokio::test] async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_for_reply() { let (parent_session, parent_ctx, rx_events) = 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 960e1d3943..65a655a79f 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -7,13 +7,13 @@ 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; use crate::shell::ShellType; +use crate::tools::approval_dispatch::request_automated_approval; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::exec_env_for_sandbox_permissions; use crate::tools::runtimes::prepend_zsh_fork_bin_to_path; @@ -29,6 +29,7 @@ use codex_execpolicy::Evaluation; use codex_execpolicy::MatchOptions; use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; +use codex_extension_api::ApprovalReviewSource; use codex_features::Feature; use codex_hooks::PermissionRequestDecision; use codex_protocol::config_types::WindowsSandboxLevel; @@ -460,10 +461,10 @@ impl CoreShellActionProvider { // 2) Route to Guardian if configured if let Some(review_id) = guardian_review_id.clone() { - let decision = review_approval_request( + let automated = request_automated_approval( &session, &turn, - review_id.clone(), + review_id, GuardianApprovalRequest::Execve { id: call_id.clone(), source, @@ -472,13 +473,26 @@ impl CoreShellActionProvider { cwd: workdir.clone(), additional_permissions, }, + turn.config.approvals_reviewer, /*retry_reason*/ None, + ApprovalReviewSource::MainTurn, ) .await; + let (decision, rejection_message) = match automated { + Ok(automated) => { + let rejection_message = matches!( + &automated.decision, + ReviewDecision::Denied | ReviewDecision::TimedOut + ) + .then(|| automated.denial_message()); + (automated.decision, rejection_message) + } + Err(message) => (ReviewDecision::Denied, Some(message)), + }; return PromptDecision { decision, guardian_review_id, - rejection_message: None, + rejection_message, }; }