Skip to content
Prev Previous commit
Next Next commit
revert: remove review MCP cleanup heuristic
  • Loading branch information
charliemarsh-oai committed Jul 5, 2026
commit 1257a17ebf8f77f7a65deb0252e6d5328dde0bdf
24 changes: 3 additions & 21 deletions codex-rs/tui/src/chatwidget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,9 +596,6 @@ pub(crate) struct ChatWidget {
/// as "running" while this is populated, even if no agent turn is currently
/// executing.
mcp_startup_status: Option<HashMap<String, McpStartupStatus>>,
/// Monotonically identifies startup rounds so review cleanup only clears a round it observed
/// starting after review entry.
mcp_startup_generation: u64,
/// Expected MCP servers for the current startup round, seeded from enabled local config.
mcp_startup_expected_servers: Option<HashSet<String>>,
/// After startup settles, ignore stale updates until enough notifications confirm a new round.
Expand Down Expand Up @@ -1246,34 +1243,19 @@ impl ChatWidget {
if self.review.pre_review_token_info.is_none() {
self.review.pre_review_token_info = Some(self.token_info.clone());
}
if !from_replay {
self.review.mcp_startup_generation_at_entry = Some(self.mcp_startup_generation);
if !self.bottom_pane.is_task_running() {
self.bottom_pane.set_task_running(/*running*/ true);
}
if !from_replay && !self.bottom_pane.is_task_running() {
self.bottom_pane.set_task_running(/*running*/ true);
}
self.review.is_review_mode = true;
let banner = format!(">> Code review started: {hint} <<");
self.add_to_history(history_cell::new_review_status_line(banner));
self.request_redraw();
}

fn exit_review_mode_after_item(&mut self, from_replay: bool) {
fn exit_review_mode_after_item(&mut self) {
self.flush_answer_stream_with_separator();
self.flush_interrupt_queue();
self.flush_active_cell();
let mcp_startup_generation_at_entry = self.review.mcp_startup_generation_at_entry.take();
// Interrupting a live review can close its child event stream before terminal startup
// updates arrive. Clear only a startup round first observed after entering review; an
// inherited generation belongs to the parent runtime and must finish normally. Replayed
// review items must not clear newer live startup state.
if !from_replay
&& self.review.is_review_mode
&& self.mcp_startup_status.is_some()
&& mcp_startup_generation_at_entry != Some(self.mcp_startup_generation)
{
self.clear_mcp_startup_state();
}
self.review.is_review_mode = false;
self.restore_pre_review_token_info();
self.add_to_history(history_cell::new_review_status_line(
Expand Down
1 change: 0 additions & 1 deletion codex-rs/tui/src/chatwidget/constructor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ impl ChatWidget {
task_complete_pending: false,
unified_exec_processes: Vec::new(),
mcp_startup_status: None,
mcp_startup_generation: 0,
mcp_startup_expected_servers: None,
mcp_startup_ignore_updates_until_next_start: false,
mcp_startup_allow_terminal_only_next_round: false,
Expand Down
10 changes: 1 addition & 9 deletions codex-rs/tui/src/chatwidget/mcp_startup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ impl ChatWidget {
status: McpStartupStatus,
complete_when_settled: bool,
) {
let had_active_round = self.mcp_startup_status.is_some();
let mut activated_pending_round = false;
let startup_status = if self.mcp_startup_ignore_updates_until_next_start {
// Ignore-mode buffers the next plausible round so stale post-finish
Expand Down Expand Up @@ -103,9 +102,6 @@ impl ChatWidget {
}
}
}
if !had_active_round {
self.mcp_startup_generation = self.mcp_startup_generation.wrapping_add(1);
}
self.mcp_startup_status = Some(startup_status);
self.update_task_running_state();

Expand Down Expand Up @@ -200,11 +196,6 @@ impl ChatWidget {
self.on_warning(format!("MCP startup incomplete ({})", parts.join("; ")));
}

self.clear_mcp_startup_state();
self.maybe_send_next_queued_input();
}

pub(super) fn clear_mcp_startup_state(&mut self) {
let mcp_startup_owned_status = self.status_header_is_mcp_startup_owned();
self.mcp_startup_status = None;
self.mcp_startup_ignore_updates_until_next_start = true;
Expand All @@ -215,6 +206,7 @@ impl ChatWidget {
if self.bottom_pane.is_task_running() && mcp_startup_owned_status {
self.restore_reasoning_status_header();
}
self.maybe_send_next_queued_input();
self.request_redraw();
}

Expand Down
2 changes: 1 addition & 1 deletion codex-rs/tui/src/chatwidget/replay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ impl ChatWidget {
}
}
ThreadItem::ExitedReviewMode { .. } => {
self.exit_review_mode_after_item(from_replay);
self.exit_review_mode_after_item();
}
ThreadItem::ContextCompaction { .. } => {
self.add_info_message("Context compacted".to_string(), /*hint*/ None);
Expand Down
2 changes: 0 additions & 2 deletions codex-rs/tui/src/chatwidget/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ pub(super) struct ReviewState {
pub(super) recent_auto_review_denials: RecentAutoReviewDenials,
/// Simple review mode flag; used to adjust layout and banners.
pub(super) is_review_mode: bool,
/// MCP startup generation active when the current live review began.
pub(super) mcp_startup_generation_at_entry: Option<u64>,
/// Snapshot of token usage to restore after review mode exits.
pub(super) pre_review_token_info: Option<Option<TokenUsageInfo>>,
}

This file was deleted.

101 changes: 0 additions & 101 deletions codex-rs/tui/src/chatwidget/tests/mcp_startup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,107 +142,6 @@ async fn mcp_startup_complete_does_not_clear_running_task() {
assert_eq!(chat.status_state.current_status.header, "Working");
}

#[tokio::test]
async fn interrupted_review_clears_mcp_startup_and_reenables_review() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.show_welcome_banner = false;
chat.set_mcp_startup_expected_servers(["pending".to_string()]);
handle_turn_started(&mut chat, "turn-1");
handle_entered_review_mode(&mut chat, "current changes");
notify_mcp_status(&mut chat, "pending", McpServerStartupState::Starting);

assert!(chat.mcp_startup_status.is_some());
assert!(chat.bottom_pane.is_task_running());

handle_exited_review_mode(&mut chat);
handle_turn_interrupted(&mut chat, "turn-1");

assert!(chat.mcp_startup_status.is_none());
assert!(!chat.bottom_pane.is_task_running());

chat.bottom_pane
.set_composer_text("/review".to_string(), Vec::new(), Vec::new());
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));

let height = chat.desired_height(/*width*/ 80);
let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(80, height))
.expect("create terminal");
terminal
.draw(|f| chat.render(f.area(), f.buffer_mut()))
.expect("draw chat widget");
assert_chatwidget_snapshot!(
"interrupted_review_reenables_review",
normalized_backend_snapshot(terminal.backend())
);
}

#[tokio::test]
async fn interrupted_review_restores_queued_input_without_submitting() {
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.set_mcp_startup_expected_servers(["pending".to_string()]);
handle_entered_review_mode(&mut chat, "current changes");
notify_mcp_status(&mut chat, "pending", McpServerStartupState::Starting);
chat.input_queue
.queued_user_messages
.push_back(UserMessage::from("edit this queued input").into());
chat.refresh_pending_input_preview();

handle_exited_review_mode(&mut chat);

assert_eq!(chat.input_queue.queued_user_messages.len(), 1);
assert_no_submit_op(&mut op_rx);

handle_turn_interrupted(&mut chat, "turn-1");

assert!(chat.input_queue.queued_user_messages.is_empty());
assert_eq!(chat.bottom_pane.composer_text(), "edit this queued input");
assert_no_submit_op(&mut op_rx);
}

#[tokio::test]
async fn review_exit_preserves_parent_mcp_refresh() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.set_mcp_startup_expected_servers(["pending".to_string()]);
notify_mcp_status(&mut chat, "pending", McpServerStartupState::Starting);

handle_entered_review_mode(&mut chat, "current changes");
handle_exited_review_mode(&mut chat);

assert!(chat.mcp_startup_status.is_some());
assert!(chat.bottom_pane.is_task_running());

notify_mcp_status_error(&mut chat, "pending", "parent MCP refresh failed");

assert!(chat.mcp_startup_status.is_none());
assert!(!chat.bottom_pane.is_task_running());
let history = drain_insert_history(&mut rx)
.iter()
.map(|lines| lines_to_single_string(lines))
.collect::<String>();
assert!(history.contains("parent MCP refresh failed"));
assert!(history.contains("MCP startup incomplete (failed: pending)"));
}

#[tokio::test]
async fn replayed_review_exit_preserves_live_mcp_startup() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.set_mcp_startup_expected_servers(["pending".to_string()]);
replay_entered_review_mode(&mut chat, "current changes");
notify_mcp_status(&mut chat, "pending", McpServerStartupState::Starting);

chat.replay_thread_item(
AppServerThreadItem::ExitedReviewMode {
id: "review-end".to_string(),
review: String::new(),
},
"turn-1".to_string(),
ReplayKind::ThreadSnapshot,
);

assert!(chat.mcp_startup_status.is_some());
assert!(chat.bottom_pane.is_task_running());
}

#[tokio::test]
async fn turn_start_preserves_active_mcp_startup_header() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
Expand Down