From 4ffeddcbcc0dd72251433abff1ca9423e8017008 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 6 Aug 2026 22:17:13 +0000 Subject: [PATCH] Fix subagent MCP startup status settling (#37344) ## Why Subagents can leave cached MCP servers deferred indefinitely, causing the TUI to keep showing MCP startup as running after every server that reported startup has settled. ## What changed - Clear configured MCP startup expectations for active subagent threads so their status is driven by actual startup notifications. - Preserve configured-server startup tracking for primary threads and side conversations. ## Testing Add coverage for active and resumed subagents with deferred servers, plus the existing side-conversation behavior. GitOrigin-RevId: 4d929a162dd067b4a76351652a764a79ce60f31f --- codex-rs/tui/src/app/app_server_events.rs | 16 +++ codex-rs/tui/src/app/tests.rs | 2 + codex-rs/tui/src/app/tests/mcp_startup.rs | 154 ++++++++++++++++++++++ 3 files changed, 172 insertions(+) create mode 100644 codex-rs/tui/src/app/tests/mcp_startup.rs diff --git a/codex-rs/tui/src/app/app_server_events.rs b/codex-rs/tui/src/app/app_server_events.rs index 2a8c26b458..72c9de441f 100644 --- a/codex-rs/tui/src/app/app_server_events.rs +++ b/codex-rs/tui/src/app/app_server_events.rs @@ -18,6 +18,22 @@ use codex_app_server_protocol::ServerRequest; impl App { pub(super) fn refresh_mcp_startup_expected_servers_from_config(&mut self) { + if self + .current_displayed_thread_id() + .zip(self.primary_thread_id) + .is_some_and(|(thread_id, primary_thread_id)| { + self.agent_navigation.is_parent_owned(thread_id) + || (thread_id != primary_thread_id + && !self.side_threads.contains_key(&thread_id)) + }) + { + // Subagents can defer cached servers indefinitely, so only servers + // that actually report startup should keep their status running. + self.chat_widget + .set_mcp_startup_expected_servers(std::iter::empty()); + return; + } + let enabled_config_mcp_servers: Vec = self .config .mcp_servers diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 4d2193b307..15fd93a7bc 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -4,6 +4,8 @@ mod advanced_reasoning_tests; #[path = "tests/key_chords.rs"] mod key_chords; +#[path = "tests/mcp_startup.rs"] +mod mcp_startup; mod model_catalog; mod plugin_catalog; mod rate_limits; diff --git a/codex-rs/tui/src/app/tests/mcp_startup.rs b/codex-rs/tui/src/app/tests/mcp_startup.rs new file mode 100644 index 0000000000..40ac1c6688 --- /dev/null +++ b/codex-rs/tui/src/app/tests/mcp_startup.rs @@ -0,0 +1,154 @@ +use super::*; +use pretty_assertions::assert_eq; + +fn configure_mcp_servers(app: &mut App) { + let config: codex_config::types::McpServerConfig = + toml::from_str::("command = 'true'") + .expect("test MCP config should parse") + .try_into() + .expect("test MCP config should deserialize"); + app.config + .mcp_servers + .set(HashMap::from([ + ("eager".to_string(), config.clone()), + ("deferred".to_string(), config), + ])) + .expect("test MCP servers should accept any configuration"); +} + +#[tokio::test] +async fn subagent_mcp_startup_settles_while_cached_servers_remain_deferred() { + let (mut app, _app_event_rx, _op_rx) = make_test_app_with_channels().await; + configure_mcp_servers(&mut app); + let app_server = crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()) + .await + .expect("embedded app server"); + let root_thread_id = ThreadId::new(); + let subagent_thread_id = ThreadId::new(); + app.primary_thread_id = Some(root_thread_id); + app.upsert_agent_picker_thread( + subagent_thread_id, + /*agent_nickname*/ None, + /*agent_role*/ None, + /*is_closed*/ false, + ); + app.ensure_thread_channel(subagent_thread_id); + app.activate_thread_channel(subagent_thread_id).await; + app.replay_thread_snapshot( + ThreadEventSnapshot { + session: Some(test_thread_session( + subagent_thread_id, + test_path_buf("/tmp/subagent"), + )), + turns: Vec::new(), + events: Vec::new(), + input_state: None, + }, + /*resume_restored_queue*/ false, + ); + + let mut visible_startup_states = Vec::new(); + for (name, status, task_running) in [ + ("eager", McpServerStartupState::Starting, true), + ("eager", McpServerStartupState::Ready, false), + ("deferred", McpServerStartupState::Starting, true), + ("deferred", McpServerStartupState::Ready, false), + ] { + app.handle_app_server_event( + &app_server, + codex_app_server_client::AppServerEvent::ServerNotification(Box::new( + ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification { + thread_id: Some(subagent_thread_id.to_string()), + name: name.to_string(), + status, + error: None, + failure_reason: None, + }), + )), + ) + .await; + let event = app + .active_thread_rx + .as_mut() + .expect("subagent receiver should be active") + .try_recv() + .expect("MCP startup update should reach the active subagent"); + app.handle_thread_event_now(event); + + assert_eq!(app.chat_widget.is_task_running_for_test(), task_running); + let rendered = render_bottom_popup(&app.chat_widget, /*width*/ 80); + let visible_status = rendered + .lines() + .find(|line| line.contains("Booting MCP server:")) + .and_then(|line| line.split_once(" (")) + .map_or("idle", |(status, _)| status); + visible_startup_states.push(format!("{name}: {visible_status}")); + } + + insta::assert_snapshot!(visible_startup_states.join("\n"), @r" + eager: • Booting MCP server: eager + eager: idle + deferred: • Booting MCP server: deferred + deferred: idle + "); +} + +#[tokio::test] +async fn resumed_subagent_mcp_startup_settles_while_cached_servers_remain_deferred() { + let mut app = make_test_app().await; + configure_mcp_servers(&mut app); + let subagent_thread_id = ThreadId::new(); + app.primary_thread_id = Some(subagent_thread_id); + app.active_thread_id = Some(subagent_thread_id); + app.agent_navigation.mark_parent_owned(subagent_thread_id); + app.refresh_mcp_startup_expected_servers_from_config(); + + for status in [ + McpServerStartupState::Starting, + McpServerStartupState::Ready, + ] { + app.chat_widget.handle_server_notification( + ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification { + thread_id: Some(subagent_thread_id.to_string()), + name: "eager".to_string(), + status, + error: None, + failure_reason: None, + }), + /*replay_kind*/ None, + ); + } + + assert!(!app.chat_widget.is_task_running_for_test()); +} + +#[tokio::test] +async fn side_conversations_wait_for_every_configured_mcp_server() { + let mut app = make_test_app().await; + configure_mcp_servers(&mut app); + let root_thread_id = ThreadId::new(); + let side_thread_id = ThreadId::new(); + app.primary_thread_id = Some(root_thread_id); + app.side_threads + .insert(side_thread_id, SideThreadState::new(root_thread_id)); + app.active_thread_id = Some(side_thread_id); + app.refresh_mcp_startup_expected_servers_from_config(); + + for status in [ + McpServerStartupState::Starting, + McpServerStartupState::Ready, + ] { + app.chat_widget.handle_server_notification( + ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification { + thread_id: Some(side_thread_id.to_string()), + name: "eager".to_string(), + status, + error: None, + failure_reason: None, + }), + /*replay_kind*/ None, + ); + } + + assert!(app.chat_widget.is_task_running_for_test()); +}