fix(gl): make doctor diagnose the node the git transport will use

`gl doctor` reported a healthy node and exited 0 on a stock install whose
`git push` and `git clone` could not reach anything. `gl`'s `--node` defaults to
https://node.gitlawb.com while `git-remote-gitlawb` fell back to
http://127.0.0.1:7545, so with GITLAWB_NODE unset the check validated a URL the
transport never contacts. An explicit `--node` outranks the environment, so the
two could disagree even when the variable was set.

Both binaries now resolve through `gitlawb_core::resolve_transport_node`, and
doctor probes whatever that returns whenever it differs from its own node,
folding the result into the existing GITLAWB_NODE row rather than adding a
second row for the same condition.

Three things the probe does that the first attempt did not. It requires a `did`
in the response before reporting the transport usable, because any local service
answering 200 on that port otherwise read as a working transport, which is the
same false green this change exists to remove. It uses a dedicated client with
`no_proxy` and a 3s timeout, since the shared client routes a loopback probe
through HTTP_PROXY and inherits a 30s timeout that stalled every stock run
behind a hung listener. And it sanitizes the node URL and single-quotes it in the
suggested-fix line, which is printed for the user to paste.

A blank or whitespace-only GITLAWB_NODE now resolves to the local default in both
binaries. It previously gave the helper an empty base, so every clone URL lost its
scheme and host.

Tiering follows who chose the broken value: an unset variable stays advisory
because `gl` itself still works and #357 requires a stock install to keep
exiting 0, while a variable the user set to something unusable fails.

crates/git-remote-gitlawb/src/main.rs

@@ -25,6 +25,7 @@
 use anyhow::{bail, Context, Result};
 use gitlawb_core::http_sig::sign_request;
 use gitlawb_core::identity::Keypair;
+use gitlawb_core::{resolve_transport_node, DEFAULT_LOCAL_NODE};
 use std::io::{self, BufRead, Read, Write};
 
 fn main() -> Result<()> {
@@ -63,9 +64,9 @@ fn main() -> Result<()> {
 
     let (_, short_owner, repo_name) = parse_gitlawb_url(url)?;
 
-    // v0.1: default to localhost. Override with GITLAWB_NODE env var.
-    let node_base =
-        std::env::var("GITLAWB_NODE").unwrap_or_else(|_| "http://127.0.0.1:7545".to_string());
+    // v0.1: default to localhost. Override with GITLAWB_NODE env var. Shared with
+    // `gl doctor` so the diagnostic reports the URL this actually resolves.
+    let node_base = node_base_from_env();
     let repo_base = format!("{}/{}/{}", node_base, short_owner, repo_name);
     tracing::debug!("repo_base: {repo_base}");
 
@@ -78,6 +79,12 @@ fn main() -> Result<()> {
     run_helper(&repo_base, keypair.as_ref())
 }
 
+/// The node base URL this helper will contact, resolved through the same function
+/// `gl doctor` reports on so the diagnostic cannot describe a different node.
+fn node_base_from_env() -> String {
+    resolve_transport_node(std::env::var("GITLAWB_NODE").ok().as_deref())
+}
+
 // ── CLI argument handling ──────────────────────────────────────────────────────
 
 /// How the binary was invoked, derived from its CLI arguments.
@@ -123,7 +130,7 @@ fn help_text() -> String {
          \x20   git clone gitlawb://did:key:z6Mk.../<repo>\n\
          \n\
          ENVIRONMENT:\n\
-         \x20   GITLAWB_NODE   Node base URL (default: http://127.0.0.1:7545)\n\
+         \x20   GITLAWB_NODE   Node base URL (default: {DEFAULT_LOCAL_NODE})\n\
          \x20   GITLAWB_KEY    Identity PEM path for signed fetch/push (default: ~/.gitlawb/identity.pem)\n\
          \x20   GITLAWB_LOG    Log filter (default: warn)\n\
          \n\
@@ -2014,9 +2021,21 @@ mod tests {
         assert!(help.contains("--version"));
         assert!(help.contains("--help"));
         assert!(help.contains("GITLAWB_NODE"));
+        // The advertised default must be the value the helper actually resolves.
+        assert!(help.contains(DEFAULT_LOCAL_NODE));
         assert!(help.ends_with('\n'));
     }
 
+    /// The helper's own resolution must stay the shared one; a site walking back to
+    /// a private literal is exactly the drift the shared function exists to stop.
+    #[test]
+    fn node_base_matches_the_shared_resolver() {
+        // No env manipulation: assert the call site delegates, for the default case.
+        let expected = resolve_transport_node(std::env::var("GITLAWB_NODE").ok().as_deref());
+        assert_eq!(node_base_from_env(), expected);
+        assert_eq!(resolve_transport_node(None), DEFAULT_LOCAL_NODE);
+    }
+
     // ── #117 multi-round fetch negotiation ───────────────────────────────────
 
     /// Encode a git pkt-line: 4-byte hex length (incl. the 4 bytes) + data.

crates/gitlawb-core/src/lib.rs

@@ -17,5 +17,50 @@ pub mod sanitize;
 pub mod scan_token;
 pub mod ucan;
 
+/// Node URL the git transport falls back to when `GITLAWB_NODE` is unset.
+///
+/// `gl` defaults to the public node instead, so the two disagree on an install
+/// that never sets the variable.
+pub const DEFAULT_LOCAL_NODE: &str = "http://127.0.0.1:7545";
+
+/// The node `git clone` and `git push` will contact, given the raw `GITLAWB_NODE`
+/// value (`None` when the variable is absent).
+///
+/// `git-remote-gitlawb` calls this to pick its base URL and `gl doctor` calls it
+/// to report that URL, so a diagnostic cannot describe a node the transport will
+/// not use. A blank or whitespace-only value is not a configured node: treating
+/// it as one gave the helper an empty base and every clone URL a missing scheme
+/// and host.
+pub fn resolve_transport_node(env_value: Option<&str>) -> String {
+    match env_value.map(str::trim) {
+        Some(v) if !v.is_empty() => v.to_string(),
+        _ => DEFAULT_LOCAL_NODE.to_string(),
+    }
+}
+
 pub use error::Error;
 pub type Result<T> = std::result::Result<T, Error>;
+
+#[cfg(test)]
+mod transport_node_tests {
+    use super::*;
+
+    #[test]
+    fn absent_or_blank_resolves_to_the_local_default() {
+        for raw in [None, Some(""), Some("   "), Some("\t\n")] {
+            assert_eq!(resolve_transport_node(raw), DEFAULT_LOCAL_NODE, "{raw:?}");
+        }
+    }
+
+    #[test]
+    fn a_configured_value_wins_and_is_trimmed() {
+        assert_eq!(
+            resolve_transport_node(Some("https://n.example")),
+            "https://n.example"
+        );
+        assert_eq!(
+            resolve_transport_node(Some("  https://n.example  ")),
+            "https://n.example"
+        );
+    }
+}

crates/gl/src/doctor.rs

@@ -13,7 +13,9 @@ use anyhow::Result;
 use clap::Args;
 use std::path::PathBuf;
 
-use crate::http::NodeClient;
+use gitlawb_core::resolve_transport_node;
+
+use crate::http::{sanitize_node_msg, NodeClient};
 
 const PUBLIC_NODE: &str = "https://node.gitlawb.com";
 const GITHUB_API_BASE: &str = "https://api.github.com";
@@ -141,27 +143,51 @@ 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.gitlawb.com",
-            ));
+    // The node `git clone` / `git push` will contact is not always the node `gl`
+    // contacts: `--node` defaults to the public node and an explicit flag outranks
+    // the environment, so the two diverge whenever the variable is unset, blank, or
+    // overridden. Probing only `gl`'s node is what let doctor greenlight an install
+    // whose transport was dead, so resolve the transport's node through the same
+    // function the helper uses and report on that one.
+    let env_node = std::env::var("GITLAWB_NODE").ok();
+    let transport_node = resolve_transport_node(env_node.as_deref());
+    let gl_node = sanitize_node_msg(&args.node);
+
+    if transport_node == args.node {
+        // gl and the transport agree, so one row covers both. A loopback host is a
+        // legitimate setup (self-hosted node, dev harness) and the connectivity
+        // check below fails loudly if it is not actually reachable, so don't
+        // red-flag the configuration itself.
+        let detail = if is_loopback_url(&transport_node) {
+            format!(
+                "{gl_node} (local node, intentional for self-hosting/dev; unset to target the public network)"
+            )
+        } else {
+            gl_node.clone()
+        };
+        checks.push(Check::pass("GITLAWB_NODE", detail));
+    } else {
+        let verdict = probe_transport(&transport_node).await;
+        let detail = format!(
+            "git push/clone will use {} ({}); gl targets {gl_node}",
+            sanitize_node_msg(&transport_node),
+            verdict.detail
+        );
+        // Single-quote the value: this line is printed under "Suggested fixes" for
+        // the user to paste into a shell, and the URL is caller-supplied.
+        let fix = format!("export GITLAWB_NODE='{gl_node}'");
+        // Tiering follows who chose the broken value. An unset variable on a stock
+        // install is advisory: `gl` works, only `git push`/`git clone` do not, and
+        // #357 requires that install to keep exiting 0. A variable the user set to
+        // something unusable is a real misconfiguration.
+        let env_was_set = env_node
+            .as_deref()
+            .map(str::trim)
+            .is_some_and(|v| !v.is_empty());
+        if verdict.usable || !env_was_set {
+            checks.push(Check::warn("GITLAWB_NODE", detail, fix));
+        } else {
+            checks.push(Check::fail("GITLAWB_NODE", detail, fix));
         }
     }
 
@@ -319,6 +345,63 @@ pub async fn run(args: DoctorArgs) -> Result<()> {
     Ok(())
 }
 
+/// Outcome of probing the node the git transport will use.
+struct TransportVerdict {
+    /// True only when the endpoint identified itself as a gitlawb node. A bare 200
+    /// is not enough: any local service answering on that port would otherwise be
+    /// reported as a working transport, which is the false green this check exists
+    /// to catch.
+    usable: bool,
+    detail: String,
+}
+
+/// Probe `url` and describe what is actually there.
+///
+/// Deliberately not `NodeClient`: this call must not follow the user's proxy
+/// (a proxy swallows a loopback probe and reports a running local node as dead)
+/// and must not inherit the 30s request timeout, which stalls every stock
+/// `gl doctor` run behind a hung listener.
+async fn probe_transport(url: &str) -> TransportVerdict {
+    let client = match reqwest::Client::builder()
+        .no_proxy()
+        .timeout(std::time::Duration::from_secs(3))
+        .build()
+    {
+        Ok(c) => c,
+        Err(e) => {
+            return TransportVerdict {
+                usable: false,
+                detail: format!("probe failed: {}", sanitize_node_msg(&e.to_string())),
+            }
+        }
+    };
+    let target = format!("{}/", url.trim_end_matches('/'));
+    match client.get(&target).send().await {
+        Ok(resp) if resp.status().is_success() => {
+            let info = resp.json::<serde_json::Value>().await.unwrap_or_default();
+            if info["did"].as_str().is_some() {
+                TransportVerdict {
+                    usable: true,
+                    detail: "reachable".to_string(),
+                }
+            } else {
+                TransportVerdict {
+                    usable: false,
+                    detail: "something is listening but it is not a gitlawb node".to_string(),
+                }
+            }
+        }
+        Ok(resp) => TransportVerdict {
+            usable: false,
+            detail: format!("returned HTTP {}", resp.status()),
+        },
+        Err(e) => TransportVerdict {
+            usable: false,
+            detail: format!("unreachable: {}", sanitize_node_msg(&e.to_string())),
+        },
+    }
+}
+
 /// 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

crates/gl/tests/doctor_transport_node.rs

@@ -0,0 +1,204 @@
+//! `gl doctor` must diagnose the node the git transport will actually use.
+//!
+//! `gl`'s `--node` defaults to the public node while `git-remote-gitlawb` falls
+//! back to a local one, and an explicit `--node` outranks the environment. So the
+//! two disagree whenever GITLAWB_NODE is unset, blank, or overridden, and doctor's
+//! own `node` row describes a URL `git clone` and `git push` never contact.
+
+use std::io::{Read, Write};
+use std::net::TcpListener;
+use std::process::Command;
+
+/// Substring that marks the divergence clause. Present only when gl and the git
+/// transport resolve different nodes.
+const DIVERGENCE: &str = "git push/clone will use";
+
+struct Run {
+    stdout: String,
+    code: Option<i32>,
+}
+
+impl Run {
+    /// The single output line carrying the divergence clause, so an assertion
+    /// cannot be satisfied by an unrelated row that happens to print the same URL.
+    fn divergence_line(&self) -> Option<&str> {
+        self.stdout.lines().find(|l| l.contains(DIVERGENCE))
+    }
+}
+
+fn doctor(env_node: Option<&str>, gl_node: &str) -> Run {
+    let dir = tempfile::tempdir().unwrap();
+    let mut cmd = Command::new(env!("CARGO_BIN_EXE_gl"));
+    cmd.args(["doctor", "--node", gl_node])
+        .arg("--dir")
+        .arg(dir.path().join("gitlawb"))
+        // Keep the run off the network: doctor also probes iCaptcha and the
+        // GitHub release API, neither of which this test is about.
+        .env("GITLAWB_ICAPTCHA_URL", "http://127.0.0.1:1");
+    match env_node {
+        Some(v) => cmd.env("GITLAWB_NODE", v),
+        None => cmd.env_remove("GITLAWB_NODE"),
+    };
+    let out = cmd.output().unwrap();
+    Run {
+        stdout: String::from_utf8_lossy(&out.stdout).into_owned(),
+        code: out.status.code(),
+    }
+}
+
+/// GITLAWB_NODE unset: the helper falls back to the local node, gl targets the
+/// public one, so the row must name the local node as the transport target.
+#[test]
+fn unset_env_reports_the_transport_node() {
+    let r = doctor(None, "http://127.0.0.1:1");
+    let line = r
+        .divergence_line()
+        .unwrap_or_else(|| panic!("no divergence row, got:\n{}", r.stdout));
+    assert!(
+        line.contains(gitlawb_core::DEFAULT_LOCAL_NODE),
+        "divergence row must name the helper's node, got: {line}"
+    );
+}
+
+/// A blank value is not a configured node. The helper must fall back exactly as
+/// it does when the variable is absent, and doctor must say so.
+#[test]
+fn blank_env_is_treated_as_unset() {
+    for blank in ["", "   ", "\t"] {
+        let r = doctor(Some(blank), "http://127.0.0.1:1");
+        let line = r
+            .divergence_line()
+            .unwrap_or_else(|| panic!("no divergence row for {blank:?}, got:\n{}", r.stdout));
+        assert!(
+            line.contains(gitlawb_core::DEFAULT_LOCAL_NODE),
+            "blank env must resolve to the local default, got: {line}"
+        );
+    }
+}
+
+/// An explicit `--node` outranks the environment, so the transport still goes
+/// somewhere gl never probes. This state produced no row at all before.
+#[test]
+fn explicit_node_flag_overriding_env_is_still_reported() {
+    let r = doctor(Some("http://127.0.0.1:2"), "http://127.0.0.1:1");
+    let line = r
+        .divergence_line()
+        .unwrap_or_else(|| panic!("no divergence row, got:\n{}", r.stdout));
+    assert!(
+        line.contains("127.0.0.1:2"),
+        "row must name the env-configured transport node, got: {line}"
+    );
+}
+
+/// No divergence when both resolve the same node: one row, no clause.
+#[test]
+fn agreeing_env_and_flag_produce_no_divergence_row() {
+    let r = doctor(Some("http://127.0.0.1:1"), "http://127.0.0.1:1");
+    assert!(
+        r.divergence_line().is_none(),
+        "gl and the transport agree; expected no divergence row, got:\n{}",
+        r.stdout
+    );
+}
+
+/// The bug this check exists to prevent, one level down: something answering 200
+/// on the transport port is not proof a gitlawb node is there. Reporting it as a
+/// healthy transport is the same false green doctor already shipped once.
+#[test]
+fn a_non_gitlawb_200_is_not_a_healthy_transport() {
+    let Ok(listener) = TcpListener::bind("127.0.0.1:0") else {
+        return;
+    };
+    let port = listener.local_addr().unwrap().port();
+    std::thread::spawn(move || {
+        for stream in listener.incoming().take(1) {
+            let Ok(mut s) = stream else { continue };
+            // Read the request first. Writing the response without draining the
+            // request leaves the client seeing a reset rather than a 200, which
+            // silently turns this test into a probe of the unreachable path.
+            let mut buf = [0u8; 1024];
+            let _ = s.read(&mut buf);
+            let body = b"<html>not a node</html>";
+            let _ = write!(
+                s,
+                "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n",
+                body.len()
+            );
+            let _ = s.write_all(body);
+            let _ = s.flush();
+        }
+    });
+    let r = doctor(
+        Some(&format!("http://127.0.0.1:{port}")),
+        "http://127.0.0.1:1",
+    );
+    let line = r
+        .divergence_line()
+        .unwrap_or_else(|| panic!("no divergence row, got:\n{}", r.stdout));
+    // Positive assertion: only the identity check can produce this text, so the
+    // test cannot pass by the probe quietly failing instead.
+    assert!(
+        line.contains("not a gitlawb node"),
+        "a non-gitlawb 200 must be called out, not reported reachable, got: {line}"
+    );
+}
+
+/// Control characters from an attacker-influenceable node URL must not reach the
+/// terminal raw, including inside the paste-ready remedy line.
+///
+/// Scoped to the rows this check owns. The unchanged `node` row at doctor.rs:180,
+/// :199 and :206 still interpolates the raw URL and is a separate pre-existing
+/// leak, so asserting over the whole of stdout would fail for a reason this
+/// change did not introduce and cannot fix without touching adjacent code.
+#[test]
+fn control_characters_in_the_node_url_are_stripped() {
+    let r = doctor(None, "http://127.0.0.1:1/\u{1b}[31m\u{7}\u{202e}");
+    let owned: String = r
+        .stdout
+        .lines()
+        .filter(|l| l.contains(DIVERGENCE) || l.trim_start().starts_with("GITLAWB_NODE:"))
+        .collect::<Vec<_>>()
+        .join("\n");
+    assert!(
+        owned.contains(DIVERGENCE),
+        "expected the divergence row and its remedy, got:\n{}",
+        r.stdout
+    );
+    assert!(
+        !owned.contains('\u{1b}') && !owned.contains('\u{7}') && !owned.contains('\u{202e}'),
+        "raw control characters reached stdout:\n{owned:?}"
+    );
+}
+
+/// #357's tiering: an unset variable is advisory (gl still works), a variable the
+/// user set to something unusable is a real failure. Nothing pinned this, and the
+/// difference decides whether a stock install exits non-zero once #391 lands.
+#[test]
+fn unset_env_warns_while_a_broken_configured_node_fails() {
+    let stock = doctor(None, "http://127.0.0.1:1");
+    let line = stock.divergence_line().unwrap();
+    assert!(
+        line.trim_start().starts_with('\u{26a0}'),
+        "an unset variable must stay advisory on a stock install, got: {line}"
+    );
+
+    let misconfigured = doctor(Some("http://127.0.0.1:2"), "http://127.0.0.1:1");
+    let line = misconfigured.divergence_line().unwrap();
+    assert!(
+        line.trim_start().starts_with('\u{2717}'),
+        "a configured but unusable node must fail, got: {line}"
+    );
+}
+
+/// #357's constraint: doctor must not start failing a stock install just because
+/// it now says more. Nothing pinned this before.
+#[test]
+fn doctor_still_exits_zero() {
+    let r = doctor(None, "http://127.0.0.1:1");
+    assert_eq!(
+        r.code,
+        Some(0),
+        "doctor must keep exiting 0, got:\n{}",
+        r.stdout
+    );
+}