openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Centralize tool approval handling in Session (#37128)

Browse Source 
## What changed

- Move permission hooks, reviewer routing, approval caching, and user approval requests into the session-level approval flow.
- Have shell, unified exec, and apply-patch runtimes describe requests as `ApprovalAction` values, which now provide their hook payloads and cache keys.
- Pass a shared `ApprovalContext` through initial and retry approvals so telemetry and approval reasons are handled consistently.

GitOrigin-RevId: 5bfa61daa97e4427f3a648f6cf9d1c27f1551942
This commit is contained in:
Dylan Hurd
2026-08-05 17:48:45 +00:00
committed by copyberry
parent 9d00bb01c0
commit 778b869829
10 changed files with 414 additions and 451 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/session/tests.rs
+5 -18
View File
@@ -1196,34 +1196,21 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an
}
impl crate::tools::sandboxing::Approvable<TurnEnvironment> for ProbeToolRuntime {
type ApprovalKey = String;
fn approval_keys(&self, _req: &TurnEnvironment) -> Vec<Self::ApprovalKey> {
vec!["probe".to_string()]
}
fn start_approval_async<'a>(
&'a mut self,
_req: &'a TurnEnvironment,
_ctx: crate::tools::sandboxing::ApprovalCtx<'a>,
) -> futures::future::BoxFuture<'a, ReviewDecision> {
Box::pin(async { ReviewDecision::Approved })
}
fn approval_action(
&self,
_req: &TurnEnvironment,
ctx: &crate::tools::sandboxing::ApprovalCtx<'_>,
call_id: &str,
) -> std::io::Result<crate::tools::sandboxing::ApprovalAction> {
Ok(crate::tools::sandboxing::ApprovalAction::Shell {
id: ctx.call_id.to_string(),
id: call_id.to_string(),
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
command: Vec::new(),
#[allow(deprecated)]
cwd: PathUri::from_abs_path(&ctx.turn.cwd),
hook_command: String::new(),
cwd: PathUri::from_abs_path(&std::env::temp_dir().abs()),
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
additional_permissions: None,
justification: None,
proposed_execpolicy_amendment: None,
})
}
}
codex-rs/core/src/tools/approvals.rs
+297 -87
View File
@@ -1,24 +1,48 @@
//! Central approval policy-stage execution and reviewer routing.
use crate::command_canonicalization::canonicalize_command_for_approval;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::guardian::routes_approval_to_guardian_with_reviewer;
use crate::hook_runtime::run_permission_request_hooks;
use crate::sandboxing::SandboxPermissions;
use crate::session::session::Session;
use crate::session::turn_context::TurnContext;
use crate::tools::flat_tool_name;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::hook_names::HookToolName;
use crate::tools::runtimes::apply_patch::ApplyPatchApprovalKey;
use crate::tools::runtimes::shell::ApprovalKey;
use crate::tools::runtimes::unified_exec::UnifiedExecApprovalKey;
use crate::tools::sandboxing::ApprovalRequestReasons;
use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use codex_config::types::ApprovalsReviewer;
use crate::tools::sandboxing::with_cached_approval;
use codex_hooks::PermissionRequestDecision;
use codex_otel::ToolDecisionSource;
use codex_protocol::approvals::ExecPolicyAmendment;
use codex_protocol::approvals::NetworkApprovalContext;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
use codex_tools::ToolName;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use std::collections::HashMap;
use std::path::PathBuf;
use std::sync::Arc;
#[derive(Clone)]
pub(crate) struct ApprovalContext {
pub(crate) turn: Arc<TurnContext>,
pub(crate) call_id: String,
pub(crate) tool_name: ToolName,
pub(crate) strict_auto_review: bool,
pub(crate) approval_reason: Option<String>,
pub(crate) retry_reason: Option<String>,
pub(crate) network_approval_context: Option<NetworkApprovalContext>,
}
#[derive(Clone, Debug, PartialEq)]
pub(crate) enum ApprovalAction {
@@ -26,20 +50,24 @@ pub(crate) enum ApprovalAction {
id: String,
environment_id: String,
command: Vec<String>,
hook_command: String,
cwd: PathUri,
sandbox_permissions: SandboxPermissions,
additional_permissions: Option<AdditionalPermissionProfile>,
justification: Option<String>,
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
},
ExecCommand {
id: String,
environment_id: String,
command: Vec<String>,
hook_command: String,
cwd: PathUri,
sandbox_permissions: SandboxPermissions,
additional_permissions: Option<AdditionalPermissionProfile>,
justification: Option<String>,
tty: bool,
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
},
ApplyPatch {
id: String,
@@ -47,10 +75,88 @@ pub(crate) enum ApprovalAction {
cwd: PathUri,
files: Vec<PathUri>,
patch: String,
changes: Arc<HashMap<PathBuf, FileChange>>,
permissions_preapproved: bool,
},
}
#[derive(Clone, Debug, Eq, Hash, PartialEq, serde::Serialize)]
#[serde(untagged)]
pub(crate) enum ApprovalCacheKey {
Shell(ApprovalKey),
ExecCommand(UnifiedExecApprovalKey),
ApplyPatch(ApplyPatchApprovalKey),
}
impl ApprovalAction {
pub(crate) fn permission_request_payload(&self) -> PermissionRequestPayload {
match self {
Self::Shell {
hook_command,
justification,
..
}
| Self::ExecCommand {
hook_command,
justification,
..
} => PermissionRequestPayload::bash(hook_command.clone(), justification.clone()),
Self::ApplyPatch { patch, .. } => PermissionRequestPayload {
tool_name: HookToolName::apply_patch(),
tool_input: serde_json::json!({ "command": patch }),
},
}
}
pub(crate) fn cache_keys(&self) -> Vec<ApprovalCacheKey> {
match self {
Self::Shell {
environment_id,
command,
cwd,
sandbox_permissions,
additional_permissions,
..
} => vec![ApprovalCacheKey::Shell(ApprovalKey {
environment_id: environment_id.clone(),
command: canonicalize_command_for_approval(command),
cwd: cwd.clone(),
sandbox_permissions: *sandbox_permissions,
additional_permissions: additional_permissions.clone(),
})],
Self::ExecCommand {
environment_id,
command,
cwd,
tty,
sandbox_permissions,
additional_permissions,
..
} => vec![ApprovalCacheKey::ExecCommand(UnifiedExecApprovalKey {
environment_id: environment_id.clone(),
command: canonicalize_command_for_approval(command),
cwd: cwd.clone(),
tty: *tty,
sandbox_permissions: *sandbox_permissions,
additional_permissions: additional_permissions.clone(),
})],
Self::ApplyPatch {
environment_id,
files,
..
} => files
.iter()
.cloned()
.map(|path| {
ApprovalCacheKey::ApplyPatch(ApplyPatchApprovalKey {
environment_id: environment_id.clone(),
path,
})
})
.collect(),
}
}
fn into_guardian_request(self) -> std::io::Result<crate::guardian::GuardianApprovalRequest> {
Ok(match self {
Self::Shell {
@@ -61,6 +167,7 @@ impl ApprovalAction {
sandbox_permissions,
additional_permissions,
justification,
..
} => crate::guardian::GuardianApprovalRequest::Shell {
id,
command,
@@ -78,6 +185,7 @@ impl ApprovalAction {
additional_permissions,
justification,
tty,
..
} => crate::guardian::GuardianApprovalRequest::ExecCommand {
id,
command,
@@ -93,6 +201,7 @@ impl ApprovalAction {
cwd,
files,
patch,
..
} => crate::guardian::GuardianApprovalRequest::ApplyPatch {
id,
cwd: guardian_cwd(&environment_id, cwd)?,
@@ -129,18 +238,14 @@ fn guardian_cwd(environment_id: &str, cwd: PathUri) -> std::io::Result<AbsoluteP
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(super) enum ApprovalReviewer {
enum ApprovalReviewer {
Guardian,
User,
}
impl ApprovalReviewer {
pub(super) fn for_turn(turn: &TurnContext) -> Self {
Self::for_reviewer(turn, turn.config.approvals_reviewer)
}
fn for_reviewer(turn: &TurnContext, reviewer: ApprovalsReviewer) -> Self {
if routes_approval_to_guardian_with_reviewer(turn, reviewer) {
fn for_turn(turn: &TurnContext) -> Self {
if routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) {
Self::Guardian
} else {
Self::User
@@ -187,102 +292,207 @@ impl ApprovalResolution {
}
}
pub(super) async fn resolve_tool_approval<Rq, Out, T>(
tool: &mut T,
req: &Rq,
ctx: ApprovalCtx<'_>,
reviewer: ApprovalReviewer,
) -> Result<ReviewDecision, ToolError>
where
T: ToolRuntime<Rq, Out>,
{
if let Some(permission_request) = tool.permission_request_payload(req) {
impl Session {
pub(crate) async fn request_approval(
self: &Arc<Self>,
action: ApprovalAction,
ctx: ApprovalContext,
) -> Result<ReviewDecision, ToolError> {
let permission_request_run_id = ctx
.reasons
.retry
.retry_reason
.as_ref()
.map(|_| format!("{}:retry", ctx.call_id));
match run_permission_request_hooks(
ctx.session,
ctx.turn,
permission_request_run_id.as_deref().unwrap_or(ctx.call_id),
permission_request,
// Approval precedence is:
// 1. Hooks
// 2. If StrictAutoReview || Guardian enabled, then Guardian. Else, user.
let resolution = match run_permission_request_hooks(
self,
&ctx.turn,
permission_request_run_id.as_deref().unwrap_or(&ctx.call_id),
action.permission_request_payload(),
)
.await
{
Some(PermissionRequestDecision::Allow) => {
let resolution = ApprovalResolution {
decision: ReviewDecision::Approved,
source: ApprovalResolutionSource::Hook,
};
record_resolution(&ctx, &resolution);
return resolution.into_tool_result();
}
Some(PermissionRequestDecision::Deny { message }) => {
let resolution = ApprovalResolution {
decision: ReviewDecision::denied(message),
source: ApprovalResolutionSource::Hook,
};
record_resolution(&ctx, &resolution);
return resolution.into_tool_result();
}
None => {}
}
Some(PermissionRequestDecision::Allow) => ApprovalResolution {
decision: ReviewDecision::Approved,
source: ApprovalResolutionSource::Hook,
},
Some(PermissionRequestDecision::Deny { message }) => ApprovalResolution {
decision: ReviewDecision::denied(message),
source: ApprovalResolutionSource::Hook,
},
None => self.request_reviewer_approval(action, &ctx).await,
};
record_resolution(&ctx, &resolution);
resolution.into_tool_result()
}
let resolution = match reviewer {
ApprovalReviewer::Guardian => {
let review_id = new_guardian_review_id();
let action = match tool
.approval_action(req, &ctx)
.and_then(ApprovalAction::into_guardian_request)
{
Ok(action) => action,
Err(err) => {
tracing::error!(%err, "failed to build automatic approval action");
let resolution = ApprovalResolution {
decision: ReviewDecision::denied(
"automatic approval review could not prepare the action",
),
source: ApprovalResolutionSource::Guardian,
};
record_resolution(&ctx, &resolution);
return resolution.into_tool_result();
async fn request_reviewer_approval(
self: &Arc<Self>,
action: ApprovalAction,
ctx: &ApprovalContext,
) -> ApprovalResolution {
let reviewer = if ctx.strict_auto_review {
ApprovalReviewer::Guardian
} else {
ApprovalReviewer::for_turn(&ctx.turn)
};
let decision = match reviewer {
ApprovalReviewer::Guardian => self.request_guardian_approval(action, ctx).await,
ApprovalReviewer::User => self.request_user_approval(&action, ctx).await,
};
let source = match reviewer {
ApprovalReviewer::Guardian => ApprovalResolutionSource::Guardian,
ApprovalReviewer::User => ApprovalResolutionSource::User,
};
ApprovalResolution { decision, source }
}
async fn request_guardian_approval(
self: &Arc<Self>,
action: ApprovalAction,
ctx: &ApprovalContext,
) -> ReviewDecision {
let review_id = new_guardian_review_id();
let action = match action.into_guardian_request() {
Ok(action) => action,
Err(err) => {
tracing::error!(%err, "failed to build automatic approval action");
return ReviewDecision::denied(
"automatic approval review could not prepare the action",
);
}
};
review_approval_request(
self,
&ctx.turn,
review_id,
action,
ApprovalRequestReasons {
approval: ctx.approval_reason.clone(),
retry: ctx.retry_reason.clone(),
},
)
.await
}
async fn request_user_approval(
&self,
action: &ApprovalAction,
ctx: &ApprovalContext,
) -> ReviewDecision {
match action {
ApprovalAction::Shell {
environment_id,
command,
cwd,
additional_permissions,
justification,
proposed_execpolicy_amendment,
..
}
| ApprovalAction::ExecCommand {
environment_id,
command,
cwd,
additional_permissions,
justification,
proposed_execpolicy_amendment,
..
} => {
let cwd = match guardian_cwd(environment_id, cwd.clone()) {
Ok(cwd) => cwd,
Err(err) => {
tracing::error!(%err, "failed to resolve approval command cwd");
return ReviewDecision::Abort;
}
};
let tool_name = match action {
ApprovalAction::Shell { .. } => "shell",
ApprovalAction::ExecCommand { .. } => "unified_exec",
ApprovalAction::ApplyPatch { .. } => unreachable!("matched command approval"),
};
let reason = ctx
.retry_reason
.clone()
.or_else(|| ctx.approval_reason.clone())
.or_else(|| justification.clone());
with_cached_approval(&self.services, tool_name, action.cache_keys(), || async {
self.request_command_approval(
&ctx.turn,
ctx.call_id.clone(),
/*approval_id*/ None,
Some(environment_id.clone()),
command.clone(),
cwd,
reason,
ctx.network_approval_context.clone(),
proposed_execpolicy_amendment.clone(),
additional_permissions.clone(),
/*available_decisions*/ None,
/*plugin_attribution_override*/ None,
)
.await
})
.await
}
ApprovalAction::ApplyPatch {
changes,
permissions_preapproved,
..
} => {
let reason = ctx
.retry_reason
.clone()
.or_else(|| ctx.approval_reason.clone());
if *permissions_preapproved && reason.is_none() {
return ReviewDecision::Approved;
}
};
review_approval_request(
ctx.session,
ctx.turn,
review_id,
action,
ctx.reasons.clone(),
)
.await
if reason.is_some() {
return self
.request_patch_approval(
&ctx.turn,
ctx.call_id.clone(),
changes.as_ref().clone(),
reason,
/*grant_root*/ None,
)
.await;
}
with_cached_approval(
&self.services,
"apply_patch",
action.cache_keys(),
|| async {
self.request_patch_approval(
&ctx.turn,
ctx.call_id.clone(),
changes.as_ref().clone(),
/*reason*/ None,
/*grant_root*/ None,
)
.await
},
)
.await
}
}
ApprovalReviewer::User => tool.start_approval_async(req, ctx.clone()).await,
};
let source = match reviewer {
ApprovalReviewer::Guardian => ApprovalResolutionSource::Guardian,
ApprovalReviewer::User => ApprovalResolutionSource::User,
};
let resolution = ApprovalResolution {
decision: resolution,
source,
};
record_resolution(&ctx, &resolution);
resolution.into_tool_result()
}
}
fn record_resolution(ctx: &ApprovalCtx<'_>, resolution: &ApprovalResolution) {
fn record_resolution(ctx: &ApprovalContext, resolution: &ApprovalResolution) {
let source = match resolution.source {
ApprovalResolutionSource::Hook => ToolDecisionSource::Config,
ApprovalResolutionSource::Guardian => ToolDecisionSource::AutomatedReviewer,
ApprovalResolutionSource::User => ToolDecisionSource::User,
};
let tool_name = flat_tool_name(ctx.tool_name);
let tool_name = flat_tool_name(&ctx.tool_name);
ctx.turn.session_telemetry.tool_decision(
tool_name.as_ref(),
ctx.call_id,
&ctx.call_id,
&resolution.decision,
source,
);
codex-rs/core/src/tools/handlers/apply_patch.rs
+1 -1
View File
@@ -556,7 +556,7 @@ async fn execute_verified_patch(
turn_environment,
action: apply.action,
file_paths,
changes,
changes: Arc::new(changes),
exec_approval_requirement: apply.exec_approval_requirement,
additional_permissions: effective_additional_permissions.additional_permissions,
permissions_preapproved: effective_additional_permissions.permissions_preapproved,
codex-rs/core/src/tools/orchestrator.rs
+49 -51
View File
@@ -6,9 +6,8 @@ simple sequence for any ToolRuntime: approval → select sandbox → attempt →
retry with an escalated sandbox strategy on denial (no re‑approval thanks to
caching).
*/
use super::approvals::ApprovalReviewer;
use super::approvals::resolve_tool_approval;
use crate::network_policy_decision::network_approval_context_from_payload;
use crate::tools::approvals::ApprovalContext;
use crate::tools::flat_tool_name;
use crate::tools::network_approval::ActiveNetworkApproval;
use crate::tools::network_approval::DeferredNetworkApproval;
@@ -16,8 +15,6 @@ use crate::tools::network_approval::NetworkApprovalMode;
use crate::tools::network_approval::begin_network_approval;
use crate::tools::network_approval::finish_deferred_network_approval;
use crate::tools::network_approval::finish_immediate_network_approval;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ApprovalRequestReasons;
use crate::tools::sandboxing::ExecApprovalRequirement;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::SandboxOverride;
@@ -35,6 +32,7 @@ use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxManager;
use codex_sandboxing::SandboxType;
use std::sync::Arc;
use std::time::Instant;
pub(crate) struct ToolOrchestrator {
@@ -162,15 +160,23 @@ impl ToolOrchestrator {
match &requirement {
ExecApprovalRequirement::Skip { .. } => {
if strict_auto_review {
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
tool_name: &tool_ctx.tool_name,
reasons: ApprovalRequestReasons::default(),
let action = tool
.approval_action(req, &tool_ctx.call_id)
.map_err(|err| {
ToolError::Rejected(format!("could not prepare approval action: {err}"))
})?;
let approval_ctx = ApprovalContext {
turn: Arc::clone(&tool_ctx.turn),
call_id: tool_ctx.call_id.clone(),
tool_name: tool_ctx.tool_name.clone(),
strict_auto_review,
approval_reason: None,
retry_reason: None,
network_approval_context: None,
};
resolve_tool_approval(tool, req, approval_ctx, ApprovalReviewer::Guardian)
tool_ctx
.session
.request_approval(action, approval_ctx)
.await?;
already_approved = true;
} else {
@@ -186,28 +192,24 @@ impl ToolOrchestrator {
return Err(ToolError::Rejected(reason.clone()));
}
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
tool_name: &tool_ctx.tool_name,
reasons: ApprovalRequestReasons {
approval: reason.clone(),
retry: None,
},
let action = tool
.approval_action(req, &tool_ctx.call_id)
.map_err(|err| {
ToolError::Rejected(format!("could not prepare approval action: {err}"))
})?;
let approval_ctx = ApprovalContext {
turn: Arc::clone(&tool_ctx.turn),
call_id: tool_ctx.call_id.clone(),
tool_name: tool_ctx.tool_name.clone(),
strict_auto_review,
approval_reason: reason.clone(),
retry_reason: None,
network_approval_context: None,
};
resolve_tool_approval(
tool,
req,
approval_ctx,
if strict_auto_review {
ApprovalReviewer::Guardian
} else {
ApprovalReviewer::for_turn(turn_ctx)
},
)
.await?;
tool_ctx
.session
.request_approval(action, approval_ctx)
.await?;
already_approved = true;
}
}
@@ -386,29 +388,25 @@ impl ToolOrchestrator {
ExecApprovalRequirement::Skip { .. }
| ExecApprovalRequirement::Forbidden { .. } => None,
};
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
tool_name: &tool_ctx.tool_name,
reasons: ApprovalRequestReasons {
approval: approval_reason,
retry: Some(retry_reason),
},
let action = tool
.approval_action(req, &tool_ctx.call_id)
.map_err(|err| {
ToolError::Rejected(format!("could not prepare approval action: {err}"))
})?;
let approval_ctx = ApprovalContext {
turn: Arc::clone(&tool_ctx.turn),
call_id: tool_ctx.call_id.clone(),
tool_name: tool_ctx.tool_name.clone(),
strict_auto_review,
approval_reason,
retry_reason: Some(retry_reason),
network_approval_context: network_approval_context.clone(),
};
resolve_tool_approval(
tool,
req,
approval_ctx,
if strict_auto_review {
ApprovalReviewer::Guardian
} else {
ApprovalReviewer::for_turn(turn_ctx)
},
)
.await?;
tool_ctx
.session
.request_approval(action, approval_ctx)
.await?;
}
let retry_sandbox_requested = !unsandboxed_allowed
codex-rs/core/src/tools/runtimes/apply_patch.rs
+8 -78
View File
@@ -5,18 +5,14 @@
//! sandboxing enforced by the explicit filesystem sandbox context.
use crate::exec::is_likely_sandbox_denied;
use crate::session::turn_context::TurnEnvironment;
use crate::tools::hook_names::HookToolName;
use crate::tools::sandboxing::Approvable;
use crate::tools::sandboxing::ApprovalAction;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ExecApprovalRequirement;
use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::Sandboxable;
use crate::tools::sandboxing::ToolCtx;
use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use crate::tools::sandboxing::with_cached_approval;
use codex_apply_patch::AppliedPatchDelta;
use codex_apply_patch::ApplyPatchAction;
use codex_exec_server::FileSystemSandboxContext;
@@ -27,20 +23,19 @@ use codex_protocol::exec_output::StreamOutput;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxType;
use codex_sandboxing::SandboxablePreference;
use codex_sandboxing::policy_transforms::effective_permission_profile;
use codex_sandboxing::record_filesystem_sandbox_violation;
use codex_utils_path_uri::PathUri;
use futures::future::BoxFuture;
use std::path::PathBuf;
use std::sync::Arc;
use std::time::Instant;
#[derive(Clone, Debug, Eq, PartialEq, Hash, serde::Serialize)]
pub(crate) struct ApplyPatchApprovalKey {
environment_id: String,
path: PathUri,
pub(crate) environment_id: String,
pub(crate) path: PathUri,
}
#[derive(Debug)]
@@ -48,7 +43,7 @@ pub struct ApplyPatchRequest {
pub turn_environment: TurnEnvironment,
pub action: ApplyPatchAction,
pub file_paths: Vec<PathUri>,
pub changes: std::collections::HashMap<PathBuf, FileChange>,
pub changes: Arc<std::collections::HashMap<PathBuf, FileChange>>,
pub exec_approval_requirement: ExecApprovalRequirement,
pub additional_permissions: Option<AdditionalPermissionProfile>,
pub permissions_preapproved: bool,
@@ -81,6 +76,8 @@ impl ApplyPatchRuntime {
cwd: req.action.cwd.clone(),
files: req.file_paths.clone(),
patch: req.action.patch.clone(),
changes: Arc::clone(&req.changes),
permissions_preapproved: req.permissions_preapproved,
}
}
@@ -118,69 +115,12 @@ impl Sandboxable for ApplyPatchRuntime {
}
impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
type ApprovalKey = ApplyPatchApprovalKey;
fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec<Self::ApprovalKey> {
req.file_paths
.iter()
.cloned()
.map(|path| ApplyPatchApprovalKey {
environment_id: req.turn_environment.environment_id.clone(),
path,
})
.collect()
}
fn start_approval_async<'a>(
&'a mut self,
req: &'a ApplyPatchRequest,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision> {
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
let approval_reason = ctx.reasons.approval.clone();
let retry_reason = ctx.reasons.retry.clone();
let approval_keys = self.approval_keys(req);
let changes = req.changes.clone();
Box::pin(async move {
if req.permissions_preapproved && approval_reason.is_none() && retry_reason.is_none() {
return ReviewDecision::Approved;
}
if let Some(reason) = retry_reason.or(approval_reason) {
return session
.request_patch_approval(
turn,
call_id,
changes.clone(),
Some(reason),
/*grant_root*/ None,
)
.await;
}
with_cached_approval(
&session.services,
"apply_patch",
approval_keys,
|| async move {
session
.request_patch_approval(
turn, call_id, changes, /*reason*/ None, /*grant_root*/ None,
)
.await
},
)
.await
})
}
fn approval_action(
&self,
req: &ApplyPatchRequest,
ctx: &ApprovalCtx<'_>,
call_id: &str,
) -> std::io::Result<ApprovalAction> {
Ok(ApplyPatchRuntime::build_approval_action(req, ctx.call_id))
Ok(ApplyPatchRuntime::build_approval_action(req, call_id))
}
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
@@ -202,16 +142,6 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
) -> Option<ExecApprovalRequirement> {
Some(req.exec_approval_requirement.clone())
}
fn permission_request_payload(
&self,
req: &ApplyPatchRequest,
) -> Option<PermissionRequestPayload> {
Some(PermissionRequestPayload {
tool_name: HookToolName::apply_patch(),
tool_input: serde_json::json!({ "command": req.action.patch }),
})
}
}
impl ToolRuntime<ApplyPatchRequest, ApplyPatchRuntimeOutput> for ApplyPatchRuntime {
codex-rs/core/src/tools/runtimes/apply_patch_tests.rs
+14 -11
View File
@@ -65,7 +65,7 @@ async fn approval_action_preserves_patch_path_uris() {
turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID),
action,
file_paths: vec![path.clone()],
changes: HashMap::new(),
changes: Arc::new(HashMap::new()),
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
@@ -84,13 +84,14 @@ async fn approval_action_preserves_patch_path_uris() {
cwd: expected_cwd,
files: vec![path],
patch: expected_patch,
changes: Arc::new(HashMap::new()),
permissions_preapproved: false,
}
);
}
#[tokio::test]
async fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() {
let runtime = ApplyPatchRuntime::new();
let path = std::env::temp_dir()
.join("apply-patch-permission-request-payload.txt")
.abs();
@@ -101,7 +102,7 @@ async fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() {
turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID),
action,
file_paths: vec![PathUri::from_abs_path(&path)],
changes: HashMap::new(),
changes: Arc::new(HashMap::new()),
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
@@ -110,9 +111,8 @@ async fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() {
permissions_preapproved: false,
};
let payload = runtime
.permission_request_payload(&req)
.expect("permission request payload");
let payload =
ApplyPatchRuntime::build_approval_action(&req, "call-1").permission_request_payload();
assert_eq!(payload.tool_name.name(), "apply_patch");
assert_eq!(
@@ -136,7 +136,7 @@ async fn approval_keys_include_environment_id() {
turn_environment: test_turn_environment("remote"),
action: ApplyPatchAction::new_add_for_test(&path_uri, "hello".to_string()),
file_paths: vec![path_uri.clone()],
changes: HashMap::new(),
changes: Arc::new(HashMap::new()),
exec_approval_requirement: ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
@@ -145,7 +145,10 @@ async fn approval_keys_include_environment_id() {
permissions_preapproved: false,
};
let keys = runtime.approval_keys(&req);
let keys = runtime
.approval_action(&req, "call-1")
.expect("build approval action")
.cache_keys();
assert_eq!(
serde_json::to_value(&keys).expect("serialize approval keys"),
@@ -171,7 +174,7 @@ async fn sandbox_cwd_uses_patch_action_cwd() {
"hello".to_string(),
),
file_paths: vec![PathUri::from_abs_path(&path)],
changes: HashMap::new(),
changes: Arc::new(HashMap::new()),
exec_approval_requirement: ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
@@ -202,7 +205,7 @@ async fn file_system_sandbox_context_preserves_executor_workspace_permissions()
"hello".to_string(),
),
file_paths: vec![PathUri::from_abs_path(&path)],
changes: HashMap::new(),
changes: Arc::new(HashMap::new()),
exec_approval_requirement: ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
@@ -275,7 +278,7 @@ async fn no_sandbox_attempt_has_no_file_system_context() {
"hello".to_string(),
),
file_paths: vec![PathUri::from_abs_path(&path)],
changes: HashMap::new(),
changes: Arc::new(HashMap::new()),
exec_approval_requirement: ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
codex-rs/core/src/tools/runtimes/shell.rs
+12 -76
View File
@@ -8,7 +8,6 @@ builds sandbox transform inputs, and runs them under the current SandboxAttempt.
pub(crate) mod unix_escalation;
pub(crate) mod zsh_fork_backend;
use crate::command_canonicalization::canonicalize_command_for_approval;
use crate::exec::ExecCapturePolicy;
use crate::guardian::GuardianNetworkAccessTrigger;
use crate::sandboxing::ExecOptions;
@@ -28,9 +27,7 @@ use crate::tools::runtimes::exec_env_for_sandbox_permissions;
use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot;
use crate::tools::sandboxing::Approvable;
use crate::tools::sandboxing::ApprovalAction;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ExecApprovalRequirement;
use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::Sandboxable;
use crate::tools::sandboxing::ToolCtx;
@@ -38,16 +35,13 @@ use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use crate::tools::sandboxing::managed_network_for_sandbox_permissions;
use crate::tools::sandboxing::sandbox_permissions_preserving_denied_reads;
use crate::tools::sandboxing::with_cached_approval;
use codex_network_proxy::NetworkProxy;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxablePreference;
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use futures::future::BoxFuture;
use std::collections::HashMap;
use tokio_util::sync::CancellationToken;
@@ -93,11 +87,11 @@ pub struct ShellRuntime {
#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)]
pub(crate) struct ApprovalKey {
environment_id: String,
command: Vec<String>,
cwd: PathUri,
sandbox_permissions: SandboxPermissions,
additional_permissions: Option<AdditionalPermissionProfile>,
pub(crate) environment_id: String,
pub(crate) command: Vec<String>,
pub(crate) cwd: PathUri,
pub(crate) sandbox_permissions: SandboxPermissions,
pub(crate) additional_permissions: Option<AdditionalPermissionProfile>,
}
impl ShellRuntime {
@@ -124,75 +118,24 @@ impl Sandboxable for ShellRuntime {
}
impl Approvable<ShellRequest> for ShellRuntime {
type ApprovalKey = ApprovalKey;
fn approval_keys(&self, req: &ShellRequest) -> Vec<Self::ApprovalKey> {
vec![ApprovalKey {
environment_id: req.turn_environment.environment_id.clone(),
command: canonicalize_command_for_approval(&req.command),
cwd: PathUri::from_abs_path(&req.cwd),
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
}]
}
fn start_approval_async<'a>(
&'a mut self,
req: &'a ShellRequest,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision> {
let keys = self.approval_keys(req);
let command = req.command.clone();
let cwd = req.cwd.clone();
let environment_id = Some(req.turn_environment.environment_id.clone());
let reason = ctx
.reasons
.retry
.clone()
.or_else(|| ctx.reasons.approval.clone())
.or_else(|| req.justification.clone());
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
Box::pin(async move {
with_cached_approval(&session.services, "shell", keys, move || async move {
let available_decisions = None;
session
.request_command_approval(
turn,
call_id,
/*approval_id*/ None,
environment_id,
command,
cwd,
reason,
ctx.network_approval_context.clone(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
req.additional_permissions.clone(),
available_decisions,
/*plugin_attribution_override*/ None,
)
.await
})
.await
})
}
fn approval_action(
&self,
req: &ShellRequest,
ctx: &ApprovalCtx<'_>,
call_id: &str,
) -> std::io::Result<ApprovalAction> {
Ok(ApprovalAction::Shell {
id: ctx.call_id.to_string(),
id: call_id.to_string(),
environment_id: req.turn_environment.environment_id.clone(),
command: req.command.clone(),
hook_command: req.hook_command.clone(),
cwd: PathUri::from_abs_path(&req.cwd),
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
proposed_execpolicy_amendment: req
.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
})
}
@@ -200,13 +143,6 @@ impl Approvable<ShellRequest> for ShellRuntime {
Some(req.exec_approval_requirement.clone())
}
fn permission_request_payload(&self, req: &ShellRequest) -> Option<PermissionRequestPayload> {
Some(PermissionRequestPayload::bash(
req.hook_command.clone(),
req.justification.clone(),
))
}
fn sandbox_permissions(&self, req: &ShellRequest) -> SandboxPermissions {
req.sandbox_permissions
}
codex-rs/core/src/tools/runtimes/shell_tests.rs
+11 -4
View File
@@ -1,6 +1,7 @@
use super::*;
use crate::config::PermissionProfileSnapshot;
use crate::session::turn_context::EnvironmentConfig;
use crate::tools::approvals::ApprovalCacheKey;
use codex_exec_server::Environment;
use codex_protocol::models::PermissionProfile;
use codex_utils_path_uri::PathUri;
@@ -45,19 +46,25 @@ async fn approval_key_uses_path_uri_and_includes_environment_id() {
},
};
let runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic);
let original_key = runtime.approval_keys(&request);
let original_key = runtime
.approval_action(&request, "call-1")
.expect("build approval action")
.cache_keys();
assert_eq!(
original_key,
vec![ApprovalKey {
vec![ApprovalCacheKey::Shell(ApprovalKey {
environment_id: "remote".to_string(),
command: request.command.clone(),
cwd: PathUri::from_abs_path(&cwd),
sandbox_permissions: request.sandbox_permissions,
additional_permissions: request.additional_permissions.clone(),
}]
})]
);
request.turn_environment.environment_id = "other".to_string();
let other_key = runtime.approval_keys(&request);
let other_key = runtime
.approval_action(&request, "call-1")
.expect("build approval action")
.cache_keys();
assert_ne!(original_key, other_key);
}
codex-rs/core/src/tools/runtimes/unified_exec.rs
+15 -86
View File
@@ -4,7 +4,6 @@ Runtime: unified exec
Handles approval + sandbox orchestration for unified exec requests, delegating to
the process manager to spawn PTYs once an ExecRequest is prepared.
*/
use crate::command_canonicalization::canonicalize_command_for_approval;
use crate::exec::ExecCapturePolicy;
use crate::exec::ExecExpiration;
use crate::guardian::GUARDIAN_REVIEW_TIMEOUT;
@@ -27,9 +26,7 @@ use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot;
use crate::tools::runtimes::shell::zsh_fork_backend;
use crate::tools::sandboxing::Approvable;
use crate::tools::sandboxing::ApprovalAction;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ExecApprovalRequirement;
use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::Sandboxable;
use crate::tools::sandboxing::ToolCtx;
@@ -37,7 +34,6 @@ use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use crate::tools::sandboxing::managed_network_for_sandbox_permissions;
use crate::tools::sandboxing::sandbox_permissions_preserving_denied_reads;
use crate::tools::sandboxing::with_cached_approval;
use crate::unified_exec::NoopSpawnLifecycle;
use crate::unified_exec::UnifiedExecError;
use crate::unified_exec::UnifiedExecProcess;
@@ -47,18 +43,15 @@ use codex_network_proxy::NetworkProxy;
use codex_protocol::error::CodexErr;
use codex_protocol::error::SandboxErr;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxCommand;
use codex_sandboxing::SandboxablePreference;
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
use codex_tools::UnifiedExecShellMode;
use codex_utils_path_uri::PathUri;
use futures::future::BoxFuture;
use std::collections::HashMap;
use std::io;
use std::time::Duration;
use tokio_util::sync::CancellationToken;
use tracing::error;
// Allow 5s for Guardian cleanup and 5s for controller processing after review.
const REMOTE_NETWORK_POLICY_DECISION_MARGIN: Duration = Duration::from_secs(10);
@@ -160,85 +153,25 @@ impl Sandboxable for UnifiedExecRuntime<'_> {
}
impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
type ApprovalKey = UnifiedExecApprovalKey;
fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec<Self::ApprovalKey> {
vec![UnifiedExecApprovalKey {
environment_id: req.turn_environment.environment_id.clone(),
command: canonicalize_command_for_approval(&req.command),
cwd: req.cwd.clone(),
tty: req.tty,
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
}]
}
fn start_approval_async<'b>(
&'b mut self,
req: &'b UnifiedExecRequest,
ctx: ApprovalCtx<'b>,
) -> BoxFuture<'b, ReviewDecision> {
let keys = self.approval_keys(req);
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
let command = req.command.clone();
let environment_id = Some(req.turn_environment.environment_id.clone());
let reason = ctx
.reasons
.retry
.clone()
.or_else(|| ctx.reasons.approval.clone())
.or_else(|| req.justification.clone());
Box::pin(async move {
let native_cwd = match req.cwd.to_abs_path() {
Ok(c) => c,
Err(e) => {
// TODO(anp) make sandboxing work for foreign OSes, in the meantime this should
// be impossible for single-OS app-servers
error!(cwd = %req.cwd, ?e, "got non-native path in start_approval_async");
return ReviewDecision::Abort;
}
};
with_cached_approval(&session.services, "unified_exec", keys, || async move {
let available_decisions = None;
session
.request_command_approval(
turn,
call_id,
/*approval_id*/ None,
environment_id,
command,
native_cwd,
reason,
ctx.network_approval_context.clone(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
req.additional_permissions.clone(),
available_decisions,
/*plugin_attribution_override*/ None,
)
.await
})
.await
})
}
fn approval_action(
&self,
req: &UnifiedExecRequest,
ctx: &ApprovalCtx<'_>,
call_id: &str,
) -> std::io::Result<ApprovalAction> {
Ok(ApprovalAction::ExecCommand {
id: ctx.call_id.to_string(),
id: call_id.to_string(),
environment_id: req.turn_environment.environment_id.clone(),
command: req.command.clone(),
hook_command: req.hook_command.clone(),
cwd: req.cwd.clone(),
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
tty: req.tty,
proposed_execpolicy_amendment: req
.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
})
}
@@ -249,16 +182,6 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
Some(req.exec_approval_requirement.clone())
}
fn permission_request_payload(
&self,
req: &UnifiedExecRequest,
) -> Option<PermissionRequestPayload> {
Some(PermissionRequestPayload::bash(
req.hook_command.clone(),
req.justification.clone(),
))
}
fn sandbox_permissions(&self, req: &UnifiedExecRequest) -> SandboxPermissions {
req.sandbox_permissions
}
@@ -618,9 +541,15 @@ mod tests {
},
);
request.turn_environment.environment_id = "remote".to_string();
let original_key = runtime.approval_keys(&request);
let original_key = runtime
.approval_action(&request, "call-1")
.expect("build approval action")
.cache_keys();
request.turn_environment.environment_id = "other".to_string();
let other_key = runtime.approval_keys(&request);
let other_key = runtime
.approval_action(&request, "call-1")
.expect("build approval action")
.cache_keys();
assert_ne!(original_key, other_key);
}
codex-rs/core/src/tools/sandboxing.rs
+2 -39
View File
@@ -1,7 +1,7 @@
//! Shared approvals and sandboxing traits used by tool runtimes.
//!
//! Consolidates the approval flow primitives (`ApprovalDecision`, `ApprovalStore`,
//! `ApprovalCtx`, `Approvable`) together with the sandbox orchestration traits
//! `Approvable`) together with the sandbox orchestration traits
//! and helpers (`Sandboxable`, `ToolRuntime`, `SandboxAttempt`, etc.).
use crate::sandboxing::ExecOptions;
@@ -15,7 +15,6 @@ use crate::tools::network_approval::NetworkApprovalSpec;
use codex_file_system::FileSystemSandboxContext;
use codex_network_proxy::NetworkProxy;
use codex_protocol::approvals::ExecPolicyAmendment;
use codex_protocol::approvals::NetworkApprovalContext;
use codex_protocol::error::CodexErr;
use codex_protocol::permissions::FileSystemSandboxKind;
use codex_protocol::permissions::FileSystemSandboxPolicy;
@@ -30,11 +29,8 @@ use codex_sandboxing::policy_transforms::effective_permission_profile;
use codex_tools::ToolName;
use codex_utils_path_uri::PathUri;
use futures::Future;
use futures::future::BoxFuture;
use serde::Serialize;
use std::collections::HashMap;
use std::fmt::Debug;
use std::hash::Hash;
use std::sync::Arc;
use tokio_util::sync::CancellationToken;
@@ -123,16 +119,6 @@ pub(crate) struct ApprovalRequestReasons {
pub(crate) retry: Option<String>,
}
#[derive(Clone)]
pub(crate) struct ApprovalCtx<'a> {
pub session: &'a Arc<Session>,
pub turn: &'a Arc<TurnContext>,
pub call_id: &'a str,
pub tool_name: &'a ToolName,
pub reasons: ApprovalRequestReasons,
pub network_approval_context: Option<NetworkApprovalContext>,
}
pub(crate) use super::approvals::ApprovalAction;
#[derive(Clone, Debug, PartialEq, Eq)]
@@ -318,17 +304,6 @@ pub(crate) fn managed_network_for_sandbox_permissions(
}
pub(crate) trait Approvable<Req> {
type ApprovalKey: Hash + Eq + Clone + Debug + Serialize;
// In most cases (shell, unified_exec), a request will have a single approval key.
//
// However, apply_patch needs session "Allow, don't ask again" semantics that
// apply to multiple atomic targets (e.g., apply_patch approves per file path). Returning
// a list of keys lets the runtime treat the request as approved-for-session only if
// *all* keys are already approved, while still caching approvals per-key so future
// requests touching a subset can be auto-approved.
fn approval_keys(&self, req: &Req) -> Vec<Self::ApprovalKey>;
/// Return per-request sandbox permissions for first-attempt sandbox
/// selection. Most tools use the ambient sandbox policy unchanged.
fn sandbox_permissions(&self, _req: &Req) -> SandboxPermissions {
@@ -349,12 +324,6 @@ pub(crate) trait Approvable<Req> {
None
}
/// Return hook input for approval-time policy hooks when this runtime wants
/// hook evaluation to run before guardian or user approval.
fn permission_request_payload(&self, _req: &Req) -> Option<PermissionRequestPayload> {
None
}
/// Decide we can request an approval for no-sandbox execution.
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
match policy {
@@ -365,13 +334,7 @@ pub(crate) trait Approvable<Req> {
}
}
fn start_approval_async<'a>(
&'a mut self,
req: &'a Req,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision>;
fn approval_action(&self, req: &Req, ctx: &ApprovalCtx<'_>) -> std::io::Result<ApprovalAction>;
fn approval_action(&self, req: &Req, call_id: &str) -> std::io::Result<ApprovalAction>;
}
pub(crate) trait Sandboxable {