fix(doctor): exit nonzero on failed checks

This commit is contained in:
NianJiuZst
2026-07-10 22:39:41 +08:00
parent 913a3f80a4
commit dca2b58cbd
5 changed files with 57 additions and 12 deletions
+21
View File
@@ -104,6 +104,12 @@ pub fn run(output: Output) -> Result<Vec<CheckResult>> {
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
+25 -2
View File
@@ -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<u8> = 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
+6 -3
View File
@@ -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 {
+4 -4
View File
@@ -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::Value> = 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::Value> = serde_json::from_str(stdout.trim()).unwrap();