From ba463a9dc78180d9cd61b28ef6562e03342a14be Mon Sep 17 00:00:00 2001 From: friel-openai Date: Sun, 15 Mar 2026 22:17:25 -0700 Subject: [PATCH] Preserve background terminals on interrupt and rename cleanup command to /stop (#14602) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation - Interrupting a running turn (Ctrl+C / Esc) currently also terminates long‑running background shells, which is surprising for workflows like local dev servers or file watchers. - The existing cleanup command name was confusing; callers expect an explicit command to stop background terminals rather than a UI clear action. - Make background‑shell termination explicit and surface a clearer command name while preserving backward compatibility. ### Description - Renamed the background‑terminal cleanup slash command from `Clean` (`/clean`) to `Stop` (`/stop`) and kept `clean` as an alias in the command parsing/visibility layer, updated the user descriptions and command popup wiring accordingly. - Updated the unified‑exec footer text and snapshots to point to `/stop` (and trimmed corresponding snapshot output to match the new label). - Changed interrupt behavior so `Op::Interrupt` (Ctrl+C / Esc interrupt) no longer closes or clears tracked unified exec / background terminal processes in the TUI or core cleanup path; background shells are now preserved after an interrupt. - Updated protocol/docs to clarify that `turn/interrupt` (or `Op::Interrupt`) interrupts the active turn but does not terminate background terminals, and that `thread/backgroundTerminals/clean` is the explicit API to stop those shells. - Updated unit/integration tests and insta snapshots in the TUI and core unified‑exec suites to reflect the new semantics and command name. ### Testing - Ran formatting with `just fmt` in `codex-rs` (succeeded). - Ran `cargo test -p codex-protocol` (succeeded). - Attempted `cargo test -p codex-tui` but the build could not complete in this environment due to a native build dependency that requires `libcap` development headers (the `codex-linux-sandbox` vendored build step); install `libcap-dev` / make `libcap.pc` available in `PKG_CONFIG_PATH` to run the TUI test suite locally. - Updated and accepted the affected `insta` snapshots for the TUI changes so visual diffs reflect the new `/stop` wording and preserved interrupt behavior. ------ [Codex Task](https://chatgpt.com/codex/tasks/task_i_69b39c44b6dc8323bd133ae206310fae) --- codex-rs/app-server/README.md | 2 +- codex-rs/core/src/tasks/mod.rs | 4 +- .../core/src/unified_exec/process_manager.rs | 72 +++++++++++-------- codex-rs/core/tests/suite/unified_exec.rs | 9 ++- codex-rs/protocol/src/protocol.rs | 3 +- .../tui/src/bottom_pane/slash_commands.rs | 23 +++++- ...c_footer__tests__render_more_sessions.snap | 2 +- .../src/bottom_pane/unified_exec_footer.rs | 2 +- codex-rs/tui/src/chatwidget.rs | 17 +---- ...t_preserves_unified_exec_wait_streak.snap} | 0 codex-rs/tui/src/chatwidget/tests.rs | 18 +++-- codex-rs/tui/src/slash_command.rs | 25 ++++++- 12 files changed, 112 insertions(+), 65 deletions(-) rename codex-rs/tui/src/chatwidget/snapshots/{codex_tui__chatwidget__tests__interrupt_clears_unified_exec_wait_streak.snap => codex_tui__chatwidget__tests__interrupt_preserves_unified_exec_wait_streak.snap} (100%) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 4dcf93bc2..d043496b9 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -531,7 +531,7 @@ You can cancel a running Turn with `turn/interrupt`. { "id": 31, "result": {} } ``` -The server requests cancellations for running subprocesses, then emits a `turn/completed` event with `status: "interrupted"`. Rely on the `turn/completed` to know when Codex-side cleanup is done. +The server requests cancellation of the active turn, then emits a `turn/completed` event with `status: "interrupted"`. This does not terminate background terminals; use `thread/backgroundTerminals/clean` when you explicitly want to stop those shells. Rely on the `turn/completed` event to know when turn interruption has finished. ### Example: Clean background terminals diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index a7a8f3813..5908fb35a 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -56,7 +56,7 @@ pub(crate) use user_shell::UserShellCommandTask; pub(crate) use user_shell::execute_user_shell_command; const GRACEFULL_INTERRUPTION_TIMEOUT_MS: u64 = 100; -const TURN_ABORTED_INTERRUPTED_GUIDANCE: &str = "The user interrupted the previous turn on purpose. Any running unified exec processes were terminated. If any tools/commands were aborted, they may have partially executed; verify current state before retrying."; +const TURN_ABORTED_INTERRUPTED_GUIDANCE: &str = "The user interrupted the previous turn on purpose. Any running unified exec processes may still be running in the background. If any tools/commands were aborted, they may have partially executed; verify current state before retrying."; fn emit_turn_network_proxy_metric( session_telemetry: &SessionTelemetry, @@ -394,8 +394,6 @@ impl Session { } pub(crate) async fn cleanup_after_interrupt(&self, turn_context: &Arc) { - self.close_unified_exec_processes().await; - if let Some(manager) = turn_context.js_repl.manager_if_initialized() && let Err(err) = manager.interrupt_turn_exec(&turn_context.sub_id).await { diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 95b328a82..6494c4ffb 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -191,9 +191,29 @@ impl UnifiedExecProcessManager { emitter.emit(event_ctx, ToolEventStage::Begin).await; start_streaming_output(&process, context, Arc::clone(&transcript)); - let yield_time_ms = clamp_yield_time(request.yield_time_ms); - let start = Instant::now(); + // Persist live sessions before the initial yield wait so interrupting the + // turn cannot drop the last Arc and terminate the background process. + let process_started_alive = !process.has_exited() && process.exit_code().is_none(); + if process_started_alive { + let network_approval_id = deferred_network_approval + .as_ref() + .map(|deferred| deferred.registration_id().to_string()); + self.store_process( + Arc::clone(&process), + context, + &request.command, + cwd.clone(), + start, + request.process_id, + request.tty, + network_approval_id, + Arc::clone(&transcript), + ) + .await; + } + + let yield_time_ms = clamp_yield_time(request.yield_time_ms); // For the initial exec_command call, we both stream output to events // (via start_streaming_output above) and collect a snapshot here for // the tool response body. @@ -222,15 +242,28 @@ impl UnifiedExecProcessManager { let wall_time = Instant::now().saturating_duration_since(start); let text = String::from_utf8_lossy(&collected).to_string(); - let exit_code = process.exit_code(); - let has_exited = process.has_exited() || exit_code.is_some(); let chunk_id = generate_chunk_id(); let process_id = request.process_id; - - if has_exited { + let (response_process_id, exit_code) = if process_started_alive { + match self.refresh_process_state(process_id).await { + ProcessStatus::Alive { + exit_code, + process_id, + .. + } => (Some(process_id), exit_code), + ProcessStatus::Exited { exit_code, .. } => { + process.check_for_sandbox_denial_with_text(&text).await?; + (None, exit_code) + } + ProcessStatus::Unknown => { + return Err(UnifiedExecError::UnknownProcessId { process_id }); + } + } + } else { // Short‑lived command: emit ExecCommandEnd immediately using the // same helper as the background watcher, so all end events share // one implementation. + let exit_code = process.exit_code(); let exit = exit_code.unwrap_or(-1); emit_exec_end_for_unified_exec( Arc::clone(&context.session), @@ -253,26 +286,7 @@ impl UnifiedExecProcessManager { ) .await; process.check_for_sandbox_denial_with_text(&text).await?; - } else { - // Long‑lived command: persist the process so write_stdin can reuse - // it, and register a background watcher that will emit - // ExecCommandEnd when the PTY eventually exits (even if no further - // tool calls are made). - let network_approval_id = deferred_network_approval - .as_ref() - .map(|deferred| deferred.registration_id().to_string()); - self.store_process( - Arc::clone(&process), - context, - &request.command, - cwd.clone(), - start, - process_id, - request.tty, - network_approval_id, - Arc::clone(&transcript), - ) - .await; + (None, exit_code) }; let original_token_count = approx_token_count(&text); @@ -282,11 +296,7 @@ impl UnifiedExecProcessManager { wall_time, raw_output: collected, max_output_tokens: request.max_output_tokens, - process_id: if has_exited { - None - } else { - Some(request.process_id) - }, + process_id: response_process_id, exit_code, original_token_count: Some(original_token_count), session_command: Some(request.command.clone()), diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 0d8399376..848e77750 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -2018,7 +2018,7 @@ async fn unified_exec_keeps_long_running_session_after_turn_end() -> Result<()> } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn unified_exec_interrupt_terminates_long_running_session() -> Result<()> { +async fn unified_exec_interrupt_preserves_long_running_session() -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); skip_if_windows!(Ok(())); @@ -2092,6 +2092,13 @@ async fn unified_exec_interrupt_terminates_long_running_session() -> Result<()> codex.submit(Op::Interrupt).await?; wait_for_event(&codex, |event| matches!(event, EventMsg::TurnAborted(_))).await; + + assert!( + process_is_alive(&pid)?, + "expected unified exec process to remain alive after interrupt" + ); + + codex.submit(Op::CleanBackgroundTerminals).await?; wait_for_process_exit(&pid).await?; Ok(()) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 8ac5242dc..fb97783b9 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -193,11 +193,12 @@ pub struct ConversationTextParams { #[allow(clippy::large_enum_variant)] #[non_exhaustive] pub enum Op { - /// Abort current task. + /// Abort current task without terminating background terminal processes. /// This server sends [`EventMsg::TurnAborted`] in response. Interrupt, /// Terminate all running background terminal processes for this thread. + /// Use this when callers intentionally want to stop long-lived background shells. CleanBackgroundTerminals, /// Start a realtime conversation stream. diff --git a/codex-rs/tui/src/bottom_pane/slash_commands.rs b/codex-rs/tui/src/bottom_pane/slash_commands.rs index 85b301386..15b70f232 100644 --- a/codex-rs/tui/src/bottom_pane/slash_commands.rs +++ b/codex-rs/tui/src/bottom_pane/slash_commands.rs @@ -3,6 +3,8 @@ //! The same sandbox- and feature-gating rules are used by both the composer //! and the command popup. Centralizing them here keeps those call sites small //! and ensures they stay in sync. +use std::str::FromStr; + use codex_utils_fuzzy_match::fuzzy_match; use crate::slash_command::SlashCommand; @@ -38,10 +40,11 @@ pub(crate) fn builtins_for_input(flags: BuiltinCommandFlags) -> Vec<(&'static st /// Find a single built-in command by exact name, after applying the gating rules. pub(crate) fn find_builtin_command(name: &str, flags: BuiltinCommandFlags) -> Option { + let cmd = SlashCommand::from_str(name).ok()?; builtins_for_input(flags) .into_iter() - .find(|(command_name, _)| *command_name == name) - .map(|(_, cmd)| cmd) + .any(|(_, visible_cmd)| visible_cmd == cmd) + .then_some(cmd) } /// Whether any visible built-in fuzzily matches the provided prefix. @@ -82,6 +85,22 @@ mod tests { ); } + #[test] + fn stop_command_resolves_for_dispatch() { + assert_eq!( + find_builtin_command("stop", all_enabled_flags()), + Some(SlashCommand::Stop) + ); + } + + #[test] + fn clean_command_alias_resolves_for_dispatch() { + assert_eq!( + find_builtin_command("clean", all_enabled_flags()), + Some(SlashCommand::Stop) + ); + } + #[test] fn fast_command_is_hidden_when_disabled() { let mut flags = all_enabled_flags(); diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap index 26c16791c..e9820815f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap @@ -5,7 +5,7 @@ expression: "format!(\"{buf:?}\")" Buffer { area: Rect { x: 0, y: 0, width: 50, height: 1 }, content: [ - " 1 background terminal running · /ps to view · /c", + " 1 background terminal running · /ps to view · /s", ], styles: [ x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, diff --git a/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs b/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs index f0b7afcaf..3714aa495 100644 --- a/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs +++ b/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs @@ -50,7 +50,7 @@ impl UnifiedExecFooter { let count = self.processes.len(); let plural = if count == 1 { "" } else { "s" }; Some(format!( - "{count} background terminal{plural} running · /ps to view · /clean to close" + "{count} background terminal{plural} running · /ps to view · /stop to close" )) } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 4c8e76874..76d7a5ecf 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2172,9 +2172,6 @@ impl ChatWidget { fn on_interrupted_turn(&mut self, reason: TurnAbortReason) { // Finalize, log a gentle prompt, and clear running state. self.finalize_turn(); - if reason == TurnAbortReason::Interrupted { - self.clear_unified_exec_processes(); - } let send_pending_steers_immediately = self.submit_pending_steers_after_interrupt; self.submit_pending_steers_after_interrupt = false; if reason != TurnAbortReason::ReviewEnded { @@ -2834,14 +2831,6 @@ impl ChatWidget { } } - fn clear_unified_exec_processes(&mut self) { - if self.unified_exec_processes.is_empty() { - return; - } - self.unified_exec_processes.clear(); - self.sync_unified_exec_footer(); - } - fn on_mcp_tool_call_begin(&mut self, ev: McpToolCallBeginEvent) { let ev2 = ev.clone(); self.defer_or_handle(|q| q.push_mcp_begin(ev), |s| s.handle_mcp_begin_now(ev2)); @@ -4282,7 +4271,7 @@ impl ChatWidget { } pub(crate) fn can_run_ctrl_l_clear_now(&mut self) -> bool { - // Ctrl+L is not a slash command, but it follows /clear's current rule: + // Ctrl+L is not a slash command, but it follows /clear's rule: // block while a task is running. if !self.bottom_pane.is_task_running() { return true; @@ -4557,7 +4546,7 @@ impl ChatWidget { SlashCommand::Ps => { self.add_ps_output(); } - SlashCommand::Clean => { + SlashCommand::Stop => { self.clean_background_terminals(); } SlashCommand::MemoryDrop => { @@ -8601,7 +8590,7 @@ impl ChatWidget { /// /// The first press arms a time-bounded quit shortcut and shows a footer hint via the bottom /// pane. If cancellable work is active, Ctrl+C also submits `Op::Interrupt` after the shortcut - /// is armed. + /// is armed; this interrupts the turn but intentionally preserves background terminals. /// /// If the same quit shortcut is pressed again before expiry, this requests a shutdown-first /// quit. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__interrupt_clears_unified_exec_wait_streak.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__interrupt_preserves_unified_exec_wait_streak.snap similarity index 100% rename from codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__interrupt_clears_unified_exec_wait_streak.snap rename to codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__interrupt_preserves_unified_exec_wait_streak.snap diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 730aa9900..907020e5b 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -3820,7 +3820,7 @@ async fn enqueueing_history_prompt_multiple_times_is_stable() { } #[tokio::test] -async fn streaming_final_answer_keeps_task_running_state() { +async fn streaming_final_answer_ctrl_c_interrupt_preserves_background_shells() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; chat.thread_id = Some(ThreadId::new()); @@ -3843,12 +3843,16 @@ async fn streaming_final_answer_keeps_task_running_state() { ); assert_matches!(op_rx.try_recv(), Err(TryRecvError::Empty)); + begin_unified_exec_startup(&mut chat, "call-1", "process-1", "npm run dev"); + assert_eq!(chat.unified_exec_processes.len(), 1); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL)); match op_rx.try_recv() { Ok(Op::Interrupt) => {} other => panic!("expected Op::Interrupt, got {other:?}"), } assert!(!chat.bottom_pane.quit_shortcut_hint_visible()); + assert_eq!(chat.unified_exec_processes.len(), 1); } #[tokio::test] @@ -6025,10 +6029,10 @@ async fn slash_exit_requests_exit() { } #[tokio::test] -async fn slash_clean_submits_background_terminal_cleanup() { +async fn slash_stop_submits_background_terminal_cleanup() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; - chat.dispatch_command(SlashCommand::Clean); + chat.dispatch_command(SlashCommand::Stop); assert_matches!(op_rx.try_recv(), Ok(Op::CleanBackgroundTerminals)); let cells = drain_insert_history(&mut rx); @@ -9061,7 +9065,7 @@ async fn interrupt_prepends_queued_messages_before_existing_composer_text() { } #[tokio::test] -async fn interrupt_clears_unified_exec_processes() { +async fn interrupt_keeps_unified_exec_processes() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; begin_unified_exec_startup(&mut chat, "call-1", "process-1", "sleep 5"); @@ -9076,7 +9080,7 @@ async fn interrupt_clears_unified_exec_processes() { }), }); - assert!(chat.unified_exec_processes.is_empty()); + assert_eq!(chat.unified_exec_processes.len(), 2); let _ = drain_insert_history(&mut rx); } @@ -9119,7 +9123,7 @@ async fn review_ended_keeps_unified_exec_processes() { } #[tokio::test] -async fn interrupt_clears_unified_exec_wait_streak_snapshot() { +async fn interrupt_preserves_unified_exec_wait_streak_snapshot() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; chat.handle_codex_event(Event { @@ -9150,7 +9154,7 @@ async fn interrupt_clears_unified_exec_wait_streak_snapshot() { .collect::>() .join("\n"); let snapshot = format!("cells={}\n{combined}", cells.len()); - assert_snapshot!("interrupt_clears_unified_exec_wait_streak", snapshot); + assert_snapshot!("interrupt_preserves_unified_exec_wait_streak", snapshot); } #[tokio::test] diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index f3be1b4db..d83135c2f 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -48,7 +48,8 @@ pub enum SlashCommand { Feedback, Rollout, Ps, - Clean, + #[strum(to_string = "stop", serialize = "clean")] + Stop, Clear, Personality, Realtime, @@ -87,7 +88,7 @@ impl SlashCommand { SlashCommand::Statusline => "configure which items appear in the status line", SlashCommand::Theme => "choose a syntax highlighting theme", SlashCommand::Ps => "list background terminals", - SlashCommand::Clean => "stop all background terminals", + SlashCommand::Stop => "stop all background terminals", SlashCommand::MemoryDrop => "DO NOT USE", SlashCommand::MemoryUpdate => "DO NOT USE", SlashCommand::Model => "choose what model and reasoning effort to use", @@ -162,7 +163,7 @@ impl SlashCommand { | SlashCommand::Status | SlashCommand::DebugConfig | SlashCommand::Ps - | SlashCommand::Clean + | SlashCommand::Stop | SlashCommand::Mcp | SlashCommand::Apps | SlashCommand::Feedback @@ -196,3 +197,21 @@ pub fn built_in_slash_commands() -> Vec<(&'static str, SlashCommand)> { .map(|c| (c.command(), c)) .collect() } + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + use std::str::FromStr; + + use super::SlashCommand; + + #[test] + fn stop_command_is_canonical_name() { + assert_eq!(SlashCommand::Stop.command(), "stop"); + } + + #[test] + fn clean_alias_parses_to_stop_command() { + assert_eq!(SlashCommand::from_str("clean"), Ok(SlashCommand::Stop)); + } +}