mirror of
https://github.com/openai/codex.git
synced 2026-11-03 08:22:30 +00:00
fix(tui): implement /mcp inventory for tui_app_server (#14931)
## Problem
The `/mcp` command did not work in the app-server TUI (remote mode). On
`main`, `add_mcp_output()` called `McpManager::effective_servers()`
in-process, which only sees locally configured servers, and then emitted
a generic stub message for the app-server to handle. In remote usage,
that left `/mcp` without a real inventory view.
## Solution
Implement `/mcp` for the app-server TUI by fetching MCP server inventory
directly from the app-server via the paginated `mcpServerStatus/list`
RPC and rendering the results into chat history.
The command now follows a three-phase lifecycle:
1. Loading: `ChatWidget::add_mcp_output()` inserts a transient
`McpInventoryLoadingCell` and emits `AppEvent::FetchMcpInventory`. This
gives immediate feedback that the command registered.
2. Fetch: `App::fetch_mcp_inventory()` spawns a background task that
calls `fetch_all_mcp_server_statuses()` over an app-server request
handle. When the RPC completes, it sends `AppEvent::McpInventoryLoaded {
result }`.
3. Resolve: `App::handle_mcp_inventory_result()` clears the loading cell
and renders either `new_mcp_tools_output_from_statuses(...)` or an error
message.
This keeps the main app event loop responsive, so the TUI can repaint
before the remote RPC finishes.
## Notes
- No `app-server` changes were required.
- The rendered inventory includes auth, tools, resources, and resource
templates, plus transport details when they are available from local
config for display enrichment.
- The app-server RPC does not expose authoritative `enabled` or
`disabled_reason` state for MCP servers, so the remote `/mcp` view no
longer renders a `Status:` row rather than guessing from local config.
- RPC failures surface in history as `Failed to load MCP inventory:
...`.
## Tests
- `slash_mcp_requests_inventory_via_app_server`
- `mcp_inventory_maps_prefix_tool_names_by_server`
- `handle_mcp_inventory_result_clears_committed_loading_cell`
- `mcp_tools_output_from_statuses_renders_status_only_servers`
- `mcp_inventory_loading_snapshot`
This commit is contained in:
1 parent
0d2ff40a58
commit
43ee72a9b9
7 files changed
+581
-9
No files matched your search
@@ -44,7 +44,13 @@ use crate::tui::TuiEvent;
|
||||
use crate::update_action::UpdateAction;
|
||||
use crate::version::CODEX_CLI_VERSION;
|
||||
use codex_ansi_escape::ansi_escape_line;
|
||||
use codex_app_server_client::AppServerRequestHandle;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_app_server_protocol::ListMcpServerStatusParams;
|
||||
use codex_app_server_protocol::ListMcpServerStatusResponse;
|
||||
use codex_app_server_protocol::McpServerStatus;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
@@ -75,6 +81,8 @@ use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::FinalOutput;
|
||||
use codex_protocol::protocol::ListSkillsResponseEvent;
|
||||
#[cfg(test)]
|
||||
use codex_protocol::protocol::McpAuthStatus;
|
||||
#[cfg(test)]
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SessionConfiguredEvent;
|
||||
@@ -111,6 +119,7 @@ use tokio::sync::mpsc::error::TrySendError;
|
||||
use tokio::sync::mpsc::unbounded_channel;
|
||||
use tokio::task::JoinHandle;
|
||||
use toml::Value as TomlValue;
|
||||
use uuid::Uuid;
|
||||
mod agent_navigation;
|
||||
mod app_server_adapter;
|
||||
mod app_server_requests;
|
||||
@@ -1536,6 +1545,72 @@ 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
|
||||
/// `AppEvent::McpInventoryLoaded`.
|
||||
///
|
||||
/// The spawned task is fire-and-forget: no `JoinHandle` is stored, so a stale
|
||||
/// result may arrive after the user has moved on. We currently accept that
|
||||
/// tradeoff because the effect is limited to stale inventory output in history,
|
||||
/// while request-token invalidation would add cross-cutting async state for a
|
||||
/// low-severity path.
|
||||
fn fetch_mcp_inventory(&mut self, app_server: &AppServerSession) {
|
||||
let request_handle = app_server.request_handle();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
tokio::spawn(async move {
|
||||
let result = fetch_all_mcp_server_statuses(request_handle)
|
||||
.await
|
||||
.map_err(|err| err.to_string());
|
||||
app_event_tx.send(AppEvent::McpInventoryLoaded { result });
|
||||
});
|
||||
}
|
||||
|
||||
/// Process the completed MCP inventory fetch: clear the loading spinner, then
|
||||
/// render either the full tool/resource listing or an error into chat history.
|
||||
///
|
||||
/// When both the local config and the app-server report zero servers, a special
|
||||
/// "empty" cell is shown instead of the full table.
|
||||
fn handle_mcp_inventory_result(&mut self, result: Result<Vec<McpServerStatus>, String>) {
|
||||
let config = self.chat_widget.config_ref().clone();
|
||||
self.chat_widget.clear_mcp_inventory_loading();
|
||||
self.clear_committed_mcp_inventory_loading();
|
||||
|
||||
let statuses = match result {
|
||||
Ok(statuses) => statuses,
|
||||
Err(err) => {
|
||||
self.chat_widget
|
||||
.add_error_message(format!("Failed to load MCP inventory: {err}"));
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
if config.mcp_servers.get().is_empty() && statuses.is_empty() {
|
||||
self.chat_widget
|
||||
.add_to_history(history_cell::empty_mcp_output());
|
||||
return;
|
||||
}
|
||||
|
||||
self.chat_widget
|
||||
.add_to_history(history_cell::new_mcp_tools_output_from_statuses(
|
||||
&config, &statuses,
|
||||
));
|
||||
}
|
||||
|
||||
fn clear_committed_mcp_inventory_loading(&mut self) {
|
||||
let Some(index) = self
|
||||
.transcript_cells
|
||||
.iter()
|
||||
.rposition(|cell| cell.as_any().is::<history_cell::McpInventoryLoadingCell>())
|
||||
else {
|
||||
return;
|
||||
};
|
||||
|
||||
self.transcript_cells.remove(index);
|
||||
if let Some(Overlay::Transcript(overlay)) = &mut self.overlay {
|
||||
overlay.replace_cells(self.transcript_cells.clone());
|
||||
}
|
||||
}
|
||||
|
||||
async fn try_submit_active_thread_op_via_app_server(
|
||||
&mut self,
|
||||
app_server: &mut AppServerSession,
|
||||
@@ -3047,6 +3122,12 @@ impl App {
|
||||
AppEvent::RefreshConnectors { force_refetch } => {
|
||||
self.chat_widget.refresh_connectors(force_refetch);
|
||||
}
|
||||
AppEvent::FetchMcpInventory => {
|
||||
self.fetch_mcp_inventory(app_server);
|
||||
}
|
||||
AppEvent::McpInventoryLoaded { result } => {
|
||||
self.handle_mcp_inventory_result(result);
|
||||
}
|
||||
AppEvent::StartFileSearch(query) => {
|
||||
self.file_search.on_user_query(query);
|
||||
}
|
||||
@@ -4469,6 +4550,80 @@ impl App {
|
||||
}
|
||||
}
|
||||
|
||||
/// Collect every MCP server status 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.
|
||||
async fn fetch_all_mcp_server_statuses(
|
||||
request_handle: AppServerRequestHandle,
|
||||
) -> Result<Vec<McpServerStatus>> {
|
||||
let mut cursor = None;
|
||||
let mut statuses = Vec::new();
|
||||
|
||||
loop {
|
||||
let request_id = RequestId::String(format!("mcp-inventory-{}", Uuid::new_v4()));
|
||||
let response: ListMcpServerStatusResponse = request_handle
|
||||
.request_typed(ClientRequest::McpServerStatusList {
|
||||
request_id,
|
||||
params: ListMcpServerStatusParams {
|
||||
cursor: cursor.clone(),
|
||||
limit: Some(100),
|
||||
},
|
||||
})
|
||||
.await
|
||||
.wrap_err("mcpServerStatus/list failed in app-server TUI")?;
|
||||
statuses.extend(response.data);
|
||||
if let Some(next_cursor) = response.next_cursor {
|
||||
cursor = Some(next_cursor);
|
||||
} else {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(statuses)
|
||||
}
|
||||
|
||||
/// Convert flat `McpServerStatus` responses into the per-server maps used by the
|
||||
/// in-process MCP subsystem (tools keyed as `mcp__{server}__{tool}`, plus
|
||||
/// per-server resource/template/auth maps). Test-only because the app-server TUI
|
||||
/// renders directly from `McpServerStatus` rather than these maps.
|
||||
#[cfg(test)]
|
||||
type McpInventoryMaps = (
|
||||
HashMap<String, codex_protocol::mcp::Tool>,
|
||||
HashMap<String, Vec<codex_protocol::mcp::Resource>>,
|
||||
HashMap<String, Vec<codex_protocol::mcp::ResourceTemplate>>,
|
||||
HashMap<String, McpAuthStatus>,
|
||||
);
|
||||
|
||||
#[cfg(test)]
|
||||
fn mcp_inventory_maps_from_statuses(statuses: Vec<McpServerStatus>) -> McpInventoryMaps {
|
||||
let mut tools = HashMap::new();
|
||||
let mut resources = HashMap::new();
|
||||
let mut resource_templates = HashMap::new();
|
||||
let mut auth_statuses = HashMap::new();
|
||||
|
||||
for status in statuses {
|
||||
let server_name = status.name;
|
||||
auth_statuses.insert(
|
||||
server_name.clone(),
|
||||
match status.auth_status {
|
||||
codex_app_server_protocol::McpAuthStatus::Unsupported => McpAuthStatus::Unsupported,
|
||||
codex_app_server_protocol::McpAuthStatus::NotLoggedIn => McpAuthStatus::NotLoggedIn,
|
||||
codex_app_server_protocol::McpAuthStatus::BearerToken => McpAuthStatus::BearerToken,
|
||||
codex_app_server_protocol::McpAuthStatus::OAuth => McpAuthStatus::OAuth,
|
||||
},
|
||||
);
|
||||
resources.insert(server_name.clone(), status.resources);
|
||||
resource_templates.insert(server_name.clone(), status.resource_templates);
|
||||
for (tool_name, tool) in status.tools {
|
||||
tools.insert(format!("mcp__{server_name}__{tool_name}"), tool);
|
||||
}
|
||||
}
|
||||
|
||||
(tools, resources, resource_templates, auth_statuses)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
@@ -4500,11 +4655,13 @@ mod tests {
|
||||
use codex_protocol::config_types::CollaborationModeMask;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
use codex_protocol::config_types::Settings;
|
||||
use codex_protocol::mcp::Tool;
|
||||
use codex_protocol::openai_models::ModelAvailabilityNux;
|
||||
use codex_protocol::protocol::AgentMessageDeltaEvent;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::McpAuthStatus;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SessionConfiguredEvent;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
@@ -4545,6 +4702,75 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_inventory_maps_prefix_tool_names_by_server() {
|
||||
let statuses = vec![
|
||||
McpServerStatus {
|
||||
name: "docs".to_string(),
|
||||
tools: HashMap::from([(
|
||||
"list".to_string(),
|
||||
Tool {
|
||||
description: None,
|
||||
name: "list".to_string(),
|
||||
title: None,
|
||||
input_schema: serde_json::json!({"type": "object"}),
|
||||
output_schema: None,
|
||||
annotations: None,
|
||||
icons: None,
|
||||
meta: None,
|
||||
},
|
||||
)]),
|
||||
resources: Vec::new(),
|
||||
resource_templates: Vec::new(),
|
||||
auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported,
|
||||
},
|
||||
McpServerStatus {
|
||||
name: "disabled".to_string(),
|
||||
tools: HashMap::new(),
|
||||
resources: Vec::new(),
|
||||
resource_templates: Vec::new(),
|
||||
auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported,
|
||||
},
|
||||
];
|
||||
|
||||
let (tools, resources, resource_templates, auth_statuses) =
|
||||
mcp_inventory_maps_from_statuses(statuses);
|
||||
let mut resource_names = resources.keys().cloned().collect::<Vec<_>>();
|
||||
resource_names.sort();
|
||||
let mut template_names = resource_templates.keys().cloned().collect::<Vec<_>>();
|
||||
template_names.sort();
|
||||
|
||||
assert_eq!(
|
||||
tools.keys().cloned().collect::<Vec<_>>(),
|
||||
vec!["mcp__docs__list".to_string()]
|
||||
);
|
||||
assert_eq!(resource_names, vec!["disabled", "docs"]);
|
||||
assert_eq!(template_names, vec!["disabled", "docs"]);
|
||||
assert_eq!(
|
||||
auth_statuses.get("disabled"),
|
||||
Some(&McpAuthStatus::Unsupported)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn handle_mcp_inventory_result_clears_committed_loading_cell() {
|
||||
let mut app = make_test_app().await;
|
||||
app.transcript_cells
|
||||
.push(Arc::new(history_cell::new_mcp_inventory_loading(
|
||||
/*animations_enabled*/ false,
|
||||
)));
|
||||
|
||||
app.handle_mcp_inventory_result(Ok(vec![McpServerStatus {
|
||||
name: "docs".to_string(),
|
||||
tools: HashMap::new(),
|
||||
resources: Vec::new(),
|
||||
resource_templates: Vec::new(),
|
||||
auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported,
|
||||
}]));
|
||||
|
||||
assert_eq!(app.transcript_cells.len(), 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn startup_waiting_gate_is_only_for_fresh_or_exit_session_selection() {
|
||||
assert_eq!(
|
||||
|
||||
Reference in new issue
Block a user