Repository navigation
fix(tui,mcp): show MCP servers that failed to start in /mcp, bound to their server and with credentials redacted - #835
Vasanthdev2004 wants to merge 35 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 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: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | The PR passes Runtime.Skipped() into the TUI and renders failed MCP servers with recorded, sanitized errors as required by issue #825. |
| Out of Scope Changes check | ✅ Passed | The changes support the linked issue by adding failure-state handling, redaction, sanitization, bounds, canonicalization, and regression tests. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 81.37% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 102 functions across 27 files. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: it shows MCP servers that failed to start in /mcp and identifies the server-scoped, credential-redacted failure display. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
fix/825-mcp-panel-shows-failures
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.
Comment @coderabbitai help to get the list of available commands.
![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.
…compactions Two blocking review findings from the draft review. Control bytes reached the terminal. redact() scrubbed secrets but not control characters, and the title and structural fields (name, toolCallId, role) skipped it entirely, so an imported title or message carrying ESC or NUL forged a picker row or corrupted a transcript line — the #835/#876 class, on strictly more attacker-influenced input. redact() now composes a stripControl pass (C0 except tab/newline, DEL, C1), and every rendered string — including the title at the import chokepoint — routes through control-stripping. The activity summary was emitted as EventCompaction, whose payload it did not satisfy. RehydrateEvents restructures the transcript around the last EventCompaction; a summary with no CompactableEvents/CompactedThroughSequence is hoisted to the front of the transcript on resume. It is now an assistant EventMessage, which still passes promptContextEvents (the resume digest) but carries none of that replay-side contract. A payload marker keeps it distinguishable from a translated turn, so filters and the digest can tell a Zero-generated summary from the foreign transcript. Also: the import-tag comment now matches ImportTag's actual output. Tests: regression coverage for both fixes, mutation-checked (removing the control strip surfaces the surviving byte; the summary type is asserted not to be EventCompaction). Existing tests updated for the new summary shape via a shared NoteEventIsSummary marker rather than the old EventCompaction type check. Origin-Session: local-13d543 | Claude Code | 2 prompts Origin-Snapshot: a939509c08a8
redact() was stripControl(RedactString(x)). RedactString matches secrets by SHAPE, and stripControl deletes a control byte without leaving a gap, which makes it a reassembler as well as a sanitizer. Running it second meant a foreign transcript could split a credential with a NUL, an ESC, a backspace, a DEL or any C1 byte, sail past the shape patterns because neither half looks like a key, and then have the halves rejoined on the way out to a picker row or a transcript line. Every recognized shape leaked that way: sk-ant-, ghp_, AKIA. The unsplit value redacted correctly, which is why it survived review twice; every existing test used unsplit values. Swapping the order fixes it. The patterns now see the same text the reader will see, which is the only text worth matching against. This is the same defect as #835, where an MCP failure reason was redacted before the terminal sanitizer rejoined its halves. The general rule is worth stating where the next person will hit it: any normalizer that removes bytes without leaving a gap has to run BEFORE whatever matches on them. The regression covers three key shapes against five splitters and fails against the old order with the intact credential in the output. It also asserts the splitter is non-empty, because a lost literal would make every Contains check vacuously true, and keeps a newline case to pin that separators which survive stripping are left alone rather than swept up with the rest. Origin-Session: local-13d543 | Claude Code | 2 prompts Origin-Snapshot: a939509c08a8
…oreign title cannot repaint the picker All four raised by CodeRabbit on ad57dd3. Two are real defects in this PR's own feature, not test issues. newSessionPicker gave up on an EMPTY local history: metas, err := m.sessionStore.ListResumable() if err != nil || len(metas) == 0 { return nil } Foreign sessions are discovered independently of the store, so the person with no Zero sessions at all — someone who just installed it and wants to carry on work another agent started — got nil before discovery ran. The import path was reachable only after they had already done by hand the thing it exists to save them. A failed read is still a reason to give up; an empty one is not. Emptiness is now decided after combining both sources, in pickerFromParts, split out so that decision is testable without a session store on disk. registry.go strips control bytes when a session is IMPORTED, and its comment names this picker row as the reason (#835/#876). But the picker lists a session BEFORE anything is imported, reading the title straight out of the other agent's transcript — so the vector that comment describes was the one path the stripping did not cover. An escape repaints the rows above, a carriage return hides the rest of the label, a NUL can truncate the row. sanitizePickerLabel drops control bytes and keeps the printable text: a title that is merely unusual must stay readable, because the row is how the user recognises their own work. t.Errorf("incomplete index entry: %+v", session) That walks the REAL store, so a failure put the user's session titles, working directories and file paths into the test output and into any log or pasted report carrying it. It now names which fields are empty, which is the whole diagnostic — the missing value is by definition not the interesting part. NOT taken from the same comment: gating the live-store tests behind an explicit opt-in. @Vasanthdev2004 asked for the opposite in the review this branch is answering — a skip "would also stop it finding anything, so I would rather have the fixture" — and the tests now only report. The data leak was the substantive half and it is fixed. It checked only that Discover and Read AGREE, which passes in two opposite worlds: both correctly refusing a path reached through a symlink, and both happily following it out of the store. Containment is now asserted directly — "sneaky" must not be listed and must not be readable — and agreement is kept afterwards, since that is what the original fast path broke. Not taken, out of scope: three findings in internal/tui/model.go, which this branch does not touch (CodeRabbit marks them "outside diff"). They look real — particularly toolResultSessionPayload persisting displayPreview for a redacted result — and deserve their own issue rather than a drive-by in a draft. Mutations: restoring the len(metas) == 0 return makes the new-user test fail; removing sanitizePickerLabel lets all four control bytes through. Pre-existing here and unrelated: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-13d543 | Claude Code | 4 prompts Origin-Snapshot: 8781ce78fdbe
…re path Seven gaps from review, in two contracts. Credential material through the failure display: - One parser finds where a flag ends and its value begins, before anything inside the value is read, and the failure collector, the Target row and the URL collector all use it. A packed "--api-key <b64>==" or an attached "-HName: <b64>==" was cut at its own padding, so the collector kept "=" and the Target row printed the body. - A positional endpoint is read as a URL. Cut at its first "=", the text in front of it named a credential, so one reader printed the userinfo and the other printed the whole element and masked the next one. - The tail of a known authentication value keeps its provenance. "Bearer s3cr3t" lost its six-byte token to the floor meant for ambiguous values. - The escaped userinfo password is known, like the decoded one. - Fragment values are collected, classified the way the Target row does. - The HTTP client reports when it cut a response body, and the display repairs a partial credential at a cut it did not make. Configuration keys and runtime names: - mcp check matches the registration outcome by the normalized runtime name, so a padded key no longer reports a failed server as reachable. - mcp enable validates the prospective state with the shared active-name rule before writing. Disable and remove are never refused. Regressions run the real producers (HTTP 500 through the network client, a stdio child that echoes its argument) and check both /mcp surfaces.
A server that normalizes base64 before echoing drops the padding, so the message carries the body while exact-value redaction only knows the padded spelling. The whole value already had its unpadded form; the forms a known value is taken apart into did not, and "Bearer <base64>==" is the usual shape. Known forms get it at any length, ambiguous ones only when what is left still passes the readability floor, and stored tokens are treated like the rest.
|
@jatmn all seven are fixed on 1. Padding taken for the boundary. There is now one parser ( Writing that regression turned up two more problems in the same area, both mine:
2. Short bearer. 3. The cut made by the HTTP reader. 4 and 5. Fragment and escaped password. Fragment pairs are collected with the classification Target already uses (known under a credential key, otherwise the floor), in raw and decoded form. The raw userinfo password is known, like the decoded one, and the username stays ambiguous. Both regressions start a real stdio child that reports what it was handed (the whole endpoint, the raw password alone, the decoded password alone, the username alone) and check the Error and Target rows separately. A bare 6. Enable. 7. Check. I reverted each fix on its own and the named test fails with the output you described: One existing test changed. Not covered: the enable check validates the file being written, the same as add, so a collision across config layers is still only caught at load. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve — and this withdraws my CHANGES_REQUESTED from 072e0366, which is what has been holding this branch alongside jatmn's approval.
Reviewed at 6cc1ec40, base 99721c76.
Withdrawing my previous block
My 2026-09-12 review said explicitly "I found no new code-level blocker" and requested changes only because the branch was 29 commits behind main. That is done: 6cc1ec40 is 0 behind origin/main, so the reason no longer exists. Changes-requested survives a push where an approval does not, so it needed a new submission rather than an edit — hence this review.
What I verified on this head
- Merged tree:
git merge origin/mainis already-up-to-date;go build ./...,go vet ./...and the fullgo test ./...are clean on macOS. - Gut-the-fix on four load-bearing behaviours — each reverted in turn,
internal/tuirerun, all four caught:- a recorded failure rendering as
failed— revert and the panel shows a server that never started as enabled - the trimmed-identity lookup (
strings.TrimSpace(rawName)) — revert to the raw map key and" docs "misses its failure entry, which is the exact defect the comment atmcp_state.go:85-97describes disabledwinning over a recorded failureredactMCPFailureReasonon the displayed reason — replacing it with a plainerr.Error()is caught, so invariant #6 is held by a test and not just by the call site
- a recorded failure rendering as
That last one is the one I most wanted to see fail, since a startup error is exactly where a path or bearer leaks into a panel.
Follow-up from my earlier pass, now closed
I had asked (non-blocking) whether a successful re-register clears the entry so the panel flips back to enabled. The observation now carries its credential fingerprint (SkippedCredentials, mcpCredentialFingerprint) and buildMCPServerViews compares against it, so a stale failure is not rendered against refreshed material. That answers it.
Not covered
macOS only. I did not exercise a real failing MCP server end-to-end, nor the Windows paths; the evidence above is the merged tree plus targeted mutation of the display logic.
|
@coderabbitai the three open threads, checked against
Both of those are test-only and I have them ready. Pushing now would dismiss both approvals, so I'm holding them. They go out with the next change this PR needs, or right after merge if there isn't one. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git rev-parse --short HEAD
fd 'mcp_(raw_bound|credential_boundary|error_redaction|failure_redaction)_test\.go$|mcp_state\.go$|mcp_view\.go$' .
ast-grep outline internal/tui/mcp_raw_bound_test.go --match 'TestASecretStraddlingTheBound|Test' --view expanded
ast-grep outline internal/tui/mcp_credential_boundary_test.go --match 'TestNoCredentialPrefixSurvivesTheRawBound' --view expanded
rg -n -C 5 'TestASecretStraddlingTheBound|TestNoCredentialPrefixSurvivesTheRawBound|TestShortConfiguredValuesDoNotShredTheMessage|0090|longestPrefixSuffix|maxMCPReasonRawLen|maxMCPSecretMatchWindow' internal/tui/mcp_raw_bound_test.go internal/tui/mcp_credential_boundary_test.go internal/tui/mcp_error_redaction_test.go internal/tui/mcp_failure_redaction_test.go internal/tui/mcp_state.go internal/tui/mcp_view.goLength of output: 13997 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '43,84p' internal/tui/mcp_raw_bound_test.go
sed -n '1,108p' internal/tui/mcp_credential_boundary_test.go
sed -n '65,93p' internal/tui/mcp_error_redaction_test.go
rg -n -C 7 'C1|U\+0090|\\u0090|\\x90|TestFailedServerReasonRedactsASecretSplitByControlBytes' internal/tui/mcp_failure_redaction_test.go
sed -n '195,233p' internal/tui/mcp_state.go
sed -n '265,291p' internal/tui/mcp_state.go
rg -n -C 3 'dropTrailingSecretPrefix|boundMCPFailureError|stripTerminalRejoiners' internal/tui/mcp_state.goLength of output: 14035
The other two test findings remain open. Thanks for the precise boundary explanation. ✏️ Learnings added
You are interacting with an AI system. |
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
The failure reasons come from the server and pass the redaction helper before rendering (with dedicated tests for authorization-header echo, oversized secrets, and path credentials) plus truncation bounds, and disabled wins over failed in the display. Scope note: this bundles credential fingerprints, an OAuth store, and identity work well beyond a fix(tui) title, though it arrives with about 34 test files.
Vasanthdev2004
changed the title
|
@anandh8x your 3 August change request is still the block here. The terminal-sanitization finding was fixed in c7421d9 two days later, and your payload is pinned by |
…ws-failures # Conflicts: # internal/tui/command_views.go # internal/tui/mcp_state.go
|
@jatmn @gnanam1990 this push dismissed your approvals, sorry. It merges main at 2ffe183 and renames this branch's imports to the Twigpine module path from #1105, which the branch needed before it could build on main. The only conflicts were the import blocks in |
…the C1 fixture TestShortConfiguredValuesDoNotShredTheMessage checked only that "connection refused" survived, so it passed just as well with the 1 and the true redacted, which are the values it exists to protect. It now compares the whole message. The C1 control fixture held a raw U+0090, which staticcheck reports as ST1018. It is written as an escape now; the input is the same.
|
One more push on top of the rename: 761fbd3 carries the two CodeRabbit test fixes I had held since September so as not to dismiss approvals. The short-value test now compares the whole message, and the C1 splitter is written as an escape sequence instead of a raw invisible character. Test-only; |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked this PR on head 27218dc (full diff, focused tests, blind search). One code item remains before I’d approve; the rest of what I raised on 42a95b9 (redaction pipeline, arg boundaries, manager ConfigKey, optional completion, collision/enable paths, etc.) is addressed on current head or is explicitly out of scope for #825.
Merge readiness
- Branch is not behind
origin/main; required CI on27218dcis green. - My CHANGES_REQUESTED on
42a95b9is stale relative to this head—treat it as superseded by this note once the finding below is fixed (or tell me if you want any prior thread item re-opened with a specific repro on27218dc).
🟡 P2 — Use each row’s ConfigKey in transcript MCP action hints
📍 Where: internal/tui/mcp_view.go (mcpServerActionLine, consumed from mcpManagerServerLines in the /mcp transcript text).
💥 What fails: The panel lists suggested CLI commands with the canonical runtime name, not the configuration map key:
actions := []string{"zero mcp check " + name} // name = strings.TrimSpace(server.Name)For a server stored under a padded key such as " docs ", the row displays as docs but zero mcp check (and siblings) look up cfg.Servers[serverName] by exact key. The manager overlay already dispatches check/disable/enable/remove with ConfigKey; copying the transcript hint runs the wrong command or reports “not configured” while the panel still shows a failed server.
🔎 Root cause: This PR split display identity (Name, trimmed runtime) from action identity (ConfigKey, exact map key) and wired ConfigKey through the manager, but the transcript action line still builds commands from Name only—the last in-diff operator surface missing from the same identity contract you already applied in mcp_manager.go.
📜 Stated contract:
ConfigKey is the EXACT key in the configuration map, untrimmed. Actions address the config by that key, so it is the only identity that can select one row when a padded alias renders under the same canonical name.
🏷️ Attribution: Incomplete fix on this PR at 27218dc. Misleading hints for padded keys existed at merge-base too; what’s new is the documented ConfigKey rule and manager compliance—transcript hints need to match in the same pass.
📌 In this PR:
internal/tui/mcp_view.go—mcpServerActionLine/mcpManagerServerLinestranscript actions (must useConfigKey)internal/tui/mcp_manager.go— already correct (do not regress)
🔒 Unchanged on main: CLI key lookup; stderr / mcp check redaction (still generic redaction.ErrorMessage as before this PR’s panel work).
🔧 Required correction: Build every zero mcp … hint in mcpServerActionLine from server.ConfigKey (shell-quote when the key contains whitespace or shell metacharacters). Add a regression test with a padded key that the action line includes the exact map key, not only the trimmed display name.
🛠️ Author fix: Fix transcript action hints and the test in one pass; leave manager dispatch, CLI, registration, stderr, and mcp check redaction alone unless you’re deliberately expanding scope.
🚫 Out of scope for this round: Panel-grade redaction on stderr / mcp check / ACP; optional MCP “enabled” while background registration runs; view cache keyed on late-failure count only; in-session re-register after config edits (per your PR note).
The commands printed under each server in the /mcp transcript were built from the trimmed display name, while check, enable, disable and remove look the server up by its exact config key. For a padded key such as " docs " the hint reported "not configured", and for an alias pair it addressed the other entry. The manager overlay already dispatches with ConfigKey; the transcript hints now use it too. A key with space padding is double-quoted, which keeps the spaces in POSIX shells, PowerShell, cmd and /mcp. Padding that cannot be typed back from a terminal line, such as a tab, gets no key-addressed hint. oauth login keeps the trimmed name, because it matches the normalized server list rather than the config map.
|
@jatmn fixed in 3a9dcb0, plus a merge of main. The check, enable, disable and remove hints in the transcript now use Two cases don't use the key:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found four issues that need to be addressed before this is ready. The latest transcript action-hint fix is correct, including the author's distinction between exact configuration keys and OAuth's normalized names. I also verified the explanations about disabled aliases, the overlap-versus-cut tests, and the deliberately excluded reconnect/stderr behavior.
Reviewed head 2c36c67ac8be29ff13b59b18f580ac656b9cb6e0 against merge-base and live main 4247a07aba0f89c3c2597314a33edaede56be70c.
Merge readiness
The branch is mergeable and 0 commits behind main; no rebase or release-metadata conflict was found. All 12 executed checks succeeded; CodeRabbit was skipped. GitHub reports BLOCKED / CHANGES_REQUESTED, so fresh review approval is needed after the fixes. The closed #822 diagnostic issue is complementary, and merged #1113 supplies the OAuth fields involved below; neither supersedes #835.
Findings
🟠 P1 — Include supported runtime credentials in the new failure-redaction input
📍 Where: internal/mcp/oauth_store.go:286-294 (SecretValues) and internal/tui/mcp_state.go:915-921 (environment collection).
💥 What fails: Three real startup paths disclose an opaque credential in both the transcript panel text and manager detail:
- With
auth: oauthand no configured OAuth block, a stored dynamically registeredClientSecretis submitted on refresh. A token endpoint'serror_descriptionecho is displayed verbatim. - A credential in the discovered/stored
TokenEndpointis likewise absent from the new redaction input. A real failing token POST that echoes itsworkspacequery value exposes it. - A stdio child can inherit
COMPANY_TOKENfrom the launching environment, as the documented secret-manager setup permits. When the child reports that value while failing initialization, the new reason displays it even though Target contains no credential.
I reproduced the last case with a real child through the existing execution adapter's supported degraded path; its environment scrubber leaves this unlisted variable available. All three corresponding display probes pass at the merge base, where startup error text is not rendered.
🔎 Root cause: The new safety input is reconstructed from configured fields and only part of stored OAuth material:
values = append(values, stored.AccessToken, stored.RefreshToken)
// The configured-environment collector separately visits only raw.Env.It misses accessible credentials that the supported startup paths actually use. The token-context fingerprints also receive the incomplete stored set.
📜 Stated contract:
“Redact success and error paths (including stderr).” —
AGENTS.md
“ClientSecret is sensitive.” — the existing
StoredTokencontract
“A secret manager that injects the env var your MCP server reads on its own (the command and args then run inside that environment).” —
docs/EXTENDING.md
🏷️ Attribution: PR-introduced disclosure. Stored discovery fields and inherited environment already exist on base/main, but the newly added failure-reason path exposes them. This is not a request to repair the older stderr policy.
📌 In this PR:
| Surface | Current result |
|---|---|
oauth_store.go:SecretValues and its new tests |
Access/refresh tokens covered; stored client secret and token-endpoint credential components missing |
mcp_state.go:mcpServerSecretValues |
Configured headers/env/args/URL/OAuth covered; inherited sensitive stdio variables missing |
registry.go, cli/app.go, tui/model.go, command_views.go |
Capture/current-context handling consumes the same incomplete credential inputs |
mcp_view.go and mcp_manager.go |
Both consume the resulting unsafe reason; shared input correction should protect both |
🔒 Unchanged on main: Login, refresh, environment merging/scrubbing, and token persistence already implement these startup sources. Public client IDs, scopes, expiry, token types and ordinary environment metadata are not credentials.
🔧 Required correction: Extend the PR's new safety-input collection to cover the stored client secret, credential-bearing stored token-endpoint components using the existing URL rules, and inherited credential-named stdio variables such as COMPANY_TOKEN. Keep capture/current context consistent for mutable stored material. Preserve read-only enumeration, existing bounds, ordinary metadata and current configured-value protection. Verify real refresh/child failures through both displays, including relevant credential-change suppression.
🛠️ Author fix: Close the missing-input rule across every row above in one pass, including source comments and regression fixtures. Do not fix only the OAuth example or only the environment example; do not require a new observation architecture or plaintext credential store.
🚫 Out of scope: OAuth protocol changes, sandbox environment policy, introspecting secrets created inside arbitrary wrapper scripts, unrelated logging and reconnect behavior.
🟠 P1 — Preserve query credential provenance across raw and decoded spellings
📍 Where: internal/tui/mcp_state.go:1103-1134 (mcpURLSecretValues).
💥 What fails: Real HTTP registration failures expose configured URL credentials in both displays:
?api%5Fkey=%73crtfollowed by a value-only echo of%73crt: Target recognizes the decoded sensitive key and masks it, but Error prints the recoverable raw credential spelling.?workspace=opaque%2Dworkspace%2D9f3c2b7&filter=a;bfollowed by a decoded-value echo: the unrelated semicolon error causes the valid decoded workspace candidate to be discarded. An unrelated invalid escape has the same effect. A normal valid-query control is protected.
🔎 Root cause: The raw pass classifies an undecoded key, while the decoded pass is discarded wholesale on any query parse error. Raw and decoded representations therefore lose either their credential classification or their membership in the candidate set.
if isSensitiveMCPDisplayKey(rawKey) { /* ... */ }
query, err := url.ParseQuery(parsed.RawQuery)
if err != nil {
return known, values
}📜 Stated contract:
“Redact success and error paths (including stderr).” —
AGENTS.md
The existing TestShortValuesUnderSensitiveKeysAreRedacted and TestPercentEncodedEndpointCredentialsAreRedacted assert the specific supported contracts: known credentials bypass the readability floor, and configured escaped/decoded URL values remain protected.
🏷️ Attribution: PR-introduced. The collector and rendered failure reason are new; identical real transport/display probes do not expose these values at base/live main.
📌 In this PR:
| Surface | Current result |
|---|---|
| Main server URL | Both failures reproduced |
| Positional/separate/packed URL-valued stdio arguments | Same mcpURLSecretValues collector |
| Configured OAuth token, authorization, registration and issuer URLs | Same collector |
| Target query renderer, fragment collector and raw userinfo-password handling | Already retain the relevant decoded-key/per-value provenance; preserve these |
| New URL/provenance/assembled-state tests | Cover valid escaped values, but not escaped short-value keys or a valid pair beside a parse error |
🔒 Unchanged on main: URL admission and generic redaction. The accepted URL loader does not reject these query strings; this finding does not require changing that policy.
🔧 Required correction: Classify each raw query value using its decoded key, retaining both supported value spellings with that classification. Preserve independently decodable pairs when another pair fails. Use the corrected shared helper for every listed caller, with regression controls for ordinary short metadata and all demonstrated combinations.
🛠️ Author fix: Fix the query extraction rule and all listed caller coverage together. Do not patch one parameter name or one URL source; do not rewrite core URL validation or generic redaction.
🚫 Out of scope: New encoding schemes, broad configuration rejection rules and unrelated CLI output.
🟠 P1 — Mark interrupted HTTP error details as incomplete
📍 Where: internal/mcp/network_client.go:643-646 (httpStatusError).
💥 What fails: An HTTP or SSE server returns 500 with Content-Length: 2048, sends only the first 24 bytes of a configured 32-byte opaque credential, then closes. The real client retains those bytes, but ErrorDetailTruncated is false. Both new failure displays show Qw7ZmPr4aBcD9eFgH2jKlM6n, 24 of the 32 secret bytes. Complete-body controls remain protected.
🔎 Root cause: The new incompleteness signal recognizes only the size cutoff and discards the body-read failure:
body, _ := io.ReadAll(io.LimitReader(response.Body, maxHTTPErrorDetail+1))
truncated := len(body) > maxHTTPErrorDetailA short partial body cannot match the complete configured credential, and neither this false marker nor the local 20 KiB bound activates the existing tail repair.
📜 Stated contract:
“Redact success and error paths (including stderr).” —
AGENTS.md
The existing TestCredentialPrefixCutByTheHTTPReaderIsNotDisplayed protects credential prefixes cut at the producer; this is an observable interrupted-read variant of that same boundary.
🏷️ Attribution: PR-activated disclosure. Base/main already discard the read error, but do not render this startup body in the MCP views. The PR adds both the reason consumers and the incomplete producer marker. Identical HTTP/SSE display probes pass at base.
📌 In this PR:
httpStatusError: shared by HTTP POST, SSE stream GET and SSE POST.ErrorDetailTruncated/redactMCPFailureReason: existing new consumer correctly repairs marked partial text.- Both reason renderers: affected through shared state.
- New producer/truncation tests: size-cut and complete-body cases covered; read-error case absent.
🔒 Unchanged on main: Network transport architecture and old stderr/check behavior.
🔧 Required correction: Treat a failed body read as incomplete, or withhold its partial detail while retaining useful status context. Preserve byte bounds and complete-response behavior. If reusing the flag, keep size slicing dependent on actual body length so a short failed read cannot panic. Add interrupted-response regressions through real HTTP/SSE registration and both consumers.
🛠️ Author fix: Close the shared producer incompleteness rule for every listed route in one pass. Reuse the existing marker/tail handling; do not replace the networking framework or expand this into arbitrary partial-string recognition.
🚫 Out of scope: Detecting prefixes deliberately authored as complete error messages or changing unrelated error-output policy.
🟡 P2 — Isolate the new startup-wiring test from user token stores
📍 Where: internal/cli/app_mcp_skipped_test.go:31-68.
💥 What fails: The test leaves newMCPTokenStore at its production default. Running this exact new test with simulated user ZERO_MCP_OAUTH_TOKENS_PATH / ZERO_OAUTH_TOKENS_PATH values migrated the seeded legacy token, renamed its original file and created the unified store. On Windows, the helper sets only APPDATA, while both OAuth paths fall back through HOME/USERPROFILE; an inherited keyring backend also bypasses a temporary config path.
The reproduction used only review-owned simulated user files.
🔎 Root cause: Redirecting the config root does not isolate the credential-store constructor or its migration/backend selection.
📜 Stated contract:
“Tests must not read or write the developer's real config, cache, or state directories.”
“Redirect every root the code under test resolves, not just config.” —
AGENTS.md
🏷️ Attribution: PR-introduced test defect. This test file and constructor invocation path are absent at base/live main. The old shared helper's limitations do not justify adding another uncontained caller.
📌 In this PR:
- New startup test: production token constructor and inherited backend/path settings remain active.
- New direct token-store helpers in
internal/mcpandinternal/tui: already use explicit temporary paths andEnv: map[string]string{}. - New CLI identity/check tests: non-OAuth stdio paths do not invoke this token constructor.
🔒 Unchanged on main: Credential-store and legacy-migration implementations; unrelated older tests.
🔧 Required correction: Supply an explicitly temporary file-backed token store with an isolated environment in this new test, or equivalent complete local isolation that also prevents legacy migration and host-keyring access. The existing dependency seam and store options already support this.
🛠️ Author fix: Contain every credential-store resolution reached by the new startup test in one pass, and verify seeded user paths remain untouched. Do not redesign the store or repair unrelated main tests.
🚫 Out of scope: Shared storage architecture and repository-wide test cleanup.
Needs maintainer decision
A child can interpret --api-key -Q... as a value, but the current collector intentionally treats a following dash-prefixed item as another flag; its negative test explicitly preserves --api-key --verbose. A real-child probe confirms the separated spelling can expose an opaque value, while equals/packed spellings redact it. There is no established child-independent grammar resolving that ambiguity. I am not requiring parser expansion or removal of the existing negative control in this PR; support for that separated form needs an explicit product choice.
Reviewed contract coverage
| Lifecycle / ordering | Changed entry points and siblings checked | Operator claims |
|---|---|---|
| Configure -> prospectively validate -> existing locked atomic write -> reload -> adopt/invalidate | Add, enable, disable, remove, manager actions and wizard; active/disabled aliases and exact/runtime identities | No changed markdown/help/generated operator files; quoted transcript hints now address exact keys |
| Critical/optional capture -> retain -> completion -> consume -> config/token change -> close | Connect/list/validation, snapshot/getter/channel, Init/handler/cache, both displays, replacement/disable/logout and runtime cancellation | Reconnect and pending optional labels remain the documented scope exclusions |
| Error -> bound -> normalize/redact -> partial-tail repair -> display | Configured/stored/inherited source matrix; HTTP/SSE body producer; real stdio capture | In-memory transcript exposure verified; no claim that these command rows are directly persisted to session storage |
| Shared field/source | Producer / load | Capture / consumer | Tests | Docs / renderer |
|---|---|---|---|---|
| Exact key / runtime Name | JSON, name validation, active uniqueness | Exact actions vs canonical observations/tool counts/OAuth | Padded keys, aliases, collisions, receiving CLI and hint tests | Source comments consistent; latest hint fix verified |
| Configured credential fields and URL components | Existing normalization and shared arg/URL parsing | Reason and Target classifiers | Short/escaped/split/padded/bounded fixtures plus query counterexamples | Both displays traced; no changed operator-doc claim |
| Stored access/refresh/client-secret/endpoint and inherited sensitive env | Store/login/refresh; documented env injection and launch merge/scrub | Pre-connect/current context and both reasons | Real refresh/child probes; credential-change controls inspected | F1 covers omitted inputs; public identifiers/metadata preserved |
| Partial-body signal | HTTP/SSE size/read boundary and error wrapping | Existing tail suppression | Complete, size-cut and interrupted-body probes | F3 closes incomplete producer signaling |
Malformed-input review covered empty/trailing/next flags, packed and equals boundaries, base64 padding, positional URLs, escaped query/fragment data, invalid URL fallback, independently invalid query pairs, wrong config shapes/types, missing transport requirements, invalid names/hosts/schemes, nil stores/errors, oversized values and UTF-8 cuts. The findings require local safety corrections, not new admission policy.
Validation
go build ./...andgo vet ./internal/tui ./internal/mcp ./internal/redaction ./internal/clipassed.- Affected-package tests passed for MCP, redaction and CLI. The TUI package's only failure was
TestRelativeAgeFormatsLastActivity(2025-05-31versus2025-06-01); the identical failure reproduces at base. - Focused MCP/credential tests with
-racepassed across TUI, MCP and CLI. - Review-owned overlay probes exercised real HTTP/SSE/stdio paths, both displays, immutable-base comparisons and simulated user-store migration. The reviewed checkout is unchanged.
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).What else this grew into
Review turned up more than the panel, and those fixes are all here too, so the original title undersold it (thanks @euxaristia). From the most code to the least:
TokenStore.SecretValues. Each is redacted in its plain, escaped and split forms, and the text has a fixed size bound.internal/redaction/overlapping_secrets_test.goand the MCP tests pin the forms.CredentialFingerprintof its credential values. So a renamed or colliding alias doesn't show another server's failure, and neither does a server whose credentials changed since startup.Summary by CodeRabbit
New Features
/mcppanel.Bug Fixes