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

internal/cli/app.go

@@ -839,6 +839,15 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a
 		mcpTokenStore = nil
 		err = nil
 	}
+	// Checked on the WHOLE configuration, before the split. The two halves
+	// normalize separately, so a collision that straddles them is invisible to
+	// either call: a user-configured "  exa" is critical while the built-in
+	// default "exa" is optional, and they are one runtime server. Ambiguous
+	// identity is worth refusing to start over, because every downstream join is
+	// keyed on that name and one of the two entries is unreachable regardless.
+	if err := mcp.ValidateUniqueNames(mcpConfig); err != nil {
+		return writeAppError(stderr, err.Error(), 1)
+	}
 	criticalMCPConfig, optionalMCPConfig := splitMCPStartupConfig(mcpConfig)
 	mcpRuntime := mcpToolRuntime(noopMCPRuntime{})
 	if len(criticalMCPConfig.Servers) > 0 {
@@ -847,6 +856,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a
 			Autonomy:        mcp.AutonomyLow,
 			Execution:       executionRunner,
 			WorkspaceRoot:   workspaceRoot,
+			SecretValues:    mcpTokenStore.SecretValues,
 		})
 		if registerErr != nil {
 			closeMCPRuntime(stderr, runtime)
@@ -929,6 +939,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a
 				Autonomy:        mcp.AutonomyLow,
 				Execution:       executionRunner,
 				WorkspaceRoot:   workspaceRoot,
+				SecretValues:    mcpTokenStore.SecretValues,
 			},
 			deps.registerMCPTools,
 			func() {
@@ -1000,8 +1011,22 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a
 		PeerService:        peerService,
 		SandboxStore:       sandboxStore,
 		MCPConfig:          mcpConfig,
-		MCPPermissionStore: mcpPermissionStore,
-		MCPTokenStore:      mcpTokenStore,
+		// 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(),
+		// Optional servers register on a background goroutine, so their failures
+		// are not known yet and a snapshot cannot carry them. Pulled instead, or
+		// they stay rendered from configuration alone: enabled, with no
+		// explanation, for a server that never connected.
+		MCPLateSkipped: optionalMCPRuntime.Skipped,
+		// And a signal to rebuild when it finishes. Pulling alone leaves an
+		// already-open manager showing the configuration-derived state, because a
+		// background completion schedules no render of its own.
+		MCPStartupCompleted: optionalMCPRuntime.completed(),
+		MCPPermissionStore:  mcpPermissionStore,
+		MCPTokenStore:       mcpTokenStore,
 		MCPCommand: func(ctx context.Context, args []string) tui.MCPCommandResult {
 			if ctx == nil {
 				ctx = context.Background()

internal/cli/app_mcp_skipped_test.go

@@ -0,0 +1,82 @@
+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
+		},
+		// A CONFIGURED server, so it lands in the critical half startup registers
+		// synchronously. Startup splits MCP into a critical set registered before
+		// the TUI launches and an optional set (unconfigured built-in defaults)
+		// registered on a background goroutine; with no servers configured at all,
+		// the registration this test stubs is never reached.
+		resolveMCPConfig: func(string, bool) (config.MCPConfig, error) {
+			return config.MCPConfig{Servers: map[string]config.MCPServerConfig{
+				"docs": {URL: "https://host.invalid/mcp"},
+			}}, 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/cli/mcp_config.go

@@ -641,11 +641,44 @@ func (cfg *mcpWritableConfig) ensureRaw() {
 	}
 }
 
+// refuseColliding validates the PROSPECTIVE configuration with the same rule the
+// read and startup paths use.
+//
+// This used to compare key spellings here and reject every trimmed collision,
+// which is a second and weaker implementation of the active-identity rule.
+// ValidateUniqueNames skips disabled entries, because registration skips them
+// and a disabled server claims no runtime identity; the write side did not, so
+// `zero mcp add docs` failed when a disabled " docs" was configured, and even a
+// plain update of an enabled "docs" was refused while its disabled alias
+// existed. It also could not decide the inverse case at all, because it never
+// saw whether the incoming server was itself disabled.
+//
+// Building the combined map and handing it to the shared rule removes the
+// second implementation rather than teaching it the same exceptions.
+func (cfg *mcpWritableConfig) refuseColliding(name string, incoming config.MCPServerConfig) error {
+	prospective := make(map[string]config.MCPServerConfig, len(cfg.file.MCP.Servers)+1)
+	for key, server := range cfg.file.MCP.Servers {
+		prospective[key] = server
+	}
+	prospective[name] = incoming
+	return mcp.ValidateUniqueNames(config.MCPConfig{Servers: prospective})
+}
+
 func (cfg *mcpWritableConfig) upsertServer(name string, server config.MCPServerConfig) (bool, error) {
 	cfg.ensureRaw()
 	if cfg.file.MCP.Servers == nil {
 		cfg.file.MCP.Servers = map[string]config.MCPServerConfig{}
 	}
+	// One config key per runtime identity. Registration trims the key, so a new
+	// "docs" written next to an existing "  docs" produces two entries that are
+	// one server everywhere downstream: they share a tool count and a failure,
+	// map iteration decides which configuration survives, and each redacts that
+	// shared failure with its own credentials, so the one that did not fail can
+	// print the other's. Refusing at the write boundary keeps the collision out
+	// of the file rather than reporting it on every later load.
+	if err := cfg.refuseColliding(name, server); err != nil {
+		return false, err
+	}
 	existingRaw, updated := cfg.serverRaw[name]
 	existingServer := cfg.file.MCP.Servers[name]
 	if !updated {

internal/cli/mcp_server_identity_test.go

@@ -0,0 +1,123 @@
+package cli
+
+import (
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/Gitlawb/zero/internal/config"
+	"github.com/Gitlawb/zero/internal/mcp"
+)
+
+// ONE CONFIG KEY PER RUNTIME IDENTITY, ENFORCED WHERE THE KEY IS WRITTEN.
+//
+// Registration trims the config-map key, so "docs" and "  docs" are two entries
+// in the file and one server everywhere downstream. They share a tool count and
+// a failure, map iteration decides which configuration wins, and each row
+// redacts that shared failure with its own credentials, so the row that did not
+// fail can print the other one's.
+//
+// Validation on the way in checks the incoming server by itself and cannot see
+// the collision, so the check belongs at the write.
+func TestUpsertRefusesAKeyThatCollidesWithAnExistingServer(t *testing.T) {
+	cfg := &mcpWritableConfig{}
+	cfg.ensureRaw()
+	cfg.file.MCP.Servers = map[string]config.MCPServerConfig{
+		"  docs": {URL: "https://a.invalid/mcp"},
+	}
+
+	_, err := cfg.upsertServer("docs", config.MCPServerConfig{URL: "https://b.invalid/mcp"})
+	if err == nil {
+		t.Fatalf("a colliding key was written: %#v", cfg.file.MCP.Servers)
+	}
+	for _, want := range []string{`"docs"`, `"  docs"`, "rename"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("the refusal does not mention %s: %v", want, err)
+		}
+	}
+	if len(cfg.file.MCP.Servers) != 1 {
+		t.Errorf("the refused server was written anyway: %#v", cfg.file.MCP.Servers)
+	}
+}
+
+// Updating a server through its own key is the ordinary case and must not be
+// mistaken for a collision with itself.
+func TestUpsertStillUpdatesTheSameKey(t *testing.T) {
+	cfg := &mcpWritableConfig{}
+	cfg.ensureRaw()
+	cfg.file.MCP.Servers = map[string]config.MCPServerConfig{
+		"  docs": {URL: "https://a.invalid/mcp"},
+	}
+	if _, err := cfg.upsertServer("  docs", config.MCPServerConfig{URL: "https://b.invalid/mcp"}); err != nil {
+		t.Fatalf("updating a server through its own key was refused: %v", err)
+	}
+	if got := cfg.file.MCP.Servers["  docs"].URL; got != "https://b.invalid/mcp" {
+		t.Errorf("URL = %q, want the update applied", got)
+	}
+}
+
+// And an unrelated new server is not blocked by an existing one.
+func TestUpsertAllowsADistinctName(t *testing.T) {
+	cfg := &mcpWritableConfig{}
+	cfg.ensureRaw()
+	cfg.file.MCP.Servers = map[string]config.MCPServerConfig{
+		"docs": {URL: "https://a.invalid/mcp"},
+	}
+	if _, err := cfg.upsertServer("search", config.MCPServerConfig{URL: "https://b.invalid/mcp"}); err != nil {
+		t.Fatalf("a distinct server was refused: %v", err)
+	}
+	if len(cfg.file.MCP.Servers) != 2 {
+		t.Errorf("servers = %#v, want both", cfg.file.MCP.Servers)
+	}
+}
+
+// THE UNIQUENESS CHECK HAS TO RUN ON THE WHOLE CONFIGURATION, BEFORE THE SPLIT.
+//
+// Startup separates unconfigured built-in defaults from the servers the user
+// asked for, so the two halves are normalized by separate calls. A collision
+// that straddles the split is invisible to both of them: a user-configured
+// "  exa" is critical, the built-in "exa" is optional, and they are one runtime
+// server with two panel rows sharing a failure and a tool count.
+func TestACollisionAcrossTheStartupSplitIsStillRefused(t *testing.T) {
+	defaults := config.DefaultMCPServers()
+	names := make([]string, 0, len(defaults))
+	for name := range defaults {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	if len(names) == 0 {
+		t.Skip("no built-in MCP defaults to collide with")
+	}
+	name := names[0]
+
+	// The user's own entry under a padded key: same runtime identity, different
+	// configuration, so it is not the untouched default.
+	mine := defaults[name]
+	mine.Headers = map[string]string{"X-Api-Key": "opaque-workspace-9f3c2b7ae1d8"}
+
+	cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{
+		name:        defaults[name],
+		"  " + name: mine,
+	}}
+
+	critical, optional := splitMCPStartupConfig(cfg)
+	if len(critical.Servers) != 1 || len(optional.Servers) != 1 {
+		t.Fatalf("the two entries did not straddle the split: critical=%v optional=%v", critical.Servers, optional.Servers)
+	}
+	// Each half on its own sees one server and no collision, which is why the
+	// per-half check cannot be the guard.
+	if err := mcp.ValidateUniqueNames(critical); err != nil {
+		t.Fatalf("the critical half alone reported a collision: %v", err)
+	}
+	if err := mcp.ValidateUniqueNames(optional); err != nil {
+		t.Fatalf("the optional half alone reported a collision: %v", err)
+	}
+
+	err := mcp.ValidateUniqueNames(cfg)
+	if err == nil {
+		t.Fatal("a collision straddling the startup split was accepted")
+	}
+	if !strings.Contains(err.Error(), name) || !strings.Contains(err.Error(), "rename") {
+		t.Errorf("the refusal is not actionable: %v", err)
+	}
+}

internal/cli/mcp_startup.go

@@ -142,6 +142,40 @@ func (startup *optionalMCPStartup) closeRuntime() {
 	})
 }
 
+// Skipped reports the optional servers that failed, or nothing while startup is
+// still running.
+//
+// A NIL RETURN WAS NOT "NOT READY", IT WAS "NONE". These servers are ordinary
+// rows on the /mcp panel: splitMCPStartupConfig moves them off the critical path
+// so a slow one cannot delay the first response, it does not make them invisible.
+// Discarding their failures here left every one of them rendered from
+// configuration alone, which reports a server that never connected as enabled --
+// the single thing that panel exists to prevent.
+// completed exposes the completion signal so an already-open surface can be
+// told to rebuild. Nil when there is no background registration, which is what
+// the consumer treats as "nothing to wait for".
+func (startup *optionalMCPStartup) completed() <-chan struct{} {
+	if startup == nil {
+		return nil
+	}
+	return startup.done
+}
+
 func (startup *optionalMCPStartup) Skipped() []mcp.SkippedServer {
-	return nil
+	if startup == nil {
+		return nil
+	}
+	select {
+	case <-startup.done:
+	default:
+		// Still connecting. There is no observation yet, and blocking here would
+		// stall a render on a server deliberately kept off the critical path.
+		return nil
+	}
+	startup.mu.Lock()
+	defer startup.mu.Unlock()
+	if startup.runtime == nil {
+		return nil
+	}
+	return startup.runtime.Skipped()
 }