Skip to content

Commit 86b1d98

Browse files
committed
.
1 parent 174a8dd commit 86b1d98

File tree

8 files changed

+98
-83
lines changed

8 files changed

+98
-83
lines changed

codex-rs/core/src/codex.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,10 @@ impl Session {
862862
.await
863863
}
864864

865+
/// Adds a prefix rule to the exec policy
866+
///
867+
/// This mutates the in-memory execpolicy so the current conversation can use the new
868+
/// prefix and persists the change in default.execpolicy so new conversations will also allow the new prefix.
865869
pub(crate) async fn persist_command_allow_prefix(
866870
&self,
867871
prefix: &[String],

codex-rs/core/src/exec_policy.rs

Lines changed: 50 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ use codex_protocol::protocol::SandboxPolicy;
1616
use thiserror::Error;
1717
use tokio::fs;
1818
use tokio::sync::RwLock;
19+
use tokio::task::spawn_blocking;
1920

2021
use crate::bash::parse_shell_lc_plain_commands;
2122
use crate::features::Feature;
2223
use crate::features::Features;
2324
use crate::sandboxing::SandboxPermissions;
24-
use crate::tools::sandboxing::ApprovalRequirement;
25+
use crate::tools::sandboxing::ExecApprovalRequirement;
2526

2627
const FORBIDDEN_REASON: &str = "execpolicy forbids this command";
2728
const PROMPT_REASON: &str = "execpolicy requires approval for this command";
@@ -55,6 +56,9 @@ pub enum ExecPolicyUpdateError {
5556
#[error("failed to update execpolicy file {path}: {source}")]
5657
AppendRule { path: PathBuf, source: AmendError },
5758

59+
#[error("failed to join blocking execpolicy update task: {source}")]
60+
JoinBlockingTask { source: tokio::task::JoinError },
61+
5862
#[error("failed to update in-memory execpolicy: {source}")]
5963
AddRule {
6064
#[from]
@@ -114,41 +118,47 @@ pub(crate) async fn append_allow_prefix_rule_and_update(
114118
prefix: &[String],
115119
) -> Result<(), ExecPolicyUpdateError> {
116120
let policy_path = default_policy_path(codex_home);
117-
blocking_append_allow_prefix_rule(&policy_path, prefix).map_err(|source| {
118-
ExecPolicyUpdateError::AppendRule {
119-
path: policy_path,
120-
source,
121-
}
121+
let prefix = prefix.to_vec();
122+
spawn_blocking({
123+
let policy_path = policy_path.clone();
124+
let prefix = prefix.clone();
125+
move || blocking_append_allow_prefix_rule(&policy_path, &prefix)
126+
})
127+
.await
128+
.map_err(|source| ExecPolicyUpdateError::JoinBlockingTask { source })?
129+
.map_err(|source| ExecPolicyUpdateError::AppendRule {
130+
path: policy_path,
131+
source,
122132
})?;
123133

124134
current_policy
125135
.write()
126136
.await
127-
.add_prefix_rule(prefix, Decision::Allow)?;
137+
.add_prefix_rule(&prefix, Decision::Allow)?;
128138

129139
Ok(())
130140
}
131141

132142
fn requirement_from_decision(
133143
decision: Decision,
134144
approval_policy: AskForApproval,
135-
) -> ApprovalRequirement {
145+
) -> ExecApprovalRequirement {
136146
match decision {
137-
Decision::Forbidden => ApprovalRequirement::Forbidden {
147+
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
138148
reason: FORBIDDEN_REASON.to_string(),
139149
},
140150
Decision::Prompt => {
141151
let reason = PROMPT_REASON.to_string();
142152
if matches!(approval_policy, AskForApproval::Never) {
143-
ApprovalRequirement::Forbidden { reason }
153+
ExecApprovalRequirement::Forbidden { reason }
144154
} else {
145-
ApprovalRequirement::NeedsApproval {
155+
ExecApprovalRequirement::NeedsApproval {
146156
reason: Some(reason),
147157
allow_prefix: None,
148158
}
149159
}
150160
}
151-
Decision::Allow => ApprovalRequirement::Skip {
161+
Decision::Allow => ExecApprovalRequirement::Skip {
152162
bypass_sandbox: true,
153163
},
154164
}
@@ -170,17 +180,16 @@ fn allow_prefix_if_applicable(
170180
}
171181
}
172182

173-
pub(crate) async fn create_approval_requirement_for_command(
183+
pub(crate) async fn create_exec_approval_requirement_for_command(
174184
exec_policy: &Arc<RwLock<Policy>>,
175185
features: &Features,
176186
command: &[String],
177187
approval_policy: AskForApproval,
178188
sandbox_policy: &SandboxPolicy,
179189
sandbox_permissions: SandboxPermissions,
180-
) -> ApprovalRequirement {
190+
) -> ExecApprovalRequirement {
181191
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
182-
let policy = exec_policy.read().await;
183-
let evaluation = policy.check_multiple(commands.iter());
192+
let evaluation = exec_policy.read().await.check_multiple(commands.iter());
184193

185194
match evaluation {
186195
Evaluation::Match { decision, .. } => requirement_from_decision(decision, approval_policy),
@@ -191,12 +200,12 @@ pub(crate) async fn create_approval_requirement_for_command(
191200
command,
192201
sandbox_permissions,
193202
) {
194-
ApprovalRequirement::NeedsApproval {
203+
ExecApprovalRequirement::NeedsApproval {
195204
reason: None,
196205
allow_prefix: allow_prefix_if_applicable(&commands, features),
197206
}
198207
} else {
199-
ApprovalRequirement::Skip {
208+
ExecApprovalRequirement::Skip {
200209
bypass_sandbox: false,
201210
}
202211
}
@@ -349,7 +358,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
349358
"rm -rf /tmp".to_string(),
350359
];
351360

352-
let requirement = create_approval_requirement_for_command(
361+
let requirement = create_exec_approval_requirement_for_command(
353362
&policy,
354363
&Features::with_defaults(),
355364
&forbidden_script,
@@ -361,14 +370,14 @@ prefix_rule(pattern=["rm"], decision="forbidden")
361370

362371
assert_eq!(
363372
requirement,
364-
ApprovalRequirement::Forbidden {
373+
ExecApprovalRequirement::Forbidden {
365374
reason: FORBIDDEN_REASON.to_string()
366375
}
367376
);
368377
}
369378

370379
#[tokio::test]
371-
async fn approval_requirement_prefers_execpolicy_match() {
380+
async fn exec_approval_requirement_prefers_execpolicy_match() {
372381
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
373382
let mut parser = PolicyParser::new();
374383
parser
@@ -377,7 +386,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
377386
let policy = Arc::new(RwLock::new(parser.build()));
378387
let command = vec!["rm".to_string()];
379388

380-
let requirement = create_approval_requirement_for_command(
389+
let requirement = create_exec_approval_requirement_for_command(
381390
&policy,
382391
&Features::with_defaults(),
383392
&command,
@@ -389,15 +398,15 @@ prefix_rule(pattern=["rm"], decision="forbidden")
389398

390399
assert_eq!(
391400
requirement,
392-
ApprovalRequirement::NeedsApproval {
401+
ExecApprovalRequirement::NeedsApproval {
393402
reason: Some(PROMPT_REASON.to_string()),
394403
allow_prefix: None,
395404
}
396405
);
397406
}
398407

399408
#[tokio::test]
400-
async fn approval_requirement_respects_approval_policy() {
409+
async fn exec_approval_requirement_respects_approval_policy() {
401410
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
402411
let mut parser = PolicyParser::new();
403412
parser
@@ -406,7 +415,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
406415
let policy = Arc::new(RwLock::new(parser.build()));
407416
let command = vec!["rm".to_string()];
408417

409-
let requirement = create_approval_requirement_for_command(
418+
let requirement = create_exec_approval_requirement_for_command(
410419
&policy,
411420
&Features::with_defaults(),
412421
&command,
@@ -418,18 +427,18 @@ prefix_rule(pattern=["rm"], decision="forbidden")
418427

419428
assert_eq!(
420429
requirement,
421-
ApprovalRequirement::Forbidden {
430+
ExecApprovalRequirement::Forbidden {
422431
reason: PROMPT_REASON.to_string()
423432
}
424433
);
425434
}
426435

427436
#[tokio::test]
428-
async fn approval_requirement_falls_back_to_heuristics() {
429-
let command = vec!["python".to_string()];
437+
async fn exec_approval_requirement_falls_back_to_heuristics() {
438+
let command = vec!["cargo".to_string(), "build".to_string()];
430439

431440
let empty_policy = Arc::new(RwLock::new(Policy::empty()));
432-
let requirement = create_approval_requirement_for_command(
441+
let requirement = create_exec_approval_requirement_for_command(
433442
&empty_policy,
434443
&Features::with_defaults(),
435444
&command,
@@ -441,7 +450,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
441450

442451
assert_eq!(
443452
requirement,
444-
ApprovalRequirement::NeedsApproval {
453+
ExecApprovalRequirement::NeedsApproval {
445454
reason: None,
446455
allow_prefix: Some(command)
447456
}
@@ -499,10 +508,10 @@ prefix_rule(pattern=["rm"], decision="forbidden")
499508

500509
#[tokio::test]
501510
async fn allow_prefix_is_present_for_single_command_without_policy_match() {
502-
let command = vec!["python".to_string()];
511+
let command = vec!["cargo".to_string(), "build".to_string()];
503512

504513
let empty_policy = Arc::new(RwLock::new(Policy::empty()));
505-
let requirement = create_approval_requirement_for_command(
514+
let requirement = create_exec_approval_requirement_for_command(
506515
&empty_policy,
507516
&Features::with_defaults(),
508517
&command,
@@ -514,7 +523,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
514523

515524
assert_eq!(
516525
requirement,
517-
ApprovalRequirement::NeedsApproval {
526+
ExecApprovalRequirement::NeedsApproval {
518527
reason: None,
519528
allow_prefix: Some(command)
520529
}
@@ -523,12 +532,12 @@ prefix_rule(pattern=["rm"], decision="forbidden")
523532

524533
#[tokio::test]
525534
async fn allow_prefix_is_disabled_when_execpolicy_feature_disabled() {
526-
let command = vec!["python".to_string()];
535+
let command = vec!["cargo".to_string(), "build".to_string()];
527536

528537
let mut features = Features::with_defaults();
529538
features.disable(Feature::ExecPolicy);
530539

531-
let requirement = create_approval_requirement_for_command(
540+
let requirement = create_exec_approval_requirement_for_command(
532541
&Arc::new(RwLock::new(Policy::empty())),
533542
&features,
534543
&command,
@@ -540,7 +549,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
540549

541550
assert_eq!(
542551
requirement,
543-
ApprovalRequirement::NeedsApproval {
552+
ExecApprovalRequirement::NeedsApproval {
544553
reason: None,
545554
allow_prefix: None,
546555
}
@@ -557,7 +566,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
557566
let policy = Arc::new(RwLock::new(parser.build()));
558567
let command = vec!["rm".to_string()];
559568

560-
let requirement = create_approval_requirement_for_command(
569+
let requirement = create_exec_approval_requirement_for_command(
561570
&policy,
562571
&Features::with_defaults(),
563572
&command,
@@ -569,7 +578,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
569578

570579
assert_eq!(
571580
requirement,
572-
ApprovalRequirement::NeedsApproval {
581+
ExecApprovalRequirement::NeedsApproval {
573582
reason: Some(PROMPT_REASON.to_string()),
574583
allow_prefix: None,
575584
}
@@ -581,9 +590,9 @@ prefix_rule(pattern=["rm"], decision="forbidden")
581590
let command = vec![
582591
"bash".to_string(),
583592
"-lc".to_string(),
584-
"python && echo ok".to_string(),
593+
"cargo build && echo ok".to_string(),
585594
];
586-
let requirement = create_approval_requirement_for_command(
595+
let requirement = create_exec_approval_requirement_for_command(
587596
&Arc::new(RwLock::new(Policy::empty())),
588597
&Features::with_defaults(),
589598
&command,
@@ -595,7 +604,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
595604

596605
assert_eq!(
597606
requirement,
598-
ApprovalRequirement::NeedsApproval {
607+
ExecApprovalRequirement::NeedsApproval {
599608
reason: None,
600609
allow_prefix: None,
601610
}

codex-rs/core/src/tools/handlers/shell.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::sync::Arc;
66
use crate::codex::TurnContext;
77
use crate::exec::ExecParams;
88
use crate::exec_env::create_env;
9-
use crate::exec_policy::create_approval_requirement_for_command;
9+
use crate::exec_policy::create_exec_approval_requirement_for_command;
1010
use crate::function_tool::FunctionCallError;
1111
use crate::is_safe_command::is_known_safe_command;
1212
use crate::protocol::ExecCommandSource;
@@ -232,7 +232,7 @@ impl ShellHandler {
232232
emitter.begin(event_ctx).await;
233233

234234
let features = session.features().await;
235-
let approval_requirement = create_approval_requirement_for_command(
235+
let exec_approval_requirement = create_exec_approval_requirement_for_command(
236236
&turn.exec_policy,
237237
&features,
238238
&exec_params.command,
@@ -241,15 +241,15 @@ impl ShellHandler {
241241
SandboxPermissions::from(exec_params.with_escalated_permissions.unwrap_or(false)),
242242
)
243243
.await;
244-
244+
245245
let req = ShellRequest {
246246
command: exec_params.command.clone(),
247247
cwd: exec_params.cwd.clone(),
248248
timeout_ms: exec_params.expiration.timeout_ms(),
249249
env: exec_params.env.clone(),
250250
with_escalated_permissions: exec_params.with_escalated_permissions,
251251
justification: exec_params.justification.clone(),
252-
approval_requirement,
252+
exec_approval_requirement,
253253
};
254254
let mut orchestrator = ToolOrchestrator::new();
255255
let mut runtime = ShellRuntime::new();

codex-rs/core/src/tools/orchestrator.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,14 @@ use crate::error::get_error_message_ui;
1111
use crate::exec::ExecToolCallOutput;
1212
use crate::sandboxing::SandboxManager;
1313
use crate::tools::sandboxing::ApprovalCtx;
14-
use crate::tools::sandboxing::ApprovalRequirement;
14+
use crate::tools::sandboxing::ExecApprovalRequirement;
1515
use crate::tools::sandboxing::ProvidesSandboxRetryData;
1616
use crate::tools::sandboxing::SandboxAttempt;
1717
use crate::tools::sandboxing::SandboxOverride;
1818
use crate::tools::sandboxing::ToolCtx;
1919
use crate::tools::sandboxing::ToolError;
2020
use crate::tools::sandboxing::ToolRuntime;
21-
use crate::tools::sandboxing::default_approval_requirement;
21+
use crate::tools::sandboxing::default_exec_approval_requirement;
2222
use codex_protocol::protocol::AskForApproval;
2323
use codex_protocol::protocol::ReviewDecision;
2424

@@ -54,17 +54,17 @@ impl ToolOrchestrator {
5454
// 1) Approval
5555
let mut already_approved = false;
5656

57-
let requirement = tool.approval_requirement(req).unwrap_or_else(|| {
58-
default_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
57+
let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| {
58+
default_exec_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
5959
});
6060
match requirement {
61-
ApprovalRequirement::Skip { .. } => {
61+
ExecApprovalRequirement::Skip { .. } => {
6262
otel.tool_decision(otel_tn, otel_ci, &ReviewDecision::Approved, otel_cfg);
6363
}
64-
ApprovalRequirement::Forbidden { reason } => {
64+
ExecApprovalRequirement::Forbidden { reason } => {
6565
return Err(ToolError::Rejected(reason));
6666
}
67-
ApprovalRequirement::NeedsApproval { reason, .. } => {
67+
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
6868
let mut risk = None;
6969

7070
if let Some(metadata) = req.sandbox_retry_data() {

0 commit comments

Comments
 (0)