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 28 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: 🟠 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. 🚥 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.
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.
The input cap returned the whole value and nothing else once a configured value crossed 8 KiB, which made a work bound into a change of security semantics. An Authorization value of "Bearer <opaque token>" longer than the cap stopped offering the bare token as a candidate, so a failed server echoing only the token matched nothing; an opaque token has no shape for the fallback redactor to catch either, and the first 400 characters reached both /mcp render paths and the persisted transcript. Measured before the fix: an 8205-byte value yields 1 candidate with the bare token absent, while the same shape at 71 bytes yields 2 with it present. The threshold alone decided whether the credential was redactable. Bound where a separator is LOOKED FOR instead of how long a value may be. Every convention this walks, "<scheme> <credential>" and "<header>: <scheme> <credential>", puts its separators in a short prefix, so scanning that prefix keeps the cost fixed while the tails survive at any length. The candidate count stays capped, and the delimiter-heavy cost regression that motivated the original bound still passes. The old oversized test asserted the defect as the contract: it required exactly one candidate for a long value. It is replaced by tests that a long credential still yields its token, that the header form yields both tails, and that the scan boundary is where it says it is.
MCPServerView carried only the canonical runtime name. That is the right spelling for display, for joining against skipped-failure records and for tool counts, and it is deliberately not unique: ValidateUniqueNames accepts an enabled "docs" beside a disabled " docs" because a disabled entry claims no runtime identity. The manager copied that name into its item, looked the selected server up again by it, and passed it to check, enable, disable and remove. Those address the configuration map by exact key. So a single padded key such as " docs " rendered correctly and sent every action to a key that does not exist, and with an alias pair both rows rendered as "docs", detail lookup returned whichever came first, and an action chosen on the disabled row could operate on the enabled entry. One string was doing three jobs: display label, runtime join key, and persistence identity. The first two are the same and stay on Name; the third is now ConfigKey, the exact map key, and every action and lookup uses it. The existing view expectations gained the new field rather than being loosened: the value they now assert is the exact key each fixture is configured under.
refuseColliding compared key spellings and rejected every trimmed collision. That is a second and weaker implementation of a rule the read and startup paths already own: ValidateUniqueNames skips disabled entries, because registration skips them and a disabled server claims no runtime identity. So the write side refused combinations the running configuration accepts. With a disabled " docs" configured, `zero mcp add docs ...` failed, and even a plain update of the enabled "docs" was refused while that alias existed. It also could not decide the inverse case at all, because it never received whether the incoming server was itself disabled. Build the prospective combined configuration and hand it to the shared rule, rather than teaching the local copy the same exceptions. The invariant is then identical on both sides by construction: two enabled keys resolving to one canonical name are refused, and a disabled entry on either side does not block the active server. Covered in both directions, including updating the enabled entry while its disabled alias remains, which is the case the old rule got wrong most quietly.
|
Rebased onto main at e1bd9ca as asked. Clean replay of the twenty-three commits, nothing resolved by hand; the MCP failure-observation, identity, redaction and overlay behaviour is unchanged. cli, mcp and redaction packages green here and linux and darwin cross-builds pass. The tui package shows one failure on this box, TestAltScreenTranscriptScrollKeepsFooterFixed, which renders the working directory into the title bar and wraps on my local path; it fails identically on main here and passes on the runners, so it is not this branch. @gnanam1990 this is the only change since your review. @jatmn your approval at 072e036 will have been dismissed by the push; the content is identical. @anandh8x your review at be58076 is a long way behind this. |
072e036 to
e1bd9ca
Compare
…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. ## The feature was invisible to the user it exists for 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. ## A foreign title reached the terminal unfiltered 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. ## The live-store test printed the developer's own sessions 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. ## The symlink test asserted the weaker half of its property 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
|
Both of your asks are done here too. @anandh8x the failure reason is sanitized before it is rendered. @gnanam1990 the rebase you asked for is done. Head |
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.
The failure visibility is useful, but the cases below leave gaps in two contracts this PR now relies on: keeping configured credentials out of the new diagnostic surface, and carrying server identity consistently through configuration, runtime observations, and manager actions. Please address these as two connected areas and validate the complete affected paths before requesting another review.
Findings
[P1] Include URL-valued stdio arguments in failure redaction
internal/tui/mcp_state.go:886
A stdio bridge can receive its authenticated endpoint through arguments instead of raw.URL. For example:
{
"command": "npx",
"args": [
"mcp-remote",
"https://host.invalid/mcp?workspace=opaque-workspace-9f3c2b7ae1d8"
]
}If initialization fails and the child prints that endpoint to stderr, connectStdio includes the captured stderr in the registration error. This PR brings that error into the failed-server view. However, sensitiveMCPArgValues does not collect components from this positional URL, and the subsequent URL collection reads raw.URL and OAuth endpoints, leaving this source out. A flag-valued URL such as --url=https://... has the same gap.
Given a skipped error containing child cannot connect to <endpoint>, the assembled view renders the opaque query value in the failure reason while the Target row beneath it replaces that same value with [REDACTED]. The discrepancy matters because hiding a credential in Target does not protect the new Error field or the transcript containing it.
The underlying issue is that URL recognition in the display path is broader than URL-source collection for failure redaction. Please apply the existing credential-bearing URL policy to URLs carried by supported stdio argument forms as well. The outcome should be the same protection regardless of whether an endpoint is configured in URL or supplied to a bridge through Args; this does not require changing what arguments a child accepts.
Validate positional and flag-valued URL cases with an opaque query value under an ordinary key, so a generic token= pattern cannot accidentally make the test pass. Drive the failed-server state through the transcript view and manager detail, and assert that neither reason nor Target exposes the value. Preserve useful host information, readable routes, and ordinary short parameters such as mode=sse under the current policy.
This finding concerns the newly displayed failure reason. It does not ask this PR to solve the separately identified, pre-existing CLI stderr redaction behavior.
[P1] Parse the argument boundary before treating value padding as a delimiter
internal/tui/mcp_state.go:1286
The supported attached-header form can contain = inside its credential:
-HX-Workspace-Id: YWJjZGVmZ2hpag==
The initial strings.Cut(arg, "=") runs before the attached-header parser. Here the first = is base64 padding, not a flag/value separator. The preceding string is nevertheless recognized as a header flag, leaving only = to collect as the secret. With a failure that echoes the credential, the rendered reason becomes:
upstream rejected YWJjZGVmZ2hpag[REDACTED][REDACTED]
The credential body remains visible and recoverable; replacing padding does not protect it. The Target parser at line 505 has the same ordering problem and exposes the body too. A packed argument such as --api-key YWJjZGVmZ2hpag== follows the same incorrect split.
Please establish the supported argument form and its actual value boundary before applying sensitive-value classification. Sharing flag predicates alone does not establish that boundary: both consumers can agree that something is sensitive while disagreeing with the argument's structure about where its secret begins. A small shared parse result is one possible approach; the required outcome is consistent value extraction and display for the forms already supported, with = inside a value preserved as value data.
Cover separated, equals-delimited, packed, and attached-header forms with values containing =. For each relevant form, assert both that the collector contains the complete credential and that the assembled view exposes no recoverable body. Keep the flag/header name and a following unrelated argument readable. Preserve the existing distinction between -H and help flag -h and avoid consuming the next argument when the current argument already carries its value.
The new failure-reason disclosure and the incomplete parser hardening are the PR-owned behavior here. This is a bounded parsing correction, not a request to define a new general-purpose command-line grammar.
[P1] Redact short bearer tokens independently of their scheme
internal/tui/mcp_state.go:850
For a configured header:
Authorization: Bearer s3cr3t
addKnown retains the complete Bearer s3cr3t value, but delegates derived candidates to credentialCandidates, which applies the eight-byte floor. The six-byte s3cr3t suffix is dropped. Passing upstream echoed s3cr3t through failure redaction and terminal sanitization leaves that token unchanged. Both failure renderers consume this insufficiently redacted reason.
The mismatch is between two already-supported properties: known credentials bypass the ambiguity floor, and a bearer credential must be protected when an error quotes only its token. Each property can pass in isolation while their combination fails. Whole-header replacement does not cover an error containing only the bare token, and the bare opaque token gives the generic redactor no header or scheme to recognize.
Please preserve known-credential treatment when extracting the token from the supported bearer form, so the bare token is redacted even below eight bytes. Do not remove the floor from all short configuration values or classify arbitrary words as secrets. Ordinary short values such as mode=sse should retain their existing readability, and the candidate-count, separator-scan, and error-processing bounds should remain intact.
Add a regression combining known header provenance, the bearer form, and a short opaque token. Exercise a value-only error as well as a whole-header echo; the latter alone can pass while this defect remains. Include an ordinary short-value control and confirm the resulting failure reason is safe in both display surfaces. The requested correction is this known bearer case, not arbitrary transformations or encodings of every possible secret.
[P2] Reject conflicting enables before persisting the configuration
internal/cli/mcp_config.go:753
The PR intentionally accepts an enabled server named docs alongside a disabled entry named " docs", because disabled entries claim no runtime identity. That valid starting configuration can become invalid through the ordinary enable operation:
zero mcp enable ' docs'
The prospective collision check protects upsertServer, but this operation uses setServerDisabled. That setter enables the entry without calling the shared uniqueness validator, and the command persists the change and reports success. Both active keys then normalize to docs. On the next launch, this PR's whole-config uniqueness gate rejects the configuration and startup exits.
The setter/normalization sequence reproduces the mismatch: enable succeeds, then NormalizeConfig rejects the resulting configuration with the message that " docs" and "docs" both resolve to "docs". The command's write path makes this a persisted problem rather than a temporary display inconsistency.
The root cause is that the new active-name invariant is enforced during normalization and add/update but is bypassed by another operation that acquires an active identity. Please check the prospective configuration before a conflicting enable is committed. Reuse the same active-name rule instead of introducing another approximation of it. Rejection should leave the persisted configuration unchanged and explain the conflicting keys.
Validate the actual enable command against a persisted enabled/disabled alias pair, verify that rejection preserves the file, and reload the result through startup normalization. Also retain coverage for enabling a nonconflicting padded key, updating the enabled entry while its alias stays disabled, and disabling or removing entries to recover from a collision. A blanket rejection of every mutation in an already-conflicting configuration would obstruct those recovery operations.
The enable setter predates this PR. The PR-owned consequence is the newly fatal startup gate combined with an ordinary mutation that can violate it. Keep that startup guard and the intentional acceptance of disabled aliases; the missing piece is consistency before enable persists a new active identity.
[P2] Match padded manager checks against the normalized runtime name
internal/tui/mcp_manager.go:145
For a server stored under the exact key " docs ", the manager now correctly dispatches check with ConfigKey. That fixes configuration lookup, but the receiving command then crosses into a different identity domain:
runMCPChecklooks up the raw key" docs "successfully.- Normalization produces runtime name
docs. - A failed registration is recorded as
SkippedServer{Name: "docs"}. Registration is best-effort, so this failure is carried inSkipped()rather than returned as the top-level registration error. - The check compares
skipped.Namewith the raw command argument" docs ", ignores the failure, and follows its success path.
With registration failing, the command returns success and JSON equivalent to:
{"serverName":" docs ","status":"ok","toolCount":0,"tools":[]}The text path likewise reports that the server is reachable. This is misleading precisely when the user selects the check action to investigate a failed server.
Please retain the exact configuration key for lookup and use the normalized runtime identity when matching registration observations. The fix should carry the distinction through the receiving command, not undo the manager's ConfigKey change or trim every string indiscriminately.
Cover a manager-selected padded key through command dispatch and a failed registration, asserting failure status and a nonzero exit result. Include an unpadded control and a successfully connected server with zero tools, since zero tools alone is not evidence of failed connectivity. Keep exact-key selection and enable/disable/remove behavior intact, including the enabled/disabled alias case.
The direct CLI mismatch already existed. Previously the manager sent the trimmed display name and failed to find this raw configuration key; this PR's corrected dispatch makes the false-success path reachable from the manager. That changed path is why this belongs in the PR.
Guidance for closing this out
These findings cluster around incomplete propagation of meaning across boundaries. In the redaction path, the code needs to retain where a value came from, how its supported argument form is parsed, and whether it is known credential material. In the identity path, it needs to retain whether a string is an exact configuration key or a normalized runtime name, and whether a mutation changes which entries claim that runtime name.
That explains how individual fixes and their tests can pass while nearby combinations remain broken: URL display can pass while failure collection omits the same URL; long bearer tails and short whole secrets can each pass while a short bearer tail fails; add/update collision checks can pass while enable bypasses them; correct manager dispatch can pass while its command ignores the resulting runtime failure. The next revision should validate these combinations together.
Please use the existing contracts as the boundary for remediation:
- Make supported argument parsing and credential-source interpretation consistent between collection and display. If sharing a small helper or parsed representation prevents the two consumers from diverging, that is useful; a cross-package redesign is not a condition of this review.
- Preserve known bearer-token treatment through the supported derivation, while keeping the existing ambiguity and resource limits. Avoid fixing a failing example by broadly redacting ordinary diagnostic text.
- Apply the same active-name rule wherever an operation creates or enables an active identity, and match observations using the identity registration actually produces. Preserve exact keys for configuration actions and preserve disable/remove recovery.
- Test complete affected paths, with both failure cases and compatibility controls. Collector-only, dispatch-only, and setter-only tests are useful locally, but the final assertions should also cover rendered output, command status, and persisted configuration where those are the user-visible consequences.
The concrete cases and controls above are intended as one consolidated validation set for this revision. Please check that each regression fails when its corresponding correction is removed; otherwise a generic redaction pattern, mocked success path, or assertion on only one surface can hide the original defect. Existing sanitizer, bounds, credential-context retention, and disabled-alias tests should continue to pass.
This guidance does not add separate findings or make broader credential storage, pre-existing CLI stderr hardening, or new reconnect behavior prerequisites. The requested outcome is to finish the failure-display and identity contracts already changed here, with the interactions covered together before the next review round.
sensitiveMCPArgValues collects by flag name, so a stdio bridge handed its
authenticated endpoint positionally ("mcp-remote https://...") or packed into
a value ("--url=https://...") contributed no redaction candidate. Those URLs
carry credentials exactly as raw.URL does, in a query key the operator named
or in userinfo, and when the child rejects its own invocation connectStdio
appends that stderr to the registration error this panel renders.
The display side already recognised all three shapes through
looksLikeMCPDisplayURLValue, which is how the two surfaces came to disagree:
the Target row replaced the opaque value while the failure reason directly
above it printed it intact, and both are persisted to the transcript.
mcpArgURLCandidates mirrors that recognition rather than inventing a second
policy, and its output goes through the existing mcpURLSecretValues with the
same known/ambiguous split raw.URL uses. Sharing the predicate is the contract:
a value the Target row treats as a URL is now necessarily a collection
candidate.
Reported by @jatmn.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found seven issues that need to be addressed before this is ready. The useful behavior remains the same as issue #825: show recorded startup failures accurately, with a reason that is safe to display and retain in the transcript.
I want to consolidate the feedback here so the next revision addresses the underlying gaps together. The recurring problems are concentrated in two contracts: preserving credential information through the failure-display pipeline, and preserving the distinction between configuration keys and runtime identities through configuration operations. The findings below include their triggers, causal paths, required outcomes and regression expectations. Example credentials are synthetic.
Why the fixes have continued to leave gaps
Several fixes cover one dimension of a case while another stage loses the information needed to keep that fix effective:
- Packed arguments are recognized, and equals-containing values are supported, but combining the two lets padding become the argument separator.
- Known short credentials are protected, and scheme-prefixed credentials are expanded, but the expansion applies the ambiguous-value length floor again.
- Locally truncated errors receive tail repair, but an earlier HTTP body truncation is invisible to that decision.
- URL arguments are collected, but individual URL components and representations are classified differently from their display counterparts.
- Exact configuration keys reach manager actions, but the receiving check still compares them with canonical runtime names.
- Add/update uses the new active-name invariant, but enable can acquire an active identity without that validation.
This explains why a regression for an individual fix can pass while a combination or downstream operation remains broken. The implementation needs to preserve the relevant facts through each transformation: the credential's boundaries and provenance, the possibility that an error is already truncated, and which kind of identity a string represents. Adding more examples to independent string heuristics will continue to leave room for disagreement between stages.
Findings
1. [P1] Parse packed and attached argument boundaries before credential padding
Location: internal/tui/mcp_state.go:1336–1342; related Target parsing at :505–512.
The previously reported equals-boundary case remains. These are two separate accepted argument examples:
["--api-key YWJjZGVmZ2hpag==", "--verbose"]
["-HX-Workspace-Id: YWJjZGVmZ2hpag==", "--verbose"]
sensitiveMCPArgValues cuts at the first = before recognizing the packed flag/value or attached header boundary. In these examples, that equals sign belongs to the credential's padding. The collector retains only =, leaving YWJjZGVmZ2hpag outside the secret set. An initialization error echoing that body survives into both failure displays. The Target parser also renders the recoverable body followed by =[REDACTED].
The old Target split already had this defect at the merge base. The PR adds a separate exposure through the retained startup reason and is also attempting to make these supported argument forms safe in Target. That is the scope of this finding.
Required outcome: determine the flag/header/value boundary before interpreting equals signs inside the value. Both failure collection and Target rendering must use the resulting credential boundary consistently. A shared parsing result is one possible implementation; the requirement is that the two consumers agree without changing what is actually passed to the child.
Regression expectation: combine packed and attached forms with padded values, assert that the collector retains the entire credential, and check both Error and Target through the view-state entry point and both renderers. Keep --verbose and the relevant flag/header label readable. Retain passing controls for separate arguments and conventional --api-key=value input. Exercise a complete-value echo as well as the recoverable body so replacing only padding cannot satisfy the assertion.
2. [P1] Preserve known credential provenance when deriving a short bearer
Location: internal/tui/mcp_state.go:878–882.
Configure:
{
"url": "https://host.invalid/mcp",
"headers": { "Authorization": "Bearer s3cr3t" }
}If initialization fails with upstream echoed s3cr3t, the bare credential reaches the overlay and transcript. addKnown correctly retains Bearer s3cr3t, but derives its tails through credentialCandidates, which drops the six-byte s3cr3t under the eight-byte floor. Generic pattern redaction cannot identify an otherwise opaque bare value in that message.
The missing information is provenance: deriving the actual credential from a known authentication value does not make it an ambiguous ordinary configuration string. Existing protections for short known values and long bearer tails do not cover their intersection.
Required outcome: retain nonempty credential tails from the supported known authentication-value forms without reapplying the ambiguity floor. Keep candidate expansion bounded and keep the readability heuristic for ordinary values. This does not require treating every short substring of a known value as secret.
Regression expectation: cover short bare-token echoes from both configured Authorization headers and supported stdio header arguments. Assert that the token disappears from both failure renderers, while ordinary short values such as mode=sse remain readable. Keep the oversized-value and bounded-expansion tests so closing this hole does not restore unbounded suffix generation.
3. [P1] Handle credential prefixes cut by the HTTP response reader
Location: internal/tui/mcp_state.go:195–197; producer at internal/mcp/network_client.go:606–613.
dropTrailingSecretPrefix runs only when the TUI's own raw-error bound reports truncation. However, httpStatusError has already limited the response body to 1024 bytes before the error reaches the TUI.
A concrete HTTP 500 response demonstrates the gap. Configure a header with this 32-byte opaque value:
Qw7ZmPr4aBcD9eFgH2jKlM6nOpR8sTuV
Return a body containing 250 copies of the four-byte erase-line sequence \x1b[2K, followed by that value. The control sequences occupy 1000 bytes, so the HTTP reader retains only the first 24 bytes of the credential. The wrapped error is below the TUI's raw bound, making its truncated flag false. Exact-value redaction cannot match the incomplete credential. Terminal normalization removes the control sequences, leaving this visible suffix in both failure displays:
Qw7ZmPr4aBcD9eFgH2jKlM6n
The HTTP limit is pre-existing. The new exposure is displaying and retaining its partial credential through this PR's failure path.
Required outcome: make the displayed observation safe when truncation occurred before the local bound. The mechanism can be a bounded partial-match policy or information supplied by the producer; the fix must not depend solely on whether this function performed the cut. Preserve bounded response reading, bounded redaction work and the useful diagnostic context.
Regression expectation: include a non-2xx response through the actual HTTP connection/error producer, then feed the resulting error into the failure view. Assert that the substantial prefix is absent from both renderers. Keep separate tests for the local cutoff, ordinary non-secret HTTP errors and oversized credentials. A fixture that starts with an already constructed long error around the TUI cutoff cannot cover this earlier boundary.
4. [P1] Collect credential-bearing fragment values from stdio endpoint arguments
Location: internal/tui/mcp_state.go:1076–1077, within mcpURLSecretValues.
This is a narrower stdio case. It does not depend on an HTTP server receiving a URL fragment.
Supply a stdio child with this endpoint argument:
https://host.invalid/mcp#workspace=opaque-fragment-9f3c2b7ae1d8
The child receives the complete argument. If it prints that endpoint to stderr and exits during initialization, connectStdio appends the stderr to the initialization error. mcpArgURLCandidates recognizes the URL, but mcpURLSecretValues collects path, userinfo and query material without collecting fragment values. The resulting failure displays the complete fragment value even though Target masks it.
This failure is reproducible with a stdio child that echoes its supplied endpoint. I am not claiming that a particular deployed MCP bridge is known to emit this error. The relevant contract is the existing handling of credential-bearing URL arguments: material protected in Target must also be protected when the same argument appears in the newly retained failure reason.
Required outcome: collect credential-bearing fragment values using the corresponding value classification already applied to their display. Preserve the actual argument sent to the child and retain harmless short fragment metadata. This does not require adding fragment-based authentication to Zero or declaring every fragment a secret.
Regression expectation: run an endpoint-echoing stdio initialization failure and check Error and Target independently. Include the applicable escaped/decoded representation and an ordinary short fragment value as controls. A query-only URL fixture will not cover the omitted component.
5. [P1] Keep the original escaped userinfo password in the known-secret set
Location: internal/tui/mcp_state.go:1069–1072.
This is another narrow stdio case, involving a password-only error rather than a whole-URL echo. Supply:
https://u:%73crt@host.invalid/mcp
The password decodes to scrt. The collector correctly marks that decoded password as known, but appends the original six-byte %73crt spelling to the ambiguous values. The length floor then discards it.
A child that extracts and reports the raw password alone during initialization produces an error ending in %73crt, which survives into the failure display and transcript. Generic userinfo masking protects a complete URL; it has no URL to recognize in this password-only output.
The failure is reproducible through an actual stdio initialization error, using a child constructed to emit the raw password. As with the fragment case, this establishes the supported failure path rather than the frequency of this behavior in deployed bridges. The scope is the existing promise to protect short userinfo passwords and the configured raw/decoded representations.
Required outcome: retain the raw password's known-credential classification alongside the decoded password. Keep the ordinary short-username heuristic. There is no requirement here to enumerate arbitrary encodings or transformations a server might invent.
Regression expectation: cover the original escaped password echoed alone, its decoded spelling echoed alone, and the complete URL. Check both failure renderers. Keep a short ordinary username control. A whole-URL-only test passes through generic masking and cannot demonstrate that the raw password entered the exact-secret set.
6. [P2] Reject conflicting enable operations before persisting configuration
Location: internal/cli/mcp_config.go:753–755; affected sibling setServerDisabled at :861–926; startup consequence at internal/cli/app.go:849.
The previously reported enable transition bypasses the prospective collision check added to upsertServer. Start with a configuration containing these two keys:
{
"docs": { "command": "docs-mcp" },
" docs": { "command": "other-mcp", "disabled": true }
}This is valid under the new rule because the disabled entry claims no active runtime identity. Then run:
zero mcp enable ' docs'
setServerDisabled removes the disabled flag, persists both entries as enabled and reports success. On the next load, the new ValidateUniqueNames rule rejects both keys resolving to docs; interactive startup aborts. The manager's exact-key enable action reaches the same operation.
The setter existed before this PR. The new active-name rejection makes its unchecked activation newly fatal. Applying the invariant only to add/update leaves a supported mutation able to write a configuration that this version refuses to start.
Required outcome: validate the prospective enabled state before committing the mutation. A conflicting enable must fail without changing the file. Use the existing active-name policy consistently; enabled/disabled aliases remain valid, and disabling or removing an entry must remain available for recovery.
Regression expectation: drive the actual toggle command against a temporary configuration file, assert failure and unchanged file bytes on collision, then reload the file. Also cover enabling a nonconflicting entry and disabling/removing an entry from a colliding configuration. Testing refuseColliding directly does not establish that enable calls it before persistence.
7. [P2] Match check failures using the canonical runtime identity
Location: internal/tui/mcp_manager.go:145 and :205; receiving comparison at internal/cli/mcp_config.go:391–393.
The previously reported padded-key check remains. With only the exact configured key " docs ", the manager now correctly sends that key to runMCPCheck. Configuration lookup succeeds. Registration normalizes the server's name to docs, and a failed connection produces SkippedServer{Name: "docs", ...}.
The check compares this canonical name with the original " docs " argument. It skips the failure and returns exit 0 with status: "ok" and zero tools. The same failed registration under an unpadded key correctly returns unreachable and exit 1.
The direct CLI comparison bug predates the PR. The changed manager dispatch makes the false-success path newly reachable: previously the manager sent the trimmed label and failed the exact configuration lookup. The correct exact-key change needs a corresponding identity-aware consumer.
Required outcome: preserve the exact key for configuration lookup and use the normalized runtime identity when matching registration outcomes. Keep the shortcut and Enter action consistent. A failed connection must produce an unsuccessful check result and its failure reason.
Regression expectation: drive a padded-key manager action through the receiving check with a failed registration. Assert the failure status and exit result, using an unpadded key as a control. Cover both action dispatches, or prove they share the tested receiving path. Tests asserting only the row's ConfigKey or dispatched arguments stop before the faulty comparison.
Root-cause guidance for the next revision
Please address this as one completion pass over the existing contracts. The following are acceptance criteria; the implementation can stay within the current package boundaries.
| Existing contract | What needs to remain true through the full path |
|---|---|
| Supported credential-bearing arguments | Parsing identifies the complete value before syntax inside that value is interpreted. Failure collection and Target rendering agree on that boundary. |
| Known versus ambiguous material | Decoding or extracting a supported credential representation preserves its known-secret classification. Ordinary short metadata retains the readability policy. |
| Bounded failure text | The displayed result stays safe when the producer or the TUI has cut a credential. Bounds remain in place and diagnostics remain useful. |
| Configuration identity | Exact keys address stored entries; canonical names join runtime results. Conversion happens deliberately at that boundary. |
| Active-name uniqueness | Every operation that activates an identity leaves a reloadable configuration, and rejected operations leave persistent state unchanged. |
A small shared parser or intermediate result that keeps value boundaries and credential classification together may help prevent the first group from recurring. Similarly, reusing the existing prospective-state validator for activation and deriving the runtime name from normalization can keep the identity rules consistent. Those are implementation options, not a requirement to introduce new types, reorganize packages or replace the observation architecture.
For validation, combine the dimensions that currently pass separately: packed/header form with equals padding; known provenance with short scheme tails; password provenance with escaped spelling; URL arguments with fragment components; and a pre-truncated transport error with terminal normalization. Then carry representative cases through the consumer that matters: both failure displays, persistent file reload, or the receiving check command. The tests should fail for the demonstrated output or state transition when the corresponding correction is removed.
The completion scope is these seven demonstrated failures and the directly shared paths needed to fix them consistently. Runtime reconnection, credential-store redesign, broader CLI logging changes and unrelated display cleanup are not requested here. Preserve disabled aliases, exact-key actions, existing input bounds and readable ordinary metadata. There is also no requirement to recognize arbitrary secret encodings beyond the configured representations and supported extraction rules involved in these findings.
Please send the next revision with these cases addressed together and the regression evidence for the complete paths. That will make it possible to assess whether the underlying contracts hold, rather than checking another set of isolated examples that leave the neighboring combination uncovered.
…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
This branch has not been deployed
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).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