fix(gl): make doctor exit non-zero on Fail-class checks (#357)

[Ayush7614]

Closes #357.

What

gl doctor printed per-row ✓/⚠/✗ status and the 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 shouldn't (#357).

Fix

Three changes in crates/gl/src/doctor.rs:

1. Re-tier GITLAWB_NODE unset 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) directly 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.

Why these tiers

Per the issue, the Fail-class checks are: identity missing or unparseable, registration missing or malformed, node unreachable or non-2xx, git-remote-gitlawb absent, git absent. The Warn-class checks are: iCaptcha reachability, shell-alias shadowing, version drift, and now GITLAWB_NODE unset.

Verification

cargo fmt --all -- --check                                # clean
cargo check -p gl --all-targets                           # OK
cargo clippy -p gl --all-targets -- -D warnings           # OK
cargo test -p gl                                          # 366 passed; 0 failed
cargo test -p gl doctor::                                 # 19 passed; 0 failed

End-to-end against the public gitlawb node (/doctor is the binary):

scenario	rows	exit
unset GITLAWB_NODE, no identity/registration	✗ identity, ✗ registration, ⚠ GITLAWB_NODE, ✗ git-remote-gitlawb	1
unset GITLAWB_NODE, full healthy install	✓ x7, ⚠ GITLAWB_NODE	0
missing git-remote-gitlawb in PATH	✗ git-remote-gitlawb, ✗ git	1

The first row is the new exit-on-Fail path; the second is the regression-guard for the re-tier; the third confirms the git helper absence still fails the install.

(Ignored the unrelated pre-existing clippy::duplicated_attributes at crates/gitlawb-node/src/api/ipfs.rs:2169 — present on upstream/main before this PR; not touched here.)

[coderabbitai]

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: c3d5a0f2-7b2f-495f-9808-6c0e6cf5b61c

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44f9 and 787acea.

📒 Files selected for processing (2)

• crates/gl/src/doctor.rs
• crates/gl/tests/doctor_exit.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[beardthelion]

Core is right: the exit gate works, the re-tier is the one #357 asked for, and the tier lists match the issue. One ask before this merges.

Findings

• [P2] Pin the exit wiring, not just the predicate
  crates/gl/src/doctor.rs:325
  The three added tests assert has_failures over hand-built Vec<Check>, so they never touch the std::process::exit(1) call or the Check::warn re-tier. I deleted the exit gate outright and cargo test -p gl doctor:: stayed at 19 passed / 0 failed; a binary probe on the same two trees gave exit 1 pristine and exit 0 gutted, so the mutation is behavior-changing and the suite cannot see it. Same for the re-tier: flipping GITLAWB_NODE back to Check::fail leaves exit_predicate_is_false_for_warn_only green, because that test builds its own warn rows rather than reading the ones run writes. An integration test under crates/gl/tests/ driving env!("CARGO_BIN_EXE_gl") with a temp --dir and --node http://127.0.0.1:1 closes the first half; I wrote it, confirmed it passes on your head and fails when the exit gate is removed, and tempfile is already a gl dev-dependency so it costs no new dep. For the re-tier half, extracting the env-classification arm into a small function returning a Check would let a unit test pin Warn directly.

Not asks

The GITLAWB_NODE re-tier stands as you wrote it. One thing that surfaced underneath it is mine, not yours: git-remote-gitlawb falls back to http://127.0.0.1:7545 when the env is unset (crates/git-remote-gitlawb/src/main.rs:68) while every gl subcommand defaults to https://node.gitlawb.com, so on a stock install doctor probes and greenlights a node the transport will never contact and exits 0 while pushes are broken. That predates this PR and #357 did not confront it. I will file it separately rather than hang it on this change.

Verified here: full diff at base bfc44f9, both production changes mutated one at a time with the suite re-run, binary exit codes driven end to end on a broken install and on an unset-env install, doctor::run callers grepped (crates/gl/src/main.rs:171 is the only one, and nothing in-repo consumes the exit status), and the gl vs git-remote-gitlawb node defaults read side by side.

[Ayush7614]

Thanks for the review @beardthelion

Addressed review feedback (pushed 787acea):

P2 – Pin exit wiring, not just predicate crates/gl/src/doctor.rs:325: Done.

• Extracted GITLAWB_NODE env classification into gitlawb_node_env_check(Option<\&str>) -> Check (crates/gl/src/doctor.rs:316). This lets unit tests pin the Warn tier directly – flipping it back to Check::fail now breaks gitlawb_node_env_unset_is_warn. Added:

  • gitlawb_node_env_unset_is_warn (None/"" → Warn)
  • gitlawb_node_env_set_is_pass
  • gitlawb_node_env_loopback_is_pass

• Added binary probe crates/gl/tests/doctor_exit.rs:10 driving env!("CARGO_BIN_EXE_gl") with tempfile::TempDir + --dir + --node http://127.0.0.1:1 and GITLAWB_NODE unset, asserting exit 1. Deleting std::process::exit(1) keeps cargo test -p gl doctor:: green but makes this probe fail (verified locally: pristine 1, gutted 0). tempfile already a dev-dep, no new dep.

Verification: cargo check -p gl --tests --all-targets OK, cargo clippy -p gl -- -D warnings OK, cargo test -p gl 22 passed + 1 integration passed, cargo fmt --check clean. --node default vs git-remote-gitlawb fallback drift noted as out-of-scope per your \“Not asks\”.

Thanks for the detailed mutation notes – both halves (exit gate + re-tier) are now independently load-bearing.

Addressed on 787acea: exit wiring probe and GITLAWB_NODE Warn tier pinned.

[beardthelion]

Re-reviewed head 787acea against base bfc44f92. The prior ask is landed: gitlawb_node_env_check pins the Warn tier directly, and crates/gl/tests/doctor_exit.rs drives the real gl binary for exit 1. I mutation-checked both guards (exit gate removal and re-tier flip each go RED). cargo test -p gl -- doctor:: doctor_exits is 22 unit + 1 integration green locally; CI is 12/12 on the push.

Not an ask, recorded only: the module doc at crates/gl/src/doctor.rs:6 still says check 3 requires a non-localhost URL; behavior is now Warn on unset/empty. Optional one-line doc fix, not blocking.

Not an ask, recorded only: with GITLAWB_NODE unset, doctor probes --node (default https://node.gitlawb.com) while git-remote-gitlawb falls back to 127.0.0.1:7545. That predates this PR; tracked separately (#394 class).

One process note, not a finding: rebasing may conflict with #331/#394 on doctor.rs.

[jatmn]

LGTM

[beardthelion]

@kevincodex1 LGTM

[euxaristia]

Exit 1 exactly when a Fail-class row exists, warn-only stays 0, and the GITLAWB_NODE-unset re-tier from Fail to Warn avoids the stock-install regression #357 warned about, pinned so it cannot flip back silently. The binary-level probe running the real gl binary and asserting exit code exactly 1 is the only test that catches a deleted exit gate, and the PR says so candidly.