From 0452dca986b4abc637bb702213cf9e49f8867f7e Mon Sep 17 00:00:00 2001 From: Abhinav Date: Tue, 5 May 2026 12:13:55 -0700 Subject: [PATCH] hook trust metadata and enforcement (#20321) # Why We want shared hook trust that both the app and the TUI can build on, but the metadata is only useful if runtime behavior agrees with it. This PR adds a single backend trust model for hooks so unmanaged hooks cannot run until the current definition has been reviewed, while managed hooks remain runnable and non-configurable. # What - persist `trusted_hash` alongside hook state in `config.toml` - expose `currentHash` and derived `trustStatus` through `hooks/list` - derive trust from normalized hook definitions so equivalent hooks from `config.toml` and `hooks.json` share the same trust identity - gate unmanaged hooks on trust before they enter the runnable handler set # Reviewer Notes - key file to review is `codex-rs/hooks/src/engine/discovery.rs` - the only **core** change is schema related --- codex-rs/Cargo.lock | 1 + .../codex_app_server_protocol.schemas.json | 19 +- .../codex_app_server_protocol.v2.schemas.json | 19 +- .../schema/json/v2/HooksListResponse.json | 19 +- .../schema/typescript/v2/HookMetadata.ts | 3 +- .../schema/typescript/v2/HookTrustStatus.ts | 5 + .../schema/typescript/v2/index.ts | 1 + .../app-server-protocol/src/protocol/v2.rs | 9 + codex-rs/app-server/README.md | 10 +- .../request_processors/catalog_processor.rs | 2 + .../app-server/tests/suite/v2/hooks_list.rs | 329 ++++++++++++++ codex-rs/config/src/hook_config.rs | 2 + codex-rs/config/src/hooks_tests.rs | 2 + codex-rs/core/config.schema.json | 3 + codex-rs/core/src/mcp_tool_call_tests.rs | 14 +- codex-rs/core/src/session/tests.rs | 85 +++- .../runtimes/shell/unix_escalation_tests.rs | 25 +- codex-rs/core/tests/common/Cargo.toml | 1 + codex-rs/core/tests/common/hooks.rs | 70 +++ codex-rs/core/tests/common/lib.rs | 1 + codex-rs/core/tests/suite/hooks.rs | 316 +++++--------- codex-rs/core/tests/suite/hooks_mcp.rs | 6 +- codex-rs/core/tests/suite/openai_file_mcp.rs | 5 +- codex-rs/hooks/src/config_rules.rs | 133 ++++-- codex-rs/hooks/src/engine/discovery.rs | 193 ++++++--- codex-rs/hooks/src/engine/mod.rs | 3 + codex-rs/hooks/src/engine/mod_tests.rs | 139 +++++- codex-rs/protocol/src/protocol.rs | 34 +- codex-rs/tui/src/app.rs | 1 + codex-rs/tui/src/app/background_requests.rs | 88 ++++ codex-rs/tui/src/app/event_dispatch.rs | 8 + codex-rs/tui/src/app/startup_prompts.rs | 10 + codex-rs/tui/src/app/tests.rs | 11 + codex-rs/tui/src/app_event.rs | 11 + .../tui/src/bottom_pane/hooks_browser_view.rs | 405 ++++++++++++++++-- ..._hooks_browser_capped_command_details.snap | 1 + ...oks_browser_events_with_review_column.snap | 17 + ...r_view__tests__hooks_browser_handlers.snap | 1 + ..._tests__hooks_browser_managed_handler.snap | 1 + ...__hooks_browser_review_needed_handler.snap | 18 + ...ests__hooks_browser_scrolled_handlers.snap | 1 + ...ooks_browser_selected_managed_handler.snap | 1 + ...oks_browser_untrusted_enabled_handler.snap | 18 + ..._hooks_needing_review_startup_warning.snap | 5 + 44 files changed, 1661 insertions(+), 385 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/HookTrustStatus.ts create mode 100644 codex-rs/core/tests/common/hooks.rs create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_events_with_review_column.snap create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_review_needed_handler.snap create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_untrusted_enabled_handler.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__app__tests__hooks_needing_review_startup_warning.snap diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 8c0c64bae..1e1590e6c 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4219,6 +4219,7 @@ dependencies = [ "codex-core", "codex-exec-server", "codex-features", + "codex-hooks", "codex-login", "codex-model-provider-info", "codex-models-manager", 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 2eaba62ac..ff1071f13 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 @@ -9833,6 +9833,9 @@ "null" ] }, + "currentHash": { + "type": "string" + }, "displayOrder": { "format": "int64", "type": "integer" @@ -9880,9 +9883,13 @@ "format": "uint64", "minimum": 0.0, "type": "integer" + }, + "trustStatus": { + "$ref": "#/definitions/v2/HookTrustStatus" } }, "required": [ + "currentHash", "displayOrder", "enabled", "eventName", @@ -9891,7 +9898,8 @@ "key", "source", "sourcePath", - "timeoutSec" + "timeoutSec", + "trustStatus" ], "type": "object" }, @@ -10081,6 +10089,15 @@ "title": "HookStartedNotification", "type": "object" }, + "HookTrustStatus": { + "enum": [ + "managed", + "untrusted", + "trusted", + "modified" + ], + "type": "string" + }, "HooksListEntry": { "properties": { "cwd": { 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 e99aa6653..29a40ea28 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 @@ -6400,6 +6400,9 @@ "null" ] }, + "currentHash": { + "type": "string" + }, "displayOrder": { "format": "int64", "type": "integer" @@ -6447,9 +6450,13 @@ "format": "uint64", "minimum": 0.0, "type": "integer" + }, + "trustStatus": { + "$ref": "#/definitions/HookTrustStatus" } }, "required": [ + "currentHash", "displayOrder", "enabled", "eventName", @@ -6458,7 +6465,8 @@ "key", "source", "sourcePath", - "timeoutSec" + "timeoutSec", + "trustStatus" ], "type": "object" }, @@ -6648,6 +6656,15 @@ "title": "HookStartedNotification", "type": "object" }, + "HookTrustStatus": { + "enum": [ + "managed", + "untrusted", + "trusted", + "modified" + ], + "type": "string" + }, "HooksListEntry": { "properties": { "cwd": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json index 5190b2271..ae9cd9e63 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json @@ -47,6 +47,9 @@ "null" ] }, + "currentHash": { + "type": "string" + }, "displayOrder": { "format": "int64", "type": "integer" @@ -94,9 +97,13 @@ "format": "uint64", "minimum": 0.0, "type": "integer" + }, + "trustStatus": { + "$ref": "#/definitions/HookTrustStatus" } }, "required": [ + "currentHash", "displayOrder", "enabled", "eventName", @@ -105,7 +112,8 @@ "key", "source", "sourcePath", - "timeoutSec" + "timeoutSec", + "trustStatus" ], "type": "object" }, @@ -124,6 +132,15 @@ ], "type": "string" }, + "HookTrustStatus": { + "enum": [ + "managed", + "untrusted", + "trusted", + "modified" + ], + "type": "string" + }, "HooksListEntry": { "properties": { "cwd": { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts index 8ccd2b182..94e3c30c9 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HookMetadata.ts @@ -5,5 +5,6 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; import type { HookEventName } from "./HookEventName"; import type { HookHandlerType } from "./HookHandlerType"; import type { HookSource } from "./HookSource"; +import type { HookTrustStatus } from "./HookTrustStatus"; -export type HookMetadata = { key: string, eventName: HookEventName, handlerType: HookHandlerType, matcher: string | null, command: string | null, timeoutSec: bigint, statusMessage: string | null, sourcePath: AbsolutePathBuf, source: HookSource, pluginId: string | null, displayOrder: bigint, enabled: boolean, isManaged: boolean, }; +export type HookMetadata = { key: string, eventName: HookEventName, handlerType: HookHandlerType, matcher: string | null, command: string | null, timeoutSec: bigint, statusMessage: string | null, sourcePath: AbsolutePathBuf, source: HookSource, pluginId: string | null, displayOrder: bigint, enabled: boolean, isManaged: boolean, currentHash: string, trustStatus: HookTrustStatus, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookTrustStatus.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookTrustStatus.ts new file mode 100644 index 000000000..692fdc4c1 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HookTrustStatus.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 HookTrustStatus = "managed" | "untrusted" | "trusted" | "modified"; 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 a226ebe11..4484d61ad 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -168,6 +168,7 @@ export type { HookRunSummary } from "./HookRunSummary"; export type { HookScope } from "./HookScope"; export type { HookSource } from "./HookSource"; export type { HookStartedNotification } from "./HookStartedNotification"; +export type { HookTrustStatus } from "./HookTrustStatus"; export type { HooksListEntry } from "./HooksListEntry"; export type { HooksListParams } from "./HooksListParams"; export type { HooksListResponse } from "./HooksListResponse"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 52e248a37..d7f60e74b 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -80,6 +80,7 @@ use codex_protocol::protocol::HookRunStatus as CoreHookRunStatus; use codex_protocol::protocol::HookRunSummary as CoreHookRunSummary; use codex_protocol::protocol::HookScope as CoreHookScope; use codex_protocol::protocol::HookSource as CoreHookSource; +use codex_protocol::protocol::HookTrustStatus as CoreHookTrustStatus; use codex_protocol::protocol::ModelRerouteReason as CoreModelRerouteReason; use codex_protocol::protocol::ModelVerification as CoreModelVerification; use codex_protocol::protocol::NetworkAccess as CoreNetworkAccess; @@ -482,6 +483,12 @@ v2_enum_from_core!( } ); +v2_enum_from_core!( + pub enum HookTrustStatus from CoreHookTrustStatus { + Managed, Untrusted, Trusted, Modified + } +); + fn default_hook_source() -> HookSource { HookSource::Unknown } @@ -5032,6 +5039,8 @@ pub struct HookMetadata { pub display_order: i64, pub enabled: bool, pub is_managed: bool, + pub current_hash: String, + pub trust_status: HookTrustStatus, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 3b80234ad..c5b3e9a1e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1539,7 +1539,11 @@ To enable or disable a skill by name: } ``` -Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. Each entry is evaluated using that `cwd`'s effective config, so feature gating and discovered config layers can differ across entries in the same request. Disabled hooks are still returned with `"enabled": false` so clients can render and re-enable them. Hook state is stored under `hooks.state`; clients should treat hooks from managed sources as non-configurable, and user config entries for those keys are ignored during loading. Hook keys combine the source identity with a trailing event/group/handler selector that is currently positional. +Use `hooks/list` to fetch discovered hooks for one or more `cwds`. Each result is evaluated with that `cwd`'s effective config, so feature gates and discovered config layers can differ within a single response. + +Hooks are returned even when disabled so clients can render and re-enable them. User-controlled state lives under `hooks.state`. Managed hooks are non-configurable, and user entries for managed hook keys are ignored during loading. + +For unmanaged hooks, `currentHash` and `trustStatus` describe whether the current definition is first-seen, approved, or changed since approval. Only trusted unmanaged hooks become runnable. Hook keys combine the source identity with a trailing event/group/handler selector that is currently positional. ```json { @@ -1570,7 +1574,9 @@ Use `hooks/list` to fetch the discovered hooks for one or more `cwds`. Each entr "source": "user", "pluginId": null, "displayOrder": 0, - "enabled": true + "enabled": true, + "currentHash": "sha256:...", + "trustStatus": "untrusted" }], "warnings": [], "errors": [] diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index c2876514a..19d1a4beb 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -72,6 +72,8 @@ fn hooks_to_info(hooks: &[codex_hooks::HookListEntry]) -> Vec { display_order: hook.display_order, enabled: hook.enabled, is_managed: hook.is_managed, + current_hash: hook.current_hash.clone(), + trust_status: hook.trust_status.into(), }) .collect() } diff --git a/codex-rs/app-server/tests/suite/v2/hooks_list.rs b/codex-rs/app-server/tests/suite/v2/hooks_list.rs index f80d59d96..623896626 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -11,6 +11,7 @@ use codex_app_server_protocol::HookEventName; use codex_app_server_protocol::HookHandlerType; use codex_app_server_protocol::HookMetadata; use codex_app_server_protocol::HookSource; +use codex_app_server_protocol::HookTrustStatus; use codex_app_server_protocol::HooksListEntry; use codex_app_server_protocol::HooksListParams; use codex_app_server_protocol::HooksListResponse; @@ -26,11 +27,44 @@ use codex_protocol::config_types::TrustLevel; use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::skip_if_windows; use pretty_assertions::assert_eq; +use serde::Serialize; use tempfile::TempDir; use tokio::time::timeout; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(30); +#[derive(Serialize)] +struct NormalizedHookIdentity { + event_name: &'static str, + #[serde(flatten)] + group: codex_config::MatcherGroup, +} + +fn command_hook_hash( + event_name: &'static str, + matcher: Option<&str>, + command: &str, + timeout_sec: u64, + status_message: Option<&str>, +) -> String { + let identity = NormalizedHookIdentity { + event_name, + group: codex_config::MatcherGroup { + matcher: matcher.map(ToOwned::to_owned), + hooks: vec![codex_config::HookHandlerConfig::Command { + command: command.to_string(), + timeout_sec: Some(timeout_sec), + r#async: false, + status_message: status_message.map(ToOwned::to_owned), + }], + }, + }; + let Ok(value) = codex_config::TomlValue::try_from(identity) else { + unreachable!("normalized hook identity should serialize to TOML"); + }; + codex_config::version_for_toml(&value) +} + fn write_user_hook_config(codex_home: &std::path::Path) -> Result<()> { std::fs::write( codex_home.join("config.toml"), @@ -113,6 +147,14 @@ async fn hooks_list_shows_discovered_hook() -> Result<()> { display_order: 0, enabled: true, is_managed: false, + current_hash: command_hook_hash( + "pre_tool_use", + Some("Bash"), + "python3 /tmp/listed-hook.py", + /*timeout_sec*/ 5, + Some("running listed hook"), + ), + trust_status: HookTrustStatus::Untrusted, }], warnings: Vec::new(), errors: Vec::new(), @@ -183,6 +225,14 @@ async fn hooks_list_shows_discovered_plugin_hook() -> Result<()> { display_order: 0, enabled: true, is_managed: false, + current_hash: command_hook_hash( + "pre_tool_use", + Some("Bash"), + "echo plugin hook", + /*timeout_sec*/ 7, + Some("running plugin hook"), + ), + trust_status: HookTrustStatus::Untrusted, }], warnings: Vec::new(), errors: Vec::new(), @@ -300,6 +350,14 @@ timeout = 5 display_order: 0, enabled: true, is_managed: false, + current_hash: command_hook_hash( + "pre_tool_use", + Some("Bash"), + "echo project hook", + /*timeout_sec*/ 5, + /*status_message*/ None, + ), + trust_status: HookTrustStatus::Untrusted, }], warnings: Vec::new(), errors: Vec::new(), @@ -408,6 +466,254 @@ async fn config_batch_write_toggles_user_hook() -> Result<()> { Ok(()) } +#[tokio::test] +async fn config_batch_write_updates_hook_trust_for_loaded_session() -> Result<()> { + skip_if_windows!(Ok(())); + + let responses = vec![ + create_final_assistant_message_sse_response("Warmup")?, + create_final_assistant_message_sse_response("Untrusted turn")?, + create_final_assistant_message_sse_response("Trusted turn")?, + create_final_assistant_message_sse_response("Modified turn")?, + ]; + let server = create_mock_responses_server_sequence_unchecked(responses).await; + let codex_home = TempDir::new()?; + let hook_script_path = codex_home.path().join("user_prompt_submit_hook.py"); + let hook_log_path = codex_home.path().join("user_prompt_submit_hook_log.jsonl"); + std::fs::write( + &hook_script_path, + format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{hook_log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +"#, + hook_log_path = hook_log_path.display(), + ), + )?; + std::fs::write( + codex_home.path().join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 + +[hooks] + +[[hooks.UserPromptSubmit]] + +[[hooks.UserPromptSubmit.hooks]] +type = "command" +command = "python3 {hook_script_path}" +"#, + server_uri = server.uri(), + hook_script_path = hook_script_path.display(), + ), + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let hook_list_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![codex_home.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(hook_list_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + let hook = data[0].hooks[0].clone(); + assert_eq!(hook.trust_status, HookTrustStatus::Untrusted); + + let thread_start_id = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + ..Default::default() + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response(response)?; + + let first_turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "first turn".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(first_turn_id)), + ) + .await??; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + assert!(!std::fs::exists(&hook_log_path)?); + + let write_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: "hooks.state".to_string(), + value: serde_json::json!({ + hook.key.clone(): { + "trusted_hash": hook.current_hash.clone() + } + }), + merge_strategy: MergeStrategy::Upsert, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(write_id)), + ) + .await??; + let _: codex_app_server_protocol::ConfigWriteResponse = to_response(response)?; + + let hook_list_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![codex_home.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(hook_list_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + let trusted_hook = &data[0].hooks[0]; + assert_eq!(trusted_hook.key, hook.key); + assert_eq!(trusted_hook.current_hash, hook.current_hash); + assert_eq!(trusted_hook.trust_status, HookTrustStatus::Trusted); + + let second_turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "second turn".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_turn_id)), + ) + .await??; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + assert_eq!( + std::fs::read_to_string(&hook_log_path)? + .lines() + .filter(|line| !line.is_empty()) + .count(), + 1 + ); + + let write_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: "hooks.UserPromptSubmit".to_string(), + value: serde_json::json!([{ + "hooks": [{ + "type": "command", + "command": format!("python3 {}", hook_script_path.display()), + "statusMessage": "modified hook", + }], + }]), + merge_strategy: MergeStrategy::Replace, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(write_id)), + ) + .await??; + let _: codex_app_server_protocol::ConfigWriteResponse = to_response(response)?; + + let hook_list_id = mcp + .send_hooks_list_request(HooksListParams { + cwds: vec![codex_home.path().to_path_buf()], + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(hook_list_id)), + ) + .await??; + let HooksListResponse { data } = to_response(response)?; + let modified_hook = &data[0].hooks[0]; + assert_eq!(modified_hook.key, hook.key); + assert_ne!(modified_hook.current_hash, hook.current_hash); + assert_eq!(modified_hook.trust_status, HookTrustStatus::Modified); + + let third_turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + input: vec![V2UserInput::Text { + text: "third turn".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(third_turn_id)), + ) + .await??; + timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + assert_eq!( + std::fs::read_to_string(&hook_log_path)? + .lines() + .filter(|line| !line.is_empty()) + .count(), + 1 + ); + Ok(()) +} + #[tokio::test] async fn config_batch_write_disables_hook_for_loaded_session() -> Result<()> { skip_if_windows!(Ok(())); @@ -482,6 +788,29 @@ command = "python3 {hook_script_path}" let hook = &data[0].hooks[0]; assert_eq!(hook.enabled, true); + let write_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: "hooks.state".to_string(), + value: serde_json::json!({ + hook.key.clone(): { + "trusted_hash": hook.current_hash.clone() + } + }), + merge_strategy: MergeStrategy::Upsert, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(write_id)), + ) + .await??; + let _: codex_app_server_protocol::ConfigWriteResponse = to_response(response)?; + let thread_start_id = mcp .send_thread_start_request(ThreadStartParams { model: Some("mock-model".to_string()), diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs index d947ebb86..27cca781c 100644 --- a/codex-rs/config/src/hook_config.rs +++ b/codex-rs/config/src/hook_config.rs @@ -25,6 +25,8 @@ pub struct HooksToml { pub struct HookStateToml { #[serde(default, skip_serializing_if = "Option::is_none")] pub enabled: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub trusted_hash: Option, } #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] diff --git a/codex-rs/config/src/hooks_tests.rs b/codex-rs/config/src/hooks_tests.rs index 93541ee7f..69fcd3fe9 100644 --- a/codex-rs/config/src/hooks_tests.rs +++ b/codex-rs/config/src/hooks_tests.rs @@ -90,6 +90,7 @@ fn hooks_toml_deserializes_inline_events_and_state_map() { r#" [state."/tmp/hooks.json:pre_tool_use:0:0"] enabled = false +trusted_hash = "sha256:abc123" [[PreToolUse]] matcher = "^Bash$" @@ -120,6 +121,7 @@ command = "python3 /tmp/pre.py" "/tmp/hooks.json:pre_tool_use:0:0".to_string(), super::HookStateToml { enabled: Some(false), + trusted_hash: Some("sha256:abc123".to_string()), }, )]), } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index a736b7a1d..fc8d3b274 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -999,6 +999,9 @@ "properties": { "enabled": { "type": "boolean" + }, + "trusted_hash": { + "type": "string" } }, "type": "object" diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index d83307e80..3d81f05c7 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -20,6 +20,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; use core_test_support::PathExt; +use core_test_support::hooks::trusted_config_layer_stack; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; @@ -163,13 +164,24 @@ print({hook_output:?}) .to_string(), ) .expect("write hooks.json"); + let hook_list = codex_hooks::list_hooks(HooksConfig { + feature_enabled: true, + config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + ..HooksConfig::default() + }); + assert_eq!(hook_list.hooks.len(), 1); + let trusted_config_layer_stack = trusted_config_layer_stack( + &turn_context.config.config_layer_stack, + &turn_context.config.codex_home, + hook_list.hooks, + ); session .services .hooks .store(Arc::new(Hooks::new(HooksConfig { feature_enabled: true, - config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + config_layer_stack: Some(trusted_config_layer_stack), shell_program: (!cfg!(windows)).then_some("/bin/sh".to_string()), shell_args: if cfg!(windows) { Vec::new() diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index dbb0fab95..13fd3768b 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1173,15 +1173,17 @@ async fn reload_user_config_layer_refreshes_hooks() -> anyhow::Result<()> { .await?; let codex_home = session.codex_home().await; std::fs::create_dir_all(&codex_home)?; - std::fs::write( - codex_home.join(CONFIG_TOML_FILE), - r#" -[hooks] - -[[hooks.SessionStart]] -hooks = [{ type = "command", command = "python3 /tmp/user.py" }] -"#, - )?; + let config_toml_path = codex_home.join(CONFIG_TOML_FILE); + let user_config: codex_config::TomlValue = serde_json::from_value(serde_json::json!({ + "hooks": { + "SessionStart": [{ + "hooks": [{ + "type": "command", + "command": "python3 /tmp/user.py", + }], + }], + }, + }))?; let request = codex_hooks::SessionStartRequest { session_id: session.conversation_id, @@ -1193,6 +1195,39 @@ hooks = [{ type = "command", command = "python3 /tmp/user.py" }] }; assert!(session.hooks().preview_session_start(&request).is_empty()); + let config = session.get_config().await; + let hook_list = codex_hooks::list_hooks(codex_hooks::HooksConfig { + feature_enabled: true, + config_layer_stack: Some( + config + .config_layer_stack + .with_user_config(&config_toml_path, user_config.clone()), + ), + ..codex_hooks::HooksConfig::default() + }); + assert_eq!(hook_list.hooks.len(), 1); + assert_eq!( + hook_list.hooks[0].trust_status, + codex_protocol::protocol::HookTrustStatus::Untrusted + ); + + let trusted_user_config: codex_config::TomlValue = serde_json::from_value(serde_json::json!({ + "hooks": { + "SessionStart": [{ + "hooks": [{ + "type": "command", + "command": "python3 /tmp/user.py", + }], + }], + "state": { + hook_list.hooks[0].key.clone(): { + "trusted_hash": hook_list.hooks[0].current_hash.clone(), + }, + }, + }, + }))?; + std::fs::write(&config_toml_path, toml::to_string(&trusted_user_config)?)?; + session.reload_user_config_layer().await; assert_eq!(session.hooks().preview_session_start(&request).len(), 1); @@ -8568,17 +8603,27 @@ async fn session_start_hooks_only_load_from_trusted_project_layers() -> std::io: .build() .await?; - let preview = preview_session_start_hooks(&config).await?; + let hook_list = codex_hooks::list_hooks(codex_hooks::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config.config_layer_stack.clone()), + ..codex_hooks::HooksConfig::default() + }); let expected_source_path = codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path( nested_dot_codex.join("hooks.json"), )?; assert_eq!( - preview + hook_list + .hooks .iter() - .map(|run| &run.source_path) + .map(|hook| &hook.source_path) .collect::>(), vec![&expected_source_path], ); + assert_eq!( + hook_list.hooks[0].trust_status, + codex_protocol::protocol::HookTrustStatus::Untrusted + ); + assert!(preview_session_start_hooks(&config).await?.is_empty()); Ok(()) } @@ -8618,11 +8663,23 @@ async fn session_start_hooks_require_project_trust_without_config_toml() -> std: .build() .await?; + let hook_list = codex_hooks::list_hooks(codex_hooks::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config.config_layer_stack.clone()), + ..codex_hooks::HooksConfig::default() + }); assert_eq!( - preview_session_start_hooks(&config).await?.len(), + hook_list.hooks.len(), expected_hooks, - "unexpected hook count for {name}", + "unexpected discovered hook count for {name}", ); + assert!(preview_session_start_hooks(&config).await?.is_empty()); + if expected_hooks == 1 { + assert_eq!( + hook_list.hooks[0].trust_status, + codex_protocol::protocol::HookTrustStatus::Untrusted + ); + } } Ok(()) diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 84e469e22..7c2aa5e8e 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -371,6 +371,29 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul .to_string(), ) .context("write hooks.json")?; + let config_toml_path = turn_context + .config + .codex_home + .join(codex_config::CONFIG_TOML_FILE); + let hook_list = codex_hooks::list_hooks(HooksConfig { + feature_enabled: true, + config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + ..HooksConfig::default() + }); + assert_eq!(hook_list.hooks.len(), 1); + let trusted_config_layer_stack = turn_context.config.config_layer_stack.with_user_config( + &config_toml_path, + serde_json::from_value(serde_json::json!({ + "hooks": { + "state": { + hook_list.hooks[0].key.clone(): { + "trusted_hash": hook_list.hooks[0].current_hash.clone(), + }, + }, + }, + })) + .context("build trusted hook state")?, + ); let mut hook_shell_argv = session .user_shell() @@ -382,7 +405,7 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul .hooks .store(Arc::new(Hooks::new(HooksConfig { feature_enabled: true, - config_layer_stack: Some(turn_context.config.config_layer_stack.clone()), + config_layer_stack: Some(trusted_config_layer_stack), shell_program: Some(hook_shell_program), shell_args: hook_shell_argv, ..HooksConfig::default() diff --git a/codex-rs/core/tests/common/Cargo.toml b/codex-rs/core/tests/common/Cargo.toml index f710aa36c..c59edf57a 100644 --- a/codex-rs/core/tests/common/Cargo.toml +++ b/codex-rs/core/tests/common/Cargo.toml @@ -19,6 +19,7 @@ codex-config = { workspace = true } codex-core = { workspace = true } codex-exec-server = { workspace = true } codex-features = { workspace = true } +codex-hooks = { workspace = true } codex-login = { workspace = true } codex-model-provider-info = { workspace = true } codex-models-manager = { workspace = true } diff --git a/codex-rs/core/tests/common/hooks.rs b/codex-rs/core/tests/common/hooks.rs new file mode 100644 index 000000000..239041a22 --- /dev/null +++ b/codex-rs/core/tests/common/hooks.rs @@ -0,0 +1,70 @@ +use codex_config::CONFIG_TOML_FILE; +use codex_config::ConfigLayerStack; +use codex_config::TomlValue; +use codex_core::config::Config; +use codex_features::Feature; +use codex_hooks::HookListEntry; +use codex_utils_absolute_path::AbsolutePathBuf; + +pub fn trust_discovered_hooks(config: &mut Config) { + if let Err(err) = config.features.enable(Feature::CodexHooks) { + panic!("test config should allow feature update: {err}"); + } + + let listed = codex_hooks::list_hooks(codex_hooks::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config.config_layer_stack.clone()), + ..codex_hooks::HooksConfig::default() + }); + assert!( + !listed.hooks.is_empty(), + "trusted hook fixture should discover at least one hook" + ); + trust_hooks(config, listed.hooks); +} + +pub fn trust_hooks(config: &mut Config, hooks: Vec) { + config.config_layer_stack = + trusted_config_layer_stack(&config.config_layer_stack, &config.codex_home, hooks); +} + +pub fn trusted_config_layer_stack( + config_layer_stack: &ConfigLayerStack, + codex_home: &AbsolutePathBuf, + hooks: Vec, +) -> ConfigLayerStack { + let mut user_config = config_layer_stack + .get_user_layer() + .map(|layer| layer.config.clone()) + .unwrap_or_else(|| TomlValue::Table(Default::default())); + let Some(user_table) = user_config.as_table_mut() else { + panic!("user config should be a table"); + }; + let Some(hooks_table) = user_table + .entry("hooks") + .or_insert_with(|| TomlValue::Table(Default::default())) + .as_table_mut() + else { + panic!("hooks config should be a table"); + }; + let Some(state_table) = hooks_table + .entry("state") + .or_insert_with(|| TomlValue::Table(Default::default())) + .as_table_mut() + else { + panic!("hook state config should be a table"); + }; + for hook in hooks { + let mut hook_state = TomlValue::Table(Default::default()); + let Some(hook_state_table) = hook_state.as_table_mut() else { + panic!("hook state should be a table"); + }; + hook_state_table.insert( + "trusted_hash".to_string(), + TomlValue::String(hook.current_hash), + ); + state_table.insert(hook.key, hook_state); + } + + config_layer_stack.with_user_config(&codex_home.join(CONFIG_TOML_FILE), user_config) +} diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index d2ef9ddc6..70e1a3f0e 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -24,6 +24,7 @@ use std::path::PathBuf; pub mod apps_test_server; pub mod context_snapshot; +pub mod hooks; pub mod process; pub mod responses; pub mod streaming_sse; diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 58009e35d..92c8c10a0 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -3,8 +3,11 @@ use std::path::Path; use anyhow::Context; use anyhow::Result; +use codex_core::config::Config; use codex_core::config::Constrained; use codex_features::Feature; +use codex_plugin::PluginHookSource; +use codex_plugin::PluginId; use codex_protocol::items::parse_hook_prompt_fragment; use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; @@ -16,6 +19,9 @@ use codex_protocol::protocol::Op; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; use codex_protocol::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::hooks::trust_discovered_hooks; +use core_test_support::hooks::trust_hooks; use core_test_support::managed_network_requirements_loader; use core_test_support::responses::ev_apply_patch_function_call; use core_test_support::responses::ev_assistant_message; @@ -67,6 +73,23 @@ fn network_workspace_write_profile() -> PermissionProfile { ) } +fn trust_plugin_hooks(config: &mut Config, plugin_hook_sources: Vec) { + if let Err(err) = config.features.enable(Feature::CodexHooks) { + panic!("test config should allow feature update: {err}"); + } + let listed = codex_hooks::list_hooks(codex_hooks::HooksConfig { + feature_enabled: true, + config_layer_stack: Some(config.config_layer_stack.clone()), + plugin_hook_sources, + ..codex_hooks::HooksConfig::default() + }); + assert!( + !listed.hooks.is_empty(), + "trusted plugin hook fixture should discover at least one hook" + ); + trust_hooks(config, listed.hooks); +} + fn write_stop_hook(home: &Path, block_prompts: &[&str]) -> Result<()> { let script_path = home.join("stop_hook.py"); let log_path = home.join("stop_hook_log.jsonl"); @@ -835,12 +858,7 @@ async fn stop_hook_can_block_multiple_times_in_same_turn() -> Result<()> { panic!("failed to write stop hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("hello from the sea").await?; @@ -934,12 +952,7 @@ async fn session_start_hook_sees_materialized_transcript_path() -> Result<()> { panic!("failed to write session start hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("hello").await?; @@ -984,12 +997,7 @@ async fn session_start_hook_spills_large_additional_context() -> Result<()> { } } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("hello").await?; @@ -1041,12 +1049,7 @@ async fn stop_hook_spills_large_continuation_prompt() -> Result<()> { } } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("hello from the sea").await?; @@ -1091,12 +1094,7 @@ async fn resumed_thread_keeps_stop_continuation_prompt_in_history() -> Result<() panic!("failed to write stop hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let initial = initial_builder.build(&server).await?; let home = initial.home.clone(); let rollout_path = initial @@ -1119,12 +1117,7 @@ async fn resumed_thread_keeps_stop_continuation_prompt_in_history() -> Result<() ) .await; - let mut resume_builder = test_codex().with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + let mut resume_builder = test_codex().with_config(trust_discovered_hooks); let resumed = resume_builder.resume(&server, home, rollout_path).await?; resumed.submit_turn("and now continue").await?; @@ -1170,12 +1163,7 @@ async fn multiple_blocking_stop_hooks_persist_multiple_hook_prompt_fragments() - panic!("failed to write parallel stop hook fixtures: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("hello again").await?; @@ -1228,12 +1216,7 @@ async fn blocked_user_prompt_submit_persists_additional_context_for_next_turn() panic!("failed to write user prompt submit hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("blocked first prompt").await?; @@ -1335,12 +1318,7 @@ async fn blocked_queued_prompt_does_not_strand_earlier_accepted_prompt() -> Resu panic!("failed to write user prompt submit hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build_with_streaming_server(&server).await?; test.codex @@ -1489,12 +1467,7 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> panic!("failed to write permission request hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; fs::write(&marker, "seed").context("create permission request marker")?; @@ -1576,10 +1549,7 @@ async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; let target_path = test.workspace_path(&patch_path); @@ -1653,10 +1623,7 @@ async fn permission_request_hook_sees_raw_exec_command_input() -> Result<()> { }) .with_config(|config| { config.use_experimental_unified_exec_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); config .features .enable(Feature::UnifiedExec) @@ -1741,10 +1708,7 @@ allow_local_binding = true }) .with_cloud_requirements(managed_network_requirements_loader()) .with_config(move |config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); config.permissions.approval_policy = Constrained::allow_any(approval_policy); config .permissions @@ -1853,12 +1817,7 @@ async fn permission_request_hook_sees_retry_context_after_sandbox_denial() -> Re panic!("failed to write permission request hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; let marker_path = test.workspace_path(marker); let _ = fs::remove_file(&marker_path); @@ -1925,12 +1884,7 @@ async fn pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { panic!("failed to write pre tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; if marker.exists() { @@ -2027,12 +1981,7 @@ async fn pre_tool_use_records_additional_context_for_shell_command() -> Result<( panic!("failed to write pre tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("run the shell command with pre hook") @@ -2098,12 +2047,7 @@ async fn blocked_pre_tool_use_records_additional_context_for_shell_command() -> panic!("failed to write pre tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; if marker.exists() { @@ -2215,9 +2159,7 @@ print(json.dumps({{ ), ) .context("write plugin pre tool use hook script")?; - fs::write( - hooks_dir.join("hooks.json"), - r#"{ + let plugin_hooks_json = r#"{ "hooks": { "PreToolUse": [{ "matcher": "^Bash$", @@ -2227,21 +2169,34 @@ print(json.dumps({{ }] }] } -}"#, - ) - .context("write plugin hooks config")?; +}"#; + let plugin_hooks_path = hooks_dir.join("hooks.json"); + fs::write(&plugin_hooks_path, plugin_hooks_json).context("write plugin hooks config")?; + let plugin_root_abs = + AbsolutePathBuf::try_from(plugin_root.clone()).context("absolute plugin root")?; + let plugin_hooks_path_abs = + AbsolutePathBuf::try_from(plugin_hooks_path).context("absolute plugin hooks path")?; + let plugin_data_root = + AbsolutePathBuf::try_from(plugin_root.join("data")).context("absolute plugin data root")?; + let plugin_hook_sources = vec![PluginHookSource { + plugin_id: PluginId::parse("sample@test").context("plugin id")?, + plugin_root: plugin_root_abs, + plugin_data_root, + source_path: plugin_hooks_path_abs, + source_relative_path: "hooks/hooks.json".to_string(), + hooks: serde_json::from_str::(plugin_hooks_json) + .context("parse plugin hooks")? + .hooks, + }]; let mut builder = test_codex() .with_home(Arc::clone(&home)) - .with_config(|config| { + .with_config(move |config| { config .features .enable(Feature::Plugins) .expect("test config should allow feature update"); - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_plugin_hooks(config, plugin_hook_sources); config .features .enable(Feature::PluginHooks) @@ -2315,18 +2270,20 @@ async fn pre_tool_use_blocks_shell_when_defined_in_config_toml() -> Result<()> { ) .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 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}"); + } + }) + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; if marker.exists() { @@ -2397,21 +2354,23 @@ async fn pre_tool_use_merges_hooks_json_and_config_toml() -> Result<()> { ) .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 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}"); + } + }) + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("run the shell command with merged hook sources") @@ -2510,12 +2469,7 @@ async fn pre_tool_use_blocks_local_shell_before_execution() -> Result<()> { panic!("failed to write pre tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; if marker.exists() { @@ -2603,10 +2557,7 @@ async fn pre_tool_use_blocks_exec_command_before_execution() -> Result<()> { }) .with_config(|config| { config.use_experimental_unified_exec_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); config .features .enable(Feature::UnifiedExec) @@ -2693,10 +2644,7 @@ async fn pre_tool_use_blocks_apply_patch_before_execution() -> Result<()> { }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -2767,10 +2715,7 @@ async fn pre_tool_use_blocks_apply_patch_with_write_alias() -> Result<()> { }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -2843,12 +2788,7 @@ async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { panic!("failed to write pre tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("update the plan").await?; @@ -2912,12 +2852,7 @@ async fn post_tool_use_records_additional_context_for_shell_command() -> Result< panic!("failed to write post tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("run the shell command with post hook") @@ -3009,12 +2944,7 @@ async fn post_tool_use_block_decision_replaces_shell_command_output_with_reason( panic!("failed to write post tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("run the shell command with blocking post hook") @@ -3078,12 +3008,7 @@ async fn post_tool_use_continue_false_replaces_shell_command_output_with_stop_re panic!("failed to write post tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("run the shell command with stop-style post hook") @@ -3149,12 +3074,7 @@ async fn post_tool_use_records_additional_context_for_local_shell() -> Result<() panic!("failed to write post tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("run the local shell command with post hook") @@ -3222,10 +3142,7 @@ async fn post_tool_use_exit_two_replaces_one_shot_exec_command_output_with_feedb }) .with_config(|config| { config.use_experimental_unified_exec_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); config .features .enable(Feature::UnifiedExec) @@ -3300,10 +3217,7 @@ async fn post_tool_use_spills_large_feedback_message() -> Result<()> { }) .with_config(|config| { config.use_experimental_unified_exec_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); config .features .enable(Feature::UnifiedExec) @@ -3388,10 +3302,7 @@ async fn post_tool_use_blocks_when_exec_session_completes_via_write_stdin() -> R }) .with_config(|config| { config.use_experimental_unified_exec_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); config .features .enable(Feature::UnifiedExec) @@ -3475,10 +3386,7 @@ async fn post_tool_use_records_additional_context_for_apply_patch() -> Result<() }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -3566,10 +3474,7 @@ async fn post_tool_use_records_apply_patch_context_with_edit_alias() -> Result<( }) .with_config(|config| { config.include_apply_patch_tool = true; - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; @@ -3642,12 +3547,7 @@ async fn post_tool_use_does_not_fire_for_plan_tool() -> Result<()> { panic!("failed to write post tool use hook test fixture: {error}"); } }) - .with_config(|config| { - config - .features - .enable(Feature::CodexHooks) - .expect("test config should allow feature update"); - }); + .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; test.submit_turn("update the plan").await?; diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 2157630e0..26e305318 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -9,7 +9,7 @@ use codex_config::types::AppToolApproval; use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; use codex_core::config::Config; -use codex_features::Feature; +use core_test_support::hooks::trust_discovered_hooks; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call_with_namespace; @@ -163,9 +163,7 @@ fn enable_hooks_and_rmcp_server( rmcp_test_server_bin: String, approval_mode: AppToolApproval, ) { - if let Err(err) = config.features.enable(Feature::CodexHooks) { - panic!("test config should allow feature update: {err}"); - } + trust_discovered_hooks(config); insert_rmcp_test_server(config, rmcp_test_server_bin, approval_mode); } diff --git a/codex-rs/core/tests/suite/openai_file_mcp.rs b/codex-rs/core/tests/suite/openai_file_mcp.rs index ac49b5334..0f0dcf46f 100644 --- a/codex-rs/core/tests/suite/openai_file_mcp.rs +++ b/codex-rs/core/tests/suite/openai_file_mcp.rs @@ -12,6 +12,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use core_test_support::apps_test_server::AppsTestServer; use core_test_support::apps_test_server::DOCUMENT_EXTRACT_TEXT_RESOURCE_URI; +use core_test_support::hooks::trust_discovered_hooks; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call_with_namespace; @@ -162,9 +163,7 @@ async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Res }) .with_config(move |config| { configure_apps(config, apps_server.chatgpt_base_url.as_str()); - if let Err(err) = config.features.enable(Feature::CodexHooks) { - panic!("test config should allow feature update: {err}"); - } + trust_discovered_hooks(config); }); let test = builder.build(&server).await?; tokio::fs::write(test.cwd.path().join("report.txt"), b"hello world").await?; diff --git a/codex-rs/hooks/src/config_rules.rs b/codex-rs/hooks/src/config_rules.rs index b9fa87150..359c068ee 100644 --- a/codex-rs/hooks/src/config_rules.rs +++ b/codex-rs/hooks/src/config_rules.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::HashMap; use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; @@ -6,21 +6,21 @@ use codex_config::ConfigLayerStackOrdering; use codex_config::HookStateToml; use codex_config::TomlValue; -/// Build hook enablement rules from config layers that are allowed to override +/// Build effective hook state from config layers that are allowed to override /// user preferences. /// /// This intentionally reads only user and session flag layers, including /// disabled layers, to match the skills config behavior. Project, managed, and -/// plugin layers can discover hooks, but they do not get to write user -/// enablement state. -pub(crate) fn disabled_hook_keys_from_stack( +/// plugin layers can discover hooks, but they do not get to write user hook +/// state. +pub(crate) fn hook_states_from_stack( config_layer_stack: Option<&ConfigLayerStack>, -) -> HashSet { +) -> HashMap { let Some(config_layer_stack) = config_layer_stack else { - return HashSet::new(); + return HashMap::new(); }; - let mut disabled_keys = HashSet::new(); + let mut states: HashMap = HashMap::new(); for layer in config_layer_stack.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, /*include_disabled*/ true, @@ -54,21 +54,19 @@ pub(crate) fn disabled_hook_keys_from_stack( if key.is_empty() { continue; } - // Later layers win. Hooks without an explicit enabled override can - // still carry future per-hook state without changing enablement. - match state.enabled { - Some(false) => { - disabled_keys.insert(key.to_string()); - } - Some(true) => { - disabled_keys.remove(key); - } - None => {} + // Later layers win field-by-field so a future per-hook state write + // does not accidentally erase an existing enablement override. + let effective_state = states.entry(key.to_string()).or_default(); + if let Some(enabled) = state.enabled { + effective_state.enabled = Some(enabled); + } + if let Some(trusted_hash) = state.trusted_hash { + effective_state.trusted_hash = Some(trusted_hash); } } } - disabled_keys + states } #[cfg(test)] @@ -82,7 +80,7 @@ mod tests { use super::*; #[test] - fn disabled_hook_keys_from_stack_respects_layer_precedence() { + fn hook_states_from_stack_respects_layer_precedence() { let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; let stack = ConfigLayerStack::new( vec![ @@ -102,11 +100,65 @@ mod tests { ) .expect("config layer stack"); - assert_eq!(disabled_hook_keys_from_stack(Some(&stack)), HashSet::new()); + assert_eq!( + hook_states_from_stack(Some(&stack)), + HashMap::from([( + key.to_string(), + HookStateToml { + enabled: Some(true), + trusted_hash: None, + }, + )]) + ); } #[test] - fn disabled_hook_keys_from_stack_ignores_malformed_hook_events() { + fn hook_states_from_stack_merges_fields_across_layers() { + let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; + let stack = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: test_path_buf("/tmp/config.toml").abs(), + }, + config_with_hook_state( + key, + HookStateToml { + enabled: Some(/*enabled*/ false), + trusted_hash: None, + }, + ), + ), + ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + config_with_hook_state( + key, + HookStateToml { + enabled: None, + trusted_hash: Some("sha256:trusted".to_string()), + }, + ), + ), + ], + Default::default(), + Default::default(), + ) + .expect("config layer stack"); + + assert_eq!( + hook_states_from_stack(Some(&stack)), + HashMap::from([( + key.to_string(), + HookStateToml { + enabled: Some(false), + trusted_hash: Some("sha256:trusted".to_string()), + }, + )]) + ); + } + + #[test] + fn hook_states_from_stack_ignores_malformed_hook_events() { let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; let config: TomlValue = serde_json::from_value(serde_json::json!({ "hooks": { @@ -132,13 +184,19 @@ mod tests { .expect("config layer stack"); assert_eq!( - disabled_hook_keys_from_stack(Some(&stack)), - HashSet::from([key.to_string()]) + hook_states_from_stack(Some(&stack)), + HashMap::from([( + key.to_string(), + HookStateToml { + enabled: Some(false), + trusted_hash: None, + }, + )]) ); } #[test] - fn disabled_hook_keys_from_stack_ignores_malformed_state_entries() { + fn hook_states_from_stack_ignores_malformed_state_entries() { let key = "file:/tmp/hooks.json:pre_tool_use:0:0"; let config: TomlValue = serde_json::from_value(serde_json::json!({ "hooks": { @@ -166,16 +224,29 @@ mod tests { .expect("config layer stack"); assert_eq!( - disabled_hook_keys_from_stack(Some(&stack)), - HashSet::from([key.to_string()]) + hook_states_from_stack(Some(&stack)), + HashMap::from([( + key.to_string(), + HookStateToml { + enabled: Some(false), + trusted_hash: None, + }, + )]) ); } fn config_with_hook_override(key: &str, enabled: Option) -> TomlValue { - let hook_state = match enabled { - Some(enabled) => serde_json::json!({ "enabled": enabled }), - None => serde_json::json!({}), - }; + config_with_hook_state( + key, + HookStateToml { + enabled, + trusted_hash: None, + }, + ) + } + + fn config_with_hook_state(key: &str, state: HookStateToml) -> TomlValue { + let hook_state = serde_json::to_value(state).expect("hook state should serialize"); serde_json::from_value(serde_json::json!({ "hooks": { "state": { diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 8c520b749..f24da13bb 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -8,23 +8,27 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; use codex_config::HookEventsToml; use codex_config::HookHandlerConfig; +use codex_config::HookStateToml; use codex_config::HooksFile; use codex_config::ManagedHooksRequirementsToml; use codex_config::MatcherGroup; use codex_config::RequirementSource; +use codex_config::TomlValue; +use codex_config::version_for_toml; use codex_plugin::PluginHookSource; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; +use serde::Serialize; use std::collections::HashMap; -use std::collections::HashSet; use super::ConfiguredHandler; use super::HookListEntry; -use crate::config_rules::disabled_hook_keys_from_stack; +use crate::config_rules::hook_states_from_stack; use crate::events::common::matcher_pattern_for_event; use crate::events::common::validate_matcher_pattern; use codex_protocol::protocol::HookHandlerType; use codex_protocol::protocol::HookSource; +use codex_protocol::protocol::HookTrustStatus; pub(crate) struct DiscoveryResult { pub handlers: Vec, @@ -36,7 +40,8 @@ struct HookHandlerSource<'a> { path: &'a AbsolutePathBuf, key_source: String, source: HookSource, - disabled_hook_keys: &'a HashSet, + is_managed: bool, + hook_states: &'a HashMap, env: HashMap, plugin_id: Option, } @@ -50,7 +55,7 @@ pub(crate) fn discover_handlers( let mut hook_entries = Vec::new(); let mut warnings = plugin_hook_load_warnings; let mut display_order = 0_i64; - let disabled_hook_keys = disabled_hook_keys_from_stack(config_layer_stack); + let hook_states = hook_states_from_stack(config_layer_stack); if let Some(config_layer_stack) = config_layer_stack { append_managed_requirement_handlers( @@ -59,14 +64,14 @@ pub(crate) fn discover_handlers( &mut warnings, &mut display_order, config_layer_stack, - &disabled_hook_keys, + &hook_states, ); for layer in config_layer_stack.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, /*include_disabled*/ false, ) { - let hook_source = hook_source_for_config_layer_source(&layer.name); + let (hook_source, is_managed) = hook_metadata_for_config_layer_source(&layer.name); let json_hooks = load_hooks_json(layer.config_folder().as_deref(), &mut warnings); let toml_hooks = load_toml_hooks_from_layer(layer, &mut warnings); @@ -92,7 +97,8 @@ pub(crate) fn discover_handlers( path: &source_path, key_source: source_path.display().to_string(), source: hook_source, - disabled_hook_keys: &disabled_hook_keys, + is_managed, + hook_states: &hook_states, env: HashMap::new(), plugin_id: None, }, @@ -108,7 +114,7 @@ pub(crate) fn discover_handlers( &mut warnings, &mut display_order, plugin_hook_sources, - &disabled_hook_keys, + &hook_states, ); DiscoveryResult { @@ -124,7 +130,7 @@ fn append_managed_requirement_handlers( warnings: &mut Vec, display_order: &mut i64, config_layer_stack: &ConfigLayerStack, - disabled_hook_keys: &HashSet, + hook_states: &HashMap, ) { let Some(managed_hooks) = config_layer_stack.requirements().managed_hooks.as_ref() else { return; @@ -143,7 +149,8 @@ fn append_managed_requirement_handlers( path: &source_path, key_source: source_path.display().to_string(), source: hook_source_for_requirement_source(managed_hooks.source.as_ref()), - disabled_hook_keys, + is_managed: true, + hook_states, env: HashMap::new(), plugin_id: None, }, @@ -157,9 +164,8 @@ fn append_plugin_hook_sources( warnings: &mut Vec, display_order: &mut i64, plugin_hook_sources: Vec, - disabled_hook_keys: &HashSet, + hook_states: &HashMap, ) { - // TODO(abhinav): check enabled/trusted state here before plugin hooks become runnable. for source in plugin_hook_sources { let PluginHookSource { plugin_root, @@ -188,7 +194,8 @@ fn append_plugin_hook_sources( path: &source_path, key_source: format!("{plugin_id}:{source_relative_path}"), source: HookSource::Plugin, - disabled_hook_keys, + is_managed: false, + hook_states, env, plugin_id: Some(plugin_id), }, @@ -374,7 +381,7 @@ fn append_matcher_groups( )); continue; } - for (handler_index, handler) in group.hooks.into_iter().enumerate() { + for (handler_index, handler) in group.hooks.iter().cloned().enumerate() { match handler { HookHandlerConfig::Command { command, @@ -396,10 +403,18 @@ fn append_matcher_groups( )); continue; } + let timeout_sec = timeout_sec.unwrap_or(600).max(1); + let normalized_handler = HookHandlerConfig::Command { + command: command.clone(), + timeout_sec: Some(timeout_sec), + r#async, + status_message: status_message.clone(), + }; + let current_hash = + command_hook_hash(event_name, matcher, &group, normalized_handler); let command = source.env.iter().fold(command, |command, (key, value)| { command.replace(&format!("${{{key}}}"), value) }); - let timeout_sec = timeout_sec.unwrap_or(600).max(1); // TODO(abhinav): replace this positional suffix with a durable hook id. let key = format!( "{}:{}:{}:{}", @@ -408,8 +423,11 @@ fn append_matcher_groups( group_index, handler_index ); - let enabled = - source.source.is_managed() || !source.disabled_hook_keys.contains(&key); + let state = source.hook_states.get(&key); + let enabled = hook_enabled(source.is_managed, state); + let trusted_hash = hook_trusted_hash(source.is_managed, state); + let trust_status = + hook_trust_status(source.is_managed, ¤t_hash, trusted_hash); hook_entries.push(HookListEntry { key, event_name, @@ -423,9 +441,16 @@ fn append_matcher_groups( plugin_id: source.plugin_id.clone(), display_order: *display_order, enabled, - is_managed: source.source.is_managed(), + is_managed: source.is_managed, + current_hash, + trust_status, }); - if enabled { + if enabled + && matches!( + trust_status, + HookTrustStatus::Managed | HookTrustStatus::Trusted + ) + { handlers.push(ConfiguredHandler { event_name, matcher: matcher.map(ToOwned::to_owned), @@ -453,6 +478,34 @@ fn append_matcher_groups( } } +/// Hash a normalized, config-derived identity instead of source text so equivalent +/// hooks from config TOML and hooks.json converge on the same trust identity. +#[derive(Serialize)] +struct NormalizedHookIdentity { + event_name: &'static str, + #[serde(flatten)] + group: MatcherGroup, +} + +fn command_hook_hash( + event_name: codex_protocol::protocol::HookEventName, + matcher: Option<&str>, + group: &MatcherGroup, + normalized_handler: HookHandlerConfig, +) -> String { + let mut group = group.clone(); + group.matcher = matcher.map(ToOwned::to_owned); + group.hooks = vec![normalized_handler]; + let identity = NormalizedHookIdentity { + event_name: hook_event_key_label(event_name), + group, + }; + let Ok(value) = TomlValue::try_from(identity) else { + unreachable!("normalized hook identity should serialize to TOML"); + }; + version_for_toml(&value) +} + fn hook_event_key_label(event_name: codex_protocol::protocol::HookEventName) -> &'static str { match event_name { codex_protocol::protocol::HookEventName::PreToolUse => "pre_tool_use", @@ -464,17 +517,45 @@ fn hook_event_key_label(event_name: codex_protocol::protocol::HookEventName) -> } } -fn hook_source_for_config_layer_source(source: &ConfigLayerSource) -> HookSource { - match source { - ConfigLayerSource::System { .. } => HookSource::System, - ConfigLayerSource::User { .. } => HookSource::User, - ConfigLayerSource::Project { .. } => HookSource::Project, - ConfigLayerSource::Mdm { .. } => HookSource::Mdm, - ConfigLayerSource::SessionFlags => HookSource::SessionFlags, - ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } => { - HookSource::LegacyManagedConfigFile +fn hook_trust_status( + is_managed: bool, + current_hash: &str, + trusted_hash: Option<&str>, +) -> HookTrustStatus { + if is_managed { + HookTrustStatus::Managed + } else { + match trusted_hash { + Some(trusted_hash) if trusted_hash == current_hash => HookTrustStatus::Trusted, + Some(_) => HookTrustStatus::Modified, + None => HookTrustStatus::Untrusted, + } + } +} + +fn hook_enabled(is_managed: bool, state: Option<&HookStateToml>) -> bool { + is_managed || state.and_then(|state| state.enabled) != Some(false) +} + +fn hook_trusted_hash(is_managed: bool, state: Option<&HookStateToml>) -> Option<&str> { + (!is_managed) + .then(|| state.and_then(|state| state.trusted_hash.as_deref())) + .flatten() +} + +fn hook_metadata_for_config_layer_source(source: &ConfigLayerSource) -> (HookSource, bool) { + match source { + ConfigLayerSource::System { .. } => (HookSource::System, true), + ConfigLayerSource::User { .. } => (HookSource::User, false), + ConfigLayerSource::Project { .. } => (HookSource::Project, false), + ConfigLayerSource::Mdm { .. } => (HookSource::Mdm, true), + ConfigLayerSource::SessionFlags => (HookSource::SessionFlags, false), + ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } => { + (HookSource::LegacyManagedConfigFile, true) + } + ConfigLayerSource::LegacyManagedConfigTomlFromMdm => { + (HookSource::LegacyManagedConfigMdm, true) } - ConfigLayerSource::LegacyManagedConfigTomlFromMdm => HookSource::LegacyManagedConfigMdm, } } @@ -508,6 +589,7 @@ mod tests { use super::ConfiguredHandler; use super::append_matcher_groups; use codex_config::HookHandlerConfig; + use codex_config::HookStateToml; use codex_config::MatcherGroup; use codex_config::TomlValue; @@ -516,18 +598,19 @@ mod tests { } fn hook_source() -> HookSource { - HookSource::User + HookSource::System } fn hook_handler_source<'a>( path: &'a AbsolutePathBuf, - disabled_hook_keys: &'a std::collections::HashSet, + hook_states: &'a std::collections::HashMap, ) -> super::HookHandlerSource<'a> { super::HookHandlerSource { path, key_source: path.display().to_string(), source: hook_source(), - disabled_hook_keys, + is_managed: true, + hook_states, env: std::collections::HashMap::new(), plugin_id: None, } @@ -551,14 +634,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); - let disabled_hook_keys = std::collections::HashSet::new(); + let hook_states = std::collections::HashMap::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &disabled_hook_keys), + &hook_handler_source(&source_path, &hook_states), HookEventName::UserPromptSubmit, vec![command_group(Some("["))], ); @@ -586,14 +669,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); - let disabled_hook_keys = std::collections::HashSet::new(); + let hook_states = std::collections::HashMap::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &disabled_hook_keys), + &hook_handler_source(&source_path, &hook_states), HookEventName::PreToolUse, vec![command_group(Some("^Bash$"))], ); @@ -621,14 +704,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); - let disabled_hook_keys = std::collections::HashSet::new(); + let hook_states = std::collections::HashMap::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &disabled_hook_keys), + &hook_handler_source(&source_path, &hook_states), HookEventName::PreToolUse, vec![command_group(Some("*"))], ); @@ -644,14 +727,14 @@ mod tests { let mut warnings = Vec::new(); let mut display_order = 0; let source_path = source_path(); - let disabled_hook_keys = std::collections::HashSet::new(); + let hook_states = std::collections::HashMap::new(); append_matcher_groups( &mut handlers, &mut Vec::new(), &mut warnings, &mut display_order, - &hook_handler_source(&source_path, &disabled_hook_keys), + &hook_handler_source(&source_path, &hook_states), HookEventName::PostToolUse, vec![command_group(Some("Edit|Write"))], ); @@ -713,50 +796,50 @@ mod tests { } #[test] - fn hook_source_for_config_layer_source_discards_source_details() { + fn hook_metadata_for_config_layer_source_discards_source_details() { let config_file = test_path_buf("/tmp/.codex/config.toml").abs(); let dot_codex_folder = test_path_buf("/tmp/worktree/.codex").abs(); assert_eq!( - super::hook_source_for_config_layer_source(&ConfigLayerSource::System { + super::hook_metadata_for_config_layer_source(&ConfigLayerSource::System { file: config_file.clone(), }), - HookSource::System, + (HookSource::System, true), ); assert_eq!( - super::hook_source_for_config_layer_source(&ConfigLayerSource::User { + super::hook_metadata_for_config_layer_source(&ConfigLayerSource::User { file: config_file.clone(), }), - HookSource::User, + (HookSource::User, false), ); assert_eq!( - super::hook_source_for_config_layer_source(&ConfigLayerSource::Project { + super::hook_metadata_for_config_layer_source(&ConfigLayerSource::Project { dot_codex_folder }), - HookSource::Project, + (HookSource::Project, false), ); assert_eq!( - super::hook_source_for_config_layer_source(&ConfigLayerSource::Mdm { + super::hook_metadata_for_config_layer_source(&ConfigLayerSource::Mdm { domain: "com.openai.codex".to_string(), key: "config".to_string(), }), - HookSource::Mdm, + (HookSource::Mdm, true), ); assert_eq!( - super::hook_source_for_config_layer_source(&ConfigLayerSource::SessionFlags), - HookSource::SessionFlags, + super::hook_metadata_for_config_layer_source(&ConfigLayerSource::SessionFlags), + (HookSource::SessionFlags, false), ); assert_eq!( - super::hook_source_for_config_layer_source( + super::hook_metadata_for_config_layer_source( &ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: config_file }, ), - HookSource::LegacyManagedConfigFile, + (HookSource::LegacyManagedConfigFile, true), ); assert_eq!( - super::hook_source_for_config_layer_source( + super::hook_metadata_for_config_layer_source( &ConfigLayerSource::LegacyManagedConfigTomlFromMdm, ), - HookSource::LegacyManagedConfigMdm, + (HookSource::LegacyManagedConfigMdm, true), ); } } diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 37967862a..18262989e 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -26,6 +26,7 @@ use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookHandlerType; use codex_protocol::protocol::HookRunSummary; use codex_protocol::protocol::HookSource; +use codex_protocol::protocol::HookTrustStatus; use codex_utils_absolute_path::AbsolutePathBuf; #[derive(Debug, Clone)] @@ -84,6 +85,8 @@ pub struct HookListEntry { pub display_order: i64, pub enabled: bool, pub is_managed: bool, + pub current_hash: String, + pub trust_status: HookTrustStatus, } #[derive(Clone)] diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index c37539bb1..32739165f 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -22,6 +22,7 @@ use codex_protocol::ThreadId; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookSource; +use codex_protocol::protocol::HookTrustStatus; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -121,7 +122,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: assert!(engine.warnings().is_empty()); assert_eq!(engine.handlers.len(), 1); - assert!(engine.handlers[0].source.is_managed()); + assert_eq!(engine.handlers[0].source, HookSource::CloudRequirements); let listed = crate::list_hooks(crate::HooksConfig { legacy_notify_argv: None, feature_enabled: true, @@ -168,6 +169,68 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\"")); } +#[test] +fn unknown_requirement_source_hooks_stay_managed() { + let temp = tempdir().expect("create temp dir"); + let managed_dir = + AbsolutePathBuf::try_from(temp.path().join("managed-hooks")).expect("absolute path"); + fs::create_dir_all(managed_dir.as_path()).expect("create managed hooks dir"); + let managed_hooks = managed_hooks_for_current_platform( + managed_dir, + HookEventsToml { + pre_tool_use: vec![MatcherGroup { + matcher: Some("^Bash$".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "python3 /tmp/managed.py".to_string(), + timeout_sec: Some(10), + r#async: false, + status_message: Some("checking".to_string()), + }], + }], + ..Default::default() + }, + ); + let config_layer_stack = ConfigLayerStack::new( + Vec::new(), + ConfigRequirements { + managed_hooks: Some(ConstrainedWithSource::new( + Constrained::allow_any(managed_hooks.clone()), + Some(RequirementSource::Unknown), + )), + ..ConfigRequirements::default() + }, + ConfigRequirementsToml { + hooks: Some(managed_hooks), + ..ConfigRequirementsToml::default() + }, + ) + .expect("config layer stack"); + + let engine = ClaudeHooksEngine::new( + /*enabled*/ true, + Some(&config_layer_stack), + Vec::new(), + Vec::new(), + CommandShell { + program: String::new(), + args: Vec::new(), + }, + ); + + assert_eq!(engine.handlers.len(), 1); + assert_eq!(engine.handlers[0].source, HookSource::Unknown); + let discovered = + super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new()); + assert_eq!(discovered.hook_entries.len(), 1); + assert_eq!(discovered.hook_entries[0].source, HookSource::Unknown); + assert_eq!(discovered.hook_entries[0].enabled, true); + assert_eq!(discovered.hook_entries[0].is_managed, true); + assert_eq!( + discovered.hook_entries[0].trust_status, + HookTrustStatus::Managed + ); +} + #[test] fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() { let temp = tempdir().expect("create temp dir"); @@ -228,13 +291,17 @@ fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() { ); assert_eq!(engine.handlers.len(), 1); - assert!(engine.handlers[0].source.is_managed()); + assert_eq!(engine.handlers[0].source, HookSource::CloudRequirements); let discovered = super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new()); assert_eq!(discovered.hook_entries.len(), 2); assert_eq!(discovered.hook_entries[0].key, managed_disabled_key); assert_eq!(discovered.hook_entries[0].enabled, true); assert!(discovered.hook_entries[0].is_managed); + assert_eq!( + discovered.hook_entries[0].trust_status, + HookTrustStatus::Managed + ); assert_eq!(discovered.hook_entries[1].key, user_disabled_key); assert_eq!(discovered.hook_entries[1].enabled, false); assert!(!discovered.hook_entries[1].is_managed); @@ -281,13 +348,20 @@ fn user_disablement_does_not_filter_managed_layer_hooks() { ); assert_eq!(engine.handlers.len(), 1); - assert!(engine.handlers[0].source.is_managed()); + assert_eq!( + engine.handlers[0].source, + HookSource::LegacyManagedConfigFile + ); let discovered = super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new()); assert_eq!(discovered.hook_entries.len(), 1); assert_eq!(discovered.hook_entries[0].key, managed_key); assert_eq!(discovered.hook_entries[0].enabled, true); assert!(discovered.hook_entries[0].is_managed); + assert_eq!( + discovered.hook_entries[0].trust_status, + HookTrustStatus::Managed + ); } fn config_with_hook_state(key: &str, enabled: bool) -> TomlValue { @@ -339,6 +413,45 @@ fn config_with_pre_tool_use_hook(command: &str) -> TomlValue { .expect("config TOML should deserialize") } +fn trusted_plugin_hook_stack( + config_path: AbsolutePathBuf, + plugin_hook_sources: &[PluginHookSource], +) -> ConfigLayerStack { + let discovered = super::discovery::discover_handlers( + /*config_layer_stack*/ None, + plugin_hook_sources.to_vec(), + Vec::new(), + ); + let state = discovered + .hook_entries + .into_iter() + .map(|entry| { + ( + entry.key, + serde_json::json!({ + "trusted_hash": entry.current_hash, + }), + ) + }) + .collect::>(); + let config = serde_json::from_value(serde_json::json!({ + "hooks": { + "state": state, + }, + })) + .expect("config TOML should deserialize"); + + ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { file: config_path }, + config, + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("config layer stack") +} + #[test] fn requirements_managed_hooks_warn_when_managed_dir_is_missing() { let temp = tempdir().expect("create temp dir"); @@ -473,7 +586,7 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { config_table.insert("hooks".to_string(), hooks_table); let config_layer_stack = ConfigLayerStack::new( vec![ConfigLayerEntry::new( - ConfigLayerSource::User { + ConfigLayerSource::System { file: config_path.clone(), }, config_toml, @@ -514,11 +627,13 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() { tool_input: serde_json::json!({ "command": "echo hello" }), }); assert_eq!(preview.len(), 2); - assert!( + assert_eq!( engine .handlers .iter() - .all(|handler| !handler.source.is_managed()) + .map(|handler| handler.source) + .collect::>(), + vec![HookSource::System, HookSource::System] ); assert_eq!(preview[0].source_path, hooks_json_path); assert_eq!(preview[1].source_path, config_path); @@ -567,9 +682,13 @@ print(json.dumps({ ..Default::default() }, }]; + let config_layer_stack = trusted_plugin_hook_stack( + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"), + &plugin_hook_sources, + ); let engine = ClaudeHooksEngine::new( /*enabled*/ true, - /*config_layer_stack*/ None, + Some(&config_layer_stack), plugin_hook_sources.clone(), Vec::new(), CommandShell { @@ -671,9 +790,13 @@ fn plugin_hook_sources_expand_plugin_placeholders() { ..Default::default() }, }]; + let config_layer_stack = trusted_plugin_hook_stack( + AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"), + &plugin_hook_sources, + ); let engine = ClaudeHooksEngine::new( /*enabled*/ true, - /*config_layer_stack*/ None, + Some(&config_layer_stack), plugin_hook_sources, Vec::new(), CommandShell { diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index a84685cb9..3675806e6 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1559,19 +1559,13 @@ pub enum HookSource { Unknown, } -impl HookSource { - /// Returns whether hooks from this source are managed and therefore not - /// user-configurable. - pub fn is_managed(self) -> bool { - matches!( - self, - Self::System - | Self::Mdm - | Self::CloudRequirements - | Self::LegacyManagedConfigFile - | Self::LegacyManagedConfigMdm - ) - } +#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "snake_case")] +pub enum HookTrustStatus { + Managed, + Untrusted, + Trusted, + Modified, } #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] @@ -3996,20 +3990,6 @@ mod tests { use tempfile::NamedTempFile; use tempfile::TempDir; - #[test] - fn hook_source_managedness_is_source_derived() { - assert_eq!(HookSource::System.is_managed(), true); - assert_eq!(HookSource::Mdm.is_managed(), true); - assert_eq!(HookSource::CloudRequirements.is_managed(), true); - assert_eq!(HookSource::LegacyManagedConfigFile.is_managed(), true); - assert_eq!(HookSource::LegacyManagedConfigMdm.is_managed(), true); - assert_eq!(HookSource::User.is_managed(), false); - assert_eq!(HookSource::Project.is_managed(), false); - assert_eq!(HookSource::SessionFlags.is_managed(), false); - assert_eq!(HookSource::Plugin.is_managed(), false); - assert_eq!(HookSource::Unknown.is_managed(), false); - } - fn sorted_writable_roots(roots: Vec) -> Vec<(PathBuf, Vec)> { let mut sorted_roots: Vec<(PathBuf, Vec)> = roots .into_iter() diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 66c5dfd18..0ecbd02d6 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -953,6 +953,7 @@ See the Codex keymap documentation for supported actions and examples." tui.frame_requester().schedule_frame(); app.refresh_startup_skills(&app_server); + app.refresh_startup_hooks(&app_server); // Kick off a non-blocking rate-limit prefetch so the first `/status` // already has data, without delaying the initial frame render. if requires_openai_auth && has_chatgpt_account { diff --git a/codex-rs/tui/src/app/background_requests.rs b/codex-rs/tui/src/app/background_requests.rs index 36155fb33..3233db827 100644 --- a/codex-rs/tui/src/app/background_requests.rs +++ b/codex-rs/tui/src/app/background_requests.rs @@ -5,6 +5,7 @@ //! the main event loop remains single-threaded. use super::*; +use codex_app_server_protocol::HookTrustStatus; use codex_app_server_protocol::MarketplaceAddParams; use codex_app_server_protocol::MarketplaceAddResponse; use codex_app_server_protocol::MarketplaceRemoveParams; @@ -88,6 +89,47 @@ impl App { }); } + /// Emits the initial hook review warning without delaying the first interactive frame. + pub(super) fn refresh_startup_hooks(&mut self, app_server: &AppServerSession) { + let request_handle = app_server.request_handle(); + let app_event_tx = self.app_event_tx.clone(); + let cwd = self.config.cwd.to_path_buf(); + tokio::spawn(async move { + let result = fetch_hooks_list(request_handle, cwd.clone()).await; + let response = match result { + Ok(response) => response, + Err(err) => { + tracing::warn!("failed to load startup hook review state: {err:#}"); + return; + } + }; + let hooks_needing_review = response + .data + .into_iter() + .find(|entry| entry.cwd.as_path() == cwd.as_path()) + .map(|entry| { + entry + .hooks + .into_iter() + .filter(|hook| { + matches!( + hook.trust_status, + HookTrustStatus::Untrusted | HookTrustStatus::Modified + ) + }) + .count() + }) + .unwrap_or_default(); + if let Some(message) = + startup_prompts::hooks_needing_review_warning(hooks_needing_review) + { + app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( + history_cell::new_warning_event(message), + ))); + } + }); + } + pub(super) fn fetch_plugins_list(&mut self, app_server: &AppServerSession, cwd: PathBuf) { let request_handle = app_server.request_handle(); let app_event_tx = self.app_event_tx.clone(); @@ -322,6 +364,23 @@ impl App { }); } + pub(super) fn trust_hook( + &mut self, + app_server: &AppServerSession, + key: String, + current_hash: String, + ) { + let request_handle = app_server.request_handle(); + let app_event_tx = self.app_event_tx.clone(); + tokio::spawn(async move { + let result = write_hook_trust(request_handle, key, current_hash) + .await + .map(|_| ()) + .map_err(|err| format!("Failed to trust hook: {err}")); + app_event_tx.send(AppEvent::HookTrusted { result }); + }); + } + pub(super) fn refresh_plugin_mentions(&mut self) { let config = self.config.clone(); let app_event_tx = self.app_event_tx.clone(); @@ -805,6 +864,35 @@ pub(super) async fn write_hook_enabled( .wrap_err("config/batchWrite failed while updating hook enablement in TUI") } +pub(super) async fn write_hook_trust( + request_handle: AppServerRequestHandle, + key: String, + current_hash: String, +) -> Result { + let request_id = RequestId::String(format!("hooks-config-write-{}", Uuid::new_v4())); + let value = serde_json::json!({ + key: { + "trusted_hash": current_hash, + } + }); + request_handle + .request_typed(ClientRequest::ConfigBatchWrite { + request_id, + params: ConfigBatchWriteParams { + edits: vec![codex_app_server_protocol::ConfigEdit { + key_path: "hooks.state".to_string(), + value, + merge_strategy: MergeStrategy::Upsert, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }, + }) + .await + .wrap_err("config/batchWrite failed while updating hook trust in TUI") +} + pub(super) fn build_feedback_upload_params( origin_thread_id: Option, rollout_path: Option, diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 4536688a9..068084839 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -1699,6 +1699,9 @@ impl App { AppEvent::SetHookEnabled { key, enabled } => { self.set_hook_enabled(app_server, key, enabled); } + AppEvent::TrustHook { key, current_hash } => { + self.trust_hook(app_server, key, current_hash); + } AppEvent::HookEnabledSet { key, enabled, @@ -1723,6 +1726,11 @@ impl App { } } } + AppEvent::HookTrusted { result } => { + if let Err(err) = result { + self.chat_widget.add_error_message(err); + } + } AppEvent::OpenPermissionsPopup => { self.chat_widget.open_permissions_popup(); } diff --git a/codex-rs/tui/src/app/startup_prompts.rs b/codex-rs/tui/src/app/startup_prompts.rs index 41972e675..482c75b3f 100644 --- a/codex-rs/tui/src/app/startup_prompts.rs +++ b/codex-rs/tui/src/app/startup_prompts.rs @@ -77,6 +77,16 @@ pub(super) fn emit_system_bwrap_warning(app_event_tx: &AppEventSender, config: & ))); } +pub(super) fn hooks_needing_review_warning(count: usize) -> Option { + match count { + 0 => None, + 1 => Some("1 hook needs review before it can run. Open /hooks to review it.".to_string()), + count => Some(format!( + "{count} hooks need review before they can run. Open /hooks to review them." + )), + } +} + pub(super) fn should_show_model_migration_prompt( current_model: &str, target_model: &str, diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index dacfabcb7..3de69743b 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -299,6 +299,17 @@ async fn ignore_same_thread_resume_allows_reattaching_displayed_inactive_thread( assert!(app.transcript_cells.is_empty()); } +#[test] +fn hooks_needing_review_startup_warning_snapshot() { + let message = startup_prompts::hooks_needing_review_warning(/*count*/ 2) + .expect("review-needed hooks should produce a startup warning"); + let rendered = lines_to_single_string( + &history_cell::new_warning_event(message).display_lines(/*width*/ 80), + ); + + assert_app_snapshot!("hooks_needing_review_startup_warning", rendered); +} + #[tokio::test] async fn enqueue_primary_thread_session_replays_buffered_approval_after_attach() -> Result<()> { let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 89b19a49e..c88ff1711 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -758,6 +758,12 @@ pub(crate) enum AppEvent { enabled: bool, }, + /// Trust the current definition for a hook by stable hook key. + TrustHook { + key: String, + current_hash: String, + }, + /// Result of persisting hook enabled state. HookEnabledSet { key: String, @@ -765,6 +771,11 @@ pub(crate) enum AppEvent { result: Result<(), String>, }, + /// Result of persisting hook trust state. + HookTrusted { + result: Result<(), String>, + }, + /// Notify that the manage skills popup was closed. ManageSkillsClosed, diff --git a/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs b/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs index 2f4c6a8a0..c146bae8b 100644 --- a/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs +++ b/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs @@ -2,6 +2,7 @@ use codex_app_server_protocol::HookErrorInfo; use codex_app_server_protocol::HookEventName; use codex_app_server_protocol::HookMetadata; use codex_app_server_protocol::HookSource; +use codex_app_server_protocol::HookTrustStatus; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -67,7 +68,12 @@ impl HooksBrowserView { app_event_tx, }; if view.page_len() > 0 { - view.state.selected_idx = Some(0); + view.state.selected_idx = Some( + view.event_rows() + .iter() + .position(|row| row.needs_review > 0) + .unwrap_or(0), + ); } view } @@ -84,14 +90,18 @@ impl HooksBrowserView { let active = self .hooks .iter() - .filter(|hook| { - hook.event_name == event_name && (hook.enabled || hook.is_managed) - }) + .filter(|hook| hook.event_name == event_name && hook_is_active(hook)) + .count(); + let needs_review = self + .hooks + .iter() + .filter(|hook| hook.event_name == event_name && hook_needs_review(hook)) .count(); EventRow { event_name, installed, active, + needs_review, } }) .collect() @@ -169,6 +179,9 @@ impl HooksBrowserView { if hook.is_managed { return; } + if hook_needs_review(hook) { + return; + } hook.enabled = !hook.enabled; self.app_event_tx.send(AppEvent::SetHookEnabled { @@ -177,6 +190,24 @@ impl HooksBrowserView { }); } + fn trust_selected_hook(&mut self, event_name: HookEventName) { + let Some(idx) = self.selected_hook_index(event_name) else { + return; + }; + let Some(hook) = self.hooks.get_mut(idx) else { + return; + }; + if !hook_needs_review(hook) { + return; + } + + hook.trust_status = HookTrustStatus::Trusted; + self.app_event_tx.send(AppEvent::TrustHook { + key: hook.key.clone(), + current_hash: hook.current_hash.clone(), + }); + } + fn close(&mut self) { self.complete = true; } @@ -205,26 +236,50 @@ impl HooksBrowserView { ] } - fn handler_header_lines(event_name: HookEventName) -> Vec> { - vec![ - format!("{} hooks", event_label(event_name)).bold().into(), - "Turn hooks on or off. Your changes are saved automatically." - .dim() - .into(), - ] + fn handler_header_lines( + event_name: HookEventName, + review_needed_count: usize, + ) -> Vec> { + let mut lines = vec![format!("{} hooks", event_label(event_name)).bold().into()]; + match review_needed_count { + 0 => lines.push( + "Turn hooks on or off. Your changes are saved automatically." + .dim() + .into(), + ), + 1 => lines.push("1 hook needs review before it can run.".dim().into()), + count => lines.push( + format!("{count} hooks need review before they can run.") + .dim() + .into(), + ), + } + lines + } + + fn review_needed_count(&self, event_name: HookEventName) -> usize { + self.handlers_for_event(event_name) + .filter(|hook| hook_needs_review(hook)) + .count() } fn event_table_lines(&self) -> Vec> { + let rows = self.event_rows(); + let show_review = rows.iter().any(|row| row.needs_review > 0); let mut lines = Vec::new(); - lines.push(Line::from(vec![ + let mut header = vec![ format!("{: { + format!("[{marker}] {} · modified", hook_title(idx)) + } + HookTrustStatus::Untrusted => format!("[{marker}] {} · new", hook_title(idx)), + HookTrustStatus::Managed | HookTrustStatus::Trusted => { + format!("[{marker}] {}", hook_title(idx)) + } + }; let mut line = Line::from(row); line = truncate_line_with_ellipsis_if_overflow(line, width); if hook.is_managed { @@ -335,6 +414,7 @@ impl HooksBrowserView { Some(MAX_COMMAND_DETAIL_LINES), )); lines.push(detail_line("Timeout", &format!("{}s", hook.timeout_sec))); + lines.push(detail_line("Trust", hook_trust_label(hook.trust_status))); lines } @@ -367,6 +447,14 @@ impl HooksBrowserView { key_hint::plain(KeyCode::Esc).into(), " to go back".into(), ]) + } else if selected_hook.is_some_and(hook_needs_review) { + Line::from(vec![ + "Press ".into(), + key_hint::plain(KeyCode::Char('t')).into(), + " to trust; ".into(), + key_hint::plain(KeyCode::Esc).into(), + " to go back".into(), + ]) } else { Line::from(vec![ "Press ".into(), @@ -427,6 +515,15 @@ impl BottomPaneView for HooksBrowserView { self.toggle_selected_hook(event_name); } } + KeyEvent { + code: KeyCode::Char('t'), + modifiers: KeyModifiers::NONE, + .. + } => { + if let HooksBrowserPage::Handlers(event_name) = self.page { + self.trust_selected_hook(event_name); + } + } KeyEvent { code: KeyCode::Esc, .. } => match self.page { @@ -458,11 +555,14 @@ impl Renderable for HooksBrowserView { HooksBrowserPage::Events => self.event_page_lines().len(), HooksBrowserPage::Handlers(event_name) => { let row_count = self.handler_row_lines(event_name, content_width).len(); + let header_line_count = + Self::handler_header_lines(event_name, self.review_needed_count(event_name)) + .len(); if row_count == 0 { - Self::handler_header_lines(event_name).len() + 2 + header_line_count + 2 } else { let visible_row_count = row_count.min(MAX_POPUP_ROWS); - Self::handler_header_lines(event_name).len() + header_line_count + 1 + visible_row_count + 1 @@ -485,7 +585,8 @@ impl Renderable for HooksBrowserView { let lines = match self.page { HooksBrowserPage::Events => self.event_page_lines(), HooksBrowserPage::Handlers(event_name) => { - let mut lines = Self::handler_header_lines(event_name); + let mut lines = + Self::handler_header_lines(event_name, self.review_needed_count(event_name)); let rows = self.handler_row_lines(event_name, width); if rows.is_empty() { lines.push(Line::default()); @@ -525,10 +626,35 @@ impl Renderable for HooksBrowserView { } } +fn hook_is_active(hook: &HookMetadata) -> bool { + hook.enabled + && matches!( + hook.trust_status, + HookTrustStatus::Managed | HookTrustStatus::Trusted + ) +} + struct EventRow { event_name: HookEventName, installed: usize, active: usize, + needs_review: usize, +} + +fn hook_needs_review(hook: &HookMetadata) -> bool { + matches!( + hook.trust_status, + HookTrustStatus::Untrusted | HookTrustStatus::Modified + ) +} + +fn hook_trust_label(status: HookTrustStatus) -> &'static str { + match status { + HookTrustStatus::Managed => "Managed", + HookTrustStatus::Trusted => "Trusted", + HookTrustStatus::Untrusted => "New hook - review required", + HookTrustStatus::Modified => "Modified since last trusted - review required", + } } fn event_label(event_name: HookEventName) -> &'static str { @@ -661,6 +787,7 @@ mod tests { use codex_app_server_protocol::HookHandlerType; use codex_app_server_protocol::HookMetadata; use codex_app_server_protocol::HookSource; + use codex_app_server_protocol::HookTrustStatus; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use insta::assert_snapshot; @@ -706,6 +833,7 @@ mod tests { is_managed: bool, display_order: i64, ) -> HookMetadata { + let current_hash = "sha256:current".to_string(); HookMetadata { key: key.to_string(), event_name, @@ -720,6 +848,12 @@ mod tests { plugin_id: plugin_id.map(str::to_string), display_order, enabled, + current_hash, + trust_status: if is_managed { + HookTrustStatus::Managed + } else { + HookTrustStatus::Trusted + }, } } @@ -770,6 +904,33 @@ mod tests { assert_snapshot!("hooks_browser_events", render_lines(&view, /*width*/ 112)); } + #[test] + fn renders_event_browser_with_review_column_when_needed() { + let (tx_raw, _rx) = unbounded_channel::(); + let mut untrusted_hook = hook( + "path:untrusted", + HookEventName::PreToolUse, + HookSource::User, + /*plugin_id*/ None, + "/tmp/pre-tool-use-check.sh", + /*enabled*/ false, + /*is_managed*/ false, + /*display_order*/ 0, + ); + untrusted_hook.trust_status = HookTrustStatus::Untrusted; + let view = HooksBrowserView::new( + vec![untrusted_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + + assert_snapshot!( + "hooks_browser_events_with_review_column", + render_lines(&view, /*width*/ 112) + ); + } + #[test] fn renders_event_browser_with_issues() { let (tx_raw, _rx) = unbounded_channel::(); @@ -796,6 +957,34 @@ mod tests { assert_snapshot!("hooks_browser_handlers", render_lines(&view, /*width*/ 112)); } + #[test] + fn renders_untrusted_enabled_handler_as_inactive() { + let (tx_raw, _rx) = unbounded_channel::(); + let mut untrusted_hook = hook( + "path:untrusted", + HookEventName::PreToolUse, + HookSource::User, + /*plugin_id*/ None, + "~/bin/untrusted.sh", + /*enabled*/ true, + /*is_managed*/ false, + /*display_order*/ 0, + ); + untrusted_hook.trust_status = HookTrustStatus::Untrusted; + let mut view = HooksBrowserView::new( + vec![untrusted_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + assert_snapshot!( + "hooks_browser_untrusted_enabled_handler", + render_lines(&view, /*width*/ 112) + ); + } + #[test] fn renders_managed_handler_without_toggle_hint() { let mut view = view(); @@ -928,7 +1117,7 @@ mod tests { HookSource::System, /*plugin_id*/ None, "/enterprise/hooks/pre-tool-use-check.sh", - /*enabled*/ false, + /*enabled*/ true, /*is_managed*/ true, /*display_order*/ 0, )], @@ -947,6 +1136,93 @@ mod tests { assert_eq!(pre_tool_use.active, 1); } + #[test] + fn review_needed_hooks_are_not_active() { + let (tx_raw, _rx) = unbounded_channel::(); + let mut untrusted_hook = hook( + "path:untrusted", + HookEventName::PreToolUse, + HookSource::User, + /*plugin_id*/ None, + "/tmp/pre-tool-use-check.sh", + /*enabled*/ true, + /*is_managed*/ false, + /*display_order*/ 0, + ); + untrusted_hook.trust_status = HookTrustStatus::Untrusted; + let view = HooksBrowserView::new( + vec![untrusted_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + + let rows = view.event_rows(); + let pre_tool_use = rows + .into_iter() + .find(|row| row.event_name == HookEventName::PreToolUse) + .expect("pre tool use row"); + + assert_eq!(pre_tool_use.installed, 1); + assert_eq!(pre_tool_use.active, 0); + assert_eq!(pre_tool_use.needs_review, 1); + } + + #[test] + fn review_needed_event_is_selected_by_default() { + let (tx_raw, _rx) = unbounded_channel::(); + let mut untrusted_hook = hook( + "path:untrusted", + HookEventName::PermissionRequest, + HookSource::User, + /*plugin_id*/ None, + "/tmp/permission-request-check.sh", + /*enabled*/ false, + /*is_managed*/ false, + /*display_order*/ 0, + ); + untrusted_hook.trust_status = HookTrustStatus::Untrusted; + let view = HooksBrowserView::new( + vec![untrusted_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + + assert_eq!( + view.selected_event(), + Some(HookEventName::PermissionRequest) + ); + } + + #[test] + fn renders_review_needed_handler() { + let (tx_raw, _rx) = unbounded_channel::(); + let mut untrusted_hook = hook( + "path:untrusted", + HookEventName::PreToolUse, + HookSource::User, + /*plugin_id*/ None, + "/tmp/pre-tool-use-check.sh", + /*enabled*/ false, + /*is_managed*/ false, + /*display_order*/ 0, + ); + untrusted_hook.trust_status = HookTrustStatus::Untrusted; + let mut view = HooksBrowserView::new( + vec![untrusted_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + assert_snapshot!( + "hooks_browser_review_needed_handler", + render_lines(&view, /*width*/ 112) + ); + } + fn assert_unmanaged_toggle_key(key_code: KeyCode) { let (tx_raw, mut rx) = unbounded_channel::(); let mut view = HooksBrowserView::new( @@ -1007,6 +1283,81 @@ mod tests { assert!(rx.try_recv().is_err()); } + #[test] + fn trust_key_trusts_review_needed_handler_without_changing_enablement() { + let (tx_raw, mut rx) = unbounded_channel::(); + let mut untrusted_hook = hook( + "path:untrusted", + HookEventName::PreToolUse, + HookSource::User, + /*plugin_id*/ None, + "/tmp/pre-tool-use-check.sh", + /*enabled*/ false, + /*is_managed*/ false, + /*display_order*/ 0, + ); + untrusted_hook.trust_status = HookTrustStatus::Untrusted; + let current_hash = untrusted_hook.current_hash.clone(); + let mut view = HooksBrowserView::new( + vec![untrusted_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + view.handle_key_event(KeyEvent::from(KeyCode::Char('t'))); + + match rx.try_recv().expect("trust event") { + AppEvent::TrustHook { + key, + current_hash: hash_to_trust, + } => { + assert_eq!(key, "path:untrusted"); + assert_eq!(hash_to_trust, current_hash); + } + other => panic!("expected hook trust event, got {other:?}"), + } + } + + #[test] + fn trust_key_preserves_disabled_modified_handler() { + let (tx_raw, mut rx) = unbounded_channel::(); + let mut modified_hook = hook( + "path:modified", + HookEventName::PreToolUse, + HookSource::User, + /*plugin_id*/ None, + "/tmp/pre-tool-use-check.sh", + /*enabled*/ false, + /*is_managed*/ false, + /*display_order*/ 0, + ); + modified_hook.trust_status = HookTrustStatus::Modified; + let current_hash = modified_hook.current_hash.clone(); + let mut view = HooksBrowserView::new( + vec![modified_hook], + Vec::new(), + Vec::new(), + AppEventSender::new(tx_raw), + ); + view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + view.handle_key_event(KeyEvent::from(KeyCode::Char('t'))); + + let hook = view.hooks.first().expect("trusted hook"); + assert!(!hook.enabled); + assert_eq!(hook.trust_status, HookTrustStatus::Trusted); + match rx.try_recv().expect("trust event") { + AppEvent::TrustHook { + key, + current_hash: hash_to_trust, + } => { + assert_eq!(key, "path:modified"); + assert_eq!(hash_to_trust, current_hash); + } + other => panic!("expected hook trust event, got {other:?}"), + } + } + #[test] fn escape_returns_to_the_selected_event() { let mut view = view(); diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_capped_command_details.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_capped_command_details.snap index 7af93e3c5..808b9dedb 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_capped_command_details.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_capped_command_details.snap @@ -15,5 +15,6 @@ expression: "render_lines(&view, 44)" seven eight nine ten eleven twelve thirteen fourteen… Timeout 30s + Trust Trusted Press space or enter to toggle; esc to go diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_events_with_review_column.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_events_with_review_column.snap new file mode 100644 index 000000000..85e930c68 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_events_with_review_column.snap @@ -0,0 +1,17 @@ +--- +source: tui/src/bottom_pane/hooks_browser_view.rs +expression: "render_lines(&view, 112)" +--- + + Hooks + Lifecycle hooks from config and enabled plugins. + + Event Installed Active Review Description + PreToolUse 1 0 1 Before a tool executes + PermissionRequest 0 0 0 When permission is requested + PostToolUse 0 0 0 After a tool executes + SessionStart 0 0 0 When a new session starts + UserPromptSubmit 0 0 0 When the user submits a prompt + Stop 0 0 0 Right before Codex ends its turn + + Press enter to view hooks; esc to close diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_handlers.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_handlers.snap index c44f4b866..6e8873498 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_handlers.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_handlers.snap @@ -14,5 +14,6 @@ expression: "render_lines(&view, 112)" Source Plugin - superpowers@openai-curated Command ${CODEX_PLUGIN_ROOT}/hooks/pre-tool-use-check.sh Timeout 30s + Trust Trusted Press space or enter to toggle; esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_managed_handler.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_managed_handler.snap index 21c59065f..d073b11b3 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_managed_handler.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_managed_handler.snap @@ -13,5 +13,6 @@ expression: "render_lines(&view, 112)" Source Admin config Command /enterprise/hooks/permission-check.sh Timeout 30s + Trust Managed Managed hooks are always on; press esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_review_needed_handler.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_review_needed_handler.snap new file mode 100644 index 000000000..b4a5c117e --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_review_needed_handler.snap @@ -0,0 +1,18 @@ +--- +source: tui/src/bottom_pane/hooks_browser_view.rs +expression: "render_lines(&view, 112)" +--- + + PreToolUse hooks + 1 hook needs review before it can run. + + [!] Hook 1 · new + + Event PreToolUse + Matcher Bash + Source User config - /tmp/hooks.json + Command /tmp/pre-tool-use-check.sh + Timeout 30s + Trust New hook - review required + + Press t to trust; esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_scrolled_handlers.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_scrolled_handlers.snap index 4f4a4377c..efeb0b240 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_scrolled_handlers.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_scrolled_handlers.snap @@ -20,5 +20,6 @@ expression: "render_lines(&view, 112)" Source User config - /tmp/hooks.json Command /tmp/hook-8.sh Timeout 30s + Trust Trusted Press space or enter to toggle; esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_selected_managed_handler.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_selected_managed_handler.snap index 9a53b95d6..514a8917a 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_selected_managed_handler.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_selected_managed_handler.snap @@ -14,5 +14,6 @@ expression: "render_lines(&view, 112)" Source Admin config Command /enterprise/hooks/pre-tool-use-2.sh Timeout 30s + Trust Managed Managed hooks are always on; press esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_untrusted_enabled_handler.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_untrusted_enabled_handler.snap new file mode 100644 index 000000000..4fa01776f --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__hooks_browser_view__tests__hooks_browser_untrusted_enabled_handler.snap @@ -0,0 +1,18 @@ +--- +source: tui/src/bottom_pane/hooks_browser_view.rs +expression: "render_lines(&view, 112)" +--- + + PreToolUse hooks + 1 hook needs review before it can run. + + [!] Hook 1 · new + + Event PreToolUse + Matcher Bash + Source User config - /tmp/hooks.json + Command ~/bin/untrusted.sh + Timeout 30s + Trust New hook - review required + + Press t to trust; esc to go back diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__hooks_needing_review_startup_warning.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__hooks_needing_review_startup_warning.snap new file mode 100644 index 000000000..f044b95e8 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__hooks_needing_review_startup_warning.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/app/tests.rs +expression: rendered +--- +⚠ 2 hooks need review before they can run. Open /hooks to review them.