mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Use thread config for TUI MCP inventory (#24532)
## Summary `/mcp` in the TUI should reflect the current loaded thread, including project-local MCP servers from that thread config. Before this change, `mcpServerStatus/list` only read the latest global MCP config, so the active chat could miss project-local servers. This adds optional `threadId` to `mcpServerStatus/list`. When present, app-server resolves the loaded thread and lists MCP status from the refreshed effective config for that thread; when omitted, existing global config behavior stays unchanged. The TUI now sends the active chat thread id for `/mcp` and `/mcp verbose`, carries that origin through the async inventory result, and ignores stale completions if the user has switched threads before the fetch returns. The app-server schemas were regenerated. ## Follow-up Once this app-server API change lands, the desktop app should make the same `threadId` plumbing so its MCP inventory also uses the current thread config. Fixes #23874
This commit is contained in:
committed by
GitHub
Unverified
parent
c4e53d103c
commit
0f91e869bd
@@ -1143,6 +1143,12 @@
|
||||
"integer",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"threadId": {
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
|
||||
+6
@@ -10343,6 +10343,12 @@
|
||||
"integer",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"threadId": {
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
}
|
||||
},
|
||||
"title": "ListMcpServerStatusParams",
|
||||
|
||||
+6
@@ -6872,6 +6872,12 @@
|
||||
"integer",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"threadId": {
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
}
|
||||
},
|
||||
"title": "ListMcpServerStatusParams",
|
||||
|
||||
@@ -36,6 +36,12 @@
|
||||
"integer",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"threadId": {
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
}
|
||||
},
|
||||
"title": "ListMcpServerStatusParams",
|
||||
|
||||
+1
-1
@@ -16,4 +16,4 @@ limit?: number | null,
|
||||
* Controls how much MCP inventory data to fetch for each server.
|
||||
* Defaults to `Full` when omitted.
|
||||
*/
|
||||
detail?: McpServerStatusDetail | null, };
|
||||
detail?: McpServerStatusDetail | null, threadId?: string | null, };
|
||||
|
||||
@@ -36,6 +36,8 @@ pub struct ListMcpServerStatusParams {
|
||||
/// Defaults to `Full` when omitted.
|
||||
#[ts(optional = nullable)]
|
||||
pub detail: Option<McpServerStatusDetail>,
|
||||
#[ts(optional = nullable)]
|
||||
pub thread_id: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)]
|
||||
|
||||
@@ -216,7 +216,7 @@ Example with notification opt-out:
|
||||
- `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes.
|
||||
- `tool/requestUserInput` — prompt the user with 1–3 short questions for a tool call and return their answers (experimental).
|
||||
- `config/mcpServer/reload` — reload MCP server config from disk and queue a refresh for loaded threads (applied on each thread's next active turn); returns `{}`. Use this after editing `config.toml` without restarting the server.
|
||||
- `mcpServerStatus/list` — enumerate configured MCP servers with their tools and auth status, plus resources/resource templates for `full` detail; supports cursor+limit pagination. If `detail` is omitted, the server defaults to `full`.
|
||||
- `mcpServerStatus/list` — enumerate configured MCP servers with their tools and auth status, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`.
|
||||
- `mcpServer/resource/read` — read a resource from a configured MCP server by optional `threadId`, `server`, and `uri`, returning text/blob resource `contents`. If `threadId` is omitted, the server reads from the latest MCP config directly.
|
||||
- `mcpServer/tool/call` — call a tool on a thread's configured MCP server by `threadId`, `server`, `tool`, optional `arguments`, and optional `_meta`, returning the MCP tool result.
|
||||
- `windowsSandbox/setupStart` — start Windows sandbox setup for the selected mode (`elevated` or `unelevated`); accepts an optional absolute `cwd` to target setup for a specific workspace, returns `{ started: true }` immediately, and later emits `windowsSandbox/setupCompleted`.
|
||||
|
||||
@@ -199,15 +199,25 @@ impl McpRequestProcessor {
|
||||
let request = request_id.clone();
|
||||
|
||||
let outgoing = Arc::clone(&self.outgoing);
|
||||
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||
let config = match params.thread_id.as_deref() {
|
||||
Some(thread_id) => {
|
||||
let (_, thread) = self.load_thread(thread_id).await?;
|
||||
let thread_config = thread.config().await;
|
||||
self.config_manager
|
||||
.load_latest_config_for_thread(thread_config.as_ref())
|
||||
.await
|
||||
.map_err(|err| internal_error(format!("failed to reload config: {err}")))?
|
||||
}
|
||||
None => self.load_latest_config(/*fallback_cwd*/ None).await?,
|
||||
};
|
||||
let mcp_config = config
|
||||
.to_mcp_config(self.thread_manager.plugins_manager().as_ref())
|
||||
.await;
|
||||
let auth = self.auth_manager.auth().await;
|
||||
let environment_manager = self.thread_manager.environment_manager();
|
||||
// This threadless status path has no turn cwd or turn-selected
|
||||
// environment. Use config cwd only as the local stdio fallback; named
|
||||
// environment stdio MCPs must declare their own absolute cwd.
|
||||
// This status path has no turn-selected environment. Use config cwd
|
||||
// as the local stdio fallback; named environment stdio MCPs must
|
||||
// declare their own absolute cwd.
|
||||
let runtime_context =
|
||||
McpRuntimeContext::new(Arc::clone(&environment_manager), config.cwd.to_path_buf());
|
||||
|
||||
|
||||
@@ -14,6 +14,10 @@ use codex_app_server_protocol::ListMcpServerStatusParams;
|
||||
use codex_app_server_protocol::ListMcpServerStatusResponse;
|
||||
use codex_app_server_protocol::McpServerStatusDetail;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_core::config::set_project_trust_level;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use pretty_assertions::assert_eq;
|
||||
use rmcp::handler::server::ServerHandler;
|
||||
use rmcp::model::JsonObject;
|
||||
@@ -70,6 +74,7 @@ url = "{mcp_server_url}/mcp"
|
||||
cursor: None,
|
||||
limit: None,
|
||||
detail: None,
|
||||
thread_id: None,
|
||||
})
|
||||
.await?;
|
||||
let response = timeout(
|
||||
@@ -101,6 +106,98 @@ url = "{mcp_server_url}/mcp"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> {
|
||||
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
||||
let (mcp_server_url, mcp_server_handle) = start_mcp_server("project_lookup").await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
let workspace = TempDir::new()?;
|
||||
write_mock_responses_config_toml(
|
||||
codex_home.path(),
|
||||
&server.uri(),
|
||||
&BTreeMap::new(),
|
||||
/*auto_compact_limit*/ 1024,
|
||||
/*requires_openai_auth*/ None,
|
||||
"mock_provider",
|
||||
"compact",
|
||||
)?;
|
||||
std::fs::create_dir_all(workspace.path().join(".git"))?;
|
||||
set_project_trust_level(codex_home.path(), workspace.path(), TrustLevel::Trusted)?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let thread_start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
cwd: Some(workspace.path().to_string_lossy().into_owned()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let thread_start_response = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response(thread_start_response)?;
|
||||
|
||||
let project_config_dir = workspace.path().join(".codex");
|
||||
std::fs::create_dir_all(&project_config_dir)?;
|
||||
std::fs::write(
|
||||
project_config_dir.join("config.toml"),
|
||||
format!(
|
||||
r#"
|
||||
[mcp_servers.project-server]
|
||||
url = "{mcp_server_url}/mcp"
|
||||
"#
|
||||
),
|
||||
)?;
|
||||
|
||||
let threadless_request_id = mcp
|
||||
.send_list_mcp_server_status_request(ListMcpServerStatusParams {
|
||||
cursor: None,
|
||||
limit: None,
|
||||
detail: Some(McpServerStatusDetail::ToolsAndAuthOnly),
|
||||
thread_id: None,
|
||||
})
|
||||
.await?;
|
||||
let threadless_response = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(threadless_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let threadless_response: ListMcpServerStatusResponse = to_response(threadless_response)?;
|
||||
assert_eq!(threadless_response.data, Vec::new());
|
||||
|
||||
let thread_request_id = mcp
|
||||
.send_list_mcp_server_status_request(ListMcpServerStatusParams {
|
||||
cursor: None,
|
||||
limit: None,
|
||||
detail: Some(McpServerStatusDetail::ToolsAndAuthOnly),
|
||||
thread_id: Some(thread.id),
|
||||
})
|
||||
.await?;
|
||||
let thread_response = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(thread_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let thread_response: ListMcpServerStatusResponse = to_response(thread_response)?;
|
||||
|
||||
assert_eq!(thread_response.next_cursor, None);
|
||||
assert_eq!(thread_response.data.len(), 1);
|
||||
let status = &thread_response.data[0];
|
||||
assert_eq!(status.name, "project-server");
|
||||
assert_eq!(
|
||||
status.tools.keys().cloned().collect::<BTreeSet<_>>(),
|
||||
BTreeSet::from(["project_lookup".to_string()])
|
||||
);
|
||||
|
||||
mcp_server_handle.abort();
|
||||
let _ = mcp_server_handle.await;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct McpStatusServer {
|
||||
tool_name: Arc<String>,
|
||||
@@ -241,6 +338,7 @@ url = "{mcp_server_url}/mcp"
|
||||
cursor: None,
|
||||
limit: None,
|
||||
detail: Some(McpServerStatusDetail::ToolsAndAuthOnly),
|
||||
thread_id: None,
|
||||
})
|
||||
.await?;
|
||||
let response = timeout(
|
||||
@@ -305,6 +403,7 @@ url = "{underscore_server_url}/mcp"
|
||||
cursor: None,
|
||||
limit: None,
|
||||
detail: None,
|
||||
thread_id: None,
|
||||
})
|
||||
.await?;
|
||||
let response = timeout(
|
||||
|
||||
@@ -28,17 +28,33 @@ impl App {
|
||||
&mut self,
|
||||
app_server: &AppServerSession,
|
||||
detail: McpServerStatusDetail,
|
||||
thread_id: Option<ThreadId>,
|
||||
) {
|
||||
let request_handle = app_server.request_handle();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
let request_thread_id = self.mcp_inventory_request_thread_id(thread_id);
|
||||
tokio::spawn(async move {
|
||||
let result = fetch_all_mcp_server_statuses(request_handle, detail)
|
||||
let result = fetch_all_mcp_server_statuses(request_handle, detail, request_thread_id)
|
||||
.await
|
||||
.map_err(|err| err.to_string());
|
||||
app_event_tx.send(AppEvent::McpInventoryLoaded { result, detail });
|
||||
app_event_tx.send(AppEvent::McpInventoryLoaded {
|
||||
result,
|
||||
detail,
|
||||
thread_id,
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
fn mcp_inventory_request_thread_id(&self, thread_id: Option<ThreadId>) -> Option<ThreadId> {
|
||||
thread_id.filter(|thread_id| {
|
||||
self.active_thread_id == Some(*thread_id)
|
||||
&& self
|
||||
.agent_navigation
|
||||
.get(thread_id)
|
||||
.is_none_or(|entry| !entry.is_closed)
|
||||
})
|
||||
}
|
||||
|
||||
/// Spawns a background task to fetch account rate limits and deliver the
|
||||
/// result as a `RateLimitsLoaded` event.
|
||||
///
|
||||
@@ -535,7 +551,12 @@ impl App {
|
||||
&mut self,
|
||||
result: Result<Vec<McpServerStatus>, String>,
|
||||
detail: McpServerStatusDetail,
|
||||
thread_id: Option<ThreadId>,
|
||||
) {
|
||||
if thread_id.is_some() && thread_id != self.current_displayed_thread_id() {
|
||||
return;
|
||||
}
|
||||
|
||||
self.chat_widget.clear_mcp_inventory_loading();
|
||||
self.clear_committed_mcp_inventory_loading();
|
||||
|
||||
@@ -579,9 +600,11 @@ impl App {
|
||||
pub(super) async fn fetch_all_mcp_server_statuses(
|
||||
request_handle: AppServerRequestHandle,
|
||||
detail: McpServerStatusDetail,
|
||||
thread_id: Option<ThreadId>,
|
||||
) -> Result<Vec<McpServerStatus>> {
|
||||
let mut cursor = None;
|
||||
let mut statuses = Vec::new();
|
||||
let thread_id = thread_id.map(|id| id.to_string());
|
||||
|
||||
loop {
|
||||
let request_id = RequestId::String(format!("mcp-inventory-{}", Uuid::new_v4()));
|
||||
@@ -592,6 +615,7 @@ pub(super) async fn fetch_all_mcp_server_statuses(
|
||||
cursor: cursor.clone(),
|
||||
limit: Some(100),
|
||||
detail: Some(detail),
|
||||
thread_id: thread_id.clone(),
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -963,6 +987,7 @@ pub(super) fn mcp_inventory_maps_from_statuses(statuses: Vec<McpServerStatus>) -
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::app::test_support::make_test_app;
|
||||
use codex_app_server_protocol::PluginMarketplaceEntry;
|
||||
use codex_protocol::mcp::Tool;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -1081,6 +1106,26 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mcp_inventory_omits_thread_id_for_closed_agent_thread() {
|
||||
let mut app = make_test_app().await;
|
||||
let thread_id = ThreadId::new();
|
||||
app.active_thread_id = Some(thread_id);
|
||||
app.agent_navigation.upsert(
|
||||
thread_id, /*agent_nickname*/ None, /*agent_role*/ None,
|
||||
/*is_closed*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
app.mcp_inventory_request_thread_id(Some(thread_id)),
|
||||
Some(thread_id)
|
||||
);
|
||||
|
||||
app.agent_navigation.mark_closed(thread_id);
|
||||
|
||||
assert_eq!(app.mcp_inventory_request_thread_id(Some(thread_id)), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_feedback_upload_params_includes_thread_id_and_rollout_path() {
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
@@ -678,11 +678,15 @@ impl App {
|
||||
.on_plugin_enabled_set(cwd, plugin_id, enabled, result);
|
||||
}
|
||||
}
|
||||
AppEvent::FetchMcpInventory { detail } => {
|
||||
self.fetch_mcp_inventory(app_server, detail);
|
||||
AppEvent::FetchMcpInventory { detail, thread_id } => {
|
||||
self.fetch_mcp_inventory(app_server, detail, thread_id);
|
||||
}
|
||||
AppEvent::McpInventoryLoaded { result, detail } => {
|
||||
self.handle_mcp_inventory_result(result, detail);
|
||||
AppEvent::McpInventoryLoaded {
|
||||
result,
|
||||
detail,
|
||||
thread_id,
|
||||
} => {
|
||||
self.handle_mcp_inventory_result(result, detail, thread_id);
|
||||
}
|
||||
AppEvent::SkillsListLoaded { result } => {
|
||||
self.handle_skills_list_result(
|
||||
|
||||
@@ -137,7 +137,7 @@ async fn next_thread_settings_updated(
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn handle_mcp_inventory_result_clears_committed_loading_cell() {
|
||||
async fn handle_mcp_inventory_result_respects_origin_thread() {
|
||||
let mut app = make_test_app().await;
|
||||
app.transcript_cells
|
||||
.push(Arc::new(history_cell::new_mcp_inventory_loading(
|
||||
@@ -153,9 +153,24 @@ async fn handle_mcp_inventory_result_clears_committed_loading_cell() {
|
||||
auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported,
|
||||
}]),
|
||||
McpServerStatusDetail::ToolsAndAuthOnly,
|
||||
/*thread_id*/ None,
|
||||
);
|
||||
|
||||
assert_eq!(app.transcript_cells.len(), 0);
|
||||
|
||||
app.active_thread_id = Some(ThreadId::new());
|
||||
app.transcript_cells
|
||||
.push(Arc::new(history_cell::new_mcp_inventory_loading(
|
||||
/*animations_enabled*/ false,
|
||||
)));
|
||||
|
||||
app.handle_mcp_inventory_result(
|
||||
Ok(Vec::new()),
|
||||
McpServerStatusDetail::ToolsAndAuthOnly,
|
||||
Some(ThreadId::new()),
|
||||
);
|
||||
|
||||
assert_eq!(app.transcript_cells.len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -545,12 +545,14 @@ pub(crate) enum AppEvent {
|
||||
/// Fetch MCP inventory via app-server RPCs and render it into history.
|
||||
FetchMcpInventory {
|
||||
detail: McpServerStatusDetail,
|
||||
thread_id: Option<ThreadId>,
|
||||
},
|
||||
|
||||
/// Result of fetching MCP inventory via app-server RPCs.
|
||||
McpInventoryLoaded {
|
||||
result: Result<Vec<McpServerStatus>, String>,
|
||||
detail: McpServerStatusDetail,
|
||||
thread_id: Option<ThreadId>,
|
||||
},
|
||||
|
||||
/// Result of the startup skills refresh that runs after the first frame is scheduled.
|
||||
|
||||
@@ -1468,8 +1468,10 @@ impl ChatWidget {
|
||||
)));
|
||||
self.bump_active_cell_revision();
|
||||
self.request_redraw();
|
||||
self.app_event_tx
|
||||
.send(AppEvent::FetchMcpInventory { detail });
|
||||
self.app_event_tx.send(AppEvent::FetchMcpInventory {
|
||||
detail,
|
||||
thread_id: self.thread_id(),
|
||||
});
|
||||
}
|
||||
|
||||
/// Remove the MCP loading spinner if it is still the active cell.
|
||||
|
||||
@@ -1747,6 +1747,8 @@ async fn slash_memory_drop_reports_stubbed_feature() {
|
||||
#[tokio::test]
|
||||
async fn slash_mcp_requests_inventory_via_app_server() {
|
||||
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let thread_id = ThreadId::new();
|
||||
chat.thread_id = Some(thread_id);
|
||||
|
||||
chat.dispatch_command(SlashCommand::Mcp);
|
||||
|
||||
@@ -1754,8 +1756,9 @@ async fn slash_mcp_requests_inventory_via_app_server() {
|
||||
assert_matches!(
|
||||
rx.try_recv(),
|
||||
Ok(AppEvent::FetchMcpInventory {
|
||||
detail: McpServerStatusDetail::ToolsAndAuthOnly
|
||||
})
|
||||
detail: McpServerStatusDetail::ToolsAndAuthOnly,
|
||||
thread_id: Some(actual_thread_id)
|
||||
}) if actual_thread_id == thread_id
|
||||
);
|
||||
assert!(op_rx.try_recv().is_err(), "expected no core op to be sent");
|
||||
}
|
||||
@@ -1763,6 +1766,8 @@ async fn slash_mcp_requests_inventory_via_app_server() {
|
||||
#[tokio::test]
|
||||
async fn slash_mcp_verbose_requests_full_inventory_via_app_server() {
|
||||
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let thread_id = ThreadId::new();
|
||||
chat.thread_id = Some(thread_id);
|
||||
|
||||
submit_composer_text(&mut chat, "/mcp verbose");
|
||||
|
||||
@@ -1770,8 +1775,9 @@ async fn slash_mcp_verbose_requests_full_inventory_via_app_server() {
|
||||
assert_matches!(
|
||||
rx.try_recv(),
|
||||
Ok(AppEvent::FetchMcpInventory {
|
||||
detail: McpServerStatusDetail::Full
|
||||
})
|
||||
detail: McpServerStatusDetail::Full,
|
||||
thread_id: Some(actual_thread_id)
|
||||
}) if actual_thread_id == thread_id
|
||||
);
|
||||
assert!(op_rx.try_recv().is_err(), "expected no core op to be sent");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user