mirror of
https://github.com/openai/codex.git
synced 2026-09-04 15:08:45 +00:00
Harden one-shot command approval retries
This commit is contained in:
@@ -398,6 +398,83 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn handle_exec_approval_preserves_retry_id_through_non_guardian_parent_round_trip() {
|
||||
let (parent_session, parent_ctx, rx_events) =
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
*parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default());
|
||||
let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref");
|
||||
let mut config = (*parent_ctx.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
parent_ctx.config = Arc::new(config);
|
||||
let parent_ctx = Arc::new(parent_ctx);
|
||||
|
||||
let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY);
|
||||
let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY);
|
||||
let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit);
|
||||
let codex = Arc::new(Codex {
|
||||
tx_sub,
|
||||
rx_event: rx_events_child,
|
||||
agent_status,
|
||||
session: Arc::clone(&parent_session),
|
||||
session_loop_termination: completed_session_loop_termination(),
|
||||
});
|
||||
|
||||
let cancel_token = CancellationToken::new();
|
||||
let handle = tokio::spawn({
|
||||
let codex = Arc::clone(&codex);
|
||||
let parent_session = Arc::clone(&parent_session);
|
||||
let parent_ctx = Arc::clone(&parent_ctx);
|
||||
let cancel_token = cancel_token.clone();
|
||||
async move {
|
||||
handle_exec_approval(
|
||||
codex.as_ref(),
|
||||
"child-turn-1".to_string(),
|
||||
&parent_session,
|
||||
&parent_ctx,
|
||||
delegated_retry_exec_approval_event(),
|
||||
&cancel_token,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
});
|
||||
|
||||
let request_event = timeout(Duration::from_secs(1), rx_events.recv())
|
||||
.await
|
||||
.expect("exec approval event timed out")
|
||||
.expect("exec approval event missing");
|
||||
let EventMsg::ExecApprovalRequest(request) = request_event.msg else {
|
||||
panic!("expected ExecApprovalRequest event");
|
||||
};
|
||||
assert_eq!(request.call_id, "command-item-1");
|
||||
assert_eq!(request.approval_id.as_deref(), Some("retry-id"));
|
||||
assert_eq!(
|
||||
request.approval_purpose,
|
||||
Some(ExecApprovalPurpose::SandboxRetry)
|
||||
);
|
||||
|
||||
parent_session
|
||||
.notify_exec_approval("retry-id", ReviewDecision::Approved)
|
||||
.await;
|
||||
|
||||
timeout(Duration::from_secs(1), handle)
|
||||
.await
|
||||
.expect("handle_exec_approval hung")
|
||||
.expect("handle_exec_approval join error");
|
||||
let submission = timeout(Duration::from_secs(1), rx_sub.recv())
|
||||
.await
|
||||
.expect("exec approval response timed out")
|
||||
.expect("exec approval response missing");
|
||||
assert_eq!(
|
||||
submission.op,
|
||||
Op::ExecApproval {
|
||||
id: "retry-id".to_string(),
|
||||
turn_id: Some("child-turn-1".to_string()),
|
||||
decision: ReviewDecision::Approved,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn delegated_exec_preserves_raw_purpose_and_cancel_cleans_retry_waiter() {
|
||||
let (parent_session, parent_ctx, rx_events) =
|
||||
|
||||
@@ -2130,10 +2130,13 @@ impl Session {
|
||||
|
||||
/// Emit an exec approval request event and await the user's decision.
|
||||
///
|
||||
/// The request is keyed by `call_id` + `approval_id` so matching responses
|
||||
/// are delivered to the correct in-flight turn. If the pending approval is
|
||||
/// cleared before a response arrives, treat it as an abort so interrupted
|
||||
/// turns do not continue on a synthetic denial.
|
||||
/// `call_id` is the stable command-item identity. A callback-specific
|
||||
/// `approval_id`, when present, is the response-routing identity; otherwise
|
||||
/// `call_id` is used for both. Each callback-specific ID must be unique so a
|
||||
/// delayed or replayed response cannot resolve a later prompt for the same
|
||||
/// item. If the pending approval is cleared before a response arrives,
|
||||
/// treat it as an abort so interrupted turns do not continue on a synthetic
|
||||
/// denial.
|
||||
///
|
||||
/// If `available_decisions` is `None`, callback-scoped requests use the
|
||||
/// one-shot approve/abort set; other requests derive their decisions via
|
||||
@@ -2158,8 +2161,9 @@ impl Session {
|
||||
additional_permissions: Option<AdditionalPermissionProfile>,
|
||||
available_decisions: Option<Vec<ReviewDecision>>,
|
||||
) -> ReviewDecision {
|
||||
// Command-level approvals use `call_id`; callback-scoped approvals can
|
||||
// supply a distinct ID for matching the response.
|
||||
// Cacheable single-prompt approvals use `call_id`. Callback-scoped
|
||||
// approvals carry a fresh opaque ID for response routing while
|
||||
// `call_id` remains the stable command-item identity.
|
||||
let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone());
|
||||
let proposed_network_policy_amendments = network_approval_context.as_ref().map(|context| {
|
||||
vec![
|
||||
|
||||
@@ -3,8 +3,8 @@ Module: orchestrator
|
||||
|
||||
Central place for approvals + sandbox selection + retry semantics. Drives a
|
||||
simple sequence for any ToolRuntime: approval → select sandbox → attempt →
|
||||
retry with an escalated sandbox strategy on denial (no re‑approval thanks to
|
||||
caching).
|
||||
retry with an escalated sandbox strategy on denial. Cacheable approvals can
|
||||
cover that retry; explicitly one-shot approvals cannot.
|
||||
*/
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::guardian_timeout_message;
|
||||
@@ -35,12 +35,14 @@ use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::SandboxErr;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::ExecApprovalPurpose;
|
||||
use codex_protocol::protocol::NetworkPolicyRuleAction;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_sandboxing::SandboxManager;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::time::Instant;
|
||||
use uuid::Uuid;
|
||||
|
||||
pub(crate) struct ToolOrchestrator {
|
||||
sandbox: SandboxManager,
|
||||
@@ -51,6 +53,13 @@ pub(crate) struct OrchestratorRunResult<Out> {
|
||||
pub deferred_network_approval: Option<DeferredNetworkApproval>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
enum PermissionRequestHookMode {
|
||||
Skip,
|
||||
DenyOnly,
|
||||
AllowAndDeny,
|
||||
}
|
||||
|
||||
impl ToolOrchestrator {
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
@@ -164,6 +173,8 @@ impl ToolOrchestrator {
|
||||
session: &tool_ctx.session,
|
||||
turn: &tool_ctx.turn,
|
||||
call_id: &tool_ctx.call_id,
|
||||
approval_id: None,
|
||||
approval_purpose: ExecApprovalPurpose::Initial,
|
||||
guardian_review_id: guardian_review_id.clone(),
|
||||
retry_reason: None,
|
||||
network_approval_context: None,
|
||||
@@ -174,7 +185,7 @@ impl ToolOrchestrator {
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ false,
|
||||
PermissionRequestHookMode::Skip,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
@@ -193,12 +204,18 @@ impl ToolOrchestrator {
|
||||
ExecApprovalRequirement::Forbidden { reason } => {
|
||||
return Err(ToolError::Rejected(reason.clone()));
|
||||
}
|
||||
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
ExecApprovalRequirement::NeedsApproval { reason, .. }
|
||||
| ExecApprovalRequirement::NeedsOneShotApproval { reason } => {
|
||||
let guardian_review_id = use_guardian.then(new_guardian_review_id);
|
||||
let approval_id = requirement
|
||||
.is_one_shot()
|
||||
.then(|| Uuid::new_v4().to_string());
|
||||
let approval_ctx = ApprovalCtx {
|
||||
session: &tool_ctx.session,
|
||||
turn: &tool_ctx.turn,
|
||||
call_id: &tool_ctx.call_id,
|
||||
approval_id,
|
||||
approval_purpose: ExecApprovalPurpose::Initial,
|
||||
guardian_review_id: guardian_review_id.clone(),
|
||||
retry_reason: reason.clone(),
|
||||
network_approval_context: None,
|
||||
@@ -209,7 +226,7 @@ impl ToolOrchestrator {
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
permission_request_hook_mode(strict_auto_review, &requirement),
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
@@ -386,15 +403,25 @@ impl ToolOrchestrator {
|
||||
|
||||
// Strict auto-review approval covers the sandboxed attempt only;
|
||||
// retrying without the sandbox requires a fresh guardian review.
|
||||
let bypass_retry_approval = !strict_auto_review
|
||||
&& tool.should_bypass_approval(approval_policy, already_approved)
|
||||
&& network_approval_context.is_none();
|
||||
let bypass_retry_approval = can_bypass_retry_approval(
|
||||
strict_auto_review,
|
||||
&requirement,
|
||||
tool.should_bypass_approval(approval_policy, already_approved),
|
||||
network_approval_context.is_some(),
|
||||
);
|
||||
if !bypass_retry_approval {
|
||||
// A tool item can produce both an initial approval and a
|
||||
// later retry approval. Give the retry its own unguessable
|
||||
// waiter ID so a duplicated/replayed response for the
|
||||
// initial prompt cannot resolve the new waiter.
|
||||
let retry_approval_id = Uuid::new_v4().to_string();
|
||||
let guardian_review_id = use_guardian.then(new_guardian_review_id);
|
||||
let approval_ctx = ApprovalCtx {
|
||||
session: &tool_ctx.session,
|
||||
turn: &tool_ctx.turn,
|
||||
call_id: &tool_ctx.call_id,
|
||||
approval_id: Some(retry_approval_id),
|
||||
approval_purpose: ExecApprovalPurpose::SandboxRetry,
|
||||
guardian_review_id: guardian_review_id.clone(),
|
||||
retry_reason: Some(retry_reason),
|
||||
network_approval_context: network_approval_context.clone(),
|
||||
@@ -407,7 +434,7 @@ impl ToolOrchestrator {
|
||||
&permission_request_run_id,
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
permission_request_hook_mode(strict_auto_review, &requirement),
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
@@ -516,13 +543,13 @@ impl ToolOrchestrator {
|
||||
permission_request_run_id: &str,
|
||||
approval_ctx: ApprovalCtx<'_>,
|
||||
tool_ctx: &ToolCtx,
|
||||
evaluate_permission_request_hooks: bool,
|
||||
permission_request_hook_mode: PermissionRequestHookMode,
|
||||
otel: &codex_otel::SessionTelemetry,
|
||||
) -> Result<ReviewDecision, ToolError>
|
||||
where
|
||||
T: ToolRuntime<Rq, Out>,
|
||||
{
|
||||
if evaluate_permission_request_hooks
|
||||
if permission_request_hook_mode != PermissionRequestHookMode::Skip
|
||||
&& let Some(permission_request) = tool.permission_request_payload(req)
|
||||
{
|
||||
let tool_name = flat_tool_name(&tool_ctx.tool_name);
|
||||
@@ -534,7 +561,9 @@ impl ToolOrchestrator {
|
||||
)
|
||||
.await
|
||||
{
|
||||
Some(PermissionRequestDecision::Allow) => {
|
||||
Some(PermissionRequestDecision::Allow)
|
||||
if permission_request_hook_mode == PermissionRequestHookMode::AllowAndDeny =>
|
||||
{
|
||||
let decision = ReviewDecision::Approved;
|
||||
otel.tool_decision(
|
||||
tool_name.as_ref(),
|
||||
@@ -554,7 +583,7 @@ impl ToolOrchestrator {
|
||||
);
|
||||
return Err(ToolError::Rejected(message));
|
||||
}
|
||||
None => {}
|
||||
Some(PermissionRequestDecision::Allow) | None => {}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -604,6 +633,31 @@ impl ToolOrchestrator {
|
||||
}
|
||||
}
|
||||
|
||||
fn can_bypass_retry_approval(
|
||||
strict_auto_review: bool,
|
||||
requirement: &ExecApprovalRequirement,
|
||||
policy_bypasses_approval: bool,
|
||||
has_network_approval_context: bool,
|
||||
) -> bool {
|
||||
!strict_auto_review
|
||||
&& !requirement.is_one_shot()
|
||||
&& policy_bypasses_approval
|
||||
&& !has_network_approval_context
|
||||
}
|
||||
|
||||
fn permission_request_hook_mode(
|
||||
strict_auto_review: bool,
|
||||
requirement: &ExecApprovalRequirement,
|
||||
) -> PermissionRequestHookMode {
|
||||
if strict_auto_review {
|
||||
PermissionRequestHookMode::Skip
|
||||
} else if requirement.is_one_shot() {
|
||||
PermissionRequestHookMode::DenyOnly
|
||||
} else {
|
||||
PermissionRequestHookMode::AllowAndDeny
|
||||
}
|
||||
}
|
||||
|
||||
fn sandbox_outcome_from_tool_error(err: &ToolError) -> Option<&'static str> {
|
||||
match err {
|
||||
ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { .. })) => Some("denied"),
|
||||
@@ -618,3 +672,7 @@ fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {
|
||||
// output so we can evolve heuristics later without touching call sites.
|
||||
"command failed; retry without sandbox?".to_string()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "orchestrator_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
370
codex-rs/core/src/tools/orchestrator_tests.rs
Normal file
370
codex-rs/core/src/tools/orchestrator_tests.rs
Normal file
@@ -0,0 +1,370 @@
|
||||
use super::*;
|
||||
use crate::state::ActiveTurn;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::PermissionRequestPayload;
|
||||
use crate::tools::sandboxing::Sandboxable;
|
||||
use codex_hooks::Hooks;
|
||||
use codex_hooks::HooksConfig;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
use codex_protocol::exec_output::StreamOutput;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_sandboxing::SandboxablePreference;
|
||||
use futures::future::BoxFuture;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use tokio::time::timeout;
|
||||
|
||||
#[derive(Default)]
|
||||
struct OneShotProbe {
|
||||
attempts: usize,
|
||||
}
|
||||
|
||||
impl Approvable<()> for OneShotProbe {
|
||||
type ApprovalKey = String;
|
||||
|
||||
fn approval_keys(&self, _req: &()) -> Vec<Self::ApprovalKey> {
|
||||
Vec::new()
|
||||
}
|
||||
|
||||
fn exec_approval_requirement(&self, _req: &()) -> Option<ExecApprovalRequirement> {
|
||||
Some(ExecApprovalRequirement::NeedsOneShotApproval {
|
||||
reason: Some("one run only".to_string()),
|
||||
})
|
||||
}
|
||||
|
||||
fn permission_request_payload(&self, _req: &()) -> Option<PermissionRequestPayload> {
|
||||
Some(PermissionRequestPayload::bash(
|
||||
"probe".to_string(),
|
||||
Some("one-shot probe".to_string()),
|
||||
))
|
||||
}
|
||||
|
||||
fn start_approval_async<'a>(
|
||||
&'a mut self,
|
||||
_req: &'a (),
|
||||
ctx: ApprovalCtx<'a>,
|
||||
) -> BoxFuture<'a, ReviewDecision> {
|
||||
Box::pin(async move {
|
||||
let cwd = ctx
|
||||
.turn
|
||||
.environments
|
||||
.primary()
|
||||
.expect("primary environment")
|
||||
.cwd()
|
||||
.to_abs_path()
|
||||
.expect("local environment cwd");
|
||||
ctx.session
|
||||
.request_command_approval(
|
||||
ctx.turn,
|
||||
ctx.call_id.to_string(),
|
||||
ctx.approval_id,
|
||||
Some(ctx.approval_purpose),
|
||||
/*environment_id*/ None,
|
||||
vec!["probe".to_string()],
|
||||
cwd,
|
||||
ctx.retry_reason,
|
||||
ctx.network_approval_context,
|
||||
/*proposed_execpolicy_amendment*/ None,
|
||||
/*additional_permissions*/ None,
|
||||
Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]),
|
||||
)
|
||||
.await
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
fn install_sequenced_permission_hook(
|
||||
session: &Arc<crate::session::session::Session>,
|
||||
turn: &Arc<crate::session::turn_context::TurnContext>,
|
||||
modes: &[&str],
|
||||
) -> std::path::PathBuf {
|
||||
let home = &turn.config.codex_home;
|
||||
std::fs::create_dir_all(home).expect("recreate codex home");
|
||||
let script_path = home.join("permission_request_hook.py");
|
||||
let log_path = home.join("permission_request_hook_log.jsonl");
|
||||
std::fs::write(
|
||||
&script_path,
|
||||
format!(
|
||||
r#"import json
|
||||
from pathlib import Path
|
||||
import sys
|
||||
|
||||
log_path = Path(r"{log_path}")
|
||||
modes = {modes}
|
||||
payload = json.load(sys.stdin)
|
||||
seen = [] if not log_path.exists() else log_path.read_text(encoding="utf-8").splitlines()
|
||||
with log_path.open("a", encoding="utf-8") as handle:
|
||||
handle.write(json.dumps(payload) + "\n")
|
||||
mode = modes[min(len(seen), len(modes) - 1)]
|
||||
decision = {{"behavior": "allow"}} if mode == "allow" else {{"behavior": "deny", "message": "blocked by test hook"}}
|
||||
print(json.dumps({{"hookSpecificOutput": {{"hookEventName": "PermissionRequest", "decision": decision}}}}))
|
||||
"#,
|
||||
log_path = log_path.display(),
|
||||
modes = serde_json::to_string(modes).expect("serialize hook modes"),
|
||||
),
|
||||
)
|
||||
.expect("write permission hook");
|
||||
std::fs::write(
|
||||
home.join("hooks.json"),
|
||||
serde_json::json!({
|
||||
"hooks": {
|
||||
"PermissionRequest": [{
|
||||
"matcher": "^Bash$",
|
||||
"hooks": [{
|
||||
"type": "command",
|
||||
"command": format!(
|
||||
"{} \"{}\"",
|
||||
if cfg!(windows) { "python" } else { "python3" },
|
||||
script_path.display(),
|
||||
),
|
||||
}],
|
||||
}],
|
||||
},
|
||||
})
|
||||
.to_string(),
|
||||
)
|
||||
.expect("write hooks config");
|
||||
|
||||
let mut shell_argv = session
|
||||
.user_shell()
|
||||
.derive_exec_args("", /*use_login_shell*/ false);
|
||||
let shell_program = shell_argv.remove(0);
|
||||
let _ = shell_argv.pop();
|
||||
session
|
||||
.services
|
||||
.hooks
|
||||
.store(Arc::new(Hooks::new(HooksConfig {
|
||||
feature_enabled: true,
|
||||
bypass_hook_trust: true,
|
||||
config_layer_stack: Some(turn.config.config_layer_stack.clone()),
|
||||
shell_program: Some(shell_program),
|
||||
shell_args: shell_argv,
|
||||
..HooksConfig::default()
|
||||
})));
|
||||
log_path.into_path_buf()
|
||||
}
|
||||
|
||||
async fn next_exec_approval(
|
||||
events: &async_channel::Receiver<codex_protocol::protocol::Event>,
|
||||
) -> codex_protocol::protocol::ExecApprovalRequestEvent {
|
||||
loop {
|
||||
let event = events.recv().await.expect("approval event");
|
||||
if let EventMsg::ExecApprovalRequest(approval) = event.msg {
|
||||
return approval;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Sandboxable for OneShotProbe {
|
||||
fn sandbox_preference(&self) -> SandboxablePreference {
|
||||
SandboxablePreference::Auto
|
||||
}
|
||||
}
|
||||
|
||||
impl ToolRuntime<(), ()> for OneShotProbe {
|
||||
async fn run(
|
||||
&mut self,
|
||||
_req: &(),
|
||||
_attempt: &SandboxAttempt<'_>,
|
||||
_ctx: &ToolCtx,
|
||||
) -> Result<(), ToolError> {
|
||||
self.attempts += 1;
|
||||
if self.attempts == 1 {
|
||||
let output = ExecToolCallOutput {
|
||||
exit_code: 1,
|
||||
stdout: StreamOutput::new(String::new()),
|
||||
stderr: StreamOutput::new("sandbox denied".to_string()),
|
||||
aggregated_output: StreamOutput::new("sandbox denied".to_string()),
|
||||
duration: Duration::from_millis(1),
|
||||
timed_out: false,
|
||||
};
|
||||
Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
|
||||
output: Box::new(output),
|
||||
network_policy_decision: None,
|
||||
})))
|
||||
} else {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn one_shot_retry_uses_distinct_waiter_and_ignores_stale_initial_responses() {
|
||||
let (session, turn, events) = crate::session::tests::make_session_and_context_with_rx().await;
|
||||
*session.active_turn.lock().await = Some(ActiveTurn::default());
|
||||
let call_id = "one-shot-probe".to_string();
|
||||
let tool_ctx = ToolCtx {
|
||||
session: Arc::clone(&session),
|
||||
turn: Arc::clone(&turn),
|
||||
call_id: call_id.clone(),
|
||||
tool_name: codex_tools::ToolName::plain("probe"),
|
||||
};
|
||||
let mut run = tokio::spawn(async move {
|
||||
let mut probe = OneShotProbe::default();
|
||||
let result = ToolOrchestrator::new()
|
||||
.run(
|
||||
&mut probe,
|
||||
&(),
|
||||
&tool_ctx,
|
||||
turn.as_ref(),
|
||||
AskForApproval::UnlessTrusted,
|
||||
)
|
||||
.await;
|
||||
(result, probe.attempts)
|
||||
});
|
||||
|
||||
let initial = events.recv().await.expect("initial approval event");
|
||||
let EventMsg::ExecApprovalRequest(initial) = initial.msg else {
|
||||
panic!("expected initial exec approval");
|
||||
};
|
||||
assert_eq!(initial.call_id, call_id);
|
||||
assert_eq!(initial.approval_purpose, Some(ExecApprovalPurpose::Initial));
|
||||
let initial_id = initial.approval_id.clone().expect("initial callback ID");
|
||||
assert_ne!(initial_id, call_id);
|
||||
Uuid::parse_str(&initial_id).expect("initial callback ID should be a UUID");
|
||||
assert_eq!(
|
||||
initial.effective_available_decisions(),
|
||||
vec![ReviewDecision::Approved, ReviewDecision::Abort]
|
||||
);
|
||||
session
|
||||
.notify_exec_approval(&initial_id, ReviewDecision::Approved)
|
||||
.await;
|
||||
|
||||
let retry = events.recv().await.expect("retry approval event");
|
||||
let EventMsg::ExecApprovalRequest(retry) = retry.msg else {
|
||||
panic!("expected retry exec approval");
|
||||
};
|
||||
assert_eq!(retry.call_id, call_id);
|
||||
assert_eq!(
|
||||
retry.approval_purpose,
|
||||
Some(ExecApprovalPurpose::SandboxRetry)
|
||||
);
|
||||
let retry_id = retry.approval_id.expect("retry callback ID");
|
||||
assert_ne!(retry_id, call_id);
|
||||
assert_ne!(retry_id, initial_id);
|
||||
Uuid::parse_str(&retry_id).expect("retry callback ID should be a UUID");
|
||||
|
||||
for stale_id in [&call_id, &initial_id] {
|
||||
for stale_decision in [ReviewDecision::Approved, ReviewDecision::Abort] {
|
||||
session.notify_exec_approval(stale_id, stale_decision).await;
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
timeout(Duration::from_millis(50), &mut run).await.is_err(),
|
||||
"stale initial responses must not resolve the retry waiter"
|
||||
);
|
||||
|
||||
session
|
||||
.notify_exec_approval(&retry_id, ReviewDecision::Approved)
|
||||
.await;
|
||||
let (result, attempts) = timeout(Duration::from_secs(1), &mut run)
|
||||
.await
|
||||
.expect("orchestrator timed out")
|
||||
.expect("orchestrator task failed");
|
||||
result.expect("approved retry should succeed");
|
||||
assert_eq!(attempts, 2);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn one_shot_hooks_allow_falls_through_and_deny_blocks_each_phase() {
|
||||
for (modes, expected_approvals, expected_attempts, should_succeed) in [
|
||||
(&["allow", "allow"][..], 2, 2, true),
|
||||
(&["deny"][..], 0, 0, false),
|
||||
(&["allow", "deny"][..], 1, 1, false),
|
||||
] {
|
||||
let (session, turn, events) =
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
*session.active_turn.lock().await = Some(ActiveTurn::default());
|
||||
let log_path = install_sequenced_permission_hook(&session, &turn, modes);
|
||||
let tool_ctx = ToolCtx {
|
||||
session: Arc::clone(&session),
|
||||
turn: Arc::clone(&turn),
|
||||
call_id: format!("hook-probe-{}", modes.join("-")),
|
||||
tool_name: codex_tools::ToolName::plain("probe"),
|
||||
};
|
||||
let run = tokio::spawn(async move {
|
||||
let mut probe = OneShotProbe::default();
|
||||
let result = ToolOrchestrator::new()
|
||||
.run(
|
||||
&mut probe,
|
||||
&(),
|
||||
&tool_ctx,
|
||||
turn.as_ref(),
|
||||
AskForApproval::UnlessTrusted,
|
||||
)
|
||||
.await;
|
||||
(result, probe.attempts)
|
||||
});
|
||||
|
||||
for index in 0..expected_approvals {
|
||||
let approval = next_exec_approval(&events).await;
|
||||
assert_eq!(
|
||||
approval.approval_purpose,
|
||||
Some(if index == 0 {
|
||||
ExecApprovalPurpose::Initial
|
||||
} else {
|
||||
ExecApprovalPurpose::SandboxRetry
|
||||
})
|
||||
);
|
||||
session
|
||||
.notify_exec_approval(&approval.effective_approval_id(), ReviewDecision::Approved)
|
||||
.await;
|
||||
}
|
||||
|
||||
let (result, attempts) = timeout(Duration::from_secs(5), run)
|
||||
.await
|
||||
.expect("hook scenario timed out")
|
||||
.expect("hook scenario task failed");
|
||||
assert_eq!(result.is_ok(), should_succeed, "hook modes: {modes:?}");
|
||||
assert_eq!(attempts, expected_attempts, "hook modes: {modes:?}");
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(log_path)
|
||||
.expect("read permission hook log")
|
||||
.lines()
|
||||
.count(),
|
||||
modes.len(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn one_shot_never_bypasses_retry_approval() {
|
||||
let requirement = ExecApprovalRequirement::NeedsOneShotApproval {
|
||||
reason: Some("one run only".to_string()),
|
||||
};
|
||||
|
||||
assert!(!can_bypass_retry_approval(
|
||||
/*strict_auto_review*/ false,
|
||||
&requirement,
|
||||
/*policy_bypasses_approval*/ true,
|
||||
/*has_network_approval_context*/ false,
|
||||
));
|
||||
assert_eq!(
|
||||
permission_request_hook_mode(/*strict_auto_review*/ false, &requirement),
|
||||
PermissionRequestHookMode::DenyOnly,
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn cacheable_approval_keeps_session_retry_and_hook_behavior() {
|
||||
let requirement = ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
};
|
||||
|
||||
assert!(can_bypass_retry_approval(
|
||||
/*strict_auto_review*/ false,
|
||||
&requirement,
|
||||
/*policy_bypasses_approval*/ true,
|
||||
/*has_network_approval_context*/ false,
|
||||
));
|
||||
assert_eq!(
|
||||
permission_request_hook_mode(/*strict_auto_review*/ false, &requirement),
|
||||
PermissionRequestHookMode::AllowAndDeny,
|
||||
);
|
||||
assert_eq!(
|
||||
permission_request_hook_mode(/*strict_auto_review*/ true, &requirement),
|
||||
PermissionRequestHookMode::Skip,
|
||||
);
|
||||
}
|
||||
@@ -127,6 +127,9 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
type ApprovalKey = ApprovalKey;
|
||||
|
||||
fn approval_keys(&self, req: &ShellRequest) -> Vec<Self::ApprovalKey> {
|
||||
if req.exec_approval_requirement.is_one_shot() {
|
||||
return Vec::new();
|
||||
}
|
||||
vec![ApprovalKey {
|
||||
environment_id: req.turn_environment.environment_id.clone(),
|
||||
command: canonicalize_command_for_approval(&req.command),
|
||||
@@ -150,7 +153,11 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
let session = ctx.session;
|
||||
let turn = ctx.turn;
|
||||
let call_id = ctx.call_id.to_string();
|
||||
let approval_id = ctx.approval_id.clone();
|
||||
let approval_purpose = ctx.approval_purpose;
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
let network_approval_context = ctx.network_approval_context.clone();
|
||||
let one_shot = req.exec_approval_requirement.is_one_shot();
|
||||
Box::pin(async move {
|
||||
if let Some(review_id) = guardian_review_id {
|
||||
return review_approval_request(
|
||||
@@ -170,18 +177,19 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
.await;
|
||||
}
|
||||
with_cached_approval(&session.services, "shell", keys, move || async move {
|
||||
let available_decisions = None;
|
||||
let available_decisions =
|
||||
one_shot.then(|| vec![ReviewDecision::Approved, ReviewDecision::Abort]);
|
||||
session
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
/*approval_id*/ None,
|
||||
/*approval_purpose*/ None,
|
||||
approval_id,
|
||||
Some(approval_purpose),
|
||||
environment_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
ctx.network_approval_context.clone(),
|
||||
network_approval_context,
|
||||
req.exec_approval_requirement
|
||||
.proposed_execpolicy_amendment()
|
||||
.cloned(),
|
||||
|
||||
@@ -1,8 +1,41 @@
|
||||
use super::*;
|
||||
use crate::state::ActiveTurn;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecApprovalPurpose;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::sync::Arc;
|
||||
|
||||
fn one_shot_request() -> ShellRequest {
|
||||
let cwd = AbsolutePathBuf::try_from(std::env::current_dir().expect("read current dir"))
|
||||
.expect("current dir is absolute");
|
||||
ShellRequest {
|
||||
command: vec!["echo".to_string(), "one shot".to_string()],
|
||||
turn_environment: TurnEnvironment::new(
|
||||
"remote".to_string(),
|
||||
Arc::new(Environment::default_for_tests()),
|
||||
PathUri::from_abs_path(&cwd),
|
||||
/*shell*/ None,
|
||||
),
|
||||
shell_type: None,
|
||||
hook_command: "echo 'one shot'".to_string(),
|
||||
cwd,
|
||||
timeout_ms: None,
|
||||
cancellation_token: CancellationToken::new(),
|
||||
env: HashMap::new(),
|
||||
explicit_env_overrides: HashMap::new(),
|
||||
network: None,
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
#[cfg(unix)]
|
||||
additional_permissions_preapproved: false,
|
||||
justification: None,
|
||||
exec_approval_requirement: ExecApprovalRequirement::NeedsOneShotApproval {
|
||||
reason: Some("one run only".to_string()),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approval_key_includes_environment_id() {
|
||||
let cwd = AbsolutePathBuf::try_from(std::env::current_dir().expect("read current dir"))
|
||||
@@ -40,3 +73,56 @@ async fn approval_key_includes_environment_id() {
|
||||
|
||||
assert_ne!(original_key, other_key);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn one_shot_approval_has_no_session_cache_key() {
|
||||
let request = one_shot_request();
|
||||
let runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic);
|
||||
|
||||
assert!(runtime.approval_keys(&request).is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn one_shot_approval_routes_by_callback_id() {
|
||||
let (session, turn, events) = crate::session::tests::make_session_and_context_with_rx().await;
|
||||
*session.active_turn.lock().await = Some(ActiveTurn::default());
|
||||
let mut runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic);
|
||||
let request = one_shot_request();
|
||||
let call_id = "command-item";
|
||||
let approval_id = "retry-callback";
|
||||
let approval = runtime.start_approval_async(
|
||||
&request,
|
||||
ApprovalCtx {
|
||||
session: &session,
|
||||
turn: &turn,
|
||||
call_id,
|
||||
approval_id: Some(approval_id.to_string()),
|
||||
approval_purpose: ExecApprovalPurpose::SandboxRetry,
|
||||
guardian_review_id: None,
|
||||
retry_reason: Some("sandbox denied".to_string()),
|
||||
network_approval_context: None,
|
||||
},
|
||||
);
|
||||
let respond = async {
|
||||
let event = events.recv().await.expect("approval event");
|
||||
let EventMsg::ExecApprovalRequest(event) = event.msg else {
|
||||
panic!("expected exec approval");
|
||||
};
|
||||
assert_eq!(event.call_id, call_id);
|
||||
assert_eq!(event.approval_id.as_deref(), Some(approval_id));
|
||||
assert_eq!(
|
||||
event.approval_purpose,
|
||||
Some(ExecApprovalPurpose::SandboxRetry)
|
||||
);
|
||||
assert_eq!(
|
||||
event.effective_available_decisions(),
|
||||
vec![ReviewDecision::Approved, ReviewDecision::Abort]
|
||||
);
|
||||
session
|
||||
.notify_exec_approval(approval_id, ReviewDecision::Approved)
|
||||
.await;
|
||||
};
|
||||
|
||||
let (decision, ()) = tokio::join!(approval, respond);
|
||||
assert_eq!(decision, ReviewDecision::Approved);
|
||||
}
|
||||
|
||||
@@ -158,6 +158,9 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
type ApprovalKey = UnifiedExecApprovalKey;
|
||||
|
||||
fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec<Self::ApprovalKey> {
|
||||
if req.exec_approval_requirement.is_one_shot() {
|
||||
return Vec::new();
|
||||
}
|
||||
vec![UnifiedExecApprovalKey {
|
||||
environment_id: req.turn_environment.environment_id.clone(),
|
||||
command: canonicalize_command_for_approval(&req.command),
|
||||
@@ -177,11 +180,15 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
let session = ctx.session;
|
||||
let turn = ctx.turn;
|
||||
let call_id = ctx.call_id.to_string();
|
||||
let approval_id = ctx.approval_id.clone();
|
||||
let approval_purpose = ctx.approval_purpose;
|
||||
let command = req.command.clone();
|
||||
let environment_id = Some(req.turn_environment.environment_id.clone());
|
||||
let retry_reason = ctx.retry_reason.clone();
|
||||
let reason = retry_reason.clone().or_else(|| req.justification.clone());
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
let network_approval_context = ctx.network_approval_context.clone();
|
||||
let one_shot = req.exec_approval_requirement.is_one_shot();
|
||||
Box::pin(async move {
|
||||
let native_cwd = match req.cwd.to_abs_path() {
|
||||
Ok(c) => c,
|
||||
@@ -211,18 +218,19 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
.await;
|
||||
}
|
||||
with_cached_approval(&session.services, "unified_exec", keys, || async move {
|
||||
let available_decisions = None;
|
||||
let available_decisions =
|
||||
one_shot.then(|| vec![ReviewDecision::Approved, ReviewDecision::Abort]);
|
||||
session
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
/*approval_id*/ None,
|
||||
/*approval_purpose*/ None,
|
||||
approval_id,
|
||||
Some(approval_purpose),
|
||||
environment_id,
|
||||
command,
|
||||
native_cwd,
|
||||
reason,
|
||||
ctx.network_approval_context.clone(),
|
||||
network_approval_context,
|
||||
req.exec_approval_requirement
|
||||
.proposed_execpolicy_amendment()
|
||||
.cloned(),
|
||||
@@ -492,9 +500,12 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS;
|
||||
use crate::state::ActiveTurn;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::LOCAL_ENVIRONMENT_ID;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecApprovalPurpose;
|
||||
use codex_tools::ZshForkConfig;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -552,6 +563,72 @@ mod tests {
|
||||
assert_ne!(original_key, other_key);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn one_shot_approval_has_no_session_cache_key() {
|
||||
let manager = UnifiedExecProcessManager::default();
|
||||
let runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct);
|
||||
let request = test_request(
|
||||
SandboxPermissions::UseDefault,
|
||||
ExecApprovalRequirement::NeedsOneShotApproval {
|
||||
reason: Some("one run only".to_string()),
|
||||
},
|
||||
);
|
||||
|
||||
assert!(runtime.approval_keys(&request).is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn one_shot_approval_routes_by_callback_id() {
|
||||
let (session, turn, events) =
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
*session.active_turn.lock().await = Some(ActiveTurn::default());
|
||||
let manager = UnifiedExecProcessManager::default();
|
||||
let mut runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct);
|
||||
let request = test_request(
|
||||
SandboxPermissions::UseDefault,
|
||||
ExecApprovalRequirement::NeedsOneShotApproval {
|
||||
reason: Some("one run only".to_string()),
|
||||
},
|
||||
);
|
||||
let call_id = "command-item";
|
||||
let approval_id = "retry-callback";
|
||||
let approval = runtime.start_approval_async(
|
||||
&request,
|
||||
ApprovalCtx {
|
||||
session: &session,
|
||||
turn: &turn,
|
||||
call_id,
|
||||
approval_id: Some(approval_id.to_string()),
|
||||
approval_purpose: ExecApprovalPurpose::SandboxRetry,
|
||||
guardian_review_id: None,
|
||||
retry_reason: Some("sandbox denied".to_string()),
|
||||
network_approval_context: None,
|
||||
},
|
||||
);
|
||||
let respond = async {
|
||||
let event = events.recv().await.expect("approval event");
|
||||
let EventMsg::ExecApprovalRequest(event) = event.msg else {
|
||||
panic!("expected exec approval");
|
||||
};
|
||||
assert_eq!(event.call_id, call_id);
|
||||
assert_eq!(event.approval_id.as_deref(), Some(approval_id));
|
||||
assert_eq!(
|
||||
event.approval_purpose,
|
||||
Some(ExecApprovalPurpose::SandboxRetry)
|
||||
);
|
||||
assert_eq!(
|
||||
event.effective_available_decisions(),
|
||||
vec![ReviewDecision::Approved, ReviewDecision::Abort]
|
||||
);
|
||||
session
|
||||
.notify_exec_approval(approval_id, ReviewDecision::Approved)
|
||||
.await;
|
||||
};
|
||||
|
||||
let (decision, ()) = tokio::join!(approval, respond);
|
||||
assert_eq!(decision, ReviewDecision::Approved);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unified_exec_uses_the_trusted_sandbox_cwd() {
|
||||
let cwd_dir = tempdir().expect("create process temp dir");
|
||||
|
||||
@@ -19,6 +19,7 @@ use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::permissions::FileSystemSandboxKind;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::ExecApprovalPurpose;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_sandboxing::SandboxCommand;
|
||||
use codex_sandboxing::SandboxManager;
|
||||
@@ -81,9 +82,19 @@ where
|
||||
F: FnOnce() -> Fut,
|
||||
Fut: Future<Output = ReviewDecision>,
|
||||
{
|
||||
// To be defensive here, don't bother with checking the cache if keys are empty.
|
||||
// Empty keys deliberately make this request one-shot: ask and record the
|
||||
// decision, but neither consult nor populate the session cache.
|
||||
if keys.is_empty() {
|
||||
return fetch().await;
|
||||
let decision = fetch().await;
|
||||
services.session_telemetry.counter(
|
||||
"codex.approval.requested",
|
||||
/*inc*/ 1,
|
||||
&[
|
||||
("tool", tool_name),
|
||||
("approved", decision.to_opaque_string()),
|
||||
],
|
||||
);
|
||||
return decision;
|
||||
}
|
||||
|
||||
let already_approved = {
|
||||
@@ -122,6 +133,12 @@ pub(crate) struct ApprovalCtx<'a> {
|
||||
pub session: &'a Arc<Session>,
|
||||
pub turn: &'a Arc<TurnContext>,
|
||||
pub call_id: &'a str,
|
||||
/// Identifies this specific approval prompt when a tool item can prompt
|
||||
/// more than once (for example, an unsandboxed retry).
|
||||
pub approval_id: Option<String>,
|
||||
/// Describes why this callback is being emitted independently of display
|
||||
/// text or whether it carries a callback-specific ID.
|
||||
pub approval_purpose: ExecApprovalPurpose,
|
||||
/// Guardian review lifecycle ID for this approval, when guardian is reviewing it.
|
||||
///
|
||||
/// This is separate from `call_id`: `call_id` identifies the tool item under
|
||||
@@ -176,6 +193,10 @@ pub(crate) enum ExecApprovalRequirement {
|
||||
/// See core/src/exec_policy.rs for more details on how proposed_execpolicy_amendment is determined.
|
||||
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
|
||||
},
|
||||
/// Approval is required for this invocation, but the decision must not be
|
||||
/// reused from or written to the session approval cache.
|
||||
#[cfg_attr(not(windows), allow(dead_code))]
|
||||
NeedsOneShotApproval { reason: Option<String> },
|
||||
/// Execution forbidden for this tool call.
|
||||
Forbidden { reason: String },
|
||||
}
|
||||
@@ -194,6 +215,10 @@ impl ExecApprovalRequirement {
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn is_one_shot(&self) -> bool {
|
||||
matches!(self, Self::NeedsOneShotApproval { .. })
|
||||
}
|
||||
}
|
||||
|
||||
/// - Never: do not ask
|
||||
|
||||
Reference in New Issue
Block a user