Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions codex-rs/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions codex-rs/app-server/src/bespoke_event_handling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ pub(crate) async fn apply_bespoke_event_handling(
cwd,
reason,
risk,
allow_prefix: _allow_prefix,
parsed_cmd,
}) => match api_version {
ApiVersion::V1 => {
Expand Down Expand Up @@ -610,6 +611,7 @@ async fn on_exec_approval_response(
.submit(Op::ExecApproval {
id: event_id,
decision: response.decision,
allow_prefix: None,
})
.await
{
Expand Down Expand Up @@ -783,6 +785,7 @@ async fn on_command_execution_request_approval_response(
.submit(Op::ExecApproval {
id: event_id,
decision,
allow_prefix: None,
})
.await
{
Expand Down
67 changes: 64 additions & 3 deletions codex-rs/core/src/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ use crate::error::CodexErr;
use crate::error::Result as CodexResult;
#[cfg(test)]
use crate::exec::StreamOutput;
use crate::exec_policy::ExecPolicyUpdateError;
use crate::mcp::auth::compute_auth_statuses;
use crate::mcp_connection_manager::McpConnectionManager;
use crate::model_family::find_family_for_model;
Expand Down Expand Up @@ -845,11 +846,43 @@ impl Session {
.await
}

pub(crate) async fn persist_command_allow_prefix(
&self,
prefix: &[String],
) -> Result<(), ExecPolicyUpdateError> {
let (features, codex_home) = {
let state = self.state.lock().await;
(
state.session_configuration.features.clone(),
state
.session_configuration
.original_config_do_not_use
.codex_home
.clone(),
)
};

let policy =
crate::exec_policy::append_allow_prefix_rule_and_reload(&features, &codex_home, prefix)
.await?;
Comment on lines +865 to +867
Copy link
Contributor

Choose a reason for hiding this comment

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

P0 Badge Restore missing execpolicy update helper

The new allow-prefix flow calls crate::exec_policy::append_allow_prefix_rule_and_reload when persisting approvals, but no such function exists in core/exec_policy. This call will not compile (unresolved function) and the allow-prefix approval path cannot run at all.

Useful? React with 👍 / 👎.


let mut state = self.state.lock().await;
state.session_configuration.exec_policy = policy;

Ok(())
}

pub(crate) async fn current_exec_policy(&self) -> Arc<ExecPolicy> {
let state = self.state.lock().await;
state.session_configuration.exec_policy.clone()
}

/// Emit an exec approval request event and await the user's decision.
///
/// The request is keyed by `sub_id`/`call_id` so matching responses are delivered
/// to the correct in-flight turn. If the task is aborted, this returns the
/// default `ReviewDecision` (`Denied`).
#[allow(clippy::too_many_arguments)]
pub async fn request_command_approval(
&self,
turn_context: &TurnContext,
Expand All @@ -858,6 +891,7 @@ impl Session {
cwd: PathBuf,
reason: Option<String>,
risk: Option<SandboxCommandAssessment>,
allow_prefix: Option<Vec<String>>,
) -> ReviewDecision {
let sub_id = turn_context.sub_id.clone();
// Add the tx_approve callback to the map before sending the request.
Expand Down Expand Up @@ -885,6 +919,7 @@ impl Session {
cwd,
reason,
risk,
allow_prefix,
parsed_cmd,
});
self.send_event(turn_context, event).await;
Expand Down Expand Up @@ -1383,8 +1418,12 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op, &mut previous_context)
.await;
}
Op::ExecApproval { id, decision } => {
handlers::exec_approval(&sess, id, decision).await;
Op::ExecApproval {
id,
decision,
allow_prefix,
} => {
handlers::exec_approval(&sess, id, decision, allow_prefix).await;
}
Op::PatchApproval { id, decision } => {
handlers::patch_approval(&sess, id, decision).await;
Expand Down Expand Up @@ -1453,6 +1492,7 @@ mod handlers {
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::ReviewRequest;
use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::WarningEvent;

use codex_protocol::user_input::UserInput;
use std::sync::Arc;
Expand Down Expand Up @@ -1538,7 +1578,28 @@ mod handlers {
*previous_context = Some(turn_context);
}

pub async fn exec_approval(sess: &Arc<Session>, id: String, decision: ReviewDecision) {
pub async fn exec_approval(
sess: &Arc<Session>,
id: String,
decision: ReviewDecision,
allow_prefix: Option<Vec<String>>,
) {
if let Some(prefix) = allow_prefix
&& matches!(
decision,
ReviewDecision::Approved | ReviewDecision::ApprovedForSession
)
&& let Err(err) = sess.persist_command_allow_prefix(&prefix).await
{
let message = format!("Failed to update execpolicy allow list: {err}");
tracing::warn!("{message}");
let warning = EventMsg::Warning(WarningEvent { message });
sess.send_event_raw(Event {
id: id.clone(),
msg: warning,
})
.await;
}
match decision {
ReviewDecision::Abort => {
sess.interrupt_task().await;
Expand Down
9 changes: 8 additions & 1 deletion codex-rs/core/src/codex_delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ async fn handle_exec_approval(
event.cwd,
event.reason,
event.risk,
event.allow_prefix,
);
let decision = await_approval_with_cancel(
approval_fut,
Expand All @@ -244,7 +245,13 @@ async fn handle_exec_approval(
)
.await;

let _ = codex.submit(Op::ExecApproval { id, decision }).await;
let _ = codex
.submit(Op::ExecApproval {
id,
decision,
allow_prefix: None,
Comment on lines +248 to +252
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 Badge Delegate drops allow_prefix on exec approvals

When the delegate handles an exec approval request that includes allow_prefix, it forwards the decision back to the parent with allow_prefix: None. Any user choice to persist the prefix in a delegated session is silently discarded, so the execpolicy allow list is never updated in this flow.

Useful? React with 👍 / 👎.

})
.await;
}

/// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying.
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/core/src/tools/handlers/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ impl ShellHandler {
let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None);
emitter.begin(event_ctx).await;

let exec_policy = session.current_exec_policy().await;
let req = ShellRequest {
command: exec_params.command.clone(),
cwd: exec_params.cwd.clone(),
Expand All @@ -305,7 +306,7 @@ impl ShellHandler {
with_escalated_permissions: exec_params.with_escalated_permissions,
justification: exec_params.justification.clone(),
approval_requirement: create_approval_requirement_for_command(
&turn.exec_policy,
exec_policy.as_ref(),
&exec_params.command,
turn.approval_policy,
&turn.sandbox_policy,
Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core/src/tools/orchestrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ impl ToolOrchestrator {
ApprovalRequirement::Forbidden { reason } => {
return Err(ToolError::Rejected(reason));
}
ApprovalRequirement::NeedsApproval { reason } => {
ApprovalRequirement::NeedsApproval { reason, .. } => {
let mut risk = None;

if let Some(metadata) = req.sandbox_retry_data() {
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/tools/runtimes/apply_patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
cwd,
Some(reason),
risk,
None,
)
.await
} else if user_explicitly_approved {
Expand Down
10 changes: 9 additions & 1 deletion codex-rs/core/src/tools/runtimes/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,15 @@ impl Approvable<ShellRequest> for ShellRuntime {
Box::pin(async move {
with_cached_approval(&session.services, key, move || async move {
session
.request_command_approval(turn, call_id, command, cwd, reason, risk)
.request_command_approval(
turn,
call_id,
command,
cwd,
reason,
risk,
req.approval_requirement.allow_prefix().cloned(),
)
.await
})
.await
Expand Down
10 changes: 9 additions & 1 deletion codex-rs/core/src/tools/runtimes/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,15 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
Box::pin(async move {
with_cached_approval(&session.services, key, || async move {
session
.request_command_approval(turn, call_id, command, cwd, reason, risk)
.request_command_approval(
turn,
call_id,
command,
cwd,
reason,
risk,
req.approval_requirement.allow_prefix().cloned(),
)
.await
})
.await
Expand Down
22 changes: 20 additions & 2 deletions codex-rs/core/src/tools/sandboxing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,26 @@ pub(crate) enum ApprovalRequirement {
/// No approval required for this tool call
Skip,
/// Approval required for this tool call
NeedsApproval { reason: Option<String> },
NeedsApproval {
reason: Option<String>,
allow_prefix: Option<Vec<String>>,
},
/// Execution forbidden for this tool call
Forbidden { reason: String },
}

impl ApprovalRequirement {
pub fn allow_prefix(&self) -> Option<&Vec<String>> {
match self {
Self::NeedsApproval {
allow_prefix: Some(prefix),
..
} => Some(prefix),
_ => None,
}
}
}

/// - Never, OnFailure: do not ask
/// - OnRequest: ask unless sandbox policy is DangerFullAccess
/// - UnlessTrusted: always ask
Expand All @@ -111,7 +126,10 @@ pub(crate) fn default_approval_requirement(
};

if needs_approval {
ApprovalRequirement::NeedsApproval { reason: None }
ApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
}
} else {
ApprovalRequirement::Skip
}
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/core/src/unified_exec/session_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,14 +445,15 @@ impl UnifiedExecSessionManager {
) -> Result<UnifiedExecSession, UnifiedExecError> {
let mut orchestrator = ToolOrchestrator::new();
let mut runtime = UnifiedExecRuntime::new(self);
let exec_policy = context.session.current_exec_policy().await;
let req = UnifiedExecToolRequest::new(
command.to_vec(),
cwd,
create_env(&context.turn.shell_environment_policy),
with_escalated_permissions,
justification,
create_approval_requirement_for_command(
&context.turn.exec_policy,
exec_policy.as_ref(),
command,
context.turn.approval_policy,
&context.turn.sandbox_policy,
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/tests/suite/approvals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1524,6 +1524,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
.submit(Op::ExecApproval {
id: "0".into(),
decision: *decision,
allow_prefix: None,
})
.await?;
wait_for_completion(&test).await;
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/tests/suite/codex_delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Approved,
allow_prefix: None,
})
.await
.expect("submit exec approval");
Expand Down
6 changes: 6 additions & 0 deletions codex-rs/core/tests/suite/otel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Approved,
allow_prefix: None,
})
.await
.unwrap();
Expand Down Expand Up @@ -901,6 +902,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision()
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::ApprovedForSession,
allow_prefix: None,
})
.await
.unwrap();
Expand Down Expand Up @@ -959,6 +961,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Approved,
allow_prefix: None,
})
.await
.unwrap();
Expand Down Expand Up @@ -1017,6 +1020,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Denied,
allow_prefix: None,
})
.await
.unwrap();
Expand Down Expand Up @@ -1075,6 +1079,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision()
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::ApprovedForSession,
allow_prefix: None,
})
.await
.unwrap();
Expand Down Expand Up @@ -1134,6 +1139,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Denied,
allow_prefix: None,
})
.await
.unwrap();
Expand Down
1 change: 1 addition & 0 deletions codex-rs/execpolicy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ thiserror = { workspace = true }

[dev-dependencies]
pretty_assertions = { workspace = true }
tempfile = { workspace = true }
Loading
Loading