From 97766fc9f04c52398415ee1929927947b0451c00 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 22 Sep 2026 23:47:53 -0400 Subject: [PATCH 1/7] Add mapbox doctor: a read-only snapshot of the environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auth whoami already answers which token the next command will use; this answers the rest of what commonly goes wrong before a real command finds out the hard way: a proxy variable silently not doing what someone thinks, or the update-check/telemetry switches resolving to something other than what was intended. Reuses auth::resolve_source for the token report rather than re-deriving the precedence — that function is the one place the order is decided, and auth::whoami's own docs explain why a second opinion here would be worse than none. token_expires_at and token_usage move from private to pub(crate) for this; token_account and load_credentials were already public. --verify additionally checks that api.mapbox.com is reachable, through the same http::client() (and therefore the same proxy handling) every other request in this crate uses. Nothing is sent otherwise — the same precedent auth whoami --verify already sets: a diagnostic command should not itself be the request that reveals the problem it exists to describe. MAPBOX_INTERNAL_DOCTOR_URL overrides the checked host, for the same reason update_check.rs's MAPBOX_INTERNAL_UPDATE_URL exists: without a seam, --verify could only ever be tested against the real network. tests/doctor.rs drives both the reachable and unreachable cases through a loopback server, and confirms nothing is sent at all when --verify is absent. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 7 ++ docs/commands.md | 76 ++++++++++++++ src/auth.rs | 6 +- src/doctor.rs | 260 +++++++++++++++++++++++++++++++++++++++++++++++ src/main.rs | 15 +++ src/schema.rs | 4 + tests/doctor.rs | 225 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 590 insertions(+), 3 deletions(-) create mode 100644 src/doctor.rs create mode 100644 tests/doctor.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e52b20..dcbb61c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,13 @@ that may never merge. They are not releases and are not listed here. ### Added +- `mapbox doctor` — a read-only snapshot of what the next command would + see: which token wins and its state, which proxy variables are in effect, + and where the update-check and telemetry switches currently stand. + `--verify` additionally checks that `api.mapbox.com` is reachable, the + only part of this that makes a request — the same precedent + `auth whoami --verify` sets. + - `mapbox config` — `get`/`set` for settings that persist across shells and sessions, written to `~/.mapbox/config.json` (or `$MAPBOX_CONFIG_DIR`) rather than an environment variable that only lasts for the session it was diff --git a/docs/commands.md b/docs/commands.md index 972f0c5..f36b25c 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -64,6 +64,8 @@ nests, and is typed `mapbox styles draft get`. [config.set](#mapbox-config-set) · [config.list](#mapbox-config-list) · [config.unset](#mapbox-config-unset) +**[Doctor](#doctor)** — [doctor](#mapbox-doctor) + **[Usage](#usage)** — [usage](#mapbox-usage) **[Accounts](#accounts)** — @@ -3280,6 +3282,80 @@ update-check cleared, now on (default). --- +## Doctor + +### `mapbox doctor` + +A read-only snapshot of what the next command would see: which token wins +and its state, which proxy variables are in effect, and where the +update-check and telemetry switches currently stand. `auth whoami` answers +which token the next command will use; this answers the rest of what +commonly goes wrong before a real command finds out the hard way — a proxy +variable that silently isn't doing what someone thinks, or a switch +resolving to something other than what was intended. + +Nothing here is sent unless `--verify` asks for the one check that needs a +request — the same precedent `auth whoami --verify` sets. + +#### Parameters + +| Parameter | Effect | +| --- | --- | +| `--verify` | Also check that `api.mapbox.com` is reachable, through the same client and proxy handling every other request uses. | + +#### Examples + +```sh +mapbox doctor + +mapbox doctor --verify +``` + +#### Outputs + + + + +
textjson
+ +``` +mapbox 0.3.0 (production) +Token: available, from login (sk) +Proxy: none set +Update check: on +Telemetry: on +``` + + + +```json +{ + "build": { "version": "0.3.0", "channel": "production" }, + "proxy": { "active": [] }, + "switches": { + "telemetry_allowed": true, + "update_check_env_opt_out": false, + "update_check_persisted": true + }, + "token": { + "available": true, + "source": "login", + "account": "user", + "usage": "sk", + "expires_at": 1788276540 + } +} +``` + +
+ +With `--verify`, a `connectivity` object joins the JSON and a `Reachable:` +line joins the text — `{ "reachable": true, "status": 200 }`, or `{ +"reachable": false }` (plus an `error` field under `--debug`) when the +request itself failed rather than answered. + +--- + ## Usage ### `mapbox usage` diff --git a/src/auth.rs b/src/auth.rs index bd81c80..ea003e3 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -520,7 +520,7 @@ fn save_credentials(creds: &Credentials, profile: Option<&str>) -> Result<()> { .with_context(|| format!("Failed to write credentials to {}", path.display())) } -fn token_expires_at(token: &str) -> Option { +pub(crate) fn token_expires_at(token: &str) -> Option { let payload_b64 = token.split('.').nth(1)?; let decoded = URL_SAFE_NO_PAD.decode(payload_b64).ok()?; let json: serde_json::Value = serde_json::from_slice(&decoded).ok()?; @@ -932,7 +932,7 @@ impl TokenSource { /// The name a program reads. The prose form is built in /// [`Identity::source_prose`], where it can name the variable or profile /// the bare word leaves out. - fn as_str(self) -> &'static str { + pub(crate) fn as_str(self) -> &'static str { match self { TokenSource::Flag => "flag", TokenSource::Environment => "environment", @@ -973,7 +973,7 @@ pub(crate) fn resolve_source<'a>( /// Read off the prefix rather than the payload because that is where the API /// itself reports it — and because the prefix is the one part of a token that /// is safe to print. -fn token_usage(token: &str) -> Option<&str> { +pub(crate) fn token_usage(token: &str) -> Option<&str> { let (usage, rest) = token.split_once('.')?; (!rest.is_empty() && matches!(usage, "pk" | "sk" | "tk")).then_some(usage) } diff --git a/src/doctor.rs b/src/doctor.rs new file mode 100644 index 0000000..79f198e --- /dev/null +++ b/src/doctor.rs @@ -0,0 +1,260 @@ +//! `mapbox doctor` — a read-only snapshot of the environment the next +//! command would run in. +//! +//! `auth whoami` already answers which token the next command will use; this +//! answers the rest of what commonly goes wrong before a real command finds +//! out the hard way: a proxy variable that silently isn't doing what someone +//! thinks, or the update-check/telemetry switches resolving to something +//! other than what was intended. +//! +//! Nothing here is sent unless `--verify` asks for the one check that needs +//! a request — the same precedent `auth whoami --verify` already sets: a +//! diagnostic command should not itself be the request that reveals the +//! problem it exists to describe. + +use std::time::Duration; + +use anyhow::Result; +use clap::{Arg, ArgAction, ArgMatches, Command}; +use serde_json::json; + +use crate::auth; +use crate::config; +use crate::http; +use crate::output::{self, Mode}; +use crate::telemetry; +use crate::update_check; + +pub const COMMAND: &str = "doctor"; + +/// The one host worth checking reachability against: what every bundled +/// spec resolves to when it names no `servers` entry of its own — see +/// `spec::parse_spec`'s own fallback. Not a `use` of that constant, because +/// this is diagnostic and that one is a build-time default; two names for +/// the same string would be one more thing to keep in sync for a value that +/// is already effectively pinned by nearly every operation's own tests. +const API_HOST: &str = "https://api.mapbox.com"; + +/// Overrides [`API_HOST`], for the same reason `update_check.rs`'s +/// `MAPBOX_INTERNAL_UPDATE_URL` exists: this crate makes no other outbound +/// request nothing here controls, so without a seam `--verify` could only +/// ever be tested against the real network — undocumented, and read only by +/// `tests/doctor.rs`. +const HOST_OVERRIDE_ENV: &str = "MAPBOX_INTERNAL_DOCTOR_URL"; + +fn api_host() -> String { + std::env::var(HOST_OVERRIDE_ENV) + .ok() + .filter(|value| !value.trim().is_empty()) + .unwrap_or_else(|| API_HOST.to_string()) +} + +/// A budget of its own, not [`http::client`]'s ordinary one: this is a +/// diagnostic a person is waiting on at a terminal, not a request whose +/// payload a command line bounds, so it fails fast rather than waiting out +/// a stalled connection for as long as a real command would. +const VERIFY_TIMEOUT: Duration = Duration::from_secs(5); + +/// The proxy variables `http::build` actually reads — see that module's own +/// docs for why `.no_proxy()` is never called. Named here, once, so a +/// variable added there and forgotten here is at least this file's problem +/// to notice, not only `tests/proxy.rs`'s. +const PROXY_VARS: &[&str] = &["HTTPS_PROXY", "HTTP_PROXY", "ALL_PROXY", "NO_PROXY"]; + +pub fn command() -> Command { + Command::new(COMMAND) + .about("Show the environment the next command would run in") + .long_about( + "A read-only snapshot of what the next command would see: which token wins \ + and its state, which proxy variables are in effect, and where the \ + update-check and telemetry switches currently stand. `--verify` additionally \ + checks that api.mapbox.com is reachable — the one part of this that makes a \ + request.", + ) + .arg( + Arg::new("verify") + .long("verify") + .action(ArgAction::SetTrue) + .help("Also check that api.mapbox.com is reachable"), + ) +} + +/// The token report, reusing [`auth::resolve_source`] rather than +/// re-deriving the precedence — that function is the one place the order is +/// decided, and `auth::whoami`'s own docs explain why a second opinion here +/// would be worse than none. +fn token_report(matches: &ArgMatches, use_login: bool, profile: Option<&str>) -> serde_json::Value { + let flag = auth::typed_token(matches); + let environment = flag + .is_none() + .then(|| matches.get_one::("token").cloned()) + .flatten(); + let stored = auth::load_credentials(profile); + + match auth::resolve_source( + flag.as_deref(), + environment.as_deref(), + stored.as_ref().map(|c| c.access_token.as_str()), + use_login, + ) { + Some((source, token)) => json!({ + "available": true, + "source": source.as_str(), + "account": auth::token_account(token), + "usage": auth::token_usage(token), + "expires_at": auth::token_expires_at(token), + }), + None => json!({ "available": false }), + } +} + +fn proxy_report() -> serde_json::Value { + let active: Vec<&str> = PROXY_VARS + .iter() + .copied() + .filter(|name| std::env::var_os(name).is_some_and(|v| !v.is_empty())) + .collect(); + json!({ "active": active }) +} + +/// The two switches, and how each is currently set — not the full four-way +/// gate `update_check::enabled` applies, which also asks about the build +/// channel and whether stderr is a terminal. Those two are facts about this +/// run, not something a person adjusts, and belong in `build`/the shell +/// rather than here. +fn switches_report() -> serde_json::Value { + let env_opted_out = + std::env::var(update_check::NO_UPDATE_CHECK_ENV).is_ok_and(|v| !v.trim().is_empty()); + json!({ + "telemetry_allowed": telemetry::telemetry_allowed(), + "update_check_env_opt_out": env_opted_out, + "update_check_persisted": config::update_check_enabled(), + }) +} + +fn build_report() -> serde_json::Value { + json!({ + "version": env!("CARGO_PKG_VERSION"), + "channel": option_env!("MAPBOX_CLI_BUILD_ENV"), + }) +} + +/// Whether `API_HOST` answers at all, through the same client and proxy +/// handling every other request in this crate uses — so this reports what a +/// real command would actually experience, not a bare TCP probe that a +/// proxy-unaware check would get wrong in either direction. Any HTTP +/// response counts as reachable, including an error one: this is asking +/// whether the network path works, not whether the endpoint likes an +/// unauthenticated request to its root. +fn connectivity_report(debug: bool, host: &str) -> serde_json::Value { + let outcome = http::client().and_then(|client| { + client + .get(host) + .timeout(VERIFY_TIMEOUT) + .send() + .map_err(anyhow::Error::from) + }); + + match outcome { + Ok(response) => json!({ "reachable": true, "status": response.status().as_u16() }), + Err(e) => json!({ + "reachable": false, + "error": if debug { Some(e.to_string()) } else { None }, + }), + } +} + +/// `matches` is the top-level parse, the same one `whoami` reads `--token` +/// from — `verify` is not global, so it comes from `doctor_matches`, the +/// subcommand's own. +pub fn run( + matches: &ArgMatches, + use_login: bool, + debug: bool, + profile: Option<&str>, + mode: Mode, + doctor_matches: &ArgMatches, +) -> Result<()> { + let verify = doctor_matches.get_flag("verify"); + let host = api_host(); + + let token = token_report(matches, use_login, profile); + let proxy = proxy_report(); + let switches = switches_report(); + let build = build_report(); + let connectivity = verify.then(|| connectivity_report(debug, &host)); + + let mut lines = vec![format!( + "mapbox {} ({})", + build["version"].as_str().unwrap_or("unknown"), + build["channel"].as_str().unwrap_or("dev") + )]; + + lines.push(if token["available"] == true { + format!( + "Token: available, from {} ({})", + token["source"].as_str().unwrap_or("unknown"), + token["usage"].as_str().unwrap_or("unrecognized prefix"), + ) + } else { + "Token: none available — run `mapbox auth login` or set MAPBOX_ACCESS_TOKEN" + .to_string() + }); + + lines.push(match proxy["active"].as_array() { + Some(active) if !active.is_empty() => format!( + "Proxy: {}", + active + .iter() + .filter_map(|v| v.as_str()) + .collect::>() + .join(", ") + ), + _ => "Proxy: none set".to_string(), + }); + + let update_check_on = + switches["update_check_persisted"] == true && switches["update_check_env_opt_out"] == false; + lines.push(format!( + "Update check: {}{}", + if update_check_on { "on" } else { "off" }, + if switches["update_check_env_opt_out"] == true { + format!(" ({} is set)", update_check::NO_UPDATE_CHECK_ENV) + } else if switches["update_check_persisted"] == false { + " (mapbox config set update-check off)".to_string() + } else { + String::new() + } + )); + lines.push(format!( + "Telemetry: {}", + if switches["telemetry_allowed"] == true { + "on" + } else { + "off" + } + )); + + if let Some(connectivity) = &connectivity { + lines.push(if connectivity["reachable"] == true { + format!( + "Reachable: yes ({host} answered {})", + connectivity["status"] + ) + } else { + format!("Reachable: no ({host})") + }); + } + + let mut json = json!({ + "build": build, + "token": token, + "proxy": proxy, + "switches": switches, + }); + if let Some(connectivity) = connectivity { + json["connectivity"] = connectivity; + } + + output::emit(mode, &lines.join("\n"), json) +} diff --git a/src/main.rs b/src/main.rs index a3a7961..743b0ed 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,6 +20,7 @@ mod completion; mod config; mod confirm; mod deprecation; +mod doctor; mod executor; mod generate_skills; mod http; @@ -596,6 +597,13 @@ fn build_app(specs: &[ServiceSpec]) -> Command { // machine, never the network. app = app.subcommand(config::command()); + // Reads what the other hand-written commands above also read — the + // token store, the proxy environment, the config and telemetry + // switches — so it belongs beside them rather than the API surface + // below. `--verify` is its one exception, the same opt-in + // `auth whoami --verify` already sets a precedent for. + app = app.subcommand(doctor::command()); + app = app.subcommand(account_usage::command()); app.subcommand(tilesets_cli::command()) @@ -1056,6 +1064,13 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - Some(("unset", unset_matches)) => config::unset(unset_matches, mode)?, _ => unreachable!("`config` sets subcommand_required(true)"), }, + // Also ahead of the generic service arm: read-only except for the + // opt-in `--verify` request, and needs no credential load of its own + // — it reports what one would resolve to, not what a fresh one + // would be. + Some((doctor::COMMAND, doctor_matches)) => { + doctor::run(matches, use_login, debug, profile, mode, doctor_matches)? + } // Token resolution mirrors the service arm below, minus path // placeholders, a request body, and `--dry-run` — this GET always refreshes. Some((account_usage::COMMAND, usage_matches)) => { diff --git a/src/schema.rs b/src/schema.rs index d49d655..27c5e57 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -379,6 +379,10 @@ fn commands(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec PathBuf { + let home = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("doctor-{name}")); + let _ = std::fs::remove_dir_all(&home); + std::fs::create_dir_all(home.join("config")).expect("create the scratch config dir"); + home +} + +fn config_dir(home: &Path) -> PathBuf { + home.join("config") +} + +/// The real binary, isolated the same way the other command-level test +/// files isolate it — no token, no output mode, no config directory from +/// the developer's own environment. +fn command(home: &Path) -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + cmd.env_remove("MAPBOX_ACCESS_TOKEN") + .env_remove("MapboxAccessToken") + .env_remove("MAPBOX_USERNAME") + .env_remove("MAPBOX_OUTPUT") + .env_remove("HTTPS_PROXY") + .env_remove("HTTP_PROXY") + .env_remove("ALL_PROXY") + .env_remove("NO_PROXY") + .env_remove("MAPBOX_CLI_NO_TELEMETRY") + .env_remove("MAPBOX_NO_UPDATE_CHECK") + .env("HOME", home) + .env("XDG_CONFIG_HOME", home.join(".config")) + .env("MAPBOX_CONFIG_DIR", config_dir(home)); + cmd +} + +fn stdout(output: &Output) -> serde_json::Value { + let text = String::from_utf8_lossy(&output.stdout); + serde_json::from_str(text.trim()) + .unwrap_or_else(|e| panic!("expected JSON, got {text:?} ({e})")) +} + +/// A loopback stand-in for `api.mapbox.com`, answering every request with +/// the given status and closing. +fn server(status_line: &str) -> (std::thread::JoinHandle<()>, String) { + let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); + let addr = listener.local_addr().expect("the bound address"); + let status_line = status_line.to_string(); + + let handle = std::thread::spawn(move || { + let Ok((mut stream, _)) = listener.accept() else { + return; + }; + let _ = stream.set_read_timeout(Some(Duration::from_secs(10))); + // Enough to know a request landed; the content doesn't matter here. + let mut buf = [0u8; 1024]; + let _ = stream.read(&mut buf); + let response = format!("{status_line}\r\nContent-Length: 0\r\n\r\n"); + let _ = stream.write_all(response.as_bytes()); + }); + + (handle, format!("http://{addr}/")) +} + +/// A port nothing is listening on, so a connection is refused rather than +/// merely slow — the unreachable case, answered quickly instead of by the +/// 5-second budget expiring. +fn closed_port_url() -> String { + let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); + let addr = listener.local_addr().expect("the bound address"); + drop(listener); + format!("http://{addr}/") +} + +#[test] +fn with_no_token_and_nothing_configured() { + let home = scratch("bare"); + + let out = command(&home) + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + + let value = stdout(&out); + assert_eq!(value["token"]["available"], false); + assert_eq!(value["proxy"]["active"], serde_json::json!([])); + assert_eq!(value["switches"]["update_check_persisted"], true); + assert_eq!(value["switches"]["update_check_env_opt_out"], false); + assert_eq!(value["switches"]["telemetry_allowed"], true); + assert!( + value.get("connectivity").is_none(), + "no --verify, no connectivity field" + ); +} + +#[test] +fn a_token_in_the_environment_is_reported() { + let home = scratch("env-token"); + + let out = command(&home) + .env("MAPBOX_ACCESS_TOKEN", "sk.eyJ1IjoiYWxpY2UifQ.sig") + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!(out.status.success()); + + let value = stdout(&out); + assert_eq!(value["token"]["available"], true); + assert_eq!(value["token"]["source"], "environment"); + assert_eq!(value["token"]["account"], "alice"); + assert_eq!(value["token"]["usage"], "sk"); +} + +#[test] +fn the_persisted_and_environment_switches_are_both_reflected() { + let home = scratch("switches"); + + let set = command(&home) + .args(["config", "set", "update-check", "off"]) + .output() + .expect("run mapbox config set"); + assert!(set.status.success()); + + let out = command(&home) + .env("MAPBOX_NO_UPDATE_CHECK", "1") + .env("MAPBOX_CLI_NO_TELEMETRY", "1") + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!(out.status.success()); + + let value = stdout(&out); + assert_eq!(value["switches"]["update_check_persisted"], false); + assert_eq!(value["switches"]["update_check_env_opt_out"], true); + assert_eq!(value["switches"]["telemetry_allowed"], false); +} + +#[test] +fn a_proxy_variable_is_named() { + let home = scratch("proxy"); + + let out = command(&home) + .env("HTTPS_PROXY", "http://localhost:9") + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!(out.status.success()); + + let active = stdout(&out)["proxy"]["active"].clone(); + assert_eq!(active, serde_json::json!(["HTTPS_PROXY"])); +} + +#[test] +fn without_verify_nothing_is_sent() { + let home = scratch("no-verify"); + // A port nothing answers: if this were reached, the process would hang + // for the budget's duration instead of returning immediately. + let url = closed_port_url(); + + let start = std::time::Instant::now(); + let out = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!(out.status.success()); + assert!( + start.elapsed() < Duration::from_secs(2), + "doctor without --verify should not have touched the network at all" + ); + assert!(stdout(&out).get("connectivity").is_none()); +} + +#[test] +fn verify_reports_a_reachable_host() { + let home = scratch("verify-ok"); + let (thread, url) = server("HTTP/1.1 200 OK"); + + let out = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + .args(["-o", "json", "doctor", "--verify"]) + .output() + .expect("run mapbox doctor --verify"); + assert!(out.status.success()); + + let value = stdout(&out); + assert_eq!(value["connectivity"]["reachable"], true); + assert_eq!(value["connectivity"]["status"], 200); + thread.join().expect("the loopback server thread"); +} + +#[test] +fn verify_reports_an_unreachable_host() { + let home = scratch("verify-fail"); + let url = closed_port_url(); + + let out = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + .args(["-o", "json", "doctor", "--verify"]) + .output() + .expect("run mapbox doctor --verify"); + assert!( + out.status.success(), + "a failed check is still a successful run" + ); + + let value = stdout(&out); + assert_eq!(value["connectivity"]["reachable"], false); +} From e1120c1059bb2b97f374b3d502b41752984e30f5 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 09:17:02 -0400 Subject: [PATCH 2/7] Address review on mapbox doctor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugs: - PROXY_VARS now lists both cases (HTTPS_PROXY and https_proxy, etc.) — reqwest reads both, curl's own convention, and the old list only had the uppercase spelling. Documented the remaining gap against the true question rather than closing it silently: this still answers "is a variable set", not "would this request use one" (NO_PROXY can exempt a host, a scheme-specific variable only applies to that scheme), since reqwest has no public API to ask "was a proxy applied" directly. - The reported update-check state now depends on telemetry_allowed too, not just the persisted setting and the dedicated env switch. MAPBOX_CLI_NO_TELEMETRY silences the update check as well (see update_check.rs's module docs), which the first version of this line forgot, so MAPBOX_CLI_NO_TELEMETRY=1 alone printed "Update check: on" for a check that would not run. - --verify now threads http::requested(matches) through instead of a hardcoded 5-second budget, so --timeout/MAPBOX_TIMEOUT affect it like any other request. Falls back to 5 seconds — a diagnostic someone is waiting on, not a request a command line's payload bounds — only when neither was given. - The connectivity JSON's error field is now omitted outside --debug via skip_serializing_if, rather than present and set to null. Structural fix requested in review: lines and JSON used to be built independently from the same underlying values, which is exactly how the telemetry bug above happened — the JSON read three raw switches while the text line's own condition asked only two of them. Report and its four constituent structs are now the one source both the text and the JSON renderer read from, so the same drift can't happen twice. tests/doctor.rs covers all four bugs: a lowercase proxy variable, telemetry alone turning the reported update check off, an explicit --timeout cutting a hung connection short, and the error field's presence keyed to --debug. Co-Authored-By: Claude Sonnet 5 --- docs/commands.md | 9 +- src/doctor.rs | 448 ++++++++++++++++++++++++++++++----------------- tests/doctor.rs | 112 ++++++++++++ 3 files changed, 404 insertions(+), 165 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index f36b25c..204eb23 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3297,11 +3297,18 @@ resolving to something other than what was intended. Nothing here is sent unless `--verify` asks for the one check that needs a request — the same precedent `auth whoami --verify` sets. +`Proxy:` names which of `HTTPS_PROXY`/`HTTP_PROXY`/`ALL_PROXY`/`NO_PROXY` are +set, upper- or lowercase (`reqwest` reads both, curl's convention). It +answers "is a proxy variable set", not "would this request actually use +one" — `NO_PROXY` can exempt `api.mapbox.com` specifically, and a +scheme-specific variable only ever applied to that scheme in the first +place. + #### Parameters | Parameter | Effect | | --- | --- | -| `--verify` | Also check that `api.mapbox.com` is reachable, through the same client and proxy handling every other request uses. | +| `--verify` | Also check that `api.mapbox.com` is reachable, through the same client and proxy handling every other request uses. Honors `--timeout`/`MAPBOX_TIMEOUT` like any other request; defaults to 5 seconds when neither is given, since this is a diagnostic someone is waiting on, not a request whose payload bounds a longer budget. | #### Examples diff --git a/src/doctor.rs b/src/doctor.rs index 79f198e..cce4dbb 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -11,12 +11,18 @@ //! a request — the same precedent `auth whoami --verify` already sets: a //! diagnostic command should not itself be the request that reveals the //! problem it exists to describe. +//! +//! One [`Report`], read by both renderers. Building the text and the JSON +//! from two independently assembled values is exactly how the update-check +//! line first shipped forgetting `telemetry_allowed` — the JSON carried the +//! fact, the text line's own condition just didn't ask it. A single struct +//! removes the seam that let the two drift. use std::time::Duration; use anyhow::Result; use clap::{Arg, ArgAction, ArgMatches, Command}; -use serde_json::json; +use serde::Serialize; use crate::auth; use crate::config; @@ -49,17 +55,39 @@ fn api_host() -> String { .unwrap_or_else(|| API_HOST.to_string()) } -/// A budget of its own, not [`http::client`]'s ordinary one: this is a -/// diagnostic a person is waiting on at a terminal, not a request whose -/// payload a command line bounds, so it fails fast rather than waiting out -/// a stalled connection for as long as a real command would. -const VERIFY_TIMEOUT: Duration = Duration::from_secs(5); +/// The fallback when neither `--timeout` nor `MAPBOX_TIMEOUT` named one — +/// short on purpose: this is a diagnostic a person is waiting on at a +/// terminal, not a request whose payload a command line bounds, so the +/// default fails fast rather than waiting out a stalled connection for as +/// long as a real command would. An explicit budget always wins, the same +/// as it does everywhere else — see [`http::requested`]. +const DEFAULT_VERIFY_TIMEOUT: Duration = Duration::from_secs(5); -/// The proxy variables `http::build` actually reads — see that module's own -/// docs for why `.no_proxy()` is never called. Named here, once, so a -/// variable added there and forgotten here is at least this file's problem -/// to notice, not only `tests/proxy.rs`'s. -const PROXY_VARS: &[&str] = &["HTTPS_PROXY", "HTTP_PROXY", "ALL_PROXY", "NO_PROXY"]; +/// The proxy variables `reqwest` reads, upper- and lowercase both — curl's +/// convention, which is also `libcurl`-derived `getenv` logic's, and reqwest +/// follows it. Listing both spellings explicitly rather than comparing +/// case-insensitively means a reader can see exactly what is checked without +/// having to know that convention exists. Named here, once, so a variable +/// added or removed from that behavior is at least this file's problem to +/// notice, not only `tests/proxy.rs`'s. +/// +/// This still only answers "is a proxy variable set", not "would this +/// request actually use one" — `NO_PROXY` can exempt a specific host, and a +/// scheme-specific variable only applies to that scheme. Both are real gaps +/// against the true question, kept because reqwest has no public API that +/// answers "was a proxy applied to this request" for a caller to read back; +/// closing them means asking upstream for one, not maintaining a longer list +/// here. +const PROXY_VARS: &[&str] = &[ + "HTTPS_PROXY", + "https_proxy", + "HTTP_PROXY", + "http_proxy", + "ALL_PROXY", + "all_proxy", + "NO_PROXY", + "no_proxy", +]; pub fn command() -> Command { Command::new(COMMAND) @@ -79,182 +107,274 @@ pub fn command() -> Command { ) } -/// The token report, reusing [`auth::resolve_source`] rather than -/// re-deriving the precedence — that function is the one place the order is -/// decided, and `auth::whoami`'s own docs explain why a second opinion here -/// would be worse than none. -fn token_report(matches: &ArgMatches, use_login: bool, profile: Option<&str>) -> serde_json::Value { - let flag = auth::typed_token(matches); - let environment = flag - .is_none() - .then(|| matches.get_one::("token").cloned()) - .flatten(); - let stored = auth::load_credentials(profile); - - match auth::resolve_source( - flag.as_deref(), - environment.as_deref(), - stored.as_ref().map(|c| c.access_token.as_str()), - use_login, - ) { - Some((source, token)) => json!({ - "available": true, - "source": source.as_str(), - "account": auth::token_account(token), - "usage": auth::token_usage(token), - "expires_at": auth::token_expires_at(token), - }), - None => json!({ "available": false }), +#[derive(Serialize)] +struct Build { + version: &'static str, + #[serde(skip_serializing_if = "Option::is_none")] + channel: Option<&'static str>, +} + +impl Build { + fn current() -> Self { + Build { + version: env!("CARGO_PKG_VERSION"), + channel: option_env!("MAPBOX_CLI_BUILD_ENV"), + } + } + + fn line(&self) -> String { + format!( + "mapbox {} ({})", + self.version, + self.channel.unwrap_or("dev") + ) } } -fn proxy_report() -> serde_json::Value { - let active: Vec<&str> = PROXY_VARS - .iter() - .copied() - .filter(|name| std::env::var_os(name).is_some_and(|v| !v.is_empty())) - .collect(); - json!({ "active": active }) +#[derive(Serialize)] +struct TokenReport { + available: bool, + #[serde(skip_serializing_if = "Option::is_none")] + source: Option<&'static str>, + #[serde(skip_serializing_if = "Option::is_none")] + account: Option, + #[serde(skip_serializing_if = "Option::is_none")] + usage: Option, + #[serde(skip_serializing_if = "Option::is_none")] + expires_at: Option, } -/// The two switches, and how each is currently set — not the full four-way -/// gate `update_check::enabled` applies, which also asks about the build -/// channel and whether stderr is a terminal. Those two are facts about this -/// run, not something a person adjusts, and belong in `build`/the shell -/// rather than here. -fn switches_report() -> serde_json::Value { - let env_opted_out = - std::env::var(update_check::NO_UPDATE_CHECK_ENV).is_ok_and(|v| !v.trim().is_empty()); - json!({ - "telemetry_allowed": telemetry::telemetry_allowed(), - "update_check_env_opt_out": env_opted_out, - "update_check_persisted": config::update_check_enabled(), - }) +impl TokenReport { + /// Reuses [`auth::resolve_source`] rather than re-deriving the + /// precedence — that function is the one place the order is decided, + /// and `auth::whoami`'s own docs explain why a second opinion here + /// would be worse than none. + fn resolve(matches: &ArgMatches, use_login: bool, profile: Option<&str>) -> Self { + let flag = auth::typed_token(matches); + let environment = flag + .is_none() + .then(|| matches.get_one::("token").cloned()) + .flatten(); + let stored = auth::load_credentials(profile); + + match auth::resolve_source( + flag.as_deref(), + environment.as_deref(), + stored.as_ref().map(|c| c.access_token.as_str()), + use_login, + ) { + Some((source, token)) => TokenReport { + available: true, + source: Some(source.as_str()), + account: auth::token_account(token), + usage: auth::token_usage(token).map(str::to_string), + expires_at: auth::token_expires_at(token), + }, + None => TokenReport { + available: false, + source: None, + account: None, + usage: None, + expires_at: None, + }, + } + } + + fn line(&self) -> String { + if self.available { + format!( + "Token: available, from {} ({})", + self.source.unwrap_or("unknown"), + self.usage.as_deref().unwrap_or("unrecognized prefix"), + ) + } else { + "Token: none available — run `mapbox auth login` or set MAPBOX_ACCESS_TOKEN" + .to_string() + } + } } -fn build_report() -> serde_json::Value { - json!({ - "version": env!("CARGO_PKG_VERSION"), - "channel": option_env!("MAPBOX_CLI_BUILD_ENV"), - }) +#[derive(Serialize)] +struct ProxyReport { + active: Vec<&'static str>, } -/// Whether `API_HOST` answers at all, through the same client and proxy -/// handling every other request in this crate uses — so this reports what a -/// real command would actually experience, not a bare TCP probe that a -/// proxy-unaware check would get wrong in either direction. Any HTTP -/// response counts as reachable, including an error one: this is asking -/// whether the network path works, not whether the endpoint likes an -/// unauthenticated request to its root. -fn connectivity_report(debug: bool, host: &str) -> serde_json::Value { - let outcome = http::client().and_then(|client| { - client - .get(host) - .timeout(VERIFY_TIMEOUT) - .send() - .map_err(anyhow::Error::from) - }); - - match outcome { - Ok(response) => json!({ "reachable": true, "status": response.status().as_u16() }), - Err(e) => json!({ - "reachable": false, - "error": if debug { Some(e.to_string()) } else { None }, - }), +impl ProxyReport { + fn current() -> Self { + let active = PROXY_VARS + .iter() + .copied() + .filter(|name| std::env::var_os(name).is_some_and(|v| !v.is_empty())) + .collect(); + ProxyReport { active } + } + + fn line(&self) -> String { + if self.active.is_empty() { + "Proxy: none set".to_string() + } else { + format!("Proxy: {}", self.active.join(", ")) + } } } -/// `matches` is the top-level parse, the same one `whoami` reads `--token` -/// from — `verify` is not global, so it comes from `doctor_matches`, the -/// subcommand's own. -pub fn run( - matches: &ArgMatches, - use_login: bool, - debug: bool, - profile: Option<&str>, - mode: Mode, - doctor_matches: &ArgMatches, -) -> Result<()> { - let verify = doctor_matches.get_flag("verify"); - let host = api_host(); +#[derive(Serialize)] +struct SwitchesReport { + telemetry_allowed: bool, + update_check_env_opt_out: bool, + update_check_persisted: bool, +} - let token = token_report(matches, use_login, profile); - let proxy = proxy_report(); - let switches = switches_report(); - let build = build_report(); - let connectivity = verify.then(|| connectivity_report(debug, &host)); +impl SwitchesReport { + fn current() -> Self { + let env_opted_out = + std::env::var(update_check::NO_UPDATE_CHECK_ENV).is_ok_and(|v| !v.trim().is_empty()); + SwitchesReport { + telemetry_allowed: telemetry::telemetry_allowed(), + update_check_env_opt_out: env_opted_out, + update_check_persisted: config::update_check_enabled(), + } + } - let mut lines = vec![format!( - "mapbox {} ({})", - build["version"].as_str().unwrap_or("unknown"), - build["channel"].as_str().unwrap_or("dev") - )]; + /// Whether the update check would actually run right now, on every + /// switch a person can adjust — not the full gate `update_check::enabled` + /// applies, which also asks about the build channel and whether stderr + /// is a terminal. Those two are facts about this run, reported in + /// `build`/the shell rather than here. `MAPBOX_CLI_NO_TELEMETRY` silences + /// the update check too (see `update_check.rs`'s module docs), which the + /// first version of this line forgot: it read `update_check_persisted` + /// and `update_check_env_opt_out` alone, so `MAPBOX_CLI_NO_TELEMETRY=1` + /// printed "Update check: on" for a check that would not run. + fn update_check_on(&self) -> bool { + self.update_check_persisted && !self.update_check_env_opt_out && self.telemetry_allowed + } - lines.push(if token["available"] == true { - format!( - "Token: available, from {} ({})", - token["source"].as_str().unwrap_or("unknown"), - token["usage"].as_str().unwrap_or("unrecognized prefix"), - ) - } else { - "Token: none available — run `mapbox auth login` or set MAPBOX_ACCESS_TOKEN" - .to_string() - }); - - lines.push(match proxy["active"].as_array() { - Some(active) if !active.is_empty() => format!( - "Proxy: {}", - active - .iter() - .filter_map(|v| v.as_str()) - .collect::>() - .join(", ") - ), - _ => "Proxy: none set".to_string(), - }); - - let update_check_on = - switches["update_check_persisted"] == true && switches["update_check_env_opt_out"] == false; - lines.push(format!( - "Update check: {}{}", - if update_check_on { "on" } else { "off" }, - if switches["update_check_env_opt_out"] == true { + fn update_check_line(&self) -> String { + let reason = if self.update_check_env_opt_out { format!(" ({} is set)", update_check::NO_UPDATE_CHECK_ENV) - } else if switches["update_check_persisted"] == false { + } else if !self.telemetry_allowed { + " (MAPBOX_CLI_NO_TELEMETRY silences this too)".to_string() + } else if !self.update_check_persisted { " (mapbox config set update-check off)".to_string() } else { String::new() - } - )); - lines.push(format!( - "Telemetry: {}", - if switches["telemetry_allowed"] == true { - "on" - } else { - "off" - } - )); + }; + format!( + "Update check: {}{reason}", + if self.update_check_on() { "on" } else { "off" } + ) + } - if let Some(connectivity) = &connectivity { - lines.push(if connectivity["reachable"] == true { - format!( - "Reachable: yes ({host} answered {})", - connectivity["status"] - ) - } else { - format!("Reachable: no ({host})") + fn telemetry_line(&self) -> String { + format!( + "Telemetry: {}", + if self.telemetry_allowed { "on" } else { "off" } + ) + } +} + +#[derive(Serialize)] +struct ConnectivityReport { + reachable: bool, + #[serde(skip_serializing_if = "Option::is_none")] + status: Option, + #[serde(skip_serializing_if = "Option::is_none")] + error: Option, +} + +impl ConnectivityReport { + /// Whether `host` answers at all, through the same client and proxy + /// handling every other request in this crate uses — so this reports + /// what a real command would actually experience, not a bare TCP probe + /// that a proxy-unaware check would get wrong in either direction. Any + /// HTTP response counts as reachable, including an error one: this is + /// asking whether the network path works, not whether the endpoint + /// likes an unauthenticated request to its root. + /// + /// `error` is left out of the JSON entirely outside `--debug` (via + /// `skip_serializing_if`), not set to `null` — the same shape the rest + /// of this crate uses `Option` for, and what the docs promise. + fn check(debug: bool, host: &str, timeout: Duration) -> Self { + let outcome = http::client().and_then(|client| { + client + .get(host) + .timeout(timeout) + .send() + .map_err(anyhow::Error::from) }); + + match outcome { + Ok(response) => ConnectivityReport { + reachable: true, + status: Some(response.status().as_u16()), + error: None, + }, + Err(e) => ConnectivityReport { + reachable: false, + status: None, + error: debug.then(|| e.to_string()), + }, + } + } + + fn line(&self, host: &str) -> String { + match self.status { + Some(status) => format!("Reachable: yes ({host} answered {status})"), + None => format!("Reachable: no ({host})"), + } } +} - let mut json = json!({ - "build": build, - "token": token, - "proxy": proxy, - "switches": switches, - }); - if let Some(connectivity) = connectivity { - json["connectivity"] = connectivity; +#[derive(Serialize)] +struct Report { + build: Build, + token: TokenReport, + proxy: ProxyReport, + switches: SwitchesReport, + #[serde(skip_serializing_if = "Option::is_none")] + connectivity: Option, +} + +impl Report { + fn text(&self, host: &str) -> String { + let mut lines = vec![ + self.build.line(), + self.token.line(), + self.proxy.line(), + self.switches.update_check_line(), + self.switches.telemetry_line(), + ]; + if let Some(connectivity) = &self.connectivity { + lines.push(connectivity.line(host)); + } + lines.join("\n") } +} + +/// `matches` is the top-level parse, the same one `whoami` reads `--token` +/// and `--timeout` from — `verify` is not global, so it comes from +/// `doctor_matches`, the subcommand's own. +pub fn run( + matches: &ArgMatches, + use_login: bool, + debug: bool, + profile: Option<&str>, + mode: Mode, + doctor_matches: &ArgMatches, +) -> Result<()> { + let verify = doctor_matches.get_flag("verify"); + let host = api_host(); + let timeout = http::requested(matches).unwrap_or(DEFAULT_VERIFY_TIMEOUT); + + let report = Report { + build: Build::current(), + token: TokenReport::resolve(matches, use_login, profile), + proxy: ProxyReport::current(), + switches: SwitchesReport::current(), + connectivity: verify.then(|| ConnectivityReport::check(debug, &host, timeout)), + }; - output::emit(mode, &lines.join("\n"), json) + let text = report.text(&host); + let json = serde_json::to_value(&report)?; + output::emit(mode, &text, json) } diff --git a/tests/doctor.rs b/tests/doctor.rs index 2b5c298..a564673 100644 --- a/tests/doctor.rs +++ b/tests/doctor.rs @@ -34,11 +34,16 @@ fn command(home: &Path) -> Command { .env_remove("MAPBOX_USERNAME") .env_remove("MAPBOX_OUTPUT") .env_remove("HTTPS_PROXY") + .env_remove("https_proxy") .env_remove("HTTP_PROXY") + .env_remove("http_proxy") .env_remove("ALL_PROXY") + .env_remove("all_proxy") .env_remove("NO_PROXY") + .env_remove("no_proxy") .env_remove("MAPBOX_CLI_NO_TELEMETRY") .env_remove("MAPBOX_NO_UPDATE_CHECK") + .env_remove("MAPBOX_TIMEOUT") .env("HOME", home) .env("XDG_CONFIG_HOME", home.join(".config")) .env("MAPBOX_CONFIG_DIR", config_dir(home)); @@ -83,6 +88,25 @@ fn closed_port_url() -> String { format!("http://{addr}/") } +/// Accepts a connection and then never answers it — unlike +/// [`closed_port_url`], where the connection is refused immediately, this is +/// what actually exercises a timeout budget rather than an instant refusal. +/// The thread outlives the test (nothing tells it to stop), which is fine: +/// it holds one socket open until the process exits. +fn hanging_server() -> String { + let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); + let addr = listener.local_addr().expect("the bound address"); + + std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + // Held open, on purpose, for as long as this process runs. + std::mem::forget(stream); + } + }); + + format!("http://{addr}/") +} + #[test] fn with_no_token_and_nothing_configured() { let home = scratch("bare"); @@ -223,3 +247,91 @@ fn verify_reports_an_unreachable_host() { let value = stdout(&out); assert_eq!(value["connectivity"]["reachable"], false); } + +#[test] +fn a_lowercase_proxy_variable_is_named_too() { + let home = scratch("proxy-lowercase"); + + let out = command(&home) + .env("https_proxy", "http://localhost:9") + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!(out.status.success()); + + let active = stdout(&out)["proxy"]["active"].clone(); + assert_eq!(active, serde_json::json!(["https_proxy"])); +} + +/// The bug this pins: the text line used to read only +/// `update_check_persisted`/`update_check_env_opt_out`, so +/// `MAPBOX_CLI_NO_TELEMETRY=1` alone — with the persisted setting still on +/// its default and the dedicated env switch unset — printed +/// "Update check: on" for a check that `update_check::enabled` would not +/// actually run, since that silences it too. +#[test] +fn telemetry_off_alone_is_enough_to_turn_the_reported_update_check_off() { + let home = scratch("telemetry-only"); + + let out = command(&home) + .env("MAPBOX_CLI_NO_TELEMETRY", "1") + .args(["-o", "text", "doctor"]) + .output() + .expect("run mapbox doctor"); + assert!(out.status.success()); + + let text = String::from_utf8_lossy(&out.stdout).to_string(); + assert!( + text.lines().any(|line| line.trim_start() == "Update check: off" + || line.starts_with("Update check: off")), + "{text}" + ); +} + +#[test] +fn verify_honors_an_explicit_timeout() { + let home = scratch("verify-timeout"); + let url = hanging_server(); + + let start = std::time::Instant::now(); + let out = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + .args(["-o", "json", "--timeout", "1", "doctor", "--verify"]) + .output() + .expect("run mapbox --timeout 1 doctor --verify"); + let elapsed = start.elapsed(); + + assert!(out.status.success()); + assert_eq!(stdout(&out)["connectivity"]["reachable"], false); + assert!( + elapsed < Duration::from_secs(3), + "an explicit --timeout 1 should have cut this short, took {elapsed:?}" + ); +} + +#[test] +fn the_error_field_is_absent_without_debug_and_present_with_it() { + let home = scratch("verify-error-field"); + let url = closed_port_url(); + + let without_debug = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + .args(["-o", "json", "doctor", "--verify"]) + .output() + .expect("run mapbox doctor --verify"); + assert!(without_debug.status.success()); + let value = stdout(&without_debug); + assert!( + value["connectivity"].get("error").is_none(), + "error should be absent, not null, outside --debug: {value}" + ); + + let with_debug = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + .args(["-o", "json", "--debug", "doctor", "--verify"]) + .output() + .expect("run mapbox --debug doctor --verify"); + assert!(with_debug.status.success()); + let value = stdout(&with_debug); + assert!(value["connectivity"]["error"].is_string(), "{value}"); +} From 1a03d7d71d650c5a0d219a134bfcfa857b9cf80a Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 09:17:36 -0400 Subject: [PATCH 3/7] Move the doctor CHANGELOG entry into Unreleased, matching the rest --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a049d9f..16f4279 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,10 +23,6 @@ that may never merge. They are not releases and are not listed here. the same `k1=v1&k2=v2` shape as a URL's own query string — for an API parameter this CLI's specs don't declare a flag for. -## 0.3.0 - 2026-09-22 - -### Added - - `mapbox doctor` — a read-only snapshot of what the next command would see: which token wins and its state, which proxy variables are in effect, and where the update-check and telemetry switches currently stand. @@ -34,6 +30,10 @@ that may never merge. They are not releases and are not listed here. only part of this that makes a request — the same precedent `auth whoami --verify` sets. +## 0.3.0 - 2026-09-22 + +### Added + - `mapbox config` — `get`/`set` for settings that persist across shells and sessions, written to `~/.mapbox/config.json` (or `$MAPBOX_CONFIG_DIR`) rather than an environment variable that only lasts for the session it was From f96811efa545511614e9ac6b3457af5fa97a8e54 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 09:20:13 -0400 Subject: [PATCH 4/7] cargo fmt --- tests/doctor.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/doctor.rs b/tests/doctor.rs index a564673..4515795 100644 --- a/tests/doctor.rs +++ b/tests/doctor.rs @@ -282,8 +282,9 @@ fn telemetry_off_alone_is_enough_to_turn_the_reported_update_check_off() { let text = String::from_utf8_lossy(&out.stdout).to_string(); assert!( - text.lines().any(|line| line.trim_start() == "Update check: off" - || line.starts_with("Update check: off")), + text.lines() + .any(|line| line.trim_start() == "Update check: off" + || line.starts_with("Update check: off")), "{text}" ); } From 958c0e681c6f87bf94e69443afdc37990971a301 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 09:25:15 -0400 Subject: [PATCH 5/7] Fix double-reporting a proxy variable on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows environment variables are case-insensitive, so HTTPS_PROXY and https_proxy are the same variable there. Checking both spellings as independent PROXY_VARS entries reported a single value set once under both names on Windows CI — caught by the platform's own test run, not locally on macOS where the two are genuinely distinct. PROXY_VARS is now four (uppercase, lowercase) pairs; either being set reports one entry, always spelled with the uppercase name. This also answers a cleaner question than four-to-eight independent checks did: "is a proxy configured for this concern" rather than "list every spelling that happens to be set", which nobody was asking for anyway. Co-Authored-By: Claude Sonnet 5 --- src/doctor.rs | 42 ++++++++++++++++++++++++------------------ tests/doctor.rs | 9 +++++++-- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/doctor.rs b/src/doctor.rs index cce4dbb..25e67a3 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -63,13 +63,22 @@ fn api_host() -> String { /// as it does everywhere else — see [`http::requested`]. const DEFAULT_VERIFY_TIMEOUT: Duration = Duration::from_secs(5); -/// The proxy variables `reqwest` reads, upper- and lowercase both — curl's -/// convention, which is also `libcurl`-derived `getenv` logic's, and reqwest -/// follows it. Listing both spellings explicitly rather than comparing -/// case-insensitively means a reader can see exactly what is checked without -/// having to know that convention exists. Named here, once, so a variable -/// added or removed from that behavior is at least this file's problem to -/// notice, not only `tests/proxy.rs`'s. +/// The proxy variables `reqwest` reads, paired with the lowercase spelling +/// it also honors — curl's convention, which is also `libcurl`-derived +/// `getenv` logic's. Checked as a pair rather than as four independent +/// names because Windows environment variables are case-insensitive: +/// `HTTPS_PROXY` and `https_proxy` are the same variable there, so checking +/// both spellings as separate entries reported it twice on Windows the +/// first time this shipped, once under each spelling, for a value set only +/// once. Reporting under the uppercase name when either is set avoids that +/// without pretending Unix's two independently-settable variables don't +/// exist — it just means this asks "is *a* proxy configured for this +/// concern", not "list every spelling that happens to be set", which is the +/// question a person actually has. +/// +/// Named here, once, so a variable added or removed from `reqwest`'s +/// behavior is at least this file's problem to notice, not only +/// `tests/proxy.rs`'s. /// /// This still only answers "is a proxy variable set", not "would this /// request actually use one" — `NO_PROXY` can exempt a specific host, and a @@ -78,15 +87,11 @@ const DEFAULT_VERIFY_TIMEOUT: Duration = Duration::from_secs(5); /// answers "was a proxy applied to this request" for a caller to read back; /// closing them means asking upstream for one, not maintaining a longer list /// here. -const PROXY_VARS: &[&str] = &[ - "HTTPS_PROXY", - "https_proxy", - "HTTP_PROXY", - "http_proxy", - "ALL_PROXY", - "all_proxy", - "NO_PROXY", - "no_proxy", +const PROXY_VARS: &[(&str, &str)] = &[ + ("HTTPS_PROXY", "https_proxy"), + ("HTTP_PROXY", "http_proxy"), + ("ALL_PROXY", "all_proxy"), + ("NO_PROXY", "no_proxy"), ]; pub fn command() -> Command { @@ -201,10 +206,11 @@ struct ProxyReport { impl ProxyReport { fn current() -> Self { + let set = |name: &str| std::env::var_os(name).is_some_and(|v| !v.is_empty()); let active = PROXY_VARS .iter() - .copied() - .filter(|name| std::env::var_os(name).is_some_and(|v| !v.is_empty())) + .filter(|(upper, lower)| set(upper) || set(lower)) + .map(|(upper, _)| *upper) .collect(); ProxyReport { active } } diff --git a/tests/doctor.rs b/tests/doctor.rs index 4515795..e1b3c29 100644 --- a/tests/doctor.rs +++ b/tests/doctor.rs @@ -249,7 +249,7 @@ fn verify_reports_an_unreachable_host() { } #[test] -fn a_lowercase_proxy_variable_is_named_too() { +fn a_lowercase_proxy_variable_is_reported_under_its_uppercase_name() { let home = scratch("proxy-lowercase"); let out = command(&home) @@ -259,8 +259,13 @@ fn a_lowercase_proxy_variable_is_named_too() { .expect("run mapbox doctor"); assert!(out.status.success()); + // Reported as `HTTPS_PROXY`, not `https_proxy`: Windows environment + // variables are case-insensitive, so the two spellings are the same + // variable there, and reporting under whichever was literally set would + // print `HTTPS_PROXY` on Windows for this exact test regardless — the + // canonical name is what both platforms agree on. let active = stdout(&out)["proxy"]["active"].clone(); - assert_eq!(active, serde_json::json!(["https_proxy"])); + assert_eq!(active, serde_json::json!(["HTTPS_PROXY"])); } /// The bug this pins: the text line used to read only From 9c77cc597452ce3a0c531c2f10300c8fd07b7a03 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 09:35:36 -0400 Subject: [PATCH 6/7] Note two more proxy-detection gaps: mixed case, macOS system settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified against hyper-util's own source rather than assumed from curl's reputation: get_first_env(&["HTTPS_PROXY", "https_proxy"]) checks exactly those two spellings and nothing else, so a mixed-case Https_Proxy is not a gap in this list — it would not be honored by the client either, on a case-sensitive OS. The real fourth gap, worth naming: on macOS, hyper-util falls back to the system's Dynamic Store proxy settings when no env var is set at all, and this reads only the environment, so a proxy configured solely through Network settings is invisible to `mapbox doctor` even though reqwest would still use it. Co-Authored-By: Claude Sonnet 5 --- docs/commands.md | 12 +++++++----- src/doctor.rs | 18 ++++++++++++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 204eb23..449ae91 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3298,11 +3298,13 @@ Nothing here is sent unless `--verify` asks for the one check that needs a request — the same precedent `auth whoami --verify` sets. `Proxy:` names which of `HTTPS_PROXY`/`HTTP_PROXY`/`ALL_PROXY`/`NO_PROXY` are -set, upper- or lowercase (`reqwest` reads both, curl's convention). It -answers "is a proxy variable set", not "would this request actually use -one" — `NO_PROXY` can exempt `api.mapbox.com` specifically, and a -scheme-specific variable only ever applied to that scheme in the first -place. +set — upper- or lowercase, exactly those two spellings each, which is what +`reqwest` itself reads. It answers "is a proxy variable set", not "would +this request actually use one" — `NO_PROXY` can exempt `api.mapbox.com` +specifically, a scheme-specific variable only ever applied to that scheme +in the first place, and on macOS a proxy configured only through system +Network settings (rather than an environment variable) is invisible here +even though `reqwest` would still use it. #### Parameters diff --git a/src/doctor.rs b/src/doctor.rs index 25e67a3..0350178 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -64,9 +64,15 @@ fn api_host() -> String { const DEFAULT_VERIFY_TIMEOUT: Duration = Duration::from_secs(5); /// The proxy variables `reqwest` reads, paired with the lowercase spelling -/// it also honors — curl's convention, which is also `libcurl`-derived -/// `getenv` logic's. Checked as a pair rather than as four independent -/// names because Windows environment variables are case-insensitive: +/// it also honors — exactly these two spellings and no others, confirmed by +/// reading `hyper-util`'s own `get_first_env(&["HTTPS_PROXY", +/// "https_proxy"])` (`hyper-util/src/client/proxy/matcher.rs`) rather than +/// assumed from curl's reputation for case-insensitivity: a value under +/// `Https_Proxy` or any other mixed case would not be honored by this +/// client either, on a case-sensitive OS, so not checking for it here is +/// reporting the client's real behavior rather than a gap in this list. +/// Checked as a pair rather than as four independent names because Windows +/// environment variables are case-insensitive: /// `HTTPS_PROXY` and `https_proxy` are the same variable there, so checking /// both spellings as separate entries reported it twice on Windows the /// first time this shipped, once under each spelling, for a value set only @@ -82,7 +88,11 @@ const DEFAULT_VERIFY_TIMEOUT: Duration = Duration::from_secs(5); /// /// This still only answers "is a proxy variable set", not "would this /// request actually use one" — `NO_PROXY` can exempt a specific host, and a -/// scheme-specific variable only applies to that scheme. Both are real gaps +/// scheme-specific variable only applies to that scheme. On macOS there is +/// a fourth gap the same shape: `hyper-util` falls back to the system's +/// Dynamic Store proxy settings (Network settings → Proxies) when no env +/// var is set at all, and a proxy configured only that way is invisible +/// here — this reads the environment, not that store. All are real gaps /// against the true question, kept because reqwest has no public API that /// answers "was a proxy applied to this request" for a caller to read back; /// closing them means asking upstream for one, not maintaining a longer list From 963be6310ee6080056e51cc5da7089430cfb461c Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 10:24:08 -0400 Subject: [PATCH 7/7] Trigger CI (base branch retarget didn't fire a check run)