mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix /review mode MCP startup render issue (#21624)
This change fixes the case where the UI can sit on _"Starting MCP servers"_ even though the review work is already running or has already completed. - MCP startup status header is visible when a `/review` turn starts with enabled MCP server startups - Restore the underlying _Working..._ status after MCP startup completes or fails - Add regression coverage for overlapping startup/turn flows and status restoration _De-scoped from a broader thread-scoped MCP status change that would have made it easier to route MCP startup statuses to the appropriate thread (parent vs. review). These changes address the UI regression without requiring more significant changes across app-server & core._ Fixes #18792.
This commit is contained in:
committed by
GitHub
Unverified
parent
3dc278b68e
commit
66af217865
@@ -11,6 +11,9 @@ use codex_app_server_protocol::McpServerStatusUpdatedNotification;
|
||||
|
||||
use super::ChatWidget;
|
||||
|
||||
const MCP_STARTUP_SINGLE_HEADER_PREFIX: &str = "Booting MCP server:";
|
||||
const MCP_STARTUP_MULTI_HEADER_PREFIX: &str = "Starting MCP servers";
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub(crate) enum McpStartupStatus {
|
||||
Starting,
|
||||
@@ -153,11 +156,11 @@ impl ChatWidget {
|
||||
}
|
||||
let header = if total > 1 {
|
||||
format!(
|
||||
"Starting MCP servers ({completed}/{total}): {}",
|
||||
"{MCP_STARTUP_MULTI_HEADER_PREFIX} ({completed}/{total}): {}",
|
||||
to_show.join(", ")
|
||||
)
|
||||
} else {
|
||||
format!("Booting MCP server: {first}")
|
||||
format!("{MCP_STARTUP_SINGLE_HEADER_PREFIX} {first}")
|
||||
};
|
||||
self.set_status_header(header);
|
||||
}
|
||||
@@ -187,12 +190,16 @@ impl ChatWidget {
|
||||
self.on_warning(format!("MCP startup incomplete ({})", parts.join("; ")));
|
||||
}
|
||||
|
||||
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;
|
||||
self.mcp_startup_allow_terminal_only_next_round = false;
|
||||
self.mcp_startup_pending_next_round.clear();
|
||||
self.mcp_startup_pending_next_round_saw_starting = false;
|
||||
self.update_task_running_state();
|
||||
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();
|
||||
}
|
||||
@@ -234,6 +241,18 @@ impl ChatWidget {
|
||||
self.finish_mcp_startup(failed, cancelled);
|
||||
}
|
||||
|
||||
pub(super) fn status_header_is_mcp_startup_owned(&self) -> bool {
|
||||
self.status_state
|
||||
.current_status
|
||||
.header
|
||||
.starts_with(MCP_STARTUP_SINGLE_HEADER_PREFIX)
|
||||
|| self
|
||||
.status_state
|
||||
.current_status
|
||||
.header
|
||||
.starts_with(MCP_STARTUP_MULTI_HEADER_PREFIX)
|
||||
}
|
||||
|
||||
pub(super) fn on_mcp_server_status_updated(
|
||||
&mut self,
|
||||
notification: McpServerStatusUpdatedNotification,
|
||||
|
||||
@@ -57,6 +57,46 @@ async fn mcp_startup_complete_does_not_clear_running_task() {
|
||||
|
||||
assert!(chat.bottom_pane.is_task_running());
|
||||
assert!(chat.bottom_pane.status_indicator_visible());
|
||||
assert_eq!(chat.status_state.current_status.header, "Working");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn turn_start_preserves_active_mcp_startup_header() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.set_mcp_startup_expected_servers(["schaltwerk".to_string()]);
|
||||
|
||||
notify_mcp_status(&mut chat, "schaltwerk", McpServerStartupState::Starting);
|
||||
handle_turn_started(&mut chat, "turn-1");
|
||||
|
||||
assert!(chat.bottom_pane.is_task_running());
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.header,
|
||||
"Booting MCP server: schaltwerk"
|
||||
);
|
||||
|
||||
notify_mcp_status(&mut chat, "schaltwerk", McpServerStartupState::Ready);
|
||||
|
||||
assert_eq!(chat.status_state.current_status.header, "Working");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn turn_start_replaces_idle_completed_mcp_startup_header() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.set_mcp_startup_expected_servers(["schaltwerk".to_string()]);
|
||||
|
||||
notify_mcp_status(&mut chat, "schaltwerk", McpServerStartupState::Starting);
|
||||
notify_mcp_status(&mut chat, "schaltwerk", McpServerStartupState::Ready);
|
||||
|
||||
assert!(!chat.bottom_pane.is_task_running());
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.header,
|
||||
"Booting MCP server: schaltwerk"
|
||||
);
|
||||
|
||||
handle_turn_started(&mut chat, "turn-1");
|
||||
|
||||
assert!(chat.bottom_pane.is_task_running());
|
||||
assert_eq!(chat.status_state.current_status.header, "Working");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -125,6 +165,82 @@ async fn app_server_mcp_startup_failure_renders_warning_history() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mcp_startup_failure_restores_running_status_header() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.show_welcome_banner = false;
|
||||
chat.set_mcp_startup_expected_servers(["alpha".to_string(), "beta".to_string()]);
|
||||
handle_turn_started(&mut chat, "turn-1");
|
||||
|
||||
notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Starting);
|
||||
notify_mcp_status(&mut chat, "beta", McpServerStartupState::Starting);
|
||||
assert!(
|
||||
chat.status_state
|
||||
.current_status
|
||||
.header
|
||||
.starts_with("Starting MCP servers")
|
||||
);
|
||||
|
||||
notify_mcp_status_error(
|
||||
&mut chat,
|
||||
"alpha",
|
||||
"MCP client for `alpha` failed to start: handshake failed",
|
||||
);
|
||||
notify_mcp_status(&mut chat, "beta", McpServerStartupState::Ready);
|
||||
let _ = drain_insert_history(&mut rx);
|
||||
|
||||
assert!(chat.bottom_pane.is_task_running());
|
||||
assert!(chat.bottom_pane.status_indicator_visible());
|
||||
assert_eq!(chat.status_state.current_status.header, "Working");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mcp_startup_complete_preserves_review_status() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.show_welcome_banner = false;
|
||||
chat.set_mcp_startup_expected_servers(["alpha".to_string()]);
|
||||
handle_turn_started(&mut chat, "turn-1");
|
||||
|
||||
notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Starting);
|
||||
assert!(
|
||||
chat.status_state
|
||||
.current_status
|
||||
.header
|
||||
.starts_with("Booting MCP server")
|
||||
);
|
||||
|
||||
chat.on_guardian_assessment(GuardianAssessmentEvent {
|
||||
id: "guardian-1".to_string(),
|
||||
target_item_id: Some("guardian-target-1".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'".to_string(),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
},
|
||||
});
|
||||
|
||||
notify_mcp_status(&mut chat, "alpha", McpServerStartupState::Ready);
|
||||
|
||||
assert!(chat.bottom_pane.is_task_running());
|
||||
assert!(chat.bottom_pane.status_indicator_visible());
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.header,
|
||||
"Reviewing approval request"
|
||||
);
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.details,
|
||||
Some("rm -rf '/tmp/guardian target'".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_server_mcp_startup_lag_settles_startup_and_ignores_late_updates() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -66,7 +66,9 @@ impl ChatWidget {
|
||||
self.bottom_pane
|
||||
.set_interrupt_hint_visible(/*visible*/ true);
|
||||
self.status_state.terminal_title_status_kind = TerminalTitleStatusKind::Working;
|
||||
self.set_status_header(String::from("Working"));
|
||||
if self.mcp_startup_status.is_none() || !self.status_header_is_mcp_startup_owned() {
|
||||
self.set_status_header(String::from("Working"));
|
||||
}
|
||||
self.full_reasoning_buffer.clear();
|
||||
self.reasoning_buffer.clear();
|
||||
self.set_ambient_pet_notification(
|
||||
|
||||
Reference in New Issue
Block a user