From c7c824dce4da186e5142af5d9a1587ae553efe46 Mon Sep 17 00:00:00 2001 From: teddywyly-oai Date: Tue, 1 Sep 2026 17:53:07 +0000 Subject: [PATCH] Treat bundled cleanup hooks as built-ins (#42110) ## What changed - Centralize the allowlist for bundled MCP cleanup hooks and use it for both local and executor-discovered plugins, including `unified-computer-use` cleanup through `cua_repl`. - Mark matching cleanup hooks as trusted built-ins so they run without saved hook trust and remain active when regular hooks or their per-hook state are disabled. Plugin enablement and managed-only policy still apply. - Hide built-in cleanup hooks from hook listings and lifecycle notifications while retaining their metrics. Keep the built-in classification out of serialized protocol data. ## Testing - Cover allowlist boundaries, trust and enablement behavior, inline and file-based hook declarations, MCP success and error responses, hook listing, lifecycle notifications, metrics, and protocol serialization. GitOrigin-RevId: f93b7bc99f4ed9694f529def8ec383b45f31430e --- .../request_processors/catalog_processor.rs | 1 + .../app-server/tests/suite/v2/hooks_list.rs | 98 ++++++ codex-rs/core-plugins/src/executor_hooks.rs | 241 ++------------ .../core-plugins/src/executor_hooks_tests.rs | 24 ++ codex-rs/core/src/hook_runtime.rs | 82 ++++- codex-rs/core/tests/suite/hooks.rs | 281 ++++++++++++++++ .../hooks/src/engine/command_runner_tests.rs | 4 + codex-rs/hooks/src/engine/discovery.rs | 34 +- codex-rs/hooks/src/engine/dispatcher.rs | 3 + codex-rs/hooks/src/engine/mcp_runner_tests.rs | 1 + codex-rs/hooks/src/engine/mod.rs | 15 +- codex-rs/hooks/src/engine/mod_tests.rs | 309 +++++++++++++++++- codex-rs/hooks/src/events/compact.rs | 1 + codex-rs/hooks/src/events/interrupt_tests.rs | 1 + codex-rs/hooks/src/events/post_tool_use.rs | 1 + codex-rs/hooks/src/events/pre_tool_use.rs | 1 + .../hooks/src/events/session_end_tests.rs | 1 + codex-rs/hooks/src/events/session_start.rs | 1 + codex-rs/hooks/src/events/stop.rs | 1 + .../hooks/src/events/user_prompt_submit.rs | 1 + codex-rs/plugin/src/bundled_hooks.rs | 152 +++++++++ codex-rs/plugin/src/lib.rs | 2 + codex-rs/protocol/src/protocol.rs | 42 +++ 23 files changed, 1058 insertions(+), 239 deletions(-) create mode 100644 codex-rs/plugin/src/bundled_hooks.rs diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index 9c7bd4e80690..0b2178ea9280 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -66,6 +66,7 @@ fn skills_to_info( fn hooks_to_info(hooks: &[codex_hooks::HookListEntry]) -> Vec { hooks .iter() + .filter(|hook| !hook.builtin) .map(|hook| { let handler = match &hook.handler { HookListEntryHandler::Command { command, r#async } => { diff --git a/codex-rs/app-server/tests/suite/v2/hooks_list.rs b/codex-rs/app-server/tests/suite/v2/hooks_list.rs index a2285b1e2e01..6843820f6c7c 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -818,6 +818,104 @@ async fn hooks_list_shows_discovered_plugin_mcp_tool_hook() -> Result<()> { Ok(()) } +#[test_case::test_case("browser", "node_repl"; "node_repl")] +#[test_case::test_case("unified-computer-use", "cua_repl"; "cua_repl")] +#[tokio::test] +async fn hooks_list_hides_builtin_cleanup_and_preserves_other_plugin_hooks( + plugin_name: &str, + server: &str, +) -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let plugin_id = format!("{plugin_name}@openai-bundled"); + let plugin_root = codex_home + .path() + .join(format!("plugins/cache/openai-bundled/{plugin_name}/local")); + std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?; + std::fs::create_dir_all(plugin_root.join("hooks"))?; + std::fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + serde_json::to_vec(&serde_json::json!({ "name": plugin_name }))?, + )?; + let ordinary_command = "echo ordinary plugin hook"; + std::fs::write( + plugin_root.join("hooks/hooks.json"), + serde_json::to_vec(&serde_json::json!({ + "hooks": { "Stop": [{ "hooks": [ + { "type": "mcp_tool", "server": server, "tool": "turn_ended" }, + { "type": "command", "command": ordinary_command, "timeout": 5 }, + ] }] }, + }))?, + )?; + std::fs::write( + codex_home.path().join("config.toml"), + format!( + r#"[features] +plugins = true +hooks = true + +[plugins."{plugin_id}"] +enabled = true + +[hooks.state."{plugin_id}:hooks/hooks.json:stop:0:1"] +enabled = false +"# + ), + )?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + let request_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![cwd.path().to_path_buf()], + }) + .await?; + let HooksListResponse { data } = + timeout(DEFAULT_TIMEOUT, mcp.read_response(request_id)).await??; + let source_path = AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( + plugin_root.join("hooks/hooks.json"), + )?)?; + assert_eq!( + data, + vec![HooksListEntry { + cwd: cwd.path().to_path_buf(), + hooks: vec![HookMetadata { + key: format!("{plugin_id}:hooks/hooks.json:stop:0:1"), + event_name: HookEventName::Stop, + handler: HookHandlerMetadata::Command { + command: ordinary_command.to_string(), + r#async: false, + }, + matcher: None, + timeout_sec: 5, + status_message: None, + additional_context_limit: None, + source_path, + source: HookSource::Plugin, + plugin_id: Some(plugin_id), + display_order: 1, + enabled: false, + is_managed: false, + current_hash: command_hook_hash( + "stop", + /*matcher*/ None, + ordinary_command, + /*timeout_sec*/ 5, + /*async*/ false, + /*status_message*/ None, + /*additional_context_limit*/ None, + ), + trust_status: HookTrustStatus::Untrusted, + }], + warnings: Vec::new(), + errors: Vec::new(), + }] + ); + Ok(()) +} + #[tokio::test] async fn hooks_list_warms_plugin_capabilities_for_thread_start() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core-plugins/src/executor_hooks.rs b/codex-rs/core-plugins/src/executor_hooks.rs index c5aaed3e77fb..b848d97502de 100644 --- a/codex-rs/core-plugins/src/executor_hooks.rs +++ b/codex-rs/core-plugins/src/executor_hooks.rs @@ -8,172 +8,14 @@ use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; use codex_mcp::ToolInfo; use codex_plugin::ExecutorPluginHookSource; use codex_plugin::PluginId; +use codex_plugin::is_allowlisted_bundled_cleanup_hook; use codex_plugin::manifest::PluginManifestHooks; use codex_protocol::capabilities::CapabilityRootLocation; -use codex_protocol::protocol::HookEventName; use serde_json::Map; use serde_json::Value; use crate::manifest::parse_plugin_manifest_uri; -struct AllowlistedExecutorPluginHook { - plugin_id: &'static str, - event: HookEventName, - target: ExecutorPluginHookTarget, -} - -enum ExecutorPluginHookTarget { - Executor { - server: &'static str, - tool: &'static str, - }, - App { - connector_id: &'static str, - tool: &'static str, - }, -} - -// Executor plugin manifests are unsigned, so temporarily hardcode the expected -// plugin identities, events, and MCP targets until plugin signing lands. -const ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS: &[AllowlistedExecutorPluginHook] = &[ - AllowlistedExecutorPluginHook { - plugin_id: "browser@openai-bundled", - event: HookEventName::Stop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "browser@openai-bundled", - event: HookEventName::Interrupt, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "browser@openai-bundled", - event: HookEventName::SubagentStop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome@openai-bundled", - event: HookEventName::Stop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome@openai-bundled", - event: HookEventName::Interrupt, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome@openai-bundled", - event: HookEventName::SubagentStop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome-dev@openai-bundled", - event: HookEventName::Stop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome-dev@openai-bundled", - event: HookEventName::Interrupt, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome-dev@openai-bundled", - event: HookEventName::SubagentStop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome-internal@openai-bundled", - event: HookEventName::Stop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome-internal@openai-bundled", - event: HookEventName::Interrupt, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "chrome-internal@openai-bundled", - event: HookEventName::SubagentStop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "computer-use@openai-bundled", - event: HookEventName::Stop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "computer-use@openai-bundled", - event: HookEventName::Interrupt, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "computer-use@openai-bundled", - event: HookEventName::SubagentStop, - target: ExecutorPluginHookTarget::Executor { - server: "node_repl", - tool: "turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "browser@openai-curated-remote", - event: HookEventName::Stop, - target: ExecutorPluginHookTarget::App { - connector_id: "connector_openai_browser", - tool: "browser.turn_ended", - }, - }, - AllowlistedExecutorPluginHook { - plugin_id: "browser@openai-curated-remote", - event: HookEventName::SubagentStop, - target: ExecutorPluginHookTarget::App { - connector_id: "connector_openai_browser", - tool: "browser.turn_ended", - }, - }, -]; - /// Returns accepted inline hook sources from executor-discovered plugin manifests. /// Each source carries its trusted MCP routing metadata. `lookup_enabled_tool` must use a /// consistent catalog/config snapshot, include app-only tools, and exclude tools disabled by @@ -183,7 +25,7 @@ const ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS: &[AllowlistedExecutorPluginHook] = &[ /// after earlier lifecycle events have passed. /// /// Note: Executor manifests are not signed yet, so temporarily we only admit the known cleanup -/// hooks from the bundled plugins above and the remote Browser plugin. +/// hooks from the bundled cleanup allowlist and the remote Browser plugin. pub fn executor_plugin_hook_sources<'a>( snapshot: &ExecutorCapabilityDiscoverySnapshot, lookup_enabled_tool: impl Fn(&str, &str) -> Option<&'a ToolInfo>, @@ -239,62 +81,37 @@ pub fn executor_plugin_hook_sources<'a>( // FIXME: Remove this temporary filter once executor plugin hooks can be trusted. sources .into_iter() - .filter_map(|source| allowlisted_source(source, &lookup_enabled_tool)) + .filter_map(|mut source| { + let plugin_id = source.plugin_id.as_key(); + for (event, groups) in source.hooks.matcher_groups_mut() { + groups.retain_mut(|group| { + group.hooks.retain(|handler| { + let app_connector_id = match handler { + HookHandlerConfig::McpTool { server, tool, .. } + if server == CODEX_APPS_MCP_SERVER_NAME => + { + lookup_enabled_tool(server, tool) + .and_then(|info| info.connector_id.as_deref()) + } + _ => None, + }; + is_allowlisted_bundled_cleanup_hook( + &plugin_id, + event, + group.matcher.as_deref(), + handler, + app_connector_id, + ) + }); + !group.hooks.is_empty() + }); + } + (!source.hooks.is_empty()).then_some(source) + }) .filter_map(|source| resolve_mcp_routing(source, &lookup_enabled_tool)) .collect() } -fn allowlisted_source<'a>( - mut source: ExecutorPluginHookSource, - lookup_enabled_tool: &impl Fn(&str, &str) -> Option<&'a ToolInfo>, -) -> Option { - let plugin_id = source.plugin_id.as_key(); - for (event, groups) in source.hooks.matcher_groups_mut() { - groups.retain_mut(|group| { - if group.matcher.is_some() { - return false; - } - group.hooks.retain(|handler| { - let HookHandlerConfig::McpTool { - server, - tool, - input, - .. - } = handler - else { - return false; - }; - let Some(hook) = ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS - .iter() - .find(|hook| hook.plugin_id == plugin_id && hook.event == event) - else { - return false; - }; - match hook.target { - ExecutorPluginHookTarget::Executor { - server: expected_server, - tool: expected_tool, - } => server == expected_server && tool == expected_tool, - ExecutorPluginHookTarget::App { - connector_id, - tool: expected_tool, - } => { - // Raw Apps tool names can collide; admission must also match the listed connector. - server == CODEX_APPS_MCP_SERVER_NAME - && tool == expected_tool - && input.is_empty() - && lookup_enabled_tool(server, tool).is_some_and(|tool_info| { - tool_info.connector_id.as_deref() == Some(connector_id) - }) - } - } - }); - !group.hooks.is_empty() - }); - } - (!source.hooks.is_empty()).then_some(source) -} - /// Resolves routing for an admitted MCP hook source. fn resolve_mcp_routing<'a>( mut source: ExecutorPluginHookSource, diff --git a/codex-rs/core-plugins/src/executor_hooks_tests.rs b/codex-rs/core-plugins/src/executor_hooks_tests.rs index 5c31d10c9f56..d3e031dff767 100644 --- a/codex-rs/core-plugins/src/executor_hooks_tests.rs +++ b/codex-rs/core-plugins/src/executor_hooks_tests.rs @@ -146,6 +146,30 @@ fn discovers_allowlisted_executor_plugin_hook_sources() { ); } +#[test] +fn discovers_unified_computer_use_cleanup_hooks() { + let mut manifest = cleanup_hook_manifest(); + manifest["name"] = json!("unified-computer-use"); + manifest["hooks"]["hooks"]["Stop"][0]["hooks"][0]["server"] = json!("cua_repl"); + let snapshot = snapshot_for_manifest( + "unified-computer-use@openai-bundled", + "executor-a", + "file:///plugins/computer-use/.codex-plugin/plugin.json", + manifest, + ); + let mut expected = expected_source(/*index*/ 0); + expected.plugin_id = PluginId::parse("unified-computer-use@openai-bundled").expect("plugin id"); + let HookHandlerConfig::McpTool { server, .. } = &mut expected.hooks.stop[0].hooks[0] else { + panic!("expected an MCP tool hook"); + }; + *server = "cua_repl".to_string(); + + assert_eq!( + executor_plugin_hook_sources(&snapshot, |_, _| None), + vec![expected] + ); +} + #[test] fn filters_mixed_handlers_without_rewriting_allowed_groups() { let mut expected = expected_source(/*index*/ 0); diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 7ae49b3a233d..4964905418c4 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -844,6 +844,10 @@ fn additional_context_messages(additional_contexts: Vec) -> Vec bool { + !run.builtin && run.execution_mode == HookExecutionMode::Sync +} + async fn emit_hook_started_events( sess: &Arc, turn_context: &Arc, @@ -851,7 +855,7 @@ async fn emit_hook_started_events( ) { for run in preview_runs .into_iter() - .filter(|run| run.execution_mode == HookExecutionMode::Sync) + .filter(should_emit_hook_notification) { sess.send_event( turn_context, @@ -889,7 +893,7 @@ pub(crate) async fn emit_hook_completed_events( for completed in completed_events { emit_hook_completed_metrics(turn_context, &completed); track_hook_completed_analytics(sess, turn_context, &completed); - if completed.run.execution_mode == HookExecutionMode::Sync { + if should_emit_hook_notification(&completed.run) { sess.send_event(turn_context, EventMsg::HookCompleted(completed)) .await; } @@ -1036,6 +1040,12 @@ fn compaction_trigger_label(value: CompactionTrigger) -> &'static str { #[cfg(test)] mod tests { + use std::sync::Arc; + + use codex_otel::HOOK_RUN_DURATION_METRIC; + use codex_otel::HOOK_RUN_METRIC; + use codex_otel::MetricsClient; + use codex_otel::MetricsConfig; use codex_protocol::models::ContentItem; use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookExecutionMode; @@ -1043,6 +1053,11 @@ mod tests { use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookScope; use codex_protocol::protocol::HookSource; + use opentelemetry_sdk::metrics::InMemoryMetricExporter; + use opentelemetry_sdk::metrics::data::AggregatedMetrics; + use opentelemetry_sdk::metrics::data::HistogramDataPoint; + use opentelemetry_sdk::metrics::data::MetricData; + use opentelemetry_sdk::metrics::data::SumDataPoint; use pretty_assertions::assert_eq; use super::additional_context_messages; @@ -1094,18 +1109,42 @@ mod tests { } #[tokio::test] - async fn hook_lifecycle_notifications_only_report_synchronous_runs() { - let (session, turn_context, events) = make_session_and_context_with_rx().await; + async fn hook_lifecycle_notifications_hide_builtin_and_async_runs_but_preserve_metrics() { + let metrics = MetricsClient::new( + MetricsConfig::in_memory( + "test", + "codex-core", + env!("CARGO_PKG_VERSION"), + InMemoryMetricExporter::default(), + ) + .with_runtime_reader(), + ) + .expect("in-memory metrics client"); + let (session, mut turn_context, events) = make_session_and_context_with_rx().await; + let turn_context_mut = Arc::get_mut(&mut turn_context).expect("single turn context ref"); + turn_context_mut.session_telemetry = turn_context_mut + .session_telemetry + .clone() + .with_metrics(metrics.clone()); let mut synchronous_run = sample_hook_run(HookRunStatus::Running, HookSource::User); synchronous_run.id = "synchronous-hook".to_string(); let mut asynchronous_run = synchronous_run.clone(); asynchronous_run.id = "asynchronous-hook".to_string(); asynchronous_run.execution_mode = HookExecutionMode::Async; + let mut builtin_run = synchronous_run.clone(); + builtin_run.id = "builtin-hook".to_string(); + builtin_run.builtin = true; + builtin_run.source = HookSource::Plugin; + builtin_run.handler_type = HookHandlerType::McpTool; emit_hook_started_events( &session, &turn_context, - vec![asynchronous_run.clone(), synchronous_run.clone()], + vec![ + builtin_run.clone(), + asynchronous_run.clone(), + synchronous_run.clone(), + ], ) .await; @@ -1117,12 +1156,17 @@ mod tests { )); assert!(events.try_recv().is_err()); + builtin_run.status = HookRunStatus::Completed; asynchronous_run.status = HookRunStatus::Completed; synchronous_run.status = HookRunStatus::Completed; emit_hook_completed_events( &session, &turn_context, vec![ + HookCompletedEvent { + turn_id: Some(turn_context.sub_id.clone()), + run: builtin_run, + }, HookCompletedEvent { turn_id: Some(turn_context.sub_id.clone()), run: asynchronous_run, @@ -1142,6 +1186,33 @@ mod tests { if event.run.id == synchronous_run.id )); assert!(events.try_recv().is_err()); + + let snapshot = metrics.snapshot().expect("metrics snapshot"); + let counter = snapshot + .scope_metrics() + .flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics) + .find(|metric| metric.name() == HOOK_RUN_METRIC) + .expect("hook run counter"); + let AggregatedMetrics::U64(MetricData::Sum(sum)) = counter.data() else { + panic!("expected hook run counter"); + }; + assert_eq!(sum.data_points().map(SumDataPoint::value).sum::(), 3); + + let duration = snapshot + .scope_metrics() + .flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics) + .find(|metric| metric.name() == HOOK_RUN_DURATION_METRIC) + .expect("hook run duration histogram"); + let AggregatedMetrics::F64(MetricData::Histogram(histogram)) = duration.data() else { + panic!("expected hook run duration histogram"); + }; + assert_eq!( + histogram + .data_points() + .map(HistogramDataPoint::sum) + .sum::(), + 81.0, + ); } #[tokio::test] @@ -1237,6 +1308,7 @@ mod tests { event_name: HookEventName::Stop, handler_type: HookHandlerType::Command, execution_mode: HookExecutionMode::Sync, + builtin: false, scope: HookScope::Turn, source_path: test_path_buf("/tmp/hooks.json").abs(), source, diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 90f683248186..119697581c94 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -1,8 +1,11 @@ +use std::collections::HashMap; use std::fs; use std::path::Path; use anyhow::Context; use anyhow::Result; +use codex_config::HookStateToml; +use codex_config::McpServerConfig; use codex_config::test_support::CloudConfigBundleFixture; use codex_core::StartThreadOptions; use codex_core::TurnInput; @@ -26,6 +29,7 @@ use codex_protocol::models::ResponseItem; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::Op; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; @@ -64,6 +68,7 @@ use core_test_support::test_codex::test_codex; use core_test_support::test_codex::turn_permission_fields; use core_test_support::test_target_os; use core_test_support::wait_for_event; +use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; use serde_json::Value; use std::sync::Arc; @@ -4036,6 +4041,282 @@ async fn post_tool_use_exit_two_rejects_code_mode_tool_promise() -> Result<()> { .await } +enum CleanupHookDeclaration { + Inline, + File, +} + +enum CleanupHookResponse { + Success, + McpError, +} + +enum CleanupPluginState { + Enabled, + Disabled, +} + +enum CleanupHooksFeature { + Enabled, + Disabled, +} + +#[test_case::test_matrix( + [("computer-use", "node_repl"), ("unified-computer-use", "cua_repl")], + [CleanupHookDeclaration::Inline, CleanupHookDeclaration::File], + [CleanupHookResponse::Success, CleanupHookResponse::McpError], + [CleanupPluginState::Enabled], + [CleanupHooksFeature::Enabled] +)] +#[test_case::test_matrix( + [("computer-use", "node_repl"), ("unified-computer-use", "cua_repl")], + [CleanupHookDeclaration::Inline], + [CleanupHookResponse::Success], + [CleanupPluginState::Disabled], + [CleanupHooksFeature::Enabled] +)] +#[test_case::test_matrix( + [("computer-use", "node_repl"), ("unified-computer-use", "cua_repl")], + [CleanupHookDeclaration::Inline], + [CleanupHookResponse::Success], + [CleanupPluginState::Enabled, CleanupPluginState::Disabled], + [CleanupHooksFeature::Disabled] +)] +#[tokio::test] +async fn local_bundled_cleanup_hook_runs_without_saved_trust( + plugin: (&'static str, &'static str), + declaration: CleanupHookDeclaration, + hook_response: CleanupHookResponse, + plugin_state: CleanupPluginState, + hooks_feature: CleanupHooksFeature, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let (plugin_name, mcp_server_name) = plugin; + let plugin_enabled = matches!(plugin_state, CleanupPluginState::Enabled); + let hooks_enabled = matches!(hooks_feature, CleanupHooksFeature::Enabled); + let hook_response = match hook_response { + CleanupHookResponse::Success => { + serde_json::json!({ "content": [{ "type": "text", "text": "{}" }] }) + } + CleanupHookResponse::McpError => serde_json::json!({ + "isError": true, + "content": [{ + "type": "text", + "text": r#"{"decision":"block","reason":"error output must not continue the turn"}"#, + }], + }), + }; + let server = start_mock_server().await; + wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path("/repl")) + .respond_with(move |request: &wiremock::Request| { + let request: Value = serde_json::from_slice(&request.body).expect("MCP request"); + let result = match request["method"].as_str() { + Some("initialize") => serde_json::json!({ + "protocolVersion": request["params"]["protocolVersion"], + "capabilities": { "tools": {} }, + "serverInfo": { "name": mcp_server_name, "version": "1.0.0" }, + }), + Some("notifications/initialized") => return wiremock::ResponseTemplate::new(202), + Some("tools/list") => serde_json::json!({ "tools": [ + { "name": "turn_ended", "inputSchema": { "type": "object" } }, + { "name": "regular_stop", "inputSchema": { "type": "object" } }, + ] }), + Some("tools/call") => hook_response.clone(), + method => panic!("unexpected MCP request: {method:?}"), + }; + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": request["id"], + "result": result, + })) + }) + .mount(&server) + .await; + let response = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ]), + ) + .await; + + let home = Arc::new(TempDir::new()?); + if !hooks_enabled { + fs::write( + home.path().join("hooks.json"), + serde_json::to_vec(&serde_json::json!({ "hooks": { "Stop": [{ "hooks": [{ + "type": "mcp_tool", + "server": mcp_server_name, + "tool": "regular_stop", + }] }] } }))?, + )?; + } + let plugin_root = home + .path() + .join(format!("plugins/cache/openai-bundled/{plugin_name}/local")); + fs::create_dir_all(plugin_root.join(".codex-plugin"))?; + let hooks = serde_json::json!({ "hooks": { "Stop": [{ "hooks": [{ + "type": "mcp_tool", + "server": mcp_server_name, + "tool": "turn_ended", + "input": { + "hook_event_name": "${hook_event_name}", + "session_id": "${session_id}", + "turn_id": "${turn_id}", + }, + }] }] } }); + let (manifest_hooks, source_relative_path) = match declaration { + CleanupHookDeclaration::Inline => (hooks, "plugin.json#hooks[0]"), + CleanupHookDeclaration::File => { + fs::create_dir_all(plugin_root.join("hooks"))?; + fs::write( + plugin_root.join("hooks/hooks.json"), + serde_json::to_vec(&hooks)?, + )?; + (serde_json::json!("./hooks/hooks.json"), "hooks/hooks.json") + } + }; + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + serde_json::to_vec(&serde_json::json!({ + "name": plugin_name, + "hooks": manifest_hooks, + }))?, + )?; + let hook_key = codex_hooks::hook_key( + &format!("{plugin_name}@openai-bundled:{source_relative_path}"), + HookEventName::Stop, + /*group_index*/ 0, + /*handler_index*/ 0, + ); + fs::write( + home.path().join("config.toml"), + format!( + "[features]\nhooks = {hooks_enabled}\n\n[plugins.\"{plugin_name}@openai-bundled\"]\nenabled = {plugin_enabled}\n\n[hooks.state.\"{hook_key}\"]\nenabled = false\n" + ), + )?; + let repl_url = format!("{}/repl", server.uri()); + let mut builder = test_codex().with_home(home).with_config(move |config| { + for feature in [Feature::Plugins, Feature::CodexHooks] { + config.features.enable(feature).expect("enable feature"); + } + if !hooks_enabled { + trust_discovered_hooks(config); + config + .features + .disable(Feature::CodexHooks) + .expect("disable regular hooks"); + } + config + .features + .disable(Feature::ExecutorCapabilityDiscovery) + .expect("disable executor capability discovery"); + let repl: McpServerConfig = serde_json::from_value(serde_json::json!({ + "url": repl_url, + "environment_id": super::rmcp_client::remote_aware_environment_id(), + })) + .expect("valid MCP configuration"); + config + .mcp_servers + .set( + [(String::from(mcp_server_name), repl)] + .into_iter() + .collect(), + ) + .expect("configure MCP server"); + }); + let test = builder.build_with_auto_env(&server).await?; + assert!(!test.config.bypass_hook_trust); + assert_eq!( + test.config.features.enabled(Feature::CodexHooks), + hooks_enabled + ); + let mut expected_hook_states = HashMap::from([( + hook_key, + HookStateToml { + enabled: Some(false), + trusted_hash: None, + }, + )]); + if !hooks_enabled { + let regular_hooks = codex_hooks::list_hooks(codex_hooks::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(test.config.config_layer_stack.clone()), + ..Default::default() + }); + assert_eq!(regular_hooks.hooks.len(), 1); + for hook in regular_hooks.hooks { + expected_hook_states.insert( + hook.key, + HookStateToml { + enabled: None, + trusted_hash: Some(hook.current_hash), + }, + ); + } + } + assert_eq!( + codex_hooks::hook_states_from_stack(Some(&test.config.config_layer_stack)), + expected_hook_states + ); + wait_for_mcp_server(&test.codex, mcp_server_name).await?; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "finish this turn".into(), + text_elements: Vec::new(), + }])) + .await?; + let mut hook_notifications = Vec::new(); + wait_for_event(&test.codex, |event| { + if matches!(event, EventMsg::HookStarted(_) | EventMsg::HookCompleted(_)) { + hook_notifications.push(event.clone()); + } + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert!( + hook_notifications.is_empty(), + "unexpected cleanup hook notifications: {hook_notifications:?}" + ); + assert_eq!(response.requests().len(), 1); + + let calls = server + .received_requests() + .await + .unwrap_or_default() + .into_iter() + .filter(|request| request.url.path() == "/repl") + .filter_map(|request| serde_json::from_slice::(&request.body).ok()) + .filter(|request| request["method"] == "tools/call") + .map(|request| { + serde_json::json!({ + "name": request["params"]["name"], + "arguments": request["params"]["arguments"], + }) + }) + .collect::>(); + assert_eq!( + calls, + if plugin_enabled { + vec![serde_json::json!({ + "name": "turn_ended", + "arguments": { + "hook_event_name": "Stop", + "session_id": test.session_configured.thread_id.to_string(), + "turn_id": response.single_request().body_json()["client_metadata"]["turn_id"], + }, + })] + } else { + Vec::new() + } + ); + Ok(()) +} + #[tokio::test] async fn plugin_pre_tool_use_blocks_exec_command_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/engine/command_runner_tests.rs b/codex-rs/hooks/src/engine/command_runner_tests.rs index cb1ff333d406..caf2a6634050 100644 --- a/codex-rs/hooks/src/engine/command_runner_tests.rs +++ b/codex-rs/hooks/src/engine/command_runner_tests.rs @@ -53,6 +53,7 @@ async fn cmd_shell_runs_quoted_hook_command_path() { let command = format!(r#""{}" notify"#, hook_path.display()); let env = HashMap::new(); let handler = ConfiguredHandler { + builtin: false, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 10, @@ -102,6 +103,7 @@ async fn fast_exiting_hook_preserves_stdout_when_stdin_is_not_consumed() { let command = "echo hook-ran"; let env = HashMap::new(); let handler = ConfiguredHandler { + builtin: false, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 10, @@ -140,6 +142,7 @@ async fn command_hook_does_not_expose_configured_noise_auth_token() { ("CODEX_HOOK_SAFE_ENV".to_string(), "visible".to_string()), ]); let handler = ConfiguredHandler { + builtin: false, event_name: HookEventName::SessionStart, matcher: None, timeout_sec: 10, @@ -298,6 +301,7 @@ fn write_handler(temp: &TempDir, source: &str) -> ConfiguredHandler { let script_path = temp.path().join("async_hook.py"); std::fs::write(&script_path, source).expect("write async test hook"); ConfiguredHandler { + builtin: false, event_name: HookEventName::UserPromptSubmit, matcher: None, timeout_sec: 10, diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 656612d2fbe4..5003dbd24f08 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -17,6 +17,7 @@ use codex_config::RequirementSource; use codex_config::TomlValue; use codex_config::version_for_toml; use codex_plugin::PluginHookSource; +use codex_plugin::is_allowlisted_bundled_cleanup_hook; use codex_protocol::protocol::HookEventName; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; @@ -660,12 +661,22 @@ fn append_matcher_groups( status_message, additional_context_limit, } = normalized; - let current_hash = hook_hash(event_name, matcher, &group, config); + let current_hash = hook_hash(event_name, matcher, &group, &config); let key = crate::hook_key(&source.key_source, event_name, group_index, handler_index); let state = source.hook_states.get(&key); - let enabled = hook_enabled(source.is_managed, state); + let builtin = source.plugin_id.as_deref().is_some_and(|plugin_id| { + is_allowlisted_bundled_cleanup_hook( + plugin_id, + event_name, + group.matcher.as_deref(), + &config, + /*app_connector_id*/ None, + ) + }); + let enabled = hook_enabled(source.is_managed, builtin, state); let trusted_hash = hook_trusted_hash(source.is_managed, state); - let trust_status = hook_trust_status(source.is_managed, ¤t_hash, trusted_hash); + let trust_status = + hook_trust_status(source.is_managed, builtin, ¤t_hash, trusted_hash); let handler = match &kind { ConfiguredHandlerKind::Command { command, r#async, .. @@ -682,6 +693,7 @@ fn append_matcher_groups( }; hook_entries.push(HookListEntry { + builtin, key, event_name, handler, @@ -706,6 +718,7 @@ fn append_matcher_groups( )) { handlers.push(ConfiguredHandler { + builtin, event_name, matcher: matcher.map(ToOwned::to_owned), timeout_sec, @@ -763,11 +776,11 @@ fn hook_hash( event_name: codex_protocol::protocol::HookEventName, matcher: Option<&str>, group: &MatcherGroup, - normalized_handler: HookHandlerConfig, + normalized_handler: &HookHandlerConfig, ) -> String { let mut group = group.clone(); group.matcher = matcher.map(ToOwned::to_owned); - group.hooks = vec![normalized_handler]; + group.hooks = vec![normalized_handler.clone()]; let identity = NormalizedHookIdentity { event_name: crate::hook_event_key_label(event_name), group, @@ -780,10 +793,13 @@ fn hook_hash( fn hook_trust_status( is_managed: bool, + is_builtin: bool, current_hash: &str, trusted_hash: Option<&str>, ) -> HookTrustStatus { - if is_managed { + if is_builtin { + HookTrustStatus::Trusted + } else if is_managed { HookTrustStatus::Managed } else { match trusted_hash { @@ -794,8 +810,8 @@ fn hook_trust_status( } } -fn hook_enabled(is_managed: bool, state: Option<&HookStateToml>) -> bool { - is_managed || state.and_then(|state| state.enabled) != Some(false) +fn hook_enabled(is_managed: bool, is_builtin: bool, state: Option<&HookStateToml>) -> bool { + is_builtin || is_managed || state.and_then(|state| state.enabled) != Some(false) } fn hook_trusted_hash(is_managed: bool, state: Option<&HookStateToml>) -> Option<&str> { @@ -1238,6 +1254,7 @@ mod tests { assert_eq!( handlers, vec![ConfiguredHandler { + builtin: false, event_name: HookEventName::UserPromptSubmit, matcher: None, timeout_sec: 600, @@ -1277,6 +1294,7 @@ mod tests { assert_eq!( handlers, vec![ConfiguredHandler { + builtin: false, event_name: HookEventName::PreToolUse, matcher: Some("^Bash$".to_string()), timeout_sec: 600, diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index 8e77e9bbca3a..c5ee0d2ce1da 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -80,6 +80,7 @@ pub(crate) fn running_summary(handler: &ConfiguredHandler) -> HookRunSummary { unreachable!("executor-scoped hooks do not produce public hook summaries"); }; HookRunSummary { + builtin: handler.builtin, id: handler.run_id(), event_name: handler.event_name, handler_type: handler.handler_type(), @@ -234,6 +235,7 @@ pub(crate) fn completed_summary( unreachable!("executor-scoped hooks do not produce public hook summaries"); }; HookRunSummary { + builtin: handler.builtin, id: handler.run_id(), event_name: handler.event_name, handler_type: handler.handler_type(), @@ -348,6 +350,7 @@ mod tests { display_order: i64, ) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name, matcher: matcher.map(str::to_owned), timeout_sec: 5, diff --git a/codex-rs/hooks/src/engine/mcp_runner_tests.rs b/codex-rs/hooks/src/engine/mcp_runner_tests.rs index ceff78a1cf4e..a16613ea071d 100644 --- a/codex-rs/hooks/src/engine/mcp_runner_tests.rs +++ b/codex-rs/hooks/src/engine/mcp_runner_tests.rs @@ -91,6 +91,7 @@ async fn mcp_tool_results_use_command_hook_output_contract() { })) .expect("object input"); let handler = ConfiguredHandler { + builtin: false, event_name: HookEventName::PostToolUse, matcher: None, timeout_sec: 30, diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 67bb23644c89..deaf29679a8c 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -57,6 +57,8 @@ pub(crate) struct CommandShell { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct ConfiguredHandler { + /// Internally admitted cleanup hook, enabled independently of per-hook state. + pub builtin: bool, pub event_name: codex_protocol::protocol::HookEventName, pub matcher: Option, pub timeout_sec: u64, @@ -195,6 +197,8 @@ pub enum HookListEntryHandler { #[derive(Debug, Clone, PartialEq, Eq)] pub struct HookListEntry { + /// Builtin hooks remain available internally but are omitted from the public hooks list. + pub builtin: bool, pub key: String, pub event_name: HookEventName, pub handler: HookListEntryHandler, @@ -231,7 +235,7 @@ impl ClaudeHooksEngine { command_runtime: CommandHookRuntime, mcp_executor: Arc, ) -> Self { - if !enabled { + if !enabled && plugin_hook_sources.is_empty() { return Self { handlers: Vec::new(), warnings: Vec::new(), @@ -242,12 +246,18 @@ impl ClaudeHooksEngine { } let _ = schema_loader::generated_hook_schemas(); - let discovered = discovery::discover_handlers( + let mut discovered = discovery::discover_handlers( config_layer_stack, plugin_hook_sources, plugin_hook_load_warnings, bypass_hook_trust, ); + if !enabled { + discovered.handlers.retain(|handler| handler.builtin); + // Disabled ordinary hooks must not emit warnings or reject session startup. + discovered.warnings.clear(); + discovered.required_load_errors.clear(); + } Self { handlers: discovered.handlers, warnings: discovered.warnings, @@ -308,6 +318,7 @@ impl ClaudeHooksEngine { continue; } self.handlers.push(ConfiguredHandler { + builtin: true, event_name, matcher: None, timeout_sec: timeout_sec.unwrap_or(5).max(1), diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 9bf691d15535..a185e45029ff 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -90,6 +90,7 @@ fn permission_request_timeout_only_counts_synchronous_handlers() { ); let command = "echo synchronous permission hook"; let synchronous_handler = ConfiguredHandler { + builtin: false, event_name: HookEventName::PermissionRequest, matcher: None, timeout_sec: 5, @@ -252,24 +253,34 @@ fn required_hooks_stack( fn required_managed_hooks_allow_disabled_hooks_feature() { let temp = tempdir().expect("create temp dir"); let managed_hooks = - managed_hooks_for_current_platform(temp.path(), pre_tool_use_hook_events("echo managed")); + managed_hooks_for_current_platform(temp.path(), pre_tool_use_hook_events(" ")); let config_layer_stack = required_hooks_stack( managed_hooks, RequirementSource::LegacyManagedConfigTomlFromMdm, ); - let (hooks, _result_receiver) = crate::Hooks::new( - crate::HooksConfig { - feature_enabled: false, - config_layer_stack: Some(config_layer_stack), - ..Default::default() - }, - ThreadId::new(), - mcp_executor(), - ) - .expect("disabled hooks feature should not enforce managed requirements hooks"); + for plugin_hook_sources in [ + Vec::new(), + vec![bundled_cleanup_source( + "browser@openai-bundled", + "node_repl", + "Stop", + )], + ] { + let (hooks, _result_receiver) = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: false, + config_layer_stack: Some(config_layer_stack.clone()), + plugin_hook_sources, + ..Default::default() + }, + ThreadId::new(), + mcp_executor(), + ) + .expect("disabled hooks feature should not enforce managed requirements hooks"); - assert!(hooks.startup_warnings().is_empty()); + assert!(hooks.startup_warnings().is_empty()); + } } #[test] @@ -1717,6 +1728,277 @@ fn malformed_hooks_json_is_reported_as_startup_warning() { assert!(engine.warnings()[0].contains("unknown field `SessionStart`")); } +fn bundled_cleanup_source(plugin_id: &str, server: &str, event: &str) -> PluginHookSource { + let plugin_root = cwd().join("bundled-plugin"); + PluginHookSource { + plugin_id: PluginId::parse(plugin_id).expect("plugin ID"), + plugin_data_root: plugin_root.join("data"), + source_path: plugin_root.join(".codex-plugin/plugin.json"), + source_relative_path: "plugin.json#hooks[0]".to_string(), + plugin_root, + hooks: serde_json::from_value(serde_json::json!({ + (event): [{"hooks": [{ + "type": "mcp_tool", + "server": server, + "tool": "turn_ended", + "input": {"turn_id": "${turn_id}"}, + }]}], + })) + .expect("cleanup hooks"), + } +} + +#[test] +fn bundled_cleanup_hooks_are_trusted_without_saved_hashes() { + for (plugin_id, server) in [ + ("browser@openai-bundled", "node_repl"), + ("chrome@openai-bundled", "node_repl"), + ("chrome-dev@openai-bundled", "node_repl"), + ("chrome-internal@openai-bundled", "node_repl"), + ("computer-use@openai-bundled", "node_repl"), + ("unified-computer-use@openai-bundled", "cua_repl"), + ] { + for event in ["Stop", "Interrupt", "SubagentStop"] { + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + vec![bundled_cleanup_source(plugin_id, server, event)], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert_eq!(discovered.handlers.len(), 1, "{plugin_id} {event}"); + assert_eq!( + discovered + .hook_entries + .iter() + .map(|entry| ( + entry.plugin_id.as_deref(), + entry.trust_status, + entry.enabled, + entry.is_managed, + entry.builtin, + )) + .collect::>(), + vec![(Some(plugin_id), HookTrustStatus::Trusted, true, false, true)], + "{plugin_id} {event}" + ); + assert!(discovered.handlers[0].builtin, "{plugin_id} {event}"); + } + } +} + +#[test] +fn bundled_cleanup_trust_does_not_extend_to_other_handlers() { + let mut source = bundled_cleanup_source("browser@openai-bundled", "node_repl", "Stop"); + let cleanup = source.hooks.stop[0].hooks[0].clone(); + source.hooks.stop[0].hooks.extend([ + HookHandlerConfig::McpTool { + server: "node_repl".to_string(), + tool: "evaluate".to_string(), + input: Default::default(), + timeout_sec: None, + status_message: None, + }, + HookHandlerConfig::McpTool { + server: "other_server".to_string(), + tool: "turn_ended".to_string(), + input: Default::default(), + timeout_sec: None, + status_message: None, + }, + HookHandlerConfig::Command { + command: "echo cleanup".to_string(), + command_windows: None, + timeout_sec: None, + r#async: false, + status_message: None, + additional_context_limit: None, + }, + ]); + source.hooks.stop.push(MatcherGroup { + matcher: Some("Bash".to_string()), + hooks: vec![cleanup], + }); + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + vec![source], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert_eq!(discovered.handlers.len(), 1); + assert_eq!( + discovered + .hook_entries + .iter() + .map(|entry| (entry.trust_status, entry.builtin)) + .collect::>(), + vec![ + (HookTrustStatus::Trusted, true), + (HookTrustStatus::Untrusted, false), + (HookTrustStatus::Untrusted, false), + (HookTrustStatus::Untrusted, false), + (HookTrustStatus::Untrusted, false), + ] + ); +} + +#[test] +fn bundled_cleanup_trust_requires_matching_plugin_server_and_event() { + for (plugin_id, server, event) in [ + ("other@openai-bundled", "node_repl", "Stop"), + ("browser@other", "node_repl", "Stop"), + ("browser@openai-bundled-alpha", "node_repl", "Stop"), + ("browser@openai-bundled", "node_repl", "PreToolUse"), + ("browser@openai-bundled", "cua_repl", "Stop"), + ("unified-computer-use@openai-bundled", "node_repl", "Stop"), + ("unified-computer-use@other", "cua_repl", "Stop"), + ( + "unified-computer-use@openai-bundled", + "cua_repl", + "PreToolUse", + ), + ] { + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + vec![bundled_cleanup_source(plugin_id, server, event)], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert!(discovered.handlers.is_empty(), "{plugin_id} {event}"); + assert_eq!(discovered.hook_entries.len(), 1); + assert_eq!( + discovered.hook_entries[0].trust_status, + HookTrustStatus::Untrusted + ); + } +} + +#[test] +fn local_hosted_app_cleanup_hooks_require_saved_trust() { + let mut source = bundled_cleanup_source("browser@openai-curated-remote", "codex_apps", "Stop"); + source.hooks.stop[0].hooks[0] = HookHandlerConfig::McpTool { + server: "codex_apps".to_string(), + tool: "browser.turn_ended".to_string(), + input: Default::default(), + timeout_sec: None, + status_message: None, + }; + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + vec![source], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert!(discovered.handlers.is_empty()); + assert_eq!( + discovered + .hook_entries + .iter() + .map(|entry| entry.trust_status) + .collect::>(), + vec![HookTrustStatus::Untrusted] + ); +} + +#[test] +fn builtin_cleanup_ignores_disablement_but_preserves_managed_only_policy() { + let source = bundled_cleanup_source("browser@openai-bundled", "node_repl", "Stop"); + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + vec![source.clone()], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + let expected_entry = discovered.hook_entries[0].clone(); + let disabled_stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: cwd().join("config.toml"), + profile: None, + }, + config_with_hook_state(&expected_entry.key, /*enabled*/ false), + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("disabled hook config"); + let disabled = super::discovery::discover_handlers( + Some(&disabled_stack), + vec![source.clone()], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert_eq!(disabled.handlers, discovered.handlers); + assert_eq!(disabled.hook_entries, vec![expected_entry]); + + let (requirements, requirements_toml) = requirements_with_managed_hooks_only( + /*allow_managed_hooks_only*/ true, /*managed_hooks*/ None, + ); + let managed_stack = ConfigLayerStack::new(Vec::new(), requirements, requirements_toml) + .expect("managed-only config"); + let managed_only = super::discovery::discover_handlers( + Some(&managed_stack), + vec![source.clone()], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert!(managed_only.handlers.is_empty()); + assert!(managed_only.hook_entries.is_empty()); + + for (stack, expected) in [ + (&disabled_stack, discovered.handlers), + (&managed_stack, Vec::new()), + ] { + let feature_disabled = ClaudeHooksEngine::new( + /*enabled*/ false, + /*bypass_hook_trust*/ false, + Some(stack), + vec![source.clone()], + Vec::new(), + command_runtime(CommandShell { + program: String::new(), + args: Vec::new(), + }), + mcp_executor(), + ); + assert_eq!(feature_disabled.handlers, expected); + } +} + +#[test] +fn disabled_hooks_feature_keeps_builtin_cleanup_but_not_trusted_plugin_hooks() { + let mut source = bundled_cleanup_source("browser@openai-bundled", "node_repl", "Stop"); + source.hooks.stop[0].hooks.push(HookHandlerConfig::Command { + command: "echo ordinary hook".to_string(), + command_windows: None, + timeout_sec: None, + r#async: false, + status_message: None, + additional_context_limit: None, + }); + let stack = trusted_plugin_hook_stack(cwd().join("config.toml"), &[source.clone()]); + let discovered = super::discovery::discover_handlers( + Some(&stack), + vec![source.clone()], + Vec::new(), + /*bypass_hook_trust*/ false, + ); + assert_eq!(discovered.handlers.len(), 2); + + let engine = ClaudeHooksEngine::new( + /*enabled*/ false, + /*bypass_hook_trust*/ false, + Some(&stack), + vec![source], + Vec::new(), + command_runtime(CommandShell { + program: String::new(), + args: Vec::new(), + }), + mcp_executor(), + ); + assert_eq!(engine.handlers, vec![discovered.handlers[0].clone()]); +} + #[tokio::test] async fn plugin_hook_sources_run_with_plugin_env_and_plugin_source() { let temp = tempdir().expect("create temp dir"); @@ -2030,6 +2312,7 @@ fn executor_stop_hook_fixture() -> ( assert_eq!( engine.handlers, vec![ConfiguredHandler { + builtin: true, event_name: HookEventName::Stop, matcher: None, timeout_sec: 5, @@ -2118,6 +2401,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { ); engine.handlers.push(ConfiguredHandler { + builtin: false, event_name: HookEventName::Stop, matcher: None, timeout_sec: 30, @@ -2152,6 +2436,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { ); engine.handlers.push(ConfiguredHandler { + builtin: false, event_name: HookEventName::Stop, matcher: None, timeout_sec: 30, diff --git a/codex-rs/hooks/src/events/compact.rs b/codex-rs/hooks/src/events/compact.rs index bf9d5634845c..82bd6c6cf55d 100644 --- a/codex-rs/hooks/src/events/compact.rs +++ b/codex-rs/hooks/src/events/compact.rs @@ -523,6 +523,7 @@ mod tests { fn handler(event_name: HookEventName) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name, matcher: None, timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/interrupt_tests.rs b/codex-rs/hooks/src/events/interrupt_tests.rs index 5e1c61776eae..3d8642c8457b 100644 --- a/codex-rs/hooks/src/events/interrupt_tests.rs +++ b/codex-rs/hooks/src/events/interrupt_tests.rs @@ -98,6 +98,7 @@ fn error(text: &str) -> HookOutputEntry { fn handler() -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name: HookEventName::Interrupt, matcher: None, timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index f2a36e433876..aeb0ed20a7a1 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -589,6 +589,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name: HookEventName::PostToolUse, matcher: Some("^Bash$".to_string()), timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 137731357a5f..c1baebd4f01f 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -773,6 +773,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name: HookEventName::PreToolUse, matcher: Some("^Bash$".to_string()), timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/session_end_tests.rs b/codex-rs/hooks/src/events/session_end_tests.rs index 2197bae5a2be..0c100ee4a4c6 100644 --- a/codex-rs/hooks/src/events/session_end_tests.rs +++ b/codex-rs/hooks/src/events/session_end_tests.rs @@ -60,6 +60,7 @@ fn session_end_ignores_successful_output() { fn handler(matcher: Option<&str>) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name: HookEventName::SessionEnd, matcher: matcher.map(str::to_string), timeout_sec: 2, diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index 07604c30a43c..82993f256ead 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -540,6 +540,7 @@ mod tests { fn handler_for(event_name: HookEventName) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name, matcher: None, timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/stop.rs b/codex-rs/hooks/src/events/stop.rs index 89a59a65efce..be2be9100e35 100644 --- a/codex-rs/hooks/src/events/stop.rs +++ b/codex-rs/hooks/src/events/stop.rs @@ -692,6 +692,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name: HookEventName::Stop, matcher: None, timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/user_prompt_submit.rs b/codex-rs/hooks/src/events/user_prompt_submit.rs index c70deba2f7ba..9124431bc194 100644 --- a/codex-rs/hooks/src/events/user_prompt_submit.rs +++ b/codex-rs/hooks/src/events/user_prompt_submit.rs @@ -461,6 +461,7 @@ mod tests { fn handler_with_async(r#async: bool) -> ConfiguredHandler { ConfiguredHandler { + builtin: false, event_name: HookEventName::UserPromptSubmit, matcher: None, timeout_sec: 5, diff --git a/codex-rs/plugin/src/bundled_hooks.rs b/codex-rs/plugin/src/bundled_hooks.rs new file mode 100644 index 000000000000..a39644e4274f --- /dev/null +++ b/codex-rs/plugin/src/bundled_hooks.rs @@ -0,0 +1,152 @@ +//! Temporary cleanup-hook allowlist shared by local and executor plugin discovery. +//! This narrowly authorizes known MCP cleanup calls; it does not verify plugin signatures. + +use codex_config::HookHandlerConfig; +use codex_protocol::protocol::HookEventName; + +struct BundledHook { + plugin_id: &'static str, + events: &'static [HookEventName], + target: BundledHookTarget, +} + +enum BundledHookTarget { + McpServer { + server: &'static str, + tool: &'static str, + }, + App { + server: &'static str, + connector_id: &'static str, + tool: &'static str, + }, +} + +// Keep unsigned plugin exceptions together so they can be removed as signing lands. +const ALLOWLISTED_BUNDLED_HOOKS: &[BundledHook] = &[ + BundledHook { + plugin_id: "browser@openai-bundled", + events: &[ + HookEventName::Stop, + HookEventName::Interrupt, + HookEventName::SubagentStop, + ], + target: BundledHookTarget::McpServer { + server: "node_repl", + tool: "turn_ended", + }, + }, + BundledHook { + plugin_id: "chrome@openai-bundled", + events: &[ + HookEventName::Stop, + HookEventName::Interrupt, + HookEventName::SubagentStop, + ], + target: BundledHookTarget::McpServer { + server: "node_repl", + tool: "turn_ended", + }, + }, + BundledHook { + plugin_id: "chrome-dev@openai-bundled", + events: &[ + HookEventName::Stop, + HookEventName::Interrupt, + HookEventName::SubagentStop, + ], + target: BundledHookTarget::McpServer { + server: "node_repl", + tool: "turn_ended", + }, + }, + BundledHook { + plugin_id: "chrome-internal@openai-bundled", + events: &[ + HookEventName::Stop, + HookEventName::Interrupt, + HookEventName::SubagentStop, + ], + target: BundledHookTarget::McpServer { + server: "node_repl", + tool: "turn_ended", + }, + }, + BundledHook { + plugin_id: "computer-use@openai-bundled", + events: &[ + HookEventName::Stop, + HookEventName::Interrupt, + HookEventName::SubagentStop, + ], + target: BundledHookTarget::McpServer { + server: "node_repl", + tool: "turn_ended", + }, + }, + BundledHook { + plugin_id: "unified-computer-use@openai-bundled", + events: &[ + HookEventName::Stop, + HookEventName::Interrupt, + HookEventName::SubagentStop, + ], + target: BundledHookTarget::McpServer { + server: "cua_repl", + tool: "turn_ended", + }, + }, + BundledHook { + plugin_id: "browser@openai-curated-remote", + events: &[HookEventName::Stop, HookEventName::SubagentStop], + target: BundledHookTarget::App { + server: "codex_apps", + connector_id: "connector_openai_browser", + tool: "browser.turn_ended", + }, + }, +]; + +/// Matches a temporary unsigned cleanup exception. App targets additionally require the +/// connector identity for this handler's server and tool from the caller's enabled tool +/// catalog; callers without that catalog must pass `None`. +pub fn is_allowlisted_bundled_cleanup_hook( + plugin_id: &str, + event: HookEventName, + matcher: Option<&str>, + handler: &HookHandlerConfig, + app_connector_id: Option<&str>, +) -> bool { + let HookHandlerConfig::McpTool { + server, + tool, + input, + .. + } = handler + else { + return false; + }; + + matcher.is_none() + && ALLOWLISTED_BUNDLED_HOOKS.iter().any(|hook| { + hook.plugin_id == plugin_id + && hook.events.contains(&event) + && match hook.target { + BundledHookTarget::McpServer { + server: expected_server, + tool: expected_tool, + } => server == expected_server && tool == expected_tool, + BundledHookTarget::App { + server: expected_server, + connector_id, + tool: expected_tool, + } => { + // Raw Apps tool names can collide; require the registered connector. + server == expected_server + && tool == expected_tool + && input.is_empty() + && app_connector_id == Some(connector_id) + } + } + }) +} diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs index 177520552306..0ba937589706 100644 --- a/codex-rs/plugin/src/lib.rs +++ b/codex-rs/plugin/src/lib.rs @@ -4,11 +4,13 @@ use std::collections::HashSet; pub use codex_utils_plugins::mention_syntax; +mod bundled_hooks; mod load_outcome; pub mod manifest; mod plugin_id; mod provider; +pub use bundled_hooks::is_allowlisted_bundled_cleanup_hook; use codex_config::HookEventsToml; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index d4d236d963c2..9c59568b0b93 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1663,6 +1663,11 @@ pub struct HookOutputEntry { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub struct HookRunSummary { + /// Internal classification used to suppress lifecycle notifications without losing telemetry. + #[serde(skip)] + #[schemars(skip)] + #[ts(skip)] + pub builtin: bool, pub id: String, pub event_name: HookEventName, pub handler_type: HookHandlerType, @@ -4436,6 +4441,43 @@ mod tests { Ok(()) } + #[test] + fn hook_builtin_classification_stays_internal() -> Result<()> { + let wire = json!({ + "id": "cleanup-hook", + "event_name": "stop", + "handler_type": "mcp_tool", + "execution_mode": "sync", + "scope": "turn", + "source_path": test_path_buf("/tmp/hooks.json").abs(), + "source": "plugin", + "display_order": 0, + "status": "completed", + "status_message": null, + "started_at": 10, + "completed_at": 11, + "duration_ms": 1000, + "entries": [], + }); + let mut run: HookRunSummary = serde_json::from_value(wire.clone())?; + assert!(!run.builtin); + run.builtin = true; + assert_eq!(serde_json::to_value(run)?, wire); + + let mut untrusted_wire = wire; + untrusted_wire["builtin"] = json!(true); + assert!(!serde_json::from_value::(untrusted_wire)?.builtin); + let schema = serde_json::to_value(schemars::schema_for!(HookRunSummary))?; + assert!( + !schema["properties"] + .as_object() + .expect("hook properties") + .contains_key("builtin") + ); + assert!(!HookRunSummary::decl().contains("builtin:")); + Ok(()) + } + #[test] fn feature_thread_source_serializes_as_its_app_owned_label() -> Result<()> { let source = ThreadSource::Feature("automation".to_string());