Files
ersatztv/scripts/ci-step-ran.sh
T
timothy b0ed61d3d5 fix(756): round-2 — nine findings from two independent cold reviews
Marker identity no longer falls back silently: GITHUB_JOB/GITHUB_RUN_ID are
measured present and now REFUSE when absent, because the old nojob/norunid/1
defaults are REUSABLE and a stale marker there makes the guard PASS on a
dropped run. GITHUB_RUN_ATTEMPT is NOT measured (absent from the probe's
2.9MB log), so it warns and defaults rather than reddening every merge in the
repo over an assumption — these are required checks and over-strictness is not
free.

The delimiter ban now matches the raw `${{` OPENER, not a closed pair: an
unclosed opener triggers the same rewrite and evaded the 'absolute' rule. It
also covers `build`, whose Smoke + IPTV E2E step runs AFTER Build and push —
a drop there publishes a release candidate that was never booted and that
prod promotion then pulls. Its two payloads moved into env:, so the ban was
free. The record's claim that non-required jobs are 'a smaller cost' was
false for exactly that step.

Also: reject any marked step whose if: the guard protocol cannot model (it was
silently bucketed --always, which would redden a required context on a run
where the step is legitimately skipped); assert the guard's env VALUES, not
just its names; re-check anti-vacuity AFTER the skip gate narrows the set
(reproduced: --always empty + gate fired printed 'All 0 expected step(s)
executed' and exited 0); parametrise the gate tests over both jobs; correct the
migrations guard comment, which was copied verbatim from test's and named keys
migrations does not have.

Docs: restore the build-lane measurement, which a dirty-tree `git checkout -b`
had swept onto a probe branch, and add the positive control measured in the
same run (migrations: All 6 expected step(s) executed, job success). Corrected
the RUNNER_TEMP rationale — that #751 measurement was taken on a job with no
container: and does not transfer to these two.

Decisions-Edit: yes
Refs #756
2026-08-10 22:33:17 +02:00

222 lines
13 KiB
Bash
Executable File

#!/usr/bin/env bash
# Per-step execution markers for the two REQUIRED docker-build.yml jobs (ersatztv#756).
#
# WHY THIS EXISTS. A `run:` body the runner declines to interpolate is DROPPED, and the job still
# concludes `success` (ersatztv#751, `ci.workflow-run-body-no-expressions`). In
# `review-verdict.yml` that is fail-CLOSED — the required `review-verdict/h10` is simply absent and
# the merge is blocked. In `docker-build.yml` it is fail-OPEN: `Build & test (.NET)` and
# `EF migration integrity (SQLite + MySql)` are the other two required contexts on `main`, so a
# dropped step there sends a required check GREEN having done no work. #751 guarded the safe
# direction because that is where the live bug was, not because these were checked.
#
# WHY PER STEP, NOT PER JOB, which is what #756 proposed. A marker written by the job's FIRST step
# only proves the job started. The dangerous drop is not step 1 — it is `Test`, or the migration
# replay: the job runs everything around them, reports green, and nothing ran that anyone cared
# about. A guard that cannot see the fail-open case it was built for is the "guard that never
# executed" failure one level up. So every consequential step marks itself and a trailing guard
# asserts the whole expected SET.
#
# THAT GUARD CARRIES NO `if:` — unlike the #751 one, which uses `if: always()` because its job has a
# single real step. These jobs have a dozen, and a genuine early failure legitimately skips every
# later step, so `always()` would print a false "these steps never executed" on top of every ordinary
# red build. The default `success()` is the wanted condition: the guard is skipped only when an
# earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every green
# path runs the guard.
#
# WHY A SCRIPT AND NOT AN INLINE BODY, unlike the #751 guard. Two reasons, and the second is the
# load-bearing one:
#
# * The path literal exists ONCE. The #751 guard carries it twice (write + assert) and its tests
# spend real effort proving the two copies agree, because a divergence reddens every run and
# then gets deleted as broken. Here they cannot diverge.
# * A one-line `run: scripts/ci-step-ran.sh …` CANNOT CONTAIN AN EXPRESSION DELIMITER, so the
# mechanism this guards against cannot drop the guard itself. #751's own record names this as
# the stronger construction ("the body would have had to move into scripts/, where a one-line
# run: makes the class unreachable") and settled for inline only because the measurement showed
# it was not required there.
#
# WHY A SCRIPT IS ACCEPTABLE HERE THOUGH IT WOULD NOT BE IN review-verdict.yml. That workflow
# checks out the PR's BASE precisely so a PR cannot supply the code that judges it. `docker-build.yml`
# is head-resolved by design — a PR already supplies every test this job runs — so calling a script
# from the head adds no authority a PR did not already have. This is a CORRECTNESS gate against
# silent no-ops, not a security gate against a hostile PR; that job belongs to `review-verdict/h10`.
# Do not copy this reasoning back into the gate workflow.
#
# THE MARKER FILE IS KEYED ON THE RUN, and BE PRECISE ABOUT WHY — the obvious justification is a
# #751 measurement that does NOT transfer to these jobs, and saying so is the point. #751 measured
# `RUNNER_TEMP` to be `/tmp` and called it "not a private per-job directory"; that was taken on
# `review-verdict.yml`, which runs WITHOUT a `container:`. `test` and `migrations` run INSIDE the CI
# toolchain image, so their `/tmp` is the job container's own and starts empty (confirmed on the
# build-lane probe: the marker landed at `/tmp/etv-ci-steps-ran-test-1910-1`). The fresh container is
# therefore what actually rules out a stale marker here, and the keying is defence in depth.
#
# It is kept because container-per-job is a property of how the lane is configured today, not a
# guarantee, and a STALE marker is the one failure that makes this guard PASS on a run whose step was
# dropped — a silent success, i.e. the exact thing being removed. Cheap insurance against a lane
# change nobody would think to re-check this against.
set -euo pipefail
usage() {
cat >&2 <<'EOF'
usage:
ci-step-ran.sh mark <key>
Record that this step began executing. Call it as the step's FIRST act, before
anything in the body can fail.
ci-step-ran.sh assert --always <key>... [--gated <key>...]
Fail unless every expected key was marked. --always keys are always required.
--gated keys are required only when the job's skip gates did NOT fire, read from
ETV_DOCS_ONLY / ETV_REVALIDATE_SKIP so this mirrors the steps' own `if:`.
EOF
exit 2
}
# NO SILENT FALLBACK FOR THE RUN IDENTITY — found by cold review. The first version defaulted to
# `nojob`/`norunid`/`1`, and those are REUSABLE: with `GITHUB_RUN_ID` unset, every run on the host
# would share ONE marker file, so a leftover from any earlier run would satisfy the guard on a run
# whose step was dropped. A silent PASS — the exact failure the keying exists to remove, reintroduced
# by the code meant to implement it.
#
# THE TWO HALVES ARE TREATED DIFFERENTLY, ON EVIDENCE, because the blast radii differ and this is a
# REQUIRED check — a wrong refusal deadlocks every merge, so strictness is not free:
#
# * `GITHUB_JOB` and `GITHUB_RUN_ID` are MEASURED present on this runner (#756's build-lane probe
# wrote `/tmp/etv-ci-steps-ran-test-1910-1`; `test` is the job id and 1910 is the real API run
# id). Absence would mean the runner changed under us, so refusing is safe AND correct.
# * `GITHUB_RUN_ATTEMPT` is NOT measured — it appears nowhere in that 2.9 MB job log, so its
# presence is an assumption either way. Requiring it would risk reddening every job over an
# unverified variable. It therefore WARNS and defaults, which is loud rather than silent.
#
# The residual after this needs BOTH conditions at once: `GITHUB_RUN_ATTEMPT` unset AND a lane that
# stopped giving each job a fresh container (see above) — a rerun could then inherit attempt 1's
# markers. Stated rather than papered over; if the warning is ever seen in a real run, promote it.
marker_path() {
local missing="" attempt="${GITHUB_RUN_ATTEMPT:-}"
[ -n "${GITHUB_JOB:-}" ] || missing="$missing GITHUB_JOB"
[ -n "${GITHUB_RUN_ID:-}" ] || missing="$missing GITHUB_RUN_ID"
if [ -n "$missing" ]; then
echo "::error::ci-step-ran.sh cannot identify this run —${missing} not set. The marker path would fall back to a name other runs also use, and a stale marker would make the dropped-step guard PASS on a run whose step never executed (ersatztv#756). Refusing rather than degrading to a reusable name." >&2
exit 3
fi
if [ -z "$attempt" ]; then
echo "::warning::GITHUB_RUN_ATTEMPT is not set, so this run's markers share a path with its other attempts. Harmless while each job gets a fresh container; see scripts/ci-step-ran.sh (ersatztv#756)." >&2
attempt=1
fi
printf '%s/etv-ci-steps-ran-%s-%s-%s' \
"${RUNNER_TEMP:-${GITHUB_WORKSPACE:-/tmp}}" \
"$GITHUB_JOB" "$GITHUB_RUN_ID" "$attempt"
}
cmd_mark() {
[ "$#" -eq 1 ] && [ -n "$1" ] || usage
# Appended, never truncated: every step in the job shares one file, and a `>` here would erase
# its predecessors and make the guard red on every run.
#
# A failure to write is NOT swallowed. The step is running under `bash -e`, so a non-zero here
# fails the step and reddens the job — which is the same direction the guard would take a moment
# later, but with a message pointing at the real cause instead of at a missing marker.
local target
# NOT `>> "$(marker_path)"`: the refusal above `exit`s a SUBSHELL there, and bash discards a
# command substitution's exit status when it is only part of a redirection — the write would go
# to an empty path and the error would read as a redirection failure rather than the real cause.
target="$(marker_path)" || exit $?
printf '%s\n' "$1" >> "$target"
}
cmd_assert() {
local -a always=() gated=()
local bucket=""
while [ "$#" -gt 0 ]; do
case "$1" in
--always) bucket=always ;;
--gated) bucket=gated ;;
-*) usage ;;
*)
case "$bucket" in
always) always+=("$1") ;;
gated) gated+=("$1") ;;
*) usage ;;
esac ;;
esac
shift
done
# ANTI-VACUITY, at runtime rather than only in the test suite. An `assert` called with no
# expectations passes unconditionally and reports "every expected step executed" — a guard that
# proves nothing while looking like it proved everything. Refuse instead.
if [ "${#always[@]}" -eq 0 ] && [ "${#gated[@]}" -eq 0 ]; then
echo "::error::ci-step-ran.sh assert was called with no expected keys, so it would pass unconditionally. This is a workflow bug, not a build failure." >&2
exit 2
fi
# The skip gates, mirroring the `if:` every gated step carries:
# steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true'
# Anything other than the exact string `true` means the step was expected to run — including the
# EMPTY string, which is what these read as when the detect step itself was dropped. That
# direction is deliberate: a dropped detect step must widen what is required, never narrow it.
local skipped=no
if [ "${ETV_DOCS_ONLY:-}" = "true" ] || [ "${ETV_REVALIDATE_SKIP:-}" = "true" ]; then
skipped=yes
fi
local marker
# `|| exit $?` because `set -e` does NOT fire on a failing command substitution in an assignment;
# without it a degraded identity would leave `marker` empty and every key would read as missing —
# fail-closed by luck, with a misleading message.
marker="$(marker_path)" || exit $?
# `${arr[@]+"${arr[@]}"}` rather than a bare `"${arr[@]}"`: under `set -u` bash 3.2 (the system
# bash on the Macs this suite also runs on) treats expanding an EMPTY array as an unbound
# variable and aborts. The CI image ships bash 5, where it is fine — which is exactly the kind of
# difference that makes a guard pass locally and die on the runner, or the reverse.
local -a expected=(${always[@]+"${always[@]}"})
if [ "$skipped" = no ]; then
expected+=(${gated[@]+"${gated[@]}"})
else
echo "Skip gate fired (docs_only='${ETV_DOCS_ONLY:-}', already_validated='${ETV_REVALIDATE_SKIP:-}') — the gated steps were not expected to run."
fi
# RE-CHECKED AFTER GATING, not only on argv — found by cold review, which reproduced it:
# `ETV_DOCS_ONLY=true … assert --always --gated foo` printed "All 0 expected step(s) executed"
# and exited 0. The argv check above cannot see that, because the set is emptied by the gate, not
# by the caller. Unreachable with today's argv (both jobs pass `--always detect revalidate`), but
# it directly contradicted the comment above it, and a guard that reports proving everything
# while proving nothing is the failure this whole file exists to remove.
if [ "${#expected[@]}" -eq 0 ]; then
echo "::error::ci-step-ran.sh assert ended up with NO expected keys after the skip gate, so it would pass unconditionally. This is a workflow bug, not a build failure." >&2
exit 2
fi
local -a missing=()
local key
for key in "${expected[@]}"; do
# `grep -qxF` over a FILE, never a pipeline: `grep -q` exits at its first match and would
# SIGPIPE a producer, which under `set -o pipefail` inverts the result for large inputs
# (ersatztv#698). Reading the file directly has no producer to kill. `-x` so a key cannot be
# satisfied by another key that contains it, `-F` so a key is never read as a pattern.
if ! grep -qxF "$key" "$marker" 2>/dev/null; then
missing+=("$key")
fi
done
if [ "${#missing[@]}" -gt 0 ]; then
echo "::error::These steps of job '${GITHUB_JOB:-?}' never executed: ${missing[*]}. The runner DROPPED them (an interpolation failure over a run: body does this and still reports the job GREEN — ersatztv#751/#756) or their \`if:\` no longer matches the guard's expectations. This job is a REQUIRED check, so a green here would mean a required context passed having done no work. Failing the job so it is visible."
if [ -f "$marker" ]; then
echo "Marker file ${marker} recorded:"
sed 's/^/ /' "$marker"
else
echo "There is no marker file at ${marker} at all — not one step of this job executed."
fi
exit 1
fi
echo "All ${#expected[@]} expected step(s) executed: ${expected[*]}"
}
[ "$#" -ge 1 ] || usage
sub="$1"
shift
case "$sub" in
mark) cmd_mark "$@" ;;
assert) cmd_assert "$@" ;;
*) usage ;;
esac