feat(tui): rescue MCP and plugin session boot - #5677
Conversation
Plugin discovery and every enabled MCP server now boot as session-owned state: the activity strip shows `MCP · N connecting` with named chips, and a compact receipt keeps per-server outcomes (connected, failed, needs login, disabled) until retry. Failures are Event::McpSessionBoot, not toast-only Status. connect_all is concurrent; optional servers do not block the first LLM call. Never tell users `/mcp auth`. Co-authored-by: Codewhale Agent <codewhale-agent@hmbown.local>
Enabled servers now appear as connecting from configured names at session start (`MCP · N connecting · alpha · beta · …`), not after the engine snapshot. Failures stay on the session receipt with `/mcp retry` / `/mcp login`; a late boot event cannot overwrite a newer `/mcp` mutation. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Co-authored-by: Codewhale Agent <codewhale-agent@hmbown.local>
`/mcp` Show now snapshots the engine-owned pool via bootstrap_mcp so the handle method and BootstrapMcp op are live production paths, not lib-only dead code. Collapse identical disabled-row action branches and name the pending-connect return type so clippy is quiet. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
connect_all must only handshake configured servers, matching the sequential loop: dynamic runtime entries stay registered and connect via get_or_connect. An empty session must not emit McpSessionBoot, which had raced ChangeMode and idle host-managed engines. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 6m 39s —— View job Review: feat(tui): rescue MCP and plugin session boot
SummaryThis is a solid, well-tested rescue of the MCP/plugin session-boot feature — Correctness bugs1. Plugin-authority-revoked errors are silently dropped once the background boot task reports in —
But the spawned task (engine.rs:6221 Net effect: a server dropped for a revoked/changed plugin authority ends up with no error and Compare with Fix direction: seed the spawned task's local 2. The live session-boot surface stops updating after the first if app.mcp_snapshot_generation > 0 {
return;
}
The doc comment says this exists so "a late spawn-time boot result cannot overwrite a newer user action," which is a reasonable one-shot race guard, but the implementation is a permanent kill-switch rather than a staleness check. Concretely: if a user has several servers still connecting and retries one already-failed server mid-boot, Fix direction: gate on a per-event/generation comparison (e.g. stamp the event with the generation at emission time and only drop if it's older than current) rather than a one-way boolean-ish latch, or scope the bump to only the actions that actually race with the background task ( Minor / lower confidence
Good choices worth noting
|
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: be4b9a69f1
ℹ️ 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".
| for (name, result) in results { | ||
| remaining.retain(|pending_name| pending_name != &name); | ||
| match result { | ||
| Ok(connection) => pool.store_ready_connection(name, connection), |
There was a problem hiding this comment.
When /mcp reload or a config-changing MCP action runs while the spawn-time handshakes are still in flight, the reload can replace the config and reconnect under a new catalog generation before this task acquires the lock. This unconditional insertion then overwrites the new same-name connection—or restores a server removed by the reload—with a transport created from the old captured config; store_ready_connection stamps it with the current generation, so later catalog checks do not reject it. Cancel/join the boot pass during reload or verify the captured generation/config before inserting each result.
Useful? React with 👍 / 👎.
| network_policy, | ||
| catalog_generation, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
When one configured server is slow but another finishes quickly, connect_pending_concurrently still waits for the entire JoinSet and returns a complete Vec, so no successful connection is inserted into the shared pool until the slowest attempt settles. A first model turn during that interval therefore sees none of the fast server's tools despite mcp_tools claiming to snapshot currently ready servers; stream completions into the pool as each task finishes instead of awaiting the whole batch first.
Useful? React with 👍 / 👎.
| catalog_generation, | ||
| ) | ||
| .await; | ||
| let mut errors = HashMap::new(); |
There was a problem hiding this comment.
When collect_pending_connects reports a plugin-authority failure and at least one other server remains pending, that diagnosis is first saved in self.mcp_connection_errors but this fresh empty map becomes the source of every progress and finished update. The first update consequently erases the authority failure, leaving the affected optional server disconnected with no error or correct recovery guidance in the settled snapshot; initialize the task's map from auth_errors or merge updates instead of replacing them.
Useful? React with 👍 / 👎.
|
|
||
| fn server_row_text(row: &McpServerBootRow, locale: Locale) -> String { | ||
| let state = match row.state { | ||
| McpServerBootState::Connecting => Cow::Borrowed("connecting"), |
There was a problem hiding this comment.
When the user selects any non-English locale, the new session boot receipt renders the literal English word connecting (and the Extensions projection uses the same hard-coded label) while surrounding states are translated. Route this user-visible lifecycle state through tr(locale, MessageId::...) so the newly added surface does not become partially English.
AGENTS.md reference: crates/tui/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
| if snapshot.servers.is_empty() && connecting.is_empty() { | ||
| return; | ||
| } | ||
| let _ = self.tx_event.try_send(Event::McpSessionBoot { |
There was a problem hiding this comment.
When the bounded engine event channel is full as boot settles—for example while the UI is draining a large turn—try_send silently drops the only finished: true transition. The app then retains its initial mcp_initializing and connecting names indefinitely, so the activity strip and receipt continue reporting already-settled servers as connecting until another MCP action or turn happens to refresh them; send the terminal state reliably or retain and retry it, while keeping only intermediate progress best-effort.
Useful? React with 👍 / 👎.
| if let Some(chip) = crate::tui::session_boot::activity_chip( | ||
| app, | ||
| available.saturating_sub(used + GROUP_GAP_WIDTH), | ||
| ) { |
There was a problem hiding this comment.
When a status toast and an MCP boot chip are visible together, the notice is fitted against the width used before this chip is added. The chip then consumes additional columns and the already-fitted notice is appended without recalculation, causing the right side of warning/error notices to be clipped even though the preceding logic promises urgent notices win the row; include the boot chip in the fit budget or suppress it when the notice needs that space.
Useful? React with 👍 / 👎.
| } else if matches!( | ||
| plugin.trust_status, | ||
| PluginTrustStatus::NeverReviewed | PluginTrustStatus::CapabilitiesChanged | ||
| ) { |
There was a problem hiding this comment.
When a previously reviewed plugin's bytes change without changing its declared capabilities, the registry assigns PluginTrustStatus::ContentChanged and LoadedPlugin::active() rejects it, but this summary does not increment either invalid or needs_setup. The new receipt therefore reports only that the plugin is loaded and gives no indication that it is inactive pending review; include ContentChanged in the setup-required state derived from the typed trust enum.
AGENTS.md reference: crates/tui/AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
| error.contains("mcp login") | ||
| || error.contains("auth required") | ||
| || (error.contains("oauth") && error.contains("authenticat")) |
There was a problem hiding this comment.
When an OAuth MCP server reports common failures such as 401 Unauthorized, authentication_required, or not logged in, this new classifier returns false even though mcp::oauth::error_text_looks_auth_required recognizes those exact shapes. The boot receipt consequently labels the server as a generic failure and recommends /mcp retry instead of /mcp login; use the existing classifier rather than maintaining this narrower mirror.
AGENTS.md reference: AGENTS.md:L11-L14
Useful? React with 👍 / 👎.
| // engine must wait for its host to claim and explicitly dispatch the | ||
| // next turn so events cannot be attached to the wrong durable record. | ||
| let host_managed_turns = self.host_managed_turns(); | ||
| self.start_mcp_session_boot().await; |
There was a problem hiding this comment.
When runtime_chat_isolated is true and MCP remains enabled in the shared feature config, this unconditional startup call still reads MCP configuration and launches configured stdio or remote servers. That bypasses the existing text-only isolation boundary in build_turn_tool_registry_and_catalog, which explicitly must not connect or snapshot MCP, and isolated runtime construction also omits the normal network-policy decider; guard session boot with the same isolation condition so account-owned Chat cannot execute unusable MCP processes.
Useful? React with 👍 / 👎.
| let workflow_panel_height = desired_workflow_panel_height.min( | ||
| auxiliary_budget | ||
| .saturating_sub(preview_height) | ||
| .saturating_sub(session_boot_height), |
There was a problem hiding this comment.
When an expanded workflow panel and the new boot receipt compete in a short terminal, session_boot_height is allocated first and then subtracted from the workflow panel's budget, so up to six rows of boot chrome can shrink or completely remove the interactive workflow content. Compact layouts are required to shed chrome before content; cap or hide the receipt after reserving the workflow panel rather than giving it higher priority.
AGENTS.md reference: crates/tui/AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 3m 22s —— View job Review: feat(tui): rescue MCP and plugin session boot (update after
|
1 participant
Summary
Rescue #5658 onto current
mainwhile preserving its four original commits, author metadata, dates, subjects, sign-offs, and co-author trailers./mcp retryand/mcp loginactions.connect_all, and avoid an empty-session boot event.This replacement supersedes stale, conflicting #5658. The stale PR should be closed only after this replacement lands.
No-Issue: current-main rescue of #5658
Verification
cargo fmt --all -- --checkgit diff --checkcargo check -p codewhale-tui --lockedaccumulate_runtime_cost_coveragebaseline lint allowed; strict-D warningsotherwise stops only at unchangedruntime_threads.rs:2562Guardrails
BASE_PROMPTchange.streamable_http.rsremains identical to current main after semantic conflict resolution.