Add --health flag, JSONC config support, and gh CLI discovery#40
Add --health flag, JSONC config support, and gh CLI discovery#40
Conversation
The MCP CLI had no lightweight way to check server reachability (only --list which fetches all tools), config files broke on // comments that users naturally add, and gh's help format (uppercase headers like "CORE COMMANDS", colon-suffixed names like "auth:") wasn't recognized by the parser. Health check does connect-handshake-disconnect reusing the existing initialize flow, returning exit 0/1. Config parser now strips single-line and block comments before JSON parsing, respecting string literals so URLs survive. CLI discovery relaxes section header regex to not require trailing colon and accepts colon-suffixed command names, with an indentation guard to prevent description text from false-matching as section headers. closes #11 Signed-off-by: Avelino <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds a lightweight server reachability check to the MCP CLI, improves configuration file robustness by supporting JSON-with-comments, and expands CLI help parsing to correctly discover GitHub CLI-style subcommands.
Changes:
- Add
mcp <server> --healthcommand path for connect/initialize/shutdown reachability checks. - Strip
//and/* */comments from config content before JSON parsing, with tests. - Relax CLI discovery parsing to recognize uppercase “... COMMANDS” sections and colon-suffixed command names (e.g.
auth:), with a newgh-style test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/main.rs |
Adds --health usage + handling in server command flow. |
src/config.rs |
Adds JSON comment stripping and tests; applies it during config load. |
src/cli_discovery.rs |
Updates help parser regexes and adds gh-format subcommand test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if args.len() >= 2 && args[1] == "--health" { | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| audit.log(audit::AuditEntry { | ||
| timestamp: chrono::Local::now().to_rfc3339(), | ||
| source: "cli".to_string(), | ||
| method: "health".to_string(), | ||
| tool_name: None, | ||
| server_name: Some(server_name.clone()), | ||
| identity: "local".to_string(), | ||
| duration_ms: start.elapsed().as_millis() as u64, | ||
| success: true, | ||
| error_message: None, | ||
| arguments: None, | ||
| }); |
There was a problem hiding this comment.
--health audit logging is currently misleading: the timer starts after McpClient::connect() (so it excludes connect/initialize time), duration_ms is computed immediately (near-zero), and success is hard-coded true before shutdown() runs. Consider timing the full connect→initialize→shutdown flow and setting success/error_message based on the actual result, logging after the operation completes (or in an error path).
| fn strip_json_comments(input: &str) -> String { | ||
| let mut out = String::with_capacity(input.len()); | ||
| let bytes = input.as_bytes(); | ||
| let len = bytes.len(); | ||
| let mut i = 0; | ||
|
|
||
| while i < len { | ||
| match bytes[i] { | ||
| b'"' => { | ||
| out.push('"'); | ||
| i += 1; | ||
| while i < len { | ||
| if bytes[i] == b'\\' && i + 1 < len { | ||
| out.push(bytes[i] as char); | ||
| out.push(bytes[i + 1] as char); | ||
| i += 2; | ||
| } else if bytes[i] == b'"' { | ||
| out.push('"'); | ||
| i += 1; | ||
| break; | ||
| } else { | ||
| out.push(bytes[i] as char); | ||
| i += 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
strip_json_comments builds out by pushing bytes[i] as char, which corrupts any non-ASCII UTF-8 in the config (each UTF-8 byte becomes its own Unicode codepoint, e.g. é → é). Consider operating on bytes (e.g., Vec<u8>) and constructing the output with String::from_utf8, or iterating over char_indices() while tracking comment/string state.
| i += 2; | ||
| while i + 1 < len && !(bytes[i] == b'*' && bytes[i + 1] == b'/') { | ||
| i += 1; | ||
| } | ||
| if i + 1 < len { | ||
| i += 2; // skip */ | ||
| } |
There was a problem hiding this comment.
Block comments are removed without leaving any separator. If a /* ... */ appears between two tokens that rely on separation, stripping can concatenate them and change meaning or break parsing (e.g. 1/*c*/2 → 12). Consider emitting at least a single whitespace (and ideally preserving embedded newlines) when skipping block comments.
| i += 2; | |
| while i + 1 < len && !(bytes[i] == b'*' && bytes[i + 1] == b'/') { | |
| i += 1; | |
| } | |
| if i + 1 < len { | |
| i += 2; // skip */ | |
| } | |
| i += 2; | |
| let mut had_newline = false; | |
| while i + 1 < len && !(bytes[i] == b'*' && bytes[i + 1] == b'/') { | |
| if bytes[i] == b'\n' { | |
| out.push('\n'); | |
| had_newline = true; | |
| } | |
| i += 1; | |
| } | |
| if i + 1 < len { | |
| i += 2; // skip */ | |
| } | |
| if !had_newline { | |
| // Ensure at least one separator so tokens are not concatenated | |
| out.push(' '); | |
| } |
| #[test] | ||
| fn test_strip_single_line_comments() { | ||
| let input = r#"{ | ||
| // this is a comment | ||
| "mcpServers": {} | ||
| }"#; | ||
| let stripped = strip_json_comments(input); | ||
| let _: serde_json::Value = serde_json::from_str(&stripped).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_strip_block_comments() { | ||
| let input = r#"{ | ||
| /* block comment */ | ||
| "mcpServers": {} | ||
| }"#; | ||
| let stripped = strip_json_comments(input); | ||
| let _: serde_json::Value = serde_json::from_str(&stripped).unwrap(); | ||
| } |
There was a problem hiding this comment.
The new comment-stripping tests cover //, /* */, and URL preservation, but they don't cover non-ASCII characters or block comments between adjacent tokens (cases where the current implementation can corrupt UTF-8 or concatenate tokens). Adding targeted tests for these scenarios would help prevent regressions.
The MCP CLI had no lightweight way to check server reachability (only --list which fetches all tools), config files broke on // comments that users naturally add, and gh's help format (uppercase headers like "CORE COMMANDS", colon-suffixed names like "auth:") wasn't recognized by the parser.
Health check does connect-handshake-disconnect reusing the existing initialize flow, returning exit 0/1. Config parser now strips single-line and block comments before JSON parsing, respecting string literals so URLs survive. CLI discovery relaxes section header regex to not require trailing colon and accepts colon-suffixed command names, with an indentation guard to prevent description text from false-matching as section headers.
closes #11