mirror of
https://github.com/openai/codex.git
synced 2026-11-06 20:20:34 +00:00
Add PR description file for execpolicy refactor
This commit is contained in:
@@ -1,6 +0,0 @@
|
||||
## Summary
|
||||
- thread fallback evaluation through execpolicy so pipelines evaluate the first unmatched command instead of stopping at the first allow match
|
||||
- update core exec_policy integration, execpolicycheck CLI, and docs to use the new fallback path and avoid skipping heuristics on later piped commands
|
||||
|
||||
## Testing
|
||||
- not run (not requested)
|
||||
@@ -179,7 +179,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
allow_prefix: _allow_prefix,
|
||||
proposed_execpolicy_amendment: _proposed_execpolicy_amendment,
|
||||
parsed_cmd,
|
||||
}) => match api_version {
|
||||
ApiVersion::V1 => {
|
||||
|
||||
@@ -71,7 +71,7 @@ pub(crate) async fn apply_patch(
|
||||
.await;
|
||||
match rx_approve.await.unwrap_or_default() {
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedAllowPrefix { .. }
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => {
|
||||
InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
|
||||
action,
|
||||
|
||||
+15
-15
@@ -862,13 +862,9 @@ impl Session {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Adds a prefix rule to the exec policy
|
||||
///
|
||||
/// This mutates the in-memory execpolicy so the current conversation can use the new
|
||||
/// prefix and persists the change in default.execpolicy so new conversations will also allow the new prefix.
|
||||
pub(crate) async fn persist_command_allow_prefix(
|
||||
pub(crate) async fn persist_execpolicy_amendment(
|
||||
&self,
|
||||
prefix: &[String],
|
||||
amendment: &[String],
|
||||
) -> Result<(), ExecPolicyUpdateError> {
|
||||
let (features, codex_home, current_policy) = {
|
||||
let state = self.state.lock().await;
|
||||
@@ -888,10 +884,10 @@ impl Session {
|
||||
return Err(ExecPolicyUpdateError::FeatureDisabled);
|
||||
}
|
||||
|
||||
crate::exec_policy::append_allow_prefix_rule_and_update(
|
||||
crate::exec_policy::append_execpolicy_amendment_and_update(
|
||||
&codex_home,
|
||||
¤t_policy,
|
||||
prefix,
|
||||
amendment,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -912,7 +908,7 @@ impl Session {
|
||||
cwd: PathBuf,
|
||||
reason: Option<String>,
|
||||
risk: Option<SandboxCommandAssessment>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
proposed_execpolicy_amendment: Option<Vec<String>>,
|
||||
) -> ReviewDecision {
|
||||
let sub_id = turn_context.sub_id.clone();
|
||||
// Add the tx_approve callback to the map before sending the request.
|
||||
@@ -940,7 +936,7 @@ impl Session {
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
allow_prefix,
|
||||
proposed_execpolicy_amendment,
|
||||
parsed_cmd,
|
||||
});
|
||||
self.send_event(turn_context, event).await;
|
||||
@@ -1672,13 +1668,17 @@ mod handlers {
|
||||
}
|
||||
}
|
||||
|
||||
/// Propagate a user's exec approval decision to the session
|
||||
/// Also optionally whitelists command in execpolicy
|
||||
/// Propagate a user's exec approval decision to the session.
|
||||
/// Also optionally applies an execpolicy amendment.
|
||||
pub async fn exec_approval(sess: &Arc<Session>, id: String, decision: ReviewDecision) {
|
||||
if let ReviewDecision::ApprovedAllowPrefix { allow_prefix } = &decision
|
||||
&& let Err(err) = sess.persist_command_allow_prefix(allow_prefix).await
|
||||
if let ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment,
|
||||
} = &decision
|
||||
&& let Err(err) = sess
|
||||
.persist_execpolicy_amendment(proposed_execpolicy_amendment)
|
||||
.await
|
||||
{
|
||||
let message = format!("Failed to update execpolicy allow list: {err}");
|
||||
let message = format!("Failed to apply execpolicy amendment: {err}");
|
||||
tracing::warn!("{message}");
|
||||
let warning = EventMsg::Warning(WarningEvent { message });
|
||||
sess.send_event_raw(Event {
|
||||
|
||||
@@ -275,7 +275,7 @@ async fn handle_exec_approval(
|
||||
event.cwd,
|
||||
event.reason,
|
||||
event.risk,
|
||||
event.allow_prefix,
|
||||
event.proposed_execpolicy_amendment,
|
||||
);
|
||||
let decision = await_approval_with_cancel(
|
||||
approval_fut,
|
||||
|
||||
+110
-120
@@ -23,7 +23,7 @@ use crate::bash::parse_shell_lc_plain_commands;
|
||||
use crate::features::Feature;
|
||||
use crate::features::Features;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
|
||||
const FORBIDDEN_REASON: &str = "execpolicy forbids this command";
|
||||
const PROMPT_CONFLICT_REASON: &str =
|
||||
@@ -115,7 +115,7 @@ pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf {
|
||||
codex_home.join(POLICY_DIR_NAME).join(DEFAULT_POLICY_FILE)
|
||||
}
|
||||
|
||||
pub(crate) async fn append_allow_prefix_rule_and_update(
|
||||
pub(crate) async fn append_execpolicy_amendment_and_update(
|
||||
codex_home: &Path,
|
||||
current_policy: &Arc<RwLock<Policy>>,
|
||||
prefix: &[String],
|
||||
@@ -142,59 +142,25 @@ pub(crate) async fn append_allow_prefix_rule_and_update(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn create_exec_approval_requirement_for_command(
|
||||
exec_policy: &Arc<RwLock<Policy>>,
|
||||
features: &Features,
|
||||
command: &[String],
|
||||
approval_policy: AskForApproval,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
) -> ExecApprovalRequirement {
|
||||
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
|
||||
let heuristics_fallback = |cmd: &[String]| {
|
||||
if requires_initial_appoval(approval_policy, sandbox_policy, cmd, sandbox_permissions) {
|
||||
Decision::Prompt
|
||||
} else {
|
||||
Decision::Allow
|
||||
}
|
||||
};
|
||||
let policy = exec_policy.read().await;
|
||||
let evaluation = policy.check_multiple(commands.iter(), &heuristics_fallback);
|
||||
let has_policy_allow = evaluation.matched_rules.iter().any(|rule_match| {
|
||||
!matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })
|
||||
&& rule_match.decision() == Decision::Allow
|
||||
});
|
||||
|
||||
match evaluation.decision {
|
||||
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
|
||||
reason: FORBIDDEN_REASON.to_string(),
|
||||
},
|
||||
Decision::Prompt => {
|
||||
let prompt_reason = derive_prompt_reason(&evaluation);
|
||||
if matches!(approval_policy, AskForApproval::Never) {
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: prompt_reason.unwrap_or_else(|| PROMPT_CONFLICT_REASON.to_string()),
|
||||
}
|
||||
} else {
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: prompt_reason.or_else(|| Some(PROMPT_REASON.to_string())),
|
||||
allow_prefix: allow_prefix_if_applicable(&evaluation, &commands, features),
|
||||
}
|
||||
}
|
||||
}
|
||||
Decision::Allow => ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: has_policy_allow,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// Return an allow-prefix option when a command needs approval and execpolicy did not drive the decision
|
||||
fn allow_prefix_if_applicable(
|
||||
/// Returns a proposed execpolicy amendment only when heuristics caused
|
||||
/// the prompt decision, so we can offer to apply that amendment for future runs.
|
||||
///
|
||||
/// The amendment uses the first command heuristics marked as `Prompt`. If any explicit
|
||||
/// execpolicy rule also prompts, we return `None` because applying the amendment would not
|
||||
/// skip that policy requirement.
|
||||
///
|
||||
/// Examples:
|
||||
/// - execpolicy: empty. Command: `["python"]`. Heuristics prompt -> `Some(vec!["python"])`.
|
||||
/// - execpolicy: empty. Command: `["bash", "-c", "cd /some/folder && prog1 --option1 arg1 && prog2 --option2 arg2"]`.
|
||||
/// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. If heuristics allow `cd` but prompt
|
||||
/// on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`.
|
||||
/// - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above,
|
||||
/// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"].
|
||||
fn proposed_execpolicy_amendment(
|
||||
evaluation: &Evaluation,
|
||||
commands: &[Vec<String>],
|
||||
features: &Features,
|
||||
) -> Option<Vec<String>> {
|
||||
if !features.enabled(Feature::ExecPolicy) || commands.len() != 1 {
|
||||
if !features.enabled(Feature::ExecPolicy) {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -220,6 +186,55 @@ fn allow_prefix_if_applicable(
|
||||
first_prompt_from_heuristics
|
||||
}
|
||||
|
||||
pub(crate) async fn create_approval_requirement_for_command(
|
||||
exec_policy: &Arc<RwLock<Policy>>,
|
||||
features: &Features,
|
||||
command: &[String],
|
||||
approval_policy: AskForApproval,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
) -> ApprovalRequirement {
|
||||
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
|
||||
let heuristics_fallback = |cmd: &[String]| {
|
||||
if requires_initial_appoval(approval_policy, sandbox_policy, cmd, sandbox_permissions) {
|
||||
Decision::Prompt
|
||||
} else {
|
||||
Decision::Allow
|
||||
}
|
||||
};
|
||||
let policy = exec_policy.read().await;
|
||||
let evaluation = policy.check_multiple(commands.iter(), &heuristics_fallback);
|
||||
let has_policy_allow = evaluation.matched_rules.iter().any(|rule_match| {
|
||||
!matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })
|
||||
&& rule_match.decision() == Decision::Allow
|
||||
});
|
||||
|
||||
match evaluation.decision {
|
||||
Decision::Forbidden => ApprovalRequirement::Forbidden {
|
||||
reason: FORBIDDEN_REASON.to_string(),
|
||||
},
|
||||
Decision::Prompt => {
|
||||
let prompt_reason = derive_prompt_reason(&evaluation);
|
||||
if matches!(approval_policy, AskForApproval::Never) {
|
||||
ApprovalRequirement::Forbidden {
|
||||
reason: PROMPT_CONFLICT_REASON.to_string(),
|
||||
}
|
||||
} else {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: prompt_reason,
|
||||
proposed_execpolicy_amendment: proposed_execpolicy_amendment(
|
||||
&evaluation,
|
||||
features,
|
||||
),
|
||||
}
|
||||
}
|
||||
}
|
||||
Decision::Allow => ApprovalRequirement::Skip {
|
||||
bypass_sandbox: has_policy_allow,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// Only return PROMPT_REASON when an execpolicy rule drove the prompt decision
|
||||
fn derive_prompt_reason(evaluation: &Evaluation) -> Option<String> {
|
||||
evaluation.matched_rules.iter().find_map(|rule_match| {
|
||||
@@ -405,7 +420,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
"rm -rf /tmp".to_string(),
|
||||
];
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&forbidden_script,
|
||||
@@ -417,14 +432,14 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
ApprovalRequirement::Forbidden {
|
||||
reason: FORBIDDEN_REASON.to_string()
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_approval_requirement_prefers_execpolicy_match() {
|
||||
async fn approval_requirement_prefers_execpolicy_match() {
|
||||
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
@@ -433,7 +448,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
let policy = Arc::new(RwLock::new(parser.build()));
|
||||
let command = vec!["rm".to_string()];
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -445,15 +460,15 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: Some(PROMPT_REASON.to_string()),
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_approval_requirement_respects_approval_policy() {
|
||||
async fn approval_requirement_respects_approval_policy() {
|
||||
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
@@ -462,7 +477,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
let policy = Arc::new(RwLock::new(parser.build()));
|
||||
let command = vec!["rm".to_string()];
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -474,18 +489,18 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: PROMPT_REASON.to_string()
|
||||
ApprovalRequirement::Forbidden {
|
||||
reason: PROMPT_CONFLICT_REASON.to_string()
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_approval_requirement_falls_back_to_heuristics() {
|
||||
let command = vec!["cargo".to_string(), "build".to_string()];
|
||||
async fn approval_requirement_falls_back_to_heuristics() {
|
||||
let command = vec!["python".to_string()];
|
||||
|
||||
let empty_policy = Arc::new(RwLock::new(Policy::empty()));
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&empty_policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -497,9 +512,9 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: Some(command)
|
||||
proposed_execpolicy_amendment: Some(command)
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -519,7 +534,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
create_exec_approval_requirement_for_command(
|
||||
create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -528,20 +543,20 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
SandboxPermissions::UseDefault,
|
||||
)
|
||||
.await,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None
|
||||
proposed_execpolicy_amendment: Some(vec!["orange".to_string()])
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn append_allow_prefix_rule_updates_policy_and_file() {
|
||||
async fn append_execpolicy_amendment_updates_policy_and_file() {
|
||||
let codex_home = tempdir().expect("create temp dir");
|
||||
let current_policy = Arc::new(RwLock::new(Policy::empty()));
|
||||
let prefix = vec!["echo".to_string(), "hello".to_string()];
|
||||
|
||||
append_allow_prefix_rule_and_update(codex_home.path(), ¤t_policy, &prefix)
|
||||
append_execpolicy_amendment_and_update(codex_home.path(), ¤t_policy, &prefix)
|
||||
.await
|
||||
.expect("update policy");
|
||||
|
||||
@@ -567,12 +582,12 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn append_allow_prefix_rule_rejects_empty_prefix() {
|
||||
async fn append_execpolicy_amendment_rejects_empty_prefix() {
|
||||
let codex_home = tempdir().expect("create temp dir");
|
||||
let current_policy = Arc::new(RwLock::new(Policy::empty()));
|
||||
|
||||
let result =
|
||||
append_allow_prefix_rule_and_update(codex_home.path(), ¤t_policy, &[]).await;
|
||||
append_execpolicy_amendment_and_update(codex_home.path(), ¤t_policy, &[]).await;
|
||||
|
||||
assert!(matches!(
|
||||
result,
|
||||
@@ -584,11 +599,11 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn allow_prefix_is_present_for_single_command_without_policy_match() {
|
||||
let command = vec!["cargo".to_string(), "build".to_string()];
|
||||
async fn proposed_execpolicy_amendment_is_present_for_single_command_without_policy_match() {
|
||||
let command = vec!["python".to_string()];
|
||||
|
||||
let empty_policy = Arc::new(RwLock::new(Policy::empty()));
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&empty_policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -600,21 +615,21 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: Some(command)
|
||||
proposed_execpolicy_amendment: Some(command)
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn allow_prefix_is_disabled_when_execpolicy_feature_disabled() {
|
||||
let command = vec!["cargo".to_string(), "build".to_string()];
|
||||
async fn proposed_execpolicy_amendment_is_disabled_when_execpolicy_feature_disabled() {
|
||||
let command = vec!["python".to_string()];
|
||||
|
||||
let mut features = Features::with_defaults();
|
||||
features.disable(Feature::ExecPolicy);
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&Arc::new(RwLock::new(Policy::empty())),
|
||||
&features,
|
||||
&command,
|
||||
@@ -626,15 +641,15 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn allow_prefix_is_omitted_when_policy_prompts() {
|
||||
async fn proposed_execpolicy_amendment_is_omitted_when_policy_prompts() {
|
||||
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
@@ -643,7 +658,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
let policy = Arc::new(RwLock::new(parser.build()));
|
||||
let command = vec!["rm".to_string()];
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
let requirement = create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -655,42 +670,16 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: Some(PROMPT_REASON.to_string()),
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn allow_prefix_is_omitted_for_multi_command_scripts() {
|
||||
let command = vec![
|
||||
"bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"cargo build && echo ok".to_string(),
|
||||
];
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
&Arc::new(RwLock::new(Policy::empty())),
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
AskForApproval::UnlessTrusted,
|
||||
&SandboxPolicy::ReadOnly,
|
||||
SandboxPermissions::UseDefault,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn allow_prefix_uses_first_no_match_in_multi_command_scripts() {
|
||||
let policy_src = r#"prefix_rule(pattern=["python"], decision="allow")"#;
|
||||
async fn proposed_execpolicy_amendment_uses_first_no_match_in_multi_command_scripts() {
|
||||
let policy_src = r#"prefix_rule(pattern=["cat"], decision="allow")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
.parse("test.codexpolicy", policy_src)
|
||||
@@ -700,11 +689,11 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
let command = vec![
|
||||
"bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"python && echo ok".to_string(),
|
||||
"cat && apple".to_string(),
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
create_exec_approval_requirement_for_command(
|
||||
create_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
@@ -713,8 +702,9 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
SandboxPermissions::UseDefault,
|
||||
)
|
||||
.await,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: Some(vec!["apple".to_string()]),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
@@ -6,7 +6,7 @@ use std::sync::Arc;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::exec::ExecParams;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::exec_policy::create_exec_approval_requirement_for_command;
|
||||
use crate::exec_policy::create_approval_requirement_for_command;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::is_safe_command::is_known_safe_command;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
@@ -232,7 +232,7 @@ impl ShellHandler {
|
||||
emitter.begin(event_ctx).await;
|
||||
|
||||
let features = session.features().await;
|
||||
let exec_approval_requirement = create_exec_approval_requirement_for_command(
|
||||
let approval_requirement = create_approval_requirement_for_command(
|
||||
&turn.exec_policy,
|
||||
&features,
|
||||
&exec_params.command,
|
||||
@@ -241,6 +241,7 @@ impl ShellHandler {
|
||||
SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)),
|
||||
)
|
||||
.await;
|
||||
|
||||
let req = ShellRequest {
|
||||
command: exec_params.command.clone(),
|
||||
cwd: exec_params.cwd.clone(),
|
||||
@@ -248,7 +249,7 @@ impl ShellHandler {
|
||||
env: exec_params.env.clone(),
|
||||
with_escalated_permissions: exec_params.with_escalated_permissions,
|
||||
justification: exec_params.justification.clone(),
|
||||
exec_approval_requirement,
|
||||
approval_requirement,
|
||||
};
|
||||
let mut orchestrator = ToolOrchestrator::new();
|
||||
let mut runtime = ShellRuntime::new();
|
||||
|
||||
@@ -11,14 +11,14 @@ use crate::error::get_error_message_ui;
|
||||
use crate::exec::ExecToolCallOutput;
|
||||
use crate::sandboxing::SandboxManager;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
use crate::tools::sandboxing::ProvidesSandboxRetryData;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::default_exec_approval_requirement;
|
||||
use crate::tools::sandboxing::default_approval_requirement;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
|
||||
@@ -54,17 +54,17 @@ impl ToolOrchestrator {
|
||||
// 1) Approval
|
||||
let mut already_approved = false;
|
||||
|
||||
let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| {
|
||||
default_exec_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
|
||||
let requirement = tool.approval_requirement(req).unwrap_or_else(|| {
|
||||
default_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
|
||||
});
|
||||
match requirement {
|
||||
ExecApprovalRequirement::Skip { .. } => {
|
||||
ApprovalRequirement::Skip { .. } => {
|
||||
otel.tool_decision(otel_tn, otel_ci, &ReviewDecision::Approved, otel_cfg);
|
||||
}
|
||||
ExecApprovalRequirement::Forbidden { reason } => {
|
||||
ApprovalRequirement::Forbidden { reason } => {
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
ApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
let mut risk = None;
|
||||
|
||||
if let Some(metadata) = req.sandbox_retry_data() {
|
||||
@@ -95,7 +95,7 @@ impl ToolOrchestrator {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedAllowPrefix { .. }
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
}
|
||||
already_approved = true;
|
||||
@@ -120,87 +120,50 @@ impl ToolOrchestrator {
|
||||
codex_linux_sandbox_exe: turn_ctx.codex_linux_sandbox_exe.as_ref(),
|
||||
};
|
||||
|
||||
match tool.run(req, &initial_attempt, tool_ctx).await {
|
||||
Ok(out) => {
|
||||
// We have a successful initial result
|
||||
Ok(out)
|
||||
}
|
||||
Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output }))) => {
|
||||
if !tool.escalate_on_failure() {
|
||||
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
|
||||
output,
|
||||
})));
|
||||
}
|
||||
// Under `Never` or `OnRequest`, do not retry without sandbox; surface a concise
|
||||
// sandbox denial that preserves the original output.
|
||||
if !tool.wants_no_sandbox_approval(approval_policy) {
|
||||
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
|
||||
output,
|
||||
})));
|
||||
}
|
||||
let mut out = tool.run(req, &initial_attempt, tool_ctx).await;
|
||||
|
||||
// Ask for approval before retrying without sandbox.
|
||||
if !tool.should_bypass_approval(approval_policy, already_approved) {
|
||||
let mut risk = None;
|
||||
if matches!(initial_sandbox, crate::exec::SandboxType::None) {
|
||||
// If we already skipped sandboxing on the first attempt, there's no
|
||||
// fallback path.
|
||||
return out;
|
||||
}
|
||||
|
||||
if let Some(metadata) = req.sandbox_retry_data() {
|
||||
let err = SandboxErr::Denied {
|
||||
output: output.clone(),
|
||||
};
|
||||
let friendly = get_error_message_ui(&CodexErr::Sandbox(err));
|
||||
let failure_summary = format!("failed in sandbox: {friendly}");
|
||||
|
||||
risk = tool_ctx
|
||||
.session
|
||||
.assess_sandbox_command(
|
||||
turn_ctx,
|
||||
&tool_ctx.call_id,
|
||||
&metadata.command,
|
||||
Some(failure_summary.as_str()),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
let reason_msg = build_denial_reason_from_output(output.as_ref());
|
||||
let approval_ctx = ApprovalCtx {
|
||||
session: tool_ctx.session,
|
||||
turn: turn_ctx,
|
||||
call_id: &tool_ctx.call_id,
|
||||
retry_reason: Some(reason_msg),
|
||||
risk,
|
||||
};
|
||||
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
otel.tool_decision(otel_tn, otel_ci, &decision, otel_user);
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedAllowPrefix { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
}
|
||||
}
|
||||
|
||||
let escalated_attempt = SandboxAttempt {
|
||||
// 3) Retry without sandbox on approval.
|
||||
if matches!(&out, Err(ToolError::Codex(CodexErr::Sandbox(_))))
|
||||
&& tool.sandbox_preference() != crate::tools::sandboxing::SandboxablePreference::Require
|
||||
&& tool.escalate_on_failure()
|
||||
&& tool.should_bypass_approval(approval_policy, already_approved)
|
||||
{
|
||||
// Attempt a retry without sandbox.
|
||||
out = tool.run(
|
||||
req,
|
||||
&SandboxAttempt {
|
||||
sandbox: crate::exec::SandboxType::None,
|
||||
policy: &turn_ctx.sandbox_policy,
|
||||
policy: &crate::protocol::SandboxPolicy::DangerFullAccess,
|
||||
manager: &self.sandbox,
|
||||
sandbox_cwd: &turn_ctx.cwd,
|
||||
codex_linux_sandbox_exe: None,
|
||||
};
|
||||
|
||||
// Second attempt.
|
||||
(*tool).run(req, &escalated_attempt, tool_ctx).await
|
||||
codex_linux_sandbox_exe: turn_ctx.codex_linux_sandbox_exe.as_ref(),
|
||||
},
|
||||
tool_ctx,
|
||||
)
|
||||
.await;
|
||||
if let Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output }))) = &out {
|
||||
return Err(ToolError::Rejected(format!(
|
||||
"sandbox denied the command, even after approving it without sandbox: {}",
|
||||
get_error_message_ui(output)
|
||||
)));
|
||||
}
|
||||
other => other,
|
||||
}
|
||||
|
||||
out
|
||||
}
|
||||
|
||||
/// Translate result from tool runner to library level result (errors not in ToolError become CodexErr).
|
||||
pub fn translate_response(result: Result<ExecToolCallOutput, ToolError>) -> Result<(), CodexErr> {
|
||||
match result {
|
||||
Ok(_) => Ok(()),
|
||||
Err(ToolError::Codex(err)) => Err(err),
|
||||
Err(ToolError::Rejected(reason)) => Err(CodexErr::Rejected(reason)),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {
|
||||
// Keep approval reason terse and stable for UX/tests, but accept the
|
||||
// output so we can evolve heuristics later without touching call sites.
|
||||
"command failed; retry without sandbox?".to_string()
|
||||
}
|
||||
|
||||
@@ -9,7 +9,7 @@ use crate::sandboxing::execute_env;
|
||||
use crate::tools::runtimes::build_command_spec;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
use crate::tools::sandboxing::ProvidesSandboxRetryData;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
@@ -32,7 +32,7 @@ pub struct ShellRequest {
|
||||
pub env: std::collections::HashMap<String, String>,
|
||||
pub with_escalated_permissions: Option<bool>,
|
||||
pub justification: Option<String>,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
pub approval_requirement: ApprovalRequirement,
|
||||
}
|
||||
|
||||
impl ProvidesSandboxRetryData for ShellRequest {
|
||||
@@ -114,7 +114,9 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
req.exec_approval_requirement.allow_prefix().cloned(),
|
||||
req.approval_requirement
|
||||
.proposed_execpolicy_amendment()
|
||||
.cloned(),
|
||||
)
|
||||
.await
|
||||
})
|
||||
@@ -122,15 +124,15 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
})
|
||||
}
|
||||
|
||||
fn exec_approval_requirement(&self, req: &ShellRequest) -> Option<ExecApprovalRequirement> {
|
||||
Some(req.exec_approval_requirement.clone())
|
||||
fn approval_requirement(&self, req: &ShellRequest) -> Option<ApprovalRequirement> {
|
||||
Some(req.approval_requirement.clone())
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride {
|
||||
if req.with_escalated_permissions.unwrap_or(false)
|
||||
|| matches!(
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
req.approval_requirement,
|
||||
ApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
}
|
||||
)
|
||||
|
||||
@@ -10,7 +10,7 @@ use crate::exec::ExecExpiration;
|
||||
use crate::tools::runtimes::build_command_spec;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
use crate::tools::sandboxing::ProvidesSandboxRetryData;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
@@ -36,7 +36,7 @@ pub struct UnifiedExecRequest {
|
||||
pub env: HashMap<String, String>,
|
||||
pub with_escalated_permissions: Option<bool>,
|
||||
pub justification: Option<String>,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
pub approval_requirement: ApprovalRequirement,
|
||||
}
|
||||
|
||||
impl ProvidesSandboxRetryData for UnifiedExecRequest {
|
||||
@@ -66,7 +66,7 @@ impl UnifiedExecRequest {
|
||||
env: HashMap<String, String>,
|
||||
with_escalated_permissions: Option<bool>,
|
||||
justification: Option<String>,
|
||||
exec_approval_requirement: ExecApprovalRequirement,
|
||||
approval_requirement: ApprovalRequirement,
|
||||
) -> Self {
|
||||
Self {
|
||||
command,
|
||||
@@ -74,7 +74,7 @@ impl UnifiedExecRequest {
|
||||
env,
|
||||
with_escalated_permissions,
|
||||
justification,
|
||||
exec_approval_requirement,
|
||||
approval_requirement,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -132,7 +132,9 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
req.exec_approval_requirement.allow_prefix().cloned(),
|
||||
req.approval_requirement
|
||||
.proposed_execpolicy_amendment()
|
||||
.cloned(),
|
||||
)
|
||||
.await
|
||||
})
|
||||
@@ -140,18 +142,15 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
})
|
||||
}
|
||||
|
||||
fn exec_approval_requirement(
|
||||
&self,
|
||||
req: &UnifiedExecRequest,
|
||||
) -> Option<ExecApprovalRequirement> {
|
||||
Some(req.exec_approval_requirement.clone())
|
||||
fn approval_requirement(&self, req: &UnifiedExecRequest) -> Option<ApprovalRequirement> {
|
||||
Some(req.approval_requirement.clone())
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride {
|
||||
if req.with_escalated_permissions.unwrap_or(false)
|
||||
|| matches!(
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
req.approval_requirement,
|
||||
ApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
}
|
||||
)
|
||||
|
||||
@@ -88,28 +88,29 @@ pub(crate) struct ApprovalCtx<'a> {
|
||||
|
||||
// Specifies what tool orchestrator should do with a given tool call.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub(crate) enum ExecApprovalRequirement {
|
||||
pub(crate) enum ApprovalRequirement {
|
||||
/// No approval required for this tool call.
|
||||
Skip {
|
||||
/// The first attempt should skip sandboxing (e.g., when explicitly
|
||||
/// greenlit by policy).
|
||||
bypass_sandbox: bool,
|
||||
},
|
||||
/// Approval required for this tool call.
|
||||
/// Approval required for this tool call
|
||||
NeedsApproval {
|
||||
reason: Option<String>,
|
||||
/// Prefix that can be whitelisted via execpolicy to skip future approvals for similar commands
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
/// Proposed execpolicy amendment to skip future approvals for similar commands
|
||||
/// See core/src/exec_policy.rs for more details on how proposed_execpolicy_amendment is determined.
|
||||
proposed_execpolicy_amendment: Option<Vec<String>>,
|
||||
},
|
||||
/// Execution forbidden for this tool call.
|
||||
/// Execution forbidden for this tool call
|
||||
Forbidden { reason: String },
|
||||
}
|
||||
|
||||
impl ExecApprovalRequirement {
|
||||
pub fn allow_prefix(&self) -> Option<&Vec<String>> {
|
||||
impl ApprovalRequirement {
|
||||
pub fn proposed_execpolicy_amendment(&self) -> Option<&Vec<String>> {
|
||||
match self {
|
||||
Self::NeedsApproval {
|
||||
allow_prefix: Some(prefix),
|
||||
proposed_execpolicy_amendment: Some(prefix),
|
||||
..
|
||||
} => Some(prefix),
|
||||
_ => None,
|
||||
@@ -120,10 +121,10 @@ impl ExecApprovalRequirement {
|
||||
/// - Never, OnFailure: do not ask
|
||||
/// - OnRequest: ask unless sandbox policy is DangerFullAccess
|
||||
/// - UnlessTrusted: always ask
|
||||
pub(crate) fn default_exec_approval_requirement(
|
||||
pub(crate) fn default_approval_requirement(
|
||||
policy: AskForApproval,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
) -> ExecApprovalRequirement {
|
||||
) -> ApprovalRequirement {
|
||||
let needs_approval = match policy {
|
||||
AskForApproval::Never | AskForApproval::OnFailure => false,
|
||||
AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess),
|
||||
@@ -131,12 +132,12 @@ pub(crate) fn default_exec_approval_requirement(
|
||||
};
|
||||
|
||||
if needs_approval {
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
ApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
} else {
|
||||
ExecApprovalRequirement::Skip {
|
||||
ApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
}
|
||||
}
|
||||
@@ -168,9 +169,10 @@ pub(crate) trait Approvable<Req> {
|
||||
matches!(policy, AskForApproval::Never)
|
||||
}
|
||||
|
||||
/// Return `Some(_)` to specify a custom exec approval requirement, or `None`
|
||||
/// to fall back to policy-based default.
|
||||
fn exec_approval_requirement(&self, _req: &Req) -> Option<ExecApprovalRequirement> {
|
||||
/// Override the default approval requirement. Return `Some(_)` to specify
|
||||
/// a custom requirement, or `None` to fall back to
|
||||
/// policy-based default.
|
||||
fn approval_requirement(&self, _req: &Req) -> Option<ApprovalRequirement> {
|
||||
None
|
||||
}
|
||||
|
||||
|
||||
@@ -15,7 +15,7 @@ use crate::codex::TurnContext;
|
||||
use crate::exec::ExecToolCallOutput;
|
||||
use crate::exec::StreamOutput;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::exec_policy::create_exec_approval_requirement_for_command;
|
||||
use crate::exec_policy::create_approval_requirement_for_command;
|
||||
use crate::protocol::BackgroundEventEvent;
|
||||
use crate::protocol::EventMsg;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
@@ -555,7 +555,7 @@ impl UnifiedExecSessionManager {
|
||||
let features = context.session.features().await;
|
||||
let mut orchestrator = ToolOrchestrator::new();
|
||||
let mut runtime = UnifiedExecRuntime::new(self);
|
||||
let exec_approval_requirement = create_exec_approval_requirement_for_command(
|
||||
let approval_requirement = create_approval_requirement_for_command(
|
||||
&context.turn.exec_policy,
|
||||
&features,
|
||||
command,
|
||||
@@ -570,7 +570,7 @@ impl UnifiedExecSessionManager {
|
||||
env,
|
||||
with_escalated_permissions,
|
||||
justification,
|
||||
exec_approval_requirement,
|
||||
approval_requirement,
|
||||
);
|
||||
let tool_ctx = ToolCtx {
|
||||
session: context.session.as_ref(),
|
||||
|
||||
@@ -1559,29 +1559,28 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(unix)]
|
||||
async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Result<()> {
|
||||
async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::UnlessTrusted;
|
||||
let sandbox_policy = SandboxPolicy::ReadOnly;
|
||||
let sandbox_policy = SandboxPolicy::DangerFullAccess;
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.approval_policy = approval_policy;
|
||||
config.sandbox_policy = sandbox_policy_for_config;
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let allow_prefix_path = test.cwd.path().join("allow-prefix.txt");
|
||||
let _ = fs::remove_file(&allow_prefix_path);
|
||||
|
||||
let call_id_first = "allow-prefix-first";
|
||||
let (first_event, expected_command) = ActionKind::RunCommand {
|
||||
command: "touch allow-prefix.txt",
|
||||
command: "printf allow-prefix-ok",
|
||||
}
|
||||
.prepare(&test, &server, call_id_first, false)
|
||||
.await?;
|
||||
let expected_command =
|
||||
expected_command.expect("allow prefix scenario should produce a shell command");
|
||||
let expected_allow_prefix = vec!["touch".to_string(), "allow-prefix.txt".to_string()];
|
||||
expected_command.expect("execpolicy amendment scenario should produce a shell command");
|
||||
let expected_execpolicy_amendment = expected_command
|
||||
.split_whitespace()
|
||||
.map(ToString::to_string)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
let _ = mount_sse_once(
|
||||
&server,
|
||||
@@ -1610,13 +1609,16 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re
|
||||
.await?;
|
||||
|
||||
let approval = expect_exec_approval(&test, expected_command.as_str()).await;
|
||||
assert_eq!(approval.allow_prefix, Some(expected_allow_prefix.clone()));
|
||||
assert_eq!(
|
||||
approval.proposed_execpolicy_amendment,
|
||||
Some(expected_execpolicy_amendment.clone())
|
||||
);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: "0".into(),
|
||||
decision: ReviewDecision::ApprovedAllowPrefix {
|
||||
allow_prefix: expected_allow_prefix.clone(),
|
||||
decision: ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: expected_execpolicy_amendment.clone(),
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1626,7 +1628,7 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re
|
||||
let policy_contents = fs::read_to_string(&policy_path)?;
|
||||
assert!(
|
||||
policy_contents
|
||||
.contains(r#"prefix_rule(pattern=["touch", "allow-prefix.txt"], decision="allow")"#),
|
||||
.contains(r#"prefix_rule(pattern=["printf", "allow-prefix-ok"], decision="allow")"#),
|
||||
"unexpected policy contents: {policy_contents}"
|
||||
);
|
||||
|
||||
@@ -1637,19 +1639,14 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re
|
||||
);
|
||||
assert_eq!(first_output.exit_code.unwrap_or(0), 0);
|
||||
assert!(
|
||||
first_output.stdout.is_empty(),
|
||||
first_output.stdout.contains("allow-prefix-ok"),
|
||||
"unexpected stdout: {}",
|
||||
first_output.stdout
|
||||
);
|
||||
assert_eq!(
|
||||
fs::read_to_string(&allow_prefix_path)?,
|
||||
"",
|
||||
"unexpected file contents after first run"
|
||||
);
|
||||
|
||||
let call_id_second = "allow-prefix-second";
|
||||
let (second_event, second_command) = ActionKind::RunCommand {
|
||||
command: "touch allow-prefix.txt",
|
||||
command: "printf allow-prefix-ok",
|
||||
}
|
||||
.prepare(&test, &server, call_id_second, false)
|
||||
.await?;
|
||||
@@ -1690,15 +1687,10 @@ async fn approving_allow_prefix_persists_policy_and_skips_future_prompts() -> Re
|
||||
);
|
||||
assert_eq!(second_output.exit_code.unwrap_or(0), 0);
|
||||
assert!(
|
||||
second_output.stdout.is_empty(),
|
||||
second_output.stdout.contains("allow-prefix-ok"),
|
||||
"unexpected stdout: {}",
|
||||
second_output.stdout
|
||||
);
|
||||
assert_eq!(
|
||||
fs::read_to_string(&allow_prefix_path)?,
|
||||
"",
|
||||
"unexpected file contents after second run"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -180,7 +180,7 @@ async fn run_codex_tool_session_inner(
|
||||
call_id,
|
||||
reason: _,
|
||||
risk,
|
||||
allow_prefix: _,
|
||||
proposed_execpolicy_amendment: _,
|
||||
parsed_cmd,
|
||||
}) => {
|
||||
handle_exec_approval_request(
|
||||
|
||||
@@ -51,10 +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.
|
||||
/// Proposed execpolicy amendment that can be applied to allow future runs.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional, type = "Array<string>")]
|
||||
pub allow_prefix: Option<Vec<String>>,
|
||||
pub proposed_execpolicy_amendment: Option<Vec<String>>,
|
||||
pub parsed_cmd: Vec<ParsedCommand>,
|
||||
}
|
||||
|
||||
|
||||
@@ -1652,9 +1652,11 @@ pub enum ReviewDecision {
|
||||
/// User has approved this command and the agent should execute it.
|
||||
Approved,
|
||||
|
||||
/// User has approved this command and wants to add the command prefix to
|
||||
/// the execpolicy allow list so future matching commands are permitted.
|
||||
ApprovedAllowPrefix { allow_prefix: Vec<String> },
|
||||
/// User has approved this command and wants to apply the proposed execpolicy
|
||||
/// amendment so future matching commands are permitted.
|
||||
ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: Vec<String>,
|
||||
},
|
||||
|
||||
/// User has approved this command and wants to automatically approve any
|
||||
/// future identical instances (`command` and `cwd` match exactly) for the
|
||||
|
||||
@@ -43,7 +43,7 @@ pub(crate) enum ApprovalRequest {
|
||||
command: Vec<String>,
|
||||
reason: Option<String>,
|
||||
risk: Option<SandboxCommandAssessment>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
proposed_execpolicy_amendment: Option<Vec<String>>,
|
||||
},
|
||||
ApplyPatch {
|
||||
id: String,
|
||||
@@ -105,8 +105,11 @@ impl ApprovalOverlay {
|
||||
header: Box<dyn Renderable>,
|
||||
) -> (Vec<ApprovalOption>, SelectionViewParams) {
|
||||
let (options, title) = match &variant {
|
||||
ApprovalVariant::Exec { allow_prefix, .. } => (
|
||||
exec_options(allow_prefix.clone()),
|
||||
ApprovalVariant::Exec {
|
||||
proposed_execpolicy_amendment,
|
||||
..
|
||||
} => (
|
||||
exec_options(proposed_execpolicy_amendment.clone()),
|
||||
"Would you like to run the following command?".to_string(),
|
||||
),
|
||||
ApprovalVariant::ApplyPatch { .. } => (
|
||||
@@ -337,7 +340,7 @@ impl From<ApprovalRequest> for ApprovalRequestState {
|
||||
command,
|
||||
reason,
|
||||
risk,
|
||||
allow_prefix,
|
||||
proposed_execpolicy_amendment,
|
||||
} => {
|
||||
let reason = reason.filter(|item| !item.is_empty());
|
||||
let has_reason = reason.is_some();
|
||||
@@ -360,7 +363,7 @@ impl From<ApprovalRequest> for ApprovalRequestState {
|
||||
variant: ApprovalVariant::Exec {
|
||||
id,
|
||||
command,
|
||||
allow_prefix,
|
||||
proposed_execpolicy_amendment,
|
||||
},
|
||||
header: Box::new(Paragraph::new(header).wrap(Wrap { trim: false })),
|
||||
}
|
||||
@@ -437,7 +440,7 @@ enum ApprovalVariant {
|
||||
Exec {
|
||||
id: String,
|
||||
command: Vec<String>,
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
proposed_execpolicy_amendment: Option<Vec<String>>,
|
||||
},
|
||||
ApplyPatch {
|
||||
id: String,
|
||||
@@ -470,7 +473,7 @@ impl ApprovalOption {
|
||||
}
|
||||
}
|
||||
|
||||
fn exec_options(allow_prefix: Option<Vec<String>>) -> Vec<ApprovalOption> {
|
||||
fn exec_options(proposed_execpolicy_amendment: Option<Vec<String>>) -> Vec<ApprovalOption> {
|
||||
vec![
|
||||
ApprovalOption {
|
||||
label: "Yes, proceed".to_string(),
|
||||
@@ -486,10 +489,10 @@ fn exec_options(allow_prefix: Option<Vec<String>>) -> Vec<ApprovalOption> {
|
||||
},
|
||||
]
|
||||
.into_iter()
|
||||
.chain(allow_prefix.map(|prefix| ApprovalOption {
|
||||
.chain(proposed_execpolicy_amendment.map(|prefix| ApprovalOption {
|
||||
label: "Yes, and don't ask again for commands with this prefix".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix {
|
||||
allow_prefix: prefix,
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: prefix,
|
||||
}),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))],
|
||||
@@ -556,7 +559,7 @@ mod tests {
|
||||
command: vec!["echo".to_string(), "hi".to_string()],
|
||||
reason: Some("reason".to_string()),
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -590,7 +593,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_prefix_option_emits_allow_prefix() {
|
||||
fn exec_prefix_option_emits_execpolicy_amendment() {
|
||||
let (tx, mut rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let mut view = ApprovalOverlay::new(
|
||||
@@ -599,7 +602,7 @@ mod tests {
|
||||
command: vec!["echo".to_string()],
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: Some(vec!["echo".to_string()]),
|
||||
proposed_execpolicy_amendment: Some(vec!["echo".to_string()]),
|
||||
},
|
||||
tx,
|
||||
);
|
||||
@@ -609,8 +612,8 @@ mod tests {
|
||||
if let AppEvent::CodexOp(Op::ExecApproval { decision, .. }) = ev {
|
||||
assert_eq!(
|
||||
decision,
|
||||
ReviewDecision::ApprovedAllowPrefix {
|
||||
allow_prefix: vec!["echo".to_string()]
|
||||
ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: vec!["echo".to_string()]
|
||||
}
|
||||
);
|
||||
saw_op = true;
|
||||
@@ -633,7 +636,7 @@ mod tests {
|
||||
command,
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
};
|
||||
|
||||
let view = ApprovalOverlay::new(exec_request, tx);
|
||||
|
||||
@@ -563,7 +563,7 @@ mod tests {
|
||||
command: vec!["echo".into(), "ok".into()],
|
||||
reason: None,
|
||||
risk: None,
|
||||
allow_prefix: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1069,7 +1069,7 @@ impl ChatWidget {
|
||||
command: ev.command,
|
||||
reason: ev.reason,
|
||||
risk: ev.risk,
|
||||
allow_prefix: ev.allow_prefix,
|
||||
proposed_execpolicy_amendment: ev.proposed_execpolicy_amendment,
|
||||
};
|
||||
self.bottom_pane.push_approval_request(request);
|
||||
self.request_redraw();
|
||||
|
||||
@@ -677,7 +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,
|
||||
proposed_execpolicy_amendment: None,
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -722,7 +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,
|
||||
proposed_execpolicy_amendment: None,
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -773,7 +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,
|
||||
proposed_execpolicy_amendment: None,
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -1972,7 +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()]),
|
||||
proposed_execpolicy_amendment: Some(vec!["echo".into(), "hello".into(), "world".into()]),
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -2019,7 +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()]),
|
||||
proposed_execpolicy_amendment: Some(vec!["echo".into(), "hello".into(), "world".into()]),
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
@@ -2233,7 +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()]),
|
||||
proposed_execpolicy_amendment: Some(vec!["echo".into(), "hello world".into()]),
|
||||
parsed_cmd: vec![],
|
||||
};
|
||||
chat.handle_codex_event(Event {
|
||||
|
||||
@@ -408,7 +408,7 @@ pub fn new_approval_decision_cell(
|
||||
],
|
||||
)
|
||||
}
|
||||
ApprovedAllowPrefix { .. } => {
|
||||
ApprovedExecpolicyAmendment { .. } => {
|
||||
let snippet = Span::from(exec_snippet(&command)).dim();
|
||||
(
|
||||
"✔ ".green(),
|
||||
@@ -417,7 +417,7 @@ pub fn new_approval_decision_cell(
|
||||
"approved".bold(),
|
||||
" codex to run ".into(),
|
||||
snippet,
|
||||
" and added its prefix to your allow list".bold(),
|
||||
" and applied the execpolicy amendment".bold(),
|
||||
],
|
||||
)
|
||||
}
|
||||
|
||||
+1
-1
@@ -603,7 +603,7 @@ metadata above):
|
||||
- `codex.tool_decision`
|
||||
- `tool_name`
|
||||
- `call_id`
|
||||
- `decision` (`approved`, `approved_allow_prefix`, `approved_for_session`, `denied`, or `abort`)
|
||||
- `decision` (`approved`, `approved_execpolicy_amendment`, `approved_for_session`, `denied`, or `abort`)
|
||||
- `source` (`config` or `user`)
|
||||
- `codex.tool_result`
|
||||
- `tool_name`
|
||||
|
||||
+2
-2
@@ -53,10 +53,10 @@ When no rules match, `matchedRules` is an empty array and `decision` is omitted.
|
||||
|
||||
```json
|
||||
{
|
||||
"matchedRules": [],
|
||||
"matchedRules": []
|
||||
}
|
||||
```
|
||||
|
||||
## Status
|
||||
|
||||
`execpolicy` commands are still in preview. The API may have breaking changes in the future.
|
||||
```
|
||||
|
||||
Reference in New Issue
Block a user