mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[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`
This commit is contained in:
committed by
GitHub
Unverified
parent
e783341b70
commit
46e2250bcf
@@ -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<FunctionCallError> {
|
||||
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;
|
||||
|
||||
@@ -37,9 +37,6 @@ pub fn legacy_notify_json(payload: &HookPayload) -> Result<String, serde_json::E
|
||||
last_assistant_message: event.last_assistant_message.clone(),
|
||||
})
|
||||
}
|
||||
HookEvent::AfterToolUse { .. } => Err(serde_json::Error::io(std::io::Error::other(
|
||||
"legacy notify payload is only supported for after_agent",
|
||||
))),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -46,7 +46,6 @@ pub struct HookListOutcome {
|
||||
#[derive(Clone)]
|
||||
pub struct Hooks {
|
||||
after_agent: Vec<Hook>,
|
||||
after_tool_use: Vec<Hook>,
|
||||
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,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<String>,
|
||||
}
|
||||
|
||||
#[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<String>,
|
||||
pub workdir: Option<String>,
|
||||
pub timeout_ms: Option<u64>,
|
||||
pub sandbox_permissions: Option<SandboxPermissions>,
|
||||
pub prefix_rule: Option<Vec<String>>,
|
||||
pub justification: Option<String>,
|
||||
}
|
||||
|
||||
#[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<S>(value: &DateTime<Utc>, serializer: S) -> Result<S::Ok, S::Error>
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user