diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 9a928545152..2872e76459b 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -10,6 +10,7 @@ use codex_hooks::PermissionRequestRequest; use codex_hooks::PostToolUseOutcome; use codex_hooks::PostToolUseRequest; use codex_hooks::PreToolUseOutcome; +use codex_hooks::PreToolUsePermissionDecision; use codex_hooks::PreToolUseRequest; use codex_hooks::SessionStartOutcome; use codex_hooks::UserPromptSubmitOutcome; @@ -140,7 +141,7 @@ pub(crate) async fn run_pre_tool_use_hooks( tool_use_id: String, tool_name: &HookToolName, tool_input: &Value, -) -> Option { +) -> Result, String> { let request = PreToolUseRequest { session_id: sess.conversation_id, turn_id: turn_context.sub_id.clone(), @@ -161,24 +162,28 @@ pub(crate) async fn run_pre_tool_use_hooks( hook_events, should_block, block_reason, + permission_decision, } = hooks.run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; if should_block { - block_reason.map(|reason| { - if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch") - && let Some(command) = tool_input.get("command").and_then(Value::as_str) - { - format!("Command blocked by PreToolUse hook: {reason}. Command: {command}") - } else { - format!( - "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", - tool_name.name() - ) - } - }) + Err(block_reason.map_or_else( + || "Tool call blocked by PreToolUse hook".to_string(), + |reason| { + if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch") + && let Some(command) = tool_input.get("command").and_then(Value::as_str) + { + format!("Command blocked by PreToolUse hook: {reason}. Command: {command}") + } else { + format!( + "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", + tool_name.name() + ) + } + }, + )) } else { - None + Ok(permission_decision) } } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 1b1ddbf1eab..2c47260b74a 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -37,6 +37,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; @@ -87,6 +88,7 @@ const MCP_TOOL_CALL_EVENT_RESULT_MAX_BYTES: usize = DEFAULT_OUTPUT_BYTES_CAP; /// Handles the specified tool call dispatches the appropriate /// `McpToolCallBegin` and `McpToolCallEnd` events to the `Session`. +#[allow(clippy::too_many_arguments)] pub(crate) async fn handle_mcp_tool_call( sess: Arc, turn_context: &Arc, @@ -95,6 +97,7 @@ pub(crate) async fn handle_mcp_tool_call( tool_name: String, hook_tool_name: String, arguments: String, + pre_tool_use_permission_decision: Option, ) -> HandledMcpToolCall { // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON // is not. @@ -191,7 +194,7 @@ pub(crate) async fn handle_mcp_tool_call( }); notify_mcp_tool_call_event(sess.as_ref(), turn_context.as_ref(), tool_call_begin_event).await; - if let Some(decision) = maybe_request_mcp_tool_approval( + if let Some(decision) = maybe_request_mcp_tool_approval_with_pre_tool_use_decision( &sess, turn_context, &call_id, @@ -199,6 +202,7 @@ pub(crate) async fn handle_mcp_tool_call( &hook_tool_name, metadata.as_ref(), approval_mode, + pre_tool_use_permission_decision.as_ref(), ) .await { @@ -1051,6 +1055,7 @@ fn mcp_tool_approval_prompt_options( } } +#[cfg(test)] async fn maybe_request_mcp_tool_approval( sess: &Arc, turn_context: &Arc, @@ -1060,6 +1065,41 @@ async fn maybe_request_mcp_tool_approval( metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, ) -> Option { + maybe_request_mcp_tool_approval_with_pre_tool_use_decision( + sess, + turn_context, + call_id, + invocation, + hook_tool_name, + metadata, + approval_mode, + None, + ) + .await +} + +#[allow(clippy::too_many_arguments)] +async fn maybe_request_mcp_tool_approval_with_pre_tool_use_decision( + sess: &Arc, + turn_context: &Arc, + call_id: &str, + invocation: &McpInvocation, + hook_tool_name: &str, + metadata: Option<&McpToolApprovalMetadata>, + approval_mode: AppToolApproval, + pre_tool_use_permission_decision: Option<&PreToolUsePermissionDecision>, +) -> Option { + if matches!( + pre_tool_use_permission_decision, + Some(PreToolUsePermissionDecision::Allow { .. }) + ) { + return None; + } + + let force_user_prompt = matches!( + pre_tool_use_permission_decision, + Some(PreToolUsePermissionDecision::Ask { .. }) + ); if mcp_permission_prompt_is_auto_approved( turn_context.approval_policy.value(), &turn_context.permission_profile(), @@ -1067,20 +1107,23 @@ async fn maybe_request_mcp_tool_approval( approvals_reviewer: Some(turn_context.config.approvals_reviewer), tool_approval_mode: Some(approval_mode), }, - ) { + ) && !force_user_prompt + { 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 { + if !approval_required && approval_mode != AppToolApproval::Prompt && !force_user_prompt { return None; } - let mut monitor_reason = None; + let mut monitor_reason = pre_tool_use_permission_decision + .and_then(PreToolUsePermissionDecision::reason) + .map(ToString::to_string); let auto_approved_by_policy = approval_mode == AppToolApproval::Approve; - if auto_approved_by_policy { + if auto_approved_by_policy && !force_user_prompt { match maybe_monitor_auto_approved_mcp_tool_call( sess, turn_context, @@ -1105,7 +1148,8 @@ 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 !force_user_prompt + && let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { return Some(McpToolApprovalDecision::Accept); @@ -1114,23 +1158,25 @@ async fn maybe_request_mcp_tool_approval( let approval_request = build_mcp_tool_approval_request(call_id, hook_tool_name, invocation, metadata); - match run_permission_request_hooks(sess, turn_context, call_id, { - let Some(payload) = approval_request.permission_request_payload() else { - unreachable!("MCP approvals always project a permission request payload"); - }; - payload - }) - .await - { - Some(PermissionRequestDecision::Allow) => { - return Some(McpToolApprovalDecision::Accept); - } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); + if !force_user_prompt { + match run_permission_request_hooks(sess, turn_context, call_id, { + let Some(payload) = approval_request.permission_request_payload() else { + unreachable!("MCP approvals always project a permission request payload"); + }; + payload + }) + .await + { + Some(PermissionRequestDecision::Allow) => { + return Some(McpToolApprovalDecision::Accept); + } + Some(PermissionRequestDecision::Deny { message }) => { + return Some(McpToolApprovalDecision::Decline { + message: Some(message), + }); + } + None => {} } - None => {} } let tool_call_mcp_elicitation_enabled = turn_context @@ -1138,7 +1184,7 @@ async fn maybe_request_mcp_tool_approval( .features .enabled(Feature::ToolCallMcpElicitation); - if routes_approval_to_guardian(turn_context) { + if !force_user_prompt && routes_approval_to_guardian(turn_context) { let review_id = new_guardian_review_id(); let decision = review_approval_request( sess, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index f45ea6c38cb..8de5d679605 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2090,6 +2090,47 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { assert_eq!(decision, None); } +#[tokio::test] +async fn pre_tool_use_allow_skips_mcp_approval_flow() { + let (session, turn_context) = make_session_and_context().await; + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: "custom_server".to_string(), + tool: "dangerous_tool".to_string(), + arguments: None, + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations( + Some(false), + Some(true), + /*open_world*/ None, + )), + connector_id: None, + connector_name: None, + connector_description: None, + tool_title: Some("Dangerous Tool".to_string()), + tool_description: None, + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + }; + + let decision = maybe_request_mcp_tool_approval_with_pre_tool_use_decision( + &session, + &turn_context, + "call-pretool-allow", + &invocation, + "mcp__test__tool", + Some(&metadata), + AppToolApproval::Prompt, + Some(&codex_hooks::PreToolUsePermissionDecision::Allow { reason: None }), + ) + .await; + + assert_eq!(decision, None); +} + #[tokio::test] async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { use wiremock::Mock; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 89633daf35d..6a74b922362 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -975,6 +975,7 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an turn: Arc::clone(&turn), call_id: "probe-call".to_string(), tool_name: "probe".to_string(), + pre_tool_use_permission_decision: None, }; orchestrator @@ -993,6 +994,178 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an Ok(()) } +#[tokio::test] +async fn pre_tool_use_allow_bypasses_required_approval() -> anyhow::Result<()> { + #[derive(Default)] + struct ApprovalProbeRuntime { + approval_calls: Vec<(Option, Option)>, + } + + impl crate::tools::sandboxing::Approvable<()> for ApprovalProbeRuntime { + type ApprovalKey = String; + + fn approval_keys(&self, _req: &()) -> Vec { + vec!["probe".to_string()] + } + + fn exec_approval_requirement( + &self, + _req: &(), + ) -> Option { + Some( + crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval { + reason: Some("required".to_string()), + proposed_execpolicy_amendment: None, + }, + ) + } + + fn start_approval_async<'a>( + &'a mut self, + _req: &'a (), + ctx: crate::tools::sandboxing::ApprovalCtx<'a>, + ) -> futures::future::BoxFuture<'a, ReviewDecision> { + self.approval_calls + .push((ctx.guardian_review_id.clone(), ctx.retry_reason)); + Box::pin(async { ReviewDecision::Approved }) + } + } + + impl crate::tools::sandboxing::Sandboxable for ApprovalProbeRuntime { + fn sandbox_preference(&self) -> codex_sandboxing::SandboxablePreference { + codex_sandboxing::SandboxablePreference::Auto + } + } + + impl crate::tools::sandboxing::ToolRuntime<(), ()> for ApprovalProbeRuntime { + async fn run( + &mut self, + _req: &(), + _attempt: &crate::tools::sandboxing::SandboxAttempt<'_>, + _ctx: &crate::tools::sandboxing::ToolCtx, + ) -> Result<(), crate::tools::sandboxing::ToolError> { + Ok(()) + } + } + + let (session, turn) = make_session_and_context().await; + let session = Arc::new(session); + let turn = Arc::new(turn); + let mut orchestrator = crate::tools::orchestrator::ToolOrchestrator::new(); + let mut tool = ApprovalProbeRuntime::default(); + let tool_ctx = crate::tools::sandboxing::ToolCtx { + session: Arc::clone(&session), + turn: Arc::clone(&turn), + call_id: "allow-call".to_string(), + tool_name: "probe".to_string(), + pre_tool_use_permission_decision: Some(codex_hooks::PreToolUsePermissionDecision::Allow { + reason: None, + }), + }; + + orchestrator + .run( + &mut tool, + &(), + &tool_ctx, + turn.as_ref(), + AskForApproval::OnRequest, + ) + .await + .expect("pre-tool-use allow should bypass approval"); + + assert_eq!(tool.approval_calls, vec![]); + Ok(()) +} + +#[tokio::test] +async fn pre_tool_use_ask_forces_user_prompt_ahead_of_guardian() -> anyhow::Result<()> { + #[derive(Default)] + struct ApprovalProbeRuntime { + approval_calls: Vec<(Option, Option)>, + } + + impl crate::tools::sandboxing::Approvable<()> for ApprovalProbeRuntime { + type ApprovalKey = String; + + fn approval_keys(&self, _req: &()) -> Vec { + vec!["probe".to_string()] + } + + fn exec_approval_requirement( + &self, + _req: &(), + ) -> Option { + Some(crate::tools::sandboxing::ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }) + } + + fn start_approval_async<'a>( + &'a mut self, + _req: &'a (), + ctx: crate::tools::sandboxing::ApprovalCtx<'a>, + ) -> futures::future::BoxFuture<'a, ReviewDecision> { + self.approval_calls + .push((ctx.guardian_review_id.clone(), ctx.retry_reason)); + Box::pin(async { ReviewDecision::Approved }) + } + } + + impl crate::tools::sandboxing::Sandboxable for ApprovalProbeRuntime { + fn sandbox_preference(&self) -> codex_sandboxing::SandboxablePreference { + codex_sandboxing::SandboxablePreference::Auto + } + } + + impl crate::tools::sandboxing::ToolRuntime<(), ()> for ApprovalProbeRuntime { + async fn run( + &mut self, + _req: &(), + _attempt: &crate::tools::sandboxing::SandboxAttempt<'_>, + _ctx: &crate::tools::sandboxing::ToolCtx, + ) -> Result<(), crate::tools::sandboxing::ToolError> { + Ok(()) + } + } + + let (session, mut turn) = make_session_and_context().await; + let session = Arc::new(session); + let mut config = (*turn.config).clone(); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + turn.config = Arc::new(config); + let turn = Arc::new(turn); + let mut orchestrator = crate::tools::orchestrator::ToolOrchestrator::new(); + let mut tool = ApprovalProbeRuntime::default(); + let tool_ctx = crate::tools::sandboxing::ToolCtx { + session: Arc::clone(&session), + turn: Arc::clone(&turn), + call_id: "ask-call".to_string(), + tool_name: "probe".to_string(), + pre_tool_use_permission_decision: Some(codex_hooks::PreToolUsePermissionDecision::Ask { + reason: Some("please confirm".to_string()), + }), + }; + + orchestrator + .run( + &mut tool, + &(), + &tool_ctx, + turn.as_ref(), + AskForApproval::OnFailure, + ) + .await + .expect("pre-tool-use ask should force approval"); + + assert_eq!( + tool.approval_calls, + vec![(None, Some("please confirm".to_string()))] + ); + Ok(()) +} + #[tokio::test] async fn workspace_write_turns_continue_to_expose_managed_network_proxy() -> anyhow::Result<()> { let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); @@ -8157,6 +8330,7 @@ async fn create_goal_tool_rejects_existing_goal() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await .expect("initial create_goal should succeed"); @@ -8177,6 +8351,7 @@ async fn create_goal_tool_rejects_existing_goal() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; @@ -8219,6 +8394,7 @@ async fn update_goal_tool_rejects_pausing_goal() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await .expect("initial create_goal should succeed"); @@ -8238,6 +8414,7 @@ async fn update_goal_tool_rejects_pausing_goal() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; @@ -8279,6 +8456,7 @@ async fn update_goal_tool_marks_goal_complete() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await .expect("initial create_goal should succeed"); @@ -8298,6 +8476,7 @@ async fn update_goal_tool_marks_goal_complete() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await .expect("update_goal should mark the goal complete"); @@ -8385,6 +8564,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; @@ -8458,6 +8638,7 @@ async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request() }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index d6a87d466ae..75d466cc7bf 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -349,6 +349,7 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; @@ -470,6 +471,7 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; @@ -516,6 +518,7 @@ async fn guardian_allows_unified_exec_additional_permissions_requests_past_polic }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; @@ -637,6 +640,7 @@ async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_per }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index cf6b3fac91e..0d2ccf935ee 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -7,6 +7,7 @@ use crate::tools::TELEMETRY_PREVIEW_MAX_LINES; use crate::tools::TELEMETRY_PREVIEW_TRUNCATION_NOTICE; use crate::turn_diff_tracker::TurnDiffTracker; use crate::unified_exec::resolve_max_tokens; +use codex_hooks::PreToolUsePermissionDecision; use codex_protocol::mcp::CallToolResult; use codex_protocol::models::DEFAULT_IMAGE_DETAIL; use codex_protocol::models::FunctionCallOutputBody; @@ -54,6 +55,7 @@ pub struct ToolInvocation { pub tool_name: ToolName, pub source: ToolCallSource, pub payload: ToolPayload, + pub pre_tool_use_permission_decision: Option, } #[derive(Clone, Debug)] diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 294e1614835..79e19703a71 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -343,6 +343,7 @@ impl ToolHandler for ApplyPatchHandler { call_id, tool_name, payload, + pre_tool_use_permission_decision, .. } = invocation; @@ -421,6 +422,7 @@ impl ToolHandler for ApplyPatchHandler { turn: turn.clone(), call_id: call_id.clone(), tool_name: tool_name.display(), + pre_tool_use_permission_decision, }; let out = orchestrator .run( @@ -473,6 +475,7 @@ pub(crate) async fn intercept_apply_patch( tracker: Option<&SharedTurnDiffTracker>, call_id: &str, tool_name: &str, + pre_tool_use_permission_decision: Option, ) -> Result, FunctionCallError> { let sandbox = turn .primary_environment() @@ -528,6 +531,7 @@ pub(crate) async fn intercept_apply_patch( turn: turn.clone(), call_id: call_id.to_string(), tool_name: tool_name.to_string(), + pre_tool_use_permission_decision, }; let out = orchestrator .run( diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 04472e4623a..c76d52086e3 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -39,6 +39,7 @@ async fn invocation_for_payload(payload: ToolPayload) -> ToolInvocation { tool_name: codex_tools::ToolName::plain("apply_patch"), source: crate::tools::context::ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, } } diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 568e4561583..2db5378a179 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -60,6 +60,7 @@ impl ToolHandler for McpHandler { call_id, tool_name: model_tool_name, payload, + pre_tool_use_permission_decision, .. } = invocation; @@ -88,6 +89,7 @@ impl ToolHandler for McpHandler { tool, model_tool_name.display(), arguments_str, + pre_tool_use_permission_decision, ) .await; @@ -145,6 +147,7 @@ mod tests { tool_name: codex_tools::ToolName::namespaced("mcp__memory__", "create_entities"), source: ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }), Some(PreToolUsePayload { tool_name: HookToolName::new("mcp__memory__create_entities"), @@ -194,6 +197,7 @@ mod tests { tool_name: codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"), source: ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }; assert_eq!( McpHandler.post_tool_use_payload(&invocation, &output), diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index 61dc77eb363..1db7b6a7aec 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -75,6 +75,7 @@ fn invocation( tool_name: codex_tools::ToolName::plain(tool_name), source: crate::tools::context::ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, } } diff --git a/codex-rs/core/src/tools/handlers/request_user_input_tests.rs b/codex-rs/core/src/tools/handlers/request_user_input_tests.rs index 7c577c54bef..0a14eddcf9e 100644 --- a/codex-rs/core/src/tools/handlers/request_user_input_tests.rs +++ b/codex-rs/core/src/tools/handlers/request_user_input_tests.rs @@ -53,6 +53,7 @@ async fn multi_agent_v2_request_user_input_rejects_subagent_threads() { }) .to_string(), }, + pre_tool_use_permission_decision: None, }) .await; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index fb80845bdd5..817a02350a5 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -87,6 +87,7 @@ struct RunExecLikeArgs { call_id: String, freeform: bool, shell_runtime_backend: ShellRuntimeBackend, + pre_tool_use_permission_decision: Option, } impl ShellHandler { @@ -236,6 +237,7 @@ impl ToolHandler for ShellHandler { call_id, tool_name, payload, + pre_tool_use_permission_decision, .. } = invocation; @@ -258,6 +260,7 @@ impl ToolHandler for ShellHandler { call_id, freeform: false, shell_runtime_backend: ShellRuntimeBackend::Generic, + pre_tool_use_permission_decision, }) .await } @@ -276,6 +279,7 @@ impl ToolHandler for ShellHandler { call_id, freeform: false, shell_runtime_backend: ShellRuntimeBackend::Generic, + pre_tool_use_permission_decision, }) .await } @@ -350,6 +354,7 @@ impl ToolHandler for ShellCommandHandler { call_id, tool_name, payload, + pre_tool_use_permission_decision, .. } = invocation; @@ -390,6 +395,7 @@ impl ToolHandler for ShellCommandHandler { call_id, freeform: true, shell_runtime_backend: self.shell_runtime_backend(), + pre_tool_use_permission_decision, }) .await } @@ -409,6 +415,7 @@ impl ShellHandler { call_id, freeform, shell_runtime_backend, + pre_tool_use_permission_decision, } = args; let mut exec_params = exec_params; @@ -492,6 +499,7 @@ impl ShellHandler { Some(&tracker), &call_id, tool_name.as_str(), + pre_tool_use_permission_decision.clone(), ) .await? { @@ -563,6 +571,7 @@ impl ShellHandler { turn: turn.clone(), call_id: call_id.clone(), tool_name, + pre_tool_use_permission_decision, }; let out = orchestrator .run( diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 49e2cf8f75d..a9046584583 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -233,6 +233,7 @@ async fn shell_pre_tool_use_payload_uses_joined_command() { tool_name: codex_tools::ToolName::plain("shell"), source: crate::tools::context::ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }), Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), @@ -261,6 +262,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { tool_name: codex_tools::ToolName::plain("shell_command"), source: crate::tools::context::ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }), Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), @@ -292,6 +294,7 @@ async fn build_post_tool_use_payload_uses_tool_output_wire_value() { tool_name: codex_tools::ToolName::plain("shell_command"), source: ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }; assert_eq!( handler.post_tool_use_payload(&invocation, &output), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 5aec8c8ba5a..cc959ba2f65 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -184,6 +184,7 @@ impl ToolHandler for UnifiedExecHandler { call_id, tool_name, payload, + pre_tool_use_permission_decision, .. } = invocation; @@ -204,7 +205,12 @@ impl ToolHandler for UnifiedExecHandler { let fs = turn_environment.environment.get_filesystem(); let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager; - let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone()); + let context = UnifiedExecContext::new( + session.clone(), + turn.clone(), + call_id.clone(), + pre_tool_use_permission_decision, + ); let response = match tool_name.name.as_str() { "exec_command" => { @@ -313,6 +319,7 @@ impl ToolHandler for UnifiedExecHandler { Some(&tracker), &context.call_id, &tool_name.name, + context.pre_tool_use_permission_decision.clone(), ) .await? { diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 1bdd0b82f97..375b7f89dd8 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -38,6 +38,7 @@ async fn invocation_for_payload( tool_name: codex_tools::ToolName::plain(tool_name), source: ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, } } @@ -236,6 +237,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { tool_name: codex_tools::ToolName::plain("exec_command"), source: crate::tools::context::ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }), Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), @@ -262,6 +264,7 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { tool_name: codex_tools::ToolName::plain("write_stdin"), source: crate::tools::context::ToolCallSource::Direct, payload, + pre_tool_use_permission_decision: None, }), None ); diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 54227006dc6..1db9b56b0e7 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; @@ -76,6 +77,7 @@ impl ToolOrchestrator { turn: tool_ctx.turn.clone(), call_id: tool_ctx.call_id.clone(), tool_name: tool_ctx.tool_name.clone(), + pre_tool_use_permission_decision: tool_ctx.pre_tool_use_permission_decision.clone(), }; let attempt_with_network_approval = SandboxAttempt { sandbox: attempt.sandbox, @@ -148,8 +150,83 @@ impl ToolOrchestrator { let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| { default_exec_approval_requirement(approval_policy, &file_system_sandbox_policy) }); - match requirement { - ExecApprovalRequirement::Skip { .. } => { + match (&tool_ctx.pre_tool_use_permission_decision, requirement) { + ( + Some(PreToolUsePermissionDecision::Allow { .. }), + ExecApprovalRequirement::Forbidden { reason }, + ) => { + return Err(ToolError::Rejected(reason)); + } + ( + Some(PreToolUsePermissionDecision::Allow { .. }), + ExecApprovalRequirement::Skip { .. } + | ExecApprovalRequirement::NeedsApproval { .. }, + ) => { + let decision = ReviewDecision::Approved; + otel.tool_decision(otel_tn, otel_ci, &decision, ToolDecisionSource::Config); + already_approved = true; + } + ( + Some(PreToolUsePermissionDecision::Ask { .. }), + ExecApprovalRequirement::Forbidden { + reason: forbidden_reason, + }, + ) => { + return Err(ToolError::Rejected(forbidden_reason)); + } + ( + Some(PreToolUsePermissionDecision::Ask { reason }), + ExecApprovalRequirement::Skip { .. }, + ) => { + let approval_ctx = ApprovalCtx { + session: &tool_ctx.session, + turn: &tool_ctx.turn, + call_id: &tool_ctx.call_id, + guardian_review_id: None, + retry_reason: reason.clone(), + 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*/ false, + &otel, + ) + .await?; + Self::reject_if_not_approved(tool_ctx, None, decision).await?; + already_approved = true; + } + ( + Some(PreToolUsePermissionDecision::Ask { + reason: hook_reason, + }), + ExecApprovalRequirement::NeedsApproval { reason, .. }, + ) => { + let approval_ctx = ApprovalCtx { + session: &tool_ctx.session, + turn: &tool_ctx.turn, + call_id: &tool_ctx.call_id, + guardian_review_id: None, + retry_reason: hook_reason.clone().or(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*/ false, + &otel, + ) + .await?; + Self::reject_if_not_approved(tool_ctx, None, decision).await?; + already_approved = true; + } + (None, ExecApprovalRequirement::Skip { .. }) => { if strict_auto_review { let guardian_review_id = Some(new_guardian_review_id()); let approval_ctx = ApprovalCtx { @@ -182,10 +259,10 @@ impl ToolOrchestrator { ); } } - ExecApprovalRequirement::Forbidden { reason } => { + (None, ExecApprovalRequirement::Forbidden { reason }) => { return Err(ToolError::Rejected(reason)); } - ExecApprovalRequirement::NeedsApproval { reason, .. } => { + (None, ExecApprovalRequirement::NeedsApproval { reason, .. }) => { let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, @@ -315,17 +392,30 @@ 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 + let bypass_retry_approval = matches!( + tool_ctx.pre_tool_use_permission_decision, + Some(PreToolUsePermissionDecision::Allow { .. }) + ) || (!strict_auto_review && tool.should_bypass_approval(approval_policy, already_approved) - && network_approval_context.is_none(); + && network_approval_context.is_none()); if !bypass_retry_approval { - let guardian_review_id = use_guardian.then(new_guardian_review_id); + let force_user_prompt = matches!( + tool_ctx.pre_tool_use_permission_decision, + Some(PreToolUsePermissionDecision::Ask { .. }) + ); + let guardian_review_id = + (!force_user_prompt && 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: tool_ctx + .pre_tool_use_permission_decision + .as_ref() + .and_then(PreToolUsePermissionDecision::reason) + .map(ToString::to_string) + .or(Some(retry_reason)), network_approval_context: network_approval_context.clone(), }; @@ -336,7 +426,8 @@ impl ToolOrchestrator { &permission_request_run_id, approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, + /*evaluate_permission_request_hooks*/ + !strict_auto_review && !force_user_prompt, &otel, ) .await?; diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index e1027c9fa90..c8344297ad4 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -266,6 +266,7 @@ impl ToolRegistry { &self, invocation: ToolInvocation, ) -> Result { + let mut invocation = invocation; let tool_name = invocation.tool_name.clone(); let display_name = tool_name.display(); let call_id_owned = invocation.call_id.clone(); @@ -354,8 +355,8 @@ impl ToolRegistry { return Err(err); } - if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) - && let Some(message) = run_pre_tool_use_hooks( + if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) { + match run_pre_tool_use_hooks( &invocation.session, &invocation.turn, invocation.call_id.clone(), @@ -363,10 +364,16 @@ impl ToolRegistry { &pre_tool_use_payload.tool_input, ) .await - { - let err = FunctionCallError::RespondToModel(message); - dispatch_trace.record_failed(&err); - return Err(err); + { + Ok(permission_decision) => { + invocation.pre_tool_use_permission_decision = permission_decision; + } + Err(message) => { + let err = FunctionCallError::RespondToModel(message); + dispatch_trace.record_failed(&err); + return Err(err); + } + } } let is_mutating = handler.is_mutating(&invocation).await; diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index aeba3b0556e..30930cf8ae9 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -291,6 +291,7 @@ impl ToolRouter { tool_name, source, payload, + pre_tool_use_permission_decision: None, }; self.registry.dispatch_any(invocation).await diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 2ef5c8f2b73..23036ecc127 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -12,6 +12,7 @@ use crate::session::turn_context::TurnContext; use crate::state::SessionServices; use crate::tools::hook_names::HookToolName; use crate::tools::network_approval::NetworkApprovalSpec; +use codex_hooks::PreToolUsePermissionDecision; use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::approvals::NetworkApprovalContext; @@ -350,6 +351,7 @@ pub(crate) struct ToolCtx { pub turn: Arc, pub call_id: String, pub tool_name: String, + pub pre_tool_use_permission_decision: Option, } #[derive(Debug)] diff --git a/codex-rs/core/src/tools/tool_dispatch_trace_tests.rs b/codex-rs/core/src/tools/tool_dispatch_trace_tests.rs index 5f11816553c..f277b70b38e 100644 --- a/codex-rs/core/src/tools/tool_dispatch_trace_tests.rs +++ b/codex-rs/core/src/tools/tool_dispatch_trace_tests.rs @@ -266,6 +266,7 @@ fn test_invocation_with_payload( tool_name, source, payload, + pre_tool_use_permission_decision: None, } } diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 97b37e8d80d..984301893a5 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -74,14 +74,21 @@ pub(crate) struct UnifiedExecContext { pub session: Arc, pub turn: Arc, pub call_id: String, + pub pre_tool_use_permission_decision: Option, } impl UnifiedExecContext { - pub fn new(session: Arc, turn: Arc, call_id: String) -> Self { + pub fn new( + session: Arc, + turn: Arc, + call_id: String, + pre_tool_use_permission_decision: Option, + ) -> Self { Self { session, turn, call_id, + pre_tool_use_permission_decision, } } } diff --git a/codex-rs/core/src/unified_exec/mod_tests.rs b/codex-rs/core/src/unified_exec/mod_tests.rs index 4420f11e3c1..71fb0d8228e 100644 --- a/codex-rs/core/src/unified_exec/mod_tests.rs +++ b/codex-rs/core/src/unified_exec/mod_tests.rs @@ -103,8 +103,12 @@ async fn exec_command_with_tty( ) .await?, ); - let context = - UnifiedExecContext::new(Arc::clone(session), Arc::clone(turn), "call".to_string()); + let context = UnifiedExecContext::new( + Arc::clone(session), + Arc::clone(turn), + "call".to_string(), + None, + ); let started_at = Instant::now(); let process_started_alive = !process.has_exited() && process.exit_code().is_none(); if process_started_alive { diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index c67abc48d6d..23aabc0eebf 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1043,6 +1043,7 @@ impl UnifiedExecProcessManager { turn: context.turn.clone(), call_id: context.call_id.clone(), tool_name: "exec_command".to_string(), + pre_tool_use_permission_decision: context.pre_tool_use_permission_decision.clone(), }; orchestrator .run( diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index 0c5b7141611..11726a158cd 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -164,6 +164,7 @@ async fn failed_initial_end_for_unstored_process_uses_fallback_output() { Arc::clone(&session), Arc::clone(&turn), "call-unified-denied".to_string(), + None, ); let request = ExecCommandRequest { command: vec![ diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 0a3a994e19d..415bebc6e0a 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -16,6 +16,7 @@ pub(crate) struct SessionStartOutput { pub(crate) struct PreToolUseOutput { pub universal: UniversalOutput, pub block_reason: Option, + pub permission_decision: Option, pub invalid_reason: Option, } @@ -59,6 +60,7 @@ pub(crate) struct StopOutput { pub invalid_block_reason: Option, } +use crate::events::pre_tool_use::PreToolUsePermissionDecision; use crate::schema::BlockDecisionWire; use crate::schema::HookUniversalOutputWire; use crate::schema::PermissionRequestBehaviorWire; @@ -123,10 +125,34 @@ 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(|output| 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) | None => None, + }) + } else { + None + }; Some(PreToolUseOutput { universal, block_reason, + permission_decision, invalid_reason, }) } @@ -348,12 +374,8 @@ fn unsupported_pre_tool_use_hook_specific_output( Some("PreToolUse hook returned unsupported additionalContext".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 diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 6fe1555229c..a09a42d086a 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -18,6 +18,20 @@ use crate::engine::dispatcher; use crate::engine::output_parser; use crate::schema::PreToolUseCommandInput; +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PreToolUsePermissionDecision { + Allow { reason: Option }, + Ask { reason: Option }, +} + +impl PreToolUsePermissionDecision { + pub fn reason(&self) -> Option<&str> { + match self { + Self::Allow { reason } | Self::Ask { reason } => reason.as_deref(), + } + } +} + #[derive(Debug, Clone)] pub struct PreToolUseRequest { pub session_id: ThreadId, @@ -37,12 +51,14 @@ pub struct PreToolUseOutcome { pub hook_events: Vec, pub should_block: bool, pub block_reason: Option, + pub permission_decision: Option, } #[derive(Debug, Default, PartialEq, Eq)] struct PreToolUseHandlerData { should_block: bool, block_reason: Option, + permission_decision: Option, } pub(crate) fn preview( @@ -78,6 +94,7 @@ pub(crate) async fn run( hook_events: Vec::new(), should_block: false, block_reason: None, + permission_decision: None, }; } @@ -108,6 +125,17 @@ pub(crate) async fn run( let block_reason = results .iter() .find_map(|result| result.data.block_reason.clone()); + let permission_decision = results + .iter() + .find_map(|result| match &result.data.permission_decision { + Some(decision @ PreToolUsePermissionDecision::Ask { .. }) => Some(decision.clone()), + _ => None, + }) + .or_else(|| { + results + .iter() + .find_map(|result| result.data.permission_decision.clone()) + }); PreToolUseOutcome { hook_events: results @@ -118,6 +146,7 @@ pub(crate) async fn run( .collect(), should_block, block_reason, + permission_decision, } } @@ -151,6 +180,7 @@ fn parse_completed( let mut status = HookRunStatus::Completed; let mut should_block = false; let mut block_reason = None; + let mut permission_decision = None; match run_result.error.as_deref() { Some(error) => { @@ -185,6 +215,8 @@ fn parse_completed( kind: HookOutputEntryKind::Feedback, text: reason, }); + } else { + permission_decision = parsed.permission_decision; } } else if trimmed_stdout.starts_with('{') || trimmed_stdout.starts_with('[') { status = HookRunStatus::Failed; @@ -238,6 +270,7 @@ fn parse_completed( data: PreToolUseHandlerData { should_block, block_reason, + permission_decision, }, } } @@ -247,6 +280,7 @@ fn serialization_failure_outcome(hook_events: Vec) -> PreToo hook_events, should_block: false, block_reason: None, + permission_decision: None, } } @@ -298,6 +332,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); @@ -327,6 +362,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); @@ -340,12 +376,12 @@ mod tests { } #[test] - fn unsupported_permission_decision_fails_open() { + fn permission_decision_allow_flows_through() { let parsed = parse_completed( &handler(), run_result( Some(0), - r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"please confirm"}}"#, + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","permissionDecisionReason":"already reviewed"}}"#, "", ), Some("turn-1".to_string()), @@ -356,16 +392,39 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: Some(super::PreToolUsePermissionDecision::Allow { + reason: Some("already reviewed".to_string()), + }), } ); - assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!(parsed.completed.run.entries, vec![]); + } + + #[test] + fn permission_decision_ask_flows_through() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"please confirm"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + assert_eq!( - parsed.completed.run.entries, - vec![HookOutputEntry { - kind: HookOutputEntryKind::Error, - text: "PreToolUse hook returned unsupported permissionDecision:ask".to_string(), - }] + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + permission_decision: Some(super::PreToolUsePermissionDecision::Ask { + reason: Some("please confirm".to_string()), + }), + } ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!(parsed.completed.run.entries, vec![]); } #[test] @@ -381,6 +440,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -410,6 +470,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -435,6 +496,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); @@ -454,6 +516,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -479,6 +542,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("blocked by policy".to_string()), + permission_decision: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 4e16969a587..2d2ff9d589b 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;