fix(cli): report MCP servers that failed to start (#822)

internal/cli/mcp_config.go

@@ -308,6 +308,34 @@ func runMCPCheck(ctx context.Context, args []string, stdout io.Writer, stderr io
 	}
 	defer closeMCPRuntime(stderr, mcpRuntime)
 
+	// A connect failure does not come back as err. RegisterTools is deliberately
+	// best-effort so one unreachable server cannot stop Zero launching: it records
+	// the failure on the runtime and returns nil. That is right for startup and
+	// wrong to ignore here, because this is the command a user runs precisely when
+	// a server is misbehaving, and reporting "is reachable" for a server that never
+	// started sends them looking somewhere else.
+	for _, skipped := range mcpRuntime.Skipped() {
+		if skipped.Name != serverName {
+			continue
+		}
+		message := fmt.Sprintf("MCP server %s is not reachable", serverName)
+		if skipped.Err != nil {
+			message = fmt.Sprintf("%s: %s", message, redaction.ErrorMessage(skipped.Err, redaction.Options{}))
+		}
+		if options.json {
+			payload := struct {
+				ServerName string `json:"serverName"`
+				Status     string `json:"status"`
+				Error      string `json:"error,omitempty"`
+			}{ServerName: serverName, Status: "unreachable", Error: message}
+			if err := writePrettyJSON(stdout, redaction.RedactValue(payload, redaction.Options{})); err != nil {
+				return exitCrash
+			}
+			return exitCrash
+		}
+		return writeAppError(stderr, message, exitCrash)
+	}
+
 	items := mcpToolList(registry)
 	if options.json {
 		payload := struct {

internal/cli/mcp_trust_edge_test.go

@@ -6,6 +6,7 @@ package cli
 
 import (
 	"bytes"
+	"encoding/json"
 	"errors"
 	"os"
 	"path/filepath"
@@ -219,3 +220,81 @@ func TestRunMCPCheckNormalizeError(t *testing.T) {
 		t.Fatalf("want normalize error, got %q", got)
 	}
 }
+
+// TestRunMCPCheckReportsUnreachableServer covers the case the command exists
+// for: a server that fails to start.
+//
+// RegisterTools is best-effort by design, so a connect failure is recorded on
+// the runtime and registration returns nil. runMCPCheck used to inspect only
+// that nil error and then print "is reachable", which told the user their
+// broken server was fine. The command is spawned for real here rather than
+// stubbed, because the whole failure mode was a mismatch between what
+// registration returns and what it records.
+func TestRunMCPCheckReportsUnreachableServer(t *testing.T) {
+	setTrustConfigRoot(t)
+	deps := appDeps{
+		getwd: func() (string, error) { return t.TempDir(), nil },
+		resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) {
+			return config.MCPConfig{Servers: map[string]config.MCPServerConfig{
+				"broken": {Type: "stdio", Command: "zero-definitely-not-a-real-binary-xyz"},
+			}}, nil
+		},
+	}
+	var out, errBuf bytes.Buffer
+	if code := runWithDeps([]string{"mcp", "check", "broken"}, &out, &errBuf, deps); code == exitSuccess {
+		t.Fatalf("mcp check must fail for a server that did not start; stdout=%q stderr=%q", out.String(), errBuf.String())
+	}
+	if got := out.String(); strings.Contains(got, "is reachable") {
+		t.Fatalf("mcp check reported a failed server as reachable: %q", got)
+	}
+	if got := errBuf.String(); !strings.Contains(got, "not reachable") {
+		t.Fatalf("want an unreachable error naming the server, got %q", got)
+	}
+}
+
+// The --json contract for an unreachable server, which the text case above does
+// not touch.
+//
+// The JSON branch is a separate return path with its own payload, so a caller
+// parsing it would otherwise be relying on a shape nothing pins: reverting the
+// check leaves it emitting status "ok" with a tool count, which is valid JSON
+// and completely wrong.
+func TestRunMCPCheckReportsUnreachableServerJSON(t *testing.T) {
+	setTrustConfigRoot(t)
+	deps := appDeps{
+		getwd: func() (string, error) { return t.TempDir(), nil },
+		resolveMCPConfig: func(_ string, _ bool) (config.MCPConfig, error) {
+			return config.MCPConfig{Servers: map[string]config.MCPServerConfig{
+				"broken": {Type: "stdio", Command: "zero-definitely-not-a-real-binary-xyz"},
+			}}, nil
+		},
+	}
+	var out, errBuf bytes.Buffer
+	if code := runWithDeps([]string{"mcp", "check", "broken", "--json"}, &out, &errBuf, deps); code == exitSuccess {
+		t.Fatalf("mcp check --json must fail for a server that did not start; stdout=%q stderr=%q", out.String(), errBuf.String())
+	}
+	var payload struct {
+		ServerName string `json:"serverName"`
+		Status     string `json:"status"`
+		Error      string `json:"error"`
+		ToolCount  int    `json:"toolCount"`
+	}
+	if err := json.Unmarshal(out.Bytes(), &payload); err != nil {
+		t.Fatalf("decode mcp check --json output %q: %v", out.String(), err)
+	}
+	if payload.ServerName != "broken" {
+		t.Fatalf("serverName = %q, want the server that was checked", payload.ServerName)
+	}
+	if payload.Status != "unreachable" {
+		t.Fatalf("status = %q, want %q", payload.Status, "unreachable")
+	}
+	if !strings.Contains(payload.Error, "not reachable") {
+		t.Fatalf("error = %q, want it to say the server is not reachable", payload.Error)
+	}
+	// A reachable payload carries a tool count; an unreachable one must not
+	// imply zero tools were found, which reads as a server that simply exposes
+	// none.
+	if payload.ToolCount != 0 {
+		t.Fatalf("toolCount = %d, want it absent from an unreachable payload", payload.ToolCount)
+	}
+}