mirror of
https://github.com/instructkr/claude-code.git
synced 2026-05-28 16:36:45 +00:00
fix(#788): skills show not-found emitted duplicate JSON error envelope; use exit(1) instead of Err propagation
This commit is contained in:
@@ -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 <not-found>` 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.
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<serde_json::Value> = {
|
||||
let mut objects = Vec::new();
|
||||
let mut remaining = stdout.trim();
|
||||
while !remaining.is_empty() {
|
||||
match serde_json::from_str::<serde_json::Value>(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");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user