openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address PR review feedback (#16041)

Browse Source
This commit is contained in:
Eric Traut
2026-03-28 00:49:47 -06:00
parent 5fe64361d0
commit ab305249d8
3 changed files with 121 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui_app_server/src/app/app_server_adapter.rs
+12 -26
View File
@@ -119,6 +119,7 @@ impl App {
skipped,
"app-server event consumer lagged; dropping ignored events"
);
self.chat_widget.finish_mcp_startup_after_lag();
}
AppServerEvent::ServerNotification(notification) => {
self.handle_server_notification_event(app_server_client, notification)
@@ -138,7 +139,7 @@ impl App {
async fn handle_server_notification_event(
&mut self,
app_server_client: &AppServerSession,
_app_server_client: &AppServerSession,
notification: ServerNotification,
) {
match &notification {
@@ -148,31 +149,16 @@ impl App {
}
ServerNotification::McpServerStatusUpdated(_) => {
if self.chat_widget.needs_mcp_startup_expected_servers() {
match super::fetch_all_mcp_server_statuses(app_server_client.request_handle())
.await
{
Ok(statuses) => {
let disabled_config_mcp_servers: std::collections::HashSet<String> =
self.chat_widget
.config_ref()
.mcp_servers
.get()
.iter()
.filter_map(|(name, server)| {
(!server.enabled).then_some(name.clone())
})
.collect();
self.chat_widget.set_mcp_startup_expected_servers(
statuses.into_iter().filter_map(|status| {
(!disabled_config_mcp_servers.contains(&status.name))
.then_some(status.name)
}),
);
}
Err(err) => {
tracing::warn!(%err, "failed to seed expected MCP startup servers");
}
}
let enabled_config_mcp_servers: Vec<String> = self
.chat_widget
.config_ref()
.mcp_servers
.get()
.iter()
.filter_map(|(name, server)| server.enabled.then_some(name.clone()))
.collect();
self.chat_widget
.set_mcp_startup_expected_servers(enabled_config_mcp_servers);
}
}
ServerNotification::AccountRateLimitsUpdated(notification) => {
codex-rs/tui_app_server/src/chatwidget.rs
+50 -2
View File
@@ -797,8 +797,10 @@ pub(crate) struct ChatWidget {
/// bottom pane is treated as "running" while this is populated, even if no agent turn is
/// currently executing.
mcp_startup_status: Option<HashMap<String, McpStartupStatus>>,
/// Expected MCP servers for the current startup round, seeded from the app-server runtime.
/// Expected MCP servers for the current startup round, seeded from enabled local config.
mcp_startup_expected_servers: Option<HashSet<String>>,
/// MCP startup is a one-time app boot sequence; ignore straggling status updates after settle.
mcp_startup_complete: bool,
connectors_cache: ConnectorsCacheState,
connectors_partial_snapshot: Option<ConnectorsSnapshot>,
connectors_prefetch_in_flight: bool,
@@ -2790,6 +2792,9 @@ impl ChatWidget {
status: McpStartupStatus,
complete_when_settled: bool,
) {
if self.mcp_startup_complete {
return;
}
let mut startup_status = self.mcp_startup_status.take().unwrap_or_default();
if let McpStartupStatus::Failed { error } = &status {
self.on_warning(error);
@@ -2862,7 +2867,7 @@ impl ChatWidget {
}
pub(crate) fn needs_mcp_startup_expected_servers(&self) -> bool {
self.mcp_startup_expected_servers.is_none()
!self.mcp_startup_complete && self.mcp_startup_expected_servers.is_none()
}
pub(crate) fn set_mcp_startup_expected_servers<I>(&mut self, server_names: I)
@@ -2894,11 +2899,53 @@ impl ChatWidget {
self.mcp_startup_status = None;
self.mcp_startup_expected_servers = None;
self.mcp_startup_complete = true;
self.update_task_running_state();
self.maybe_send_next_queued_input();
self.request_redraw();
}
pub(crate) fn finish_mcp_startup_after_lag(&mut self) {
if self.mcp_startup_complete {
return;
}
let Some(current) = &self.mcp_startup_status else {
return;
};
let mut failed = Vec::new();
let mut cancelled = Vec::new();
if let Some(expected_servers) = &self.mcp_startup_expected_servers {
for name in expected_servers {
match current.get(name) {
Some(McpStartupStatus::Ready) => {}
Some(McpStartupStatus::Failed { .. }) => failed.push(name.clone()),
Some(McpStartupStatus::Cancelled | McpStartupStatus::Starting) | None => {
cancelled.push(name.clone());
}
}
}
} else {
for (name, state) in current {
match state {
McpStartupStatus::Ready => {}
McpStartupStatus::Failed { .. } => failed.push(name.clone()),
McpStartupStatus::Cancelled | McpStartupStatus::Starting => {
cancelled.push(name.clone());
}
}
}
}
failed.sort();
failed.dedup();
cancelled.sort();
cancelled.dedup();
self.finish_mcp_startup(failed, cancelled);
}
#[cfg(test)]
fn on_mcp_startup_complete(&mut self, ev: McpStartupCompleteEvent) {
let failed = ev.failed.into_iter().map(|f| f.server).collect();
@@ -4626,6 +4673,7 @@ impl ChatWidget {
mcp_startup_status: None,
pending_turn_copyable_output: None,
mcp_startup_expected_servers: None,
mcp_startup_complete: false,
connectors_cache: ConnectorsCacheState::default(),
connectors_partial_snapshot: None,
connectors_prefetch_in_flight: false,
codex-rs/tui_app_server/src/chatwidget/tests.rs
+59
View File
@@ -2074,6 +2074,7 @@ async fn make_chatwidget_manual(
agent_turn_running: false,
mcp_startup_status: None,
mcp_startup_expected_servers: None,
mcp_startup_complete: false,
connectors_cache: ConnectorsCacheState::default(),
connectors_partial_snapshot: None,
plugin_install_apps_needing_auth: Vec::new(),
@@ -11687,6 +11688,64 @@ async fn app_server_mcp_startup_failure_renders_warning_history() {
);
}
#[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(None).await;
chat.show_welcome_banner = false;
chat.set_mcp_startup_expected_servers(["alpha".to_string(), "beta".to_string()]);
chat.handle_server_notification(
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
name: "alpha".to_string(),
status: McpServerStartupState::Starting,
error: None,
}),
None,
);
chat.handle_server_notification(
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
name: "alpha".to_string(),
status: McpServerStartupState::Failed,
error: Some("MCP client for `alpha` failed to start: handshake failed".to_string()),
}),
None,
);
chat.handle_server_notification(
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
name: "beta".to_string(),
status: McpServerStartupState::Starting,
error: None,
}),
None,
);
let _ = drain_insert_history(&mut rx);
assert!(chat.bottom_pane.is_task_running());
chat.finish_mcp_startup_after_lag();
let summary_text = drain_insert_history(&mut rx)
.iter()
.map(|lines| lines_to_single_string(lines))
.collect::<String>();
assert!(summary_text.contains("MCP startup interrupted"));
assert!(summary_text.contains("beta"));
assert!(summary_text.contains("MCP startup incomplete (failed: alpha)"));
assert!(!chat.bottom_pane.is_task_running());
chat.handle_server_notification(
ServerNotification::McpServerStatusUpdated(McpServerStatusUpdatedNotification {
name: "beta".to_string(),
status: McpServerStartupState::Ready,
error: None,
}),
None,
);
assert!(drain_insert_history(&mut rx).is_empty());
assert!(!chat.bottom_pane.is_task_running());
}
#[tokio::test]
async fn background_event_updates_status_header() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;