From 2c6b0a39dea02427cbe30b2dc2cb47a92ed2bd3c Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 22:02:54 +0200 Subject: [PATCH 1/8] fix(756): per-step execution markers on docker-build.yml's two REQUIRED jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A run: body the runner declines to interpolate is DROPPED and the job still concludes success (#751). In review-verdict.yml that is fail-CLOSED; in docker-build.yml's `test` and `migrations` — the other two required contexts on main — it is fail-OPEN: the required check goes green having done no work. Every consequential run: step now records that it began, and a trailing guard fails the job when the expected set is incomplete. Per STEP, not per job as proposed: a job-start marker only proves step 1 ran, and the dangerous drop is `Test` or the migration replay. Refs #756 --- .gitea/workflows/docker-build.yml | 151 +++++- scripts/ci-step-ran.sh | 161 ++++++ scripts/tests/test_ci_dropped_step_guard.py | 512 ++++++++++++++++++++ 3 files changed, 806 insertions(+), 18 deletions(-) create mode 100755 scripts/ci-step-ran.sh create mode 100644 scripts/tests/test_ci_dropped_step_guard.py diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index b90794028..1d07c636b 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -122,14 +122,23 @@ 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: | + "${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 +150,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 +167,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 +224,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 +286,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 +393,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 +419,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 +437,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 +461,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 +496,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" + ) -- 2.47.3 From 7e769c1b1ffd4d47c2e4c32d2b5f2a857f752e15 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 22:08:29 +0200 Subject: [PATCH 2/8] docs(756): record the per-step marker contract and its build-lane measurement Adds ci.required-job-step-execution-markers, updates ci.workflow-run-body-no-expressions (whose rule: named #756 as the open residual and whose scope paragraph was tracking REQUIRED contexts rather than one file), and documents the guard in docs/ci-cd.md. Decisions-Edit: yes Refs #756 --- docs/ci-cd.md | 64 +++++++++++++++++-- docs/decisions/README.md | 3 +- .../ci/required-job-step-execution-markers.md | 58 +++++++++++++++++ .../ci/workflow-run-body-no-expressions.md | 8 ++- 4 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 docs/decisions/records/ci/required-job-step-execution-markers.md diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 83cd4732e..b9e06819a 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -605,6 +605,61 @@ CI-validated, so the tree-match check correctly declines. So the skip is a genui win (clean, up-to-date, un-rebased merges in quiet periods) — correct-but-conservative by construction, not a general dedup. It never fires unsafely; when in doubt it runs the full matrix. +### Dropped-step guard on the required jobs (ersatztv#756) + +`test` and `migrations` write the only two `docker-build.yml` contexts branch protection requires on +`main`. A step the runner declines to interpolate is **dropped, and the job still concludes +`success`** (ersatztv#751), so in these two jobs that failure is **fail-OPEN**: a required check +reports green having done no work. In `review-verdict.yml` the same drop is fail-closed — the status +is simply absent and the merge is blocked — which is why #751 fixed the safe direction first. + +Two independent mechanisms hold it, and neither is redundant: + +- **A static ban on expression delimiters** in any `run:` body of those two jobs. The drop mechanism + *requires* a delimiter in the scalar, so this makes the class unreachable rather than merely + detected. It is the cheaper and more general half, and it is only enforceable because both jobs + were 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 stops at the required pair and is not + repo-wide). When a step genuinely needs a value, pass it through the step's `env:` block, which is + interpolated **per value**, so a bad payload there cannot take the body with it. +- **Runtime per-step markers**, for a step that fails to run for any *other* reason. Every `run:` + step that is not `continue-on-error: true` calls + `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its **first act**, and the job's last + step calls `ci-step-ran.sh assert --always … --gated …`, which fails the job when an expected key + was never recorded. + +**Per step, not per job.** A marker written by the first step only proves the job *began*, which was +never in doubt. The drop that costs something is `Test`, `Build` or a migration replay — all well +past step one — so a job-level marker would have been a guard that cannot see the case it exists for. + +**The guard carries no `if:`, and that is deliberate.** The #751 guard uses `if: always()` because its +job has one real step. These have a dozen, and a genuine failure in an early step legitimately skips +every later one — an `always()` guard would then announce a false *"these steps never executed: +typecheck web-test build dotnet-test"* on top of every ordinary red build, and a guard that cries wolf +gets deleted. The default `if:` is `success()`, which is the wanted condition, and the invariant that +makes relying on it safe rather than lucky is: **the guard is skipped only when an earlier step +failed, and that 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 the guard. + +**Adding a step to either job?** Mark it, and add its key to that job's guard list in the right +bucket (`--always` for the two detect steps, `--gated` for anything carrying the docs-only / +already-validated `if:`). `scripts/tests/test_ci_dropped_step_guard.py` derives the expected set from +the workflow, so an unmarked step or a bucket mismatch is a red — it does not rely on anyone +remembering. + +**The marker path is keyed on job + run id + attempt**, because `RUNNER_TEMP` is `/tmp` on these +runners, not a private per-job directory. A fixed name would let a stale file satisfy the guard on a +run whose step *was* dropped — a silent pass, the exact failure being removed. + +**The premise was re-measured on the build lane.** The whole thing rests on the runner still executing +a later step after dropping an earlier one. #751 established that on the `small` lane; these jobs run +in a `container:` on `ubuntu-latest`, so it was measured there rather than assumed — scratch PR #765 +(Gitea 1.27.1, 2026-08-10) reintroduced the exact #751 defect in the `test` job's `revalidate` step. +MEASUREMENT_PLACEHOLDER + +Full rationale: `docs/decisions/records/ci/required-job-step-execution-markers.md`. + ### `docs-reminder` job (non-blocking, PR-only — in `pr-checks.yml`) A lightweight nudge that enforces the CLAUDE.md "docs-update is part of done" rule for the @@ -1141,10 +1196,11 @@ dropped. `README.md` is deliberately not listed — ordinary prose, no enforceme #706's known residual returns with the working fence: while `rt_ok` was never `yes`, route 1 was closed by accident. -**The guard is scoped to this file, and that is a gap rather than a decision** — `docker-build.yml`'s -`test` and `migrations` jobs are also required contexts, and there a dropped step is **fail-OPEN**: the -required check goes green having done no work, which is strictly worse than an absent status (compare -#684). Tracked as ersatztv#756. +**That gap is now closed** — `docker-build.yml`'s `test` and `migrations` jobs are also required +contexts, and there a dropped step is **fail-OPEN**: the required check goes green having done no work, +which is strictly worse than an absent status (compare #684). ersatztv#756 gave those two jobs +per-**step** execution markers and extended the delimiter ban to them; see +"Dropped-step guard on the required jobs" above. It lives in its **own workflow file** on purpose: `pr-checks.yml` sets `cancel-in-progress: true`, and a cancelled run there would leave an exempt PR with no status and no further push to diff --git a/docs/decisions/README.md b/docs/decisions/README.md index dfab343f5..f53595829 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -56,6 +56,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.monitor-armed-at-pr-open` | Arm a CI monitor on the PR head sha the moment the PR opens, polling the commit-status endpoint — not at the end of the work. | 2026-07-21 | [link](records/ci/monitor-armed-at-pr-open.md) | | `ci.no-host-health-gating` | Push when your work is validated — never SSH to bumblebee to sample load/RAM first, and never hand-schedule around other sessions' runs. | 2026-07-21 | [link](records/ci/no-host-health-gating.md) | | `ci.peak-anon-measurement` | The `test` job's headline memory figure is a sampled high-water mark of cgroup `anon`, produced by `scripts/ci-peak-anon.sh`; `memory.peak` and the end-of-job `anon`/`file` split are kept only as a cache-inflated reference. | 2026-07-19 | [link](records/ci/peak-anon-measurement.md) | +| `ci.required-job-step-execution-markers` | A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`'s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job's LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, NO expression delimiter may appear in any `run:` body of those two jobs — the drop mechanism requires one, so this makes the class unreachable rather than merely caught; pass values in through the step's `env:`, which is interpolated per value. Both halves are scoped to the REQUIRED pair only: `build`, `api-docs` and `format` legitimately interpolate into `run:` bodies and a repo-wide ban would be false. | 2026-08-10 | [link](records/ci/required-job-step-execution-markers.md) | | `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) | | `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) | | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | @@ -65,7 +66,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. Abstaining is a handoff, not a stall, and that is the property the design rests on: every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops and the last run writes the final answer. `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | | `ci.web-test-per-test-timeouts` | Give heavy-render web tests an explicit per-test vitest timeout (e.g. 15s); never raise the global default to fix one slow test. | 2026-07-21 | [link](records/ci/web-test-per-test-timeouts.md) | -| `ci.workflow-run-body-no-expressions` | A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step's `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard's own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing is NOT yet a repo-wide property: `docker-build.yml`'s `test` and `migrations` jobs are also required contexts and a dropped step there is fail-OPEN (the required check goes green having done no work), which is strictly worse than here; extending the guard to them is ersatztv#756, not something this record claims is done. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | +| `ci.workflow-run-body-no-expressions` | A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step's `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard's own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`'s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file's delimiter ban to those two jobs. It is still not a repo-wide property: `build`, `api-docs` and `format` interpolate into `run:` bodies legitimately and are not required contexts, so a drop there fails no gate. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | | `concurrency.diff-scalar-fanout` | The frozen Block optimistic-concurrency recipe (api-conventions §7a) fans out to Collection/Playout×2/MultiCollection/RerunCollection, keeping a guard-returned `PreconditionFailedError` out of any handler's generic `catch(Exception)`→422 mapping, and preserving each aggregate's existing `SaveChangesAsync() > 0` gate semantics under the new unconditional `Version++`. | 2026-07-11 | [link](records/concurrency/diff-scalar-fanout.md) | | `concurrency.etag-rotation-completion` | Every handler that mutates a versioned root's editor-visible config state must bump `Version` (rotating the ETag) with no per-aggregate carve-outs, short-circuiting on a genuine no-op before the bump so idempotent re-submits don't fire spurious rebuild fan-out; `SaveChangesForcingVersion` rebases the retry (stored + pending delta), never adopts the stored token verbatim. | 2026-07-12 | [link](records/concurrency/etag-rotation-completion.md) | | `concurrency.force-write-non-ifmatch` | Any handler that leaves a versioned root `Modified` or `Deleted` but takes no `If-Match` (deletes, item add/remove bumpers, scalar-config writers) must save through `ConcurrencyExtensions.SaveChangesForcingVersion` — force-write past a concurrent `Version` bump rather than throw an unhandled `DbUpdateConcurrencyException` (500). | 2026-07-12 | [link](records/concurrency/force-write-non-ifmatch.md) | diff --git a/docs/decisions/records/ci/required-job-step-execution-markers.md b/docs/decisions/records/ci/required-job-step-execution-markers.md new file mode 100644 index 000000000..e7f8075fc --- /dev/null +++ b/docs/decisions/records/ci/required-job-step-execution-markers.md @@ -0,0 +1,58 @@ +--- +key: ci.required-job-step-execution-markers +title: '2026-08-10 — every consequential `run:` step in docker-build.yml''s two REQUIRED jobs records that it executed, and a trailing guard fails the job when the set is incomplete (#756)' +status: active +since: '2026-08-10' +supersedes: none +superseded-by: none +rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, NO expression delimiter may appear in any `run:` body of those two jobs — the drop mechanism requires one, so this makes the class unreachable rather than merely caught; pass values in through the step''s `env:`, which is interpolated per value. Both halves are scoped to the REQUIRED pair only: `build`, `api-docs` and `format` legitimately interpolate into `run:` bodies and a repo-wide ban would be false.' +signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py` · issues: #756, #751, #684' +mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge; the path is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID`/`GITHUB_RUN_ATTEMPT` because `RUNNER_TEMP` is `/tmp` here and a stale marker would make the guard PASS on a dropped run. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' +--- + +**Why per step, when #756 proposed per job.** A job-start marker answers "did this job begin", which +was never in doubt. The fail-open it is supposed to close is a required context reporting success +while the work inside it did not happen, and the steps that carry that work are `Test`, `Build` and +the two migration replays — all of them well past step one. A guard positioned where it cannot see +the case it was built for is the "guard that never executed" failure one level up, and this repo has +now shipped that twice in the same file (#751's retarget fence, and #751's own guard). + +**Why a script rather than an inline body, when #751 chose inline.** Two reasons and the second is +the load-bearing one. The path literal exists once, so the write and the read cannot drift — #751 +carries it twice and spends real test effort proving the copies agree, because a divergence reddens +every run and then gets deleted as broken. And a one-line `run: scripts/ci-step-ran.sh …` cannot +contain an expression delimiter, so the mechanism being guarded against cannot drop the guard. #751's +own record names that as the stronger construction and settled for inline only because its +measurement showed it was not required there. + +**Why a script is acceptable here and 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 +carry this reasoning back into the gate workflow. + +**The premise was re-measured on the BUILD lane, not inherited.** The whole guard rests on the runner +still executing a LATER step after dropping an earlier one. #751 established that on the `small` +lane; these two jobs run in a `container:` on `ubuntu-latest`, which is a different lane, so assuming +it transfers would be the same shape of mistake the guard exists to catch. Measured by scratch PR +#765 (Gitea 1.27.1, 2026-08-10), which reintroduced the exact #751 defect — an invalid expression +payload inside a shell comment — in the `test` job's `revalidate` step: see docs/ci-cd.md for the +recorded outcome. The negative control is #756's own PR, where all twelve steps mark and the job is +green. + +**The two halves are deliberately different in kind, and neither is redundant.** The delimiter ban is +static and absolute, and it makes the defect class UNREACHABLE in these jobs rather than merely +detected — it is the cheaper and more general half, and it is enforceable today only because both +jobs are already delimiter-free (measured 2026-08-10: `test` 0, `migrations` 0; `build` 2, `api-docs` +1, `format` 1, which is why the rule stops at the required pair). The runtime markers catch a step +that fails to run for any OTHER reason, including reasons not yet met. Keeping only the static half +would be trusting that this is the only way a step can vanish, which is exactly the assumption #751 +falsified about shell comments. + +**What this does not claim.** The guard proves a step STARTED, never that it did its work correctly +— that is what the step's own exit status is for. It does not cover `uses:` steps, which are not +`run:` bodies and cannot be dropped this way. And it does not cover the non-required jobs; a drop in +`build`, `api-docs` or `format` still goes green, which is a smaller cost (no required context lies) +but is not zero. Extending it there would mean either giving up their legitimate interpolation or +moving those bodies into `scripts/`, and neither was worth doing blind. diff --git a/docs/decisions/records/ci/workflow-run-body-no-expressions.md b/docs/decisions/records/ci/workflow-run-body-no-expressions.md index f7e65732f..975d462b7 100644 --- a/docs/decisions/records/ci/workflow-run-body-no-expressions.md +++ b/docs/decisions/records/ci/workflow-run-body-no-expressions.md @@ -5,7 +5,7 @@ status: active since: '2026-08-06' supersedes: none superseded-by: none -rule: 'A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step''s `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard''s own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing is NOT yet a repo-wide property: `docker-build.yml`''s `test` and `migrations` jobs are also required contexts and a dropped step there is fail-OPEN (the required check goes green having done no work), which is strictly worse than here; extending the guard to them is ersatztv#756, not something this record claims is done.' +rule: 'A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step''s `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard''s own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`''s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file''s delimiter ban to those two jobs. It is still not a repo-wide property: `build`, `api-docs` and `format` interpolate into `run:` bodies legitimately and are not required contexts, so a drop there fails no gate.' signals: 'Unable to interpolate expression format(, step never ran but job green, missing Run Main step marker, review-verdict/h10 absent after a green run, docs-only PR unmergeable, Renovate PR unmergeable, exemption stopped working, expression in a shell comment, workflow comment changed behaviour · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #751, #706, #748' mechanics: '`RAN_MARKER` written at the top of the classify step and asserted by the `Assert the classifier actually executed` step (`if: always()`, expression-free body, `exit 1` on a missing marker); static guards `test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body` (raw scalar, absolute, gate file only) and `test_every_workflow_expression_names_a_REAL_context_or_function` (repo-wide, allow-list of contexts/functions) plus `test_a_dropped_classify_step_FAILS_the_job_instead_of_going_green` (pins marker path agreement and guard ordering across ONE yaml parse)' --- @@ -40,6 +40,12 @@ false and would be deleted the first time it got in someone's way. `review-verdi absolute rule on two counts: it writes the branch-protection-required status, and its `run:` bodies are ~700 lines of dense prose — the only place the delimiter has ever appeared by accident. +The first of those two counts turned out to apply elsewhere as well, and #756 acted on it: the +absolute ban now also covers `docker-build.yml`'s `test` and `migrations` jobs, which write the other +two required contexts and were delimiter-free already, so the rule cost nothing to impose there. The +remaining 4 occurrences sit in jobs that gate nothing. Read this paragraph as scoping the rule to +REQUIRED contexts rather than to this one file — that is what it was always tracking. + **The probe found a SECOND, independent reason the gate posted nothing**, and it is why fixing the interpolation alone would not have restored the exemptions: a page past the end of `/issues/{n}/timeline` is JSON `null`, not `[]`, so the retarget fence never trusted its count for ANY -- 2.47.3 From b0ed61d3d5c1a7eca630d41a2ae8bf824897d2bb Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 22:33:17 +0200 Subject: [PATCH 3/8] =?UTF-8?q?fix(756):=20round-2=20=E2=80=94=20nine=20fi?= =?UTF-8?q?ndings=20from=20two=20independent=20cold=20reviews?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .gitea/workflows/docker-build.yml | 30 +++- docs/ci-cd.md | 60 +++++-- docs/decisions/README.md | 2 +- .../ci/required-job-step-execution-markers.md | 27 ++- scripts/ci-step-ran.sh | 86 +++++++-- scripts/tests/test_ci_dropped_step_guard.py | 169 +++++++++++++++--- 6 files changed, 305 insertions(+), 69 deletions(-) diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index 1d07c636b..5c13ba352 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -504,12 +504,11 @@ jobs: # # 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. + # exactly one real step, so there is no ordinary red for it to talk over. Here a genuine + # failure in an early step (a failing `dotnet build`, a MySql replay error) SKIPS every later + # step — an `always()` guard would then announce "these steps never executed: sqlite mysql" 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 @@ -731,11 +730,26 @@ jobs: cache-from: type=registry,ref=192.168.1.95:3000/timothy/ersatztv:buildcache cache-to: type=registry,ref=192.168.1.95:3000/timothy/ersatztv:buildcache,mode=max,ignore-error=true + # THE TWO VALUES COME IN THROUGH `env:`, NOT INLINE (ersatztv#756). This step runs AFTER + # `Build and push`, so on a `v*` tag the image is already in the registry as the release + # candidate — and it is this smoke run that decides whether the candidate was ever booted at + # all. A stray expression delimiter anywhere in this body (a comment is not inert — #751) would + # DROP the step and conclude the job `success`: a candidate published, never smoke-tested, and + # `DeployStack jazz-media` promotes exactly that image. `env:` is interpolated PER VALUE, so a + # bad payload there fails that value instead of taking the whole body with it, and with the + # body delimiter-free the class is unreachable here — held by + # test_the_delimiter_banned_jobs_have_NO_expression_delimiter_in_any_run_body. + # + # This job gets the ban but NOT the per-step markers: it is not a required context, so the + # marker machinery would buy little beyond what removing the delimiters already buys. - name: Smoke + IPTV E2E (assert key endpoints) if: ${{ (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v')) && steps.detect.outputs.docs_only != 'true' }} + env: + SMOKE_SHORT_SHA: ${{ steps.meta.outputs.short }} + SMOKE_RUN_ID: ${{ github.run_id }} run: | - IMG="${IMAGE}:${{ steps.meta.outputs.short }}" - NAME="etv-smoke-${{ github.run_id }}" + IMG="${IMAGE}:${SMOKE_SHORT_SHA}" + NAME="etv-smoke-${SMOKE_RUN_ID}" trap 'docker rm -f "$NAME" >/dev/null 2>&1 || true' EXIT echo "Pulling ${IMG}" docker pull "$IMG" diff --git a/docs/ci-cd.md b/docs/ci-cd.md index b9e06819a..80e25f3e9 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -615,13 +615,22 @@ is simply absent and the merge is blocked — which is why #751 fixed the safe d Two independent mechanisms hold it, and neither is redundant: -- **A static ban on expression delimiters** in any `run:` body of those two jobs. The drop mechanism - *requires* a delimiter in the scalar, so this makes the class unreachable rather than merely - detected. It is the cheaper and more general half, and it is only enforceable because both jobs - were 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 stops at the required pair and is not - repo-wide). When a step genuinely needs a value, pass it through the step's `env:` block, which is - interpolated **per value**, so a bad payload there cannot take the body with it. +- **A static ban on expression delimiters** in any `run:` body of `test`, `migrations` **and + `build`**. The drop mechanism *requires* an opener in the scalar, so this makes the class + unreachable rather than merely detected — and it is the raw `${{` opener that is banned, not a + well-formed pair, because an unclosed one triggers the same rewrite. When a step genuinely needs a + value, pass it through the step's `env:` block, which is interpolated **per value**, so a bad + payload there cannot take the body with it. + + **Why `build` is in the ban although it is not a required context.** Its one delimiter-bearing body + was `Smoke + IPTV E2E`, which runs *after* `Build and push` — so on a `v*` tag the image is already + in the registry as the release candidate and that step is what decides whether the candidate was + ever booted. A drop there publishes an unsmoked candidate, reports green, and `DeployStack + jazz-media` promotes exactly that image. Its two payloads moved into the step's `env:`, so the ban + cost nothing. `functional-e2e` is delimiter-free too but is deliberately **not** banned: it is + advisory by declaration, and the rule is "ban where a drop is consequential", not "ban wherever it + is currently free". `api-docs` and `format` keep one `github.base_ref` each in a detect step and + gate nothing that ships. - **Runtime per-step markers**, for a step that fails to run for any *other* reason. Every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its **first act**, and the job's last @@ -636,7 +645,8 @@ past step one — so a job-level marker would have been a guard that cannot see job has one real step. These have a dozen, and a genuine failure in an early step legitimately skips every later one — an `always()` guard would then announce a false *"these steps never executed: typecheck web-test build dotnet-test"* on top of every ordinary red build, and a guard that cries wolf -gets deleted. The default `if:` is `success()`, which is the wanted condition, and the invariant that +gets deleted. (`migrations` is smaller — six marked steps — but the same argument applies, and its +guard comment is worded for its own keys rather than copied from `test`'s.) The default `if:` is `success()`, which is the wanted condition, and the invariant that makes relying on it safe rather than lucky is: **the guard is skipped only when an earlier step failed, and that 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 @@ -648,15 +658,41 @@ already-validated `if:`). `scripts/tests/test_ci_dropped_step_guard.py` derives the workflow, so an unmarked step or a bucket mismatch is a red — it does not rely on anyone remembering. -**The marker path is keyed on job + run id + attempt**, because `RUNNER_TEMP` is `/tmp` on these -runners, not a private per-job directory. A fixed name would let a stale file satisfy the guard on a -run whose step *was* dropped — a silent pass, the exact failure being removed. +**The marker path is keyed on job + run id + attempt** — and be precise about why, because the +obvious justification is a #751 measurement that does *not* transfer. #751 found `RUNNER_TEMP` to be +`/tmp` and called it "not a private per-job directory", but that was taken on `review-verdict.yml`, +which runs *without* a `container:`. These two jobs run **inside** the CI toolchain image, so their +`/tmp` is the job container's own and starts empty. The fresh container is therefore what actually +rules out a stale marker here; the keying is defence in depth against a lane change nobody would +think to re-check this against. `GITHUB_JOB` and `GITHUB_RUN_ID` are *measured* present and the +script refuses without them rather than falling back to a name other runs share. +`GITHUB_RUN_ATTEMPT` is **not** measured — it appears nowhere in the probe's 2.9 MB job log — so it +warns and defaults instead: these are required checks, and reddening every merge over an unverified +variable is a worse trade than a residual that needs an unset variable *and* a non-fresh container to +bite. **The premise was re-measured on the build lane.** The whole thing rests on the runner still executing a later step after dropping an earlier one. #751 established that on the `small` lane; these jobs run in a `container:` on `ubuntu-latest`, so it was measured there rather than assumed — scratch PR #765 (Gitea 1.27.1, 2026-08-10) reintroduced the exact #751 defect in the `test` job's `revalidate` step. -MEASUREMENT_PLACEHOLDER +Recorded outcome (job `test`, run 1910, 20:03:49→20:13:31Z — a full 9m42s heavy run, so `Build` and +`Test` really executed): + +- `Unable to interpolate expression 'format('# PROBE ONLY … {0}\n…', pr number)'` at 20:04:06 — the + step was **dropped**, exactly as #751 describes, and it reported conclusion `success`. +- **Every one of the eleven later marked steps still ran**, through `build` and `dotnet-test`. The + premise holds on this lane. +- The guard ran at 20:13:29, reported `These steps of job 'test' never executed: revalidate`, and was + the **only** ❌ in the entire job log — every other step succeeded. Without it this run would have + concluded `success` having never executed that step, which is precisely the fail-open being closed. +- Incidental but kept: the dropped step's output arrived as `ETV_REVALIDATE_SKIP:` **empty**, not + `false` — the case the guard must read as "widen what is required", never as a skip. + +**The positive control is the same run's `migrations` job**, which the probe did not touch: it marked +all six steps, the guard reported `All 6 expected step(s) executed: detect revalidate restore build +sqlite mysql`, and the job concluded **success**. So one run demonstrates both directions on the build +lane — a drop caught and reddened, and a clean job passing. The twelve-step `test` positive control is +this change's own CI run. Full rationale: `docs/decisions/records/ci/required-job-step-execution-markers.md`. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f53595829..97dbc63a3 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -56,7 +56,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.monitor-armed-at-pr-open` | Arm a CI monitor on the PR head sha the moment the PR opens, polling the commit-status endpoint — not at the end of the work. | 2026-07-21 | [link](records/ci/monitor-armed-at-pr-open.md) | | `ci.no-host-health-gating` | Push when your work is validated — never SSH to bumblebee to sample load/RAM first, and never hand-schedule around other sessions' runs. | 2026-07-21 | [link](records/ci/no-host-health-gating.md) | | `ci.peak-anon-measurement` | The `test` job's headline memory figure is a sampled high-water mark of cgroup `anon`, produced by `scripts/ci-peak-anon.sh`; `memory.peak` and the end-of-job `anon`/`file` split are kept only as a cache-inflated reference. | 2026-07-19 | [link](records/ci/peak-anon-measurement.md) | -| `ci.required-job-step-execution-markers` | A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`'s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job's LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, NO expression delimiter may appear in any `run:` body of those two jobs — the drop mechanism requires one, so this makes the class unreachable rather than merely caught; pass values in through the step's `env:`, which is interpolated per value. Both halves are scoped to the REQUIRED pair only: `build`, `api-docs` and `format` legitimately interpolate into `run:` bodies and a repo-wide ban would be false. | 2026-08-10 | [link](records/ci/required-job-step-execution-markers.md) | +| `ci.required-job-step-execution-markers` | A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`'s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job's LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step's `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships. | 2026-08-10 | [link](records/ci/required-job-step-execution-markers.md) | | `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) | | `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) | | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | diff --git a/docs/decisions/records/ci/required-job-step-execution-markers.md b/docs/decisions/records/ci/required-job-step-execution-markers.md index e7f8075fc..22100c61e 100644 --- a/docs/decisions/records/ci/required-job-step-execution-markers.md +++ b/docs/decisions/records/ci/required-job-step-execution-markers.md @@ -5,7 +5,7 @@ status: active since: '2026-08-10' supersedes: none superseded-by: none -rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, NO expression delimiter may appear in any `run:` body of those two jobs — the drop mechanism requires one, so this makes the class unreachable rather than merely caught; pass values in through the step''s `env:`, which is interpolated per value. Both halves are scoped to the REQUIRED pair only: `build`, `api-docs` and `format` legitimately interpolate into `run:` bodies and a repo-wide ban would be false.' +rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step''s `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships.' signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py` · issues: #756, #751, #684' mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge; the path is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID`/`GITHUB_RUN_ATTEMPT` because `RUNNER_TEMP` is `/tmp` here and a stale marker would make the guard PASS on a dropped run. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' --- @@ -37,9 +37,12 @@ still executing a LATER step after dropping an earlier one. #751 established tha lane; these two jobs run in a `container:` on `ubuntu-latest`, which is a different lane, so assuming it transfers would be the same shape of mistake the guard exists to catch. Measured by scratch PR #765 (Gitea 1.27.1, 2026-08-10), which reintroduced the exact #751 defect — an invalid expression -payload inside a shell comment — in the `test` job's `revalidate` step: see docs/ci-cd.md for the -recorded outcome. The negative control is #756's own PR, where all twelve steps mark and the job is -green. +payload inside a shell comment — in the `test` job's `revalidate` step. The step was dropped, all +eleven later marked steps still ran (through `dotnet-test`), and the guard was the ONLY failing step +in the job — so without it that run would have concluded `success` having skipped a step. The SAME +run supplies the positive control on the same lane: its untouched `migrations` job marked all six +steps, reported `All 6 expected step(s) executed`, and concluded `success`. Full log extract in +docs/ci-cd.md. **The two halves are deliberately different in kind, and neither is redundant.** The delimiter ban is static and absolute, and it makes the defect class UNREACHABLE in these jobs rather than merely @@ -52,7 +55,15 @@ falsified about shell comments. **What this does not claim.** The guard proves a step STARTED, never that it did its work correctly — that is what the step's own exit status is for. It does not cover `uses:` steps, which are not -`run:` bodies and cannot be dropped this way. And it does not cover the non-required jobs; a drop in -`build`, `api-docs` or `format` still goes green, which is a smaller cost (no required context lies) -but is not zero. Extending it there would mean either giving up their legitimate interpolation or -moving those bodies into `scripts/`, and neither was worth doing blind. +`run:` bodies and cannot be dropped this way. + +An earlier draft dismissed the non-required jobs as "a smaller cost (no required context lies)", and +cold review showed that was false for the one that matters. `build`'s only delimiter-bearing body was +`Smoke + IPTV E2E`, which runs AFTER `Build and push`: on a `v*` tag the candidate image is already +published, and that step is the only thing that boots it. A drop there ships an unsmoked release +candidate under a green tick, and prod promotion pulls exactly that image. It was also the cheap case +— both payloads were plain values, so moving them into `env:` cost nothing and let `build` join the +ban. The claim not to repeat is the draft's dichotomy ("give up interpolation or move into +`scripts/`"); the `env:` escape hatch this record prescribes was the answer all along. What genuinely +remains uncovered is `api-docs` and `format`, whose one `github.base_ref` each sits in a detect step +that gates nothing that ships, and `functional-e2e`, which is advisory by declaration. diff --git a/scripts/ci-step-ran.sh b/scripts/ci-step-ran.sh index c1dacf695..08f8e9fc2 100755 --- a/scripts/ci-step-ran.sh +++ b/scripts/ci-step-ran.sh @@ -13,8 +13,15 @@ # 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. +# 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: @@ -35,12 +42,18 @@ # 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. +# 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 @@ -59,12 +72,40 @@ 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:-nojob}" \ - "${GITHUB_RUN_ID:-norunid}" \ - "${GITHUB_RUN_ATTEMPT:-1}" + "$GITHUB_JOB" "$GITHUB_RUN_ID" "$attempt" } cmd_mark() { @@ -75,7 +116,12 @@ cmd_mark() { # 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)" + 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() { @@ -114,7 +160,10 @@ cmd_assert() { fi local marker - marker="$(marker_path)" + # `|| 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 @@ -126,6 +175,17 @@ cmd_assert() { 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 diff --git a/scripts/tests/test_ci_dropped_step_guard.py b/scripts/tests/test_ci_dropped_step_guard.py index ef13b58ef..47b7e2c9b 100644 --- a/scripts/tests/test_ci_dropped_step_guard.py +++ b/scripts/tests/test_ci_dropped_step_guard.py @@ -40,8 +40,30 @@ SCRIPT = REPO_ROOT / "scripts" / "ci-step-ran.sh" # 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") +# Per-step markers apply to the two REQUIRED contexts, where a dropped step is fail-OPEN. +MARKED_JOBS = ("test", "migrations") +# The delimiter ban is WIDER than the marker set, and the extra job is not an afterthought. +# `build`'s "Smoke + IPTV E2E" step runs AFTER `Build and push`, so on a `v*` tag the image is +# already in the registry as the release candidate and this step is what decides whether it was ever +# booted. A drop there publishes an unsmoked candidate and goes green, and `DeployStack jazz-media` +# promotes exactly that image — not a "smaller cost than a required context", which is what an +# earlier draft of the decision record claimed. Its two payloads moved into the step's `env:`, which +# is the free half of the escape hatch, so the ban costs nothing there. +# +# `functional-e2e` is deliberately NOT here even though it is delimiter-free today: it is advisory by +# declaration (not a required check, not a `needs:` of `build`), so the rule stays "ban where a drop +# is consequential" rather than "ban wherever it happens to be free right now". +# `api-docs` and `format` keep one delimiter each, both `github.base_ref` in a detect step, and gate +# nothing that ships. +DELIMITER_BAN_JOBS = ("test", "migrations", "build") + +# THE RAW OPENER, not a closed `${{ … }}` pair — found by cold review. The runner's rewrite is +# triggered by the OPENER; a closed-pair regex therefore misses `# ${{` with no closer, which would +# sail through an "absolute" ban and still drop the step. Nothing in these jobs may contain the +# opener at all, so matching it directly is both simpler and strictly stronger. `_EXPR` is kept for +# reporting the payload of a well-formed one in the failure message. +_OPENER = re.compile(r"\$\{\{") _EXPR = re.compile(r"\$\{\{(.*?)\}\}", re.S) _MARK = re.compile(r'ci-step-ran\.sh"?\s+mark\s+(\S+)') @@ -111,8 +133,8 @@ def _is_gated(step) -> bool: # ------------------------------------------------------------------------------------------------ -@pytest.mark.parametrize("job", REQUIRED_JOBS) -def test_the_required_jobs_have_NO_expression_delimiter_in_any_run_body(job): +@pytest.mark.parametrize("job", DELIMITER_BAN_JOBS) +def test_the_delimiter_banned_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 @@ -128,11 +150,12 @@ def test_the_required_jobs_have_NO_expression_delimiter_in_any_run_body(job): — 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"]) - ] + offenders = [] + for s in _run_steps(job): + for m in _OPENER.finditer(s["run"]): + closed = _EXPR.match(s["run"], m.start()) + payload = closed.group(1).strip() if closed else "" + offenders.append(f"{s.get('name', '?')}: {payload!r}") 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 " @@ -146,12 +169,12 @@ def test_the_required_jobs_have_NO_expression_delimiter_in_any_run_body(job): # 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, ( + assert len(_run_steps(job)) == declared >= 3, ( f"the walk reached {len(_run_steps(job))} run: bodies but job '{job}' declares {declared}" ) -@pytest.mark.parametrize("job", REQUIRED_JOBS) +@pytest.mark.parametrize("job", MARKED_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. @@ -203,7 +226,7 @@ def test_every_consequential_run_step_marks_itself_as_its_FIRST_act(job): ) -@pytest.mark.parametrize("job", REQUIRED_JOBS) +@pytest.mark.parametrize("job", MARKED_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. @@ -228,7 +251,23 @@ def test_the_guard_expects_EXACTLY_the_set_of_marked_keys_in_the_right_bucket(jo f"{sorted(keys)}. Keys marked-but-unexpected are unguarded drops; keys " "expected-but-unmarked redden every run." ) + # AN UNRECOGNISED `if:` IS REJECTED, never silently bucketed — found by both reviewers. The + # protocol only knows two conditions: absent (always runs) and exactly the skip gate. A marked + # step carrying a third condition (`if: github.event_name == 'push'`, or the `always() && ` + # spelling the peak-anon steps already use) would fall through to "always", the suite would go + # green, and the guard would then demand a step the runner legitimately skipped — reddening a + # REQUIRED context and deadlocking `main`. There is already a near-miss in this file: `Report + # peak container memory` carries that third spelling and escapes only because it is + # `continue-on-error: true` and therefore exempt from marking. for step, key in marked: + cond = re.sub(r"\s+", "", str(step.get("if", ""))) + assert cond in ("", SKIP_GATE), ( + f"job '{job}': marked step {step.get('name')!r} has an `if:` the guard protocol does not " + f"model ({step.get('if')!r}). Only 'absent' and the exact skip gate are understood; " + "anything else would be bucketed as --always and would fail the job on a run where the " + "step is legitimately skipped. Extend the protocol deliberately, or leave the step " + "unmarked." + ) want = "gated" if _is_gated(step) else "always" got = "gated" if key in gated else "always" assert want == got, ( @@ -237,7 +276,7 @@ def test_the_guard_expects_EXACTLY_the_set_of_marked_keys_in_the_right_bucket(jo ) -@pytest.mark.parametrize("job", REQUIRED_JOBS) +@pytest.mark.parametrize("job", MARKED_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. @@ -279,7 +318,7 @@ def test_the_guard_is_the_LAST_step_carries_no_if_and_is_not_advisory(job): ) -@pytest.mark.parametrize("job", REQUIRED_JOBS) +@pytest.mark.parametrize("job", MARKED_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. @@ -293,7 +332,7 @@ def test_the_guards_OWN_body_cannot_be_dropped_by_the_mechanism_it_guards_agains 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"]), ( + assert not _OPENER.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." @@ -302,9 +341,19 @@ def test_the_guards_OWN_body_cannot_be_dropped_by_the_mechanism_it_guards_agains 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"}, ( + # THE VALUES, not just the names — found by cold review. Asserting the keys alone accepts + # `ETV_DOCS_ONLY: ${{ steps.detect.outputs.doc_only }}` (note the typo), which names a real + # context so the repo-wide expression check passes it too. The guard would then read an EMPTY + # value on a docs-only run, demand the gated steps that were correctly skipped, and redden a + # REQUIRED context on every docs-only PR. + assert guard.get("env") == { + "ETV_DOCS_ONLY": "${{ steps.detect.outputs.docs_only }}", + "ETV_REVALIDATE_SKIP": "${{ steps.revalidate.outputs.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." + "outputs, spelled exactly as the gated steps' own `if:` reads them. A typo here is silent: " + "the value arrives empty, the guard demands steps that were legitimately skipped, and every " + "docs-only run of a required context goes red." ) @@ -343,7 +392,7 @@ def _run(script: str, env): capture_output=True, text=True) -@pytest.mark.parametrize("job", REQUIRED_JOBS) +@pytest.mark.parametrize("job", MARKED_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. @@ -362,7 +411,7 @@ def test_the_guard_PASSES_when_every_step_marked_itself(job, tmp_path): assert "All" in r.stdout and "executed" in r.stdout, r.stdout -@pytest.mark.parametrize("job", REQUIRED_JOBS) +@pytest.mark.parametrize("job", MARKED_JOBS) def test_dropping_ANY_single_step_FAILS_the_guard(job, tmp_path): """Every marked step, one at a time — not a sample. @@ -388,8 +437,9 @@ def test_dropping_ANY_single_step_FAILS_the_guard(job, tmp_path): ) +@pytest.mark.parametrize("job", MARKED_JOBS) @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): +def test_a_fired_skip_gate_does_not_require_the_gated_steps(gate, job, 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 @@ -397,7 +447,6 @@ def test_a_fired_skip_gate_does_not_require_the_gated_steps(gate, tmp_path): 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"})) @@ -408,15 +457,16 @@ def test_a_fired_skip_gate_does_not_require_the_gated_steps(gate, tmp_path): assert "Skip gate fired" in r.stdout, r.stdout -def test_a_fired_skip_gate_STILL_requires_the_ALWAYS_steps(tmp_path): +@pytest.mark.parametrize("job", MARKED_JOBS) +def test_a_fired_skip_gate_STILL_requires_the_ALWAYS_steps(job, 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")) + guard = _guard(job)["run"] + r = _run("\n".join(["set -e", guard]), _env(tmp_path, GITHUB_JOB=job, 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." @@ -424,7 +474,8 @@ def test_a_fired_skip_gate_STILL_requires_the_ALWAYS_steps(tmp_path): assert "detect" in (r.stdout + r.stderr), r.stdout -def test_an_EMPTY_gate_value_requires_the_gated_steps(tmp_path): +@pytest.mark.parametrize("job", MARKED_JOBS) +def test_an_EMPTY_gate_value_requires_the_gated_steps(job, 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 @@ -432,10 +483,11 @@ def test_an_EMPTY_gate_value_requires_the_gated_steps(tmp_path): 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="")) + guard = _guard(job)["run"] + r = _run("\n".join(["set -e", guard]), + _env(tmp_path, GITHUB_JOB=job, ETV_DOCS_ONLY="", ETV_REVALIDATE_SKIP="")) assert r.returncode != 0 - assert "dotnet-test" in (r.stdout + r.stderr), ( + assert _guard_buckets(job)[1][-1] in (r.stdout + r.stderr), ( f"empty gate values were read as a skip, so the gated steps went unchecked: {r.stdout}" ) @@ -510,3 +562,66 @@ def test_a_key_is_matched_WHOLE_not_as_a_substring(tmp_path): "the key 'build' was satisfied by a marker for 'web-build' — a dropped `dotnet build` would " "pass unnoticed" ) + + +def test_a_degraded_run_IDENTITY_refuses_rather_than_sharing_a_marker_path(tmp_path): + """`GITHUB_RUN_ID` absent must REFUSE, not fall back to a name every run shares. + + The first version of `marker_path` defaulted to `nojob`/`norunid`/`1`. Those are reusable, so a + leftover marker from any earlier run on the host would satisfy the guard on a run whose step was + dropped — a silent PASS, which is the precise failure the run-keying exists to remove, + reintroduced by the code implementing it. Found by cold review. + + Asserted on BOTH sub-commands: a refusal that only `assert` honoured would let `mark` write to a + shared path and leave the two disagreeing about where the file is. + """ + env = _env(tmp_path) + for var in ("GITHUB_RUN_ID", "GITHUB_JOB"): + degraded = {k: v for k, v in env.items() if k != var} + for argv in (f"{SCRIPT} mark alpha", f"{SCRIPT} assert --always alpha"): + r = _run(argv, degraded) + assert r.returncode != 0, ( + f"with {var} unset, `{argv.split()[-2]}` continued and used a fallback path that " + f"other runs also use — a stale marker there passes the guard on a dropped run.\n" + f"{r.stdout}{r.stderr}" + ) + assert "cannot identify this run" in (r.stdout + r.stderr), ( + f"refused, but without naming the cause: {r.stdout!r} {r.stderr!r}") + assert not list(tmp_path.iterdir()), ( + "a degraded-identity `mark` still created a marker file somewhere under RUNNER_TEMP") + + +def test_a_missing_RUN_ATTEMPT_warns_and_continues_rather_than_deadlocking(tmp_path): + """The deliberate asymmetry, and the reason it is not simply "require all three". + + `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`). `GITHUB_RUN_ATTEMPT` is not — it appears nowhere in + that job log. These jobs write REQUIRED contexts, so refusing on an unverified variable would + risk reddening every merge in the repo to close a residual that needs a second condition (a lane + that stopped giving each job a fresh container) to bite at all. It warns instead, which is loud + rather than silent, and the warning is the signal to promote it. + """ + env = {k: v for k, v in _env(tmp_path).items() if k != "GITHUB_RUN_ATTEMPT"} + r = _run(f"{SCRIPT} mark alpha && {SCRIPT} assert --always alpha", env) + assert r.returncode == 0, ( + "a missing GITHUB_RUN_ATTEMPT reddened the job. That variable is unmeasured on this runner, " + f"so this would deadlock every merge over an assumption.\n{r.stdout}{r.stderr}") + assert "::warning::" in (r.stdout + r.stderr) and "GITHUB_RUN_ATTEMPT" in (r.stdout + r.stderr), ( + f"it continued SILENTLY, which is the shape being avoided: {r.stdout!r} {r.stderr!r}") + + +def test_a_skip_gate_that_empties_the_expected_set_REFUSES(tmp_path): + """The anti-vacuity check has to run AFTER the gate, not only on argv. Cold review reproduced + this exactly: + + ETV_DOCS_ONLY=true … assert --always --gated foo + -> "All 0 expected step(s) executed", exit 0 + + The argv check cannot see it, because the set is emptied by the gate rather than by the caller. + Unreachable with today's argv, but it contradicted the comment directly above it — and "reports + that it proved everything while proving nothing" is the failure this whole file exists to remove. + """ + r = _run(f"{SCRIPT} assert --always --gated foo", _env(tmp_path, ETV_DOCS_ONLY="true")) + assert r.returncode != 0, ( + f"the guard passed with an empty post-gate expectation set: {r.stdout!r}") + assert "no expected keys" in (r.stdout + r.stderr).lower() or "NO expected keys" in r.stderr -- 2.47.3 From daead0885b2cfe67eee521df45baf46c884f4028 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 22:37:42 +0200 Subject: [PATCH 4/8] docs(756): measure the if:-less guard's one plausible counterexample MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard carries no if:, resting on 'guard skipped => job red'. The path where that could be silently false is a continue-on-error step that FAILS: if that flipped success(), the guard would be skipped on a still-green job — a no-op produced by the very failure mode it exists to catch. Both advisory steps sit immediately before it, so the path is live. Probe PR #766 (run 1913 job 8075): the advisory step exits 1, the log carries its ❌, and the guard STILL ran, reported 'All 12 expected step(s) executed', and the job concluded success. The invariant holds. That run doubles as the test job's twelve-key positive control on the build lane, which #765 could not give (its test job was the negative case). Decisions-Edit: yes Refs #756 --- docs/ci-cd.md | 11 +++++++++++ .../records/ci/required-job-step-execution-markers.md | 7 ++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 80e25f3e9..d5d28f3bb 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -652,6 +652,17 @@ failed, and that already fails the job**. So *guard skipped ⇒ job red*, and ev runs the guard. A dropped step is invisible precisely *because* it concludes `success` — which keeps the job green and therefore reaches the guard. +That invariant has one path where it could plausibly be false and where being wrong would be silent: +a step marked `continue-on-error: true` that FAILS. If that flipped `success()`, the guard would be +skipped on a job that still concluded green — the guard rendered a no-op by exactly the failure mode +it exists to catch, with no signal. Both advisory steps sit immediately before the guard, so this is +a live path, not a theoretical one. **Measured** (scratch PR #766, run 1913 job 8075): the last +advisory step was made to `exit 1`, the log carries `❌ Failure - Main Report peak container memory`, +and the guard **still ran**, reported `All 12 expected step(s) executed`, and the job concluded +`success`. A failing `continue-on-error` step does not flip `success()` on this runner, so the +invariant holds where it mattered most. That run is also the `test` job's full twelve-key positive +control on the build lane. + **Adding a step to either job?** Mark it, and add its key to that job's guard list in the right bucket (`--always` for the two detect steps, `--gated` for anything carrying the docs-only / already-validated `if:`). `scripts/tests/test_ci_dropped_step_guard.py` derives the expected set from diff --git a/docs/decisions/records/ci/required-job-step-execution-markers.md b/docs/decisions/records/ci/required-job-step-execution-markers.md index 22100c61e..dad37f72c 100644 --- a/docs/decisions/records/ci/required-job-step-execution-markers.md +++ b/docs/decisions/records/ci/required-job-step-execution-markers.md @@ -41,7 +41,12 @@ payload inside a shell comment — in the `test` job's `revalidate` step. The st eleven later marked steps still ran (through `dotnet-test`), and the guard was the ONLY failing step in the job — so without it that run would have concluded `success` having skipped a step. The SAME run supplies the positive control on the same lane: its untouched `migrations` job marked all six -steps, reported `All 6 expected step(s) executed`, and concluded `success`. Full log extract in +steps, reported `All 6 expected step(s) executed`, and concluded `success`. + +A second probe (PR #766, run 1913) settled the one path on which the `if:`-less guard could have been +a silent no-op: a FAILING `continue-on-error` step. Had that flipped `success()`, the guard would be +skipped on a still-green job. It does not — the advisory step failed, the guard ran anyway, reported +`All 12 expected step(s) executed`, and the job stayed `success`. Full log extracts in docs/ci-cd.md. **The two halves are deliberately different in kind, and neither is redundant.** The delimiter ban is -- 2.47.3 From 93115b1f27e0ec5a9c123c94da0cc3e71b9bab50 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 22:59:01 +0200 Subject: [PATCH 5/8] =?UTF-8?q?fix(756):=20round-3=20=E2=80=94=20the=20har?= =?UTF-8?q?ness=20could=20not=20see=20its=20own=20central=20predicate=20br?= =?UTF-8?q?eak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The important one: no behavioural test ever drove the guard at the gate value the runner actually sends. _env() left ETV_DOCS_ONLY/ETV_REVALIDATE_SKIP UNSET, and unset happens to behave correctly. Cold review mutated the gate from `= "true"` to `-n` and all 30 tests stayed GREEN, while the guard run with the real environment reported 'Skip gate fired (docs_only=false) … All 2 expected step(s) executed' and exited 0 — Build, Test and both migration replays unguarded on every ordinary run, with the guard announcing it had proved everything. Read off the cited probe's own log: ETV_DOCS_ONLY: false. _env() now defaults both to `false` and the positive control is parametrised over false/empty/unset; the mutation is killed by the drop test on both jobs. Also: the sibling record ci.workflow-run-body-no-expressions still asserted that `build` interpolates legitimately and that 'a drop there fails no gate' — the exact claim the new record was written to refute, and it is the `rule:` field the catalog surfaces. Corrected, along with its occurrence count. The measurement was off by one: ten marked steps follow the drop, eleven markers survived in total (detect being the earlier eleventh). Three copies of the retracted RUNNER_TEMP rationale and two of the retracted 'scoped to the required pair' survived their own retraction — swept by subject. The script also cited the probe as evidence for a claim (container-private /tmp) the probe never took; that follows from `container:`, not from a measurement. Relaxed the guard-env assertion from whole-dict equality to per-key, so an added variable or ${{x}} spacing no longer false-reds a required check. Honest residual, now stated in both docs and tracked as ersatztv#767: the `build` ban is enforced only by script-tests, which is on: pull_request and NOT a required context — so it is a review-time check, not a fail-closed gate on the tag/release path. Not closed here because Smoke's if: carries a third condition the bucket protocol does not model, and measuring a guard there would mean cutting a real release tag. Decisions-Edit: yes Refs #756 --- docs/ci-cd.md | 18 ++++- docs/decisions/README.md | 2 +- .../ci/required-job-step-execution-markers.md | 20 +++-- .../ci/workflow-run-body-no-expressions.md | 15 ++-- scripts/ci-step-ran.sh | 8 +- scripts/tests/test_ci_dropped_step_guard.py | 76 ++++++++++++++----- 6 files changed, 101 insertions(+), 38 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index d5d28f3bb..9b4f14616 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -627,7 +627,18 @@ Two independent mechanisms hold it, and neither is redundant: in the registry as the release candidate and that step is what decides whether the candidate was ever booted. A drop there publishes an unsmoked candidate, reports green, and `DeployStack jazz-media` promotes exactly that image. Its two payloads moved into the step's `env:`, so the ban - cost nothing. `functional-e2e` is delimiter-free too but is deliberately **not** banned: it is + cost nothing. + + **Be precise about what that buys, because it is less than it looks.** The ban is enforced by the + `script-tests` job, which lives in `pr-checks.yml` (`on: pull_request`) and is **not** a required + context. So it is a *review-time* check on the PR that would introduce a delimiter — not a + fail-closed gate on the release path. `pr-checks.yml` does not run on a `v*` tag push at all, and + `build` has no markers and no guard, so a delimiter that ever reached `main` would still drop + `Smoke` on the tag build and go green. `main` being PR-only (#743) means such a change must pass + through a PR where `script-tests` reddens, but a red on a non-required check does not block the + merge server-side. Strictly better than before, not closed. Tracked as **ersatztv#767**, which also + records why it was not done here: `Smoke`'s `if:` carries a third condition the guard protocol does + not model, and measuring a guard on that path would mean cutting a real release tag. `functional-e2e` is delimiter-free too but is deliberately **not** banned: it is advisory by declaration, and the rule is "ban where a drop is consequential", not "ban wherever it is currently free". `api-docs` and `format` keep one `github.base_ref` each in a detect step and gate nothing that ships. @@ -691,8 +702,9 @@ Recorded outcome (job `test`, run 1910, 20:03:49→20:13:31Z — a full 9m42s he - `Unable to interpolate expression 'format('# PROBE ONLY … {0}\n…', pr number)'` at 20:04:06 — the step was **dropped**, exactly as #751 describes, and it reported conclusion `success`. -- **Every one of the eleven later marked steps still ran**, through `build` and `dotnet-test`. The - premise holds on this lane. +- **Every other marked step still ran** — eleven markers were recorded, ten of them AFTER the drop + (`restore npm-ci check-api lint typecheck web-test web-build strip-scanner build dotnet-test`), + `detect` being the eleventh and earlier. The premise holds on this lane. - The guard ran at 20:13:29, reported `These steps of job 'test' never executed: revalidate`, and was the **only** ❌ in the entire job log — every other step succeeded. Without it this run would have concluded `success` having never executed that step, which is precisely the fail-open being closed. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 97dbc63a3..a9a3def24 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -66,7 +66,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. Abstaining is a handoff, not a stall, and that is the property the design rests on: every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops and the last run writes the final answer. `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | | `ci.web-test-per-test-timeouts` | Give heavy-render web tests an explicit per-test vitest timeout (e.g. 15s); never raise the global default to fix one slow test. | 2026-07-21 | [link](records/ci/web-test-per-test-timeouts.md) | -| `ci.workflow-run-body-no-expressions` | A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step's `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard's own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`'s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file's delimiter ban to those two jobs. It is still not a repo-wide property: `build`, `api-docs` and `format` interpolate into `run:` bodies legitimately and are not required contexts, so a drop there fails no gate. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | +| `ci.workflow-run-body-no-expressions` | A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step's `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard's own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`'s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file's delimiter ban to those two jobs. It is still not a repo-wide property, but the remaining exceptions are narrower than this record originally said: `build` was brought into the ban too (its `Smoke + IPTV E2E` runs AFTER the image is pushed, so a drop there ships an unsmoked release candidate — its two payloads moved to `env:`, so the ban was free), leaving only `api-docs` and `format`, whose one `github.base_ref` each sits in a detect step that gates nothing that ships. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | | `concurrency.diff-scalar-fanout` | The frozen Block optimistic-concurrency recipe (api-conventions §7a) fans out to Collection/Playout×2/MultiCollection/RerunCollection, keeping a guard-returned `PreconditionFailedError` out of any handler's generic `catch(Exception)`→422 mapping, and preserving each aggregate's existing `SaveChangesAsync() > 0` gate semantics under the new unconditional `Version++`. | 2026-07-11 | [link](records/concurrency/diff-scalar-fanout.md) | | `concurrency.etag-rotation-completion` | Every handler that mutates a versioned root's editor-visible config state must bump `Version` (rotating the ETag) with no per-aggregate carve-outs, short-circuiting on a genuine no-op before the bump so idempotent re-submits don't fire spurious rebuild fan-out; `SaveChangesForcingVersion` rebases the retry (stored + pending delta), never adopts the stored token verbatim. | 2026-07-12 | [link](records/concurrency/etag-rotation-completion.md) | | `concurrency.force-write-non-ifmatch` | Any handler that leaves a versioned root `Modified` or `Deleted` but takes no `If-Match` (deletes, item add/remove bumpers, scalar-config writers) must save through `ConcurrencyExtensions.SaveChangesForcingVersion` — force-write past a concurrent `Version` bump rather than throw an unhandled `DbUpdateConcurrencyException` (500). | 2026-07-12 | [link](records/concurrency/force-write-non-ifmatch.md) | diff --git a/docs/decisions/records/ci/required-job-step-execution-markers.md b/docs/decisions/records/ci/required-job-step-execution-markers.md index dad37f72c..94cc7e6c8 100644 --- a/docs/decisions/records/ci/required-job-step-execution-markers.md +++ b/docs/decisions/records/ci/required-job-step-execution-markers.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step''s `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships.' signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py` · issues: #756, #751, #684' -mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge; the path is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID`/`GITHUB_RUN_ATTEMPT` because `RUNNER_TEMP` is `/tmp` here and a stale marker would make the guard PASS on a dropped run. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' +mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`, which is unmeasured on this runner and therefore WARNS and defaults to 1 rather than reddening a required check over an assumption. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' --- **Why per step, when #756 proposed per job.** A job-start marker answers "did this job begin", which @@ -37,8 +37,9 @@ still executing a LATER step after dropping an earlier one. #751 established tha lane; these two jobs run in a `container:` on `ubuntu-latest`, which is a different lane, so assuming it transfers would be the same shape of mistake the guard exists to catch. Measured by scratch PR #765 (Gitea 1.27.1, 2026-08-10), which reintroduced the exact #751 defect — an invalid expression -payload inside a shell comment — in the `test` job's `revalidate` step. The step was dropped, all -eleven later marked steps still ran (through `dotnet-test`), and the guard was the ONLY failing step +payload inside a shell comment — in the `test` job's `revalidate` step. The step was dropped, the +other eleven markers were still recorded — ten of them AFTER the drop, `detect` being the earlier +eleventh — and the guard was the ONLY failing step in the job — so without it that run would have concluded `success` having skipped a step. The SAME run supplies the positive control on the same lane: its untouched `migrations` job marked all six steps, reported `All 6 expected step(s) executed`, and concluded `success`. @@ -52,8 +53,9 @@ docs/ci-cd.md. **The two halves are deliberately different in kind, and neither is redundant.** The delimiter ban is static and absolute, and it makes the defect class UNREACHABLE in these jobs rather than merely detected — it is the cheaper and more general half, and it is enforceable today only because both -jobs are already delimiter-free (measured 2026-08-10: `test` 0, `migrations` 0; `build` 2, `api-docs` -1, `format` 1, which is why the rule stops at the required pair). The runtime markers catch a step +jobs were already delimiter-free (measured 2026-08-10: `test` 0, `migrations` 0), and `build` was +brought in by moving its two payloads to `env:` — leaving `api-docs` and `format` with one +`github.base_ref` each, in detect steps that gate nothing that ships. The runtime markers catch a step that fails to run for any OTHER reason, including reasons not yet met. Keeping only the static half would be trusting that this is the only way a step can vanish, which is exactly the assumption #751 falsified about shell comments. @@ -72,3 +74,11 @@ ban. The claim not to repeat is the draft's dichotomy ("give up interpolation or `scripts/`"); the `env:` escape hatch this record prescribes was the answer all along. What genuinely remains uncovered is `api-docs` and `format`, whose one `github.base_ref` each sits in a detect step that gates nothing that ships, and `functional-e2e`, which is advisory by declaration. + +**And the `build` ban is review-time, not fail-closed — do not read it as more.** It is enforced by +`script-tests`, which is `on: pull_request` and is NOT a required context, so nothing re-checks it +when a release is actually cut and `build` carries no markers or guard. A delimiter that reached +`main` would still drop `Smoke` on the tag build and report green. Closing that needs a third +bucket modelling `Smoke`'s publish-ref condition, plus a way to measure it that does not involve +pushing a real `v*` tag — deliberately deferred to ersatztv#767 rather than shipped unmeasured onto +the release path, since every other claim in this record is backed by a live probe. diff --git a/docs/decisions/records/ci/workflow-run-body-no-expressions.md b/docs/decisions/records/ci/workflow-run-body-no-expressions.md index 975d462b7..37e2baf8b 100644 --- a/docs/decisions/records/ci/workflow-run-body-no-expressions.md +++ b/docs/decisions/records/ci/workflow-run-body-no-expressions.md @@ -5,7 +5,7 @@ status: active since: '2026-08-06' supersedes: none superseded-by: none -rule: 'A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step''s `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard''s own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`''s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file''s delimiter ban to those two jobs. It is still not a repo-wide property: `build`, `api-docs` and `format` interpolate into `run:` bodies legitimately and are not required contexts, so a drop there fails no gate.' +rule: 'A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step''s `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard''s own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`''s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file''s delimiter ban to those two jobs. It is still not a repo-wide property, but the remaining exceptions are narrower than this record originally said: `build` was brought into the ban too (its `Smoke + IPTV E2E` runs AFTER the image is pushed, so a drop there ships an unsmoked release candidate — its two payloads moved to `env:`, so the ban was free), leaving only `api-docs` and `format`, whose one `github.base_ref` each sits in a detect step that gates nothing that ships.' signals: 'Unable to interpolate expression format(, step never ran but job green, missing Run Main step marker, review-verdict/h10 absent after a green run, docs-only PR unmergeable, Renovate PR unmergeable, exemption stopped working, expression in a shell comment, workflow comment changed behaviour · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #751, #706, #748' mechanics: '`RAN_MARKER` written at the top of the classify step and asserted by the `Assert the classifier actually executed` step (`if: always()`, expression-free body, `exit 1` on a missing marker); static guards `test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body` (raw scalar, absolute, gate file only) and `test_every_workflow_expression_names_a_REAL_context_or_function` (repo-wide, allow-list of contexts/functions) plus `test_a_dropped_classify_step_FAILS_the_job_instead_of_going_green` (pins marker path agreement and guard ordering across ONE yaml parse)' --- @@ -34,8 +34,9 @@ next day, so by the time this was found the workaround that had been absorbing t the next docs-only or Renovate-manifest PR would have been permanently stuck. The two Renovate PRs in the window escaped by timing alone, merging minutes before the bad commit landed. -**Scope of the strict rule, and why it is not repo-wide.** `docker-build.yml`, `ci-image.yml` and -`pr-checks.yml` interpolate into `run:` bodies legitimately (7 occurrences). A repo-wide ban would be +**Scope of the strict rule, and why it is not repo-wide.** As of 2026-08-06, `docker-build.yml`, +`ci-image.yml` and `pr-checks.yml` interpolated into `run:` bodies legitimately (7 occurrences; #756 +later removed `build`'s two — see below). A repo-wide ban would be false and would be deleted the first time it got in someone's way. `review-verdict.yml` earns the absolute rule on two counts: it writes the branch-protection-required status, and its `run:` bodies are ~700 lines of dense prose — the only place the delimiter has ever appeared by accident. @@ -43,8 +44,12 @@ are ~700 lines of dense prose — the only place the delimiter has ever appeared The first of those two counts turned out to apply elsewhere as well, and #756 acted on it: the absolute ban now also covers `docker-build.yml`'s `test` and `migrations` jobs, which write the other two required contexts and were delimiter-free already, so the rule cost nothing to impose there. The -remaining 4 occurrences sit in jobs that gate nothing. Read this paragraph as scoping the rule to -REQUIRED contexts rather than to this one file — that is what it was always tracking. +remaining 2 occurrences inside `docker-build.yml` — `api-docs` and `format`, one `github.base_ref` +each — sit in jobs that gate nothing that ships (5 repo-wide, counting `ci-image.yml` and the two +`pr-checks.yml` gates). `build` is banned too, and NOT because it is required (it is not): its +`Smoke + IPTV E2E` step runs after the image is pushed, so a drop there publishes a release candidate +that was never booted. Read this paragraph as scoping the rule to steps whose non-execution is +CONSEQUENTIAL — required contexts and the release path — rather than to this one file. **The probe found a SECOND, independent reason the gate posted nothing**, and it is why fixing the interpolation alone would not have restored the exemptions: a page past the end of diff --git a/scripts/ci-step-ran.sh b/scripts/ci-step-ran.sh index 08f8e9fc2..eb85954bf 100755 --- a/scripts/ci-step-ran.sh +++ b/scripts/ci-step-ran.sh @@ -46,9 +46,11 @@ # #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. +# toolchain image, so their `/tmp` is the job container's own and starts empty. That follows from +# `container:`, NOT from a measurement: the build-lane probe confirmed only that `RUNNER_TEMP` is +# `/tmp` here (the marker landed at `/tmp/etv-ci-steps-ran-test-1910-1`) — it says nothing about the +# directory being private or empty, and an earlier draft of this comment cited it as though it did. +# The fresh container is what actually rules out a stale marker here; 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 diff --git a/scripts/tests/test_ci_dropped_step_guard.py b/scripts/tests/test_ci_dropped_step_guard.py index 47b7e2c9b..b40e7af5b 100644 --- a/scripts/tests/test_ci_dropped_step_guard.py +++ b/scripts/tests/test_ci_dropped_step_guard.py @@ -137,11 +137,14 @@ def _is_gated(step) -> bool: def test_the_delimiter_banned_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). + This is the cheaper and more general half of #756: the drop mechanism REQUIRES an opener 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. + + The scope is the three jobs in `DELIMITER_BAN_JOBS` — see the comment there for why `build` is in + and `functional-e2e` is not. Do NOT restate this docstring as "scoped to the required pair": + round 2 moved `build`'s two payloads into `env:` and brought it into the ban, and this docstring + sits directly above the decorator that parametrises over the wider set. 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. @@ -346,15 +349,21 @@ def test_the_guards_OWN_body_cannot_be_dropped_by_the_mechanism_it_guards_agains # context so the repo-wide expression check passes it too. The guard would then read an EMPTY # value on a docs-only run, demand the gated steps that were correctly skipped, and redden a # REQUIRED context on every docs-only PR. - assert guard.get("env") == { - "ETV_DOCS_ONLY": "${{ steps.detect.outputs.docs_only }}", - "ETV_REVALIDATE_SKIP": "${{ steps.revalidate.outputs.skip }}", - }, ( - f"the '{job}' guard's env: is {guard.get('env')} — it must pass exactly the two skip-gate " - "outputs, spelled exactly as the gated steps' own `if:` reads them. A typo here is silent: " - "the value arrives empty, the guard demands steps that were legitimately skipped, and every " - "docs-only run of a required context goes red." - ) + # THE TWO MAPPINGS MUST BE PRESENT AND CORRECT — but this deliberately does NOT demand that the + # `env:` block contain ONLY them. An earlier version compared the whole dict, which false-redded + # on adding an unrelated variable (an `LC_ALL`, say) and on the equally-valid `${{x}}` spacing; + # a red here blocks every merge through the combined status, so brittleness is a real cost and + # not a free strictness win. Whitespace inside the delimiters is normalised for the same reason. + env = {k: re.sub(r"\s+", "", str(v)) for k, v in (guard.get("env") or {}).items()} + for name, want in (("ETV_DOCS_ONLY", "${{steps.detect.outputs.docs_only}}"), + ("ETV_REVALIDATE_SKIP", "${{steps.revalidate.outputs.skip}}")): + assert env.get(name) == want, ( + f"the '{job}' guard's env: has {name}={guard.get('env', {}).get(name)!r}, expected the " + f"output the gated steps' own `if:` reads ({want}). A typo here is SILENT rather than " + "loud: it still names a real context, so the repo-wide expression check passes it, the " + "value arrives empty, and the guard then demands steps that were legitimately skipped — " + "reddening a REQUIRED context on every docs-only run." + ) # ------------------------------------------------------------------------------------------------ @@ -374,6 +383,22 @@ def _mark_line(step) -> str: return line.strip() +# THE GATE VALUES DEFAULT TO `"false"`, WHICH IS WHAT THE RUNNER ACTUALLY SENDS — and getting this +# wrong made the whole suite blind. Found by cold review, which demonstrated it: every behavioural +# test used to leave these UNSET, so the guard was never once driven at its production values. Change +# the gate in `ci-step-ran.sh` from `= "true"` to `-n` — a one-token regression — and all 30 tests +# stayed GREEN while the guard, run with the real environment, reported +# `Skip gate fired (docs_only='false') … All 2 expected step(s) executed` and exited 0. `Build`, +# `Test` and both migration replays would have been unguarded on every ordinary run, with the guard +# announcing that it had proved everything. +# +# Read off the live log of the probe this change cites (run 1910, job 8064): `ETV_DOCS_ONLY: false`, +# `ETV_REVALIDATE_SKIP:` (empty). So `"false"` and `""` are both real, and `unset` is what a dropped +# detect step produces. A test double is an assertion about what the real system sends, and this one +# was wrong about the only field the guard branches on. +GATE_VALUES_IN_THE_WILD = ("false", "", None) + + def _env(tmp_path, **extra): env = { "PATH": os.environ["PATH"], @@ -382,9 +407,11 @@ def _env(tmp_path, **extra): "GITHUB_JOB": "test", "GITHUB_RUN_ID": "424242", "GITHUB_RUN_ATTEMPT": "7", + "ETV_DOCS_ONLY": "false", + "ETV_REVALIDATE_SKIP": "false", } env.update(extra) - return env + return {k: v for k, v in env.items() if v is not None} def _run(script: str, env): @@ -392,8 +419,10 @@ def _run(script: str, env): capture_output=True, text=True) +@pytest.mark.parametrize("gate", GATE_VALUES_IN_THE_WILD, + ids=["gate-false", "gate-empty", "gate-unset"]) @pytest.mark.parametrize("job", MARKED_JOBS) -def test_the_guard_PASSES_when_every_step_marked_itself(job, tmp_path): +def test_the_guard_PASSES_when_every_step_marked_itself(job, gate, 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 @@ -402,7 +431,10 @@ def test_the_guard_PASSES_when_every_step_marked_itself(job, tmp_path): """ marks = [_mark_line(s) for s, _ in _marked(job)] guard = _guard(job)["run"] - env = _env(tmp_path, GITHUB_JOB=job) + # Parametrised over every NOT-SKIPPING spelling the runner emits — `false` on an ordinary run, + # empty when the producing step was dropped, absent if the output is never set. All three must + # require the gated steps; a gate that treats any of them as a skip is fail-open on that path. + env = _env(tmp_path, GITHUB_JOB=job, ETV_DOCS_ONLY=gate, ETV_REVALIDATE_SKIP=gate) 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 " @@ -493,11 +525,13 @@ def test_an_EMPTY_gate_value_requires_the_gated_steps(job, tmp_path): 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). + """A marker from another run, attempt or job must never answer for this one. - 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. + Do NOT restate this as "RUNNER_TEMP is /tmp, not a private per-job directory". That is a #751 + measurement taken on a job with no `container:`, and it does not transfer: these two jobs run + inside the CI toolchain image, so their `/tmp` is the container's own. The fresh container is + what actually rules out staleness here; the keying is defence in depth against a lane change + nobody would think to re-check this against, and that is why it is still worth testing. """ marks = [_mark_line(s) for s, _ in _marked("test")] guard = _guard("test")["run"] -- 2.47.3 From 52c76d97fbad067d5ce282bfafe8af27c9e0661a Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 23:29:09 +0200 Subject: [PATCH 6/8] =?UTF-8?q?fix(756):=20round-4=20=E2=80=94=20a=20VOIDE?= =?UTF-8?q?D=20trailer=20block,=20and=20three=20claims=20contradicted=20by?= =?UTF-8?q?=20evidence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blocker: every one of the five commits carried `Decisions-Edit: yes`, and git parsed NONE of them. `Refs #756` has no colon, so it is not a trailer, and one bad line voids the whole block — `git log --format='%(trailers:key=...)'` returned empty for all five and `decisions_validate.py --base origin/main` failed. It would have surfaced as a red `decisions lifecycle`, which is the one red this repo is standing-instructed to treat as a known flake. This commit's trailer block is clean; `Refs:` now carries its colon. Committed the gate value MATRIX (16 cases). Cold review showed round 3's `gate-unset` parametrisation was vacuous in the same way round 3 itself had just fixed: the positive control still passes when the gate wrongly skips, because the guard prints "All 2 expected step(s) executed" and exits 0. Only a test asserting skip-iff-exactly-true discriminates, and `(true, true)` — a docs-only merge to main on an already-validated tree — had no coverage at all. Three claims were wrong rather than merely stale: * The workflow comment said `build` gets no markers because "it is not a required context, so they would buy little" — the exact dismissal the record names as the claim not to repeat. Markers there would buy MORE than the ban, which only runs on pull_request. * The #767 deferral claimed measuring a guard on the release path "would mean cutting a real release tag". False: `build` runs on every push to main and Smoke's ref condition admits refs/heads/main. * "Both advisory steps" — there are three. On GITHUB_RUN_ATTEMPT: cold review argued it IS set, from the absence of the script's warning in the probe logs. That needs step stderr to reach the log, and the control offered was an `::error::` this script writes to STDOUT — so the premise was unestablished in both directions, and I did not adopt it. Instead `assert` now prints its resolved identity on stdout every run, where capture is not in question; read that line on this PR's run and promote the variable if it says "from the runner". Adding that reporting introduced, and the suite immediately caught, a real bug: marker_path's STDOUT *is* the path, so the notice printed there landed inside it. Decisions-Edit: yes Refs: #756 --- .gitea/workflows/docker-build.yml | 10 ++- docs/ci-cd.md | 41 +++++++--- .../ci/required-job-step-execution-markers.md | 10 ++- .../ci/workflow-run-body-no-expressions.md | 4 +- scripts/ci-step-ran.sh | 22 +++++- scripts/tests/test_ci_dropped_step_guard.py | 78 ++++++++++++++++--- scripts/tests/test_pr_changed_files.py | 8 +- 7 files changed, 138 insertions(+), 35 deletions(-) diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index 5c13ba352..c36ce15c0 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -740,8 +740,14 @@ jobs: # body delimiter-free the class is unreachable here — held by # test_the_delimiter_banned_jobs_have_NO_expression_delimiter_in_any_run_body. # - # This job gets the ban but NOT the per-step markers: it is not a required context, so the - # marker machinery would buy little beyond what removing the delimiters already buys. + # This job gets the ban but NOT (yet) the per-step markers — and NOT because "it is not a + # required context, so the markers would buy little", which is the dismissal + # `ci.required-job-step-execution-markers` explicitly names as the claim not to repeat. Markers + # here would buy strictly MORE than the ban does: the ban is enforced only by `script-tests`, + # which is `on: pull_request` and not a required check, so nothing re-checks it when a release + # is actually cut, whereas markers would be a runtime, fail-closed gate on the tag path itself. + # It is deferred because this step's `if:` carries a publish-ref condition the guard's + # always/gated buckets do not model. Tracked as ersatztv#767. - name: Smoke + IPTV E2E (assert key endpoints) if: ${{ (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v')) && steps.detect.outputs.docs_only != 'true' }} env: diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 9b4f14616..6108f77db 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -636,9 +636,16 @@ Two independent mechanisms hold it, and neither is redundant: `build` has no markers and no guard, so a delimiter that ever reached `main` would still drop `Smoke` on the tag build and go green. `main` being PR-only (#743) means such a change must pass through a PR where `script-tests` reddens, but a red on a non-required check does not block the - merge server-side. Strictly better than before, not closed. Tracked as **ersatztv#767**, which also - records why it was not done here: `Smoke`'s `if:` carries a third condition the guard protocol does - not model, and measuring a guard on that path would mean cutting a real release tag. `functional-e2e` is delimiter-free too but is deliberately **not** banned: it is + merge server-side. Strictly better than before, not closed. Tracked as **ersatztv#767**. + + The reason it was deferred is that `Smoke`'s `if:` carries a publish-ref condition the guard's + always/gated buckets do not model — that is the whole of it. An earlier draft here also claimed + measuring such a guard "would mean cutting a real release tag", and that was simply **wrong**: + `build` runs on every push to `main` (`if: github.event_name != 'pull_request'`) and `Smoke`'s ref + condition admits `refs/heads/main`, so the runs-side is exercised by any ordinary merge — including + the one that lands this change — and the legitimately-skipped side by a `workflow_dispatch` on any + other ref. Left recorded because a false cost estimate is exactly what talks the next person out of + a tracked follow-up. `functional-e2e` is delimiter-free too but is deliberately **not** banned: it is advisory by declaration, and the rule is "ban where a drop is consequential", not "ban wherever it is currently free". `api-docs` and `format` keep one `github.base_ref` each in a detect step and gate nothing that ships. @@ -666,8 +673,8 @@ the job green and therefore reaches the guard. That invariant has one path where it could plausibly be false and where being wrong would be silent: a step marked `continue-on-error: true` that FAILS. If that flipped `success()`, the guard would be skipped on a job that still concluded green — the guard rendered a no-op by exactly the failure mode -it exists to catch, with no signal. Both advisory steps sit immediately before the guard, so this is -a live path, not a theoretical one. **Measured** (scratch PR #766, run 1913 job 8075): the last +it exists to catch, with no signal. The `test` job has three `continue-on-error` steps and two of them sit immediately before the guard, +so this is a live path, not a theoretical one. **Measured** (scratch PR #766, run 1913 job 8075): the last advisory step was made to `exit 1`, the log carries `❌ Failure - Main Report peak container memory`, and the guard **still ran**, reported `All 12 expected step(s) executed`, and the job concluded `success`. A failing `continue-on-error` step does not flip `success()` on this runner, so the @@ -678,7 +685,11 @@ control on the build lane. bucket (`--always` for the two detect steps, `--gated` for anything carrying the docs-only / already-validated `if:`). `scripts/tests/test_ci_dropped_step_guard.py` derives the expected set from the workflow, so an unmarked step or a bucket mismatch is a red — it does not rely on anyone -remembering. +remembering. One caveat, since this section is careful about it elsewhere: that red is `script-tests`, +the same non-required, PR-only check discussed above. For the delimiter ban on `test`/`migrations` +that hardly matters, because the runtime guard is the fail-closed backstop — but a **newly added, +unmarked** step is caught by the static test *alone*, since the runtime guard cannot expect a key +nobody declared. **The marker path is keyed on job + run id + attempt** — and be precise about why, because the obvious justification is a #751 measurement that does *not* transfer. #751 found `RUNNER_TEMP` to be @@ -688,10 +699,15 @@ which runs *without* a `container:`. These two jobs run **inside** the CI toolch rules out a stale marker here; the keying is defence in depth against a lane change nobody would think to re-check this against. `GITHUB_JOB` and `GITHUB_RUN_ID` are *measured* present and the script refuses without them rather than falling back to a name other runs share. -`GITHUB_RUN_ATTEMPT` is **not** measured — it appears nowhere in the probe's 2.9 MB job log — so it -warns and defaults instead: these are required checks, and reddening every merge over an unverified -variable is a worse trade than a residual that needs an unset variable *and* a non-fresh container to -bite. +`GITHUB_RUN_ATTEMPT` is **not** established either way, so it warns and defaults instead: these are +required checks, and reddening every merge over an unverified variable is a worse trade than a +residual needing an unset variable *and* a non-fresh container to bite. Cold review argued it *is* +set, reasoning that the script's "not set" warning appears nowhere in the probe logs — but that needs +step **stderr** to reach the job log, and the control offered for it (an `::error::` line in the log) +turned out to be one this script writes to *stdout*. Rather than settle it by argument, the guard now +prints `Marker identity: job=… run=… attempt=… (from the runner | DEFAULTED …)` on stdout every run. +Read that line on this change's own CI run; if it says "from the runner", promote the variable to the +refuse bucket alongside the other two. **The premise was re-measured on the build lane.** The whole thing rests on the runner still executing a later step after dropping an earlier one. #751 established that on the `small` lane; these jobs run @@ -1157,7 +1173,10 @@ Three things now hold the line, and they are deliberately different in kind: body of this file (absolute — a dropped step here is a dead merge gate, and its bodies are ~700 lines of prose), and repo-wide, every expression payload's **head token** must name a context or function the runner can resolve (permissive, because the other workflows interpolate into `run:` - legitimately — 7 occurrences today). Be precise about the second one's reach: it catches the + legitimately — 5 occurrences today, in `ci-image.yml`, `docker-build.yml`'s `api-docs`/`format` + and `pr-checks.yml`'s two git-diff gates; #756 removed `build`'s two and banned that job as + well, so the ban now covers `test`, `migrations` and `build`). Be precise about the second + one's reach: it catches the historical defect (`pr number`) and a nonexistent context, but **not** a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}` passes), nor a renamed output (`steps.metadata.outputs.shortsha` passes — every token after the first is preceded by `.` and is diff --git a/docs/decisions/records/ci/required-job-step-execution-markers.md b/docs/decisions/records/ci/required-job-step-execution-markers.md index 94cc7e6c8..355917d58 100644 --- a/docs/decisions/records/ci/required-job-step-execution-markers.md +++ b/docs/decisions/records/ci/required-job-step-execution-markers.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step''s `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships.' signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py` · issues: #756, #751, #684' -mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`, which is unmeasured on this runner and therefore WARNS and defaults to 1 rather than reddening a required check over an assumption. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' +mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`, which is NOT established either way on this runner and therefore WARNS and defaults to 1 rather than reddening a required check over an assumption; `assert` prints `Marker identity: … attempt=… (from the runner | DEFAULTED …)` on stdout every run so the next reader settles it by reading rather than inferring. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' --- **Why per step, when #756 proposed per job.** A job-start marker answers "did this job begin", which @@ -79,6 +79,8 @@ that gates nothing that ships, and `functional-e2e`, which is advisory by declar `script-tests`, which is `on: pull_request` and is NOT a required context, so nothing re-checks it when a release is actually cut and `build` carries no markers or guard. A delimiter that reached `main` would still drop `Smoke` on the tag build and report green. Closing that needs a third -bucket modelling `Smoke`'s publish-ref condition, plus a way to measure it that does not involve -pushing a real `v*` tag — deliberately deferred to ersatztv#767 rather than shipped unmeasured onto -the release path, since every other claim in this record is backed by a live probe. +bucket modelling `Smoke`'s publish-ref condition — deferred to ersatztv#767 rather than shipped +unmeasured onto the release path, since every other claim in this record is backed by a live probe. +Do NOT repeat the cost estimate an earlier draft gave for it ("would require pushing a real `v*` +tag"): `build` runs on every push to `main` and `Smoke`'s ref condition admits `refs/heads/main`, so +both directions are measurable without cutting a release. diff --git a/docs/decisions/records/ci/workflow-run-body-no-expressions.md b/docs/decisions/records/ci/workflow-run-body-no-expressions.md index 37e2baf8b..0b1c9a7f2 100644 --- a/docs/decisions/records/ci/workflow-run-body-no-expressions.md +++ b/docs/decisions/records/ci/workflow-run-body-no-expressions.md @@ -35,8 +35,8 @@ the next docs-only or Renovate-manifest PR would have been permanently stuck. Th the window escaped by timing alone, merging minutes before the bad commit landed. **Scope of the strict rule, and why it is not repo-wide.** As of 2026-08-06, `docker-build.yml`, -`ci-image.yml` and `pr-checks.yml` interpolated into `run:` bodies legitimately (7 occurrences; #756 -later removed `build`'s two — see below). A repo-wide ban would be +`ci-image.yml` and `pr-checks.yml` interpolated into `run:` bodies legitimately (7 occurrences then; +#756 removed `build`'s two, leaving 5 today — see below). A repo-wide ban would be false and would be deleted the first time it got in someone's way. `review-verdict.yml` earns the absolute rule on two counts: it writes the branch-protection-required status, and its `run:` bodies are ~700 lines of dense prose — the only place the delimiter has ever appeared by accident. diff --git a/scripts/ci-step-ran.sh b/scripts/ci-step-ran.sh index eb85954bf..ab6cc5cc8 100755 --- a/scripts/ci-step-ran.sh +++ b/scripts/ci-step-ran.sh @@ -102,6 +102,14 @@ marker_path() { exit 3 fi if [ -z "$attempt" ]; then + # STDERR ONLY, AND THAT IS NOT STYLE. This function's STDOUT *is* its return value — it is + # always called inside `$( )` — so anything printed here is captured INTO THE PATH. An + # earlier version of this block echoed the notice to stdout and produced a marker path with + # the notice embedded in it; both sub-commands then failed on a nonexistent directory. + # Caught by test_a_missing_RUN_ATTEMPT_*, which is why that test asserts a clean exit rather + # than only the presence of the warning. + # + # The stdout half of this reporting lives in `cmd_assert`, which is free to print. 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 @@ -161,11 +169,16 @@ cmd_assert() { skipped=yes fi - local marker + local marker attempt_used # `|| 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 $? + # Read the attempt back OFF THE RESOLVED PATH rather than re-deriving it. `marker_path` runs in a + # subshell, so anything it sets cannot reach here; re-deriving would be a second copy of the + # defaulting rule, free to drift from the one that actually built the path. + attempt_used="${marker##*-}" + if [ -z "${GITHUB_RUN_ATTEMPT:-}" ]; then ETV_ATTEMPT_DEFAULTED=yes; fi # `${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 @@ -210,6 +223,13 @@ cmd_assert() { fi exit 1 fi + # The resolved identity, on stdout, every run. This is what turns "is GITHUB_RUN_ATTEMPT + # exported here?" from an inference into something a reader just looks up — and it is why the + # variable is still WARN-and-default rather than REFUSE: `GITHUB_JOB` and `GITHUB_RUN_ID` have + # positive evidence (the probe's marker filename), this one does not yet, and refusing on an + # unestablished variable would redden a REQUIRED check. Promote it once a run has printed + # `attempt= (from the runner)`. + echo "Marker identity: job=${GITHUB_JOB} run=${GITHUB_RUN_ID} attempt=${attempt_used} ($([ "${ETV_ATTEMPT_DEFAULTED:-no}" = yes ] && echo "DEFAULTED — GITHUB_RUN_ATTEMPT unset" || echo "from the runner"))" echo "All ${#expected[@]} expected step(s) executed: ${expected[*]}" } diff --git a/scripts/tests/test_ci_dropped_step_guard.py b/scripts/tests/test_ci_dropped_step_guard.py index b40e7af5b..3f3695749 100644 --- a/scripts/tests/test_ci_dropped_step_guard.py +++ b/scripts/tests/test_ci_dropped_step_guard.py @@ -161,10 +161,12 @@ def test_the_delimiter_banned_jobs_have_NO_expression_delimiter_in_any_run_body( offenders.append(f"{s.get('name', '?')}: {payload!r}") 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:` " + f"{offenders}. A dropped step in this job is CONSEQUENTIAL — `test`/`migrations` write " + "REQUIRED status contexts, and `build` publishes the release candidate before its smoke step " + "runs. 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 the check passes 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." ) @@ -392,9 +394,19 @@ def _mark_line(step) -> str: # `Test` and both migration replays would have been unguarded on every ordinary run, with the guard # announcing that it had proved everything. # -# Read off the live log of the probe this change cites (run 1910, job 8064): `ETV_DOCS_ONLY: false`, -# `ETV_REVALIDATE_SKIP:` (empty). So `"false"` and `""` are both real, and `unset` is what a dropped -# detect step produces. A test double is an assertion about what the real system sends, and this one +# THE COMPLETE VALUE SET, and where each comes from — worth spelling out, because the obvious reading +# of the evidence is wrong. Both producers document `true|false` and write exactly that +# (`scripts/ci-detect-docs-only.sh` -> `docs_only=`, `scripts/ci-detect-already-validated.sh` -> +# `skip=`), so an ordinary run sends `false` and a skipping run sends `true`. +# +# The live log of the probe this change cites (run 1910, job 8064) shows `ETV_DOCS_ONLY: false` and +# `ETV_REVALIDATE_SKIP:` EMPTY — but do NOT read that as revalidate's normal output. `revalidate` was +# the step the probe deliberately dropped, so it wrote no output at all. The empty string is +# therefore not an odd third state: it is the SIGNATURE OF THE VERY FAILURE THIS GUARD EXISTS TO +# CATCH, which is exactly why the gate must treat anything that is not `true` as "widen what is +# required". `None` (unset) is the same case reached a different way. +# +# A test double is an assertion about what the real system sends, and the earlier version of this one # was wrong about the only field the guard branches on. GATE_VALUES_IN_THE_WILD = ("false", "", None) @@ -629,11 +641,16 @@ def test_a_missing_RUN_ATTEMPT_warns_and_continues_rather_than_deadlocking(tmp_p """The deliberate asymmetry, and the reason it is not simply "require all three". `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`). `GITHUB_RUN_ATTEMPT` is not — it appears nowhere in - that job log. These jobs write REQUIRED contexts, so refusing on an unverified variable would - risk reddening every merge in the repo to close a residual that needs a second condition (a lane - that stopped giving each job a fresh container) to bite at all. It warns instead, which is loud - rather than silent, and the warning is the signal to promote it. + wrote `/tmp/etv-ci-steps-ran-test-1910-1`). `GITHUB_RUN_ATTEMPT` is not established either way: + cold review argued its absence from the probe logs proves it IS set, but that inference needs + step stderr to reach the job log and the control offered for it was an `::error::` this script + writes to STDOUT. These jobs write REQUIRED contexts, so refusing on an unverified variable would + risk reddening every merge in the repo to close a residual needing a second condition (a lane + that stopped giving each job a fresh container) to bite at all. + + This test also pins the bug that fix introduced: `marker_path`'s STDOUT *is* the path, so a + notice printed there lands INSIDE it. Asserting a clean exit — not merely that the warning + appeared — is what catches that. """ env = {k: v for k, v in _env(tmp_path).items() if k != "GITHUB_RUN_ATTEMPT"} r = _run(f"{SCRIPT} mark alpha && {SCRIPT} assert --always alpha", env) @@ -659,3 +676,40 @@ def test_a_skip_gate_that_empties_the_expected_set_REFUSES(tmp_path): assert r.returncode != 0, ( f"the guard passed with an empty post-gate expectation set: {r.stdout!r}") assert "no expected keys" in (r.stdout + r.stderr).lower() or "NO expected keys" in r.stderr + + +@pytest.mark.parametrize("revalidate", ["true", "false", "", None], + ids=lambda v: f"reval-{v if v is not None else 'unset'}") +@pytest.mark.parametrize("docs_only", ["true", "false", "", None], + ids=lambda v: f"docs-{v if v is not None else 'unset'}") +def test_the_skip_gate_over_the_WHOLE_value_matrix(docs_only, revalidate, tmp_path): + """Every combination of the two gate values, not just the diagonal — cold review's last finding. + + Round 3 fixed the suite's blindness to the production value `false`, but still only exercised + matched pairs and single-`true` cases. `(true, true)` is REACHABLE — a docs-only PR merged to + `main` whose tree was already validated sets both — and an exclusive-or regression would pass + every other test here while demanding all the gated markers on a run that legitimately skipped + those steps. That reddens BOTH required contexts, which is the false-red direction: it deadlocks + every merge rather than letting one through. + + The property asserted is the whole contract in one line: with only the `--always` keys marked, + the guard passes exactly when the gate says the gated steps were skipped — `true` in EITHER + variable, and nothing else. Sixteen cases, so no combination is a special case anyone has to + remember. + + On `unset`: the workflow's `env:` block always defines both, emitting EMPTY for an output the + producing step never wrote, so unset is not reachable through the workflow. It is covered because + the script is also runnable by hand, and because "not exactly true" is the property that must + hold for every spelling rather than for an enumerated list. + """ + job = "test" + always, gated = _guard_buckets(job) + marks = [_mark_line(s) for s, k in _marked(job) if k in always] + r = _run("\n".join(["set -e", *marks, _guard(job)["run"]]), + _env(tmp_path, ETV_DOCS_ONLY=docs_only, ETV_REVALIDATE_SKIP=revalidate)) + should_skip = docs_only == "true" or revalidate == "true" + assert (r.returncode == 0) is should_skip, ( + f"with docs_only={docs_only!r} and revalidate={revalidate!r} the guard " + f"{'passed' if r.returncode == 0 else 'failed'}, expected it to " + f"{'skip the gated keys' if should_skip else 'require them'}. The gate must treat a value as " + "a skip if and only if it is exactly `true` in EITHER variable.\n" + r.stdout + r.stderr) diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index eed2be30d..787628492 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -2321,9 +2321,11 @@ def _workflow_files(): def test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body(): """The absolute rule, and it is scoped to this one file on purpose. - Other workflows legitimately interpolate into a `run:` body (7 occurrences across - docker-build/ci-image/pr-checks today), so a repo-wide ban would be false and would be deleted - the first time it got in someone's way. This file is different in two ways that justify the + Other workflows legitimately interpolate into a `run:` body (5 occurrences today across + ci-image, docker-build's `api-docs`/`format`, and pr-checks' two git-diff gates), so a + repo-wide ban would be false and would be deleted the first time it got in someone's way. + `test`, `migrations` and `build` carry their own absolute ban — see + scripts/tests/test_ci_dropped_step_guard.py (ersatztv#756). This file is different in two ways that justify the strict rule: it writes the branch-protection-required status, so a dropped step here is a dead merge gate rather than a failed build; and its `run:` bodies are ~700 lines of dense prose, which is the only place the delimiter has ever appeared by accident. -- 2.47.3 From bd152bc46f205b9c68d807e2228a617de5251fb1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 11 Aug 2026 00:18:03 +0200 Subject: [PATCH 7/8] =?UTF-8?q?fix(756):=20round-5=20=E2=80=94=20kill=20th?= =?UTF-8?q?ree=20surviving=20mutations=20on=20the=20identity=20line?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The final review verdict was MERGEABLE, but demonstrated three mutations of round 4's reporting line surviving a 50-green suite: deleting the echo, mis-deriving the attempt (`${marker#*-}` for `##`), and inverting the provenance test. That line is a documented contract — the record's mechanics: asserts it — and the decision to promote GITHUB_RUN_ATTEMPT to the refuse bucket is meant to be made by READING it off a run log. An unasserted reporting line that the operator is told to trust is the same shape as the rest of this issue. Two assertions; all three mutations now die. Also: ETV_ATTEMPT_DEFAULTED was a non-local global with no else branch, so the same name inherited from the environment made the line self-contradictory (`attempt=5 (DEFAULTED — GITHUB_RUN_ATTEMPT unset)`). Reachable, since ETV_* is a namespace the guard's own env: writes into. Computed inline now. And the workflow comment claimed ci.required-job-step-execution-markers "explicitly names" a dismissal it does not name — the record's do-not-repeat sentence is about a different clause. A wrong cross-reference in a commit whose subject is wrong claims about other files. Refs: #756 --- .gitea/workflows/docker-build.yml | 5 +++-- scripts/ci-step-ran.sh | 10 ++++++++-- scripts/tests/test_ci_dropped_step_guard.py | 16 ++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index c36ce15c0..04508546f 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -741,8 +741,9 @@ jobs: # test_the_delimiter_banned_jobs_have_NO_expression_delimiter_in_any_run_body. # # This job gets the ban but NOT (yet) the per-step markers — and NOT because "it is not a - # required context, so the markers would buy little", which is the dismissal - # `ci.required-job-step-execution-markers` explicitly names as the claim not to repeat. Markers + # required context, so the markers would buy little". That is the same family as the + # "smaller cost (no required context lies)" dismissal that + # `ci.required-job-step-execution-markers` records as FALSE about this very step. Markers # here would buy strictly MORE than the ban does: the ban is enforced only by `script-tests`, # which is `on: pull_request` and not a required check, so nothing re-checks it when a release # is actually cut, whereas markers would be a runtime, fail-closed gate on the tag path itself. diff --git a/scripts/ci-step-ran.sh b/scripts/ci-step-ran.sh index ab6cc5cc8..208211516 100755 --- a/scripts/ci-step-ran.sh +++ b/scripts/ci-step-ran.sh @@ -178,7 +178,11 @@ cmd_assert() { # subshell, so anything it sets cannot reach here; re-deriving would be a second copy of the # defaulting rule, free to drift from the one that actually built the path. attempt_used="${marker##*-}" - if [ -z "${GITHUB_RUN_ATTEMPT:-}" ]; then ETV_ATTEMPT_DEFAULTED=yes; fi + # The provenance is computed INLINE below rather than held in a variable. An earlier version set + # an `ETV_ATTEMPT_DEFAULTED` global with no `else` branch, so the same name inherited from the + # environment made the line report `attempt=5 (DEFAULTED — GITHUB_RUN_ATTEMPT unset)` — self + # contradictory. `ETV_*` is a namespace this workflow already writes into via the guard's `env:`, + # so that was reachable, not hypothetical. # `${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 @@ -229,7 +233,9 @@ cmd_assert() { # positive evidence (the probe's marker filename), this one does not yet, and refusing on an # unestablished variable would redden a REQUIRED check. Promote it once a run has printed # `attempt= (from the runner)`. - echo "Marker identity: job=${GITHUB_JOB} run=${GITHUB_RUN_ID} attempt=${attempt_used} ($([ "${ETV_ATTEMPT_DEFAULTED:-no}" = yes ] && echo "DEFAULTED — GITHUB_RUN_ATTEMPT unset" || echo "from the runner"))" + local provenance="from the runner" + if [ -z "${GITHUB_RUN_ATTEMPT:-}" ]; then provenance="DEFAULTED — GITHUB_RUN_ATTEMPT unset"; fi + echo "Marker identity: job=${GITHUB_JOB} run=${GITHUB_RUN_ID} attempt=${attempt_used} (${provenance})" echo "All ${#expected[@]} expected step(s) executed: ${expected[*]}" } diff --git a/scripts/tests/test_ci_dropped_step_guard.py b/scripts/tests/test_ci_dropped_step_guard.py index 3f3695749..9b0d01775 100644 --- a/scripts/tests/test_ci_dropped_step_guard.py +++ b/scripts/tests/test_ci_dropped_step_guard.py @@ -453,6 +453,12 @@ def test_the_guard_PASSES_when_every_step_marked_itself(job, gate, tmp_path): 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 + # The other half of the identity contract: with GITHUB_RUN_ATTEMPT set (`_env` sends 7) the line + # must report the REAL value and say so. A mis-derivation (`${marker#*-}` rather than `##`) or an + # inverted provenance test would otherwise ship silently, and the operator reading this line to + # settle the promotion question would read it wrong. + assert "attempt=7 (from the runner)" in r.stdout, ( + f"the guard misreported its marker identity: {r.stdout!r}") @pytest.mark.parametrize("job", MARKED_JOBS) @@ -659,6 +665,16 @@ def test_a_missing_RUN_ATTEMPT_warns_and_continues_rather_than_deadlocking(tmp_p f"so this would deadlock every merge over an assumption.\n{r.stdout}{r.stderr}") assert "::warning::" in (r.stdout + r.stderr) and "GITHUB_RUN_ATTEMPT" in (r.stdout + r.stderr), ( f"it continued SILENTLY, which is the shape being avoided: {r.stdout!r} {r.stderr!r}") + # THE IDENTITY LINE ON STDOUT, which is the whole point of the warn-and-default compromise: the + # open question ("does this runner export GITHUB_RUN_ATTEMPT?") is answered by READING a run + # rather than by inference, and the decision to promote the variable to the refuse bucket hangs + # on it. Asserted because cold review showed three mutations of this reporting — deleting the + # echo, mis-deriving the attempt, inverting the provenance — all survived a 50-green suite. The + # `::warning::` goes to stderr, whose capture in a job log is exactly what is NOT established; + # stdout is not in question, so the stdout half is the load-bearing one. + assert "attempt=1 (DEFAULTED" in r.stdout, ( + "the guard did not report a DEFAULTED attempt on stdout, so a reader cannot tell from a run " + f"log whether the marker path was keyed on a real attempt: {r.stdout!r}") def test_a_skip_gate_that_empties_the_expected_set_REFUSES(tmp_path): -- 2.47.3 From 0d0518d3cbdfea337e6607db77ed49cf45eb1222 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 11 Aug 2026 00:59:12 +0200 Subject: [PATCH 8/8] =?UTF-8?q?fix(756):=20promote=20GITHUB=5FRUN=5FATTEMP?= =?UTF-8?q?T=20to=20required=20=E2=80=94=20its=20presence=20is=20now=20MEA?= =?UTF-8?q?SURED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #768's own run answered the question the identity line was added to answer: Marker identity: job=test run=1916 attempt=1 (from the runner) Marker identity: job=migrations run=1916 attempt=1 (from the runner) Both required jobs, on the build lane, printed to stdout where capture is not in question. So the variable IS exported here, the warn-and-default compromise has served its purpose, and all three identity components now REFUSE rather than falling back to a name other runs share. The residual that compromise left — a rerun inheriting attempt 1's markers — no longer exists. Recording HOW this was settled, because the first two attempts were worthless and the next reader will be tempted by both. Grepping a job log for the variable NAME proves nothing: logs do not dump the environment. Inferring it from the ABSENCE of the script's "not set" warning proves nothing either — that warning goes to stderr, and whether step stderr reaches a job log here was itself never established (the control offered for it turned out to be an ::error:: this script writes to STDOUT). Only making the script report its own resolved identity on stdout, and then reading it, settled anything. The docs said "not established either way", which this run falsifies — so the prose had to change regardless; promoting was the same edit surface. The identity line is kept, now as standing evidence rather than an open question, and is still asserted so it cannot rot into a line nobody can trust. Decisions-Edit: yes Refs: #756 --- docs/ci-cd.md | 23 ++++--- .../ci/required-job-step-execution-markers.md | 2 +- scripts/ci-step-ran.sh | 60 +++++++++---------- scripts/tests/test_ci_dropped_step_guard.py | 54 +++++++---------- 4 files changed, 67 insertions(+), 72 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 6108f77db..3bc241a9f 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -699,15 +699,20 @@ which runs *without* a `container:`. These two jobs run **inside** the CI toolch rules out a stale marker here; the keying is defence in depth against a lane change nobody would think to re-check this against. `GITHUB_JOB` and `GITHUB_RUN_ID` are *measured* present and the script refuses without them rather than falling back to a name other runs share. -`GITHUB_RUN_ATTEMPT` is **not** established either way, so it warns and defaults instead: these are -required checks, and reddening every merge over an unverified variable is a worse trade than a -residual needing an unset variable *and* a non-fresh container to bite. Cold review argued it *is* -set, reasoning that the script's "not set" warning appears nowhere in the probe logs — but that needs -step **stderr** to reach the job log, and the control offered for it (an `::error::` line in the log) -turned out to be one this script writes to *stdout*. Rather than settle it by argument, the guard now -prints `Marker identity: job=… run=… attempt=… (from the runner | DEFAULTED …)` on stdout every run. -Read that line on this change's own CI run; if it says "from the runner", promote the variable to the -refuse bucket alongside the other two. +`GITHUB_RUN_ATTEMPT` is required too — but **how** that was established is the part worth keeping, +because the first two attempts at it were both worthless. Grepping a job log for the variable *name* +proves nothing: logs do not dump the environment. Inferring it from the *absence* of the script's +"not set" warning proves nothing either, because that warning goes to **stderr**, and whether step +stderr reaches a job log here was itself never established — the control offered for that turned out +to be an `::error::` line this script writes to *stdout*. So the script was made to **report its +resolved identity on stdout**, where capture is not in question, and the answer was simply read off +this change's own run: `Marker identity: job=test run=1916 attempt=1 (from the runner)`, and the same +for `migrations`. Both required jobs, on the lane that matters. + +That measurement is what promoted it from warn-and-default to required, and it is why the residual +this paragraph used to describe — a rerun inheriting attempt 1's markers — no longer exists. The +identity line stays, as the standing evidence a future reader checks first if the keying is ever +doubted again. **The premise was re-measured on the build lane.** The whole thing rests on the runner still executing a later step after dropping an earlier one. #751 established that on the `small` lane; these jobs run diff --git a/docs/decisions/records/ci/required-job-step-execution-markers.md b/docs/decisions/records/ci/required-job-step-execution-markers.md index 355917d58..106267460 100644 --- a/docs/decisions/records/ci/required-job-step-execution-markers.md +++ b/docs/decisions/records/ci/required-job-step-execution-markers.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark ` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always --gated `, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step''s `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships.' signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py` · issues: #756, #751, #684' -mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`, which is NOT established either way on this runner and therefore WARNS and defaults to 1 rather than reddening a required check over an assumption; `assert` prints `Marker identity: … attempt=… (from the runner | DEFAULTED …)` on stdout every run so the next reader settles it by reading rather than inferring. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' +mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`. All three REFUSE rather than falling back to a reusable name. The third was warn-and-default until its presence was measured: grepping a log for the variable NAME proves nothing, and inferring it from the absence of a stderr warning proves nothing either (stderr capture was itself unestablished), so `assert` was made to print `Marker identity: job=… run=… attempt=… (from the runner)` on STDOUT and the answer was read off run 1916 for both required jobs. That line is retained as standing evidence. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn).' --- **Why per step, when #756 proposed per job.** A job-start marker answers "did this job begin", which diff --git a/scripts/ci-step-ran.sh b/scripts/ci-step-ran.sh index 208211516..c9d3c8a63 100755 --- a/scripts/ci-step-ran.sh +++ b/scripts/ci-step-ran.sh @@ -86,36 +86,37 @@ EOF # * `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. +# * `GITHUB_RUN_ATTEMPT` is measured present TOO, as of ersatztv#756's own PR run — but note how, +# because the first two attempts to settle it were both bad. Grepping a job log for the variable +# NAME proves nothing (logs do not dump the environment). Inferring it from the ABSENCE of this +# script's "not set" warning proves nothing either, because that warning goes to stderr and +# whether step stderr reaches a job log here was itself never established. So the script was made +# to REPORT its resolved identity on stdout, where capture is not in question, and the answer was +# then simply read off run 1916: `Marker identity: job=test run=1916 attempt=1 (from the runner)` +# and the same for `migrations`. Both required jobs, on the lane that matters. # -# 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. +# That measurement is what promoted it from warn-and-default to REQUIRED, which is why the residual +# this comment used to describe — a rerun inheriting attempt 1's markers — no longer exists. If a +# future runner stops exporting any of the three, every job reddens with a message naming the +# variable; that is loud, instantly diagnosable, and the correct direction for a required check. marker_path() { - local missing="" attempt="${GITHUB_RUN_ATTEMPT:-}" + local missing="" [ -n "${GITHUB_JOB:-}" ] || missing="$missing GITHUB_JOB" [ -n "${GITHUB_RUN_ID:-}" ] || missing="$missing GITHUB_RUN_ID" + [ -n "${GITHUB_RUN_ATTEMPT:-}" ] || missing="$missing GITHUB_RUN_ATTEMPT" if [ -n "$missing" ]; then + # NOTHING IS PRINTED TO STDOUT HERE, and that is load-bearing rather than style: this + # function's stdout IS its return value (it is always called inside `$( )`), so a notice + # printed here is captured INTO the path. An earlier revision did exactly that and both + # sub-commands then failed on a nonexistent directory. Caught by + # test_a_degraded_run_IDENTITY_*, which is why that test asserts on the exit status and on + # the absence of any marker file rather than only on the message. 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 - # STDERR ONLY, AND THAT IS NOT STYLE. This function's STDOUT *is* its return value — it is - # always called inside `$( )` — so anything printed here is captured INTO THE PATH. An - # earlier version of this block echoed the notice to stdout and produced a marker path with - # the notice embedded in it; both sub-commands then failed on a nonexistent directory. - # Caught by test_a_missing_RUN_ATTEMPT_*, which is why that test asserts a clean exit rather - # than only the presence of the warning. - # - # The stdout half of this reporting lives in `cmd_assert`, which is free to print. - 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" + "$GITHUB_JOB" "$GITHUB_RUN_ID" "$GITHUB_RUN_ATTEMPT" } cmd_mark() { @@ -174,15 +175,10 @@ cmd_assert() { # 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 $? - # Read the attempt back OFF THE RESOLVED PATH rather than re-deriving it. `marker_path` runs in a - # subshell, so anything it sets cannot reach here; re-deriving would be a second copy of the - # defaulting rule, free to drift from the one that actually built the path. + # Read the attempt back OFF THE RESOLVED PATH rather than from the environment. It reports what + # the path was actually keyed on, so a future change to how the path is built cannot silently + # disagree with the line that documents it. attempt_used="${marker##*-}" - # The provenance is computed INLINE below rather than held in a variable. An earlier version set - # an `ETV_ATTEMPT_DEFAULTED` global with no `else` branch, so the same name inherited from the - # environment made the line report `attempt=5 (DEFAULTED — GITHUB_RUN_ATTEMPT unset)` — self - # contradictory. `ETV_*` is a namespace this workflow already writes into via the guard's `env:`, - # so that was reachable, not hypothetical. # `${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 @@ -233,9 +229,11 @@ cmd_assert() { # positive evidence (the probe's marker filename), this one does not yet, and refusing on an # unestablished variable would redden a REQUIRED check. Promote it once a run has printed # `attempt= (from the runner)`. - local provenance="from the runner" - if [ -z "${GITHUB_RUN_ATTEMPT:-}" ]; then provenance="DEFAULTED — GITHUB_RUN_ATTEMPT unset"; fi - echo "Marker identity: job=${GITHUB_JOB} run=${GITHUB_RUN_ID} attempt=${attempt_used} (${provenance})" + # Kept after the promotion, though all three components are now required and the line can no + # longer report anything but the runner's own values. It is the standing evidence: this is the + # line that settled whether GITHUB_RUN_ATTEMPT is exported, and it is what a future reader checks + # first if the keying is ever doubted again. + echo "Marker identity: job=${GITHUB_JOB} run=${GITHUB_RUN_ID} attempt=${attempt_used} (from the runner)" echo "All ${#expected[@]} expected step(s) executed: ${expected[*]}" } diff --git a/scripts/tests/test_ci_dropped_step_guard.py b/scripts/tests/test_ci_dropped_step_guard.py index 9b0d01775..2f4a198f5 100644 --- a/scripts/tests/test_ci_dropped_step_guard.py +++ b/scripts/tests/test_ci_dropped_step_guard.py @@ -457,7 +457,7 @@ def test_the_guard_PASSES_when_every_step_marked_itself(job, gate, tmp_path): # must report the REAL value and say so. A mis-derivation (`${marker#*-}` rather than `##`) or an # inverted provenance test would otherwise ship silently, and the operator reading this line to # settle the promotion question would read it wrong. - assert "attempt=7 (from the runner)" in r.stdout, ( + assert f"Marker identity: job={job} run=424242 attempt=7 (from the runner)" in r.stdout, ( f"the guard misreported its marker identity: {r.stdout!r}") @@ -628,7 +628,7 @@ def test_a_degraded_run_IDENTITY_refuses_rather_than_sharing_a_marker_path(tmp_p shared path and leave the two disagreeing about where the file is. """ env = _env(tmp_path) - for var in ("GITHUB_RUN_ID", "GITHUB_JOB"): + for var in ("GITHUB_RUN_ID", "GITHUB_JOB", "GITHUB_RUN_ATTEMPT"): degraded = {k: v for k, v in env.items() if k != var} for argv in (f"{SCRIPT} mark alpha", f"{SCRIPT} assert --always alpha"): r = _run(argv, degraded) @@ -643,38 +643,30 @@ def test_a_degraded_run_IDENTITY_refuses_rather_than_sharing_a_marker_path(tmp_p "a degraded-identity `mark` still created a marker file somewhere under RUNNER_TEMP") -def test_a_missing_RUN_ATTEMPT_warns_and_continues_rather_than_deadlocking(tmp_path): - """The deliberate asymmetry, and the reason it is not simply "require all three". +def test_the_marker_identity_is_REPORTED_on_stdout_every_run(tmp_path): + """The line that settled `GITHUB_RUN_ATTEMPT`, kept as standing evidence. - `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`). `GITHUB_RUN_ATTEMPT` is not established either way: - cold review argued its absence from the probe logs proves it IS set, but that inference needs - step stderr to reach the job log and the control offered for it was an `::error::` this script - writes to STDOUT. These jobs write REQUIRED contexts, so refusing on an unverified variable would - risk reddening every merge in the repo to close a residual needing a second condition (a lane - that stopped giving each job a fresh container) to bite at all. + Worth recording HOW that was settled, because the first two attempts were both bad. Grepping a + job log for the variable NAME proves nothing (logs do not dump the environment). Inferring it + from the ABSENCE of a "not set" warning proves nothing either, because that warning goes to + stderr and whether step stderr reaches a job log here was itself never established — the control + offered for that was an `::error::` this script writes to STDOUT. So the script was made to + REPORT its resolved identity on stdout, where capture is not in question, and the answer was read + off ersatztv#756's own PR run: `Marker identity: job=test run=1916 attempt=1 (from the runner)`, + and the same for `migrations`. That is what promoted the variable from warn-and-default to + required. - This test also pins the bug that fix introduced: `marker_path`'s STDOUT *is* the path, so a - notice printed there lands INSIDE it. Asserting a clean exit — not merely that the warning - appeared — is what catches that. + Asserted because cold review demonstrated three mutations of this reporting — deleting the echo, + mis-deriving the attempt, inverting the provenance — all surviving a 50-green suite. It is a + documented contract (the record's `mechanics:`), and a future reader is told to trust it. """ - env = {k: v for k, v in _env(tmp_path).items() if k != "GITHUB_RUN_ATTEMPT"} - r = _run(f"{SCRIPT} mark alpha && {SCRIPT} assert --always alpha", env) - assert r.returncode == 0, ( - "a missing GITHUB_RUN_ATTEMPT reddened the job. That variable is unmeasured on this runner, " - f"so this would deadlock every merge over an assumption.\n{r.stdout}{r.stderr}") - assert "::warning::" in (r.stdout + r.stderr) and "GITHUB_RUN_ATTEMPT" in (r.stdout + r.stderr), ( - f"it continued SILENTLY, which is the shape being avoided: {r.stdout!r} {r.stderr!r}") - # THE IDENTITY LINE ON STDOUT, which is the whole point of the warn-and-default compromise: the - # open question ("does this runner export GITHUB_RUN_ATTEMPT?") is answered by READING a run - # rather than by inference, and the decision to promote the variable to the refuse bucket hangs - # on it. Asserted because cold review showed three mutations of this reporting — deleting the - # echo, mis-deriving the attempt, inverting the provenance — all survived a 50-green suite. The - # `::warning::` goes to stderr, whose capture in a job log is exactly what is NOT established; - # stdout is not in question, so the stdout half is the load-bearing one. - assert "attempt=1 (DEFAULTED" in r.stdout, ( - "the guard did not report a DEFAULTED attempt on stdout, so a reader cannot tell from a run " - f"log whether the marker path was keyed on a real attempt: {r.stdout!r}") + marks = [_mark_line(s) for s, _ in _marked("test")] + r = _run("\n".join(["set -e", *marks, _guard("test")["run"]]), + _env(tmp_path, GITHUB_RUN_ID="1916", GITHUB_RUN_ATTEMPT="4")) + assert r.returncode == 0, r.stdout + r.stderr + assert "Marker identity: job=test run=1916 attempt=4 (from the runner)" in r.stdout, ( + "the guard did not report the identity its marker path was actually keyed on, so a reader " + f"cannot audit the keying from a run log: {r.stdout!r}") def test_a_skip_gate_that_empties_the_expected_set_REFUSES(tmp_path): -- 2.47.3