From eba705f4488ff0613187dbdaa54fcfde27baf178 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 14 Apr 2026 17:22:07 -0700 Subject: [PATCH] Support decision.updatedInput in PermissionRequest hook output --- codex-rs/core/src/hook_runtime.rs | 6 +- codex-rs/core/src/tools/handlers/shell.rs | 9 ++ .../core/src/tools/handlers/shell_tests.rs | 66 +++++++++++++ .../core/src/tools/handlers/unified_exec.rs | 32 +++++-- .../src/tools/handlers/unified_exec_tests.rs | 82 ++++++++++++++++ codex-rs/core/src/tools/orchestrator.rs | 73 +++++++++----- codex-rs/core/src/tools/runtimes/shell.rs | 42 +++++++++ .../core/src/tools/runtimes/unified_exec.rs | 51 ++++++++++ codex-rs/core/src/tools/sandboxing.rs | 10 ++ codex-rs/core/src/unified_exec/mod.rs | 2 + .../core/src/unified_exec/process_manager.rs | 2 + codex-rs/core/tests/suite/hooks.rs | 94 +++++++++++++++++++ ...mission-request.command.output.schema.json | 23 ++++- codex-rs/hooks/src/engine/output_parser.rs | 53 +++++++++-- .../hooks/src/events/permission_request.rs | 69 +++++++++++++- codex-rs/hooks/src/lib.rs | 1 + codex-rs/hooks/src/schema.rs | 15 +-- 17 files changed, 572 insertions(+), 58 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 0330dbd797..70924cb38e 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -165,8 +165,9 @@ pub(crate) async fn run_pre_tool_use_hooks( } // PermissionRequest hooks share the same preview/start/completed event flow as -// other hook types, but they return an optional decision instead of mutating -// tool input or post-run state. +// other hook types, but they return an optional approval decision plus any +// selected permission updates, and allow decisions may rewrite the hook-visible +// command input before execution. pub(crate) async fn run_permission_request_hooks( sess: &Arc, turn_context: &Arc, @@ -197,6 +198,7 @@ pub(crate) async fn run_permission_request_hooks( if let Some(PermissionRequestDecision::Allow { updated_permissions, + updated_input: _, }) = &decision { apply_permission_updates_from_hook(sess, turn_context, updated_permissions).await; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 2ba68a554c..b5445ab3ec 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -31,6 +31,7 @@ use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRequest; +use crate::tools::runtimes::shell::ShellRequestCommandInputKind; use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::runtimes::shell::ShellRuntimeBackend; use crate::tools::sandboxing::ToolCtx; @@ -79,6 +80,7 @@ struct RunExecLikeArgs { tool_name: String, exec_params: ExecParams, hook_command: String, + command_input_kind: ShellRequestCommandInputKind, additional_permissions: Option, prefix_rule: Option>, session: Arc, @@ -244,6 +246,7 @@ impl ToolHandler for ShellHandler { tool_name: tool_name.display(), exec_params, hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), + command_input_kind: ShellRequestCommandInputKind::Argv, additional_permissions: params.additional_permissions.clone(), prefix_rule, session, @@ -262,6 +265,7 @@ impl ToolHandler for ShellHandler { tool_name: tool_name.display(), exec_params, hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), + command_input_kind: ShellRequestCommandInputKind::Argv, additional_permissions: None, prefix_rule: None, session, @@ -351,6 +355,8 @@ impl ToolHandler for ShellCommandHandler { let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; + let use_login_shell = + Self::resolve_use_login_shell(params.login, turn.tools_config.allow_login_shell)?; let workdir = turn.resolve_path(params.workdir.clone()); maybe_emit_implicit_skill_invocation( session.as_ref(), @@ -371,6 +377,7 @@ impl ToolHandler for ShellCommandHandler { tool_name: tool_name.display(), exec_params, hook_command: params.command, + command_input_kind: ShellRequestCommandInputKind::ShellString { use_login_shell }, additional_permissions: params.additional_permissions.clone(), prefix_rule, session, @@ -390,6 +397,7 @@ impl ShellHandler { tool_name, exec_params, hook_command, + command_input_kind, additional_permissions, prefix_rule, session, @@ -530,6 +538,7 @@ impl ShellHandler { let req = ShellRequest { command: exec_params.command.clone(), + command_input_kind, hook_command, cwd: exec_params.cwd.clone(), timeout_ms: exec_params.expiration.timeout_ms(), diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 5c92a988ce..d87ced96b8 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -16,11 +16,19 @@ use crate::tools::context::ToolPayload; use crate::tools::handlers::ShellCommandHandler; use crate::tools::handlers::ShellHandler; use crate::tools::registry::ToolHandler; +use crate::tools::runtimes::shell::ShellRequest; +use crate::tools::runtimes::shell::ShellRequestCommandInputKind; +use crate::tools::runtimes::shell::ShellRuntime; +use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::turn_diff_tracker::TurnDiffTracker; +use codex_hooks::PermissionRequestToolInput; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_shell_command::powershell::try_find_powershell_executable_blocking; use codex_shell_command::powershell::try_find_pwsh_executable_blocking; use serde_json::json; +use std::collections::HashMap; use tokio::sync::Mutex; use tokio::sync::watch; @@ -199,6 +207,64 @@ fn shell_command_handler_rejects_login_when_disallowed() { ); } +#[tokio::test] +async fn shell_runtime_rewrite_updates_command_and_justification() { + let (session, turn) = make_session_and_context().await; + let session = Arc::new(session); + let turn = Arc::new(turn); + let req = ShellRequest { + command: session + .user_shell() + .derive_exec_args("echo original", /*use_login_shell*/ true), + command_input_kind: ShellRequestCommandInputKind::ShellString { + use_login_shell: true, + }, + hook_command: "echo original".to_string(), + cwd: turn.cwd.clone(), + timeout_ms: None, + env: HashMap::new(), + explicit_env_overrides: HashMap::new(), + network: None, + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + #[cfg(unix)] + additional_permissions_preapproved: false, + justification: Some("old justification".to_string()), + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + }; + let runtime = ShellRuntime::for_shell_command(super::ShellRuntimeBackend::ShellCommandClassic); + + let updated = runtime + .updated_request_from_permission_request( + &req, + &PermissionRequestToolInput { + command: "echo rewritten".to_string(), + description: Some("new justification".to_string()), + }, + &ApprovalCtx { + session: &session, + turn: &turn, + call_id: "call-1", + guardian_review_id: None, + retry_reason: None, + network_approval_context: None, + }, + ) + .expect("rewrite should succeed"); + + assert_eq!( + updated.command, + session + .user_shell() + .derive_exec_args("echo rewritten", /*use_login_shell*/ true) + ); + assert_eq!(updated.hook_command, "echo rewritten"); + assert_eq!(updated.justification, Some("new justification".to_string())); +} + #[tokio::test] async fn shell_pre_tool_use_payload_uses_joined_command() { let payload = ToolPayload::LocalShell { diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 9c8649cb64..a278b6cfa2 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -208,10 +208,15 @@ impl ToolHandler for UnifiedExecHandler { turn.tools_config.allow_login_shell, ) .map_err(FunctionCallError::RespondToModel)?; + let use_login_shell = + resolve_use_login_shell(args.login, turn.tools_config.allow_login_shell) + .map_err(FunctionCallError::RespondToModel)?; let command_for_display = codex_shell_command::parse_command::shlex_join(&command); + let hook_command = args.cmd.clone(); let ExecCommandArgs { workdir, + shell, tty, yield_time_ms, max_output_tokens, @@ -314,7 +319,9 @@ impl ToolHandler for UnifiedExecHandler { .exec_command( ExecCommandRequest { command, - hook_command: args.cmd, + hook_command, + shell, + use_login_shell, process_id, yield_time_ms, max_output_tokens, @@ -388,15 +395,7 @@ pub(crate) fn get_command( shell_mode: &UnifiedExecShellMode, allow_login_shell: bool, ) -> Result, String> { - let use_login_shell = match args.login { - Some(true) if !allow_login_shell => { - return Err( - "login shell is disabled by config; omit `login` or set it to false.".to_string(), - ); - } - Some(use_login_shell) => use_login_shell, - None => allow_login_shell, - }; + let use_login_shell = resolve_use_login_shell(args.login, allow_login_shell)?; match shell_mode { UnifiedExecShellMode::Direct => { @@ -416,6 +415,19 @@ pub(crate) fn get_command( } } +pub(crate) fn resolve_use_login_shell( + login: Option, + allow_login_shell: bool, +) -> Result { + match login { + Some(true) if !allow_login_shell => { + Err("login shell is disabled by config; omit `login` or set it to false.".to_string()) + } + Some(use_login_shell) => Ok(use_login_shell), + None => Ok(allow_login_shell), + } +} + #[cfg(test)] #[path = "unified_exec_tests.rs"] mod tests; 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 b641814516..982621bd6e 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -2,6 +2,12 @@ use super::*; use crate::shell::default_user_shell; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; +use crate::tools::runtimes::unified_exec::UnifiedExecRequest; +use crate::tools::runtimes::unified_exec::UnifiedExecRuntime; +use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ExecApprovalRequirement; +use codex_hooks::PermissionRequestToolInput; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_tools::UnifiedExecShellMode; @@ -9,6 +15,7 @@ use codex_tools::ZshForkConfig; use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::PathExt; use pretty_assertions::assert_eq; +use std::collections::HashMap; use std::fs; use std::sync::Arc; use tempfile::tempdir; @@ -198,6 +205,81 @@ fn exec_command_args_resolve_relative_additional_permissions_against_workdir() - Ok(()) } +#[tokio::test] +async fn unified_exec_runtime_rewrite_updates_command_and_justification() { + let (session, turn) = make_session_and_context().await; + let session = Arc::new(session); + let turn = Arc::new(turn); + let manager = UnifiedExecProcessManager::default(); + let runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct); + let req = UnifiedExecRequest { + command: session + .user_shell() + .derive_exec_args("echo original", /*use_login_shell*/ true), + hook_command: "echo original".to_string(), + shell: Some("/bin/bash".to_string()), + use_login_shell: true, + process_id: 7, + cwd: turn.cwd.clone(), + env: HashMap::new(), + explicit_env_overrides: HashMap::new(), + network: None, + tty: false, + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + #[cfg(unix)] + additional_permissions_preapproved: false, + justification: Some("old justification".to_string()), + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + }; + + let updated = runtime + .updated_request_from_permission_request( + &req, + &PermissionRequestToolInput { + command: "echo rewritten".to_string(), + description: Some("new justification".to_string()), + }, + &ApprovalCtx { + session: &session, + turn: &turn, + call_id: "call-1", + guardian_review_id: None, + retry_reason: None, + network_approval_context: None, + }, + ) + .expect("rewrite should succeed"); + + assert_eq!( + updated.command, + get_command( + &ExecCommandArgs { + cmd: "echo rewritten".to_string(), + workdir: None, + shell: Some("/bin/bash".to_string()), + login: Some(true), + tty: false, + yield_time_ms: 10_000, + max_output_tokens: None, + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + prefix_rule: None, + }, + session.user_shell(), + &UnifiedExecShellMode::Direct, + /*allow_login_shell*/ true, + ) + .expect("command should build") + ); + assert_eq!(updated.hook_command, "echo rewritten"); + assert_eq!(updated.justification, Some("new justification".to_string())); +} + #[tokio::test] async fn exec_command_pre_tool_use_payload_uses_raw_command() { let payload = ToolPayload::Function { diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 1ec21fb786..81e7d8c310 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, } +struct ApprovalResult { + decision: ReviewDecision, + updated_req: Option, +} + struct ApprovalTelemetry<'a> { otel: &'a SessionTelemetry, tool_name: &'a str, @@ -124,6 +129,7 @@ impl ToolOrchestrator { let otel_tn = &tool_ctx.tool_name; let otel_ci = &tool_ctx.call_id; let use_guardian = routes_approval_to_guardian(turn_ctx); + let mut effective_req = None; // 1) Approval let mut already_approved = false; @@ -153,9 +159,9 @@ impl ToolOrchestrator { retry_reason: reason, network_approval_context: None, }; - let decision = Self::request_approval( + let approval = Self::request_approval( tool, - req, + effective_req.as_ref().unwrap_or(req), tool_ctx.call_id.as_str(), approval_ctx, turn_ctx, @@ -166,10 +172,11 @@ impl ToolOrchestrator { }, ) .await?; + effective_req = approval.updated_req.or(effective_req); Self::enforce_approval_decision( tool_ctx.session.as_ref(), guardian_review_id.as_deref(), - decision, + approval.decision, ) .await?; already_approved = true; @@ -183,16 +190,17 @@ impl ToolOrchestrator { .requirements_toml() .network .is_some(); - let initial_sandbox = match tool.sandbox_mode_for_first_attempt(req) { - SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None, - SandboxOverride::NoOverride => self.sandbox.select_initial( - &turn_ctx.file_system_sandbox_policy, - turn_ctx.network_sandbox_policy, - tool.sandbox_preference(), - turn_ctx.windows_sandbox_level, - has_managed_network_requirements, - ), - }; + let initial_sandbox = + match tool.sandbox_mode_for_first_attempt(effective_req.as_ref().unwrap_or(req)) { + SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None, + SandboxOverride::NoOverride => self.sandbox.select_initial( + &turn_ctx.file_system_sandbox_policy, + turn_ctx.network_sandbox_policy, + tool.sandbox_preference(), + turn_ctx.windows_sandbox_level, + has_managed_network_requirements, + ), + }; // Platform-specific flag gating is handled by SandboxManager::select_initial. let use_legacy_landlock = turn_ctx.features.use_legacy_landlock(); @@ -215,7 +223,7 @@ impl ToolOrchestrator { let (first_result, first_deferred_network_approval) = Self::run_attempt( tool, - req, + effective_req.as_ref().unwrap_or(req), tool_ctx, &initial_attempt, has_managed_network_requirements, @@ -298,9 +306,9 @@ impl ToolOrchestrator { network_approval_context: network_approval_context.clone(), }; - let decision = Self::request_approval( + let approval = Self::request_approval( tool, - req, + effective_req.as_ref().unwrap_or(req), &format!("{}:retry", tool_ctx.call_id), approval_ctx, turn_ctx, @@ -311,14 +319,16 @@ impl ToolOrchestrator { }, ) .await?; + effective_req = approval.updated_req.or(effective_req); Self::enforce_approval_decision( tool_ctx.session.as_ref(), guardian_review_id.as_deref(), - decision, + approval.decision, ) .await?; } + let req = effective_req.as_ref().unwrap_or(req); let escalated_attempt = SandboxAttempt { sandbox: SandboxType::None, policy: &turn_ctx.sandbox_policy, @@ -364,7 +374,7 @@ impl ToolOrchestrator { approval_ctx: ApprovalCtx<'_>, turn_ctx: &crate::codex::TurnContext, telemetry: ApprovalTelemetry<'_>, - ) -> Result + ) -> Result, ToolError> where T: ToolRuntime, { @@ -377,14 +387,32 @@ impl ToolOrchestrator { ) .await { - Some(PermissionRequestDecision::Allow { .. }) => { + Some(PermissionRequestDecision::Allow { updated_input, .. }) => { + let updated_req = updated_input + .as_ref() + .map(|updated_input| { + tool.updated_request_from_permission_request( + req, + updated_input, + &approval_ctx, + ) + }) + .transpose() + .map_err(|err| { + ToolError::Rejected(format!( + "PermissionRequest hook returned invalid updatedInput: {err}" + )) + })?; telemetry.otel.tool_decision( telemetry.tool_name, telemetry.call_id, &ReviewDecision::Approved, ToolDecisionSource::Config, ); - return Ok(ReviewDecision::Approved); + return Ok(ApprovalResult { + decision: ReviewDecision::Approved, + updated_req, + }); } Some(PermissionRequestDecision::Deny { message }) => { telemetry.otel.tool_decision( @@ -411,7 +439,10 @@ impl ToolOrchestrator { &decision, otel_source, ); - Ok(decision) + Ok(ApprovalResult { + decision, + updated_req: None, + }) } // Normalizes approval outcomes from hooks, guardian, and user prompts into diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 768ae0f483..eaaf8b4812 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -33,6 +33,7 @@ use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::approval_permission_suggestions; use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; +use codex_hooks::PermissionRequestToolInput; use codex_hooks::PermissionUpdateDestination; use codex_network_proxy::NetworkProxy; use codex_protocol::exec_output::ExecToolCallOutput; @@ -42,11 +43,13 @@ 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 shlex::split as shlex_split; use std::collections::HashMap; #[derive(Clone, Debug)] pub struct ShellRequest { pub command: Vec, + pub command_input_kind: ShellRequestCommandInputKind, pub hook_command: String, pub cwd: AbsolutePathBuf, pub timeout_ms: Option, @@ -61,6 +64,12 @@ pub struct ShellRequest { pub exec_approval_requirement: ExecApprovalRequirement, } +#[derive(Clone, Debug)] +pub enum ShellRequestCommandInputKind { + Argv, + ShellString { use_login_shell: bool }, +} + /// Selects `ShellRuntime` behavior for different callers. /// /// Note: `Generic` is not the same as `ShellCommandClassic`. @@ -221,11 +230,44 @@ impl Approvable for ShellRuntime { }) } + fn updated_request_from_permission_request( + &self, + req: &ShellRequest, + updated_input: &PermissionRequestToolInput, + approval_ctx: &ApprovalCtx<'_>, + ) -> Result { + let mut updated_req = req.clone(); + updated_req.command = match req.command_input_kind { + ShellRequestCommandInputKind::Argv => parse_command_argv(&updated_input.command)?, + ShellRequestCommandInputKind::ShellString { use_login_shell } => approval_ctx + .session + .user_shell() + .derive_exec_args(&updated_input.command, use_login_shell), + }; + updated_req.hook_command = updated_input.command.clone(); + updated_req.justification = updated_input.description.clone(); + Ok(updated_req) + } + fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride { sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement) } } +fn parse_command_argv(command: &str) -> Result, String> { + let parsed = shlex_split(command).unwrap_or_else(|| { + command + .split_whitespace() + .map(ToString::to_string) + .collect() + }); + if parsed.is_empty() { + Err("command cannot be empty".to_string()) + } else { + Ok(parsed) + } +} + impl ToolRuntime for ShellRuntime { fn network_approval_spec( &self, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 4fe7bfe5c0..eaf177c0db 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -11,7 +11,9 @@ use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; +use crate::shell::Shell; use crate::shell::ShellType; +use crate::shell::get_shell_by_model_provided_path; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_sandbox_command; @@ -34,6 +36,7 @@ use crate::unified_exec::NoopSpawnLifecycle; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecProcess; use crate::unified_exec::UnifiedExecProcessManager; +use codex_hooks::PermissionRequestToolInput; use codex_hooks::PermissionUpdateDestination; use codex_network_proxy::NetworkProxy; use codex_protocol::error::CodexErr; @@ -46,6 +49,8 @@ use codex_tools::UnifiedExecShellMode; use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; /// Request payload used by the unified-exec runtime after approvals and /// sandbox preferences have been resolved for the current turn. @@ -53,6 +58,8 @@ use std::collections::HashMap; pub struct UnifiedExecRequest { pub command: Vec, pub hook_command: String, + pub shell: Option, + pub use_login_shell: bool, pub process_id: i32, pub cwd: AbsolutePathBuf, pub env: HashMap, @@ -201,6 +208,25 @@ impl Approvable for UnifiedExecRuntime<'_> { }) } + fn updated_request_from_permission_request( + &self, + req: &UnifiedExecRequest, + updated_input: &PermissionRequestToolInput, + approval_ctx: &ApprovalCtx<'_>, + ) -> Result { + let mut updated_req = req.clone(); + updated_req.command = build_command( + &updated_input.command, + req.shell.as_deref(), + req.use_login_shell, + approval_ctx.session.user_shell(), + &self.shell_mode, + ); + updated_req.hook_command = updated_input.command.clone(); + updated_req.justification = updated_input.description.clone(); + Ok(updated_req) + } + fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride { sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement) } @@ -347,3 +373,28 @@ impl<'a> ToolRuntime for UnifiedExecRunt }) } } + +fn build_command( + cmd: &str, + shell: Option<&str>, + use_login_shell: bool, + session_shell: Arc, + shell_mode: &UnifiedExecShellMode, +) -> Vec { + match shell_mode { + UnifiedExecShellMode::Direct => { + let model_shell = shell.map(|shell_str| { + let mut shell = get_shell_by_model_provided_path(&PathBuf::from(shell_str)); + shell.shell_snapshot = crate::shell::empty_shell_snapshot_receiver(); + shell + }); + let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); + shell.derive_exec_args(cmd, use_login_shell) + } + UnifiedExecShellMode::ZshFork(zsh_fork_config) => vec![ + zsh_fork_config.shell_zsh_path.to_string_lossy().to_string(), + if use_login_shell { "-lc" } else { "-c" }.to_string(), + cmd.to_string(), + ], + } +} diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index dd84f4d08c..ce57c31c13 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -10,6 +10,7 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::state::SessionServices; use crate::tools::network_approval::NetworkApprovalSpec; +use codex_hooks::PermissionRequestToolInput; use codex_hooks::PermissionUpdate; use codex_hooks::PermissionUpdateBehavior; use codex_hooks::PermissionUpdateDestination; @@ -341,6 +342,15 @@ pub(crate) trait Approvable { None } + fn updated_request_from_permission_request( + &self, + _req: &Req, + _updated_input: &PermissionRequestToolInput, + _approval_ctx: &ApprovalCtx<'_>, + ) -> Result { + Err("PermissionRequest hook returned unsupported updatedInput".to_string()) + } + /// Decide we can request an approval for no-sandbox execution. fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { match policy { diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 4285ef2e51..b524059ba6 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -89,6 +89,8 @@ impl UnifiedExecContext { pub(crate) struct ExecCommandRequest { pub command: Vec, pub hook_command: String, + pub shell: Option, + pub use_login_shell: bool, pub process_id: i32, pub yield_time_ms: u64, pub max_output_tokens: Option, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 9034ff24db..2dd04dc759 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -688,6 +688,8 @@ impl UnifiedExecProcessManager { let req = UnifiedExecToolRequest { command: request.command.clone(), hook_command: request.hook_command.clone(), + shell: request.shell.clone(), + use_login_shell: request.use_login_shell, process_id: request.process_id, cwd, env, diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 2f43074903..1065409fa2 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -284,6 +284,17 @@ if mode == "allow": "decision": {{"behavior": "allow"}} }} }})) +elif mode == "rewrite": + updated = json.loads(reason) + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PermissionRequest", + "decision": {{ + "behavior": "allow", + "updatedInput": updated + }} + }} + }})) elif mode == "allow_selected_session": print(json.dumps({{ "hookSpecificOutput": {{ @@ -1346,6 +1357,89 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_rewrites_shell_command_input_before_execution() -> 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-shell-command-original-marker"); + let rewritten_marker = + std::env::temp_dir().join("permissionrequest-shell-command-rewritten-marker"); + let original_command = format!("rm -f {}", original_marker.display()); + let rewritten_command = format!("rm -f {}", rewritten_marker.display()); + let rewritten_description = "rewrite the command before execution"; + 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_input = serde_json::json!({ + "command": rewritten_command, + "description": rewritten_description, + }); + 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), + "rewrite", + &rewritten_input.to_string(), + ) { + panic!("failed to write permission request 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(&original_marker, "seed").context("create original marker")?; + fs::write(&rewritten_marker, "seed").context("create rewritten marker")?; + + test.submit_turn_with_policies( + "run the rewritten shell command after hook approval", + AskForApproval::OnRequest, + codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + requests[1].function_call_output(call_id); + assert!( + original_marker.exists(), + "original command should not run after hook rewrite" + ); + assert!( + !rewritten_marker.exists(), + "rewritten command should remove rewritten marker" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn permission_request_hook_sees_raw_exec_command_input() -> 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 fb2d81ce52..e79d75f873 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 @@ -36,8 +36,12 @@ "type": "string" }, "updatedInput": { - "default": null, - "description": "Reserved for a future input-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present." + "allOf": [ + { + "$ref": "#/definitions/PermissionRequestToolInput" + } + ], + "default": null }, "updatedPermissions": { "default": null, @@ -72,6 +76,21 @@ ], "type": "object" }, + "PermissionRequestToolInput": { + "additionalProperties": false, + "properties": { + "command": { + "type": "string" + }, + "description": { + "type": "string" + } + }, + "required": [ + "command" + ], + "type": "object" + }, "PermissionUpdate": { "oneOf": [ { diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index bcad73da28..e8ed153f4b 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -23,6 +23,7 @@ pub(crate) struct PreToolUseOutput { pub(crate) enum PermissionRequestDecision { Allow { updated_permissions: Vec, + updated_input: Option, }, Deny { message: String, @@ -63,6 +64,7 @@ pub(crate) struct StopOutput { pub invalid_block_reason: Option, } +use crate::events::permission_request::PermissionRequestToolInput; use crate::schema::BlockDecisionWire; use crate::schema::HookUniversalOutputWire; use crate::schema::PermissionRequestBehaviorWire; @@ -302,8 +304,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 matches!(decision.behavior, PermissionRequestBehaviorWire::Deny) + && decision.updated_input.is_some() + { + Some("PermissionRequest hook returned updatedInput for deny decision".to_string()) } else if matches!(decision.behavior, PermissionRequestBehaviorWire::Deny) && decision.updated_permissions.is_some() { @@ -321,6 +325,7 @@ fn permission_request_decision( match decision.behavior { PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow { updated_permissions: decision.updated_permissions.clone().unwrap_or_default(), + updated_input: decision.updated_input.clone(), }, PermissionRequestBehaviorWire::Deny => PermissionRequestDecision::Deny { message: decision @@ -431,9 +436,10 @@ mod tests { use super::PermissionRequestDecision; use super::parse_permission_request; + use crate::events::permission_request::PermissionRequestToolInput; #[test] - fn permission_request_rejects_reserved_updated_input_field() { + fn permission_request_accepts_updated_input_for_allow_decision() { let parsed = parse_permission_request( &json!({ "continue": true, @@ -441,7 +447,9 @@ mod tests { "hookEventName": "PermissionRequest", "decision": { "behavior": "allow", - "updatedInput": {} + "updatedInput": { + "command": "echo hi" + } } } }) @@ -450,8 +458,14 @@ mod tests { .expect("permission request hook output should parse"); assert_eq!( - parsed.invalid_reason, - Some("PermissionRequest hook returned unsupported updatedInput".to_string()) + parsed.decision, + Some(PermissionRequestDecision::Allow { + updated_permissions: vec![], + updated_input: Some(PermissionRequestToolInput { + command: "echo hi".to_string(), + description: None, + }), + }) ); } @@ -493,6 +507,7 @@ mod tests { crate::events::permission_request::PermissionUpdateBehavior::Allow, destination: crate::events::permission_request::PermissionUpdateDestination::UserSettings, }], + updated_input: None, }) ); } @@ -528,10 +543,36 @@ mod tests { crate::events::permission_request::PermissionUpdateDestination::Session, }, ], + updated_input: None, }) ); } + #[test] + fn permission_request_rejects_updated_input_for_deny_decision() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "deny", + "updatedInput": { + "command": "echo nope" + } + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!( + parsed.invalid_reason, + Some("PermissionRequest hook returned updatedInput for deny decision".to_string()) + ); + } + #[test] fn permission_request_rejects_updated_permissions_for_deny_decision() { let parsed = parse_permission_request( diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 96d4996879..547859a8e1 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 +//! rewriting the hook-visible tool input for allow decisions, or decline to +//! decide and let the normal approval flow continue. //! //! The event also mirrors the rest of the hook system's lifecycle: //! @@ -23,7 +23,6 @@ use crate::engine::command_runner::CommandRunResult; use crate::engine::dispatcher; use crate::engine::output_parser; use crate::schema::PermissionRequestCommandInput; -use crate::schema::PermissionRequestToolInput; use codex_protocol::ThreadId; use codex_protocol::protocol::HookCompletedEvent; use codex_protocol::protocol::HookEventName; @@ -88,10 +87,20 @@ pub struct PermissionRequestRequest { pub permission_suggestions: Vec, } +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +#[serde(deny_unknown_fields)] +pub struct PermissionRequestToolInput { + pub command: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub description: Option, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum PermissionRequestDecision { Allow { updated_permissions: Vec, + updated_input: Option, }, Deny { message: String, @@ -212,10 +221,12 @@ fn resolve_permission_request_decision<'a>( ) -> Option { let mut saw_allow = false; let mut updated_permissions = Vec::new(); + let mut updated_input = None; for decision in decisions { match decision { PermissionRequestDecision::Allow { updated_permissions: selected_permissions, + updated_input: selected_input, } => { saw_allow = true; for permission in selected_permissions { @@ -223,6 +234,9 @@ fn resolve_permission_request_decision<'a>( updated_permissions.push(permission.clone()); } } + if let Some(selected_input) = selected_input { + updated_input = Some(selected_input.clone()); + } } PermissionRequestDecision::Deny { message } => { return Some(PermissionRequestDecision::Deny { @@ -233,6 +247,7 @@ fn resolve_permission_request_decision<'a>( } saw_allow.then_some(PermissionRequestDecision::Allow { updated_permissions, + updated_input, }) } @@ -242,6 +257,7 @@ fn invalid_permission_updates( ) -> Option { let PermissionRequestDecision::Allow { updated_permissions, + updated_input: _, } = decision? else { return None; @@ -315,9 +331,11 @@ fn parse_completed( match parsed_decision { output_parser::PermissionRequestDecision::Allow { updated_permissions, + updated_input, } => { decision = Some(PermissionRequestDecision::Allow { updated_permissions, + updated_input, }); } output_parser::PermissionRequestDecision::Deny { message } => { @@ -387,6 +405,7 @@ mod tests { use pretty_assertions::assert_eq; use super::PermissionRequestDecision; + use super::PermissionRequestToolInput; use super::PermissionUpdate; use super::PermissionUpdateBehavior; use super::PermissionUpdateDestination; @@ -399,6 +418,7 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![], + updated_input: None, }, PermissionRequestDecision::Deny { message: "repo deny".to_string(), @@ -418,9 +438,11 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![], + updated_input: None, }, PermissionRequestDecision::Allow { updated_permissions: vec![], + updated_input: None, }, ]; @@ -428,6 +450,7 @@ mod tests { resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { updated_permissions: vec![], + updated_input: None, }) ); } @@ -451,9 +474,11 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![permission.clone()], + updated_input: None, }, PermissionRequestDecision::Allow { updated_permissions: vec![permission.clone()], + updated_input: None, }, ]; @@ -461,6 +486,7 @@ mod tests { resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { updated_permissions: vec![permission], + updated_input: None, }) ); } @@ -474,9 +500,11 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![directory_update.clone()], + updated_input: None, }, PermissionRequestDecision::Allow { updated_permissions: vec![directory_update.clone()], + updated_input: None, }, ]; @@ -484,6 +512,38 @@ mod tests { resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { updated_permissions: vec![directory_update], + updated_input: None, + }) + ); + } + + #[test] + fn permission_request_uses_last_updated_input_from_allows() { + let decisions = [ + PermissionRequestDecision::Allow { + updated_permissions: vec![], + updated_input: Some(PermissionRequestToolInput { + command: "echo first".to_string(), + description: Some("first".to_string()), + }), + }, + PermissionRequestDecision::Allow { + updated_permissions: vec![], + updated_input: Some(PermissionRequestToolInput { + command: "echo second".to_string(), + description: Some("second".to_string()), + }), + }, + ]; + + assert_eq!( + resolve_permission_request_decision(decisions.iter()), + Some(PermissionRequestDecision::Allow { + updated_permissions: vec![], + updated_input: Some(PermissionRequestToolInput { + command: "echo second".to_string(), + description: Some("second".to_string()), + }), }) ); } @@ -505,6 +565,7 @@ mod tests { behavior: PermissionUpdateBehavior::Allow, destination: PermissionUpdateDestination::UserSettings, }], + updated_input: None, }; assert_eq!( diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 30cbe2b97f..752f29713c 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -8,6 +8,7 @@ mod types; pub use events::permission_request::PermissionRequestDecision; pub use events::permission_request::PermissionRequestOutcome; pub use events::permission_request::PermissionRequestRequest; +pub use events::permission_request::PermissionRequestToolInput; pub use events::permission_request::PermissionUpdate; pub use events::permission_request::PermissionUpdateBehavior; pub use events::permission_request::PermissionUpdateDestination; diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 7b27a14ac0..b07f844fbb 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -12,6 +12,7 @@ use serde_json::Value; use std::path::Path; use std::path::PathBuf; +use crate::events::permission_request::PermissionRequestToolInput; use crate::events::permission_request::PermissionUpdate; const GENERATED_DIR: &str = "generated"; @@ -140,11 +141,8 @@ pub(crate) struct PermissionRequestHookSpecificOutputWire { #[serde(deny_unknown_fields)] pub(crate) struct PermissionRequestDecisionWire { pub behavior: PermissionRequestBehaviorWire, - /// Reserved for a future input-rewrite capability. - /// - /// PermissionRequest hooks currently fail closed if this field is present. #[serde(default)] - pub updated_input: Option, + pub updated_input: Option, #[serde(default)] pub updated_permissions: Option>, #[serde(default)] @@ -236,15 +234,6 @@ pub(crate) struct PreToolUseCommandInput { pub tool_use_id: String, } -#[derive(Debug, Clone, Serialize, JsonSchema)] -#[serde(rename_all = "camelCase")] -#[serde(deny_unknown_fields)] -pub(crate) struct PermissionRequestToolInput { - pub command: String, - #[serde(skip_serializing_if = "Option::is_none")] - pub description: Option, -} - #[derive(Debug, Clone, Serialize, JsonSchema)] #[serde(deny_unknown_fields)] #[schemars(rename = "permission-request.command.input")]