From 0ba5c65b0fa31a77de2c7e4f046c000d8d0e1cde Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 29 Jul 2026 22:21:39 +0530 Subject: [PATCH 1/2] fix(cli): report MCP servers that failed to start (#822) `zero mcp check` inspected only the error returned by registration and then printed "is reachable". RegisterTools is best-effort by design, so a server that fails to connect is recorded on the runtime and registration returns nil. That is right for startup, where one bad server must not stop Zero launching, and wrong here: this is the command a user runs when a server is misbehaving, and it told them the broken server was fine. The failure data already existed as SkippedServer{Name, Err}; nothing consumed it in this path. The check now fails with the recorded error, in both text and JSON output. Covered by a test that spawns a genuinely missing command rather than stubbing the runtime, because the defect was precisely a mismatch between what registration returns and what it records. Reverting the fix makes it print "MCP server broken is reachable. 0 tool(s) available." --- internal/cli/mcp_config.go | 28 ++++++++++++++++++++++++++ internal/cli/mcp_trust_edge_test.go | 31 +++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index 6abd20365..c8c56fab6 100644 --- a/internal/cli/mcp_config.go +++ b/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 { diff --git a/internal/cli/mcp_trust_edge_test.go b/internal/cli/mcp_trust_edge_test.go index e2c6960c7..25bc3c68d 100644 --- a/internal/cli/mcp_trust_edge_test.go +++ b/internal/cli/mcp_trust_edge_test.go @@ -219,3 +219,34 @@ 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) + } +} From 41d8dbf54af10cf562dcc09fc9be52f17cfbb9be Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 30 Jul 2026 14:03:42 +0530 Subject: [PATCH 2/2] test(cli): cover the unreachable --json contract The text path was covered and the JSON path was not, though the change adds its own return with a different payload: status "unreachable" plus a redacted error, rather than status "ok" with a tool count. A caller parsing that shape was relying on something nothing pinned. Reverting the fix now fails both cases. The JSON one is the more useful of the two, because the wrong output is still valid JSON and would decode cleanly into a caller expecting success. --- internal/cli/mcp_trust_edge_test.go | 48 +++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/internal/cli/mcp_trust_edge_test.go b/internal/cli/mcp_trust_edge_test.go index 25bc3c68d..69060a0a8 100644 --- a/internal/cli/mcp_trust_edge_test.go +++ b/internal/cli/mcp_trust_edge_test.go @@ -6,6 +6,7 @@ package cli import ( "bytes" + "encoding/json" "errors" "os" "path/filepath" @@ -250,3 +251,50 @@ func TestRunMCPCheckReportsUnreachableServer(t *testing.T) { 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) + } +}