From 8a299fb7043b645cecb50b239beb7d0e9e4ac7e7 Mon Sep 17 00:00:00 2001 From: canvrno-oai Date: Tue, 9 Jun 2026 09:41:58 -0700 Subject: [PATCH] fix: Prevent /review crash when entering Esc on steer message (#22879) This changes the `/review` escape path so `Esc` no longer behaves like the normal queued-follow-up interrupt flow while a review is running. Steering is not currently supported in `/review` mode, without this change users are able to attempt a steer but it leads to a crash (see #22815). If the user has already tried to send additional guidance during `/review`, the TUI now keeps the review running and shows a warning that steer messages are not supported in that mode, while still pointing users to `Ctrl+C` if they actually want to cancel. It also adds regression coverage for the review-specific warning behavior. When users do cancel with Ctrl+C during /review, the TUI now tolerates the active-turn race that can happen during review handoff, and any queued steer messages are restored to the composer instead of being discarded. - Special-case `Esc` during an active `/review` when follow-up steer input is pending or has already been deferred. - Show a clear warning instead of interrupting the running review. - Make the Ctrl+C cancel path during /review resilient to active-turn races, while preserving any queued steer text by restoring it to the composer. - Add review-mode test coverage for the warning path. ## Testing 1. Start a `/review` with a diff large enough that the review stays active for more than a few seconds. 2. While the review is still running, type a follow-up / steer message, submit it, and then press `Esc`. Before: `Esc` causes the TUI to close abruptly. After: the review keeps running and the transcript shows a warning that steer messages are not supported during `/review`, with guidance to use `Ctrl+C` if you want to cancel. 3. Press `Ctrl+C` if you actually want to stop the review. Before: (after restarting the test since Pt. 2 crashed) this is the intentional cancellation path. After: this remains the intentional cancellation path, and any queued follow-up steer text is restored to the composer instead of being lost. ## Note: `/review` mode explicitly does not support steering at this time (as noted in `turn_processer.rs`, if we want to explore that in the future this code will need to be modified). This change keeps unsupported steer attempts from crashing the TUI and preserves queued follow-up text if the user cancels with Ctrl+C. --- codex-rs/tui/src/app.rs | 19 +++++++++ codex-rs/tui/src/app/tests.rs | 17 ++++++++ codex-rs/tui/src/app/thread_routing.rs | 41 +++++++++++++++++-- codex-rs/tui/src/app_server_session.rs | 10 +++-- codex-rs/tui/src/chatwidget/interaction.rs | 14 +++++++ ...s__review_submission_warning_snapshot.snap | 8 ++-- .../tui/src/chatwidget/tests/review_mode.rs | 23 +++++++++++ 7 files changed, 121 insertions(+), 11 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 63a5d4a1c..e5a3d0732 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -698,6 +698,25 @@ fn archived_session_guidance(err: &color_eyre::eyre::Report) -> Option { Some(message.to_string()) } +fn active_turn_interrupt_race(error: &TypedRequestError) -> Option { + let TypedRequestError::Server { method, source } = error else { + return None; + }; + if method != "turn/interrupt" { + return None; + } + let mismatch_prefix = "expected active turn id "; + let mismatch_separator = " but found "; + Some( + source + .message + .strip_prefix(mismatch_prefix)? + .split_once(mismatch_separator)? + .1 + .to_string(), + ) +} + impl App { pub fn chatwidget_init_for_forked_or_resumed_thread( &self, diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 99d4c3533..ed89c7287 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -4709,6 +4709,23 @@ fn active_turn_steer_race_extracts_actual_turn_id_from_mismatch() { ); } +#[test] +fn active_turn_interrupt_race_extracts_actual_turn_id_from_mismatch() { + let error = TypedRequestError::Server { + method: "turn/interrupt".to_string(), + source: JSONRPCErrorError { + code: -32602, + message: "expected active turn id turn-expected but found turn-actual".to_string(), + data: None, + }, + }; + + assert_eq!( + active_turn_interrupt_race(&error), + Some("turn-actual".to_string()) + ); +} + #[tokio::test] async fn fresh_session_config_uses_current_service_tier() { let mut app = make_test_app().await; diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 80b67b19d..938174254 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -500,9 +500,39 @@ impl App { match op { AppCommand::Interrupt { .. } => { if let Some(turn_id) = self.active_turn_id_for_thread(thread_id).await { - app_server.turn_interrupt(thread_id, turn_id).await?; + let mut interrupt_turn_id = turn_id; + for retried_after_turn_mismatch in [false, true] { + match app_server + .turn_interrupt(thread_id, interrupt_turn_id.clone()) + .await + { + Ok(()) => return Ok(true), + Err(error) if !retried_after_turn_mismatch => { + let Some(actual_turn_id) = active_turn_interrupt_race(&error) + else { + return Err(error).wrap_err("turn/interrupt failed in TUI"); + }; + if actual_turn_id == interrupt_turn_id { + return Err(error).wrap_err("turn/interrupt failed in TUI"); + } + // Review flows can swap the active turn before the TUI processes + // the corresponding notification. Retry once with the + // server-reported turn id so Ctrl+C/Esc do not fatally exit on that + // stale cache, but let lifecycle notifications own the cached + // active turn id. + interrupt_turn_id = actual_turn_id; + } + Err(error) => { + return Err(error).wrap_err("turn/interrupt failed in TUI"); + } + } + } + unreachable!("interrupt retry loop should return"); } else { - app_server.startup_interrupt(thread_id).await?; + app_server + .startup_interrupt(thread_id) + .await + .wrap_err("turn/interrupt failed in TUI")?; } Ok(true) } @@ -652,7 +682,12 @@ impl App { Ok(true) } AppCommand::Review { target } => { - app_server.review_start(thread_id, target.clone()).await?; + let response = app_server.review_start(thread_id, target.clone()).await?; + let review_thread_id = ThreadId::from_string(&response.review_thread_id) + .wrap_err("review/start returned invalid review thread id")?; + let store = Arc::clone(&self.ensure_thread_channel(review_thread_id).store); + let mut store = store.lock().await; + store.active_turn_id = Some(response.turn.id); Ok(true) } AppCommand::CleanBackgroundTerminals => { diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 32db3d2cf..e29b40111 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -736,7 +736,7 @@ impl AppServerSession { &mut self, thread_id: ThreadId, turn_id: String, - ) -> Result<()> { + ) -> std::result::Result<(), TypedRequestError> { let request_id = self.next_request_id(); let _: TurnInterruptResponse = self .client @@ -747,12 +747,14 @@ impl AppServerSession { turn_id, }, }) - .await - .wrap_err("turn/interrupt failed in TUI")?; + .await?; Ok(()) } - pub(crate) async fn startup_interrupt(&mut self, thread_id: ThreadId) -> Result<()> { + pub(crate) async fn startup_interrupt( + &mut self, + thread_id: ThreadId, + ) -> std::result::Result<(), TypedRequestError> { self.turn_interrupt(thread_id, String::new()).await } diff --git a/codex-rs/tui/src/chatwidget/interaction.rs b/codex-rs/tui/src/chatwidget/interaction.rs index a94d483f0..7a3385034 100644 --- a/codex-rs/tui/src/chatwidget/interaction.rs +++ b/codex-rs/tui/src/chatwidget/interaction.rs @@ -112,6 +112,20 @@ impl ChatWidget { return; } + const REVIEW_STEER_UNAVAILABLE_MESSAGE: &str = "Steer messages aren't supported during /review. Press Ctrl+C now to cancel the review."; + + if self.chat_keymap.interrupt_turn.is_pressed(key_event) + && self.review.is_review_mode + && (!self.input_queue.pending_steers.is_empty() + || !self.input_queue.rejected_steers_queue.is_empty()) + && self.bottom_pane.is_task_running() + && self.bottom_pane.no_modal_or_popup_active() + && !self.should_handle_vim_insert_escape(key_event) + { + self.add_warning_message(REVIEW_STEER_UNAVAILABLE_MESSAGE.to_string()); + return; + } + if self.chat_keymap.interrupt_turn.is_pressed(key_event) && !self.input_queue.pending_steers.is_empty() && self.bottom_pane.is_task_running() diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_submission_warning_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_submission_warning_snapshot.snap index c8bbb7c2a..1a28a9222 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_submission_warning_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_submission_warning_snapshot.snap @@ -1,7 +1,7 @@ --- source: tui/src/chatwidget/tests/review_mode.rs -expression: lines_to_single_string(last) +assertion_line: 307 +expression: last --- -⚠ Steer messages aren't supported during /review. Send your message after the - review finishes, or press Ctrl+C now to cancel the review. - +⚠ Steer messages aren't supported during /review. Press Ctrl+C now to cancel the + review. diff --git a/codex-rs/tui/src/chatwidget/tests/review_mode.rs b/codex-rs/tui/src/chatwidget/tests/review_mode.rs index faf8636ad..af5816cac 100644 --- a/codex-rs/tui/src/chatwidget/tests/review_mode.rs +++ b/codex-rs/tui/src/chatwidget/tests/review_mode.rs @@ -285,6 +285,29 @@ async fn steer_rejection_queues_review_follow_up_before_existing_queued_messages } } +#[tokio::test] +async fn esc_with_review_queued_steers_shows_warning_and_does_not_interrupt() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + handle_turn_started(&mut chat, "turn-1"); + handle_entered_review_mode(&mut chat, "feature branch"); + let _ = drain_insert_history(&mut rx); + chat.input_queue + .pending_steers + .push_back(pending_steer("review follow-up")); + chat.refresh_pending_input_preview(); + + chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + + assert!(!chat.input_queue.submit_pending_steers_after_interrupt); + assert_eq!(chat.input_queue.pending_steers.len(), 1); + assert_no_submit_op(&mut op_rx); + + let cells = drain_insert_history(&mut rx); + let last = lines_to_single_string(cells.last().expect("review warning")); + assert_chatwidget_snapshot!("review_submission_warning_snapshot", last); +} + #[tokio::test] async fn live_agent_message_renders_during_review_mode() { let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await;