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
This commit is contained in:
Dylan Hurd
2026-08-10 17:24:51 +00:00
committed by copyberry
parent 78d3665d15
commit d06dc73290
8 changed files with 581 additions and 139 deletions
+6 -5
View File
@@ -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<TurnContext>, 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(
@@ -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,
+86 -18
View File
@@ -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<StepContext>,
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<ExecPolicyAmendment>,
},
#[cfg(unix)]
Execve {
id: String,
approval_id: String,
environment_id: String,
source: GuardianCommandSource,
program: AbsolutePathBuf,
argv: Vec<String>,
command: Vec<String>,
cwd: AbsolutePathBuf,
additional_permissions: Option<AdditionalPermissionProfile>,
},
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<ReviewDecision, ToolError> {
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,
@@ -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"))
+9 -4
View File
@@ -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,
@@ -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<AdditionalPermissionProfile>,
) -> anyhow::Result<ReviewDecision> {
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)]
@@ -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<Self>,
_session: Arc<crate::session::session::Session>,
_turn_context: Arc<crate::session::turn_context::TurnContext>,
_input: Vec<crate::session::TurnInput>,
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,