From cf49a59d55cc2617d38561228272d880eec07600 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 1 Sep 2026 14:23:32 +0530 Subject: [PATCH 1/2] fix(gl): make doctor exit non-zero on Fail-class checks (#357) gl doctor printed per-row status and 'Some checks failed' prose, but its exit code was always 0. A diagnostic whose exit status cannot express failure is a trap waiting for the first 'gl doctor && ...' to land somewhere it should not (#357). Three changes: 1. Re-tier the 'GITLAWB_NODE unset' check from Fail to Warn. The CLI's --node flag defaults to https://node.gitlawb.com (PUBLIC_NODE) and the CLI works fine without the env var, so an unset env is advisory, not a failure. Without this re-tier the obvious 'return non-zero on any failure' change would have flipped gl doctor to exit 1 on a stock working install, which is exactly the regression the issue warns against. 2. Route the exit-code decision through a new has_failures(&[Check]) helper: exit 1 if and only if at least one row is Fail-class. Warn-class rows (iCaptcha offline, version drift, shell-alias shadowing, GITLAWB_NODE unset) keep the process at exit 0 because those conditions are still printed to the user and the obvious '&& pipeline' pattern stays valid. Use std::process::exit(1) rather than returning Err so anyhow's error frame does not duplicate the user-facing summary that already prints. 3. Three unit tests pin the new predicate against the three regimes: all-Ok, warn-only, and a single Fail row tripping the exit. The warn-only case is the regression guard for the re-tier: if anyone flips GITLAWB_NODE back to Fail, exit_predicate_is_false_for_warn_only fails too, so the two halves of the fix cannot drift apart. Verified end-to-end against the public gitlawb node: * unset GITLAWB_NODE, missing identity/registration -> exit 1 * unset GITLAWB_NODE, full healthy install -> exit 0 (warn) * missing git-remote-gitlawb -> exit 1 --- crates/gl/src/doctor.rs | 91 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 9 deletions(-) diff --git a/crates/gl/src/doctor.rs b/crates/gl/src/doctor.rs index 86f503344..dd70393da 100644 --- a/crates/gl/src/doctor.rs +++ b/crates/gl/src/doctor.rs @@ -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"); @@ -156,8 +155,14 @@ pub async fn run(args: DoctorArgs) -> Result<()> { Ok(v) if !v.is_empty() => { checks.push(Check::pass("GITLAWB_NODE", v.to_string())); } + // `--node` defaults to `https://node.gitlawb.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). _ => { - checks.push(Check::fail( + checks.push(Check::warn( "GITLAWB_NODE", "not set — git-remote-gitlawb will fall back to http://127.0.0.1:7545", "export GITLAWB_NODE=https://node.gitlawb.com", @@ -285,23 +290,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 +314,31 @@ 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(()) } +/// 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 +664,57 @@ 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)); + } } From 787acea233a17bb75d68c36811bf86783662390b Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Wed, 2 Sep 2026 13:02:31 +0530 Subject: [PATCH 2/2] fix(gl): pin doctor exit wiring and GITLAWB_NODE Warn tier - Extract GITLAWB_NODE env classification into gitlawb_node_env_check() so unit tests can pin the Warn tier directly; the previous has_failures-only tests built their own Check::warn rows and could not see a re-tier back to Fail. - Add three unit tests for the extracted helper (unset/empty is Warn, set is Pass, loopback is Pass). - Add integration probe crates/gl/tests/doctor_exit.rs that drives the real binary with a temp --dir and --node http://127.0.0.1:1 and asserts exit 1. Deleting the std::process::exit(1) gate keeps the unit suite green but breaks this probe, closing the gap reviewer noted. Co-authored-by: review fix for #391 --- crates/gl/src/doctor.rs | 94 +++++++++++++++++++++++----------- crates/gl/tests/doctor_exit.rs | 48 +++++++++++++++++ 2 files changed, 113 insertions(+), 29 deletions(-) create mode 100644 crates/gl/tests/doctor_exit.rs diff --git a/crates/gl/src/doctor.rs b/crates/gl/src/doctor.rs index dd70393da..24ce432f9 100644 --- a/crates/gl/src/doctor.rs +++ b/crates/gl/src/doctor.rs @@ -140,35 +140,9 @@ 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())); - } - // `--node` defaults to `https://node.gitlawb.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). - _ => { - checks.push(Check::warn( - "GITLAWB_NODE", - "not set — git-remote-gitlawb will fall back to http://127.0.0.1:7545", - "export GITLAWB_NODE=https://node.gitlawb.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); @@ -332,6 +306,37 @@ pub async fn run(args: DoctorArgs) -> Result<()> { 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.gitlawb.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.gitlawb.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", + "export GITLAWB_NODE=https://node.gitlawb.com", + ), + } +} + /// 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. @@ -717,4 +722,35 @@ mod tests { ]; 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)); + } + + #[test] + fn gitlawb_node_env_set_is_pass() { + let check = gitlawb_node_env_check(Some("https://node.gitlawb.com")); + assert!(matches!(check.state, CheckState::Ok)); + assert!(check.detail.contains("https://node.gitlawb.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" + ); +}