From 8f3c06cc97bbb045fe5790a6388625c0db35af7f Mon Sep 17 00:00:00 2001 From: Abhinav Date: Wed, 29 Apr 2026 21:46:32 -0700 Subject: [PATCH] Add persisted hook enablement state (#19840) ## Why After `hooks/list` exposes the hook inventory, clients need a way to persist user hook preferences, make those changes effective in already-open sessions, and distinguish user-controllable hooks from managed requirements without adding another bespoke app-server write API. ## What - Extends `hooks/list` entries with effective `enabled` state. - Persists user-level hook state under `hooks.state.` so the model can grow beyond a single boolean over time. - Uses the existing `config/batchWrite` path for hook state updates instead of introducing a dedicated hook write RPC. - Refreshes live session hook engines after config writes so already-open threads observe updated enablement without a restart. ## Stack 1. openai/codex#19705 2. openai/codex#19778 3. This PR - openai/codex#19840 4. openai/codex#19882 ## Reviewer Notes The generated schema files account for much of the raw diff. The core behavior is in: - `hooks/src/config_rules.rs`, which resolves per-hook user state from the config layer stack. - `hooks/src/engine/discovery.rs`, which projects effective enablement into `hooks/list` from source-derived managedness. - `config/src/hook_config.rs`, which defines the new `hooks.state` representation. - `core/src/session/mod.rs`, which rebuilds live hook state after user config reloads. --------- Co-authored-by: Codex --- .../analytics/src/analytics_client_tests.rs | 11 + codex-rs/analytics/src/events.rs | 1 + .../schema/json/ServerNotification.json | 1 + .../codex_app_server_protocol.schemas.json | 13 + .../codex_app_server_protocol.v2.schemas.json | 13 + .../json/v2/HookCompletedNotification.json | 1 + .../json/v2/HookStartedNotification.json | 1 + .../schema/json/v2/HooksListResponse.json | 13 + .../schema/typescript/v2/HookMetadata.ts | 2 +- .../schema/typescript/v2/HookSource.ts | 2 +- .../app-server-protocol/src/protocol/v2.rs | 4 + codex-rs/app-server/README.md | 29 +- .../app-server/src/codex_message_processor.rs | 3 + .../app-server/tests/suite/v2/hooks_list.rs | 313 +++++++++++++++++- codex-rs/config/src/config_toml.rs | 6 +- codex-rs/config/src/hook_config.rs | 15 + codex-rs/config/src/hooks_tests.rs | 45 +++ codex-rs/config/src/lib.rs | 2 + codex-rs/core/config.schema.json | 112 ++++--- codex-rs/core/src/hook_runtime.rs | 39 ++- codex-rs/core/src/mcp_tool_call_tests.rs | 25 +- codex-rs/core/src/session/mod.rs | 73 +++- codex-rs/core/src/session/session.rs | 27 +- codex-rs/core/src/session/tests.rs | 45 ++- codex-rs/core/src/session/turn.rs | 5 +- codex-rs/core/src/state/service.rs | 3 +- .../runtimes/shell/unix_escalation_tests.rs | 20 +- codex-rs/hooks/src/config_rules.rs | 188 +++++++++++ codex-rs/hooks/src/engine/discovery.rs | 160 +++++++-- codex-rs/hooks/src/engine/dispatcher.rs | 1 - codex-rs/hooks/src/engine/mod.rs | 4 +- codex-rs/hooks/src/engine/mod_tests.rs | 180 +++++++++- codex-rs/hooks/src/events/post_tool_use.rs | 1 - codex-rs/hooks/src/events/pre_tool_use.rs | 1 - codex-rs/hooks/src/events/session_start.rs | 1 - codex-rs/hooks/src/events/stop.rs | 1 - .../hooks/src/events/user_prompt_submit.rs | 1 - codex-rs/hooks/src/lib.rs | 1 + codex-rs/protocol/src/protocol.rs | 30 ++ 39 files changed, 1212 insertions(+), 181 deletions(-) create mode 100644 codex-rs/hooks/src/config_rules.rs diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 0b5fc3b87..515fcc563 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -1559,6 +1559,15 @@ fn hook_run_metadata_maps_sources_and_statuses() { }, )) .expect("serialize project hook"); + let cloud_requirements = serde_json::to_value(codex_hook_run_metadata( + &tracking, + HookRunFact { + event_name: HookEventName::Stop, + hook_source: HookSource::CloudRequirements, + status: HookRunStatus::Blocked, + }, + )) + .expect("serialize cloud requirements hook"); let unknown = serde_json::to_value(codex_hook_run_metadata( &tracking, HookRunFact { @@ -1573,6 +1582,8 @@ fn hook_run_metadata_maps_sources_and_statuses() { assert_eq!(system["status"], "completed"); assert_eq!(project["hook_source"], "project"); assert_eq!(project["status"], "blocked"); + assert_eq!(cloud_requirements["hook_source"], "cloud_requirements"); + assert_eq!(cloud_requirements["status"], "blocked"); assert_eq!(unknown["hook_source"], "unknown"); assert_eq!(unknown["status"], "failed"); } diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 24ae8e00b..7120960b6 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -685,6 +685,7 @@ fn analytics_hook_source(source: HookSource) -> &'static str { HookSource::Mdm => "mdm", HookSource::SessionFlags => "session_flags", HookSource::Plugin => "plugin", + HookSource::CloudRequirements => "cloud_requirements", HookSource::LegacyManagedConfigFile => "legacy_managed_config_file", HookSource::LegacyManagedConfigMdm => "legacy_managed_config_mdm", HookSource::Unknown => "unknown", diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 5bbd51bb9..32ab775c1 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1901,6 +1901,7 @@ "mdm", "sessionFlags", "plugin", + "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", "unknown" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index ea9c050fc..59c653096 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -9698,12 +9698,21 @@ "format": "int64", "type": "integer" }, + "enabled": { + "type": "boolean" + }, "eventName": { "$ref": "#/definitions/v2/HookEventName" }, "handlerType": { "$ref": "#/definitions/v2/HookHandlerType" }, + "isManaged": { + "type": "boolean" + }, + "key": { + "type": "string" + }, "matcher": { "type": [ "string", @@ -9736,8 +9745,11 @@ }, "required": [ "displayOrder", + "enabled", "eventName", "handlerType", + "isManaged", + "key", "source", "sourcePath", "timeoutSec" @@ -9900,6 +9912,7 @@ "mdm", "sessionFlags", "plugin", + "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", "unknown" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 3cb4e970b..45a6147bf 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -6308,12 +6308,21 @@ "format": "int64", "type": "integer" }, + "enabled": { + "type": "boolean" + }, "eventName": { "$ref": "#/definitions/HookEventName" }, "handlerType": { "$ref": "#/definitions/HookHandlerType" }, + "isManaged": { + "type": "boolean" + }, + "key": { + "type": "string" + }, "matcher": { "type": [ "string", @@ -6346,8 +6355,11 @@ }, "required": [ "displayOrder", + "enabled", "eventName", "handlerType", + "isManaged", + "key", "source", "sourcePath", "timeoutSec" @@ -6510,6 +6522,7 @@ "mdm", "sessionFlags", "plugin", + "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", "unknown" diff --git a/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json index 7c03e3554..d55c059a7 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json @@ -161,6 +161,7 @@ "mdm", "sessionFlags", "plugin", + "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", "unknown" diff --git a/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json index d08300d52..03d2998ca 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json @@ -161,6 +161,7 @@ "mdm", "sessionFlags", "plugin", + "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", "unknown" diff --git a/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json index c345a35d0..5190b2271 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json @@ -51,12 +51,21 @@ "format": "int64", "type": "integer" }, + "enabled": { + "type": "boolean" + }, "eventName": { "$ref": "#/definitions/HookEventName" }, "handlerType": { "$ref": "#/definitions/HookHandlerType" }, + "isManaged": { + "type": "boolean" + }, + "key": { + "type": "string" + }, "matcher": { "type": [ "string", @@ -89,8 +98,11 @@ }, "required": [ "displayOrder", + "enabled", "eventName", "handlerType", + "isManaged", + "key", "source", "sourcePath", "timeoutSec" @@ -105,6 +117,7 @@ "mdm", "sessionFlags", "plugin", + "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", "unknown" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts index 074d0cfb6..8ccd2b182 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts @@ -6,4 +6,4 @@ import type { HookEventName } from "./HookEventName"; import type { HookHandlerType } from "./HookHandlerType"; import type { HookSource } from "./HookSource"; -export type HookMetadata = { eventName: HookEventName, handlerType: HookHandlerType, matcher: string | null, command: string | null, timeoutSec: bigint, statusMessage: string | null, sourcePath: AbsolutePathBuf, source: HookSource, pluginId: string | null, displayOrder: bigint, }; +export type HookMetadata = { key: string, eventName: HookEventName, handlerType: HookHandlerType, matcher: string | null, command: string | null, timeoutSec: bigint, statusMessage: string | null, sourcePath: AbsolutePathBuf, source: HookSource, pluginId: string | null, displayOrder: bigint, enabled: boolean, isManaged: boolean, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts index 24a06bd13..98bbe1e41 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts @@ -2,4 +2,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -export type HookSource = "system" | "user" | "project" | "mdm" | "sessionFlags" | "plugin" | "legacyManagedConfigFile" | "legacyManagedConfigMdm" | "unknown"; +export type HookSource = "system" | "user" | "project" | "mdm" | "sessionFlags" | "plugin" | "cloudRequirements" | "legacyManagedConfigFile" | "legacyManagedConfigMdm" | "unknown"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 17aafe1ae..2c1f2abc3 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -472,6 +472,7 @@ v2_enum_from_core!( Mdm, SessionFlags, Plugin, + CloudRequirements, LegacyManagedConfigFile, LegacyManagedConfigMdm, Unknown, @@ -4716,6 +4717,7 @@ pub struct HooksListEntry { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct HookMetadata { + pub key: String, pub event_name: HookEventName, pub handler_type: HookHandlerType, pub matcher: Option, @@ -4726,6 +4728,8 @@ pub struct HookMetadata { pub source: HookSource, pub plugin_id: Option, pub display_order: i64, + pub enabled: bool, + pub is_managed: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index ce86a4df0..3aae4ae61 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1456,7 +1456,7 @@ To enable or disable a skill by name: } ``` -Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. +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. Hook keys combine the source identity with a trailing event/group/handler selector that is currently positional. ```json { @@ -1475,8 +1475,10 @@ Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. "data": [{ "cwd": "/Users/me/project", "hooks": [{ + "key": "/Users/me/.codex/config.toml:pre_tool_use:0:0", "eventName": "pre_tool_use", "handlerType": "command", + "isManaged": false, "matcher": "Bash", "command": "python3 /Users/me/hook.py", "timeoutSec": 5, @@ -1484,7 +1486,8 @@ Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. "sourcePath": "/Users/me/.codex/config.toml", "source": "user", "pluginId": null, - "displayOrder": 0 + "displayOrder": 0, + "enabled": true }], "warnings": [], "errors": [] @@ -1493,6 +1496,28 @@ Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. } ``` +To disable a non-managed hook, upsert a state entry at `hooks.state` with `config/batchWrite`: + +```json +{ + "method": "config/batchWrite", + "id": 29, + "params": { + "edits": [{ + "keyPath": "hooks.state", + "value": { + "/Users/me/.codex/config.toml:pre_tool_use:0:0": { + "enabled": false + } + }, + "mergeStrategy": "upsert" + }], + "reloadUserConfig": true + } +} +``` + +To re-enable it, upsert the same hook key with `"enabled": true`. ## Apps Use `app/list` to fetch available apps (connectors). Each entry includes metadata like the app `id`, display `name`, `installUrl`, `branding`, `appMetadata`, `labels`, whether it is currently accessible, and whether it is enabled in config. diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 853e76564..ea82c306b 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -8728,6 +8728,7 @@ fn hooks_to_info(hooks: &[codex_hooks::HookListEntry]) -> Vec { hooks .iter() .map(|hook| HookMetadata { + key: hook.key.clone(), event_name: hook.event_name.into(), handler_type: hook.handler_type.into(), matcher: hook.matcher.clone(), @@ -8738,6 +8739,8 @@ fn hooks_to_info(hooks: &[codex_hooks::HookListEntry]) -> Vec { source: hook.source.into(), plugin_id: hook.plugin_id.clone(), display_order: hook.display_order, + enabled: hook.enabled, + is_managed: hook.is_managed, }) .collect() } 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 67d2821a1..87f4decc2 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -2,7 +2,11 @@ use std::time::Duration; use anyhow::Result; use app_test_support::McpProcess; +use app_test_support::create_final_assistant_message_sse_response; +use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; +use codex_app_server_protocol::ConfigBatchWriteParams; +use codex_app_server_protocol::ConfigEdit; use codex_app_server_protocol::HookEventName; use codex_app_server_protocol::HookHandlerType; use codex_app_server_protocol::HookMetadata; @@ -11,10 +15,16 @@ use codex_app_server_protocol::HooksListEntry; use codex_app_server_protocol::HooksListParams; use codex_app_server_protocol::HooksListResponse; use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::MergeStrategy; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; +use codex_app_server_protocol::TurnStartParams; +use codex_app_server_protocol::UserInput as V2UserInput; use codex_core::config::set_project_trust_level; use codex_protocol::config_types::TrustLevel; use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::skip_if_windows; use pretty_assertions::assert_eq; use tempfile::TempDir; use tokio::time::timeout; @@ -82,23 +92,27 @@ async fn hooks_list_shows_discovered_hook() -> Result<()> { ) .await??; let HooksListResponse { data } = to_response(response)?; + let config_path = AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( + codex_home.path().join("config.toml"), + )?)?; assert_eq!( data, vec![HooksListEntry { cwd: cwd.path().to_path_buf(), hooks: vec![HookMetadata { + key: format!("{}:pre_tool_use:0:0", config_path.as_path().display()), event_name: HookEventName::PreToolUse, handler_type: HookHandlerType::Command, matcher: Some("Bash".to_string()), command: Some("python3 /tmp/listed-hook.py".to_string()), timeout_sec: 5, status_message: Some("running listed hook".to_string()), - source_path: AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( - codex_home.path().join("config.toml") - )?,)?, + source_path: config_path, source: HookSource::User, plugin_id: None, display_order: 0, + enabled: true, + is_managed: false, }], warnings: Vec::new(), errors: Vec::new(), @@ -146,25 +160,29 @@ async fn hooks_list_shows_discovered_plugin_hook() -> Result<()> { ) .await??; let HooksListResponse { data } = to_response(response)?; + let plugin_hooks_path = AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( + codex_home + .path() + .join("plugins/cache/test/demo/local/hooks/hooks.json"), + )?)?; assert_eq!( data, vec![HooksListEntry { cwd: cwd.path().to_path_buf(), hooks: vec![HookMetadata { + key: "demo@test:hooks/hooks.json:pre_tool_use:0:0".to_string(), event_name: HookEventName::PreToolUse, handler_type: HookHandlerType::Command, matcher: Some("Bash".to_string()), command: Some("echo plugin hook".to_string()), timeout_sec: 7, status_message: Some("running plugin hook".to_string()), - source_path: AbsolutePathBuf::from_absolute_path(std::fs::canonicalize( - codex_home - .path() - .join("plugins/cache/test/demo/local/hooks/hooks.json"), - )?,)?, + source_path: plugin_hooks_path, source: HookSource::Plugin, plugin_id: Some("demo@test".to_string()), display_order: 0, + enabled: true, + is_managed: false, }], warnings: Vec::new(), errors: Vec::new(), @@ -252,6 +270,8 @@ timeout = 5 ) .await??; let HooksListResponse { data } = to_response(response)?; + let project_config_path = + AbsolutePathBuf::try_from(workspace.path().join(".codex/config.toml"))?; assert_eq!( data, vec![ @@ -264,18 +284,22 @@ timeout = 5 HooksListEntry { cwd: workspace.path().to_path_buf(), hooks: vec![HookMetadata { + key: format!( + "{}:pre_tool_use:0:0", + project_config_path.as_path().display() + ), event_name: HookEventName::PreToolUse, handler_type: HookHandlerType::Command, matcher: Some("Bash".to_string()), command: Some("echo project hook".to_string()), timeout_sec: 5, status_message: None, - source_path: AbsolutePathBuf::try_from( - workspace.path().join(".codex/config.toml"), - )?, + source_path: project_config_path, source: HookSource::Project, plugin_id: None, display_order: 0, + enabled: true, + is_managed: false, }], warnings: Vec::new(), errors: Vec::new(), @@ -284,3 +308,270 @@ timeout = 5 ); Ok(()) } + +#[tokio::test] +async fn config_batch_write_toggles_user_hook() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + write_user_hook_config(codex_home.path())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![cwd.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + 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(): { + "enabled": false + } + }), + 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 request_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![cwd.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + assert_eq!(data[0].hooks.len(), 1); + assert_eq!(data[0].hooks[0].key, hook.key); + assert_eq!(data[0].hooks[0].enabled, false); + + 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(): { + "enabled": true + } + }), + 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 request_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![cwd.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + assert_eq!(data[0].hooks[0].enabled, true); + Ok(()) +} + +#[tokio::test] +async fn config_batch_write_disables_hook_for_loaded_session() -> Result<()> { + skip_if_windows!(Ok(())); + + let responses = vec![ + create_final_assistant_message_sse_response("Warmup")?, + create_final_assistant_message_sse_response("First turn")?, + create_final_assistant_message_sse_response("Second turn")?, + ]; + let server = create_mock_responses_server_sequence_unchecked(responses).await; + let codex_home = TempDir::new()?; + let hook_script_path = codex_home.path().join("user_prompt_submit_hook.py"); + let hook_log_path = codex_home.path().join("user_prompt_submit_hook_log.jsonl"); + std::fs::write( + &hook_script_path, + 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") +"#, + hook_log_path = hook_log_path.display(), + ), + )?; + std::fs::write( + codex_home.path().join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 + +[hooks] + +[[hooks.UserPromptSubmit]] + +[[hooks.UserPromptSubmit.hooks]] +type = "command" +command = "python3 {hook_script_path}" +"#, + server_uri = server.uri(), + hook_script_path = hook_script_path.display(), + ), + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let hook_list_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![codex_home.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(hook_list_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + let hook = &data[0].hooks[0]; + assert_eq!(hook.enabled, true); + + let thread_start_id = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + ..Default::default() + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response(response)?; + + let first_turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "first turn".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(first_turn_id)), + ) + .await??; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + assert_eq!( + std::fs::read_to_string(&hook_log_path)? + .lines() + .filter(|line| !line.is_empty()) + .count(), + 1 + ); + + 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(): { + "enabled": false + } + }), + 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 second_turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + input: vec![V2UserInput::Text { + text: "second turn".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_turn_id)), + ) + .await??; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + assert_eq!( + std::fs::read_to_string(&hook_log_path)? + .lines() + .filter(|line| !line.is_empty()) + .count(), + 1 + ); + Ok(()) +} diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index 3bd19f556..2e89550dd 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -4,7 +4,7 @@ use std::collections::BTreeMap; use std::collections::HashMap; use std::path::Path; -use crate::HookEventsToml; +use crate::HooksToml; use crate::permissions_toml::PermissionsToml; use crate::profile_toml::ConfigProfile; use crate::types::AnalyticsConfigToml; @@ -343,8 +343,8 @@ pub struct ConfigToml { /// User-level skill config entries keyed by SKILL.md path. pub skills: Option, - /// Lifecycle hooks configured inline in TOML. - pub hooks: Option, + /// Lifecycle hooks configured inline in TOML plus user-level overrides. + pub hooks: Option, /// User-level plugin config entries keyed by plugin name. #[serde(default)] diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs index 8a5c73d6b..d947ebb86 100644 --- a/codex-rs/config/src/hook_config.rs +++ b/codex-rs/config/src/hook_config.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::path::Path; use std::path::PathBuf; @@ -12,6 +13,20 @@ pub struct HooksFile { pub hooks: HookEventsToml, } +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct HooksToml { + #[serde(flatten)] + pub events: HookEventsToml, + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub state: BTreeMap, +} + +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct HookStateToml { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub enabled: Option, +} + #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct HookEventsToml { #[serde(rename = "PreToolUse", default)] diff --git a/codex-rs/config/src/hooks_tests.rs b/codex-rs/config/src/hooks_tests.rs index 5e3f1df67..93541ee7f 100644 --- a/codex-rs/config/src/hooks_tests.rs +++ b/codex-rs/config/src/hooks_tests.rs @@ -1,8 +1,11 @@ use pretty_assertions::assert_eq; +use std::collections::BTreeMap; + use super::HookEventsToml; use super::HookHandlerConfig; use super::HooksFile; +use super::HooksToml; use super::ManagedHooksRequirementsToml; use super::MatcherGroup; @@ -81,6 +84,48 @@ statusMessage = "checking" ); } +#[test] +fn hooks_toml_deserializes_inline_events_and_state_map() { + let parsed: HooksToml = toml::from_str( + r#" +[state."/tmp/hooks.json:pre_tool_use:0:0"] +enabled = false + +[[PreToolUse]] +matcher = "^Bash$" + +[[PreToolUse.hooks]] +type = "command" +command = "python3 /tmp/pre.py" +"#, + ) + .expect("hooks TOML should deserialize"); + + assert_eq!( + parsed, + HooksToml { + events: HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "python3 /tmp/pre.py".to_string(), + timeout_sec: None, + r#async: false, + status_message: None, + }], + }], + ..Default::default() + }, + state: BTreeMap::from([( + "/tmp/hooks.json:pre_tool_use:0:0".to_string(), + super::HookStateToml { + enabled: Some(false), + }, + )]), + } + ); +} + #[test] fn managed_hooks_requirements_flatten_hook_events() { let parsed: ManagedHooksRequirementsToml = toml::from_str( diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 3ad16cc34..cf3dd68dc 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -73,7 +73,9 @@ pub use diagnostics::io_error_from_config_error; pub use fingerprint::version_for_toml; pub use hook_config::HookEventsToml; pub use hook_config::HookHandlerConfig; +pub use hook_config::HookStateToml; pub use hook_config::HooksFile; +pub use hook_config::HooksToml; pub use hook_config::ManagedHooksRequirementsToml; pub use hook_config::MatcherGroup; pub use host_name::host_name; diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index c8640c514..28c81d69c 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -884,53 +884,6 @@ } ] }, - "HookEventsToml": { - "properties": { - "PermissionRequest": { - "default": [], - "items": { - "$ref": "#/definitions/MatcherGroup" - }, - "type": "array" - }, - "PostToolUse": { - "default": [], - "items": { - "$ref": "#/definitions/MatcherGroup" - }, - "type": "array" - }, - "PreToolUse": { - "default": [], - "items": { - "$ref": "#/definitions/MatcherGroup" - }, - "type": "array" - }, - "SessionStart": { - "default": [], - "items": { - "$ref": "#/definitions/MatcherGroup" - }, - "type": "array" - }, - "Stop": { - "default": [], - "items": { - "$ref": "#/definitions/MatcherGroup" - }, - "type": "array" - }, - "UserPromptSubmit": { - "default": [], - "items": { - "$ref": "#/definitions/MatcherGroup" - }, - "type": "array" - } - }, - "type": "object" - }, "HookHandlerConfig": { "oneOf": [ { @@ -995,6 +948,67 @@ } ] }, + "HookStateToml": { + "properties": { + "enabled": { + "type": "boolean" + } + }, + "type": "object" + }, + "HooksToml": { + "properties": { + "PermissionRequest": { + "default": [], + "items": { + "$ref": "#/definitions/MatcherGroup" + }, + "type": "array" + }, + "PostToolUse": { + "default": [], + "items": { + "$ref": "#/definitions/MatcherGroup" + }, + "type": "array" + }, + "PreToolUse": { + "default": [], + "items": { + "$ref": "#/definitions/MatcherGroup" + }, + "type": "array" + }, + "SessionStart": { + "default": [], + "items": { + "$ref": "#/definitions/MatcherGroup" + }, + "type": "array" + }, + "Stop": { + "default": [], + "items": { + "$ref": "#/definitions/MatcherGroup" + }, + "type": "array" + }, + "UserPromptSubmit": { + "default": [], + "items": { + "$ref": "#/definitions/MatcherGroup" + }, + "type": "array" + }, + "state": { + "additionalProperties": { + "$ref": "#/definitions/HookStateToml" + }, + "type": "object" + } + }, + "type": "object" + }, "KeybindingsSpec": { "anyOf": [ { @@ -3653,10 +3667,10 @@ "hooks": { "allOf": [ { - "$ref": "#/definitions/HookEventsToml" + "$ref": "#/definitions/HooksToml" } ], - "description": "Lifecycle hooks configured inline in TOML." + "description": "Lifecycle hooks configured inline in TOML plus user-level overrides." }, "include_apps_instructions": { "description": "Whether to inject the `` developer block.", diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index b534c63cf..9a9285451 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -116,13 +116,13 @@ pub(crate) async fn run_pending_session_start_hooks( permission_mode: hook_permission_mode(turn_context), source: session_start_source, }; - let preview_runs = sess.hooks().preview_session_start(&request); + let hooks = sess.hooks(); + let preview_runs = hooks.preview_session_start(&request); run_context_injecting_hook( sess, turn_context, preview_runs, - sess.hooks() - .run_session_start(request, Some(turn_context.sub_id.clone())), + hooks.run_session_start(request, Some(turn_context.sub_id.clone())), ) .await .record_additional_contexts(sess, turn_context) @@ -153,14 +153,15 @@ pub(crate) async fn run_pre_tool_use_hooks( tool_use_id, tool_input: tool_input.clone(), }; - let preview_runs = sess.hooks().preview_pre_tool_use(&request); + let hooks = sess.hooks(); + let preview_runs = hooks.preview_pre_tool_use(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; let PreToolUseOutcome { hook_events, should_block, block_reason, - } = sess.hooks().run_pre_tool_use(request).await; + } = hooks.run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; if should_block { @@ -202,13 +203,14 @@ pub(crate) async fn run_permission_request_hooks( run_id_suffix: run_id_suffix.to_string(), tool_input: payload.tool_input, }; - let preview_runs = sess.hooks().preview_permission_request(&request); + let hooks = sess.hooks(); + let preview_runs = hooks.preview_permission_request(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; let PermissionRequestOutcome { hook_events, decision, - } = sess.hooks().run_permission_request(request).await; + } = hooks.run_permission_request(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; decision @@ -242,10 +244,11 @@ pub(crate) async fn run_post_tool_use_hooks( tool_input, tool_response, }; - let preview_runs = sess.hooks().preview_post_tool_use(&request); + let hooks = sess.hooks(); + let preview_runs = hooks.preview_post_tool_use(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; - let outcome = sess.hooks().run_post_tool_use(request).await; + let outcome = hooks.run_post_tool_use(request).await; emit_hook_completed_events(sess, turn_context, outcome.hook_events.clone()).await; outcome } @@ -264,12 +267,13 @@ pub(crate) async fn run_user_prompt_submit_hooks( permission_mode: hook_permission_mode(turn_context), prompt, }; - let preview_runs = sess.hooks().preview_user_prompt_submit(&request); + let hooks = sess.hooks(); + let preview_runs = hooks.preview_user_prompt_submit(&request); run_context_injecting_hook( sess, turn_context, preview_runs, - sess.hooks().run_user_prompt_submit(request), + hooks.run_user_prompt_submit(request), ) .await } @@ -474,6 +478,7 @@ fn hook_run_metric_tags(run: &HookRunSummary) -> [(&'static str, &'static str); HookSource::Mdm => "mdm", HookSource::SessionFlags => "session_flags", HookSource::Plugin => "plugin", + HookSource::CloudRequirements => "cloud_requirements", HookSource::LegacyManagedConfigFile => "legacy_managed_config_file", HookSource::LegacyManagedConfigMdm => "legacy_managed_config_mdm", HookSource::Unknown => "unknown", @@ -605,6 +610,18 @@ mod tests { ("status", "blocked"), ] ); + + let cloud_requirements = + sample_hook_run(HookRunStatus::Blocked, HookSource::CloudRequirements); + + assert_eq!( + hook_run_metric_tags(&cloud_requirements), + [ + ("hook_name", "Stop"), + ("source", "cloud_requirements"), + ("status", "blocked"), + ] + ); } #[test] diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 6f9c323eb..37df6dc6e 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -163,17 +163,20 @@ print({hook_output:?}) ) .expect("write hooks.json"); - session.services.hooks = Hooks::new(HooksConfig { - feature_enabled: true, - config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), - shell_program: (!cfg!(windows)).then_some("/bin/sh".to_string()), - shell_args: if cfg!(windows) { - Vec::new() - } else { - vec!["-c".to_string()] - }, - ..HooksConfig::default() - }); + session + .services + .hooks + .store(Arc::new(Hooks::new(HooksConfig { + feature_enabled: true, + config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + shell_program: (!cfg!(windows)).then_some("/bin/sh".to_string()), + shell_args: if cfg!(windows) { + Vec::new() + } else { + vec!["-c".to_string()] + }, + ..HooksConfig::default() + }))); log_path.to_path_buf() } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 65a5366dc..e45db2084 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -1381,6 +1381,9 @@ impl Session { } pub(crate) async fn reload_user_config_layer(&self) { + // Refresh layer-backed runtime state for an existing session, including enabled plugin, + // skill, and hook state. Derived config fields such as feature gates and legacy notify + // settings remain session-static. let config_toml_path = { let state = self.state.lock().await; state @@ -1406,16 +1409,36 @@ impl Session { } }; - let mut state = self.state.lock().await; - let mut config = (*state.session_configuration.original_config_do_not_use).clone(); - config.config_layer_stack = config - .config_layer_stack - .with_user_config(&config_toml_path, user_config); - config.tool_suggest = - resolve_tool_suggest_config_from_layer_stack(&config.config_layer_stack); - state.session_configuration.original_config_do_not_use = Arc::new(config); + let config = { + let mut state = self.state.lock().await; + let mut config = (*state.session_configuration.original_config_do_not_use).clone(); + config.config_layer_stack = config + .config_layer_stack + .with_user_config(&config_toml_path, user_config); + config.tool_suggest = + resolve_tool_suggest_config_from_layer_stack(&config.config_layer_stack); + let config = Arc::new(config); + state.session_configuration.original_config_do_not_use = Arc::clone(&config); + config + }; self.services.skills_manager.clear_cache(); self.services.plugins_manager.clear_cache(); + let hooks = build_hooks_for_config( + config.as_ref(), + self.services.plugins_manager.as_ref(), + self.services.user_shell.as_ref(), + ) + .await; + + let state = self.state.lock().await; + // A newer reload may have updated the config while this hook build was in flight. + // Only publish hooks derived from the current config snapshot. + if Arc::ptr_eq( + &state.session_configuration.original_config_do_not_use, + &config, + ) { + self.services.hooks.store(Arc::new(hooks)); + } } async fn build_settings_update_items( @@ -3201,8 +3224,8 @@ impl Session { } } - pub(crate) fn hooks(&self) -> &Hooks { - &self.services.hooks + pub(crate) fn hooks(&self) -> Arc { + self.services.hooks.load_full() } pub(crate) fn user_shell(&self) -> Arc { @@ -3328,5 +3351,35 @@ fn errors_to_info(errors: &[SkillError]) -> Vec { use codex_memories_read::build_memory_tool_developer_instructions; +/// Builds the hook engine for one config snapshot, including any enabled plugin hooks. +async fn build_hooks_for_config( + config: &Config, + plugins_manager: &PluginsManager, + user_shell: &crate::shell::Shell, +) -> Hooks { + let mut hook_shell_argv = user_shell.derive_exec_args("", /*use_login_shell*/ false); + 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_outcome = plugins_manager.plugins_for_config(config).await; + ( + plugin_outcome.effective_plugin_hook_sources(), + plugin_outcome.effective_plugin_hook_warnings(), + ) + } else { + (Vec::new(), Vec::new()) + }; + 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, + }) +} + #[cfg(test)] pub(crate) mod tests; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index f4e8c2a98..6c6cc22a0 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -765,29 +765,8 @@ impl Session { (None, None) }; - let mut hook_shell_argv = - default_shell.derive_exec_args("", /*use_login_shell*/ false); - 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_outcome = plugins_manager.plugins_for_config(&config).await; - ( - plugin_outcome.effective_plugin_hook_sources(), - plugin_outcome.effective_plugin_hook_warnings(), - ) - } else { - (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, - }); + let hooks = + build_hooks_for_config(&config, plugins_manager.as_ref(), &default_shell).await; for warning in hooks.startup_warnings() { post_session_configured_events.push(Event { id: INITIAL_SUBMIT_ID.to_owned(), @@ -824,7 +803,7 @@ impl Session { shell_zsh_path: config.zsh_path.clone(), main_execve_wrapper_exe: config.main_execve_wrapper_exe.clone(), analytics_events_client, - hooks, + hooks: arc_swap::ArcSwap::from_pointee(hooks), rollout_thread_trace, user_shell: Arc::new(default_shell), shell_snapshot_tx, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 8286c7ce0..1afb28f35 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1156,6 +1156,43 @@ async fn reload_user_config_layer_updates_effective_apps_config() { assert_eq!(app.destructive_enabled, Some(false)); } +#[tokio::test] +async fn reload_user_config_layer_refreshes_hooks() -> anyhow::Result<()> { + let session = make_session_with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("enable Codex hooks"); + }) + .await?; + let codex_home = session.codex_home().await; + std::fs::create_dir_all(&codex_home)?; + std::fs::write( + codex_home.join(CONFIG_TOML_FILE), + r#" +[hooks] + +[[hooks.SessionStart]] +hooks = [{ type = "command", command = "python3 /tmp/user.py" }] +"#, + )?; + + let request = codex_hooks::SessionStartRequest { + session_id: session.conversation_id, + cwd: session.get_config().await.cwd.clone(), + transcript_path: None, + model: "gpt-5.2".to_string(), + permission_mode: "default".to_string(), + source: codex_hooks::SessionStartSource::Startup, + }; + assert!(session.hooks().preview_session_start(&request).is_empty()); + + session.reload_user_config_layer().await; + + assert_eq!(session.hooks().preview_session_start(&request).len(), 1); + Ok(()) +} + #[tokio::test] async fn reload_user_config_layer_updates_effective_tool_suggest_config() { let (session, _turn_context) = make_session_and_context().await; @@ -3476,10 +3513,10 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { config.chatgpt_base_url.trim_end_matches('/').to_string(), config.analytics_enabled, ), - hooks: Hooks::new(HooksConfig { + hooks: arc_swap::ArcSwap::from_pointee(Hooks::new(HooksConfig { legacy_notify_argv: config.notify.clone(), ..HooksConfig::default() - }), + })), rollout_thread_trace: codex_rollout_trace::ThreadTraceContext::disabled(), user_shell: Arc::new(default_user_shell()), shell_snapshot_tx: watch::channel(None).0, @@ -4905,10 +4942,10 @@ where config.chatgpt_base_url.trim_end_matches('/').to_string(), config.analytics_enabled, ), - hooks: Hooks::new(HooksConfig { + hooks: arc_swap::ArcSwap::from_pointee(Hooks::new(HooksConfig { legacy_notify_argv: config.notify.clone(), ..HooksConfig::default() - }), + })), rollout_thread_trace: codex_rollout_trace::ThreadTraceContext::disabled(), user_shell: Arc::new(default_user_shell()), shell_snapshot_tx: watch::channel(None).0, diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 823dcbfc3..07e4e3004 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -520,7 +520,8 @@ pub(crate) async fn run_turn( stop_hook_active, last_assistant_message: last_agent_message.clone(), }; - for run in sess.hooks().preview_stop(&stop_request) { + let hooks = sess.hooks(); + for run in hooks.preview_stop(&stop_request) { sess.send_event( &turn_context, EventMsg::HookStarted(codex_protocol::protocol::HookStartedEvent { @@ -530,7 +531,7 @@ pub(crate) async fn run_turn( ) .await; } - let stop_outcome = sess.hooks().run_stop(stop_request).await; + let stop_outcome = hooks.run_stop(stop_request).await; emit_hook_completed_events(&sess, &turn_context, stop_outcome.hook_events) .await; if stop_outcome.should_block { diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index fe27d89ae..ddf212217 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -15,6 +15,7 @@ use crate::tools::code_mode::CodeModeService; use crate::tools::network_approval::NetworkApprovalService; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecProcessManager; +use arc_swap::ArcSwap; use codex_analytics::AnalyticsEventsClient; use codex_exec_server::EnvironmentManager; use codex_hooks::Hooks; @@ -42,7 +43,7 @@ pub(crate) struct SessionServices { #[cfg_attr(not(unix), allow(dead_code))] pub(crate) main_execve_wrapper_exe: Option, pub(crate) analytics_events_client: AnalyticsEventsClient, - pub(crate) hooks: Hooks, + pub(crate) hooks: ArcSwap, pub(crate) rollout_thread_trace: ThreadTraceContext, pub(crate) user_shell: Arc, pub(crate) shell_snapshot_tx: watch::Sender>>, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 2ec5ede4a..84e469e22 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -38,6 +38,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::Value; use std::path::PathBuf; +use std::sync::Arc; use std::time::Duration; use tokio::sync::RwLock; @@ -324,7 +325,7 @@ fn shell_request_escalation_execution_is_explicit() { #[tokio::test(flavor = "current_thread")] async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Result<()> { - let (mut session, mut turn_context) = make_session_and_context().await; + let (session, mut turn_context) = make_session_and_context().await; std::fs::create_dir_all(&turn_context.config.codex_home) .context("recreate codex home for hook fixtures")?; let script_path = turn_context @@ -376,13 +377,16 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul .derive_exec_args("", /*use_login_shell*/ false); let hook_shell_program = hook_shell_argv.remove(0); let _ = hook_shell_argv.pop(); - session.services.hooks = Hooks::new(HooksConfig { - feature_enabled: true, - config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), - shell_program: Some(hook_shell_program), - shell_args: hook_shell_argv, - ..HooksConfig::default() - }); + session + .services + .hooks + .store(Arc::new(Hooks::new(HooksConfig { + feature_enabled: true, + config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + shell_program: Some(hook_shell_program), + shell_args: hook_shell_argv, + ..HooksConfig::default() + }))); turn_context.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); turn_context.permission_profile = PermissionProfile::from_runtime_permissions( diff --git a/codex-rs/hooks/src/config_rules.rs b/codex-rs/hooks/src/config_rules.rs new file mode 100644 index 000000000..b9fa87150 --- /dev/null +++ b/codex-rs/hooks/src/config_rules.rs @@ -0,0 +1,188 @@ +use std::collections::HashSet; + +use codex_config::ConfigLayerSource; +use codex_config::ConfigLayerStack; +use codex_config::ConfigLayerStackOrdering; +use codex_config::HookStateToml; +use codex_config::TomlValue; + +/// Build hook enablement rules from config layers that are allowed to override +/// user preferences. +/// +/// This intentionally reads only user and session flag layers, including +/// disabled layers, to match the skills config behavior. Project, managed, and +/// plugin layers can discover hooks, but they do not get to write user +/// enablement state. +pub(crate) fn disabled_hook_keys_from_stack( + config_layer_stack: Option<&ConfigLayerStack>, +) -> HashSet { + let Some(config_layer_stack) = config_layer_stack else { + return HashSet::new(); + }; + + let mut disabled_keys = HashSet::new(); + for layer in config_layer_stack.get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ true, + ) { + if !matches!( + layer.name, + ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags + ) { + continue; + } + + let Some(state_value) = layer + .config + .get("hooks") + .and_then(|hooks| hooks.get("state")) + else { + continue; + }; + let TomlValue::Table(state_by_key) = state_value else { + continue; + }; + + for (key, state_value) in state_by_key { + let state: HookStateToml = match state_value.clone().try_into() { + Ok(state) => state, + Err(_) => { + continue; + } + }; + let key = key.trim(); + if key.is_empty() { + continue; + } + // Later layers win. Hooks without an explicit enabled override can + // still carry future per-hook state without changing enablement. + match state.enabled { + Some(false) => { + disabled_keys.insert(key.to_string()); + } + Some(true) => { + disabled_keys.remove(key); + } + None => {} + } + } + } + + disabled_keys +} + +#[cfg(test)] +mod tests { + use codex_config::ConfigLayerEntry; + use codex_config::TomlValue; + use codex_utils_absolute_path::test_support::PathBufExt; + use codex_utils_absolute_path::test_support::test_path_buf; + use pretty_assertions::assert_eq; + + use super::*; + + #[test] + fn disabled_hook_keys_from_stack_respects_layer_precedence() { + let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; + let stack = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: test_path_buf("/tmp/config.toml").abs(), + }, + config_with_hook_override(key, Some(/*enabled*/ false)), + ), + ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + config_with_hook_override(key, Some(/*enabled*/ true)), + ), + ], + Default::default(), + Default::default(), + ) + .expect("config layer stack"); + + assert_eq!(disabled_hook_keys_from_stack(Some(&stack)), HashSet::new()); + } + + #[test] + fn disabled_hook_keys_from_stack_ignores_malformed_hook_events() { + let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; + let config: TomlValue = serde_json::from_value(serde_json::json!({ + "hooks": { + "state": { + (key): { + "enabled": false, + }, + }, + "SessionStart": "not a matcher list", + }, + })) + .expect("config TOML should deserialize"); + let stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: test_path_buf("/tmp/config.toml").abs(), + }, + config, + )], + Default::default(), + Default::default(), + ) + .expect("config layer stack"); + + assert_eq!( + disabled_hook_keys_from_stack(Some(&stack)), + HashSet::from([key.to_string()]) + ); + } + + #[test] + fn disabled_hook_keys_from_stack_ignores_malformed_state_entries() { + let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; + let config: TomlValue = serde_json::from_value(serde_json::json!({ + "hooks": { + "state": { + (key): { + "enabled": false, + }, + "malformed": { + "enabled": "not a bool", + }, + }, + }, + })) + .expect("config TOML should deserialize"); + let stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: test_path_buf("/tmp/config.toml").abs(), + }, + config, + )], + Default::default(), + Default::default(), + ) + .expect("config layer stack"); + + assert_eq!( + disabled_hook_keys_from_stack(Some(&stack)), + HashSet::from([key.to_string()]) + ); + } + + fn config_with_hook_override(key: &str, enabled: Option) -> TomlValue { + let hook_state = match enabled { + Some(enabled) => serde_json::json!({ "enabled": enabled }), + None => serde_json::json!({}), + }; + serde_json::from_value(serde_json::json!({ + "hooks": { + "state": { + (key): hook_state, + }, + }, + })) + .expect("config TOML should deserialize") + } +} diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 0da34f0e2..8c520b749 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -16,9 +16,11 @@ use codex_plugin::PluginHookSource; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use std::collections::HashMap; +use std::collections::HashSet; use super::ConfiguredHandler; use super::HookListEntry; +use crate::config_rules::disabled_hook_keys_from_stack; use crate::events::common::matcher_pattern_for_event; use crate::events::common::validate_matcher_pattern; use codex_protocol::protocol::HookHandlerType; @@ -30,11 +32,11 @@ pub(crate) struct DiscoveryResult { pub warnings: Vec, } -#[derive(Clone)] struct HookHandlerSource<'a> { path: &'a AbsolutePathBuf, - is_managed: bool, + key_source: String, source: HookSource, + disabled_hook_keys: &'a HashSet, env: HashMap, plugin_id: Option, } @@ -48,6 +50,7 @@ pub(crate) fn discover_handlers( let mut hook_entries = Vec::new(); let mut warnings = plugin_hook_load_warnings; let mut display_order = 0_i64; + let disabled_hook_keys = disabled_hook_keys_from_stack(config_layer_stack); if let Some(config_layer_stack) = config_layer_stack { append_managed_requirement_handlers( @@ -56,6 +59,7 @@ pub(crate) fn discover_handlers( &mut warnings, &mut display_order, config_layer_stack, + &disabled_hook_keys, ); for layer in config_layer_stack.get_layers( @@ -86,8 +90,9 @@ pub(crate) fn discover_handlers( &mut display_order, HookHandlerSource { path: &source_path, - is_managed: false, + key_source: source_path.display().to_string(), source: hook_source, + disabled_hook_keys: &disabled_hook_keys, env: HashMap::new(), plugin_id: None, }, @@ -103,6 +108,7 @@ pub(crate) fn discover_handlers( &mut warnings, &mut display_order, plugin_hook_sources, + &disabled_hook_keys, ); DiscoveryResult { @@ -118,6 +124,7 @@ fn append_managed_requirement_handlers( warnings: &mut Vec, display_order: &mut i64, config_layer_stack: &ConfigLayerStack, + disabled_hook_keys: &HashSet, ) { let Some(managed_hooks) = config_layer_stack.requirements().managed_hooks.as_ref() else { return; @@ -134,8 +141,9 @@ fn append_managed_requirement_handlers( display_order, HookHandlerSource { path: &source_path, - is_managed: true, + key_source: source_path.display().to_string(), source: hook_source_for_requirement_source(managed_hooks.source.as_ref()), + disabled_hook_keys, env: HashMap::new(), plugin_id: None, }, @@ -149,6 +157,7 @@ fn append_plugin_hook_sources( warnings: &mut Vec, display_order: &mut i64, plugin_hook_sources: Vec, + disabled_hook_keys: &HashSet, ) { // TODO(abhinav): check enabled/trusted state here before plugin hooks become runnable. for source in plugin_hook_sources { @@ -157,8 +166,8 @@ fn append_plugin_hook_sources( plugin_id, plugin_data_root, source_path, + source_relative_path, hooks, - .. } = source; let mut env = HashMap::new(); let plugin_root_value = plugin_root.display().to_string(); @@ -177,8 +186,9 @@ fn append_plugin_hook_sources( display_order, HookHandlerSource { path: &source_path, - is_managed: false, + key_source: format!("{plugin_id}:{source_relative_path}"), source: HookSource::Plugin, + disabled_hook_keys, env, plugin_id: Some(plugin_id), }, @@ -337,7 +347,7 @@ fn append_hook_events( hook_entries, warnings, display_order, - source.clone(), + &source, event_name, groups, ); @@ -349,11 +359,11 @@ fn append_matcher_groups( hook_entries: &mut Vec, warnings: &mut Vec, display_order: &mut i64, - source: HookHandlerSource<'_>, + source: &HookHandlerSource<'_>, event_name: codex_protocol::protocol::HookEventName, groups: Vec, ) { - for group in groups { + for (group_index, group) in groups.into_iter().enumerate() { let matcher = matcher_pattern_for_event(event_name, group.matcher.as_deref()); if let Some(matcher) = matcher && let Err(err) = validate_matcher_pattern(matcher) @@ -364,8 +374,7 @@ fn append_matcher_groups( )); continue; } - - for handler in group.hooks { + for (handler_index, handler) in group.hooks.into_iter().enumerate() { match handler { HookHandlerConfig::Command { command, @@ -391,7 +400,18 @@ fn append_matcher_groups( command.replace(&format!("${{{key}}}"), value) }); let timeout_sec = timeout_sec.unwrap_or(600).max(1); + // TODO(abhinav): replace this positional suffix with a durable hook id. + let key = format!( + "{}:{}:{}:{}", + source.key_source, + hook_event_key_label(event_name), + group_index, + handler_index + ); + let enabled = + source.source.is_managed() || !source.disabled_hook_keys.contains(&key); hook_entries.push(HookListEntry { + key, event_name, handler_type: HookHandlerType::Command, matcher: matcher.map(ToOwned::to_owned), @@ -402,19 +422,22 @@ fn append_matcher_groups( source: source.source, plugin_id: source.plugin_id.clone(), display_order: *display_order, + enabled, + is_managed: source.source.is_managed(), }); - handlers.push(ConfiguredHandler { - event_name, - is_managed: source.is_managed, - matcher: matcher.map(ToOwned::to_owned), - command, - timeout_sec, - status_message, - source_path: source.path.clone(), - source: source.source, - display_order: *display_order, - env: source.env.clone(), - }); + if enabled { + handlers.push(ConfiguredHandler { + event_name, + matcher: matcher.map(ToOwned::to_owned), + command, + timeout_sec, + status_message, + source_path: source.path.clone(), + source: source.source, + display_order: *display_order, + env: source.env.clone(), + }); + } *display_order += 1; } HookHandlerConfig::Prompt {} => warnings.push(format!( @@ -430,6 +453,17 @@ fn append_matcher_groups( } } +fn hook_event_key_label(event_name: codex_protocol::protocol::HookEventName) -> &'static str { + match event_name { + codex_protocol::protocol::HookEventName::PreToolUse => "pre_tool_use", + codex_protocol::protocol::HookEventName::PermissionRequest => "permission_request", + codex_protocol::protocol::HookEventName::PostToolUse => "post_tool_use", + codex_protocol::protocol::HookEventName::SessionStart => "session_start", + codex_protocol::protocol::HookEventName::UserPromptSubmit => "user_prompt_submit", + codex_protocol::protocol::HookEventName::Stop => "stop", + } +} + fn hook_source_for_config_layer_source(source: &ConfigLayerSource) -> HookSource { match source { ConfigLayerSource::System { .. } => HookSource::System, @@ -454,15 +488,16 @@ fn hook_source_for_requirement_source(source: Option<&RequirementSource>) -> Hoo Some(RequirementSource::LegacyManagedConfigTomlFromMdm) => { HookSource::LegacyManagedConfigMdm } - Some(RequirementSource::CloudRequirements | RequirementSource::Unknown) | None => { - HookSource::Unknown - } + Some(RequirementSource::CloudRequirements) => HookSource::CloudRequirements, + Some(RequirementSource::Unknown) | None => HookSource::Unknown, } } #[cfg(test)] mod tests { + use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; + use codex_config::HookEventsToml; use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookSource; use codex_utils_absolute_path::AbsolutePathBuf; @@ -474,6 +509,7 @@ mod tests { use super::append_matcher_groups; use codex_config::HookHandlerConfig; use codex_config::MatcherGroup; + use codex_config::TomlValue; fn source_path() -> AbsolutePathBuf { test_path_buf("/tmp/hooks.json").abs() @@ -483,11 +519,15 @@ mod tests { HookSource::User } - fn hook_handler_source(path: &AbsolutePathBuf) -> super::HookHandlerSource<'_> { + fn hook_handler_source<'a>( + path: &'a AbsolutePathBuf, + disabled_hook_keys: &'a std::collections::HashSet, + ) -> super::HookHandlerSource<'a> { super::HookHandlerSource { path, - is_managed: false, + key_source: path.display().to_string(), source: hook_source(), + disabled_hook_keys, env: std::collections::HashMap::new(), plugin_id: None, } @@ -511,13 +551,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); + let disabled_hook_keys = std::collections::HashSet::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - hook_handler_source(&source_path), + &hook_handler_source(&source_path, &disabled_hook_keys), HookEventName::UserPromptSubmit, vec![command_group(Some("["))], ); @@ -527,7 +568,6 @@ mod tests { handlers, vec![ConfiguredHandler { event_name: HookEventName::UserPromptSubmit, - is_managed: false, matcher: None, command: "echo hello".to_string(), timeout_sec: 600, @@ -546,13 +586,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); + let disabled_hook_keys = std::collections::HashSet::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - hook_handler_source(&source_path), + &hook_handler_source(&source_path, &disabled_hook_keys), HookEventName::PreToolUse, vec![command_group(Some("^Bash$"))], ); @@ -562,7 +603,6 @@ mod tests { handlers, vec![ConfiguredHandler { event_name: HookEventName::PreToolUse, - is_managed: false, matcher: Some("^Bash$".to_string()), command: "echo hello".to_string(), timeout_sec: 600, @@ -581,13 +621,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); + let disabled_hook_keys = std::collections::HashSet::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - hook_handler_source(&source_path), + &hook_handler_source(&source_path, &disabled_hook_keys), HookEventName::PreToolUse, vec![command_group(Some("*"))], ); @@ -603,13 +644,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); + let disabled_hook_keys = std::collections::HashSet::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - hook_handler_source(&source_path), + &hook_handler_source(&source_path, &disabled_hook_keys), HookEventName::PostToolUse, vec![command_group(Some("Edit|Write"))], ); @@ -620,6 +662,56 @@ mod tests { assert_eq!(handlers[0].matcher.as_deref(), Some("Edit|Write")); } + #[test] + fn toml_hook_discovery_ignores_malformed_state_entries() { + let layer = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: test_path_buf("/tmp/config.toml").abs(), + }, + config_with_malformed_state_and_session_start_hook(), + ); + let mut warnings = Vec::new(); + + let (_, hooks) = super::load_toml_hooks_from_layer(&layer, &mut warnings) + .expect("valid hook events should still load"); + + assert_eq!(warnings, Vec::::new()); + assert_eq!( + hooks, + HookEventsToml { + session_start: vec![MatcherGroup { + matcher: None, + hooks: vec![HookHandlerConfig::Command { + command: "echo hello".to_string(), + timeout_sec: None, + r#async: false, + status_message: None, + }], + }], + ..Default::default() + } + ); + } + + fn config_with_malformed_state_and_session_start_hook() -> TomlValue { + serde_json::from_value(serde_json::json!({ + "hooks": { + "state": { + "some_key": { + "enabled": "not a bool", + }, + }, + "SessionStart": [{ + "hooks": [{ + "type": "command", + "command": "echo hello", + }], + }], + }, + })) + .expect("config TOML should deserialize") + } + #[test] fn hook_source_for_config_layer_source_discards_source_details() { let config_file = test_path_buf("/tmp/.codex/config.toml").abs(); diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index c19b31184..c44b1fe69 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -156,7 +156,6 @@ mod tests { ) -> ConfiguredHandler { ConfiguredHandler { event_name, - is_managed: false, matcher: matcher.map(str::to_owned), command: command.to_string(), timeout_sec: 5, diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 83482d500..c06c9fabd 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -36,7 +36,6 @@ pub(crate) struct CommandShell { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct ConfiguredHandler { pub event_name: codex_protocol::protocol::HookEventName, - pub is_managed: bool, pub matcher: Option, pub command: String, pub timeout_sec: u64, @@ -71,6 +70,7 @@ impl ConfiguredHandler { #[derive(Debug, Clone, PartialEq, Eq)] pub struct HookListEntry { + pub key: String, pub event_name: HookEventName, pub handler_type: HookHandlerType, pub matcher: Option, @@ -81,6 +81,8 @@ pub struct HookListEntry { pub source: HookSource, pub plugin_id: Option, pub display_order: i64, + pub enabled: bool, + pub is_managed: bool, } #[derive(Clone)] diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 886ed2255..be9fdad7b 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -121,7 +121,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: assert!(engine.warnings().is_empty()); assert_eq!(engine.handlers.len(), 1); - assert!(engine.handlers[0].is_managed); + assert!(engine.handlers[0].source.is_managed()); let cwd = cwd(); let preview = engine.preview_pre_tool_use(&PreToolUseRequest { session_id: ThreadId::new(), @@ -158,6 +158,177 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\"")); } +#[test] +fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() { + let temp = tempdir().expect("create temp dir"); + let managed_dir = + AbsolutePathBuf::try_from(temp.path().join("managed-hooks")).expect("absolute path"); + fs::create_dir_all(managed_dir.as_path()).expect("create managed hooks dir"); + let managed_hooks = managed_hooks_for_current_platform( + managed_dir.clone(), + HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "python3 /tmp/managed.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + ); + let config_path = + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute path"); + let managed_disabled_key = format!("{}:pre_tool_use:0:0", managed_dir.display()); + let user_disabled_key = format!("{}:pre_tool_use:0:0", config_path.display()); + let user_config = config_with_pre_tool_use_hook_and_states( + "python3 /tmp/user.py", + [&managed_disabled_key, &user_disabled_key], + ); + let config_layer_stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { file: config_path }, + user_config, + )], + ConfigRequirements { + managed_hooks: Some(ConstrainedWithSource::new( + Constrained::allow_any(managed_hooks.clone()), + Some(RequirementSource::CloudRequirements), + )), + ..ConfigRequirements::default() + }, + ConfigRequirementsToml { + hooks: Some(managed_hooks), + ..ConfigRequirementsToml::default() + }, + ) + .expect("config layer stack"); + + let engine = ClaudeHooksEngine::new( + /*enabled*/ true, + Some(&config_layer_stack), + Vec::new(), + Vec::new(), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert_eq!(engine.handlers.len(), 1); + assert!(engine.handlers[0].source.is_managed()); + let discovered = + super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new()); + assert_eq!(discovered.hook_entries.len(), 2); + assert_eq!(discovered.hook_entries[0].key, managed_disabled_key); + assert_eq!(discovered.hook_entries[0].enabled, true); + assert!(discovered.hook_entries[0].is_managed); + assert_eq!(discovered.hook_entries[1].key, user_disabled_key); + assert_eq!(discovered.hook_entries[1].enabled, false); + assert!(!discovered.hook_entries[1].is_managed); +} + +#[test] +fn user_disablement_does_not_filter_managed_layer_hooks() { + let temp = tempdir().expect("create temp dir"); + let managed_config_path = + AbsolutePathBuf::try_from(temp.path().join("managed_config.toml")).expect("absolute path"); + let user_config_path = + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute path"); + let managed_key = format!("{}:pre_tool_use:0:0", managed_config_path.display()); + + let config_layer_stack = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_config_path, + }, + config_with_hook_state(&managed_key, /*enabled*/ false), + ), + ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: managed_config_path, + }, + config_with_pre_tool_use_hook("python3 /tmp/managed-layer.py"), + ), + ], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack"); + + let engine = ClaudeHooksEngine::new( + /*enabled*/ true, + Some(&config_layer_stack), + Vec::new(), + Vec::new(), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert_eq!(engine.handlers.len(), 1); + assert!(engine.handlers[0].source.is_managed()); + let discovered = + super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new()); + assert_eq!(discovered.hook_entries.len(), 1); + assert_eq!(discovered.hook_entries[0].key, managed_key); + assert_eq!(discovered.hook_entries[0].enabled, true); + assert!(discovered.hook_entries[0].is_managed); +} + +fn config_with_hook_state(key: &str, enabled: bool) -> TomlValue { + serde_json::from_value(serde_json::json!({ + "hooks": { + "state": { + (key): { + "enabled": enabled, + }, + }, + }, + })) + .expect("config TOML should deserialize") +} + +fn config_with_pre_tool_use_hook_and_states( + command: &str, + disabled_keys: [&str; N], +) -> TomlValue { + let state = disabled_keys + .into_iter() + .map(|key| (key.to_string(), serde_json::json!({ "enabled": false }))) + .collect::>(); + serde_json::from_value(serde_json::json!({ + "hooks": { + "state": state, + "PreToolUse": [{ + "hooks": [{ + "type": "command", + "command": command, + }], + }], + }, + })) + .expect("config TOML should deserialize") +} + +fn config_with_pre_tool_use_hook(command: &str) -> TomlValue { + serde_json::from_value(serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "hooks": [{ + "type": "command", + "command": command, + }], + }], + }, + })) + .expect("config TOML should deserialize") +} + #[test] fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { let temp = tempdir().expect("create temp dir"); @@ -333,7 +504,12 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { tool_input: serde_json::json!({ "command": "echo hello" }), }); assert_eq!(preview.len(), 2); - assert!(engine.handlers.iter().all(|handler| !handler.is_managed)); + assert!( + engine + .handlers + .iter() + .all(|handler| !handler.source.is_managed()) + ); assert_eq!(preview[0].source_path, hooks_json_path); assert_eq!(preview[1].source_path, config_path); } diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index c01cebf78..63045ef42 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -543,7 +543,6 @@ mod tests { fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::PostToolUse, - is_managed: false, matcher: Some("^Bash$".to_string()), command: "python3 post_tool_use_hook.py".to_string(), timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 3b20c2c2c..6fe155522 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -534,7 +534,6 @@ mod tests { fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::PreToolUse, - is_managed: false, matcher: Some("^Bash$".to_string()), command: "echo hook".to_string(), timeout_sec: 5, diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index 54c7f5173..f064f67b2 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -356,7 +356,6 @@ mod tests { fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::SessionStart, - is_managed: false, matcher: None, command: "echo hook".to_string(), timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/stop.rs b/codex-rs/hooks/src/events/stop.rs index 392f15eee..8fc176a47 100644 --- a/codex-rs/hooks/src/events/stop.rs +++ b/codex-rs/hooks/src/events/stop.rs @@ -523,7 +523,6 @@ mod tests { fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::Stop, - is_managed: false, matcher: None, command: "echo hook".to_string(), timeout_sec: 600, diff --git a/codex-rs/hooks/src/events/user_prompt_submit.rs b/codex-rs/hooks/src/events/user_prompt_submit.rs index 8aaf3ad60..a04711eb4 100644 --- a/codex-rs/hooks/src/events/user_prompt_submit.rs +++ b/codex-rs/hooks/src/events/user_prompt_submit.rs @@ -414,7 +414,6 @@ mod tests { fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::UserPromptSubmit, - is_managed: false, matcher: None, command: "echo hook".to_string(), timeout_sec: 5, diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 1b10a3389..4e16969a5 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -1,3 +1,4 @@ +mod config_rules; mod engine; pub(crate) mod events; mod legacy_notify; diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 6cdce3404..fa7a9aa6a 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1571,12 +1571,28 @@ pub enum HookSource { Mdm, SessionFlags, Plugin, + CloudRequirements, LegacyManagedConfigFile, LegacyManagedConfigMdm, #[default] Unknown, } +impl HookSource { + /// Returns whether hooks from this source are managed and therefore not + /// user-configurable. + pub fn is_managed(self) -> bool { + matches!( + self, + Self::System + | Self::Mdm + | Self::CloudRequirements + | Self::LegacyManagedConfigFile + | Self::LegacyManagedConfigMdm + ) + } +} + #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub enum HookRunStatus { @@ -3991,6 +4007,20 @@ mod tests { use tempfile::NamedTempFile; use tempfile::TempDir; + #[test] + fn hook_source_managedness_is_source_derived() { + assert_eq!(HookSource::System.is_managed(), true); + assert_eq!(HookSource::Mdm.is_managed(), true); + assert_eq!(HookSource::CloudRequirements.is_managed(), true); + assert_eq!(HookSource::LegacyManagedConfigFile.is_managed(), true); + assert_eq!(HookSource::LegacyManagedConfigMdm.is_managed(), true); + assert_eq!(HookSource::User.is_managed(), false); + assert_eq!(HookSource::Project.is_managed(), false); + assert_eq!(HookSource::SessionFlags.is_managed(), false); + assert_eq!(HookSource::Plugin.is_managed(), false); + assert_eq!(HookSource::Unknown.is_managed(), false); + } + fn sorted_writable_roots(roots: Vec) -> Vec<(PathBuf, Vec)> { let mut sorted_roots: Vec<(PathBuf, Vec)> = roots .into_iter()