diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 0aa7a115fb..df2c6492b6 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -483,7 +483,6 @@ async fn load_plugin( mcp_servers: HashMap::new(), apps: Vec::new(), hook_sources: Vec::new(), - hook_load_warnings: Vec::new(), error: None, }; @@ -552,9 +551,7 @@ async fn load_plugin( } loaded_plugin.mcp_servers = mcp_servers; loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await; - let hook_discovery = load_plugin_hooks(&plugin_root, &loaded_plugin_id, manifest_paths); - loaded_plugin.hook_sources = hook_discovery.sources; - loaded_plugin.hook_load_warnings = hook_discovery.warnings; + loaded_plugin.hook_sources = load_plugin_hooks(&plugin_root, &loaded_plugin_id, manifest_paths); loaded_plugin } @@ -684,22 +681,19 @@ fn default_app_config_paths(plugin_root: &Path) -> Vec { paths } -#[derive(Debug, Default)] -pub struct PluginHookDiscovery { - pub sources: Vec, - pub warnings: Vec, -} - +// Discover plugin-bundled hooks from manifest `hooks` entries when present +// (path, paths, inline object, or inline objects), otherwise from the default +// `hooks/hooks.json` file. pub fn load_plugin_hooks( plugin_root: &AbsolutePathBuf, plugin_id: &PluginId, manifest_paths: &PluginManifestPaths, -) -> PluginHookDiscovery { - let mut discovery = PluginHookDiscovery::default(); +) -> Vec { + let mut sources = Vec::new(); match &manifest_paths.hooks { Some(PluginManifestHooks::Paths(paths)) => { for path in paths { - append_plugin_hook_file(plugin_root, plugin_id, path, &mut discovery); + append_plugin_hook_file(plugin_root, plugin_id, path, &mut sources); } } Some(PluginManifestHooks::Inline(hooks_files)) => { @@ -710,7 +704,7 @@ pub fn load_plugin_hooks( if hooks_file.hooks.is_empty() { continue; } - discovery.sources.push(PluginHookSource { + sources.push(PluginHookSource { plugin_id: plugin_id.clone(), plugin_root: plugin_root.clone(), source_path: manifest_path.clone(), @@ -722,36 +716,38 @@ pub fn load_plugin_hooks( None => { let default_path = plugin_root.join(DEFAULT_HOOKS_CONFIG_FILE); if default_path.as_path().is_file() { - append_plugin_hook_file(plugin_root, plugin_id, &default_path, &mut discovery); + append_plugin_hook_file(plugin_root, plugin_id, &default_path, &mut sources); } } } - discovery + sources } +// Load one resolved plugin hook file and keep source metadata with its parsed +// hook events so runtime discovery can report plugin-originated hook runs. fn append_plugin_hook_file( plugin_root: &AbsolutePathBuf, plugin_id: &PluginId, path: &AbsolutePathBuf, - discovery: &mut PluginHookDiscovery, + sources: &mut Vec, ) { let contents = match fs::read_to_string(path.as_path()) { Ok(contents) => contents, Err(err) => { - discovery.warnings.push(format!( - "failed to read plugin hooks config {}: {err}", - path.display() - )); + warn!( + path = %path.display(), + "failed to read plugin hooks config: {err}" + ); return; } }; let parsed = match serde_json::from_str::(&contents) { Ok(parsed) => parsed, Err(err) => { - discovery.warnings.push(format!( - "failed to parse plugin hooks config {}: {err}", - path.display() - )); + warn!( + path = %path.display(), + "failed to parse plugin hooks config: {err}" + ); return; } }; @@ -759,7 +755,7 @@ fn append_plugin_hook_file( return; } - discovery.sources.push(PluginHookSource { + sources.push(PluginHookSource { plugin_id: plugin_id.clone(), plugin_root: plugin_root.clone(), source_path: path.clone(), @@ -1241,19 +1237,15 @@ mod tests { let manifest = load_plugin_manifest(plugin_root.as_path()).expect("manifest"); let plugin_id = PluginId::parse("demo-plugin@test-marketplace").expect("plugin id"); - let discovery = load_plugin_hooks(&plugin_root, &plugin_id, &manifest.paths); + let sources = load_plugin_hooks(&plugin_root, &plugin_id, &manifest.paths); - assert_eq!(discovery.warnings, Vec::::new()); - assert_eq!(discovery.sources.len(), 1); + assert_eq!(sources.len(), 1); assert_eq!( - discovery.sources[0].plugin_id, + sources[0].plugin_id, PluginId::parse("demo-plugin@test-marketplace").expect("plugin id") ); - assert_eq!( - discovery.sources[0].source_relative_path, - "hooks/hooks.json" - ); - assert_eq!(discovery.sources[0].hooks.handler_count(), 1); + assert_eq!(sources[0].source_relative_path, "hooks/hooks.json"); + assert_eq!(sources[0].hooks.handler_count(), 1); } #[test] @@ -1313,20 +1305,17 @@ mod tests { let manifest = load_plugin_manifest(plugin_root.as_path()).expect("manifest"); let plugin_id = PluginId::parse("demo-plugin@test-marketplace").expect("plugin id"); - let discovery = load_plugin_hooks(&plugin_root, &plugin_id, &manifest.paths); + let sources = load_plugin_hooks(&plugin_root, &plugin_id, &manifest.paths); - assert_eq!(discovery.warnings, Vec::::new()); assert_eq!( - discovery - .sources + sources .iter() .map(|source| source.source_relative_path.as_str()) .collect::>(), vec!["hooks/one.json", "hooks/two.json"] ); assert_eq!( - discovery - .sources + sources .iter() .map(|source| source.hooks.handler_count()) .collect::>(), @@ -1360,15 +1349,11 @@ mod tests { let manifest = load_plugin_manifest(plugin_root.as_path()).expect("manifest"); let plugin_id = PluginId::parse("demo-plugin@test-marketplace").expect("plugin id"); - let discovery = load_plugin_hooks(&plugin_root, &plugin_id, &manifest.paths); + let sources = load_plugin_hooks(&plugin_root, &plugin_id, &manifest.paths); - assert_eq!(discovery.warnings, Vec::::new()); - assert_eq!(discovery.sources.len(), 1); - assert_eq!( - discovery.sources[0].source_relative_path, - "plugin.json#hooks[0]" - ); - assert_eq!(discovery.sources[0].hooks.handler_count(), 1); + assert_eq!(sources.len(), 1); + assert_eq!(sources[0].source_relative_path, "plugin.json#hooks[0]"); + assert_eq!(sources[0].hooks.handler_count(), 1); } #[test] diff --git a/codex-rs/core/src/plugins/manager_tests.rs b/codex-rs/core/src/plugins/manager_tests.rs index 60b7dd6f4d..4e794506c0 100644 --- a/codex-rs/core/src/plugins/manager_tests.rs +++ b/codex-rs/core/src/plugins/manager_tests.rs @@ -220,7 +220,6 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { )]), apps: vec![AppConnectorId("connector_example".to_string())], hook_sources: Vec::new(), - hook_load_warnings: Vec::new(), error: None, }] ); @@ -722,7 +721,6 @@ async fn load_plugins_preserves_disabled_plugins_without_effective_contributions mcp_servers: HashMap::new(), apps: Vec::new(), hook_sources: Vec::new(), - hook_load_warnings: Vec::new(), error: None, }] ); @@ -841,7 +839,6 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() { mcp_servers: HashMap::new(), apps: Vec::new(), hook_sources: Vec::new(), - hook_load_warnings: Vec::new(), error: None, }; let summary = |config_name: &str, display_name: &str| PluginCapabilitySummary { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 409398f788..e49e25ac6e 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -691,21 +691,17 @@ impl Session { let hook_shell_program = hook_shell_argv.remove(0); let _ = hook_shell_argv.pop(); let plugin_hooks_enabled = config.features.enabled(Feature::PluginHooks); - let (plugin_hook_sources, plugin_hook_load_warnings) = if plugin_hooks_enabled { + let plugin_hook_sources = if plugin_hooks_enabled { let plugin_outcome = plugins_manager.plugins_for_config(&config).await; - ( - plugin_outcome.effective_plugin_hook_sources(), - plugin_outcome.effective_plugin_hook_warnings(), - ) + plugin_outcome.effective_plugin_hook_sources() } else { - (Vec::new(), Vec::new()) + Vec::new() }; let hooks = Hooks::new(HooksConfig { legacy_notify_argv: config.notify.clone(), feature_enabled: config.features.enabled(Feature::CodexHooks), config_layer_stack: Some(config.config_layer_stack.clone()), plugin_hook_sources, - plugin_hook_load_warnings, shell_program: Some(hook_shell_program), shell_args: hook_shell_argv, }); diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index ebeda10ab9..ce42908c09 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -38,11 +38,10 @@ struct HookHandlerSource<'a> { pub(crate) fn discover_handlers( config_layer_stack: Option<&ConfigLayerStack>, plugin_hook_sources: Vec, - plugin_hook_load_warnings: Vec, ) -> DiscoveryResult { let Some(config_layer_stack) = config_layer_stack else { let mut handlers = Vec::new(); - let mut warnings = plugin_hook_load_warnings; + let mut warnings = Vec::new(); let mut display_order = 0_i64; append_plugin_hook_sources( &mut handlers, @@ -54,7 +53,7 @@ pub(crate) fn discover_handlers( }; let mut handlers = Vec::new(); - let mut warnings = plugin_hook_load_warnings; + let mut warnings = Vec::new(); let mut display_order = 0_i64; append_managed_requirement_handlers( @@ -169,7 +168,8 @@ fn append_plugin_hook_sources( } = source; let mut env = HashMap::new(); let plugin_root_value = plugin_root.display().to_string(); - env.insert("AGENTS_PLUGIN_ROOT".to_string(), plugin_root_value.clone()); + env.insert("PLUGIN_ROOT".to_string(), plugin_root_value.clone()); + // For OOTB compat with existing plugins that use this env var. env.insert("CLAUDE_PLUGIN_ROOT".to_string(), plugin_root_value); append_hook_events( handlers, diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 89daf501ca..5c121136f7 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -79,7 +79,6 @@ impl ClaudeHooksEngine { enabled: bool, config_layer_stack: Option<&ConfigLayerStack>, plugin_hook_sources: Vec, - plugin_hook_load_warnings: Vec, shell: CommandShell, ) -> Self { if !enabled { @@ -91,11 +90,7 @@ impl ClaudeHooksEngine { } let _ = schema_loader::generated_hook_schemas(); - let discovered = discovery::discover_handlers( - config_layer_stack, - plugin_hook_sources, - plugin_hook_load_warnings, - ); + let discovered = discovery::discover_handlers(config_layer_stack, plugin_hook_sources); Self { handlers: discovered.handlers, warnings: discovered.warnings, diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index c64bb1fbf4..4b382df4a6 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -109,7 +109,6 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: /*enabled*/ true, Some(&config_layer_stack), Vec::new(), - Vec::new(), CommandShell { program: String::new(), args: Vec::new(), @@ -194,7 +193,6 @@ fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { /*enabled*/ true, Some(&config_layer_stack), Vec::new(), - Vec::new(), CommandShell { program: String::new(), args: Vec::new(), @@ -303,7 +301,6 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { /*enabled*/ true, Some(&config_layer_stack), Vec::new(), - Vec::new(), CommandShell { program: String::new(), args: Vec::new(), @@ -352,7 +349,7 @@ import os from pathlib import Path Path(r"{log_path}").write_text(json.dumps({{ - "agents": os.environ.get("AGENTS_PLUGIN_ROOT"), + "plugin": os.environ.get("PLUGIN_ROOT"), "claude": os.environ.get("CLAUDE_PLUGIN_ROOT"), }}), encoding="utf-8") "#, @@ -383,7 +380,6 @@ Path(r"{log_path}").write_text(json.dumps({{ /*enabled*/ true, None, plugin_hook_sources, - Vec::new(), CommandShell { program: String::new(), args: Vec::new(), @@ -429,7 +425,7 @@ Path(r"{log_path}").write_text(json.dumps({{ assert_eq!( logged, serde_json::json!({ - "agents": plugin_root.display().to_string(), + "plugin": plugin_root.display().to_string(), "claude": plugin_root.display().to_string(), }) ); diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index 7dd93213a1..4509a8a631 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -27,7 +27,6 @@ pub struct HooksConfig { pub feature_enabled: bool, pub config_layer_stack: Option, pub plugin_hook_sources: Vec, - pub plugin_hook_load_warnings: Vec, pub shell_program: Option, pub shell_args: Vec, } @@ -57,7 +56,6 @@ impl Hooks { config.feature_enabled, config.config_layer_stack.as_ref(), config.plugin_hook_sources, - config.plugin_hook_load_warnings, CommandShell { program: config.shell_program.unwrap_or_default(), args: config.shell_args, diff --git a/codex-rs/plugin/src/load_outcome.rs b/codex-rs/plugin/src/load_outcome.rs index 0865b9020f..40dba4ae2a 100644 --- a/codex-rs/plugin/src/load_outcome.rs +++ b/codex-rs/plugin/src/load_outcome.rs @@ -23,7 +23,6 @@ pub struct LoadedPlugin { pub mcp_servers: HashMap, pub apps: Vec, pub hook_sources: Vec, - pub hook_load_warnings: Vec, pub error: Option, } @@ -151,14 +150,6 @@ impl PluginLoadOutcome { .collect() } - pub fn effective_plugin_hook_warnings(&self) -> Vec { - self.plugins - .iter() - .filter(|plugin| plugin.is_active()) - .flat_map(|plugin| plugin.hook_load_warnings.iter().cloned()) - .collect() - } - pub fn capability_summaries(&self) -> &[PluginCapabilitySummary] { &self.capability_summaries }