Drop stale release-channel and Statistics API preview language - #22
Conversation
The install channel at cli.mapbox.com started serving real builds with the 0.2.1 release (manifest confirmed live), so the "not serving yet" note is stale.
The version that first started serving is history, not usage guidance; the commands above are enough.
The Statistics API is generally available now, not gated behind Mapbox support enabling an account. Update the spec, docs, CLI help text, and error remedy accordingly; a 403 is no longer explained as a preview-enablement gate.
It was a no-op (switch already true everywhere) now that the Statistics API it gated is generally available and `mapbox usage` ships unconditionally. Fold statistics:read directly into DEFAULT_SCOPES and register the command unconditionally; drop the feature_flags module since nothing uses it anymore.
mattpodwysocki
left a comment
There was a problem hiding this comment.
Good change, and the test plan is the right shape — claims about a live
channel and a live API, each checked rather than asserted. I verified the ones
I could: 595 tests pass, clippy clean, Cargo.toml at 0.2.2, feature_flags
is gone with no dangling references, and statistics:read is in
DEFAULT_SCOPES.
Removing the flag is clearly right. It was the only entry in flags::ALL with
its switch already true, so the mechanism had no live user and its module
docs described a gate that gated nothing.
Requesting one change, on the strength of running it.
The 403 remedy misdiagnoses the case the API actually uses 403 for
mapbox usage on this branch, with a token that predates the scope:
{"code":"http_403",
"message":"This API requires a token with statistics:read scope.",
"fix":"This account doesn't have access to the Statistics API; contact Mapbox support if that's unexpected.",
"status":403}The API says the problem is a scope. The fix sitting next to it says the
problem is account access, and sends the reader to support — for something
they can fix themselves with mapbox auth login.
The advice for that case does exist, but it is on the wrong branch:
401 => Remedy::default().with_fix(
"Check the token has the `statistics:read` scope — run `mapbox auth login` again \
if it predates that scope, or pass one from account.mapbox.com with --token.",
),
403 => Remedy::default().with_fix(
"This account doesn't have access to the Statistics API; contact Mapbox support …",
),The API returns 403 for a missing scope, not 401. So the scope remedy is
attached to a status this endpoint doesn't use for that, and the one that
fires gives advice that cannot help.
To be fair: this PR doesn't introduce it. The old 403 text — "a private
preview: ask Mapbox support to enable it" — was also support-only, so the
mismatch predates you. Two things make it worth fixing here anyway. This is
the PR touching that remedy. And the change removes what used to compensate
for it: with the preview framing gone, support is now the whole of the
advice, where before a reader had the surrounding "needs the statistics:read
scope" language nearby to reinterpret it.
The help text still carries the right explanation, and it's good —
the token needs the
statistics:readscope.mapbox auth loginrequests it
by default; log in again if your stored token predates that.
— but that's the text someone reads before running the command, not the line
they get when it fails.
I'd have the 403 lead with the scope and keep support as the second half:
something like "Check the token has the statistics:read scope — run mapbox auth login again if it predates it. If the token has the scope, the account
may not have access; contact Mapbox support." That covers the common case
first and still lands the real one, and it stops fix contradicting message
on the same line of output.
Worth checking whether 401 is reachable here at all. If the endpoint only ever
answers 403 for both, the two remedies could collapse into one; if it uses 401
for a bad token and 403 for scope-or-access, then the 401 text should lose its
scope sentence, since it would then be describing the other branch's failure.
Smaller
- The changelog is in the right place now, which I checked because it
didn't used to be: the entry goes here under## 0.2.2, and this repo's
CHANGELOG.mdis now the canonical one — the private copy is gone and the
release tag no longer gates on it. Nothing to do; noting it because a
reviewer with older context would flag it. docs_contractpassing is worth calling out as more than a formality:
mapbox usagenow registers unconditionally rather than behind a
build-gated flag, so the documented surface and the real one had to move
together, and they did.
And one claim I could not confirm either way
That the Statistics API is "generally available now, not gated behind a
Mapbox-support enablement step". My own 403 is a scope problem rather than an
access one, so it neither supports nor contradicts it — I can't distinguish
"GA for everyone" from "enabled for this account long ago" from here. The
error path is safe either way, since a genuine access failure still points at
support, so this isn't a blocker. But if the GA claim came from the Statistics
API team rather than from the endpoint's behaviour, it's worth a line in the
PR saying so, because it's the premise the rest of the change rests on.
The Statistics API answers 403, not 401, when a token is only missing statistics:read — confirmed against a live account. The 403 remedy now leads with "run mapbox auth login again" before falling back to Mapbox support, and the 401 remedy no longer claims a scope problem it doesn't cause. Updated the OpenAPI spec and docs/commands.md to match. Also split DEFAULT_SCOPES into a DEFAULT_SCOPES_LIST array joined into a string on first use, so the scope list reads as one per line instead of one long literal.
|
Good catch, thanks. Pushed a fix: 403 now leads with "run On whether 401 is reachable at all: yes. Hit the endpoint directly with a garbage token: Same message the existing test already mocks. So the split is real: 401 = bad/missing token, 403 = valid token but missing scope, or (rarer) account has no access. Updated the OpenAPI spec and docs/commands.md to describe that instead of the old (wrong) 401=scope/403=account-not-enabled model. Also folded |
mattpodwysocki
left a comment
There was a problem hiding this comment.
Approving. The 403/401 fix is right, and I checked it the same way I found the
problem — by running it rather than by reading it.
Live, with a token that predates the scope:
Error: This API requires a token with statistics:read scope. (HTTP 403)
Fix: Check the token has the `statistics:read` scope — run `mapbox auth login`
again if it predates that scope. If it already has the scope, this account
doesn't have access to the Statistics API; contact Mapbox support.
The fix now agrees with the message beside it, leads with the case a
reader can act on, and still lands the real access problem second.
You also answered the follow-on question rather than leaving it. 401 loses
its scope sentence and becomes "the token is missing or invalid", which is what
that status actually means here. Having both remedies name the scope would have
left the same ambiguity pointing the other way.
And the test fixture now uses the API's real message. It asserted against
"Statistics API feature is not enabled for this account", which was invented;
it now uses "This API requires a token with statistics:read scope.", which is
what the endpoint returns. That is worth more than the remedy change — a test
built on a made-up response shape agrees with itself forever. The comment
recording why 403 rather than 401 ("confirmed live against a real account")
is the part that stops someone swapping them back.
The DEFAULT_SCOPES change is a real improvement, and I verified it changes nothing
Going from one long space-joined string to DEFAULT_SCOPES_LIST plus a cached
default_scopes() removes three call sites that were doing .split(' ') to get
back a list the constant used to be. OnceLock for the joined form is the right
weight for something that cannot change at runtime.
Since a silently dropped scope would break commands only after someone's next
login — the kind of thing no test here would notice — I compared the sets rather
than reading the list:
the refactor commit alone: 16 -> 16
identical, same order: True
And across the PR as a whole, against main: 15 → 16, statistics:read added,
nothing removed. Exactly what folding the flag in should do.
Verified
- 595 tests,
cargo clippy --locked --all-targetsclean,cargo fmt --check
clean. mapbox usageregisters unconditionally anddocs_contractstill passes, so
the documented surface and the real one moved together.feature_flagsis gone with no dangling references.- The changelog entry is in this repo under
## 0.2.2, which is now the right
place — the private copy is gone and the release tag no longer gates on one.
The GA premise I could not confirm from here still stands as the thing the rest
rests on, and it no longer worries me: whichever way it goes, a 403 now tells
the reader the two things that could be wrong in the order they can act on
them.
Summary
cli.mapbox.cominstall channel was not serving yet. Verified it now is:install.sh/install.ps1return 200, andlatest/manifest.jsonlists a live 0.2.1 release with checksummed artifacts for all five targets. Removed the stale note (the working commands already show usage).mapbox usage) is generally available now, not gated behind a Mapbox-support enablement step — confirmed directly by the Statistics API team. Dropped the "private preview" / support-enablement language from the OpenAPI spec,docs/commands.md, the CLI help text, and the 403 error remedy.ACCOUNT_USAGEfeature flag: it was a no-op (switch alreadytrueeverywhere) now that the API it gated is GA.statistics:readis folded directly into scopes,mapbox usageregisters unconditionally, and thefeature_flagsmodule is gone since nothing used it anymore.scripts/prepare-release.sh 0.2.2.mapbox usageerror remedy (see review discussion): the API answers 403, not 401, when a token is only missingstatistics:read— confirmed live, including that 401 is reachable for a genuinely invalid token. The 403 fix now leads with "runmapbox auth loginagain" before falling back to Mapbox support.Test plan
curl -fsSL https://cli.mapbox.com/install.shreturns 200 with real script contentcurl https://cli.mapbox.com/latest/manifest.jsonreturns a populated manifest for 0.2.1curl https://api.mapbox.com/statistics/v1?access_token=<invalid>returns 401Not Authorized - Invalid Token, confirming 401 is reachable and distinct from the scope/access 403scargo test(full suite, includingdocs_contractandaccount_usage)cargo clippy --locked --all-targets -- -D warnings