fix(doctor): distinguish configuration from live health#4485
Conversation
Keep MCP static validation, protocol readiness, and backend health as distinct evidence in human, JSON, and setup output. Require an explicit local-provider probe so ordinary diagnostics cannot wake a desktop-managed daemon. Refs #4406 Signed-off-by: Hunter B <hmbown@gmail.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @Hmbown's task in 5m 31s —— View job Review of PR #4485
Summary: This is a well-scoped, non-side-effecting change that does exactly what it says: it relabels doctor/setup output so "configuration valid" is never conflated with "process/protocol/backend healthy," and it correctly gates the local-provider live probe ( Two findings posted inline:
Minor/non-blocking:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn doctor_mcp_command_status(server: &McpServerConfig) -> McpCommandDoctorStatus { | ||
| if server.url.is_some() { | ||
| return McpCommandDoctorStatus::NotApplicable; | ||
| } | ||
| let Some(cmd) = server.command.as_deref().map(str::trim) else { | ||
| return McpCommandDoctorStatus::NotChecked; | ||
| }; | ||
| if cmd.is_empty() { | ||
| return McpCommandDoctorStatus::Missing; | ||
| } | ||
|
|
||
| let path = Path::new(cmd); | ||
| let is_absolute = path.is_absolute() || cmd.starts_with('/'); | ||
| if is_absolute { | ||
| return if path.is_file() { | ||
| McpCommandDoctorStatus::Available | ||
| } else { | ||
| McpCommandDoctorStatus::Missing | ||
| }; | ||
| } | ||
|
|
||
| if is_relative_stdio_path_arg(cmd) { | ||
| let Some(cwd) = server.cwd.as_deref() else { | ||
| return McpCommandDoctorStatus::NotChecked; | ||
| }; | ||
| return if cwd.join(path).is_file() { | ||
| McpCommandDoctorStatus::Available | ||
| } else { | ||
| McpCommandDoctorStatus::Missing | ||
| }; | ||
| } | ||
|
|
||
| if is_command_available(cmd) { |
There was a problem hiding this comment.
doctor_mcp_command_status resolves a bare command (e.g. node, npx, uvx) only against the doctor process's own std::env::var_os("PATH"). It never looks at server.env.
That's a real gap: McpServerConfig.env supports a PATH override (allowlisted through child_env::sanitized_mcp_env, see crates/tui/src/child_env.rs:143 and the explicit_child_env_values_win_over_parent_allowlist test), and StdioTransport::spawn (crates/tui/src/mcp/stdio.rs:83-91) applies the expanded server.env — including a PATH override — before spawning. This is a documented, commonly-recommended MCP pattern for GUI-launched apps where the default PATH doesn't include nvm/uv-managed binaries.
With this change, a server that adds "env": {"PATH": "..."} specifically to make its command resolvable will now get a command not found Error from doctor_check_mcp_server (line 6887-6889) even though it would actually launch fine — a false positive that didn't exist before this PR (previously only absolute-path commands were checked for existence at all).
Consider expanding+merging server.env's PATH (mirroring the allowlist/expansion StdioTransport::spawn already does) before doing the PATH search here, or downgrading a PATH-miss to a Warning rather than Error given this blind spot.
| match classify_config_server(server) { | ||
| InventoryStatus::Healthy => { | ||
| healthy += 1; | ||
| configured += 1; |
There was a problem hiding this comment.
classify_config_server (line 387, doc comment: "Safe static probe aligned with doctor_check_mcp_server") is now out of sync with doctor_check_mcp_server/doctor_mcp_command_status in crates/tui/src/main.rs (6797-6889): the doctor path now also validates bare/relative commands against $PATH via is_command_available, but this classifier still only checks is_absolute && !path.exists() (line 400 area).
Net effect: for a stdio server whose command isn't on $PATH, codewhale doctor will now report command not found (Error), while this setup-TUI inventory still classifies the same server as configured/healthy — the two "configuration" evidence sources can disagree about the same server, which cuts against the PR's stated goal of consistent evidence labeling. Consider reusing doctor_mcp_command_status here too, or updating the doc comment to note the two checks are intentionally different in scope.
Follow up on the unresolved review findings from Hmbown#4485 by making doctor and setup resolve MCP stdio commands against the same expanded, sanitized environment used at spawn time. This is a partial Hmbown#4406 health slice: it aligns static configured-command evidence but intentionally does not claim live process reachability, protocol initialization, backend tool health, or provider readiness. Signed-off-by: Hunter B <hmbown@gmail.com>
Summary
Advance #4406 by making the diagnostic evidence boundary truthful without introducing a broad or side-effecting deep probe.
probe_scope,live_health_checked, and per-stage checks including command availability--probe-localis explicitly supplied, because that request may wake a desktop-managed daemon such as Ollamaconfigured; live manager snapshots can sayprotocol_ready; neither is mislabeled as backend healthyThis intentionally references rather than closes #4406. Generic backend/tool invocation, Moraine backend-down versus fully healthy evidence, and inactive-route reporting remain follow-up work.
Refs #4406
Verification
codewhale-tuibinary suite: 7,119 passed, 0 failed, 2 ignoredcargo build -p codewhale-tui --lockeddoctor --helpanddoctor --jsonevidence checkedOnly the known local macOS
__eh_frame section too largelinker warning appeared.