diff --git a/crates/gl/src/doctor.rs b/crates/gl/src/doctor.rs index 50d1221c1..110bc88de 100644 --- a/crates/gl/src/doctor.rs +++ b/crates/gl/src/doctor.rs @@ -3,7 +3,7 @@ //! Checks: //! 1. Identity — ~/.gitlawb/identity.pem exists, DID parseable //! 2. Registration — ~/.gitlawb/ucan.json exists (registered with a node) -//! 3. GITLAWB_NODE — env var is set to a non-localhost URL +//! 3. GITLAWB_NODE — env var points at a public node (unset is advisory) //! 4. Node — GITLAWB_NODE is reachable (default: https://node.twigpine.com) //! 5. git helper — git-remote-gitlawb is in PATH //! 6. git — git is installed @@ -80,7 +80,6 @@ pub async fn run(args: DoctorArgs) -> Result<()> { }); let mut checks = Vec::new(); - let mut all_ok = true; // ── 1. Identity ─────────────────────────────────────────────────────── let pem_path = dir.join("identity.pem"); @@ -141,32 +140,16 @@ pub async fn run(args: DoctorArgs) -> Result<()> { } // ── 3. GITLAWB_NODE env var ─────────────────────────────────────────── - match std::env::var("GITLAWB_NODE") { - // A loopback host is a legitimate setup (self-hosted node, dev - // harness) — the connectivity check below fails loudly if it is not - // actually reachable, so don't red-flag the configuration itself. - Ok(v) if is_loopback_url(&v) => { - checks.push(Check::pass( - "GITLAWB_NODE", - format!( - "{v} (local node — intentional for self-hosting/dev; unset to target the public network)" - ), - )); - } - Ok(v) if !v.is_empty() => { - checks.push(Check::pass("GITLAWB_NODE", v.to_string())); - } - _ => { - checks.push(Check::fail( - "GITLAWB_NODE", - "not set — git-remote-gitlawb will fall back to http://127.0.0.1:7545", - "export GITLAWB_NODE=https://node.twigpine.com", - )); - } - } + checks.push(gitlawb_node_env_check( + std::env::var("GITLAWB_NODE").ok().as_deref(), + )); // ── 4. Node connectivity ────────────────────────────────────────────── - let client = NodeClient::new(&args.node, None); + // GITLAWB_NODE="" binds args.node to "" through clap, and check 3 groups + // empty with unset. The probe must apply the same normalization or it + // would dial "" and Fail while the env row only Warns. + let node_url = effective_node_url(&args.node); + let client = NodeClient::new(node_url, None); match client.get("/").await { Ok(resp) if resp.status().is_success() => { let info = resp.json::().await.unwrap_or_default(); @@ -175,7 +158,7 @@ pub async fn run(args: DoctorArgs) -> Result<()> { let short_did = did.chars().take(30).collect::(); checks.push(Check::pass( "node", - format!("{} — v{version} ({short_did}…)", args.node), + format!("{} — v{version} ({short_did}…)", node_url), )); // Capability drift: a node newer than this CLI may require features // (RFC 9421 signing, iCaptcha) the CLI doesn't speak. @@ -194,14 +177,14 @@ pub async fn run(args: DoctorArgs) -> Result<()> { Ok(resp) => { checks.push(Check::fail( "node", - format!("{} returned HTTP {}", args.node, resp.status()), + format!("{} returned HTTP {}", node_url, resp.status()), "check GITLAWB_NODE env var or try gl register --node ", )); } Err(e) => { checks.push(Check::fail( "node", - format!("{} unreachable: {e}", args.node), + format!("{} unreachable: {e}", node_url), "check your internet connection or set GITLAWB_NODE", )); } @@ -285,23 +268,21 @@ pub async fn run(args: DoctorArgs) -> Result<()> { CheckState::Fail => "✗", }; println!(" {icon} {:<24} {}", check.label, check.detail); - if matches!(check.state, CheckState::Fail) { - all_ok = false; - } } println!(); + let has_failures_now = has_failures(&checks); let has_issues = checks .iter() .any(|c| matches!(c.state, CheckState::Fail | CheckState::Warn)); if !has_issues { println!("Everything looks good. Run `gl quickstart` to create your first repo."); } else { - if all_ok { - println!("Setup looks good with some warnings:"); - } else { + if has_failures_now { println!("Some checks failed. Suggested fixes:"); + } else { + println!("Setup looks good with some warnings:"); } for check in &checks { if matches!(check.state, CheckState::Fail | CheckState::Warn) { @@ -311,14 +292,75 @@ pub async fn run(args: DoctorArgs) -> Result<()> { } } println!(); - if !all_ok { + if has_failures_now { println!("Run `gl quickstart` for a guided setup."); } } + // #357: a non-zero exit is the only signal scripting and CI have. Warn-only + // rows stay exit 0 (iCaptcha offline, version drift, shell-alias shadowing + // are all non-fatal); Fail-class rows (identity, registration, node + // reachable, git-remote-gitlawb present, git present) flip to exit 1. + // Direct `process::exit` rather than returning Err: the prose above is the + // user-facing summary, and an anyhow Error frame would just duplicate it. + if has_failures(&checks) { + std::process::exit(1); + } + Ok(()) } +/// Classify the `GITLAWB_NODE` env var for `gl doctor`. Extracted so unit +/// tests can pin the Warn tier directly (#357): an unset env is advisory, not +/// a failure, because `--node` defaults to `https://node.twigpine.com` and the +/// CLI works without it. Flipping this back to `Check::fail` must break a +/// test, not just change prose. +fn gitlawb_node_env_check(env_val: Option<&str>) -> Check { + match env_val { + // A loopback host is a legitimate setup (self-hosted node, dev + // harness) — the connectivity check below fails loudly if it is not + // actually reachable, so don't red-flag the configuration itself. + Some(v) if is_loopback_url(v) => Check::pass( + "GITLAWB_NODE", + format!( + "{v} (local node — intentional for self-hosting/dev; unset to target the public network)" + ), + ), + Some(v) if !v.is_empty() => Check::pass("GITLAWB_NODE", v.to_string()), + // `--node` defaults to `https://node.twigpine.com` and the CLI works + // fine without the variable, so an unset env is advisory, not a + // failure. Warn-class keeps the exit code 0 for a stock working + // install; this used to be Fail, which the obvious "return non-zero + // on any failure" change would have flipped to exit 1 on every + // default-config install (#357). + _ => Check::warn( + "GITLAWB_NODE", + "not set — git-remote-gitlawb will fall back to http://127.0.0.1:7545", + format!("export GITLAWB_NODE={PUBLIC_NODE}"), + ), + } +} + +/// The URL the connectivity probe should dial. `GITLAWB_NODE` set but empty +/// (or whitespace-only) binds `args.node` to `""` through clap rather than +/// to the `default_value`, and check 3 already groups empty with unset, so +/// the probe applies the same normalization: an empty effective URL means +/// the public node, same as unset. +fn effective_node_url(node: &str) -> &str { + if node.trim().is_empty() { + PUBLIC_NODE + } else { + node + } +} + +/// True when at least one check is Fail-class. Warn-only runs (iCaptcha offline, +/// version drift, shell-alias shadowing, GITLAWB_NODE unset) keep exit 0; +/// this is the gating predicate for #357's exit-status fix. +fn has_failures(checks: &[Check]) -> bool { + checks.iter().any(|c| matches!(c.state, CheckState::Fail)) +} + /// Check if a binary name exists anywhere on PATH. /// True when the rc file contains a real `unalias` command naming `gl` — /// not a comment, and not a longer word like `unalias global`. Ordering @@ -644,4 +686,103 @@ mod tests { assert!(matches!(check.state, CheckState::Ok)); assert!(check.detail.contains("GitHub API returned HTTP 403")); } + + /// #357: an all-Ok run does not flip the exit code to non-zero — a healthy + /// install must keep returning 0 so scripts like `gl doctor && gl push` + /// keep working. + #[test] + fn exit_predicate_is_false_for_all_ok() { + let checks = vec![ + Check::pass("identity", "ok"), + Check::pass("registration", "ok"), + Check::pass("git", "ok"), + ]; + assert!(!has_failures(&checks)); + } + + /// #357: Warn-class rows (iCaptcha offline, version drift, shell-alias + /// shadowing, GITLAWB_NODE unset on a default-config install) are NOT + /// exit-status failures — they are still printed to the user but the + /// process exits 0. This is the rule that the previous "any + /// Fail-or-Warn row → exit 1" change would have broken for stock + /// installs that simply have not set the env var. + #[test] + fn exit_predicate_is_false_for_warn_only() { + let checks = vec![ + Check::pass("identity", "ok"), + Check::warn("GITLAWB_NODE", "unset", "export GITLAWB_NODE=..."), + Check::warn("iCaptcha", "offline", "check connectivity"), + Check::warn("version", "drift", "upgrade"), + Check::warn("shell alias", "omz git plugin", "unalias gl"), + ]; + assert!(!has_failures(&checks)); + } + + /// #357: a single Fail-class row trips the exit code. The set of Fail + /// rows is whatever `run` writes through `Check::fail`: identity missing + /// or unparseable, registration missing/malformed, node unreachable, + /// git-remote-gitlawb absent, git absent. + #[test] + fn exit_predicate_is_true_when_a_fail_row_is_present() { + let checks = vec![ + Check::pass("registration", "ok"), + Check::fail("identity", "missing", "gl identity new"), + Check::pass("git", "ok"), + ]; + assert!(has_failures(&checks)); + + let mixed_warns = vec![ + Check::pass("identity", "ok"), + Check::warn("GITLAWB_NODE", "unset", "export"), + Check::fail("node", "unreachable", "check network"), + Check::warn("iCaptcha", "offline", "check"), + ]; + assert!(has_failures(&mixed_warns)); + } + + /// #357: `GITLAWB_NODE` unset must be Warn, not Fail. This pins the + /// re-tier so flipping it back to `Check::fail` breaks the suite — the + /// previous `has_failures`-only tests built their own `Check::warn` rows + /// and could not see the re-tier at all. + #[test] + fn gitlawb_node_env_unset_is_warn() { + let check = gitlawb_node_env_check(None); + assert!(matches!(check.state, CheckState::Warn)); + assert!(check.detail.contains("not set")); + let empty = gitlawb_node_env_check(Some("")); + assert!(matches!(empty.state, CheckState::Warn)); + } + + /// `GITLAWB_NODE=""` binds `args.node` to `""` through clap, and check 3 + /// groups empty with unset. The probe must dial the same URL unset + /// would — removing the normalization makes check 4 probe `""` and Fail + /// while the env row only Warns, so an empty env install exits 1. + #[test] + fn effective_node_url_treats_empty_and_blank_as_unset() { + assert_eq!(effective_node_url(""), PUBLIC_NODE); + assert_eq!(effective_node_url(" "), PUBLIC_NODE); + assert_eq!(effective_node_url("\t\n"), PUBLIC_NODE); + assert_eq!( + effective_node_url("http://127.0.0.1:7545"), + "http://127.0.0.1:7545" + ); + } + + #[test] + fn gitlawb_node_env_set_is_pass() { + let check = gitlawb_node_env_check(Some("https://node.twigpine.com")); + assert!(matches!(check.state, CheckState::Ok)); + assert!(check.detail.contains("https://node.twigpine.com")); + } + + #[test] + fn gitlawb_node_env_loopback_is_pass() { + for url in ["http://127.0.0.1:7545", "http://localhost:7545"] { + let check = gitlawb_node_env_check(Some(url)); + assert!( + matches!(check.state, CheckState::Ok), + "loopback {url} must be Ok, not Warn/Fail" + ); + } + } } diff --git a/crates/gl/tests/doctor_exit.rs b/crates/gl/tests/doctor_exit.rs new file mode 100644 index 000000000..d29f353f9 --- /dev/null +++ b/crates/gl/tests/doctor_exit.rs @@ -0,0 +1,48 @@ +//! #357: pin the `gl doctor` exit-code wiring, not just the predicate. +//! +//! The unit tests for `has_failures` never touch `std::process::exit(1)` or +//! the `GITLAWB_NODE` Warn tier. Deleting the exit gate outright kept +//! `cargo test -p gl doctor::` green, so this binary probe is the only thing +//! that breaks when the wiring is gutted. + +use std::process::Command; + +#[test] +fn doctor_exits_nonzero_when_fail_rows_present() { + let dir = tempfile::tempdir().expect("temp dir"); + // Use an unroutable node so the "node unreachable" check is Fail-class. + // The temp dir has no identity.pem / ucan.json, so identity + registration + // are also Fail — at least one Fail must flip the process to exit 1. + let bin = env!("CARGO_BIN_EXE_gl"); + let output = Command::new(bin) + .args([ + "doctor", + "--dir", + dir.path().to_str().unwrap(), + "--node", + "http://127.0.0.1:1", + ]) + // Inherit PATH but neutralize GITLAWB_NODE so the test is deterministic + // across machines that have it set (otherwise it would be Pass). + .env_remove("GITLAWB_NODE") + // iCaptcha probes a real URL by default; keep it unreachable too so + // it stays Warn and does not affect the Fail gating. + .env("GITLAWB_ICAPTCHA_URL", "http://127.0.0.1:1") + .output() + .expect("run gl doctor"); + + // The command should have exited 1 because at least one Fail row exists. + // If the `std::process::exit(1)` gate is removed, this becomes 0 and the + // test fails — that is the regression it pins. + assert!( + !output.status.success(), + "gl doctor must exit non-zero when Fail rows are present; stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + output.status.code(), + Some(1), + "exit code must be 1, not some other non-zero" + ); +}