From 61724ae0832376f0d55f51751562a4dbd31570a3 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 30 Apr 2026 16:34:32 -0700 Subject: [PATCH 01/20] support hook input rewrites --- codex-rs/core/src/function_tool.rs | 2 + codex-rs/core/src/hook_runtime.rs | 41 +- codex-rs/core/src/mcp_tool_call.rs | 77 ++- codex-rs/core/src/mcp_tool_call_tests.rs | 74 +++ codex-rs/core/src/stream_events_utils.rs | 5 + codex-rs/core/src/tools/events.rs | 7 + .../core/src/tools/handlers/apply_patch.rs | 41 ++ codex-rs/core/src/tools/handlers/mcp.rs | 20 + codex-rs/core/src/tools/handlers/shell.rs | 85 ++++ .../core/src/tools/handlers/unified_exec.rs | 41 ++ codex-rs/core/src/tools/network_approval.rs | 14 +- codex-rs/core/src/tools/orchestrator.rs | 20 +- codex-rs/core/src/tools/registry.rs | 71 ++- .../tools/runtimes/shell/unix_escalation.rs | 16 +- codex-rs/core/src/tools/sandboxing.rs | 1 + codex-rs/core/src/unified_exec/errors.rs | 3 + .../core/src/unified_exec/process_manager.rs | 6 + codex-rs/core/tests/suite/hooks.rs | 438 ++++++++++++++++++ codex-rs/core/tests/suite/hooks_mcp.rs | 114 +++++ ...mission-request.command.output.schema.json | 2 +- codex-rs/hooks/src/engine/output_parser.rs | 70 ++- .../hooks/src/events/permission_request.rs | 47 +- codex-rs/hooks/src/events/pre_tool_use.rs | 49 ++ codex-rs/hooks/src/schema.rs | 4 +- 24 files changed, 1182 insertions(+), 66 deletions(-) diff --git a/codex-rs/core/src/function_tool.rs b/codex-rs/core/src/function_tool.rs index 240e04361cd4..5fd6639958eb 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 9a9285451521..bed525246452 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -43,6 +43,11 @@ pub(crate) struct HookRuntimeOutcome { pub additional_contexts: Vec, } +pub(crate) enum PreToolUseHookResult { + Continue { updated_input: Option }, + Blocked(String), +} + pub(crate) enum PendingInputHookDisposition { Accepted(Box), Blocked { additional_contexts: Vec }, @@ -140,7 +145,7 @@ pub(crate) async fn run_pre_tool_use_hooks( tool_use_id: String, tool_name: &HookToolName, tool_input: &Value, -) -> Option { +) -> PreToolUseHookResult { let request = PreToolUseRequest { session_id: sess.conversation_id, turn_id: turn_context.sub_id.clone(), @@ -161,24 +166,32 @@ pub(crate) async fn run_pre_tool_use_hooks( hook_events, should_block, block_reason, + updated_input, } = hooks.run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; if should_block { - block_reason.map(|reason| { - if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch") - && let Some(command) = tool_input.get("command").and_then(Value::as_str) - { - format!("Command blocked by PreToolUse hook: {reason}. Command: {command}") - } else { - format!( - "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", - tool_name.name() - ) - } - }) + block_reason.map_or( + PreToolUseHookResult::Continue { + updated_input: None, + }, + |reason| { + if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch") + && let Some(command) = tool_input.get("command").and_then(Value::as_str) + { + PreToolUseHookResult::Blocked(format!( + "Command blocked by PreToolUse hook: {reason}. Command: {command}" + )) + } else { + PreToolUseHookResult::Blocked(format!( + "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", + tool_name.name() + )) + } + }, + ) } else { - None + PreToolUseHookResult::Continue { updated_input } } } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index ed8451e352fb..34f37a98aa8a 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -192,18 +192,45 @@ pub(crate) async fn handle_mcp_tool_call( }); notify_mcp_tool_call_event(sess.as_ref(), turn_context.as_ref(), tool_call_begin_event).await; - if let Some(decision) = maybe_request_mcp_tool_approval( - &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 > 8 { + 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 => { @@ -270,8 +297,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, }; } @@ -747,6 +773,7 @@ async fn maybe_track_codex_app_used( #[derive(Debug, Clone, PartialEq, Eq)] enum McpToolApprovalDecision { Accept, + AcceptWithUpdatedInput(JsonValue), AcceptForSession, AcceptAndRemember, Decline { message: Option }, @@ -1000,21 +1027,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 }) => { @@ -1768,6 +1808,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 6a14126681bd..a04088306951 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2069,6 +2069,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 5a31d180201a..328bce0c141f 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 2b215a043d2e..fabc9a8141ec 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -353,6 +353,13 @@ impl ToolEmitter { let result = Err(FunctionCallError::RespondToModel(normalized)); (event, result) } + Err(ToolError::UpdatedInput(updated_input)) => { + let event = ToolEventStage::Failure(ToolEventFailure::Message( + "tool input rewritten by hook".to_string(), + )); + let result = Err(FunctionCallError::UpdatedInput(updated_input)); + (event, result) + } }; self.emit(ctx, event).await; result diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index d71eb7931a35..71a40a42114f 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -318,6 +318,47 @@ impl ToolHandler for ApplyPatchHandler { }) } + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + let patch = updated_input + .get("command") + .and_then(serde_json::Value::as_str) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned updatedInput without string field `command`".to_string(), + ) + })?; + invocation.payload = match invocation.payload { + ToolPayload::Function { arguments } => { + let mut arguments: serde_json::Value = parse_arguments(&arguments)?; + let serde_json::Value::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel( + "apply_patch arguments must be an object".to_string(), + )); + }; + arguments.insert( + "input".to_string(), + serde_json::Value::String(patch.to_string()), + ); + ToolPayload::Function { + arguments: serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten apply_patch arguments: {err}" + )) + })?, + } + } + ToolPayload::Custom { .. } => ToolPayload::Custom { + input: patch.to_string(), + }, + payload => payload, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 568e4561583c..7bda02f7a476 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -34,6 +34,26 @@ impl ToolHandler for McpHandler { }) } + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: Value, + ) -> Result { + invocation.payload = match invocation.payload { + ToolPayload::Mcp { server, tool, .. } => ToolPayload::Mcp { + server, + tool, + raw_arguments: serde_json::to_string(&updated_input).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten MCP arguments: {err}" + )) + })?, + }, + payload => payload, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index b7512b707618..c0e3d93340c3 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -75,6 +75,17 @@ fn shell_command_payload_command(payload: &ToolPayload) -> Option { .map(|params| params.command) } +fn updated_hook_command(updated_input: &JsonValue) -> Result<&str, FunctionCallError> { + updated_input + .get("command") + .and_then(JsonValue::as_str) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned updatedInput without string field `command`".to_string(), + ) + }) +} + struct RunExecLikeArgs { tool_name: String, exec_params: ExecParams, @@ -212,6 +223,50 @@ impl ToolHandler for ShellHandler { }) } + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: JsonValue, + ) -> Result { + let command = updated_hook_command(&updated_input)?; + invocation.payload = match invocation.payload { + ToolPayload::Function { arguments } => { + let command = shlex::split(command).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + let mut arguments: JsonValue = parse_arguments(&arguments)?; + let JsonValue::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel( + "shell arguments must be an object".to_string(), + )); + }; + arguments.insert( + "command".to_string(), + JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), + ); + ToolPayload::Function { + arguments: serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten shell arguments: {err}" + )) + })?, + } + } + ToolPayload::LocalShell { mut params } => { + params.command = shlex::split(command).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + ToolPayload::LocalShell { params } + } + payload => payload, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -326,6 +381,36 @@ impl ToolHandler for ShellCommandHandler { }) } + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: JsonValue, + ) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported shell_command payload".to_string(), + )); + }; + let mut arguments: JsonValue = parse_arguments(&arguments)?; + let JsonValue::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel( + "shell_command arguments must be an object".to_string(), + )); + }; + arguments.insert( + "command".to_string(), + JsonValue::String(updated_hook_command(&updated_input)?.to_string()), + ); + invocation.payload = ToolPayload::Function { + arguments: serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten shell_command arguments: {err}" + )) + })?, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 10c8deeb3f6a..1fd8cbf0a8fc 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -152,6 +152,44 @@ impl ToolHandler for UnifiedExecHandler { }) } + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported unified_exec payload".to_string(), + )); + }; + let command = updated_input + .get("command") + .and_then(serde_json::Value::as_str) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned updatedInput without string field `command`".to_string(), + ) + })?; + let mut arguments: serde_json::Value = parse_arguments(&arguments)?; + let serde_json::Value::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel( + "exec_command arguments must be an object".to_string(), + )); + }; + arguments.insert( + "cmd".to_string(), + serde_json::Value::String(command.to_string()), + ); + invocation.payload = ToolPayload::Function { + arguments: serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten exec_command arguments: {err}" + )) + })?, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -355,6 +393,9 @@ impl ToolHandler for UnifiedExecHandler { .await { Ok(response) => response, + Err(UnifiedExecError::UpdatedInput(updated_input)) => { + return 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 14af2c9c5f3c..76d66f140b51 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -473,7 +473,19 @@ impl NetworkApprovalService { .await { match permission_request_decision { - PermissionRequestDecision::Allow => { + PermissionRequestDecision::Allow { + updated_input: Some(_), + } => { + 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 dcb42c36c6e5..05e0a530d9e3 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -397,6 +397,7 @@ impl ToolOrchestrator { if evaluate_permission_request_hooks && let Some(permission_request) = tool.permission_request_payload(req) { + let permission_request_tool_input = permission_request.tool_input.clone(); match run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, @@ -405,7 +406,24 @@ impl ToolOrchestrator { ) .await { - Some(PermissionRequestDecision::Allow) => { + Some(PermissionRequestDecision::Allow { + updated_input: Some(updated_input), + }) => { + if updated_input != permission_request_tool_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 e1027c9fa907..e51d30f85645 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -78,6 +78,20 @@ pub trait ToolHandler: Send + Sync { None } + /// Rebuilds a tool invocation from hook-facing `tool_input`. + /// + /// Tools that opt into input-rewriting hooks should invert the same stable + /// hook contract they expose from `pre_tool_use_payload`. + fn with_updated_hook_input( + &self, + _invocation: ToolInvocation, + _updated_input: Value, + ) -> Result { + Err(FunctionCallError::RespondToModel( + "tool does not support hook input rewriting".to_string(), + )) + } + /// Creates an optional consumer for streamed tool argument diffs. fn create_diff_consumer(&self) -> Option> { None @@ -166,6 +180,12 @@ trait AnyToolHandler: Send + Sync { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: Value, + ) -> Result; + fn create_diff_consumer(&self) -> Option>; fn handle_any<'a>( &'a self, @@ -189,6 +209,14 @@ where ToolHandler::pre_tool_use_payload(self, invocation) } + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: Value, + ) -> Result { + ToolHandler::with_updated_hook_input(self, invocation, updated_input) + } + fn create_diff_consumer(&self) -> Option> { ToolHandler::create_diff_consumer(self) } @@ -197,9 +225,25 @@ where invocation: ToolInvocation, ) -> 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 > 8 { + 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 output = self.handle(invocation.clone()).await?; let post_tool_use_payload = ToolHandler::post_tool_use_payload(self, &invocation, &output); Ok(AnyToolResult { @@ -264,7 +308,7 @@ impl ToolRegistry { )] pub(crate) async fn dispatch_any( &self, - invocation: ToolInvocation, + mut invocation: ToolInvocation, ) -> Result { let tool_name = invocation.tool_name.clone(); let display_name = tool_name.display(); @@ -354,8 +398,8 @@ impl ToolRegistry { return Err(err); } - if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) - && let Some(message) = run_pre_tool_use_hooks( + if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) { + match run_pre_tool_use_hooks( &invocation.session, &invocation.turn, invocation.call_id.clone(), @@ -363,10 +407,21 @@ impl ToolRegistry { &pre_tool_use_payload.tool_input, ) .await - { - let err = FunctionCallError::RespondToModel(message); - dispatch_trace.record_failed(&err); - return Err(err); + { + crate::hook_runtime::PreToolUseHookResult::Blocked(message) => { + let err = FunctionCallError::RespondToModel(message); + dispatch_trace.record_failed(&err); + return Err(err); + } + crate::hook_runtime::PreToolUseHookResult::Continue { + updated_input: Some(updated_input), + } => { + invocation = handler.with_updated_hook_input(invocation, updated_input)?; + } + crate::hook_runtime::PreToolUseHookResult::Continue { + updated_input: None, + } => {} + } } let is_mutating = handler.is_mutating(&invocation).await; 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 41aa552d896c..074b4fdd16aa 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 122cd00fad6f..f6581368cfa3 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -350,6 +350,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/unified_exec/errors.rs b/codex-rs/core/src/unified_exec/errors.rs index 0576e7d7cfd2..501503e9df98 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 c67abc48d6d9..f1d21b80ea1d 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(), } } @@ -1065,6 +1068,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 daf3053adc4d..d4aa02ee800f 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -268,6 +268,54 @@ elif mode == "exit_2": Ok(()) } +fn write_updating_pre_tool_use_hook( + home: &Path, + matcher: &str, + updated_input: &Value, +) -> Result<()> { + let script_path = home.join("pre_tool_use_hook.py"); + let log_path = home.join("pre_tool_use_hook_log.jsonl"); + let updated_input_json = + serde_json::to_string(updated_input).context("serialize updated pre tool 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": "PreToolUse", + "permissionDecision": "allow", + "updatedInput": {updated_input_json} + }} +}})) +"#, + log_path = log_path.display(), + updated_input_json = updated_input_json, + ); + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "matcher": matcher, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "rewriting pre tool input", + }] + }] + } + }); + + fs::write(&script_path, script).context("write updating pre tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn write_pre_tool_use_hook_toml( home: &Path, script_name: &str, @@ -409,6 +457,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, @@ -1374,6 +1472,95 @@ 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| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + 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(())); @@ -1451,6 +1638,101 @@ 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; + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + 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(())); @@ -1831,6 +2113,85 @@ async fn pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { Ok(()) } +#[tokio::test] +async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-shell-command-rewrite"; + let original_marker = std::env::temp_dir().join("pretooluse-shell-command-original-marker"); + let rewritten_marker = std::env::temp_dir().join("pretooluse-shell-command-rewritten-marker"); + let original_command = format!("printf original > {}", 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", "hook rewrote it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({ "command": rewritten_command }); + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_updating_pre_tool_use_hook(home, "^Bash$", &updated_input) { + panic!("failed to write updating pre tool use hook fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + if original_marker.exists() { + fs::remove_file(&original_marker).context("remove stale original pre tool marker")?; + } + if rewritten_marker.exists() { + fs::remove_file(&rewritten_marker).context("remove stale rewritten pre tool marker")?; + } + + test.submit_turn_with_permission_profile( + "run the rewritten shell command", + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + requests[1].function_call_output(call_id); + assert!( + !original_marker.exists(), + "original shell command should not execute after rewrite" + ); + assert_eq!( + fs::read_to_string(&rewritten_marker).context("read rewritten pre tool marker")?, + "rewritten" + ); + + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["tool_input"]["command"], original_command); + + Ok(()) +} + #[tokio::test] async fn plugin_pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); @@ -2417,6 +2778,83 @@ async fn pre_tool_use_blocks_apply_patch_before_execution() -> Result<()> { Ok(()) } +#[tokio::test] +async fn pre_tool_use_rewrites_apply_patch_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-apply-patch-rewrite"; + let original_file = "pre_tool_use_apply_patch_original.txt"; + let rewritten_file = "pre_tool_use_apply_patch_rewritten.txt"; + let original_patch = format!( + r#"*** Begin Patch +*** Add File: {original_file} ++original +*** End Patch"# + ); + let rewritten_patch = format!( + r#"*** Begin Patch +*** Add File: {rewritten_file} ++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", "apply_patch rewritten"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({ "command": rewritten_patch }); + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = + write_updating_pre_tool_use_hook(home, "^apply_patch$", &updated_input) + { + panic!("failed to write updating pre tool use hook fixture: {error}"); + } + }) + .with_config(|config| { + config.include_apply_patch_tool = true; + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + test.submit_turn("apply the rewritten patch").await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + requests[1].function_call_output(call_id); + assert!( + !test.workspace_path(original_file).exists(), + "original patch should not create its target file" + ); + assert_eq!( + fs::read_to_string(test.workspace_path(rewritten_file)) + .context("read rewritten apply_patch file")?, + "rewritten\n" + ); + + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["tool_input"]["command"], original_patch); + + Ok(()) +} + #[tokio::test] async fn pre_tool_use_blocks_apply_patch_with_write_alias() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 2157630e02b0..e24862b3c149 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -74,6 +74,50 @@ print(json.dumps({{ Ok(()) } +fn write_updating_pre_tool_use_hook(home: &Path, updated_message: &str) -> Result<()> { + let script_path = home.join("pre_tool_use_hook.py"); + let log_path = home.join("pre_tool_use_hook_log.jsonl"); + let updated_message_json = + serde_json::to_string(updated_message).context("serialize updated MCP message")?; + 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": "PreToolUse", + "permissionDecision": "allow", + "updatedInput": {{ "message": {updated_message_json} }} + }} +}})) +"#, + log_path = log_path.display(), + updated_message_json = updated_message_json, + ); + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "matcher": RMCP_HOOK_MATCHER, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "rewriting MCP pre tool input", + }] + }] + } + }); + + fs::write(&script_path, script).context("write updating pre tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn write_post_tool_use_hook(home: &Path, additional_context: &str) -> Result<()> { let script_path = home.join("post_tool_use_hook.py"); let log_path = home.join("post_tool_use_hook_log.jsonl"); @@ -251,6 +295,76 @@ async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_rewrites_mcp_tool_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-rmcp-echo-rewrite"; + let rewritten_message = "rewritten mcp hook input"; + let arguments = json!({ "message": RMCP_ECHO_MESSAGE }).to_string(); + let call_mock = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_function_call_with_namespace(call_id, RMCP_NAMESPACE, "echo", &arguments), + ev_completed("resp-1"), + ]), + ) + .await; + let final_mock = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "mcp pre hook rewrote it"), + ev_completed("resp-2"), + ]), + ) + .await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let test = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_updating_pre_tool_use_hook(home, rewritten_message) { + panic!("failed to write MCP updating pre tool use hook fixture: {error}"); + } + }) + .with_config(move |config| { + enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Approve); + }) + .build(&server) + .await?; + + test.submit_turn("call the rmcp echo tool with the MCP pre hook rewrite") + .await?; + + let final_request = final_mock.single_request(); + let output_item = final_request.function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("MCP tool output string"); + assert!( + output.contains(&format!("ECHOING: {rewritten_message}")), + "MCP tool should execute the rewritten input", + ); + assert!( + !output.contains(RMCP_ECHO_MESSAGE), + "MCP tool should not execute the original input", + ); + + let hook_inputs = read_hook_inputs(test.codex_home_path(), "pre_tool_use_hook_log.jsonl")?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + hook_inputs[0]["tool_input"], + json!({ "message": RMCP_ECHO_MESSAGE }), + ); + + call_mock.single_request(); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json index 21d45382b0e5..5b4692556855 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 0a3a994e19da..5d8f14882f6f 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -16,13 +16,18 @@ pub(crate) struct SessionStartOutput { pub(crate) struct PreToolUseOutput { pub universal: UniversalOutput, pub block_reason: Option, + pub updated_input: Option, pub invalid_reason: Option, } #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PermissionRequestDecision { - Allow, - Deny { message: String }, + Allow { + updated_input: Option, + }, + Deny { + message: String, + }, } #[derive(Debug, Clone)] @@ -123,10 +128,23 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { } else { None }; + let updated_input = if invalid_reason.is_none() { + hook_specific_output.and_then(|output| { + matches!( + output.permission_decision, + Some(PreToolUsePermissionDecisionWire::Allow) + ) + .then(|| output.updated_input.clone()) + .flatten() + }) + } else { + None + }; Some(PreToolUseOutput { universal, block_reason, + updated_input, invalid_reason, }) } @@ -298,7 +316,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()) @@ -313,7 +333,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 @@ -337,8 +359,13 @@ fn unsupported_post_tool_use_hook_specific_output( fn unsupported_pre_tool_use_hook_specific_output( output: &crate::schema::PreToolUseHookSpecificOutputWire, ) -> Option { - if output.updated_input.is_some() { - Some("PreToolUse hook returned unsupported updatedInput".to_string()) + if output.updated_input.is_some() + && !matches!( + output.permission_decision, + Some(PreToolUsePermissionDecisionWire::Allow) + ) + { + Some("PreToolUse hook returned updatedInput without permissionDecision:allow".to_string()) } else if output .additional_context .as_deref() @@ -348,9 +375,7 @@ fn unsupported_pre_tool_use_hook_specific_output( Some("PreToolUse hook returned unsupported additionalContext".to_string()) } else { match output.permission_decision { - Some(PreToolUsePermissionDecisionWire::Allow) => { - Some("PreToolUse hook returned unsupported permissionDecision:allow".to_string()) - } + Some(PreToolUsePermissionDecisionWire::Allow) => None, Some(PreToolUsePermissionDecisionWire::Ask) => { Some("PreToolUse hook returned unsupported permissionDecision:ask".to_string()) } @@ -424,7 +449,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, @@ -440,6 +465,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 2cebd0e002ed..133461a26f70 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/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 6fe1555229c9..c83abf913c3b 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -37,12 +37,14 @@ pub struct PreToolUseOutcome { pub hook_events: Vec, pub should_block: bool, pub block_reason: Option, + pub updated_input: Option, } #[derive(Debug, Default, PartialEq, Eq)] struct PreToolUseHandlerData { should_block: bool, block_reason: Option, + updated_input: Option, } pub(crate) fn preview( @@ -78,6 +80,7 @@ pub(crate) async fn run( hook_events: Vec::new(), should_block: false, block_reason: None, + updated_input: None, }; } @@ -108,6 +111,14 @@ pub(crate) async fn run( let block_reason = results .iter() .find_map(|result| result.data.block_reason.clone()); + let updated_input = if should_block { + None + } else { + results + .iter() + .rev() + .find_map(|result| result.data.updated_input.clone()) + }; PreToolUseOutcome { hook_events: results @@ -118,6 +129,7 @@ pub(crate) async fn run( .collect(), should_block, block_reason, + updated_input, } } @@ -151,6 +163,7 @@ fn parse_completed( let mut status = HookRunStatus::Completed; let mut should_block = false; let mut block_reason = None; + let mut updated_input = None; match run_result.error.as_deref() { Some(error) => { @@ -185,6 +198,8 @@ fn parse_completed( kind: HookOutputEntryKind::Feedback, text: reason, }); + } else { + updated_input = parsed.updated_input; } } else if trimmed_stdout.starts_with('{') || trimmed_stdout.starts_with('[') { status = HookRunStatus::Failed; @@ -238,6 +253,7 @@ fn parse_completed( data: PreToolUseHandlerData { should_block, block_reason, + updated_input, }, } } @@ -247,6 +263,7 @@ fn serialization_failure_outcome(hook_events: Vec) -> PreToo hook_events, should_block: false, block_reason: None, + updated_input: None, } } @@ -298,6 +315,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); @@ -310,6 +328,30 @@ mod tests { ); } + #[test] + fn permission_decision_allow_can_update_input() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","updatedInput":{"command":"echo rewritten"}}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + updated_input: Some(serde_json::json!({ "command": "echo rewritten" })), + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!(parsed.completed.run.entries, vec![]); + } + #[test] fn deprecated_block_decision_blocks_processing() { let parsed = parse_completed( @@ -327,6 +369,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); @@ -356,6 +399,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -381,6 +425,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -410,6 +455,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -435,6 +481,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); @@ -454,6 +501,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -479,6 +527,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("blocked by policy".to_string()), + updated_input: None, } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index d08cce6ee293..7365529415d8 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. From bcccc6c208ec91e882de9b0a75f0ea88dc8a35dd Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 30 Apr 2026 16:51:14 -0700 Subject: [PATCH 02/20] address hook rewrite review feedback --- codex-rs/core/src/tools/events.rs | 6 +----- codex-rs/core/src/tools/network_approval.rs | 10 +++++++++ codex-rs/core/src/tools/orchestrator.rs | 4 ++-- codex-rs/core/src/tools/sandboxing.rs | 8 +++++++ codex-rs/core/src/tools/sandboxing_tests.rs | 24 +++++++++++++++++++++ 5 files changed, 45 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index fabc9a8141ec..175cb41af114 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -354,11 +354,7 @@ impl ToolEmitter { (event, result) } Err(ToolError::UpdatedInput(updated_input)) => { - let event = ToolEventStage::Failure(ToolEventFailure::Message( - "tool input rewritten by hook".to_string(), - )); - let result = Err(FunctionCallError::UpdatedInput(updated_input)); - (event, result) + return Err(FunctionCallError::UpdatedInput(updated_input)); } }; self.emit(ctx, event).await; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 76d66f140b51..c77dcad54bad 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -476,6 +476,16 @@ impl NetworkApprovalService { 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); diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 05e0a530d9e3..89c378104f46 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -397,7 +397,7 @@ impl ToolOrchestrator { if evaluate_permission_request_hooks && let Some(permission_request) = tool.permission_request_payload(req) { - let permission_request_tool_input = permission_request.tool_input.clone(); + let permission_request_for_noop_check = permission_request.clone(); match run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, @@ -409,7 +409,7 @@ impl ToolOrchestrator { Some(PermissionRequestDecision::Allow { updated_input: Some(updated_input), }) => { - if updated_input != permission_request_tool_input { + if !permission_request_for_noop_check.updated_input_is_noop(&updated_input) { return Err(ToolError::UpdatedInput(updated_input)); } let decision = ReviewDecision::Approved; diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index f6581368cfa3..08a4e8026c0f 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. diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 19eb7a67d967..f2fb189137d3 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 { From 7932199328f159beadbe7d51895e4a8a388c0f48 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 4 May 2026 16:58:54 -0700 Subject: [PATCH 03/20] simplify hook input rewrites --- codex-rs/core/src/hook_runtime.rs | 2 + codex-rs/core/src/mcp_tool_call.rs | 3 +- .../core/src/tools/handlers/apply_patch.rs | 38 ++++--------- codex-rs/core/src/tools/handlers/mod.rs | 43 +++++++++++++++ codex-rs/core/src/tools/handlers/shell.rs | 55 +++++-------------- .../core/src/tools/handlers/unified_exec.rs | 31 +++-------- codex-rs/core/src/tools/registry.rs | 3 +- 7 files changed, 83 insertions(+), 92 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index bed525246452..e078cbebf1bd 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 34f37a98aa8a..3925e6a31c26 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; @@ -215,7 +216,7 @@ pub(crate) async fn handle_mcp_tool_call( let result = match decision { McpToolApprovalDecision::AcceptWithUpdatedInput(updated_input) => { input_rewrites += 1; - if input_rewrites > 8 { + if input_rewrites > MAX_HOOK_INPUT_REWRITES { notify_mcp_tool_call_skip( sess.as_ref(), turn_context.as_ref(), diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 71a40a42114f..581dd7747ec6 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -22,6 +22,8 @@ use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::parse_arguments; +use crate::tools::handlers::rewrite_function_string_argument; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::PostToolUsePayload; @@ -323,34 +325,16 @@ impl ToolHandler for ApplyPatchHandler { mut invocation: ToolInvocation, updated_input: serde_json::Value, ) -> Result { - let patch = updated_input - .get("command") - .and_then(serde_json::Value::as_str) - .ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned updatedInput without string field `command`".to_string(), - ) - })?; + let patch = updated_hook_command(&updated_input)?; invocation.payload = match invocation.payload { - ToolPayload::Function { arguments } => { - let mut arguments: serde_json::Value = parse_arguments(&arguments)?; - let serde_json::Value::Object(arguments) = &mut arguments else { - return Err(FunctionCallError::RespondToModel( - "apply_patch arguments must be an object".to_string(), - )); - }; - arguments.insert( - "input".to_string(), - serde_json::Value::String(patch.to_string()), - ); - ToolPayload::Function { - arguments: serde_json::to_string(&arguments).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten apply_patch arguments: {err}" - )) - })?, - } - } + ToolPayload::Function { arguments } => ToolPayload::Function { + arguments: rewrite_function_string_argument( + &arguments, + "apply_patch", + "input", + patch, + )?, + }, ToolPayload::Custom { .. } => ToolPayload::Custom { input: patch.to_string(), }, diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 0ddd1e5062d2..bd5331155645 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -25,6 +25,7 @@ use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::Deserialize; +use serde_json::Map; use serde_json::Value; use std::path::Path; @@ -74,6 +75,48 @@ where parse_arguments(arguments) } +fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { + updated_input + .get("command") + .and_then(Value::as_str) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned updatedInput without string field `command`".to_string(), + ) + }) +} + +fn rewrite_function_arguments( + arguments: &str, + tool_name: &str, + rewrite: impl FnOnce(&mut Map) -> Result<(), FunctionCallError>, +) -> Result { + let mut arguments: Value = parse_arguments(arguments)?; + let Value::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel(format!( + "{tool_name} arguments must be an object" + ))); + }; + rewrite(arguments)?; + serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten {tool_name} arguments: {err}" + )) + }) +} + +fn rewrite_function_string_argument( + arguments: &str, + tool_name: &str, + field_name: &str, + value: &str, +) -> Result { + rewrite_function_arguments(arguments, tool_name, |arguments| { + arguments.insert(field_name.to_string(), Value::String(value.to_string())); + Ok(()) + }) +} + fn resolve_workdir_base_path( arguments: &str, default_cwd: &AbsolutePathBuf, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index c0e3d93340c3..18ab33b2bde4 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -25,6 +25,9 @@ use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; +use crate::tools::handlers::rewrite_function_arguments; +use crate::tools::handlers::rewrite_function_string_argument; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::PostToolUsePayload; @@ -75,17 +78,6 @@ fn shell_command_payload_command(payload: &ToolPayload) -> Option { .map(|params| params.command) } -fn updated_hook_command(updated_input: &JsonValue) -> Result<&str, FunctionCallError> { - updated_input - .get("command") - .and_then(JsonValue::as_str) - .ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned updatedInput without string field `command`".to_string(), - ) - }) -} - struct RunExecLikeArgs { tool_name: String, exec_params: ExecParams, @@ -236,21 +228,13 @@ impl ToolHandler for ShellHandler { "hook returned shell input with an invalid command string".to_string(), ) })?; - let mut arguments: JsonValue = parse_arguments(&arguments)?; - let JsonValue::Object(arguments) = &mut arguments else { - return Err(FunctionCallError::RespondToModel( - "shell arguments must be an object".to_string(), - )); - }; - arguments.insert( - "command".to_string(), - JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), - ); ToolPayload::Function { - arguments: serde_json::to_string(&arguments).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten shell arguments: {err}" - )) + arguments: rewrite_function_arguments(&arguments, "shell", |arguments| { + arguments.insert( + "command".to_string(), + JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), + ); + Ok(()) })?, } } @@ -391,22 +375,13 @@ impl ToolHandler for ShellCommandHandler { "hook input rewrite received unsupported shell_command payload".to_string(), )); }; - let mut arguments: JsonValue = parse_arguments(&arguments)?; - let JsonValue::Object(arguments) = &mut arguments else { - return Err(FunctionCallError::RespondToModel( - "shell_command arguments must be an object".to_string(), - )); - }; - arguments.insert( - "command".to_string(), - JsonValue::String(updated_hook_command(&updated_input)?.to_string()), - ); invocation.payload = ToolPayload::Function { - arguments: serde_json::to_string(&arguments).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten shell_command arguments: {err}" - )) - })?, + arguments: rewrite_function_string_argument( + &arguments, + "shell_command", + "command", + updated_hook_command(&updated_input)?, + )?, }; Ok(invocation) } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 1fd8cbf0a8fc..9c2853aab03d 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -14,6 +14,8 @@ use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; +use crate::tools::handlers::rewrite_function_string_argument; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PreToolUsePayload; @@ -162,30 +164,13 @@ impl ToolHandler for UnifiedExecHandler { "hook input rewrite received unsupported unified_exec payload".to_string(), )); }; - let command = updated_input - .get("command") - .and_then(serde_json::Value::as_str) - .ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned updatedInput without string field `command`".to_string(), - ) - })?; - let mut arguments: serde_json::Value = parse_arguments(&arguments)?; - let serde_json::Value::Object(arguments) = &mut arguments else { - return Err(FunctionCallError::RespondToModel( - "exec_command arguments must be an object".to_string(), - )); - }; - arguments.insert( - "cmd".to_string(), - serde_json::Value::String(command.to_string()), - ); invocation.payload = ToolPayload::Function { - arguments: serde_json::to_string(&arguments).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten exec_command arguments: {err}" - )) - })?, + arguments: rewrite_function_string_argument( + &arguments, + "exec_command", + "cmd", + updated_hook_command(&updated_input)?, + )?, }; Ok(invocation) } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index e51d30f85645..21ce0c4f73af 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; @@ -231,7 +232,7 @@ where match self.handle(invocation.clone()).await { Err(FunctionCallError::UpdatedInput(updated_input)) => { rewrites += 1; - if rewrites > 8 { + if rewrites > MAX_HOOK_INPUT_REWRITES { return Err(FunctionCallError::RespondToModel( "hook input rewrite limit exceeded".to_string(), )); From b1ea7125f672196e42802a2be41a9c13db704054 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 5 May 2026 11:10:57 -0700 Subject: [PATCH 04/20] simplify hook rewrite helpers --- codex-rs/core/src/hook_runtime.rs | 41 +++++++++++------------ codex-rs/core/src/tools/handlers/mod.rs | 5 ++- codex-rs/core/src/tools/handlers/shell.rs | 1 - 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index e078cbebf1bd..ad9aba8fabbd 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -172,28 +172,27 @@ pub(crate) async fn run_pre_tool_use_hooks( } = hooks.run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; - if should_block { - block_reason.map_or( - PreToolUseHookResult::Continue { - updated_input: None, - }, - |reason| { - if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch") - && let Some(command) = tool_input.get("command").and_then(Value::as_str) - { - PreToolUseHookResult::Blocked(format!( - "Command blocked by PreToolUse hook: {reason}. Command: {command}" - )) - } else { - PreToolUseHookResult::Blocked(format!( - "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", - tool_name.name() - )) - } - }, - ) + if !should_block { + return PreToolUseHookResult::Continue { updated_input }; + } + + let Some(reason) = block_reason else { + return PreToolUseHookResult::Continue { + updated_input: None, + }; + }; + + if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch") + && let Some(command) = tool_input.get("command").and_then(Value::as_str) + { + PreToolUseHookResult::Blocked(format!( + "Command blocked by PreToolUse hook: {reason}. Command: {command}" + )) } else { - PreToolUseHookResult::Continue { updated_input } + PreToolUseHookResult::Blocked(format!( + "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", + tool_name.name() + )) } } diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index bd5331155645..8822efdd1a80 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -89,7 +89,7 @@ fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError fn rewrite_function_arguments( arguments: &str, tool_name: &str, - rewrite: impl FnOnce(&mut Map) -> Result<(), FunctionCallError>, + rewrite: impl FnOnce(&mut Map), ) -> Result { let mut arguments: Value = parse_arguments(arguments)?; let Value::Object(arguments) = &mut arguments else { @@ -97,7 +97,7 @@ fn rewrite_function_arguments( "{tool_name} arguments must be an object" ))); }; - rewrite(arguments)?; + rewrite(arguments); serde_json::to_string(&arguments).map_err(|err| { FunctionCallError::RespondToModel(format!( "failed to serialize rewritten {tool_name} arguments: {err}" @@ -113,7 +113,6 @@ fn rewrite_function_string_argument( ) -> Result { rewrite_function_arguments(arguments, tool_name, |arguments| { arguments.insert(field_name.to_string(), Value::String(value.to_string())); - Ok(()) }) } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 18ab33b2bde4..cce9ca2371f0 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -234,7 +234,6 @@ impl ToolHandler for ShellHandler { "command".to_string(), JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), ); - Ok(()) })?, } } From a4176794dc5d2bbbe7cfaa874541e70ba2224b47 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 11:03:56 -0700 Subject: [PATCH 05/20] Trust hook fixtures in rewrite tests --- codex-rs/core/tests/suite/hooks.rs | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 2c5422fc0a11..ec16367ba425 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -1645,10 +1645,7 @@ async fn permission_request_hook_rewrites_shell_command_before_execution() -> Re } }) .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -1816,10 +1813,7 @@ async fn permission_request_hook_rewrites_apply_patch_before_execution() -> Resu }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -2406,10 +2400,7 @@ async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { } }) .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -3083,10 +3074,7 @@ async fn pre_tool_use_rewrites_apply_patch_before_execution() -> Result<()> { }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; From 2f423b129d64503222dfb9a65e33cf8f311339bb Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 11:08:56 -0700 Subject: [PATCH 06/20] Split out PermissionRequest rewrites --- codex-rs/core/src/function_tool.rs | 2 - codex-rs/core/src/hook_runtime.rs | 2 - codex-rs/core/src/mcp_tool_call.rs | 87 ++----- codex-rs/core/src/mcp_tool_call_tests.rs | 134 +++------- codex-rs/core/src/stream_events_utils.rs | 5 - codex-rs/core/src/tools/events.rs | 3 - .../core/src/tools/handlers/unified_exec.rs | 3 - codex-rs/core/src/tools/network_approval.rs | 24 +- codex-rs/core/src/tools/orchestrator.rs | 20 +- codex-rs/core/src/tools/registry.rs | 19 +- .../tools/runtimes/shell/unix_escalation.rs | 16 +- codex-rs/core/src/tools/sandboxing.rs | 9 - codex-rs/core/src/tools/sandboxing_tests.rs | 24 -- codex-rs/core/src/unified_exec/errors.rs | 3 - .../core/src/unified_exec/process_manager.rs | 6 - codex-rs/core/tests/suite/hooks.rs | 228 ------------------ ...mission-request.command.output.schema.json | 2 +- codex-rs/hooks/src/engine/output_parser.rs | 43 +--- .../hooks/src/events/permission_request.rs | 47 +--- codex-rs/hooks/src/schema.rs | 4 +- 20 files changed, 87 insertions(+), 594 deletions(-) diff --git a/codex-rs/core/src/function_tool.rs b/codex-rs/core/src/function_tool.rs index 5fd6639958eb..240e04361cd4 100644 --- a/codex-rs/core/src/function_tool.rs +++ b/codex-rs/core/src/function_tool.rs @@ -4,8 +4,6 @@ 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 d34f91a03ab5..8b6b28cfc314 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 117eca7c5c37..91e2e079ab60 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; @@ -34,6 +33,7 @@ use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; +use crate::turn_metadata::McpTurnMetadataContext; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; use codex_analytics::build_track_events_context; @@ -197,45 +197,18 @@ 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; - }; - let tool_input = invocation - .arguments - .clone() - .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())); + 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 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 => { @@ -302,7 +275,8 @@ pub(crate) async fn handle_mcp_tool_call( return HandledMcpToolCall { result: CallToolResult::from_result(result), - tool_input, + tool_input: arguments_value + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), }; } @@ -841,7 +815,6 @@ async fn maybe_track_codex_app_used( #[derive(Debug, Clone, PartialEq, Eq)] enum McpToolApprovalDecision { Accept, - AcceptWithUpdatedInput(JsonValue), AcceptForSession, AcceptAndRemember, Decline { message: Option }, @@ -923,7 +896,13 @@ fn build_mcp_tool_call_request_meta( ) -> Option { let mut request_meta = serde_json::Map::new(); - if let Some(turn_metadata) = turn_context.turn_metadata_state.current_meta_value() { + if let Some(turn_metadata) = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(McpTurnMetadataContext { + model: turn_context.model_info.slug.as_str(), + reasoning_effort: turn_context.effective_reasoning_effort(), + }) + { request_meta.insert( crate::X_CODEX_TURN_METADATA_HEADER.to_string(), turn_metadata, @@ -1099,34 +1078,21 @@ 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: permission_request_tool_input.clone(), + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), }, ) .await { - Some(PermissionRequestDecision::Allow { - updated_input: Some(updated_input), - }) => { - if updated_input == permission_request_tool_input { - return Some(McpToolApprovalDecision::Accept); - } - return Some(McpToolApprovalDecision::AcceptWithUpdatedInput( - updated_input, - )); - } - Some(PermissionRequestDecision::Allow { - updated_input: None, - }) => { + Some(PermissionRequestDecision::Allow) => { return Some(McpToolApprovalDecision::Accept); } Some(PermissionRequestDecision::Deny { message }) => { @@ -1880,7 +1846,6 @@ 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 37d1c7f4f002..a6818579a600 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -4,6 +4,7 @@ use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; +use crate::turn_metadata::McpTurnMetadataContext; use codex_config::CONFIG_TOML_FILE; use codex_config::config_toml::ConfigToml; use codex_config::types::AppConfig; @@ -71,6 +72,13 @@ fn approval_metadata( } } +fn mcp_turn_metadata_context(turn_context: &TurnContext) -> McpTurnMetadataContext<'_> { + McpTurnMetadataContext { + model: turn_context.model_info.slug.as_str(), + reasoning_effort: turn_context.effective_reasoning_effort(), + } +} + fn write_sample_plugin_mcp(codex_home: &std::path::Path) { let plugin_root = codex_home.join("plugins/cache/test/sample/local"); std::fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create plugin manifest dir"); @@ -920,13 +928,10 @@ fn truncate_mcp_tool_result_for_event_bounds_large_error() { #[tokio::test] async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = serde_json::from_str::( - &turn_context - .turn_metadata_state - .current_header_value() - .expect("turn metadata header"), - ) - .expect("turn metadata json"); + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); let meta = build_mcp_tool_call_request_meta( &turn_context, @@ -935,6 +940,25 @@ async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() { /*metadata*/ None, ) .expect("custom servers should receive turn metadata"); + let turn_metadata = meta + .get(crate::X_CODEX_TURN_METADATA_HEADER) + .expect("turn metadata should be present"); + + assert_eq!( + turn_metadata + .get("model") + .and_then(serde_json::Value::as_str), + Some(turn_context.model_info.slug.as_str()) + ); + assert_eq!( + turn_metadata + .get("reasoning_effort") + .and_then(serde_json::Value::as_str), + turn_context + .effective_reasoning_effort() + .map(|effort| effort.to_string()) + .as_deref() + ); assert_eq!( meta, @@ -973,13 +997,10 @@ async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() { #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = serde_json::from_str::( - &turn_context - .turn_metadata_state - .current_header_value() - .expect("turn metadata header"), - ) - .expect("turn metadata json"); + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); let metadata = McpToolApprovalMetadata { annotations: None, connector_id: Some("calendar".to_string()), @@ -1023,13 +1044,10 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_call_id_without_existing_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = serde_json::from_str::( - &turn_context - .turn_metadata_state - .current_header_value() - .expect("turn metadata header"), - ) - .expect("turn metadata json"); + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); assert_eq!( build_mcp_tool_call_request_meta( @@ -2082,80 +2100,6 @@ 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 00dc66df206d..fc608aed93e4 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -336,11 +336,6 @@ 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 a618c8d59af2..f97e51fd8ad0 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -353,9 +353,6 @@ 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 96d5beaa97cc..2c19c6b6c72a 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -372,9 +372,6 @@ 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 c77dcad54bad..14af2c9c5f3c 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -473,29 +473,7 @@ impl NetworkApprovalService { .await { match permission_request_decision { - 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, - } => { + PermissionRequestDecision::Allow => { 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 f438da301b06..c618d778d6ee 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -398,7 +398,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, @@ -407,24 +406,7 @@ impl ToolOrchestrator { ) .await { - 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, - }) => { + Some(PermissionRequestDecision::Allow) => { 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 1735bf8daaec..82ee5efd3ab8 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; @@ -229,23 +228,7 @@ where invocation: ToolInvocation, ) -> 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 = self.handle(invocation.clone()).await?; 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 b45c7cb7b1a3..fef8db5ca9e5 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -420,21 +420,7 @@ impl CoreShellActionProvider { ) .await { - 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, - }) => { + Some(PermissionRequestDecision::Allow) => { 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 2c139a027c07..c17247beb47c 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. @@ -358,7 +350,6 @@ 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 f2fb189137d3..19eb7a67d967 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/errors.rs b/codex-rs/core/src/unified_exec/errors.rs index 501503e9df98..0576e7d7cfd2 100644 --- a/codex-rs/core/src/unified_exec/errors.rs +++ b/codex-rs/core/src/unified_exec/errors.rs @@ -1,5 +1,4 @@ use codex_protocol::exec_output::ExecToolCallOutput; -use serde_json::Value; use thiserror::Error; #[derive(Debug, Error)] @@ -24,8 +23,6 @@ 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 9a8bb85c4f25..4f85b1ddf7c7 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -221,9 +221,6 @@ 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(), } } @@ -1066,9 +1063,6 @@ 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 ec16367ba425..1c92918c9d3e 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -496,56 +496,6 @@ 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, @@ -1603,92 +1553,6 @@ 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(())); @@ -1763,98 +1627,6 @@ 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 5b4692556855..21d45382b0e5 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": "Replaces the entire tool input object when `behavior` is `allow`." + "description": "Reserved for a future input-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present." }, "updatedPermissions": { "default": null, diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 53e9b52ab373..9c3d442dacf4 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -23,12 +23,8 @@ pub(crate) struct PreToolUseOutput { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PermissionRequestDecision { - Allow { - updated_input: Option, - }, - Deny { - message: String, - }, + Allow, + Deny { message: String }, } #[derive(Debug, Clone)] @@ -319,9 +315,7 @@ fn unsupported_permission_request_hook_specific_output( decision: Option<&PermissionRequestDecisionWire>, ) -> Option { let decision = decision?; - if decision.updated_input.is_some() - && !matches!(decision.behavior, PermissionRequestBehaviorWire::Allow) - { + if decision.updated_input.is_some() { Some("PermissionRequest hook returned unsupported updatedInput".to_string()) } else if decision.updated_permissions.is_some() { Some("PermissionRequest hook returned unsupported updatedPermissions".to_string()) @@ -336,9 +330,7 @@ fn permission_request_decision( decision: &PermissionRequestDecisionWire, ) -> PermissionRequestDecision { match decision.behavior { - PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow { - updated_input: decision.updated_input.clone(), - }, + PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow, PermissionRequestBehaviorWire::Deny => PermissionRequestDecision::Deny { message: decision .message @@ -445,7 +437,7 @@ mod tests { use super::parse_permission_request; #[test] - fn permission_request_accepts_allow_updated_input_field() { + fn permission_request_rejects_reserved_updated_input_field() { let parsed = parse_permission_request( &json!({ "continue": true, @@ -461,31 +453,6 @@ 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 133461a26f70..2cebd0e002ed 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 { updated_input: Option }, + Allow, Deny { message: String }, } @@ -154,10 +154,8 @@ fn resolve_permission_request_decision<'a>( let mut resolved_allow = None; for decision in decisions { match decision { - PermissionRequestDecision::Allow { updated_input } => { - resolved_allow = Some(PermissionRequestDecision::Allow { - updated_input: updated_input.clone(), - }); + PermissionRequestDecision::Allow => { + resolved_allow = Some(PermissionRequestDecision::Allow); } PermissionRequestDecision::Deny { message } => { return Some(PermissionRequestDecision::Deny { @@ -221,8 +219,8 @@ fn parse_completed( }); } else if let Some(parsed_decision) = parsed.decision { match parsed_decision { - output_parser::PermissionRequestDecision::Allow { updated_input } => { - decision = Some(PermissionRequestDecision::Allow { updated_input }); + output_parser::PermissionRequestDecision::Allow => { + decision = Some(PermissionRequestDecision::Allow); } output_parser::PermissionRequestDecision::Deny { message } => { status = HookRunStatus::Blocked; @@ -296,9 +294,7 @@ mod tests { #[test] fn permission_request_deny_overrides_earlier_allow() { let decisions = [ - PermissionRequestDecision::Allow { - updated_input: None, - }, + PermissionRequestDecision::Allow, PermissionRequestDecision::Deny { message: "repo deny".to_string(), }, @@ -315,38 +311,13 @@ mod tests { #[test] fn permission_request_returns_allow_when_no_handler_denies() { let decisions = [ - PermissionRequestDecision::Allow { - updated_input: None, - }, - PermissionRequestDecision::Allow { - updated_input: None, - }, + PermissionRequestDecision::Allow, + PermissionRequestDecision::Allow, ]; assert_eq!( resolve_permission_request_decision(decisions.iter()), - 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" })), - }) + Some(PermissionRequestDecision::Allow) ); } diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 7365529415d8..d08cce6ee293 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -138,7 +138,9 @@ pub(crate) struct PermissionRequestHookSpecificOutputWire { #[serde(deny_unknown_fields)] pub(crate) struct PermissionRequestDecisionWire { pub behavior: PermissionRequestBehaviorWire, - /// Replaces the entire tool input object when `behavior` is `allow`. + /// Reserved for a future input-rewrite capability. + /// + /// PermissionRequest hooks currently fail closed if this field is present. #[serde(default)] pub updated_input: Option, /// Reserved for a future permission-rewrite capability. From c92387f864c514855e5a25f5b867392b93d3f01b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 11:10:55 -0700 Subject: [PATCH 07/20] Use the PR merge base for the split --- codex-rs/core/src/mcp_tool_call.rs | 9 +--- codex-rs/core/src/mcp_tool_call_tests.rs | 60 +++++++++--------------- 2 files changed, 22 insertions(+), 47 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 91e2e079ab60..58f26cb25e64 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -33,7 +33,6 @@ use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; -use crate::turn_metadata::McpTurnMetadataContext; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; use codex_analytics::build_track_events_context; @@ -896,13 +895,7 @@ fn build_mcp_tool_call_request_meta( ) -> Option { let mut request_meta = serde_json::Map::new(); - if let Some(turn_metadata) = turn_context - .turn_metadata_state - .current_meta_value_for_mcp_request(McpTurnMetadataContext { - model: turn_context.model_info.slug.as_str(), - reasoning_effort: turn_context.effective_reasoning_effort(), - }) - { + if let Some(turn_metadata) = turn_context.turn_metadata_state.current_meta_value() { request_meta.insert( crate::X_CODEX_TURN_METADATA_HEADER.to_string(), turn_metadata, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index a6818579a600..3d81f05c72c1 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -4,7 +4,6 @@ use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; -use crate::turn_metadata::McpTurnMetadataContext; use codex_config::CONFIG_TOML_FILE; use codex_config::config_toml::ConfigToml; use codex_config::types::AppConfig; @@ -72,13 +71,6 @@ fn approval_metadata( } } -fn mcp_turn_metadata_context(turn_context: &TurnContext) -> McpTurnMetadataContext<'_> { - McpTurnMetadataContext { - model: turn_context.model_info.slug.as_str(), - reasoning_effort: turn_context.effective_reasoning_effort(), - } -} - fn write_sample_plugin_mcp(codex_home: &std::path::Path) { let plugin_root = codex_home.join("plugins/cache/test/sample/local"); std::fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create plugin manifest dir"); @@ -928,10 +920,13 @@ fn truncate_mcp_tool_result_for_event_bounds_large_error() { #[tokio::test] async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = turn_context - .turn_metadata_state - .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) - .expect("turn metadata"); + let expected_turn_metadata = serde_json::from_str::( + &turn_context + .turn_metadata_state + .current_header_value() + .expect("turn metadata header"), + ) + .expect("turn metadata json"); let meta = build_mcp_tool_call_request_meta( &turn_context, @@ -940,25 +935,6 @@ async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() { /*metadata*/ None, ) .expect("custom servers should receive turn metadata"); - let turn_metadata = meta - .get(crate::X_CODEX_TURN_METADATA_HEADER) - .expect("turn metadata should be present"); - - assert_eq!( - turn_metadata - .get("model") - .and_then(serde_json::Value::as_str), - Some(turn_context.model_info.slug.as_str()) - ); - assert_eq!( - turn_metadata - .get("reasoning_effort") - .and_then(serde_json::Value::as_str), - turn_context - .effective_reasoning_effort() - .map(|effort| effort.to_string()) - .as_deref() - ); assert_eq!( meta, @@ -997,10 +973,13 @@ async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() { #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = turn_context - .turn_metadata_state - .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) - .expect("turn metadata"); + let expected_turn_metadata = serde_json::from_str::( + &turn_context + .turn_metadata_state + .current_header_value() + .expect("turn metadata header"), + ) + .expect("turn metadata json"); let metadata = McpToolApprovalMetadata { annotations: None, connector_id: Some("calendar".to_string()), @@ -1044,10 +1023,13 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_call_id_without_existing_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = turn_context - .turn_metadata_state - .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) - .expect("turn metadata"); + let expected_turn_metadata = serde_json::from_str::( + &turn_context + .turn_metadata_state + .current_header_value() + .expect("turn metadata header"), + ) + .expect("turn metadata json"); assert_eq!( build_mcp_tool_call_request_meta( From afa8cb9c4c34826bd1647403f99494830ab6e1cb Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 11:42:20 -0700 Subject: [PATCH 08/20] Document hook input inversions --- codex-rs/core/src/tools/handlers/apply_patch.rs | 3 +++ codex-rs/core/src/tools/handlers/mcp.rs | 3 +++ codex-rs/core/src/tools/handlers/shell.rs | 2 ++ codex-rs/core/src/tools/handlers/unified_exec.rs | 2 ++ 4 files changed, 10 insertions(+) diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 37e663c55300..1dc06780ae6b 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -325,6 +325,9 @@ impl ToolHandler for ApplyPatchHandler { }) } + // Hooks expose apply_patch through the stable `{ "command": ... }` shape, + // while the underlying tool stores the patch as either the function + // argument `input` or freeform custom-tool input. fn with_updated_hook_input( &self, mut invocation: ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index d00d6e6b0f7a..7397f5a45bf0 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -48,6 +48,9 @@ impl ToolHandler for McpHandler { }) } + // MCP hooks expose the full arguments object, so rewrites replace the + // serialized raw argument payload wholesale rather than patching one + // handler-owned field. fn with_updated_hook_input( &self, mut invocation: ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 8524fa77137f..45365dfbfd9a 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -379,6 +379,8 @@ impl ToolHandler for LocalShellHandler { }) } + // Hooks see a joined shell command string, but local_shell stores argv. + // Split on the way back in to invert the hook-facing representation. fn with_updated_hook_input( &self, mut invocation: ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 2c19c6b6c72a..1f56841425b2 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -164,6 +164,8 @@ impl ToolHandler for ExecCommandHandler { }) } + // Hooks normalize Bash-like tools to `{ "command": ... }`, while the + // exec_command wire schema still names the field `cmd`. fn with_updated_hook_input( &self, mut invocation: ToolInvocation, From bbfa80cda9c69b04ca6926d84e7977c015d4a5b6 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 11:47:55 -0700 Subject: [PATCH 09/20] Keep base handler result ordering unchanged --- codex-rs/core/src/tools/registry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 82ee5efd3ab8..eb9ede7cb9de 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -228,9 +228,9 @@ where invocation: ToolInvocation, ) -> BoxFuture<'a, Result> { Box::pin(async move { - let output = self.handle(invocation.clone()).await?; let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); + let output = self.handle(invocation.clone()).await?; let post_tool_use_payload = ToolHandler::post_tool_use_payload(self, &invocation, &output); Ok(AnyToolResult { From 04ae865e76e43e38bd529702ffcf5c9e448ee1f0 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 12:22:50 -0700 Subject: [PATCH 10/20] support rewrites for legacy shell handlers --- codex-rs/core/src/tools/handlers/shell.rs | 45 ++++++++ .../core/src/tools/handlers/shell_tests.rs | 103 ++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 45365dfbfd9a..0bbf06f36bbb 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -89,6 +89,35 @@ fn shell_command_payload_command(payload: &ToolPayload) -> Option { .map(|params| params.command) } +// Hooks expose legacy function shell tools as joined command strings, while +// their function payload stores argv. Split on the way back in to invert the +// hook-facing representation. +fn rewrite_shell_function_updated_hook_input( + mut invocation: ToolInvocation, + updated_input: JsonValue, + tool_name: &str, +) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel(format!( + "hook input rewrite received unsupported {tool_name} payload" + ))); + }; + let command = shlex::split(updated_hook_command(&updated_input)?).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_arguments(&arguments, tool_name, |arguments| { + arguments.insert( + "command".to_string(), + JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), + ); + })?, + }; + Ok(invocation) +} + struct RunExecLikeArgs { tool_name: String, exec_params: ExecParams, @@ -222,6 +251,14 @@ impl ToolHandler for ShellHandler { shell_function_pre_tool_use_payload(invocation) } + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: JsonValue, + ) -> Result { + rewrite_shell_function_updated_hook_input(invocation, updated_input, "shell") + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -300,6 +337,14 @@ impl ToolHandler for ContainerExecHandler { shell_function_pre_tool_use_payload(invocation) } + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: JsonValue, + ) -> Result { + rewrite_shell_function_updated_hook_input(invocation, updated_input, "container.exec") + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 8a32e5404b1d..4f44b953f4b7 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; use std::sync::Arc; use codex_protocol::models::ShellCommandToolCallParams; +use codex_protocol::models::ShellToolCallParams; use core_test_support::PathBufExt; use core_test_support::test_path_buf; use pretty_assertions::assert_eq; @@ -16,8 +17,10 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::ContainerExecHandler; use crate::tools::handlers::LocalShellHandler; use crate::tools::handlers::ShellCommandHandler; +use crate::tools::handlers::ShellHandler; use crate::tools::hook_names::HookToolName; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; @@ -269,6 +272,106 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { ); } +#[tokio::test] +async fn shell_handler_rewrites_hook_command_back_to_argv() { + let payload = ToolPayload::Function { + arguments: json!({ + "command": ["bash", "-lc", "printf old"], + "workdir": "subdir", + }) + .to_string(), + }; + let (session, turn) = make_session_and_context().await; + let handler = ShellHandler; + + let invocation = handler + .with_updated_hook_input( + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-43".to_string(), + tool_name: codex_tools::ToolName::plain("shell"), + source: ToolCallSource::Direct, + payload, + }, + json!({ "command": "bash -lc 'printf new'" }), + ) + .expect("shell rewrite should succeed"); + + let ToolPayload::Function { arguments } = invocation.payload else { + panic!("shell rewrite should preserve a function payload"); + }; + assert_eq!( + serde_json::from_str::(&arguments) + .expect("rewritten shell arguments should parse"), + ShellToolCallParams { + command: vec![ + "bash".to_string(), + "-lc".to_string(), + "printf new".to_string(), + ], + workdir: Some("subdir".to_string()), + timeout_ms: None, + sandbox_permissions: None, + additional_permissions: None, + prefix_rule: None, + justification: None, + } + ); +} + +#[tokio::test] +async fn container_exec_handler_rewrites_hook_command_back_to_argv() { + let payload = ToolPayload::Function { + arguments: json!({ + "command": ["bash", "-lc", "printf old"], + "workdir": "subdir", + }) + .to_string(), + }; + let (session, turn) = make_session_and_context().await; + let handler = ContainerExecHandler; + + let invocation = handler + .with_updated_hook_input( + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-44".to_string(), + tool_name: codex_tools::ToolName::plain("container.exec"), + source: ToolCallSource::Direct, + payload, + }, + json!({ "command": "bash -lc 'printf new'" }), + ) + .expect("container.exec rewrite should succeed"); + + let ToolPayload::Function { arguments } = invocation.payload else { + panic!("container.exec rewrite should preserve a function payload"); + }; + assert_eq!( + serde_json::from_str::(&arguments) + .expect("rewritten container.exec arguments should parse"), + ShellToolCallParams { + command: vec![ + "bash".to_string(), + "-lc".to_string(), + "printf new".to_string(), + ], + workdir: Some("subdir".to_string()), + timeout_ms: None, + sandbox_permissions: None, + additional_permissions: None, + prefix_rule: None, + justification: None, + } + ); +} + #[tokio::test] async fn build_post_tool_use_payload_uses_tool_output_wire_value() { let payload = ToolPayload::Function { From 786882a0bcd499e95d509bf3f87e5bb184e432bf Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 13:05:15 -0700 Subject: [PATCH 11/20] Log rewritten tool payloads --- codex-rs/core/src/tools/registry.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index eb9ede7cb9de..4547fe8ebfe2 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -294,8 +294,6 @@ impl ToolRegistry { let display_name = tool_name.display(); let call_id_owned = invocation.call_id.clone(); let otel = invocation.turn.session_telemetry.clone(); - let payload_for_response = invocation.payload.clone(); - let log_payload = payload_for_response.log_payload(); let metric_tags = [ ( "sandbox", @@ -343,6 +341,7 @@ impl ToolRegistry { Some(handler) => handler, None => { let message = unsupported_tool_call_message(&invocation.payload, &tool_name); + let log_payload = invocation.payload.log_payload(); otel.tool_result_with_tags( &display_name, &call_id_owned, @@ -362,6 +361,7 @@ impl ToolRegistry { if !handler.matches_kind(&invocation.payload) { let message = format!("tool {display_name} invoked with incompatible payload"); + let log_payload = invocation.payload.log_payload(); otel.tool_result_with_tags( &display_name, &call_id_owned, @@ -409,6 +409,7 @@ impl ToolRegistry { let invocation_for_tool = invocation.clone(); let started = Instant::now(); + let log_payload = invocation.payload.log_payload(); let result = otel .log_tool_result_with_tags( &display_name, From 173156bf690dbca8dd5bf61bac93f0a7a5e68fea Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 13:20:24 -0700 Subject: [PATCH 12/20] Test code mode PreToolUse rewrites --- codex-rs/core/tests/suite/hooks.rs | 83 ++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 1c92918c9d3e..f587fb5f23e4 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -26,6 +26,7 @@ use core_test_support::managed_network_requirements_loader; use core_test_support::responses::ev_apply_patch_function_call; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_custom_tool_call; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_message_item_added; use core_test_support::responses::ev_output_text_delta; @@ -2208,6 +2209,88 @@ async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { Ok(()) } +#[tokio::test] +async fn pre_tool_use_rewrites_code_mode_nested_exec_command_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-code-mode-rewrite"; + let original_marker = std::env::temp_dir().join("pretooluse-code-mode-original-marker"); + let rewritten_marker = std::env::temp_dir().join("pretooluse-code-mode-rewritten-marker"); + let original_command = format!("printf original > {}", original_marker.display()); + let rewritten_command = format!("printf rewritten > {}", rewritten_marker.display()); + let original_command_json = + serde_json::to_string(&original_command).context("serialize original command")?; + let code = format!( + r#" +const output = await tools.exec_command({{ cmd: {original_command_json} }}); +text(output.output); +"# + ); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_custom_tool_call(call_id, "exec", &code), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "hook rewrote the nested command"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({ "command": rewritten_command }); + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_pre_build_hook(move |home| { + if let Err(error) = write_updating_pre_tool_use_hook(home, "^Bash$", &updated_input) { + panic!("failed to write updating pre tool use hook fixture: {error}"); + } + }) + .with_config(|config| { + let _ = config.features.enable(Feature::CodeMode); + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + if original_marker.exists() { + fs::remove_file(&original_marker).context("remove stale original pre tool marker")?; + } + if rewritten_marker.exists() { + fs::remove_file(&rewritten_marker).context("remove stale rewritten pre tool marker")?; + } + + test.submit_turn_with_permission_profile( + "run the rewritten shell command from code mode", + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + requests[1].custom_tool_call_output(call_id); + assert!( + !original_marker.exists(), + "original nested shell command should not execute after rewrite" + ); + assert_eq!( + fs::read_to_string(&rewritten_marker) + .context("read rewritten code mode pre tool marker")?, + "rewritten" + ); + + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["tool_input"]["command"], original_command); + + Ok(()) +} + #[tokio::test] async fn plugin_pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); From 1cd3e86056d062af01a1157fcd8ddb17f84d19a7 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 14:16:38 -0700 Subject: [PATCH 13/20] Centralize pre-hook tool compatibility --- .../core/src/tools/handlers/apply_patch.rs | 44 +-- .../src/tools/handlers/apply_patch_tests.rs | 8 +- codex-rs/core/src/tools/handlers/mcp.rs | 58 +--- codex-rs/core/src/tools/handlers/mod.rs | 47 +-- codex-rs/core/src/tools/handlers/shell.rs | 142 +-------- .../core/src/tools/handlers/shell_tests.rs | 75 ++--- .../core/src/tools/handlers/unified_exec.rs | 41 +-- .../src/tools/handlers/unified_exec_tests.rs | 8 +- codex-rs/core/src/tools/hook_compat.rs | 276 ++++++++++++++++++ codex-rs/core/src/tools/mod.rs | 1 + codex-rs/core/src/tools/registry.rs | 43 +-- 11 files changed, 330 insertions(+), 413 deletions(-) create mode 100644 codex-rs/core/src/tools/hook_compat.rs diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 1dc06780ae6b..cccc517ba392 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -22,12 +22,9 @@ use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::parse_arguments; -use crate::tools::handlers::rewrite_function_string_argument; -use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolArgumentDiffConsumer; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; @@ -243,12 +240,8 @@ fn write_permissions_for_paths( normalize_additional_permissions(permissions).ok() } -/// Extracts the raw patch text used as the command-shaped hook input for apply_patch. -/// -/// The apply_patch tool can arrive as the older JSON/function shape or as a -/// freeform custom tool call. Both represent the same file edit operation, so -/// hooks see the raw patch body in `tool_input.command` either way. -fn apply_patch_payload_command(payload: &ToolPayload) -> Option { +/// Extracts the raw patch text from either supported apply_patch payload form. +pub(crate) fn apply_patch_payload_command(payload: &ToolPayload) -> Option { match payload { ToolPayload::Function { arguments } => parse_arguments::(arguments) .ok() @@ -318,39 +311,6 @@ impl ToolHandler for ApplyPatchHandler { Some(Box::::default()) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - apply_patch_payload_command(&invocation.payload).map(|command| PreToolUsePayload { - tool_name: HookToolName::apply_patch(), - tool_input: serde_json::json!({ "command": command }), - }) - } - - // Hooks expose apply_patch through the stable `{ "command": ... }` shape, - // while the underlying tool stores the patch as either the function - // argument `input` or freeform custom-tool input. - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: serde_json::Value, - ) -> Result { - let patch = updated_hook_command(&updated_input)?; - invocation.payload = match invocation.payload { - ToolPayload::Function { arguments } => ToolPayload::Function { - arguments: rewrite_function_string_argument( - &arguments, - "apply_patch", - "input", - patch, - )?, - }, - ToolPayload::Custom { .. } => ToolPayload::Custom { - input: patch.to_string(), - }, - payload => payload, - }; - Ok(invocation) - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 04472e4623a9..4ee6bd245018 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -49,10 +49,8 @@ async fn pre_tool_use_payload_uses_json_patch_input() { arguments: json!({ "input": patch }).to_string(), }; let invocation = invocation_for_payload(payload).await; - let handler = ApplyPatchHandler; - assert_eq!( - handler.pre_tool_use_payload(&invocation), + crate::tools::hook_compat::pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), @@ -67,10 +65,8 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { input: patch.to_string(), }; let invocation = invocation_for_payload(payload).await; - let handler = ApplyPatchHandler; - assert_eq!( - handler.pre_tool_use_payload(&invocation), + crate::tools::hook_compat::pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 7397f5a45bf0..1b9d6a613b0a 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -10,11 +10,9 @@ use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; -use serde_json::Value; pub struct McpHandler { tool_name: ToolName, @@ -37,40 +35,6 @@ impl ToolHandler for McpHandler { ToolKind::Mcp } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let ToolPayload::Mcp { raw_arguments, .. } = &invocation.payload else { - return None; - }; - - Some(PreToolUsePayload { - tool_name: HookToolName::new(self.tool_name.display()), - tool_input: mcp_hook_tool_input(raw_arguments), - }) - } - - // MCP hooks expose the full arguments object, so rewrites replace the - // serialized raw argument payload wholesale rather than patching one - // handler-owned field. - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: Value, - ) -> Result { - invocation.payload = match invocation.payload { - ToolPayload::Mcp { server, tool, .. } => ToolPayload::Mcp { - server, - tool, - raw_arguments: serde_json::to_string(&updated_input).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten MCP arguments: {err}" - )) - })?, - }, - payload => payload, - }; - Ok(invocation) - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -137,14 +101,6 @@ impl ToolHandler for McpHandler { } } -fn mcp_hook_tool_input(raw_arguments: &str) -> Value { - if raw_arguments.trim().is_empty() { - return Value::Object(serde_json::Map::new()); - } - - serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) -} - #[cfg(test)] mod tests { use super::*; @@ -170,13 +126,8 @@ mod tests { .to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = McpHandler::new(codex_tools::ToolName::namespaced( - "mcp__memory__", - "create_entities", - )); - assert_eq!( - handler.pre_tool_use_payload(&ToolInvocation { + crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -186,7 +137,7 @@ mod tests { source: ToolCallSource::Direct, payload, }), - Some(PreToolUsePayload { + Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::new("mcp__memory__create_entities"), tool_input: json!({ "entities": [{ @@ -262,6 +213,9 @@ mod tests { #[test] fn mcp_hook_tool_input_defaults_empty_args_to_object() { - assert_eq!(mcp_hook_tool_input(" "), json!({})); + assert_eq!( + crate::tools::hook_compat::mcp_hook_tool_input(" "), + json!({}) + ); } } diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 8a6cb164cf5a..60ea73303b06 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -24,7 +24,6 @@ use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::Deserialize; -use serde_json::Map; use serde_json::Value; use std::path::Path; @@ -54,6 +53,9 @@ pub use shell::ContainerExecHandler; pub use shell::LocalShellHandler; pub use shell::ShellCommandHandler; pub use shell::ShellHandler; +pub(crate) use shell::local_shell_payload_command; +pub(crate) use shell::shell_command_payload_command; +pub(crate) use shell::shell_function_payload_command; pub use test_sync::TestSyncHandler; pub use tool_search::ToolSearchHandler; pub use unavailable_tool::UnavailableToolHandler; @@ -62,7 +64,7 @@ pub use unified_exec::ExecCommandHandler; pub use unified_exec::WriteStdinHandler; pub use view_image::ViewImageHandler; -fn parse_arguments(arguments: &str) -> Result +pub(crate) fn parse_arguments(arguments: &str) -> Result where T: for<'de> Deserialize<'de>, { @@ -82,47 +84,6 @@ where parse_arguments(arguments) } -fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { - updated_input - .get("command") - .and_then(Value::as_str) - .ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned updatedInput without string field `command`".to_string(), - ) - }) -} - -fn rewrite_function_arguments( - arguments: &str, - tool_name: &str, - rewrite: impl FnOnce(&mut Map), -) -> Result { - let mut arguments: Value = parse_arguments(arguments)?; - let Value::Object(arguments) = &mut arguments else { - return Err(FunctionCallError::RespondToModel(format!( - "{tool_name} arguments must be an object" - ))); - }; - rewrite(arguments); - serde_json::to_string(&arguments).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten {tool_name} arguments: {err}" - )) - }) -} - -fn rewrite_function_string_argument( - arguments: &str, - tool_name: &str, - field_name: &str, - value: &str, -) -> Result { - rewrite_function_arguments(arguments, tool_name, |arguments| { - arguments.insert(field_name.to_string(), Value::String(value.to_string())); - }) -} - fn resolve_workdir_base_path( arguments: &str, default_cwd: &AbsolutePathBuf, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 0bbf06f36bbb..26c4e9f820e8 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -25,13 +25,9 @@ use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; -use crate::tools::handlers::rewrite_function_arguments; -use crate::tools::handlers::rewrite_function_string_argument; -use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRequest; @@ -59,7 +55,7 @@ pub struct ShellCommandHandler { backend: ShellCommandBackend, } -fn shell_function_payload_command(payload: &ToolPayload) -> Option { +pub(crate) fn shell_function_payload_command(payload: &ToolPayload) -> Option { let ToolPayload::Function { arguments } = payload else { return None; }; @@ -69,7 +65,7 @@ fn shell_function_payload_command(payload: &ToolPayload) -> Option { .map(|params| codex_shell_command::parse_command::shlex_join(¶ms.command)) } -fn local_shell_payload_command(payload: &ToolPayload) -> Option { +pub(crate) fn local_shell_payload_command(payload: &ToolPayload) -> Option { let ToolPayload::LocalShell { params } = payload else { return None; }; @@ -79,7 +75,7 @@ fn local_shell_payload_command(payload: &ToolPayload) -> Option { )) } -fn shell_command_payload_command(payload: &ToolPayload) -> Option { +pub(crate) fn shell_command_payload_command(payload: &ToolPayload) -> Option { let ToolPayload::Function { arguments } = payload else { return None; }; @@ -89,35 +85,6 @@ fn shell_command_payload_command(payload: &ToolPayload) -> Option { .map(|params| params.command) } -// Hooks expose legacy function shell tools as joined command strings, while -// their function payload stores argv. Split on the way back in to invert the -// hook-facing representation. -fn rewrite_shell_function_updated_hook_input( - mut invocation: ToolInvocation, - updated_input: JsonValue, - tool_name: &str, -) -> Result { - let ToolPayload::Function { arguments } = invocation.payload else { - return Err(FunctionCallError::RespondToModel(format!( - "hook input rewrite received unsupported {tool_name} payload" - ))); - }; - let command = shlex::split(updated_hook_command(&updated_input)?).ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned shell input with an invalid command string".to_string(), - ) - })?; - invocation.payload = ToolPayload::Function { - arguments: rewrite_function_arguments(&arguments, tool_name, |arguments| { - arguments.insert( - "command".to_string(), - JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), - ); - })?, - }; - Ok(invocation) -} - struct RunExecLikeArgs { tool_name: String, exec_params: ExecParams, @@ -247,18 +214,6 @@ impl ToolHandler for ShellHandler { .unwrap_or(true) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - shell_function_pre_tool_use_payload(invocation) - } - - fn with_updated_hook_input( - &self, - invocation: ToolInvocation, - updated_input: JsonValue, - ) -> Result { - rewrite_shell_function_updated_hook_input(invocation, updated_input, "shell") - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -333,18 +288,6 @@ impl ToolHandler for ContainerExecHandler { .unwrap_or(true) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - shell_function_pre_tool_use_payload(invocation) - } - - fn with_updated_hook_input( - &self, - invocation: ToolInvocation, - updated_input: JsonValue, - ) -> Result { - rewrite_shell_function_updated_hook_input(invocation, updated_input, "container.exec") - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -417,50 +360,6 @@ impl ToolHandler for LocalShellHandler { !is_known_safe_command(¶ms.command) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - local_shell_payload_command(&invocation.payload).map(|command| PreToolUsePayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ "command": command }), - }) - } - - // Hooks see a joined shell command string, but local_shell stores argv. - // Split on the way back in to invert the hook-facing representation. - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: JsonValue, - ) -> Result { - let command = updated_hook_command(&updated_input)?; - invocation.payload = match invocation.payload { - ToolPayload::Function { arguments } => { - let command = shlex::split(command).ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned shell input with an invalid command string".to_string(), - ) - })?; - ToolPayload::Function { - arguments: rewrite_function_arguments(&arguments, "shell", |arguments| { - arguments.insert( - "command".to_string(), - JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), - ); - })?, - } - } - ToolPayload::LocalShell { mut params } => { - params.command = shlex::split(command).ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned shell input with an invalid command string".to_string(), - ) - })?; - ToolPayload::LocalShell { params } - } - payload => payload, - }; - Ok(invocation) - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -512,13 +411,6 @@ impl ToolHandler for LocalShellHandler { } } -fn shell_function_pre_tool_use_payload(invocation: &ToolInvocation) -> Option { - shell_function_payload_command(&invocation.payload).map(|command| PreToolUsePayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ "command": command }), - }) -} - fn shell_function_post_tool_use_payload( invocation: &ToolInvocation, result: &FunctionToolOutput, @@ -569,34 +461,6 @@ impl ToolHandler for ShellCommandHandler { .unwrap_or(true) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - shell_command_payload_command(&invocation.payload).map(|command| PreToolUsePayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ "command": command }), - }) - } - - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: JsonValue, - ) -> Result { - let ToolPayload::Function { arguments } = invocation.payload else { - return Err(FunctionCallError::RespondToModel( - "hook input rewrite received unsupported shell_command payload".to_string(), - )); - }; - invocation.payload = ToolPayload::Function { - arguments: rewrite_function_string_argument( - &arguments, - "shell_command", - "command", - updated_hook_command(&updated_input)?, - )?, - }; - Ok(invocation) - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 4f44b953f4b7..449f6c94bdd6 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -17,10 +17,7 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; -use crate::tools::handlers::ContainerExecHandler; -use crate::tools::handlers::LocalShellHandler; use crate::tools::handlers::ShellCommandHandler; -use crate::tools::handlers::ShellHandler; use crate::tools::hook_names::HookToolName; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; @@ -224,10 +221,8 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { }, }; let (session, turn) = make_session_and_context().await; - let handler = LocalShellHandler; - assert_eq!( - handler.pre_tool_use_payload(&ToolInvocation { + crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -250,12 +245,8 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { arguments: json!({ "command": "printf shell command" }).to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = ShellCommandHandler { - backend: super::ShellCommandBackend::Classic, - }; - assert_eq!( - handler.pre_tool_use_payload(&ToolInvocation { + crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -282,23 +273,20 @@ async fn shell_handler_rewrites_hook_command_back_to_argv() { .to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = ShellHandler; - - let invocation = handler - .with_updated_hook_input( - ToolInvocation { - session: session.into(), - turn: turn.into(), - cancellation_token: tokio_util::sync::CancellationToken::new(), - tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), - call_id: "call-43".to_string(), - tool_name: codex_tools::ToolName::plain("shell"), - source: ToolCallSource::Direct, - payload, - }, - json!({ "command": "bash -lc 'printf new'" }), - ) - .expect("shell rewrite should succeed"); + let invocation = crate::tools::hook_compat::apply_updated_input( + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-43".to_string(), + tool_name: codex_tools::ToolName::plain("shell"), + source: ToolCallSource::Direct, + payload, + }, + json!({ "command": "bash -lc 'printf new'" }), + ) + .expect("shell rewrite should succeed"); let ToolPayload::Function { arguments } = invocation.payload else { panic!("shell rewrite should preserve a function payload"); @@ -332,23 +320,20 @@ async fn container_exec_handler_rewrites_hook_command_back_to_argv() { .to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = ContainerExecHandler; - - let invocation = handler - .with_updated_hook_input( - ToolInvocation { - session: session.into(), - turn: turn.into(), - cancellation_token: tokio_util::sync::CancellationToken::new(), - tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), - call_id: "call-44".to_string(), - tool_name: codex_tools::ToolName::plain("container.exec"), - source: ToolCallSource::Direct, - payload, - }, - json!({ "command": "bash -lc 'printf new'" }), - ) - .expect("container.exec rewrite should succeed"); + let invocation = crate::tools::hook_compat::apply_updated_input( + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-44".to_string(), + tool_name: codex_tools::ToolName::plain("container.exec"), + source: ToolCallSource::Direct, + payload, + }, + json!({ "command": "bash -lc 'printf new'" }), + ) + .expect("container.exec rewrite should succeed"); let ToolPayload::Function { arguments } = invocation.payload else { panic!("container.exec rewrite should preserve a function payload"); diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 1f56841425b2..224c0aaacd9b 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -14,11 +14,8 @@ use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_tool_environment; -use crate::tools::handlers::rewrite_function_string_argument; -use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::unified_exec::ExecCommandRequest; @@ -48,7 +45,7 @@ pub struct WriteStdinHandler; #[derive(Debug, Deserialize)] pub(crate) struct ExecCommandArgs { - cmd: String, + pub(crate) cmd: String, #[serde(default)] pub(crate) workdir: Option, #[serde(default)] @@ -151,42 +148,6 @@ impl ToolHandler for ExecCommandHandler { !is_known_safe_command(&command) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let ToolPayload::Function { arguments } = &invocation.payload else { - return None; - }; - - parse_arguments::(arguments) - .ok() - .map(|args| PreToolUsePayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ "command": args.cmd }), - }) - } - - // Hooks normalize Bash-like tools to `{ "command": ... }`, while the - // exec_command wire schema still names the field `cmd`. - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: serde_json::Value, - ) -> Result { - let ToolPayload::Function { arguments } = invocation.payload else { - return Err(FunctionCallError::RespondToModel( - "hook input rewrite received unsupported exec_command payload".to_string(), - )); - }; - invocation.payload = ToolPayload::Function { - arguments: rewrite_function_string_argument( - &arguments, - "exec_command", - "cmd", - updated_hook_command(&updated_input)?, - )?, - }; - Ok(invocation) - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, 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 8818b2e3442c..451c5fa3b32e 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -184,10 +184,8 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { arguments: serde_json::json!({ "cmd": "printf exec command" }).to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = ExecCommandHandler; - assert_eq!( - handler.pre_tool_use_payload(&ToolInvocation { + crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -210,10 +208,8 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { arguments: serde_json::json!({ "chars": "echo hi" }).to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = WriteStdinHandler; - assert_eq!( - handler.pre_tool_use_payload(&ToolInvocation { + crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), diff --git a/codex-rs/core/src/tools/hook_compat.rs b/codex-rs/core/src/tools/hook_compat.rs new file mode 100644 index 000000000000..5821598e6679 --- /dev/null +++ b/codex-rs/core/src/tools/hook_compat.rs @@ -0,0 +1,276 @@ +use serde_json::Map; +use serde_json::Value; + +use crate::function_tool::FunctionCallError; +use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolPayload; +use crate::tools::handlers::apply_patch::apply_patch_payload_command; +use crate::tools::handlers::local_shell_payload_command; +use crate::tools::handlers::parse_arguments; +use crate::tools::handlers::shell_command_payload_command; +use crate::tools::handlers::shell_function_payload_command; +use crate::tools::handlers::unified_exec::ExecCommandArgs; +use crate::tools::hook_names::HookToolName; +use crate::tools::registry::PreToolUsePayload; + +/// Projects native tool payloads into the stable input shape exposed to hooks. +/// +/// The hook protocol intentionally hides a few native tool-schema differences: +/// +/// - Bash-like tools are exposed as `Bash` with `{ "command": }`, +/// even though their native payloads store commands as argv, `command`, or +/// `cmd` depending on the concrete tool. +/// - `apply_patch` exposes its raw patch body through `{ "command": }` +/// for compatibility with existing hook consumers. +/// - MCP tools already use arbitrary JSON argument objects, so hooks see those +/// arguments directly rather than through a compatibility shape. +pub(crate) fn pre_tool_use_payload(invocation: &ToolInvocation) -> Option { + match invocation.tool_name.name.as_str() { + "shell" | "container.exec" => { + shell_function_payload_command(&invocation.payload).map(bash_payload) + } + "local_shell" => local_shell_payload_command(&invocation.payload).map(bash_payload), + "shell_command" => shell_command_payload_command(&invocation.payload).map(bash_payload), + "exec_command" => exec_command_payload_command(&invocation.payload).map(bash_payload), + "apply_patch" => { + apply_patch_payload_command(&invocation.payload).map(|command| PreToolUsePayload { + tool_name: HookToolName::apply_patch(), + tool_input: serde_json::json!({ "command": command }), + }) + } + _ => mcp_payload(invocation), + } +} + +/// Rebuilds native tool payloads from hook-facing `updatedInput`. +/// +/// This is the inverse of [`pre_tool_use_payload`]: Bash-like and +/// `apply_patch` updates come back through the compatibility `{ "command": ... }` +/// shape and must be written into each tool's native schema, while MCP updates +/// replace the raw argument object wholesale because the hook-facing and native +/// representations are the same. +pub(crate) fn apply_updated_input( + invocation: ToolInvocation, + updated_input: Value, +) -> Result { + match invocation.tool_name.name.as_str() { + "shell" => rewrite_shell_function_updated_input(invocation, updated_input, "shell"), + "container.exec" => { + rewrite_shell_function_updated_input(invocation, updated_input, "container.exec") + } + "local_shell" => rewrite_local_shell_updated_input(invocation, updated_input), + "shell_command" => rewrite_shell_command_updated_input(invocation, updated_input), + "exec_command" => rewrite_exec_command_updated_input(invocation, updated_input), + "apply_patch" => rewrite_apply_patch_updated_input(invocation, updated_input), + _ => rewrite_mcp_updated_input(invocation, updated_input), + } +} + +fn bash_payload(command: String) -> PreToolUsePayload { + PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": command }), + } +} + +fn exec_command_payload_command(payload: &ToolPayload) -> Option { + let ToolPayload::Function { arguments } = payload else { + return None; + }; + + parse_arguments::(arguments) + .ok() + .map(|args| args.cmd) +} + +fn mcp_payload(invocation: &ToolInvocation) -> Option { + let ToolPayload::Mcp { raw_arguments, .. } = &invocation.payload else { + return None; + }; + + Some(PreToolUsePayload { + tool_name: HookToolName::new(invocation.tool_name.display()), + tool_input: mcp_hook_tool_input(raw_arguments), + }) +} + +fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { + updated_input + .get("command") + .and_then(Value::as_str) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned updatedInput without string field `command`".to_string(), + ) + }) +} + +fn rewrite_function_arguments( + arguments: &str, + tool_name: &str, + rewrite: impl FnOnce(&mut Map), +) -> Result { + let mut arguments: Value = parse_arguments(arguments)?; + let Value::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel(format!( + "{tool_name} arguments must be an object" + ))); + }; + rewrite(arguments); + serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten {tool_name} arguments: {err}" + )) + }) +} + +fn rewrite_function_string_argument( + arguments: &str, + tool_name: &str, + field_name: &str, + value: &str, +) -> Result { + rewrite_function_arguments(arguments, tool_name, |arguments| { + arguments.insert(field_name.to_string(), Value::String(value.to_string())); + }) +} + +/// Rehydrates legacy function-style shell tools from hook-facing command text. +fn rewrite_shell_function_updated_input( + mut invocation: ToolInvocation, + updated_input: Value, + tool_name: &str, +) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel(format!( + "hook input rewrite received unsupported {tool_name} payload" + ))); + }; + let command = shlex::split(updated_hook_command(&updated_input)?).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_arguments(&arguments, tool_name, |arguments| { + arguments.insert( + "command".to_string(), + Value::Array(command.into_iter().map(Value::String).collect()), + ); + })?, + }; + Ok(invocation) +} + +/// Rehydrates `local_shell` argv from hook-facing command text. +fn rewrite_local_shell_updated_input( + mut invocation: ToolInvocation, + updated_input: Value, +) -> Result { + let command = updated_hook_command(&updated_input)?; + invocation.payload = match invocation.payload { + ToolPayload::LocalShell { mut params } => { + params.command = shlex::split(command).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + ToolPayload::LocalShell { params } + } + payload => payload, + }; + Ok(invocation) +} + +/// Stores hook-facing command text back into the native `shell_command.command`. +fn rewrite_shell_command_updated_input( + mut invocation: ToolInvocation, + updated_input: Value, +) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported shell_command payload".to_string(), + )); + }; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_string_argument( + &arguments, + "shell_command", + "command", + updated_hook_command(&updated_input)?, + )?, + }; + Ok(invocation) +} + +/// Stores hook-facing command text back into the native `exec_command.cmd`. +fn rewrite_exec_command_updated_input( + mut invocation: ToolInvocation, + updated_input: Value, +) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported exec_command payload".to_string(), + )); + }; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_string_argument( + &arguments, + "exec_command", + "cmd", + updated_hook_command(&updated_input)?, + )?, + }; + Ok(invocation) +} + +/// Stores hook-facing patch text back into the native apply_patch payload form. +fn rewrite_apply_patch_updated_input( + mut invocation: ToolInvocation, + updated_input: Value, +) -> Result { + let patch = updated_hook_command(&updated_input)?; + invocation.payload = match invocation.payload { + ToolPayload::Function { arguments } => ToolPayload::Function { + arguments: rewrite_function_string_argument(&arguments, "apply_patch", "input", patch)?, + }, + ToolPayload::Custom { .. } => ToolPayload::Custom { + input: patch.to_string(), + }, + payload => payload, + }; + Ok(invocation) +} + +/// Replaces MCP raw arguments directly because MCP hooks expose that JSON object as-is. +fn rewrite_mcp_updated_input( + mut invocation: ToolInvocation, + updated_input: Value, +) -> Result { + invocation.payload = match invocation.payload { + ToolPayload::Mcp { server, tool, .. } => ToolPayload::Mcp { + server, + tool, + raw_arguments: serde_json::to_string(&updated_input).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten MCP arguments: {err}" + )) + })?, + }, + payload => { + return Err(FunctionCallError::RespondToModel(format!( + "tool {} does not support hook input rewriting for payload {payload:?}", + invocation.tool_name.display() + ))); + } + }; + Ok(invocation) +} + +pub(crate) fn mcp_hook_tool_input(raw_arguments: &str) -> Value { + if raw_arguments.trim().is_empty() { + return Value::Object(Map::new()); + } + + serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) +} diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 659a7d3e549a..eeea95687e2a 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -2,6 +2,7 @@ pub(crate) mod code_mode; pub(crate) mod context; pub(crate) mod events; pub(crate) mod handlers; +pub(crate) mod hook_compat; pub(crate) mod hook_names; pub(crate) mod network_approval; pub(crate) mod orchestrator; diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 4547fe8ebfe2..c3aa087313a5 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -16,6 +16,7 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; +use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::tool_dispatch_trace::ToolDispatchTrace; use codex_hooks::HookEvent; @@ -69,10 +70,6 @@ pub trait ToolHandler: Send + Sync { async { false } } - fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { - None - } - fn post_tool_use_payload( &self, _invocation: &ToolInvocation, @@ -81,20 +78,6 @@ pub trait ToolHandler: Send + Sync { None } - /// Rebuilds a tool invocation from hook-facing `tool_input`. - /// - /// Tools that opt into input-rewriting hooks should invert the same stable - /// hook contract they expose from `pre_tool_use_payload`. - fn with_updated_hook_input( - &self, - _invocation: ToolInvocation, - _updated_input: Value, - ) -> Result { - Err(FunctionCallError::RespondToModel( - "tool does not support hook input rewriting".to_string(), - )) - } - /// Creates an optional consumer for streamed tool argument diffs. fn create_diff_consumer(&self) -> Option> { None @@ -181,14 +164,6 @@ trait AnyToolHandler: Send + Sync { fn is_mutating<'a>(&'a self, invocation: &'a ToolInvocation) -> BoxFuture<'a, bool>; - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; - - fn with_updated_hook_input( - &self, - invocation: ToolInvocation, - updated_input: Value, - ) -> Result; - fn create_diff_consumer(&self) -> Option>; fn handle_any<'a>( &'a self, @@ -208,18 +183,6 @@ where Box::pin(ToolHandler::is_mutating(self, invocation)) } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - ToolHandler::pre_tool_use_payload(self, invocation) - } - - fn with_updated_hook_input( - &self, - invocation: ToolInvocation, - updated_input: Value, - ) -> Result { - ToolHandler::with_updated_hook_input(self, invocation, updated_input) - } - fn create_diff_consumer(&self) -> Option> { ToolHandler::create_diff_consumer(self) } @@ -378,7 +341,7 @@ impl ToolRegistry { return Err(err); } - if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) { + if let Some(pre_tool_use_payload) = hook_compat::pre_tool_use_payload(&invocation) { match run_pre_tool_use_hooks( &invocation.session, &invocation.turn, @@ -396,7 +359,7 @@ impl ToolRegistry { crate::hook_runtime::PreToolUseHookResult::Continue { updated_input: Some(updated_input), } => { - invocation = handler.with_updated_hook_input(invocation, updated_input)?; + invocation = hook_compat::apply_updated_input(invocation, updated_input)?; } crate::hook_runtime::PreToolUseHookResult::Continue { updated_input: None, From be8d71927506bf5e19f1568804969c070aba29cd Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 14:28:12 -0700 Subject: [PATCH 14/20] Polish hook compatibility cleanup --- codex-rs/core/src/tools/handlers/apply_patch.rs | 6 +++++- codex-rs/core/src/tools/handlers/apply_patch_tests.rs | 5 +++-- codex-rs/core/src/tools/handlers/mcp.rs | 8 +++----- codex-rs/core/src/tools/handlers/shell_tests.rs | 5 +++-- codex-rs/core/src/tools/handlers/unified_exec_tests.rs | 5 +++-- codex-rs/core/src/tools/registry.rs | 5 +++-- 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index cccc517ba392..45ade8bbe0ff 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -240,7 +240,11 @@ fn write_permissions_for_paths( normalize_additional_permissions(permissions).ok() } -/// Extracts the raw patch text from either supported apply_patch payload form. +/// Extracts the raw patch text used as the command-shaped hook input for apply_patch. +/// +/// The apply_patch tool can arrive as the older JSON/function shape or as a +/// freeform custom tool call. Both represent the same file edit operation, so +/// hooks see the raw patch body in `tool_input.command` either way. pub(crate) fn apply_patch_payload_command(payload: &ToolPayload) -> Option { match payload { ToolPayload::Function { arguments } => parse_arguments::(arguments) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 4ee6bd245018..4fad39b79776 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -16,6 +16,7 @@ use tokio::sync::Mutex; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolInvocation; +use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PreToolUsePayload; @@ -50,7 +51,7 @@ async fn pre_tool_use_payload_uses_json_patch_input() { }; let invocation = invocation_for_payload(payload).await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&invocation), + hook_compat::pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), @@ -66,7 +67,7 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { }; let invocation = invocation_for_payload(payload).await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&invocation), + hook_compat::pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 1b9d6a613b0a..4536a00616e0 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -106,6 +106,7 @@ mod tests { use super::*; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolCallSource; + use crate::tools::hook_compat; use crate::turn_diff_tracker::TurnDiffTracker; use pretty_assertions::assert_eq; use serde_json::json; @@ -127,7 +128,7 @@ mod tests { }; let (session, turn) = make_session_and_context().await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { + hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -213,9 +214,6 @@ mod tests { #[test] fn mcp_hook_tool_input_defaults_empty_args_to_object() { - assert_eq!( - crate::tools::hook_compat::mcp_hook_tool_input(" "), - json!({}) - ); + assert_eq!(hook_compat::mcp_hook_tool_input(" "), json!({})); } } diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 9e5aff4d2138..e3208ffe63d8 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -17,6 +17,7 @@ use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::ShellCommandHandler; +use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; @@ -222,7 +223,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { let (session, turn) = make_session_and_context().await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { + hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -247,7 +248,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { let (session, turn) = make_session_and_context().await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { + hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), 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 451c5fa3b32e..c654ac98efe8 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -11,6 +11,7 @@ use crate::tools::context::ExecCommandToolOutput; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; @@ -185,7 +186,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { }; let (session, turn) = make_session_and_context().await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { + hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -209,7 +210,7 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { }; let (session, turn) = make_session_and_context().await; assert_eq!( - crate::tools::hook_compat::pre_tool_use_payload(&ToolInvocation { + hook_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index c3aa087313a5..c15850449b59 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::PreToolUseHookResult; 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; @@ -351,12 +352,12 @@ impl ToolRegistry { ) .await { - crate::hook_runtime::PreToolUseHookResult::Blocked(message) => { + PreToolUseHookResult::Blocked(message) => { let err = FunctionCallError::RespondToModel(message); dispatch_trace.record_failed(&err); return Err(err); } - crate::hook_runtime::PreToolUseHookResult::Continue { + PreToolUseHookResult::Continue { updated_input: Some(updated_input), } => { invocation = hook_compat::apply_updated_input(invocation, updated_input)?; From 5ecbcf609b53d910c7e3839676699e0ff3a9ef4f Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 6 May 2026 14:34:31 -0700 Subject: [PATCH 15/20] Move tool hook compatibility under hook runtime --- codex-rs/core/src/hook_runtime.rs | 2 ++ .../tool_compat.rs} | 15 ++++++++++++- .../src/tools/handlers/apply_patch_tests.rs | 8 +++---- codex-rs/core/src/tools/handlers/mcp.rs | 8 +++---- .../core/src/tools/handlers/shell_tests.rs | 10 ++++----- .../src/tools/handlers/unified_exec_tests.rs | 8 +++---- codex-rs/core/src/tools/mod.rs | 1 - codex-rs/core/src/tools/registry.rs | 22 ++++--------------- 8 files changed, 37 insertions(+), 37 deletions(-) rename codex-rs/core/src/{tools/hook_compat.rs => hook_runtime/tool_compat.rs} (95%) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 8b6b28cfc314..795821285684 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -38,6 +38,8 @@ use crate::session::turn_context::TurnContext; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; +pub(crate) mod tool_compat; + pub(crate) struct HookRuntimeOutcome { pub should_stop: bool, pub additional_contexts: Vec, diff --git a/codex-rs/core/src/tools/hook_compat.rs b/codex-rs/core/src/hook_runtime/tool_compat.rs similarity index 95% rename from codex-rs/core/src/tools/hook_compat.rs rename to codex-rs/core/src/hook_runtime/tool_compat.rs index 5821598e6679..f25fb60f4b85 100644 --- a/codex-rs/core/src/tools/hook_compat.rs +++ b/codex-rs/core/src/hook_runtime/tool_compat.rs @@ -11,7 +11,20 @@ use crate::tools::handlers::shell_command_payload_command; use crate::tools::handlers::shell_function_payload_command; use crate::tools::handlers::unified_exec::ExecCommandArgs; use crate::tools::hook_names::HookToolName; -use crate::tools::registry::PreToolUsePayload; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct PreToolUsePayload { + /// Hook-facing tool name model. + /// + /// The canonical name is serialized to hook stdin, while aliases are used + /// only for matcher compatibility. + pub(crate) tool_name: HookToolName, + /// Tool-specific input exposed at `tool_input`. + /// + /// Shell-like tools use `{ "command": ... }`; MCP tools use their resolved + /// JSON arguments. + pub(crate) tool_input: Value, +} /// Projects native tool payloads into the stable input shape exposed to hooks. /// diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 4fad39b79776..c046c38f5d7a 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -14,12 +14,12 @@ use std::sync::Arc; use tempfile::TempDir; use tokio::sync::Mutex; +use crate::hook_runtime::tool_compat; +use crate::hook_runtime::tool_compat::PreToolUsePayload; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolInvocation; -use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::turn_diff_tracker::TurnDiffTracker; fn sample_patch() -> &'static str { @@ -51,7 +51,7 @@ async fn pre_tool_use_payload_uses_json_patch_input() { }; let invocation = invocation_for_payload(payload).await; assert_eq!( - hook_compat::pre_tool_use_payload(&invocation), + tool_compat::pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), @@ -67,7 +67,7 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { }; let invocation = invocation_for_payload(payload).await; assert_eq!( - hook_compat::pre_tool_use_payload(&invocation), + tool_compat::pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 4536a00616e0..bb00ece39422 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -104,9 +104,9 @@ impl ToolHandler for McpHandler { #[cfg(test)] mod tests { use super::*; + use crate::hook_runtime::tool_compat; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolCallSource; - use crate::tools::hook_compat; use crate::turn_diff_tracker::TurnDiffTracker; use pretty_assertions::assert_eq; use serde_json::json; @@ -128,7 +128,7 @@ mod tests { }; let (session, turn) = make_session_and_context().await; assert_eq!( - hook_compat::pre_tool_use_payload(&ToolInvocation { + tool_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -138,7 +138,7 @@ mod tests { source: ToolCallSource::Direct, payload, }), - Some(crate::tools::registry::PreToolUsePayload { + Some(tool_compat::PreToolUsePayload { tool_name: HookToolName::new("mcp__memory__create_entities"), tool_input: json!({ "entities": [{ @@ -214,6 +214,6 @@ mod tests { #[test] fn mcp_hook_tool_input_defaults_empty_args_to_object() { - assert_eq!(hook_compat::mcp_hook_tool_input(" "), json!({})); + assert_eq!(tool_compat::mcp_hook_tool_input(" "), json!({})); } } diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index e3208ffe63d8..c6e4d70abd68 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -7,6 +7,7 @@ use core_test_support::test_path_buf; use pretty_assertions::assert_eq; use crate::exec_env::create_env; +use crate::hook_runtime::tool_compat; use crate::sandboxing::SandboxPermissions; use crate::session::tests::make_session_and_context; use crate::shell::Shell; @@ -17,7 +18,6 @@ use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::ShellCommandHandler; -use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; @@ -223,7 +223,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { let (session, turn) = make_session_and_context().await; assert_eq!( - hook_compat::pre_tool_use_payload(&ToolInvocation { + tool_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -233,7 +233,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(crate::tools::registry::PreToolUsePayload { + Some(tool_compat::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "bash -lc 'printf hi'" }), }) @@ -248,7 +248,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { let (session, turn) = make_session_and_context().await; assert_eq!( - hook_compat::pre_tool_use_payload(&ToolInvocation { + tool_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -258,7 +258,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(crate::tools::registry::PreToolUsePayload { + Some(tool_compat::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "printf shell command" }), }) 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 c654ac98efe8..b649fb797dfa 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -6,12 +6,12 @@ use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::sync::Arc; +use crate::hook_runtime::tool_compat; use crate::session::tests::make_session_and_context; use crate::tools::context::ExecCommandToolOutput; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; -use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; @@ -186,7 +186,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { }; let (session, turn) = make_session_and_context().await; assert_eq!( - hook_compat::pre_tool_use_payload(&ToolInvocation { + tool_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -196,7 +196,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(crate::tools::registry::PreToolUsePayload { + Some(tool_compat::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": "printf exec command" }), }) @@ -210,7 +210,7 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { }; let (session, turn) = make_session_and_context().await; assert_eq!( - hook_compat::pre_tool_use_payload(&ToolInvocation { + tool_compat::pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index eeea95687e2a..659a7d3e549a 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -2,7 +2,6 @@ pub(crate) mod code_mode; pub(crate) mod context; pub(crate) mod events; pub(crate) mod handlers; -pub(crate) mod hook_compat; pub(crate) mod hook_names; pub(crate) mod network_approval; pub(crate) mod orchestrator; diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index c15850449b59..749c9f575c99 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -9,6 +9,7 @@ use crate::hook_runtime::PreToolUseHookResult; 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; +use crate::hook_runtime::tool_compat; use crate::memory_usage::emit_metric_for_tool_read; use crate::sandbox_tags::permission_profile_policy_tag; use crate::sandbox_tags::permission_profile_sandbox_tag; @@ -17,7 +18,6 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; -use crate::tools::hook_compat; use crate::tools::hook_names::HookToolName; use crate::tools::tool_dispatch_trace::ToolDispatchTrace; use codex_hooks::HookEvent; @@ -131,20 +131,6 @@ impl AnyToolResult { } } -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct PreToolUsePayload { - /// Hook-facing tool name model. - /// - /// The canonical name is serialized to hook stdin, while aliases are used - /// only for matcher compatibility. - pub(crate) tool_name: HookToolName, - /// Tool-specific input exposed at `tool_input`. - /// - /// Shell-like tools use `{ "command": ... }`; MCP tools use their resolved - /// JSON arguments. - pub(crate) tool_input: Value, -} - #[derive(Debug, Clone, PartialEq)] pub(crate) struct PostToolUsePayload { /// Hook-facing tool name model. @@ -342,7 +328,7 @@ impl ToolRegistry { return Err(err); } - if let Some(pre_tool_use_payload) = hook_compat::pre_tool_use_payload(&invocation) { + if let Some(pre_tool_use_payload) = tool_compat::pre_tool_use_payload(&invocation) { match run_pre_tool_use_hooks( &invocation.session, &invocation.turn, @@ -360,9 +346,9 @@ impl ToolRegistry { PreToolUseHookResult::Continue { updated_input: Some(updated_input), } => { - invocation = hook_compat::apply_updated_input(invocation, updated_input)?; + invocation = tool_compat::apply_updated_input(invocation, updated_input)?; } - crate::hook_runtime::PreToolUseHookResult::Continue { + PreToolUseHookResult::Continue { updated_input: None, } => {} } From 240213adb4f580e7f79cc0ab06d58409899aa416 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 8 May 2026 13:58:48 -0400 Subject: [PATCH 16/20] Address updatedInput review feedback --- codex-rs/core/src/hook_runtime/tool_compat.rs | 14 ++- codex-rs/core/src/tools/handlers/mcp.rs | 57 ++++++++++++ codex-rs/hooks/src/engine/dispatcher.rs | 33 ++++--- codex-rs/hooks/src/engine/output_parser.rs | 6 +- .../hooks/src/events/permission_request.rs | 1 + codex-rs/hooks/src/events/post_tool_use.rs | 1 + codex-rs/hooks/src/events/pre_tool_use.rs | 87 ++++++++++++++++++- codex-rs/hooks/src/events/session_start.rs | 1 + codex-rs/hooks/src/events/stop.rs | 1 + .../hooks/src/events/user_prompt_submit.rs | 1 + 10 files changed, 184 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/hook_runtime/tool_compat.rs b/codex-rs/core/src/hook_runtime/tool_compat.rs index f25fb60f4b85..8d94f586c747 100644 --- a/codex-rs/core/src/hook_runtime/tool_compat.rs +++ b/codex-rs/core/src/hook_runtime/tool_compat.rs @@ -38,6 +38,10 @@ pub(crate) struct PreToolUsePayload { /// - MCP tools already use arbitrary JSON argument objects, so hooks see those /// arguments directly rather than through a compatibility shape. pub(crate) fn pre_tool_use_payload(invocation: &ToolInvocation) -> Option { + if matches!(invocation.payload, ToolPayload::Mcp { .. }) { + return mcp_payload(invocation); + } + match invocation.tool_name.name.as_str() { "shell" | "container.exec" => { shell_function_payload_command(&invocation.payload).map(bash_payload) @@ -66,6 +70,11 @@ pub(crate) fn apply_updated_input( invocation: ToolInvocation, updated_input: Value, ) -> Result { + // MCP tool names can share builtin basenames, so payload kind is authoritative here. + if matches!(invocation.payload, ToolPayload::Mcp { .. }) { + return rewrite_mcp_updated_input(invocation, updated_input); + } + match invocation.tool_name.name.as_str() { "shell" => rewrite_shell_function_updated_input(invocation, updated_input, "shell"), "container.exec" => { @@ -75,7 +84,10 @@ pub(crate) fn apply_updated_input( "shell_command" => rewrite_shell_command_updated_input(invocation, updated_input), "exec_command" => rewrite_exec_command_updated_input(invocation, updated_input), "apply_patch" => rewrite_apply_patch_updated_input(invocation, updated_input), - _ => rewrite_mcp_updated_input(invocation, updated_input), + _ => Err(FunctionCallError::RespondToModel(format!( + "tool {} does not support hook input rewriting", + invocation.tool_name.display() + ))), } } diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index bb00ece39422..a7a57c21cd01 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -150,6 +150,63 @@ mod tests { ); } + #[tokio::test] + async fn mcp_pre_tool_use_payload_keeps_builtin_like_tool_names_namespaced() { + let payload = ToolPayload::Mcp { + server: "foo".to_string(), + tool: "exec_command".to_string(), + raw_arguments: json!({ "message": "hello" }).to_string(), + }; + let (session, turn) = make_session_and_context().await; + + assert_eq!( + tool_compat::pre_tool_use_payload(&ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-mcp-pre-builtin-like".to_string(), + tool_name: codex_tools::ToolName::namespaced("mcp__foo__", "exec_command"), + source: ToolCallSource::Direct, + payload, + }), + Some(tool_compat::PreToolUsePayload { + tool_name: HookToolName::new("mcp__foo__exec_command"), + tool_input: json!({ "message": "hello" }), + }) + ); + } + + #[tokio::test] + async fn mcp_updated_input_rewrites_builtin_like_tool_names_as_mcp() { + let payload = ToolPayload::Mcp { + server: "foo".to_string(), + tool: "exec_command".to_string(), + raw_arguments: json!({ "message": "hello" }).to_string(), + }; + let (session, turn) = make_session_and_context().await; + + let invocation = tool_compat::apply_updated_input( + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-mcp-rewrite-builtin-like".to_string(), + tool_name: codex_tools::ToolName::namespaced("mcp__foo__", "exec_command"), + source: ToolCallSource::Direct, + payload, + }, + json!({ "message": "rewritten" }), + ) + .expect("MCP rewrite should succeed"); + + let ToolPayload::Mcp { raw_arguments, .. } = invocation.payload else { + panic!("builtin-like MCP tool should stay MCP"); + }; + assert_eq!(raw_arguments, json!({ "message": "rewritten" }).to_string()); + } + #[tokio::test] async fn mcp_post_tool_use_payload_uses_model_tool_name_args_and_result() { let payload = ToolPayload::Mcp { diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index c44b1fe69d77..0bf23547ca7f 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -1,6 +1,7 @@ use std::path::Path; -use futures::future::join_all; +use futures::StreamExt; +use futures::stream::FuturesUnordered; use codex_protocol::protocol::HookCompletedEvent; use codex_protocol::protocol::HookEventName; @@ -20,6 +21,7 @@ use crate::events::common::matches_matcher; pub(crate) struct ParsedHandler { pub completed: HookCompletedEvent, pub data: T, + pub completion_order: usize, } pub(crate) fn select_handlers( @@ -88,18 +90,25 @@ pub(crate) async fn execute_handlers( turn_id: Option, parse: fn(&ConfiguredHandler, CommandRunResult, Option) -> ParsedHandler, ) -> Vec> { - let results = join_all( - handlers - .iter() - .map(|handler| run_command(shell, handler, &input_json, cwd)), - ) - .await; + let mut pending = FuturesUnordered::new(); + for (configured_order, handler) in handlers.into_iter().enumerate() { + let input_json = input_json.clone(); + let turn_id = turn_id.clone(); + pending.push(async move { + let result = run_command(shell, &handler, &input_json, cwd).await; + (configured_order, parse(&handler, result, turn_id)) + }); + } - handlers - .into_iter() - .zip(results) - .map(|(handler, result)| parse(&handler, result, turn_id.clone())) - .collect() + let mut completed = Vec::new(); + let mut completion_order = 0; + while let Some((configured_order, mut parsed)) = pending.next().await { + parsed.completion_order = completion_order; + completion_order += 1; + completed.push((configured_order, parsed)); + } + completed.sort_by_key(|(configured_order, _)| *configured_order); + completed.into_iter().map(|(_, parsed)| parsed).collect() } pub(crate) fn completed_summary( diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 9c3d442dacf4..b46ee638e05f 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -363,7 +363,11 @@ fn unsupported_pre_tool_use_hook_specific_output( Some("PreToolUse hook returned updatedInput without permissionDecision:allow".to_string()) } else { match output.permission_decision { - Some(PreToolUsePermissionDecisionWire::Allow) => None, + Some(PreToolUsePermissionDecisionWire::Allow) => { + output.updated_input.is_none().then(|| { + "PreToolUse hook returned unsupported permissionDecision:allow".to_string() + }) + } Some(PreToolUsePermissionDecisionWire::Ask) => { Some("PreToolUse hook returned unsupported permissionDecision:ask".to_string()) } diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 2cebd0e002ed..7193d7e6741f 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -281,6 +281,7 @@ fn parse_completed( dispatcher::ParsedHandler { completed, data: PermissionRequestHandlerData { decision }, + completion_order: 0, } } diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 63045ef4258b..afa1131452f5 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -298,6 +298,7 @@ fn parse_completed( additional_contexts_for_model, feedback_messages_for_model, }, + completion_order: 0, } } diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 9b9086627496..869658660d03 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -122,10 +122,7 @@ pub(crate) async fn run( let updated_input = if should_block { None } else { - results - .iter() - .rev() - .find_map(|result| result.data.updated_input.clone()) + latest_updated_input(&results) }; PreToolUseOutcome { @@ -142,6 +139,26 @@ pub(crate) async fn run( } } +/// Chooses the rewrite from the hook that actually finished last. +/// +/// Hook results stay in configured order for stable reporting, but the +/// `PreToolUse` contract resolves competing rewrites by completion order. +fn latest_updated_input( + results: &[dispatcher::ParsedHandler], +) -> Option { + results + .iter() + .filter_map(|result| { + result + .data + .updated_input + .clone() + .map(|updated_input| (result.completion_order, updated_input)) + }) + .max_by_key(|(completion_order, _)| *completion_order) + .map(|(_, updated_input)| updated_input) +} + /// Serializes command stdin for a selected `PreToolUse` hook. /// /// Handler selection may include internal matcher aliases, but hook stdin keeps @@ -276,6 +293,7 @@ fn parse_completed( additional_contexts_for_model, updated_input, }, + completion_order: 0, } } @@ -302,6 +320,7 @@ mod tests { use super::PreToolUseHandlerData; use super::command_input_json; + use super::latest_updated_input; use super::parse_completed; use super::preview; use crate::engine::ConfiguredHandler; @@ -376,6 +395,66 @@ mod tests { assert_eq!(parsed.completed.run.entries, vec![]); } + #[test] + fn last_completed_updated_input_wins() { + let mut later_configured = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","updatedInput":{"command":"echo configured later"}}}"#, + "", + ), + Some("turn-1".to_string()), + ); + later_configured.completion_order = 0; + let mut earlier_configured = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","updatedInput":{"command":"echo finished later"}}}"#, + "", + ), + Some("turn-1".to_string()), + ); + earlier_configured.completion_order = 1; + + assert_eq!( + latest_updated_input(&[later_configured, earlier_configured]), + Some(serde_json::json!({ "command": "echo finished later" })) + ); + } + + #[test] + fn permission_decision_allow_without_updated_input_fails_open() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + additional_contexts_for_model: Vec::new(), + updated_input: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "PreToolUse hook returned unsupported permissionDecision:allow".to_string(), + }] + ); + } + #[test] fn deprecated_block_decision_blocks_processing() { let parsed = parse_completed( diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index f064f67b2029..0758bec7ee95 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -234,6 +234,7 @@ fn parse_completed( stop_reason, additional_contexts_for_model, }, + completion_order: 0, } } diff --git a/codex-rs/hooks/src/events/stop.rs b/codex-rs/hooks/src/events/stop.rs index 8fc176a47439..3cd2d4427019 100644 --- a/codex-rs/hooks/src/events/stop.rs +++ b/codex-rs/hooks/src/events/stop.rs @@ -259,6 +259,7 @@ fn parse_completed( block_reason, continuation_fragments, }, + completion_order: 0, } } diff --git a/codex-rs/hooks/src/events/user_prompt_submit.rs b/codex-rs/hooks/src/events/user_prompt_submit.rs index a04711eb4098..00075a239e34 100644 --- a/codex-rs/hooks/src/events/user_prompt_submit.rs +++ b/codex-rs/hooks/src/events/user_prompt_submit.rs @@ -255,6 +255,7 @@ fn parse_completed( stop_reason, additional_contexts_for_model, }, + completion_order: 0, } } From 8cb4c6f31b13d8862a20ef1351405ec521e71a52 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 8 May 2026 14:33:17 -0400 Subject: [PATCH 17/20] Clarify non-MCP hook payload fallback --- codex-rs/core/src/hook_runtime/tool_compat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/core/src/hook_runtime/tool_compat.rs b/codex-rs/core/src/hook_runtime/tool_compat.rs index 8d94f586c747..e2264dd1029d 100644 --- a/codex-rs/core/src/hook_runtime/tool_compat.rs +++ b/codex-rs/core/src/hook_runtime/tool_compat.rs @@ -55,7 +55,7 @@ pub(crate) fn pre_tool_use_payload(invocation: &ToolInvocation) -> Option mcp_payload(invocation), + _ => None, } } From 1e479e0d11bf424f3ffaab8450c8010957251a46 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 11 May 2026 20:37:57 -0400 Subject: [PATCH 18/20] Move hook input rewrites back to handlers --- codex-rs/core/src/hook_runtime.rs | 2 - codex-rs/core/src/hook_runtime/tool_compat.rs | 301 ------------------ .../core/src/tools/handlers/apply_patch.rs | 30 +- .../src/tools/handlers/apply_patch_tests.rs | 22 +- codex-rs/core/src/tools/handlers/mcp.rs | 88 +++-- codex-rs/core/src/tools/handlers/mod.rs | 45 ++- codex-rs/core/src/tools/handlers/shell.rs | 42 ++- .../tools/handlers/shell/container_exec.rs | 15 + .../src/tools/handlers/shell/local_shell.rs | 29 ++ .../src/tools/handlers/shell/shell_command.rs | 31 ++ .../src/tools/handlers/shell/shell_handler.rs | 15 + .../core/src/tools/handlers/shell_tests.rs | 12 +- .../handlers/unified_exec/exec_command.rs | 38 +++ .../src/tools/handlers/unified_exec_tests.rs | 10 +- codex-rs/core/src/tools/registry.rs | 60 +++- codex-rs/core/tests/suite/hooks.rs | 4 +- 16 files changed, 376 insertions(+), 368 deletions(-) delete mode 100644 codex-rs/core/src/hook_runtime/tool_compat.rs diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index cf0240756b9c..175813a544ad 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -39,8 +39,6 @@ use crate::session::turn_context::TurnContext; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; -pub(crate) mod tool_compat; - pub(crate) struct HookRuntimeOutcome { pub should_stop: bool, pub additional_contexts: Vec, diff --git a/codex-rs/core/src/hook_runtime/tool_compat.rs b/codex-rs/core/src/hook_runtime/tool_compat.rs deleted file mode 100644 index e2264dd1029d..000000000000 --- a/codex-rs/core/src/hook_runtime/tool_compat.rs +++ /dev/null @@ -1,301 +0,0 @@ -use serde_json::Map; -use serde_json::Value; - -use crate::function_tool::FunctionCallError; -use crate::tools::context::ToolInvocation; -use crate::tools::context::ToolPayload; -use crate::tools::handlers::apply_patch::apply_patch_payload_command; -use crate::tools::handlers::local_shell_payload_command; -use crate::tools::handlers::parse_arguments; -use crate::tools::handlers::shell_command_payload_command; -use crate::tools::handlers::shell_function_payload_command; -use crate::tools::handlers::unified_exec::ExecCommandArgs; -use crate::tools::hook_names::HookToolName; - -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct PreToolUsePayload { - /// Hook-facing tool name model. - /// - /// The canonical name is serialized to hook stdin, while aliases are used - /// only for matcher compatibility. - pub(crate) tool_name: HookToolName, - /// Tool-specific input exposed at `tool_input`. - /// - /// Shell-like tools use `{ "command": ... }`; MCP tools use their resolved - /// JSON arguments. - pub(crate) tool_input: Value, -} - -/// Projects native tool payloads into the stable input shape exposed to hooks. -/// -/// The hook protocol intentionally hides a few native tool-schema differences: -/// -/// - Bash-like tools are exposed as `Bash` with `{ "command": }`, -/// even though their native payloads store commands as argv, `command`, or -/// `cmd` depending on the concrete tool. -/// - `apply_patch` exposes its raw patch body through `{ "command": }` -/// for compatibility with existing hook consumers. -/// - MCP tools already use arbitrary JSON argument objects, so hooks see those -/// arguments directly rather than through a compatibility shape. -pub(crate) fn pre_tool_use_payload(invocation: &ToolInvocation) -> Option { - if matches!(invocation.payload, ToolPayload::Mcp { .. }) { - return mcp_payload(invocation); - } - - match invocation.tool_name.name.as_str() { - "shell" | "container.exec" => { - shell_function_payload_command(&invocation.payload).map(bash_payload) - } - "local_shell" => local_shell_payload_command(&invocation.payload).map(bash_payload), - "shell_command" => shell_command_payload_command(&invocation.payload).map(bash_payload), - "exec_command" => exec_command_payload_command(&invocation.payload).map(bash_payload), - "apply_patch" => { - apply_patch_payload_command(&invocation.payload).map(|command| PreToolUsePayload { - tool_name: HookToolName::apply_patch(), - tool_input: serde_json::json!({ "command": command }), - }) - } - _ => None, - } -} - -/// Rebuilds native tool payloads from hook-facing `updatedInput`. -/// -/// This is the inverse of [`pre_tool_use_payload`]: Bash-like and -/// `apply_patch` updates come back through the compatibility `{ "command": ... }` -/// shape and must be written into each tool's native schema, while MCP updates -/// replace the raw argument object wholesale because the hook-facing and native -/// representations are the same. -pub(crate) fn apply_updated_input( - invocation: ToolInvocation, - updated_input: Value, -) -> Result { - // MCP tool names can share builtin basenames, so payload kind is authoritative here. - if matches!(invocation.payload, ToolPayload::Mcp { .. }) { - return rewrite_mcp_updated_input(invocation, updated_input); - } - - match invocation.tool_name.name.as_str() { - "shell" => rewrite_shell_function_updated_input(invocation, updated_input, "shell"), - "container.exec" => { - rewrite_shell_function_updated_input(invocation, updated_input, "container.exec") - } - "local_shell" => rewrite_local_shell_updated_input(invocation, updated_input), - "shell_command" => rewrite_shell_command_updated_input(invocation, updated_input), - "exec_command" => rewrite_exec_command_updated_input(invocation, updated_input), - "apply_patch" => rewrite_apply_patch_updated_input(invocation, updated_input), - _ => Err(FunctionCallError::RespondToModel(format!( - "tool {} does not support hook input rewriting", - invocation.tool_name.display() - ))), - } -} - -fn bash_payload(command: String) -> PreToolUsePayload { - PreToolUsePayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ "command": command }), - } -} - -fn exec_command_payload_command(payload: &ToolPayload) -> Option { - let ToolPayload::Function { arguments } = payload else { - return None; - }; - - parse_arguments::(arguments) - .ok() - .map(|args| args.cmd) -} - -fn mcp_payload(invocation: &ToolInvocation) -> Option { - let ToolPayload::Mcp { raw_arguments, .. } = &invocation.payload else { - return None; - }; - - Some(PreToolUsePayload { - tool_name: HookToolName::new(invocation.tool_name.display()), - tool_input: mcp_hook_tool_input(raw_arguments), - }) -} - -fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { - updated_input - .get("command") - .and_then(Value::as_str) - .ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned updatedInput without string field `command`".to_string(), - ) - }) -} - -fn rewrite_function_arguments( - arguments: &str, - tool_name: &str, - rewrite: impl FnOnce(&mut Map), -) -> Result { - let mut arguments: Value = parse_arguments(arguments)?; - let Value::Object(arguments) = &mut arguments else { - return Err(FunctionCallError::RespondToModel(format!( - "{tool_name} arguments must be an object" - ))); - }; - rewrite(arguments); - serde_json::to_string(&arguments).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten {tool_name} arguments: {err}" - )) - }) -} - -fn rewrite_function_string_argument( - arguments: &str, - tool_name: &str, - field_name: &str, - value: &str, -) -> Result { - rewrite_function_arguments(arguments, tool_name, |arguments| { - arguments.insert(field_name.to_string(), Value::String(value.to_string())); - }) -} - -/// Rehydrates legacy function-style shell tools from hook-facing command text. -fn rewrite_shell_function_updated_input( - mut invocation: ToolInvocation, - updated_input: Value, - tool_name: &str, -) -> Result { - let ToolPayload::Function { arguments } = invocation.payload else { - return Err(FunctionCallError::RespondToModel(format!( - "hook input rewrite received unsupported {tool_name} payload" - ))); - }; - let command = shlex::split(updated_hook_command(&updated_input)?).ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned shell input with an invalid command string".to_string(), - ) - })?; - invocation.payload = ToolPayload::Function { - arguments: rewrite_function_arguments(&arguments, tool_name, |arguments| { - arguments.insert( - "command".to_string(), - Value::Array(command.into_iter().map(Value::String).collect()), - ); - })?, - }; - Ok(invocation) -} - -/// Rehydrates `local_shell` argv from hook-facing command text. -fn rewrite_local_shell_updated_input( - mut invocation: ToolInvocation, - updated_input: Value, -) -> Result { - let command = updated_hook_command(&updated_input)?; - invocation.payload = match invocation.payload { - ToolPayload::LocalShell { mut params } => { - params.command = shlex::split(command).ok_or_else(|| { - FunctionCallError::RespondToModel( - "hook returned shell input with an invalid command string".to_string(), - ) - })?; - ToolPayload::LocalShell { params } - } - payload => payload, - }; - Ok(invocation) -} - -/// Stores hook-facing command text back into the native `shell_command.command`. -fn rewrite_shell_command_updated_input( - mut invocation: ToolInvocation, - updated_input: Value, -) -> Result { - let ToolPayload::Function { arguments } = invocation.payload else { - return Err(FunctionCallError::RespondToModel( - "hook input rewrite received unsupported shell_command payload".to_string(), - )); - }; - invocation.payload = ToolPayload::Function { - arguments: rewrite_function_string_argument( - &arguments, - "shell_command", - "command", - updated_hook_command(&updated_input)?, - )?, - }; - Ok(invocation) -} - -/// Stores hook-facing command text back into the native `exec_command.cmd`. -fn rewrite_exec_command_updated_input( - mut invocation: ToolInvocation, - updated_input: Value, -) -> Result { - let ToolPayload::Function { arguments } = invocation.payload else { - return Err(FunctionCallError::RespondToModel( - "hook input rewrite received unsupported exec_command payload".to_string(), - )); - }; - invocation.payload = ToolPayload::Function { - arguments: rewrite_function_string_argument( - &arguments, - "exec_command", - "cmd", - updated_hook_command(&updated_input)?, - )?, - }; - Ok(invocation) -} - -/// Stores hook-facing patch text back into the native apply_patch payload form. -fn rewrite_apply_patch_updated_input( - mut invocation: ToolInvocation, - updated_input: Value, -) -> Result { - let patch = updated_hook_command(&updated_input)?; - invocation.payload = match invocation.payload { - ToolPayload::Function { arguments } => ToolPayload::Function { - arguments: rewrite_function_string_argument(&arguments, "apply_patch", "input", patch)?, - }, - ToolPayload::Custom { .. } => ToolPayload::Custom { - input: patch.to_string(), - }, - payload => payload, - }; - Ok(invocation) -} - -/// Replaces MCP raw arguments directly because MCP hooks expose that JSON object as-is. -fn rewrite_mcp_updated_input( - mut invocation: ToolInvocation, - updated_input: Value, -) -> Result { - invocation.payload = match invocation.payload { - ToolPayload::Mcp { server, tool, .. } => ToolPayload::Mcp { - server, - tool, - raw_arguments: serde_json::to_string(&updated_input).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten MCP arguments: {err}" - )) - })?, - }, - payload => { - return Err(FunctionCallError::RespondToModel(format!( - "tool {} does not support hook input rewriting for payload {payload:?}", - invocation.tool_name.display() - ))); - } - }; - Ok(invocation) -} - -pub(crate) fn mcp_hook_tool_input(raw_arguments: &str) -> Value { - if raw_arguments.trim().is_empty() { - return Value::Object(Map::new()); - } - - serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) -} diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 9063c71421b6..b50cf093051e 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -22,9 +22,11 @@ use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::apply_patch_spec::create_apply_patch_freeform_tool; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolArgumentDiffConsumer; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; @@ -241,11 +243,7 @@ fn write_permissions_for_paths( } /// Extracts the raw patch text used as the command-shaped hook input for apply_patch. -/// -/// The apply_patch tool can arrive as the older JSON/function shape or as a -/// freeform custom tool call. Both represent the same file edit operation, so -/// hooks see the raw patch body in `tool_input.command` either way. -pub(crate) fn apply_patch_payload_command(payload: &ToolPayload) -> Option { +fn apply_patch_payload_command(payload: &ToolPayload) -> Option { match payload { ToolPayload::Custom { input } => Some(input.clone()), _ => None, @@ -313,6 +311,28 @@ impl ToolHandler for ApplyPatchHandler { Some(Box::::default()) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + apply_patch_payload_command(&invocation.payload).map(|command| PreToolUsePayload { + tool_name: HookToolName::apply_patch(), + tool_input: serde_json::json!({ "command": command }), + }) + } + + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + let patch = updated_hook_command(&updated_input)?; + invocation.payload = match invocation.payload { + ToolPayload::Custom { .. } => ToolPayload::Custom { + input: patch.to_string(), + }, + payload => payload, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index cdbb214540eb..f1b6ac8fae57 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -14,12 +14,11 @@ use std::sync::Arc; use tempfile::TempDir; use tokio::sync::Mutex; -use crate::hook_runtime::tool_compat; -use crate::hook_runtime::tool_compat::PreToolUsePayload; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolInvocation; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::turn_diff_tracker::TurnDiffTracker; fn sample_patch() -> &'static str { @@ -43,22 +42,6 @@ async fn invocation_for_payload(payload: ToolPayload) -> ToolInvocation { } } -#[tokio::test] -async fn pre_tool_use_payload_uses_json_patch_input() { - let patch = sample_patch(); - let payload = ToolPayload::Function { - arguments: json!({ "input": patch }).to_string(), - }; - let invocation = invocation_for_payload(payload).await; - assert_eq!( - tool_compat::pre_tool_use_payload(&invocation), - Some(PreToolUsePayload { - tool_name: HookToolName::apply_patch(), - tool_input: json!({ "command": patch }), - }) - ); -} - #[tokio::test] async fn pre_tool_use_payload_uses_freeform_patch_input() { let patch = sample_patch(); @@ -66,8 +49,9 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { input: patch.to_string(), }; let invocation = invocation_for_payload(payload).await; + let handler = ApplyPatchHandler; assert_eq!( - tool_compat::pre_tool_use_payload(&invocation), + handler.pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), tool_input: json!({ "command": patch }), diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 1cb1dd372346..35752af589bd 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -11,9 +11,12 @@ use crate::tools::context::ToolPayload; use crate::tools::flat_tool_name; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use serde_json::Map; +use serde_json::Value; pub struct McpHandler { tool_name: ToolName, @@ -47,6 +50,31 @@ impl ToolHandler for McpHandler { tool_input: mcp_hook_tool_input(raw_arguments), }) } + + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: Value, + ) -> Result { + invocation.payload = match invocation.payload { + ToolPayload::Mcp { server, tool, .. } => ToolPayload::Mcp { + server, + tool, + raw_arguments: serde_json::to_string(&updated_input).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten MCP arguments: {err}" + )) + })?, + }, + payload => { + return Err(FunctionCallError::RespondToModel(format!( + "tool {} does not support hook input rewriting for payload {payload:?}", + flat_tool_name(&self.tool_name) + ))); + } + }; + Ok(invocation) + } fn post_tool_use_payload( &self, invocation: &ToolInvocation, @@ -115,10 +143,17 @@ impl ToolHandler for McpHandler { } } +fn mcp_hook_tool_input(raw_arguments: &str) -> Value { + if raw_arguments.trim().is_empty() { + return Value::Object(Map::new()); + } + + serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) +} + #[cfg(test)] mod tests { use super::*; - use crate::hook_runtime::tool_compat; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolCallSource; use crate::turn_diff_tracker::TurnDiffTracker; @@ -141,8 +176,12 @@ mod tests { .to_string(), }; let (session, turn) = make_session_and_context().await; + let handler = McpHandler::new(codex_tools::ToolName::namespaced( + "mcp__memory__", + "create_entities", + )); assert_eq!( - tool_compat::pre_tool_use_payload(&ToolInvocation { + handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -152,7 +191,7 @@ mod tests { source: ToolCallSource::Direct, payload, }), - Some(tool_compat::PreToolUsePayload { + Some(PreToolUsePayload { tool_name: HookToolName::new("mcp__memory__create_entities"), tool_input: json!({ "entities": [{ @@ -172,9 +211,13 @@ mod tests { raw_arguments: json!({ "message": "hello" }).to_string(), }; let (session, turn) = make_session_and_context().await; + let handler = McpHandler::new(codex_tools::ToolName::namespaced( + "mcp__foo__", + "exec_command", + )); assert_eq!( - tool_compat::pre_tool_use_payload(&ToolInvocation { + handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -184,7 +227,7 @@ mod tests { source: ToolCallSource::Direct, payload, }), - Some(tool_compat::PreToolUsePayload { + Some(PreToolUsePayload { tool_name: HookToolName::new("mcp__foo__exec_command"), tool_input: json!({ "message": "hello" }), }) @@ -199,21 +242,26 @@ mod tests { raw_arguments: json!({ "message": "hello" }).to_string(), }; let (session, turn) = make_session_and_context().await; + let handler = McpHandler::new(codex_tools::ToolName::namespaced( + "mcp__foo__", + "exec_command", + )); - let invocation = tool_compat::apply_updated_input( - ToolInvocation { - session: session.into(), - turn: turn.into(), - cancellation_token: tokio_util::sync::CancellationToken::new(), - tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), - call_id: "call-mcp-rewrite-builtin-like".to_string(), - tool_name: codex_tools::ToolName::namespaced("mcp__foo__", "exec_command"), - source: ToolCallSource::Direct, - payload, - }, - json!({ "message": "rewritten" }), - ) - .expect("MCP rewrite should succeed"); + let invocation = handler + .with_updated_hook_input( + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-mcp-rewrite-builtin-like".to_string(), + tool_name: codex_tools::ToolName::namespaced("mcp__foo__", "exec_command"), + source: ToolCallSource::Direct, + payload, + }, + json!({ "message": "rewritten" }), + ) + .expect("MCP rewrite should succeed"); let ToolPayload::Mcp { raw_arguments, .. } = invocation.payload else { panic!("builtin-like MCP tool should stay MCP"); @@ -285,6 +333,6 @@ mod tests { #[test] fn mcp_hook_tool_input_defaults_empty_args_to_object() { - assert_eq!(tool_compat::mcp_hook_tool_input(" "), json!({})); + assert_eq!(mcp_hook_tool_input(" "), json!({})); } } diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index b396d3436d75..4438794f1e39 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -37,6 +37,7 @@ use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::Deserialize; +use serde_json::Map; use serde_json::Value; use std::path::Path; @@ -67,9 +68,6 @@ pub use shell::LocalShellHandler; pub use shell::ShellCommandHandler; pub(crate) use shell::ShellCommandHandlerOptions; pub use shell::ShellHandler; -pub(crate) use shell::local_shell_payload_command; -pub(crate) use shell::shell_command_payload_command; -pub(crate) use shell::shell_function_payload_command; pub use test_sync::TestSyncHandler; pub use tool_search::ToolSearchHandler; pub use unavailable_tool::UnavailableToolHandler; @@ -88,6 +86,47 @@ where }) } +pub(crate) fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { + updated_input + .get("command") + .and_then(Value::as_str) + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned updatedInput without string field `command`".to_string(), + ) + }) +} + +pub(crate) fn rewrite_function_arguments( + arguments: &str, + tool_name: &str, + rewrite: impl FnOnce(&mut Map), +) -> Result { + let mut arguments: Value = parse_arguments(arguments)?; + let Value::Object(arguments) = &mut arguments else { + return Err(FunctionCallError::RespondToModel(format!( + "{tool_name} arguments must be an object" + ))); + }; + rewrite(arguments); + serde_json::to_string(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten {tool_name} arguments: {err}" + )) + }) +} + +pub(crate) fn rewrite_function_string_argument( + arguments: &str, + tool_name: &str, + field_name: &str, + value: &str, +) -> Result { + rewrite_function_arguments(arguments, tool_name, |arguments| { + arguments.insert(field_name.to_string(), Value::String(value.to_string())); + }) +} + fn parse_arguments_with_base_path( arguments: &str, base_path: &AbsolutePathBuf, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 203fce9f08f2..dc3446ad64f2 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -19,9 +19,12 @@ use crate::tools::handlers::apply_patch::intercept_apply_patch; use crate::tools::handlers::implicit_granted_permissions; use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; +use crate::tools::handlers::rewrite_function_arguments; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::runtimes::shell::ShellRequest; use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::runtimes::shell::ShellRuntimeBackend; @@ -41,7 +44,7 @@ pub use shell_command::ShellCommandHandler; pub(crate) use shell_command::ShellCommandHandlerOptions; pub use shell_handler::ShellHandler; -pub(crate) fn shell_function_payload_command(payload: &ToolPayload) -> Option { +fn shell_function_payload_command(payload: &ToolPayload) -> Option { let ToolPayload::Function { arguments } = payload else { return None; }; @@ -51,7 +54,7 @@ pub(crate) fn shell_function_payload_command(payload: &ToolPayload) -> Option Option { +fn local_shell_payload_command(payload: &ToolPayload) -> Option { let ToolPayload::LocalShell { params } = payload else { return None; }; @@ -61,7 +64,7 @@ pub(crate) fn local_shell_payload_command(payload: &ToolPayload) -> Option Option { +fn shell_command_payload_command(payload: &ToolPayload) -> Option { let ToolPayload::Function { arguments } = payload else { return None; }; @@ -85,6 +88,39 @@ struct RunExecLikeArgs { shell_runtime_backend: ShellRuntimeBackend, } +fn shell_function_pre_tool_use_payload(invocation: &ToolInvocation) -> Option { + shell_function_payload_command(&invocation.payload).map(|command| PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": command }), + }) +} + +fn rewrite_shell_function_updated_hook_input( + mut invocation: ToolInvocation, + updated_input: JsonValue, + tool_name: &str, +) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel(format!( + "hook input rewrite received unsupported {tool_name} payload" + ))); + }; + let command = shlex::split(updated_hook_command(&updated_input)?).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_arguments(&arguments, tool_name, |arguments| { + arguments.insert( + "command".to_string(), + JsonValue::Array(command.into_iter().map(JsonValue::String).collect()), + ); + })?, + }; + Ok(invocation) +} + fn shell_function_post_tool_use_payload( invocation: &ToolInvocation, result: &FunctionToolOutput, diff --git a/codex-rs/core/src/tools/handlers/shell/container_exec.rs b/codex-rs/core/src/tools/handlers/shell/container_exec.rs index a17327e009ce..24af6d4f0010 100644 --- a/codex-rs/core/src/tools/handlers/shell/container_exec.rs +++ b/codex-rs/core/src/tools/handlers/shell/container_exec.rs @@ -9,13 +9,16 @@ use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; use super::RunExecLikeArgs; +use super::rewrite_shell_function_updated_hook_input; use super::run_exec_like; use super::shell_function_post_tool_use_payload; +use super::shell_function_pre_tool_use_payload; use super::shell_handler::ShellHandler; pub struct ContainerExecHandler; @@ -45,6 +48,18 @@ impl ToolHandler for ContainerExecHandler { .unwrap_or(true) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + shell_function_pre_tool_use_payload(invocation) + } + + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + rewrite_shell_function_updated_hook_input(invocation, updated_input, "container.exec") + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell/local_shell.rs b/codex-rs/core/src/tools/handlers/shell/local_shell.rs index 9470fbf0610f..edfde10eec7e 100644 --- a/codex-rs/core/src/tools/handlers/shell/local_shell.rs +++ b/codex-rs/core/src/tools/handlers/shell/local_shell.rs @@ -6,8 +6,10 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; @@ -61,6 +63,33 @@ impl ToolHandler for LocalShellHandler { !is_known_safe_command(¶ms.command) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + local_shell_payload_command(&invocation.payload).map(|command| PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": command }), + }) + } + + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + let command = updated_hook_command(&updated_input)?; + invocation.payload = match invocation.payload { + ToolPayload::LocalShell { mut params } => { + params.command = shlex::split(command).ok_or_else(|| { + FunctionCallError::RespondToModel( + "hook returned shell input with an invalid command string".to_string(), + ) + })?; + ToolPayload::LocalShell { params } + } + payload => payload, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index 1fb36aa3556c..305699814d58 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -17,8 +17,11 @@ use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; +use crate::tools::handlers::rewrite_function_string_argument; +use crate::tools::handlers::updated_hook_command; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; @@ -172,6 +175,34 @@ impl ToolHandler for ShellCommandHandler { .unwrap_or(true) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + shell_command_payload_command(&invocation.payload).map(|command| PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": command }), + }) + } + + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported shell_command payload".to_string(), + )); + }; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_string_argument( + &arguments, + "shell_command", + "command", + updated_hook_command(&updated_input)?, + )?, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell/shell_handler.rs b/codex-rs/core/src/tools/handlers/shell/shell_handler.rs index da884fed6977..347738529c0f 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_handler.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_handler.rs @@ -14,6 +14,7 @@ use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_workdir_base_path; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; @@ -22,8 +23,10 @@ use codex_tools::ToolSpec; use super::super::shell_spec::ShellToolOptions; use super::super::shell_spec::create_shell_tool; use super::RunExecLikeArgs; +use super::rewrite_shell_function_updated_hook_input; use super::run_exec_like; use super::shell_function_post_tool_use_payload; +use super::shell_function_pre_tool_use_payload; #[derive(Default)] pub struct ShellHandler { @@ -94,6 +97,18 @@ impl ToolHandler for ShellHandler { .unwrap_or(true) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + shell_function_pre_tool_use_payload(invocation) + } + + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + rewrite_shell_function_updated_hook_input(invocation, updated_input, "shell") + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index c6e4d70abd68..f18257d99646 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -7,7 +7,6 @@ use core_test_support::test_path_buf; use pretty_assertions::assert_eq; use crate::exec_env::create_env; -use crate::hook_runtime::tool_compat; use crate::sandboxing::SandboxPermissions; use crate::session::tests::make_session_and_context; use crate::shell::Shell; @@ -19,6 +18,7 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::ShellCommandHandler; use crate::tools::hook_names::HookToolName; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; use codex_shell_command::is_safe_command::is_known_safe_command; @@ -221,9 +221,10 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { }, }; let (session, turn) = make_session_and_context().await; + let handler = super::LocalShellHandler::default(); assert_eq!( - tool_compat::pre_tool_use_payload(&ToolInvocation { + handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -233,7 +234,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(tool_compat::PreToolUsePayload { + Some(PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "bash -lc 'printf hi'" }), }) @@ -246,9 +247,10 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { arguments: json!({ "command": "printf shell command" }).to_string(), }; let (session, turn) = make_session_and_context().await; + let handler = ShellCommandHandler::from(codex_tools::ShellCommandBackendConfig::Classic); assert_eq!( - tool_compat::pre_tool_use_payload(&ToolInvocation { + handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -258,7 +260,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(tool_compat::PreToolUsePayload { + Some(PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "printf shell command" }), }) diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index b26b505890d0..2b7470a1a2a2 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -12,7 +12,11 @@ use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_tool_environment; +use crate::tools::handlers::rewrite_function_string_argument; +use crate::tools::handlers::updated_hook_command; +use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::unified_exec::ExecCommandRequest; @@ -118,6 +122,40 @@ impl ToolHandler for ExecCommandHandler { !is_known_safe_command(&command) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + + parse_arguments::(arguments) + .ok() + .map(|args| PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": args.cmd }), + }) + } + + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: serde_json::Value, + ) -> Result { + let ToolPayload::Function { arguments } = invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported exec_command payload".to_string(), + )); + }; + invocation.payload = ToolPayload::Function { + arguments: rewrite_function_string_argument( + &arguments, + "exec_command", + "cmd", + updated_hook_command(&updated_input)?, + )?, + }; + Ok(invocation) + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, 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 983ae164a5cc..12fa7ef81384 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -6,13 +6,13 @@ use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::sync::Arc; -use crate::hook_runtime::tool_compat; use crate::session::tests::make_session_and_context; use crate::tools::context::ExecCommandToolOutput; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::hook_names::HookToolName; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; use tokio::sync::Mutex; @@ -185,8 +185,9 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { arguments: serde_json::json!({ "cmd": "printf exec command" }).to_string(), }; let (session, turn) = make_session_and_context().await; + let handler = ExecCommandHandler::default(); assert_eq!( - tool_compat::pre_tool_use_payload(&ToolInvocation { + handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), @@ -196,7 +197,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(tool_compat::PreToolUsePayload { + Some(PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": "printf exec command" }), }) @@ -209,8 +210,9 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { arguments: serde_json::json!({ "chars": "echo hi" }).to_string(), }; let (session, turn) = make_session_and_context().await; + let handler = WriteStdinHandler; assert_eq!( - tool_compat::pre_tool_use_payload(&ToolInvocation { + handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), turn: turn.into(), cancellation_token: tokio_util::sync::CancellationToken::new(), diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 832e86a8c45e..3b15c281ed54 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -8,7 +8,6 @@ use crate::hook_runtime::PreToolUseHookResult; 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; -use crate::hook_runtime::tool_compat; use crate::memory_usage::emit_metric_for_tool_read; use crate::sandbox_tags::permission_profile_policy_tag; use crate::sandbox_tags::permission_profile_sandbox_tag; @@ -84,6 +83,24 @@ pub trait ToolHandler: Send + Sync { None } + fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { + None + } + + /// Rebuilds a tool invocation from hook-facing `tool_input`. + /// + /// Tools that opt into input-rewriting hooks should invert the same stable + /// hook contract they expose from `pre_tool_use_payload`. + fn with_updated_hook_input( + &self, + _invocation: ToolInvocation, + _updated_input: Value, + ) -> Result { + Err(FunctionCallError::RespondToModel( + "tool does not support hook input rewriting".to_string(), + )) + } + /// Creates an optional consumer for streamed tool argument diffs. fn create_diff_consumer(&self) -> Option> { None @@ -136,6 +153,20 @@ impl AnyToolResult { } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct PreToolUsePayload { + /// Hook-facing tool name model. + /// + /// The canonical name is serialized to hook stdin, while aliases are used + /// only for matcher compatibility. + pub(crate) tool_name: HookToolName, + /// Tool-specific input exposed at `tool_input`. + /// + /// Shell-like tools use `{ "command": ... }`; MCP tools use their resolved + /// JSON arguments. + pub(crate) tool_input: Value, +} + #[derive(Debug, Clone, PartialEq)] pub(crate) struct PostToolUsePayload { /// Hook-facing tool name model. @@ -156,6 +187,14 @@ trait AnyToolHandler: Send + Sync { fn is_mutating<'a>(&'a self, invocation: &'a ToolInvocation) -> BoxFuture<'a, bool>; + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; + + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: Value, + ) -> Result; + fn create_diff_consumer(&self) -> Option>; fn handle_any<'a>( &'a self, @@ -175,6 +214,18 @@ where Box::pin(ToolHandler::is_mutating(self, invocation)) } + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + ToolHandler::pre_tool_use_payload(self, invocation) + } + + fn with_updated_hook_input( + &self, + invocation: ToolInvocation, + updated_input: Value, + ) -> Result { + ToolHandler::with_updated_hook_input(self, invocation, updated_input) + } + fn create_diff_consumer(&self) -> Option> { ToolHandler::create_diff_consumer(self) } @@ -313,9 +364,9 @@ impl ToolRegistry { return Err(err); } }; - if !handler.matches_kind(&invocation.payload) { let message = format!("tool {tool_name} invoked with incompatible payload"); + let log_payload = invocation.payload.log_payload(); otel.tool_result_with_tags( tool_name_flat.as_ref(), &call_id_owned, @@ -332,7 +383,7 @@ impl ToolRegistry { return Err(err); } - if let Some(pre_tool_use_payload) = tool_compat::pre_tool_use_payload(&invocation) { + if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) { match run_pre_tool_use_hooks( &invocation.session, &invocation.turn, @@ -350,7 +401,7 @@ impl ToolRegistry { PreToolUseHookResult::Continue { updated_input: Some(updated_input), } => { - invocation = tool_compat::apply_updated_input(invocation, updated_input)?; + invocation = handler.with_updated_hook_input(invocation, updated_input)?; } PreToolUseHookResult::Continue { updated_input: None, @@ -361,6 +412,7 @@ impl ToolRegistry { let is_mutating = handler.is_mutating(&invocation).await; let response_cell = tokio::sync::Mutex::new(None); let invocation_for_tool = invocation.clone(); + let log_payload = invocation.payload.log_payload(); let result = otel .log_tool_result_with_tags( diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 016f6e3f51d5..b98bcbe077b5 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -2906,7 +2906,7 @@ async fn pre_tool_use_rewrites_apply_patch_before_execution() -> Result<()> { vec![ sse(vec![ ev_response_created("resp-1"), - ev_apply_patch_function_call(call_id, &original_patch), + ev_apply_patch_custom_tool_call(call_id, &original_patch), ev_completed("resp-1"), ]), sse(vec![ @@ -2937,7 +2937,7 @@ async fn pre_tool_use_rewrites_apply_patch_before_execution() -> Result<()> { let requests = responses.requests(); assert_eq!(requests.len(), 2); - requests[1].function_call_output(call_id); + requests[1].custom_tool_call_output(call_id); assert!( !test.workspace_path(original_file).exists(), "original patch should not create its target file" From ebbcdbd957d22241cdd77ccd1c17657e3ad963ef Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 11 May 2026 20:48:14 -0400 Subject: [PATCH 19/20] Trim updatedInput merge churn --- codex-rs/core/src/tools/handlers/apply_patch_tests.rs | 1 + codex-rs/core/src/tools/handlers/mod.rs | 6 +++--- codex-rs/core/src/tools/handlers/shell_tests.rs | 8 ++++---- codex-rs/core/src/tools/handlers/unified_exec.rs | 2 +- codex-rs/core/src/tools/handlers/unified_exec_tests.rs | 5 +++-- 5 files changed, 12 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 0af446bc43fe..99854d23eaa6 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -50,6 +50,7 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { }; let invocation = invocation_for_payload(payload).await; let handler = ApplyPatchHandler::default(); + assert_eq!( handler.pre_tool_use_payload(&invocation), Some(PreToolUsePayload { diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 4438794f1e39..d689b0cf356c 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -86,7 +86,7 @@ where }) } -pub(crate) fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { +fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { updated_input .get("command") .and_then(Value::as_str) @@ -97,7 +97,7 @@ pub(crate) fn updated_hook_command(updated_input: &Value) -> Result<&str, Functi }) } -pub(crate) fn rewrite_function_arguments( +fn rewrite_function_arguments( arguments: &str, tool_name: &str, rewrite: impl FnOnce(&mut Map), @@ -116,7 +116,7 @@ pub(crate) fn rewrite_function_arguments( }) } -pub(crate) fn rewrite_function_string_argument( +fn rewrite_function_string_argument( arguments: &str, tool_name: &str, field_name: &str, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index f18257d99646..ce97b8317e71 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -16,9 +16,9 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::LocalShellHandler; use crate::tools::handlers::ShellCommandHandler; use crate::tools::hook_names::HookToolName; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; use codex_shell_command::is_safe_command::is_known_safe_command; @@ -221,7 +221,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { }, }; let (session, turn) = make_session_and_context().await; - let handler = super::LocalShellHandler::default(); + let handler = LocalShellHandler::default(); assert_eq!( handler.pre_tool_use_payload(&ToolInvocation { @@ -234,7 +234,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(PreToolUsePayload { + Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "bash -lc 'printf hi'" }), }) @@ -260,7 +260,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(PreToolUsePayload { + Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "printf shell command" }), }) diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 485810493422..c97f5bb6f2d1 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -27,7 +27,7 @@ pub use write_stdin::WriteStdinHandler; #[derive(Debug, Deserialize)] pub(crate) struct ExecCommandArgs { - pub(crate) cmd: String, + cmd: String, #[serde(default)] pub(crate) workdir: Option, #[serde(default)] 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 12fa7ef81384..02123c4b6460 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -12,7 +12,6 @@ use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::hook_names::HookToolName; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; use tokio::sync::Mutex; @@ -186,6 +185,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { }; let (session, turn) = make_session_and_context().await; let handler = ExecCommandHandler::default(); + assert_eq!( handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), @@ -197,7 +197,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { source: crate::tools::context::ToolCallSource::Direct, payload, }), - Some(PreToolUsePayload { + Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": "printf exec command" }), }) @@ -211,6 +211,7 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { }; let (session, turn) = make_session_and_context().await; let handler = WriteStdinHandler; + assert_eq!( handler.pre_tool_use_payload(&ToolInvocation { session: session.into(), From 7dcba7dd251972acfc9f234ee8fe6831ac182101 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 11 May 2026 22:06:27 -0400 Subject: [PATCH 20/20] Cover Bash-like updatedInput rewrites --- codex-rs/core/tests/suite/hooks.rs | 149 +++++++++++++++++++++++++---- 1 file changed, 130 insertions(+), 19 deletions(-) diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index b98bcbe077b5..563719cec220 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -2133,27 +2133,115 @@ async fn blocked_pre_tool_use_records_additional_context_for_shell_command() -> Ok(()) } -#[tokio::test] -async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { +#[derive(Clone, Copy)] +enum BashRewriteSurface { + ContainerExec, + ExecCommand, + LocalShell, + Shell, + ShellCommand, +} + +impl BashRewriteSurface { + fn slug(self) -> &'static str { + match self { + BashRewriteSurface::ContainerExec => "container-exec", + BashRewriteSurface::ExecCommand => "exec-command", + BashRewriteSurface::LocalShell => "local-shell", + BashRewriteSurface::Shell => "shell", + BashRewriteSurface::ShellCommand => "shell-command", + } + } + + fn tool_call(self, call_id: &str, command: &[String], command_text: &str) -> Result { + match self { + BashRewriteSurface::ContainerExec => Ok(ev_function_call( + call_id, + "container.exec", + &serde_json::to_string(&serde_json::json!({ "command": command }))?, + )), + BashRewriteSurface::ExecCommand => Ok(ev_function_call( + call_id, + "exec_command", + &serde_json::to_string(&serde_json::json!({ "cmd": command_text }))?, + )), + BashRewriteSurface::LocalShell => { + Ok(core_test_support::responses::ev_local_shell_call( + call_id, + "completed", + command.iter().map(String::as_str).collect(), + )) + } + BashRewriteSurface::Shell => Ok(ev_function_call( + call_id, + "shell", + &serde_json::to_string(&serde_json::json!({ "command": command }))?, + )), + BashRewriteSurface::ShellCommand => Ok(ev_function_call( + call_id, + "shell_command", + &serde_json::to_string(&serde_json::json!({ "command": command_text }))?, + )), + } + } + + fn original_command(self, marker: &Path) -> (Vec, String) { + let command_text = format!("printf original > {}", marker.display()); + match self { + BashRewriteSurface::ContainerExec + | BashRewriteSurface::LocalShell + | BashRewriteSurface::Shell => { + let command = vec!["/bin/sh".to_string(), "-c".to_string(), command_text]; + let command_text = codex_shell_command::parse_command::shlex_join(&command); + (command, command_text) + } + BashRewriteSurface::ExecCommand | BashRewriteSurface::ShellCommand => { + (Vec::new(), command_text) + } + } + } + + fn rewritten_command(self, marker: &Path) -> String { + let command_text = format!("printf rewritten > {}", marker.display()); + match self { + BashRewriteSurface::ContainerExec + | BashRewriteSurface::LocalShell + | BashRewriteSurface::Shell => codex_shell_command::parse_command::shlex_join(&[ + "/bin/sh".to_string(), + "-c".to_string(), + command_text, + ]), + BashRewriteSurface::ExecCommand | BashRewriteSurface::ShellCommand => command_text, + } + } + + fn configure(self, config: &mut Config) { + trust_discovered_hooks(config); + if matches!(self, BashRewriteSurface::ExecCommand) { + config.use_experimental_unified_exec_tool = true; + if let Err(error) = config.features.enable(Feature::UnifiedExec) { + panic!("test config should allow feature update: {error}"); + } + } + } +} + +async fn assert_pre_tool_use_rewrites_bash_surface(surface: BashRewriteSurface) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let call_id = "pretooluse-shell-command-rewrite"; - let original_marker = std::env::temp_dir().join("pretooluse-shell-command-original-marker"); - let rewritten_marker = std::env::temp_dir().join("pretooluse-shell-command-rewritten-marker"); - let original_command = format!("printf original > {}", original_marker.display()); - let rewritten_command = format!("printf rewritten > {}", rewritten_marker.display()); - let args = serde_json::json!({ "command": original_command }); + let slug = surface.slug(); + let call_id = format!("pretooluse-{slug}-rewrite"); + let original_marker = std::env::temp_dir().join(format!("pretooluse-{slug}-original-marker")); + let rewritten_marker = std::env::temp_dir().join(format!("pretooluse-{slug}-rewritten-marker")); + let (tool_command, original_command) = surface.original_command(&original_marker); + let rewritten_command = surface.rewritten_command(&rewritten_marker); 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)?, - ), + surface.tool_call(&call_id, &tool_command, &original_command)?, ev_completed("resp-1"), ]), sse(vec![ @@ -2172,9 +2260,7 @@ async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { panic!("failed to write updating pre tool use hook fixture: {error}"); } }) - .with_config(|config| { - trust_discovered_hooks(config); - }); + .with_config(move |config| surface.configure(config)); let test = builder.build(&server).await?; if original_marker.exists() { @@ -2185,17 +2271,17 @@ async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { } test.submit_turn_with_permission_profile( - "run the rewritten shell command", + &format!("run the rewritten {slug} command"), PermissionProfile::Disabled, ) .await?; let requests = responses.requests(); assert_eq!(requests.len(), 2); - requests[1].function_call_output(call_id); + requests[1].function_call_output(&call_id); assert!( !original_marker.exists(), - "original shell command should not execute after rewrite" + "original {slug} command should not execute after rewrite" ); assert_eq!( fs::read_to_string(&rewritten_marker).context("read rewritten pre tool marker")?, @@ -2209,6 +2295,31 @@ async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { Ok(()) } +#[tokio::test] +async fn pre_tool_use_rewrites_shell_before_execution() -> Result<()> { + assert_pre_tool_use_rewrites_bash_surface(BashRewriteSurface::Shell).await +} + +#[tokio::test] +async fn pre_tool_use_rewrites_container_exec_before_execution() -> Result<()> { + assert_pre_tool_use_rewrites_bash_surface(BashRewriteSurface::ContainerExec).await +} + +#[tokio::test] +async fn pre_tool_use_rewrites_local_shell_before_execution() -> Result<()> { + assert_pre_tool_use_rewrites_bash_surface(BashRewriteSurface::LocalShell).await +} + +#[tokio::test] +async fn pre_tool_use_rewrites_shell_command_before_execution() -> Result<()> { + assert_pre_tool_use_rewrites_bash_surface(BashRewriteSurface::ShellCommand).await +} + +#[tokio::test] +async fn pre_tool_use_rewrites_exec_command_before_execution() -> Result<()> { + assert_pre_tool_use_rewrites_bash_surface(BashRewriteSurface::ExecCommand).await +} + #[tokio::test] async fn pre_tool_use_rewrites_code_mode_nested_exec_command_before_execution() -> Result<()> { skip_if_no_network!(Ok(()));