diff --git a/CHANGELOG.md b/CHANGELOG.md index bfca32f..d0bbaa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,18 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +- `mapbox mcp list`/`mapbox mcp install`: registers a Mapbox MCP server + (direct tool-calling access to Mapbox's APIs, not just guidance about + them) with a coding agent's own CLI or config. Claude Code, Codex, VS Code + and Cursor are supported today, against the hosted Mapbox MCP endpoints. + An existing server with the same name is left alone rather than replaced. + VS Code and Cursor currently register for every project regardless of + `--global`, since neither has a working way to scope it to one project; + Codex may report a server `installed, login incomplete` when its own + OAuth step fails against Mapbox's hosted MCP server, a known + incompatibility between the two rather than something this command + controls. + - `install.sh`/`install.ps1`: when the install finds a coding agent on the machine, it now asks, once, whether to write this CLI's own skill and install the Mapbox Agent Skills library for it, naming the agent and what diff --git a/Cargo.lock b/Cargo.lock index 1696f16..ea70de7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -722,6 +722,16 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "jsonc-parser" +version = "0.34.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ecded084f9b9a718d39668a42657c32253663d1f6afb3fbb9134f0305f73f48" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "libc" version = "0.2.189" @@ -765,6 +775,7 @@ dependencies = [ "clap_complete", "dirs", "flate2", + "jsonc-parser", "open", "rand", "reqwest", @@ -935,7 +946,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 6a51d0b..7c38069 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -91,6 +91,14 @@ base64 = "0.22" rand = "0.10" open = "5" dirs = "5" +# VS Code's and Cursor's own config files are JSONC (comments, trailing +# commas), which `serde_json` alone refuses — `mcp.rs` reads them to check +# whether a server is already registered before ever writing to them, so a +# normal, commented file must not misread as corrupt. String-aware, so a +# `//` inside a URL value is never mistaken for a comment — both features +# are needed for `parse_to_serde_value`, which decodes straight into a +# `serde_json::Value`. +jsonc-parser = { version = "0.34.0", features = ["serde", "serde_json"] } [dev-dependencies] # Integration tests build fake Mapbox tokens; same crate the CLI already uses. diff --git a/README.md b/README.md index 089f9e1..8cddff3 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,7 @@ mapbox styles list - [For AI agents](#for-ai-agents) - [Agent skills](#agent-skills) - [Generate skills](#generate-skills) + - [MCP servers](#mcp-servers) - [Global options](#global-options) - [Dry runs](#dry-runs) - [Timeouts](#timeouts) @@ -340,6 +341,26 @@ Codex. `--agent`, `--global`, `--dir` and `--service` narrow it, and mapbox agent-skills uninstall mapbox-cli ``` +### MCP servers + +```sh +mapbox mcp list +mapbox mcp install +``` + +Different kind of "install" from `agent-skills`/`generate-skills` above: +those write a directory this CLI owns, this registers an MCP server — +direct tool-calling access to Mapbox's APIs, not just guidance about them — +with a coding agent's *own* config, since that config belongs to the agent +and may already list other servers. Claude Code, Codex, VS Code and Cursor +are supported today, against the hosted Mapbox MCP endpoints: no token, no +npm package, no Node version to manage. An existing server with the same +name is left alone rather than replaced. VS Code and Cursor currently +register for every project regardless of `--global`, since neither has a +working way to scope it to one; Codex may report a server as installed with +its own login incomplete, an OAuth incompatibility between Codex and +Mapbox's hosted MCP server rather than something this command controls. + ## Global options These apply to every command, not just the API ones. diff --git a/docs/commands.md b/docs/commands.md index 06ca591..c3098a9 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -20,9 +20,9 @@ leaving the account as it was found. Every API command's **Outputs** block below is that snapshot rather than a live reading, and is re-taken by hand — nothing schedules it and nothing -enforces it. The auth, `completion`, `generate-skills` and tilesets-cli blocks -are not captures: those are the CLI's own rendering, which the test suite does -cover. +enforces it. The auth, `completion`, `generate-skills`, `mcp` and +tilesets-cli blocks are not captures: those are the CLI's own rendering, +which the test suite does cover. Nothing re-checks the captures in between, because what the Mapbox APIs return is not this repo's to monitor. What *is* ours — the commands and the flags they take — is held to `mapbox --schema` on every @@ -58,6 +58,9 @@ nests, and is typed `mapbox styles draft get`. **[Generate skills](#generate-skills)** — [generate-skills](#mapbox-generate-skills) +**[MCP servers](#mcp-servers)** — [mcp.list](#mapbox-mcp-list) · +[mcp.install](#mapbox-mcp-install) + **[Uninstall](#uninstall)** — [uninstall](#mapbox-uninstall) **[Config](#config)** — [config.get](#mapbox-config-get) · @@ -3190,6 +3193,196 @@ until `--force`. --- +## MCP servers + +Registers a Mapbox MCP server — direct tool-calling access to Mapbox's APIs, +not just guidance about them — with a coding agent's own CLI. + +**Not the same kind of "install" as [`agent-skills`](#agent-skills) or +[`generate-skills`](#generate-skills).** Those write a directory this CLI +fully owns. An MCP server has to be added to a config store that belongs to +the *client*, which may already list other servers, so this shells out to +the client's own tooling rather than editing that store directly — the same +reasoning [`tilesets-cli`](#tilesets-cli) has for exec-ing `tilesets` rather +than reimplementing it. + +Against the hosted Mapbox MCP endpoints only — no token, no npm package, no +Node version to manage. Four clients today, two different ways of driving +them: + +- **Claude Code and Codex** each have their own `mcp add`/`mcp get`, so this + runs that rather than touching either one's config file. The two need + different argv (`claude mcp add --transport http ` vs. `codex + mcp add --url `), and Codex has a real quirk worth knowing: + registering a server that advertises OAuth support starts a login flow as + *part of* `add`, and if that login fails — which it currently does + against the real Mapbox hosted endpoint, an incompatibility between + Codex's OAuth client and this server, not something this command can fix + — the config entry is written anyway. That's reported as `installed, login + incomplete` (`"status": "installed_login_incomplete"` in JSON) rather + than either a flat success or a flat failure. +- **VS Code and Cursor** have no `mcp` subcommand at all, but both expose a + top-level `--add-mcp ''` flag. Neither refuses a duplicate name — + both would silently overwrite an existing entry under the same name if + asked to — so this reads each client's own config file directly first + rather than ever calling that flag for a server already there. Where that + file lives is genuinely different per client: VS Code keeps a dedicated + `mcp.json`; Cursor keeps the same data inside `settings.json` under an + `"mcp"` key. Neither client currently has a working way to register a + server for one project rather than every one — confirmed directly, not + assumed — so both always register for every project, and this says so + rather than pretending otherwise. + +An existing server with the same name is left alone rather than replaced, +whichever of the two mechanisms above applies — the same +never-overwrite-what-you-didn't-write rule `agent-skills` follows for a +skill directory someone has edited. A config file that exists but can't be +parsed is reported as such (`config unreadable` / +`"status": "config_unreadable"`) rather than guessed past, since guessing +wrong could mean silently discarding whatever was in it. + +### `mapbox mcp list` + +Every known server and client, and whether each is already registered. + +#### Examples + +```sh +mapbox mcp list +``` + +#### Outputs + + + + +
textjson
+ +``` +mapbox claude-code not installed +mapbox-devkit claude-code not installed +mapbox codex not installed +mapbox-devkit codex not installed +mapbox vscode client not found +mapbox-devkit vscode client not found +mapbox cursor not installed +mapbox-devkit cursor not installed +``` + + + +```json +{ + "servers": [ + { "server": "mapbox", "client": "claude-code", "status": "not_installed" }, + { "server": "mapbox-devkit", "client": "claude-code", "status": "not_installed" }, + { "server": "mapbox", "client": "codex", "status": "not_installed" }, + { "server": "mapbox-devkit", "client": "codex", "status": "not_installed" }, + { "server": "mapbox", "client": "vscode", "status": "client_not_found" }, + { "server": "mapbox-devkit", "client": "vscode", "status": "client_not_found" }, + { "server": "mapbox", "client": "cursor", "status": "not_installed" }, + { "server": "mapbox-devkit", "client": "cursor", "status": "not_installed" } + ] +} +``` + +
+ +`client not found` in place of a status is that client's own CLI not being on +`PATH` at all — see [`mcp install`](#mapbox-mcp-install) below for what that +means for installing. `config unreadable` means VS Code's or Cursor's own +config file exists but didn't parse (checked as JSONC, so an ordinary +commented file is not what triggers this). `-o json` spells every status in +snake_case (`not_installed`, `client_not_found`, `config_unreadable`); this +page's prose and the `text` column above use the spaced form for reading, +never for matching against. + +--- + +### `mapbox mcp install` + +Registers every known server for every detected client. With no `--client` +named and no known client's CLI reachable at all, this is an error — +`mcp_client_not_found`, exit 1 — rather than a silent no-op; see the end of +this section for its exact shape. A client named explicitly, or detected, +but whose CLI goes unreachable partway through (or whose config can't be +read) is different: that one pair is skipped and named, the rest of the run +continues. + +#### Parameters + +| Parameter | Effect | +| --- | --- | +| `--server ` | Repeatable. Defaults to every known server (`mapbox`, `mapbox-devkit`). | +| `--client ` | Repeatable. Defaults to whichever clients are detected. One of `claude-code`, `codex`, `vscode`, `cursor`. | +| `--global` | Register for every project rather than just this one. Only Claude Code (`--scope user`) draws that distinction — the other three currently register for every project regardless. | +| `--dry-run` | Report what would be installed, then exit without installing it. | + +#### Examples + +```sh +mapbox mcp install + +mapbox mcp install --server mapbox --client vscode + +mapbox mcp install --server mapbox --global + +mapbox mcp install --dry-run +``` + +#### Outputs + + + + +
textjson
+ +``` +Mapbox MCP: https://mcp.mapbox.com/mcp for Claude Code — installed. +Mapbox DevKit MCP: https://mcp-devkit.mapbox.com/mcp for Claude Code — installed. +Mapbox MCP: https://mcp.mapbox.com/mcp for Codex — installed, login incomplete. +Mapbox MCP: https://mcp.mapbox.com/mcp for VS Code — installed. +``` + + + +```json +{ + "results": [ + { "server": "mapbox", "client": "claude-code", "status": "installed" }, + { "server": "mapbox-devkit", "client": "claude-code", "status": "installed" }, + { "server": "mapbox", "client": "codex", "status": "installed_login_incomplete", "error": "..." }, + { "server": "mapbox", "client": "vscode", "status": "installed" } + ] +} +``` + +
+ +Run again, `installed` reads `already installed` (`already_installed` in +JSON) for each — the server is left exactly as it is, nothing is re-written. +`--dry-run` reads `would install` (`would_install`) instead of attempting +anything. A client whose CLI isn't reachable reads `is not on PATH, skipped` +(`"status": "client_not_found"`) rather than stopping the rest of the run, +and one whose config exists but couldn't be parsed (VS Code, Cursor) reads +`'s config could not be read, skipped` (`"status": "config_unreadable"`), +also without stopping the rest of the run. A registration that actually +fails reads `failed`, with the detail in `"error"`, and makes the whole +command exit 1 once everything has been attempted and reported — a skipped +pair (client not found, config unreadable) does not count toward that, +since nothing was attempted there to call a failure. + +With no `--client` named and no known client's CLI reachable at all, this is +an error rather than a silent no-op — `mcp_client_not_found` in `-o json`, +naming the clients it looked for: + +``` +Error: No supported coding-agent CLI was found: claude-code, codex, vscode, cursor. +Fix: Install one of these CLIs, or pass --client to name one anyway (claude-code, codex, vscode, cursor). +``` + +--- + ## Uninstall ### `mapbox uninstall` diff --git a/src/main.rs b/src/main.rs index de67a96..24de09f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -27,6 +27,7 @@ mod generate_skills; mod history; mod http; mod link; +mod mcp; mod output; mod remedy; mod run_history; @@ -612,6 +613,13 @@ fn build_app(specs: &[ServiceSpec]) -> Command { // same agent directories, which is why they share `skill_dest`. app = app.subcommand(agent_skills::command()); + // The other half of "make a coding agent Mapbox-aware": the two skills + // commands above write guidance an agent reads, this registers an MCP + // server an agent can actually call. It shells out to the agent's own + // CLI rather than sharing `skill_dest`, since it edits a config store + // that CLI owns rather than a directory this one does. + app = app.subcommand(mcp::command()); + // Between the other two hand-written leaves, so the help lists the three // that make no request together and in the order someone meets them. // What it prints is built from `app` itself, which is why nothing here @@ -1099,6 +1107,11 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - agent_skills::RunFlags { debug, assume_yes }, mode, )?, + // Ahead of the generic service arm for the same reason as its + // neighbors: it makes no Mapbox request and needs no token. It does + // shell out to another process (the agent's own CLI), but that is + // local, not a network call. + Some((mcp::COMMAND, mcp_matches)) => mcp::run(mcp_matches, mode)?, // Ahead of the generic service arm for the same reason again, and // handed `app` for the same reason `generate-skills` is: the script // it prints is a rendering of the command tree already in memory. diff --git a/src/mcp.rs b/src/mcp.rs new file mode 100644 index 0000000..ac11282 --- /dev/null +++ b/src/mcp.rs @@ -0,0 +1,999 @@ +//! `mapbox mcp` — register a Mapbox MCP server with a coding agent's CLI. +//! +//! Not the same kind of "install" as [`crate::agent_skills`] or +//! [`crate::generate_skills`]. Those write a directory this CLI fully owns; +//! an MCP server has to be registered into a config store that belongs to +//! the agent and may already list other servers, so writing it directly +//! would mean parsing and rewriting someone else's file. Every client here +//! offers its own way to do that write instead, so this shells out rather +//! than reimplementing it — the same reasoning [`crate::tilesets_cli`] has +//! for exec-ing `tilesets` rather than reimplementing it. +//! +//! # Servers and clients +//! +//! [`SERVERS`] is every Mapbox MCP server this CLI knows how to point a +//! client at — a name, a label, and the hosted HTTP endpoint that serves it. +//! [`CLIENTS`] is every coding-agent CLI this command knows how to drive, and +//! [`ClientKind`] is the one real fork in how that happens: +//! +//! - **[`ClientKind::Verb`]** — Claude Code and Codex each have their own +//! ` mcp add`/` mcp get`, so this shells out to that rather +//! than touching either one's config file. The two still need their own +//! `verb_add` arm apiece: Claude Code takes `add --transport http +//! `, Codex takes `add --url `, and Codex additionally +//! starts an OAuth flow as *part of* `add` for a server that advertises +//! support for it — verified live against the real Mapbox hosted +//! endpoint, where that flow itself fails (an incompatibility between +//! Codex's OAuth client and this server, not something this command can +//! fix) while the config entry is written regardless. See [`verb_add`]'s +//! Codex arm for how that's told apart from an add that really failed. +//! - **[`ClientKind::AddMcpFlag`]** — VS Code and Cursor have no `mcp` +//! subcommand at all, but both expose a top-level `--add-mcp ''` +//! flag, and the same input JSON works for both — confirmed live, in an +//! isolated `--user-data-dir`, that it correctly merges with an existing, +//! differently-named entry on each. Where it lands is genuinely different +//! per client, though, and [`AddMcpConfig`] names that per row rather than +//! assuming one editor's behavior for its fork: VS Code writes a small, +//! dedicated `User/mcp.json`; Cursor writes into `User/settings.json` +//! under an `"mcp"` key instead — and that difference showed up *during +//! this feature's own development*, when updating the installed Cursor +//! (which additionally had a broken `--add-mcp` before the update, a +//! separate, now-fixed bug) moved its own storage from the former shape to +//! the latter. Neither client's `--add-mcp` refuses a duplicate name — +//! both **silently overwrite** an existing entry under the *same* name if +//! its content differs — so [`file_get`] reads the config file directly +//! (never writes it) to decide whether to call `--add-mcp` at all, since +//! neither the flag nor either file format gives this command anything to +//! check first on its own. Both editors' `--mcp-workspace`-style +//! per-project flag was also tested live and found non-functional in the +//! installed versions (Cursor's is not a recognized option at all; +//! neither one is documented for `code`), so both always register in the +//! user profile regardless of `--global`, disclosed with one printed line +//! rather than silently ignored. +//! +//! Adding a server is one row in [`SERVERS`], since every function here is +//! generic over `Client`/`Server`. Adding a *client* of a kind already here +//! is a new `Client` row plus one new arm in `verb_add`/`verb_get`'s match +//! (for `Verb`) or just a new [`AddMcpConfig`] (for `AddMcpFlag`, since +//! `add_mcp_flag_add`/`file_get` are already generic over where the config +//! lives). A client that needs a config file hand-edited with no +//! CLI-driven write path at all — Claude Desktop, Goose — needs a third +//! `ClientKind` and is real, separate work, not covered here. +//! +//! Deliberately not [`crate::skill_dest::Agent`]: that table is entirely +//! about *where a skill file goes under an agent's home directory*, and its +//! own module docs say "installed" means a directory exists, not a `PATH` +//! lookup. What this needs is the opposite signal — is the client's own CLI +//! binary invocable at all — so it keeps its own, much smaller table. +//! +//! # Checking first +//! +//! Every client here is checked before ever attempting to register a +//! server, and the check means something slightly different per kind: for +//! `Verb`, ` mcp get ` (a spawn failure with +//! [`std::io::ErrorKind::NotFound`] means the client isn't there, a +//! non-zero exit means the server isn't registered yet, success means it +//! already is — stronger than parsing `add`'s own failure text, which for +//! Claude Code is plain, uncoded prose and for Codex doesn't exist at all, +//! since Codex's `add` doesn't refuse a duplicate on its own). For +//! `AddMcpFlag`, reading the client's own config file directly (see +//! [`AddMcpConfig`]), since nothing about the flag or either file format +//! offers an existence check any other way. + +use std::io::{self, BufRead, BufReader}; +use std::path::{Path, PathBuf}; +use std::process::{Command, ExitStatus, Stdio}; + +use anyhow::Result; +use clap::builder::PossibleValuesParser; +use clap::{Arg, ArgAction, ArgMatches, Command as ClapCommand}; +use serde_json::json; + +use crate::executor; +use crate::output::{self, CliError, Mode}; +use crate::remedy::Remedy; + +pub const COMMAND: &str = "mcp"; + +const LIST: &str = "list"; +const INSTALL: &str = "install"; + +const SERVER_ARG: &str = "server"; +const CLIENT_ARG: &str = "client"; +const GLOBAL_ARG: &str = "global"; + +/// One Mapbox MCP server this command can point a client at. +struct Server { + /// `--server` value, and the name it's registered under in the client. + flag: &'static str, + label: &'static str, + /// The hosted, OAuth-authenticated endpoint — no npm package, no token, + /// no Node version to worry about. See the module docs for why this is + /// the only transport supported so far. + url: &'static str, +} + +const SERVERS: &[Server] = &[ + Server { + flag: "mapbox", + label: "Mapbox MCP", + url: "https://mcp.mapbox.com/mcp", + }, + Server { + flag: "mapbox-devkit", + label: "Mapbox DevKit MCP", + url: "https://mcp-devkit.mapbox.com/mcp", + }, +]; + +/// How a [`Client`] is driven. See the module docs for what each means. +#[derive(PartialEq, Eq)] +enum ClientKind { + Verb, + AddMcpFlag, +} + +/// One coding-agent CLI this command knows how to drive. +struct Client { + /// `--client` value. + flag: &'static str, + label: &'static str, + /// Overrides which binary answers to `flag`, the same escape hatch + /// `MAPBOX_TILESETS_CLI` gives `tilesets_cli` — a binary not on `PATH`, + /// or a wrapper script standing in for it in a test. + binary_env: &'static str, + default_binary: &'static str, + kind: ClientKind, + /// `AddMcpFlag` only. `None` for `Verb` clients, which have no config + /// file this command ever reads. + add_mcp: Option, +} + +/// Where an `AddMcpFlag` client's own MCP registrations actually live — +/// genuinely different per client, and confirmed live for the exact +/// installed version at the time, not assumed from one editor's behavior +/// applying to its fork. This could drift with a future release the same +/// way it already did once *during this feature's own development*: an +/// older installed Cursor wrote a dedicated `mcp.json` (like VS Code still +/// does); updating it moved the same data into `settings.json` under an +/// `"mcp"` key instead, with no `mcp.json` written at all. +struct AddMcpConfig { + /// Directory name this editor's fork uses under the OS's config-home + /// (`dirs::config_dir()` — `~/Library/Application Support` on macOS, + /// `$XDG_CONFIG_HOME`/`~/.config` on Linux, `%APPDATA%` on Windows) — + /// "Code", not "VS Code". + app_dir_name: &'static str, + /// File under `User/` that holds the servers this flag writes. + file_name: &'static str, + /// Path to the servers object inside that file's JSON, root first: + /// `["servers"]` for VS Code's dedicated file, `["mcp", "servers"]` for + /// Cursor's entry inside its general settings. + servers_path: &'static [&'static str], + /// Overrides the resolved config-home base directory entirely — the + /// same kind of escape hatch `binary_env` gives the binary path, and + /// how tests point this at a scratch directory rather than the real + /// one. + env_override: &'static str, +} + +const CLIENTS: &[Client] = &[ + Client { + flag: "claude-code", + label: "Claude Code", + binary_env: "MAPBOX_CLAUDE_CLI", + default_binary: "claude", + kind: ClientKind::Verb, + add_mcp: None, + }, + Client { + flag: "codex", + label: "Codex", + binary_env: "MAPBOX_CODEX_CLI", + default_binary: "codex", + kind: ClientKind::Verb, + add_mcp: None, + }, + Client { + flag: "vscode", + label: "VS Code", + binary_env: "MAPBOX_CODE_CLI", + default_binary: "code", + kind: ClientKind::AddMcpFlag, + add_mcp: Some(AddMcpConfig { + app_dir_name: "Code", + file_name: "mcp.json", + servers_path: &["servers"], + env_override: "MAPBOX_CODE_CONFIG_DIR", + }), + }, + Client { + flag: "cursor", + label: "Cursor", + binary_env: "MAPBOX_CURSOR_CLI", + default_binary: "cursor", + kind: ClientKind::AddMcpFlag, + add_mcp: Some(AddMcpConfig { + app_dir_name: "Cursor", + file_name: "settings.json", + servers_path: &["mcp", "servers"], + env_override: "MAPBOX_CURSOR_CONFIG_DIR", + }), + }, +]; + +impl Client { + /// The binary to run: the override if set to something non-empty, + /// otherwise the bare name resolved against `PATH` — identical to + /// `tilesets_cli::binary`. + fn binary(&self) -> PathBuf { + match std::env::var_os(self.binary_env) { + Some(path) if !path.is_empty() => PathBuf::from(path), + _ => PathBuf::from(self.default_binary), + } + } +} + +/// `binary`, then — on Windows only, and only when it carries no extension +/// of its own — the same name again with `.cmd`, `.bat` and `.exe` +/// appended, in that order. +/// +/// `CreateProcessW`, what `std::process::Command` calls on Windows, does +/// not probe `PATHEXT` the way `cmd.exe` does for a bare name, so a CLI +/// installed as a `.cmd` shim (every npm-installed one — VS Code's and +/// Cursor's own launchers are `.cmd` too) is invisible to a plain +/// `Command::new("code")` there. Not verified live (no Windows machine +/// here), but this is the documented shape of the gap, and trying the +/// extensions costs nothing extra on a bare name that already resolves. +fn command_candidates(binary: &Path) -> Vec { + if !cfg!(windows) || binary.extension().is_some() { + return vec![binary.to_path_buf()]; + } + let mut out = vec![binary.to_path_buf()]; + for ext in ["cmd", "bat", "exe"] { + let mut candidate = binary.as_os_str().to_os_string(); + candidate.push("."); + candidate.push(ext); + out.push(PathBuf::from(candidate)); + } + out +} + +/// Runs `client`'s binary with `args`, captured, trying +/// [`command_candidates`] in turn until one is not `NotFound`. For a quick +/// check (`--version`, `mcp get`) where nothing the child prints needs to +/// reach a person live — see [`spawn_live`] for the one call site where +/// that is not true. +fn spawn(client: &Client, args: &[&str]) -> io::Result { + let mut last_err = None; + for candidate in command_candidates(&client.binary()) { + match Command::new(&candidate).args(args).output() { + Ok(output) => return Ok(output), + Err(e) if e.kind() == io::ErrorKind::NotFound => last_err = Some(e), + Err(e) => return Err(e), + } + } + Err(last_err.unwrap_or_else(|| io::Error::from(io::ErrorKind::NotFound))) +} + +/// [`spawn`], but for the one family of calls that can have something worth +/// a person seeing *while it runs*: registering a server. Codex's own `add` +/// starts an OAuth flow for a server that advertises support for it, which +/// can print a URL to open — captured alone, as every other call here is, +/// that URL is never shown and the run looks like it hung. Forwards both of +/// the child's streams to this process's own stderr line by line as they +/// arrive, on a second thread for stdout so draining one pipe can never +/// block behind the other filling up, and returns the exit status plus +/// everything printed, concatenated, for a caller that still wants the text +/// (an error detail, Codex's login-incomplete message). +fn spawn_live(client: &Client, args: &[&str]) -> io::Result<(ExitStatus, String)> { + let mut last_err = None; + for candidate in command_candidates(&client.binary()) { + let mut child = match Command::new(&candidate) + .args(args) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + { + Ok(child) => child, + Err(e) if e.kind() == io::ErrorKind::NotFound => { + last_err = Some(e); + continue; + } + Err(e) => return Err(e), + }; + + let stdout = child.stdout.take().expect("stdout is piped above"); + let stderr = child.stderr.take().expect("stderr is piped above"); + + let stdout_thread = std::thread::spawn(move || { + let mut captured = String::new(); + for line in BufReader::new(stdout).lines().map_while(Result::ok) { + eprintln!("{line}"); + captured.push_str(&line); + captured.push('\n'); + } + captured + }); + + let mut captured = String::new(); + for line in BufReader::new(stderr).lines().map_while(Result::ok) { + eprintln!("{line}"); + captured.push_str(&line); + captured.push('\n'); + } + captured.push_str(&stdout_thread.join().unwrap_or_default()); + + let status = child.wait()?; + return Ok((status, captured)); + } + Err(last_err.unwrap_or_else(|| io::Error::from(io::ErrorKind::NotFound))) +} + +pub fn command() -> ClapCommand { + let server_flags: Vec<&'static str> = SERVERS.iter().map(|s| s.flag).collect(); + let client_flags: Vec<&'static str> = CLIENTS.iter().map(|c| c.flag).collect(); + + ClapCommand::new(COMMAND) + .about("Set up a Mapbox MCP server for a coding agent") + .long_about( + "Registers a Mapbox MCP server — direct tool-calling access to Mapbox's APIs, \ + not just guidance about them — with a coding agent's own CLI or config store.\n\n\ + Different from `mapbox agent-skills` and `mapbox generate-skills`, which write \ + files this CLI fully owns. An MCP server has to be added to a config store that \ + belongs to the agent and may already list other servers, so this shells out to \ + the agent's own tooling (`claude mcp add`, `codex mcp add`, `code --add-mcp`) \ + rather than editing that store directly.\n\n\ + Against the hosted Mapbox MCP endpoints only — no token, no npm package, no Node \ + version to manage. `mapbox mcp list` names every server and client this command \ + knows about.", + ) + .subcommand_required(true) + .subcommand( + ClapCommand::new(LIST).about("List known MCP servers and clients, and what's already installed"), + ) + .subcommand( + ClapCommand::new(INSTALL) + .about("Register a server with a client. With neither flag, every known server for every detected client") + .arg( + Arg::new(SERVER_ARG) + .long(SERVER_ARG) + .value_name("SERVER") + .action(ArgAction::Append) + .value_parser(PossibleValuesParser::new(server_flags)) + .help("Server to install, repeatable. Defaults to every known server"), + ) + .arg( + Arg::new(CLIENT_ARG) + .long(CLIENT_ARG) + .value_name("CLIENT") + .action(ArgAction::Append) + .value_parser(PossibleValuesParser::new(client_flags)) + .help("Client to install for, repeatable. Defaults to whichever clients are detected"), + ) + .arg( + Arg::new(GLOBAL_ARG) + .long(GLOBAL_ARG) + .action(ArgAction::SetTrue) + .help( + "Register for every project rather than this one. Some clients \ + have no per-project scope and register globally either way", + ), + ) + .arg(executor::dry_run_arg( + "Report what would be installed, then exit without installing it", + )), + ) +} + +/// What checking a (client, server) pair before installing found. +enum GetOutcome { + Installed, + NotInstalled, + ClientNotFound, + /// `AddMcpFlag` only: the config file exists but didn't parse as JSON. + /// Never guessed past — see [`file_get`]. + Unreadable, +} + +fn client_get(client: &Client, server_name: &str) -> GetOutcome { + match client.kind { + ClientKind::Verb => verb_get(client, server_name), + ClientKind::AddMcpFlag => file_get(client, server_name), + } +} + +/// ` mcp get ` — the same invocation for every `Verb` client; +/// only `add` differs per client. See the module docs. +fn verb_get(client: &Client, server_name: &str) -> GetOutcome { + match spawn(client, &["mcp", "get", server_name]) { + Ok(output) if output.status.success() => GetOutcome::Installed, + Ok(_) => GetOutcome::NotInstalled, + // Every spawn failure, not only `NotFound`: this client's `mcp get` + // cannot be driven either way, which is what `ClientNotFound` + // means here. + Err(_) => GetOutcome::ClientNotFound, + } +} + +/// `AddMcpFlag` clients: reads the config file directly rather than writing +/// it. A file that doesn't exist yet is "not installed" (a fresh client); +/// one that exists but can't be read or doesn't parse is `Unreadable` +/// rather than guessed past as either state — proceeding past content this +/// command can't understand is exactly the kind of guess that could +/// silently discard something `--add-mcp` itself would have clobbered. +/// Parsed as JSONC, not plain JSON: VS Code's `mcp.json` and Cursor's +/// `settings.json` both allow comments and a trailing comma, and a normal, +/// untouched file of either kind otherwise reads as unreadable on first +/// contact. +fn file_get(client: &Client, server_name: &str) -> GetOutcome { + if !client_reachable(client) { + return GetOutcome::ClientNotFound; + } + let Some(config) = &client.add_mcp else { + return GetOutcome::NotInstalled; + }; + let Some(path) = config_path(config) else { + return GetOutcome::NotInstalled; + }; + let text = match std::fs::read_to_string(&path) { + Ok(text) => text, + // Only "the file isn't there yet" means "not installed." Anything + // else reading it, no permission, it's a directory, invalid UTF-8, + // means this command cannot tell, and must not guess: a `NotFound` + // only check here was the actual bug — every other error used to + // fall into "not installed" too, which then ran `--add-mcp` over a + // file this command had never actually looked at. + Err(e) if e.kind() == io::ErrorKind::NotFound => return GetOutcome::NotInstalled, + Err(_) => return GetOutcome::Unreadable, + }; + let value: serde_json::Value = + match jsonc_parser::parse_to_serde_value(&text, &jsonc_parser::ParseOptions::default()) { + Ok(value) => value, + Err(_) => return GetOutcome::Unreadable, + }; + let mut cursor = &value; + for key in config.servers_path { + match cursor.get(key) { + Some(next) => cursor = next, + None => return GetOutcome::NotInstalled, + } + } + if cursor.get(server_name).is_some() { + GetOutcome::Installed + } else { + GetOutcome::NotInstalled + } +} + +/// An `AddMcpFlag` client's config file, honoring `env_override` before +/// falling back to `dirs::config_dir()` — the macOS/Linux/Windows-correct +/// base, confirmed live against the real path these editors write to +/// (`~/Library/Application Support//User/...` on macOS). Deliberately +/// not `skill_dest`'s `XdgConfig` base, which exists precisely because +/// *that* module's agents read `~/.config` even on macOS — the opposite of +/// what was verified here. +fn config_path(config: &AddMcpConfig) -> Option { + if let Some(value) = std::env::var_os(config.env_override) { + if !value.is_empty() { + return Some(PathBuf::from(value).join("User").join(config.file_name)); + } + } + Some( + dirs::config_dir()? + .join(config.app_dir_name) + .join("User") + .join(config.file_name), + ) +} + +/// Whether `client`'s CLI can be run at all, without asking about any +/// particular server. `--version` rather than a bogus `mcp get NAME`: it is +/// what the binary is *for*, so a client that answers has no reason to +/// treat it specially, unlike a lookup for a server name this command made +/// up. The same check for every kind: `AddMcpFlag` clients answer +/// `--version` too, confirmed live for both `code` and `cursor`. +fn client_reachable(client: &Client) -> bool { + spawn(client, &["--version"]).is_ok() +} + +/// What attempting to register a server found, once it wasn't already +/// there. Distinct from a plain success because Codex's own exit code +/// answers a different question than "was this written" — see `verb_add`'s +/// Codex arm. +enum AddOutcome { + Installed, + /// The config entry was written (confirmed via a follow-up `get`), but + /// the client's own login/OAuth step that normally accompanies it did + /// not complete. `String` is the detail to show, not a failure. + InstalledLoginIncomplete(String), +} + +fn client_add(client: &Client, server: &Server, global: bool) -> Result { + match client.kind { + ClientKind::Verb => verb_add(client, server, global), + ClientKind::AddMcpFlag => add_mcp_flag_add(client, server, global), + } +} + +fn verb_add(client: &Client, server: &Server, global: bool) -> Result { + match client.flag { + "claude-code" => { + let mut args = vec!["mcp", "add", "--transport", "http", server.flag, server.url]; + if global { + args.push("--scope"); + args.push("user"); + } + let (status, captured) = + spawn_live(client, &args).map_err(|e| spawn_failed(client, e))?; + if status.success() { + Ok(AddOutcome::Installed) + } else { + Err(add_failed(client, server, &captured)) + } + } + "codex" => { + // No local/user scope distinction in codex's own CLI (every + // add is what it calls a "global" server), so `global` is + // unused here — same as it is for VS Code. + let (status, captured) = + spawn_live(client, &["mcp", "add", server.flag, "--url", server.url]) + .map_err(|e| spawn_failed(client, e))?; + + // Codex starts an OAuth flow as part of `add` for a server + // that advertises support for it, and its exit code reflects + // whether *that* succeeded, not whether the entry was written + // — confirmed live: against the real Mapbox hosted endpoint, + // the OAuth step itself fails (an incompatibility between + // Codex's OAuth client and this server), but `codex mcp get` + // immediately afterward shows the entry present regardless. + // So the truth this reports is a follow-up `get`, not this + // exit code. Its output is forwarded live rather than only + // captured (`spawn_live`, not `spawn`) because that OAuth step + // can print a URL to open, and a captured-only child would + // leave it unshown while the run looks hung. + let written = matches!(verb_get(client, server.flag), GetOutcome::Installed); + if !written { + return Err(add_failed(client, server, &captured)); + } + if status.success() { + Ok(AddOutcome::Installed) + } else { + Ok(AddOutcome::InstalledLoginIncomplete( + captured.trim().to_string(), + )) + } + } + _ => unreachable!("every `Verb` client has an arm here"), + } +} + +/// ` --add-mcp ''`. `global` is accepted for symmetry with +/// `verb_add` but unused: neither VS Code's nor Cursor's `--add-mcp` has a +/// working per-project scope (checked directly: `code --help` names no such +/// flag at all, and Cursor's documented `--mcp-workspace` is rejected by +/// its own CLI as an unrecognized option in the version tested), so this +/// always registers in the user profile and says so once, plainly, rather +/// than silently ignoring what was asked for. +fn add_mcp_flag_add(client: &Client, server: &Server, global: bool) -> Result { + if !global { + output::progress(&format!( + "{} has no per-project scope for this; registering in the user profile instead.", + client.label + )); + } + + let payload = json!({ + "name": server.flag, + "type": "http", + "url": server.url, + }) + .to_string(); + + let (status, captured) = + spawn_live(client, &["--add-mcp", &payload]).map_err(|e| spawn_failed(client, e))?; + + if status.success() { + Ok(AddOutcome::Installed) + } else { + Err(add_failed(client, server, &captured)) + } +} + +fn add_failed(client: &Client, server: &Server, captured: &str) -> anyhow::Error { + CliError::new( + "error", + format!( + "{} could not register {}: {}", + client.label, + server.label, + captured.trim() + ), + ) + .into() +} + +fn spawn_failed(client: &Client, err: io::Error) -> CliError { + if err.kind() == io::ErrorKind::NotFound { + return client_not_found(std::slice::from_ref(client)); + } + CliError::new( + "error", + format!("Could not run `{}`: {err}", client.default_binary), + ) +} + +/// The `no_agent_detected`-shaped error: no `--client` was named, and no +/// known client's CLI could be run. `known` is `CLIENTS` unless a specific +/// client was asked for and wasn't there, in which case it names just that +/// one — a different, unrelated case from "nothing was named at all," the +/// same asymmetry `skill_dest::resolve` draws for `--agent`. +fn client_not_found(known: &[Client]) -> CliError { + let flags: Vec<&str> = known.iter().map(|c| c.flag).collect(); + CliError::new( + "mcp_client_not_found", + format!( + "No supported coding-agent CLI was found: {}.", + flags.join(", ") + ), + ) + .with_remedy(Remedy::default().with_fix(&format!( + "Install one of these CLIs, or pass --client to name one anyway ({}).", + flags.join(", ") + ))) +} + +pub fn run(matches: &ArgMatches, mode: Mode) -> Result<()> { + let (action, action_matches) = matches + .subcommand() + .expect("`mcp` sets subcommand_required(true)"); + + match action { + LIST => list(mode), + INSTALL => install(action_matches, mode), + _ => unreachable!("`mcp` declares only two subcommands"), + } +} + +fn wanted_servers(matches: &ArgMatches) -> Vec<&'static Server> { + let requested: Vec<&str> = matches + .get_many::(SERVER_ARG) + .into_iter() + .flatten() + .map(String::as_str) + .collect(); + if requested.is_empty() { + return SERVERS.iter().collect(); + } + SERVERS + .iter() + .filter(|s| requested.contains(&s.flag)) + .collect() +} + +fn wanted_clients(matches: &ArgMatches) -> Result> { + let requested: Vec<&str> = matches + .get_many::(CLIENT_ARG) + .into_iter() + .flatten() + .map(String::as_str) + .collect(); + + if !requested.is_empty() { + // Clap already refused anything not a known flag, so every one of + // these resolves. + return Ok(CLIENTS + .iter() + .filter(|c| requested.contains(&c.flag)) + .collect()); + } + + let detected: Vec<&'static Client> = CLIENTS.iter().filter(|c| client_reachable(c)).collect(); + + if detected.is_empty() { + return Err(client_not_found(CLIENTS).into()); + } + Ok(detected) +} + +/// Every status either `list` or `install` can report, in both the text a +/// person reads and the JSON value a script matches on. A fixed table +/// rather than deriving one rendering from the other: that was the actual +/// bug behind `installed__login_incomplete` (two underscores) — replacing +/// each space *and* comma in "installed, login incomplete" one character at +/// a time hits both the comma and the space that follows it. Also what +/// keeps `list` and `install` naming the same state the same way, since +/// both read `.json()` off the one list here instead of building their own +/// spelling. +enum Status { + Installed, + NotInstalled, + AlreadyInstalled, + WouldInstall, + InstalledLoginIncomplete, + Failed, + ClientNotFound, + ConfigUnreadable, +} + +impl Status { + fn text(&self) -> &'static str { + match self { + Self::Installed => "installed", + Self::NotInstalled => "not installed", + Self::AlreadyInstalled => "already installed", + Self::WouldInstall => "would install", + Self::InstalledLoginIncomplete => "installed, login incomplete", + Self::Failed => "failed", + Self::ClientNotFound => "client not found", + Self::ConfigUnreadable => "config unreadable", + } + } + + fn json(&self) -> &'static str { + match self { + Self::Installed => "installed", + Self::NotInstalled => "not_installed", + Self::AlreadyInstalled => "already_installed", + Self::WouldInstall => "would_install", + Self::InstalledLoginIncomplete => "installed_login_incomplete", + Self::Failed => "failed", + Self::ClientNotFound => "client_not_found", + Self::ConfigUnreadable => "config_unreadable", + } + } +} + +fn status_for(outcome: &GetOutcome) -> Status { + match outcome { + GetOutcome::Installed => Status::Installed, + GetOutcome::NotInstalled => Status::NotInstalled, + GetOutcome::ClientNotFound => Status::ClientNotFound, + GetOutcome::Unreadable => Status::ConfigUnreadable, + } +} + +fn list(mode: Mode) -> Result<()> { + let mut lines = Vec::new(); + let mut rows = Vec::new(); + + for client in CLIENTS { + let reachable = client_reachable(client); + for server in SERVERS { + let status = if !reachable { + Status::ClientNotFound + } else { + status_for(&client_get(client, server.flag)) + }; + lines.push(format!( + "{:14} {:10} {}", + server.flag, + client.flag, + status.text() + )); + rows.push(json!({ + "server": server.flag, + "client": client.flag, + "status": status.json(), + })); + } + } + + output::emit(mode, &lines.join("\n"), json!({ "servers": rows })) +} + +fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { + let servers = wanted_servers(matches); + let clients = wanted_clients(matches)?; + let global = matches.get_flag(GLOBAL_ARG); + let dry_run = executor::wants_dry_run(matches); + + let mut lines = Vec::new(); + let mut results = Vec::new(); + let mut any_failed = false; + + for client in &clients { + for server in &servers { + let outcome = client_get(client, server.flag); + let (status, error) = match outcome { + GetOutcome::Installed => (Status::AlreadyInstalled, None), + GetOutcome::ClientNotFound => { + lines.push(format!( + "{}: {} is not on PATH, skipped.", + server.label, client.label + )); + results.push(json!({ + "server": server.flag, + "client": client.flag, + "status": Status::ClientNotFound.json(), + })); + continue; + } + GetOutcome::Unreadable => { + lines.push(format!( + "{}: {}'s config could not be read, skipped.", + server.label, client.label + )); + results.push(json!({ + "server": server.flag, + "client": client.flag, + "status": Status::ConfigUnreadable.json(), + })); + continue; + } + GetOutcome::NotInstalled if dry_run => (Status::WouldInstall, None), + GetOutcome::NotInstalled => match client_add(client, server, global) { + Ok(AddOutcome::Installed) => (Status::Installed, None), + Ok(AddOutcome::InstalledLoginIncomplete(detail)) => { + (Status::InstalledLoginIncomplete, Some(detail)) + } + Err(e) => { + any_failed = true; + (Status::Failed, Some(e.to_string())) + } + }, + }; + + lines.push(format!( + "{}: {} for {} — {}.", + server.label, + server.url, + client.label, + status.text() + )); + let mut row = json!({ + "server": server.flag, + "client": client.flag, + "status": status.json(), + }); + if let Some(message) = error { + row["error"] = json!(message); + } + results.push(row); + } + } + + output::emit(mode, &lines.join("\n"), json!({ "results": results }))?; + + // The structured result above already names which pair failed and why + // — this only decides the exit code, for a caller that checks that and + // nothing else. Skipped pairs (client not found, config unreadable) + // don't count: nothing was attempted there to call a failure. + if any_failed { + return Err(CliError::new( + "error", + "At least one server could not be registered; see the results above.", + ) + .into()); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_server_has_a_distinct_flag() { + let flags: std::collections::BTreeSet<&str> = SERVERS.iter().map(|s| s.flag).collect(); + assert_eq!(flags.len(), SERVERS.len()); + } + + #[test] + fn every_client_has_a_distinct_flag() { + let flags: std::collections::BTreeSet<&str> = CLIENTS.iter().map(|c| c.flag).collect(); + assert_eq!(flags.len(), CLIENTS.len()); + } + + #[test] + fn every_verb_client_has_an_add_arm() { + for client in CLIENTS.iter().filter(|c| c.kind == ClientKind::Verb) { + assert!( + ["claude-code", "codex"].contains(&client.flag), + "{} is a Verb client with no arm in verb_add's match", + client.flag + ); + } + } + + #[test] + fn every_add_mcp_flag_client_names_its_config_file() { + for client in CLIENTS.iter().filter(|c| c.kind == ClientKind::AddMcpFlag) { + assert!( + client.add_mcp.is_some(), + "{} is an AddMcpFlag client with no AddMcpConfig", + client.flag + ); + } + } + + #[test] + fn the_command_line_parses() { + command() + .try_get_matches_from([ + "mcp", + "install", + "--server", + "mapbox", + "--client", + "claude-code", + ]) + .expect("parses"); + command() + .try_get_matches_from(["mcp", "list"]) + .expect("parses"); + } + + /// The actual bug: deriving JSON from the display text by replacing + /// each space and comma one at a time turned "installed, login + /// incomplete" into "installed__login_incomplete" (two underscores, + /// one per character replaced, not one per word boundary). A fixed + /// table per status can't make that mistake. + #[test] + fn every_json_status_is_single_underscore_snake_case() { + for status in [ + Status::Installed, + Status::NotInstalled, + Status::AlreadyInstalled, + Status::WouldInstall, + Status::InstalledLoginIncomplete, + Status::Failed, + Status::ClientNotFound, + Status::ConfigUnreadable, + ] { + let json = status.json(); + assert!(!json.contains("__"), "{json:?} has a double underscore"); + assert!(!json.contains(' '), "{json:?} has a space"); + assert!(!json.contains(','), "{json:?} has a comma"); + assert_eq!(json, json.to_lowercase(), "{json:?} is not lowercase"); + } + assert_eq!( + Status::InstalledLoginIncomplete.json(), + "installed_login_incomplete" + ); + } + + /// `list` and `install` share one status table now, rather than each + /// spelling the same state differently in JSON (`"not installed"` with + /// a space from one, `"client_not_found"` snake_case from the other). + #[test] + fn list_and_install_json_statuses_agree_for_shared_states() { + assert_eq!( + status_for(&GetOutcome::ClientNotFound).json(), + Status::ClientNotFound.json() + ); + assert_eq!( + status_for(&GetOutcome::Unreadable).json(), + Status::ConfigUnreadable.json() + ); + assert_eq!( + status_for(&GetOutcome::Installed).json(), + Status::Installed.json() + ); + } + + #[test] + fn a_bare_name_tries_cmd_bat_and_exe_only_on_windows() { + let candidates = command_candidates(Path::new("code")); + if cfg!(windows) { + let exts: Vec<_> = candidates + .iter() + .skip(1) + .map(|p| p.extension().and_then(|e| e.to_str()).unwrap_or("")) + .collect(); + assert_eq!(exts, ["cmd", "bat", "exe"]); + assert_eq!(candidates[0], Path::new("code")); + } else { + assert_eq!(candidates, vec![PathBuf::from("code")]); + } + } + + #[test] + fn a_name_that_already_has_an_extension_is_tried_once() { + // Windows or not: a caller that named `.exe` explicitly meant it, + // and guessing further extensions on top would be wrong either way. + assert_eq!( + command_candidates(Path::new("code.exe")), + vec![PathBuf::from("code.exe")] + ); + } +} diff --git a/src/schema.rs b/src/schema.rs index de31d92..2d6d9b8 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -363,6 +363,10 @@ fn commands(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec` and `mcp +/// add ...`, each controlled by an environment variable so one stub script +/// covers every scenario below rather than one per test. +/// +/// `STUB_GET_EXIT` (default 1, "not installed") is what `mcp get` exits +/// with. `STUB_ADD_EXIT` (default 0) is what `mcp add` exits with, and it +/// also writes `add-called` to a marker file so a test can assert `add` was +/// never reached at all (the `--dry-run` and already-installed cases). +const STUB: &str = r#"#!/bin/sh +case "$1" in + --version) + echo "stub-claude 0.0.0" + exit 0 + ;; + mcp) + case "$2" in + get) + echo "get-called:$3" + exit "${STUB_GET_EXIT:-1}" + ;; + add) + echo "add-called:$*" >>"$STUB_MARKER" + if [ "${STUB_ADD_EXIT:-0}" != "0" ]; then + echo "some failure detail" >&2 + fi + exit "${STUB_ADD_EXIT:-0}" + ;; + esac + ;; +esac +exit 1 +"#; + +/// Writes the stub and a fresh, empty marker file under this test target's +/// temp dir, named per test so parallel tests never share either. +fn stub_for(test: &str) -> (PathBuf, PathBuf) { + let stub = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("claude-stub-{test}")); + std::fs::write(&stub, STUB).expect("write stub"); + std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)).expect("chmod stub"); + + let marker = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("claude-marker-{test}")); + std::fs::write(&marker, "").expect("write marker"); + + (stub, marker) +} + +fn marker_was_written(marker: &Path) -> bool { + std::fs::read_to_string(marker) + .map(|s| !s.is_empty()) + .unwrap_or(false) +} + +/// Every client's binary-override variable, each defaulted to a path that +/// doesn't exist. Applied first in every test's `Command`, so a real +/// `code`/`cursor`/`codex`/`claude` installed on the machine running this +/// suite never leaks into a scenario that isn't testing it — a test then +/// overrides only the one it cares about. +fn isolate_every_client(cmd: &mut Command) -> &mut Command { + cmd.env("MAPBOX_CLAUDE_CLI", "/nonexistent/claude") + .env("MAPBOX_CODEX_CLI", "/nonexistent/codex") + .env("MAPBOX_CODE_CLI", "/nonexistent/code") + .env("MAPBOX_CURSOR_CLI", "/nonexistent/cursor") +} + +/// Runs `mapbox mcp ` with `MAPBOX_CLAUDE_CLI` pointed at the stub. +fn run(stub: &Path, marker: &Path, get_exit: &str, add_exit: &str, args: &[&str]) -> Output { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + isolate_every_client(&mut cmd); + cmd.arg("mcp") + .args(args) + .env("MAPBOX_CLAUDE_CLI", stub) + .env("STUB_MARKER", marker) + .env("STUB_GET_EXIT", get_exit) + .env("STUB_ADD_EXIT", add_exit) + .output() + .expect("run the mapbox binary") +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +#[test] +fn a_fresh_server_is_installed() { + let (stub, marker) = stub_for("fresh"); + let output = run( + &stub, + &marker, + "1", // mcp get: not installed + "0", // mcp add: succeeds + &[ + "install", + "--server", + "mapbox", + "--client", + "claude-code", + "-o", + "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!(marker_was_written(&marker), "add was never called"); + let stdout = stdout(&output); + assert!(stdout.contains("installed"), "{stdout:?}"); + assert!(!stdout.contains("already installed"), "{stdout:?}"); +} + +#[test] +fn an_already_installed_server_is_left_alone() { + let (stub, marker) = stub_for("already-installed"); + let output = run( + &stub, + &marker, + "0", // mcp get: already there + "0", + &[ + "install", + "--server", + "mapbox", + "--client", + "claude-code", + "-o", + "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + !marker_was_written(&marker), + "add was called for a server that was already installed" + ); + assert!(stdout(&output).contains("already installed")); +} + +#[test] +fn dry_run_never_calls_add() { + let (stub, marker) = stub_for("dry-run"); + let output = run( + &stub, + &marker, + "1", // not installed + "0", + &[ + "install", + "--server", + "mapbox", + "--client", + "claude-code", + "--dry-run", + "-o", + "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!(!marker_was_written(&marker), "--dry-run ran add anyway"); + assert!(stdout(&output).contains("would install")); +} + +#[test] +fn a_failed_add_is_reported_without_failing_the_whole_run() { + let (stub, marker) = stub_for("add-fails"); + let output = run( + &stub, + &marker, + "1", // not installed + "1", // add fails + &[ + "install", + "--server", + "mapbox", + "--client", + "claude-code", + "-o", + "text", + ], + ); + // A failed add makes the whole run exit non-zero (for a caller that + // only checks the exit code), but the result is still reported in full + // rather than cut short. + assert_eq!(output.status.code(), Some(1)); + assert!(marker_was_written(&marker)); + assert!(stdout(&output).contains("failed"), "{}", stdout(&output)); +} + +#[test] +fn global_passes_scope_user_to_add() { + let (stub, marker) = stub_for("global"); + let output = run( + &stub, + &marker, + "1", + "0", + &[ + "install", + "--server", + "mapbox", + "--client", + "claude-code", + "--global", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + let logged = std::fs::read_to_string(&marker).expect("read marker"); + assert!(logged.contains("--scope user"), "{logged:?}"); +} + +#[test] +fn no_global_omits_scope_and_lets_claude_default_to_local() { + let (stub, marker) = stub_for("no-global"); + let output = run( + &stub, + &marker, + "1", + "0", + &["install", "--server", "mapbox", "--client", "claude-code"], + ); + assert!(output.status.success(), "{}", stderr(&output)); + let logged = std::fs::read_to_string(&marker).expect("read marker"); + assert!(!logged.contains("--scope"), "{logged:?}"); +} + +#[test] +fn a_client_not_on_path_is_skipped_and_named() { + // Every client's binary override points at nothing at all, so + // auto-detection finds none of them reachable. + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + isolate_every_client(&mut cmd); + let output = cmd + .args(["mcp", "install"]) + .output() + .expect("run the mapbox binary"); + assert_eq!(output.status.code(), Some(1)); + let stderr = stderr(&output); + assert!(stderr.contains("mcp_client_not_found") || stderr.contains("No supported")); +} + +#[test] +fn list_reports_status_for_every_known_server_and_client() { + let (stub, marker) = stub_for("list"); + let output = run( + &stub, + &marker, + "0", // installed + "0", + &["list", "-o", "json"], + ); + assert!(output.status.success(), "{}", stderr(&output)); + let stdout = stdout(&output); + assert!(stdout.contains("\"server\":\"mapbox\""), "{stdout:?}"); + assert!( + stdout.contains("\"server\":\"mapbox-devkit\""), + "{stdout:?}" + ); + assert!(stdout.contains("\"status\":\"installed\""), "{stdout:?}"); +} + +// --- Codex: same `mcp get`/`mcp add` shape as Claude Code, but `add` takes +// its name and URL differently, doesn't refuse a duplicate, and (for a +// server that advertises OAuth support) starts a login flow whose own exit +// code answers a different question than "was this written" — verified +// live against the real Mapbox hosted endpoint, see `src/mcp.rs`'s module +// docs. The stub below models exactly that: `add` always writes to +// `STUB_STATE` (unless told not to, for the one scenario that means nothing +// was written at all) regardless of what `STUB_CODEX_ADD_EXIT` says, and +// `get` answers from that state rather than a fixed exit code — because the +// real bug is precisely that those two are decoupled. + +const CODEX_STUB: &str = r#"#!/bin/sh +case "$1" in + --version) + echo "stub-codex 0.0.0" + exit 0 + ;; + mcp) + case "$2" in + get) + if [ -s "$STUB_STATE" ]; then exit 0; else exit 1; fi + ;; + add) + echo "add-called:$*" >>"$STUB_MARKER" + echo "codex-stub-stderr-line" >&2 + if [ "${STUB_CODEX_WRITES:-1}" = "1" ]; then + echo installed >"$STUB_STATE" + fi + exit "${STUB_CODEX_ADD_EXIT:-0}" + ;; + esac + ;; +esac +exit 1 +"#; + +/// The Codex stub, its marker and its state file, each fresh and named per +/// test. +fn codex_stub_for(test: &str) -> (PathBuf, PathBuf, PathBuf) { + let stub = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("codex-stub-{test}")); + std::fs::write(&stub, CODEX_STUB).expect("write stub"); + std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)).expect("chmod stub"); + + let marker = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("codex-marker-{test}")); + std::fs::write(&marker, "").expect("write marker"); + + let state = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("codex-state-{test}")); + let _ = std::fs::remove_file(&state); + + (stub, marker, state) +} + +fn run_codex( + stub: &Path, + marker: &Path, + state: &Path, + add_exit: &str, + writes: &str, + args: &[&str], +) -> Output { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + isolate_every_client(&mut cmd); + cmd.arg("mcp") + .args(args) + .env("MAPBOX_CODEX_CLI", stub) + .env("STUB_MARKER", marker) + .env("STUB_STATE", state) + .env("STUB_CODEX_ADD_EXIT", add_exit) + .env("STUB_CODEX_WRITES", writes) + .output() + .expect("run the mapbox binary") +} + +#[test] +fn codex_fresh_install_succeeds_cleanly() { + let (stub, marker, state) = codex_stub_for("fresh"); + let output = run_codex( + &stub, + &marker, + &state, + "0", + "1", + &[ + "install", "--server", "mapbox", "--client", "codex", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!(marker_was_written(&marker)); + let stdout = stdout(&output); + assert!(stdout.contains("installed"), "{stdout:?}"); + assert!(!stdout.contains("login incomplete"), "{stdout:?}"); +} + +/// The real bug this whole design works around: `add` exits non-zero +/// because its own OAuth step failed, but the config entry is written +/// regardless. This must be reported as installed-with-a-caveat, not as a +/// flat failure. +#[test] +fn codex_add_that_fails_oauth_but_writes_the_entry_is_not_reported_as_failed() { + let (stub, marker, state) = codex_stub_for("oauth-fails"); + let output = run_codex( + &stub, + &marker, + &state, + "1", // add's own exit code: OAuth step failed + "1", // ...but it still wrote the entry + &[ + "install", "--server", "mapbox", "--client", "codex", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!(marker_was_written(&marker)); + let stdout = stdout(&output); + assert!(stdout.contains("login incomplete"), "{stdout:?}"); + assert!(!stdout.contains(": failed"), "{stdout:?}"); +} + +/// The other half of that bug: when `add` really did fail and nothing was +/// written, this must still report a genuine failure rather than assuming +/// success just because the exit code alone can't be trusted here. +#[test] +fn codex_add_that_writes_nothing_is_reported_as_failed() { + let (stub, marker, state) = codex_stub_for("writes-nothing"); + let output = run_codex( + &stub, + &marker, + &state, + "1", + "0", // nothing was actually written this time + &[ + "install", "--server", "mapbox", "--client", "codex", "-o", "text", + ], + ); + // A failed add makes the whole run exit non-zero (for a caller that + // only checks the exit code), but the result is still reported in full + // rather than cut short. + assert_eq!(output.status.code(), Some(1)); + assert!(marker_was_written(&marker)); + assert!(stdout(&output).contains("failed"), "{}", stdout(&output)); +} + +#[test] +fn codex_already_installed_is_left_alone() { + let (stub, marker, state) = codex_stub_for("already-installed"); + std::fs::write(&state, "installed").expect("seed state"); + let output = run_codex( + &stub, + &marker, + &state, + "0", + "1", + &[ + "install", "--server", "mapbox", "--client", "codex", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!(!marker_was_written(&marker), "add was called anyway"); + assert!(stdout(&output).contains("already installed")); +} + +// --- VS Code / Cursor: no `mcp` subcommand at all, just a top-level +// `--add-mcp ''` flag that neither refuses a duplicate name nor +// offers a `get`/`list` to check one — so this command reads the config +// file directly instead of ever writing it, and each editor keeps that +// file somewhere genuinely different (`src/mcp.rs`'s `AddMcpConfig`, +// confirmed live for each). The stub only needs to answer `--version` and +// `--add-mcp`; the "already installed" case is set up by writing the real +// config file shape directly, the same file `file_get` reads, rather than +// teaching the stub to simulate a merge. + +const ADD_MCP_FLAG_STUB: &str = r#"#!/bin/sh +case "$1" in + --version) + echo "stub-editor 0.0.0" + exit 0 + ;; + --add-mcp) + echo "add-mcp-called:$2" >>"$STUB_MARKER" + exit "${STUB_ADD_EXIT:-0}" + ;; +esac +exit 1 +"#; + +fn add_mcp_flag_stub_for(test: &str) -> (PathBuf, PathBuf) { + let stub = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("editor-stub-{test}")); + std::fs::write(&stub, ADD_MCP_FLAG_STUB).expect("write stub"); + std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)).expect("chmod stub"); + + let marker = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!("editor-marker-{test}")); + std::fs::write(&marker, "").expect("write marker"); + + (stub, marker) +} + +fn run_vscode( + stub: &Path, + marker: &Path, + config_dir: &Path, + add_exit: &str, + args: &[&str], +) -> Output { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + isolate_every_client(&mut cmd); + cmd.arg("mcp") + .args(args) + .env("MAPBOX_CODE_CLI", stub) + .env("MAPBOX_CODE_CONFIG_DIR", config_dir) + .env("STUB_MARKER", marker) + .env("STUB_ADD_EXIT", add_exit) + .output() + .expect("run the mapbox binary") +} + +#[test] +fn vscode_fresh_install_calls_add_mcp() { + let (stub, marker) = add_mcp_flag_stub_for("vscode-fresh"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("vscode-config-fresh"); + let output = run_vscode( + &stub, + &marker, + &config_dir, + "0", + &[ + "install", "--server", "mapbox", "--client", "vscode", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!(marker_was_written(&marker), "--add-mcp was never called"); + assert!(stdout(&output).contains("installed"), "{}", stdout(&output)); +} + +/// The whole reason this reads the config file first: `--add-mcp` itself +/// would silently overwrite a same-named entry, so this command must never +/// call it for a server that's already there. +#[test] +fn vscode_already_installed_is_never_passed_to_add_mcp() { + let (stub, marker) = add_mcp_flag_stub_for("vscode-already-installed"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("vscode-config-already"); + let user_dir = config_dir.join("User"); + std::fs::create_dir_all(&user_dir).expect("create config dir"); + std::fs::write( + user_dir.join("mcp.json"), + r#"{"servers":{"mapbox":{"type":"http","url":"https://mcp.mapbox.com/mcp"}},"inputs":[]}"#, + ) + .expect("seed mcp.json"); + + let output = run_vscode( + &stub, + &marker, + &config_dir, + "0", + &[ + "install", "--server", "mapbox", "--client", "vscode", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + !marker_was_written(&marker), + "--add-mcp was called for a server already in mcp.json" + ); + assert!(stdout(&output).contains("already installed")); +} + +/// An `mcp.json` that exists but doesn't parse must never be guessed past — +/// see `GetOutcome::Unreadable` in `src/mcp.rs`. +#[test] +fn vscode_unreadable_config_is_reported_rather_than_guessed_past() { + let (stub, marker) = add_mcp_flag_stub_for("vscode-unreadable"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("vscode-config-unreadable"); + let user_dir = config_dir.join("User"); + std::fs::create_dir_all(&user_dir).expect("create config dir"); + std::fs::write(user_dir.join("mcp.json"), "{not valid json").expect("seed mcp.json"); + + let output = run_vscode( + &stub, + &marker, + &config_dir, + "0", + &[ + "install", "--server", "mapbox", "--client", "vscode", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + !marker_was_written(&marker), + "--add-mcp was called despite an unreadable config" + ); + assert!( + stdout(&output).contains("could not be read"), + "{}", + stdout(&output) + ); +} + +/// VS Code's `mcp.json` and Cursor's `settings.json` are both JSONC: real +/// comments and a trailing comma are normal in a file a person has actually +/// looked at, and `serde_json` alone refuses both. +#[test] +fn vscode_config_with_comments_and_a_trailing_comma_still_parses() { + let (stub, marker) = add_mcp_flag_stub_for("vscode-jsonc"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("vscode-config-jsonc"); + let user_dir = config_dir.join("User"); + std::fs::create_dir_all(&user_dir).expect("create config dir"); + std::fs::write( + user_dir.join("mcp.json"), + "{\n // my servers\n \"servers\": {\n \"mapbox\": {\"type\": \"http\", \"url\": \"https://mcp.mapbox.com/mcp\"},\n },\n}\n", + ) + .expect("seed mcp.json"); + + let output = run_vscode( + &stub, + &marker, + &config_dir, + "0", + &[ + "install", "--server", "mapbox", "--client", "vscode", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + !marker_was_written(&marker), + "--add-mcp was called for a server a commented file already lists" + ); + assert!( + stdout(&output).contains("already installed"), + "a JSONC file with comments and a trailing comma was treated as unreadable: {}", + stdout(&output) + ); +} + +/// Only `NotFound` means "not installed." Every other read error (no +/// permission, the path is a directory, not valid UTF-8) must not be +/// guessed past as the same thing — that guess is what let `install` run +/// `--add-mcp` over a server that may already be there under a config this +/// command had never actually managed to read. +#[test] +fn a_config_path_that_is_a_directory_is_unreadable_not_not_installed() { + let (stub, marker) = add_mcp_flag_stub_for("vscode-dir"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("vscode-config-dir"); + // `User/mcp.json` is a directory, not a file — reading it fails with + // something other than `NotFound`. + std::fs::create_dir_all(config_dir.join("User").join("mcp.json")).expect("create dir"); + + let output = run_vscode( + &stub, + &marker, + &config_dir, + "0", + &[ + "install", "--server", "mapbox", "--client", "vscode", "-o", "json", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + !marker_was_written(&marker), + "--add-mcp was called despite mcp.json being unreadable (a directory)" + ); + assert!( + stdout(&output).contains("\"status\":\"config_unreadable\""), + "{}", + stdout(&output) + ); +} + +/// Codex's OAuth step can print a URL to open as part of `add` itself — +/// captured-and-shown-only-on-failure would leave that unseen while the run +/// looks hung. `spawn_live` forwards both of the child's streams to this +/// process's own stderr as they arrive, not only after the child exits. +#[test] +fn codex_add_output_is_forwarded_to_stderr_live() { + let (stub, marker, state) = codex_stub_for("live-output"); + let output = run_codex( + &stub, + &marker, + &state, + "0", + "1", + &[ + "install", "--server", "mapbox", "--client", "codex", "-o", "text", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + stderr(&output).contains("codex-stub-stderr-line"), + "the child's own stderr never reached ours: {}", + stderr(&output) + ); +} + +#[test] +fn vscode_has_no_per_project_scope_and_says_so() { + let (stub, marker) = add_mcp_flag_stub_for("vscode-no-global"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("vscode-config-no-global"); + let output = run_vscode( + &stub, + &marker, + &config_dir, + "0", + &["install", "--server", "mapbox", "--client", "vscode"], + ); + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + stderr(&output).contains("no per-project scope"), + "{}", + stderr(&output) + ); +} + +/// Cursor speaks the identical `--add-mcp` flag but keeps the result +/// somewhere different (`settings.json`'s `"mcp"` key, not a dedicated +/// `mcp.json`) — confirmed live, see `src/mcp.rs`'s module docs. This +/// exercises that nested path rather than retesting `--add-mcp` itself. +#[test] +fn cursor_reads_its_own_settings_json_shape() { + let (stub, marker) = add_mcp_flag_stub_for("cursor-already-installed"); + let config_dir = Path::new(env!("CARGO_TARGET_TMPDIR")).join("cursor-config-already"); + let user_dir = config_dir.join("User"); + std::fs::create_dir_all(&user_dir).expect("create config dir"); + std::fs::write( + user_dir.join("settings.json"), + r#"{"editor.fontSize":14,"mcp":{"servers":{"mapbox":{"type":"http","url":"https://mcp.mapbox.com/mcp"}}}}"#, + ) + .expect("seed settings.json"); + + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + isolate_every_client(&mut cmd); + let output = cmd + .arg("mcp") + .args([ + "install", "--server", "mapbox", "--client", "cursor", "-o", "text", + ]) + .env("MAPBOX_CURSOR_CLI", &stub) + .env("MAPBOX_CURSOR_CONFIG_DIR", &config_dir) + .env("STUB_MARKER", &marker) + .env("STUB_ADD_EXIT", "0") + .output() + .expect("run the mapbox binary"); + + assert!(output.status.success(), "{}", stderr(&output)); + assert!( + !marker_was_written(&marker), + "--add-mcp was called for a server already in settings.json" + ); + assert!(stdout(&output).contains("already installed")); +}