From be580766672c50aaf7da294cca87511bfcafa88e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 30 Jul 2026 16:14:14 +0530 Subject: [PATCH] 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