From 46e2250bcfc6a789f7561a0243f20b4c98e78dcf Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 8 May 2026 13:20:05 -0700 Subject: [PATCH] [codex] Remove legacy after tool use hooks (#21805) ## Why The legacy `AfterToolUse` hook path was still wired through core tool dispatch even though the hooks registry never populated any handlers for it. The supported hook surface is `PostToolUse`, so the old infrastructure was dead code on the hot path. ## What changed - Removed the legacy `AfterToolUse` dispatch from `codex-core` tool execution. - Removed the unused legacy hook payload types and exports from `codex-hooks`. - Simplified legacy notify handling now that `HookEvent` only carries `AfterAgent`. ## Validation - `cargo test -p codex-hooks` - `cargo test -p codex-core registry` --- codex-rs/core/src/tools/registry.rs | 165 +--------------------------- codex-rs/hooks/src/legacy_notify.rs | 3 - codex-rs/hooks/src/lib.rs | 4 - codex-rs/hooks/src/registry.rs | 3 - codex-rs/hooks/src/types.rs | 140 ----------------------- 5 files changed, 3 insertions(+), 312 deletions(-) diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 89c5ea0bf7..01a3b811c5 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; -use std::time::Instant; use crate::function_tool::FunctionCallError; use crate::goals::GoalRuntimeEvent; @@ -20,13 +19,6 @@ use crate::tools::flat_tool_name; use crate::tools::hook_names::HookToolName; use crate::tools::tool_dispatch_trace::ToolDispatchTrace; use crate::util::error_or_panic; -use codex_hooks::HookEvent; -use codex_hooks::HookEventAfterToolUse; -use codex_hooks::HookPayload; -use codex_hooks::HookResult; -use codex_hooks::HookToolInput; -use codex_hooks::HookToolInputLocalShell; -use codex_hooks::HookToolKind; use codex_protocol::models::ResponseInputItem; use codex_protocol::protocol::EventMsg; use codex_tools::ConfiguredToolSpec; @@ -379,7 +371,6 @@ impl ToolRegistry { let response_cell = tokio::sync::Mutex::new(None); let invocation_for_tool = invocation.clone(); - let started = Instant::now(); let result = otel .log_tool_result_with_tags( tool_name_flat.as_ref(), @@ -411,10 +402,9 @@ impl ToolRegistry { }, ) .await; - let duration = started.elapsed(); - let (output_preview, success) = match &result { - Ok((preview, success)) => (preview.clone(), *success), - Err(err) => (err.to_string(), false), + let success = match &result { + Ok((_preview, success)) => *success, + Err(_) => false, }; emit_metric_for_tool_read(&invocation, success).await; let post_tool_use_payload = if success { @@ -441,21 +431,6 @@ impl ToolRegistry { } else { None }; - // Deprecated: this is the legacy AfterToolUse hook. Prefer the new PostToolUse - let hook_abort_error = dispatch_after_tool_use_hook(AfterToolUseHookDispatch { - invocation: &invocation, - output_preview, - success, - executed: true, - duration, - mutating: is_mutating, - }) - .await; - - if let Some(err) = hook_abort_error { - dispatch_trace.record_failed(&err); - return Err(err); - } if let Some(outcome) = &post_tool_use_outcome { record_additional_contexts( @@ -580,140 +555,6 @@ fn unsupported_tool_call_message(payload: &ToolPayload, tool_name: &ToolName) -> } } -// Hooks use a separate wire-facing input type so hook payload JSON stays stable -// and decoupled from core's internal tool runtime representation. -impl From<&ToolPayload> for HookToolInput { - fn from(payload: &ToolPayload) -> Self { - match payload { - ToolPayload::Function { arguments } => HookToolInput::Function { - arguments: arguments.clone(), - }, - ToolPayload::ToolSearch { arguments } => HookToolInput::Function { - arguments: serde_json::json!({ - "query": arguments.query, - "limit": arguments.limit, - }) - .to_string(), - }, - ToolPayload::Custom { input } => HookToolInput::Custom { - input: input.clone(), - }, - ToolPayload::LocalShell { params } => HookToolInput::LocalShell { - params: HookToolInputLocalShell { - command: params.command.clone(), - workdir: params.workdir.clone(), - timeout_ms: params.timeout_ms, - sandbox_permissions: params.sandbox_permissions, - prefix_rule: params.prefix_rule.clone(), - justification: params.justification.clone(), - }, - }, - ToolPayload::Mcp { - server, - tool, - raw_arguments, - } => HookToolInput::Mcp { - server: server.clone(), - tool: tool.clone(), - arguments: raw_arguments.clone(), - }, - } - } -} - -fn hook_tool_kind(tool_input: &HookToolInput) -> HookToolKind { - match tool_input { - HookToolInput::Function { .. } => HookToolKind::Function, - HookToolInput::Custom { .. } => HookToolKind::Custom, - HookToolInput::LocalShell { .. } => HookToolKind::LocalShell, - HookToolInput::Mcp { .. } => HookToolKind::Mcp, - } -} - -struct AfterToolUseHookDispatch<'a> { - invocation: &'a ToolInvocation, - output_preview: String, - success: bool, - executed: bool, - duration: Duration, - mutating: bool, -} - -async fn dispatch_after_tool_use_hook( - dispatch: AfterToolUseHookDispatch<'_>, -) -> Option { - let AfterToolUseHookDispatch { invocation, .. } = dispatch; - let session = invocation.session.as_ref(); - let turn = invocation.turn.as_ref(); - let tool_input = HookToolInput::from(&invocation.payload); - let tool_name = &invocation.tool_name; - let hook_tool_name = flat_tool_name(tool_name); - let hook_outcomes = session - .hooks() - .dispatch(HookPayload { - session_id: session.conversation_id, - cwd: turn.cwd.clone(), - client: turn.app_server_client_name.clone(), - triggered_at: chrono::Utc::now(), - hook_event: HookEvent::AfterToolUse { - event: HookEventAfterToolUse { - turn_id: turn.sub_id.clone(), - call_id: invocation.call_id.clone(), - tool_name: hook_tool_name.into_owned(), - tool_kind: hook_tool_kind(&tool_input), - tool_input, - executed: dispatch.executed, - success: dispatch.success, - duration_ms: u64::try_from(dispatch.duration.as_millis()).unwrap_or(u64::MAX), - mutating: dispatch.mutating, - sandbox: permission_profile_sandbox_tag( - &turn.permission_profile, - turn.windows_sandbox_level, - turn.network.is_some(), - ) - .to_string(), - sandbox_policy: permission_profile_policy_tag( - &turn.permission_profile, - turn.cwd.as_path(), - ) - .to_string(), - output_preview: dispatch.output_preview.clone(), - }, - }, - }) - .await; - - for hook_outcome in hook_outcomes { - let hook_name = hook_outcome.hook_name; - match hook_outcome.result { - HookResult::Success => {} - HookResult::FailedContinue(error) => { - warn!( - call_id = %invocation.call_id, - tool_name = %invocation.tool_name, - hook_name = %hook_name, - error = %error, - "after_tool_use hook failed; continuing" - ); - } - HookResult::FailedAbort(error) => { - warn!( - call_id = %invocation.call_id, - tool_name = %invocation.tool_name, - hook_name = %hook_name, - error = %error, - "after_tool_use hook failed; aborting operation" - ); - return Some(FunctionCallError::Fatal(format!( - "after_tool_use hook '{hook_name}' failed and aborted operation: {error}" - ))); - } - } - } - - None -} - #[cfg(test)] #[path = "registry_tests.rs"] mod tests; diff --git a/codex-rs/hooks/src/legacy_notify.rs b/codex-rs/hooks/src/legacy_notify.rs index b1cd23f877..0e273b421f 100644 --- a/codex-rs/hooks/src/legacy_notify.rs +++ b/codex-rs/hooks/src/legacy_notify.rs @@ -37,9 +37,6 @@ pub fn legacy_notify_json(payload: &HookPayload) -> Result Err(serde_json::Error::io(std::io::Error::other( - "legacy notify payload is only supported for after_agent", - ))), } } diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 7faa845077..ec59368d30 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -69,13 +69,9 @@ pub use schema::write_schema_fixtures; pub use types::Hook; pub use types::HookEvent; pub use types::HookEventAfterAgent; -pub use types::HookEventAfterToolUse; pub use types::HookPayload; pub use types::HookResponse; pub use types::HookResult; -pub use types::HookToolInput; -pub use types::HookToolInputLocalShell; -pub use types::HookToolKind; /// Returns the hook event label used in persisted hook-state keys. pub fn hook_event_key_label(event_name: HookEventName) -> &'static str { diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index 74c9a84539..8b79ee873b 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -46,7 +46,6 @@ pub struct HookListOutcome { #[derive(Clone)] pub struct Hooks { after_agent: Vec, - after_tool_use: Vec, engine: ClaudeHooksEngine, } @@ -76,7 +75,6 @@ impl Hooks { ); Self { after_agent, - after_tool_use: Vec::new(), engine, } } @@ -88,7 +86,6 @@ impl Hooks { fn hooks_for_event(&self, hook_event: &HookEvent) -> &[Hook] { match hook_event { HookEvent::AfterAgent { .. } => &self.after_agent, - HookEvent::AfterToolUse { .. } => &self.after_tool_use, } } diff --git a/codex-rs/hooks/src/types.rs b/codex-rs/hooks/src/types.rs index a01e442c30..6639308aa0 100644 --- a/codex-rs/hooks/src/types.rs +++ b/codex-rs/hooks/src/types.rs @@ -4,7 +4,6 @@ use chrono::DateTime; use chrono::SecondsFormat; use chrono::Utc; use codex_protocol::ThreadId; -use codex_protocol::models::SandboxPermissions; use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; use serde::Serialize; @@ -81,62 +80,6 @@ pub struct HookEventAfterAgent { pub last_assistant_message: Option, } -#[derive(Debug, Clone, Serialize, PartialEq, Eq)] -#[serde(rename_all = "snake_case")] -pub enum HookToolKind { - Function, - Custom, - LocalShell, - Mcp, -} - -#[derive(Debug, Clone, Serialize, PartialEq)] -#[serde(rename_all = "snake_case")] -pub struct HookToolInputLocalShell { - pub command: Vec, - pub workdir: Option, - pub timeout_ms: Option, - pub sandbox_permissions: Option, - pub prefix_rule: Option>, - pub justification: Option, -} - -#[derive(Debug, Clone, Serialize, PartialEq)] -#[serde(tag = "input_type", rename_all = "snake_case")] -pub enum HookToolInput { - Function { - arguments: String, - }, - Custom { - input: String, - }, - LocalShell { - params: HookToolInputLocalShell, - }, - Mcp { - server: String, - tool: String, - arguments: String, - }, -} - -#[derive(Debug, Clone, Serialize, PartialEq)] -#[serde(rename_all = "snake_case")] -pub struct HookEventAfterToolUse { - pub turn_id: String, - pub call_id: String, - pub tool_name: String, - pub tool_kind: HookToolKind, - pub tool_input: HookToolInput, - pub executed: bool, - pub success: bool, - pub duration_ms: u64, - pub mutating: bool, - pub sandbox: String, - pub sandbox_policy: String, - pub output_preview: String, -} - fn serialize_triggered_at(value: &DateTime, serializer: S) -> Result where S: Serializer, @@ -151,10 +94,6 @@ pub enum HookEvent { #[serde(flatten)] event: HookEventAfterAgent, }, - AfterToolUse { - #[serde(flatten)] - event: HookEventAfterToolUse, - }, } #[cfg(test)] @@ -162,7 +101,6 @@ mod tests { use chrono::TimeZone; use chrono::Utc; use codex_protocol::ThreadId; - use codex_protocol::models::SandboxPermissions; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; @@ -170,11 +108,7 @@ mod tests { use super::HookEvent; use super::HookEventAfterAgent; - use super::HookEventAfterToolUse; use super::HookPayload; - use super::HookToolInput; - use super::HookToolInputLocalShell; - use super::HookToolKind; #[test] fn hook_payload_serializes_stable_wire_shape() { @@ -215,78 +149,4 @@ mod tests { assert_eq!(actual, expected); } - - #[test] - fn after_tool_use_payload_serializes_stable_wire_shape() { - let session_id = ThreadId::new(); - let cwd = test_path_buf("/tmp").abs(); - let payload = HookPayload { - session_id, - cwd: cwd.clone(), - client: None, - triggered_at: Utc - .with_ymd_and_hms(2025, 1, 1, 0, 0, 0) - .single() - .expect("valid timestamp"), - hook_event: HookEvent::AfterToolUse { - event: HookEventAfterToolUse { - turn_id: "turn-2".to_string(), - call_id: "call-1".to_string(), - tool_name: "local_shell".to_string(), - tool_kind: HookToolKind::LocalShell, - tool_input: HookToolInput::LocalShell { - params: HookToolInputLocalShell { - command: vec!["cargo".to_string(), "fmt".to_string()], - workdir: Some("codex-rs".to_string()), - timeout_ms: Some(60_000), - sandbox_permissions: Some(SandboxPermissions::UseDefault), - justification: None, - prefix_rule: None, - }, - }, - executed: true, - success: true, - duration_ms: 42, - mutating: true, - sandbox: "none".to_string(), - sandbox_policy: "danger-full-access".to_string(), - output_preview: "ok".to_string(), - }, - }, - }; - - let actual = serde_json::to_value(payload).expect("serialize hook payload"); - let expected = json!({ - "session_id": session_id.to_string(), - "cwd": cwd.display().to_string(), - "triggered_at": "2025-01-01T00:00:00Z", - "hook_event": { - "event_type": "after_tool_use", - "turn_id": "turn-2", - "call_id": "call-1", - "tool_name": "local_shell", - "tool_kind": "local_shell", - "tool_input": { - "input_type": "local_shell", - "params": { - "command": ["cargo", "fmt"], - "workdir": "codex-rs", - "timeout_ms": 60000, - "sandbox_permissions": "use_default", - "justification": null, - "prefix_rule": null, - }, - }, - "executed": true, - "success": true, - "duration_ms": 42, - "mutating": true, - "sandbox": "none", - "sandbox_policy": "danger-full-access", - "output_preview": "ok", - }, - }); - - assert_eq!(actual, expected); - } }