From 7cd4bc1a4f204909c39450730d4f70a85a04528d Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 15 Apr 2026 00:50:46 -0700 Subject: [PATCH 01/12] WIP managed hooks requirements --- .../codex_app_server_protocol.schemas.json | 148 ++++++++ .../codex_app_server_protocol.v2.schemas.json | 148 ++++++++ .../v2/ConfigRequirementsReadResponse.json | 148 ++++++++ .../typescript/v2/ConfiguredHookHandler.ts | 5 + .../v2/ConfiguredHookMatcherGroup.ts | 6 + .../typescript/v2/ManagedHooksRequirements.ts | 6 + .../schema/typescript/v2/index.ts | 3 + .../app-server-protocol/src/protocol/v2.rs | 58 ++++ codex-rs/app-server/README.md | 2 +- codex-rs/app-server/src/config_api.rs | 109 ++++++ codex-rs/config/src/config_requirements.rs | 168 ++++++++++ codex-rs/config/src/config_toml.rs | 4 + codex-rs/config/src/hook_config.rs | 197 +++++++++++ codex-rs/config/src/lib.rs | 9 + codex-rs/core/config.schema.json | 128 +++++++ codex-rs/core/src/config/config_tests.rs | 2 + codex-rs/core/src/config/mod.rs | 1 + codex-rs/core/src/config_loader/mod.rs | 4 + codex-rs/core/src/config_loader/tests.rs | 56 ++++ codex-rs/core/tests/common/test_codex.rs | 6 +- codex-rs/core/tests/suite/hooks.rs | 243 +++++++++++++- codex-rs/hooks/src/engine/config.rs | 48 --- codex-rs/hooks/src/engine/discovery.rs | 317 ++++++++++++++---- codex-rs/hooks/src/engine/dispatcher.rs | 1 + codex-rs/hooks/src/engine/mod.rs | 303 ++++++++++++++++- 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/tui/src/debug_config.rs | 78 +++++ 31 files changed, 2067 insertions(+), 136 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookMatcherGroup.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/ManagedHooksRequirements.ts create mode 100644 codex-rs/config/src/hook_config.rs delete mode 100644 codex-rs/hooks/src/engine/config.rs 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 1f2072070154..a18f32228d16 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 @@ -6974,6 +6974,100 @@ "title": "ConfigWriteResponse", "type": "object" }, + "ConfiguredHookHandler": { + "oneOf": [ + { + "properties": { + "async": { + "type": "boolean" + }, + "command": { + "type": "string" + }, + "statusMessage": { + "type": [ + "string", + "null" + ] + }, + "timeoutSec": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "type": { + "enum": [ + "command" + ], + "title": "CommandConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "async", + "command", + "type" + ], + "title": "CommandConfiguredHookHandler", + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "prompt" + ], + "title": "PromptConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "PromptConfiguredHookHandler", + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "agent" + ], + "title": "AgentConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "AgentConfiguredHookHandler", + "type": "object" + } + ] + }, + "ConfiguredHookMatcherGroup": { + "properties": { + "hooks": { + "items": { + "$ref": "#/definitions/v2/ConfiguredHookHandler" + }, + "type": "array" + }, + "matcher": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "hooks" + ], + "type": "object" + }, "ContentItem": { "oneOf": [ { @@ -9030,6 +9124,60 @@ "title": "LogoutAccountResponse", "type": "object" }, + "ManagedHooksRequirements": { + "properties": { + "PostToolUse": { + "items": { + "$ref": "#/definitions/v2/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "PreToolUse": { + "items": { + "$ref": "#/definitions/v2/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "SessionStart": { + "items": { + "$ref": "#/definitions/v2/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "Stop": { + "items": { + "$ref": "#/definitions/v2/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "UserPromptSubmit": { + "items": { + "$ref": "#/definitions/v2/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "managedDir": { + "type": [ + "string", + "null" + ] + }, + "windowsManagedDir": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "PostToolUse", + "PreToolUse", + "SessionStart", + "Stop", + "UserPromptSubmit" + ], + "type": "object" + }, "MarketplaceInterface": { "properties": { "displayName": { 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 d4d76de9f0ed..0e524b638780 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 @@ -3615,6 +3615,100 @@ "title": "ConfigWriteResponse", "type": "object" }, + "ConfiguredHookHandler": { + "oneOf": [ + { + "properties": { + "async": { + "type": "boolean" + }, + "command": { + "type": "string" + }, + "statusMessage": { + "type": [ + "string", + "null" + ] + }, + "timeoutSec": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "type": { + "enum": [ + "command" + ], + "title": "CommandConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "async", + "command", + "type" + ], + "title": "CommandConfiguredHookHandler", + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "prompt" + ], + "title": "PromptConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "PromptConfiguredHookHandler", + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "agent" + ], + "title": "AgentConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "AgentConfiguredHookHandler", + "type": "object" + } + ] + }, + "ConfiguredHookMatcherGroup": { + "properties": { + "hooks": { + "items": { + "$ref": "#/definitions/ConfiguredHookHandler" + }, + "type": "array" + }, + "matcher": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "hooks" + ], + "type": "object" + }, "ContentItem": { "oneOf": [ { @@ -5826,6 +5920,60 @@ "title": "LogoutAccountResponse", "type": "object" }, + "ManagedHooksRequirements": { + "properties": { + "PostToolUse": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "PreToolUse": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "SessionStart": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "Stop": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "UserPromptSubmit": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "managedDir": { + "type": [ + "string", + "null" + ] + }, + "windowsManagedDir": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "PostToolUse", + "PreToolUse", + "SessionStart", + "Stop", + "UserPromptSubmit" + ], + "type": "object" + }, "MarketplaceInterface": { "properties": { "displayName": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json index ae6eb1dc7d3f..fad284031d3a 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json @@ -110,6 +110,154 @@ }, "type": "object" }, + "ConfiguredHookHandler": { + "oneOf": [ + { + "properties": { + "async": { + "type": "boolean" + }, + "command": { + "type": "string" + }, + "statusMessage": { + "type": [ + "string", + "null" + ] + }, + "timeoutSec": { + "format": "uint64", + "minimum": 0.0, + "type": [ + "integer", + "null" + ] + }, + "type": { + "enum": [ + "command" + ], + "title": "CommandConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "async", + "command", + "type" + ], + "title": "CommandConfiguredHookHandler", + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "prompt" + ], + "title": "PromptConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "PromptConfiguredHookHandler", + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "agent" + ], + "title": "AgentConfiguredHookHandlerType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "AgentConfiguredHookHandler", + "type": "object" + } + ] + }, + "ConfiguredHookMatcherGroup": { + "properties": { + "hooks": { + "items": { + "$ref": "#/definitions/ConfiguredHookHandler" + }, + "type": "array" + }, + "matcher": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "hooks" + ], + "type": "object" + }, + "ManagedHooksRequirements": { + "properties": { + "PostToolUse": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "PreToolUse": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "SessionStart": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "Stop": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "UserPromptSubmit": { + "items": { + "$ref": "#/definitions/ConfiguredHookMatcherGroup" + }, + "type": "array" + }, + "managedDir": { + "type": [ + "string", + "null" + ] + }, + "windowsManagedDir": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "PostToolUse", + "PreToolUse", + "SessionStart", + "Stop", + "UserPromptSubmit" + ], + "type": "object" + }, "NetworkDomainPermission": { "enum": [ "allow", diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts new file mode 100644 index 000000000000..a81ce61f6e97 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts @@ -0,0 +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. + +export type ConfiguredHookHandler = { "type": "command", command: string, timeoutSec: bigint | null, async: boolean, statusMessage: string | null, } | { "type": "prompt", } | { "type": "agent", }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookMatcherGroup.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookMatcherGroup.ts new file mode 100644 index 000000000000..2c00fc166c2d --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookMatcherGroup.ts @@ -0,0 +1,6 @@ +// 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 { ConfiguredHookHandler } from "./ConfiguredHookHandler"; + +export type ConfiguredHookMatcherGroup = { matcher: string | null, hooks: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ManagedHooksRequirements.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ManagedHooksRequirements.ts new file mode 100644 index 000000000000..4c0e8a78bc62 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ManagedHooksRequirements.ts @@ -0,0 +1,6 @@ +// 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 { ConfiguredHookMatcherGroup } from "./ConfiguredHookMatcherGroup"; + +export type ManagedHooksRequirements = { managedDir: string | null, windowsManagedDir: string | null, PreToolUse: Array, PostToolUse: Array, SessionStart: Array, UserPromptSubmit: Array, Stop: Array, }; 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 f815fee3e9d3..dcfa2f6a4a34 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -69,6 +69,8 @@ export type { ConfigRequirementsReadResponse } from "./ConfigRequirementsReadRes export type { ConfigValueWriteParams } from "./ConfigValueWriteParams"; export type { ConfigWarningNotification } from "./ConfigWarningNotification"; export type { ConfigWriteResponse } from "./ConfigWriteResponse"; +export type { ConfiguredHookHandler } from "./ConfiguredHookHandler"; +export type { ConfiguredHookMatcherGroup } from "./ConfiguredHookMatcherGroup"; export type { ContextCompactedNotification } from "./ContextCompactedNotification"; export type { CreditsSnapshot } from "./CreditsSnapshot"; export type { DeprecationNoticeNotification } from "./DeprecationNoticeNotification"; @@ -149,6 +151,7 @@ export type { ListMcpServerStatusResponse } from "./ListMcpServerStatusResponse" export type { LoginAccountParams } from "./LoginAccountParams"; export type { LoginAccountResponse } from "./LoginAccountResponse"; export type { LogoutAccountResponse } from "./LogoutAccountResponse"; +export type { ManagedHooksRequirements } from "./ManagedHooksRequirements"; export type { MarketplaceInterface } from "./MarketplaceInterface"; export type { MarketplaceLoadErrorInfo } from "./MarketplaceLoadErrorInfo"; export type { McpAuthStatus } from "./McpAuthStatus"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 8a6a6e57b30b..0e2011f6e162 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -869,11 +869,68 @@ pub struct ConfigRequirements { pub allowed_sandbox_modes: Option>, pub allowed_web_search_modes: Option>, pub feature_requirements: Option>, + #[experimental("configRequirements/read.hooks")] + pub hooks: Option, pub enforce_residency: Option, #[experimental("configRequirements/read.network")] pub network: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ManagedHooksRequirements { + pub managed_dir: Option, + pub windows_managed_dir: Option, + #[serde(rename = "PreToolUse")] + #[ts(rename = "PreToolUse")] + pub pre_tool_use: Vec, + #[serde(rename = "PostToolUse")] + #[ts(rename = "PostToolUse")] + pub post_tool_use: Vec, + #[serde(rename = "SessionStart")] + #[ts(rename = "SessionStart")] + pub session_start: Vec, + #[serde(rename = "UserPromptSubmit")] + #[ts(rename = "UserPromptSubmit")] + pub user_prompt_submit: Vec, + #[serde(rename = "Stop")] + #[ts(rename = "Stop")] + pub stop: Vec, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ConfiguredHookMatcherGroup { + pub matcher: Option, + pub hooks: Vec, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(tag = "type")] +#[ts(tag = "type", export_to = "v2/")] +pub enum ConfiguredHookHandler { + #[serde(rename = "command")] + #[ts(rename = "command")] + Command { + command: String, + #[serde(rename = "timeoutSec")] + #[ts(rename = "timeoutSec")] + timeout_sec: Option, + r#async: bool, + #[serde(rename = "statusMessage")] + #[ts(rename = "statusMessage")] + status_message: Option, + }, + #[serde(rename = "prompt")] + #[ts(rename = "prompt")] + Prompt {}, + #[serde(rename = "agent")] + #[ts(rename = "agent")] + Agent {}, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -7587,6 +7644,7 @@ mod tests { allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, + hooks: None, enforce_residency: None, network: None, }); diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index b0a16616aa2a..8487dcb34582 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -198,7 +198,7 @@ Example with notification opt-out: - `externalAgentConfig/import` — apply selected external-agent migration items by passing explicit `migrationItems` with `cwd` (`null` for home). - `config/value/write` — write a single config key/value to the user's config.toml on disk. - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads. -- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), pinned feature values (`featureRequirements`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. +- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), pinned feature values (`featureRequirements`), managed lifecycle hooks (`hooks`), `enforceResidency`, and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. ### Example: Start or resume a thread diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index e85f137bc946..c84e97bab79f 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -10,9 +10,12 @@ use codex_app_server_protocol::ConfigRequirementsReadResponse; use codex_app_server_protocol::ConfigValueWriteParams; use codex_app_server_protocol::ConfigWriteErrorCode; use codex_app_server_protocol::ConfigWriteResponse; +use codex_app_server_protocol::ConfiguredHookHandler; +use codex_app_server_protocol::ConfiguredHookMatcherGroup; use codex_app_server_protocol::ExperimentalFeatureEnablementSetParams; use codex_app_server_protocol::ExperimentalFeatureEnablementSetResponse; use codex_app_server_protocol::JSONRPCErrorError; +use codex_app_server_protocol::ManagedHooksRequirements; use codex_app_server_protocol::NetworkDomainPermission; use codex_app_server_protocol::NetworkRequirements; use codex_app_server_protocol::NetworkUnixSocketPermission; @@ -23,7 +26,11 @@ use codex_core::config::ConfigService; use codex_core::config::ConfigServiceError; use codex_core::config_loader::CloudRequirementsLoader; use codex_core::config_loader::ConfigRequirementsToml; +use codex_core::config_loader::HookEventsToml; +use codex_core::config_loader::HookHandlerConfig as CoreHookHandlerConfig; use codex_core::config_loader::LoaderOverrides; +use codex_core::config_loader::ManagedHooksRequirementsToml; +use codex_core::config_loader::MatcherGroup as CoreMatcherGroup; use codex_core::config_loader::ResidencyRequirement as CoreResidencyRequirement; use codex_core::config_loader::SandboxModeRequirement as CoreSandboxModeRequirement; use codex_core::plugins::PluginId; @@ -391,6 +398,7 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR feature_requirements: requirements .feature_requirements .map(|requirements| requirements.entries), + hooks: requirements.hooks.map(map_hooks_requirements_to_api), enforce_residency: requirements .enforce_residency .map(map_residency_requirement_to_api), @@ -398,6 +406,69 @@ fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigR } } +fn map_hooks_requirements_to_api(hooks: ManagedHooksRequirementsToml) -> ManagedHooksRequirements { + let ManagedHooksRequirementsToml { + managed_dir, + windows_managed_dir, + hooks, + } = hooks; + let HookEventsToml { + pre_tool_use, + post_tool_use, + session_start, + user_prompt_submit, + stop, + } = hooks; + + ManagedHooksRequirements { + managed_dir, + windows_managed_dir, + pre_tool_use: map_hook_matcher_groups_to_api(pre_tool_use), + post_tool_use: map_hook_matcher_groups_to_api(post_tool_use), + session_start: map_hook_matcher_groups_to_api(session_start), + user_prompt_submit: map_hook_matcher_groups_to_api(user_prompt_submit), + stop: map_hook_matcher_groups_to_api(stop), + } +} + +fn map_hook_matcher_groups_to_api( + groups: Vec, +) -> Vec { + groups + .into_iter() + .map(map_hook_matcher_group_to_api) + .collect() +} + +fn map_hook_matcher_group_to_api(group: CoreMatcherGroup) -> ConfiguredHookMatcherGroup { + ConfiguredHookMatcherGroup { + matcher: group.matcher, + hooks: group + .hooks + .into_iter() + .map(map_hook_handler_to_api) + .collect(), + } +} + +fn map_hook_handler_to_api(handler: CoreHookHandlerConfig) -> ConfiguredHookHandler { + match handler { + CoreHookHandlerConfig::Command { + command, + timeout_sec, + r#async, + status_message, + } => ConfiguredHookHandler::Command { + command, + timeout_sec, + r#async, + status_message, + }, + CoreHookHandlerConfig::Prompt {} => ConfiguredHookHandler::Prompt {}, + CoreHookHandlerConfig::Agent {} => ConfiguredHookHandler::Agent {}, + } +} + fn map_sandbox_mode_requirement_to_api(mode: CoreSandboxModeRequirement) -> Option { match mode { CoreSandboxModeRequirement::ReadOnly => Some(SandboxMode::ReadOnly), @@ -571,6 +642,22 @@ mod tests { ("personality".to_string(), true), ]), }), + hooks: Some(ManagedHooksRequirementsToml { + managed_dir: Some(PathBuf::from("/enterprise/hooks")), + windows_managed_dir: Some(PathBuf::from(r"C:\enterprise\hooks")), + hooks: HookEventsToml { + pre_tool_use: vec![CoreMatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![CoreHookHandlerConfig::Command { + command: "python3 /enterprise/hooks/pre.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + }), mcp_servers: None, apps: None, rules: None, @@ -637,6 +724,26 @@ mod tests { ("personality".to_string(), true), ])), ); + assert_eq!( + mapped.hooks, + Some(ManagedHooksRequirements { + managed_dir: Some(PathBuf::from("/enterprise/hooks")), + windows_managed_dir: Some(PathBuf::from(r"C:\enterprise\hooks")), + pre_tool_use: vec![ConfiguredHookMatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![ConfiguredHookHandler::Command { + command: "python3 /enterprise/hooks/pre.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + post_tool_use: Vec::new(), + session_start: Vec::new(), + user_prompt_submit: Vec::new(), + stop: Vec::new(), + }), + ); assert_eq!( mapped.enforce_residency, Some(codex_app_server_protocol::ResidencyRequirement::Us), @@ -677,6 +784,7 @@ mod tests { allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -736,6 +844,7 @@ mod tests { allowed_web_search_modes: Some(Vec::new()), guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 2e6756d81e86..a52704e8f38a 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -14,6 +14,7 @@ use super::requirements_exec_policy::RequirementsExecPolicy; use super::requirements_exec_policy::RequirementsExecPolicyToml; use crate::Constrained; use crate::ConstraintError; +use crate::ManagedHooksRequirementsToml; #[derive(Debug, Clone, PartialEq, Eq)] pub enum RequirementSource { @@ -83,6 +84,7 @@ pub struct ConfigRequirements { pub sandbox_policy: ConstrainedWithSource, pub web_search_mode: ConstrainedWithSource, pub feature_requirements: Option>, + pub managed_hooks: Option>, pub mcp_servers: Option>>, pub exec_policy: Option>, pub enforce_residency: ConstrainedWithSource>, @@ -110,6 +112,7 @@ impl Default for ConfigRequirements { /*source*/ None, ), feature_requirements: None, + managed_hooks: None, mcp_servers: None, exec_policy: None, enforce_residency: ConstrainedWithSource::new( @@ -508,6 +511,7 @@ pub struct ConfigRequirementsToml { pub allowed_web_search_modes: Option>, #[serde(rename = "features", alias = "feature_requirements")] pub feature_requirements: Option, + pub hooks: Option, pub mcp_servers: Option>, pub apps: Option, pub rules: Option, @@ -546,6 +550,7 @@ pub struct ConfigRequirementsWithSources { pub allowed_sandbox_modes: Option>>, pub allowed_web_search_modes: Option>>, pub feature_requirements: Option>, + pub hooks: Option>, pub mcp_servers: Option>>, pub apps: Option>, pub rules: Option>, @@ -578,6 +583,7 @@ impl ConfigRequirementsWithSources { allowed_sandbox_modes: _, allowed_web_search_modes: _, feature_requirements: _, + hooks: _, mcp_servers: _, apps: _, rules: _, @@ -604,6 +610,7 @@ impl ConfigRequirementsWithSources { allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, + hooks, mcp_servers, rules, enforce_residency, @@ -628,6 +635,7 @@ impl ConfigRequirementsWithSources { allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, + hooks, mcp_servers, apps, rules, @@ -641,6 +649,7 @@ impl ConfigRequirementsWithSources { allowed_sandbox_modes: allowed_sandbox_modes.map(|sourced| sourced.value), allowed_web_search_modes: allowed_web_search_modes.map(|sourced| sourced.value), feature_requirements: feature_requirements.map(|sourced| sourced.value), + hooks: hooks.map(|sourced| sourced.value), mcp_servers: mcp_servers.map(|sourced| sourced.value), apps: apps.map(|sourced| sourced.value), rules: rules.map(|sourced| sourced.value), @@ -694,6 +703,10 @@ impl ConfigRequirementsToml { .feature_requirements .as_ref() .is_none_or(FeatureRequirementsToml::is_empty) + && self + .hooks + .as_ref() + .is_none_or(ManagedHooksRequirementsToml::is_empty) && self.mcp_servers.is_none() && self .apps @@ -719,6 +732,7 @@ impl TryFrom for ConfigRequirements { allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, + hooks, mcp_servers, apps: _apps, rules, @@ -898,6 +912,34 @@ impl TryFrom for ConfigRequirements { }; let feature_requirements = feature_requirements.filter(|requirements| !requirements.value.is_empty()); + let managed_hooks = hooks + .filter(|managed_hooks| managed_hooks.value.handler_count() > 0) + .map(|sourced_hooks| { + let Sourced { + value, + source: requirement_source, + } = sourced_hooks; + let allowed = value; + let allowed_for_error = format!("{allowed:?}"); + let requirement_source_for_error = requirement_source.clone(); + let constrained = Constrained::new(allowed.clone(), move |candidate| { + if candidate == &allowed { + Ok(()) + } else { + Err(ConstraintError::InvalidValue { + field_name: "hooks", + candidate: format!("{candidate:?}"), + allowed: allowed_for_error.clone(), + requirement_source: requirement_source_for_error.clone(), + }) + } + })?; + Ok(ConstrainedWithSource::new( + constrained, + Some(requirement_source), + )) + }) + .transpose()?; let enforce_residency = match enforce_residency { Some(Sourced { @@ -935,6 +977,7 @@ impl TryFrom for ConfigRequirements { sandbox_policy, web_search_mode, feature_requirements, + managed_hooks, mcp_servers, exec_policy, enforce_residency, @@ -946,6 +989,7 @@ impl TryFrom for ConfigRequirements { #[cfg(test)] mod tests { use super::*; + use crate::HookEventsToml; use anyhow::Result; use codex_execpolicy::Decision; use codex_execpolicy::Evaluation; @@ -972,6 +1016,7 @@ mod tests { allowed_sandbox_modes, allowed_web_search_modes, feature_requirements, + hooks, mcp_servers, apps, rules, @@ -990,6 +1035,7 @@ mod tests { .map(|value| Sourced::new(value, RequirementSource::Unknown)), feature_requirements: feature_requirements .map(|value| Sourced::new(value, RequirementSource::Unknown)), + hooks: hooks.map(|value| Sourced::new(value, RequirementSource::Unknown)), mcp_servers: mcp_servers.map(|value| Sourced::new(value, RequirementSource::Unknown)), apps: apps.map(|value| Sourced::new(value, RequirementSource::Unknown)), rules: rules.map(|value| Sourced::new(value, RequirementSource::Unknown)), @@ -1032,6 +1078,7 @@ mod tests { allowed_sandbox_modes: Some(allowed_sandbox_modes.clone()), allowed_web_search_modes: Some(allowed_web_search_modes.clone()), feature_requirements: Some(feature_requirements.clone()), + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1062,6 +1109,7 @@ mod tests { feature_requirements, enforce_source.clone(), )), + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1098,6 +1146,7 @@ mod tests { allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1142,6 +1191,7 @@ mod tests { allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1803,6 +1853,124 @@ allowed_approvals_reviewers = ["user"] Ok(()) } + #[test] + fn deserialize_managed_hooks_requirements() -> Result<()> { + let toml_str = r#" + managed_dir = "/enterprise/hooks" + windows_managed_dir = 'C:\enterprise\hooks' + + [[PreToolUse]] + matcher = "^Bash$" + + [[PreToolUse.hooks]] + type = "command" + command = "python3 /enterprise/hooks/pre.py" + timeoutSec = 10 + statusMessage = "checking" + "#; + let hooks: ManagedHooksRequirementsToml = from_str(toml_str)?; + + assert_eq!( + hooks.managed_dir.as_deref(), + Some(std::path::Path::new("/enterprise/hooks")) + ); + assert_eq!(hooks.handler_count(), 1); + assert_eq!(hooks.hooks.pre_tool_use.len(), 1); + Ok(()) + } + + #[test] + fn merge_unset_fields_does_not_overwrite_existing_hooks() -> Result<()> { + let mut target = ConfigRequirementsWithSources::default(); + target.merge_unset_fields( + RequirementSource::CloudRequirements, + from_str::( + r#" + [hooks] + managed_dir = "/cloud/hooks" + + [[hooks.PreToolUse]] + matcher = "^Bash$" + + [[hooks.PreToolUse.hooks]] + type = "command" + command = "python3 /cloud/hooks/pre.py" + "#, + )?, + ); + target.merge_unset_fields( + RequirementSource::SystemRequirementsToml { + file: system_requirements_toml_file_for_test()?, + }, + from_str::( + r#" + [hooks] + managed_dir = "/system/hooks" + + [[hooks.PreToolUse]] + matcher = "^Bash$" + + [[hooks.PreToolUse.hooks]] + type = "command" + command = "python3 /system/hooks/pre.py" + "#, + )?, + ); + + assert_eq!( + target + .hooks + .as_ref() + .and_then(|hooks| hooks.value.managed_dir.as_ref()) + .map(std::path::PathBuf::as_path), + Some(std::path::Path::new("/cloud/hooks")) + ); + assert_eq!( + target.hooks.as_ref().map(|hooks| hooks.source.clone()), + Some(RequirementSource::CloudRequirements) + ); + Ok(()) + } + + #[test] + fn managed_hooks_constraint_rejects_drift() -> Result<()> { + let config: ConfigRequirementsToml = from_str( + r#" + [hooks] + managed_dir = "/enterprise/hooks" + + [[hooks.PreToolUse]] + matcher = "^Bash$" + + [[hooks.PreToolUse.hooks]] + type = "command" + command = "python3 /enterprise/hooks/pre.py" + "#, + )?; + let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; + let mut managed_hooks = requirements + .managed_hooks + .expect("expected managed hooks requirements"); + + let err = managed_hooks + .set(ManagedHooksRequirementsToml { + managed_dir: Some(std::path::PathBuf::from("/other/hooks")), + windows_managed_dir: None, + hooks: HookEventsToml::default(), + }) + .expect_err("managed hooks should reject drift"); + + assert!(matches!( + err, + ConstraintError::InvalidValue { + field_name: "hooks", + requirement_source: RequirementSource::Unknown, + .. + } + )); + Ok(()) + } + #[test] fn network_requirements_are_preserved_as_constraints_with_source() -> Result<()> { let toml_str = r#" diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index 92e5304fe583..fae5d76f1f0c 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -4,6 +4,7 @@ use std::collections::BTreeMap; use std::collections::HashMap; use std::path::Path; +use crate::HookEventsToml; use crate::permissions_toml::PermissionsToml; use crate::profile_toml::ConfigProfile; use crate::types::AnalyticsConfigToml; @@ -322,6 +323,9 @@ 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, + /// User-level plugin config entries keyed by plugin name. #[serde(default)] pub plugins: HashMap, diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs new file mode 100644 index 000000000000..be9893d1dd82 --- /dev/null +++ b/codex-rs/config/src/hook_config.rs @@ -0,0 +1,197 @@ +use std::path::Path; +use std::path::PathBuf; + +use codex_protocol::protocol::HookEventName; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; + +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct HooksFile { + #[serde(default)] + pub hooks: HookEventsToml, +} + +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct HookEventsToml { + #[serde(rename = "PreToolUse", default)] + pub pre_tool_use: Vec, + #[serde(rename = "PostToolUse", default)] + pub post_tool_use: Vec, + #[serde(rename = "SessionStart", default)] + pub session_start: Vec, + #[serde(rename = "UserPromptSubmit", default)] + pub user_prompt_submit: Vec, + #[serde(rename = "Stop", default)] + pub stop: Vec, +} + +impl HookEventsToml { + pub fn is_empty(&self) -> bool { + self.pre_tool_use.is_empty() + && self.post_tool_use.is_empty() + && self.session_start.is_empty() + && self.user_prompt_submit.is_empty() + && self.stop.is_empty() + } + + pub fn handler_count(&self) -> usize { + [ + &self.pre_tool_use, + &self.post_tool_use, + &self.session_start, + &self.user_prompt_submit, + &self.stop, + ] + .into_iter() + .flatten() + .map(|group| group.hooks.len()) + .sum() + } + + pub fn into_matcher_groups(self) -> [(HookEventName, Vec); 5] { + [ + (HookEventName::PreToolUse, self.pre_tool_use), + (HookEventName::PostToolUse, self.post_tool_use), + (HookEventName::SessionStart, self.session_start), + (HookEventName::UserPromptSubmit, self.user_prompt_submit), + (HookEventName::Stop, self.stop), + ] + } +} + +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct MatcherGroup { + #[serde(default)] + pub matcher: Option, + #[serde(default)] + pub hooks: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(tag = "type")] +pub enum HookHandlerConfig { + #[serde(rename = "command")] + Command { + command: String, + #[serde(default, rename = "timeout", alias = "timeoutSec")] + timeout_sec: Option, + #[serde(default)] + r#async: bool, + #[serde(default, rename = "statusMessage")] + status_message: Option, + }, + #[serde(rename = "prompt")] + Prompt {}, + #[serde(rename = "agent")] + Agent {}, +} + +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ManagedHooksRequirementsToml { + pub managed_dir: Option, + pub windows_managed_dir: Option, + #[serde(flatten)] + pub hooks: HookEventsToml, +} + +impl ManagedHooksRequirementsToml { + pub fn is_empty(&self) -> bool { + self.managed_dir.is_none() && self.windows_managed_dir.is_none() && self.hooks.is_empty() + } + + pub fn handler_count(&self) -> usize { + self.hooks.handler_count() + } + + pub fn managed_dir_for_current_platform(&self) -> Option<&Path> { + #[cfg(windows)] + { + self.windows_managed_dir.as_deref() + } + + #[cfg(not(windows))] + { + self.managed_dir.as_deref() + } + } +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::HookEventsToml; + use super::HooksFile; + use super::ManagedHooksRequirementsToml; + + #[test] + fn hooks_file_deserializes_existing_json_shape() { + let parsed: HooksFile = serde_json::from_str( + r#"{ + "hooks": { + "PreToolUse": [ + { + "matcher": "^Bash$", + "hooks": [ + { + "type": "command", + "command": "python3 /tmp/pre.py", + "timeoutSec": 10, + "statusMessage": "checking" + } + ] + } + ] + } + }"#, + ) + .expect("hooks.json should deserialize"); + + assert_eq!(parsed.hooks.pre_tool_use.len(), 1); + assert_eq!(parsed.hooks.pre_tool_use[0].hooks.len(), 1); + } + + #[test] + fn hook_events_deserialize_from_toml_arrays_of_tables() { + let parsed: HookEventsToml = toml::from_str( + r#" + [[PreToolUse]] + matcher = "^Bash$" + + [[PreToolUse.hooks]] + type = "command" + command = "python3 /tmp/pre.py" + timeoutSec = 10 + statusMessage = "checking" + "#, + ) + .expect("hook events TOML should deserialize"); + + assert_eq!(parsed.pre_tool_use.len(), 1); + assert_eq!(parsed.pre_tool_use[0].hooks.len(), 1); + } + + #[test] + fn managed_hooks_requirements_flatten_hook_events() { + let parsed: ManagedHooksRequirementsToml = toml::from_str( + r#" + managed_dir = "/enterprise/place" + + [[PreToolUse]] + matcher = "^Bash$" + + [[PreToolUse.hooks]] + type = "command" + command = "python3 /enterprise/place/pre.py" + "#, + ) + .expect("requirements hooks TOML should deserialize"); + + assert_eq!( + parsed.managed_dir.as_deref(), + Some(std::path::Path::new("/enterprise/place")) + ); + assert_eq!(parsed.hooks.pre_tool_use.len(), 1); + } +} diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index ffed56198458..a750d5f7dcfe 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -4,6 +4,7 @@ pub mod config_toml; mod constraint; mod diagnostics; mod fingerprint; +mod hook_config; mod marketplace_edit; mod mcp_edit; mod mcp_types; @@ -24,6 +25,8 @@ pub const CONFIG_TOML_FILE: &str = "config.toml"; pub use cloud_requirements::CloudRequirementsLoadError; pub use cloud_requirements::CloudRequirementsLoadErrorCode; pub use cloud_requirements::CloudRequirementsLoader; +pub use codex_app_server_protocol::ConfigLayerSource; +pub use codex_utils_absolute_path::AbsolutePathBuf; pub use config_requirements::AppRequirementToml; pub use config_requirements::AppsRequirementsToml; pub use config_requirements::ConfigRequirements; @@ -59,6 +62,11 @@ pub use diagnostics::format_config_error; 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::HookEventsToml; +pub use hook_config::HookHandlerConfig; +pub use hook_config::HooksFile; +pub use hook_config::ManagedHooksRequirementsToml; +pub use hook_config::MatcherGroup; pub use marketplace_edit::MarketplaceConfigUpdate; pub use marketplace_edit::record_user_marketplace; pub use mcp_edit::ConfigEditsBuilder; @@ -86,3 +94,4 @@ pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::ConfigLayerStackOrdering; pub use state::LoaderOverrides; +pub use toml::Value as TomlValue; diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index bd74c46595e6..802e81df5ebc 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -767,6 +767,110 @@ } ] }, + "HookEventsToml": { + "properties": { + "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": [ + { + "properties": { + "async": { + "default": false, + "type": "boolean" + }, + "command": { + "type": "string" + }, + "statusMessage": { + "default": null, + "type": "string" + }, + "timeout": { + "default": null, + "format": "uint64", + "minimum": 0.0, + "type": "integer" + }, + "type": { + "enum": [ + "command" + ], + "type": "string" + } + }, + "required": [ + "command", + "type" + ], + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "prompt" + ], + "type": "string" + } + }, + "required": [ + "type" + ], + "type": "object" + }, + { + "properties": { + "type": { + "enum": [ + "agent" + ], + "type": "string" + } + }, + "required": [ + "type" + ], + "type": "object" + } + ] + }, "MarketplaceConfig": { "additionalProperties": false, "properties": { @@ -811,6 +915,22 @@ ], "type": "string" }, + "MatcherGroup": { + "properties": { + "hooks": { + "default": [], + "items": { + "$ref": "#/definitions/HookHandlerConfig" + }, + "type": "array" + }, + "matcher": { + "default": null, + "type": "string" + } + }, + "type": "object" + }, "McpServerToolConfig": { "additionalProperties": false, "description": "Per-tool approval settings for a single MCP server tool.", @@ -2425,6 +2545,14 @@ "default": null, "description": "Settings that govern if and what will be written to `~/.codex/history.jsonl`." }, + "hooks": { + "allOf": [ + { + "$ref": "#/definitions/HookEventsToml" + } + ], + "description": "Lifecycle hooks configured inline in TOML." + }, "include_apps_instructions": { "description": "Whether to inject the `` developer block.", "type": "boolean" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 079f0653cd45..cdd88692d6d8 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5097,6 +5097,7 @@ fn test_requirements_web_search_mode_allowlist_does_not_warn_when_unset() -> any crate::config_loader::WebSearchModeRequirement::Cached, ]), feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -5718,6 +5719,7 @@ async fn explicit_sandbox_mode_falls_back_when_disallowed_by_requirements() -> s allowed_sandbox_modes: Some(vec![crate::config_loader::SandboxModeRequirement::ReadOnly]), allowed_web_search_modes: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index a5ddd3e19509..77b0ff382703 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1436,6 +1436,7 @@ impl Config { sandbox_policy: mut constrained_sandbox_policy, web_search_mode: mut constrained_web_search_mode, feature_requirements, + managed_hooks: _, mcp_servers, exec_policy: _, enforce_residency, diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 36fc956bf442..75651f9c3ce2 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -40,7 +40,11 @@ pub use codex_config::ConfigRequirements; pub use codex_config::ConfigRequirementsToml; pub use codex_config::ConstrainedWithSource; pub use codex_config::FeatureRequirementsToml; +pub use codex_config::HookEventsToml; +pub use codex_config::HookHandlerConfig; pub use codex_config::LoaderOverrides; +pub use codex_config::ManagedHooksRequirementsToml; +pub use codex_config::MatcherGroup; pub use codex_config::McpServerIdentity; pub use codex_config::McpServerRequirement; pub use codex_config::NetworkConstraints; diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index ed69682e86de..b16da4b3225a 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -631,6 +631,7 @@ allowed_approval_policies = ["on-request"] allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -683,6 +684,7 @@ allowed_approval_policies = ["on-request"] allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -724,6 +726,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> allowed_sandbox_modes: None, allowed_web_search_modes: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -763,6 +766,59 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> Ok(()) } +#[tokio::test] +async fn load_config_layers_includes_cloud_hook_requirements() -> anyhow::Result<()> { + let tmp = tempdir()?; + let codex_home = tmp.path().join("home"); + tokio::fs::create_dir_all(&codex_home).await?; + let managed_dir = tmp.path().join("managed-hooks"); + tokio::fs::create_dir_all(&managed_dir).await?; + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path())?; + + let requirements = ConfigRequirementsToml { + hooks: Some(codex_config::ManagedHooksRequirementsToml { + managed_dir: Some(managed_dir.clone()), + windows_managed_dir: None, + hooks: codex_config::HookEventsToml { + pre_tool_use: vec![codex_config::MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![codex_config::HookHandlerConfig::Command { + command: format!("python3 {}/pre.py", managed_dir.display()), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + }), + ..ConfigRequirementsToml::default() + }; + let expected = requirements.clone(); + let cloud_requirements = CloudRequirementsLoader::new(async move { Ok(Some(requirements)) }); + + let layers = load_config_layers_state( + &codex_home, + Some(cwd), + &[] as &[(String, TomlValue)], + LoaderOverrides::default(), + cloud_requirements, + ) + .await?; + + assert_eq!(layers.requirements_toml().hooks, expected.hooks); + assert_eq!( + layers + .requirements() + .managed_hooks + .as_ref() + .map(|hooks| hooks.source.clone()), + Some(Some(RequirementSource::CloudRequirements)) + ); + + Ok(()) +} + #[tokio::test] async fn load_config_layers_fails_when_cloud_requirements_loader_fails() -> anyhow::Result<()> { let tmp = tempdir()?; diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index d138f4ed193c..6a7c963f186e 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -599,12 +599,12 @@ impl TestCodexBuilder { ..built_in_model_providers(/*openai_base_url*/ None)["openai"].clone() }; let cwd = Arc::new(TempDir::new()?); - let mut config = load_default_config_for_test(home).await; - config.cwd = cwd_override; - config.model_provider = model_provider; for hook in self.pre_build_hooks.drain(..) { hook(home.path()); } + let mut config = load_default_config_for_test(home).await; + config.cwd = cwd_override; + config.model_provider = model_provider; if let Ok(path) = codex_utils_cargo_bin::cargo_bin("codex") { config.codex_self_exe = Some(path); } else if let Ok(path) = codex_utils_cargo_bin::cargo_bin("codex-exec") { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 47cd251bbfde..f49c377b186f 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -237,6 +237,74 @@ elif mode == "exit_2": Ok(()) } +fn write_pre_tool_use_hook_toml( + home: &Path, + script_name: &str, + log_name: &str, + matcher: Option<&str>, + mode: &str, + reason: &str, +) -> Result<()> { + let script_path = home.join(script_name); + let log_path = home.join(log_name); + let mode_json = serde_json::to_string(mode).context("serialize pre tool use mode")?; + let reason_json = serde_json::to_string(reason).context("serialize pre tool use reason")?; + let script = format!( + r#"import json +from pathlib import Path +import sys + +log_path = Path(r"{log_path}") +mode = {mode_json} +reason = {reason_json} + +payload = json.load(sys.stdin) + +with log_path.open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +if mode == "json_deny": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason + }} + }})) +elif mode == "exit_2": + sys.stderr.write(reason + "\n") + raise SystemExit(2) +"#, + log_path = log_path.display(), + mode_json = mode_json, + reason_json = reason_json, + ); + let matcher_line = matcher + .map(|matcher| format!("matcher = '{matcher}'\n")) + .unwrap_or_default(); + let config_toml = format!( + r#"[features] +codex_hooks = true + +[hooks] + +[[hooks.PreToolUse]] +{matcher_line} + +[[hooks.PreToolUse.hooks]] +type = "command" +command = 'python3 {script_path}' +statusMessage = "running pre tool use hook" +"#, + matcher_line = matcher_line, + script_path = script_path.display(), + ); + + fs::write(&script_path, script).context("write TOML pre tool use hook script")?; + fs::write(home.join("config.toml"), config_toml).context("write config.toml hooks")?; + Ok(()) +} + fn write_post_tool_use_hook( home: &Path, matcher: Option<&str>, @@ -389,20 +457,19 @@ fn read_stop_hook_inputs(home: &Path) -> Result> { } fn read_pre_tool_use_hook_inputs(home: &Path) -> Result> { - fs::read_to_string(home.join("pre_tool_use_hook_log.jsonl")) - .context("read pre tool use hook log")? - .lines() - .filter(|line| !line.trim().is_empty()) - .map(|line| serde_json::from_str(line).context("parse pre tool use hook log line")) - .collect() + read_hook_inputs_from_log(home.join("pre_tool_use_hook_log.jsonl").as_path()) } fn read_post_tool_use_hook_inputs(home: &Path) -> Result> { - fs::read_to_string(home.join("post_tool_use_hook_log.jsonl")) - .context("read post tool use hook log")? + read_hook_inputs_from_log(home.join("post_tool_use_hook_log.jsonl").as_path()) +} + +fn read_hook_inputs_from_log(log_path: &Path) -> Result> { + fs::read_to_string(log_path) + .with_context(|| format!("read hook log {}", log_path.display()))? .lines() .filter(|line| !line.trim().is_empty()) - .map(|line| serde_json::from_str(line).context("parse post tool use hook log line")) + .map(|line| serde_json::from_str(line).context("parse hook log line")) .collect() } @@ -1107,6 +1174,164 @@ async fn pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_blocks_shell_when_defined_in_config_toml() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-config-toml"; + let marker = std::env::temp_dir().join("pretooluse-config-toml-marker"); + let command = format!("printf blocked > {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "shell_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "config.toml hook blocked it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex().with_pre_build_hook(|home| { + if let Err(error) = write_pre_tool_use_hook_toml( + home, + "pre_tool_use_config_hook.py", + "pre_tool_use_config_hook_log.jsonl", + Some("^Bash$"), + "json_deny", + "blocked by config toml hook", + ) { + panic!("failed to write config.toml hook test fixture: {error}"); + } + }); + let test = builder.build(&server).await?; + + if marker.exists() { + fs::remove_file(&marker).context("remove leftover config.toml marker")?; + } + + test.submit_turn_with_policy( + "run the blocked shell command from config toml", + codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("shell command output string"); + assert!( + output.contains("Command blocked by PreToolUse hook: blocked by config toml hook"), + "blocked tool output should surface the config.toml hook reason", + ); + assert!( + !marker.exists(), + "config.toml hook should block command execution" + ); + + let hook_inputs = read_hook_inputs_from_log( + test.codex_home_path() + .join("pre_tool_use_config_hook_log.jsonl") + .as_path(), + )?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["hook_event_name"], "PreToolUse"); + assert_eq!(hook_inputs[0]["tool_use_id"], call_id); + assert_eq!(hook_inputs[0]["tool_input"]["command"], command); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_merges_hooks_json_and_config_toml() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-merged-sources"; + let command = "printf merged-hooks".to_string(); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "shell_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "merged hook context observed"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex().with_pre_build_hook(|home| { + if let Err(error) = write_pre_tool_use_hook(home, Some("^Bash$"), "allow", "unused") { + panic!("failed to write hooks.json hook fixture: {error}"); + } + if let Err(error) = write_pre_tool_use_hook_toml( + home, + "pre_tool_use_toml_hook.py", + "pre_tool_use_toml_hook_log.jsonl", + Some("^Bash$"), + "allow", + "unused", + ) { + panic!("failed to write config.toml hook fixture: {error}"); + } + }); + let test = builder.build(&server).await?; + + test.submit_turn("run the shell command with merged hook sources") + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("shell command output string"); + assert!( + output.contains("merged-hooks"), + "shell command output should still reach the model", + ); + + let json_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + let toml_hook_inputs = read_hook_inputs_from_log( + test.codex_home_path() + .join("pre_tool_use_toml_hook_log.jsonl") + .as_path(), + )?; + assert_eq!(json_hook_inputs.len(), 1); + assert_eq!(toml_hook_inputs.len(), 1); + assert_eq!(json_hook_inputs[0]["tool_use_id"], call_id); + assert_eq!(toml_hook_inputs[0]["tool_use_id"], call_id); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn pre_tool_use_blocks_local_shell_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/engine/config.rs b/codex-rs/hooks/src/engine/config.rs deleted file mode 100644 index 839fee825cfb..000000000000 --- a/codex-rs/hooks/src/engine/config.rs +++ /dev/null @@ -1,48 +0,0 @@ -use serde::Deserialize; - -#[derive(Debug, Default, Deserialize)] -pub(crate) struct HooksFile { - #[serde(default)] - pub hooks: HookEvents, -} - -#[derive(Debug, Default, Deserialize)] -pub(crate) struct HookEvents { - #[serde(rename = "PreToolUse", default)] - pub pre_tool_use: Vec, - #[serde(rename = "PostToolUse", default)] - pub post_tool_use: Vec, - #[serde(rename = "SessionStart", default)] - pub session_start: Vec, - #[serde(rename = "UserPromptSubmit", default)] - pub user_prompt_submit: Vec, - #[serde(rename = "Stop", default)] - pub stop: Vec, -} - -#[derive(Debug, Default, Deserialize)] -pub(crate) struct MatcherGroup { - #[serde(default)] - pub matcher: Option, - #[serde(default)] - pub hooks: Vec, -} - -#[derive(Debug, Clone, Deserialize)] -#[serde(tag = "type")] -pub(crate) enum HookHandlerConfig { - #[serde(rename = "command")] - Command { - command: String, - #[serde(default, rename = "timeout", alias = "timeoutSec")] - timeout_sec: Option, - #[serde(default)] - r#async: bool, - #[serde(default, rename = "statusMessage")] - status_message: Option, - }, - #[serde(rename = "prompt")] - Prompt {}, - #[serde(rename = "agent")] - Agent {}, -} diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 465caa376ddf..41791a076f60 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -1,13 +1,20 @@ use std::fs; use std::path::Path; +use std::path::PathBuf; +use codex_config::CONFIG_TOML_FILE; +use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; +use codex_config::HookEventsToml; +use codex_config::HookHandlerConfig; +use codex_config::HooksFile; +use codex_config::ManagedHooksRequirementsToml; +use codex_config::MatcherGroup; +use codex_config::RequirementSource; +use serde::Deserialize; use super::ConfiguredHandler; -use super::config::HookHandlerConfig; -use super::config::HooksFile; -use super::config::MatcherGroup; use crate::events::common::matcher_pattern_for_event; use crate::events::common::validate_matcher_pattern; @@ -16,6 +23,12 @@ pub(crate) struct DiscoveryResult { pub warnings: Vec, } +#[derive(Clone, Copy)] +struct HookHandlerSource<'a> { + path: &'a Path, + is_managed: bool, +} + pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) -> DiscoveryResult { let Some(config_layer_stack) = config_layer_stack else { return DiscoveryResult { @@ -28,74 +41,55 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - let mut warnings = Vec::new(); let mut display_order = 0_i64; + append_managed_requirement_handlers( + &mut handlers, + &mut warnings, + &mut display_order, + config_layer_stack, + ); + for layer in config_layer_stack.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, /*include_disabled*/ false, ) { - let Some(folder) = layer.config_folder() else { - continue; - }; - let source_path = folder.join("hooks.json"); - if !source_path.as_path().is_file() { - continue; - } + let json_hooks = load_hooks_json(layer.config_folder().as_deref(), &mut warnings); + let toml_hooks = load_toml_hooks_from_layer(layer, &mut warnings); - let contents = match fs::read_to_string(source_path.as_path()) { - Ok(contents) => contents, - Err(err) => { - warnings.push(format!( - "failed to read hooks config {}: {err}", - source_path.display() - )); - continue; - } - }; + if let (Some((json_source_path, json_events)), Some((toml_source_path, toml_events))) = + (&json_hooks, &toml_hooks) + && !json_events.is_empty() + && !toml_events.is_empty() + { + warnings.push(format!( + "loading hooks from both {} and {}; prefer a single representation for this layer", + json_source_path.display(), + toml_source_path.display() + )); + } - let parsed: HooksFile = match serde_json::from_str(&contents) { - Ok(parsed) => parsed, - Err(err) => { - warnings.push(format!( - "failed to parse hooks config {}: {err}", - source_path.display() - )); - continue; - } - }; + if let Some((source_path, hook_events)) = json_hooks { + append_hook_events( + &mut handlers, + &mut warnings, + &mut display_order, + HookHandlerSource { + path: source_path.as_path(), + is_managed: false, + }, + hook_events, + ); + } - let super::config::HookEvents { - pre_tool_use, - post_tool_use, - session_start, - user_prompt_submit, - stop, - } = parsed.hooks; - - for (event_name, groups) in [ - ( - codex_protocol::protocol::HookEventName::PreToolUse, - pre_tool_use, - ), - ( - 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), - ] { - append_matcher_groups( + if let Some((source_path, hook_events)) = toml_hooks { + append_hook_events( &mut handlers, &mut warnings, &mut display_order, - source_path.as_path(), - event_name, - groups, + HookHandlerSource { + path: source_path.as_path(), + is_managed: false, + }, + hook_events, ); } } @@ -103,11 +97,171 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - DiscoveryResult { handlers, warnings } } +fn append_managed_requirement_handlers( + handlers: &mut Vec, + warnings: &mut Vec, + display_order: &mut i64, + config_layer_stack: &ConfigLayerStack, +) { + let Some(managed_hooks) = config_layer_stack.requirements().managed_hooks.as_ref() else { + return; + }; + let Some(source_path) = + managed_hooks_source_path(managed_hooks.get(), managed_hooks.source.as_ref(), warnings) + else { + return; + }; + append_hook_events( + handlers, + warnings, + display_order, + HookHandlerSource { + path: source_path.as_path(), + is_managed: true, + }, + managed_hooks.get().hooks.clone(), + ); +} + +fn managed_hooks_source_path( + managed_hooks: &ManagedHooksRequirementsToml, + requirement_source: Option<&RequirementSource>, + warnings: &mut Vec, +) -> Option { + let source = requirement_source + .map(ToString::to_string) + .unwrap_or_else(|| "managed requirements".to_string()); + let Some(source_path) = managed_hooks.managed_dir_for_current_platform() else { + warnings.push(format!( + "skipping managed hooks from {source}: no managed hook directory is configured for this platform" + )); + return None; + }; + if !source_path.is_absolute() { + warnings.push(format!( + "skipping managed hooks from {source}: managed hook directory {} is not absolute", + source_path.display() + )); + return None; + } + if !source_path.exists() { + warnings.push(format!( + "skipping managed hooks from {source}: managed hook directory {} does not exist", + source_path.display() + )); + return None; + } + if !source_path.is_dir() { + warnings.push(format!( + "skipping managed hooks from {source}: managed hook directory {} is not a directory", + source_path.display() + )); + return None; + } + Some(source_path.to_path_buf()) +} + +fn load_hooks_json( + config_folder: Option<&Path>, + warnings: &mut Vec, +) -> Option<(PathBuf, HookEventsToml)> { + let source_path = config_folder?.join("hooks.json"); + if !source_path.as_path().is_file() { + return None; + } + + let contents = match fs::read_to_string(source_path.as_path()) { + Ok(contents) => contents, + Err(err) => { + warnings.push(format!( + "failed to read hooks config {}: {err}", + source_path.display() + )); + return None; + } + }; + + let parsed: HooksFile = match serde_json::from_str(&contents) { + Ok(parsed) => parsed, + Err(err) => { + warnings.push(format!( + "failed to parse hooks config {}: {err}", + source_path.display() + )); + return None; + } + }; + + (!parsed.hooks.is_empty()).then_some((source_path, parsed.hooks)) +} + +fn load_toml_hooks_from_layer( + layer: &codex_config::ConfigLayerEntry, + warnings: &mut Vec, +) -> Option<(PathBuf, HookEventsToml)> { + let source_path = config_toml_source_path(layer)?; + let hook_value = layer.config.get("hooks")?.clone(); + let parsed = match HookEventsToml::deserialize(hook_value) { + Ok(parsed) => parsed, + Err(err) => { + warnings.push(format!( + "failed to parse TOML hooks in {}: {err}", + source_path.display() + )); + return None; + } + }; + + (!parsed.is_empty()).then_some((source_path, parsed)) +} + +fn config_toml_source_path(layer: &codex_config::ConfigLayerEntry) -> Option { + match &layer.name { + ConfigLayerSource::System { file } + | ConfigLayerSource::User { file } + | ConfigLayerSource::LegacyManagedConfigTomlFromFile { file } => { + Some(file.as_path().to_path_buf()) + } + ConfigLayerSource::Project { dot_codex_folder } => Some( + dot_codex_folder + .join(CONFIG_TOML_FILE) + .as_path() + .to_path_buf(), + ), + ConfigLayerSource::Mdm { domain, key } => Some(PathBuf::from(format!( + "/{CONFIG_TOML_FILE}" + ))), + ConfigLayerSource::LegacyManagedConfigTomlFromMdm => Some(PathBuf::from( + "/managed_config.toml", + )), + ConfigLayerSource::SessionFlags => None, + } +} + +fn append_hook_events( + handlers: &mut Vec, + warnings: &mut Vec, + display_order: &mut i64, + source: HookHandlerSource<'_>, + hook_events: HookEventsToml, +) { + for (event_name, groups) in hook_events.into_matcher_groups() { + append_matcher_groups( + handlers, + warnings, + display_order, + source, + event_name, + groups, + ); + } +} + fn append_group_handlers( handlers: &mut Vec, warnings: &mut Vec, display_order: &mut i64, - source_path: &Path, + source: HookHandlerSource<'_>, event_name: codex_protocol::protocol::HookEventName, matcher: Option<&str>, group_handlers: Vec, @@ -117,7 +271,7 @@ fn append_group_handlers( { warnings.push(format!( "invalid matcher {matcher:?} in {}: {err}", - source_path.display() + source.path.display() )); return; } @@ -133,36 +287,37 @@ fn append_group_handlers( if r#async { warnings.push(format!( "skipping async hook in {}: async hooks are not supported yet", - source_path.display() + source.path.display() )); continue; } if command.trim().is_empty() { warnings.push(format!( "skipping empty hook command in {}", - source_path.display() + source.path.display() )); continue; } let timeout_sec = timeout_sec.unwrap_or(600).max(1); 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.to_path_buf(), + source_path: source.path.to_path_buf(), display_order: *display_order, }); *display_order += 1; } HookHandlerConfig::Prompt {} => warnings.push(format!( "skipping prompt hook in {}: prompt hooks are not supported yet", - source_path.display() + source.path.display() )), HookHandlerConfig::Agent {} => warnings.push(format!( "skipping agent hook in {}: agent hooks are not supported yet", - source_path.display() + source.path.display() )), } } @@ -172,7 +327,7 @@ fn append_matcher_groups( handlers: &mut Vec, warnings: &mut Vec, display_order: &mut i64, - source_path: &Path, + source: HookHandlerSource<'_>, event_name: codex_protocol::protocol::HookEventName, groups: Vec, ) { @@ -181,7 +336,7 @@ fn append_matcher_groups( handlers, warnings, display_order, - source_path, + source, event_name, matcher_pattern_for_event(event_name, group.matcher.as_deref()), group.hooks, @@ -212,7 +367,10 @@ mod tests { &mut handlers, &mut warnings, &mut display_order, - Path::new("/tmp/hooks.json"), + super::HookHandlerSource { + path: Path::new("/tmp/hooks.json"), + is_managed: false, + }, HookEventName::UserPromptSubmit, matcher_pattern_for_event(HookEventName::UserPromptSubmit, Some("[")), vec![HookHandlerConfig::Command { @@ -228,6 +386,7 @@ mod tests { handlers, vec![ConfiguredHandler { event_name: HookEventName::UserPromptSubmit, + is_managed: false, matcher: None, command: "echo hello".to_string(), timeout_sec: 600, @@ -248,7 +407,10 @@ mod tests { &mut handlers, &mut warnings, &mut display_order, - Path::new("/tmp/hooks.json"), + super::HookHandlerSource { + path: Path::new("/tmp/hooks.json"), + is_managed: false, + }, HookEventName::PreToolUse, matcher_pattern_for_event(HookEventName::PreToolUse, Some("^Bash$")), vec![HookHandlerConfig::Command { @@ -264,6 +426,7 @@ 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, @@ -284,7 +447,10 @@ mod tests { &mut handlers, &mut warnings, &mut display_order, - Path::new("/tmp/hooks.json"), + super::HookHandlerSource { + path: Path::new("/tmp/hooks.json"), + is_managed: false, + }, HookEventName::PreToolUse, matcher_pattern_for_event(HookEventName::PreToolUse, Some("*")), vec![HookHandlerConfig::Command { @@ -310,7 +476,10 @@ mod tests { &mut handlers, &mut warnings, &mut display_order, - Path::new("/tmp/hooks.json"), + super::HookHandlerSource { + path: Path::new("/tmp/hooks.json"), + is_managed: false, + }, HookEventName::PostToolUse, matcher_pattern_for_event(HookEventName::PostToolUse, Some("Edit|Write")), vec![HookHandlerConfig::Command { diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index 642231ff1955..419e0e84c941 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -132,6 +132,7 @@ 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 b6ae4a365e07..0bf97df39180 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -1,5 +1,4 @@ pub(crate) mod command_runner; -pub(crate) mod config; pub(crate) mod discovery; pub(crate) mod dispatcher; pub(crate) mod output_parser; @@ -30,6 +29,7 @@ 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, @@ -152,3 +152,304 @@ impl ClaudeHooksEngine { crate::events::stop::run(&self.handlers, &self.shell, request).await } } + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::PathBuf; + + use codex_config::AbsolutePathBuf; + use codex_config::ConfigLayerEntry; + use codex_config::ConfigLayerSource; + use codex_config::ConfigLayerStack; + use codex_config::ConfigRequirements; + use codex_config::ConfigRequirementsToml; + use codex_config::Constrained; + use codex_config::ConstrainedWithSource; + use codex_config::HookEventsToml; + use codex_config::HookHandlerConfig; + use codex_config::ManagedHooksRequirementsToml; + use codex_config::MatcherGroup; + use codex_config::RequirementSource; + use codex_config::TomlValue; + use codex_protocol::ThreadId; + use pretty_assertions::assert_eq; + use tempfile::tempdir; + + use super::ClaudeHooksEngine; + use super::CommandShell; + use crate::events::pre_tool_use::PreToolUseRequest; + + #[tokio::test] + async fn requirements_managed_hooks_execute_from_managed_dir() { + let temp = tempdir().expect("create temp dir"); + let managed_dir = temp.path().join("managed-hooks"); + fs::create_dir_all(&managed_dir).expect("create managed hooks dir"); + let script_path = managed_dir.join("pre_tool_use.py"); + let log_path = managed_dir.join("pre_tool_use_log.jsonl"); + fs::write( + &script_path, + format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +"#, + log_path = log_path.display(), + ), + ) + .expect("write managed hook script"); + + let managed_hooks = ManagedHooksRequirementsToml { + managed_dir: Some(managed_dir.clone()), + windows_managed_dir: None, + hooks: HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: format!("python3 {}", script_path.display()), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + }; + let config_layer_stack = ConfigLayerStack::new( + Vec::new(), + 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), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert!(engine.warnings().is_empty()); + assert_eq!(engine.handlers.len(), 1); + assert!(engine.handlers[0].is_managed); + let preview = engine.preview_pre_tool_use(&PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: PathBuf::from("/tmp"), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }); + assert_eq!(preview.len(), 1); + assert_eq!(preview[0].source_path, managed_dir); + + let outcome = engine + .run_pre_tool_use(PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: PathBuf::from("/tmp"), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }) + .await; + + assert!(!outcome.should_block); + let log_contents = fs::read_to_string(log_path).expect("read managed hook log"); + assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\"")); + } + + #[test] + fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { + let temp = tempdir().expect("create temp dir"); + let missing_dir = temp.path().join("missing-managed-hooks"); + let managed_hooks = ManagedHooksRequirementsToml { + managed_dir: Some(missing_dir.clone()), + windows_managed_dir: None, + hooks: HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: format!("python3 {}", missing_dir.join("pre.py").display()), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + }; + let config_layer_stack = ConfigLayerStack::new( + Vec::new(), + 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), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert!(engine.warnings().iter().any(|warning| { + warning.contains("managed hook directory") + && warning.contains("does not exist") + && warning.contains(&missing_dir.display().to_string()) + })); + assert!( + engine + .preview_pre_tool_use(&PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: PathBuf::from("/tmp"), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }) + .is_empty() + ); + } + + #[test] + fn discovers_hooks_from_json_and_toml_in_the_same_layer() { + let temp = tempdir().expect("create temp dir"); + let config_path = AbsolutePathBuf::try_from(temp.path().join("config.toml")) + .expect("absolute config path"); + let hooks_json_path = temp.path().join("hooks.json"); + fs::write( + &hooks_json_path, + r#"{ + "hooks": { + "PreToolUse": [ + { + "matcher": "^Bash$", + "hooks": [ + { + "type": "command", + "command": "python3 /tmp/json-hook.py" + } + ] + } + ] + } + }"#, + ) + .expect("write hooks.json"); + let mut config_toml = TomlValue::Table(Default::default()); + let TomlValue::Table(config_table) = &mut config_toml else { + unreachable!("config TOML root should be a table"); + }; + let mut hooks_table = TomlValue::Table(Default::default()); + let TomlValue::Table(hooks_entries) = &mut hooks_table else { + unreachable!("hooks entry should be a table"); + }; + let mut pre_tool_use_group = TomlValue::Table(Default::default()); + let TomlValue::Table(pre_tool_use_group_entries) = &mut pre_tool_use_group else { + unreachable!("PreToolUse group should be a table"); + }; + pre_tool_use_group_entries.insert( + "matcher".to_string(), + TomlValue::String("^Bash$".to_string()), + ); + pre_tool_use_group_entries.insert( + "hooks".to_string(), + TomlValue::Array(vec![TomlValue::Table(Default::default())]), + ); + let Some(TomlValue::Array(hooks_array)) = pre_tool_use_group_entries.get_mut("hooks") + else { + unreachable!("PreToolUse hooks should be an array"); + }; + let Some(TomlValue::Table(handler_entries)) = hooks_array.first_mut() else { + unreachable!("PreToolUse handler should be a table"); + }; + handler_entries.insert("type".to_string(), TomlValue::String("command".to_string())); + handler_entries.insert( + "command".to_string(), + TomlValue::String("python3 /tmp/toml-hook.py".to_string()), + ); + hooks_entries.insert( + "PreToolUse".to_string(), + TomlValue::Array(vec![pre_tool_use_group]), + ); + config_table.insert("hooks".to_string(), hooks_table); + let config_layer_stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_path.clone(), + }, + config_toml, + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack"); + + let engine = ClaudeHooksEngine::new( + /*enabled*/ true, + Some(&config_layer_stack), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert!(engine.warnings().iter().any(|warning| { + warning.contains("loading hooks from both") + && warning.contains(&hooks_json_path.display().to_string()) + && warning.contains(&config_path.display().to_string()) + })); + + let preview = engine.preview_pre_tool_use(&PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: PathBuf::from("/tmp"), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }); + assert_eq!(preview.len(), 2); + assert!(engine.handlers.iter().all(|handler| !handler.is_managed)); + assert_eq!(preview[0].source_path, hooks_json_path); + assert_eq!(preview[1].source_path, config_path.as_path()); + } +} diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 9df05338e66a..957fa64d6e26 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -513,6 +513,7 @@ 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 44c9021b998f..3f99f102d788 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -502,6 +502,7 @@ 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 b7be9a9f66e3..93c8e7bc7141 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -355,6 +355,7 @@ 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 5eba1414b919..20bba5a1ba44 100644 --- a/codex-rs/hooks/src/events/stop.rs +++ b/codex-rs/hooks/src/events/stop.rs @@ -522,6 +522,7 @@ 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 b909c183be4d..b16bef1fbcf6 100644 --- a/codex-rs/hooks/src/events/user_prompt_submit.rs +++ b/codex-rs/hooks/src/events/user_prompt_submit.rs @@ -413,6 +413,7 @@ 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/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index aa9526c4ed07..741c10239b45 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -3,6 +3,7 @@ use crate::legacy_core::config::Config; use crate::legacy_core::config_loader::ConfigLayerEntry; use crate::legacy_core::config_loader::ConfigLayerStack; use crate::legacy_core::config_loader::ConfigLayerStackOrdering; +use crate::legacy_core::config_loader::ManagedHooksRequirementsToml; use crate::legacy_core::config_loader::NetworkConstraints; use crate::legacy_core::config_loader::NetworkDomainPermissionToml; use crate::legacy_core::config_loader::NetworkUnixSocketPermissionToml; @@ -160,6 +161,17 @@ fn render_debug_config_lines(stack: &ConfigLayerStack) -> Vec> { )); } + if let Some(hooks) = requirements_toml.hooks.as_ref() { + requirement_lines.push(requirement_line( + "hooks", + format_managed_hooks_requirements(hooks), + requirements + .managed_hooks + .as_ref() + .and_then(|managed_hooks| managed_hooks.source.as_ref()), + )); + } + if let Some(servers) = requirements_toml.mcp_servers.as_ref() { let value = join_or_empty(servers.keys().cloned().collect::>()); requirement_lines.push(requirement_line( @@ -233,6 +245,23 @@ fn render_session_flag_details(config: &TomlValue) -> Vec> { .collect() } +fn format_managed_hooks_requirements(hooks: &ManagedHooksRequirementsToml) -> String { + let mut parts = Vec::new(); + + if let Some(managed_dir) = hooks.managed_dir.as_ref() { + parts.push(format!("managed_dir={}", managed_dir.display())); + } + if let Some(windows_managed_dir) = hooks.windows_managed_dir.as_ref() { + parts.push(format!( + "windows_managed_dir={}", + windows_managed_dir.display() + )); + } + parts.push(format!("handlers={}", hooks.handler_count())); + + join_or_empty(parts) +} + fn render_mdm_layer_details(layer: &ConfigLayerEntry) -> Vec> { let value = layer .raw_toml() @@ -464,6 +493,7 @@ mod tests { use crate::legacy_core::config_loader::ConfigRequirementsToml; use crate::legacy_core::config_loader::ConstrainedWithSource; use crate::legacy_core::config_loader::FeatureRequirementsToml; + use crate::legacy_core::config_loader::ManagedHooksRequirementsToml; use crate::legacy_core::config_loader::McpServerIdentity; use crate::legacy_core::config_loader::McpServerRequirement; use crate::legacy_core::config_loader::NetworkConstraints; @@ -622,6 +652,7 @@ mod tests { feature_requirements: Some(FeatureRequirementsToml { entries: BTreeMap::from([("guardian_approval".to_string(), true)]), }), + hooks: None, mcp_servers: Some(BTreeMap::from([( "docs".to_string(), McpServerRequirement { @@ -813,6 +844,7 @@ approval_policy = "never" allowed_web_search_modes: Some(Vec::new()), guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -829,6 +861,52 @@ approval_policy = "never" ); } + #[test] + fn debug_config_output_lists_managed_hooks_requirement() { + let requirements = ConfigRequirements { + managed_hooks: Some(ConstrainedWithSource::new( + Constrained::allow_any(ManagedHooksRequirementsToml { + managed_dir: Some(if cfg!(windows) { + std::path::PathBuf::from(r"C:\enterprise\hooks") + } else { + std::path::PathBuf::from("/enterprise/hooks") + }), + windows_managed_dir: Some(std::path::PathBuf::from(r"C:\enterprise\hooks")), + hooks: crate::legacy_core::config_loader::HookEventsToml { + pre_tool_use: vec![crate::legacy_core::config_loader::MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![ + crate::legacy_core::config_loader::HookHandlerConfig::Command { + command: "python3 /enterprise/hooks/pre.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }, + ], + }], + ..Default::default() + }, + }), + Some(RequirementSource::CloudRequirements), + )), + ..ConfigRequirements::default() + }; + let requirements_toml = ConfigRequirementsToml { + hooks: requirements + .managed_hooks + .as_ref() + .map(|hooks| hooks.get().clone()), + ..ConfigRequirementsToml::default() + }; + let stack = ConfigLayerStack::new(Vec::new(), requirements, requirements_toml) + .expect("config layer stack"); + + let rendered = render_to_text(&render_debug_config_lines(&stack)); + assert!(rendered.contains("hooks:")); + assert!(rendered.contains("handlers=1")); + assert!(rendered.contains("(source: cloud requirements)")); + } + #[test] fn session_all_proxy_url_uses_socks_when_enabled() { assert_eq!( From 618bd3ddf57395b9d82fa83232adb1a2bd083284 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Tue, 21 Apr 2026 15:18:55 -0700 Subject: [PATCH 02/12] codex: fix CI failure on PR #18893 --- codex-rs/cloud-requirements/src/lib.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index 914958d2c72c..431af31d5c70 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -1168,6 +1168,7 @@ mod tests { allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1199,6 +1200,7 @@ mod tests { allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1230,6 +1232,7 @@ mod tests { allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1278,6 +1281,7 @@ mod tests { allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1362,6 +1366,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1436,6 +1441,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1508,6 +1514,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1707,6 +1714,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1744,6 +1752,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1801,6 +1810,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1853,6 +1863,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1909,6 +1920,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -1966,6 +1978,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -2023,6 +2036,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -2113,6 +2127,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, @@ -2142,6 +2157,7 @@ enabled = false allowed_web_search_modes: None, guardian_policy_config: None, feature_requirements: None, + hooks: None, mcp_servers: None, apps: None, rules: None, From 98ec2f8a99258563f7e09406384e378040b3ffd3 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Tue, 21 Apr 2026 15:51:34 -0700 Subject: [PATCH 03/12] codex: fix windows managed hooks tests --- codex-rs/hooks/src/engine/mod.rs | 38 +++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index e66382b53a53..9332607c81d1 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -201,6 +201,26 @@ mod tests { AbsolutePathBuf::current_dir().expect("current dir") } + fn managed_hooks_for_current_platform>( + managed_dir: P, + hooks: HookEventsToml, + ) -> ManagedHooksRequirementsToml { + let managed_dir = managed_dir.into(); + ManagedHooksRequirementsToml { + managed_dir: if cfg!(windows) { + None + } else { + Some(managed_dir.clone()) + }, + windows_managed_dir: if cfg!(windows) { + Some(managed_dir) + } else { + None + }, + hooks, + } + } + #[tokio::test] async fn requirements_managed_hooks_execute_from_managed_dir() { let temp = tempdir().expect("create temp dir"); @@ -225,10 +245,9 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: ) .expect("write managed hook script"); - let managed_hooks = ManagedHooksRequirementsToml { - managed_dir: Some(managed_dir.clone().into()), - windows_managed_dir: None, - hooks: HookEventsToml { + 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 { @@ -240,7 +259,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: }], ..Default::default() }, - }; + ); let config_layer_stack = ConfigLayerStack::new( Vec::new(), ConfigRequirements { @@ -307,10 +326,9 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { let temp = tempdir().expect("create temp dir"); let missing_dir = temp.path().join("missing-managed-hooks"); - let managed_hooks = ManagedHooksRequirementsToml { - managed_dir: Some(missing_dir.clone()), - windows_managed_dir: None, - hooks: HookEventsToml { + let managed_hooks = managed_hooks_for_current_platform( + missing_dir.clone(), + HookEventsToml { pre_tool_use: vec![MatcherGroup { matcher: Some("^Bash$".to_string()), hooks: vec![HookHandlerConfig::Command { @@ -322,7 +340,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: }], ..Default::default() }, - }; + ); let config_layer_stack = ConfigLayerStack::new( Vec::new(), ConfigRequirements { From 0f2f2b951a6cfa84163a1b77792eeb5efd06e133 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Tue, 21 Apr 2026 17:50:48 -0700 Subject: [PATCH 04/12] app-server: stabilize tracing test on windows --- .../src/message_processor/tracing_tests.rs | 182 +++++++++++------- 1 file changed, 109 insertions(+), 73 deletions(-) diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index bf5986f6de69..fe531fd7cf43 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -48,6 +48,7 @@ use opentelemetry_sdk::trace::SpanData; use pretty_assertions::assert_eq; use serial_test::serial; use std::collections::BTreeMap; +use std::future::Future; use std::path::Path; use std::sync::Arc; use std::sync::OnceLock; @@ -291,6 +292,28 @@ fn build_test_processor( (processor, outgoing_rx) } +fn run_current_thread_test_with_stack(name: &str, future: F) -> Result<()> +where + F: Future> + Send + 'static, +{ + const TEST_STACK_SIZE_BYTES: usize = 4 * 1024 * 1024; + + let handle = std::thread::Builder::new() + .name(name.to_string()) + .stack_size(TEST_STACK_SIZE_BYTES) + .spawn(move || -> Result<()> { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()?; + runtime.block_on(Box::pin(future)) + })?; + + match handle.join() { + Ok(result) => result, + Err(_) => Err(anyhow::anyhow!("{name} thread panicked")), + } +} + fn span_attr<'a>(span: &'a SpanData, key: &str) -> Option<&'a str> { span.attributes .iter() @@ -568,83 +591,96 @@ where spans.into_iter().skip(baseline_len).collect() } -#[tokio::test(flavor = "current_thread")] +#[test] #[serial(app_server_tracing)] -async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { - let mut harness = TracingHarness::new().await?; - - let RemoteTrace { - trace_id: remote_trace_id, - parent_span_id: remote_parent_span_id, - context: remote_trace, - .. - } = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022"); - - let _: ThreadStartResponse = harness - .start_thread(/*request_id*/ 20_002, /*trace*/ None) - .await; - let untraced_spans = wait_for_exported_spans(harness.tracing, |spans| { - spans.iter().any(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.method") == Some("thread/start") - }) - }) - .await; - let untraced_server_span = find_rpc_span_with_trace( - &untraced_spans, - SpanKind::Server, - "thread/start", - untraced_spans - .iter() - .rev() - .find(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.system") == Some("jsonrpc") - && span_attr(span, "rpc.method") == Some("thread/start") +fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { + run_current_thread_test_with_stack( + "thread_start_jsonrpc_span_exports_server_span_and_parents_children", + async { + let mut harness = TracingHarness::new().await?; + + let RemoteTrace { + trace_id: remote_trace_id, + parent_span_id: remote_parent_span_id, + context: remote_trace, + .. + } = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022"); + + let _: ThreadStartResponse = harness + .start_thread(/*request_id*/ 20_002, /*trace*/ None) + .await; + let untraced_spans = wait_for_exported_spans(harness.tracing, |spans| { + spans.iter().any(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.method") == Some("thread/start") + }) }) - .unwrap_or_else(|| { - panic!( - "missing latest thread/start server span; exported spans:\n{}", - format_spans(&untraced_spans) - ) + .await; + let untraced_server_span = find_rpc_span_with_trace( + &untraced_spans, + SpanKind::Server, + "thread/start", + untraced_spans + .iter() + .rev() + .find(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.system") == Some("jsonrpc") + && span_attr(span, "rpc.method") == Some("thread/start") + }) + .unwrap_or_else(|| { + panic!( + "missing latest thread/start server span; exported spans:\n{}", + format_spans(&untraced_spans) + ) + }) + .span_context + .trace_id(), + ); + assert_has_internal_descendant_at_min_depth( + &untraced_spans, + untraced_server_span, + /*min_depth*/ 1, + ); + + let baseline_len = untraced_spans.len(); + let _: ThreadStartResponse = harness + .start_thread(/*request_id*/ 20_003, Some(remote_trace)) + .await; + let spans = wait_for_new_exported_spans(harness.tracing, baseline_len, |spans| { + spans.iter().any(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.method") == Some("thread/start") + && span.span_context.trace_id() == remote_trace_id + }) && spans.iter().any(|span| { + span.name.as_ref() == "app_server.thread_start.notify_started" + && span.span_context.trace_id() == remote_trace_id + }) }) - .span_context - .trace_id(), - ); - assert_has_internal_descendant_at_min_depth( - &untraced_spans, - untraced_server_span, - /*min_depth*/ 1, - ); - - let baseline_len = untraced_spans.len(); - let _: ThreadStartResponse = harness - .start_thread(/*request_id*/ 20_003, Some(remote_trace)) - .await; - let spans = wait_for_new_exported_spans(harness.tracing, baseline_len, |spans| { - spans.iter().any(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.method") == Some("thread/start") - && span.span_context.trace_id() == remote_trace_id - }) && spans.iter().any(|span| { - span.name.as_ref() == "app_server.thread_start.notify_started" - && span.span_context.trace_id() == remote_trace_id - }) - }) - .await; - - let server_request_span = - find_rpc_span_with_trace(&spans, SpanKind::Server, "thread/start", remote_trace_id); - assert_eq!(server_request_span.name.as_ref(), "thread/start"); - assert_eq!(server_request_span.parent_span_id, remote_parent_span_id); - assert!(server_request_span.parent_span_is_remote); - assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id); - assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID); - assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 1); - assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 2); - harness.shutdown().await; + .await; - Ok(()) + let server_request_span = + find_rpc_span_with_trace(&spans, SpanKind::Server, "thread/start", remote_trace_id); + assert_eq!(server_request_span.name.as_ref(), "thread/start"); + assert_eq!(server_request_span.parent_span_id, remote_parent_span_id); + assert!(server_request_span.parent_span_is_remote); + assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id); + assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID); + assert_has_internal_descendant_at_min_depth( + &spans, + server_request_span, + /*min_depth*/ 1, + ); + assert_has_internal_descendant_at_min_depth( + &spans, + server_request_span, + /*min_depth*/ 2, + ); + harness.shutdown().await; + + Ok(()) + }, + ) } #[tokio::test(flavor = "current_thread")] From 98d3656ca0edfee794b5aa95b92ecc4572d264d0 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Tue, 21 Apr 2026 20:43:14 -0700 Subject: [PATCH 05/12] codex: fix CI failure on PR #18893 --- codex-rs/hooks/src/engine/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 9332607c81d1..bd4b619085bd 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -297,6 +297,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: model: "gpt-test".to_string(), permission_mode: "default".to_string(), tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), command: "echo hello".to_string(), }); @@ -312,6 +313,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: model: "gpt-test".to_string(), permission_mode: "default".to_string(), tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), command: "echo hello".to_string(), }) @@ -382,6 +384,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: model: "gpt-test".to_string(), permission_mode: "default".to_string(), tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), command: "echo hello".to_string(), }) @@ -488,6 +491,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: model: "gpt-test".to_string(), permission_mode: "default".to_string(), tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), tool_use_id: "tool-1".to_string(), command: "echo hello".to_string(), }); From b159d96e60d99a119ef66c658e3f99e7b3ab5c63 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 22 Apr 2026 16:05:39 -0700 Subject: [PATCH 06/12] app-server: box tracing wrappers on windows --- .../app-server/src/codex_message_processor.rs | 41 +-- .../src/message_processor/tracing_tests.rs | 301 ++++++++---------- 2 files changed, 153 insertions(+), 189 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 0eb465e54af4..8ce610481aa7 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -868,13 +868,15 @@ impl CodexMessageProcessor { } // === v2 Thread/Turn APIs === ClientRequest::ThreadStart { request_id, params } => { - self.thread_start( + // Keep the large thread-start setup future off this request + // wrapper's stack on Windows. + Box::pin(self.thread_start( to_connection_request_id(request_id), params, app_server_client_name.clone(), app_server_client_version.clone(), request_context, - ) + )) .await; } ClientRequest::ThreadUnsubscribe { request_id, params } => { @@ -2387,24 +2389,23 @@ impl CodexMessageProcessor { }; let request_trace = request_context.request_trace(); let config_manager = self.config_manager.clone(); - let thread_start_task = async move { - Self::thread_start_task( - listener_task_context, - config_manager, - request_id, - app_server_client_name, - app_server_client_version, - config, - typesafe_overrides, - dynamic_tools, - session_start_source, - persist_extended_history, - service_name, - experimental_raw_events, - request_trace, - ) - .await; - }; + // Heap-allocate the spawned task future so constructing it here does + // not inline the full thread-start state machine on the caller stack. + let thread_start_task = Box::pin(Self::thread_start_task( + listener_task_context, + config_manager, + request_id, + app_server_client_name, + app_server_client_version, + config, + typesafe_overrides, + dynamic_tools, + session_start_source, + persist_extended_history, + service_name, + experimental_raw_events, + request_trace, + )); self.background_tasks .spawn(thread_start_task.instrument(request_context.span())); } diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index af519762a439..2263f38bca4c 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -48,7 +48,6 @@ use opentelemetry_sdk::trace::SpanData; use pretty_assertions::assert_eq; use serial_test::serial; use std::collections::BTreeMap; -use std::future::Future; use std::path::Path; use std::sync::Arc; use std::sync::OnceLock; @@ -120,13 +119,13 @@ struct TracingHarness { impl TracingHarness { async fn new() -> Result { - Self::new_with_origin(ConnectionOrigin::WebSocket).await + Box::pin(Self::new_with_origin(ConnectionOrigin::WebSocket)).await } async fn new_with_origin(origin: ConnectionOrigin) -> Result { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - let config = Arc::new(build_test_config(codex_home.path(), &server.uri()).await?); + let config = Arc::new(Box::pin(build_test_config(codex_home.path(), &server.uri())).await?); let (processor, outgoing_rx) = build_test_processor(config); let tracing = init_test_tracing(); tracing.exporter.reset(); @@ -184,14 +183,13 @@ impl TracingHarness { let mut request = request_from_client_request(request); request.trace = trace; - self.processor - .process_request( - TEST_CONNECTION_ID, - request, - AppServerTransport::Stdio, - Arc::clone(&self.session), - ) - .await; + Box::pin(self.processor.process_request( + TEST_CONNECTION_ID, + request, + AppServerTransport::Stdio, + Arc::clone(&self.session), + )) + .await; read_response(&mut self.outgoing_rx, request_id).await } @@ -207,14 +205,13 @@ impl TracingHarness { let mut request = request_from_client_request(request); request.trace = trace; - self.processor - .process_request( - TEST_CONNECTION_ID, - request, - AppServerTransport::Stdio, - Arc::clone(&self.session), - ) - .await; + Box::pin(self.processor.process_request( + TEST_CONNECTION_ID, + request, + AppServerTransport::Stdio, + Arc::clone(&self.session), + )) + .await; read_error(&mut self.outgoing_rx, request_id).await } @@ -223,18 +220,17 @@ impl TracingHarness { request_id: i64, trace: Option, ) -> ThreadStartResponse { - let response = self - .request( - ClientRequest::ThreadStart { - request_id: RequestId::Integer(request_id), - params: ThreadStartParams { - ephemeral: Some(true), - ..ThreadStartParams::default() - }, + let response = Box::pin(self.request( + ClientRequest::ThreadStart { + request_id: RequestId::Integer(request_id), + params: ThreadStartParams { + ephemeral: Some(true), + ..ThreadStartParams::default() }, - trace, - ) - .await; + }, + trace, + )) + .await; read_thread_started_notification(&mut self.outgoing_rx).await; response } @@ -292,28 +288,6 @@ fn build_test_processor( (processor, outgoing_rx) } -fn run_current_thread_test_with_stack(name: &str, future: F) -> Result<()> -where - F: Future> + Send + 'static, -{ - const TEST_STACK_SIZE_BYTES: usize = 4 * 1024 * 1024; - - let handle = std::thread::Builder::new() - .name(name.to_string()) - .stack_size(TEST_STACK_SIZE_BYTES) - .spawn(move || -> Result<()> { - let runtime = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build()?; - runtime.block_on(Box::pin(future)) - })?; - - match handle.join() { - Ok(result) => result, - Err(_) => Err(anyhow::anyhow!("{name} thread panicked")), - } -} - fn span_attr<'a>(span: &'a SpanData, key: &str) -> Option<&'a str> { span.attributes .iter() @@ -591,96 +565,85 @@ where spans.into_iter().skip(baseline_len).collect() } -#[test] +#[tokio::test(flavor = "current_thread")] #[serial(app_server_tracing)] -fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { - run_current_thread_test_with_stack( - "thread_start_jsonrpc_span_exports_server_span_and_parents_children", - async { - let mut harness = TracingHarness::new().await?; - - let RemoteTrace { - trace_id: remote_trace_id, - parent_span_id: remote_parent_span_id, - context: remote_trace, - .. - } = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022"); - - let _: ThreadStartResponse = harness - .start_thread(/*request_id*/ 20_002, /*trace*/ None) - .await; - let untraced_spans = wait_for_exported_spans(harness.tracing, |spans| { - spans.iter().any(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.method") == Some("thread/start") - }) +async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { + let mut harness = Box::pin(TracingHarness::new()).await?; + + let RemoteTrace { + trace_id: remote_trace_id, + parent_span_id: remote_parent_span_id, + context: remote_trace, + .. + } = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022"); + + let _: ThreadStartResponse = + Box::pin(harness.start_thread(/*request_id*/ 20_002, /*trace*/ None)).await; + let untraced_spans = Box::pin(wait_for_exported_spans(harness.tracing, |spans| { + spans.iter().any(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.method") == Some("thread/start") + }) + })) + .await; + let untraced_server_span = find_rpc_span_with_trace( + &untraced_spans, + SpanKind::Server, + "thread/start", + untraced_spans + .iter() + .rev() + .find(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.system") == Some("jsonrpc") + && span_attr(span, "rpc.method") == Some("thread/start") }) - .await; - let untraced_server_span = find_rpc_span_with_trace( - &untraced_spans, - SpanKind::Server, - "thread/start", - untraced_spans - .iter() - .rev() - .find(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.system") == Some("jsonrpc") - && span_attr(span, "rpc.method") == Some("thread/start") - }) - .unwrap_or_else(|| { - panic!( - "missing latest thread/start server span; exported spans:\n{}", - format_spans(&untraced_spans) - ) - }) - .span_context - .trace_id(), - ); - assert_has_internal_descendant_at_min_depth( - &untraced_spans, - untraced_server_span, - /*min_depth*/ 1, - ); - - let baseline_len = untraced_spans.len(); - let _: ThreadStartResponse = harness - .start_thread(/*request_id*/ 20_003, Some(remote_trace)) - .await; - let spans = wait_for_new_exported_spans(harness.tracing, baseline_len, |spans| { - spans.iter().any(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.method") == Some("thread/start") - && span.span_context.trace_id() == remote_trace_id - }) && spans.iter().any(|span| { - span.name.as_ref() == "app_server.thread_start.notify_started" - && span.span_context.trace_id() == remote_trace_id - }) + .unwrap_or_else(|| { + panic!( + "missing latest thread/start server span; exported spans:\n{}", + format_spans(&untraced_spans) + ) }) - .await; + .span_context + .trace_id(), + ); + assert_has_internal_descendant_at_min_depth( + &untraced_spans, + untraced_server_span, + /*min_depth*/ 1, + ); - let server_request_span = - find_rpc_span_with_trace(&spans, SpanKind::Server, "thread/start", remote_trace_id); - assert_eq!(server_request_span.name.as_ref(), "thread/start"); - assert_eq!(server_request_span.parent_span_id, remote_parent_span_id); - assert!(server_request_span.parent_span_is_remote); - assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id); - assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID); - assert_has_internal_descendant_at_min_depth( - &spans, - server_request_span, - /*min_depth*/ 1, - ); - assert_has_internal_descendant_at_min_depth( - &spans, - server_request_span, - /*min_depth*/ 2, - ); - harness.shutdown().await; - - Ok(()) + let baseline_len = untraced_spans.len(); + let _: ThreadStartResponse = + Box::pin(harness.start_thread(/*request_id*/ 20_003, Some(remote_trace))).await; + let spans = Box::pin(wait_for_new_exported_spans( + harness.tracing, + baseline_len, + |spans| { + spans.iter().any(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.method") == Some("thread/start") + && span.span_context.trace_id() == remote_trace_id + }) && spans.iter().any(|span| { + span.name.as_ref() == "app_server.thread_start.notify_started" + && span.span_context.trace_id() == remote_trace_id + }) }, - ) + )) + .await; + + let server_request_span = + find_rpc_span_with_trace(&spans, SpanKind::Server, "thread/start", remote_trace_id); + assert_eq!(server_request_span.name.as_ref(), "thread/start"); + assert_eq!(server_request_span.parent_span_id, remote_parent_span_id); + assert!(server_request_span.parent_span_is_remote); + assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id); + assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID); + assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 1); + assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 2); + Box::pin(harness.shutdown()).await; + + Ok(()) } #[tokio::test(flavor = "current_thread")] @@ -727,8 +690,9 @@ async fn remote_control_origin_rejects_device_key_requests() -> Result<()> { #[tokio::test(flavor = "current_thread")] #[serial(app_server_tracing)] async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { - let mut harness = TracingHarness::new().await?; - let thread_start_response = harness.start_thread(/*request_id*/ 2, /*trace*/ None).await; + let mut harness = Box::pin(TracingHarness::new()).await?; + let thread_start_response = + Box::pin(harness.start_thread(/*request_id*/ 2, /*trace*/ None)).await; let thread_id = thread_start_response.thread.id.clone(); harness.reset_tracing(); @@ -738,35 +702,34 @@ async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { parent_span_id: remote_parent_span_id, context: remote_trace, } = RemoteTrace::new("00000000000000000000000000000077", "0000000000000088"); - let turn_start_response: TurnStartResponse = harness - .request( - ClientRequest::TurnStart { - request_id: RequestId::Integer(3), - params: TurnStartParams { - environments: None, - thread_id, - input: vec![UserInput::Text { - text: "hello".to_string(), - text_elements: Vec::new(), - }], - responsesapi_client_metadata: None, - cwd: None, - approval_policy: None, - sandbox_policy: None, - approvals_reviewer: None, - model: None, - service_tier: None, - effort: None, - summary: None, - personality: None, - output_schema: None, - collaboration_mode: None, - }, + let turn_start_response: TurnStartResponse = Box::pin(harness.request( + ClientRequest::TurnStart { + request_id: RequestId::Integer(3), + params: TurnStartParams { + environments: None, + thread_id, + input: vec![UserInput::Text { + text: "hello".to_string(), + text_elements: Vec::new(), + }], + responsesapi_client_metadata: None, + cwd: None, + approval_policy: None, + sandbox_policy: None, + approvals_reviewer: None, + model: None, + service_tier: None, + effort: None, + summary: None, + personality: None, + output_schema: None, + collaboration_mode: None, }, - Some(remote_trace), - ) - .await; - let spans = wait_for_exported_spans(harness.tracing, |spans| { + }, + Some(remote_trace), + )) + .await; + let spans = Box::pin(wait_for_exported_spans(harness.tracing, |spans| { spans.iter().any(|span| { span.span_kind == SpanKind::Server && span_attr(span, "rpc.method") == Some("turn/start") @@ -775,7 +738,7 @@ async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { span_attr(span, "codex.op") == Some("user_input") && span.span_context.trace_id() == remote_trace_id }) - }) + })) .await; let server_request_span = @@ -793,7 +756,7 @@ async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { Some(turn_start_response.turn.id.as_str()) ); assert_span_descends_from(&spans, core_turn_span, server_request_span); - harness.shutdown().await; + Box::pin(harness.shutdown()).await; Ok(()) } From 1add3886735d0f7cb16f333e31c6971f665eb825 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 22 Apr 2026 16:19:07 -0700 Subject: [PATCH 07/12] codex: address hook config review nits --- codex-rs/config/src/config_requirements.rs | 60 +++++----- codex-rs/config/src/hook_config.rs | 128 ++++++--------------- codex-rs/config/src/hooks_tests.rs | 119 +++++++++++++++++++ 3 files changed, 186 insertions(+), 121 deletions(-) create mode 100644 codex-rs/config/src/hooks_tests.rs diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index 1c906020b714..c921990239cc 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -2285,17 +2285,17 @@ allowed_approvals_reviewers = ["user"] #[test] fn deserialize_managed_hooks_requirements() -> Result<()> { let toml_str = r#" - managed_dir = "/enterprise/hooks" - windows_managed_dir = 'C:\enterprise\hooks' +managed_dir = "/enterprise/hooks" +windows_managed_dir = 'C:\enterprise\hooks' - [[PreToolUse]] - matcher = "^Bash$" +[[PreToolUse]] +matcher = "^Bash$" - [[PreToolUse.hooks]] - type = "command" - command = "python3 /enterprise/hooks/pre.py" - timeoutSec = 10 - statusMessage = "checking" +[[PreToolUse.hooks]] +type = "command" +command = "python3 /enterprise/hooks/pre.py" +timeout = 10 +statusMessage = "checking" "#; let hooks: ManagedHooksRequirementsToml = from_str(toml_str)?; @@ -2315,15 +2315,15 @@ allowed_approvals_reviewers = ["user"] RequirementSource::CloudRequirements, from_str::( r#" - [hooks] - managed_dir = "/cloud/hooks" +[hooks] +managed_dir = "/cloud/hooks" - [[hooks.PreToolUse]] - matcher = "^Bash$" +[[hooks.PreToolUse]] +matcher = "^Bash$" - [[hooks.PreToolUse.hooks]] - type = "command" - command = "python3 /cloud/hooks/pre.py" +[[hooks.PreToolUse.hooks]] +type = "command" +command = "python3 /cloud/hooks/pre.py" "#, )?, ); @@ -2333,15 +2333,15 @@ allowed_approvals_reviewers = ["user"] }, from_str::( r#" - [hooks] - managed_dir = "/system/hooks" +[hooks] +managed_dir = "/system/hooks" - [[hooks.PreToolUse]] - matcher = "^Bash$" +[[hooks.PreToolUse]] +matcher = "^Bash$" - [[hooks.PreToolUse.hooks]] - type = "command" - command = "python3 /system/hooks/pre.py" +[[hooks.PreToolUse.hooks]] +type = "command" +command = "python3 /system/hooks/pre.py" "#, )?, ); @@ -2365,15 +2365,15 @@ allowed_approvals_reviewers = ["user"] fn managed_hooks_constraint_rejects_drift() -> Result<()> { let config: ConfigRequirementsToml = from_str( r#" - [hooks] - managed_dir = "/enterprise/hooks" +[hooks] +managed_dir = "/enterprise/hooks" - [[hooks.PreToolUse]] - matcher = "^Bash$" +[[hooks.PreToolUse]] +matcher = "^Bash$" - [[hooks.PreToolUse.hooks]] - type = "command" - command = "python3 /enterprise/hooks/pre.py" +[[hooks.PreToolUse.hooks]] +type = "command" +command = "python3 /enterprise/hooks/pre.py" "#, )?; let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs index 87f9dd415731..8a5c73d6b9ba 100644 --- a/codex-rs/config/src/hook_config.rs +++ b/codex-rs/config/src/hook_config.rs @@ -30,22 +30,38 @@ pub struct HookEventsToml { impl HookEventsToml { pub fn is_empty(&self) -> bool { - self.pre_tool_use.is_empty() - && self.permission_request.is_empty() - && self.post_tool_use.is_empty() - && self.session_start.is_empty() - && self.user_prompt_submit.is_empty() - && self.stop.is_empty() + let Self { + pre_tool_use, + permission_request, + post_tool_use, + session_start, + user_prompt_submit, + stop, + } = self; + pre_tool_use.is_empty() + && permission_request.is_empty() + && post_tool_use.is_empty() + && session_start.is_empty() + && user_prompt_submit.is_empty() + && stop.is_empty() } pub fn handler_count(&self) -> usize { + let Self { + pre_tool_use, + permission_request, + post_tool_use, + session_start, + user_prompt_submit, + stop, + } = self; [ - &self.pre_tool_use, - &self.permission_request, - &self.post_tool_use, - &self.session_start, - &self.user_prompt_submit, - &self.stop, + pre_tool_use, + permission_request, + post_tool_use, + session_start, + user_prompt_submit, + stop, ] .into_iter() .flatten() @@ -79,7 +95,7 @@ pub enum HookHandlerConfig { #[serde(rename = "command")] Command { command: String, - #[serde(default, rename = "timeout", alias = "timeoutSec")] + #[serde(default, rename = "timeout")] timeout_sec: Option, #[serde(default)] r#async: bool, @@ -102,7 +118,12 @@ pub struct ManagedHooksRequirementsToml { impl ManagedHooksRequirementsToml { pub fn is_empty(&self) -> bool { - self.managed_dir.is_none() && self.windows_managed_dir.is_none() && self.hooks.is_empty() + let Self { + managed_dir, + windows_managed_dir, + hooks, + } = self; + managed_dir.is_none() && windows_managed_dir.is_none() && hooks.is_empty() } pub fn handler_count(&self) -> usize { @@ -123,80 +144,5 @@ impl ManagedHooksRequirementsToml { } #[cfg(test)] -mod tests { - use pretty_assertions::assert_eq; - - use super::HookEventsToml; - use super::HooksFile; - use super::ManagedHooksRequirementsToml; - - #[test] - fn hooks_file_deserializes_existing_json_shape() { - let parsed: HooksFile = serde_json::from_str( - r#"{ - "hooks": { - "PreToolUse": [ - { - "matcher": "^Bash$", - "hooks": [ - { - "type": "command", - "command": "python3 /tmp/pre.py", - "timeoutSec": 10, - "statusMessage": "checking" - } - ] - } - ] - } - }"#, - ) - .expect("hooks.json should deserialize"); - - assert_eq!(parsed.hooks.pre_tool_use.len(), 1); - assert_eq!(parsed.hooks.pre_tool_use[0].hooks.len(), 1); - } - - #[test] - fn hook_events_deserialize_from_toml_arrays_of_tables() { - let parsed: HookEventsToml = toml::from_str( - r#" - [[PreToolUse]] - matcher = "^Bash$" - - [[PreToolUse.hooks]] - type = "command" - command = "python3 /tmp/pre.py" - timeoutSec = 10 - statusMessage = "checking" - "#, - ) - .expect("hook events TOML should deserialize"); - - assert_eq!(parsed.pre_tool_use.len(), 1); - assert_eq!(parsed.pre_tool_use[0].hooks.len(), 1); - } - - #[test] - fn managed_hooks_requirements_flatten_hook_events() { - let parsed: ManagedHooksRequirementsToml = toml::from_str( - r#" - managed_dir = "/enterprise/place" - - [[PreToolUse]] - matcher = "^Bash$" - - [[PreToolUse.hooks]] - type = "command" - command = "python3 /enterprise/place/pre.py" - "#, - ) - .expect("requirements hooks TOML should deserialize"); - - assert_eq!( - parsed.managed_dir.as_deref(), - Some(std::path::Path::new("/enterprise/place")) - ); - assert_eq!(parsed.hooks.pre_tool_use.len(), 1); - } -} +#[path = "hooks_tests.rs"] +mod tests; diff --git a/codex-rs/config/src/hooks_tests.rs b/codex-rs/config/src/hooks_tests.rs new file mode 100644 index 000000000000..5e3f1df67475 --- /dev/null +++ b/codex-rs/config/src/hooks_tests.rs @@ -0,0 +1,119 @@ +use pretty_assertions::assert_eq; + +use super::HookEventsToml; +use super::HookHandlerConfig; +use super::HooksFile; +use super::ManagedHooksRequirementsToml; +use super::MatcherGroup; + +#[test] +fn hooks_file_deserializes_existing_json_shape() { + let parsed: HooksFile = serde_json::from_str( + r#"{ + "hooks": { + "PreToolUse": [ + { + "matcher": "^Bash$", + "hooks": [ + { + "type": "command", + "command": "python3 /tmp/pre.py", + "timeout": 10, + "statusMessage": "checking" + } + ] + } + ] + } +}"#, + ) + .expect("hooks.json should deserialize"); + + assert_eq!( + parsed, + HooksFile { + hooks: HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "python3 /tmp/pre.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + } + ); +} + +#[test] +fn hook_events_deserialize_from_toml_arrays_of_tables() { + let parsed: HookEventsToml = toml::from_str( + r#" +[[PreToolUse]] +matcher = "^Bash$" + +[[PreToolUse.hooks]] +type = "command" +command = "python3 /tmp/pre.py" +timeout = 10 +statusMessage = "checking" +"#, + ) + .expect("hook events TOML should deserialize"); + + assert_eq!( + parsed, + HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "python3 /tmp/pre.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + } + ); +} + +#[test] +fn managed_hooks_requirements_flatten_hook_events() { + let parsed: ManagedHooksRequirementsToml = toml::from_str( + r#" +managed_dir = "/enterprise/place" + +[[PreToolUse]] +matcher = "^Bash$" + +[[PreToolUse.hooks]] +type = "command" +command = "python3 /enterprise/place/pre.py" +"#, + ) + .expect("requirements hooks TOML should deserialize"); + + assert_eq!( + parsed, + ManagedHooksRequirementsToml { + managed_dir: Some(std::path::PathBuf::from("/enterprise/place")), + windows_managed_dir: None, + hooks: HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "python3 /enterprise/place/pre.py".to_string(), + timeout_sec: None, + r#async: false, + status_message: None, + }], + }], + ..Default::default() + }, + } + ); +} From 4bb233c546f6c093ae748be310e5403ac3bd373a Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 22 Apr 2026 16:24:59 -0700 Subject: [PATCH 08/12] codex: tighten merged hook input assertion --- codex-rs/core/tests/suite/hooks.rs | 38 +++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 8afc82cc7d01..ae9df5229606 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -1937,16 +1937,42 @@ async fn pre_tool_use_merges_hooks_json_and_config_toml() -> Result<()> { "shell command output should still reach the model", ); - let json_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + let json_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())? + .into_iter() + .map(|hook_input| { + serde_json::json!({ + "hook_event_name": hook_input["hook_event_name"], + "tool_name": hook_input["tool_name"], + "tool_use_id": hook_input["tool_use_id"], + "tool_input": hook_input["tool_input"], + }) + }) + .collect::>(); let toml_hook_inputs = read_hook_inputs_from_log( test.codex_home_path() .join("pre_tool_use_toml_hook_log.jsonl") .as_path(), - )?; - assert_eq!(json_hook_inputs.len(), 1); - assert_eq!(toml_hook_inputs.len(), 1); - assert_eq!(json_hook_inputs[0]["tool_use_id"], call_id); - assert_eq!(toml_hook_inputs[0]["tool_use_id"], call_id); + )? + .into_iter() + .map(|hook_input| { + serde_json::json!({ + "hook_event_name": hook_input["hook_event_name"], + "tool_name": hook_input["tool_name"], + "tool_use_id": hook_input["tool_use_id"], + "tool_input": hook_input["tool_input"], + }) + }) + .collect::>(); + let expected_hook_inputs = vec![serde_json::json!({ + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_use_id": call_id, + "tool_input": { + "command": command, + }, + })]; + assert_eq!(expected_hook_inputs, json_hook_inputs); + assert_eq!(expected_hook_inputs, toml_hook_inputs); Ok(()) } From 02f21ba030456477049a27fccdaccd55cbec6047 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 22 Apr 2026 16:50:37 -0700 Subject: [PATCH 09/12] codex: address hooks engine review nits --- codex-rs/hooks/src/engine/mod.rs | 331 +------------------------ codex-rs/hooks/src/engine/mod_tests.rs | 327 ++++++++++++++++++++++++ 2 files changed, 329 insertions(+), 329 deletions(-) create mode 100644 codex-rs/hooks/src/engine/mod_tests.rs diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index bd4b619085bd..3bfb17f6d6f0 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -172,332 +172,5 @@ impl ClaudeHooksEngine { } #[cfg(test)] -mod tests { - use std::fs; - - use codex_config::AbsolutePathBuf; - use codex_config::ConfigLayerEntry; - use codex_config::ConfigLayerSource; - use codex_config::ConfigLayerStack; - use codex_config::ConfigRequirements; - use codex_config::ConfigRequirementsToml; - use codex_config::Constrained; - use codex_config::ConstrainedWithSource; - use codex_config::HookEventsToml; - use codex_config::HookHandlerConfig; - use codex_config::ManagedHooksRequirementsToml; - use codex_config::MatcherGroup; - use codex_config::RequirementSource; - use codex_config::TomlValue; - use codex_protocol::ThreadId; - use pretty_assertions::assert_eq; - use tempfile::tempdir; - - use super::ClaudeHooksEngine; - use super::CommandShell; - use crate::events::pre_tool_use::PreToolUseRequest; - - fn cwd() -> AbsolutePathBuf { - AbsolutePathBuf::current_dir().expect("current dir") - } - - fn managed_hooks_for_current_platform>( - managed_dir: P, - hooks: HookEventsToml, - ) -> ManagedHooksRequirementsToml { - let managed_dir = managed_dir.into(); - ManagedHooksRequirementsToml { - managed_dir: if cfg!(windows) { - None - } else { - Some(managed_dir.clone()) - }, - windows_managed_dir: if cfg!(windows) { - Some(managed_dir) - } else { - None - }, - hooks, - } - } - - #[tokio::test] - async fn requirements_managed_hooks_execute_from_managed_dir() { - 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 script_path = managed_dir.join("pre_tool_use.py"); - let log_path = managed_dir.join("pre_tool_use_log.jsonl"); - fs::write( - script_path.as_path(), - format!( - r#"import json -from pathlib import Path -import sys - -payload = json.load(sys.stdin) -with Path(r"{log_path}").open("a", encoding="utf-8") as handle: - handle.write(json.dumps(payload) + "\n") -"#, - log_path = log_path.display(), - ), - ) - .expect("write managed hook script"); - - 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: format!("python3 {}", script_path.display()), - timeout_sec: Some(10), - r#async: false, - status_message: Some("checking".to_string()), - }], - }], - ..Default::default() - }, - ); - let config_layer_stack = ConfigLayerStack::new( - Vec::new(), - 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), - CommandShell { - program: String::new(), - args: Vec::new(), - }, - ); - - assert!(engine.warnings().is_empty()); - assert_eq!(engine.handlers.len(), 1); - assert!(engine.handlers[0].is_managed); - let cwd = cwd(); - let preview = engine.preview_pre_tool_use(&PreToolUseRequest { - session_id: ThreadId::new(), - turn_id: "turn-1".to_string(), - cwd: cwd.clone(), - transcript_path: None, - model: "gpt-test".to_string(), - permission_mode: "default".to_string(), - tool_name: "Bash".to_string(), - matcher_aliases: Vec::new(), - tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), - }); - assert_eq!(preview.len(), 1); - assert_eq!(preview[0].source_path, managed_dir); - - let outcome = engine - .run_pre_tool_use(PreToolUseRequest { - session_id: ThreadId::new(), - turn_id: "turn-1".to_string(), - cwd, - transcript_path: None, - model: "gpt-test".to_string(), - permission_mode: "default".to_string(), - tool_name: "Bash".to_string(), - matcher_aliases: Vec::new(), - tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), - }) - .await; - - assert!(!outcome.should_block); - let log_contents = fs::read_to_string(log_path).expect("read managed hook log"); - assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\"")); - } - - #[test] - fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { - let temp = tempdir().expect("create temp dir"); - let missing_dir = temp.path().join("missing-managed-hooks"); - let managed_hooks = managed_hooks_for_current_platform( - missing_dir.clone(), - HookEventsToml { - pre_tool_use: vec![MatcherGroup { - matcher: Some("^Bash$".to_string()), - hooks: vec![HookHandlerConfig::Command { - command: format!("python3 {}", missing_dir.join("pre.py").display()), - timeout_sec: Some(10), - r#async: false, - status_message: Some("checking".to_string()), - }], - }], - ..Default::default() - }, - ); - let config_layer_stack = ConfigLayerStack::new( - Vec::new(), - 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), - CommandShell { - program: String::new(), - args: Vec::new(), - }, - ); - - assert!(engine.warnings().iter().any(|warning| { - warning.contains("managed hook directory") - && warning.contains("does not exist") - && warning.contains(&missing_dir.display().to_string()) - })); - let cwd = cwd(); - assert!( - engine - .preview_pre_tool_use(&PreToolUseRequest { - session_id: ThreadId::new(), - turn_id: "turn-1".to_string(), - cwd, - transcript_path: None, - model: "gpt-test".to_string(), - permission_mode: "default".to_string(), - tool_name: "Bash".to_string(), - matcher_aliases: Vec::new(), - tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), - }) - .is_empty() - ); - } - - #[test] - fn discovers_hooks_from_json_and_toml_in_the_same_layer() { - let temp = tempdir().expect("create temp dir"); - let config_path = AbsolutePathBuf::try_from(temp.path().join("config.toml")) - .expect("absolute config path"); - let hooks_json_path = - AbsolutePathBuf::try_from(temp.path().join("hooks.json")).expect("absolute hooks path"); - fs::write( - hooks_json_path.as_path(), - r#"{ - "hooks": { - "PreToolUse": [ - { - "matcher": "^Bash$", - "hooks": [ - { - "type": "command", - "command": "python3 /tmp/json-hook.py" - } - ] - } - ] - } - }"#, - ) - .expect("write hooks.json"); - let mut config_toml = TomlValue::Table(Default::default()); - let TomlValue::Table(config_table) = &mut config_toml else { - unreachable!("config TOML root should be a table"); - }; - let mut hooks_table = TomlValue::Table(Default::default()); - let TomlValue::Table(hooks_entries) = &mut hooks_table else { - unreachable!("hooks entry should be a table"); - }; - let mut pre_tool_use_group = TomlValue::Table(Default::default()); - let TomlValue::Table(pre_tool_use_group_entries) = &mut pre_tool_use_group else { - unreachable!("PreToolUse group should be a table"); - }; - pre_tool_use_group_entries.insert( - "matcher".to_string(), - TomlValue::String("^Bash$".to_string()), - ); - pre_tool_use_group_entries.insert( - "hooks".to_string(), - TomlValue::Array(vec![TomlValue::Table(Default::default())]), - ); - let Some(TomlValue::Array(hooks_array)) = pre_tool_use_group_entries.get_mut("hooks") - else { - unreachable!("PreToolUse hooks should be an array"); - }; - let Some(TomlValue::Table(handler_entries)) = hooks_array.first_mut() else { - unreachable!("PreToolUse handler should be a table"); - }; - handler_entries.insert("type".to_string(), TomlValue::String("command".to_string())); - handler_entries.insert( - "command".to_string(), - TomlValue::String("python3 /tmp/toml-hook.py".to_string()), - ); - hooks_entries.insert( - "PreToolUse".to_string(), - TomlValue::Array(vec![pre_tool_use_group]), - ); - config_table.insert("hooks".to_string(), hooks_table); - let config_layer_stack = ConfigLayerStack::new( - vec![ConfigLayerEntry::new( - ConfigLayerSource::User { - file: config_path.clone(), - }, - config_toml, - )], - ConfigRequirements::default(), - ConfigRequirementsToml::default(), - ) - .expect("config layer stack"); - - let engine = ClaudeHooksEngine::new( - /*enabled*/ true, - Some(&config_layer_stack), - CommandShell { - program: String::new(), - args: Vec::new(), - }, - ); - - assert!(engine.warnings().iter().any(|warning| { - warning.contains("loading hooks from both") - && warning.contains(&hooks_json_path.display().to_string()) - && warning.contains(&config_path.display().to_string()) - })); - - let cwd = cwd(); - let preview = engine.preview_pre_tool_use(&PreToolUseRequest { - session_id: ThreadId::new(), - turn_id: "turn-1".to_string(), - cwd, - transcript_path: None, - model: "gpt-test".to_string(), - permission_mode: "default".to_string(), - tool_name: "Bash".to_string(), - matcher_aliases: Vec::new(), - tool_use_id: "tool-1".to_string(), - command: "echo hello".to_string(), - }); - assert_eq!(preview.len(), 2); - assert!(engine.handlers.iter().all(|handler| !handler.is_managed)); - assert_eq!(preview[0].source_path, hooks_json_path); - assert_eq!(preview[1].source_path, config_path); - } -} +#[path = "mod_tests.rs"] +mod tests; diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs new file mode 100644 index 000000000000..527574b500a7 --- /dev/null +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -0,0 +1,327 @@ +use std::fs; +use std::path::Path; + +use codex_config::AbsolutePathBuf; +use codex_config::ConfigLayerEntry; +use codex_config::ConfigLayerSource; +use codex_config::ConfigLayerStack; +use codex_config::ConfigRequirements; +use codex_config::ConfigRequirementsToml; +use codex_config::Constrained; +use codex_config::ConstrainedWithSource; +use codex_config::HookEventsToml; +use codex_config::HookHandlerConfig; +use codex_config::ManagedHooksRequirementsToml; +use codex_config::MatcherGroup; +use codex_config::RequirementSource; +use codex_config::TomlValue; +use codex_protocol::ThreadId; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +use super::ClaudeHooksEngine; +use super::CommandShell; +use crate::events::pre_tool_use::PreToolUseRequest; + +fn cwd() -> AbsolutePathBuf { + AbsolutePathBuf::current_dir().expect("current dir") +} + +fn managed_hooks_for_current_platform( + managed_dir: impl AsRef, + hooks: HookEventsToml, +) -> ManagedHooksRequirementsToml { + let managed_dir = managed_dir.as_ref().to_path_buf(); + ManagedHooksRequirementsToml { + managed_dir: if cfg!(windows) { + None + } else { + Some(managed_dir.clone()) + }, + windows_managed_dir: if cfg!(windows) { + Some(managed_dir) + } else { + None + }, + hooks, + } +} + +#[tokio::test] +async fn requirements_managed_hooks_execute_from_managed_dir() { + 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 script_path = managed_dir.join("pre_tool_use.py"); + let log_path = managed_dir.join("pre_tool_use_log.jsonl"); + fs::write( + script_path.as_path(), + format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +"#, + log_path = log_path.display(), + ), + ) + .expect("write managed hook script"); + + 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: format!("python3 {}", script_path.display()), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + ); + let config_layer_stack = ConfigLayerStack::new( + Vec::new(), + 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), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert!(engine.warnings().is_empty()); + assert_eq!(engine.handlers.len(), 1); + assert!(engine.handlers[0].is_managed); + let cwd = cwd(); + let preview = engine.preview_pre_tool_use(&PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: cwd.clone(), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }); + assert_eq!(preview.len(), 1); + assert_eq!(preview[0].source_path, managed_dir); + + let outcome = engine + .run_pre_tool_use(PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd, + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }) + .await; + + assert!(!outcome.should_block); + let log_contents = fs::read_to_string(log_path).expect("read managed hook log"); + assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\"")); +} + +#[test] +fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { + let temp = tempdir().expect("create temp dir"); + let missing_dir = temp.path().join("missing-managed-hooks"); + let managed_hooks = managed_hooks_for_current_platform( + missing_dir.clone(), + HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: format!("python3 {}", missing_dir.join("pre.py").display()), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + ); + let config_layer_stack = ConfigLayerStack::new( + Vec::new(), + 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), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert!(engine.warnings().iter().any(|warning| { + warning.contains("managed hook directory") + && warning.contains("does not exist") + && warning.contains(&missing_dir.display().to_string()) + })); + let cwd = cwd(); + assert!( + engine + .preview_pre_tool_use(&PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd, + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }) + .is_empty() + ); +} + +#[test] +fn discovers_hooks_from_json_and_toml_in_the_same_layer() { + let temp = tempdir().expect("create temp dir"); + let config_path = + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"); + let hooks_json_path = + AbsolutePathBuf::try_from(temp.path().join("hooks.json")).expect("absolute hooks path"); + fs::write( + hooks_json_path.as_path(), + r#"{ + "hooks": { + "PreToolUse": [ + { + "matcher": "^Bash$", + "hooks": [ + { + "type": "command", + "command": "python3 /tmp/json-hook.py" + } + ] + } + ] + } + }"#, + ) + .expect("write hooks.json"); + let mut config_toml = TomlValue::Table(Default::default()); + let TomlValue::Table(config_table) = &mut config_toml else { + unreachable!("config TOML root should be a table"); + }; + let mut hooks_table = TomlValue::Table(Default::default()); + let TomlValue::Table(hooks_entries) = &mut hooks_table else { + unreachable!("hooks entry should be a table"); + }; + let mut pre_tool_use_group = TomlValue::Table(Default::default()); + let TomlValue::Table(pre_tool_use_group_entries) = &mut pre_tool_use_group else { + unreachable!("PreToolUse group should be a table"); + }; + pre_tool_use_group_entries.insert( + "matcher".to_string(), + TomlValue::String("^Bash$".to_string()), + ); + pre_tool_use_group_entries.insert( + "hooks".to_string(), + TomlValue::Array(vec![TomlValue::Table(Default::default())]), + ); + let Some(TomlValue::Array(hooks_array)) = pre_tool_use_group_entries.get_mut("hooks") else { + unreachable!("PreToolUse hooks should be an array"); + }; + let Some(TomlValue::Table(handler_entries)) = hooks_array.first_mut() else { + unreachable!("PreToolUse handler should be a table"); + }; + handler_entries.insert("type".to_string(), TomlValue::String("command".to_string())); + handler_entries.insert( + "command".to_string(), + TomlValue::String("python3 /tmp/toml-hook.py".to_string()), + ); + hooks_entries.insert( + "PreToolUse".to_string(), + TomlValue::Array(vec![pre_tool_use_group]), + ); + config_table.insert("hooks".to_string(), hooks_table); + let config_layer_stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_path.clone(), + }, + config_toml, + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack"); + + let engine = ClaudeHooksEngine::new( + /*enabled*/ true, + Some(&config_layer_stack), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert!(engine.warnings().iter().any(|warning| { + warning.contains("loading hooks from both") + && warning.contains(&hooks_json_path.display().to_string()) + && warning.contains(&config_path.display().to_string()) + })); + + let cwd = cwd(); + let preview = engine.preview_pre_tool_use(&PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd, + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + matcher_aliases: Vec::new(), + tool_use_id: "tool-1".to_string(), + command: "echo hello".to_string(), + }); + assert_eq!(preview.len(), 2); + assert!(engine.handlers.iter().all(|handler| !handler.is_managed)); + assert_eq!(preview[0].source_path, hooks_json_path); + assert_eq!(preview[1].source_path, config_path); +} From c61cbf28ba5a4177206e326a6f16b51f124eef12 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 22 Apr 2026 16:53:54 -0700 Subject: [PATCH 10/12] codex: streamline managed hook path checks --- codex-rs/hooks/src/engine/discovery.rs | 31 +++++++++++++------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 95e265b6c1c2..4e704e0a0358 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -144,36 +144,35 @@ fn managed_hooks_source_path( )); return None; }; + if !source_path.is_absolute() { warnings.push(format!( "skipping managed hooks from {source}: managed hook directory {} is not absolute", source_path.display() )); - return None; - } - if !source_path.exists() { + None + } else if !source_path.exists() { warnings.push(format!( "skipping managed hooks from {source}: managed hook directory {} does not exist", source_path.display() )); - return None; - } - if !source_path.is_dir() { + None + } else if !source_path.is_dir() { warnings.push(format!( "skipping managed hooks from {source}: managed hook directory {} is not a directory", source_path.display() )); - return None; + None + } else { + AbsolutePathBuf::from_absolute_path(source_path) + .inspect_err(|err| { + warnings.push(format!( + "skipping managed hooks from {source}: could not normalize managed hook directory {}: {err}", + source_path.display() + )); + }) + .ok() } - - AbsolutePathBuf::from_absolute_path(source_path) - .inspect_err(|err| { - warnings.push(format!( - "skipping managed hooks from {source}: could not normalize managed hook directory {}: {err}", - source_path.display() - )); - }) - .ok() } fn load_hooks_json( From 7c3ebaae9f4affaa1de462c8421fdb81e3dbc9af Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 22 Apr 2026 19:27:02 -0700 Subject: [PATCH 11/12] Revert "app-server: box tracing wrappers on windows" This reverts commit b159d96e60d99a119ef66c658e3f99e7b3ab5c63. --- .../app-server/src/codex_message_processor.rs | 41 ++- .../src/message_processor/tracing_tests.rs | 303 ++++++++++-------- 2 files changed, 190 insertions(+), 154 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index feb335333d60..525e75e65018 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -870,15 +870,13 @@ impl CodexMessageProcessor { } // === v2 Thread/Turn APIs === ClientRequest::ThreadStart { request_id, params } => { - // Keep the large thread-start setup future off this request - // wrapper's stack on Windows. - Box::pin(self.thread_start( + self.thread_start( to_connection_request_id(request_id), params, app_server_client_name.clone(), app_server_client_version.clone(), request_context, - )) + ) .await; } ClientRequest::ThreadUnsubscribe { request_id, params } => { @@ -2401,23 +2399,24 @@ impl CodexMessageProcessor { }; let request_trace = request_context.request_trace(); let config_manager = self.config_manager.clone(); - // Heap-allocate the spawned task future so constructing it here does - // not inline the full thread-start state machine on the caller stack. - let thread_start_task = Box::pin(Self::thread_start_task( - listener_task_context, - config_manager, - request_id, - app_server_client_name, - app_server_client_version, - config, - typesafe_overrides, - dynamic_tools, - session_start_source, - persist_extended_history, - service_name, - experimental_raw_events, - request_trace, - )); + let thread_start_task = async move { + Self::thread_start_task( + listener_task_context, + config_manager, + request_id, + app_server_client_name, + app_server_client_version, + config, + typesafe_overrides, + dynamic_tools, + session_start_source, + persist_extended_history, + service_name, + experimental_raw_events, + request_trace, + ) + .await; + }; self.background_tasks .spawn(thread_start_task.instrument(request_context.span())); } diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index 4da75f409062..84fddf5e8524 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -48,6 +48,7 @@ use opentelemetry_sdk::trace::SpanData; use pretty_assertions::assert_eq; use serial_test::serial; use std::collections::BTreeMap; +use std::future::Future; use std::path::Path; use std::sync::Arc; use std::sync::OnceLock; @@ -119,13 +120,13 @@ struct TracingHarness { impl TracingHarness { async fn new() -> Result { - Box::pin(Self::new_with_origin(ConnectionOrigin::WebSocket)).await + Self::new_with_origin(ConnectionOrigin::WebSocket).await } async fn new_with_origin(origin: ConnectionOrigin) -> Result { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - let config = Arc::new(Box::pin(build_test_config(codex_home.path(), &server.uri())).await?); + let config = Arc::new(build_test_config(codex_home.path(), &server.uri()).await?); let (processor, outgoing_rx) = build_test_processor(config); let tracing = init_test_tracing(); tracing.exporter.reset(); @@ -183,13 +184,14 @@ impl TracingHarness { let mut request = request_from_client_request(request); request.trace = trace; - Box::pin(self.processor.process_request( - TEST_CONNECTION_ID, - request, - AppServerTransport::Stdio, - Arc::clone(&self.session), - )) - .await; + self.processor + .process_request( + TEST_CONNECTION_ID, + request, + AppServerTransport::Stdio, + Arc::clone(&self.session), + ) + .await; read_response(&mut self.outgoing_rx, request_id).await } @@ -205,13 +207,14 @@ impl TracingHarness { let mut request = request_from_client_request(request); request.trace = trace; - Box::pin(self.processor.process_request( - TEST_CONNECTION_ID, - request, - AppServerTransport::Stdio, - Arc::clone(&self.session), - )) - .await; + self.processor + .process_request( + TEST_CONNECTION_ID, + request, + AppServerTransport::Stdio, + Arc::clone(&self.session), + ) + .await; read_error(&mut self.outgoing_rx, request_id).await } @@ -220,17 +223,18 @@ impl TracingHarness { request_id: i64, trace: Option, ) -> ThreadStartResponse { - let response = Box::pin(self.request( - ClientRequest::ThreadStart { - request_id: RequestId::Integer(request_id), - params: ThreadStartParams { - ephemeral: Some(true), - ..ThreadStartParams::default() + let response = self + .request( + ClientRequest::ThreadStart { + request_id: RequestId::Integer(request_id), + params: ThreadStartParams { + ephemeral: Some(true), + ..ThreadStartParams::default() + }, }, - }, - trace, - )) - .await; + trace, + ) + .await; read_thread_started_notification(&mut self.outgoing_rx).await; response } @@ -288,6 +292,28 @@ fn build_test_processor( (processor, outgoing_rx) } +fn run_current_thread_test_with_stack(name: &str, future: F) -> Result<()> +where + F: Future> + Send + 'static, +{ + const TEST_STACK_SIZE_BYTES: usize = 4 * 1024 * 1024; + + let handle = std::thread::Builder::new() + .name(name.to_string()) + .stack_size(TEST_STACK_SIZE_BYTES) + .spawn(move || -> Result<()> { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()?; + runtime.block_on(Box::pin(future)) + })?; + + match handle.join() { + Ok(result) => result, + Err(_) => Err(anyhow::anyhow!("{name} thread panicked")), + } +} + fn span_attr<'a>(span: &'a SpanData, key: &str) -> Option<&'a str> { span.attributes .iter() @@ -565,85 +591,96 @@ where spans.into_iter().skip(baseline_len).collect() } -#[tokio::test(flavor = "current_thread")] +#[test] #[serial(app_server_tracing)] -async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { - let mut harness = Box::pin(TracingHarness::new()).await?; - - let RemoteTrace { - trace_id: remote_trace_id, - parent_span_id: remote_parent_span_id, - context: remote_trace, - .. - } = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022"); - - let _: ThreadStartResponse = - Box::pin(harness.start_thread(/*request_id*/ 20_002, /*trace*/ None)).await; - let untraced_spans = Box::pin(wait_for_exported_spans(harness.tracing, |spans| { - spans.iter().any(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.method") == Some("thread/start") - }) - })) - .await; - let untraced_server_span = find_rpc_span_with_trace( - &untraced_spans, - SpanKind::Server, - "thread/start", - untraced_spans - .iter() - .rev() - .find(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.system") == Some("jsonrpc") - && span_attr(span, "rpc.method") == Some("thread/start") +fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { + run_current_thread_test_with_stack( + "thread_start_jsonrpc_span_exports_server_span_and_parents_children", + async { + let mut harness = TracingHarness::new().await?; + + let RemoteTrace { + trace_id: remote_trace_id, + parent_span_id: remote_parent_span_id, + context: remote_trace, + .. + } = RemoteTrace::new("00000000000000000000000000000011", "0000000000000022"); + + let _: ThreadStartResponse = harness + .start_thread(/*request_id*/ 20_002, /*trace*/ None) + .await; + let untraced_spans = wait_for_exported_spans(harness.tracing, |spans| { + spans.iter().any(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.method") == Some("thread/start") + }) }) - .unwrap_or_else(|| { - panic!( - "missing latest thread/start server span; exported spans:\n{}", - format_spans(&untraced_spans) - ) + .await; + let untraced_server_span = find_rpc_span_with_trace( + &untraced_spans, + SpanKind::Server, + "thread/start", + untraced_spans + .iter() + .rev() + .find(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.system") == Some("jsonrpc") + && span_attr(span, "rpc.method") == Some("thread/start") + }) + .unwrap_or_else(|| { + panic!( + "missing latest thread/start server span; exported spans:\n{}", + format_spans(&untraced_spans) + ) + }) + .span_context + .trace_id(), + ); + assert_has_internal_descendant_at_min_depth( + &untraced_spans, + untraced_server_span, + /*min_depth*/ 1, + ); + + let baseline_len = untraced_spans.len(); + let _: ThreadStartResponse = harness + .start_thread(/*request_id*/ 20_003, Some(remote_trace)) + .await; + let spans = wait_for_new_exported_spans(harness.tracing, baseline_len, |spans| { + spans.iter().any(|span| { + span.span_kind == SpanKind::Server + && span_attr(span, "rpc.method") == Some("thread/start") + && span.span_context.trace_id() == remote_trace_id + }) && spans.iter().any(|span| { + span.name.as_ref() == "app_server.thread_start.notify_started" + && span.span_context.trace_id() == remote_trace_id + }) }) - .span_context - .trace_id(), - ); - assert_has_internal_descendant_at_min_depth( - &untraced_spans, - untraced_server_span, - /*min_depth*/ 1, - ); + .await; - let baseline_len = untraced_spans.len(); - let _: ThreadStartResponse = - Box::pin(harness.start_thread(/*request_id*/ 20_003, Some(remote_trace))).await; - let spans = Box::pin(wait_for_new_exported_spans( - harness.tracing, - baseline_len, - |spans| { - spans.iter().any(|span| { - span.span_kind == SpanKind::Server - && span_attr(span, "rpc.method") == Some("thread/start") - && span.span_context.trace_id() == remote_trace_id - }) && spans.iter().any(|span| { - span.name.as_ref() == "app_server.thread_start.notify_started" - && span.span_context.trace_id() == remote_trace_id - }) + let server_request_span = + find_rpc_span_with_trace(&spans, SpanKind::Server, "thread/start", remote_trace_id); + assert_eq!(server_request_span.name.as_ref(), "thread/start"); + assert_eq!(server_request_span.parent_span_id, remote_parent_span_id); + assert!(server_request_span.parent_span_is_remote); + assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id); + assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID); + assert_has_internal_descendant_at_min_depth( + &spans, + server_request_span, + /*min_depth*/ 1, + ); + assert_has_internal_descendant_at_min_depth( + &spans, + server_request_span, + /*min_depth*/ 2, + ); + harness.shutdown().await; + + Ok(()) }, - )) - .await; - - let server_request_span = - find_rpc_span_with_trace(&spans, SpanKind::Server, "thread/start", remote_trace_id); - assert_eq!(server_request_span.name.as_ref(), "thread/start"); - assert_eq!(server_request_span.parent_span_id, remote_parent_span_id); - assert!(server_request_span.parent_span_is_remote); - assert_eq!(server_request_span.span_context.trace_id(), remote_trace_id); - assert_ne!(server_request_span.span_context.span_id(), SpanId::INVALID); - assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 1); - assert_has_internal_descendant_at_min_depth(&spans, server_request_span, /*min_depth*/ 2); - Box::pin(harness.shutdown()).await; - - Ok(()) + ) } #[tokio::test(flavor = "current_thread")] @@ -690,9 +727,8 @@ async fn remote_control_origin_rejects_device_key_requests() -> Result<()> { #[tokio::test(flavor = "current_thread")] #[serial(app_server_tracing)] async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { - let mut harness = Box::pin(TracingHarness::new()).await?; - let thread_start_response = - Box::pin(harness.start_thread(/*request_id*/ 2, /*trace*/ None)).await; + let mut harness = TracingHarness::new().await?; + let thread_start_response = harness.start_thread(/*request_id*/ 2, /*trace*/ None).await; let thread_id = thread_start_response.thread.id.clone(); harness.reset_tracing(); @@ -702,35 +738,36 @@ async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { parent_span_id: remote_parent_span_id, context: remote_trace, } = RemoteTrace::new("00000000000000000000000000000077", "0000000000000088"); - let turn_start_response: TurnStartResponse = Box::pin(harness.request( - ClientRequest::TurnStart { - request_id: RequestId::Integer(3), - params: TurnStartParams { - environments: None, - thread_id, - input: vec![UserInput::Text { - text: "hello".to_string(), - text_elements: Vec::new(), - }], - responsesapi_client_metadata: None, - cwd: None, - approval_policy: None, - sandbox_policy: None, - permission_profile: None, - approvals_reviewer: None, - model: None, - service_tier: None, - effort: None, - summary: None, - personality: None, - output_schema: None, - collaboration_mode: None, + let turn_start_response: TurnStartResponse = harness + .request( + ClientRequest::TurnStart { + request_id: RequestId::Integer(3), + params: TurnStartParams { + environments: None, + thread_id, + input: vec![UserInput::Text { + text: "hello".to_string(), + text_elements: Vec::new(), + }], + responsesapi_client_metadata: None, + cwd: None, + approval_policy: None, + sandbox_policy: None, + permission_profile: None, + approvals_reviewer: None, + model: None, + service_tier: None, + effort: None, + summary: None, + personality: None, + output_schema: None, + collaboration_mode: None, + }, }, - }, - Some(remote_trace), - )) - .await; - let spans = Box::pin(wait_for_exported_spans(harness.tracing, |spans| { + Some(remote_trace), + ) + .await; + let spans = wait_for_exported_spans(harness.tracing, |spans| { spans.iter().any(|span| { span.span_kind == SpanKind::Server && span_attr(span, "rpc.method") == Some("turn/start") @@ -739,7 +776,7 @@ async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { span_attr(span, "codex.op") == Some("user_input") && span.span_context.trace_id() == remote_trace_id }) - })) + }) .await; let server_request_span = @@ -757,7 +794,7 @@ async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { Some(turn_start_response.turn.id.as_str()) ); assert_span_descends_from(&spans, core_turn_span, server_request_span); - Box::pin(harness.shutdown()).await; + harness.shutdown().await; Ok(()) } From 173d2e1a6df7002658a6a5f25e44f90b43da9642 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 22 Apr 2026 20:03:55 -0700 Subject: [PATCH 12/12] tui: fix approvals popup disabled shortcut test --- .../tui/src/chatwidget/tests/permissions.rs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index f9fcd3179785..6a3421fe33a8 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -189,6 +189,7 @@ async fn approvals_popup_shows_disabled_presets() { #[tokio::test] async fn approvals_popup_navigation_skips_disabled() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.set_feature_enabled(Feature::GuardianApproval, /*enabled*/ false); chat.config.permissions.approval_policy = Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { @@ -198,14 +199,23 @@ async fn approvals_popup_navigation_skips_disabled() { .expect("construct constrained approval policy"); chat.open_approvals_popup(); - // The approvals popup is the active bottom-pane view; drive navigation via chat handle_key_event. - // Start selected at idx 0 (enabled), move down twice; the disabled option should be skipped - // and selection should wrap back to idx 0 (also enabled). + // Keep the popup layout stable across platforms and feature defaults so + // the hidden numeric shortcut below still targets the disabled Full Access row. + let disabled_shortcut = if cfg!(target_os = "windows") { + '3' + } else { + '2' + }; + + // The approvals popup is the active bottom-pane view; drive navigation via + // chat.handle_key_event. Move through the menu so selection stays on an + // enabled preset even when disabled rows are skipped. chat.handle_key_event(KeyEvent::from(KeyCode::Down)); chat.handle_key_event(KeyEvent::from(KeyCode::Down)); - // Press numeric shortcut for the disabled row (3 => idx 2); should not close or accept. - chat.handle_key_event(KeyEvent::from(KeyCode::Char('3'))); + // Press the hidden numeric shortcut for the disabled Full Access row; it + // should not close the popup or accept the preset. + chat.handle_key_event(KeyEvent::from(KeyCode::Char(disabled_shortcut))); // Ensure the popup remains open and no selection actions were sent. let width = 80;