openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor

Browse Source
This commit is contained in:
kevin zhao
2025-11-20 13:17:58 -05:00
parent 219aab7720
commit a2a9b77e5c
8 changed files with 88 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/codex.rs
+1
View File
@@ -881,6 +881,7 @@ impl Session {
/// The request is keyed by `sub_id`/`call_id` so matching responses are delivered
/// to the correct in-flight turn. If the task is aborted, this returns the
/// default `ReviewDecision` (`Denied`).
#[allow(clippy::too_many_arguments)]
pub async fn request_command_approval(
&self,
turn_context: &TurnContext,
codex-rs/core/src/exec_policy.rs
+48 -45
View File
@@ -27,12 +27,6 @@ const POLICY_DIR_NAME: &str = "policy";
const POLICY_EXTENSION: &str = "codexpolicy";
const DEFAULT_POLICY_FILE: &str = "default.codexpolicy";
#[derive(Debug, PartialEq)]
pub(crate) struct CommandApprovalOutcome {
pub requirement: ApprovalRequirement,
pub allow_prefix: Option<Vec<String>>,
}
#[derive(Debug, Error)]
pub enum ExecPolicyError {
#[error("failed to read execpolicy files from {dir}: {source}")]
@@ -139,6 +133,7 @@ fn requirement_from_decision(
} else {
ApprovalRequirement::NeedsApproval {
reason: Some(reason),
allow_prefix: None,
}
}
}
@@ -146,33 +141,36 @@ fn requirement_from_decision(
}
}
/// Attach an allow-prefix option when a single plain command needs approval without
/// any matching policy rule. We only surface the prefix opt-in when execpolicy did
/// not already drive the decision (NoMatch) and when the command is a single
/// unrolled command (multi-part scripts shouldn’t be whitelisted via prefix).
fn allow_prefix_if_applicable(
commands: &[Vec<String>],
evaluation: &Evaluation,
requirement: &ApprovalRequirement,
) -> Option<Vec<String>> {
if !matches!(requirement, ApprovalRequirement::NeedsApproval { .. }) {
return None;
requirement: ApprovalRequirement,
) -> ApprovalRequirement {
match requirement {
ApprovalRequirement::NeedsApproval {
reason,
allow_prefix: None,
} if matches!(evaluation, Evaluation::NoMatch) && commands.len() == 1 => {
ApprovalRequirement::NeedsApproval {
reason,
allow_prefix: Some(commands[0].clone()),
}
}
other => other,
}
if !matches!(evaluation, Evaluation::NoMatch) {
return None;
}
let [single_command] = commands else {
return None;
};
Some(single_command.clone())
}
pub(crate) fn command_approval_outcome_for_command(
pub(crate) fn command_approval_requirement_for_command(
policy: &Policy,
command: &[String],
approval_policy: AskForApproval,
sandbox_policy: &SandboxPolicy,
sandbox_permissions: SandboxPermissions,
) -> CommandApprovalOutcome {
) -> ApprovalRequirement {
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
let evaluation = policy.check_multiple(commands.iter());
@@ -185,19 +183,17 @@ pub(crate) fn command_approval_outcome_for_command(
command,
sandbox_permissions,
) {
ApprovalRequirement::NeedsApproval { reason: None }
ApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
}
} else {
ApprovalRequirement::Skip
}
}
};
let allow_prefix = allow_prefix_if_applicable(&commands, &evaluation, &requirement);
CommandApprovalOutcome {
requirement,
allow_prefix,
}
allow_prefix_if_applicable(&commands, &evaluation, requirement)
}
#[cfg(test)]
@@ -225,14 +221,13 @@ pub(crate) fn create_approval_requirement_for_command(
sandbox_policy: &SandboxPolicy,
sandbox_permissions: SandboxPermissions,
) -> ApprovalRequirement {
command_approval_outcome_for_command(
command_approval_requirement_for_command(
policy,
command,
approval_policy,
sandbox_policy,
sandbox_permissions,
)
.requirement
}
async fn collect_policy_files(dir: &Path) -> Result<Vec<PathBuf>, ExecPolicyError> {
@@ -412,7 +407,8 @@ prefix_rule(pattern=["rm"], decision="forbidden")
assert_eq!(
requirement,
ApprovalRequirement::NeedsApproval {
reason: Some(PROMPT_REASON.to_string())
reason: Some(PROMPT_REASON.to_string()),
allow_prefix: None,
}
);
}
@@ -458,7 +454,10 @@ prefix_rule(pattern=["rm"], decision="forbidden")
assert_eq!(
requirement,
ApprovalRequirement::NeedsApproval { reason: None }
ApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: Some(command)
}
);
}
@@ -467,7 +466,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
let command = vec!["python".to_string()];
let empty_policy = Policy::empty();
let outcome = command_approval_outcome_for_command(
let requirement = command_approval_requirement_for_command(
&empty_policy,
&command,
AskForApproval::UnlessTrusted,
@@ -476,10 +475,12 @@ prefix_rule(pattern=["rm"], decision="forbidden")
);
assert_eq!(
outcome.requirement,
ApprovalRequirement::NeedsApproval { reason: None }
requirement,
ApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: Some(command)
}
);
assert_eq!(outcome.allow_prefix, Some(command));
}
#[test]
@@ -492,7 +493,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
let policy = parser.build();
let command = vec!["rm".to_string()];
let outcome = command_approval_outcome_for_command(
let requirement = command_approval_requirement_for_command(
&policy,
&command,
AskForApproval::OnRequest,
@@ -501,12 +502,12 @@ prefix_rule(pattern=["rm"], decision="forbidden")
);
assert_eq!(
outcome.requirement,
requirement,
ApprovalRequirement::NeedsApproval {
reason: Some(PROMPT_REASON.to_string())
reason: Some(PROMPT_REASON.to_string()),
allow_prefix: None,
}
);
assert_eq!(outcome.allow_prefix, None);
}
#[test]
@@ -516,7 +517,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
"-lc".to_string(),
"python && echo ok".to_string(),
];
let outcome = command_approval_outcome_for_command(
let requirement = command_approval_requirement_for_command(
&Policy::empty(),
&command,
AskForApproval::UnlessTrusted,
@@ -525,9 +526,11 @@ prefix_rule(pattern=["rm"], decision="forbidden")
);
assert_eq!(
outcome.requirement,
ApprovalRequirement::NeedsApproval { reason: None }
requirement,
ApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
}
);
assert_eq!(outcome.allow_prefix, None);
}
}
codex-rs/core/src/tools/handlers/shell.rs
+8 -10
View File
@@ -9,7 +9,7 @@ use crate::apply_patch::convert_apply_patch_to_protocol;
use crate::codex::TurnContext;
use crate::exec::ExecParams;
use crate::exec_env::create_env;
use crate::exec_policy::command_approval_outcome_for_command;
use crate::exec_policy::command_approval_requirement_for_command;
use crate::function_tool::FunctionCallError;
use crate::is_safe_command::is_known_safe_command;
use crate::protocol::ExecCommandSource;
@@ -298,13 +298,6 @@ impl ShellHandler {
emitter.begin(event_ctx).await;
let exec_policy = session.current_exec_policy().await;
let approval_outcome = command_approval_outcome_for_command(
exec_policy.as_ref(),
&exec_params.command,
turn.approval_policy,
&turn.sandbox_policy,
SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)),
);
let req = ShellRequest {
command: exec_params.command.clone(),
cwd: exec_params.cwd.clone(),
@@ -312,8 +305,13 @@ impl ShellHandler {
env: exec_params.env.clone(),
with_escalated_permissions: exec_params.with_escalated_permissions,
justification: exec_params.justification.clone(),
approval_requirement: approval_outcome.requirement,
approval_allow_prefix: approval_outcome.allow_prefix,
approval_requirement: command_approval_requirement_for_command(
exec_policy.as_ref(),
&exec_params.command,
turn.approval_policy,
&turn.sandbox_policy,
SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)),
),
};
let mut orchestrator = ToolOrchestrator::new();
let mut runtime = ShellRuntime::new();
codex-rs/core/src/tools/orchestrator.rs
+1 -1
View File
@@ -63,7 +63,7 @@ impl ToolOrchestrator {
ApprovalRequirement::Forbidden { reason } => {
return Err(ToolError::Rejected(reason));
}
ApprovalRequirement::NeedsApproval { reason } => {
ApprovalRequirement::NeedsApproval { reason, .. } => {
let mut risk = None;
if let Some(metadata) = req.sandbox_retry_data() {
codex-rs/core/src/tools/runtimes/shell.rs
+1 -2
View File
@@ -32,7 +32,6 @@ pub struct ShellRequest {
pub with_escalated_permissions: Option<bool>,
pub justification: Option<String>,
pub approval_requirement: ApprovalRequirement,
pub approval_allow_prefix: Option<Vec<String>>,
}
impl ProvidesSandboxRetryData for ShellRequest {
@@ -101,7 +100,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
.clone()
.or_else(|| req.justification.clone());
let risk = ctx.risk.clone();
let allow_prefix = req.approval_allow_prefix.clone();
let allow_prefix = req.approval_requirement.allow_prefix().cloned();
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
codex-rs/core/src/tools/runtimes/unified_exec.rs
+1 -4
View File
@@ -35,7 +35,6 @@ pub struct UnifiedExecRequest {
pub with_escalated_permissions: Option<bool>,
pub justification: Option<String>,
pub approval_requirement: ApprovalRequirement,
pub approval_allow_prefix: Option<Vec<String>>,
}
impl ProvidesSandboxRetryData for UnifiedExecRequest {
@@ -66,7 +65,6 @@ impl UnifiedExecRequest {
with_escalated_permissions: Option<bool>,
justification: Option<String>,
approval_requirement: ApprovalRequirement,
approval_allow_prefix: Option<Vec<String>>,
) -> Self {
Self {
command,
@@ -75,7 +73,6 @@ impl UnifiedExecRequest {
with_escalated_permissions,
justification,
approval_requirement,
approval_allow_prefix,
}
}
}
@@ -123,7 +120,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
.clone()
.or_else(|| req.justification.clone());
let risk = ctx.risk.clone();
let allow_prefix = req.approval_allow_prefix.clone();
let allow_prefix = req.approval_requirement.allow_prefix().cloned();
Box::pin(async move {
with_cached_approval(&session.services, key, || async move {
session
codex-rs/core/src/tools/sandboxing.rs
+20 -2
View File
@@ -92,11 +92,26 @@ pub(crate) enum ApprovalRequirement {
/// No approval required for this tool call
Skip,
/// Approval required for this tool call
NeedsApproval { reason: Option<String> },
NeedsApproval {
reason: Option<String>,
allow_prefix: Option<Vec<String>>,
},
/// Execution forbidden for this tool call
Forbidden { reason: String },
}
impl ApprovalRequirement {
pub fn allow_prefix(&self) -> Option<&Vec<String>> {
match self {
Self::NeedsApproval {
allow_prefix: Some(prefix),
..
} => Some(prefix),
_ => None,
}
}
}
/// - Never, OnFailure: do not ask
/// - OnRequest: ask unless sandbox policy is DangerFullAccess
/// - UnlessTrusted: always ask
@@ -111,7 +126,10 @@ pub(crate) fn default_approval_requirement(
};
if needs_approval {
ApprovalRequirement::NeedsApproval { reason: None }
ApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
}
} else {
ApprovalRequirement::Skip
}
codex-rs/core/src/unified_exec/session_manager.rs
+8 -10
View File
@@ -11,7 +11,7 @@ use crate::codex::TurnContext;
use crate::exec::ExecToolCallOutput;
use crate::exec::StreamOutput;
use crate::exec_env::create_env;
use crate::exec_policy::command_approval_outcome_for_command;
use crate::exec_policy::command_approval_requirement_for_command;
use crate::protocol::BackgroundEventEvent;
use crate::protocol::EventMsg;
use crate::protocol::ExecCommandSource;
@@ -446,21 +446,19 @@ impl UnifiedExecSessionManager {
let mut orchestrator = ToolOrchestrator::new();
let mut runtime = UnifiedExecRuntime::new(self);
let exec_policy = context.session.current_exec_policy().await;
let approval_outcome = command_approval_outcome_for_command(
exec_policy.as_ref(),
command,
context.turn.approval_policy,
&context.turn.sandbox_policy,
SandboxPermissions::from(with_escalated_permissions.unwrap_or(false)),
);
let req = UnifiedExecToolRequest::new(
command.to_vec(),
cwd,
create_env(&context.turn.shell_environment_policy),
with_escalated_permissions,
justification,
approval_outcome.requirement,
approval_outcome.allow_prefix,
command_approval_requirement_for_command(
exec_policy.as_ref(),
command,
context.turn.approval_policy,
&context.turn.sandbox_policy,
SandboxPermissions::from(with_escalated_permissions.unwrap_or(false)),
),
);
let tool_ctx = ToolCtx {
session: context.session.as_ref(),