diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index b90794028..a7451d9bf 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -122,14 +122,26 @@ jobs: # ersatztv#416: is this a docs-only change? If so, every heavy step below is skipped and this # REQUIRED job reports success in seconds. It still RUNS (never `if:`-skipped) so the required # context keeps reporting — see the workflow header and docs/ci-cd.md -> "Docs-only skip". + # EVERY consequential `run:` step in this job marks itself as its FIRST act (ersatztv#756), + # and the trailing `Assert every expected step executed` guard fails the job when one is + # missing. This is a REQUIRED context on `main`, and a step the runner drops takes the job + # GREEN having done no work — see scripts/ci-step-ran.sh for why that is fail-OPEN here while + # the same drop in review-verdict.yml is fail-CLOSED. - name: Detect docs-only changes id: detect - run: scripts/ci-detect-docs-only.sh + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark detect + scripts/ci-detect-docs-only.sh - name: Detect already-validated tree (#420) id: revalidate env: ETV_STATUS_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }} - run: scripts/ci-detect-already-validated.sh + run: | + # PROBE ONLY (ersatztv#756) - reintroduces the #751 defect: `pr number` is not a valid + # expression, so the runner fails to interpolate this whole scalar and DROPS the step: + # ${{ pr number }} + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark revalidate + scripts/ci-detect-already-validated.sh - name: Cache NuGet packages if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' @@ -141,7 +153,9 @@ jobs: - name: Restore if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' - run: dotnet restore + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark restore + dotnet restore # Replaces setup-node's built-in `cache: npm`. The toolchain image supplies node/npm, but # the SPA's package downloads are project deps, so they stay cached per lockfile. @@ -156,36 +170,50 @@ jobs: - name: Install SPA dependencies if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' working-directory: web - run: npm ci + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark npm-ci + npm ci - name: Check generated SPA API client if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' working-directory: web - run: npm run check:api + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark check-api + npm run check:api - name: Lint SPA if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' working-directory: web - run: npm run lint + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark lint + npm run lint - name: Typecheck SPA if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' working-directory: web - run: npm run typecheck + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark typecheck + npm run typecheck - name: Test SPA if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' working-directory: web - run: npm test -- --run + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark web-test + npm test -- --run - name: Build SPA if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' working-directory: web - run: npm run build + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark web-build + npm run build - name: Strip Scanner project ref (matches Docker build) if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' - run: sed -i '/Scanner/d' ErsatzTV/ErsatzTV.csproj + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark strip-scanner + sed -i '/Scanner/d' ErsatzTV/ErsatzTV.csproj # Start the true peak-anon sampler just before the memory-heavy dotnet Build/Test/Coverage so # its high-water mark spans them (SPA build/test above are comparatively light). Paired with the @@ -199,13 +227,16 @@ jobs: - name: Build if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' - run: dotnet build --configuration Release --no-restore + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark build + dotnet build --configuration Release --no-restore - name: Test if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' - run: >- - dotnet test --configuration Release --no-build --blame-hang-timeout "2m" --verbosity normal - --collect:"XPlat Code Coverage" --settings coverlet.runsettings --results-directory ./coverage + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark dotnet-test + dotnet test --configuration Release --no-build --blame-hang-timeout "2m" --verbosity normal \ + --collect:"XPlat Code Coverage" --settings coverlet.runsettings --results-directory ./coverage # Coverage reporting (ersatztv#15 scope item 4): coverlet.collector emits a Cobertura report # per test project (via --collect above); ReportGenerator merges them into a human-readable @@ -258,6 +289,43 @@ jobs: continue-on-error: true run: scripts/ci-peak-anon.sh report + # THE DROPPED-STEP GUARD (ersatztv#756). Every `run:` step above records that it began; this + # asserts the whole expected SET was recorded. A step the runner declines to interpolate is + # DROPPED and still concludes `success` (ersatztv#751), so without this a REQUIRED context + # reports green having done no work — fail-OPEN, and strictly worse than the fail-CLOSED + # version of the same bug that #751 fixed in review-verdict.yml. + # + # NO `if:` HERE, WHICH IS A DELIBERATE DEPARTURE FROM THE #751 GUARD and the one decision in + # this block that is easy to "fix" wrongly. That guard uses `if: always()` because its job has + # exactly one real step, so there is no ordinary red for it to talk over. Here there are + # twelve, and a genuine failure in an early one (a lint error, a failing test) SKIPS every + # later step — an `always()` guard would then announce "these steps never executed: typecheck + # web-test build dotnet-test" on top of every normal red build. That is not a dropped step, it + # is the runner doing what it is told, and a guard that cries wolf on every red build is a + # guard that gets deleted. + # + # The default `if:` is `success()`, which is exactly the condition wanted, and the invariant it + # rests on is worth stating because it is what makes the omission safe rather than lucky: this + # step is skipped ONLY when an earlier step failed, and an earlier step failing already fails + # the job. So `guard skipped => job red`, and the only path to a green job runs the guard. A + # dropped step is invisible precisely because it concludes `success`, which keeps the job green + # and therefore reaches here. + # + # ITS OWN BODY CANNOT BE DROPPED BY THE MECHANISM IT GUARDS AGAINST: it is a single command + # with no expression delimiter anywhere in the scalar, so the runner has nothing to rewrite. + # The two gate values come in through `env:`, which is interpolated PER VALUE — a bad payload + # there cannot take the body with it (`ci.workflow-run-body-no-expressions`), and both paths + # are held to naming a real context by + # test_every_workflow_expression_names_a_REAL_context_or_function. + - name: Assert every expected step executed (ersatztv#756) + env: + ETV_DOCS_ONLY: ${{ steps.detect.outputs.docs_only }} + ETV_REVALIDATE_SKIP: ${{ steps.revalidate.outputs.skip }} + run: >- + scripts/ci-step-ran.sh assert + --always detect revalidate + --gated restore npm-ci check-api lint typecheck web-test web-build strip-scanner build dotnet-test + migrations: name: EF migration integrity (SQLite + MySql) runs-on: ubuntu-latest @@ -328,14 +396,21 @@ jobs: # ersatztv#416: docs-only? Skip the build + migration replay; the job still reports success in # seconds. REQUIRED context, so it always RUNS (never `if:`-skipped). See the workflow header. + # Same per-step marker contract as the `test` job above (ersatztv#756) — this is the other + # REQUIRED context, so a dropped migration-replay step would report EF integrity green having + # replayed nothing. - name: Detect docs-only changes id: detect - run: scripts/ci-detect-docs-only.sh + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark detect + scripts/ci-detect-docs-only.sh - name: Detect already-validated tree (#420) id: revalidate env: ETV_STATUS_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }} - run: scripts/ci-detect-already-validated.sh + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark revalidate + scripts/ci-detect-already-validated.sh - name: Cache NuGet packages if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' @@ -347,11 +422,15 @@ jobs: - name: Restore if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' - run: dotnet restore + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark restore + dotnet restore - name: Build if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' - run: dotnet build --configuration Release --no-restore + run: | + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark build + dotnet build --configuration Release --no-restore # dotnet-ef is baked into the CI toolchain image (docker/ci/Dockerfile) and already on PATH # — no per-run `dotnet tool install`. Bump its version there (ersatztv#390). @@ -361,6 +440,7 @@ jobs: if: steps.detect.outputs.docs_only != 'true' && steps.revalidate.outputs.skip != 'true' run: | set -euo pipefail + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark sqlite echo "::group::SQLite model drift (has-pending-model-changes)" dotnet ef migrations has-pending-model-changes --no-build --configuration Release \ --context TvContext --startup-project ErsatzTV --project ErsatzTV.Infrastructure.Sqlite -- --provider Sqlite @@ -384,6 +464,7 @@ jobs: MySql__ConnectionString: "Server=mysql;Port=3306;Database=ersatztv_migrations;Uid=root;Pwd=ersatztv;DefaultCommandTimeout=300;" run: | set -euo pipefail + "${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark mysql echo "::group::MySql model drift (has-pending-model-changes)" dotnet ef migrations has-pending-model-changes --no-build --configuration Release \ --context TvContext --startup-project ErsatzTV --project ErsatzTV.Infrastructure.MySql -- --provider MySql @@ -418,6 +499,43 @@ jobs: # how the original defects escaped. The fixture itself is retained and is opt-in via # ETV_TEST_MYSQL_CONNECTION (skipped, visibly, without it). Re-arming it here is tracked by #627. + # THE DROPPED-STEP GUARD (ersatztv#756). Every `run:` step above records that it began; this + # asserts the whole expected SET was recorded. A step the runner declines to interpolate is + # DROPPED and still concludes `success` (ersatztv#751), so without this a REQUIRED context + # reports green having done no work — fail-OPEN, and strictly worse than the fail-CLOSED + # version of the same bug that #751 fixed in review-verdict.yml. + # + # NO `if:` HERE, WHICH IS A DELIBERATE DEPARTURE FROM THE #751 GUARD and the one decision in + # this block that is easy to "fix" wrongly. That guard uses `if: always()` because its job has + # exactly one real step, so there is no ordinary red for it to talk over. Here there are + # twelve, and a genuine failure in an early one (a lint error, a failing test) SKIPS every + # later step — an `always()` guard would then announce "these steps never executed: typecheck + # web-test build dotnet-test" on top of every normal red build. That is not a dropped step, it + # is the runner doing what it is told, and a guard that cries wolf on every red build is a + # guard that gets deleted. + # + # The default `if:` is `success()`, which is exactly the condition wanted, and the invariant it + # rests on is worth stating because it is what makes the omission safe rather than lucky: this + # step is skipped ONLY when an earlier step failed, and an earlier step failing already fails + # the job. So `guard skipped => job red`, and the only path to a green job runs the guard. A + # dropped step is invisible precisely because it concludes `success`, which keeps the job green + # and therefore reaches here. + # + # ITS OWN BODY CANNOT BE DROPPED BY THE MECHANISM IT GUARDS AGAINST: it is a single command + # with no expression delimiter anywhere in the scalar, so the runner has nothing to rewrite. + # The two gate values come in through `env:`, which is interpolated PER VALUE — a bad payload + # there cannot take the body with it (`ci.workflow-run-body-no-expressions`), and both paths + # are held to naming a real context by + # test_every_workflow_expression_names_a_REAL_context_or_function. + - name: Assert every expected step executed (ersatztv#756) + env: + ETV_DOCS_ONLY: ${{ steps.detect.outputs.docs_only }} + ETV_REVALIDATE_SKIP: ${{ steps.revalidate.outputs.skip }} + run: >- + scripts/ci-step-ran.sh assert + --always detect revalidate + --gated restore build sqlite mysql + functional-e2e: name: Functional E2E (curl + UI contracts) runs-on: ubuntu-latest diff --git a/scripts/ci-step-ran.sh b/scripts/ci-step-ran.sh new file mode 100755 index 000000000..c1dacf695 --- /dev/null +++ b/scripts/ci-step-ran.sh @@ -0,0 +1,161 @@ +#!/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 the trailing +# `if: always()` guard asserts the whole expected SET. +# +# 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, not a fixed name. `RUNNER_TEMP` was measured to be `/tmp` on +# this instance (ersatztv#751) — a shared directory, not a private per-job dir — so a fixed name +# lives where another job can also write it, and a STALE marker would satisfy the guard on a run +# whose step was dropped: a silent PASS, the exact failure being removed. Job id, run id and attempt +# together mean no other run or job can answer for this one. If the runner exports none of them the +# path degrades to a fixed name, which is no worse than having no key at all. + +set -euo pipefail + +usage() { + cat >&2 <<'EOF' +usage: + ci-step-ran.sh mark + 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 ... [--gated ...] + 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 +} + +marker_path() { + printf '%s/etv-ci-steps-ran-%s-%s-%s' \ + "${RUNNER_TEMP:-${GITHUB_WORKSPACE:-/tmp}}" \ + "${GITHUB_JOB:-nojob}" \ + "${GITHUB_RUN_ID:-norunid}" \ + "${GITHUB_RUN_ATTEMPT:-1}" +} + +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. + printf '%s\n' "$1" >> "$(marker_path)" +} + +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 + marker="$(marker_path)" + # `${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 + + 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 diff --git a/scripts/tests/test_ci_dropped_step_guard.py b/scripts/tests/test_ci_dropped_step_guard.py new file mode 100644 index 000000000..ef13b58ef --- /dev/null +++ b/scripts/tests/test_ci_dropped_step_guard.py @@ -0,0 +1,512 @@ +"""The dropped-step guard on docker-build.yml's two REQUIRED jobs (ersatztv#756). + +WHAT THIS IS PROTECTING. A `run:` body the runner declines to interpolate is DROPPED, and the job +still concludes `success` (ersatztv#751, `ci.workflow-run-body-no-expressions`). #751 fixed that in +`review-verdict.yml`, where the consequence is fail-CLOSED — `review-verdict/h10` is absent and the +merge is blocked. It left the two places where the same drop 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. + +THE TESTS COME IN THREE KINDS AND NONE SUBSTITUTES FOR ANOTHER, which is the lesson #751 paid for: + + * STATIC — the marker set and the guard's expectations agree, and the guard is positioned so it + can actually run. Cheap, and the only kind that catches a NEW step added without a marker. + * BEHAVIOURAL — the guard's real command line is EXECUTED against markers written by the steps' + real marker lines, both extracted from the parsed workflow. A structural test cannot prove an + exit code, and `exit 1` in a body is satisfiable by dead code. + * A LIVE PROBE — that the runner still executes a LATER step after dropping an earlier one, on the + BUILD lane rather than the `small` lane #751 measured. That is the premise the whole guard rests + on and no test here can establish it; it is recorded in docs/ci-cd.md and on the issue. +""" + +from __future__ import annotations + +import os +import re +import subprocess +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "docker-build.yml" +SCRIPT = REPO_ROOT / "scripts" / "ci-step-ran.sh" + +# The jobs whose contexts branch protection REQUIRES on `main`. Read live on 2026-08-10: +# Build ErsatzTV Image / Build & test (.NET) (pull_request) +# Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) +# review-verdict/h10 +# The third is guarded by test_pr_changed_files.py; these two are this file's subject. `build`, +# `api-docs` and `format` are deliberately NOT here — they are not required, and all three +# legitimately interpolate into a `run:` body, so extending the absolute rule to them would be false. +REQUIRED_JOBS = ("test", "migrations") + +_EXPR = re.compile(r"\$\{\{(.*?)\}\}", re.S) +_MARK = re.compile(r'ci-step-ran\.sh"?\s+mark\s+(\S+)') + + +# ONE parse, shared. `yaml.safe_load` per call returns a fresh object graph, so an identity test +# across two helpers (`steps[-1] is guard`) would compare structurally-equal but distinct dicts and +# fail — or, worse in the other direction, an `is not` filter would exclude nothing and a step would +# match as its own guard. That is not hypothetical: test_pr_changed_files.py records exactly this +# going wrong in the #751 guard test, where the assertions then ran against the wrong step. +_DOC = yaml.safe_load(WORKFLOW.read_text()) + + +def _doc(): + return _DOC + + +def _steps(job: str): + return _doc()["jobs"][job]["steps"] + + +def _run_steps(job: str): + return [s for s in _steps(job) if s.get("run")] + + +def _guard(job: str): + """The trailing assert step. Located by CONTENT, never by index. + + Locating it as `steps[-1]` here and then asserting it is last elsewhere would be circular — the + position test would hold by construction. This finds the step that invokes the assert + sub-command, and `test_the_guard_is_the_LAST_step` independently checks where it sits. + """ + hits = [s for s in _run_steps(job) if "ci-step-ran.sh assert" in s["run"]] + assert len(hits) == 1, f"job '{job}' has {len(hits)} assert steps, expected exactly 1" + return hits[0] + + +def _marked(job: str): + """[(step, key)] for every step that records its own execution, in declaration order.""" + out = [] + for s in _run_steps(job): + m = _MARK.search(s["run"]) + if m: + out.append((s, m.group(1))) + return out + + +def _guard_buckets(job: str): + """(always_keys, gated_keys) as the guard's own argv spells them.""" + argv = _guard(job)["run"].split() + assert "--always" in argv and "--gated" in argv, argv + a, g = argv.index("--always"), argv.index("--gated") + return argv[a + 1:g], argv[g + 1:] + + +# Mirrors the `if:` every gated step in these jobs carries. Compared as a normalised string rather +# than by parsing the expression: what matters is that a step's gating and the guard's bucketing are +# the SAME condition, and any rewrite of one that is not mirrored in the other should be loud. +SKIP_GATE = ("steps.detect.outputs.docs_only!='true'&&steps.revalidate.outputs.skip!='true'") + + +def _is_gated(step) -> bool: + return re.sub(r"\s+", "", str(step.get("if", ""))) == SKIP_GATE + + +# ------------------------------------------------------------------------------------------------ +# STATIC +# ------------------------------------------------------------------------------------------------ + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_the_required_jobs_have_NO_expression_delimiter_in_any_run_body(job): + """The absolute rule from `review-verdict.yml`, extended to the two required build jobs. + + This is the cheaper and more general half of #756: the drop mechanism REQUIRES a delimiter in + the scalar, so a job with none is immune by construction and the runtime markers are a backstop + rather than the only line of defence. It is enforceable today because both jobs are already + delimiter-free (measured 2026-08-10: `test` 0, `migrations` 0, while `build` has 2, `api-docs` 1 + and `format` 1 — which is exactly why the rule is scoped to the required pair and not repo-wide). + + The escape hatch when a value really is needed is the step's `env:` block, which is interpolated + PER VALUE, so a payload that does not evaluate cannot take the body with it. + + The `run:` SCALAR AS PARSED, comments and all. A shell comment inside a `run:` body is NOT inert + — that is the whole #751 defect — so this must never filter comments out. Ordinary YAML comments + outside a `run:` body ARE inert and are not read here. + """ + offenders = [ + f"{s.get('name', '?')}: {m.group(1).strip()!r}" + for s in _run_steps(job) + for m in _EXPR.finditer(s["run"]) + ] + assert not offenders, ( + f"job '{job}' of docker-build.yml has an expression delimiter inside a run: body — " + f"{offenders}. This job writes a REQUIRED status context, and even in a comment a delimiter " + "is unsafe: the runner rewrites the WHOLE body into a format(...) call, and if the payload " + "does not parse it DROPS THE STEP and reports the job green — so a required check would " + "pass having done no work (ersatztv#751/#756). Pass the value in through the step's `env:` " + "block instead; to describe an expression in prose, name it rather than quoting the " + "delimiters." + ) + # ANTI-VACUITY. A walk that reached no bodies, or only the trivial ones, would make the + # assertion above green while proving nothing. Counted against the job's own step list read + # here, so a helper that silently stopped yielding steps is caught rather than rewarded. + declared = sum(1 for s in _steps(job) if isinstance(s, dict) and s.get("run")) + assert len(_run_steps(job)) == declared > 5, ( + f"the walk reached {len(_run_steps(job))} run: bodies but job '{job}' declares {declared}" + ) + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_every_consequential_run_step_marks_itself_as_its_FIRST_act(job): + """The completeness half — and the only test that catches a NEWLY ADDED step with no marker. + + A guard that checks a fixed list can go quietly incomplete: someone adds a `Test SPA (part 2)` + step, it is never marked, the guard never expects it, and a drop of exactly that step is + invisible again. So the expectation is DERIVED from the workflow rather than written down twice. + + EXEMPT: steps carrying `continue-on-error: true`. Those are advisory by construction (the + peak-anon sampler, the coverage summary) — the workflow already declares that their failure must + not redden the job, so their non-execution cannot be a fail-open either. Making them mandatory + would be asserting the opposite of what `continue-on-error` means. + + FIRST ACT, not merely present. A marker written at the END of a body records completion, not + execution — and this repo has legitimate early-exit paths. More importantly a marker further down + can be skipped by an early `exit 0` while the step did nothing, which is the fail-open again one + line lower. `set -euo pipefail` is allowed to precede it: it cannot fail, and it is what makes + the rest of the body honest. + """ + missing, late = [], [] + for s in _run_steps(job): + if s.get("continue-on-error") is True or "ci-step-ran.sh assert" in s["run"]: + continue + m = _MARK.search(s["run"]) + if not m: + missing.append(s.get("name", "?")) + continue + # By LINE, not by byte offset. The marker sits mid-line (the command is quoted and + # prefixed with $GITHUB_WORKSPACE), so slicing at `m.start()` counts the marker's OWN line + # prefix as a preceding command and reddens every correctly-written step. + lines = s["run"].splitlines() + at = next(i for i, ln in enumerate(lines) if _MARK.search(ln)) + preceding = [ + ln.strip() for ln in lines[:at] + if ln.strip() and not ln.strip().startswith("#") + ] + if [ln for ln in preceding if not ln.startswith("set -")]: + late.append((s.get("name", "?"), preceding)) + assert not missing, ( + f"these run: steps of the REQUIRED job '{job}' do not record that they executed: {missing}. " + "A step the runner drops concludes success, so without a marker its non-execution takes the " + "whole required context green having done no work (ersatztv#756). Add " + '`\"${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh\" mark ` as the step\'s first line and ' + "the key to the guard step's --always/--gated list." + ) + assert not late, ( + f"these steps of '{job}' mark themselves only after other commands have run: {late}. The " + "marker must be the first act, or a body that exits early records nothing while the guard " + "still expects it — or worse, records success for work that did not happen." + ) + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_the_guard_expects_EXACTLY_the_set_of_marked_keys_in_the_right_bucket(job): + """Set equality in BOTH directions, plus the bucket, because each failure is silent differently. + + A key marked but not expected → the guard never notices that step being dropped: a fail-open + that looks fully guarded. A key expected but not marked → the guard reddens on every single run, + which is fail-closed but reads as "this guard is broken" and is how a correct guard gets deleted. + + The BUCKET has to match the step's own `if:`. A gated step listed under `--always` reddens every + docs-only and already-validated run — the two paths whose entire purpose is to report green in + seconds. An always-run step listed under `--gated` stops being checked the moment either skip + gate fires, which is a fail-open on precisely the runs where least else is happening. + """ + marked = _marked(job) + keys = [k for _, k in marked] + assert len(keys) == len(set(keys)), ( + f"job '{job}' reuses a marker key: {[k for k in keys if keys.count(k) > 1]}. Two steps " + "sharing a key means either one satisfies the guard for both, so dropping one is invisible." + ) + always, gated = _guard_buckets(job) + assert sorted(always + gated) == sorted(keys), ( + f"job '{job}': the guard expects {sorted(always + gated)} but the steps mark " + f"{sorted(keys)}. Keys marked-but-unexpected are unguarded drops; keys " + "expected-but-unmarked redden every run." + ) + for step, key in marked: + want = "gated" if _is_gated(step) else "always" + got = "gated" if key in gated else "always" + assert want == got, ( + f"job '{job}': step {step.get('name')!r} is {want} (if: {step.get('if')!r}) but the " + f"guard lists its key {key!r} under --{got}." + ) + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_the_guard_is_the_LAST_step_carries_no_if_and_is_not_advisory(job): + """Position and condition, which together are what make the guard reachable and quiet. + + LAST, because a guard placed before a marked step reads a marker not yet written and fails on + every run. + + NO `if:` — a deliberate departure from the #751 guard's `if: always()`, and the thing most likely + to be "corrected" back. That job has one real step, so `always()` costs nothing. These jobs have + a dozen, and a genuine failure in an early one SKIPS every later step: an `always()` guard would + then report "these steps never executed: typecheck web-test build dotnet-test" on top of every + ordinary red build. That is the runner obeying its own gating, not a dropped step, and a guard + that cries wolf on every red build gets deleted. + + The default `if:` is `success()`, and the invariant that makes relying on it safe rather than + lucky: this step is skipped only when an earlier step FAILED, and that failure already fails the + job. So `guard skipped => job red`, and every path to a green job runs the guard. A dropped step + is invisible precisely because it concludes `success` — which keeps the job green and therefore + reaches here. + + NOT `continue-on-error`, which would let it observe the failure and go green anyway — the whole + defect, one attribute over. + """ + steps = _steps(job) + guard = _guard(job) + assert steps[-1] is guard, ( + f"the dropped-step guard is not the last step of '{job}' — it is at index " + f"{steps.index(guard)} of {len(steps)}, so any marked step after it would be unguarded and " + "the guard would read a marker that has not been written yet." + ) + assert "if" not in guard, ( + f"the '{job}' guard carries `if: {guard.get('if')!r}`. It must have none: the default " + "`success()` is what keeps it silent on ordinary red builds, and `always()` would make it " + "announce a false 'these steps never executed' on every failing run. See the comment above " + "the step for why this is a deliberate departure from the #751 guard." + ) + assert guard.get("continue-on-error") is not True, ( + f"the '{job}' guard is continue-on-error, so it detects the dropped step and lets the job go " + "green regardless — which is the defect it exists to remove." + ) + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_the_guards_OWN_body_cannot_be_dropped_by_the_mechanism_it_guards_against(job): + """A guard the guarded mechanism can silently delete is worse than no guard. + + Its absence is silent too: the job simply goes green with nothing checked, which is + indistinguishable from a clean run. #751 states the rule; here it is stronger than there, + because the body is a single command with no delimiter possible rather than 20 lines of prose + that must be kept clean by hand. + + The gate VALUES arrive through `env:`, which the runner interpolates per value — a bad payload + there fails that value, not the body. Both are additionally held to naming a real context by + test_every_workflow_expression_names_a_REAL_context_or_function in test_pr_changed_files.py. + """ + guard = _guard(job) + assert not _EXPR.search(guard["run"]), ( + f"the '{job}' guard's own run body contains an expression delimiter, so the runner can drop " + "the guard the same way it drops the steps the guard is watching — and that absence is " + "silent as well." + ) + assert guard["run"].strip().startswith("scripts/ci-step-ran.sh assert"), ( + f"the '{job}' guard is no longer a bare invocation: {guard['run']!r}. Keeping it to one " + "command is what makes a delimiter impossible rather than merely absent." + ) + assert set(guard.get("env", {})) == {"ETV_DOCS_ONLY", "ETV_REVALIDATE_SKIP"}, ( + f"the '{job}' guard's env: is {guard.get('env')} — it must pass exactly the two skip-gate " + "values the script reads, or the gated bucket is evaluated against stale/absent state." + ) + + +# ------------------------------------------------------------------------------------------------ +# BEHAVIOURAL — the guard's real command line, against markers written by the steps' real lines +# ------------------------------------------------------------------------------------------------ + + +def _mark_line(step) -> str: + """The step's OWN marker line, verbatim from the workflow. + + Extracted rather than rebuilt in Python ON PURPOSE. A test that composed the command itself + would keep passing after the workflow and the script drifted apart on the path, the quoting or + the sub-command — and that divergence is exactly the failure that makes the guard fail on every + run and then get deleted as broken. Running the real line proves the two agree by construction. + """ + line = next(ln for ln in step["run"].splitlines() if _MARK.search(ln)) + return line.strip() + + +def _env(tmp_path, **extra): + env = { + "PATH": os.environ["PATH"], + "GITHUB_WORKSPACE": str(REPO_ROOT), + "RUNNER_TEMP": str(tmp_path), + "GITHUB_JOB": "test", + "GITHUB_RUN_ID": "424242", + "GITHUB_RUN_ATTEMPT": "7", + } + env.update(extra) + return env + + +def _run(script: str, env): + return subprocess.run(["bash", "-c", script], cwd=REPO_ROOT, env=env, + capture_output=True, text=True) + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_the_guard_PASSES_when_every_step_marked_itself(job, tmp_path): + """The positive control. Without it, a guard that always failed would satisfy every case below. + + `GITHUB_JOB` is set to the job under test, so this also covers the marker file being keyed per + job: if it were not, the two jobs would share a file and one job's markers would answer for the + other's dropped steps. + """ + marks = [_mark_line(s) for s, _ in _marked(job)] + guard = _guard(job)["run"] + env = _env(tmp_path, GITHUB_JOB=job) + r = _run("\n".join(["set -e", *marks, guard]), env) + assert r.returncode == 0, ( + f"the '{job}' guard rejected a run in which every step marked itself — the steps and the " + f"guard disagree, so this would fail on every run.\n{r.stdout}\n{r.stderr}" + ) + assert "All" in r.stdout and "executed" in r.stdout, r.stdout + + +@pytest.mark.parametrize("job", REQUIRED_JOBS) +def test_dropping_ANY_single_step_FAILS_the_guard(job, tmp_path): + """Every marked step, one at a time — not a sample. + + An arbitrary sample gives false negatives here: the interesting drop is `Test` or the migration + replay, and a test that only omitted the first step would prove the guard catches the one case + that was never fail-open anyway. Dropping each key in turn is the only version that establishes + the property the issue asks for. + """ + marked = _marked(job) + guard = _guard(job)["run"] + for dropped_step, dropped_key in marked: + d = tmp_path / dropped_key + d.mkdir() + marks = [_mark_line(s) for s, k in marked if k != dropped_key] + r = _run("\n".join(["set -e", *marks, guard]), _env(d, GITHUB_JOB=job)) + assert r.returncode != 0, ( + f"job '{job}': the guard went GREEN with {dropped_step.get('name')!r} " + f"(key {dropped_key!r}) never having executed. That is a REQUIRED context reporting " + f"success having skipped that work — the exact fail-open of ersatztv#756.\n{r.stdout}" + ) + assert dropped_key in (r.stdout + r.stderr), ( + f"the guard failed but did not name the missing step {dropped_key!r}: {r.stdout}" + ) + + +@pytest.mark.parametrize("gate", ["ETV_DOCS_ONLY", "ETV_REVALIDATE_SKIP"]) +def test_a_fired_skip_gate_does_not_require_the_gated_steps(gate, tmp_path): + """The docs-only and already-validated paths must still report green in seconds. + + They are the reason these jobs are never `if:`-skipped at the JOB level (a skipped required + context is a state this repo deliberately does not rely on — ersatztv#416/#418), so a guard that + reddened them would make every docs-only PR unmergeable. Which is #751's user-visible symptom + arriving from the opposite direction, and worth a test rather than a comment. + """ + job = "test" + marks = [_mark_line(s) for s, k in _marked(job) if k in _guard_buckets(job)[0]] + guard = _guard(job)["run"] + r = _run("\n".join(["set -e", *marks, guard]), _env(tmp_path, GITHUB_JOB=job, **{gate: "true"})) + assert r.returncode == 0, ( + f"with {gate}=true the guard still demanded the gated steps, so every docs-only / " + f"already-validated run of a REQUIRED job would be red.\n{r.stdout}\n{r.stderr}" + ) + assert "Skip gate fired" in r.stdout, r.stdout + + +def test_a_fired_skip_gate_STILL_requires_the_ALWAYS_steps(tmp_path): + """The negative control for the test above — otherwise `ETV_DOCS_ONLY=true` would be a blanket + off-switch and the previous test would be passing for the wrong reason. + + This is the case that matters most on a docs-only run: the detect steps are the only things that + execute, so if their drop were unguarded the skip path would be entirely unchecked. + """ + guard = _guard("test")["run"] + r = _run("\n".join(["set -e", guard]), _env(tmp_path, ETV_DOCS_ONLY="true")) + assert r.returncode != 0, ( + "with ETV_DOCS_ONLY=true and NO steps marked at all, the guard passed — the skip gate is " + "acting as a blanket off-switch rather than as a narrowing of what is expected." + ) + assert "detect" in (r.stdout + r.stderr), r.stdout + + +def test_an_EMPTY_gate_value_requires_the_gated_steps(tmp_path): + """A dropped `detect` step leaves its outputs EMPTY, not 'false'. + + Reading empty as "skipped" would mean the one drop that disables the detect step also disables + the guard for everything downstream — the guard switching itself off in response to the very + failure it exists to catch. The direction has to be: anything that is not exactly `true` widens + what is required. + """ + guard = _guard("test")["run"] + r = _run("\n".join(["set -e", guard]), _env(tmp_path, ETV_DOCS_ONLY="", ETV_REVALIDATE_SKIP="")) + assert r.returncode != 0 + assert "dotnet-test" in (r.stdout + r.stderr), ( + f"empty gate values were read as a skip, so the gated steps went unchecked: {r.stdout}" + ) + + +def test_a_STALE_marker_from_another_run_cannot_satisfy_the_guard(tmp_path): + """RUNNER_TEMP is `/tmp` on these runners, not a private per-job directory (measured in #751). + + So a fixed marker name would live where another run can also write it, and a leftover file would + satisfy the guard on a run whose step was dropped — a silent PASS, the exact failure mode being + removed. The key is run id + attempt + job. + """ + marks = [_mark_line(s) for s, _ in _marked("test")] + guard = _guard("test")["run"] + # Run 1 marks everything. + first = _env(tmp_path, GITHUB_JOB="test", GITHUB_RUN_ID="111", GITHUB_RUN_ATTEMPT="1") + assert _run("\n".join(["set -e", *marks]), first).returncode == 0 + # Run 2 shares RUNNER_TEMP but marks nothing. It must NOT inherit run 1's markers. + second = _env(tmp_path, GITHUB_JOB="test", GITHUB_RUN_ID="222", GITHUB_RUN_ATTEMPT="1") + r = _run(guard, second) + assert r.returncode != 0, ( + "a marker file left by a DIFFERENT run satisfied the guard, so a run whose steps were all " + f"dropped would pass silently.\n{r.stdout}" + ) + # ...and a RETRY of run 1 must not inherit run 1's either. + retry = _env(tmp_path, GITHUB_JOB="test", GITHUB_RUN_ID="111", GITHUB_RUN_ATTEMPT="2") + assert _run(guard, retry).returncode != 0, ( + "a re-run inherited the first attempt's markers, so a step dropped only on the retry passes" + ) + # ...nor may the OTHER job in the same run inherit them. + sibling = _env(tmp_path, GITHUB_JOB="migrations", GITHUB_RUN_ID="111", GITHUB_RUN_ATTEMPT="1") + assert _run(_guard("migrations")["run"], sibling).returncode != 0, ( + "the two required jobs share one marker file, so one job's markers answer for the other's " + "dropped steps" + ) + + +def test_assert_with_no_expected_keys_REFUSES_instead_of_passing(tmp_path): + """The script's own anti-vacuity check, exercised rather than trusted. + + `assert` with an empty expectation list would print "All 0 expected step(s) executed" and exit 0 + — a guard that proves nothing while reporting that it proved everything. That is how a guard + ends up shipped and dead, which this repo has now done twice (#751's fence, #751's own guard). + """ + r = _run(f"{SCRIPT} assert", _env(tmp_path)) + assert r.returncode == 2, f"expected a usage refusal, got {r.returncode}: {r.stdout} {r.stderr}" + assert "no expected keys" in (r.stdout + r.stderr) + + +def test_mark_APPENDS_so_one_step_does_not_erase_its_predecessors(tmp_path): + """`>` instead of `>>` in the script would leave only the last step's key. + + The guard would then redden on every run — fail-closed, but it would look like the guard is + broken rather than like a real drop, and that is the state in which a correct guard gets removed. + """ + env = _env(tmp_path) + assert _run(f"{SCRIPT} mark alpha && {SCRIPT} mark beta", env).returncode == 0 + r = _run(f"{SCRIPT} assert --always alpha beta", env) + assert r.returncode == 0, f"the second mark erased the first: {r.stdout} {r.stderr}" + + +def test_a_key_is_matched_WHOLE_not_as_a_substring(tmp_path): + """`build` must not be satisfied by `web-build`, and `test` not by `web-test`. + + Both pairs are live key names in the `test` job, so a substring match would mean dropping the + real `Build` or `Test` step — the two most consequential steps in the whole workflow — is + invisible because an SPA step of a similar name ran. + """ + env = _env(tmp_path) + assert _run(f"{SCRIPT} mark web-build && {SCRIPT} mark web-test", env).returncode == 0 + r = _run(f"{SCRIPT} assert --always build", env) + assert r.returncode != 0, ( + "the key 'build' was satisfied by a marker for 'web-build' — a dropped `dotnet build` would " + "pass unnoticed" + )