Skip to content

Fix what an agent — and a Windows user — hits when using this CLI - #28

Merged
zmofei merged 6 commits into
mainfrom
jeremy-agent-friction
Sep 18, 2026
Merged

zmofei merged 6 commits into
mainfrom
jeremy-agent-friction

Conversation

@mattpodwysocki

Copy link
Copy Markdown
Contributor

From a real session: someone pointed a coding agent at this CLI and it stumbled five times. Three of the five were the CLI behaving exactly as designed and saying so clearly — I reproduced each against the released 0.2.1 binary before touching anything. These are the two that weren't.

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 option 3 — "credentials stored by mapbox auth login" — and tries it. That command opens a browser and waits for a person.

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're working for if it's missing, and don't reach for auth login when a command reports no token.

The error auth login gives was already right and is unchanged:

{"code":"interactive_required",
 "message":"`mapbox auth login` needs a browser and someone to use it, and this run has no terminal on stdin or stderr.",
 "fix":"Set MAPBOX_ACCESS_TOKEN for a script or a CI job."}

Reaching it at all is the waste.

A blocked config directory now says when it holds a credential

A plain file at ~/.mapbox is what older Mapbox tooling left, and it's a one-line access token — src/auth.rs already anticipated this case by name. 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. The honest reading of "move it aside" is that something is being thrown away.

When the file looks like a token, it now adds that it still works and how to keep using it:

export MAPBOX_ACCESS_TOKEN="$(cat ~/.mapbox.bak)"

Naming .bak matters — that's the file as it exists after the mv directly above it — and a test holds it, because two steps that contradict each other are worse than one.

The detection 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. That's the assertion worth having, since an error message reaches every terminal and log that the file's 0600 permissions were keeping it out of. Verified by planting an actual leak and watching the test fail:

test auth::tests::a_legacy_token_file_says_the_token_is_not_lost ... FAILED
  the token itself must never be printed: …

(My first attempt at that check ran cargo test --lib on a binary crate, which errors rather than running anything — it proved nothing until I noticed and redid it.)

What I deliberately didn't change

  • mapbox agent-skills uninstall <NAME> already removes project-local skills by default and is documented in references/agent-skills.md, which we ship. The reported failure was the agent's harness denying rm, not us.
  • The curl | sh denial is a distribution problem, not a bug — filed separately.
  • MCP connection cleanup isn't this CLI.

598 tests, fmt and clippy clean.

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.
@mattpodwysocki
mattpodwysocki requested a review from a team as a code owner September 16, 2026 04:21
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.
@mattpodwysocki mattpodwysocki changed the title Stop walking agents into the one command they cannot run Fix what an agent — and a Windows user — hits when using this CLI Sep 16, 2026
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.
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.
Comment thread src/auth.rs
}

impl DirectoryBlocked {
/// Moving the file aside, in every shell this CLI can be run from.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think moving the user’s files is a good idea. Could we change where we store our credentials instead? For example, if ~/.mapbox already exists and isn’t a directory, we could use ~/.mapbox-cli/.

@mattpodwysocki mattpodwysocki left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good challenge, and I agree with the principle — a CLI shouldn't make you rearrange your home directory before it works. Two things make me want to keep the current shape, though, and the first one is that it mostly already does what you're asking.

The file doesn't block anything today

I checked each path with MAPBOX_ACCESS_TOKEN cleared, so nothing was quietly working via the environment:

# legacy ~/.mapbox present, no env token
$ mapbox styles list -u someone
Warning: … is a file, not a directory, so no stored credentials can be read. Move it aside: mv …
{"code":"http_401", …}            ← the command ran

$ mapbox auth whoami
{"code":"not_authenticated","message":"No Mapbox token available.", …}

# legacy ~/.mapbox present, with an env token
$ mapbox auth whoami
{"source":"environment","usage":"pk", …}     ← works

So the file degrades to "there are no stored credentials", the same as a fresh machine. Commands run. It's only auth login and auth logout that refuse, because those are the two that genuinely need the directory to exist.

Worth being precise about the other half too: we never move anything. The message prints a command; whether to run it is the reader's call. Nothing on disk is touched by us.

And the fallback has a trap I'd rather not ship

If the store's location depends on whether ~/.mapbox is a file, then the location can change under the user without them doing anything to the CLI:

  1. ~/.mapbox is a legacy file → we store credentials in ~/.mapbox-cli/
  2. They log in. Works.
  3. Later they tidy up and delete the stray ~/.mapbox file
  4. Next command → ~/.mapbox is now free, so we use it → they appear logged out, with their real credentials stranded in ~/.mapbox-cli/

That's a hard one to diagnose from the outside, and the trigger is an action that looks completely unrelated. It also means two documented locations rather than one, which the README and any support conversation then has to hedge on.

MAPBOX_CONFIG_DIR already covers the deliberate version of this — someone who wants the store elsewhere can say so, explicitly and permanently, and a container usually does.

If the friction is still the objection

The change in this PR doesn't add the friction; it adds a sentence to a message that already existed. Before it, the advice was "move this file aside" with no indication of whether you were discarding junk or a working credential — and it is usually a working credential, which is exactly why it's worth saying.

If you'd still rather the legacy file didn't require any action at all, the version I'd argue for is reading it as a token source — treat a one-line pk./sk./tk. file at ~/.mapbox the way we treat MAPBOX_ACCESS_TOKEN, so the old token just works and only auth login ever needs the directory. That keeps one store location, and it removes the request rather than relocating around it.

Happy to do that as a follow-up if you like it. I'd rather not put the second location in, though — that one I think we'd regret.

zmofei
zmofei previously approved these changes Sep 17, 2026

@zmofei zmofei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattpodwysocki thanks for the reply! The idea I'd like to follow here: when the program can solve a problem itself, it should, instead of asking the user to.

On that idea: automatically fall back to ~/.mapbox-cli when ~/.mapbox is blocked, instead of asking for a manual mv.

To your point about reliability: agreed, we should not trust the legacy file's content as a token. It's leftover state, not something the user gave us on purpose like MAPBOX_ACCESS_TOKEN.

To close the trap you described: prefer whichever directory already has a valid store, not whichever path happens to be free. So deleting the stray ~/.mapbox file later won't strand credentials in ~/.mapbox-cli — the CLI keeps using it. If both are valid, ~/.mapbox wins.

This is a real change to how the credential directory is resolved, so it should be its own PR, not ride on this one. Filed #30 to track it. Approving this PR — it's unaffected.

Only CHANGELOG.md conflicted: #26 added an Added and a Fixed entry where
this branch has a Changed block. All seven entries belong, so the section is
now Added / Changed / Fixed, the order 0.2.0 uses. The source changes merged
on their own.
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.
@zmofei
zmofei merged commit 8da2306 into main Sep 18, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants