From ed83bfba3bd50c02d04ebbeadaa4b5b482dd8e63 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 14:14:51 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20reconcile=20store=20isolation=20with=20?= =?UTF-8?q?--stored-auth-only=20in=20the=20=C2=A78=20script?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 4. The complete script created an empty isolated OAuth store and then always passed --stored-auth-only, so against an OAuth-protected server it could never authenticate — exit 3 by construction — while being presented as the general "arbitrary MCP server" template. The two requirements are genuinely in tension: §6 wants isolation so a run cannot read or rotate real tokens, and --stored-auth-only wants a token to reuse. Round 2 made the isolation airtight without reconciling that, which is what turned a latent gap into a guaranteed one. Resolved by making credential provisioning explicit rather than by scoping the script down to unauthenticated servers: it now takes a bearer token from the CI secret store when MCP_TOKEN is set — the approach §6 already recommends for unattended runs — and is unchanged when it is not. Added a callout naming the tension, wired MCP_TOKEN through the Actions job, and noted that a grant requiring genuine interactive OAuth does not belong in a CI smoke test at all. The conditional argument uses "${auth[@]+"${auth[@]}"}" because an empty array is an unbound variable under `set -u` in bash before 4.4, and macOS still ships 3.2 (verified locally). Documented why, so it is not simplified into ${MCP_TOKEN:+--header "..."} — that form happens to preserve its inner quoting in bash, but as a shell-specific subtlety a reader cannot check rather than something the script states. Verified against a live HTTP test server both ways: MCP_TOKEN unset and MCP_TOKEN set to a value containing a space both pass all three assertions. Argv construction checked directly — the header arrives as one word with the token intact, and no empty argument is passed when the variable is unset. 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 | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/docs/cli-smoke-testing.md b/docs/cli-smoke-testing.md index 06410591..f86fdfb4 100644 --- a/docs/cli-smoke-testing.md +++ b/docs/cli-smoke-testing.md @@ -425,7 +425,16 @@ editing a schema. ## 8. Putting it together A complete smoke script. It bounds the connect, isolates the token store, and -fails the job on the first assertion that does not hold: +fails the job on the first assertion that does not hold. + +⚠️ **An isolated store starts empty, so `--stored-auth-only` alone cannot +authenticate.** Those two are deliberately in tension: §6 wants isolation so a +run cannot touch real tokens, and `--stored-auth-only` wants a token to reuse. +Against an OAuth-protected server the combination is exit `3` every time, by +construction. Resolve it explicitly rather than by accident — the script below +takes a bearer token from your CI secret store when one is set, which is the +approach §6 recommends for unattended runs, and works unchanged against a server +that needs no credential at all. ```bash #!/usr/bin/env bash @@ -433,16 +442,26 @@ fails the job on the first assertion that does not hold: set -euo pipefail SERVER_URL="${SERVER_URL:?set SERVER_URL}" + # Both, in precedence order — MCP_INSPECTOR_OAUTH_STATE_PATH is checked first, # so an inherited one would defeat the scratch directory. See §6. export MCP_STORAGE_DIR="$(mktemp -d)" export MCP_INSPECTOR_OAUTH_STATE_PATH="$MCP_STORAGE_DIR/oauth.json" trap 'rm -rf "$MCP_STORAGE_DIR"' EXIT +# The isolated store above is empty, so --stored-auth-only can never satisfy an +# OAuth challenge on its own. Supply a credential here instead when the server +# needs one; leave MCP_TOKEN unset for a server that does not. +auth=() +if [ -n "${MCP_TOKEN:-}" ]; then + auth=(--header "Authorization: Bearer $MCP_TOKEN") +fi + mcp() { npx --yes @modelcontextprotocol/inspector@2.5.0 --cli \ --transport http --server-url "$SERVER_URL" \ - --connect-timeout 10000 --stored-auth-only --format json "$@" + --connect-timeout 10000 --stored-auth-only --format json \ + "${auth[@]+"${auth[@]}"}" "$@" } # 1. Handshake. @@ -479,8 +498,24 @@ smoke: - run: bash smoke.sh env: SERVER_URL: ${{ vars.MCP_SERVER_URL }} + # Omit for a server that needs no credential; the script adapts. + MCP_TOKEN: ${{ secrets.MCP_TOKEN }} ``` +`"${auth[@]+"${auth[@]}"}"` rather than `"${auth[@]}"`: under `set -u` an empty +array is an unbound variable in bash before 4.4, and the runner is not the only +place this script runs — macOS still ships bash 3.2. The guarded form expands to +nothing when `auth` is empty and to two correctly-quoted words when it is not. +Do not "simplify" it to `${MCP_TOKEN:+--header "Authorization: Bearer …"}`, +whose quoting behavior inside the expansion is a bash-specific subtlety rather +than something a reader can check. + +If your server genuinely requires an interactive OAuth grant that no static +credential can stand in for, a CI smoke test is the wrong place to complete it — +authorize once in the web inspector and hand the CLI the resulting token via +`--use-stored-auth` against a **deliberately shared** store, accepting that the +run then reads and rotates real credentials. + The script exits non-zero on the first failed assertion, and the CLI's own exit code propagates through `set -e`, so the job's status already carries the result. Add the `case "$status"` block from [§5](#5-branch-on-exit-codes) when