Skip to content

Commit bd130e4

Browse files
zhao-oaizhao-oai
committed
proposing execpolicy amendment when prompting due to sandbox denial
1 parent a8cbbdb commit bd130e4

File tree

5 files changed

+142
-33
lines changed

5 files changed

+142
-33
lines changed

‎codex-rs/core/src/exec_policy.rs‎

Lines changed: 101 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -147,41 +147,54 @@ pub(crate) async fn append_execpolicy_amendment_and_update(
147147
Ok(())
148148
}
149149

150-
/// Returns a proposed execpolicy amendment only when heuristics caused
151-
/// the prompt decision, so we can offer to apply that amendment for future runs.
152-
///
153-
/// The amendment uses the first command heuristics marked as `Prompt`. If any explicit
154-
/// execpolicy rule also prompts, we return `None` because applying the amendment would not
155-
/// skip that policy requirement.
156-
///
157-
/// Examples:
158-
/// - execpolicy: empty. Command: `["python"]`. Heuristics prompt -> `Some(vec!["python"])`.
159-
/// - execpolicy: empty. Command: `["bash", "-c", "cd /some/folder && prog1 --option1 arg1 && prog2 --option2 arg2"]`.
160-
/// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. If heuristics allow `cd` but prompt
161-
/// on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`.
162-
/// - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above,
163-
/// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"].
164-
fn proposed_execpolicy_amendment(evaluation: &Evaluation) -> Option<ExecPolicyAmendment> {
165-
if evaluation.decision != Decision::Prompt {
150+
// - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement.
151+
// - Otherwise return the first heuristics Prompt.
152+
// Examples:
153+
// - execpolicy: empty. Command: `["python"]`. Heuristics prompt -> `Some(vec!["python"])`.
154+
// - execpolicy: empty. Command: `["bash", "-c", "cd /some/folder && prog1 --option1 arg1 && prog2 --option2 arg2"]`.
155+
// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. If heuristics allow `cd` but prompt
156+
// on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`.
157+
// - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above,
158+
// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"].
159+
fn prompt_execpolicy_amendment(matched_rules: &[RuleMatch]) -> Option<ExecPolicyAmendment> {
160+
if matched_rules.iter().any(|rule_match| {
161+
!matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })
162+
&& rule_match.decision() == Decision::Prompt
163+
}) {
166164
return None;
167165
}
168166

169-
let mut first_prompt_from_heuristics: Option<Vec<String>> = None;
170-
for rule_match in &evaluation.matched_rules {
171-
match rule_match {
172-
RuleMatch::HeuristicsRuleMatch { command, decision } => {
173-
if *decision == Decision::Prompt && first_prompt_from_heuristics.is_none() {
174-
first_prompt_from_heuristics = Some(command.clone());
175-
}
176-
}
177-
_ if rule_match.decision() == Decision::Prompt => {
178-
return None;
179-
}
180-
_ => {}
181-
}
167+
matched_rules
168+
.iter()
169+
.find_map(|rule_match| match rule_match {
170+
RuleMatch::HeuristicsRuleMatch {
171+
command,
172+
decision: Decision::Prompt,
173+
} => Some(ExecPolicyAmendment::from(command.clone())),
174+
_ => None,
175+
})
176+
}
177+
178+
// - Note: we only use this amendment when the command fails to run in sandbox and codex prompts the user to run outside the sandbox
179+
// - The purpose of this amendment is to bypass sandbox for similar commands in the future
180+
// - If any execpolicy rule matches, return None, because we would already be running command outside the sandbox
181+
fn allow_execpolicy_amendment(matched_rules: &[RuleMatch]) -> Option<ExecPolicyAmendment> {
182+
if matched_rules
183+
.iter()
184+
.any(|rule_match| !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. }))
185+
{
186+
return None;
182187
}
183188

184-
first_prompt_from_heuristics.map(ExecPolicyAmendment::from)
189+
matched_rules
190+
.iter()
191+
.find_map(|rule_match| match rule_match {
192+
RuleMatch::HeuristicsRuleMatch {
193+
command,
194+
decision: Decision::Allow,
195+
} => Some(ExecPolicyAmendment::from(command.clone())),
196+
_ => None,
197+
})
185198
}
186199

187200
/// Only return PROMPT_REASON when an execpolicy rule drove the prompt decision.
@@ -233,7 +246,7 @@ pub(crate) async fn create_exec_approval_requirement_for_command(
233246
ExecApprovalRequirement::NeedsApproval {
234247
reason: derive_prompt_reason(&evaluation),
235248
proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) {
236-
proposed_execpolicy_amendment(&evaluation)
249+
prompt_execpolicy_amendment(&evaluation.matched_rules)
237250
} else {
238251
None
239252
},
@@ -242,6 +255,11 @@ pub(crate) async fn create_exec_approval_requirement_for_command(
242255
}
243256
Decision::Allow => ExecApprovalRequirement::Skip {
244257
bypass_sandbox: has_policy_allow,
258+
proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) {
259+
allow_execpolicy_amendment(&evaluation.matched_rules)
260+
} else {
261+
None
262+
},
245263
},
246264
}
247265
}
@@ -730,4 +748,56 @@ prefix_rule(pattern=["rm"], decision="forbidden")
730748
}
731749
);
732750
}
751+
752+
#[tokio::test]
753+
async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() {
754+
let command = vec!["echo".to_string(), "safe".to_string()];
755+
756+
let requirement = create_exec_approval_requirement_for_command(
757+
&Arc::new(RwLock::new(Policy::empty())),
758+
&Features::with_defaults(),
759+
&command,
760+
AskForApproval::OnRequest,
761+
&SandboxPolicy::ReadOnly,
762+
SandboxPermissions::UseDefault,
763+
)
764+
.await;
765+
766+
assert_eq!(
767+
requirement,
768+
ExecApprovalRequirement::Skip {
769+
bypass_sandbox: false,
770+
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)),
771+
}
772+
);
773+
}
774+
775+
#[tokio::test]
776+
async fn proposed_execpolicy_amendment_is_suppressed_when_policy_matches_allow() {
777+
let policy_src = r#"prefix_rule(pattern=["echo"], decision="allow")"#;
778+
let mut parser = PolicyParser::new();
779+
parser
780+
.parse("test.codexpolicy", policy_src)
781+
.expect("parse policy");
782+
let policy = Arc::new(RwLock::new(parser.build()));
783+
let command = vec!["echo".to_string(), "safe".to_string()];
784+
785+
let requirement = create_exec_approval_requirement_for_command(
786+
&policy,
787+
&Features::with_defaults(),
788+
&command,
789+
AskForApproval::OnRequest,
790+
&SandboxPolicy::ReadOnly,
791+
SandboxPermissions::UseDefault,
792+
)
793+
.await;
794+
795+
assert_eq!(
796+
requirement,
797+
ExecApprovalRequirement::Skip {
798+
bypass_sandbox: true,
799+
proposed_execpolicy_amendment: None,
800+
}
801+
);
802+
}
733803
}

‎codex-rs/core/src/tools/runtimes/shell.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,8 @@ impl Approvable<ShellRequest> for ShellRuntime {
133133
|| matches!(
134134
req.exec_approval_requirement,
135135
ExecApprovalRequirement::Skip {
136-
bypass_sandbox: true
136+
bypass_sandbox: true,
137+
..
137138
}
138139
)
139140
{

‎codex-rs/core/src/tools/runtimes/unified_exec.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,8 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
154154
|| matches!(
155155
req.exec_approval_requirement,
156156
ExecApprovalRequirement::Skip {
157-
bypass_sandbox: true
157+
bypass_sandbox: true,
158+
..
158159
}
159160
)
160161
{

‎codex-rs/core/src/tools/sandboxing.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,9 @@ pub(crate) enum ExecApprovalRequirement {
9595
/// The first attempt should skip sandboxing (e.g., when explicitly
9696
/// greenlit by policy).
9797
bypass_sandbox: bool,
98+
/// Proposed execpolicy amendment to skip future approvals for similar commands
99+
/// Only applies if the command fails to run in sandbox and codex prompts the user to run outside the sandbox.
100+
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
98101
},
99102
/// Approval required for this tool call.
100103
NeedsApproval {
@@ -114,6 +117,10 @@ impl ExecApprovalRequirement {
114117
proposed_execpolicy_amendment: Some(prefix),
115118
..
116119
} => Some(prefix),
120+
Self::Skip {
121+
proposed_execpolicy_amendment: Some(prefix),
122+
..
123+
} => Some(prefix),
117124
_ => None,
118125
}
119126
}
@@ -140,6 +147,7 @@ pub(crate) fn default_exec_approval_requirement(
140147
} else {
141148
ExecApprovalRequirement::Skip {
142149
bypass_sandbox: false,
150+
proposed_execpolicy_amendment: None,
143151
}
144152
}
145153
}

‎codex-rs/core/tests/suite/approvals.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,7 @@ enum Outcome {
453453
ExecApproval {
454454
decision: ReviewDecision,
455455
expected_reason: Option<&'static str>,
456+
expect_proposed_execpolicy_amendment: bool,
456457
},
457458
PatchApproval {
458459
decision: ReviewDecision,
@@ -773,6 +774,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
773774
outcome: Outcome::ExecApproval {
774775
decision: ReviewDecision::Approved,
775776
expected_reason: None,
777+
expect_proposed_execpolicy_amendment: false,
776778
},
777779
expectation: Expectation::FileCreated {
778780
target: TargetPath::OutsideWorkspace("dfa_unless_trusted.txt"),
@@ -793,6 +795,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
793795
outcome: Outcome::ExecApproval {
794796
decision: ReviewDecision::Approved,
795797
expected_reason: None,
798+
expect_proposed_execpolicy_amendment: false,
796799
},
797800
expectation: Expectation::FileCreatedNoExitCode {
798801
target: TargetPath::OutsideWorkspace("dfa_unless_trusted_5_1.txt"),
@@ -847,6 +850,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
847850
outcome: Outcome::ExecApproval {
848851
decision: ReviewDecision::Approved,
849852
expected_reason: None,
853+
expect_proposed_execpolicy_amendment: false,
850854
},
851855
expectation: Expectation::FileCreated {
852856
target: TargetPath::Workspace("ro_on_request.txt"),
@@ -867,6 +871,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
867871
outcome: Outcome::ExecApproval {
868872
decision: ReviewDecision::Approved,
869873
expected_reason: None,
874+
expect_proposed_execpolicy_amendment: false,
870875
},
871876
expectation: Expectation::FileCreatedNoExitCode {
872877
target: TargetPath::Workspace("ro_on_request_5_1.txt"),
@@ -931,6 +936,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
931936
outcome: Outcome::ExecApproval {
932937
decision: ReviewDecision::Denied,
933938
expected_reason: None,
939+
expect_proposed_execpolicy_amendment: false,
934940
},
935941
expectation: Expectation::FileNotCreated {
936942
target: TargetPath::Workspace("ro_on_request_denied.txt"),
@@ -952,6 +958,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
952958
outcome: Outcome::ExecApproval {
953959
decision: ReviewDecision::Approved,
954960
expected_reason: Some("command failed; retry without sandbox?"),
961+
expect_proposed_execpolicy_amendment: true,
955962
},
956963
expectation: Expectation::FileCreated {
957964
target: TargetPath::Workspace("ro_on_failure.txt"),
@@ -973,6 +980,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
973980
outcome: Outcome::ExecApproval {
974981
decision: ReviewDecision::Approved,
975982
expected_reason: Some("command failed; retry without sandbox?"),
983+
expect_proposed_execpolicy_amendment: true,
976984
},
977985
expectation: Expectation::FileCreatedNoExitCode {
978986
target: TargetPath::Workspace("ro_on_failure_5_1.txt"),
@@ -993,6 +1001,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
9931001
outcome: Outcome::ExecApproval {
9941002
decision: ReviewDecision::Approved,
9951003
expected_reason: None,
1004+
expect_proposed_execpolicy_amendment: false,
9961005
},
9971006
expectation: Expectation::NetworkSuccess {
9981007
body_contains: "read-only-network-ok",
@@ -1012,6 +1021,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
10121021
outcome: Outcome::ExecApproval {
10131022
decision: ReviewDecision::Approved,
10141023
expected_reason: None,
1024+
expect_proposed_execpolicy_amendment: false,
10151025
},
10161026
expectation: Expectation::NetworkSuccessNoExitCode {
10171027
body_contains: "read-only-network-ok",
@@ -1184,6 +1194,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
11841194
outcome: Outcome::ExecApproval {
11851195
decision: ReviewDecision::Approved,
11861196
expected_reason: None,
1197+
expect_proposed_execpolicy_amendment: false,
11871198
},
11881199
expectation: Expectation::FileCreated {
11891200
target: TargetPath::Workspace("ro_unless_trusted.txt"),
@@ -1204,6 +1215,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
12041215
outcome: Outcome::ExecApproval {
12051216
decision: ReviewDecision::Approved,
12061217
expected_reason: None,
1218+
expect_proposed_execpolicy_amendment: false,
12071219
},
12081220
expectation: Expectation::FileCreatedNoExitCode {
12091221
target: TargetPath::Workspace("ro_unless_trusted_5_1.txt"),
@@ -1294,6 +1306,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
12941306
outcome: Outcome::ExecApproval {
12951307
decision: ReviewDecision::Approved,
12961308
expected_reason: None,
1309+
expect_proposed_execpolicy_amendment: false,
12971310
},
12981311
expectation: Expectation::FileCreated {
12991312
target: TargetPath::OutsideWorkspace("ww_on_request_outside.txt"),
@@ -1331,6 +1344,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
13311344
outcome: Outcome::ExecApproval {
13321345
decision: ReviewDecision::Approved,
13331346
expected_reason: Some("command failed; retry without sandbox?"),
1347+
expect_proposed_execpolicy_amendment: false,
13341348
},
13351349
expectation: Expectation::FileCreated {
13361350
target: TargetPath::OutsideWorkspace("ww_on_failure.txt"),
@@ -1351,6 +1365,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
13511365
outcome: Outcome::ExecApproval {
13521366
decision: ReviewDecision::Approved,
13531367
expected_reason: None,
1368+
expect_proposed_execpolicy_amendment: false,
13541369
},
13551370
expectation: Expectation::FileCreated {
13561371
target: TargetPath::OutsideWorkspace("ww_unless_trusted.txt"),
@@ -1413,6 +1428,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
14131428
outcome: Outcome::ExecApproval {
14141429
decision: ReviewDecision::Approved,
14151430
expected_reason: Some(DEFAULT_UNIFIED_EXEC_JUSTIFICATION),
1431+
expect_proposed_execpolicy_amendment: false,
14161432
},
14171433
expectation: Expectation::CommandSuccess {
14181434
stdout_contains: "escalated unified exec",
@@ -1432,6 +1448,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
14321448
outcome: Outcome::ExecApproval {
14331449
decision: ReviewDecision::Denied,
14341450
expected_reason: None,
1451+
expect_proposed_execpolicy_amendment: false,
14351452
},
14361453
expectation: Expectation::CommandFailure {
14371454
output_contains: "rejected by user",
@@ -1508,6 +1525,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
15081525
Outcome::ExecApproval {
15091526
decision,
15101527
expected_reason,
1528+
expect_proposed_execpolicy_amendment,
15111529
} => {
15121530
let command = expected_command
15131531
.as_deref()
@@ -1521,6 +1539,17 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
15211539
scenario.name
15221540
);
15231541
}
1542+
if *expect_proposed_execpolicy_amendment {
1543+
let amendment = approval
1544+
.proposed_execpolicy_amendment
1545+
.as_ref()
1546+
.expect("expected proposed execpolicy amendment in approval request");
1547+
assert_eq!(
1548+
amendment.command().last(),
1549+
approval.command.last(),
1550+
"expected proposed amendment to match the first heuristics command"
1551+
);
1552+
}
15241553
test.codex
15251554
.submit(Op::ExecApproval {
15261555
id: "0".into(),

0 commit comments

Comments
 (0)