From dafa805c3c56d5c2b504d54b93a4954b3651368b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 30 Apr 2026 12:19:03 -0700 Subject: [PATCH] Gate runnable hooks on trust --- codex-rs/app-server/README.md | 6 +- .../app-server/tests/suite/v2/hooks_list.rs | 31 ++++++++-- codex-rs/hooks/src/engine/discovery.rs | 10 +++- codex-rs/hooks/src/engine/mod_tests.rs | 60 +++++++++++++++++-- 4 files changed, 94 insertions(+), 13 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 91cee0a6c9..0184785c62 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1457,7 +1457,11 @@ To enable or disable a skill by name: } ``` -Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. Each entry is evaluated using that `cwd`'s effective config, so feature gating and discovered config layers can differ across entries in the same request. Disabled hooks are still returned with `"enabled": false` so clients can render and re-enable them. Hook state is stored under `hooks.state`; clients should treat hooks from managed sources as non-configurable, and user config entries for those keys are ignored during loading. Unmanaged hooks expose a `currentHash`, persisted `trustedHash`, and derived `trustStatus` so clients can distinguish first-seen hooks from hooks whose approved contents have changed. Hook keys combine the source identity with a trailing event/group/handler selector that is currently positional. +Use `hooks/list` to fetch discovered hooks for one or more `cwds`. Each result is evaluated with that `cwd`'s effective config, so feature gates and discovered config layers can differ within a single response. + +Hooks are returned even when disabled so clients can render and re-enable them. User-controlled state lives under `hooks.state`. Managed hooks are non-configurable, and user entries for managed hook keys are ignored during loading. + +For unmanaged hooks, `currentHash`, `trustedHash`, and `trustStatus` describe whether the current definition is first-seen, approved, or changed since approval. Only trusted unmanaged hooks become runnable. Hook keys combine the source identity with a trailing event/group/handler selector that is currently positional. ```json { diff --git a/codex-rs/app-server/tests/suite/v2/hooks_list.rs b/codex-rs/app-server/tests/suite/v2/hooks_list.rs index e76a610d3e..740b752b23 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -161,7 +161,7 @@ async fn hooks_list_shows_discovered_hook() -> Result<()> { "pre_tool_use", Some("Bash"), "python3 /tmp/listed-hook.py", - 5, + /*timeout_sec*/ 5, Some("running listed hook"), ), trusted_hash: None, @@ -240,7 +240,7 @@ async fn hooks_list_shows_discovered_plugin_hook() -> Result<()> { "pre_tool_use", Some("Bash"), "echo plugin hook", - 7, + /*timeout_sec*/ 7, Some("running plugin hook"), ), trusted_hash: None, @@ -366,8 +366,8 @@ timeout = 5 "pre_tool_use", Some("Bash"), "echo project hook", - 5, - None, + /*timeout_sec*/ 5, + /*status_message*/ None, ), trusted_hash: None, trust_status: HookTrustStatus::Untrusted, @@ -553,6 +553,29 @@ command = "python3 {hook_script_path}" let hook = &data[0].hooks[0]; assert_eq!(hook.enabled, true); + let write_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: "hooks.state".to_string(), + value: serde_json::json!({ + hook.key.clone(): { + "trusted_hash": hook.current_hash.clone() + } + }), + merge_strategy: MergeStrategy::Upsert, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(write_id)), + ) + .await??; + let _: codex_app_server_protocol::ConfigWriteResponse = to_response(response)?; + let thread_start_id = mcp .send_thread_start_request(ThreadStartParams { model: Some("mock-model".to_string()), diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 4d441c7bb1..1f8503bdff 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -162,7 +162,6 @@ fn append_plugin_hook_sources( plugin_hook_sources: Vec, hook_states: &HashMap, ) { - // TODO(abhinav): check enabled/trusted state here before plugin hooks become runnable. for source in plugin_hook_sources { let PluginHookSource { plugin_root, @@ -444,7 +443,12 @@ fn append_matcher_groups( trusted_hash, trust_status, }); - if enabled { + if enabled + && matches!( + trust_status, + HookTrustStatus::Managed | HookTrustStatus::Trusted + ) + { handlers.push(ConfiguredHandler { event_name, matcher: matcher.map(ToOwned::to_owned), @@ -593,7 +597,7 @@ mod tests { } fn hook_source() -> HookSource { - HookSource::User + HookSource::System } fn hook_handler_source<'a>( diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index aa88c9a281..7835ddbd18 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -326,6 +326,7 @@ fn unmanaged_hook_trust_status_tracks_stored_hash() { HookTrustStatus::Untrusted ); assert_eq!(untrusted.hook_entries[0].trusted_hash, None); + assert_eq!(untrusted.handlers, Vec::new()); let current_hash = untrusted.hook_entries[0].current_hash.clone(); let trusted_stack = ConfigLayerStack::new( @@ -351,6 +352,7 @@ fn unmanaged_hook_trust_status_tracks_stored_hash() { HookTrustStatus::Trusted ); assert_eq!(trusted.hook_entries[0].trusted_hash, Some(current_hash)); + assert_eq!(trusted.handlers.len(), 1); let changed_stack = ConfigLayerStack::new( vec![ConfigLayerEntry::new( @@ -380,6 +382,7 @@ fn unmanaged_hook_trust_status_tracks_stored_hash() { changed.hook_entries[0].trusted_hash.as_deref(), Some("sha256:old") ); + assert_eq!(changed.handlers, Vec::new()); } fn config_with_hook_state(key: &str, enabled: bool) -> TomlValue { @@ -452,6 +455,45 @@ fn config_with_pre_tool_use_hook(command: &str) -> TomlValue { .expect("config TOML should deserialize") } +fn trusted_plugin_hook_stack( + config_path: AbsolutePathBuf, + plugin_hook_sources: &[PluginHookSource], +) -> ConfigLayerStack { + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + plugin_hook_sources.to_vec(), + Vec::new(), + ); + let state = discovered + .hook_entries + .into_iter() + .map(|entry| { + ( + entry.key, + serde_json::json!({ + "trusted_hash": entry.current_hash, + }), + ) + }) + .collect::>(); + let config = serde_json::from_value(serde_json::json!({ + "hooks": { + "state": state, + }, + })) + .expect("config TOML should deserialize"); + + ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { file: config_path }, + config, + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack") +} + #[test] fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { let temp = tempdir().expect("create temp dir"); @@ -586,7 +628,7 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { config_table.insert("hooks".to_string(), hooks_table); let config_layer_stack = ConfigLayerStack::new( vec![ConfigLayerEntry::new( - ConfigLayerSource::User { + ConfigLayerSource::System { file: config_path.clone(), }, config_toml, @@ -631,7 +673,7 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { engine .handlers .iter() - .all(|handler| !handler.source.is_managed()) + .all(|handler| handler.source.is_managed()) ); assert_eq!(preview[0].source_path, hooks_json_path); assert_eq!(preview[1].source_path, config_path); @@ -680,10 +722,14 @@ print(json.dumps({ ..Default::default() }, }]; + let config_layer_stack = trusted_plugin_hook_stack( + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"), + &plugin_hook_sources, + ); let engine = ClaudeHooksEngine::new( /*enabled*/ true, - /*config_layer_stack*/ None, - plugin_hook_sources.clone(), + Some(&config_layer_stack), + plugin_hook_sources, Vec::new(), CommandShell { program: String::new(), @@ -784,9 +830,13 @@ fn plugin_hook_sources_expand_plugin_placeholders() { ..Default::default() }, }]; + let config_layer_stack = trusted_plugin_hook_stack( + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"), + &plugin_hook_sources, + ); let engine = ClaudeHooksEngine::new( /*enabled*/ true, - /*config_layer_stack*/ None, + Some(&config_layer_stack), plugin_hook_sources, Vec::new(), CommandShell {