mirror of
https://github.com/openai/codex.git
synced 2026-10-31 19:28:49 +00:00
refactor(approvals): introduce neutral approval action
This commit is contained in:
@@ -1053,7 +1053,7 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an
|
||||
id: ctx.call_id.to_string(),
|
||||
command: Vec::new(),
|
||||
#[allow(deprecated)]
|
||||
cwd: ctx.turn.cwd.clone(),
|
||||
cwd: PathUri::from_abs_path(&ctx.turn.cwd),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: None,
|
||||
|
||||
@@ -8,6 +8,7 @@ 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;
|
||||
@@ -18,10 +19,90 @@ use crate::tools::sandboxing::ToolRuntime;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_hooks::PermissionRequestDecision;
|
||||
use codex_otel::ToolDecisionSource;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_protocol::protocol::NetworkPolicyRuleAction;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
|
||||
pub(super) type ApprovalAction = crate::guardian::GuardianApprovalRequest;
|
||||
#[derive(Clone, Debug, PartialEq)]
|
||||
pub(crate) enum ApprovalAction {
|
||||
Shell {
|
||||
id: String,
|
||||
command: Vec<String>,
|
||||
cwd: PathUri,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions: Option<AdditionalPermissionProfile>,
|
||||
justification: Option<String>,
|
||||
},
|
||||
ExecCommand {
|
||||
id: String,
|
||||
command: Vec<String>,
|
||||
cwd: PathUri,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions: Option<AdditionalPermissionProfile>,
|
||||
justification: Option<String>,
|
||||
tty: bool,
|
||||
},
|
||||
ApplyPatch {
|
||||
id: String,
|
||||
cwd: PathUri,
|
||||
files: Vec<PathUri>,
|
||||
patch: String,
|
||||
},
|
||||
}
|
||||
|
||||
impl ApprovalAction {
|
||||
fn into_guardian_request(self) -> std::io::Result<crate::guardian::GuardianApprovalRequest> {
|
||||
Ok(match self {
|
||||
Self::Shell {
|
||||
id,
|
||||
command,
|
||||
cwd,
|
||||
sandbox_permissions,
|
||||
additional_permissions,
|
||||
justification,
|
||||
} => crate::guardian::GuardianApprovalRequest::Shell {
|
||||
id,
|
||||
command,
|
||||
cwd: cwd.to_abs_path()?,
|
||||
sandbox_permissions,
|
||||
additional_permissions,
|
||||
justification,
|
||||
},
|
||||
Self::ExecCommand {
|
||||
id,
|
||||
command,
|
||||
cwd,
|
||||
sandbox_permissions,
|
||||
additional_permissions,
|
||||
justification,
|
||||
tty,
|
||||
} => crate::guardian::GuardianApprovalRequest::ExecCommand {
|
||||
id,
|
||||
command,
|
||||
cwd: cwd.to_abs_path()?,
|
||||
sandbox_permissions,
|
||||
additional_permissions,
|
||||
justification,
|
||||
tty,
|
||||
},
|
||||
Self::ApplyPatch {
|
||||
id,
|
||||
cwd,
|
||||
files,
|
||||
patch,
|
||||
} => crate::guardian::GuardianApprovalRequest::ApplyPatch {
|
||||
id,
|
||||
cwd: cwd.to_abs_path()?,
|
||||
files: files
|
||||
.into_iter()
|
||||
.map(|path| path.to_abs_path())
|
||||
.collect::<std::io::Result<Vec<_>>>()?,
|
||||
patch,
|
||||
},
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub(super) enum ApprovalReviewer {
|
||||
@@ -113,7 +194,10 @@ where
|
||||
let resolution = match reviewer {
|
||||
ApprovalReviewer::Guardian => {
|
||||
let review_id = new_guardian_review_id();
|
||||
let action = match tool.approval_action(req, &ctx) {
|
||||
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");
|
||||
|
||||
@@ -73,23 +73,13 @@ impl ApplyPatchRuntime {
|
||||
&self.committed_delta
|
||||
}
|
||||
|
||||
fn build_guardian_review_request(
|
||||
req: &ApplyPatchRequest,
|
||||
call_id: &str,
|
||||
) -> 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(ApprovalAction::ApplyPatch {
|
||||
fn build_approval_action(req: &ApplyPatchRequest, call_id: &str) -> ApprovalAction {
|
||||
ApprovalAction::ApplyPatch {
|
||||
id: call_id.to_string(),
|
||||
cwd,
|
||||
files,
|
||||
cwd: req.action.cwd.clone(),
|
||||
files: req.file_paths.clone(),
|
||||
patch: req.action.patch.clone(),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
fn file_system_sandbox_context_for_attempt(
|
||||
@@ -190,7 +180,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
req: &ApplyPatchRequest,
|
||||
ctx: &ApprovalCtx<'_>,
|
||||
) -> std::io::Result<ApprovalAction> {
|
||||
ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id)
|
||||
Ok(ApplyPatchRuntime::build_approval_action(req, ctx.call_id))
|
||||
}
|
||||
|
||||
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
|
||||
|
||||
@@ -49,24 +49,17 @@ fn wants_no_sandbox_approval_granular_respects_sandbox_flag() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_request_includes_patch_context() {
|
||||
let path = std::env::temp_dir()
|
||||
.join("guardian-apply-patch-test.txt")
|
||||
.abs();
|
||||
let action =
|
||||
ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&path), "hello".to_string());
|
||||
let expected_cwd = action.cwd.to_abs_path().expect("native patch cwd");
|
||||
async fn approval_action_preserves_patch_path_uris() {
|
||||
let path = PathUri::parse("file:///C:/workspace/guardian-apply-patch-test.txt")
|
||||
.expect("valid foreign path URI");
|
||||
let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string());
|
||||
let expected_cwd = action.cwd.clone();
|
||||
let expected_patch = action.patch.clone();
|
||||
let request = ApplyPatchRequest {
|
||||
turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID),
|
||||
action,
|
||||
file_paths: vec![PathUri::from_abs_path(&path)],
|
||||
changes: HashMap::from([(
|
||||
path.to_path_buf(),
|
||||
FileChange::Add {
|
||||
content: "hello".to_string(),
|
||||
},
|
||||
)]),
|
||||
file_paths: vec![path.clone()],
|
||||
changes: HashMap::new(),
|
||||
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
@@ -75,11 +68,10 @@ async fn guardian_review_request_includes_patch_context() {
|
||||
permissions_preapproved: false,
|
||||
};
|
||||
|
||||
let guardian_request = ApplyPatchRuntime::build_guardian_review_request(&request, "call-1")
|
||||
.expect("native guardian request cwd");
|
||||
let approval_action = ApplyPatchRuntime::build_approval_action(&request, "call-1");
|
||||
|
||||
assert_eq!(
|
||||
guardian_request,
|
||||
approval_action,
|
||||
ApprovalAction::ApplyPatch {
|
||||
id: "call-1".to_string(),
|
||||
cwd: expected_cwd,
|
||||
|
||||
@@ -46,6 +46,7 @@ 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;
|
||||
@@ -184,7 +185,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
Ok(ApprovalAction::Shell {
|
||||
id: ctx.call_id.to_string(),
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
cwd: PathUri::from_abs_path(&req.cwd),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: req.justification.clone(),
|
||||
|
||||
@@ -224,7 +224,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
Ok(ApprovalAction::ExecCommand {
|
||||
id: ctx.call_id.to_string(),
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.to_abs_path()?,
|
||||
cwd: req.cwd.clone(),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: req.justification.clone(),
|
||||
|
||||
@@ -126,7 +126,7 @@ pub(crate) struct ApprovalCtx<'a> {
|
||||
pub network_approval_context: Option<NetworkApprovalContext>,
|
||||
}
|
||||
|
||||
pub(crate) type ApprovalAction = super::approvals::ApprovalAction;
|
||||
pub(crate) use super::approvals::ApprovalAction;
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub(crate) struct PermissionRequestPayload {
|
||||
|
||||
Reference in New Issue
Block a user