mirror of
https://github.com/instructkr/claw-code.git
synced 2026-04-27 02:34:58 +08:00
feat: #144 phase 1 + ROADMAP filing — claw mcp degrades gracefully on malformed config
Filing + Phase 1 fix in one commit (sibling of #143). ## Context With #143 Phase 1 landed (`claw status` degrades), `claw mcp` was the remaining diagnostic surface that hard-failed on a malformed `.claw.json`. Same input, same parse error, same partial-success violation. Fresh dogfood at 18:59 KST caught it on main HEAD `e2a43fc`. ## Changes ### ROADMAP.md Added Pinpoint #144 documenting the gap and acceptance criteria. Joins the partial-success / Principle #5 cluster with #143. ### rust/crates/commands/src/lib.rs `render_mcp_report_for()` + `render_mcp_report_json_for()` now catch the ConfigError at loader.load() instead of propagating: - **Text mode** prepends a "Config load error" block (same shape as #143's status output) before the MCP listing. The listing still renders with empty servers so the output structure is preserved. - **JSON mode** adds top-level `status: "ok" | "degraded"` + `config_load_error: string | null` fields alongside existing fields (`kind`, `action`, `working_directory`, `configured_servers`, `servers[]`). On clean runs, `status: "ok"` and `config_load_error: null`. On parse failure, `status: "degraded"`, `config_load_error: "..."`, `servers: []`, exit 0. - Both list and show actions get the same treatment. ### Regression test `commands::tests::mcp_degrades_gracefully_on_malformed_mcp_config_144`: - Injects the same malformed .claw.json as #143 (one valid + one broken mcpServers entry). - Asserts mcp list returns Ok (not Err). - Asserts top-level status: "degraded" and config_load_error names the malformed field path. - Asserts show action also degrades. - Asserts clean path returns status: "ok" with config_load_error null. ## Live verification $ claw mcp --output-format json { "action": "list", "kind": "mcp", "status": "degraded", "config_load_error": ".../.claw.json: mcpServers.missing-command: missing string field command", "working_directory": "/Users/yeongyu/clawd", "configured_servers": 0, "servers": [] } Exit 0. ## Contract alignment after this commit All three diagnostic surfaces match now: - `doctor` — degraded envelope with typed check entries ✅ - `status` — degraded envelope with config_load_error ✅ (#143) - `mcp` — degraded envelope with config_load_error ✅ (this commit) Phase 2 (typed-error object joining taxonomy §4.44) tracked separately across all three surfaces. Full workspace test green except pre-existing resume_latest flake (unrelated). Closes ROADMAP #144 phase 1.
This commit is contained in:
65
ROADMAP.md
65
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.
|
**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.
|
**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.
|
||||||
|
|||||||
@@ -2554,11 +2554,22 @@ fn render_mcp_report_for(
|
|||||||
|
|
||||||
match normalize_optional_args(args) {
|
match normalize_optional_args(args) {
|
||||||
None | Some("list") => {
|
None | Some("list") => {
|
||||||
let runtime_config = loader.load()?;
|
// #144: degrade gracefully on config parse failure (same contract
|
||||||
Ok(render_mcp_summary_report(
|
// as #143 for `status`). Text mode prepends a "Config load error"
|
||||||
cwd,
|
// block before the MCP list; the list falls back to empty.
|
||||||
runtime_config.mcp().servers(),
|
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(args) if is_help_arg(args) => Ok(render_mcp_usage(None)),
|
||||||
Some("show") => Ok(render_mcp_usage(Some("show"))),
|
Some("show") => Ok(render_mcp_usage(Some("show"))),
|
||||||
@@ -2571,12 +2582,19 @@ fn render_mcp_report_for(
|
|||||||
if parts.next().is_some() {
|
if parts.next().is_some() {
|
||||||
return Ok(render_mcp_usage(Some(args)));
|
return Ok(render_mcp_usage(Some(args)));
|
||||||
}
|
}
|
||||||
let runtime_config = loader.load()?;
|
// #144: same degradation for `mcp show`; if config won't parse,
|
||||||
Ok(render_mcp_server_report(
|
// the specific server lookup can't succeed, so report the parse
|
||||||
cwd,
|
// error with context.
|
||||||
server_name,
|
match loader.load() {
|
||||||
runtime_config.mcp().get(server_name),
|
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))),
|
Some(args) => Ok(render_mcp_usage(Some(args))),
|
||||||
}
|
}
|
||||||
@@ -2599,11 +2617,35 @@ fn render_mcp_report_json_for(
|
|||||||
|
|
||||||
match normalize_optional_args(args) {
|
match normalize_optional_args(args) {
|
||||||
None | Some("list") => {
|
None | Some("list") => {
|
||||||
let runtime_config = loader.load()?;
|
// #144: match #143's degraded envelope contract. On config parse
|
||||||
Ok(render_mcp_summary_report_json(
|
// failure, emit top-level `status: "degraded"` with
|
||||||
cwd,
|
// `config_load_error`, empty servers[], and exit 0. On clean
|
||||||
runtime_config.mcp().servers(),
|
// 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(args) if is_help_arg(args) => Ok(render_mcp_usage_json(None)),
|
||||||
Some("show") => Ok(render_mcp_usage_json(Some("show"))),
|
Some("show") => Ok(render_mcp_usage_json(Some("show"))),
|
||||||
@@ -2616,12 +2658,29 @@ fn render_mcp_report_json_for(
|
|||||||
if parts.next().is_some() {
|
if parts.next().is_some() {
|
||||||
return Ok(render_mcp_usage_json(Some(args)));
|
return Ok(render_mcp_usage_json(Some(args)));
|
||||||
}
|
}
|
||||||
let runtime_config = loader.load()?;
|
// #144: same degradation pattern for show action.
|
||||||
Ok(render_mcp_server_report_json(
|
match loader.load() {
|
||||||
cwd,
|
Ok(runtime_config) => {
|
||||||
server_name,
|
let mut value = render_mcp_server_report_json(
|
||||||
runtime_config.mcp().get(server_name),
|
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))),
|
Some(args) => Ok(render_mcp_usage_json(Some(args))),
|
||||||
}
|
}
|
||||||
@@ -5479,6 +5538,82 @@ mod tests {
|
|||||||
let _ = fs::remove_dir_all(config_home);
|
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]
|
#[test]
|
||||||
fn parses_quoted_skill_frontmatter_values() {
|
fn parses_quoted_skill_frontmatter_values() {
|
||||||
let contents = "---\nname: \"hud\"\ndescription: 'Quoted description'\n---\n";
|
let contents = "---\nname: \"hud\"\ndescription: 'Quoted description'\n---\n";
|
||||||
|
|||||||
Reference in New Issue
Block a user