Skip to content

fix(tui): keep MCP boot detail out of chat - #5760

Open
Hmbown wants to merge 4 commits into
mainfrom
fix/tideline-mcp-quiet-20260830
Open

fix(tui): keep MCP boot detail out of chat#5760
Hmbown wants to merge 4 commits into
mainfrom
fix/tideline-mcp-quiet-20260830

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Closes #5759

Product change

Moves per-server MCP boot detail out of the chat/composer shell. The existing footer is the compact status surface; /mcp remains the detailed diagnostic and action surface. The retired multi-row receipt renderer and its dead action metadata are removed rather than left as warning-producing legacy code.

Verification

  • cargo fmt --all -- --check
  • git diff --check
  • RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib --locked failed_mcp_is_a_footer_chip_not_multiline_chat_boot_output
  • CODEWHALE_DEV_NEXTEST=0 ./scripts/dev-test.sh tui session_boot (12 passed)

No live-terminal visual acceptance is claimed by this PR.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-30T22:48:59.414072Z 5512159 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 35s —— View job


Review: fix(tui): keep MCP boot detail out of chat

  • Gather context (diff, changed files, related code)
  • Understand the request (code review)
  • Review session_boot.rs changes
  • Review frame.rs changes
  • Review tests.rs additions
  • Check provider/model safety implications (not applicable to this diff)
  • Post final review

Summary

Clean, well-scoped removal. session_boot::render/receipt_height and the layout slot they occupied in frame.rs are deleted together, and the Layout constraint list, slot indices (plugin_cta_slot/composer_slot/footer_slot), and every downstream body_chunks[..] use were checked — they line up correctly (8 constraints ↔ indices 0–7, no stale session_boot_slot reference left behind). No correctness bugs found.

Reuse / simplification

crates/tui/src/tui/session_boot.rs:230receipt_lines (plus its only helpers row_from_snapshot, server_row_text, plugin_receipt_line, truncate_to_width, and MAX_RECEIPT_ROWS) lost their only production call site when render()/receipt_height() were deleted. They're still pub fn and now exercised solely by this module's own unit tests (receipt_lines(...) at lines 521, 540, 570, 617, 678, 695, 717, 737, 764, 795) — i.e. dead production code kept alive by self-referential tests. The updated module doc says "detailed diagnosis and actions belong in /mcp," but /mcp's own view (crates/tui/src/tui/setup/tools_mcp.rs) doesn't call into receipt_lines either — it appears to have its own independent rendering path. Worth either wiring receipt_lines into the /mcp detail surface the doc comment now points to, or removing it and its dedicated tests along with the rest of the boot-receipt code in this cleanup, so the module doesn't carry orphaned logic.

This is a simplification note, not a blocker — no behavior is broken by leaving it.

Tests

failed_mcp_is_a_footer_chip_not_multiline_chat_boot_output (tests.rs:4137) is a good, focused regression test: it pins the composer area to be identical between a baseline app and one with a failed MCP server (guarding against the receipt reclaiming rows), and asserts the footer chip text (MCP · 0 connected · 1 failed) appears while per-server detail (alpha · failed, /mcp retry alpha) does not. Matches the product intent described in the PR body.

Provider/model/route safety

Not applicable — no model/provider selection logic touched.

Security

Not applicable — no secret handling, exec, or input-validation surface touched.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5512159052

ℹ️ 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".

Constraint::Length(preview_height), // Pending input preview (0 if empty)
Constraint::Length(indicator_height), // Background-work chip (#5286, 0 if idle)
Constraint::Length(session_boot_height), // MCP+plugin boot receipt (0 if quiet)
Constraint::Length(plugin_cta_height), // Live plugin CTA (0 unless matched)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve plugin boot diagnostics on a visible surface

On initial startup with an invalid, duplicate, or unreviewed plugin and no MCP problem, removing the session-boot layout slot here makes the plugin receipt unreachable. SessionBootSurface::activity_chip only creates MCP candidates, and initialization does not emit a replacement plugin toast, so users receive no indication that a plugin failed or needs setup unless they independently open /plugin; retain a compact plugin warning on its designated status surface while removing the MCP detail rows.

AGENTS.md reference: crates/tui/AGENTS.md:L8-L10

Useful? React with 👍 / 👎.

@claude

claude Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

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


Review: fix(tui): keep MCP boot detail out of chat

  • Gather context (diff, changed files, related code)
  • Understand the request
  • Review session_boot.rs changes
  • Review frame.rs changes
  • Review tests.rs additions
  • Investigate Codex's plugin-diagnostics concern
  • Post final review

Summary

The second commit (a936482) fully deletes the dead receipt_lines/render/receipt_height code path that the earlier Claude review flagged as orphaned, along with its self-referential tests — good cleanup, nothing left dangling. frame.rs's Layout constraints and slot indices (plugin_cta_slot/composer_slot/footer_slot) were re-checked against this update: 7 constraints ↔ indices 0–6, no stale session_boot_slot/is_hidden references remain (grep confirms is_hidden has no other call sites). No compile-breaking or layout-indexing bugs found.

Confirming Codex's P2 finding: plugin diagnostics are now unreachable, not just de-emphasized

I traced this and it's a real regression, not just a nitpick. PluginBootSummary (session_boot.rs:42-83) still computes invalid/duplicate/needs_setup counts from PluginRegistry, and is_quiet() still gates SessionBootPhase (session_boot.rs:143), but:

  • activity_chip() (session_boot.rs:170-207) only branches on self.servers (MCP rows) — it never reads self.plugins at all.
  • The deleted plugin_receipt_line was the only code that ever rendered PluginBootSummary's contents anywhere.
  • So today: a plugin that fails to load, is a name/root duplicate, or needs trust review produces zero passive UI signal. If there are no MCP servers configured, phase becomes Settled (not Hidden, since plugins.is_quiet() is false) — yet activity_chip still returns None in that case, so nothing appears in the footer either. /mcp (the surface the module doc now points to) is MCP-only; extensions.rs//plugin shows plugin diagnostics but only if the user proactively navigates there.

Before this PR, the multi-row receipt was the only thing that surfaced this passively. This PR's stated intent ("MCP+plugin boot surface" per the module doc, session_boot.rs:1-6) still claims to cover plugins, but the plugin half of that contract is now dead data computed and never shown. Codex's suggested fix (retain a compact plugin warning on its own status surface, e.g. a footer chip alongside the MCP one) is the right shape — this doesn't need the multi-row chat-area receipt back, just a passive signal.

Tests

failed_mcp_is_a_footer_chip_not_multiline_chat_boot_output (tests.rs:4137) is a solid, focused regression test for the MCP side: pins the composer area, confirms the footer chip (MCP · 0 connected · 1 failed) appears, and per-server detail/actions (alpha · failed, /mcp retry alpha) does not. There's no equivalent test for a plugin-only-problem scenario (invalid/duplicate plugin, no MCP servers) — adding one would have caught the regression above, since it would show the plugin summary now renders nowhere.

Provider/model/route safety & Security

Not applicable — no provider/model selection or secret/exec/input-validation surface touched.

@codewhale-agent codewhale-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewhale review

PR #5760 removes the retired MCP+plugin boot receipt renderer and McpServerAction metadata, updates the TUI frame layout slots to match, and adds a regression test ensuring failed MCP state appears only in the footer chip rather than consuming chat/composer rows.

Assessment

The change is focused and consistent. Frame slot indices are correctly shifted after deleting the session_boot row, dead action metadata is fully removed, and the new test covers the reported regression. No correctness issues found.


Advisory review by Codewhale (codewhale review --pr 5760 --post, head a9364824d92898e4c9e12b21ba64ed910450fe49). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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


Review: fix(tui): keep MCP boot detail out of chat

  • Gather context (diff, changed files, prior review threads)
  • Check whether the latest commit (c04a3cc) resolves the plugin-diagnostics regression Codex/Claude flagged earlier
  • Review phase_strip.rs, session_boot.rs, frame.rs, tests.rs changes
  • Check provider/model/route safety implications
  • Post final review

Summary

The third commit, c04a3cc, resolves the plugin-diagnostics regression that Codex's P2 comment and the earlier Claude review both flagged against a936482: with the multi-row receipt gone, an invalid/duplicate/unreviewed plugin previously had no passive UI signal at all. This commit adds a compact Plugins · Problems: N · /plugins footer notice (with graceful width-shedding to Plugins · Problems: N then Plugins · N), gated behind a new SessionBootActivityLevel (Active/Attention/Failure) so phase_strip.rs maps severity through the same palette as every other footer fact instead of guessing from text. MCP activity correctly outranks plugin problems when both are present (mcp_activity_outranks_plugin_problems), and healthy/quiet plugin sets still resolve to SessionBootPhase::Hidden (healthy_loaded_plugins_do_not_claim_the_boot_surface). This is the right shape — a compact status-surface signal, not the old multi-row chat receipt.

Nice catch bundled into the same commit: PluginTrustStatus::ContentChanged was previously excluded from needs_setup on origin/main (only NeverReviewed | CapabilitiesChanged triggered it) — that gap is now closed via plugin_trust_needs_setup, with a dedicated test (changed_plugin_content_requires_setup).

I re-verified the points earlier reviews already checked and confirm they still hold on this diff:

  • frame.rs layout constraints/slot indices (plugin_cta_slot/composer_slot/footer_slot) are consistent — 7 constraints ↔ indices 0–6, no stale session_boot_slot reference.
  • No dangling references to the removed session_boot::render/receipt_height/McpServerAction anywhere in the crate (the McpServerActionReceipt hits in runtime_api.rs are an unrelated HTTP API struct).
  • No other call site reads SessionBootSurface::phase outside session_boot.rs itself, so the is_quiet() semantics change (dropping loaded == 0 from the quiet check) has no effect beyond gating this footer notice.
  • /plugins is a real alias (crates/tui/src/commands/groups/plugins/mod.rs:60), so the chip's hint text is accurate.

Minor notes (non-blocking)

  • failed_mcp_is_a_footer_chip_not_multiline_chat_boot_output (tests.rs:4137) is the only render-level/integration test, and it's MCP-only. The new plugin-problem coverage (plugin_problems_have_a_compact_footer_action, plugin_review_notice_uses_attention_and_sheds_whole_fields) stops at the SessionBootSurface unit level — there's no equivalent full-frame render test proving a plugin-only problem (no MCP servers) actually reaches the rendered footer through frame.rs/phase_strip.rs. Given the render path is shared with the already-tested MCP case, this is low risk, but an integration test would fully close the loop the original regression opened.
  • has_failures() treats plugin duplicate (name/root conflicts) the same as invalid (validation errors) for Failure severity. That reads as a deliberate, reasonable severity call rather than a bug — flagging only so the author can confirm duplicates are meant to read as red rather than yellow.

Provider/model/route safety & Security

Not applicable — no provider/model selection or secret/exec/input-validation surface touched.

Note on verification

I could not run cargo test/cargo check in this environment (tool approval for cargo commands wasn't available here); this review is based on static reading of the diff plus grep-verified dependency/reference checks (slot indices, dangling symbols, message IDs, command aliases) rather than a live compile/test run. The PR body's own cargo test/dev-test.sh results should be trusted for that evidence.

@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 5s —— View job


Review: fix(tui): keep MCP boot detail out of chat

  • Gather context (diff, changed files, prior review threads)
  • Review latest commit e2c7225 ("keep boot status API single path") against earlier flagged issues
  • Re-check phase_strip.rs, session_boot.rs, frame.rs, event_loop.rs, tests.rs for correctness
  • Check provider/model/route safety implications
  • Post final review

Summary

e2c7225 is a small, correct follow-up cleanup. It deletes SessionBootSurface::activity_chip (session_boot.rs), which had become a pure passthrough wrapper (activity_notice(...).map(|n| n.text)) after the earlier commits in this PR — phase_strip.rs:1072 already called activity_notice directly in production code, so activity_chip had no remaining non-test callers. This commit updates the module's own unit tests and event_loop.rs's session_boot_event_tests to call activity_notice(...).map(|notice| notice.text) directly, matching production usage. I grepped the crate and confirmed:

  • No remaining references to activity_chip anywhere.
  • No dangling references to earlier-removed symbols (session_boot_slot, receipt_lines, receipt_height, McpServerAction).
  • SessionBootActivityNotice's text/level fields (session_boot.rs:122-123) are pub, matching the new .map(|notice| notice.text) call pattern.

This closes out the "single path" API cleanup implied by earlier reviews' dead-code notes — good incremental hygiene, no behavior change.

Status of previously raised issues

  • The P2 plugin-diagnostics regression (Codex + earlier Claude reviews on a936482) was resolved in c04a3cc via the new Plugins · Problems: N · /plugins footer notice gated by SessionBootActivityLevel, and remains correctly wired after this commit.
  • The one still-open, non-blocking note from the prior review stands: there's no full-frame/integration test for a plugin-only problem (no MCP servers) proving the notice reaches the rendered footer through frame.rs/phase_strip.rs — coverage stops at the SessionBootSurface unit level (plugin_problems_have_a_compact_footer_action, etc.). e2c7225 doesn't add or need to add this; it's a mechanical cleanup commit. Given the render path is shared with the already-tested MCP case, this remains low risk but worth closing in a follow-up if you want full loop closure.

Provider/model/route safety & Security

Not applicable — no provider/model selection, secret handling, exec, or input-validation surface touched.

Note on verification

cargo build/cargo test were not available in this environment (no tool approval for cargo commands here); this review is based on static reading of the diff plus grep-verified reference checks (no dangling symbols, field visibility matches new call sites, single remaining production call site for activity_notice). The PR body's own cargo test/dev-test.sh results should be trusted as the executed evidence for this commit.

@codewhale-agent codewhale-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewhale review

PR removes the multi-row MCP boot receipt renderer and centralizes boot activity as a compact footer chip with typed severity. The refactor is coherent and the new tests cover the footer mapping and layout stability. A couple of semantic changes deserve confirmation.

Findings

  • [WARNING] PluginBootSummary::is_quiet semantics changed (crates/tui/src/tui/session_boot.rs:53)
    is_quiet previously returned true only when no plugins were loaded and there were zero problems. It now returns true whenever problem_count() == 0, so a state with loaded plugins but no problems is considered quiet. This is intentional for the new footer notice, but any other caller expecting the old meaning will silently suppress plugin-loaded UI.
  • [INFO] ContentChanged is newly treated as needs_setup (crates/tui/src/tui/session_boot.rs)
    plugin_trust_needs_setup now includes PluginTrustStatus::ContentChanged, which was not counted before. This changes plugin boot classification and should be confirmed against the product spec (the test explicitly asserts it).
  • [INFO] McpServerBootRow::action removal is a breaking change (crates/tui/src/tui/session_boot.rs)
    The action field and McpServerAction enum were removed. The diff updates internal constructors and tests, but any other module constructing or matching McpServerBootRow will fail to compile. The PR verification presumably covers this, but it is worth a grep for remaining uses.

Assessment

The PR is well-structured and achieves its goal of removing multi-row boot output from the chat. Tests for the new footer mapping and composer stability are present. The main residual risk is semantic drift in is_quiet and ContentChanged classification; if those are intentional, the change looks good to merge.


Advisory review by Codewhale (codewhale review --pr 5760 --post, head e2c7225a2e7fa68a61afa699edb106415bb09113). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

#[must_use]
pub fn is_quiet(self) -> bool {
self.loaded == 0 && self.invalid == 0 && self.duplicate == 0 && self.needs_setup == 0
self.problem_count() == 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] PluginBootSummary::is_quiet semantics changed

is_quiet previously returned true only when no plugins were loaded and there were zero problems. It now returns true whenever problem_count() == 0, so a state with loaded plugins but no problems is considered quiet. This is intentional for the new footer notice, but any other caller expecting the old meaning will silently suppress plugin-loaded UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keep MCP boot diagnostics out of the chat transcript

1 participant