fix(cli): report MCP servers that failed to start (#822) - #827
fix(cli): report MCP servers that failed to start (#822)#827Vasanthdev2004 wants to merge 2 commits 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 Sequence Diagram(s)sequenceDiagram
participant MCPRuntime
participant runMCPCheck
participant CLIOutput
MCPRuntime->>runMCPCheck: Return skipped server entry
runMCPCheck->>CLIOutput: Emit unreachable text or JSON status
runMCPCheck->>CLIOutput: Exit with failure
🚥 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
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. |
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.
|
Taken in The JSON case is the more useful of the two, because reverting the fix leaves it emitting Reverting the fix fails both cases. @coderabbitai review |
|
✅ Action performedReview finished.
|
3 participants
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
mcp checkto consistently report an unreachablestatusand related error details.mcp check --json.