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..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" @@ -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) + } +}