Skip to content

chore(approvals) consolidate guardian calls for shell tools #31267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dylan-hurd-oai merged 1 commit into from
Jul 6, 2026
Merged

chore(approvals) consolidate guardian calls for shell tools #31267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions codex-rs/core/src/session/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,22 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an
) -> futures::future::BoxFuture<'a, ReviewDecision> {
Box::pin(async { ReviewDecision::Approved })
}

fn approval_action(
&self,
_req: &(),
ctx: &crate::tools::sandboxing::ApprovalCtx<'_>,
) -> std::io::Result<crate::tools::sandboxing::ApprovalAction> {
Ok(crate::tools::sandboxing::ApprovalAction::Shell {
id: ctx.call_id.to_string(),
command: Vec::new(),
#[allow(deprecated)]
cwd: ctx.turn.cwd.clone(),
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
additional_permissions: None,
justification: None,
})
}
}

impl crate::tools::sandboxing::Sandboxable for ProbeToolRuntime {
Expand Down
22 changes: 21 additions & 1 deletion codex-rs/core/src/tools/orchestrator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ caching).
use crate::guardian::guardian_rejection_message;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::guardian::routes_approval_to_guardian;
use crate::hook_runtime::run_permission_request_hooks;
use crate::network_policy_decision::network_approval_context_from_payload;
Expand Down Expand Up @@ -563,7 +564,26 @@ impl ToolOrchestrator {
} else {
ToolDecisionSource::User
};
let decision = tool.start_approval_async(req, approval_ctx).await;
let decision = if let Some(review_id) = approval_ctx.guardian_review_id.clone() {
match tool.approval_action(req, &approval_ctx) {
Ok(action) => {
review_approval_request(
approval_ctx.session,
approval_ctx.turn,
review_id,
action,
approval_ctx.retry_reason.clone(),
)
.await
}
Err(err) => {
tracing::error!(%err, "failed to build guardian approval action");
ReviewDecision::Abort
}
}
} else {
tool.start_approval_async(req, approval_ctx).await
};
let tool_name = flat_tool_name(&tool_ctx.tool_name);
otel.tool_decision(
tool_name.as_ref(),
Expand Down
30 changes: 11 additions & 19 deletions codex-rs/core/src/tools/runtimes/apply_patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@
//! selected turn environment filesystem for both local and remote turns, with
//! sandboxing enforced by the explicit filesystem sandbox context.
use crate::exec::is_likely_sandbox_denied;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::review_approval_request;
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;
Expand Down Expand Up @@ -77,15 +76,15 @@ impl ApplyPatchRuntime {
fn build_guardian_review_request(
req: &ApplyPatchRequest,
call_id: &str,
) -> std::io::Result<GuardianApprovalRequest> {
) -> std::io::Result<ApprovalAction> {
// TODO(anp): Remove this conversion once the guardian API supports PathUri.
let cwd = req.action.cwd.to_abs_path()?;
let files = req
.file_paths
.iter()
.map(PathUri::to_abs_path)
.collect::<std::io::Result<Vec<_>>>()?;
Ok(GuardianApprovalRequest::ApplyPatch {
Ok(ApprovalAction::ApplyPatch {
id: call_id.to_string(),
cwd,
files,
Expand Down Expand Up @@ -152,22 +151,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
let retry_reason = ctx.retry_reason.clone();
let approval_keys = self.approval_keys(req);
let changes = req.changes.clone();
let guardian_review_id = ctx.guardian_review_id.clone();
Box::pin(async move {
if let Some(review_id) = guardian_review_id {
let action = match ApplyPatchRuntime::build_guardian_review_request(
req,
ctx.call_id,
) {
Ok(action) => action,
Err(err) => {
tracing::error!(cwd = %req.action.cwd, %err, "guardian apply_patch cwd is not host-native");
return ReviewDecision::Abort;
}
};
return review_approval_request(session, turn, review_id, action, retry_reason)
.await;
}
if req.permissions_preapproved && retry_reason.is_none() {
return ReviewDecision::Approved;
}
Expand Down Expand Up @@ -201,6 +185,14 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
})
}

fn approval_action(
&self,
req: &ApplyPatchRequest,
ctx: &ApprovalCtx<'_>,
) -> std::io::Result<ApprovalAction> {
ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id)
}

fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
match policy {
AskForApproval::Never => false,
Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core/src/tools/runtimes/apply_patch_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ async fn guardian_review_request_includes_patch_context() {

assert_eq!(
guardian_request,
GuardianApprovalRequest::ApplyPatch {
ApprovalAction::ApplyPatch {
id: "call-1".to_string(),
cwd: expected_cwd,
files: vec![path],
Expand Down
42 changes: 20 additions & 22 deletions codex-rs/core/src/tools/runtimes/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ pub(crate) mod zsh_fork_backend;

use crate::command_canonicalization::canonicalize_command_for_approval;
use crate::exec::ExecCapturePolicy;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::GuardianNetworkAccessTrigger;
use crate::guardian::review_approval_request;
use crate::sandboxing::ExecOptions;
use crate::sandboxing::SandboxPermissions;
use crate::sandboxing::execute_env;
Expand All @@ -29,6 +27,7 @@ use crate::tools::runtimes::disable_powershell_profile_for_elevated_windows_sand
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;
Expand Down Expand Up @@ -145,30 +144,14 @@ impl Approvable<ShellRequest> for ShellRuntime {
let command = req.command.clone();
let cwd = req.cwd.clone();
let environment_id = Some(req.turn_environment.environment_id.clone());
let retry_reason = ctx.retry_reason.clone();
let reason = retry_reason.clone().or_else(|| req.justification.clone());
let reason = ctx
.retry_reason
.clone()
.or_else(|| req.justification.clone());
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
let guardian_review_id = ctx.guardian_review_id.clone();
Box::pin(async move {
if let Some(review_id) = guardian_review_id {
return review_approval_request(
session,
turn,
review_id,
GuardianApprovalRequest::Shell {
id: call_id,
command,
cwd: cwd.clone(),
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
},
retry_reason,
)
.await;
}
with_cached_approval(&session.services, "shell", keys, move || async move {
let available_decisions = None;
session
Expand All @@ -193,6 +176,21 @@ impl Approvable<ShellRequest> for ShellRuntime {
})
}

fn approval_action(
&self,
req: &ShellRequest,
ctx: &ApprovalCtx<'_>,
) -> std::io::Result<ApprovalAction> {
Ok(ApprovalAction::Shell {
id: ctx.call_id.to_string(),
command: req.command.clone(),
cwd: req.cwd.clone(),
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
})
}

fn exec_approval_requirement(&self, req: &ShellRequest) -> Option<ExecApprovalRequirement> {
Some(req.exec_approval_requirement.clone())
}
Expand Down
44 changes: 21 additions & 23 deletions codex-rs/core/src/tools/runtimes/unified_exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ 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::GuardianApprovalRequest;
use crate::guardian::GuardianNetworkAccessTrigger;
use crate::guardian::review_approval_request;
use crate::sandboxing::ExecOptions;
use crate::sandboxing::ExecServerEnvConfig;
use crate::sandboxing::SandboxPermissions;
Expand All @@ -26,6 +24,7 @@ use crate::tools::runtimes::exec_env_for_sandbox_permissions;
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;
Expand Down Expand Up @@ -179,9 +178,10 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
let call_id = ctx.call_id.to_string();
let command = req.command.clone();
let environment_id = Some(req.turn_environment.environment_id.clone());
let retry_reason = ctx.retry_reason.clone();
let reason = retry_reason.clone().or_else(|| req.justification.clone());
let guardian_review_id = ctx.guardian_review_id.clone();
let reason = ctx
.retry_reason
.clone()
.or_else(|| req.justification.clone());
Box::pin(async move {
let native_cwd = match req.cwd.to_abs_path() {
Ok(c) => c,
Expand All @@ -192,24 +192,6 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
return ReviewDecision::Abort;
}
};
if let Some(review_id) = guardian_review_id {
return review_approval_request(
session,
turn,
review_id,
GuardianApprovalRequest::ExecCommand {
id: call_id,
command,
cwd: native_cwd.clone(),
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
tty: req.tty,
},
retry_reason,
)
.await;
}
with_cached_approval(&session.services, "unified_exec", keys, || async move {
let available_decisions = None;
session
Expand All @@ -234,6 +216,22 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
})
}

fn approval_action(
&self,
req: &UnifiedExecRequest,
ctx: &ApprovalCtx<'_>,
) -> std::io::Result<ApprovalAction> {
Ok(ApprovalAction::ExecCommand {

@pakrym-oai pakrym-oai Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO should be the same action as shell but it's outside of this PR scope.

id: ctx.call_id.to_string(),
command: req.command.clone(),
cwd: req.cwd.to_abs_path()?,
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
tty: req.tty,
})
}

fn exec_approval_requirement(
&self,
req: &UnifiedExecRequest,
Expand Down
4 changes: 4 additions & 0 deletions codex-rs/core/src/tools/sandboxing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ pub(crate) struct ApprovalCtx<'a> {
pub network_approval_context: Option<NetworkApprovalContext>,
}

pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest;

#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) struct PermissionRequestPayload {
pub tool_name: HookToolName,
Expand Down Expand Up @@ -368,6 +370,8 @@ pub(crate) trait Approvable<Req> {
req: &'a Req,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision>;

fn approval_action(&self, req: &Req, ctx: &ApprovalCtx<'_>) -> std::io::Result<ApprovalAction>;
}

pub(crate) trait Sandboxable {
Expand Down
Loading
Loading