From ef596c68ca35dcd42283a0014f9b7fb057147269 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 13 Aug 2026 15:18:01 +0000 Subject: [PATCH] Reject sessions with unloadable required managed hooks (#38394) ## Why Hooks supplied as managed requirements must not be silently skipped when their handlers cannot be loaded. ## What changed - Fail session and app-server thread startup when an enabled managed requirement contains an invalid matcher, an empty command, or an unsupported handler type. - Keep load failures for ordinary managed configuration hooks as warnings, and do not enforce managed hook requirements while the hooks feature is disabled. ## Testing Added hook-engine, core session, and app-server coverage for valid and invalid managed hook requirements. GitOrigin-RevId: 91a23b0c1d85e143aba6aabe874c5274e52fc6b2 --- .../app-server/tests/suite/v2/thread_start.rs | 81 +++++++ codex-rs/core/src/session/session.rs | 2 +- codex-rs/core/src/session/tests.rs | 9 +- codex-rs/core/tests/suite/hooks.rs | 32 +++ .../hooks/src/engine/command_runner_tests.rs | 1 + codex-rs/hooks/src/engine/discovery.rs | 106 +++++--- codex-rs/hooks/src/engine/mod.rs | 7 + codex-rs/hooks/src/engine/mod_tests.rs | 228 ++++++++++++++++++ codex-rs/hooks/src/registry.rs | 14 +- 9 files changed, 440 insertions(+), 40 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index 0a2b9dc997..c4a12be684 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -1083,6 +1083,87 @@ async fn thread_start_fails_when_required_mcp_server_fails_to_initialize() -> Re Ok(()) } +#[tokio::test] +async fn thread_start_fails_when_managed_hook_matcher_is_invalid() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + std::fs::write( + codex_home.path().join("requirements.toml"), + r#"[hooks] + +[[hooks.PreToolUse]] +matcher = "[" + +[[hooks.PreToolUse.hooks]] +type = "command" +command = "echo managed" +"#, + )?; + + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let request_id = app_server + .send_thread_start_request_with_auto_env(ThreadStartParams::default()) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert!( + error.error.message.contains("managed") && error.error.message.contains("invalid matcher"), + "unexpected error message: {}", + error.error.message + ); + + Ok(()) +} + +#[tokio::test] +async fn thread_start_fails_when_managed_hook_handler_is_unsupported() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + std::fs::write( + codex_home.path().join("requirements.toml"), + r#"[hooks] + +[[hooks.PreToolUse]] +matcher = "^Bash$" + +[[hooks.PreToolUse.hooks]] +type = "mcp_tool" +server = "security" +tool = "scan" +"#, + )?; + + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let request_id = app_server + .send_thread_start_request_with_auto_env(ThreadStartParams::default()) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert!( + error.error.message.contains("MCP tool hook"), + "unexpected error message: {}", + error.error.message + ); + + Ok(()) +} + #[tokio::test] async fn thread_start_emits_mcp_server_status_updated_notifications() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index d3e2c930f8..c7ac5baed3 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1162,7 +1162,7 @@ impl Session { resolved_environments.single_local_environment(), ) .await; - let (hooks, async_hook_results) = Hooks::new(hooks_config, thread_id); + let (hooks, async_hook_results) = Hooks::new(hooks_config, thread_id)?; for warning in hooks.startup_warnings() { post_session_configured_events.push(Event { id: INITIAL_SUBMIT_ID.to_owned(), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 098434ca72..7dac028ffc 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -782,7 +782,8 @@ async fn preview_session_start_hooks( ..HooksConfig::default() }, thread_id, - ); + ) + .expect("initialize hooks for session-start preview"); Ok( hooks.preview_session_start(&codex_hooks::SessionStartRequest { @@ -5840,7 +5841,8 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { ..HooksConfig::default() }, thread_id, - ); + ) + .expect("initialize test hooks"); let services = SessionServices { mcp_runtime, mcp_handler_cache: Default::default(), @@ -8023,7 +8025,8 @@ where ..HooksConfig::default() }, thread_id, - ); + ) + .expect("initialize test hooks"); let services = SessionServices { mcp_runtime, mcp_handler_cache: Default::default(), diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 1d6e990469..f4714df156 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -3,6 +3,7 @@ use std::path::Path; use anyhow::Context; use anyhow::Result; +use codex_config::test_support::CloudConfigBundleFixture; use codex_core::StartThreadOptions; use codex_core::TurnInputRequest; use codex_core::config::Config; @@ -75,6 +76,37 @@ 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"; +#[tokio::test] +async fn managed_hook_discovery_failure_rejects_session_startup_when_hooks_enabled() -> Result<()> { + let server = start_mock_server().await; + let mut builder = test_codex().with_cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement( + r#" +[hooks] + +[[hooks.PreToolUse]] +matcher = "[" + +[[hooks.PreToolUse.hooks]] +type = "command" +command = "echo managed" +"#, + ), + ); + + let result = builder.build_with_auto_env(&server).await; + let Err(error) = result else { + panic!("session startup should reject an invalid managed hook matcher"); + }; + let error = format!("{error:#}"); + assert!( + error.contains("managed") && error.contains("invalid matcher"), + "unexpected managed hook startup error: {error}" + ); + + Ok(()) +} + fn restrictive_workspace_write_profile() -> PermissionProfile { PermissionProfile::workspace_write_with( &[], diff --git a/codex-rs/hooks/src/engine/command_runner_tests.rs b/codex-rs/hooks/src/engine/command_runner_tests.rs index bc7ab3a8ad..d63d533bc3 100644 --- a/codex-rs/hooks/src/engine/command_runner_tests.rs +++ b/codex-rs/hooks/src/engine/command_runner_tests.rs @@ -154,6 +154,7 @@ async fn schedule(runtime: &CommandHookRuntime, handler: ConfiguredHandler, cwd: let engine = ClaudeHooksEngine { handlers: vec![handler], warnings: Vec::new(), + required_load_errors: Vec::new(), command_runtime: runtime.clone(), }; engine diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index d94078dfd6..158eac1205 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -40,6 +40,7 @@ pub(crate) struct DiscoveryResult { pub handlers: Vec, pub hook_entries: Vec, pub warnings: Vec, + pub required_load_errors: Vec, } struct HookHandlerSource<'a> { @@ -47,12 +48,27 @@ struct HookHandlerSource<'a> { key_source: String, source: HookSource, is_managed: bool, + requirement: HookRequirement<'a>, bypass_hook_trust: bool, hook_states: &'a HashMap, env: HashMap, plugin_id: Option, } +enum HookRequirement<'a> { + Required(&'a mut Vec), + Optional, +} + +impl HookHandlerSource<'_> { + fn record_load_failure(&mut self, warning: String, warnings: &mut Vec) { + if let HookRequirement::Required(required_load_errors) = &mut self.requirement { + required_load_errors.push(warning.clone()); + } + warnings.push(warning); + } +} + struct NormalizedHandler { config: HookHandlerConfig, kind: ConfiguredHandlerKind, @@ -82,6 +98,7 @@ pub(crate) fn discover_handlers( let mut handlers = Vec::new(); let mut hook_entries = Vec::new(); let mut warnings = plugin_hook_load_warnings; + let mut required_load_errors = Vec::new(); let mut display_order = 0_i64; let mut visited_json_hook_folders = HashSet::new(); let hook_states = hook_states_from_stack(config_layer_stack); @@ -97,7 +114,7 @@ pub(crate) fn discover_handlers( }; if let Some(config_layer_stack) = config_layer_stack { - append_managed_requirement_handlers( + required_load_errors = append_managed_requirement_handlers( &mut handlers, &mut hook_entries, &mut warnings, @@ -115,6 +132,7 @@ pub(crate) fn discover_handlers( key_source: policy_path.display().to_string(), source: hook_source, is_managed, + requirement: HookRequirement::Optional, bypass_hook_trust: false, hook_states: &hook_states, env: HashMap::new(), @@ -154,6 +172,7 @@ pub(crate) fn discover_handlers( key_source: source_path.display().to_string(), source: hook_source, is_managed, + requirement: HookRequirement::Optional, bypass_hook_trust: policy.bypass_hook_trust, hook_states: &hook_states, env: HashMap::new(), @@ -180,6 +199,7 @@ pub(crate) fn discover_handlers( handlers, hook_entries, warnings, + required_load_errors, } } @@ -191,10 +211,11 @@ fn append_managed_requirement_handlers( config_layer_stack: &ConfigLayerStack, hook_states: &HashMap, policy: HookDiscoveryPolicy, -) { +) -> Vec { let Some(managed_hooks) = config_layer_stack.requirements().managed_hooks.as_ref() else { - return; + return Vec::new(); }; + let mut required_load_errors = Vec::new(); let source_path = managed_hooks_source_path(managed_hooks.get(), managed_hooks.source.as_ref()); append_hook_events( handlers, @@ -206,6 +227,7 @@ fn append_managed_requirement_handlers( key_source: source_path.display().to_string(), source: hook_source_for_requirement_source(managed_hooks.source.as_ref()), is_managed: true, + requirement: HookRequirement::Required(&mut required_load_errors), bypass_hook_trust: false, hook_states, env: HashMap::new(), @@ -214,6 +236,7 @@ fn append_managed_requirement_handlers( managed_hooks.get().hooks.clone(), policy, ); + required_load_errors } fn append_plugin_hook_sources( @@ -257,6 +280,7 @@ fn append_plugin_hook_sources( ), source: HookSource::Plugin, is_managed: false, + requirement: HookRequirement::Optional, bypass_hook_trust: policy.bypass_hook_trust, hook_states, env, @@ -428,7 +452,7 @@ fn append_hook_events( hook_entries: &mut Vec, warnings: &mut Vec, display_order: &mut i64, - source: HookHandlerSource<'_>, + mut source: HookHandlerSource<'_>, hook_events: HookEventsToml, policy: HookDiscoveryPolicy, ) { @@ -442,7 +466,7 @@ fn append_hook_events( hook_entries, warnings, display_order, - &source, + &mut source, event_name, groups, ); @@ -454,7 +478,7 @@ fn append_matcher_groups( hook_entries: &mut Vec, warnings: &mut Vec, display_order: &mut i64, - source: &HookHandlerSource<'_>, + source: &mut HookHandlerSource<'_>, event_name: codex_protocol::protocol::HookEventName, groups: Vec, ) { @@ -463,10 +487,15 @@ fn append_matcher_groups( if let Some(matcher) = matcher && let Err(err) = validate_matcher_pattern(matcher) { - warnings.push(format!( + let warning = format!( "invalid matcher {matcher:?} in {}: {err}", source.path.display() - )); + ); + if group.hooks.is_empty() { + warnings.push(warning); + } else { + source.record_load_failure(warning, warnings); + } continue; } for (handler_index, handler) in group.hooks.iter().cloned().enumerate() { @@ -485,10 +514,10 @@ fn append_matcher_groups( command }; if command.trim().is_empty() { - warnings.push(format!( - "skipping empty hook command in {}", - source.path.display() - )); + source.record_load_failure( + format!("skipping empty hook command in {}", source.path.display()), + warnings, + ); continue; } let timeout_sec = normalize_command_hook( @@ -549,24 +578,33 @@ fn append_matcher_groups( } } HookHandlerConfig::McpTool { .. } => { - warnings.push(format!( - "skipping MCP tool hook in {}: MCP tool hooks are not supported yet", - source.path.display() - )); + source.record_load_failure( + format!( + "skipping MCP tool hook in {}: MCP tool hooks are not supported yet", + source.path.display() + ), + warnings, + ); continue; } HookHandlerConfig::Prompt {} => { - warnings.push(format!( - "skipping prompt hook in {}: prompt hooks are not supported yet", - source.path.display() - )); + source.record_load_failure( + format!( + "skipping prompt hook in {}: prompt hooks are not supported yet", + source.path.display() + ), + warnings, + ); continue; } HookHandlerConfig::Agent {} => { - warnings.push(format!( - "skipping agent hook in {}: agent hooks are not supported yet", - source.path.display() - )); + source.record_load_failure( + format!( + "skipping agent hook in {}: agent hooks are not supported yet", + source.path.display() + ), + warnings, + ); continue; } }; @@ -797,6 +835,7 @@ mod tests { key_source: path.display().to_string(), source: hook_source(), is_managed: true, + requirement: super::HookRequirement::Optional, bypass_hook_trust: false, hook_states, env: std::collections::HashMap::new(), @@ -814,6 +853,7 @@ mod tests { key_source: path.display().to_string(), source: HookSource::User, is_managed: false, + requirement: super::HookRequirement::Optional, bypass_hook_trust, hook_states, env: std::collections::HashMap::new(), @@ -901,7 +941,7 @@ mod tests { &mut entries, &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), event_name, vec![match additional_context_limit { Some(limit) => command_group_with_additional_context_limit(limit), @@ -985,7 +1025,7 @@ mod tests { &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), HookEventName::UserPromptSubmit, vec![command_group(Some("["))], ); @@ -1024,7 +1064,7 @@ mod tests { &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), HookEventName::PreToolUse, vec![command_group(Some("^Bash$"))], ); @@ -1064,7 +1104,7 @@ mod tests { &mut hook_entries, &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), HookEventName::SessionEnd, vec![MatcherGroup { matcher: Some("other".to_string()), @@ -1146,7 +1186,7 @@ mod tests { &mut hook_entries, &mut warnings, &mut display_order, - &unmanaged_hook_handler_source( + &mut unmanaged_hook_handler_source( &source_path, &hook_states, /*bypass_hook_trust*/ true, @@ -1182,7 +1222,7 @@ mod tests { &mut hook_entries, &mut warnings, &mut display_order, - &unmanaged_hook_handler_source( + &mut unmanaged_hook_handler_source( &source_path, &hook_states, /*bypass_hook_trust*/ true, @@ -1211,7 +1251,7 @@ mod tests { &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), HookEventName::PreToolUse, vec![command_group(Some("*"))], ); @@ -1234,7 +1274,7 @@ mod tests { &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), HookEventName::PostToolUse, vec![command_group(Some("Edit|Write"))], ); @@ -1292,7 +1332,7 @@ mod tests { &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &hook_states), + &mut hook_handler_source(&source_path, &hook_states), HookEventName::PreToolUse, vec![MatcherGroup { matcher: Some("^Bash$".to_string()), diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 997803f599..421e25537c 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -146,6 +146,7 @@ pub struct HookListEntry { pub(crate) struct ClaudeHooksEngine { pub(crate) handlers: Vec, warnings: Vec, + required_load_errors: Vec, pub(crate) command_runtime: CommandHookRuntime, } @@ -162,6 +163,7 @@ impl ClaudeHooksEngine { return Self { handlers: Vec::new(), warnings: Vec::new(), + required_load_errors: Vec::new(), command_runtime, }; } @@ -177,6 +179,7 @@ impl ClaudeHooksEngine { Self { handlers: discovered.handlers, warnings: discovered.warnings, + required_load_errors: discovered.required_load_errors, command_runtime, } } @@ -185,6 +188,10 @@ impl ClaudeHooksEngine { &self.warnings } + pub(crate) fn required_load_errors(&self) -> &[String] { + &self.required_load_errors + } + pub(crate) fn preview_session_start( &self, request: &SessionStartRequest, diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index d8dd2a8dda..8f6194a19f 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -198,6 +198,234 @@ fn requirements_with_managed_hooks_only( ) } +fn required_hooks_stack( + managed_hooks: ManagedHooksRequirementsToml, + source: RequirementSource, +) -> ConfigLayerStack { + ConfigLayerStack::new( + Vec::new(), + ConfigRequirements { + managed_hooks: Some(ConstrainedWithSource::new( + Constrained::allow_any(managed_hooks.clone()), + Some(source), + )), + ..ConfigRequirements::default() + }, + ConfigRequirementsToml { + hooks: Some(managed_hooks), + ..ConfigRequirementsToml::default() + }, + ) + .expect("config layer stack") +} + +#[test] +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")); + 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(), + ) + .expect("disabled hooks feature should not enforce managed requirements hooks"); + + assert!(hooks.startup_warnings().is_empty()); +} + +#[test] +fn required_managed_hooks_reject_invalid_matchers() { + let temp = tempdir().expect("create temp dir"); + let mut events = pre_tool_use_hook_events("echo managed"); + events.pre_tool_use[0].matcher = Some("[".to_string()); + let config_layer_stack = required_hooks_stack( + managed_hooks_for_current_platform(temp.path(), events), + RequirementSource::LegacyManagedConfigTomlFromMdm, + ); + + let error = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .err() + .expect("invalid required managed matcher should reject startup"); + + assert!(error.to_string().contains("invalid matcher")); +} + +#[test] +fn required_managed_hooks_allow_invalid_matchers_without_handlers() { + let temp = tempdir().expect("create temp dir"); + let mut events = pre_tool_use_hook_events("echo managed"); + events.pre_tool_use.push(MatcherGroup { + matcher: Some("[".to_string()), + hooks: Vec::new(), + }); + let config_layer_stack = required_hooks_stack( + managed_hooks_for_current_platform(temp.path(), events), + RequirementSource::LegacyManagedConfigTomlFromMdm, + ); + + let (hooks, _result_receiver) = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .expect("an empty matcher group should not prevent required managed hooks from loading"); + + assert_eq!(hooks.startup_warnings().len(), 1); + assert!(hooks.startup_warnings()[0].contains("invalid matcher")); +} + +#[test] +fn required_managed_hooks_reject_empty_commands() { + let temp = tempdir().expect("create temp dir"); + let config_layer_stack = required_hooks_stack( + managed_hooks_for_current_platform(temp.path(), pre_tool_use_hook_events(" ")), + RequirementSource::LegacyManagedConfigTomlFromMdm, + ); + + let error = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .err() + .expect("empty required managed command should reject startup"); + + assert!(error.to_string().contains("skipping empty hook command")); +} + +#[test] +fn required_managed_hooks_reject_unsupported_handler_types() { + let temp = tempdir().expect("create temp dir"); + let events = HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::McpTool { + server: "policy".to_string(), + tool: "check".to_string(), + input: Default::default(), + timeout_sec: None, + status_message: None, + }], + }], + ..Default::default() + }; + let config_layer_stack = required_hooks_stack( + managed_hooks_for_current_platform(temp.path(), events), + RequirementSource::LegacyManagedConfigTomlFromMdm, + ); + + let error = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .err() + .expect("unsupported required managed handler should reject startup"); + + assert!( + error + .to_string() + .contains("MCP tool hooks are not supported yet") + ); +} + +#[test] +fn required_managed_hooks_with_unknown_source_still_reject_discovery_failures() { + let temp = tempdir().expect("create temp dir"); + let config_layer_stack = required_hooks_stack( + managed_hooks_for_current_platform(temp.path(), pre_tool_use_hook_events("")), + RequirementSource::Unknown, + ); + + let error = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .err() + .expect("unknown-source managed requirements hook should still reject startup"); + + assert!(error.to_string().contains("skipping empty hook command")); +} + +#[test] +fn valid_required_managed_hooks_allow_startup() { + let temp = tempdir().expect("create temp dir"); + let config_layer_stack = required_hooks_stack( + managed_hooks_for_current_platform(temp.path(), pre_tool_use_hook_events("echo managed")), + RequirementSource::LegacyManagedConfigTomlFromMdm, + ); + + let (hooks, _result_receiver) = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .expect("valid managed requirements hook should allow startup"); + + assert!(hooks.startup_warnings().is_empty()); +} + +#[test] +fn managed_config_layer_hook_failures_remain_startup_warnings() { + let temp = tempdir().expect("create temp dir"); + let config_path = + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"); + let config_layer_stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::System { file: config_path }, + config_toml_with_pre_tool_use(" "), + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack"); + + let (hooks, _result_receiver) = crate::Hooks::new( + crate::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config_layer_stack), + ..Default::default() + }, + ThreadId::new(), + ) + .expect("managed config layer hooks should remain optional"); + + assert_eq!(hooks.startup_warnings().len(), 1); + assert!(hooks.startup_warnings()[0].contains("skipping empty hook command")); +} + #[tokio::test] async fn requirements_managed_hooks_execute_from_managed_dir() { let temp = tempdir().expect("create temp dir"); diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index 9d857602fe..8f3680c1b2 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -57,16 +57,24 @@ pub struct Hooks { } impl Hooks { - /// Bind this session's hook runtime and output files to its thread. + /// Bind this session's hook runtime and output files to its thread, rejecting unloadable + /// required managed hooks. pub fn new( config: HooksConfig, thread_id: ThreadId, - ) -> (Self, Receiver) { + ) -> anyhow::Result<(Self, Receiver)> { let (result_sender, result_receiver) = async_channel::unbounded(); let hooks = Self::from_config(config, |shell| { CommandHookRuntime::new(shell, thread_id, result_sender) }); - (hooks, result_receiver) + let required_load_errors = hooks.engine.required_load_errors(); + if !required_load_errors.is_empty() { + anyhow::bail!( + "failed to load required managed hooks: {}", + required_load_errors.join("; ") + ); + } + Ok((hooks, result_receiver)) } /// Preserve in-flight background hooks while applying a refreshed configuration.