fix(cli): report MCP servers that failed to start (#822) - #827
Open
Vasanthdev2004 wants to merge 1 commit into
Open
fix(cli): report MCP servers that failed to start (#822)#827Vasanthdev2004 wants to merge 1 commit into
Vasanthdev2004 wants to merge 1 commit into
Conversation
`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."
Walkthrough
ChangesMCP unreachable-server reporting
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/mcp_trust_edge_test.go`:
- Around line 243-251: The existing unreachable-server test only validates text
output; extend the test coverage around runWithDeps to exercise the --json
response for the broken server. Decode stdout as JSON and assert serverName,
status equal to "unreachable", and a non-empty redacted error field, while
preserving the existing failure and text-output assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62b34002-a3a7-459c-b0f2-b99032ed2f7b
📒 Files selected for processing (2)
internal/cli/mcp_config.gointernal/cli/mcp_trust_edge_test.go
Comment on lines
+243
to
+251
| 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) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the JSON unreachable response.
This only exercises text output. Add a --json case that decodes stdout and asserts serverName, status: "unreachable", and a non-empty redacted error; the PR changes that contract too.
As per coding guidelines, “**/*_test.go: Keep tests beside their source files … and add a regression test for behavior changes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/mcp_trust_edge_test.go` around lines 243 - 251, The existing
unreachable-server test only validates text output; extend the test coverage
around runWithDeps to exercise the --json response for the broken server. Decode
stdout as JSON and assert serverName, status equal to "unreachable", and a
non-empty redacted error field, while preserving the existing failure and
text-output assertions.
Source: Coding guidelines
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #822.
zero mcp checklooked only at the error returned by registration and then printed "is reachable".RegisterToolsis best-effort by design: a server that fails to connect is recorded on the runtime and registration returns nil, so one broken server cannot stop Zero launching. Right for startup, wrong here, since this is the command a user runs precisely when a server is misbehaving.The failure data already existed as
SkippedServer{Name, Err}and nothing in this path read it. It now fails with the recorded error, in both text and JSON.The test spawns a genuinely missing command rather than stubbing the runtime. The defect was a mismatch between what registration returns and what it records, so a stub could have been written to pass either way. Reverting the fix makes it print:
Two files, 59 lines added.
One note for CI:
TestBuildServeScopeKeepsLexicalPathsfails on my machine on cleanmaintoo, because creating a symlink needs a privilege an unelevated Windows shell does not hold. Unrelated to this change.Summary by CodeRabbit