diff --git a/codex-rs/core/src/tools/approval.rs b/codex-rs/core/src/tools/approval.rs index 9addd82577..64de12a303 100644 --- a/codex-rs/core/src/tools/approval.rs +++ b/codex-rs/core/src/tools/approval.rs @@ -13,10 +13,15 @@ use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::state::SessionServices; +use codex_protocol::approvals::ExecPolicyAmendment; +use codex_protocol::approvals::NetworkApprovalContext; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::ReviewDecision; use futures::Future; use serde::Serialize; use std::collections::HashMap; +use std::path::PathBuf; use std::sync::Arc; #[derive(Clone, Default, Debug)] @@ -43,6 +48,70 @@ impl ApprovalStore { } } +#[derive(Debug)] +pub(crate) enum ApprovalCache { + None, + SessionApproveOnly { + tool_name: &'static str, + keys: Vec, + }, +} + +#[derive(Debug)] +pub(crate) struct ApprovalOutcome { + pub decision: ReviewDecision, + pub guardian_review_id: Option, +} + +#[derive(Debug)] +pub(crate) struct CommandApprovalRequest { + pub call_id: String, + pub approval_id: Option, + pub command: Vec, + pub cwd: PathBuf, + pub reason: Option, + pub network_approval_context: Option, + pub proposed_execpolicy_amendment: Option, + pub additional_permissions: Option, + pub available_decisions: Option>, +} + +#[derive(Debug)] +pub(crate) struct PatchApprovalRequest { + pub call_id: String, + pub changes: HashMap, + pub reason: Option, + pub grant_root: Option, +} + +#[derive(Debug)] +pub(crate) enum UserApprovalRequest { + Command(CommandApprovalRequest), + Patch(PatchApprovalRequest), +} + +#[derive(Debug)] +pub(crate) struct GuardianApproval { + pub request: GuardianApprovalRequest, + pub retry_reason: Option, +} + +impl GuardianApproval { + pub(crate) fn new(request: GuardianApprovalRequest, retry_reason: Option) -> Self { + Self { + request, + retry_reason, + } + } +} + +#[derive(Debug)] +pub(crate) struct ApprovalPlan { + pub cache: ApprovalCache, + pub user: UserApprovalRequest, + pub guardian: GuardianApproval, +} + pub(crate) fn guardian_review_id_for_turn(turn: &crate::codex::TurnContext) -> Option { routes_approval_to_guardian(turn).then(new_guardian_review_id) } @@ -99,28 +168,94 @@ where decision } -pub(crate) async fn route_approval( +async fn dispatch_user_approval( + session: &Arc, + turn: &Arc, + request: UserApprovalRequest, +) -> ReviewDecision { + match request { + UserApprovalRequest::Command(request) => { + session + .request_command_approval( + turn.as_ref(), + request.call_id, + request.approval_id, + request.command, + request.cwd, + request.reason, + request.network_approval_context, + request.proposed_execpolicy_amendment, + request.additional_permissions, + request.available_decisions, + ) + .await + } + UserApprovalRequest::Patch(request) => { + let rx_approve = session + .request_patch_approval( + turn.as_ref(), + request.call_id, + request.changes, + request.reason, + request.grant_root, + ) + .await; + rx_approve.await.unwrap_or_default() + } + } +} + +pub(crate) async fn request_approval( session: &Arc, turn: &Arc, guardian_review_id: Option, - cache: Option<(&'static str, Vec)>, - guardian_request: GuardianApprovalRequest, - retry_reason: Option, - user: F, -) -> ReviewDecision + plan: ApprovalPlan, +) -> ApprovalOutcome where K: Serialize, - F: FnOnce() -> Fut, - Fut: Future, { - if let Some(review_id) = guardian_review_id { - return review_approval_request(session, turn, review_id, guardian_request, retry_reason) - .await; + let ApprovalPlan { + cache, + user, + guardian, + } = plan; + if let Some(review_id) = guardian_review_id.clone() { + return ApprovalOutcome { + decision: review_approval_request( + session, + turn, + review_id, + guardian.request, + guardian.retry_reason, + ) + .await, + guardian_review_id, + }; } - if let Some((tool_name, keys)) = cache { - with_cached_approval(&session.services, tool_name, keys, user).await - } else { - user().await + let decision = match cache { + ApprovalCache::None => dispatch_user_approval(session, turn, user).await, + ApprovalCache::SessionApproveOnly { tool_name, keys } => { + with_cached_approval(&session.services, tool_name, keys, || { + dispatch_user_approval(session, turn, user) + }) + .await + } + }; + + ApprovalOutcome { + decision, + guardian_review_id: None, } } + +pub(crate) async fn request_approval_for_turn( + session: &Arc, + turn: &Arc, + plan: ApprovalPlan, +) -> ApprovalOutcome +where + K: Serialize, +{ + request_approval(session, turn, guardian_review_id_for_turn(turn), plan).await +} diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index aec1243deb..1df8c4c09f 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -3,8 +3,12 @@ use crate::guardian::GuardianApprovalRequest; use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::network_policy_decision::denied_network_policy_message; -use crate::tools::approval::guardian_review_id_for_turn; -use crate::tools::approval::route_approval; +use crate::tools::approval::ApprovalCache; +use crate::tools::approval::ApprovalPlan; +use crate::tools::approval::CommandApprovalRequest; +use crate::tools::approval::GuardianApproval; +use crate::tools::approval::UserApprovalRequest; +use crate::tools::approval::request_approval_for_turn; use crate::tools::sandboxing::ToolError; use codex_network_proxy::BlockedRequest; use codex_network_proxy::BlockedRequestObserver; @@ -369,45 +373,41 @@ impl NetworkApprovalService { protocol, }; let owner_call = self.resolve_single_active_call().await; - let guardian_review_id = guardian_review_id_for_turn(&turn_context); - let approval_id = Self::approval_id_for_key(&key); - let user_session = Arc::clone(&session); - let user_turn_context = Arc::clone(&turn_context); - let user_network_approval_context = network_approval_context.clone(); - let approval_decision = route_approval( + let approval_outcome = request_approval_for_turn( &session, &turn_context, - guardian_review_id.clone(), - None::<(&'static str, Vec)>, - GuardianApprovalRequest::NetworkAccess { - id: approval_id.clone(), - turn_id: owner_call - .as_ref() - .map_or_else(|| turn_context.sub_id.clone(), |call| call.turn_id.clone()), - target: target.clone(), - host: request.host, - protocol, - port: key.port, - }, - Some(policy_denial_message.clone()), - || async move { - user_session - .request_command_approval( - user_turn_context.as_ref(), - approval_id, - /*approval_id*/ None, - vec!["network-access".to_string(), target], - user_turn_context.cwd.to_path_buf(), - Some(prompt_reason), - Some(user_network_approval_context), - /*proposed_execpolicy_amendment*/ None, - /*additional_permissions*/ None, - None, - ) - .await + ApprovalPlan { + cache: ApprovalCache::::None, + user: UserApprovalRequest::Command(CommandApprovalRequest { + call_id: Self::approval_id_for_key(&key), + approval_id: None, + command: vec!["network-access".to_string(), target.clone()], + cwd: turn_context.cwd.to_path_buf(), + reason: Some(prompt_reason), + network_approval_context: Some(network_approval_context.clone()), + proposed_execpolicy_amendment: None, + additional_permissions: None, + available_decisions: None, + }), + guardian: GuardianApproval::new( + GuardianApprovalRequest::NetworkAccess { + id: Self::approval_id_for_key(&key), + turn_id: owner_call.as_ref().map_or_else( + || turn_context.sub_id.clone(), + |call| call.turn_id.clone(), + ), + target, + host: request.host, + protocol, + port: key.port, + }, + Some(policy_denial_message.clone()), + ), }, ) .await; + let guardian_review_id = approval_outcome.guardian_review_id; + let approval_decision = approval_outcome.decision; 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 253127dbde..3346ffcf18 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -9,7 +9,12 @@ use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; use crate::sandboxing::ExecOptions; use crate::sandboxing::execute_env; -use crate::tools::approval::route_approval; +use crate::tools::approval::ApprovalCache; +use crate::tools::approval::ApprovalPlan; +use crate::tools::approval::GuardianApproval; +use crate::tools::approval::PatchApprovalRequest; +use crate::tools::approval::UserApprovalRequest; +use crate::tools::approval::request_approval; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; @@ -150,46 +155,51 @@ impl Approvable for ApplyPatchRuntime { return ReviewDecision::Approved; } if let Some(reason) = retry_reason.clone() { - return route_approval( + return request_approval( session, turn, ctx.guardian_review_id.clone(), - None::<(&'static str, Vec)>, - ApplyPatchRuntime::build_guardian_review_request(req, &call_id), - retry_reason, - || async move { - let rx_approve = session - .request_patch_approval( - turn, - call_id, - changes.clone(), - Some(reason), - /*grant_root*/ None, - ) - .await; - rx_approve.await.unwrap_or_default() + ApprovalPlan { + cache: ApprovalCache::::None, + user: UserApprovalRequest::Patch(PatchApprovalRequest { + call_id: call_id.clone(), + changes: changes.clone(), + reason: Some(reason), + grant_root: None, + }), + guardian: GuardianApproval::new( + ApplyPatchRuntime::build_guardian_review_request(req, &call_id), + retry_reason, + ), }, ) - .await; + .await + .decision; } - route_approval( + request_approval( session, turn, ctx.guardian_review_id.clone(), - Some(("apply_patch", approval_keys)), - ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id), - retry_reason, - || async move { - let rx_approve = session - .request_patch_approval( - turn, call_id, changes, /*reason*/ None, /*grant_root*/ None, - ) - .await; - rx_approve.await.unwrap_or_default() + ApprovalPlan { + cache: ApprovalCache::SessionApproveOnly { + tool_name: "apply_patch", + keys: approval_keys, + }, + user: UserApprovalRequest::Patch(PatchApprovalRequest { + call_id: call_id.clone(), + changes, + reason: None, + grant_root: None, + }), + guardian: GuardianApproval::new( + ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id), + retry_reason, + ), }, ) .await + .decision }) } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index f80fa5e7e2..507dc3dca2 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -15,7 +15,12 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::shell::ShellType; -use crate::tools::approval::route_approval; +use crate::tools::approval::ApprovalCache; +use crate::tools::approval::ApprovalPlan; +use crate::tools::approval::CommandApprovalRequest; +use crate::tools::approval::GuardianApproval; +use crate::tools::approval::UserApprovalRequest; +use crate::tools::approval::request_approval; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_sandbox_command; @@ -151,41 +156,44 @@ impl Approvable for ShellRuntime { let turn = ctx.turn; let call_id = ctx.call_id.to_string(); Box::pin(async move { - let guardian_request = GuardianApprovalRequest::Shell { - 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(), - }; - route_approval( + request_approval( session, turn, ctx.guardian_review_id.clone(), - Some(("shell", keys)), - guardian_request, - retry_reason, - || async move { - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, + ApprovalPlan { + cache: ApprovalCache::SessionApproveOnly { + tool_name: "shell", + keys, + }, + user: UserApprovalRequest::Command(CommandApprovalRequest { + call_id: call_id.clone(), + approval_id: None, + command: command.clone(), + cwd: cwd.clone(), + reason, + network_approval_context: ctx.network_approval_context.clone(), + proposed_execpolicy_amendment: req + .exec_approval_requirement + .proposed_execpolicy_amendment() + .cloned(), + additional_permissions: req.additional_permissions.clone(), + available_decisions: None, + }), + guardian: GuardianApproval::new( + GuardianApprovalRequest::Shell { + id: call_id, command, cwd, - reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement - .proposed_execpolicy_amendment() - .cloned(), - req.additional_permissions.clone(), - None, - ) - .await + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + }, + retry_reason, + ), }, ) .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 f97b47cb2f..827c88a428 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -9,8 +9,12 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecRequest; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; -use crate::tools::approval::guardian_review_id_for_turn; -use crate::tools::approval::route_approval; +use crate::tools::approval::ApprovalCache; +use crate::tools::approval::ApprovalPlan; +use crate::tools::approval::CommandApprovalRequest; +use crate::tools::approval::GuardianApproval; +use crate::tools::approval::UserApprovalRequest; +use crate::tools::approval::request_approval_for_turn; use crate::tools::runtimes::build_sandbox_command; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; @@ -390,46 +394,44 @@ impl CoreShellActionProvider { let call_id = self.call_id.clone(); let approval_id = Some(Uuid::new_v4().to_string()); let source = self.tool_name; - let guardian_review_id = guardian_review_id_for_turn(&turn); - let user_session = session.clone(); - let user_turn = turn.clone(); Ok(stopwatch .pause_for(async move { - let decision = route_approval( + let outcome = request_approval_for_turn( &session, &turn, - guardian_review_id.clone(), - None::<(&'static str, Vec)>, - GuardianApprovalRequest::Execve { - id: call_id.clone(), - source, - program: program.to_string_lossy().into_owned(), - argv: argv.to_vec(), - cwd: workdir.clone(), - additional_permissions: additional_permissions.clone(), - }, - /*retry_reason*/ None, - || async move { - user_session - .request_command_approval( - &user_turn, - call_id, - approval_id, - command, - workdir, - /*reason*/ None, - /*network_approval_context*/ None, - /*proposed_execpolicy_amendment*/ None, + ApprovalPlan { + cache: ApprovalCache::::None, + user: UserApprovalRequest::Command(CommandApprovalRequest { + call_id: call_id.clone(), + approval_id, + command: command.clone(), + cwd: workdir.clone(), + reason: None, + network_approval_context: None, + proposed_execpolicy_amendment: None, + additional_permissions: additional_permissions.clone(), + available_decisions: Some(vec![ + ReviewDecision::Approved, + ReviewDecision::Abort, + ]), + }), + guardian: GuardianApproval::new( + GuardianApprovalRequest::Execve { + id: call_id, + source, + program: program.to_string_lossy().into_owned(), + argv: argv.to_vec(), + cwd: workdir, additional_permissions, - Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), - ) - .await + }, + /*retry_reason*/ None, + ), }, ) .await; PromptDecision { - decision, - guardian_review_id, + decision: outcome.decision, + guardian_review_id: outcome.guardian_review_id, } }) .await) diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 5d8104b7dd..8d4a1c8f56 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -11,7 +11,12 @@ use crate::guardian::GuardianApprovalRequest; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; -use crate::tools::approval::route_approval; +use crate::tools::approval::ApprovalCache; +use crate::tools::approval::ApprovalPlan; +use crate::tools::approval::CommandApprovalRequest; +use crate::tools::approval::GuardianApproval; +use crate::tools::approval::UserApprovalRequest; +use crate::tools::approval::request_approval; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_sandbox_command; @@ -127,42 +132,45 @@ impl Approvable for UnifiedExecRuntime<'_> { let retry_reason = ctx.retry_reason.clone(); let reason = retry_reason.clone().or_else(|| req.justification.clone()); Box::pin(async move { - let guardian_request = GuardianApprovalRequest::ExecCommand { - 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, - }; - route_approval( + request_approval( session, turn, ctx.guardian_review_id.clone(), - Some(("unified_exec", keys)), - guardian_request, - retry_reason, - || async move { - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, + ApprovalPlan { + cache: ApprovalCache::SessionApproveOnly { + tool_name: "unified_exec", + keys, + }, + user: UserApprovalRequest::Command(CommandApprovalRequest { + call_id: call_id.clone(), + approval_id: None, + command: command.clone(), + cwd: cwd.clone(), + reason, + network_approval_context: ctx.network_approval_context.clone(), + proposed_execpolicy_amendment: req + .exec_approval_requirement + .proposed_execpolicy_amendment() + .cloned(), + additional_permissions: req.additional_permissions.clone(), + available_decisions: None, + }), + guardian: GuardianApproval::new( + GuardianApprovalRequest::ExecCommand { + id: call_id, command, cwd, - reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement - .proposed_execpolicy_amendment() - .cloned(), - req.additional_permissions.clone(), - None, - ) - .await + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: req.tty, + }, + retry_reason, + ), }, ) .await + .decision }) }