From 5c20513a1b3d15898429abd92b3676b76795a892 Mon Sep 17 00:00:00 2001 From: Abhinav Date: Fri, 22 May 2026 17:56:58 -0700 Subject: [PATCH] Default function tools into tool hooks (#23757) # Why `PreToolUse`, `PostToolUse`, and `updatedInput` coverage for local function tools currently depends on each handler remembering to wire up the hook contract itself. That makes coverage easy to miss as new function tools are added, even though most of them share the same basic shape: a model-facing function call with JSON arguments. # What This makes `CoreToolRuntime` provide the default hook contract for ordinary local function tools: - build generic `PreToolUse` and `PostToolUse` payloads from the function tool name and arguments - apply `updatedInput` rewrites back into function-tool arguments through the same default path - let tool outputs override the post-hook input or response when they have a more stable hook-facing contract The exceptions stay explicit: - hosted tools remain outside the generic local function path - code-mode `wait` and `write_stdin` opt out for now - `PostToolUse` feedback replaces only the model-visible response, so code mode keeps its typed tool result With the generic path in place, the MCP and extension-tool adapters no longer need their own duplicate pre/post hook plumbing. The new coverage exercises the registry default plus end-to-end local function behavior for pre-hook blocking, `updatedInput` rewriting, and post-hook context. --- .../core/src/tools/code_mode/wait_handler.rs | 23 ++- .../src/tools/handlers/extension_tools.rs | 45 +--- codex-rs/core/src/tools/handlers/mcp.rs | 73 +------ .../handlers/unified_exec/write_stdin.rs | 10 + codex-rs/core/src/tools/hook_names.rs | 12 ++ codex-rs/core/src/tools/registry.rs | 126 ++++++++++-- codex-rs/core/src/tools/registry_tests.rs | 192 ++++++++++++++++++ codex-rs/core/tests/suite/hooks.rs | 180 ++++++++-------- 8 files changed, 437 insertions(+), 224 deletions(-) diff --git a/codex-rs/core/src/tools/code_mode/wait_handler.rs b/codex-rs/core/src/tools/code_mode/wait_handler.rs index fccf516b1..d0c0453df 100644 --- a/codex-rs/core/src/tools/code_mode/wait_handler.rs +++ b/codex-rs/core/src/tools/code_mode/wait_handler.rs @@ -2,9 +2,12 @@ use serde::Deserialize; use crate::function_tool::FunctionCallError; use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::registry::CoreToolRuntime; +use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use codex_tools::ToolName; use codex_tools::ToolSpec; @@ -110,4 +113,22 @@ impl ToolExecutor for CodeModeWaitHandler { } } -impl CoreToolRuntime for CodeModeWaitHandler {} +impl CoreToolRuntime for CodeModeWaitHandler { + fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { + // Code-mode `wait` is runtime control for an existing code cell, not a + // standalone user action. Tool calls made from code mode still flow + // through normal dispatch, but hooks should not block or rewrite the + // wait loop itself. + None + } + + fn post_tool_use_payload( + &self, + _invocation: &ToolInvocation, + _result: &dyn ToolOutput, + ) -> Option { + // The wait result feeds code-mode control flow, so do not let + // PostToolUse replace it with model-facing hook feedback. + None + } +} diff --git a/codex-rs/core/src/tools/handlers/extension_tools.rs b/codex-rs/core/src/tools/handlers/extension_tools.rs index 470b32bea..8c9f55c46 100644 --- a/codex-rs/core/src/tools/handlers/extension_tools.rs +++ b/codex-rs/core/src/tools/handlers/extension_tools.rs @@ -4,17 +4,12 @@ use codex_tools::ConversationHistory; use codex_tools::ToolCall as ExtensionToolCall; use codex_tools::ToolName; use codex_tools::ToolSpec; -use serde_json::Value; use crate::function_tool::FunctionCallError; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; -use crate::tools::flat_tool_name; -use crate::tools::hook_names::HookToolName; use crate::tools::registry::CoreToolRuntime; -use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; pub(crate) struct ExtensionToolAdapter(Arc>); @@ -23,13 +18,6 @@ impl ExtensionToolAdapter { pub(crate) fn new(executor: Arc>) -> Self { Self(executor) } - - fn arguments_from_payload<'a>(&self, payload: &'a ToolPayload) -> Option<&'a str> { - let ToolPayload::Function { arguments } = payload else { - return None; - }; - Some(arguments) - } } #[async_trait::async_trait] @@ -60,30 +48,7 @@ impl ToolExecutor for ExtensionToolAdapter { impl CoreToolRuntime for ExtensionToolAdapter { fn matches_kind(&self, payload: &ToolPayload) -> bool { - self.arguments_from_payload(payload).is_some() - } - - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let arguments = self.arguments_from_payload(&invocation.payload)?; - Some(PreToolUsePayload { - tool_name: HookToolName::new(flat_tool_name(&self.tool_name()).into_owned()), - tool_input: extension_tool_hook_input(arguments), - }) - } - - fn post_tool_use_payload( - &self, - invocation: &ToolInvocation, - result: &dyn ToolOutput, - ) -> Option { - let arguments = self.arguments_from_payload(&invocation.payload)?; - Some(PostToolUsePayload { - tool_name: HookToolName::new(flat_tool_name(&self.tool_name()).into_owned()), - tool_use_id: invocation.call_id.clone(), - tool_input: extension_tool_hook_input(arguments), - tool_response: result - .post_tool_use_response(&invocation.call_id, &invocation.payload)?, - }) + matches!(payload, ToolPayload::Function { .. }) } } @@ -100,14 +65,6 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall { } } -fn extension_tool_hook_input(arguments: &str) -> Value { - if arguments.trim().is_empty() { - return Value::Object(serde_json::Map::new()); - } - - serde_json::from_str(arguments).unwrap_or_else(|_| Value::String(arguments.to_string())) -} - #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 5f6261323..0d5c15827 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -9,10 +9,7 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::flat_tool_name; -use crate::tools::hook_names::HookToolName; use crate::tools::registry::CoreToolRuntime; -use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use crate::tools::registry::ToolTelemetryTags; use crate::tools::tool_search_entry::ToolSearchInfo; @@ -23,8 +20,6 @@ use codex_tools::ToolName; use codex_tools::ToolSearchSourceInfo; use codex_tools::ToolSpec; use codex_tools::mcp_tool_to_responses_api_tool; -use serde_json::Map; -use serde_json::Value; pub struct McpHandler { tool_info: ToolInfo, @@ -143,58 +138,6 @@ impl CoreToolRuntime for McpHandler { tags }) } - - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let ToolPayload::Function { arguments } = &invocation.payload else { - return None; - }; - - Some(PreToolUsePayload { - tool_name: HookToolName::new(self.tool_name().to_string()), - tool_input: mcp_hook_tool_input(arguments), - }) - } - - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: Value, - ) -> Result { - invocation.payload = match invocation.payload { - ToolPayload::Function { .. } => ToolPayload::Function { - arguments: serde_json::to_string(&updated_input).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten MCP arguments: {err}" - )) - })?, - }, - payload => { - return Err(FunctionCallError::RespondToModel(format!( - "tool {} does not support hook input rewriting for payload {payload:?}", - self.tool_name() - ))); - } - }; - Ok(invocation) - } - fn post_tool_use_payload( - &self, - invocation: &ToolInvocation, - result: &dyn crate::tools::context::ToolOutput, - ) -> Option { - let ToolPayload::Function { .. } = &invocation.payload else { - return None; - }; - - let tool_response = - result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; - Some(PostToolUsePayload { - tool_name: HookToolName::new(self.tool_name().to_string()), - tool_use_id: invocation.call_id.clone(), - tool_input: result.post_tool_use_input(&invocation.payload)?, - tool_response, - }) - } } fn create_tool_spec(tool_info: &ToolInfo) -> Result { @@ -223,14 +166,6 @@ fn create_tool_spec(tool_info: &ToolInfo) -> Result })) } -fn mcp_hook_tool_input(raw_arguments: &str) -> Value { - if raw_arguments.trim().is_empty() { - return Value::Object(Map::new()); - } - - serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) -} - fn build_mcp_search_text(info: &ToolInfo) -> String { let tool_name = info.canonical_tool_name(); let mut schema_properties = info @@ -288,6 +223,9 @@ mod tests { use super::*; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolCallSource; + use crate::tools::hook_names::HookToolName; + use crate::tools::registry::PostToolUsePayload; + use crate::tools::registry::PreToolUsePayload; use crate::turn_diff_tracker::TurnDiffTracker; use pretty_assertions::assert_eq; use serde_json::json; @@ -447,11 +385,6 @@ mod tests { ); } - #[test] - fn mcp_hook_tool_input_defaults_empty_args_to_object() { - assert_eq!(mcp_hook_tool_input(" "), json!({})); - } - #[test] fn mcp_read_only_hint_supports_parallel_calls_without_server_opt_in() { let mut read_only_info = tool_info("foo", "mcp__foo__", "read"); diff --git a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs index dfa4240eb..a639ea006 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs @@ -5,6 +5,7 @@ use crate::tools::context::boxed_tool_output; use crate::tools::handlers::parse_arguments; use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use crate::unified_exec::WriteStdinRequest; use codex_protocol::protocol::EventMsg; @@ -101,11 +102,20 @@ impl CoreToolRuntime for WriteStdinHandler { matches!(payload, ToolPayload::Function { .. }) } + fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { + // `write_stdin` is transport for an existing exec session. Empty writes + // are background polls, and non-empty writes continue a command that + // already ran PreToolUse as Bash, so do not emit a second pre hook here. + None + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, result: &dyn crate::tools::context::ToolOutput, ) -> Option { + // A `write_stdin` poll can observe final completion for the original + // `exec_command`; emit that command's matching Bash PostToolUse. post_unified_exec_tool_use_payload(invocation, result) } } diff --git a/codex-rs/core/src/tools/hook_names.rs b/codex-rs/core/src/tools/hook_names.rs index 9d3b6c240..92ebe8aa5 100644 --- a/codex-rs/core/src/tools/hook_names.rs +++ b/codex-rs/core/src/tools/hook_names.rs @@ -38,6 +38,18 @@ impl HookToolName { } } + /// Returns the hook identity for spawning sub-agents. + /// + /// The serialized name remains `spawn_agent`, while `Agent` is accepted as + /// a matcher alias for compatibility with hook configurations that describe + /// sub-agent creation using Claude Code-style names. + pub(crate) fn spawn_agent() -> Self { + Self { + name: "spawn_agent".to_string(), + matcher_aliases: vec!["Agent".to_string()], + } + } + /// Returns the hook identity historically used for shell-like tools. pub(crate) fn bash() -> Self { Self::new("Bash") diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 411ce1435..a08d5c783 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -19,6 +19,7 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::flat_tool_name; +use crate::tools::handlers::multi_agents_spec::MULTI_AGENT_V1_NAMESPACE; use crate::tools::hook_names::HookToolName; use crate::tools::lifecycle::notify_tool_finish; use crate::tools::lifecycle::notify_tool_start; @@ -26,6 +27,7 @@ use crate::tools::tool_dispatch_trace::ToolDispatchTrace; use crate::tools::tool_search_entry::ToolSearchInfo; use crate::util::error_or_panic; use codex_extension_api::ToolCallOutcome; +use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ResponseInputItem; use codex_protocol::protocol::EventMsg; use codex_tools::ToolName; @@ -64,14 +66,47 @@ pub(crate) trait CoreToolRuntime: ToolExecutor { fn post_tool_use_payload( &self, - _invocation: &ToolInvocation, - _result: &dyn ToolOutput, + invocation: &ToolInvocation, + result: &dyn ToolOutput, ) -> Option { - None + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + + Some(PostToolUsePayload { + tool_name: function_hook_tool_name(invocation), + tool_use_id: result.post_tool_use_id(&invocation.call_id), + tool_input: result + .post_tool_use_input(&invocation.payload) + .unwrap_or_else(|| function_hook_tool_input(arguments)), + tool_response: result + .post_tool_use_response(&invocation.call_id, &invocation.payload) + .or_else(|| { + // Most function tools can expose their model-facing output + // as the hook response. Outputs with a more stable hook + // contract should override post_tool_use_response above. + let ResponseInputItem::FunctionCallOutput { + output: FunctionCallOutputPayload { body, .. }, + .. + } = result.to_response_item(&invocation.call_id, &invocation.payload) + else { + return None; + }; + + serde_json::to_value(body).ok() + })?, + }) } - fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { - None + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + + Some(PreToolUsePayload { + tool_name: function_hook_tool_name(invocation), + tool_input: function_hook_tool_input(arguments), + }) } /// Rebuilds a tool invocation from hook-facing `tool_input`. @@ -80,12 +115,25 @@ pub(crate) trait CoreToolRuntime: ToolExecutor { /// hook contract they expose from `pre_tool_use_payload`. fn with_updated_hook_input( &self, - _invocation: ToolInvocation, - _updated_input: Value, + invocation: ToolInvocation, + updated_input: Value, ) -> Result { - Err(FunctionCallError::RespondToModel( - "tool does not support hook input rewriting".to_string(), - )) + let ToolPayload::Function { .. } = &invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported function tool payload".to_string(), + )); + }; + + let arguments = serde_json::to_string(&updated_input).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten {} arguments: {err}", + flat_tool_name(&invocation.tool_name) + )) + })?; + Ok(ToolInvocation { + payload: ToolPayload::Function { arguments }, + ..invocation + }) } /// Creates an optional consumer for streamed tool argument diffs. @@ -133,6 +181,29 @@ impl AnyToolResult { } } +struct PostToolUseFeedbackOutput { + original: Box, + model_visible: FunctionToolOutput, +} + +impl ToolOutput for PostToolUseFeedbackOutput { + fn log_preview(&self) -> String { + self.original.log_preview() + } + + fn success_for_logging(&self) -> bool { + self.original.success_for_logging() + } + + fn to_response_item(&self, call_id: &str, payload: &ToolPayload) -> ResponseInputItem { + self.model_visible.to_response_item(call_id, payload) + } + + fn code_mode_result(&self, payload: &ToolPayload) -> Value { + self.original.code_mode_result(payload) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct PreToolUsePayload { /// Hook-facing tool name model. @@ -538,11 +609,15 @@ impl ToolRegistry { }; if let Some(replacement_text) = replacement_text { let mut guard = response_cell.lock().await; - if let Some(result) = guard.as_mut() { - result.result = Box::new(FunctionToolOutput::from_text( - replacement_text, - /*success*/ None, - )); + if let Some(mut result) = guard.take() { + result.result = Box::new(PostToolUseFeedbackOutput { + original: result.result, + model_visible: FunctionToolOutput::from_text( + replacement_text, + /*success*/ None, + ), + }); + *guard = Some(result); } } } @@ -618,6 +693,27 @@ async fn handle_any_tool( }) } +fn function_hook_tool_name(invocation: &ToolInvocation) -> HookToolName { + if invocation.tool_name.name == "spawn_agent" + && matches!( + invocation.tool_name.namespace.as_deref(), + None | Some(MULTI_AGENT_V1_NAMESPACE) + ) + { + return HookToolName::spawn_agent(); + } + + HookToolName::new(flat_tool_name(&invocation.tool_name).into_owned()) +} + +fn function_hook_tool_input(arguments: &str) -> Value { + if arguments.trim().is_empty() { + return Value::Object(serde_json::Map::new()); + } + + serde_json::from_str(arguments).unwrap_or_else(|_| Value::String(arguments.to_string())) +} + fn unsupported_tool_call_message(payload: &ToolPayload, tool_name: &ToolName) -> String { match payload { ToolPayload::Custom { .. } => format!("unsupported custom tool call: {tool_name}"), diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index 8517730d3..f2566251c 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -172,6 +172,198 @@ fn handler_looks_up_namespaced_aliases_explicitly() { ); } +#[tokio::test] +async fn function_tools_expose_default_hook_payloads_and_rewrites() -> anyhow::Result<()> { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let tool_name = codex_tools::ToolName::namespaced("functions.", "echo"); + let handler = TestHandler { + tool_name: tool_name.clone(), + }; + let invocation = ToolInvocation { + payload: ToolPayload::Function { + arguments: serde_json::json!({ "message": "hello" }).to_string(), + }, + ..test_invocation(Arc::new(session), Arc::new(turn), "call-1", tool_name) + }; + let output = + crate::tools::context::FunctionToolOutput::from_text("echoed".to_string(), Some(true)); + + assert_eq!( + handler.pre_tool_use_payload(&invocation), + Some(PreToolUsePayload { + tool_name: HookToolName::new("functions.echo"), + tool_input: serde_json::json!({ "message": "hello" }), + }) + ); + assert_eq!( + handler.post_tool_use_payload(&invocation, &output), + Some(PostToolUsePayload { + tool_name: HookToolName::new("functions.echo"), + tool_use_id: "call-1".to_string(), + tool_input: serde_json::json!({ "message": "hello" }), + tool_response: serde_json::json!("echoed"), + }) + ); + + let invocation = handler + .with_updated_hook_input(invocation, serde_json::json!({ "message": "rewritten" }))?; + let ToolPayload::Function { arguments } = invocation.payload else { + panic!("generic rewritten function payload should remain function-shaped"); + }; + assert_eq!( + serde_json::from_str::(&arguments)?, + serde_json::json!({ "message": "rewritten" }) + ); + + Ok(()) +} + +#[tokio::test] +async fn function_hook_input_defaults_empty_arguments_to_object() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let tool_name = codex_tools::ToolName::plain("echo"); + let handler = TestHandler { + tool_name: tool_name.clone(), + }; + let invocation = ToolInvocation { + payload: ToolPayload::Function { + arguments: " ".to_string(), + }, + ..test_invocation(Arc::new(session), Arc::new(turn), "call-1", tool_name) + }; + + assert_eq!( + handler.pre_tool_use_payload(&invocation), + Some(PreToolUsePayload { + tool_name: HookToolName::new("echo"), + tool_input: serde_json::json!({}), + }) + ); +} + +#[tokio::test] +async fn spawn_agent_function_tools_use_agent_matcher_alias() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let session = Arc::new(session); + let turn = Arc::new(turn); + + let hook_payloads = [ + codex_tools::ToolName::plain("spawn_agent"), + codex_tools::ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "spawn_agent"), + ] + .into_iter() + .map(|tool_name| { + let handler = TestHandler { + tool_name: tool_name.clone(), + }; + let invocation = ToolInvocation { + payload: ToolPayload::Function { + arguments: serde_json::json!({ "message": "inspect this repo" }).to_string(), + }, + ..test_invocation(Arc::clone(&session), Arc::clone(&turn), "call-1", tool_name) + }; + handler.pre_tool_use_payload(&invocation) + }) + .collect::>(); + + assert_eq!( + hook_payloads, + vec![ + Some(PreToolUsePayload { + tool_name: HookToolName::spawn_agent(), + tool_input: serde_json::json!({ "message": "inspect this repo" }), + }), + Some(PreToolUsePayload { + tool_name: HookToolName::spawn_agent(), + tool_input: serde_json::json!({ "message": "inspect this repo" }), + }), + ] + ); +} + +#[tokio::test] +async fn code_mode_wait_does_not_expose_default_hook_payloads() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let output = crate::tools::context::FunctionToolOutput::from_text("ok".to_string(), Some(true)); + + let wait = crate::tools::handlers::CodeModeWaitHandler; + let wait_invocation = test_invocation( + Arc::new(session), + Arc::new(turn), + "wait-call", + wait.tool_name(), + ); + assert_eq!(wait.pre_tool_use_payload(&wait_invocation), None); + assert_eq!(wait.post_tool_use_payload(&wait_invocation, &output), None); +} + +#[tokio::test] +async fn write_stdin_does_not_expose_default_pre_tool_use_payload() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + + let write_stdin = crate::tools::handlers::WriteStdinHandler; + let invocation = test_invocation( + Arc::new(session), + Arc::new(turn), + "write-stdin-call", + write_stdin.tool_name(), + ); + + assert_eq!(write_stdin.pre_tool_use_payload(&invocation), None); +} + +#[test] +fn post_tool_use_feedback_output_keeps_code_mode_result_typed() { + let result = AnyToolResult { + call_id: "call-1".to_string(), + payload: ToolPayload::Function { + arguments: "{}".to_string(), + }, + result: Box::new(PostToolUseFeedbackOutput { + original: Box::new(codex_tools::JsonToolOutput::new( + serde_json::json!({ "typed": true }), + )), + model_visible: crate::tools::context::FunctionToolOutput::from_text( + "hook feedback".to_string(), + /*success*/ None, + ), + }), + post_tool_use_payload: None, + }; + + assert_eq!( + result.into_response(), + ResponseInputItem::FunctionCallOutput { + call_id: "call-1".to_string(), + output: codex_protocol::models::FunctionCallOutputPayload::from_text( + "hook feedback".to_string() + ), + } + ); + + let result = AnyToolResult { + call_id: "call-1".to_string(), + payload: ToolPayload::Function { + arguments: "{}".to_string(), + }, + result: Box::new(PostToolUseFeedbackOutput { + original: Box::new(codex_tools::JsonToolOutput::new( + serde_json::json!({ "typed": true }), + )), + model_visible: crate::tools::context::FunctionToolOutput::from_text( + "hook feedback".to_string(), + /*success*/ None, + ), + }), + post_tool_use_payload: None, + }; + + assert_eq!( + result.code_mode_result(), + serde_json::json!({ "typed": true }) + ); +} + #[tokio::test] async fn dispatch_notifies_tool_lifecycle_contributors() -> anyhow::Result<()> { let (mut session, turn) = crate::session::tests::make_session_and_context().await; diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index ee1826ceb..379174655 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -3463,42 +3463,35 @@ async fn pre_tool_use_blocks_apply_patch_with_write_alias() -> Result<()> { } #[tokio::test] -async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { +async fn pre_tool_use_blocks_local_function_tool_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let call_id = "pretooluse-update-plan"; - let args = serde_json::json!({ - "plan": [{ - "step": "watch the tide", - "status": "pending", - }] - }); + let call_id = "pretooluse-local-function-tool"; + let args = serde_json::json!({}); let responses = mount_sse_sequence( &server, vec![ sse(vec![ ev_response_created("resp-1"), - core_test_support::responses::ev_function_call( - call_id, - "update_plan", - &serde_json::to_string(&args)?, - ), + ev_function_call(call_id, "test_sync_tool", &serde_json::to_string(&args)?), ev_completed("resp-1"), ]), sse(vec![ ev_response_created("resp-2"), - ev_assistant_message("msg-1", "plan updated"), + ev_assistant_message("msg-1", "local function hook blocked it"), ev_completed("resp-2"), ]), ], ) .await; + let reason = "blocked local function pre hook"; let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") .with_pre_build_hook(|home| { if let Err(error) = - write_pre_tool_use_hook(home, /*matcher*/ None, "json_deny", "should not fire") + write_pre_tool_use_hook(home, Some("^test_sync_tool$"), "json_deny", reason) { panic!("failed to write pre tool use hook test fixture: {error}"); } @@ -3506,7 +3499,8 @@ async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; - test.submit_turn("update the plan").await?; + test.submit_turn("call the local function tool with the pre hook") + .await?; let requests = responses.requests(); assert_eq!(requests.len(), 2); @@ -3514,17 +3508,85 @@ async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { let output = output_item .get("output") .and_then(Value::as_str) - .expect("update plan output string"); + .expect("blocked local function tool output string"); assert!( - !output.contains("should not fire"), - "non-shell tool output should not be blocked by PreToolUse", + output.contains(&format!( + "Tool call blocked by PreToolUse hook: {reason}. Tool: test_sync_tool" + )), + "blocked local function output should surface the hook reason and tool name", ); - let hook_log_path = test.codex_home_path().join("pre_tool_use_hook_log.jsonl"); - assert!( - !hook_log_path.exists(), - "plan tool should not trigger pre tool use hooks", - ); + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["hook_event_name"], "PreToolUse"); + assert_eq!(hook_inputs[0]["tool_name"], "test_sync_tool"); + assert_eq!(hook_inputs[0]["tool_use_id"], call_id); + assert_eq!(hook_inputs[0]["tool_input"], args); + + Ok(()) +} + +#[tokio::test] +async fn pre_tool_use_rewrites_local_function_tool_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-local-function-tool-rewrite"; + let original_args = serde_json::json!({ + "barrier": { + "id": "pretooluse-local-function-invalid-barrier", + "participants": 0, + } + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + call_id, + "test_sync_tool", + &serde_json::to_string(&original_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "local function hook rewrote it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({}); + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_pre_build_hook(move |home| { + if let Err(error) = + write_updating_pre_tool_use_hook(home, "^test_sync_tool$", &updated_input) + { + panic!("failed to write updating pre tool use hook test fixture: {error}"); + } + }) + .with_config(trust_discovered_hooks); + let test = builder.build(&server).await?; + + test.submit_turn("call the local function tool with the pre hook rewrite") + .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("rewritten local function tool output string"); + assert_eq!(output, "ok"); + + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["tool_input"], original_args); Ok(()) } @@ -4133,73 +4195,3 @@ async fn post_tool_use_records_apply_patch_context_with_edit_alias() -> Result<( Ok(()) } - -#[tokio::test] -async fn post_tool_use_does_not_fire_for_plan_tool() -> Result<()> { - skip_if_no_network!(Ok(())); - - let server = start_mock_server().await; - let call_id = "posttooluse-update-plan"; - let args = serde_json::json!({ - "plan": [{ - "step": "watch the tide", - "status": "pending", - }] - }); - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-1"), - core_test_support::responses::ev_function_call( - call_id, - "update_plan", - &serde_json::to_string(&args)?, - ), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "plan updated"), - ev_completed("resp-2"), - ]), - ], - ) - .await; - - let mut builder = test_codex() - .with_pre_build_hook(|home| { - if let Err(error) = write_post_tool_use_hook( - home, - /*matcher*/ None, - "decision_block", - "should not fire", - ) { - panic!("failed to write post tool use hook test fixture: {error}"); - } - }) - .with_config(trust_discovered_hooks); - let test = builder.build(&server).await?; - - test.submit_turn("update the plan").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("update plan output string"); - assert!( - !output.contains("should not fire"), - "non-shell tool output should not be affected by PostToolUse", - ); - - let hook_log_path = test.codex_home_path().join("post_tool_use_hook_log.jsonl"); - assert!( - !hook_log_path.exists(), - "plan tool should not trigger post tool use hooks", - ); - - Ok(()) -}