diff --git a/ROADMAP.md b/ROADMAP.md index 49f24a0b..15a94650 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -6395,7 +6395,7 @@ Original filing (2026-04-18): the session emitted `SessionStart hook (completed) 439. **DONE — memory discovery is git-root bounded and reports memory origins** — fixed 2026-06-04 in `fix: bound parent memory discovery`. Project memory discovery now walks only from the current directory up to the nearest git root when one exists, and otherwise stays cwd-local, so stale parent `CLAUDE.md` files outside the project no longer bleed into scratch workspaces. Loaded memory JSON in both `status --output-format json` and `system-prompt --output-format json` now includes `origin`, `scope_path`, and `outside_project` alongside `path`, `source`, `chars`, and `contributes`; origins distinguish `workspace`, `parent_dir`, `ancestor`, `home`, and `outside_project`. The `memory` doctor check warns if an outside-project memory file is ever loaded, while still listing loaded and skipped memory candidates structurally. Docs in `USAGE.md` and `rust/README.md` describe the git-root boundary and expanded JSON fields. Regression coverage: `discovery_stops_at_git_root_boundary_439`, `discovery_without_git_root_stays_cwd_local_439`, and `memory_discovery_stops_at_git_root_and_reports_origins_439`. -440. **One invalid `mcpServers` entry blocks ALL OTHER valid MCP servers from loading — `mcp list --output-format json` returns `configured_servers: 0, servers: []` when even one server has a missing/invalid `command` field, despite other servers in the same config being well-formed; sibling: config parser halts on first invalid entry, never reports the remaining invalid entries** — dogfooded 2026-05-11 by Jobdori on `bd126905` in response to Clawhip pinpoint nudge at `1503343442904879156`. Reproduction: write `.claw.json` containing six `mcpServers` entries — one valid (`valid-server: {command:"/bin/echo", args:["hello"]}`) and five with progressive defects (missing-command, empty-command, null-command, wrong-type-command, extra-unknown-field). Run `claw mcp list --output-format json` → `{"action":"list","config_load_error":"/private/tmp/claw-mcp-probe/.claw.json: mcpServers.missing-command-server: missing string field command","configured_servers":0,"kind":"mcp","servers":[],"status":"degraded"}`. The error mentions only `missing-command-server` (the first invalid entry in JSON-object iteration order); the other four invalid entries are never surfaced. The valid `valid-server` entry is silently dropped because the parser bails on the first error. `status --output-format json` correctly propagates the same `config_load_error` and sets `status:"degraded"`, but no field tells automation which servers are valid vs broken — `servers:[]` is the only signal. **Three problems compounded:** (a) **all-or-nothing loading**: ROADMAP product principle #5 says "partial success is first-class," but mcp config loading is binary. One bad server kills the entire MCP plane; (b) **first-error-only reporting**: a `.claw.json` with five invalid entries surfaces only one error message — the user fixes that one and runs again, gets the next error, and so on. Five iterations needed to discover all errors; (c) **no per-server status**: even with the partial-success fix, the JSON envelope needs `servers:[{name, valid:bool, error?, command?, args?}]` so automation can see which entries are usable. **Required fix shape:** (a) the MCP config parser must collect ALL invalid entries into an `invalid_servers:[{name, error_field, reason}]` array and load all valid ones into `servers:[]`; do not abort on first error; (b) `configured_servers` reflects the count of *valid* loaded servers (not zero) when there are valid entries alongside invalid ones; (c) expose `total_configured:int` (count of entries in source `.claw.json`) AND `valid_count:int` (loaded), AND `invalid_count:int` (rejected) — three distinct counts; (d) `doctor --output-format json` adds an `mcp_validation` check that lists each invalid entry with its error message; (e) regression test: `.claw.json` with one valid + one invalid entry results in `configured_servers: 1, invalid_servers: [{name:"...", reason:"..."}]`. **Why this matters:** users iterate on MCP server lists during onboarding — one typo kills the entire plane, including servers they got working previously. The first-error-only reporting forces N iterations through N invalid entries instead of a single fix-everything-at-once pass. Cross-references #407 (config files no load_error per-file), #415 (config section merged_keys count only), #416 (plugins list prose), #428 (default permission mode), and Product Principle #5. Source: Jobdori live dogfood, `bd126905`, 2026-05-11. +440. **DONE — invalid `mcpServers` siblings no longer drop valid MCP servers** — fixed 2026-06-04 in `fix: load partial MCP configs`. MCP config loading now records every invalid server entry as `invalid_servers:[{name, scope, path, error_field, reason, valid:false}]` while retaining valid siblings in `servers[]`; valid entries carry `valid:true`, `configured_servers` and `valid_count` report loaded valid servers, `invalid_count` reports rejected entries, and `total_configured` reports all discovered entries. `status --output-format json` mirrors the `mcp_validation` summary, and `doctor --output-format json` includes an `mcp validation` check for one-pass repair. Empty stdio commands and unknown per-transport fields are per-server validation errors instead of global config failures. Regression coverage: `loads_valid_mcp_servers_and_collects_all_invalid_siblings_440`, `records_invalid_mcp_server_shapes_without_rejecting_config_440`, `mcp_loads_valid_servers_and_reports_invalid_siblings_440`, and `mcp_degraded_config_and_failed_usage_are_distinct_json_contracts`. 441. **`hooks` config schema diverges from Claude Code documented format — claw-code expects `{"hooks":{"PreToolUse":["command-string"]}}` (array of command strings) while Claude Code documentation specifies `{"hooks":{"PreToolUse":[{"matcher":"Read","hooks":[{"type":"command","command":"..."}]}]}}` (structured matcher objects); users copy-pasting from Claude Code docs see `field "hooks.PreToolUse" must be an array of strings`** — dogfooded 2026-05-11 by Jobdori on `86ff83c2` in response to Clawhip pinpoint nudge at `1503350990680887418`. Reproduction: write `.claw.json` with the Claude-Code-documented hook format `{"hooks":{"PreToolUse":[{"matcher":"Read","hooks":[{"type":"command","command":"/bin/echo pretool"}]}]}}`. Run `claw status --output-format json` → `config_load_error: "/private/tmp/claw-hook-probe/.claw.json: field \"hooks.PreToolUse\" must be an array of strings, got an array (line 3)"`, `status: "degraded"`. The error wording ("must be an array of strings, got an array") is confusingly tautological — the user did provide an array; the parser objects that the array contains objects instead of strings. Replacing with the claw-code-actual format `{"hooks":{"PreToolUse":["/bin/echo pretool"]}}` succeeds: `config_load_error: null, status: "ok"`. The two formats are fundamentally incompatible: claw-code drops the `matcher` field (no tool-specific filtering at the config layer), drops the `type:"command"` discriminator (no future expansion to other hook types), and treats each entry as a bare command string instead of a structured hook spec. **Sibling: PR #3000 (justcode049) was attempting to tolerate object-style hook entries** — that PR's title `fix: tolerate object-style hook entries in config parser` confirms this is a known user complaint, but the PR is still conflicting and unmerged. **Three sibling findings in same probe:** (a) **unknown event names reject entire hooks config**: `.claw.json` with `hooks.InvalidEvent` (not a real event name like `PreToolUse`/`PostToolUse`/`Stop`/`Notification`) triggers `config_load_error: "unknown key \"hooks.InvalidEvent\""` and rejects ALL hooks in the same file, even valid ones — same "one bad apple kills all" pattern as #440 (MCP servers). (b) **`kind:"unknown"` for the validation error** — should be `kind:"invalid_hooks_config"` or `kind:"unknown_hook_event"` (catch-all cluster #422/#423/#424/#428/#430/#431/#432/#433/#435 — 13th occurrence). (c) **first-error-only halting**: a `.claw.json` with `hooks.Stop:"not-an-array"` (type mismatch) AND `hooks.InvalidEvent` (unknown name) AND `hooks.Notification:[{}]` (empty entry) surfaces only the FIRST error in iteration order — user must fix one at a time across 3 iterations. **Required fix shape:** (a) **adopt Claude Code's structured hook format as the canonical**: support `{matcher, hooks:[{type, command}]}` natively, with `matcher` for tool-filtering, `type` for hook-type discriminator (future-proof for `inline`/`webhook`/etc beyond just `command`); (b) **keep backward compat for bare command strings**: legacy `["command-string"]` arrays still load, but emit a deprecation warning suggesting migration to the structured form; (c) **partial-success loading**: invalid hook entries surface in `invalid_hooks:[{event, index, reason}]` while valid ones load — same fix as #440 for MCP; (d) **typed `kind:"invalid_hooks_config"` envelope** instead of `kind:"unknown"`; (e) **rebase and merge PR #3000** which addresses this directly; (f) regression test: Claude-Code-documented hook config loads without error on claw-code. **Why this matters:** users migrating from Claude Code to Claw Code hit this on their first `.claw.json` write. The error message ("array of strings, got an array") is unhelpful; the documentation doesn't surface the schema divergence; and Claude Code's structured format is strictly more expressive (matchers, types) than claw-code's bare-string format. Cross-references #407 (config files no load_error), #410 (list-envelope schema drift), #428 (default permission mode), #440 (one invalid MCP entry blocks all), PR #3000 (justcode049's pending fix). Source: Jobdori live dogfood, `86ff83c2`, 2026-05-11. diff --git a/USAGE.md b/USAGE.md index 26092824..edea2d1e 100644 --- a/USAGE.md +++ b/USAGE.md @@ -570,6 +570,30 @@ Runtime config is loaded in this order, with later entries overriding earlier on The list is also the precedence chain: project-local settings override project settings, project settings override the legacy project `.claw.json`, and project files override user files. `claw --output-format json config` includes each discovered file's `precedence_rank`, `wins_for_keys`, and `shadowed_keys` so automation can see which file controls each effective key without reimplementing the merge order. +## MCP server validation + +`claw mcp --output-format json` loads valid `mcpServers` entries even when sibling entries are malformed. The JSON list envelope distinguishes the total configured entries from the valid and invalid subsets: + +```json +{ + "configured_servers": 1, + "total_configured": 2, + "valid_count": 1, + "invalid_count": 1, + "servers": [{ "name": "valid-server", "valid": true }], + "invalid_servers": [ + { + "name": "missing-command", + "error_field": "command", + "reason": ".claw.json: mcpServers.missing-command: missing string field command", + "valid": false + } + ] +} +``` + +`status --output-format json` mirrors this under `mcp_validation`, and `doctor --output-format json` includes an `mcp validation` check so automation can repair every rejected server entry without losing usable MCP servers. + ## Hook configuration `hooks.PreToolUse`, `hooks.PostToolUse`, and `hooks.PostToolUseFailure` accept either legacy command strings or object-style entries with a `matcher` and nested command hooks: diff --git a/rust/README.md b/rust/README.md index a7e3df9c..a1e609c0 100644 --- a/rust/README.md +++ b/rust/README.md @@ -150,6 +150,7 @@ Top-level commands: `--output-format` accepts `text` or `json` in any casing. `CLAW_OUTPUT_FORMAT=json` selects JSON as the default for non-interactive commands, explicit flags override it, repeated flags warn on stderr, and status JSON exposes `format_source`, `format_raw`, and `format_overridden`. Help and doctor output also surface `CLAW_LOG` / `RUST_LOG` as the logging environment knobs. `claw version --output-format json` is the provenance probe for automation: it reports full `git_sha`, derived `git_sha_short`, `is_dirty`, `branch`, `commit_date`, `commit_timestamp`, `rustc_version`, runtime `executable_path`, and `binary_provenance`; the text report is available as `human_readable` instead of a duplicate `message` field. `status --output-format json` reports loaded project memory files under `workspace.memory_files[]` with each file's `path`, `source` (`claude_md`, `claw_md`, `agents_md`, or scoped/rule sources), `origin`, `scope_path`, `outside_project`, `chars`, and `contributes`; `claw doctor --output-format json` includes a dedicated `memory` check. Root instruction-file priority is `CLAUDE.md`, then `CLAW.md`, then `AGENTS.md`, discovery is bounded to the current git root when present (otherwise cwd only), and all non-duplicate loaded files contribute to the rendered system prompt. +`claw mcp --output-format json` reports partial MCP config success: valid servers remain in `servers[]` while malformed siblings appear in `invalid_servers[]`, with `total_configured`, `valid_count`, and `invalid_count` split out for automation. `status` mirrors this as `mcp_validation`, and doctor includes an `mcp validation` check. Shorthand prompt mode honors the POSIX `--` end-of-flags separator, so `claw -- "-prompt-with-dash"` and unknown dash-prefixed non-flag text stay on the prompt path instead of being treated as CLI options. `claw dump-manifests` is self-contained: it emits the Rust resolver inventory for the selected workspace (commands, tools, agents, skills, and bootstrap phases) without requiring an upstream Claude Code TypeScript checkout. Use `--manifests-dir PATH` only to scope resolver discovery to another directory. diff --git a/rust/crates/commands/src/lib.rs b/rust/crates/commands/src/lib.rs index 314a3598..088a386b 100644 --- a/rust/crates/commands/src/lib.rs +++ b/rust/crates/commands/src/lib.rs @@ -6,8 +6,9 @@ use std::path::{Path, PathBuf}; use plugins::{PluginError, PluginLoadFailure, PluginManager, PluginSummary}; use runtime::{ - compact_session, CompactionConfig, ConfigLoader, ConfigSource, McpOAuthConfig, McpServerConfig, - RuntimeConfig, ScopedMcpServerConfig, Session, + compact_session, CompactionConfig, ConfigLoader, ConfigSource, McpConfigCollection, + McpInvalidServerConfig, McpOAuthConfig, McpServerConfig, RuntimeConfig, ScopedMcpServerConfig, + Session, }; use serde_json::{json, Value}; @@ -3064,24 +3065,16 @@ fn render_mcp_report_for( } match normalize_optional_args(args) { - None | Some("list") => { - // #144: degrade gracefully on config parse failure (same contract - // as #143 for `status`). Text mode prepends a "Config load error" - // block before the MCP list; the list falls back to empty. - match loader.load() { - Ok(runtime_config) => Ok(render_mcp_summary_report( - cwd, - runtime_config.mcp().servers(), - )), - Err(err) => { - let empty = std::collections::BTreeMap::new(); - Ok(format!( - "Config load error\n Status fail\n Summary runtime config failed to load; reporting partial MCP view\n Details {err}\n Hint `claw doctor` classifies config parse errors; fix the listed field and rerun\n\n{}", - render_mcp_summary_report(cwd, &empty) - )) - } + None | Some("list") => match loader.load() { + Ok(runtime_config) => Ok(render_mcp_summary_report(cwd, runtime_config.mcp())), + Err(err) => { + let empty = McpConfigCollection::default(); + Ok(format!( + "Config load error\n Status fail\n Summary runtime config failed to load; reporting partial MCP view\n Details {err}\n Hint `claw doctor` classifies config parse errors; fix the listed field and rerun\n\n{}", + render_mcp_summary_report(cwd, &empty) + )) } - } + }, Some(args) if is_help_arg(args) => Ok(render_mcp_usage(None)), Some("show") => Ok(render_mcp_missing_argument_text("show")), Some(args) if args.split_whitespace().next() == Some("show") => { @@ -3100,7 +3093,7 @@ fn render_mcp_report_for( Ok(runtime_config) => Ok(render_mcp_server_report( cwd, server_name, - runtime_config.mcp().get(server_name), + runtime_config.mcp(), )), Err(err) => Ok(format!( "Config load error\n Status fail\n Summary runtime config failed to load; cannot resolve `{server_name}`\n Details {err}\n Hint `claw doctor` classifies config parse errors; fix the listed field and rerun" @@ -3162,35 +3155,38 @@ fn render_mcp_report_json_for( } match normalize_optional_args(args) { - None | Some("list") => { - // #144: match #143's degraded envelope contract. On config parse - // failure, emit top-level `status: "degraded"` with - // `config_load_error`, empty servers[], and exit 0. On clean - // runs, the existing serializer adds `status: "ok"` below. - match load_runtime_config_without_stderr_warnings(loader) { - Ok(runtime_config) => { - let mut value = - render_mcp_summary_report_json(cwd, runtime_config.mcp().servers()); - if let Some(map) = value.as_object_mut() { - map.insert("status".to_string(), Value::String("ok".to_string())); - map.insert("config_load_error".to_string(), Value::Null); - } - Ok(value) - } - Err(err) => { - let empty = std::collections::BTreeMap::new(); - let mut value = render_mcp_summary_report_json(cwd, &empty); - if let Some(map) = value.as_object_mut() { - map.insert("status".to_string(), Value::String("degraded".to_string())); - map.insert( - "config_load_error".to_string(), - Value::String(err.to_string()), - ); - } - Ok(value) + None | Some("list") => match load_runtime_config_without_stderr_warnings(loader) { + Ok(runtime_config) => { + let mut value = render_mcp_summary_report_json(cwd, runtime_config.mcp()); + if let Some(map) = value.as_object_mut() { + map.insert( + "status".to_string(), + Value::String( + if runtime_config.mcp().has_invalid_servers() { + "degraded" + } else { + "ok" + } + .to_string(), + ), + ); + map.insert("config_load_error".to_string(), Value::Null); } + Ok(value) } - } + Err(err) => { + let empty = McpConfigCollection::default(); + let mut value = render_mcp_summary_report_json(cwd, &empty); + if let Some(map) = value.as_object_mut() { + map.insert("status".to_string(), Value::String("degraded".to_string())); + map.insert( + "config_load_error".to_string(), + Value::String(err.to_string()), + ); + } + Ok(value) + } + }, Some(args) if is_help_arg(args) => Ok(render_mcp_usage_json(None)), Some("show") => Ok(render_mcp_missing_argument_json("show")), Some(args) if args.split_whitespace().next() == Some("show") => { @@ -3205,16 +3201,21 @@ fn render_mcp_report_json_for( // #144: same degradation pattern for show action. match load_runtime_config_without_stderr_warnings(loader) { Ok(runtime_config) => { - let mut value = render_mcp_server_report_json( - cwd, - server_name, - runtime_config.mcp().get(server_name), - ); + let mut value = + render_mcp_server_report_json(cwd, server_name, runtime_config.mcp()); if let Some(map) = value.as_object_mut() { - // Only override status to "ok" if the server was found; - // render_mcp_server_report_json already sets status:"error" for not-found. if map.get("found") == Some(&Value::Bool(true)) { - map.insert("status".to_string(), Value::String("ok".to_string())); + map.insert( + "status".to_string(), + Value::String( + if runtime_config.mcp().has_invalid_servers() { + "degraded" + } else { + "ok" + } + .to_string(), + ), + ); } map.insert("config_load_error".to_string(), Value::Null); } @@ -4426,55 +4427,80 @@ fn io_error_reason(error: &std::io::Error) -> &'static str { } } -fn render_mcp_summary_report( - cwd: &Path, - servers: &BTreeMap, -) -> String { +fn render_mcp_summary_report(cwd: &Path, mcp: &McpConfigCollection) -> String { + let servers = mcp.servers(); let mut lines = vec![ "MCP".to_string(), format!(" Working directory {}", cwd.display()), - format!(" Configured servers {}", servers.len()), + format!(" Configured servers {}", mcp.valid_count()), + format!(" Total entries {}", mcp.total_configured()), + format!(" Invalid entries {}", mcp.invalid_count()), ]; if servers.is_empty() { - lines.push(" No MCP servers configured.".to_string()); - return lines.join("\n"); + lines.push(" No valid MCP servers configured.".to_string()); } - lines.push(String::new()); - for (name, server) in servers { - lines.push(format!( - " {name:<16} {transport:<13} {scope:<7} {summary}", - transport = mcp_transport_label(&server.config), - scope = config_source_label(server.scope), - summary = mcp_server_summary(&server.config) - )); + if !servers.is_empty() { + lines.push(String::new()); + for (name, server) in servers { + lines.push(format!( + " {name:<16} {transport:<13} {scope:<7} {summary}", + transport = mcp_transport_label(&server.config), + scope = config_source_label(server.scope), + summary = mcp_server_summary(&server.config) + )); + } + } + + if !mcp.invalid_servers().is_empty() { + lines.push(String::new()); + lines.push(" Invalid MCP servers".to_string()); + for invalid in mcp.invalid_servers() { + lines.push(format!(" - {}: {}", invalid.name, invalid.reason)); + } } lines.join("\n") } -fn render_mcp_summary_report_json( - cwd: &Path, - servers: &BTreeMap, -) -> Value { +fn render_mcp_summary_report_json(cwd: &Path, mcp: &McpConfigCollection) -> Value { json!({ "kind": "mcp", "action": "list", "working_directory": cwd.display().to_string(), - "configured_servers": servers.len(), - "servers": servers + "configured_servers": mcp.valid_count(), + "total_configured": mcp.total_configured(), + "valid_count": mcp.valid_count(), + "invalid_count": mcp.invalid_count(), + "invalid_servers": invalid_mcp_servers_json(mcp.invalid_servers()), + "servers": mcp + .servers() .iter() .map(|(name, server)| mcp_server_json(name, server)) .collect::>(), }) } -fn render_mcp_server_report( - cwd: &Path, - server_name: &str, - server: Option<&ScopedMcpServerConfig>, -) -> String { - let Some(server) = server else { +fn invalid_mcp_servers_json(invalid_servers: &[McpInvalidServerConfig]) -> Value { + Value::Array( + invalid_servers + .iter() + .map(|server| { + json!({ + "name": &server.name, + "scope": config_source_json(server.scope), + "path": server.path.display().to_string(), + "error_field": &server.error_field, + "reason": &server.reason, + "valid": false, + }) + }) + .collect::>(), + ) +} + +fn render_mcp_server_report(cwd: &Path, server_name: &str, mcp: &McpConfigCollection) -> String { + let Some(server) = mcp.get(server_name) else { return format!( "MCP\n Working directory {}\n Result server `{server_name}` is not configured", cwd.display() @@ -4552,9 +4578,9 @@ fn render_mcp_server_report( fn render_mcp_server_report_json( cwd: &Path, server_name: &str, - server: Option<&ScopedMcpServerConfig>, + mcp: &McpConfigCollection, ) -> Value { - match server { + match mcp.get(server_name) { Some(server) => json!({ "kind": "mcp", "action": "show", @@ -4562,6 +4588,10 @@ fn render_mcp_server_report_json( "working_directory": cwd.display().to_string(), "found": true, "server": mcp_server_json(server_name, server), + "total_configured": mcp.total_configured(), + "valid_count": mcp.valid_count(), + "invalid_count": mcp.invalid_count(), + "invalid_servers": invalid_mcp_servers_json(mcp.invalid_servers()), }), None => json!({ "kind": "mcp", @@ -4574,6 +4604,10 @@ fn render_mcp_server_report_json( "message": format!("server `{server_name}` is not configured"), // #761: hint so callers know how to enumerate configured MCP servers "hint": "Run `claw mcp list` to see configured servers.", + "total_configured": mcp.total_configured(), + "valid_count": mcp.valid_count(), + "invalid_count": mcp.invalid_count(), + "invalid_servers": invalid_mcp_servers_json(mcp.invalid_servers()), }), } } @@ -4967,6 +5001,7 @@ fn mcp_server_details_json(config: &McpServerConfig) -> Value { fn mcp_server_json(name: &str, server: &ScopedMcpServerConfig) -> Value { json!({ "name": name, + "valid": true, "required": server.required, "scope": config_source_json(server.scope), "transport": mcp_transport_json(&server.config), @@ -6619,12 +6654,9 @@ mod tests { } #[test] - fn mcp_degrades_gracefully_on_malformed_mcp_config_144() { - // #144: mirror of #143's partial-success contract for `claw mcp`. - // Previously `mcp` hard-failed on any config parse error, hiding - // well-formed servers and forcing claws to fall back to `doctor`. - // Now `mcp` emits a degraded envelope instead: exit 0, status: - // "degraded", config_load_error populated, servers[] empty. + fn mcp_loads_valid_servers_and_reports_invalid_siblings_440() { + // #440: invalid sibling MCP entries must not drop valid servers, and + // the JSON envelope must expose all rejected entries for one-pass repair. let _guard = env_guard(); let workspace = temp_dir("mcp-degrades-144"); let config_home = temp_dir("mcp-degrades-144-cfg"); @@ -6654,17 +6686,19 @@ mod tests { Some("degraded"), "top-level status should be 'degraded': {list}" ); - let err = list["config_load_error"] + assert!(list["config_load_error"].is_null()); + assert_eq!(list["configured_servers"], 1); + assert_eq!(list["total_configured"], 2); + assert_eq!(list["valid_count"], 1); + assert_eq!(list["invalid_count"], 1); + assert_eq!(list["servers"][0]["name"], "everything"); + assert_eq!(list["servers"][0]["valid"], true); + assert_eq!(list["invalid_servers"][0]["name"], "missing-command"); + assert!(list["invalid_servers"][0]["reason"] .as_str() - .expect("config_load_error must be a string on degraded runs"); - assert!( - err.contains("mcpServers.missing-command"), - "config_load_error should name the malformed field path: {err}" - ); - assert_eq!(list["configured_servers"], 0); - assert!(list["servers"].as_array().unwrap().is_empty()); + .is_some_and(|reason| reason.contains("missing string field command"))); - // show action: should also degrade (not hard-fail). + // show action still resolves valid siblings while carrying validation metadata. let show = render_mcp_report_json_for(&loader, &workspace, Some("show everything")) .expect("mcp show should not hard-fail on config parse errors (#144)"); assert_eq!(show["kind"], "mcp"); @@ -6674,7 +6708,11 @@ mod tests { Some("degraded"), "show action should also report status: 'degraded': {show}" ); - assert!(show["config_load_error"].is_string()); + assert!(show["config_load_error"].is_null()); + assert_eq!(show["found"], true); + assert_eq!(show["server"]["name"], "everything"); + assert_eq!(show["server"]["valid"], true); + assert_eq!(show["invalid_count"], 1); // Clean path: status: "ok", config_load_error: null. let clean_ws = temp_dir("mcp-degrades-144-clean"); diff --git a/rust/crates/runtime/src/config.rs b/rust/crates/runtime/src/config.rs index d03b5bc7..806c3ed5 100644 --- a/rust/crates/runtime/src/config.rs +++ b/rust/crates/runtime/src/config.rs @@ -207,9 +207,19 @@ pub struct RuntimePermissionRuleConfig { #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct McpConfigCollection { servers: BTreeMap, + invalid_servers: Vec, + total_configured: usize, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct McpInvalidServerConfig { + pub name: String, + pub scope: ConfigSource, + pub path: PathBuf, + pub error_field: String, + pub reason: String, } -/// MCP server config paired with the scope that defined it. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ScopedMcpServerConfig { pub required: bool, @@ -386,7 +396,7 @@ impl ConfigLoader { pub fn load(&self) -> Result { let mut merged = BTreeMap::new(); let mut loaded_entries = Vec::new(); - let mut mcp_servers = BTreeMap::new(); + let mut mcp = McpConfigCollection::default(); let mut all_warnings = Vec::new(); for entry in self.discover() { @@ -405,7 +415,7 @@ impl ConfigLoader { } all_warnings.extend(validation.warnings); validate_optional_hooks_config(&parsed.object, &entry.path)?; - merge_mcp_servers(&mut mcp_servers, entry.source, &parsed.object, &entry.path)?; + merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path)?; deep_merge_objects(&mut merged, &parsed.object); loaded_entries.push(entry); } @@ -414,7 +424,7 @@ impl ConfigLoader { emit_config_warning_once(&warning.to_string()); } - build_runtime_config(merged, loaded_entries, mcp_servers) + build_runtime_config(merged, loaded_entries, mcp) } /// Like [`load`] but also returns the list of validation warnings collected during @@ -425,7 +435,7 @@ impl ConfigLoader { pub fn load_collecting_warnings(&self) -> Result<(RuntimeConfig, Vec), ConfigError> { let mut merged = BTreeMap::new(); let mut loaded_entries = Vec::new(); - let mut mcp_servers = BTreeMap::new(); + let mut mcp = McpConfigCollection::default(); let mut all_warnings: Vec = Vec::new(); for entry in self.discover() { @@ -444,12 +454,12 @@ impl ConfigLoader { } all_warnings.extend(validation.warnings.iter().map(|w| w.to_string())); validate_optional_hooks_config(&parsed.object, &entry.path)?; - merge_mcp_servers(&mut mcp_servers, entry.source, &parsed.object, &entry.path)?; + merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path)?; deep_merge_objects(&mut merged, &parsed.object); loaded_entries.push(entry); } - let config = build_runtime_config(merged, loaded_entries, mcp_servers)?; + let config = build_runtime_config(merged, loaded_entries, mcp)?; Ok((config, all_warnings)) } @@ -462,7 +472,7 @@ impl ConfigLoader { pub fn inspect_collecting_warnings(&self) -> ConfigInspection { let mut merged = BTreeMap::new(); let mut loaded_entries = Vec::new(); - let mut mcp_servers = BTreeMap::new(); + let mut mcp = McpConfigCollection::default(); let mut warnings = Vec::new(); let mut files = Vec::new(); let mut load_error = None; @@ -546,7 +556,7 @@ impl ConfigLoader { } if let Err(error) = - merge_mcp_servers(&mut mcp_servers, entry.source, &parsed.object, &entry.path) + merge_mcp_servers(&mut mcp, entry.source, &parsed.object, &entry.path) { let detail = error.to_string(); load_error.get_or_insert_with(|| detail.clone()); @@ -567,7 +577,7 @@ impl ConfigLoader { annotate_config_file_precedence(&mut files); - let runtime_config = match build_runtime_config(merged, loaded_entries, mcp_servers) { + let runtime_config = match build_runtime_config(merged, loaded_entries, mcp) { Ok(config) => Some(config), Err(error) => { load_error.get_or_insert_with(|| error.to_string()); @@ -703,16 +713,14 @@ fn collect_config_key_paths_for_value(prefix: &str, value: &JsonValue, keys: &mu fn build_runtime_config( merged: BTreeMap, loaded_entries: Vec, - mcp_servers: BTreeMap, + mcp: McpConfigCollection, ) -> Result { let merged_value = JsonValue::Object(merged.clone()); let feature_config = RuntimeFeatureConfig { hooks: parse_optional_hooks_config(&merged_value)?, plugins: parse_optional_plugin_config(&merged_value)?, - mcp: McpConfigCollection { - servers: mcp_servers, - }, + mcp, oauth: parse_optional_oauth_config(&merged_value, "merged settings.oauth")?, model: parse_optional_model(&merged_value), aliases: parse_optional_aliases(&merged_value)?, @@ -1330,6 +1338,31 @@ impl McpConfigCollection { &self.servers } + #[must_use] + pub fn invalid_servers(&self) -> &[McpInvalidServerConfig] { + &self.invalid_servers + } + + #[must_use] + pub fn total_configured(&self) -> usize { + self.total_configured + } + + #[must_use] + pub fn valid_count(&self) -> usize { + self.servers.len() + } + + #[must_use] + pub fn invalid_count(&self) -> usize { + self.invalid_servers.len() + } + + #[must_use] + pub fn has_invalid_servers(&self) -> bool { + !self.invalid_servers.is_empty() + } + #[must_use] pub fn get(&self, name: &str) -> Option<&ScopedMcpServerConfig> { self.servers.get(name) @@ -1421,7 +1454,7 @@ fn read_optional_json_object(path: &Path) -> Result, + target: &mut McpConfigCollection, source: ConfigSource, root: &BTreeMap, path: &Path, @@ -1430,21 +1463,48 @@ fn merge_mcp_servers( return Ok(()); }; let servers = expect_object(mcp_servers, &format!("{}: mcpServers", path.display()))?; + target.total_configured += servers.len(); for (name, value) in servers { - let parsed = parse_mcp_server_config( - name, - value, - &format!("{}: mcpServers.{name}", path.display()), - )?; - target.insert( + let context = format!("{}: mcpServers.{name}", path.display()); + let Ok(object) = expect_object(value, &context) else { + let error = expect_object(value, &context).expect_err("object parse must fail"); + target.servers.remove(name); + target + .invalid_servers + .push(mcp_invalid_server(name, source, path, &context, &error)); + continue; + }; + let required = match optional_bool(object, "required", &context) { + Ok(required) => required.unwrap_or(false), + Err(error) => { + target.servers.remove(name); + target + .invalid_servers + .push(mcp_invalid_server(name, source, path, &context, &error)); + continue; + } + }; + if let Err(error) = validate_mcp_server_keys(name, object, &context) { + target.servers.remove(name); + target + .invalid_servers + .push(mcp_invalid_server(name, source, path, &context, &error)); + continue; + } + let parsed = match parse_mcp_server_config(name, value, &context) { + Ok(parsed) => parsed, + Err(error) => { + target.servers.remove(name); + target + .invalid_servers + .push(mcp_invalid_server(name, source, path, &context, &error)); + continue; + } + }; + target.servers.insert( name.clone(), ScopedMcpServerConfig { - required: optional_bool( - expect_object(value, &format!("{}: mcpServers.{name}", path.display()))?, - "required", - &format!("{}: mcpServers.{name}", path.display()), - )? - .unwrap_or(false), + required, scope: source, config: parsed, }, @@ -1453,6 +1513,98 @@ fn merge_mcp_servers( Ok(()) } +fn mcp_invalid_server( + name: &str, + source: ConfigSource, + path: &Path, + context: &str, + error: &ConfigError, +) -> McpInvalidServerConfig { + let reason = config_error_detail(error); + McpInvalidServerConfig { + name: name.to_string(), + scope: source, + path: path.to_path_buf(), + error_field: mcp_error_field(name, context, &reason), + reason, + } +} + +fn config_error_detail(error: &ConfigError) -> String { + match error { + ConfigError::Io(error) => error.to_string(), + ConfigError::Parse(reason) => reason.clone(), + } +} + +fn mcp_error_field(name: &str, context: &str, reason: &str) -> String { + if let Some(field) = reason + .split("missing string field ") + .nth(1) + .and_then(|tail| tail.split_whitespace().next()) + { + return field + .trim_matches(|ch: char| !ch.is_ascii_alphanumeric() && ch != '_') + .to_string(); + } + if let Some(field) = reason + .split("field ") + .nth(1) + .and_then(|tail| tail.split_whitespace().next()) + { + return field + .trim_matches(|ch: char| !ch.is_ascii_alphanumeric() && ch != '_') + .to_string(); + } + reason + .split_once(context) + .and_then(|(_, tail)| tail.trim_start_matches('.').split(':').next()) + .filter(|field| !field.is_empty()) + .map(str::to_string) + .unwrap_or_else(|| format!("mcpServers.{name}")) +} + +fn validate_mcp_server_keys( + server_name: &str, + object: &BTreeMap, + context: &str, +) -> Result<(), ConfigError> { + let server_type = + optional_string(object, "type", context)?.unwrap_or_else(|| infer_mcp_server_type(object)); + let allowed = match server_type { + "stdio" => &[ + "type", + "command", + "args", + "env", + "toolCallTimeoutMs", + "required", + ][..], + "sse" | "http" => &[ + "type", + "url", + "headers", + "headersHelper", + "oauth", + "required", + ][..], + "ws" => &["type", "url", "headers", "headersHelper", "required"][..], + "sdk" => &["type", "name", "required"][..], + "claudeai-proxy" => &["type", "url", "id", "required"][..], + other => { + return Err(ConfigError::Parse(format!( + "{context}: unsupported MCP server type for {server_name}: {other}" + ))); + } + }; + if let Some(key) = object.keys().find(|key| !allowed.contains(&key.as_str())) { + return Err(ConfigError::Parse(format!( + "{context}: unknown MCP server field {key}" + ))); + } + Ok(()) +} + fn parse_optional_model(root: &JsonValue) -> Option { root.as_object() .and_then(|object| object.get("model")) @@ -1719,7 +1871,7 @@ fn parse_mcp_server_config( optional_string(object, "type", context)?.unwrap_or_else(|| infer_mcp_server_type(object)); match server_type { "stdio" => Ok(McpServerConfig::Stdio(McpStdioServerConfig { - command: expect_string(object, "command", context)?.to_string(), + command: expect_non_empty_string(object, "command", context)?.to_string(), args: optional_string_array(object, "args", context)?.unwrap_or_default(), env: optional_string_map(object, "env", context)?.unwrap_or_default(), tool_call_timeout_ms: optional_u64(object, "toolCallTimeoutMs", context)?, @@ -1794,6 +1946,20 @@ fn expect_object<'a>( .ok_or_else(|| ConfigError::Parse(format!("{context}: expected JSON object"))) } +fn expect_non_empty_string<'a>( + object: &'a BTreeMap, + key: &str, + context: &str, +) -> Result<&'a str, ConfigError> { + let value = expect_string(object, key, context)?; + if value.trim().is_empty() { + return Err(ConfigError::Parse(format!( + "{context}: field {key} must be a non-empty string" + ))); + } + Ok(value) +} + fn expect_string<'a>( object: &'a BTreeMap, key: &str, @@ -2843,7 +3009,7 @@ mod tests { } #[test] - fn rejects_invalid_mcp_server_shapes() { + fn records_invalid_mcp_server_shapes_without_rejecting_config_440() { // given let root = temp_dir(); let cwd = root.join("project"); @@ -2857,18 +3023,72 @@ mod tests { .expect("write broken settings"); // when - let error = ConfigLoader::new(&cwd, &home) + let loaded = ConfigLoader::new(&cwd, &home) .load() - .expect_err("config should fail"); + .expect("invalid MCP entries should not block otherwise loadable config"); // then - assert!(error - .to_string() + assert!(loaded.mcp().servers().is_empty()); + assert_eq!(loaded.mcp().total_configured(), 1); + assert_eq!(loaded.mcp().invalid_count(), 1); + let invalid = &loaded.mcp().invalid_servers()[0]; + assert_eq!(invalid.name, "broken"); + assert_eq!(invalid.error_field, "url"); + assert!(invalid + .reason .contains("mcpServers.broken: missing string field url")); fs::remove_dir_all(root).expect("cleanup temp dir"); } + #[test] + fn loads_valid_mcp_servers_and_collects_all_invalid_siblings_440() { + let root = temp_dir(); + let cwd = root.join("project"); + let home = root.join("home").join(".claw"); + fs::create_dir_all(&home).expect("home config dir"); + fs::create_dir_all(&cwd).expect("project dir"); + fs::write( + home.join("settings.json"), + r#"{ + "mcpServers": { + "valid-server": {"command": "/bin/echo", "args": ["hello"]}, + "missing-command": {"args": ["arg-only"]}, + "empty-command": {"command": ""}, + "wrong-type-command": {"command": 42}, + "extra-unknown-field": {"command": "/bin/echo", "extra": true} + } + }"#, + ) + .expect("write mixed settings"); + + let loaded = ConfigLoader::new(&cwd, &home) + .load() + .expect("valid MCP entries should load beside invalid siblings"); + + assert_eq!(loaded.mcp().total_configured(), 5); + assert_eq!(loaded.mcp().valid_count(), 1); + assert_eq!(loaded.mcp().invalid_count(), 4); + assert!(loaded.mcp().get("valid-server").is_some()); + let invalid_names = loaded + .mcp() + .invalid_servers() + .iter() + .map(|server| server.name.as_str()) + .collect::>(); + assert_eq!( + invalid_names, + vec![ + "empty-command", + "extra-unknown-field", + "missing-command", + "wrong-type-command", + ] + ); + + fs::remove_dir_all(root).expect("cleanup temp dir"); + } + #[test] fn parses_user_defined_model_aliases_from_settings() { // given diff --git a/rust/crates/runtime/src/lib.rs b/rust/crates/runtime/src/lib.rs index b54dedfb..e11b91d8 100644 --- a/rust/crates/runtime/src/lib.rs +++ b/rust/crates/runtime/src/lib.rs @@ -67,11 +67,12 @@ pub use compact::{ pub use config::{ suppress_config_warnings_for_json_mode, ConfigEntry, ConfigError, ConfigFileReport, ConfigFileStatus, ConfigInspection, ConfigLoader, ConfigSource, McpConfigCollection, - McpManagedProxyServerConfig, McpOAuthConfig, McpRemoteServerConfig, McpSdkServerConfig, - McpServerConfig, McpStdioServerConfig, McpTransport, McpWebSocketServerConfig, OAuthConfig, - ProviderFallbackConfig, ResolvedPermissionMode, RulesImportConfig, RuntimeConfig, - RuntimeFeatureConfig, RuntimeHookCommand, RuntimeHookConfig, RuntimePermissionRuleConfig, - RuntimePluginConfig, ScopedMcpServerConfig, CLAW_SETTINGS_SCHEMA_NAME, + McpInvalidServerConfig, McpManagedProxyServerConfig, McpOAuthConfig, McpRemoteServerConfig, + McpSdkServerConfig, McpServerConfig, McpStdioServerConfig, McpTransport, + McpWebSocketServerConfig, OAuthConfig, ProviderFallbackConfig, ResolvedPermissionMode, + RulesImportConfig, RuntimeConfig, RuntimeFeatureConfig, RuntimeHookCommand, RuntimeHookConfig, + RuntimePermissionRuleConfig, RuntimePluginConfig, ScopedMcpServerConfig, + CLAW_SETTINGS_SCHEMA_NAME, }; pub use config_validate::{ check_unsupported_format, format_diagnostics, validate_config_file, ConfigDiagnostic, diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 1f45f339..a4f14a2e 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -56,10 +56,10 @@ use runtime::{ load_system_prompt, load_system_prompt_with_context, pricing_for_model, resolve_expected_base, resolve_sandbox_status, ApiClient, ApiRequest, AssistantEvent, BaseCommitState, CompactionConfig, ConfigFileReport, ConfigLoader, ConfigSource, ContentBlock, ContextFile, - ConversationMessage, ConversationRuntime, McpServer, McpServerManager, McpServerSpec, McpTool, - MessageRole, ModelPricing, PermissionMode, PermissionPolicy, ProjectContext, PromptCacheEvent, - ResolvedPermissionMode, RuntimeError, Session, TokenUsage, ToolError, ToolExecutor, - UsageTracker, + ConversationMessage, ConversationRuntime, McpConfigCollection, McpInvalidServerConfig, + McpServer, McpServerManager, McpServerSpec, McpTool, MessageRole, ModelPricing, PermissionMode, + PermissionPolicy, ProjectContext, PromptCacheEvent, ResolvedPermissionMode, RuntimeError, + Session, TokenUsage, ToolError, ToolExecutor, UsageTracker, }; use serde::Deserialize; use serde_json::{json, Map, Value}; @@ -3498,6 +3498,11 @@ fn render_doctor_report( project_root.as_deref(), &project_context.instruction_files, ); + let mcp_validation = config + .as_ref() + .ok() + .map(|runtime_config| McpValidationSummary::from_collection(runtime_config.mcp())) + .unwrap_or_default(); let context = StatusContext { cwd: cwd.clone(), session_path: None, @@ -3526,11 +3531,13 @@ fn render_doctor_report( // fed into health renderers that don't read config_load_error. config_load_error: config.as_ref().err().map(ToString::to_string), config_load_error_kind: None, + mcp_validation: mcp_validation.clone(), }; Ok(DoctorReport { checks: vec![ check_auth_health(), check_config_health(&config_loader, config.as_ref()), + check_mcp_validation_health(&mcp_validation), check_install_source_health(), check_workspace_health(&context), check_memory_health(&context), @@ -3791,8 +3798,14 @@ fn check_config_health( } details.push(format!( "MCP servers {}", - runtime_config.mcp().servers().len() + runtime_config.mcp().valid_count() )); + if runtime_config.mcp().invalid_count() > 0 { + details.push(format!( + "MCP invalid {}", + runtime_config.mcp().invalid_count() + )); + } if present_paths.is_empty() { details.push("Discovered files (defaults active)".to_string()); } else { @@ -3819,7 +3832,11 @@ fn check_config_health( ("resolved_model".to_string(), json!(runtime_config.model())), ( "mcp_servers".to_string(), - json!(runtime_config.mcp().servers().len()), + json!(runtime_config.mcp().valid_count()), + ), + ( + "mcp_invalid_servers".to_string(), + json!(runtime_config.mcp().invalid_count()), ), ])) } @@ -3851,6 +3868,56 @@ fn check_config_health( } } +fn check_mcp_validation_health(summary: &McpValidationSummary) -> DiagnosticCheck { + let mut details = vec![ + format!("Total entries {}", summary.total_configured), + format!("Valid entries {}", summary.valid_count), + format!("Invalid entries {}", summary.invalid_count()), + ]; + details.extend( + summary + .invalid_servers + .iter() + .map(|server| format!("Invalid server {} ({})", server.name, server.reason)), + ); + + DiagnosticCheck::new( + "MCP validation", + if summary.has_invalid_servers() { + DiagnosticLevel::Warn + } else { + DiagnosticLevel::Ok + }, + if summary.has_invalid_servers() { + format!( + "{} MCP server entries are invalid; {} valid entries remain loaded", + summary.invalid_count(), + summary.valid_count + ) + } else { + format!("{} MCP server entries validated", summary.valid_count) + }, + ) + .with_hint(if summary.has_invalid_servers() { + "Inspect `claw mcp list --output-format json` invalid_servers and fix each rejected mcpServers entry." + } else { + "" + }) + .with_details(details) + .with_data(Map::from_iter([ + ( + "total_configured".to_string(), + json!(summary.total_configured), + ), + ("valid_count".to_string(), json!(summary.valid_count)), + ("invalid_count".to_string(), json!(summary.invalid_count())), + ( + "invalid_servers".to_string(), + Value::Array(invalid_mcp_servers_json(&summary.invalid_servers)), + ), + ])) +} + fn check_permission_health(permission_mode: PermissionModeProvenance) -> DiagnosticCheck { let mode = permission_mode.mode.as_str(); let source = permission_mode.source.as_str(); @@ -4796,6 +4863,65 @@ impl MemoryFileSummary { } } +#[derive(Debug, Clone, Default, PartialEq, Eq)] +struct McpValidationSummary { + total_configured: usize, + valid_count: usize, + invalid_servers: Vec, +} + +impl McpValidationSummary { + fn from_collection(collection: &McpConfigCollection) -> Self { + Self { + total_configured: collection.total_configured(), + valid_count: collection.valid_count(), + invalid_servers: collection.invalid_servers().to_vec(), + } + } + + fn invalid_count(&self) -> usize { + self.invalid_servers.len() + } + + fn has_invalid_servers(&self) -> bool { + !self.invalid_servers.is_empty() + } + + fn json_value(&self) -> serde_json::Value { + json!({ + "total_configured": self.total_configured, + "valid_count": self.valid_count, + "invalid_count": self.invalid_count(), + "invalid_servers": invalid_mcp_servers_json(&self.invalid_servers), + }) + } +} + +fn invalid_mcp_servers_json(invalid_servers: &[McpInvalidServerConfig]) -> Vec { + invalid_servers + .iter() + .map(|server| { + json!({ + "name": &server.name, + "scope": config_source_json_value(server.scope), + "path": server.path.display().to_string(), + "error_field": &server.error_field, + "reason": &server.reason, + "valid": false, + }) + }) + .collect() +} + +fn config_source_json_value(source: ConfigSource) -> serde_json::Value { + let id = match source { + ConfigSource::User => "user", + ConfigSource::Project => "project", + ConfigSource::Local => "local", + }; + json!({"id": id, "label": id}) +} + fn memory_file_summaries_for( cwd: &Path, project_root: Option<&Path>, @@ -4933,6 +5059,7 @@ struct StatusContext { /// readable string so downstream claws can switch on the kind token /// instead of regex-scraping the prose. config_load_error_kind: Option<&'static str>, + mcp_validation: McpValidationSummary, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -6132,6 +6259,7 @@ fn run_resume_command( "load_failures": payload.load_failures.len(), }, "config_load_error": payload.config_load_error, + "mcp_validation": payload.mcp_validation.json_value(), "plugins": payload.plugins, "load_failures": payload.load_failures, }); @@ -8040,6 +8168,7 @@ impl LiveCli { "load_failures": payload.load_failures.len(), }, "config_load_error": payload.config_load_error, + "mcp_validation": payload.mcp_validation.json_value(), "plugins": filtered_plugins, "load_failures": payload.load_failures, }); @@ -8843,9 +8972,10 @@ fn status_json_value( json!({ "kind": "status", "action": "show", - "status": if degraded { "degraded" } else { "ok" }, + "status": if degraded || context.mcp_validation.has_invalid_servers() { "degraded" } else { "ok" }, "config_load_error": context.config_load_error, "config_load_error_kind": context.config_load_error_kind, + "mcp_validation": context.mcp_validation.json_value(), "model": model, "model_source": model_source, "model_raw": model_raw, @@ -8912,6 +9042,7 @@ fn status_json_value( "memory_file_count": context.memory_file_count, "memory_files": memory_files_json(&context.memory_files), "unloaded_memory_files": context.unloaded_memory_files, + "mcp_validation": context.mcp_validation.json_value(), }, "sandbox": { "enabled": context.sandbox_status.enabled, @@ -8985,6 +9116,11 @@ fn status_context( project_root.as_deref(), &project_context.instruction_files, ); + let mcp_validation = runtime_config + .as_ref() + .ok() + .map(|runtime_config| McpValidationSummary::from_collection(runtime_config.mcp())) + .unwrap_or_default(); Ok(StatusContext { cwd: cwd.clone(), session_path: session_path.map(Path::to_path_buf), @@ -9008,6 +9144,7 @@ fn status_context( binary_provenance: binary_provenance_for(Some(&cwd)), config_load_error, config_load_error_kind, + mcp_validation, }) } @@ -9590,7 +9727,7 @@ fn render_doctor_help_json() -> serde_json::Value { "requires_session_resume": false, "mutates_workspace": false, "output_fields": ["kind", "action", "status", "message", "report", "has_failures", "summary", "checks", "allowed_tools"], - "check_names": ["auth", "config", "install source", "workspace", "memory", "boot preflight", "sandbox", "permissions", "system"], + "check_names": ["auth", "config", "mcp validation", "install source", "workspace", "memory", "boot preflight", "sandbox", "permissions", "system"], "status_values": ["ok", "warn", "fail"], "options": [ { @@ -10920,6 +11057,7 @@ struct PluginsCommandPayload { reload_runtime: bool, status: &'static str, config_load_error: Option, + mcp_validation: McpValidationSummary, plugins: Vec, load_failures: Vec, } @@ -10932,9 +11070,16 @@ fn plugins_command_payload_for( ) -> Result> { let loader = ConfigLoader::default_for(cwd); let loaded_config = load_config_with_warning_mode(&loader, config_warning_mode); - let (runtime_config, config_load_error) = match loaded_config { - Ok(runtime_config) => (runtime_config, None), - Err(error) => (runtime::RuntimeConfig::empty(), Some(error.to_string())), + let (runtime_config, config_load_error, mcp_validation) = match loaded_config { + Ok(runtime_config) => { + let mcp_validation = McpValidationSummary::from_collection(runtime_config.mcp()); + (runtime_config, None, mcp_validation) + } + Err(error) => ( + runtime::RuntimeConfig::empty(), + Some(error.to_string()), + McpValidationSummary::default(), + ), }; let mut manager = build_plugin_manager(cwd, &loader, &runtime_config); let result = handle_plugins_slash_command(action, target, &mut manager)?; @@ -10942,6 +11087,7 @@ fn plugins_command_payload_for( Ok(plugins_command_payload_from_result( result, config_load_error, + mcp_validation, &report, )) } @@ -10949,10 +11095,14 @@ fn plugins_command_payload_for( fn plugins_command_payload_from_result( result: PluginsCommandResult, config_load_error: Option, + mcp_validation: McpValidationSummary, report: &plugins::PluginRegistryReport, ) -> PluginsCommandPayload { let failures = report.failures(); - let status = if config_load_error.is_some() || !failures.is_empty() { + let status = if config_load_error.is_some() + || mcp_validation.has_invalid_servers() + || !failures.is_empty() + { "degraded" } else { "ok" @@ -10962,6 +11112,11 @@ fn plugins_command_payload_from_result( "Config load error\n Status fail\n Summary runtime config failed to load; reporting partial plugins view\n Details {error}\n Hint `claw doctor` classifies config parse errors; fix the listed field and rerun\n\n{}", result.message ), + None if mcp_validation.has_invalid_servers() => format!( + "MCP validation\n Status warn\n Summary {} MCP server entries are invalid; reporting plugins with valid MCP siblings only\n Hint Inspect `claw mcp list --output-format json` invalid_servers and fix each rejected mcpServers entry.\n\n{}", + mcp_validation.invalid_count(), + result.message + ), None => result.message, }; PluginsCommandPayload { @@ -10969,6 +11124,7 @@ fn plugins_command_payload_from_result( reload_runtime: result.reload_runtime, status, config_load_error, + mcp_validation, plugins: report.summaries().iter().map(plugin_summary_json).collect(), load_failures: failures.iter().map(plugin_load_failure_json).collect(), } @@ -14726,9 +14882,10 @@ mod tests { } #[test] - fn plugins_degrades_gracefully_on_malformed_mcp_config() { - // Keep the plugins surface consistent with status/doctor/mcp: a bad - // MCP entry should not make local plugin introspection unusable. + fn plugins_degrades_on_invalid_mcp_server_without_global_config_error_440() { + // #440: invalid MCP entries should not make local plugin introspection + // unusable, and should surface as validation metadata instead of a + // whole-config parse failure. let _guard = env_lock(); let root = temp_dir(); let cwd = root.join("project-with-malformed-mcp-for-plugins"); @@ -14761,16 +14918,19 @@ mod tests { } assert_eq!(payload.status, "degraded"); - let err = payload - .config_load_error - .as_deref() - .expect("config_load_error should be populated"); - assert!( - err.contains("mcpServers.missing-command"), - "config_load_error should name the malformed MCP field: {err}" + assert!(payload.config_load_error.is_none()); + assert_eq!(payload.mcp_validation.total_configured, 1); + assert_eq!(payload.mcp_validation.valid_count, 0); + assert_eq!(payload.mcp_validation.invalid_count(), 1); + assert_eq!( + payload.mcp_validation.invalid_servers[0].name, + "missing-command" ); - assert!(payload.message.contains("Config load error")); - assert!(payload.message.contains("partial plugins view")); + assert!(payload.mcp_validation.invalid_servers[0] + .reason + .contains("missing string field command")); + assert!(payload.message.contains("MCP validation")); + assert!(payload.message.contains("valid MCP siblings only")); assert!(payload.message.contains("Plugins")); let _ = std::fs::remove_dir_all(root); @@ -14786,14 +14946,13 @@ mod tests { let root = temp_dir(); let cwd = root.join("project-with-malformed-mcp"); std::fs::create_dir_all(&cwd).expect("project dir should exist"); - // One valid server + one malformed entry missing `command`. + // Top-level `mcpServers` shape errors still degrade through the + // config_load_error path; per-server errors are handled by the #440 + // MCP validation summary instead. std::fs::write( cwd.join(".claw.json"), r#"{ - "mcpServers": { - "everything": {"command": "npx", "args": ["-y", "@modelcontextprotocol/server-everything"]}, - "missing-command": {"args": ["arg-only-no-command"]} - } + "mcpServers": "not-an-object" } "#, ) @@ -14804,17 +14963,17 @@ mod tests { .expect("status_context should not hard-fail on config parse errors (#143)") }); - // Phase 1 contract: config_load_error is populated with the parse error. + // Config-shape errors still populate config_load_error. let err = context .config_load_error .as_ref() - .expect("config_load_error should be Some when config parse fails"); + .expect("config_load_error should be Some when config shape parsing fails"); assert!( - err.contains("mcpServers.missing-command"), - "config_load_error should name the malformed field path: {err}" + err.contains("mcpServers"), + "config_load_error should name the malformed mcpServers path: {err}" ); assert!( - err.contains("missing string field command"), + err.contains("must be an object"), "config_load_error should carry the underlying parse error: {err}" ); @@ -14856,7 +15015,7 @@ mod tests { assert!( json.get("config_load_error") .and_then(|v| v.as_str()) - .is_some_and(|s| s.contains("mcpServers.missing-command")), + .is_some_and(|s| s.contains("mcpServers")), "config_load_error should surface in JSON output: {json}" ); // Independent fields still populated. @@ -16576,6 +16735,7 @@ mod tests { binary_provenance: super::binary_provenance_for(None), config_load_error: None, config_load_error_kind: None, + mcp_validation: super::McpValidationSummary::default(), }, None, // #148 None, @@ -16726,6 +16886,7 @@ mod tests { binary_provenance: super::binary_provenance_for(None), config_load_error: None, config_load_error_kind: None, + mcp_validation: super::McpValidationSummary::default(), }; let check = super::check_workspace_health(&context); @@ -16775,6 +16936,7 @@ mod tests { binary_provenance: super::binary_provenance_for(None), config_load_error: None, config_load_error_kind: None, + mcp_validation: super::McpValidationSummary::default(), }; let check = super::check_memory_health(&context); @@ -16816,6 +16978,7 @@ mod tests { binary_provenance: super::binary_provenance_for(None), config_load_error: None, config_load_error_kind: None, + mcp_validation: super::McpValidationSummary::default(), }; let value = status_json_value( diff --git a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs index 907094bd..091ff59e 100644 --- a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs +++ b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs @@ -111,6 +111,7 @@ fn assert_doctor_help_json_contract(parsed: &Value) { assert!(checks.iter().any(|check| check == "auth")); assert!(checks.iter().any(|check| check == "boot preflight")); assert!(checks.iter().any(|check| check == "memory")); + assert!(checks.iter().any(|check| check == "mcp validation")); } #[test] @@ -1458,7 +1459,7 @@ fn doctor_and_resume_status_emit_json_when_requested() { .is_some_and(|available| available.iter().any(|name| name == "web_fetch"))); let checks = doctor["checks"].as_array().expect("doctor checks"); - assert_eq!(checks.len(), 9); + assert_eq!(checks.len(), 10); let check_names = checks .iter() .map(|check| { @@ -1479,6 +1480,7 @@ fn doctor_and_resume_status_emit_json_when_requested() { vec![ "auth", "config", + "mcp validation", "install source", "workspace", "memory", @@ -1822,6 +1824,12 @@ fn mcp_json_reports_required_optional_and_redacts_secret_values() { assert_eq!(list["action"], "list"); assert_eq!(list["status"], "ok"); assert_eq!(list["configured_servers"], 2); + assert_eq!(list["total_configured"], 2); + assert_eq!(list["valid_count"], 2); + assert_eq!(list["invalid_count"], 0); + assert!(list["invalid_servers"] + .as_array() + .is_some_and(Vec::is_empty)); let servers = list["servers"].as_array().expect("servers array"); let required = servers .iter() @@ -1832,6 +1840,8 @@ fn mcp_json_reports_required_optional_and_redacts_secret_values() { .find(|server| server["name"] == "optional-remote") .expect("optional remote server should be listed"); assert_eq!(required["required"], true); + assert_eq!(required["valid"], true); + assert_eq!(optional["valid"], true); assert_eq!(optional["required"], false); assert_eq!(required["details"]["env_keys"][0], "TOKEN"); assert_eq!(optional["details"]["header_keys"][0], "Authorization"); @@ -1850,6 +1860,10 @@ fn mcp_json_reports_required_optional_and_redacts_secret_values() { assert_eq!(show["action"], "show"); assert_eq!(show["status"], "ok"); assert_eq!(show["server"]["required"], false); + assert_eq!(show["server"]["valid"], true); + assert_eq!(show["total_configured"], 2); + assert_eq!(show["valid_count"], 2); + assert_eq!(show["invalid_count"], 0); assert_eq!(show["server"]["details"]["header_keys"][0], "Authorization"); let show_text = serde_json::to_string(&show).expect("mcp show json should serialize"); assert!(!show_text.contains("secret-header-value")); @@ -1859,18 +1873,29 @@ fn mcp_json_reports_required_optional_and_redacts_secret_values() { #[test] fn mcp_degraded_config_and_failed_usage_are_distinct_json_contracts() { let root = unique_temp_dir("mcp-degraded-vs-failed"); + let workspace = root.join("workspace"); let config_home = root.join("config-home"); let home = root.join("home"); - fs::create_dir_all(&root).expect("workspace should exist"); + fs::create_dir_all(&workspace).expect("workspace should exist"); fs::create_dir_all(&config_home).expect("config home should exist"); fs::create_dir_all(&home).expect("home should exist"); fs::write( - root.join(".claw.json"), + workspace.join(".claw.json"), r#"{ "mcpServers": { + "valid-server": { + "command": "/bin/echo", + "args": ["hello"] + }, "missing-command": { "args": ["arg-only-no-command"], "required": true + }, + "empty-command": { + "command": "" + }, + "wrong-type-command": { + "command": 42 } } }"#, @@ -1884,18 +1909,56 @@ fn mcp_degraded_config_and_failed_usage_are_distinct_json_contracts() { ("HOME", home.to_str().expect("home")), ]; - let degraded = assert_json_command_with_env(&root, &["--output-format", "json", "mcp"], &envs); + let degraded = + assert_json_command_with_env(&workspace, &["--output-format", "json", "mcp"], &envs); assert_eq!(degraded["kind"], "mcp"); assert_eq!(degraded["action"], "list"); assert_eq!(degraded["status"], "degraded"); - assert!(degraded["config_load_error"] + assert!(degraded["config_load_error"].is_null()); + assert_eq!(degraded["configured_servers"], 1); + assert_eq!(degraded["total_configured"], 4); + assert_eq!(degraded["valid_count"], 1); + assert_eq!(degraded["invalid_count"], 3); + assert_eq!(degraded["servers"][0]["name"], "valid-server"); + assert_eq!(degraded["servers"][0]["valid"], true); + assert_eq!(degraded["invalid_servers"][0]["name"], "empty-command"); + assert_eq!(degraded["invalid_servers"][0]["error_field"], "command"); + assert!(degraded["invalid_servers"][0]["reason"] .as_str() - .is_some_and(|error| error.contains("mcpServers.missing-command"))); - assert_eq!(degraded["configured_servers"], 0); - assert!(degraded["servers"].as_array().expect("servers").is_empty()); + .is_some_and(|error| error.contains("non-empty string"))); + assert_eq!(degraded["invalid_servers"][1]["name"], "missing-command"); + assert_eq!(degraded["invalid_servers"][1]["error_field"], "command"); + assert!(degraded["invalid_servers"][1]["reason"] + .as_str() + .is_some_and(|error| error.contains("missing string field command"))); + assert_eq!(degraded["invalid_servers"][2]["name"], "wrong-type-command"); + assert_eq!(degraded["invalid_servers"][2]["error_field"], "command"); + + let status = + assert_json_command_with_env(&workspace, &["--output-format", "json", "status"], &envs); + assert_eq!(status["status"], "degraded"); + assert!(status["config_load_error"].is_null()); + assert_eq!(status["mcp_validation"]["total_configured"], 4); + assert_eq!(status["mcp_validation"]["valid_count"], 1); + assert_eq!(status["mcp_validation"]["invalid_count"], 3); + + let doctor = + assert_json_command_with_env(&workspace, &["--output-format", "json", "doctor"], &envs); + let mcp_validation = doctor["checks"] + .as_array() + .expect("doctor checks") + .iter() + .find(|check| check["name"] == "mcp validation") + .expect("mcp validation check"); + assert_eq!(mcp_validation["status"], "warn"); + assert_eq!(mcp_validation["invalid_count"], 3); + assert_eq!( + mcp_validation["invalid_servers"][0]["name"], + "empty-command" + ); let failed_output = run_claw( - &root, + &workspace, &["--output-format", "json", "mcp", "list", "extra"], &envs, );