fix(tui): show MCP servers that failed to start in /mcp - #835
fix(tui): show MCP servers that failed to start in /mcp#835Vasanthdev2004 wants to merge 3 commits into
Conversation
The panel derived every server's state from config alone: `disabled` if the
user turned it off, `enabled` otherwise. MCP registration is best-effort —
a server that cannot be reached is recorded and startup continues — so a
server that never connected was listed as enabled with its tools silently
missing and nothing in the panel to explain it.
Startup already knows: it prints a warning per skipped server to stderr.
That scrolls away behind the first screen of output, and /mcp is where a
user goes afterwards to ask what is actually running.
Thread the skipped set from the MCP runtime through to the panel and render
a third state, `failed`, with the recorded reason underneath the server:
› docs · failed · stdio
exec: "docs-mcp": executable file not found in $PATH
The reason comes from the server, so it goes through redaction — a
handshake error that echoes back the Authorization header would otherwise
print the token into the transcript. Disabled still wins over failed: the
user turned that one off, so it was never expected to connect.
The stderr warning is unchanged; the panel is an addition to it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMCP startup failures now flow from the CLI runtime into TUI options and model state. The ChangesMCP failure visibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPRuntime
participant CLI
participant TUIModel
participant MCPView
MCPRuntime->>CLI: Return skipped server failures
CLI->>TUIModel: Pass MCPSkipped through tui.Options
TUIModel->>MCPView: Build MCP view state
MCPView->>MCPView: Render failed status and sanitized reason
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/app_mcp_skipped_test.go (1)
63-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the failure reason is forwarded.
The test only verifies the server name. Also assert
MCPSkipped[0].Errcontains"connection refused"so a regression that drops the recorded error cannot pass.Proposed test strengthening
if len(launchedOptions.MCPSkipped) != 1 || - launchedOptions.MCPSkipped[0].Name != "docs" { + launchedOptions.MCPSkipped[0].Name != "docs" || + launchedOptions.MCPSkipped[0].Err == nil || + launchedOptions.MCPSkipped[0].Err.Error() != "connection refused" { t.Fatalf("MCPSkipped = %#v, want the failure startup recorded", launchedOptions.MCPSkipped) }As per coding guidelines, add a regression test for behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/app_mcp_skipped_test.go` around lines 63 - 66, Strengthen the existing MCPSkipped assertion in the test by also verifying that MCPSkipped[0].Err contains “connection refused,” while preserving the current server-name check and failure-count validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/app_mcp_skipped_test.go`:
- Around line 63-66: Strengthen the existing MCPSkipped assertion in the test by
also verifying that MCPSkipped[0].Err contains “connection refused,” while
preserving the current server-name check and failure-count validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6847c24c-e809-446d-80a2-a6db7325507d
📒 Files selected for processing (8)
internal/cli/app.gointernal/cli/app_mcp_skipped_test.gointernal/tui/command_views.gointernal/tui/mcp_failed_state_test.gointernal/tui/mcp_state.gointernal/tui/mcp_view.gointernal/tui/model.gointernal/tui/options.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
The failed-state wiring and secret redaction look good, but the new failure-reason rendering needs terminal sanitization before merge.
BuildMCPViewState passes redaction.ErrorMessage(err, ...) into MCPServerView.Error, and mcpManagerServerLines inserts that value directly into the rendered lines. Redaction removes credentials but does not remove ANSI/OSC sequences, other control characters, or embedded newlines. I reproduced this with an MCP error containing connection refused\x1b[2J\n› forged · enabled; the resulting server line retained both the escape sequence and newline unchanged. A server-controlled handshake error can therefore manipulate the terminal or forge extra /mcp rows.
Please normalize the displayed reason to safe single-line terminal text: strip ANSI/OSC and control characters, flatten CR/LF, apply a reasonable length cap, and add a regression covering escape and newline injection.
Everything else in the change looks correct, and the focused tests and CI are green.
The failure reason is the only value on the /mcp panel that the MCP server writes itself, and it went to the terminal with nothing but TrimSpace. redaction.ErrorMessage strips credentials, not control bytes, so a hostile handshake error could clear the screen, move the cursor, or embed a newline followed by text shaped like a real entry and forge a row for a server that does not exist. Reproduced with @anandh8x's payload from the review. Before the fix the rendered panel was: > evil . failed . http connection refused\x1b[2J > forged . enabled actions: zero mcp check evil | ... The escape sequence and the forged row both survived intact. sanitizeTerminalReason consumes escape sequences whole rather than dropping ESC alone, since removing the ESC and leaving "[2J" behind would print visible junk and an abandoned OSC payload can still smuggle a title-set or hyperlink. CSI runs to its final byte, OSC to BEL or ST. Newlines and tabs collapse to spaces so the reason stays on the single row the panel counted for it, other control bytes are dropped, and the result is capped at 400 runes so one verbose server cannot push the panel off screen. Truncation is by rune, not byte, so a multi-byte character is never cut in half. Two regressions cover it. The injection test asserts no escape byte survives, no rendered line carries its own newline, the forged text never begins a row, and the real reason is still shown. The cap test drives 5000 characters through and asserts the rendered line stays bounded. Both fail on the code before this commit.
c7421d9
|
@anandh8x fixed in Before the fix the panel rendered your payload as: Escape sequence intact, forged row on its own line.
Two regressions, both of which fail on the previous commit: one asserts no escape byte survives, no line carries its own newline, the forged text never starts a row, and the real reason still shows. The other pushes 5000 characters through and asserts the line stays bounded. Re-requesting you and @kevincodex1, since the push dismissed his approval. One thing worth flagging beyond this PR: the same class exists at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/mcp_view.go`:
- Around line 192-238: Bound MCPServerView.Error before processing in the
sanitizer around the visible rune conversion and strings.Builder accumulation.
Verify whether Runtime.Skipped() already imposes a strict size limit; if not,
limit the raw input before converting to []rune and stop accumulating once the
maxMCPReasonLen display budget is reached, while preserving ANSI stripping,
whitespace normalization, and truncation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63dc33b5-a906-45b6-b233-5fc22f09514c
📒 Files selected for processing (2)
internal/tui/mcp_failed_state_test.gointernal/tui/mcp_view.go
The display cap runs at the end, so the sanitizer walked the whole server-authored string first. Escape sequences are consumed without producing output, so they spend input against a budget that never fills: 64KB of "\x1b[2J" was walked in full and the text after it still rendered. Nothing upstream bounds the handshake error, and the panel re-runs this on every redraw. Cap the raw input at 16KB before the walk, well above the 400 rune display cap so a long error is still truncated by display rules. Trim back a character the cut splits so the panel never renders a replacement character it produced itself.
|
Pushed one more commit for the bot's finding, which was real. The 400 rune cap ran at the end, so the sanitizer walked the whole server string first, and escape sequences get consumed without producing output. 64KB of The bot's other claim on #866 (duplicate |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Verified empirically on the branch (checked out, built).
What I checked
- Gut-the-fix: forcing the failure branch off (
mcp_state.go:71) so a skipped server renders as "enabled" turns the TUI MCP tests red. The tests exercise the behavior. - Precedence is right and tested: a server that is both disabled and recorded-failed shows as "disabled", not "failed" (
mcp_state.go:66-69), and this is asserted directly (mcp_failed_state_test.go:45— "disabled to win over a recorded failure"). Correct — a server you turned off was never expected to connect. - Reason is redacted before display (
redaction.ErrorMessage,mcp_state.go:73) — invariant 6, so a path/credential in a startup error can't leak into the panel. Empty-error fallback ("server did not start") is handled too. - Skipped set reaches the panel via
MCPSkipped: mcpRuntime.Skipped(); companion to #822 which did the same forzero mcp check. - Clean scope — every file is the MCP panel or its plumbing.
Worth a quick confirm (non-blocking)
- On reconnect (
zero mcp enableafter a failure), the panel is rebuilt from a freshSkippedset, so it should flip back to enabled — worth a sanity check that the runtime clears the entry on a successful re-register.
Good fix.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge and review state
- Review decision: GitHub still reports
CHANGES_REQUESTED(anandh8x’s review onbe580766is not dismissed). Terminal sanitization and raw-input bounding from that review look addressed on the current head, but the merge gate still needs a fresh approval on6521c367. gnanam1990 approved on6521c367; CodeRabbit approved on the same head. - Mergeability:
MERGEABLE, no conflict markers in the PR diff.mergeStateStatusisBLOCKEDpending required reviews. - Checks: All CI / CodeQL / Zero Review / CodeRabbit checks passed on head.
Prior review alignment
gnanam1990’s approval is valid for what they checked: skipped servers map to failed, disabled wins over recorded failure, reasons are redacted at build (redaction.ErrorMessage), MCPSkipped reaches the TUI, and the focused tests go red when the failure branch is gutted. Those checks exercise buildMCPServerViews, renderMCPView, and m.mcpText() — not the bare /mcp manager overlay entry point. Their non-blocking note about enable clearing Skipped on reconnect is separate from this finding; live reconnect from the TUI is out of scope per the author, and mcpSkipped remains a startup snapshot.
That approval does not negate the remaining gap below: state and redaction work on the transcript/renderMCPView path, but the overlay users get from bare /mcp still omits the reason line.
Findings
- [P2] Bare
/mcpshowsfailedin the manager overlay but not the failure reason
internal/tui/model.go(commandMCP→openMCPManager),internal/tui/mcp_manager.go(mcpManagerOverlay,mcpManagerServerMeta,mcpManagerSelectionDetail),internal/tui/mcp_view.go(mcpManagerServerLines,renderMCPView)
Empty/mcphas long routed toopenMCPManager()— this PR did not change that. What it did change is meaningful:buildMCPServerViewsnow marks skipped servers asfailed, so the overlay meta and detail pane correctly sayfailedinstead of the pre-PRenabledwith missing tools. The recorded reason, however, is rendered only inmcpManagerServerLinesinsiderenderMCPView(). The overlay never readsserver.Erroror callssanitizeTerminalReason, so a user who types/mcpafter a startup warning sees the right state but not the “why” issue #825 and this PR’s description target. TherenderMCPViewpath does show the sanitized reason — on/mcp listand other transcript subcommands, and in transcript output appended after manager actions such as check or list — but the primary overlay surface is still incomplete relative to the stated fix.TestModelMCPPanelReportsStartupFailuresexercisesm.mcpText()/renderMCPView(), not the bare/mcpentry point. Please surface the sanitized reason in the manager overlay (list meta, selection detail, or both), reusing the samesanitizeTerminalReasonpathmcpManagerServerLinesalready uses.
5 participants
Fixes #825. Companion to #822, which fixed the same blind spot in
zero mcp check./mcpworked out each server's state from the config file — disabled if you turned it off, enabled otherwise. But MCP registration is best-effort: a server that can't be reached gets recorded and startup carries on. So a server that never connected showed up as enabled, its tools quietly missing, and nothing in the panel said why.Startup does know — it prints a warning per skipped server to stderr. That's gone by the time you notice, and
/mcpis exactly where you go afterwards to ask what's actually running.So the skipped set now reaches the panel, and a server that failed renders as failed with the reason under it:
Two details worth calling out:
The reason comes from the server, so it goes through
redaction.ErrorMessagebefore it's rendered. A handshake error that echoes theAuthorizationheader back would otherwise print the bearer token straight into the transcript. There's a test for that.Disabled wins over failed. If you turned a server off it was never expected to connect, and calling it failed would be misleading.
The stderr warning is unchanged — non-interactive users still get it, and the panel is an addition rather than a replacement.
Still not fixed, and out of scope here: enabling a server from inside the TUI updates the config but doesn't reconnect anything, so it'll show as enabled while not actually running until you restart. That's pre-existing and a bigger change; happy to file it separately if you'd like.
Verified with mutation testing — eight mutations across the state builder, the renderer, and both wiring points, all killed.
TestAltScreenTranscriptScrollKeepsFooterFixedandTestBuildServeScopeKeepsLexicalPathsfail on my Windows box on cleanmaintoo (the second needs symlink privilege).Summary by CodeRabbit
New Features
/mcppanel.Bug Fixes