From bf6994a3333efcb2cc04923b8bba13bf435ebf9b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Sun, 12 Apr 2026 21:05:41 -0700 Subject: [PATCH] Centralize approval prompting Move shared approval routing and prompt dispatch into tools::approval, and rewire shell, unified exec, apply_patch, network approval, and execve to use the shared plan-based flow. Co-authored-by: Codex --- codex-rs/core/src/codex.rs | 2 +- codex-rs/core/src/state/service.rs | 2 +- codex-rs/core/src/tools/approval.rs | 261 ++++++++++++++++++ codex-rs/core/src/tools/mod.rs | 1 + codex-rs/core/src/tools/network_approval.rs | 84 +++--- .../core/src/tools/runtimes/apply_patch.rs | 77 ++++-- codex-rs/core/src/tools/runtimes/shell.rs | 74 ++--- .../tools/runtimes/shell/unix_escalation.rs | 82 +++--- .../core/src/tools/runtimes/unified_exec.rs | 76 ++--- codex-rs/core/src/tools/sandboxing.rs | 88 +----- 10 files changed, 475 insertions(+), 272 deletions(-) create mode 100644 codex-rs/core/src/tools/approval.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index a3a674cb60..b1c32d7e8e 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -308,6 +308,7 @@ use crate::tasks::ReviewTask; use crate::tasks::SessionTask; use crate::tasks::SessionTaskContext; use crate::tools::ToolRouter; +use crate::tools::approval::ApprovalStore; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::js_repl::JsReplHandle; use crate::tools::js_repl::resolve_compatible_node; @@ -316,7 +317,6 @@ use crate::tools::network_approval::build_blocked_request_observer; use crate::tools::network_approval::build_network_policy_decider; use crate::tools::parallel::ToolCallRuntime; use crate::tools::router::ToolRouterParams; -use crate::tools::sandboxing::ApprovalStore; use crate::turn_diff_tracker::TurnDiffTracker; use crate::turn_timing::TurnTimingState; use crate::turn_timing::record_turn_ttfm_metric; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 2e89585f99..93353bd0d4 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -11,9 +11,9 @@ use crate::guardian::GuardianRejection; use crate::mcp::McpManager; use crate::plugins::PluginsManager; use crate::skills_watcher::SkillsWatcher; +use crate::tools::approval::ApprovalStore; use crate::tools::code_mode::CodeModeService; use crate::tools::network_approval::NetworkApprovalService; -use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecProcessManager; use codex_analytics::AnalyticsEventsClient; use codex_exec_server::Environment; diff --git a/codex-rs/core/src/tools/approval.rs b/codex-rs/core/src/tools/approval.rs new file mode 100644 index 0000000000..64de12a303 --- /dev/null +++ b/codex-rs/core/src/tools/approval.rs @@ -0,0 +1,261 @@ +//! Shared approval coordination primitives used across tool and non-tool flows. +//! +//! This module centralizes: +//! - session-scoped approval caching for "approve for session" decisions +//! - routing approval prompts to the user or guardian reviewer +//! - dispatching typed user/guardian approval prompts +//! - returning prompt metadata needed by caller-specific result handling + +use crate::codex::Session; +use crate::codex::TurnContext; +use crate::guardian::GuardianApprovalRequest; +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)] +pub(crate) struct ApprovalStore { + map: HashMap, +} + +impl ApprovalStore { + pub fn get(&self, key: &K) -> Option + where + K: Serialize, + { + let s = serde_json::to_string(key).ok()?; + self.map.get(&s).cloned() + } + + pub fn put(&mut self, key: K, value: ReviewDecision) + where + K: Serialize, + { + if let Ok(s) = serde_json::to_string(&key) { + self.map.insert(s, value); + } + } +} + +#[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) +} + +/// Takes a vector of approval keys and returns a ReviewDecision. +/// There will be one key in most cases, but apply_patch can modify multiple files at once. +/// +/// - If all keys are already approved for session, we skip prompting. +/// - If the user approves for session, we store the decision for each key individually +/// so future requests touching any subset can also skip prompting. +pub(crate) async fn with_cached_approval( + services: &SessionServices, + tool_name: &str, + keys: Vec, + fetch: F, +) -> ReviewDecision +where + K: Serialize, + F: FnOnce() -> Fut, + Fut: Future, +{ + if keys.is_empty() { + return fetch().await; + } + + let already_approved = { + let store = services.tool_approvals.lock().await; + keys.iter() + .all(|key| matches!(store.get(key), Some(ReviewDecision::ApprovedForSession))) + }; + + if already_approved { + return ReviewDecision::ApprovedForSession; + } + + let decision = fetch().await; + + services.session_telemetry.counter( + "codex.approval.requested", + /*inc*/ 1, + &[ + ("tool", tool_name), + ("approved", decision.to_opaque_string()), + ], + ); + + if matches!(decision, ReviewDecision::ApprovedForSession) { + let mut store = services.tool_approvals.lock().await; + for key in keys { + store.put(key, ReviewDecision::ApprovedForSession); + } + } + + decision +} + +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, + plan: ApprovalPlan, +) -> ApprovalOutcome +where + K: Serialize, +{ + 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, + }; + } + + 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/mod.rs b/codex-rs/core/src/tools/mod.rs index 06c20d6922..ba70a6510c 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod approval; pub(crate) mod code_mode; pub(crate) mod context; pub(crate) mod events; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 9ec9842b74..1df8c4c09f 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -2,10 +2,13 @@ use crate::codex::Session; 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::network_policy_decision::denied_network_policy_message; +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; @@ -370,46 +373,41 @@ impl NetworkApprovalService { protocol, }; let owner_call = self.resolve_single_active_call().await; - let guardian_approval_id = Self::approval_id_for_key(&key); - let use_guardian = routes_approval_to_guardian(&turn_context); - let guardian_review_id = use_guardian.then(new_guardian_review_id); - let approval_decision = if let Some(review_id) = guardian_review_id.clone() { - review_approval_request( - &session, - &turn_context, - review_id, - GuardianApprovalRequest::NetworkAccess { - id: guardian_approval_id.clone(), - 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 - } else { - let approval_id = Self::approval_id_for_key(&key); - let prompt_command = vec!["network-access".to_string(), target.clone()]; - let available_decisions = None; - session - .request_command_approval( - turn_context.as_ref(), - approval_id, - /*approval_id*/ None, - prompt_command, - turn_context.cwd.to_path_buf(), - Some(prompt_reason), - Some(network_approval_context.clone()), - /*proposed_execpolicy_amendment*/ None, - /*additional_permissions*/ None, - available_decisions, - ) - .await - }; + let approval_outcome = request_approval_for_turn( + &session, + &turn_context, + 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 8324f365be..3346ffcf18 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -7,9 +7,14 @@ //! `SandboxAttempt` with a minimal environment for local turns. use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; -use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::execute_env; +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; @@ -18,7 +23,6 @@ use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; -use crate::tools::sandboxing::with_cached_approval; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1; use codex_protocol::exec_output::ExecToolCallOutput; @@ -146,43 +150,56 @@ impl Approvable for ApplyPatchRuntime { let retry_reason = ctx.retry_reason.clone(); let approval_keys = self.approval_keys(req); let changes = req.changes.clone(); - let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { if req.permissions_preapproved && retry_reason.is_none() { return ReviewDecision::Approved; } - if let Some(review_id) = guardian_review_id { - let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); - return review_approval_request(session, turn, review_id, action, retry_reason) - .await; - } - if let Some(reason) = retry_reason { - let rx_approve = session - .request_patch_approval( - turn, - call_id, - changes.clone(), - Some(reason), - /*grant_root*/ None, - ) - .await; - return rx_approve.await.unwrap_or_default(); + if let Some(reason) = retry_reason.clone() { + return request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + 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 + .decision; } - with_cached_approval( - &session.services, - "apply_patch", - approval_keys, - || 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() + request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + 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 6f1c13e000..507dc3dca2 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -11,11 +11,16 @@ pub(crate) mod zsh_fork_backend; use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; -use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::shell::ShellType; +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; @@ -30,7 +35,6 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::sandbox_override_for_first_attempt; -use crate::tools::sandboxing::with_cached_approval; use codex_network_proxy::NetworkProxy; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::PermissionProfile; @@ -151,45 +155,45 @@ impl Approvable for ShellRuntime { let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); - let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if let Some(review_id) = guardian_review_id { - return review_approval_request( - session, - turn, - review_id, - GuardianApprovalRequest::Shell { - id: call_id, - command, - cwd, - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), + request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + ApprovalPlan { + cache: ApprovalCache::SessionApproveOnly { + tool_name: "shell", + keys, }, - retry_reason, - ) - .await; - } - with_cached_approval(&session.services, "shell", keys, move || async move { - let available_decisions = None; - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, - command, - cwd, + user: UserApprovalRequest::Command(CommandApprovalRequest { + call_id: call_id.clone(), + approval_id: None, + command: command.clone(), + cwd: cwd.clone(), reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement + network_approval_context: ctx.network_approval_context.clone(), + proposed_execpolicy_amendment: req + .exec_approval_requirement .proposed_execpolicy_amendment() .cloned(), - req.additional_permissions.clone(), - available_decisions, - ) - .await - }) + additional_permissions: req.additional_permissions.clone(), + available_decisions: None, + }), + guardian: GuardianApproval::new( + GuardianApprovalRequest::Shell { + id: call_id, + command, + cwd, + 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 07c78d2e79..827c88a428 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -5,13 +5,16 @@ 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::sandboxing::ExecOptions; use crate::sandboxing::ExecRequest; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; +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; @@ -391,47 +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 = routes_approval_to_guardian(&turn).then(new_guardian_review_id); Ok(stopwatch .pause_for(async move { - if let Some(review_id) = guardian_review_id.clone() { - let decision = review_approval_request( - &session, - &turn, - review_id, - GuardianApprovalRequest::Execve { - id: call_id.clone(), - source, - program: program.to_string_lossy().into_owned(), - argv: argv.to_vec(), - cwd: workdir, - additional_permissions, - }, - /*retry_reason*/ None, - ) - .await; - return PromptDecision { - decision, - guardian_review_id, - }; - } - let decision = session - .request_command_approval( - &turn, - call_id, - approval_id, - command, - workdir, - /*reason*/ None, - /*network_approval_context*/ None, - /*proposed_execpolicy_amendment*/ None, - additional_permissions, - Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), - ) - .await; + let outcome = request_approval_for_turn( + &session, + &turn, + 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, + }, + /*retry_reason*/ None, + ), + }, + ) + .await; PromptDecision { - decision, - guardian_review_id: None, + 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 94d370fbcd..8d4a1c8f56 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -8,10 +8,15 @@ use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::guardian::GuardianApprovalRequest; -use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; +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; @@ -27,7 +32,6 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::sandbox_override_for_first_attempt; -use crate::tools::sandboxing::with_cached_approval; use crate::unified_exec::NoopSpawnLifecycle; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecProcess; @@ -127,46 +131,46 @@ impl Approvable for UnifiedExecRuntime<'_> { let cwd = req.cwd.to_path_buf(); let retry_reason = ctx.retry_reason.clone(); let reason = retry_reason.clone().or_else(|| req.justification.clone()); - let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if let Some(review_id) = guardian_review_id { - return review_approval_request( - session, - turn, - review_id, - GuardianApprovalRequest::ExecCommand { - id: call_id, - command, - cwd, - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - tty: req.tty, + request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + ApprovalPlan { + cache: ApprovalCache::SessionApproveOnly { + tool_name: "unified_exec", + keys, }, - retry_reason, - ) - .await; - } - with_cached_approval(&session.services, "unified_exec", keys, || async move { - let available_decisions = None; - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, - command, - cwd, + user: UserApprovalRequest::Command(CommandApprovalRequest { + call_id: call_id.clone(), + approval_id: None, + command: command.clone(), + cwd: cwd.clone(), reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement + network_approval_context: ctx.network_approval_context.clone(), + proposed_execpolicy_amendment: req + .exec_approval_requirement .proposed_execpolicy_amendment() .cloned(), - req.additional_permissions.clone(), - available_decisions, - ) - .await - }) + additional_permissions: req.additional_permissions.clone(), + available_decisions: None, + }), + guardian: GuardianApproval::new( + GuardianApprovalRequest::ExecCommand { + id: call_id, + command, + cwd, + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: req.tty, + }, + retry_reason, + ), + }, + ) .await + .decision }) } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index f5ceca5c2c..8f46f7aa95 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -1,14 +1,13 @@ //! Shared approvals and sandboxing traits used by tool runtimes. //! -//! Consolidates the approval flow primitives (`ApprovalDecision`, `ApprovalStore`, -//! `ApprovalCtx`, `Approvable`) together with the sandbox orchestration traits -//! and helpers (`Sandboxable`, `ToolRuntime`, `SandboxAttempt`, etc.). +//! Consolidates the sandbox orchestration traits and helpers used by tool +//! runtimes (`ApprovalCtx`, `Approvable`, `Sandboxable`, `ToolRuntime`, +//! `SandboxAttempt`, etc.). use crate::codex::Session; use crate::codex::TurnContext; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; -use crate::state::SessionServices; use crate::tools::network_approval::NetworkApprovalSpec; use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::ExecPolicyAmendment; @@ -27,94 +26,13 @@ use codex_sandboxing::SandboxTransformError; use codex_sandboxing::SandboxTransformRequest; use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; -use futures::Future; use futures::future::BoxFuture; use serde::Serialize; -use std::collections::HashMap; use std::fmt::Debug; use std::hash::Hash; use std::path::Path; use std::sync::Arc; -#[derive(Clone, Default, Debug)] -pub(crate) struct ApprovalStore { - // Store serialized keys for generic caching across requests. - map: HashMap, -} - -impl ApprovalStore { - pub fn get(&self, key: &K) -> Option - where - K: Serialize, - { - let s = serde_json::to_string(key).ok()?; - self.map.get(&s).cloned() - } - - pub fn put(&mut self, key: K, value: ReviewDecision) - where - K: Serialize, - { - if let Ok(s) = serde_json::to_string(&key) { - self.map.insert(s, value); - } - } -} - -/// Takes a vector of approval keys and returns a ReviewDecision. -/// There will be one key in most cases, but apply_patch can modify multiple files at once. -/// -/// - If all keys are already approved for session, we skip prompting. -/// - If the user approves for session, we store the decision for each key individually -/// so future requests touching any subset can also skip prompting. -pub(crate) async fn with_cached_approval( - services: &SessionServices, - // Name of the tool, used for metrics collection. - tool_name: &str, - keys: Vec, - fetch: F, -) -> ReviewDecision -where - K: Serialize, - F: FnOnce() -> Fut, - Fut: Future, -{ - // To be defensive here, don't bother with checking the cache if keys are empty. - if keys.is_empty() { - return fetch().await; - } - - let already_approved = { - let store = services.tool_approvals.lock().await; - keys.iter() - .all(|key| matches!(store.get(key), Some(ReviewDecision::ApprovedForSession))) - }; - - if already_approved { - return ReviewDecision::ApprovedForSession; - } - - let decision = fetch().await; - - services.session_telemetry.counter( - "codex.approval.requested", - /*inc*/ 1, - &[ - ("tool", tool_name), - ("approved", decision.to_opaque_string()), - ], - ); - - if matches!(decision, ReviewDecision::ApprovedForSession) { - let mut store = services.tool_approvals.lock().await; - for key in keys { - store.put(key, ReviewDecision::ApprovedForSession); - } - } - - decision -} - #[derive(Clone)] pub(crate) struct ApprovalCtx<'a> { pub session: &'a Arc,