mirror of
https://github.com/openai/codex.git
synced 2026-11-01 19:33:35 +00:00
core: dispatch delegated approval reviews through extensions
This commit is contained in:
@@ -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<Session>,
|
||||
parent_ctx: &Arc<TurnContext>,
|
||||
approval_id: &str,
|
||||
request: GuardianApprovalRequest,
|
||||
retry_reason: Option<String>,
|
||||
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::<Vec<_>>();
|
||||
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::<Vec<_>>()
|
||||
.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 {
|
||||
|
||||
@@ -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<std::sync::Mutex<Option<ApprovalReviewSource>>>,
|
||||
}
|
||||
|
||||
impl ApprovalReviewContributor for ApprovingDelegatedReviewContributor {
|
||||
fn review<'a>(
|
||||
&'a self,
|
||||
input: ApprovalReviewInput<'a>,
|
||||
) -> ExtensionFuture<'a, Result<ApprovalReviewOutcome, ApprovalReviewError>> {
|
||||
*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::<Config>::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) =
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user