Skip to content

Commit 3d35cb4

Browse files
authored
Refactor execpolicy fallback evaluation (openai#7544)
## Refactor of the `execpolicy` crate To illustrate why we need this refactor, consider an agent attempting to run `apple | rm -rf ./`. Suppose `apple` is allowed by `execpolicy`. Before this PR, `execpolicy` would consider `apple` and `pear` and only render one rule match: `Allow`. We would skip any heuristics checks on `rm -rf ./` and immediately approve `apple | rm -rf ./` to run. To fix this, we now thread a `fallback` evaluation function into `execpolicy` that runs when no `execpolicy` rules match a given command. In our example, we would run `fallback` on `rm -rf ./` and prevent `apple | rm -rf ./` from being run without approval.
1 parent e925a38 commit 3d35cb4

File tree

27 files changed

+539
-258
lines changed

27 files changed

+539
-258
lines changed

codex-rs/app-server/src/bespoke_event_handling.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ pub(crate) async fn apply_bespoke_event_handling(
179179
cwd,
180180
reason,
181181
risk,
182-
allow_prefix: _allow_prefix,
182+
proposed_execpolicy_amendment: _,
183183
parsed_cmd,
184184
}) => match api_version {
185185
ApiVersion::V1 => {

codex-rs/cli/tests/execpolicy.rs

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,15 @@ prefix_rule(
4040
assert_eq!(
4141
result,
4242
json!({
43-
"match": {
44-
"decision": "forbidden",
45-
"matchedRules": [
46-
{
47-
"prefixRuleMatch": {
48-
"matchedPrefix": ["git", "push"],
49-
"decision": "forbidden"
50-
}
43+
"decision": "forbidden",
44+
"matchedRules": [
45+
{
46+
"prefixRuleMatch": {
47+
"matchedPrefix": ["git", "push"],
48+
"decision": "forbidden"
5149
}
52-
]
53-
}
50+
}
51+
]
5452
})
5553
);
5654

codex-rs/core/src/apply_patch.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ pub(crate) async fn apply_patch(
7171
.await;
7272
match rx_approve.await.unwrap_or_default() {
7373
ReviewDecision::Approved
74-
| ReviewDecision::ApprovedAllowPrefix { .. }
74+
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
7575
| ReviewDecision::ApprovedForSession => {
7676
InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
7777
action,

codex-rs/core/src/codex.rs

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ use crate::util::error_or_panic;
2525
use async_channel::Receiver;
2626
use async_channel::Sender;
2727
use codex_protocol::ConversationId;
28+
use codex_protocol::approvals::ExecPolicyAmendment;
2829
use codex_protocol::items::TurnItem;
2930
use codex_protocol::protocol::FileChange;
3031
use codex_protocol::protocol::HasLegacyEvent;
@@ -871,13 +872,11 @@ impl Session {
871872
.await
872873
}
873874

874-
/// Adds a prefix rule to the exec policy
875-
///
876-
/// This mutates the in-memory execpolicy so the current conversation can use the new
877-
/// prefix and persists the change in default.execpolicy so new conversations will also allow the new prefix.
878-
pub(crate) async fn persist_command_allow_prefix(
875+
/// Adds an execpolicy amendment to both the in-memory and on-disk policies so future
876+
/// commands can use the newly approved prefix.
877+
pub(crate) async fn persist_execpolicy_amendment(
879878
&self,
880-
prefix: &[String],
879+
amendment: &ExecPolicyAmendment,
881880
) -> Result<(), ExecPolicyUpdateError> {
882881
let features = self.features.clone();
883882
let (codex_home, current_policy) = {
@@ -897,10 +896,10 @@ impl Session {
897896
return Err(ExecPolicyUpdateError::FeatureDisabled);
898897
}
899898

900-
crate::exec_policy::append_allow_prefix_rule_and_update(
899+
crate::exec_policy::append_execpolicy_amendment_and_update(
901900
&codex_home,
902901
&current_policy,
903-
prefix,
902+
&amendment.command,
904903
)
905904
.await?;
906905

@@ -921,7 +920,7 @@ impl Session {
921920
cwd: PathBuf,
922921
reason: Option<String>,
923922
risk: Option<SandboxCommandAssessment>,
924-
allow_prefix: Option<Vec<String>>,
923+
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
925924
) -> ReviewDecision {
926925
let sub_id = turn_context.sub_id.clone();
927926
// Add the tx_approve callback to the map before sending the request.
@@ -949,7 +948,7 @@ impl Session {
949948
cwd,
950949
reason,
951950
risk,
952-
allow_prefix,
951+
proposed_execpolicy_amendment,
953952
parsed_cmd,
954953
});
955954
self.send_event(turn_context, event).await;
@@ -1672,13 +1671,17 @@ mod handlers {
16721671
}
16731672
}
16741673

1675-
/// Propagate a user's exec approval decision to the session
1676-
/// Also optionally whitelists command in execpolicy
1674+
/// Propagate a user's exec approval decision to the session.
1675+
/// Also optionally applies an execpolicy amendment.
16771676
pub async fn exec_approval(sess: &Arc<Session>, id: String, decision: ReviewDecision) {
1678-
if let ReviewDecision::ApprovedAllowPrefix { allow_prefix } = &decision
1679-
&& let Err(err) = sess.persist_command_allow_prefix(allow_prefix).await
1677+
if let ReviewDecision::ApprovedExecpolicyAmendment {
1678+
proposed_execpolicy_amendment,
1679+
} = &decision
1680+
&& let Err(err) = sess
1681+
.persist_execpolicy_amendment(proposed_execpolicy_amendment)
1682+
.await
16801683
{
1681-
let message = format!("Failed to update execpolicy allow list: {err}");
1684+
let message = format!("Failed to apply execpolicy amendment: {err}");
16821685
tracing::warn!("{message}");
16831686
let warning = EventMsg::Warning(WarningEvent { message });
16841687
sess.send_event_raw(Event {

codex-rs/core/src/codex_delegate.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ async fn handle_exec_approval(
281281
event.cwd,
282282
event.reason,
283283
event.risk,
284-
event.allow_prefix,
284+
event.proposed_execpolicy_amendment,
285285
);
286286
let decision = await_approval_with_cancel(
287287
approval_fut,

0 commit comments

Comments
 (0)