From be580766672c50aaf7da294cca87511bfcafa88e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 30 Jul 2026 16:14:14 +0530 Subject: [PATCH 1/3] fix(tui): show MCP servers that failed to start in /mcp (#825) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The panel derived every server's state from config alone: `disabled` if the user turned it off, `enabled` otherwise. MCP registration is best-effort — a server that cannot be reached is recorded and startup continues — so a server that never connected was listed as enabled with its tools silently missing and nothing in the panel to explain it. Startup already knows: it prints a warning per skipped server to stderr. That scrolls away behind the first screen of output, and /mcp is where a user goes afterwards to ask what is actually running. Thread the skipped set from the MCP runtime through to the panel and render a third state, `failed`, with the recorded reason underneath the server: › docs · failed · stdio exec: "docs-mcp": executable file not found in $PATH The reason comes from the server, so it goes through redaction — a handshake error that echoes back the Authorization header would otherwise print the token into the transcript. Disabled still wins over failed: the user turned that one off, so it was never expected to connect. The stderr warning is unchanged; the panel is an addition to it. Co-Authored-By: Claude Opus 5 --- internal/cli/app.go | 13 ++- internal/cli/app_mcp_skipped_test.go | 72 +++++++++++++ internal/tui/command_views.go | 1 + internal/tui/mcp_failed_state_test.go | 139 ++++++++++++++++++++++++++ internal/tui/mcp_state.go | 29 +++++- internal/tui/mcp_view.go | 9 ++ internal/tui/model.go | 2 + internal/tui/options.go | 19 ++-- 8 files changed, 270 insertions(+), 14 deletions(-) create mode 100644 internal/cli/app_mcp_skipped_test.go create mode 100644 internal/tui/mcp_failed_state_test.go diff --git a/internal/cli/app.go b/internal/cli/app.go index 80854beb4..772a07ed7 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -833,10 +833,15 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a RunCompletionWarning: func() string { return scratchFileWarning(workspaceRoot, scratchBaseline) }, - Registry: registry, - SessionStore: deps.newSessionStore(), - SandboxStore: sandboxStore, - MCPConfig: mcpConfig, + Registry: registry, + SessionStore: deps.newSessionStore(), + SandboxStore: sandboxStore, + MCPConfig: mcpConfig, + // The panel needs the failures too. A startup warning on stderr scrolls + // away behind the first screen of output, so /mcp is where a user goes + // to ask what is actually running — it should not answer from config + // alone and report a server that never connected as enabled. + MCPSkipped: mcpRuntime.Skipped(), MCPPermissionStore: mcpPermissionStore, MCPTokenStore: mcpTokenStore, MCPCommand: func(ctx context.Context, args []string) tui.MCPCommandResult { diff --git a/internal/cli/app_mcp_skipped_test.go b/internal/cli/app_mcp_skipped_test.go new file mode 100644 index 000000000..3ac39d715 --- /dev/null +++ b/internal/cli/app_mcp_skipped_test.go @@ -0,0 +1,72 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/tui" +) + +type skippingMCPRuntime struct { + skipped []mcp.SkippedServer +} + +func (r skippingMCPRuntime) Close() error { return nil } +func (r skippingMCPRuntime) Skipped() []mcp.SkippedServer { return r.skipped } + +// Startup already knows which servers failed — it prints a warning about each. +// That warning is gone by the time anyone looks, so the same set has to reach +// the TUI, which is where /mcp answers "what is actually running". +func TestRunPassesSkippedMCPServersToTheTUI(t *testing.T) { + var stdout, stderr bytes.Buffer + cwd := t.TempDir() + setCLIUserConfigRoot(t) + projectConfigPath := filepath.Join(cwd, ".zero", "config.json") + if err := os.MkdirAll(filepath.Dir(projectConfigPath), 0o700); err != nil { + t.Fatalf("create project config parent: %v", err) + } + if err := os.WriteFile(projectConfigPath, []byte("{}"), 0o600); err != nil { + t.Fatalf("write project config: %v", err) + } + var launchedOptions tui.Options + + exitCode := runWithDeps([]string{}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{MaxTurns: 12}, nil + }, + userConfigPath: func() (string, error) { + return filepath.Join(t.TempDir(), "zero", "config.json"), nil + }, + registerMCPTools: func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error) { + return skippingMCPRuntime{skipped: []mcp.SkippedServer{ + {Name: "docs", Err: errors.New("connection refused")}, + }}, nil + }, + runTUI: func(_ context.Context, options tui.Options) int { + launchedOptions = options + return 0 + }, + }) + + if exitCode != 0 { + t.Fatalf("exit code = %d, want 0 (stderr: %s)", exitCode, stderr.String()) + } + if len(launchedOptions.MCPSkipped) != 1 || + launchedOptions.MCPSkipped[0].Name != "docs" { + t.Fatalf("MCPSkipped = %#v, want the failure startup recorded", launchedOptions.MCPSkipped) + } + // The stderr warning stays: it is what a non-interactive user sees, and the + // panel is an addition to it, not a replacement. + if !strings.Contains(stderr.String(), "docs") { + t.Errorf("startup no longer warns about the skipped server: %q", stderr.String()) + } +} diff --git a/internal/tui/command_views.go b/internal/tui/command_views.go index b7b3f21e4..ca5d336b1 100644 --- a/internal/tui/command_views.go +++ b/internal/tui/command_views.go @@ -76,6 +76,7 @@ func (m *model) refreshMCPViewState() { PermissionStore: m.mcpPermissionStore, PermissionMode: string(m.permissionMode), TokenStore: m.mcpTokenStore, + Skipped: m.mcpSkipped, }) m.mcpViewStateReady = true } diff --git a/internal/tui/mcp_failed_state_test.go b/internal/tui/mcp_failed_state_test.go new file mode 100644 index 000000000..92c9ab16e --- /dev/null +++ b/internal/tui/mcp_failed_state_test.go @@ -0,0 +1,139 @@ +package tui + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// The panel has to report what is running, not what is written down. MCP +// registration is best-effort — a server that fails to start is recorded and +// startup continues — so without the skipped set the panel calls a server that +// never connected "enabled" and the user has no way to tell from here why its +// tools are missing. +func TestBuildMCPViewStateReportsServersThatFailedToStart(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + "linear": {Type: "http", URL: "https://linear.example/mcp"}, + "offline": {Type: "http", URL: "https://offline.example/mcp", Disabled: true}, + }} + skipped := []mcp.SkippedServer{ + {Name: "docs", Err: errors.New(`exec: "docs-mcp": executable file not found in $PATH`)}, + // Disabled servers are never started, so one should not appear here. + // Assert the precedence anyway: if it ever does, the user turned this + // server off and "failed" would be a lie. + {Name: "offline", Err: errors.New("should not be reported")}, + } + + state := BuildMCPViewState(MCPStateOptions{Config: cfg, Skipped: skipped}) + + byName := make(map[string]MCPServerView, len(state.Servers)) + for _, server := range state.Servers { + byName[server.Name] = server + } + if got := byName["docs"]; got.State != "failed" || + got.Error != `exec: "docs-mcp": executable file not found in $PATH` { + t.Errorf("failed server = %#v, want state \"failed\" carrying the recorded reason", got) + } + if got := byName["linear"]; got.State != "enabled" || got.Error != "" { + t.Errorf("healthy server = %#v, want it left as enabled with no error", got) + } + if got := byName["offline"]; got.State != "disabled" || got.Error != "" { + t.Errorf("disabled server = %#v, want disabled to win over a recorded failure", got) + } +} + +// The reason is rendered from an error the server produced, so it is untrusted +// text that can carry whatever the transport echoed back — including the +// credential Zero sent it. +func TestBuildMCPViewStateRedactsTheFailureReason(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "linear": {Type: "http", URL: "https://linear.example/mcp"}, + }} + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcp.SkippedServer{{ + Name: "linear", + Err: errors.New("handshake rejected: Authorization: Bearer sk-live-abcdef0123456789abcdef"), + }}, + }) + + reason := state.Servers[0].Error + if strings.Contains(reason, "sk-live-abcdef0123456789abcdef") { + t.Fatalf("failure reason leaked the bearer token: %q", reason) + } + if !strings.Contains(reason, "handshake rejected") { + t.Errorf("redaction ate the diagnostic part of the reason: %q", reason) + } +} + +// A nil or blank error still means the server is not running. "failed" with +// nothing after it reads like a rendering bug, so fall back to a plain +// statement rather than an empty line. +func TestBuildMCPViewStateFallsBackWhenTheFailureHasNoMessage(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }} + for name, err := range map[string]error{ + "nil error": nil, + "blank error": errors.New(" "), + } { + t.Run(name, func(t *testing.T) { + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: err}}, + }) + got := state.Servers[0] + if got.State != "failed" { + t.Fatalf("state = %q, want \"failed\" even without a message", got.State) + } + if strings.TrimSpace(got.Error) == "" { + t.Error("failed server rendered with no reason at all") + } + }) + } +} + +// The reason has to survive into the text the user actually reads. +func TestRenderMCPViewShowsTheFailureReason(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }} + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: errors.New("connection refused")}}, + }) + + rendered := renderMCPView(state, 100) + if !strings.Contains(rendered, "failed") { + t.Errorf("panel does not say the server failed:\n%s", rendered) + } + if !strings.Contains(rendered, "connection refused") { + t.Errorf("panel does not show why it failed:\n%s", rendered) + } + if strings.Contains(rendered, "docs · enabled") { + t.Errorf("panel still calls the failed server enabled:\n%s", rendered) + } +} + +// End to end through the model: what startup recorded is what /mcp reports. +// The wiring is the whole point — the builder can be correct while the panel +// still renders from a set nobody handed it. +func TestModelMCPPanelReportsStartupFailures(t *testing.T) { + m := newModel(context.Background(), Options{ + MCPConfig: config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, + MCPSkipped: []mcp.SkippedServer{ + {Name: "docs", Err: errors.New("connection refused")}, + }, + }) + panel := m.mcpText() + if !strings.Contains(panel, "failed") || !strings.Contains(panel, "connection refused") { + t.Errorf("/mcp panel did not carry the startup failure through:\n%s", panel) + } +} diff --git a/internal/tui/mcp_state.go b/internal/tui/mcp_state.go index f675102d3..96ccf68e2 100644 --- a/internal/tui/mcp_state.go +++ b/internal/tui/mcp_state.go @@ -8,6 +8,7 @@ import ( "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/redaction" "github.com/Gitlawb/zero/internal/tools" ) @@ -19,6 +20,11 @@ type MCPStateOptions struct { PermissionMode string PromptCount int DeniedCount int + // Skipped are the servers registration could not start. Registration is + // best-effort so one unreachable server cannot stop Zero launching, which + // means a failure is recorded here rather than returned. Without it this + // panel reports configuration instead of reality. + Skipped []mcp.SkippedServer } type mcpServerNamedTool interface { @@ -38,21 +44,37 @@ func BuildMCPViewState(options MCPStateOptions) MCPViewState { } return MCPViewState{ - Servers: buildMCPServerViews(options.Config, toolCounts), + Servers: buildMCPServerViews(options.Config, toolCounts, options.Skipped), Tools: toolViews, Permissions: buildMCPPermissionSummary(options), OAuth: buildMCPOAuthSummary(options.Config, options.TokenStore), } } -func buildMCPServerViews(cfg config.MCPConfig, toolCounts map[string]int) []MCPServerView { +func buildMCPServerViews(cfg config.MCPConfig, toolCounts map[string]int, skipped []mcp.SkippedServer) []MCPServerView { + failures := make(map[string]error, len(skipped)) + for _, entry := range skipped { + failures[entry.Name] = entry.Err + } names := sortedMCPServerNames(cfg) servers := make([]MCPServerView, 0, len(names)) for _, name := range names { raw := cfg.Servers[name] state := "enabled" - if raw.Disabled { + message := "" + switch { + case raw.Disabled: + // Disabled wins: the user turned it off, so it was never expected to + // connect and reporting it as failed would be misleading. state = "disabled" + default: + if err, ok := failures[name]; ok { + state = "failed" + message = redaction.ErrorMessage(err, redaction.Options{}) + if strings.TrimSpace(message) == "" { + message = "server did not start" + } + } } servers = append(servers, MCPServerView{ Name: name, @@ -61,6 +83,7 @@ func buildMCPServerViews(cfg config.MCPConfig, toolCounts map[string]int) []MCPS Target: mcpServerTarget(raw), Auth: strings.TrimSpace(raw.Auth), ToolCount: toolCounts[name], + Error: message, }) } return servers diff --git a/internal/tui/mcp_view.go b/internal/tui/mcp_view.go index fc4061722..a08ff645f 100644 --- a/internal/tui/mcp_view.go +++ b/internal/tui/mcp_view.go @@ -20,6 +20,8 @@ type MCPServerView struct { Target string Auth string ToolCount int + // Error explains a "failed" state. Empty for every other state. + Error string } type MCPToolView struct { @@ -153,6 +155,13 @@ func mcpManagerServerLines(servers []MCPServerView) []string { } parts = append(parts, transport) lines = append(lines, prefix+strings.Join(parts, " · ")) + // The reason sits directly under the server rather than in the actions + // line, because "failed" on its own sends the reader to check their + // config when the answer is usually in the error: a missing binary, a + // refused connection, a bad token. + if reason := strings.TrimSpace(server.Error); reason != "" { + lines = append(lines, " "+reason) + } if target := strings.TrimSpace(server.Target); target != "" { lines = append(lines, " "+target) } diff --git a/internal/tui/model.go b/internal/tui/model.go index 239a08dd5..4e7ccd9d9 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -88,6 +88,7 @@ type model struct { sessionStore *sessions.Store sandboxStore *sandbox.GrantStore mcpConfig config.MCPConfig + mcpSkipped []internalmcp.SkippedServer mcpPermissionStore *internalmcp.PermissionStore mcpTokenStore *internalmcp.TokenStore mcpCommand func(context.Context, []string) MCPCommandResult @@ -882,6 +883,7 @@ func newModel(ctx context.Context, options Options) model { sessionStore: sessionStore, sandboxStore: sandboxStore, mcpConfig: options.MCPConfig, + mcpSkipped: options.MCPSkipped, mcpPermissionStore: options.MCPPermissionStore, mcpTokenStore: options.MCPTokenStore, mcpCommand: options.MCPCommand, diff --git a/internal/tui/options.go b/internal/tui/options.go index 409110704..f51131e85 100644 --- a/internal/tui/options.go +++ b/internal/tui/options.go @@ -45,13 +45,18 @@ type Options struct { SessionStore *sessions.Store SandboxStore *sandbox.GrantStore MCPConfig config.MCPConfig - MCPPermissionStore *mcp.PermissionStore - MCPTokenStore *mcp.TokenStore - MCPCommand func(context.Context, []string) MCPCommandResult - SandboxSetupCommand func(context.Context) SandboxSetupCommandResult - UsageTracker *usage.Tracker - SessionCompactor SessionCompactor - PrService *PrService + // MCPSkipped carries the servers that failed to start, so /mcp can report + // what is actually running rather than what is configured. Startup already + // records these; without them the panel derives state from config alone and + // shows a server that never connected as "enabled" with no explanation. + MCPSkipped []mcp.SkippedServer + MCPPermissionStore *mcp.PermissionStore + MCPTokenStore *mcp.TokenStore + MCPCommand func(context.Context, []string) MCPCommandResult + SandboxSetupCommand func(context.Context) SandboxSetupCommandResult + UsageTracker *usage.Tracker + SessionCompactor SessionCompactor + PrService *PrService AgentOptions agent.Options // LoadSkills returns the installed skills (default skills dir merged with any From c7421d97955224a7e024efbfdabe6bb0db6686d0 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 5 Aug 2026 15:15:08 +0530 Subject: [PATCH 2/3] fix(tui): sanitize the MCP failure reason before it reaches the terminal The failure reason is the only value on the /mcp panel that the MCP server writes itself, and it went to the terminal with nothing but TrimSpace. redaction.ErrorMessage strips credentials, not control bytes, so a hostile handshake error could clear the screen, move the cursor, or embed a newline followed by text shaped like a real entry and forge a row for a server that does not exist. Reproduced with @anandh8x's payload from the review. Before the fix the rendered panel was: > evil . failed . http connection refused\x1b[2J > forged . enabled actions: zero mcp check evil | ... The escape sequence and the forged row both survived intact. sanitizeTerminalReason consumes escape sequences whole rather than dropping ESC alone, since removing the ESC and leaving "[2J" behind would print visible junk and an abandoned OSC payload can still smuggle a title-set or hyperlink. CSI runs to its final byte, OSC to BEL or ST. Newlines and tabs collapse to spaces so the reason stays on the single row the panel counted for it, other control bytes are dropped, and the result is capped at 400 runes so one verbose server cannot push the panel off screen. Truncation is by rune, not byte, so a multi-byte character is never cut in half. Two regressions cover it. The injection test asserts no escape byte survives, no rendered line carries its own newline, the forged text never begins a row, and the real reason is still shown. The cap test drives 5000 characters through and asserts the rendered line stays bounded. Both fail on the code before this commit. --- internal/tui/mcp_failed_state_test.go | 48 ++++++++++++++++++ internal/tui/mcp_view.go | 70 ++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/internal/tui/mcp_failed_state_test.go b/internal/tui/mcp_failed_state_test.go index 92c9ab16e..9152a0e6a 100644 --- a/internal/tui/mcp_failed_state_test.go +++ b/internal/tui/mcp_failed_state_test.go @@ -137,3 +137,51 @@ func TestModelMCPPanelReportsStartupFailures(t *testing.T) { t.Errorf("/mcp panel did not carry the startup failure through:\n%s", panel) } } + +// The failure reason is the one string on this panel an MCP server writes +// itself, and it lands in a terminal. redaction.ErrorMessage removes +// credentials, not control bytes, so a hostile handshake error can clear the +// screen, move the cursor, or forge a row that looks like another server. +// +// Payload is anandh8x's from the #835 review: an escape sequence and a newline +// carrying a line shaped exactly like a real entry. +func TestMCPFailureReasonCannotInjectTerminalControl(t *testing.T) { + hostile := "connection refused\x1b[2J\n\u203a forged \u00b7 enabled" + lines := mcpManagerServerLines([]MCPServerView{ + {Name: "evil", Transport: "http", State: "failed", Error: hostile}, + }) + + joined := strings.Join(lines, "\n") + if strings.ContainsRune(joined, '\x1b') { + t.Errorf("escape byte survived into the rendered panel:\n%q", joined) + } + for _, line := range lines { + if strings.ContainsAny(line, "\r\n") { + t.Errorf("a rendered line carries its own newline, so it occupies rows the panel did not count:\n%q", line) + } + } + // The forged text may appear as inert characters, but never as its own row: + // that is what makes it read as a second server. + for _, line := range lines { + if strings.HasPrefix(strings.TrimSpace(line), "\u203a forged") { + t.Errorf("hostile reason forged a server row: %q", line) + } + } + // The real reason must still reach the user; sanitizing must not blank it. + if !strings.Contains(joined, "connection refused") { + t.Errorf("the actual failure reason was lost:\n%q", joined) + } +} + +// A server that returns megabytes of error text must not push the rest of the +// panel off screen. +func TestMCPFailureReasonIsLengthCapped(t *testing.T) { + lines := mcpManagerServerLines([]MCPServerView{ + {Name: "verbose", Transport: "http", State: "failed", Error: strings.Repeat("x", 5000)}, + }) + for _, line := range lines { + if len(line) > 512 { + t.Errorf("rendered line is %d bytes, want it capped", len(line)) + } + } +} diff --git a/internal/tui/mcp_view.go b/internal/tui/mcp_view.go index a08ff645f..5d20ff0e0 100644 --- a/internal/tui/mcp_view.go +++ b/internal/tui/mcp_view.go @@ -159,7 +159,7 @@ func mcpManagerServerLines(servers []MCPServerView) []string { // line, because "failed" on its own sends the reader to check their // config when the answer is usually in the error: a missing binary, a // refused connection, a bad token. - if reason := strings.TrimSpace(server.Error); reason != "" { + if reason := sanitizeTerminalReason(server.Error); reason != "" { lines = append(lines, " "+reason) } if target := strings.TrimSpace(server.Target); target != "" { @@ -172,6 +172,74 @@ func mcpManagerServerLines(servers []MCPServerView) []string { return lines } +// maxMCPReasonLen bounds the failure reason so one verbose server cannot push +// the rest of the panel off screen. +const maxMCPReasonLen = 400 + +// sanitizeTerminalReason turns a server-authored string into one safe terminal +// line. +// +// The failure reason is the only value on this panel that the MCP server writes +// itself, and it goes straight to a terminal. redaction.ErrorMessage removes +// credentials, not control bytes, so without this a hostile handshake error can +// clear the screen, reposition the cursor, or embed a newline followed by text +// shaped like a real entry and forge a row for a server that does not exist. +// +// Escape sequences are consumed whole rather than dropping ESC alone: removing +// the ESC and leaving "[2J" behind would print visible junk, and an abandoned +// OSC payload can still smuggle a title-set or a hyperlink. +func sanitizeTerminalReason(value string) string { + var out strings.Builder + runes := []rune(value) + for index := 0; index < len(runes); index++ { + current := runes[index] + if current == 0x1b { + index++ + if index >= len(runes) { + break + } + switch runes[index] { + case '[': // CSI: parameters, then a final byte in @ to ~ + index++ + for index < len(runes) && (runes[index] < '@' || runes[index] > '~') { + index++ + } + case ']': // OSC: runs until BEL or ST + index++ + for index < len(runes) { + if runes[index] == 0x07 { + break + } + if runes[index] == 0x1b && index+1 < len(runes) && runes[index+1] == '\\' { + index++ + break + } + index++ + } + } + continue + } + // Newlines and tabs become spaces so the reason stays on the single row + // the panel counted for it. Every other control byte is dropped: none + // carries a display meaning worth preserving here. + if current == '\n' || current == '\r' || current == '\t' { + out.WriteRune(' ') + continue + } + if current < 0x20 || current == 0x7f || (current >= 0x80 && current <= 0x9f) { + continue + } + out.WriteRune(current) + } + // Fields also collapses the runs of spaces the substitutions above create. + collapsed := strings.Join(strings.Fields(out.String()), " ") + if trimmed := []rune(collapsed); len(trimmed) > maxMCPReasonLen { + // Truncate by rune so a multi-byte character is never cut in half. + collapsed = string(trimmed[:maxMCPReasonLen]) + "..." + } + return collapsed +} + func mcpToolLines(tools []MCPToolView) []string { grouped := map[string][]MCPToolView{} order := []string{} From 6521c367f949cb5119c093768d0c81084fb5e25c Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 5 Aug 2026 15:26:29 +0530 Subject: [PATCH 3/3] fix(tui): bound the MCP failure reason before sanitizing it The display cap runs at the end, so the sanitizer walked the whole server-authored string first. Escape sequences are consumed without producing output, so they spend input against a budget that never fills: 64KB of "\x1b[2J" was walked in full and the text after it still rendered. Nothing upstream bounds the handshake error, and the panel re-runs this on every redraw. Cap the raw input at 16KB before the walk, well above the 400 rune display cap so a long error is still truncated by display rules. Trim back a character the cut splits so the panel never renders a replacement character it produced itself. --- internal/tui/mcp_failed_state_test.go | 39 +++++++++++++++++++++++++++ internal/tui/mcp_view.go | 21 +++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/internal/tui/mcp_failed_state_test.go b/internal/tui/mcp_failed_state_test.go index 9152a0e6a..90a26e9c2 100644 --- a/internal/tui/mcp_failed_state_test.go +++ b/internal/tui/mcp_failed_state_test.go @@ -185,3 +185,42 @@ func TestMCPFailureReasonIsLengthCapped(t *testing.T) { } } } + +// The display cap alone cannot stop the walk: escape sequences are consumed +// without producing any visible output, so a server can spend unbounded input +// against a budget that never fills. Only a bound on the raw input ends it, and +// the panel re-renders this string on every redraw. +func TestMCPFailureReasonBoundsRawInput(t *testing.T) { + raw := strings.Repeat("\x1b[2J", maxMCPReasonRawLen) + "TAIL" + got := sanitizeTerminalReason(raw) + if strings.Contains(got, "TAIL") { + t.Fatalf("sanitizeTerminalReason walked past the raw bound and reached byte %d: %q", len(raw)-4, got) + } + if got != "" { + t.Fatalf("sanitizeTerminalReason(escape sequences only) = %q, want it to render nothing", got) + } +} + +// Cutting the raw input must not leave half of a multi-byte character behind: +// the panel would show a replacement character it invented itself. Escape +// sequences render as nothing, so they carry the cut far past what the display +// cap would have removed and leave the split character as the visible tail. +func TestMCPFailureReasonRawBoundKeepsRunesWhole(t *testing.T) { + const invisible = "\x1b[2J" + // The last character starts two bytes before the bound, so the cut keeps two + // of its three bytes. + prefix := strings.Repeat(invisible, (maxMCPReasonRawLen-2)/len(invisible)) + prefix += strings.Repeat("a", maxMCPReasonRawLen-2-len(prefix)) + raw := prefix + "\u203a" + if len(raw) <= maxMCPReasonRawLen { + t.Fatalf("test setup: raw input is %d bytes, it must straddle the %d byte bound", len(raw), maxMCPReasonRawLen) + } + + got := sanitizeTerminalReason(raw) + if strings.ContainsRune(got, '\ufffd') { + t.Fatalf("the raw bound split a rune: %q", got) + } + if got != "aa" { + t.Fatalf("sanitizeTerminalReason(...) = %q, want the whole characters before the cut", got) + } +} diff --git a/internal/tui/mcp_view.go b/internal/tui/mcp_view.go index 5d20ff0e0..b770ace61 100644 --- a/internal/tui/mcp_view.go +++ b/internal/tui/mcp_view.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" "time" + "unicode/utf8" ) type MCPViewState struct { @@ -176,6 +177,14 @@ func mcpManagerServerLines(servers []MCPServerView) []string { // the rest of the panel off screen. const maxMCPReasonLen = 400 +// maxMCPReasonRawLen bounds the input the sanitizer walks. Nothing upstream caps +// the handshake error a server hands back, and maxMCPReasonLen alone cannot end +// the walk: escape sequences are consumed without producing output, so they +// spend input against a budget that never fills. The bound sits far above the +// visible cap so a genuinely long error is still truncated by display rules +// rather than by this. +const maxMCPReasonRawLen = 16 * 1024 + // sanitizeTerminalReason turns a server-authored string into one safe terminal // line. // @@ -189,6 +198,18 @@ const maxMCPReasonLen = 400 // the ESC and leaving "[2J" behind would print visible junk, and an abandoned // OSC payload can still smuggle a title-set or a hyperlink. func sanitizeTerminalReason(value string) string { + if len(value) > maxMCPReasonRawLen { + value = value[:maxMCPReasonRawLen] + // The cut lands on an arbitrary byte. Drop a rune the bound split so the + // panel never shows a replacement character it produced itself. + for len(value) > 0 { + decoded, width := utf8.DecodeLastRuneInString(value) + if decoded != utf8.RuneError || width > 1 { + break + } + value = value[:len(value)-1] + } + } var out strings.Builder runes := []rune(value) for index := 0; index < len(runes); index++ {