mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.
This commit is contained in:
committed by
GitHub
Unverified
parent
6a9a49b334
commit
8a299fb704
@@ -698,6 +698,25 @@ fn archived_session_guidance(err: &color_eyre::eyre::Report) -> Option<String> {
|
||||
Some(message.to_string())
|
||||
}
|
||||
|
||||
fn active_turn_interrupt_race(error: &TypedRequestError) -> Option<String> {
|
||||
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,
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 => {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
+4
-4
@@ -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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user