From 907bd323225049492f1c0316793cef83b5367378 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 15 Jun 2026 04:21:42 +0000 Subject: [PATCH] load app-bundled internal hooks --- codex-rs/Cargo.lock | 1 + codex-rs/analytics/src/events.rs | 9 +- codex-rs/analytics/src/lib.rs | 2 + .../src/protocol/v2/hook.rs | 3 + .../src/protocol/v2/shared.rs | 9 +- codex-rs/core-plugins/Cargo.toml | 1 + .../core-plugins/src/app_bundled_internal.rs | 146 +++++++++++ .../src/app_bundled_internal_tests.rs | 103 ++++++++ codex-rs/core-plugins/src/lib.rs | 1 + codex-rs/core-plugins/src/loader.rs | 26 +- codex-rs/core-plugins/src/manager.rs | 25 +- codex-rs/core/src/hook_runtime.rs | 93 ++++--- codex-rs/core/tests/suite/hooks.rs | 231 ++++++++++++++++++ codex-rs/hooks/src/declarations.rs | 6 +- codex-rs/hooks/src/engine/discovery.rs | 64 +++-- codex-rs/hooks/src/engine/mod.rs | 10 +- codex-rs/hooks/src/engine/mod_tests.rs | 125 ++++++++++ codex-rs/plugin/src/lib.rs | 2 + codex-rs/protocol/src/protocol.rs | 1 + 19 files changed, 780 insertions(+), 78 deletions(-) create mode 100644 codex-rs/core-plugins/src/app_bundled_internal.rs create mode 100644 codex-rs/core-plugins/src/app_bundled_internal_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3a1bec2a27..99e6593d52 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2715,6 +2715,7 @@ dependencies = [ "codex-app-server-protocol", "codex-config", "codex-core-skills", + "codex-desktop-installation", "codex-exec-server", "codex-git-utils", "codex-hooks", diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index eed972852a..c06a570f43 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -1101,13 +1101,13 @@ pub(crate) fn codex_hook_run_metadata( thread_id: Some(tracking.thread_id.clone()), turn_id: Some(tracking.turn_id.clone()), model_slug: Some(tracking.model_slug.clone()), - hook_name: Some(analytics_hook_event_name(hook.event_name).to_owned()), - hook_source: Some(analytics_hook_source(hook.hook_source)), + hook_name: Some(hook_event_name(hook.event_name).to_owned()), + hook_source: Some(hook_source_name(hook.hook_source)), status: Some(analytics_hook_status(hook.status)), } } -fn analytics_hook_event_name(event_name: HookEventName) -> &'static str { +pub fn hook_event_name(event_name: HookEventName) -> &'static str { match event_name { HookEventName::PreToolUse => "PreToolUse", HookEventName::PermissionRequest => "PermissionRequest", @@ -1122,7 +1122,7 @@ fn analytics_hook_event_name(event_name: HookEventName) -> &'static str { } } -fn analytics_hook_source(source: HookSource) -> &'static str { +pub fn hook_source_name(source: HookSource) -> &'static str { match source { HookSource::System => "system", HookSource::User => "user", @@ -1130,6 +1130,7 @@ fn analytics_hook_source(source: HookSource) -> &'static str { HookSource::Mdm => "mdm", HookSource::SessionFlags => "session_flags", HookSource::Plugin => "plugin", + HookSource::AppBundledInternal => "app_bundled_internal", HookSource::CloudRequirements => "cloud_requirements", HookSource::CloudManagedConfig => "cloud_managed_config", HookSource::LegacyManagedConfigFile => "legacy_managed_config_file", diff --git a/codex-rs/analytics/src/lib.rs b/codex-rs/analytics/src/lib.rs index 6e47232754..e6e4c9176e 100644 --- a/codex-rs/analytics/src/lib.rs +++ b/codex-rs/analytics/src/lib.rs @@ -21,6 +21,8 @@ pub use events::GuardianReviewSessionKind; pub use events::GuardianReviewTerminalStatus; pub use events::GuardianReviewTrackContext; pub use events::GuardianReviewedAction; +pub use events::hook_event_name; +pub use events::hook_source_name; pub use facts::AcceptedLineFingerprint; pub use facts::AnalyticsJsonRpcError; pub use facts::AppInvocation; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/hook.rs b/codex-rs/app-server-protocol/src/protocol/v2/hook.rs index b8f47276a6..e251aaa28f 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/hook.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/hook.rs @@ -53,6 +53,9 @@ v2_enum_from_core!( LegacyManagedConfigMdm, Unknown, } + aliases { + AppBundledInternal => Plugin, + } ); v2_enum_from_core!( diff --git a/codex-rs/app-server-protocol/src/protocol/v2/shared.rs b/codex-rs/app-server-protocol/src/protocol/v2/shared.rs index cb881d6267..17c96734ae 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/shared.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/shared.rs @@ -24,6 +24,10 @@ macro_rules! v2_enum_from_core { pub enum $Name:ident from $Src:path { $( $(#[$variant_meta:meta])* $Variant:ident ),+ $(,)? } + // Map private core-only variants onto existing public wire variants. + $(aliases { + $( $SourceVariant:ident => $TargetVariant:ident ),+ $(,)? + })? ) => { #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] $(#[$enum_meta])* @@ -41,7 +45,10 @@ macro_rules! v2_enum_from_core { impl From<$Src> for $Name { fn from(value: $Src) -> Self { - match value { $( <$Src>::$Variant => $Name::$Variant ),+ } + match value { + $( <$Src>::$Variant => $Name::$Variant ),+, + $( $( <$Src>::$SourceVariant => $Name::$TargetVariant ),+ )? + } } } }; diff --git a/codex-rs/core-plugins/Cargo.toml b/codex-rs/core-plugins/Cargo.toml index 1d361559aa..e7d9e3d637 100644 --- a/codex-rs/core-plugins/Cargo.toml +++ b/codex-rs/core-plugins/Cargo.toml @@ -18,6 +18,7 @@ codex-analytics = { workspace = true } codex-app-server-protocol = { workspace = true } codex-config = { workspace = true } codex-core-skills = { workspace = true } +codex-desktop-installation = { workspace = true } codex-exec-server = { workspace = true } codex-git-utils = { workspace = true } codex-hooks = { workspace = true } diff --git a/codex-rs/core-plugins/src/app_bundled_internal.rs b/codex-rs/core-plugins/src/app_bundled_internal.rs new file mode 100644 index 0000000000..7118d68ece --- /dev/null +++ b/codex-rs/core-plugins/src/app_bundled_internal.rs @@ -0,0 +1,146 @@ +use std::path::PathBuf; +use std::sync::OnceLock; + +use codex_desktop_installation::DesktopResources; +use codex_desktop_installation::locate_current_or_installed_resources; +use codex_plugin::PluginHookSource; +use codex_plugin::PluginId; +use codex_protocol::protocol::HookSource; +use codex_utils_absolute_path::AbsolutePathBuf; +use serde::Deserialize; +use tracing::warn; + +use crate::OPENAI_BUNDLED_MARKETPLACE_NAME; +use crate::loader::load_plugin_hooks; +use crate::manifest::load_plugin_manifest; + +const BUNDLED_MARKETPLACE_PATH: &str = "plugins/openai-bundled/.agents/plugins/marketplace.json"; +// Standalone CLI discovery may invoke LaunchServices or PowerShell, so do it once per process. +static DESKTOP_RESOURCES: OnceLock> = OnceLock::new(); + +pub(crate) fn is_app_bundled_plugin(plugin_id: &PluginId) -> bool { + plugin_id.marketplace_name == OPENAI_BUNDLED_MARKETPLACE_NAME +} + +pub(crate) async fn load_app_bundled_internal_hooks( + plugin_id: &PluginId, + plugin_data_root: &AbsolutePathBuf, +) -> Vec { + let worker_plugin_id = plugin_id.clone(); + let plugin_data_root = plugin_data_root.clone(); + let worker = tokio::task::spawn_blocking(move || { + let resources = DESKTOP_RESOURCES + .get_or_init(|| { + locate_current_or_installed_resources().map_err(|error| error.to_string()) + }) + .as_ref() + .map_err(Clone::clone)?; + load_app_bundled_internal_hooks_from_resources( + resources, + &worker_plugin_id, + &plugin_data_root, + ) + }) + .await; + let result = match worker { + Ok(result) => result, + Err(error) => Err(format!("Desktop discovery worker failed: {error}")), + }; + match result { + Ok(sources) => sources, + Err(error) => { + warn!( + diagnostic_code = "app_bundled_internal_hook_load_failed", + plugin_id = %plugin_id.as_key(), + error, + "app-bundled internal hooks failed closed" + ); + Vec::new() + } + } +} + +/// Loads this plugin's hooks only from the located Desktop resources root. +pub(crate) fn load_app_bundled_internal_hooks_from_resources( + resources: &DesktopResources, + plugin_id: &PluginId, + plugin_data_root: &AbsolutePathBuf, +) -> Result, String> { + if !is_app_bundled_plugin(plugin_id) { + return Err("plugin is not from the app-bundled marketplace".to_string()); + } + + let marketplace_path = resources + .contained_file(BUNDLED_MARKETPLACE_PATH) + .map_err(|error| format!("invalid bundled marketplace: {error}"))?; + let contents = std::fs::read_to_string(marketplace_path.as_path()) + .map_err(|error| format!("failed to read bundled marketplace: {error}"))?; + let marketplace: BundledMarketplace = serde_json::from_str(&contents) + .map_err(|error| format!("failed to parse bundled marketplace: {error}"))?; + if marketplace.name != OPENAI_BUNDLED_MARKETPLACE_NAME { + return Err("bundled marketplace has the wrong name".to_string()); + } + + let mut matching_plugins = marketplace + .plugins + .into_iter() + .filter(|plugin| plugin.name == plugin_id.plugin_name); + let plugin = matching_plugins + .next() + .ok_or_else(|| "plugin is missing from the bundled marketplace".to_string())?; + if matching_plugins.next().is_some() { + return Err("bundled marketplace contains duplicate plugin entries".to_string()); + } + let expected_source = format!("./plugins/{}", plugin_id.plugin_name); + if plugin.source.source != "local" || plugin.source.path != expected_source { + return Err("bundled plugin has an unexpected source path".to_string()); + } + + let plugin_relative = PathBuf::from("plugins") + .join(OPENAI_BUNDLED_MARKETPLACE_NAME) + .join("plugins") + .join(&plugin_id.plugin_name); + let plugin_root = resources + .contained_directory(plugin_relative) + .map_err(|error| format!("invalid bundled plugin root: {error}"))?; + let manifest = load_plugin_manifest(plugin_root.as_path()) + .ok_or_else(|| "bundled plugin manifest is missing or invalid".to_string())?; + if manifest.name != plugin_id.plugin_name { + return Err("bundled plugin manifest has the wrong name".to_string()); + } + + let (mut sources, warnings) = + load_plugin_hooks(&plugin_root, plugin_id, plugin_data_root, &manifest.paths); + if !warnings.is_empty() { + return Err(format!( + "failed to load bundled plugin hooks: {}", + warnings.join("; ") + )); + } + for source in &mut sources { + source.source = HookSource::AppBundledInternal; + } + Ok(sources) +} + +#[derive(Deserialize)] +struct BundledMarketplace { + name: String, + plugins: Vec, +} + +#[derive(Deserialize)] +struct BundledMarketplacePlugin { + name: String, + source: BundledMarketplacePluginSource, +} + +#[derive(Deserialize)] +struct BundledMarketplacePluginSource { + source: String, + path: String, +} + +#[cfg(test)] +#[path = "app_bundled_internal_tests.rs"] +mod tests; diff --git a/codex-rs/core-plugins/src/app_bundled_internal_tests.rs b/codex-rs/core-plugins/src/app_bundled_internal_tests.rs new file mode 100644 index 0000000000..5063a8b0cd --- /dev/null +++ b/codex-rs/core-plugins/src/app_bundled_internal_tests.rs @@ -0,0 +1,103 @@ +use std::fs; +use std::path::Path; +use std::path::PathBuf; + +use codex_desktop_installation::DesktopResources; +use codex_plugin::PluginId; +use codex_utils_absolute_path::AbsolutePathBuf; +use serde_json::json; +use tempfile::TempDir; + +use super::*; + +struct Fixture { + _temp: TempDir, + marketplace_path: PathBuf, + resources: DesktopResources, + plugin_data_root: AbsolutePathBuf, +} + +fn fixture() -> Fixture { + let temp = tempfile::tempdir().expect("temp dir"); + let resources_root = temp.path().join("resources"); + let marketplace_path = resources_root.join(BUNDLED_MARKETPLACE_PATH); + write( + &marketplace_path, + &json!({ + "name": "openai-bundled", + "plugins": [{ + "name": "computer-use", + "source": {"source": "local", "path": "./plugins/computer-use"} + }] + }) + .to_string(), + ); + + let plugin_data_root = temp.path().join("data/computer-use"); + fs::create_dir_all(&plugin_data_root).expect("plugin data root"); + + Fixture { + resources: DesktopResources::from_trusted_path(resources_root).expect("Desktop resources"), + plugin_data_root: AbsolutePathBuf::try_from(plugin_data_root) + .expect("absolute plugin data root"), + marketplace_path, + _temp: temp, + } +} + +fn write(path: &Path, contents: &str) { + fs::create_dir_all(path.parent().expect("file parent")).expect("create parent"); + fs::write(path, contents).expect("write fixture"); +} + +fn replace_marketplace(fixture: &Fixture, marketplace: serde_json::Value) { + write(&fixture.marketplace_path, &marketplace.to_string()); +} + +fn load(fixture: &Fixture) -> Result, String> { + let plugin_id = PluginId::parse("computer-use@openai-bundled").expect("plugin id"); + load_app_bundled_internal_hooks_from_resources( + &fixture.resources, + &plugin_id, + &fixture.plugin_data_root, + ) +} + +#[test] +fn marketplace_identity_and_plugin_source_must_match() { + let cases = [ + ( + "wrong marketplace", + json!({"name": "spoofed", "plugins": []}), + ), + ( + "wrong source", + json!({ + "name": "openai-bundled", + "plugins": [{ + "name": "computer-use", + "source": {"source": "local", "path": "./plugins/other"} + }] + }), + ), + ]; + + for (label, marketplace) in cases { + let fixture = fixture(); + replace_marketplace(&fixture, marketplace); + load(&fixture).expect_err(label); + } +} + +#[test] +fn non_bundled_marketplace_cannot_request_internal_hook_loading() { + let fixture = fixture(); + let plugin_id = PluginId::parse("computer-use@spoofed").expect("plugin id"); + + load_app_bundled_internal_hooks_from_resources( + &fixture.resources, + &plugin_id, + &fixture.plugin_data_root, + ) + .expect_err("wrong marketplace must be rejected"); +} diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 620ec2086b..e8204a3238 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -1,3 +1,4 @@ +mod app_bundled_internal; mod discoverable; pub mod installed_marketplaces; pub mod loader; diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index ef447e9c1c..3025c4a39a 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -1,4 +1,6 @@ use crate::OPENAI_CURATED_MARKETPLACE_NAME; +use crate::app_bundled_internal::is_app_bundled_plugin; +use crate::app_bundled_internal::load_app_bundled_internal_hooks; use crate::manifest::PluginManifestHooks; use crate::manifest::PluginManifestPaths; use crate::manifest::load_plugin_manifest; @@ -34,6 +36,7 @@ use codex_plugin::PluginIdError; use codex_plugin::PluginLoadOutcome; use codex_plugin::PluginTelemetryMetadata; use codex_plugin::app_connector_ids_from_declarations; +use codex_protocol::protocol::HookSource; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -706,12 +709,21 @@ async fn load_plugin( } PluginLoadScope::HooksOnly => {} } - let (hook_sources, hook_load_warnings) = load_plugin_hooks( - &plugin_root, - &loaded_plugin_id, - &store.plugin_data_root(&loaded_plugin_id), - manifest_paths, - ); + let plugin_data_root = store.plugin_data_root(&loaded_plugin_id); + // Hooks load from Desktop resources; runtime-variant plugin capabilities remain cache-backed. + let (hook_sources, hook_load_warnings) = if is_app_bundled_plugin(&loaded_plugin_id) { + ( + load_app_bundled_internal_hooks(&loaded_plugin_id, &plugin_data_root).await, + Vec::new(), + ) + } else { + load_plugin_hooks( + &plugin_root, + &loaded_plugin_id, + &plugin_data_root, + manifest_paths, + ) + }; loaded_plugin.hook_sources = hook_sources; loaded_plugin.hook_load_warnings = hook_load_warnings; loaded_plugin @@ -914,6 +926,7 @@ pub fn load_plugin_hooks( source_path: manifest_path.clone(), source_relative_path: format!("plugin.json#hooks[{index}]"), hooks: hooks_file.hooks.clone(), + source: HookSource::Plugin, }); } } @@ -982,6 +995,7 @@ fn append_plugin_hook_file( source_path: path.clone(), source_relative_path, hooks: parsed.hooks, + source: HookSource::Plugin, }); } diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 7ac0fd070f..7449c8e894 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -1,5 +1,6 @@ use super::PluginLoadOutcome; use crate::OPENAI_CURATED_MARKETPLACE_NAME; +use crate::app_bundled_internal::is_app_bundled_plugin; use crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack; use crate::loader::PluginHookLoadOutcome; use crate::loader::configured_curated_plugin_ids_from_codex_home; @@ -1309,16 +1310,20 @@ impl PluginsManager { ), ) .await; - let plugin_data_root = self.store.plugin_data_root(&plugin_id); - let (hook_sources, _hook_load_warnings) = - load_plugin_hooks(&source_path, &plugin_id, &plugin_data_root, &manifest.paths); - let hooks = plugin_hook_declarations(&hook_sources) - .into_iter() - .map(|hook| PluginHookSummary { - key: hook.key, - event_name: hook.event_name, - }) - .collect(); + let hooks = if is_app_bundled_plugin(&plugin_id) { + Vec::new() + } else { + let plugin_data_root = self.store.plugin_data_root(&plugin_id); + let (sources, _warnings) = + load_plugin_hooks(&source_path, &plugin_id, &plugin_data_root, &manifest.paths); + plugin_hook_declarations(&sources) + .into_iter() + .map(|hook| PluginHookSummary { + key: hook.key, + event_name: hook.event_name, + }) + .collect() + }; let app_declarations = load_plugin_apps(source_path.as_path()).await; let apps = app_connector_ids_from_declarations(&app_declarations); let mut seen_app_connector_ids = HashSet::new(); diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 325fd17c9f..2ff8abb332 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_analytics::hook_event_name; +use codex_analytics::hook_source_name; use codex_hooks::PermissionRequestDecision; use codex_hooks::PermissionRequestOutcome; use codex_hooks::PermissionRequestRequest; @@ -27,7 +29,6 @@ use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::HookCompletedEvent; -use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookRunSummary; use codex_protocol::protocol::HookSource; @@ -613,12 +614,19 @@ fn additional_context_messages(additional_contexts: Vec) -> Vec, turn_context: &Arc, preview_runs: Vec, ) { for run in preview_runs { + if run.source == HookSource::AppBundledInternal { + continue; + } sess.send_event( turn_context, EventMsg::HookStarted(HookStartedEvent { @@ -630,6 +638,11 @@ async fn emit_hook_started_events( } } +/// Records metrics and analytics for every completed hook, while emitting lifecycle-completion +/// events only for user-visible hooks. +/// +/// App-bundled internal hook notifications are intentionally suppressed because they are a +/// Desktop implementation detail. pub(crate) async fn emit_hook_completed_events( sess: &Arc, turn_context: &Arc, @@ -638,6 +651,9 @@ 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.source == HookSource::AppBundledInternal { + continue; + } sess.send_event(turn_context, EventMsg::HookCompleted(completed)) .await; } @@ -694,31 +710,6 @@ fn hook_run_analytics_payload( } fn hook_run_metric_tags(run: &HookRunSummary) -> [(&'static str, &'static str); 3] { - let hook_name = match run.event_name { - HookEventName::PreToolUse => "PreToolUse", - HookEventName::PermissionRequest => "PermissionRequest", - HookEventName::PostToolUse => "PostToolUse", - HookEventName::PreCompact => "PreCompact", - HookEventName::PostCompact => "PostCompact", - HookEventName::SessionStart => "SessionStart", - HookEventName::UserPromptSubmit => "UserPromptSubmit", - HookEventName::SubagentStart => "SubagentStart", - HookEventName::SubagentStop => "SubagentStop", - HookEventName::Stop => "Stop", - }; - let hook_source = match run.source { - HookSource::System => "system", - HookSource::User => "user", - HookSource::Project => "project", - HookSource::Mdm => "mdm", - HookSource::SessionFlags => "session_flags", - HookSource::Plugin => "plugin", - HookSource::CloudRequirements => "cloud_requirements", - HookSource::CloudManagedConfig => "cloud_managed_config", - HookSource::LegacyManagedConfigFile => "legacy_managed_config_file", - HookSource::LegacyManagedConfigMdm => "legacy_managed_config_mdm", - HookSource::Unknown => "unknown", - }; let status = match run.status { HookRunStatus::Running => "running", HookRunStatus::Completed => "completed", @@ -728,8 +719,8 @@ fn hook_run_metric_tags(run: &HookRunSummary) -> [(&'static str, &'static str); }; [ - ("hook_name", hook_name), - ("source", hook_source), + ("hook_name", hook_event_name(run.event_name)), + ("source", hook_source_name(run.source)), ("status", status), ] } @@ -785,9 +776,13 @@ mod tests { use pretty_assertions::assert_eq; use super::additional_context_messages; + use super::emit_hook_completed_events; + use super::emit_hook_started_events; use super::hook_run_analytics_payload; use super::hook_run_metric_tags; use crate::session::tests::make_session_and_context; + use crate::session::tests::make_session_and_context_with_rx; + use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::HookCompletedEvent; use codex_protocol::protocol::HookRunSummary; use codex_utils_absolute_path::test_support::PathBufExt; @@ -862,6 +857,38 @@ mod tests { assert_eq!(hook.status, HookRunStatus::Failed); } + #[tokio::test] + async fn user_visible_hook_notifications_are_emitted() { + let (session, turn_context, rx_event) = make_session_and_context_with_rx().await; + while rx_event.try_recv().is_ok() {} + + let regular = sample_hook_run(HookRunStatus::Completed, HookSource::Plugin); + emit_hook_started_events(&session, &turn_context, vec![regular.clone()]).await; + emit_hook_completed_events( + &session, + &turn_context, + vec![HookCompletedEvent { + turn_id: Some(turn_context.sub_id.clone()), + run: regular, + }], + ) + .await; + assert!(matches!( + rx_event.try_recv().expect("regular hook started event"), + codex_protocol::protocol::Event { + msg: EventMsg::HookStarted(_), + .. + } + )); + assert!(matches!( + rx_event.try_recv().expect("regular hook completed event"), + codex_protocol::protocol::Event { + msg: EventMsg::HookCompleted(_), + .. + } + )); + } + #[test] fn hook_run_metric_tags_match_analytics_shape() { let run = sample_hook_run(HookRunStatus::Blocked, HookSource::Project); @@ -900,6 +927,16 @@ mod tests { ("status", "completed"), ] ); + + let internal = sample_hook_run(HookRunStatus::Completed, HookSource::AppBundledInternal); + assert_eq!( + hook_run_metric_tags(&internal), + [ + ("hook_name", "Stop"), + ("source", "app_bundled_internal"), + ("status", "completed"), + ] + ); } fn sample_hook_run(status: HookRunStatus, source: HookSource) -> HookRunSummary { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 31e54c5e7b..98f01d6e1a 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -1,3 +1,4 @@ +use std::ffi::OsString; use std::fs; use std::path::Path; @@ -17,6 +18,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::HookSource; use codex_protocol::protocol::Op; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; @@ -46,6 +48,7 @@ use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use serde_json::Value; +use serial_test::serial; use std::sync::Arc; use std::time::Duration; use tempfile::TempDir; @@ -58,6 +61,33 @@ const SECOND_CONTINUATION_PROMPT: &str = "Now tighten it to just: meow."; const BLOCKED_PROMPT_CONTEXT: &str = "Remember the blocked lighthouse note."; const PERMISSION_REQUEST_HOOK_MATCHER: &str = "^Bash$"; const PERMISSION_REQUEST_ALLOW_REASON: &str = "should not be used for allow"; +const DESKTOP_RESOURCES_PATH_ENV_VAR: &str = "CODEX_DESKTOP_RESOURCES_PATH"; + +struct EnvVarGuard { + key: &'static str, + original: Option, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &std::ffi::OsStr) -> Self { + let original = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, original } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { + match &self.original { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } +} fn restrictive_workspace_write_profile() -> PermissionProfile { PermissionProfile::workspace_write_with( @@ -2814,6 +2844,206 @@ text(output.output); Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +#[serial(desktop_resources)] +async fn app_bundled_internal_pre_tool_use_runs_with_hooks_disabled_without_lifecycle_events() +-> Result<()> { + skip_if_no_network!(Ok(())); + + let resources = TempDir::new()?; + let resources_root = resources.path(); + let marketplace_root = resources_root.join("plugins/openai-bundled"); + let plugin_root = marketplace_root.join("plugins/computer-use"); + let hooks_dir = plugin_root.join("hooks"); + let hook_log_path = resources_root.join("internal_hook_log.jsonl"); + fs::create_dir_all(marketplace_root.join(".agents/plugins")) + .context("create bundled marketplace metadata directory")?; + fs::create_dir_all(plugin_root.join(".codex-plugin")) + .context("create bundled plugin manifest directory")?; + fs::create_dir_all(&hooks_dir).context("create bundled plugin hooks directory")?; + fs::write( + marketplace_root.join(".agents/plugins/marketplace.json"), + serde_json::json!({ + "name": "openai-bundled", + "plugins": [{ + "name": "computer-use", + "source": {"source": "local", "path": "./plugins/computer-use"} + }] + }) + .to_string(), + ) + .context("write bundled marketplace metadata")?; + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"computer-use"}"#, + ) + .context("write bundled plugin manifest")?; + fs::write( + hooks_dir.join("pre_tool_use_hook.py"), + format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{hook_log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": "blocked by app-bundled internal hook" + }} +}})) +"#, + hook_log_path = hook_log_path.display(), + ), + ) + .context("write bundled pre tool use hook script")?; + fs::write( + hooks_dir.join("hooks.json"), + r#"{ + "hooks": { + "PreToolUse": [{ + "matcher": "^Bash$", + "hooks": [{ + "type": "command", + "command": "python3 ${PLUGIN_ROOT}/hooks/pre_tool_use_hook.py" + }] + }] + } +}"#, + ) + .context("write bundled hooks config")?; + + let _desktop_resources = + EnvVarGuard::set(DESKTOP_RESOURCES_PATH_ENV_VAR, resources_root.as_os_str()); + let server = start_mock_server().await; + let call_id = "app-bundled-internal-pretooluse-shell-command"; + let marker = resources_root.join("blocked-command-marker"); + let command = format!("printf should-not-run > {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "internal hook blocked it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let home = Arc::new(TempDir::new()?); + let cached_plugin_root = home + .path() + .join("plugins/cache/openai-bundled/computer-use/local"); + fs::create_dir_all(cached_plugin_root.join(".codex-plugin")) + .context("create cached plugin manifest directory")?; + fs::write( + cached_plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"computer-use"}"#, + ) + .context("write cached plugin manifest")?; + fs::write( + home.path().join("config.toml"), + r#"[plugins."computer-use@openai-bundled"] +enabled = true +"#, + ) + .context("write app-bundled plugin config")?; + + let mut builder = test_codex() + .with_home(Arc::clone(&home)) + .with_config(|config| { + config + .features + .enable(Feature::Plugins) + .expect("test config should allow feature update"); + config + .features + .disable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run the shell command blocked by an internal hook".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::Never), + sandbox_policy: Some(codex_protocol::protocol::SandboxPolicy::DangerFullAccess), + ..Default::default() + }, + }) + .await?; + + let mut turn_id = None; + let mut saw_hook_started = false; + let mut saw_hook_completed = false; + timeout(Duration::from_secs(10), async { + loop { + let event = test + .codex + .next_event() + .await + .expect("event stream should remain open"); + match event.msg { + EventMsg::HookStarted(_) => saw_hook_started = true, + EventMsg::HookCompleted(_) => saw_hook_completed = true, + EventMsg::TurnStarted(event) => turn_id = Some(event.turn_id), + EventMsg::TurnComplete(event) + if turn_id.as_deref() == Some(event.turn_id.as_str()) => + { + break; + } + _ => {} + } + } + }) + .await + .context("wait for app-bundled internal hook turn")?; + + assert!(!saw_hook_started, "internal hook start should stay hidden"); + assert!( + !saw_hook_completed, + "internal hook completion should stay hidden" + ); + assert!(!marker.exists(), "internal hook should block execution"); + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("shell command output string"); + assert!( + output.contains("Command blocked by PreToolUse hook: blocked by app-bundled internal hook"), + "blocked tool output should surface the internal hook reason", + ); + let hook_inputs = read_hook_inputs_from_log(&hook_log_path)?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["hook_event_name"], "PreToolUse"); + assert_eq!(hook_inputs[0]["tool_name"], "Bash"); + assert_eq!(hook_inputs[0]["tool_use_id"], call_id); + assert_eq!(hook_inputs[0]["tool_input"]["command"], command); + + Ok(()) +} + #[tokio::test] async fn plugin_pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); @@ -2916,6 +3146,7 @@ print(json.dumps({{ hooks: serde_json::from_str::(plugin_hooks_json) .context("parse plugin hooks")? .hooks, + source: HookSource::Plugin, }]; let mut builder = test_codex() diff --git a/codex-rs/hooks/src/declarations.rs b/codex-rs/hooks/src/declarations.rs index 468fbc947a..6f85a0efec 100644 --- a/codex-rs/hooks/src/declarations.rs +++ b/codex-rs/hooks/src/declarations.rs @@ -52,7 +52,7 @@ mod tests { fn lists_declared_plugin_handlers_with_persisted_hook_keys() { let plugin_root = test_path_buf("/tmp/plugin").abs(); let source_path = plugin_root.join("hooks/hooks.json"); - let declarations = plugin_hook_declarations(&[PluginHookSource { + let source = PluginHookSource { plugin_id: PluginId::parse("demo@test").expect("plugin id"), plugin_root: plugin_root.clone(), plugin_data_root: plugin_root.join("data"), @@ -78,7 +78,9 @@ mod tests { }], ..Default::default() }, - }]); + source: codex_protocol::protocol::HookSource::Plugin, + }; + let declarations = plugin_hook_declarations(std::slice::from_ref(&source)); assert_eq!( declarations, diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index cab28b64a1..408b29a46c 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -56,7 +56,9 @@ struct HookDiscoveryPolicy { impl HookDiscoveryPolicy { fn allows(self, source: &HookHandlerSource<'_>) -> bool { - !self.allow_managed_hooks_only || source.is_managed + !self.allow_managed_hooks_only + || source.is_managed + || source.source == HookSource::AppBundledInternal } } @@ -223,7 +225,14 @@ fn append_plugin_hook_sources( source_path, source_relative_path, hooks, + source, } = source; + if !matches!(source, HookSource::Plugin | HookSource::AppBundledInternal) { + warnings.push(format!( + "skipping plugin hook source with invalid source {source:?}" + )); + continue; + } let mut env = HashMap::new(); let plugin_root_value = plugin_root.display().to_string(); let plugin_data_root_value = plugin_data_root.display().to_string(); @@ -245,7 +254,7 @@ fn append_plugin_hook_sources( plugin_id.as_str(), source_relative_path.as_str(), ), - source: HookSource::Plugin, + source, is_managed: false, bypass_hook_trust: policy.bypass_hook_trust, hook_states, @@ -502,30 +511,37 @@ fn append_matcher_groups( // TODO(abhinav): replace this positional suffix with a durable hook id. 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 is_forced = source.source == HookSource::AppBundledInternal; + let state = (!is_forced).then(|| source.hook_states.get(&key)).flatten(); + let enabled = is_forced || hook_enabled(source.is_managed, state); let trusted_hash = hook_trusted_hash(source.is_managed, state); - let trust_status = - hook_trust_status(source.is_managed, ¤t_hash, trusted_hash); - hook_entries.push(HookListEntry { - key, - event_name, - handler_type: HookHandlerType::Command, - matcher: matcher.map(ToOwned::to_owned), - command: Some(command.clone()), - timeout_sec, - status_message: status_message.clone(), - source_path: source.path.clone(), - source: source.source, - plugin_id: source.plugin_id.clone(), - display_order: *display_order, - enabled, - is_managed: source.is_managed, - current_hash, - trust_status, - }); + let trust_status = if is_forced { + HookTrustStatus::Trusted + } else { + hook_trust_status(source.is_managed, ¤t_hash, trusted_hash) + }; + if !is_forced { + hook_entries.push(HookListEntry { + key, + event_name, + handler_type: HookHandlerType::Command, + matcher: matcher.map(ToOwned::to_owned), + command: Some(command.clone()), + timeout_sec, + status_message: status_message.clone(), + source_path: source.path.clone(), + source: source.source, + plugin_id: source.plugin_id.clone(), + display_order: *display_order, + enabled, + is_managed: source.is_managed, + current_hash, + trust_status, + }); + } if enabled - && (source.bypass_hook_trust + && (is_forced + || source.bypass_hook_trust || matches!( trust_status, HookTrustStatus::Managed | HookTrustStatus::Trusted diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 859fc54069..f12ac82492 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -109,11 +109,15 @@ impl ClaudeHooksEngine { enabled: bool, bypass_hook_trust: bool, config_layer_stack: Option<&ConfigLayerStack>, - plugin_hook_sources: Vec, - plugin_hook_load_warnings: Vec, + mut plugin_hook_sources: Vec, + mut plugin_hook_load_warnings: Vec, shell: CommandShell, ) -> Self { if !enabled { + plugin_hook_sources.retain(|source| source.source == HookSource::AppBundledInternal); + plugin_hook_load_warnings.clear(); + } + if !enabled && plugin_hook_sources.is_empty() { return Self { handlers: Vec::new(), warnings: Vec::new(), @@ -124,7 +128,7 @@ impl ClaudeHooksEngine { let _ = schema_loader::generated_hook_schemas(); let discovered = discovery::discover_handlers( - config_layer_stack, + config_layer_stack.filter(|_| enabled), plugin_hook_sources, plugin_hook_load_warnings, bypass_hook_trust, diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 544c67e7ce..ad8f9bc444 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -20,11 +20,13 @@ use codex_config::TomlValue; use codex_plugin::PluginHookSource; use codex_plugin::PluginId; use codex_protocol::ThreadId; +use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookSource; use codex_protocol::protocol::HookTrustStatus; +use codex_utils_absolute_path::test_support::PathBufExt; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -900,6 +902,7 @@ fn allow_managed_hooks_only_skips_unmanaged_plugin_hooks() { source_path, source_relative_path: "hooks/hooks.json".to_string(), hooks: pre_tool_use_hook_events("python3 /tmp/plugin-hook.py"), + source: HookSource::Plugin, }]; let (requirements, requirements_toml) = requirements_with_managed_hooks_only( /*allow_managed_hooks_only*/ true, /*managed_hooks*/ None, @@ -923,6 +926,126 @@ fn allow_managed_hooks_only_skips_unmanaged_plugin_hooks() { assert!(engine.warnings().is_empty()); } +fn build_engine( + enabled: bool, + config_layer_stack: Option<&ConfigLayerStack>, + plugin_hook_sources: Vec, +) -> ClaudeHooksEngine { + ClaudeHooksEngine::new( + enabled, + /*bypass_hook_trust*/ false, + config_layer_stack, + plugin_hook_sources, + Vec::new(), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ) +} + +fn plugin_hook_source(root: &Path, source: HookSource) -> PluginHookSource { + let plugin_root = AbsolutePathBuf::try_from(root.join("demo-plugin")).expect("plugin root"); + let plugin_data_root = + AbsolutePathBuf::try_from(root.join("plugin-data")).expect("plugin data root"); + let source_path = plugin_root.join("hooks/hooks.json"); + PluginHookSource { + plugin_id: PluginId::parse("demo-plugin@test-marketplace").expect("plugin id"), + plugin_root, + plugin_data_root, + source_path, + source_relative_path: "hooks/hooks.json".to_string(), + hooks: pre_tool_use_hook_events(r#""${PLUGIN_ROOT}/internal-hook""#), + source, + } +} + +#[test] +fn plugin_hook_source_rejects_non_plugin_provenance() { + let temp = tempdir().expect("create temp dir"); + let invalid_engine = build_engine( + /*enabled*/ true, + /*config_layer_stack*/ None, + vec![plugin_hook_source(temp.path(), HookSource::User)], + ); + assert!(invalid_engine.handlers.is_empty()); + assert_eq!( + invalid_engine.warnings(), + &["skipping plugin hook source with invalid source User".to_string()] + ); +} + +#[test] +fn app_bundled_internal_hooks_ignore_user_controls_and_are_hidden() { + let temp = tempdir().expect("create temp dir"); + let plugin_hook_sources = vec![plugin_hook_source( + temp.path(), + HookSource::AppBundledInternal, + )]; + let key_source = crate::declarations::plugin_hook_key_source( + "demo-plugin@test-marketplace", + "hooks/hooks.json", + ); + let hook_key = crate::hook_key( + &key_source, + HookEventName::PreToolUse, + /*group_index*/ 0, + /*handler_index*/ 0, + ); + let state = [( + hook_key, + serde_json::json!({ "enabled": false, "trusted_hash": "stale" }), + )] + .into_iter() + .collect::>(); + let config = serde_json::from_value(serde_json::json!({ + "hooks": { "state": state }, + })) + .expect("config TOML should deserialize"); + let disabled_stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: temp.path().join("config.toml").abs(), + profile: None, + }, + config, + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config stack"); + let disabled_state_engine = build_engine( + /*enabled*/ true, + Some(&disabled_stack), + plugin_hook_sources.clone(), + ); + assert_eq!(disabled_state_engine.handlers.len(), 1); + + let (requirements, requirements_toml) = requirements_with_managed_hooks_only( + /*allow_managed_hooks_only*/ true, /*managed_hooks*/ None, + ); + let managed_only_stack = + ConfigLayerStack::new(Vec::new(), requirements, requirements_toml).expect("config stack"); + let managed_only_engine = build_engine( + /*enabled*/ true, + Some(&managed_only_stack), + plugin_hook_sources.clone(), + ); + assert_eq!(managed_only_engine.handlers.len(), 1); + + let listed = crate::list_hooks(crate::HooksConfig { + legacy_notify_argv: None, + feature_enabled: false, + bypass_hook_trust: false, + config_layer_stack: None, + plugin_hook_sources, + plugin_hook_load_warnings: Vec::new(), + shell_program: None, + shell_args: Vec::new(), + }); + assert!(listed.hooks.is_empty()); +} + #[test] fn allow_managed_hooks_only_keeps_managed_requirement_and_config_layer_hooks() { let temp = tempdir().expect("create temp dir"); @@ -1324,6 +1447,7 @@ print(json.dumps({ }], ..Default::default() }, + source: HookSource::Plugin, }]; let config_layer_stack = trusted_plugin_hook_stack( AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"), @@ -1443,6 +1567,7 @@ fn plugin_hook_sources_expand_plugin_placeholders() { }], ..Default::default() }, + source: HookSource::Plugin, }]; let config_layer_stack = trusted_plugin_hook_stack( AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"), diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs index 7daa78bc06..2618d4f4ca 100644 --- a/codex-rs/plugin/src/lib.rs +++ b/codex-rs/plugin/src/lib.rs @@ -11,6 +11,7 @@ mod plugin_id; mod provider; use codex_config::HookEventsToml; +use codex_protocol::protocol::HookSource; use codex_utils_absolute_path::AbsolutePathBuf; pub use load_outcome::EffectiveSkillRoots; pub use load_outcome::LoadedPlugin; @@ -66,6 +67,7 @@ pub struct PluginHookSource { pub source_path: AbsolutePathBuf, pub source_relative_path: String, pub hooks: HookEventsToml, + pub source: HookSource, } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index b41a02b889..828c973500 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1429,6 +1429,7 @@ pub enum HookSource { Mdm, SessionFlags, Plugin, + AppBundledInternal, CloudRequirements, CloudManagedConfig, LegacyManagedConfigFile,