From b4ac46fce188496c17fb25f1e00872264dad2c89 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 21 Jul 2026 18:07:56 +0200 Subject: [PATCH] ci(535): split PR-only git gates into pr-checks.yml so release tags don't red MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three PR-only git-diff gates (ci-image-pin, docs-reminder, decisions-guard) lived in docker-build.yml, which also triggers on push to main and v* tags. Gitea dispatches a job as a runner task even when its `if` skips it, so on every tag/main push these three were dispatched to the `small` lane just to evaluate the skip. On the v26.12.0 tag those dispatched skip-tasks wedged in act's setup phase and were killed by a runner restart mid-setup, reporting `failure` (no logs) and reddening the tag's overall commit status even though the release built, scanned, and deployed fine. The two identical-`if:` jobs on ubuntu-latest (api-docs, format) skipped cleanly — the job logic was never the problem; the kill lands in the dispatch window before any step or skip runs, so tweaking the `if:`/step logic could not fix it. Relocate exactly those three (pure checkout + git-diff, no container:, no image pin) verbatim into a dedicated pr-checks.yml that triggers `on: pull_request` only. Gitea evaluates a workflow's trigger before creating any job, so on a tag/main push this workflow produces zero jobs: no dispatch, no kill, no spurious red — for the whole class, permanently. - ci-image-pin carries no pin and still greps docker-build.yml, where all five pin-bearing jobs (test/migrations/functional-e2e/api-docs/format) remain, so its validation is unchanged. - None of the three are required checks (only Build & test + EF migration integrity are), so the status-context prefix change (Build ErsatzTV Image / ... -> PR Gates / ...) does not affect merges; the merge-consent hook reads the prefix-agnostic combined status. - pr-checks.yml declares `defaults: run: shell: bash` (ci-image-pin uses mapfile / set -o pipefail). docs/ci-cd.md: new "PR gates workflow" section + cross-references. fixes #535 Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/workflows/docker-build.yml | 117 +--------------------- .gitea/workflows/pr-checks.yml | 156 ++++++++++++++++++++++++++++++ docs/ci-cd.md | 45 ++++++++- 3 files changed, 202 insertions(+), 116 deletions(-) create mode 100644 .gitea/workflows/pr-checks.yml diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index 7a43edf2a..fe7845d3f 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -6,6 +6,10 @@ name: Build ErsatzTV Image # push tag v* -> :prod + : + : (prod release) # workflow_dispatch -> manual run; only publishes when the ref is main or a v* tag # +# The PR-only git-diff gates (ci-image-pin, docs-reminder, decisions-guard) live in the sibling +# .gitea/workflows/pr-checks.yml (`on: pull_request`). They were split out of this file so they +# are not dispatched-and-killed on a tag/main push (ersatztv#535 — see that file's header). +# # Runner + registry provisioned in server-management#172. The Gitea registry is # HTTP-only, so BuildKit needs the inline `http = true` config below (it does not # inherit the host daemon's insecure-registries setting). @@ -642,119 +646,6 @@ jobs: exit 1 fi - # BLOCKING (ersatztv#390): the CI toolchain image pin in this file must name the image that - # ci-image.yml actually last published — i.e. the short sha of the last commit to touch the image's - # sources. Without this detector, a PR that edits docker/ci/** publishes a NEW image but runs its own - # jobs against the OLD pin: CI green-lights a toolchain it never executed, and once merged, main's - # Dockerfile silently disagrees with what CI runs. **Renovate actively generates exactly that PR** — - # it manages docker/ci/Dockerfile's base pins (dockerfile manager) but cannot bump an opaque - # `:` in `container.image`, so it would leave the pin behind every time. - # - # Failing here forces the documented two-step (docs/ci-cd.md -> "CI toolchain image"): push the - # Dockerfile change, let ci-image.yml publish `:`, then update the pin to that sha. Seconds-long - # git+grep -> keep it off the build runners. - ci-image-pin: - name: CI image pin matches docker/ci - runs-on: small - if: github.event_name == 'pull_request' - steps: - - name: Checkout - uses: actions/checkout@v4 - with: - # need real history: `git log -- ` on a shallow clone can't find the last - # commit that touched the image sources - fetch-depth: 0 - - name: Verify the pin matches the last-published image - run: | - set -euo pipefail - # ci-image.yml tags the image `git rev-parse --short HEAD` of the push that built it, and it - # only builds on pushes touching these paths — so the published image is named by the last - # commit to touch them. - # - # Compare RESOLVED FULL shas, never the abbreviations: git auto-scales abbreviation length - # with the repo's object count, so the tag built in CI from a `fetch-depth: 1` shallow clone - # is 7 chars while `%h` here (full clone) is 8. Comparing those strings would fail always. - expected="$(git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml)" - mapfile -t pins < <(grep -oE 'ersatztv-ci:[0-9a-f]+' .gitea/workflows/docker-build.yml | cut -d: -f2 | sort -u) - echo "Image sources last changed in: ${expected}" - echo "Pins found in docker-build.yml: ${pins[*]} (${#pins[@]} distinct)" - if [ "${#pins[@]}" -ne 1 ]; then - echo "::error::docker-build.yml pins MORE THAN ONE ersatztv-ci tag (${pins[*]}). All jobs must pin the same image — bump them together." - exit 1 - fi - pin_full="$(git rev-parse --verify --quiet "${pins[0]}^{commit}" || true)" - if [ -z "$pin_full" ]; then - echo "::error::The pinned CI image tag ersatztv-ci:${pins[0]} does not resolve to a commit in this repo, so it cannot correspond to an image ci-image.yml built from these sources. Rebuild the image and pin the sha it prints." - exit 1 - fi - if [ "$pin_full" != "$expected" ]; then - echo "::error::CI toolchain image pin is stale: docker-build.yml pins ersatztv-ci:${pins[0]} ($pin_full), but docker/ci was last changed in $expected. Your jobs are testing an image that is NOT built from this PR's docker/ci. Let ci-image.yml publish the new :, then update the pin in ALL jobs to it (docs/ci-cd.md -> 'CI toolchain image')." - exit 1 - fi - echo "Pin is current: ersatztv-ci:${pins[0]} resolves to $pin_full = docker/ci's last change." - - # Non-blocking nudge: if a PR migrates/adds a route but forgets the parity tracker, warn. - # The rule lives in CLAUDE.md → Conventions; this only surfaces an easy-to-miss omission. - # Deliberately no setup-dotnet/setup-node (and thus no actions/cache) so it can't hit the - # cache-save issues seen on the relocated runner (server-management#570). - docs-reminder: - name: Docs update reminder - runs-on: small # seconds-long git diff; keep it off the build runners - if: github.event_name == 'pull_request' - steps: - - name: Checkout - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Warn when a screen/route change skips the parity doc - run: | - base_ref="${{ github.base_ref }}" - git fetch --no-tags --depth=100 origin "$base_ref" || true - changed="$(git diff --name-only "origin/${base_ref}...HEAD" 2>/dev/null || true)" - echo "Changed files in this PR:"; printf '%s\n' "$changed" - screen_or_route=no - if printf '%s\n' "$changed" | grep -Eq '^web/src/screens/.+\.tsx$|^ErsatzTV/LegacyUiRedirects\.cs$'; then - screen_or_route=yes - fi - parity=no - if printf '%s\n' "$changed" | grep -qx 'docs/blazor-route-parity.md'; then - parity=yes - fi - if [ "$screen_or_route" = yes ] && [ "$parity" = no ]; then - echo "::warning::This PR touches a SPA screen or LegacyUiRedirects.cs but does not update docs/blazor-route-parity.md. If you added/migrated/redirected a route, update the parity tracker (and docs/domain-model.md) in THIS PR — see CLAUDE.md → Conventions." - else - echo "Parity-doc reminder: nothing to flag." - fi - - # BLOCKING (ersatztv#521, supersedes the ersatztv#303 H9 append-only mechanic): validates decision- - # record lifecycle invariants (metadata schema, one active record per key, reciprocal - # supersedes/superseded-by links, no rationale-prose rewrite without [decisions-edit], no record - # vanishing from the active set without an archive copy) and that the generated active catalog - # (docs/decisions/README.md) is in sync. Same validator the Husky pre-commit hook shim calls, so - # local and CI enforcement can't drift. Seconds-long git diff + parse -> keep it off the build runners. - decisions-guard: - name: decisions lifecycle - runs-on: small - if: github.event_name == 'pull_request' - steps: - - name: Checkout - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.x' - - name: Validate decision lifecycle - run: | - base_ref="${{ github.base_ref }}" - git fetch --no-tags --depth=200 origin "$base_ref" || true - PYTHONPATH=. python3 scripts/decisions_validate.py --base "origin/${base_ref}" --head HEAD - - name: Active catalog in sync - run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check - - name: Kickoff guard - run: bash scripts/check-kickoff-guard.sh - # BLOCKING (unlike docs-reminder): the mechanizable half of the "docs-update in the # same PR" rule for the API contract (ersatztv#303 H4/H5). If a PR touches the API # surface (ErsatzTV/Controllers/Api/** or ErsatzTV.Core/Api/**), the generated diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml new file mode 100644 index 000000000..fe0574a9e --- /dev/null +++ b/.gitea/workflows/pr-checks.yml @@ -0,0 +1,156 @@ +name: PR Gates + +# Fast, git-only PR gates split out of docker-build.yml into a dedicated `on: pull_request` +# workflow (ersatztv#535) so they are NEVER created on a tag/main push. +# +# WHY THIS FILE EXISTS. These three checks are pure `checkout + git diff` gates: they carry no +# `container:`, run on the `small` lane (git-only, 1 GiB; server-management#639), and are PR-only. +# While they lived in docker-build.yml — which also triggers on push to main and on `v*` tags — +# Gitea still DISPATCHED them as runner tasks on every such push to evaluate the `if:` skip, because +# **Gitea dispatches a job as a runner task even when its `if` skips it** (docs/ci-cd.md -> the +# `small` lane). On the v26.12.0 release tag those dispatched skip-tasks wedged in act's setup phase +# and were killed by a runner restart mid-setup, so they reported `failure` (no logs) and reddened +# the tag's overall commit status even though the release built, scanned, and deployed fine +# (ersatztv#535). The two PR-only jobs on `ubuntu-latest` (`api-docs`, `format`) carry the identical +# `if:` and skipped cleanly on the same tag — the job logic was never the problem; the kill happens +# in the dispatch window before any step or `if:`-skip runs. +# +# Gitea evaluates a workflow's TRIGGER before creating any job, so a `pull_request`-only workflow +# produces ZERO jobs on a tag/main push: no dispatch, no kill, no spurious red. That is the whole +# fix. The per-job `if: github.event_name == 'pull_request'` guards are kept as belt-and-suspenders +# (they also encode "these steps need a PR base_ref"; harmless given the trigger). +# +# These stay on `runs-on: small` and carry NO CI toolchain image pin, so `ci-image-pin`'s grep of +# docker-build.yml still validates the five pin-bearing jobs (test/migrations/functional-e2e/ +# api-docs/format) that remain there. None of these three are required checks — branch protection +# requires only `Build & test (.NET)` and `EF migration integrity` — so relocating them (which +# changes their status-context prefix from "Build ErsatzTV Image / …" to "PR Gates / …") does not +# affect merges. See docs/ci-cd.md -> "PR gates workflow". + +on: + pull_request: + +# git-only host-runner jobs: no `container:`, so the runner default shell would be bash anyway, but +# declare it explicitly — ci-image-pin uses `mapfile`/`set -o pipefail`, which die under dash. +defaults: + run: + shell: bash + +# Per-ref: a new push to the PR supersedes its in-flight gate run. Only runs on PRs, so always cancel. +concurrency: + group: ersatztv-pr-gates-${{ github.ref }} + cancel-in-progress: true + +jobs: + # BLOCKING (ersatztv#390): the CI toolchain image pin in docker-build.yml must name the image that + # ci-image.yml actually last published — i.e. the short sha of the last commit to touch the image's + # sources. Without this detector, a PR that edits docker/ci/** publishes a NEW image but runs its own + # jobs against the OLD pin: CI green-lights a toolchain it never executed, and once merged, main's + # Dockerfile silently disagrees with what CI runs. **Renovate actively generates exactly that PR** — + # it manages docker/ci/Dockerfile's base pins (dockerfile manager) but cannot bump an opaque + # `:` in `container.image`, so it would leave the pin behind every time. + # + # Failing here forces the documented two-step (docs/ci-cd.md -> "CI toolchain image"): push the + # Dockerfile change, let ci-image.yml publish `:`, then update the pin to that sha. Seconds-long + # git+grep -> keep it off the build runners. + ci-image-pin: + name: CI image pin matches docker/ci + runs-on: small + if: github.event_name == 'pull_request' + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + # need real history: `git log -- ` on a shallow clone can't find the last + # commit that touched the image sources + fetch-depth: 0 + - name: Verify the pin matches the last-published image + run: | + set -euo pipefail + # ci-image.yml tags the image `git rev-parse --short HEAD` of the push that built it, and it + # only builds on pushes touching these paths — so the published image is named by the last + # commit to touch them. + # + # Compare RESOLVED FULL shas, never the abbreviations: git auto-scales abbreviation length + # with the repo's object count, so the tag built in CI from a `fetch-depth: 1` shallow clone + # is 7 chars while `%h` here (full clone) is 8. Comparing those strings would fail always. + expected="$(git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml)" + mapfile -t pins < <(grep -oE 'ersatztv-ci:[0-9a-f]+' .gitea/workflows/docker-build.yml | cut -d: -f2 | sort -u) + echo "Image sources last changed in: ${expected}" + echo "Pins found in docker-build.yml: ${pins[*]} (${#pins[@]} distinct)" + if [ "${#pins[@]}" -ne 1 ]; then + echo "::error::docker-build.yml pins MORE THAN ONE ersatztv-ci tag (${pins[*]}). All jobs must pin the same image — bump them together." + exit 1 + fi + pin_full="$(git rev-parse --verify --quiet "${pins[0]}^{commit}" || true)" + if [ -z "$pin_full" ]; then + echo "::error::The pinned CI image tag ersatztv-ci:${pins[0]} does not resolve to a commit in this repo, so it cannot correspond to an image ci-image.yml built from these sources. Rebuild the image and pin the sha it prints." + exit 1 + fi + if [ "$pin_full" != "$expected" ]; then + echo "::error::CI toolchain image pin is stale: docker-build.yml pins ersatztv-ci:${pins[0]} ($pin_full), but docker/ci was last changed in $expected. Your jobs are testing an image that is NOT built from this PR's docker/ci. Let ci-image.yml publish the new :, then update the pin in ALL jobs to it (docs/ci-cd.md -> 'CI toolchain image')." + exit 1 + fi + echo "Pin is current: ersatztv-ci:${pins[0]} resolves to $pin_full = docker/ci's last change." + + # Non-blocking nudge: if a PR migrates/adds a route but forgets the parity tracker, warn. + # The rule lives in CLAUDE.md → Conventions; this only surfaces an easy-to-miss omission. + # Deliberately no setup-dotnet/setup-node (and thus no actions/cache) so it can't hit the + # cache-save issues seen on the relocated runner (server-management#570). + docs-reminder: + name: Docs update reminder + runs-on: small # seconds-long git diff; keep it off the build runners + if: github.event_name == 'pull_request' + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Warn when a screen/route change skips the parity doc + run: | + base_ref="${{ github.base_ref }}" + git fetch --no-tags --depth=100 origin "$base_ref" || true + changed="$(git diff --name-only "origin/${base_ref}...HEAD" 2>/dev/null || true)" + echo "Changed files in this PR:"; printf '%s\n' "$changed" + screen_or_route=no + if printf '%s\n' "$changed" | grep -Eq '^web/src/screens/.+\.tsx$|^ErsatzTV/LegacyUiRedirects\.cs$'; then + screen_or_route=yes + fi + parity=no + if printf '%s\n' "$changed" | grep -qx 'docs/blazor-route-parity.md'; then + parity=yes + fi + if [ "$screen_or_route" = yes ] && [ "$parity" = no ]; then + echo "::warning::This PR touches a SPA screen or LegacyUiRedirects.cs but does not update docs/blazor-route-parity.md. If you added/migrated/redirected a route, update the parity tracker (and docs/domain-model.md) in THIS PR — see CLAUDE.md → Conventions." + else + echo "Parity-doc reminder: nothing to flag." + fi + + # BLOCKING (ersatztv#521, supersedes the ersatztv#303 H9 append-only mechanic): validates decision- + # record lifecycle invariants (metadata schema, one active record per key, reciprocal + # supersedes/superseded-by links, no rationale-prose rewrite without [decisions-edit], no record + # vanishing from the active set without an archive copy) and that the generated active catalog + # (docs/decisions/README.md) is in sync. Same validator the Husky pre-commit hook shim calls, so + # local and CI enforcement can't drift. Seconds-long git diff + parse -> keep it off the build runners. + decisions-guard: + name: decisions lifecycle + runs-on: small + if: github.event_name == 'pull_request' + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.x' + - name: Validate decision lifecycle + run: | + base_ref="${{ github.base_ref }}" + git fetch --no-tags --depth=200 origin "$base_ref" || true + PYTHONPATH=. python3 scripts/decisions_validate.py --base "origin/${base_ref}" --head HEAD + - name: Active catalog in sync + run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check + - name: Kickoff guard + run: bash scripts/check-kickoff-guard.sh diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 800067488..6270d99f5 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -143,6 +143,10 @@ stalling every PR run. `--cpu-shares` below the default 1024 is what makes a run prod media host acceptable: under contention CI loses to the transcoders (ersatztv 1536 / jellyfin), which are the reason those hosts exist. +That same "dispatched even when `if` skips it" behavior is why the git-only PR gates live in +their own **[PR gates workflow](#pr-gates-workflow)** (`pr-checks.yml`, `on: pull_request`) +rather than in `docker-build.yml` — see that section (ersatztv#535). + **`small` is git-only, and that is load-bearing (server-management#639).** Everything in the lane is a checkout plus a `git diff`: `decisions-guard`, `ci-image-pin`, `docs-reminder`. Nothing there runs a compiler or a `docker build`, which is why the lane @@ -535,7 +539,7 @@ 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. -### `docs-reminder` job (non-blocking, PR-only) +### `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 one case that's easy to forget and easy to detect: a PR that touches a SPA screen @@ -548,7 +552,7 @@ thus no `actions/cache`), so it can't hit the cache-save hangs seen on the VM-12 (domain-model, spa-conventions) — those stay on the author. (The API contract is mechanized by the blocking `api-docs` job, and `docs/decisions.md` by the blocking `decisions-guard` job below.) -### `decisions-guard` job (`decisions lifecycle`, blocking, PR-only) +### `decisions-guard` job (`decisions lifecycle`, blocking, PR-only — in `pr-checks.yml`) Enforces decision-record lifecycle invariants (ersatztv#521, supersedes the ersatztv#303 H9 append-only mechanic): well-formed 5-field metadata, exactly one `active` record per `key`, @@ -565,6 +569,40 @@ compiler/docker build), so it doesn't violate the "small is git-only" lane rule. `docs-reminder`, otherwise a seconds-long `git diff` + parse with no dotnet/node setup (`runs-on: small`). +## PR gates workflow + +**File:** `.gitea/workflows/pr-checks.yml` — `on: pull_request` only. + +The three git-only PR gates — `ci-image-pin`, `docs-reminder`, `decisions-guard` (described above) +— live here, **not** in `docker-build.yml`, and that separation is the fix for **ersatztv#535**. + +**Why they are split out.** All three are pure `checkout + git diff` gates on the `small` lane +(no `container:`) and are PR-only (`if: github.event_name == 'pull_request'`). While they lived in +`docker-build.yml` — which also triggers on push to `main` and on `v*` tags — Gitea still +**dispatched them as runner tasks on every such push to evaluate the skip** (the `small`-lane +behavior documented above: *a job is dispatched even when its `if` skips it*). On the **v26.12.0** +release tag those dispatched skip-tasks wedged in act's setup phase and were killed by a runner +restart mid-setup, so they reported `failure` (no logs) and reddened the tag's overall commit +status even though the release built, scanned, and deployed fine. The two PR-only jobs on +`ubuntu-latest` (`api-docs`, `format`) carry the **identical** `if:` and skipped cleanly on the +same tag — the job logic was never the problem; the kill lands in the dispatch window, before any +step or `if:`-skip runs, so tweaking the `if:`/step logic could not fix it. + +**Why a separate workflow fixes it.** Gitea evaluates a workflow's **trigger** before creating any +job, so a `pull_request`-only workflow produces **zero** jobs on a tag/main push: no dispatch, no +kill, no spurious red — for the whole class, permanently. The per-job `if:` guards are kept as +belt-and-suspenders (they also encode "these steps need a PR `base_ref`"). + +**What stays put and why.** These three carry **no** CI toolchain image pin, so `ci-image-pin`'s +grep of `docker-build.yml` still validates the five pin-bearing jobs +(`test`/`migrations`/`functional-e2e`/`api-docs`/`format`) that remain there. `api-docs` and +`format` stay in `docker-build.yml` because they carry the shared-image `container:` + pin and run +on the healthy `ubuntu-latest` lane (where they skipped correctly). None of the three moved jobs is +a **required** check — branch protection requires only `Build & test (.NET)` and `EF migration +integrity` — so relocating them (their status-context prefix changes from `Build ErsatzTV Image / +…` to `PR Gates / …`) does not affect merges. The file declares `defaults: run: shell: bash` +because `ci-image-pin` uses `mapfile`/`set -o pipefail`. + ## CI toolchain image (`docker/ci/Dockerfile`, `.gitea/workflows/ci-image.yml`) The jobs that need a toolchain — `test`, `migrations`, `functional-e2e`, `api-docs`, `format` — @@ -617,7 +655,8 @@ registry. Renovate tracks the Dockerfile's image pins (`dockerfile` manager, see (`entrypoint=["/bin/sleep" "10800"]`), so this is belt-and-braces for anyone running the image by hand — unlike the two above, which are real. -**Bumping the pin is enforced, not remembered.** The `ci-image-pin` job (blocking, PR-only) fails if +**Bumping the pin is enforced, not remembered.** The `ci-image-pin` job (blocking, PR-only; defined +in `pr-checks.yml`, but it greps `docker-build.yml` where the pins live) fails if `docker-build.yml`'s pin isn't the short sha of the last commit to touch `docker/ci/**` or `ci-image.yml`, or if the five jobs ever pin different tags. This exists because **Renovate manages `docker/ci/Dockerfile`'s base pins but cannot bump an opaque `:`** in `container.image` — so a -- 2.47.3