From d06dc73290729d2bcb464b955a4cfd9992abc35d Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Mon, 10 Aug 2026 17:19:17 +0000 Subject: [PATCH] Route intercepted exec approvals through shared review (#37851) ## What changed - Send Unix `execve` approvals intercepted by the zsh fork through the shared approval pipeline, including permission hooks, Guardian review, user prompts, and telemetry. - Resolve the active turn and its auto-review setting when an intercepted command needs approval, so commands sent to persistent terminals use the current turn's reviewer. - Give each intercepted command a distinct approval ID and propagate an aborted approval as a turn abort. ## Testing - Cover Guardian review for intercepted `unified_exec` commands and persistent terminals across turns. - Verify repeated identical intercepted commands receive separate user approvals. GitOrigin-RevId: e6cccf160637e4246aff4714c22f08c90b65306d --- codex-rs/core/src/session/mod.rs | 11 +- .../core/src/session/tests/guardian_tests.rs | 12 + codex-rs/core/src/tools/approvals.rs | 104 ++++- codex-rs/core/src/tools/approvals_tests.rs | 18 + codex-rs/core/src/tools/orchestrator.rs | 13 +- .../tools/runtimes/shell/unix_escalation.rs | 127 +++--- .../runtimes/shell/unix_escalation_tests.rs | 46 ++- .../suite/unified_exec_zsh_fork_approvals.rs | 389 ++++++++++++++++-- 8 files changed, 581 insertions(+), 139 deletions(-) diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 213d009a23..b03315eebe 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2891,13 +2891,14 @@ impl Session { clippy::await_holding_invalid_type, reason = "active turn reads must stay consistent with the matching turn state" )] - pub(crate) async fn strict_auto_review_enabled_for_turn(&self) -> bool { + pub(crate) async fn active_turn_context_and_strict_auto_review( + &self, + ) -> Option<(Arc, bool)> { let active = self.active_turn.lock().await; - let Some(active) = active.as_ref() else { - return false; - }; + let active = active.as_ref()?; + let turn_context = Arc::clone(&active.task.as_ref()?.turn_context); let ts = active.turn_state.lock().await; - ts.strict_auto_review_enabled() + Some((turn_context, ts.strict_auto_review_enabled())) } pub(crate) async fn granted_session_permissions( diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 8c3ce63d84..b9937800a9 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -455,6 +455,18 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_shell_command_policy_ ); let session = Arc::new(session); let turn_context = Arc::new(turn_context_raw); + session + .start_task( + Arc::clone(&turn_context), + Vec::new(), + super::NeverEndingTask { + kind: crate::state::TaskKind::Regular, + listen_to_cancellation_token: true, + }, + /*input_persisted*/ None, + crate::tasks::MailboxParentProvenance::Ignore, + ) + .await; let handler = crate::tools::handlers::ShellCommandHandler::from( codex_tools::ShellCommandBackendConfig::Classic, diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 708583c466..1fb02ccd88 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -1,6 +1,7 @@ //! Central approval policy-stage execution and reviewer routing. use crate::command_canonicalization::canonicalize_command_for_approval; +use crate::guardian::GuardianReviewContext; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; @@ -8,7 +9,6 @@ use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; -use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; use crate::tools::flat_tool_name; use crate::tools::hook_names::HookToolName; @@ -22,7 +22,10 @@ use crate::tools::sandboxing::with_cached_approval; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::approvals::ExecPolicyAmendment; +#[cfg(unix)] +use codex_protocol::approvals::GuardianCommandSource; use codex_protocol::approvals::NetworkApprovalContext; +use codex_protocol::error::CodexErr; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkPolicyRuleAction; @@ -36,7 +39,7 @@ use std::sync::Arc; #[derive(Clone)] pub(crate) struct ApprovalContext { - pub(crate) step_context: Arc, + pub(crate) review_context: GuardianReviewContext, pub(crate) call_id: String, pub(crate) tool_name: ToolName, pub(crate) strict_auto_review: bool, @@ -70,6 +73,18 @@ pub(crate) enum ApprovalAction { tty: bool, proposed_execpolicy_amendment: Option, }, + #[cfg(unix)] + Execve { + id: String, + approval_id: String, + environment_id: String, + source: GuardianCommandSource, + program: AbsolutePathBuf, + argv: Vec, + command: Vec, + cwd: AbsolutePathBuf, + additional_permissions: Option, + }, ApplyPatch { id: String, environment_id: String, @@ -102,6 +117,11 @@ impl ApprovalAction { justification, .. } => PermissionRequestPayload::bash(hook_command.clone(), justification.clone()), + #[cfg(unix)] + Self::Execve { command, .. } => PermissionRequestPayload::bash( + codex_shell_command::parse_command::shlex_join(command), + /*description*/ None, + ), Self::ApplyPatch { patch, .. } => PermissionRequestPayload { tool_name: HookToolName::apply_patch(), tool_input: serde_json::json!({ "command": patch }), @@ -141,6 +161,8 @@ impl ApprovalAction { sandbox_permissions: *sandbox_permissions, additional_permissions: additional_permissions.clone(), })], + #[cfg(unix)] + Self::Execve { .. } => Vec::new(), Self::ApplyPatch { environment_id, files, @@ -196,6 +218,23 @@ impl ApprovalAction { justification, tty, }, + #[cfg(unix)] + Self::Execve { + id, + source, + program, + argv, + cwd, + additional_permissions, + .. + } => crate::guardian::GuardianApprovalRequest::Execve { + id, + source, + program: program.to_string_lossy().into_owned(), + argv, + cwd, + additional_permissions, + }, Self::ApplyPatch { id, environment_id, @@ -285,9 +324,7 @@ impl ApprovalResolution { } ReviewDecision::Denied { rejection } => Err(ToolError::Rejected(rejection)), ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())), - ReviewDecision::Abort => { - Err(ToolError::Rejected("approval request aborted".to_string())) - } + ReviewDecision::Abort => Err(ToolError::Codex(CodexErr::TurnAborted)), decision => Ok(decision), } } @@ -299,18 +336,20 @@ impl Session { action: ApprovalAction, ctx: ApprovalContext, ) -> Result { - let permission_request_run_id = ctx - .retry_reason - .as_ref() - .map(|_| format!("{}:retry", ctx.call_id)); + let permission_request_run_id = match &action { + #[cfg(unix)] + ApprovalAction::Execve { approval_id, .. } => approval_id.clone(), + _ if ctx.retry_reason.is_some() => format!("{}:retry", ctx.call_id), + _ => ctx.call_id.clone(), + }; // Approval precedence is: // 1. Hooks // 2. If StrictAutoReview || Guardian enabled, then Guardian. Else, user. let resolution = match run_permission_request_hooks( self, - &ctx.step_context.turn, - permission_request_run_id.as_deref().unwrap_or(&ctx.call_id), + ctx.review_context.turn(), + &permission_request_run_id, action.permission_request_payload(), ) .await @@ -337,7 +376,7 @@ impl Session { let reviewer = if ctx.strict_auto_review { ApprovalReviewer::Guardian } else { - ApprovalReviewer::for_turn(&ctx.step_context.turn) + ApprovalReviewer::for_turn(ctx.review_context.turn()) }; let decision = match reviewer { @@ -369,7 +408,7 @@ impl Session { review_approval_request( self, - &ctx.step_context, + ctx.review_context.clone(), review_id, action, ApprovalRequestReasons { @@ -408,12 +447,16 @@ impl Session { Ok(cwd) => cwd, Err(err) => { tracing::error!(%err, "failed to resolve approval command cwd"); - return ReviewDecision::Abort; + return ReviewDecision::denied(format!( + "failed to resolve approval command cwd: {err}" + )); } }; let tool_name = match action { ApprovalAction::Shell { .. } => "shell", ApprovalAction::ExecCommand { .. } => "unified_exec", + #[cfg(unix)] + ApprovalAction::Execve { .. } => unreachable!("matched command approval"), ApprovalAction::ApplyPatch { .. } => unreachable!("matched command approval"), }; let reason = ctx @@ -423,7 +466,7 @@ impl Session { .or_else(|| justification.clone()); with_cached_approval(&self.services, tool_name, action.cache_keys(), || async { self.request_command_approval( - &ctx.step_context.turn, + ctx.review_context.turn(), ctx.call_id.clone(), /*approval_id*/ None, Some(environment_id.clone()), @@ -440,6 +483,31 @@ impl Session { }) .await } + #[cfg(unix)] + ApprovalAction::Execve { + approval_id, + environment_id, + command, + cwd, + additional_permissions, + .. + } => { + self.request_command_approval( + ctx.review_context.turn(), + ctx.call_id.clone(), + Some(approval_id.clone()), + Some(environment_id.clone()), + command.clone(), + cwd.clone(), + /*reason*/ None, + /*network_approval_context*/ None, + /*proposed_execpolicy_amendment*/ None, + additional_permissions.clone(), + Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), + /*plugin_attribution_override*/ None, + ) + .await + } ApprovalAction::ApplyPatch { changes, permissions_preapproved, @@ -455,7 +523,7 @@ impl Session { if reason.is_some() { return self .request_patch_approval( - &ctx.step_context.turn, + ctx.review_context.turn(), ctx.call_id.clone(), changes.as_ref().clone(), reason, @@ -469,7 +537,7 @@ impl Session { action.cache_keys(), || async { self.request_patch_approval( - &ctx.step_context.turn, + ctx.review_context.turn(), ctx.call_id.clone(), changes.as_ref().clone(), /*reason*/ None, @@ -491,7 +559,7 @@ fn record_resolution(ctx: &ApprovalContext, resolution: &ApprovalResolution) { ApprovalResolutionSource::User => ToolDecisionSource::User, }; let tool_name = flat_tool_name(&ctx.tool_name); - ctx.step_context.turn.session_telemetry.tool_decision( + ctx.review_context.turn().session_telemetry.tool_decision( tool_name.as_ref(), &ctx.call_id, &resolution.decision, diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index 4ab5daadc9..af616fde6b 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -13,12 +13,30 @@ fn approval_resolution_rejects_denied_network_policy_amendment() { }, source: ApprovalResolutionSource::User, }; + assert!(matches!( resolution.into_tool_result(), Err(ToolError::Rejected(rejection)) if rejection == "rejected by user" )); } +#[test] +fn approval_resolution_aborts_turn_when_approval_is_aborted() { + let resolution = ApprovalResolution { + decision: ReviewDecision::Abort, + source: ApprovalResolutionSource::User, + }; + + assert!(matches!( + resolution.into_tool_result(), + Err(ToolError::Codex(error)) + if matches!( + error.details(), + codex_protocol::error::CodexErrorDetails::TurnAborted + ) + )); +} + #[test] fn guardian_cwd_preserves_drive_shaped_local_posix_path() { let native_cwd = AbsolutePathBuf::try_from(std::path::PathBuf::from("/C:/workspace")) diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 8a74f46c90..2f3ee18701 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -6,6 +6,7 @@ simple sequence for any ToolRuntime: approval → select sandbox → attempt → retry with an escalated sandbox strategy on denial (no re‑approval thanks to caching). */ +use crate::guardian::GuardianReviewContext; use crate::network_policy_decision::network_approval_context_from_payload; use crate::tools::approvals::ApprovalContext; use crate::tools::flat_tool_name; @@ -145,7 +146,11 @@ impl ToolOrchestrator { let otel = turn_ctx.session_telemetry.clone(); let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned(); let otel_ci = &tool_ctx.call_id; - let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await; + let strict_auto_review = tool_ctx + .session + .active_turn_context_and_strict_auto_review() + .await + .is_some_and(|(_, strict_auto_review)| strict_auto_review); // 1) Approval let mut already_approved = false; @@ -172,7 +177,7 @@ impl ToolOrchestrator { ToolError::Rejected(format!("could not prepare approval action: {err}")) })?; let approval_ctx = ApprovalContext { - step_context: Arc::clone(&tool_ctx.step_context), + review_context: GuardianReviewContext::from(&tool_ctx.step_context), call_id: tool_ctx.call_id.clone(), tool_name: tool_ctx.tool_name.clone(), strict_auto_review, @@ -204,7 +209,7 @@ impl ToolOrchestrator { ToolError::Rejected(format!("could not prepare approval action: {err}")) })?; let approval_ctx = ApprovalContext { - step_context: Arc::clone(&tool_ctx.step_context), + review_context: GuardianReviewContext::from(&tool_ctx.step_context), call_id: tool_ctx.call_id.clone(), tool_name: tool_ctx.tool_name.clone(), strict_auto_review, @@ -400,7 +405,7 @@ impl ToolOrchestrator { ToolError::Rejected(format!("could not prepare approval action: {err}")) })?; let approval_ctx = ApprovalContext { - step_context: Arc::clone(&tool_ctx.step_context), + review_context: GuardianReviewContext::from(&tool_ctx.step_context), call_id: tool_ctx.call_id.clone(), tool_name: tool_ctx.tool_name.clone(), strict_auto_review, 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 acc7ff82ee..37b0a9c660 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -3,20 +3,16 @@ use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::exec::cancel_when_either; use crate::exec::is_likely_sandbox_denied; -use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianReviewContext; -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::approvals::ApprovalAction; +use crate::tools::approvals::ApprovalContext; 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; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; @@ -29,7 +25,6 @@ use codex_execpolicy::MatchOptions; use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; use codex_features::Feature; -use codex_hooks::PermissionRequestDecision; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; @@ -63,6 +58,7 @@ use codex_shell_escalation::ResolvedPermissionProfile; use codex_shell_escalation::ShellCommandExecutor; use codex_shell_escalation::ShellCommandExecutorFuture; use codex_shell_escalation::Stopwatch; +use codex_tools::ToolName; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use std::collections::HashMap; @@ -238,7 +234,8 @@ pub(super) async fn try_run_zsh_fork( review_context: GuardianReviewContext::from(&ctx.step_context), call_id: ctx.call_id.clone(), environment_id: req.turn_environment.environment_id.clone(), - tool_name: GuardianCommandSource::Shell, + source: GuardianCommandSource::Shell, + tool_name: ctx.tool_name.clone(), approval_policy: ctx.step_context.turn.approval_policy(), permission_profile: command_executor.permission_profile.clone(), sandbox_permissions: req.sandbox_permissions, @@ -324,7 +321,8 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( review_context: GuardianReviewContext::from(&ctx.step_context), call_id: ctx.call_id.clone(), environment_id: req.turn_environment.environment_id.clone(), - tool_name: GuardianCommandSource::UnifiedExec, + source: GuardianCommandSource::UnifiedExec, + tool_name: ctx.tool_name.clone(), approval_policy: ctx.step_context.turn.approval_policy(), permission_profile: exec_request.permission_profile.clone(), sandbox_permissions: req.sandbox_permissions, @@ -358,7 +356,8 @@ struct CoreShellActionProvider { review_context: GuardianReviewContext, call_id: String, environment_id: String, - tool_name: GuardianCommandSource, + source: GuardianCommandSource, + tool_name: ToolName, approval_policy: AskForApproval, permission_profile: PermissionProfile, sandbox_permissions: SandboxPermissions, @@ -441,78 +440,46 @@ impl CoreShellActionProvider { additional_permissions: Option, ) -> anyhow::Result { let command = join_program_and_argv(program, argv); - let workdir = workdir.clone(); - let session = self.session.clone(); - let review_context = self.review_context.clone(); - let turn = Arc::clone(review_context.turn()); - let call_id = self.call_id.clone(); - let approval_id = Some(Uuid::new_v4().to_string()); - let environment_id = Some(self.environment_id.clone()); - 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 { - // 1) Run PermissionRequest hooks - let permission_request = PermissionRequestPayload::bash( - codex_shell_command::parse_command::shlex_join(&command), - /*description*/ None, - ); - let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); - match run_permission_request_hooks( - &session, - &turn, - &effective_approval_id, - permission_request, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return ReviewDecision::Approved; - } - Some(PermissionRequestDecision::Deny { message }) => { - return ReviewDecision::denied(message); - } - None => {} - } - - // 2) Route to Guardian if configured - if let Some(review_id) = guardian_review_id { - return review_approval_request( - &session, - review_context, - review_id, - GuardianApprovalRequest::Execve { - id: call_id.clone(), - source, - program: program.to_string_lossy().into_owned(), - argv: argv.to_vec(), - cwd: workdir.clone(), - additional_permissions, - }, - Default::default(), - ) - .await; - } - - // 3) Fall back to regular user prompt - session - .request_command_approval( - &turn, - call_id, - approval_id, - environment_id, - command, - workdir.clone(), - /*reason*/ None, - /*network_approval_context*/ None, - /*proposed_execpolicy_amendment*/ None, - additional_permissions, - Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), - /*plugin_attribution_override*/ None, - ) + let action = ApprovalAction::Execve { + id: self.call_id.clone(), + approval_id: Uuid::new_v4().to_string(), + environment_id: self.environment_id.clone(), + source: self.source, + program: program.clone(), + argv: argv.to_vec(), + command, + cwd: workdir.clone(), + additional_permissions, + }; + match stopwatch + .pause_for(async { + let (turn_context, strict_auto_review) = self + .session + .active_turn_context_and_strict_auto_review() .await + .ok_or_else(|| { + ToolError::Rejected( + "cannot approve intercepted execution without an active turn" + .to_string(), + ) + })?; + let approval_ctx = ApprovalContext { + review_context: GuardianReviewContext::from(turn_context), + call_id: self.call_id.clone(), + tool_name: self.tool_name.clone(), + strict_auto_review, + approval_reason: None, + retry_reason: None, + network_approval_context: None, + }; + self.session.request_approval(action, approval_ctx).await }) - .await) + .await + { + Ok(decision) => Ok(decision), + Err(ToolError::Rejected(rejection)) => Ok(ReviewDecision::denied(rejection)), + Err(ToolError::Codex(err)) => Err(err.into()), + } } #[allow(clippy::too_many_arguments)] diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 009ec80379..67205e0366 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -38,6 +38,7 @@ use codex_shell_escalation::EscalationExecution; use codex_shell_escalation::EscalationPermissions; use codex_shell_escalation::ExecResult; use codex_shell_escalation::ResolvedPermissionProfile; +use codex_tools::ToolName; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::Value; @@ -429,7 +430,8 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho review_context: GuardianReviewContext::from(Arc::new(turn_context)), call_id: "preapproved-additional-permissions".to_string(), environment_id: "local".to_string(), - tool_name: GuardianCommandSource::Shell, + source: GuardianCommandSource::Shell, + tool_name: ToolName::plain("shell_command"), approval_policy: AskForApproval::OnRequest, permission_profile: permission_profile.clone(), sandbox_permissions: SandboxPermissions::WithAdditionalPermissions, @@ -561,13 +563,43 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul let command = vec!["touch".to_string(), target_str.clone()]; let expected_hook_command = codex_shell_command::parse_command::shlex_join(&["/usr/bin/touch".to_string(), target_str]); + + struct PendingApprovalTask; + + impl crate::tasks::SessionTask for PendingApprovalTask { + fn kind(&self) -> crate::state::TaskKind { + crate::state::TaskKind::Regular + } + + fn span_name(&self) -> &'static str { + "session_task.pending_execve_approval" + } + + async fn run( + self: Arc, + _session: Arc, + _turn_context: Arc, + _input: Vec, + cancellation_token: tokio_util::sync::CancellationToken, + ) -> crate::tasks::SessionTaskResult { + cancellation_token.cancelled().await; + Ok(None) + } + } + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + session + .spawn_task(Arc::clone(&turn_context), Vec::new(), PendingApprovalTask) + .await; let provider = CoreShellActionProvider { policy: std::sync::Arc::new(RwLock::new(codex_execpolicy::Policy::empty())), - session: std::sync::Arc::new(session), - review_context: GuardianReviewContext::from(Arc::new(turn_context)), + session: Arc::clone(&session), + review_context: GuardianReviewContext::from(turn_context), call_id: "execve-hook-call".to_string(), environment_id: "local".to_string(), - tool_name: GuardianCommandSource::Shell, + source: GuardianCommandSource::Shell, + tool_name: ToolName::plain("shell_command"), approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), sandbox_permissions: SandboxPermissions::RequireEscalated, @@ -777,7 +809,8 @@ prefix_rule(pattern = ["{cat_path_literal}"], decision = "allow") review_context: GuardianReviewContext::from(Arc::new(turn_context)), call_id: "deny-read-prefix-allow".to_string(), environment_id: "local".to_string(), - tool_name: GuardianCommandSource::Shell, + source: GuardianCommandSource::Shell, + tool_name: ToolName::plain("shell_command"), approval_policy: AskForApproval::OnRequest, permission_profile, sandbox_permissions: SandboxPermissions::UseDefault, @@ -813,7 +846,8 @@ async fn denied_reads_keep_granular_sandbox_rejection_for_escalation() -> anyhow review_context: GuardianReviewContext::from(Arc::new(turn_context)), call_id: "deny-read-granular-sandbox-reject".to_string(), environment_id: "local".to_string(), - tool_name: GuardianCommandSource::Shell, + source: GuardianCommandSource::Shell, + tool_name: ToolName::plain("shell_command"), approval_policy: AskForApproval::Granular(GranularApprovalConfig { sandbox_approval: false, rules: true, diff --git a/codex-rs/core/tests/suite/unified_exec_zsh_fork_approvals.rs b/codex-rs/core/tests/suite/unified_exec_zsh_fork_approvals.rs index 4c4fc0e895..8f338b4a30 100644 --- a/codex-rs/core/tests/suite/unified_exec_zsh_fork_approvals.rs +++ b/codex-rs/core/tests/suite/unified_exec_zsh_fork_approvals.rs @@ -17,6 +17,9 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; +use codex_protocol::protocol::GuardianAssessmentAction; +use codex_protocol::protocol::GuardianAssessmentStatus; +use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::Op; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::ThreadSettingsOverrides; @@ -27,6 +30,7 @@ use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; +use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; @@ -90,6 +94,7 @@ async fn unified_exec_zsh_fork_parent_approval_preserves_denied_reads() -> Resul &test, "run approved unified exec denied read through zsh fork", approval_policy, + ApprovalsReviewer::User, ) .await?; approve_expected_exec(&test, &command).await?; @@ -149,6 +154,7 @@ async fn unified_exec_zsh_fork_parent_approval_escalates_intercepted_exec() -> R &test, "run approved unified exec through zsh fork", approval_policy, + ApprovalsReviewer::User, ) .await?; approve_expected_exec(&test, &command).await?; @@ -181,7 +187,8 @@ async fn unified_exec_zsh_fork_parent_approval_keeps_explicit_prompt_rule() -> R let outside_path = outside_dir .path() .join("unified-exec-zsh-fork-explicit-prompt-rule.txt"); - let command = format!("touch {outside_path:?}"); + let intercepted_command = format!("touch {outside_path:?}"); + let command = format!("{intercepted_command} && {intercepted_command}"); let rules = r#"prefix_rule(pattern=["touch"], decision="prompt")"#.to_string(); let outside_path_for_hook = outside_path.clone(); @@ -214,38 +221,64 @@ async fn unified_exec_zsh_fork_parent_approval_keeps_explicit_prompt_rule() -> R &test, "run approved unified exec prompt rule through zsh fork", approval_policy, + ApprovalsReviewer::User, ) .await?; approve_expected_exec(&test, &command).await?; - let approval_event = wait_for_event_with_timeout( - &test.codex, - |event| { - matches!( - event, - EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + let mut intercepted_approval_ids = Vec::new(); + for _ in 0..2 { + let approval_event = wait_for_event_with_timeout( + &test.codex, + |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }, + Duration::from_secs(10), + ) + .await; + let EventMsg::ExecApprovalRequest(inner_approval) = approval_event else { + panic!("expected explicit prompt rule approval before completion"); + }; + assert_eq!( + ( + inner_approval.call_id.as_str(), + inner_approval.environment_id.as_deref(), + inner_approval.available_decisions.as_deref(), + ), + ( + call_id, + Some(codex_exec_server::LOCAL_ENVIRONMENT_ID), + Some([ReviewDecision::Approved, ReviewDecision::Abort].as_slice()), ) - }, - Duration::from_secs(10), - ) - .await; - let EventMsg::ExecApprovalRequest(inner_approval) = approval_event else { - panic!("expected explicit prompt rule approval before completion"); - }; - assert!( - inner_approval - .command - .iter() - .any(|arg| arg.ends_with("/touch")) - && inner_approval + ); + let approval_id = inner_approval + .approval_id + .as_ref() + .context("intercepted execve should have a subprocess approval id")?; + assert_ne!(approval_id, call_id); + assert!( + !intercepted_approval_ids.contains(approval_id), + "identical intercepted commands must receive distinct approval ids" + ); + intercepted_approval_ids.push(approval_id.clone()); + assert!( + inner_approval .command .iter() - .any(|arg| arg == outside_path.to_string_lossy().as_ref()), - "expected explicit prompt rule approval for intercepted touch, got: {:?}", - inner_approval.command - ); + .any(|arg| arg.ends_with("/touch")) + && inner_approval + .command + .iter() + .any(|arg| arg == outside_path.to_string_lossy().as_ref()), + "expected explicit prompt rule approval for intercepted touch, got: {:?}", + inner_approval.command + ); - approve_exec(&test, inner_approval.effective_approval_id()).await?; + approve_exec(&test, inner_approval.effective_approval_id()).await?; + } wait_for_completion(&test).await; let result = command_result(&results, call_id); @@ -263,6 +296,309 @@ async fn unified_exec_zsh_fork_parent_approval_keeps_explicit_prompt_rule() -> R Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_zsh_fork_guardian_reviews_intercepted_execve() -> Result<()> { + skip_if_no_network!(Ok(())); + + let approval_policy = AskForApproval::OnRequest; + let permission_profile = restrictive_workspace_write_profile(); + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir + .path() + .join("unified-exec-zsh-fork-guardian-execve.txt"); + let command = format!("touch {outside_path:?}"); + let rules = r#"prefix_rule(pattern=["touch"], decision="prompt")"#.to_string(); + + let outside_path_for_hook = outside_path.clone(); + let Some((server, test)) = build_unified_exec_zsh_fork_test_or_skip( + "unified-exec zsh-fork guardian execve approval test", + approval_policy, + permission_profile, + move |home| { + let _ = fs::remove_file(&outside_path_for_hook); + let rules_dir = home.join("rules"); + fs::create_dir_all(&rules_dir).unwrap(); + fs::write(rules_dir.join("default.rules"), &rules).unwrap(); + }, + ) + .await? + else { + return Ok(()); + }; + + let call_id = "uexec-zsh-fork-guardian-execve"; + let parent_tool_call = exec_command_event( + call_id, + &command, + Some(30_000), + SandboxPermissions::RequireEscalated, + "exercise Guardian review of intercepted execve", + )?; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-guardian-execve-parent"), + parent_tool_call, + ev_completed("resp-guardian-execve-parent"), + ]), + sse(vec![ + ev_response_created("resp-guardian-parent-review"), + ev_assistant_message("msg-guardian-parent-review", r#"{"outcome":"allow"}"#), + ev_completed("resp-guardian-parent-review"), + ]), + sse(vec![ + ev_response_created("resp-guardian-execve-review"), + ev_assistant_message("msg-guardian-execve-review", r#"{"outcome":"allow"}"#), + ev_completed("resp-guardian-execve-review"), + ]), + sse(vec![ + ev_response_created("resp-guardian-execve-done"), + ev_assistant_message("msg-guardian-execve-done", "done"), + ev_completed("resp-guardian-execve-done"), + ]), + ], + ) + .await; + + submit_turn_with_session_permissions( + &test, + "run unified exec with an intercepted command requiring Guardian review", + approval_policy, + ApprovalsReviewer::AutoReview, + ) + .await?; + + let mut intercepted_assessments = Vec::new(); + loop { + let event = tokio::time::timeout(Duration::from_secs(30), test.codex.next_event()) + .await + .context("timed out waiting for intercepted execve Guardian review")??; + match event.msg { + EventMsg::GuardianAssessment(assessment) + if assessment.status == GuardianAssessmentStatus::Approved + && matches!(assessment.action, GuardianAssessmentAction::Execve { .. }) => + { + intercepted_assessments.push(assessment); + } + EventMsg::ExecApprovalRequest(_) => { + panic!("Guardian-reviewed intercepted execution must not prompt the user") + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + + let [assessment] = intercepted_assessments.as_slice() else { + panic!( + "expected one approved intercepted execve Guardian assessment, got {intercepted_assessments:?}" + ); + }; + let GuardianAssessmentAction::Execve { + source, + program, + argv, + cwd, + } = &assessment.action + else { + unreachable!("intercepted assessments contain only execve actions"); + }; + let expected_cwd = test.config.cwd.canonicalize()?; + assert_eq!( + ( + *source, + argv.as_slice(), + cwd, + assessment.target_item_id.as_deref(), + ), + ( + GuardianCommandSource::UnifiedExec, + [ + "touch".to_string(), + outside_path.to_string_lossy().into_owned() + ] + .as_slice(), + &expected_cwd, + Some(call_id), + ) + ); + assert!( + program.ends_with("/touch"), + "unexpected execve program: {program}" + ); + + let guardian_requests = responses + .requests() + .into_iter() + .filter(|request| { + request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian") + }) + .collect::>(); + assert_eq!(guardian_requests.len(), 2); + assert!(guardian_requests[1].body_contains_text(&outside_path.to_string_lossy())); + assert!( + outside_path.exists(), + "Guardian-approved intercepted touch should create the out-of-workspace file" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_zsh_fork_guardian_reviews_persistent_terminal_in_current_turn() -> Result<()> +{ + skip_if_no_network!(Ok(())); + + let approval_policy = AskForApproval::OnRequest; + let permission_profile = restrictive_workspace_write_profile(); + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir + .path() + .join("unified-exec-zsh-fork-current-turn.txt"); + let rules = r#"prefix_rule(pattern=["touch"], decision="prompt")"#.to_string(); + + let outside_path_for_hook = outside_path.clone(); + let Some((server, test)) = build_unified_exec_zsh_fork_test_or_skip( + "unified-exec zsh-fork current-turn guardian approval test", + approval_policy, + permission_profile, + move |home| { + let _ = fs::remove_file(&outside_path_for_hook); + let rules_dir = home.join("rules"); + fs::create_dir_all(&rules_dir).unwrap(); + fs::write(rules_dir.join("default.rules"), &rules).unwrap(); + }, + ) + .await? + else { + return Ok(()); + }; + + let open_call_id = "uexec-zsh-fork-cross-turn-open"; + let open_command = "while IFS= read -r command; do eval \"$command\"; done"; + let open_args = json!({ + "cmd": open_command, + "yield_time_ms": 250, + "tty": true, + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "start an interactive terminal for the next turn", + }); + let write_call_id = "uexec-zsh-fork-cross-turn-write"; + let write_args = json!({ + "chars": format!("touch {outside_path:?}\nexit\n"), + "session_id": 1000, + "yield_time_ms": 5_000, + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-cross-turn-open"), + ev_function_call( + open_call_id, + "exec_command", + &serde_json::to_string(&open_args)?, + ), + ev_completed("resp-cross-turn-open"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-first-done"), + ev_assistant_message("msg-cross-turn-first-done", "terminal is running"), + ev_completed("resp-cross-turn-first-done"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-write"), + ev_function_call( + write_call_id, + "write_stdin", + &serde_json::to_string(&write_args)?, + ), + ev_completed("resp-cross-turn-write"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-guardian"), + ev_assistant_message("msg-cross-turn-guardian", r#"{"outcome":"allow"}"#), + ev_completed("resp-cross-turn-guardian"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-second-done"), + ev_assistant_message("msg-cross-turn-second-done", "done"), + ev_completed("resp-cross-turn-second-done"), + ]), + ], + ) + .await; + + submit_turn_with_session_permissions( + &test, + "start a persistent terminal with user approvals", + approval_policy, + ApprovalsReviewer::User, + ) + .await?; + approve_expected_exec(&test, open_command).await?; + let first_completion = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + let EventMsg::TurnComplete(first_completion) = first_completion else { + unreachable!("completion wait only returns turn-complete events"); + }; + + submit_turn_with_session_permissions( + &test, + "run a command in the persistent terminal with Guardian approvals", + approval_policy, + ApprovalsReviewer::AutoReview, + ) + .await?; + + let mut current_turn_id = None; + let mut intercepted_assessment = None; + loop { + let event = tokio::time::timeout(Duration::from_secs(30), test.codex.next_event()) + .await + .context("timed out waiting for current-turn intercepted execve Guardian review")??; + match event.msg { + EventMsg::TurnStarted(started) => current_turn_id = Some(started.turn_id), + EventMsg::GuardianAssessment(assessment) + if assessment.status == GuardianAssessmentStatus::Approved + && matches!(assessment.action, GuardianAssessmentAction::Execve { .. }) => + { + intercepted_assessment = Some(assessment); + } + EventMsg::ExecApprovalRequest(_) => { + panic!("persistent-terminal approval used the previous turn's user reviewer") + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + + let current_turn_id = current_turn_id.context("expected the second turn to start")?; + assert_ne!(current_turn_id, first_completion.turn_id); + let assessment = intercepted_assessment + .context("expected an approved Guardian assessment for the persistent terminal")?; + assert_eq!(assessment.turn_id, current_turn_id); + assert_eq!(assessment.target_item_id.as_deref(), Some(open_call_id)); + assert!( + outside_path.exists(), + "Guardian-approved command from the second turn should create the file" + ); + + let guardian_requests = responses + .requests() + .into_iter() + .filter(|request| { + request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian") + }) + .collect::>(); + assert_eq!(guardian_requests.len(), 1); + assert!(guardian_requests[0].body_contains_text(&outside_path.to_string_lossy())); + + Ok(()) +} + struct CommandResult { exit_code: Option, stdout: String, @@ -396,6 +732,7 @@ async fn submit_turn_with_session_permissions( test: &TestCodex, prompt: &str, approval_policy: AskForApproval, + approvals_reviewer: ApprovalsReviewer, ) -> Result<()> { let session_model = test.session_configured.model.clone(); let (sandbox_policy, permission_profile) = turn_permission_fields( @@ -414,7 +751,7 @@ async fn submit_turn_with_session_permissions( thread_settings: ThreadSettingsOverrides { environments: Some(local_selections(test.config.cwd.clone())), approval_policy: Some(approval_policy), - approvals_reviewer: Some(ApprovalsReviewer::User), + approvals_reviewer: Some(approvals_reviewer), sandbox_policy: Some(sandbox_policy), permission_profile, collaboration_mode: Some(CollaborationMode {