feat(tui): surface MCP and plugin boot as a session set

[Hmbown]

Summary

Hunter’s first turn sat on working · 22s · 0 steps while plugin discovery and every enabled MCP server booted invisibly. connect_all was sequential, the UI stored only mcp_configured_count, and failures were toast-only Event::status("Failed to connect MCP server 'X'"). Slack is one server in that set — this PR treats MCP + plugins as session-owned state.

• First frame names every enabled server. App init reads configured names into mcp_connecting, so the activity strip shows MCP · N connecting · alpha · beta · gamma · docs before the engine snapshot. Narrow width sheds names and keeps MCP · N connecting.
• Compact boot receipt on the session page (not only /mcp): one row per server once settled (connected | failed | needs login | disabled) with next action (/mcp retry <name>, /mcp login <name>, /mcp doctor). Failures stay on the page until retry. No toast-only UX. Never /mcp auth.
• Plugin line when the registry is not quiet: Plugins · 12 loaded · 1 invalid · 2 duplicate.
• Engine spawn starts a concurrent connect pass (semaphore of 8). Optional servers do not block the first LLM call; the catalog refreshes as they become ready (KV-cache prefix re-pin: mcp-session-boot). A late boot event cannot overwrite a newer /mcp mutation.

No-Issue: session-boot visibility surface; settings recovery remains PR #5655.

Session UI naming

State	Activity chip	Receipt
0 servers, quiet plugins	none	none
N connecting	MCP · N connecting · alpha · beta · … (names shed on a narrow row)	same named connecting line; plugin summary if not quiet
mixed settled	MCP · 3 connected · 1 failed	MCP · 3 connected plus one row per failed / needs login / disabled, capped at 6 with +N more · /mcp
one failed	MCP · 0 connected · 1 failed	alpha · failed · /mcp retry alpha
one needs login		gamma · needs login · /mcp login gamma

Tests use alpha / beta / gamma / docs, never a Slack special-case.

Testing

• cargo fmt --all -- --check (changed files formatted)
• cargo clippy --workspace --all-targets --all-features --locked (not run; focused lib tests used)
• cargo test --workspace --all-features --locked (not run)

Focused codewhale-tui --lib evidence:

• tui::session_boot::tests::* (0 / 1 / N, first-frame names, count fallback, retry/login rows, overflow +N more, plugin line)
• tui::ui::event_loop::session_boot_event_tests::*
• tui::phase_strip::tests::activity_band_names_connecting_mcp_servers
• core::engine::tests::bootstrap_and_retry_mcp_use_the_engine_owned_pool
• commands::groups::utility::mcp::tests::* (retry action)

python3 scripts/check-coauthor-trailers.py --range 018d32811..HEAD passed.

Checklist

• Updated docs or comments as needed
• Added or updated tests where relevant
• Verified TUI behavior manually if UI changes
• Harvested/co-authored credit uses a recognized agent identity (Codewhale Agent)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @Hmbown's task in 6m 9s —— View job

---

Claude is reviewing this PR

Todo list

• Read trigger comment and repo guidance (CLAUDE.md / AGENTS.md)
• Review diff: crates/tui/src/core/engine.rs (concurrent connect pass)
• Review diff: crates/tui/src/mcp.rs and mcp/streamable_http.rs
• Review diff: crates/tui/src/tui/session_boot.rs (new, 714 lines)
• Review diff: crates/tui/src/tui/phase_strip.rs, ui/frame.rs, ui/event_loop.rs, ui/handlers.rs, views/extensions.rs
• Review diff: commands/groups/utility/mcp.rs, core/engine/handle.rs, core/events.rs, core/ops.rs
• Check for provider/model prefix inference issues (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant) — none found, this PR doesn't touch routing
• Post final review summary

---

Review

Nice piece of work overall: the concurrent connect pass is a real improvement over the old N×timeout sequential loop, failures move off toast-only Status events onto the session-owned snapshot, and the 0/1/N contract in session_boot.rs is genuinely tested with alpha/beta/gamma/docs rather than a Slack special case. /mcp auth → /mcp login is fixed everywhere it's user-facing, including mcp/streamable_http.rs.

1. /mcp retry <name> clobbers sibling boot progress for still-connecting servers (High)

Engine::retry_mcp_server (crates/tui/src/core/engine.rs:6246-6280) always emits:

let _ = self.tx_event.try_send(Event::McpSessionBoot {
    snapshot: snapshot.clone(),
    connecting: Vec::new(),
    finished: true,
});

regardless of self.mcp_boot_in_flight. apply_mcp_session_boot_event (crates/tui/src/tui/ui/event_loop.rs:5757-5769) applies this unconditionally: app.mcp_connecting = connecting (now empty) and app.mcp_initializing = !finished (now false).

Reproduction that matches the PR's own described UX (retry one failed server while others are still connecting): alpha connects, beta fails fast, gamma/docs are still mid-handshake in the spawn-time background task. The receipt correctly shows beta as failed with /mcp retry beta. The user runs it. That single-server retry now wipes app.mcp_connecting and flips app.mcp_initializing to false — even though gamma/docs are still genuinely connecting.

row_from_snapshot (crates/tui/src/tui/session_boot.rs:387): let connecting_now = initializing || connecting.iter().any(...). With both now false/empty, gamma and docs immediately render as Failed with a /mcp retry gamma action — even though nothing failed and no error was ever recorded for them. This persists until the background task's next per-server completion sends a corrected Progress update, which given slow handshakes could be several seconds. Worse, if the user acts on the (incorrect) prompt and runs /mcp retry gamma while the background task is still connecting gamma, McpPool::retry_connection races the still-in-flight background connect for the same server name — whichever finishes last wins the self.connections insert, orphaning/leaking the other transport.

Fix direction: thread the engine's actual mcp_boot_in_flight state (and the real still-connecting set) through retry_mcp_server's emitted event instead of hardcoding connecting: Vec::new(), finished: true.

Fix this →

2. mcp_snapshot_generation is written but never read (Medium, root cause link to #1)

App::mcp_snapshot_generation (crates/tui/src/tui/app.rs:1802) is documented as: "Monotonic UI generation. Explicit MCP actions advance this so a late boot result cannot overwrite newer state." It's incremented once, in handlers.rs:625, and never read anywhere else (confirmed via repo-wide grep). apply_mcp_session_boot_event — the function that actually applies every McpSessionBoot event, including late/stale ones from the background boot task — never checks it. The field currently documents a guarantee the code doesn't provide, which is exactly the gap behind finding #1.

3. Background boot connect can resurrect a connection for a server just removed from config (Medium)

start_mcp_session_boot intentionally connects pending servers without holding the pool lock (crates/tui/src/mcp.rs, connect_pending_concurrently doc at line ~2868), then reacquires the lock to call store_ready_connection per result. If the user runs /mcp remove <name> (or edits mcp.json) while that background pass is still in flight, reload_from_config_sources drops all connections and bumps catalog_generation — but the in-flight background task for the just-removed server can still complete afterward and call:

pub(crate) fn store_ready_connection(&mut self, name: String, mut connection: McpConnection) {
    connection.catalog_generation = self.catalog_generation.load(Ordering::SeqCst);
    self.connections.insert(name, connection);
}

with no check against current self.config.servers. manager_snapshot iterates self.config so the UI receipt stays correct, but all_tools()/to_api_tools() iterate self.connections directly (crates/tui/src/mcp.rs:3048-3079), so the removed server's tools can still reach the model, and its subprocess/transport is never closed — a leak plus a config-integrity gap (removed server briefly still "live").

4. Minor / cleanup

• Engine::bootstrap_mcp_pool (crates/tui/src/core/engine.rs:6232-6245) has an else if self.mcp_pool.is_some() && self.mcp_connection_errors.is_empty() { /* comment only, no code */ } branch. The comment claims it will "connect now without blocking later turns," but nothing executes. Currently unreachable from the real app (only the new engine test calls handle.bootstrap_mcp(); /mcp doctor maps to Validate, not BootstrapMcp), so low severity today, but worth either implementing or removing the misleading comment.
• session_boot.rs:351-356, the disabled-server row: action: if valid_name { McpServerAction::None } else { McpServerAction::None } — both arms are identical; can just be McpServerAction::None.
• views/extensions.rs ~929: mcp_error_is_timeout(error) || !error.is_empty() — since this branch is only reached when error.as_deref() was Some, the timeout check is redundant with !error.is_empty() in all but a pathological empty-string-error case.

Test coverage

The new bootstrap_and_retry_mcp_use_the_engine_owned_pool test (crates/tui/src/core/engine/tests.rs) calls bootstrap_mcp() (which waits for boot to fully settle) before calling retry_mcp_server, so it doesn't exercise the mid-boot retry race in finding #1 — that's the gap that let it through. A test that retries one server while a sibling is still artificially slow to connect (e.g. a server pointed at a command that sleeps past the others' resolution) would catch it.

No provider/model-prefix routing concerns — this PR doesn't touch provider selection or model routing.

[claude]

Claude finished @Hmbown's task in 3m 13s —— View job

---

Claude is reviewing this PR

Todo list

• Read trigger comment and repo guidance (CLAUDE.md / AGENTS.md)
• Check what changed since the last review (new commit 87603c1)
• Re-review whether High/Medium findings from the previous pass were addressed
• Review full diff against origin/main for new issues
• Post final review summary

---

Review of 87603c1 (the sync commit)

The new commit adds first-frame server naming (MCP · 4 connecting · alpha · beta · … before any snapshot arrives) and attempts to close last review's finding #1/#2 by reading mcp_snapshot_generation. The naming work itself is solid — session_boot.rs's unnamed_connecting handling and its new tests (first_frame_names_enabled_servers_before_a_snapshot_arrives, initializing_without_names_still_shows_the_count) are good, and the receipt_lines overflow calc got simplified in a way that's easier to follow than the old double-min expression.

1. The new mcp_snapshot_generation guard permanently freezes the boot receipt after the first /mcp action, for the rest of the session (High — new regression, and it doesn't actually fix last review's finding #1)

apply_mcp_session_boot_event (crates/tui/src/tui/ui/event_loop.rs:5760-5775) now does:

if app.mcp_snapshot_generation > 0 {
    return;
}

mcp_snapshot_generation is bumped once, unconditionally, on any successful /mcp mutation (handle_mcp_ui_action, crates/tui/src/tui/ui/handlers.rs:625) and is never reset for the life of the session (crates/tui/src/tui/app/init.rs sets it to 0 only at startup). So this isn't "ignore events older than the last mutation" — it's "ignore every engine-emitted McpSessionBoot event forever, from the first /mcp command onward." That's the only consumer of EngineEvent::McpSessionBoot (crates/tui/src/tui/ui/event_loop.rs:2184-2190), and it gates mcp_snapshot, mcp_connecting, mcp_initializing, mcp_configured_count, and the hotbar's MCP tool list — not just the receipt row state.

Reproduction (matches the PR's own described UX): alpha connects, beta fails fast, gamma/docs are still mid-handshake in the spawn-time background task. User runs /mcp retry beta. handle_mcp_ui_action (crates/tui/src/tui/ui/handlers.rs:613-632) applies the retry's snapshot directly (not through apply_mcp_session_boot_event) and unconditionally does app.mcp_connecting.clear(); app.mcp_initializing = false; — regardless of gamma/docs still connecting — then bumps generation to 1. gamma/docs are not connected and have no error yet, so row_from_snapshot (crates/tui/src/tui/session_boot.rs:405-420: connecting_now = initializing || connecting.contains(name), now both false) renders them as Failed with /mcp retry gamma — this is exactly last review's finding #1, still reproducible.

The new guard doesn't prevent that; it makes it worse: previously the next background-task completion event for gamma/docs would self-correct the receipt within seconds. Now, because generation is already 1, every subsequent Event::McpSessionBoot (including the real completions for gamma/docs) is silently dropped by the early return. The mislabeled "Failed" rows — and any newly-available tools from servers that finish connecting after the first /mcp action — never update again until the user runs another manual /mcp command. The new test later_user_mcp_mutation_wins_over_a_late_boot_event (event_loop.rs:5966-5976) only checks that one late event is dropped once; it doesn't cover a legitimate subsequent boot completion being dropped, which is the actual bug.

Fix direction: don't gate on generation > 0. Stamp each in-flight boot/retry with the generation it was started under (or the actual still-connecting server set + mcp_boot_in_flight, as suggested last review) and compare event_generation == app.mcp_snapshot_generation before applying, so events from before the last mutation are dropped but events for servers unaffected by that mutation still land.

Previously reported, still open (unchanged by this commit)

• Finding fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3 (background connect can resurrect a connection for a server just removed from config — store_ready_connection at crates/tui/src/mcp.rs:2790-2793 still doesn't check membership against current self.config.servers) is untouched by this commit.
• Minor Remove Normal mode and consolidate to Agent #4 items are still present: session_boot.rs:369-373 still has the identical if valid_name { None } else { None } arms for disabled rows; views/extensions.rs:939 still has the redundant mcp_error_is_timeout(error) || !error.is_empty().

Fix this →

[claude]

Claude finished @Hmbown's task in 2m 49s —— View job

---

Todo list

• Read trigger comment and repo guidance (CLAUDE.md / AGENTS.md)
• Check what changed in the new sync commit (3636825)
• Re-check whether prior High/Medium findings (mcp_snapshot_generation guard, finding fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3, minor Remove Normal mode and consolidate to Agent #4) are addressed
• Review full diff against origin/main for new issues
• Post final review summary

Review of 3636825 (the latest sync commit)

This commit is scoped to clippy/dead-code cleanup: it wires plain /mcp (McpUiAction::Show) to snapshot the engine-owned live pool via the new bootstrap_mcp()/Op::BootstrapMcp path instead of a config-only discovery read, and collapses the identical-arms disabled-row branch in session_boot.rs.

Fixed from prior review:

• Minor Remove Normal mode and consolidate to Agent #4's session_boot.rs:369-373 if valid_name { None } else { None } → collapsed to McpServerAction::None. ✅
• Engine::bootstrap_mcp_pool/Op::BootstrapMcp are no longer dead/test-only — /mcp now calls them in production. ✅

Still open, unchanged by this commit:

• High finding Fix cargo fmt formatting issues #1 / Simplify UI and improve footer display (v0.3.14) #2 (generation guard permanently freezes the boot receipt) — apply_mcp_session_boot_event (crates/tui/src/tui/ui/event_loop.rs:5766-5768) still does if app.mcp_snapshot_generation > 0 { return; }, and mcp_snapshot_generation is still bumped once and never reset (crates/tui/src/tui/ui/handlers.rs:628). Confirmed by re-reading current source, not just diff.
• Medium finding fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3 — store_ready_connection (crates/tui/src/mcp.rs:2793-2796) still inserts into self.connections with no check against current self.config.servers, so a background connect for a just-removed server can still land.
• views/extensions.rs:939 — mcp_error_is_timeout(error) || !error.is_empty() redundancy is still present.

New concern from this commit's wiring (Medium/High — worth a look before merge):

handle_mcp_ui_action (crates/tui/src/tui/ui/handlers.rs:581-582) now routes plain /mcp through engine_handle.bootstrap_mcp().await → Engine::bootstrap_mcp_pool (crates/tui/src/core/engine.rs:6226-6236), which calls self.wait_for_mcp_boot().await whenever self.mcp_boot_in_flight is true. The engine's run() loop (crates/tui/src/core/engine.rs:2389-2420) is a single sequential loop { match next_run_input... } — there is exactly one turn/op loop per AGENTS.md, no per-op task spawn. That means: if a user runs plain /mcp while spawn-time background boot is still connecting servers (a state the whole PR is designed to make visible and expected to happen), the entire engine mailbox blocks — no SendMessage, no cancel, no other Op — until every optional server's connect attempt settles (up to the connect timeout). This directly cuts against the PR's stated goal ("Optional servers do not block the first LLM call") for the whole app, not just the /mcp response; mcp_tools() (engine.rs:6288-6294) deliberately avoids this exact wait during a turn, so the asymmetry looks unintentional rather than designed. Since Show was previously routed through the non-blocking discovery/config-read branch, this blocking behavior is newly reachable from production via this commit, not pre-existing.

Fix this →

Net assessment

The cleanup commit does what its message says (clippy/dead-code) but doesn't touch the two previously-reported correctness findings, and its one behavioral change (Show → live-pool bootstrap) introduces a new way for a user to stall the entire session mid-boot. Recommend addressing the generation-guard bug (#1/#2) and the bootstrap-blocking issue above before merge; #3 and the extensions.rs redundancy remain lower-priority cleanup.

No provider/model-prefix routing concerns — this PR still doesn't touch provider selection or model routing.

[Hmbown]

Closing this stale PR as superseded by #5677.

The replacement retained the original four implementation commits and their author/co-author metadata, then added the focused correctness repair for plugin-authority auth errors and post-/mcp boot events before integrating current main. It passed the complete exact-head gate, including Ubuntu, macOS, Windows, lint, safety, CodeQL, Claude review, RSS, lockfile, and offline-eval checks.

Merged replacement: #5677
Merge commit: 5ac07db4566607b329d5b5e8e68e745ce1f20cc3
Merged head: ef5c5246037a40d2634290ea1eae2be6f872b763
Merge parents: 7de6e98757e4e0bc676d51f8063c9e5e5ec33e22 + ef5c5246037a40d2634290ea1eae2be6f872b763

Thank you for carrying this work through the rescue and repair path.