Copilot round 7. The copyable job inherited the repository's default
GITHUB_TOKEN scopes and left that token in the runner's git config while
`npx` executed a package downloaded from npm.
Adds `permissions: contents: read` and `persist-credentials: false`,
each with a comment giving its reason.
The review cited `.github/workflows/main.yml:11-18` as precedent for
both. It supports one: main.yml does set `permissions: contents: read`
at workflow level, for exactly the stated reason, and every workflow in
this repo carries a block. It does not support the other —
`persist-credentials` appears nowhere in .github/workflows. That half is
taken on its own merit rather than as convention: the job uses git for
nothing after checkout, so the credential is only attack surface while
third-party code runs.
Both matter more in this example than in a workflow we own, because it
is written to be copied into a repository whose defaults the reader
cannot see — which is now stated in the guide.
The edited block was parsed as YAML to confirm both keys land where
intended rather than eyeballing the indentation.
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>
Two findings, both real, both mine — and both found by a review round
requested after an explicit "approval recommended", which is the
argument for not stopping at one quiet round.
1. The guide said the `hasApp:false` case is reported "through exit code
2 rather than through a key". It is reported BOTH ways:
`{"appInfo":{"hasApp":false,"toolName":"echo"}}` with exit 2. This
was the round-3 fix over-correcting — that round established there is
no `result` key under --app-info, and I generalized it into "no key
reports this", which does not follow. Now documents both signals and
when each is the right one to branch on: the exit code short-circuits
an && chain without parsing, while the field is the only one of the
two that survives `tools/list --app-info`, whose NDJSON reports every
tool over one connection and exits 0 regardless.
2. §8 claimed "the CLI's own exit code propagates through set -e". It
does not. `pipefail` reports the RIGHTMOST non-zero status, so a
tools/call returning isError:true — which exits 5 and also falsifies
the assertion — surfaces as 1, and the §5 `case` block the guide
pointed at would print the catch-all instead of naming the tool
error. Verified: left=5/right=1 → 1; left=5/right=0 → 5; and the real
CLI piped into `jq -e '.result.isError != true'` → 1.
Added the status table and corrected §8 to say the job carries
pass/fail but not the class, with the capture-then-assert form as the
way to get it. Verified that form recovers exit 5 where the pipeline
reported 1, and that the happy path still passes.
Worth recording: the first attempt to reproduce finding 2 was itself
wrong — feeding jq invalid JSON made it exit on a parse error and the
pipeline returned 5, appearing to refute the claim. The controlled test
and the real CLI both confirm it.
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>
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings, both real; the first was introduced by the round-2 fix.
1. The envelope table said `result` is present "Always". It is not:
`emitResult` returns early for `--app-info` and writes `{appInfo}`
alone, with no `result` key — in the `hasApp:false` case too, which
is signalled through exit code 2 rather than through a key. So a
consumer that requires `.result` breaks on every probe. Documented
as a distinct shape rather than a footnote on the table, since that
is what it is. Verified against the mcp-app-http fixture: keys are
["appInfo"] with --app-info for a tool with and without an app, and
["appInfo","result"] for the same tool called without it.
Worth noting this was self-inflicted — the table arrived in the
round-2 fix for `schemaFindings`, and stating a shape more precisely
is exactly where an over-broad "always" slips in.
2. The Actions job ran `./smoke.sh` while the guide never tells anyone
to chmod +x, and a file copied out of a code block does not carry the
mode bit. Now `bash smoke.sh`, with a comment saying why so it is not
tidied back. Verified: ./nonexec.sh fails with "permission denied",
bash nonexec.sh runs.
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>
Four distinct issues across 2 inline and 3 suppressed comments. All were
verified against the CLI's own code and behavior before being accepted;
one was diagnosed correctly but explained wrongly, and is documented as
it actually behaves rather than as the comment stated.
1. MCP_STORAGE_DIR alone is not isolation. MCP_INSPECTOR_OAUTH_STATE_PATH
is checked first, so an inherited value silently wins and a smoke run
still reaches the real token file — the guide documented that
precedence and then ignored it one paragraph later. Both variables are
now exported, in precedence order, in §6 and in the §8 script, with a
--list-stored-auth check that prints the path actually resolved.
Verified: with a poisoned MCP_INSPECTOR_OAUTH_STATE_PATH in the
environment, the two-line export wins.
2. `--format json` can carry a third top-level key. `emitResult` sets
`schemaFindings` whenever --strict is passed to tools/list and there
is at least one finding, so "that is the whole contract" was wrong.
Replaced with a table of all three keys and a note to parse by key.
3. --strict findings under --format json. The actionable half is right —
a JSON pipeline should read `schemaFindings` from stdout. The stated
mechanism is not: `writeSchemaLintReport` runs unconditionally, so
--format json --strict emits the full human report on stderr AND the
structured copy on stdout. Documented both streams as measured, with
a jq gate over the per-tool grouping. The `?` operators in that filter
are load-bearing: the key is absent on a clean run, and without them
jq errors instead of passing. Both arms verified.
4. The ErrorEnvelope snippet returned 0. `cmd || { … }` makes the brace
group's status the list's, so a successful jq turned a failed run
green — the same failure class the guide warns about twice elsewhere,
which makes shipping it worse. Now captures, reports, and exits with
the status; verified propagating exit 4.
The §8 script was re-run end to end against a live HTTP test server with
the added export and still passes.
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>
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>
Adds `docs/cli-smoke-testing.md` — a copyable connect → list → call →
assert workflow for testing an arbitrary MCP server from a shell or a CI
job, plus index rows in the root README and a pointer from the CLI
README.
The gap this fills is a user-facing one. `clients/cli/README.md` is a
flag reference and `scripts/smoke-cli.mjs` is an internal end-to-end
check of the launcher → CLI path; neither tells someone how to compose
those flags into a check they run on every commit against their own
server.
Covers, in order: connecting over stdio/HTTP/SSE (including the `--`
separator, which splits the opposite way under `--cli`); `--format json`
and its `{result[, appInfo]}` envelope; asserting a tool exists with
`jq -e`; one representative `tools/call` and the `--tool-arg` vs
`--tool-args-json` coercion difference; the exit-code map and how to
branch on it; keeping OAuth non-interactive with `--stored-auth-only`
and an isolated `MCP_STORAGE_DIR`; and negative assertions — refusals,
a secret-shape scan of captured output, and `--strict` schema
portability. Ends with a complete script and a GitHub Actions job.
Every command and output shape in the guide was verified against the
bundled composable test servers over both stdio and HTTP rather than
transcribed from the reference, which corrected three claims that would
otherwise have shipped wrong:
- `--tool-arg zip=012` does not send `12`. `012` is not valid JSON, so
it is sent as the literal string. The real trap is `zip=10001`, which
does coerce to a number and is then rejected by a string schema.
- `if ! cmd; then status=$?` yields `0`, because `!` inverts the
pipeline's status — every failure class would look identical. The
guide uses `|| status=$?` and says why.
- The sample script does not report a failure class on its own; that is
the opt-in `case` block from §5.
The §8 script was run end to end against a live HTTP test server, and
exits 0/3/4/5/6 were each reproduced against a real server.
Closes#1886
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>
