From 22a9936e6bf1c6726be187072ea81b31185b9b58 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 13:24:32 +0300 Subject: [PATCH 1/9] Add mapbox report for feedback about the CLI itself Sends one message to the Feedback API's CLI endpoint, which files it under Mapbox's own account rather than the caller's. The message is the positional; lat/lon are sent as 0 and never typed. --- CHANGELOG.md | 5 + README.md | 13 +- custom-openapi/report/openapi/report.yaml | 74 ++++++++ docs/commands.md | 62 ++++++- src/executor.rs | 9 +- src/help_layout.rs | 2 + src/main.rs | 16 +- src/remedy.rs | 6 + src/schema.rs | 8 +- src/spec.rs | 203 ++++++++++++++++++++-- tests/dry_run.rs | 59 +++++++ tests/fixtures/api_command_surface.txt | 1 + 12 files changed, 432 insertions(+), 26 deletions(-) create mode 100644 custom-openapi/report/openapi/report.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a71156..ca7f16e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,11 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +- New `mapbox report ""` sends feedback about the CLI itself to + the Mapbox CLI team. It works with any valid token, needs no + `user-feedback:write` scope, and files nothing under your own account. + `--category` is optional and defaults to `general`. + - The CLI's own token, used for telemetry when you have no token of your own, is now fetched from `https://cli.mapbox.com/config/token.json` and kept in `~/.mapbox/cli-token.json` instead of being built into the binary, diff --git a/README.md b/README.md index f757e93..a14a87d 100644 --- a/README.md +++ b/README.md @@ -252,7 +252,7 @@ mapbox tilesets ``` `mapbox directions`, `mapbox isochrone`, `mapbox map-match`, -`mapbox matrix`, and `mapbox places` are the exceptions: each API has a +`mapbox matrix`, `mapbox places` and `mapbox report` are the exceptions: each API has a single exposed operation, so there's a bare command with no subcommand at all, the same shape `mapbox usage` already has, see [docs/commands.md](./docs/commands.md) for their own parameters. @@ -323,6 +323,17 @@ unexpectedly. It makes no request unless you pass `--verify`. See [Config](./docs/commands.md#config) for details, and [Command history](#command-history) below. +### Feedback about the CLI + +```sh +mapbox report "I'd like support for optimization commands" +mapbox report "tilesets query hangs behind my proxy" --category bug +``` + +The message goes to the Mapbox CLI team, not to your account, so +`mapbox feedback list` won't show it. Any valid token works. See +[Report](./docs/commands.md#report). + ### Shell completion Homebrew installs completions for you. Otherwise, `mapbox completion` diff --git a/custom-openapi/report/openapi/report.yaml b/custom-openapi/report/openapi/report.yaml new file mode 100644 index 0000000..d9dd500 --- /dev/null +++ b/custom-openapi/report/openapi/report.yaml @@ -0,0 +1,74 @@ +openapi: "3.0.0" +# `parse_spec` turns `info.description` below into this service's clap +# `long_about`, so it also reaches `mapbox report --help`, `--schema` and +# `generate-skills` output. Keep it to API prose only — the provenance below +# is for whoever edits this file, not for a CLI user: +# +# An undocumented Feedback API endpoint the Feedback API team added for this +# CLI (AGI-1168): the same body as `POST /user-feedback/v1/feedback`, but +# filed under Mapbox's own `locationai-mapbox` account rather than the +# caller's, and needing no `user-feedback:write`. Not on docs.mapbox.com, so +# this file is the only description of it. See `custom-openapi/README.md` for +# how a file like this is wired in. +# +# One command, `mapbox report ""`, via `FLATTENED_SERVICES`. The +# message is `POSITIONAL_BODY_FIELDS`'s one entry and `lat`/`lon` are +# `FIXED_BODY_FIELDS` (both in `src/spec.rs`): feedback about the CLI has no +# place, and the API still requires the pair. `id`, `feature` and +# `screenshot` are left out on purpose, so this command takes no `--data`. +info: + title: "Mapbox CLI feedback" + description: >- + Send feedback about the Mapbox CLI itself — a bug, a missing command, or + anything else — to the team that builds it. It goes to Mapbox, not to + your account, so `mapbox feedback list` will not show it. + version: "0.0.0" +servers: + - url: https://api.mapbox.com + description: Feedback API +paths: + /user-feedback/v1/cli-feedback: + post: + operationId: report + summary: Send feedback about the Mapbox CLI. + description: >- + Sends one message to the Mapbox CLI team. Any valid token works; no + extra scope is needed. There is no way to read it back or delete it + afterwards. + parameters: + - name: "access_token" + in: query + required: true + description: "Mapbox API Access Token" + schema: + type: string + minLength: 1 + requestBody: + required: true + content: + application/json: + schema: + type: object + required: ["feedback", "lat", "lon", "category"] + properties: + feedback: + type: string + description: "What you want to tell the Mapbox CLI team." + lat: + type: number + description: "Latitude. Always 0: CLI feedback has no place." + lon: + type: number + description: "Longitude. Always 0: CLI feedback has no place." + category: + type: string + description: >- + What kind of feedback this is, such as `bug` or + `feature_request`. Free text. + responses: + "200": + description: The created feedback item. + "400": + description: Bad Request — a missing or invalid field. + "401": + description: Unauthorized diff --git a/docs/commands.md b/docs/commands.md index bc3c8a0..5c174f2 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1,8 +1,8 @@ # Implemented commands -Every command the CLI ships, including five auth commands, 40 API operations (35 across 9 -command groups, plus `directions`, `isochrone`, `map-match`, `matrix` and -`places`), the tilesets-cli proxy, `completion` and `generate-skills`. Each is +Every command the CLI ships, including five auth commands, 41 API operations (35 across 9 +command groups, plus `directions`, `isochrone`, `map-match`, `matrix`, +`places` and `report`), the tilesets-cli proxy, `completion` and `generate-skills`. Each is shown in both of its renderings. Which one you get is decided by `--output`, whose default (`auto`) reads stdout: a terminal gets the left column, a pipe or redirect gets the right one. See @@ -81,6 +81,8 @@ nests, and is typed `mapbox styles draft get`. **[Places](#places)** — [places](#mapbox-places) +**[Report](#report)** — [report](#mapbox-report) + **[Search](#search)** — [search.forward](#mapbox-search-forward) · [search.reverse](#mapbox-search-reverse) · [search.category](#mapbox-search-category) · @@ -462,10 +464,11 @@ No stored profiles. Run `mapbox auth login` to create one. ## API command groups -35 operations across 9 command groups, plus five single-operation commands -with no group: `directions`, `isochrone`, `map-match`, `matrix` and `places`. +35 operations across 9 command groups, plus six single-operation commands +with no group: `directions`, `isochrone`, `map-match`, `matrix`, `places` +and `report`. Seven groups are generated from the OpenAPI specs vendored in `openapi/`; -`search`, `feedback` and the five single commands are the exceptions — +`search`, `feedback` and the six single commands are the exceptions — hand-authored specs versioned in this repo's own `custom-openapi/`, see `custom-openapi/README.md`. @@ -490,7 +493,7 @@ moved to `tilesets` or `static`. | [`styles`](#styles) | **9** | | [`tilesets`](#tilesets) | **3** | -The [Contents](#contents) list above names every one of the 40. +The [Contents](#contents) list above names every one of the 41. **Everything else the Mapbox specs describe is not here at all.** Not hidden, not shipped as a command that refuses: absent from the spec content @@ -598,8 +601,8 @@ Three things worth knowing about the read forms: A command that changes something takes `--dry-run`, which prints the request it would send, on stdout, and sends nothing. Which commands those are is not -a list anyone keeps: it is every `POST`, `PUT`, `PATCH` and `DELETE` — 14 of -the 40 operations — plus `auth login`, `auth logout`, `auth refresh` and +a list anyone keeps: it is every `POST`, `PUT`, `PATCH` and `DELETE` — 15 of +the 41 operations — plus `auth login`, `auth logout`, `auth refresh` and `generate-skills`. A read-only `GET` does not take it, so `mapbox styles list --dry-run` is a usage error rather than a no-op. It rehearses rather than describes: `--data` is parsed and every `--file` is read, so a @@ -2002,6 +2005,47 @@ $ echo $? --- +## Report + +Feedback about the CLI itself — a bug, a missing command, anything else — +sent to the Mapbox CLI team. It goes through an endpoint the Feedback API +added for this CLI and doesn't document on docs.mapbox.com, so this +section is its only description; see `custom-openapi/README.md` for how +that spec is wired in. + +### `mapbox report` + +Sends one message. It is filed under a Mapbox account, not yours: any +valid token works, no `user-feedback:write` scope is needed, and +`mapbox feedback list` won't show it. There is no way to read it back or +delete it, so `--dry-run` shows the body without sending it. + +#### Parameters + +`` (positional, required) — what you want to tell the team. +Quote it. + +| Flag | Effect | +| --- | --- | +| `--category ` | What kind of feedback this is, such as `bug` or `feature_request`. Free text. Defaults to `general`. | + +The API requires `lat` and `lon` as well; CLI feedback has no place, so +both are always sent as `0`. The command takes no `--data`. + +#### Examples + +```sh +mapbox report "I'd like support for optimization commands" +mapbox report "tilesets query hangs behind my proxy" --category bug +mapbox report "…" --dry-run +``` + +#### Outputs + +TODO(capture): live output pending. + +--- + ## Search The public, non-interactive surface of the Search Box API: text search, diff --git a/src/executor.rs b/src/executor.rs index 6664141..a5c6759 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -212,7 +212,14 @@ fn dispatch( let data = data_argument .as_ref() .map(|argument| argument.body.as_ref()); - let merged_body = merge_body_fields(&op.body_fields, matches, data)?; + let body_fields: Vec = op + .body_fields + .iter() + .chain(&op.body_positional) + .chain(&op.fixed_body_fields) + .cloned() + .collect(); + let merged_body = merge_body_fields(&body_fields, matches, data)?; let variadic_body = variadic_body_array_json(&op.variadic_body_array, matches); let data = variadic_body.as_deref().or(merged_body.as_deref()).or(data); let files: Vec<&str> = matches diff --git a/src/help_layout.rs b/src/help_layout.rs index 3561fba..99acdde 100644 --- a/src/help_layout.rs +++ b/src/help_layout.rs @@ -51,6 +51,7 @@ const GROUPS: &[(&str, &[&str])] = &[ "completion", "tilesets-cli", "uninstall", + "report", "help", ], ), @@ -95,6 +96,7 @@ const DESCRIPTIONS: &[(&str, &str)] = &[ "generate-skills", "Write Agent Skills describing this CLI's own commands", ), + ("report", "Send feedback about this CLI to Mapbox"), ("help", "Print help for mapbox or a command"), ]; diff --git a/src/main.rs b/src/main.rs index 7328ea7..dbfaa0b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -286,8 +286,9 @@ fn build_operation_command(op: &spec::Operation) -> Command { // A variadic positional builds the whole body on its own — see // below — so `--data` would be a second, redundant way to say the // same thing, with nothing else the body ever carries. - let takes_data = - (body.accepts_json() || text_body.is_some()) && op.variadic_body_array.is_none(); + let takes_data = (body.accepts_json() || text_body.is_some()) + && op.variadic_body_array.is_none() + && op.body_positional.is_none(); if takes_data { cmd = cmd.arg( Arg::new("data") @@ -349,6 +350,17 @@ fn build_operation_command(op: &spec::Operation) -> Command { cmd = cmd.arg(arg); } + // `merge_body_fields` reads it by `arg_name`, the same as a flag. A + // `fixed_body_fields` entry has no argument at all; its default is what + // gets sent. + if let Some(field) = &op.body_positional { + cmd = cmd.arg( + Arg::new(field.arg_name.clone()) + .required(true) + .help(help_text(field)), + ); + } + if let Some(variadic) = &op.variadic_body_array { let help = variadic .description diff --git a/src/remedy.rs b/src/remedy.rs index 578023d..5a3acb7 100644 --- a/src/remedy.rs +++ b/src/remedy.rs @@ -95,6 +95,12 @@ const SERVICE_DOCS: &[(&str, &str)] = &[ ), ("matrix", "https://docs.mapbox.com/api/navigation/matrix/"), ("places", "https://docs.mapbox.com/api/search/places/"), + // Undocumented on docs.mapbox.com by design (see `report.yaml`), so + // this repository's own page is the only one. + ( + "report", + "https://github.com/mapbox/cli/blob/main/docs/commands.md#mapbox-report", + ), ("search", "https://docs.mapbox.com/api/search/search-box/"), // Static Images and Static Tiles merged into one `static` command group // (#116); neither upstream page covers both, so this points at Static diff --git a/src/schema.rs b/src/schema.rs index 6e1edf8..2f8387d 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -478,7 +478,7 @@ fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry body_arguments( body, !op.body_fields.is_empty(), - op.variadic_body_array.is_some(), + op.variadic_body_array.is_some() || op.body_positional.is_some(), declared, ) }) @@ -491,6 +491,12 @@ fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry ..parameter(field, ArgKind::Option) })); + arguments.extend(op.body_positional.iter().map(|field| Argument { + required: true, + location: Some("body"), + ..parameter(field, ArgKind::Positional) + })); + arguments.extend(op.variadic_body_array.as_ref().map(|variadic| Argument { required: true, repeatable: true, diff --git a/src/spec.rs b/src/spec.rs index c3bc8ec..8069242 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -33,6 +33,12 @@ pub struct Operation { /// A single JSON body array field exposed as one or more positional /// arguments instead of `--data`. See [`VARIADIC_BODY_ARRAY`]. pub variadic_body_array: Option, + /// A JSON body property taken as the command's positional argument. + /// See [`POSITIONAL_BODY_FIELDS`]. + pub body_positional: Option, + /// JSON body properties sent with a fixed value and never typed. See + /// [`FIXED_BODY_FIELDS`]. + pub fixed_body_fields: Vec, pub base_url: String, /// Set when the operation needs a scope that isn't registrable as OAuth /// (see `UNSUPPORTED_OPERATIONS` below). A `mapbox auth login` token can @@ -83,16 +89,37 @@ type BodyField = (&'static str, Option<&'static str>); /// The API requires `lat`/`lon`, but some feedback has no place. The /// Feedback API team's advice (2026-10-06) is to send 0,0 until the pair /// becomes optional; drop the defaults then. -const BODY_FIELD_FLAGS: &[(&str, &str, &[BodyField])] = &[( - "feedback", - "create", - &[ - ("feedback", None), - ("lat", Some("0")), - ("lon", Some("0")), - ("category", None), - ], -)]; +const BODY_FIELD_FLAGS: &[(&str, &str, &[BodyField])] = &[ + ( + "feedback", + "create", + &[ + ("feedback", None), + ("lat", Some("0")), + ("lon", Some("0")), + ("category", None), + ], + ), + ("report", "report", &[("category", Some("general"))]), +]; + +/// (service, operationId, body field name, CLI positional name) for a +/// required string body property that is what the command is for, typed as +/// its positional argument: `mapbox report ""`, not `--feedback`. +/// An operation with one takes no `--data`, the same as +/// [`VARIADIC_BODY_ARRAY`]'s. +const POSITIONAL_BODY_FIELDS: &[(&str, &str, &str, &str)] = + &[("report", "report", "feedback", "message")]; + +/// A body property and the value always sent for it. +type FixedBodyField = (&'static str, &'static str); + +/// JSON body properties sent with the same value on every request and never +/// offered as a flag, for a property the API requires but the command has no +/// use for. `report` sends feedback about the CLI, which has no place, and +/// the API still requires `lat`/`lon` — see [`BODY_FIELD_FLAGS`] for why 0. +const FIXED_BODY_FIELDS: &[(&str, &str, &[FixedBodyField])] = + &[("report", "report", &[("lat", "0"), ("lon", "0")])]; /// (service, operationId, body field name, CLI positional name, max count) /// for an operation whose JSON body takes a single array field, exposed as @@ -238,8 +265,14 @@ fn arg_name_override(service_name: &str, param_name: &str) -> Option<&'static st /// `generate-skills`, this file's own `command()` above — reads a /// [`FLATTENED_SERVICES`] service correctly for free, because they all go /// through `command()` rather than reconstructing the string themselves. -pub const FLATTENED_SERVICES: &[&str] = - &["directions", "isochrone", "map-match", "matrix", "places"]; +pub const FLATTENED_SERVICES: &[&str] = &[ + "directions", + "isochrone", + "map-match", + "matrix", + "places", + "report", +]; /// (service, path parameter name) pairs whose value is trusted to reach the /// URL unescaped, because every legitimate value already contains a @@ -827,6 +860,10 @@ pub const CUSTOM_SPEC_ENTRIES: &[SpecEntry] = &[ name: "places", yaml: include_str!("../custom-openapi/places/openapi/places.yaml"), }, + SpecEntry { + name: "report", + yaml: include_str!("../custom-openapi/report/openapi/report.yaml"), + }, ]; /// The list the CLI actually generates commands from: [`MAPBOX_SPEC_ENTRIES`], @@ -1170,6 +1207,30 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { None => None, }; + let body_positional = + match operation_id.and_then(|id| positional_body_field_names(service_name, id)) { + Some((field, arg_name)) => Some( + positional_body_field_parameter(op, &doc, field, arg_name).with_context( + || format!("{service_name} {}", operation_id.unwrap_or("")), + )?, + ), + None => None, + }; + + let fixed_body_fields = match operation_id + .and_then(|id| fixed_body_field_values(service_name, id)) + { + Some(values) => { + let fields: Vec = values + .iter() + .map(|(name, value)| (*name, Some(*value))) + .collect(); + body_field_parameters(op, &doc, &fields) + .with_context(|| format!("{service_name} {}", operation_id.unwrap_or("")))? + } + None => vec![], + }; + let disabled_scope = operation_id.and_then(|id| unsupported_scope_for(service_name, id)); @@ -1189,6 +1250,8 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { body, body_fields, variadic_body_array, + body_positional, + fixed_body_fields, base_url: base_url.clone(), disabled_scope, detail: None, @@ -1279,6 +1342,53 @@ fn body_field_names(service: &str, operation_id: &str) -> Option<&'static [BodyF .map(|(_, _, names)| *names) } +fn positional_body_field_names( + service: &str, + operation_id: &str, +) -> Option<(&'static str, &'static str)> { + POSITIONAL_BODY_FIELDS + .iter() + .find(|(s, id, _, _)| *s == service && *id == operation_id) + .map(|(_, _, field, arg_name)| (*field, *arg_name)) +} + +fn fixed_body_field_values(service: &str, operation_id: &str) -> Option<&'static [FixedBodyField]> { + FIXED_BODY_FIELDS + .iter() + .find(|(s, id, _)| *s == service && *id == operation_id) + .map(|(_, _, values)| *values) +} + +/// A [`POSITIONAL_BODY_FIELDS`] entry as a required string [`Parameter`], +/// named for the positional rather than the property. Its description is +/// the property's own: there is no `--data` to carry it instead. +fn positional_body_field_parameter( + op: &Value, + full_spec: &Value, + field: &'static str, + arg_name: &str, +) -> Result { + let parameter = body_field_parameters(op, full_spec, &[(field, None)])?.remove(0); + anyhow::ensure!( + parameter.numeric.is_none() && parameter.enum_values.is_empty(), + "body property `{field}` is listed in POSITIONAL_BODY_FIELDS but is not a free string \ + in the spec" + ); + let schema = &op["requestBody"]["content"]["application/json"]["schema"]; + let schema = match schema["$ref"].as_str() { + Some(reference) => resolve_ref(full_spec, reference).unwrap_or(schema), + None => schema, + }; + Ok(Parameter { + arg_name: arg_name.to_string(), + required: true, + description: schema["properties"][field]["description"] + .as_str() + .map(first_paragraph), + ..parameter + }) +} + /// Nothing enforces `required`, since `--data` may carry the property; the /// description says it instead. fn body_field_parameters( @@ -2054,6 +2164,75 @@ requestBody: assert_eq!(variadic.max, 100); } + /// `report`'s message is a positional and `lat`/`lon` are never flags: + /// the table entries have to reach the parsed operation, typed by the + /// spec, or the request goes out with strings where numbers belong. + #[test] + fn report_takes_its_message_as_a_positional_and_fixes_its_place() { + let spec = parse_spec( + "report", + include_str!("../custom-openapi/report/openapi/report.yaml"), + ) + .expect("report.yaml parses"); + + assert!(FLATTENED_SERVICES.contains(&"report")); + let [op] = spec.operations.as_slice() else { + panic!("report.yaml describes exactly one operation"); + }; + assert_eq!(op.command(), "report"); + + let message = op.body_positional.as_ref().expect("a positional"); + assert_eq!( + (message.name.as_str(), message.arg_name.as_str()), + ("feedback", "message") + ); + assert!(message.required); + assert!(!message + .description + .as_deref() + .unwrap_or("") + .contains("--data")); + + let flags: Vec<&str> = op.body_fields.iter().map(|f| f.name.as_str()).collect(); + assert_eq!(flags, ["category"]); + + let fixed: Vec<(&str, Option<&str>, Option)> = op + .fixed_body_fields + .iter() + .map(|f| (f.name.as_str(), f.default.as_deref(), f.numeric)) + .collect(); + assert_eq!( + fixed, + [ + ("lat", Some("0"), Some(Numeric::Float)), + ("lon", Some("0"), Some(Numeric::Float)), + ] + ); + } + + #[test] + fn a_positional_body_field_needs_a_free_string() { + let op: Value = serde_yaml::from_str( + r#" +requestBody: + content: + application/json: + schema: + type: object + required: [text] + properties: + text: { type: string, description: "The text." } + lat: { type: number } +"#, + ) + .unwrap(); + + let field = positional_body_field_parameter(&op, &op, "text", "message").unwrap(); + assert_eq!(field.description.as_deref(), Some("The text.")); + assert!(positional_body_field_parameter(&op, &op, "lat", "message").is_err()); + assert!(positional_body_field_parameter(&op, &op, "missing", "message").is_err()); + } + #[test] fn an_operation_with_no_request_body_has_none() { let svc = service(BODIES); diff --git a/tests/dry_run.rs b/tests/dry_run.rs index 052835f..7997c51 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -509,3 +509,62 @@ fn places_refuses_more_than_a_hundred_ids_before_sending() { stderr(&out) ); } + +/// `report`'s message is its positional, `lat`/`lon` are sent without being +/// typed, and it goes to the CLI endpoint, not the one `feedback create` +/// files under the caller's account. +#[test] +fn report_sends_the_message_to_the_cli_endpoint() { + let config = config_dir("report"); + let plan = |extra: &[&str]| { + let mut args = vec![ + "-o", + "json", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "A test", + ]; + args.extend_from_slice(extra); + let out = run(&config, &args); + assert!(out.status.success(), "{}", stderr(&out)); + json(&stdout(&out)) + }; + + let defaults = plan(&[]); + assert_eq!( + defaults["url"], + "https://api.mapbox.com/user-feedback/v1/cli-feedback" + ); + assert_eq!( + defaults["body"]["json"], + serde_json::json!({ "feedback": "A test", "lat": 0, "lon": 0, "category": "general" }) + ); + + let categorized = plan(&["--category", "bug"]); + assert_eq!(categorized["body"]["json"]["category"], "bug"); +} + +/// The positional is the whole body; a `--data` would be a second way to +/// say it, or a way to send a place this endpoint has no use for. +#[test] +fn report_needs_a_message_and_takes_no_data() { + let config = config_dir("report-usage"); + for args in [ + &["--token", "sk.a-test-token", "report", "--dry-run"][..], + &[ + "--token", + "sk.a-test-token", + "report", + "A test", + "--data", + "{}", + "--dry-run", + ][..], + ] { + let out = run(&config, args); + assert_eq!(out.status.code(), Some(2), "{args:?}: {}", stderr(&out)); + assert!(stdout(&out).is_empty(), "{args:?}: {}", stdout(&out)); + } +} diff --git a/tests/fixtures/api_command_surface.txt b/tests/fixtures/api_command_surface.txt index d2bc796..7c002d3 100644 --- a/tests/fixtures/api_command_surface.txt +++ b/tests/fixtures/api_command_surface.txt @@ -15,6 +15,7 @@ mapbox isochrone | aliases: (none) mapbox map-match | aliases: (none) mapbox matrix | aliases: (none) mapbox places | aliases: (none) +mapbox report | aliases: (none) mapbox search category | aliases: (none) mapbox search forward | aliases: (none) mapbox search list-category | aliases: (none) From 81c2d0ab13b860feac14fd3f26d188b2ec78966a Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 15:07:07 +0300 Subject: [PATCH 2/9] Send mapbox report with the CLI's token first Feedback about the CLI is filed under Mapbox's own account, so report is the CLI's request rather than the user's. It is the first command in CLI_TOKEN_COMMANDS, as Use::Preferred: it sends with the CLI's token, and falls back to the user's only while the CLI's can't be used. With no token of the user's either, it fails with cli_token_unavailable or cli_token_forbidden, saying when the CLI's will be tried again. The rules for sending with the CLI's token now live in one place, cli_token::send_with_cli_token, which telemetry and commands both use: check a refusal at once, replace a dead token and resend once, rest from an API that refuses a valid one, and end the rest on a 2xx. MAPBOX_CLI_TOKEN is tried before a cooldown, which is the fetched token's. A refused request is resent without asking for confirmation again, and a command on the list may not read its body from stdin, which a resend could not read twice. --- CHANGELOG.md | 10 +- README.md | 6 +- docs/commands.md | 17 +- src/cli_token.rs | 350 ++++++++++++++++++++++++++++++++++++----- src/cli_token/fetch.rs | 135 ++++++++++++++-- src/main.rs | 174 ++++++++++++++++++-- tests/dry_run.rs | 88 +++++++++++ 7 files changed, 707 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca7f16e..4d4b413 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,13 @@ that may never merge. They are not releases and are not listed here. ## Unreleased - New `mapbox report ""` sends feedback about the CLI itself to - the Mapbox CLI team. It works with any valid token, needs no - `user-feedback:write` scope, and files nothing under your own account. - `--category` is optional and defaults to `general`. + the Mapbox CLI team. It is sent with the CLI's own token, so it needs no + login, no `user-feedback:write` scope, and files nothing under your own + account; your token stands in only while the CLI's can't be used. + `--category` is optional and defaults to `general`. Two new error codes + say why the CLI's token couldn't be used when you have no token either: + `cli_token_unavailable` and `cli_token_forbidden` (see + [Errors](docs/commands.md#errors)). - The CLI's own token, used for telemetry when you have no token of your own, is now fetched from `https://cli.mapbox.com/config/token.json` and diff --git a/README.md b/README.md index a14a87d..5e7dbec 100644 --- a/README.md +++ b/README.md @@ -331,7 +331,8 @@ mapbox report "tilesets query hangs behind my proxy" --category bug ``` The message goes to the Mapbox CLI team, not to your account, so -`mapbox feedback list` won't show it. Any valid token works. See +`mapbox feedback list` won't show it. It is sent with the CLI's own token, +so it needs no login; yours stands in only while the CLI's can't be used. See [Report](./docs/commands.md#report). ### Shell completion @@ -499,7 +500,8 @@ third in a row pauses telemetry's sending with it for an hour, as three fetches in a row that find no usable token pause fetching. The token is restricted to `https://cli.mapbox.com/`, so every request with it carries that as its `Referer`. The CLI's token is only used for what -`src/cli_token.rs` lists for it, today just telemetry. +`src/cli_token.rs` lists for it: telemetry, and `mapbox report`, which +prefers it to yours and falls back to yours while it can't be used. ### Proxies diff --git a/docs/commands.md b/docs/commands.md index 5c174f2..5356122 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2015,10 +2015,17 @@ that spec is wired in. ### `mapbox report` -Sends one message. It is filed under a Mapbox account, not yours: any -valid token works, no `user-feedback:write` scope is needed, and -`mapbox feedback list` won't show it. There is no way to read it back or -delete it, so `--dry-run` shows the body without sending it. +Sends one message. It is filed under a Mapbox account, not yours, so +`mapbox feedback list` won't show it, and no `user-feedback:write` scope is +needed. There is no way to read it back or delete it, so `--dry-run` shows +the body without sending it. + +It is sent with the CLI's own token, not yours, so it works without a +login; `--token` and `--use-login` are noted as unused. While the CLI's +token can't be used — it can't be fetched, it is revoked with no new one +yet, or this API refused it recently — your own token is used instead, and +with none the command fails with `cli_token_unavailable` or +`cli_token_forbidden`, saying when it will try the CLI's again. #### Parameters @@ -5126,6 +5133,8 @@ faults and are not: | `binary_response` | The response was bytes and stdout is a terminal. Redirect it to a file. | | `missing_subcommand` | A command group was named with no operation. | | `cancelled` | A delete was declined at the confirmation prompt. Nothing was sent. | +| `cli_token_unavailable` | A command that prefers the CLI's own token (`report`) could not use it — it could not be fetched, or it is revoked and no new one is published yet — and you have no token of your own to fall back to. | +| `cli_token_forbidden` | The CLI's own token works, but this API refused it, now or recently enough that it is not tried again yet, and you have no token of your own to fall back to. | | `usage` | Anything else clap rejected. Exit code 2, not 1. | | `error` | Unclassified. | diff --git a/src/cli_token.rs b/src/cli_token.rs index 7c2c37e..fe5b4b7 100644 --- a/src/cli_token.rs +++ b/src/cli_token.rs @@ -25,10 +25,17 @@ //! billed to the account that owns it. So it is decided by what the request //! is for, not where it goes: a request the CLI makes for itself passes //! [`send`] an order that ends in [`Fallback::CliToken`], and every such order -//! is defined in this file (today only [`TELEMETRY`]). No command uses it. -//! Where the request is sent does not matter, so a URL overridden from the +//! is defined in this file (today only [`TELEMETRY`]); a command must be in +//! [`CLI_TOKEN_COMMANDS`], which also says how it uses it ([`Use`]). Where +//! the request is sent does not matter, so a URL overridden from the //! environment keeps the same fallback. //! +//! Either way the token is sent through [`send_with_cli_token`], which holds +//! the rules: check a refusal at once, replace a dead token, rest from an API +//! that refuses a valid one. A command that cannot use the CLI's token falls +//! back to the user's, and with none says why: unavailable right now +//! ([`Unavailable`]) or refused by this API ([`Refusal`]). +//! //! # The token is public //! //! It is served from a public URL, so it must be a dedicated `pk.` token with @@ -44,7 +51,7 @@ use crate::{auth, http}; mod fetch; -use fetch::Refusal; +pub(crate) use fetch::{Refusal, Unavailable}; /// The `Referer` every request with the CLI's token carries. The published /// token is restricted to this URL, so that a web page elsewhere cannot use @@ -79,6 +86,63 @@ pub(crate) const TELEMETRY: &[Fallback] = #[cfg(test)] const ORDERS: &[&[Fallback]] = &[TELEMETRY]; +/// How a command listed in [`CLI_TOKEN_COMMANDS`] uses the CLI's token. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Use { + /// The CLI's token first, and the user's own only while the CLI's + /// cannot be used: unavailable, resting, or refused by this API. + Preferred, +} + +/// Commands that may run on the CLI's token, by the name `--schema` reports, +/// each with how and with the reason the CLI's account should carry their +/// use. Command names are a compatibility promise, so an entry does not +/// quietly move to another API when a spec changes its URL. +/// +/// A command here must not read its body from stdin: a refused request is +/// sent again, with a fresh token or the user's, and stdin can only be read +/// once. `cli_token_commands_read_nothing_from_stdin` holds that. +const CLI_TOKEN_COMMANDS: &[(&str, Use, &str)] = &[( + "report", + Use::Preferred, + "Feedback about the CLI is filed under Mapbox's own account, so it is the \ + CLI's request rather than the user's, and it works for someone not \ + logged in.", +)]; + +fn use_for(command: &str) -> Option { + CLI_TOKEN_COMMANDS + .iter() + .find(|(name, _, _)| *name == command) + .map(|(_, how, _)| *how) +} + +/// The CLI's token for `command`, or `None` when the command is not listed +/// for it: the only way to ask, so an unlisted command never gets one. +/// `fetch` is false where nothing may be sent, as in a dry run, and only a +/// token already on disk is returned then. +pub(crate) fn for_command(command: &str, fetch: bool) -> Option> { + match use_for(command)? { + Use::Preferred => Some(preferred_token(command, fetch)), + } +} + +fn preferred_token(command: &str, fetch: bool) -> Result { + // A developer's override is not the fetched token, whose cooldown does + // not apply to it. + if let Some(token) = usable(std::env::var("MAPBOX_CLI_TOKEN").ok().as_deref()) { + return Ok(token.to_owned()); + } + if let Some(until) = fetch::resting_until(command) { + return Err(Unavailable::Resting { until }); + } + if fetch { + fetch::token() + } else { + fetch::saved_token().ok_or(Unavailable::NotFetched) + } +} + /// The token the user gave this run, ranked as a command ranks it: with /// `--use-login` only a typed `--token` counts, otherwise the environment /// does too, `MapboxAccessToken` included, since that is the user's token @@ -115,35 +179,39 @@ pub(crate) fn send( ) -> Option> { let override_token = std::env::var("MAPBOX_CLI_TOKEN").ok(); let fetched = RefCell::new(None); - let dead = RefCell::new(None); - let mut attempt = |token: &str| { - let is_cli_token = fetched.borrow().as_deref() == Some(token) - || usable(override_token.as_deref()) == Some(token); - let request = request().query(&[("access_token", token)]); - let request = if is_cli_token { - request.header(reqwest::header::REFERER, REFERER) - } else { - request - }; - let response = http::send(request); - // Only the fetched token's answers are kept: the user's tokens, and - // `MAPBOX_CLI_TOKEN`, are never checked or counted. - if let (Ok(r), true) = (&response, fetched.borrow().as_deref() == Some(token)) { - if r.status().is_success() { - fetch::accepted(caller, token); - } else if is_refusal(r.status()) && fetch::refused(caller, token) == Refusal::Dead { - *dead.borrow_mut() = Some(token.to_owned()); - } + let send = |token: &str| http::send(request().query(&[("access_token", token)])); + // Any CLI token, a replacement for a dead one included. + let send_cli = |token: &str| { + http::send( + request() + .query(&[("access_token", token)]) + .header(reqwest::header::REFERER, REFERER), + ) + }; + let attempt = |token: &str| { + // Only the fetched token goes through the CLI token's rules: the + // user's tokens are never checked or counted, nor is + // `MAPBOX_CLI_TOKEN`, which still carries the CLI token's Referer. + if fetched.borrow().as_deref() != Some(token) { + return if usable(override_token.as_deref()) == Some(token) { + send_cli(token) + } else { + send(token) + }; + } + match send_with_cli_token(caller, token.to_owned(), &mut Disk, send_cli, |answer| { + answer.as_ref().ok().map(|r| r.status().as_u16()) + }) { + Sent::Answered(answer) | Sent::Refused(answer, _) => answer, } - response }; let fetch = || { // A caller resting from the CLI's token sends nothing with it. - let token = (!fetch::cooling(caller)).then(fetch::token)??; + let token = (!fetch::cooling(caller)).then(|| fetch::token().ok())??; *fetched.borrow_mut() = Some(token.clone()); Some(token) }; - let answer = resolve( + resolve( order, Sources { user: user_token, @@ -151,21 +219,77 @@ pub(crate) fn send( }, || login_token(profile), fetch, - &mut attempt, + attempt, |response| response.status() == StatusCode::UNAUTHORIZED, - ); - // A dead token is replaced at once, and the request sent once more. - match dead.take() { - Some(dead) if matches!(&answer, Some(Ok(r)) if is_refusal(r.status())) => { - match fetch::token() { - Some(fresh) if fresh != dead => { - *fetched.borrow_mut() = Some(fresh.clone()); - Some(attempt(&fresh)) + ) +} + +/// What a request sent with the CLI's token came to. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum Sent { + /// An answer that was not a refusal, or no answer at all: the caller's. + Answered(R), + /// A `401` or `403` that replacing a dead token did not fix, and why. + Refused(R, Refusal), +} + +/// The state on disk, as [`send_with_cli_token`] uses it, so its rules can +/// be tested without it. +pub(crate) trait Ledger { + fn refused(&mut self, caller: &str, token: &str) -> Refusal; + fn accepted(&mut self, caller: &str, token: &str); + fn fresh(&mut self) -> Option; +} + +/// `/cli-token.json`, through [`fetch`]. +pub(crate) struct Disk; + +impl Ledger for Disk { + fn refused(&mut self, caller: &str, token: &str) -> Refusal { + fetch::refused(caller, token) + } + fn accepted(&mut self, caller: &str, token: &str) { + fetch::accepted(caller, token); + } + fn fresh(&mut self) -> Option { + fetch::token().ok() + } +} + +/// Sends with the CLI's `token` on `caller`'s behalf: the one place the +/// rules for using it live, for telemetry and commands alike. +/// +/// A `2xx` ends `caller`'s cooldown. A `401` or `403` is checked with the +/// Tokens API at once ([`fetch::refused`]): a dead token is replaced and the +/// request sent once more, since a refused request was not acted on; any +/// other refusal starts `caller`'s cooldown and is handed back. `status` is +/// an answer's HTTP status, or `None` when there was no answer. +pub(crate) fn send_with_cli_token( + caller: &str, + mut token: String, + ledger: &mut impl Ledger, + mut send: impl FnMut(&str) -> R, + status: impl Fn(&R) -> Option, +) -> Sent { + let mut replaced = false; + loop { + let answer = send(&token); + match status(&answer) { + Some(401 | 403) => { + let refusal = ledger.refused(caller, &token); + if refusal == Refusal::Dead && !replaced { + if let Some(fresh) = ledger.fresh().filter(|fresh| *fresh != token) { + token = fresh; + replaced = true; + continue; + } } - _ => answer, + return Sent::Refused(answer, refusal); } + Some(200..=299) => ledger.accepted(caller, &token), + _ => {} } - _ => answer, + return Sent::Answered(answer); } } @@ -269,11 +393,6 @@ fn resolve( last.map(Ok) } -/// A `401` or `403`: the answers [`fetch::refused`] can explain. -pub(crate) fn is_refusal(status: StatusCode) -> bool { - status == StatusCode::UNAUTHORIZED || status == StatusCode::FORBIDDEN -} - fn usable(token: Option<&str>) -> Option<&str> { token.map(str::trim).filter(|t| !t.is_empty()) } @@ -517,6 +636,155 @@ mod tests { ); } + #[test] + fn every_cli_token_command_exists_and_says_why() { + let specs = crate::spec::effective_services().expect("the bundled specs parse"); + let commands: Vec = specs + .iter() + .flat_map(|svc| &svc.operations) + .map(|op| op.command()) + .collect(); + for (name, _, reason) in CLI_TOKEN_COMMANDS { + assert!( + commands.iter().any(|c| c == name), + "`{name}` is in CLI_TOKEN_COMMANDS but is not a command" + ); + assert!(!reason.trim().is_empty(), "`{name}` needs a reason"); + } + } + + #[test] + fn cli_token_commands_read_nothing_from_stdin() { + let specs = crate::spec::effective_services().expect("the bundled specs parse"); + for op in specs.iter().flat_map(|svc| &svc.operations) { + if use_for(&op.command()).is_none() { + continue; + } + let cmd = crate::build_operation_command(op); + for id in ["data", "file"] { + assert!( + cmd.get_arguments().all(|arg| arg.get_id() != id), + "`{}` takes `--{id}`, which can read stdin, and a refused request \ + is sent again", + op.command() + ); + } + } + } + + #[test] + fn an_unlisted_command_never_gets_the_clis_token() { + assert_eq!(for_command("styles list", true), None); + } + + /// A ledger that answers every refusal with `verdict` and has `fresh` + /// to hand out once, and records what it was told. + #[derive(Default)] + struct FakeLedger { + verdict: Option, + fresh: Option, + refused: Vec, + accepted: Vec, + } + + impl Ledger for FakeLedger { + fn refused(&mut self, _: &str, token: &str) -> Refusal { + self.refused.push(token.to_owned()); + self.verdict.expect("a verdict to give") + } + fn accepted(&mut self, _: &str, token: &str) { + self.accepted.push(token.to_owned()); + } + fn fresh(&mut self) -> Option { + self.fresh.take() + } + } + + /// Sends through [`send_with_cli_token`] to a fake API that answers + /// `statuses` in turn, and returns the outcome and the tokens sent. + fn send_cli( + ledger: &mut FakeLedger, + statuses: &[Option], + ) -> (Sent>, Vec) { + let mut sent = Vec::new(); + let mut answers = statuses.iter().copied(); + let outcome = send_with_cli_token( + "report", + "pk.old".to_owned(), + ledger, + |token| { + sent.push(token.to_owned()); + answers.next().expect("an answer for each send") + }, + |status| *status, + ); + (outcome, sent) + } + + #[test] + fn a_dead_token_is_replaced_and_sent_once_more() { + let mut ledger = FakeLedger { + verdict: Some(Refusal::Dead), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(401), Some(200)]); + assert_eq!(outcome, Sent::Answered(Some(200))); + assert_eq!(sent, ["pk.old", "pk.new"]); + assert_eq!(ledger.accepted, ["pk.new"]); + } + + #[test] + fn a_replacement_that_is_refused_too_is_not_replaced_again() { + let mut ledger = FakeLedger { + verdict: Some(Refusal::Dead), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(401), Some(401)]); + assert_eq!(outcome, Sent::Refused(Some(401), Refusal::Dead)); + assert_eq!(sent, ["pk.old", "pk.new"]); + } + + #[test] + fn a_dead_token_with_no_new_one_is_handed_back() { + let mut ledger = FakeLedger { + verdict: Some(Refusal::Dead), + fresh: Some("pk.old".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403)]); + assert_eq!(outcome, Sent::Refused(Some(403), Refusal::Dead)); + assert_eq!(sent, ["pk.old"], "the same token is not sent again"); + } + + #[test] + fn a_token_this_api_refuses_is_handed_back_without_a_fetch() { + for verdict in [Refusal::Forbidden, Refusal::Unknown] { + let mut ledger = FakeLedger { + verdict: Some(verdict), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403)]); + assert_eq!(outcome, Sent::Refused(Some(403), verdict)); + assert_eq!(sent, ["pk.old"]); + assert!(ledger.fresh.is_some(), "{verdict:?} fetched a new token"); + } + } + + #[test] + fn only_a_2xx_counts_as_accepted() { + for status in [Some(200), Some(204), Some(404), Some(500), None] { + let mut ledger = FakeLedger::default(); + let (outcome, _) = send_cli(&mut ledger, &[status]); + assert_eq!(outcome, Sent::Answered(status)); + let accepted = status.is_some_and(|s| (200..300).contains(&s)); + assert_eq!(ledger.accepted.len(), usize::from(accepted), "{status:?}"); + assert!(ledger.refused.is_empty(), "{status:?} was checked"); + } + } + #[test] fn the_override_is_trimmed_before_it_is_sent() { let (answer, sent, _) = run( diff --git a/src/cli_token/fetch.rs b/src/cli_token/fetch.rs index 6ae04d3..13fe204 100644 --- a/src/cli_token/fetch.rs +++ b/src/cli_token/fetch.rs @@ -65,6 +65,47 @@ const TRIES_BEFORE_WAITING: u32 = 3; /// Long enough for any `pk.` token; anything longer is not one. const MAX_TOKEN_LEN: usize = 512; +/// Why there is no CLI token to use right now, as a command that wanted one +/// tells its user. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Unavailable { + /// The endpoint could not be reached. Asked again on the next run. + Unreachable, + /// The endpoint answered without a usable token, so it is not asked + /// again before `retry_at` (Unix seconds). `revoked` when there is a + /// dead token it has not replaced yet, rather than never a token at all. + Waiting { retry_at: u64, revoked: bool }, + /// None on disk, and the caller asked not to fetch, as a dry run does. + NotFetched, + /// This caller's API refused the token lately, so it rests from it + /// until `until` (Unix seconds). + Resting { until: u64 }, +} + +impl std::fmt::Display for Unavailable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unreachable => write!(f, "the token endpoint could not be reached"), + Self::Waiting { retry_at, revoked } => write!( + f, + "{}; trying again in {}", + if *revoked { + "it is revoked and no new one is published yet" + } else { + "the token endpoint did not return a usable token" + }, + wait_text(retry_at.saturating_sub(now())) + ), + Self::NotFetched => write!(f, "it has not been fetched yet"), + Self::Resting { until } => write!( + f, + "this API refused it recently; trying it again in {}", + wait_text(until.saturating_sub(now())) + ), + } + } +} + /// What a fetch got back, before [`acquire`] judges it. #[derive(Debug)] pub(crate) enum Fetched { @@ -142,16 +183,18 @@ fn backoff(failures: u32) -> u64 { } } -/// Whether `caller` — today only `telemetry` — is resting from the +/// Whether `caller` — `telemetry`, or a command's name — is resting from the /// CLI's token. While it is, it sends nothing with it, so nothing is /// refused, and nothing is checked. pub(crate) fn cooling(caller: &str) -> bool { - read_state().is_some_and(|state| { - state - .cooldowns - .get(caller) - .is_some_and(|cooldown| holds(cooldown.until, now())) - }) + resting_until(caller).is_some() +} + +/// When `caller`'s cooldown ends, while it has one. +pub(crate) fn resting_until(caller: &str) -> Option { + let state = read_state()?; + let cooldown = state.cooldowns.get(caller)?; + holds(cooldown.until, now()).then_some(cooldown.until) } /// Records that `caller`'s request with the saved `token` went through, which @@ -165,16 +208,39 @@ pub(crate) fn accepted(caller: &str, token: &str) { } /// The CLI's token: the one on disk while it is not dead, otherwise a fresh -/// one from the endpoint unless it is being waited for. `None` when there is -/// none to be had right now. -pub(crate) fn token() -> Option { +/// one from the endpoint unless it is being waited for. +pub(crate) fn token() -> Result { let mut state = read_state().unwrap_or_default(); let before = state.clone(); - let token = acquire(&mut state, now(), || get(&endpoint()), check); + let mut unreachable = false; + let token = acquire( + &mut state, + now(), + || { + let fetched = get(&endpoint()); + unreachable = matches!(fetched, Fetched::Failed); + fetched + }, + check, + ); + let why = if unreachable { + Unavailable::Unreachable + } else { + Unavailable::Waiting { + retry_at: state.retry_at, + revoked: state.rejected, + } + }; if state != before { save_fetch(&before, state); } - token + token.ok_or(why) +} + +/// The token on disk while it is not dead, without asking the endpoint, for +/// a run that must not send anything, such as a dry run. +pub(crate) fn saved_token() -> Option { + read_state()?.usable_token().map(str::to_owned) } /// Writes what a fetch decided, unless another run decided first. The fetch @@ -405,6 +471,22 @@ fn now() -> u64 { .unwrap_or_default() } +fn wait_text(seconds: u64) -> String { + let minutes = seconds.div_ceil(60).max(1); + match minutes { + 1 => "1 minute".to_owned(), + m if m < 60 => format!("{m} minutes"), + m => { + let hours = m.div_ceil(60); + if hours == 1 { + "1 hour".to_owned() + } else { + format!("{hours} hours") + } + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -646,6 +728,35 @@ mod tests { assert_eq!(merged(&before, failed, counted), None); } + #[test] + fn a_wait_says_whether_a_token_was_ever_revoked() { + let waiting = |revoked| { + Unavailable::Waiting { + retry_at: 0, + revoked, + } + .to_string() + }; + assert!( + waiting(true).starts_with("it is revoked"), + "{}", + waiting(true) + ); + assert!( + waiting(false).starts_with("the token endpoint did not return"), + "{}", + waiting(false) + ); + } + + #[test] + fn the_wait_reads_in_minutes_or_hours() { + assert_eq!(wait_text(0), "1 minute"); + assert_eq!(wait_text(59 * 60), "59 minutes"); + assert_eq!(wait_text(HOUR), "1 hour"); + assert_eq!(wait_text(DAY), "24 hours"); + } + #[test] fn only_a_dead_token_is_replaced() { for code in [ diff --git a/src/main.rs b/src/main.rs index dbfaa0b..3ff9d72 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1468,6 +1468,61 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - deprecation::warn_endpoint(op, &op.command()); let dry_run = executor::wants_dry_run(op_matches); + let mut flags = executor::RunFlags { + debug, + assume_yes, + dry_run, + // Read here rather than beside the other globals at the + // top, so that `MAPBOX_TIMEOUT` is only looked at by a + // command it can apply to. A warning about an unreadable + // one in front of `generate-skills`, which sends nothing, + // would be a note about a request that is not happening. + timeout: http::requested(matches), + }; + let command = op.command(); + + // Ahead of the user's credentials, so a run the CLI's token + // serves never refreshes a login it will not send. + let mut cli_failure = None; + if let Some(cli) = cli_token::for_command(&command, !dry_run) { + let username = matches.get_one::("username").cloned(); + let attempt = try_cli_token( + op, + op_matches, + &command, + cli, + username.as_deref(), + flags, + mode, + ); + match attempt { + CliAttempt::Done(result) => { + // Whatever the result: a failure is when someone most + // needs to know which token was sent. + let ignored = if auth::typed_token(matches).is_some() { + Some("`--token`") + } else if use_login { + Some("`--use-login`") + } else { + None + }; + if let Some(choice) = ignored { + output::progress(&format!( + "{choice} was not used: this command sends with the CLI's \ + own token while it works." + )); + } + return result; + } + CliAttempt::Unusable { error, sent } => { + // The user already confirmed the request that was + // refused; sending it again with their own token is + // the same request, not a second one to agree to. + flags.assume_yes |= sent; + cli_failure = Some(error); + } + } + } // Resolve token: explicit flag/env takes priority, then stored // credentials (auto-refreshed, except under `--dry-run` — a stale @@ -1487,6 +1542,19 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - } .or_else(|| stored_creds.as_ref().map(|c| c.access_token.clone())); + // A command that could not use the CLI's token falls back to the + // user's, and with none says why the CLI's failed rather than + // sending a request bound to be refused. A dry run sends nothing + // and fetches nothing, so it plans with whatever there is. + if let Some(failure) = cli_failure.filter(|_| !dry_run) { + if token.is_none() { + return Err((*failure).into()); + } + output::progress(&format!( + "{} Using your own token instead.", + failure.message + )); + } if use_login && token.is_none() { return Err(no_stored_credentials(profile)); } @@ -1503,17 +1571,7 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - op_matches, token.as_deref(), username.as_deref(), - executor::RunFlags { - debug, - assume_yes, - dry_run, - // Read here rather than beside the other globals at the - // top, so that `MAPBOX_TIMEOUT` is only looked at by a - // command it can apply to. A warning about an unreadable - // one in front of `generate-skills`, which sends nothing, - // would be a note about a request that is not happening. - timeout: http::requested(matches), - }, + flags, mode, ) // The executor does not know where its token came from, and that @@ -1533,6 +1591,100 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - Ok(()) } +/// How [`try_cli_token`] ended. +enum CliAttempt { + /// The CLI's token was used; this is the command's own result. + Done(Result<()>), + /// It could not be used, for the reason given, and the user's token may + /// stand in. `sent` when a request went out and was refused, so any + /// confirmation was already given. + Unusable { error: Box, sent: bool }, +} + +/// Runs a [`cli_token::Use::Preferred`] command on the CLI's token, given +/// what [`cli_token::for_command`] found for it, by the rules in +/// [`cli_token::send_with_cli_token`]. +fn try_cli_token( + op: &spec::Operation, + op_matches: &clap::ArgMatches, + command: &str, + cli: Result, + username: Option<&str>, + mut flags: executor::RunFlags, + mode: output::Mode, +) -> CliAttempt { + let token = match cli { + Ok(token) => token, + Err(why) => { + return CliAttempt::Unusable { + error: Box::new(cli_token_unavailable(&why)), + sent: false, + } + } + }; + let execute = + |token: &str, flags| executor::execute(op, op_matches, Some(token), username, flags, mode); + // Sends nothing, so there is nothing to be refused, or to count. + if flags.dry_run { + return CliAttempt::Done(execute(&token, flags)); + } + let sent = cli_token::send_with_cli_token( + command, + token, + &mut cli_token::Disk, + |token| { + let result = execute(token, flags); + // Confirmed once already, if the command asks. + flags.assume_yes = true; + result + }, + |result| match result { + Ok(()) => Some(200), + Err(error) => error.downcast_ref::().and_then(|e| e.status), + }, + ); + let (result, refusal) = match sent { + cli_token::Sent::Answered(result) => return CliAttempt::Done(result), + cli_token::Sent::Refused(result, refusal) => (result, refusal), + }; + let (code, message) = match refusal { + cli_token::Refusal::Dead => ( + "cli_token_unavailable", + "The CLI's token is revoked and no new one is published yet.", + ), + cli_token::Refusal::Forbidden => ( + "cli_token_forbidden", + "The CLI's token is valid, but this API does not accept it.", + ), + cli_token::Refusal::Unknown => ( + "cli_token_forbidden", + "This API refused the CLI's token, and checking why failed.", + ), + cli_token::Refusal::NotFetched => { + ("cli_token_unavailable", "This API refused the CLI's token.") + } + }; + let request_id = result + .err() + .and_then(|e| e.downcast::().ok()) + .and_then(|e| e.request_id); + CliAttempt::Unusable { + error: Box::new(CliError::new(code, message).with_request_id(request_id)), + sent: true, + } +} + +/// Why there was no CLI token to try, as the error a command fails with +/// when the user has no token of their own either. Resting is this API's +/// doing, so it shares the code of the refusal that started it. +fn cli_token_unavailable(why: &cli_token::Unavailable) -> CliError { + let code = match why { + cli_token::Unavailable::Resting { .. } => "cli_token_forbidden", + _ => "cli_token_unavailable", + }; + CliError::new(code, format!("Can't use the CLI's token: {why}.")) +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/dry_run.rs b/tests/dry_run.rs index 7997c51..219f1bb 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -568,3 +568,91 @@ fn report_needs_a_message_and_takes_no_data() { assert!(stdout(&out).is_empty(), "{args:?}: {}", stdout(&out)); } } + +/// What `report` sends with, and what it says, when it cannot use the CLI's +/// token. Every case here is decided before a request: the token endpoint +/// is a dead port, and a cooldown is written to disk. +mod report_token { + use super::*; + + /// A saved CLI token whose cooldown for `report` runs for another hour. + fn resting(config: &Path) { + let until = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("a clock after 1970") + .as_secs() + + 3600; + std::fs::write( + config.join("cli-token.json"), + format!( + r#"{{"token":"pk.saved","overridden":true,"cooldowns":{{"report":{{"failures":1,"until":{until}}}}}}}"# + ), + ) + .expect("write the state"); + } + + fn error_code(out: &Output) -> String { + json(&stderr(out))["code"] + .as_str() + .unwrap_or_default() + .to_owned() + } + + #[test] + fn with_no_token_at_all_it_says_the_clis_is_unavailable() { + let config = config_dir("report-no-tokens"); + let out = run(&config, &["-o", "json", "report", "A test"]); + assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); + assert!(stdout(&out).is_empty(), "{}", stdout(&out)); + assert_eq!(error_code(&out), "cli_token_unavailable"); + } + + #[test] + fn while_resting_with_no_token_it_says_the_api_refused_it() { + let config = config_dir("report-resting"); + resting(&config); + let out = run(&config, &["-o", "json", "report", "A test"]); + assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); + assert_eq!(error_code(&out), "cli_token_forbidden"); + assert!( + stderr(&out).contains("refused it recently"), + "{}", + stderr(&out) + ); + } + + /// `MAPBOX_CLI_TOKEN` is a developer's, not the fetched token, so the + /// fetched token's cooldown does not hold it back. + #[test] + fn a_cooldown_does_not_hold_back_the_override() { + let config = config_dir("report-resting-override"); + resting(&config); + let out = command(&config) + .env("MAPBOX_CLI_TOKEN", "pk.cli") + .args(["--token", "sk.mine", "report", "--dry-run", "A test"]) + .output() + .expect("run mapbox"); + assert!(out.status.success(), "{}", stderr(&out)); + assert!( + stderr(&out).contains("`--token` was not used"), + "{}", + stderr(&out) + ); + } + + /// While the CLI's token cannot be used, the user's is: the plan goes + /// out with it, and a typed `--token` is not reported as ignored. + #[test] + fn while_resting_a_dry_run_plans_with_the_users_token() { + let config = config_dir("report-resting-user"); + resting(&config); + let out = run( + &config, + &["--token", "sk.mine", "report", "--dry-run", "A test"], + ); + assert!(out.status.success(), "{}", stderr(&out)); + let err = stderr(&out); + assert!(!err.contains("was not used"), "{err}"); + assert!(!err.contains("no access token was resolved"), "{err}"); + } +} From 2df18baaa40b976abf10df9c18c2113ea4ca3e77 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 15:41:30 +0300 Subject: [PATCH 3/9] Send report's CLI token with its Referer A command on the CLI's token now sends cli_token::REFERER, through a referer field on RunFlags that only the CLI-token attempt sets; the user's token, when it stands in, goes without. --debug prints the Referer when there is one. --- src/executor.rs | 10 ++++++++++ src/main.rs | 4 ++++ tests/dry_run.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/src/executor.rs b/src/executor.rs index a5c6759..0ef5fd4 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -107,6 +107,9 @@ pub struct RunFlags { /// it means "nobody said"; [`crate::http::budget`] then falls back to /// whatever default fits the request. pub timeout: Option, + /// Sent as the request's `Referer`, for a token restricted to a URL: + /// the CLI's own ([`crate::cli_token::REFERER`]). `None` for the user's. + pub referer: Option<&'static str>, } /// Runs one generated command — the request it would make, either sent or @@ -142,6 +145,7 @@ fn dispatch( assume_yes, dry_run, timeout, + referer, } = flags; let client = http::client_for(Some(op.service.as_str()))?; @@ -264,6 +268,9 @@ fn dispatch( // request actually went out — never one that only looks like it did. if debug { eprintln!("[debug] {} {}", op.method, redacted_url(&url, &query)); + if let Some(referer) = referer { + eprintln!("[debug] Referer: {referer}"); + } } let method = op.method.as_str(); @@ -293,6 +300,9 @@ fn dispatch( ), )); + if let Some(referer) = referer { + req = req.header(reqwest::header::REFERER, referer); + } if let Some(source) = body_source { req = attach_body(req, source)?; } diff --git a/src/main.rs b/src/main.rs index 3ff9d72..97c376c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1478,6 +1478,7 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - // one in front of `generate-skills`, which sends nothing, // would be a note about a request that is not happening. timeout: http::requested(matches), + referer: None, }; let command = op.command(); @@ -1622,6 +1623,9 @@ fn try_cli_token( } } }; + // Only for the CLI's token: the user's, which may stand in after, goes + // out without it. + flags.referer = Some(cli_token::REFERER); let execute = |token: &str, flags| executor::execute(op, op_matches, Some(token), username, flags, mode); // Sends nothing, so there is nothing to be refused, or to count. diff --git a/tests/dry_run.rs b/tests/dry_run.rs index 219f1bb..c6836f5 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -640,6 +640,51 @@ mod report_token { ); } + /// A request that goes nowhere: the proxy is a dead port, so `--debug` + /// shows what was about to be sent without anything reaching Mapbox. + fn sent_nowhere(config: &Path, cli_token: Option<&str>, args: &[&str]) -> Output { + let mut cmd = command(config); + cmd.env("HTTPS_PROXY", "http://127.0.0.1:9") + .env_remove("https_proxy") + .env_remove("ALL_PROXY") + .env_remove("all_proxy") + .env_remove("NO_PROXY") + .env_remove("no_proxy"); + if let Some(token) = cli_token { + cmd.env("MAPBOX_CLI_TOKEN", token); + } + cmd.args(args).output().expect("run mapbox") + } + + /// The CLI's token is restricted to its URL, so a request with it says + /// it comes from there. + #[test] + fn the_clis_token_goes_with_its_referer() { + let config = config_dir("report-referer"); + let out = sent_nowhere(&config, Some("pk.cli"), &["--debug", "report", "A test"]); + let err = stderr(&out); + assert!( + err.contains("[debug] Referer: https://cli.mapbox.com/"), + "{err}" + ); + } + + /// The user's own token is not restricted to anything of ours, and goes + /// without it. + #[test] + fn the_users_token_goes_without_a_referer() { + let config = config_dir("report-no-referer"); + resting(&config); + let out = sent_nowhere( + &config, + None, + &["--debug", "--token", "sk.mine", "report", "A test"], + ); + let err = stderr(&out); + assert!(err.contains("[debug] POST"), "{err}"); + assert!(!err.contains("Referer"), "{err}"); + } + /// While the CLI's token cannot be used, the user's is: the plan goes /// out with it, and a typed `--token` is not reported as ignored. #[test] From 20550a560ab5c752a8eab5c674f720021094546e Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 15:59:11 +0300 Subject: [PATCH 4/9] Let report fetch afresh instead of resting A command someone is waiting on is never paced. When report's request is refused, for any reason, the token endpoint is asked there and then, past any backoff, and the message is sent again with a different token if there is one; otherwise the user's token stands in. The next run tries the CLI's token again, so a token republished or fixed in the Console works at once. Telemetry keeps the cooldown. --- docs/commands.md | 15 ++-- src/cli_token.rs | 103 ++++++++++++++++++++------ src/cli_token/fetch.rs | 97 ++++++++++++++++++------ src/main.rs | 14 ++-- tests/dry_run.rs | 164 +++++++++++++++++++++++++---------------- 5 files changed, 271 insertions(+), 122 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 5356122..48aa0e6 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2021,11 +2021,14 @@ needed. There is no way to read it back or delete it, so `--dry-run` shows the body without sending it. It is sent with the CLI's own token, not yours, so it works without a -login; `--token` and `--use-login` are noted as unused. While the CLI's -token can't be used — it can't be fetched, it is revoked with no new one -yet, or this API refused it recently — your own token is used instead, and -with none the command fails with `cli_token_unavailable` or -`cli_token_forbidden`, saying when it will try the CLI's again. +login; `--token` and `--use-login` are noted as unused. If this API refuses +the CLI's token, a fresh one is fetched there and then and the message sent +again with it, so a token that was republished or fixed works at once. +When the CLI's token still can't be used — it can't be fetched, it is +revoked with no new one yet, or this API refuses it — your own token is +used instead, and with none the command fails with `cli_token_unavailable` +or `cli_token_forbidden`. Each run tries the CLI's token again; nothing +here is put off for later. #### Parameters @@ -5134,7 +5137,7 @@ faults and are not: | `missing_subcommand` | A command group was named with no operation. | | `cancelled` | A delete was declined at the confirmation prompt. Nothing was sent. | | `cli_token_unavailable` | A command that prefers the CLI's own token (`report`) could not use it — it could not be fetched, or it is revoked and no new one is published yet — and you have no token of your own to fall back to. | -| `cli_token_forbidden` | The CLI's own token works, but this API refused it, now or recently enough that it is not tried again yet, and you have no token of your own to fall back to. | +| `cli_token_forbidden` | The CLI's own token works, but this API refused it, even after fetching it afresh, and you have no token of your own to fall back to. | | `usage` | Anything else clap rejected. Exit code 2, not 1. | | `error` | Unclassified. | diff --git a/src/cli_token.rs b/src/cli_token.rs index fe5b4b7..4e7a5b4 100644 --- a/src/cli_token.rs +++ b/src/cli_token.rs @@ -123,23 +123,22 @@ fn use_for(command: &str) -> Option { /// token already on disk is returned then. pub(crate) fn for_command(command: &str, fetch: bool) -> Option> { match use_for(command)? { - Use::Preferred => Some(preferred_token(command, fetch)), + Use::Preferred => Some(preferred_token(fetch)), } } -fn preferred_token(command: &str, fetch: bool) -> Result { - // A developer's override is not the fetched token, whose cooldown does - // not apply to it. +fn preferred_token(fetch: bool) -> Result { if let Some(token) = usable(std::env::var("MAPBOX_CLI_TOKEN").ok().as_deref()) { return Ok(token.to_owned()); } - if let Some(until) = fetch::resting_until(command) { - return Err(Unavailable::Resting { until }); + if !fetch { + return fetch::saved_token().ok_or(Unavailable::NotFetched); } - if fetch { - fetch::token() - } else { - fetch::saved_token().ok_or(Unavailable::NotFetched) + // A command is not held by the backoff, which paces background fetches: + // someone is waiting, and the endpoint may have a new token by now. + match fetch::token() { + Err(why @ Unavailable::Waiting { .. }) => fetch::token_now().ok_or(why), + answer => answer, } } @@ -199,9 +198,13 @@ pub(crate) fn send( send(token) }; } - match send_with_cli_token(caller, token.to_owned(), &mut Disk, send_cli, |answer| { - answer.as_ref().ok().map(|r| r.status().as_u16()) - }) { + match send_with_cli_token( + caller, + token.to_owned(), + &mut Disk::Background, + send_cli, + |answer| answer.as_ref().ok().map(|r| r.status().as_u16()), + ) { Sent::Answered(answer) | Sent::Refused(answer, _) => answer, } }; @@ -238,21 +241,42 @@ pub(crate) enum Sent { pub(crate) trait Ledger { fn refused(&mut self, caller: &str, token: &str) -> Refusal; fn accepted(&mut self, caller: &str, token: &str); + /// Whether a request refused for `refusal` is worth sending again with + /// a [`fresh`](Ledger::fresh) token. + fn retries(&self, refusal: Refusal) -> bool; fn fresh(&mut self) -> Option; } -/// `/cli-token.json`, through [`fetch`]. -pub(crate) struct Disk; +/// `/cli-token.json`, through [`fetch`], as one of two kinds of +/// caller. +pub(crate) enum Disk { + /// Nobody is waiting, as for telemetry: paced, and a new token is tried + /// only for a dead one, when the backoff allows. + Background, + /// Someone ran it and is waiting: never paced, and any refusal tries a + /// token fetched there and then, so a token republished or fixed in the + /// Console works on the next try. + Command, +} impl Ledger for Disk { fn refused(&mut self, caller: &str, token: &str) -> Refusal { - fetch::refused(caller, token) + fetch::refused(caller, token, matches!(self, Disk::Background)) } fn accepted(&mut self, caller: &str, token: &str) { fetch::accepted(caller, token); } + fn retries(&self, refusal: Refusal) -> bool { + match self { + Disk::Background => refusal == Refusal::Dead, + Disk::Command => true, + } + } fn fresh(&mut self) -> Option { - fetch::token().ok() + match self { + Disk::Background => fetch::token().ok(), + Disk::Command => fetch::token_now(), + } } } @@ -260,10 +284,11 @@ impl Ledger for Disk { /// rules for using it live, for telemetry and commands alike. /// /// A `2xx` ends `caller`'s cooldown. A `401` or `403` is checked with the -/// Tokens API at once ([`fetch::refused`]): a dead token is replaced and the -/// request sent once more, since a refused request was not acted on; any -/// other refusal starts `caller`'s cooldown and is handed back. `status` is -/// an answer's HTTP status, or `None` when there was no answer. +/// Tokens API at once ([`fetch::refused`]); when the ledger says it is worth +/// it ([`Ledger::retries`]) and a different token is to be had, the request +/// is sent once more with it, since a refused request was not acted on. +/// Otherwise the refusal is handed back. `status` is an answer's HTTP status, +/// or `None` when there was no answer. pub(crate) fn send_with_cli_token( caller: &str, mut token: String, @@ -277,7 +302,7 @@ pub(crate) fn send_with_cli_token( match status(&answer) { Some(401 | 403) => { let refusal = ledger.refused(caller, &token); - if refusal == Refusal::Dead && !replaced { + if ledger.retries(refusal) && !replaced { if let Some(fresh) = ledger.fresh().filter(|fresh| *fresh != token) { token = fresh; replaced = true; @@ -681,6 +706,9 @@ mod tests { /// to hand out once, and records what it was told. #[derive(Default)] struct FakeLedger { + /// Retries after any refusal, as [`Disk::Command`] does, rather than + /// only a dead token, as [`Disk::Background`] does. + command: bool, verdict: Option, fresh: Option, refused: Vec, @@ -695,6 +723,9 @@ mod tests { fn accepted(&mut self, _: &str, token: &str) { self.accepted.push(token.to_owned()); } + fn retries(&self, refusal: Refusal) -> bool { + self.command || refusal == Refusal::Dead + } fn fresh(&mut self) -> Option { self.fresh.take() } @@ -773,6 +804,34 @@ mod tests { } } + #[test] + fn a_command_tries_a_fresh_token_after_any_refusal() { + for verdict in [Refusal::Forbidden, Refusal::Unknown, Refusal::NotFetched] { + let mut ledger = FakeLedger { + command: true, + verdict: Some(verdict), + fresh: Some("pk.new".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403), Some(200)]); + assert_eq!(outcome, Sent::Answered(Some(200)), "{verdict:?}"); + assert_eq!(sent, ["pk.old", "pk.new"], "{verdict:?}"); + } + } + + #[test] + fn a_command_gets_the_same_token_back_and_does_not_resend_it() { + let mut ledger = FakeLedger { + command: true, + verdict: Some(Refusal::Forbidden), + fresh: Some("pk.old".to_owned()), + ..FakeLedger::default() + }; + let (outcome, sent) = send_cli(&mut ledger, &[Some(403)]); + assert_eq!(outcome, Sent::Refused(Some(403), Refusal::Forbidden)); + assert_eq!(sent, ["pk.old"]); + } + #[test] fn only_a_2xx_counts_as_accepted() { for status in [Some(200), Some(204), Some(404), Some(500), None] { diff --git a/src/cli_token/fetch.rs b/src/cli_token/fetch.rs index 13fe204..fe97f60 100644 --- a/src/cli_token/fetch.rs +++ b/src/cli_token/fetch.rs @@ -77,9 +77,6 @@ pub(crate) enum Unavailable { Waiting { retry_at: u64, revoked: bool }, /// None on disk, and the caller asked not to fetch, as a dry run does. NotFetched, - /// This caller's API refused the token lately, so it rests from it - /// until `until` (Unix seconds). - Resting { until: u64 }, } impl std::fmt::Display for Unavailable { @@ -97,11 +94,6 @@ impl std::fmt::Display for Unavailable { wait_text(retry_at.saturating_sub(now())) ), Self::NotFetched => write!(f, "it has not been fetched yet"), - Self::Resting { until } => write!( - f, - "this API refused it recently; trying it again in {}", - wait_text(until.saturating_sub(now())) - ), } } } @@ -183,18 +175,16 @@ fn backoff(failures: u32) -> u64 { } } -/// Whether `caller` — `telemetry`, or a command's name — is resting from the +/// Whether `caller` — today only `telemetry` — is resting from the /// CLI's token. While it is, it sends nothing with it, so nothing is /// refused, and nothing is checked. pub(crate) fn cooling(caller: &str) -> bool { - resting_until(caller).is_some() -} - -/// When `caller`'s cooldown ends, while it has one. -pub(crate) fn resting_until(caller: &str) -> Option { - let state = read_state()?; - let cooldown = state.cooldowns.get(caller)?; - holds(cooldown.until, now()).then_some(cooldown.until) + read_state().is_some_and(|state| { + state + .cooldowns + .get(caller) + .is_some_and(|cooldown| holds(cooldown.until, now())) + }) } /// Records that `caller`'s request with the saved `token` went through, which @@ -237,6 +227,20 @@ pub(crate) fn token() -> Result { token.ok_or(why) } +/// [`token`], but asking the endpoint now, whatever the backoff says and +/// even with a token on disk: for a command someone is waiting on whose +/// request was just refused, so that a token republished or reconfigured is +/// picked up at once. `None` when the endpoint has nothing usable. +pub(crate) fn token_now() -> Option { + let mut state = read_state().unwrap_or_default(); + let before = state.clone(); + let token = judge(&mut state, now(), get(&endpoint()), check); + if state != before { + save_fetch(&before, state); + } + token +} + /// The token on disk while it is not dead, without asking the endpoint, for /// a run that must not send anything, such as a dry run. pub(crate) fn saved_token() -> Option { @@ -298,7 +302,11 @@ pub(crate) enum Refusal { /// `token` was refused. A dead token is marked, so the next [`token`] /// fetches a new one; otherwise `caller`'s refusals are counted, and the /// third in a row rests it for an hour. -pub(crate) fn refused(caller: &str, token: &str) -> Refusal { +/// +/// Only a caller that `rests` is paced: one in the background, such as +/// telemetry. A command someone is waiting on is never held back; it tries +/// a fresh token instead ([`token_now`]). +pub(crate) fn refused(caller: &str, token: &str, rests: bool) -> Refusal { let now = now(); if read_state() .filter(|state| state.token.as_deref() == Some(token)) @@ -313,6 +321,8 @@ pub(crate) fn refused(caller: &str, token: &str) -> Refusal { // `verdict` never answers `NotFetched`; that was returned above. if refusal == Refusal::Dead { state.rejected = true; + } else if !rests { + return refusal; } else { let cooldown = state.cooldowns.entry(caller.to_owned()).or_default(); cooldown.failures += 1; @@ -359,17 +369,31 @@ fn acquire( if holds(state.retry_at, now) { return None; } - match get() { + judge(state, now, get(), check) +} + +/// What a fetch's answer does to the state. A token that is new, or the dead +/// one taken back, replaces what is kept, cooldowns and counts with it; the +/// kept token again, not dead, leaves it as it is. +fn judge( + state: &mut State, + now: u64, + fetched: Fetched, + check: impl FnOnce(&str) -> Refusal, +) -> Option { + match fetched { Fetched::Failed => None, Fetched::Token(token) if is_plausible(&token) && (!(state.rejected && state.token.as_deref() == Some(token.as_str())) || check(&token) == Refusal::Forbidden) => { - *state = State { - token: Some(token.clone()), - ..State::default() - }; + if state.rejected || state.token.as_deref() != Some(token.as_str()) { + *state = State { + token: Some(token.clone()), + ..State::default() + }; + } Some(token) } // The rejected token again and still not valid, something that is @@ -655,6 +679,31 @@ mod tests { assert_eq!(state, saved("pk.old", true), "the schedule is untouched"); } + #[test] + fn a_forced_fetch_of_the_kept_token_keeps_what_is_known() { + let mut state = State { + cooldowns: BTreeMap::from([( + "telemetry".to_owned(), + Cooldown { + failures: 1, + until: NOW, + }, + )]), + ..saved("pk.kept", false) + }; + let before = state.clone(); + assert_eq!( + judge(&mut state, NOW, token("pk.kept"), |_| Refusal::Dead), + Some("pk.kept".to_owned()) + ); + assert_eq!(state, before); + assert_eq!( + judge(&mut state, NOW, token("pk.new"), |_| Refusal::Dead), + Some("pk.new".to_owned()) + ); + assert_eq!(state, saved("pk.new", false), "a new token starts over"); + } + #[test] fn a_wait_beyond_an_hour_is_not_honored() { let mut state = State { @@ -754,7 +803,7 @@ mod tests { assert_eq!(wait_text(0), "1 minute"); assert_eq!(wait_text(59 * 60), "59 minutes"); assert_eq!(wait_text(HOUR), "1 hour"); - assert_eq!(wait_text(DAY), "24 hours"); + assert_eq!(wait_text(2 * HOUR), "2 hours"); } #[test] diff --git a/src/main.rs b/src/main.rs index 97c376c..c019ef4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1635,7 +1635,7 @@ fn try_cli_token( let sent = cli_token::send_with_cli_token( command, token, - &mut cli_token::Disk, + &mut cli_token::Disk::Command, |token| { let result = execute(token, flags); // Confirmed once already, if the command asks. @@ -1679,14 +1679,12 @@ fn try_cli_token( } /// Why there was no CLI token to try, as the error a command fails with -/// when the user has no token of their own either. Resting is this API's -/// doing, so it shares the code of the refusal that started it. +/// when the user has no token of their own either. fn cli_token_unavailable(why: &cli_token::Unavailable) -> CliError { - let code = match why { - cli_token::Unavailable::Resting { .. } => "cli_token_forbidden", - _ => "cli_token_unavailable", - }; - CliError::new(code, format!("Can't use the CLI's token: {why}.")) + CliError::new( + "cli_token_unavailable", + format!("Can't use the CLI's token: {why}."), + ) } #[cfg(test)] diff --git a/tests/dry_run.rs b/tests/dry_run.rs index c6836f5..a562ea3 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -574,21 +574,18 @@ fn report_needs_a_message_and_takes_no_data() { /// is a dead port, and a cooldown is written to disk. mod report_token { use super::*; + use std::io::{Read, Write}; + use std::net::TcpListener; - /// A saved CLI token whose cooldown for `report` runs for another hour. - fn resting(config: &Path) { - let until = std::time::SystemTime::now() + fn now() -> u64 { + std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .expect("a clock after 1970") .as_secs() - + 3600; - std::fs::write( - config.join("cli-token.json"), - format!( - r#"{{"token":"pk.saved","overridden":true,"cooldowns":{{"report":{{"failures":1,"until":{until}}}}}}}"# - ), - ) - .expect("write the state"); + } + + fn write_state(config: &Path, state: &str) { + std::fs::write(config.join("cli-token.json"), state).expect("write the state"); } fn error_code(out: &Output) -> String { @@ -598,6 +595,47 @@ mod report_token { .to_owned() } + /// A token endpoint on loopback that hands out `token` once. + fn token_endpoint(token: &'static str) -> 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 || { + let Ok((mut stream, _)) = listener.accept() else { + return; + }; + let mut head = Vec::new(); + let mut byte = [0u8; 1]; + while !head.ends_with(b"\r\n\r\n") { + match stream.read(&mut byte) { + Ok(1) => head.push(byte[0]), + _ => break, + } + } + let body = format!(r#"{{"token":"{token}"}}"#); + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n\ + Content-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + }); + format!("http://{addr}/config/token.json") + } + + /// A request that goes nowhere: the proxy is a dead port, so `--debug` + /// shows what was about to be sent without anything reaching Mapbox. + fn sent_nowhere(config: &Path, args: &[&str]) -> Command { + let mut cmd = command(config); + cmd.env("HTTPS_PROXY", "http://127.0.0.1:9") + .env_remove("https_proxy") + .env_remove("ALL_PROXY") + .env_remove("all_proxy") + .env_remove("NO_PROXY") + .env_remove("no_proxy") + .args(args); + cmd + } + #[test] fn with_no_token_at_all_it_says_the_clis_is_unavailable() { let config = config_dir("report-no-tokens"); @@ -607,90 +645,92 @@ mod report_token { assert_eq!(error_code(&out), "cli_token_unavailable"); } + /// The CLI's token is restricted to its URL, so a request with it says + /// it comes from there. #[test] - fn while_resting_with_no_token_it_says_the_api_refused_it() { - let config = config_dir("report-resting"); - resting(&config); - let out = run(&config, &["-o", "json", "report", "A test"]); - assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); - assert_eq!(error_code(&out), "cli_token_forbidden"); + fn the_clis_token_goes_with_its_referer() { + let config = config_dir("report-referer"); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .env("MAPBOX_CLI_TOKEN", "pk.cli") + .output() + .expect("run mapbox"); + let err = stderr(&out); assert!( - stderr(&out).contains("refused it recently"), - "{}", - stderr(&out) + err.contains("[debug] Referer: https://cli.mapbox.com/"), + "{err}" ); } - /// `MAPBOX_CLI_TOKEN` is a developer's, not the fetched token, so the - /// fetched token's cooldown does not hold it back. + /// Someone ran it, so a rest telemetry might take does not hold it back: + /// it sends with the CLI's token. #[test] - fn a_cooldown_does_not_hold_back_the_override() { - let config = config_dir("report-resting-override"); - resting(&config); - let out = command(&config) - .env("MAPBOX_CLI_TOKEN", "pk.cli") - .args(["--token", "sk.mine", "report", "--dry-run", "A test"]) + fn a_cooldown_does_not_hold_report_back() { + let config = config_dir("report-not-resting"); + write_state( + &config, + &format!( + r#"{{"token":"pk.saved","overridden":true,"cooldowns":{{"report":{{"failures":3,"until":{}}}}}}}"#, + now() + 3600 + ), + ); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) .output() .expect("run mapbox"); - assert!(out.status.success(), "{}", stderr(&out)); + let err = stderr(&out); assert!( - stderr(&out).contains("`--token` was not used"), - "{}", - stderr(&out) + err.contains("[debug] Referer: https://cli.mapbox.com/"), + "{err}" ); + assert!(!err.contains("recently"), "{err}"); } - /// A request that goes nowhere: the proxy is a dead port, so `--debug` - /// shows what was about to be sent without anything reaching Mapbox. - fn sent_nowhere(config: &Path, cli_token: Option<&str>, args: &[&str]) -> Output { - let mut cmd = command(config); - cmd.env("HTTPS_PROXY", "http://127.0.0.1:9") - .env_remove("https_proxy") - .env_remove("ALL_PROXY") - .env_remove("all_proxy") - .env_remove("NO_PROXY") - .env_remove("no_proxy"); - if let Some(token) = cli_token { - cmd.env("MAPBOX_CLI_TOKEN", token); - } - cmd.args(args).output().expect("run mapbox") - } - - /// The CLI's token is restricted to its URL, so a request with it says - /// it comes from there. + /// Nor does the endpoint's backoff: with the kept token dead, it asks + /// for a new one there and then, and sends with it. #[test] - fn the_clis_token_goes_with_its_referer() { - let config = config_dir("report-referer"); - let out = sent_nowhere(&config, Some("pk.cli"), &["--debug", "report", "A test"]); + fn report_fetches_through_the_backoff() { + let config = config_dir("report-through-backoff"); + write_state( + &config, + &format!( + r#"{{"token":"pk.dead","rejected":true,"overridden":true,"failures":3,"retry_at":{}}}"#, + now() + 3600 + ), + ); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .env("MAPBOX_INTERNAL_CLI_TOKEN_URL", token_endpoint("pk.fresh")) + .output() + .expect("run mapbox"); let err = stderr(&out); assert!( err.contains("[debug] Referer: https://cli.mapbox.com/"), "{err}" ); + let state = std::fs::read_to_string(config.join("cli-token.json")).expect("the state"); + assert!(state.contains(r#""token":"pk.fresh""#), "{state}"); } /// The user's own token is not restricted to anything of ours, and goes - /// without it. + /// without the CLI token's Referer when it stands in. #[test] fn the_users_token_goes_without_a_referer() { let config = config_dir("report-no-referer"); - resting(&config); let out = sent_nowhere( &config, - None, &["--debug", "--token", "sk.mine", "report", "A test"], - ); + ) + .output() + .expect("run mapbox"); let err = stderr(&out); + assert!(err.contains("Using your own token instead"), "{err}"); assert!(err.contains("[debug] POST"), "{err}"); assert!(!err.contains("Referer"), "{err}"); } - /// While the CLI's token cannot be used, the user's is: the plan goes - /// out with it, and a typed `--token` is not reported as ignored. + /// A dry run fetches nothing, so with no CLI token kept the plan goes out + /// with the user's, and a typed `--token` is not reported as ignored. #[test] - fn while_resting_a_dry_run_plans_with_the_users_token() { - let config = config_dir("report-resting-user"); - resting(&config); + fn a_dry_run_with_no_kept_token_plans_with_the_users() { + let config = config_dir("report-dry-user"); let out = run( &config, &["--token", "sk.mine", "report", "--dry-run", "A test"], From c3b5690cee2f9313bd5e84fe692b200b1628a302 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 16:00:47 +0300 Subject: [PATCH 5/9] Show what report sent and its id, nothing else Under -o text, a successful report says it was sent and shows the message and its id. The timestamps, the always-received status, the 0,0 place and the screenshot flag are the Feedback API's record, which the sender neither chose nor can act on. -o json still prints the answer whole. --- src/output/render.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/output/render.rs b/src/output/render.rs index eda817b..6727670 100644 --- a/src/output/render.rs +++ b/src/output/render.rs @@ -33,10 +33,39 @@ pub(super) fn list_rendering(value: &Value, service: Option<&str>) -> Option tilequery_feature_rows(value).map(|rows| render_feature_list(&rows)), + Some("report") => render_report(value), _ => None, } } +/// `report`'s answer, as what the sender can use: that it went, what was +/// sent, and the id to quote. The rest is the Feedback API's record of it — +/// timestamps, a status that is always `received`, the `0,0` this command +/// always sends, a screenshot it never attaches — none of which the sender +/// chose or can act on. `-o json` still has all of it. +/// +/// `None` for an answer without both, which then falls back to the whole +/// field list rather than hiding what came back. +fn render_report(value: &Value) -> Option { + const SENT: &str = "Sent to the Mapbox CLI team."; + let map = value.as_object()?; + let fields = vec![ + ( + "feedback".to_owned(), + map.get("feedback")?.as_str()?.to_owned(), + ), + ("id".to_owned(), map.get("id")?.as_str()?.to_owned()), + ]; + Some(Rendered { + text: format!("{SENT}\n{}", field_lines(&fields, false)), + header: false, + fields: None, + colored: Some(format!("{SENT}\n{}", field_lines(&fields, true))), + shortened: false, + identifier: None, + }) +} + /// A human rendering, what it had to cut, and the first identifier in it. pub(super) struct Rendered { pub(super) text: String, @@ -2327,4 +2356,43 @@ mod tests { Some("Food and Drink".to_string()) ); } + + #[test] + fn a_report_shows_what_was_sent_and_its_id_alone() { + let answer = serde_json::json!({ + "id": "01a15433-408f-708c-929f-cad22dd7ef88", + "created_at": "2026-10-19T12:46:47.466Z", + "updated_at": "2026-10-19T12:46:47.466Z", + "received_at": "2026-10-19T12:46:47.466Z", + "feedback": "test from CLI", + "category": "general", + "status": "received", + "location": { "lat": 0, "lon": 0 }, + "has_screenshot": false + }); + let rendered = list_rendering(&answer, Some("report")).expect("a rendering"); + let lines: Vec<&str> = rendered.text.lines().collect(); + assert_eq!(lines[0], "Sent to the Mapbox CLI team."); + assert_eq!(lines.len(), 3, "{}", rendered.text); + assert!(lines[1].starts_with("feedback") && lines[1].ends_with("test from CLI")); + assert!( + lines[2].starts_with("id") + && lines[2].ends_with("01a15433-408f-708c-929f-cad22dd7ef88") + ); + for hidden in [ + "created_at", + "status", + "location", + "has_screenshot", + "category", + ] { + assert!(!rendered.text.contains(hidden), "{hidden} shown"); + } + } + + #[test] + fn a_report_answer_missing_its_id_is_shown_whole() { + let answer = serde_json::json!({ "feedback": "test", "status": "received" }); + assert!(list_rendering(&answer, Some("report")).is_none()); + } } From 215a2ff5558093702ac7cf277a29afcdc859a5ee Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 17:16:45 +0300 Subject: [PATCH 6/9] Address review on mapbox report A typed --token or --use-login now sends with the user's token, and the CLI's is not tried. MAPBOX_CLI_TOKEN is sent once and never checked, counted or replaced, as telemetry treats it. A command uses the token on disk or fetches past the backoff, and a miss on that fetch is not counted, so report no longer lengthens telemetry's backoff or reports a wait it never took. A token fetched this run is not fetched again after a refusal. An unchecked refusal is cli_token_forbidden, not cli_token_unavailable. Also: the repository URL is built from one macro, body fields are no longer cloned per request, and the report Outputs are captured live. --- CHANGELOG.md | 3 +- README.md | 3 +- docs/commands.md | 28 ++++++++-- src/cli_token.rs | 110 ++++++++++++++++++++++++++++++++------- src/cli_token/fetch.rs | 114 +++++++++++++---------------------------- src/executor.rs | 15 +++--- src/main.rs | 51 +++++++++--------- src/remedy.rs | 2 +- tests/dry_run.rs | 69 ++++++++++++++++++++++--- 9 files changed, 252 insertions(+), 143 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d4b413..665979d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,8 @@ that may never merge. They are not releases and are not listed here. - New `mapbox report ""` sends feedback about the CLI itself to the Mapbox CLI team. It is sent with the CLI's own token, so it needs no login, no `user-feedback:write` scope, and files nothing under your own - account; your token stands in only while the CLI's can't be used. + account; your token stands in only while the CLI's can't be used, or + when you pass `--token` or `--use-login`. `--category` is optional and defaults to `general`. Two new error codes say why the CLI's token couldn't be used when you have no token either: `cli_token_unavailable` and `cli_token_forbidden` (see diff --git a/README.md b/README.md index 5e7dbec..67e2223 100644 --- a/README.md +++ b/README.md @@ -332,7 +332,8 @@ mapbox report "tilesets query hangs behind my proxy" --category bug The message goes to the Mapbox CLI team, not to your account, so `mapbox feedback list` won't show it. It is sent with the CLI's own token, -so it needs no login; yours stands in only while the CLI's can't be used. See +so it needs no login; yours stands in while the CLI's can't be used, or +when you pass `--token` or `--use-login`. See [Report](./docs/commands.md#report). ### Shell completion diff --git a/docs/commands.md b/docs/commands.md index 48aa0e6..bd7c35d 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2021,9 +2021,11 @@ needed. There is no way to read it back or delete it, so `--dry-run` shows the body without sending it. It is sent with the CLI's own token, not yours, so it works without a -login; `--token` and `--use-login` are noted as unused. If this API refuses +login. A typed `--token` or `--use-login` sends with yours instead, and the +CLI's is not tried. If this API refuses the CLI's token, a fresh one is fetched there and then and the message sent again with it, so a token that was republished or fixed works at once. +A `MAPBOX_CLI_TOKEN` override is sent as it is and never replaced. When the CLI's token still can't be used — it can't be fetched, it is revoked with no new one yet, or this API refuses it — your own token is used instead, and with none the command fails with `cli_token_unavailable` @@ -2052,7 +2054,27 @@ mapbox report "…" --dry-run #### Outputs -TODO(capture): live output pending. +Captured live on 2026-10-19. `-o text` shows only that it was sent, the +message and its id; `-o json` is the answer as the API sent it: + + + + +
Terminal — -o textAgent — -o json
+ +```text +Sent to the Mapbox CLI team. +feedback Test from mapbox-cli development (report). Please ignore. +id 01a15484-6d9b-744c-bea4-a9a0a99b95c5 +``` + + + +```json +{"category":"general","created_at":"2026-10-19T14:15:32.069Z","feedback":"Test from mapbox-cli development (report). Please ignore.","has_screenshot":false,"id":"01a15484-7fc9-7328-a04d-251340f49f64","location":{"lat":0,"lon":0},"received_at":"2026-10-19T14:15:32.069Z","status":"received","updated_at":"2026-10-19T14:15:32.069Z"} +``` + +
--- @@ -5137,7 +5159,7 @@ faults and are not: | `missing_subcommand` | A command group was named with no operation. | | `cancelled` | A delete was declined at the confirmation prompt. Nothing was sent. | | `cli_token_unavailable` | A command that prefers the CLI's own token (`report`) could not use it — it could not be fetched, or it is revoked and no new one is published yet — and you have no token of your own to fall back to. | -| `cli_token_forbidden` | The CLI's own token works, but this API refused it, even after fetching it afresh, and you have no token of your own to fall back to. | +| `cli_token_forbidden` | This API refused the CLI's own token, even after fetching it afresh, and you have no token of your own to fall back to. | | `usage` | Anything else clap rejected. Exit code 2, not 1. | | `error` | Unclassified. | diff --git a/src/cli_token.rs b/src/cli_token.rs index 4e7a5b4..c7287b5 100644 --- a/src/cli_token.rs +++ b/src/cli_token.rs @@ -117,29 +117,67 @@ fn use_for(command: &str) -> Option { .map(|(_, how, _)| *how) } +/// The CLI's token as a command got it, for [`send_for_command`], which +/// handles a refusal of it by where it came from. +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct CommandToken { + pub(crate) token: String, + source: TokenSource, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TokenSource { + /// `MAPBOX_CLI_TOKEN`. + Override, + /// The one on disk. + Saved, + /// Fetched by this run. + Fetched, +} + /// The CLI's token for `command`, or `None` when the command is not listed /// for it: the only way to ask, so an unlisted command never gets one. /// `fetch` is false where nothing may be sent, as in a dry run, and only a /// token already on disk is returned then. -pub(crate) fn for_command(command: &str, fetch: bool) -> Option> { +pub(crate) fn for_command(command: &str, fetch: bool) -> Option> { match use_for(command)? { Use::Preferred => Some(preferred_token(fetch)), } } -fn preferred_token(fetch: bool) -> Result { +fn preferred_token(fetch: bool) -> Result { + let given = |token: String, source| CommandToken { token, source }; if let Some(token) = usable(std::env::var("MAPBOX_CLI_TOKEN").ok().as_deref()) { - return Ok(token.to_owned()); + return Ok(given(token.to_owned(), TokenSource::Override)); + } + if let Some(token) = fetch::saved_token() { + return Ok(given(token, TokenSource::Saved)); } if !fetch { - return fetch::saved_token().ok_or(Unavailable::NotFetched); + return Err(Unavailable::NotFetched); } // A command is not held by the backoff, which paces background fetches: // someone is waiting, and the endpoint may have a new token by now. - match fetch::token() { - Err(why @ Unavailable::Waiting { .. }) => fetch::token_now().ok_or(why), - answer => answer, - } + fetch::token_now().map(|token| given(token, TokenSource::Fetched)) +} + +/// Sends a command's request with the CLI's token by [`send_with_cli_token`]'s +/// rules, as [`Disk::Command`] for a token from disk or the endpoint. +/// `MAPBOX_CLI_TOKEN` is sent once and never checked, counted or replaced, +/// as [`send`] treats it: a developer chose that token, and a refusal of it +/// is theirs to see. +pub(crate) fn send_for_command( + command: &str, + given: CommandToken, + send: impl FnMut(&str) -> R, + status: impl Fn(&R) -> Option, +) -> Sent { + let mut ledger = match given.source { + TokenSource::Override => Disk::Unchecked, + TokenSource::Saved => Disk::Command { fetched: false }, + TokenSource::Fetched => Disk::Command { fetched: true }, + }; + send_with_cli_token(command, given.token, &mut ledger, send, status) } /// The token the user gave this run, ranked as a command ranks it: with @@ -210,7 +248,7 @@ pub(crate) fn send( }; let fetch = || { // A caller resting from the CLI's token sends nothing with it. - let token = (!fetch::cooling(caller)).then(|| fetch::token().ok())??; + let token = (!fetch::cooling(caller)).then(fetch::token)??; *fetched.borrow_mut() = Some(token.clone()); Some(token) }; @@ -247,35 +285,46 @@ pub(crate) trait Ledger { fn fresh(&mut self) -> Option; } -/// `/cli-token.json`, through [`fetch`], as one of two kinds of -/// caller. +/// `/cli-token.json`, through [`fetch`], by kind of caller. pub(crate) enum Disk { /// Nobody is waiting, as for telemetry: paced, and a new token is tried /// only for a dead one, when the backoff allows. Background, /// Someone ran it and is waiting: never paced, and any refusal tries a /// token fetched there and then, so a token republished or fixed in the - /// Console works on the next try. - Command, + /// Console works on the next try. Not when this run `fetched` the token + /// already: asking again at once would only get the same one back. + Command { fetched: bool }, + /// A token not from the endpoint (`MAPBOX_CLI_TOKEN`): the file is not + /// touched, and a refusal is handed back as it is. + Unchecked, } impl Ledger for Disk { fn refused(&mut self, caller: &str, token: &str) -> Refusal { - fetch::refused(caller, token, matches!(self, Disk::Background)) + match self { + Disk::Background => fetch::refused(caller, token, true), + Disk::Command { .. } => fetch::refused(caller, token, false), + Disk::Unchecked => Refusal::NotFetched, + } } fn accepted(&mut self, caller: &str, token: &str) { - fetch::accepted(caller, token); + if !matches!(self, Disk::Unchecked) { + fetch::accepted(caller, token); + } } fn retries(&self, refusal: Refusal) -> bool { match self { Disk::Background => refusal == Refusal::Dead, - Disk::Command => true, + Disk::Command { fetched } => !fetched, + Disk::Unchecked => false, } } fn fresh(&mut self) -> Option { match self { - Disk::Background => fetch::token().ok(), - Disk::Command => fetch::token_now(), + Disk::Background => fetch::token(), + Disk::Command { .. } => fetch::token_now().ok(), + Disk::Unchecked => None, } } } @@ -832,6 +881,31 @@ mod tests { assert_eq!(sent, ["pk.old"]); } + #[test] + fn a_command_token_just_fetched_or_overridden_is_not_replaced() { + for (ledger, retries) in [ + (Disk::Command { fetched: false }, true), + (Disk::Command { fetched: true }, false), + (Disk::Unchecked, false), + ] { + for refusal in [ + Refusal::Dead, + Refusal::Forbidden, + Refusal::Unknown, + Refusal::NotFetched, + ] { + assert_eq!(ledger.retries(refusal), retries, "{refusal:?}"); + } + } + } + + #[test] + fn an_override_is_never_checked_or_replaced() { + let mut ledger = Disk::Unchecked; + assert_eq!(ledger.refused("report", "pk.override"), Refusal::NotFetched); + assert_eq!(ledger.fresh(), None); + } + #[test] fn only_a_2xx_counts_as_accepted() { for status in [Some(200), Some(204), Some(404), Some(500), None] { diff --git a/src/cli_token/fetch.rs b/src/cli_token/fetch.rs index fe97f60..167e28e 100644 --- a/src/cli_token/fetch.rs +++ b/src/cli_token/fetch.rs @@ -71,10 +71,10 @@ const MAX_TOKEN_LEN: usize = 512; pub(crate) enum Unavailable { /// The endpoint could not be reached. Asked again on the next run. Unreachable, - /// The endpoint answered without a usable token, so it is not asked - /// again before `retry_at` (Unix seconds). `revoked` when there is a - /// dead token it has not replaced yet, rather than never a token at all. - Waiting { retry_at: u64, revoked: bool }, + /// The endpoint answered without a usable token. `revoked` when there + /// is a dead token it has not replaced yet, rather than never a token at + /// all. + NoToken { revoked: bool }, /// None on disk, and the caller asked not to fetch, as a dry run does. NotFetched, } @@ -83,16 +83,12 @@ impl std::fmt::Display for Unavailable { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::Unreachable => write!(f, "the token endpoint could not be reached"), - Self::Waiting { retry_at, revoked } => write!( - f, - "{}; trying again in {}", - if *revoked { - "it is revoked and no new one is published yet" - } else { - "the token endpoint did not return a usable token" - }, - wait_text(retry_at.saturating_sub(now())) - ), + Self::NoToken { revoked: true } => { + write!(f, "it is revoked and no new one is published yet") + } + Self::NoToken { revoked: false } => { + write!(f, "the token endpoint did not return a usable token") + } Self::NotFetched => write!(f, "it has not been fetched yet"), } } @@ -198,47 +194,39 @@ pub(crate) fn accepted(caller: &str, token: &str) { } /// The CLI's token: the one on disk while it is not dead, otherwise a fresh -/// one from the endpoint unless it is being waited for. -pub(crate) fn token() -> Result { +/// one from the endpoint unless it is being waited for. `None` when there is +/// none to be had right now. +pub(crate) fn token() -> Option { let mut state = read_state().unwrap_or_default(); let before = state.clone(); - let mut unreachable = false; - let token = acquire( - &mut state, - now(), - || { - let fetched = get(&endpoint()); - unreachable = matches!(fetched, Fetched::Failed); - fetched - }, - check, - ); - let why = if unreachable { - Unavailable::Unreachable - } else { - Unavailable::Waiting { - retry_at: state.retry_at, - revoked: state.rejected, - } - }; + let token = acquire(&mut state, now(), || get(&endpoint()), check); if state != before { save_fetch(&before, state); } - token.ok_or(why) + token } /// [`token`], but asking the endpoint now, whatever the backoff says and -/// even with a token on disk: for a command someone is waiting on whose -/// request was just refused, so that a token republished or reconfigured is -/// picked up at once. `None` when the endpoint has nothing usable. -pub(crate) fn token_now() -> Option { +/// even with a token on disk: for a command someone is waiting on, so that a +/// token republished or reconfigured is picked up at once. +/// +/// Only a token it gets is saved. A miss is not counted: the backoff paces +/// background fetches, and a command retried by hand must not lengthen it. +pub(crate) fn token_now() -> Result { let mut state = read_state().unwrap_or_default(); let before = state.clone(); - let token = judge(&mut state, now(), get(&endpoint()), check); + let fetched = get(&endpoint()); + let why = match fetched { + Fetched::Failed => Unavailable::Unreachable, + _ => Unavailable::NoToken { + revoked: state.rejected, + }, + }; + let token = judge(&mut state, now(), fetched, check).ok_or(why)?; if state != before { save_fetch(&before, state); } - token + Ok(token) } /// The token on disk while it is not dead, without asking the endpoint, for @@ -495,22 +483,6 @@ fn now() -> u64 { .unwrap_or_default() } -fn wait_text(seconds: u64) -> String { - let minutes = seconds.div_ceil(60).max(1); - match minutes { - 1 => "1 minute".to_owned(), - m if m < 60 => format!("{m} minutes"), - m => { - let hours = m.div_ceil(60); - if hours == 1 { - "1 hour".to_owned() - } else { - format!("{hours} hours") - } - } - } -} - #[cfg(test)] mod tests { use super::*; @@ -778,34 +750,20 @@ mod tests { } #[test] - fn a_wait_says_whether_a_token_was_ever_revoked() { - let waiting = |revoked| { - Unavailable::Waiting { - retry_at: 0, - revoked, - } - .to_string() - }; + fn no_token_says_whether_one_was_ever_revoked() { + let reason = |revoked| Unavailable::NoToken { revoked }.to_string(); assert!( - waiting(true).starts_with("it is revoked"), + reason(true).starts_with("it is revoked"), "{}", - waiting(true) + reason(true) ); assert!( - waiting(false).starts_with("the token endpoint did not return"), + reason(false).starts_with("the token endpoint did not return"), "{}", - waiting(false) + reason(false) ); } - #[test] - fn the_wait_reads_in_minutes_or_hours() { - assert_eq!(wait_text(0), "1 minute"); - assert_eq!(wait_text(59 * 60), "59 minutes"); - assert_eq!(wait_text(HOUR), "1 hour"); - assert_eq!(wait_text(2 * HOUR), "2 hours"); - } - #[test] fn only_a_dead_token_is_replaced() { for code in [ diff --git a/src/executor.rs b/src/executor.rs index 0ef5fd4..66b24ef 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -216,14 +216,12 @@ fn dispatch( let data = data_argument .as_ref() .map(|argument| argument.body.as_ref()); - let body_fields: Vec = op + let body_fields = op .body_fields .iter() .chain(&op.body_positional) - .chain(&op.fixed_body_fields) - .cloned() - .collect(); - let merged_body = merge_body_fields(&body_fields, matches, data)?; + .chain(&op.fixed_body_fields); + let merged_body = merge_body_fields(body_fields, matches, data)?; let variadic_body = variadic_body_array_json(&op.variadic_body_array, matches); let data = variadic_body.as_deref().or(merged_body.as_deref()).or(data); let files: Vec<&str> = matches @@ -1019,12 +1017,13 @@ fn variadic_body_array_json( /// `--data` with the body-field flags set on it, or `None` if nothing was /// set. A flag wins over `--data`; a default fills only what neither set. -fn merge_body_fields( - fields: &[Parameter], +fn merge_body_fields<'a>( + fields: impl IntoIterator, matches: &ArgMatches, data: Option<&str>, ) -> Result> { - if fields.is_empty() { + let mut fields = fields.into_iter().peekable(); + if fields.peek().is_none() { return Ok(None); } diff --git a/src/main.rs b/src/main.rs index c019ef4..361c5fd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -7,6 +7,15 @@ use clap::builder::{ use clap::error::{ContextKind, ContextValue, ErrorKind}; use clap::{Arg, ArgAction, ArgMatches, Command}; +/// The repository's URL, as a macro so that `concat!` can build links on it +/// in a `const`, such as `remedy`'s table. Defined ahead of the modules, +/// which see it only from here on. +macro_rules! repo_url { + () => { + "https://github.com/mapbox/cli" + }; +} + mod account_usage; mod agent_detect; mod agent_skills; @@ -56,7 +65,7 @@ use spec::ServiceSpec; /// The two installers keep a copy of their own each. They are downloaded and /// run on their own, with nothing of this crate beside them, so this is the /// single source for the Rust half rather than for all three. -const REPO_URL: &str = "https://github.com/mapbox/cli"; +const REPO_URL: &str = repo_url!(); /// Opens the top-level help when an agent is driving the CLI. Agents explore /// by chaining `--help` calls, one command at a time; `--schema` answers the @@ -1483,9 +1492,15 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - let command = op.command(); // Ahead of the user's credentials, so a run the CLI's token - // serves never refreshes a login it will not send. + // serves never refreshes a login it will not send. A typed + // `--token` or `--use-login` says which token to send, so the + // CLI's is not tried at all. let mut cli_failure = None; - if let Some(cli) = cli_token::for_command(&command, !dry_run) { + let chose_own_token = use_login || auth::typed_token(matches).is_some(); + let cli = (!chose_own_token) + .then(|| cli_token::for_command(&command, !dry_run)) + .flatten(); + if let Some(cli) = cli { let username = matches.get_one::("username").cloned(); let attempt = try_cli_token( op, @@ -1497,24 +1512,7 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - mode, ); match attempt { - CliAttempt::Done(result) => { - // Whatever the result: a failure is when someone most - // needs to know which token was sent. - let ignored = if auth::typed_token(matches).is_some() { - Some("`--token`") - } else if use_login { - Some("`--use-login`") - } else { - None - }; - if let Some(choice) = ignored { - output::progress(&format!( - "{choice} was not used: this command sends with the CLI's \ - own token while it works." - )); - } - return result; - } + CliAttempt::Done(result) => return result, CliAttempt::Unusable { error, sent } => { // The user already confirmed the request that was // refused; sending it again with their own token is @@ -1609,7 +1607,7 @@ fn try_cli_token( op: &spec::Operation, op_matches: &clap::ArgMatches, command: &str, - cli: Result, + cli: Result, username: Option<&str>, mut flags: executor::RunFlags, mode: output::Mode, @@ -1630,12 +1628,11 @@ fn try_cli_token( |token: &str, flags| executor::execute(op, op_matches, Some(token), username, flags, mode); // Sends nothing, so there is nothing to be refused, or to count. if flags.dry_run { - return CliAttempt::Done(execute(&token, flags)); + return CliAttempt::Done(execute(&token.token, flags)); } - let sent = cli_token::send_with_cli_token( + let sent = cli_token::send_for_command( command, token, - &mut cli_token::Disk::Command, |token| { let result = execute(token, flags); // Confirmed once already, if the command asks. @@ -1664,8 +1661,10 @@ fn try_cli_token( "cli_token_forbidden", "This API refused the CLI's token, and checking why failed.", ), + // Refused, and not checked why: `MAPBOX_CLI_TOKEN`, or the token on + // disk changed meanwhile or could not be written. cli_token::Refusal::NotFetched => { - ("cli_token_unavailable", "This API refused the CLI's token.") + ("cli_token_forbidden", "This API refused the CLI's token.") } }; let request_id = result diff --git a/src/remedy.rs b/src/remedy.rs index 5a3acb7..506961b 100644 --- a/src/remedy.rs +++ b/src/remedy.rs @@ -99,7 +99,7 @@ const SERVICE_DOCS: &[(&str, &str)] = &[ // this repository's own page is the only one. ( "report", - "https://github.com/mapbox/cli/blob/main/docs/commands.md#mapbox-report", + concat!(repo_url!(), "/blob/main/docs/commands.md#mapbox-report"), ), ("search", "https://docs.mapbox.com/api/search/search-box/"), // Static Images and Static Tiles merged into one `static` command group diff --git a/tests/dry_run.rs b/tests/dry_run.rs index a562ea3..68f1c28 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -709,35 +709,90 @@ mod report_token { assert!(state.contains(r#""token":"pk.fresh""#), "{state}"); } + /// A command fetches past the backoff, so the reason it gives is what + /// that fetch found, not the backoff's wait, which it never waited for. + #[test] + fn an_unreachable_endpoint_is_reported_through_the_backoff() { + let config = config_dir("report-unreachable-backoff"); + write_state( + &config, + &format!( + r#"{{"overridden":true,"failures":3,"retry_at":{}}}"#, + now() + 3600 + ), + ); + let out = run(&config, &["-o", "json", "report", "A test"]); + assert_eq!(error_code(&out), "cli_token_unavailable"); + let err = stderr(&out); + assert!(err.contains("could not be reached"), "{err}"); + assert!(!err.contains("trying again"), "{err}"); + } + + /// A miss on a command's fetch does not count toward the backoff, which + /// paces telemetry's fetches in the background. + #[test] + fn a_commands_miss_leaves_the_backoff_alone() { + let config = config_dir("report-miss-uncounted"); + let state = r#"{"overridden":true,"failures":2}"#; + write_state(&config, state); + let out = command(&config) + .env( + "MAPBOX_INTERNAL_CLI_TOKEN_URL", + token_endpoint("not-a-token"), + ) + .args(["-o", "json", "report", "A test"]) + .output() + .expect("run mapbox"); + assert_eq!(error_code(&out), "cli_token_unavailable"); + let after = std::fs::read_to_string(config.join("cli-token.json")).expect("the state"); + assert_eq!(after, state); + } + /// The user's own token is not restricted to anything of ours, and goes /// without the CLI token's Referer when it stands in. #[test] fn the_users_token_goes_without_a_referer() { let config = config_dir("report-no-referer"); + let out = sent_nowhere(&config, &["--debug", "report", "A test"]) + .env("MAPBOX_ACCESS_TOKEN", "sk.mine") + .output() + .expect("run mapbox"); + let err = stderr(&out); + assert!(err.contains("Using your own token instead"), "{err}"); + assert!(err.contains("[debug] POST"), "{err}"); + assert!(!err.contains("Referer"), "{err}"); + } + + /// A typed `--token` is the user's choice for this run, so the CLI's + /// token is not tried even when there is one. + #[test] + fn a_typed_token_outranks_the_clis() { + let config = config_dir("report-typed-token"); let out = sent_nowhere( &config, &["--debug", "--token", "sk.mine", "report", "A test"], ) + .env("MAPBOX_CLI_TOKEN", "pk.cli") .output() .expect("run mapbox"); let err = stderr(&out); - assert!(err.contains("Using your own token instead"), "{err}"); assert!(err.contains("[debug] POST"), "{err}"); assert!(!err.contains("Referer"), "{err}"); + assert!(!err.contains("CLI's token"), "{err}"); } /// A dry run fetches nothing, so with no CLI token kept the plan goes out - /// with the user's, and a typed `--token` is not reported as ignored. + /// with the user's. #[test] fn a_dry_run_with_no_kept_token_plans_with_the_users() { let config = config_dir("report-dry-user"); - let out = run( - &config, - &["--token", "sk.mine", "report", "--dry-run", "A test"], - ); + let out = command(&config) + .env("MAPBOX_ACCESS_TOKEN", "sk.mine") + .args(["report", "--dry-run", "A test"]) + .output() + .expect("run mapbox"); assert!(out.status.success(), "{}", stderr(&out)); let err = stderr(&out); - assert!(!err.contains("was not used"), "{err}"); assert!(!err.contains("no access token was resolved"), "{err}"); } } From 9a2e7b9ad42f7c707207ac04b94a08ef633535a6 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 19:03:13 +0300 Subject: [PATCH 7/9] Address second review on mapbox report A report now ends with the CLI's version and OS, appended to the message since the Feedback API has no field for them. A 429 on the CLI's token falls back to the user's token, as 401/403 do: every install shares that token, and a 429 was not acted on. With no token of their own, the 429 is the answer. An empty or blank message is refused, and a message starting with `-` gets a tip that quotes it whole and says to put it after `--`. The dry run shows the body and which token it would use under `-o text`, and no longer says "from --data". When the CLI's token can't be used and there is no other, the error explains what that token is and points at `mapbox auth login` or `--token`. The help text, README and changelog now say the CLI's token goes first, that the user's token is on the request after a fallback, and that the version and OS are sent. --- CHANGELOG.md | 10 +- README.md | 18 ++- custom-openapi/report/openapi/report.yaml | 9 +- docs/commands.md | 24 +-- src/cli_token.rs | 5 + src/executor.rs | 141 +++++++++++++---- src/main.rs | 178 ++++++++++++++++++---- src/spec.rs | 19 +++ tests/dry_run.rs | 108 ++++++++++++- 9 files changed, 427 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 665979d..11c1878 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,10 +20,12 @@ that may never merge. They are not releases and are not listed here. ## Unreleased - New `mapbox report ""` sends feedback about the CLI itself to - the Mapbox CLI team. It is sent with the CLI's own token, so it needs no - login, no `user-feedback:write` scope, and files nothing under your own - account; your token stands in only while the CLI's can't be used, or - when you pass `--token` or `--use-login`. + the Mapbox CLI team, with the CLI's version and OS appended. It is filed + under Mapbox's account, not yours. It is sent with the CLI's own token, + so it needs no login and no `user-feedback:write` scope; your token + stands in while the CLI's can't be used (including when it is + rate-limited), or when you pass `--token` or `--use-login`, and the + request then carries your token. `--category` is optional and defaults to `general`. Two new error codes say why the CLI's token couldn't be used when you have no token either: `cli_token_unavailable` and `cli_token_forbidden` (see diff --git a/README.md b/README.md index 67e2223..18edc2c 100644 --- a/README.md +++ b/README.md @@ -330,10 +330,12 @@ mapbox report "I'd like support for optimization commands" mapbox report "tilesets query hangs behind my proxy" --category bug ``` -The message goes to the Mapbox CLI team, not to your account, so +The message goes to the Mapbox CLI team, with the CLI's version and OS +appended, and is filed under Mapbox's account, not yours, so `mapbox feedback list` won't show it. It is sent with the CLI's own token, so it needs no login; yours stands in while the CLI's can't be used, or -when you pass `--token` or `--use-login`. See +when you pass `--token` or `--use-login`, and the request then carries your +token. See [Report](./docs/commands.md#report). ### Shell completion @@ -489,9 +491,10 @@ and `--dry-run` show it alongside everything else on the request. ### The CLI's own requests -Requests the CLI makes for itself rather than for your commands use your own -token whenever you have one: `--token` or `MAPBOX_ACCESS_TOKEN`, then your -login. Only when you have none does it use the CLI's token: +The CLI has a token of its own, for requests it makes for itself. Telemetry +uses your own token whenever you have one: `--token` or +`MAPBOX_ACCESS_TOKEN`, then your login. Only when you have none does it use +the CLI's token: `MAPBOX_CLI_TOKEN`, or a public token fetched from `https://cli.mapbox.com/config/token.json` and kept in `~/.mapbox/cli-token.json`. It is replaced only when the Tokens API says it @@ -501,8 +504,9 @@ third in a row pauses telemetry's sending with it for an hour, as three fetches in a row that find no usable token pause fetching. The token is restricted to `https://cli.mapbox.com/`, so every request with it carries that as its `Referer`. The CLI's token is only used for what -`src/cli_token.rs` lists for it: telemetry, and `mapbox report`, which -prefers it to yours and falls back to yours while it can't be used. +`src/cli_token.rs` lists for it: telemetry, and `mapbox report`, which is +the other way around: it prefers the CLI's token to yours and falls back to +yours while the CLI's can't be used. ### Proxies diff --git a/custom-openapi/report/openapi/report.yaml b/custom-openapi/report/openapi/report.yaml index d9dd500..33310f3 100644 --- a/custom-openapi/report/openapi/report.yaml +++ b/custom-openapi/report/openapi/report.yaml @@ -32,9 +32,12 @@ paths: operationId: report summary: Send feedback about the Mapbox CLI. description: >- - Sends one message to the Mapbox CLI team. Any valid token works; no - extra scope is needed. There is no way to read it back or delete it - afterwards. + Sends one message to the Mapbox CLI team, with the CLI's version and + OS appended. It is sent with a token the CLI provides, so it needs no + login; while that token can't be used, or with `--token` or + `--use-login`, it is sent with yours. Either way it is filed under + Mapbox's account, not yours. Any valid token works; no extra scope is + needed. There is no way to read it back or delete it afterward. parameters: - name: "access_token" in: query diff --git a/docs/commands.md b/docs/commands.md index bd7c35d..4e141e0 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2015,10 +2015,12 @@ that spec is wired in. ### `mapbox report` -Sends one message. It is filed under a Mapbox account, not yours, so -`mapbox feedback list` won't show it, and no `user-feedback:write` scope is -needed. There is no way to read it back or delete it, so `--dry-run` shows -the body without sending it. +Sends one message, with the CLI's version and OS appended to it +(`— mapbox-cli on `). It is filed under a Mapbox account, not +yours, so `mapbox feedback list` won't show it, and no `user-feedback:write` +scope is needed. There is no way to read it back or delete it, so +`--dry-run` shows the body, and which token it would go with, without +sending it. It is sent with the CLI's own token, not yours, so it works without a login. A typed `--token` or `--use-login` sends with yours instead, and the @@ -2027,15 +2029,19 @@ the CLI's token, a fresh one is fetched there and then and the message sent again with it, so a token that was republished or fixed works at once. A `MAPBOX_CLI_TOKEN` override is sent as it is and never replaced. When the CLI's token still can't be used — it can't be fetched, it is -revoked with no new one yet, or this API refuses it — your own token is -used instead, and with none the command fails with `cli_token_unavailable` -or `cli_token_forbidden`. Each run tries the CLI's token again; nothing -here is put off for later. +revoked with no new one yet, this API refuses it, or it is rate-limited +(`429`) — your own token is used instead, so the request then carries +your token, though the message is still filed under Mapbox's account. With +none of your own, the command fails with `cli_token_unavailable` or +`cli_token_forbidden`, or with the `429` itself. Each run tries the CLI's +token again; nothing here is put off for later. #### Parameters `` (positional, required) — what you want to tell the team. -Quote it. +Quote it. An empty one is refused. One that starts with `-` goes after +`--`, with every flag before it: `mapbox report --category bug -- "-x is +ignored"`. | Flag | Effect | | --- | --- | diff --git a/src/cli_token.rs b/src/cli_token.rs index c7287b5..bce3892 100644 --- a/src/cli_token.rs +++ b/src/cli_token.rs @@ -110,6 +110,11 @@ const CLI_TOKEN_COMMANDS: &[(&str, Use, &str)] = &[( logged in.", )]; +/// Whether `command` may run on the CLI's token at all. +pub(crate) fn may_use(command: &str) -> bool { + use_for(command).is_some() +} + fn use_for(command: &str) -> Option { CLI_TOKEN_COMMANDS .iter() diff --git a/src/executor.rs b/src/executor.rs index 66b24ef..38687d0 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -5,6 +5,7 @@ use std::time::Duration; use anyhow::{anyhow, Result}; use clap::ArgMatches; +use crate::cli_token; use crate::confirm; use crate::http; use crate::link; @@ -221,7 +222,7 @@ fn dispatch( .iter() .chain(&op.body_positional) .chain(&op.fixed_body_fields); - let merged_body = merge_body_fields(body_fields, matches, data)?; + let merged_body = merge_body_fields(body_fields, matches, data, op.signed_body_field)?; let variadic_body = variadic_body_array_json(&op.variadic_body_array, matches); let data = variadic_body.as_deref().or(merged_body.as_deref()).or(data); let files: Vec<&str> = matches @@ -250,7 +251,8 @@ fn dispatch( "Note: no access token was resolved, so the real request would be unauthenticated.", ); } - return describe_request(op, mode, &url, &query, body_source.as_ref()); + let sent_with = token.is_some().then_some(referer); + return describe_request(op, mode, &url, &query, body_source.as_ref(), sent_with); } // Placed after the dry-run branch deliberately: a dry run sends @@ -707,20 +709,40 @@ fn shown_value<'a>(name: &str, value: &'a str) -> &'a str { /// a note about it. So `-o json` gives a script an object to check before /// letting a real delete run, and a terminal gets one readable request /// line. +/// +/// `sent_with` is the token's `referer`, when there is a token: `Some(Some(_))` +/// for the CLI's own, `Some(None)` for the user's. fn describe_request( op: &Operation, mode: Mode, url: &str, query: &[(String, String)], body: Option<&BodySource<'_>>, + sent_with: Option>, ) -> Result<()> { let method = op.method.to_ascii_uppercase(); - let described = body.map(describe_body).transpose()?.flatten(); + // A positional body is the whole body, typed as an argument: there is + // no `--data` it came from, and nothing else shows the text a person + // is about to send. + let from_arguments = op.body_positional.is_some(); + let described = body + .map(|body| describe_body(body, from_arguments)) + .transpose()? + .flatten(); + // Only for a command that may go out on either token, where which one + // is not obvious from what was typed. + let token = sent_with.filter(|_| cli_token::may_use(&op.command())); let mut text = format!( "Dry run — nothing was sent.\n{method} {}", redacted_url(url, query) ); + if let Some(referer) = token { + text.push_str(&match referer { + Some(referer) => format!("\nToken: the CLI's own, sent with Referer {referer}"), + None => "\nToken: yours".to_owned(), + }); + } if let Some(described) = &described { text.push('\n'); text.push_str(&described.text); @@ -738,21 +760,21 @@ fn describe_request( }) .collect(); - output::emit( - mode, - &text, - serde_json::json!({ - "dry_run": true, - "command": op.command(), - "method": method, - // No query string here — `query` carries that separately. - // Rejoining them would produce a URL nobody can use, since the - // token in it is redacted. - "url": url, - "query": shown_query, - "body": described.map(|described| described.json), - }), - ) + let mut json = serde_json::json!({ + "dry_run": true, + "command": op.command(), + "method": method, + // No query string here — `query` carries that separately. + // Rejoining them would produce a URL nobody can use, since the + // token in it is redacted. + "url": url, + "query": shown_query, + "body": described.map(|described| described.json), + }); + if let Some(referer) = token { + json["token"] = serde_json::json!(if referer.is_some() { "cli" } else { "user" }); + } + output::emit(mode, &text, json) } /// A request body, as the dry run reports it. @@ -771,13 +793,30 @@ struct DescribedBody { /// call — exactly the surprise `--dry-run` is meant to rule out. The /// operations that take `--file` (sprites, upload chunks) are small, so /// the read costs nothing worth skipping. -fn describe_body(source: &BodySource<'_>) -> Result> { +/// +/// `from_arguments` when a JSON body was built from the command's arguments +/// alone, with no `--data`: it is then shown in full, in text as well. +fn describe_body(source: &BodySource<'_>, from_arguments: bool) -> Result> { Ok(match source { BodySource::Empty => None, BodySource::Json(data) => { let json: serde_json::Value = serde_json::from_str(data).map_err(|e| { CliError::new("invalid_data", format!("Invalid JSON for --data: {e}")) })?; + if from_arguments { + return Ok(Some(DescribedBody { + text: format!( + "Body ({JSON_CONTENT_TYPE}):\n{}", + serde_json::to_string_pretty(&json)? + ), + json: serde_json::json!({ + "source": "arguments", + "content_type": JSON_CONTENT_TYPE, + "bytes": data.len(), + "json": json, + }), + })); + } Some(DescribedBody { text: format!( "Body: {} bytes of {JSON_CONTENT_TYPE} from --data", @@ -1017,10 +1056,12 @@ fn variadic_body_array_json( /// `--data` with the body-field flags set on it, or `None` if nothing was /// set. A flag wins over `--data`; a default fills only what neither set. +/// The `signed` property, when one was set, carries [`signature`]. fn merge_body_fields<'a>( fields: impl IntoIterator, matches: &ArgMatches, data: Option<&str>, + signed: Option<&str>, ) -> Result> { let mut fields = fields.into_iter().peekable(); if fields.peek().is_none() { @@ -1058,6 +1099,9 @@ fn merge_body_fields<'a>( (None, _) => continue, }; let value = match field.numeric { + None if signed == Some(field.name.as_str()) => { + serde_json::Value::String(format!("{raw}{}", signature())) + } None => serde_json::Value::String(raw.clone()), Some(_) => serde_json::from_str::(raw) .map(serde_json::Value::Number) @@ -1076,6 +1120,17 @@ fn merge_body_fields<'a>( Ok(changed.then(|| serde_json::Value::Object(body).to_string())) } +/// What a [`crate::spec::Operation::signed_body_field`] ends with: the CLI's +/// version and OS, which a report about the CLI needs and the Feedback API +/// has no field for. +fn signature() -> String { + format!( + "\n\n— mapbox-cli {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ) +} + /// Picks between `--data` and `--file` for an operation that takes a body. /// /// Kept pure: every rejection here is a mistake the caller can fix from @@ -2034,7 +2089,8 @@ mod tests { /// let it pass and fail later. #[test] fn a_dry_run_rejects_the_json_that_sending_would_reject() { - let err = describe_body(&BodySource::Json("{oops")).expect_err("invalid JSON is refused"); + let err = + describe_body(&BodySource::Json("{oops"), false).expect_err("invalid JSON is refused"); assert_eq!(refusal(err).code, "invalid_data"); } @@ -2042,10 +2098,13 @@ mod tests { /// a metadata check would wave an unreadable file through. #[test] fn a_dry_run_rejects_a_file_that_cannot_be_read() { - let err = describe_body(&BodySource::Raw { - path: "/nonexistent/zz-clitest.svg", - content_type: "image/svg+xml", - }) + let err = describe_body( + &BodySource::Raw { + path: "/nonexistent/zz-clitest.svg", + content_type: "image/svg+xml", + }, + false, + ) .expect_err("an unreadable file is refused"); assert_eq!(refusal(err).code, "invalid_file"); } @@ -2053,7 +2112,7 @@ mod tests { /// A "Body:" line on every `delete` would just be noise. #[test] fn a_body_that_was_never_given_is_not_described() { - assert!(describe_body(&BodySource::Empty) + assert!(describe_body(&BodySource::Empty, false) .expect("an absent body is not an error") .is_none()); } @@ -2071,10 +2130,13 @@ mod tests { std::fs::write(&icon, b"").expect("write icon"); let path = icon.to_str().expect("utf-8 temp path"); - let described = describe_body(&BodySource::Multipart { - paths: vec![path], - field: "images", - }) + let described = describe_body( + &BodySource::Multipart { + paths: vec![path], + field: "images", + }, + false, + ) .expect("a readable file describes") .expect("a multipart body is described"); @@ -2318,7 +2380,7 @@ mod tests { .arg(clap::Arg::new("lat").long("lat")) .arg(clap::Arg::new("category").long("category")) .get_matches_from(std::iter::once("create").chain(args.iter().copied())); - merge_body_fields(&fields, &matches, data) + merge_body_fields(&fields, &matches, data, None) } #[test] @@ -2326,6 +2388,25 @@ mod tests { assert_eq!(merge(&[], Some(r#"{"lat": 1.50}"#)).unwrap(), None); } + #[test] + fn a_signed_field_ends_with_the_version_and_os() { + let matches = clap::Command::new("report") + .arg(clap::Arg::new("category").long("category")) + .get_matches_from(["report", "--category", "bug"]); + let body = merge_body_fields(&[param("category")], &matches, None, Some("category")) + .unwrap() + .unwrap(); + let body: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!( + body["category"], + format!( + "bug\n\n— mapbox-cli {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ) + ); + } + #[test] fn a_default_fills_only_what_nothing_else_set() { let body = |args: &[&str], data| -> serde_json::Value { diff --git a/src/main.rs b/src/main.rs index 361c5fd..3e37c13 100644 --- a/src/main.rs +++ b/src/main.rs @@ -366,6 +366,7 @@ fn build_operation_command(op: &spec::Operation) -> Command { cmd = cmd.arg( Arg::new(field.arg_name.clone()) .required(true) + .value_parser(not_blank) .help(help_text(field)), ); } @@ -975,6 +976,7 @@ fn cli() -> u8 { /// error that makes it matter most. fn report_parse_result(err: clap::Error, raw_argv: &[std::ffi::OsString]) -> u8 { let err = drop_subcommand_from_short_circuit_usage(err); + let err = whole_value_after_double_dash(err, raw_argv); // Clap uses 2 for a usage error and 0 for help/version; preserving that // is more useful to a caller than flattening everything to 1. @@ -1159,6 +1161,53 @@ fn drop_subcommand_from_short_circuit_usage(mut err: clap::Error) -> clap::Error err } +/// A positional body field is what the command is for, so an empty one +/// would send a request with nothing in it. +fn not_blank(value: &str) -> std::result::Result { + if value.trim().is_empty() { + return Err("it is empty".to_owned()); + } + Ok(value.to_owned()) +} + +/// Rewrites clap's tip for a value that starts with `-`, such as +/// `mapbox report "-x is broken"`. +/// +/// Clap names only the flag-like start of it (`-x`) and suggests +/// `'-- -x'`, which would send `-x` alone. The tip has to quote the +/// argument as typed, and say that flags go before the `--`: everything +/// after it is a value, so a `--dry-run` there would become part of the +/// message. Left alone for any other tip, such as a misspelled flag's. +fn whole_value_after_double_dash(mut err: clap::Error, argv: &[std::ffi::OsString]) -> clap::Error { + if err.kind() != ErrorKind::UnknownArgument { + return err; + } + let passes_as_value = matches!( + err.get(ContextKind::Suggested), + Some(ContextValue::StyledStrs(tips)) + if tips.iter().any(|tip| tip.to_string().starts_with("to pass ")) + ); + let Some(ContextValue::String(start)) = err.get(ContextKind::InvalidArg) else { + return err; + }; + if !passes_as_value { + return err; + } + let Some(typed) = argv + .iter() + .filter_map(|arg| arg.to_str()) + .find(|arg| arg.starts_with(start.as_str())) + else { + return err; + }; + let mut tip = StyledStr::new(); + tip.push_str(&format!( + "to pass '{typed}' as a value, put '--' before it, and every flag before the '--'" + )); + err.insert(ContextKind::Suggested, ContextValue::StyledStrs(vec![tip])); + err +} + /// Whether `text` is nothing but ANSI style sequences (`ESC [ … m`). fn is_style_codes(text: &str) -> bool { without_trailing_style_codes(text).is_empty() @@ -1513,12 +1562,12 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - ); match attempt { CliAttempt::Done(result) => return result, - CliAttempt::Unusable { error, sent } => { + CliAttempt::Unusable { error, why, sent } => { // The user already confirmed the request that was // refused; sending it again with their own token is // the same request, not a second one to agree to. flags.assume_yes |= sent; - cli_failure = Some(error); + cli_failure = Some((error, why)); } } } @@ -1545,14 +1594,11 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - // user's, and with none says why the CLI's failed rather than // sending a request bound to be refused. A dry run sends nothing // and fetches nothing, so it plans with whatever there is. - if let Some(failure) = cli_failure.filter(|_| !dry_run) { + if let Some((failure, why)) = cli_failure.filter(|_| !dry_run) { if token.is_none() { - return Err((*failure).into()); + return Err((*failure).with_remedy(no_own_token_remedy()).into()); } - output::progress(&format!( - "{} Using your own token instead.", - failure.message - )); + output::progress(&format!("{why} Using your own token instead.")); } if use_login && token.is_none() { return Err(no_stored_credentials(profile)); @@ -1594,10 +1640,15 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - enum CliAttempt { /// The CLI's token was used; this is the command's own result. Done(Result<()>), - /// It could not be used, for the reason given, and the user's token may - /// stand in. `sent` when a request went out and was refused, so any - /// confirmation was already given. - Unusable { error: Box, sent: bool }, + /// It could not be used, and the user's token may stand in: `error` is + /// what the command fails with when there is none, and `why` says what + /// happened when there is. `sent` when a request went out and was + /// refused, so any confirmation was already given. + Unusable { + error: Box, + why: String, + sent: bool, + }, } /// Runs a [`cli_token::Use::Preferred`] command on the CLI's token, given @@ -1615,10 +1666,12 @@ fn try_cli_token( let token = match cli { Ok(token) => token, Err(why) => { + let error = cli_token_unavailable(command, &why); return CliAttempt::Unusable { - error: Box::new(cli_token_unavailable(&why)), + why: error.message.clone(), + error: Box::new(error), sent: false, - } + }; } }; // Only for the CLI's token: the user's, which may stand in after, goes @@ -1645,51 +1698,116 @@ fn try_cli_token( }, ); let (result, refusal) = match sent { - cli_token::Sent::Answered(result) => return CliAttempt::Done(result), + cli_token::Sent::Answered(result) => match rate_limited(result) { + // Every install shares the CLI's token, so a limit on it says + // nothing about the user's. A `429` was not acted on, so sending + // it again is not a second report; with no token of their own, + // the `429` is the answer. + Ok(error) => { + return CliAttempt::Unusable { + why: provided_token(command, "Mapbox is rate-limiting it right now"), + error, + sent: true, + } + } + Err(result) => return CliAttempt::Done(result), + }, cli_token::Sent::Refused(result, refusal) => (result, refusal), }; - let (code, message) = match refusal { + let (code, reason) = match refusal { cli_token::Refusal::Dead => ( "cli_token_unavailable", - "The CLI's token is revoked and no new one is published yet.", - ), - cli_token::Refusal::Forbidden => ( - "cli_token_forbidden", - "The CLI's token is valid, but this API does not accept it.", + "it is revoked and no new one is published yet", ), + cli_token::Refusal::Forbidden => ("cli_token_forbidden", "this API does not accept it"), cli_token::Refusal::Unknown => ( "cli_token_forbidden", - "This API refused the CLI's token, and checking why failed.", + "this API refused it, and checking why failed", ), // Refused, and not checked why: `MAPBOX_CLI_TOKEN`, or the token on // disk changed meanwhile or could not be written. - cli_token::Refusal::NotFetched => { - ("cli_token_forbidden", "This API refused the CLI's token.") - } + cli_token::Refusal::NotFetched => ("cli_token_forbidden", "this API refused it"), }; let request_id = result .err() .and_then(|e| e.downcast::().ok()) .and_then(|e| e.request_id); + let message = provided_token(command, reason); CliAttempt::Unusable { + why: message.clone(), error: Box::new(CliError::new(code, message).with_request_id(request_id)), sent: true, } } +/// The error of a request the API answered `429`, or the result as it was. +fn rate_limited(result: Result<()>) -> std::result::Result, Result<()>> { + match result { + Err(error) if error.downcast_ref::().and_then(|e| e.status) == Some(429) => Ok( + Box::new(error.downcast::().expect("checked above")), + ), + result => Err(result), + } +} + +/// Why `command` could not use the token the CLI provides, in words for +/// someone who has never heard of it: what it is, then `reason`. +fn provided_token(command: &str, reason: &str) -> String { + format!( + "`mapbox {command}` sends with a token the CLI provides, so it needs no login, \ + but {reason}." + ) +} + /// Why there was no CLI token to try, as the error a command fails with /// when the user has no token of their own either. -fn cli_token_unavailable(why: &cli_token::Unavailable) -> CliError { - CliError::new( - "cli_token_unavailable", - format!("Can't use the CLI's token: {why}."), - ) +fn cli_token_unavailable(command: &str, why: &cli_token::Unavailable) -> CliError { + let reason = match why { + cli_token::Unavailable::Unreachable => { + "cli.mapbox.com, where the CLI gets it, could not be reached" + } + cli_token::Unavailable::NoToken { revoked: true } => { + "it is revoked and no new one is published yet" + } + cli_token::Unavailable::NoToken { revoked: false } => { + "cli.mapbox.com did not return a usable one" + } + cli_token::Unavailable::NotFetched => "it has not been fetched yet", + }; + CliError::new("cli_token_unavailable", provided_token(command, reason)) +} + +/// What to do when a command could not use the CLI's token and the user has +/// none of their own: give it theirs. +fn no_own_token_remedy() -> Remedy { + Remedy::default() + .with_fix("Log in with `mapbox auth login`, or pass `--token`, and it is sent with yours.") + .with_action(Some("mapbox auth login".to_owned())) } #[cfg(test)] mod tests { use super::*; + /// Every install shares the CLI's token, so its rate limit is no reason + /// to fail someone whose own token would work; nothing else is. + #[test] + fn only_a_429_on_the_clis_token_falls_back() { + let failed = |status: u16| -> Result<()> { + Err(CliError { + status: Some(status), + ..CliError::new(format!("http_{status}"), "Refused") + } + .into()) + }; + let error = rate_limited(failed(429)).expect("a 429 falls back"); + assert_eq!(error.code, "http_429"); + for status in [400, 404, 500] { + assert!(rate_limited(failed(status)).is_err(), "{status}"); + } + assert!(matches!(rate_limited(Ok(())), Err(Ok(())))); + } + /// Half the world has a negative longitude. Clap reads `-74.0` as a /// cluster of short flags unless told otherwise, which made every /// operation taking a coordinate unusable west of Greenwich — as a diff --git a/src/spec.rs b/src/spec.rs index 8069242..ec9e189 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -39,6 +39,9 @@ pub struct Operation { /// JSON body properties sent with a fixed value and never typed. See /// [`FIXED_BODY_FIELDS`]. pub fixed_body_fields: Vec, + /// A string body property sent with the CLI's version and OS appended. + /// See [`SIGNED_BODY_FIELDS`]. + pub signed_body_field: Option<&'static str>, pub base_url: String, /// Set when the operation needs a scope that isn't registrable as OAuth /// (see `UNSUPPORTED_OPERATIONS` below). A `mapbox auth login` token can @@ -121,6 +124,12 @@ type FixedBodyField = (&'static str, &'static str); const FIXED_BODY_FIELDS: &[(&str, &str, &[FixedBodyField])] = &[("report", "report", &[("lat", "0"), ("lon", "0")])]; +/// (service, operationId, body field name) for a string body property sent +/// with the CLI's version and OS appended. A report about the CLI is little +/// use without them, and the Feedback API has no field of its own for +/// either, so they go in the text, where `--dry-run` shows them too. +const SIGNED_BODY_FIELDS: &[(&str, &str, &str)] = &[("report", "report", "feedback")]; + /// (service, operationId, body field name, CLI positional name, max count) /// for an operation whose JSON body takes a single array field, exposed as /// one or more positional arguments collected into it instead of `--data`. @@ -1252,6 +1261,8 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { variadic_body_array, body_positional, fixed_body_fields, + signed_body_field: operation_id + .and_then(|id| signed_body_field_name(service_name, id)), base_url: base_url.clone(), disabled_scope, detail: None, @@ -1352,6 +1363,13 @@ fn positional_body_field_names( .map(|(_, _, field, arg_name)| (*field, *arg_name)) } +fn signed_body_field_name(service: &str, operation_id: &str) -> Option<&'static str> { + SIGNED_BODY_FIELDS + .iter() + .find(|(s, id, _)| *s == service && *id == operation_id) + .map(|(_, _, field)| *field) +} + fn fixed_body_field_values(service: &str, operation_id: &str) -> Option<&'static [FixedBodyField]> { FIXED_BODY_FIELDS .iter() @@ -2195,6 +2213,7 @@ requestBody: let flags: Vec<&str> = op.body_fields.iter().map(|f| f.name.as_str()).collect(); assert_eq!(flags, ["category"]); + assert_eq!(op.signed_body_field, Some("feedback")); let fixed: Vec<(&str, Option<&str>, Option)> = op .fixed_body_fields diff --git a/tests/dry_run.rs b/tests/dry_run.rs index 68f1c28..92416b1 100644 --- a/tests/dry_run.rs +++ b/tests/dry_run.rs @@ -537,22 +537,110 @@ fn report_sends_the_message_to_the_cli_endpoint() { defaults["url"], "https://api.mapbox.com/user-feedback/v1/cli-feedback" ); + let feedback = format!( + "A test\n\n— mapbox-cli {} on {}", + env!("CARGO_PKG_VERSION"), + std::env::consts::OS + ); assert_eq!( defaults["body"]["json"], - serde_json::json!({ "feedback": "A test", "lat": 0, "lon": 0, "category": "general" }) + serde_json::json!({ "feedback": feedback, "lat": 0, "lon": 0, "category": "general" }) ); + assert_eq!(defaults["body"]["source"], "arguments"); + assert_eq!(defaults["token"], "user"); let categorized = plan(&["--category", "bug"]); assert_eq!(categorized["body"]["json"]["category"], "bug"); } +/// A terminal's dry run shows the message as it would be sent, and which +/// token it would go with, rather than only the body's size. +#[test] +fn report_dry_run_shows_the_body_and_token_in_text() { + let config = config_dir("report-text"); + let out = run( + &config, + &[ + "-o", + "text", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "A test", + ], + ); + assert!(out.status.success(), "{}", stderr(&out)); + let text = stdout(&out); + assert!(text.contains("Token: yours"), "{text}"); + assert!( + text.contains(r#""feedback": "A test\n\n— mapbox-cli "#), + "{text}" + ); + assert!(!text.contains("--data"), "{text}"); +} + +/// Clap names only `-x` of `"-x is broken"` and would have it passed alone; +/// the tip quotes the whole message. +#[test] +fn report_says_how_to_send_a_message_starting_with_a_dash() { + let config = config_dir("report-dash"); + let out = run( + &config, + &[ + "-o", + "json", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "-x is broken", + ], + ); + assert_eq!(out.status.code(), Some(2), "{}", stderr(&out)); + let fix = json(&stderr(&out))["fix"] + .as_str() + .unwrap_or_default() + .to_owned(); + assert!( + fix.contains("'-x is broken'") && fix.contains("'--'"), + "{fix}" + ); + + let out = run( + &config, + &[ + "-o", + "json", + "--token", + "sk.a-test-token", + "report", + "--dry-run", + "--", + "-x is broken", + ], + ); + assert!(out.status.success(), "{}", stderr(&out)); + let feedback = json(&stdout(&out))["body"]["json"]["feedback"].clone(); + assert!( + feedback + .as_str() + .unwrap_or_default() + .starts_with("-x is broken\n"), + "{feedback}" + ); +} + /// The positional is the whole body; a `--data` would be a second way to -/// say it, or a way to send a place this endpoint has no use for. +/// say it, or a way to send a place this endpoint has no use for. An empty +/// message is refused before anything is sent. #[test] fn report_needs_a_message_and_takes_no_data() { let config = config_dir("report-usage"); for args in [ &["--token", "sk.a-test-token", "report", "--dry-run"][..], + &["--token", "sk.a-test-token", "report", "--dry-run", ""][..], + &["--token", "sk.a-test-token", "report", "--dry-run", " \n "][..], &[ "--token", "sk.a-test-token", @@ -643,6 +731,22 @@ mod report_token { assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); assert!(stdout(&out).is_empty(), "{}", stdout(&out)); assert_eq!(error_code(&out), "cli_token_unavailable"); + let error = json(&stderr(&out)); + assert!( + error["fix"] + .as_str() + .unwrap_or_default() + .contains("--token"), + "{error}" + ); + assert_eq!(error["next_actions"][0], "mapbox auth login"); + assert!( + !error["message"] + .as_str() + .unwrap_or_default() + .contains("CLI's token"), + "{error}" + ); } /// The CLI's token is restricted to its URL, so a request with it says From 98681c0829d92cf3530bbbdb19944790cac0608f Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 19:29:19 +0300 Subject: [PATCH 8/9] Say telemetry settings don't apply to mapbox report The token report fetches is shared and public, and fetching it sends nothing about the user, so it is not telemetry. --- docs/commands.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/commands.md b/docs/commands.md index 4e141e0..d923ece 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2034,7 +2034,9 @@ revoked with no new one yet, this API refuses it, or it is rate-limited your token, though the message is still filed under Mapbox's account. With none of your own, the command fails with `cli_token_unavailable` or `cli_token_forbidden`, or with the `429` itself. Each run tries the CLI's -token again; nothing here is put off for later. +token again; nothing here is put off for later. Telemetry settings don't +apply: the token is shared and public, and fetching it sends nothing about +you. #### Parameters From cce473154b53d0e29c89d37f3c6427ffef76b99a Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Mon, 19 Oct 2026 19:35:02 +0300 Subject: [PATCH 9/9] Line up a report's later lines, and recapture its outputs from one send A report's message now has the CLI's version and OS on lines of its own, and -o text printed them back at the label column. They now go under the first line. The Outputs in docs/commands.md are one live send, shown under -o text and -o json, so the two ids match. It was sent on the CLI's token with its Referer, and the API kept the version and OS. --- docs/commands.md | 11 +++++++---- src/output/render.rs | 32 ++++++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index d923ece..be5790b 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2062,8 +2062,9 @@ mapbox report "…" --dry-run #### Outputs -Captured live on 2026-10-19. `-o text` shows only that it was sent, the -message and its id; `-o json` is the answer as the API sent it: +One report, captured live on 2026-10-19 and shown both ways. `-o text` +shows only that it was sent, the message and its id; `-o json` is the +answer as the API sent it: @@ -2072,13 +2073,15 @@ message and its id; `-o json` is the answer as the API sent it: ```text Sent to the Mapbox CLI team. feedback Test from mapbox-cli development (report). Please ignore. -id 01a15484-6d9b-744c-bea4-a9a0a99b95c5 + + — mapbox-cli 0.3.0 on macos +id 01a15503-8d64-75ed-aa9b-790ae4ba69a0 ``` diff --git a/src/output/render.rs b/src/output/render.rs index 6727670..3e69f2a 100644 --- a/src/output/render.rs +++ b/src/output/render.rs @@ -49,11 +49,18 @@ pub(super) fn list_rendering(value: &Value, service: Option<&str>) -> Option Option { const SENT: &str = "Sent to the Mapbox CLI team."; let map = value.as_object()?; + // The message is several lines once the CLI's version and OS are on + // it; each goes under the first, not back at the label column. + let indent = format!("\n{}", " ".repeat("feedback".len() + 2)); + let feedback = map + .get("feedback")? + .as_str()? + .split('\n') + .collect::>() + .join(&indent) + .replace(&format!("{indent}{indent}"), &format!("\n{indent}")); let fields = vec![ - ( - "feedback".to_owned(), - map.get("feedback")?.as_str()?.to_owned(), - ), + ("feedback".to_owned(), feedback), ("id".to_owned(), map.get("id")?.as_str()?.to_owned()), ]; Some(Rendered { @@ -2390,6 +2397,23 @@ mod tests { } } + #[test] + fn a_reports_later_lines_line_up_under_its_first() { + let answer = serde_json::json!({ + "id": "an-id", + "feedback": "A test\n\n— mapbox-cli 0.3.0 on macos", + }); + let rendered = list_rendering(&answer, Some("report")).expect("a rendering"); + assert_eq!( + rendered.text, + "Sent to the Mapbox CLI team.\n\ + feedback A test\n\ + \n \ + — mapbox-cli 0.3.0 on macos\n\ + id an-id" + ); + } + #[test] fn a_report_answer_missing_its_id_is_shown_whole() { let answer = serde_json::json!({ "feedback": "test", "status": "received" });
Terminal — -o textAgent — -o json
```json -{"category":"general","created_at":"2026-10-19T14:15:32.069Z","feedback":"Test from mapbox-cli development (report). Please ignore.","has_screenshot":false,"id":"01a15484-7fc9-7328-a04d-251340f49f64","location":{"lat":0,"lon":0},"received_at":"2026-10-19T14:15:32.069Z","status":"received","updated_at":"2026-10-19T14:15:32.069Z"} +{"category":"general","created_at":"2026-10-19T16:34:18.626Z","feedback":"Test from mapbox-cli development (report). Please ignore.\n\n— mapbox-cli 0.3.0 on macos","has_screenshot":false,"id":"01a15503-8d64-75ed-aa9b-790ae4ba69a0","location":{"lat":0,"lon":0},"received_at":"2026-10-19T16:34:18.626Z","status":"received","updated_at":"2026-10-19T16:34:18.626Z"} ```