diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index c7e665cb02ab..c75ba04d4192 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -215,6 +215,7 @@ pub(crate) async fn handle_mcp_tool_call( ) .await; + let mut invocation = invocation; if let Some(decision) = maybe_request_mcp_tool_approval( &sess, turn_context, @@ -223,9 +224,28 @@ pub(crate) async fn handle_mcp_tool_call( &hook_tool_name, metadata.as_ref(), approval_mode, + /*evaluate_permission_request_hooks*/ true, ) .await { + let decision = match decision { + McpToolApprovalDecision::AcceptWithUpdatedInput(updated_input) => { + invocation.arguments = Some(updated_input); + maybe_request_mcp_tool_approval( + &sess, + turn_context, + &call_id, + &invocation, + &hook_tool_name, + metadata.as_ref(), + approval_mode, + /*evaluate_permission_request_hooks*/ false, + ) + .await + .unwrap_or(McpToolApprovalDecision::Accept) + } + decision => decision, + }; let result = match decision { McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession @@ -279,6 +299,9 @@ pub(crate) async fn handle_mcp_tool_call( ) .await } + McpToolApprovalDecision::AcceptWithUpdatedInput(_) => { + unreachable!("updated MCP tool input should be re-approved before dispatch") + } }; let status = if result.is_ok() { "ok" } else { "error" }; @@ -951,6 +974,7 @@ async fn maybe_track_codex_app_used( #[derive(Debug, Clone, PartialEq, Eq)] enum McpToolApprovalDecision { Accept, + AcceptWithUpdatedInput(JsonValue), AcceptForSession, AcceptAndRemember, Decline { message: Option }, @@ -1139,6 +1163,7 @@ fn mcp_tool_approval_prompt_options( } } +#[allow(clippy::too_many_arguments)] async fn maybe_request_mcp_tool_approval( sess: &Arc, turn_context: &Arc, @@ -1147,6 +1172,7 @@ async fn maybe_request_mcp_tool_approval( hook_tool_name: &str, metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, + evaluate_permission_request_hooks: bool, ) -> Option { if mcp_permission_prompt_is_auto_approved( turn_context.approval_policy.value(), @@ -1199,29 +1225,40 @@ async fn maybe_request_mcp_tool_approval( 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); - } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); + if evaluate_permission_request_hooks { + 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 { + updated_input: Some(updated_input), + }) => { + return Some(McpToolApprovalDecision::AcceptWithUpdatedInput( + updated_input, + )); + } + Some(PermissionRequestDecision::Allow { + updated_input: None, + }) => { + 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 @@ -1967,6 +2004,7 @@ async fn apply_mcp_tool_approval_decision( } } McpToolApprovalDecision::Accept + | McpToolApprovalDecision::AcceptWithUpdatedInput(_) | McpToolApprovalDecision::Decline { .. } | McpToolApprovalDecision::Cancel | McpToolApprovalDecision::BlockedBySafetyMonitor(_) => {} diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index a556e228ac95..ee36b32ff00e 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2189,6 +2189,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2262,6 +2263,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { "mcp__test__tool", Some(&metadata), AppToolApproval::Auto, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2318,6 +2320,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { "mcp__memory__create_entities", Some(&metadata), AppToolApproval::Auto, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2378,6 +2381,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { "mcp__memory__create_entities", /*metadata*/ None, AppToolApproval::Auto, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2455,6 +2459,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { "mcp__memory__create_entities", Some(&metadata), AppToolApproval::Auto, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2535,6 +2540,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { "mcp__test__tool", Some(&metadata), AppToolApproval::Auto, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2592,6 +2598,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval "mcp__test__tool", Some(&metadata), AppToolApproval::Prompt, + /*evaluate_permission_request_hooks*/ true, ) .await }) @@ -2667,6 +2674,7 @@ async fn approve_mode_skips_arc_interrupt_for_model() { "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2734,6 +2742,7 @@ async fn custom_approve_mode_skips_arc_interrupt_for_model() { "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2801,6 +2810,7 @@ async fn approve_mode_skips_arc_interrupt_without_annotations() { "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2878,6 +2888,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { "mcp__test__tool", Some(&metadata), approval_mode, + /*evaluate_permission_request_hooks*/ true, ) .await; @@ -2981,6 +2992,7 @@ async fn approve_mode_skips_arc_and_guardian_in_every_permission_mode() { "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, + /*evaluate_permission_request_hooks*/ true, ) .await; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index cae3e1f97810..cc82b0c6ed63 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1031,7 +1031,7 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an orchestrator .run( &mut tool, - &(), + (), &tool_ctx, turn.as_ref(), AskForApproval::Never, diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 45ade8bbe0ff..eb77c9b6cbab 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -422,7 +422,7 @@ impl ToolHandler for ApplyPatchHandler { let out = orchestrator .run( &mut runtime, - &req, + req, &tool_ctx, turn.as_ref(), turn.approval_policy.value(), @@ -530,7 +530,7 @@ pub(crate) async fn intercept_apply_patch( let out = orchestrator .run( &mut runtime, - &req, + req, &tool_ctx, turn.as_ref(), turn.approval_policy.value(), diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index e33c8b2ee9db..fca928ab7622 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -229,7 +229,7 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result Result Result { + PermissionRequestDecision::Allow { + updated_input: None, + } => { pending .set_decision(PendingApprovalDecision::AllowOnce) .await; @@ -481,6 +483,22 @@ impl NetworkApprovalService { pending_approvals.remove(&key); return NetworkDecision::Allow; } + PermissionRequestDecision::Allow { + updated_input: Some(_), + } => { + let message = "updatedInput is not supported for network approvals".to_string(); + if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::DeniedByPolicy(message), + ) + .await; + } + pending.set_decision(PendingApprovalDecision::Deny).await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + return NetworkDecision::deny(REASON_NOT_ALLOWED); + } PermissionRequestDecision::Deny { message } => { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index c618d778d6ee..8de08353534b 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -46,6 +46,11 @@ pub(crate) struct OrchestratorRunResult { pub deferred_network_approval: Option, } +enum ApprovalResult { + Decision(ReviewDecision), + UpdatedInput(serde_json::Value), +} + impl ToolOrchestrator { pub fn new() -> Self { Self { @@ -126,7 +131,7 @@ impl ToolOrchestrator { pub async fn run( &mut self, tool: &mut T, - req: &Rq, + mut req: Rq, tool_ctx: &ToolCtx, turn_ctx: &crate::session::turn_context::TurnContext, approval_policy: AskForApproval, @@ -142,79 +147,109 @@ impl ToolOrchestrator { // 1) Approval let mut already_approved = false; + let mut permission_request_hooks_available = !strict_auto_review; let file_system_sandbox_policy = turn_ctx.file_system_sandbox_policy(); let network_sandbox_policy = turn_ctx.network_sandbox_policy(); - let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| { - default_exec_approval_requirement(approval_policy, &file_system_sandbox_policy) - }); - match requirement { - ExecApprovalRequirement::Skip { .. } => { - if strict_auto_review { - let guardian_review_id = Some(new_guardian_review_id()); + loop { + let requirement = tool.exec_approval_requirement(&req).unwrap_or_else(|| { + default_exec_approval_requirement(approval_policy, &file_system_sandbox_policy) + }); + match requirement { + ExecApprovalRequirement::Skip { .. } => { + if strict_auto_review { + let guardian_review_id = Some(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, + network_approval_context: None, + }; + let ApprovalResult::Decision(decision) = Self::request_approval( + tool, + &req, + tool_ctx.call_id.as_str(), + approval_ctx, + tool_ctx, + /*evaluate_permission_request_hooks*/ false, + &otel, + ) + .await? + else { + unreachable!("permission hooks are disabled under strict auto review"); + }; + 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, + ); + } + break; + } + 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: None, + retry_reason: reason, network_approval_context: None, }; - let decision = Self::request_approval( + match Self::request_approval( tool, - req, + &req, tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ false, + permission_request_hooks_available, &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, - ); + .await? + { + ApprovalResult::Decision(decision) => { + Self::reject_if_not_approved( + tool_ctx, + guardian_review_id.as_deref(), + decision, + ) + .await?; + already_approved = true; + break; + } + ApprovalResult::UpdatedInput(updated_input) => { + req = tool + .with_updated_permission_request_input( + &req, + updated_input, + tool_ctx, + approval_policy, + ) + .await?; + permission_request_hooks_available = false; + } + } } } - 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) - .await?; - already_approved = true; - } } // 2) First attempt under the selected sandbox. let managed_network_active = turn_ctx.network.is_some(); - let initial_sandbox = match tool.sandbox_mode_for_first_attempt(req) { + let initial_sandbox = match tool.sandbox_mode_for_first_attempt(&req) { SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None, SandboxOverride::NoOverride => self.sandbox.select_initial( &file_system_sandbox_policy, @@ -227,7 +262,7 @@ impl ToolOrchestrator { // Platform-specific flag gating is handled by SandboxManager::select_initial. let use_legacy_landlock = turn_ctx.features.use_legacy_landlock(); - let sandbox_cwd = tool.sandbox_cwd(req).unwrap_or(&turn_ctx.cwd); + let sandbox_cwd = tool.sandbox_cwd(&req).unwrap_or(&turn_ctx.cwd); let initial_attempt = SandboxAttempt { sandbox: initial_sandbox, permissions: &turn_ctx.permission_profile, @@ -246,7 +281,7 @@ impl ToolOrchestrator { let (first_result, first_deferred_network_approval) = Self::run_attempt( tool, - req, + &req, tool_ctx, &initial_attempt, managed_network_active, @@ -333,15 +368,20 @@ impl ToolOrchestrator { let permission_request_run_id = format!("{}:retry", tool_ctx.call_id); let decision = Self::request_approval( tool, - req, + &req, &permission_request_run_id, approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, + permission_request_hooks_available, &otel, ) .await?; + let ApprovalResult::Decision(decision) = decision else { + return Err(ToolError::Rejected( + "updatedInput is not supported during retry approval".to_string(), + )); + }; Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) .await?; } @@ -365,7 +405,7 @@ impl ToolOrchestrator { // Second attempt. let (retry_result, retry_deferred_network_approval) = Self::run_attempt( tool, - req, + &req, tool_ctx, &escalated_attempt, managed_network_active, @@ -391,7 +431,7 @@ impl ToolOrchestrator { tool_ctx: &ToolCtx, evaluate_permission_request_hooks: bool, otel: &codex_otel::SessionTelemetry, - ) -> Result + ) -> Result where T: ToolRuntime, { @@ -406,7 +446,14 @@ impl ToolOrchestrator { ) .await { - Some(PermissionRequestDecision::Allow) => { + Some(PermissionRequestDecision::Allow { + updated_input: Some(updated_input), + }) => { + return Ok(ApprovalResult::UpdatedInput(updated_input)); + } + Some(PermissionRequestDecision::Allow { + updated_input: None, + }) => { let decision = ReviewDecision::Approved; otel.tool_decision( &tool_ctx.tool_name, @@ -414,7 +461,7 @@ impl ToolOrchestrator { &decision, ToolDecisionSource::Config, ); - return Ok(decision); + return Ok(ApprovalResult::Decision(decision)); } Some(PermissionRequestDecision::Deny { message }) => { let decision = ReviewDecision::Denied; @@ -442,7 +489,7 @@ impl ToolOrchestrator { &decision, otel_source, ); - Ok(decision) + Ok(ApprovalResult::Decision(decision)) } async fn reject_if_not_approved( diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 7f17285db9d9..d6238de372bf 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -38,11 +38,13 @@ use crate::tools::sandboxing::with_cached_approval; use codex_network_proxy::NetworkProxy; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxablePreference; use codex_shell_command::powershell::prefix_powershell_script_with_utf8; use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; +use serde_json::Value; use std::collections::HashMap; #[derive(Clone, Debug)] @@ -59,6 +61,7 @@ pub struct ShellRequest { #[cfg(unix)] pub additional_permissions_preapproved: bool, pub justification: Option, + pub prefix_rule: Option>, pub exec_approval_requirement: ExecApprovalRequirement, } @@ -215,6 +218,75 @@ impl Approvable for ShellRuntime { } impl ToolRuntime for ShellRuntime { + fn with_updated_permission_request_input<'a>( + &'a self, + req: &'a ShellRequest, + updated_input: Value, + ctx: &'a ToolCtx, + approval_policy: AskForApproval, + ) -> BoxFuture<'a, Result> { + Box::pin(async move { + let command = updated_input + .get("command") + .and_then(Value::as_str) + .ok_or_else(|| { + ToolError::Rejected( + "PermissionRequest updatedInput must include string field `command`" + .to_string(), + ) + })?; + let mut updated = req.clone(); + updated.hook_command = command.to_string(); + updated.justification = match updated_input.get("description") { + Some(Value::String(description)) => Some(description.clone()), + Some(_) => { + return Err(ToolError::Rejected( + "PermissionRequest updatedInput field `description` must be a string" + .to_string(), + )); + } + None => None, + }; + updated.command = match self.backend { + ShellRuntimeBackend::Generic => shlex::split(command).ok_or_else(|| { + ToolError::Rejected( + "PermissionRequest updatedInput contained an invalid shell command" + .to_string(), + ) + })?, + ShellRuntimeBackend::ShellCommandClassic + | ShellRuntimeBackend::ShellCommandZshFork => { + let mut command_argv = updated.command; + let Some(script) = command_argv.last_mut() else { + return Err(ToolError::Rejected( + "shell command args are empty".to_string(), + )); + }; + *script = command.to_string(); + command_argv + } + }; + let file_system_sandbox_policy = ctx.turn.file_system_sandbox_policy(); + updated.exec_approval_requirement = ctx + .session + .services + .exec_policy + .create_exec_approval_requirement_for_command( + crate::exec_policy::ExecApprovalRequest { + command: &updated.command, + approval_policy, + permission_profile: ctx.turn.permission_profile(), + file_system_sandbox_policy: &file_system_sandbox_policy, + sandbox_cwd: updated.cwd.as_path(), + sandbox_permissions: updated_exec_policy_sandbox_permissions(&updated), + prefix_rule: updated.prefix_rule.clone(), + }, + ) + .await; + Ok(updated) + }) + } + fn network_approval_spec( &self, req: &ShellRequest, @@ -292,3 +364,12 @@ impl ToolRuntime for ShellRuntime { Ok(out) } } + +fn updated_exec_policy_sandbox_permissions(req: &ShellRequest) -> SandboxPermissions { + #[cfg(unix)] + if req.additional_permissions_preapproved { + return SandboxPermissions::UseDefault; + } + + req.sandbox_permissions +} 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 fef8db5ca9e5..ac604b720dcd 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -420,13 +420,27 @@ impl CoreShellActionProvider { ) .await { - Some(PermissionRequestDecision::Allow) => { + Some(PermissionRequestDecision::Allow { + updated_input: None, + }) => { return PromptDecision { decision: ReviewDecision::Approved, guardian_review_id: None, rejection_message: None, }; } + Some(PermissionRequestDecision::Allow { + updated_input: Some(_), + }) => { + return PromptDecision { + decision: ReviewDecision::Denied, + guardian_review_id: None, + rejection_message: Some( + "updatedInput is not supported for intercepted exec approvals" + .to_string(), + ), + }; + } Some(PermissionRequestDecision::Deny { message }) => { return PromptDecision { decision: ReviewDecision::Denied, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 42f311bfcb68..2504e4cadd65 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -42,12 +42,14 @@ use codex_network_proxy::NetworkProxy; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxablePreference; use codex_shell_command::powershell::prefix_powershell_script_with_utf8; use codex_tools::UnifiedExecShellMode; use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; +use serde_json::Value; use std::collections::HashMap; use std::sync::Arc; use tokio_util::sync::CancellationToken; @@ -71,6 +73,7 @@ pub struct UnifiedExecRequest { #[cfg(unix)] pub additional_permissions_preapproved: bool, pub justification: Option, + pub prefix_rule: Option>, pub exec_approval_requirement: ExecApprovalRequirement, } @@ -217,6 +220,62 @@ impl Approvable for UnifiedExecRuntime<'_> { } impl<'a> ToolRuntime for UnifiedExecRuntime<'a> { + fn with_updated_permission_request_input<'b>( + &'b self, + req: &'b UnifiedExecRequest, + updated_input: Value, + ctx: &'b ToolCtx, + approval_policy: AskForApproval, + ) -> BoxFuture<'b, Result> { + Box::pin(async move { + let command = updated_input + .get("command") + .and_then(Value::as_str) + .ok_or_else(|| { + ToolError::Rejected( + "PermissionRequest updatedInput must include string field `command`" + .to_string(), + ) + })?; + let mut updated = req.clone(); + updated.hook_command = command.to_string(); + updated.justification = match updated_input.get("description") { + Some(Value::String(description)) => Some(description.clone()), + Some(_) => { + return Err(ToolError::Rejected( + "PermissionRequest updatedInput field `description` must be a string" + .to_string(), + )); + } + None => None, + }; + let Some(script) = updated.command.last_mut() else { + return Err(ToolError::Rejected( + "unified exec command args are empty".to_string(), + )); + }; + *script = command.to_string(); + let file_system_sandbox_policy = ctx.turn.file_system_sandbox_policy(); + updated.exec_approval_requirement = ctx + .session + .services + .exec_policy + .create_exec_approval_requirement_for_command( + crate::exec_policy::ExecApprovalRequest { + command: &updated.command, + approval_policy, + permission_profile: ctx.turn.permission_profile(), + file_system_sandbox_policy: &file_system_sandbox_policy, + sandbox_cwd: updated.cwd.as_path(), + sandbox_permissions: updated_exec_policy_sandbox_permissions(&updated), + prefix_rule: updated.prefix_rule.clone(), + }, + ) + .await; + Ok(updated) + }) + } + fn sandbox_cwd<'b>(&self, req: &'b UnifiedExecRequest) -> Option<&'b AbsolutePathBuf> { Some(&req.cwd) } @@ -358,6 +417,15 @@ impl<'a> ToolRuntime for UnifiedExecRunt } } +fn updated_exec_policy_sandbox_permissions(req: &UnifiedExecRequest) -> SandboxPermissions { + #[cfg(unix)] + if req.additional_permissions_preapproved { + return SandboxPermissions::UseDefault; + } + + req.sandbox_permissions +} + #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index c17247beb47c..d735e29ed2e3 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -31,6 +31,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use futures::Future; use futures::future::BoxFuture; use serde::Serialize; +use serde_json::Value; use std::collections::HashMap; use std::fmt::Debug; use std::hash::Hash; @@ -362,6 +363,26 @@ pub(crate) trait ToolRuntime: Approvable + Sandboxable { None } + /// Rebuilds a typed request after a `PermissionRequest` hook replaces the + /// full hook-facing input object. + /// + /// Implementations should only support this when they can faithfully + /// reconstruct the request that will later be evaluated by the normal + /// policy / guardian / user approval path. + fn with_updated_permission_request_input<'a>( + &'a self, + _req: &'a Req, + _updated_input: Value, + _ctx: &'a ToolCtx, + _approval_policy: AskForApproval, + ) -> BoxFuture<'a, Result> { + Box::pin(async { + Err(ToolError::Rejected( + "updatedInput is not supported for this PermissionRequest target".to_string(), + )) + }) + } + async fn run( &mut self, req: &Req, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 4f85b1ddf7c7..9ec46b2e24ed 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1034,6 +1034,7 @@ impl UnifiedExecProcessManager { #[cfg(unix)] additional_permissions_preapproved: request.additional_permissions_preapproved, justification: request.justification.clone(), + prefix_rule: request.prefix_rule.clone(), exec_approval_requirement, }; let tool_ctx = ToolCtx { @@ -1045,7 +1046,7 @@ impl UnifiedExecProcessManager { orchestrator .run( &mut runtime, - &req, + req, &tool_ctx, &context.turn, context.turn.approval_policy.value(), diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index f587fb5f23e4..9d6c7e6b7afd 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -456,6 +456,16 @@ if mode == "allow": "decision": {{"behavior": "allow"}} }} }})) +elif mode == "allow_update": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PermissionRequest", + "decision": {{ + "behavior": "allow", + "updatedInput": {{"command": reason}} + }} + }} + }})) elif mode == "deny": print(json.dumps({{ "hookSpecificOutput": {{ @@ -1554,6 +1564,82 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test] +async fn permission_request_hook_rewrites_shell_command_before_normal_approval() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "permissionrequest-shell-command-rewrite"; + let original_marker = std::env::temp_dir().join("permissionrequest-original-marker"); + let original_command = format!("rm -f {}", original_marker.display()); + let rewritten_command = "echo rewritten-by-permission-request".to_string(); + let args = serde_json::json!({ "command": original_command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::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", "permission request hook rewrote it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let rewritten_command_for_hook = rewritten_command.clone(); + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_permission_request_hook( + home, + Some(PERMISSION_REQUEST_HOOK_MATCHER), + "allow_update", + &rewritten_command_for_hook, + ) { + panic!("failed to write permission request hook test fixture: {error}"); + } + }) + .with_config(trust_discovered_hooks); + let test = builder.build(&server).await?; + + fs::write(&original_marker, "seed").context("create original permission request marker")?; + test.submit_turn_with_approval_and_permission_profile( + "run the shell command after hook rewrite", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("shell command output string"); + assert!( + original_marker.exists(), + "original command should not execute after hook rewrite" + ); + assert!(output.contains("rewritten-by-permission-request")); + + assert_single_permission_request_hook_input( + test.codex_home_path(), + &original_command, + /*description*/ None, + )?; + + 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/permission-request.command.output.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json index 21d45382b0e5..be6d25e30c3b 100644 --- a/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json +++ b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json @@ -37,7 +37,7 @@ }, "updatedInput": { "default": null, - "description": "Reserved for a future input-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present." + "description": "Replacement input for the pending tool call when `behavior` is `allow`.\n\nThis replaces the full input object rather than merging into it." }, "updatedPermissions": { "default": null, diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 9c3d442dacf4..141e7b88387a 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -23,8 +23,12 @@ pub(crate) struct PreToolUseOutput { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PermissionRequestDecision { - Allow, - Deny { message: String }, + Allow { + updated_input: Option, + }, + Deny { + message: String, + }, } #[derive(Debug, Clone)] @@ -315,8 +319,10 @@ fn unsupported_permission_request_hook_specific_output( decision: Option<&PermissionRequestDecisionWire>, ) -> Option { let decision = decision?; - if decision.updated_input.is_some() { - Some("PermissionRequest hook returned unsupported updatedInput".to_string()) + if decision.updated_input.is_some() + && !matches!(decision.behavior, PermissionRequestBehaviorWire::Allow) + { + Some("PermissionRequest hook returned updatedInput without behavior:allow".to_string()) } else if decision.updated_permissions.is_some() { Some("PermissionRequest hook returned unsupported updatedPermissions".to_string()) } else if decision.interrupt { @@ -330,7 +336,9 @@ fn permission_request_decision( decision: &PermissionRequestDecisionWire, ) -> PermissionRequestDecision { match decision.behavior { - PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow, + PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow { + updated_input: decision.updated_input.clone(), + }, PermissionRequestBehaviorWire::Deny => PermissionRequestDecision::Deny { message: decision .message @@ -437,7 +445,7 @@ mod tests { use super::parse_permission_request; #[test] - fn permission_request_rejects_reserved_updated_input_field() { + fn permission_request_accepts_updated_input_for_allow() { let parsed = parse_permission_request( &json!({ "continue": true, @@ -453,9 +461,36 @@ mod tests { ) .expect("permission request hook output should parse"); + assert_eq!( + parsed.decision, + Some(super::PermissionRequestDecision::Allow { + updated_input: Some(json!({})), + }) + ); + assert_eq!(parsed.invalid_reason, None); + } + + #[test] + fn permission_request_rejects_updated_input_for_deny() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "deny", + "message": "blocked", + "updatedInput": {} + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + assert_eq!( parsed.invalid_reason, - Some("PermissionRequest hook returned unsupported updatedInput".to_string()) + Some("PermissionRequest hook returned updatedInput without behavior:allow".to_string()) ); } diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 2cebd0e002ed..b94bd89648e5 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -1,9 +1,9 @@ //! Permission-request hook execution. //! //! This event runs in the approval path, before guardian or user approval UI is -//! shown. Unlike `pre_tool_use`, handlers do not rewrite tool input or block by -//! stopping execution outright; instead they can return a concrete allow/deny -//! decision, or decline to decide and let the normal approval flow continue. +//! shown. Handlers can return a concrete allow/deny decision, optionally pair an +//! allow decision with a one-shot input rewrite, or decline to decide and let +//! the normal approval flow continue. //! //! The event also mirrors the rest of the hook system's lifecycle: //! @@ -47,7 +47,7 @@ pub struct PermissionRequestRequest { #[derive(Debug, Clone, PartialEq, Eq)] pub enum PermissionRequestDecision { - Allow, + Allow { updated_input: Option }, Deny { message: String }, } @@ -154,8 +154,10 @@ fn resolve_permission_request_decision<'a>( let mut resolved_allow = None; for decision in decisions { match decision { - PermissionRequestDecision::Allow => { - resolved_allow = Some(PermissionRequestDecision::Allow); + PermissionRequestDecision::Allow { updated_input } => { + resolved_allow = Some(PermissionRequestDecision::Allow { + updated_input: updated_input.clone(), + }); } PermissionRequestDecision::Deny { message } => { return Some(PermissionRequestDecision::Deny { @@ -219,8 +221,8 @@ fn parse_completed( }); } else if let Some(parsed_decision) = parsed.decision { match parsed_decision { - output_parser::PermissionRequestDecision::Allow => { - decision = Some(PermissionRequestDecision::Allow); + output_parser::PermissionRequestDecision::Allow { updated_input } => { + decision = Some(PermissionRequestDecision::Allow { updated_input }); } output_parser::PermissionRequestDecision::Deny { message } => { status = HookRunStatus::Blocked; @@ -294,7 +296,9 @@ mod tests { #[test] fn permission_request_deny_overrides_earlier_allow() { let decisions = [ - PermissionRequestDecision::Allow, + PermissionRequestDecision::Allow { + updated_input: None, + }, PermissionRequestDecision::Deny { message: "repo deny".to_string(), }, @@ -311,13 +315,19 @@ mod tests { #[test] fn permission_request_returns_allow_when_no_handler_denies() { let decisions = [ - PermissionRequestDecision::Allow, - PermissionRequestDecision::Allow, + PermissionRequestDecision::Allow { + updated_input: None, + }, + PermissionRequestDecision::Allow { + updated_input: Some(serde_json::json!({"command": "echo rewritten"})), + }, ]; assert_eq!( resolve_permission_request_decision(decisions.iter()), - Some(PermissionRequestDecision::Allow) + Some(PermissionRequestDecision::Allow { + updated_input: Some(serde_json::json!({"command": "echo rewritten"})), + }) ); } diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index d08cce6ee293..b7ad446d3dda 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -138,9 +138,9 @@ pub(crate) struct PermissionRequestHookSpecificOutputWire { #[serde(deny_unknown_fields)] pub(crate) struct PermissionRequestDecisionWire { pub behavior: PermissionRequestBehaviorWire, - /// Reserved for a future input-rewrite capability. + /// Replacement input for the pending tool call when `behavior` is `allow`. /// - /// PermissionRequest hooks currently fail closed if this field is present. + /// This replaces the full input object rather than merging into it. #[serde(default)] pub updated_input: Option, /// Reserved for a future permission-rewrite capability.