From 67a80378872828ea45321374ab907385b24c7ba1 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Mon, 13 Apr 2026 20:58:02 -0700 Subject: [PATCH 01/22] initial --- codex-rs/core/src/tools/context.rs | 3 +- codex-rs/core/src/tools/context_tests.rs | 1 + codex-rs/core/src/tools/handlers/shell.rs | 6 +- .../core/src/tools/handlers/shell_tests.rs | 1 + .../core/src/tools/handlers/unified_exec.rs | 25 +-- .../src/tools/handlers/unified_exec_tests.rs | 112 +++++++++++- codex-rs/core/src/tools/parallel.rs | 1 + codex-rs/core/src/tools/registry.rs | 35 ++-- codex-rs/core/src/unified_exec/mod.rs | 2 + codex-rs/core/src/unified_exec/mod_tests.rs | 2 + .../core/src/unified_exec/process_manager.rs | 8 + codex-rs/core/tests/suite/hooks.rs | 165 ++++++++++++++++++ codex-rs/hooks/src/events/common.rs | 32 +++- 13 files changed, 350 insertions(+), 43 deletions(-) diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index dff1e444da2d..547ac186b1ac 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -351,6 +351,7 @@ pub struct ExecCommandToolOutput { pub exit_code: Option, pub original_token_count: Option, pub session_command: Option>, + pub raw_command: Option, } impl ToolOutput for ExecCommandToolOutput { @@ -374,7 +375,7 @@ impl ToolOutput for ExecCommandToolOutput { } fn post_tool_use_response(&self, _call_id: &str, _payload: &ToolPayload) -> Option { - if self.process_id.is_some() || self.session_command.is_none() { + if self.process_id.is_some() || self.raw_command.is_none() { return None; } diff --git a/codex-rs/core/src/tools/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index d45f351833e8..827699783576 100644 --- a/codex-rs/core/src/tools/context_tests.rs +++ b/codex-rs/core/src/tools/context_tests.rs @@ -389,6 +389,7 @@ fn exec_command_tool_output_formats_truncated_response() { exit_code: Some(0), original_token_count: Some(10), session_command: None, + raw_command: None, } .to_response_item("call-42", &payload); diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 3ed21bd2ed2b..197ec5be5224 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -211,10 +211,11 @@ impl ToolHandler for ShellHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; Some(PostToolUsePayload { + tool_use_id: call_id.to_string(), command: shell_payload_command(payload)?, tool_response, }) @@ -318,10 +319,11 @@ impl ToolHandler for ShellCommandHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; Some(PostToolUsePayload { + tool_use_id: call_id.to_string(), command: shell_command_payload_command(payload)?, tool_response, }) diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 5c92a988ced4..e29a92d8108d 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -276,6 +276,7 @@ fn build_post_tool_use_payload_uses_tool_output_wire_value() { assert_eq!( handler.post_tool_use_payload("call-42", &payload, &output), Some(crate::tools::registry::PostToolUsePayload { + tool_use_id: "call-42".to_string(), command: "printf shell command".to_string(), tool_response: json!("shell output"), }) diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index a604e9762cf7..9283945a41cd 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -140,20 +140,22 @@ impl ToolHandler for UnifiedExecHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { - let ToolPayload::Function { arguments } = payload else { + let ToolPayload::Function { .. } = payload else { return None; }; - let args = parse_arguments::(arguments).ok()?; - if args.tty { - return None; - } - - let tool_response = result.post_tool_use_response(call_id, payload)?; + let command = result.raw_command.clone()?; + let tool_use_id = if result.event_call_id.is_empty() { + call_id.to_string() + } else { + result.event_call_id.clone() + }; + let tool_response = result.post_tool_use_response(&tool_use_id, payload)?; Some(PostToolUsePayload { - command: args.cmd, + tool_use_id, + command, tool_response, }) } @@ -192,11 +194,12 @@ impl ToolHandler for UnifiedExecHandler { "exec_command" => { let cwd = resolve_workdir_base_path(&arguments, &context.turn.cwd)?; let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; + let raw_command = args.cmd.clone(); let workdir = context.turn.resolve_path(args.workdir.clone()); maybe_emit_implicit_skill_invocation( session.as_ref(), context.turn.as_ref(), - &args.cmd, + &raw_command, &workdir, ) .await; @@ -306,6 +309,7 @@ impl ToolHandler for UnifiedExecHandler { exit_code: None, original_token_count: None, session_command: None, + raw_command: None, }); } @@ -314,6 +318,7 @@ impl ToolHandler for UnifiedExecHandler { .exec_command( ExecCommandRequest { command, + raw_command, process_id, yield_time_ms, max_output_tokens, 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 b64181451665..ae3b73895d58 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -248,7 +248,7 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co arguments: serde_json::json!({ "cmd": "echo three", "tty": false }).to_string(), }; let output = ExecCommandToolOutput { - event_call_id: "event-43".to_string(), + event_call_id: "call-43".to_string(), chunk_id: "chunk-1".to_string(), wall_time: std::time::Duration::from_millis(498), raw_output: b"three".to_vec(), @@ -261,11 +261,13 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co "-lc".to_string(), "echo three".to_string(), ]), + raw_command: Some("echo three".to_string()), }; assert_eq!( UnifiedExecHandler.post_tool_use_payload("call-43", &payload, &output), Some(crate::tools::registry::PostToolUsePayload { + tool_use_id: "call-43".to_string(), command: "echo three".to_string(), tool_response: serde_json::json!("three"), }) @@ -273,12 +275,12 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co } #[test] -fn exec_command_post_tool_use_payload_skips_interactive_exec() { +fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "cmd": "echo three", "tty": true }).to_string(), }; let output = ExecCommandToolOutput { - event_call_id: "event-44".to_string(), + event_call_id: "call-44".to_string(), chunk_id: "chunk-1".to_string(), wall_time: std::time::Duration::from_millis(498), raw_output: b"three".to_vec(), @@ -291,11 +293,16 @@ fn exec_command_post_tool_use_payload_skips_interactive_exec() { "-lc".to_string(), "echo three".to_string(), ]), + raw_command: Some("echo three".to_string()), }; assert_eq!( UnifiedExecHandler.post_tool_use_payload("call-44", &payload, &output), - None + Some(crate::tools::registry::PostToolUsePayload { + tool_use_id: "call-44".to_string(), + command: "echo three".to_string(), + tool_response: serde_json::json!("three"), + }) ); } @@ -318,6 +325,7 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { "-lc".to_string(), "echo three".to_string(), ]), + raw_command: Some("echo three".to_string()), }; assert_eq!( @@ -325,3 +333,99 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { None ); } + +#[test] +fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_completion() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ + "session_id": 45, + "chars": "", + }) + .to_string(), + }; + let output = ExecCommandToolOutput { + event_call_id: "exec-call-45".to_string(), + chunk_id: "chunk-2".to_string(), + wall_time: std::time::Duration::from_millis(498), + raw_output: b"finished\n".to_vec(), + max_output_tokens: None, + process_id: None, + exit_code: Some(0), + original_token_count: None, + session_command: Some(vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "sleep 1; echo finished".to_string(), + ]), + raw_command: Some("sleep 1; echo finished".to_string()), + }; + + assert_eq!( + UnifiedExecHandler.post_tool_use_payload("write-stdin-call", &payload, &output), + Some(crate::tools::registry::PostToolUsePayload { + tool_use_id: "exec-call-45".to_string(), + command: "sleep 1; echo finished".to_string(), + tool_response: serde_json::json!("finished\n"), + }) + ); +} + +#[test] +fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ "session_id": 45, "chars": "" }).to_string(), + }; + let output_a = ExecCommandToolOutput { + event_call_id: "exec-call-a".to_string(), + chunk_id: "chunk-a".to_string(), + wall_time: std::time::Duration::from_millis(498), + raw_output: b"alpha\n".to_vec(), + max_output_tokens: None, + process_id: None, + exit_code: Some(0), + original_token_count: None, + session_command: Some(vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "sleep 2; echo alpha".to_string(), + ]), + raw_command: Some("sleep 2; echo alpha".to_string()), + }; + let output_b = ExecCommandToolOutput { + event_call_id: "exec-call-b".to_string(), + chunk_id: "chunk-b".to_string(), + wall_time: std::time::Duration::from_millis(498), + raw_output: b"beta\n".to_vec(), + max_output_tokens: None, + process_id: None, + exit_code: Some(0), + original_token_count: None, + session_command: Some(vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "sleep 1; echo beta".to_string(), + ]), + raw_command: Some("sleep 1; echo beta".to_string()), + }; + + let payloads = [ + UnifiedExecHandler.post_tool_use_payload("write-call-b", &payload, &output_b), + UnifiedExecHandler.post_tool_use_payload("write-call-a", &payload, &output_a), + ]; + + assert_eq!( + payloads, + [ + Some(crate::tools::registry::PostToolUsePayload { + tool_use_id: "exec-call-b".to_string(), + command: "sleep 1; echo beta".to_string(), + tool_response: serde_json::json!("beta\n"), + }), + Some(crate::tools::registry::PostToolUsePayload { + tool_use_id: "exec-call-a".to_string(), + command: "sleep 2; echo alpha".to_string(), + tool_response: serde_json::json!("alpha\n"), + }), + ] + ); +} diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index a0818dfc8307..347314a3573c 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -168,6 +168,7 @@ impl ToolCallRuntime { result: Box::new(AbortedToolOutput { message: Self::abort_message(call, secs), }), + post_tool_use_payload: None, } } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 6231c7ea1da1..785f71c87abc 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -69,7 +69,7 @@ pub trait ToolHandler: Send + Sync { &self, _call_id: &str, _payload: &ToolPayload, - _result: &dyn ToolOutput, + _result: &Self::Output, ) -> Option { None } @@ -86,6 +86,7 @@ pub(crate) struct AnyToolResult { pub(crate) call_id: String, pub(crate) payload: ToolPayload, pub(crate) result: Box, + pub(crate) post_tool_use_payload: Option, } impl AnyToolResult { @@ -114,6 +115,7 @@ pub(crate) struct PreToolUsePayload { #[derive(Debug, Clone, PartialEq)] pub(crate) struct PostToolUsePayload { + pub(crate) tool_use_id: String, pub(crate) command: String, pub(crate) tool_response: Value, } @@ -125,13 +127,6 @@ trait AnyToolHandler: Send + Sync { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; - fn post_tool_use_payload( - &self, - call_id: &str, - payload: &ToolPayload, - result: &dyn ToolOutput, - ) -> Option; - fn handle_any<'a>( &'a self, invocation: ToolInvocation, @@ -154,15 +149,6 @@ where ToolHandler::pre_tool_use_payload(self, invocation) } - fn post_tool_use_payload( - &self, - call_id: &str, - payload: &ToolPayload, - result: &dyn ToolOutput, - ) -> Option { - ToolHandler::post_tool_use_payload(self, call_id, payload, result) - } - fn handle_any<'a>( &'a self, invocation: ToolInvocation, @@ -171,10 +157,13 @@ where let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); let output = self.handle(invocation).await?; + let post_tool_use_payload = + ToolHandler::post_tool_use_payload(self, &call_id, &payload, &output); Ok(AnyToolResult { call_id, payload, result: Box::new(output), + post_tool_use_payload, }) }) } @@ -347,13 +336,9 @@ impl ToolRegistry { emit_metric_for_tool_read(&invocation, success).await; let post_tool_use_payload = if success { let guard = response_cell.lock().await; - guard.as_ref().and_then(|result| { - handler.post_tool_use_payload( - &result.call_id, - &result.payload, - result.result.as_ref(), - ) - }) + guard + .as_ref() + .and_then(|result| result.post_tool_use_payload.clone()) } else { None }; @@ -362,7 +347,7 @@ impl ToolRegistry { run_post_tool_use_hooks( &invocation.session, &invocation.turn, - invocation.call_id.clone(), + post_tool_use_payload.tool_use_id, post_tool_use_payload.command, post_tool_use_payload.tool_response, ) diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 3f3b018df4e5..e5e9fb9dd17d 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -88,6 +88,7 @@ impl UnifiedExecContext { #[derive(Debug)] pub(crate) struct ExecCommandRequest { pub command: Vec, + pub raw_command: String, pub process_id: i32, pub yield_time_ms: u64, pub max_output_tokens: Option, @@ -148,6 +149,7 @@ struct ProcessEntry { call_id: String, process_id: i32, command: Vec, + raw_command: String, tty: bool, network_approval_id: Option, session: Weak, diff --git a/codex-rs/core/src/unified_exec/mod_tests.rs b/codex-rs/core/src/unified_exec/mod_tests.rs index 2cc280e1e47e..46163c4c3140 100644 --- a/codex-rs/core/src/unified_exec/mod_tests.rs +++ b/codex-rs/core/src/unified_exec/mod_tests.rs @@ -114,6 +114,7 @@ async fn exec_command_with_tty( call_id: context.call_id.clone(), process_id, command: command.clone(), + raw_command: cmd.to_string(), tty, network_approval_id: None, session: Arc::downgrade(session), @@ -166,6 +167,7 @@ async fn exec_command_with_tty( exit_code, original_token_count: Some(approx_token_count(&text)), session_command: Some(command), + raw_command: Some(cmd.to_string()), }) } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 20cc7c5f7f82..6d8aa2208c42 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -166,6 +166,7 @@ struct PreparedProcessHandles { cancellation_token: CancellationToken, pause_state: Option>, command: Vec, + raw_command: String, process_id: i32, tty: bool, } @@ -275,6 +276,7 @@ impl UnifiedExecProcessManager { Arc::clone(&process), context, &request.command, + request.raw_command.clone(), cwd.clone(), start, request.process_id, @@ -395,6 +397,7 @@ impl UnifiedExecProcessManager { exit_code, original_token_count: Some(original_token_count), session_command: Some(request.command.clone()), + raw_command: Some(request.raw_command.clone()), }; Ok(response) @@ -415,6 +418,7 @@ impl UnifiedExecProcessManager { cancellation_token, pause_state, command: session_command, + raw_command, process_id, tty, .. @@ -514,6 +518,7 @@ impl UnifiedExecProcessManager { exit_code, original_token_count: Some(original_token_count), session_command: Some(session_command.clone()), + raw_command: Some(raw_command), }; Ok(response) @@ -582,6 +587,7 @@ impl UnifiedExecProcessManager { cancellation_token, pause_state, command: entry.command.clone(), + raw_command: entry.raw_command.clone(), process_id: entry.process_id, tty: entry.tty, }) @@ -593,6 +599,7 @@ impl UnifiedExecProcessManager { process: Arc, context: &UnifiedExecContext, command: &[String], + raw_command: String, cwd: AbsolutePathBuf, started_at: Instant, process_id: i32, @@ -605,6 +612,7 @@ impl UnifiedExecProcessManager { call_id: context.call_id.clone(), process_id, command: command.to_vec(), + raw_command, tty, network_approval_id, session: Arc::downgrade(&context.session), diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 47cd251bbfde..0a4429ed74e9 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -22,6 +22,7 @@ use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_windows; use core_test_support::streaming_sse::StreamingSseChunk; use core_test_support::streaming_sse::start_streaming_sse_server; use core_test_support::test_codex::test_codex; @@ -309,6 +310,64 @@ elif mode == "exit_2": Ok(()) } +fn write_logging_pre_and_blocking_post_tool_use_hooks(home: &Path, feedback: &str) -> Result<()> { + let pre_script_path = home.join("pre_tool_use_hook.py"); + let pre_log_path = home.join("pre_tool_use_hook_log.jsonl"); + let post_script_path = home.join("post_tool_use_hook.py"); + let post_log_path = home.join("post_tool_use_hook_log.jsonl"); + let feedback_json = + serde_json::to_string(feedback).context("serialize post tool use feedback")?; + let pre_script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{pre_log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +"#, + pre_log_path = pre_log_path.display(), + ); + let post_script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{post_log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +sys.stderr.write({feedback_json} + "\n") +raise SystemExit(2) +"#, + post_log_path = post_log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "matcher": "Bash", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", pre_script_path.display()), + "statusMessage": "running pre tool use hook", + }] + }], + "PostToolUse": [{ + "matcher": "Bash", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", post_script_path.display()), + "statusMessage": "running post tool use hook", + }] + }] + } + }); + + fs::write(&pre_script_path, pre_script).context("write pre tool use hook script")?; + fs::write(&post_script_path, post_script).context("write post tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn write_session_start_hook_recording_transcript(home: &Path) -> Result<()> { let script_path = home.join("session_start_hook.py"); let log_path = home.join("session_start_hook_log.jsonl"); @@ -1743,6 +1802,112 @@ async fn post_tool_use_exit_two_replaces_one_shot_exec_command_output_with_feedb Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn post_tool_use_blocks_when_exec_session_completes_via_write_stdin() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_windows!(Ok(())); + + let server = start_mock_server().await; + let start_call_id = "posttooluse-exec-session-start"; + let poll_call_id = "posttooluse-exec-session-poll"; + let command = "sleep 1; printf session-post-hook-output".to_string(); + let start_args = serde_json::json!({ + "cmd": command, + "shell": "/bin/sh", + "login": false, + "tty": false, + "yield_time_ms": 250, + }); + let poll_args = serde_json::json!({ + "session_id": 1000, + "chars": "", + "yield_time_ms": 5_000, + }); + let feedback = "blocked by session post hook"; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + core_test_support::responses::ev_function_call( + poll_call_id, + "write_stdin", + &serde_json::to_string(&poll_args)?, + ), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-1", "session post hook observed"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = write_logging_pre_and_blocking_post_tool_use_hooks(home, feedback) { + panic!("failed to write tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + test.submit_turn("run the exec command session with post hook") + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 3); + let output_item = requests[2].function_call_output(poll_call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("write_stdin output string"); + assert_eq!(output, feedback); + + let pre_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(pre_hook_inputs.len(), 1); + assert_eq!(pre_hook_inputs[0]["tool_name"], "Bash"); + assert_eq!(pre_hook_inputs[0]["tool_use_id"], start_call_id); + assert_eq!(pre_hook_inputs[0]["tool_input"]["command"], command); + + let post_hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(post_hook_inputs.len(), 1); + assert_eq!(post_hook_inputs[0]["hook_event_name"], "PostToolUse"); + assert_eq!(post_hook_inputs[0]["tool_name"], "Bash"); + assert_eq!(post_hook_inputs[0]["tool_use_id"], start_call_id); + assert_eq!(post_hook_inputs[0]["tool_input"]["command"], command); + assert!( + post_hook_inputs[0]["tool_response"] + .as_str() + .is_some_and(|tool_response| tool_response.contains("session-post-hook-output")), + "PostToolUse should see the final session output, got {:?}", + post_hook_inputs[0]["tool_response"] + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn post_tool_use_does_not_fire_for_non_shell_tools() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index 305a1214adb5..b9b27fef1f79 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -108,7 +108,7 @@ pub(crate) fn matcher_pattern_for_event( } pub(crate) fn validate_matcher_pattern(matcher: &str) -> Result<(), regex::Error> { - if is_match_all_matcher(matcher) { + if is_match_all_matcher(matcher) || is_exact_matcher(matcher) { return Ok(()); } regex::Regex::new(matcher).map(|_| ()) @@ -118,6 +118,9 @@ pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> boo match matcher { None => true, Some(matcher) if is_match_all_matcher(matcher) => true, + Some(matcher) if is_exact_matcher(matcher) => input + .map(|input| matcher.split('|').any(|candidate| candidate == input)) + .unwrap_or(false), Some(matcher) => input .and_then(|input| { regex::Regex::new(matcher) @@ -132,6 +135,12 @@ fn is_match_all_matcher(matcher: &str) -> bool { matcher.is_empty() || matcher == "*" } +fn is_exact_matcher(matcher: &str) -> bool { + matcher + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '|') +} + #[cfg(test)] mod tests { use codex_protocol::protocol::HookEventName; @@ -169,6 +178,27 @@ mod tests { assert_eq!(validate_matcher_pattern("Edit|Write"), Ok(())); } + #[test] + fn matcher_exact_string_does_not_substring_match() { + assert!(matches_matcher(Some("Bash"), Some("Bash"))); + assert!(!matches_matcher(Some("Bash"), Some("BashOutput"))); + assert_eq!(validate_matcher_pattern("Bash"), Ok(())); + } + + #[test] + fn matcher_uses_regex_when_it_contains_other_characters() { + assert!(matches_matcher(Some("^Bash"), Some("BashOutput"))); + assert!(matches_matcher( + Some("mcp__memory__.*"), + Some("mcp__memory__create_entities") + )); + assert!(!matches_matcher( + Some("mcp__memory"), + Some("mcp__memory__create_entities") + )); + assert_eq!(validate_matcher_pattern("mcp__memory__.*"), Ok(())); + } + #[test] fn matcher_supports_anchored_regexes() { assert!(matches_matcher(Some("^Bash$"), Some("Bash"))); From 34514df72bded2725519064d174a7177105208a7 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Tue, 21 Apr 2026 14:44:05 -0700 Subject: [PATCH 02/22] codex: fix CI failure on PR #18888 --- codex-rs/core/src/tools/handlers/unified_exec.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 5b021285d900..5a0e6e6b5360 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -322,7 +322,7 @@ impl ToolHandler for UnifiedExecHandler { .exec_command( ExecCommandRequest { command, - raw_command, + raw_command: raw_command.clone(), hook_command: raw_command.clone(), process_id, yield_time_ms, @@ -358,6 +358,7 @@ impl ToolHandler for UnifiedExecHandler { exit_code: Some(output.exit_code), original_token_count: Some(original_token_count), session_command: Some(session_command), + raw_command: Some(raw_command), } } Err(err) => { From 70a373a6defe1bb7e9fa5655396be267889139a6 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Tue, 21 Apr 2026 16:14:19 -0700 Subject: [PATCH 03/22] core: simplify unified exec hook command state --- codex-rs/core/src/tools/context.rs | 5 +-- codex-rs/core/src/tools/context_tests.rs | 3 +- .../core/src/tools/handlers/unified_exec.rs | 16 +++---- .../src/tools/handlers/unified_exec_tests.rs | 42 +++---------------- codex-rs/core/src/unified_exec/mod.rs | 4 +- codex-rs/core/src/unified_exec/mod_tests.rs | 6 +-- .../core/src/unified_exec/process_manager.rs | 22 ++++------ 7 files changed, 26 insertions(+), 72 deletions(-) diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index d986b1314129..0cd29dc230f0 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -359,8 +359,7 @@ pub struct ExecCommandToolOutput { pub process_id: Option, pub exit_code: Option, pub original_token_count: Option, - pub session_command: Option>, - pub raw_command: Option, + pub hook_command: Option, } impl ToolOutput for ExecCommandToolOutput { @@ -384,7 +383,7 @@ impl ToolOutput for ExecCommandToolOutput { } fn post_tool_use_response(&self, _call_id: &str, _payload: &ToolPayload) -> Option { - if self.process_id.is_some() || self.raw_command.is_none() { + if self.process_id.is_some() || self.hook_command.is_none() { return None; } diff --git a/codex-rs/core/src/tools/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index f6c4cbbcfd97..60ad4561519f 100644 --- a/codex-rs/core/src/tools/context_tests.rs +++ b/codex-rs/core/src/tools/context_tests.rs @@ -390,8 +390,7 @@ fn exec_command_tool_output_formats_truncated_response() { process_id: None, exit_code: Some(0), original_token_count: Some(10), - session_command: None, - raw_command: None, + hook_command: None, } .to_response_item("call-42", &payload); diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 5a0e6e6b5360..2c7e3d3f6494 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -149,7 +149,7 @@ impl ToolHandler for UnifiedExecHandler { return None; }; - let command = result.raw_command.clone()?; + let command = result.hook_command.clone()?; let tool_use_id = if result.event_call_id.is_empty() { call_id.to_string() } else { @@ -197,12 +197,12 @@ impl ToolHandler for UnifiedExecHandler { "exec_command" => { let cwd = resolve_workdir_base_path(&arguments, &context.turn.cwd)?; let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; - let raw_command = args.cmd.clone(); + let hook_command = args.cmd.clone(); let workdir = context.turn.resolve_path(args.workdir.clone()); maybe_emit_implicit_skill_invocation( session.as_ref(), context.turn.as_ref(), - &raw_command, + &hook_command, &workdir, ) .await; @@ -311,19 +311,16 @@ impl ToolHandler for UnifiedExecHandler { process_id: None, exit_code: None, original_token_count: None, - session_command: None, - raw_command: None, + hook_command: None, }); } emit_unified_exec_tty_metric(&turn.session_telemetry, tty); - let session_command = command.clone(); match manager .exec_command( ExecCommandRequest { command, - raw_command: raw_command.clone(), - hook_command: raw_command.clone(), + hook_command: hook_command.clone(), process_id, yield_time_ms, max_output_tokens, @@ -357,8 +354,7 @@ impl ToolHandler for UnifiedExecHandler { process_id: None, exit_code: Some(output.exit_code), original_token_count: Some(original_token_count), - session_command: Some(session_command), - raw_command: Some(raw_command), + hook_command: Some(hook_command), } } Err(err) => { 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 6f3a73369dca..9effefc1c115 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -258,12 +258,7 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "echo three".to_string(), - ]), - raw_command: Some("echo three".to_string()), + hook_command: Some("echo three".to_string()), }; assert_eq!( @@ -290,12 +285,7 @@ fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() { process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "echo three".to_string(), - ]), - raw_command: Some("echo three".to_string()), + hook_command: Some("echo three".to_string()), }; assert_eq!( @@ -322,12 +312,7 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { process_id: Some(45), exit_code: None, original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "echo three".to_string(), - ]), - raw_command: Some("echo three".to_string()), + hook_command: Some("echo three".to_string()), }; assert_eq!( @@ -354,12 +339,7 @@ fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_c process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "sleep 1; echo finished".to_string(), - ]), - raw_command: Some("sleep 1; echo finished".to_string()), + hook_command: Some("sleep 1; echo finished".to_string()), }; assert_eq!( @@ -386,12 +366,7 @@ fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "sleep 2; echo alpha".to_string(), - ]), - raw_command: Some("sleep 2; echo alpha".to_string()), + hook_command: Some("sleep 2; echo alpha".to_string()), }; let output_b = ExecCommandToolOutput { event_call_id: "exec-call-b".to_string(), @@ -402,12 +377,7 @@ fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "sleep 1; echo beta".to_string(), - ]), - raw_command: Some("sleep 1; echo beta".to_string()), + hook_command: Some("sleep 1; echo beta".to_string()), }; let payloads = [ diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index d63a4009b37c..a5a6e69f896d 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -88,7 +88,6 @@ impl UnifiedExecContext { #[derive(Debug)] pub(crate) struct ExecCommandRequest { pub command: Vec, - pub raw_command: String, pub hook_command: String, pub process_id: i32, pub yield_time_ms: u64, @@ -149,8 +148,7 @@ struct ProcessEntry { process: Arc, call_id: String, process_id: i32, - command: Vec, - raw_command: String, + hook_command: String, tty: bool, network_approval_id: Option, session: Weak, diff --git a/codex-rs/core/src/unified_exec/mod_tests.rs b/codex-rs/core/src/unified_exec/mod_tests.rs index a62099e41a6f..297c7057a336 100644 --- a/codex-rs/core/src/unified_exec/mod_tests.rs +++ b/codex-rs/core/src/unified_exec/mod_tests.rs @@ -113,8 +113,7 @@ async fn exec_command_with_tty( process: Arc::clone(&process), call_id: context.call_id.clone(), process_id, - command: command.clone(), - raw_command: cmd.to_string(), + hook_command: cmd.to_string(), tty, network_approval_id: None, session: Arc::downgrade(session), @@ -166,8 +165,7 @@ async fn exec_command_with_tty( process_id: response_process_id, exit_code, original_token_count: Some(approx_token_count(&text)), - session_command: Some(command), - raw_command: Some(cmd.to_string()), + hook_command: Some(cmd.to_string()), }) } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 14f4a1de9e7d..ec0b00cf590b 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -169,8 +169,7 @@ struct PreparedProcessHandles { output_closed_notify: Arc, cancellation_token: CancellationToken, pause_state: Option>, - command: Vec, - raw_command: String, + hook_command: String, process_id: i32, tty: bool, } @@ -280,7 +279,7 @@ impl UnifiedExecProcessManager { Arc::clone(&process), context, &request.command, - request.raw_command.clone(), + request.hook_command.clone(), cwd.clone(), start, request.process_id, @@ -400,8 +399,7 @@ impl UnifiedExecProcessManager { process_id: response_process_id, exit_code, original_token_count: Some(original_token_count), - session_command: Some(request.command.clone()), - raw_command: Some(request.raw_command.clone()), + hook_command: Some(request.hook_command.clone()), }; Ok(response) @@ -421,8 +419,7 @@ impl UnifiedExecProcessManager { output_closed_notify, cancellation_token, pause_state, - command: session_command, - raw_command, + hook_command, process_id, tty, .. @@ -521,8 +518,7 @@ impl UnifiedExecProcessManager { process_id, exit_code, original_token_count: Some(original_token_count), - session_command: Some(session_command.clone()), - raw_command: Some(raw_command), + hook_command: Some(hook_command), }; Ok(response) @@ -590,8 +586,7 @@ impl UnifiedExecProcessManager { output_closed_notify, cancellation_token, pause_state, - command: entry.command.clone(), - raw_command: entry.raw_command.clone(), + hook_command: entry.hook_command.clone(), process_id: entry.process_id, tty: entry.tty, }) @@ -603,7 +598,7 @@ impl UnifiedExecProcessManager { process: Arc, context: &UnifiedExecContext, command: &[String], - raw_command: String, + hook_command: String, cwd: AbsolutePathBuf, started_at: Instant, process_id: i32, @@ -615,8 +610,7 @@ impl UnifiedExecProcessManager { process: Arc::clone(&process), call_id: context.call_id.clone(), process_id, - command: command.to_vec(), - raw_command, + hook_command, tty, network_approval_id, session: Arc::downgrade(&context.session), From 21637b14a58b8237872d121594d0cb425d458268 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 12:12:34 -0700 Subject: [PATCH 04/22] initial impl --- codex-rs/core/src/hook_runtime.rs | 13 +- codex-rs/core/src/mcp_tool_call.rs | 37 +++ codex-rs/core/src/mcp_tool_call_tests.rs | 217 ++++++++++++++++++ codex-rs/core/src/tools/context.rs | 4 + codex-rs/core/src/tools/handlers/mcp.rs | 135 +++++++++++ codex-rs/core/src/tools/handlers/shell.rs | 14 +- .../core/src/tools/handlers/shell_tests.rs | 11 +- .../core/src/tools/handlers/unified_exec.rs | 6 +- .../src/tools/handlers/unified_exec_tests.rs | 14 +- codex-rs/core/src/tools/network_approval.rs | 6 +- codex-rs/core/src/tools/registry.rs | 35 ++- codex-rs/core/src/tools/runtimes/shell.rs | 6 +- .../tools/runtimes/shell/unix_escalation.rs | 6 +- .../core/src/tools/runtimes/unified_exec.rs | 6 +- codex-rs/core/src/tools/sandboxing.rs | 3 +- ...rmission-request.command.input.schema.json | 19 +- .../post-tool-use.command.input.schema.json | 16 +- .../pre-tool-use.command.input.schema.json | 16 +- codex-rs/hooks/src/events/common.rs | 43 +++- .../hooks/src/events/permission_request.rs | 10 +- codex-rs/hooks/src/events/post_tool_use.rs | 12 +- codex-rs/hooks/src/events/pre_tool_use.rs | 12 +- codex-rs/hooks/src/schema.rs | 29 +-- 23 files changed, 532 insertions(+), 138 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index c647d146d46b..f2a8c80c177f 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -139,7 +139,7 @@ pub(crate) async fn run_pre_tool_use_hooks( tool_use_id: String, tool_name: String, matcher_aliases: Vec, - command: String, + tool_input: Value, ) -> Option { let request = PreToolUseRequest { session_id: sess.conversation_id, @@ -151,7 +151,7 @@ pub(crate) async fn run_pre_tool_use_hooks( tool_name, matcher_aliases, tool_use_id, - command, + tool_input, }; let preview_runs = sess.hooks().preview_pre_tool_use(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; @@ -185,8 +185,7 @@ pub(crate) async fn run_permission_request_hooks( tool_name: payload.tool_name.name().to_string(), matcher_aliases: payload.tool_name.matcher_aliases().to_vec(), run_id_suffix: run_id_suffix.to_string(), - command: payload.command, - description: payload.description, + tool_input: payload.tool_input, }; let preview_runs = sess.hooks().preview_permission_request(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; @@ -202,7 +201,7 @@ pub(crate) async fn run_permission_request_hooks( /// Runs matching `PostToolUse` hooks after a tool has produced a successful output. /// -/// The `tool_name`, matcher aliases, `command`, and `tool_response` values are +/// The `tool_name`, matcher aliases, `tool_input`, and `tool_response` values are /// already adapted by the tool handler into the stable hook contract. Passing /// raw internal tool data here would leak implementation details into user hook /// matchers and hook logs. @@ -212,7 +211,7 @@ pub(crate) async fn run_post_tool_use_hooks( tool_use_id: String, tool_name: String, matcher_aliases: Vec, - command: String, + tool_input: Value, tool_response: Value, ) -> PostToolUseOutcome { let request = PostToolUseRequest { @@ -225,7 +224,7 @@ pub(crate) async fn run_post_tool_use_hooks( tool_name, matcher_aliases, tool_use_id, - command, + tool_input, tool_response, }; let preview_runs = sess.hooks().preview_post_tool_use(&request); diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 82bbc7ca4a4d..e588b95fd2e3 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -25,16 +25,19 @@ 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::run_permission_request_hooks; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::tools::sandboxing::PermissionRequestPayload; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; use codex_analytics::build_track_events_context; use codex_config::types::AppToolApproval; use codex_features::Feature; +use codex_hooks::PermissionRequestDecision; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::SandboxState; use codex_mcp::declared_openai_file_input_param_names; @@ -654,6 +657,7 @@ pub(crate) struct McpToolApprovalMetadata { connector_description: Option, tool_title: Option, tool_description: Option, + model_tool_name: String, mcp_app_resource_uri: Option, codex_apps_meta: Option>, openai_file_input_params: Option>, @@ -863,6 +867,35 @@ async fn maybe_request_mcp_tool_approval( { return Some(McpToolApprovalDecision::Accept); } + + let tool_name = metadata + .map(|metadata| metadata.model_tool_name.clone()) + .unwrap_or_else(|| format!("mcp__{}__{}", invocation.server, invocation.tool)); + match run_permission_request_hooks( + sess, + turn_context, + call_id, + PermissionRequestPayload { + tool_name, + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), + }, + ) + .await + { + Some(PermissionRequestDecision::Allow) => { + return Some(McpToolApprovalDecision::Accept); + } + Some(PermissionRequestDecision::Deny { message }) => { + return Some(McpToolApprovalDecision::Decline { + message: Some(message), + }); + } + None => {} + } + let tool_call_mcp_elicitation_enabled = turn_context .config .features @@ -1149,6 +1182,10 @@ pub(crate) async fn lookup_mcp_tool_metadata( connector_description, tool_title: tool_info.tool.title, tool_description: tool_info.tool.description.map(std::borrow::Cow::into_owned), + model_tool_name: format!( + "{}{}", + tool_info.callable_namespace, tool_info.callable_name + ), mcp_app_resource_uri: get_mcp_app_resource_uri(tool_info.tool.meta.as_deref()), codex_apps_meta: tool_info .tool diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index d20f2339510e..b47a81a94344 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -12,6 +12,8 @@ use codex_config::types::ApprovalsReviewer; use codex_config::types::AppsConfigToml; use codex_config::types::McpServerConfig; use codex_config::types::McpServerToolConfig; +use codex_hooks::Hooks; +use codex_hooks::HooksConfig; use codex_model_provider::create_model_provider; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; @@ -60,6 +62,7 @@ fn approval_metadata( connector_description: connector_description.map(str::to_string), tool_title: tool_title.map(str::to_string), tool_description: tool_description.map(str::to_string), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -76,6 +79,67 @@ fn prompt_options( } } +fn install_mcp_permission_request_hook( + session: &mut Session, + turn_context: &TurnContext, + matcher: &str, + hook_output: &serde_json::Value, +) -> std::path::PathBuf { + let script_path = turn_context + .config + .codex_home + .join("mcp_permission_request_hook.py"); + let log_path = turn_context + .config + .codex_home + .join("mcp_permission_request_hook_log.jsonl"); + let hook_output = hook_output.to_string(); + std::fs::create_dir_all(&turn_context.config.codex_home) + .expect("create codex home for MCP permission hook"); + 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({hook_output:?}) +"#, + log_path = log_path.display(), + hook_output = hook_output, + ); + + std::fs::write(&script_path, script).expect("write MCP permission hook script"); + std::fs::write( + turn_context.config.codex_home.join("hooks.json"), + serde_json::json!({ + "hooks": { + "PermissionRequest": [{ + "matcher": matcher, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + }] + }] + } + }) + .to_string(), + ) + .expect("write hooks.json"); + + session.services.hooks = Hooks::new(HooksConfig { + feature_enabled: true, + config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + shell_program: Some("/bin/sh".to_string()), + shell_args: vec!["-c".to_string()], + ..HooksConfig::default() + }); + + log_path.to_path_buf() +} + #[test] fn mcp_app_resource_uri_reads_known_tool_meta_keys() { let nested = serde_json::json!({ @@ -620,6 +684,7 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps connector_description: Some("Manage events".to_string()), tool_title: Some("Create Event".to_string()), tool_description: Some("Create a calendar event.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: Some( serde_json::json!({ @@ -807,6 +872,7 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { connector_description: None, tool_title: None, tool_description: None, + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1371,6 +1437,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1443,6 +1510,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1461,6 +1529,148 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { assert_eq!(decision, None); } +#[tokio::test] +async fn permission_request_hook_allows_mcp_tool_call() { + let (mut session, turn_context) = make_session_and_context().await; + let log_path = install_mcp_permission_request_hook( + &mut session, + &turn_context, + "mcp__memory__.*", + &serde_json::json!({ + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { "behavior": "allow" } + } + }), + ); + 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, + model_tool_name: "mcp__memory__create_entities".to_string(), + 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, + Some(&metadata), + AppToolApproval::Auto, + ) + .await; + + assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); + let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log"); + let inputs = log + .lines() + .map(|line| serde_json::from_str::(line).expect("parse hook input")) + .collect::>(); + assert_eq!( + inputs, + vec![serde_json::json!({ + "session_id": session.conversation_id, + "turn_id": "turn_id", + "cwd": turn_context.cwd, + "transcript_path": null, + "model": turn_context.model_info.slug, + "permission_mode": "default", + "tool_name": "mcp__memory__create_entities", + "hook_event_name": "PermissionRequest", + "tool_input": { + "entities": [{ + "name": "Ada", + "entityType": "person" + }] + } + })] + ); +} + +#[tokio::test] +async fn permission_request_hook_runs_after_remembered_mcp_approval() { + let (mut session, turn_context) = make_session_and_context().await; + let log_path = install_mcp_permission_request_hook( + &mut session, + &turn_context, + "mcp__memory__.*", + &serde_json::json!({ + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "deny", + "message": "should be skipped" + } + } + }), + ); + let invocation = McpInvocation { + server: "memory".to_string(), + tool: "create_entities".to_string(), + arguments: Some(serde_json::json!({ "entities": [] })), + }; + 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, + model_tool_name: "mcp__memory__create_entities".to_string(), + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + }; + let remembered_key = + session_mcp_tool_approval_key(&invocation, Some(&metadata), AppToolApproval::Auto) + .expect("memory MCP tool should support session approval"); + remember_mcp_tool_approval(&session, remembered_key).await; + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-mcp-remembered", + &invocation, + Some(&metadata), + AppToolApproval::Auto, + ) + .await; + + assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); + assert!( + !log_path.exists(), + "remembered approval should skip PermissionRequest hooks" + ); +} + #[tokio::test] async fn guardian_mode_mcp_denial_returns_rationale_message() { let server = start_mock_server().await; @@ -1518,6 +1728,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { connector_description: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Reads calendar data.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1570,6 +1781,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1648,6 +1860,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1719,6 +1932,7 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { connector_description: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1790,6 +2004,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1869,6 +2084,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1975,6 +2191,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index 15ada81bb96f..9e703768b736 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -162,6 +162,10 @@ impl ToolOutput for McpToolOutput { JsonValue::String(format!("failed to serialize mcp result: {err}")) }) } + + fn post_tool_use_response(&self, _call_id: &str, _payload: &ToolPayload) -> Option { + serde_json::to_value(&self.result).ok() + } } impl McpToolOutput { diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index b24db734b913..760a5c6269b0 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -6,9 +6,14 @@ use crate::mcp_tool_call::handle_mcp_tool_call; use crate::original_image_detail::can_request_original_image_detail; use crate::tools::context::McpToolOutput; use crate::tools::context::ToolInvocation; +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 serde_json::Value; pub struct McpHandler; impl ToolHandler for McpHandler { @@ -18,6 +23,37 @@ 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(invocation.tool_name.display()), + tool_input: mcp_hook_tool_input(raw_arguments), + display_command: None, + }) + } + + fn post_tool_use_payload( + &self, + call_id: &str, + tool_name: &codex_tools::ToolName, + payload: &ToolPayload, + result: &dyn ToolOutput, + ) -> Option { + let ToolPayload::Mcp { raw_arguments, .. } = payload else { + return None; + }; + + let tool_response = result.post_tool_use_response(call_id, payload)?; + Some(PostToolUsePayload { + tool_name: HookToolName::new(tool_name.display()), + tool_input: mcp_hook_tool_input(raw_arguments), + tool_response, + }) + } + async fn handle(&self, invocation: ToolInvocation) -> Result { let ToolInvocation { session, @@ -61,3 +97,102 @@ 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::*; + use crate::session::tests::make_session_and_context; + use crate::turn_diff_tracker::TurnDiffTracker; + use pretty_assertions::assert_eq; + use serde_json::json; + use std::time::Duration; + use tokio::sync::Mutex; + + #[tokio::test] + async fn mcp_pre_tool_use_payload_uses_model_tool_name_and_raw_args() { + let payload = ToolPayload::Mcp { + server: "memory".to_string(), + tool: "create_entities".to_string(), + raw_arguments: json!({ + "entities": [{ + "name": "Ada", + "entityType": "person" + }] + }) + .to_string(), + }; + let (session, turn) = make_session_and_context().await; + + assert_eq!( + McpHandler.pre_tool_use_payload(&ToolInvocation { + session: session.into(), + turn: turn.into(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-mcp-pre".to_string(), + tool_name: codex_tools::ToolName::namespaced("mcp__memory__", "create_entities"), + payload, + }), + Some(PreToolUsePayload { + tool_name: "mcp__memory__create_entities".to_string(), + tool_input: json!({ + "entities": [{ + "name": "Ada", + "entityType": "person" + }] + }), + display_command: None, + }) + ); + } + + #[test] + fn mcp_post_tool_use_payload_uses_model_tool_name_args_and_result() { + let payload = ToolPayload::Mcp { + server: "filesystem".to_string(), + tool: "read_file".to_string(), + raw_arguments: json!({ "path": "/tmp/notes.txt" }).to_string(), + }; + let output = McpToolOutput { + result: codex_protocol::mcp::CallToolResult { + content: vec![json!({ + "type": "text", + "text": "notes" + })], + structured_content: Some(json!({ "bytes": 5 })), + is_error: None, + meta: None, + }, + wall_time: Duration::from_millis(42), + original_image_detail_supported: true, + }; + let tool_name = codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"); + + assert_eq!( + McpHandler.post_tool_use_payload("call-mcp-post", &tool_name, &payload, &output), + Some(PostToolUsePayload { + tool_name: "mcp__filesystem__read_file".to_string(), + tool_input: json!({ "path": "/tmp/notes.txt" }), + tool_response: json!({ + "content": [{ + "type": "text", + "text": "notes" + }], + "structuredContent": { "bytes": 5 } + }), + }) + ); + } + + #[test] + fn mcp_hook_tool_input_defaults_empty_args_to_object() { + assert_eq!(mcp_hook_tool_input(" "), json!({})); + } +} diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 00343d3ceaf3..8367c1677326 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -208,20 +208,23 @@ impl ToolHandler for ShellHandler { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { shell_payload_command(&invocation.payload).map(|command| PreToolUsePayload { tool_name: HookToolName::bash(), - command, + tool_input: serde_json::json!({ "command": command }), + display_command: Some(command), }) } fn post_tool_use_payload( &self, call_id: &str, + _tool_name: &codex_tools::ToolName, payload: &ToolPayload, result: &dyn ToolOutput, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; + let command = shell_payload_command(payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - command: shell_payload_command(payload)?, + tool_input: serde_json::json!({ "command": command }), tool_response, }) } @@ -320,20 +323,23 @@ impl ToolHandler for ShellCommandHandler { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { shell_command_payload_command(&invocation.payload).map(|command| PreToolUsePayload { tool_name: HookToolName::bash(), - command, + tool_input: serde_json::json!({ "command": command }), + display_command: Some(command), }) } fn post_tool_use_payload( &self, call_id: &str, + _tool_name: &codex_tools::ToolName, payload: &ToolPayload, result: &dyn ToolOutput, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; + let command = shell_command_payload_command(payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - command: shell_command_payload_command(payload)?, + tool_input: serde_json::json!({ "command": command }), tool_response, }) } diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index b36dd0225c97..7f6639196686 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -234,7 +234,8 @@ async fn shell_pre_tool_use_payload_uses_joined_command() { }), Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), - command: "bash -lc 'printf hi'".to_string(), + tool_input: json!({ "command": "bash -lc 'printf hi'" }), + display_command: Some("bash -lc 'printf hi'".to_string()), }) ); } @@ -261,7 +262,8 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { }), Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), - command: "printf shell command".to_string(), + tool_input: json!({ "command": "printf shell command" }), + display_command: Some("printf shell command".to_string()), }) ); } @@ -279,12 +281,13 @@ fn build_post_tool_use_payload_uses_tool_output_wire_value() { let handler = ShellCommandHandler { backend: super::ShellCommandBackend::Classic, }; + let tool_name = codex_tools::ToolName::plain("shell_command"); assert_eq!( - handler.post_tool_use_payload("call-42", &payload, &output), + handler.post_tool_use_payload("call-42", &tool_name, &payload, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), - command: "printf shell command".to_string(), + tool_input: json!({ "command": "printf shell command" }), tool_response: json!("shell output"), }) ); diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index b25f5a69b791..ffdb23bb17ea 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -139,13 +139,15 @@ impl ToolHandler for UnifiedExecHandler { .ok() .map(|args| PreToolUsePayload { tool_name: HookToolName::bash(), - command: args.cmd, + tool_input: serde_json::json!({ "command": args.cmd }), + display_command: Some(args.cmd), }) } fn post_tool_use_payload( &self, call_id: &str, + _tool_name: &codex_tools::ToolName, payload: &ToolPayload, result: &dyn ToolOutput, ) -> Option { @@ -161,7 +163,7 @@ impl ToolHandler for UnifiedExecHandler { let tool_response = result.post_tool_use_response(call_id, payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - command: args.cmd, + tool_input: serde_json::json!({ "command": args.cmd }), tool_response, }) } 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 26f86bd66df9..b396c1b2b619 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -219,7 +219,8 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { }), Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), - command: "printf exec command".to_string(), + tool_input: serde_json::json!({ "command": "printf exec command" }), + display_command: Some("printf exec command".to_string()), }) ); } @@ -266,12 +267,13 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co "echo three".to_string(), ]), }; + let tool_name = codex_tools::ToolName::plain("exec_command"); assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-43", &payload, &output), + UnifiedExecHandler.post_tool_use_payload("call-43", &tool_name, &payload, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), - command: "echo three".to_string(), + tool_input: serde_json::json!({ "command": "echo three" }), tool_response: serde_json::json!("three"), }) ); @@ -297,9 +299,10 @@ fn exec_command_post_tool_use_payload_skips_interactive_exec() { "echo three".to_string(), ]), }; + let tool_name = codex_tools::ToolName::plain("exec_command"); assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-44", &payload, &output), + UnifiedExecHandler.post_tool_use_payload("call-44", &tool_name, &payload, &output), None ); } @@ -324,9 +327,10 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { "echo three".to_string(), ]), }; + let tool_name = codex_tools::ToolName::plain("exec_command"); assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-45", &payload, &output), + UnifiedExecHandler.post_tool_use_payload("call-45", &tool_name, &payload, &output), None ); } diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 4be18bad844c..cf7b0b5a23b0 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -385,8 +385,10 @@ impl NetworkApprovalService { &guardian_approval_id, PermissionRequestPayload { tool_name: HookToolName::bash(), - command, - description: Some(format!("network-access {target}")), + tool_input: serde_json::json!({ + "command": command, + "description": format!("network-access {target}"), + }), }, ) .await diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index d70550d0e542..f1e9d7f61565 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -71,6 +71,7 @@ pub trait ToolHandler: Send + Sync { fn post_tool_use_payload( &self, _call_id: &str, + _tool_name: &ToolName, _payload: &ToolPayload, _result: &dyn ToolOutput, ) -> Option { @@ -135,8 +136,12 @@ pub(crate) struct PreToolUsePayload { /// The canonical name is serialized to hook stdin, while aliases are used /// only for matcher compatibility. pub(crate) tool_name: HookToolName, - /// Command-shaped input exposed at `tool_input.command`. - pub(crate) command: String, + /// Tool-specific input exposed at `tool_input`. + /// + /// Shell-like tools use `{ "command": ... }`; MCP tools use their resolved + /// JSON arguments. + pub(crate) tool_input: Value, + pub(crate) display_command: Option, } #[derive(Debug, Clone, PartialEq)] @@ -146,8 +151,8 @@ pub(crate) struct PostToolUsePayload { /// Keep this aligned with the corresponding pre-use payload so external /// hook consumers can pair events by `tool_use_id`. pub(crate) tool_name: HookToolName, - /// Command-shaped input exposed at `tool_input.command`. - pub(crate) command: String, + /// Tool-specific input exposed at `tool_input`. + pub(crate) tool_input: Value, /// Tool result exposed at `tool_response`. pub(crate) tool_response: Value, } @@ -162,6 +167,7 @@ trait AnyToolHandler: Send + Sync { fn post_tool_use_payload( &self, call_id: &str, + tool_name: &ToolName, payload: &ToolPayload, result: &dyn ToolOutput, ) -> Option; @@ -193,10 +199,11 @@ where fn post_tool_use_payload( &self, call_id: &str, + tool_name: &ToolName, payload: &ToolPayload, result: &dyn ToolOutput, ) -> Option { - ToolHandler::post_tool_use_payload(self, call_id, payload, result) + ToolHandler::post_tool_use_payload(self, call_id, tool_name, payload, result) } fn create_diff_consumer(&self) -> Option> { @@ -346,14 +353,19 @@ impl ToolRegistry { invocation.call_id.clone(), pre_tool_use_payload.tool_name.name().to_string(), pre_tool_use_payload.tool_name.matcher_aliases().to_vec(), - pre_tool_use_payload.command.clone(), + pre_tool_use_payload.tool_input.clone(), ) .await { - return Err(FunctionCallError::RespondToModel(format!( - "Command blocked by PreToolUse hook: {reason}. Command: {}", - pre_tool_use_payload.command - ))); + let message = if let Some(command) = pre_tool_use_payload.display_command { + format!("Command blocked by PreToolUse hook: {reason}. Command: {command}") + } else { + format!( + "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", + pre_tool_use_payload.tool_name.name() + ) + }; + return Err(FunctionCallError::RespondToModel(message)); } let is_mutating = handler.is_mutating(&invocation).await; @@ -403,6 +415,7 @@ impl ToolRegistry { guard.as_ref().and_then(|result| { handler.post_tool_use_payload( &result.call_id, + &tool_name, &result.payload, result.result.as_ref(), ) @@ -418,7 +431,7 @@ impl ToolRegistry { invocation.call_id.clone(), post_tool_use_payload.tool_name.name().to_string(), post_tool_use_payload.tool_name.matcher_aliases().to_vec(), - post_tool_use_payload.command, + post_tool_use_payload.tool_input, post_tool_use_payload.tool_response, ) .await, diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 34292421a088..5a2183a17c58 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -203,8 +203,10 @@ impl Approvable for ShellRuntime { fn permission_request_payload(&self, req: &ShellRequest) -> Option { Some(PermissionRequestPayload { tool_name: HookToolName::bash(), - command: req.hook_command.clone(), - description: req.justification.clone(), + tool_input: serde_json::json!({ + "command": req.hook_command, + "description": req.justification, + }), }) } 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 361de9a96baa..9f746cd2326c 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -404,8 +404,10 @@ impl CoreShellActionProvider { // 1) Run PermissionRequest hooks let permission_request = PermissionRequestPayload { tool_name: HookToolName::bash(), - command: codex_shell_command::parse_command::shlex_join(&command), - description: None, + tool_input: serde_json::json!({ + "command": codex_shell_command::parse_command::shlex_join(&command), + "description": null, + }), }; let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); match run_permission_request_hooks( diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 955f7d830cee..41e31903aa4a 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -188,8 +188,10 @@ impl Approvable for UnifiedExecRuntime<'_> { ) -> Option { Some(PermissionRequestPayload { tool_name: HookToolName::bash(), - command: req.hook_command.clone(), - description: req.justification.clone(), + tool_input: serde_json::json!({ + "command": req.hook_command, + "description": req.justification, + }), }) } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index fcdcd785072f..161625b083c0 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -135,8 +135,7 @@ pub(crate) struct ApprovalCtx<'a> { #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct PermissionRequestPayload { pub tool_name: HookToolName, - pub command: String, - pub description: Option, + pub tool_input: serde_json::Value, } // Specifies what tool orchestrator should do with a given tool call. diff --git a/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json index 7d432e1551ee..55b3843c0b89 100644 --- a/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json @@ -7,21 +7,6 @@ "string", "null" ] - }, - "PermissionRequestToolInput": { - "additionalProperties": false, - "properties": { - "command": { - "type": "string" - }, - "description": { - "type": "string" - } - }, - "required": [ - "command" - ], - "type": "object" } }, "properties": { @@ -48,9 +33,7 @@ "session_id": { "type": "string" }, - "tool_input": { - "$ref": "#/definitions/PermissionRequestToolInput" - }, + "tool_input": true, "tool_name": { "type": "string" }, diff --git a/codex-rs/hooks/schema/generated/post-tool-use.command.input.schema.json b/codex-rs/hooks/schema/generated/post-tool-use.command.input.schema.json index 6a3fcfc47c89..1ec5fb3082df 100644 --- a/codex-rs/hooks/schema/generated/post-tool-use.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/post-tool-use.command.input.schema.json @@ -7,18 +7,6 @@ "string", "null" ] - }, - "PostToolUseToolInput": { - "additionalProperties": false, - "properties": { - "command": { - "type": "string" - } - }, - "required": [ - "command" - ], - "type": "object" } }, "properties": { @@ -45,9 +33,7 @@ "session_id": { "type": "string" }, - "tool_input": { - "$ref": "#/definitions/PostToolUseToolInput" - }, + "tool_input": true, "tool_name": { "type": "string" }, diff --git a/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json b/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json index df3bec7ff6e5..f9cde0102028 100644 --- a/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json @@ -7,18 +7,6 @@ "string", "null" ] - }, - "PreToolUseToolInput": { - "additionalProperties": false, - "properties": { - "command": { - "type": "string" - } - }, - "required": [ - "command" - ], - "type": "object" } }, "properties": { @@ -45,9 +33,7 @@ "session_id": { "type": "string" }, - "tool_input": { - "$ref": "#/definitions/PreToolUseToolInput" - }, + "tool_input": true, "tool_name": { "type": "string" }, diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index e74c2912dd48..7e0abccaeac5 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -109,7 +109,7 @@ pub(crate) fn matcher_pattern_for_event( } pub(crate) fn validate_matcher_pattern(matcher: &str) -> Result<(), regex::Error> { - if is_match_all_matcher(matcher) { + if is_match_all_matcher(matcher) || is_exact_matcher(matcher) { return Ok(()); } regex::Regex::new(matcher).map(|_| ()) @@ -119,6 +119,7 @@ pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> boo match matcher { None => true, Some(matcher) if is_match_all_matcher(matcher) => true, + Some(matcher) if is_exact_matcher(matcher) => input == Some(matcher), Some(matcher) => input .and_then(|input| { regex::Regex::new(matcher) @@ -144,6 +145,15 @@ fn is_match_all_matcher(matcher: &str) -> bool { matcher.is_empty() || matcher == "*" } +fn is_exact_matcher(matcher: &str) -> bool { + !matcher.chars().any(|ch| { + matches!( + ch, + '.' | '+' | '*' | '?' | '^' | '$' | '(' | ')' | '[' | ']' | '{' | '}' | '|' | '\\' + ) + }) +} + #[cfg(test)] mod tests { use codex_protocol::protocol::HookEventName; @@ -181,6 +191,37 @@ mod tests { assert_eq!(validate_matcher_pattern("Edit|Write"), Ok(())); } + #[test] + fn literal_matcher_uses_exact_matching() { + assert!(matches_matcher(Some("Bash"), Some("Bash"))); + assert!(!matches_matcher(Some("Bash"), Some("BashOutput"))); + assert!(matches_matcher( + Some("mcp__memory__create_entities"), + Some("mcp__memory__create_entities") + )); + assert!(!matches_matcher( + Some("mcp__memory"), + Some("mcp__memory__create_entities") + )); + assert_eq!(validate_matcher_pattern("mcp__memory"), Ok(())); + } + + #[test] + fn mcp_matchers_support_regex_wildcards() { + assert!(matches_matcher( + Some("mcp__memory__.*"), + Some("mcp__memory__create_entities") + )); + assert!(matches_matcher( + Some("mcp__.*__write.*"), + Some("mcp__filesystem__write_file") + )); + assert!(!matches_matcher( + Some("mcp__.*__write.*"), + Some("mcp__filesystem__read_file") + )); + } + #[test] fn matcher_supports_anchored_regexes() { assert!(matches_matcher(Some("^Bash$"), Some("Bash"))); diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 59b8f61d22da..2cebd0e002ed 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -22,7 +22,6 @@ use crate::engine::command_runner::CommandRunResult; use crate::engine::dispatcher; use crate::engine::output_parser; use crate::schema::PermissionRequestCommandInput; -use crate::schema::PermissionRequestToolInput; use codex_protocol::ThreadId; use codex_protocol::protocol::HookCompletedEvent; use codex_protocol::protocol::HookEventName; @@ -30,6 +29,7 @@ use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookRunSummary; +use serde_json::Value; #[derive(Debug, Clone)] pub struct PermissionRequestRequest { @@ -42,8 +42,7 @@ pub struct PermissionRequestRequest { pub tool_name: String, pub matcher_aliases: Vec, pub run_id_suffix: String, - pub command: String, - pub description: Option, + pub tool_input: Value, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -178,10 +177,7 @@ fn build_command_input(request: &PermissionRequestRequest) -> PermissionRequestC model: request.model.clone(), permission_mode: request.permission_mode.clone(), tool_name: request.tool_name.clone(), - tool_input: PermissionRequestToolInput { - command: request.command.clone(), - description: request.description.clone(), - }, + tool_input: request.tool_input.clone(), } } diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 294b08628e21..2e9a7aeda55e 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -17,7 +17,6 @@ use crate::engine::command_runner::CommandRunResult; use crate::engine::dispatcher; use crate::engine::output_parser; use crate::schema::PostToolUseCommandInput; -use crate::schema::PostToolUseToolInput; #[derive(Debug, Clone)] pub struct PostToolUseRequest { @@ -30,7 +29,7 @@ pub struct PostToolUseRequest { pub tool_name: String, pub matcher_aliases: Vec, pub tool_use_id: String, - pub command: String, + pub tool_input: Value, pub tool_response: Value, } @@ -146,7 +145,8 @@ pub(crate) async fn run( /// /// Handler selection may include internal matcher aliases, but hook stdin keeps /// the canonical `tool_name` for logs and for consumers that pair pre/post -/// events across processes. +/// events across processes. Shell-like tools pass `{ "command": ... }` as +/// `tool_input`; MCP tools pass their resolved JSON arguments. fn command_input_json(request: &PostToolUseRequest) -> Result { serde_json::to_string(&PostToolUseCommandInput { session_id: request.session_id.to_string(), @@ -157,9 +157,7 @@ fn command_input_json(request: &PostToolUseRequest) -> Result, pub tool_use_id: String, - pub command: String, + pub tool_input: Value, } #[derive(Debug)] @@ -124,7 +125,8 @@ pub(crate) async fn run( /// /// Handler selection may include internal matcher aliases, but hook stdin keeps /// the canonical `tool_name` so audit logs and downstream policy decisions stay -/// stable. +/// stable. Shell-like tools pass `{ "command": ... }` as `tool_input`; MCP +/// tools pass their resolved JSON arguments. fn command_input_json(request: &PreToolUseRequest) -> Result { serde_json::to_string(&PreToolUseCommandInput { session_id: request.session_id.to_string(), @@ -135,9 +137,7 @@ fn command_input_json(request: &PreToolUseRequest) -> Result, -} - #[derive(Debug, Clone, Serialize, JsonSchema)] #[serde(deny_unknown_fields)] #[schemars(rename = "permission-request.command.input")] @@ -260,14 +244,7 @@ pub(crate) struct PermissionRequestCommandInput { #[schemars(schema_with = "permission_mode_schema")] pub permission_mode: String, pub tool_name: String, - pub tool_input: PermissionRequestToolInput, -} - -#[derive(Debug, Clone, Serialize, JsonSchema)] -#[serde(rename_all = "camelCase")] -#[serde(deny_unknown_fields)] -pub(crate) struct PostToolUseToolInput { - pub command: String, + pub tool_input: Value, } #[derive(Debug, Clone, Serialize, JsonSchema)] @@ -285,7 +262,7 @@ pub(crate) struct PostToolUseCommandInput { #[schemars(schema_with = "permission_mode_schema")] pub permission_mode: String, pub tool_name: String, - pub tool_input: PostToolUseToolInput, + pub tool_input: Value, pub tool_response: Value, pub tool_use_id: String, } From 7009fc86aa46104eef1597d5d73d002698893da1 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 12:30:43 -0700 Subject: [PATCH 05/22] clean up display_command --- codex-rs/core/src/tools/handlers/mcp.rs | 2 -- codex-rs/core/src/tools/handlers/shell.rs | 2 -- codex-rs/core/src/tools/handlers/shell_tests.rs | 2 -- codex-rs/core/src/tools/handlers/unified_exec.rs | 1 - codex-rs/core/src/tools/handlers/unified_exec_tests.rs | 1 - codex-rs/core/src/tools/registry.rs | 9 +++++++-- 6 files changed, 7 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 760a5c6269b0..04bd2961d486 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -31,7 +31,6 @@ impl ToolHandler for McpHandler { Some(PreToolUsePayload { tool_name: HookToolName::new(invocation.tool_name.display()), tool_input: mcp_hook_tool_input(raw_arguments), - display_command: None, }) } @@ -148,7 +147,6 @@ mod tests { "entityType": "person" }] }), - display_command: None, }) ); } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 8367c1677326..b8038fc49302 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -209,7 +209,6 @@ impl ToolHandler for ShellHandler { shell_payload_command(&invocation.payload).map(|command| PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": command }), - display_command: Some(command), }) } @@ -324,7 +323,6 @@ impl ToolHandler for ShellCommandHandler { shell_command_payload_command(&invocation.payload).map(|command| PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": command }), - display_command: Some(command), }) } diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 7f6639196686..962acc8a93b9 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -235,7 +235,6 @@ async fn shell_pre_tool_use_payload_uses_joined_command() { Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "bash -lc 'printf hi'" }), - display_command: Some("bash -lc 'printf hi'".to_string()), }) ); } @@ -263,7 +262,6 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "printf shell command" }), - display_command: Some("printf shell command".to_string()), }) ); } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index ffdb23bb17ea..769e433a3d05 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -140,7 +140,6 @@ impl ToolHandler for UnifiedExecHandler { .map(|args| PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": args.cmd }), - display_command: Some(args.cmd), }) } 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 b396c1b2b619..43660c485323 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -220,7 +220,6 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { Some(crate::tools::registry::PreToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": "printf exec command" }), - display_command: Some("printf exec command".to_string()), }) ); } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index f1e9d7f61565..f023e5f00a52 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -141,7 +141,6 @@ pub(crate) struct PreToolUsePayload { /// Shell-like tools use `{ "command": ... }`; MCP tools use their resolved /// JSON arguments. pub(crate) tool_input: Value, - pub(crate) display_command: Option, } #[derive(Debug, Clone, PartialEq)] @@ -357,7 +356,13 @@ impl ToolRegistry { ) .await { - let message = if let Some(command) = pre_tool_use_payload.display_command { + // Bash hook payloads expose the executable text as tool_input.command. + let message = if pre_tool_use_payload.tool_name == "Bash" + && let Some(command) = pre_tool_use_payload + .tool_input + .get("command") + .and_then(Value::as_str) + { format!("Command blocked by PreToolUse hook: {reason}. Command: {command}") } else { format!( From a2b962784ea61bef31289602765057628f032a78 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 13:02:28 -0700 Subject: [PATCH 06/22] pass ToolInvocation --- codex-rs/core/src/tools/handlers/mcp.rs | 29 ++++++---- codex-rs/core/src/tools/handlers/shell.rs | 18 +++---- .../core/src/tools/handlers/shell_tests.rs | 18 +++++-- .../core/src/tools/handlers/unified_exec.rs | 9 ++-- .../src/tools/handlers/unified_exec_tests.rs | 54 ++++++++++++++----- codex-rs/core/src/tools/registry.rs | 23 +++----- 6 files changed, 93 insertions(+), 58 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 04bd2961d486..7a3675d1c471 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -36,18 +36,17 @@ impl ToolHandler for McpHandler { fn post_tool_use_payload( &self, - call_id: &str, - tool_name: &codex_tools::ToolName, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &dyn ToolOutput, ) -> Option { - let ToolPayload::Mcp { raw_arguments, .. } = payload else { + let ToolPayload::Mcp { raw_arguments, .. } = &invocation.payload else { return None; }; - let tool_response = result.post_tool_use_response(call_id, payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { - tool_name: HookToolName::new(tool_name.display()), + tool_name: HookToolName::new(invocation.tool_name.display()), tool_input: mcp_hook_tool_input(raw_arguments), tool_response, }) @@ -151,8 +150,8 @@ mod tests { ); } - #[test] - fn mcp_post_tool_use_payload_uses_model_tool_name_args_and_result() { + #[tokio::test] + async fn mcp_post_tool_use_payload_uses_model_tool_name_args_and_result() { let payload = ToolPayload::Mcp { server: "filesystem".to_string(), tool: "read_file".to_string(), @@ -171,10 +170,20 @@ mod tests { wall_time: Duration::from_millis(42), original_image_detail_supported: true, }; - let tool_name = codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"); + let (session, turn) = make_session_and_context().await; assert_eq!( - McpHandler.post_tool_use_payload("call-mcp-post", &tool_name, &payload, &output), + McpHandler.post_tool_use_payload( + &ToolInvocation { + session: session.into(), + turn: turn.into(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-mcp-post".to_string(), + tool_name: codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"), + payload, + }, + &output, + ), Some(PostToolUsePayload { tool_name: "mcp__filesystem__read_file".to_string(), tool_input: json!({ "path": "/tmp/notes.txt" }), diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index b8038fc49302..f57cf6e28cb8 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -214,13 +214,12 @@ impl ToolHandler for ShellHandler { fn post_tool_use_payload( &self, - call_id: &str, - _tool_name: &codex_tools::ToolName, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &dyn ToolOutput, ) -> Option { - let tool_response = result.post_tool_use_response(call_id, payload)?; - let command = shell_payload_command(payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; + let command = shell_payload_command(&invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": command }), @@ -328,13 +327,12 @@ impl ToolHandler for ShellCommandHandler { fn post_tool_use_payload( &self, - call_id: &str, - _tool_name: &codex_tools::ToolName, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &dyn ToolOutput, ) -> Option { - let tool_response = result.post_tool_use_response(call_id, payload)?; - let command = shell_command_payload_command(payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; + let command = shell_command_payload_command(&invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": command }), diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 962acc8a93b9..809fecda932b 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -266,8 +266,8 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() { ); } -#[test] -fn build_post_tool_use_payload_uses_tool_output_wire_value() { +#[tokio::test] +async fn build_post_tool_use_payload_uses_tool_output_wire_value() { let payload = ToolPayload::Function { arguments: json!({ "command": "printf shell command" }).to_string(), }; @@ -279,10 +279,20 @@ fn build_post_tool_use_payload_uses_tool_output_wire_value() { let handler = ShellCommandHandler { backend: super::ShellCommandBackend::Classic, }; - let tool_name = codex_tools::ToolName::plain("shell_command"); + let (session, turn) = make_session_and_context().await; assert_eq!( - handler.post_tool_use_payload("call-42", &tool_name, &payload, &output), + handler.post_tool_use_payload( + &ToolInvocation { + session: session.into(), + turn: turn.into(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-42".to_string(), + tool_name: codex_tools::ToolName::plain("shell_command"), + payload, + }, + &output, + ), Some(crate::tools::registry::PostToolUsePayload { 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 769e433a3d05..974a1cd618a3 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -145,12 +145,10 @@ impl ToolHandler for UnifiedExecHandler { fn post_tool_use_payload( &self, - call_id: &str, - _tool_name: &codex_tools::ToolName, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &dyn ToolOutput, ) -> Option { - let ToolPayload::Function { arguments } = payload else { + let ToolPayload::Function { arguments } = &invocation.payload else { return None; }; @@ -159,7 +157,8 @@ impl ToolHandler for UnifiedExecHandler { return None; } - let tool_response = result.post_tool_use_response(call_id, payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": args.cmd }), 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 43660c485323..b7dca0d25963 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -246,8 +246,8 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { ); } -#[test] -fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_commands() { +#[tokio::test] +async fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_commands() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "cmd": "echo three", "tty": false }).to_string(), }; @@ -266,10 +266,20 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co "echo three".to_string(), ]), }; - let tool_name = codex_tools::ToolName::plain("exec_command"); + let (session, turn) = make_session_and_context().await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-43", &tool_name, &payload, &output), + UnifiedExecHandler.post_tool_use_payload( + &ToolInvocation { + session: session.into(), + turn: turn.into(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-43".to_string(), + tool_name: codex_tools::ToolName::plain("exec_command"), + payload, + }, + &output, + ), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_input: serde_json::json!({ "command": "echo three" }), @@ -278,8 +288,8 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co ); } -#[test] -fn exec_command_post_tool_use_payload_skips_interactive_exec() { +#[tokio::test] +async fn exec_command_post_tool_use_payload_skips_interactive_exec() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "cmd": "echo three", "tty": true }).to_string(), }; @@ -298,16 +308,26 @@ fn exec_command_post_tool_use_payload_skips_interactive_exec() { "echo three".to_string(), ]), }; - let tool_name = codex_tools::ToolName::plain("exec_command"); + let (session, turn) = make_session_and_context().await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-44", &tool_name, &payload, &output), + UnifiedExecHandler.post_tool_use_payload( + &ToolInvocation { + session: session.into(), + turn: turn.into(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-44".to_string(), + tool_name: codex_tools::ToolName::plain("exec_command"), + payload, + }, + &output, + ), None ); } -#[test] -fn exec_command_post_tool_use_payload_skips_running_sessions() { +#[tokio::test] +async fn exec_command_post_tool_use_payload_skips_running_sessions() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "cmd": "echo three", "tty": false }).to_string(), }; @@ -326,10 +346,20 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { "echo three".to_string(), ]), }; - let tool_name = codex_tools::ToolName::plain("exec_command"); + let (session, turn) = make_session_and_context().await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-45", &tool_name, &payload, &output), + UnifiedExecHandler.post_tool_use_payload( + &ToolInvocation { + session: session.into(), + turn: turn.into(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-45".to_string(), + tool_name: codex_tools::ToolName::plain("exec_command"), + payload, + }, + &output, + ), None ); } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index f023e5f00a52..b365687a14e6 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -70,9 +70,7 @@ pub trait ToolHandler: Send + Sync { fn post_tool_use_payload( &self, - _call_id: &str, - _tool_name: &ToolName, - _payload: &ToolPayload, + _invocation: &ToolInvocation, _result: &dyn ToolOutput, ) -> Option { None @@ -165,9 +163,7 @@ trait AnyToolHandler: Send + Sync { fn post_tool_use_payload( &self, - call_id: &str, - tool_name: &ToolName, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &dyn ToolOutput, ) -> Option; @@ -197,12 +193,10 @@ where fn post_tool_use_payload( &self, - call_id: &str, - tool_name: &ToolName, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &dyn ToolOutput, ) -> Option { - ToolHandler::post_tool_use_payload(self, call_id, tool_name, payload, result) + ToolHandler::post_tool_use_payload(self, invocation, result) } fn create_diff_consumer(&self) -> Option> { @@ -357,7 +351,7 @@ impl ToolRegistry { .await { // Bash hook payloads expose the executable text as tool_input.command. - let message = if pre_tool_use_payload.tool_name == "Bash" + let message = if pre_tool_use_payload.tool_name.name() == "Bash" && let Some(command) = pre_tool_use_payload .tool_input .get("command") @@ -418,12 +412,7 @@ impl ToolRegistry { let post_tool_use_payload = if success { let guard = response_cell.lock().await; guard.as_ref().and_then(|result| { - handler.post_tool_use_payload( - &result.call_id, - &tool_name, - &result.payload, - result.result.as_ref(), - ) + handler.post_tool_use_payload(&invocation, result.result.as_ref()) }) } else { None From 3c1825b5e5b303ce9add87ea7aeb8b80d0639359 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 13:44:21 -0700 Subject: [PATCH 07/22] Preserve Bash permission hook payload shape --- codex-rs/core/src/tools/network_approval.rs | 9 +----- codex-rs/core/src/tools/runtimes/shell.rs | 12 +++----- .../tools/runtimes/shell/unix_escalation.rs | 12 +++----- .../core/src/tools/runtimes/unified_exec.rs | 12 +++----- codex-rs/core/src/tools/sandboxing.rs | 18 +++++++++++ codex-rs/core/src/tools/sandboxing_tests.rs | 30 +++++++++++++++++++ 6 files changed, 61 insertions(+), 32 deletions(-) diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index cf7b0b5a23b0..ac8fdd03c244 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -7,7 +7,6 @@ use crate::guardian::routes_approval_to_guardian; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::denied_network_policy_message; use crate::session::session::Session; -use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; use codex_hooks::PermissionRequestDecision; @@ -383,13 +382,7 @@ impl NetworkApprovalService { &session, &turn_context, &guardian_approval_id, - PermissionRequestPayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ - "command": command, - "description": format!("network-access {target}"), - }), - }, + PermissionRequestPayload::bash(command, Some(format!("network-access {target}"))), ) .await { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 5a2183a17c58..e88a44a507e3 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -16,7 +16,6 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::shell::ShellType; -use crate::tools::hook_names::HookToolName; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_sandbox_command; @@ -201,13 +200,10 @@ impl Approvable for ShellRuntime { } fn permission_request_payload(&self, req: &ShellRequest) -> Option { - Some(PermissionRequestPayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ - "command": req.hook_command, - "description": req.justification, - }), - }) + Some(PermissionRequestPayload::bash( + req.hook_command.clone(), + req.justification.clone(), + )) } fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride { 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 9f746cd2326c..c04fa33c9443 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -13,7 +13,6 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecRequest; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; -use crate::tools::hook_names::HookToolName; use crate::tools::runtimes::build_sandbox_command; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; @@ -402,13 +401,10 @@ impl CoreShellActionProvider { Ok(stopwatch .pause_for(async move { // 1) Run PermissionRequest hooks - let permission_request = PermissionRequestPayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ - "command": codex_shell_command::parse_command::shlex_join(&command), - "description": null, - }), - }; + let permission_request = PermissionRequestPayload::bash( + codex_shell_command::parse_command::shlex_join(&command), + None, + ); let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); match run_permission_request_hooks( &session, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 41e31903aa4a..947a910bfc61 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -13,7 +13,6 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecServerEnvConfig; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; -use crate::tools::hook_names::HookToolName; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; use crate::tools::runtimes::build_sandbox_command; @@ -186,13 +185,10 @@ impl Approvable for UnifiedExecRuntime<'_> { &self, req: &UnifiedExecRequest, ) -> Option { - Some(PermissionRequestPayload { - tool_name: HookToolName::bash(), - tool_input: serde_json::json!({ - "command": req.hook_command, - "description": req.justification, - }), - }) + Some(PermissionRequestPayload::bash( + req.hook_command.clone(), + req.justification.clone(), + )) } fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride { diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 161625b083c0..922ce6a1d765 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -138,6 +138,24 @@ pub(crate) struct PermissionRequestPayload { pub tool_input: serde_json::Value, } +impl PermissionRequestPayload { + pub(crate) fn bash(command: String, description: Option) -> Self { + let mut tool_input = serde_json::Map::new(); + tool_input.insert("command".to_string(), serde_json::Value::String(command)); + if let Some(description) = description { + tool_input.insert( + "description".to_string(), + serde_json::Value::String(description), + ); + } + + Self { + tool_name: HookToolName::bash(), + tool_input: serde_json::Value::Object(tool_input), + } + } +} + // Specifies what tool orchestrator should do with a given tool call. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum ExecApprovalRequirement { diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 4a4dac3e8144..3c43b2fc455f 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -1,8 +1,38 @@ use super::*; use crate::sandboxing::SandboxPermissions; +use crate::tools::hook_names::HookToolName; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::NetworkAccess; use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn bash_permission_request_payload_omits_missing_description() { + assert_eq!( + PermissionRequestPayload::bash("echo hi".to_string(), None), + PermissionRequestPayload { + tool_name: HookToolName::bash(), + tool_input: json!({ "command": "echo hi" }), + } + ); +} + +#[test] +fn bash_permission_request_payload_includes_description_when_present() { + assert_eq!( + PermissionRequestPayload::bash( + "echo hi".to_string(), + Some("network-access example.com".to_string()), + ), + PermissionRequestPayload { + tool_name: HookToolName::bash(), + tool_input: json!({ + "command": "echo hi", + "description": "network-access example.com", + }), + } + ); +} #[test] fn external_sandbox_skips_exec_approval_on_request() { From a4c2437668fcc23f814b4fddcfa4f37cf0d3bfd8 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 13:59:36 -0700 Subject: [PATCH 08/22] codex: fix CI failure on PR #18385 --- codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs | 2 +- codex-rs/core/src/tools/sandboxing_tests.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 c04fa33c9443..83075c3674e7 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -403,7 +403,7 @@ impl CoreShellActionProvider { // 1) Run PermissionRequest hooks let permission_request = PermissionRequestPayload::bash( codex_shell_command::parse_command::shlex_join(&command), - None, + /*description*/ None, ); let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); match run_permission_request_hooks( diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 3c43b2fc455f..19eb7a67d967 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -9,7 +9,7 @@ use serde_json::json; #[test] fn bash_permission_request_payload_omits_missing_description() { assert_eq!( - PermissionRequestPayload::bash("echo hi".to_string(), None), + PermissionRequestPayload::bash("echo hi".to_string(), /*description*/ None), PermissionRequestPayload { tool_name: HookToolName::bash(), tool_input: json!({ "command": "echo hi" }), From c94ce27e3625626016e0c9cc37f3685a24030550 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 19:13:24 -0700 Subject: [PATCH 09/22] Add MCP hook integration tests --- codex-rs/core/tests/suite/hooks_mcp.rs | 346 +++++++++++++++++++++++++ codex-rs/core/tests/suite/mod.rs | 2 + 2 files changed, 348 insertions(+) create mode 100644 codex-rs/core/tests/suite/hooks_mcp.rs diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs new file mode 100644 index 000000000000..409594bbd644 --- /dev/null +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -0,0 +1,346 @@ +use std::collections::HashMap; +use std::fs; +use std::path::Path; +use std::time::Duration; + +use anyhow::Context; +use anyhow::Result; +use codex_config::types::AppToolApproval; +use codex_config::types::McpServerConfig; +use codex_config::types::McpServerTransportConfig; +use codex_core::config::Config; +use codex_features::Feature; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call_with_namespace; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_once; +use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::stdio_server_bin; +use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; + +const RMCP_SERVER: &str = "rmcp"; +const RMCP_NAMESPACE: &str = "mcp__rmcp__"; +const RMCP_ECHO_TOOL_NAME: &str = "mcp__rmcp__echo"; +const RMCP_HOOK_MATCHER: &str = "mcp__rmcp__.*"; +const RMCP_ECHO_MESSAGE: &str = "hook e2e ping"; + +fn write_pre_tool_use_hook(home: &Path, reason: &str) -> Result<()> { + let script_path = home.join("pre_tool_use_hook.py"); + let log_path = home.join("pre_tool_use_hook_log.jsonl"); + let reason_json = serde_json::to_string(reason).context("serialize pre tool use reason")?; + 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": "deny", + "permissionDecisionReason": {reason_json} + }} +}})) +"#, + log_path = log_path.display(), + reason_json = reason_json, + ); + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "matcher": RMCP_HOOK_MATCHER, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running MCP pre tool use hook", + }] + }] + } + }); + + fs::write(&script_path, script).context("write 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"); + let additional_context_json = + serde_json::to_string(additional_context).context("serialize post tool use context")?; + 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": "PostToolUse", + "additionalContext": {additional_context_json} + }} +}})) +"#, + log_path = log_path.display(), + additional_context_json = additional_context_json, + ); + let hooks = serde_json::json!({ + "hooks": { + "PostToolUse": [{ + "matcher": RMCP_HOOK_MATCHER, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running MCP post tool use hook", + }] + }] + } + }); + + fs::write(&script_path, script).context("write post tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + +fn read_hook_inputs(home: &Path, log_name: &str) -> Result> { + fs::read_to_string(home.join(log_name)) + .with_context(|| format!("read {log_name}"))? + .lines() + .filter(|line| !line.trim().is_empty()) + .map(|line| serde_json::from_str(line).with_context(|| format!("parse {log_name} line"))) + .collect() +} + +fn insert_rmcp_test_server(config: &mut Config, command: String) { + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + RMCP_SERVER.to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + default_tools_approval_mode: Some(AppToolApproval::Approve), + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth_resource: None, + tools: HashMap::new(), + }, + ); + if let Err(err) = config.mcp_servers.set(servers) { + panic!("test mcp servers should accept any configuration: {err}"); + } +} + +fn enable_hooks_and_rmcp_server(config: &mut Config, rmcp_test_server_bin: String) { + if let Err(err) = config.features.enable(Feature::CodexHooks) { + panic!("test config should allow feature update: {err}"); + } + insert_rmcp_test_server(config, rmcp_test_server_bin); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-rmcp-echo"; + let arguments = json!({ "message": RMCP_ECHO_MESSAGE }).to_string(); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call_with_namespace(call_id, RMCP_NAMESPACE, "echo", &arguments), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "mcp hook blocked it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let block_reason = "blocked mcp pre hook"; + let rmcp_test_server_bin = stdio_server_bin()?; + let test = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_pre_tool_use_hook(home, block_reason) { + panic!("failed to write MCP pre tool use hook fixture: {error}"); + } + }) + .with_config(move |config| { + enable_hooks_and_rmcp_server(config, rmcp_test_server_bin); + }) + .build(&server) + .await?; + + test.submit_turn("call the rmcp echo tool with the MCP pre hook") + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("blocked MCP tool output string"); + assert!( + output.contains(&format!( + "Tool call blocked by PreToolUse hook: {block_reason}. Tool: {RMCP_ECHO_TOOL_NAME}" + )), + "blocked MCP tool output should surface the hook reason and tool name", + ); + + let hook_inputs = read_hook_inputs(test.codex_home_path(), "pre_tool_use_hook_log.jsonl")?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + json!({ + "hook_event_name": hook_inputs[0]["hook_event_name"], + "tool_name": hook_inputs[0]["tool_name"], + "tool_use_id": hook_inputs[0]["tool_use_id"], + "tool_input": hook_inputs[0]["tool_input"], + }), + json!({ + "hook_event_name": "PreToolUse", + "tool_name": RMCP_ECHO_TOOL_NAME, + "tool_use_id": call_id, + "tool_input": { "message": RMCP_ECHO_MESSAGE }, + }) + ); + let transcript_path = hook_inputs[0]["transcript_path"] + .as_str() + .expect("pre tool use hook transcript_path"); + assert!( + Path::new(transcript_path).exists(), + "pre tool use hook transcript_path should be materialized on disk", + ); + + 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(())); + + let server = start_mock_server().await; + let call_id = "posttooluse-rmcp-echo"; + 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 post hook context observed"), + ev_completed("resp-2"), + ]), + ) + .await; + + let post_context = "Remember the MCP post-tool note."; + let rmcp_test_server_bin = stdio_server_bin()?; + let test = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_post_tool_use_hook(home, post_context) { + panic!("failed to write MCP post tool use hook fixture: {error}"); + } + }) + .with_config(move |config| { + enable_hooks_and_rmcp_server(config, rmcp_test_server_bin); + }) + .build(&server) + .await?; + + test.submit_turn("call the rmcp echo tool with the MCP post hook") + .await?; + + let final_request = final_mock.single_request(); + assert!( + final_request + .message_input_texts("developer") + .contains(&post_context.to_string()), + "follow-up request should include MCP post tool use additional context", + ); + 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: {RMCP_ECHO_MESSAGE}")), + "MCP tool output should still reach the model", + ); + + let hook_inputs = read_hook_inputs(test.codex_home_path(), "post_tool_use_hook_log.jsonl")?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + json!({ + "hook_event_name": hook_inputs[0]["hook_event_name"], + "tool_name": hook_inputs[0]["tool_name"], + "tool_use_id": hook_inputs[0]["tool_use_id"], + "tool_input": hook_inputs[0]["tool_input"], + "tool_response": hook_inputs[0]["tool_response"], + }), + json!({ + "hook_event_name": "PostToolUse", + "tool_name": RMCP_ECHO_TOOL_NAME, + "tool_use_id": call_id, + "tool_input": { "message": RMCP_ECHO_MESSAGE }, + "tool_response": { + "content": [], + "structuredContent": { + "echo": format!("ECHOING: {RMCP_ECHO_MESSAGE}"), + "env": null, + }, + "isError": false, + }, + }) + ); + let transcript_path = hook_inputs[0]["transcript_path"] + .as_str() + .expect("post tool use hook transcript_path"); + assert!( + Path::new(transcript_path).exists(), + "post tool use hook transcript_path should be materialized on disk", + ); + + call_mock.single_request(); + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 6c4cb1981f91..8f9f9e68406f 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -47,6 +47,8 @@ mod fork_thread; mod hierarchical_agents; #[cfg(not(target_os = "windows"))] mod hooks; +#[cfg(not(target_os = "windows"))] +mod hooks_mcp; mod image_rollout; mod items; mod js_repl; From 7e61709b6366af2bcb9eae42fc464d53671fc45b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 17 Apr 2026 19:16:35 -0700 Subject: [PATCH 10/22] Add MCP permission hook integration test --- codex-rs/core/tests/suite/hooks_mcp.rs | 138 +++++++++++++++++++++++-- 1 file changed, 132 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 409594bbd644..c9a43a7ade87 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -10,6 +10,8 @@ use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; use codex_core::config::Config; use codex_features::Feature; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::SandboxPolicy; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call_with_namespace; @@ -117,6 +119,46 @@ print(json.dumps({{ Ok(()) } +fn write_permission_request_hook(home: &Path) -> Result<()> { + let script_path = home.join("permission_request_hook.py"); + let log_path = home.join("permission_request_hook_log.jsonl"); + 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"}} + }} +}})) +"#, + log_path = log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "PermissionRequest": [{ + "matcher": RMCP_HOOK_MATCHER, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running MCP permission request hook", + }] + }] + } + }); + + fs::write(&script_path, script).context("write permission request hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn read_hook_inputs(home: &Path, log_name: &str) -> Result> { fs::read_to_string(home.join(log_name)) .with_context(|| format!("read {log_name}"))? @@ -126,7 +168,7 @@ fn read_hook_inputs(home: &Path, log_name: &str) -> Result> { .collect() } -fn insert_rmcp_test_server(config: &mut Config, command: String) { +fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: AppToolApproval) { let mut servers = config.mcp_servers.get().clone(); servers.insert( RMCP_SERVER.to_string(), @@ -145,7 +187,7 @@ fn insert_rmcp_test_server(config: &mut Config, command: String) { disabled_reason: None, startup_timeout_sec: Some(Duration::from_secs(10)), tool_timeout_sec: None, - default_tools_approval_mode: Some(AppToolApproval::Approve), + default_tools_approval_mode: Some(approval_mode), enabled_tools: None, disabled_tools: None, scopes: None, @@ -158,11 +200,15 @@ fn insert_rmcp_test_server(config: &mut Config, command: String) { } } -fn enable_hooks_and_rmcp_server(config: &mut Config, rmcp_test_server_bin: String) { +fn enable_hooks_and_rmcp_server( + config: &mut Config, + rmcp_test_server_bin: String, + approval_mode: AppToolApproval, +) { if let Err(err) = config.features.enable(Feature::CodexHooks) { panic!("test config should allow feature update: {err}"); } - insert_rmcp_test_server(config, rmcp_test_server_bin); + insert_rmcp_test_server(config, rmcp_test_server_bin, approval_mode); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -198,7 +244,7 @@ async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { } }) .with_config(move |config| { - enable_hooks_and_rmcp_server(config, rmcp_test_server_bin); + enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Approve); }) .build(&server) .await?; @@ -282,7 +328,7 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { } }) .with_config(move |config| { - enable_hooks_and_rmcp_server(config, rmcp_test_server_bin); + enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Approve); }) .build(&server) .await?; @@ -344,3 +390,83 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_allows_mcp_tool_call() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "permissionrequest-rmcp-echo"; + 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 permission hook allowed 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_permission_request_hook(home) { + panic!("failed to write MCP permission request hook fixture: {error}"); + } + }) + .with_config(move |config| { + enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Prompt); + }) + .build(&server) + .await?; + + test.submit_turn_with_policies( + "call the rmcp echo tool with the MCP permission hook", + AskForApproval::OnRequest, + SandboxPolicy::new_read_only_policy(), + ) + .await?; + + let hook_inputs = + read_hook_inputs(test.codex_home_path(), "permission_request_hook_log.jsonl")?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + json!({ + "hook_event_name": hook_inputs[0]["hook_event_name"], + "tool_name": hook_inputs[0]["tool_name"], + "tool_use_id": hook_inputs[0]["tool_use_id"], + "tool_input": hook_inputs[0]["tool_input"], + }), + json!({ + "hook_event_name": "PermissionRequest", + "tool_name": RMCP_ECHO_TOOL_NAME, + "tool_use_id": call_id, + "tool_input": { "message": RMCP_ECHO_MESSAGE }, + }) + ); + + 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: {RMCP_ECHO_MESSAGE}")), + "MCP tool output should reach the model after PermissionRequest allows it", + ); + + call_mock.single_request(); + + Ok(()) +} From e19fceba53f606abc1ea924023ce3f4f3d579b2b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 21 Apr 2026 16:11:15 -0700 Subject: [PATCH 11/22] Remove duplicate MCP permission hook suite test --- codex-rs/core/tests/suite/hooks_mcp.rs | 122 ------------------------- 1 file changed, 122 deletions(-) diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index c9a43a7ade87..2157630e02b0 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -10,8 +10,6 @@ use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; use codex_core::config::Config; use codex_features::Feature; -use codex_protocol::protocol::AskForApproval; -use codex_protocol::protocol::SandboxPolicy; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call_with_namespace; @@ -119,46 +117,6 @@ print(json.dumps({{ Ok(()) } -fn write_permission_request_hook(home: &Path) -> Result<()> { - let script_path = home.join("permission_request_hook.py"); - let log_path = home.join("permission_request_hook_log.jsonl"); - 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"}} - }} -}})) -"#, - log_path = log_path.display(), - ); - let hooks = serde_json::json!({ - "hooks": { - "PermissionRequest": [{ - "matcher": RMCP_HOOK_MATCHER, - "hooks": [{ - "type": "command", - "command": format!("python3 {}", script_path.display()), - "statusMessage": "running MCP permission request hook", - }] - }] - } - }); - - fs::write(&script_path, script).context("write permission request hook script")?; - fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; - Ok(()) -} - fn read_hook_inputs(home: &Path, log_name: &str) -> Result> { fs::read_to_string(home.join(log_name)) .with_context(|| format!("read {log_name}"))? @@ -390,83 +348,3 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { Ok(()) } - -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn permission_request_hook_allows_mcp_tool_call() -> Result<()> { - skip_if_no_network!(Ok(())); - - let server = start_mock_server().await; - let call_id = "permissionrequest-rmcp-echo"; - 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 permission hook allowed 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_permission_request_hook(home) { - panic!("failed to write MCP permission request hook fixture: {error}"); - } - }) - .with_config(move |config| { - enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Prompt); - }) - .build(&server) - .await?; - - test.submit_turn_with_policies( - "call the rmcp echo tool with the MCP permission hook", - AskForApproval::OnRequest, - SandboxPolicy::new_read_only_policy(), - ) - .await?; - - let hook_inputs = - read_hook_inputs(test.codex_home_path(), "permission_request_hook_log.jsonl")?; - assert_eq!(hook_inputs.len(), 1); - assert_eq!( - json!({ - "hook_event_name": hook_inputs[0]["hook_event_name"], - "tool_name": hook_inputs[0]["tool_name"], - "tool_use_id": hook_inputs[0]["tool_use_id"], - "tool_input": hook_inputs[0]["tool_input"], - }), - json!({ - "hook_event_name": "PermissionRequest", - "tool_name": RMCP_ECHO_TOOL_NAME, - "tool_use_id": call_id, - "tool_input": { "message": RMCP_ECHO_MESSAGE }, - }) - ); - - 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: {RMCP_ECHO_MESSAGE}")), - "MCP tool output should reach the model after PermissionRequest allows it", - ); - - call_mock.single_request(); - - Ok(()) -} From d2d08bd405edc3f3ca2604f2ff80e436753929e9 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 21 Apr 2026 17:28:28 -0700 Subject: [PATCH 12/22] Fix MCP hook payloads --- codex-rs/core/src/mcp_tool_call.rs | 205 +++++++++++------- codex-rs/core/src/tools/context.rs | 1 + codex-rs/core/src/tools/context_tests.rs | 3 + codex-rs/core/src/tools/handlers/mcp.rs | 24 +- codex-rs/core/src/tools/handlers/shell.rs | 4 +- .../core/src/tools/handlers/unified_exec.rs | 2 +- codex-rs/core/src/tools/parallel.rs | 1 + codex-rs/core/src/tools/registry.rs | 28 +-- codex-rs/core/tests/suite/openai_file_mcp.rs | 80 ++++++- 9 files changed, 238 insertions(+), 110 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e588b95fd2e3..5df1abef4dd2 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -62,6 +62,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use rmcp::model::ToolAnnotations; use serde::Deserialize; use serde::Serialize; +use serde_json::Value as JsonValue; use std::sync::Arc; use toml_edit::value; use tracing::Instrument; @@ -81,7 +82,7 @@ pub(crate) async fn handle_mcp_tool_call( server: String, tool_name: String, arguments: String, -) -> CallToolResult { +) -> HandledMcpToolCall { // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON // is not. let arguments_value = if arguments.trim().is_empty() { @@ -91,7 +92,10 @@ pub(crate) async fn handle_mcp_tool_call( Ok(value) => Some(value), Err(e) => { error!("failed to parse tool call arguments: {e}"); - return CallToolResult::from_error_text(format!("err: {e}")); + return HandledMcpToolCall { + result: CallToolResult::from_error_text(format!("err: {e}")), + tool_input: JsonValue::Object(serde_json::Map::new()), + }; } } }; @@ -147,7 +151,11 @@ pub(crate) async fn handle_mcp_tool_call( /*inc*/ 1, &[("status", status)], ); - return CallToolResult::from_result(result); + return HandledMcpToolCall { + result: CallToolResult::from_result(result), + tool_input: arguments_value + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), + }; } let request_meta = build_mcp_tool_call_request_meta(turn_context.as_ref(), &server, metadata.as_ref()); @@ -182,7 +190,7 @@ pub(crate) async fn handle_mcp_tool_call( ) .await { - let (result, call_duration) = match decision { + let result = match decision { McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember => { @@ -223,7 +231,10 @@ pub(crate) async fn handle_mcp_tool_call( invocation, mcp_app_resource_uri: mcp_app_resource_uri.clone(), duration, - result: result.clone(), + result: result + .as_ref() + .map(|result| result.result.clone()) + .map_err(Clone::clone), }); notify_mcp_tool_call_event( sess.as_ref(), @@ -238,54 +249,55 @@ pub(crate) async fn handle_mcp_tool_call( &tool_name, ) .await; - (result, Some(duration)) + let status = if result.is_ok() { "ok" } else { "error" }; + emit_mcp_call_metrics( + turn_context.as_ref(), + status, + &tool_name, + connector_id.as_deref(), + connector_name.as_deref(), + Some(duration), + ); + + return build_handled_mcp_tool_call(result, arguments_value); } McpToolApprovalDecision::Decline { message } => { let message = message.unwrap_or_else(|| "user rejected MCP tool call".to_string()); - ( - notify_mcp_tool_call_skip( - sess.as_ref(), - turn_context.as_ref(), - &call_id, - invocation, - mcp_app_resource_uri.clone(), - message, - /*already_started*/ true, - ) - .await, - None, + notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + mcp_app_resource_uri.clone(), + message, + /*already_started*/ true, ) + .await } McpToolApprovalDecision::Cancel => { let message = "user cancelled MCP tool call".to_string(); - ( - notify_mcp_tool_call_skip( - sess.as_ref(), - turn_context.as_ref(), - &call_id, - invocation, - mcp_app_resource_uri.clone(), - message, - /*already_started*/ true, - ) - .await, - None, + notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + mcp_app_resource_uri.clone(), + message, + /*already_started*/ true, ) + .await } McpToolApprovalDecision::BlockedBySafetyMonitor(message) => { - ( - notify_mcp_tool_call_skip( - sess.as_ref(), - turn_context.as_ref(), - &call_id, - invocation, - mcp_app_resource_uri.clone(), - message, - /*already_started*/ true, - ) - .await, - None, + notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + mcp_app_resource_uri.clone(), + message, + /*already_started*/ true, ) + .await } }; @@ -296,10 +308,14 @@ pub(crate) async fn handle_mcp_tool_call( &tool_name, connector_id.as_deref(), connector_name.as_deref(), - call_duration, + None, ); - return CallToolResult::from_result(result); + return HandledMcpToolCall { + result: CallToolResult::from_result(result), + tool_input: arguments_value + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), + }; } maybe_mark_thread_memory_mode_polluted(sess.as_ref(), turn_context.as_ref()).await; @@ -339,7 +355,10 @@ pub(crate) async fn handle_mcp_tool_call( invocation, mcp_app_resource_uri, duration, - result: result.clone(), + result: result + .as_ref() + .map(|result| result.result.clone()) + .map_err(Clone::clone), }); notify_mcp_tool_call_event( @@ -360,7 +379,33 @@ pub(crate) async fn handle_mcp_tool_call( Some(duration), ); - CallToolResult::from_result(result) + build_handled_mcp_tool_call(result, arguments_value) +} + +pub(crate) struct HandledMcpToolCall { + pub(crate) result: CallToolResult, + pub(crate) tool_input: JsonValue, +} + +struct ExecutedMcpToolCall { + result: CallToolResult, + arguments: Option, +} + +fn build_handled_mcp_tool_call( + result: Result, + fallback_arguments: Option, +) -> HandledMcpToolCall { + let tool_input = result + .as_ref() + .ok() + .and_then(|result| result.arguments.clone()) + .or(fallback_arguments) + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())); + HandledMcpToolCall { + result: CallToolResult::from_result(result.map(|result| result.result)), + tool_input, + } } fn emit_mcp_call_metrics( @@ -469,10 +514,10 @@ async fn execute_mcp_tool_call( turn_context: &TurnContext, server: &str, tool_name: &str, - arguments_value: Option, + arguments_value: Option, metadata: Option<&McpToolApprovalMetadata>, - request_meta: Option, -) -> Result { + request_meta: Option, +) -> Result { let rewritten_arguments = rewrite_mcp_tool_arguments_for_openai_files( sess, turn_context, @@ -480,6 +525,7 @@ async fn execute_mcp_tool_call( metadata.and_then(|metadata| metadata.openai_file_input_params.as_deref()), ) .await?; + let arguments = rewritten_arguments.clone(); let request_meta = with_mcp_tool_call_thread_id_meta(request_meta, &sess.conversation_id.to_string()); let request_meta = @@ -490,13 +536,14 @@ async fn execute_mcp_tool_call( .call_tool(server, tool_name, rewritten_arguments, request_meta) .await .map_err(|e| format!("tool call error: {e:?}"))?; - sanitize_mcp_tool_result_for_model( + let result = sanitize_mcp_tool_result_for_model( turn_context .model_info .input_modalities .contains(&InputModality::Image), Ok(result), - ) + )?; + Ok(ExecutedMcpToolCall { result, arguments }) } #[expect( @@ -868,32 +915,31 @@ async fn maybe_request_mcp_tool_approval( return Some(McpToolApprovalDecision::Accept); } - let tool_name = metadata - .map(|metadata| metadata.model_tool_name.clone()) - .unwrap_or_else(|| format!("mcp__{}__{}", invocation.server, invocation.tool)); - match run_permission_request_hooks( - sess, - turn_context, - call_id, - PermissionRequestPayload { - tool_name, - tool_input: invocation - .arguments - .clone() - .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), - }, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return Some(McpToolApprovalDecision::Accept); - } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); + if let Some(metadata) = metadata { + match run_permission_request_hooks( + sess, + turn_context, + call_id, + PermissionRequestPayload { + tool_name: metadata.model_tool_name.clone(), + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), + }, + ) + .await + { + Some(PermissionRequestDecision::Allow) => { + return Some(McpToolApprovalDecision::Accept); + } + Some(PermissionRequestDecision::Deny { message }) => { + return Some(McpToolApprovalDecision::Decline { + message: Some(message), + }); + } + None => {} } - None => {} } let tool_call_mcp_elicitation_enabled = turn_context @@ -1175,6 +1221,8 @@ pub(crate) async fn lookup_mcp_tool_metadata( None }; + let model_tool_name = tool_info.canonical_tool_name().display(); + Some(McpToolApprovalMetadata { annotations: tool_info.tool.annotations, connector_id: tool_info.connector_id, @@ -1182,10 +1230,7 @@ pub(crate) async fn lookup_mcp_tool_metadata( connector_description, tool_title: tool_info.tool.title, tool_description: tool_info.tool.description.map(std::borrow::Cow::into_owned), - model_tool_name: format!( - "{}{}", - tool_info.callable_namespace, tool_info.callable_name - ), + model_tool_name, mcp_app_resource_uri: get_mcp_app_resource_uri(tool_info.tool.meta.as_deref()), codex_apps_meta: tool_info .tool diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index 9e703768b736..13eba40bfc41 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -132,6 +132,7 @@ impl ToolOutput for CallToolResult { #[derive(Clone, Debug)] pub struct McpToolOutput { pub result: CallToolResult, + pub tool_input: JsonValue, pub wall_time: Duration, pub original_image_detail_supported: bool, } diff --git a/codex-rs/core/src/tools/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index 4b903347aba4..0d4760f0726a 100644 --- a/codex-rs/core/src/tools/context_tests.rs +++ b/codex-rs/core/src/tools/context_tests.rs @@ -98,6 +98,7 @@ fn mcp_tool_output_response_item_includes_wall_time() { is_error: Some(false), meta: None, }, + tool_input: json!({}), wall_time: std::time::Duration::from_millis(1250), original_image_detail_supported: false, }; @@ -150,6 +151,7 @@ fn mcp_tool_output_response_item_preserves_content_items() { is_error: Some(false), meta: None, }, + tool_input: json!({}), wall_time: std::time::Duration::from_millis(500), original_image_detail_supported: false, }; @@ -203,6 +205,7 @@ fn mcp_tool_output_code_mode_result_stays_raw_call_tool_result() { is_error: Some(false), meta: None, }, + tool_input: json!({}), wall_time: std::time::Duration::from_millis(1250), original_image_detail_supported: false, }; diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 7a3675d1c471..424da160e373 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -37,9 +37,9 @@ impl ToolHandler for McpHandler { fn post_tool_use_payload( &self, invocation: &ToolInvocation, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { - let ToolPayload::Mcp { raw_arguments, .. } = &invocation.payload else { + let ToolPayload::Mcp { .. } = &invocation.payload else { return None; }; @@ -47,7 +47,7 @@ impl ToolHandler for McpHandler { result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::new(invocation.tool_name.display()), - tool_input: mcp_hook_tool_input(raw_arguments), + tool_input: result.tool_input.clone(), tool_response, }) } @@ -89,7 +89,8 @@ impl ToolHandler for McpHandler { .await; Ok(McpToolOutput { - result, + result: result.result, + tool_input: result.tool_input, wall_time: started.elapsed(), original_image_detail_supported: can_request_original_image_detail(&turn.model_info), }) @@ -139,7 +140,7 @@ mod tests { payload, }), Some(PreToolUsePayload { - tool_name: "mcp__memory__create_entities".to_string(), + tool_name: HookToolName::new("mcp__memory__create_entities"), tool_input: json!({ "entities": [{ "name": "Ada", @@ -167,6 +168,11 @@ mod tests { is_error: None, meta: None, }, + tool_input: json!({ + "path": { + "file_id": "file_123" + } + }), wall_time: Duration::from_millis(42), original_image_detail_supported: true, }; @@ -185,8 +191,12 @@ mod tests { &output, ), Some(PostToolUsePayload { - tool_name: "mcp__filesystem__read_file".to_string(), - tool_input: json!({ "path": "/tmp/notes.txt" }), + tool_name: HookToolName::new("mcp__filesystem__read_file"), + tool_input: json!({ + "path": { + "file_id": "file_123" + } + }), tool_response: json!({ "content": [{ "type": "text", diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index f57cf6e28cb8..73477ed2547d 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -215,7 +215,7 @@ impl ToolHandler for ShellHandler { fn post_tool_use_payload( &self, invocation: &ToolInvocation, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; @@ -328,7 +328,7 @@ impl ToolHandler for ShellCommandHandler { fn post_tool_use_payload( &self, invocation: &ToolInvocation, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 974a1cd618a3..1224f4701a43 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -146,7 +146,7 @@ impl ToolHandler for UnifiedExecHandler { fn post_tool_use_payload( &self, invocation: &ToolInvocation, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let ToolPayload::Function { arguments } = &invocation.payload else { return None; diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index 06cf7e5eed0e..384a800b4b11 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -178,6 +178,7 @@ impl ToolCallRuntime { result: Box::new(AbortedToolOutput { message: Self::abort_message(call, secs), }), + post_tool_use_payload: None, } } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index b365687a14e6..20b268b48f73 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -71,7 +71,7 @@ pub trait ToolHandler: Send + Sync { fn post_tool_use_payload( &self, _invocation: &ToolInvocation, - _result: &dyn ToolOutput, + _result: &Self::Output, ) -> Option { None } @@ -106,6 +106,7 @@ pub(crate) struct AnyToolResult { pub(crate) call_id: String, pub(crate) payload: ToolPayload, pub(crate) result: Box, + pub(crate) post_tool_use_payload: Option, } impl AnyToolResult { @@ -161,12 +162,6 @@ trait AnyToolHandler: Send + Sync { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; - fn post_tool_use_payload( - &self, - invocation: &ToolInvocation, - result: &dyn ToolOutput, - ) -> Option; - fn create_diff_consumer(&self) -> Option>; fn handle_any<'a>( @@ -191,14 +186,6 @@ where ToolHandler::pre_tool_use_payload(self, invocation) } - fn post_tool_use_payload( - &self, - invocation: &ToolInvocation, - result: &dyn ToolOutput, - ) -> Option { - ToolHandler::post_tool_use_payload(self, invocation, result) - } - fn create_diff_consumer(&self) -> Option> { ToolHandler::create_diff_consumer(self) } @@ -210,11 +197,14 @@ where Box::pin(async move { let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); - let output = self.handle(invocation).await?; + let output = self.handle(invocation.clone()).await?; + let post_tool_use_payload = + ToolHandler::post_tool_use_payload(self, &invocation, &output); Ok(AnyToolResult { call_id, payload, result: Box::new(output), + post_tool_use_payload, }) }) } @@ -411,9 +401,9 @@ impl ToolRegistry { emit_metric_for_tool_read(&invocation, success).await; let post_tool_use_payload = if success { let guard = response_cell.lock().await; - guard.as_ref().and_then(|result| { - handler.post_tool_use_payload(&invocation, result.result.as_ref()) - }) + guard + .as_ref() + .and_then(|result| result.post_tool_use_payload.clone()) } else { None }; diff --git a/codex-rs/core/tests/suite/openai_file_mcp.rs b/codex-rs/core/tests/suite/openai_file_mcp.rs index 211aa6738141..2912b5fb0e21 100644 --- a/codex-rs/core/tests/suite/openai_file_mcp.rs +++ b/codex-rs/core/tests/suite/openai_file_mcp.rs @@ -1,5 +1,9 @@ #![cfg(not(target_os = "windows"))] +use std::fs; +use std::path::Path; + +use anyhow::Context; use anyhow::Result; use codex_core::config::Config; use codex_features::Feature; @@ -28,6 +32,7 @@ use wiremock::matchers::path; const DOCUMENT_EXTRACT_NAMESPACE: &str = "mcp__codex_apps__calendar"; const DOCUMENT_EXTRACT_TOOL: &str = "_extract_text"; +const DOCUMENT_EXTRACT_HOOK_MATCHER: &str = "mcp__codex_apps__calendar_extract_text"; fn configure_apps(config: &mut Config, chatgpt_base_url: &str) { if let Err(err) = config.features.enable(Feature::Apps) { @@ -36,6 +41,55 @@ fn configure_apps(config: &mut Config, chatgpt_base_url: &str) { config.chatgpt_base_url = chatgpt_base_url.to_string(); } +fn write_post_tool_use_hook(home: &Path) -> Result<()> { + let script_path = home.join("post_tool_use_hook.py"); + let log_path = home.join("post_tool_use_hook_log.jsonl"); + 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": "PostToolUse", + "additionalContext": "observed apps file payload" + }} +}})) +"#, + log_path = log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "PostToolUse": [{ + "matcher": DOCUMENT_EXTRACT_HOOK_MATCHER, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running apps file post tool use hook", + }] + }] + } + }); + + fs::write(&script_path, script).context("write post tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + +fn read_post_tool_use_hook_inputs(home: &Path) -> Result> { + fs::read_to_string(home.join("post_tool_use_hook_log.jsonl")) + .context("read post tool use hook log")? + .lines() + .filter(|line| !line.trim().is_empty()) + .map(|line| serde_json::from_str(line).context("parse post tool use hook input")) + .collect() +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Result<()> { let server = start_mock_server().await; @@ -101,7 +155,17 @@ async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Res let mut builder = test_codex() .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) - .with_config(move |config| configure_apps(config, apps_server.chatgpt_base_url.as_str())); + .with_pre_build_hook(move |home| { + if let Err(error) = write_post_tool_use_hook(home) { + panic!("failed to write apps file post tool use hook fixture: {error}"); + } + }) + .with_config(move |config| { + configure_apps(config, apps_server.chatgpt_base_url.as_str()); + if let Err(err) = config.features.enable(Feature::CodexHooks) { + panic!("test config should allow feature update: {err}"); + } + }); let test = builder.build(&server).await?; tokio::fs::write(test.cwd.path().join("report.txt"), b"hello world").await?; @@ -164,6 +228,20 @@ async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Res })) ); + let hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + hook_inputs[0]["tool_input"]["file"], + json!({ + "download_url": format!("{}/download/file_123", server.uri()), + "file_id": "file_123", + "mime_type": "text/plain", + "file_name": "report.txt", + "uri": "sediment://file_123", + "file_size_bytes": 11, + }) + ); + server.verify().await; Ok(()) } From 114db76e80fef5c770b8482bcb00d911dc0c1cbd Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 21 Apr 2026 17:45:25 -0700 Subject: [PATCH 13/22] Move pre-tool hook block message formatting Keep ToolRegistry focused on dispatching and let the hook runtime return the final model-facing block message for Bash and MCP tool calls. Co-authored-by: Codex --- codex-rs/core/src/hook_runtime.rs | 29 ++++++++++++++++++++++------- codex-rs/core/src/tools/registry.rs | 21 +++------------------ 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index f2a8c80c177f..db47688685bc 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -35,6 +35,7 @@ use crate::context::HookAdditionalContext; use crate::event_mapping::parse_turn_item; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; pub(crate) struct HookRuntimeOutcome { @@ -137,9 +138,8 @@ pub(crate) async fn run_pre_tool_use_hooks( sess: &Arc, turn_context: &Arc, tool_use_id: String, - tool_name: String, - matcher_aliases: Vec, - tool_input: Value, + tool_name: &HookToolName, + tool_input: &Value, ) -> Option { let request = PreToolUseRequest { session_id: sess.conversation_id, @@ -148,10 +148,10 @@ pub(crate) async fn run_pre_tool_use_hooks( transcript_path: sess.hook_transcript_path().await, model: turn_context.model_info.slug.clone(), permission_mode: hook_permission_mode(turn_context), - tool_name, - matcher_aliases, + tool_name: tool_name.name().to_string(), + matcher_aliases: tool_name.matcher_aliases().to_vec(), tool_use_id, - tool_input, + tool_input: tool_input.clone(), }; let preview_runs = sess.hooks().preview_pre_tool_use(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; @@ -163,7 +163,22 @@ pub(crate) async fn run_pre_tool_use_hooks( } = sess.hooks().run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; - if should_block { block_reason } else { None } + 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() + ) + } + }) + } else { + None + } } // PermissionRequest hooks share the same preview/start/completed event flow as diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 20b268b48f73..1dc65b24f6af 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -330,30 +330,15 @@ impl ToolRegistry { } if let Some(pre_tool_use_payload) = handler.pre_tool_use_payload(&invocation) - && let Some(reason) = run_pre_tool_use_hooks( + && let Some(message) = run_pre_tool_use_hooks( &invocation.session, &invocation.turn, invocation.call_id.clone(), - pre_tool_use_payload.tool_name.name().to_string(), - pre_tool_use_payload.tool_name.matcher_aliases().to_vec(), - pre_tool_use_payload.tool_input.clone(), + &pre_tool_use_payload.tool_name, + &pre_tool_use_payload.tool_input, ) .await { - // Bash hook payloads expose the executable text as tool_input.command. - let message = if pre_tool_use_payload.tool_name.name() == "Bash" - && let Some(command) = pre_tool_use_payload - .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: {}", - pre_tool_use_payload.tool_name.name() - ) - }; return Err(FunctionCallError::RespondToModel(message)); } From 9d32374e07d96815788df9000e8d1a6dfef9a4ab Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 10:27:05 -0700 Subject: [PATCH 14/22] Fix hook payload rebase fallout Align apply_patch and MCP permission payloads with the rebased HookToolName plus generic tool_input model. Co-authored-by: Codex --- codex-rs/core/src/mcp_tool_call.rs | 5 +++-- codex-rs/core/src/tools/handlers/apply_patch.rs | 14 ++++++++------ .../core/src/tools/handlers/apply_patch_tests.rs | 13 +++++++------ codex-rs/core/src/tools/handlers/mcp.rs | 2 ++ codex-rs/core/src/tools/handlers/shell_tests.rs | 1 + .../core/src/tools/handlers/unified_exec_tests.rs | 3 +++ codex-rs/core/src/tools/runtimes/apply_patch.rs | 3 +-- .../core/src/tools/runtimes/apply_patch_tests.rs | 6 ++++-- 8 files changed, 29 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 5df1abef4dd2..f8150b35daeb 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -31,6 +31,7 @@ use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; @@ -308,7 +309,7 @@ pub(crate) async fn handle_mcp_tool_call( &tool_name, connector_id.as_deref(), connector_name.as_deref(), - None, + /*duration*/ None, ); return HandledMcpToolCall { @@ -921,7 +922,7 @@ async fn maybe_request_mcp_tool_approval( turn_context, call_id, PermissionRequestPayload { - tool_name: metadata.model_tool_name.clone(), + tool_name: HookToolName::new(metadata.model_tool_name.clone()), tool_input: invocation .arguments .clone() diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 8eca2caf3f07..812064c6c24d 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -316,20 +316,22 @@ impl ToolHandler for ApplyPatchHandler { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { apply_patch_payload_command(&invocation.payload).map(|command| PreToolUsePayload { tool_name: HookToolName::apply_patch(), - command, + tool_input: serde_json::json!({ "command": command }), }) } fn post_tool_use_payload( &self, - call_id: &str, - payload: &ToolPayload, - result: &dyn ToolOutput, + invocation: &ToolInvocation, + result: &Self::Output, ) -> Option { - let tool_response = result.post_tool_use_response(call_id, payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::apply_patch(), - command: apply_patch_payload_command(payload)?, + tool_input: serde_json::json!({ + "command": apply_patch_payload_command(&invocation.payload)?, + }), tool_response, }) } 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 979b4d4e96d2..bd48db152c4b 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -54,7 +54,7 @@ async fn pre_tool_use_payload_uses_json_patch_input() { handler.pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), - command: patch.to_string(), + tool_input: json!({ "command": patch }), }) ); } @@ -72,25 +72,26 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { handler.pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), - command: patch.to_string(), + tool_input: json!({ "command": patch }), }) ); } -#[test] -fn post_tool_use_payload_uses_patch_input_and_tool_output() { +#[tokio::test] +async fn post_tool_use_payload_uses_patch_input_and_tool_output() { let patch = sample_patch(); let payload = ToolPayload::Custom { input: patch.to_string(), }; + let invocation = invocation_for_payload(payload).await; let output = ApplyPatchToolOutput::from_text("Success. Updated files.".to_string()); let handler = ApplyPatchHandler; assert_eq!( - handler.post_tool_use_payload("call-apply-patch", &payload, &output), + handler.post_tool_use_payload(&invocation, &output), Some(PostToolUsePayload { tool_name: HookToolName::apply_patch(), - command: patch.to_string(), + tool_input: json!({ "command": patch }), tool_response: json!("Success. Updated files."), }) ); diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 424da160e373..b915661d2cf7 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -134,6 +134,7 @@ mod tests { McpHandler.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".to_string(), tool_name: codex_tools::ToolName::namespaced("mcp__memory__", "create_entities"), @@ -183,6 +184,7 @@ mod tests { &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-post".to_string(), tool_name: codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"), diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 809fecda932b..ac7dbfdf557b 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -286,6 +286,7 @@ async fn build_post_tool_use_payload_uses_tool_output_wire_value() { &ToolInvocation { session: session.into(), turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), call_id: "call-42".to_string(), tool_name: codex_tools::ToolName::plain("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 b7dca0d25963..905c5a2cdb17 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -273,6 +273,7 @@ async fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_s &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("exec_command"), @@ -315,6 +316,7 @@ async fn exec_command_post_tool_use_payload_skips_interactive_exec() { &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("exec_command"), @@ -353,6 +355,7 @@ async fn exec_command_post_tool_use_payload_skips_running_sessions() { &ToolInvocation { session: session.into(), turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), call_id: "call-45".to_string(), tool_name: codex_tools::ToolName::plain("exec_command"), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index afc8751fa904..5efa636754d5 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -204,8 +204,7 @@ impl Approvable for ApplyPatchRuntime { ) -> Option { Some(PermissionRequestPayload { tool_name: HookToolName::apply_patch(), - command: req.action.patch.clone(), - description: None, + tool_input: serde_json::json!({ "command": req.action.patch }), }) } } diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index fea640b4482b..112a3d3a56be 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -105,8 +105,10 @@ fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { payload.tool_name.matcher_aliases(), &["Write".to_string(), "Edit".to_string()] ); - assert_eq!(payload.command, expected_patch); - assert_eq!(payload.description, None); + assert_eq!( + payload.tool_input, + serde_json::json!({ "command": expected_patch }) + ); } #[test] From c9dfe040baf1c26130c92bf95b626d236e485421 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 15:09:36 -0700 Subject: [PATCH 15/22] Use invocation metadata for post tool hooks --- .../core/src/tools/handlers/apply_patch.rs | 10 ++-- .../src/tools/handlers/apply_patch_tests.rs | 2 +- codex-rs/core/src/tools/handlers/mcp.rs | 24 +++++++--- codex-rs/core/src/tools/handlers/shell.rs | 20 ++++---- .../core/src/tools/handlers/shell_tests.rs | 12 ++++- .../core/src/tools/handlers/unified_exec.rs | 9 ++-- .../src/tools/handlers/unified_exec_tests.rs | 47 ++++++++++++++----- codex-rs/core/src/tools/registry.rs | 7 ++- 8 files changed, 86 insertions(+), 45 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 7aa4c52c749f..91d5543c0f52 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -322,16 +322,16 @@ impl ToolHandler for ApplyPatchHandler { fn post_tool_use_payload( &self, - call_id: &str, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &Self::Output, ) -> Option { - let tool_response = result.post_tool_use_response(call_id, payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::apply_patch(), - tool_use_id: call_id.to_string(), + tool_use_id: invocation.call_id.clone(), tool_input: serde_json::json!({ - "command": apply_patch_payload_command(payload)?, + "command": apply_patch_payload_command(&invocation.payload)?, }), tool_response, }) 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 13693a86a3aa..b1b9d0cbbe44 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -88,7 +88,7 @@ async fn post_tool_use_payload_uses_patch_input_and_tool_output() { let handler = ApplyPatchHandler; assert_eq!( - handler.post_tool_use_payload(&invocation.call_id, &invocation.payload, &output), + handler.post_tool_use_payload(&invocation, &output), Some(PostToolUsePayload { tool_name: HookToolName::apply_patch(), tool_use_id: "call-apply-patch".to_string(), diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index e939ee1177e0..d2cad19d03ae 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -36,18 +36,18 @@ impl ToolHandler for McpHandler { fn post_tool_use_payload( &self, - call_id: &str, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &Self::Output, ) -> Option { - let ToolPayload::Mcp { server, tool, .. } = payload else { + let ToolPayload::Mcp { .. } = &invocation.payload else { return None; }; - let tool_response = result.post_tool_use_response(call_id, payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { - tool_name: HookToolName::new(format!("mcp__{server}__{tool}")), - tool_use_id: call_id.to_string(), + tool_name: HookToolName::new(invocation.tool_name.display()), + tool_use_id: invocation.call_id.clone(), tool_input: result.tool_input.clone(), tool_response, }) @@ -178,8 +178,18 @@ mod tests { wall_time: Duration::from_millis(42), original_image_detail_supported: true, }; + let (session, turn) = make_session_and_context().await; + let invocation = 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-post".to_string(), + tool_name: codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"), + payload, + }; assert_eq!( - McpHandler.post_tool_use_payload("call-mcp-post", &payload, &output), + McpHandler.post_tool_use_payload(&invocation, &output), Some(PostToolUsePayload { tool_name: HookToolName::new("mcp__filesystem__read_file"), tool_use_id: "call-mcp-post".to_string(), diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 0b79d8989a32..f4ab61b52481 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -214,15 +214,15 @@ impl ToolHandler for ShellHandler { fn post_tool_use_payload( &self, - call_id: &str, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &Self::Output, ) -> Option { - let tool_response = result.post_tool_use_response(call_id, payload)?; - let command = shell_payload_command(payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; + let command = shell_payload_command(&invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - tool_use_id: call_id.to_string(), + tool_use_id: invocation.call_id.clone(), tool_input: serde_json::json!({ "command": command }), tool_response, }) @@ -328,15 +328,15 @@ impl ToolHandler for ShellCommandHandler { fn post_tool_use_payload( &self, - call_id: &str, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &Self::Output, ) -> Option { - let tool_response = result.post_tool_use_response(call_id, payload)?; - let command = shell_command_payload_command(payload)?; + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; + let command = shell_command_payload_command(&invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - tool_use_id: call_id.to_string(), + tool_use_id: invocation.call_id.clone(), tool_input: serde_json::json!({ "command": command }), tool_response, }) diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 349b3f9bc11d..83f52d0ea5e1 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -279,8 +279,18 @@ async fn build_post_tool_use_payload_uses_tool_output_wire_value() { let handler = ShellCommandHandler { backend: super::ShellCommandBackend::Classic, }; + let (session, turn) = make_session_and_context().await; + let invocation = ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-42".to_string(), + tool_name: codex_tools::ToolName::plain("shell_command"), + payload, + }; assert_eq!( - handler.post_tool_use_payload("call-42", &payload, &output), + handler.post_tool_use_payload(&invocation, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id: "call-42".to_string(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index de442f3ea8e1..05b54ff884a4 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -145,21 +145,20 @@ impl ToolHandler for UnifiedExecHandler { fn post_tool_use_payload( &self, - call_id: &str, - payload: &ToolPayload, + invocation: &ToolInvocation, result: &Self::Output, ) -> Option { - let ToolPayload::Function { .. } = payload else { + let ToolPayload::Function { .. } = &invocation.payload else { return None; }; let command = result.hook_command.clone()?; let tool_use_id = if result.event_call_id.is_empty() { - call_id.to_string() + invocation.call_id.clone() } else { result.event_call_id.clone() }; - let tool_response = result.post_tool_use_response(&tool_use_id, payload)?; + let tool_response = result.post_tool_use_response(&tool_use_id, &invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id, 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 18e0d88a25ca..0dd656bd6571 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -22,6 +22,23 @@ use crate::tools::registry::ToolHandler; use crate::turn_diff_tracker::TurnDiffTracker; use tokio::sync::Mutex; +async fn invocation_for_payload( + tool_name: &str, + call_id: &str, + payload: ToolPayload, +) -> ToolInvocation { + let (session, turn) = make_session_and_context().await; + ToolInvocation { + session: session.into(), + turn: turn.into(), + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: call_id.to_string(), + tool_name: codex_tools::ToolName::plain(tool_name), + payload, + } +} + #[test] fn test_get_command_uses_default_shell_when_unspecified() -> anyhow::Result<()> { let json = r#"{"cmd": "echo hello"}"#; @@ -262,8 +279,9 @@ async fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_s original_token_count: None, hook_command: Some("echo three".to_string()), }; + let invocation = invocation_for_payload("exec_command", "call-43", payload).await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-43", &payload, &output), + UnifiedExecHandler.post_tool_use_payload(&invocation, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id: "call-43".to_string(), @@ -273,8 +291,8 @@ async fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_s ); } -#[test] -fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() { +#[tokio::test] +async fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "cmd": "echo three", "tty": true }).to_string(), }; @@ -289,9 +307,10 @@ fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() { original_token_count: None, hook_command: Some("echo three".to_string()), }; + let invocation = invocation_for_payload("exec_command", "call-44", payload).await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-44", &payload, &output), + UnifiedExecHandler.post_tool_use_payload(&invocation, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id: "call-44".to_string(), @@ -317,14 +336,15 @@ async fn exec_command_post_tool_use_payload_skips_running_sessions() { original_token_count: None, hook_command: Some("echo three".to_string()), }; + let invocation = invocation_for_payload("exec_command", "call-45", payload).await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("call-45", &payload, &output), + UnifiedExecHandler.post_tool_use_payload(&invocation, &output), None ); } -#[test] -fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_completion() { +#[tokio::test] +async fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_completion() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "session_id": 45, @@ -343,9 +363,10 @@ fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_c original_token_count: None, hook_command: Some("sleep 1; echo finished".to_string()), }; + let invocation = invocation_for_payload("write_stdin", "write-stdin-call", payload).await; assert_eq!( - UnifiedExecHandler.post_tool_use_payload("write-stdin-call", &payload, &output), + UnifiedExecHandler.post_tool_use_payload(&invocation, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id: "exec-call-45".to_string(), @@ -355,8 +376,8 @@ fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_c ); } -#[test] -fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() { +#[tokio::test] +async fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "session_id": 45, "chars": "" }).to_string(), }; @@ -382,10 +403,12 @@ fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() original_token_count: None, hook_command: Some("sleep 1; echo beta".to_string()), }; + let invocation_b = invocation_for_payload("write_stdin", "write-call-b", payload.clone()).await; + let invocation_a = invocation_for_payload("write_stdin", "write-call-a", payload).await; let payloads = [ - UnifiedExecHandler.post_tool_use_payload("write-call-b", &payload, &output_b), - UnifiedExecHandler.post_tool_use_payload("write-call-a", &payload, &output_a), + UnifiedExecHandler.post_tool_use_payload(&invocation_b, &output_b), + UnifiedExecHandler.post_tool_use_payload(&invocation_a, &output_a), ]; assert_eq!( diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 8d506a166c32..523d8ed65042 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -70,8 +70,7 @@ pub trait ToolHandler: Send + Sync { fn post_tool_use_payload( &self, - _call_id: &str, - _payload: &ToolPayload, + _invocation: &ToolInvocation, _result: &Self::Output, ) -> Option { None @@ -198,9 +197,9 @@ where Box::pin(async move { let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); - let output = self.handle(invocation).await?; + let output = self.handle(invocation.clone()).await?; let post_tool_use_payload = - ToolHandler::post_tool_use_payload(self, &call_id, &payload, &output); + ToolHandler::post_tool_use_payload(self, &invocation, &output); Ok(AnyToolResult { call_id, payload, From 6f7f1338b8f97f6de20ac1ea0dee9bdc60c7d514 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 15:41:38 -0700 Subject: [PATCH 16/22] Simplify MCP post-hook tool input capture --- codex-rs/core/src/mcp_tool_call.rs | 99 +++++++++++++++++------------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index f8150b35daeb..61822ed961fb 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -198,14 +198,24 @@ pub(crate) async fn handle_mcp_tool_call( maybe_mark_thread_memory_mode_polluted(sess.as_ref(), turn_context.as_ref()).await; let start = Instant::now(); + // Capture the rewritten arguments here so PostToolUse hooks see + // the same input that is sent to the MCP server. + let rewrite = rewrite_mcp_tool_input( + sess.as_ref(), + turn_context.as_ref(), + arguments_value.clone(), + metadata.as_ref(), + ) + .await; + let tool_input = tool_input_for_rewrite(&rewrite, arguments_value.clone()); let result = async { + let rewritten_arguments = rewrite?; execute_mcp_tool_call( sess.as_ref(), turn_context.as_ref(), &server, &tool_name, - arguments_value.clone(), - metadata.as_ref(), + rewritten_arguments, request_meta.clone(), ) .await @@ -232,10 +242,7 @@ pub(crate) async fn handle_mcp_tool_call( invocation, mcp_app_resource_uri: mcp_app_resource_uri.clone(), duration, - result: result - .as_ref() - .map(|result| result.result.clone()) - .map_err(Clone::clone), + result: result.clone(), }); notify_mcp_tool_call_event( sess.as_ref(), @@ -260,7 +267,10 @@ pub(crate) async fn handle_mcp_tool_call( Some(duration), ); - return build_handled_mcp_tool_call(result, arguments_value); + return HandledMcpToolCall { + result: CallToolResult::from_result(result), + tool_input, + }; } McpToolApprovalDecision::Decline { message } => { let message = message.unwrap_or_else(|| "user rejected MCP tool call".to_string()); @@ -322,14 +332,24 @@ pub(crate) async fn handle_mcp_tool_call( maybe_mark_thread_memory_mode_polluted(sess.as_ref(), turn_context.as_ref()).await; let start = Instant::now(); + // Capture the rewritten arguments here so PostToolUse hooks see the same + // input that is sent to the MCP server. + let rewrite = rewrite_mcp_tool_input( + sess.as_ref(), + turn_context.as_ref(), + arguments_value.clone(), + metadata.as_ref(), + ) + .await; + let tool_input = tool_input_for_rewrite(&rewrite, arguments_value.clone()); let result = async { + let rewritten_arguments = rewrite?; execute_mcp_tool_call( sess.as_ref(), turn_context.as_ref(), &server, &tool_name, - arguments_value.clone(), - metadata.as_ref(), + rewritten_arguments, request_meta, ) .await @@ -356,10 +376,7 @@ pub(crate) async fn handle_mcp_tool_call( invocation, mcp_app_resource_uri, duration, - result: result - .as_ref() - .map(|result| result.result.clone()) - .map_err(Clone::clone), + result: result.clone(), }); notify_mcp_tool_call_event( @@ -380,7 +397,10 @@ pub(crate) async fn handle_mcp_tool_call( Some(duration), ); - build_handled_mcp_tool_call(result, arguments_value) + HandledMcpToolCall { + result: CallToolResult::from_result(result), + tool_input, + } } pub(crate) struct HandledMcpToolCall { @@ -388,25 +408,32 @@ pub(crate) struct HandledMcpToolCall { pub(crate) tool_input: JsonValue, } -struct ExecutedMcpToolCall { - result: CallToolResult, - arguments: Option, +async fn rewrite_mcp_tool_input( + sess: &Session, + turn_context: &TurnContext, + arguments_value: Option, + metadata: Option<&McpToolApprovalMetadata>, +) -> Result, String> { + rewrite_mcp_tool_arguments_for_openai_files( + sess, + turn_context, + arguments_value, + metadata.and_then(|metadata| metadata.openai_file_input_params.as_deref()), + ) + .await } -fn build_handled_mcp_tool_call( - result: Result, +fn tool_input_for_rewrite( + rewrite: &Result, String>, fallback_arguments: Option, -) -> HandledMcpToolCall { - let tool_input = result +) -> JsonValue { + rewrite .as_ref() .ok() - .and_then(|result| result.arguments.clone()) + .cloned() + .flatten() .or(fallback_arguments) - .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())); - HandledMcpToolCall { - result: CallToolResult::from_result(result.map(|result| result.result)), - tool_input, - } + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())) } fn emit_mcp_call_metrics( @@ -515,18 +542,9 @@ async fn execute_mcp_tool_call( turn_context: &TurnContext, server: &str, tool_name: &str, - arguments_value: Option, - metadata: Option<&McpToolApprovalMetadata>, + rewritten_arguments: Option, request_meta: Option, -) -> Result { - let rewritten_arguments = rewrite_mcp_tool_arguments_for_openai_files( - sess, - turn_context, - arguments_value, - metadata.and_then(|metadata| metadata.openai_file_input_params.as_deref()), - ) - .await?; - let arguments = rewritten_arguments.clone(); +) -> Result { let request_meta = with_mcp_tool_call_thread_id_meta(request_meta, &sess.conversation_id.to_string()); let request_meta = @@ -537,14 +555,13 @@ async fn execute_mcp_tool_call( .call_tool(server, tool_name, rewritten_arguments, request_meta) .await .map_err(|e| format!("tool call error: {e:?}"))?; - let result = sanitize_mcp_tool_result_for_model( + sanitize_mcp_tool_result_for_model( turn_context .model_info .input_modalities .contains(&InputModality::Image), Ok(result), - )?; - Ok(ExecutedMcpToolCall { result, arguments }) + ) } #[expect( From 4248d43f0176f04cfc7a0a3198ff022f55aa97fb Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 16:10:51 -0700 Subject: [PATCH 17/22] Use invocation tool name for MCP hooks --- codex-rs/core/src/mcp_tool_call.rs | 53 ++++++++-------- codex-rs/core/src/mcp_tool_call_tests.rs | 80 +++++++++++++++++++----- codex-rs/core/src/tools/handlers/mcp.rs | 2 + 3 files changed, 93 insertions(+), 42 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 61822ed961fb..5675dd5b87bc 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -82,6 +82,7 @@ pub(crate) async fn handle_mcp_tool_call( call_id: String, server: String, tool_name: String, + hook_tool_name: String, arguments: String, ) -> HandledMcpToolCall { // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON @@ -186,6 +187,7 @@ pub(crate) async fn handle_mcp_tool_call( turn_context, &call_id, &invocation, + &hook_tool_name, metadata.as_ref(), approval_mode, ) @@ -722,7 +724,6 @@ pub(crate) struct McpToolApprovalMetadata { connector_description: Option, tool_title: Option, tool_description: Option, - model_tool_name: String, mcp_app_resource_uri: Option, codex_apps_meta: Option>, openai_file_input_params: Option>, @@ -883,6 +884,7 @@ async fn maybe_request_mcp_tool_approval( turn_context: &Arc, call_id: &str, invocation: &McpInvocation, + hook_tool_name: &str, metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, ) -> Option { @@ -933,31 +935,29 @@ async fn maybe_request_mcp_tool_approval( return Some(McpToolApprovalDecision::Accept); } - if let Some(metadata) = metadata { - match run_permission_request_hooks( - sess, - turn_context, - call_id, - PermissionRequestPayload { - tool_name: HookToolName::new(metadata.model_tool_name.clone()), - tool_input: invocation - .arguments - .clone() - .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), - }, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return Some(McpToolApprovalDecision::Accept); - } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); - } - None => {} + match run_permission_request_hooks( + sess, + turn_context, + call_id, + PermissionRequestPayload { + tool_name: HookToolName::new(hook_tool_name), + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), + }, + ) + .await + { + Some(PermissionRequestDecision::Allow) => { + return Some(McpToolApprovalDecision::Accept); } + Some(PermissionRequestDecision::Deny { message }) => { + return Some(McpToolApprovalDecision::Decline { + message: Some(message), + }); + } + None => {} } let tool_call_mcp_elicitation_enabled = turn_context @@ -1239,8 +1239,6 @@ pub(crate) async fn lookup_mcp_tool_metadata( None }; - let model_tool_name = tool_info.canonical_tool_name().display(); - Some(McpToolApprovalMetadata { annotations: tool_info.tool.annotations, connector_id: tool_info.connector_id, @@ -1248,7 +1246,6 @@ pub(crate) async fn lookup_mcp_tool_metadata( connector_description, tool_title: tool_info.tool.title, tool_description: tool_info.tool.description.map(std::borrow::Cow::into_owned), - model_tool_name, mcp_app_resource_uri: get_mcp_app_resource_uri(tool_info.tool.meta.as_deref()), codex_apps_meta: tool_info .tool diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index b47a81a94344..583a5ed22f18 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -62,7 +62,6 @@ fn approval_metadata( connector_description: connector_description.map(str::to_string), tool_title: tool_title.map(str::to_string), tool_description: tool_description.map(str::to_string), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -684,7 +683,6 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps connector_description: Some("Manage events".to_string()), tool_title: Some("Create Event".to_string()), tool_description: Some("Create a calendar event.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: Some( serde_json::json!({ @@ -872,7 +870,6 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { connector_description: None, tool_title: None, tool_description: None, - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1437,7 +1434,6 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1448,6 +1444,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &turn_context, "call-1", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -1510,7 +1507,6 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1521,6 +1517,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &turn_context, "call-guardian", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Auto, ) @@ -1566,7 +1563,6 @@ async fn permission_request_hook_allows_mcp_tool_call() { connector_description: None, tool_title: Some("Create entities".to_string()), tool_description: None, - model_tool_name: "mcp__memory__create_entities".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1577,6 +1573,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &turn_context, "call-mcp-hook", &invocation, + "mcp__memory__create_entities", Some(&metadata), AppToolApproval::Auto, ) @@ -1609,6 +1606,61 @@ async fn permission_request_hook_allows_mcp_tool_call() { ); } +#[tokio::test] +async fn permission_request_hook_uses_hook_tool_name_without_metadata() { + let (mut session, turn_context) = make_session_and_context().await; + let log_path = install_mcp_permission_request_hook( + &mut session, + &turn_context, + "mcp__memory__.*", + &serde_json::json!({ + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { "behavior": "allow" } + } + }), + ); + 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": [] })), + }; + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-mcp-hook-no-metadata", + &invocation, + "mcp__memory__create_entities", + /*metadata*/ None, + AppToolApproval::Auto, + ) + .await; + + assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); + let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log"); + let inputs = log + .lines() + .map(|line| serde_json::from_str::(line).expect("parse hook input")) + .collect::>(); + assert_eq!( + inputs, + vec![serde_json::json!({ + "session_id": session.conversation_id, + "turn_id": "turn_id", + "cwd": turn_context.cwd, + "transcript_path": null, + "model": turn_context.model_info.slug, + "permission_mode": "default", + "tool_name": "mcp__memory__create_entities", + "hook_event_name": "PermissionRequest", + "tool_input": { "entities": [] } + })] + ); +} + #[tokio::test] async fn permission_request_hook_runs_after_remembered_mcp_approval() { let (mut session, turn_context) = make_session_and_context().await; @@ -1642,7 +1694,6 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { connector_description: None, tool_title: Some("Create entities".to_string()), tool_description: None, - model_tool_name: "mcp__memory__create_entities".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1659,6 +1710,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &turn_context, "call-mcp-remembered", &invocation, + "mcp__memory__create_entities", Some(&metadata), AppToolApproval::Auto, ) @@ -1728,7 +1780,6 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { connector_description: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Reads calendar data.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1739,6 +1790,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { &turn_context, "call-guardian-deny", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Auto, ) @@ -1781,7 +1833,6 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1796,6 +1847,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &turn_context, "call-prompt", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Prompt, ) @@ -1860,7 +1912,6 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1871,6 +1922,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { &turn_context, "call-2", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -1932,7 +1984,6 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { connector_description: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -1943,6 +1994,7 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { &turn_context, "call-2-custom", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -2004,7 +2056,6 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -2015,6 +2066,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { &turn_context, "call-3", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -2084,7 +2136,6 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -2100,6 +2151,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { &turn_context, "call-2", &invocation, + "mcp__test__tool", Some(&metadata), approval_mode, ) @@ -2191,7 +2243,6 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), - model_tool_name: "mcp__test__tool".to_string(), mcp_app_resource_uri: None, codex_apps_meta: None, openai_file_input_params: None, @@ -2202,6 +2253,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ &turn_context, "call-3", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index d2cad19d03ae..0de45ea6bcda 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -58,6 +58,7 @@ impl ToolHandler for McpHandler { session, turn, call_id, + tool_name: model_tool_name, payload, .. } = invocation; @@ -85,6 +86,7 @@ impl ToolHandler for McpHandler { call_id.clone(), server, tool, + model_tool_name.display(), arguments_str, ) .await; From e58dfb7e53154b8e54a8d85142036bc90b19f52c Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 16:42:04 -0700 Subject: [PATCH 18/22] Simplify approved MCP tool execution --- codex-rs/core/src/mcp_tool_call.rs | 206 ++++++++++------------------- 1 file changed, 68 insertions(+), 138 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 5675dd5b87bc..a9a0d6eb5c18 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -167,13 +167,6 @@ pub(crate) async fn handle_mcp_tool_call( let connector_name = metadata .as_ref() .and_then(|metadata| metadata.connector_name.clone()); - let server_origin = sess - .services - .mcp_connection_manager - .read() - .await - .server_origin(&server) - .map(str::to_string); let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent { call_id: call_id.clone(), @@ -197,82 +190,16 @@ pub(crate) async fn handle_mcp_tool_call( McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember => { - maybe_mark_thread_memory_mode_polluted(sess.as_ref(), turn_context.as_ref()).await; - - let start = Instant::now(); - // Capture the rewritten arguments here so PostToolUse hooks see - // the same input that is sent to the MCP server. - let rewrite = rewrite_mcp_tool_input( + return handle_approved_mcp_tool_call( sess.as_ref(), turn_context.as_ref(), - arguments_value.clone(), - metadata.as_ref(), - ) - .await; - let tool_input = tool_input_for_rewrite(&rewrite, arguments_value.clone()); - let result = async { - let rewritten_arguments = rewrite?; - execute_mcp_tool_call( - sess.as_ref(), - turn_context.as_ref(), - &server, - &tool_name, - rewritten_arguments, - request_meta.clone(), - ) - .await - } - .instrument(mcp_tool_call_span( - sess.as_ref(), - turn_context.as_ref(), - McpToolCallSpanFields { - server_name: &server, - tool_name: &tool_name, - call_id: &call_id, - server_origin: server_origin.as_deref(), - connector_id: connector_id.as_deref(), - connector_name: connector_name.as_deref(), - }, - )) - .await; - if let Err(error) = &result { - tracing::warn!("MCP tool call error: {error:?}"); - } - let duration = start.elapsed(); - let tool_call_end_event = EventMsg::McpToolCallEnd(McpToolCallEndEvent { - call_id: call_id.clone(), + &call_id, invocation, - mcp_app_resource_uri: mcp_app_resource_uri.clone(), - duration, - result: result.clone(), - }); - notify_mcp_tool_call_event( - sess.as_ref(), - turn_context.as_ref(), - tool_call_end_event.clone(), - ) - .await; - maybe_track_codex_app_used( - sess.as_ref(), - turn_context.as_ref(), - &server, - &tool_name, + metadata.as_ref(), + request_meta, + mcp_app_resource_uri, ) .await; - let status = if result.is_ok() { "ok" } else { "error" }; - emit_mcp_call_metrics( - turn_context.as_ref(), - status, - &tool_name, - connector_id.as_deref(), - connector_name.as_deref(), - Some(duration), - ); - - return HandledMcpToolCall { - result: CallToolResult::from_result(result), - tool_input, - }; } McpToolApprovalDecision::Decline { message } => { let message = message.unwrap_or_else(|| "user rejected MCP tool call".to_string()); @@ -331,24 +258,66 @@ pub(crate) async fn handle_mcp_tool_call( }; } - maybe_mark_thread_memory_mode_polluted(sess.as_ref(), turn_context.as_ref()).await; - - let start = Instant::now(); - // Capture the rewritten arguments here so PostToolUse hooks see the same - // input that is sent to the MCP server. - let rewrite = rewrite_mcp_tool_input( + handle_approved_mcp_tool_call( sess.as_ref(), turn_context.as_ref(), - arguments_value.clone(), + &call_id, + invocation, metadata.as_ref(), + request_meta, + mcp_app_resource_uri, + ) + .await +} + +pub(crate) struct HandledMcpToolCall { + pub(crate) result: CallToolResult, + pub(crate) tool_input: JsonValue, +} + +async fn handle_approved_mcp_tool_call( + sess: &Session, + turn_context: &TurnContext, + call_id: &str, + invocation: McpInvocation, + metadata: Option<&McpToolApprovalMetadata>, + request_meta: Option, + mcp_app_resource_uri: Option, +) -> HandledMcpToolCall { + maybe_mark_thread_memory_mode_polluted(sess, turn_context).await; + + let server = invocation.server.clone(); + let tool_name = invocation.tool.clone(); + let arguments_value = invocation.arguments.clone(); + let connector_id = metadata.and_then(|metadata| metadata.connector_id.as_deref()); + let connector_name = metadata.and_then(|metadata| metadata.connector_name.as_deref()); + let server_origin = sess + .services + .mcp_connection_manager + .read() + .await + .server_origin(&server) + .map(str::to_string); + + let start = Instant::now(); + let rewrite = rewrite_mcp_tool_arguments_for_openai_files( + sess, + turn_context, + arguments_value.clone(), + metadata.and_then(|metadata| metadata.openai_file_input_params.as_deref()), ) .await; - let tool_input = tool_input_for_rewrite(&rewrite, arguments_value.clone()); + let tool_input = match &rewrite { + Ok(Some(rewritten_arguments)) => rewritten_arguments.clone(), + Ok(None) | Err(_) => arguments_value + .clone() + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), + }; let result = async { let rewritten_arguments = rewrite?; execute_mcp_tool_call( - sess.as_ref(), - turn_context.as_ref(), + sess, + turn_context, &server, &tool_name, rewritten_arguments, @@ -357,15 +326,15 @@ pub(crate) async fn handle_mcp_tool_call( .await } .instrument(mcp_tool_call_span( - sess.as_ref(), - turn_context.as_ref(), + sess, + turn_context, McpToolCallSpanFields { server_name: &server, tool_name: &tool_name, - call_id: &call_id, + call_id, server_origin: server_origin.as_deref(), - connector_id: connector_id.as_deref(), - connector_name: connector_name.as_deref(), + connector_id, + connector_name, }, )) .await; @@ -374,28 +343,22 @@ pub(crate) async fn handle_mcp_tool_call( } let duration = start.elapsed(); let tool_call_end_event = EventMsg::McpToolCallEnd(McpToolCallEndEvent { - call_id: call_id.clone(), + call_id: call_id.to_string(), invocation, mcp_app_resource_uri, duration, result: result.clone(), }); - - notify_mcp_tool_call_event( - sess.as_ref(), - turn_context.as_ref(), - tool_call_end_event.clone(), - ) - .await; - maybe_track_codex_app_used(sess.as_ref(), turn_context.as_ref(), &server, &tool_name).await; + notify_mcp_tool_call_event(sess, turn_context, tool_call_end_event.clone()).await; + maybe_track_codex_app_used(sess, turn_context, &server, &tool_name).await; let status = if result.is_ok() { "ok" } else { "error" }; emit_mcp_call_metrics( - turn_context.as_ref(), + turn_context, status, &tool_name, - connector_id.as_deref(), - connector_name.as_deref(), + connector_id, + connector_name, Some(duration), ); @@ -405,39 +368,6 @@ pub(crate) async fn handle_mcp_tool_call( } } -pub(crate) struct HandledMcpToolCall { - pub(crate) result: CallToolResult, - pub(crate) tool_input: JsonValue, -} - -async fn rewrite_mcp_tool_input( - sess: &Session, - turn_context: &TurnContext, - arguments_value: Option, - metadata: Option<&McpToolApprovalMetadata>, -) -> Result, String> { - rewrite_mcp_tool_arguments_for_openai_files( - sess, - turn_context, - arguments_value, - metadata.and_then(|metadata| metadata.openai_file_input_params.as_deref()), - ) - .await -} - -fn tool_input_for_rewrite( - rewrite: &Result, String>, - fallback_arguments: Option, -) -> JsonValue { - rewrite - .as_ref() - .ok() - .cloned() - .flatten() - .or(fallback_arguments) - .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())) -} - fn emit_mcp_call_metrics( turn_context: &TurnContext, status: &str, From b010ef367630d5f39f5c2b6fb5c0a3b05ba5b103 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 21:36:34 -0700 Subject: [PATCH 19/22] Restore realtime delegated shell timeout --- codex-rs/app-server/tests/suite/v2/realtime_conversation.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs b/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs index 793efbe1e182..dfc3fea31820 100644 --- a/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs +++ b/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs @@ -72,6 +72,7 @@ use wiremock::matchers::path; use wiremock::matchers::path_regex; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); +const DELEGATED_SHELL_TOOL_TIMEOUT_MS: u64 = 30_000; const STARTUP_CONTEXT_HEADER: &str = "Startup context from Codex."; const V2_STEERING_ACKNOWLEDGEMENT: &str = "This was sent to steer the previous background agent task."; @@ -1781,7 +1782,9 @@ async fn webrtc_v2_tool_call_delegated_turn_can_execute_shell_tool() -> Result<( create_shell_command_sse_response( realtime_tool_ok_command(), /*workdir*/ None, - Some(5000), + // Windows CI can spend several seconds starting the nested PowerShell command. This + // test verifies delegated shell-tool plumbing, not timeout enforcement. + Some(DELEGATED_SHELL_TOOL_TIMEOUT_MS), "shell_call", )?, create_final_assistant_message_sse_response("shell tool finished")?, From 9986073c8a504038fec0d2ab9a0ab94d23e9ffa7 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 22:10:49 -0700 Subject: [PATCH 20/22] Fix hook engine test request payloads --- codex-rs/hooks/src/engine/mod_tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 527574b500a7..81004aefb42b 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -125,7 +125,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: tool_name: "Bash".to_string(), matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), + tool_input: serde_json::json!({ "command": "echo hello" }), }); assert_eq!(preview.len(), 1); assert_eq!(preview[0].source_path, managed_dir); @@ -141,7 +141,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: tool_name: "Bash".to_string(), matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), + tool_input: serde_json::json!({ "command": "echo hello" }), }) .await; @@ -212,7 +212,7 @@ fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { tool_name: "Bash".to_string(), matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), + tool_input: serde_json::json!({ "command": "echo hello" }), }) .is_empty() ); @@ -318,7 +318,7 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { tool_name: "Bash".to_string(), matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), + tool_input: serde_json::json!({ "command": "echo hello" }), }); assert_eq!(preview.len(), 2); assert!(engine.handlers.iter().all(|handler| !handler.is_managed)); From c81fbb13ccb65949c5720fe92e2ead54e6ede0f3 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 22 Apr 2026 23:25:54 -0700 Subject: [PATCH 21/22] Fix MCP hook tests on Windows --- codex-rs/core/src/mcp_tool_call_tests.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index c56407658d7f..211120eaceba 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -27,6 +27,7 @@ use core_test_support::responses::start_mock_server; use pretty_assertions::assert_eq; use serde::Deserialize; use std::collections::HashMap; +use std::path::Path; use std::sync::Arc; use tempfile::tempdir; use tracing::Instrument; @@ -111,6 +112,7 @@ print({hook_output:?}) ); std::fs::write(&script_path, script).expect("write MCP permission hook script"); + let python = if cfg!(windows) { "python" } else { "python3" }; std::fs::write( turn_context.config.codex_home.join("hooks.json"), serde_json::json!({ @@ -119,7 +121,8 @@ print({hook_output:?}) "matcher": matcher, "hooks": [{ "type": "command", - "command": format!("python3 {}", script_path.display()), + "command": format!("{python} {}", quote_hook_script_path(&script_path)), + "timeout_sec": 5, }] }] } @@ -131,14 +134,27 @@ print({hook_output:?}) session.services.hooks = Hooks::new(HooksConfig { feature_enabled: true, config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), - shell_program: Some("/bin/sh".to_string()), - shell_args: vec!["-c".to_string()], + shell_program: (!cfg!(windows)).then_some("/bin/sh".to_string()), + shell_args: if cfg!(windows) { + Vec::new() + } else { + vec!["-c".to_string()] + }, ..HooksConfig::default() }); log_path.to_path_buf() } +fn quote_hook_script_path(path: &Path) -> String { + let path = path.display().to_string(); + if cfg!(windows) { + format!("\"{}\"", path.replace('"', "\\\"")) + } else { + format!("'{}'", path.replace('\'', "'\\''")) + } +} + #[test] fn mcp_app_resource_uri_reads_known_tool_meta_keys() { let nested = serde_json::json!({ From a8a7c9e9519e432d065ffc66eb3f351a78436045 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 23 Apr 2026 00:18:23 -0700 Subject: [PATCH 22/22] Fix Windows MCP hook test command --- codex-rs/core/src/mcp_tool_call_tests.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 211120eaceba..522686446b01 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -27,7 +27,6 @@ use core_test_support::responses::start_mock_server; use pretty_assertions::assert_eq; use serde::Deserialize; use std::collections::HashMap; -use std::path::Path; use std::sync::Arc; use tempfile::tempdir; use tracing::Instrument; @@ -113,6 +112,14 @@ print({hook_output:?}) std::fs::write(&script_path, script).expect("write MCP permission hook script"); let python = if cfg!(windows) { "python" } else { "python3" }; + let script_path_arg = if cfg!(windows) { + script_path.display().to_string() + } else { + format!( + "'{}'", + script_path.display().to_string().replace('\'', "'\\''") + ) + }; std::fs::write( turn_context.config.codex_home.join("hooks.json"), serde_json::json!({ @@ -121,7 +128,7 @@ print({hook_output:?}) "matcher": matcher, "hooks": [{ "type": "command", - "command": format!("{python} {}", quote_hook_script_path(&script_path)), + "command": format!("{python} {script_path_arg}"), "timeout_sec": 5, }] }] @@ -146,15 +153,6 @@ print({hook_output:?}) log_path.to_path_buf() } -fn quote_hook_script_path(path: &Path) -> String { - let path = path.display().to_string(); - if cfg!(windows) { - format!("\"{}\"", path.replace('"', "\\\"")) - } else { - format!("'{}'", path.replace('\'', "'\\''")) - } -} - #[test] fn mcp_app_resource_uri_reads_known_tool_meta_keys() { let nested = serde_json::json!({