docs: correct app-info signaling and the pipefail claim (Copilot round 6)
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>
This commit is contained in:
parent
ed83bfba3b
commit
e20559ce84
1 changed files with 43 additions and 9 deletions
|
|
@ -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
|
⚠️ **`--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
|
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
|
`{"appInfo": …}` **alone**. A consumer that requires `.result` will break on
|
||||||
reported through exit code `2` rather than through a key. A consumer that
|
every `--app-info` run:
|
||||||
requires `.result` will break on every `--app-info` run:
|
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
mcp-inspector --cli <server> --method tools/call --tool-name <t> --app-info --format json
|
# Tool that has an App:
|
||||||
# → {"appInfo":{"hasApp":true,…}} — no "result" key, in either case
|
# → {"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
|
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
|
reads `.result` and stops will drop the `schemaFindings` diagnostics described in
|
||||||
[§7](#7-negative-assertions), and one that *requires* it will reject the
|
[§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
|
pipeline's last status is checked unless `set -o pipefail` is also on. Every
|
||||||
example here pipes into `jq`, so keep `pipefail`.
|
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
|
## 6. Never let CI wait on interactive OAuth
|
||||||
|
|
||||||
The CLI's interactive OAuth flow opens a browser and waits on a loopback
|
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
|
`--use-stored-auth` against a **deliberately shared** store, accepting that the
|
||||||
run then reads and rotates real credentials.
|
run then reads and rotates real credentials.
|
||||||
|
|
||||||
The script exits non-zero on the first failed assertion, and the CLI's own exit
|
The script exits non-zero on the first failed assertion, so the job's status
|
||||||
code propagates through `set -e`, so the job's status already carries the
|
already carries pass/fail. **It does not carry the failure *class*:** each step
|
||||||
result. Add the `case "$status"` block from [§5](#5-branch-on-exit-codes) when
|
pipes into `jq`, and `pipefail` reports the rightmost non-zero status, so a
|
||||||
you want the annotation to name the failure class.
|
`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
|
## What this does not cover
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue