From dca2b58cbd2269dbaa020a02f0d75e66e9dc5cf5 Mon Sep 17 00:00:00 2001 From: NianJiuZst <3235467914@qq.com> Date: Fri, 10 Jul 2026 22:39:41 +0800 Subject: [PATCH] fix(doctor): exit nonzero on failed checks --- apps/extension/vitest.config.ts | 4 +--- crates/bsk-cli/src/cli/doctor.rs | 21 +++++++++++++++++++++ crates/bsk-cli/src/cli/error.rs | 27 +++++++++++++++++++++++++-- crates/bsk-cli/src/main.rs | 9 ++++++--- crates/bsk-cli/tests/status_cmd.rs | 8 ++++---- 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/apps/extension/vitest.config.ts b/apps/extension/vitest.config.ts index 2e1c157..00060bf 100644 --- a/apps/extension/vitest.config.ts +++ b/apps/extension/vitest.config.ts @@ -21,9 +21,7 @@ export default defineConfig({ }, define: { __BSK_EXT_VERSION__: JSON.stringify(pkg.version), - __BSK_DAEMON_WS_URL__: JSON.stringify( - process.env.BSK_DAEMON_WS_URL ?? "ws://127.0.0.1:52800", - ), + __BSK_DAEMON_WS_URL__: JSON.stringify(process.env.BSK_DAEMON_WS_URL ?? "ws://127.0.0.1:52800"), }, resolve: { alias: { diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index c4aa2a2..fa33718 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -104,6 +104,12 @@ pub fn run(output: Output) -> Result> { Ok(checks) } +/// Whether the rendered doctor report contains an active failure. +/// `NotApplicable` remains informational and must not change the exit code. +pub fn has_failures(checks: &[CheckResult]) -> bool { + checks.iter().any(|check| check.status == CheckStatus::Fail) +} + /// Ensure the daemon is reachable and give the browser extension time to /// connect before checks run. Returns a single [`DaemonState`] snapshot /// for check evaluation. @@ -500,6 +506,21 @@ mod m2_tests { ); } + #[test] + fn only_active_failures_make_doctor_unsuccessful() { + let healthy = vec![ + CheckResult::ok("ok", "ready"), + CheckResult::na("optional", "not connected"), + ]; + assert!(!has_failures(&healthy)); + + let unhealthy = vec![ + CheckResult::ok("ok", "ready"), + CheckResult::fail("broken", "not ready", "repair it"), + ]; + assert!(has_failures(&unhealthy)); + } + #[test] fn browsers_check_reports_na_when_no_browser_connected() { // Review M2: 0 browsers means there is nothing to compare diff --git a/crates/bsk-cli/src/cli/error.rs b/crates/bsk-cli/src/cli/error.rs index 4490a81..f8a7b1e 100644 --- a/crates/bsk-cli/src/cli/error.rs +++ b/crates/bsk-cli/src/cli/error.rs @@ -43,6 +43,12 @@ pub enum CliError { #[error("{message}")] Rendered { code: ErrorCode, message: String }, + /// Command already rendered its result and only needs to communicate + /// a command-specific non-zero status. Unlike `Rendered`, this is not + /// pretending that a daemon protocol error occurred. + #[error("command reported an unsuccessful status")] + RenderedExit { exit_code: u8 }, + /// Local transport / setup failure (e.g. couldn't reach the /// daemon, JSON encode failed). Maps to exit code 2. #[error(transparent)] @@ -63,6 +69,7 @@ impl CliError { match self { CliError::Rpc { code, .. } => Some(*code), CliError::Rendered { code, .. } => Some(*code), + CliError::RenderedExit { .. } => None, CliError::Local(_) => None, } } @@ -76,6 +83,9 @@ impl CliError { /// Map this error to the CLI exit code (ยง3.1). pub fn exit_code(&self) -> u8 { + if let CliError::RenderedExit { exit_code } = self { + return *exit_code; + } match self.code() { Some(code) => exit_code_for(code), None => 2, // local / protocol-level transport failure @@ -207,7 +217,10 @@ pub fn render_with_extras( extras: Option<&dyn RenderExtras>, ) -> ExitCode { let exit = err.exit_code(); - if matches!(err, CliError::Rendered { .. }) { + if matches!( + err, + CliError::Rendered { .. } | CliError::RenderedExit { .. } + ) { return ExitCode::from(exit); } let render_info = render_info_for(err); @@ -259,7 +272,10 @@ pub fn render_with_extras( #[cfg(test)] pub(crate) fn render_human_to_string(err: &CliError, extras: Option<&dyn RenderExtras>) -> String { let mut buf: Vec = Vec::new(); - if matches!(err, CliError::Rendered { .. }) { + if matches!( + err, + CliError::Rendered { .. } | CliError::RenderedExit { .. } + ) { return String::new(); } let render_info = render_info_for(err); @@ -340,6 +356,13 @@ mod tests { assert_eq!(err.exit_code(), 3); } + #[test] + fn rendered_exit_uses_command_specific_status_without_protocol_code() { + let err = CliError::RenderedExit { exit_code: 1 }; + assert_eq!(err.code(), None); + assert_eq!(err.exit_code(), 1); + } + /// Review I2: the `--json` payload must be flat at the top level. /// The previous shape nested everything under `"error"` which was /// awkward for shell scripts (`jq -r '.error.code'`) and contradicted diff --git a/crates/bsk-cli/src/main.rs b/crates/bsk-cli/src/main.rs index ab13d10..8053cd9 100644 --- a/crates/bsk-cli/src/main.rs +++ b/crates/bsk-cli/src/main.rs @@ -56,9 +56,12 @@ fn dispatch(cli: Cli, format: Format) -> Result<(), CliError> { } else { Output::Human }; - cli::doctor::run(output) - .map(|_| ()) - .map_err(CliError::Local) + let checks = cli::doctor::run(output).map_err(CliError::Local)?; + if cli::doctor::has_failures(&checks) { + Err(CliError::RenderedExit { exit_code: 1 }) + } else { + Ok(()) + } } Command::InstallSkill(args) => { let output = if cli.flags.json { diff --git a/crates/bsk-cli/tests/status_cmd.rs b/crates/bsk-cli/tests/status_cmd.rs index 13205d9..0e32914 100644 --- a/crates/bsk-cli/tests/status_cmd.rs +++ b/crates/bsk-cli/tests/status_cmd.rs @@ -92,7 +92,7 @@ fn bsk_doctor_runs_without_running_daemon() { .env("BSK_DOCTOR_BROWSER_WAIT_MS", "200") .output() .expect("run bsk doctor"); - assert!(out.status.success()); + assert_eq!(out.status.code(), Some(1)); let stdout = String::from_utf8(out.stdout).unwrap(); assert!( stdout.contains("bsk home writable"), @@ -137,7 +137,7 @@ fn bsk_doctor_json_returns_structured_checks() { .env("BSK_DOCTOR_BROWSER_WAIT_MS", "200") .output() .expect("run bsk doctor --json"); - assert!(out.status.success()); + assert_eq!(out.status.code(), Some(1)); let stdout = String::from_utf8(out.stdout).unwrap(); let parsed: serde_json::Value = serde_json::from_str(stdout.trim()).expect("doctor --json should be valid JSON"); @@ -203,7 +203,7 @@ fn bsk_doctor_does_not_treat_live_non_daemon_pid_as_running() { .env("RUST_LOG", "warn") .output() .expect("run bsk doctor --json"); - assert!(out.status.success()); + assert_eq!(out.status.code(), Some(1)); let stdout = String::from_utf8(out.stdout).unwrap(); let checks: Vec = serde_json::from_str(stdout.trim()).unwrap(); let daemon = checks @@ -262,7 +262,7 @@ fn bsk_doctor_flags_pid_mismatch_against_running_daemon() { .env("RUST_LOG", "warn") .output() .expect("bsk doctor --json"); - assert!(out.status.success()); + assert_eq!(out.status.code(), Some(1)); let stdout = String::from_utf8(out.stdout).unwrap(); let checks: Vec = serde_json::from_str(stdout.trim()).unwrap();