Preserve background terminals on interrupt and rename cleanup command to /stop#14602
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4dac1c67d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
codex-rs/tui/src/slash_command.rs
Outdated
| Rollout, | ||
| Ps, | ||
| Clean, | ||
| #[strum(serialize = "stop", serialize = "clean")] |
There was a problem hiding this comment.
Make /stop the canonical serialized name for Stop command
Defining Stop as #[strum(serialize = "stop", serialize = "clean")] breaks lookup: command() uses IntoStaticStr, which selects Strum's preferred serialization (longest string), so this variant resolves to "clean". find_builtin_command matches only that canonical name, so /stop is not recognized even though UI/docs now instruct users to run /stop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Fixed slash-command canonicalization for
Stopby removingIntoStaticStrusage and makingSlashCommand::command()explicitly return"stop"forSlashCommand::Stopwhile keeping existing behavior (as_ref()) for other commands. This ensures/stopis the canonical emitted name used by popup/lookup paths.cleanremains as a parse alias via the existing Strum serialize attributes on the enum variant. codex-rs/tui/src/slash_command.rsL1-L8 codex-rs/tui/src/slash_command.rsL48-L49 codex-rs/tui/src/slash_command.rsL113-L120 - Added a regression unit test to lock in that
SlashCommand::Stop.command()is"stop". codex-rs/tui/src/slash_command.rsL200-L208
Testing
- ✅
just fmt ⚠️ cargo test -p codex-tui(fails in this environment becausecodex-linux-sandbox’s build script cannot findlibcap/libcap.pcviapkg-config)⚠️ curl -I https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html | head(web lookup attempt blocked by proxy with HTTP 403)
Committed as 664b77a and created a follow-up PR via the make_pr tool.
There was a problem hiding this comment.
@codex No, I don't like that design. We shouldn't change the derive in codex-rs/tui/src/slash_command.rs, instead we should fix the enum variant or member.
I'm skeptical of your design.
Isn't it simpler to
#[strum(serialize = "stop", serialize = "clean")]
Stop,
Change the command variant to Clean and allow it to accept stop as an alternative serialization?
There was a problem hiding this comment.
Summary
- Updated the
Stopslash-command variant metadata to use Strum’s preferred-name override (to_string = "stop") while keeping"clean"as a parse alias, so canonical command emission remains/stopwithout changing enum derives orcommand()behavior. codex-rs/tui/src/slash_command.rsL51-L52 codex-rs/tui/src/slash_command.rsL116-L120 - Added regression tests to lock in both behaviors: canonical name for
Stopis"stop", and"clean"still parses toSlashCommand::Stop. codex-rs/tui/src/slash_command.rsL200-L215 - Committed the change as
0490d8band created a follow-up PR via themake_prtool.
Testing
- ✅
just fmt ⚠️ cargo test -p codex-tui(fails in this environment becausecodex-linux-sandboxrequires systemlibcap/libcap.pcviapkg-config)
There was a problem hiding this comment.
@codex fix these tests. the first might be a red herring - a race? but look into it anyhow
────────────
Nextest run ID 5b5b8d29-401d-4c11-abc6-3ee4f5e0d6e5 with nextest profile: default
Starting 5612 tests across 132 binaries (28 tests skipped)
SLOW [> 15.000s] (─────────) codex-core unified_exec::tests::unified_exec_timeouts
FAIL [ 0.454s] (3244/5612) codex-core::all suite::unified_exec::unified_exec_interrupt_preserves_long_running_session
stdout ───
running 1 test
test suite::unified_exec::unified_exec_interrupt_preserves_long_running_session ... FAILED
failures:
failures:
suite::unified_exec::unified_exec_interrupt_preserves_long_running_session
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 803 filtered out; finished in 0.44s
stderr ───
kill: 1155: No such process
thread 'suite::unified_exec::unified_exec_interrupt_preserves_long_running_session' (257029) panicked at core/tests/suite/unified_exec.rs:2096:5:
expected unified exec process to remain alive after interrupt
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library/std/src/panicking.rs:689:5
1: core::panicking::panic_fmt
at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library/core/src/panicking.rs:80:14
2: all::suite::unified_exec::unified_exec_interrupt_preserves_long_running_session::{{closure}}
at ./tests/suite/unified_exec.rs:2096:5
3: <core::pin::Pin<P> as core::future::future::Future>::poll
at /Users/runner/.rustup/toolchains/1.93.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/future/future.rs:133:9
4: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/park.rs:284:71
5: tokio::task::coop::with_budget
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/task/coop/mod.rs:167:5
6: tokio::task::coop::budget
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/task/coop/mod.rs:133:5
7: tokio::runtime::park::CachedParkThread::block_on
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/park.rs:284:31
8: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/context/blocking.rs:66:14
9: tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/scheduler/multi_thread/mod.rs:89:22
10: tokio::runtime::context::runtime::enter_runtime
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/context/runtime.rs:65:16
11: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/scheduler/multi_thread/mod.rs:88:9
12: tokio::runtime::runtime::Runtime::block_on_inner
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/runtime.rs:370:50
13: tokio::runtime::runtime::Runtime::block_on
at /Users/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/runtime.rs:342:18
14: all::suite::unified_exec::unified_exec_interrupt_preserves_long_running_session
at ./tests/suite/unified_exec.rs:2104:7
15: all::suite::unified_exec::unified_exec_interrupt_preserves_long_running_session::{{closure}}
at ./tests/suite/unified_exec.rs:2021:69
16: core::ops::function::FnOnce::call_once
at /Users/runner/.rustup/toolchains/1.93.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
17: core::ops::function::FnOnce::call_once
at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
FAIL [ 0.221s] (4582/5612) codex-tui bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch
stdout ───
running 1 test
test bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch ... FAILED
failures:
failures:
bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1316 filtered out; finished in 0.21s
stderr ───
thread 'bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch' (273078) panicked at tui/src/bottom_pane/slash_commands.rs:95:9:
assertion failed: `(left == right)`
Diff < left / right > :
<None
>Some(
> Stop,
>)
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library/std/src/panicking.rs:689:5
1: core::panicking::panic_fmt
at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library/core/src/panicking.rs:80:14
2: codex_tui::bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch
at ./src/bottom_pane/slash_commands.rs:95:9
3: codex_tui::bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch::{{closure}}
at ./src/bottom_pane/slash_commands.rs:94:51
4: core::ops::function::FnOnce::call_once
at /Users/runner/.rustup/toolchains/1.93.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
5: core::ops::function::FnOnce::call_once
at /rustc/254b59607d4417e9dffbc307138ae5c86280fe4c/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
────────────
Summary [ 165.095s] 5612 tests run: 5610 passed (1 slow), 2 failed, 28 skipped
FAIL [ 0.454s] (3244/5612) codex-core::all suite::unified_exec::unified_exec_interrupt_preserves_long_running_session
FAIL [ 0.221s] (4582/5612) codex-tui bottom_pane::slash_commands::tests::clean_command_alias_resolves_for_dispatch
error: test run failed
There was a problem hiding this comment.
Summary
- Updated built-in slash-command resolution to support both canonical names and aliases in dispatch:
find_builtin_commandnow first matches the visible canonical command string, then falls back to parsing aliases (likeclean) and maps them back through visible built-ins so/cleancorrectly resolves toSlashCommand::Stop. codex-rs/tui/src/bottom_pane/slash_commands.rsL39-L55 - Changed unified-exec process lifecycle so spawned processes are persisted before waiting for the initial yield window; this prevents interrupt-time cancellation from dropping an in-flight long-running process before it is tracked as a background terminal. Also simplified post-collection cleanup so exited processes are released from the store in one path. codex-rs/core/src/unified_exec/process_manager.rsL153-L267
Testing
- ✅
just fmt ⚠️ cargo test -p codex-tui clean_command_alias_resolves_for_dispatch -- --nocapture(build blocked in this environment: missing systemlibcap/libcap.pcrequired bycodex-linux-sandbox)⚠️ cargo test -p codex-core unified_exec_interrupt_preserves_long_running_session -- --nocapture(samelibcapenvironment limitation)
Committed as 1f8ef85, and I also created the follow-up PR via the make_pr tool.
2e91351 to
8334001
Compare
Preserve background unified-exec processes across interrupts, keep /stop as the cleanup command, and collapse the branch back to a single commit for easier future restacks.
8334001 to
2560335
Compare
Motivation
Description
Clean(/clean) toStop(/stop) and keptcleanas an alias in the command parsing/visibility layer, updated the user descriptions and command popup wiring accordingly./stop(and trimmed corresponding snapshot output to match the new label).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.turn/interrupt(orOp::Interrupt) interrupts the active turn but does not terminate background terminals, and thatthread/backgroundTerminals/cleanis the explicit API to stop those shells.Testing
just fmtincodex-rs(succeeded).cargo test -p codex-protocol(succeeded).cargo test -p codex-tuibut the build could not complete in this environment due to a native build dependency that requireslibcapdevelopment headers (thecodex-linux-sandboxvendored build step); installlibcap-dev/ makelibcap.pcavailable inPKG_CONFIG_PATHto run the TUI test suite locally.instasnapshots for the TUI changes so visual diffs reflect the new/stopwording and preserved interrupt behavior.Codex Task