diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index dd71ae7174..e429f1d6c3 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -398,6 +398,83 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f ); } +#[tokio::test] +async fn handle_exec_approval_preserves_retry_id_through_non_guardian_parent_round_trip() { + let (parent_session, parent_ctx, rx_events) = + crate::session::tests::make_session_and_context_with_rx().await; + *parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); + let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref"); + let mut config = (*parent_ctx.config).clone(); + config.approvals_reviewer = ApprovalsReviewer::User; + parent_ctx.config = Arc::new(config); + let parent_ctx = Arc::new(parent_ctx); + + let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); + let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY); + let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); + let codex = Arc::new(Codex { + tx_sub, + rx_event: rx_events_child, + agent_status, + session: Arc::clone(&parent_session), + session_loop_termination: completed_session_loop_termination(), + }); + + let cancel_token = CancellationToken::new(); + let handle = tokio::spawn({ + let codex = Arc::clone(&codex); + let parent_session = Arc::clone(&parent_session); + let parent_ctx = Arc::clone(&parent_ctx); + let cancel_token = cancel_token.clone(); + async move { + handle_exec_approval( + codex.as_ref(), + "child-turn-1".to_string(), + &parent_session, + &parent_ctx, + delegated_retry_exec_approval_event(), + &cancel_token, + ) + .await; + } + }); + + let request_event = timeout(Duration::from_secs(1), rx_events.recv()) + .await + .expect("exec approval event timed out") + .expect("exec approval event missing"); + let EventMsg::ExecApprovalRequest(request) = request_event.msg else { + panic!("expected ExecApprovalRequest event"); + }; + assert_eq!(request.call_id, "command-item-1"); + assert_eq!(request.approval_id.as_deref(), Some("retry-id")); + assert_eq!( + request.approval_purpose, + Some(ExecApprovalPurpose::SandboxRetry) + ); + + parent_session + .notify_exec_approval("retry-id", ReviewDecision::Approved) + .await; + + timeout(Duration::from_secs(1), handle) + .await + .expect("handle_exec_approval hung") + .expect("handle_exec_approval join error"); + let submission = timeout(Duration::from_secs(1), rx_sub.recv()) + .await + .expect("exec approval response timed out") + .expect("exec approval response missing"); + assert_eq!( + submission.op, + Op::ExecApproval { + id: "retry-id".to_string(), + turn_id: Some("child-turn-1".to_string()), + decision: ReviewDecision::Approved, + } + ); +} + #[tokio::test] async fn delegated_exec_preserves_raw_purpose_and_cancel_cleans_retry_waiter() { let (parent_session, parent_ctx, rx_events) = diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 922242eb08..ebb11fcfca 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2130,10 +2130,13 @@ impl Session { /// Emit an exec approval request event and await the user's decision. /// - /// The request is keyed by `call_id` + `approval_id` so matching responses - /// are delivered to the correct in-flight turn. If the pending approval is - /// cleared before a response arrives, treat it as an abort so interrupted - /// turns do not continue on a synthetic denial. + /// `call_id` is the stable command-item identity. A callback-specific + /// `approval_id`, when present, is the response-routing identity; otherwise + /// `call_id` is used for both. Each callback-specific ID must be unique so a + /// delayed or replayed response cannot resolve a later prompt for the same + /// item. If the pending approval is cleared before a response arrives, + /// treat it as an abort so interrupted turns do not continue on a synthetic + /// denial. /// /// If `available_decisions` is `None`, callback-scoped requests use the /// one-shot approve/abort set; other requests derive their decisions via @@ -2158,8 +2161,9 @@ impl Session { additional_permissions: Option, available_decisions: Option>, ) -> ReviewDecision { - // Command-level approvals use `call_id`; callback-scoped approvals can - // supply a distinct ID for matching the response. + // Cacheable single-prompt approvals use `call_id`. Callback-scoped + // approvals carry a fresh opaque ID for response routing while + // `call_id` remains the stable command-item identity. let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); let proposed_network_policy_amendments = network_approval_context.as_ref().map(|context| { vec![ diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index aaa0dfe9d1..92a33a350d 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -3,8 +3,8 @@ Module: orchestrator Central place for approvals + sandbox selection + retry semantics. Drives a simple sequence for any ToolRuntime: approval → select sandbox → attempt → -retry with an escalated sandbox strategy on denial (no re‑approval thanks to -caching). +retry with an escalated sandbox strategy on denial. Cacheable approvals can +cover that retry; explicitly one-shot approvals cannot. */ use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; @@ -35,12 +35,14 @@ use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxType; use codex_utils_path_uri::PathUri; use std::time::Instant; +use uuid::Uuid; pub(crate) struct ToolOrchestrator { sandbox: SandboxManager, @@ -51,6 +53,13 @@ pub(crate) struct OrchestratorRunResult { pub deferred_network_approval: Option, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum PermissionRequestHookMode { + Skip, + DenyOnly, + AllowAndDeny, +} + impl ToolOrchestrator { pub fn new() -> Self { Self { @@ -164,6 +173,8 @@ impl ToolOrchestrator { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, + approval_id: None, + approval_purpose: ExecApprovalPurpose::Initial, guardian_review_id: guardian_review_id.clone(), retry_reason: None, network_approval_context: None, @@ -174,7 +185,7 @@ impl ToolOrchestrator { tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ false, + PermissionRequestHookMode::Skip, &otel, ) .await?; @@ -193,12 +204,18 @@ impl ToolOrchestrator { ExecApprovalRequirement::Forbidden { reason } => { return Err(ToolError::Rejected(reason.clone())); } - ExecApprovalRequirement::NeedsApproval { reason, .. } => { + ExecApprovalRequirement::NeedsApproval { reason, .. } + | ExecApprovalRequirement::NeedsOneShotApproval { reason } => { let guardian_review_id = use_guardian.then(new_guardian_review_id); + let approval_id = requirement + .is_one_shot() + .then(|| Uuid::new_v4().to_string()); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, + approval_id, + approval_purpose: ExecApprovalPurpose::Initial, guardian_review_id: guardian_review_id.clone(), retry_reason: reason.clone(), network_approval_context: None, @@ -209,7 +226,7 @@ impl ToolOrchestrator { tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, + permission_request_hook_mode(strict_auto_review, &requirement), &otel, ) .await?; @@ -386,15 +403,25 @@ impl ToolOrchestrator { // Strict auto-review approval covers the sandboxed attempt only; // retrying without the sandbox requires a fresh guardian review. - let bypass_retry_approval = !strict_auto_review - && tool.should_bypass_approval(approval_policy, already_approved) - && network_approval_context.is_none(); + let bypass_retry_approval = can_bypass_retry_approval( + strict_auto_review, + &requirement, + tool.should_bypass_approval(approval_policy, already_approved), + network_approval_context.is_some(), + ); if !bypass_retry_approval { + // A tool item can produce both an initial approval and a + // later retry approval. Give the retry its own unguessable + // waiter ID so a duplicated/replayed response for the + // initial prompt cannot resolve the new waiter. + let retry_approval_id = Uuid::new_v4().to_string(); let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, + approval_id: Some(retry_approval_id), + approval_purpose: ExecApprovalPurpose::SandboxRetry, guardian_review_id: guardian_review_id.clone(), retry_reason: Some(retry_reason), network_approval_context: network_approval_context.clone(), @@ -407,7 +434,7 @@ impl ToolOrchestrator { &permission_request_run_id, approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, + permission_request_hook_mode(strict_auto_review, &requirement), &otel, ) .await?; @@ -516,13 +543,13 @@ impl ToolOrchestrator { permission_request_run_id: &str, approval_ctx: ApprovalCtx<'_>, tool_ctx: &ToolCtx, - evaluate_permission_request_hooks: bool, + permission_request_hook_mode: PermissionRequestHookMode, otel: &codex_otel::SessionTelemetry, ) -> Result where T: ToolRuntime, { - if evaluate_permission_request_hooks + if permission_request_hook_mode != PermissionRequestHookMode::Skip && let Some(permission_request) = tool.permission_request_payload(req) { let tool_name = flat_tool_name(&tool_ctx.tool_name); @@ -534,7 +561,9 @@ impl ToolOrchestrator { ) .await { - Some(PermissionRequestDecision::Allow) => { + Some(PermissionRequestDecision::Allow) + if permission_request_hook_mode == PermissionRequestHookMode::AllowAndDeny => + { let decision = ReviewDecision::Approved; otel.tool_decision( tool_name.as_ref(), @@ -554,7 +583,7 @@ impl ToolOrchestrator { ); return Err(ToolError::Rejected(message)); } - None => {} + Some(PermissionRequestDecision::Allow) | None => {} } } @@ -604,6 +633,31 @@ impl ToolOrchestrator { } } +fn can_bypass_retry_approval( + strict_auto_review: bool, + requirement: &ExecApprovalRequirement, + policy_bypasses_approval: bool, + has_network_approval_context: bool, +) -> bool { + !strict_auto_review + && !requirement.is_one_shot() + && policy_bypasses_approval + && !has_network_approval_context +} + +fn permission_request_hook_mode( + strict_auto_review: bool, + requirement: &ExecApprovalRequirement, +) -> PermissionRequestHookMode { + if strict_auto_review { + PermissionRequestHookMode::Skip + } else if requirement.is_one_shot() { + PermissionRequestHookMode::DenyOnly + } else { + PermissionRequestHookMode::AllowAndDeny + } +} + fn sandbox_outcome_from_tool_error(err: &ToolError) -> Option<&'static str> { match err { ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { .. })) => Some("denied"), @@ -618,3 +672,7 @@ fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String { // output so we can evolve heuristics later without touching call sites. "command failed; retry without sandbox?".to_string() } + +#[cfg(test)] +#[path = "orchestrator_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/tools/orchestrator_tests.rs b/codex-rs/core/src/tools/orchestrator_tests.rs new file mode 100644 index 0000000000..cfb779ecb7 --- /dev/null +++ b/codex-rs/core/src/tools/orchestrator_tests.rs @@ -0,0 +1,370 @@ +use super::*; +use crate::state::ActiveTurn; +use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::PermissionRequestPayload; +use crate::tools::sandboxing::Sandboxable; +use codex_hooks::Hooks; +use codex_hooks::HooksConfig; +use codex_protocol::exec_output::ExecToolCallOutput; +use codex_protocol::exec_output::StreamOutput; +use codex_protocol::protocol::EventMsg; +use codex_sandboxing::SandboxablePreference; +use futures::future::BoxFuture; +use pretty_assertions::assert_eq; +use std::sync::Arc; +use std::time::Duration; +use tokio::time::timeout; + +#[derive(Default)] +struct OneShotProbe { + attempts: usize, +} + +impl Approvable<()> for OneShotProbe { + type ApprovalKey = String; + + fn approval_keys(&self, _req: &()) -> Vec { + Vec::new() + } + + fn exec_approval_requirement(&self, _req: &()) -> Option { + Some(ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some("one run only".to_string()), + }) + } + + fn permission_request_payload(&self, _req: &()) -> Option { + Some(PermissionRequestPayload::bash( + "probe".to_string(), + Some("one-shot probe".to_string()), + )) + } + + fn start_approval_async<'a>( + &'a mut self, + _req: &'a (), + ctx: ApprovalCtx<'a>, + ) -> BoxFuture<'a, ReviewDecision> { + Box::pin(async move { + let cwd = ctx + .turn + .environments + .primary() + .expect("primary environment") + .cwd() + .to_abs_path() + .expect("local environment cwd"); + ctx.session + .request_command_approval( + ctx.turn, + ctx.call_id.to_string(), + ctx.approval_id, + Some(ctx.approval_purpose), + /*environment_id*/ None, + vec!["probe".to_string()], + cwd, + ctx.retry_reason, + ctx.network_approval_context, + /*proposed_execpolicy_amendment*/ None, + /*additional_permissions*/ None, + Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), + ) + .await + }) + } +} + +fn install_sequenced_permission_hook( + session: &Arc, + turn: &Arc, + modes: &[&str], +) -> std::path::PathBuf { + let home = &turn.config.codex_home; + std::fs::create_dir_all(home).expect("recreate codex home"); + let script_path = home.join("permission_request_hook.py"); + let log_path = home.join("permission_request_hook_log.jsonl"); + std::fs::write( + &script_path, + format!( + r#"import json +from pathlib import Path +import sys + +log_path = Path(r"{log_path}") +modes = {modes} +payload = json.load(sys.stdin) +seen = [] if not log_path.exists() else log_path.read_text(encoding="utf-8").splitlines() +with log_path.open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +mode = modes[min(len(seen), len(modes) - 1)] +decision = {{"behavior": "allow"}} if mode == "allow" else {{"behavior": "deny", "message": "blocked by test hook"}} +print(json.dumps({{"hookSpecificOutput": {{"hookEventName": "PermissionRequest", "decision": decision}}}})) +"#, + log_path = log_path.display(), + modes = serde_json::to_string(modes).expect("serialize hook modes"), + ), + ) + .expect("write permission hook"); + std::fs::write( + home.join("hooks.json"), + serde_json::json!({ + "hooks": { + "PermissionRequest": [{ + "matcher": "^Bash$", + "hooks": [{ + "type": "command", + "command": format!( + "{} \"{}\"", + if cfg!(windows) { "python" } else { "python3" }, + script_path.display(), + ), + }], + }], + }, + }) + .to_string(), + ) + .expect("write hooks config"); + + let mut shell_argv = session + .user_shell() + .derive_exec_args("", /*use_login_shell*/ false); + let shell_program = shell_argv.remove(0); + let _ = shell_argv.pop(); + session + .services + .hooks + .store(Arc::new(Hooks::new(HooksConfig { + feature_enabled: true, + bypass_hook_trust: true, + config_layer_stack: Some(turn.config.config_layer_stack.clone()), + shell_program: Some(shell_program), + shell_args: shell_argv, + ..HooksConfig::default() + }))); + log_path.into_path_buf() +} + +async fn next_exec_approval( + events: &async_channel::Receiver, +) -> codex_protocol::protocol::ExecApprovalRequestEvent { + loop { + let event = events.recv().await.expect("approval event"); + if let EventMsg::ExecApprovalRequest(approval) = event.msg { + return approval; + } + } +} + +impl Sandboxable for OneShotProbe { + fn sandbox_preference(&self) -> SandboxablePreference { + SandboxablePreference::Auto + } +} + +impl ToolRuntime<(), ()> for OneShotProbe { + async fn run( + &mut self, + _req: &(), + _attempt: &SandboxAttempt<'_>, + _ctx: &ToolCtx, + ) -> Result<(), ToolError> { + self.attempts += 1; + if self.attempts == 1 { + let output = ExecToolCallOutput { + exit_code: 1, + stdout: StreamOutput::new(String::new()), + stderr: StreamOutput::new("sandbox denied".to_string()), + aggregated_output: StreamOutput::new("sandbox denied".to_string()), + duration: Duration::from_millis(1), + timed_out: false, + }; + Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { + output: Box::new(output), + network_policy_decision: None, + }))) + } else { + Ok(()) + } + } +} + +#[tokio::test] +async fn one_shot_retry_uses_distinct_waiter_and_ignores_stale_initial_responses() { + let (session, turn, events) = crate::session::tests::make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let call_id = "one-shot-probe".to_string(); + let tool_ctx = ToolCtx { + session: Arc::clone(&session), + turn: Arc::clone(&turn), + call_id: call_id.clone(), + tool_name: codex_tools::ToolName::plain("probe"), + }; + let mut run = tokio::spawn(async move { + let mut probe = OneShotProbe::default(); + let result = ToolOrchestrator::new() + .run( + &mut probe, + &(), + &tool_ctx, + turn.as_ref(), + AskForApproval::UnlessTrusted, + ) + .await; + (result, probe.attempts) + }); + + let initial = events.recv().await.expect("initial approval event"); + let EventMsg::ExecApprovalRequest(initial) = initial.msg else { + panic!("expected initial exec approval"); + }; + assert_eq!(initial.call_id, call_id); + assert_eq!(initial.approval_purpose, Some(ExecApprovalPurpose::Initial)); + let initial_id = initial.approval_id.clone().expect("initial callback ID"); + assert_ne!(initial_id, call_id); + Uuid::parse_str(&initial_id).expect("initial callback ID should be a UUID"); + assert_eq!( + initial.effective_available_decisions(), + vec![ReviewDecision::Approved, ReviewDecision::Abort] + ); + session + .notify_exec_approval(&initial_id, ReviewDecision::Approved) + .await; + + let retry = events.recv().await.expect("retry approval event"); + let EventMsg::ExecApprovalRequest(retry) = retry.msg else { + panic!("expected retry exec approval"); + }; + assert_eq!(retry.call_id, call_id); + assert_eq!( + retry.approval_purpose, + Some(ExecApprovalPurpose::SandboxRetry) + ); + let retry_id = retry.approval_id.expect("retry callback ID"); + assert_ne!(retry_id, call_id); + assert_ne!(retry_id, initial_id); + Uuid::parse_str(&retry_id).expect("retry callback ID should be a UUID"); + + for stale_id in [&call_id, &initial_id] { + for stale_decision in [ReviewDecision::Approved, ReviewDecision::Abort] { + session.notify_exec_approval(stale_id, stale_decision).await; + } + } + assert!( + timeout(Duration::from_millis(50), &mut run).await.is_err(), + "stale initial responses must not resolve the retry waiter" + ); + + session + .notify_exec_approval(&retry_id, ReviewDecision::Approved) + .await; + let (result, attempts) = timeout(Duration::from_secs(1), &mut run) + .await + .expect("orchestrator timed out") + .expect("orchestrator task failed"); + result.expect("approved retry should succeed"); + assert_eq!(attempts, 2); +} + +#[tokio::test] +async fn one_shot_hooks_allow_falls_through_and_deny_blocks_each_phase() { + for (modes, expected_approvals, expected_attempts, should_succeed) in [ + (&["allow", "allow"][..], 2, 2, true), + (&["deny"][..], 0, 0, false), + (&["allow", "deny"][..], 1, 1, false), + ] { + let (session, turn, events) = + crate::session::tests::make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let log_path = install_sequenced_permission_hook(&session, &turn, modes); + let tool_ctx = ToolCtx { + session: Arc::clone(&session), + turn: Arc::clone(&turn), + call_id: format!("hook-probe-{}", modes.join("-")), + tool_name: codex_tools::ToolName::plain("probe"), + }; + let run = tokio::spawn(async move { + let mut probe = OneShotProbe::default(); + let result = ToolOrchestrator::new() + .run( + &mut probe, + &(), + &tool_ctx, + turn.as_ref(), + AskForApproval::UnlessTrusted, + ) + .await; + (result, probe.attempts) + }); + + for index in 0..expected_approvals { + let approval = next_exec_approval(&events).await; + assert_eq!( + approval.approval_purpose, + Some(if index == 0 { + ExecApprovalPurpose::Initial + } else { + ExecApprovalPurpose::SandboxRetry + }) + ); + session + .notify_exec_approval(&approval.effective_approval_id(), ReviewDecision::Approved) + .await; + } + + let (result, attempts) = timeout(Duration::from_secs(5), run) + .await + .expect("hook scenario timed out") + .expect("hook scenario task failed"); + assert_eq!(result.is_ok(), should_succeed, "hook modes: {modes:?}"); + assert_eq!(attempts, expected_attempts, "hook modes: {modes:?}"); + assert_eq!( + std::fs::read_to_string(log_path) + .expect("read permission hook log") + .lines() + .count(), + modes.len(), + ); + } +} + +#[test] +fn one_shot_never_bypasses_retry_approval() { + let requirement = ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some("one run only".to_string()), + }; + + assert!(!can_bypass_retry_approval( + /*strict_auto_review*/ false, + &requirement, + /*policy_bypasses_approval*/ true, + /*has_network_approval_context*/ false, + )); + assert_eq!( + permission_request_hook_mode(/*strict_auto_review*/ false, &requirement), + PermissionRequestHookMode::DenyOnly, + ); +} + +#[test] +fn cacheable_approval_keeps_session_retry_and_hook_behavior() { + let requirement = ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }; + + assert!(can_bypass_retry_approval( + /*strict_auto_review*/ false, + &requirement, + /*policy_bypasses_approval*/ true, + /*has_network_approval_context*/ false, + )); + assert_eq!( + permission_request_hook_mode(/*strict_auto_review*/ false, &requirement), + PermissionRequestHookMode::AllowAndDeny, + ); + assert_eq!( + permission_request_hook_mode(/*strict_auto_review*/ true, &requirement), + PermissionRequestHookMode::Skip, + ); +} diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 551e5dc142..1577a07550 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -127,6 +127,9 @@ impl Approvable for ShellRuntime { type ApprovalKey = ApprovalKey; fn approval_keys(&self, req: &ShellRequest) -> Vec { + if req.exec_approval_requirement.is_one_shot() { + return Vec::new(); + } vec![ApprovalKey { environment_id: req.turn_environment.environment_id.clone(), command: canonicalize_command_for_approval(&req.command), @@ -150,7 +153,11 @@ impl Approvable for ShellRuntime { let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); + let approval_id = ctx.approval_id.clone(); + let approval_purpose = ctx.approval_purpose; let guardian_review_id = ctx.guardian_review_id.clone(); + let network_approval_context = ctx.network_approval_context.clone(); + let one_shot = req.exec_approval_requirement.is_one_shot(); Box::pin(async move { if let Some(review_id) = guardian_review_id { return review_approval_request( @@ -170,18 +177,19 @@ impl Approvable for ShellRuntime { .await; } with_cached_approval(&session.services, "shell", keys, move || async move { - let available_decisions = None; + let available_decisions = + one_shot.then(|| vec![ReviewDecision::Approved, ReviewDecision::Abort]); session .request_command_approval( turn, call_id, - /*approval_id*/ None, - /*approval_purpose*/ None, + approval_id, + Some(approval_purpose), environment_id, command, cwd, reason, - ctx.network_approval_context.clone(), + network_approval_context, req.exec_approval_requirement .proposed_execpolicy_amendment() .cloned(), diff --git a/codex-rs/core/src/tools/runtimes/shell_tests.rs b/codex-rs/core/src/tools/runtimes/shell_tests.rs index eaa7adf48c..dda220d707 100644 --- a/codex-rs/core/src/tools/runtimes/shell_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell_tests.rs @@ -1,8 +1,41 @@ use super::*; +use crate::state::ActiveTurn; use codex_exec_server::Environment; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_utils_path_uri::PathUri; use std::sync::Arc; +fn one_shot_request() -> ShellRequest { + let cwd = AbsolutePathBuf::try_from(std::env::current_dir().expect("read current dir")) + .expect("current dir is absolute"); + ShellRequest { + command: vec!["echo".to_string(), "one shot".to_string()], + turn_environment: TurnEnvironment::new( + "remote".to_string(), + Arc::new(Environment::default_for_tests()), + PathUri::from_abs_path(&cwd), + /*shell*/ None, + ), + shell_type: None, + hook_command: "echo 'one shot'".to_string(), + cwd, + timeout_ms: None, + cancellation_token: CancellationToken::new(), + env: HashMap::new(), + explicit_env_overrides: HashMap::new(), + network: None, + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + #[cfg(unix)] + additional_permissions_preapproved: false, + justification: None, + exec_approval_requirement: ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some("one run only".to_string()), + }, + } +} + #[tokio::test] async fn approval_key_includes_environment_id() { let cwd = AbsolutePathBuf::try_from(std::env::current_dir().expect("read current dir")) @@ -40,3 +73,56 @@ async fn approval_key_includes_environment_id() { assert_ne!(original_key, other_key); } + +#[tokio::test] +async fn one_shot_approval_has_no_session_cache_key() { + let request = one_shot_request(); + let runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic); + + assert!(runtime.approval_keys(&request).is_empty()); +} + +#[tokio::test] +async fn one_shot_approval_routes_by_callback_id() { + let (session, turn, events) = crate::session::tests::make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let mut runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic); + let request = one_shot_request(); + let call_id = "command-item"; + let approval_id = "retry-callback"; + let approval = runtime.start_approval_async( + &request, + ApprovalCtx { + session: &session, + turn: &turn, + call_id, + approval_id: Some(approval_id.to_string()), + approval_purpose: ExecApprovalPurpose::SandboxRetry, + guardian_review_id: None, + retry_reason: Some("sandbox denied".to_string()), + network_approval_context: None, + }, + ); + let respond = async { + let event = events.recv().await.expect("approval event"); + let EventMsg::ExecApprovalRequest(event) = event.msg else { + panic!("expected exec approval"); + }; + assert_eq!(event.call_id, call_id); + assert_eq!(event.approval_id.as_deref(), Some(approval_id)); + assert_eq!( + event.approval_purpose, + Some(ExecApprovalPurpose::SandboxRetry) + ); + assert_eq!( + event.effective_available_decisions(), + vec![ReviewDecision::Approved, ReviewDecision::Abort] + ); + session + .notify_exec_approval(approval_id, ReviewDecision::Approved) + .await; + }; + + let (decision, ()) = tokio::join!(approval, respond); + assert_eq!(decision, ReviewDecision::Approved); +} diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index e00192cdab..d39c0243cf 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -158,6 +158,9 @@ impl Approvable for UnifiedExecRuntime<'_> { type ApprovalKey = UnifiedExecApprovalKey; fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec { + if req.exec_approval_requirement.is_one_shot() { + return Vec::new(); + } vec![UnifiedExecApprovalKey { environment_id: req.turn_environment.environment_id.clone(), command: canonicalize_command_for_approval(&req.command), @@ -177,11 +180,15 @@ impl Approvable for UnifiedExecRuntime<'_> { let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); + let approval_id = ctx.approval_id.clone(); + let approval_purpose = ctx.approval_purpose; let command = req.command.clone(); let environment_id = Some(req.turn_environment.environment_id.clone()); 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(); + let network_approval_context = ctx.network_approval_context.clone(); + let one_shot = req.exec_approval_requirement.is_one_shot(); Box::pin(async move { let native_cwd = match req.cwd.to_abs_path() { Ok(c) => c, @@ -211,18 +218,19 @@ impl Approvable for UnifiedExecRuntime<'_> { .await; } with_cached_approval(&session.services, "unified_exec", keys, || async move { - let available_decisions = None; + let available_decisions = + one_shot.then(|| vec![ReviewDecision::Approved, ReviewDecision::Abort]); session .request_command_approval( turn, call_id, - /*approval_id*/ None, - /*approval_purpose*/ None, + approval_id, + Some(approval_purpose), environment_id, command, native_cwd, reason, - ctx.network_approval_context.clone(), + network_approval_context, req.exec_approval_requirement .proposed_execpolicy_amendment() .cloned(), @@ -492,9 +500,12 @@ impl<'a> ToolRuntime for UnifiedExecRunt mod tests { use super::*; use crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS; + use crate::state::ActiveTurn; use crate::tools::sandboxing::ToolRuntime; use codex_exec_server::Environment; use codex_exec_server::LOCAL_ENVIRONMENT_ID; + use codex_protocol::protocol::EventMsg; + use codex_protocol::protocol::ExecApprovalPurpose; use codex_tools::ZshForkConfig; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; @@ -552,6 +563,72 @@ mod tests { assert_ne!(original_key, other_key); } + #[tokio::test] + async fn one_shot_approval_has_no_session_cache_key() { + let manager = UnifiedExecProcessManager::default(); + let runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct); + let request = test_request( + SandboxPermissions::UseDefault, + ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some("one run only".to_string()), + }, + ); + + assert!(runtime.approval_keys(&request).is_empty()); + } + + #[tokio::test] + async fn one_shot_approval_routes_by_callback_id() { + let (session, turn, events) = + crate::session::tests::make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let manager = UnifiedExecProcessManager::default(); + let mut runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct); + let request = test_request( + SandboxPermissions::UseDefault, + ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some("one run only".to_string()), + }, + ); + let call_id = "command-item"; + let approval_id = "retry-callback"; + let approval = runtime.start_approval_async( + &request, + ApprovalCtx { + session: &session, + turn: &turn, + call_id, + approval_id: Some(approval_id.to_string()), + approval_purpose: ExecApprovalPurpose::SandboxRetry, + guardian_review_id: None, + retry_reason: Some("sandbox denied".to_string()), + network_approval_context: None, + }, + ); + let respond = async { + let event = events.recv().await.expect("approval event"); + let EventMsg::ExecApprovalRequest(event) = event.msg else { + panic!("expected exec approval"); + }; + assert_eq!(event.call_id, call_id); + assert_eq!(event.approval_id.as_deref(), Some(approval_id)); + assert_eq!( + event.approval_purpose, + Some(ExecApprovalPurpose::SandboxRetry) + ); + assert_eq!( + event.effective_available_decisions(), + vec![ReviewDecision::Approved, ReviewDecision::Abort] + ); + session + .notify_exec_approval(approval_id, ReviewDecision::Approved) + .await; + }; + + let (decision, ()) = tokio::join!(approval, respond); + assert_eq!(decision, ReviewDecision::Approved); + } + #[tokio::test] async fn unified_exec_uses_the_trusted_sandbox_cwd() { let cwd_dir = tempdir().expect("create process temp dir"); diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 8e9e274f24..0a22aa2995 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -19,6 +19,7 @@ use codex_protocol::error::CodexErr; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxCommand; use codex_sandboxing::SandboxManager; @@ -81,9 +82,19 @@ where F: FnOnce() -> Fut, Fut: Future, { - // To be defensive here, don't bother with checking the cache if keys are empty. + // Empty keys deliberately make this request one-shot: ask and record the + // decision, but neither consult nor populate the session cache. if keys.is_empty() { - return fetch().await; + let decision = fetch().await; + services.session_telemetry.counter( + "codex.approval.requested", + /*inc*/ 1, + &[ + ("tool", tool_name), + ("approved", decision.to_opaque_string()), + ], + ); + return decision; } let already_approved = { @@ -122,6 +133,12 @@ pub(crate) struct ApprovalCtx<'a> { pub session: &'a Arc, pub turn: &'a Arc, pub call_id: &'a str, + /// Identifies this specific approval prompt when a tool item can prompt + /// more than once (for example, an unsandboxed retry). + pub approval_id: Option, + /// Describes why this callback is being emitted independently of display + /// text or whether it carries a callback-specific ID. + pub approval_purpose: ExecApprovalPurpose, /// Guardian review lifecycle ID for this approval, when guardian is reviewing it. /// /// This is separate from `call_id`: `call_id` identifies the tool item under @@ -176,6 +193,10 @@ pub(crate) enum ExecApprovalRequirement { /// See core/src/exec_policy.rs for more details on how proposed_execpolicy_amendment is determined. proposed_execpolicy_amendment: Option, }, + /// Approval is required for this invocation, but the decision must not be + /// reused from or written to the session approval cache. + #[cfg_attr(not(windows), allow(dead_code))] + NeedsOneShotApproval { reason: Option }, /// Execution forbidden for this tool call. Forbidden { reason: String }, } @@ -194,6 +215,10 @@ impl ExecApprovalRequirement { _ => None, } } + + pub(crate) fn is_one_shot(&self) -> bool { + matches!(self, Self::NeedsOneShotApproval { .. }) + } } /// - Never: do not ask