fix(tui): show MCP servers that failed to start in /mcp

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 {

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())
+	}
+}

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
 }

internal/tui/mcp_failed_state_test.go

@@ -0,0 +1,226 @@
+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)
+	}
+}
+
+// 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))
+		}
+	}
+}
+
+// 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)
+	}
+}

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