From 27a8f5ae78ce1550e2ecc9861b1aa75d7e80697e Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 15 Sep 2026 15:54:09 -0400 Subject: [PATCH 1/2] Add AGENTS.md, and a CLAUDE.md pointer to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This repository already assumes the file exists. `rust-toolchain.toml` ends its explanation with "see 'Conventions' in AGENTS.md for what that PR has to show", and there is no AGENTS.md to see — a reader who follows it learns that a requirement exists but not what it is. That is the only dangling reference; the other AGENTS.md matches in the tree are the files `generate-skills` writes. It is a map, not a manual. Every module here already opens with a `//!` block explaining what it is for and what it refuses to do, and those stay the authority — a second description of the code is a second thing to keep in step, and it would rot. So the file says which one to read, which invariants are held by something other than a reviewer's memory, and which mistakes this repository has already made once. What it covers: what is generated versus what only looks generated (`openapi/` is derived and an edit to it survives until the next regeneration; `docs/commands.md` is hand-written and is the half that drifts), the output contract and the three separate things that enforce it, the single HTTP client, the helpers for values that reach a URL, the guards in tests/source_guards.rs and why adding yourself to one of their lists is almost never the fix, what `cargo test` cannot tell you, the conventions, and the compatibility and changelog rules. Every specific claim in it was checked against the code rather than remembered: the four modules in MAY_WRITE_STDOUT and why each is there, the `Payload` variants, `output::emit` and `output::progress`, `path_segment`'s four characters, `no_module_builds_its_own_client`, the anchor it links in CONTRIBUTING.md, and that `build.rs` reads PINNED_SOURCE. AGENTS.md is added to `prose_files`, so `prose_is_american_english` covers it from the start — the document stating the conventions is held to them. Verified by planting a British spelling and watching it fail at AGENTS.md:39. Stacked on the American English branch, since it cites that guard by name. 597 tests, fmt and clippy clean. --- AGENTS.md | 193 +++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 3 + tests/source_guards.rs | 1 + 3 files changed, 197 insertions(+) create mode 100644 AGENTS.md create mode 100644 CLAUDE.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..eaa654f --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,193 @@ +# Agent notes for mapbox-cli + +A map, not a manual. Every module here opens with a `//!` block explaining +what it is for and what it refuses to do, and those are the authority — this +file exists so that you know which one to read, which invariants are held by +something other than a reviewer's memory, and which mistakes this repository +has already made once. + +[CONTRIBUTING.md](CONTRIBUTING.md) is the human-facing version and covers the +same ground more briefly. [README.md](README.md) is the user-facing one. + +## What is generated, and what only looks it + +**The commands are generated at build time.** `src/spec.rs` names each +document in `openapi/` through `include_str!`, so the command tree is a +function of those specs: a spec change rebuilds the commands, and a spec +renamed upstream is a broken build rather than a silent gap. A clone compiles +with no network beyond crates.io and no second checkout. + +**`openapi/` is derived. Do not edit it.** The documents come from Mapbox's +own API descriptions through a maintainer-only regeneration step, which also +writes the `PINNED_SOURCE` file `build.rs` reads. An edit here survives until +the next regeneration and no longer, so a fix to a description belongs +upstream, not in this directory. `custom-openapi/` is the same in spirit. + +**`docs/commands.md` is written by hand, and that is the one that drifts.** +Nothing in this repository generates it. Its **Parameters** tables were +transcribed from the specs by a person and its **Outputs** blocks are real +captured responses, re-taken by hand. `tests/docs_contract.rs` holds it to the +surface the binary reports, which catches a command that vanished or was +renamed; it cannot catch a parameter description that quietly stopped being +true. Read that file's header before changing the page. + +## The output contract + +The one invariant to internalize before touching anything: + +**stdout is the result. Everything else is stderr.** `output::emit` is the +single place a result is written and the single place `--output` is honored. +Progress, warnings, hints and errors go to stderr through `output::progress` +and friends, in both output modes, so that `mapbox … > file` produces a file +holding only the answer. + +Three things enforce it, and they are there because a `println!` is such an +easy thing to add: + +- `clippy::print_stdout` is denied in `Cargo.toml`, so the macros cannot come + back. +- `only_output_completion_and_binary_responses_write_to_stdout` in + `tests/source_guards.rs` catches the other way in — taking the handle + directly. +- `tests/output_contract.rs` checks which stream each kind of output actually + lands on, in a real child process. + +Four modules may touch stdout, and the guard lists each with its reason: +`output.rs`, which is the machinery; `completion.rs`, because a shell script +wrapped in JSON is unsourceable; `executor.rs`, because a binary API response +wrapped in JSON is a corrupt PNG; and `telemetry.rs`, which does not write at +all and only reads `stdout().is_terminal()`. Adding a fifth means editing that +list, on purpose, in front of a reviewer. + +## One HTTP client + +`http::client_for` builds every request-sending client in the crate, because +that is where the token, the timeouts, the proxy handling and the `User-Agent` +are attached. A client built anywhere else is a request that arrives +anonymous, untimed, or without the caller's proxy — none of which fails +loudly. `no_module_builds_its_own_client` in `src/http.rs` holds it. + +Timeouts are a budget per kind of payload rather than one number: +`Payload::Bounded` for something a command line can hold, `Payload::File` for +a transfer. `--timeout` and `MAPBOX_TIMEOUT` override both. + +## Values that reach a URL + +`executor.rs`'s `path_segment` percent-encodes `/`, `?`, `#` and `\` in path +parameters and refuses a value of `.` or `..`, including its `%2e` spellings. +This is a fix, not a precaution: values were substituted into a path template, +so one carrying URL syntax moved the request rather than naming a segment in +it — with the caller's token attached. See the 0.2.1 entry in +[CHANGELOG.md](CHANGELOG.md). + +`update_check.rs` restricts the *shape* of a version string it reads from the +network or from its own on-disk cache rather than trying to sanitize the +content, for the same class of reason. + +If you are adding something that puts a caller's value into a URL, a header or +a filename, assume this repository has been wrong about it before and look for +the existing helper. + +## The guards + +`tests/source_guards.rs` holds the invariants that no type and no lint can +express, by reading the source and failing on the pattern. They are blunt on +purpose: they do not prove a call is correct, they make it *conspicuous*, so +that adding one is a decision somebody made rather than a line nobody looked +at twice. Today they hold which modules may delete from the filesystem, which +may write to stdout, which must carry a request id into an error, that every +telemetry marker is disclosed in README.md, and that the prose is American +English. + +Each keeps a list with a reason per entry, and each fails with a message +saying what to do. **If a guard fails, the fix is almost never to add +yourself to its list** — read the reason first. When it genuinely is, add the +entry *and* the sentence explaining it. + +## What the tests can and cannot tell you + +`cargo test` answers to fake tokens and a fake server. It runs offline, on a +machine that has never logged in, and it proves the CLI builds the request it +meant to and renders the answer it was handed. **It cannot prove Mapbox +accepts that request**, because nothing in it has ever sent one. Keep that +boundary in mind when a change is about what the API does rather than about +what this code does — the honest move is to run the command against the real +API by hand and say so, not to add a test that agrees with your assumption. + +The pattern in this repository is unit tests for pure functions plus an +end-to-end file for anything whose promise involves a real process: which +stream, which exit code, what is on disk afterwards. `tests/output_contract.rs`, +`dry_run.rs`, `non_interactive.rs` and `schema_contract.rs` all open by saying +what their unit-test counterparts cannot reach. Follow that. + +`scripts/test-install.sh` and `test-install.ps1` exercise the installers end +to end without touching the network. `scripts/test-completion.sh` does the +same for the completion scripts. + +## Conventions + +- **American English**, in comments and documentation as well as in anything + the CLI prints. `prose_is_american_english` checks it. Two deliberate + exemptions: fenced code blocks, because sample output and captured API + responses are quoted rather than written, and the `cancelled` error code, + which is a compatibility promise rather than a spelling. +- **Four rules the compiler holds rather than a reviewer**, declared in + `Cargo.toml` with the reasoning beside each: no `unsafe`, no `println!`, no + `dbg!`, no `todo!`/`unimplemented!`. `print_stderr` is deliberately *not* + denied — progress belongs there. +- **The toolchain is pinned, and the floor is a different number.** + `rust-toolchain.toml` names the exact Rust every clone and every workflow + builds with; `rust-version` in `Cargo.toml` names the oldest Rust the crate + still compiles on. They answer different questions and are meant to be far + apart. rustup applies the pin on its own, so do not add a `rustup override`: + it outranks the file and is invisible to everyone else. Bumping either is + its own PR, carrying the evidence. +- **CI builds `--locked`.** What CI tests is the graph in `Cargo.lock`. Run + without it locally, since a legitimate dependency change has to be able to + write the lockfile, then commit the result in the same PR. +- **Comments explain why, not what.** The density here is high and + deliberate: a comment that records the failure a line prevents is what stops + the next person removing it. Match the surrounding style rather than the + minimum. + +## Compatibility and the changelog + +Command names, flags, the two output modes and the exit codes are promises; +the Mapbox APIs' own response bodies are not. The rules are in +[CONTRIBUTING.md](CONTRIBUTING.md#compatibility). + +[CHANGELOG.md](CHANGELOG.md) is written by hand, newest first, because the +commit subject rarely explains why a change matters. A user-visible change +needs an entry under `## Unreleased` saying what it means for a script that +already works. Error *message* text is not one of the promises, so a change to +message prose does not need one. + +Machine-readable values are contracts even when they look like prose. The +`cancelled` error code is the standing example: it is documented in +`docs/commands.md` and asserted in `tests/non_interactive.rs`, so renaming it +is a breaking change rather than a tidy-up. + +## Before you open a PR + +```sh +cargo build +cargo fmt +cargo clippy --all-targets -- -D warnings +cargo test +``` + +All four are a condition of landing, and CI runs the same ones. + +Two habits this repository has learned the hard way, both worth more than the +four commands above: + +**Verify the claim, not the diff.** A green suite proves the tests agree with +the code, which is a different statement from the change being right. If a PR +says an API returns a header, read the header off a real response. If it says +a fix prevents something, revert the fix and watch the test fail — twice in +this repository's short history a test passed for a reason unrelated to what +it claimed to check. + +**Say what you did not check.** An honest gap named in the PR body is worth +more than a confident summary, and it is the thing a reviewer cannot recover +on their own. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..05a227b --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,3 @@ +See [AGENTS.md](./AGENTS.md). That file is the source of truth for Claude Code +and every other agent tool; this pointer exists only so Claude-specific +tooling that looks for `CLAUDE.md` finds it. diff --git a/tests/source_guards.rs b/tests/source_guards.rs index e12e961..90c4382 100644 --- a/tests/source_guards.rs +++ b/tests/source_guards.rs @@ -375,6 +375,7 @@ fn prose_files() -> Vec<(String, String)> { let mut out = vec![]; for name in [ + "AGENTS.md", "README.md", "CONTRIBUTING.md", "CHANGELOG.md", From b1d40ce26d0972d3248039b448068e1dab3c2d2d Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Fri, 18 Sep 2026 09:33:57 -0400 Subject: [PATCH 2/2] Import AGENTS.md rather than linking it, and fold in four notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The import, per @zmofei's suggestion, and he's right.** The docs are explicit: "Claude Code reads `CLAUDE.md`, not `AGENTS.md`. If your repository already uses `AGENTS.md` for other coding agents, create a `CLAUDE.md` that imports it", with `@AGENTS.md` as the example. The difference is not cosmetic. A prose "see AGENTS.md" is a sentence asking Claude to go and read a file, which costs a tool call and may not happen. `@AGENTS.md` is expanded into context at session start. So the file now does deterministically what it previously only suggested. The note below it is a block-level HTML comment, which the docs say is stripped before injection — so it explains the mechanism to a human reader without spending context on it. **Four notes from @zmofei's review**, with one correction to his first. He reported `cargo check --target x86_64-pc-windows-msvc` failing on `ring`'s build script wanting `assert.h`. I could not reproduce that, and the reason turned out to be worth more than the tip: `rustc` here resolves to `/opt/homebrew/bin/rustc`, not rustup's shim, so the check fails earlier with `can't find crate for core` and `rustup target add` reports "up to date" about a toolchain that is not the one running. Which means Homebrew's cargo does not read `rust-toolchain.toml` at all, and the pin this repository relies on is inert in that setup. It only goes unnoticed because Homebrew currently ships the same 1.98.1 the file asks for. `rustc --version` printing `(Homebrew)` is the only tell. That is now written down, since AGENTS.md otherwise says "rustup applies the pin on its own", which is true of rustup and silently false here. His conclusion stands either way — Windows-gated code cannot be checked from a Mac, and the standalone-file workaround is the way to look at it. `actionlint` and `mapbox --schema` are his too, unverified by me: no actionlint on this machine to try it with. CLAUDE.md joins `prose_files`, so the spelling guard covers it now that it holds prose. AGENTS.md is 209 lines, past the 200 the docs suggest as a target. Worth knowing before the next addition — the answer then is probably `.claude/rules/` with a `paths:` scope rather than more of this file. 597 tests, fmt and clippy clean. --- AGENTS.md | 16 ++++++++++++++++ CLAUDE.md | 12 +++++++++--- tests/source_guards.rs | 1 + 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index eaa654f..c7d0dbd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -145,6 +145,22 @@ same for the completion scripts. - **CI builds `--locked`.** What CI tests is the graph in `Cargo.lock`. Run without it locally, since a legitimate dependency change has to be able to write the lockfile, then commit the result in the same PR. +- **Two things you cannot check from a Mac, and one that lies about it.** + `cargo check --target x86_64-pc-windows-msvc` does not work here, so + `#[cfg(windows)]` code is only ever compiled by CI. To check it locally, + lift it into a standalone file with stub consts and run `rustc` or + `clippy-driver` against that, with no Cargo in the way. + + And confirm which toolchain you actually have: a Homebrew `rust` puts + `cargo` ahead of rustup's shim on `PATH`, and Homebrew's cargo does not read + `rust-toolchain.toml` at all. The pin below is then inert, and nothing says + so — `rustc --version` reporting `(Homebrew)` is the only tell. +- **`actionlint` before pushing a workflow change.** Valid YAML is not a valid + workflow — `shell:` takes no expression context, for one — and a bad + workflow file fails with no job and no log to read. +- **`mapbox --schema` beats `--help` for exploring.** One JSON document + describes every command, its arguments and the request each makes, rather + than a `--help` per command. - **Comments explain why, not what.** The density here is high and deliberate: a comment that records the failure a line prevents is what stops the next person removing it. Match the surrounding style rather than the diff --git a/CLAUDE.md b/CLAUDE.md index 05a227b..158c669 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,3 +1,9 @@ -See [AGENTS.md](./AGENTS.md). That file is the source of truth for Claude Code -and every other agent tool; this pointer exists only so Claude-specific -tooling that looks for `CLAUDE.md` finds it. +@AGENTS.md + + diff --git a/tests/source_guards.rs b/tests/source_guards.rs index 90c4382..0be47ea 100644 --- a/tests/source_guards.rs +++ b/tests/source_guards.rs @@ -376,6 +376,7 @@ fn prose_files() -> Vec<(String, String)> { for name in [ "AGENTS.md", + "CLAUDE.md", "README.md", "CONTRIBUTING.md", "CHANGELOG.md",