Preserve background terminals on interrupt and rename cleanup command to /stop (#14602)
### 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)
This commit is contained in:
friel-openai
GitHub
parent
d4af6053e2
commit
ba463a9dc7
12 changed files with 112 additions and 65 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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<TurnContext>) {
|
||||
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
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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()),
|
||||
|
|
|
|||
|
|
@ -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(())
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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<SlashCommand> {
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
))
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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::<Vec<_>>()
|
||||
.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]
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue