Merge pull request #23 from NianJiuZst/codex/doctor-fails-nonzero

fix(doctor): exit nonzero on failed checks

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: {

crates/bsk-cli/src/cli/doctor.rs

@@ -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

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<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

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 {

crates/bsk-cli/tests/browser_list_ordering.rs

@@ -16,7 +16,6 @@ use bsk_protocol::system::BrowserStatusEntry;
 use bsk_protocol::{ErrorCode, Method};
 use rand::Rng;
 use serde::Deserialize;
-use tokio::net::TcpListener;
 use tokio::sync::mpsc;
 
 fn tempfile_path(prefix: &str) -> PathBuf {
@@ -30,9 +29,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port);
     let sock = tempfile_path("bsk-test-browser-list");

crates/bsk-cli/tests/browser_wait.rs

@@ -15,7 +15,6 @@ use bsk_protocol::system::{BrowserListParams, HandshakeParams, HandshakeResult,
 use bsk_protocol::{BrowserPeerInfo, Method, RequestFrame, ResponseBody, ResponseFrame};
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
-use tokio::net::TcpListener;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
 use tokio_tungstenite::tungstenite::protocol::Message;
@@ -77,9 +76,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port);
     let sock = tempfile_path("bsk-test-browser-wait");

crates/bsk-cli/tests/cancel_forwarding.rs

@@ -20,7 +20,6 @@ use bsk_protocol::{
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
 use serde_json::json;
-use tokio::net::TcpListener;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
 use tokio_tungstenite::tungstenite::protocol::Message;
@@ -40,9 +39,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port);
     let sock = tempfile_path("bsk-test-cancel");

crates/bsk-cli/tests/handshake_compat.rs

@@ -10,7 +10,6 @@ use bsk_protocol::system::{HandshakeParams, HandshakeResult, StatusResult};
 use bsk_protocol::{BrowserPeerInfo, ErrorCode, Method, RequestFrame, ResponseBody, ResponseFrame};
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
-use tokio::net::TcpListener;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
 use tokio_tungstenite::tungstenite::protocol::Message;
@@ -28,9 +27,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port);
     let sock = tempfile_path("bsk-test-handshake");

crates/bsk-cli/tests/per_session_queue.rs

@@ -27,7 +27,6 @@ use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
 use serde::Deserialize;
 use serde_json::json;
-use tokio::net::TcpListener;
 use tokio::sync::{Mutex, mpsc};
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
@@ -46,9 +45,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
     let sock = tempfile_path("bsk-test-queue");
     let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone()))
         .await

crates/bsk-cli/tests/session_user_interrupt.rs

@@ -26,7 +26,6 @@ use bsk_protocol::{
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
 use serde_json::json;
-use tokio::net::TcpListener;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
 use tokio_tungstenite::tungstenite::protocol::Message;
@@ -46,9 +45,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port);
     let sock = tempfile_path("bsk-test-user-interrupt");

crates/bsk-cli/tests/sessions_ipc.rs

@@ -15,7 +15,6 @@ use bsk_protocol::tools::{SessionStartParams, SessionStartResult, SessionStopPar
 use bsk_protocol::{BrowserPeerInfo, Frame, Method, RequestFrame, ResponseBody, ResponseFrame};
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
-use tokio::net::TcpListener;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
 use tokio_tungstenite::tungstenite::protocol::Message;
@@ -39,9 +38,7 @@ async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
 }
 
 async fn spawn_daemon_with_connect_wait(connect_wait: Duration) -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port).with_extension_connect_wait(connect_wait);
     let sock = tempfile_path("bsk-test-ipc");

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::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();
 

crates/bsk-cli/tests/tools_ipc.rs

@@ -21,7 +21,6 @@ use futures_util::stream::{SplitSink, SplitStream};
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
 use serde_json::{Value, json};
-use tokio::net::TcpListener;
 use tokio::sync::Mutex;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
@@ -40,9 +39,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
     let sock = tempfile_path("bsk-test-tools");
     let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone()))
         .await

crates/bsk-cli/tests/tools_m7_ipc.rs

@@ -25,7 +25,6 @@ use futures_util::stream::{SplitSink, SplitStream};
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
 use serde_json::{Value, json};
-use tokio::net::TcpListener;
 use tokio::sync::Mutex;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
@@ -44,9 +43,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
     let sock = tempfile_path("bsk-test-tools-m7");
     let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone()))
         .await

crates/bsk-cli/tests/tools_m8_ipc.rs

@@ -24,7 +24,6 @@ use futures_util::stream::{SplitSink, SplitStream};
 use futures_util::{SinkExt, StreamExt};
 use rand::Rng;
 use serde_json::Value;
-use tokio::net::TcpListener;
 use tokio::sync::Mutex;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
@@ -43,9 +42,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
     let sock = tempfile_path("bsk-test-tools-m8");
     let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone()))
         .await

crates/bsk-cli/tests/tools_m9_ipc.rs

@@ -30,7 +30,6 @@ use rand::Rng;
 use serde_json::{Value, json};
 #[cfg(unix)]
 use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
-use tokio::net::TcpListener;
 #[cfg(unix)]
 use tokio::net::UnixStream;
 use tokio::sync::Mutex;
@@ -53,9 +52,7 @@ fn tempfile_path(prefix: &str) -> PathBuf {
 }
 
 async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) {
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
     let sock = tempfile_path("bsk-test-tools-m9");
     let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone()))
         .await

crates/bsk-cli/tests/ws_handshake.rs

@@ -8,7 +8,6 @@ use bsk::daemon::{self, DaemonConfig};
 use bsk_protocol::system::{HandshakeParams, HandshakeResult};
 use bsk_protocol::{BrowserPeerInfo, Method, RequestFrame, ResponseBody, ResponseFrame};
 use futures_util::{SinkExt, StreamExt};
-use tokio::net::TcpListener;
 use tokio_tungstenite::tungstenite::handshake::client::generate_key;
 use tokio_tungstenite::tungstenite::http::Request;
 use tokio_tungstenite::tungstenite::protocol::Message;
@@ -17,9 +16,7 @@ pub const TEST_EXT_ID: &str = "abcdefghijklmnopabcdefghijklmnop"; // 32 chars in
 
 pub async fn spawn_daemon() -> daemon::DaemonHandle {
     // Bind to any free TCP port.
-    let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
-    let port = probe.local_addr().unwrap().port();
-    drop(probe);
+    let port = 0;
 
     let config = DaemonConfig::new(port);
     daemon::run(config, None).await.unwrap()