docs: address Copilot review round 1 on the CLI smoke-testing guide

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-06 13:22:49 -04:00
commit c80f90ed09

View file

@ -17,9 +17,15 @@ that composes those flags.
**Prerequisites:** Node `>= 22.19.0` and [`jq`](https://jqlang.github.io/jq/) **Prerequisites:** Node `>= 22.19.0` and [`jq`](https://jqlang.github.io/jq/)
for the assertions. Every example invokes the CLI as for the assertions. Every example invokes the CLI as
`npx @modelcontextprotocol/inspector --cli`; pin the version in CI `npx @modelcontextprotocol/inspector --cli`, which resolves the **latest**
(`npx @modelcontextprotocol/inspector@2 --cli …`) so a release never changes the release each time it runs — fine while you are working at a terminal, wrong for
meaning of your job. 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 ## 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. completely wrong for a runner.
**Use `--stored-auth-only`.** It never starts interactive OAuth or step-up and **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, never opens a browser. When the server issues an authentication challenge it
and otherwise fails immediately with exit **3** (`auth_required`). 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 ```bash
npx @modelcontextprotocol/inspector --cli \ npx @modelcontextprotocol/inspector --cli \
--transport http --server-url https://example.com/mcp \ --transport http --server-url https://example.com/mcp \
--method tools/list --stored-auth-only --format json --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 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 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 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: your server returns:
```bash ```bash
status=0
out=$(npx @modelcontextprotocol/inspector --cli node build/index.js \ 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 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 echo "::error::tool output matched a secret pattern"; exit 1
fi 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 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. 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 trap 'rm -rf "$MCP_STORAGE_DIR"' EXIT
mcp() { mcp() {
npx --yes @modelcontextprotocol/inspector@2 --cli \ npx --yes @modelcontextprotocol/inspector@2.5.0 --cli \
--transport http --server-url "$SERVER_URL" \ --transport http --server-url "$SERVER_URL" \
--connect-timeout 10000 --stored-auth-only --format json "$@" --connect-timeout 10000 --stored-auth-only --format json "$@"
} }