feat: keep child MCP warnings out of parent transcript (#27174)

## Why

MCP startup status notifications are thread-owned, but `ChatWidget`
trusted upstream routing. If routing state delivered a tagged child
notification to the active parent widget, the child MCP failure could
still mutate the parent's startup state and transcript. Rejecting it
only inside the MCP handler was also too late because shared
notification handling could already restore and consume the parent's
retry status.

## What changed

- Validate a tagged MCP status notification against the visible
`ChatWidget` thread before shared notification handling mutates any
parent state.
- Cover child `Starting` and `Failed` notifications delivered to a
retrying parent widget, asserting that they preserve its visible retry
error and saved status header while producing no history or MCP status
mutation.

## User impact

Subagent MCP startup failures remain scoped to the child transcript
instead of appearing as duplicate warnings in the parent transcript.

## Testing

- `just test -p codex-tui mcp_startup_ignores_status_for_other_thread`
- `just test -p codex-tui
primary_thread_ignores_child_mcp_startup_notifications`
- `just fmt`
This commit is contained in:
jif
2026-06-10 11:45:49 +02:00
committed by GitHub
Unverified
parent a7b6baecc6
commit 0ffcefaf3d
2 changed files with 50 additions and 0 deletions
+9
View File
@@ -6,6 +6,15 @@ impl ChatWidget {
notification: ServerNotification,
replay_kind: Option<ReplayKind>,
) {
// Reject misrouted child updates before shared notification handling mutates parent state.
if let ServerNotification::McpServerStatusUpdated(notification) = &notification
&& let (Some(notification_thread_id), Some(thread_id)) =
(notification.thread_id.as_deref(), self.thread_id())
&& notification_thread_id != thread_id.to_string()
{
return;
}
let from_replay = replay_kind.is_some();
let is_resume_initial_replay =
matches!(replay_kind, Some(ReplayKind::ResumeInitialMessages));
@@ -25,6 +25,47 @@ fn notify_mcp_status_error(chat: &mut ChatWidget, name: &str, error: &str) {
);
}
#[tokio::test]
async fn mcp_startup_ignores_status_for_other_thread() {
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(["sentry".to_string()]);
let parent_thread_id = ThreadId::new();
let child_thread_id = ThreadId::new();
chat.thread_id = Some(parent_thread_id);
chat.on_stream_error(
"Connection interrupted, retrying".to_string(),
/*additional_details*/ None,
);
let status_before = chat.status_state.current_status.clone();
let retry_status_header_before = chat.status_state.retry_status_header.clone();
for status in [
McpServerStartupState::Starting,
McpServerStartupState::Failed,
] {
chat.handle_server_notification(
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
thread_id: Some(child_thread_id.to_string()),
name: "sentry".to_string(),
status,
error: matches!(status, McpServerStartupState::Failed)
.then(|| "sentry is not logged in".to_string()),
}),
/*replay_kind*/ None,
);
}
assert!(drain_insert_history(&mut rx).is_empty());
assert!(!chat.bottom_pane.is_task_running());
assert!(chat.mcp_startup_status.is_none());
assert_eq!(chat.status_state.current_status, status_before);
assert_eq!(
chat.status_state.retry_status_header,
retry_status_header_before
);
}
#[tokio::test]
async fn mcp_startup_dedupes_same_round_duplicate_failure_warning() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;