From a729ebdea7ed93ce582ec605e4f1def6370e08ca Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 16 Sep 2026 00:20:30 -0400 Subject: [PATCH 1/5] Stop walking agents into the one command they cannot run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of these come from a real session: someone pointed a coding agent at this CLI and it stumbled five times. Three of the five were this CLI behaving exactly as designed and saying so clearly. These are the two that were not. **The skill we ship for agents recommended `auth login`.** Its Authentication section listed the three token sources in precedence order with no comment on which an agent can use, so an agent with no token reads "credentials stored by `mapbox auth login`" as the third option and tries it. That command opens a browser and waits for a person; in an agent session it can only fail. We wrote a skill *for agents* that walks them into the one command agents cannot run. It now says so on the entry itself, and adds a line after the list: have `MAPBOX_ACCESS_TOKEN` set, ask the person you are working for if it is missing, and do not reach for `auth login` when a command reports no token. The error `auth login` gives was already right — `interactive_required`, with "Set MAPBOX_ACCESS_TOKEN for a script or a CI job" and a link to where tokens come from. Nothing about it changes. Reaching it at all is the waste. **And a blocked config directory now says when it holds a credential.** A plain file at `~/.mapbox` is what older Mapbox tooling left, and it is a one-line access token. The message named the file and gave the `mv`, which is the right fix, but nothing told the reader whether they were moving junk or a working token — so the honest reading of "move it aside" is that something is being thrown away. When the file looks like a token, the message now adds that it still works and how to keep using it: export MAPBOX_ACCESS_TOKEN="$(cat ~/.mapbox.bak)" Naming `.bak` matters: it is the file as it exists *after* the `mv` directly above it, and a test holds that, because two steps that contradict each other are worse than one. The check is deliberately shallow — a bounded read, a `pk.`/`sk.`/`tk.` prefix, no whitespace — and any read error answers "no". It runs while reporting a different problem and must not replace it. **The token is never printed**, and that is the assertion worth having: an error message reaches every terminal and log that the file's 0600 permissions were keeping it out of. Verified by planting a leak and watching the test fail, then restoring it. 598 tests, fmt and clippy clean. --- CHANGELOG.md | 18 +++++++ src/auth.rs | 103 ++++++++++++++++++++++++++++++++++++++++- src/generate_skills.rs | 13 +++++- 3 files changed, 131 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bcbbd7..6d75113 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,24 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +### Changed + +- The skill `mapbox generate-skills` writes now tells an agent which of the + three ways to authenticate it can actually use. It listed all three without + comment, so an agent with no token would read "credentials stored by + `mapbox auth login`" as an option and try it — a command that opens a + browser and waits for a person, which in a coding-agent session can only + fail. It now says to have `MAPBOX_ACCESS_TOKEN` set, and to ask for one + rather than reaching for `auth login`. Reported from a real session that hit + exactly this. + +- A file sitting where the credential directory belongs now says so when it + looks like an access token, which the one older Mapbox tools left at + `~/.mapbox` does. The advice was to move it aside, with nothing to tell the + reader whether they were moving junk or a working credential; it now says + the token still works and gives the `MAPBOX_ACCESS_TOKEN` line that keeps + using it. The token itself is never printed, and a test holds that. + ## 0.2.2 - 2026-09-15 ### Changed diff --git a/src/auth.rs b/src/auth.rs index fd577b7..4793880 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -246,6 +246,42 @@ pub(crate) fn config_dir_path() -> Option { #[derive(Debug)] struct DirectoryBlocked { path: PathBuf, + /// Whether the file looks like the token older Mapbox tooling left here. + holds_a_token: bool, +} + +/// Does this file look like the one-line token file older tooling wrote? +/// +/// Worth answering because the answer changes the advice. "Move it aside" is +/// the fix either way, but a reader who does not know what the file *is* +/// cannot tell whether moving it loses something — and in this case it holds +/// a working credential they can keep using in one line. +/// +/// Deliberately shallow. The read is bounded, because nothing guarantees the +/// thing in the way is small, and a wrong answer here costs a sentence of +/// advice rather than a failed command. Any read error is a `false`: this +/// runs while reporting a different problem and must not replace it. +fn looks_like_a_legacy_token_file(path: &Path) -> bool { + use std::io::Read; + + // Longer than any token Mapbox issues, short enough that a stray file + // is not worth reading. + const MOST: usize = 512; + + let Ok(mut file) = std::fs::File::open(path) else { + return false; + }; + let mut buffer = vec![0; MOST]; + let Ok(read) = file.read(&mut buffer) else { + return false; + }; + let Ok(text) = std::str::from_utf8(&buffer[..read]) else { + return false; + }; + + let token = text.trim(); + ["pk.", "sk.", "tk."].iter().any(|p| token.starts_with(p)) + && !token.contains(char::is_whitespace) } impl DirectoryBlocked { @@ -273,7 +309,20 @@ impl std::fmt::Display for DirectoryBlocked { Move it aside to continue:\n\n \ mv {shown} {shown}.bak\n\n\ Or set {CONFIG_DIR_ENV} to keep credentials somewhere else entirely." - ) + )?; + // Only once the reader knows the fix. The token is never printed: it + // is a live credential, and this text reaches logs and terminals that + // the file's permissions were protecting it from. + if self.holds_a_token { + write!( + f, + "\n\nIt holds what looks like an access token, left by older Mapbox \ + tooling. Nothing is lost by moving it — the token still works, and \ + `{CLAP_TOKEN_ENV}` is how to keep using it:\n\n \ + export {CLAP_TOKEN_ENV}=\"$(cat {shown}.bak)\"" + )?; + } + Ok(()) } } @@ -291,6 +340,7 @@ fn prepare_config_dir(dir: &Path) -> Result<()> { if dir.exists() && !dir.is_dir() { return Err(DirectoryBlocked { path: dir.to_path_buf(), + holds_a_token: looks_like_a_legacy_token_file(dir), } .into()); } @@ -2630,6 +2680,57 @@ mod tests { ); } + /// The obstruction usually *is* a credential, and saying so changes what + /// the reader does about it. + /// + /// Reported from a real session: someone found `~/.mapbox` in the way, + /// was told to move it aside, and did — with no way to know from the + /// message that the file held a working token rather than junk. + #[test] + fn a_legacy_token_file_says_the_token_is_not_lost() { + const TOKEN: &str = "sk.eyJ1IjoiZmFrZSJ9.not-a-real-token"; + + let path = scratch("config-dir-legacy-token").join(".mapbox"); + std::fs::write(&path, format!("{TOKEN}\n")).unwrap(); + + let full = prepare_config_dir(&path).unwrap_err().to_string(); + + assert!( + full.contains(CLAP_TOKEN_ENV), + "the way to keep using it belongs in the message: {full}" + ); + // The whole point of the `.bak` suffix here: the export has to name + // the file as it will be *after* the `mv` above it, or the reader + // follows two steps that contradict each other. + assert!( + full.contains(&format!("{}.bak", path.display())), + "the export has to name the moved file, not the original: {full}" + ); + + // The one thing this must never do. A live credential in an error + // message reaches every terminal and log the file's 0600 permissions + // were keeping it out of. + assert!( + !full.contains(TOKEN), + "the token itself must never be printed: {full}" + ); + } + + /// And the sentence is earned rather than always shown. + #[test] + fn a_file_that_is_not_a_token_gets_no_token_advice() { + let path = scratch("config-dir-not-a-token").join(".mapbox"); + std::fs::write(&path, "[profile default]\nsomething = else\n").unwrap(); + + let full = prepare_config_dir(&path).unwrap_err().to_string(); + + assert!(full.contains("is a file"), "{full}"); + assert!( + !full.contains(CLAP_TOKEN_ENV), + "nothing here is a token, so the advice would be a guess: {full}" + ); + } + #[test] fn the_config_directory_is_created_and_restricted() { let path = scratch("config-dir-create").join(".mapbox"); diff --git a/src/generate_skills.rs b/src/generate_skills.rs index 28eb90f..b9d603d 100644 --- a/src/generate_skills.rs +++ b/src/generate_skills.rs @@ -478,7 +478,13 @@ fn render_body( out.push_str(&format!("{}. {prose}\n", index + 1)); } out.push_str( - "\nThe account a URL asks for is separate from the token: pass `--username`/`-u`, \ + "\nIf you are an agent, step 2 is the one that works: have \ + `MAPBOX_ACCESS_TOKEN` set before running anything, and ask the person you are \ + working for to provide one from account.mapbox.com if it is missing. Do not \ + reach for `mapbox auth login` when a command reports no token — it cannot \ + succeed without a human at a browser, and asking is faster than finding that \ + out.\n\n\ + The account a URL asks for is separate from the token: pass `--username`/`-u`, \ set `MAPBOX_USERNAME`, or let a stored login supply it. A command whose URL has \ an account placeholder cannot run without one.\n\n\ `--use-login` removes step 2, and only step 2 — a `--token` typed on the command \ @@ -1210,7 +1216,10 @@ fn token_precedence() -> [(crate::auth::TokenSource, &'static str); 3] { ( TokenSource::Login, "Credentials stored by `mapbox auth login`, refreshed automatically when \ - stale. `--profile ` picks which set.", + stale. `--profile ` picks which set. **`auth login` is the one \ + command here you cannot run.** It opens a browser and waits for a person \ + to approve, so without a terminal it refuses with `interactive_required` \ + rather than hanging. Use step 2.", ), ] } From ac28a83291122e529618102366065df915e48c57 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 16 Sep 2026 00:37:46 -0400 Subject: [PATCH 2/5] Write pasteable advice for the shell the reader actually has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught in review on the commit before this one: the repair for a blocked credential directory offered export MAPBOX_ACCESS_TOKEN="$(cat ~/.mapbox.bak)" which is POSIX-only on three counts, in a message read by somebody already stuck, on a CLI that ships a Windows build and a PowerShell installer. Reviewing the rest of auth.rs for the same mistake found two more, both unconditional: the `mv` above that line, and `Tip: export MAPBOX_USERNAME=…` printed after a successful login — which a Windows user does reach, since logging in means they had a terminal. Windows now gets `Move-Item`, `$env:NAME = Get-Content '…'` and `$env:MAPBOX_USERNAME = '…'`. PowerShell rather than cmd, for the same reason `update_check::notice` offers `irm … | iex`: it is the shell our own Windows installer is written in. (`mv` happens to work in PowerShell, which aliases it to `Move-Item` — but it is not what a Windows reader would write, and it fails outright in cmd.) **Paths are quoted now.** A Windows home directory routinely contains a space, and `Move-Item C:\Users\Jane Smith\.mapbox …` is two arguments — the repair would silently do the wrong thing on exactly the machines this change is for. `#[cfg(windows)]` would have been the easier mechanism and the wrong one: `tilesets_cli.rs` uses it, and the cost is that its PowerShell wording is compiled out of every CI run we do, so no test on any machine we build on can see it. The platform is a parameter here instead, fed `cfg!(windows)` at the one production callsite — the shape `update_check::notice` already uses, for a reason its own tests demonstrate by passing `false` and then `true`. So `the_repair_is_written_for_the_shell_the_reader_has` renders both and asserts each is free of the other's syntax, on whatever host runs it. Verified by making `rendered` ignore its argument and watching it fail. One prose fix too: "Run `mapbox auth login`, export MAPBOX_ACCESS_TOKEN, or pass --token" now says "set", which is true in every shell. 601 tests, fmt and clippy clean. --- CHANGELOG.md | 27 ++++++ README.md | 11 +++ src/auth.rs | 185 ++++++++++++++++++++++++++++++++------- src/generate_skills.rs | 36 +++++++- tests/generate_skills.rs | 68 ++++++++++++++ tests/non_interactive.rs | 21 +++-- 6 files changed, 311 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d75113..d127c71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,33 @@ that may never merge. They are not releases and are not listed here. rather than reaching for `auth login`. Reported from a real session that hit exactly this. +- `mapbox generate-skills` now prints the command that removes what it wrote, + in both output modes, and the JSON carries it as `remove_with`. The write is + `generate-skills` and the undo is `agent-skills uninstall`, a differently + named command belonging to a different feature, so there was no way to get + from one to the other — a reader of `generate-skills --help` saw no way back. + Reported after an agent reached for `rm -rf` instead, had it refused by its + sandbox, and left untracked directories in a git working tree. A dry run does + not print it, since nothing was written. `--help`, the generated reference + page and the README say it too. + +- Advice that is a command to paste is now written for the reader's shell on + Windows too. The repair for a file blocking the credential directory offered + `mv` and `export NAME="$(cat …)"`, none of which a Windows reader can run, + and the tip after a successful login offered `export MAPBOX_USERNAME=…`; + those now render as `Move-Item`, `$env:NAME = Get-Content …` and + `$env:MAPBOX_USERNAME = …` on Windows. Paths in them are quoted, because a + Windows home directory routinely contains a space and an unquoted path is two + arguments. + +- The refusal from `mapbox auth login` with no terminal now names both ways + out. It said "Set MAPBOX_ACCESS_TOKEN for a script or a CI job", which + describes automation and assumes that is who is asking; a person working + through a coding agent hits this too, and hits it again when they try the + command themselves in that agent's shell, which has no terminal either. The + fix now leads with running it in a terminal window and keeps the token as the + answer for a script, a CI job or an agent. + - A file sitting where the credential directory belongs now says so when it looks like an access token, which the one older Mapbox tools left at `~/.mapbox` does. The advice was to move it aside, with nothing to tell the diff --git a/README.md b/README.md index e3d9ceb..f3ae019 100644 --- a/README.md +++ b/README.md @@ -206,6 +206,17 @@ Skill](https://code.claude.com/docs/en/skills): `.claude/skills` for Claude Code, `.agents/skills` for Codex. `--agent`, `--global`, `--dir`, and `--service` narrow it; `--dry-run` lists files without writing them. +Without `--global` it writes into the current project, once per agent it +finds, and it prints every directory it used. To take them out again: + +```sh +mapbox agent-skills uninstall mapbox-cli +``` + +That removes every copy this command wrote, which is more than a hand-written +`rm` usually catches — a default run writes for each agent on the machine, not +just the one you had in mind. + ### Tileset CLI ```sh diff --git a/src/auth.rs b/src/auth.rs index 4793880..6ac7881 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -284,45 +284,96 @@ fn looks_like_a_legacy_token_file(path: &Path) -> bool { && !token.contains(char::is_whitespace) } +/// Setting an environment variable, written for the reader's shell. +/// +/// `export NAME=value` is a line a Windows reader cannot run, and this CLI +/// ships a Windows build and a PowerShell installer. PowerShell is the shell +/// to write for there — the same choice [`crate::update_check`] makes when it +/// offers `irm … | iex` rather than `curl … | sh`. +/// +/// The platform arrives as an argument so both renderings can be rendered in +/// a test on any host, rather than one of them being compiled out of every +/// run on the machines this repository is actually built on. +fn set_env_hint(name: &str, value: &str, windows: bool) -> String { + if windows { + format!("$env:{name} = '{value}'") + } else { + format!("export {name}={value}") + } +} + impl DirectoryBlocked { + /// The command that moves the obstruction aside, in a shell the reader has. + /// + /// `mv` is not it on Windows. PowerShell aliases `mv` to `Move-Item` so it + /// happens to work there, but `cmd.exe` has only `move`, and the installer + /// this CLI ships for Windows is PowerShell — so PowerShell is the shell + /// to write for, the same choice `update_check::notice` makes when it + /// offers `irm … | iex` instead of `curl … | sh`. + /// + /// Paths are quoted because Windows home directories routinely contain a + /// space, and `Move-Item C:\Users\Jane Smith\.mapbox …` is two arguments. + fn move_aside(&self, windows: bool) -> String { + let shown = self.path.display(); + if windows { + format!("Move-Item '{shown}' '{shown}.bak'") + } else { + format!("mv '{shown}' '{shown}.bak'") + } + } + /// The same fact in one line, fix included. /// - /// The `mv` stays. Pointing at another command to *learn* the fix would - /// send the reader to one that fails for this very reason, and "run + /// The move command stays. Pointing at another command to *learn* the fix + /// would send the reader to one that fails for this very reason, and "run /// `auth login`" reads as "you need to log in" when logging in is exactly /// what cannot help. - fn one_line(&self) -> String { + fn one_line(&self, windows: bool) -> String { let shown = self.path.display(); format!( "{shown} is a file, not a directory, so no stored credentials can be read. \ - Move it aside: mv {shown} {shown}.bak" + Move it aside: {}", + self.move_aside(windows) ) } -} -impl std::fmt::Display for DirectoryBlocked { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + /// The whole message, with the shell chosen by the caller. + /// + /// Taken as a parameter rather than read from `cfg!` in here, so a test + /// can render both and neither depends on the host it runs on — the shape + /// [`crate::update_check`]'s `notice` uses, and for the same reason. A + /// `#[cfg(windows)]` block would leave the Windows wording compiled out of + /// every CI run this repository does. + fn rendered(&self, windows: bool) -> String { let shown = self.path.display(); - write!( - f, + let mut out = format!( "{shown} is a file, but that is the directory credentials are stored in.\n\n\ - Move it aside to continue:\n\n \ - mv {shown} {shown}.bak\n\n\ - Or set {CONFIG_DIR_ENV} to keep credentials somewhere else entirely." - )?; + Move it aside to continue:\n\n {}\n\n\ + Or set {CONFIG_DIR_ENV} to keep credentials somewhere else entirely.", + self.move_aside(windows) + ); // Only once the reader knows the fix. The token is never printed: it // is a live credential, and this text reaches logs and terminals that // the file's permissions were protecting it from. if self.holds_a_token { - write!( - f, + let keep = if windows { + format!("$env:{CLAP_TOKEN_ENV} = Get-Content '{shown}.bak'") + } else { + format!("export {CLAP_TOKEN_ENV}=\"$(cat '{shown}.bak')\"") + }; + out.push_str(&format!( "\n\nIt holds what looks like an access token, left by older Mapbox \ tooling. Nothing is lost by moving it — the token still works, and \ - `{CLAP_TOKEN_ENV}` is how to keep using it:\n\n \ - export {CLAP_TOKEN_ENV}=\"$(cat {shown}.bak)\"" - )?; + `{CLAP_TOKEN_ENV}` is how to keep using it:\n\n {keep}" + )); } - Ok(()) + out + } +} + +impl std::fmt::Display for DirectoryBlocked { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.rendered(cfg!(windows))) } } @@ -798,7 +849,7 @@ pub fn load_fresh_credentials(debug: bool, profile: Option<&str>) -> Option() { // Not a locking problem, and saying so would send the reader // looking in the wrong place. - Some(blocked) => eprintln!("Warning: {}", blocked.one_line()), + Some(blocked) => eprintln!("Warning: {}", blocked.one_line(cfg!(windows))), None => eprintln!("Warning: could not lock credentials — {e}"), } return load_credentials(profile); @@ -1037,7 +1088,7 @@ fn nothing_to_report(use_login: bool, profile: Option<&str>) -> anyhow::Error { profile_name(profile) ) } else { - format!("Run `mapbox auth login`, export {CLAP_TOKEN_ENV}, or pass `--token`.") + format!("Run `mapbox auth login`, set {CLAP_TOKEN_ENV}, or pass `--token`.") }; CliError::new("not_authenticated", "No Mapbox token available.") @@ -1772,9 +1823,19 @@ fn login_has_no_way_to_show_the_url() -> anyhow::Error { /// could never complete. A flag about confirmations has no business asserting /// that a human is present. /// -/// So the answer for a headless caller is a token, and the fix says so. A -/// login on a machine with no terminal at all wants the device authorization -/// grant, which is a feature, not an escape hatch on this one. +/// The fix names both ways out, because there are two kinds of caller here +/// and only one of them is headless. +/// +/// This said only "set MAPBOX_ACCESS_TOKEN for a script or a CI job", which +/// describes automation and quietly assumes that is who is asking. Often it +/// is not: a person working through a coding agent hits this, and so does the +/// same person when they try the command themselves in that agent's shell — +/// which has no terminal either, so it fails identically. For them a token is +/// the *workaround* and the real answer is a terminal window, which the +/// message never mentioned. Reported by somebody who went looking for it. +/// +/// A login on a machine with no terminal at all wants the device +/// authorization grant, which is a feature, not an escape hatch on this one. fn login_needs_a_terminal() -> anyhow::Error { CliError::new( "interactive_required", @@ -1783,7 +1844,12 @@ fn login_needs_a_terminal() -> anyhow::Error { ) .with_remedy( Remedy::default() - .with_fix("Set MAPBOX_ACCESS_TOKEN for a script or a CI job.") + .with_fix( + "Run it in a terminal window — a shell an agent or an editor runs \ + commands through has no terminal, so the same command fails there \ + the same way. Or set MAPBOX_ACCESS_TOKEN, which is what a script, a \ + CI job or an agent should use.", + ) .with_doc(Some(remedy::TOKENS_DOC)), ) .into() @@ -1866,7 +1932,9 @@ pub fn login(debug: bool, profile: Option<&str>, mode: Mode) -> Result<()> { let text = match &creds.username { Some(u) => format!( "Logged in as {u}{profile_note}.\n\ - Tip: export MAPBOX_USERNAME={u} to skip --username on each command." + Tip: {} to skip --username on each command.", + // The name `main.rs` binds this flag to; there is no const for it. + set_env_hint("MAPBOX_USERNAME", u, cfg!(windows)) ), None => format!("Logged in successfully{profile_note}."), }; @@ -2654,7 +2722,7 @@ mod tests { assert!(full.contains("is a file"), "{full}"); assert!(full.contains(&path.display().to_string()), "{full}"); assert!( - full.contains("mv "), + full.contains("mv ") || full.contains("Move-Item "), "the message has to carry the fix, not just the diagnosis: {full}" ); assert!( @@ -2664,14 +2732,14 @@ mod tests { // The same fact for a command that is not about credentials at all, // which only needs to explain why the stored ones went missing. - let brief = err + let blocked = err .downcast_ref::() - .expect("the obstruction has to survive as its own type") - .one_line(); + .expect("the obstruction has to survive as its own type"); + let brief = blocked.one_line(cfg!(windows)); assert_eq!(brief.lines().count(), 1, "{brief}"); assert!(brief.contains("not a directory"), "{brief}"); assert!( - brief.contains(&format!("mv {}", path.display())), + brief.contains(&path.display().to_string()), "the short form still carries the fix itself: {brief}" ); assert!( @@ -2680,6 +2748,61 @@ mod tests { ); } + /// Both shells, on whichever host the suite happens to run. + /// + /// The advice is a command the reader is meant to paste, so it has to be + /// one their shell has. `export` and `$(cat …)` are neither of the two + /// things a Windows user runs, and a `#[cfg(windows)]` block would have + /// left that wording compiled out of every CI run this repository does — + /// which is why the platform is a parameter here, the way + /// `update_check::notice` takes it. + /// + /// Caught in review, on a change that had already shipped the POSIX-only + /// version to a PR. + #[test] + fn the_repair_is_written_for_the_shell_the_reader_has() { + let path = scratch("config-dir-shells").join(".mapbox"); + std::fs::write(&path, "sk.a-legacy-token").unwrap(); + let blocked = DirectoryBlocked { + path: path.clone(), + holds_a_token: true, + }; + + let unix = blocked.rendered(false); + assert!(unix.contains("mv '"), "{unix}"); + assert!(unix.contains("export "), "{unix}"); + assert!(unix.contains("$(cat "), "{unix}"); + assert!( + !unix.contains("Move-Item") && !unix.contains("$env:"), + "PowerShell has no business in the POSIX rendering: {unix}" + ); + + let windows = blocked.rendered(true); + assert!(windows.contains("Move-Item '"), "{windows}"); + assert!( + windows.contains(&format!("$env:{CLAP_TOKEN_ENV} =")), + "{windows}" + ); + assert!(windows.contains("Get-Content "), "{windows}"); + assert!( + !windows.contains("export ") && !windows.contains("$(cat "), + "a Windows reader cannot run any of that: {windows}" + ); + + // Windows home directories routinely contain a space, so an unquoted + // path is two arguments and the repair silently does the wrong thing. + for rendered in [&unix, &windows] { + assert!( + rendered.contains(&format!("'{}'", path.display())), + "the path has to be quoted: {rendered}" + ); + } + + // The short form forks the same way. + assert!(blocked.one_line(false).contains("mv '")); + assert!(blocked.one_line(true).contains("Move-Item '")); + } + /// The obstruction usually *is* a credential, and saying so changes what /// the reader does about it. /// diff --git a/src/generate_skills.rs b/src/generate_skills.rs index b9d603d..10bfd49 100644 --- a/src/generate_skills.rs +++ b/src/generate_skills.rs @@ -111,7 +111,12 @@ pub fn command() -> Command { committed and a diff means the CLI changed. Nothing here reaches the \ network, and no token is needed.\n\n\ With no flags, writes into the project directory of every agent whose \ - home directory is present." + home directory is present — so a run usually writes several places, \ + all of them named in the output.\n\n\ + `mapbox agent-skills uninstall mapbox-cli` removes every copy again. \ + Reach for that rather than deleting the directories by hand: it knows \ + all of the places this command writes to, and a sandboxed agent is \ + often allowed to run it when it is not allowed to remove files." )) .args(skill_dest::args()) .arg( @@ -1386,6 +1391,24 @@ fn write_tree(root: &Path, files: &[GeneratedFile]) -> Result<()> { Ok(()) } +/// The command that removes what this one wrote. +/// +/// Printed with the destinations, and it exists because leaving it out cost +/// somebody real work. A coding agent generated skills into a project, found +/// them redundant, and reached for `rm -rf` — which its sandbox refused, +/// leaving untracked directories in a git working tree for a human to clear +/// by hand. The undo was there the whole time; nothing pointed at it. +/// +/// Nothing *would* have. The command that writes is `generate-skills` and the +/// command that removes is `agent-skills uninstall`, which is named for a +/// different feature and documented on a different page. No amount of reading +/// `generate-skills --help` gets you there. +/// +/// It is also the more correct cleanup than the one a person would type: a +/// default run writes to every agent it detects, so the `rm -rf` in that +/// report named two directories where three had been written. +const HOW_TO_REMOVE: &str = "Remove them with: mapbox agent-skills uninstall mapbox-cli"; + /// What was written, or what would have been. fn report(mode: Mode, plans: &[Plan], dry_run: bool) -> Result<()> { let mut lines: Vec = vec![]; @@ -1409,9 +1432,20 @@ fn report(mode: Mode, plans: &[Plan], dry_run: bool) -> Result<()> { } } + // After the list rather than before it: on a dry run the reader has not + // written anything yet, and on a real one this is what they need next. + if !dry_run { + lines.push(String::new()); + lines.push(HOW_TO_REMOVE.to_string()); + } + let json = json!({ "dry_run": dry_run, "skill": SKILL_NAME, + // The same fact where a program reads, not only where a person does — + // an agent is the caller that most needs it and the least likely to + // be parsing the text rendering. + "remove_with": (!dry_run).then_some("mapbox agent-skills uninstall mapbox-cli"), "destinations": plans .iter() .map(|plan| json!({ diff --git a/tests/generate_skills.rs b/tests/generate_skills.rs index 3083686..1d9f12d 100644 --- a/tests/generate_skills.rs +++ b/tests/generate_skills.rs @@ -457,3 +457,71 @@ fn no_token_is_needed_and_the_help_says_what_it_does() { assert!(help.contains(flag), "--help does not mention {flag}"); } } + +/// Writing tells you how to unwrite, in both renderings. +/// +/// From a real report: an agent generated skills into a project, found them +/// redundant, and reached for `rm -rf` — which its sandbox refused, leaving +/// untracked directories in a git working tree for a person to clear by hand. +/// The undo existed the whole time and nothing pointed at it, and nothing +/// would have: the command that writes is `generate-skills` and the command +/// that removes is `agent-skills uninstall`, named for a different feature. +/// +/// Both renderings, because the caller who most needs this is the one least +/// likely to be reading the text one. +#[test] +fn what_was_written_carries_the_command_that_removes_it() { + const REMOVAL: &str = "mapbox agent-skills uninstall mapbox-cli"; + + let dir = scratch("removal-hint"); + let out = dir.join("out"); + std::fs::create_dir_all(&out).expect("create the destination"); + let target = out.to_str().unwrap(); + + let text = stdout(&generate( + "removal-hint-text", + &["generate-skills", "--dir", target, "-o", "text"], + )); + assert!( + text.contains(REMOVAL), + "the text output does not say how to remove what it wrote: {text}" + ); + + let json = stdout(&generate( + "removal-hint-json", + &["generate-skills", "--dir", target, "--force", "-o", "json"], + )); + let parsed: serde_json::Value = serde_json::from_str(&json).expect("JSON on stdout"); + assert_eq!( + parsed["remove_with"], REMOVAL, + "the JSON output does not carry it: {json}" + ); +} + +/// And a dry run does not, because nothing is there to remove. +/// +/// Advice to undo something that did not happen is noise, and worse, it +/// implies the write went ahead. +#[test] +fn a_dry_run_does_not_offer_to_remove_what_it_did_not_write() { + let dir = scratch("removal-hint-dry"); + let out = dir.join("out"); + std::fs::create_dir_all(&out).expect("create the destination"); + + let json = stdout(&generate( + "removal-hint-dry", + &[ + "generate-skills", + "--dry-run", + "--dir", + out.to_str().unwrap(), + "-o", + "json", + ], + )); + let parsed: serde_json::Value = serde_json::from_str(&json).expect("JSON on stdout"); + assert!( + parsed["remove_with"].is_null(), + "a dry run offered a removal for files it never wrote: {json}" + ); +} diff --git a/tests/non_interactive.rs b/tests/non_interactive.rs index 3d41282..132e95a 100644 --- a/tests/non_interactive.rs +++ b/tests/non_interactive.rs @@ -128,12 +128,23 @@ fn the_refusal_carries_a_code_and_a_fix() { assert_eq!(error_code(&out), "interactive_required"); let value: serde_json::Value = serde_json::from_str(stderr(&out).trim()).expect("JSON on stderr"); + let fix = value["fix"].as_str().unwrap_or_default(); assert!( - value["fix"] - .as_str() - .is_some_and(|fix| fix.contains("MAPBOX_ACCESS_TOKEN")), - "the fix is the actionable half, and the only action that helps here \ - is a token: {}", + fix.contains("MAPBOX_ACCESS_TOKEN"), + "the fix is the actionable half, and a token is what unblocks a script, \ + a CI job or an agent: {}", + stderr(&out) + ); + // The other way out, and the one this used to omit. A person working + // through a coding agent lands here, and so does that person when they try + // the command themselves in the agent's shell — which has no terminal + // either, so it fails identically. For them the token is the workaround and + // a terminal window is the answer, and a fix that names only the token + // reads as "you are automation" to somebody who is not. + assert!( + fix.contains("terminal"), + "a human who can open a terminal is told to set an environment \ + variable instead: {}", stderr(&out) ); // Both ways out are changes to how the command is invoked, so there is no From 05a8dc650c7ffbfc410fe08f036aaa89a0a41c9a Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 16 Sep 2026 00:46:44 -0400 Subject: [PATCH 3/5] Name every shell instead of guessing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rounds of review found that the previous commit's fix was wrong in a more interesting way than the bug it fixed. **The operating system does not determine the shell.** That fix keyed on `cfg!(windows)`, which hands PowerShell-on-macOS the `export … "$(cat …)"` form and Git-Bash-on-Windows the `$env:` form — each of them the other's syntax. `scripts/install.sh` already goes out of its way to recognise Git Bash, MSYS2 and Cygwin, so this repository knew those users existed. **And an alias is not a promise.** The same fix leaned on `mv` being a PowerShell alias for `Move-Item`. It is, on Windows — and it is not on Unix, where PowerShell drops the alias so the native tool wins, and it is something else again on a machine with GNU coreutils installed. None of that is knowable from here. So nothing is inferred now. Each row is labelled with the shell it belongs to and uses the name that shell owns: bash, zsh, fish: mv '…' '….bak' PowerShell: Move-Item '…' '….bak' cmd.exe: move "…" "….bak" bash, zsh: export MAPBOX_ACCESS_TOKEN="$(cat '….bak')" fish: set -gx MAPBOX_ACCESS_TOKEN (cat '….bak') PowerShell: $env:MAPBOX_ACCESS_TOKEN = Get-Content '….bak' cmd.exe: set /p MAPBOX_ACCESS_TOKEN=<"….bak" `Move-Item` and `Get-Content` rather than `mv` and `cat`: both are cmdlets in `Microsoft.PowerShell.Management` and cannot be shadowed out from under the reader. **Labels lead rather than trail.** `#` does not start a comment in `cmd.exe`, so a trailing `# cmd.exe` would be part of the command for the one reader least equipped to notice. A test asserts no line carries one. Verified where a shell was available rather than asserted: fish 4.9 runs the fish row and leaves the variable exported, and pwsh 7.6 reports no `mv` alias while having `Move-Item` as a cmdlet. The fish row is *not* there because `export` is missing — fish ships an `export` function for bash compatibility and the bash row does work there — but because `set -gx` is what a fish user writes, and a compatibility shim in someone else's shell is a thinner promise than that shell's own spelling. The `cmd.exe` rows are documented syntax; no Windows machine here to run them on, which is worth saying rather than papering over. `every_shell_gets_a_line_it_can_run` pins all seven rows, the quoting, and the leading labels. Verified by deleting a row and by moving a label to the end, and watching each fail. 600 tests, fmt and clippy clean. --- CHANGELOG.md | 22 +++-- src/auth.rs | 240 +++++++++++++++++++++++++-------------------------- 2 files changed, 133 insertions(+), 129 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d127c71..3973de9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,14 +40,20 @@ that may never merge. They are not releases and are not listed here. not print it, since nothing was written. `--help`, the generated reference page and the README say it too. -- Advice that is a command to paste is now written for the reader's shell on - Windows too. The repair for a file blocking the credential directory offered - `mv` and `export NAME="$(cat …)"`, none of which a Windows reader can run, - and the tip after a successful login offered `export MAPBOX_USERNAME=…`; - those now render as `Move-Item`, `$env:NAME = Get-Content …` and - `$env:MAPBOX_USERNAME = …` on Windows. Paths in them are quoted, because a - Windows home directory routinely contains a space and an unquoted path is two - arguments. +- Advice that is a command to paste now names every shell instead of guessing + one. The repair for a file blocking the credential directory offered `mv` and + `export NAME="$(cat …)"`, which a Windows reader cannot run; it now gives + `mv`/`Move-Item`/`move` and all four of `export`, fish's `set -gx`, + PowerShell's `$env: … Get-Content` and `cmd.exe`'s `set /p`, each labelled + with the shell it belongs to. The tip printed after a successful login no + longer offers `export MAPBOX_USERNAME=…` either; it says what to set rather + than how. + + Keying this on the operating system would have been wrong in both + directions, which is why it does not: PowerShell runs on macOS and Linux and + Git Bash runs on Windows. Each row also uses the name its shell owns rather + than one that may be aliased — `Move-Item` and `Get-Content` rather than `mv` + and `cat`, which resolve differently depending on what is installed. - The refusal from `mapbox auth login` with no terminal now names both ways out. It said "Set MAPBOX_ACCESS_TOKEN for a script or a CI job", which diff --git a/src/auth.rs b/src/auth.rs index 6ac7881..7c6a7d9 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -284,87 +284,107 @@ fn looks_like_a_legacy_token_file(path: &Path) -> bool { && !token.contains(char::is_whitespace) } -/// Setting an environment variable, written for the reader's shell. -/// -/// `export NAME=value` is a line a Windows reader cannot run, and this CLI -/// ships a Windows build and a PowerShell installer. PowerShell is the shell -/// to write for there — the same choice [`crate::update_check`] makes when it -/// offers `irm … | iex` rather than `curl … | sh`. -/// -/// The platform arrives as an argument so both renderings can be rendered in -/// a test on any host, rather than one of them being compiled out of every -/// run on the machines this repository is actually built on. -fn set_env_hint(name: &str, value: &str, windows: bool) -> String { - if windows { - format!("$env:{name} = '{value}'") - } else { - format!("export {name}={value}") +impl DirectoryBlocked { + /// Moving the file aside, in every shell this CLI can be run from. + /// + /// Each row uses the name that shell owns, not one that might be aliased + /// to it. `mv` in PowerShell is an alias for `Move-Item` on Windows and no + /// alias at all on Unix, where the native tool is found instead — and a + /// machine with GNU coreutils installed has a third answer. None of that + /// is knowable from here, so the PowerShell row says `Move-Item`, which is + /// a cmdlet in `Microsoft.PowerShell.Management` and cannot be shadowed + /// out from under the reader. + /// + /// This is what keying on the *shell* buys over keying on the operating + /// system. An earlier version picked `Move-Item` for Windows and `mv` + /// everywhere else, which handed Git Bash on Windows a cmdlet it does not + /// have and PowerShell on macOS a syntax it does not use. + /// + /// Paths are quoted because a Windows home directory routinely contains a + /// space, and an unquoted `C:\Users\Jane Smith\.mapbox` is two arguments. + fn move_aside(&self) -> Vec { + let shown = self.path.display(); + vec![ + format!("bash, zsh, fish: mv '{shown}' '{shown}.bak'"), + format!("PowerShell: Move-Item '{shown}' '{shown}.bak'"), + format!("cmd.exe: move \"{shown}\" \"{shown}.bak\""), + ] } -} -impl DirectoryBlocked { - /// The command that moves the obstruction aside, in a shell the reader has. + /// Setting the token, in every shell, because there is no common spelling. + /// + /// `export` with `$(…)`, fish's `set -gx` with `(…)`, PowerShell's `$env:` + /// with `Get-Content`, and `cmd.exe`'s `set /p` reading a redirect. /// - /// `mv` is not it on Windows. PowerShell aliases `mv` to `Move-Item` so it - /// happens to work there, but `cmd.exe` has only `move`, and the installer - /// this CLI ships for Windows is PowerShell — so PowerShell is the shell - /// to write for, the same choice `update_check::notice` makes when it - /// offers `irm … | iex` instead of `curl … | sh`. + /// The fish row is not there because `export` is missing — fish ships an + /// `export` function for bash compatibility, and the bash row does work + /// there (checked on fish 4.9). It is there because `set -gx` is what a + /// fish user writes, and because a compatibility shim in someone else's + /// shell is a thinner promise than that shell's own spelling. /// - /// Paths are quoted because Windows home directories routinely contain a - /// space, and `Move-Item C:\Users\Jane Smith\.mapbox …` is two arguments. - fn move_aside(&self, windows: bool) -> String { + /// The label goes in front rather than in a trailing `# comment`, because + /// `#` does not start a comment in `cmd.exe` — a trailing label would be + /// part of the command for the one reader least able to spot it. + fn keep_the_token(&self) -> Vec { let shown = self.path.display(); - if windows { - format!("Move-Item '{shown}' '{shown}.bak'") - } else { - format!("mv '{shown}' '{shown}.bak'") - } + vec![ + format!("bash, zsh: export {CLAP_TOKEN_ENV}=\"$(cat '{shown}.bak')\""), + format!("fish: set -gx {CLAP_TOKEN_ENV} (cat '{shown}.bak')"), + format!("PowerShell: $env:{CLAP_TOKEN_ENV} = Get-Content '{shown}.bak'"), + format!("cmd.exe: set /p {CLAP_TOKEN_ENV}=<\"{shown}.bak\""), + ] } /// The same fact in one line, fix included. /// + /// One line means one shell's spelling, so it is `mv` — the one that works + /// in four of the five. A `cmd.exe` reader gets the full form from any + /// `auth` command, which is where the repair actually belongs. + /// /// The move command stays. Pointing at another command to *learn* the fix /// would send the reader to one that fails for this very reason, and "run /// `auth login`" reads as "you need to log in" when logging in is exactly /// what cannot help. - fn one_line(&self, windows: bool) -> String { + fn one_line(&self) -> String { let shown = self.path.display(); format!( "{shown} is a file, not a directory, so no stored credentials can be read. \ - Move it aside: {}", - self.move_aside(windows) + Move it aside: mv '{shown}' '{shown}.bak'" ) } - /// The whole message, with the shell chosen by the caller. + /// The whole message. /// - /// Taken as a parameter rather than read from `cfg!` in here, so a test - /// can render both and neither depends on the host it runs on — the shape - /// [`crate::update_check`]'s `notice` uses, and for the same reason. A - /// `#[cfg(windows)]` block would leave the Windows wording compiled out of - /// every CI run this repository does. - fn rendered(&self, windows: bool) -> String { + /// Every shell is spelled out rather than one being guessed at. The guess + /// this replaces was `cfg!(windows)`, which is the wrong question: it names + /// the operating system and the answer depends on the shell. PowerShell + /// runs on macOS and Linux, Git Bash runs on Windows, and that fork handed + /// both of them the other one's syntax. + fn rendered(&self) -> String { let shown = self.path.display(); + let indented = |lines: Vec| { + lines + .into_iter() + .map(|line| format!(" {line}")) + .collect::>() + .join("\n") + }; + let mut out = format!( "{shown} is a file, but that is the directory credentials are stored in.\n\n\ - Move it aside to continue:\n\n {}\n\n\ + Move it aside to continue:\n\n{}\n\n\ Or set {CONFIG_DIR_ENV} to keep credentials somewhere else entirely.", - self.move_aside(windows) + indented(self.move_aside()) ); // Only once the reader knows the fix. The token is never printed: it // is a live credential, and this text reaches logs and terminals that // the file's permissions were protecting it from. if self.holds_a_token { - let keep = if windows { - format!("$env:{CLAP_TOKEN_ENV} = Get-Content '{shown}.bak'") - } else { - format!("export {CLAP_TOKEN_ENV}=\"$(cat '{shown}.bak')\"") - }; out.push_str(&format!( "\n\nIt holds what looks like an access token, left by older Mapbox \ tooling. Nothing is lost by moving it — the token still works, and \ - `{CLAP_TOKEN_ENV}` is how to keep using it:\n\n {keep}" + `{CLAP_TOKEN_ENV}` is how to keep using it:\n\n{}", + indented(self.keep_the_token()) )); } out @@ -373,7 +393,7 @@ impl DirectoryBlocked { impl std::fmt::Display for DirectoryBlocked { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.rendered(cfg!(windows))) + write!(f, "{}", self.rendered()) } } @@ -849,7 +869,7 @@ pub fn load_fresh_credentials(debug: bool, profile: Option<&str>) -> Option() { // Not a locking problem, and saying so would send the reader // looking in the wrong place. - Some(blocked) => eprintln!("Warning: {}", blocked.one_line(cfg!(windows))), + Some(blocked) => eprintln!("Warning: {}", blocked.one_line()), None => eprintln!("Warning: could not lock credentials — {e}"), } return load_credentials(profile); @@ -1932,9 +1952,7 @@ pub fn login(debug: bool, profile: Option<&str>, mode: Mode) -> Result<()> { let text = match &creds.username { Some(u) => format!( "Logged in as {u}{profile_note}.\n\ - Tip: {} to skip --username on each command.", - // The name `main.rs` binds this flag to; there is no const for it. - set_env_hint("MAPBOX_USERNAME", u, cfg!(windows)) + Tip: setting MAPBOX_USERNAME to {u} skips --username on each command." ), None => format!("Logged in successfully{profile_note}."), }; @@ -2735,7 +2753,7 @@ mod tests { let blocked = err .downcast_ref::() .expect("the obstruction has to survive as its own type"); - let brief = blocked.one_line(cfg!(windows)); + let brief = blocked.one_line(); assert_eq!(brief.lines().count(), 1, "{brief}"); assert!(brief.contains("not a directory"), "{brief}"); assert!( @@ -2748,86 +2766,66 @@ mod tests { ); } - /// Both shells, on whichever host the suite happens to run. + /// Every shell gets a line it can actually run. /// - /// The advice is a command the reader is meant to paste, so it has to be - /// one their shell has. `export` and `$(cat …)` are neither of the two - /// things a Windows user runs, and a `#[cfg(windows)]` block would have - /// left that wording compiled out of every CI run this repository does — - /// which is why the platform is a parameter here, the way - /// `update_check::notice` takes it. + /// The advice is meant to be pasted, so it has to be in the reader's + /// language. This started as `export …="$(cat …)"` unconditionally, became + /// a `cfg!(windows)` fork, and neither was right: the operating system does + /// not determine the shell. PowerShell runs on macOS and Linux, Git Bash + /// runs on Windows, and that fork handed each of them the other's syntax. /// - /// Caught in review, on a change that had already shipped the POSIX-only - /// version to a PR. + /// So every shell is spelled out and none is guessed at. Checked by hand + /// where a shell was available: fish 4.9 runs the fish row and leaves the + /// variable exported, and pwsh 7.6 has no `mv` alias but does have + /// `Move-Item`. The `cmd.exe` rows are from its documented syntax; there + /// is no Windows machine here to run them on, and that is worth knowing + /// rather than papering over. #[test] - fn the_repair_is_written_for_the_shell_the_reader_has() { + fn every_shell_gets_a_line_it_can_run() { + const TOKEN: &str = "sk.eyJ1IjoiZmFrZSJ9.not-a-real-token"; + let path = scratch("config-dir-shells").join(".mapbox"); - std::fs::write(&path, "sk.a-legacy-token").unwrap(); + std::fs::write(&path, format!("{TOKEN}\n")).unwrap(); let blocked = DirectoryBlocked { path: path.clone(), holds_a_token: true, }; - - let unix = blocked.rendered(false); - assert!(unix.contains("mv '"), "{unix}"); - assert!(unix.contains("export "), "{unix}"); - assert!(unix.contains("$(cat "), "{unix}"); - assert!( - !unix.contains("Move-Item") && !unix.contains("$env:"), - "PowerShell has no business in the POSIX rendering: {unix}" - ); - - let windows = blocked.rendered(true); - assert!(windows.contains("Move-Item '"), "{windows}"); - assert!( - windows.contains(&format!("$env:{CLAP_TOKEN_ENV} =")), - "{windows}" - ); - assert!(windows.contains("Get-Content "), "{windows}"); - assert!( - !windows.contains("export ") && !windows.contains("$(cat "), - "a Windows reader cannot run any of that: {windows}" - ); - - // Windows home directories routinely contain a space, so an unquoted - // path is two arguments and the repair silently does the wrong thing. - for rendered in [&unix, &windows] { - assert!( - rendered.contains(&format!("'{}'", path.display())), - "the path has to be quoted: {rendered}" - ); + let full = blocked.rendered(); + + // Moving it aside: each shell's own name for the operation, never one + // that depends on an alias being present. + for expected in [ + &format!("mv '{}'", path.display()), + &format!("Move-Item '{}'", path.display()), + &format!("move \"{}\"", path.display()), + ] { + assert!(full.contains(expected), "no row for {expected}: {full}"); } - // The short form forks the same way. - assert!(blocked.one_line(false).contains("mv '")); - assert!(blocked.one_line(true).contains("Move-Item '")); - } - - /// The obstruction usually *is* a credential, and saying so changes what - /// the reader does about it. - /// - /// Reported from a real session: someone found `~/.mapbox` in the way, - /// was told to move it aside, and did — with no way to know from the - /// message that the file held a working token rather than junk. - #[test] - fn a_legacy_token_file_says_the_token_is_not_lost() { - const TOKEN: &str = "sk.eyJ1IjoiZmFrZSJ9.not-a-real-token"; - - let path = scratch("config-dir-legacy-token").join(".mapbox"); - std::fs::write(&path, format!("{TOKEN}\n")).unwrap(); - - let full = prepare_config_dir(&path).unwrap_err().to_string(); + // Keeping the token: four genuinely different languages. + for expected in [ + &format!("export {CLAP_TOKEN_ENV}=\"$(cat "), + &format!("set -gx {CLAP_TOKEN_ENV} (cat "), + &format!("$env:{CLAP_TOKEN_ENV} = Get-Content "), + &format!("set /p {CLAP_TOKEN_ENV}=<"), + ] { + assert!(full.contains(expected), "no row for {expected}: {full}"); + } + // Labels lead rather than trail. `#` does not start a comment in + // `cmd.exe`, so a trailing label would be part of the command for the + // one reader least equipped to notice. assert!( - full.contains(CLAP_TOKEN_ENV), - "the way to keep using it belongs in the message: {full}" + !full.contains(" # "), + "a trailing label is part of the command in cmd.exe: {full}" ); - // The whole point of the `.bak` suffix here: the export has to name - // the file as it will be *after* the `mv` above it, or the reader - // follows two steps that contradict each other. + + // Quoted, because a Windows home directory routinely contains a space + // and an unquoted path is two arguments. assert!( - full.contains(&format!("{}.bak", path.display())), - "the export has to name the moved file, not the original: {full}" + full.contains(&format!("'{}.bak'", path.display())) + && full.contains(&format!("\"{}.bak\"", path.display())), + "paths have to be quoted in both quoting styles: {full}" ); // The one thing this must never do. A live credential in an error From bca373eca51baec5ab9296953bb235f833de2fbf Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 16 Sep 2026 00:54:26 -0400 Subject: [PATCH 4/5] Say 'deleting by hand' rather than naming one shell's command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same conflation as the commit before it, one file over: `rm` is bash, zsh and fish. A Windows reader deletes with `Remove-Item` or `del`, and the sentence was telling them our uninstall catches more than a command they do not have. The point does not need the command named at all — it is that a default run writes to every agent on the machine, so any by-hand cleanup misses whichever ones you did not think of. --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index f3ae019..781a6f4 100644 --- a/README.md +++ b/README.md @@ -213,9 +213,9 @@ finds, and it prints every directory it used. To take them out again: mapbox agent-skills uninstall mapbox-cli ``` -That removes every copy this command wrote, which is more than a hand-written -`rm` usually catches — a default run writes for each agent on the machine, not -just the one you had in mind. +That removes every copy this command wrote, which is more than deleting the +directories by hand usually catches — a default run writes for each agent on +the machine, not just the one you had in mind. ### Tileset CLI From 12ec591fc45011f037af0f58a79734571c24594e Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Thu, 17 Sep 2026 10:50:09 -0400 Subject: [PATCH 5/5] Spell it labeled, which #25's guard now enforces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This branch was written before #25 merged, so its changelog prose had never been checked by `prose_is_american_english`. Merging main brought the guard in and it caught `labelled` on the first run — which is the guard working, one PR after it landed. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1897a30..0c8b1a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,7 @@ that may never merge. They are not releases and are not listed here. one. The repair for a file blocking the credential directory offered `mv` and `export NAME="$(cat …)"`, which a Windows reader cannot run; it now gives `mv`/`Move-Item`/`move` and all four of `export`, fish's `set -gx`, - PowerShell's `$env: … Get-Content` and `cmd.exe`'s `set /p`, each labelled + PowerShell's `$env: … Get-Content` and `cmd.exe`'s `set /p`, each labeled with the shell it belongs to. The tip printed after a successful login no longer offers `export MAPBOX_USERNAME=…` either; it says what to set rather than how.