Skip to content

fix(exec_policy) heredoc parsing file_redirect #20113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dylan-hurd-oai merged 9 commits into from
May 1, 2026
Merged

fix(exec_policy) heredoc parsing file_redirect #20113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
be8e4e4
fix(exec_policy) heredoc parsing file_redirect
dylan-hurd-oai Apr 29, 2026
4e677e0
rm extra test
dylan-hurd-oai Apr 29, 2026
d175375
simplify
dylan-hurd-oai Apr 29, 2026
5cba368
integraiton test
dylan-hurd-oai Apr 29, 2026
54ad74d
env vars and amendments
dylan-hurd-oai Apr 29, 2026
f3e0a64
test(exec-policy): skip heredoc assignment regression on windows
dylan-hurd-oai Apr 29, 2026
9ca8c57
test formatting
dylan-hurd-oai Apr 30, 2026
0df6562
test formating
dylan-hurd-oai Apr 30, 2026
4ea9341
Merge branch 'main' into dh--heredoc-redirect-patch
dylan-hurd-oai May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions codex-rs/core/src/exec_policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,14 +271,18 @@ impl ExecPolicyManager {
&match_options,
);

let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule(
prefix_rule.as_ref(),
&evaluation.matched_rules,
exec_policy.as_ref(),
&commands,
&exec_policy_fallback,
&match_options,
);
let requested_amendment = if auto_amendment_allowed {
derive_requested_execpolicy_amendment_from_prefix_rule(
prefix_rule.as_ref(),
&evaluation.matched_rules,
exec_policy.as_ref(),
&commands,
&exec_policy_fallback,
&match_options,
)
} else {
None
};

match evaluation.decision {
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
Expand Down
121 changes: 121 additions & 0 deletions codex-rs/core/src/exec_policy_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,127 @@ async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_wont_mat
.await;
}

#[tokio::test]
async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_matches() {
assert_exec_approval_requirement_for_command(
ExecApprovalRequirementScenario {
policy_src: None,
command: vec![
"bash".to_string(),
"-lc".to_string(),
"python3 <<'PY'\nprint('hello')\nPY".to_string(),
],
approval_policy: AskForApproval::UnlessTrusted,
sandbox_policy: SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
prefix_rule: Some(vec!["python3".to_string()]),
},
ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
},
)
.await;
}

#[tokio::test]
#[cfg(not(windows))]
async fn heredoc_with_variable_assignment_is_not_reduced_to_allowed_prefix() {
assert_exec_approval_requirement_for_command(
ExecApprovalRequirementScenario {
policy_src: Some(r#"prefix_rule(pattern=["cat"], decision="allow")"#.to_string()),
command: vec![
"bash".to_string(),
"-lc".to_string(),
"PATH=/tmp/evil:$PATH cat <<'EOF'\nhello\nEOF".to_string(),
],
approval_policy: AskForApproval::OnRequest,
sandbox_policy: SandboxPolicy::new_read_only_policy(),
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
prefix_rule: None,
},
ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"bash".to_string(),
"-lc".to_string(),
"PATH=/tmp/evil:$PATH cat <<'EOF'\nhello\nEOF".to_string(),
])),
},
)
.await;
}

#[tokio::test]
async fn heredoc_redirect_without_escalation_runs_inside_sandbox() {
assert_exec_approval_requirement_for_command(
ExecApprovalRequirementScenario {
policy_src: None,
command: vec![
"zsh".to_string(),
"-lc".to_string(),
r#"cat <<'EOF' > /some/important/folder/test.txt
hello world
EOF"#
.to_string(),
],
approval_policy: AskForApproval::OnRequest,
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::UseDefault,
prefix_rule: None,
},
ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"zsh".to_string(),
"-lc".to_string(),
r#"cat <<'EOF' > /some/important/folder/test.txt
hello world
EOF"#
.to_string(),
])),
},
)
.await;
}

#[tokio::test]
async fn heredoc_redirect_with_escalation_requires_approval() {
assert_exec_approval_requirement_for_command(
ExecApprovalRequirementScenario {
policy_src: Some(r#"prefix_rule(pattern=["cat"], decision="allow")"#.to_string()),
command: vec![
"zsh".to_string(),
"-lc".to_string(),
r#"cat <<'EOF' > /some/important/folder/test.txt
hello world
EOF"#
.to_string(),
],
approval_policy: AskForApproval::OnRequest,
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(),
sandbox_permissions: SandboxPermissions::RequireEscalated,
prefix_rule: None,
},
ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"zsh".to_string(),
"-lc".to_string(),
r#"cat <<'EOF' > /some/important/folder/test.txt
hello world
EOF"#
.to_string(),
])),
},
)
.await;
}

#[tokio::test]
async fn justification_is_included_in_forbidden_exec_approval_requirement() {
assert_exec_approval_requirement_for_command(
Expand Down
162 changes: 161 additions & 1 deletion codex-rs/core/tests/suite/approvals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ enum ActionKind {
RunCommand {
command: &'static str,
},
RunCommandWithPolicy {
command: &'static str,
policy_src: &'static str,
},
RunCommandWithPrefixRule {
command: &'static str,
prefix_rule: &'static [&'static str],
},
RunUnifiedExecCommand {
command: &'static str,
justification: Option<&'static str>,
Expand All @@ -114,6 +122,20 @@ const DEFAULT_UNIFIED_EXEC_JUSTIFICATION: &str =
"Requires escalated permissions to bypass the sandbox in tests.";

impl ActionKind {
fn policy_src(&self) -> Option<&'static str> {
match self {
ActionKind::RunCommandWithPolicy { policy_src, .. } => Some(*policy_src),
ActionKind::WriteFile { .. }
| ActionKind::FetchUrlNoProxy { .. }
| ActionKind::FetchUrl { .. }
| ActionKind::RunCommand { .. }
| ActionKind::RunCommandWithPrefixRule { .. }
| ActionKind::RunUnifiedExecCommand { .. }
| ActionKind::ApplyPatchFunction { .. }
| ActionKind::ApplyPatchShell { .. } => None,
}
}

async fn prepare(
&self,
test: &TestCodex,
Expand Down Expand Up @@ -204,6 +226,31 @@ impl ActionKind {
)?;
Ok((event, Some(command.to_string())))
}
ActionKind::RunCommandWithPolicy { command, .. } => {
// Bazel Linux runners can be heavily oversubscribed while this

@bolinfest bolinfest Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadpanda

// matrix runs, so avoid making scheduling latency look like an
// approval behavior failure.
let event = shell_event(
call_id,
command,
/*timeout_ms*/ 30_000,
sandbox_permissions,
)?;
Ok((event, Some(command.to_string())))
}
ActionKind::RunCommandWithPrefixRule {
command,
prefix_rule,
} => {
let event = shell_event_with_prefix_rule(
call_id,
command,
/*timeout_ms*/ 30_000,
sandbox_permissions,
Some(prefix_rule.iter().map(|part| (*part).to_string()).collect()),
)?;
Ok((event, Some(command.to_string())))
}
ActionKind::RunUnifiedExecCommand {
command,
justification,
Expand Down Expand Up @@ -549,6 +596,11 @@ enum Outcome {
decision: ReviewDecision,
expected_reason: Option<&'static str>,
},
ExecApprovalWithAmendment {
decision: ReviewDecision,
expected_reason: Option<&'static str>,
expected_execpolicy_amendment: Option<&'static [&'static str]>,
},
PatchApproval {
decision: ReviewDecision,
expected_reason: Option<&'static str>,
Expand Down Expand Up @@ -913,6 +965,70 @@ fn scenarios() -> Vec<ScenarioSpec> {
output_contains: "rejected by user",
},
},
ScenarioSpec {
name: "cat_heredoc_file_redirect_prefix_rule_requires_escalation_approval",
approval_policy: OnRequest,
sandbox_policy: workspace_write(false),
action: ActionKind::RunCommandWithPrefixRule {
command: r#"cat <<'EOF' > /tmp/out.txt
hello
EOF"#,
prefix_rule: &["cat"],
},
sandbox_permissions: SandboxPermissions::RequireEscalated,
features: vec![],
model_override: Some("gpt-5.2"),
outcome: Outcome::ExecApproval {
decision: ReviewDecision::Denied,
expected_reason: None,
},
expectation: Expectation::CommandFailure {
output_contains: "rejected by user",
},
},
ScenarioSpec {
name: "cat_heredoc_variable_assignment_policy_requires_escalation_approval",
approval_policy: OnRequest,
sandbox_policy: workspace_write(false),
action: ActionKind::RunCommandWithPolicy {
command: r#"PATH=/tmp/evil:$PATH cat <<'EOF'
hello
EOF"#,
policy_src: r#"prefix_rule(pattern=["cat"], decision="allow")"#,
},
sandbox_permissions: SandboxPermissions::RequireEscalated,
features: vec![],
model_override: Some("gpt-5.2"),
outcome: Outcome::ExecApproval {
decision: ReviewDecision::Denied,
expected_reason: None,
},
expectation: Expectation::CommandFailure {
output_contains: "rejected by user",
},
},
ScenarioSpec {
name: "python_heredoc_requested_prefix_rule_omits_amendment",
approval_policy: OnRequest,
sandbox_policy: workspace_write(false),
action: ActionKind::RunCommandWithPrefixRule {
command: r#"python3 <<'PY'
print('hello')
PY"#,
prefix_rule: &["python3"],
},
sandbox_permissions: SandboxPermissions::RequireEscalated,
features: vec![],
model_override: Some("gpt-5.2"),
outcome: Outcome::ExecApprovalWithAmendment {
decision: ReviewDecision::Denied,
expected_reason: None,
expected_execpolicy_amendment: None,
},
expectation: Expectation::CommandFailure {
output_contains: "rejected by user",
},
},
ScenarioSpec {
name: "danger_full_access_on_failure_allows_outside_write",
approval_policy: OnFailure,
Expand Down Expand Up @@ -1703,7 +1819,9 @@ fn scenario_group(scenario: &ScenarioSpec) -> ScenarioGroup {
ActionKind::WriteFile { .. }
| ActionKind::FetchUrlNoProxy { .. }
| ActionKind::FetchUrl { .. }
| ActionKind::RunCommand { .. } => match &scenario.sandbox_policy {
| ActionKind::RunCommand { .. }
| ActionKind::RunCommandWithPolicy { .. }
| ActionKind::RunCommandWithPrefixRule { .. } => match &scenario.sandbox_policy {
SandboxPolicy::DangerFullAccess => ScenarioGroup::DangerFullAccess,
SandboxPolicy::ReadOnly { .. } => ScenarioGroup::ReadOnly,
SandboxPolicy::WorkspaceWrite { .. } => ScenarioGroup::WorkspaceWrite,
Expand All @@ -1720,6 +1838,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
let features = scenario.features.clone();
let model_override = scenario.model_override;
let model = model_override.unwrap_or("gpt-5.4");
let policy_src = scenario.action.policy_src();

let mut builder = test_codex().with_model(model).with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
Expand All @@ -1733,6 +1852,13 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
.expect("test config should allow feature update");
}
});
if let Some(policy_src) = policy_src {
builder = builder.with_pre_build_hook(move |home| {
let rules_dir = home.join("rules");
fs::create_dir_all(&rules_dir).expect("create rules dir");
fs::write(rules_dir.join("default.rules"), policy_src).expect("write policy");
});
}
let test = builder.build(&server).await?;

let call_id = scenario.name;
Expand Down Expand Up @@ -1799,6 +1925,40 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
.await?;
wait_for_completion(&test).await;
}
Outcome::ExecApprovalWithAmendment {
decision,
expected_reason,
expected_execpolicy_amendment,
} => {
let command = expected_command
.as_deref()
.expect("exec approval requires shell command");
let approval = expect_exec_approval(&test, command).await;
if let Some(expected_reason) = expected_reason {
assert_eq!(
approval.reason.as_deref(),
Some(*expected_reason),
"unexpected approval reason for {}",
scenario.name
);
}
let expected_execpolicy_amendment = expected_execpolicy_amendment.map(|command| {
ExecPolicyAmendment::new(command.iter().map(|part| (*part).to_string()).collect())
});
assert_eq!(
approval.proposed_execpolicy_amendment, expected_execpolicy_amendment,
"unexpected execpolicy amendment for {}",
scenario.name
);
test.codex
.submit(Op::ExecApproval {
id: approval.effective_approval_id(),
turn_id: None,
decision: decision.clone(),
})
.await?;
wait_for_completion(&test).await;
}
Outcome::PatchApproval {
decision,
expected_reason,
Expand Down
Loading
Loading