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 c105c371955a..8a4919d25773 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 @@ -10375,6 +10375,11 @@ "null" ] }, + "failClosed": { + "default": false, + "description": "When true, a failure of this hook blocks the tool instead of failing open.", + "type": "boolean" + }, "statusMessage": { "type": [ "string", 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 1a38e4fe91b7..5e4aa94961ba 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 @@ -6211,6 +6211,11 @@ "null" ] }, + "failClosed": { + "default": false, + "description": "When true, a failure of this hook blocks the tool instead of failing open.", + "type": "boolean" + }, "statusMessage": { "type": [ "string", diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json index f2faa534962d..f8f730eb3b19 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigRequirementsReadResponse.json @@ -652,6 +652,11 @@ "null" ] }, + "failClosed": { + "default": false, + "description": "When true, a failure of this hook blocks the tool instead of failing open.", + "type": "boolean" + }, "statusMessage": { "type": [ "string", diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst index eef430182091..4ff8f33dd72c 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index eb2ee812b34f..e72125317d44 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts index 6ee1b16ba36e..77b8a25110fa 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ConfiguredHookHandler.ts @@ -10,4 +10,8 @@ export type ConfiguredHookHandler = { "type": "command", command: string, comman * evaluated against the original context; a spilled preview also includes recovery * metadata. */ -additionalContextLimit: number | null, } | { "type": "mcp_tool", server: string, tool: string, input: { [key in string]?: JsonValue }, timeoutSec: bigint | null, statusMessage: string | null, } | { "type": "prompt", } | { "type": "agent", }; +additionalContextLimit: number | null, +/** + * When true, a failure of this hook blocks the tool instead of failing open. + */ +failClosed: boolean, } | { "type": "mcp_tool", server: string, tool: string, input: { [key in string]?: JsonValue }, timeoutSec: bigint | null, statusMessage: string | null, } | { "type": "prompt", } | { "type": "agent", }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/config.rs b/codex-rs/app-server-protocol/src/protocol/v2/config.rs index 29539116025c..ee5c0ec5c585 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/config.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/config.rs @@ -685,6 +685,10 @@ pub enum ConfiguredHookHandler { #[serde(rename = "additionalContextLimit")] #[ts(rename = "additionalContextLimit")] additional_context_limit: Option, + /// When true, a failure of this hook blocks the tool instead of failing open. + #[serde(default, rename = "failClosed")] + #[ts(rename = "failClosed")] + fail_closed: bool, }, #[serde(rename = "mcp_tool")] #[ts(rename = "mcp_tool")] diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index 914abf4fd8b8..63599b6ab395 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -766,6 +766,7 @@ fn map_hook_handler_to_api(handler: CoreHookHandlerConfig) -> ConfiguredHookHand r#async, status_message, additional_context_limit, + fail_closed, } => ConfiguredHookHandler::Command { command, command_windows, @@ -773,6 +774,7 @@ fn map_hook_handler_to_api(handler: CoreHookHandlerConfig) -> ConfiguredHookHand r#async, status_message, additional_context_limit, + fail_closed, }, CoreHookHandlerConfig::McpTool { server, diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index c4ba5937d13e..e13422905a7c 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -256,6 +256,7 @@ statusMessage = "Scanning file" r#async: false, status_message: None, additional_context_limit: Some(4_096), + fail_closed: false, }, ConfiguredHookHandler::McpTool { server: "security".to_string(), 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 beebe306a148..fb7c05bc06df 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -67,6 +67,7 @@ fn command_hook_hash( r#async, status_message: status_message.map(ToOwned::to_owned), additional_context_limit, + fail_closed: false, }], }, }; diff --git a/codex-rs/config/src/hook_config.rs b/codex-rs/config/src/hook_config.rs index 7575fa5fcb10..8fbcd7ac79bd 100644 --- a/codex-rs/config/src/hook_config.rs +++ b/codex-rs/config/src/hook_config.rs @@ -182,6 +182,15 @@ pub enum HookHandlerConfig { skip_serializing_if = "Option::is_none" )] additional_context_limit: Option, + /// When true, a failure of this command hook blocks the tool instead of + /// failing open. Defaults to false, so existing handlers are unchanged. + /// Only command hooks carry this field; MCP tool hooks always fail open. + /// It also has no effect on a hook that never runs, such as an untrusted + /// user or project hook, or when the hooks feature is disabled. An async + /// handler that sets it runs synchronously, because a block can only be + /// applied before the tool starts. + #[serde(default, rename = "failClosed")] + fail_closed: bool, }, #[serde(rename = "mcp_tool")] McpTool { diff --git a/codex-rs/config/src/hooks_tests.rs b/codex-rs/config/src/hooks_tests.rs index bd71a7f0e323..8c83854fc69f 100644 --- a/codex-rs/config/src/hooks_tests.rs +++ b/codex-rs/config/src/hooks_tests.rs @@ -48,6 +48,7 @@ fn hooks_file_deserializes_existing_json_shape() { r#async: false, status_message: Some("checking".to_string()), additional_context_limit: Some(4096), + fail_closed: false, }], }], ..Default::default() @@ -56,6 +57,33 @@ fn hooks_file_deserializes_existing_json_shape() { ); } +#[test] +fn command_hook_fail_closed_defaults_false_and_round_trips() { + let omitted = serde_json::from_str::( + r#"{ "type": "command", "command": "python3 /tmp/pre.py" }"#, + ) + .expect("deserialize"); + assert!(matches!( + omitted, + HookHandlerConfig::Command { + fail_closed: false, + .. + } + )); + + let set = serde_json::from_str::( + r#"{ "type": "command", "command": "python3 /tmp/pre.py", "failClosed": true }"#, + ) + .expect("deserialize"); + assert!(matches!( + set, + HookHandlerConfig::Command { + fail_closed: true, + .. + } + )); +} + #[test] fn hooks_file_deserializes_mcp_tool_handler_with_json_inputs() { let parsed: HooksFile = serde_json::from_value(serde_json::json!({ @@ -182,6 +210,7 @@ additionalContextLimit = 4096 r#async: false, status_message: Some("checking".to_string()), additional_context_limit: Some(4096), + fail_closed: false, }], }], ..Default::default() @@ -220,6 +249,7 @@ command = "python3 /tmp/pre.py" r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -266,6 +296,7 @@ command = "python3 /enterprise/place/pre.py" r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -303,6 +334,7 @@ command_windows = "powershell -File C:\\enterprise\\hooks\\pre.ps1" r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -339,6 +371,7 @@ commandWindows = "powershell -File C:\\enterprise\\hooks\\pre.ps1" r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -355,6 +388,7 @@ fn hook_handler_omits_unset_additional_context_limit() { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }; let serialized = serde_json::to_value(handler).expect("hook handler should serialize"); diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 5864a5df77c6..48c08c36bbc9 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2110,6 +2110,11 @@ "default": null, "type": "string" }, + "failClosed": { + "default": false, + "description": "When true, a failure of this command hook blocks the tool instead of failing open. Defaults to false, so existing handlers are unchanged. Only command hooks carry this field; MCP tool hooks always fail open. It also has no effect on a hook that never runs, such as an untrusted user or project hook, or when the hooks feature is disabled. An async handler that sets it runs synchronously, because a block can only be applied before the tool starts.", + "type": "boolean" + }, "statusMessage": { "default": null, "type": "string" diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 496114a113a6..6683dcb766b3 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1997,6 +1997,7 @@ async fn refresh_runtime_config_refreshes_hooks() -> anyhow::Result<()> { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }, }; diff --git a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs index 3a6aa8d96f8d..1dbec8ddfcec 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs @@ -6,6 +6,7 @@ use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::resolve_tool_environment; +use crate::tools::hook_names::HookToolName; use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PreToolUsePayload; @@ -141,11 +142,30 @@ impl CoreToolRuntime for WriteStdinHandler { matches!(payload, ToolPayload::Function { .. }) } - fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { - // `write_stdin` is transport for an existing exec session. Empty writes - // are background polls, and non-empty writes continue a command that - // already ran PreToolUse as Bash, so do not emit a second pre hook here. - None + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + // Empty writes are background polls and carry nothing to review. A + // non-empty write is text the model is typing into the open shell, so + // review it as a Bash command before it reaches the process. + let args = parse_arguments::(arguments).ok()?; + if args.chars.is_empty() { + return None; + } + // The directory the shell was started in, not its current directory: a + // `cd` typed into the shell is not tracked. Null when the process is gone. + let workdir = invocation + .session + .services + .unified_exec_manager + .start_cwd_for_process(args.session_id) + .map(|cwd| serde_json::Value::String(cwd.to_string())) + .unwrap_or(serde_json::Value::Null); + Some(PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": args.chars, "workdir": workdir }), + }) } fn post_tool_use_payload( diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 61d379bf4d6d..6729a5fd79c6 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -632,9 +632,10 @@ async fn exec_command_pre_tool_use_payload_resolves_workdir() { } #[tokio::test] -async fn exec_command_pre_tool_use_payload_skips_write_stdin() { +async fn write_stdin_pre_tool_use_payload_reviews_nonempty_input_as_bash() { let payload = ToolPayload::Function { - arguments: serde_json::json!({ "chars": "echo hi" }).to_string(), + arguments: serde_json::json!({ "session_id": 1, "chars": "git reset --hard\n" }) + .to_string(), }; let (session, turn) = make_session_and_context().await; let turn = Arc::new(turn); @@ -652,6 +653,34 @@ async fn exec_command_pre_tool_use_payload_skips_write_stdin() { source: crate::tools::context::ToolCallSource::Direct, payload, }), + Some(crate::tools::registry::PreToolUsePayload { + tool_name: HookToolName::bash(), + tool_input: serde_json::json!({ "command": "git reset --hard\n", "workdir": null }), + }) + ); +} + +#[tokio::test] +async fn write_stdin_pre_tool_use_payload_skips_empty_poll() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ "session_id": 1, "chars": "" }).to_string(), + }; + let (session, turn) = make_session_and_context().await; + let turn = Arc::new(turn); + let handler = WriteStdinHandler; + + assert_eq!( + handler.pre_tool_use_payload(&ToolInvocation { + session: session.into(), + step_context: StepContext::for_test(Arc::clone(&turn)), + turn, + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-45".to_string(), + tool_name: codex_tools::ToolName::plain("write_stdin"), + source: crate::tools::context::ToolCallSource::Direct, + payload, + }), None ); } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 2940235c6a3b..c650702111d1 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1820,6 +1820,19 @@ impl UnifiedExecProcessManager { .map(|entry| entry.environment_id.clone()) } + /// The directory the exec process was started in. This is the start cwd, + /// not the shell's current directory: a `cd` typed into the shell is not tracked. + /// Returns `None` when the process is gone or the store is busy, so the caller + /// never blocks on the lock. + pub(crate) fn start_cwd_for_process(&self, process_id: i32) -> Option { + self.process_store + .try_lock() + .ok()? + .processes + .get(&process_id) + .map(|entry| entry.cwd.clone()) + } + pub(crate) async fn list_processes(&self) -> Vec { let store = self.process_store.lock().await; let mut entries = store diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index ce56681e68b4..9f4e2ef12fe1 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -678,3 +678,65 @@ async fn pruning_does_not_evict_live_process_while_exited_process_is_finalizing( (None, MAX_UNIFIED_EXEC_PROCESSES) ); } + +#[tokio::test] +async fn start_cwd_for_process_returns_the_spawn_directory() { + let (_, turn) = crate::session::tests::make_session_and_context().await; + let manager = UnifiedExecProcessManager::default(); + let process_id = manager.allocate_process_id().await; + let cwd = PathUri::parse("file:///tmp/other-repo").expect("cwd"); + let process = Arc::new( + crate::unified_exec::process_tests::remote_process( + codex_exec_server::WriteStatus::Accepted, + /*terminate_error*/ None, + codex_sandboxing::SandboxType::None, + ) + .await, + ); + manager.process_store.lock().await.processes.insert( + process_id, + ProcessEntry { + process, + plugin_metrics_sidecar: None, + call_id: "call".to_string(), + process_id, + cwd: cwd.clone(), + initial_exec_command_active: Arc::new(std::sync::atomic::AtomicBool::new(false)), + hook_command: "bash".to_string(), + tty: true, + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + permissions: super::super::TerminalPermissions::for_launch( + turn.initial_environments + .primary() + .expect("turn environment"), + &turn, + super::super::TerminalSandboxSource::Native, + crate::sandboxing::SandboxPermissions::UseDefault, + /*additional_permissions*/ None, + /*internal_permissions*/ None, + ), + network_approval: None, + session: std::sync::Weak::new(), + last_used: Instant::now(), + }, + ); + + assert_eq!( + manager.start_cwd_for_process(process_id).as_ref(), + Some(&cwd) + ); + assert_eq!(manager.start_cwd_for_process(process_id + 1), None); +} + +#[tokio::test] +async fn start_cwd_for_process_returns_none_without_blocking_while_locked() { + let manager = UnifiedExecProcessManager::default(); + let _guard = manager.process_store.lock().await; + + let started = Instant::now(); + assert_eq!(manager.start_cwd_for_process(1), None); + assert!( + started.elapsed() < Duration::from_millis(200), + "lookup blocked on the held lock" + ); +} diff --git a/codex-rs/hooks/src/declarations.rs b/codex-rs/hooks/src/declarations.rs index 42ca76a2ed02..4b9a28ae1236 100644 --- a/codex-rs/hooks/src/declarations.rs +++ b/codex-rs/hooks/src/declarations.rs @@ -70,6 +70,7 @@ mod tests { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }, ], }], diff --git a/codex-rs/hooks/src/engine/command_runner_tests.rs b/codex-rs/hooks/src/engine/command_runner_tests.rs index a7b90a8c0ed1..d75f7264b4a2 100644 --- a/codex-rs/hooks/src/engine/command_runner_tests.rs +++ b/codex-rs/hooks/src/engine/command_runner_tests.rs @@ -117,6 +117,7 @@ async fn hook_shell_startup_does_not_stop_on_controlling_terminal() { timeout_sec: 2, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: AbsolutePathBuf::try_from(temp.path().join("hooks.json")) .expect("absolute hook configuration path") .into(), @@ -221,6 +222,7 @@ async fn cmd_shell_runs_quoted_hook_command_path() { timeout_sec: 10, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: source_path.into(), source: HookSource::User, display_order: 0, @@ -271,6 +273,7 @@ async fn fast_exiting_hook_preserves_stdout_when_stdin_is_not_consumed() { timeout_sec: 10, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: source_path.into(), source: HookSource::User, display_order: 0, @@ -359,6 +362,7 @@ async fn command_hook_does_not_expose_configured_noise_auth_token() { timeout_sec: 10, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: source_path.into(), source: HookSource::User, display_order: 0, @@ -497,6 +501,7 @@ fn write_handler(temp: &TempDir, source: &str) -> ConfiguredHandler { timeout_sec: 10, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: AbsolutePathBuf::try_from(temp.path().join("hooks.json")) .expect("absolute test hook path") .into(), diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 0c5a28ef35ef..a5a8709a9d4a 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -77,6 +77,7 @@ struct NormalizedHandler { timeout_sec: u64, status_message: Option, additional_context_limit: Option, + fail_closed: bool, } #[derive(Clone, Copy)] @@ -511,6 +512,7 @@ fn append_matcher_groups( r#async, status_message, additional_context_limit, + fail_closed, } => { let command = if cfg!(windows) { command_windows.unwrap_or(command) @@ -538,6 +540,19 @@ fn append_matcher_groups( source.path.display() )); } + // `failClosed` only takes effect on a synchronous handler, so an + // async handler that asks for it runs synchronously. Skipping it + // would let the tool run with no guard at all. + let runs_async = if runs_async && fail_closed { + warnings.push(format!( + "running async {} hook synchronously in {} because failClosed is set", + hook_event_name_label(event_name), + source.path.display() + )); + false + } else { + runs_async + }; let additional_context_limit = if matches!( event_name, codex_protocol::protocol::HookEventName::PreToolUse @@ -565,6 +580,7 @@ fn append_matcher_groups( r#async, status_message: status_message.clone(), additional_context_limit: normalized_additional_context_limit, + fail_closed, }; let command = source.env.iter().fold(command, |command, (key, value)| { command.replace(&format!("${{{key}}}"), value) @@ -579,6 +595,7 @@ fn append_matcher_groups( timeout_sec, status_message, additional_context_limit, + fail_closed, } } HookHandlerConfig::McpTool { @@ -632,6 +649,7 @@ fn append_matcher_groups( timeout_sec, status_message, additional_context_limit: None, + fail_closed: false, } } HookHandlerConfig::Prompt {} => { @@ -662,6 +680,7 @@ fn append_matcher_groups( timeout_sec, status_message, additional_context_limit, + fail_closed, } = normalized; let current_hash = hook_hash(event_name, matcher, &group, &config); let key = crate::hook_key(&source.key_source, event_name, group_index, handler_index); @@ -728,6 +747,7 @@ fn append_matcher_groups( additional_context_limit: AdditionalContextLimit::from_config( additional_context_limit, ), + fail_closed, source_path: source.path.clone().into(), source: source.source, display_order: *display_order, @@ -979,6 +999,7 @@ mod tests { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], } } @@ -995,6 +1016,7 @@ mod tests { r#async: false, status_message: None, additional_context_limit: Some(additional_context_limit), + fail_closed: false, }], } } @@ -1263,6 +1285,7 @@ mod tests { timeout_sec: 600, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: source_path.clone().into(), source: hook_source(), display_order: 0, @@ -1303,6 +1326,7 @@ mod tests { timeout_sec: 600, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: source_path.clone().into(), source: hook_source(), display_order: 0, @@ -1341,6 +1365,7 @@ mod tests { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }, HookHandlerConfig::Command { command: "echo clamped".to_string(), @@ -1349,6 +1374,7 @@ mod tests { r#async: true, status_message: None, additional_context_limit: None, + fail_closed: false, }, ], }], @@ -1431,6 +1457,7 @@ mod tests { r#async: true, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ); @@ -1475,6 +1502,45 @@ mod tests { ); } + #[test] + fn fail_closed_forces_an_async_handler_to_run_synchronously() { + let source_path = source_path(); + let hook_states = std::collections::HashMap::new(); + let mut handlers = Vec::new(); + let mut warnings = Vec::new(); + + append_matcher_groups( + &mut handlers, + &mut Vec::new(), + &mut warnings, + &mut 0, + &mut hook_handler_source(&source_path, &hook_states), + HookEventName::PreToolUse, + vec![MatcherGroup { + matcher: Some("Bash".to_string()), + hooks: vec![HookHandlerConfig::Command { + command: "echo guard".to_string(), + command_windows: None, + timeout_sec: None, + r#async: true, + status_message: None, + additional_context_limit: None, + fail_closed: true, + }], + }], + ); + + assert_eq!(handlers.len(), 1); + assert!(handlers[0].fail_closed); + assert_eq!(handlers[0].execution_mode(), HookExecutionMode::Sync); + assert!( + warnings + .iter() + .any(|warning| warning.contains("failClosed")), + "expected a warning about the async/failClosed combination, got {warnings:?}" + ); + } + #[test] fn bypass_hook_trust_allows_enabled_untrusted_handlers() { let mut handlers = Vec::new(); @@ -1621,6 +1687,7 @@ mod tests { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -1652,6 +1719,7 @@ mod tests { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ); diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index b34118d6f2b8..f90f8d179eac 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -357,6 +357,7 @@ mod tests { timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: HookSource::User, display_order, diff --git a/codex-rs/hooks/src/engine/mcp_runner_tests.rs b/codex-rs/hooks/src/engine/mcp_runner_tests.rs index a16613ea071d..297013a6a3c5 100644 --- a/codex-rs/hooks/src/engine/mcp_runner_tests.rs +++ b/codex-rs/hooks/src/engine/mcp_runner_tests.rs @@ -97,6 +97,7 @@ async fn mcp_tool_results_use_command_hook_output_contract() { timeout_sec: 30, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index a007f3b1a459..e71de4bf1961 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -67,6 +67,8 @@ pub(crate) struct ConfiguredHandler { pub timeout_sec: u64, pub status_message: Option, pub additional_context_limit: AdditionalContextLimit, + /// Opt-in: a failed run of this handler blocks the tool instead of failing open. + pub fail_closed: bool, pub source_path: HandlerSourcePath, pub source: HookSource, pub display_order: i64, @@ -327,6 +329,7 @@ impl ClaudeHooksEngine { timeout_sec: timeout_sec.unwrap_or(5).max(1), status_message, additional_context_limit: Default::default(), + fail_closed: false, source_path: HandlerSourcePath::ExecutorScoped { plugin_id: source.plugin_id.clone(), environment_id: source.environment_id.clone(), diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index a185e45029ff..28416ad132f9 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -96,6 +96,7 @@ fn permission_request_timeout_only_counts_synchronous_handlers() { timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: cwd().join("hooks.json").into(), source: HookSource::User, display_order: 0, @@ -156,6 +157,7 @@ fn pre_tool_use_hook_events(command: impl Into) -> HookEventsToml { r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -584,6 +586,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle: r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -692,6 +695,7 @@ async fn requirements_managed_hooks_execute_windows_command_override() { r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -773,6 +777,7 @@ fn unknown_requirement_source_hooks_stay_managed() { r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -843,6 +848,7 @@ fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() { r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -1087,6 +1093,7 @@ fn requirements_managed_hooks_load_when_managed_dir_is_missing() { r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -1812,6 +1819,7 @@ fn bundled_cleanup_trust_does_not_extend_to_other_handlers() { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }, ]); source.hooks.stop.push(MatcherGroup { @@ -1974,6 +1982,7 @@ fn disabled_hooks_feature_keeps_builtin_cleanup_but_not_trusted_plugin_hooks() { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }); let stack = trusted_plugin_hook_stack(cwd().join("config.toml"), &[source.clone()]); let discovered = super::discovery::discover_handlers( @@ -2039,6 +2048,7 @@ print(json.dumps({ r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -2160,6 +2170,7 @@ fn plugin_hook_sources_expand_plugin_placeholders() { r#async: false, status_message: None, additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() @@ -2318,6 +2329,7 @@ fn executor_stop_hook_fixture() -> ( timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: HandlerSourcePath::ExecutorScoped { plugin_id: PluginId::parse("computer-use@openai-bundled").expect("valid plugin ID"), environment_id: "executor-a".to_string(), @@ -2407,6 +2419,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { timeout_sec: 30, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: cwd().join("hooks.json").into(), source: HookSource::User, display_order: 0, @@ -2442,6 +2455,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { timeout_sec: 30, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: cwd().join("hooks.json").into(), source: HookSource::User, display_order: 1, diff --git a/codex-rs/hooks/src/events/compact.rs b/codex-rs/hooks/src/events/compact.rs index 82bd6c6cf55d..0a123b933816 100644 --- a/codex-rs/hooks/src/events/compact.rs +++ b/codex-rs/hooks/src/events/compact.rs @@ -529,6 +529,7 @@ mod tests { timeout_sec: 5, status_message: Some("running compact hook".to_string()), additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/events/interrupt_tests.rs b/codex-rs/hooks/src/events/interrupt_tests.rs index 3d8642c8457b..dc255e03b282 100644 --- a/codex-rs/hooks/src/events/interrupt_tests.rs +++ b/codex-rs/hooks/src/events/interrupt_tests.rs @@ -104,6 +104,7 @@ fn handler() -> ConfiguredHandler { timeout_sec: 600, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 0ca0f6c673d8..69ccffc22203 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -595,6 +595,7 @@ mod tests { timeout_sec: 5, status_message: Some("running post tool use hook".to_string()), additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index e57b823a1c86..1c57b192bbf4 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -292,6 +292,24 @@ fn parse_completed( }, } + // `failClosed` turns every failure into a block the model sees. A handler + // without the flag keeps failing open, so the tool still runs. + if handler.fail_closed && handler.can_apply_control_effects() && status == HookRunStatus::Failed + { + status = HookRunStatus::Blocked; + should_block = true; + let reason = entries + .iter() + .find(|entry| entry.kind == HookOutputEntryKind::Error) + .map(|entry| format!("hook failed closed: {}", entry.text)) + .unwrap_or_else(|| "hook failed closed".to_string()); + block_reason = Some(reason.clone()); + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: reason, + }); + } + let completed = HookCompletedEvent { turn_id, run: dispatcher::completed_summary(handler, &run_result, status, entries), @@ -332,6 +350,7 @@ mod tests { use super::PreToolUseHandlerData; use super::command_input_json; + use super::dispatcher; use super::latest_updated_input; use super::parse_completed; use super::preview; @@ -779,6 +798,7 @@ mod tests { timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, @@ -802,6 +822,137 @@ mod tests { } } + fn run_result_error(error: &str) -> HandlerRunResult { + HandlerRunResult { + started_at: 1, + completed_at: 2, + duration_ms: 1, + exit_code: None, + stdout: String::new(), + stderr: String::new(), + error: Some(error.to_string()), + } + } + + fn assert_failed_open(parsed: &dispatcher::ParsedHandler) { + assert!(!parsed.data.should_block); + assert_eq!(parsed.data.block_reason, None); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + } + + fn assert_failed_closed( + parsed: &dispatcher::ParsedHandler, + reason_contains: &str, + ) { + assert!(parsed.data.should_block); + let reason = parsed.data.block_reason.as_deref().unwrap_or_default(); + assert!( + reason.contains(reason_contains), + "block reason {reason:?} should mention {reason_contains:?}" + ); + assert!(reason.starts_with("hook failed closed: ")); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); + assert!( + parsed + .completed + .run + .entries + .iter() + .any(|entry| entry.kind == HookOutputEntryKind::Feedback + && entry.text.contains(reason_contains)) + ); + } + + #[test] + fn fail_closed_blocks_nonzero_exit_other_than_two() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed(&handler, run_result(Some(1), "", "boom"), None); + assert_failed_closed(&parsed, "hook exited with code 1"); + } + + #[test] + fn fail_closed_blocks_exit_two_without_a_reason() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed(&handler, run_result(Some(2), "", " "), None); + assert_failed_closed(&parsed, "did not write a blocking reason"); + } + + #[test] + fn fail_closed_blocks_missing_status_code() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed(&handler, run_result(None, "", ""), None); + assert_failed_closed(&parsed, "exited without a status code"); + } + + #[test] + fn fail_closed_blocks_spawn_failure() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed( + &handler, + run_result_error("No such file or directory (os error 2)"), + None, + ); + assert_failed_closed(&parsed, "No such file or directory"); + } + + #[test] + fn fail_closed_blocks_timeout() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed(&handler, run_result_error("hook timed out after 5s"), None); + assert_failed_closed(&parsed, "timed out"); + } + + #[test] + fn fail_closed_blocks_crash_or_signal() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed( + &handler, + run_result_error("wait_error: process terminated by signal"), + None, + ); + assert_failed_closed(&parsed, "terminated by signal"); + } + + #[test] + fn fail_closed_blocks_invalid_json() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed(&handler, run_result(Some(0), "{not json", ""), None); + assert_failed_closed(&parsed, "invalid pre-tool-use JSON"); + } + + #[test] + fn fail_closed_blocks_unreadable_command_output() { + let mut handler = handler(); + handler.fail_closed = true; + let parsed = parse_completed( + &handler, + run_result_error("stream did not contain valid UTF-8"), + None, + ); + assert_failed_closed(&parsed, "valid UTF-8"); + } + + #[test] + fn handler_without_fail_closed_still_fails_open() { + for result in [ + run_result(Some(1), "", ""), + run_result(Some(2), "", ""), + run_result(None, "", ""), + run_result(Some(0), "{not json", ""), + run_result_error("No such file or directory (os error 2)"), + run_result_error("hook timed out after 5s"), + ] { + assert_failed_open(&parse_completed(&handler(), result, None)); + } + } + fn request_for_tool_use(tool_use_id: &str) -> super::PreToolUseRequest { super::PreToolUseRequest { session_id: ThreadId::new(), diff --git a/codex-rs/hooks/src/events/session_end_tests.rs b/codex-rs/hooks/src/events/session_end_tests.rs index 05fa107d4fa9..dc6c378b3f8a 100644 --- a/codex-rs/hooks/src/events/session_end_tests.rs +++ b/codex-rs/hooks/src/events/session_end_tests.rs @@ -67,6 +67,7 @@ fn handler(matcher: Option<&str>) -> ConfiguredHandler { timeout_sec: 2, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index 3aff0fe946eb..95bc42cf274c 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -548,6 +548,7 @@ mod tests { timeout_sec: 600, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/events/stop.rs b/codex-rs/hooks/src/events/stop.rs index be2be9100e35..32802fd9705b 100644 --- a/codex-rs/hooks/src/events/stop.rs +++ b/codex-rs/hooks/src/events/stop.rs @@ -698,6 +698,7 @@ mod tests { timeout_sec: 600, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, diff --git a/codex-rs/hooks/src/events/user_prompt_submit.rs b/codex-rs/hooks/src/events/user_prompt_submit.rs index 9124431bc194..65a9f252f3c2 100644 --- a/codex-rs/hooks/src/events/user_prompt_submit.rs +++ b/codex-rs/hooks/src/events/user_prompt_submit.rs @@ -467,6 +467,7 @@ mod tests { timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), + fail_closed: false, source_path: test_path_buf("/tmp/hooks.json").abs().into(), source: codex_protocol::protocol::HookSource::User, display_order: 0, diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index 2840e99609fd..929a81d49723 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -1395,6 +1395,7 @@ approval_policy = "never" r#async: false, status_message: Some("checking".to_string()), additional_context_limit: None, + fail_closed: false, }], }], ..Default::default() diff --git a/sdk/python/src/openai_codex/generated/v2_all.py b/sdk/python/src/openai_codex/generated/v2_all.py index 5bbd86342beb..dc8a0e9da697 100644 --- a/sdk/python/src/openai_codex/generated/v2_all.py +++ b/sdk/python/src/openai_codex/generated/v2_all.py @@ -1001,6 +1001,13 @@ class CommandConfiguredHookHandler(BaseModel): async_: Annotated[bool, Field(alias="async")] command: str command_windows: Annotated[str | None, Field(alias="commandWindows")] = None + fail_closed: Annotated[ + bool | None, + Field( + alias="failClosed", + description="When true, a failure of this hook blocks the tool instead of failing open.", + ), + ] = False status_message: Annotated[str | None, Field(alias="statusMessage")] = None timeout_sec: Annotated[int | None, Field(alias="timeoutSec", ge=0)] = None type: Annotated[Literal["command"], Field(title="CommandConfiguredHookHandlerType")]