diff --git a/ROADMAP.md b/ROADMAP.md index 81b995a..a9f87c6 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -5455,3 +5455,68 @@ Doctor keeps going and produces a full typed report. Status refuses to produce a **Blocker.** None for Phase 1. Phase 2 depends on the typed-error taxonomy landing (ROADMAP §4.44), but Phase 1 can ship independently and be tightened later. **Source.** Jobdori dogfood 2026-04-21 18:30 KST on main HEAD `e73b6a2`, surfaced by running `claw status` in `/Users/yeongyu/clawd` which contains a `.claw.json` with deliberately broken MCP entries. Joins **partial-success / degraded-mode** cluster (Principle #5, Phase 6) and **surface consistency** cluster (#141 help-contract unification, #108 typo guard). Session tally: ROADMAP #143. + +## Pinpoint #144. `claw mcp` hard-fails on malformed MCP config — same surface inconsistency as #143, one command over + +**Gap.** With `claw status` fixed in #143 Phase 1, `claw mcp` is now the remaining diagnostic surface that hard-fails on a malformed `.claw.json`. Same input, same parse error, same partial-success violation. + +**Verified on main HEAD `e2a43fc` (2026-04-21 18:59 KST):** + +Same `.claw.json` used for #143 repro (one valid `everything` server + one malformed `missing-command` entry). + +`claw mcp`: +``` +error: /Users/.../.claw.json: mcpServers.missing-command: missing string field command +Run `claw --help` for usage. +``` +Exit 1. No list. The well-formed `everything` server is invisible. + +`claw mcp --output-format json`: +```json +{"error":"/Users/.../.claw.json: mcpServers.missing-command: missing string field command","type":"error"} +``` +Exit 1. Same story. + +`claw status --output-format json` on the same file (post-#143): +```json +{"kind":"status","status":"degraded","config_load_error":"...","workspace":{...},"sandbox":{...},...} +``` +Exit 0. Full envelope with error surfaced. + +**Why this is a clawability gap (same family as #143).** +1. **Principle #5 violation**: partial success is first-class. One malformed entry shouldn't make the entire MCP subsystem invisible. +2. **Surface inconsistency (cluster of 3)**: after #143 Phase 1, the behavior matrix is: + - `doctor` — degraded envelope ✅ + - `status` — degraded envelope ✅ (#143) + - `mcp` — hard-fail ❌ (this pinpoint) +3. **Clawhip impact**: `claw mcp --output-format json` is used by orchestrators to detect which MCP servers are available before invoking tools. A broken probe forces clawhip to fall back to doctor parse, which is suboptimal. + +**Fix shape (~40 lines, mirrors #143 Phase 1).** +1. Make `render_mcp_report_json_for()` and `render_mcp_report_for()` catch the `ConfigError` at `loader.load()?`. +2. On parse failure, emit a degraded envelope: + ```json + { + "kind": "mcp", + "action": "list", + "status": "degraded", + "config_load_error": "...", + "working_directory": "...", + "configured_servers": 0, + "servers": [] + } + ``` +3. Text mode: prepend a "Config load error" block (same shape as #143) before the "MCP" block. +4. Exit 0 so downstream probes don't treat a parse error as process death. + +**Acceptance.** +- `claw mcp` and `claw mcp --output-format json` on a workspace with malformed config exit 0. +- JSON mode includes `status: "degraded"` and `config_load_error` field. +- Text mode shows the parse error in a separate block, not as the only output. +- Clean path (no config errors) still returns `status: "ok"` (or equivalent — align with #143 serializer). +- Regression test: inject malformed config, assert mcp returns degraded envelope. + +**Blocker.** None. Mirrors #143 Phase 1 shape exactly. + +**Future phase (joins #143 Phase 2).** When typed-error taxonomy lands (§4.44), promote `config_load_error` from string to typed object across `doctor`, `status`, and `mcp` in one pass. + +**Source.** Jobdori dogfood 2026-04-21 18:59 KST on main HEAD `e2a43fc`. Joins **partial-success** cluster (#143, Principle #5) and **surface consistency** cluster. Session tally: ROADMAP #144. diff --git a/rust/crates/commands/src/lib.rs b/rust/crates/commands/src/lib.rs index bf6d793..9d69393 100644 --- a/rust/crates/commands/src/lib.rs +++ b/rust/crates/commands/src/lib.rs @@ -2554,11 +2554,22 @@ fn render_mcp_report_for( match normalize_optional_args(args) { None | Some("list") => { - let runtime_config = loader.load()?; - Ok(render_mcp_summary_report( - cwd, - runtime_config.mcp().servers(), - )) + // #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) + )) + } + } } Some(args) if is_help_arg(args) => Ok(render_mcp_usage(None)), Some("show") => Ok(render_mcp_usage(Some("show"))), @@ -2571,12 +2582,19 @@ fn render_mcp_report_for( if parts.next().is_some() { return Ok(render_mcp_usage(Some(args))); } - let runtime_config = loader.load()?; - Ok(render_mcp_server_report( - cwd, - server_name, - runtime_config.mcp().get(server_name), - )) + // #144: same degradation for `mcp show`; if config won't parse, + // the specific server lookup can't succeed, so report the parse + // error with context. + match loader.load() { + Ok(runtime_config) => Ok(render_mcp_server_report( + cwd, + server_name, + runtime_config.mcp().get(server_name), + )), + 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" + )), + } } Some(args) => Ok(render_mcp_usage(Some(args))), } @@ -2599,11 +2617,35 @@ fn render_mcp_report_json_for( match normalize_optional_args(args) { None | Some("list") => { - let runtime_config = loader.load()?; - Ok(render_mcp_summary_report_json( - cwd, - runtime_config.mcp().servers(), - )) + // #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 loader.load() { + 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) + } + } } Some(args) if is_help_arg(args) => Ok(render_mcp_usage_json(None)), Some("show") => Ok(render_mcp_usage_json(Some("show"))), @@ -2616,12 +2658,29 @@ fn render_mcp_report_json_for( if parts.next().is_some() { return Ok(render_mcp_usage_json(Some(args))); } - let runtime_config = loader.load()?; - Ok(render_mcp_server_report_json( - cwd, - server_name, - runtime_config.mcp().get(server_name), - )) + // #144: same degradation pattern for show action. + match loader.load() { + Ok(runtime_config) => { + let mut value = render_mcp_server_report_json( + cwd, + server_name, + runtime_config.mcp().get(server_name), + ); + 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) => Ok(serde_json::json!({ + "kind": "mcp", + "action": "show", + "server": server_name, + "status": "degraded", + "config_load_error": err.to_string(), + "working_directory": cwd.display().to_string(), + })), + } } Some(args) => Ok(render_mcp_usage_json(Some(args))), } @@ -5479,6 +5538,82 @@ mod tests { let _ = fs::remove_dir_all(config_home); } + #[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. + let _guard = env_guard(); + let workspace = temp_dir("mcp-degrades-144"); + let config_home = temp_dir("mcp-degrades-144-cfg"); + fs::create_dir_all(workspace.join(".claw")).expect("create workspace .claw dir"); + fs::create_dir_all(&config_home).expect("create config home"); + // One valid server + one malformed entry missing `command`. + fs::write( + workspace.join(".claw.json"), + r#"{ + "mcpServers": { + "everything": {"command": "npx", "args": ["-y", "@modelcontextprotocol/server-everything"]}, + "missing-command": {"args": ["arg-only-no-command"]} + } +} +"#, + ) + .expect("write malformed .claw.json"); + + let loader = ConfigLoader::new(&workspace, &config_home); + // list action: must return Ok (not Err) with degraded envelope. + let list = render_mcp_report_json_for(&loader, &workspace, None) + .expect("mcp list should not hard-fail on config parse errors (#144)"); + assert_eq!(list["kind"], "mcp"); + assert_eq!(list["action"], "list"); + assert_eq!( + list["status"].as_str(), + Some("degraded"), + "top-level status should be 'degraded': {list}" + ); + let err = list["config_load_error"] + .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()); + + // show action: should also degrade (not hard-fail). + 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"); + assert_eq!(show["action"], "show"); + assert_eq!( + show["status"].as_str(), + Some("degraded"), + "show action should also report status: 'degraded': {show}" + ); + assert!(show["config_load_error"].is_string()); + + // Clean path: status: "ok", config_load_error: null. + let clean_ws = temp_dir("mcp-degrades-144-clean"); + fs::create_dir_all(&clean_ws).expect("clean ws"); + let clean_loader = ConfigLoader::new(&clean_ws, &config_home); + let clean_list = render_mcp_report_json_for(&clean_loader, &clean_ws, None) + .expect("clean mcp list should succeed"); + assert_eq!( + clean_list["status"].as_str(), + Some("ok"), + "clean run should report status: 'ok'" + ); + assert!(clean_list["config_load_error"].is_null()); + + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(config_home); + let _ = fs::remove_dir_all(clean_ws); + } + #[test] fn parses_quoted_skill_frontmatter_values() { let contents = "---\nname: \"hud\"\ndescription: 'Quoted description'\n---\n";