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 20 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMCP startup failures now flow from the CLI runtime into TUI state. The ChangesMCP failure visibility
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The change can display an MCP credential in the /mcp panel and hide the next argument instead, potentially persisting a secret in the transcript. This security issue makes the PR unsafe to merge until packed sensitive arguments are redacted correctly. Sequence Diagram(s)sequenceDiagram
participant MCPRuntime
participant CLI
participant TUIModel
participant MCPState
participant MCPView
MCPRuntime->>CLI: Return skipped server failures
CLI->>TUIModel: Pass MCPSkipped through tui.Options
TUIModel->>MCPState: Build failed server state
MCPState->>MCPView: Provide redacted failure reason
MCPView->>MCPView: Sanitize and bound rendered output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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.
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
|
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.
ff70596
6521c36 to
ff70596
Compare
An empty /mcp opens the manager overlay, and it reported the state without the reason. The recorded "why" was rendered only by mcpManagerServerLines inside renderMCPView, which serves /mcp list and the transcript, so the panel said "failed" and stopped exactly where someone goes to find out why after the startup warning has scrolled away. The reason now sits in the selection detail, directly under the header and above the target, through the same sanitizeTerminalReason path the transcript uses. TestModelMCPPanelReportsStartupFailures drives m.mcpText() and passes with or without this, which is how the gap survived review. The new tests drive openMCPManager().mcpManagerOverlay() instead. Mutation-verified: feeding the sanitizer an empty reason fails the first one. The sanitization test deliberately does not search for a bare escape byte. The overlay is lipgloss-styled and therefore full of escape sequences it wrote itself, so the assertion is that the SERVER's payload did not survive: no clear-screen sequence, and no row carrying the forged text on its own. The sanitizer collapses the newline, so the forged text stays inert on the reason line rather than becoming an entry of its own. Reported by jatmn on #835.
|
Rebased onto main and fixed the overlay gap. Thanks @jatmn, that was exactly right and the reason it survived review is worth stating. The finding. Bare Why it got through. One thing I got wrong while writing the sanitization test, worth recording. My first version asserted the overlay contained no The rebase. Two conflicts against #884:
@anandh8x your changes-requested predates the terminal-sanitization work you asked for, which landed a while back; a re-look when you have a moment would unblock this. |
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.
Findings
- [P1] Redact the configured server credentials before retaining its error
internal/tui/mcp_state.go:73
ErrorMessageis called with empty options, even though remote MCP clients send every value inMCPServerConfig.Headers. A server can echo an arbitrary configured value (for exampleX-Workspace-Credential: <value>) in its failed startup response; that value is not covered by the generic redaction patterns, is saved inMCPServerView.Error, and is newly rendered in both/mcpsurfaces and the session transcript. Pass the configured secret values into redaction (and cover an echoed custom header); this also avoids relying on the syntacticAuthorization:matcher, which terminal control bytes can evade before the later sanitizer removes them.
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>
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.
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.
An empty /mcp opens the manager overlay, and it reported the state without the reason. The recorded "why" was rendered only by mcpManagerServerLines inside renderMCPView, which serves /mcp list and the transcript, so the panel said "failed" and stopped exactly where someone goes to find out why after the startup warning has scrolled away. The reason now sits in the selection detail, directly under the header and above the target, through the same sanitizeTerminalReason path the transcript uses. TestModelMCPPanelReportsStartupFailures drives m.mcpText() and passes with or without this, which is how the gap survived review. The new tests drive openMCPManager().mcpManagerOverlay() instead. Mutation-verified: feeding the sanitizer an empty reason fails the first one. The sanitization test deliberately does not search for a bare escape byte. The overlay is lipgloss-styled and therefore full of escape sequences it wrote itself, so the assertion is that the SERVER's payload did not survive: no clear-screen sequence, and no row carrying the forged text on its own. The sanitizer collapses the newline, so the forged text stays inert on the reason line rather than becoming an entry of its own. Reported by jatmn on #835.
…error
Reported by jatmn. BuildMCPViewState passed redaction.Options{} when rendering a
failed server's startup error, so only the generic patterns applied.
Those patterns match shapes they recognise. A remote MCP server is configured
with ARBITRARY headers, so a credential can sit under a name nobody can predict:
X-Workspace-Credential matches nothing. A server that echoes the request it
failed on puts that value into MCPServerView.Error, which this PR newly renders
in both /mcp surfaces AND the session transcript. So the exposure is one this
change introduces rather than one it inherits.
The configured values are now passed as ExtraSecretValues, which redacts by
equality instead of by shape, so the header name does not have to be guessable.
That also drops the dependence on the syntactic Authorization: matcher, which
terminal control bytes can split before the later sanitizer strips them.
Env values are included for the same reason on the stdio path: the child is
launched with them and a failure to exec commonly reports the environment it was
given. Auth and an OAuth client secret are included too.
Values shorter than eight characters are skipped. A configured "1" or "true" is
not a credential, and redacting it by equality would punch holes through
unrelated text, which is its own way of making an error useless. There is a test
for that, because the fix would otherwise be free to shred the message.
Tests cover an echoed custom header, an echoed env secret, the short-value case,
and a healthy server carrying no error text at all. Verified by mutation:
dropping the options renders the credential verbatim.
The reason is written by the server that failed, and it is both rendered and persisted to the transcript, so it is untrusted text that can carry back whatever Zero sent. Four separate ways a credential survived it. Redaction ran before the display sanitizer, and matched configured values literally. A server echoing a secret with a control byte pushed into the middle of it matched nothing, and the sanitizer then removed that byte WITHOUT leaving a gap and reassembled the intact credential on screen. Every control byte it drops rejoins the same way, so this was never specific to ANSI. The reason is now normalized to what the reader will see and redacted again against that. The stripping half is split out of sanitizeTerminalReason as stripTerminalRejoiners so redaction does not inherit the display truncation, which would cut a secret in half and leave the head of it unmatched. Only the OAuth client secret was redacted, not the bearer that is actually sent. A failed OAuth server can echo an opaque token in its error body, and no pattern can recognize one by shape. TokenStore.SecretValues reads that material. It enumerates rather than looking up by server name, because the login path saves identity-bound and a per-name Load finds nothing for exactly the servers holding a real bearer. It is read only: LoadForServer, which the runtime bearer path uses, migrates a legacy entry as a side effect, and opening a panel must not rewrite the token store. Args were not collected at all, though a stdio child that rejects its own invocation prints it back and connectStdio appends that stderr to the error. sensitiveMCPArgValues collects the values behind a sensitive flag, sharing the predicates with the display pass so the two cannot drift, and handling the shapes the display pass gets wrong or would answer with an already-redacted string. Extra secret values were replaced in slice order, so a secret that is a prefix of another consumed its head and left the tail of a real credential printed as [REDACTED]XYZ. The partial replacement also destroyed the token shape, so the pattern passes could not recover it. Callers collect from maps and Go randomizes iteration, so which happened was decided per run. RedactString now applies values longest-first and deduped, which fixes every caller rather than this one. Each fix is falsified independently by its regression: reverting the ordering prints [REDACTED]XYZ, reverting the normalization reassembles wk-live-4f9c2b7ae1d8 for display, reverting the arg collection prints the argument secret, and reverting the token read prints the stored bearer.
Adversarial review of the previous commit found three holes in it. Invisible Unicode rejoins like a control byte, and more comfortably. A zero-width space, soft hyphen, word joiner, bidi control or byte order mark inside an echoed credential is not equal to the configured value, so redaction misses it, and the reader sees an unbroken secret because the character renders as nothing. The whole Cf category is now dropped alongside the control bytes. Combining marks are deliberately kept: they are ordinary content in most of the world's scripts, and deleting them to close a redaction hole would corrupt error messages written in those languages. The configured value is usually not the credential. Zero's own documented config spells an authenticated server as "Authorization": "Bearer <token>", so a server quoting back only the token, without the scheme word, matched nothing. Same shape through a composite --header argument, which carries a header name too. Each tail after a space or colon is now offered as its own candidate, which covers both without needing to know the scheme vocabulary, and the length floor keeps "Bearer" and the header name out of the set so those words are not blanked out of unrelated text. Collecting argument values over-reached. isSensitiveMCPDisplayFlag strips leading dashes before matching, so it says yes to a bare positional word, and the documented GitHub server config passes the env var NAME positionally: the docker image name went into the redaction set and the pull failure lost the one string that explained it. A positional argument is not a flag and no longer introduces a value. Two of the tests for this were vacuous when first written and were rewritten after reverting the fix did not fail them. The Unicode one asserted on bytes, but these characters rejoin in the reader's eye rather than in the string, so it now asserts on the perceived text. The scheme one used an sk- style value that the shape patterns already caught, so it proved the pattern list rather than the fix; it now uses an opaque credential only equality can match.
…ored tokens of any length
…re, and stop reusing stale startup failures Five findings from review, plus one the review did not raise. Header flags were classified by their NAME. isSensitiveMCPDisplayKey matches token/secret/auth/credential and friends against the flag, and neither "header" nor "H" is any of those, so a credential riding in a header whose name the operator chose was never collected. All seven forms leaked once the header name itself carried no matching word: separated, equals and packed, long and short, and with no space after the colon. A stdio child that rejects its invocation echoes it into captured stderr, which this panel renders and the transcript keeps. The short form is matched case-sensitively on purpose, because -h is help and folding case there would put the next argument into the redaction set. The endpoint was never examined at all. HTTP and SSE send the configured URL verbatim and it accepts userinfo and arbitrary query keys, so ?workspace=<token> walked through the generic query redaction, which only recognises conventional key names. Userinfo passwords and conventionally-named parameters were already covered; the arbitrary name and the userinfo username were not. Every query value is collected now, with the existing length floor keeping v=1 and mode=sse readable. WHAT THE REVIEW DID NOT RAISE, found while verifying the first two: the Target row prints the same credential verbatim, one line under the Error row that is correctly redacted, on the same panel and into the same transcript. Fixing the error alone would have handed it straight back. The display path now redacts header values while keeping the header name, and long query values while keeping the host and path. The raw bound sat at the very end, inside sanitizeTerminalReason, so the whole server-controlled string was redacted, walked rune by rune into a fresh builder and a fresh []rune, and redacted again before being cut, for a panel that shows at most 400 runes. It is applied at ingress now, with a lookahead margin sized to the longest secret so a credential straddling the cut cannot lose its tail and leave a matching prefix visible. Failures were matched against raw config-map keys while registration records the trimmed name, so a server configured as " docs " that failed to start rendered as enabled, and lost its tool count the same way. One canonical identity now, and it is the registry's. And a skipped entry is an observation about a server, not about a name. The startup snapshot was never invalidated, so removing a failed endpoint and adding a different one under the same name made the replacement inherit the dead endpoint's error and failed state, in the panel and in the command transcript. Observations are dropped when their subject is removed or changed.
… forms Two problems, both in the path that turns a failed MCP server's error into panel text. The raw bound was outside redaction.ErrorMessage rather than around the error going into it, so the whole server-controlled string was redacted, walked rune by rune, redacted again, and only then cut. An eight megabyte reason took 2.2s and allocated 286MB for a panel that shows 400 runes; it is now 9ms and 1.5MB, flat across input sizes. The lookahead margin past the cut was sized to the longest configured secret, which made the real limit "the cap plus whatever the other side configured". A two megabyte credential raised the retained error to 65546 bytes against a nominal cap of 16384. It is a fixed 4KB now. A credential longer than that can still straddle the cut and leave a prefix, which is a stated limit rather than an oversight, and a far smaller one than an unbounded margin. Credential collection only ever saw decoded values. url.Parse and url.ParseQuery decode, so a token configured as opaque%2Dworkspace%2Dtoken was collected as opaque-workspace-token and matched nothing when the server echoed back the escaped spelling it was given. parsed.User.String() is not a way out either: it re-escapes by Go's rules and leaves unreserved characters alone, so %2D comes back as a hyphen. Both forms are collected now, the raw one taken from RawQuery and from the original string's userinfo.
The existing regressions call redactMCPFailureReason directly, which proves the helper and nothing else. BuildMCPViewState is the path the panel and the transcript actually take, and it is where a second surface could reintroduce either problem: the row renderer inspects the raw query field separately from the error pipeline. Four cases through the entry point: an arbitrary percent-encoded query key, percent-encoded userinfo, a multi-megabyte failure, and a multi-megabyte configured secret. Each one fails without its fix. Reverting the fixed window reproduces the 65546-byte retained error against a 20480-byte budget, and reverting the ingress bound puts state building back over six seconds. One honest note on coverage: the percent-encoded userinfo PASSWORD case passes either way, because generic URL redaction already covered passwords. The query key and the username are the two that were actually leaking.
…edential material Three things, all in the failure-display pipeline. The bound cuts the raw error before redaction, and redaction matches whole values, so a credential the cut sliced in half matched nothing and its surviving prefix was ordinary text. The fixed overlap made that need a secret longer than the window, and nothing caps a configured header, URL, environment or stored token value, so it was a configuration away rather than impossible. A server can also spend the raw budget on control sequences that later vanish, putting the start of the credential right at the cut and its prefix at the top of the panel. Only the final cut can split anything, so the fix looks at the tail alone and drops any run that begins a configured secret. It costs one comparison per secret against a bounded window and does not care how long the secret is, which is the property a wider overlap could never give. credentialCandidates walked every suffix after every space or colon and kept them all, so a delimiter-heavy value produced thousands of candidates and RedactString ran a replacement pass for each. That expansion is on the config side, outside the raw-error bound, so the cost did not depend on the server's error being long: a value with 4000 delimiters yielded 7999 candidates however short the failure was. Input size and candidate count are bounded now. The value itself is still redacted whole; only the suffix enumeration is dropped, and the tails exist for one narrow case, a header configured as "Bearer <token>" whose server echoes only the token. And the path was treated as an identifier while query and userinfo were treated as secret-bearing. The configuration contract accepts an arbitrary HTTP or SSE path and opaque path-segment credentials are an ordinary endpoint convention. This needs no crafted response body: a failing http.Client.Do returns a *url.Error carrying the request URL, which the failed-server path wraps and renders, so the token reached the reason, the panel and the transcript, with the target row showing it too. Opaque segments are collected for redaction and replaced in the displayed target, by the same length floor used elsewhere, so a route like /v1/sse survives and the operator can still tell what failed. One note on the first test I wrote for the oversized case: it used an "sk-live-" prefix, which the generic patterns catch whatever the bound does, so it passed with the fix removed and proved nothing. The fixture is opaque now and fails with a 2800-character prefix reaching the panel.
…open Five leaks, four of them the same mistake in different places: a rule that was sized to a constant, or to a heuristic, instead of to the thing it was guarding. The tail repair inspected a fixed 4 KiB at the end of the rendered text, so a credential beginning before that window could never be matched: the inspected span starts partway through it, and a middle is not a prefix. Measured here, a 6000-byte value positioned across the cut left 5000 of its bytes on the panel. The search is now sized to the credential and answered in one KMP pass, so the work is linear in the operator's own configured value rather than in anything the remote server sent. The flat eight-byte floor went with it: seven bytes of an eight-byte credential is the credential, so the rule is proportional as well as absolute. Values already known by provenance to be secret, an OAuth client secret and the value of a credential-bearing flag, were routed through the ambiguity heuristic that exists to keep v=1 and mode=sse readable, and were discarded for being short. They skip it now; genuinely ambiguous values still do not. The OAuth endpoints were outside the candidate set entirely, although a refresh posts to TokenEndpoint during startup and a dial failure comes back wrapped in a url.Error that keeps the path and query. The collector and the target row each derived the accepted header spellings separately and neither recognised the conventional attached form, so the value was missing from the redaction set and printed verbatim one row below. Both go through one parser now. And the retained startup failure kept the raw error, re-redacted on every render from whatever the token store held at that moment, so logging out deleted the bearer that was hiding itself and the next render wrote it into the panel and the transcript. The observation now carries a fingerprint of the material that made it safe, and withholds the reason rather than re-deriving a weaker one. A fingerprint, not a copy: a second plaintext store would be its own problem.
Three findings with one shape: a classification made after the evidence for it had already been discarded. Header, environment and query values were flattened into bare strings before the readability heuristic ran, so a value under a key that names it as a credential went through the floor that exists to keep mode=sse and v=1 readable. API_KEY=s3cr3t contributed no exact candidate, and a child echoing the value on its own reached the panel and the transcript where generic shape matching has nothing to recognise. Keys now travel with their values to the decision, and the endpoint parser returns the key-classified parts separately from the ambiguous ones. The userinfo password is classified by position, since no key names it. The Target row recognised a flag packed with its value as sensitive, printed the whole element verbatim, and redacted the NEXT argument instead, so the row under the redacted reason carried the credential and blanked an unrelated flag. The display now parses the packed form the way the collector already did. And raw.Auth was being treated as credential material although it is the public authentication MODE selector, whose only accepted value is the word the panel itself displays. Every failure from the OAuth stack lost the token naming the subsystem: "oauth: fetch authorization server metadata" became "[REDACTED]: fetch authorization server metadata". That was invisible while ambiguous values ran through the length floor, which discarded a five-character string on its own; removing the floor for known provenance is what surfaced it, which is the tell that the field was miscategorised rather than the floor load-bearing.
5dbf86f to
99cfbb9
Compare
…reached Startup now splits MCP into a critical set registered before the TUI launches and an optional set, the unconfigured built-in defaults, registered on a background goroutine. The critical branch only runs when something is configured, and this test stubbed registration without configuring any server, so after the rebase the stub was never called and the assertion failed against a nil list. It stubs resolveMCPConfig with a configured server now, which is what puts the failure in the half startup registers synchronously and hands to the TUI. Worth recording what this does NOT yet cover: an unconfigured default that fails is registered asynchronously, so its skipped entry does not exist when the model is constructed and never reaches the panel. That is the same observation-timing boundary as the outstanding review finding about binding the redaction context at capture, and it is fixed there rather than here.
…observed with Two structural problems behind the panel's redaction, both about identity. The runtime name is an identity, so it has to be unique. Registration trims the config key, so "docs" and " docs" were two configured entries and one runtime server: they shared a tool count and a failure, map iteration decided which configuration survived, and each row redacted that shared error with its own candidate set, so the row that did not fail could print the other's credential. NormalizeConfig now refuses two names that resolve to one identity, and the config writer refuses a key that collides with an existing one, since validation on the way in only sees the incoming server and cannot detect the collision. A single padded name still works; trimming was never the problem. The context that makes an error safe has to be recorded where the error is produced. A skipped entry keeps the raw failure and is redacted at display time against whatever the token store holds then, so the surface needs to know whether that set is still the one that was hiding the credential. It was sampled when the surface was built, which is after registration and after anything in between could have rotated the store, and a 401 during connect refreshes the bearer that the same attempt's error text quotes. SkippedServer now carries a fingerprint sampled before connecting, and the panel prefers it over its own sample. Also: optional servers register on a background goroutine, and the runtime wrapper returned nil for its skipped list unconditionally. Moving them off the critical path is a scheduling decision, not a visibility one, so every one of them rendered from configuration alone: enabled, unexplained, for a server that never connected. The panel now pulls those failures and refreshes when one arrives.
…up split Startup separates unconfigured built-in defaults from the servers the user asked for, and the two halves are normalized by separate calls. A collision that straddles the split is invisible to both: a user-configured " exa" is critical while the built-in "exa" is optional, and they are one runtime server with two panel rows sharing a failure, a tool count, and each other's redaction context. The check moves out of NormalizeConfig into ValidateUniqueNames, which NormalizeConfig still calls, and startup runs it on the merged configuration before splitting.
|
All five are in, plus two things the first two turned up. Packed sensitive arguments in the Target row. The collector and the renderer share one flag classification now, so a value classified as sensitive is absent from the rendered row in all three spellings (separate, One canonical server per observation. Two keys resolving to one runtime name are refused with an error naming both spellings. A single padded name still works, and a disabled entry claims no identity. Two places needed it rather than one: The length floor and provenance. Header, environment and query values keep their key classification through candidate construction, so a value under a key that names it as credential material bypasses the floor at any length while ordinary short values like Capture the context where the failure is observed. The public oauth selector. One extra that fell out of the capture work: the optional-MCP wrapper returned Each guard added this round was checked by reverting it and confirming the test fails naming the missing thing. |
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.
Findings
-
[P2] Deliver optional-startup completion to an open MCP manager
internal/cli/mcp_startup.go:59
The new late-failure path is pull-only: optional startup stores its result and closes its completion channel, while the TUI reads late skipped observations only when it next builds MCP view state. If a user opens the MCP manager while an optional server is still connecting, the overlay caches the configuration-derived enabled state. When registration later fails while Bubble Tea is idle, no completion message, callback, timer, or other update causes the model to render again. The already-open overlay therefore remains enabled until unrelated input or a resize happens to redraw it, despite the skipped failure being available.Please repair the completion-to-view lifecycle rather than only adding another cache condition. Optional startup should notify the running TUI/model when it completes, and the handler should invalidate or rebuild MCP view state so the active manager consumes the completed observation. Preserve the non-blocking startup path, existing tool-readiness behavior, and the policy that unconfigured built-in defaults do not produce startup warnings. Add an end-to-end regression that opens the manager before optional startup completes, completes it with a skipped server, processes the completion notification without user input, and confirms the visible overlay changes to failed and displays its reason.
Review guidance
This review has needed repeated follow-up because the feature crosses several independent contracts: asynchronous MCP registration, CLI-to-TUI handoff, cached view state, terminal-safe rendering, transcript persistence, configuration mutation, token rotation, and redaction provenance. The difficult failures have consistently appeared at boundaries between those contracts, where a local fix made one surface correct but did not prove the same behavior through a second entry point or lifecycle stage.
For follow-up work, please approach this as one end-to-end state machine rather than as isolated rendering or sanitizer changes:
- Enumerate each failure observation from creation through every consumer: critical startup, optional startup, manager overlay, slash-command/transcript rendering, config edits, token refresh/logout, and shutdown.
- For every asynchronous producer, identify the concrete event that makes an already-visible consumer refresh. A getter or cache-invalidating condition is not sufficient if nothing schedules another model update.
- Keep security transformations aligned across all outputs. Any value displayed in an error, target row, warning, overlay, or persisted transcript needs the same provenance-aware handling; test each relevant output rather than only the helper.
- Prefer regression tests that model the real ordering boundary: render before completion, complete asynchronously, deliver the notification, then assert the existing surface changes without incidental keyboard, resize, or command activity.
- Before requesting another review, run a focused matrix for each changed contract across create/apply, async completion, render/consume, configuration replacement, credential rotation or deletion, and cleanup. Include negative cases that would pass if the observation, redaction context, or invalidation signal were omitted.
That process should reduce follow-up churn by validating the full producer-to-consumer contract before individual edge cases become review comments.
…mpletes Optional MCP registration runs on its own goroutine so a slow server cannot delay the first response, which means its result arrives with no user input behind it. Bubble Tea renders only in response to a message, so reporting late failures through a getter was not enough on its own: a manager opened while an optional server was still connecting kept showing the configuration-derived enabled state until unrelated input or a resize happened to redraw it. The startup's completion channel is now surfaced to the model, Init schedules a wait on it, and the resulting message rebuilds the MCP view state. Startup stays non-blocking, tool readiness is untouched, and unconfigured built-in defaults still produce no startup warning. The regression asserts on the rebuilt cache rather than on a render. Rendering calls mcpViewState, which invalidates on demand, so an overlay drawn after the message looks correct even when the handler does nothing: the first version of this test passed with the rebuild deleted, which is the failure mode it exists to catch.
|
Fixed. You were right that a getter is not a delivery mechanism. The optional startup's completion channel is surfaced to the model, The interesting part is the test, and it is worth telling you because my first version was wrong in the way this PR keeps being wrong. I wrote the regression to open the manager, complete startup with a skipped server, deliver the message with no key or resize, and assert the rendered overlay now says failed. It passed. Then I deleted the rebuild from the handler and it still passed, because rendering goes through It asserts on the rebuilt cache now, before anything renders, which is the thing that is actually new. Deleting the rebuild fails it: and a second test pins that So both halves are covered independently: the message gets produced, and the message rebuilds. Neither passes on the other's behalf. On your guidance about approaching this as one state machine rather than isolated fixes: that is a fair description of how this PR has gone, and the pull-versus-push distinction is the clearest example of it. I had built something that answered correctly whenever it was asked and never arranged for anyone to ask.
|
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.
Findings
-
[P2] Keep the enabled server’s failure when a disabled alias is present
internal/tui/mcp_skipped_invalidation.go:70
ValidateUniqueNamesintentionally accepts a configuration containing enableddocsand disabled" docs": disabled entries do not claim a runtime identity during registration. However, the newcanonicalMCPServershelper copies both raw entries into a map keyed bystrings.TrimSpace(name). Since Go map iteration is randomized, the disabled and enabled configurations race to become thedocsvalue independently in the before and after snapshots used byretainedMCPSkipped. A normal/mcpconfig operation that leaves the enabled entry unchanged can therefore compare it against the disabled alias, discard its startup observation, and make/mcpreport the still-unavailable server as enabled.Please make observation aging operate on the same active-server set as registration: exclude disabled entries before canonicalizing (or otherwise deterministically retain the enabled entry for a canonical name). This should preserve the intended disabled-wins display behavior and support for a single padded name; the important invariant is that an enabled server’s retained failure is not affected by a disabled alias that does not participate in startup.
-
[P2] Do not let an arbitrary configured secret bypass the failure-render work bound
internal/tui/mcp_state.go:206
The new raw-error cap bounds the server-controlled message, but the new tail-repair pass runs after that cap and is driven by every configured/stored secret. On a truncated failure,dropTrailingSecretPrefixcallslongestPrefixSuffixfor each candidate.credentialCandidatesintentionally preserves an oversized value as a whole, andlongestPrefixSuffixconstructspattern + sentinel + textplus an[]intsized to the complete pattern. Headers, environment values, URL components, OAuth credentials, and token-store values have no corresponding size limit, so a failed server with a multi-megabyte configured value performs multi-megabyte allocation and scanning on every/mcpstate rebuild despite the nominal fixed failure-render budget; several values multiply that cost. The existing entry-point test already reaches this path with a 2 MiB URL value, but only checks retained output length.Please bound the root cause—the work performed per candidate—not just the rendered error. Tail-prefix suppression needs a fixed memory/CPU budget independent of configured secret length, while still removing a meaningful prefix that a truncation could disclose. A bounded/streaming comparison or an explicitly bounded tail-repair representation would work; retain regression coverage that uses an oversized configured secret and asserts resource behavior as well as no prefix disclosure.
Overall guidance
This PR has had repeated follow-up findings because it is not only a UI-state change. It introduces a new cross-cutting contract: a startup-time MCP failure, its identity, its credential context, and its rendered explanation must remain correct and safe while configuration, registration state, and token material change independently. The implementation has addressed individual symptoms carefully, but the remaining defects come from places where that contract is reconstructed from different representations later in the lifecycle.
Before another revision, it would help to treat the feature as one state machine and audit it by invariant rather than by individual rendering path:
-
Use one canonical active-server identity at every join. Registration, skipped observations, config mutation, cache invalidation, and rendering should agree on which configured entry represents a live runtime server. Disabled entries are intentionally outside registration, so they should not influence the identity used to retain an observation about an enabled entry. Exercise enabled/disabled aliases, whitespace-normalized names, add/remove/replace operations, and both critical and optional startup in the same lifecycle tests.
-
Separate the disclosure budget from every input that can influence work. The remote failure string is bounded, but the current repair step still lets configuration and token-store values determine work and allocation. Establish an explicit upper bound for total candidate count, candidate bytes examined, temporary allocations, and tail-repair work per render. Apply it consistently to headers, env, args, URLs, OAuth fields, and stored tokens—not only to the final displayed text. Keep a regression that combines a truncated hostile error with oversized configured values and verifies both no visible secret prefix and bounded execution/allocation.
-
Test transitions, not only snapshots. The most valuable tests for this feature should start with a failed registration, then mutate exactly one relevant input—disable/enable, replace endpoint, rotate/logout credentials, optional startup completion, or token-store read failure—and assert the resulting state, reason visibility, and transcript/overlay output. A test that only constructs the final view can miss mismatched identities or stale observation state.
-
Keep the same safety policy at every consumer.
/mcplist output, the manager overlay, cached view state, transcript persistence, and startup diagnostics all consume variants of the same failure. When adding a new field or safeguard, trace producer → retained observation → every consumer, and decide whether each consumer needs the original reason, a redacted reason, or an explicit withheld-state message. This makes omissions visible before they become a sequence of narrow follow-ups.
The goal is not a broader refactor or to revisit resolved feedback. It is to make the newly introduced failure-observation contract explicit and bounded, so the present fixes do not require further point-by-point review cycles.
…alias ValidateUniqueNames deliberately accepts an enabled "docs" alongside a disabled " docs": a disabled entry claims no runtime identity during registration. canonicalMCPServers copied both into a map keyed by the trimmed name, so they collided, and Go randomises map iteration. The before and after snapshots each picked a winner independently, so an unrelated /mcp operation that left the enabled entry untouched could compare it against the disabled alias, find them different, and discard the failure. A server that was still unavailable then showed as fine, on roughly one run in five. Exclude disabled entries, the way NormalizeConfig does when it decides what to register, so aging operates on the same active-server set. A canonical name claimed by two ENABLED entries is ambiguous rather than arbitrary: ValidateUniqueNames rejects that config, but if it arrives there is no way to say which entry an observation was about, so the name is dropped and the observation ages out with it.
dropTrailingSecretPrefix runs on every /mcp state rebuild, once per configured or stored credential, and longestPrefixSuffix built pattern+sentinel+text with an []int over the whole thing. The candidate's length therefore drove the work, and configured headers, env values, URL components, OAuth fields and token-store values have no size limit. Three 2 MiB candidates measured 54.5 MiB and 33ms on every rebuild, against a render budget that is nominally fixed. The raw-error cap bounds the server-controlled message but not this pass, so a bound sized to the attacker's input was no bound. The longest prefix of the candidate that is a suffix of the text can be at most len(text) long, so any longer prefix is unreachable and scanning it changes no answer. Truncate the pattern to the text length before the search. The same three candidates now measure under 1 MiB. A differential test checks the truncated search against the definition across many inputs so the bound cannot change a result, and it catches a one-byte-wrong bound.
|
Both fixed in d88cc0f and 4c0db19, and I re-verified them on the current head rather than trusting my own commit messages. The disabled alias
A canonical name claimed by two ENABLED entries is a different case: ambiguous rather than arbitrary. Falsified by putting the disabled entry back: the regression fails, and it fails on a subset of runs rather than all of them, which is the map-iteration randomness the bug depended on. It reproduced at about one run in five before the fix. The tail-repair boundThe candidate's length drove the work. The longest prefix of the candidate that is a suffix of the text is at most Worth being explicit about the coverage, because one test alone would have been misleading. The differential test checks the truncated search against the definition across many inputs, and it catches a one-byte-wrong bound. It does NOT catch the bound being deleted, and it cannot: removing it returns identical answers, which is the whole point. Deleting the truncation is caught by the allocation test instead: So correctness of the bound and presence of the bound are pinned by different tests, and I checked both by reverting each independently. A third test keeps the visible behaviour honest, since a bound that silently stopped removing a disclosed prefix would also pass the other two.
|
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.
Overall guidance
The remaining findings are not three unrelated corner cases. This change crosses two boundaries that currently use different definitions of the same data:
- An MCP server has both an exact configuration identity (the raw map key used to update or remove it) and a canonical runtime identity (the trimmed name used by registration, skipped observations, tool ownership, and tool counts). The current implementation sometimes stores only the canonical name and elsewhere reimplements collision rules from raw keys. That makes each individual consumer look locally correct while the end-to-end config → runtime → view → action round trip is inconsistent.
- A skipped-server error is remote-controlled text that is now rendered and persisted. The redaction code correctly recognizes that a known composite credential may be echoed in a derived spelling, but its work bounds can currently change which spellings receive protection. A resource bound must limit computation without weakening the confidentiality invariant at the threshold.
Please address those invariants centrally rather than adding another special case at each failing call site. For identity, carry the raw config key and canonical runtime name as distinct values, and use one shared definition of whether a server claims an active runtime identity across validation, writes, observation matching, view construction, and manager actions. For failure safety, define one bounded candidate policy in which every value already classified as a credential retains protection for the supported whole and derived spellings regardless of size.
The regression matrix should exercise the public behavior on both sides of each boundary: a single padded key; an enabled key plus a disabled canonical alias; two enabled aliases; add/update/enable/disable/remove/check through the manager and CLI; and composite credentials just below, at, and above the 8 KiB boundary when the server echoes the whole value or only its token suffix. Testing those cases through the assembled view/action and config-write paths—not only the normalization and candidate helpers—should prevent another locally correct fix from leaving a downstream consumer inconsistent.
Merge readiness
- [P1] Rebase onto current
mainbefore merge
AGENTS.md:75
This head is based on27b319ca, while the live target iseeea3308. The synthetic merge is conflict-free, but the repository explicitly treats a stale review base as a hard blocker. Please rebase onto the current target and rerun the required validation against the combined tree.
Findings
-
[P1] Keep derived tokens protected past the candidate input bound
internal/tui/mcp_state.go:1104
addKnownrecords the whole configured value and then relies oncredentialCandidatesto add the shorter spellings a server may echo. At this branch, however, any value longer than 8 KiB returns only the whole composite. With anAuthorizationvalue ofBearer <opaque-token>, a failed server that echoes only<opaque-token>no longer matches the whole candidate; an opaque token also has no generic shape for the fallback redactor to recognize. The first 400 characters then reach both/mcprender paths and the persisted transcript. This reproduces with an 8,198-byte opaque token, while the existing oversized regression covers only a server echoing the whole configured value.The root cause is that the work bound doubles as a change in the security semantics: crossing a length threshold silently removes a supported credential spelling. Keep the candidate count and processing cost bounded, but preserve a bounded number of semantic tails for values already classified as credentials—for example, the token portion of the supported
<scheme> <credential>and<header>: <scheme> <credential>forms. Add boundary regressions that echo only the suffix atlimit-1,limit, andlimit+1; whole-value redaction alone does not cover this contract. -
[P2] Preserve the raw config key for manager actions
internal/tui/mcp_state.go:127
This assigns the trimmed runtime name to the only identity carried byMCPServerView. The manager copies that value intomcpManagerItem.Name, uses it to look the selected server up again, and passes it tocheck,enable,disable, andremove. Those CLI operations address the configuration map by exact key. As a result, a supported single key such as" docs "renders correctly as runtime serverdocs, but every action is sent for a different, nonexistent config key. With enabled"docs"plus disabled" docs"—a configurationValidateUniqueNamesdeliberately accepts—the view produces two items both nameddocs; detail lookup returns the first match, and an action selected from the disabled row can instead operate on the enabled entry.The root cause is overloading one string with three roles: display label, runtime join key, and persistence/action key. Preserve the canonical name for display, skipped-failure lookup, and tool accounting, but carry the exact raw config key (or another unambiguous row/action identity) alongside it. Manager selection and commands must use that stable identity rather than re-resolving a row by its non-unique display name. Cover both a single padded key and the enabled/disabled alias pair through the actual manager item, detail, and command-dispatch paths.
-
[P2] Match the write collision rule to the disabled-server policy
internal/cli/mcp_config.go:648
ValidateUniqueNamesskips disabled entries because registration also skips them: a disabled server claims no runtime identity. This new write-side loop sees only key spellings and rejects every trimmed collision regardless of either server's disabled state. Therefore, with disabled" docs"already configured,zero mcp add docs ...fails even though the prospective combined configuration passes runtime validation. It also rejects a normal update of exact enabled"docs"when a disabled alias is present, and the current helper cannot correctly decide the inverse case because it receives the new name but not whether the new server is disabled.The root cause is a second, weaker implementation of the active-identity rule at the write boundary. Build the prospective combined configuration and validate it with the same shared rule used by the read/startup path, or make the collision helper consume both existing and incoming server state and the same
claims active identitypredicate. The resulting invariant should be: two enabled keys with the same canonical name are rejected, while a disabled entry on either side does not block the active server that replaces it. Add write-path tests for both directions and for updating the enabled entry while its disabled alias remains present.
6 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