diff --git a/codex-rs/core/src/function_tool.rs b/codex-rs/core/src/function_tool.rs index 240e04361c..5fd6639958 100644 --- a/codex-rs/core/src/function_tool.rs +++ b/codex-rs/core/src/function_tool.rs @@ -4,6 +4,8 @@ use thiserror::Error; pub enum FunctionCallError { #[error("{0}")] RespondToModel(String), + #[error("tool input rewritten by hook")] + UpdatedInput(serde_json::Value), #[error("LocalShellCall without call_id or id")] MissingLocalShellCallId, #[error("Fatal error: {0}")] diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 8b6b28cfc3..d34f91a03a 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -48,6 +48,8 @@ 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 58f26cb25e..117eca7c5c 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -25,6 +25,7 @@ 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; @@ -196,18 +197,45 @@ pub(crate) async fn handle_mcp_tool_call( ) .await; - 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 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; + }; + 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; + } + } McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember => { @@ -274,8 +302,7 @@ pub(crate) async fn handle_mcp_tool_call( return HandledMcpToolCall { result: CallToolResult::from_result(result), - tool_input: arguments_value - .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), + tool_input, }; } @@ -814,6 +841,7 @@ async fn maybe_track_codex_app_used( #[derive(Debug, Clone, PartialEq, Eq)] enum McpToolApprovalDecision { Accept, + AcceptWithUpdatedInput(JsonValue), AcceptForSession, AcceptAndRemember, Decline { message: Option }, @@ -1071,21 +1099,34 @@ async fn maybe_request_mcp_tool_approval( return Some(McpToolApprovalDecision::Accept); } + let permission_request_tool_input = invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())); 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())), + tool_input: permission_request_tool_input.clone(), }, ) .await { - Some(PermissionRequestDecision::Allow) => { + Some(PermissionRequestDecision::Allow { + updated_input: Some(updated_input), + }) => { + if updated_input == permission_request_tool_input { + return Some(McpToolApprovalDecision::Accept); + } + return Some(McpToolApprovalDecision::AcceptWithUpdatedInput( + updated_input, + )); + } + Some(PermissionRequestDecision::Allow { + updated_input: None, + }) => { return Some(McpToolApprovalDecision::Accept); } Some(PermissionRequestDecision::Deny { message }) => { @@ -1839,6 +1880,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 3d81f05c72..37d1c7f4f0 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2082,6 +2082,80 @@ async fn permission_request_hook_allows_mcp_tool_call() { ); } +#[tokio::test] +async fn permission_request_hook_can_update_mcp_tool_input() { + let (mut session, turn_context) = make_session_and_context().await; + install_mcp_permission_request_hook( + &mut session, + &turn_context, + "mcp__memory__.*", + &serde_json::json!({ + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow", + "updatedInput": { + "entities": [{ + "name": "Grace", + "entityType": "person" + }] + } + } + } + }), + ); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: "memory".to_string(), + tool: "create_entities".to_string(), + arguments: Some(serde_json::json!({ + "entities": [{ + "name": "Ada", + "entityType": "person" + }] + })), + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations( + Some(false), + Some(true), + /*open_world*/ None, + )), + connector_id: None, + connector_name: None, + connector_description: None, + tool_title: Some("Create entities".to_string()), + tool_description: None, + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + }; + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-mcp-hook", + &invocation, + "mcp__memory__create_entities", + Some(&metadata), + AppToolApproval::Auto, + ) + .await; + + assert_eq!( + decision, + Some(McpToolApprovalDecision::AcceptWithUpdatedInput( + serde_json::json!({ + "entities": [{ + "name": "Grace", + "entityType": "person" + }] + }) + )) + ); +} + #[tokio::test] async fn permission_request_hook_uses_hook_tool_name_without_metadata() { let (mut session, turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/stream_events_utils.rs b/codex-rs/core/src/stream_events_utils.rs index fc608aed93..00dc66df20 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -336,6 +336,11 @@ pub(crate) async fn handle_output_item_done( output.needs_follow_up = true; } + Err(FunctionCallError::UpdatedInput(_)) => { + return Err(CodexErr::Fatal( + "updated tool input escaped dispatch".to_string(), + )); + } // A fatal error occurred; surface it back into history. Err(FunctionCallError::Fatal(message)) => { return Err(CodexErr::Fatal(message)); diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index f97e51fd8a..a618c8d59a 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -353,6 +353,9 @@ impl ToolEmitter { let result = Err(FunctionCallError::RespondToModel(normalized)); (event, result) } + Err(ToolError::UpdatedInput(updated_input)) => { + return Err(FunctionCallError::UpdatedInput(updated_input)); + } }; self.emit(ctx, event).await; result diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 2c19c6b6c7..96d5beaa97 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -372,6 +372,9 @@ impl ToolHandler for ExecCommandHandler { .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); diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 14af2c9c5f..c77dcad54b 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -473,7 +473,29 @@ impl NetworkApprovalService { .await { match permission_request_decision { - PermissionRequestDecision::Allow => { + PermissionRequestDecision::Allow { + updated_input: Some(_), + } => { + if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::DeniedByPolicy( + "updatedInput is not supported for network approval requests" + .to_string(), + ), + ) + .await; + } + pending.set_decision(PendingApprovalDecision::Deny).await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + return NetworkDecision::deny( + "updatedInput is not supported for network approval requests", + ); + } + PermissionRequestDecision::Allow { + updated_input: None, + } => { pending .set_decision(PendingApprovalDecision::AllowOnce) .await; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index c618d778d6..f438da301b 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -398,6 +398,7 @@ 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, @@ -406,7 +407,24 @@ impl ToolOrchestrator { ) .await { - Some(PermissionRequestDecision::Allow) => { + 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); + } + Some(PermissionRequestDecision::Allow { + updated_input: None, + }) => { let decision = ReviewDecision::Approved; otel.tool_decision( &tool_ctx.tool_name, diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 82ee5efd3a..1735bf8daa 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -5,6 +5,7 @@ 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; @@ -228,7 +229,23 @@ where invocation: ToolInvocation, ) -> BoxFuture<'a, Result> { Box::pin(async move { - let output = self.handle(invocation.clone()).await?; + 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 call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); let post_tool_use_payload = diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index fef8db5ca9..b45c7cb7b1 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -420,7 +420,21 @@ impl CoreShellActionProvider { ) .await { - Some(PermissionRequestDecision::Allow) => { + 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::Allow { + updated_input: None, + }) => { return PromptDecision { decision: ReviewDecision::Approved, guardian_review_id: None, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index c17247beb4..2c139a027c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -154,6 +154,14 @@ 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. @@ -350,6 +358,7 @@ pub(crate) struct ToolCtx { #[derive(Debug)] pub(crate) enum ToolError { Rejected(String), + UpdatedInput(serde_json::Value), Codex(CodexErr), } diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 19eb7a67d9..f2fb189137 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -34,6 +34,30 @@ 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/errors.rs b/codex-rs/core/src/unified_exec/errors.rs index 0576e7d7cf..501503e9df 100644 --- a/codex-rs/core/src/unified_exec/errors.rs +++ b/codex-rs/core/src/unified_exec/errors.rs @@ -1,4 +1,5 @@ use codex_protocol::exec_output::ExecToolCallOutput; +use serde_json::Value; use thiserror::Error; #[derive(Debug, Error)] @@ -23,6 +24,8 @@ pub(crate) enum UnifiedExecError { message: String, output: ExecToolCallOutput, }, + #[error("tool input rewritten by hook")] + UpdatedInput(Value), } impl UnifiedExecError { diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 4f85b1ddf7..9a8bb85c4f 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -221,6 +221,9 @@ async fn finish_deferred_network_approval_for_session( fn network_approval_error_message(err: ToolError) -> String { match err { ToolError::Rejected(message) => message, + ToolError::UpdatedInput(_) => { + "updatedInput is not supported for deferred network approvals".to_string() + } ToolError::Codex(err) => err.to_string(), } } @@ -1063,6 +1066,9 @@ impl UnifiedExecProcessManager { }; UnifiedExecError::sandbox_denied(message, output) } + ToolError::UpdatedInput(updated_input) => { + UnifiedExecError::UpdatedInput(updated_input) + } other => UnifiedExecError::create_process(format!("{other:?}")), }) } diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 1c92918c9d..ec16367ba4 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -496,6 +496,56 @@ elif mode == "exit_2": Ok(()) } +fn write_updating_permission_request_hook( + home: &Path, + matcher: &str, + updated_input: &Value, +) -> Result<()> { + let script_path = home.join("permission_request_hook.py"); + let log_path = home.join("permission_request_hook_log.jsonl"); + let updated_input_json = + serde_json::to_string(updated_input).context("serialize updated permission input")?; + let script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) + +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PermissionRequest", + "decision": {{ + "behavior": "allow", + "updatedInput": {updated_input_json} + }} + }} +}})) +"#, + log_path = log_path.display(), + updated_input_json = updated_input_json, + ); + let hooks = serde_json::json!({ + "hooks": { + "PermissionRequest": [{ + "matcher": matcher, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "rewriting permission request input", + }] + }] + } + }); + + fs::write(&script_path, script).context("write updating permission hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn install_allow_permission_request_hook(home: &Path) -> Result<()> { write_permission_request_hook( home, @@ -1553,6 +1603,92 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test] +async fn permission_request_hook_rewrites_shell_command_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"); + let rewritten_marker = std::env::temp_dir().join("permissionrequest-shell-command-rewritten"); + let original_command = format!("rm -f {}", original_marker.display()); + let rewritten_command = format!("printf rewritten > {}", rewritten_marker.display()); + 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 hook rewrote it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({ "command": rewritten_command.clone() }); + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = + write_updating_permission_request_hook(home, "^Bash$", &updated_input) + { + panic!("failed to write updating permission hook fixture: {error}"); + } + }) + .with_config(|config| { + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + if original_marker.exists() { + fs::remove_file(&original_marker).context("remove stale original permission marker")?; + } + if rewritten_marker.exists() { + fs::remove_file(&rewritten_marker).context("remove stale rewritten permission marker")?; + } + fs::write(&original_marker, "seed").context("create original permission marker")?; + + test.submit_turn_with_approval_and_permission_profile( + "run the rewritten shell command after hook approval", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .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 execute after permission rewrite" + ); + assert_eq!( + fs::read_to_string(&rewritten_marker).context("read rewritten permission marker")?, + "rewritten" + ); + + let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_permission_request_hook_input( + &hook_inputs[0], + "Bash", + &original_command, + /*description*/ None, + ); + assert!(hook_inputs[0].get("tool_use_id").is_none()); + + Ok(()) +} + #[tokio::test] async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1627,6 +1763,98 @@ async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result Ok(()) } +#[tokio::test] +async fn permission_request_hook_rewrites_apply_patch_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "permissionrequest-apply-patch-rewrite"; + let original_file = "permission_request_apply_patch_original.txt"; + let rewritten_file = "permission_request_apply_patch_rewritten.txt"; + let original_path = format!("../{original_file}"); + let rewritten_path = format!("../{rewritten_file}"); + let original_patch = format!( + r#"*** Begin Patch +*** Add File: {original_path} ++original +*** End Patch"# + ); + let rewritten_patch = format!( + r#"*** Begin Patch +*** Add File: {rewritten_path} ++rewritten +*** End Patch"# + ); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_function_call(call_id, &original_patch), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "permission hook rewrote apply_patch"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({ "command": rewritten_patch.clone() }); + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = + write_updating_permission_request_hook(home, "^apply_patch$", &updated_input) + { + panic!("failed to write updating permission hook fixture: {error}"); + } + }) + .with_config(|config| { + config.include_apply_patch_tool = true; + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_approval_and_permission_profile( + "apply the rewritten patch after hook approval", + AskForApproval::OnRequest, + restrictive_workspace_write_profile(), + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + requests[1].function_call_output(call_id); + assert!( + !test.workspace_path(&original_path).exists(), + "original patch should not create its target file" + ); + assert_eq!( + fs::read_to_string(test.workspace_path(&rewritten_path)) + .context("read rewritten permission apply_patch file")?, + "rewritten\n" + ); + + let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 2); + 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(()) +} + #[tokio::test] 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 21d45382b0..5b46925568 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": "Replaces the entire tool input object when `behavior` is `allow`." }, "updatedPermissions": { "default": null, diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 9c3d442dac..53e9b52ab3 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,7 +319,9 @@ fn unsupported_permission_request_hook_specific_output( decision: Option<&PermissionRequestDecisionWire>, ) -> Option { let decision = decision?; - if decision.updated_input.is_some() { + if decision.updated_input.is_some() + && !matches!(decision.behavior, PermissionRequestBehaviorWire::Allow) + { Some("PermissionRequest hook returned unsupported updatedInput".to_string()) } else if decision.updated_permissions.is_some() { Some("PermissionRequest hook returned unsupported updatedPermissions".to_string()) @@ -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_allow_updated_input_field() { let parsed = parse_permission_request( &json!({ "continue": true, @@ -453,6 +461,31 @@ mod tests { ) .expect("permission request hook output should parse"); + assert_eq!( + parsed.decision, + Some(super::PermissionRequestDecision::Allow { + updated_input: Some(json!({})), + }) + ); + } + + #[test] + fn permission_request_rejects_deny_updated_input_field() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "deny", + "updatedInput": {} + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + assert_eq!( parsed.invalid_reason, Some("PermissionRequest hook returned unsupported updatedInput".to_string()) diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 2cebd0e002..133461a26f 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -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,38 @@ 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: None, + }, ]; assert_eq!( resolve_permission_request_decision(decisions.iter()), - Some(PermissionRequestDecision::Allow) + Some(PermissionRequestDecision::Allow { + updated_input: None, + }) + ); + } + + #[test] + fn permission_request_keeps_highest_precedence_allow_update() { + let decisions = [ + PermissionRequestDecision::Allow { + updated_input: Some(serde_json::json!({ "command": "first" })), + }, + PermissionRequestDecision::Allow { + updated_input: Some(serde_json::json!({ "command": "second" })), + }, + ]; + + assert_eq!( + resolve_permission_request_decision(decisions.iter()), + Some(PermissionRequestDecision::Allow { + updated_input: Some(serde_json::json!({ "command": "second" })), + }) ); } diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index d08cce6ee2..7365529415 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -138,9 +138,7 @@ 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. + /// Replaces the entire tool input object when `behavior` is `allow`. #[serde(default)] pub updated_input: Option, /// Reserved for a future permission-rewrite capability.