mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Add approval allow-prefix flow in core and tui
This commit is contained in:
@@ -179,6 +179,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
allow_prefix: _allow_prefix,
|
||||
parsed_cmd,
|
||||
}) => match api_version {
|
||||
ApiVersion::V1 => {
|
||||
@@ -949,6 +950,7 @@ async fn on_exec_approval_response(
|
||||
.submit(Op::ExecApproval {
|
||||
id: event_turn_id,
|
||||
decision: response.decision,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
{
|
||||
@@ -1153,6 +1155,7 @@ async fn on_command_execution_request_approval_response(
|
||||
.submit(Op::ExecApproval {
|
||||
id: event_turn_id,
|
||||
decision,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
{
|
||||
|
||||
@@ -71,6 +71,7 @@ use crate::error::CodexErr;
|
||||
use crate::error::Result as CodexResult;
|
||||
#[cfg(test)]
|
||||
use crate::exec::StreamOutput;
|
||||
use crate::exec_policy::ExecPolicyUpdateError;
|
||||
use crate::mcp::auth::compute_auth_statuses;
|
||||
use crate::mcp_connection_manager::McpConnectionManager;
|
||||
use crate::model_family::find_family_for_model;
|
||||
@@ -861,11 +862,43 @@ impl Session {
|
||||
.await
|
||||
}
|
||||
|
||||
pub(crate) async fn persist_command_allow_prefix(
|
||||
&self,
|
||||
prefix: &[String],
|
||||
) -> Result<(), ExecPolicyUpdateError> {
|
||||
let (features, codex_home) = {
|
||||
let state = self.state.lock().await;
|
||||
(
|
||||
state.session_configuration.features.clone(),
|
||||
state
|
||||
.session_configuration
|
||||
.original_config_do_not_use
|
||||
.codex_home
|
||||
.clone(),
|
||||
)
|
||||
};
|
||||
|
||||
let policy =
|
||||
crate::exec_policy::append_allow_prefix_rule_and_reload(&features, &codex_home, prefix)
|
||||
.await?;
|
||||
|
||||
let mut state = self.state.lock().await;
|
||||
state.session_configuration.exec_policy = policy;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn current_exec_policy(&self) -> Arc<ExecPolicy> {
|
||||
let state = self.state.lock().await;
|
||||
state.session_configuration.exec_policy.clone()
|
||||
}
|
||||
|
||||
/// Emit an exec approval request event and await the user's decision.
|
||||
///
|
||||
/// 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,
|
||||
@@ -874,6 +907,7 @@ impl Session {
|
||||
cwd: PathBuf,
|
||||
reason: Option<String>,
|
||||
risk: Option<SandboxCommandAssessment>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
) -> ReviewDecision {
|
||||
let sub_id = turn_context.sub_id.clone();
|
||||
// Add the tx_approve callback to the map before sending the request.
|
||||
@@ -901,6 +935,7 @@ impl Session {
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
allow_prefix,
|
||||
parsed_cmd,
|
||||
});
|
||||
self.send_event(turn_context, event).await;
|
||||
@@ -1429,8 +1464,12 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
|
||||
handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op, &mut previous_context)
|
||||
.await;
|
||||
}
|
||||
Op::ExecApproval { id, decision } => {
|
||||
handlers::exec_approval(&sess, id, decision).await;
|
||||
Op::ExecApproval {
|
||||
id,
|
||||
decision,
|
||||
allow_prefix,
|
||||
} => {
|
||||
handlers::exec_approval(&sess, id, decision, allow_prefix).await;
|
||||
}
|
||||
Op::PatchApproval { id, decision } => {
|
||||
handlers::patch_approval(&sess, id, decision).await;
|
||||
@@ -1508,6 +1547,7 @@ mod handlers {
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::ReviewRequest;
|
||||
use codex_protocol::protocol::TurnAbortReason;
|
||||
use codex_protocol::protocol::WarningEvent;
|
||||
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_rmcp_client::ElicitationAction;
|
||||
@@ -1622,7 +1662,28 @@ mod handlers {
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn exec_approval(sess: &Arc<Session>, id: String, decision: ReviewDecision) {
|
||||
pub async fn exec_approval(
|
||||
sess: &Arc<Session>,
|
||||
id: String,
|
||||
decision: ReviewDecision,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
) {
|
||||
if let Some(prefix) = allow_prefix
|
||||
&& matches!(
|
||||
decision,
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession
|
||||
)
|
||||
&& let Err(err) = sess.persist_command_allow_prefix(&prefix).await
|
||||
{
|
||||
let message = format!("Failed to update execpolicy allow list: {err}");
|
||||
tracing::warn!("{message}");
|
||||
let warning = EventMsg::Warning(WarningEvent { message });
|
||||
sess.send_event_raw(Event {
|
||||
id: id.clone(),
|
||||
msg: warning,
|
||||
})
|
||||
.await;
|
||||
}
|
||||
match decision {
|
||||
ReviewDecision::Abort => {
|
||||
sess.interrupt_task().await;
|
||||
|
||||
@@ -275,6 +275,7 @@ async fn handle_exec_approval(
|
||||
event.cwd,
|
||||
event.reason,
|
||||
event.risk,
|
||||
event.allow_prefix,
|
||||
);
|
||||
let decision = await_approval_with_cancel(
|
||||
approval_fut,
|
||||
@@ -284,7 +285,13 @@ async fn handle_exec_approval(
|
||||
)
|
||||
.await;
|
||||
|
||||
let _ = codex.submit(Op::ExecApproval { id, decision }).await;
|
||||
let _ = codex
|
||||
.submit(Op::ExecApproval {
|
||||
id,
|
||||
decision,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
/// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying.
|
||||
|
||||
@@ -4,10 +4,12 @@ use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
use crate::command_safety::is_dangerous_command::requires_initial_appoval;
|
||||
use codex_execpolicy::AmendError;
|
||||
use codex_execpolicy::Decision;
|
||||
use codex_execpolicy::Evaluation;
|
||||
use codex_execpolicy::Policy;
|
||||
use codex_execpolicy::PolicyParser;
|
||||
use codex_execpolicy::append_allow_prefix_rule;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use thiserror::Error;
|
||||
@@ -23,6 +25,7 @@ const FORBIDDEN_REASON: &str = "execpolicy forbids this command";
|
||||
const PROMPT_REASON: &str = "execpolicy requires approval for this command";
|
||||
const POLICY_DIR_NAME: &str = "policy";
|
||||
const POLICY_EXTENSION: &str = "codexpolicy";
|
||||
const DEFAULT_POLICY_FILE: &str = "default.codexpolicy";
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
pub enum ExecPolicyError {
|
||||
@@ -45,6 +48,15 @@ pub enum ExecPolicyError {
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
pub enum ExecPolicyUpdateError {
|
||||
#[error("failed to update execpolicy file {path}: {source}")]
|
||||
AppendRule { path: PathBuf, source: AmendError },
|
||||
|
||||
#[error("failed to reload execpolicy after updating policy: {0}")]
|
||||
Reload(#[from] ExecPolicyError),
|
||||
}
|
||||
|
||||
pub(crate) async fn exec_policy_for(
|
||||
features: &Features,
|
||||
codex_home: &Path,
|
||||
@@ -84,37 +96,68 @@ pub(crate) async fn exec_policy_for(
|
||||
Ok(policy)
|
||||
}
|
||||
|
||||
fn evaluate_with_policy(
|
||||
policy: &Policy,
|
||||
command: &[String],
|
||||
approval_policy: AskForApproval,
|
||||
) -> Option<ApprovalRequirement> {
|
||||
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
|
||||
let evaluation = policy.check_multiple(commands.iter());
|
||||
pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf {
|
||||
codex_home.join(POLICY_DIR_NAME).join(DEFAULT_POLICY_FILE)
|
||||
}
|
||||
|
||||
match evaluation {
|
||||
Evaluation::Match { decision, .. } => match decision {
|
||||
Decision::Forbidden => Some(ApprovalRequirement::Forbidden {
|
||||
reason: FORBIDDEN_REASON.to_string(),
|
||||
}),
|
||||
Decision::Prompt => {
|
||||
let reason = PROMPT_REASON.to_string();
|
||||
if matches!(approval_policy, AskForApproval::Never) {
|
||||
Some(ApprovalRequirement::Forbidden { reason })
|
||||
} else {
|
||||
Some(ApprovalRequirement::NeedsApproval {
|
||||
reason: Some(reason),
|
||||
})
|
||||
pub(crate) async fn append_allow_prefix_rule_and_reload(
|
||||
features: &Features,
|
||||
codex_home: &Path,
|
||||
prefix: &[String],
|
||||
) -> Result<Arc<Policy>, ExecPolicyUpdateError> {
|
||||
let policy_path = default_policy_path(codex_home);
|
||||
append_allow_prefix_rule(&policy_path, prefix).map_err(|source| {
|
||||
ExecPolicyUpdateError::AppendRule {
|
||||
path: policy_path,
|
||||
source,
|
||||
}
|
||||
})?;
|
||||
|
||||
exec_policy_for(features, codex_home)
|
||||
.await
|
||||
.map_err(ExecPolicyUpdateError::Reload)
|
||||
}
|
||||
|
||||
fn requirement_from_decision(
|
||||
decision: Decision,
|
||||
approval_policy: AskForApproval,
|
||||
) -> ApprovalRequirement {
|
||||
match decision {
|
||||
Decision::Forbidden => ApprovalRequirement::Forbidden {
|
||||
reason: FORBIDDEN_REASON.to_string(),
|
||||
},
|
||||
Decision::Prompt => {
|
||||
let reason = PROMPT_REASON.to_string();
|
||||
if matches!(approval_policy, AskForApproval::Never) {
|
||||
ApprovalRequirement::Forbidden { reason }
|
||||
} else {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: Some(reason),
|
||||
allow_prefix: None,
|
||||
}
|
||||
}
|
||||
Decision::Allow => Some(ApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
}),
|
||||
}
|
||||
Decision::Allow => ApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
},
|
||||
Evaluation::NoMatch { .. } => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Return 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,
|
||||
) -> Option<Vec<String>> {
|
||||
if matches!(evaluation, Evaluation::NoMatch) && commands.len() == 1 {
|
||||
return Some(commands[0].clone());
|
||||
}
|
||||
|
||||
None
|
||||
}
|
||||
|
||||
pub(crate) fn create_approval_requirement_for_command(
|
||||
policy: &Policy,
|
||||
command: &[String],
|
||||
@@ -122,20 +165,27 @@ pub(crate) fn create_approval_requirement_for_command(
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
) -> ApprovalRequirement {
|
||||
if let Some(requirement) = evaluate_with_policy(policy, command, approval_policy) {
|
||||
return requirement;
|
||||
}
|
||||
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
|
||||
let evaluation = policy.check_multiple(commands.iter());
|
||||
|
||||
if requires_initial_appoval(
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
command,
|
||||
sandbox_permissions,
|
||||
) {
|
||||
ApprovalRequirement::NeedsApproval { reason: None }
|
||||
} else {
|
||||
ApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
match evaluation {
|
||||
Evaluation::Match { decision, .. } => requirement_from_decision(decision, approval_policy),
|
||||
Evaluation::NoMatch { .. } => {
|
||||
if requires_initial_appoval(
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
command,
|
||||
sandbox_permissions,
|
||||
) {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: allow_prefix_if_applicable(&commands, &evaluation),
|
||||
}
|
||||
} else {
|
||||
ApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -253,7 +303,7 @@ mod tests {
|
||||
let temp_dir = tempdir().expect("create temp dir");
|
||||
fs::write(
|
||||
temp_dir.path().join("root.codexpolicy"),
|
||||
r#"prefix_rule(pattern=["ls"], decision="prompt")"#,
|
||||
r#"prefix_rule(pattern=[\"ls\"], decision=\"prompt\")"#,
|
||||
)
|
||||
.expect("write policy file");
|
||||
|
||||
@@ -284,9 +334,13 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
"rm -rf /tmp".to_string(),
|
||||
];
|
||||
|
||||
let requirement =
|
||||
evaluate_with_policy(&policy, &forbidden_script, AskForApproval::OnRequest)
|
||||
.expect("expected match for forbidden command");
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&forbidden_script,
|
||||
AskForApproval::OnRequest,
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
SandboxPermissions::UseDefault,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
@@ -317,7 +371,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,
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -363,7 +418,83 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ApprovalRequirement::NeedsApproval { reason: None }
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: Some(command)
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allow_prefix_is_present_for_single_command_without_policy_match() {
|
||||
let command = vec!["python".to_string()];
|
||||
|
||||
let empty_policy = Policy::empty();
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&empty_policy,
|
||||
&command,
|
||||
AskForApproval::UnlessTrusted,
|
||||
&SandboxPolicy::ReadOnly,
|
||||
SandboxPermissions::UseDefault,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: Some(command)
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allow_prefix_is_omitted_when_policy_prompts() {
|
||||
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
.parse("test.codexpolicy", policy_src)
|
||||
.expect("parse policy");
|
||||
let policy = parser.build();
|
||||
let command = vec!["rm".to_string()];
|
||||
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&command,
|
||||
AskForApproval::OnRequest,
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
SandboxPermissions::UseDefault,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: Some(PROMPT_REASON.to_string()),
|
||||
allow_prefix: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allow_prefix_is_omitted_for_multi_command_scripts() {
|
||||
let command = vec![
|
||||
"bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"python && echo ok".to_string(),
|
||||
];
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&Policy::empty(),
|
||||
&command,
|
||||
AskForApproval::UnlessTrusted,
|
||||
&SandboxPolicy::ReadOnly,
|
||||
SandboxPermissions::UseDefault,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -231,6 +231,7 @@ impl ShellHandler {
|
||||
let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None);
|
||||
emitter.begin(event_ctx).await;
|
||||
|
||||
let exec_policy = session.current_exec_policy().await;
|
||||
let req = ShellRequest {
|
||||
command: exec_params.command.clone(),
|
||||
cwd: exec_params.cwd.clone(),
|
||||
@@ -239,7 +240,7 @@ impl ShellHandler {
|
||||
with_escalated_permissions: exec_params.with_escalated_permissions,
|
||||
justification: exec_params.justification.clone(),
|
||||
approval_requirement: create_approval_requirement_for_command(
|
||||
&turn.exec_policy,
|
||||
exec_policy.as_ref(),
|
||||
&exec_params.command,
|
||||
turn.approval_policy,
|
||||
&turn.sandbox_policy,
|
||||
|
||||
@@ -64,7 +64,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() {
|
||||
|
||||
@@ -127,6 +127,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
cwd,
|
||||
Some(reason),
|
||||
risk,
|
||||
None,
|
||||
)
|
||||
.await
|
||||
} else if user_explicitly_approved {
|
||||
|
||||
@@ -107,7 +107,15 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
Box::pin(async move {
|
||||
with_cached_approval(&session.services, key, move || async move {
|
||||
session
|
||||
.request_command_approval(turn, call_id, command, cwd, reason, risk)
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
req.approval_requirement.allow_prefix().cloned(),
|
||||
)
|
||||
.await
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -125,7 +125,15 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
Box::pin(async move {
|
||||
with_cached_approval(&session.services, key, || async move {
|
||||
session
|
||||
.request_command_approval(turn, call_id, command, cwd, reason, risk)
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
req.approval_requirement.allow_prefix().cloned(),
|
||||
)
|
||||
.await
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -96,11 +96,26 @@ pub(crate) enum ApprovalRequirement {
|
||||
bypass_sandbox: bool,
|
||||
},
|
||||
/// 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
|
||||
@@ -115,7 +130,10 @@ pub(crate) fn default_approval_requirement(
|
||||
};
|
||||
|
||||
if needs_approval {
|
||||
ApprovalRequirement::NeedsApproval { reason: None }
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None,
|
||||
}
|
||||
} else {
|
||||
ApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
|
||||
@@ -554,6 +554,7 @@ impl UnifiedExecSessionManager {
|
||||
let env = apply_unified_exec_env(create_env(&context.turn.shell_environment_policy));
|
||||
let mut orchestrator = ToolOrchestrator::new();
|
||||
let mut runtime = UnifiedExecRuntime::new(self);
|
||||
let exec_policy = context.session.current_exec_policy().await;
|
||||
let req = UnifiedExecToolRequest::new(
|
||||
command.to_vec(),
|
||||
cwd,
|
||||
@@ -561,7 +562,7 @@ impl UnifiedExecSessionManager {
|
||||
with_escalated_permissions,
|
||||
justification,
|
||||
create_approval_requirement_for_command(
|
||||
&context.turn.exec_policy,
|
||||
exec_policy.as_ref(),
|
||||
command,
|
||||
context.turn.approval_policy,
|
||||
&context.turn.sandbox_policy,
|
||||
|
||||
@@ -1524,6 +1524,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: *decision,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
@@ -95,6 +95,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() {
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::Approved,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.expect("submit exec approval");
|
||||
|
||||
@@ -843,6 +843,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() {
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::Approved,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -901,6 +902,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision()
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -959,6 +961,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() {
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::Approved,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -1017,6 +1020,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() {
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::Denied,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -1075,6 +1079,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision()
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -1134,6 +1139,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() {
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::Denied,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
@@ -180,6 +180,7 @@ async fn run_codex_tool_session_inner(
|
||||
call_id,
|
||||
reason: _,
|
||||
risk,
|
||||
allow_prefix: _,
|
||||
parsed_cmd,
|
||||
}) => {
|
||||
handle_exec_approval_request(
|
||||
|
||||
@@ -150,6 +150,7 @@ async fn on_exec_approval_response(
|
||||
.submit(Op::ExecApproval {
|
||||
id: event_id,
|
||||
decision: response.decision,
|
||||
allow_prefix: None,
|
||||
})
|
||||
.await
|
||||
{
|
||||
|
||||
@@ -51,6 +51,10 @@ pub struct ExecApprovalRequestEvent {
|
||||
/// Optional model-provided risk assessment describing the blocked command.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub risk: Option<SandboxCommandAssessment>,
|
||||
/// Prefix rule that can be added to the user's execpolicy to allow future runs.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional, type = "Array<string>")]
|
||||
pub allow_prefix: Option<Vec<String>>,
|
||||
pub parsed_cmd: Vec<ParsedCommand>,
|
||||
}
|
||||
|
||||
|
||||
@@ -146,6 +146,9 @@ pub enum Op {
|
||||
id: String,
|
||||
/// The user's decision in response to the request.
|
||||
decision: ReviewDecision,
|
||||
/// When set, persist this prefix to the execpolicy allow list.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
},
|
||||
|
||||
/// Approve a code patch
|
||||
|
||||
@@ -43,6 +43,7 @@ pub(crate) enum ApprovalRequest {
|
||||
command: Vec<String>,
|
||||
reason: Option<String>,
|
||||
risk: Option<SandboxCommandAssessment>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
},
|
||||
ApplyPatch {
|
||||
id: String,
|
||||
@@ -104,8 +105,8 @@ impl ApprovalOverlay {
|
||||
header: Box<dyn Renderable>,
|
||||
) -> (Vec<ApprovalOption>, SelectionViewParams) {
|
||||
let (options, title) = match &variant {
|
||||
ApprovalVariant::Exec { .. } => (
|
||||
exec_options(),
|
||||
ApprovalVariant::Exec { allow_prefix, .. } => (
|
||||
exec_options(allow_prefix.clone()),
|
||||
"Would you like to run the following command?".to_string(),
|
||||
),
|
||||
ApprovalVariant::ApplyPatch { .. } => (
|
||||
@@ -160,9 +161,9 @@ impl ApprovalOverlay {
|
||||
return;
|
||||
};
|
||||
if let Some(variant) = self.current_variant.as_ref() {
|
||||
match (&variant, &option.decision) {
|
||||
(ApprovalVariant::Exec { id, command }, ApprovalDecision::Review(decision)) => {
|
||||
self.handle_exec_decision(id, command, *decision);
|
||||
match (variant, &option.decision) {
|
||||
(ApprovalVariant::Exec { id, command, .. }, ApprovalDecision::Review(decision)) => {
|
||||
self.handle_exec_decision(id, command, *decision, option.allow_prefix.clone());
|
||||
}
|
||||
(ApprovalVariant::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => {
|
||||
self.handle_patch_decision(id, *decision);
|
||||
@@ -184,12 +185,19 @@ impl ApprovalOverlay {
|
||||
self.advance_queue();
|
||||
}
|
||||
|
||||
fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) {
|
||||
fn handle_exec_decision(
|
||||
&self,
|
||||
id: &str,
|
||||
command: &[String],
|
||||
decision: ReviewDecision,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
) {
|
||||
let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision);
|
||||
self.app_event_tx.send(AppEvent::InsertHistoryCell(cell));
|
||||
self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval {
|
||||
id: id.to_string(),
|
||||
decision,
|
||||
allow_prefix,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -273,8 +281,8 @@ impl BottomPaneView for ApprovalOverlay {
|
||||
&& let Some(variant) = self.current_variant.as_ref()
|
||||
{
|
||||
match &variant {
|
||||
ApprovalVariant::Exec { id, command } => {
|
||||
self.handle_exec_decision(id, command, ReviewDecision::Abort);
|
||||
ApprovalVariant::Exec { id, command, .. } => {
|
||||
self.handle_exec_decision(id, command, ReviewDecision::Abort, None);
|
||||
}
|
||||
ApprovalVariant::ApplyPatch { id, .. } => {
|
||||
self.handle_patch_decision(id, ReviewDecision::Abort);
|
||||
@@ -336,6 +344,7 @@ impl From<ApprovalRequest> for ApprovalRequestState {
|
||||
command,
|
||||
reason,
|
||||
risk,
|
||||
allow_prefix,
|
||||
} => {
|
||||
let reason = reason.filter(|item| !item.is_empty());
|
||||
let has_reason = reason.is_some();
|
||||
@@ -355,7 +364,11 @@ impl From<ApprovalRequest> for ApprovalRequestState {
|
||||
}
|
||||
header.extend(full_cmd_lines);
|
||||
Self {
|
||||
variant: ApprovalVariant::Exec { id, command },
|
||||
variant: ApprovalVariant::Exec {
|
||||
id,
|
||||
command,
|
||||
allow_prefix,
|
||||
},
|
||||
header: Box::new(Paragraph::new(header).wrap(Wrap { trim: false })),
|
||||
}
|
||||
}
|
||||
@@ -431,6 +444,7 @@ enum ApprovalVariant {
|
||||
Exec {
|
||||
id: String,
|
||||
command: Vec<String>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
},
|
||||
ApplyPatch {
|
||||
id: String,
|
||||
@@ -453,6 +467,7 @@ struct ApprovalOption {
|
||||
decision: ApprovalDecision,
|
||||
display_shortcut: Option<KeyBinding>,
|
||||
additional_shortcuts: Vec<KeyBinding>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
impl ApprovalOption {
|
||||
@@ -463,27 +478,39 @@ impl ApprovalOption {
|
||||
}
|
||||
}
|
||||
|
||||
fn exec_options() -> Vec<ApprovalOption> {
|
||||
fn exec_options(allow_prefix: Option<Vec<String>>) -> Vec<ApprovalOption> {
|
||||
vec![
|
||||
ApprovalOption {
|
||||
label: "Yes, proceed".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Approved),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "Yes, and don't ask again for this command".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "No, and tell Codex what to do differently".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Abort),
|
||||
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
]
|
||||
.into_iter()
|
||||
.chain(allow_prefix.map(|prefix| ApprovalOption {
|
||||
label: "Yes, and don't ask again for commands with this prefix".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))],
|
||||
allow_prefix: Some(prefix),
|
||||
}))
|
||||
.chain([ApprovalOption {
|
||||
label: "No, and tell Codex what to do differently".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Abort),
|
||||
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
|
||||
allow_prefix: None,
|
||||
}])
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn patch_options() -> Vec<ApprovalOption> {
|
||||
@@ -493,12 +520,14 @@ fn patch_options() -> Vec<ApprovalOption> {
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Approved),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "No, and tell Codex what to do differently".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Abort),
|
||||
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
]
|
||||
}
|
||||
@@ -510,18 +539,21 @@ fn elicitation_options() -> Vec<ApprovalOption> {
|
||||
decision: ApprovalDecision::McpElicitation(ElicitationAction::Accept),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "No, but continue without it".to_string(),
|
||||
decision: ApprovalDecision::McpElicitation(ElicitationAction::Decline),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "Cancel this request".to_string(),
|
||||
decision: ApprovalDecision::McpElicitation(ElicitationAction::Cancel),
|
||||
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('c'))],
|
||||
allow_prefix: None,
|
||||
},
|
||||
]
|
||||
}
|
||||
@@ -539,6 +571,7 @@ mod tests {
|
||||
command: vec!["echo".to_string(), "hi".to_string()],
|
||||
reason: Some("reason".to_string()),
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -571,6 +604,41 @@ mod tests {
|
||||
assert!(saw_op, "expected approval decision to emit an op");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_prefix_option_emits_allow_prefix() {
|
||||
let (tx, mut rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let mut view = ApprovalOverlay::new(
|
||||
ApprovalRequest::Exec {
|
||||
id: "test".to_string(),
|
||||
command: vec!["echo".to_string()],
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: Some(vec!["echo".to_string()]),
|
||||
},
|
||||
tx,
|
||||
);
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE));
|
||||
let mut saw_op = false;
|
||||
while let Ok(ev) = rx.try_recv() {
|
||||
if let AppEvent::CodexOp(Op::ExecApproval {
|
||||
allow_prefix,
|
||||
decision,
|
||||
..
|
||||
}) = ev
|
||||
{
|
||||
assert_eq!(decision, ReviewDecision::ApprovedForSession);
|
||||
assert_eq!(allow_prefix, Some(vec!["echo".to_string()]));
|
||||
saw_op = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
saw_op,
|
||||
"expected approval decision to emit an op with allow prefix"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn header_includes_command_snippet() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
@@ -581,6 +649,7 @@ mod tests {
|
||||
command,
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
};
|
||||
|
||||
let view = ApprovalOverlay::new(exec_request, tx);
|
||||
|
||||
@@ -563,6 +563,7 @@ mod tests {
|
||||
command: vec!["echo".into(), "ok".into()],
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1069,6 +1069,7 @@ impl ChatWidget {
|
||||
command: ev.command,
|
||||
reason: ev.reason,
|
||||
risk: ev.risk,
|
||||
allow_prefix: ev.allow_prefix,
|
||||
};
|
||||
self.bottom_pane.push_approval_request(request);
|
||||
self.request_redraw();
|
||||
|
||||
@@ -677,6 +677,7 @@ fn exec_approval_emits_proposed_command_and_decision_history() {
|
||||
"this is a test reason such as one that would be produced by the model".into(),
|
||||
),
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -721,6 +722,7 @@ fn exec_approval_decision_truncates_multiline_and_long_commands() {
|
||||
"this is a test reason such as one that would be produced by the model".into(),
|
||||
),
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -771,6 +773,7 @@ fn exec_approval_decision_truncates_multiline_and_long_commands() {
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -1969,6 +1972,7 @@ fn approval_modal_exec_snapshot() {
|
||||
"this is a test reason such as one that would be produced by the model".into(),
|
||||
),
|
||||
risk: None,
|
||||
allow_prefix: Some(vec!["echo".into(), "hello".into(), "world".into()]),
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -2015,6 +2019,7 @@ fn approval_modal_exec_without_reason_snapshot() {
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: Some(vec!["echo".into(), "hello".into(), "world".into()]),
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -2228,6 +2233,7 @@ fn status_widget_and_approval_modal_snapshot() {
|
||||
"this is a test reason such as one that would be produced by the model".into(),
|
||||
),
|
||||
risk: None,
|
||||
allow_prefix: Some(vec!["echo".into(), "hello world".into()]),
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
|
||||
Reference in New Issue
Block a user