mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
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
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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(
|
||||
&[],
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -40,6 +40,7 @@ pub(crate) struct DiscoveryResult {
|
||||
pub handlers: Vec<ConfiguredHandler>,
|
||||
pub hook_entries: Vec<HookListEntry>,
|
||||
pub warnings: Vec<String>,
|
||||
pub required_load_errors: Vec<String>,
|
||||
}
|
||||
|
||||
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<String, HookStateToml>,
|
||||
env: HashMap<String, String>,
|
||||
plugin_id: Option<String>,
|
||||
}
|
||||
|
||||
enum HookRequirement<'a> {
|
||||
Required(&'a mut Vec<String>),
|
||||
Optional,
|
||||
}
|
||||
|
||||
impl HookHandlerSource<'_> {
|
||||
fn record_load_failure(&mut self, warning: String, warnings: &mut Vec<String>) {
|
||||
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<String, HookStateToml>,
|
||||
policy: HookDiscoveryPolicy,
|
||||
) {
|
||||
) -> Vec<String> {
|
||||
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<HookListEntry>,
|
||||
warnings: &mut Vec<String>,
|
||||
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<HookListEntry>,
|
||||
warnings: &mut Vec<String>,
|
||||
display_order: &mut i64,
|
||||
source: &HookHandlerSource<'_>,
|
||||
source: &mut HookHandlerSource<'_>,
|
||||
event_name: codex_protocol::protocol::HookEventName,
|
||||
groups: Vec<MatcherGroup>,
|
||||
) {
|
||||
@@ -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()),
|
||||
|
||||
@@ -146,6 +146,7 @@ pub struct HookListEntry {
|
||||
pub(crate) struct ClaudeHooksEngine {
|
||||
pub(crate) handlers: Vec<ConfiguredHandler>,
|
||||
warnings: Vec<String>,
|
||||
required_load_errors: Vec<String>,
|
||||
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,
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<codex_protocol::protocol::HookCompletedEvent>) {
|
||||
) -> anyhow::Result<(Self, Receiver<codex_protocol::protocol::HookCompletedEvent>)> {
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user