fix: restore MCP startup progress messages in TUI (fixes #7827) #7828
Conversation
The blocking call to notify_sandbox_state_change() in Session::new() prevents the TUI event loop from starting, causing McpStartupUpdateEvent messages to be emitted but never consumed/displayed. This makes the app appear to hang during startup with no "Booting MCP server: {name}" message.
This fix moves sandbox state notification into each MCP server's background init task, sending the notification immediately after the server becomes Ready. This avoids blocking Session::new() while ensuring each server receives its sandbox state before handling any tool calls.
Changes:
- Add ManagedClient::notify_sandbox_state() method
- Pass sandbox_state to McpConnectionManager::initialize()
- Send notification in background task after Ready status
- Remove blocking notify_sandbox_state_change() methods
Additional:
- Add chatwidget snapshot test for "Booting MCP server" status line
Regression cause: bisected to PR openai#7112 (openai#7112)
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
@ivanmurashko thanks for the thoughtful fix! Just a few requests.
| .map(|tool| (tool.server_name.clone(), tool.tool_name.clone())) | ||
| } | ||
|
|
||
| pub async fn notify_sandbox_state_change(&self, sandbox_state: &SandboxState) -> Result<()> { |
There was a problem hiding this comment.
While this currently has only one call site, my intention was to update the event loop in codex.rs so that if the sandbox_policy for the TurnContext changes between turns, then this method should also be invoked in that case.
There was a problem hiding this comment.
Addressed in commit 6ab7dc1 Session::new_turn_with_sub_id now detects sandbox_policy changes between turns and calls mcp_connection_manager.notify_sandbox_state_change to resend the sandbox state to all MCP servers.
| self.client.clone().await | ||
| } | ||
|
|
||
| async fn notify_sandbox_state_change(&self, sandbox_state: &SandboxState) -> Result<()> { |
There was a problem hiding this comment.
In order to keep the method at the end of codex-rs/core/src/mcp_connection_manager.rs, should this change to:
let managed = self.client().await?;
managed.notify_sandbox_state_change(sandbox_state).awaitThere was a problem hiding this comment.
Addressed in commit edfeabd AsyncManagedClient::notify_sandbox_state_change now awaits the managed client then calls managed.notify_sandbox_state_change(sandbox_state).await, keeping the helper at the end of mcp_connection_manager.rs as requested.
Co-authored-by: Michael Bolin <bolinfest@gmail.com>
|
@ivanmurashko, thanks for the update. Looks like there are some lint (cargo clippy) issues that need to be addressed. |
|
@etraut-openai @bolinfest I’ve addressed the comments and all checks are passing. Could you take another look at the updates? |
codex-rs/core/src/codex.rs
Outdated
| let sandbox_state = SandboxState { | ||
| sandbox_policy: session_configuration.sandbox_policy.clone(), | ||
| codex_linux_sandbox_exe: session_configuration | ||
| .original_config_do_not_use |
There was a problem hiding this comment.
I'm going to push a fix to this PR so we don't have to do another round of review.
In particular, I think the right thing is to compute per_turn_config first and then derive this sandbox_state using that so we don't exercise original_config_do_not_use.
| } | ||
|
|
||
| impl ManagedClient { | ||
| async fn notify_sandbox_state(&self, sandbox_state: &SandboxState) -> Result<()> { |
There was a problem hiding this comment.
I'm going to rename to notify_sandbox_state_change and delete the other method.
use per_turn_config
unify methods
3 participants
Problem
The introduction of
notify_sandbox_state_change()in #7112 caused a regression where the blocking call inSession::new()waits for all MCP servers to fully initialize before returning. This prevents the TUI event loop from starting, resulting inMcpStartupUpdateEventmessages being emitted but never consumed or displayed. As a result, the app appears to hang during startup, and users do not see the expected "Booting MCP server: {name}" status line.Issue: #7827
Solution
This change moves sandbox state notification into each MCP server's background initialization task. The notification is sent immediately after the server transitions to the Ready state. This approach:
Session::new(), allowing the TUI event loop to start promptly.Key Changes
ManagedClient::notify_sandbox_state()method.McpConnectionManager::initialize().Regression Details
Regression was bisected to #7112, which introduced the blocking behavior.