From 1db48f13ce6c6653922bdedf75aa676e8878939d Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Tue, 20 Oct 2026 13:46:53 +0300 Subject: [PATCH 01/10] Add mapbox commands search and commands list Finds a command by what it does: local keyword search over the same command surface --schema describes, with no request, token or model. A table at a terminal, [{command, summary}] under json. Top-level help, the agent hint and an unknown-command error point to it. Three query sets under tests/fixtures/commands_search are held to score floors by eval_set_* tests, and a CI job writes each PR's scores and the queries it fixed and broke against its base to the job summary. --- .github/workflows/ci.yml | 50 ++ AGENTS.md | 16 + CHANGELOG.md | 7 + Cargo.lock | 1 + Cargo.toml | 3 + README.md | 12 + docs/commands.md | 123 ++++ scripts/commands-search-report.py | 125 ++++ src/commands.rs | 795 ++++++++++++++++++++++ src/config.rs | 4 +- src/doctor.rs | 6 +- src/help_layout.rs | 14 +- src/history.rs | 4 +- src/main.rs | 38 +- src/mcp.rs | 5 + src/output/mod.rs | 2 +- src/schema.rs | 8 + src/tilesets_cli.rs | 7 +- src/uninstall.rs | 2 +- tests/fixtures/commands_search/README.md | 74 ++ tests/fixtures/commands_search/docs.tsv | 64 ++ tests/fixtures/commands_search/issues.tsv | 104 +++ tests/fixtures/commands_search/tuning.tsv | 104 +++ tests/output_contract.rs | 107 ++- tests/telemetry_events.rs | 32 + 25 files changed, 1686 insertions(+), 21 deletions(-) create mode 100644 scripts/commands-search-report.py create mode 100644 src/commands.rs create mode 100644 tests/fixtures/commands_search/README.md create mode 100644 tests/fixtures/commands_search/docs.tsv create mode 100644 tests/fixtures/commands_search/issues.tsv create mode 100644 tests/fixtures/commands_search/tuning.tsv diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a7db73..98c4d4f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,6 +113,56 @@ jobs: shell: bash run: sh scripts/test-completion.sh + # Which `commands search` queries a PR fixed and broke, against its base, + # written to the job summary. It reports and never fails: the floors that + # `cargo test` holds in the build job are the gate. A job summary rather + # than a PR comment because it needs no write token, so a PR from a fork + # gets one too. One runner: ranking is the same everywhere. + search-eval: + name: commands search eval + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + steps: + # Whole history, so the base commit is there to check out beside it. + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + + # As in the build job: install exactly the pinned toolchain. + - name: Read the pinned toolchain + id: toolchain + run: | + echo "toolchain=$(sed -n 's/^channel = "\(.*\)"/\1/p' rust-toolchain.toml | head -1)" >>"$GITHUB_OUTPUT" + + - name: Install Rust + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ steps.toolchain.outputs.toolchain }} + + # `|| true`: a run below its floor still writes its reports, and the + # build job is where that failure is reported. + - name: Run the eval on this branch + run: MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-head" cargo test --locked --bin mapbox eval_set || true + + # In a worktree sharing this target directory, so only the crate itself + # rebuilds. A base from before the eval existed matches no test and + # writes no reports; the summary then shows this branch's scores alone. + - name: Run the eval on the base + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + git worktree add --detach "$RUNNER_TEMP/base" "$BASE_SHA" + cd "$RUNNER_TEMP/base" + CARGO_TARGET_DIR="$GITHUB_WORKSPACE/target" \ + MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-base" \ + cargo test --locked --bin mapbox eval_set || true + + - name: Write the summary + run: python3 scripts/commands-search-report.py "$RUNNER_TEMP/eval-head" "$RUNNER_TEMP/eval-base" >>"$GITHUB_STEP_SUMMARY" + # No Rust: this is where the repo's shell gets checked, install.sh above all # — it is served to machines we do not control, and this is the only thing # that checks it. Both runners, because the portability it has to get right diff --git a/AGENTS.md b/AGENTS.md index c99ce88..d27c65d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,6 +126,22 @@ One table outside that file works the same way: `GROUPS` in an entry outlives its command. A command it misses still shows, under "Other", but CI will not let that ship. +## `commands search` and its query sets + +`mapbox commands search` ranks commands by the words in their names, summaries, +descriptions and help-group lines, so **editing a command's `about` or a line +in `DESCRIPTIONS` changes search results**, not only `--help`. The `eval_set_*` +tests in `src/commands.rs` run the query sets in `tests/fixtures/commands_search/` +and hold each to a floor of how many queries rank right. Each file's header says +where its queries came from and how much the search was tuned against them. +`cargo test eval_set -- --nocapture` prints the scores and every miss, and +that directory's README explains each set and how to read its score. + +When a change raises a score, raise its floor in the same commit. Lowering one +is a decision to explain in the PR. Don't tune against the sets by +special-casing a query; fix the kind of miss. New real queries go in a new +file, so there is always one set the search has not been fitted to. + ## What the tests can and cannot tell you `cargo test` answers to fake tokens and a fake server. It runs offline, on a diff --git a/CHANGELOG.md b/CHANGELOG.md index 11c1878..a7c6555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,13 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +- New `mapbox commands search ""` finds the commands that match a + task, best first, and `mapbox commands list` lists every command with its + summary. Both are local keyword lookups: no request, no token. `json` + gives `[{command, summary}]`. The query is sent in telemetry as its length + only. Top-level help now points to `commands search` (agents see it in + their hint), and an unknown command with no near spelling suggests it in + the error's `fix`. No existing command or output changes. - New `mapbox report ""` sends feedback about the CLI itself to 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, diff --git a/Cargo.lock b/Cargo.lock index dce78d6..a29ad23 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -800,6 +800,7 @@ dependencies = [ "serde_json", "serde_yaml", "sha2", + "strsim", "tar", "terminal-colorsaurus", "terminal_size", diff --git a/Cargo.toml b/Cargo.toml index 51cfd7e..06c0dab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,6 +65,9 @@ clap = { version = "4", features = ["derive", "env", "string", "wrap_help"] } # top-level help, which src/help_layout.rs renders itself and so has to wrap # to the terminal itself. terminal_size = "0.4" +# Typo tolerance for `mapbox commands search`. Already in the graph through +# clap's own "did you mean" suggestions; the same version. +strsim = "0.11" # Renders `mapbox completion ` from the same `Command` tree everything # else here is built from. Kept to the default features on purpose: the # dynamic-completion half is `unstable-dynamic`, and value completion is out diff --git a/README.md b/README.md index 18edc2c..ec12515 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,7 @@ mapbox styles list - [Named profiles](#named-profiles) - [Commands](#commands) - [API commands](#api-commands) + - [Finding a command](#finding-a-command) - [Diagnostics and settings](#diagnostics-and-settings) - [Shell completion](#shell-completion) - [Tileset CLI](#tileset-cli) @@ -308,6 +309,17 @@ account it belongs to, alongside the event. With no token of your own and no way to fetch the CLI's, the event is dropped. `MAPBOX_CLI_NO_TELEMETRY=1` or `mapbox config set telemetry off` turns it off. +### Finding a command + +```sh +mapbox commands search upload a font # the commands that match a task, best first +mapbox commands list # every command with its summary +``` + +Both are local keyword lookups over the CLI's own commands: no model, no +request and no token. Agents get the same matches as JSON. See +[Commands](./docs/commands.md#commands). + ### Diagnostics and settings ```sh diff --git a/docs/commands.md b/docs/commands.md index be5790b..6fd7693 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -134,6 +134,9 @@ nests, and is typed `mapbox styles draft get`. **[History](#history)** — [history.list](#mapbox-history-list) · [history.show](#mapbox-history-show) +**[Commands](#commands)** — [commands.search](#mapbox-commands-search) · +[commands.list](#mapbox-commands-list) + **[Doctor](#doctor)** — [doctor](#mapbox-doctor) **[Usage](#usage)** — [usage](#mapbox-usage) @@ -4736,6 +4739,126 @@ Log no longer available: it expired or was removed to keep diagnostic logs --- +## Commands + +Finding a command by what it does. Both read the same command surface +`mapbox --schema` describes, so they cover every command this build has, +and both are local: no request and no token. `json` gives each command as +`{command, summary}`; `mapbox --schema` has the rest. + +### `mapbox commands search` + +The commands that match a task, best first. It is keyword matching, not a +model: each word of the query is matched against a command's name, summary +and description, its group's description and its arguments' help, with a +command's name counting most. A word can be a prefix (`geo`), a different +ending of the same stem (`geocoding` for `geocoder`) or one typo away +(`uplaod`); a plural matches its singular. A few common words stand in for +the ones commands use ("remove" for "delete", "rename" for "update"). +Words like "how", "my", "to" and "mapbox" are ignored. It cannot find a +command whose text uses none of the query's words, so a query names the +action and the thing acted on: "upload a font", not "get my typeface onto +Mapbox". Matches far below the best one are left out. A task no command +does usually still comes back with a few weak guesses, so check the +summaries before running one. + +The query can be quoted or not. No match is not an error: `text` prints +nothing and says so on stderr, `json` prints `[]`, and both exit 0. The +`commands` group itself is left out of the results. Telemetry records the +query's length, never its text. + +#### Parameters + +| Parameter | Effect | +| --- | --- | +| `...` | What you want to do, in a few words. | +| `--limit ` | How many matches to show. Defaults to `5`; `0` shows every match. | + +#### Examples + +```sh +mapbox commands search upload a font + +mapbox commands search "snap a gps trace to roads" + +mapbox commands search tiles --limit 0 +``` + +#### Outputs + + + + +
textjson
+ +``` +COMMAND SUMMARY +mapbox fonts upload Upload a font +mapbox fonts list List fonts for an owner +mapbox fonts delete Delete a font +``` + + + +```json +[ + { + "command": "mapbox fonts upload", + "summary": "Upload a font" + }, + { + "command": "mapbox fonts list", + "summary": "List fonts for an owner" + }, + ... +] +``` + +
+ +### `mapbox commands list` + +Every command with its summary, sorted by name. + +#### Examples + +```sh +mapbox commands list +``` + +#### Outputs + + + + +
textjson
+ +``` +COMMAND SUMMARY +mapbox accounts list-scopes List scopes +mapbox accounts list-tokens List tokens +mapbox accounts retrieve-token Retrieve a token +... +``` + + + +```json +[ + { + "command": "mapbox accounts list-scopes", + "summary": "List scopes" + }, + { + "command": "mapbox accounts list-tokens", + "summary": "List tokens" + }, + ... +] +``` + +
+ ## Doctor ### `mapbox doctor` diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py new file mode 100644 index 0000000..d233ec5 --- /dev/null +++ b/scripts/commands-search-report.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 +"""Markdown summary of `mapbox commands search` eval results, for a CI job +summary. + + commands-search-report.py [] + +Each directory holds the per-set reports the `eval_set_*` tests write when +MAPBOX_SEARCH_EVAL_REPORT names it. With a base, the summary says which +queries the change fixed and broke; without one (a base from before the +eval existed), only the scores. + +Reports, never gates: the floors in `cargo test` are the gate. Standard +library only, so the job needs no install step. +""" + +import sys +from pathlib import Path + + +def load(directory): + sets = {} + if directory is None or not directory.is_dir(): + return sets + for path in sorted(directory.iterdir()): + floors, rows = None, {} + for line in path.read_text().splitlines(): + columns = line.split("\t") + if columns[0] == "# floors": + floors = (int(columns[1]), int(columns[2])) + continue + query, expected, found, first, top3 = columns + rows[query] = { + "expected": expected.replace("|", " or ") or "(nothing)", + "found": found.replace("|", ", ") or "(nothing)", + "first": first == "1", + "top3": top3 == "1", + } + sets[path.name] = (floors, rows) + return sets + + +def score(rows, key): + return sum(row[key] for row in rows.values()) + + +def delta(head, base): + if base is None: + return f"{head}" + change = head - base + return f"{head} ({change:+d})" if change else f"{head}" + + +def changed(head_rows, base_rows): + """Queries in both whose first result went from right to wrong or back.""" + fixed, broken = [], [] + for query, row in head_rows.items(): + before = base_rows.get(query) + if before is None or before["first"] == row["first"]: + continue + line = ( + f"| {query} | {row['expected']} | {before['found']} | {row['found']} |" + ) + (fixed if row["first"] else broken).append(line) + return fixed, broken + + +def main(): + head = load(Path(sys.argv[1])) + base = load(Path(sys.argv[2])) if len(sys.argv) > 2 else {} + out = ["## `commands search` eval", ""] + if not head: + out.append("No reports were written; the eval tests did not run.") + print("\n".join(out)) + return + if not base: + out += ["The base branch has no eval reports, so only this branch's scores are shown.", ""] + + out += [ + "| Set | Rows | First | Top 3 | Floor |", + "| --- | --- | --- | --- | --- |", + ] + details = [] + for name, (floors, rows) in head.items(): + base_rows = base.get(name, (None, None))[1] + first, top3 = score(rows, "first"), score(rows, "top3") + base_first = score(base_rows, "first") if base_rows else None + base_top3 = score(base_rows, "top3") if base_rows else None + floor = f"{floors[0]} / {floors[1]}" if floors else "-" + below = floors and (first < floors[0] or top3 < floors[1]) + out.append( + f"| `{name}` | {len(rows)} | {delta(first, base_first)} | " + f"{delta(top3, base_top3)} | {floor}{' ⚠️ below' if below else ''} |" + ) + + if base_rows: + fixed, broken = changed(rows, base_rows) + header = ["| Query | Want | Base | This branch |", "| --- | --- | --- | --- |"] + if broken: + details += [f"### `{name}`: {len(broken)} no longer first", ""] + header + broken + [""] + if fixed: + details += [f"### `{name}`: {len(fixed)} now first", ""] + header + fixed + [""] + + misses = [ + f"| {query} | {row['expected']} | {row['found']} |" + for query, row in rows.items() + if not row["first"] + ] + if misses: + details += [ + f"
{name}: {len(misses)} not first", + "", + "| Query | Want | Got (top 3) |", + "| --- | --- | --- |", + *misses, + "", + "
", + "", + ] + + out += ["", *details] + print("\n".join(out)) + + +if __name__ == "__main__": + main() diff --git a/src/commands.rs b/src/commands.rs new file mode 100644 index 0000000..e3ee362 --- /dev/null +++ b/src/commands.rs @@ -0,0 +1,795 @@ +//! `mapbox commands` — finding a command by what it does. +//! +//! `search` ranks every command against a description of a task; `list` is +//! every command with its summary, on one page. Both read the command +//! surface [`crate::schema`] builds, so there is no second description of +//! the CLI to keep in step, and both answer for people and agents alike: a +//! table at a terminal, `[{command, summary}]` under `json`. +//! +//! The search is local keyword matching and nothing more: no model, no +//! request, no index on disk. At a few dozen commands, scoring every one on +//! each call takes no measurable time, and anything smarter would be a +//! dependency to ship for a list a person could read in a minute. The cost +//! is that it only finds words the command's own text uses, or one of a +//! handful of synonyms for them — "upload a worker script" will not find +//! `deploy` — so the summaries are what decide how well it works. +//! +//! The query is a free string, so telemetry records its length and never +//! its text (see [`crate::telemetry_event`]). No token, no request. + +use std::collections::BTreeSet; + +use anyhow::Result; +use clap::{value_parser, Arg, ArgMatches, Command}; +use serde_json::{json, Value}; + +use crate::help_layout; +use crate::output::{self, Mode}; +use crate::schema::{self, CommandEntry}; +use crate::spec::ServiceSpec; + +pub const COMMAND: &str = "commands"; + +const DEFAULT_LIMIT: usize = 5; + +/// How much each part of a command counts toward a match, after +/// `cf cli search`'s: a word in the command's own name says far more than +/// the same word somewhere in one of its arguments' help. +const NAME_WEIGHT: f64 = 8.0; +const SUMMARY_WEIGHT: f64 = 5.0; +const DESCRIPTION_WEIGHT: f64 = 2.0; +/// The group's own one-line description. Many summaries are a few words +/// ("Tilequery", "Forward geocoding"), and the group is often where the +/// plainer word is: `tilesets` "query features at a point". +const GROUP_WEIGHT: f64 = 1.5; +const ARGUMENTS_WEIGHT: f64 = 0.5; +/// How much a term has to score in one command before it counts as +/// answered in full: an exact match anywhere but an argument's help. A word +/// found only there, or a near miss, answers it in part, so a passing +/// mention cannot lift a command to full coverage of the query. +const FULL_MATCH: f64 = GROUP_WEIGHT; +/// Taken off for each word of a command's name the query did not ask for, +/// so "delete a style" finds `styles delete` ahead of `styles draft +/// delete`, whose summary and description match it just as well. +const UNASKED_NAME_WORD: f64 = 1.0; + +/// What a `batch` command's score is scaled by when the query did not ask +/// for many. +const UNASKED_BATCH: f64 = 0.5; + +/// A match this far below the best one is noise beside it. +/// +/// Relative only. A fixed minimum also emptied the answer for tasks this CLI +/// cannot do, but it emptied it just as readily for ones it can, worded +/// loosely: "how far can I walk in ten minutes" finds `isochrone` with one +/// word. A wrong guess costs the caller a look; no answer when there was one +/// costs them the command. +const MIN_SHARE_OF_BEST: f64 = 0.1; + +/// Words that describe the asking rather than the task. Left in, "how do I +/// list my styles" would rank whatever says "my" most. +/// +/// "mapbox" is one too: every command is Mapbox's, so it only rewards the +/// commands whose text happens to repeat the name. +const STOPWORDS: &[&str] = &[ + "a", "an", "and", "are", "as", "at", "be", "by", "can", "do", "for", "from", "how", "i", "in", + "into", "is", "it", "mapbox", "me", "my", "of", "on", "or", "the", "this", "to", "up", "want", + "what", "with", "you", "your", +]; + +/// Words people use for something the commands' own text says another way, +/// each with the words it stands in for. One way only: "remove" finds +/// `delete`, but "delete" does not go looking for "remove". Keys are in the +/// form [`tokens`] leaves them, plural `s` gone. +const SYNONYMS: &[(&str, &[&str])] = &[ + ("signin", &["login"]), + ("signout", &["logout"]), + ("remove", &["delete"]), + ("discard", &["delete"]), + ("erase", &["delete"]), + ("edit", &["update"]), + ("rename", &["update"]), + ("modify", &["update"]), + ("change", &["update"]), + // Not "many" or "several": they as often describe what comes back + // ("a reverse lookup that returns several results") as what goes in. + ("bulk", &["multiple", "batch"]), + ("picture", &["image"]), + ("png", &["image"]), + ("jpg", &["image"]), + ("thumbnail", &["image"]), + ("screenshot", &["image"]), + ("permission", &["scope"]), + ("saved", &["stored"]), + ("add", &["upload"]), +]; + +/// A synonym's match counts for less than the word itself, so a command +/// that uses the caller's own word still comes first. +const SYNONYM_WEIGHT: f64 = 0.8; + +pub fn command() -> Command { + Command::new(COMMAND) + .about("Find a command by what it does") + .long_about( + "Find a command by what it does. `search` ranks every command against a \ + description of a task; `list` shows every command with its summary. \ + Both are local: no request and no token.", + ) + .subcommand_required(true) + .subcommand( + Command::new("search") + .about("Find the commands that match a task, best first") + .arg( + Arg::new("query") + .required(true) + .num_args(1..) + .help("What you want to do, in a few words: \"upload a font\""), + ) + .arg( + Arg::new("limit") + .long("limit") + .value_parser(value_parser!(usize)) + .default_value(DEFAULT_LIMIT.to_string()) + .help("How many matches to show; 0 shows every match"), + ), + ) + .subcommand(Command::new("list").about("List every command with its summary")) +} + +pub fn search( + app: &Command, + specs: &[ServiceSpec], + matches: &ArgMatches, + mode: Mode, +) -> Result<()> { + let query = matches + .get_many::("query") + .expect("required") + .map(String::as_str) + .collect::>() + .join(" "); + let limit = *matches.get_one::("limit").expect("has a default"); + + let mut found = rank(app, specs, &query); + if limit > 0 { + found.truncate(limit); + } + if found.is_empty() { + output::progress(&format!( + "No command matches \"{query}\". `mapbox commands list` shows every command." + )); + } + emit(mode, &found, !found.is_empty()) +} + +pub fn list(app: &Command, specs: &[ServiceSpec], mode: Mode) -> Result<()> { + let mut all = schema::build(app, specs, &[]).commands; + all.sort_by(|a, b| a.command.cmp(&b.command)); + emit(mode, &all, true) +} + +/// The commands that match `query`, best first, down to +/// [`MIN_SHARE_OF_BEST`]. +fn rank(app: &Command, specs: &[ServiceSpec], query: &str) -> Vec { + let index = index(app, specs); + let order = rank_in(&index, query); + let mut entries: Vec> = + index.into_iter().map(|(_, entry)| Some(entry)).collect(); + order + .into_iter() + .map(|i| entries[i].take().expect("each position once")) + .collect() +} + +/// Every command `search` can answer with, and its words. Built once per +/// run, or once for a whole test set. +fn index(app: &Command, specs: &[ServiceSpec]) -> Vec<(Fields, CommandEntry)> { + schema::build(app, specs, &[]) + .commands + .into_iter() + // The caller is already here, and "find a command" would otherwise + // rank this group's own summaries first. + .filter(|entry| entry.service != COMMAND) + .map(|entry| (Fields::of(app, &entry), entry)) + .collect() +} + +/// Positions in `index` of the commands that match `query`, best first. +/// Ties go to the shorter command name, then alphabetically, so the same +/// query always answers the same way. +fn rank_in(index: &[(Fields, CommandEntry)], query: &str) -> Vec { + let query = Query::parse(query); + if query.terms.is_empty() { + return vec![]; + } + let mut scored: Vec<(f64, usize)> = index + .iter() + .enumerate() + .map(|(i, (fields, entry))| (score(&query, fields, entry.deprecated), i)) + .collect(); + scored.sort_by(|(a_score, a), (b_score, b)| { + let (a, b) = (&index[*a].1.command, &index[*b].1.command); + b_score + .total_cmp(a_score) + .then(a.len().cmp(&b.len())) + .then(a.cmp(b)) + }); + above_floor(scored).into_iter().map(|(_, i)| i).collect() +} + +/// `scored` sorted best first, without the zero scores and the ones too far +/// below the best. +fn above_floor(scored: Vec<(f64, T)>) -> Vec<(f64, T)> { + let floor = scored + .first() + .map_or(0.0, |(best, _)| best * MIN_SHARE_OF_BEST); + scored + .into_iter() + .take_while(|(score, _)| *score > 0.0 && *score >= floor) + .collect() +} + +struct Query { + terms: Vec, + /// Neighboring words written together, "log out" as "logout", tried + /// only for an exact match, with the two words they were made from. + /// Kept apart from `terms` so that one that matches nothing — almost all + /// of them — does not count as a word of the query going unanswered. + compounds: Vec<(String, [String; 2])>, +} + +impl Query { + fn parse(query: &str) -> Self { + let all: Vec = tokens(query).collect(); + // Before stopwords come out: the "in" of "log in" is one. + let compounds = all + .windows(2) + .map(|pair| (pair.concat(), [pair[0].clone(), pair[1].clone()])) + .collect(); + Query { + terms: terms(query), + compounds, + } + } +} + +/// A command's text, as the words each weighted part of it holds. +struct Fields { + name: BTreeSet, + parts: Vec<(f64, BTreeSet)>, +} + +impl Fields { + fn of(app: &Command, entry: &CommandEntry) -> Self { + let name = entry + .command + .strip_prefix("mapbox ") + .unwrap_or(&entry.command); + // The help list's line before the command's own `about`: for an API + // group, `about` is the spec's title ("Mapbox Tokens API"), which + // names the API rather than what it does. + let group = help_layout::description(&entry.service) + .map(ToString::to_string) + .or_else(|| { + app.find_subcommand(&entry.service) + .and_then(Command::get_about) + .map(ToString::to_string) + }) + .unwrap_or_default(); + let arguments = entry + .arguments + .iter() + .flat_map(|arg| [Some(arg.name.as_str()), arg.description.as_deref()]) + .flatten() + .collect::>() + .join(" "); + Fields { + name: words(name), + parts: vec![ + (NAME_WEIGHT, words(name)), + (SUMMARY_WEIGHT, words(&entry.summary)), + ( + DESCRIPTION_WEIGHT, + words(entry.description.as_deref().unwrap_or_default()), + ), + (GROUP_WEIGHT, words(&group)), + (ARGUMENTS_WEIGHT, words(&arguments)), + ], + } + } +} + +/// Each term counts once per part, at its best match there, so a long +/// description does not outrank a short summary by repeating a word. The +/// total is then scaled by how much of the query matched (see +/// [`FULL_MATCH`]): a command that answers the whole task beats one that +/// answers half of it loudly. +fn score(query: &Query, fields: &Fields, deprecated: bool) -> f64 { + let mut total = 0.0; + // A matched compound answers the words it was made from, which on + // their own usually match nothing: "out" of "log out". + let mut answered = BTreeSet::new(); + for (compound, parts) in &query.compounds { + for (weight, words) in &fields.parts { + let best = words + .iter() + .map(|word| exact_or_synonym(compound, word)) + .fold(0.0, f64::max); + if best > 0.0 { + total += weight * best; + answered.extend(parts.iter().cloned()); + } + } + } + let mut matched = 0.0; + for term in &query.terms { + let mut term_total = 0.0; + for (weight, words) in &fields.parts { + let best = words + .iter() + .map(|word| relatedness(term, word)) + .fold(0.0, f64::max); + term_total += weight * best; + } + matched += if answered.contains(term) { + 1.0 + } else { + (term_total / FULL_MATCH).min(1.0) + }; + total += term_total; + } + let unasked = fields + .name + .iter() + .filter(|word| { + query + .terms + .iter() + .all(|term| relatedness(term, word) == 0.0) + && !query + .compounds + .iter() + .any(|(compound, _)| exact_or_synonym(compound, word) > 0.0) + }) + .count(); + // A `batch` command does what a sibling does, for many inputs at once, + // and its text says so at length: "forward or reverse", every option the + // sibling has. Unless the query asks for many, the sibling is the answer. + let unasked_batch = fields.name.contains("batch") + && !query + .terms + .iter() + .any(|term| relatedness(term, "batch") > 0.0 || relatedness(term, "multiple") > 0.0); + let total = (total - UNASKED_NAME_WORD * unasked as f64).max(0.0); + let total = if unasked_batch { + total * UNASKED_BATCH + } else { + total + }; + let coverage = matched / query.terms.len() as f64; + // Still found, so a script that names one can be told what it was, but + // below anything it would be better to start using. + let standing = if deprecated { 0.5 } else { 1.0 }; + total * coverage * coverage * standing +} + +/// [`similarity`], or a synonym's exact match if that is closer. A synonym +/// is not fuzzy-matched: it is already a guess, and "batch" one letter off +/// is "match". +fn relatedness(term: &str, word: &str) -> f64 { + let direct = similarity(term, word); + if synonyms(term).contains(&word) { + direct.max(SYNONYM_WEIGHT) + } else { + direct + } +} + +/// Compounds are only ever matched whole: "logout", not a word it starts. +fn exact_or_synonym(compound: &str, word: &str) -> f64 { + if compound == word { + 1.0 + } else if synonyms(compound).contains(&word) { + SYNONYM_WEIGHT + } else { + 0.0 + } +} + +fn synonyms(term: &str) -> &'static [&'static str] { + SYNONYMS + .iter() + .find(|(key, _)| *key == term) + .map_or(&[], |(_, synonyms)| synonyms) +} + +/// How well a query term matches one word of a command's text, from 0 to 1. +fn similarity(term: &str, word: &str) -> f64 { + if term == word { + return 1.0; + } + // Typing the start of a word: "geo" for "geocoder". + if term.len() >= 3 && word.starts_with(term) { + return 0.7; + } + // The same stem, spelled on for a different ending: "geocoding" and + // "geocoder". + let shared = term + .chars() + .zip(word.chars()) + .take_while(|(a, b)| a == b) + .count(); + if shared >= 5 { + return 0.5; + } + // A typo. Short words are left out: up to four letters, one edit turns + // too many words into another real one — "shop" into "show" — and up to + // seven, two do — "convert" into "content". + let edits = strsim::damerau_levenshtein(term, word); + let allowed = match term.len().min(word.len()) { + 0..=4 => 0, + 5..=7 => 1, + _ => 2, + }; + if edits <= allowed { + return 0.4; + } + 0.0 +} + +/// The query's words, minus the ones that only describe the asking. +fn terms(query: &str) -> Vec { + let mut seen = BTreeSet::new(); + raw_tokens(query) + // Before the plural `s` comes off, which would leave "this" as "thi". + .filter(|word| !STOPWORDS.contains(&word.as_str())) + .map(singular) + .filter(|word| seen.insert(word.clone())) + .collect() +} + +fn words(text: &str) -> BTreeSet { + tokens(text).collect() +} + +/// Lowercased runs of letters and digits, each with a plural `s` taken off, +/// so "styles" in a query matches "style" in a summary. Cruder than +/// stemming, and enough for the words this CLI's commands use. +fn tokens(text: &str) -> impl Iterator + '_ { + raw_tokens(text).map(singular) +} + +fn raw_tokens(text: &str) -> impl Iterator + '_ { + text.split(|c: char| !c.is_alphanumeric()) + .filter(|word| !word.is_empty()) + .map(str::to_lowercase) +} + +fn singular(word: String) -> String { + match word.strip_suffix('s') { + Some(stem) if stem.len() >= 3 && !stem.ends_with('s') => stem.to_string(), + _ => word, + } +} + +fn emit(mode: Mode, entries: &[CommandEntry], next_step: bool) -> Result<()> { + let width = entries + .iter() + .map(|entry| entry.command.len()) + .max() + .unwrap_or(0) + .max("COMMAND".len()); + let text = if entries.is_empty() { + String::new() + } else { + std::iter::once(format!("{:width$} SUMMARY", "COMMAND")) + .chain( + entries + .iter() + .map(|entry| format!("{:width$} {}", entry.command, entry.summary)), + ) + .collect::>() + .join("\n") + }; + let json = entries + .iter() + .map(|entry| json!({ "command": entry.command, "summary": entry.summary })) + .collect(); + output::emit(mode, &text, Value::Array(json))?; + if next_step && !mode.is_json() { + output::print_tips(&["`mapbox --help` shows what one takes.".to_string()]); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn top(query: &str) -> Vec { + let specs = crate::spec::effective_services().expect("specs load"); + let app = crate::build_app(&specs); + rank(&app, &specs, query) + .into_iter() + .take(3) + .map(|entry| entry.command) + .collect() + } + + /// A query set under `tests/fixtures/commands_search/`, and the fewest + /// rows each must keep answering right: first result, and anywhere in + /// the first three. A `-` row is right when nothing comes back. + /// + /// The floors are the scores when they were last raised. A change that + /// improves one raises its floor in the same commit, so the gain cannot + /// quietly be given back; one that lowers a floor says why in its PR. + /// New real queries go in a new file rather than into these: a set the + /// search has been tuned against stops saying how it does on the next + /// query nobody has seen. + const EVAL_SETS: &[(&str, &str, usize, usize)] = &[ + ( + "tuning.tsv", + include_str!("../tests/fixtures/commands_search/tuning.tsv"), + 84, + 97, + ), + ( + "issues.tsv", + include_str!("../tests/fixtures/commands_search/issues.tsv"), + 50, + 67, + ), + ( + "docs.tsv", + include_str!("../tests/fixtures/commands_search/docs.tsv"), + 22, + 30, + ), + ]; + + /// Where [`eval_set_keeps_its_scores`] writes its reports, when set. + const REPORT_DIR_ENV: &str = "MAPBOX_SEARCH_EVAL_REPORT"; + + /// Holds one set to its floors. `cargo test eval_set -- --nocapture` + /// prints the scores, and each query that did not rank first. One test + /// per set, so they run in parallel. + /// + /// With [`REPORT_DIR_ENV`] set, also writes every query's result to + /// `/`, before the floors are checked so a failing run still + /// leaves one. CI diffs a PR's reports against its base's. + fn eval_set_keeps_its_scores(wanted: &str) { + let specs = crate::spec::effective_services().expect("specs load"); + let app = crate::build_app(&specs); + let known: BTreeSet = schema::build(&app, &specs, &[]) + .commands + .into_iter() + .map(|entry| entry.command) + .collect(); + let index = index(&app, &specs); + + let mut failures = vec![]; + for (name, text, min_first, min_top3) in EVAL_SETS.iter().filter(|(n, ..)| *n == wanted) { + let (mut rows, mut first, mut top3) = (0, 0, 0); + let mut misses = vec![]; + let mut report = format!("# floors\t{min_first}\t{min_top3}\n"); + for line in text + .lines() + .filter(|l| !l.starts_with('#') && !l.trim().is_empty()) + { + let mut columns = line.split('\t'); + let query = columns.next().expect("a query"); + let expected: Vec<&str> = match columns.next().expect("an expected column") { + "-" => vec![], + answers => answers.split('|').collect(), + }; + // A renamed command would leave its rows unwinnable, and + // the floor would read that as the search getting worse. + for command in &expected { + assert!( + known.contains(*command), + "{name}: `{command}` is not a command (\"{query}\")" + ); + } + let found: Vec = rank_in(&index, query) + .into_iter() + .take(3) + .map(|i| index[i].1.command.clone()) + .collect(); + let (is_first, in_top3) = if expected.is_empty() { + (found.is_empty(), found.is_empty()) + } else { + ( + found + .first() + .is_some_and(|c| expected.contains(&c.as_str())), + found.iter().any(|c| expected.contains(&c.as_str())), + ) + }; + report.push_str(&format!( + "{query}\t{}\t{}\t{}\t{}\n", + expected.join("|"), + found.join("|"), + u8::from(is_first), + u8::from(in_top3) + )); + rows += 1; + first += usize::from(is_first); + top3 += usize::from(in_top3); + if !is_first { + misses.push(format!(" \"{query}\": want {expected:?}, got {found:?}")); + } + } + if let Some(dir) = std::env::var_os(REPORT_DIR_ENV) { + let dir = std::path::PathBuf::from(dir); + std::fs::create_dir_all(&dir).expect("create the report directory"); + std::fs::write(dir.join(name), report).expect("write the report"); + } + eprintln!( + "{name}: {first}/{rows} first (floor {min_first}), \ + {top3}/{rows} in top 3 (floor {min_top3})" + ); + if first < *min_first || top3 < *min_top3 { + failures.push(format!( + "{name} fell below its floor: {first} first (floor {min_first}), \ + {top3} in top 3 (floor {min_top3}). If the trade-off is intended — \ + help text made accurate at the cost of a match, say — lower the floor \ + in EVAL_SETS to {}/{} and say why in the PR. Not first:\n{}", + first.min(*min_first), + top3.min(*min_top3), + misses.join("\n") + )); + } + if first > *min_first || top3 > *min_top3 { + eprintln!(" above its floor: raise it to {first}/{top3} in EVAL_SETS"); + } + } + assert!(failures.is_empty(), "{}", failures.join("\n\n")); + } + + #[test] + fn eval_set_tuning() { + eval_set_keeps_its_scores("tuning.tsv"); + } + + #[test] + fn eval_set_issues() { + eval_set_keeps_its_scores("issues.tsv"); + } + + #[test] + fn eval_set_docs() { + eval_set_keeps_its_scores("docs.tsv"); + } + + /// Phrasings a person or an agent might actually type, each held to the + /// command it should find first. A change to the scoring that loses one + /// of these has made the search worse for that caller. + #[test] + fn a_task_finds_its_command_first() { + for (query, expected) in [ + ("upload a font", "mapbox fonts upload"), + ("list my styles", "mapbox styles list"), + ("delete a style", "mapbox styles delete"), + ("geocode an address", "mapbox geocoder forward"), + ("driving route between two points", "mapbox directions"), + ("travel time matrix", "mapbox matrix"), + ("snap gps trace to roads", "mapbox map-match"), + ("static map image", "mapbox static get-image"), + ("log in", "mapbox auth login"), + ("list access tokens", "mapbox accounts list-tokens"), + ("shell completion", "mapbox completion"), + ("install mcp server", "mapbox mcp install"), + ("find coffee shops near me", "mapbox search category"), + ("query features at a point", "mapbox tilesets query"), + ("vector tile", "mapbox tilesets get-mvt"), + ("add an icon to a sprite", "mapbox sprites upload"), + ("download a style", "mapbox styles download"), + ("reachable area in 10 minutes", "mapbox isochrone"), + ("feedback about the cli", "mapbox report"), + ("sign in", "mapbox auth login"), + ("discard a style draft", "mapbox styles draft delete"), + ("rename my style", "mapbox styles update"), + ( + "bulk upload icons to a sprite", + "mapbox sprites upload-batch", + ), + ("add a ttf font", "mapbox fonts upload"), + ("publish a tileset", "mapbox tilesets-cli"), + ("turn off telemetry", "mapbox config set"), + ("check my proxy settings", "mapbox doctor"), + ("uninstall the cli", "mapbox uninstall"), + ] { + let found = top(query); + assert_eq!( + found.first().map(String::as_str), + Some(expected), + "\"{query}\" ranked {found:?}" + ); + } + } + + #[test] + fn a_typo_still_finds_the_command() { + assert_eq!( + top("uplaod font").first().map(String::as_str), + Some("mapbox fonts upload") + ); + } + + #[test] + fn a_query_of_only_stopwords_matches_nothing() { + assert!(top("how do I").is_empty()); + } + + #[test] + fn nonsense_matches_nothing() { + assert!(top("xyzzy").is_empty()); + } + + #[test] + fn matches_far_below_the_best_are_dropped() { + let kept: Vec<&str> = above_floor(vec![(10.0, "best"), (1.0, "tenth"), (0.9, "noise")]) + .into_iter() + .map(|(_, name)| name) + .collect(); + assert_eq!(kept, ["best", "tenth"]); + } + + /// A loosely worded task matched by one word is still answered: a + /// fixed minimum score once emptied this. + #[test] + fn a_weak_only_match_is_still_an_answer() { + assert_eq!( + top("how far can i walk in ten minutes") + .first() + .map(String::as_str), + Some("mapbox isochrone") + ); + } + + /// `geocoder batch`'s description names everything a single lookup + /// does, so without the discount it ranks first for a single query. + #[test] + fn batch_comes_second_unless_asked_for() { + assert_eq!( + top("geocoding results inside a bbox") + .first() + .map(String::as_str), + Some("mapbox geocoder forward") + ); + assert_eq!( + top("bulk geocode addresses").first().map(String::as_str), + Some("mapbox geocoder batch") + ); + } + + #[test] + fn stopwords_are_dropped_before_the_plural_is() { + assert!(terms("what is this").is_empty()); + } + + #[test] + fn a_synonym_counts_for_less_than_the_word_itself() { + assert_eq!(relatedness("remove", "remove"), 1.0); + assert_eq!(relatedness("remove", "delete"), SYNONYM_WEIGHT); + assert_eq!(relatedness("delete", "remove"), 0.0); + } + + /// "batch" is one letter from "match"; a synonym is matched exactly. + #[test] + fn a_synonym_is_not_fuzzy_matched() { + assert_eq!(relatedness("many", "match"), 0.0); + } + + #[test] + fn a_plural_matches_its_singular() { + assert!(similarity(&terms("styles")[0], &terms("style")[0]) == 1.0); + } + + #[test] + fn short_words_are_not_fuzzy_matched() { + assert_eq!(similarity("map", "mcp"), 0.0); + assert_eq!(similarity("shop", "show"), 0.0); + } +} diff --git a/src/config.rs b/src/config.rs index c924efe..57ce7b8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -182,7 +182,7 @@ pub fn command() -> Command { }; Command::new(COMMAND) - .about("Get or set a persisted mapbox setting") + .about("Get or set persisted settings: telemetry, update checks, history and logs") .long_about( "Get or set a mapbox setting that persists across shells and sessions, \ written to a file beside the stored credentials rather than an \ @@ -196,7 +196,7 @@ pub fn command() -> Command { ) .subcommand( Command::new("set") - .about("Persist a setting") + .about("Turn a setting on or off, and keep it that way") .arg(key_arg()) .arg(Arg::new("value").required(true).value_parser([ON, OFF])), ) diff --git a/src/doctor.rs b/src/doctor.rs index ab91fb3..962e236 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -107,13 +107,15 @@ const PROXY_VARS: &[(&str, &str)] = &[ pub fn command() -> Command { Command::new(COMMAND) - .about("Show the environment the next command would run in") + .about("Show the token, proxy and settings the next command would run with") .long_about( "A read-only snapshot of what the next command would see: which token wins \ and its state, which proxy variables are in effect, and where the \ update-check and telemetry switches currently stand. `--verify` additionally \ checks that api.mapbox.com is reachable — the one part of this that makes a \ - request.", + request.\n\n\ + Start here when requests fail for no obvious reason: a missing or unexpected \ + token, or a network, proxy or firewall in the way.", ) .arg( Arg::new("verify") diff --git a/src/help_layout.rs b/src/help_layout.rs index 99acdde..e7e3cc4 100644 --- a/src/help_layout.rs +++ b/src/help_layout.rs @@ -45,6 +45,7 @@ const GROUPS: &[(&str, &[&str])] = &[ ( "CLI", &[ + "commands", "config", "history", "doctor", @@ -80,7 +81,10 @@ const DESCRIPTIONS: &[(&str, &str)] = &[ "places", "Full detail for a place, by its Search Box result id", ), - ("feedback", "Submit and list feedback about Mapbox data"), + ( + "feedback", + "Report wrong or missing map data to Mapbox, and list what was reported", + ), ("directions", "Route between waypoints"), ( "matrix", @@ -416,7 +420,9 @@ fn render(sections: &[Section], width: usize, palette: &Palette) -> String { let lit = palette.literal; page.push_str(&format!( "{muted}Run{muted:#} {lit}mapbox --help{lit:#} \ - {muted}for a command's own options.{muted:#}", + {muted}for a command's own options.{muted:#}\n\ + {muted}Run{muted:#} {lit}mapbox commands search \"\"{lit:#} \ + {muted}to find a command by what it does.{muted:#}", muted = palette.muted, )); page @@ -634,7 +640,9 @@ fn grouped(rows: Vec<(String, String)>) -> Vec<(&'static str, Vec<(String, Strin groups } -fn description(name: &str) -> Option<&'static str> { +/// Also read by `commands search`, for which these are the plainest words +/// a group has. +pub(crate) fn description(name: &str) -> Option<&'static str> { DESCRIPTIONS .iter() .find(|(n, _)| *n == name) diff --git a/src/history.rs b/src/history.rs index b0cb9d8..0133cac 100644 --- a/src/history.rs +++ b/src/history.rs @@ -36,7 +36,7 @@ pub fn command() -> Command { .subcommand_required(true) .subcommand( Command::new("list") - .about("List the most recent runs, newest first") + .about("List the commands you ran recently, newest first") .arg( Arg::new("limit") .long("limit") @@ -47,7 +47,7 @@ pub fn command() -> Command { ) .subcommand( Command::new("show") - .about("Show everything recorded about one run") + .about("Show everything recorded about one run, the last one by default") .arg( Arg::new("id") .help("The run's id, or a prefix of it; the newest run when left out"), diff --git a/src/main.rs b/src/main.rs index 4c1823e..e6b81a1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,6 +26,7 @@ mod agent_skills; mod api_command_surface; mod auth; mod cli_token; +mod commands; mod completion; mod config; mod confirm; @@ -71,8 +72,9 @@ const REPO_URL: &str = repo_url!(); /// by chaining `--help` calls, one command at a time; `--schema` answers the /// same question for every command in one document. const AGENT_HELP_HINT: &str = "\ -Agents: `mapbox --schema` describes every command, its arguments and the request it makes -as one JSON document. Use it instead of chaining --help calls."; +Agents: `mapbox commands search \"\"` finds the command for a task. `mapbox --schema` +describes every command, its arguments and the request it makes as one JSON document. Use +these instead of chaining --help calls."; /// Help headings for the global options, which otherwise share one /// "Options:" list with every command's own. @@ -784,6 +786,10 @@ fn build_app(specs: &[ServiceSpec]) -> Command { // Beside `config`, which turns the history it reads on and off. app = app.subcommand(history::command()); + // Reads the same tree `--schema` does, so it is built from the commands + // above rather than listing them itself. + app = app.subcommand(commands::command()); + // Reads what the other hand-written commands above also read — the // token store, the proxy environment, the config and telemetry // switches — so it belongs beside them rather than the API surface @@ -1032,6 +1038,9 @@ fn report_parse_result(err: clap::Error, raw_argv: &[std::ffi::OsString]) -> u8 run_record::add_stdout_bytes(err.render().to_string().len()); } let _ = err.print(); + if let Some(fix) = search_fix(&err, &rendered) { + output::print_tips(&[fix.to_string()]); + } return code; } @@ -1054,8 +1063,11 @@ fn report_parse_result(err: clap::Error, raw_argv: &[std::ffi::OsString]) -> u8 // answers that, and inventing one would be worse than the message // alone — but clap's own suggester sometimes already has the answer, // and `-o text` shows it; carry it over rather than losing it here. - _ => match clap_tip(&rendered) { - Some(tip) => Remedy::default().with_fix(&tip), + _ => match clap_tip(&rendered) + .as_deref() + .or(search_fix(&err, &rendered)) + { + Some(fix) => Remedy::default().with_fix(fix), None => Remedy::default(), }, }); @@ -1064,6 +1076,16 @@ fn report_parse_result(err: clap::Error, raw_argv: &[std::ffi::OsString]) -> u8 code } +/// Where to go after a command name nothing resembles, which is usually a +/// guess at what the command is called — the question `commands search` +/// answers. A fix rather than an action: the query is the caller's to write. +/// Nothing when clap already suggested a near spelling, which is the better +/// answer. +fn search_fix(err: &clap::Error, rendered: &str) -> Option<&'static str> { + (err.kind() == ErrorKind::InvalidSubcommand && clap_tip(rendered).is_none()) + .then_some("To find a command by what it does: mapbox commands search \"\"") +} + /// The `tip: …` line clap's own suggester renders for an unrecognized /// subcommand or flag, if there is one. /// @@ -1395,6 +1417,14 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - Some(("show", show_matches)) => history::show(show_matches, mode)?, _ => unreachable!("`history` sets subcommand_required(true)"), }, + // Ahead of the generic service arm too, and handed `app` for the + // same reason `generate-skills` is: it reads the command tree already + // in memory. No token and no request. + Some((commands::COMMAND, commands_matches)) => match commands_matches.subcommand() { + Some(("search", search_matches)) => commands::search(app, specs, search_matches, mode)?, + Some(("list", _)) => commands::list(app, specs, mode)?, + _ => unreachable!("`commands` sets subcommand_required(true)"), + }, // Also ahead of the generic service arm: read-only except for the // opt-in `--verify` request, and needs no credential load of its own // — it reports what one would resolve to, not what a fresh one diff --git a/src/mcp.rs b/src/mcp.rs index ac11282..eb21565 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -354,6 +354,11 @@ pub fn command() -> ClapCommand { .subcommand( ClapCommand::new(INSTALL) .about("Register a server with a client. With neither flag, every known server for every detected client") + .long_about( + "Register the Mapbox MCP and Mapbox DevKit MCP servers with a coding agent: \ + Claude Code, Codex, VS Code or Cursor. With neither flag, registers every known \ + server with every detected client.", + ) .arg( Arg::new(SERVER_ARG) .long(SERVER_ARG) diff --git a/src/output/mod.rs b/src/output/mod.rs index dc84b18..bab5b68 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -324,7 +324,7 @@ pub fn progress(message: &str) { /// caller needs no guard. A blank line first — these are notes about /// whatever was just printed, not more of it, and butted against the last /// line they'd read as one. -fn print_tips(tips: &[String]) { +pub(crate) fn print_tips(tips: &[String]) { if tips.is_empty() { return; } diff --git a/src/schema.rs b/src/schema.rs index 2f8387d..d034072 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -394,6 +394,14 @@ fn commands(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec Command { Command::new(COMMAND) - .about("Run the Mapbox Tilesets CLI (`tilesets`, installed separately)") + .about("Run the Mapbox Tilesets CLI (installed separately): upload data, publish tilesets") .long_about(format!( - "Forward arguments to the Mapbox Tilesets CLI.\n\n\ + "Forward arguments to the Mapbox Tilesets CLI, the command-line client for \ + Mapbox Tiling Service (MTS): upload GeoJSON as a tileset source, validate a \ + recipe, create and publish a tileset, and check on its jobs. It also lists, \ + updates and deletes tilesets and tileset sources.\n\n\ Everything after `{COMMAND}` is passed through to the `tilesets` binary \ untouched — including flags, so `{COMMAND} --help` shows the Tilesets \ CLI's own help rather than this one. `tilesets` ships separately as the \ diff --git a/src/uninstall.rs b/src/uninstall.rs index 42449e3..be90736 100644 --- a/src/uninstall.rs +++ b/src/uninstall.rs @@ -31,7 +31,7 @@ const DRY_RUN_HELP: &str = "Describe what this would delete, then exit without d pub fn command() -> Command { Command::new(COMMAND) - .about("Remove the installed `mapbox` binary") + .about("Uninstall this CLI by removing the installed `mapbox` binary") .long_about( "Remove the `mapbox` binary this process is running from.\n\n\ Deletes only the executable. Stored credentials, `auth logout`'s file, \ diff --git a/tests/fixtures/commands_search/README.md b/tests/fixtures/commands_search/README.md new file mode 100644 index 0000000..81a5976 --- /dev/null +++ b/tests/fixtures/commands_search/README.md @@ -0,0 +1,74 @@ +# `commands search` query sets + +Queries that `mapbox commands search` is scored against. The `eval_set_*` +tests in `src/commands.rs` run every file here and fail when a set ranks +fewer queries right than its floor. `cargo test eval_set -- --nocapture` +prints each set's score and every query that did not rank first. On a PR, +CI's `commands search eval` job puts the scores in its job summary, with the +queries the change fixed and broke against the base branch. + +## Format + +Tab-separated, one query per line, `#` lines are comments: + +| Column | Meaning | +| --- | --- | +| query | What a person or an agent types, as one string. | +| expected | The command that should rank first, as `mapbox …`. `\|` separates several acceptable answers; `-` means the CLI cannot do this and nothing should come back. | +| kind | What sort of query it is (below). | +| source | Where the phrasing came from (`issues.tsv` and `docs.tsv` only). | + +A query counts as right when its first result is one of the expected +commands, or, for `-`, when the result is empty. Top 3 counts it when an +expected command is anywhere in the first three. + +## The sets + +The sets differ mostly in how much the search has been tuned against them. +The more it was, the better the score looks and the less it says about a +query nobody has seen. + +| File | Rows | Source | Tuned against | +| --- | --- | --- | --- | +| `tuning.tsv` | 100 | Written by hand while building the search | Heavily: scoring and help text were changed until these ranked well | +| `issues.tsv` | 100 | Mapbox GitHub issue and help-center titles, lightly rewritten | Partly: its misses were read to choose which kinds of fix to make | +| `docs.tsv` | 60 | Task phrasings from Mapbox docs, blog posts and tutorials | Least: unseen until the last round, then used once to choose between two changes | + +Kinds: + +- `agent`: worded the way the CLI's own text is, as an agent that has read + `--help` would ("get sprite json"). +- `human`: a person's own words ("which fonts do I have"). +- `synonym`: a word the command's text says differently ("rename my style"). +- `typo`: a realistic misspelling ("uplaod a font"). +- `symptom`: a bug report rather than a task ("estimate area returns 0 km2"). + The search is not built for these; they are a secondary measure. +- `none`: a task the CLI cannot do ("build a store locator"). + +## Reading the scores + +- A drop in `tuning.tsv` or `issues.tsv` means something that worked broke. +- `docs.tsv` is the best estimate of how the search does on unseen + phrasing. A gain there is the one worth believing. +- Keyword search has a ceiling: a query that shares no words with any + command's text cannot be found. Most of the remaining misses are of that + kind, and tuning will not fix them. + +## Limits + +- Stack Overflow, GIS StackExchange and Reddit could not be fetched while + `issues.tsv` and `docs.tsv` were gathered, so neither has queries from + them, and both lean toward developer phrasing. +- Many `docs.tsv` sources are a page title seen in search results, not a page + that was read in full. +- The expected command is a judgment call for some rows, which is why some + list more than one. + +## Changing them + +- Raise a set's floor in `EVAL_SETS` when a change improves it, in the same + commit. Lowering one is a decision to explain in the PR. +- Fix the kind of miss rather than special-casing a query. +- Put new real queries, such as ones the team actually typed, in a new file + rather than these, so there is always a set the search has not been + fitted to. Add it to `EVAL_SETS` with its own test. diff --git a/tests/fixtures/commands_search/docs.tsv b/tests/fixtures/commands_search/docs.tsv new file mode 100644 index 0000000..ceaa388 --- /dev/null +++ b/tests/fixtures/commands_search/docs.tsv @@ -0,0 +1,64 @@ +# Task phrasings from Mapbox docs, blog posts and tutorials, unseen until the +# last tuning round and then used once to choose between two changes. The +# least tuned-against of the three. Columns: query, expected first result, +# kind, source. +turn a list of customer addresses into coordinates mapbox geocoder batch|mapbox geocoder forward human https://hackersandslackers.com/preparing-data-for-mapbox-geocoding/ +find the address nearest to a gps point mapbox geocoder reverse|mapbox search reverse human https://docs.mapbox.com/help/dive-deeper/geocoding.md +look up a place name and get its coordinates mapbox geocoder forward|mapbox search forward human https://designcode.io/react-hooks-handbook-geocoding-mapbox +geocode addresses for a spreadsheet mapbox geocoder batch|mapbox geocoder forward human https://interworks.com/blog/2020/07/28/geocoding-in-tableau-prep-with-mapbox/ +search for an address in japan mapbox search forward|mapbox geocoder forward human https://docs.mapbox.com/help/troubleshooting/japan-specific-considerations-search-api.md +reverse lookup that returns several results instead of one mapbox geocoder reverse|mapbox search reverse human https://docs.mapbox.com/android/search/api/core/2.7.0/sdk/com.mapbox.search/-reverse-geo-options/limit.html +route that avoids toll roads mapbox directions human https://docs.mapbox.com/api/navigation/directions/ +cycling route with distance and duration mapbox directions human https://docs.mapbox.com/help/dive-deeper/directions.md +route through several stops in order mapbox directions human https://rdrr.io/cran/mapboxapi/man/mb_directions.html +driving time with live traffic mapbox directions human https://docs.mapbox.com/api/navigation/directions/ +add a pin to a static map mapbox static get-image human https://blog.mapbox.com/static-api-with-overlays-932ffc5fcf3d +map image url to use in an img tag mapbox static get-image human https://scour.ing/@tomasz/p/https://dev.to/mapbox/server-rendered-maps-for-web-and-mobile-51f7 +make a map picture for printing mapbox static get-image human https://docs.mapbox.com/help/dive-deeper/static-maps.md +static map with a custom icon marker mapbox static get-image human https://docs.mapbox.com/android/java/guides/static-image/ +export my studio style to a json file mapbox styles get|mapbox styles download human https://docs.mapbox.com/atlas/guides/styles/ +create a style from a json file mapbox styles create human https://docs.mapbox.com/style-spec/guides/ +change a published style without opening studio mapbox styles update human https://docs.mapbox.com/style-spec/guides/ +save work in progress on a style before publishing mapbox styles draft update human https://docs.mapbox.com/atlas/guides/styles/ +add svg icons to my style mapbox sprites upload|mapbox sprites upload-batch human https://docs.mapbox.com/studio-manual/examples/custom-icon +add a whole folder of svg icons at once mapbox sprites upload-batch human https://docs.mapbox.com/style-spec/reference/sprite.md +which fontstacks can i use for labels mapbox fonts list human https://docs.mapbox.com/api/maps/fonts/ +remove a font i uploaded by mistake mapbox fonts delete human https://docs.mapbox.com/api/maps/fonts/ +turn populated places data into vector tiles mapbox tilesets-cli human https://docs.mapbox.com/help/tutorials/get-started-mts-and-tilesets-cli.md +check a recipe is valid before using it mapbox tilesets-cli human https://pypi.org/project/mapbox-tilesets/1.5.0 +why did my tileset fail to publish mapbox tilesets-cli human https://docs.mapbox.com/mapbox-tiling-service/guides/errors/ +find out which source features were dropped mapbox tilesets-cli human https://docs.mapbox.com/mapbox-tiling-service/guides/frequentlyaskedquestions.md +pick the minimum and maximum zoom for my tiles mapbox tilesets-cli human https://docs.mapbox.com/mapbox-tiling-service/examples/basic-recipe-using-zoom-levels/ +which poi and building layers are within 30 meters of a point mapbox tilesets query human https://docs.mapbox.com/api/maps/tilequery.md +get the nearest features from a tileset around a location mapbox tilesets query human https://docs.mapbox.com/android/java/guides/services/tilequery/ +make a token that only works on my website - none https://docs.mapbox.com/help/dive-deeper/access-tokens/ +which scopes do i need to upload fonts mapbox accounts list-scopes human https://docs.mapbox.com/api/maps/fonts/ +what is the difference between public and secret tokens mapbox accounts list-scopes|mapbox accounts list-tokens human https://docs.mapbox.com/accounts/guides/tokens/ +see every token on my account and when it was created mapbox accounts list-tokens human https://docs.mapbox.com/accounts/guides/tokens/ +switch between two saved accounts mapbox auth profiles|mapbox auth login human https://docs.mapbox.com/cli/reference.md +take mapbox off my laptop mapbox uninstall human https://docs.mapbox.com/cli/reference/uninstall/ +tab completion for fish mapbox completion human https://docs.mapbox.com/cli/reference.md +disable the update check mapbox config set human https://docs.mapbox.com/cli/reference.md +what did i run yesterday mapbox history list human https://docs.mapbox.com/cli/reference.md +connect cursor to mapbox tools mapbox mcp install human https://cdn.jsdelivr.net/gh/mapbox/mcp-devkit-server@main/README.md +how many requests did i make last week mapbox usage human https://docs.mapbox.com/accounts/guides/statistics/ +which product is using most of my quota mapbox usage human https://docs.mapbox.com/help/faq/what-statistics-are-available-for-my-mapbox-account/ +how far can i walk in ten minutes mapbox isochrone human https://www.mapbox.com/insights/isochrones +delivery catchment area around a store mapbox isochrone human https://www.mapbox.com/blog/using-isochrones-to-understand-reachability-in-web-and-mobile-applications +durations from one warehouse to many customers mapbox matrix human https://docs.mapbox.com/api/navigation/matrix.md +pick the closest depot by drive time mapbox matrix|mapbox directions human https://blog.afi.io/blog/real-time-traffic-with-the-mapbox-api/ +fix gps jitter in a recorded ride mapbox map-match human https://docs.mapbox.com/playground/map-matching/ +find bookstores along my route mapbox search category human https://docs.mapbox.com/mapbox-search-js/api/core/search.md +get details and coordinates for a venue from a search result mapbox places human https://mapbox.com/blog/introducing-search-box +tell mapbox a road is missing from the map mapbox feedback create human https://docs.mapbox.com/help/dive-deeper/mapbox-data.md +check on a map problem i reported earlier mapbox feedback get|mapbox feedback list human https://docs.mapbox.com/help/dive-deeper/mapbox-data.md +show map labels in another language in gl js - none https://docs.mapbox.com/help/dive-deeper/change-language +show a popup when someone clicks a point - none https://docs.mapbox.com/mapbox-gl-js/example/popup-on-click/ +how much does mapbox cost per month - none https://docs.mapbox.com/accounts/overview/pricing/ +delete my mapbox account - none https://docs.mapbox.com/help/account/settings +add 3d buildings to my map - none https://docs.mapbox.com/help/dive-deeper/z_add-3d-buildings-studio.md +make a choropleth map - none https://docs.mapbox.com/help/tutorials/choropleth-studio-gl-pt-1.md +offline maps in an ios app - none https://docs.mapbox.com/help/tutorials/ios-offline-maps.md +toggle layers with checkboxes in react - none https://docs.mapbox.com/help/tutorials/react-toggle-layers.md +turn by turn navigation in my ios app - none https://docs.mapbox.com/help/tutorials/ios-navigation.md +geofencing in an android app - none https://docs.mapbox.com/help/tutorials/android-geofencing.md diff --git a/tests/fixtures/commands_search/issues.tsv b/tests/fixtures/commands_search/issues.tsv new file mode 100644 index 0000000..7c1dc79 --- /dev/null +++ b/tests/fixtures/commands_search/issues.tsv @@ -0,0 +1,104 @@ +# Lightly rewritten titles of Mapbox GitHub issues and help-center pages. Used +# to pick which kinds of fix to make, so partly tuned against. `symptom` rows +# are bug reports rather than tasks; `none` rows are tasks this CLI cannot do. +# Columns: query, expected first result, kind, source. +save static api response to a png file mapbox static get-image human https://github.com/mapbox/mapbox-sdk-js/issues/317 +static image request entity too large mapbox static get-image symptom https://github.com/mapbox/mapbox-sdk-js/issues/324 +add text to a static map image mapbox static get-image human https://docs.mapbox.com/help/tutorials/static-images-text +put a static map in an email receipt mapbox static get-image human https://docs.mapbox.com/help/tutorials/receipt-map +geojson polygon overlay on a static image mapbox static get-image human https://github.com/mapbox/mapbox-sdk-js/issues/283 +hide multiple layers in static images mapbox static get-image human https://github.com/mapbox/mapbox-gl-js/issues/12607 +offset a custom marker in a static map mapbox static get-image human https://github.com/mapbox/mapbox-sdk-js/issues/431 +static map with language and worldview mapbox static get-image human https://github.com/mapbox/mapbox-gl-js/issues/11005 +mpaboxstatic coordinates auto requires zoom mapbox static get-image symptom https://github.com/mapbox/mapbox-sdk-js/issues/268 +static tiles of a custom style look wrong mapbox static get-tile|mapbox static get-image symptom https://github.com/mapbox/mapbox-gl-js/issues/13504 +serving raster tiles source mapbox tilesets get-tile|mapbox static get-tile human https://github.com/mapbox/mapbox-gl-js/issues/9280 +create a style with a geojson source mapbox styles create human https://github.com/mapbox/mapbox-sdk-js/issues/210 +create a custom basemap style mapbox styles create human https://docs.mapbox.com/help/tutorials/create-a-custom-style +list styles with fresh results not cached mapbox styles list symptom https://github.com/mapbox/mapbox-sdk-js/issues/389 +transfer styles between accounts mapbox styles download|mapbox styles create human https://docs.mapbox.com/help/dive-deeper/transfer-styles-between-accounts +add a map style offline mapbox styles download human https://github.com/mapbox/mapbox-gl-js/issues/9861 +creating a style and rendering it mapbox styles create|mapbox static get-image human https://github.com/mapbox/mapbox-sdk-js/issues/322 +map does not show the latest styling changes mapbox styles get|mapbox styles draft get symptom https://docs.mapbox.com/help/troubleshooting/stale-data +sprite property in style adding custom images mapbox sprites upload human https://github.com/mapbox/mapbox-gl-js/issues/9854 +deleting icons dont work right away mapbox sprites delete symptom https://github.com/mapbox/mapbox-sdk-js/issues/388 +svg image errors when uploading an icon mapbox sprites upload symptom https://docs.mapbox.com/help/troubleshooting/studio-svg-upload-errors +custom sprite in a custom style does not work mapbox sprites upload|mapbox sprites get-json symptom https://github.com/mapbox/mapbox-gl-js/issues/13268 +add a custom font to a map style mapbox fonts upload human https://docs.mapbox.com/help/how-to-videos/add-custom-font-studio-video +manage your fontstacks mapbox fonts list human https://docs.mapbox.com/help/dive-deeper/manage-fontstacks +custom font appears in preview but not in published style mapbox fonts list|mapbox fonts upload symptom https://github.com/mapbox/mapbox-gl-js/issues/13577 +no geojson features processed make sure line delimited mapbox tilesets-cli symptom https://github.com/mapbox/tilesets-cli/issues/155 +how do i deal with a tileset bigger than 50 gb mapbox tilesets-cli human https://github.com/mapbox/tilesets-cli/issues/164 +cant re-publish a tileset after adding new sources mapbox tilesets-cli symptom https://github.com/mapbox/tilesets-cli/issues/162 +delete a tileset source mapbox tilesets-cli human https://github.com/mapbox/tilesets-cli/issues/171 +list only shows the last 100 tilesets mapbox tilesets-cli symptom https://github.com/mapbox/tilesets-cli/issues/135 +estimate area returns 0 km2 mapbox tilesets-cli symptom https://github.com/mapbox/tilesets-cli/issues/161 +upload large source files to mapbox tiling service mapbox tilesets-cli human https://docs.mapbox.com/help/troubleshooting/upload-large-source-files-mts +keep a tileset updated with incremental updates mapbox tilesets-cli human https://docs.mapbox.com/help/tutorials/mts-incremental-updates +cluster point data with the tiling service mapbox tilesets-cli human https://docs.mapbox.com/help/tutorials/cluster-point-data-with-mts +view the recipe of a tileset mapbox tilesets-cli human https://github.com/mapbox/tilesets-cli/issues/126 +status of an unpublished tileset mapbox tilesets-cli human https://github.com/mapbox/tilesets-cli/issues/127 +adjust the zoom extent of my tileset mapbox tilesets-cli human https://docs.mapbox.com/help/troubleshooting/adjust-tileset-zoom-extent +csv file errors on upload mapbox tilesets-cli symptom https://docs.mapbox.com/help/troubleshooting/csv-upload-errors +how to remove an incomplete upload mapbox tilesets-cli human https://github.com/mapbox/mapbox-cli-py/issues/90 +retreiving tilesets mapbox tilesets-cli|mapbox tilesets get-mvt symptom https://github.com/mapbox/mapbox-cli-py/issues/130 +upload data to mapbox mapbox tilesets-cli human https://docs.mapbox.com/help/troubleshooting/uploads +feature ids in tilequery response mapbox tilesets query human https://github.com/mapbox/mapbox-sdk-js/issues/265 +point in polygon query mapbox tilesets query human https://docs.mapbox.com/help/tutorials/point-in-polygon-query-with-mapbox-boundaries +get the city and street where my marker is mapbox geocoder reverse|mapbox search reverse human https://github.com/mapbox/mapbox-gl-js/issues/12302 +unable to change location name to coordinates mapbox geocoder forward|mapbox search forward human https://github.com/mapbox/mapbox-sdk-js/issues/424 +country code in reverse geocode response mapbox geocoder reverse|mapbox search reverse human https://github.com/mapbox/mapbox-sdk-js/issues/423 +geocode with proximity set to ip mapbox geocoder forward|mapbox search forward human https://github.com/mapbox/mapbox-sdk-js/issues/466 +geocoding bbox parameter mapbox geocoder forward|mapbox search forward human https://github.com/mapbox/mapbox-cli-py/issues/70 +perform batch geocoding mapbox geocoder batch human https://docs.mapbox.com/help/how-to-videos/alteryx-macro-search-for-batch-geocoding-video +v6 geocoding batch api mapbox geocoder batch human https://github.com/mapbox/mapbox-sdk-js/issues/485 +geocoding with structured input and secondary address mapbox geocoder forward|mapbox search forward human https://github.com/mapbox/mapbox-sdk-js/issues/489 +temporary versus permanent geocoding mapbox geocoder forward|mapbox geocoder batch human https://docs.mapbox.com/help/dive-deeper/understand-temporary-vs-permanent-geocoding +specific address shows incorrect location wrong zipcode mapbox feedback create|mapbox geocoder forward symptom https://github.com/mapbox/mapbox-gl-js/issues/10373 +address geocoding format guide mapbox geocoder forward|mapbox search forward human https://docs.mapbox.com/help/troubleshooting/address-geocoding-format-guide +add point of interest poi search to a map mapbox search category|mapbox search forward human https://docs.mapbox.com/help/tutorials/poi-search-react +retrieve api returning 404 mapbox places symptom https://github.com/mapbox/mapbox-gl-js/issues/13402 +filter by counties for us mapbox geocoder forward|mapbox search forward human https://github.com/mapbox/mapbox-gl-js/issues/9950 +forward geocoding doesnt return bbox property mapbox geocoder forward symptom https://github.com/mapbox/mapbox-gl-js/issues/10897 +403 forbidden for geocoding api request mapbox auth whoami|mapbox accounts list-scopes symptom https://github.com/mapbox/mapbox-gl-js/issues/8155 +city name needs to be changed mapbox feedback create human https://github.com/mapbox/mapbox-gl-js/issues/11593 +reporting data issues mapbox feedback create human https://docs.mapbox.com/help/dive-deeper/reporting-data-issues +report an error from a map search or route result mapbox feedback create human https://docs.mapbox.com/help/troubleshooting/reporting-a-problem +mapbox returns driving instead of walking directions mapbox directions symptom https://github.com/mapbox/mapbox-cli-py/issues/71 +add directions route lines to a web map mapbox directions human https://docs.mapbox.com/help/tutorials/getting-started-directions-api +calculation of distances with different values mapbox directions|mapbox matrix human https://github.com/mapbox/mapbox-gl-js/issues/10939 +generate an optimized route mapbox directions|mapbox matrix human https://docs.mapbox.com/help/tutorials/optimization-api +matrix returning null values mapbox matrix symptom https://github.com/mapbox/mapbox-sdk-js/issues/312 +add travel time isochrones to a web map mapbox isochrone human https://docs.mapbox.com/help/tutorials/get-started-isochrone-api +isochrone api polygon mapbox isochrone human https://traveltime.com/blog/mapbox-isochrone-api-polygon +map matching api always returns an http error mapbox map-match symptom https://github.com/mapbox/mapbox-sdk-js/issues/286 +need a matching full example mapbox map-match human https://github.com/mapbox/mapbox-sdk-js/issues/284 +mapmatching for multi feature inputs mapbox map-match human https://github.com/mapbox/mapbox-cli-py/issues/37 +get started with the map matching api mapbox map-match human https://docs.mapbox.com/help/tutorials/get-started-map-matching-api +what is token with styles:list scope mapbox accounts list-scopes human https://github.com/mapbox/mapbox-sdk-js/issues/281 +create a temporary token - none https://github.com/mapbox/mapbox-sdk-js/issues/291 +troubleshooting access tokens mapbox accounts list-tokens|mapbox accounts retrieve-token|mapbox auth whoami human https://docs.mapbox.com/help/troubleshooting/token-errors +receiving error 401 unauthorized mapbox auth whoami|mapbox accounts retrieve-token symptom https://github.com/mapbox/mapbox-gl-js/issues/13325 +get token details by token id mapbox accounts retrieve-token human https://github.com/mapbox/mapbox-sdk-js/issues/463 +access token missing command mapbox accounts list-tokens|mapbox accounts retrieve-token symptom https://github.com/mapbox/mapbox-cli-py/issues/82 +trouble setting access token mapbox auth login|mapbox auth whoami human https://github.com/mapbox/mapbox-cli-py/issues/69 +mapboxaccesstoken or mapbox_access_token mapbox auth whoami|mapbox doctor symptom https://github.com/mapbox/mapbox-cli-py/issues/53 +token without the right permissions gives unhelpful error mapbox accounts list-scopes|mapbox auth whoami symptom https://github.com/mapbox/tilesets-cli/issues/110 +add cli for analytics api mapbox usage human https://github.com/mapbox/mapbox-cli-py/issues/93 +manage your web map costs mapbox usage human https://docs.mapbox.com/help/troubleshooting/manage-web-map-costs +disable event tracking mapbox config set human https://github.com/mapbox/mapbox-gl-js/issues/13226 +give more guidance on config file location mapbox config list|mapbox doctor human https://github.com/mapbox/mapbox-cli-py/issues/65 +getaddrinfo enotfound api.mapbox.com mapbox doctor symptom https://github.com/mapbox/mapbox-sdk-js/issues/325 +working with firewalls mapbox doctor human https://docs.mapbox.com/help/dive-deeper/firewalls +ai assisted development with mapbox mapbox mcp install|mapbox agent-skills install human https://docs.mapbox.com/help/getting-started/ai-assisted-development +build your own agent with the mapbox mcp server mapbox mcp install|mapbox mcp list human https://docs.mapbox.com/help/tutorials/build-your-own-agent +how to regain access to your account - none https://docs.mapbox.com/help/troubleshooting/account-lockout +switch from google maps javascript api to mapbox gl js - none https://docs.mapbox.com/help/tutorials/google-to-mapbox +use mapbox gl js in a react app - none https://docs.mapbox.com/help/tutorials/use-mapbox-gl-js-with-react +add address autofill to a form on a website - none https://docs.mapbox.com/help/tutorials/add-address-autofill-to-your-website +add custom markers in mapbox gl js - none https://docs.mapbox.com/help/tutorials/custom-markers-gl-js +make a heatmap with mapbox gl js - none https://docs.mapbox.com/help/tutorials/make-a-heatmap-with-mapbox-gl-js +mapbox logo disappears on streets view - none https://github.com/mapbox/mapbox-gl-js/issues/13561 +improve the performance of mapbox gl js maps - none https://docs.mapbox.com/help/troubleshooting/mapbox-gl-js-performance +build a store locator - none https://docs.mapbox.com/help/tutorials/building-a-store-locator +how to draw arrow on line layer - none https://github.com/mapbox/mapbox-gl-js/issues/12891 diff --git a/tests/fixtures/commands_search/tuning.tsv b/tests/fixtures/commands_search/tuning.tsv new file mode 100644 index 0000000..e9a40db --- /dev/null +++ b/tests/fixtures/commands_search/tuning.tsv @@ -0,0 +1,104 @@ +# Written by hand while building the search, and tuned against: the scoring and +# the help text were changed until these ranked well, so this set flatters it. +# Columns: query, expected first result (`|` between acceptable answers, `-` +# for none), kind. +how do I log in to mapbox mapbox auth login human +sign in mapbox auth login synonym +log out of my account mapbox auth logout human +remove my saved credentials mapbox auth logout human +refresh my access token mapbox auth refresh human +who am I logged in as mapbox auth whoami human +which token will the next command use mapbox auth whoami agent +show stored credential profiles mapbox auth profiles human +list my access tokens mapbox accounts list-tokens human +what scopes can a token have mapbox accounts list-scopes human +token permissions mapbox accounts list-scopes synonym +get details of one token mapbox accounts retrieve-token human +create a new map style mapbox styles create human +list all styles in my account mapbox styles list human +get the json of a style mapbox styles get agent +update an existing style mapbox styles update human +delete a style mapbox styles delete human +download a style as a zip mapbox styles download human +back up my style offline mapbox styles download synonym +edit the draft of a style mapbox styles draft update human +get a style draft mapbox styles draft get agent +discard a style draft mapbox styles draft delete synonym +rename my style mapbox styles update synonym +find styles I deleted mapbox styles list human +add an icon to a sprite mapbox sprites upload human +upload many images to a sprite at once mapbox sprites upload-batch human +remove an image from the sprite mapbox sprites delete human +delete several sprite images mapbox sprites delete-batch human +get sprite json mapbox sprites get-json agent +what icons are in my sprite mapbox sprites get-json human +upload a custom font mapbox fonts upload human +list fonts mapbox fonts list agent +delete a font mapbox fonts delete agent +add a ttf font mapbox fonts upload human +which fonts do I have mapbox fonts list human +get a vector tile mapbox tilesets get-mvt agent +fetch a raster tile from a tileset mapbox tilesets get-tile agent +query features near a coordinate mapbox tilesets query agent +what features are at this point mapbox tilesets query human +upload a tileset mapbox tilesets-cli human +create a tileset from geojson mapbox tilesets-cli human +download mvt mapbox tilesets get-mvt agent +render a static map image mapbox static get-image agent +make a png of a map mapbox static get-image human +static raster tile mapbox static get-tile agent +map picture for an email mapbox static get-image synonym +thumbnail of my style mapbox static get-image synonym +search for an address mapbox search forward|mapbox geocoder forward human +find a poi by name mapbox search forward agent +what is at this coordinate mapbox search reverse|mapbox geocoder reverse|mapbox tilesets query human +find restaurants nearby mapbox search category human +list poi categories mapbox search list-category agent +category ids for search mapbox search list-category agent +get details for a place mapbox places human +look up a place by its id mapbox places agent +geocode an address mapbox geocoder forward|mapbox search forward agent +reverse geocode a coordinate mapbox geocoder reverse|mapbox search reverse agent +batch geocode many addresses mapbox geocoder batch agent +turn coordinates into an address mapbox geocoder reverse|mapbox search reverse human +convert an address to lat long mapbox geocoder forward|mapbox search forward human +directions from a to b mapbox directions human +driving route mapbox directions agent +walking directions between two points mapbox directions human +travel time between many locations mapbox matrix human +distance matrix mapbox matrix agent +area reachable in 15 minutes mapbox isochrone human +drive time polygon mapbox isochrone synonym +snap gps points to roads mapbox map-match human +match a gps trace mapbox map-match agent +clean up a noisy gps track mapbox map-match synonym +report a wrong address on the map mapbox feedback create human +list data feedback I submitted mapbox feedback list human +get a feedback item mapbox feedback get agent +send feedback about the cli mapbox report human +file a bug about this tool mapbox report synonym +install an mcp server mapbox mcp install agent +list mcp servers mapbox mcp list agent +set up mapbox for claude code mapbox mcp install|mapbox agent-skills install human +install agent skills mapbox agent-skills install agent +update installed skills mapbox agent-skills update agent +remove skills mapbox agent-skills uninstall human +list available skills mapbox agent-skills list agent +write a skill describing this cli mapbox generate-skills agent +shell completion for zsh mapbox completion human +enable tab completion mapbox completion human +uninstall the cli mapbox uninstall human +turn off telemetry mapbox config set human +show current settings mapbox config list human +reset a setting to its default mapbox config unset human +show recent commands I ran mapbox history list human +details of the last run mapbox history show human +check my proxy settings mapbox doctor human +how much have I used this month mapbox usage human +api usage by day mapbox usage agent +uplaod a font mapbox fonts upload typo +direcitons mapbox directions typo +isochorne mapbox isochrone typo +dleete a style mapbox styles delete typo +bake a cake - none +deploy to kubernetes - none diff --git a/tests/output_contract.rs b/tests/output_contract.rs index 22e66c2..ce1dec6 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -443,12 +443,13 @@ fn help_names_env_variables_without_their_values() { assert!(!text.contains("=]"), "an env note still shows `=`: {text}"); } -const AGENT_HINT: &str = "Agents: `mapbox --schema`"; +const AGENT_HINT: &str = "Agents: `mapbox commands search"; -/// An agent reading top-level help is pointed at `--schema` rather than left -/// to chain `--help` calls. Only there: subcommand help stays as it was. +/// An agent reading top-level help is pointed at `commands search` and +/// `--schema` rather than left to chain `--help` calls. Only there: +/// subcommand help stays as it was. #[test] -fn an_agent_is_pointed_at_the_schema() { +fn an_agent_is_pointed_at_search_and_the_schema() { let out = command() .env("CLAUDECODE", "1") .arg("--help") @@ -456,6 +457,11 @@ fn an_agent_is_pointed_at_the_schema() { .expect("run mapbox"); assert!(out.status.success()); assert!(stdout(&out).contains(AGENT_HINT), "{}", stdout(&out)); + assert!( + stdout(&out).contains("`mapbox --schema`"), + "{}", + stdout(&out) + ); let sub = command() .env("CLAUDECODE", "1") @@ -1294,3 +1300,96 @@ fn text_into_a_pipe_carries_no_color_and_no_banner() { } } } + +/// `commands search` answers on stdout alone under `json`: the matches as +/// `[{command, summary}]`, best first, with nothing on stderr for a caller +/// to have to tell apart from a warning. +#[test] +fn commands_search_prints_its_matches_as_json() { + let out = run(&["-o", "json", "commands", "search", "upload", "a", "font"]); + assert!(out.status.success(), "{}", stderr(&out)); + assert_eq!(stderr(&out), ""); + let matches = json(&stdout(&out)); + let matches = matches.as_array().expect("an array"); + assert_eq!(matches[0]["command"], "mapbox fonts upload"); + assert!(matches.len() <= 5, "{matches:?}"); + for entry in matches { + let keys: Vec<&String> = entry.as_object().expect("an object").keys().collect(); + assert_eq!(keys, ["command", "summary"], "{entry}"); + } +} + +/// A person gets a table on stdout and the next step on stderr, so +/// `> file` keeps the table alone. +#[test] +fn commands_search_prints_a_table_and_a_tip_as_text() { + let out = run(&["-o", "text", "commands", "search", "upload a font"]); + assert!(out.status.success(), "{}", stderr(&out)); + let text = stdout(&out); + assert!(text.starts_with("COMMAND"), "{text}"); + assert!(text.contains("mapbox fonts upload"), "{text}"); + assert!(stderr(&out).contains("Tip:"), "{}", stderr(&out)); +} + +/// No match is an empty answer, not a failure: the query was fine, the CLI +/// just has nothing for it. +#[test] +fn commands_search_with_no_match_is_an_empty_list() { + let out = run(&["-o", "json", "commands", "search", "xyzzy"]); + assert!(out.status.success(), "{}", stderr(&out)); + assert_eq!(json(&stdout(&out)), serde_json::json!([])); +} + +#[test] +fn commands_list_names_every_command_once() { + let out = run(&["-o", "json", "commands", "list"]); + assert!(out.status.success(), "{}", stderr(&out)); + let listed = json(&stdout(&out)); + let listed: Vec<&str> = listed + .as_array() + .expect("an array") + .iter() + .map(|entry| entry["command"].as_str().expect("a command")) + .collect(); + let schema = json(&stdout(&run(&["--schema"]))); + let mut described: Vec<&str> = schema["commands"] + .as_array() + .expect("commands") + .iter() + .map(|entry| entry["command"].as_str().expect("a command")) + .collect(); + described.sort_unstable(); + assert_eq!(listed, described); +} + +/// A command name nothing resembles is pointed at `commands search`, in +/// both modes. One clap has a near spelling for keeps clap's suggestion, +/// which is the better answer. +#[test] +fn an_unknown_command_is_pointed_at_search() { + let out = run(&["-o", "json", "upload-font"]); + assert_eq!(out.status.code(), Some(2)); + let error = json(&stderr(&out)); + assert!( + error["fix"] + .as_str() + .is_some_and(|fix| fix.contains("mapbox commands search")), + "{error}" + ); + + let out = run(&["-o", "text", "upload-font"]); + assert!( + stderr(&out).contains("mapbox commands search"), + "{}", + stderr(&out) + ); + + let out = run(&["-o", "json", "styles", "lst"]); + let error = json(&stderr(&out)); + assert!( + error["fix"] + .as_str() + .is_some_and(|fix| fix.contains("'list'")), + "{error}" + ); +} diff --git a/tests/telemetry_events.rs b/tests/telemetry_events.rs index 8ce38a9..4cdd528 100644 --- a/tests/telemetry_events.rs +++ b/tests/telemetry_events.rs @@ -1276,3 +1276,35 @@ fn an_unexplained_refusal_starts_a_cooldown_too() { assert_eq!(state["rejected"], false); assert!(state["cooldowns"]["telemetry"]["until"].is_u64(), "{state}"); } + +/// A search query is whatever the caller typed — a customer's name, a +/// token pasted by mistake — so it leaves as its length and never as text. +#[test] +fn a_search_query_is_sent_as_its_length() { + let home = scratch("commands-search"); + let (out, event) = run_sent( + &home, + &[ + "-o", + "json", + "commands", + "search", + "secret-customer-name", + "upload", + ], + ); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(event["command"], serde_json::json!(["commands", "search"])); + assert!( + !event.to_string().contains("secret-customer-name"), + "the query's text was sent: {event}" + ); + assert_eq!( + event["params"], + serde_json::json!([{ "name": "query", "length": "secret-customer-name upload".len() }]) + ); +} From 6ef9197cd0f05bcf49ee367d1caafba1e0af89d3 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Tue, 20 Oct 2026 14:03:32 +0300 Subject: [PATCH 02/10] Fix self-review findings for commands search - Text-mode no match writes nothing to stdout. - A real word is no longer typo-matched into another, so "patch a style" stops ranking batch commands first. - The CI summary says why there is nothing to compare: no base, a base from before the eval, or a failed run. The report script never raises, and has its own shell test. - Help rows fit 80 columns again. - Tests for --limit, the human help line, the commands group being left out, and a synonym test that failed for the wrong reason. --- .github/workflows/ci.yml | 63 ++- AGENTS.md | 5 +- docs/commands.md | 3 +- scripts/commands-search-report.py | 257 ++++++++++--- scripts/test-commands-search-report.sh | 163 ++++++++ src/commands.rs | 464 +++++++++++++++-------- src/config.rs | 2 +- src/doctor.rs | 2 +- src/help_layout.rs | 2 +- src/main.rs | 6 +- src/tilesets_cli.rs | 2 +- tests/fixtures/commands_search/README.md | 23 +- tests/output_contract.rs | 88 +++++ 13 files changed, 847 insertions(+), 233 deletions(-) create mode 100755 scripts/test-commands-search-report.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 98c4d4f..ec5df74 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,9 +126,11 @@ jobs: contents: read steps: # Whole history, so the base commit is there to check out beside it. + # No credential left behind: nothing here pushes or fetches again. - uses: actions/checkout@v7 with: fetch-depth: 0 + persist-credentials: false # As in the build job: install exactly the pinned toolchain. - name: Read the pinned toolchain @@ -141,27 +143,53 @@ jobs: with: toolchain: ${{ steps.toolchain.outputs.toolchain }} - # `|| true`: a run below its floor still writes its reports, and the - # build job is where that failure is reported. + # The exit status is recorded rather than failing the step: a run below + # its floor still writes its reports, the build job is where that + # failure is reported, and the summary needs the status to say whether + # missing reports mean a failed run or one with no eval to run. - name: Run the eval on this branch - run: MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-head" cargo test --locked --bin mapbox eval_set || true + id: head + run: | + status=0 + MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-head" cargo test --locked --bin mapbox eval_set || status=$? + echo "status=$status" >>"$GITHUB_OUTPUT" # In a worktree sharing this target directory, so only the crate itself - # rebuilds. A base from before the eval existed matches no test and - # writes no reports; the summary then shows this branch's scores alone. + # rebuilds. A base from before the eval existed matches no test, passes + # and writes no reports; one that fails to build or check out exits + # non-zero. The summary tells the two apart by the status. - name: Run the eval on the base + id: base if: github.event_name == 'pull_request' env: BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | - git worktree add --detach "$RUNNER_TEMP/base" "$BASE_SHA" - cd "$RUNNER_TEMP/base" - CARGO_TARGET_DIR="$GITHUB_WORKSPACE/target" \ - MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-base" \ - cargo test --locked --bin mapbox eval_set || true - + status=0 + { + git worktree add --detach "$RUNNER_TEMP/base" "$BASE_SHA" && + cd "$RUNNER_TEMP/base" && + CARGO_TARGET_DIR="$GITHUB_WORKSPACE/target" \ + MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-base" \ + cargo test --locked --bin mapbox eval_set + } || status=$? + echo "status=$status" >>"$GITHUB_OUTPUT" + + # No `--base` on a push: the script then says there was no base rather + # than that the base had no reports. A status left empty by a step that + # never finished reads as "did not finish". - name: Write the summary - run: python3 scripts/commands-search-report.py "$RUNNER_TEMP/eval-head" "$RUNNER_TEMP/eval-base" >>"$GITHUB_STEP_SUMMARY" + if: ${{ !cancelled() }} + env: + EVENT: ${{ github.event_name }} + HEAD_STATUS: ${{ steps.head.outputs.status }} + BASE_STATUS: ${{ steps.base.outputs.status }} + run: | + set -- --head "$RUNNER_TEMP/eval-head" --head-status "$HEAD_STATUS" \ + --sets tests/fixtures/commands_search + if [ "$EVENT" = pull_request ]; then + set -- "$@" --base "$RUNNER_TEMP/eval-base" --base-status "$BASE_STATUS" + fi + python3 scripts/commands-search-report.py "$@" >>"$GITHUB_STEP_SUMMARY" # No Rust: this is where the repo's shell gets checked, install.sh above all # — it is served to machines we do not control, and this is the only thing @@ -186,6 +214,7 @@ jobs: sh -n scripts/install.sh sh -n scripts/test-install.sh sh -n scripts/test-completion.sh + sh -n scripts/test-commands-search-report.sh # -s sh, not the default: bash would accept things `curl | sh` will not. # Once is enough — the findings do not depend on the runner. The version @@ -195,11 +224,19 @@ jobs: # Kept clean against 0.9, 0.10 and 0.11. - name: Shellcheck if: runner.os == 'Linux' - run: shellcheck -s sh scripts/install.sh scripts/test-install.sh scripts/test-completion.sh + run: shellcheck -s sh scripts/install.sh scripts/test-install.sh scripts/test-completion.sh scripts/test-commands-search-report.sh - name: Install, verify and prompt against a local channel run: sh scripts/test-install.sh + # The `commands search eval` job's summary script, against hand-written + # reports: that job never fails, so this is where a summary that says + # the wrong thing would be caught. Python only, so it lives here rather + # than beside the Rust. Once is enough, as for shellcheck. + - name: Summarize hand-written search eval reports + if: runner.os == 'Linux' + run: sh scripts/test-commands-search-report.sh + # The other half of the same job, for the installer install.sh stops short # of. Both shells, because both are on a stock Windows 11 and they differ # exactly where this script lives: the default TLS versions, what `2>` does diff --git a/AGENTS.md b/AGENTS.md index d27c65d..3723215 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -129,8 +129,9 @@ an entry outlives its command. A command it misses still shows, under ## `commands search` and its query sets `mapbox commands search` ranks commands by the words in their names, summaries, -descriptions and help-group lines, so **editing a command's `about` or a line -in `DESCRIPTIONS` changes search results**, not only `--help`. The `eval_set_*` +descriptions, help-group lines and argument help, so **editing a command's +`about`, an argument's `help` or a line in `DESCRIPTIONS` changes search +results**, not only `--help`. The `eval_set_*` tests in `src/commands.rs` run the query sets in `tests/fixtures/commands_search/` and hold each to a floor of how many queries rank right. Each file's header says where its queries came from and how much the search was tuned against them. diff --git a/docs/commands.md b/docs/commands.md index 6fd7693..c063d5e 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4753,7 +4753,8 @@ model: each word of the query is matched against a command's name, summary and description, its group's description and its arguments' help, with a command's name counting most. A word can be a prefix (`geo`), a different ending of the same stem (`geocoding` for `geocoder`) or one typo away -(`uplaod`); a plural matches its singular. A few common words stand in for +(`uplaod`), unless some command's text uses the word as it is: "match" is a +word, not a typo for "batch". A plural matches its singular. A few common words stand in for the ones commands use ("remove" for "delete", "rename" for "update"). Words like "how", "my", "to" and "mapbox" are ignored. It cannot find a command whose text uses none of the query's words, so a query names the diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py index d233ec5..0e78f4f 100644 --- a/scripts/commands-search-report.py +++ b/scripts/commands-search-report.py @@ -2,45 +2,95 @@ """Markdown summary of `mapbox commands search` eval results, for a CI job summary. - commands-search-report.py [] + commands-search-report.py --head DIR --head-status N + [--base DIR --base-status N] [--sets DIR] Each directory holds the per-set reports the `eval_set_*` tests write when -MAPBOX_SEARCH_EVAL_REPORT names it. With a base, the summary says which -queries the change fixed and broke; without one (a base from before the -eval existed), only the scores. +MAPBOX_SEARCH_EVAL_REPORT names it; each status is the exit code of the +`cargo test` run that wrote them. No `--base` means there was no base to run +(not a pull request). `--sets` is the fixture directory, so a set whose +report is missing is named rather than left out. -Reports, never gates: the floors in `cargo test` are the gate. Standard -library only, so the job needs no install step. +The status is what tells apart a base that predates the eval (the run passed +and wrote nothing) from one whose run failed: the reports alone look the +same for both. + +Reports, never gates: the floors in `cargo test` are the gate. So this never +fails the job on bad input: a malformed line is skipped and counted, and an +unreadable report is named. Standard library only, so the job needs no +install step. """ +import argparse import sys from pathlib import Path +class Set: + def __init__(self): + self.floors = None + self.rows = {} + self.invalid = [] + self.skipped = 0 + self.unreadable = None + + def load(directory): + """Every report in `directory`, by set name. Empty when there is none.""" sets = {} if directory is None or not directory.is_dir(): return sets for path in sorted(directory.iterdir()): - floors, rows = None, {} - for line in path.read_text().splitlines(): - columns = line.split("\t") - if columns[0] == "# floors": - floors = (int(columns[1]), int(columns[2])) - continue - query, expected, found, first, top3 = columns - rows[query] = { - "expected": expected.replace("|", " or ") or "(nothing)", - "found": found.replace("|", ", ") or "(nothing)", - "first": first == "1", - "top3": top3 == "1", - } - sets[path.name] = (floors, rows) + report = sets[path.name] = Set() + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as error: + report.unreadable = str(error) + continue + for line in text.splitlines(): + parse_line(report, line) return sets -def score(rows, key): - return sum(row[key] for row in rows.values()) +def parse_line(report, line): + columns = line.split("\t") + if columns[0] == "# floors": + try: + report.floors = (int(columns[1]), int(columns[2])) + except (IndexError, ValueError): + report.skipped += 1 + return + if columns[0] == "# invalid": + report.invalid.append(" ".join(columns[1:])) + return + if columns[0].startswith("#") or not line.strip(): + return + if len(columns) != 5 or columns[3] not in ("0", "1") or columns[4] not in ("0", "1"): + report.skipped += 1 + return + query, expected, found, first, top3 = columns + report.rows[query] = { + "expected": " or ".join(expected.split("|")) if expected else "(nothing)", + "found": ", ".join(found.split("|")) if found else "(nothing)", + "first": first == "1", + "top3": top3 == "1", + } + + +def cell(text): + """`text` made safe for one Markdown table cell.""" + return ( + str(text) + .replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace("|", "\\|") + .replace("\n", " ") + ) + + +def score(report, key): + return sum(row[key] for row in report.rows.values()) def delta(head, base): @@ -58,56 +108,130 @@ def changed(head_rows, base_rows): if before is None or before["first"] == row["first"]: continue line = ( - f"| {query} | {row['expected']} | {before['found']} | {row['found']} |" + f"| {cell(query)} | {cell(row['expected'])} | " + f"{cell(before['found'])} | {cell(row['found'])} |" ) (fixed if row["first"] else broken).append(line) return fixed, broken -def main(): - head = load(Path(sys.argv[1])) - base = load(Path(sys.argv[2])) if len(sys.argv) > 2 else {} +def status_note(status): + return "did not finish" if status is None else f"failed (exit {status})" + + +def parse_status(text): + try: + return int(text) + except (TypeError, ValueError): + return None + + +def base_note(base_given, base, base_status): + """Why there is nothing to compare against, or what to know about it.""" + if not base_given: + return "Not a pull request, so there is no base to compare against." + if not base: + if base_status == 0: + return ( + "The base predates the eval (its run had no eval tests), " + "so only this branch's scores are shown." + ) + return ( + f"The base run {status_note(base_status)} before writing " + "any report, so only this branch's scores are shown." + ) + if base_status != 0: + return ( + f"The base run {status_note(base_status)}; " + "its scores are as far as it got." + ) + return None + + +def notes(report, floors_missed): + out = [] + if report.unreadable: + out.append(f"unreadable: {report.unreadable}") + if floors_missed: + out.append("**below its floor**") + if report.invalid: + out.append(f"{len(report.invalid)} invalid rows") + if report.skipped: + out.append(f"{report.skipped} malformed lines skipped") + return out + + +def render(head, head_status, base, base_given, base_status, expected_sets): out = ["## `commands search` eval", ""] if not head: - out.append("No reports were written; the eval tests did not run.") - print("\n".join(out)) - return - if not base: - out += ["The base branch has no eval reports, so only this branch's scores are shown.", ""] + if head_status == 0: + out.append("No reports were written, though the eval run passed: no `eval_set_*` test ran.") + else: + out.append( + f"The eval run on this branch {status_note(head_status)} " + "before writing any report. The build job's test step has the details." + ) + return out + if head_status != 0: + out += [ + f"The eval run on this branch {status_note(head_status)}: a set is " + "below its floor, a row is invalid, or the tests did not build. The build " + "job's test step has the details.", + "", + ] + note = base_note(base_given, base, base_status) + if note: + out += [note, ""] + compare = bool(base) out += [ - "| Set | Rows | First | Top 3 | Floor |", - "| --- | --- | --- | --- | --- |", + "| Set | Rows | First | Top 3 | Floor | Note |", + "| --- | --- | --- | --- | --- | --- |", ] details = [] - for name, (floors, rows) in head.items(): - base_rows = base.get(name, (None, None))[1] - first, top3 = score(rows, "first"), score(rows, "top3") - base_first = score(base_rows, "first") if base_rows else None - base_top3 = score(base_rows, "top3") if base_rows else None + names = sorted(set(head) | set(base) | set(expected_sets)) + for name in names: + label = f"`{cell(name)}`" + if name not in head: + where = "only in the base" if name in base else "missing from this run" + out.append(f"| {label} | | | | | {where} |") + continue + report = head[name] + base_report = base.get(name) + first, top3 = score(report, "first"), score(report, "top3") + base_first = score(base_report, "first") if base_report else None + base_top3 = score(base_report, "top3") if base_report else None + floors = report.floors floor = f"{floors[0]} / {floors[1]}" if floors else "-" - below = floors and (first < floors[0] or top3 < floors[1]) + missed = bool(floors) and (first < floors[0] or top3 < floors[1]) + row_notes = notes(report, missed) + if compare and base_report is None: + row_notes.insert(0, "only in this branch") out.append( - f"| `{name}` | {len(rows)} | {delta(first, base_first)} | " - f"{delta(top3, base_top3)} | {floor}{' ⚠️ below' if below else ''} |" + f"| {label} | {len(report.rows)} | {delta(first, base_first)} | " + f"{delta(top3, base_top3)} | {floor} | {cell('; '.join(row_notes))} |" ) - if base_rows: - fixed, broken = changed(rows, base_rows) + if report.invalid: + details += [f"### {label}: rows that could not be scored", ""] + details += [f"- {cell(problem)}" for problem in report.invalid] + [""] + + if base_report: + fixed, broken = changed(report.rows, base_report.rows) header = ["| Query | Want | Base | This branch |", "| --- | --- | --- | --- |"] if broken: - details += [f"### `{name}`: {len(broken)} no longer first", ""] + header + broken + [""] + details += [f"### {label}: {len(broken)} no longer first", ""] + header + broken + [""] if fixed: - details += [f"### `{name}`: {len(fixed)} now first", ""] + header + fixed + [""] + details += [f"### {label}: {len(fixed)} now first", ""] + header + fixed + [""] misses = [ - f"| {query} | {row['expected']} | {row['found']} |" - for query, row in rows.items() + f"| {cell(query)} | {cell(row['expected'])} | {cell(row['found'])} |" + for query, row in report.rows.items() if not row["first"] ] if misses: details += [ - f"
{name}: {len(misses)} not first", + f"
{cell(name)}: {len(misses)} not first", "", "| Query | Want | Got (top 3) |", "| --- | --- | --- |", @@ -117,9 +241,40 @@ def main(): "", ] - out += ["", *details] - print("\n".join(out)) + return out + ["", *details] + + +def main(): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--head", type=Path, required=True) + parser.add_argument("--head-status", default="") + parser.add_argument("--base", type=Path) + parser.add_argument("--base-status", default="") + parser.add_argument("--sets", type=Path) + args = parser.parse_args() + + expected_sets = [] + if args.sets is not None and args.sets.is_dir(): + expected_sets = [path.name for path in args.sets.glob("*.tsv")] + lines = render( + load(args.head), + parse_status(args.head_status), + load(args.base), + args.base is not None, + parse_status(args.base_status), + expected_sets, + ) + print("\n".join(lines)) if __name__ == "__main__": - main() + try: + main() + except SystemExit: + raise + except Exception as error: # noqa: BLE001 - a summary, never a gate + # The job is promised not to fail on this script; say what broke in + # the summary itself rather than leaving it empty. + print("## `commands search` eval\n") + print(f"The summary script failed: `{cell(repr(error))}`. This is a bug in the script.") + sys.exit(0) diff --git a/scripts/test-commands-search-report.sh b/scripts/test-commands-search-report.sh new file mode 100755 index 0000000..5fbdf6f --- /dev/null +++ b/scripts/test-commands-search-report.sh @@ -0,0 +1,163 @@ +#!/bin/sh +# Checks that `scripts/commands-search-report.py` says what happened for each +# way the CI `commands search eval` job can go, from hand-written reports. +# +# The job promises never to fail, and its summary is the only place a +# reviewer sees the result, so a summary that is wrong is worse than none: +# the cases here are the ones it once got wrong or could not tell apart. A +# base whose run failed, a push with no base and a base from before the eval +# all used to read "the base branch has no eval reports"; a malformed line +# raised a traceback and turned the job red. +# +# No Rust and no network: the reports are written here in the format the +# `eval_set_*` tests write. +# +# Usage: +# sh scripts/test-commands-search-report.sh + +set -eu + +SCRIPT="$(dirname "$0")/commands-search-report.py" +PYTHON="${PYTHON:-python3}" + +failures=0 + +say() { + printf '%s\n' "$*" +} + +fail() { + printf 'FAIL: %s\n' "$*" >&2 + failures=$((failures + 1)) +} + +WORK="$(mktemp -d)" +cleanup() { + rm -rf "$WORK" +} +trap cleanup EXIT INT TERM + +# One report: a floors line, then each argument as a row. +report() { + file="$1" + shift + mkdir -p "$(dirname "$file")" + printf '# floors\t1\t1\n' >"$file" + for row in "$@"; do + printf '%s\n' "$row" >>"$file" + done +} + +TAB="$(printf '\t')" +row() { + printf '%s' "$1${TAB}$2${TAB}$3${TAB}$4${TAB}$5" +} + +# Runs the script, which must exit 0, into `$WORK/out`. +summarize() { + if ! "$PYTHON" "$SCRIPT" "$@" >"$WORK/out" 2>"$WORK/err"; then + fail "exited non-zero for: $*: $(cat "$WORK/err")" + return 1 + fi + if [ -s "$WORK/err" ]; then + fail "wrote to stderr for: $*: $(cat "$WORK/err")" + fi +} + +expect() { + case_name="$1" + text="$2" + if grep -qF -- "$text" "$WORK/out"; then + say "ok: $case_name" + else + fail "$case_name: no \"$text\" in:" + sed 's/^/ /' "$WORK/out" >&2 + fi +} + +reject() { + case_name="$1" + text="$2" + if grep -qF -- "$text" "$WORK/out"; then + fail "$case_name: unexpected \"$text\" in:" + sed 's/^/ /' "$WORK/out" >&2 + else + say "ok: $case_name" + fi +} + +SETS="$WORK/sets" +mkdir -p "$SETS" +: >"$SETS/a.tsv" +: >"$SETS/b.tsv" + +HEAD="$WORK/head" +report "$HEAD/a.tsv" \ + "$(row 'upload a font' 'mapbox fonts upload' 'mapbox fonts upload' 1 1)" \ + "$(row 'a | b query' 'mapbox x|mapbox y' 'mapbox z' 0 0)" +report "$HEAD/b.tsv" \ + "$(row 'list styles' 'mapbox styles list' 'mapbox styles list' 1 1)" + +BASE="$WORK/base" +report "$BASE/a.tsv" \ + "$(row 'upload a font' 'mapbox fonts upload' 'mapbox fonts list' 0 1)" \ + "$(row 'a | b query' 'mapbox x|mapbox y' 'mapbox z' 0 0)" + +if summarize --head "$HEAD" --head-status 0 --base "$BASE" --base-status 0 --sets "$SETS"; then + expect "a fix against the base is listed" "1 now first" + expect "a set new in this branch is marked" "only in this branch" + expect "a pipe in a cell is escaped" 'a \| b query' + reject "no emoji" "⚠" +fi + +if summarize --head "$HEAD" --head-status 0 --sets "$SETS"; then + expect "a push has no base" "Not a pull request" +fi + +if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 0 --sets "$SETS"; then + expect "a base from before the eval" "The base predates the eval" +fi + +if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 101 --sets "$SETS"; then + expect "a base whose run failed" "The base run failed (exit 101)" +fi + +if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status '' --sets "$SETS"; then + expect "a base whose step never recorded a status" "The base run did not finish" +fi + +ONLY_A="$WORK/only-a" +report "$ONLY_A/a.tsv" "$(row 'q' 'mapbox x' 'mapbox x' 1 1)" +if summarize --head "$ONLY_A" --head-status 101 --base "$BASE" --base-status 0 --sets "$SETS"; then + expect "a set missing from this run is named" "missing from this run" + expect "a failing head run says so" "failed (exit 101)" +fi + +if summarize --head "$ONLY_A" --head-status 0 --base "$HEAD" --base-status 0; then + expect "a set only in the base is marked" "only in the base" +fi + +if summarize --head "$WORK/none" --head-status 101; then + expect "no reports and a failed run" "before writing any report" +fi + +BAD="$WORK/bad" +report "$BAD/a.tsv" \ + "$(row 'fine' 'mapbox x' 'mapbox x' 1 1)" \ + "too${TAB}few${TAB}columns" \ + "$(row 'odd flags' 'mapbox x' 'mapbox x' yes 1)" +printf '# floors\tnot-a-number\n' >>"$BAD/a.tsv" +printf '# invalid\tmapbox gone is not a command search answers with ("q")\n' >>"$BAD/a.tsv" +printf '\377\376 not utf-8\n' >"$BAD/b.tsv" +if summarize --head "$BAD" --head-status 101 --sets "$SETS"; then + expect "malformed lines are skipped and counted" "3 malformed lines skipped" + expect "an invalid row is listed" "mapbox gone" + expect "an unreadable report is named" "unreadable" +fi + +if [ "$failures" -ne 0 ]; then + say "$failures check(s) failed." + exit 1 +fi + +say "All checks passed." diff --git a/src/commands.rs b/src/commands.rs index e3ee362..06b28f3 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -44,9 +44,10 @@ const DESCRIPTION_WEIGHT: f64 = 2.0; const GROUP_WEIGHT: f64 = 1.5; const ARGUMENTS_WEIGHT: f64 = 0.5; /// How much a term has to score in one command before it counts as -/// answered in full: an exact match anywhere but an argument's help. A word -/// found only there, or a near miss, answers it in part, so a passing -/// mention cannot lift a command to full coverage of the query. +/// answered in full: an exact match anywhere but an argument's help, or a +/// near miss in the name or summary. A word found only in an argument's +/// help, or near-missed in a description or group line, answers it in part, +/// so a passing mention cannot lift a command to full coverage of the query. const FULL_MATCH: f64 = GROUP_WEIGHT; /// Taken off for each word of a command's name the query did not ask for, /// so "delete a style" finds `styles delete` ahead of `styles draft @@ -57,13 +58,25 @@ const UNASKED_NAME_WORD: f64 = 1.0; /// for many. const UNASKED_BATCH: f64 = 0.5; +/// What a deprecated command's score is scaled by. Still found, so a script +/// that names one can be told what it was, but below anything it would be +/// better to start using. +const DEPRECATED: f64 = 0.5; + +/// How well a query term matches a word, by kind of match (see +/// [`similarity`]). +const EXACT: f64 = 1.0; +const PREFIX: f64 = 0.7; +const STEM: f64 = 0.5; +const TYPO: f64 = 0.4; + /// A match this far below the best one is noise beside it. /// /// Relative only. A fixed minimum also emptied the answer for tasks this CLI /// cannot do, but it emptied it just as readily for ones it can, worded -/// loosely: "how far can I walk in ten minutes" finds `isochrone` with one -/// word. A wrong guess costs the caller a look; no answer when there was one -/// costs them the command. +/// loosely: "how far can I cycle in twenty minutes" finds `isochrone` with +/// one word. A wrong guess costs the caller a look; no answer when there was +/// one costs them the command. const MIN_SHARE_OF_BEST: f64 = 0.1; /// Words that describe the asking rather than the task. Left in, "how do I @@ -160,13 +173,13 @@ pub fn search( "No command matches \"{query}\". `mapbox commands list` shows every command." )); } - emit(mode, &found, !found.is_empty()) + emit(mode, &found) } pub fn list(app: &Command, specs: &[ServiceSpec], mode: Mode) -> Result<()> { let mut all = schema::build(app, specs, &[]).commands; all.sort_by(|a, b| a.command.cmp(&b.command)); - emit(mode, &all, true) + emit(mode, &all) } /// The commands that match `query`, best first, down to @@ -199,7 +212,12 @@ fn index(app: &Command, specs: &[ServiceSpec]) -> Vec<(Fields, CommandEntry)> { /// Ties go to the shorter command name, then alphabetically, so the same /// query always answers the same way. fn rank_in(index: &[(Fields, CommandEntry)], query: &str) -> Vec { - let query = Query::parse(query); + let vocabulary: BTreeSet<&str> = index + .iter() + .flat_map(|(fields, _)| fields.parts.iter().flat_map(|(_, words)| words)) + .map(String::as_str) + .collect(); + let query = Query::parse(query, &vocabulary); if query.terms.is_empty() { return vec![]; } @@ -218,8 +236,8 @@ fn rank_in(index: &[(Fields, CommandEntry)], query: &str) -> Vec { above_floor(scored).into_iter().map(|(_, i)| i).collect() } -/// `scored` sorted best first, without the zero scores and the ones too far -/// below the best. +/// What is left of `scored`, already sorted best first, once the zero scores +/// and the ones too far below the best are dropped. fn above_floor(scored: Vec<(f64, T)>) -> Vec<(f64, T)> { let floor = scored .first() @@ -231,7 +249,7 @@ fn above_floor(scored: Vec<(f64, T)>) -> Vec<(f64, T)> { } struct Query { - terms: Vec, + terms: Vec, /// Neighboring words written together, "log out" as "logout", tried /// only for an exact match, with the two words they were made from. /// Kept apart from `terms` so that one that matches nothing — almost all @@ -239,16 +257,33 @@ struct Query { compounds: Vec<(String, [String; 2])>, } +struct Term { + word: String, + /// Whether a word a typo away counts. Not for a word some command's text + /// uses as it is: that is a real word, not a misspelling, and matching + /// it one edit off turns "match" into "batch". + typos: bool, +} + impl Query { - fn parse(query: &str) -> Self { + fn parse(query: &str, vocabulary: &BTreeSet<&str>) -> Self { let all: Vec = tokens(query).collect(); + let mut seen = BTreeSet::new(); // Before stopwords come out: the "in" of "log in" is one. let compounds = all .windows(2) .map(|pair| (pair.concat(), [pair[0].clone(), pair[1].clone()])) + // "log in log in" asks once. + .filter(|(compound, _)| seen.insert(compound.clone())) .collect(); Query { - terms: terms(query), + terms: terms(query) + .into_iter() + .map(|word| Term { + typos: !vocabulary.contains(word.as_str()), + word, + }) + .collect(), compounds, } } @@ -284,10 +319,11 @@ impl Fields { .flatten() .collect::>() .join(" "); + let name = words(name); Fields { - name: words(name), + name: name.clone(), parts: vec![ - (NAME_WEIGHT, words(name)), + (NAME_WEIGHT, name), (SUMMARY_WEIGHT, words(&entry.summary)), ( DESCRIPTION_WEIGHT, @@ -332,14 +368,31 @@ fn score(query: &Query, fields: &Fields, deprecated: bool) -> f64 { .fold(0.0, f64::max); term_total += weight * best; } - matched += if answered.contains(term) { + matched += if answered.contains(&term.word) { 1.0 } else { (term_total / FULL_MATCH).min(1.0) }; total += term_total; } - let unasked = fields + let mut total = (total - UNASKED_NAME_WORD * unasked_name_words(query, fields) as f64).max(0.0); + // A `batch` command does what a sibling does, for many inputs at once, + // and its text says so at length: "forward or reverse", every option the + // sibling has. Unless the query asks for many, the sibling is the answer. + if fields.name.contains("batch") && !asks_for_many(query) { + total *= UNASKED_BATCH; + } + if deprecated { + total *= DEPRECATED; + } + let coverage = matched / query.terms.len() as f64; + total * coverage * coverage +} + +/// How many words of a command's name no term or compound of the query +/// asked for. +fn unasked_name_words(query: &Query, fields: &Fields) -> usize { + fields .name .iter() .filter(|word| { @@ -352,34 +405,26 @@ fn score(query: &Query, fields: &Fields, deprecated: bool) -> f64 { .iter() .any(|(compound, _)| exact_or_synonym(compound, word) > 0.0) }) - .count(); - // A `batch` command does what a sibling does, for many inputs at once, - // and its text says so at length: "forward or reverse", every option the - // sibling has. Unless the query asks for many, the sibling is the answer. - let unasked_batch = fields.name.contains("batch") - && !query - .terms - .iter() - .any(|term| relatedness(term, "batch") > 0.0 || relatedness(term, "multiple") > 0.0); - let total = (total - UNASKED_NAME_WORD * unasked as f64).max(0.0); - let total = if unasked_batch { - total * UNASKED_BATCH - } else { - total - }; - let coverage = matched / query.terms.len() as f64; - // Still found, so a script that names one can be told what it was, but - // below anything it would be better to start using. - let standing = if deprecated { 0.5 } else { 1.0 }; - total * coverage * coverage * standing + .count() +} + +/// Whether the query says it has many inputs. A typo does not: "patch" and +/// "watch" are one letter from "batch", and a query that means one of them +/// is not asking for a `batch` command. +fn asks_for_many(query: &Query) -> bool { + query.terms.iter().any(|term| { + ["batch", "multiple"].iter().any(|many| { + similarity(&term.word, many, false) > 0.0 || synonyms(&term.word).contains(many) + }) + }) } /// [`similarity`], or a synonym's exact match if that is closer. A synonym /// is not fuzzy-matched: it is already a guess, and "batch" one letter off /// is "match". -fn relatedness(term: &str, word: &str) -> f64 { - let direct = similarity(term, word); - if synonyms(term).contains(&word) { +fn relatedness(term: &Term, word: &str) -> f64 { + let direct = similarity(&term.word, word, term.typos); + if synonyms(&term.word).contains(&word) { direct.max(SYNONYM_WEIGHT) } else { direct @@ -405,13 +450,15 @@ fn synonyms(term: &str) -> &'static [&'static str] { } /// How well a query term matches one word of a command's text, from 0 to 1. -fn similarity(term: &str, word: &str) -> f64 { +/// `typos` allows a match one or two edits off. +fn similarity(term: &str, word: &str, typos: bool) -> f64 { if term == word { - return 1.0; + return EXACT; } + let term_len = term.chars().count(); // Typing the start of a word: "geo" for "geocoder". - if term.len() >= 3 && word.starts_with(term) { - return 0.7; + if term_len >= 3 && word.starts_with(term) { + return PREFIX; } // The same stem, spelled on for a different ending: "geocoding" and // "geocoder". @@ -421,19 +468,22 @@ fn similarity(term: &str, word: &str) -> f64 { .take_while(|(a, b)| a == b) .count(); if shared >= 5 { - return 0.5; + return STEM; } - // A typo. Short words are left out: up to four letters, one edit turns - // too many words into another real one — "shop" into "show" — and up to - // seven, two do — "convert" into "content". + if !typos { + return 0.0; + } + // Short words are left out: up to four letters, one edit turns too many + // words into another real one — "shop" into "show" — and up to seven, + // two do — "convert" into "content". let edits = strsim::damerau_levenshtein(term, word); - let allowed = match term.len().min(word.len()) { + let allowed = match term_len.min(word.chars().count()) { 0..=4 => 0, 5..=7 => 1, _ => 2, }; if edits <= allowed { - return 0.4; + return TYPO; } 0.0 } @@ -468,36 +518,38 @@ fn raw_tokens(text: &str) -> impl Iterator + '_ { fn singular(word: String) -> String { match word.strip_suffix('s') { - Some(stem) if stem.len() >= 3 && !stem.ends_with('s') => stem.to_string(), + Some(stem) if stem.chars().count() >= 3 && !stem.ends_with('s') => stem.to_string(), _ => word, } } -fn emit(mode: Mode, entries: &[CommandEntry], next_step: bool) -> Result<()> { +fn emit(mode: Mode, entries: &[CommandEntry]) -> Result<()> { + // No table, not an empty line: `text` with no match prints nothing, so + // `mapbox commands search … > file` leaves an empty file. `json` still + // answers `[]`, the one shape a parser can take. + if entries.is_empty() && !mode.is_json() { + return Ok(()); + } let width = entries .iter() .map(|entry| entry.command.len()) .max() .unwrap_or(0) .max("COMMAND".len()); - let text = if entries.is_empty() { - String::new() - } else { - std::iter::once(format!("{:width$} SUMMARY", "COMMAND")) - .chain( - entries - .iter() - .map(|entry| format!("{:width$} {}", entry.command, entry.summary)), - ) - .collect::>() - .join("\n") - }; + let text = std::iter::once(format!("{:width$} SUMMARY", "COMMAND")) + .chain( + entries + .iter() + .map(|entry| format!("{:width$} {}", entry.command, entry.summary)), + ) + .collect::>() + .join("\n"); let json = entries .iter() .map(|entry| json!({ "command": entry.command, "summary": entry.summary })) .collect(); output::emit(mode, &text, Value::Array(json))?; - if next_step && !mode.is_json() { + if !mode.is_json() { output::print_tips(&["`mapbox --help` shows what one takes.".to_string()]); } Ok(()) @@ -556,95 +608,127 @@ mod tests { /// per set, so they run in parallel. /// /// With [`REPORT_DIR_ENV`] set, also writes every query's result to - /// `/`, before the floors are checked so a failing run still - /// leaves one. CI diffs a PR's reports against its base's. + /// `/`, before anything is asserted, so a failing run still + /// leaves one: a set missing from CI's summary would read as a set + /// nobody added. A row that cannot be scored goes in the report as a + /// `# invalid` line rather than stopping the run there. fn eval_set_keeps_its_scores(wanted: &str) { + let &(name, text, min_first, min_top3) = EVAL_SETS + .iter() + .find(|(name, ..)| *name == wanted) + .unwrap_or_else(|| panic!("`{wanted}` is not in EVAL_SETS")); let specs = crate::spec::effective_services().expect("specs load"); let app = crate::build_app(&specs); - let known: BTreeSet = schema::build(&app, &specs, &[]) - .commands - .into_iter() - .map(|entry| entry.command) - .collect(); let index = index(&app, &specs); + // From the index rather than the whole schema: an expected + // `commands` command is one search never answers with. + let known: BTreeSet<&str> = index + .iter() + .map(|(_, entry)| entry.command.as_str()) + .collect(); - let mut failures = vec![]; - for (name, text, min_first, min_top3) in EVAL_SETS.iter().filter(|(n, ..)| *n == wanted) { - let (mut rows, mut first, mut top3) = (0, 0, 0); - let mut misses = vec![]; - let mut report = format!("# floors\t{min_first}\t{min_top3}\n"); - for line in text - .lines() - .filter(|l| !l.starts_with('#') && !l.trim().is_empty()) - { - let mut columns = line.split('\t'); - let query = columns.next().expect("a query"); - let expected: Vec<&str> = match columns.next().expect("an expected column") { - "-" => vec![], - answers => answers.split('|').collect(), - }; - // A renamed command would leave its rows unwinnable, and - // the floor would read that as the search getting worse. - for command in &expected { - assert!( - known.contains(*command), - "{name}: `{command}` is not a command (\"{query}\")" - ); - } - let found: Vec = rank_in(&index, query) - .into_iter() - .take(3) - .map(|i| index[i].1.command.clone()) - .collect(); - let (is_first, in_top3) = if expected.is_empty() { - (found.is_empty(), found.is_empty()) - } else { - ( - found - .first() - .is_some_and(|c| expected.contains(&c.as_str())), - found.iter().any(|c| expected.contains(&c.as_str())), - ) - }; - report.push_str(&format!( - "{query}\t{}\t{}\t{}\t{}\n", - expected.join("|"), - found.join("|"), - u8::from(is_first), - u8::from(in_top3) - )); - rows += 1; - first += usize::from(is_first); - top3 += usize::from(in_top3); - if !is_first { - misses.push(format!(" \"{query}\": want {expected:?}, got {found:?}")); - } - } - if let Some(dir) = std::env::var_os(REPORT_DIR_ENV) { - let dir = std::path::PathBuf::from(dir); - std::fs::create_dir_all(&dir).expect("create the report directory"); - std::fs::write(dir.join(name), report).expect("write the report"); - } - eprintln!( - "{name}: {first}/{rows} first (floor {min_first}), \ - {top3}/{rows} in top 3 (floor {min_top3})" - ); - if first < *min_first || top3 < *min_top3 { - failures.push(format!( - "{name} fell below its floor: {first} first (floor {min_first}), \ - {top3} in top 3 (floor {min_top3}). If the trade-off is intended — \ - help text made accurate at the cost of a match, say — lower the floor \ - in EVAL_SETS to {}/{} and say why in the PR. Not first:\n{}", - first.min(*min_first), - top3.min(*min_top3), - misses.join("\n") + let (mut rows, mut first, mut top3) = (0, 0, 0); + let (mut misses, mut invalid) = (vec![], vec![]); + let mut report = format!("# floors\t{min_first}\t{min_top3}\n"); + for line in text + .lines() + .filter(|l| !l.starts_with('#') && !l.trim().is_empty()) + { + let mut columns = line.split('\t'); + let (Some(query), Some(expected)) = (columns.next(), columns.next()) else { + invalid.push(format!("no expected column: {line:?}")); + continue; + }; + let expected: Vec<&str> = match expected { + "-" => vec![], + answers => answers.split('|').collect(), + }; + // A renamed command would leave its row unwinnable, and the + // floor would read that as the search getting worse. + if let Some(command) = expected.iter().find(|c| !known.contains(**c)) { + invalid.push(format!( + "`{command}` is not a command search answers with (\"{query}\")" )); + continue; } - if first > *min_first || top3 > *min_top3 { - eprintln!(" above its floor: raise it to {first}/{top3} in EVAL_SETS"); + let found: Vec = rank_in(&index, query) + .into_iter() + .take(3) + .map(|i| index[i].1.command.clone()) + .collect(); + let (is_first, in_top3) = if expected.is_empty() { + (found.is_empty(), found.is_empty()) + } else { + ( + found + .first() + .is_some_and(|c| expected.contains(&c.as_str())), + found.iter().any(|c| expected.contains(&c.as_str())), + ) + }; + report.push_str(&format!( + "{query}\t{}\t{}\t{}\t{}\n", + expected.join("|"), + found.join("|"), + u8::from(is_first), + u8::from(in_top3) + )); + rows += 1; + first += usize::from(is_first); + top3 += usize::from(in_top3); + if !is_first { + misses.push(format!(" \"{query}\": want {expected:?}, got {found:?}")); } } - assert!(failures.is_empty(), "{}", failures.join("\n\n")); + for problem in &invalid { + report.push_str(&format!("# invalid\t{problem}\n")); + } + if let Some(dir) = std::env::var_os(REPORT_DIR_ENV) { + let dir = std::path::PathBuf::from(dir); + std::fs::create_dir_all(&dir).expect("create the report directory"); + std::fs::write(dir.join(name), report).expect("write the report"); + } + + eprintln!( + "{name}: {first}/{rows} first (floor {min_first}), \ + {top3}/{rows} in top 3 (floor {min_top3})" + ); + for miss in &misses { + eprintln!("{miss}"); + } + assert!(invalid.is_empty(), "{name}:\n{}", invalid.join("\n")); + assert!( + first >= min_first && top3 >= min_top3, + "{name} fell below its floor: {first} first (floor {min_first}), \ + {top3} in top 3 (floor {min_top3}). If the trade-off is intended — \ + help text made accurate at the cost of a match, say — lower the floor \ + in EVAL_SETS to {}/{} and say why in the PR. The misses are above.", + first.min(min_first), + top3.min(min_top3), + ); + if first > min_first || top3 > min_top3 { + eprintln!(" above its floor: raise it to {first}/{top3} in EVAL_SETS"); + } + } + + /// A file added to the directory but not to [`EVAL_SETS`] would be a + /// set that is never scored. + #[test] + fn every_query_file_is_an_eval_set() { + let dir = concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/commands_search" + ); + let mut files: Vec = std::fs::read_dir(dir) + .expect("read the fixture directory") + .map(|entry| entry.expect("a directory entry").file_name()) + .filter_map(|name| name.into_string().ok()) + .filter(|name| name.ends_with(".tsv")) + .collect(); + files.sort(); + let mut sets: Vec<&str> = EVAL_SETS.iter().map(|(name, ..)| *name).collect(); + sets.sort_unstable(); + assert_eq!(files, sets); } #[test] @@ -736,14 +820,15 @@ mod tests { assert_eq!(kept, ["best", "tenth"]); } + const WEAK_QUERY: &str = "how far can i cycle in twenty minutes"; + /// A loosely worded task matched by one word is still answered: a - /// fixed minimum score once emptied this. + /// fixed minimum score once emptied this. Not a phrasing from any query + /// set, so it does not lean on one the search was fitted to. #[test] fn a_weak_only_match_is_still_an_answer() { assert_eq!( - top("how far can i walk in ten minutes") - .first() - .map(String::as_str), + top(WEAK_QUERY).first().map(String::as_str), Some("mapbox isochrone") ); } @@ -764,32 +849,95 @@ mod tests { ); } + /// "patch" and "match" are a letter from "batch", and both are words a + /// command's text uses as they are, so neither asks for many. + #[test] + fn a_word_one_letter_from_batch_does_not_find_a_batch_command() { + for query in ["patch a style", "match a style"] { + let found = top(query); + assert!( + found.first().is_some_and(|c| !c.contains("batch")), + "\"{query}\" ranked {found:?}" + ); + } + } + + #[test] + fn a_word_some_command_uses_is_not_a_typo() { + let vocabulary = BTreeSet::from(["match"]); + let query = Query::parse("match uplaod", &vocabulary); + let typos: Vec = query.terms.iter().map(|term| term.typos).collect(); + assert_eq!(typos, [false, true]); + assert_eq!(relatedness(&query.terms[0], "batch"), 0.0); + } + + /// The caller is already in `commands`, and its own summaries would + /// otherwise answer every query about finding a command. + #[test] + fn the_commands_group_is_left_out() { + let specs = crate::spec::effective_services().expect("specs load"); + let app = crate::build_app(&specs); + let found = rank(&app, &specs, "find a command search list commands"); + assert!( + found.iter().all(|entry| entry.service != COMMAND), + "{:?}", + found.iter().map(|entry| &entry.command).collect::>() + ); + } + + #[test] + fn a_repeated_pair_is_one_compound() { + let query = Query::parse("log in log in", &BTreeSet::new()); + let compounds: Vec<&str> = query.compounds.iter().map(|(c, _)| c.as_str()).collect(); + assert_eq!(compounds, ["login", "inlog"]); + } + #[test] fn stopwords_are_dropped_before_the_plural_is() { assert!(terms("what is this").is_empty()); } + fn term(word: &str) -> Term { + Term { + word: word.to_string(), + typos: true, + } + } + #[test] fn a_synonym_counts_for_less_than_the_word_itself() { - assert_eq!(relatedness("remove", "remove"), 1.0); - assert_eq!(relatedness("remove", "delete"), SYNONYM_WEIGHT); - assert_eq!(relatedness("delete", "remove"), 0.0); + assert_eq!(relatedness(&term("remove"), "remove"), EXACT); + assert_eq!(relatedness(&term("remove"), "delete"), SYNONYM_WEIGHT); + assert_eq!(relatedness(&term("delete"), "remove"), 0.0); } - /// "batch" is one letter from "match"; a synonym is matched exactly. + /// "bulk" stands in for "batch", and "batch" one letter off is + /// "match": the stand-in is matched exactly, not within a typo. #[test] fn a_synonym_is_not_fuzzy_matched() { - assert_eq!(relatedness("many", "match"), 0.0); + assert_eq!(relatedness(&term("bulk"), "batch"), SYNONYM_WEIGHT); + assert_eq!(similarity("batch", "match", true), TYPO); + assert_eq!(relatedness(&term("bulk"), "match"), 0.0); } #[test] fn a_plural_matches_its_singular() { - assert!(similarity(&terms("styles")[0], &terms("style")[0]) == 1.0); + assert_eq!( + similarity(&terms("styles")[0], &terms("style")[0], true), + EXACT + ); } #[test] fn short_words_are_not_fuzzy_matched() { - assert_eq!(similarity("map", "mcp"), 0.0); - assert_eq!(similarity("shop", "show"), 0.0); + assert_eq!(similarity("map", "mcp", true), 0.0); + assert_eq!(similarity("shop", "show", true), 0.0); + } + + /// Letters, not bytes: "café" is four letters, too short for a typo. + #[test] + fn lengths_count_letters() { + assert_eq!(similarity("cafè", "café", true), 0.0); + assert_eq!(singular("éés".to_string()), "éés"); } } diff --git a/src/config.rs b/src/config.rs index 57ce7b8..cfd159a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -182,7 +182,7 @@ pub fn command() -> Command { }; Command::new(COMMAND) - .about("Get or set persisted settings: telemetry, update checks, history and logs") + .about("Get or set settings: telemetry, update checks, history, logs") .long_about( "Get or set a mapbox setting that persists across shells and sessions, \ written to a file beside the stored credentials rather than an \ diff --git a/src/doctor.rs b/src/doctor.rs index 962e236..3e9faf5 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -107,7 +107,7 @@ const PROXY_VARS: &[(&str, &str)] = &[ pub fn command() -> Command { Command::new(COMMAND) - .about("Show the token, proxy and settings the next command would run with") + .about("Show the token, proxy and settings the next command would use") .long_about( "A read-only snapshot of what the next command would see: which token wins \ and its state, which proxy variables are in effect, and where the \ diff --git a/src/help_layout.rs b/src/help_layout.rs index e7e3cc4..4e58596 100644 --- a/src/help_layout.rs +++ b/src/help_layout.rs @@ -83,7 +83,7 @@ const DESCRIPTIONS: &[(&str, &str)] = &[ ), ( "feedback", - "Report wrong or missing map data to Mapbox, and list what was reported", + "Report wrong or missing map data, and list what was reported", ), ("directions", "Route between waypoints"), ( diff --git a/src/main.rs b/src/main.rs index e6b81a1..3c08bea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -72,9 +72,9 @@ const REPO_URL: &str = repo_url!(); /// by chaining `--help` calls, one command at a time; `--schema` answers the /// same question for every command in one document. const AGENT_HELP_HINT: &str = "\ -Agents: `mapbox commands search \"\"` finds the command for a task. `mapbox --schema` -describes every command, its arguments and the request it makes as one JSON document. Use -these instead of chaining --help calls."; +Agents: `mapbox commands search \"\"` finds the command for a task. +`mapbox --schema` describes every command, its arguments and the request it +makes as one JSON document. Use these instead of chaining --help calls."; /// Help headings for the global options, which otherwise share one /// "Options:" list with every command's own. diff --git a/src/tilesets_cli.rs b/src/tilesets_cli.rs index dec06ee..4b79e7a 100644 --- a/src/tilesets_cli.rs +++ b/src/tilesets_cli.rs @@ -178,7 +178,7 @@ pub fn warn_yes_ignored(matches: &ArgMatches) { pub fn command() -> Command { Command::new(COMMAND) - .about("Run the Mapbox Tilesets CLI (installed separately): upload data, publish tilesets") + .about("Run the separate Tilesets CLI: upload data, publish tilesets") .long_about(format!( "Forward arguments to the Mapbox Tilesets CLI, the command-line client for \ Mapbox Tiling Service (MTS): upload GeoJSON as a tileset source, validate a \ diff --git a/tests/fixtures/commands_search/README.md b/tests/fixtures/commands_search/README.md index 81a5976..9efcc99 100644 --- a/tests/fixtures/commands_search/README.md +++ b/tests/fixtures/commands_search/README.md @@ -5,7 +5,11 @@ tests in `src/commands.rs` run every file here and fail when a set ranks fewer queries right than its floor. `cargo test eval_set -- --nocapture` prints each set's score and every query that did not rank first. On a PR, CI's `commands search eval` job puts the scores in its job summary, with the -queries the change fixed and broke against the base branch. +queries the change fixed and broke against the base branch. It also says +when there was nothing to compare against, and why: a push with no base, a +base from before these sets, or a base run that failed. +`scripts/test-commands-search-report.sh` checks that summary against +hand-written reports. ## Format @@ -53,6 +57,23 @@ Kinds: - Keyword search has a ceiling: a query that shares no words with any command's text cannot be found. Most of the remaining misses are of that kind, and tuning will not fix them. +- A `-` row can only be won when no word of the query matches anything, + because matches are cut relative to the best one, never at a fixed + minimum (see `MIN_SHARE_OF_BEST`). A task the CLI cannot do almost always + shares a word with some command, so those rows are lost today, and the + rows a set can actually win are fewer than its total: + + | File | Rows | `-` rows | `-` rows won | Rows that can win | + | --- | --- | --- | --- | --- | + | `tuning.tsv` | 100 | 2 | 2 | 100 | + | `issues.tsv` | 100 | 11 | 0 | 89 | + | `docs.tsv` | 60 | 11 | 0 | 49 | + + They stay in the score, rather than being scored apart, so a set's score + keeps one meaning across changes, and a change that starts answering + "nothing" for them shows up as a gain in the same number. `tuning.tsv` + wins its two only because "bake a cake" and "deploy to kubernetes" share + no word with any command. ## Limits diff --git a/tests/output_contract.rs b/tests/output_contract.rs index ce1dec6..d220bcf 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1340,6 +1340,38 @@ fn commands_search_with_no_match_is_an_empty_list() { assert_eq!(json(&stdout(&out)), serde_json::json!([])); } +/// Under `text` the empty answer is no output at all, not a blank line, so +/// `> file` leaves an empty file. Where to look instead goes to stderr, and +/// the `--help` tip, which is about a match, does not. +#[test] +fn commands_search_with_no_match_prints_nothing_as_text() { + let out = run(&["-o", "text", "commands", "search", "xyzzy"]); + assert!(out.status.success(), "{}", stderr(&out)); + assert_eq!(stdout(&out), ""); + assert!( + stderr(&out).contains("mapbox commands list"), + "{}", + stderr(&out) + ); + assert!(!stderr(&out).contains("Tip:"), "{}", stderr(&out)); +} + +/// `--limit` caps the matches at its value, five by default, and `0` +/// lifts the cap. "style" matches more than five commands. +#[test] +fn commands_search_limit_caps_the_matches() { + let count = |extra: &[&str]| { + let mut args = vec!["-o", "json", "commands", "search", "style"]; + args.extend_from_slice(extra); + let out = run(&args); + assert!(out.status.success(), "{}", stderr(&out)); + json(&stdout(&out)).as_array().expect("an array").len() + }; + assert_eq!(count(&[]), 5); + assert_eq!(count(&["--limit", "1"]), 1); + assert!(count(&["--limit", "0"]) > 5); +} + #[test] fn commands_list_names_every_command_once() { let out = run(&["-o", "json", "commands", "list"]); @@ -1360,6 +1392,9 @@ fn commands_list_names_every_command_once() { .collect(); described.sort_unstable(); assert_eq!(listed, described); + let mut unique = listed.clone(); + unique.dedup(); + assert_eq!(unique, listed, "a command is listed twice"); } /// A command name nothing resembles is pointed at `commands search`, in @@ -1392,4 +1427,57 @@ fn an_unknown_command_is_pointed_at_search() { .is_some_and(|fix| fix.contains("'list'")), "{error}" ); + assert!( + !error["fix"] + .as_str() + .is_some_and(|fix| fix.contains("commands search")), + "{error}" + ); + + let out = run(&["-o", "text", "styles", "lst"]); + assert!(stderr(&out).contains("'list'"), "{}", stderr(&out)); + assert!( + !stderr(&out).contains("commands search"), + "{}", + stderr(&out) + ); +} + +/// The closing line of top-level help points a person at `commands search` +/// too, not only an agent reading the hint above it. +#[test] +fn top_level_help_points_a_person_at_search() { + let out = run(&["--help"]); + assert!(out.status.success()); + let text = stdout(&out); + assert!( + text.lines() + .any(|line| line.starts_with("Run mapbox commands search \"\"")), + "{text}" + ); +} + +/// Each command's row in top-level help fits one line at 80 columns: a +/// summary that wraps reads as two rows at a glance. The options may wrap, +/// since their environment notes are long by nature. +#[test] +fn every_command_fits_one_help_line_at_80_columns() { + let out = command() + .env("COLUMNS", "80") + .env("CLAUDECODE", "1") + .arg("--help") + .output() + .expect("run mapbox"); + let text = stdout(&out); + let (hint, page) = text.split_at(text.find("Usage:").expect("a usage line")); + for line in hint.lines() { + assert!(line.chars().count() <= 80, "over 80 columns: {line:?}"); + } + let commands = &page[..page.find("Authentication:").expect("an options section")]; + for line in commands.lines() { + assert!( + !line.starts_with(" "), + "a command's summary wraps: {line:?}\n{commands}" + ); + } } From 62e19f021fb89633b5f8ded9744142aaee8bdf60 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Tue, 20 Oct 2026 14:08:10 +0300 Subject: [PATCH 03/10] Rewrap two docs paragraphs to the files' line width --- AGENTS.md | 12 ++++++------ docs/commands.md | 5 +++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3723215..295a9fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,12 +128,12 @@ an entry outlives its command. A command it misses still shows, under ## `commands search` and its query sets -`mapbox commands search` ranks commands by the words in their names, summaries, -descriptions, help-group lines and argument help, so **editing a command's -`about`, an argument's `help` or a line in `DESCRIPTIONS` changes search -results**, not only `--help`. The `eval_set_*` -tests in `src/commands.rs` run the query sets in `tests/fixtures/commands_search/` -and hold each to a floor of how many queries rank right. Each file's header says +`mapbox commands search` ranks commands by the words in their names, +summaries, descriptions, help-group lines and argument help, so **editing a +command's `about`, an argument's `help` or a line in `DESCRIPTIONS` changes +search results**, not only `--help`. The `eval_set_*` tests in +`src/commands.rs` run the query sets in `tests/fixtures/commands_search/` and +hold each to a floor of how many queries rank right. Each file's header says where its queries came from and how much the search was tuned against them. `cargo test eval_set -- --nocapture` prints the scores and every miss, and that directory's README explains each set and how to read its score. diff --git a/docs/commands.md b/docs/commands.md index c063d5e..b7ca483 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4754,8 +4754,9 @@ and description, its group's description and its arguments' help, with a command's name counting most. A word can be a prefix (`geo`), a different ending of the same stem (`geocoding` for `geocoder`) or one typo away (`uplaod`), unless some command's text uses the word as it is: "match" is a -word, not a typo for "batch". A plural matches its singular. A few common words stand in for -the ones commands use ("remove" for "delete", "rename" for "update"). +word, not a typo for "batch". A plural matches its singular. A few common +words stand in for the ones commands use ("remove" for "delete", "rename" +for "update"). Words like "how", "my", "to" and "mapbox" are ignored. It cannot find a command whose text uses none of the query's words, so a query names the action and the thing acted on: "upload a font", not "get my typeface onto From d2594f29b961cb200bdf1454a97edc9b635408d3 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Tue, 20 Oct 2026 14:13:02 +0300 Subject: [PATCH 04/10] Name the eval summary's columns plainly and show each score as a share Scores read "22 (37%)", with the change against the base when there is one, and a legend under the table says what each column means. --- scripts/commands-search-report.py | 29 ++++++++++++++++++-------- scripts/test-commands-search-report.sh | 4 ++++ 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py index 0e78f4f..6e680fc 100644 --- a/scripts/commands-search-report.py +++ b/scripts/commands-search-report.py @@ -93,11 +93,22 @@ def score(report, key): return sum(row[key] for row in report.rows.values()) -def delta(head, base): - if base is None: - return f"{head}" - change = head - base - return f"{head} ({change:+d})" if change else f"{head}" +# Under the table, so a reader need not know the tests to read it. +LEGEND = ( + "**Ranked first**: the expected command was the first result. " + "**In top 3**: it was among the first three. " + "**Required**: the floor in `EVAL_SETS` (src/commands.rs); a lower score fails `cargo test`. " + "A query the CLI cannot serve counts as right only when nothing comes back. " + "How each set was gathered is in tests/fixtures/commands_search/README.md." +) + + +def count(head, total, base): + """`22 (37%)`, with the change against the base when there is one: + `22 (37%, +1)`.""" + share = f"{round(100 * head / total)}%" if total else "-" + change = head - base if base is not None else 0 + return f"{head} ({share}, {change:+d})" if change else f"{head} ({share})" def changed(head_rows, base_rows): @@ -185,7 +196,7 @@ def render(head, head_status, base, base_given, base_status, expected_sets): compare = bool(base) out += [ - "| Set | Rows | First | Top 3 | Floor | Note |", + "| Query set | Queries | Ranked first | In top 3 | Required (first / top 3) | Note |", "| --- | --- | --- | --- | --- | --- |", ] details = [] @@ -208,8 +219,8 @@ def render(head, head_status, base, base_given, base_status, expected_sets): if compare and base_report is None: row_notes.insert(0, "only in this branch") out.append( - f"| {label} | {len(report.rows)} | {delta(first, base_first)} | " - f"{delta(top3, base_top3)} | {floor} | {cell('; '.join(row_notes))} |" + f"| {label} | {len(report.rows)} | {count(first, len(report.rows), base_first)} | " + f"{count(top3, len(report.rows), base_top3)} | {floor} | {cell('; '.join(row_notes))} |" ) if report.invalid: @@ -241,7 +252,7 @@ def render(head, head_status, base, base_given, base_status, expected_sets): "", ] - return out + ["", *details] + return out + ["", LEGEND, "", *details] def main(): diff --git a/scripts/test-commands-search-report.sh b/scripts/test-commands-search-report.sh index 5fbdf6f..46b7803 100755 --- a/scripts/test-commands-search-report.sh +++ b/scripts/test-commands-search-report.sh @@ -107,6 +107,10 @@ if summarize --head "$HEAD" --head-status 0 --base "$BASE" --base-status 0 --set expect "a fix against the base is listed" "1 now first" expect "a set new in this branch is marked" "only in this branch" expect "a pipe in a cell is escaped" 'a \| b query' + expect "a score carries its share and its change" "| 1 (50%, +1) |" + expect "an unchanged score carries its share alone" "| 1 (50%) |" + expect "the columns are named" "| Query set | Queries | Ranked first | In top 3 |" + expect "the legend says what the columns mean" "**Ranked first**: the expected command" reject "no emoji" "⚠" fi From 4d57d8f6190db6829040e62b5069ecd79abcbb1a Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Tue, 20 Oct 2026 14:18:33 +0300 Subject: [PATCH 05/10] Comment on a PR when commands search drops far against its base A set ranking 3 or more fewer queries first, or losing 5 or more points, gets the PR one comment, updated on later pushes and marked recovered once the drop is gone. CI's search-eval job only writes the comment as an artifact, since a fork's PR has a read-only token; a workflow_run workflow posts it without checking out or running the PR's code. --- .github/workflows/ci.yml | 14 ++- .github/workflows/search-eval-comment.yml | 107 ++++++++++++++++++++++ scripts/commands-search-report.py | 105 +++++++++++++++++++-- scripts/test-commands-search-report.sh | 50 ++++++++++ tests/fixtures/commands_search/README.md | 7 +- 5 files changed, 272 insertions(+), 11 deletions(-) create mode 100644 .github/workflows/search-eval-comment.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec5df74..3d01d7b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -187,10 +187,22 @@ jobs: set -- --head "$RUNNER_TEMP/eval-head" --head-status "$HEAD_STATUS" \ --sets tests/fixtures/commands_search if [ "$EVENT" = pull_request ]; then - set -- "$@" --base "$RUNNER_TEMP/eval-base" --base-status "$BASE_STATUS" + set -- "$@" --base "$RUNNER_TEMP/eval-base" --base-status "$BASE_STATUS" \ + --comment-dir "$RUNNER_TEMP/eval-comment" fi python3 scripts/commands-search-report.py "$@" >>"$GITHUB_STEP_SUMMARY" + # For search-eval-comment.yml, which posts a PR comment on a large drop. + # This job cannot post one itself: a fork's PR gets a read-only token. + - name: Keep the comment for the comment workflow + if: ${{ !cancelled() && github.event_name == 'pull_request' }} + uses: actions/upload-artifact@v7 + with: + name: search-eval-comment + path: ${{ runner.temp }}/eval-comment + retention-days: 1 + if-no-files-found: ignore + # No Rust: this is where the repo's shell gets checked, install.sh above all # — it is served to machines we do not control, and this is the only thing # that checks it. Both runners, because the portability it has to get right diff --git a/.github/workflows/search-eval-comment.yml b/.github/workflows/search-eval-comment.yml new file mode 100644 index 0000000..d8574d4 --- /dev/null +++ b/.github/workflows/search-eval-comment.yml @@ -0,0 +1,107 @@ +name: commands search eval comment + +# Comments on a PR whose `commands search` eval dropped far enough against +# its base (see LARGE_DROP_QUERIES in scripts/commands-search-report.py), and +# updates that one comment on later pushes, to say so once it recovers. +# +# Two workflows rather than one step in CI: a `pull_request` run from a fork +# gets a read-only token, so CI's search-eval job only leaves an artifact. +# This runs from main's copy of this file, with write access, and so must +# never run the PR's code. It checks nothing out and reads only that +# artifact, as untrusted text: the PR is found from the triggering run's head +# commit rather than from anything the artifact says, the body is capped, +# and @-mentions in it are defused. +# +# `workflow_run` only fires for the copy of this file on the default branch, +# so a PR that changes it does not run its own version. +on: + workflow_run: + workflows: [CI] + types: [completed] + +permissions: {} + +jobs: + comment: + if: github.event.workflow_run.event == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + actions: read + pull-requests: write + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + RUN_ID: ${{ github.event.workflow_run.id }} + RUN_URL: ${{ github.event.workflow_run.html_url }} + steps: + # Missing when the run was canceled before the eval, or failed to build + # it; there is then nothing to say. + - name: Download the eval's comment + run: | + gh run download "$RUN_ID" --repo "$REPO" --name search-eval-comment \ + --dir "$RUNNER_TEMP/comment" || echo "No comment artifact." + + - name: Post or update the comment + run: | + dir="$RUNNER_TEMP/comment" + if [ ! -f "$dir/state" ]; then + echo "Nothing to do." + exit 0 + fi + # Matched against fixed words and never echoed: printed, a crafted + # value could read as a workflow command. + state=$(head -n 1 "$dir/state") + + # The open PR whose head is the commit CI ran on. Never the + # artifact's word for it: a fork controls what its run uploads. + pr=$(gh pr list --repo "$REPO" --state open --limit 200 \ + --json number,headRefOid \ + --jq ".[] | select(.headRefOid == \"$HEAD_SHA\") | .number" | head -1) + if [ -z "$pr" ]; then + echo "No open PR has head $HEAD_SHA." + exit 0 + fi + + marker='' + existing=$(gh api --paginate "repos/$REPO/issues/$pr/comments" \ + --jq ".[] | select(.user.login == \"github-actions[bot]\" and (.body | startswith(\"$marker\"))) | .id" | head -1) + + body="$RUNNER_TEMP/body.md" + case "$state" in + drop) + { + echo "$marker" + # A zero-width space after each @ keeps a crafted body from + # pinging anyone. + head -c 60000 "$dir/body.md" | sed 's/@/@\xe2\x80\x8b/g' + echo + echo "Every query that changed is in the [job summary]($RUN_URL). Updated for $HEAD_SHA." + } >"$body" + ;; + ok) + if [ -z "$existing" ]; then + echo "No large drop, and no earlier comment to update." + exit 0 + fi + { + echo "$marker" + echo "### \`commands search\` recovered" + echo + echo "No query set ranks far fewer queries first than the base any more, as of $HEAD_SHA. [Job summary]($RUN_URL)." + } >"$body" + ;; + *) + echo "Nothing to compare against." + exit 0 + ;; + esac + + if [ -n "$existing" ]; then + gh api --method PATCH "repos/$REPO/issues/comments/$existing" -F "body=@$body" >/dev/null + echo "Updated comment $existing on #$pr." + else + gh api "repos/$REPO/issues/$pr/comments" -F "body=@$body" >/dev/null + echo "Commented on #$pr." + fi diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py index 6e680fc..602a6b8 100644 --- a/scripts/commands-search-report.py +++ b/scripts/commands-search-report.py @@ -4,6 +4,7 @@ commands-search-report.py --head DIR --head-status N [--base DIR --base-status N] [--sets DIR] + [--comment-dir DIR] Each directory holds the per-set reports the `eval_set_*` tests write when MAPBOX_SEARCH_EVAL_REPORT names it; each status is the exit code of the @@ -11,6 +12,12 @@ (not a pull request). `--sets` is the fixture directory, so a set whose report is missing is named rather than left out. +With `--comment-dir`, also writes what the PR comment workflow +(.github/workflows/search-eval-comment.yml) needs: `state`, one of `drop` +(some set dropped by LARGE_DROP_QUERIES or LARGE_DROP_POINTS against the +base), `ok` (compared, no large drop) or `none` (nothing to compare), and, +for `drop`, `body.md`. + The status is what tells apart a base that predates the eval (the run passed and wrote nothing) from one whose run failed: the reports alone look the same for both. @@ -93,6 +100,16 @@ def score(report, key): return sum(row[key] for row in report.rows.values()) +# A set losing this many first-place queries against the base, or this many +# percentage points of its first-place share, gets the PR a comment. Smaller +# trade-offs stay in the job summary: the floors already make each one a +# decision in the diff, and a comment for every one would be noise. +LARGE_DROP_QUERIES = 3 +LARGE_DROP_POINTS = 5 + +# Rows of "no longer first" per set in a comment; the job summary has the rest. +COMMENT_ROWS = 20 + # Under the table, so a reader need not know the tests to read it. LEGEND = ( "**Ranked first**: the expected command was the first result. " @@ -255,6 +272,80 @@ def render(head, head_status, base, base_given, base_status, expected_sets): return out + ["", LEGEND, "", *details] +def share(first, total): + return 100 * first / total if total else 0.0 + + +def large_drops(head, base): + """Each set that dropped far enough against the base to comment on, as + (name, base first, base rows, head first, head rows).""" + drops = [] + for name in sorted(set(head) & set(base)): + report, base_report = head[name], base[name] + first, base_first = score(report, "first"), score(base_report, "first") + rows, base_rows = len(report.rows), len(base_report.rows) + lost = base_first - first + points = share(base_first, base_rows) - share(first, rows) + if lost >= LARGE_DROP_QUERIES or points >= LARGE_DROP_POINTS: + drops.append((name, base_first, base_rows, first, rows)) + return drops + + +def comment(head, head_status, base, base_status): + """The comment state, and its body when there is a large drop.""" + if not head or not base or base_status != 0 or head_status is None: + return "none", None + drops = large_drops(head, base) + if not drops: + return "ok", None + out = [ + "### `commands search` ranks fewer queries first than the base", + "", + "| Query set | Base | This PR | Change |", + "| --- | --- | --- | --- |", + ] + details = [] + for name, base_first, base_rows, first, rows in drops: + points = share(base_first, base_rows) - share(first, rows) + out.append( + f"| `{cell(name)}` | {count(base_first, base_rows, None)} | " + f"{count(first, rows, None)} | {first - base_first:+d} queries, " + f"{-points:+.0f} points |" + ) + _, broken = changed(head[name].rows, base[name].rows) + if broken: + shown = broken[:COMMENT_ROWS] + more = len(broken) - len(shown) + details += [ + f"
{cell(name)}: {len(broken)} no longer first", + "", + "| Query | Want | Base | This PR |", + "| --- | --- | --- | --- |", + *shown, + *([f"\n{more} more in the job summary."] if more else []), + "", + "
", + "", + ] + out += [ + "", + f"A set losing {LARGE_DROP_QUERIES} or more first-place queries, or " + f"{LARGE_DROP_POINTS} or more points, posts this. If the trade-off is " + "intended, say why in the PR; the floors in `EVAL_SETS` decide whether " + "`cargo test` passes.", + "", + *details, + ] + return "drop", "\n".join(out) + + +def write_comment(directory, state, body): + directory.mkdir(parents=True, exist_ok=True) + (directory / "state").write_text(state + "\n", encoding="utf-8") + if body is not None: + (directory / "body.md").write_text(body + "\n", encoding="utf-8") + + def main(): parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) parser.add_argument("--head", type=Path, required=True) @@ -262,20 +353,18 @@ def main(): parser.add_argument("--base", type=Path) parser.add_argument("--base-status", default="") parser.add_argument("--sets", type=Path) + parser.add_argument("--comment-dir", type=Path) args = parser.parse_args() expected_sets = [] if args.sets is not None and args.sets.is_dir(): expected_sets = [path.name for path in args.sets.glob("*.tsv")] - lines = render( - load(args.head), - parse_status(args.head_status), - load(args.base), - args.base is not None, - parse_status(args.base_status), - expected_sets, - ) + head, base = load(args.head), load(args.base) + head_status, base_status = parse_status(args.head_status), parse_status(args.base_status) + lines = render(head, head_status, base, args.base is not None, base_status, expected_sets) print("\n".join(lines)) + if args.comment_dir is not None: + write_comment(args.comment_dir, *comment(head, head_status, base, base_status)) if __name__ == "__main__": diff --git a/scripts/test-commands-search-report.sh b/scripts/test-commands-search-report.sh index 46b7803..a6820d0 100755 --- a/scripts/test-commands-search-report.sh +++ b/scripts/test-commands-search-report.sh @@ -159,6 +159,56 @@ if summarize --head "$BAD" --head-status 101 --sets "$SETS"; then expect "an unreadable report is named" "unreadable" fi +# The PR comment: `drop` only past the threshold, `ok` when compared and +# below it, `none` when there was nothing to compare against. +# Four rows each; a "won" row ranks first, a "lost" one does not. +won() { row "$1" 'mapbox x' 'mapbox x' 1 1; } +lost() { row "$1" 'mapbox x' 'mapbox y' 0 0; } +report "$WORK/cbase/a.tsv" "$(won q1)" "$(won q2)" "$(won q3)" "$(won q4)" +report "$WORK/cdrop/a.tsv" "$(lost q1)" "$(lost q2)" "$(lost q3)" "$(won q4)" +report "$WORK/cslip/a.tsv" "$(won q1)" "$(won q2)" "$(won q3)" "$(won q4)" + +check_state() { + case_name="$1" + want="$2" + got="$(cat "$WORK/comment/state" 2>/dev/null || echo missing)" + if [ "$got" = "$want" ]; then + say "ok: $case_name" + else + fail "$case_name: state is $got, not $want" + fi +} + +if summarize --head "$WORK/cdrop" --head-status 0 --base "$WORK/cbase" --base-status 0 \ + --comment-dir "$WORK/comment"; then + check_state "three queries lost is a drop" drop + cp "$WORK/comment/body.md" "$WORK/out" + expect "the comment names the set and the change" "| \`a.tsv\` | 4 (100%) | 1 (25%) | -3 queries, -75 points |" + expect "the comment lists what is no longer first" "3 no longer first" +fi +rm -rf "$WORK/comment" + +if summarize --head "$WORK/cslip" --head-status 0 --base "$WORK/cbase" --base-status 0 \ + --comment-dir "$WORK/comment"; then + check_state "no loss is ok" ok + if [ -e "$WORK/comment/body.md" ]; then + fail "an ok state wrote a comment body" + else + say "ok: an ok state writes no body" + fi +fi +rm -rf "$WORK/comment" + +if summarize --head "$WORK/cdrop" --head-status 0 --base "$WORK/cbase" --base-status 101 \ + --comment-dir "$WORK/comment"; then + check_state "a failed base run is nothing to compare" none +fi +rm -rf "$WORK/comment" + +if summarize --head "$WORK/cdrop" --head-status 0 --comment-dir "$WORK/comment"; then + check_state "no base is nothing to compare" none +fi + if [ "$failures" -ne 0 ]; then say "$failures check(s) failed." exit 1 diff --git a/tests/fixtures/commands_search/README.md b/tests/fixtures/commands_search/README.md index 9efcc99..6e5dbb9 100644 --- a/tests/fixtures/commands_search/README.md +++ b/tests/fixtures/commands_search/README.md @@ -8,8 +8,11 @@ CI's `commands search eval` job puts the scores in its job summary, with the queries the change fixed and broke against the base branch. It also says when there was nothing to compare against, and why: a push with no base, a base from before these sets, or a base run that failed. -`scripts/test-commands-search-report.sh` checks that summary against -hand-written reports. +When a set ranks 3 or more fewer queries first than the base, or loses 5 or +more points, `.github/workflows/search-eval-comment.yml` also comments on +the PR, and updates that comment once a later push recovers. +`scripts/test-commands-search-report.sh` checks the summary and the +comment's content against hand-written reports. ## Format From d596ddae998f08ad4248e8ef2425ccb4f2f35e59 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Tue, 20 Oct 2026 18:42:48 +0300 Subject: [PATCH 06/10] Move commands search's insertions away from mapbox workflow's --- CHANGELOG.md | 14 +++++++------- src/main.rs | 14 +++++++------- src/schema.rs | 8 ++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7c6555..066c5e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,13 +19,6 @@ that may never merge. They are not releases and are not listed here. ## Unreleased -- New `mapbox commands search ""` finds the commands that match a - task, best first, and `mapbox commands list` lists every command with its - summary. Both are local keyword lookups: no request, no token. `json` - gives `[{command, summary}]`. The query is sent in telemetry as its length - only. Top-level help now points to `commands search` (agents see it in - their hint), and an unknown command with no near spelling suggests it in - the error's `fix`. No existing command or output changes. - New `mapbox report ""` sends feedback about the CLI itself to 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, @@ -38,6 +31,13 @@ that may never merge. They are not releases and are not listed here. `cli_token_unavailable` and `cli_token_forbidden` (see [Errors](docs/commands.md#errors)). +- New `mapbox commands search ""` finds the commands that match a + task, best first, and `mapbox commands list` lists every command with its + summary. Both are local keyword lookups: no request, no token. `json` + gives `[{command, summary}]`. The query is sent in telemetry as its length + only. Top-level help now points to `commands search` (agents see it in + their hint), and an unknown command with no near spelling suggests it in + the error's `fix`. No existing command or output changes. - 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/src/main.rs b/src/main.rs index 3c08bea..4a8173d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1410,13 +1410,6 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - Some(("unset", unset_matches)) => config::unset(unset_matches, mode)?, _ => unreachable!("`config` sets subcommand_required(true)"), }, - // Ahead of the generic service arm too: it reads local history and - // makes no request. - Some((history::COMMAND, history_matches)) => match history_matches.subcommand() { - Some(("list", list_matches)) => history::list(list_matches, mode)?, - Some(("show", show_matches)) => history::show(show_matches, mode)?, - _ => unreachable!("`history` sets subcommand_required(true)"), - }, // Ahead of the generic service arm too, and handed `app` for the // same reason `generate-skills` is: it reads the command tree already // in memory. No token and no request. @@ -1425,6 +1418,13 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - Some(("list", _)) => commands::list(app, specs, mode)?, _ => unreachable!("`commands` sets subcommand_required(true)"), }, + // Ahead of the generic service arm too: it reads local history and + // makes no request. + Some((history::COMMAND, history_matches)) => match history_matches.subcommand() { + Some(("list", list_matches)) => history::list(list_matches, mode)?, + Some(("show", show_matches)) => history::show(show_matches, mode)?, + _ => unreachable!("`history` sets subcommand_required(true)"), + }, // Also ahead of the generic service arm: read-only except for the // opt-in `--verify` request, and needs no credential load of its own // — it reports what one would resolve to, not what a fresh one diff --git a/src/schema.rs b/src/schema.rs index d034072..731f012 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -386,18 +386,18 @@ fn commands(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec Date: Tue, 20 Oct 2026 18:43:10 +0300 Subject: [PATCH 07/10] Place the commands search changelog entry where no other open PR edits --- CHANGELOG.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 066c5e4..5ee600a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,13 +31,6 @@ that may never merge. They are not releases and are not listed here. `cli_token_unavailable` and `cli_token_forbidden` (see [Errors](docs/commands.md#errors)). -- New `mapbox commands search ""` finds the commands that match a - task, best first, and `mapbox commands list` lists every command with its - summary. Both are local keyword lookups: no request, no token. `json` - gives `[{command, summary}]`. The query is sent in telemetry as its length - only. Top-level help now points to `commands search` (agents see it in - their hint), and an unknown command with no near spelling suggests it in - the error's `fix`. No existing command or output changes. - 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, @@ -57,6 +50,13 @@ that may never merge. They are not releases and are not listed here. from `tilequery get` and the renderer kept matching the old name. `-o json` is unchanged. +- New `mapbox commands search ""` finds the commands that match a + task, best first, and `mapbox commands list` lists every command with its + summary. Both are local keyword lookups: no request, no token. `json` + gives `[{command, summary}]`. The query is sent in telemetry as its length + only. Top-level help now points to `commands search` (agents see it in + their hint), and an unknown command with no near spelling suggests it in + the error's `fix`. No existing command or output changes. - `mapbox usage` reads as a chart: one row per product with no blank lines between, every sparkline as wide as the period, bars scaled from zero so their height is proportional to the value, and the tallest From 0375a092fa57bd67287770d48a8d3bf3b578de36 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Wed, 21 Oct 2026 11:03:42 +0300 Subject: [PATCH 08/10] Address review on commands search and move its eval tooling out Drop search-eval-comment.yml: a fork's CI run could make it post arbitrary markdown. Move the search-eval CI job and its report scripts to a follow-up. The eval sets now fail only when a set drops more than 3 queries below its recorded score, so a reworded summary no longer fails the build. Search results under json carry a score from 0 to 1, a query with no words is a usage error, and the mcp install and config help text is built from SERVERS, CLIENTS and KEYS. --- .github/workflows/ci.yml | 101 +----- .github/workflows/search-eval-comment.yml | 107 ------ AGENTS.md | 15 +- CHANGELOG.md | 3 +- docs/commands.md | 15 +- scripts/commands-search-report.py | 380 ---------------------- scripts/test-commands-search-report.sh | 217 ------------ src/commands.rs | 155 +++++---- src/config.rs | 2 +- src/mcp.rs | 20 +- tests/fixtures/commands_search/README.md | 19 +- tests/output_contract.rs | 29 +- 12 files changed, 157 insertions(+), 906 deletions(-) delete mode 100644 .github/workflows/search-eval-comment.yml delete mode 100644 scripts/commands-search-report.py delete mode 100755 scripts/test-commands-search-report.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d01d7b..3a7db73 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,96 +113,6 @@ jobs: shell: bash run: sh scripts/test-completion.sh - # Which `commands search` queries a PR fixed and broke, against its base, - # written to the job summary. It reports and never fails: the floors that - # `cargo test` holds in the build job are the gate. A job summary rather - # than a PR comment because it needs no write token, so a PR from a fork - # gets one too. One runner: ranking is the same everywhere. - search-eval: - name: commands search eval - runs-on: ubuntu-latest - timeout-minutes: 20 - permissions: - contents: read - steps: - # Whole history, so the base commit is there to check out beside it. - # No credential left behind: nothing here pushes or fetches again. - - uses: actions/checkout@v7 - with: - fetch-depth: 0 - persist-credentials: false - - # As in the build job: install exactly the pinned toolchain. - - name: Read the pinned toolchain - id: toolchain - run: | - echo "toolchain=$(sed -n 's/^channel = "\(.*\)"/\1/p' rust-toolchain.toml | head -1)" >>"$GITHUB_OUTPUT" - - - name: Install Rust - uses: dtolnay/rust-toolchain@stable - with: - toolchain: ${{ steps.toolchain.outputs.toolchain }} - - # The exit status is recorded rather than failing the step: a run below - # its floor still writes its reports, the build job is where that - # failure is reported, and the summary needs the status to say whether - # missing reports mean a failed run or one with no eval to run. - - name: Run the eval on this branch - id: head - run: | - status=0 - MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-head" cargo test --locked --bin mapbox eval_set || status=$? - echo "status=$status" >>"$GITHUB_OUTPUT" - - # In a worktree sharing this target directory, so only the crate itself - # rebuilds. A base from before the eval existed matches no test, passes - # and writes no reports; one that fails to build or check out exits - # non-zero. The summary tells the two apart by the status. - - name: Run the eval on the base - id: base - if: github.event_name == 'pull_request' - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - run: | - status=0 - { - git worktree add --detach "$RUNNER_TEMP/base" "$BASE_SHA" && - cd "$RUNNER_TEMP/base" && - CARGO_TARGET_DIR="$GITHUB_WORKSPACE/target" \ - MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-base" \ - cargo test --locked --bin mapbox eval_set - } || status=$? - echo "status=$status" >>"$GITHUB_OUTPUT" - - # No `--base` on a push: the script then says there was no base rather - # than that the base had no reports. A status left empty by a step that - # never finished reads as "did not finish". - - name: Write the summary - if: ${{ !cancelled() }} - env: - EVENT: ${{ github.event_name }} - HEAD_STATUS: ${{ steps.head.outputs.status }} - BASE_STATUS: ${{ steps.base.outputs.status }} - run: | - set -- --head "$RUNNER_TEMP/eval-head" --head-status "$HEAD_STATUS" \ - --sets tests/fixtures/commands_search - if [ "$EVENT" = pull_request ]; then - set -- "$@" --base "$RUNNER_TEMP/eval-base" --base-status "$BASE_STATUS" \ - --comment-dir "$RUNNER_TEMP/eval-comment" - fi - python3 scripts/commands-search-report.py "$@" >>"$GITHUB_STEP_SUMMARY" - - # For search-eval-comment.yml, which posts a PR comment on a large drop. - # This job cannot post one itself: a fork's PR gets a read-only token. - - name: Keep the comment for the comment workflow - if: ${{ !cancelled() && github.event_name == 'pull_request' }} - uses: actions/upload-artifact@v7 - with: - name: search-eval-comment - path: ${{ runner.temp }}/eval-comment - retention-days: 1 - if-no-files-found: ignore - # No Rust: this is where the repo's shell gets checked, install.sh above all # — it is served to machines we do not control, and this is the only thing # that checks it. Both runners, because the portability it has to get right @@ -226,7 +136,6 @@ jobs: sh -n scripts/install.sh sh -n scripts/test-install.sh sh -n scripts/test-completion.sh - sh -n scripts/test-commands-search-report.sh # -s sh, not the default: bash would accept things `curl | sh` will not. # Once is enough — the findings do not depend on the runner. The version @@ -236,19 +145,11 @@ jobs: # Kept clean against 0.9, 0.10 and 0.11. - name: Shellcheck if: runner.os == 'Linux' - run: shellcheck -s sh scripts/install.sh scripts/test-install.sh scripts/test-completion.sh scripts/test-commands-search-report.sh + run: shellcheck -s sh scripts/install.sh scripts/test-install.sh scripts/test-completion.sh - name: Install, verify and prompt against a local channel run: sh scripts/test-install.sh - # The `commands search eval` job's summary script, against hand-written - # reports: that job never fails, so this is where a summary that says - # the wrong thing would be caught. Python only, so it lives here rather - # than beside the Rust. Once is enough, as for shellcheck. - - name: Summarize hand-written search eval reports - if: runner.os == 'Linux' - run: sh scripts/test-commands-search-report.sh - # The other half of the same job, for the installer install.sh stops short # of. Both shells, because both are on a stock Windows 11 and they differ # exactly where this script lives: the default TLS versions, what `2>` does diff --git a/.github/workflows/search-eval-comment.yml b/.github/workflows/search-eval-comment.yml deleted file mode 100644 index d8574d4..0000000 --- a/.github/workflows/search-eval-comment.yml +++ /dev/null @@ -1,107 +0,0 @@ -name: commands search eval comment - -# Comments on a PR whose `commands search` eval dropped far enough against -# its base (see LARGE_DROP_QUERIES in scripts/commands-search-report.py), and -# updates that one comment on later pushes, to say so once it recovers. -# -# Two workflows rather than one step in CI: a `pull_request` run from a fork -# gets a read-only token, so CI's search-eval job only leaves an artifact. -# This runs from main's copy of this file, with write access, and so must -# never run the PR's code. It checks nothing out and reads only that -# artifact, as untrusted text: the PR is found from the triggering run's head -# commit rather than from anything the artifact says, the body is capped, -# and @-mentions in it are defused. -# -# `workflow_run` only fires for the copy of this file on the default branch, -# so a PR that changes it does not run its own version. -on: - workflow_run: - workflows: [CI] - types: [completed] - -permissions: {} - -jobs: - comment: - if: github.event.workflow_run.event == 'pull_request' - runs-on: ubuntu-latest - timeout-minutes: 5 - permissions: - actions: read - pull-requests: write - env: - GH_TOKEN: ${{ github.token }} - REPO: ${{ github.repository }} - HEAD_SHA: ${{ github.event.workflow_run.head_sha }} - RUN_ID: ${{ github.event.workflow_run.id }} - RUN_URL: ${{ github.event.workflow_run.html_url }} - steps: - # Missing when the run was canceled before the eval, or failed to build - # it; there is then nothing to say. - - name: Download the eval's comment - run: | - gh run download "$RUN_ID" --repo "$REPO" --name search-eval-comment \ - --dir "$RUNNER_TEMP/comment" || echo "No comment artifact." - - - name: Post or update the comment - run: | - dir="$RUNNER_TEMP/comment" - if [ ! -f "$dir/state" ]; then - echo "Nothing to do." - exit 0 - fi - # Matched against fixed words and never echoed: printed, a crafted - # value could read as a workflow command. - state=$(head -n 1 "$dir/state") - - # The open PR whose head is the commit CI ran on. Never the - # artifact's word for it: a fork controls what its run uploads. - pr=$(gh pr list --repo "$REPO" --state open --limit 200 \ - --json number,headRefOid \ - --jq ".[] | select(.headRefOid == \"$HEAD_SHA\") | .number" | head -1) - if [ -z "$pr" ]; then - echo "No open PR has head $HEAD_SHA." - exit 0 - fi - - marker='' - existing=$(gh api --paginate "repos/$REPO/issues/$pr/comments" \ - --jq ".[] | select(.user.login == \"github-actions[bot]\" and (.body | startswith(\"$marker\"))) | .id" | head -1) - - body="$RUNNER_TEMP/body.md" - case "$state" in - drop) - { - echo "$marker" - # A zero-width space after each @ keeps a crafted body from - # pinging anyone. - head -c 60000 "$dir/body.md" | sed 's/@/@\xe2\x80\x8b/g' - echo - echo "Every query that changed is in the [job summary]($RUN_URL). Updated for $HEAD_SHA." - } >"$body" - ;; - ok) - if [ -z "$existing" ]; then - echo "No large drop, and no earlier comment to update." - exit 0 - fi - { - echo "$marker" - echo "### \`commands search\` recovered" - echo - echo "No query set ranks far fewer queries first than the base any more, as of $HEAD_SHA. [Job summary]($RUN_URL)." - } >"$body" - ;; - *) - echo "Nothing to compare against." - exit 0 - ;; - esac - - if [ -n "$existing" ]; then - gh api --method PATCH "repos/$REPO/issues/comments/$existing" -F "body=@$body" >/dev/null - echo "Updated comment $existing on #$pr." - else - gh api "repos/$REPO/issues/$pr/comments" -F "body=@$body" >/dev/null - echo "Commented on #$pr." - fi diff --git a/AGENTS.md b/AGENTS.md index 295a9fc..da285f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -133,13 +133,14 @@ summaries, descriptions, help-group lines and argument help, so **editing a command's `about`, an argument's `help` or a line in `DESCRIPTIONS` changes search results**, not only `--help`. The `eval_set_*` tests in `src/commands.rs` run the query sets in `tests/fixtures/commands_search/` and -hold each to a floor of how many queries rank right. Each file's header says -where its queries came from and how much the search was tuned against them. -`cargo test eval_set -- --nocapture` prints the scores and every miss, and -that directory's README explains each set and how to read its score. - -When a change raises a score, raise its floor in the same commit. Lowering one -is a decision to explain in the PR. Don't tune against the sets by +fail when a set ranks more than a few queries fewer right than its recorded +score. Each file's header says where its queries came from and how much the +search was tuned against them. `cargo test eval_set -- --nocapture` prints +the scores and every miss, and that directory's README explains each set and +how to read its score. + +When a change raises a score, record it in the same commit. Dropping past the +slack is a decision to explain in the PR. Don't tune against the sets by special-casing a query; fix the kind of miss. New real queries go in a new file, so there is always one set the search has not been fitted to. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ee600a..73304f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,8 @@ that may never merge. They are not releases and are not listed here. - New `mapbox commands search ""` finds the commands that match a task, best first, and `mapbox commands list` lists every command with its summary. Both are local keyword lookups: no request, no token. `json` - gives `[{command, summary}]`. The query is sent in telemetry as its length + gives `[{command, summary, score}]`, where `score` (0 to 1) tells a full + match from a weak guess. The query is sent in telemetry as its length only. Top-level help now points to `commands search` (agents see it in their hint), and an unknown command with no near spelling suggests it in the error's `fix`. No existing command or output changes. diff --git a/docs/commands.md b/docs/commands.md index b7ca483..1ecb513 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4744,7 +4744,8 @@ Log no longer available: it expired or was removed to keep diagnostic logs Finding a command by what it does. Both read the same command surface `mapbox --schema` describes, so they cover every command this build has, and both are local: no request and no token. `json` gives each command as -`{command, summary}`; `mapbox --schema` has the rest. +`{command, summary}`, plus a `score` from `search`; `mapbox +--schema` has the rest. ### `mapbox commands search` @@ -4764,8 +4765,14 @@ Mapbox". Matches far below the best one are left out. A task no command does usually still comes back with a few weak guesses, so check the summaries before running one. -The query can be quoted or not. No match is not an error: `text` prints -nothing and says so on stderr, `json` prints `[]`, and both exit 0. The +Under `json`, each match has a `score` from 0 to 1 that reads the same for +any query: 1 means the command's name and summary use every word of it, a +few tenths is a guess on a word found elsewhere, and under 0.1 is barely a +match. + +The query can be quoted or not. One with no words at all, such as an +empty string, is a usage error (exit 2). No match is not an error: `text` +prints nothing and says so on stderr, `json` prints `[]`, and both exit 0. The `commands` group itself is left out of the results. Telemetry records the query's length, never its text. @@ -4805,10 +4812,12 @@ mapbox fonts delete Delete a font [ { "command": "mapbox fonts upload", + "score": 1.0, "summary": "Upload a font" }, { "command": "mapbox fonts list", + "score": 0.65, "summary": "List fonts for an owner" }, ... diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py deleted file mode 100644 index 602a6b8..0000000 --- a/scripts/commands-search-report.py +++ /dev/null @@ -1,380 +0,0 @@ -#!/usr/bin/env python3 -"""Markdown summary of `mapbox commands search` eval results, for a CI job -summary. - - commands-search-report.py --head DIR --head-status N - [--base DIR --base-status N] [--sets DIR] - [--comment-dir DIR] - -Each directory holds the per-set reports the `eval_set_*` tests write when -MAPBOX_SEARCH_EVAL_REPORT names it; each status is the exit code of the -`cargo test` run that wrote them. No `--base` means there was no base to run -(not a pull request). `--sets` is the fixture directory, so a set whose -report is missing is named rather than left out. - -With `--comment-dir`, also writes what the PR comment workflow -(.github/workflows/search-eval-comment.yml) needs: `state`, one of `drop` -(some set dropped by LARGE_DROP_QUERIES or LARGE_DROP_POINTS against the -base), `ok` (compared, no large drop) or `none` (nothing to compare), and, -for `drop`, `body.md`. - -The status is what tells apart a base that predates the eval (the run passed -and wrote nothing) from one whose run failed: the reports alone look the -same for both. - -Reports, never gates: the floors in `cargo test` are the gate. So this never -fails the job on bad input: a malformed line is skipped and counted, and an -unreadable report is named. Standard library only, so the job needs no -install step. -""" - -import argparse -import sys -from pathlib import Path - - -class Set: - def __init__(self): - self.floors = None - self.rows = {} - self.invalid = [] - self.skipped = 0 - self.unreadable = None - - -def load(directory): - """Every report in `directory`, by set name. Empty when there is none.""" - sets = {} - if directory is None or not directory.is_dir(): - return sets - for path in sorted(directory.iterdir()): - report = sets[path.name] = Set() - try: - text = path.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError) as error: - report.unreadable = str(error) - continue - for line in text.splitlines(): - parse_line(report, line) - return sets - - -def parse_line(report, line): - columns = line.split("\t") - if columns[0] == "# floors": - try: - report.floors = (int(columns[1]), int(columns[2])) - except (IndexError, ValueError): - report.skipped += 1 - return - if columns[0] == "# invalid": - report.invalid.append(" ".join(columns[1:])) - return - if columns[0].startswith("#") or not line.strip(): - return - if len(columns) != 5 or columns[3] not in ("0", "1") or columns[4] not in ("0", "1"): - report.skipped += 1 - return - query, expected, found, first, top3 = columns - report.rows[query] = { - "expected": " or ".join(expected.split("|")) if expected else "(nothing)", - "found": ", ".join(found.split("|")) if found else "(nothing)", - "first": first == "1", - "top3": top3 == "1", - } - - -def cell(text): - """`text` made safe for one Markdown table cell.""" - return ( - str(text) - .replace("&", "&") - .replace("<", "<") - .replace(">", ">") - .replace("|", "\\|") - .replace("\n", " ") - ) - - -def score(report, key): - return sum(row[key] for row in report.rows.values()) - - -# A set losing this many first-place queries against the base, or this many -# percentage points of its first-place share, gets the PR a comment. Smaller -# trade-offs stay in the job summary: the floors already make each one a -# decision in the diff, and a comment for every one would be noise. -LARGE_DROP_QUERIES = 3 -LARGE_DROP_POINTS = 5 - -# Rows of "no longer first" per set in a comment; the job summary has the rest. -COMMENT_ROWS = 20 - -# Under the table, so a reader need not know the tests to read it. -LEGEND = ( - "**Ranked first**: the expected command was the first result. " - "**In top 3**: it was among the first three. " - "**Required**: the floor in `EVAL_SETS` (src/commands.rs); a lower score fails `cargo test`. " - "A query the CLI cannot serve counts as right only when nothing comes back. " - "How each set was gathered is in tests/fixtures/commands_search/README.md." -) - - -def count(head, total, base): - """`22 (37%)`, with the change against the base when there is one: - `22 (37%, +1)`.""" - share = f"{round(100 * head / total)}%" if total else "-" - change = head - base if base is not None else 0 - return f"{head} ({share}, {change:+d})" if change else f"{head} ({share})" - - -def changed(head_rows, base_rows): - """Queries in both whose first result went from right to wrong or back.""" - fixed, broken = [], [] - for query, row in head_rows.items(): - before = base_rows.get(query) - if before is None or before["first"] == row["first"]: - continue - line = ( - f"| {cell(query)} | {cell(row['expected'])} | " - f"{cell(before['found'])} | {cell(row['found'])} |" - ) - (fixed if row["first"] else broken).append(line) - return fixed, broken - - -def status_note(status): - return "did not finish" if status is None else f"failed (exit {status})" - - -def parse_status(text): - try: - return int(text) - except (TypeError, ValueError): - return None - - -def base_note(base_given, base, base_status): - """Why there is nothing to compare against, or what to know about it.""" - if not base_given: - return "Not a pull request, so there is no base to compare against." - if not base: - if base_status == 0: - return ( - "The base predates the eval (its run had no eval tests), " - "so only this branch's scores are shown." - ) - return ( - f"The base run {status_note(base_status)} before writing " - "any report, so only this branch's scores are shown." - ) - if base_status != 0: - return ( - f"The base run {status_note(base_status)}; " - "its scores are as far as it got." - ) - return None - - -def notes(report, floors_missed): - out = [] - if report.unreadable: - out.append(f"unreadable: {report.unreadable}") - if floors_missed: - out.append("**below its floor**") - if report.invalid: - out.append(f"{len(report.invalid)} invalid rows") - if report.skipped: - out.append(f"{report.skipped} malformed lines skipped") - return out - - -def render(head, head_status, base, base_given, base_status, expected_sets): - out = ["## `commands search` eval", ""] - if not head: - if head_status == 0: - out.append("No reports were written, though the eval run passed: no `eval_set_*` test ran.") - else: - out.append( - f"The eval run on this branch {status_note(head_status)} " - "before writing any report. The build job's test step has the details." - ) - return out - if head_status != 0: - out += [ - f"The eval run on this branch {status_note(head_status)}: a set is " - "below its floor, a row is invalid, or the tests did not build. The build " - "job's test step has the details.", - "", - ] - note = base_note(base_given, base, base_status) - if note: - out += [note, ""] - compare = bool(base) - - out += [ - "| Query set | Queries | Ranked first | In top 3 | Required (first / top 3) | Note |", - "| --- | --- | --- | --- | --- | --- |", - ] - details = [] - names = sorted(set(head) | set(base) | set(expected_sets)) - for name in names: - label = f"`{cell(name)}`" - if name not in head: - where = "only in the base" if name in base else "missing from this run" - out.append(f"| {label} | | | | | {where} |") - continue - report = head[name] - base_report = base.get(name) - first, top3 = score(report, "first"), score(report, "top3") - base_first = score(base_report, "first") if base_report else None - base_top3 = score(base_report, "top3") if base_report else None - floors = report.floors - floor = f"{floors[0]} / {floors[1]}" if floors else "-" - missed = bool(floors) and (first < floors[0] or top3 < floors[1]) - row_notes = notes(report, missed) - if compare and base_report is None: - row_notes.insert(0, "only in this branch") - out.append( - f"| {label} | {len(report.rows)} | {count(first, len(report.rows), base_first)} | " - f"{count(top3, len(report.rows), base_top3)} | {floor} | {cell('; '.join(row_notes))} |" - ) - - if report.invalid: - details += [f"### {label}: rows that could not be scored", ""] - details += [f"- {cell(problem)}" for problem in report.invalid] + [""] - - if base_report: - fixed, broken = changed(report.rows, base_report.rows) - header = ["| Query | Want | Base | This branch |", "| --- | --- | --- | --- |"] - if broken: - details += [f"### {label}: {len(broken)} no longer first", ""] + header + broken + [""] - if fixed: - details += [f"### {label}: {len(fixed)} now first", ""] + header + fixed + [""] - - misses = [ - f"| {cell(query)} | {cell(row['expected'])} | {cell(row['found'])} |" - for query, row in report.rows.items() - if not row["first"] - ] - if misses: - details += [ - f"
{cell(name)}: {len(misses)} not first", - "", - "| Query | Want | Got (top 3) |", - "| --- | --- | --- |", - *misses, - "", - "
", - "", - ] - - return out + ["", LEGEND, "", *details] - - -def share(first, total): - return 100 * first / total if total else 0.0 - - -def large_drops(head, base): - """Each set that dropped far enough against the base to comment on, as - (name, base first, base rows, head first, head rows).""" - drops = [] - for name in sorted(set(head) & set(base)): - report, base_report = head[name], base[name] - first, base_first = score(report, "first"), score(base_report, "first") - rows, base_rows = len(report.rows), len(base_report.rows) - lost = base_first - first - points = share(base_first, base_rows) - share(first, rows) - if lost >= LARGE_DROP_QUERIES or points >= LARGE_DROP_POINTS: - drops.append((name, base_first, base_rows, first, rows)) - return drops - - -def comment(head, head_status, base, base_status): - """The comment state, and its body when there is a large drop.""" - if not head or not base or base_status != 0 or head_status is None: - return "none", None - drops = large_drops(head, base) - if not drops: - return "ok", None - out = [ - "### `commands search` ranks fewer queries first than the base", - "", - "| Query set | Base | This PR | Change |", - "| --- | --- | --- | --- |", - ] - details = [] - for name, base_first, base_rows, first, rows in drops: - points = share(base_first, base_rows) - share(first, rows) - out.append( - f"| `{cell(name)}` | {count(base_first, base_rows, None)} | " - f"{count(first, rows, None)} | {first - base_first:+d} queries, " - f"{-points:+.0f} points |" - ) - _, broken = changed(head[name].rows, base[name].rows) - if broken: - shown = broken[:COMMENT_ROWS] - more = len(broken) - len(shown) - details += [ - f"
{cell(name)}: {len(broken)} no longer first", - "", - "| Query | Want | Base | This PR |", - "| --- | --- | --- | --- |", - *shown, - *([f"\n{more} more in the job summary."] if more else []), - "", - "
", - "", - ] - out += [ - "", - f"A set losing {LARGE_DROP_QUERIES} or more first-place queries, or " - f"{LARGE_DROP_POINTS} or more points, posts this. If the trade-off is " - "intended, say why in the PR; the floors in `EVAL_SETS` decide whether " - "`cargo test` passes.", - "", - *details, - ] - return "drop", "\n".join(out) - - -def write_comment(directory, state, body): - directory.mkdir(parents=True, exist_ok=True) - (directory / "state").write_text(state + "\n", encoding="utf-8") - if body is not None: - (directory / "body.md").write_text(body + "\n", encoding="utf-8") - - -def main(): - parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) - parser.add_argument("--head", type=Path, required=True) - parser.add_argument("--head-status", default="") - parser.add_argument("--base", type=Path) - parser.add_argument("--base-status", default="") - parser.add_argument("--sets", type=Path) - parser.add_argument("--comment-dir", type=Path) - args = parser.parse_args() - - expected_sets = [] - if args.sets is not None and args.sets.is_dir(): - expected_sets = [path.name for path in args.sets.glob("*.tsv")] - head, base = load(args.head), load(args.base) - head_status, base_status = parse_status(args.head_status), parse_status(args.base_status) - lines = render(head, head_status, base, args.base is not None, base_status, expected_sets) - print("\n".join(lines)) - if args.comment_dir is not None: - write_comment(args.comment_dir, *comment(head, head_status, base, base_status)) - - -if __name__ == "__main__": - try: - main() - except SystemExit: - raise - except Exception as error: # noqa: BLE001 - a summary, never a gate - # The job is promised not to fail on this script; say what broke in - # the summary itself rather than leaving it empty. - print("## `commands search` eval\n") - print(f"The summary script failed: `{cell(repr(error))}`. This is a bug in the script.") - sys.exit(0) diff --git a/scripts/test-commands-search-report.sh b/scripts/test-commands-search-report.sh deleted file mode 100755 index a6820d0..0000000 --- a/scripts/test-commands-search-report.sh +++ /dev/null @@ -1,217 +0,0 @@ -#!/bin/sh -# Checks that `scripts/commands-search-report.py` says what happened for each -# way the CI `commands search eval` job can go, from hand-written reports. -# -# The job promises never to fail, and its summary is the only place a -# reviewer sees the result, so a summary that is wrong is worse than none: -# the cases here are the ones it once got wrong or could not tell apart. A -# base whose run failed, a push with no base and a base from before the eval -# all used to read "the base branch has no eval reports"; a malformed line -# raised a traceback and turned the job red. -# -# No Rust and no network: the reports are written here in the format the -# `eval_set_*` tests write. -# -# Usage: -# sh scripts/test-commands-search-report.sh - -set -eu - -SCRIPT="$(dirname "$0")/commands-search-report.py" -PYTHON="${PYTHON:-python3}" - -failures=0 - -say() { - printf '%s\n' "$*" -} - -fail() { - printf 'FAIL: %s\n' "$*" >&2 - failures=$((failures + 1)) -} - -WORK="$(mktemp -d)" -cleanup() { - rm -rf "$WORK" -} -trap cleanup EXIT INT TERM - -# One report: a floors line, then each argument as a row. -report() { - file="$1" - shift - mkdir -p "$(dirname "$file")" - printf '# floors\t1\t1\n' >"$file" - for row in "$@"; do - printf '%s\n' "$row" >>"$file" - done -} - -TAB="$(printf '\t')" -row() { - printf '%s' "$1${TAB}$2${TAB}$3${TAB}$4${TAB}$5" -} - -# Runs the script, which must exit 0, into `$WORK/out`. -summarize() { - if ! "$PYTHON" "$SCRIPT" "$@" >"$WORK/out" 2>"$WORK/err"; then - fail "exited non-zero for: $*: $(cat "$WORK/err")" - return 1 - fi - if [ -s "$WORK/err" ]; then - fail "wrote to stderr for: $*: $(cat "$WORK/err")" - fi -} - -expect() { - case_name="$1" - text="$2" - if grep -qF -- "$text" "$WORK/out"; then - say "ok: $case_name" - else - fail "$case_name: no \"$text\" in:" - sed 's/^/ /' "$WORK/out" >&2 - fi -} - -reject() { - case_name="$1" - text="$2" - if grep -qF -- "$text" "$WORK/out"; then - fail "$case_name: unexpected \"$text\" in:" - sed 's/^/ /' "$WORK/out" >&2 - else - say "ok: $case_name" - fi -} - -SETS="$WORK/sets" -mkdir -p "$SETS" -: >"$SETS/a.tsv" -: >"$SETS/b.tsv" - -HEAD="$WORK/head" -report "$HEAD/a.tsv" \ - "$(row 'upload a font' 'mapbox fonts upload' 'mapbox fonts upload' 1 1)" \ - "$(row 'a | b query' 'mapbox x|mapbox y' 'mapbox z' 0 0)" -report "$HEAD/b.tsv" \ - "$(row 'list styles' 'mapbox styles list' 'mapbox styles list' 1 1)" - -BASE="$WORK/base" -report "$BASE/a.tsv" \ - "$(row 'upload a font' 'mapbox fonts upload' 'mapbox fonts list' 0 1)" \ - "$(row 'a | b query' 'mapbox x|mapbox y' 'mapbox z' 0 0)" - -if summarize --head "$HEAD" --head-status 0 --base "$BASE" --base-status 0 --sets "$SETS"; then - expect "a fix against the base is listed" "1 now first" - expect "a set new in this branch is marked" "only in this branch" - expect "a pipe in a cell is escaped" 'a \| b query' - expect "a score carries its share and its change" "| 1 (50%, +1) |" - expect "an unchanged score carries its share alone" "| 1 (50%) |" - expect "the columns are named" "| Query set | Queries | Ranked first | In top 3 |" - expect "the legend says what the columns mean" "**Ranked first**: the expected command" - reject "no emoji" "⚠" -fi - -if summarize --head "$HEAD" --head-status 0 --sets "$SETS"; then - expect "a push has no base" "Not a pull request" -fi - -if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 0 --sets "$SETS"; then - expect "a base from before the eval" "The base predates the eval" -fi - -if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 101 --sets "$SETS"; then - expect "a base whose run failed" "The base run failed (exit 101)" -fi - -if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status '' --sets "$SETS"; then - expect "a base whose step never recorded a status" "The base run did not finish" -fi - -ONLY_A="$WORK/only-a" -report "$ONLY_A/a.tsv" "$(row 'q' 'mapbox x' 'mapbox x' 1 1)" -if summarize --head "$ONLY_A" --head-status 101 --base "$BASE" --base-status 0 --sets "$SETS"; then - expect "a set missing from this run is named" "missing from this run" - expect "a failing head run says so" "failed (exit 101)" -fi - -if summarize --head "$ONLY_A" --head-status 0 --base "$HEAD" --base-status 0; then - expect "a set only in the base is marked" "only in the base" -fi - -if summarize --head "$WORK/none" --head-status 101; then - expect "no reports and a failed run" "before writing any report" -fi - -BAD="$WORK/bad" -report "$BAD/a.tsv" \ - "$(row 'fine' 'mapbox x' 'mapbox x' 1 1)" \ - "too${TAB}few${TAB}columns" \ - "$(row 'odd flags' 'mapbox x' 'mapbox x' yes 1)" -printf '# floors\tnot-a-number\n' >>"$BAD/a.tsv" -printf '# invalid\tmapbox gone is not a command search answers with ("q")\n' >>"$BAD/a.tsv" -printf '\377\376 not utf-8\n' >"$BAD/b.tsv" -if summarize --head "$BAD" --head-status 101 --sets "$SETS"; then - expect "malformed lines are skipped and counted" "3 malformed lines skipped" - expect "an invalid row is listed" "mapbox gone" - expect "an unreadable report is named" "unreadable" -fi - -# The PR comment: `drop` only past the threshold, `ok` when compared and -# below it, `none` when there was nothing to compare against. -# Four rows each; a "won" row ranks first, a "lost" one does not. -won() { row "$1" 'mapbox x' 'mapbox x' 1 1; } -lost() { row "$1" 'mapbox x' 'mapbox y' 0 0; } -report "$WORK/cbase/a.tsv" "$(won q1)" "$(won q2)" "$(won q3)" "$(won q4)" -report "$WORK/cdrop/a.tsv" "$(lost q1)" "$(lost q2)" "$(lost q3)" "$(won q4)" -report "$WORK/cslip/a.tsv" "$(won q1)" "$(won q2)" "$(won q3)" "$(won q4)" - -check_state() { - case_name="$1" - want="$2" - got="$(cat "$WORK/comment/state" 2>/dev/null || echo missing)" - if [ "$got" = "$want" ]; then - say "ok: $case_name" - else - fail "$case_name: state is $got, not $want" - fi -} - -if summarize --head "$WORK/cdrop" --head-status 0 --base "$WORK/cbase" --base-status 0 \ - --comment-dir "$WORK/comment"; then - check_state "three queries lost is a drop" drop - cp "$WORK/comment/body.md" "$WORK/out" - expect "the comment names the set and the change" "| \`a.tsv\` | 4 (100%) | 1 (25%) | -3 queries, -75 points |" - expect "the comment lists what is no longer first" "3 no longer first" -fi -rm -rf "$WORK/comment" - -if summarize --head "$WORK/cslip" --head-status 0 --base "$WORK/cbase" --base-status 0 \ - --comment-dir "$WORK/comment"; then - check_state "no loss is ok" ok - if [ -e "$WORK/comment/body.md" ]; then - fail "an ok state wrote a comment body" - else - say "ok: an ok state writes no body" - fi -fi -rm -rf "$WORK/comment" - -if summarize --head "$WORK/cdrop" --head-status 0 --base "$WORK/cbase" --base-status 101 \ - --comment-dir "$WORK/comment"; then - check_state "a failed base run is nothing to compare" none -fi -rm -rf "$WORK/comment" - -if summarize --head "$WORK/cdrop" --head-status 0 --comment-dir "$WORK/comment"; then - check_state "no base is nothing to compare" none -fi - -if [ "$failures" -ne 0 ]; then - say "$failures check(s) failed." - exit 1 -fi - -say "All checks passed." diff --git a/src/commands.rs b/src/commands.rs index 06b28f3..6102a56 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -70,6 +70,14 @@ const PREFIX: f64 = 0.7; const STEM: f64 = 0.5; const TYPO: f64 = 0.4; +/// What each word of the query scores when both the command's name and its +/// summary use it. The score a caller sees is the total against this, capped +/// at 1, so it reads the same for a query of any length: 1 is a command that +/// says every word in its name and summary, a few tenths is a guess on a +/// word found elsewhere ("create a token" finds `feedback create` at 0.35), +/// and under 0.1 is barely a match at all. +const STRONG_TERM: f64 = NAME_WEIGHT + SUMMARY_WEIGHT; + /// A match this far below the best one is noise beside it. /// /// Relative only. A fixed minimum also emptied the answer for tasks this CLI @@ -137,6 +145,15 @@ pub fn command() -> Command { Arg::new("query") .required(true) .num_args(1..) + // Catches a script passing an empty variable, which + // would otherwise look like a query that found nothing. + .value_parser(|value: &str| { + if value.chars().any(char::is_alphanumeric) { + Ok(value.to_string()) + } else { + Err("has no words to search for") + } + }) .help("What you want to do, in a few words: \"upload a font\""), ) .arg( @@ -173,25 +190,41 @@ pub fn search( "No command matches \"{query}\". `mapbox commands list` shows every command." )); } - emit(mode, &found) + let json = found + .iter() + .map(|(score, entry)| { + json!({ + "command": entry.command, + "summary": entry.summary, + // Never 0: everything returned matched something. + "score": ((score * 100.0).round() / 100.0).max(0.01), + }) + }) + .collect(); + let entries: Vec<&CommandEntry> = found.iter().map(|(_, entry)| entry).collect(); + emit(mode, &entries, json) } pub fn list(app: &Command, specs: &[ServiceSpec], mode: Mode) -> Result<()> { let mut all = schema::build(app, specs, &[]).commands; all.sort_by(|a, b| a.command.cmp(&b.command)); - emit(mode, &all) + let json = all + .iter() + .map(|entry| json!({ "command": entry.command, "summary": entry.summary })) + .collect(); + emit(mode, &all.iter().collect::>(), json) } /// The commands that match `query`, best first, down to -/// [`MIN_SHARE_OF_BEST`]. -fn rank(app: &Command, specs: &[ServiceSpec], query: &str) -> Vec { +/// [`MIN_SHARE_OF_BEST`], each with its score against [`STRONG_TERM`]. +fn rank(app: &Command, specs: &[ServiceSpec], query: &str) -> Vec<(f64, CommandEntry)> { let index = index(app, specs); let order = rank_in(&index, query); let mut entries: Vec> = index.into_iter().map(|(_, entry)| Some(entry)).collect(); order .into_iter() - .map(|i| entries[i].take().expect("each position once")) + .map(|(score, i)| (score, entries[i].take().expect("each position once"))) .collect() } @@ -208,10 +241,10 @@ fn index(app: &Command, specs: &[ServiceSpec]) -> Vec<(Fields, CommandEntry)> { .collect() } -/// Positions in `index` of the commands that match `query`, best first. -/// Ties go to the shorter command name, then alphabetically, so the same -/// query always answers the same way. -fn rank_in(index: &[(Fields, CommandEntry)], query: &str) -> Vec { +/// Positions in `index` of the commands that match `query`, best first, each +/// with its score against [`STRONG_TERM`]. Ties go to the shorter command name, then +/// alphabetically, so the same query always answers the same way. +fn rank_in(index: &[(Fields, CommandEntry)], query: &str) -> Vec<(f64, usize)> { let vocabulary: BTreeSet<&str> = index .iter() .flat_map(|(fields, _)| fields.parts.iter().flat_map(|(_, words)| words)) @@ -233,7 +266,11 @@ fn rank_in(index: &[(Fields, CommandEntry)], query: &str) -> Vec { .then(a.len().cmp(&b.len())) .then(a.cmp(b)) }); - above_floor(scored).into_iter().map(|(_, i)| i).collect() + let strong = query.terms.len() as f64 * STRONG_TERM; + above_floor(scored) + .into_iter() + .map(|(score, i)| ((score / strong).min(1.0), i)) + .collect() } /// What is left of `scored`, already sorted best first, once the zero scores @@ -523,7 +560,7 @@ fn singular(word: String) -> String { } } -fn emit(mode: Mode, entries: &[CommandEntry]) -> Result<()> { +fn emit(mode: Mode, entries: &[&CommandEntry], json: Vec) -> Result<()> { // No table, not an empty line: `text` with no match prints nothing, so // `mapbox commands search … > file` leaves an empty file. `json` still // answers `[]`, the one shape a parser can take. @@ -544,10 +581,6 @@ fn emit(mode: Mode, entries: &[CommandEntry]) -> Result<()> { ) .collect::>() .join("\n"); - let json = entries - .iter() - .map(|entry| json!({ "command": entry.command, "summary": entry.summary })) - .collect(); output::emit(mode, &text, Value::Array(json))?; if !mode.is_json() { output::print_tips(&["`mapbox --help` shows what one takes.".to_string()]); @@ -565,20 +598,20 @@ mod tests { rank(&app, &specs, query) .into_iter() .take(3) - .map(|entry| entry.command) + .map(|(_, entry)| entry.command) .collect() } - /// A query set under `tests/fixtures/commands_search/`, and the fewest - /// rows each must keep answering right: first result, and anywhere in - /// the first three. A `-` row is right when nothing comes back. + /// A query set under `tests/fixtures/commands_search/`, and how many + /// rows it answered right when last recorded: first result, and anywhere + /// in the first three. A `-` row is right when nothing comes back. /// - /// The floors are the scores when they were last raised. A change that - /// improves one raises its floor in the same commit, so the gain cannot - /// quietly be given back; one that lowers a floor says why in its PR. - /// New real queries go in a new file rather than into these: a set the - /// search has been tuned against stops saying how it does on the next - /// query nobody has seen. + /// A set fails only when it drops more than [`SLACK`] below these. A + /// change that improves one records the new score in the same commit, so + /// the gain cannot quietly be given back; one that drops past the slack + /// lowers it and says why in its PR. New real queries go in a new file + /// rather than into these: a set the search has been tuned against stops + /// saying how it does on the next query nobody has seen. const EVAL_SETS: &[(&str, &str, usize, usize)] = &[ ( "tuning.tsv", @@ -600,20 +633,17 @@ mod tests { ), ]; - /// Where [`eval_set_keeps_its_scores`] writes its reports, when set. - const REPORT_DIR_ENV: &str = "MAPBOX_SEARCH_EVAL_REPORT"; + /// How many queries a set may drop below its recorded score before it + /// fails. Without it, any change that moves one query by one place fails + /// the build, including a spec regeneration that rewords a summary. The + /// targeted tests below are the hard gate for specific queries. + const SLACK: usize = 3; - /// Holds one set to its floors. `cargo test eval_set -- --nocapture` - /// prints the scores, and each query that did not rank first. One test - /// per set, so they run in parallel. - /// - /// With [`REPORT_DIR_ENV`] set, also writes every query's result to - /// `/`, before anything is asserted, so a failing run still - /// leaves one: a set missing from CI's summary would read as a set - /// nobody added. A row that cannot be scored goes in the report as a - /// `# invalid` line rather than stopping the run there. + /// Holds one set to its recorded scores, less [`SLACK`]. + /// `cargo test eval_set -- --nocapture` prints the scores, and each query + /// that did not rank first. One test per set, so they run in parallel. fn eval_set_keeps_its_scores(wanted: &str) { - let &(name, text, min_first, min_top3) = EVAL_SETS + let &(name, text, recorded_first, recorded_top3) = EVAL_SETS .iter() .find(|(name, ..)| *name == wanted) .unwrap_or_else(|| panic!("`{wanted}` is not in EVAL_SETS")); @@ -629,7 +659,6 @@ mod tests { let (mut rows, mut first, mut top3) = (0, 0, 0); let (mut misses, mut invalid) = (vec![], vec![]); - let mut report = format!("# floors\t{min_first}\t{min_top3}\n"); for line in text .lines() .filter(|l| !l.starts_with('#') && !l.trim().is_empty()) @@ -654,7 +683,7 @@ mod tests { let found: Vec = rank_in(&index, query) .into_iter() .take(3) - .map(|i| index[i].1.command.clone()) + .map(|(_, i)| index[i].1.command.clone()) .collect(); let (is_first, in_top3) = if expected.is_empty() { (found.is_empty(), found.is_empty()) @@ -666,13 +695,6 @@ mod tests { found.iter().any(|c| expected.contains(&c.as_str())), ) }; - report.push_str(&format!( - "{query}\t{}\t{}\t{}\t{}\n", - expected.join("|"), - found.join("|"), - u8::from(is_first), - u8::from(in_top3) - )); rows += 1; first += usize::from(is_first); top3 += usize::from(in_top3); @@ -680,34 +702,28 @@ mod tests { misses.push(format!(" \"{query}\": want {expected:?}, got {found:?}")); } } - for problem in &invalid { - report.push_str(&format!("# invalid\t{problem}\n")); - } - if let Some(dir) = std::env::var_os(REPORT_DIR_ENV) { - let dir = std::path::PathBuf::from(dir); - std::fs::create_dir_all(&dir).expect("create the report directory"); - std::fs::write(dir.join(name), report).expect("write the report"); - } - eprintln!( - "{name}: {first}/{rows} first (floor {min_first}), \ - {top3}/{rows} in top 3 (floor {min_top3})" + "{name}: {first}/{rows} first (recorded {recorded_first}), \ + {top3}/{rows} in top 3 (recorded {recorded_top3})" ); for miss in &misses { eprintln!("{miss}"); } assert!(invalid.is_empty(), "{name}:\n{}", invalid.join("\n")); assert!( - first >= min_first && top3 >= min_top3, - "{name} fell below its floor: {first} first (floor {min_first}), \ - {top3} in top 3 (floor {min_top3}). If the trade-off is intended — \ - help text made accurate at the cost of a match, say — lower the floor \ - in EVAL_SETS to {}/{} and say why in the PR. The misses are above.", - first.min(min_first), - top3.min(min_top3), + first + SLACK >= recorded_first && top3 + SLACK >= recorded_top3, + "{name} dropped more than {SLACK} below its recorded score: {first} first \ + (recorded {recorded_first}), {top3} in top 3 (recorded {recorded_top3}). \ + If the trade-off is intended — help text made accurate at the cost of a \ + match, say — record {first}/{top3} in EVAL_SETS and say why in the PR. \ + The misses are above.", ); - if first > min_first || top3 > min_top3 { - eprintln!(" above its floor: raise it to {first}/{top3} in EVAL_SETS"); + if first > recorded_first || top3 > recorded_top3 { + eprintln!( + " above its recorded score: record {}/{} in EVAL_SETS", + first.max(recorded_first), + top3.max(recorded_top3) + ); } } @@ -879,9 +895,12 @@ mod tests { let app = crate::build_app(&specs); let found = rank(&app, &specs, "find a command search list commands"); assert!( - found.iter().all(|entry| entry.service != COMMAND), + found.iter().all(|(_, entry)| entry.service != COMMAND), "{:?}", - found.iter().map(|entry| &entry.command).collect::>() + found + .iter() + .map(|(_, entry)| &entry.command) + .collect::>() ); } diff --git a/src/config.rs b/src/config.rs index cfd159a..dfd9c97 100644 --- a/src/config.rs +++ b/src/config.rs @@ -182,7 +182,7 @@ pub fn command() -> Command { }; Command::new(COMMAND) - .about("Get or set settings: telemetry, update checks, history, logs") + .about(format!("Get or set settings: {}", KEYS.join(", "))) .long_about( "Get or set a mapbox setting that persists across shells and sessions, \ written to a file beside the stored credentials rather than an \ diff --git a/src/mcp.rs b/src/mcp.rs index eb21565..5b22d92 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -329,6 +329,15 @@ fn spawn_live(client: &Client, args: &[&str]) -> io::Result<(ExitStatus, String) Err(last_err.unwrap_or_else(|| io::Error::from(io::ErrorKind::NotFound))) } +/// "A, B or C", for help text built from [`SERVERS`] and [`CLIENTS`]. +fn joined(items: impl Iterator, last: &str) -> String { + let items: Vec<&str> = items.collect(); + match items.split_last() { + Some((tail, init)) if !init.is_empty() => format!("{} {last} {tail}", init.join(", ")), + _ => items.concat(), + } +} + pub fn command() -> ClapCommand { let server_flags: Vec<&'static str> = SERVERS.iter().map(|s| s.flag).collect(); let client_flags: Vec<&'static str> = CLIENTS.iter().map(|c| c.flag).collect(); @@ -354,11 +363,12 @@ pub fn command() -> ClapCommand { .subcommand( ClapCommand::new(INSTALL) .about("Register a server with a client. With neither flag, every known server for every detected client") - .long_about( - "Register the Mapbox MCP and Mapbox DevKit MCP servers with a coding agent: \ - Claude Code, Codex, VS Code or Cursor. With neither flag, registers every known \ - server with every detected client.", - ) + .long_about(format!( + "Register the {} servers with a coding agent: {}. With neither flag, registers \ + every known server with every detected client.", + joined(SERVERS.iter().map(|s| s.label), "and"), + joined(CLIENTS.iter().map(|c| c.label), "or"), + )) .arg( Arg::new(SERVER_ARG) .long(SERVER_ARG) diff --git a/tests/fixtures/commands_search/README.md b/tests/fixtures/commands_search/README.md index 6e5dbb9..6ffea84 100644 --- a/tests/fixtures/commands_search/README.md +++ b/tests/fixtures/commands_search/README.md @@ -2,17 +2,10 @@ Queries that `mapbox commands search` is scored against. The `eval_set_*` tests in `src/commands.rs` run every file here and fail when a set ranks -fewer queries right than its floor. `cargo test eval_set -- --nocapture` -prints each set's score and every query that did not rank first. On a PR, -CI's `commands search eval` job puts the scores in its job summary, with the -queries the change fixed and broke against the base branch. It also says -when there was nothing to compare against, and why: a push with no base, a -base from before these sets, or a base run that failed. -When a set ranks 3 or more fewer queries first than the base, or loses 5 or -more points, `.github/workflows/search-eval-comment.yml` also comments on -the PR, and updates that comment once a later push recovers. -`scripts/test-commands-search-report.sh` checks the summary and the -comment's content against hand-written reports. +more than `SLACK` (3) queries fewer right than its recorded score, so a +reworded summary that moves one query does not fail the build. +`cargo test eval_set -- --nocapture` prints each set's score and every +query that did not rank first. ## Format @@ -90,8 +83,8 @@ Kinds: ## Changing them -- Raise a set's floor in `EVAL_SETS` when a change improves it, in the same - commit. Lowering one is a decision to explain in the PR. +- Record a set's new score in `EVAL_SETS` when a change improves it, in the + same commit. Dropping past the slack is a decision to explain in the PR. - Fix the kind of miss rather than special-casing a query. - Put new real queries, such as ones the team actually typed, in a new file rather than these, so there is always a set the search has not been diff --git a/tests/output_contract.rs b/tests/output_contract.rs index d220bcf..fc1801f 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1301,9 +1301,9 @@ fn text_into_a_pipe_carries_no_color_and_no_banner() { } } -/// `commands search` answers on stdout alone under `json`: the matches as -/// `[{command, summary}]`, best first, with nothing on stderr for a caller -/// to have to tell apart from a warning. +/// `commands search` answers on stdout under `json`: the matches as +/// `[{command, summary, score}]`, best first. A match leaves stderr empty; +/// no match adds a hint there (below). #[test] fn commands_search_prints_its_matches_as_json() { let out = run(&["-o", "json", "commands", "search", "upload", "a", "font"]); @@ -1315,7 +1315,28 @@ fn commands_search_prints_its_matches_as_json() { assert!(matches.len() <= 5, "{matches:?}"); for entry in matches { let keys: Vec<&String> = entry.as_object().expect("an object").keys().collect(); - assert_eq!(keys, ["command", "summary"], "{entry}"); + assert_eq!(keys, ["command", "score", "summary"], "{entry}"); + } + let scores: Vec = matches + .iter() + .map(|entry| entry["score"].as_f64().expect("a number")) + .collect(); + assert_eq!(scores[0], 1.0, "{scores:?}"); + assert!( + scores.windows(2).all(|pair| pair[0] >= pair[1]) && scores.iter().all(|s| *s > 0.0), + "{scores:?}" + ); +} + +/// An empty query is most likely a script's empty variable, so it fails as +/// a usage error rather than answering like a query that found nothing. +#[test] +fn commands_search_refuses_a_query_with_no_words() { + for query in ["", " ", "?!"] { + let out = run(&["-o", "json", "commands", "search", query]); + assert_eq!(out.status.code(), Some(2), "{query:?}: {}", stderr(&out)); + assert_eq!(json(&stderr(&out))["code"], "usage", "{query:?}"); + assert_eq!(stdout(&out), "", "{query:?}"); } } From 183bab67c324225f94abe5e40647d22a5848828b Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Wed, 21 Oct 2026 11:44:51 +0300 Subject: [PATCH 09/10] Separate the commands search changelog entry from the next one --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73304f9..45631c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ that may never merge. They are not releases and are not listed here. only. Top-level help now points to `commands search` (agents see it in their hint), and an unknown command with no near spelling suggests it in the error's `fix`. No existing command or output changes. + - `mapbox usage` reads as a chart: one row per product with no blank lines between, every sparkline as wide as the period, bars scaled from zero so their height is proportional to the value, and the tallest From a29edf3adc858679e3bd952b4c2526feef696816 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Wed, 21 Oct 2026 19:56:09 +0300 Subject: [PATCH 10/10] Check commands search's query as a whole and describe its score plainly An empty argument beside real words is no longer a usage error; only a query with no words overall is. The score is documented as how much of the query a command's text covers, not as a confidence. --- CHANGELOG.md | 4 ++-- docs/commands.md | 9 ++++---- src/commands.rs | 44 ++++++++++++++++++++++++++++------------ src/main.rs | 5 +++++ tests/output_contract.rs | 11 ++++++++++ 5 files changed, 54 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45631c9..bc64f5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,8 +53,8 @@ that may never merge. They are not releases and are not listed here. - New `mapbox commands search ""` finds the commands that match a task, best first, and `mapbox commands list` lists every command with its summary. Both are local keyword lookups: no request, no token. `json` - gives `[{command, summary, score}]`, where `score` (0 to 1) tells a full - match from a weak guess. The query is sent in telemetry as its length + gives `[{command, summary, score}]`, where `score` (0 to 1) is how much + of the query the command's own text covers. The query is sent in telemetry as its length only. Top-level help now points to `commands search` (agents see it in their hint), and an unknown command with no near spelling suggests it in the error's `fix`. No existing command or output changes. diff --git a/docs/commands.md b/docs/commands.md index 1ecb513..e6958ac 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4765,10 +4765,11 @@ Mapbox". Matches far below the best one are left out. A task no command does usually still comes back with a few weak guesses, so check the summaries before running one. -Under `json`, each match has a `score` from 0 to 1 that reads the same for -any query: 1 means the command's name and summary use every word of it, a -few tenths is a guess on a word found elsewhere, and under 0.1 is barely a -match. +Under `json`, each match has a `score` from 0 to 1: how much of the query +the command's own text covers. 1 means its name and summary use every word +of the query, a few tenths is a word found elsewhere, and under 0.1 is +barely a match. It is not a confidence, so don't use it as a cutoff: the +right command can score lower than a wrong one on another query. The query can be quoted or not. One with no words at all, such as an empty string, is a usage error (exit 2). No match is not an error: `text` diff --git a/src/commands.rs b/src/commands.rs index 6102a56..2b3d4a3 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -72,10 +72,12 @@ const TYPO: f64 = 0.4; /// What each word of the query scores when both the command's name and its /// summary use it. The score a caller sees is the total against this, capped -/// at 1, so it reads the same for a query of any length: 1 is a command that -/// says every word in its name and summary, a few tenths is a guess on a -/// word found elsewhere ("create a token" finds `feedback create` at 0.35), -/// and under 0.1 is barely a match at all. +/// at 1, so it measures how much of the query a command's own text covers, +/// for a query of any length: 1 is a command that says every word in its +/// name and summary, a few tenths is a word found elsewhere, and under 0.1 is +/// barely a match at all. It is not a confidence: "make a map image" puts the +/// right `static get-image` at 0.27, "create a token" the wrong `feedback +/// create` at 0.35. const STRONG_TERM: f64 = NAME_WEIGHT + SUMMARY_WEIGHT; /// A match this far below the best one is noise beside it. @@ -145,15 +147,6 @@ pub fn command() -> Command { Arg::new("query") .required(true) .num_args(1..) - // Catches a script passing an empty variable, which - // would otherwise look like a query that found nothing. - .value_parser(|value: &str| { - if value.chars().any(char::is_alphanumeric) { - Ok(value.to_string()) - } else { - Err("has no words to search for") - } - }) .help("What you want to do, in a few words: \"upload a font\""), ) .arg( @@ -167,6 +160,31 @@ pub fn command() -> Command { .subcommand(Command::new("list").about("List every command with its summary")) } +/// Refuses a query with no words in it as a usage error, so a script +/// passing an empty variable fails rather than looking like a query that +/// found nothing. Checked on the joined query, not per argument: +/// `search font "$EMPTY"` still has a word to search for. +pub fn check_query(matches: &ArgMatches) -> std::result::Result<(), clap::Error> { + let Some(("search", search)) = matches + .subcommand_matches(COMMAND) + .and_then(ArgMatches::subcommand) + else { + return Ok(()); + }; + let has_words = search + .get_many::("query") + .expect("required") + .any(|part| part.chars().any(char::is_alphanumeric)); + if has_words { + Ok(()) + } else { + Err(clap::Error::raw( + clap::error::ErrorKind::InvalidValue, + "the query has no words to search for\n", + )) + } +} + pub fn search( app: &Command, specs: &[ServiceSpec], diff --git a/src/main.rs b/src/main.rs index 4a8173d..578a8d0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -949,6 +949,11 @@ fn cli() -> u8 { }, }; + if let Err(e) = commands::check_query(&matches) { + run_record::set_unparsed(&app, &raw_argv, e.kind()); + return report_parse_result(e, &raw_argv); + } + // Before anything resolves a token: describing a command makes no // request, and asking for a login first would be a poor way to answer a // question about what a login is for. diff --git a/tests/output_contract.rs b/tests/output_contract.rs index fc1801f..8f25d72 100644 --- a/tests/output_contract.rs +++ b/tests/output_contract.rs @@ -1340,6 +1340,17 @@ fn commands_search_refuses_a_query_with_no_words() { } } +/// The check is on the whole query: an empty variable beside real words +/// still leaves something to search for. +#[test] +fn commands_search_accepts_an_empty_part_beside_words() { + let out = run(&[ + "-o", "json", "commands", "search", "upload", "a", "font", "", "?", + ]); + assert!(out.status.success(), "{}", stderr(&out)); + assert_eq!(json(&stdout(&out))[0]["command"], "mapbox fonts upload"); +} + /// A person gets a table on stdout and the next step on stderr, so /// `> file` keeps the table alone. #[test]