diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index d34f91a03a..8b6b28cfc3 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -48,8 +48,6 @@ pub(crate) enum PreToolUseHookResult { Blocked(String), } -pub(crate) const MAX_HOOK_INPUT_REWRITES: usize = 8; - pub(crate) enum PendingInputHookDisposition { Accepted(Box), Blocked { additional_contexts: Vec }, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index f1fc054d0f..ed1955a48f 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -25,7 +25,6 @@ use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; -use crate::hook_runtime::MAX_HOOK_INPUT_REWRITES; use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; @@ -203,43 +202,34 @@ pub(crate) async fn handle_mcp_tool_call( .await; let mut invocation = invocation; - let mut input_rewrites = 0; - loop { - let Some(decision) = maybe_request_mcp_tool_approval( - &sess, - turn_context, - &call_id, - &invocation, - &hook_tool_name, - metadata.as_ref(), - approval_mode, - ) - .await - else { - break; - }; + if let Some(decision) = maybe_request_mcp_tool_approval( + &sess, + turn_context, + &call_id, + &invocation, + &hook_tool_name, + metadata.as_ref(), + approval_mode, + ) + .await + { let tool_input = invocation .arguments .clone() .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())); let result = match decision { McpToolApprovalDecision::AcceptWithUpdatedInput(updated_input) => { - input_rewrites += 1; - if input_rewrites > MAX_HOOK_INPUT_REWRITES { - notify_mcp_tool_call_skip( - sess.as_ref(), - turn_context.as_ref(), - &call_id, - invocation, - mcp_app_resource_uri.clone(), - "hook input rewrite limit exceeded".to_string(), - /*already_started*/ true, - ) - .await - } else { - invocation.arguments = Some(updated_input); - continue; - } + invocation.arguments = Some(updated_input); + return handle_approved_mcp_tool_call( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + metadata.as_ref(), + request_meta, + mcp_app_resource_uri, + ) + .await; } McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index a5c5ec0ca4..577b45a8d9 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1035,6 +1035,7 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an &tool_ctx, turn.as_ref(), AskForApproval::Never, + crate::tools::sandboxing::InitialApprovalState::Evaluate, ) .await .expect("probe runtime should succeed"); diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 1dc06780ae..075ba00b94 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -33,6 +33,7 @@ use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::apply_patch::ApplyPatchRequest; use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; +use crate::tools::sandboxing::InitialApprovalState; use crate::tools::sandboxing::ToolCtx; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; @@ -369,129 +370,143 @@ impl ToolHandler for ApplyPatchHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { - session, - turn, - tracker, - call_id, - tool_name, - payload, - .. - } = invocation; + handle_apply_patch_invocation(invocation, InitialApprovalState::Evaluate).await + } - let patch_input = match payload { - ToolPayload::Function { arguments } => { - let args: ApplyPatchToolArgs = parse_arguments(&arguments)?; - args.input - } - ToolPayload::Custom { input } => input, - _ => { - return Err(FunctionCallError::RespondToModel( - "apply_patch handler received unsupported payload".to_string(), - )); - } - }; + async fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> Result { + handle_apply_patch_invocation(invocation, InitialApprovalState::AlreadyApproved).await + } +} - // Re-parse and verify the patch so we can compute changes and approval. - // Avoid building temporary ExecParams/command vectors; derive directly from inputs. - let cwd = turn.cwd.clone(); - let command = vec!["apply_patch".to_string(), patch_input.clone()]; - let Some(turn_environment) = turn.environments.primary() else { +async fn handle_apply_patch_invocation( + invocation: ToolInvocation, + initial_approval_state: InitialApprovalState, +) -> Result { + let ToolInvocation { + session, + turn, + tracker, + call_id, + tool_name, + payload, + .. + } = invocation; + + let patch_input = match payload { + ToolPayload::Function { arguments } => { + let args: ApplyPatchToolArgs = parse_arguments(&arguments)?; + args.input + } + ToolPayload::Custom { input } => input, + _ => { return Err(FunctionCallError::RespondToModel( - "apply_patch is unavailable in this session".to_string(), + "apply_patch handler received unsupported payload".to_string(), )); - }; - let fs = turn_environment.environment.get_filesystem(); - let sandbox = turn_environment - .environment - .is_remote() - .then(|| turn.file_system_sandbox_context(/*additional_permissions*/ None)); - match codex_apply_patch::maybe_parse_apply_patch_verified( - &command, - &cwd, - fs.as_ref(), - sandbox.as_ref(), - ) - .await - { - codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = - effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await; - match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) - .await - { - InternalApplyPatchInvocation::Output(item) => { - let content = item?; - Ok(ApplyPatchToolOutput::from_text(content)) - } - InternalApplyPatchInvocation::DelegateToRuntime(apply) => { - let changes = convert_apply_patch_to_protocol(&apply.action); - let emitter = - ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); - let event_ctx = ToolEventCtx::new( - session.as_ref(), - turn.as_ref(), - &call_id, - Some(&tracker), - ); - emitter.begin(event_ctx).await; + } + }; - let req = ApplyPatchRequest { - action: apply.action, - file_paths, - changes, - exec_approval_requirement: apply.exec_approval_requirement, - additional_permissions: effective_additional_permissions - .additional_permissions, - permissions_preapproved: effective_additional_permissions - .permissions_preapproved, - }; + // Re-parse and verify the patch so we can compute changes and approval. + // Avoid building temporary ExecParams/command vectors; derive directly from inputs. + let cwd = turn.cwd.clone(); + let command = vec!["apply_patch".to_string(), patch_input.clone()]; + let Some(turn_environment) = turn.environments.primary() else { + return Err(FunctionCallError::RespondToModel( + "apply_patch is unavailable in this session".to_string(), + )); + }; + let fs = turn_environment.environment.get_filesystem(); + let sandbox = turn_environment + .environment + .is_remote() + .then(|| turn.file_system_sandbox_context(/*additional_permissions*/ None)); + match codex_apply_patch::maybe_parse_apply_patch_verified( + &command, + &cwd, + fs.as_ref(), + sandbox.as_ref(), + ) + .await + { + codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { + let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = + effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await; + match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) + .await + { + InternalApplyPatchInvocation::Output(item) => { + let content = item?; + Ok(ApplyPatchToolOutput::from_text(content)) + } + InternalApplyPatchInvocation::DelegateToRuntime(apply) => { + let changes = convert_apply_patch_to_protocol(&apply.action); + let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); + let event_ctx = ToolEventCtx::new( + session.as_ref(), + turn.as_ref(), + &call_id, + Some(&tracker), + ); + emitter.begin(event_ctx).await; - let mut orchestrator = ToolOrchestrator::new(); - let mut runtime = ApplyPatchRuntime::new(); - let tool_ctx = ToolCtx { - session: session.clone(), - turn: turn.clone(), - call_id: call_id.clone(), - tool_name: tool_name.display(), - }; - let out = orchestrator - .run( - &mut runtime, - &req, - &tool_ctx, - turn.as_ref(), - turn.approval_policy.value(), - ) - .await - .map(|result| result.output); - let event_ctx = ToolEventCtx::new( - session.as_ref(), + let req = ApplyPatchRequest { + action: apply.action, + file_paths, + changes, + exec_approval_requirement: apply.exec_approval_requirement, + additional_permissions: effective_additional_permissions + .additional_permissions, + permissions_preapproved: effective_additional_permissions + .permissions_preapproved, + }; + + let mut orchestrator = ToolOrchestrator::new(); + let mut runtime = ApplyPatchRuntime::new(); + let tool_ctx = ToolCtx { + session: session.clone(), + turn: turn.clone(), + call_id: call_id.clone(), + tool_name: tool_name.display(), + }; + let out = orchestrator + .run( + &mut runtime, + &req, + &tool_ctx, turn.as_ref(), - &call_id, - Some(&tracker), - ); - let content = emitter.finish(event_ctx, out).await?; - Ok(ApplyPatchToolOutput::from_text(content)) - } + turn.approval_policy.value(), + initial_approval_state, + ) + .await + .map(|result| result.output); + let event_ctx = ToolEventCtx::new( + session.as_ref(), + turn.as_ref(), + &call_id, + Some(&tracker), + ); + let content = emitter.finish(event_ctx, out).await?; + Ok(ApplyPatchToolOutput::from_text(content)) } } - codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { - Err(FunctionCallError::RespondToModel(format!( - "apply_patch verification failed: {parse_error}" - ))) - } - codex_apply_patch::MaybeApplyPatchVerified::ShellParseError(error) => { - tracing::trace!("Failed to parse apply_patch input, {error:?}"); - Err(FunctionCallError::RespondToModel( - "apply_patch handler received invalid patch input".to_string(), - )) - } - codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => { - Err(FunctionCallError::RespondToModel( - "apply_patch handler received non-apply_patch input".to_string(), - )) - } + } + codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => { + Err(FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: {parse_error}" + ))) + } + codex_apply_patch::MaybeApplyPatchVerified::ShellParseError(error) => { + tracing::trace!("Failed to parse apply_patch input, {error:?}"); + Err(FunctionCallError::RespondToModel( + "apply_patch handler received invalid patch input".to_string(), + )) + } + codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => { + Err(FunctionCallError::RespondToModel( + "apply_patch handler received non-apply_patch input".to_string(), + )) } } } @@ -570,6 +585,7 @@ pub(crate) async fn intercept_apply_patch( &tool_ctx, turn.as_ref(), turn.approval_policy.value(), + InitialApprovalState::Evaluate, ) .await .map(|result| result.output); diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 0bbf06f36b..042b6a035d 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -37,6 +37,7 @@ use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRequest; use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::runtimes::shell::ShellRuntimeBackend; +use crate::tools::sandboxing::InitialApprovalState; use crate::tools::sandboxing::ToolCtx; use codex_features::Feature; use codex_protocol::models::AdditionalPermissionProfile; @@ -130,6 +131,7 @@ struct RunExecLikeArgs { call_id: String, freeform: bool, shell_runtime_backend: ShellRuntimeBackend, + initial_approval_state: InitialApprovalState, } impl ShellHandler { @@ -155,6 +157,50 @@ impl ShellHandler { arg0: None, } } + + async fn handle_with_permission_request_hooks( + invocation: ToolInvocation, + initial_approval_state: InitialApprovalState, + ) -> Result { + let ToolInvocation { + session, + turn, + tracker, + call_id, + payload, + .. + } = invocation; + + let arguments = match payload { + ToolPayload::Function { arguments } => arguments, + _ => { + return Err(FunctionCallError::RespondToModel( + "unsupported payload for shell handler".to_string(), + )); + } + }; + + let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; + let params: ShellToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; + let prefix_rule = params.prefix_rule.clone(); + let exec_params = + ShellHandler::to_exec_params(¶ms, turn.as_ref(), session.conversation_id); + ShellHandler::run_exec_like(RunExecLikeArgs { + tool_name: "shell".to_string(), + exec_params, + hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), + additional_permissions: params.additional_permissions.clone(), + prefix_rule, + session, + turn, + tracker, + call_id, + freeform: false, + shell_runtime_backend: ShellRuntimeBackend::Generic, + initial_approval_state, + }) + .await + } } impl ShellCommandHandler { @@ -210,6 +256,62 @@ impl ShellCommandHandler { arg0: None, }) } + + async fn handle_with_permission_request_hooks( + &self, + invocation: ToolInvocation, + initial_approval_state: InitialApprovalState, + ) -> Result { + let ToolInvocation { + session, + turn, + tracker, + call_id, + payload, + .. + } = invocation; + + let ToolPayload::Function { arguments } = payload else { + return Err(FunctionCallError::RespondToModel(format!( + "unsupported payload for shell_command handler: {}", + self.tool_name().display() + ))); + }; + + let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; + let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; + let workdir = turn.resolve_path(params.workdir.clone()); + maybe_emit_implicit_skill_invocation( + session.as_ref(), + turn.as_ref(), + ¶ms.command, + &workdir, + ) + .await; + let prefix_rule = params.prefix_rule.clone(); + let exec_params = Self::to_exec_params( + ¶ms, + session.as_ref(), + turn.as_ref(), + session.conversation_id, + turn.tools_config.allow_login_shell, + )?; + ShellHandler::run_exec_like(RunExecLikeArgs { + tool_name: self.tool_name().display(), + exec_params, + hook_command: params.command, + additional_permissions: params.additional_permissions.clone(), + prefix_rule, + session, + turn, + tracker, + call_id, + freeform: true, + shell_runtime_backend: self.shell_runtime_backend(), + initial_approval_state, + }) + .await + } } impl From for ShellCommandHandler { @@ -268,42 +370,17 @@ impl ToolHandler for ShellHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { - session, - turn, - tracker, - call_id, - payload, - .. - } = invocation; + Self::handle_with_permission_request_hooks(invocation, InitialApprovalState::Evaluate).await + } - let arguments = match payload { - ToolPayload::Function { arguments } => arguments, - _ => { - return Err(FunctionCallError::RespondToModel( - "unsupported payload for shell handler".to_string(), - )); - } - }; - - let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; - let params: ShellToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; - let prefix_rule = params.prefix_rule.clone(); - let exec_params = - ShellHandler::to_exec_params(¶ms, turn.as_ref(), session.conversation_id); - ShellHandler::run_exec_like(RunExecLikeArgs { - tool_name: "shell".to_string(), - exec_params, - hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), - additional_permissions: params.additional_permissions.clone(), - prefix_rule, - session, - turn, - tracker, - call_id, - freeform: false, - shell_runtime_backend: ShellRuntimeBackend::Generic, - }) + async fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> Result { + Self::handle_with_permission_request_hooks( + invocation, + InitialApprovalState::AlreadyApproved, + ) .await } } @@ -354,44 +431,69 @@ impl ToolHandler for ContainerExecHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { - session, - turn, - tracker, - call_id, - payload, - .. - } = invocation; - - let arguments = match payload { - ToolPayload::Function { arguments } => arguments, - _ => { - return Err(FunctionCallError::RespondToModel( - "unsupported payload for container.exec handler".to_string(), - )); - } - }; - - let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; - let params: ShellToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; - let prefix_rule = params.prefix_rule.clone(); - let exec_params = - ShellHandler::to_exec_params(¶ms, turn.as_ref(), session.conversation_id); - ShellHandler::run_exec_like(RunExecLikeArgs { - tool_name: "container.exec".to_string(), - exec_params, - hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), - additional_permissions: params.additional_permissions.clone(), - prefix_rule, - session, - turn, - tracker, - call_id, - freeform: false, - shell_runtime_backend: ShellRuntimeBackend::Generic, - }) + handle_shell_function_invocation( + invocation, + "container.exec", + InitialApprovalState::Evaluate, + ) .await } + + async fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> Result { + handle_shell_function_invocation( + invocation, + "container.exec", + InitialApprovalState::AlreadyApproved, + ) + .await + } +} + +async fn handle_shell_function_invocation( + invocation: ToolInvocation, + tool_name: &str, + initial_approval_state: InitialApprovalState, +) -> Result { + let ToolInvocation { + session, + turn, + tracker, + call_id, + payload, + .. + } = invocation; + + let arguments = match payload { + ToolPayload::Function { arguments } => arguments, + _ => { + return Err(FunctionCallError::RespondToModel(format!( + "unsupported payload for {tool_name} handler" + ))); + } + }; + + let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; + let params: ShellToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; + let prefix_rule = params.prefix_rule.clone(); + let exec_params = ShellHandler::to_exec_params(¶ms, turn.as_ref(), session.conversation_id); + ShellHandler::run_exec_like(RunExecLikeArgs { + tool_name: tool_name.to_string(), + exec_params, + hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), + additional_permissions: params.additional_permissions.clone(), + prefix_rule, + session, + turn, + tracker, + call_id, + freeform: false, + shell_runtime_backend: ShellRuntimeBackend::Generic, + initial_approval_state, + }) + .await } impl ToolHandler for LocalShellHandler { @@ -478,38 +580,52 @@ impl ToolHandler for LocalShellHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { - session, - turn, - tracker, - call_id, - payload, - .. - } = invocation; - - let ToolPayload::LocalShell { params } = payload else { - return Err(FunctionCallError::RespondToModel( - "unsupported payload for local_shell handler".to_string(), - )); - }; - - let exec_params = - ShellHandler::to_exec_params(¶ms, turn.as_ref(), session.conversation_id); - ShellHandler::run_exec_like(RunExecLikeArgs { - tool_name: "local_shell".to_string(), - exec_params, - hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), - additional_permissions: None, - prefix_rule: None, - session, - turn, - tracker, - call_id, - freeform: false, - shell_runtime_backend: ShellRuntimeBackend::Generic, - }) - .await + handle_local_shell_invocation(invocation, InitialApprovalState::Evaluate).await } + + async fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> Result { + handle_local_shell_invocation(invocation, InitialApprovalState::AlreadyApproved).await + } +} + +async fn handle_local_shell_invocation( + invocation: ToolInvocation, + initial_approval_state: InitialApprovalState, +) -> Result { + let ToolInvocation { + session, + turn, + tracker, + call_id, + payload, + .. + } = invocation; + + let ToolPayload::LocalShell { params } = payload else { + return Err(FunctionCallError::RespondToModel( + "unsupported payload for local_shell handler".to_string(), + )); + }; + + let exec_params = ShellHandler::to_exec_params(¶ms, turn.as_ref(), session.conversation_id); + ShellHandler::run_exec_like(RunExecLikeArgs { + tool_name: "local_shell".to_string(), + exec_params, + hook_command: codex_shell_command::parse_command::shlex_join(¶ms.command), + additional_permissions: None, + prefix_rule: None, + session, + turn, + tracker, + call_id, + freeform: false, + shell_runtime_backend: ShellRuntimeBackend::Generic, + initial_approval_state, + }) + .await } fn shell_function_pre_tool_use_payload(invocation: &ToolInvocation) -> Option { @@ -614,54 +730,16 @@ impl ToolHandler for ShellCommandHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { - session, - turn, - tracker, - call_id, - payload, - .. - } = invocation; + self.handle_with_permission_request_hooks(invocation, InitialApprovalState::Evaluate) + .await + } - let ToolPayload::Function { arguments } = payload else { - return Err(FunctionCallError::RespondToModel(format!( - "unsupported payload for shell_command handler: {}", - self.tool_name().display() - ))); - }; - - let cwd = resolve_workdir_base_path(&arguments, &turn.cwd)?; - let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; - let workdir = turn.resolve_path(params.workdir.clone()); - maybe_emit_implicit_skill_invocation( - session.as_ref(), - turn.as_ref(), - ¶ms.command, - &workdir, - ) - .await; - let prefix_rule = params.prefix_rule.clone(); - let exec_params = Self::to_exec_params( - ¶ms, - session.as_ref(), - turn.as_ref(), - session.conversation_id, - turn.tools_config.allow_login_shell, - )?; - ShellHandler::run_exec_like(RunExecLikeArgs { - tool_name: self.tool_name().display(), - exec_params, - hook_command: params.command, - additional_permissions: params.additional_permissions.clone(), - prefix_rule, - session, - turn, - tracker, - call_id, - freeform: true, - shell_runtime_backend: self.shell_runtime_backend(), - }) - .await + async fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> Result { + self.handle_with_permission_request_hooks(invocation, InitialApprovalState::AlreadyApproved) + .await } } @@ -679,6 +757,7 @@ impl ShellHandler { call_id, freeform, shell_runtime_backend, + initial_approval_state, } = args; let mut exec_params = exec_params; @@ -841,6 +920,7 @@ impl ShellHandler { &tool_ctx, &turn, turn.approval_policy.value(), + initial_approval_state, ) .await .map(|result| result.output); diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index f3c5ce9966..70107aca93 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -21,6 +21,7 @@ use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; +use crate::tools::sandboxing::InitialApprovalState; use crate::unified_exec::ExecCommandRequest; use crate::unified_exec::UnifiedExecContext; use crate::unified_exec::UnifiedExecError; @@ -196,208 +197,222 @@ impl ToolHandler for ExecCommandHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { - session, - turn, - tracker, - call_id, - payload, - .. - } = invocation; + handle_exec_command_invocation(invocation, InitialApprovalState::Evaluate).await + } - let arguments = match payload { - ToolPayload::Function { arguments } => arguments, - _ => { - return Err(FunctionCallError::RespondToModel( - "exec_command handler received unsupported payload".to_string(), - )); - } - }; + async fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> Result { + handle_exec_command_invocation(invocation, InitialApprovalState::AlreadyApproved).await + } +} - let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager; - let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone()); - let environment_args: ExecCommandEnvironmentArgs = parse_arguments(&arguments)?; - let Some(turn_environment) = - resolve_tool_environment(turn.as_ref(), environment_args.environment_id.as_deref())? - else { +async fn handle_exec_command_invocation( + invocation: ToolInvocation, + initial_approval_state: InitialApprovalState, +) -> Result { + let ToolInvocation { + session, + turn, + tracker, + call_id, + payload, + .. + } = invocation; + + let arguments = match payload { + ToolPayload::Function { arguments } => arguments, + _ => { return Err(FunctionCallError::RespondToModel( - "unified exec is unavailable in this session".to_string(), + "exec_command handler received unsupported payload".to_string(), )); - }; - let cwd = environment_args - .workdir - .as_deref() - .filter(|workdir| !workdir.is_empty()) - .map_or_else( - || turn_environment.cwd.clone(), - |workdir| turn_environment.cwd.join(workdir), - ); - let environment = Arc::clone(&turn_environment.environment); - let fs = environment.get_filesystem(); - let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; - let hook_command = args.cmd.clone(); - maybe_emit_implicit_skill_invocation( - session.as_ref(), - context.turn.as_ref(), - &hook_command, - &cwd, - ) - .await; - let process_id = manager.allocate_process_id().await; - let command = get_command( - &args, - session.user_shell(), - &turn.tools_config.unified_exec_shell_mode, - turn.tools_config.allow_login_shell, - ) - .map_err(FunctionCallError::RespondToModel)?; - let command_for_display = codex_shell_command::parse_command::shlex_join(&command); - - let ExecCommandArgs { - tty, - yield_time_ms, - max_output_tokens, - sandbox_permissions, - additional_permissions, - justification, - prefix_rule, - .. - } = args; - let max_output_tokens = - effective_max_output_tokens(max_output_tokens, turn.truncation_policy); - - let exec_permission_approvals_enabled = - session.features().enabled(Feature::ExecPermissionApprovals); - let requested_additional_permissions = additional_permissions.clone(); - let effective_additional_permissions = apply_granted_turn_permissions( - context.session.as_ref(), - cwd.as_path(), - sandbox_permissions, - additional_permissions, - ) - .await; - let additional_permissions_allowed = exec_permission_approvals_enabled - || (session.features().enabled(Feature::RequestPermissionsTool) - && effective_additional_permissions.permissions_preapproved); - - // Sticky turn permissions have already been approved, so they should - // continue through the normal exec approval flow for the command. - if effective_additional_permissions - .sandbox_permissions - .requests_sandbox_override() - && !effective_additional_permissions.permissions_preapproved - && !matches!( - context.turn.approval_policy.value(), - codex_protocol::protocol::AskForApproval::OnRequest - ) - { - let approval_policy = context.turn.approval_policy.value(); - manager.release_process_id(process_id).await; - return Err(FunctionCallError::RespondToModel(format!( - "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" - ))); } + }; - let normalized_additional_permissions = match implicit_granted_permissions( - sandbox_permissions, - requested_additional_permissions.as_ref(), - &effective_additional_permissions, - ) + let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager; + let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone()); + let environment_args: ExecCommandEnvironmentArgs = parse_arguments(&arguments)?; + let Some(turn_environment) = + resolve_tool_environment(turn.as_ref(), environment_args.environment_id.as_deref())? + else { + return Err(FunctionCallError::RespondToModel( + "unified exec is unavailable in this session".to_string(), + )); + }; + let cwd = environment_args + .workdir + .as_deref() + .filter(|workdir| !workdir.is_empty()) .map_or_else( - || { - normalize_and_validate_additional_permissions( - additional_permissions_allowed, - context.turn.approval_policy.value(), - effective_additional_permissions.sandbox_permissions, - effective_additional_permissions.additional_permissions, - effective_additional_permissions.permissions_preapproved, - &cwd, - ) - }, - |permissions| Ok(Some(permissions)), - ) { - Ok(normalized) => normalized, - Err(err) => { - manager.release_process_id(process_id).await; - return Err(FunctionCallError::RespondToModel(err)); - } - }; + || turn_environment.cwd.clone(), + |workdir| turn_environment.cwd.join(workdir), + ); + let environment = Arc::clone(&turn_environment.environment); + let fs = environment.get_filesystem(); + let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; + let hook_command = args.cmd.clone(); + maybe_emit_implicit_skill_invocation( + session.as_ref(), + context.turn.as_ref(), + &hook_command, + &cwd, + ) + .await; + let process_id = manager.allocate_process_id().await; + let command = get_command( + &args, + session.user_shell(), + &turn.tools_config.unified_exec_shell_mode, + turn.tools_config.allow_login_shell, + ) + .map_err(FunctionCallError::RespondToModel)?; + let command_for_display = codex_shell_command::parse_command::shlex_join(&command); - if let Some(output) = intercept_apply_patch( - &command, - &cwd, - fs.as_ref(), - context.session.clone(), - context.turn.clone(), - Some(&tracker), - &context.call_id, - "exec_command", + let ExecCommandArgs { + tty, + yield_time_ms, + max_output_tokens, + sandbox_permissions, + additional_permissions, + justification, + prefix_rule, + .. + } = args; + let max_output_tokens = effective_max_output_tokens(max_output_tokens, turn.truncation_policy); + + let exec_permission_approvals_enabled = + session.features().enabled(Feature::ExecPermissionApprovals); + let requested_additional_permissions = additional_permissions.clone(); + let effective_additional_permissions = apply_granted_turn_permissions( + context.session.as_ref(), + cwd.as_path(), + sandbox_permissions, + additional_permissions, + ) + .await; + let additional_permissions_allowed = exec_permission_approvals_enabled + || (session.features().enabled(Feature::RequestPermissionsTool) + && effective_additional_permissions.permissions_preapproved); + + // Sticky turn permissions have already been approved, so they should + // continue through the normal exec approval flow for the command. + if effective_additional_permissions + .sandbox_permissions + .requests_sandbox_override() + && !effective_additional_permissions.permissions_preapproved + && !matches!( + context.turn.approval_policy.value(), + codex_protocol::protocol::AskForApproval::OnRequest ) - .await? - { - manager.release_process_id(process_id).await; - return Ok(ExecCommandToolOutput { - event_call_id: String::new(), - chunk_id: String::new(), - wall_time: std::time::Duration::ZERO, - raw_output: output.into_text().into_bytes(), - max_output_tokens: Some(max_output_tokens), - process_id: None, - exit_code: None, - original_token_count: None, - hook_command: None, - }); - } + { + let approval_policy = context.turn.approval_policy.value(); + manager.release_process_id(process_id).await; + return Err(FunctionCallError::RespondToModel(format!( + "approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}" + ))); + } - emit_unified_exec_tty_metric(&turn.session_telemetry, tty); - match manager - .exec_command( - ExecCommandRequest { - command, - hook_command: hook_command.clone(), - process_id, - yield_time_ms, - max_output_tokens: Some(max_output_tokens), - cwd, - environment, - network: context.turn.network.clone(), - tty, - sandbox_permissions: effective_additional_permissions.sandbox_permissions, - additional_permissions: normalized_additional_permissions, - additional_permissions_preapproved: effective_additional_permissions - .permissions_preapproved, - justification, - prefix_rule, - }, - &context, + let normalized_additional_permissions = match implicit_granted_permissions( + sandbox_permissions, + requested_additional_permissions.as_ref(), + &effective_additional_permissions, + ) + .map_or_else( + || { + normalize_and_validate_additional_permissions( + additional_permissions_allowed, + context.turn.approval_policy.value(), + effective_additional_permissions.sandbox_permissions, + effective_additional_permissions.additional_permissions, + effective_additional_permissions.permissions_preapproved, + &cwd, ) - .await - { - Ok(response) => Ok(response), - Err(UnifiedExecError::UpdatedInput(updated_input)) => { - Err(FunctionCallError::UpdatedInput(updated_input)) - } - Err(UnifiedExecError::SandboxDenied { output, .. }) => { - let output_text = output.aggregated_output.text; - let original_token_count = approx_token_count(&output_text); - Ok(ExecCommandToolOutput { - event_call_id: context.call_id.clone(), - chunk_id: generate_chunk_id(), - wall_time: output.duration, - raw_output: output_text.into_bytes(), - max_output_tokens: Some(max_output_tokens), - // Sandbox denial is terminal, so there is no live - // process for write_stdin to resume. - process_id: None, - exit_code: Some(output.exit_code), - original_token_count: Some(original_token_count), - hook_command: Some(hook_command), - }) - } - Err(err) => Err(FunctionCallError::RespondToModel(format!( - "exec_command failed for `{command_for_display}`: {err:?}" - ))), + }, + |permissions| Ok(Some(permissions)), + ) { + Ok(normalized) => normalized, + Err(err) => { + manager.release_process_id(process_id).await; + return Err(FunctionCallError::RespondToModel(err)); } + }; + + if let Some(output) = intercept_apply_patch( + &command, + &cwd, + fs.as_ref(), + context.session.clone(), + context.turn.clone(), + Some(&tracker), + &context.call_id, + "exec_command", + ) + .await? + { + manager.release_process_id(process_id).await; + return Ok(ExecCommandToolOutput { + event_call_id: String::new(), + chunk_id: String::new(), + wall_time: std::time::Duration::ZERO, + raw_output: output.into_text().into_bytes(), + max_output_tokens: Some(max_output_tokens), + process_id: None, + exit_code: None, + original_token_count: None, + hook_command: None, + }); + } + + emit_unified_exec_tty_metric(&turn.session_telemetry, tty); + match manager + .exec_command( + ExecCommandRequest { + command, + hook_command: hook_command.clone(), + process_id, + yield_time_ms, + max_output_tokens: Some(max_output_tokens), + cwd, + environment, + network: context.turn.network.clone(), + tty, + sandbox_permissions: effective_additional_permissions.sandbox_permissions, + additional_permissions: normalized_additional_permissions, + additional_permissions_preapproved: effective_additional_permissions + .permissions_preapproved, + justification, + prefix_rule, + initial_approval_state, + }, + &context, + ) + .await + { + Ok(response) => Ok(response), + Err(UnifiedExecError::UpdatedInput(updated_input)) => { + Err(FunctionCallError::UpdatedInput(updated_input)) + } + Err(UnifiedExecError::SandboxDenied { output, .. }) => { + let output_text = output.aggregated_output.text; + let original_token_count = approx_token_count(&output_text); + Ok(ExecCommandToolOutput { + event_call_id: context.call_id.clone(), + chunk_id: generate_chunk_id(), + wall_time: output.duration, + raw_output: output_text.into_bytes(), + max_output_tokens: Some(max_output_tokens), + // Sandbox denial is terminal, so there is no live + // process for write_stdin to resume. + process_id: None, + exit_code: Some(output.exit_code), + original_token_count: Some(original_token_count), + hook_command: Some(hook_command), + }) + } + Err(err) => Err(FunctionCallError::RespondToModel(format!( + "exec_command failed for `{command_for_display}`: {err:?}" + ))), } } diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index f438da301b..3ebc738f42 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -20,6 +20,7 @@ use crate::tools::network_approval::finish_deferred_network_approval; use crate::tools::network_approval::finish_immediate_network_approval; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; +use crate::tools::sandboxing::InitialApprovalState; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; use crate::tools::sandboxing::ToolCtx; @@ -130,6 +131,7 @@ impl ToolOrchestrator { tool_ctx: &ToolCtx, turn_ctx: &crate::session::turn_context::TurnContext, approval_policy: AskForApproval, + initial_approval_state: InitialApprovalState, ) -> Result, ToolError> where T: ToolRuntime, @@ -141,23 +143,66 @@ impl ToolOrchestrator { let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review; // 1) Approval - let mut already_approved = false; + let mut already_approved = matches!( + initial_approval_state, + InitialApprovalState::AlreadyApproved + ); 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()); + if !already_approved { + 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 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, + guardian_review_id.as_deref(), + decision, + ) + .await?; + already_approved = true; + } else { + otel.tool_decision( + otel_tn, + otel_ci, + &ReviewDecision::Approved, + ToolDecisionSource::Config, + ); + } + } + ExecApprovalRequirement::Forbidden { reason } => { + return Err(ToolError::Rejected(reason)); + } + ExecApprovalRequirement::NeedsApproval { reason, .. } => { + let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, guardian_review_id: guardian_review_id.clone(), - retry_reason: None, + retry_reason: reason, network_approval_context: None, }; let decision = Self::request_approval( @@ -166,50 +211,16 @@ impl ToolOrchestrator { tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ false, + /*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; - } else { - otel.tool_decision( - otel_tn, - otel_ci, - &ReviewDecision::Approved, - ToolDecisionSource::Config, - ); } } - ExecApprovalRequirement::Forbidden { reason } => { - return Err(ToolError::Rejected(reason)); - } - ExecApprovalRequirement::NeedsApproval { reason, .. } => { - let guardian_review_id = use_guardian.then(new_guardian_review_id); - let approval_ctx = ApprovalCtx { - session: &tool_ctx.session, - turn: &tool_ctx.turn, - call_id: &tool_ctx.call_id, - guardian_review_id: guardian_review_id.clone(), - retry_reason: reason, - network_approval_context: None, - }; - let decision = Self::request_approval( - tool, - req, - tool_ctx.call_id.as_str(), - approval_ctx, - tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, - &otel, - ) - .await?; - - Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) - .await?; - already_approved = true; - } } // 2) First attempt under the selected sandbox. @@ -398,7 +409,6 @@ impl ToolOrchestrator { if evaluate_permission_request_hooks && let Some(permission_request) = tool.permission_request_payload(req) { - let permission_request_for_noop_check = permission_request.clone(); match run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, @@ -410,17 +420,7 @@ impl ToolOrchestrator { Some(PermissionRequestDecision::Allow { updated_input: Some(updated_input), }) => { - if !permission_request_for_noop_check.updated_input_is_noop(&updated_input) { - return Err(ToolError::UpdatedInput(updated_input)); - } - let decision = ReviewDecision::Approved; - otel.tool_decision( - &tool_ctx.tool_name, - &tool_ctx.call_id, - &decision, - ToolDecisionSource::Config, - ); - return Ok(decision); + return Err(ToolError::UpdatedInput(updated_input)); } Some(PermissionRequestDecision::Allow { updated_input: None, diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 02e418cedc..5ce7a24496 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -5,7 +5,6 @@ use std::time::Instant; use crate::function_tool::FunctionCallError; use crate::goals::GoalRuntimeEvent; -use crate::hook_runtime::MAX_HOOK_INPUT_REWRITES; use crate::hook_runtime::record_additional_contexts; use crate::hook_runtime::run_post_tool_use_hooks; use crate::hook_runtime::run_pre_tool_use_hooks; @@ -107,6 +106,18 @@ pub trait ToolHandler: Send + Sync { &self, invocation: ToolInvocation, ) -> impl std::future::Future> + Send; + + /// Continue execution after a `PermissionRequest` hook rewrites input. + /// + /// Implementations that route through approval-time hooks should override + /// this to reuse the hook verdict instead of evaluating `PermissionRequest` + /// again for the rewritten payload. + fn handle_after_permission_request_rewrite( + &self, + invocation: ToolInvocation, + ) -> impl std::future::Future> + Send { + self.handle(invocation) + } } /// Consumes streamed argument diffs for a tool call and emits protocol events @@ -230,21 +241,14 @@ where ) -> BoxFuture<'a, Result> { Box::pin(async move { let mut invocation = invocation; - let mut rewrites = 0; - let output = loop { - match self.handle(invocation.clone()).await { - Err(FunctionCallError::UpdatedInput(updated_input)) => { - rewrites += 1; - if rewrites > MAX_HOOK_INPUT_REWRITES { - return Err(FunctionCallError::RespondToModel( - "hook input rewrite limit exceeded".to_string(), - )); - } - invocation = - ToolHandler::with_updated_hook_input(self, invocation, updated_input)?; - } - result => break result?, + let output = match self.handle(invocation.clone()).await { + Err(FunctionCallError::UpdatedInput(updated_input)) => { + invocation = + ToolHandler::with_updated_hook_input(self, invocation, updated_input)?; + ToolHandler::handle_after_permission_request_rewrite(self, invocation.clone()) + .await? } + result => result?, }; let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 2c139a027c..6b3a1cbdad 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -154,14 +154,6 @@ impl PermissionRequestPayload { tool_input: serde_json::Value::Object(tool_input), } } - - pub(crate) fn updated_input_is_noop(&self, updated_input: &serde_json::Value) -> bool { - if self.tool_name.name() == "Bash" { - return self.tool_input.get("command") == updated_input.get("command"); - } - - self.tool_input == *updated_input - } } // Specifies what tool orchestrator should do with a given tool call. @@ -355,6 +347,12 @@ pub(crate) struct ToolCtx { pub tool_name: String, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum InitialApprovalState { + Evaluate, + AlreadyApproved, +} + #[derive(Debug)] pub(crate) enum ToolError { Rejected(String), diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index f2fb189137..19eb7a67d9 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -34,30 +34,6 @@ fn bash_permission_request_payload_includes_description_when_present() { ); } -#[test] -fn bash_updated_input_noop_ignores_description() { - let payload = PermissionRequestPayload::bash( - "echo hi".to_string(), - Some("network-access example.com".to_string()), - ); - - assert!(payload.updated_input_is_noop(&json!({ "command": "echo hi" }))); -} - -#[test] -fn non_bash_updated_input_noop_requires_full_equality() { - let payload = PermissionRequestPayload { - tool_name: HookToolName::apply_patch(), - tool_input: json!({ "command": "patch" }), - }; - - assert!(payload.updated_input_is_noop(&json!({ "command": "patch" }))); - assert!(!payload.updated_input_is_noop(&json!({ - "command": "patch", - "description": "ignored" - }))); -} - #[test] fn external_sandbox_skips_exec_approval_on_request() { let sandbox_policy = SandboxPolicy::ExternalSandbox { diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 9b74baf64c..04e16b3538 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -39,6 +39,7 @@ use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::network_approval::DeferredNetworkApproval; +use crate::tools::sandboxing::InitialApprovalState; mod async_watcher; mod errors; @@ -103,6 +104,7 @@ pub(crate) struct ExecCommandRequest { pub additional_permissions_preapproved: bool, pub justification: Option, pub prefix_rule: Option>, + pub initial_approval_state: InitialApprovalState, } #[derive(Debug)] diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 9a8bb85c4f..a39c9e9d0e 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1052,6 +1052,7 @@ impl UnifiedExecProcessManager { &tool_ctx, &context.turn, context.turn.approval_policy.value(), + request.initial_approval_state, ) .await .map(|result| (result.output, result.deferred_network_approval)) 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 bde32c9ab4..4be1f34035 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::tools::sandboxing::InitialApprovalState; use pretty_assertions::assert_eq; use tokio::time::Duration; use tokio::time::Instant; @@ -187,6 +188,7 @@ async fn failed_initial_end_for_unstored_process_uses_fallback_output() { additional_permissions_preapproved: false, justification: None, prefix_rule: None, + initial_approval_state: InitialApprovalState::Evaluate, }; let transcript = Arc::new(tokio::sync::Mutex::new(HeadTailBuffer::default())); diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index ec16367ba4..f5efc2cfcd 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -1838,19 +1838,13 @@ async fn permission_request_hook_rewrites_apply_patch_before_execution() -> Resu ); let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; - assert_eq!(hook_inputs.len(), 2); + assert_eq!(hook_inputs.len(), 1); assert_permission_request_hook_input( &hook_inputs[0], "apply_patch", &original_patch, /*description*/ None, ); - assert_permission_request_hook_input( - &hook_inputs[1], - "apply_patch", - &rewritten_patch, - /*description*/ None, - ); Ok(()) } diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 133461a26f..c3cce0f4ae 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 +//! replacing the full tool input object on allow, or decline to decide and let +//! the normal approval flow continue. //! //! The event also mirrors the rest of the hook system's lifecycle: //!