From e4c3c1aa809c0d36a68e7fd11c420d0d130c7b7c Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 27 May 2026 10:07:51 +0900 Subject: [PATCH] fix(#789): agents show and plugins show not-found now exit 1; parity with skills (#788) and mcp (#68) --- ROADMAP.md | 2 + rust/crates/rusty-claude-cli/src/main.rs | 18 +++-- .../tests/output_format_contract.rs | 79 ++++++++++++++++++- 3 files changed, 93 insertions(+), 6 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index d5ae7b33..d37c5a11 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -7743,3 +7743,5 @@ Original filing (2026-04-18): the session emitted `SessionStart hook (completed) 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. + +789. **`claw --output-format json agents show ` and `plugins show ` both returned exit 0 despite `status:"error"` in the JSON** — dogfooded 2026-05-27 on `abdbf61a`. Skills was fixed in #788 (exit 1 via process::exit). Agents and plugins had the identical gap: `print_agents` had no error check at all (just println + Ok(())); `print_plugins`'s not-found branch used `return Ok(())`. MCP was already fixed in an earlier cycle (#68). Fix: added `is_error` check in `print_agents` JSON path (exit 1 when status=="error"); changed plugins not-found branch from `return Ok(())` to `std::process::exit(1)`. Existing `inventory_commands_emit_structured_json_when_requested` test updated to use `run_claw` directly for the not-found case. Two new tests added: `agents_show_not_found_exits_nonzero_789`, `plugins_show_not_found_exits_nonzero_789`. 49 CLI contract tests pass. [SCOPE: claw-code] Source: Jobdori exit-code consistency probe on `abdbf61a`, 2026-05-27. diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index d6609efb..aae04f00 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -6281,10 +6281,17 @@ impl LiveCli { let cwd = env::current_dir()?; match output_format { CliOutputFormat::Text => println!("{}", handle_agents_slash_command(args, &cwd)?), - CliOutputFormat::Json => println!( - "{}", - serde_json::to_string_pretty(&handle_agents_slash_command_json(args, &cwd)?)? - ), + CliOutputFormat::Json => { + let value = handle_agents_slash_command_json(args, &cwd)?; + // #789: parity with print_mcp/#788 print_skills — exit 1 when envelope + // reports an error so automation can rely on exit code instead of + // parsing the JSON status field. + let is_error = value.get("status").and_then(|v| v.as_str()) == Some("error"); + println!("{}", serde_json::to_string_pretty(&value)?); + if is_error { + std::process::exit(1); + } + } } Ok(()) } @@ -6428,7 +6435,8 @@ impl LiveCli { "hint": "Run `claw plugins list` to see available plugins.", }); println!("{}", serde_json::to_string_pretty(&obj)?); - return Ok(()); + // #789: exit 1 on not-found so automation can rely on exit code + 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 2a7009a2..868cdef3 100644 --- a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs +++ b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs @@ -203,7 +203,9 @@ fn inventory_commands_emit_structured_json_when_requested() { isolated_codex.to_str().expect("utf8 codex home"), ), ]; - let agents_show_missing = assert_json_command_with_env( + // #789: agents show not-found now exits 1 (parity with skills #788); + // use run_claw directly instead of assert_json_command_with_env which checks success. + let agents_show_out = run_claw( &root, &[ "--output-format", @@ -214,6 +216,12 @@ fn inventory_commands_emit_structured_json_when_requested() { ], &agents_show_env, ); + assert!( + !agents_show_out.status.success(), + "agents show not-found must exit non-zero" + ); + let agents_show_missing: serde_json::Value = + serde_json::from_slice(&agents_show_out.stdout).expect("agents show stdout should be json"); assert_eq!(agents_show_missing["kind"], "agents", "agents show kind"); assert_eq!(agents_show_missing["action"], "show", "agents show action"); assert_eq!( @@ -2765,3 +2773,72 @@ fn skills_show_not_found_emits_single_json_object_788() { ); assert_eq!(json_objects[0]["status"], "error"); } + +#[test] +fn agents_show_not_found_exits_nonzero_789() { + // #789: `claw --output-format json agents show ` returned exit 0 despite + // emitting status:"error". print_agents had no error check — just println + Ok(()). + // Skills was fixed in #788 (exit 1 via process::exit); agents/plugins had the same gap. + let root = unique_temp_dir("agents-show-exit-789"); + 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", + "agents", + "show", + "no-such-agent-xyz-789", + ], + &[], + ); + assert!( + !output.status.success(), + "agents show not-found must exit non-zero (#789), got exit 0" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + let j: serde_json::Value = + serde_json::from_str(stdout.trim()).expect("agents show should emit valid JSON"); + assert_eq!(j["error_kind"], "agent_not_found"); + assert_eq!(j["status"], "error"); +} + +#[test] +fn plugins_show_not_found_exits_nonzero_789() { + // #789: same as agents — `claw --output-format json plugins show ` exited 0 + // despite status:"error". The not-found branch used `return Ok(())` instead of exit(1). + let root = unique_temp_dir("plugins-show-exit-789"); + 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", + "plugins", + "show", + "no-such-plugin-xyz-789", + ], + &[], + ); + assert!( + !output.status.success(), + "plugins show not-found must exit non-zero (#789), got exit 0" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + let j: serde_json::Value = + serde_json::from_str(stdout.trim()).expect("plugins show should emit valid JSON"); + assert_eq!(j["error_kind"], "plugin_not_found"); + assert_eq!(j["status"], "error"); +}