mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Speed up /mcp inventory listing (#16831)
Addresses #16244 This was a performance regression introduced when we moved the TUI on top of the app server API. Problem: `/mcp` rebuilt a full MCP inventory through `mcpServerStatus/list`, including resources and resource templates that made the TUI wait on slow inventory probes. Solution: add a lightweight `detail` mode to `mcpServerStatus/list`, have `/mcp` request tools-and-auth only, and cover the fast path with app-server and TUI tests. Testing: Confirmed slow (multi-second) response prior to change and immediate response after change. I considered two options: 1. Change the existing `mcpServerStatus/list` API to accept an optional "details" parameter so callers can request only a subset of the information. 2. Add a separate `mcpServer/list` API that returns only the servers, tools, and auth but omits the resources. I chose option 1, but option 2 is also a reasonable approach.
This commit is contained in:
+10
-5
@@ -64,6 +64,7 @@ use codex_app_server_protocol::GetAccountRateLimitsResponse;
|
||||
use codex_app_server_protocol::ListMcpServerStatusParams;
|
||||
use codex_app_server_protocol::ListMcpServerStatusResponse;
|
||||
use codex_app_server_protocol::McpServerStatus;
|
||||
use codex_app_server_protocol::McpServerStatusDetail;
|
||||
use codex_app_server_protocol::PluginInstallParams;
|
||||
use codex_app_server_protocol::PluginInstallResponse;
|
||||
use codex_app_server_protocol::PluginListParams;
|
||||
@@ -1871,8 +1872,8 @@ impl App {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Spawn a background task that fetches the full MCP server inventory from the
|
||||
/// app-server via paginated RPCs, then delivers the result back through
|
||||
/// Spawn a background task that fetches MCP server status from the app-server
|
||||
/// via paginated RPCs, then delivers the result back through
|
||||
/// `AppEvent::McpInventoryLoaded`.
|
||||
///
|
||||
/// The spawned task is fire-and-forget: no `JoinHandle` is stored, so a stale
|
||||
@@ -2125,7 +2126,9 @@ impl App {
|
||||
|
||||
self.chat_widget
|
||||
.add_to_history(history_cell::new_mcp_tools_output_from_statuses(
|
||||
&config, &statuses,
|
||||
&config,
|
||||
&statuses,
|
||||
McpServerStatusDetail::ToolsAndAuthOnly,
|
||||
));
|
||||
}
|
||||
|
||||
@@ -6002,8 +6005,9 @@ impl App {
|
||||
}
|
||||
}
|
||||
|
||||
/// Collect every MCP server status from the app-server by walking the paginated
|
||||
/// `mcpServerStatus/list` RPC until no `next_cursor` is returned.
|
||||
/// Collect every MCP server status needed for `/mcp` from the app-server by
|
||||
/// walking the paginated `mcpServerStatus/list` RPC until no `next_cursor` is
|
||||
/// returned.
|
||||
///
|
||||
/// All pages are eagerly gathered into a single `Vec` so the caller can render
|
||||
/// the inventory atomically. Each page requests up to 100 entries.
|
||||
@@ -6021,6 +6025,7 @@ async fn fetch_all_mcp_server_statuses(
|
||||
params: ListMcpServerStatusParams {
|
||||
cursor: cursor.clone(),
|
||||
limit: Some(100),
|
||||
detail: Some(McpServerStatusDetail::ToolsAndAuthOnly),
|
||||
},
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -40,6 +40,7 @@ use crate::wrapping::adaptive_wrap_line;
|
||||
use crate::wrapping::adaptive_wrap_lines;
|
||||
use base64::Engine;
|
||||
use codex_app_server_protocol::McpServerStatus;
|
||||
use codex_app_server_protocol::McpServerStatusDetail;
|
||||
use codex_config::types::McpServerTransportConfig;
|
||||
use codex_core::config::Config;
|
||||
#[cfg(test)]
|
||||
@@ -1979,10 +1980,12 @@ pub(crate) fn new_mcp_tools_output(
|
||||
/// transport details such as command, URL, cwd, and environment display.
|
||||
///
|
||||
/// This mirrors the layout of [`new_mcp_tools_output`] but sources data from
|
||||
/// the paginated RPC response rather than the in-process `McpManager`.
|
||||
/// the paginated RPC response rather than the in-process `McpManager`. The
|
||||
/// `detail` flag controls whether resources and resource templates are rendered.
|
||||
pub(crate) fn new_mcp_tools_output_from_statuses(
|
||||
config: &Config,
|
||||
statuses: &[McpServerStatus],
|
||||
detail: McpServerStatusDetail,
|
||||
) -> PlainHistoryCell {
|
||||
let mut lines: Vec<Line<'static>> = vec![
|
||||
"/mcp".magenta().into(),
|
||||
@@ -2094,48 +2097,50 @@ pub(crate) fn new_mcp_tools_output_from_statuses(
|
||||
lines.push(vec![" • Tools: ".into(), names.join(", ").into()].into());
|
||||
}
|
||||
|
||||
let server_resources = status
|
||||
.map(|status| status.resources.clone())
|
||||
.unwrap_or_default();
|
||||
if server_resources.is_empty() {
|
||||
lines.push(" • Resources: (none)".into());
|
||||
} else {
|
||||
let mut spans: Vec<Span<'static>> = vec![" • Resources: ".into()];
|
||||
if matches!(detail, McpServerStatusDetail::Full) {
|
||||
let server_resources = status
|
||||
.map(|status| status.resources.clone())
|
||||
.unwrap_or_default();
|
||||
if server_resources.is_empty() {
|
||||
lines.push(" • Resources: (none)".into());
|
||||
} else {
|
||||
let mut spans: Vec<Span<'static>> = vec![" • Resources: ".into()];
|
||||
|
||||
for (idx, resource) in server_resources.iter().enumerate() {
|
||||
if idx > 0 {
|
||||
spans.push(", ".into());
|
||||
for (idx, resource) in server_resources.iter().enumerate() {
|
||||
if idx > 0 {
|
||||
spans.push(", ".into());
|
||||
}
|
||||
|
||||
let label = resource.title.as_ref().unwrap_or(&resource.name);
|
||||
spans.push(label.clone().into());
|
||||
spans.push(" ".into());
|
||||
spans.push(format!("({})", resource.uri).dim());
|
||||
}
|
||||
|
||||
let label = resource.title.as_ref().unwrap_or(&resource.name);
|
||||
spans.push(label.clone().into());
|
||||
spans.push(" ".into());
|
||||
spans.push(format!("({})", resource.uri).dim());
|
||||
lines.push(spans.into());
|
||||
}
|
||||
|
||||
lines.push(spans.into());
|
||||
}
|
||||
let server_templates = status
|
||||
.map(|status| status.resource_templates.clone())
|
||||
.unwrap_or_default();
|
||||
if server_templates.is_empty() {
|
||||
lines.push(" • Resource templates: (none)".into());
|
||||
} else {
|
||||
let mut spans: Vec<Span<'static>> = vec![" • Resource templates: ".into()];
|
||||
|
||||
let server_templates = status
|
||||
.map(|status| status.resource_templates.clone())
|
||||
.unwrap_or_default();
|
||||
if server_templates.is_empty() {
|
||||
lines.push(" • Resource templates: (none)".into());
|
||||
} else {
|
||||
let mut spans: Vec<Span<'static>> = vec![" • Resource templates: ".into()];
|
||||
for (idx, template) in server_templates.iter().enumerate() {
|
||||
if idx > 0 {
|
||||
spans.push(", ".into());
|
||||
}
|
||||
|
||||
for (idx, template) in server_templates.iter().enumerate() {
|
||||
if idx > 0 {
|
||||
spans.push(", ".into());
|
||||
let label = template.title.as_ref().unwrap_or(&template.name);
|
||||
spans.push(label.clone().into());
|
||||
spans.push(" ".into());
|
||||
spans.push(format!("({})", template.uri_template).dim());
|
||||
}
|
||||
|
||||
let label = template.title.as_ref().unwrap_or(&template.name);
|
||||
spans.push(label.clone().into());
|
||||
spans.push(" ".into());
|
||||
spans.push(format!("({})", template.uri_template).dim());
|
||||
lines.push(spans.into());
|
||||
}
|
||||
|
||||
lines.push(spans.into());
|
||||
}
|
||||
|
||||
lines.push(Line::from(""));
|
||||
@@ -3340,7 +3345,11 @@ mod tests {
|
||||
auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported,
|
||||
}];
|
||||
|
||||
let cell = new_mcp_tools_output_from_statuses(&config, &statuses);
|
||||
let cell = new_mcp_tools_output_from_statuses(
|
||||
&config,
|
||||
&statuses,
|
||||
McpServerStatusDetail::ToolsAndAuthOnly,
|
||||
);
|
||||
let rendered = render_lines(&cell.display_lines(/*width*/ 120)).join("\n");
|
||||
|
||||
insta::assert_snapshot!(rendered);
|
||||
|
||||
-2
@@ -10,5 +10,3 @@ expression: rendered
|
||||
• Auth: Unsupported
|
||||
• Command: docs-server --stdio
|
||||
• Tools: lookup
|
||||
• Resources: (none)
|
||||
• Resource templates: (none)
|
||||
|
||||
Reference in New Issue
Block a user