mirror of
https://github.com/openai/codex.git
synced 2026-11-06 00:30:29 +00:00
fix(tui): scope MCP startup status by thread (#26639)
## Why MCP startup failures from spawned subagents were rendered as global notifications, so a child thread's failure could pollute the visible parent transcript. Routing the notification to the child exposed two related replay problems: session refresh could discard the buffered event, and a newly created child `ChatWidget` did not know the expected MCP server set, which could leave its startup spinner running after every server had settled. MCP startup diagnostics should remain visible in the thread that owns the startup without affecting other transcripts. The protocol also needs to support a future app-scoped MCP lifecycle where startup is not owned by any thread. ## Reported Behavior The [originating Slack report](https://openai.slack.com/archives/C08JZTV654K/p1780604538859939) called out that using subagents could turn MCP startup failures into a wall of yellow CLI warnings because repeated failures were not deduplicated. The intended behavior is for those diagnostics to remain visible once in the thread that owns the startup, without polluting the parent transcript. ## What Changed - add nullable `threadId` ownership to `mcpServer/startupStatus/updated` - populate it from the app-server conversation ID for the current thread-scoped lifecycle and regenerate the protocol schema and TypeScript artifacts - treat a missing or null `threadId` as app-scoped without injecting it into the active chat transcript - route and buffer thread-owned MCP startup notifications by thread in the TUI - preserve buffered MCP startup events across child session refresh - seed expected MCP servers before replaying a thread snapshot so startup reaches its terminal state - suppress an identical repeated failure warning for the same server within one startup round The owning thread still renders the detailed failure and final `MCP startup incomplete (...)` summary. ## How to Test 1. Configure an optional MCP server named `smoke` that exits during initialization. 2. Launch the TUI with multi-agent support enabled. 3. Confirm the main thread's own startup failure renders one detailed `smoke` warning and one incomplete-startup summary. 4. Spawn exactly one subagent. 5. Confirm the parent transcript does not receive the subagent's MCP startup failure. 6. Switch to the subagent thread and confirm it contains exactly one detailed `smoke` failure and one incomplete-startup summary. 7. Confirm the subagent's MCP startup spinner disappears and the thread remains usable. 8. Switch between the parent and subagent and confirm the warnings neither move nor duplicate. Targeted tests: - `just test -p codex-app-server-protocol` - `just test -p codex-app-server thread_start_emits_mcp_server_status_updated_notifications` - `just test -p codex-tui mcp_startup` The parent/child behavior and spinner completion were also exercised manually in tmux. `just argument-comment-lint` was attempted but blocked by an unrelated local Bazel LLVM empty-glob failure; touched Rust callsites were inspected manually.
This commit is contained in:
1 parent
e648ec771f
commit
5a440c03f2
18 files changed
+363
-21
No files matched your search
@@ -35,6 +35,7 @@ pub(super) fn server_request_thread_id(request: &ServerRequest) -> Option<Thread
|
||||
pub(super) enum ServerNotificationThreadTarget {
|
||||
Thread(ThreadId),
|
||||
InvalidThreadId(String),
|
||||
AppScoped,
|
||||
Global,
|
||||
}
|
||||
|
||||
@@ -147,8 +148,13 @@ pub(super) fn server_notification_thread_target(
|
||||
}
|
||||
ServerNotification::Warning(notification) => notification.thread_id.as_deref(),
|
||||
ServerNotification::GuardianWarning(notification) => Some(notification.thread_id.as_str()),
|
||||
ServerNotification::McpServerStatusUpdated(notification) => {
|
||||
match notification.thread_id.as_deref() {
|
||||
Some(thread_id) => Some(thread_id),
|
||||
None => return ServerNotificationThreadTarget::AppScoped,
|
||||
}
|
||||
}
|
||||
ServerNotification::SkillsChanged(_)
|
||||
| ServerNotification::McpServerStatusUpdated(_)
|
||||
| ServerNotification::McpServerOauthLoginCompleted(_)
|
||||
| ServerNotification::AccountUpdated(_)
|
||||
| ServerNotification::AccountRateLimitsUpdated(_)
|
||||
@@ -184,6 +190,8 @@ mod tests {
|
||||
use crate::test_support::PathBufExt;
|
||||
use crate::test_support::test_path_buf;
|
||||
use codex_app_server_protocol::GuardianWarningNotification;
|
||||
use codex_app_server_protocol::McpServerStartupState;
|
||||
use codex_app_server_protocol::McpServerStatusUpdatedNotification;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ThreadSettings;
|
||||
use codex_app_server_protocol::ThreadSettingsUpdatedNotification;
|
||||
@@ -259,6 +267,37 @@ mod tests {
|
||||
assert_eq!(target, ServerNotificationThreadTarget::Thread(thread_id));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_startup_notifications_route_to_threads() {
|
||||
let thread_id = ThreadId::new();
|
||||
let notification =
|
||||
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
|
||||
thread_id: Some(thread_id.to_string()),
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
});
|
||||
|
||||
let target = server_notification_thread_target(¬ification);
|
||||
|
||||
assert_eq!(target, ServerNotificationThreadTarget::Thread(thread_id));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_startup_notifications_without_threads_are_app_scoped() {
|
||||
let notification =
|
||||
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
|
||||
thread_id: None,
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
});
|
||||
|
||||
let target = server_notification_thread_target(¬ification);
|
||||
|
||||
assert_eq!(target, ServerNotificationThreadTarget::AppScoped);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn thread_settings_updated_notifications_route_to_threads() {
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
@@ -15,10 +15,9 @@ use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
|
||||
impl App {
|
||||
fn refresh_mcp_startup_expected_servers_from_config(&mut self) {
|
||||
pub(super) fn refresh_mcp_startup_expected_servers_from_config(&mut self) {
|
||||
let enabled_config_mcp_servers: Vec<String> = self
|
||||
.chat_widget
|
||||
.config_ref()
|
||||
.config
|
||||
.mcp_servers
|
||||
.get()
|
||||
.iter()
|
||||
@@ -141,6 +140,12 @@ impl App {
|
||||
);
|
||||
return;
|
||||
}
|
||||
ServerNotificationThreadTarget::AppScoped => {
|
||||
tracing::debug!(
|
||||
"ignoring app-scoped MCP startup notification without a TUI app-level target"
|
||||
);
|
||||
return;
|
||||
}
|
||||
ServerNotificationThreadTarget::Global => {}
|
||||
}
|
||||
|
||||
|
||||
@@ -3353,24 +3353,33 @@ async fn side_thread_snapshot_skips_session_header_preamble() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn side_thread_ignores_global_mcp_startup_notifications() {
|
||||
async fn primary_thread_ignores_child_mcp_startup_notifications() {
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
while app_event_rx.try_recv().is_ok() {}
|
||||
let sentry_config = toml::from_str::<toml::Value>("command = 'true'")
|
||||
.expect("test MCP config should parse")
|
||||
.try_into()
|
||||
.expect("test MCP config should deserialize");
|
||||
app.config
|
||||
.mcp_servers
|
||||
.set(std::collections::HashMap::from([(
|
||||
"sentry".to_string(),
|
||||
sentry_config,
|
||||
)]))
|
||||
.expect("test MCP servers should accept any configuration");
|
||||
let app_server = crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref())
|
||||
.await
|
||||
.expect("embedded app server");
|
||||
let parent_thread_id = ThreadId::new();
|
||||
let side_thread_id = ThreadId::new();
|
||||
let child_thread_id = ThreadId::new();
|
||||
app.primary_thread_id = Some(parent_thread_id);
|
||||
app.active_thread_id = Some(side_thread_id);
|
||||
app.side_threads
|
||||
.insert(side_thread_id, SideThreadState::new(parent_thread_id));
|
||||
app.sync_side_thread_ui();
|
||||
app.active_thread_id = Some(parent_thread_id);
|
||||
|
||||
app.handle_app_server_event(
|
||||
&app_server,
|
||||
codex_app_server_client::AppServerEvent::ServerNotification(
|
||||
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
|
||||
thread_id: Some(child_thread_id.to_string()),
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
@@ -3380,6 +3389,168 @@ async fn side_thread_ignores_global_mcp_startup_notifications() {
|
||||
.await;
|
||||
|
||||
assert!(app_event_rx.try_recv().is_err());
|
||||
let mut child_snapshot = app
|
||||
.thread_event_channels
|
||||
.get(&child_thread_id)
|
||||
.expect("child thread channel should be created")
|
||||
.store
|
||||
.lock()
|
||||
.await
|
||||
.snapshot();
|
||||
assert!(
|
||||
matches!(
|
||||
child_snapshot.events.as_slice(),
|
||||
[ThreadBufferedEvent::Notification(
|
||||
ServerNotification::McpServerStatusUpdated(_)
|
||||
)]
|
||||
),
|
||||
"child MCP startup notification should be buffered for the child thread"
|
||||
);
|
||||
|
||||
app.apply_refreshed_snapshot_thread(
|
||||
child_thread_id,
|
||||
AppServerStartedThread {
|
||||
session: test_thread_session(child_thread_id, test_path_buf("/tmp/child")),
|
||||
turns: Vec::new(),
|
||||
},
|
||||
&mut child_snapshot,
|
||||
)
|
||||
.await;
|
||||
app.replay_thread_snapshot(child_snapshot, /*resume_restored_queue*/ false);
|
||||
|
||||
let mut rendered_cells = Vec::new();
|
||||
while let Ok(event) = app_event_rx.try_recv() {
|
||||
if let AppEvent::InsertHistoryCell(cell) = event {
|
||||
rendered_cells.push(lines_to_single_string(&cell.display_lines(/*width*/ 120)));
|
||||
}
|
||||
}
|
||||
let rendered = rendered_cells.join("\n");
|
||||
assert_eq!(app.chat_widget.thread_id(), Some(child_thread_id));
|
||||
assert_eq!(rendered.matches("sentry is not logged in").count(), 1);
|
||||
assert_eq!(
|
||||
rendered
|
||||
.matches("MCP startup incomplete (failed: sentry)")
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_scoped_mcp_startup_notifications_do_not_render_in_active_thread() {
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
while app_event_rx.try_recv().is_ok() {}
|
||||
let app_server = crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref())
|
||||
.await
|
||||
.expect("embedded app server");
|
||||
let thread_id = ThreadId::new();
|
||||
app.primary_thread_id = Some(thread_id);
|
||||
app.active_thread_id = Some(thread_id);
|
||||
|
||||
app.handle_app_server_event(
|
||||
&app_server,
|
||||
codex_app_server_client::AppServerEvent::ServerNotification(
|
||||
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
|
||||
thread_id: None,
|
||||
name: "sentry".to_string(),
|
||||
status: McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
}),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(app_event_rx.try_recv().is_err());
|
||||
assert_eq!(
|
||||
app.chat_widget.active_cell_transcript_lines(/*width*/ 120),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn active_side_thread_renders_live_mcp_startup_notifications() {
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
while app_event_rx.try_recv().is_ok() {}
|
||||
let sentry_config = toml::from_str::<toml::Value>("command = 'true'")
|
||||
.expect("test MCP config should parse")
|
||||
.try_into()
|
||||
.expect("test MCP config should deserialize");
|
||||
app.config
|
||||
.mcp_servers
|
||||
.set(std::collections::HashMap::from([(
|
||||
"sentry".to_string(),
|
||||
sentry_config,
|
||||
)]))
|
||||
.expect("test MCP servers should accept any configuration");
|
||||
let app_server = crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref())
|
||||
.await
|
||||
.expect("embedded app server");
|
||||
let parent_thread_id = ThreadId::new();
|
||||
let side_thread_id = ThreadId::new();
|
||||
app.primary_thread_id = Some(parent_thread_id);
|
||||
app.side_threads
|
||||
.insert(side_thread_id, SideThreadState::new(parent_thread_id));
|
||||
app.ensure_thread_channel(side_thread_id);
|
||||
app.activate_thread_channel(side_thread_id).await;
|
||||
app.replay_thread_snapshot(
|
||||
ThreadEventSnapshot {
|
||||
session: Some(test_thread_session(
|
||||
side_thread_id,
|
||||
test_path_buf("/tmp/side"),
|
||||
)),
|
||||
turns: Vec::new(),
|
||||
events: Vec::new(),
|
||||
input_state: None,
|
||||
},
|
||||
/*resume_restored_queue*/ false,
|
||||
);
|
||||
app.sync_side_thread_ui();
|
||||
|
||||
for status in [
|
||||
McpServerStartupState::Starting,
|
||||
McpServerStartupState::Failed,
|
||||
] {
|
||||
app.handle_app_server_event(
|
||||
&app_server,
|
||||
codex_app_server_client::AppServerEvent::ServerNotification(
|
||||
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
|
||||
thread_id: Some(side_thread_id.to_string()),
|
||||
name: "sentry".to_string(),
|
||||
status,
|
||||
error: matches!(status, McpServerStartupState::Failed)
|
||||
.then(|| "sentry is not logged in".to_string()),
|
||||
}),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
let mut active_thread_events = Vec::new();
|
||||
let active_thread_rx = app
|
||||
.active_thread_rx
|
||||
.as_mut()
|
||||
.expect("side thread receiver should be active");
|
||||
while let Ok(event) = active_thread_rx.try_recv() {
|
||||
active_thread_events.push(event);
|
||||
}
|
||||
for event in active_thread_events {
|
||||
app.handle_thread_event_now(event);
|
||||
}
|
||||
|
||||
let mut rendered_cells = Vec::new();
|
||||
while let Ok(event) = app_event_rx.try_recv() {
|
||||
if let AppEvent::InsertHistoryCell(cell) = event {
|
||||
rendered_cells.push(lines_to_single_string(&cell.display_lines(/*width*/ 120)));
|
||||
}
|
||||
}
|
||||
let rendered = rendered_cells.join("\n");
|
||||
assert!(app.chat_widget.side_conversation_active());
|
||||
assert_eq!(rendered.matches("sentry is not logged in").count(), 1);
|
||||
assert_eq!(
|
||||
rendered
|
||||
.matches("MCP startup incomplete (failed: sentry)")
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -50,6 +50,7 @@ impl ThreadEventStore {
|
||||
ThreadBufferedEvent::Request(_)
|
||||
| ThreadBufferedEvent::Notification(ServerNotification::HookStarted(_))
|
||||
| ThreadBufferedEvent::Notification(ServerNotification::HookCompleted(_))
|
||||
| ThreadBufferedEvent::Notification(ServerNotification::McpServerStatusUpdated(_))
|
||||
| ThreadBufferedEvent::FeedbackSubmission(_)
|
||||
)
|
||||
}
|
||||
@@ -590,4 +591,31 @@ mod tests {
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn thread_event_store_rebase_preserves_mcp_startup_notifications() {
|
||||
let thread_id = ThreadId::new();
|
||||
let notification = ServerNotification::McpServerStatusUpdated(
|
||||
codex_app_server_protocol::McpServerStatusUpdatedNotification {
|
||||
thread_id: Some(thread_id.to_string()),
|
||||
name: "sentry".to_string(),
|
||||
status: codex_app_server_protocol::McpServerStartupState::Failed,
|
||||
error: Some("sentry is not logged in".to_string()),
|
||||
},
|
||||
);
|
||||
let mut store = ThreadEventStore::new(/*capacity*/ 8);
|
||||
store.push_notification(notification.clone());
|
||||
|
||||
store.rebase_buffer_after_session_refresh();
|
||||
|
||||
let snapshot = store.snapshot();
|
||||
let actual = match snapshot.events.as_slice() {
|
||||
[ThreadBufferedEvent::Notification(actual)] => actual,
|
||||
other => panic!("expected one buffered MCP notification, saw: {other:?}"),
|
||||
};
|
||||
assert_eq!(
|
||||
serde_json::to_value(actual).expect("MCP notification should serialize"),
|
||||
serde_json::to_value(notification).expect("MCP notification should serialize"),
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -1261,6 +1261,7 @@ impl App {
|
||||
snapshot: ThreadEventSnapshot,
|
||||
resume_restored_queue: bool,
|
||||
) {
|
||||
self.refresh_mcp_startup_expected_servers_from_config();
|
||||
let should_buffer_replay = self.terminal_resize_reflow_enabled()
|
||||
&& (!snapshot.turns.is_empty() || !snapshot.events.is_empty());
|
||||
if should_buffer_replay {
|
||||
|
||||
Reference in new issue
Block a user