diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 01d5ce1534..c4416a0f74 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -150,7 +150,7 @@ pub(crate) async fn run_pre_tool_use_hooks( permission_mode: hook_permission_mode(turn_context), tool_name: tool_name.name().to_string(), matcher_aliases: tool_name.matcher_aliases().to_vec(), - tool_use_id, + tool_use_id: tool_use_id.clone(), tool_input: tool_input.clone(), }; let hooks = sess.hooks(); @@ -162,9 +162,13 @@ pub(crate) async fn run_pre_tool_use_hooks( should_block, block_reason, additional_contexts, + permission_decision, } = hooks.run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; record_additional_contexts(sess, turn_context, additional_contexts).await; + if let Some(permission_decision) = permission_decision { + turn_context.record_pre_tool_use_permission_decision(tool_use_id, permission_decision); + } if should_block { block_reason.map(|reason| { diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 85fc939ba9..e6801bae94 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -39,6 +39,7 @@ use codex_analytics::build_track_events_context; use codex_config::types::AppToolApproval; use codex_features::Feature; use codex_hooks::PermissionRequestDecision; +use codex_hooks::PreToolUsePermissionDecision; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::McpPermissionPromptAutoApproveContext; use codex_mcp::SandboxState; @@ -954,20 +955,29 @@ async fn maybe_request_mcp_tool_approval( metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, ) -> Option { - if mcp_permission_prompt_is_auto_approved( + let pre_tool_use_permission_decision = turn_context.pre_tool_use_permission_decision(call_id); + let pre_tool_use_asks_user = matches!( + pre_tool_use_permission_decision.as_ref(), + Some(PreToolUsePermissionDecision::Ask { .. }) + ); + let pre_tool_use_allows = matches!( + pre_tool_use_permission_decision.as_ref(), + Some(PreToolUsePermissionDecision::Allow { .. }) + ); + let auto_approved_by_permissions = mcp_permission_prompt_is_auto_approved( turn_context.approval_policy.value(), &turn_context.permission_profile(), McpPermissionPromptAutoApproveContext { approvals_reviewer: Some(turn_context.config.approvals_reviewer), tool_approval_mode: Some(approval_mode), }, - ) { - return None; - } + ); let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref()); let approval_required = requires_mcp_tool_approval(annotations); - if !approval_required && approval_mode != AppToolApproval::Prompt { + let approval_required = !auto_approved_by_permissions + && (approval_required || approval_mode == AppToolApproval::Prompt); + if pre_tool_use_allows || (!approval_required && !pre_tool_use_asks_user) { return None; } @@ -999,35 +1009,38 @@ async fn maybe_request_mcp_tool_approval( let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode); let persistent_approval_key = persistent_mcp_tool_approval_key(invocation, metadata, approval_mode); - if let Some(key) = session_approval_key.as_ref() + if pre_tool_use_permission_decision.is_none() + && let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { return Some(McpToolApprovalDecision::Accept); } - match run_permission_request_hooks( - sess, - turn_context, - call_id, - PermissionRequestPayload { - tool_name: HookToolName::new(hook_tool_name), - tool_input: invocation - .arguments - .clone() - .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), - }, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return Some(McpToolApprovalDecision::Accept); + if pre_tool_use_permission_decision.is_none() { + match run_permission_request_hooks( + sess, + turn_context, + call_id, + PermissionRequestPayload { + tool_name: HookToolName::new(hook_tool_name), + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), + }, + ) + .await + { + Some(PermissionRequestDecision::Allow) => { + return Some(McpToolApprovalDecision::Accept); + } + Some(PermissionRequestDecision::Deny { message }) => { + return Some(McpToolApprovalDecision::Decline { + message: Some(message), + }); + } + None => {} } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); - } - None => {} } let tool_call_mcp_elicitation_enabled = turn_context @@ -1035,7 +1048,7 @@ async fn maybe_request_mcp_tool_approval( .features .enabled(Feature::ToolCallMcpElicitation); - if routes_approval_to_guardian(turn_context) { + if routes_approval_to_guardian(turn_context) && !pre_tool_use_asks_user { let review_id = new_guardian_review_id(); let decision = review_approval_request( sess, @@ -1084,8 +1097,13 @@ async fn maybe_request_mcp_tool_approval( .as_ref() .map(|rendered_template| rendered_template.question.as_str()), ); - question.question = - mcp_tool_approval_question_text(question.question, monitor_reason.as_deref()); + let prompt_reason = match &pre_tool_use_permission_decision { + Some(PreToolUsePermissionDecision::Ask { reason }) => { + reason.as_deref().or(monitor_reason.as_deref()) + } + Some(PreToolUsePermissionDecision::Allow { .. }) | None => monitor_reason.as_deref(), + }; + question.question = mcp_tool_approval_question_text(question.question, prompt_reason); if tool_call_mcp_elicitation_enabled { let request_id = rmcp::model::RequestId::String( format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}").into(), @@ -1103,7 +1121,7 @@ async fn maybe_request_mcp_tool_approval( tool_params_display: tool_params_display.as_deref(), question, message_override: rendered_template.as_ref().and_then(|rendered_template| { - monitor_reason + prompt_reason .is_none() .then_some(rendered_template.elicitation_message.as_str()) }), diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index b879c78fae..53880815ec 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -146,6 +146,7 @@ pub(super) async fn spawn_review_thread( codex_linux_sandbox_exe: parent_turn_context.codex_linux_sandbox_exe.clone(), tool_call_gate: Arc::new(ReadinessFlag::new()), dynamic_tools: parent_turn_context.dynamic_tools.clone(), + pre_tool_use_permission_decisions: Default::default(), truncation_policy: model_info.truncation_policy.into(), turn_metadata_state, turn_skills: TurnSkillsContext::new(parent_turn_context.turn_skills.outcome.clone()), diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 622588fbe8..eda6ee1e4a 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -1,5 +1,6 @@ use super::*; use crate::config::GhostSnapshotConfig; +use codex_hooks::PreToolUsePermissionDecision; use codex_model_provider::SharedModelProvider; use codex_model_provider::create_model_provider; use codex_protocol::models::AdditionalPermissionProfile; @@ -7,6 +8,8 @@ use codex_protocol::protocol::TurnEnvironmentSelection; use codex_sandboxing::compatibility_sandbox_policy_for_permission_profile; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_sandboxing::policy_transforms::effective_network_sandbox_policy; +use std::collections::HashMap; +use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -86,6 +89,8 @@ pub(crate) struct TurnContext { pub(crate) tool_call_gate: Arc, pub(crate) truncation_policy: TruncationPolicy, pub(crate) dynamic_tools: Vec, + pub(crate) pre_tool_use_permission_decisions: + Arc>>, pub(crate) turn_metadata_state: Arc, pub(crate) turn_skills: TurnSkillsContext, pub(crate) turn_timing_state: Arc, @@ -105,6 +110,28 @@ impl TurnContext { self.permission_profile.network_sandbox_policy() } + pub(crate) fn record_pre_tool_use_permission_decision( + &self, + tool_use_id: String, + decision: PreToolUsePermissionDecision, + ) { + self.pre_tool_use_permission_decisions + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(tool_use_id, decision); + } + + pub(crate) fn pre_tool_use_permission_decision( + &self, + tool_use_id: &str, + ) -> Option { + self.pre_tool_use_permission_decisions + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .get(tool_use_id) + .cloned() + } + pub(crate) fn primary_environment(&self) -> Option<&TurnEnvironment> { self.environments.first() } @@ -257,6 +284,7 @@ impl TurnContext { tool_call_gate: Arc::new(ReadinessFlag::new()), truncation_policy, dynamic_tools: self.dynamic_tools.clone(), + pre_tool_use_permission_decisions: self.pre_tool_use_permission_decisions.clone(), turn_metadata_state: self.turn_metadata_state.clone(), turn_skills: self.turn_skills.clone(), turn_timing_state: Arc::clone(&self.turn_timing_state), @@ -547,6 +575,7 @@ impl Session { tool_call_gate: Arc::new(ReadinessFlag::new()), truncation_policy: model_info.truncation_policy.into(), dynamic_tools: session_configuration.dynamic_tools.clone(), + pre_tool_use_permission_decisions: Arc::new(StdMutex::new(HashMap::new())), turn_metadata_state, turn_skills: TurnSkillsContext::new(skills_outcome), turn_timing_state: Arc::new(TurnTimingState::default()), diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 14af2c9c5f..ca7cafb108 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -11,6 +11,7 @@ use crate::session::session::Session; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; use codex_hooks::PermissionRequestDecision; +use codex_hooks::PreToolUsePermissionDecision; use codex_network_proxy::BlockedRequest; use codex_network_proxy::BlockedRequestObserver; use codex_network_proxy::NetworkDecision; @@ -51,6 +52,7 @@ pub(crate) struct NetworkApprovalSpec { pub mode: NetworkApprovalMode, pub trigger: GuardianNetworkAccessTrigger, pub command: String, + pub pre_tool_use_permission_decision: Option, } #[derive(Clone, Debug)] @@ -224,6 +226,7 @@ struct ActiveNetworkApprovalCall { turn_id: String, trigger: GuardianNetworkAccessTrigger, command: String, + pre_tool_use_permission_decision: Option, cancellation_token: CancellationToken, } @@ -267,6 +270,7 @@ impl NetworkApprovalService { turn_id: String, trigger: GuardianNetworkAccessTrigger, command: String, + pre_tool_use_permission_decision: Option, cancellation_token: CancellationToken, ) { let mut calls = self.calls.lock().await; @@ -278,6 +282,7 @@ impl NetworkApprovalService { turn_id, trigger, command, + pre_tool_use_permission_decision, cancellation_token, }), ); @@ -464,13 +469,25 @@ impl NetworkApprovalService { let command = owner_call .as_ref() .map_or_else(|| prompt_command.join(" "), |call| call.command.clone()); - if let Some(permission_request_decision) = run_permission_request_hooks( - &session, - &turn_context, - &guardian_approval_id, - PermissionRequestPayload::bash(command, Some(format!("network-access {target}"))), - ) - .await + let pre_tool_use_permission_decision = owner_call + .as_ref() + .and_then(|call| call.pre_tool_use_permission_decision.as_ref()); + let pre_tool_use_asks_user = matches!( + pre_tool_use_permission_decision, + Some(PreToolUsePermissionDecision::Ask { .. }) + ); + let pre_tool_use_allows = matches!( + pre_tool_use_permission_decision, + Some(PreToolUsePermissionDecision::Allow { .. }) + ); + if pre_tool_use_permission_decision.is_none() + && let Some(permission_request_decision) = run_permission_request_hooks( + &session, + &turn_context, + &guardian_approval_id, + PermissionRequestPayload::bash(command, Some(format!("network-access {target}"))), + ) + .await { match permission_request_decision { PermissionRequestDecision::Allow => { @@ -496,8 +513,17 @@ impl NetworkApprovalService { } } } - let use_guardian = routes_approval_to_guardian(&turn_context); - let guardian_review_id = use_guardian.then(new_guardian_review_id); + if pre_tool_use_allows { + pending + .set_decision(PendingApprovalDecision::AllowOnce) + .await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + return NetworkDecision::Allow; + } + let guardian_review_id = (routes_approval_to_guardian(&turn_context) + && !pre_tool_use_asks_user) + .then(new_guardian_review_id); let approval_decision = if let Some(review_id) = guardian_review_id.clone() { review_approval_request( &session, @@ -526,7 +552,14 @@ impl NetworkApprovalService { /*approval_id*/ None, prompt_command, turn_context.cwd.clone(), - Some(prompt_reason), + match pre_tool_use_permission_decision { + Some(PreToolUsePermissionDecision::Ask { reason }) => { + reason.clone().or(Some(prompt_reason)) + } + Some(PreToolUsePermissionDecision::Allow { .. }) | None => { + Some(prompt_reason) + } + }, Some(network_approval_context.clone()), /*proposed_execpolicy_amendment*/ None, /*additional_permissions*/ None, @@ -710,6 +743,7 @@ pub(crate) async fn begin_network_approval( mode, trigger, command, + pre_tool_use_permission_decision, } = spec?; if !managed_network_active || network.is_none() { return None; @@ -725,6 +759,7 @@ pub(crate) async fn begin_network_approval( turn_id.to_string(), trigger, command, + pre_tool_use_permission_decision, cancellation_token.clone(), ) .await; diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index 37a28b1255..15bdb5e175 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -238,6 +238,7 @@ async fn register_call_with_default_shell_trigger( tty: None, }, "curl https://example.com".to_string(), + /*pre_tool_use_permission_decision*/ None, cancellation_token.clone(), ) .await; @@ -264,6 +265,7 @@ async fn active_call_preserves_triggering_command_context() { "turn-1".to_string(), expected.clone(), "curl https://example.com".to_string(), + /*pre_tool_use_permission_decision*/ None, CancellationToken::new(), ) .await; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index dcb42c36c6..1ea71a28fd 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -27,6 +27,7 @@ use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::default_exec_approval_requirement; use codex_hooks::PermissionRequestDecision; +use codex_hooks::PreToolUsePermissionDecision; use codex_otel::ToolDecisionSource; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; @@ -138,7 +139,23 @@ impl ToolOrchestrator { let otel_tn = &tool_ctx.tool_name; let otel_ci = &tool_ctx.call_id; let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await; - let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review; + let pre_tool_use_permission_decision = tool_ctx + .turn + .pre_tool_use_permission_decision(&tool_ctx.call_id); + let pre_tool_use_asks_user = matches!( + pre_tool_use_permission_decision.as_ref(), + Some(PreToolUsePermissionDecision::Ask { .. }) + ); + let pre_tool_use_allows = matches!( + pre_tool_use_permission_decision.as_ref(), + Some(PreToolUsePermissionDecision::Allow { .. }) + ) && !strict_auto_review; + let pre_tool_use_reason = match &pre_tool_use_permission_decision { + Some(PreToolUsePermissionDecision::Ask { reason }) => reason.clone(), + Some(PreToolUsePermissionDecision::Allow { .. }) | None => None, + }; + let use_guardian = (routes_approval_to_guardian(turn_ctx) || strict_auto_review) + && !pre_tool_use_asks_user; // 1) Approval let mut already_approved = false; @@ -150,15 +167,23 @@ impl ToolOrchestrator { }); match requirement { ExecApprovalRequirement::Skip { .. } => { - if strict_auto_review { - let guardian_review_id = Some(new_guardian_review_id()); + if !strict_auto_review && !pre_tool_use_asks_user { + otel.tool_decision( + otel_tn, + otel_ci, + &ReviewDecision::Approved, + ToolDecisionSource::Config, + ); + } else { + 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, guardian_review_id: guardian_review_id.clone(), - retry_reason: None, + retry_reason: pre_tool_use_reason.clone(), network_approval_context: None, + bypass_approval_cache: pre_tool_use_asks_user, }; let decision = Self::request_approval( tool, @@ -167,48 +192,50 @@ impl ToolOrchestrator { approval_ctx, tool_ctx, /*evaluate_permission_request_hooks*/ false, - &otel, ) .await?; Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) .await?; already_approved = true; - } else { - otel.tool_decision( - otel_tn, - otel_ci, - &ReviewDecision::Approved, - ToolDecisionSource::Config, - ); } } ExecApprovalRequirement::Forbidden { reason } => { return Err(ToolError::Rejected(reason)); } ExecApprovalRequirement::NeedsApproval { reason, .. } => { - 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, - guardian_review_id: guardian_review_id.clone(), - retry_reason: reason, - network_approval_context: None, - }; - let decision = Self::request_approval( - tool, - req, - tool_ctx.call_id.as_str(), - approval_ctx, - tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, - &otel, - ) - .await?; - - Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) + if pre_tool_use_allows { + otel.tool_decision( + otel_tn, + otel_ci, + &ReviewDecision::Approved, + ToolDecisionSource::Config, + ); + } else { + 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, + guardian_review_id: guardian_review_id.clone(), + retry_reason: pre_tool_use_reason.clone().or(reason), + network_approval_context: None, + bypass_approval_cache: pre_tool_use_asks_user, + }; + let decision = Self::request_approval( + tool, + req, + tool_ctx.call_id.as_str(), + approval_ctx, + tool_ctx, + /*evaluate_permission_request_hooks*/ + !strict_auto_review && !pre_tool_use_asks_user, + ) .await?; - already_approved = true; + + Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) + .await?; + already_approved = true; + } } } @@ -318,15 +345,16 @@ impl ToolOrchestrator { let bypass_retry_approval = !strict_auto_review && tool.should_bypass_approval(approval_policy, already_approved) && network_approval_context.is_none(); - if !bypass_retry_approval { + if !bypass_retry_approval && !pre_tool_use_allows { 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, guardian_review_id: guardian_review_id.clone(), - retry_reason: Some(retry_reason), + retry_reason: pre_tool_use_reason.clone().or(Some(retry_reason)), network_approval_context: network_approval_context.clone(), + bypass_approval_cache: pre_tool_use_asks_user, }; let permission_request_run_id = format!("{}:retry", tool_ctx.call_id); @@ -336,8 +364,8 @@ impl ToolOrchestrator { &permission_request_run_id, approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, - &otel, + /*evaluate_permission_request_hooks*/ + !strict_auto_review && !pre_tool_use_asks_user, ) .await?; @@ -389,11 +417,11 @@ impl ToolOrchestrator { approval_ctx: ApprovalCtx<'_>, tool_ctx: &ToolCtx, evaluate_permission_request_hooks: bool, - otel: &codex_otel::SessionTelemetry, ) -> Result where T: ToolRuntime, { + let otel = approval_ctx.turn.session_telemetry.clone(); if evaluate_permission_request_hooks && let Some(permission_request) = tool.permission_request_payload(req) { @@ -428,7 +456,6 @@ impl ToolOrchestrator { None => {} } } - let otel_source = if approval_ctx.guardian_review_id.is_some() { ToolDecisionSource::AutomatedReviewer } else { diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index e720243f2b..0309f7eece 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -135,20 +135,19 @@ impl Approvable for ApplyPatchRuntime { return rx_approve.await.unwrap_or_default(); } - with_cached_approval( - &session.services, - "apply_patch", - approval_keys, - || async move { - let rx_approve = session - .request_patch_approval( - turn, call_id, changes, /*reason*/ None, /*grant_root*/ None, - ) - .await; - rx_approve.await.unwrap_or_default() - }, - ) - .await + let fetch = || async move { + let rx_approve = session + .request_patch_approval( + turn, call_id, changes, /*reason*/ None, /*grant_root*/ None, + ) + .await; + rx_approve.await.unwrap_or_default() + }; + if ctx.bypass_approval_cache { + fetch().await + } else { + with_cached_approval(&session.services, "apply_patch", approval_keys, fetch).await + } }) } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 7f17285db9..f27fdd5c07 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -175,7 +175,7 @@ impl Approvable for ShellRuntime { ) .await; } - with_cached_approval(&session.services, "shell", keys, move || async move { + let fetch = move || async move { let available_decisions = None; session .request_command_approval( @@ -193,8 +193,12 @@ impl Approvable for ShellRuntime { available_decisions, ) .await - }) - .await + }; + if ctx.bypass_approval_cache { + fetch().await + } else { + with_cached_approval(&session.services, "shell", keys, fetch).await + } }) } @@ -236,6 +240,9 @@ impl ToolRuntime for ShellRuntime { tty: None, }, command: req.hook_command.clone(), + pre_tool_use_permission_decision: ctx + .turn + .pre_tool_use_permission_decision(&ctx.call_id), }) } diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index fef8db5ca9..04f711c5ef 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -28,6 +28,7 @@ use codex_execpolicy::Policy; use codex_execpolicy::RuleMatch; use codex_features::Feature; use codex_hooks::PermissionRequestDecision; +use codex_hooks::PreToolUsePermissionDecision; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; @@ -403,38 +404,61 @@ impl CoreShellActionProvider { let call_id = self.call_id.clone(); let approval_id = Some(Uuid::new_v4().to_string()); let source = self.tool_name; - let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id); + let pre_tool_use_permission_decision = + self.turn.pre_tool_use_permission_decision(&self.call_id); + let pre_tool_use_asks_user = matches!( + pre_tool_use_permission_decision.as_ref(), + Some(PreToolUsePermissionDecision::Ask { .. }) + ); + let pre_tool_use_allows = matches!( + pre_tool_use_permission_decision.as_ref(), + Some(PreToolUsePermissionDecision::Allow { .. }) + ); + let guardian_review_id = (routes_approval_to_guardian(&turn) && !pre_tool_use_asks_user) + .then(new_guardian_review_id); Ok(stopwatch .pause_for(async move { - // 1) Run PermissionRequest hooks - let permission_request = PermissionRequestPayload::bash( - codex_shell_command::parse_command::shlex_join(&command), - /*description*/ None, - ); - let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); - match run_permission_request_hooks( - &session, - &turn, - &effective_approval_id, - permission_request, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return PromptDecision { - decision: ReviewDecision::Approved, - guardian_review_id: None, - rejection_message: None, - }; + if pre_tool_use_allows { + return PromptDecision { + decision: ReviewDecision::Approved, + guardian_review_id: None, + rejection_message: None, + }; + } + + // 1) Run PermissionRequest hooks when PreToolUse did not already + // provide the approval directive for this tool call. + if pre_tool_use_permission_decision.is_none() { + let permission_request = PermissionRequestPayload::bash( + codex_shell_command::parse_command::shlex_join(&command), + /*description*/ None, + ); + let effective_approval_id = + approval_id.clone().unwrap_or_else(|| call_id.clone()); + match run_permission_request_hooks( + &session, + &turn, + &effective_approval_id, + permission_request, + ) + .await + { + Some(PermissionRequestDecision::Allow) => { + return PromptDecision { + decision: ReviewDecision::Approved, + guardian_review_id: None, + rejection_message: None, + }; + } + Some(PermissionRequestDecision::Deny { message }) => { + return PromptDecision { + decision: ReviewDecision::Denied, + guardian_review_id: None, + rejection_message: Some(message), + }; + } + None => {} } - Some(PermissionRequestDecision::Deny { message }) => { - return PromptDecision { - decision: ReviewDecision::Denied, - guardian_review_id: None, - rejection_message: Some(message), - }; - } - None => {} } // 2) Route to Guardian if configured @@ -469,7 +493,10 @@ impl CoreShellActionProvider { approval_id, command, workdir.clone(), - /*reason*/ None, + match pre_tool_use_permission_decision { + Some(PreToolUsePermissionDecision::Ask { reason }) => reason, + Some(PreToolUsePermissionDecision::Allow { .. }) | None => None, + }, /*network_approval_context*/ None, /*proposed_execpolicy_amendment*/ None, additional_permissions, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 5206168230..7e255c414a 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -168,7 +168,7 @@ impl Approvable for UnifiedExecRuntime<'_> { ) .await; } - with_cached_approval(&session.services, "unified_exec", keys, || async move { + let fetch = || async move { let available_decisions = None; session .request_command_approval( @@ -186,8 +186,12 @@ impl Approvable for UnifiedExecRuntime<'_> { available_decisions, ) .await - }) - .await + }; + if ctx.bypass_approval_cache { + fetch().await + } else { + with_cached_approval(&session.services, "unified_exec", keys, fetch).await + } }) } @@ -235,6 +239,9 @@ impl<'a> ToolRuntime for UnifiedExecRunt tty: Some(req.tty), }, command: req.hook_command.clone(), + pre_tool_use_permission_decision: ctx + .turn + .pre_tool_use_permission_decision(&ctx.call_id), }) } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 122cd00fad..d5ce6b092b 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -130,6 +130,7 @@ pub(crate) struct ApprovalCtx<'a> { pub guardian_review_id: Option, pub retry_reason: Option, pub network_approval_context: Option, + pub bypass_approval_cache: bool, } #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 48c36d933e..1794a2f4ae 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -13,6 +13,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; use codex_protocol::user_input::UserInput; @@ -33,6 +34,7 @@ use core_test_support::skip_if_windows; use core_test_support::streaming_sse::StreamingSseChunk; use core_test_support::streaming_sse::start_streaming_sse_server; use core_test_support::test_codex::test_codex; +use core_test_support::test_codex::turn_permission_fields; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use serde_json::Value; @@ -67,6 +69,38 @@ fn network_workspace_write_profile() -> PermissionProfile { ) } +async fn submit_turn_without_waiting( + test: &core_test_support::test_codex::TestCodex, + prompt: &str, + approval_policy: AskForApproval, + permission_profile: PermissionProfile, +) -> Result<()> { + let (sandbox_policy, permission_profile) = + turn_permission_fields(permission_profile, test.cwd.path()); + test.codex + .submit(Op::UserTurn { + environments: None, + items: vec![UserInput::Text { + text: prompt.into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd.path().to_path_buf(), + approval_policy, + approvals_reviewer: None, + sandbox_policy, + permission_profile, + model: test.session_configured.model.clone(), + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + }) + .await?; + Ok(()) +} + fn write_stop_hook(home: &Path, block_prompts: &[&str]) -> Result<()> { let script_path = home.join("stop_hook.py"); let log_path = home.join("stop_hook_log.jsonl"); @@ -237,6 +271,22 @@ if mode == "json_deny": "permissionDecisionReason": reason }} }})) +elif mode == "json_allow": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "allow", + "permissionDecisionReason": reason + }} + }})) +elif mode == "json_ask": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "ask", + "permissionDecisionReason": reason + }} + }})) elif mode == "context": print(json.dumps({{ "hookSpecificOutput": {{ @@ -1390,6 +1440,207 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test] +async fn pre_tool_use_allow_skips_shell_command_approval() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-allow-shell-command"; + let marker = std::env::temp_dir().join("pretooluse-allow-shell-command-marker"); + let command = format!("rm -f {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "pre tool use allowed it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "json_allow", "approved by hook") + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + fs::write(&marker, "seed").context("create pre tool use marker")?; + + test.submit_turn_with_approval_and_permission_profile( + "run the shell command after pre tool use approval", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .await?; + + assert_eq!(responses.requests().len(), 2); + assert!( + !marker.exists(), + "pre tool use allow should execute without prompting" + ); + assert!( + timeout( + Duration::from_secs(2), + wait_for_event(&test.codex, |event| matches!( + event, + EventMsg::ExecApprovalRequest(_) + )) + ) + .await + .is_err(), + "pre tool use allow should bypass the approval prompt" + ); + + Ok(()) +} + +#[tokio::test] +async fn pre_tool_use_ask_prompts_user_with_hook_reason() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-ask-shell-command"; + let marker = std::env::temp_dir().join("pretooluse-ask-shell-command-marker"); + let command = format!("rm -f {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let _responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "pre tool use asked first"), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_function_call( + "pretooluse-ask-shell-command-again", + "shell_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-3"), + ]), + sse(vec![ + ev_response_created("resp-4"), + ev_assistant_message("msg-2", "pre tool use asked again"), + ev_completed("resp-4"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "json_ask", "please confirm") + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + fs::write(&marker, "seed").context("create pre tool use marker")?; + + submit_turn_without_waiting( + &test, + "run the shell command after pre tool use asks", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .await?; + + let approval_event = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::ExecApprovalRequest(_)) + }) + .await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected pre tool use ask to prompt the user"); + }; + assert_eq!(approval.reason.as_deref(), Some("please confirm")); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert!( + !marker.exists(), + "approved command should remove marker file" + ); + + fs::write(&marker, "seed again").context("recreate pre tool use marker")?; + + submit_turn_without_waiting( + &test, + "run the same shell command after pre tool use asks again", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .await?; + + let second_approval_event = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::ExecApprovalRequest(_)) + }) + .await; + let EventMsg::ExecApprovalRequest(second_approval) = second_approval_event else { + panic!("expected pre tool use ask to bypass cached approval"); + }; + assert_eq!(second_approval.reason.as_deref(), Some("please confirm")); + + test.codex + .submit(Op::ExecApproval { + id: second_approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert!( + !marker.exists(), + "second approved command should remove marker file" + ); + + Ok(()) +} + #[tokio::test] async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json b/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json index ba6bb3401b..1107906a96 100644 --- a/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json +++ b/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json @@ -52,12 +52,29 @@ "type": "object" }, "PreToolUsePermissionDecisionWire": { - "enum": [ - "allow", - "deny", - "ask" - ], - "type": "string" + "oneOf": [ + { + "enum": [ + "allow", + "deny" + ], + "type": "string" + }, + { + "description": "Always requests explicit human-user confirmation.", + "enum": [ + "ask" + ], + "type": "string" + }, + { + "description": "Reserved for the Codex exec integration.\n\nCodex does not implement this decision yet.", + "enum": [ + "defer" + ], + "type": "string" + } + ] } }, "properties": { diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 464c7f8608..aa5e20bb63 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -16,10 +16,17 @@ pub(crate) struct SessionStartOutput { pub(crate) struct PreToolUseOutput { pub universal: UniversalOutput, pub block_reason: Option, + pub permission_decision: Option, pub additional_context: Option, pub invalid_reason: Option, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum PreToolUsePermissionDecision { + Allow { reason: Option }, + Ask { reason: Option }, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PermissionRequestDecision { Allow, @@ -125,10 +132,16 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { } else { None }; + let permission_decision = if invalid_reason.is_none() { + hook_specific_output.and_then(pre_tool_use_permission_decision) + } else { + None + }; Some(PreToolUseOutput { universal, block_reason, + permission_decision, additional_context, invalid_reason, }) @@ -344,12 +357,8 @@ fn unsupported_pre_tool_use_hook_specific_output( Some("PreToolUse hook returned unsupported updatedInput".to_string()) } else { match output.permission_decision { - Some(PreToolUsePermissionDecisionWire::Allow) => { - Some("PreToolUse hook returned unsupported permissionDecision:allow".to_string()) - } - Some(PreToolUsePermissionDecisionWire::Ask) => { - Some("PreToolUse hook returned unsupported permissionDecision:ask".to_string()) - } + Some(PreToolUsePermissionDecisionWire::Allow) + | Some(PreToolUsePermissionDecisionWire::Ask) => None, Some(PreToolUsePermissionDecisionWire::Deny) => { if output .permission_decision_reason @@ -362,6 +371,9 @@ fn unsupported_pre_tool_use_hook_specific_output( None } } + Some(PreToolUsePermissionDecisionWire::Defer) => { + Some("PreToolUse hook returned unsupported permissionDecision:defer".to_string()) + } None => { if output.permission_decision_reason.is_some() { Some("PreToolUse hook returned permissionDecisionReason without permissionDecision".to_string()) @@ -403,6 +415,29 @@ fn invalid_pre_tool_use_reason_message() -> String { .to_string() } +fn pre_tool_use_permission_decision( + output: &crate::schema::PreToolUseHookSpecificOutputWire, +) -> Option { + match output.permission_decision { + Some(PreToolUsePermissionDecisionWire::Allow) => { + Some(PreToolUsePermissionDecision::Allow { + reason: output + .permission_decision_reason + .as_deref() + .and_then(trimmed_reason), + }) + } + Some(PreToolUsePermissionDecisionWire::Ask) => Some(PreToolUsePermissionDecision::Ask { + reason: output + .permission_decision_reason + .as_deref() + .and_then(trimmed_reason), + }), + Some(PreToolUsePermissionDecisionWire::Deny | PreToolUsePermissionDecisionWire::Defer) + | None => None, + } +} + fn trimmed_reason(reason: &str) -> Option { let trimmed = reason.trim(); if trimmed.is_empty() { diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 39d0e61258..73193e7622 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -16,6 +16,7 @@ use crate::engine::ConfiguredHandler; use crate::engine::command_runner::CommandRunResult; use crate::engine::dispatcher; use crate::engine::output_parser; +use crate::engine::output_parser::PreToolUsePermissionDecision as ParsedPermissionDecision; use crate::schema::PreToolUseCommandInput; #[derive(Debug, Clone)] @@ -37,13 +38,25 @@ pub struct PreToolUseOutcome { pub hook_events: Vec, pub should_block: bool, pub block_reason: Option, + pub permission_decision: Option, pub additional_contexts: Vec, } +/// Hook-authored approval guidance to apply later at the concrete permission +/// boundary for the same tool call. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PreToolUsePermissionDecision { + /// Skip an otherwise-required permission prompt when policy allows it. + Allow { reason: Option }, + /// Require explicit human-user confirmation. + Ask { reason: Option }, +} + #[derive(Debug, Default, PartialEq, Eq)] struct PreToolUseHandlerData { should_block: bool, block_reason: Option, + permission_decision: Option, additional_contexts_for_model: Vec, } @@ -80,6 +93,7 @@ pub(crate) async fn run( hook_events: Vec::new(), should_block: false, block_reason: None, + permission_decision: None, additional_contexts: Vec::new(), }; } @@ -111,6 +125,11 @@ pub(crate) async fn run( let block_reason = results .iter() .find_map(|result| result.data.block_reason.clone()); + let permission_decision = resolve_permission_decision( + results + .iter() + .filter_map(|result| result.data.permission_decision.as_ref()), + ); let additional_contexts = common::flatten_additional_contexts( results .iter() @@ -126,10 +145,33 @@ pub(crate) async fn run( .collect(), should_block, block_reason, + permission_decision, additional_contexts, } } +fn resolve_permission_decision<'a>( + decisions: impl IntoIterator, +) -> Option { + let mut resolved_allow = None; + let mut resolved_ask = None; + for decision in decisions { + match decision { + PreToolUsePermissionDecision::Allow { reason } => { + resolved_allow = Some(PreToolUsePermissionDecision::Allow { + reason: reason.clone(), + }); + } + PreToolUsePermissionDecision::Ask { reason } => { + resolved_ask = Some(PreToolUsePermissionDecision::Ask { + reason: reason.clone(), + }); + } + } + } + resolved_ask.or(resolved_allow) +} + /// Serializes command stdin for a selected `PreToolUse` hook. /// /// Handler selection may include internal matcher aliases, but hook stdin keeps @@ -160,6 +202,7 @@ fn parse_completed( let mut status = HookRunStatus::Completed; let mut should_block = false; let mut block_reason = None; + let mut permission_decision = None; let mut additional_contexts_for_model = Vec::new(); match run_result.error.as_deref() { @@ -204,6 +247,29 @@ fn parse_completed( text: reason, }); } + if let Some(parsed_decision) = parsed.permission_decision { + let (decision, reason) = match parsed_decision { + ParsedPermissionDecision::Allow { reason } => ( + PreToolUsePermissionDecision::Allow { + reason: reason.clone(), + }, + reason, + ), + ParsedPermissionDecision::Ask { reason } => ( + PreToolUsePermissionDecision::Ask { + reason: reason.clone(), + }, + reason, + ), + }; + if let Some(reason) = reason { + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: reason, + }); + } + permission_decision = Some(decision); + } } } else if trimmed_stdout.starts_with('{') || trimmed_stdout.starts_with('[') { status = HookRunStatus::Failed; @@ -257,6 +323,7 @@ fn parse_completed( data: PreToolUseHandlerData { should_block, block_reason, + permission_decision, additional_contexts_for_model, }, } @@ -267,6 +334,7 @@ fn serialization_failure_outcome(hook_events: Vec) -> PreToo hook_events, should_block: false, block_reason: None, + permission_decision: None, additional_contexts: Vec::new(), } } @@ -283,6 +351,7 @@ mod tests { use pretty_assertions::assert_eq; use super::PreToolUseHandlerData; + use super::PreToolUsePermissionDecision; use super::command_input_json; use super::parse_completed; use super::preview; @@ -319,6 +388,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); @@ -349,6 +419,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); @@ -379,6 +450,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + permission_decision: None, additional_contexts_for_model: vec!["remember this".to_string()], } ); @@ -399,7 +471,7 @@ mod tests { } #[test] - fn unsupported_permission_decision_fails_open() { + fn permission_decision_ask_is_user_visible_without_model_context() { let parsed = parse_completed( &handler(), run_result( @@ -415,6 +487,73 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: Some(PreToolUsePermissionDecision::Ask { + reason: Some("please confirm".to_string()), + }), + additional_contexts_for_model: Vec::new(), + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "please confirm".to_string(), + }] + ); + } + + #[test] + fn permission_decision_allow_is_user_visible_without_model_context() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","permissionDecisionReason":"approved by policy"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + permission_decision: Some(PreToolUsePermissionDecision::Allow { + reason: Some("approved by policy".to_string()), + }), + additional_contexts_for_model: Vec::new(), + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "approved by policy".to_string(), + }] + ); + } + + #[test] + fn permission_decision_defer_stays_unimplemented() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"defer"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); @@ -423,7 +562,7 @@ mod tests { parsed.completed.run.entries, vec![HookOutputEntry { kind: HookOutputEntryKind::Error, - text: "PreToolUse hook returned unsupported permissionDecision:ask".to_string(), + text: "PreToolUse hook returned unsupported permissionDecision:defer".to_string(), }] ); } @@ -441,6 +580,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); @@ -471,6 +611,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + permission_decision: None, additional_contexts_for_model: vec!["nope".to_string()], } ); @@ -503,6 +644,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); @@ -523,6 +665,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); @@ -549,6 +692,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("blocked by policy".to_string()), + permission_decision: None, additional_contexts_for_model: Vec::new(), } ); diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 4e16969a58..2d2ff9d589 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -34,6 +34,7 @@ pub use events::permission_request::PermissionRequestRequest; pub use events::post_tool_use::PostToolUseOutcome; pub use events::post_tool_use::PostToolUseRequest; pub use events::pre_tool_use::PreToolUseOutcome; +pub use events::pre_tool_use::PreToolUsePermissionDecision; pub use events::pre_tool_use::PreToolUseRequest; pub use events::session_start::SessionStartOutcome; pub use events::session_start::SessionStartRequest; diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index d08cce6ee2..7cd9242af3 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -198,8 +198,14 @@ pub(crate) enum PreToolUsePermissionDecisionWire { Allow, #[serde(rename = "deny")] Deny, + /// Always requests explicit human-user confirmation. #[serde(rename = "ask")] Ask, + /// Reserved for the Codex exec integration. + /// + /// Codex does not implement this decision yet. + #[serde(rename = "defer")] + Defer, } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)]