From a73e1f82ed04b19d1621b5bcc6fd42cfd2ab4bed Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 29 Sep 2026 11:20:40 -0400 Subject: [PATCH 1/3] Add mapbox mcp: register a Mapbox MCP server with Claude Code AGI-1150 Phase 2 MVP. Shells out to `claude mcp add`/`claude mcp get` rather than editing an agent's own config file directly, since that file may already list other servers. Supports the hosted Mapbox MCP and DevKit MCP endpoints for Claude Code; other clients and the local npm install path are left as documented follow-up work. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 6 + README.md | 17 ++ docs/commands.md | 142 ++++++++++++++- src/main.rs | 13 ++ src/mcp.rs | 446 +++++++++++++++++++++++++++++++++++++++++++++ src/schema.rs | 4 + tests/mcp_proxy.rs | 262 ++++++++++++++++++++++++++ 7 files changed, 887 insertions(+), 3 deletions(-) create mode 100644 src/mcp.rs create mode 100644 tests/mcp_proxy.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index c707676..aca2d96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,12 @@ 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. Only Claude Code is supported today, + against the hosted Mapbox MCP endpoints. An existing server with the same + name is left alone rather than replaced. + - Diagnostic logs, off by default: `mapbox config set log on` (or `MAPBOX_LOG=1`) keeps, for each run in history, the command line, each request and the error message, tokens redacted, on your machine only. diff --git a/README.md b/README.md index 5867392..e5a8614 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ time from OpenAPI specs, so they always match the specs. - [Agent skills](#agent-skills) - [Shell completion](#shell-completion) - [Generate Skills](#generate-skills) + - [MCP servers](#mcp-servers) - [Tileset CLI](#tileset-cli) - [Usage](#usage) - [Dry runs](#dry-runs) @@ -303,6 +304,22 @@ That removes every copy this command wrote, which is more than deleting the directories by hand usually catches — a default run writes for each agent on the machine, not just the one you had in mind. +### 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* CLI, since its config belongs to that agent and +may already list other servers. Only Claude Code is 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. + ### Tileset CLI ```sh diff --git a/docs/commands.md b/docs/commands.md index b320671..1c8adbc 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) · @@ -3131,6 +3134,139 @@ 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 *agent*, which may already list other servers, so this shells out to the +agent's own CLI (`claude mcp add`) rather than editing that file directly — +the same reasoning [`tilesets-cli`](#tilesets-cli) has for exec-ing +`tilesets` rather than reimplementing it. + +Only Claude Code is supported today, against the hosted Mapbox MCP +endpoints — no token, no npm package, no Node version to manage. Adding a +server is one row in this command's table; adding a client is a new row plus +the two functions that shell out to it, so both are meant to grow here as +more become available. + +An existing server with the same name, at any scope, is left alone rather +than replaced — `claude mcp get ` is checked first, the same +never-overwrite-what-you-didn't-write rule `agent-skills` follows for a +skill directory someone has edited. + +### `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 +``` + + + +```json +{ + "servers": [ + { "server": "mapbox", "client": "claude-code", "status": "not installed" }, + { "server": "mapbox-devkit", "client": "claude-code", "status": "not installed" } + ] +} +``` + +
+ +`client not found` in place of a status is Claude Code's own CLI not being on +`PATH` at all — see [`mcp install`](#mapbox-mcp-install) below for what that +means for installing. + +--- + +### `mapbox mcp install` + +Registers every known server for every detected client. With no client's CLI +on `PATH`, the one request this makes is to check that, and it reports +nothing was done rather than failing the run — the same shape +`generate-skills` gives a machine with no coding agent on it. + +#### Parameters + +| Parameter | Effect | +| --- | --- | +| `--server ` | Repeatable. Defaults to every known server (`mapbox`, `mapbox-devkit`). | +| `--client ` | Repeatable. Defaults to whichever clients are detected. Only `claude-code` exists today. | +| `--global` | Register for every project rather than just this one (`claude mcp add --scope user`). | +| `--dry-run` | Report what would be installed, then exit without installing it. | + +#### Examples + +```sh +mapbox mcp install + +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. +``` + + + +```json +{ + "results": [ + { "server": "mapbox", "client": "claude-code", "status": "installed" }, + { "server": "mapbox-devkit", "client": "claude-code", "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` 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. + +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. +Fix: Install one of these CLIs, or pass --client to name one anyway (claude-code). +``` + +--- + ## Uninstall ### `mapbox uninstall` diff --git a/src/main.rs b/src/main.rs index 49d4ebe..2dc12da 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; @@ -600,6 +601,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 @@ -1086,6 +1094,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..d984d9a --- /dev/null +++ b/src/mcp.rs @@ -0,0 +1,446 @@ +//! `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. Claude Code has its +//! own CLI verb for this instead (`claude mcp add`), so this shells out to +//! it rather than editing `~/.claude.json` by hand — 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 — +//! today, only Claude Code. Adding a server is one row in [`SERVERS`], since +//! [`client_get`]/[`client_add`] are generic over `Client` rather than +//! hardcoded to `claude`. Adding a *client* is only that cheap when the new +//! one happens to speak the same ` mcp get ` / ` mcp +//! add --transport http ` verbs Claude Code does — a client with +//! a genuinely different CLI (or one that needs a config file edited +//! instead of a CLI at all, like Claude Desktop or VS Code, see the module +//! docs' opening paragraph) needs its own pair of functions, not just a row. +//! +//! 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. +//! +//! # One request per (server, client) pair, no state of its own +//! +//! `claude mcp get ` is both the "is this even installed" check and +//! the "is `claude` on `PATH` at all" check in one call: 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, and success means +//! it already is. That's a stronger idempotency check than `claude mcp +//! add`'s own duplicate-add failure, which is plain text +//! ("MCP server NAME already exists in SCOPE config") with no code to match +//! on — checking `get` first also means never spamming stderr with a +//! failure that would otherwise be normal, expected output. + +use std::ffi::OsStr; +use std::io; +use std::path::PathBuf; +use std::process::Command; + +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 + /// (`claude mcp add `). + 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", + }, +]; + +/// 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 `claude` not on `PATH`, + /// or a wrapper script standing in for it. + binary_env: &'static str, + default_binary: &'static str, +} + +const CLIENTS: &[Client] = &[Client { + flag: "claude-code", + label: "Claude Code", + binary_env: "MAPBOX_CLAUDE_CLI", + default_binary: "claude", +}]; + +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), + } + } +} + +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.\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 CLI (`claude mcp add`) rather than editing that file directly.\n\n\ + Only Claude Code is supported today, against the hosted Mapbox MCP endpoints — \ + 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"), + ) + .arg(executor::dry_run_arg( + "Report what would be installed, then exit without installing it", + )), + ) +} + +/// Whether `claude mcp get ` found the server, found nothing, or +/// couldn't be run at all. +enum GetOutcome { + Installed, + NotInstalled, + ClientNotFound, +} + +fn client_get(client: &Client, server_name: &str) -> GetOutcome { + match Command::new(client.binary()) + .args(["mcp", "get", server_name]) + .output() + { + Ok(output) if output.status.success() => GetOutcome::Installed, + Ok(_) => GetOutcome::NotInstalled, + Err(e) if e.kind() == io::ErrorKind::NotFound => GetOutcome::ClientNotFound, + // Some other failure to spawn at all (permissions, an + // interpreter missing for a script wrapper): treated the same as + // "not found," since either way this client can't be driven. + Err(_) => GetOutcome::ClientNotFound, + } +} + +/// 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. +fn client_reachable(client: &Client) -> bool { + Command::new(client.binary()) + .arg("--version") + .output() + .is_ok() +} + +/// `claude mcp add --transport http [--scope user]`. +fn client_add(client: &Client, server: &Server, global: bool) -> Result<()> { + let mut args: Vec<&OsStr> = vec![ + OsStr::new("mcp"), + OsStr::new("add"), + OsStr::new("--transport"), + OsStr::new("http"), + OsStr::new(server.flag), + OsStr::new(server.url), + ]; + if global { + args.push(OsStr::new("--scope")); + args.push(OsStr::new("user")); + } + + let output = Command::new(client.binary()) + .args(&args) + .output() + .map_err(|e| spawn_failed(client, e))?; + + if output.status.success() { + return Ok(()); + } + let detail = String::from_utf8_lossy(&output.stderr); + let detail = if detail.trim().is_empty() { + String::from_utf8_lossy(&output.stdout).into_owned() + } else { + detail.into_owned() + }; + Err(CliError::new( + "error", + format!( + "{} could not register {}: {}", + client.label, + server.label, + detail.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) +} + +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 { + "client not found" + } else { + match client_get(client, server.flag) { + GetOutcome::Installed => "installed", + GetOutcome::NotInstalled | GetOutcome::ClientNotFound => "not installed", + } + }; + lines.push(format!("{:14} {:10} {status}", server.flag, client.flag)); + rows.push(json!({ + "server": server.flag, + "client": client.flag, + "status": status, + })); + } + } + + 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(); + + for client in &clients { + for server in &servers { + let outcome = client_get(client, server.flag); + let (status, error) = match outcome { + GetOutcome::Installed => ("already installed", 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": "client_not_found", + })); + continue; + } + GetOutcome::NotInstalled if dry_run => ("would install", None), + GetOutcome::NotInstalled => match client_add(client, server, global) { + Ok(()) => ("installed", None), + Err(e) => ("failed", Some(e.to_string())), + }, + }; + + lines.push(format!( + "{}: {} for {} — {status}.", + server.label, server.url, client.label + )); + let mut row = json!({ + "server": server.flag, + "client": client.flag, + "status": status.replace(' ', "_"), + }); + if let Some(message) = error { + row["error"] = json!(message); + } + results.push(row); + } + } + + output::emit(mode, &lines.join("\n"), json!({ "results": results })) +} + +#[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 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"); + } +} diff --git a/src/schema.rs b/src/schema.rs index 31a3e8a..f77b0c6 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) +} + +/// 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 { + Command::new(env!("CARGO_BIN_EXE_mapbox")) + .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", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + 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() { + let (_stub, marker) = stub_for("client-not-found"); + // MAPBOX_CLAUDE_CLI points at nothing at all — every client is + // unreachable regardless of --client, the same as a bare `claude` that + // was never installed. + let output = Command::new(env!("CARGO_BIN_EXE_mapbox")) + .args(["mcp", "install"]) + .env("MAPBOX_CLAUDE_CLI", "/nonexistent/claude") + .env("STUB_MARKER", &marker) + .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:?}"); +} From c17ab1ce09bf58194ee980892f930c461bd4527f Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 29 Sep 2026 12:25:21 -0400 Subject: [PATCH 2/3] Extend mapbox mcp to Codex, VS Code and Cursor Codex has its own mcp add/get, close to Claude Code's shape but not identical: different argv, and registering a server that advertises OAuth support starts a login flow as part of add itself. That flow fails against the real Mapbox hosted endpoint (an incompatibility between Codex's OAuth client and this server), but the config entry is written regardless of add's exit code - now reported as "installed, login incomplete" rather than a flat failure. VS Code and Cursor have no mcp subcommand, but both expose a top-level --add-mcp flag. Neither refuses a duplicate name, so this reads each client's own config file directly before ever calling that flag - and where that file lives is genuinely different per client (VS Code: a dedicated mcp.json; Cursor: settings.json under an "mcp" key), verified live rather than assumed. Neither has a working per-project scope via their CLI, so both always register for every project regardless of --global, disclosed rather than silently ignored. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 12 +- README.md | 14 +- docs/commands.md | 92 ++++++--- src/mcp.rs | 450 ++++++++++++++++++++++++++++++++++++++------- tests/mcp_proxy.rs | 377 ++++++++++++++++++++++++++++++++++++- 5 files changed, 837 insertions(+), 108 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aca2d96..579f627 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,9 +21,15 @@ that may never merge. They are not releases and are not listed here. - `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. Only Claude Code is supported today, - against the hosted Mapbox MCP endpoints. An existing server with the same - name is left alone rather than replaced. + 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. - Diagnostic logs, off by default: `mapbox config set log on` (or `MAPBOX_LOG=1`) keeps, for each run in history, the command line, each diff --git a/README.md b/README.md index e5a8614..1abdc45 100644 --- a/README.md +++ b/README.md @@ -314,11 +314,15 @@ 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* CLI, since its config belongs to that agent and -may already list other servers. Only Claude Code is 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. +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. ### Tileset CLI diff --git a/docs/commands.md b/docs/commands.md index 1c8adbc..039bb8a 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3142,21 +3142,45 @@ 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 *agent*, which may already list other servers, so this shells out to the -agent's own CLI (`claude mcp add`) rather than editing that file directly — -the same reasoning [`tilesets-cli`](#tilesets-cli) has for exec-ing -`tilesets` rather than reimplementing it. - -Only Claude Code is supported today, against the hosted Mapbox MCP -endpoints — no token, no npm package, no Node version to manage. Adding a -server is one row in this command's table; adding a client is a new row plus -the two functions that shell out to it, so both are meant to grow here as -more become available. - -An existing server with the same name, at any scope, is left alone rather -than replaced — `claude mcp get ` is checked first, the same +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. +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` @@ -3177,6 +3201,12 @@ mapbox mcp list ``` 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 ``` @@ -3185,7 +3215,13 @@ mapbox-devkit claude-code not installed { "servers": [ { "server": "mapbox", "client": "claude-code", "status": "not installed" }, - { "server": "mapbox-devkit", "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" } ] } ``` @@ -3193,9 +3229,10 @@ mapbox-devkit claude-code not installed -`client not found` in place of a status is Claude Code's own CLI not being on +`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. +means for installing. `config unreadable` means VS Code's or Cursor's own +config file exists but didn't parse. --- @@ -3211,8 +3248,8 @@ nothing was done rather than failing the run — the same shape | Parameter | Effect | | --- | --- | | `--server ` | Repeatable. Defaults to every known server (`mapbox`, `mapbox-devkit`). | -| `--client ` | Repeatable. Defaults to whichever clients are detected. Only `claude-code` exists today. | -| `--global` | Register for every project rather than just this one (`claude mcp add --scope user`). | +| `--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 @@ -3220,6 +3257,8 @@ nothing was done rather than failing the run — the same shape ```sh mapbox mcp install +mapbox mcp install --server mapbox --client vscode + mapbox mcp install --server mapbox --global mapbox mcp install --dry-run @@ -3234,6 +3273,8 @@ mapbox mcp install --dry-run ``` 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. ``` @@ -3242,7 +3283,9 @@ Mapbox DevKit MCP: https://mcp-devkit.mapbox.com/mcp for Claude Code — install { "results": [ { "server": "mapbox", "client": "claude-code", "status": "installed" }, - { "server": "mapbox-devkit", "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" } ] } ``` @@ -3254,15 +3297,18 @@ 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` 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. +(`"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. 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. -Fix: Install one of these CLIs, or pass --client to name one anyway (claude-code). +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). ``` --- diff --git a/src/mcp.rs b/src/mcp.rs index d984d9a..6f3cee8 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -4,25 +4,61 @@ //! [`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. Claude Code has its -//! own CLI verb for this instead (`claude mcp add`), so this shells out to -//! it rather than editing `~/.claude.json` by hand — the same reasoning -//! [`crate::tilesets_cli`] has for exec-ing `tilesets` rather than -//! reimplementing it. +//! 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 — -//! today, only Claude Code. Adding a server is one row in [`SERVERS`], since -//! [`client_get`]/[`client_add`] are generic over `Client` rather than -//! hardcoded to `claude`. Adding a *client* is only that cheap when the new -//! one happens to speak the same ` mcp get ` / ` mcp -//! add --transport http ` verbs Claude Code does — a client with -//! a genuinely different CLI (or one that needs a config file edited -//! instead of a CLI at all, like Claude Desktop or VS Code, see the module -//! docs' opening paragraph) needs its own pair of functions, not just a row. +//! [`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 @@ -30,19 +66,20 @@ //! 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. //! -//! # One request per (server, client) pair, no state of its own +//! # Checking first //! -//! `claude mcp get ` is both the "is this even installed" check and -//! the "is `claude` on `PATH` at all" check in one call: 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, and success means -//! it already is. That's a stronger idempotency check than `claude mcp -//! add`'s own duplicate-add failure, which is plain text -//! ("MCP server NAME already exists in SCOPE config") with no code to match -//! on — checking `get` first also means never spamming stderr with a -//! failure that would otherwise be normal, expected output. - -use std::ffi::OsStr; +//! 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; use std::path::PathBuf; use std::process::Command; @@ -67,8 +104,7 @@ 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 - /// (`claude mcp add `). + /// `--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, @@ -90,24 +126,100 @@ const SERVERS: &[Server] = &[ }, ]; +/// 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 `claude` not on `PATH`, - /// or a wrapper script standing in for it. + /// `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, } -const CLIENTS: &[Client] = &[Client { - flag: "claude-code", - label: "Claude Code", - binary_env: "MAPBOX_CLAUDE_CLI", - default_binary: "claude", -}]; +/// 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, @@ -129,14 +241,15 @@ pub fn command() -> ClapCommand { .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.\n\n\ + 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 CLI (`claude mcp add`) rather than editing that file directly.\n\n\ - Only Claude Code is supported today, against the hosted Mapbox MCP endpoints — \ - no token, no npm package, no Node version to manage. `mapbox mcp list` names \ - every server and client this command knows about.", + 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( @@ -165,7 +278,10 @@ pub fn command() -> ClapCommand { Arg::new(GLOBAL_ARG) .long(GLOBAL_ARG) .action(ArgAction::SetTrue) - .help("Register for every project rather than this one"), + .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", @@ -173,15 +289,26 @@ pub fn command() -> ClapCommand { ) } -/// Whether `claude mcp get ` found the server, found nothing, or -/// couldn't be run at all. +/// 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 Command::new(client.binary()) .args(["mcp", "get", server_name]) .output() @@ -196,11 +323,72 @@ fn client_get(client: &Client, server_name: &str) -> GetOutcome { } } +/// `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 won'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. +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, + Err(_) => return GetOutcome::NotInstalled, + }; + match serde_json::from_str::(&text) { + Ok(value) => { + 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 + } + } + Err(_) => GetOutcome::Unreadable, + } +} + +/// 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. +/// 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 { Command::new(client.binary()) .arg("--version") @@ -208,36 +396,119 @@ fn client_reachable(client: &Client) -> bool { .is_ok() } -/// `claude mcp add --transport http [--scope user]`. -fn client_add(client: &Client, server: &Server, global: bool) -> Result<()> { - let mut args: Vec<&OsStr> = vec![ - OsStr::new("mcp"), - OsStr::new("add"), - OsStr::new("--transport"), - OsStr::new("http"), - OsStr::new(server.flag), - OsStr::new(server.url), - ]; - if global { - args.push(OsStr::new("--scope")); - args.push(OsStr::new("user")); +/// 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 output = Command::new(client.binary()) + .args(&args) + .output() + .map_err(|e| spawn_failed(client, e))?; + if output.status.success() { + Ok(AddOutcome::Installed) + } else { + Err(add_failed(client, server, &output)) + } + } + "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 output = Command::new(client.binary()) + .args(["mcp", "add", server.flag, "--url", server.url]) + .output() + .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. + let written = matches!(verb_get(client, server.flag), GetOutcome::Installed); + if !written { + return Err(add_failed(client, server, &output)); + } + if output.status.success() { + Ok(AddOutcome::Installed) + } else { + let detail = String::from_utf8_lossy(&output.stderr); + Ok(AddOutcome::InstalledLoginIncomplete( + detail.trim().to_string(), + )) + } + } + _ => unreachable!("every `Verb` client has an arm here"), + } +} + +/// ` --add-mcp ''`. `global` is accepted for symmetry with +/// `verb_add` but unused: VS Code's `--add-mcp` has no per-project scope at +/// all (checked directly in `code --help` — no such flag exists), 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 output = Command::new(client.binary()) - .args(&args) + .arg("--add-mcp") + .arg(&payload) .output() .map_err(|e| spawn_failed(client, e))?; if output.status.success() { - return Ok(()); + Ok(AddOutcome::Installed) + } else { + Err(add_failed(client, server, &output)) } +} + +fn add_failed(client: &Client, server: &Server, output: &std::process::Output) -> anyhow::Error { let detail = String::from_utf8_lossy(&output.stderr); let detail = if detail.trim().is_empty() { String::from_utf8_lossy(&output.stdout).into_owned() } else { detail.into_owned() }; - Err(CliError::new( + CliError::new( "error", format!( "{} could not register {}: {}", @@ -246,7 +517,7 @@ fn client_add(client: &Client, server: &Server, global: bool) -> Result<()> { detail.trim() ), ) - .into()) + .into() } fn spawn_failed(client: &Client, err: io::Error) -> CliError { @@ -332,6 +603,15 @@ fn wanted_clients(matches: &ArgMatches) -> Result> { Ok(detected) } +fn status_text(outcome: &GetOutcome) -> &'static str { + match outcome { + GetOutcome::Installed => "installed", + GetOutcome::NotInstalled => "not installed", + GetOutcome::ClientNotFound => "not installed", + GetOutcome::Unreadable => "config unreadable", + } +} + fn list(mode: Mode) -> Result<()> { let mut lines = Vec::new(); let mut rows = Vec::new(); @@ -342,10 +622,7 @@ fn list(mode: Mode) -> Result<()> { let status = if !reachable { "client not found" } else { - match client_get(client, server.flag) { - GetOutcome::Installed => "installed", - GetOutcome::NotInstalled | GetOutcome::ClientNotFound => "not installed", - } + status_text(&client_get(client, server.flag)) }; lines.push(format!("{:14} {:10} {status}", server.flag, client.flag)); rows.push(json!({ @@ -385,9 +662,24 @@ fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { })); 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": "config_unreadable", + })); + continue; + } GetOutcome::NotInstalled if dry_run => ("would install", None), GetOutcome::NotInstalled => match client_add(client, server, global) { - Ok(()) => ("installed", None), + Ok(AddOutcome::Installed) => ("installed", None), + Ok(AddOutcome::InstalledLoginIncomplete(detail)) => { + ("installed, login incomplete", Some(detail)) + } Err(e) => ("failed", Some(e.to_string())), }, }; @@ -399,7 +691,7 @@ fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { let mut row = json!({ "server": server.flag, "client": client.flag, - "status": status.replace(' ', "_"), + "status": status.replace([' ', ','], "_"), }); if let Some(message) = error { row["error"] = json!(message); @@ -427,6 +719,28 @@ mod tests { 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() diff --git a/tests/mcp_proxy.rs b/tests/mcp_proxy.rs index ee94c7f..cbdc81a 100644 --- a/tests/mcp_proxy.rs +++ b/tests/mcp_proxy.rs @@ -68,10 +68,23 @@ fn marker_was_written(marker: &Path) -> bool { .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 { - Command::new(env!("CARGO_BIN_EXE_mapbox")) - .arg("mcp") + 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) @@ -226,14 +239,12 @@ fn no_global_omits_scope_and_lets_claude_default_to_local() { #[test] fn a_client_not_on_path_is_skipped_and_named() { - let (_stub, marker) = stub_for("client-not-found"); - // MAPBOX_CLAUDE_CLI points at nothing at all — every client is - // unreachable regardless of --client, the same as a bare `claude` that - // was never installed. - let output = Command::new(env!("CARGO_BIN_EXE_mapbox")) + // 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"]) - .env("MAPBOX_CLAUDE_CLI", "/nonexistent/claude") - .env("STUB_MARKER", &marker) .output() .expect("run the mapbox binary"); assert_eq!(output.status.code(), Some(1)); @@ -260,3 +271,351 @@ fn list_reports_status_for_every_known_server_and_client() { ); 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" + 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", + ], + ); + assert!(output.status.success(), "{}", stderr(&output)); + 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) + ); +} + +#[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")); +} From ae11dc3088c938789633e7a3e654bc1bc8eb5f45 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Thu, 1 Oct 2026 10:15:25 -0400 Subject: [PATCH 3/3] Fix 7 bugs Mofei found reviewing mapbox mcp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - JSON status values are now a fixed snake_case table (Status::json()), not derived from the display text one character at a time — that was the actual bug behind "installed__login_incomplete" (two underscores, one per character replaced in the comma-and-space in "installed, login incomplete"). list and install now share the same table, so the same state spells the same way in both. - VS Code's/Cursor's config files are parsed as JSONC (jsonc-parser crate), not plain JSON. Both allow comments and a trailing comma, so an ordinary, already-edited file was misread as "config unreadable" before this. - Only io::ErrorKind::NotFound means "not installed" now. Every other read error (no permission, the path is a directory, invalid UTF-8) is "config unreadable" instead of silently falling through to an install attempt over a file this command had never actually read. - Every spawn now tries .cmd/.bat/.exe in turn on Windows for a bare name with no extension of its own (command_candidates/spawn/ spawn_live) — CreateProcessW does not probe PATHEXT the way cmd.exe does, so an npm-installed CLI (or VS Code's/Cursor's own launcher, both .cmd there) would otherwise read as "not found". Not verified live, no Windows machine here, but the gap and the fix are both well-documented Rust/Windows behavior. - Codex's add is now run via spawn_live, which forwards both of the child's streams to this process's own stderr as they arrive (one on a second thread, so neither pipe can block behind the other filling up) rather than only capturing them — an OAuth URL it prints as part of add was previously never shown. - install now exits 1 once a registration actually fails, after reporting the full result — a skipped pair (client not found, config unreadable) doesn't count, only a real attempted-and-failed one does. Previously this always exited 0 regardless. - Fixed documentation that said a totally unreachable client makes install report nothing was done; it's actually an error, exit 1, which is what the code already did. Also merged main in (CHANGELOG.md/README.md conflicts, additive). Co-Authored-By: Claude Sonnet 5 --- Cargo.lock | 13 +- Cargo.toml | 8 + docs/commands.md | 43 +++-- src/mcp.rs | 413 +++++++++++++++++++++++++++++++++++---------- tests/mcp_proxy.rs | 106 +++++++++++- 5 files changed, 477 insertions(+), 106 deletions(-) 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/docs/commands.md b/docs/commands.md index 32f50e9..ee2963a 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -3258,14 +3258,14 @@ 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" } + { "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" } ] } ``` @@ -3276,16 +3276,23 @@ mapbox-devkit cursor 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. +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's CLI -on `PATH`, the one request this makes is to check that, and it reports -nothing was done rather than failing the run — the same shape -`generate-skills` gives a machine with no coding agent on it. +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 @@ -3339,12 +3346,16 @@ Mapbox MCP: https://mcp.mapbox.com/mcp for VS Code — 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` instead of attempting anything. A client -whose CLI isn't reachable reads `is not on PATH, skipped` +`--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. +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`, diff --git a/src/mcp.rs b/src/mcp.rs index 6f3cee8..ac11282 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -80,9 +80,9 @@ //! [`AddMcpConfig`]), since nothing about the flag or either file format //! offers an existence check any other way. -use std::io; -use std::path::PathBuf; -use std::process::Command; +use std::io::{self, BufRead, BufReader}; +use std::path::{Path, PathBuf}; +use std::process::{Command, ExitStatus, Stdio}; use anyhow::Result; use clap::builder::PossibleValuesParser; @@ -233,6 +233,102 @@ impl Client { } } +/// `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(); @@ -309,26 +405,26 @@ fn client_get(client: &Client, server_name: &str) -> GetOutcome { /// ` 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 Command::new(client.binary()) - .args(["mcp", "get", server_name]) - .output() - { + match spawn(client, &["mcp", "get", server_name]) { Ok(output) if output.status.success() => GetOutcome::Installed, Ok(_) => GetOutcome::NotInstalled, - Err(e) if e.kind() == io::ErrorKind::NotFound => GetOutcome::ClientNotFound, - // Some other failure to spawn at all (permissions, an - // interpreter missing for a script wrapper): treated the same as - // "not found," since either way this client can't be driven. + // 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 won'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. +/// 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; @@ -341,24 +437,31 @@ fn file_get(client: &Client, server_name: &str) -> GetOutcome { }; let text = match std::fs::read_to_string(&path) { Ok(text) => text, - Err(_) => return GetOutcome::NotInstalled, + // 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, }; - match serde_json::from_str::(&text) { - Ok(value) => { - 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 - } + 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, } - Err(_) => GetOutcome::Unreadable, + } + if cursor.get(server_name).is_some() { + GetOutcome::Installed + } else { + GetOutcome::NotInstalled } } @@ -390,10 +493,7 @@ fn config_path(config: &AddMcpConfig) -> Option { /// 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 { - Command::new(client.binary()) - .arg("--version") - .output() - .is_ok() + spawn(client, &["--version"]).is_ok() } /// What attempting to register a server found, once it wasn't already @@ -423,24 +523,21 @@ fn verb_add(client: &Client, server: &Server, global: bool) -> Result { // 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 output = Command::new(client.binary()) - .args(["mcp", "add", server.flag, "--url", server.url]) - .output() - .map_err(|e| spawn_failed(client, e))?; + 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 @@ -450,17 +547,19 @@ fn verb_add(client: &Client, server: &Server, global: bool) -> Result Result --add-mcp ''`. `global` is accepted for symmetry with -/// `verb_add` but unused: VS Code's `--add-mcp` has no per-project scope at -/// all (checked directly in `code --help` — no such flag exists), so this +/// `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 { @@ -488,33 +589,24 @@ fn add_mcp_flag_add(client: &Client, server: &Server, global: bool) -> Result anyhow::Error { - let detail = String::from_utf8_lossy(&output.stderr); - let detail = if detail.trim().is_empty() { - String::from_utf8_lossy(&output.stdout).into_owned() - } else { - detail.into_owned() - }; +fn add_failed(client: &Client, server: &Server, captured: &str) -> anyhow::Error { CliError::new( "error", format!( "{} could not register {}: {}", client.label, server.label, - detail.trim() + captured.trim() ), ) .into() @@ -603,12 +695,60 @@ fn wanted_clients(matches: &ArgMatches) -> Result> { Ok(detected) } -fn status_text(outcome: &GetOutcome) -> &'static str { +/// 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 => "installed", - GetOutcome::NotInstalled => "not installed", - GetOutcome::ClientNotFound => "not installed", - GetOutcome::Unreadable => "config unreadable", + GetOutcome::Installed => Status::Installed, + GetOutcome::NotInstalled => Status::NotInstalled, + GetOutcome::ClientNotFound => Status::ClientNotFound, + GetOutcome::Unreadable => Status::ConfigUnreadable, } } @@ -620,15 +760,20 @@ fn list(mode: Mode) -> Result<()> { let reachable = client_reachable(client); for server in SERVERS { let status = if !reachable { - "client not found" + Status::ClientNotFound } else { - status_text(&client_get(client, server.flag)) + status_for(&client_get(client, server.flag)) }; - lines.push(format!("{:14} {:10} {status}", server.flag, client.flag)); + lines.push(format!( + "{:14} {:10} {}", + server.flag, + client.flag, + status.text() + )); rows.push(json!({ "server": server.flag, "client": client.flag, - "status": status, + "status": status.json(), })); } } @@ -644,12 +789,13 @@ fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { 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 => ("already installed", None), + GetOutcome::Installed => (Status::AlreadyInstalled, None), GetOutcome::ClientNotFound => { lines.push(format!( "{}: {} is not on PATH, skipped.", @@ -658,7 +804,7 @@ fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { results.push(json!({ "server": server.flag, "client": client.flag, - "status": "client_not_found", + "status": Status::ClientNotFound.json(), })); continue; } @@ -670,28 +816,34 @@ fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { results.push(json!({ "server": server.flag, "client": client.flag, - "status": "config_unreadable", + "status": Status::ConfigUnreadable.json(), })); continue; } - GetOutcome::NotInstalled if dry_run => ("would install", None), + GetOutcome::NotInstalled if dry_run => (Status::WouldInstall, None), GetOutcome::NotInstalled => match client_add(client, server, global) { - Ok(AddOutcome::Installed) => ("installed", None), + Ok(AddOutcome::Installed) => (Status::Installed, None), Ok(AddOutcome::InstalledLoginIncomplete(detail)) => { - ("installed, login incomplete", Some(detail)) + (Status::InstalledLoginIncomplete, Some(detail)) + } + Err(e) => { + any_failed = true; + (Status::Failed, Some(e.to_string())) } - Err(e) => ("failed", Some(e.to_string())), }, }; lines.push(format!( - "{}: {} for {} — {status}.", - server.label, server.url, client.label + "{}: {} for {} — {}.", + server.label, + server.url, + client.label, + status.text() )); let mut row = json!({ "server": server.flag, "client": client.flag, - "status": status.replace([' ', ','], "_"), + "status": status.json(), }); if let Some(message) = error { row["error"] = json!(message); @@ -700,7 +852,20 @@ fn install(matches: &ArgMatches, mode: Mode) -> Result<()> { } } - output::emit(mode, &lines.join("\n"), json!({ "results": results })) + 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)] @@ -757,4 +922,78 @@ mod tests { .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/tests/mcp_proxy.rs b/tests/mcp_proxy.rs index cbdc81a..fee31cd 100644 --- a/tests/mcp_proxy.rs +++ b/tests/mcp_proxy.rs @@ -195,7 +195,10 @@ fn a_failed_add_is_reported_without_failing_the_whole_run() { "text", ], ); - assert!(output.status.success(), "{}", stderr(&output)); + // 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)); } @@ -296,6 +299,7 @@ case "$1" in ;; add) echo "add-called:$*" >>"$STUB_MARKER" + echo "codex-stub-stderr-line" >&2 if [ "${STUB_CODEX_WRITES:-1}" = "1" ]; then echo installed >"$STUB_STATE" fi @@ -404,7 +408,10 @@ fn codex_add_that_writes_nothing_is_reported_as_failed() { "install", "--server", "mapbox", "--client", "codex", "-o", "text", ], ); - assert!(output.status.success(), "{}", stderr(&output)); + // 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)); } @@ -563,6 +570,101 @@ fn vscode_unreadable_config_is_reported_rather_than_guessed_past() { ); } +/// 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");