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")?, diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index c647d146d46b..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, - command: String, + 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, - command, + 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 @@ -185,8 +200,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 +216,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 +226,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 +239,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..a9a0d6eb5c18 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -25,16 +25,20 @@ 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::hook_names::HookToolName; +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; @@ -59,6 +63,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; @@ -77,8 +82,9 @@ pub(crate) async fn handle_mcp_tool_call( call_id: String, server: String, tool_name: String, + hook_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() { @@ -88,7 +94,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()), + }; } } }; @@ -144,7 +153,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()); @@ -154,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(), @@ -174,115 +180,64 @@ pub(crate) async fn handle_mcp_tool_call( turn_context, &call_id, &invocation, + &hook_tool_name, metadata.as_ref(), approval_mode, ) .await { - let (result, call_duration) = match decision { + let result = match decision { McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember => { - maybe_mark_thread_memory_mode_polluted(sess.as_ref(), turn_context.as_ref()).await; - - let start = Instant::now(); - let result = async { - execute_mcp_tool_call( - sess.as_ref(), - turn_context.as_ref(), - &server, - &tool_name, - arguments_value.clone(), - metadata.as_ref(), - request_meta.clone(), - ) - .await - } - .instrument(mcp_tool_call_span( + return handle_approved_mcp_tool_call( 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; - (result, Some(duration)) } 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 } }; @@ -293,37 +248,93 @@ pub(crate) async fn handle_mcp_tool_call( &tool_name, connector_id.as_deref(), connector_name.as_deref(), - call_duration, + /*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; + handle_approved_mcp_tool_call( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + metadata.as_ref(), + request_meta, + mcp_app_resource_uri, + ) + .await +} + +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 = 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, - arguments_value.clone(), - metadata.as_ref(), + rewritten_arguments, request_meta, ) .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; @@ -332,32 +343,29 @@ 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), ); - CallToolResult::from_result(result) + HandledMcpToolCall { + result: CallToolResult::from_result(result), + tool_input, + } } fn emit_mcp_call_metrics( @@ -466,17 +474,9 @@ async fn execute_mcp_tool_call( turn_context: &TurnContext, server: &str, tool_name: &str, - arguments_value: Option, - metadata: Option<&McpToolApprovalMetadata>, - request_meta: Option, + 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 request_meta = with_mcp_tool_call_thread_id_meta(request_meta, &sess.conversation_id.to_string()); let request_meta = @@ -814,6 +814,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 { @@ -863,6 +864,32 @@ async fn maybe_request_mcp_tool_approval( { return Some(McpToolApprovalDecision::Accept); } + + 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 .config .features diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index ed355c47b4da..522686446b01 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; @@ -76,6 +78,81 @@ 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"); + 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!({ + "hooks": { + "PermissionRequest": [{ + "matcher": matcher, + "hooks": [{ + "type": "command", + "command": format!("{python} {script_path_arg}"), + "timeout_sec": 5, + }] + }] + } + }) + .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: (!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() +} + #[test] fn mcp_app_resource_uri_reads_known_tool_meta_keys() { let nested = serde_json::json!({ @@ -1381,6 +1458,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &turn_context, "call-1", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -1453,6 +1531,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, ) @@ -1461,6 +1540,203 @@ 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, + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + }; + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-mcp-hook", + &invocation, + "mcp__memory__create_entities", + Some(&metadata), + AppToolApproval::Auto, + ) + .await; + + assert_eq!(decision, Some(McpToolApprovalDecision::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_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; + 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, + 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, + "mcp__memory__create_entities", + 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; @@ -1528,6 +1804,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { &turn_context, "call-guardian-deny", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Auto, ) @@ -1584,6 +1861,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, ) @@ -1658,6 +1936,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { &turn_context, "call-2", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -1729,6 +2008,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, ) @@ -1800,6 +2080,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { &turn_context, "call-3", &invocation, + "mcp__test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -1884,6 +2165,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, ) @@ -1985,6 +2267,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/context.rs b/codex-rs/core/src/tools/context.rs index feb4b02d71b7..63bb641ef879 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, } @@ -162,6 +163,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/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index 60ad4561519f..4f40342e49fe 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/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 0ccc62d9e988..91d5543c0f52 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -316,21 +316,23 @@ 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, + 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(), - command: apply_patch_payload_command(payload)?, + tool_use_id: invocation.call_id.clone(), + 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 39c313310316..b1b9d0cbbe44 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,26 +72,27 @@ 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(), tool_use_id: "call-apply-patch".to_string(), - 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 b24db734b913..0de45ea6bcda 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,11 +23,42 @@ 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), + }) + } + + fn post_tool_use_payload( + &self, + invocation: &ToolInvocation, + result: &Self::Output, + ) -> Option { + let ToolPayload::Mcp { .. } = &invocation.payload else { + return None; + }; + + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; + Some(PostToolUsePayload { + tool_name: HookToolName::new(invocation.tool_name.display()), + tool_use_id: invocation.call_id.clone(), + tool_input: result.tool_input.clone(), + tool_response, + }) + } + async fn handle(&self, invocation: ToolInvocation) -> Result { let ToolInvocation { session, turn, call_id, + tool_name: model_tool_name, payload, .. } = invocation; @@ -50,14 +86,133 @@ impl ToolHandler for McpHandler { call_id.clone(), server, tool, + model_tool_name.display(), arguments_str, ) .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), }) } } + +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(), + 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"), + payload, + }), + Some(PreToolUsePayload { + tool_name: HookToolName::new("mcp__memory__create_entities"), + tool_input: json!({ + "entities": [{ + "name": "Ada", + "entityType": "person" + }] + }), + }) + ); + } + + #[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(), + 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, + }, + tool_input: json!({ + "path": { + "file_id": "file_123" + } + }), + 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(&invocation, &output), + Some(PostToolUsePayload { + tool_name: HookToolName::new("mcp__filesystem__read_file"), + tool_use_id: "call-mcp-post".to_string(), + tool_input: json!({ + "path": { + "file_id": "file_123" + } + }), + 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 b9718173b7d9..f4ab61b52481 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -208,21 +208,22 @@ 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 }), }) } 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)?; + let command = shell_payload_command(&invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - tool_use_id: call_id.to_string(), - command: shell_payload_command(payload)?, + tool_use_id: invocation.call_id.clone(), + tool_input: serde_json::json!({ "command": command }), tool_response, }) } @@ -321,21 +322,22 @@ 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 }), }) } 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)?; + let command = shell_command_payload_command(&invocation.payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - tool_use_id: call_id.to_string(), - command: shell_command_payload_command(payload)?, + 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 c13d834c061d..83f52d0ea5e1 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -234,7 +234,7 @@ 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'" }), }) ); } @@ -261,13 +261,13 @@ 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" }), }) ); } -#[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,13 +279,22 @@ 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(), - 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 813f4f862806..05b54ff884a4 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -139,31 +139,30 @@ impl ToolHandler for UnifiedExecHandler { .ok() .map(|args| PreToolUsePayload { tool_name: HookToolName::bash(), - command: args.cmd, + tool_input: serde_json::json!({ "command": args.cmd }), }) } 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, - command, + tool_input: serde_json::json!({ "command": command }), 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 0a5abe51d74d..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"}"#; @@ -219,7 +236,7 @@ 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" }), }) ); } @@ -246,8 +263,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(), }; @@ -262,20 +279,20 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co 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(), - command: "echo three".to_string(), + tool_input: serde_json::json!({ "command": "echo three" }), tool_response: serde_json::json!("three"), }) ); } -#[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(), }; @@ -290,20 +307,21 @@ 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(), - command: "echo three".to_string(), + tool_input: serde_json::json!({ "command": "echo three" }), tool_response: serde_json::json!("three"), }) ); } -#[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(), }; @@ -318,15 +336,15 @@ 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, @@ -345,20 +363,21 @@ 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(), - command: "sleep 1; echo finished".to_string(), + tool_input: serde_json::json!({ "command": "sleep 1; echo finished" }), tool_response: serde_json::json!("finished\n"), }) ); } -#[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(), }; @@ -384,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!( @@ -396,13 +417,13 @@ fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id: "exec-call-b".to_string(), - command: "sleep 1; echo beta".to_string(), + tool_input: serde_json::json!({ "command": "sleep 1; echo beta" }), tool_response: serde_json::json!("beta\n"), }), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), tool_use_id: "exec-call-a".to_string(), - command: "sleep 2; echo alpha".to_string(), + tool_input: serde_json::json!({ "command": "sleep 2; echo alpha" }), tool_response: serde_json::json!("alpha\n"), }), ] diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index d43859e62661..888add09fce8 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -8,7 +8,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; @@ -393,11 +392,7 @@ impl NetworkApprovalService { &session, &turn_context, &guardian_approval_id, - PermissionRequestPayload { - tool_name: HookToolName::bash(), - command, - description: Some(format!("network-access {target}")), - }, + PermissionRequestPayload::bash(command, Some(format!("network-access {target}"))), ) .await { diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 678e620bd865..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 @@ -136,8 +135,11 @@ 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, } #[derive(Debug, Clone, PartialEq)] @@ -149,8 +151,8 @@ pub(crate) struct PostToolUsePayload { pub(crate) tool_name: HookToolName, /// The originating tool-use id exposed at `tool_use_id`. pub(crate) tool_use_id: String, - /// 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, } @@ -195,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, @@ -328,20 +330,16 @@ 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.command.clone(), + &pre_tool_use_payload.tool_name, + &pre_tool_use_payload.tool_input, ) .await { - return Err(FunctionCallError::RespondToModel(format!( - "Command blocked by PreToolUse hook: {reason}. Command: {}", - pre_tool_use_payload.command - ))); + return Err(FunctionCallError::RespondToModel(message)); } let is_mutating = handler.is_mutating(&invocation).await; @@ -402,7 +400,7 @@ impl ToolRegistry { post_tool_use_payload.tool_use_id, 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/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 5de3235f836b..ed325a4e6842 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] diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 4bbbe774a7a5..e18d2c269ad3 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -17,7 +17,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; @@ -202,11 +201,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(), - }) + 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 361de9a96baa..83075c3674e7 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,11 +401,10 @@ impl CoreShellActionProvider { Ok(stopwatch .pause_for(async move { // 1) Run PermissionRequest hooks - let permission_request = PermissionRequestPayload { - tool_name: HookToolName::bash(), - command: codex_shell_command::parse_command::shlex_join(&command), - description: None, - }; + let permission_request = PermissionRequestPayload::bash( + codex_shell_command::parse_command::shlex_join(&command), + /*description*/ 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 1a4132da66d7..96d7abf64cd6 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -14,7 +14,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; @@ -187,11 +186,10 @@ impl Approvable for UnifiedExecRuntime<'_> { &self, req: &UnifiedExecRequest, ) -> Option { - Some(PermissionRequestPayload { - tool_name: HookToolName::bash(), - command: req.hook_command.clone(), - description: req.justification.clone(), - }) + 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 fcdcd785072f..922ce6a1d765 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -135,8 +135,25 @@ 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, +} + +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. diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 4a4dac3e8144..19eb7a67d967 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(), /*description*/ 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() { 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..2157630e02b0 --- /dev/null +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -0,0 +1,350 @@ +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, approval_mode: AppToolApproval) { + 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(approval_mode), + 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, + 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, approval_mode); +} + +#[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, AppToolApproval::Approve); + }) + .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, AppToolApproval::Approve); + }) + .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; 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(()) } 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/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)); diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index 80dd325378f7..de3f3292acd9 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -183,7 +183,7 @@ mod tests { } #[test] - fn matcher_uses_regex_matching() { + fn exact_matcher_supports_pipe_alternatives() { assert!(matches_matcher(Some("Edit|Write"), Some("Edit"))); assert!(matches_matcher(Some("Edit|Write"), Some("Write"))); assert!(!matches_matcher(Some("Edit|Write"), Some("Bash"))); @@ -191,22 +191,39 @@ mod tests { } #[test] - fn matcher_exact_string_does_not_substring_match() { + fn literal_matcher_uses_exact_matching() { assert!(matches_matcher(Some("Bash"), Some("Bash"))); assert!(!matches_matcher(Some("Bash"), Some("BashOutput"))); - assert_eq!(validate_matcher_pattern("Bash"), Ok(())); + 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 matcher_uses_regex_when_it_contains_other_characters() { + fn matcher_uses_regex_when_it_contains_regex_characters() { assert!(matches_matcher(Some("^Bash"), Some("BashOutput"))); + assert_eq!(validate_matcher_pattern("^Bash"), 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__memory"), - Some("mcp__memory__create_entities") + Some("mcp__.*__write.*"), + Some("mcp__filesystem__read_file") )); assert_eq!(validate_matcher_pattern("mcp__memory__.*"), Ok(())); } 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 265572069b78..20cdfd201018 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, } diff --git a/codex-rs/tui/src/app/thread_session_state.rs b/codex-rs/tui/src/app/thread_session_state.rs index cbf40d0d7541..d33df0f681f1 100644 --- a/codex-rs/tui/src/app/thread_session_state.rs +++ b/codex-rs/tui/src/app/thread_session_state.rs @@ -108,6 +108,7 @@ mod tests { approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, sandbox_policy: SandboxPolicy::new_read_only_policy(), + permission_profile: None, cwd: cwd.abs(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -155,7 +156,7 @@ mod tests { .insert(side_thread_id, SideThreadState::new(main_thread_id)); app.config.permissions.approval_policy = codex_config::Constrained::allow_any(AskForApproval::OnRequest); - app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + app.config.approvals_reviewer = ApprovalsReviewer::AutoReview; app.config.permissions.sandbox_policy = codex_config::Constrained::allow_any(SandboxPolicy::new_workspace_write_policy()); @@ -164,7 +165,7 @@ mod tests { let expected_main_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, - approvals_reviewer: ApprovalsReviewer::GuardianSubagent, + approvals_reviewer: ApprovalsReviewer::AutoReview, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), ..main_session };