From ce54f514092d2602b62797b6395e1397de82706a Mon Sep 17 00:00:00 2001 From: Mofei Zhu <13761509829@163.com> Date: Thu, 24 Sep 2026 15:19:28 +0300 Subject: [PATCH 1/3] Add a persisted telemetry setting to mapbox config mapbox config set telemetry off stops the run's telemetry event in every shell, beside MAPBOX_CLI_NO_TELEMETRY for one. The setting is read again at exit, so the run that turns it off does not report itself. --- CHANGELOG.md | 4 ++++ README.md | 2 +- docs/commands.md | 12 +++++++++--- src/config.rs | 14 +++++++++++++- src/telemetry_event.rs | 10 ++++++++-- tests/config.rs | 6 +++--- tests/telemetry_events.rs | 24 +++++++++++++++++++++++- 7 files changed, 61 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7428211..03813ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -260,6 +260,10 @@ that may never merge. They are not releases and are not listed here. code; it does find a `~/.mapbox/history` directory it didn't before, and `mapbox config list` now reports a second key, `history`. +- `mapbox config set telemetry off` turns the telemetry event off for good, + in every shell, the way `MAPBOX_CLI_NO_TELEMETRY=1` does for one. The run + that turns it off records nothing either. `mapbox config list` now also + reports `telemetry`. - Each run sends one `cli.command` telemetry event to Mapbox, from a background process the command doesn't wait for, with your own token (`--token`, `MAPBOX_ACCESS_TOKEN` or your login) or, when you have none, diff --git a/README.md b/README.md index 29734ae..6973511 100644 --- a/README.md +++ b/README.md @@ -305,7 +305,7 @@ or your login) when you have one, and with a token built into the CLI otherwise; Mapbox Events keeps the token an event was sent with, and the account it belongs to, alongside the event. A build from source has no built-in token, so with no token of your own the event is dropped. -`MAPBOX_CLI_NO_TELEMETRY=1` turns it off. +`MAPBOX_CLI_NO_TELEMETRY=1` or `mapbox config set telemetry off` turns it off. ### Diagnostics and settings diff --git a/docs/commands.md b/docs/commands.md index bd1d2e8..62ed3ca 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4130,6 +4130,7 @@ was set in, and stays in every future shell instead. | `update-check` | `on` | The update notice; mirrors `MAPBOX_NO_UPDATE_CHECK` (see [Update notices](../README.md#update-notices)) | | `history` | `on` | [Command history](../README.md#command-history), read by `mapbox history`; `MAPBOX_HISTORY=0` or `=1` overrides it for a session | | `log` | `off` | [Diagnostic logs](../README.md#diagnostic-logs), shown by `mapbox history show`; `MAPBOX_LOG=1` or `=0` overrides it for a session. Needs `history` on: `config set log on` with history off fails with `history_required` | +| `telemetry` | `on` | The run's telemetry event; mirrors `MAPBOX_CLI_NO_TELEMETRY` (see [Privacy](../README.md#privacy)) | ### `mapbox config get` @@ -4141,7 +4142,7 @@ than failing, the same forgiving read the update-check cache itself uses. | Parameter | Effect | | --- | --- | -| `` | Which setting to read: `update-check`, `history` or `log`. | +| `` | Which setting to read: `update-check`, `history`, `log` or `telemetry`. | #### Examples @@ -4180,7 +4181,7 @@ without an environment variable. | Parameter | Effect | | --- | --- | -| `` | Which setting to change: `update-check`, `history` or `log`. | +| `` | Which setting to change: `update-check`, `history`, `log` or `telemetry`. | | `` | `on` or `off`. | #### Examples @@ -4239,6 +4240,7 @@ mapbox config list update-check on history on log off +telemetry on ``` @@ -4256,6 +4258,10 @@ log off { "key": "log", "value": false + }, + { + "key": "telemetry", + "value": true } ] ``` @@ -4274,7 +4280,7 @@ default, a key explicitly set to the old default value does not. | Parameter | Effect | | --- | --- | -| `` | Which setting to clear: `update-check`, `history` or `log`. | +| `` | Which setting to clear: `update-check`, `history`, `log` or `telemetry`. | #### Examples diff --git a/src/config.rs b/src/config.rs index 165cd3a..0cb0c12 100644 --- a/src/config.rs +++ b/src/config.rs @@ -33,7 +33,8 @@ const CONFIG_FILE: &str = "config.json"; const UPDATE_CHECK_KEY: &str = "update-check"; const HISTORY_KEY: &str = "history"; const LOG_KEY: &str = "log"; -const KEYS: &[&str] = &[UPDATE_CHECK_KEY, HISTORY_KEY, LOG_KEY]; +const TELEMETRY_KEY: &str = "telemetry"; +const KEYS: &[&str] = &[UPDATE_CHECK_KEY, HISTORY_KEY, LOG_KEY, TELEMETRY_KEY]; const ON: &str = "on"; const OFF: &str = "off"; @@ -50,6 +51,8 @@ struct Config { history: Option, #[serde(default, skip_serializing_if = "Option::is_none")] log: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + telemetry: Option, } fn config_path() -> Option { @@ -102,6 +105,12 @@ pub fn log_enabled() -> bool { read_config().log.unwrap_or(false) } +/// Whether the run's telemetry event may be recorded, per the persisted +/// setting. [`crate::telemetry_event`] checks it alongside `MAPBOX_CLI_NO_TELEMETRY`. +pub fn telemetry_enabled() -> bool { + read_config().telemetry.unwrap_or(true) +} + fn on_off(enabled: bool) -> &'static str { if enabled { ON @@ -118,6 +127,7 @@ fn resolve(config: &Config, key: &str) -> bool { UPDATE_CHECK_KEY => update_check_setting(config), HISTORY_KEY => config.history.unwrap_or(true), LOG_KEY => config.log.unwrap_or(false), + TELEMETRY_KEY => config.telemetry.unwrap_or(true), _ => unreachable!("clap's value_parser restricts `key` to {KEYS:?}"), } } @@ -132,6 +142,7 @@ fn clear(config: &mut Config, key: &str) { UPDATE_CHECK_KEY => config.update_check = None, HISTORY_KEY => config.history = None, LOG_KEY => config.log = None, + TELEMETRY_KEY => config.telemetry = None, _ => unreachable!("clap's value_parser restricts `key` to {KEYS:?}"), } } @@ -214,6 +225,7 @@ pub fn set(matches: &ArgMatches, mode: Mode) -> Result<()> { UPDATE_CHECK_KEY => config.update_check = Some(enabled), HISTORY_KEY => config.history = Some(enabled), LOG_KEY => config.log = Some(enabled), + TELEMETRY_KEY => config.telemetry = Some(enabled), _ => unreachable!("clap's value_parser restricts `key` to {KEYS:?}"), } write_config(&config)?; diff --git a/src/telemetry_event.rs b/src/telemetry_event.rs index a988616..d131a70 100644 --- a/src/telemetry_event.rs +++ b/src/telemetry_event.rs @@ -18,7 +18,8 @@ //! //! Best-effort throughout: nothing here can change a command's output, its //! exit code, or how long it takes to return. With telemetry off -//! (`MAPBOX_CLI_NO_TELEMETRY`), nothing is built or written. +//! (`MAPBOX_CLI_NO_TELEMETRY`, or `mapbox config set telemetry off`), +//! nothing is built or written. use std::io::IsTerminal; use std::path::Path; @@ -336,7 +337,12 @@ fn with_workflow(f: impl FnOnce(&mut Option)) { /// second after exit, and a sender still running from that image would keep /// the file locked and the delete would fail. pub(crate) fn deliver(record: &Record) { - if !telemetry::telemetry_allowed() || std::env::var_os("SUDO_USER").is_some() { + // The config setting is read at the end of the run: the run that turns + // telemetry off is one that should not report itself. + if !telemetry::telemetry_allowed() + || !crate::config::telemetry_enabled() + || std::env::var_os("SUDO_USER").is_some() + { return; } if cfg!(windows) diff --git a/tests/config.rs b/tests/config.rs index 3357850..e07a5b0 100644 --- a/tests/config.rs +++ b/tests/config.rs @@ -138,7 +138,7 @@ fn list_reports_every_setting_including_an_unset_one() { assert!(empty.status.success()); assert_eq!( stdout(&empty), - r#"[{"key":"update-check","value":true},{"key":"history","value":true},{"key":"log","value":false}]"# + r#"[{"key":"update-check","value":true},{"key":"history","value":true},{"key":"log","value":false},{"key":"telemetry","value":true}]"# ); let set = command(&home) @@ -154,7 +154,7 @@ fn list_reports_every_setting_including_an_unset_one() { assert!(after.status.success()); assert_eq!( stdout(&after), - r#"[{"key":"update-check","value":false},{"key":"history","value":true},{"key":"log","value":false}]"# + r#"[{"key":"update-check","value":false},{"key":"history","value":true},{"key":"log","value":false},{"key":"telemetry","value":true}]"# ); let text = command(&home) @@ -164,7 +164,7 @@ fn list_reports_every_setting_including_an_unset_one() { assert!(text.status.success()); assert_eq!( stdout(&text), - "update-check off\nhistory on\nlog off" + "update-check off\nhistory on\nlog off\ntelemetry on" ); } diff --git a/tests/telemetry_events.rs b/tests/telemetry_events.rs index d270f0b..08f5509 100644 --- a/tests/telemetry_events.rs +++ b/tests/telemetry_events.rs @@ -206,7 +206,7 @@ fn help_version_and_usage_errors_record_their_invocation() { } #[test] -fn the_opt_out_records_nothing() { +fn either_opt_out_records_nothing() { let home = scratch("opt-out-env"); let out = command(&home) .env("MAPBOX_CLI_NO_TELEMETRY", "1") @@ -218,6 +218,28 @@ fn the_opt_out_records_nothing() { !config_dir(&home).join(".telemetry").exists(), "MAPBOX_CLI_NO_TELEMETRY=1 still wrote telemetry" ); + + // With a token and somewhere to send, so that nothing arriving is the + // setting's doing. + let home = scratch("opt-out-config"); + let (received, url) = events_server(true); + let with_a_token = |args: &[&str]| { + command(&home) + .env("MAPBOX_INTERNAL_TELEMETRY_URL", &url) + .env("MAPBOX_CLI_TOKEN", "pk.cli") + .args(args) + .output() + .expect("run mapbox") + }; + assert!(with_a_token(&["config", "set", "telemetry", "off"]) + .status + .success()); + // That run sends nothing either: the setting it wrote is read at exit. + let _ = with_a_token(&["config", "list"]); + assert!( + received.recv_timeout(Duration::from_secs(3)).is_err(), + "`telemetry off` still sent an event" + ); } #[test] From 2c9e93e6b2c2f7c5f3e5307a994dfda3e2acbc9b Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 9 Oct 2026 18:44:31 +0300 Subject: [PATCH 2/3] Make the telemetry setting cover what MAPBOX_CLI_NO_TELEMETRY covers The two opt-outs now mean the same thing, one for a shell and one for good: telemetry::telemetry_allowed reads both, so config set telemetry off also strips the User-Agent markers and silences the update notice, not only the event. mapbox doctor reports the setting as telemetry_persisted, with the reason in its text lines, and the Privacy section names it under How to Opt Out. --- CHANGELOG.md | 10 +++-- README.md | 15 ++++++-- docs/commands.md | 3 +- src/doctor.rs | 24 ++++++++++-- src/telemetry.rs | 14 ++++++- src/telemetry_event.rs | 13 +++---- src/telemetry_sink.rs | 5 ++- tests/doctor.rs | 86 ++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 145 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03813ad..2ec63bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -260,10 +260,12 @@ that may never merge. They are not releases and are not listed here. code; it does find a `~/.mapbox/history` directory it didn't before, and `mapbox config list` now reports a second key, `history`. -- `mapbox config set telemetry off` turns the telemetry event off for good, - in every shell, the way `MAPBOX_CLI_NO_TELEMETRY=1` does for one. The run - that turns it off records nothing either. `mapbox config list` now also - reports `telemetry`. +- `mapbox config set telemetry off` turns telemetry off for good, in every + shell, the way `MAPBOX_CLI_NO_TELEMETRY=1` does for one: the run's event, + the `User-Agent` markers and the update notice. The run that turns it off + records nothing either. `mapbox config list` now also reports `telemetry`, + and `mapbox doctor` reports it as `telemetry_persisted`. + - Each run sends one `cli.command` telemetry event to Mapbox, from a background process the command doesn't wait for, with your own token (`--token`, `MAPBOX_ACCESS_TOKEN` or your login) or, when you have none, diff --git a/README.md b/README.md index 6973511..d3aff54 100644 --- a/README.md +++ b/README.md @@ -282,8 +282,9 @@ deeper where that reads better, as in `mapbox styles draft get`. parameters and sample output. Every API request a command makes sends `User-Agent: mapbox-cli/` -and nothing else about you or your machine. `MAPBOX_CLI_NO_TELEMETRY=1` -keeps even future markers out of that header. +and nothing else about you or your machine. `MAPBOX_CLI_NO_TELEMETRY=1` or +`mapbox config set telemetry off` keeps even future markers out of that +header. Each run also sends one event to Mapbox: @@ -572,7 +573,7 @@ kept narrow: | What it sends | A `GET` for the channel's `latest/manifest.json`, with no token, no account, no command, and nothing about you or your machine beyond `User-Agent: mapbox-cli/` | | When | At most once a day, and only when stderr is a terminal, so CI and piped runs never check and never print | | Where | A detached background process. Your command never waits on it: offline, the timing is unchanged and nothing is printed | -| Off | `MAPBOX_NO_UPDATE_CHECK=1`, or `MAPBOX_CLI_NO_TELEMETRY=1`, which silences this too, for the shell session it's set in | +| Off | `MAPBOX_NO_UPDATE_CHECK=1`, or `MAPBOX_CLI_NO_TELEMETRY=1`, which silences this too, for the shell session it's set in; `mapbox config set telemetry off` silences it for good | `~/.mapbox/update-check.json` (or `$MAPBOX_CONFIG_DIR`) holds the answer between runs. A build that names no release channel never checks at all, and @@ -601,7 +602,7 @@ mapbox history show be40d711 # or one run, by any prefix of its id are not recorded. `mapbox config set history off` turns history off for good, and `MAPBOX_HISTORY=0` for one shell; with it off, nothing is written and no directory is created, but what was already recorded stays until you -delete `~/.mapbox/history`. `MAPBOX_CLI_NO_TELEMETRY` does not affect it. +delete `~/.mapbox/history`. Neither telemetry opt-out affects it. ### Diagnostic logs @@ -669,6 +670,12 @@ our CLIs by setting MAPBOX_CLI_NO_TELEMETRY=1 ``` +or, to turn it off in every shell, + +```sh +mapbox config set telemetry off +``` + For additional information on our data processing activities and your related rights, please see our Mapbox [Privacy Policy](https://www.mapbox.com/legal/privacy). diff --git a/docs/commands.md b/docs/commands.md index 62ed3ca..ddfa765 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4130,7 +4130,7 @@ was set in, and stays in every future shell instead. | `update-check` | `on` | The update notice; mirrors `MAPBOX_NO_UPDATE_CHECK` (see [Update notices](../README.md#update-notices)) | | `history` | `on` | [Command history](../README.md#command-history), read by `mapbox history`; `MAPBOX_HISTORY=0` or `=1` overrides it for a session | | `log` | `off` | [Diagnostic logs](../README.md#diagnostic-logs), shown by `mapbox history show`; `MAPBOX_LOG=1` or `=0` overrides it for a session. Needs `history` on: `config set log on` with history off fails with `history_required` | -| `telemetry` | `on` | The run's telemetry event; mirrors `MAPBOX_CLI_NO_TELEMETRY` (see [Privacy](../README.md#privacy)) | +| `telemetry` | `on` | Telemetry — the run's event, the `User-Agent` markers and the update notice; mirrors `MAPBOX_CLI_NO_TELEMETRY` (see [Privacy](../README.md#privacy)) | ### `mapbox config get` @@ -4574,6 +4574,7 @@ Telemetry: on "proxy": { "active": [] }, "switches": { "telemetry_allowed": true, + "telemetry_persisted": true, "update_check_env_opt_out": false, "update_check_persisted": true }, diff --git a/src/doctor.rs b/src/doctor.rs index ed63f19..ab91fb3 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -238,7 +238,11 @@ impl ProxyReport { #[derive(Serialize)] struct SwitchesReport { + /// `MAPBOX_CLI_NO_TELEMETRY` alone, as before `telemetry_persisted` + /// existed; a script that read it keeps reading the same fact. telemetry_allowed: bool, + /// `mapbox config set telemetry`; the event needs this and the above. + telemetry_persisted: bool, update_check_env_opt_out: bool, update_check_persisted: bool, } @@ -248,7 +252,8 @@ impl SwitchesReport { 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(), + telemetry_allowed: telemetry::env_allows_telemetry(), + telemetry_persisted: config::telemetry_enabled(), update_check_env_opt_out: env_opted_out, update_check_persisted: config::update_check_enabled(), } @@ -264,7 +269,10 @@ impl SwitchesReport { /// 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 + self.update_check_persisted + && !self.update_check_env_opt_out + && self.telemetry_allowed + && self.telemetry_persisted } fn update_check_field(&self) -> (&'static str, String) { @@ -272,6 +280,8 @@ impl SwitchesReport { format!(" ({} is set)", update_check::NO_UPDATE_CHECK_ENV) } else if !self.telemetry_allowed { " (MAPBOX_CLI_NO_TELEMETRY silences this too)".to_string() + } else if !self.telemetry_persisted { + " (mapbox config set telemetry off silences this too)".to_string() } else if !self.update_check_persisted { " (mapbox config set update-check off)".to_string() } else { @@ -282,8 +292,14 @@ impl SwitchesReport { } fn telemetry_field(&self) -> (&'static str, String) { - let state = if self.telemetry_allowed { "on" } else { "off" }; - ("Telemetry:", state.to_string()) + let value = if !self.telemetry_allowed { + "off (MAPBOX_CLI_NO_TELEMETRY is set)" + } else if !self.telemetry_persisted { + "off (mapbox config set telemetry off)" + } else { + "on" + }; + ("Telemetry:", value.to_string()) } } diff --git a/src/telemetry.rs b/src/telemetry.rs index afe4176..ce07911 100644 --- a/src/telemetry.rs +++ b/src/telemetry.rs @@ -1,5 +1,6 @@ //! What this CLI's `User-Agent` says about the environment, beyond its -//! version. `MAPBOX_CLI_NO_TELEMETRY` disables all of it. +//! version. `MAPBOX_CLI_NO_TELEMETRY` or `mapbox config set telemetry off` +//! disables all of it — and the update check and the run's event with it. //! //! `crate::http` builds the client and attaches [`user_agent`]'s result. @@ -17,8 +18,17 @@ const NOT_AN_OPT_OUT: [&str; 6] = ["0", "f", "false", "n", "no", "off"]; /// Always sent, even when telemetry is off. pub const PRODUCT_TOKEN: &str = concat!("mapbox-cli/", env!("CARGO_PKG_VERSION")); -/// Whether anything past [`PRODUCT_TOKEN`] may be sent. +/// Whether anything past [`PRODUCT_TOKEN`] may be sent: neither opt-out is +/// on. The two cover the same things, one for a shell and one for good, so +/// every caller asks this rather than either alone. The setting is read +/// afresh each time, so a run that turns it off stops at its next check. pub(crate) fn telemetry_allowed() -> bool { + env_allows_telemetry() && crate::config::telemetry_enabled() +} + +/// `MAPBOX_CLI_NO_TELEMETRY` alone, for `doctor`, which reports the two +/// opt-outs apart. +pub(crate) fn env_allows_telemetry() -> bool { env_switch(MAPBOX_CLI_NO_TELEMETRY_ENV) != Some(true) } diff --git a/src/telemetry_event.rs b/src/telemetry_event.rs index d131a70..b712e73 100644 --- a/src/telemetry_event.rs +++ b/src/telemetry_event.rs @@ -18,8 +18,8 @@ //! //! Best-effort throughout: nothing here can change a command's output, its //! exit code, or how long it takes to return. With telemetry off -//! (`MAPBOX_CLI_NO_TELEMETRY`, or `mapbox config set telemetry off`), -//! nothing is built or written. +//! (`MAPBOX_CLI_NO_TELEMETRY`, or `mapbox config set telemetry off`), nothing +//! is built or written. use std::io::IsTerminal; use std::path::Path; @@ -337,12 +337,9 @@ fn with_workflow(f: impl FnOnce(&mut Option)) { /// second after exit, and a sender still running from that image would keep /// the file locked and the delete would fail. pub(crate) fn deliver(record: &Record) { - // The config setting is read at the end of the run: the run that turns - // telemetry off is one that should not report itself. - if !telemetry::telemetry_allowed() - || !crate::config::telemetry_enabled() - || std::env::var_os("SUDO_USER").is_some() - { + // Read at the end of the run, so the run that turns telemetry off with + // `config set` does not report itself. + if !telemetry::telemetry_allowed() || std::env::var_os("SUDO_USER").is_some() { return; } if cfg!(windows) diff --git a/src/telemetry_sink.rs b/src/telemetry_sink.rs index c300a72..8c3bd60 100644 --- a/src/telemetry_sink.rs +++ b/src/telemetry_sink.rs @@ -189,8 +189,9 @@ pub fn is_send_child() -> bool { /// The whole of the child: read the event from stdin and post it. /// -/// Always succeeds, and says nothing; nobody reads its exit code. The opt-out -/// is checked again here, because this is the process that makes the request. +/// Always succeeds, and says nothing; nobody reads its exit code. Both +/// opt-outs are checked again here, because this is the process that makes +/// the request. pub fn run_send_child() -> ExitCode { if telemetry::telemetry_allowed() { // Through `send_url` again, so a production child cannot be pointed diff --git a/tests/doctor.rs b/tests/doctor.rs index 9e4d08e..847f785 100644 --- a/tests/doctor.rs +++ b/tests/doctor.rs @@ -297,6 +297,92 @@ fn telemetry_off_alone_is_enough_to_turn_the_reported_update_check_off() { ); } +/// `config set telemetry off` turns the event off as surely as the +/// environment variable, so doctor has to say so — and say which switch did +/// it — rather than report the variable alone. +#[test] +fn the_persisted_telemetry_setting_is_reported() { + let home = scratch("telemetry-config-off"); + assert!(command(&home) + .args(["config", "set", "telemetry", "off"]) + .output() + .expect("run mapbox config set") + .status + .success()); + + let json = stdout( + &command(&home) + .args(["-o", "json", "doctor"]) + .output() + .expect("run mapbox doctor"), + ); + assert_eq!(json["switches"]["telemetry_allowed"], true); + assert_eq!(json["switches"]["telemetry_persisted"], false); + + let out = command(&home) + .args(["-o", "text", "doctor"]) + .output() + .expect("run mapbox doctor"); + let text = String::from_utf8_lossy(&out.stdout).to_string(); + assert!( + text.lines().any(|line| line.contains("Telemetry:") + && line.contains("off (mapbox config set telemetry off)")), + "{text}" + ); + // The setting covers what the variable covers, the update check included. + assert!( + text.lines().any(|line| line.contains("Update check:") + && line.contains("off (mapbox config set telemetry off silences this too)")), + "{text}" + ); +} + +/// The persisted setting strips the `User-Agent` markers the way the +/// variable does, leaving the product token that is always sent. +#[test] +fn the_persisted_telemetry_setting_strips_the_user_agent_markers() { + let home = scratch("telemetry-config-user-agent"); + assert!(command(&home) + .args(["config", "set", "telemetry", "off"]) + .output() + .expect("run mapbox config set") + .status + .success()); + + let listener = TcpListener::bind("127.0.0.1:0").expect("a loopback port"); + let url = format!( + "http://{}/", + listener.local_addr().expect("the bound address") + ); + let seen = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().expect("the doctor's request"); + let _ = stream.set_read_timeout(Some(Duration::from_secs(10))); + let mut buf = [0u8; 4096]; + let n = stream.read(&mut buf).unwrap_or(0); + let _ = stream.write_all(b"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n"); + String::from_utf8_lossy(&buf[..n]).into_owned() + }); + + let out = command(&home) + .env("MAPBOX_INTERNAL_DOCTOR_URL", &url) + // A marker that would be sent with telemetry on. + .env("CLAUDECODE", "1") + .args(["-o", "json", "doctor", "--verify"]) + .output() + .expect("run mapbox doctor --verify"); + assert!(out.status.success()); + + let head = seen.join().expect("the server thread"); + let user_agent = head + .lines() + .find_map(|l| l.strip_prefix("user-agent: ")) + .expect("a user agent"); + assert_eq!( + user_agent, + concat!("mapbox-cli/", env!("CARGO_PKG_VERSION")) + ); +} + #[test] fn verify_honors_an_explicit_timeout() { let home = scratch("verify-timeout"); From e35b8178e084c2580fe0377af5a36a04a8a4b757 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Fri, 9 Oct 2026 18:55:36 +0300 Subject: [PATCH 3/3] Keep the telemetry opt-out when config.json has a bad or unknown key --- CHANGELOG.md | 5 +++- docs/commands.md | 4 +++ src/config.rs | 62 +++++++++++++++++++++++++++++++++------ src/telemetry.rs | 13 ++++++-- tests/source_guards.rs | 21 +++++++++++++ tests/telemetry_events.rs | 5 ++++ 6 files changed, 97 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ec63bf..9dc331e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -264,7 +264,10 @@ that may never merge. They are not releases and are not listed here. shell, the way `MAPBOX_CLI_NO_TELEMETRY=1` does for one: the run's event, the `User-Agent` markers and the update notice. The run that turns it off records nothing either. `mapbox config list` now also reports `telemetry`, - and `mapbox doctor` reports it as `telemetry_persisted`. + and `mapbox doctor` reports it as `telemetry_persisted`; its + `telemetry_allowed` still means the environment variable alone. Version + 0.3.0 doesn't know this key, and its `mapbox config set` drops it from the + file, so with two installs, set it again after using the older one. - Each run sends one `cli.command` telemetry event to Mapbox, from a background process the command doesn't wait for, with your own token diff --git a/docs/commands.md b/docs/commands.md index ddfa765..e999e02 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4591,6 +4591,10 @@ Telemetry: on +In `switches`, `telemetry_allowed` is `MAPBOX_CLI_NO_TELEMETRY` alone and +`telemetry_persisted` is `mapbox config set telemetry`; telemetry is on only +when both are `true`. + 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 diff --git a/src/config.rs b/src/config.rs index 0cb0c12..c924efe 100644 --- a/src/config.rs +++ b/src/config.rs @@ -19,8 +19,8 @@ use std::path::PathBuf; use anyhow::{Context, Result}; use clap::builder::PossibleValuesParser; use clap::{Arg, ArgMatches, Command}; -use serde::{Deserialize, Serialize}; -use serde_json::{json, Value}; +use serde::Serialize; +use serde_json::{json, Map, Value}; use crate::auth; use crate::output::{self, CliError, Mode}; @@ -43,7 +43,7 @@ const OFF: &str = "off"; /// default is `on` — distinct from `Some(true)`, which is someone turning it /// back on after having turned it off, but read identically by /// [`update_check_setting`]. -#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize)] struct Config { #[serde(default, skip_serializing_if = "Option::is_none")] update_check: Option, @@ -53,6 +53,33 @@ struct Config { log: Option, #[serde(default, skip_serializing_if = "Option::is_none")] telemetry: Option, + /// Keys this version doesn't know, kept so that writing the file doesn't + /// erase a setting a newer version saved. + #[serde(flatten)] + rest: Map, +} + +impl Config { + /// Reads each key on its own, so one malformed value resets only that + /// key. Parsed as a whole, a single bad value would reset every key, + /// `telemetry` included, and an opt-out would quietly turn back on. + fn parse(text: &str) -> Config { + let Ok(Value::Object(mut rest)) = serde_json::from_str(text) else { + return Config::default(); + }; + let mut flag = |key: &str| rest.remove(key).and_then(|value| value.as_bool()); + let update_check = flag("update_check"); + let history = flag("history"); + let log = flag("log"); + let telemetry = flag("telemetry"); + Config { + update_check, + history, + log, + telemetry, + rest, + } + } } fn config_path() -> Option { @@ -67,7 +94,7 @@ fn config_path() -> Option { fn read_config() -> Config { config_path() .and_then(|path| std::fs::read_to_string(path).ok()) - .and_then(|text| serde_json::from_str(&text).ok()) + .map(|text| Config::parse(&text)) .unwrap_or_default() } @@ -105,8 +132,8 @@ pub fn log_enabled() -> bool { read_config().log.unwrap_or(false) } -/// Whether the run's telemetry event may be recorded, per the persisted -/// setting. [`crate::telemetry_event`] checks it alongside `MAPBOX_CLI_NO_TELEMETRY`. +/// Whether telemetry is on, per the persisted setting. Read through +/// [`crate::telemetry::telemetry_allowed`], alongside `MAPBOX_CLI_NO_TELEMETRY`. pub fn telemetry_enabled() -> bool { read_config().telemetry.unwrap_or(true) } @@ -285,15 +312,32 @@ mod tests { }; let text = serde_json::to_string(&off).expect("serialize"); assert_eq!(text, r#"{"update_check":false}"#); - let read: Config = serde_json::from_str(&text).expect("deserialize"); - assert_eq!(read, off); + assert_eq!(Config::parse(&text), off); // A file from before this key existed, or one with nothing set yet. - let empty: Config = serde_json::from_str("{}").expect("an empty object"); + let empty = Config::parse("{}"); assert_eq!(empty.update_check, None); assert!(update_check_setting(&empty)); } + #[test] + fn a_malformed_value_resets_only_its_own_key() { + let config = Config::parse(r#"{"telemetry":false,"history":"off"}"#); + assert_eq!(config.telemetry, Some(false)); + assert_eq!(config.history, None); + assert_eq!(Config::parse("not json"), Config::default()); + } + + #[test] + fn keys_from_a_newer_version_survive_a_write() { + let mut config = Config::parse(r#"{"history":true,"future_key":[1]}"#); + config.history = Some(false); + assert_eq!( + serde_json::to_string(&config).expect("serialize"), + r#"{"history":false,"future_key":[1]}"# + ); + } + #[test] fn on_and_off_round_trip_through_on_off() { assert_eq!(on_off(true), ON); diff --git a/src/telemetry.rs b/src/telemetry.rs index ce07911..2cf413f 100644 --- a/src/telemetry.rs +++ b/src/telemetry.rs @@ -110,7 +110,13 @@ fn terminal_marker() -> String { /// The full `User-Agent`: [`PRODUCT_TOKEN`], then markers if allowed. pub fn user_agent(command_group: Option<&str>) -> String { - if telemetry_allowed() { + user_agent_if(telemetry_allowed(), command_group) +} + +/// [`user_agent`] with the decision passed in, so a test isn't at the mercy +/// of the persisted setting on the machine running it. +fn user_agent_if(allowed: bool, command_group: Option<&str>) -> String { + if allowed { assemble(&telemetry_markers(command_group)) } else { assemble(&[]) @@ -148,11 +154,11 @@ mod tests { let previous = std::env::var_os(MAPBOX_CLI_NO_TELEMETRY_ENV); let read = |value: &&str| { std::env::set_var(MAPBOX_CLI_NO_TELEMETRY_ENV, value); - user_agent(None) + user_agent_if(env_allows_telemetry(), None) }; std::env::remove_var(MAPBOX_CLI_NO_TELEMETRY_ENV); - let unset = user_agent(None); + let unset = user_agent_if(env_allows_telemetry(), None); let opted_out: Vec = opt_outs.iter().map(read).collect(); let allowed: Vec = left_alone.iter().map(read).collect(); @@ -162,6 +168,7 @@ mod tests { } assert!(unset.starts_with(PRODUCT_TOKEN), "{unset}"); + assert_ne!(unset, PRODUCT_TOKEN, "no markers with the switch unset"); for (value, agent) in opt_outs.iter().zip(&opted_out) { assert_eq!(agent, PRODUCT_TOKEN, "MAPBOX_CLI_NO_TELEMETRY={value:?}"); } diff --git a/tests/source_guards.rs b/tests/source_guards.rs index 04a4db6..bcdccb1 100644 --- a/tests/source_guards.rs +++ b/tests/source_guards.rs @@ -546,3 +546,24 @@ fn every_test_that_runs_the_binary_decides_about_telemetry() { token sends real telemetry: {undecided:?}. Set it on the command they build." ); } + +/// `env_allows_telemetry` is `MAPBOX_CLI_NO_TELEMETRY` alone, so a caller +/// that gates telemetry on it ignores `mapbox config set telemetry off`. +/// Only `telemetry.rs`, which combines the two, and `doctor.rs`, which +/// reports them apart, may call it. +#[test] +fn only_telemetry_and_doctor_read_the_env_opt_out_alone() { + let unexpected: Vec = sources() + .into_iter() + .filter(|(name, source)| { + !["telemetry.rs", "doctor.rs"].contains(&name.as_str()) + && source.contains("env_allows_telemetry") + }) + .map(|(name, _)| format!("src/{name}")) + .collect(); + assert!( + unexpected.is_empty(), + "these modules call `telemetry::env_allows_telemetry`, which ignores the \ + persisted setting: {unexpected:?}. Call `telemetry::telemetry_allowed` instead." + ); +} diff --git a/tests/telemetry_events.rs b/tests/telemetry_events.rs index 08f5509..97a075b 100644 --- a/tests/telemetry_events.rs +++ b/tests/telemetry_events.rs @@ -240,6 +240,11 @@ fn either_opt_out_records_nothing() { received.recv_timeout(Duration::from_secs(3)).is_err(), "`telemetry off` still sent an event" ); + // Not left to the send child's own check: the run itself records nothing. + assert!( + !config_dir(&home).join(".telemetry").exists(), + "`telemetry off` still wrote telemetry" + ); } #[test]