mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
refactoring with_escalated_permissions to use SandboxPermissions instead (#7750)
helpful in the future if we want more granularity for requesting escalated permissions: e.g when running in readonly sandbox, model can request to escalate to a sandbox that allows writes
This commit is contained in:
@@ -10,7 +10,6 @@ use crate::exec_policy::create_exec_approval_requirement_for_command;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::is_safe_command::is_known_safe_command;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolOutput;
|
||||
use crate::tools::context::ToolPayload;
|
||||
@@ -35,7 +34,7 @@ impl ShellHandler {
|
||||
cwd: turn_context.resolve_path(params.workdir.clone()),
|
||||
expiration: params.timeout_ms.into(),
|
||||
env: create_env(&turn_context.shell_environment_policy),
|
||||
with_escalated_permissions: params.with_escalated_permissions,
|
||||
sandbox_permissions: params.sandbox_permissions.unwrap_or_default(),
|
||||
justification: params.justification,
|
||||
arg0: None,
|
||||
}
|
||||
@@ -56,7 +55,7 @@ impl ShellCommandHandler {
|
||||
cwd: turn_context.resolve_path(params.workdir.clone()),
|
||||
expiration: params.timeout_ms.into(),
|
||||
env: create_env(&turn_context.shell_environment_policy),
|
||||
with_escalated_permissions: params.with_escalated_permissions,
|
||||
sandbox_permissions: params.sandbox_permissions.unwrap_or_default(),
|
||||
justification: params.justification,
|
||||
arg0: None,
|
||||
}
|
||||
@@ -206,7 +205,9 @@ impl ShellHandler {
|
||||
freeform: bool,
|
||||
) -> Result<ToolOutput, FunctionCallError> {
|
||||
// Approval policy guard for explicit escalation in non-OnRequest modes.
|
||||
if exec_params.with_escalated_permissions.unwrap_or(false)
|
||||
if exec_params
|
||||
.sandbox_permissions
|
||||
.requires_escalated_permissions()
|
||||
&& !matches!(
|
||||
turn.approval_policy,
|
||||
codex_protocol::protocol::AskForApproval::OnRequest
|
||||
@@ -251,7 +252,7 @@ impl ShellHandler {
|
||||
&exec_params.command,
|
||||
turn.approval_policy,
|
||||
&turn.sandbox_policy,
|
||||
SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)),
|
||||
exec_params.sandbox_permissions,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -260,7 +261,7 @@ impl ShellHandler {
|
||||
cwd: exec_params.cwd.clone(),
|
||||
timeout_ms: exec_params.expiration.timeout_ms(),
|
||||
env: exec_params.env.clone(),
|
||||
with_escalated_permissions: exec_params.with_escalated_permissions,
|
||||
sandbox_permissions: exec_params.sandbox_permissions,
|
||||
justification: exec_params.justification.clone(),
|
||||
exec_approval_requirement,
|
||||
};
|
||||
@@ -295,6 +296,7 @@ mod tests {
|
||||
use crate::codex::make_session_and_context;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::is_safe_command::is_known_safe_command;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell::ShellType;
|
||||
use crate::tools::handlers::ShellCommandHandler;
|
||||
@@ -343,7 +345,7 @@ mod tests {
|
||||
let workdir = Some("subdir".to_string());
|
||||
let login = None;
|
||||
let timeout_ms = Some(1234);
|
||||
let with_escalated_permissions = Some(true);
|
||||
let sandbox_permissions = SandboxPermissions::RequireEscalated;
|
||||
let justification = Some("because tests".to_string());
|
||||
|
||||
let expected_command = session.user_shell().derive_exec_args(&command, true);
|
||||
@@ -355,7 +357,7 @@ mod tests {
|
||||
workdir,
|
||||
login,
|
||||
timeout_ms,
|
||||
with_escalated_permissions,
|
||||
sandbox_permissions: Some(sandbox_permissions),
|
||||
justification: justification.clone(),
|
||||
};
|
||||
|
||||
@@ -366,10 +368,7 @@ mod tests {
|
||||
assert_eq!(exec_params.cwd, expected_cwd);
|
||||
assert_eq!(exec_params.env, expected_env);
|
||||
assert_eq!(exec_params.expiration.timeout_ms(), timeout_ms);
|
||||
assert_eq!(
|
||||
exec_params.with_escalated_permissions,
|
||||
with_escalated_permissions
|
||||
);
|
||||
assert_eq!(exec_params.sandbox_permissions, sandbox_permissions);
|
||||
assert_eq!(exec_params.justification, justification);
|
||||
assert_eq!(exec_params.arg0, None);
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ use crate::is_safe_command::is_known_safe_command;
|
||||
use crate::protocol::EventMsg;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
use crate::protocol::TerminalInteractionEvent;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell::get_shell_by_model_provided_path;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
@@ -40,7 +41,7 @@ struct ExecCommandArgs {
|
||||
#[serde(default)]
|
||||
max_output_tokens: Option<usize>,
|
||||
#[serde(default)]
|
||||
with_escalated_permissions: Option<bool>,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
#[serde(default)]
|
||||
justification: Option<String>,
|
||||
}
|
||||
@@ -131,12 +132,12 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
login,
|
||||
yield_time_ms,
|
||||
max_output_tokens,
|
||||
with_escalated_permissions,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
..
|
||||
} = args;
|
||||
|
||||
if with_escalated_permissions.unwrap_or(false)
|
||||
if sandbox_permissions.requires_escalated_permissions()
|
||||
&& !matches!(
|
||||
context.turn.approval_policy,
|
||||
codex_protocol::protocol::AskForApproval::OnRequest
|
||||
@@ -200,7 +201,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
yield_time_ms,
|
||||
max_output_tokens,
|
||||
workdir,
|
||||
with_escalated_permissions,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
},
|
||||
&context,
|
||||
|
||||
@@ -5,6 +5,7 @@ use crate::client_common::tools::ToolSpec;
|
||||
use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::context::SharedTurnDiffTracker;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolPayload;
|
||||
@@ -114,7 +115,7 @@ impl ToolRouter {
|
||||
command: exec.command,
|
||||
workdir: exec.working_directory,
|
||||
timeout_ms: exec.timeout_ms,
|
||||
with_escalated_permissions: None,
|
||||
sandbox_permissions: Some(SandboxPermissions::UseDefault),
|
||||
justification: None,
|
||||
};
|
||||
Ok(Some(ToolCall {
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
use crate::CODEX_APPLY_PATCH_ARG1;
|
||||
use crate::exec::ExecToolCallOutput;
|
||||
use crate::sandboxing::CommandSpec;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::sandboxing::execute_env;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
@@ -70,7 +71,7 @@ impl ApplyPatchRuntime {
|
||||
expiration: req.timeout_ms.into(),
|
||||
// Run apply_patch with a minimal environment for determinism and to avoid leaks.
|
||||
env: HashMap::new(),
|
||||
with_escalated_permissions: None,
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
justification: None,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ small and focused and reuses the orchestrator for approvals + sandbox + retry.
|
||||
*/
|
||||
use crate::exec::ExecExpiration;
|
||||
use crate::sandboxing::CommandSpec;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
@@ -21,7 +22,7 @@ pub(crate) fn build_command_spec(
|
||||
cwd: &Path,
|
||||
env: &HashMap<String, String>,
|
||||
expiration: ExecExpiration,
|
||||
with_escalated_permissions: Option<bool>,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
justification: Option<String>,
|
||||
) -> Result<CommandSpec, ToolError> {
|
||||
let (program, args) = command
|
||||
@@ -33,7 +34,7 @@ pub(crate) fn build_command_spec(
|
||||
cwd: cwd.to_path_buf(),
|
||||
env: env.clone(),
|
||||
expiration,
|
||||
with_escalated_permissions,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ Executes shell requests under the orchestrator: asks for approval when needed,
|
||||
builds a CommandSpec, and runs it under the current SandboxAttempt.
|
||||
*/
|
||||
use crate::exec::ExecToolCallOutput;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::sandboxing::execute_env;
|
||||
use crate::tools::runtimes::build_command_spec;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
@@ -30,7 +31,7 @@ pub struct ShellRequest {
|
||||
pub cwd: PathBuf,
|
||||
pub timeout_ms: Option<u64>,
|
||||
pub env: std::collections::HashMap<String, String>,
|
||||
pub with_escalated_permissions: Option<bool>,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub justification: Option<String>,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
}
|
||||
@@ -51,7 +52,7 @@ pub struct ShellRuntime;
|
||||
pub(crate) struct ApprovalKey {
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
escalated: bool,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
}
|
||||
|
||||
impl ShellRuntime {
|
||||
@@ -84,7 +85,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
ApprovalKey {
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
escalated: req.with_escalated_permissions.unwrap_or(false),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -129,7 +130,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride {
|
||||
if req.with_escalated_permissions.unwrap_or(false)
|
||||
if req.sandbox_permissions.requires_escalated_permissions()
|
||||
|| matches!(
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
@@ -157,7 +158,7 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
|
||||
&req.cwd,
|
||||
&req.env,
|
||||
req.timeout_ms.into(),
|
||||
req.with_escalated_permissions,
|
||||
req.sandbox_permissions,
|
||||
req.justification.clone(),
|
||||
)?;
|
||||
let env = attempt
|
||||
|
||||
@@ -7,6 +7,7 @@ the session manager to spawn PTYs once an ExecEnv is prepared.
|
||||
use crate::error::CodexErr;
|
||||
use crate::error::SandboxErr;
|
||||
use crate::exec::ExecExpiration;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::runtimes::build_command_spec;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
@@ -34,7 +35,7 @@ pub struct UnifiedExecRequest {
|
||||
pub command: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub env: HashMap<String, String>,
|
||||
pub with_escalated_permissions: Option<bool>,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub justification: Option<String>,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
}
|
||||
@@ -52,7 +53,7 @@ impl ProvidesSandboxRetryData for UnifiedExecRequest {
|
||||
pub struct UnifiedExecApprovalKey {
|
||||
pub command: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub escalated: bool,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
}
|
||||
|
||||
pub struct UnifiedExecRuntime<'a> {
|
||||
@@ -64,7 +65,7 @@ impl UnifiedExecRequest {
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
env: HashMap<String, String>,
|
||||
with_escalated_permissions: Option<bool>,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
justification: Option<String>,
|
||||
exec_approval_requirement: ExecApprovalRequirement,
|
||||
) -> Self {
|
||||
@@ -72,7 +73,7 @@ impl UnifiedExecRequest {
|
||||
command,
|
||||
cwd,
|
||||
env,
|
||||
with_escalated_permissions,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
exec_approval_requirement,
|
||||
}
|
||||
@@ -102,7 +103,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
UnifiedExecApprovalKey {
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
escalated: req.with_escalated_permissions.unwrap_or(false),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -150,7 +151,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride {
|
||||
if req.with_escalated_permissions.unwrap_or(false)
|
||||
if req.sandbox_permissions.requires_escalated_permissions()
|
||||
|| matches!(
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
@@ -178,7 +179,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecSession> for UnifiedExecRunt
|
||||
&req.cwd,
|
||||
&req.env,
|
||||
ExecExpiration::DefaultTimeout,
|
||||
req.with_escalated_permissions,
|
||||
req.sandbox_permissions,
|
||||
req.justification.clone(),
|
||||
)
|
||||
.map_err(|_| ToolError::Rejected("missing command line for PTY".to_string()))?;
|
||||
|
||||
@@ -174,10 +174,10 @@ fn create_exec_command_tool() -> ToolSpec {
|
||||
},
|
||||
);
|
||||
properties.insert(
|
||||
"with_escalated_permissions".to_string(),
|
||||
JsonSchema::Boolean {
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
"Whether to request escalated permissions. Set to true if command needs to be run without sandbox restrictions"
|
||||
"Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\"."
|
||||
.to_string(),
|
||||
),
|
||||
},
|
||||
@@ -186,7 +186,7 @@ fn create_exec_command_tool() -> ToolSpec {
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
"Only set if with_escalated_permissions is true. 1-sentence explanation of why we want to run this command."
|
||||
"Only set if sandbox_permissions is \"require_escalated\". 1-sentence explanation of why we want to run this command."
|
||||
.to_string(),
|
||||
),
|
||||
},
|
||||
@@ -274,15 +274,15 @@ fn create_shell_tool() -> ToolSpec {
|
||||
);
|
||||
|
||||
properties.insert(
|
||||
"with_escalated_permissions".to_string(),
|
||||
JsonSchema::Boolean {
|
||||
description: Some("Whether to request escalated permissions. Set to true if command needs to be run without sandbox restrictions".to_string()),
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\".".to_string()),
|
||||
},
|
||||
);
|
||||
properties.insert(
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Only set if with_escalated_permissions is true. 1-sentence explanation of why we want to run this command.".to_string()),
|
||||
description: Some("Only set if sandbox_permissions is \"require_escalated\". 1-sentence explanation of why we want to run this command.".to_string()),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -347,15 +347,15 @@ fn create_shell_command_tool() -> ToolSpec {
|
||||
},
|
||||
);
|
||||
properties.insert(
|
||||
"with_escalated_permissions".to_string(),
|
||||
JsonSchema::Boolean {
|
||||
description: Some("Whether to request escalated permissions. Set to true if command needs to be run without sandbox restrictions".to_string()),
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\".".to_string()),
|
||||
},
|
||||
);
|
||||
properties.insert(
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Only set if with_escalated_permissions is true. 1-sentence explanation of why we want to run this command.".to_string()),
|
||||
description: Some("Only set if sandbox_permissions is \"require_escalated\". 1-sentence explanation of why we want to run this command.".to_string()),
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user