From c80f90ed09280e4502e75cdaa6b8a8255d763544 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 13:22:49 -0400 Subject: [PATCH] docs: address Copilot review round 1 on the CLI smoke-testing guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all real. 1. The secret-scan snippet swallowed the CLI's exit code. `out=$(…)` with no status capture means a non-matching `grep` returns 1, the `if` is simply not taken, and the step exits 0 even though the tool call failed — the scan silently becomes the only assertion. Under `set -e` the opposite happens: the script dies at the assignment and never scans the error output, which is exactly where a leaked credential is most likely to appear. Now captures with `|| status=$?`, scans, then propagates, with a callout explaining both failure modes. 2. `@2` is a major-version range, not a pin, and it was written directly under a sentence claiming it pinned the version. Both occurrences now use an exact published release (2.5.0, the current `latest`), and the prerequisites note says outright that a range is not a pin. `--yes` added to the prerequisite form so an unattended job cannot hang on the first-run install prompt. 3. `--stored-auth-only` was described as failing when the store has no token. It is a no-op against a server that never challenges — the guide's own verification run passed with an empty store against a non-auth server. Reworded to say exit 3 happens only when a challenge cannot be satisfied, and added the way to actually assert that authentication happened: one run with an isolated empty MCP_STORAGE_DIR that requires exit 3. Both corrected shell snippets were executed: secret+failure exits 1, failure without a secret propagates exit 4. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B Signed-off-by: cliffhall --- docs/cli-smoke-testing.md | 43 +++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/docs/cli-smoke-testing.md b/docs/cli-smoke-testing.md index b78d9963..cc442fd4 100644 --- a/docs/cli-smoke-testing.md +++ b/docs/cli-smoke-testing.md @@ -17,9 +17,15 @@ that composes those flags. **Prerequisites:** Node `>= 22.19.0` and [`jq`](https://jqlang.github.io/jq/) for the assertions. Every example invokes the CLI as -`npx @modelcontextprotocol/inspector --cli`; pin the version in CI -(`npx @modelcontextprotocol/inspector@2 --cli …`) so a release never changes the -meaning of your job. +`npx @modelcontextprotocol/inspector --cli`, which resolves the **latest** +release each time it runs — fine while you are working at a terminal, wrong for +CI. + +**In CI, pin an exact version** — `npx --yes @modelcontextprotocol/inspector@2.5.0 +--cli …`. A range like `@2` is *not* a pin: `npx` will happily resolve a newer +2.x, so the same commit can run against a different Inspector on a later day. +`--yes` suppresses the first-run install prompt, which would otherwise hang an +unattended job. ## 1. Connect @@ -230,16 +236,25 @@ callback for **up to 15 minutes**. That is right for a human at a terminal and completely wrong for a runner. **Use `--stored-auth-only`.** It never starts interactive OAuth or step-up and -never opens a browser: it consumes the shared token store if a token is there, -and otherwise fails immediately with exit **3** (`auth_required`). +never opens a browser. When the server issues an authentication challenge it +satisfies it from the shared token store, and fails immediately with exit **3** +(`auth_required`) when the store has nothing that fits — instead of opening a +browser and waiting. ```bash npx @modelcontextprotocol/inspector --cli \ --transport http --server-url https://example.com/mcp \ --method tools/list --stored-auth-only --format json -# no token → {"error":{"code":"auth_required",…}} on stderr, exit 3 +# challenged, and no usable token → {"error":{"code":"auth_required",…}} on stderr, exit 3 ``` +⚠️ **The flag is a no-op against a server that never challenges**, so a green run +is *not* evidence that your token store was seeded correctly. A smoke job whose +server authenticates today and stops authenticating tomorrow — a misconfigured +gateway, a route that silently became public — will keep passing. If you need to +assert that authentication actually happened, assert it directly: run once +*without* a usable token in an isolated `MCP_STORAGE_DIR` and require exit `3`. + The CLI already fails fast with `auth_required` when neither stdin nor stderr is a TTY and `MCP_AUTO_OPEN_ENABLED` is unset — the typical CI shape. But that depends on the runner's TTY situation and on an env var it does not own, so it @@ -298,13 +313,25 @@ message, and it will neither catch every leak nor absolve you of reviewing what your server returns: ```bash +status=0 out=$(npx @modelcontextprotocol/inspector --cli node build/index.js \ - --method tools/call --tool-name my_tool --format json 2>&1) + --method tools/call --tool-name my_tool --format json 2>&1) || status=$? + +# Scan first — an error message is exactly where a leaked credential shows up. if grep -Eiq '(sk-[A-Za-z0-9]{16,}|ghp_[A-Za-z0-9]{20,}|AKIA[0-9A-Z]{16}|-----BEGIN [A-Z ]*PRIVATE KEY-----)' <<<"$out"; then echo "::error::tool output matched a secret pattern"; exit 1 fi +# …then propagate the call's own failure, which the capture would otherwise hide. +[ "$status" -eq 0 ] || { printf '%s\n' "$out"; exit "$status"; } ``` +⚠️ **`out=$(…)` swallows the exit code.** Without the `|| status=$?`, a +non-matching `grep` returns 1, the `if` is simply not taken, and the step exits +**0** even though the tool call failed — the scan silently becomes the only +assertion. Under `set -e` the opposite happens: the script dies at the +assignment and never scans the error output, which is where a leaked credential +is most likely to appear. Capturing the status explicitly is what gets both. + Keep the pattern list to shapes you can justify; a regex tuned for a low false positive rate is one people keep, and one that cries wolf is one they disable. @@ -336,7 +363,7 @@ export MCP_STORAGE_DIR="$(mktemp -d)" trap 'rm -rf "$MCP_STORAGE_DIR"' EXIT mcp() { - npx --yes @modelcontextprotocol/inspector@2 --cli \ + npx --yes @modelcontextprotocol/inspector@2.5.0 --cli \ --transport http --server-url "$SERVER_URL" \ --connect-timeout 10000 --stored-auth-only --format json "$@" }