From cfd5d77d63f53303aad63ef0886d3b159996a1af Mon Sep 17 00:00:00 2001 From: Abhinav Date: Tue, 8 Sep 2026 17:53:12 +0000 Subject: [PATCH] Detach Unix hook commands from the controlling terminal (#43876) ## Why Shell startup code that accesses the controlling terminal can stop a hook on background terminal I/O, preventing the hook command from running. ## What changed Use `detach_from_tty` before executing Unix hook commands to avoid inheriting the controlling terminal while preserving process-group cleanup. ## Testing Add a Unix regression test that runs under a controlling terminal and uses `BASH_ENV` to invoke `stty` during shell startup. Verify that the hook completes successfully and produces the expected output. GitOrigin-RevId: 231125d4b169fb1bd828e3f8e96476a13bd25d26 --- codex-rs/hooks/src/engine/command_runner.rs | 7 +- .../hooks/src/engine/command_runner_tests.rs | 110 ++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/codex-rs/hooks/src/engine/command_runner.rs b/codex-rs/hooks/src/engine/command_runner.rs index b68474f0cf..7f8d8be070 100644 --- a/codex-rs/hooks/src/engine/command_runner.rs +++ b/codex-rs/hooks/src/engine/command_runner.rs @@ -222,7 +222,12 @@ pub(crate) async fn run_command( .kill_on_drop(true); #[cfg(unix)] - command.process_group(0); + // Keep process-group cleanup without inheriting the controlling terminal, where + // shell startup can otherwise stop the hook on background terminal I/O. + // SAFETY: detach_from_tty only performs async-signal-safe process setup. + unsafe { + command.pre_exec(codex_utils_pty::process_group::detach_from_tty); + } #[cfg(windows)] let mut process_tree_job = JobObject::create().ok(); diff --git a/codex-rs/hooks/src/engine/command_runner_tests.rs b/codex-rs/hooks/src/engine/command_runner_tests.rs index caf2a66340..f0a45e5b4a 100644 --- a/codex-rs/hooks/src/engine/command_runner_tests.rs +++ b/codex-rs/hooks/src/engine/command_runner_tests.rs @@ -36,6 +36,116 @@ use super::build_command; use super::run_command; use crate::events::user_prompt_submit::UserPromptSubmitRequest; +#[cfg(unix)] +#[tokio::test] +async fn hook_shell_startup_does_not_stop_on_controlling_terminal() { + const CHILD_ENV: &str = "CODEX_HOOK_TERMINAL_TEST_CHILD"; + const TEST_NAME: &str = + "engine::command_runner::tests::hook_shell_startup_does_not_stop_on_controlling_terminal"; + + if std::env::var_os(CHILD_ENV).is_none() { + // Re-exec under a controlling terminal even when the test runner has none. + let executable = std::env::current_exe().expect("current test executable"); + let mut env = std::env::vars().collect::>(); + env.insert(CHILD_ENV.to_string(), "1".to_string()); + let codex_utils_pty::SpawnedProcess { + session: _session, + mut stdout_rx, + exit_rx, + .. + } = codex_utils_pty::spawn_pty_process( + executable.to_str().expect("UTF-8 test executable path"), + &[ + TEST_NAME.to_string(), + "--exact".to_string(), + "--nocapture".to_string(), + ], + &std::env::current_dir().expect("current test directory"), + &env, + /*arg0*/ &None, + codex_utils_pty::TerminalSize::default(), + &[], + ) + .await + .expect("spawn test with a controlling terminal"); + + let (exit_code, output) = timeout(Duration::from_secs(10), async { + let output = async { + let mut output = Vec::new(); + while let Some(chunk) = stdout_rx.recv().await { + output.extend_from_slice(&chunk); + } + output + }; + tokio::join!(exit_rx, output) + }) + .await + .expect("terminal hook test should finish"); + let output = String::from_utf8_lossy(&output); + assert_eq!( + exit_code.expect("terminal hook test exit status"), + 0, + "{output}" + ); + assert!( + output.contains(TEST_NAME), + "child did not run test: {output}" + ); + return; + } + + std::fs::File::open("/dev/tty").expect("test process must have a controlling terminal"); + let temp = tempdir().expect("create temp dir"); + let startup_path = temp.path().join("bashenv"); + std::fs::write( + &startup_path, + "command -v stty >/dev/null || exit 1\nstty sane < /dev/tty\n", + ) + .expect("write shell startup fixture"); + let command = "printf hook-ran"; + let env = HashMap::from([( + "BASH_ENV".to_string(), + startup_path + .to_str() + .expect("UTF-8 startup path") + .to_string(), + )]); + let handler = ConfiguredHandler { + builtin: false, + event_name: HookEventName::SessionStart, + matcher: None, + timeout_sec: 2, + status_message: None, + additional_context_limit: Default::default(), + source_path: AbsolutePathBuf::try_from(temp.path().join("hooks.json")) + .expect("absolute hook configuration path") + .into(), + source: HookSource::User, + display_order: 0, + kind: ConfiguredHandlerKind::Command { + command: command.to_string(), + r#async: false, + env: env.clone(), + }, + }; + let (result_sender, _result_receiver) = async_channel::unbounded(); + let runtime = CommandHookRuntime::new( + CommandShell { + program: "/bin/bash".to_string(), + args: vec!["-c".to_string()], + }, + Arc::new(std::env::vars_os().collect()), + ThreadId::new(), + result_sender, + ); + + let result = run_command(&runtime, &handler, command, &env, "{}", temp.path()).await; + + assert_eq!(result.exit_code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "hook-ran"); + assert_eq!(result.error, None); +} + #[cfg(windows)] #[tokio::test] async fn cmd_shell_runs_quoted_hook_command_path() {