From 5c18cc0acc3734f0e78e422a7fd94ea4a2be652e Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 20 Jul 2026 16:00:30 +0000 Subject: [PATCH] Clear stale Guardian reviews when turns end (#34371) ## Why Pending Guardian review state could outlive an interrupted or completed turn, leaving stale review details visible during a following goal turn. ## What changed - Clear pending Guardian reviews during both turn completion and finalization. - Restore an active MCP startup header after cleanup, falling back to `Working` when no MCP server is starting. ## Testing Added coverage for interrupted and completed turns, including goal continuation and overlapping MCP startup status. GitOrigin-RevId: a15ed81a5d6005e9294089f4424a8dedbc6e9cb5 --- codex-rs/tui/src/chatwidget/mcp_startup.rs | 73 ++++---- ...goal_continuation_drops_stale_reviews.snap | 11 ++ codex-rs/tui/src/chatwidget/status_state.rs | 4 + codex-rs/tui/src/chatwidget/tests/guardian.rs | 167 +++++++++++------- codex-rs/tui/src/chatwidget/turn_runtime.rs | 12 ++ 5 files changed, 163 insertions(+), 104 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_goal_continuation_drops_stale_reviews.snap diff --git a/codex-rs/tui/src/chatwidget/mcp_startup.rs b/codex-rs/tui/src/chatwidget/mcp_startup.rs index 32839b437f..4407e1b3e7 100644 --- a/codex-rs/tui/src/chatwidget/mcp_startup.rs +++ b/codex-rs/tui/src/chatwidget/mcp_startup.rs @@ -134,46 +134,47 @@ impl ChatWidget { self.finish_mcp_startup(failed, cancelled); return; } - if let Some(current) = &self.mcp_startup_status { - // Otherwise keep the status header focused on the remaining - // in-progress servers for the active round. - let total = current.len(); - let mut starting: Vec<_> = current - .iter() - .filter_map(|(name, state)| { - if matches!(state, McpStartupStatus::Starting) { - Some(name) - } else { - None - } - }) - .collect(); - starting.sort(); - if let Some(first) = starting.first() { - let completed = total.saturating_sub(starting.len()); - let max_to_show = 3; - let mut to_show: Vec = starting - .iter() - .take(max_to_show) - .map(ToString::to_string) - .collect(); - if starting.len() > max_to_show { - to_show.push("…".to_string()); - } - let header = if total > 1 { - format!( - "{MCP_STARTUP_MULTI_HEADER_PREFIX} ({completed}/{total}): {}", - to_show.join(", ") - ) - } else { - format!("{MCP_STARTUP_SINGLE_HEADER_PREFIX} {first}") - }; - self.set_status_header(header); - } + if let Some(header) = self.mcp_startup_status_header() { + self.set_status_header(header); } self.request_redraw(); } + pub(super) fn mcp_startup_status_header(&self) -> Option { + let current = self.mcp_startup_status.as_ref()?; + let total = current.len(); + let mut starting: Vec<_> = current + .iter() + .filter_map(|(name, state)| { + if matches!(state, McpStartupStatus::Starting) { + Some(name) + } else { + None + } + }) + .collect(); + starting.sort(); + let first = starting.first()?; + let completed = total.saturating_sub(starting.len()); + let max_to_show = 3; + let mut to_show: Vec = starting + .iter() + .take(max_to_show) + .map(ToString::to_string) + .collect(); + if starting.len() > max_to_show { + to_show.push("…".to_string()); + } + Some(if total > 1 { + format!( + "{MCP_STARTUP_MULTI_HEADER_PREFIX} ({completed}/{total}): {}", + to_show.join(", ") + ) + } else { + format!("{MCP_STARTUP_SINGLE_HEADER_PREFIX} {first}") + }) + } + pub(crate) fn set_mcp_startup_expected_servers(&mut self, server_names: I) where I: IntoIterator, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_goal_continuation_drops_stale_reviews.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_goal_continuation_drops_stale_reviews.snap new file mode 100644 index 0000000000..5e2806fb3d --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_goal_continuation_drops_stale_reviews.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/chatwidget/tests/guardian.rs +expression: normalize_snapshot_paths(rendered) +--- +• Reviewing approval request (0s • esc to interrupt) + └ rm -rf '/tmp/current-review' + + +› Ask Codex to do anything + + gpt-5.6-sol default · /tmp/project diff --git a/codex-rs/tui/src/chatwidget/status_state.rs b/codex-rs/tui/src/chatwidget/status_state.rs index 682e6adb95..1d0945bd74 100644 --- a/codex-rs/tui/src/chatwidget/status_state.rs +++ b/codex-rs/tui/src/chatwidget/status_state.rs @@ -63,6 +63,10 @@ impl PendingGuardianReviewStatus { self.entries.len() != original_len } + pub(super) fn clear(&mut self) { + self.entries.clear(); + } + pub(super) fn is_empty(&self) -> bool { self.entries.is_empty() } diff --git a/codex-rs/tui/src/chatwidget/tests/guardian.rs b/codex-rs/tui/src/chatwidget/tests/guardian.rs index 27a226db02..7a5fdc288d 100644 --- a/codex-rs/tui/src/chatwidget/tests/guardian.rs +++ b/codex-rs/tui/src/chatwidget/tests/guardian.rs @@ -21,6 +21,32 @@ fn auto_review_denial_event() -> GuardianAssessmentEvent { } } +fn guardian_command_event( + id: &str, + turn_id: &str, + command: &str, + status: GuardianAssessmentStatus, +) -> GuardianAssessmentEvent { + let terminal = status != GuardianAssessmentStatus::InProgress; + GuardianAssessmentEvent { + id: id.to_string(), + target_item_id: Some(format!("{id}-target")), + turn_id: turn_id.to_string(), + started_at_ms: 0, + completed_at_ms: terminal.then_some(1), + status, + risk_level: terminal.then_some(GuardianRiskLevel::High), + user_authorization: terminal.then_some(GuardianUserAuthorization::Low), + rationale: terminal.then(|| "Would delete important data.".to_string()), + decision_source: terminal.then_some(GuardianAssessmentDecisionSource::Agent), + action: GuardianAssessmentAction::Command { + source: GuardianCommandSource::Shell, + command: command.to_string(), + cwd: test_path_buf("/tmp").abs(), + }, + } +} + #[tokio::test] async fn auto_review_denials_popup_lists_stored_auto_review_denials() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; @@ -525,23 +551,12 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() { ("guardian-1", "rm -rf '/tmp/guardian target 1'"), ("guardian-2", "rm -rf '/tmp/guardian target 2'"), ] { - chat.on_guardian_assessment(GuardianAssessmentEvent { - id: id.to_string(), - target_item_id: Some(format!("{id}-target")), - turn_id: "turn-1".to_string(), - started_at_ms: 0, - completed_at_ms: None, - status: GuardianAssessmentStatus::InProgress, - risk_level: None, - user_authorization: None, - rationale: None, - decision_source: None, - action: GuardianAssessmentAction::Command { - source: GuardianCommandSource::Shell, - command: command.to_string(), - cwd: test_path_buf("/tmp").abs(), - }, - }); + chat.on_guardian_assessment(guardian_command_event( + id, + "turn-1", + command, + GuardianAssessmentStatus::InProgress, + )); } let rendered = render_bottom_popup(&chat, /*width*/ 72); @@ -556,57 +571,24 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.on_task_started(); - chat.on_guardian_assessment(GuardianAssessmentEvent { - id: "guardian-1".to_string(), - target_item_id: Some("guardian-1-target".to_string()), - turn_id: "turn-1".to_string(), - started_at_ms: 0, - completed_at_ms: None, - status: GuardianAssessmentStatus::InProgress, - risk_level: None, - user_authorization: None, - rationale: None, - decision_source: None, - action: GuardianAssessmentAction::Command { - source: GuardianCommandSource::Shell, - command: "rm -rf '/tmp/guardian target 1'".to_string(), - cwd: test_path_buf("/tmp").abs(), - }, - }); - chat.on_guardian_assessment(GuardianAssessmentEvent { - id: "guardian-2".to_string(), - target_item_id: Some("guardian-2-target".to_string()), - turn_id: "turn-1".to_string(), - started_at_ms: 0, - completed_at_ms: None, - status: GuardianAssessmentStatus::InProgress, - risk_level: None, - user_authorization: None, - rationale: None, - decision_source: None, - action: GuardianAssessmentAction::Command { - source: GuardianCommandSource::Shell, - command: "rm -rf '/tmp/guardian target 2'".to_string(), - cwd: test_path_buf("/tmp").abs(), - }, - }); - chat.on_guardian_assessment(GuardianAssessmentEvent { - id: "guardian-1".to_string(), - target_item_id: Some("guardian-1-target".to_string()), - turn_id: "turn-1".to_string(), - started_at_ms: 0, - completed_at_ms: Some(1), - status: GuardianAssessmentStatus::Denied, - risk_level: Some(GuardianRiskLevel::High), - user_authorization: Some(GuardianUserAuthorization::Low), - rationale: Some("Would delete important data.".to_string()), - decision_source: Some(GuardianAssessmentDecisionSource::Agent), - action: GuardianAssessmentAction::Command { - source: GuardianCommandSource::Shell, - command: "rm -rf '/tmp/guardian target 1'".to_string(), - cwd: test_path_buf("/tmp").abs(), - }, - }); + chat.on_guardian_assessment(guardian_command_event( + "guardian-1", + "turn-1", + "rm -rf '/tmp/guardian target 1'", + GuardianAssessmentStatus::InProgress, + )); + chat.on_guardian_assessment(guardian_command_event( + "guardian-2", + "turn-1", + "rm -rf '/tmp/guardian target 2'", + GuardianAssessmentStatus::InProgress, + )); + chat.on_guardian_assessment(guardian_command_event( + "guardian-1", + "turn-1", + "rm -rf '/tmp/guardian target 1'", + GuardianAssessmentStatus::Denied, + )); assert_eq!( chat.status_state.current_status.header, @@ -617,3 +599,52 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() Some("rm -rf '/tmp/guardian target 2'".to_string()) ); } + +#[tokio::test] +async fn guardian_cleanup_drops_stale_reviews_and_restores_mcp_status() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.set_mcp_startup_expected_servers(["alpha".to_string()]); + handle_turn_started(&mut chat, "turn-1"); + chat.on_mcp_server_status_updated(McpServerStatusUpdatedNotification { + thread_id: None, + name: "alpha".to_string(), + status: McpServerStartupState::Starting, + error: None, + failure_reason: None, + }); + chat.on_guardian_assessment(guardian_command_event( + "stale-review", + "turn-1", + "rm -rf '/tmp/stale-review'", + GuardianAssessmentStatus::InProgress, + )); + handle_turn_interrupted(&mut chat, "turn-1"); + + assert!(chat.status_state.pending_guardian_review_status.is_empty()); + assert_eq!( + chat.status_state.current_status.header, + "Booting MCP server: alpha" + ); + + handle_turn_started(&mut chat, "turn-2"); + chat.on_guardian_assessment(guardian_command_event( + "current-review", + "turn-2", + "rm -rf '/tmp/current-review'", + GuardianAssessmentStatus::InProgress, + )); + + let rendered = render_bottom_popup(&chat, /*width*/ 72); + assert_chatwidget_snapshot!( + "guardian_goal_continuation_drops_stale_reviews", + normalize_snapshot_paths(rendered) + ); + + handle_turn_completed(&mut chat, "turn-2", /*duration_ms*/ None); + assert!(chat.status_state.pending_guardian_review_status.is_empty()); + assert_eq!( + chat.status_state.current_status.header, + "Booting MCP server: alpha" + ); +} diff --git a/codex-rs/tui/src/chatwidget/turn_runtime.rs b/codex-rs/tui/src/chatwidget/turn_runtime.rs index b05cf716d4..67d4353a5c 100644 --- a/codex-rs/tui/src/chatwidget/turn_runtime.rs +++ b/codex-rs/tui/src/chatwidget/turn_runtime.rs @@ -16,6 +16,16 @@ fn is_safety_access_block_message(message: &str) -> bool { } impl ChatWidget { + fn clear_guardian_review_status(&mut self) { + self.status_state.pending_guardian_review_status.clear(); + if self.status_state.current_status.is_guardian_review() { + let header = self + .mcp_startup_status_header() + .unwrap_or_else(|| String::from("Working")); + self.set_status_header(header); + } + } + /// Synchronize the bottom-pane "task running" indicator with the current lifecycles. /// /// The bottom pane only has one running flag, but this module treats it as a derived state of @@ -177,6 +187,7 @@ impl ChatWidget { self.status_state.pending_status_indicator_restore = false; self.input_queue.user_turn_pending_start = false; self.clear_active_hook_cell(); + self.clear_guardian_review_status(); self.turn_lifecycle.finish(); self.clear_safety_buffering(); self.update_task_running_state(); @@ -322,6 +333,7 @@ impl ChatWidget { self.clear_active_hook_cell(); // Reset running state and clear streaming buffers. self.input_queue.user_turn_pending_start = false; + self.clear_guardian_review_status(); self.turn_lifecycle.finish(); self.update_task_running_state(); self.running_commands.clear();