From 24c0dcc4b3ebcb470fde9c4390d5497e9ddc2edc Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Sun, 26 Apr 2026 20:53:35 -0700 Subject: [PATCH] Refactor hook config selectors --- .../schema/json/ClientRequest.json | 114 +++++--- .../codex_app_server_protocol.schemas.json | 114 +++++--- .../codex_app_server_protocol.v2.schemas.json | 114 +++++--- .../json/v2/HooksConfigWriteParams.json | 118 +++++--- .../schema/typescript/v2/HookConfigSource.ts | 5 - .../typescript/v2/HooksConfigWriteParams.ts | 3 +- .../schema/typescript/v2/index.ts | 1 - .../app-server-protocol/src/protocol/v2.rs | 79 ++++-- .../app-server/src/codex_message_processor.rs | 95 +++---- .../app-server/tests/suite/v2/config_rpc.rs | 3 +- codex-rs/config/src/hook_config.rs | 7 + codex-rs/config/src/lib.rs | 1 + codex-rs/core/src/config/edit.rs | 257 ++++++------------ codex-rs/core/src/config/edit_tests.rs | 6 +- codex-rs/hooks/src/engine/config_rules.rs | 214 +++++++++------ 15 files changed, 637 insertions(+), 494 deletions(-) delete mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/HookConfigSource.ts diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 64cfac9391..01131e4db9 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -1447,44 +1447,90 @@ ], "type": "object" }, - "HookConfigSource": { - "enum": [ - "plugin", - "user", - "project" - ], - "type": "string" - }, "HooksConfigWriteParams": { - "properties": { - "enabled": { - "type": "boolean" + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "pluginId": { + "type": "string" + }, + "source": { + "enum": [ + "plugin" + ], + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "pluginId", + "source" + ], + "type": "object" }, - "key": { - "type": "string" + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "user" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" }, - "pluginId": { - "type": [ - "string", - "null" - ] - }, - "source": { - "$ref": "#/definitions/HookConfigSource" - }, - "sourcePath": { - "type": [ - "string", - "null" - ] + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "project" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" } - }, - "required": [ - "enabled", - "key", - "source" - ], - "type": "object" + ] }, "HooksListParams": { "properties": { 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 11373e93e4..4f3ac41886 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 @@ -9615,14 +9615,6 @@ "title": "HookCompletedNotification", "type": "object" }, - "HookConfigSource": { - "enum": [ - "plugin", - "user", - "project" - ], - "type": "string" - }, "HookErrorInfo": { "properties": { "message": { @@ -9909,36 +9901,90 @@ }, "HooksConfigWriteParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "properties": { - "enabled": { - "type": "boolean" + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "pluginId": { + "type": "string" + }, + "source": { + "enum": [ + "plugin" + ], + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "pluginId", + "source" + ], + "type": "object" }, - "key": { - "type": "string" + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "user" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" }, - "pluginId": { - "type": [ - "string", - "null" - ] - }, - "source": { - "$ref": "#/definitions/v2/HookConfigSource" - }, - "sourcePath": { - "type": [ - "string", - "null" - ] + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "project" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" } - }, - "required": [ - "enabled", - "key", - "source" ], - "title": "HooksConfigWriteParams", - "type": "object" + "title": "HooksConfigWriteParams" }, "HooksConfigWriteResponse": { "$schema": "http://json-schema.org/draft-07/schema#", 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 d61263d34c..a55a4f93b5 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 @@ -6245,14 +6245,6 @@ "title": "HookCompletedNotification", "type": "object" }, - "HookConfigSource": { - "enum": [ - "plugin", - "user", - "project" - ], - "type": "string" - }, "HookErrorInfo": { "properties": { "message": { @@ -6539,36 +6531,90 @@ }, "HooksConfigWriteParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "properties": { - "enabled": { - "type": "boolean" + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "pluginId": { + "type": "string" + }, + "source": { + "enum": [ + "plugin" + ], + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "pluginId", + "source" + ], + "type": "object" }, - "key": { - "type": "string" + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "user" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" }, - "pluginId": { - "type": [ - "string", - "null" - ] - }, - "source": { - "$ref": "#/definitions/HookConfigSource" - }, - "sourcePath": { - "type": [ - "string", - "null" - ] + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "project" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" } - }, - "required": [ - "enabled", - "key", - "source" ], - "title": "HooksConfigWriteParams", - "type": "object" + "title": "HooksConfigWriteParams" }, "HooksConfigWriteResponse": { "$schema": "http://json-schema.org/draft-07/schema#", diff --git a/codex-rs/app-server-protocol/schema/json/v2/HooksConfigWriteParams.json b/codex-rs/app-server-protocol/schema/json/v2/HooksConfigWriteParams.json index 17f3587a54..7ae34e32b2 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HooksConfigWriteParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HooksConfigWriteParams.json @@ -1,43 +1,87 @@ { "$schema": "http://json-schema.org/draft-07/schema#", - "definitions": { - "HookConfigSource": { - "enum": [ - "plugin", - "user", - "project" + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "pluginId": { + "type": "string" + }, + "source": { + "enum": [ + "plugin" + ], + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "pluginId", + "source" ], - "type": "string" + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "user" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean" + }, + "key": { + "type": "string" + }, + "source": { + "enum": [ + "project" + ], + "type": "string" + }, + "sourcePath": { + "type": "string" + } + }, + "required": [ + "enabled", + "key", + "source", + "sourcePath" + ], + "type": "object" } - }, - "properties": { - "enabled": { - "type": "boolean" - }, - "key": { - "type": "string" - }, - "pluginId": { - "type": [ - "string", - "null" - ] - }, - "source": { - "$ref": "#/definitions/HookConfigSource" - }, - "sourcePath": { - "type": [ - "string", - "null" - ] - } - }, - "required": [ - "enabled", - "key", - "source" ], - "title": "HooksConfigWriteParams", - "type": "object" + "title": "HooksConfigWriteParams" } \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookConfigSource.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookConfigSource.ts deleted file mode 100644 index ff688a18da..0000000000 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HookConfigSource.ts +++ /dev/null @@ -1,5 +0,0 @@ -// GENERATED CODE! DO NOT MODIFY BY HAND! - -// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. - -export type HookConfigSource = "plugin" | "user" | "project"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HooksConfigWriteParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HooksConfigWriteParams.ts index 8485c4263f..bdd396bf85 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HooksConfigWriteParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HooksConfigWriteParams.ts @@ -1,6 +1,5 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -import type { HookConfigSource } from "./HookConfigSource"; -export type HooksConfigWriteParams = { source: HookConfigSource, pluginId?: string | null, sourcePath?: string | null, key: string, enabled: boolean, }; +export type HooksConfigWriteParams = { "source": "plugin", pluginId: string, key: string, enabled: boolean, } | { "source": "user", sourcePath: string, key: string, enabled: boolean, } | { "source": "project", sourcePath: string, key: string, enabled: boolean, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index ec8523cb64..fcc9881dfb 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -151,7 +151,6 @@ export type { GuardianRiskLevel } from "./GuardianRiskLevel"; export type { GuardianUserAuthorization } from "./GuardianUserAuthorization"; export type { GuardianWarningNotification } from "./GuardianWarningNotification"; export type { HookCompletedNotification } from "./HookCompletedNotification"; -export type { HookConfigSource } from "./HookConfigSource"; export type { HookErrorInfo } from "./HookErrorInfo"; export type { HookEventName } from "./HookEventName"; export type { HookExecutionMode } from "./HookExecutionMode"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 9b841810fc..63fdf043ae 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -4663,27 +4663,32 @@ pub struct HookMetadata { pub enabled: bool, } -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] -#[serde(rename_all = "snake_case")] -#[ts(rename_all = "snake_case")] -#[ts(export_to = "v2/")] -pub enum HookConfigSource { - Plugin, - User, - Project, -} - #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] -#[serde(rename_all = "camelCase")] +#[serde(tag = "source", rename_all = "snake_case", deny_unknown_fields)] +#[ts(tag = "source", rename_all = "snake_case")] #[ts(export_to = "v2/")] -pub struct HooksConfigWriteParams { - pub source: HookConfigSource, - #[ts(optional = nullable)] - pub plugin_id: Option, - #[ts(optional = nullable)] - pub source_path: Option, - pub key: String, - pub enabled: bool, +pub enum HooksConfigWriteParams { + Plugin { + #[serde(rename = "pluginId")] + #[ts(rename = "pluginId")] + plugin_id: String, + key: String, + enabled: bool, + }, + User { + #[serde(rename = "sourcePath")] + #[ts(rename = "sourcePath")] + source_path: PathBuf, + key: String, + enabled: bool, + }, + Project { + #[serde(rename = "sourcePath")] + #[ts(rename = "sourcePath")] + source_path: PathBuf, + key: String, + enabled: bool, + }, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -9770,6 +9775,42 @@ mod tests { ); } + #[test] + fn hooks_config_write_params_round_trips_selector_variants() { + let source_path = PathBuf::from(absolute_path_string("repo/.codex/hooks.json")); + let value = json!({ + "source": "project", + "sourcePath": source_path, + "key": "PreToolUse:0:0", + "enabled": false, + }); + let params: HooksConfigWriteParams = + serde_json::from_value(value.clone()).expect("deserialize project hook selector"); + + assert_eq!( + params, + HooksConfigWriteParams::Project { + source_path: source_path.clone(), + key: "PreToolUse:0:0".to_string(), + enabled: false, + } + ); + assert_eq!( + serde_json::to_value(¶ms).expect("serialize project hook selector"), + value + ); + + let err = serde_json::from_value::(json!({ + "source": "project", + "pluginId": "demo-plugin@test-marketplace", + "sourcePath": source_path, + "key": "PreToolUse:0:0", + "enabled": false, + })) + .expect_err("project hook selector should reject pluginId"); + assert!(err.to_string().contains("pluginId")); + } + #[test] fn network_requirements_deserializes_legacy_fields() { let requirements: NetworkRequirements = serde_json::from_value(json!({ diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 910571966a..e78f172431 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -75,7 +75,6 @@ use codex_app_server_protocol::GetConversationSummaryParams; use codex_app_server_protocol::GetConversationSummaryResponse; use codex_app_server_protocol::GitDiffToRemoteResponse; use codex_app_server_protocol::GitInfo as ApiGitInfo; -use codex_app_server_protocol::HookConfigSource as ApiHookConfigSource; use codex_app_server_protocol::HookErrorInfo; use codex_app_server_protocol::HookMetadata; use codex_app_server_protocol::HooksConfigWriteParams; @@ -238,6 +237,7 @@ use codex_backend_client::AddCreditsNudgeCreditType as BackendAddCreditsNudgeCre use codex_backend_client::Client as BackendClient; use codex_chatgpt::connectors; use codex_chatgpt::workspace_settings; +use codex_config::HookConfigSelector; use codex_config::types::McpServerTransportConfig; use codex_core::CodexThread; use codex_core::CodexThreadTurnContextOverrides; @@ -256,7 +256,6 @@ use codex_core::config::NetworkProxyAuditMetadata; use codex_core::config::ThreadStoreConfig; use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; -use codex_core::config::edit::HookConfigEditSelector; use codex_core::config_loader::CloudRequirementsLoadError; use codex_core::config_loader::CloudRequirementsLoadErrorCode; use codex_core::config_loader::project_trust_key; @@ -7174,69 +7173,57 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: HooksConfigWriteParams, ) { - let HooksConfigWriteParams { - source, - plugin_id, - source_path, - key, - enabled, - } = params; - let edit = match source { - ApiHookConfigSource::Plugin => { - let Some(plugin_id) = plugin_id.filter(|plugin_id| !plugin_id.trim().is_empty()) - else { + let (selector, enabled) = match params { + HooksConfigWriteParams::Plugin { + plugin_id, + key, + enabled, + } => { + if plugin_id.trim().is_empty() || key.trim().is_empty() { self.send_invalid_request_error( request_id, - "hooks/config/write requires pluginId when source is plugin".to_string(), - ) - .await; - return; - }; - if key.trim().is_empty() { - self.send_invalid_request_error( - request_id, - "hooks/config/write requires a non-empty key".to_string(), + "hooks/config/write requires non-empty pluginId and key for plugin selectors" + .to_string(), ) .await; return; } - ConfigEdit::SetHookConfig { - selector: HookConfigEditSelector::Plugin { plugin_id, key }, - enabled, - } + (HookConfigSelector::Plugin { plugin_id, key }, enabled) } - ApiHookConfigSource::User | ApiHookConfigSource::Project => { - let Some(source_path) = - source_path.filter(|source_path| !source_path.as_os_str().is_empty()) - else { + HooksConfigWriteParams::User { + source_path, + key, + enabled, + } => { + if source_path.as_os_str().is_empty() || key.trim().is_empty() { self.send_invalid_request_error( request_id, - format!( - "hooks/config/write requires sourcePath when source is {}", - hook_config_source_label(source) - ), - ) - .await; - return; - }; - if key.trim().is_empty() { - self.send_invalid_request_error( - request_id, - "hooks/config/write requires a non-empty key".to_string(), + "hooks/config/write requires non-empty sourcePath and key for user selectors" + .to_string(), ) .await; return; } - let selector = match source { - ApiHookConfigSource::User => HookConfigEditSelector::User { source_path, key }, - ApiHookConfigSource::Project => { - HookConfigEditSelector::Project { source_path, key } - } - ApiHookConfigSource::Plugin => unreachable!("plugin handled above"), - }; - ConfigEdit::SetHookConfig { selector, enabled } + (HookConfigSelector::User { source_path, key }, enabled) + } + HooksConfigWriteParams::Project { + source_path, + key, + enabled, + } => { + if source_path.as_os_str().is_empty() || key.trim().is_empty() { + self.send_invalid_request_error( + request_id, + "hooks/config/write requires non-empty sourcePath and key for project selectors" + .to_string(), + ) + .await; + return; + } + (HookConfigSelector::Project { source_path, key }, enabled) } }; + let edit = ConfigEdit::SetHookConfig { selector, enabled }; let result = ConfigEditsBuilder::new(&self.config.codex_home) .with_edits([edit]) .apply() @@ -9594,14 +9581,6 @@ fn hook_to_info(hook: HookInventoryEntry) -> HookMetadata { } } -fn hook_config_source_label(source: ApiHookConfigSource) -> &'static str { - match source { - ApiHookConfigSource::Plugin => "plugin", - ApiHookConfigSource::User => "user", - ApiHookConfigSource::Project => "project", - } -} - fn local_plugin_interface_to_info(interface: PluginManifestInterface) -> PluginInterface { PluginInterface { display_name: interface.display_name, diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index c12a70edec..4db24ad04a 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -14,7 +14,6 @@ use codex_app_server_protocol::ConfigReadParams; use codex_app_server_protocol::ConfigReadResponse; use codex_app_server_protocol::ConfigValueWriteParams; use codex_app_server_protocol::ConfigWriteResponse; -use codex_app_server_protocol::HookConfigSource; use codex_app_server_protocol::HookEventName; use codex_app_server_protocol::HookSource; use codex_app_server_protocol::HooksConfigWriteResponse; @@ -108,7 +107,7 @@ async fn hooks_config_write_persists_project_selector() -> Result<()> { .send_raw_request( "hooks/config/write", Some(json!({ - "source": HookConfigSource::Project, + "source": "project", "sourcePath": project_hook_path.clone(), "key": "PreToolUse:0:0", "enabled": false, diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs index 2edd3660fa..0d8baba6e6 100644 --- a/codex-rs/config/src/hook_config.rs +++ b/codex-rs/config/src/hook_config.rs @@ -93,6 +93,13 @@ pub enum HookConfigSource { Project, } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum HookConfigSelector { + Plugin { plugin_id: String, key: String }, + User { source_path: PathBuf, key: String }, + Project { source_path: PathBuf, key: String }, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct HookConfig { pub source: HookConfigSource, diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index bd9f56c1e2..16754c1fd4 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -69,6 +69,7 @@ pub use diagnostics::format_config_error_with_source; pub use diagnostics::io_error_from_config_error; pub use fingerprint::version_for_toml; pub use hook_config::HookConfig; +pub use hook_config::HookConfigSelector; pub use hook_config::HookConfigSource; pub use hook_config::HookEventsToml; pub use hook_config::HookHandlerConfig; diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index ef1a5732ce..49a489e041 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -2,6 +2,7 @@ use crate::path_utils::resolve_symlink_write_paths; use crate::path_utils::write_atomically; use anyhow::Context; use codex_config::CONFIG_TOML_FILE; +use codex_config::HookConfigSelector; use codex_config::types::McpServerConfig; use codex_features::FEATURES; use codex_protocol::config_types::Personality; @@ -63,7 +64,7 @@ pub enum ConfigEdit { SetSkillConfigByName { name: String, enabled: bool }, /// Set or clear a hook config entry under `[[hooks.config]]`. SetHookConfig { - selector: HookConfigEditSelector, + selector: HookConfigSelector, enabled: bool, }, /// Set trust_level under `[projects.""]`, @@ -84,13 +85,6 @@ enum SkillConfigSelector { Path(PathBuf), } -#[derive(Clone, Debug, PartialEq, Eq)] -pub enum HookConfigEditSelector { - Plugin { plugin_id: String, key: String }, - User { source_path: PathBuf, key: String }, - Project { source_path: PathBuf, key: String }, -} - /// Produces a config edit that sets `[tui].theme = ""`. pub fn syntax_theme_edit(name: &str) -> ConfigEdit { ConfigEdit::SetPath { @@ -633,193 +627,87 @@ impl ConfigDocument { if matches!(&selector, SkillConfigSelector::Name(name) if name.is_empty()) { return false; } - let mut remove_skills_table = false; - let mut mutated = false; - - { - let root = self.doc.as_table_mut(); - let skills_item = match root.get_mut("skills") { - Some(item) => item, - None => { - if enabled { - return false; - } - root.insert( - "skills", - TomlItem::Table(document_helpers::new_implicit_table()), - ); - let Some(item) = root.get_mut("skills") else { - return false; - }; - item - } - }; - - if document_helpers::ensure_table_for_write(skills_item).is_none() { - if enabled { - return false; - } - *skills_item = TomlItem::Table(document_helpers::new_implicit_table()); - } - let Some(skills_table) = skills_item.as_table_mut() else { - return false; - }; - - let config_item = match skills_table.get_mut("config") { - Some(item) => item, - None => { - if enabled { - return false; - } - skills_table.insert("config", TomlItem::ArrayOfTables(ArrayOfTables::new())); - let Some(item) = skills_table.get_mut("config") else { - return false; - }; - item - } - }; - - if !matches!(config_item, TomlItem::ArrayOfTables(_)) { - if enabled { - return false; - } - *config_item = TomlItem::ArrayOfTables(ArrayOfTables::new()); - } - - let TomlItem::ArrayOfTables(overrides) = config_item else { - return false; - }; - - let existing_index = overrides.iter().enumerate().find_map(|(idx, table)| { - skill_config_selector_from_table(table) - .filter(|value| value == &selector) - .map(|_| idx) - }); - - if enabled { - if let Some(index) = existing_index { - overrides.remove(index); - mutated = true; - if overrides.is_empty() { - skills_table.remove("config"); - if skills_table.is_empty() { - remove_skills_table = true; - } - } - } - } else if let Some(index) = existing_index { - for (idx, table) in overrides.iter_mut().enumerate() { - if idx == index { - write_skill_config_selector(table, &selector); - table["enabled"] = value(false); - mutated = true; - break; - } - } - } else { - let mut entry = TomlTable::new(); - entry.set_implicit(false); - write_skill_config_selector(&mut entry, &selector); - entry["enabled"] = value(false); - overrides.push(entry); - mutated = true; - } - } - - if remove_skills_table { - let root = self.doc.as_table_mut(); - root.remove("skills"); - } - - mutated + self.set_config_table_entry( + "skills", + &selector, + enabled, + skill_config_selector_from_table, + write_skill_config_selector, + ) } - fn set_hook_config(&mut self, selector: HookConfigEditSelector, enabled: bool) -> bool { - let selector = match selector { - HookConfigEditSelector::Plugin { plugin_id, key } => HookConfigEditSelector::Plugin { - plugin_id: plugin_id.trim().to_string(), - key: key.trim().to_string(), - }, - HookConfigEditSelector::User { source_path, key } => HookConfigEditSelector::User { - source_path, - key: key.trim().to_string(), - }, - HookConfigEditSelector::Project { source_path, key } => { - HookConfigEditSelector::Project { - source_path, - key: key.trim().to_string(), - } - } - }; + fn set_hook_config(&mut self, selector: HookConfigSelector, enabled: bool) -> bool { + let selector = normalize_hook_config_selector(selector); if match &selector { - HookConfigEditSelector::Plugin { plugin_id, key } => { - plugin_id.is_empty() || key.is_empty() - } - HookConfigEditSelector::User { source_path, key } - | HookConfigEditSelector::Project { source_path, key } => { + HookConfigSelector::Plugin { plugin_id, key } => plugin_id.is_empty() || key.is_empty(), + HookConfigSelector::User { source_path, key } + | HookConfigSelector::Project { source_path, key } => { source_path.as_os_str().is_empty() || key.is_empty() } } { return false; } - let selector = match selector { - HookConfigEditSelector::Plugin { plugin_id, key } => HookConfigEditSelector::Plugin { - plugin_id: plugin_id.trim().to_string(), - key: key.trim().to_string(), - }, - HookConfigEditSelector::User { source_path, key } => HookConfigEditSelector::User { - source_path: normalize_hook_config_source_path(&source_path), - key, - }, - HookConfigEditSelector::Project { source_path, key } => { - HookConfigEditSelector::Project { - source_path: normalize_hook_config_source_path(&source_path), - key, - } - } - }; + self.set_config_table_entry( + "hooks", + &selector, + enabled, + hook_config_selector_from_table, + write_hook_config_selector, + ) + } - let mut remove_hooks_table = false; + fn set_config_table_entry( + &mut self, + section: &'static str, + selector: &S, + enabled: bool, + selector_from_table: impl Fn(&TomlTable) -> Option, + write_selector: impl Fn(&mut TomlTable, &S), + ) -> bool + where + S: Eq, + { + let mut remove_section_table = false; let mut mutated = false; { let root = self.doc.as_table_mut(); - let hooks_item = match root.get_mut("hooks") { + let section_item = match root.get_mut(section) { Some(item) => item, None => { if enabled { return false; } root.insert( - "hooks", + section, TomlItem::Table(document_helpers::new_implicit_table()), ); - let Some(item) = root.get_mut("hooks") else { + let Some(item) = root.get_mut(section) else { return false; }; item } }; - if document_helpers::ensure_table_for_write(hooks_item).is_none() { + if document_helpers::ensure_table_for_write(section_item).is_none() { if enabled { return false; } - *hooks_item = TomlItem::Table(document_helpers::new_implicit_table()); + *section_item = TomlItem::Table(document_helpers::new_implicit_table()); } - let Some(hooks_table) = hooks_item.as_table_mut() else { + let Some(section_table) = section_item.as_table_mut() else { return false; }; - let config_item = match hooks_table.get_mut("config") { + let config_item = match section_table.get_mut("config") { Some(item) => item, None => { if enabled { return false; } - hooks_table.insert("config", TomlItem::ArrayOfTables(ArrayOfTables::new())); - let Some(item) = hooks_table.get_mut("config") else { + section_table.insert("config", TomlItem::ArrayOfTables(ArrayOfTables::new())); + let Some(item) = section_table.get_mut("config") else { return false; }; item @@ -838,8 +726,8 @@ impl ConfigDocument { }; let existing_index = overrides.iter().enumerate().find_map(|(idx, table)| { - hook_config_selector_from_table(table) - .filter(|value| value == &selector) + selector_from_table(table) + .filter(|value| value == selector) .map(|_| idx) }); @@ -848,16 +736,16 @@ impl ConfigDocument { overrides.remove(index); mutated = true; if overrides.is_empty() { - hooks_table.remove("config"); - if hooks_table.is_empty() { - remove_hooks_table = true; + section_table.remove("config"); + if section_table.is_empty() { + remove_section_table = true; } } } } else if let Some(index) = existing_index { for (idx, table) in overrides.iter_mut().enumerate() { if idx == index { - write_hook_config_selector(table, &selector); + write_selector(table, selector); table["enabled"] = value(false); mutated = true; break; @@ -866,16 +754,16 @@ impl ConfigDocument { } else { let mut entry = TomlTable::new(); entry.set_implicit(false); - write_hook_config_selector(&mut entry, &selector); + write_selector(&mut entry, selector); entry["enabled"] = value(false); overrides.push(entry); mutated = true; } } - if remove_hooks_table { + if remove_section_table { let root = self.doc.as_table_mut(); - root.remove("hooks"); + root.remove(section); } mutated @@ -1027,7 +915,7 @@ fn write_skill_config_selector(table: &mut TomlTable, selector: &SkillConfigSele } } -fn hook_config_selector_from_table(table: &TomlTable) -> Option { +fn hook_config_selector_from_table(table: &TomlTable) -> Option { let source = table .get("source") .and_then(|item| item.as_str()) @@ -1051,18 +939,16 @@ fn hook_config_selector_from_table(table: &TomlTable) -> Option { - Some(HookConfigEditSelector::Plugin { - plugin_id: plugin_id.to_string(), - key: key.to_string(), - }) - } - (Some("user"), None, Some(source_path), Some(key)) => Some(HookConfigEditSelector::User { + (Some("plugin"), Some(plugin_id), None, Some(key)) => Some(HookConfigSelector::Plugin { + plugin_id: plugin_id.to_string(), + key: key.to_string(), + }), + (Some("user"), None, Some(source_path), Some(key)) => Some(HookConfigSelector::User { source_path, key: key.to_string(), }), (Some("project"), None, Some(source_path), Some(key)) => { - Some(HookConfigEditSelector::Project { + Some(HookConfigSelector::Project { source_path, key: key.to_string(), }) @@ -1071,21 +957,21 @@ fn hook_config_selector_from_table(table: &TomlTable) -> Option { + HookConfigSelector::Plugin { plugin_id, key } => { table["source"] = value("plugin"); table["plugin_id"] = value(plugin_id.clone()); table.remove("source_path"); table["key"] = value(key.clone()); } - HookConfigEditSelector::User { source_path, key } => { + HookConfigSelector::User { source_path, key } => { table["source"] = value("user"); table.remove("plugin_id"); table["source_path"] = value(source_path.to_string_lossy().to_string()); table["key"] = value(key.clone()); } - HookConfigEditSelector::Project { source_path, key } => { + HookConfigSelector::Project { source_path, key } => { table["source"] = value("project"); table.remove("plugin_id"); table["source_path"] = value(source_path.to_string_lossy().to_string()); @@ -1094,6 +980,23 @@ fn write_hook_config_selector(table: &mut TomlTable, selector: &HookConfigEditSe } } +fn normalize_hook_config_selector(selector: HookConfigSelector) -> HookConfigSelector { + match selector { + HookConfigSelector::Plugin { plugin_id, key } => HookConfigSelector::Plugin { + plugin_id: plugin_id.trim().to_string(), + key: key.trim().to_string(), + }, + HookConfigSelector::User { source_path, key } => HookConfigSelector::User { + source_path: normalize_hook_config_source_path(&source_path), + key: key.trim().to_string(), + }, + HookConfigSelector::Project { source_path, key } => HookConfigSelector::Project { + source_path: normalize_hook_config_source_path(&source_path), + key: key.trim().to_string(), + }, + } +} + fn normalize_hook_config_source_path(path: &Path) -> PathBuf { path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) } diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index eab5ffffae..e90e2bf196 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -141,7 +141,7 @@ fn set_hook_config_writes_disabled_plugin_entry() { ConfigEditsBuilder::new(codex_home) .with_edits([ConfigEdit::SetHookConfig { - selector: HookConfigEditSelector::Plugin { + selector: HookConfigSelector::Plugin { plugin_id: "demo-plugin@test-marketplace".to_string(), key: "hooks/hooks.json:PreToolUse:0:0".to_string(), }, @@ -177,7 +177,7 @@ enabled = false ConfigEditsBuilder::new(codex_home) .with_edits([ConfigEdit::SetHookConfig { - selector: HookConfigEditSelector::Plugin { + selector: HookConfigSelector::Plugin { plugin_id: "demo-plugin@test-marketplace".to_string(), key: "hooks/hooks.json:PreToolUse:0:0".to_string(), }, @@ -197,7 +197,7 @@ fn set_hook_config_writes_disabled_project_entry() { ConfigEditsBuilder::new(codex_home) .with_edits([ConfigEdit::SetHookConfig { - selector: HookConfigEditSelector::Project { + selector: HookConfigSelector::Project { source_path: PathBuf::from("/repo/.codex/hooks.json"), key: "PreToolUse:0:0".to_string(), }, diff --git a/codex-rs/hooks/src/engine/config_rules.rs b/codex-rs/hooks/src/engine/config_rules.rs index 64866061d2..3a4c518d05 100644 --- a/codex-rs/hooks/src/engine/config_rules.rs +++ b/codex-rs/hooks/src/engine/config_rules.rs @@ -5,6 +5,7 @@ use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; use codex_config::HookConfig; +use codex_config::HookConfigSelector; use codex_config::HookConfigSource; use codex_config::HookEventsToml; use codex_protocol::protocol::HookEventName; @@ -13,8 +14,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; #[derive(Default)] pub(crate) struct HookConfigRules { - plugin: HashMap<(String, String), bool>, - path: HashMap<(HookConfigSource, String, String), bool>, + entries: HashMap, } impl HookConfigRules { @@ -53,13 +53,6 @@ impl HookConfigRules { rules } - pub(crate) fn enabled_for_plugin_hook(&self, plugin_id: &str, key: &str) -> bool { - self.plugin - .get(&(plugin_id.to_string(), key.to_string())) - .copied() - .unwrap_or_else(default_hook_enabled) - } - pub(crate) fn enabled_for_hook( &self, source: HookSource, @@ -67,83 +60,25 @@ impl HookConfigRules { source_path: &AbsolutePathBuf, key: &str, ) -> bool { - match source { - HookSource::Plugin => plugin_id - .map(|plugin_id| self.enabled_for_plugin_hook(plugin_id, key)) - .unwrap_or_else(default_hook_enabled), - HookSource::User | HookSource::Project => { - let Some(source) = hook_config_source_for_hook_source(source) else { - return default_hook_enabled(); - }; - self.path - .get(&( - source, - normalize_source_path(source_path.as_path()), - key.to_string(), - )) - .copied() - .unwrap_or_else(default_hook_enabled) - } - HookSource::System - | HookSource::Mdm - | HookSource::SessionFlags - | HookSource::LegacyManagedConfigFile - | HookSource::LegacyManagedConfigMdm - | HookSource::Unknown => default_hook_enabled(), - } + hook_config_selector_for_hook(source, plugin_id, source_path, key) + .map(|selector| self.enabled_for_selector(selector)) + .unwrap_or_else(default_hook_enabled) + } + + fn enabled_for_selector(&self, selector: HookConfigSelector) -> bool { + self.entries + .get(&normalize_hook_config_selector(selector)) + .copied() + .unwrap_or_else(default_hook_enabled) } fn append(&mut self, entry: HookConfig, warnings: &mut Vec) { - match entry.source { - HookConfigSource::Plugin => { - let Some(plugin_id) = entry.plugin_id else { - warnings.push( - "ignoring plugin hooks.config entry without a plugin_id selector" - .to_string(), - ); - return; - }; - if plugin_id.trim().is_empty() { - warnings.push( - "ignoring plugin hooks.config entry with empty plugin_id".to_string(), - ); - return; - } - if entry.key.trim().is_empty() { - warnings.push("ignoring hooks.config entry with empty key".to_string()); - return; - } - self.plugin.insert((plugin_id, entry.key), entry.enabled); - } - HookConfigSource::User | HookConfigSource::Project => { - let Some(source_path) = entry.source_path else { - warnings.push(format!( - "ignoring {} hooks.config entry without a source_path selector", - hook_config_source_label(entry.source) - )); - return; - }; - if source_path.as_os_str().is_empty() { - warnings.push(format!( - "ignoring {} hooks.config entry with empty source_path", - hook_config_source_label(entry.source) - )); - return; - } - if entry.key.trim().is_empty() { - warnings.push("ignoring hooks.config entry with empty key".to_string()); - return; - } - self.path.insert( - ( - entry.source, - normalize_source_path(&source_path), - entry.key.trim().to_string(), - ), - entry.enabled, - ); - } - } + let enabled = entry.enabled; + let Some(selector) = hook_config_selector_for_config(entry, warnings) else { + return; + }; + self.entries + .insert(normalize_hook_config_selector(selector), enabled); } } @@ -166,6 +101,95 @@ fn hook_config_source_for_hook_source(source: HookSource) -> Option, + source_path: &AbsolutePathBuf, + key: &str, +) -> Option { + match source { + HookSource::Plugin => plugin_id.map(|plugin_id| HookConfigSelector::Plugin { + plugin_id: plugin_id.to_string(), + key: key.to_string(), + }), + HookSource::User | HookSource::Project => { + hook_config_source_for_hook_source(source).map(|source| match source { + HookConfigSource::User => HookConfigSelector::User { + source_path: source_path.as_path().to_path_buf(), + key: key.to_string(), + }, + HookConfigSource::Project => HookConfigSelector::Project { + source_path: source_path.as_path().to_path_buf(), + key: key.to_string(), + }, + HookConfigSource::Plugin => unreachable!("plugin hook source handled above"), + }) + } + HookSource::System + | HookSource::Mdm + | HookSource::SessionFlags + | HookSource::LegacyManagedConfigFile + | HookSource::LegacyManagedConfigMdm + | HookSource::Unknown => None, + } +} + +fn hook_config_selector_for_config( + entry: HookConfig, + warnings: &mut Vec, +) -> Option { + if entry.key.trim().is_empty() { + warnings.push("ignoring hooks.config entry with empty key".to_string()); + return None; + } + match entry.source { + HookConfigSource::Plugin => { + let Some(plugin_id) = entry.plugin_id else { + warnings.push( + "ignoring plugin hooks.config entry without a plugin_id selector".to_string(), + ); + return None; + }; + if plugin_id.trim().is_empty() { + warnings + .push("ignoring plugin hooks.config entry with empty plugin_id".to_string()); + return None; + } + Some(HookConfigSelector::Plugin { + plugin_id, + key: entry.key, + }) + } + HookConfigSource::User | HookConfigSource::Project => { + let Some(source_path) = entry.source_path else { + warnings.push(format!( + "ignoring {} hooks.config entry without a source_path selector", + hook_config_source_label(entry.source) + )); + return None; + }; + if source_path.as_os_str().is_empty() { + warnings.push(format!( + "ignoring {} hooks.config entry with empty source_path", + hook_config_source_label(entry.source) + )); + return None; + } + match entry.source { + HookConfigSource::User => Some(HookConfigSelector::User { + source_path, + key: entry.key, + }), + HookConfigSource::Project => Some(HookConfigSelector::Project { + source_path, + key: entry.key, + }), + HookConfigSource::Plugin => unreachable!("plugin config source handled above"), + } + } + } +} + fn hook_config_source_label(source: HookConfigSource) -> &'static str { match source { HookConfigSource::Plugin => "plugin", @@ -174,11 +198,25 @@ fn hook_config_source_label(source: HookConfigSource) -> &'static str { } } -fn normalize_source_path(path: &Path) -> String { - path.canonicalize() - .unwrap_or_else(|_| path.to_path_buf()) - .to_string_lossy() - .to_string() +fn normalize_hook_config_selector(selector: HookConfigSelector) -> HookConfigSelector { + match selector { + HookConfigSelector::Plugin { plugin_id, key } => HookConfigSelector::Plugin { + plugin_id: plugin_id.trim().to_string(), + key: key.trim().to_string(), + }, + HookConfigSelector::User { source_path, key } => HookConfigSelector::User { + source_path: normalize_source_path(&source_path), + key: key.trim().to_string(), + }, + HookConfigSelector::Project { source_path, key } => HookConfigSelector::Project { + source_path: normalize_source_path(&source_path), + key: key.trim().to_string(), + }, + } +} + +fn normalize_source_path(path: &Path) -> std::path::PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) } pub(crate) fn hook_config_key(