core/tui: non-blocking MCP startup #6334
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec/src/lib.rs
Lines 332 to 336 in d54213e
Queue CLI prompt until MCP startup completes
Session initialization now just spawns spawn_startup_job (see codex-rs/core/src/codex.rs lines 548‑617) so the McpConnectionManager remains empty until the background job finishes. The codex-exec CLI (and any other non‑TUI client that uses ConversationManager) still calls conversation.submit(Op::UserTurn { … }) immediately after SessionConfigured is received (lines 332‑336 below) and never waits for the new McpStartupUpdate/McpStartupComplete events. When that turn reaches run_turn (codex-rs/core/src/codex.rs line 1871) it reads mcp_connection_manager.list_all_tools().await, which is still empty, so the router exposes zero MCP tools. Because CLI invocations usually consist of a single turn, repo_map, file_context, etc. simply never run anymore. We now need to block or queue the first CLI submission until the MCP startup job finishes (or wait for McpStartupComplete) to avoid losing all MCP functionality outside the TUI.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: pakrym-oai <[email protected]>
| if has_active_turn { | ||
| self.abort_all_tasks(TurnAbortReason::Interrupted).await; | ||
| } else { | ||
| self.cancel_mcp_startup().await; |
There was a problem hiding this comment.
why do we need to cancel mcp startup on control c? Do we ever cancel it today?
There was a problem hiding this comment.
if an mcp takes a long time to start, it should be possible to interrupt it to let messages send.
There was a problem hiding this comment.
This muddles interrupt op semantics. Interrup today is specific to a running a running turn. Now it can interrupt other things that happen in the background.
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
Feels like cancellation and the startup in general can be greatly simplified in mcp_connection_manager.
Would be nice to push more logic around startup tracking from codex into mcp_connection_manager
|
@pakrym-oai i've refactored this significantly, PTAL |
|
@codex review |
This makes MCP startup not block TUI startup. Messages sent while MCPs are booting will be queued.
Screen.Recording.2025-11-06.at.11.53.24.AM.mov
Fixes #6317