From abdbf61acfa23f89d4556e3b75a401ca754705cb Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 27 May 2026 09:36:11 +0900 Subject: [PATCH] fix(#788): skills show not-found emitted duplicate JSON error envelope; use exit(1) instead of Err propagation --- ROADMAP.md | 2 + rust/crates/rusty-claude-cli/src/main.rs | 10 +- .../tests/output_format_contract.rs | 101 ++++++++++++++++++ 3 files changed, 107 insertions(+), 6 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index a065bde5..d5ae7b33 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -7741,3 +7741,5 @@ Original filing (2026-04-18): the session emitted `SessionStart hook (completed) 786. **`claw dump-manifests --manifests-dir` (missing value) and `--manifests-dir=` (empty) both returned `error_kind:"unknown"` + `hint:null`** — dogfooded 2026-05-27 on `87f43347` (pinpoint by Gaebal-gajae). Both missing `--manifests-dir` branches in `parse_dump_manifests_args` emitted plain `"--manifests-dir requires a path"` with no typed prefix; `classify_error_kind` had no matching arm so they fell to `"unknown"`. Fix: both branches now use `missing_flag_value:` prefix + `\n` usage hint. Integration test `dump_manifests_missing_dir_has_typed_kind_and_hint_786` covers both cases. 45 CLI contract tests pass. [SCOPE: claw-code] Source: Gaebal-gajae pinpoint + Jobdori implementation on `87f43347`, 2026-05-27. 787. **`claw --resume /tmp` (directory path) returned `error_kind:"session_load_failed"` + `hint:null`; resume error emission sites didn't apply `fallback_hint_for_error_kind`** — dogfooded 2026-05-27 on `22b423b6`. Two gaps: (1) the OS error `"Is a directory (os error 21)"` had no classifier arm, falling to generic `session_load_failed`; (2) both resume error emission paths (session load at line 3338, command execution at line 3484) called `split_error_hint` but not `fallback_hint_for_error_kind`, so API-layer errors with no `\n` always got `hint:null`. Fix: added `session_path_is_directory` classifier arm for `"Is a directory"` / `"os error 21"` messages; added `fallback_hint_for_error_kind` fallback to both resume error sites; added `session_path_is_directory` and `session_load_failed` to `fallback_hint_for_error_kind`. Unit test + integration test `resume_directory_path_returns_typed_kind_and_hint_787`. 46 CLI contract tests pass. [SCOPE: claw-code] Source: Jobdori resume-path probe on `22b423b6`, 2026-05-27. + +788. **`claw --output-format json skills show ` emitted two JSON objects — one from the skills handler, one duplicate from the top-level error path** — dogfooded 2026-05-27 on `113145a4`. `print_skills` in JSON mode called `println!` to emit the `skill_not_found` error envelope, then returned `Err(...)`. The `?` propagation triggered the top-level error handler which emitted a second `action:"abort"` JSON envelope on stderr. Callers reading both stdout and stderr got two JSON objects with the same `error_kind` but different `action` fields — the first was the authoritative response, the second was a duplicate. Fix: replaced `return Err(...)` with `std::process::exit(1)` after the skills error JSON is emitted, mirroring the existing `is_help_action` guard pattern. Integration test `skills_show_not_found_emits_single_json_object_788` asserts exactly 1 JSON object on stdout and no JSON on stderr. 47 CLI contract tests pass. [SCOPE: claw-code] Source: Jobdori skills double-emission probe on `113145a4`, 2026-05-27. diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 1eab83b5..d6609efb 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -6333,12 +6333,10 @@ impl LiveCli { let is_help_action = result.get("action").and_then(|v| v.as_str()) == Some("help"); println!("{}", serde_json::to_string_pretty(&result)?); if is_error && !is_help_action { - return Err(result - .get("message") - .and_then(|v| v.as_str()) - .unwrap_or("skills command failed") - .to_string() - .into()); + // #788: the error JSON is already emitted above; returning Err here + // would cause the top-level handler to emit a second error envelope. + // Exit directly to signal failure without a duplicate envelope. + std::process::exit(1); } } } 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 b39cfc25..2a7009a2 100644 --- a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs +++ b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs @@ -2664,3 +2664,104 @@ fn resume_directory_path_returns_typed_kind_and_hint_787() { "hint should explain expected path format, got: {hint:?}" ); } + +#[test] +fn skills_show_not_found_emits_single_json_object_788() { + // #788: `claw --output-format json skills show no-such-skill` emitted TWO JSON objects: + // one from the skills handler (action:"show", status:"error") and a second from the + // top-level error handler (action:"abort"). The skills handler returned Err() after + // printing its JSON, which caused the ? propagation to trigger a duplicate envelope. + // Fix: exit(1) directly after the skills JSON is emitted instead of returning Err. + let root = unique_temp_dir("skills-show-double-emit-788"); + fs::create_dir_all(&root).expect("temp dir"); + std::process::Command::new("git") + .args(["init", "-q"]) + .current_dir(&root) + .output() + .ok(); + + let output = run_claw( + &root, + &[ + "--output-format", + "json", + "skills", + "show", + "no-such-skill-xyz", + ], + &[], + ); + assert!(!output.status.success(), "skills show unknown should fail"); + // Skills handler emits JSON to stdout; the duplicate was on stderr from the main error path. + // After fix: stdout has 1 JSON object, stderr has none (no duplicate). + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + // Count JSON objects in stdout — must be exactly 1 + let json_objects: Vec = { + let mut objects = Vec::new(); + let mut remaining = stdout.trim(); + while !remaining.is_empty() { + match serde_json::from_str::(remaining) { + Ok(v) => { + objects.push(v); + break; + } + Err(_) => { + // Try finding a complete JSON object + if let Some(pos) = remaining.find('{') { + remaining = &remaining[pos..]; + let mut depth = 0i32; + let mut end = 0; + for (i, c) in remaining.char_indices() { + match c { + '{' => depth += 1, + '}' => { + depth -= 1; + if depth == 0 { + end = i + 1; + break; + } + } + _ => {} + } + } + if end > 0 { + if let Ok(v) = serde_json::from_str(&remaining[..end]) { + objects.push(v); + remaining = remaining[end..].trim_start(); + } else { + break; + } + } else { + break; + } + } else { + break; + } + } + } + } + objects + }; + + assert_eq!( + json_objects.len(), + 1, + "skills show not-found must emit exactly 1 JSON object on stdout, got {}. stdout: {} stderr: {}", + json_objects.len(), + stdout, + stderr + ); + // Verify stderr has no duplicate error JSON (the pre-#788 bug was a second abort envelope here) + let stderr_has_json = stderr.lines().any(|l| l.trim_start().starts_with('{')); + assert!( + !stderr_has_json, + "stderr must have no duplicate JSON error envelope, got: {stderr}" + ); + assert_eq!( + json_objects[0]["error_kind"], "skill_not_found", + "single JSON object must have skill_not_found error_kind" + ); + assert_eq!(json_objects[0]["status"], "error"); +}