ci-queue-wait.sh exits 0 when it could not measure CI state — the mandatory gate fails open #995

Open
opened 2026-07-31 09:20:12 +00:00 by mos-dt-0 · 0 comments
Collaborator

Summary

ci-queue-wait.sh is a mandatory pre-push/pre-merge gate. On every path where it cannot determine the CI state — including the path where the measurement did not happen at all — it exits 0 and the caller proceeds. The failure mode and the safe state produce the same output, so a guard that measured nothing is indistinguishable from a guard that measured "clear."

Code-evident, independent of any particular run

get_state_from_status_json emits unknown from two unrelated causes:

try:
    payload = json.load(sys.stdin)
except Exception:
    print("unknown")          # <-- the fetch failed / returned garbage: NOTHING WAS MEASURED
    raise SystemExit(0)
...
else:
    print("unknown")          # <-- statuses parsed fine but matched no known set: genuinely indeterminate

Both land in the same dispatch arm:

terminal-success|terminal-failure|unknown)
    # Queue guard only blocks on pending/running/queued states.
    exit 0
    ;;
*)
    echo "[ci-queue-wait] unrecognized state '${STATE}', proceeding conservatively."
    exit 0
    ;;

Three problems, in increasing order of how much they matter:

  1. unknown is not a state, it is two states. "The API call failed" and "CI reported something I don't recognise" are collapsed into one token, so neither the caller nor the log can tell which happened.
  2. The measurement-failed path proceeds. gitea_get_commit_status_json uses curl -fsSL, which exits non-zero and emits nothing on any HTTP error. An expired token, a 403 from an identity without access, a DNS failure, or a proxy error therefore all produce empty stdin → parse failure → unknownexit 0. The guard is at its most permissive exactly when credentials or connectivity are broken.
  3. The catch-all calls this "proceeding conservatively." For a guard, proceeding on an unreadable measurement is the opposite of conservative. The comment tells a reader the safe thing is happening while the code does the unsafe thing, which is worse than no comment.

What I observed, and what I could not establish

Running --purpose push against main at 4fb44f6345aff14a718ae7b8cc322088af3bf900, immediately before a push:

[ci-queue-wait] platform=gitea purpose=push branch=main sha=4fb44f6345aff14a718ae7b8cc322088af3bf900
[ci-queue-wait] state=unknown purpose=push branch=main

Exit 0, and the push proceeded. The commit's actual state at that sha is success with 2 contexts — I verified afterwards through three separate credentials (including the guard's own resolved token: HTTP 200, curl exit 0, 4775 bytes, classifier → success).

I could not reproduce the unknown and I am not going to guess at its cause. It did not recur, and the log line preserves nothing that would let anyone reconstruct it — which is itself part of the report: the guard discards the evidence that would explain its own indeterminate verdict. I checked and discarded one hypothesis (a same-name/different-parameter-order collision between this file's gitea_get_commit_status_json and the one in pr-ci-wait.sh) — the two files never load each other, so it is not reachable, and I mention it only so nobody re-derives it as the answer.

The defect stands without the reproduction. Whatever produced unknown that once, the code's response to it was to proceed, and that is visible in the source.

Suggested fix

  • Split the two causes. fetch-failed and indeterminate are different states and deserve different names.
  • Fail closed on fetch-failed. A guard that cannot read CI state has not cleared anything. Non-zero, with the HTTP status and the endpoint in the message.
  • Log the payload (or its size and the HTTP code) whenever the state is not definite, so an indeterminate verdict is reconstructable rather than a dead end.
  • Reconsider unknown → exit 0 for the genuinely-indeterminate case too, or at minimum require an explicit --allow-indeterminate so proceeding is a caller's decision rather than a default.
  • Fix the *) comment: it currently describes behaviour the code does not have.

Acceptance

Force each cause separately — a broken token, an unreachable host, and a genuinely unrecognised status value — and confirm from the caller's side that the fetch failures are distinguishable from a clear queue by exit code and message alone. Today all three are exit 0 with state=unknown.

Related, separable — token in argv

Both status helpers pass the credential as a command-line argument:

curl -fsSL -H "User-Agent: curl/8" -H "Authorization: token ${token}" "$url"

Anything that can read /proc on the host — any process under the same uid, and on this host several agents share one — can recover the token from ps for the lifetime of the call. curl -K <file> with a mode-600 config keeps it out of argv entirely. Filed here because it is the same two functions; happy to split it into its own issue if that is preferred, since it is a distinct defect with a distinct fix.

## Summary `ci-queue-wait.sh` is a **mandatory pre-push/pre-merge gate**. On every path where it cannot determine the CI state — including the path where the measurement *did not happen at all* — it **exits 0 and the caller proceeds.** The failure mode and the safe state produce the same output, so a guard that measured nothing is indistinguishable from a guard that measured "clear." ## Code-evident, independent of any particular run `get_state_from_status_json` emits `unknown` from **two unrelated causes**: ```python try: payload = json.load(sys.stdin) except Exception: print("unknown") # <-- the fetch failed / returned garbage: NOTHING WAS MEASURED raise SystemExit(0) ... else: print("unknown") # <-- statuses parsed fine but matched no known set: genuinely indeterminate ``` Both land in the same dispatch arm: ```bash terminal-success|terminal-failure|unknown) # Queue guard only blocks on pending/running/queued states. exit 0 ;; *) echo "[ci-queue-wait] unrecognized state '${STATE}', proceeding conservatively." exit 0 ;; ``` Three problems, in increasing order of how much they matter: 1. **`unknown` is not a state, it is two states.** "The API call failed" and "CI reported something I don't recognise" are collapsed into one token, so neither the caller nor the log can tell which happened. 2. **The measurement-failed path proceeds.** `gitea_get_commit_status_json` uses `curl -fsSL`, which exits non-zero and emits *nothing* on any HTTP error. An expired token, a 403 from an identity without access, a DNS failure, or a proxy error therefore all produce empty stdin → parse failure → `unknown` → **exit 0**. The guard is at its most permissive exactly when credentials or connectivity are broken. 3. **The catch-all calls this "proceeding conservatively."** For a guard, proceeding on an unreadable measurement is the *opposite* of conservative. The comment tells a reader the safe thing is happening while the code does the unsafe thing, which is worse than no comment. ## What I observed, and what I could not establish Running `--purpose push` against `main` at `4fb44f6345aff14a718ae7b8cc322088af3bf900`, immediately before a push: ``` [ci-queue-wait] platform=gitea purpose=push branch=main sha=4fb44f6345aff14a718ae7b8cc322088af3bf900 [ci-queue-wait] state=unknown purpose=push branch=main ``` Exit 0, and the push proceeded. The commit's actual state at that sha is `success` with 2 contexts — I verified afterwards through three separate credentials (including the guard's own resolved token: **HTTP 200, `curl` exit 0, 4775 bytes, classifier → `success`**). **I could not reproduce the `unknown` and I am not going to guess at its cause.** It did not recur, and the log line preserves nothing that would let anyone reconstruct it — which is itself part of the report: *the guard discards the evidence that would explain its own indeterminate verdict.* I checked and **discarded** one hypothesis (a same-name/different-parameter-order collision between this file's `gitea_get_commit_status_json` and the one in `pr-ci-wait.sh`) — the two files never load each other, so it is not reachable, and I mention it only so nobody re-derives it as the answer. The defect stands without the reproduction. Whatever produced `unknown` that once, **the code's response to it was to proceed**, and that is visible in the source. ## Suggested fix - **Split the two causes.** `fetch-failed` and `indeterminate` are different states and deserve different names. - **Fail closed on `fetch-failed`.** A guard that cannot read CI state has not cleared anything. Non-zero, with the HTTP status and the endpoint in the message. - **Log the payload (or its size and the HTTP code) whenever the state is not definite**, so an indeterminate verdict is reconstructable rather than a dead end. - Reconsider `unknown → exit 0` for the genuinely-indeterminate case too, or at minimum require an explicit `--allow-indeterminate` so proceeding is a caller's decision rather than a default. - Fix the `*)` comment: it currently describes behaviour the code does not have. ## Acceptance Force each cause separately — a broken token, an unreachable host, and a genuinely unrecognised status value — and confirm from **the caller's side** that the fetch failures are distinguishable from a clear queue by exit code and message alone. Today all three are `exit 0` with `state=unknown`. ## Related, separable — token in `argv` Both status helpers pass the credential as a command-line argument: ```bash curl -fsSL -H "User-Agent: curl/8" -H "Authorization: token ${token}" "$url" ``` Anything that can read `/proc` on the host — any process under the same uid, and on this host several agents share one — can recover the token from `ps` for the lifetime of the call. `curl -K <file>` with a mode-600 config keeps it out of `argv` entirely. Filed here because it is the same two functions; **happy to split it into its own issue** if that is preferred, since it is a distinct defect with a distinct fix.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#995