From e20559ce847098a2cec7ab668c26953dbdb365cc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 14:30:18 -0400 Subject: [PATCH] docs: correct app-info signaling and the pipefail claim (Copilot round 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B Signed-off-by: cliffhall --- docs/cli-smoke-testing.md | 52 ++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/docs/cli-smoke-testing.md b/docs/cli-smoke-testing.md index f86fdfb4..1e8d6422 100644 --- a/docs/cli-smoke-testing.md +++ b/docs/cli-smoke-testing.md @@ -105,15 +105,23 @@ For every method **except** an `--app-info` probe, the envelope carries ⚠️ **`--app-info` is a different shape, not a variation on this one.** It probes without invoking the tool, so there is no result to report and the envelope is -`{"appInfo": …}` **alone** — including in the `hasApp:false` case, which is -reported through exit code `2` rather than through a key. A consumer that -requires `.result` will break on every `--app-info` run: +`{"appInfo": …}` **alone**. A consumer that requires `.result` will break on +every `--app-info` run: ```bash -mcp-inspector --cli --method tools/call --tool-name --app-info --format json -# → {"appInfo":{"hasApp":true,…}} — no "result" key, in either case +# Tool that has an App: +# → {"appInfo":{"hasApp":true,"toolName":"…","resourceUri":"ui://…",…}} exit 0 +# Tool that does not: +# → {"appInfo":{"hasApp":false,"toolName":"…"}} exit 2 ``` +The no-App answer is reported **both ways** — as `appInfo.hasApp: false` in the +body *and* as exit code `2` — so branch on whichever suits the caller. The exit +code short-circuits an `&&` chain without parsing; the field is what a pipeline +reading many probes wants, and it is the only one of the two that survives +`tools/list --app-info`, whose NDJSON reports every tool over a single +connection and exits `0` regardless. + Parse the envelope by key rather than assuming a fixed shape — a consumer that reads `.result` and stops will drop the `schemaFindings` diagnostics described in [§7](#7-negative-assertions), and one that *requires* it will reject the @@ -263,6 +271,28 @@ every failure class looks identical. pipeline's last status is checked unless `set -o pipefail` is also on. Every example here pipes into `jq`, so keep `pipefail`. +⚠️ **`pipefail` gives you *a* failure, not *the CLI's* failure.** It reports the +**rightmost** non-zero status, so when both sides fail the CLI's class is lost: + +| CLI | `jq -e` | Pipeline status | +| --- | --- | --- | +| `5` (`tool_is_error`) | `1` (assertion false) | **`1`** — the class is gone | +| `5` | `0` | `5` — survives | +| `0` | `1` | `1` | + +The first row is the common one: a `tools/call` returning `isError:true` exits +`5` *and* makes `.result.isError != true` false, so the pipeline reports `1` and +the `case` block above would print the catch-all rather than name the tool +error. **When you need the failure class, capture the CLI's status before `jq` +touches it:** + +```bash +status=0 +out=$(mcp --method tools/call --tool-name my_tool) || status=$? +[ "$status" -eq 0 ] || { echo "::error::CLI exit $status"; exit "$status"; } +jq -e '.result.isError != true' <<<"$out" > /dev/null +``` + ## 6. Never let CI wait on interactive OAuth The CLI's interactive OAuth flow opens a browser and waits on a loopback @@ -516,10 +546,14 @@ 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 -you want the annotation to name the failure class. +The script exits non-zero on the first failed assertion, so the job's status +already carries pass/fail. **It does not carry the failure *class*:** each step +pipes into `jq`, and `pipefail` reports the rightmost non-zero status, so a +`tools/call` that exits `5` while the assertion also fails surfaces as `1`. That +is fine for a gate whose only question is "did it pass", and it is why the +`case "$status"` block from [§5](#5-branch-on-exit-codes) cannot simply be +appended here. To name the class, split the CLI call from the assertion as shown +there — capture the status first, then run `jq` over the captured output. ## What this does not cover