From 493cd354ff6fb7a37af2b4ed89d1ddccdb5859c1 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 3 Jun 2026 18:33:18 -0700 Subject: [PATCH] codex: add interrupt reasons and shutdown-bounded hooks (#26259) --- codex-rs/app-server/README.md | 8 +- codex-rs/core/src/guardian/review.rs | 2 +- codex-rs/core/src/hook_runtime.rs | 15 +- codex-rs/core/src/session/handlers.rs | 3 +- codex-rs/core/src/session/mod.rs | 3 +- codex-rs/core/src/tasks/mod.rs | 73 ++++++++-- codex-rs/core/tests/suite/interrupt_hooks.rs | 125 +++++++++++++++++ .../interrupt.command.input.schema.json | 9 ++ codex-rs/hooks/src/engine/command_runner.rs | 128 +++++++++++++++++- codex-rs/hooks/src/engine/dispatcher.rs | 64 +++++++-- codex-rs/hooks/src/engine/mod.rs | 8 +- codex-rs/hooks/src/events/common.rs | 13 +- codex-rs/hooks/src/events/interrupt.rs | 31 ++++- codex-rs/hooks/src/lib.rs | 5 +- codex-rs/hooks/src/registry.rs | 8 +- codex-rs/hooks/src/schema.rs | 16 +++ 16 files changed, 466 insertions(+), 45 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index dc02ff8c04..d8375e00d0 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -230,7 +230,7 @@ Example with notification opt-out: - `externalAgentConfig/import` — apply selected external-agent migration items by passing explicit `migrationItems` with `cwd` (`null` for home) and any plugin/session `details` returned by detect. When a request includes migration items, the server emits `externalAgentConfig/import/completed` once after the full import finishes (immediately after the response when everything completed synchronously, or after background imports finish). - `config/value/write` — write a single config key/value to the user's config.toml on disk; dotted paths such as `desktop.someKey` use the same generic write surface. - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits. -- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`, `allowedPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), computer use policy (`computerUse`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. +- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`, `allowedPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), computer use policy (`computerUse`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`, keyed by canonical event names such as `PreToolUse`, `Stop`, and `Interrupt`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. ### Example: Start or resume a thread @@ -1593,6 +1593,8 @@ For linked Git worktrees, project hook declarations come from the matching `.cod Hooks are returned even when disabled so clients can render and re-enable them. User-controlled state lives under `hooks.state`. Managed hooks are non-configurable, and user entries for managed hook keys are ignored during loading. +The returned `eventName` covers the full discovered hook surface, including `Interrupt` entries alongside the existing tool, compaction, session-start, and stop events. + For unmanaged hooks, `currentHash` and `trustStatus` describe whether the current definition is first-seen, approved, or changed since approval. Only trusted unmanaged hooks become runnable. Hook keys combine the source identity with a trailing event/group/handler selector that is currently positional. ```json @@ -1730,8 +1732,8 @@ approvals_reviewer = "user" ``` Setting the app value to `"user"` routes its approval prompts to the user -instead of Guardian; setting it to `"auto_review"` opts that app into Guardian -review when allowed by configuration requirements. +instead of `auto_review`; setting it to `"auto_review"` opts that app into +`auto_review` review when allowed by configuration requirements. Invoke an app by inserting `$` in the text input. The slug is derived from the app name and lowercased with non-alphanumeric characters replaced by `-` (for example, "Demo App" becomes `$demo-app`). Add a `mention` input item (recommended) so the server uses the exact `app://` path rather than guessing by name. Plugins use the same `mention` item shape, but with `plugin://@` paths from `plugin/installed` or `plugin/list`. diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 06d1a78931..dea07f4116 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -234,7 +234,7 @@ async fn record_guardian_denial(session: &Arc, turn: &Arc, let turn_id = turn_id.to_string(); let _abort_task = runtime_handle.spawn(async move { session - .abort_turn_if_active(&turn_id, TurnAbortReason::Interrupted) + .abort_turn_if_active_interrupted(&turn_id, codex_hooks::InterruptReason::AutoReview) .await; }); } diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 0d3f9c9fdb..081aff1cdc 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -5,6 +5,8 @@ use std::time::Duration; use codex_analytics::CompactionTrigger; use codex_analytics::HookRunFact; use codex_analytics::build_track_events_context; +use codex_hooks::CommandExecutionPolicy; +use codex_hooks::InterruptReason; use codex_hooks::InterruptRequest; use codex_hooks::PermissionRequestDecision; use codex_hooks::PermissionRequestOutcome; @@ -362,7 +364,11 @@ pub(crate) async fn run_turn_stop_hooks( outcome } -pub(crate) async fn run_turn_interrupt_hooks(sess: &Arc, turn_context: &Arc) { +pub(crate) async fn run_turn_interrupt_hooks( + sess: &Arc, + turn_context: &Arc, + reason: InterruptReason, +) { if matches!(&turn_context.session_source, SessionSource::SubAgent(_)) { return; } @@ -375,11 +381,16 @@ pub(crate) async fn run_turn_interrupt_hooks(sess: &Arc, turn_context: transcript_path: sess.hook_transcript_path().await, model: turn_context.model_info.slug.clone(), permission_mode: hook_permission_mode(turn_context), + reason, }; let hooks = sess.hooks(); emit_hook_started_events(sess, turn_context, hooks.preview_interrupt(&request)).await; - let outcome = hooks.run_interrupt(request).await; + let execution_policy = match reason { + InterruptReason::Shutdown => CommandExecutionPolicy::shutdown_bounded(), + InterruptReason::User | InterruptReason::AutoReview => CommandExecutionPolicy::Standard, + }; + let outcome = hooks.run_interrupt(request, execution_policy).await; emit_hook_completed_events(sess, turn_context, outcome.hook_events).await; } diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index 7730a30ad7..d89c199321 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -624,7 +624,8 @@ pub async fn set_thread_memory_mode(sess: &Arc, sub_id: String, mode: T } async fn shutdown_session_runtime(sess: &Arc) { - sess.abort_all_tasks(TurnAbortReason::Interrupted).await; + sess.abort_all_tasks_interrupted(codex_hooks::InterruptReason::Shutdown) + .await; let _ = sess.conversation.shutdown().await; sess.services .unified_exec_manager diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 8ce16de740..9a29cee46c 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3298,7 +3298,8 @@ impl Session { pub async fn interrupt_task(self: &Arc) { info!("interrupt received: abort current task, if any"); let had_active_turn = self.active_turn.lock().await.is_some(); - self.abort_all_tasks(TurnAbortReason::Interrupted).await; + self.abort_all_tasks_interrupted(codex_hooks::InterruptReason::User) + .await; if !had_active_turn { self.cancel_mcp_startup().await; } diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index 7096aa1568..a0150c61f1 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -9,6 +9,7 @@ use std::time::Duration; use std::time::Instant; use codex_extension_api::ExtensionData; +use codex_hooks::InterruptReason; use futures::future::BoxFuture; use tokio::select; use tokio::sync::Notify; @@ -64,6 +65,35 @@ pub(crate) use user_shell::execute_user_shell_command; const GRACEFULL_INTERRUPTION_TIMEOUT_MS: u64 = 100; const TASK_COMPACT_METRIC: &str = "codex.task.compact"; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TaskAbort { + Standard(TurnAbortReason), + Interrupted(InterruptReason), +} + +impl TaskAbort { + fn from_turn_abort_reason(reason: TurnAbortReason) -> Self { + match reason { + TurnAbortReason::Interrupted => Self::Interrupted(InterruptReason::User), + other => Self::Standard(other), + } + } + + fn turn_abort_reason(self) -> TurnAbortReason { + match self { + Self::Standard(reason) => reason, + Self::Interrupted(_) => TurnAbortReason::Interrupted, + } + } + + fn interrupt_reason(self) -> Option { + match self { + Self::Interrupted(reason) => Some(reason), + Self::Standard(_) => None, + } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum InterruptedTurnHistoryMarker { Disabled, @@ -485,6 +515,17 @@ impl Session { } pub async fn abort_all_tasks(self: &Arc, reason: TurnAbortReason) { + self.abort_all_tasks_with(TaskAbort::from_turn_abort_reason(reason)) + .await; + } + + pub async fn abort_all_tasks_interrupted(self: &Arc, origin: InterruptReason) { + self.abort_all_tasks_with(TaskAbort::Interrupted(origin)) + .await; + } + + async fn abort_all_tasks_with(self: &Arc, abort: TaskAbort) { + let reason = abort.turn_abort_reason(); let mut aborted_turn = false; let mut active_turn_to_clear = None; let mut turn_context = None; @@ -493,7 +534,7 @@ impl Session { aborted_turn = task.is_some(); turn_context = task.as_ref().map(|task| Arc::clone(&task.turn_context)); if let Some(task) = task { - self.handle_task_abort(task, reason.clone()).await; + self.handle_task_abort(task, abort).await; } if aborted_turn { active_turn_to_clear = Some(active_turn); @@ -518,7 +559,7 @@ impl Session { // in-flight approval wait can surface as a model-visible rejection before TurnAborted. self.input_queue.clear_pending(&active_turn).await; } - if reason == TurnAbortReason::Interrupted && aborted_turn { + if abort.interrupt_reason().is_some() && aborted_turn { self.maybe_start_turn_for_pending_work().await; } } @@ -528,6 +569,21 @@ impl Session { turn_id: &str, reason: TurnAbortReason, ) -> bool { + self.abort_turn_if_active_with(turn_id, TaskAbort::from_turn_abort_reason(reason)) + .await + } + + pub(crate) async fn abort_turn_if_active_interrupted( + self: &Arc, + turn_id: &str, + origin: InterruptReason, + ) -> bool { + self.abort_turn_if_active_with(turn_id, TaskAbort::Interrupted(origin)) + .await + } + + async fn abort_turn_if_active_with(self: &Arc, turn_id: &str, abort: TaskAbort) -> bool { + let reason = abort.turn_abort_reason(); let active_turn = { let mut active = self.active_turn.lock().await; if active @@ -547,7 +603,7 @@ impl Session { let task = active_turn.task.take(); let turn_context = task.as_ref().map(|task| Arc::clone(&task.turn_context)); if let Some(task) = task { - self.handle_task_abort(task, reason.clone()).await; + self.handle_task_abort(task, abort).await; } if let Some(turn_context) = turn_context.as_deref() { self.emit_turn_abort_lifecycle(reason.clone(), turn_context.extension_data.as_ref()) @@ -565,7 +621,7 @@ impl Session { // in-flight approval wait can surface as a model-visible rejection before TurnAborted. self.input_queue.clear_pending(&active_turn).await; - if reason == TurnAbortReason::Interrupted { + if abort.interrupt_reason().is_some() { self.maybe_start_turn_for_pending_work().await; } @@ -813,7 +869,8 @@ impl Session { .await; } - async fn handle_task_abort(self: &Arc, task: RunningTask, reason: TurnAbortReason) { + async fn handle_task_abort(self: &Arc, task: RunningTask, abort: TaskAbort) { + let reason = abort.turn_abort_reason(); let sub_id = task.turn_context.sub_id.clone(); if task.cancellation_token.is_cancelled() { return; @@ -844,7 +901,7 @@ impl Session { .abort(session_ctx, Arc::clone(&task.turn_context)) .await; - if reason == TurnAbortReason::Interrupted + if abort.interrupt_reason().is_some() && let Some(marker) = interrupted_turn_history_marker( InterruptedTurnHistoryMarker::from_config_and_version( task.turn_context.config.as_ref(), @@ -864,8 +921,8 @@ impl Session { } } - if reason == TurnAbortReason::Interrupted { - run_turn_interrupt_hooks(self, &task.turn_context).await; + if let Some(interrupt_reason) = abort.interrupt_reason() { + run_turn_interrupt_hooks(self, &task.turn_context, interrupt_reason).await; } let (completed_at, duration_ms) = task diff --git a/codex-rs/core/tests/suite/interrupt_hooks.rs b/codex-rs/core/tests/suite/interrupt_hooks.rs index 35676b5e0b..b9f6b25d9e 100644 --- a/codex-rs/core/tests/suite/interrupt_hooks.rs +++ b/codex-rs/core/tests/suite/interrupt_hooks.rs @@ -1,5 +1,6 @@ use std::fs; use std::path::Path; +use std::sync::Arc; use std::time::Duration; use anyhow::Context; @@ -46,6 +47,14 @@ use tokio::time::sleep; use tokio::time::timeout; fn write_interrupt_hook(home: &Path, system_message: Option<&str>) -> Result<()> { + write_interrupt_hook_with_sleep(home, system_message, /*sleep_secs*/ 0) +} + +fn write_interrupt_hook_with_sleep( + home: &Path, + system_message: Option<&str>, + sleep_secs: u64, +) -> Result<()> { let script_path = home.join("interrupt_hook.py"); let log_path = home.join("interrupt_hook_log.jsonl"); let system_message_json = @@ -54,14 +63,19 @@ fn write_interrupt_hook(home: &Path, system_message: Option<&str>) -> Result<()> r#"import json from pathlib import Path import sys +import time log_path = Path(r"{log_path}") system_message = json.loads({system_message_json:?}) +sleep_secs = {sleep_secs} payload = json.load(sys.stdin) with log_path.open("a", encoding="utf-8") as handle: handle.write(json.dumps(payload) + "\n") +if sleep_secs: + time.sleep(sleep_secs) + if system_message is None: print("{{}}") else: @@ -69,6 +83,7 @@ else: "#, log_path = log_path.display(), system_message_json = system_message_json, + sleep_secs = sleep_secs, ); let hooks = serde_json::json!({ "hooks": { @@ -250,6 +265,10 @@ async fn interrupt_hook_runs_before_turn_aborted_and_records_payload() -> Result .and_then(Value::as_str) .is_some_and(|path| !path.is_empty()) ); + assert_eq!( + payload.get("reason"), + Some(&Value::String("user".to_string())) + ); assert!(payload.get("stop_hook_active").is_none()); assert!(payload.get("last_assistant_message").is_none()); @@ -531,7 +550,113 @@ async fn guardian_interrupt_runs_interrupt_hook() -> Result<()> { .and_then(Value::as_str) .is_some_and(|path| !path.is_empty()) ); + assert_eq!( + payload.get("reason"), + Some(&Value::String("auto_review".to_string())) + ); assert!(payload.get("last_assistant_message").is_none()); Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shutdown_interrupt_runs_bounded_interrupt_hook() -> Result<()> { + skip_if_no_network!(Ok(())); + + let (gate_completed_tx, gate_completed_rx) = oneshot::channel(); + let chunks = vec![vec![ + StreamingSseChunk { + gate: None, + body: sse_event(ev_response_created("resp-shutdown-1")), + }, + StreamingSseChunk { + gate: None, + body: sse_event(ev_message_item_added("msg-shutdown-1", "")), + }, + StreamingSseChunk { + gate: None, + body: sse_event(ev_output_text_delta("shore ")), + }, + StreamingSseChunk { + gate: None, + body: sse_event(ev_message_item_done("msg-shutdown-1", "shore break")), + }, + StreamingSseChunk { + gate: Some(gate_completed_rx), + body: sse_event(ev_completed("resp-shutdown-1")), + }, + ]]; + let (server, _completions) = start_streaming_sse_server(chunks).await; + + let mut builder = test_codex() + .with_model("gpt-5.4") + .with_pre_build_hook(|home| { + if let Err(error) = write_interrupt_hook_with_sleep( + home, /*system_message*/ None, /*sleep_secs*/ 9, + ) { + panic!("failed to write interrupt hook fixture: {error}"); + } + }) + .with_config(trust_discovered_hooks); + let test = builder.build_with_streaming_server(&server).await?; + + submit_text_turn(&test, "shutdown me").await?; + + let _: TurnItem = wait_for_event_match(&test.codex, |event| match event { + EventMsg::ItemCompleted(ItemCompletedEvent { + item: TurnItem::AgentMessage(item), + .. + }) => Some(TurnItem::AgentMessage(item.clone())), + _ => None, + }) + .await; + + let codex = Arc::clone(&test.codex); + let shutdown_task = tokio::spawn(async move { codex.shutdown_and_wait().await }); + + let started = wait_for_event_match(&test.codex, |event| match event { + EventMsg::HookStarted(started) if started.run.event_name == HookEventName::Interrupt => { + Some(started.clone()) + } + _ => None, + }) + .await; + let completed = wait_for_event_match(&test.codex, |event| match event { + EventMsg::HookCompleted(completed) + if completed.run.event_name == HookEventName::Interrupt => + { + Some(completed.clone()) + } + _ => None, + }) + .await; + let aborted = wait_for_event_match(&test.codex, |event| match event { + EventMsg::TurnAborted(aborted) => Some(aborted.clone()), + _ => None, + }) + .await; + + let _ = gate_completed_tx.send(()); + shutdown_task.await??; + + assert_eq!(started.run.event_name, HookEventName::Interrupt); + assert_eq!(completed.run.event_name, HookEventName::Interrupt); + assert_eq!(completed.run.status, HookRunStatus::Failed); + assert_eq!( + completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "hook timed out after 8s".to_string(), + }] + ); + assert_eq!(aborted.reason, TurnAbortReason::Interrupted); + + let hook_inputs = read_interrupt_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + hook_inputs[0].get("reason"), + Some(&Value::String("shutdown".to_string())) + ); + + Ok(()) +} diff --git a/codex-rs/hooks/schema/generated/interrupt.command.input.schema.json b/codex-rs/hooks/schema/generated/interrupt.command.input.schema.json index e0ac2a96bb..c720c9d09a 100644 --- a/codex-rs/hooks/schema/generated/interrupt.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/interrupt.command.input.schema.json @@ -30,6 +30,14 @@ ], "type": "string" }, + "reason": { + "enum": [ + "user", + "shutdown", + "auto_review" + ], + "type": "string" + }, "session_id": { "type": "string" }, @@ -46,6 +54,7 @@ "hook_event_name", "model", "permission_mode", + "reason", "session_id", "transcript_path", "turn_id" diff --git a/codex-rs/hooks/src/engine/command_runner.rs b/codex-rs/hooks/src/engine/command_runner.rs index 7366d4ec51..053b8add05 100644 --- a/codex-rs/hooks/src/engine/command_runner.rs +++ b/codex-rs/hooks/src/engine/command_runner.rs @@ -10,6 +10,32 @@ use tokio::time::timeout; use super::CommandShell; use super::ConfiguredHandler; +const SHUTDOWN_BOUNDED_TIMEOUT_SEC: u64 = 8; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CommandExecutionPolicy { + Standard, + ShutdownBounded { timeout_cap_sec: u64 }, +} + +impl CommandExecutionPolicy { + pub const fn shutdown_bounded() -> Self { + Self::ShutdownBounded { + timeout_cap_sec: SHUTDOWN_BOUNDED_TIMEOUT_SEC, + } + } + + fn effective_timeout_sec(self, configured_timeout_sec: u64) -> u64 { + match self { + Self::Standard => configured_timeout_sec, + Self::ShutdownBounded { timeout_cap_sec } => { + configured_timeout_sec.min(timeout_cap_sec) + } + } + .max(1) + } +} + #[derive(Debug)] pub(crate) struct CommandRunResult { pub started_at: i64, @@ -26,6 +52,7 @@ pub(crate) async fn run_command( handler: &ConfiguredHandler, input_json: &str, cwd: &Path, + execution_policy: CommandExecutionPolicy, ) -> CommandRunResult { let started_at = chrono::Utc::now().timestamp(); let started = Instant::now(); @@ -68,7 +95,8 @@ pub(crate) async fn run_command( }; } - let timeout_duration = Duration::from_secs(handler.timeout_sec); + let effective_timeout_sec = execution_policy.effective_timeout_sec(handler.timeout_sec); + let timeout_duration = Duration::from_secs(effective_timeout_sec); match timeout(timeout_duration, child.wait_with_output()).await { Ok(Ok(output)) => CommandRunResult { started_at, @@ -95,7 +123,7 @@ pub(crate) async fn run_command( exit_code: None, stdout: String::new(), stderr: String::new(), - error: Some(format!("hook timed out after {}s", handler.timeout_sec)), + error: Some(format!("hook timed out after {effective_timeout_sec}s")), }, } } @@ -133,3 +161,99 @@ fn default_shell_command() -> Command { command } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::time::Duration; + use std::time::Instant; + + use codex_protocol::protocol::HookEventName; + use codex_protocol::protocol::HookSource; + use codex_utils_absolute_path::test_support::PathBufExt; + use codex_utils_absolute_path::test_support::test_path_buf; + use tempfile::tempdir; + + use super::CommandExecutionPolicy; + use super::CommandShell; + use super::ConfiguredHandler; + use super::run_command; + + #[test] + fn shutdown_bounded_policy_caps_timeout() { + assert_eq!( + CommandExecutionPolicy::Standard.effective_timeout_sec(600), + 600 + ); + assert_eq!( + CommandExecutionPolicy::shutdown_bounded().effective_timeout_sec(600), + 8 + ); + assert_eq!( + CommandExecutionPolicy::ShutdownBounded { timeout_cap_sec: 3 }.effective_timeout_sec(2), + 2 + ); + } + + #[tokio::test] + async fn shutdown_bounded_timeout_uses_capped_timeout() { + let temp_dir = tempdir().expect("create temp dir"); + let started = Instant::now(); + let result = run_command( + &test_shell(), + &handler(sleep_command(), 10), + "{}", + temp_dir.path(), + CommandExecutionPolicy::ShutdownBounded { timeout_cap_sec: 1 }, + ) + .await; + + assert!(started.elapsed() < Duration::from_millis(1500)); + assert_eq!(result.exit_code, None); + assert_eq!(result.error.as_deref(), Some("hook timed out after 1s")); + } + + fn handler(command: String, timeout_sec: u64) -> ConfiguredHandler { + ConfiguredHandler { + event_name: HookEventName::Interrupt, + matcher: None, + command, + timeout_sec, + status_message: None, + source_path: test_path_buf("/tmp/hooks.json").abs(), + source: HookSource::User, + display_order: 0, + env: HashMap::new(), + } + } + + fn test_shell() -> CommandShell { + #[cfg(windows)] + { + CommandShell { + program: "powershell".to_string(), + args: vec!["-NoProfile".to_string(), "-Command".to_string()], + } + } + + #[cfg(not(windows))] + { + CommandShell { + program: "/bin/sh".to_string(), + args: vec!["-lc".to_string()], + } + } + } + + fn sleep_command() -> String { + #[cfg(windows)] + { + "Start-Sleep -Seconds 2".to_string() + } + + #[cfg(not(windows))] + { + "sleep 2".to_string() + } + } +} diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index 7abb32c2b8..178e5621f7 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -13,6 +13,7 @@ use codex_protocol::protocol::HookScope; use super::CommandShell; use super::ConfiguredHandler; +use super::command_runner::CommandExecutionPolicy; use super::command_runner::CommandRunResult; use super::command_runner::run_command; use crate::events::common::matches_matcher; @@ -52,7 +53,8 @@ pub(crate) fn select_handlers_for_matcher_inputs( | HookEventName::SubagentStart | HookEventName::SubagentStop | HookEventName::PreCompact - | HookEventName::PostCompact => { + | HookEventName::PostCompact + | HookEventName::Interrupt => { if matcher_inputs.is_empty() { matches_matcher(handler.matcher.as_deref(), /*input*/ None) } else { @@ -61,9 +63,7 @@ pub(crate) fn select_handlers_for_matcher_inputs( .any(|input| matches_matcher(handler.matcher.as_deref(), Some(input))) } } - HookEventName::UserPromptSubmit | HookEventName::Stop | HookEventName::Interrupt => { - true - } + HookEventName::UserPromptSubmit | HookEventName::Stop => true, }) .cloned() .collect() @@ -95,13 +95,34 @@ pub(crate) async fn execute_handlers( cwd: &Path, turn_id: Option, parse: fn(&ConfiguredHandler, CommandRunResult, Option) -> ParsedHandler, +) -> Vec> { + execute_handlers_with_policy( + shell, + handlers, + input_json, + cwd, + turn_id, + CommandExecutionPolicy::Standard, + parse, + ) + .await +} + +pub(crate) async fn execute_handlers_with_policy( + shell: &CommandShell, + handlers: Vec, + input_json: String, + cwd: &Path, + turn_id: Option, + execution_policy: CommandExecutionPolicy, + parse: fn(&ConfiguredHandler, CommandRunResult, Option) -> ParsedHandler, ) -> Vec> { let mut pending = FuturesUnordered::new(); for (configured_order, handler) in handlers.into_iter().enumerate() { let input_json = input_json.clone(); let turn_id = turn_id.clone(); pending.push(async move { - let result = run_command(shell, &handler, &input_json, cwd).await; + let result = run_command(shell, &handler, &input_json, cwd, execution_policy).await; (configured_order, parse(&handler, result, turn_id)) }); } @@ -211,11 +232,11 @@ mod tests { } #[test] - fn select_handlers_ignores_interrupt_matchers() { + fn select_handlers_filters_interrupt_matchers() { let handlers = vec![ make_handler( HookEventName::Interrupt, - Some("^interrupted$"), + Some("^auto_review$"), "echo first", /*display_order*/ 0, ), @@ -227,17 +248,36 @@ mod tests { ), ]; - let selected = select_handlers( - &handlers, - HookEventName::Interrupt, - /*matcher_input*/ None, - ); + let selected = select_handlers(&handlers, HookEventName::Interrupt, Some("auto_review")); assert_eq!(selected.len(), 2); assert_eq!(selected[0].display_order, 0); assert_eq!(selected[1].display_order, 1); } + #[test] + fn select_handlers_excludes_non_matching_interrupt_handlers() { + let handlers = vec![ + make_handler( + HookEventName::Interrupt, + Some("^auto_review$"), + "echo first", + /*display_order*/ 0, + ), + make_handler( + HookEventName::Interrupt, + Some("^shutdown$"), + "echo second", + /*display_order*/ 1, + ), + ]; + + let selected = select_handlers(&handlers, HookEventName::Interrupt, Some("auto_review")); + + assert_eq!(selected.len(), 1); + assert_eq!(selected[0].display_order, 0); + } + #[test] fn select_handlers_keeps_overlapping_session_start_matchers() { let handlers = vec![ diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 5c43e2159f..4875c4a9e3 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -272,8 +272,12 @@ impl ClaudeHooksEngine { crate::events::interrupt::preview(&self.handlers, request) } - pub(crate) async fn run_interrupt(&self, request: InterruptRequest) -> InterruptOutcome { - crate::events::interrupt::run(&self.handlers, &self.shell, request).await + pub(crate) async fn run_interrupt( + &self, + request: InterruptRequest, + execution_policy: command_runner::CommandExecutionPolicy, + ) -> InterruptOutcome { + crate::events::interrupt::run(&self.handlers, &self.shell, request, execution_policy).await } async fn maybe_spill_texts(&self, session_id: ThreadId, texts: Vec) -> Vec { diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index 393604339b..e667a34f4f 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -114,8 +114,9 @@ pub(crate) fn matcher_pattern_for_event( | HookEventName::SubagentStart | HookEventName::SubagentStop | HookEventName::PreCompact - | HookEventName::PostCompact => matcher, - HookEventName::UserPromptSubmit | HookEventName::Stop | HookEventName::Interrupt => None, + | HookEventName::PostCompact + | HookEventName::Interrupt => matcher, + HookEventName::UserPromptSubmit | HookEventName::Stop => None, } } @@ -262,10 +263,6 @@ mod tests { matcher_pattern_for_event(HookEventName::Stop, Some("^done$")), None ); - assert_eq!( - matcher_pattern_for_event(HookEventName::Interrupt, Some("^interrupted$")), - None - ); } #[test] @@ -290,5 +287,9 @@ mod tests { matcher_pattern_for_event(HookEventName::PostCompact, Some("manual|auto")), Some("manual|auto") ); + assert_eq!( + matcher_pattern_for_event(HookEventName::Interrupt, Some("user|shutdown")), + Some("user|shutdown") + ); } } diff --git a/codex-rs/hooks/src/events/interrupt.rs b/codex-rs/hooks/src/events/interrupt.rs index 741fc08f29..8e8640749a 100644 --- a/codex-rs/hooks/src/events/interrupt.rs +++ b/codex-rs/hooks/src/events/interrupt.rs @@ -12,12 +12,30 @@ use codex_utils_absolute_path::AbsolutePathBuf; use super::common; use crate::engine::CommandShell; use crate::engine::ConfiguredHandler; +use crate::engine::command_runner::CommandExecutionPolicy; use crate::engine::command_runner::CommandRunResult; use crate::engine::dispatcher; use crate::engine::output_parser; use crate::schema::InterruptCommandInput; use crate::schema::NullableString; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InterruptReason { + User, + Shutdown, + AutoReview, +} + +impl InterruptReason { + pub fn as_str(self) -> &'static str { + match self { + Self::User => "user", + Self::Shutdown => "shutdown", + Self::AutoReview => "auto_review", + } + } +} + #[derive(Debug, Clone)] pub struct InterruptRequest { pub session_id: ThreadId, @@ -26,6 +44,7 @@ pub struct InterruptRequest { pub transcript_path: Option, pub model: String, pub permission_mode: String, + pub reason: InterruptReason, } #[derive(Debug, Default)] @@ -38,12 +57,12 @@ struct InterruptHandlerData; pub(crate) fn preview( handlers: &[ConfiguredHandler], - _request: &InterruptRequest, + request: &InterruptRequest, ) -> Vec { dispatcher::select_handlers( handlers, HookEventName::Interrupt, - /*matcher_input*/ None, + Some(request.reason.as_str()), ) .into_iter() .map(|handler| dispatcher::running_summary(&handler)) @@ -54,11 +73,12 @@ pub(crate) async fn run( handlers: &[ConfiguredHandler], shell: &CommandShell, request: InterruptRequest, + execution_policy: CommandExecutionPolicy, ) -> InterruptOutcome { let matched = dispatcher::select_handlers( handlers, HookEventName::Interrupt, - /*matcher_input*/ None, + Some(request.reason.as_str()), ); if matched.is_empty() { return InterruptOutcome::default(); @@ -71,6 +91,7 @@ pub(crate) async fn run( transcript_path, model, permission_mode, + reason, } = request; let input_json = match serde_json::to_string(&InterruptCommandInput { session_id: session_id.to_string(), @@ -80,6 +101,7 @@ pub(crate) async fn run( hook_event_name: "Interrupt".to_string(), model, permission_mode, + reason: reason.as_str().to_string(), }) { Ok(input_json) => input_json, Err(error) => { @@ -93,12 +115,13 @@ pub(crate) async fn run( } }; - let results = dispatcher::execute_handlers( + let results = dispatcher::execute_handlers_with_policy( shell, matched, input_json, cwd.as_path(), Some(turn_id), + execution_policy, parse_completed, ) .await; diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index a7598d9825..040055893b 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -14,6 +14,7 @@ pub use config_rules::hook_states_from_stack; pub use declarations::PluginHookDeclaration; pub use declarations::plugin_hook_declarations; pub use engine::HookListEntry; +pub use engine::command_runner::CommandExecutionPolicy; pub use events::common::SubagentHookContext; /// Hook event names as they appear in hooks JSON and config files. pub const HOOK_EVENT_NAMES: [&str; 11] = [ @@ -35,7 +36,7 @@ pub const HOOK_EVENT_NAMES: [&str; 11] = [ /// Other events can appear in hooks JSON, but Codex ignores their matcher /// fields because those events do not dispatch against a tool, compaction /// trigger, or session-start source. -pub const HOOK_EVENT_NAMES_WITH_MATCHERS: [&str; 8] = [ +pub const HOOK_EVENT_NAMES_WITH_MATCHERS: [&str; 9] = [ "PreToolUse", "PermissionRequest", "PostToolUse", @@ -44,6 +45,7 @@ pub const HOOK_EVENT_NAMES_WITH_MATCHERS: [&str; 8] = [ "SessionStart", "SubagentStart", "SubagentStop", + "Interrupt", ]; pub use events::compact::PostCompactRequest; @@ -51,6 +53,7 @@ pub use events::compact::PreCompactOutcome; pub use events::compact::PreCompactRequest; pub use events::compact::StatelessHookOutcome; pub use events::interrupt::InterruptOutcome; +pub use events::interrupt::InterruptReason; pub use events::interrupt::InterruptRequest; pub use events::permission_request::PermissionRequestDecision; pub use events::permission_request::PermissionRequestOutcome; diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index 25fdca3766..378f270e47 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -213,8 +213,12 @@ impl Hooks { self.engine.preview_interrupt(request) } - pub async fn run_interrupt(&self, request: InterruptRequest) -> InterruptOutcome { - self.engine.run_interrupt(request).await + pub async fn run_interrupt( + &self, + request: InterruptRequest, + execution_policy: crate::CommandExecutionPolicy, + ) -> InterruptOutcome { + self.engine.run_interrupt(request, execution_policy).await } } diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 3602c86b0a..b6f9c7f1e1 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -621,6 +621,8 @@ pub(crate) struct InterruptCommandInput { pub model: String, #[schemars(schema_with = "permission_mode_schema")] pub permission_mode: String, + #[schemars(schema_with = "interrupt_reason_schema")] + pub reason: String, } pub fn write_schema_fixtures(schema_root: &Path) -> anyhow::Result<()> { @@ -832,6 +834,10 @@ fn compaction_trigger_schema(_gen: &mut SchemaGenerator) -> Schema { string_enum_schema(&["manual", "auto"]) } +fn interrupt_reason_schema(_gen: &mut SchemaGenerator) -> Schema { + string_enum_schema(&["user", "shutdown", "auto_review"]) +} + fn string_const_schema(value: &str) -> Schema { let mut schema = SchemaObject { instance_type: Some(InstanceType::String.into()), @@ -1152,6 +1158,16 @@ mod tests { .get("last_assistant_message") .is_none() ); + assert_eq!( + interrupt_input["properties"]["reason"]["enum"], + serde_json::json!(["user", "shutdown", "auto_review"]) + ); + assert!( + interrupt_input["required"] + .as_array() + .expect("interrupt input required fields") + .contains(&Value::String("reason".to_string())) + ); let interrupt_output: Value = serde_json::from_slice( &schema_json::()