`scripts/ci-toolchain-image-resolves.sh` and `docs/ci-cd.md` both stated that the #772 container jobs "died after 1-2s", and the header used the same number to argue the preflight needs no `needs:` gate. Nobody measured it, and it cannot be measured from a working session without reproducing a deleted-tag incident. What the number stood for is structural and IS known: a container job that cannot pull its image fails AT the pull, before it runs a step, so it wastes no work waiting to be told and the argument against serialising the five jobs survives intact. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
2912 lines
228 KiB
Markdown
2912 lines
228 KiB
Markdown
# CI/CD for the ErsatzTV Fork
|
||
|
||
The fork builds its own Docker image via **Gitea Actions** on the homelab and pushes
|
||
to the **Gitea container registry**. Runner + registry were provisioned in
|
||
server-management#172; the build pipeline is ersatztv#4; test/prod containers are
|
||
server-management#481.
|
||
|
||
## Hosts (read this before trusting a hostname below)
|
||
|
||
| Host | IP | Role |
|
||
|---|---|---|
|
||
| **jazz** | 192.168.1.29 | **Docker host for the media transcoders** — prod `ersatztv` (8409), `ersatztv-test` (8410), Jellyfin. Release scans (`security-scan.sh`) and the prod-copy `migration-smoke.sh` run here. |
|
||
| **bumblebee** | 192.168.1.99 | CI runners (`bumblebee-runner`, `small-runner`), plus every other Docker stack. All the memory/lane measurements below were taken here. |
|
||
| **ci-runner** | VM 127 (pve4) | The other `ubuntu-latest` CI runner. |
|
||
|
||
The transcoders moved bumblebee → jazz on **2026-07-20** (server-management#633).
|
||
|
||
> **Name-reuse trap.** `jazz` was an **earlier name for the .99 host**. Anything written before
|
||
> 2026-07-20 that says "jazz" means today's **bumblebee** — resolve hostnames by **IP**, not by name,
|
||
> and don't "fix" a historical bumblebee reference into jazz.
|
||
|
||
## Versioning & releases
|
||
|
||
The fork inherits upstream ErsatzTV's scheme: **`vYY.<release-seq>.<patch>`** (lightweight, `v`-prefixed git tags).
|
||
|
||
- **`YY`** — two-digit year.
|
||
- **`<release-seq>`** — a sequential release counter **within the year**, reset at each year boundary. It is **not** the calendar month. (Evidence: `v25.2.0` shipped in June 2025, `v25.5.0` in Sep, `v26.3.0` in Feb 2026 — minors don't track months; and `v25.9.0` → `v26.1.0` shows the year-reset.)
|
||
- **`<patch>`** — a small follow-up/hotfix on the *same* release line (e.g. `v26.1.0` → `v26.1.1`, days later).
|
||
|
||
Upstream's final release was **`v26.3.0`** (archived). Our line continues from there:
|
||
|
||
| Tag | Meaning |
|
||
|-----|---------|
|
||
| `v26.3.1` | Upstream 26.3.0 **rebuilt on our infra** (Gitea CI/registry, fork ffmpeg base) — **no application changes**. A patch bump, because nothing functional changed. |
|
||
| `v26.4.0` | First fork release carrying application changes. Later 2026 releases continue `26.5.0`, `26.6.0`, …; a new year resets to `27.1.0`. |
|
||
| `v26.7.0` | Blazor-removal release: ChicoryTV became the only UI. |
|
||
| `v26.8.0` | Secured/versioned ChicoryTV SPA + REST API go-live release (#335). |
|
||
| `v26.9.0` | Configurable advertised IPTV base URL for M3U/XMLTV (#340) + SPA shell/routing + playouts modularization (#247/#245); on-air/Plex/library-path fixes (#99/#345/#371); coverage + functional-E2E CI (#15/#299). |
|
||
| `v26.10.0` | Auto-Tune channel workflow (#69) + weighted content distribution (#70); scheduling refactors, health-check remediation UX (#164), HLS cold-start instrumentation (#350), security hardening (#293/#376/#308). |
|
||
| `v26.11.0` | **QSV profiles decode via VA-API** — `QsvPreferNativeDecoder`, default **on**, fixes ~50% channel cold-start failures on Intel (#498); unified logo/on-screen bug via a shared watermark preset (#67). Media-scanner resilience: Jellyfin mixed-content libraries (#489), music-video scan correctness (#488/#494/#497), remote-stream probing before ffmpeg (#473/#480); weighted-distribution SPA (#404). **First release deployed to `jazz`** (server-management#633). |
|
||
| `v26.12.0` | **`ErsatzTV.Mcp` MCP server** — read + cautious-write over `/api/v1`, `ERSATZTV_ALLOW_WRITES`-gated (#58). **External channel-logo URLs download + cache at save time** (#525), with the on-screen bug now rendered for external-URL logos (#502). HLS cold-start hardening: burst-read the first segments so start isn't `-readrate`-bound (#350) and floor QSV extra hardware frames so an unthrottled read can't exhaust the pool (#529); remote graphics-engine image fetches bounded — timeout, size cap, decode cap, redirects, pooling (#511). Decision-lifecycle tooling + parallel-orientation startup rewrite (#520/#521); CI `docker build` lane rebalance (#508). |
|
||
| `v26.13.0` | **RuleBuilder maturation** — arbitrary-depth group nesting (#436), inline smart-query authoring in Channel Builder (#437), DB-sourced facet typeahead + relative-date operators + validation (#434/#435/#438), and an artist typeahead covering music-video/song credits with `album_artist` no longer 404ing (#578). **Per-channel On Now/Next transient overlay** (#74/#570) and **per-schedule clock-boundary padding** (#392); in-browser channel preview (#60); Auto-Tune per-source weight steppers + exclude/add-untagged (#440). Library-browse pickers now resolve by search instead of a 100-row window, closing several silent at-cap truncations (#644/#650/#651/#634). Correctness: one watermark resolver for all four attachment points, incl. `MiddleCenter` (#503/#510); QSV HDR tonemaps through OpenCL because `vpp_qsv=tonemap` is a silent no-op (#505); `LibraryFolder` unique index + concurrent-insert tolerance (#491); per-library music-video identity with soft trash (#496); Jellyfin Album/Track music-video projection (#177); metadata-collection dedup (#500); accented facet values via a registered Unicode fold on SQLite (#668); `WorkAheadSlots` atomic slot claim, never a negative count (#536/#539); on-demand guide rebuild on thaw (#68). Process/CI: the H10 review-verdict gate became a sha-bound **required** commit status and was hardened through its false-open chain (#622/#629/#632/#648/#649/#672/#698), the decision corpus split to one YAML-frontmatter record per file (#610/#620), and headless Playwright UI-E2E flows landed (#445/#533). Five dual-provider migrations. |
|
||
| `v26.14.0` | **Live TV no longer starves on embedded bitmap subtitles** — `-readrate` paces an input off its *furthest-behind* stream, and a PGS/DVD subtitle read through the video's own `-i` is sparse enough to drag the whole process to **0.53x realtime** against the 1.0x a client consumes, draining the buffer until the channel stalls. Fixed with a capability-gated `-readrate_catchup` (ffmpeg 8.0+) on realtime inputs, keeping `-readrate` on the frame-producing path so the `ffmpeg.qsv-extra-hw-frames-floor` bound is untouched; measured 0.533x → 1.067x on QSV and software, with a 240s QSV soak clean of allocation errors (#726). Affects items carrying an embedded bitmap subtitle matching the channel's subtitle mode — 3,182 of 24,646 media versions on prod, and a property of the *item*, not the channel, which is why the stall presented as random. Process/CI: the H10 review-verdict gate's repair sentinel became a fixed point and its write is now fenced on the timeline retarget count, closing a raced-sentinel false-open (#706/#707/#711). **The decisions validator now cross-checks its dependency-free frontmatter parse against PyYAML** and reports both the truncating unquoted `` #`` and the scalar-closing bare apostrophe as errors, so a record whose `rule:` silently halves under PyYAML fails the local gate instead of CI (#674/#688) — the ceiling-calibration claim was also split so the suite pins what the derivation MEANS rather than live-corpus order statistics. Dependencies: CliWrap 3.10.4, JetBrains.ReSharper.GlobalTools 2025.3.5. |
|
||
| `v26.15.0` | **`main` became PR-only, which is what makes `review-verdict/h10` load-bearing** — branch protection now carries `enable_push: false` *and* `block_admin_merge_override: true`, so a direct `git push origin HEAD:main` is refused at pre-receive for every account including a site admin, and an admin cannot force-merge past a missing or red required context (#743/#749). Gitea only evaluates `status_check_contexts` on the PR merge path, so before this the whole gate was skippable with no forgery needed. The gate was then hardened through the rest of its false-open chain: CI's registry credential scoped so a head-resolved workflow cannot forge the status (#697), a stray expression delimiter **in a comment** that killed the gate outright and is now banned on the release path itself by a `scan` job `build` depends on (#751/#767), a positive verdict inherited only from an allow-listed reviewer (#742), the PR-timeline `HEAD` alias fenced on the `pull_push` count (#803/#664), and both `/statuses/{sha}` reads paged to a validated terminator (#763). App-facing: **FFmpeg profile numeric fields are bounded with a 422 that names the bound and its consequence** instead of being silently rewritten, and readrate pacing is exposed as a profile field (#735 — one dual-provider migration, `Add_FFmpegProfile_ReadRatePacing`). **On Now / Next gets a boxed background and is on by default** (#732). Field-level progressive disclosure lands as a shared `FieldHelp` trigger + portalled panel, adopted on FFmpeg Profiles and documented as `docs/spa-conventions.md` §15 (#734). Auto-Tune proposal rows align on a grid (#721); `AddItemsDialog` resolves by search instead of windowing a whole media-library type, with stale-response and is-mounted guards (#685/#740). Correctness: a paged handler now counts **the same query it pages**, so a filtered page stops reporting the unfiltered total across playouts, rerun collections and six sibling handlers (#690/#758); nullable primitive collections are guarded at the read site rather than mutated on a tracked entity — `SongMetadata.Artists` in both search indexers (#701/#824) and the scheduling `DaysOfMonth`/`MonthsOfYear`/`DaysOfWeek` columns, where a runtime NULL reads as **unrestricted** (#823); SPA full-replace bodies are built as `Complete<T>` so an omitted optional DTO field is a type error (#807); and every `ErsatzTV.Mcp` tool is pinned to its OpenAPI contract, with `graphicsElementIds` and `padToNearestMinute` declared so an MCP channel update stops silently stripping the overlay (#754/#757). Process/CI: one rule for how a guard derives its population and one for the proof it ships with, both enforced (#774/#775) — populations come from the git index, never a filesystem walk (#806/#819), every `MUTATION` grade is executed each suite rather than asserted in prose (#790), and verification code is itself code under test (#796). Workflows declare their own per-job metadata and guard populations derive from it (#786/#789); all six declare `permissions:` (#748); `ci-image.yml` publishes from `main` only and the persisted checkout credential is gone (#744/#746/#835). Hooks report their own execution and the report is measured (#776), and `docs.no-session-narrative` was generalized to every durable artifact and the sites its sweep IDENTIFIED were remediated as judgements rather than by regex — no closure is claimed over a phrasing space (#784/#812). A ruff config is committed and enforced in CI (#780). Dependencies: Scriban.Signed 7.2.6. **Cut at `736649b3b`, not at `main`'s head**: `completeAnnotations.guard.test.ts` (merged in the very next commit) needs a git index the Dockerfile's `web-build` stage does not have, so every image build after `736649b3b` fails — including a tag build (#887). `736649b3b` is the newest commit whose `Build & push image (amd64)` actually ran green. The scheduling-recurrence fix #880 therefore slips to the next release. |
|
||
|
||
**Before cutting a release — sweep `docs/decisions.md` + `docs/decisions/`** (ersatztv#521, supersedes
|
||
the ersatztv#303 H9 append-only ritual). Supersession/retirement is now a same-PR act (add the new
|
||
active record, relocate the predecessor to `docs/decisions/archive/` with reciprocal
|
||
`supersedes`/`superseded-by` links), not a release-boundary batch job — most of the old "consolidate"
|
||
step is now continuous. The release boundary is instead where you:
|
||
1. Run `PYTHONPATH=. python3 scripts/decisions_validate.py` — confirms lifecycle metadata is
|
||
well-formed and every `supersedes`/`superseded-by` link resolves both ways. Since **ersatztv#674**
|
||
it also cross-checks its dependency-free frontmatter parse against **PyYAML when PyYAML is
|
||
importable**, failing on any file PyYAML rejects (a bare apostrophe in a single-quoted value) or
|
||
reads differently (an unquoted ` #`, which YAML truncates as a comment). Where PyYAML is absent —
|
||
the `decisions-guard` job, the Husky hooks — the cross-check is **skipped with a `::notice::`**
|
||
and every other check still runs; the read path stays dependency-free.
|
||
2. Confirm every record already classified `superseded`/`retired` actually lives under
|
||
`docs/decisions/archive/` (the validator fails this, but eyeball it at the boundary too).
|
||
3. Regenerate the active catalog: `PYTHONPATH=. python3 scripts/build_decisions_catalog.py` and
|
||
commit any drift.
|
||
4. Read the corpus size signals. Since **ersatztv#620** these are two separate things:
|
||
- a **per-record prose ceiling** (`decisions_validate.py --record-ceiling <n>`, default **60**)
|
||
— a **non-blocking `::warning::`** naming every record over it. This is the actionable signal:
|
||
it points at a file. The 60 is derived from the distribution, not picked as a round number.
|
||
Its **calibration is guarded in two pieces of different robustness** (ersatztv#688), because
|
||
four earlier single-assertion versions all failed — the first two by being vacuous or
|
||
accepting an absurd ceiling, the last two by ratcheting:
|
||
- **blocking** (`script-tests`) — only the coarse property that the ceiling flags a
|
||
**meaningful minority** of records (`0.02 <= fraction_over <= 0.25`). One record moves a
|
||
fraction by at most 1/N, so no SINGLE ordinary addition can cross it. This is measured
|
||
headroom, not immunity: from today's 18/183 it takes 38 consecutive over-ceiling additions to
|
||
breach the cap, 718 short ones to dilute below the floor, or — the tightest arm —
|
||
consolidating 15 of the 18 offenders away. The floor is
|
||
a fraction rather than "at least one record", which would accept any ceiling up to 229 on the
|
||
live corpus; as a fraction the accepted range is 43..180.
|
||
- **reported, never asserted against the LIVE corpus** — the fine claim that the ceiling sits
|
||
between the **90th and 95th percentile**, i.e. at the tail boundary. `main()` prints a
|
||
`::notice::` when it drifts; the tests assert it only on distributions they own.
|
||
It is an order statistic over a sparse distribution, so a single new record could move p90 by
|
||
21 lines and red the blocking job for whoever wrote it; a ceiling going out of date is
|
||
the passage of corpus growth, not a defect in the commit under test, so it is treated like
|
||
`stale-after`. Re-derive the constant when the notice says so.
|
||
- the **aggregate prose total**, printed every run as an unthresholded `::notice::` **trend**.
|
||
It has no pass/fail. A total over a monotonically growing corpus can only ratchet: the old
|
||
4800→5600 budget went quiet at 5228 after #610 changed the metric and was back over at 5658
|
||
**three and a half hours later the same evening**, with nobody consolidating anything — the
|
||
"permanently red = no signal" failure, not in slow motion at all. It reports record prose and
|
||
non-record scaffolding separately, because they are not the same unit. The generated catalog is no longer counted at all — it
|
||
gains one row per record and cannot be consolidated away.
|
||
|
||
**Being listed by the ceiling is an invitation to check for redundancy, not an instruction to
|
||
cut.** A long record that is entirely distinct findings is a legitimate decline — say so in the
|
||
record and move on. (`--budget` is still accepted and ignored, so old invocations keep working.)
|
||
5. Report the remaining `legacy-unmigrated` count (the validator prints it as a `::notice::`) so the
|
||
backlog is visible, even though it isn't required to hit zero before a release.
|
||
A genuine rationale-prose rewrite still needs a `Decisions-Edit: yes` git trailer on a **non-merge**
|
||
commit in the range (see the `decisions.md` header) — routine lifecycle metadata writes above do not.
|
||
|
||
**Cutting a release:** keep build and promotion as two explicit phases (#335):
|
||
|
||
1. Confirm `main` CI is green; run the full local gate plus `dotnet list package --vulnerable
|
||
--include-transitive`; then push a `vYY.N.P` tag on that exact `main` commit.
|
||
|
||
> **`v*` tags are protected — push as `timothy`.** Since ersatztv#885 the repo carries one
|
||
> tag-protection rule (`name_pattern: v*`, `whitelist_usernames: ["timothy"]`,
|
||
> `release.tag-protection-v-star`), because a `v*` push builds and publishes `:prod`. A push from
|
||
> any other account is expected to be refused — configured, not exercised, since only the
|
||
> `timothy` credential is available here. **The positive half is UNVERIFIED too**: the rule was
|
||
> applied 2026-09-04 and only its negative half was measured (a non-`v*` tag still pushes), because
|
||
> pushing a `v*` tag outside a real cut would publish `:prod`. This cut is its first real
|
||
> exercise. If the tag push is refused, unblock with
|
||
> `DELETE /api/v1/repos/timothy/ersatztv/tag_protections/1`, push, then re-`POST` the rule
|
||
> (prefer a `PATCH` adding the account if the cut has moved to a different operator, so the
|
||
> protection is never left off) — and record the outcome on `release.tag-protection-v-star`,
|
||
> which is waiting for it.
|
||
2. Wait for tag CI to build `:prod` + the immutable `:<version>` + `:<sha>` images. Run
|
||
`scripts/security-scan.sh` on jazz against **the immutable `:<version>` image**, not a
|
||
moving tag, and triage every ZAP/semgrep finding.
|
||
3. Only after the candidate passes, manually `DeployStack jazz-media` and observe its pre-deploy
|
||
output. Prod's compose deliberately follows floating `:prod` (Timothy's 2026-07-11 decision), so
|
||
no CI push or pin bump is needed.
|
||
|
||
> **The Komodo stack is `jazz-media`, not `media-servers`** (verified live 2026-07-20 during the
|
||
> v26.11.0 cut). The *compose project* is still `media-servers` — which is what the container labels
|
||
> show — but the **Komodo stack name** changed with the move to jazz. A stack named `media-servers`
|
||
> still exists on bumblebee and is `unhealthy` (the stopped migration leftovers), so
|
||
> `DeployStack media-servers` silently targets the **dead** stack. Confirm with
|
||
> `/read ListStacks` before deploying.
|
||
>
|
||
> **A third stack, literally named `ersatztv`, is also live and is NOT prod**: it owns
|
||
> `ersatztv-test`, tracks the floating `:latest` tag, and runs with `auto_update: true`. (Stack
|
||
> name, compose project, config-file path and image tag re-confirmed from the container labels on
|
||
> jazz 2026-08-11; the `auto_update` flags and the bumblebee `unhealthy` status are carried from
|
||
> the earlier checks noted above, not re-measured that day.)
|
||
> `DeployStack ersatztv` succeeds and looks healthy — it just promotes nothing to prod. Three names,
|
||
> one target:
|
||
>
|
||
> | Stack name | project_name | auto_update | owns |
|
||
> |---|---|---|---|
|
||
> | `jazz-media` | `media-servers` | `false` | `ersatztv` (**prod**, tracks `:prod`) |
|
||
> | `ersatztv` | `ersatztv` | `true` | `ersatztv-test` (tracks `:latest`) |
|
||
> | `media-servers` | — | — | dead stack on bumblebee, `unhealthy` |
|
||
>
|
||
> Don't resolve the prod stack by name at all — resolve it by the **container's**
|
||
> `com.docker.compose.project.config_files` label (e.g. via `docker inspect`), which points at the
|
||
> actual compose file path and can't be confused by a same-named stack.
|
||
>
|
||
> **There is no Global Auto Update fallback anymore**: `jazz-media` has `auto_update: false`
|
||
> (`poll_for_updates: true` only), so nothing promotes `:prod` on a timer — promotion is manual,
|
||
> full stop. The old "don't cut a tag near the 03:00 run" caveat no longer applies.
|
||
|
||
server-management#585 source-confirmed that Global Auto Update invokes the **same** `DeployStack`
|
||
execution as a manual promotion, and extended the #553 pre-deploy hook to detect a floating-tag
|
||
**digest** change. Either path now takes the fail-closed prod backup; server-management#589 then
|
||
wired `migration-smoke.sh` into that hook, against the exact candidate and the backup it just made.
|
||
A backup, fetch, or migration-smoke failure aborts before the live container is recreated. See
|
||
`homelab-docs/Docker/ErsatzTV.md` for the operational evidence and rollback procedure.
|
||
|
||
**Gotcha:** never put a `[skip ci]` token in a commit you intend to tag — Gitea reads skip-ci from the *tagged* commit and will **suppress the release build**. (Also, `workflow_dispatch` on a tag ref isn't supported on this Gitea version, so the tag *push* must do the triggering.) Release commits, and anything you'll tag, must not contain skip-ci.
|
||
|
||
**Also avoid firing several pushes back-to-back** (e.g. a `[skip ci]` commit, then `main`, then a tag, all within ~1s). Observed once on this Gitea instance: the later events were silently dropped — no `ActionRun` records created at all, even though the runner was online and the workflow `active`. Pushing again, spaced out, created the runs normally. If a push/tag doesn't produce a run, re-push (or push an empty commit) rather than assuming the runner is broken.
|
||
|
||
## Workflow token scope (`permissions:`, ersatztv#748)
|
||
|
||
Every workflow in this repo declares `permissions:`. Five declare `code: read` at the top level;
|
||
`review-verdict.yml` declares `code: write` + `issues: read` + `pull-requests: read` on its
|
||
`set-verdict-status` job and nothing at the top level. That is the whole convention — a new workflow
|
||
should declare the narrowest set it needs, and say at the declaration why it needs write if it does.
|
||
|
||
**Declaring `permissions:` is exhaustive, not additive.** A unit omitted is a unit not granted, and
|
||
that holds *at any owner-level default* — it is not conditional on the instance default being
|
||
Restricted. This is the property that makes the block load-bearing rather than decorative, and it is
|
||
also what makes a wrong one dangerous: the gate job's API calls are `curl -sf` under
|
||
`set -euo pipefail`, so a missing unit is a 403 that kills the step, and `review-verdict/h10` is
|
||
never posted. That blocks **every** merge to `main`, including the PR that would repair it, and
|
||
since ersatztv#743 (`block_admin_merge_override: true`) there is no force-merge escape.
|
||
|
||
`code: write` and not something narrower because **Gitea has no `statuses` scope**.
|
||
`POST /repos/{o}/{r}/statuses/{sha}` is gated by `reqRepoWriter(unit.TypeCode)`, so status-write *is*
|
||
Code-write (`ci.actions-credential-scoping`). `contents: write` would also work but additionally
|
||
grants `releases: write`, which nothing here uses. The two read units are not spare: the job GETs
|
||
`/issues/{n}/timeline` (the ersatztv#706 retarget fence) and, through
|
||
`scripts/pr-changed-files.sh`, `/pulls/{n}` and `/pulls/{n}/files` (the ersatztv#698 exemption path).
|
||
|
||
No workflow needs a `packages:` unit. Since ersatztv#885 the five `container:` blocks in
|
||
`docker-build.yml` carry no `credentials:` at all: they run on the `pull_request` route, so they may
|
||
hold no stored secret, and the runner pulls the toolchain image ANONYMOUSLY — this registry issues a
|
||
pull token for a public package with no credential (measured at the HTTP level 2026-09-04, and at the
|
||
daemon level 2026-09-05 — a `docker pull` of the pinned tag with an empty docker config succeeds on
|
||
the runner host, which is the same daemon that creates the job containers; see
|
||
`ci.pr-route-carries-no-stored-credential`, whose `mechanics:` names both measurements and the two
|
||
things neither exercises). So no job pulls that image with the injected
|
||
token, and none pulls it with the `REGISTRY_*` PAT either.
|
||
|
||
### The binding was measured, not assumed (2026-08-27, Gitea 1.27.1)
|
||
|
||
`ci.actions-credential-scoping` recorded for three weeks that whether `permissions:` is honored on
|
||
this instance was **unprobed** — 1.26.0 was the version it was said to start binding at, from
|
||
release notes rather than from an experiment. It was settled with the scratch-base probe below, run
|
||
as a matched pair that differed **only** in one unit:
|
||
|
||
| Control | Gate job unit | Result |
|
||
|---|---|---|
|
||
| Positive | `code: write` | job `success`; `Posted review-verdict/h10-PROBE=success`; `creator: null` |
|
||
| Negative | `code: read` | job `failure`; `curl` exit 22 (HTTP ≥400 under `-f`) at the POST; **no status written** |
|
||
|
||
Both runs reached the classify step's start marker, so neither failure was the ersatztv#751
|
||
dropped-step shape. What shows each read unit was exercised is a different observation for each, and
|
||
they are not interchangeable:
|
||
|
||
- **`issues: read`** — both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)`, which
|
||
requires the timeline walk to have read `/issues/{n}/timeline`. That log line and the function
|
||
behind it were renamed by ersatztv#803, which added the head axis to the same walk: the line now
|
||
reads `Mutation fence: N retarget event(s) and M push event(s) … (trusted=…)` and the function is
|
||
`count_pr_mutations`. The observation above is left as it was recorded — it is what those runs
|
||
printed — but grep for the new strings when reproducing it.
|
||
- **`pull-requests: read`** — the positive control posted `=success`, and a `success` is reachable
|
||
only through `complete=yes`, which requires `scripts/pr-changed-files.sh` to have paged
|
||
`/pulls/{n}` and `/pulls/{n}/files` to a validated end. The fence and the start marker do **not**
|
||
show this; the `success` does.
|
||
|
||
The negative control is the load-bearing half: it is what shows the declaration *binds* rather than
|
||
being a no-op that happens to sit next to a permissive default. Both controls carried the block **at
|
||
job level**, on `set-verdict-status` — the same level and the same job as the shipped
|
||
`review-verdict.yml`, so the gate's own shape is what was measured.
|
||
|
||
Both ran while the owner default was still `permissive`, so the binding holds **today**, without any
|
||
flip. One scope limit, stated rather than glossed: the other five workflows declare `permissions:` at
|
||
**top level**, a form the probe did not exercise. That is inferred, not measured — but the failure
|
||
direction is safe (a top-level key Gitea ignored would be a security no-op, never a brick), and
|
||
`docker-build.yml` and `pr-checks.yml` are `on: pull_request`, so this change's own CI exercises
|
||
theirs.
|
||
|
||
**What the probe does NOT establish — read this before flipping the owner default.** It says nothing
|
||
about what a declared `code: write` receives under a **Restricted** default. GitHub's semantics let
|
||
`permissions:` only *narrow*, never widen past the default; if Gitea copies that, Restricted would
|
||
cap the gate job at read and `review-verdict/h10` would become unwritable — precisely the failure
|
||
this section warns about, with no force-merge escape. Both controls ran under `permissive`, so that
|
||
case is untested. server-management#714 stays open for exactly this: re-run the scratch-base probe
|
||
**under Restricted** before trusting the flip.
|
||
|
||
`creator: null` on the positive control matters separately: it is the provenance signal
|
||
`review-verdict.yml` relies on to tell a machine-written status from a reviewer's
|
||
(`ci.exemption-provenance`, ersatztv#742). Approach A — scoping the built-in token — preserves it.
|
||
Moving status writes onto a PAT (approach B) would not, which is why B stays blocked.
|
||
|
||
**What this does and does not close.** Scoping bounds what a job may *do*; it never bounds whether
|
||
attacker YAML *runs* — that needs a trigger filter (ersatztv#744). And `review-verdict.yml` still
|
||
holds repo-write. The property gained is not "workflows can no longer assert the verdict"; it is
|
||
that *only a workflow visibly opting into repo-write can*, instead of every job on the instance
|
||
holding write implicitly. Real, and weaker than closed.
|
||
|
||
⚠️ **Changing `review-verdict.yml`'s block requires the scratch-base probe**, not CI-green on the
|
||
editing PR — that workflow is base-resolved and is never exercised by its own PR, so its own checks
|
||
say nothing about the edit. See the probe recipe in the review-verdict gate section below.
|
||
|
||
The other five split two ways, and only one half is covered by an editing PR's own CI:
|
||
|
||
- `docker-build.yml` and `pr-checks.yml` run on `pull_request`, so an editing PR exercises their
|
||
blocks directly. (`docker-build.yml`'s `build` job carries
|
||
`if: github.event_name != 'pull_request'`, so its registry-push path is first exercised on the
|
||
post-merge push to `main` — that reddens `main`, not the gate, and is recoverable.)
|
||
- `ci-image.yml` (`push` to `main` only since ersatztv#744, plus `schedule`/`workflow_dispatch`),
|
||
`dependency-scan.yml` and `renovate.yml` are **not** exercised by an editing PR at all — their
|
||
first run is after merge or on the next schedule. None is a required check, so a wrong block there reddens a non-blocking run rather than the gate, but do not read an
|
||
editing PR's green as covering them.
|
||
|
||
**`ci-image.yml` is included, and only just.** It was the one workflow this change could not
|
||
originally reach: editing it re-pointed `ci-image-pin`'s `expected` at the editing commit and
|
||
reddened a **blocking** job, and its own `paths:` made the edit publish a toolchain image. ersatztv#748 briefly
|
||
carried a documented one-name exemption for it. ersatztv#744 (`ci.toolchain-image-publish-is-a-dispatch`)
|
||
took that path out of both, so the exemption was **deleted rather than documented** — which is the
|
||
form ersatztv#835 asked for, and it is why `test_pr_changed_files.py` asserts over the whole derived
|
||
population with nothing carved out.
|
||
|
||
## Checkouts drop the persisted credential (ersatztv#746)
|
||
|
||
**Every** `actions/checkout` in this repo sets `persist-credentials: false` — with no exemption,
|
||
since ersatztv#744 removed the one that existed (see "The last exemption, and how it closed" below),
|
||
and ersatztv#835 now holds the rule mechanically rather than by convention:
|
||
`scripts/tests/test_workflow_persist_credentials.py` derives the workflow population from the git
|
||
index and asserts the flag on every checkout step it finds. Without it the action leaves an
|
||
`Authorization` header in
|
||
`.git/config`, and a head-resolved job then hands that credential to every script it runs, not just
|
||
to the checkout. That header is write-capable *only while the job's token is*: this instance's Actions default
|
||
token permission is `permissive`, where it stands after a Restricted trial was reverted on
|
||
2026-08-05 — but since ersatztv#748 that default no longer decides it. Every workflow declares
|
||
`permissions:` (see the section above), so every persisted header in this repo is read-only by
|
||
declaration. Re-applying the owner-level default is server-management#714. No job that checks out
|
||
loses anything: none of them push with git, and anonymous access covers the fetches because the repo
|
||
is public — `private: false`, and `GET /info/refs?service=git-upload-pack` answers unauthenticated,
|
||
measured 2026-08-26 from the LAN. That measures the repository, not a runner's network path to it,
|
||
and the runner path is deliberately not asserted here: it does not need to be, because the fetches
|
||
in the jobs below are unmasked: an anonymous fetch that failed from a runner reddens `api-docs`,
|
||
`Formatting` and `decisions lifecycle` instead of being swallowed. (The two masked fetches further
|
||
down are covered by their own argument, not by this one.) Renovate does push branches, but
|
||
`renovate.yml` has no `actions/checkout` step at all and carries its own `RENOVATE_TOKEN`.
|
||
|
||
The base-ref fetches that feed the changed-file skip logic — in `docs-reminder`, `decisions
|
||
lifecycle`, `api-docs` and `Formatting` — therefore run **unmasked**: a failed `git fetch` — and a failed
|
||
`git diff` at the three steps that run one inline — exit the step with an `::error::` instead of
|
||
yielding an empty changed set. That ordering is the general rule, not a fact about one
|
||
change: unmask a dependent fetch *before* you drop a persisted credential anywhere. Under `|| true`
|
||
a credential regression looked like "nothing changed" on the three steps that also masked
|
||
`git diff`, so those checks skipped themselves silently on a green job; on the other two it handed
|
||
a possibly-absent `origin/<base>` to a Python validator. Neither shape is one you want a credential
|
||
change to land on top of.
|
||
|
||
**What this does and does not make fail-closed.** A failed *fetch* now reddens `api-docs`,
|
||
`Formatting` and `decisions lifecycle`, which is the credential/network case this change exists for.
|
||
It does not make the two validator steps fail-closed in general: they run no inline `git diff`, and
|
||
if the base resolves badly despite a successful fetch, `scripts/decisions_validate.py` emits a
|
||
`::warning::` and skips its diff-dependent checks while `scripts/check-doc-narrative.py` prints
|
||
`SCANNED NOTHING` and exits 0. Both fail open there by their own design and are out of scope here —
|
||
do not read a green `decisions lifecycle` as proof its diff-dependent checks ran. Note which side of
|
||
that line the `Decisions-Edit: yes` trailer check falls on: `_edit_trailer_armed` is reached only
|
||
after the merge base resolves, so it goes quiet with the rest of them rather than surviving.
|
||
|
||
**Loud is not the same as blocking, and the split is by job.** `api-docs`, `Formatting` and
|
||
`decisions lifecycle` gate on this diff, so a failed fetch reddens them. `docs-reminder` does not —
|
||
it is a non-blocking nudge, and an advisory red still joins the combined status the merge gate
|
||
reads. All three of its non-checkout steps therefore carry `continue-on-error: true` — the parity
|
||
nudge, `Set up Python` and the doc-narrative warning — so the `::error::` reaches the log without
|
||
turning a warning into a merge block. `Checkout` is deliberately still blocking; with no tree there
|
||
is nothing to warn about.
|
||
|
||
**None of these fetches passes `--depth`.** All five run in jobs that check out `fetch-depth: 0`,
|
||
and `git fetch --depth=N` on a *complete* clone grafts it shallow: the history is still there but
|
||
cut off, so `git diff origin/<base>...HEAD` fails with `no merge base` once the branch point is
|
||
further back than N. Measured 2026-08-26: a 150-commit repo branched 130 back, `--depth=100`,
|
||
`.git/shallow` created, three-dot diff `fatal: no merge base`; the same fetch without `--depth`
|
||
returns the correct file list. Under the old `|| true` that surfaced as a silent skip; unmasked it
|
||
would have reddened `api-docs` and `Formatting` on any branch not rebased recently, sent
|
||
`docs-reminder`'s parity nudge advisory-red, and left `decisions lifecycle` green with its
|
||
diff-dependent checks silently skipped — so the depth came off with the mask. The same graft,
|
||
reached through the detector script rather than a workflow step, is what stamped `:latest` images
|
||
`InformationalVersion 0.0.0-<sha>` until ersatztv#836 — see below.
|
||
|
||
**Two masked fetches that stay masked, deliberately.** `scripts/ci-detect-docs-only.sh` and
|
||
`scripts/ci-detect-already-validated.sh` both swallow a failed fetch — and should. In both, a failed
|
||
fetch can only leave the base revision unresolvable, and an unresolvable base emits `false`: *run
|
||
the full matrix / full validation*. Neither can turn a failed fetch into a skip. Masking there costs
|
||
runner time and nothing else, which is the opposite direction from the workflow steps above, where
|
||
the same masking bought a silently skipped check. Judge these by which way the failure falls, not by
|
||
the `|| true`.
|
||
|
||
That clears their **masking** and nothing else. Their `--depth` was a separate question with a
|
||
separate answer, and it is the one that shipped a defect (ersatztv#836).
|
||
|
||
**A depth is passed only to a checkout that is already shallow.** `ci-detect-docs-only.sh` routes
|
||
both its fetches through `fetch_ref`, which asks `git rev-parse --is-shallow-repository` and drops
|
||
the `--depth` on any answer but `true`. The script has **exactly four consumers** — `test`,
|
||
`migrations` and `functional-e2e` at `fetch-depth: 2`, and `build` at `fetch-depth: 0`. Enumerate
|
||
them with `grep -rn ci-detect-docs-only .gitea/`, not by looking for a step with `id: detect`:
|
||
`api-docs` and `format` have one of those too, and it runs their own inline diff.
|
||
|
||
So `build` is the only complete-clone consumer, and the push arm is the only arm it takes. That is
|
||
where the cost landed: its `--depth=2` grafted `build`'s complete clone, the `git describe --tags`
|
||
in the immediately following step found no reachable tag, and a `|| echo v0.0.0` fallback that
|
||
cannot fail turned that into a version. Every `:latest` image published from `main` carried
|
||
`InformationalVersion 0.0.0-<sha>` instead of `26.x.y-<sha>` — read off the live test container as
|
||
`0.0.0-5a7a4ed7` on 2026-08-26, six weeks after the depth landed with #416 on 2026-07-17, because
|
||
nothing went red.
|
||
|
||
The PR arm's `--depth=200` had **no complete-clone consumer at all** (on `pull_request` only the
|
||
three shallow jobs run this script), so it was inert rather than latently firing. It goes through
|
||
the same helper anyway, because the clause is shared and the next `fetch-depth: 0` consumer of that
|
||
arm would inherit the graft silently. Keeping the depth for the shallow three is likewise
|
||
conservative rather than required: a depth-less fetch into a `--depth=2` clone leaves `.git/shallow`
|
||
in place, still writes `FETCH_HEAD`, and still reports the parents the push arm reads.
|
||
|
||
Both halves are now held by something that executes. `scripts/tests/test_docs_only_detector_clone_depth.py`
|
||
drives the real script over real `file://` clones and asserts the clone is still complete and
|
||
`git describe` still resolves afterwards, with a negative control that requires the fixture to be
|
||
able to graft at all — a plain path clone silently ignores `--depth`, which would make the whole
|
||
file vacuous. And `Compute version and tags` no longer defaults: a failed `git describe` on the
|
||
non-tag path fails the job rather than stamping `0.0.0`, so no `:latest` is published instead of a
|
||
mislabelled one. The tag path never calls `describe`, so a release cut is unaffected either way.
|
||
|
||
### The last exemption, and how it closed
|
||
|
||
`ci-image.yml`'s checkout was the one left unset, and the reason was mechanical rather than a
|
||
judgement that it was safe: any commit touching `.gitea/workflows/ci-image.yml` re-pointed
|
||
`ci-image-pin`'s `expected` at itself and reddened that blocking job, and the file's unfiltered
|
||
`push` trigger published a toolchain image from whatever branch the edit landed on. Both are gone
|
||
(ersatztv#744): the trigger is `branches: [main]` and the file is no longer one of `expected`'s
|
||
paths, so the flag went on in the same change. **What this costs the guard is nothing, and that is
|
||
the point** — ersatztv#835 was deliberately held back rather than shipped with a one-entry
|
||
`ci-image.yml` exemption, because such an entry outlives its reason silently: once the flag went on
|
||
there the exemption would still have passed and the guard would have been permanently blind to the
|
||
file it was written for.
|
||
|
||
## Per-job declarations (`env.CI_EXECUTION_CLASS`, `env.CI_JOB_ROLE` — ersatztv#786/#789)
|
||
|
||
**Every job in every tracked workflow declares `CI_JOB_ROLE`; the jobs in `docker-build.yml` also
|
||
declare `CI_EXECUTION_CLASS`** (the scopes differ — see the table). They are metadata, not runtime
|
||
inputs: nothing reads them at execution time. They exist so that a guard comparing sets of jobs
|
||
derives its expected set from the workflow instead of from a literal list in the checker.
|
||
|
||
| Marker | Values | Scope | Derived by |
|
||
| --- | --- | --- | --- |
|
||
| `CI_EXECUTION_CLASS` | `toolchain`, `bare-runner` | `docker-build.yml` only — the sole workflow using the toolchain image | `scripts/tests/test_ci_image_pin_population.py` |
|
||
| `CI_JOB_ROLE` | `guard`, `report-only`, `none` | every tracked workflow | `scripts/tests/test_workflow_job_guards.py` |
|
||
|
||
**A missing marker and an unrecognised value both fail the suite.** Neither defaults: a scheme whose
|
||
absent value reads as some safe class stops applying the moment a job is added and nobody notices.
|
||
|
||
**`CI_JOB_ROLE` is decided by what the job PRODUCES.** A `guard` job's output is a **verdict**; a
|
||
`none` job's output is an **artifact**, and a red there means the build did not work rather than that
|
||
an invariant was violated. `docker-build.yml::build` publishes an image and smoke-tests it, and is
|
||
`none`. Which jobs are `none` is read off the markers rather than listed here — a list in prose is a
|
||
second copy of the workflow that rots on the next job added. Every `guard`/`report-only` job holds a
|
||
row in `docs/guard-inventory.md` → "Workflow-job guards", cross-checked both ways.
|
||
|
||
`report-only` is the third value: a verdict the job **cannot enforce**, because every check step
|
||
carries `continue-on-error: true`.
|
||
|
||
**A rejected alternative, so it is not re-adopted:** "a guard enforces an invariant about the
|
||
*repository*, so a job exercising the *product* is `none`" reads as more principled and is wrong —
|
||
it puts `test` and `migrations` outside the population, and those are the two **required** status
|
||
contexts on `main`, i.e. precisely where a failure to fire is fail-open against branch protection.
|
||
|
||
**Why not a literal list**, since one stood here until #789 with a real argument behind it: set
|
||
equality between two *derived* sets is blind to a member leaving both at once, so a job that loses
|
||
its `container:` block leaves the declared and the pinned set together and the comparison stays
|
||
balanced. That needs an anchor that does not move with the block — and a reviewed literal was the
|
||
only one available. The marker is the same anchor, better placed: it lives in the job it describes,
|
||
so it is reachable by anyone reading that job and it moves with the job when the job is renamed.
|
||
(Not "in the same hunk" — measured, the marker sits 6–7 lines from the `container:` block in four
|
||
jobs and **58** in `migrations`, where the `services:` block sits between them. Proximity is a
|
||
convenience; the property the guard rests on is that the marker does not move when the block does.)
|
||
|
||
**Its cost is proximity**, so the scheme does not stop there. A literal in a distant file survives a
|
||
careless workflow edit; a marker a handful of lines from the `container:` block can go with it. So
|
||
`test_no_job_needs_the_toolchain_without_declaring_it` derives the requirement a third time — from
|
||
each job's own step bodies, looking for tools that exist only in the CI image (`dotnet`,
|
||
`dotnet-ef`, `node`, `npm`, `npx`, `ffmpeg`, `ffprobe`, `playwright`, `reportgenerator` — the list
|
||
lives in `TOOLCHAIN_ONLY_TOOLS`, and this sentence is a summary of it, not a second copy to keep in
|
||
sync). `git`/`jq`/`python3`/`curl`/`tar` are **excluded** because they do not discriminate: the
|
||
bare-runner lanes provide or provision them too. That check depends on neither the marker nor the
|
||
block, and it is the only one that catches a .NET step **moved** into a bare-runner job, where no
|
||
set changes at all.
|
||
|
||
It is a **necessary condition only**: a job whose *only* toolchain use sat behind a script would be
|
||
invisible to it. Measured — the blind-spot set is **empty today**: all five declared toolchain jobs
|
||
are detected directly, `functional-e2e` included (it runs `dotnet restore` and `npm ci` in its own
|
||
steps before handing off to `scripts/e2e-local.sh`). The converse is still not asserted, because a
|
||
future job could be written that way and a red on a correct tree is how a guard gets deleted.
|
||
|
||
**The residual, stated because it is the one that matters.** All three checks go BLIND together
|
||
(none of them reddens) under one plausible edit: drop the `container:` block, flip the marker, *and* move the invocation into a
|
||
script. Both set comparisons then stay balanced and the step scan sees no token. What bounds this is
|
||
the failure **mode**, not its likelihood — the job runs a missing binary and dies with
|
||
`dotnet: command not found`, loud and immediate, where ersatztv#774's original defect sent a
|
||
*required* check green on the bare runner. Trading a silent pass for a noisy crash is the point;
|
||
it is not a claim the hole is closed.
|
||
|
||
Full rationale: `docs/decisions/records/testing/workflow-declares-its-own-job-metadata.md`.
|
||
|
||
|
||
## The workflow: `.gitea/workflows/docker-build.yml`
|
||
|
||
Single workflow. Gating jobs `test` + `migrations` run in parallel and gate `build`; a
|
||
non-blocking `docs-reminder` job runs on PRs only (see below). Prod deploy is **not** a CI
|
||
job — it's Komodo Global Auto Update off the `:prod` tag (see "Cutting a release").
|
||
|
||
### Triggers & tags
|
||
|
||
| Trigger | `test` job | `build` job | Image tags pushed |
|
||
|---------|:----------:|:-----------:|-------------------|
|
||
| `pull_request` | ✅ | — (skipped) | none |
|
||
| push to `main` | ✅ | ✅ | `:latest` + `:<short-sha>` |
|
||
| push tag `v*` | ✅ | ✅ | `:prod` + `:<version>` + `:<short-sha>` |
|
||
| `workflow_dispatch` | ✅ | ✅ | only if ref is `main`/`v*`, else build-only (no push) |
|
||
|
||
A **docs-only** change (see "Docs-only skip" below) reduces every ✅ above to a seconds-long no-op
|
||
that still reports its status.
|
||
|
||
`:latest` is the **test/dev** channel (every `main` commit). Prod's compose **follows the
|
||
floating `:prod`** tag (reverted from the 2026-07-07 version pin on 2026-07-11) — never
|
||
`:latest`. Both `:prod` and `:<version>` are produced by pushing a `v*` tag; prod tracks
|
||
`:prod` and is redeployed by Komodo Global Auto Update (see "Cutting a release"). The
|
||
immutable `:<version>` tags remain for reproducible rollback (`docker run …:26.6.0`).
|
||
|
||
Concurrency is scoped **per event+ref** (`group: ersatztv-build-${{ github.event_name }}-${{ github.ref }}`,
|
||
`cancel-in-progress` for PRs): PR runs parallelize across PRs, a new sync auto-cancels
|
||
its superseded run, and image builds still serialize within their own ref. Do NOT push
|
||
`main` and a `v*` tag simultaneously — those are separate groups but share the
|
||
`:buildcache` tag and the smoke container name; tag only after the main build is green.
|
||
(History: originally one global group serializing ALL runs for the single runner —
|
||
with three runners that starved the queue; changed 2026-07-11, server-management#574.)
|
||
|
||
**Four** runners serve the fork (server-management#570/#574/#639):
|
||
|
||
| Runner | Host | Label | Slots | Per-job cap |
|
||
|---|---|---|---|---|
|
||
| `ci-runner` | VM 127 (pve4) — no prod workload | `ubuntu-latest` | 4 | `--cpus=4 --memory=10g` |
|
||
| `bumblebee-runner` | bumblebee — prod media | `ubuntu-latest` | 2 | `--cpus=4 --memory=10g --cpu-shares=256` |
|
||
| `small-runner` | bumblebee — prod media | **`small`** | 2 | `--cpus=1 --memory=1g --cpu-shares=256` |
|
||
| `jazz-small-runner` | jazz — prod media (#633) | **`small`** | 2 | `--cpus=1 --memory=1g --cpu-shares=128` |
|
||
|
||
The `small` lane exists because Gitea dispatches a job as a runner task **even when its
|
||
`if` skips it**, and those skip-tasks used to wait behind long builds (observed 31 min),
|
||
stalling every PR run. `--cpu-shares` below the default 1024 is what makes a runner on a
|
||
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` — plus `prove-fix` (ersatztv#794), the heaviest member: per commit carrying a
|
||
`Proves:` trailer it makes two `git worktree add`s and runs an arbitrary pytest selection twice,
|
||
so a PR claiming many proofs costs proportionally more than the rest of the lane combined — plus
|
||
`script-tests`, which is a checkout plus a `pytest` run needing only
|
||
`pytest` and `pyyaml` (ersatztv#631; it is NOT stdlib-only — that assumption is what turned the
|
||
job red on its first CI run, see below) — plus **`scan`** (ersatztv#767), the same
|
||
lightweight-Python shape, and **`toolchain-preflight`** (ersatztv#772), a checkout plus one `curl`.
|
||
Those last two are the lane members that live in `docker-build.yml` rather than `pr-checks.yml`, so
|
||
they are the ones that also run on a **tag push**. `scan` is still the one to think hardest about
|
||
before changing anything here: its failure does not merely redden a status but **skips `build`** —
|
||
an OOM or a wedge there yields no release image at all. `toolchain-preflight` is a `needs:` of nothing, by
|
||
design — it does not gate the jobs it diagnoses. It is not consequence-free either: like any red
|
||
job it lands in the PR's combined status, which the merge gate reads (see "When the pinned tag
|
||
disappears").
|
||
Nothing there runs a compiler or a `docker build`, which is why the lane
|
||
can be capped at 1 GiB per job. The lightweight-Python jobs are the deliberate edge of the
|
||
"git-only" rule, not an exception to it: `setup-python` + `pip install pytest` + a suite whose
|
||
heaviest allocation is a handful of temp-dir git repos stays far under the cap. Route a heavy job here and it will OOM — give it
|
||
`ubuntu-latest`, or its own label on `ci-runner`, the only host with no prod workload.
|
||
|
||
**Lane assignment (ersatztv#390).** *Slot counts below are as-of 2026-07-17; the table above is
|
||
current.* At the time, the `ubuntu-latest` lane had **4 slots** (2 + 2) and the
|
||
`small` lane 4. A 2026-07-17 audit of the Actions API found the `ubuntu-latest` lane
|
||
saturated and the `small` lane idle — **queue wait exceeded every job's runtime**:
|
||
|
||
| Job | Runtime | Queue wait | Lane |
|
||
|---|---|---|---|
|
||
| `test` | 354s | 1363s | ubuntu-latest |
|
||
| `migrations` | 639s | 1428s | ubuntu-latest |
|
||
| `functional-e2e` | 520s | 1447s | ubuntu-latest |
|
||
| `api-docs` | 5s | **1722s** | ubuntu-latest → `small` → **reverted to ubuntu-latest** (#406) |
|
||
| `format` | 37s → **~0.5s** (#469) | **1731s** | ubuntu-latest → `small` → **reverted to ubuntu-latest** (#406) |
|
||
| `docs-reminder` / `decisions-guard` | 10s | **5s** | small |
|
||
|
||
`api-docs` and `format` moved to `small` because the queue wait dwarfed their runtime. Both lanes
|
||
run the *identical* `runner-images:ubuntu-latest` base, so `small` was a label with spare capacity,
|
||
not a different capability — a move only possible because those jobs now run in the **CI toolchain
|
||
image** (below) and no longer need the runner image to supply .NET/Node.
|
||
|
||
**Reverted 2026-07-17 (ersatztv#406 / server-management#604).** #390's own caveat — *"on an
|
||
API-touching PR `api-docs` does a full `dotnet build`, so it is not always small"* — turned out to
|
||
be the deciding factor, and "capacity 4 absorbs that" held only because **nothing enforces the sum**
|
||
of the lanes' per-job caps. Each job container is correctly capped (`--memory=10g`), but 6 slots ×
|
||
10 GiB = **60 GiB on a 25 GiB host** that also runs prod media; on 2026-07-17 bumblebee hit load
|
||
340 with 21 GiB swapped. These were not small jobs — a live `docker stats` caught the `format` job
|
||
container at **3.95 GiB**, which the re-sized 2 GiB `small` lane would OOM-kill outright. #604 fixes
|
||
the queue at the source instead (`ubuntu-latest` grown to 5 slots: a 48 GiB ci-runner at capacity 4
|
||
plus a bumblebee overflow slot), so the `small` lane can be reserved for genuinely-tiny shell jobs.
|
||
(The `format` half of this is now **moot**: **ersatztv#469** moved it to `dotnet format whitespace .
|
||
--folder`, which loads no Roslyn workspace — the job's 3.95 GiB heap and multi-minute runtime are
|
||
gone, so it is no longer a reason to keep the lane large. `api-docs` on an API-touching PR still is.)
|
||
|
||
Queue wait is still a dominant cost and capacity is server-management's boundary — tracked in
|
||
**server-management#604**. The redundant triple-build behind those runtimes is **ersatztv#398**.
|
||
|
||
**The other failure mode: setup-phase starvation (server-management#639, 2026-07-20).** The
|
||
table above measures *queue wait* — time before a job is dispatched. A saturated lane also
|
||
produces a second, much more confusing symptom: a job that *is* dispatched, sits `in_progress`
|
||
for >10 minutes, writes **no log file at all** (`OpenLogs … .log.zst: file does not exist`),
|
||
and then fails — wedged in act's job-**setup** phase, before Checkout. Same-config siblings
|
||
that started 90s earlier finished in seconds; a concurrent job's log showed a normally-fast
|
||
compile taking a 7-minute gap between projects. This is the origin of the "`decisions.md` is a
|
||
known flake, just rerun it" folklore: the rerun succeeds only because it lands after load
|
||
clears, so the guard's logic gets blamed for a capacity problem.
|
||
|
||
The fix was **not** more capacity for its own sake. `small` was stuck at one slot because it
|
||
still held two heavy jobs — `docker-build.yml`'s image build and `ci-image.yml`'s toolchain
|
||
buildx (the latter reads as lightweight because it is "docker-only", but it is the heaviest
|
||
thing that ran in the lane) — and their 10 GiB requirement set the lane's per-job cap, which
|
||
on a 25 GiB host permits exactly one slot. Moving both to `ubuntu-latest` made the lane
|
||
genuinely tiny, so it could widen to **4 slots across two hosts while committing less RAM to
|
||
CI than the single slot did**. `docker-build.yml`'s `build` does not re-create #574's
|
||
skip-task queueing, because `needs: [test, migrations, scan]` means it cannot be dispatched until
|
||
the lane it would queue behind has already drained.
|
||
|
||
### CI build memory: no persistent compiler servers (ersatztv#406)
|
||
|
||
Roslyn's `VBCSCompiler` is a **persistent** compiler server — it outlives the `dotnet build` that
|
||
started it and keeps its heap warm for the next one. Locally that is a genuine speedup; in CI it
|
||
buys nothing, because each job container is torn down at the end of the run and there is never a
|
||
"next build" to warm. It was measured at **7.8 GB RSS** on bumblebee — the single largest consumer
|
||
on the host, and the reason each job needed a 10 GiB cap in the first place.
|
||
|
||
So the workflow's top-level `env:` disables the servers for every runner-side dotnet job:
|
||
|
||
| Variable | Effect |
|
||
|---|---|
|
||
| `UseSharedCompilation=false` | no persistent `VBCSCompiler`; `csc` runs per project and exits |
|
||
| `DOTNET_CLI_USE_MSBUILD_SERVER=0` | no persistent MSBuild server process |
|
||
| `MSBUILDDISABLENODEREUSE=1` | MSBuild worker nodes exit with the build instead of lingering |
|
||
|
||
These are MSBuild properties set as environment variables so they apply to every `dotnet`
|
||
invocation without touching each call site (MSBuild surfaces env vars as properties, and
|
||
`UseSharedCompilation` is only defaulted to `true` when empty, so the env var wins).
|
||
|
||
**What this does and does not shrink.** It helps the jobs that *compile* — `test`, `migrations`,
|
||
`api-docs` on an API-touching PR, and the in-Docker `build`. It never helped `format`: `dotnet
|
||
format` loaded Roslyn in-process via MSBuildWorkspace and never spawned `csc`, so the compiler-server
|
||
env vars left its measured 3.95 GiB untouched. (Moot since **ersatztv#469** switched `format` to
|
||
`dotnet format whitespace . --folder`, which skips the MSBuild/Roslyn workspace entirely — the job is
|
||
now a ~0.5 s, low-memory whitespace/BOM check with no Roslyn heap. See *Static analysis &
|
||
formatting → Formatting* below.)
|
||
|
||
The same three are repeated in `dependency-scan.yml`; workflow `env:` does not cross workflow files.
|
||
That one is lower-stakes (restore/list are MSBuild-driven, so it's lingering worker nodes rather
|
||
than a 7.8 GB VBCSCompiler) but it runs unattended on a cron against the prod media host.
|
||
|
||
**The workflow `env:` does not reach the `build` job's compilation**, which happens inside
|
||
`docker build` — the same three are set as `ENV` in the SDK stage of `docker/Dockerfile`. That is
|
||
the job server-management#570 measured pegging 5.999/6 GiB, so it is the one that most needs this.
|
||
Build-stage only; the final image is `FROM runtime-base`, so nothing lands in the shipped image.
|
||
|
||
The `test` job samples its own memory and reports it every run. Two steps (`continue-on-error`, so
|
||
they never fail a build), driven by `scripts/ci-peak-anon.sh` (ersatztv#412):
|
||
|
||
- **Start peak-anon sampler** (before the dotnet Build/Test/Coverage steps) launches a detached
|
||
background poller that tracks the high-water mark of the cgroup's **anon** memory every 2 s.
|
||
- **Report peak container memory** (the job's last step) stops the sampler and prints the sampled
|
||
**peak anon** — the headline number — alongside `memory.peak` and the end-of-job `anon`/`file`
|
||
split, to the log and the job step summary.
|
||
|
||
Read the **peak anon** off a recent run to size a cap — not `memory.peak`, and here is why:
|
||
|
||
> ⚠️ **`memory.peak` is not "peak RSS".** It is the high-water mark of `memory.current`, which
|
||
> charges **page cache** to the cgroup as well as anonymous memory. Proven on bumblebee: a
|
||
> container with `anon=0` that merely reads an 800 MB file reports `memory.peak=826 MiB`, of which
|
||
> `file=800 MiB`.
|
||
>
|
||
> This matters because the naive reading **inverts the decision**: page cache is *reclaimed* under a
|
||
> tighter cap, not OOM-killed, so a big peak that is mostly `file` is **not** evidence that the cap
|
||
> must stay high. `anon` is the part that actually forces an OOM. **Size caps on peak `anon`, not on
|
||
> `peak`.**
|
||
>
|
||
> Why a *sampler* and not just the end-of-job split: the kernel exposes `memory.peak` (peak of
|
||
> anon+cache) but has **no peak-anon counter**, and the end-of-job `anon` is the composition *then*,
|
||
> not at the peak instant — a job that peaks mid-`dotnet test` and then frees reports a misleadingly
|
||
> low `anon`. The 2 s background sampler catches the true peak-anon instant; `memory.peak` and the
|
||
> end-of-job split stay in the report as a cache-inflated ceiling and a reference. (Before #412 the
|
||
> instrument printed only `memory.peak` + the end-of-job split — see #411.)
|
||
|
||
**The compiler-server A/B (ersatztv#412).** Measured on bumblebee in the CI toolchain image,
|
||
swap-off (`--memory-swap == --memory`), full-solution `dotnet build --no-incremental`, peak anon
|
||
sampled by *this* instrument, servers shut down between arms (n=2 each, interleaved):
|
||
|
||
| Arm | shared-compilation env | peak anon (2 runs) | resident after build |
|
||
|---|---|---|---|
|
||
| **OFF** (the CI config) | disabled | **5818 / 5854 MiB** (~5.84 GiB, tight) | none |
|
||
| **ON** (dotnet defaults) | enabled | **6305 / 7604 MiB** (~7.0 GiB, noisy) | ~3 GiB `VBCSCompiler` |
|
||
|
||
Two things are solid: OFF is **consistently ~5.84 GiB** and ON is **always higher** (mean delta
|
||
~1.1 GiB, up to ~1.8 GiB), so disabling the servers is worth it; and ON leaves a ~3 GiB
|
||
`VBCSCompiler` **resident** after the build — the host-between-jobs cost #406 removed. Don't read a
|
||
*precise* delta into the ON peak: it is noisy because a parallel build's peak depends on how many
|
||
`csc`/project compilations overlap at the peak instant.
|
||
|
||
**#406's premise — *"if disabling shared compilation brings peak RSS well under 6 GiB, the whole
|
||
budget loosens"* — is NOT supported.** OFF sits at ~5.84 GiB for the *build phase alone* — right at
|
||
the 6 GiB line, not well under it — and the `test` job adds `dotnet test` + coverlet +
|
||
reportgenerator on top. Disabling the compiler servers stays right (consistent reduction, no 3 GiB
|
||
resident server) but **do not bank a looser cap budget on it**: size the cap off the **live
|
||
test-job peak anon this instrument now reports** (build + test + coverage), not off this build-only
|
||
A/B.
|
||
|
||
> The earlier PR #411 probe (`peak 9457 / anon 7134 / file 421 MiB`) read *higher* than these
|
||
> swap-off sampled numbers. Swap settings and read-method (end-of-job snapshot vs sampled peak) move
|
||
> these figures by >1 GiB (#406), so treat the committed instrument's sampled peak-anon as
|
||
> authoritative and that probe as superseded.
|
||
|
||
What *is* established: no persistent compiler server survives a build, `migrations` is green with
|
||
mysql capped at 2g swap-off, and the `test` job now self-reports a true peak-anon every run.
|
||
|
||
### `services:` containers are capped explicitly (ersatztv#406)
|
||
|
||
A runner's `container.options` (`--cpus=4 --memory=10g`) applies to the **job container only, not
|
||
to `services:`**. Verified on a live `migrations` job: the job container reported
|
||
`HostConfig.Memory=10737418240`; its `mysql:8.4` service reported `mem=0 nanocpus=0` — unbounded.
|
||
So `migrations` runs added an uncapped MySQL to an already-tight host.
|
||
|
||
The mysql service now sets `--memory=2g --memory-swap=2g --cpus=2`.
|
||
|
||
**`--memory-swap` is the part that matters, and it is easy to get wrong.** Docker defaults an unset
|
||
`--memory-swap` to **twice** `--memory`, so `--memory=2g` *alone* grants 2g RAM **plus 2g of swap**.
|
||
Verified on bumblebee:
|
||
|
||
| Options | `memory.max` | `memory.swap.max` |
|
||
|---|---|---|
|
||
| `--memory=2g` | 2147483648 | **2147483648** ← 2 GiB of swap |
|
||
| `--memory=2g --memory-swap=2g` | 2147483648 | **0** ← swap disabled |
|
||
|
||
Setting `--memory-swap` equal to `--memory` disables swap for the container. On this host that is
|
||
the whole point: swap thrash is what took prod down, and a swapping mysqld mid-DDL is precisely the
|
||
pathology behind the known `Command Timeout expired` migrations flake. **Prefer a loud OOM over
|
||
silent swapping** — an OOM is a clear signal to raise the cap; swapping just degrades everything.
|
||
|
||
⚠️ **The same 2× applies to the runners' `container.options: --memory=10g`** — each job slot is
|
||
really 10 GiB RAM *plus* 10 GiB swap. The "6 slots × 10 GiB = 60 GiB on a 25 GiB host" framing
|
||
understates the promise by 2×, and it is a plausible direct mechanism for the incident's 21 GiB of
|
||
swap. Fixing that is server-management#604's call (reported there).
|
||
|
||
**On the 2g figure, honestly:** a `mysql:8.4` container with this exact env peaked at **543 MiB**
|
||
during init and settled at 481 MiB idle (probed on bumblebee, 2026-07-17) — but that is init+idle,
|
||
**not** the 787-migration replay, which grows table/definition caches idle never touches. So 2g is a
|
||
measured *floor* plus headroom, not a measured ceiling; the `migrations` job going green is what
|
||
validates it. `--cpus=2` has no measurement behind it at all — 787 sequential DDL statements on one
|
||
connection are ~1-core-bound, so it is judgement; revisit if the apply step's tail latency grows.
|
||
|
||
**Any new `services:` container needs its own explicit cap** — it will not inherit one, and it needs
|
||
`--memory-swap` set alongside `--memory` or it silently gets 2× in swap.
|
||
|
||
### `test` job
|
||
|
||
`dotnet restore` → strip the Scanner project ref (`sed -i '/Scanner/d'`, matching the
|
||
Docker build) → `dotnet build -c Release` → `dotnet test -c Release --no-build`. Gates
|
||
the image build.
|
||
|
||
- **Code coverage** (ersatztv#15): `dotnet test` runs with `--collect:"XPlat Code Coverage"
|
||
--settings coverlet.runsettings --results-directory ./coverage`, so `coverlet.collector`
|
||
(referenced by every `*.Tests` project) emits a Cobertura report per project. A follow-up
|
||
**Coverage summary** step merges them with ReportGenerator (`TextSummary` to the log,
|
||
`MarkdownSummaryGithub` to the job step summary). No floor is enforced yet ("decide on a
|
||
floor later" — #15); the step is `continue-on-error: true`, so a missing report or a
|
||
transient tool install never blocks a build.
|
||
- **`coverlet.runsettings` excludes generated EF migration code** (`**/Migrations/*.cs`,
|
||
~2.59M generated lines vs ~200k authored). Instrumenting it OOM-killed the shared `test`
|
||
job (exit 137); excluding it cuts the instrumented surface ~126× (2.5M→20k coverable
|
||
lines in the whole-solution `Architecture.Tests` process) and makes the percentage reflect
|
||
authored code.
|
||
- **Shallow checkout**: `fetch-depth: 2` — this job never runs `git describe`/`git log`, and only
|
||
`build` needs full history/tags for version computation, so `test`, `migrations` and
|
||
`functional-e2e` all check out shallow. It was 1 (ersatztv#190) until ersatztv#420's revalidate
|
||
step needed `HEAD^2` to resolve on a main merge commit. `build`'s checkout stays `fetch-depth: 0`.
|
||
- **NuGet package cache**: both `test` and `migrations` cache `~/.nuget/packages` via
|
||
`actions/cache@v4`, keyed on `hashFiles('Directory.Packages.props', 'global.json')` with
|
||
a `restore-keys` OS-level fallback (ersatztv#190). Avoids a from-scratch `dotnet restore`
|
||
on every run; the key only changes when the central package manifest or SDK pin changes.
|
||
|
||
### `build` job
|
||
|
||
1. Compute `INFO_VERSION` (`git describe` + short sha on `main`; tag version on `v*`).
|
||
2. `docker/setup-buildx-action` with `buildkitd-config-inline` setting `http = true`
|
||
for `192.168.1.95:3000` — **BuildKit does not inherit the host daemon's
|
||
`insecure-registries`**, so without this, cache/base-image/push over the HTTP
|
||
registry fails (`http: server gave HTTP response to HTTPS client`).
|
||
3. `docker/login-action` with repo secrets `REGISTRY_USER` / `REGISTRY_PASSWORD`. **`build` is the
|
||
ONLY job in this workflow that names them** (ersatztv#885,
|
||
`ci.pr-route-carries-no-stored-credential`): it is gated `if: github.event_name != 'pull_request'`,
|
||
and every job that IS on the PR route now reads the registry and the status API anonymously,
|
||
because a `pull_request` run executes head-supplied YAML.
|
||
**`REGISTRY_PASSWORD` is a scoped PAT (`write:package` + `read:repository`), not an account
|
||
password** — deliberately, so head-resolved PR code cannot use it to forge a commit status
|
||
(`ci.actions-credential-scoping`, ersatztv#697). If a job ever fails with `token does not have at
|
||
least one of required scope(s)`, the fix is to narrow what the job does, **never** to widen the
|
||
token to `write:repository` or to put the admin password back. Note what the scope still reaches:
|
||
`write:package` covers `ersatztv:prod` (the tag prod's stack follows) and `ersatztv-ci:<sha>` (the
|
||
toolchain image five `container:` jobs execute), so this is the deployment supply chain, not an
|
||
inert endpoint — see `ci.actions-credential-scoping`.
|
||
4. `docker/build-push-action@v6`: amd64-only, `docker/Dockerfile`, `INFO_VERSION`
|
||
build-arg, registry layer cache (`type=registry,ref=…:buildcache`,
|
||
`cache-to … ignore-error=true`).
|
||
5. **Smoke + IPTV E2E test**: pull the just-pushed `:<sha>`, run it, poll for HTTP
|
||
readiness (`docker exec … python3` → `http://localhost:8409/`), then assert the real
|
||
Jellyfin-facing surfaces on the freshly built image (ersatztv#16): `/iptv/channels.m3u`
|
||
returns 2xx containing `#EXTM3U`, and `/iptv/xmltv.xml` returns 2xx containing a `<tv`
|
||
root. `xmltv.xml` needs `channels.xml` (written by the scheduler a few seconds after
|
||
boot), so each endpoint is polled with a deadline. Unique container name + `trap … EXIT`
|
||
cleanup; dumps container logs on failure. Catches routing / base-URL (#1) / migration
|
||
regressions that leave the app "up" but serving broken output.
|
||
|
||
### `functional-e2e` job (advisory; PR + main)
|
||
|
||
Boots the app **from source** and drives the manual live-E2E curl flows sessions have historically
|
||
re-run by hand, turning them into a CI regression net (ersatztv#299). It is the automatable half of
|
||
`docs/e2e-local.md`; every script it chains runs identically locally and in CI:
|
||
|
||
1. `npm ci` + `npm run build` (SPA), `dotnet build ErsatzTV.sln -c Release`. (ffmpeg comes from the
|
||
CI toolchain image; the old `apt-get install ffmpeg` step is gone — see below.)
|
||
2. `ETV_BUILD_CONFIG=Release scripts/e2e-local.sh <fresh-config>` — copies `wwwroot`, launches
|
||
`dotnet ErsatzTV.dll` in the background (logging to a file so the launch step returns once the app
|
||
is ready), prints `PID`/`CONFIG_DIR`.
|
||
3. `scripts/e2e-functional.sh http://localhost:8409 <config>` — asserts. Mostly curl-only: the
|
||
**legacy→SPA redirect sweep** (+ the `/api`, `/artwork` never-redirect exemption), the
|
||
**auth/CSRF/security-stamp** flow (setup-claim → read-gate 401/200 → re-claim 409 → CSRF 403 →
|
||
login 401/200 → logout 403/204 → post-logout stamp-revocation 401), the **library-scan status
|
||
contract** (404 unknown / 202 queued / `scan-status` 200), and the **If-Match/412** round-trip on
|
||
`rerun-collections`. Since ersatztv#363 (extended by #444) it also asserts three **lock-contention
|
||
409s** that aren't curl-only — see below. A `trap` kills the instance on step exit.
|
||
4. `ETV_UI_PORT=8410 scripts/e2e-ui.sh` — boots a second, **fresh** instance and runs the headless
|
||
Playwright UI flows (ersatztv#445). See "UI-E2E step" below.
|
||
|
||
**Lock-contention 409s (ersatztv#363).** The harness now seeds rows the API can't create — a
|
||
`LibraryPath` and a Jellyfin media-source — directly into the running instance's SQLite DB (via
|
||
python3's stdlib `sqlite3`, whose busy-timeout retry serializes behind the app's writer), and synthesizes media with the
|
||
image's ffmpeg, to exercise two `IEntityLocker` contracts **deterministically** (it only fires the
|
||
racing request once the lock is *provably* held, never a sleep-and-hope): (a) the **library-scan
|
||
"already scanning" 409** — seed ~60 tiny clips into the built-in Shows library so the scanner
|
||
subprocess runs a few seconds, poll `GET /libraries/scan-status` until the library shows active (that
|
||
window is a strict subset of the scan lock's held window), then a second `POST .../scan` is a 409
|
||
(deterministic bar a tiny residual TOCTOU gap the multi-second scan covers); (b) the
|
||
**external-collections "already scanning" 409** — the per-family lock is taken
|
||
synchronously *before* the 202, so the 202 proves it held, and pointing the seeded source at a
|
||
non-routable address keeps the background sync hung so the window stays open; and (c) the
|
||
**playout-build "build in progress" 409 + `isLocked` projection** (#215/#444) — a build is enqueued
|
||
onto the single-consumer `WorkerService` and the trigger returns *before* the handler locks, so poll
|
||
`GET /playouts/{id}` until `isLocked:true` (not the accepted trigger), then fire. Seed a Classic Flood
|
||
schedule over a few short episodes and crank `PlayoutDaysToBuild` (`playout.days_to_build`) so one
|
||
build is wide enough to observe (~5 days ≈ 43k items ≈ ~1s locally, wider on slower CI); assert `PUT
|
||
/playouts/{id}` 409, `POST .../playout/reset` 409, the list `isLocked:true`, then post-build the same
|
||
`PUT` 200s. No new CI step or dependency: ffmpeg ships in the toolchain image, and python3 was already
|
||
a harness dependency (`json_field`). The scan + build flows self-skip (advisory) if ffmpeg is ever
|
||
absent, and the build flow also self-skips if the build is never observed locked (never asserts an
|
||
unproven race).
|
||
|
||
**Advisory, by design** (the issue's "keep it a separate job so a functional-E2E flake can't block the
|
||
unit-test gate"): it is **not** a `needs:` of `build` and **not (yet) a required check**, so a flake
|
||
blocks nothing. Promote it to a required check / `build` dependency once it's proven reliable — the
|
||
same staged rollout the `migrations` job used. SQLite is the default provider, so unlike `migrations`
|
||
it needs **no** DB service container. Runs on PRs and on `main` (regression net); skipped for `v*` tag
|
||
builds. The playout-build lock 409 + `isLocked` projection (#215) landed in #444.
|
||
|
||
**UI-E2E step (ersatztv#445).** The last step of this same job (item 4 in the summary above; the
|
||
`Run UI-E2E Playwright flows (headless)` entry in the YAML) runs the headless-browser flows the curl
|
||
harness structurally cannot express — client-side form validation, `AuthGate`'s rendered states, the
|
||
session cookie authenticating the **SPA's own** `/api` XHRs, and sign-out via the `UserMenu`:
|
||
|
||
```
|
||
ETV_BUILD_CONFIG=Release ETV_UI_PORT=8410 scripts/e2e-ui.sh
|
||
```
|
||
|
||
`e2e-ui.sh` owns the lifecycle (fresh config dir → boot → `playwright test` → always kill the server)
|
||
and exits with Playwright's status. Three deliberate choices:
|
||
- **This job, not a new one.** The dominant cost here is `npm ci` + the Release build, both already
|
||
done; a separate job would duplicate them to add ~5s of browser work. The browser is baked into the
|
||
toolchain image, so the step installs nothing.
|
||
- **Its own fresh instance on port 8410.** The first spec asserts the one-shot **Setup** gate, which
|
||
step 3's auth section has already claimed on its config dir; a separate port also keeps this step
|
||
independent of step 3's teardown timing.
|
||
- **`retries: 0`, `serial`, single worker.** #445 asked for deterministic flows and a retry would let a
|
||
flaky flow merge looking green (measured: 4 consecutive clean runs, ~2s each). Full rationale and the
|
||
rule for extending the suite: `docs/e2e-local.md` → "UI-E2E harness".
|
||
|
||
### Docs-only skip (ersatztv#416)
|
||
|
||
A change that touches **only** `docs/**` or `*.md` (anywhere: `README.md`, `CLAUDE.md`, handoff
|
||
files) has nothing for the heavy jobs to validate. Before this, such a change ran the entire matrix
|
||
— `test`, `migrations` (with its `mysql:8.4` service), `functional-e2e`, `format`, `api-docs` —
|
||
~9 min of warm CI for a Markdown edit.
|
||
|
||
**The mechanism, and why it is shaped this way.** Each heavy job (`test`, `migrations`,
|
||
`functional-e2e`, `build`) runs `scripts/ci-detect-docs-only.sh` as its first post-checkout step
|
||
(`id: detect`), which emits `docs_only=true|false` to `$GITHUB_OUTPUT`. Every real step in the job
|
||
is gated `if: steps.detect.outputs.docs_only != 'true'`. On a docs-only change the job runs only
|
||
checkout + detect and **reports `success` in seconds**.
|
||
|
||
**Shallow-checkout safe (the change-set diff).** `test`, `migrations` and `functional-e2e` all check
|
||
out shallow (`fetch-depth: 2`), and
|
||
a shallow clone has **no `origin/<base>` tracking ref and no merge-base** — so a three-dot
|
||
`origin/main...HEAD` diff *errors*, the fail-safe returns `docs_only=false`, and the skip silently
|
||
never fires (this bug shipped once — every docs-only PR still ran the full matrix; caught by
|
||
#416's "verify on a real PR" box). The script therefore fetches the base and diffs against
|
||
**`FETCH_HEAD`** (always written by `git fetch`, resolves in a shallow clone) with a **two-dot** tree
|
||
diff (`git diff --no-renames FETCH_HEAD HEAD`) — no merge-base required. (`api-docs`/`format` avoided
|
||
the bug only because they check out `fetch-depth: 0`.)
|
||
|
||
The jobs are **not** `if:`-skipped. That is deliberate and it is the whole trap of this issue:
|
||
|
||
- `main`'s branch protection requires two checks **by name** — `Build ErsatzTV Image / Build & test
|
||
(.NET) (pull_request)` and `Build ErsatzTV Image / EF migration integrity (SQLite + MySql)
|
||
(pull_request)`. If a docs-only PR produced **no run** for those (a workflow-level `paths-ignore`,
|
||
or an `if:`-skipped job), those contexts would never report and the PR could **never merge** — the
|
||
naive fix *bricks* docs PRs rather than speeding them up.
|
||
- On Gitea **1.25.4** an `if:`-skipped job reports commit-status state **`skipped`**, a distinct
|
||
state (verified with a throwaway probe, PR #418) — not `success`. **Still true on 1.27.1**
|
||
(re-confirmed 2026-08-28, ersatztv#747, from production PR heads rather than a new probe PR). We do
|
||
**not** rely on how branch protection treats a `skipped` **required** context. Keeping the job running and gating its *steps*
|
||
makes the required context report `success` unconditionally, which is safe by construction.
|
||
- Non-required jobs may skip freely: production already proves a `skipped` **non-required** context
|
||
does not block merge (`build` is `skipped` on every PR). So `build` skips its image steps on a
|
||
**docs-only push to `main`** (docs are not in the image, so there is nothing to rebuild); tag
|
||
builds force `docs_only=false` in the script so a release is never skipped.
|
||
|
||
The detection **biases toward running more**: `docs_only=true` only when *every* changed path is
|
||
docs; any code path, a tag build, a non-merge push, or an undeterminable diff resolves to `false`
|
||
(run the full matrix). A false `true` would skip real tests on a code change — a correctness bug —
|
||
so every ambiguous case runs everything. The `migrations` job's `mysql` service still starts on a
|
||
docs-only run (a `services:` container starts with the job regardless of step `if:`), but the
|
||
expensive 787-migration replay is skipped; the service is capped and idle for seconds.
|
||
|
||
`api-docs` and `format` already short-circuit on docs-only changes via their own path detection (no
|
||
API path / no `.cs` changed → they pass in ~5s), so they needed no change. `docs-reminder`,
|
||
`decisions-guard`, `ci-image-pin` and `script-tests` keep running on docs-only changes — the first
|
||
two are *about* docs and must, and `script-tests` is unconditional by design (ersatztv#631).
|
||
|
||
Not in scope: the within-run triple `dotnet build` (ersatztv#398; measured and rejected as
|
||
build-once — see `docs/decisions.md`). The separate redundancy of running the **whole matrix on a
|
||
PR and again on the merge-to-`main`** over identical code (ersatztv#420) is addressed below.
|
||
|
||
### Cross-run tree-identity skip (ersatztv#420)
|
||
|
||
A merge to `main` re-runs the entire matrix over code the PR's last run already validated — the
|
||
same redundancy as docs-only, but for identical code rather than docs. On a `push` to `main` that
|
||
is a real merge commit, `test`/`migrations`/`functional-e2e` each run
|
||
`scripts/ci-detect-already-validated.sh` as a **second** detect step (`id: revalidate`, right after
|
||
the docs-only detect), and every heavy step gains an added `&& steps.revalidate.outputs.skip !=
|
||
'true'` to its existing `if:`.
|
||
|
||
**Skip condition — all four required, else fail-safe `skip=false`:**
|
||
|
||
- the event is a push to `refs/heads/main`;
|
||
- `HEAD` has a second parent `HEAD^2` (a real merge commit — the PR head CI already validated;
|
||
squash, rebase, fast-forward, or a direct push have no `HEAD^2`, so they run);
|
||
- `git rev-parse HEAD^{tree}` equals `HEAD^2^{tree}` — main did not advance since the PR's last run,
|
||
a byte-identical tree;
|
||
- `HEAD^2` has a **green Gitea combined commit status**, queried via the API **anonymously**
|
||
(ersatztv#885: the three jobs that call `scripts/ci-detect-already-validated.sh` also run on the
|
||
`pull_request` route, so they hold no stored secret; `timothy/ersatztv` is public and answers the
|
||
combined-status GET without a credential — measured 2026-09-04). Trusting the aggregate `.state`
|
||
is sound: a `skipped` context does **not** drag
|
||
the combined state below `success` (verified live against this instance — a real merge commit with
|
||
four `skipped` PR-only contexts still reported `.state == success`), and the two required jobs
|
||
never report `skipped` (they always run and report a real `success`/`failure`), so
|
||
`.state == success` implies they were green.
|
||
|
||
Those three jobs check out `fetch-depth: 2` so `HEAD^2` and its tree resolve.
|
||
|
||
**Why it's safe: `build` is not gated.** The three heavy jobs skip their *steps* (same
|
||
required-context reasoning as docs-only — they still run and report `success` in seconds), but
|
||
**`build` always runs on `main`, ungated**, building and pushing the image from that identical,
|
||
already-validated tree. No image ships from unvalidated source. The required contexts are
|
||
unchanged (`Build & test (.NET)`, `EF migration integrity (SQLite + MySql)`) — no branch-protection
|
||
change.
|
||
|
||
**Fail-safe bias.** Any uncertainty — not a main push, no `HEAD^2`, a differing tree, a
|
||
missing/failing/non-`success` status, missing auth — resolves to `skip=false` and runs the full
|
||
matrix. A false skip could ship an under-validated image, so every ambiguous case runs everything.
|
||
|
||
**Honest limitation — this fires rarely here, by design.** The tree is identical only on a
|
||
*fast-forward-equivalent* merge: main did not advance since the PR's last green run **and** the PR
|
||
head was not rebased at merge time. Two routine patterns defeat it in this repo: (1) under parallel
|
||
merges main usually advances; and (2) — the bigger one — the standard workflow **rebases a PR
|
||
before merging** to resolve the `docs/decisions.md` lifecycle conflict (see MEMORY: the
|
||
"decisions.md conflict treadmill"), which mints a new head SHA whose tree was never itself
|
||
CI-validated, so the tree-match check correctly declines. So the skip is a genuine but *occasional*
|
||
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 `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.
|
||
|
||
**The ban is re-checked on the release path itself (ersatztv#767).** It used to be enforced only by
|
||
the `script-tests` job, which lives in `pr-checks.yml` (`on: pull_request`) and is **not** a
|
||
required context — a *review-time* check on the PR that would introduce a delimiter, not a gate on
|
||
the release. `pr-checks.yml` does not run on a `v*` tag push at all, so a delimiter that ever
|
||
reached `main` would still drop `Smoke` on the tag build and go green; `main` being PR-only (#743)
|
||
meant such a change had to pass through a PR where `script-tests` reddens, but a red on a
|
||
non-required check does not block the merge server-side.
|
||
|
||
There is now a **`scan` job** (`Delimiter ban (release path)`) that runs the PyYAML-based ban test,
|
||
and **`build` lists it in `needs:`**. That single edge is the fail-closed property: a red `scan`
|
||
means `build` is skipped outright, so the image is never built, let alone pushed.
|
||
|
||
**Why a job and not a step inside `build`.** A step cannot protect the job it lives in. `build` is
|
||
what publishes, so a guard step there fails **open** if the runner drops it — and the defence
|
||
("the guard's own body has no opener, so it cannot be dropped") is circular when the only thing
|
||
enforcing that property is the same PR-only test being backstopped. That was the first design, and
|
||
it was rejected for exactly that.
|
||
|
||
**Why it runs the real pytest and not a bespoke scanner.** The same first cut hand-parsed the
|
||
workflow YAML in stdlib Python, to avoid provisioning PyYAML on `build`'s bare runner. That parser
|
||
yielded ~10 **false negatives** in one round — flow mappings (`{run: …}`), a quoted
|
||
`"run":` key, aliases, multiline quoted scalars — making it strictly *weaker* than the check it
|
||
backstopped, in the only direction that matters for a security gate. Running the existing test
|
||
needs no second definition of "what is a `run:` body", so it has no drift surface at all. `scan`
|
||
runs on `small` and provisions Python the same way `script-tests` does.
|
||
|
||
The wiring is held by `scripts/tests/test_ci_release_path_scan_job.py` — `build` depends on it, it
|
||
carries **no job-level `if:`** (one that excluded the tag push would restore the hole; one that
|
||
skipped the job would skip `build` too), no step is `continue-on-error`, and every one of its own
|
||
`run:` bodies is delimiter-free. Its steps also carry #756 markers and a trailing assert, so a drop
|
||
*inside this job* is caught as well.
|
||
|
||
The gate's guarantee is one **execution probe**, not a set of assertions about the command: it runs
|
||
the scan step's real `run:` body — with the workflow, job and step `env:` tiers layered as the
|
||
runner would — against a copy of the repo whose `Smoke` body carries an injected delimiter, and
|
||
requires a non-zero exit, with a clean-tree negative control. Shape assertions were tried twice and
|
||
lost twice (nine disarms across two rounds, from `echo`ing the command to `PYTEST_ADDOPTS` one env
|
||
tier up), so **do not replace the probe with cheaper checks about the command.** One tier is
|
||
unreachable by any static reconstruction — a step writing to `$GITHUB_ENV` injects into later steps
|
||
at runtime — and is therefore **banned** in this job rather than modelled.
|
||
|
||
**And one check deliberately is not a test at all.** The tests above prove the ban *holds*; they
|
||
cannot prove it would *notice*, because pytest's own configuration outranks them — a repo-root
|
||
`pytest.ini` with `addopts = -k "not delimiter_banned"`, or a `conftest.py` with
|
||
`pytest_collection_modifyitems`, deselects the ban test and every test guarding it, leaving the
|
||
whole suite green with a delimiter sitting in `Smoke`. So `scan`'s last working step runs
|
||
**`scripts/ci-prove-ban-detects.sh`**, which poisons the checked-out workflow, re-runs the ban
|
||
test, and requires **the ban test's `build` parametrisation to be the failure**. Not "the same
|
||
command" (the step also runs the wiring tests; the script runs only the ban file) and not merely
|
||
"fails if it passes": any other outcome — exit 5 because everything was deselected, exit 2 on a
|
||
collection error, or a failure in some unrelated test — is a **refusal**, not a pass. That
|
||
exhaustiveness is the load-bearing part, because each weaker reading was a live bug here:
|
||
"non-zero means it noticed" vouched for the gate on a collection error, and the deselection disarm this script
|
||
exists to catch exits **5**, not 1.
|
||
|
||
It runs in the **real checkout** — an isolated copy does not inherit the repo-root config a disarm
|
||
would live in, so a copy-based run reports healthy while the real invocation is deselected — which
|
||
has happened here. The workflow file is
|
||
restored by an `EXIT` trap, and the step carries a `selfcheck` marker so dropping it reddens the
|
||
job.
|
||
|
||
Two further tests exist because review disarmed the gate in ways an obvious test misses. **The
|
||
pytest invocation is checked as a command, not as a substring**: the ban test's path must appear as
|
||
a bare token on the real `python3 -m pytest` line, that line may carry none of
|
||
`-k`/`-m`/`--deselect`/`--ignore`/`--collect-only`, and its exit status may not be swallowed by
|
||
`||`/`&&`/`;`/`|`. Commenting the invocation out and echoing instead, or appending
|
||
`-k 'not delimiter_banned'`, each left the suite green with the release path unguarded. And **the
|
||
ban's SCOPE is pinned** — `DELIMITER_BAN_JOBS` must still contain `build`, because dropping that
|
||
one literal makes `scan` pass while checking nothing about the job that publishes the image.
|
||
|
||
Two operational notes. `scan` is a `small`-lane job on the tag path, which ersatztv#535 moved away
|
||
from after such jobs wedged in act's setup phase on the v26.12.0 tag; that is safe again only
|
||
because server-management#639 took `build` off the lane. If it ever returns, move `scan` to
|
||
`ubuntu-latest` — never weaken the `needs:` edge. And `scan` installs from PyPI, so a network blip
|
||
now sits between a `v*` tag and its image; fail-closed and loud, but real.
|
||
|
||
What this does **not** claim: that no step can ever fail to run for a reason other than the
|
||
interpolation drop. It moves the terminal assumption — to fail open you must now drop the pytest
|
||
step **and** the assert step, rather than either one alone.
|
||
|
||
Measuring a guard on this path does **not** require cutting a release: `build` runs on every
|
||
push to `main` (`if: github.event_name != 'pull_request'`), and a `workflow_dispatch` on any
|
||
other ref runs the
|
||
job while `Build and push` publishes nothing (its `push:` is gated on `main`/`v*`). That is how
|
||
#767 was verified — see the decision record for the run ids.
|
||
|
||
`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 <key>` 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. (`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
|
||
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. 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
|
||
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
|
||
the workflow, so an unmarked step or a bucket mismatch is a red — it does not rely on anyone
|
||
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
|
||
`/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 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 what rules out a
|
||
rerun inheriting attempt 1's markers. 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
|
||
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.
|
||
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 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.
|
||
- 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`.
|
||
|
||
### `docs-reminder` job (non-blocking, PR-only — in `pr-checks.yml`)
|
||
|
||
Two lightweight nudges, both `::warning::`-only. Neither can fail the build — they are reminders,
|
||
not gates; prose-doc gates get gamed with token edits.
|
||
|
||
**1. The parity-doc reminder** 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
|
||
(`web/src/screens/*.tsx`) or `ErsatzTV/LegacyUiRedirects.cs` but **does not** update
|
||
`docs/blazor-route-parity.md`. It diffs the PR against its base branch and emits a
|
||
`::warning::` annotation.
|
||
|
||
**2. The session-narrative reminder** (ersatztv#784) runs `scripts/check-doc-narrative.py --diff`
|
||
over the lines this PR **adds** to `docs/**/*.md` (minus `docs/decisions/**`, exempt wholesale),
|
||
root-level `*.md`, and — since ersatztv#876 — every tracked file under `.claude/`, `.gitea/`, `.husky/`
|
||
and `scripts/` regardless of extension (minus the detector and its own test, whose fixtures are the
|
||
phrasings), flagging text that narrates the artifact's own revision history —
|
||
`docs.no-session-narrative`. It is advisory **by design and permanently**: a narrative detector is a
|
||
string predicate over prose, the class `docs/defect-shapes-773.md` §4 argues must never be
|
||
load-bearing, so the script exits 0 on every path including a bad argument or an unresolvable base
|
||
ref. It does NOT rest on prose alone, and it does not rest wholly on tests either: the record's
|
||
COVERAGE BOUNDARY names which clauses carry a mutation proof and which are defensive and unproven
|
||
(the unhandled-exception arm among them). Every argument shape is asserted per shape in
|
||
`scripts/tests/test_check_doc_narrative.py` rather than only in prose. When it cannot resolve the
|
||
base it prints `SCANNED NOTHING` instead of a clean-looking line, because a silent zero-file scan is
|
||
indistinguishable from a clean one — the same failure `ci.required-job-step-execution-markers`
|
||
exists for.
|
||
|
||
Because this step needs an interpreter, the job DOES carry `actions/setup-python` — the one
|
||
exception to the no-setup-actions note below, since `python3` is not guaranteed on the bare `small`
|
||
lane. It, the script step and the parity-doc step above it all carry `continue-on-error: true`: a script
|
||
that exits 0 does not by itself keep the JOB green, a setup-action download failure would redden an
|
||
advisory check just as effectively as a hit would, and the parity step's base-ref fetch fails loudly
|
||
(ersatztv#746) — which must reach the log without reddening a warn-only job. `Checkout` is the one
|
||
step left blocking.
|
||
|
||
The job deliberately has **no** `setup-dotnet`/`setup-node` (and
|
||
thus no `actions/cache`), so it can't hit the cache-save hangs seen on the VM-127 runner
|
||
(server-management#570). It does not cover the remaining doc obligations in the CLAUDE.md table
|
||
(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 — 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`,
|
||
reciprocal `supersedes`/`superseded-by` links, no record vanishing from the active set without an
|
||
archive copy, no rationale-prose rewrite without a `Decisions-Edit: yes` trailer on a non-merge commit
|
||
in the range (ersatztv#609), a structural per-path check that every `*.md` under
|
||
`docs/decisions/records/**` and `docs/decisions/archive/**` parses to **exactly one keyed
|
||
record** (ersatztv#621 — without it, a file the dependency-free frontmatter reader cannot parse,
|
||
such as one using a YAML block scalar, yields `[]` and vanishes from the corpus with every check
|
||
still reporting green; a file directly in `archive/` is exempt only when it really is a stripped index — one keyless
|
||
record with a known generated heading — never merely by its location; the single further exemption,
|
||
`archive/README.md`, is by exact relative path, never by basename, which would otherwise exempt the
|
||
same filename in the active wing),
|
||
and the generated active catalog (`docs/decisions/README.md`) in sync with source. Two steps:
|
||
`scripts/decisions_validate.py --base origin/<base> --head HEAD` (the merge-base diff checks, which
|
||
need a base/head range — CI-only) and `scripts/build_decisions_catalog.py --check` (catalog drift).
|
||
The **same validator** backs the Husky `pre-commit` hook (`.claude/hooks/decisions-guard.sh`, no
|
||
base/head there — structural checks only, over the working tree), so local and CI enforcement can't
|
||
drift on the rules that don't need a range. `python3` isn't guaranteed on the bare `small` lane, so
|
||
the job adds `actions/setup-python@v5` before invoking it; that install is lightweight (no
|
||
compiler/docker build), so it doesn't violate the "small is git-only" lane rule. Like
|
||
`docs-reminder`, otherwise a seconds-long `git diff` + parse with no dotnet/node setup
|
||
(`runs-on: small`).
|
||
|
||
### `prove-fix` job (`Fix proofs (Proves trailers)`, PR-only — in `pr-checks.yml`)
|
||
|
||
Runs `scripts/prove-fix.sh` for **every commit in the PR that carries a `Proves: <pytest selector>`
|
||
trailer**, and fails the PR if a claimed proof does not hold. The rule and its rationale are
|
||
`testing.fix-ships-a-witnessed-red-test`; this section is the CI-side contract.
|
||
|
||
**Opt-in by trailer, enforced when present.** Requiring `Proves:` on every commit would block docs,
|
||
CI and refactor commits that have no code side to revert, and a gate that blocks ordinary work gets
|
||
switched off — which is how a check ends up running nowhere (ersatztv#631). So the trailer is the
|
||
author's *claim* and this job checks claims. **When a PR carries none, the job emits a `::notice::`
|
||
saying it asserted nothing** — its green must not be read as fix coverage.
|
||
|
||
**It needs full history** (`fetch-depth: 0`): the script reverts each commit against its PARENT, and
|
||
a shallow clone cannot resolve `<sha>^`.
|
||
|
||
Three refusals worth knowing before you write a trailer:
|
||
- a **merge** commit is rejected — several parents means "the code before this change" is ambiguous;
|
||
put the trailer on the commit carrying the fix;
|
||
- **more than one** `Proves:` trailer is rejected — only the first would be checked, so the rest
|
||
would read as proven without ever running (the job checks this itself, because passing the
|
||
selector explicitly bypasses the script's own guard);
|
||
- a **test-only** commit is rejected — there is no code side to revert.
|
||
|
||
**Only pytest exit 1 counts as red.** 2 is an interrupted collection, 3 internal, 4 usage, 5 nothing
|
||
collected, 143 a signal; `--continue-on-collection-errors` converts a genuine collection failure to
|
||
1 so add-a-file fixes stay provable. The job inherits that. Note the direction: a wobble DOES redden
|
||
this job (`prove-fix.sh` exits 5 on a harness/git failure or a signal, and the job turns that into an
|
||
error), so what the exit-code discipline buys is the other way round — a **green** here means a claim
|
||
was witnessed, never that a run was cancelled or broke.
|
||
|
||
### `script-tests` job (`Script lint and tests (ruff + pytest)`, PR-only — in `pr-checks.yml`)
|
||
|
||
> Reddens the run on failure, but like the other `pr-checks.yml` gates it is **not** one of the
|
||
> three required status checks on `main` (`Build & test (.NET)`, `EF migration integrity`,
|
||
> `review-verdict/h10`). Promoting it to required is a branch-protection change, tracked separately.
|
||
|
||
Runs the repository's Python test suite: `PYTHONPATH=. python3 -m pytest scripts/tests -q`
|
||
(773 tests at `706674272`, ~4.5 min; the suite grows fast — it was ~190 tests / ~10s when this job landed in #631 — so treat the figure as a dated snapshot, not a budget). It covers the decision-corpus parser/validator/catalog builder, the ersatztv#610
|
||
migration-equivalence harness, the merge-consent exemption logic and the ersatztv#622 review-verdict
|
||
poster.
|
||
|
||
**Until ersatztv#631, nothing ran these tests.** No workflow and no Husky hook invoked `pytest`.
|
||
`decisions-guard` executes `decisions_validate.py` and `build_decisions_catalog.py` directly — it
|
||
exercises that *code* but never its *tests* — and the `test` job is `dotnet test` only. The suite
|
||
guarding our merge-gating machinery was therefore local-only, and a test added "for CI enforcement"
|
||
was decorative.
|
||
|
||
**Why it is its own job, not a step inside `decisions-guard`.** `decisions-guard` is covered by the
|
||
standing `ci.decisions-lifecycle-flake` rule: a lone `decisions lifecycle` red is a known infra
|
||
flake and sessions are instructed *not to investigate it*. Adding the suite there would make a
|
||
genuine pytest regression surface as precisely the red everyone is told to wave through — the same
|
||
"reports success while doing nothing" failure mode ersatztv#631 exists to close. A distinct job
|
||
name keeps a real failure unambiguous.
|
||
|
||
**Why it runs unconditionally** rather than behind a `scripts/**` path filter: the suite's true
|
||
input set spans AT LEAST three top-level directories. `test_post_review_verdict.py` and
|
||
`test_merge_consent_exemption.py` execute the real `scripts/post-review-verdict.sh` and
|
||
`.claude/hooks/pretooluse-merge-consent.sh`, and since ersatztv#845 the former also READS
|
||
`.gitea/workflows/review-verdict.yml`, because the writer derives the H10 allow-list from it and
|
||
editing that literal changes the suite's outcome. So a `scripts/**` filter would silently miss a
|
||
`.claude/hooks/**` or `.gitea/workflows/**` edit. The hedge is deliberate: the corpus tests read
|
||
`docs/` too, so the real figure is higher, and an exact count here is the kind that goes stale.
|
||
The reason is the input set, not the cost: the suite was ~10s when that was decided and is now
|
||
minutes, and it would still be wrong to filter on `scripts/**`.
|
||
|
||
**It also lints (ersatztv#780).** Early in the job it installs a **pinned** `ruff==0.12.11` and runs
|
||
`ruff check` and `ruff format --check` against the repo-root `ruff.toml`. Five things are deliberate:
|
||
|
||
- The config is **committed**. Without it ruff falls back to whatever `~/.config/ruff/ruff.toml` the
|
||
operator's machine has, so a second machine lints this repo differently or not at all.
|
||
- The version is **pinned** — an unpinned install makes the verdict a function of when the job ran,
|
||
the same divergence one layer up, and the same argument as the `jq` pin below. `pytest`/`pyyaml`
|
||
stay unpinned on purpose: a pytest release does not add assertions to your suite, a ruff release
|
||
adds rules to your lint.
|
||
- Lint runs **before the jq preflight**, and after the `git` one. `Preflight jq version` is a hard
|
||
`--expect` tripwire; a lint sitting behind it goes dark for as long as the jq contract is broken,
|
||
under a red that says "jq". `Preflight external tools` stays ahead, because the lint steps consume
|
||
`git` — without it a missing git reaches them as an empty population and they blame the glob.
|
||
- Neither step is `ruff check .`. Both pass an **explicit population** from
|
||
`git ls-files -z '*.py' '*.pyi' '*.ipynb'` with `--no-force-exclude`, and fail if that list is
|
||
empty. Discovery-based invocation is silently emptied by an `exclude` in the right config scope —
|
||
top level empties both commands, `[lint]` empties `check`, `[format]` empties `format --check`
|
||
(and `[format]` is where an appended line lands) — and
|
||
`ruff check .` over zero files exits **0** with only a stderr warning, so the failure mode is a
|
||
green gate. The measured matrix is in `ci.python-lint-ruff-config-committed`.
|
||
- `RUF100` is selected, so a `# noqa` that no longer matches anything is itself a finding.
|
||
|
||
`pyright` is not gated; the reasoning and the exemption list are in
|
||
`ci.python-lint-ruff-config-committed`.
|
||
|
||
**Dependencies: `pytest` and `pyyaml`** — the complete third-party set across `scripts/`, established
|
||
by an AST import scan rather than by reading the files that looked relevant. PyYAML does **not**
|
||
contradict the dependency-free decisions *read* path: `decisions_lib._read_frontmatter` is
|
||
hand-written exactly so validation runs where nothing is installed, but the one-shot *write* path
|
||
`migrate_decisions_split.py` uses PyYAML by design, and `test_migration_equivalence.py` imports that
|
||
module. (The first cut of this job claimed "pure stdlib + pytest", passed locally on a machine that
|
||
happened to have PyYAML installed, and went red in CI on a `ModuleNotFoundError` at collection —
|
||
which is itself a small demonstration of why the suite needed to run in CI at all.) Like the other
|
||
`small`-lane Python jobs it adds `actions/setup-python@v5` first. Checkout is at default depth: every `git` call in the suite runs
|
||
against a temp repo it creates itself, never this repository's history.
|
||
|
||
Two **preflight steps** run before the suite. The first asserts `git` is on PATH; the second runs
|
||
`scripts/jq-preflight.sh --expect 1.6`, which checks jq's **version**, not merely its presence (see
|
||
"The jq contract" below). Those two tests exec the real shell scripts, which shell out to `jq` ~26
|
||
times; the tests shim `curl` on PATH but not `jq`, so a runner image without it would surface as ~20
|
||
opaque assertion failures instead of one diagnosis. Both deliberately **check** rather than install —
|
||
ersatztv#390 removed run-time `apt-get` from CI; the fix for a genuine miss is to bake the tool into
|
||
the runner image.
|
||
|
||
### The jq contract (ersatztv#648)
|
||
|
||
> Full rationale: `docs/decisions/records/ci/jq-version-contract.md`.
|
||
|
||
Every shell gate in this repo — `decisions-guard`, `script-tests`'s own harness,
|
||
`pretooluse-merge-consent.sh`, `review-verdict.yml`, `scripts/pr-changed-files.sh` — is authored and
|
||
tested on a developer Mac shipping **jq 1.8.x**. The CI runner ships **jq 1.6**. Author to the
|
||
1.6-compatible subset; three concrete constructs diverge between the two and each one produced a real
|
||
bug when it hit CI for the first time:
|
||
|
||
- **`jq -e` over EMPTY input.** Exits 4 on jq >= 1.7, but **0** on jq 1.6. A guard that infers
|
||
"transport failure" from that exit status silently passes an empty/failed page on 1.6.
|
||
- **`` contains("\u0000") `` (or any NUL literal).** The NUL escape truncates to `""` on jq 1.6, so
|
||
the containment test is vacuously true for **every** string, not just ones containing a NUL. Use
|
||
`explode | index(0)` instead — it is version-stable.
|
||
- **Parse-error exit code.** `jq empty` exits 5 on jq >= 1.7 but **4** on jq 1.6 — the same code 1.6
|
||
uses for "no output produced". Reading that exit code as a specific failure mode conflates garbage
|
||
input with an empty-but-valid response.
|
||
|
||
`scripts/jq-preflight.sh` makes the running version **observable** in every gate job's log (it prints
|
||
the parsed version and asserts a floor of 1.6) so a future divergence can be diagnosed from the log
|
||
alone instead of guessing at the runner image.
|
||
|
||
**Pin vs floor is deliberately asymmetric.** `scripts/jq-preflight.sh --expect 1.6` additionally pins
|
||
the version and fails loudly if it drifts, but that mode is used **only** by `script-tests`
|
||
(`.gitea/workflows/pr-checks.yml`) — advisory, not a required check. `review-verdict.yml` runs the
|
||
no-args floor-only mode and never pins, because that workflow writes `review-verdict/h10`, the
|
||
branch-protection-**required** status check on `main`: a hard pin there would mean the day the
|
||
runner's jq version changes (a base-image bump, a host reimage — nothing this repo controls), every
|
||
PR on `main` stops merging until someone notices and re-pins. A required merge gate cannot fail
|
||
because an upstream package manager did its job. The narrower pin on `script-tests` exists precisely
|
||
because that job is the suite's only 1.6 coverage — if the runner's jq silently changed, that coverage
|
||
would evaporate with no signal, so failing loudly there forces a human decision instead.
|
||
|
||
Baking a pinned jq into `docker/ci/Dockerfile` was considered and rejected: `review-verdict.yml` is
|
||
`runs-on: small` with no toolchain-image pin, and per `ci.small-lane-git-only` the small lane is
|
||
git-only, so it gets the **host's** jq regardless of what the toolchain image contains — a pin in the
|
||
image provably cannot reach the gate that broke. This was checked against the running binary, not
|
||
assumed.
|
||
|
||
## PR gates workflow
|
||
|
||
**File:** `.gitea/workflows/pr-checks.yml` — `on: pull_request` only.
|
||
|
||
The five PR gates — `ci-image-pin`, `docs-reminder`, `decisions-guard`, `script-tests`, and `prove-fix` (the one member that is NOT merely checkout + `git diff`: it installs pytest and creates worktrees)
|
||
(all 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 `Build & test (.NET)`, `EF migration integrity`
|
||
and `review-verdict/h10` (next section) — 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`.
|
||
|
||
## Review-verdict gate (`review-verdict/h10`, required — `.gitea/workflows/review-verdict.yml`)
|
||
|
||
**A required status check named `review-verdict/h10`, written per-sha, is what actually stops an
|
||
unreviewed commit from merging** (ersatztv#622). It is not produced by a job's success/failure; it
|
||
is a commit status that `scripts/post-review-verdict.sh` POSTs onto one specific sha.
|
||
|
||
**`main` is PR-only AND admin-override-proof, and it takes both to make the check load-bearing**
|
||
(ersatztv#743, `release.main-direct-push-disabled`). Gitea evaluates `status_check_contexts` when it
|
||
**merges a PR** — a direct `git push origin HEAD:main` never consults them. So until 2026-08-05 the
|
||
entire gate was skippable with no forgery at all, which was cheaper than every route enumerated in
|
||
#697. `main` now carries **two** fields, and citing either alone is a mistake:
|
||
|
||
- `enable_push: false` — a direct push is refused server-side at pre-receive (`Not allowed to push to
|
||
protected branch main`), for every account including a site admin. The contents API is refused too
|
||
— measured, HTTP 403 `user cannot commit to repo`. The web editor, upload, apply-patch, revert and
|
||
cherry-pick paths share that same `CanUserPush` predicate and are therefore expected to refuse as
|
||
well, but were not probed (source-attested only).
|
||
- `block_admin_merge_override: true` — without it (the default is `false`), a repo admin could
|
||
`POST /pulls/{n}/merge` with `force_merge: true` and merge straight past a missing or red
|
||
`review-verdict/h10`. Disabling push alone just moves the bypass from the push path to the merge
|
||
path, since `timothy` is admin and is the identity every session already uses. **Measured
|
||
2026-08-28 (ersatztv#747)**, replacing the earlier source-attested-only note. Two throwaway scratch
|
||
bases, each with its own rule, head and PR — differing by construction, not by mutating one rule in
|
||
place: on `scratch/747a-base` with the field `false`, an admin `force_merge` past an ABSENT required
|
||
context MERGED (HTTP 200); on `scratch/747b-base`, whose rule was identical but for the field set
|
||
`true`, the same call was refused (HTTP 405 `Not all required status checks successful`). The pair is
|
||
the evidence — arm B's 405 has no attribution control of its own. It never required merging an
|
||
unreviewed PR into `main`: a scratch BASE branch carries its own rule, so the experiment is
|
||
disposable. Full arm table, including the ordinary-merge arm and its control:
|
||
`release.main-direct-push-disabled`. `enable_bypass_allowlist` is **not** a
|
||
substitute in the one configuration tested: enabled with an EMPTY username list (and
|
||
`block_admin_merge_override: false`) the admin force-merge still returned HTTP 200. A non-empty
|
||
list was not tested.
|
||
|
||
**Operator recovery when a required context gets stuck.** `block_admin_merge_override: true` removes
|
||
the "Merge (admin)" / `force_merge: true` escape that used to unstick a PR whose required context was
|
||
absent or wrongly red — a recurring situation here (a killed run overwriting a newer green, an
|
||
advisory red counted into the combined status, a gate workflow that cannot post). That escape is gone
|
||
*by design*: it was also the bypass. The supported recovery is to fix the status
|
||
(re-run the job, or re-post the verdict with `scripts/post-review-verdict.sh`); the last resort is to
|
||
`PATCH .../branch_protections/main` setting `block_admin_merge_override: false`, merge, and set it
|
||
straight back. Do the last one deliberately and say so in the PR — it is the one action that
|
||
re-opens the hole this section exists to close.
|
||
|
||
Practical consequences: **every** change to `main` goes through a PR, including a one-line docs fix;
|
||
and the client-side Husky guards (H6/H11/H13) remain useful friction but were never the control —
|
||
they are fail-open and `--no-verify` bypasses them. Tag pushes go through a separate mechanism and
|
||
are meant to stay unaffected for the release operator: since ersatztv#885 `tag_protections` carries
|
||
one rule, `v*` whitelisted to `timothy` (`release.tag-protection-v-star`), which is intended to leave
|
||
the release cut in "Cutting a release" working unchanged while refusing a `v*` tag from the `renovate`
|
||
bot — a push that would otherwise have published a `:prod` image. Both halves of that are EXPECTED,
|
||
UNVERIFIED: only the `timothy` credential exists here, so neither a real release cut nor a refused bot
|
||
push has been exercised. `release.tag-protection-v-star` carries the same caveat and waits on the
|
||
next cut.
|
||
|
||
**A fourth arm: guard-scope freshness (ersatztv#787).** `scripts/tests/test_ci_dropped_step_guard.py`
|
||
decides which CI jobs must carry per-step execution markers by DERIVING that scope from
|
||
`.gitea/required-status-contexts.json`, the committed mirror of `main`'s required status checks. It
|
||
derives rather than repeating a literal because its own CI job, `pr-checks.yml::script-tests`, checks
|
||
out with `persist-credentials: false` and cannot ask Gitea — which leaves the snapshot as the one
|
||
hand-maintained input in the chain. So the merge hook reconciles it: it reads
|
||
`branch_protections`, hands the payload to `scripts/check-required-contexts.sh`, and **asks** (never
|
||
denies) when the live list no longer matches. The comparison is against the EFFECTIVE required set:
|
||
a rule with `enable_status_check: false` requires nothing whatever its `status_check_contexts`
|
||
still holds, so reading the list without the flag would certify disabled protection as current.
|
||
|
||
Two consequences worth knowing before you meet them. **The immediate merge path now reads a
|
||
repo-admin-scoped endpoint**: before #787 `branch_protections` was fetched only when arming a
|
||
scheduled auto-merge, so a credential without repo-admin scope kept its auto-grant on the common
|
||
path and now gets a prompt there instead. That is the fail-closed direction and it is loud, but it
|
||
is a real change in what the gate needs. **The arm runs only for the repo the snapshot names**
|
||
(its `repo` field): the merge tool is called with whatever owner/repo is being merged, and judging
|
||
another repo by this one's mirror would report a confident finding about a repo the mirror does not
|
||
describe.
|
||
|
||
And what it does not reach: a docs/process-only PR exits at the hook's docs-only passthrough before
|
||
this arm, so a PR editing only `.gitea/required-status-contexts.json` — the snapshot-narrowing
|
||
direction — never triggers it. That passthrough is a human prompt rather than an auto-grant, and the
|
||
arm could not have judged such a PR anyway, since it compares the live server against the snapshot in
|
||
the local checkout rather than the one the PR proposes. Ask rather than deny because drift does not make that
|
||
merge unsafe — Gitea enforces the live required set server-side — while a stale guard scope means a
|
||
required context may have no dropped-step guard at all. The arm runs **last**, so it can only
|
||
downgrade an otherwise-satisfied auto-grant and never masks a more serious refusal, and it runs on
|
||
**both** merge paths: the pre-existing branch-protection read sits inside the
|
||
`merge_when_checks_succeed` branch and never fires on an immediate merge. Rule selection is shared,
|
||
not copied — both the hook and the checker load `scripts/lib/branch-rule-classifier.jq`.
|
||
|
||
**The hole it closes.** `pretooluse-merge-consent.sh` proves its three consent conditions at the
|
||
moment the merge tool is called. Pass `merge_when_checks_succeed=true` and Gitea performs the merge
|
||
*later*, against whatever head is green then — while the Done-when and review-verdict checks were
|
||
proven against the head at **scheduling** time. Every commit pushed in between merges unreviewed.
|
||
|
||
This was demonstrated as a controlled A/B rather than inferred (`ci/fake` stands in for a slow CI
|
||
check so Gitea waits, as it really does): review head A → post its verdict → schedule auto-merge →
|
||
push an unreviewed commit B → CI greens on B. **Without** the required verdict context, B merged.
|
||
**With** it, the same sequence was refused, and merged only once B itself was reviewed.
|
||
|
||
Note the motivating anecdote in ersatztv#622 — "PR #619 merged 263 insertions with no verdict" —
|
||
is **wrong**: #619 does carry `Review-verdict: MERGEABLE @ 02c82b35`, posted six seconds before the
|
||
merge, explicitly re-reviewing the follow-up commits. It was filed from an API read that lagged.
|
||
The gap is real anyway, and structural: nothing *forced* that re-review inside the 45-minute window
|
||
where Gitea would have merged whatever went green. This turns diligence into construction.
|
||
|
||
**Why a commit status fixes it and a smarter hook cannot.** A status belongs to exactly one sha, so
|
||
a new commit *cannot inherit it*: the required context is simply absent on the new head, Gitea's
|
||
merge-requirement check reads that as not-passing, and the scheduled auto-merge refuses to fire.
|
||
The invariant self-invalidates — nothing has to notice the push. It also covers merge paths the
|
||
hook never sees (Gitea UI, raw API, another agent's session).
|
||
|
||
**Posting a verdict.** After reviewing a PR's *current* head:
|
||
|
||
```bash
|
||
ETV_GITEA_BASICAUTH=user:pass scripts/post-review-verdict.sh <pr> MERGEABLE [note...]
|
||
```
|
||
|
||
That posts both the `Review-verdict: … @ <sha>` comment (the human-readable artifact, and the
|
||
hook's condition (c)) and the `review-verdict/h10` status on the same sha. `BLOCKED` /
|
||
`NOT-MERGEABLE` post a `failure` status instead. The script re-reads the head after commenting: if
|
||
a commit landed mid-flight it writes **no** status and exits non-zero rather than retargeting your
|
||
verdict at a commit you never read.
|
||
|
||
**The verdict words are declared once** in `scripts/lib/review-verdict-vocabulary.sh`, and both
|
||
sides derive from it: `post-review-verdict.sh` classifies through `etv_verdict_class`, and
|
||
`check-review-verdict.sh` builds its `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Adding a word
|
||
is a one-line edit to that file and it reaches both sides by construction (ersatztv#788,
|
||
`release.verdict-vocabulary-shared`). Only the WORD SET is shared — the grammar the classifier
|
||
applies around it (the column-0 anchor, the whole-word boundary, the anchored `@ <sha>` field) stays
|
||
in `check-review-verdict.sh`, where every #629 false-open actually lived. Run
|
||
`scripts/post-review-verdict.sh` with no arguments to print the live vocabulary.
|
||
|
||
Two things follow that are easy to get wrong. **Do not reintroduce a parity test** that compares the
|
||
two scripts' sources: #774 shipped one and withdrew it after six review rounds, each finding another
|
||
shell construction that escaped the extractor or reddened it on a correct tree. And a vocabulary
|
||
that cannot be loaded or does not validate yields **no words on either side** — the reader exits 2
|
||
(which callers treat as an unreadable input and fail closed on), the writer exits 1 having posted
|
||
nothing. Words are interpolated into a regex, so a `.*` in the positive list would classify an
|
||
explicit `BLOCKED` as `positive`; validation restricts every word to `[a-z][a-z-]*`.
|
||
|
||
**What enforces that is a data dependency, not the `if` around the validator** — worth knowing
|
||
before editing either script, because the obvious reading is wrong and the first version of #788
|
||
shipped the hole. Under `set -u`, an unbound-variable error inside a function aborts the function
|
||
but not the script, and in an `if ! validate` form neither branch runs, so the `exit 2` is skipped.
|
||
The reader has no `set -e`, so unvalidated words reached the regex and an explicit `BLOCKED @ head`
|
||
classified `positive`, exit 0. Validation now sets a sentinel on its last line and the derived views
|
||
refuse without it. Likewise a top-level `exit` in a sourced library terminates the sourcing script
|
||
where it stands, so both consumers probe-source the library in a subshell and require an
|
||
end-of-file marker before trusting it.
|
||
|
||
The status description also records the base branch — `Review-verdict: MERGEABLE @ abc1234 (base:
|
||
main)` — and the merge-consent hook denies when that no longer matches the PR's live `base.ref`
|
||
(ersatztv#632). Retargeting a PR changes the effective diff without moving the head sha, so the
|
||
per-sha binding alone cannot see it. This is **detection on the hook path only**: a commit status
|
||
carries no base of its own, so a merge driven through the Gitea UI or API is unaffected. The
|
||
comparator is the base *branch*, never its tip sha — a base that merely advances is ordinary churn,
|
||
and comparing tips would invalidate every open verdict on every unrelated merge to `main`.
|
||
|
||
**Exemptions** are handled by `review-verdict.yml` on every `pull_request_target` event, which posts the
|
||
status as `success` for **Renovate-authored** PRs (it uses `platformAutomerge: true`, so a required
|
||
verdict with no exemption would stall every dependency bump) and for **docs-only** PRs, and as
|
||
`pending` for everything else so the block has a visible reason. Both exemptions are **void when the
|
||
PR touches `.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`** — a PR that can weaken the
|
||
gate must not be able to exempt itself from the gate. That includes Renovate's `docker/ci` base
|
||
bumps, which already need the manual publish-then-pin two-step anyway.
|
||
|
||
The Renovate exemption additionally requires **every** changed path to be a dependency manifest —
|
||
`Directory.Packages.props` or `.config/dotnet-tools.json`, and only those (ersatztv#698). The npm
|
||
manifests are deliberately excluded: `renovate.json` enables only `nuget`/`github-actions`/`dockerfile`,
|
||
so npm is unmanaged here, while `package.json` `scripts` are executed by CI (`npm ci`, `npm run build`)
|
||
— exempting it would put a code-execution path inside the allow-list for no benefit. An author match alone is not enough, because `pull_request.user.login` is the PR's
|
||
*immutable creator* while its head is not: pushing application code onto an open Renovate branch
|
||
leaves the PR still "authored by renovate" and, previously, still exempt. A Renovate PR touching
|
||
anything else — a `.csproj`, a source file — is not blocked, it just needs a real verdict. **If a
|
||
dependency PR is unexpectedly asking for a verdict, this is why**; the status description says so.
|
||
|
||
The two exemptions are evaluated as **independent predicates**, never as an `elif` chain: a Renovate
|
||
PR touching only `docs/` still gets the docs-only exemption on its own merits.
|
||
|
||
An existing `review-verdict/h10` **`success`** on the head is **only** left alone when it is positively
|
||
identifiable as a verdict from an approved reviewer — a `.creator.login` in the workflow's
|
||
`H10_REVIEWERS` allow-list **and** a `Review-verdict:` description, which is what
|
||
`post-review-verdict.sh` writes. Anything else, including any shape the workflow does not recognise,
|
||
is **re-derived** rather than inherited. An existing **`failure`** is left alone on a weaker test —
|
||
attributable (`Review-verdict:`, a non-null creator, and a base matching this PR's) but **not**
|
||
necessarily allow-listed — see the state asymmetry below. (Measured: a status POSTed with a user credential carries a creator; one POSTed by an
|
||
Actions job carries `"creator": null`.) Without this, an exemption obtained once was accepted
|
||
unchanged on every later run.
|
||
|
||
**The allow-list replaced a "non-null creator" test (ersatztv#742), and how wide that old test
|
||
really was is the point.**
|
||
Non-null proved only that *some* user credential POSTed the status, and this repo stores credentials
|
||
for accounts that are not reviewers. `RENOVATE_TOKEN` is a `write:repository` PAT of the `renovate`
|
||
bot; unlike the registry credential it **cannot be scoped down** (Renovate needs repo write to push
|
||
branches and open PRs), and secrets are a per-repo *store* rather than a per-workflow grant, so a
|
||
PR-added workflow can reference it and POST a `Review-verdict:` success that reads back
|
||
`creator: renovate` — non-null, and therefore inheritable. (Inferred, not observed: `renovate` has
|
||
never actually posted a commit status in this repo — every row on all six Renovate-authored PR heads
|
||
carries `creator: null`. The user-credential-vs-Actions-token asymmetry underneath it IS measured.)
|
||
Membership makes every non-reviewer account's
|
||
**`success`** re-derive exactly as an Actions-token status does. The list is a **literal in the workflow**, not a
|
||
repo `vars:` entry or a secret: those are settable without a PR, i.e. without this gate, while
|
||
`.gitea/` is on the PROTECTED path list so the edit cannot ride a docs-only exemption either.
|
||
|
||
**The allow-list governs an inherited `success`. It does NOT govern a `failure`**, and the asymmetry is
|
||
deliberate: an off-list `failure` is left alone, because inheriting a rejection can only ever withhold
|
||
an exemption while re-deriving one can turn it green on an exempt PR. The worst a forged `failure`
|
||
achieves is a stall, and `post-review-verdict.sh` POSTs unconditionally, so any human clears it in one
|
||
command.
|
||
|
||
**Beyond that, two other readers of `.creator` stay broad, and the split is load-bearing.** `read_existing_verdict` carries two flags: `ex_attributable` (an attributable
|
||
`Review-verdict:` row for this base, whoever wrote it) and `ex_human` (`ex_attributable` **plus**
|
||
membership).
|
||
|
||
- The **inheritance short-circuit** reads `ex_human`. Narrowing is safe there: an unrecognised account
|
||
falls through to re-derivation.
|
||
- The **last-moment re-read** reads `ex_attributable`, because it asks the opposite question — "did a
|
||
reviewer post a verdict while we were classifying". Narrowing it makes the job stop abstaining and
|
||
post its exemption over the row, and the post-write repair would not cover that — it is skipped
|
||
whenever the high-water mark could not be established, which since ersatztv#849 is also the case in
|
||
which no exemption is posted at all. It additionally requires the
|
||
state/creator/description triple to have *changed* since the first read, because the two calls no
|
||
longer compute an identical predicate and "changed" can no longer be inferred from "fired".
|
||
- The **post-write raced check** stays broad on `creator != null`. Not because narrowing it would let
|
||
a rejection go green — a real reviewer is on the list by construction — but for the
|
||
misconfiguration case, a second human account added to the repo and forgotten from the literal.
|
||
Its reach is narrower than it looks: it covers such a verdict landing inside the **write window**
|
||
and nothing else, since a verdict already on the head sits below the high-water mark and is
|
||
invisible to it. Within that window the errors are asymmetric — a repair to `pending` is
|
||
recoverable, a wrongly-standing `success` is not — so it stays broad. The cost: any verdict-shaped
|
||
row landing in that window trips the sticky repair sentinel and costs that head its exemption until
|
||
a human clears it.
|
||
|
||
Pinned by `test_the_MID_RUN_abstain_still_fires_for_a_NON_allowlisted_creator` and
|
||
`test_the_RACED_verdict_check_still_counts_a_NON_allowlisted_creator`.
|
||
|
||
**If a reviewer's verdicts stop being honoured, this is why.** Whenever the job declines to inherit an
|
||
existing status it logs the creator, the allow-list and the description together, and says to add the
|
||
account to `H10_REVIEWERS` in `.gitea/workflows/review-verdict.yml`. (A separate `::warning::`
|
||
annotation carrying the same three values was tried and withdrawn: its only delta was a severity whose
|
||
rendering could not be probed, and it needed a guard of its own to stop contradicting the line above
|
||
it.) `post-review-verdict.sh` posts
|
||
with whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH`, and since ersatztv#845 it
|
||
**checks that it is an allow-listed one** — at the terminal that made the mistake, not one PR event
|
||
later. It reads the status back after posting, identifies its own write by state and description, and
|
||
refuses **before writing the comment** unless `.creator.login` is a member; a `creator: null`
|
||
(Actions-token) status gets its own message, since there is no account to name. The allow-list is
|
||
DERIVED from the `H10_REVIEWERS` literal in `review-verdict.yml` by `scripts/lib/h10-reviewers.sh`,
|
||
not restated beside it, so there is one declaration rather than two plus a parity test.
|
||
|
||
**The membership requirement is `success`-only, mirroring the gate's own asymmetry.** An existing
|
||
`failure` is inherited on ATTRIBUTABILITY alone, whichever account recorded it, so requiring
|
||
membership for a `BLOCKED` verdict would refuse one the gate honours perfectly well — and would
|
||
leave an off-list reviewer no supported way to record a rejection, since the comment would be
|
||
withheld too. A negative verdict from any attributable account is written and commented normally.
|
||
|
||
What that changes is the *diagnostic*, not the safety direction: a non-allow-listed POSITIVE verdict
|
||
was always a stall, never a fail-open. Before, a changed writer credential reported success and each
|
||
*subsequent* PR event re-derived the status — to `pending` on a PR that needs a verdict, or to an
|
||
exemption **`success`** on a docs-only or Renovate-manifest one, which is the quieter and worse
|
||
half — so the symptom was a PR that kept losing its verdict rather than an immediate refusal. Now it
|
||
is an immediate refusal naming the account and the allow-list.
|
||
|
||
**The residual, stated precisely, because "the `ask` half-state" describes only half of it.** The
|
||
check runs after the POST, so a refused verdict leaves a `review-verdict/h10` **standing on the
|
||
head** with no comment beside it. On the *hook* path that is the safe half-state
|
||
`release.verdict-writes-status-before-comment` designates — nothing for condition (c) to classify,
|
||
so it asks rather than granting. On the *server* path it is weaker: branch protection binds the
|
||
context NAME and not its issuer, so a `success` posted by any account satisfies the required check
|
||
until the gate re-derives it on the next PR event. **A refusal from this tool is not a reason to
|
||
trust the green it leaves behind.** That is not a regression — it was equally true before #845, when
|
||
the tool reported success and said nothing — and it is deliberately not repaired here, because a
|
||
second corrective write is the sticky-sentinel mechanism ersatztv#849 is separately designing.
|
||
|
||
**One false ACCEPT, named because the direction matters.** The writer reads the workflow in the local
|
||
checkout while the gate runs the base-resolved one, so on a branch that ADDS a reviewer the writer
|
||
accepts an account the live gate will not yet inherit from — the #845 stall, reached through the
|
||
check meant to catch it, on the one branch where someone is editing the allow-list. Accepted:
|
||
closing it means a live remote read on the merge-consent write path, whose own read failure would
|
||
have to resolve either to refusing every verdict or to passing. The window is one PR long and the
|
||
residual is a stall, never an unreviewed merge.
|
||
|
||
This remains a *provenance* check, not an authentication one — an **allow-listed** collaborator's own
|
||
write-scoped token can still POST a verdict they did not form, branch protection binds the context
|
||
*name* rather than its issuer, and no repo-side change closes that (ersatztv#697). That provenance
|
||
asymmetry is *why* the credential scoping in `ci.actions-credential-scoping` mattered: a forgery
|
||
through an allow-listed **user** credential inherits, while one through a job's `GITEA_TOKEN` carries
|
||
`creator: null` and is re-derived, so it must win a race. Note also that re-derivation is **not** a
|
||
race the attacker can lose: it fires only on the trigger's `types`, and posting a status is not one of
|
||
them, so a POST timed after the last PR event stands until the next one.
|
||
|
||
Deciding either exemption requires the PR's **complete** changed-file list, which the workflow does
|
||
not compute itself: it calls `scripts/pr-changed-files.sh`, the single shared implementation also
|
||
used by the advisory hook `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv#649). The workflow
|
||
reads that script's **exit status** — a non-zero exit means "could not tell" and withholds the
|
||
exemption; its stdout is meaningless on any failure path and is never consumed.
|
||
|
||
**Never write a classification guard as `producer | grep -q…` here.** Under `set -o pipefail`, `grep -q`
|
||
exits at its first match, the producer takes SIGPIPE (141), and a MATCH is reported as a failed
|
||
pipeline — inverting the guard for any PR whose path list exceeds the pipe buffer. That let a large PR
|
||
be classified docs-only, and let one editing `.gitea/` skip the protected-path check entirely. A
|
||
here-string is **also** wrong (bash spills a large one to temp storage, which fails the same way when
|
||
temp is full). **Count** instead — `grep -c` drains stdin over an ordinary pipe — evaluate the counts
|
||
once at top level rather than inline in an `if`, and fail closed on a non-numeric result. Full detail:
|
||
`ci.grep-q-pipefail-inversion`.
|
||
|
||
That script takes the expected base branch as a **required 5th argument** and refuses to enumerate when
|
||
the PR's live base does not match it, checked both before and after paging (ersatztv#698).
|
||
`/pulls/{n}/files` diffs against the PR's *live* base, so retargeting changes the answer without moving
|
||
the head sha — a PR opened into `main` and retargeted mid-run was granted a docs-only exemption while
|
||
its diff against `main` carried a C# file. The workflow passes the base from the `pull_request_target`
|
||
payload, which a retarget cannot rewrite, and `edited` is in `types:` so a retarget reclassifies.
|
||
`edited` gives **detection, not atomicity**: runs are not serialized, so a stale run could still post
|
||
`success` after the reclassifying run posted `pending`.
|
||
|
||
**That residual is now fenced — NARROWED, not resolved (ersatztv#706; correction ersatztv#849).** The pre-write fence does not re-count *after* the POST, so a retarget landing between its final pre-write count and the write used to yield a **permanent** forged green: the successor run consumes the `edited` event and exits on the existing status, and the stale run then posts last with nothing left to correct it. Since ersatztv#849 a SEPARATE post-POST re-count (below) makes that green transient rather than permanent. Runs are still not serialized — instead a run that was
|
||
overtaken *declines to write its classification*. The job counts `change_target_branch` events on the PR's issue timeline
|
||
at start and again immediately before its POST, and posts **nothing** if the count moved. The count is
|
||
the key precisely because the branch *name* is ABA-vulnerable: `main → scratch → main` reads `main` at
|
||
both ends, which is how the forged exemption was obtained in the first place. Abstaining never strands
|
||
a PR, because every retarget fires `edited` — the event that makes one run abstain has already queued
|
||
its successor.
|
||
|
||
If the count can't be established (unreadable timeline, an empty FIRST page, or paging that reached
|
||
the 20-page cap on a NON-empty one — since #870 an empty page before the cap no longer ends the walk),
|
||
only the exemption `success` is withheld; `pending` still posts, since it blocks the merge
|
||
immediately and withholding it would strand ordinary PRs whenever the timeline is unreadable. That is
|
||
the right trade but not a free one — a generic `pending` still masks a rejection landing in its own
|
||
write window. What made that durable is gone: since ersatztv#849 the post-write check runs after
|
||
**every** write, so such a write is repaired to the sticky sentinel instead of being re-derived into
|
||
an exemption by a later run. **If an exempt PR is unexpectedly missing its status after a retarget,
|
||
this is why** — the job log names the counts.
|
||
|
||
**The count is also re-taken AFTER the POST, on the exemption path only** (ersatztv#849,
|
||
`ci.verdict-unverified-write-sentinel`). The pre-write fence covers *writing while overtaken*; it never
|
||
covered *being overtaken after writing*, which was the worse of the two — a retarget landing after the
|
||
final pre-write count left a stale `success` with the `edited` event already consumed by a successor
|
||
that short-circuited, so nothing remained to reclassify. It makes that green **transient rather than
|
||
permanent**, not absent: the `success` is live between its POST and the repair, so branch protection
|
||
or a scheduled auto-merge can still observe it. Closing that window needs a compare-and-set the API
|
||
does not offer. The retarget axis only: a push after the POST
|
||
moves the head, so the status no longer gates that PR, while a retarget changes the effective diff with
|
||
the sha unchanged.
|
||
|
||
Worth knowing before reaching for the obvious alternative: **a concurrency group does not work here**,
|
||
measured rather than assumed. Gitea 1.25.4 auto-cancels superseded `push` runs on a branch, but *not*
|
||
`pull_request_target` runs — two runs for one PR genuinely overlap, and adding
|
||
`concurrency: {…, cancel-in-progress: false}` changed nothing (probe runs still overlapped by 36s).
|
||
`cancel-in-progress: true` is deliberately untried, because a cancelled run leaves an exempt PR
|
||
statusless with nothing left to re-trigger it. Full measurements and the two surviving residuals:
|
||
`ci.verdict-write-retarget-fence`.
|
||
|
||
Since ersatztv#803 that fence counts **two** event types on the one timeline walk:
|
||
`change_target_branch` for the base alias (`main -> S -> main`) and `pull_push` for the HEAD alias
|
||
(a force-push `H1 -> H2 -> H1` spanning `pr-changed-files.sh`'s paging, which leaves every sha
|
||
comparison equal). Either count moving withholds the write. The run log line is
|
||
`Mutation fence: N retarget event(s) and M push event(s) … (trusted=…)`.
|
||
|
||
The `push`-supersession half of that claim stays **1.25.4-dated on purpose** (ersatztv#747,
|
||
2026-08-28). It is not unobservable, but it is no longer reproducible ON DEMAND. No workflow triggers
|
||
on a push to a *non-`main`* branch — `docker-build.yml` filters its `push` trigger to `main` plus `v*`
|
||
tags, `ci-image.yml` to `main` and `docker/ci/**` with deliberately no `tags:` key
|
||
(`ci.toolchain-image-publish-is-a-dispatch`), and the other four have no `push` trigger at all — and
|
||
`main` itself refuses direct pushes since `release.main-direct-push-disabled`. A PR merge IS a push to
|
||
`main`, so two merges landing close together remain a route; a `v*` tag push triggers `docker-build.yml`
|
||
but is **not** one, because supersession is per-ref and a release tag is pushed once. What is gone is the
|
||
cheap disposable probe — the scratch-branch route the other measurements used. Re-running it would mean
|
||
arranging two real merges, or adding a throwaway trigger; both cost more than the claim is worth,
|
||
because nothing branches on it. The `pull_request_target` half the fence actually relies on was **not**
|
||
re-measured either and is 1.25.4-dated too.
|
||
|
||
The two facts this paragraph rests on — that no workflow triggers on a push to a non-`main` branch,
|
||
and that `main` refuses direct pushes — were **re-checked on 2026-09-02 (ersatztv#869)** and both still
|
||
hold. Note what that does and does not cover: it re-checks the REASON, not the claims. The
|
||
`push`-supersession and `pull_request_target` measurements above remain 1.25.4-dated and were not
|
||
re-run by #869 either. Re-checking the reason matters on its own, because a reason decays exactly like
|
||
the claim it excuses. One limit is worth stating so this is not over-read: the enumeration covers
|
||
`push:` triggers only. Four of the six workflows carry `workflow_dispatch:`, so a disposable run can
|
||
still be raised on demand — what is gone is the cheap scratch-BRANCH push probe, not every route to a
|
||
throwaway run.
|
||
|
||
Separately, after posting **any** status the job re-reads the per-POST status history and, if a human
|
||
`Review-verdict:` row appeared during the write window, overwrites its own status with `pending` and
|
||
logs an error. It ran only for a `success` write until ersatztv#849, on the claim that a `pending`
|
||
cannot turn a rejection green — false, since a generic `pending` masks a rejection just as well and,
|
||
carrying no marker, is re-derived into an exemption by the next run with the human's row now below
|
||
THAT run's high-water mark. The repair is `pending`, never a copy of the human's verdict, which would
|
||
attribute a human decision to the job.
|
||
|
||
**Which sentinel the repair writes is part of the answer.** There are two, and they are not
|
||
interchangeable (`ci.verdict-unverified-write-sentinel`):
|
||
|
||
- `Human verdict raced this exemption write — re-post the verdict` **asserts** that a verdict existed
|
||
and a write buried it. Only the arm that actually *counted* such a row may claim it, and only a
|
||
human re-posting clears it.
|
||
- `Status write could not be verified — re-post the verdict` states only what was established —
|
||
that the write could not be checked. Every "could not check" arm writes this one: an unreadable or
|
||
over-cap history, an impossible empty history, an unusable count, no high-water mark at all, a
|
||
post-POST retarget, and an unreadable combined-status read. It is equally sticky, and additionally
|
||
**reconcilable**: a later run pages `/statuses/{sha}` in full and either finds a verdict underneath
|
||
it — upgrading to the repair sentinel — or finds none and clears it, so a transient API failure does
|
||
not cost a head its exemption permanently.
|
||
|
||
**No high-water mark means the write becomes the sentinel** — every re-derivable state, not only the
|
||
exemption — withheld before the POST rather than posted and repaired: the defect is known in advance,
|
||
and publishing a green to take it back opens a window branch protection, and an already-scheduled
|
||
auto-merge, can see. A generic `pending` is included because it is exactly what a later run
|
||
re-derives; `$REPAIR_DESC` is not, being the stronger fact.
|
||
|
||
**Every path that cannot establish what the head carries REPLACES the unknown state** rather than
|
||
merely declining to write: the combined read (retried once first), all four page-2 completeness
|
||
refusals, a row or consumed field whose type the schema does not allow, the path-predicate failure,
|
||
the fence branch that cannot trust its retarget count while holding a derived `success`, and the two
|
||
OBSERVED-mutation arms when the head carries a row the run did not inherit. Declining protects a real verdict and leaves a *forged* one standing, which is what an
|
||
off-list `success` is; the job went red on a status branch protection does not read, and the fence
|
||
branch was reached only after the classification had DECLINED to inherit the very row it then left
|
||
current. Nothing is destroyed — `/statuses/{sha}` keeps one row per POST, so the next run's
|
||
reconciliation finds a masked verdict and tells the reviewer to re-post. Writing the sentinel and
|
||
failing the job are separate decisions: the read refusals were already non-zero exits and stay red,
|
||
the fence branch exited 0 and still does.
|
||
|
||
**Reconciliation needs a witness.** It may clear the sentinel only over a complete history that
|
||
CONTAINS the sentinel's own row, matched by the `id` the combined read reported — degrading to a
|
||
DESCRIPTION match, which identifies a row with the same text rather than that row, only where the
|
||
server omits `id`. `ex_unverified` means the combined endpoint just returned that row
|
||
and `/statuses/{sha}` keeps one row per POST, so a complete-but-empty history contradicts a write
|
||
that demonstrably happened — and `page_statuses` accepts an empty page 1 as complete, which is what
|
||
made the shape reachable. The cost lands where the replacement actually cost something: on an
|
||
APPROVED PR, reconciliation finds the masked verdict and upgrades to the repair sentinel, so the
|
||
recovery is "you are asked to re-post", not "it clears itself".
|
||
|
||
**Every `/statuses/{sha}` read is PAGED** (ersatztv#763). The history is read twice on an ordinary
|
||
run — before the write for the high-water mark, after it for the race check — and a THIRD time on a
|
||
head carrying the unverified sentinel, for the reconciliation (ersatztv#849). `limit` clamps to
|
||
`MAX_RESPONSE_ITEMS` (measured 50), so a single read of a busy head returned a partial list.
|
||
|
||
**Be precise about what that cost** — ersatztv#763's framing of it is too strong. Under the server
|
||
default (`created_unix DESC`) page 1 holds the *newest* rows and ids are monotonic with `created_at`,
|
||
so page 1 already carried both the true maximum id and every row newer than the mark — the only rows
|
||
the post-write check selects on. A single-page read therefore missed a raced verdict only if **more
|
||
than 50 rows were created inside the write window**, not merely on "a head with more than 50 rows".
|
||
What actually removed PR #761's stall is the deletion of the page-2 probe below, not the paging.
|
||
|
||
The walk earns its place for a different reason: it removes the gate's one fail-toward-SUCCESS path
|
||
from depending on an **undocumented ordering** the server honours only coarsely (page 1 came back
|
||
`114,112,113,111,110` — newest-first, but not strictly, and nothing in the API contract promises even
|
||
that). Both reads walk to a **validated empty page**, never terminating on a short one, retrying each
|
||
page once. The termination rule is page-size independent; the *capacity* is not — 20 requests of 50
|
||
validate at most 950 rows, since the twentieth must be the empty terminator. The terminator is measured
|
||
per endpoint and they differ: `/statuses/{sha}` returns `[]`, `/issues/{n}/timeline` a bare `null`,
|
||
`/commits/{sha}/status` an object with `statuses: null`.
|
||
|
||
**The two walks terminate by OPPOSITE rules, and that asymmetry is deliberate** (ersatztv#870, settled
|
||
by ersatztv#893 on 2026-09-02). `count_pr_mutations` reads the timeline to its cap and trusts the
|
||
counts only when the LAST page came back empty; `page_statuses` still returns on its FIRST empty page.
|
||
They differ because the ENDPOINTS differ, established from the v1.27.1 source: `getCommitStatuses`
|
||
serializes every row the paged query returns — an unconditional `append` loop, the only filter being a
|
||
SQL `WHERE` the database evaluates in the same query as the `LIMIT`/`OFFSET` — so a serialized page is
|
||
exactly as long as its database page, and an empty page really is the end. `ListIssueCommentsAndTimeline` instead drops
|
||
`CommentTypeCode` rows AFTER paging, which is what makes a fully filtered page byte-identical to the
|
||
end of the list. The same source explains the header: `/statuses/{sha}` sets `X-Total-Count` from a SQL
|
||
COUNT, so it is a true total (page 1 of 50 on a head reporting 63, measured 2026-09-02), where the
|
||
timeline reports its post-filter page length. **Do not "tidy" the two walks into agreement.**
|
||
|
||
**The page-2 probe is gone.** ersatztv#751 treated "there are rows I did not read" as "assume raced".
|
||
That fired on Renovate PR #761: a head that grew past one page over ordinary CI re-runs had its
|
||
exemption repaired away, with an `::error::` asserting a human verdict was overwritten when the head
|
||
carried none, and the sticky sentinel then refused re-exemption on every later run.
|
||
|
||
The two directions are **not** symmetric, and the asymmetry is deliberate:
|
||
|
||
- **Post-write, uncertainty fails closed.** An unreadable history, an over-cap history, or a count that
|
||
does not come back as a number all repair to `pending`. Previously an unreadable history warned and
|
||
left the exemption green while the page-2 probe repaired on the same uncertainty — one check
|
||
disagreeing with itself.
|
||
- **Pre-write, a partial list still yields a mark.** The mark gates the post-write check entirely, so
|
||
refusing one *skips* the check and is itself a fail-open. This rests on the DESC default above: the
|
||
newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it. A
|
||
*validated* empty history is a different case and is not abandoned: it yields a mark of 0, which is
|
||
correct, since every later row is newer — that is the normal first run. What abandons the mark is a
|
||
read that both **failed** and returned nothing, the pre-existing gap tracked as ersatztv#849 and
|
||
unchanged here. A non-empty history carrying no numeric id is reported as unusable rather than
|
||
collapsed to 0.
|
||
|
||
Two id comparisons are **numeric-only**, closing the *string* half of a fail-open (a type guard, not
|
||
a value bound — a corrupt but genuinely numeric id would still inflate the mark, which is not
|
||
attacker-controllable since ids are server-assigned): jq orders strings above every number, so one
|
||
`"id": "99999"` would inflate the high-water mark until nothing looked newer, and
|
||
`.id > $since` would read any string id as newer than any mark — making a pre-existing base-mismatched
|
||
verdict look raced on every run, a permanent stall. `.creator` is likewise type-tested before it is
|
||
indexed, because `.creator.login` on a non-object exits jq 5 and, under `set -e`, took the step down
|
||
after the green was posted and before the repair.
|
||
|
||
**Rejected alternatives**, recorded so they are not re-adopted:
|
||
|
||
- **A currency witness** (require a row above the mark, proving the read reflects the POST just made).
|
||
Counting *any* row above the mark does not witness this job's write, so a stale-but-valid snapshot
|
||
carrying an unrelated newer row still passes; and a schema-valid stale read is not retried, so one
|
||
such response turns a transient anomaly into a permanent sentinel. Gitea here is a single instance
|
||
with no read replicas, so the hazard has no mechanism behind it.
|
||
- **`sort=highestindex`** (index ASC, measured), which would close the mid-walk-insert gap below. ASC
|
||
puts the *oldest* rows on page 1, which inverts the partial-mark fallback: the incomplete walk's
|
||
maximum becomes the max of the oldest rows, leaving every pre-existing row above the mark and read as
|
||
raced. That is the #761 failure, re-introduced to close a smaller one. A test asserts the request
|
||
carries no sort parameter.
|
||
|
||
**Accepted residual: a row inserted mid-walk can be missed.** These are independent offset-paginated
|
||
GETs with no snapshot token, so under DESC a new row lands at position 0 — on a page already read —
|
||
while everything else shifts down. It is bounded: a row arriving after this job's POST is not one the
|
||
job overwrote, and being newest it wins on the combined endpoint branch protection reads.
|
||
|
||
**An empty post-write history is rejected.** The walk terminates on an empty page — correct before the
|
||
write, impossible after it, since this job has just POSTed and the endpoint returns one row per POST. A
|
||
well-formed "no statuses exist" is not retried (the body is valid), so accepting it would conclude
|
||
`raced=0` from a list that cannot be real, silently. This is *not* the withdrawn currency witness: that
|
||
asked whether ANY row sat above the mark, which an unrelated newer row satisfied; this asks only whether
|
||
the list is empty, which no unrelated row can produce.
|
||
|
||
The `::error::` names its own cause, and there are three: a verdict actually **found**; a read that
|
||
could not be **completed**; and a read that completed but returned an **impossible** answer. The third
|
||
is not a variety of the second — an operator holding a sticky sentinel needs to know whether to go
|
||
looking for an API failure that never happened. The sentinel *description* stays generic, because the
|
||
classification recognises it as a fixed point.
|
||
|
||
Three properties of this workflow are security-relevant and are **structurally** asserted by tests in
|
||
`scripts/tests/test_pr_changed_files.py` — those tests pin the workflow's shape, which is not the same
|
||
as establishing that the gate cannot be forged (see the residual below, and ersatztv#697/#698):
|
||
|
||
- **The trigger is `pull_request_target`, scoped to `branches: [main]`** — never plain
|
||
`pull_request` (ersatztv#672). Gitea resolves a `pull_request` workflow *definition* from the PR's
|
||
own head, so under that trigger a PR editing `review-verdict.yml` ran its own rewritten copy and
|
||
could post `review-verdict/h10=success` for itself. The base-ref checkout below binds the scripts
|
||
this job runs; only the trigger binds the definition. The `branches` filter is half the fix, not a
|
||
refinement of it: base resolution means the *base branch* supplies the gate, so an unfiltered
|
||
trigger merely moves the rewrite to an attacker-pushed base — and a status forged there is
|
||
inherited by any later PR carrying the same head sha (ersatztv#663). `pull_request_target` is safe
|
||
here **only** because this job never checks out or executes head-supplied code. Verified on this
|
||
instance with four scratch PRs rather than inferred from GitHub; full rationale in
|
||
`docs/decisions/records/ci/gate-trigger-base-resolved.md`. **This closes the rewrite route through
|
||
this workflow, not the class:** `docker-build.yml` is also head-resolved and must stay on
|
||
`pull_request` because it builds the PR's code, so it got the read-only status identity instead —
|
||
the `ETV_STATUS_AUTH` it passed was a PAT scoped `write:package` + `read:repository`, which the
|
||
status endpoint refuses (`ci.actions-credential-scoping`, ersatztv#697). Since ersatztv#885 it
|
||
passes no credential there at all: the status read is anonymous, so the PR route materialises
|
||
nothing to refuse. The inventory was never that one
|
||
workflow, though: Gitea injects a write-capable `GITEA_TOKEN` into every job and branch protection
|
||
binds the *context*, not its issuer. Gitea >=1.26 with the Actions default set to **Restricted**
|
||
(server-management#714) binds the injected token, but does not close the class either — not against
|
||
a personal token, and not against `RENOVATE_TOKEN` (ersatztv#742). **And none of it was necessary:
|
||
direct pushes to `main` were server-side permitted, so the gate could be skipped without any forgery
|
||
(ersatztv#743). That is now CLOSED — `main` carries `enable_push: false` **and**
|
||
`block_admin_merge_override: true`, so it is reachable only through the PR merge path, the one path
|
||
on which Gitea evaluates `status_check_contexts`, and an admin cannot `force_merge` past them
|
||
(`release.main-direct-push-disabled` — neither field is citable alone).** Note the fix is *disabling* push, not whitelisting it: a
|
||
push whitelist naming `timothy` was measured to still admit the push, and `timothy` is the identity
|
||
every session, PAT and injected `GITEA_TOKEN` already acts as, so the whitelist form would have
|
||
closed nothing. The block binds a site admin at pre-receive but not a credential that can first
|
||
PATCH branch protection off — an accepted residual, recorded in that decision. The
|
||
exemption path has separate defects of its own (ersatztv#698). One operational
|
||
consequence of the trigger change: a PR whose base is not `main` now gets **no**
|
||
`review-verdict/h10` at all. That is fail-closed. `edited` **is** now among the trigger's `types`
|
||
(ersatztv#698), so a PR retargeted onto `main` reclassifies instead of staying statusless until its
|
||
next push — but note that only gives *detection*: runs are not serialized, so a stale run can still
|
||
post `success` after the reclassifying run posts `pending` (ersatztv#706).
|
||
- **The checkout takes the PR's BASE ref**, `ref: ${{ github.event.pull_request.base.sha }}` with
|
||
`persist-credentials: false` — never the head. This job judges the PR, so the PR must not supply
|
||
the code that judges it; a head checkout would let a PR edit the enumeration to return an empty
|
||
list and exempt itself.
|
||
- **`scripts/jq-preflight.sh` runs in floor-only mode**, never `--expect`. This job writes a
|
||
branch-protection-**required** status, so an exact version pin would turn any jq upgrade on the
|
||
runner into a repo-wide merge deadlock.
|
||
|
||
A PR whose base predates ersatztv#658 has no such script on its base ref; that case posts `pending`
|
||
with the reason rather than dying with no status at all.
|
||
|
||
⚠️ **Changing `review-verdict.yml` itself: it is not exercised by its own PR.** Base resolution cuts
|
||
both ways — the PR editing this workflow runs the version already on `main`, so an edit goes live
|
||
**only on merge**, repo-wide, having never run. A broken edit merges green and then breaks the gate
|
||
for every subsequent PR, and the PR that would repair it is gated by the same broken workflow. Do not
|
||
trust the editing PR's own checks. Verify the way ersatztv#672 did:
|
||
|
||
1. Push a scratch **base** branch carrying the candidate workflow.
|
||
2. Open a throwaway PR from a scratch head *into that base*, so the candidate is the definition that
|
||
runs. Have it post a **probe-named** context (e.g. `review-verdict/h10-PROBE`), never the real
|
||
`review-verdict/h10` — a probe must not be able to forge the gate it is testing.
|
||
3. Read the resulting commit statuses to see which definition actually ran, then delete both
|
||
branches.
|
||
|
||
The same shape is what makes a `branches:`/`types:` change verifiable at all, since neither can be
|
||
observed from the editing PR. Note step 2 requires the scratch **base**'s own `branches:` filter to
|
||
name that base — the definition comes from the base, so a base the filter does not admit produces no
|
||
run at all.
|
||
|
||
⚠️ **Never write an expression delimiter inside a `run:` body here — a comment is NOT inert**
|
||
(ersatztv#751, `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 so the result can be spliced back in. That
|
||
rewrite is all-or-nothing: 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`**.
|
||
|
||
That is not hypothetical. From 8f6d4f443 (2026-08-03) to 2026-08-06 the classify step **never ran**.
|
||
The #706 note above, explaining why a concurrency group does not work here, quoted a `concurrency:`
|
||
snippet containing a PR-number expression *as an illustration*, in a shell comment. `pr number` is not
|
||
a valid expression. So `review-verdict/h10` was posted by nothing but a human hand for three days,
|
||
both exemption classes silently stopped working, and every run reported success. The prose documenting
|
||
a fix disabled the fix.
|
||
|
||
**The silent green is the real defect.** An absent required status reads as "not reviewed yet", which
|
||
is indistinguishable from the correct pending state — so an ordinary PR looked ordinary while the gate
|
||
was dead, and the cost landed only where no human was in the loop. PR #739 (docs-only) merged
|
||
2026-08-05 with **zero** commit statuses on its head, and got in only because admin force-merge was
|
||
still enabled; ersatztv#743 removed that escape the next day, so a docs-only or Renovate-manifest PR
|
||
arriving after that would simply have been stuck with no bypass. The two Renovate PRs in the window
|
||
escaped by timing, merging minutes before the bad commit.
|
||
|
||
Three things now hold the line, and they are deliberately different in kind:
|
||
|
||
- **The prose names expressions instead of quoting them** — write "a
|
||
`github.event.pull_request.number` expression", not the delimiters. Pass values in through the
|
||
step's `env:` block, which is interpolated per value, so a bad payload there cannot take the body
|
||
with it.
|
||
- **A start-marker guard turns a dropped step RED.** The classifier writes a marker as its first act
|
||
and an `if: always()` step fails the job when it is missing. It asserts execution *started*, never
|
||
that it completed — the classifier has several legitimate `exit 0` abstention paths. The guard's own
|
||
body must stay expression-free, or the mechanism it guards against can delete the guard too, and
|
||
that absence would be silent as well.
|
||
- **Two static guards**, in `scripts/tests/test_pr_changed_files.py`: no delimiter in *any* `run:`
|
||
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 — 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
|
||
skipped), nor an unclosed opener. Catching those needs an expression parser — it does **not**
|
||
catch "a payload that cannot evaluate, wherever it sits".
|
||
|
||
Worth knowing why nothing caught this for three days: every *other* workflow-shape test in that file
|
||
reads `_code_lines()`, which strips comments. That is correct for what it was for, but it encodes the
|
||
assumption this bug falsifies. The strict test reads the raw scalar, and must never adopt
|
||
`_code_lines`.
|
||
|
||
⚠️ **A page past the end of `/issues/{n}/timeline` is JSON `null`, not `[]`** — and this instance is
|
||
not consistent between endpoints (`/issues/{n}/comments` returns `[]` when empty). The retarget
|
||
fence's timeline walk (`count_retargets`, renamed `count_pr_mutations` by #803) gated on
|
||
`type == "array"`, so it read the real terminator as *unreadable*:
|
||
the walk never reached a validated empty page, `rt_ok` was never `yes` for **any** PR, and the fence
|
||
therefore withheld **every** exemption `success`. Renovate and docs-only PRs got no status at all —
|
||
the same user-visible outcome as the dropped step above, by a completely unrelated route. So fixing
|
||
the interpolation alone would not have restored the exemptions.
|
||
|
||
Two things kept it invisible, and both are worth generalising:
|
||
|
||
- It shipped in the **same commit** (8f6d4f443) that stopped the step executing, so the fence had
|
||
never once run in production. A guard's first real execution is not the same event as its merge.
|
||
- The **test double asserted the wrong shape while claiming measured fidelity.** Its comment read
|
||
"Real shapes, measured on this instance and deliberately mirrored" and it printed `[]` for a page
|
||
past the end. Every fence test was green against a response the server never produces, so the
|
||
`array`-only gate was never exercised by the suite either. With the double corrected and the old
|
||
gate restored, **most of the fence suite fails** — 18 tests when first measured at `c710db4a1`, 21
|
||
once three more fence-dependent tests existed. The invariant is the point, not the count: they had
|
||
all been passing for the wrong reason. (Given as a range on purpose: a bare count goes stale within
|
||
a couple of commits, inside a section about stale claims.) When a double claims
|
||
fidelity, that claim is a test assertion and needs re-measuring like any other.
|
||
|
||
The type is now read as a value (`case` over `jq -r 'type'`) rather than through `jq -e`, whose
|
||
exit-status semantics already bit this workflow once at jq 1.6, and both `null` and `[]` are read as
|
||
the same EMPTY shape. The regression test is parameterised over both because both are live on this
|
||
server. An empty page — `null` OR `[]`, which take the SAME rule since ersatztv#803 — is **no longer
|
||
accepted as exhaustion before the page cap** (ersatztv#870, 2026-08-30). Gitea pages this endpoint at the database level and
|
||
filters afterwards, so a page whose 50 rows are all inline-code comments serializes as exactly the
|
||
value the walk used to trust as the end of the list, while later pages still hold events — 50 comments,
|
||
which a PR author can create on their own PR, bought silence on both axes. The walk now SKIPS an empty
|
||
page before the cap, reads every page up to its 20-page cap, and trusts the counts only when the LAST
|
||
page it read came back empty — the same rule the status walk above states for the CAP, though not for termination: that walk still returns on its first empty page, and whether it needs the same change is tracked at ersatztv#893 rather than assumed (measured 2026-08-30, `/statuses/{sha}` returns a TRUE `X-Total-Count`, which this endpoint does not). **That
|
||
moves the inference to the cap rather than removing it, and the residual is real**: pages 1-19 cover
|
||
offsets 0..949, so a timeline of more than 1000 rows whose offsets 950..999 are all filtered still
|
||
returns an empty page 20, and the walk publishes a too-low count as trusted. Both re-reads share the
|
||
blind spot, so this is the same defeat at about 10x the price — the 50-row filtered block is the
|
||
same, but the timeline it must sit in grows from ~100 rows to over 1000, and on their own PR the
|
||
padding is the attacker's to supply — not a closed hole. It is NOT the status walk's over-cap residual: `/statuses/{sha}` does not
|
||
filter after paging, so only here can a page-20 response be empty while later pages hold rows. An empty
|
||
FIRST page still ends the walk untrusted: every real PR's first page carries events (spot-checked
|
||
non-empty across #752/#753/#749/#739/#717; the counts are deliberately not recorded here because
|
||
timelines grow and such figures go stale within days), so an empty first page is ANOMALOUS rather than
|
||
EXHAUSTED, and the walk should not certify "no retarget happened" from a response it cannot explain.
|
||
`X-Total-Count` cannot shortcut this: on this endpoint it is the post-filter length of the PAGE, not a
|
||
total (`?limit=1` returns 1 on a 14-row timeline), unlike `/activities/feeds`, which does return one.
|
||
|
||
**The same nil-slice shape bites `/commits/{sha}/status`** — a third instance. A head with no statuses yet returns
|
||
`{"state":"pending","total_count":0,"statuses":null}` (measured on PR #739's head). `read_existing_verdict`
|
||
gated on `.statuses | type == "array"`, so it hit its `exit 1` and posted nothing at all — fail-closed,
|
||
same user-visible outcome. `null` is now accepted there only when `total_count` is 0, so a body that
|
||
merely lost its array is still refused and an existing verdict is still protected from a transient
|
||
error. `scripts/pr-changed-files.sh` was swept and is unaffected (`pulls/{n}/files` returns `[]`).
|
||
**The generalisable rule: a nil Go slice serialises to `null`, so every list-shaped field on this API
|
||
is suspect and only a per-endpoint measurement settles it.**
|
||
|
||
**Establishing that "no verdict exists" needs a second page, and both arithmetic guards for it are
|
||
no-ops here.** `read_existing_verdict` concluding absence is what licenses posting an exemption over a
|
||
verdict the job cannot see, so that conclusion has to be earned. Two obvious checks were tried and both
|
||
proved empty:
|
||
|
||
- **`.statuses | length` vs `.total_count`** — `total_count` is the count for the **page returned**, not
|
||
for the commit. Measured at 1.27.1 on `3aed43c6` (6 contexts): `?limit=1` returns
|
||
`len=1, total_count=1`, `?limit=3` returns `len=3, total_count=3`. Equal by construction, so the check
|
||
reads as a completeness proof while proving nothing.
|
||
- **"refuse when the page comes back full at the requested `limit=100`"** — this instance caps `limit`
|
||
at the server-wide `MAX_RESPONSE_ITEMS`, **measured at 50** (`/issues?limit=100` returns 50). A
|
||
response can therefore never carry 100 rows, and the comparison was **dead code**. The repo already
|
||
documented that cap in `scripts/pr-changed-files.sh`, two test files and `ci.script-tests-job`; the
|
||
guard was written against 100 anyway. Hardcoding 50 instead would
|
||
re-break the day the setting changes.
|
||
|
||
So the job **asks the server, and only when it matters**: if the `review-verdict/h10` row is on page 1
|
||
there is nothing further to learn (this endpoint returns the latest status per *context*, and a context
|
||
cannot recur on a later page). When the row is absent it reads **page 2** — any rows there mean the list
|
||
runs longer than one page and a verdict could be beyond it, so it refuses instead of concluding absence.
|
||
Cap-independent by construction. Paging is real here: measured `?limit=3&page=2` returning three further
|
||
rows, and `page=9` returning the same `statuses: null` terminator.
|
||
|
||
The `total_count` zero-check also requires the JSON **type** to be a number: `jq -r` renders `0` and
|
||
`"0"` identically, so a text compare would accept a schema-corrupted `"total_count": "0"` as "no
|
||
statuses".
|
||
|
||
**The repo-wide expression guard scans PARSED scalars, not raw file text.** A delimiter in an ordinary
|
||
top-level YAML comment is inert — the runner never evaluates it — so redding on it is a false positive,
|
||
and this file has now produced that false red twice. PyYAML drops those comments. A `run:` body is
|
||
itself a scalar and keeps its *shell* comments, which is the point: inside a `run:` scalar a comment is
|
||
not inert. Verified both directions by mutation — an inert top-level comment passes; the same payload
|
||
in a run-body comment still reds.
|
||
|
||
**`CLAUDE.md` and `AGENTS.md` are now PROTECTED paths.** `DOCS_ONLY` matched them, so the documents
|
||
that *define* the completion protocol, the merge-consent convention and the H10 rule were themselves
|
||
docs-only-exemptible while `.claude/` was protected — the same self-exemption the gate rules out, one
|
||
directory over. Driving the real classify body with a lone `CLAUDE.md` change produced
|
||
`review-verdict/h10=success`. It is fixed here rather than deferred because restoring the exemptions is
|
||
what makes it reachable: no exemption `success` was writable at all while the classify step was
|
||
dropped. `README.md` is deliberately not listed — ordinary prose, no enforcement. For the same reason,
|
||
#706's known residual returns with the working fence: while `rt_ok` was never `yes`, route 1 was closed
|
||
by accident.
|
||
|
||
**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
|
||
re-trigger it. Its own job context (`Review verdict / Set review-verdict status`) is **not** the
|
||
required check — a workflow must not satisfy the gate merely by running successfully.
|
||
|
||
Full rationale: `docs/decisions/records/release/verdict-status-check.md` and
|
||
`docs/decisions/records/ci/shared-pr-file-enumeration.md`.
|
||
|
||
## CI toolchain image (`docker/ci/Dockerfile`, `.gitea/workflows/ci-image.yml`)
|
||
|
||
The jobs that need a toolchain — `test`, `migrations`, `functional-e2e`, `api-docs`, `format` —
|
||
run **inside** a shared image via `container:` instead of installing their toolchain per run
|
||
(ersatztv#390). They therefore carry **no** `setup-dotnet`, **no** `setup-node`, **no** `apt-get`,
|
||
and **no** `dotnet tool install`.
|
||
|
||
**What it ships:** .NET 10 SDK, Node 22, prod-identical ffmpeg/ffprobe, `git`/`python3`/`jq`/`zstd`,
|
||
the `dotnet-ef` + `dotnet-reportgenerator-globaltool` global tools (which the `migrations` and
|
||
`test` jobs used to install on every run — **bump those versions in the Dockerfile, not the
|
||
workflow**), and **headless Chromium** for the UI-E2E flows (below). Project dependencies (NuGet/npm)
|
||
are deliberately **not** baked in — they change per commit and stay on `actions/cache`
|
||
(`~/.nuget/packages`, `~/.npm`).
|
||
|
||
**Headless Chromium for UI-E2E (ersatztv#445).** `PLAYWRIGHT_BROWSERS_PATH=/ms-playwright` holds
|
||
`chromium-headless-shell`, installed with `--with-deps` at image build time so the `functional-e2e`
|
||
job installs no browser per run. Measured on this exact base: the headless shell is **267M** where
|
||
full `chromium` is **656M**, and `chromium.launch()` resolves to the shell anyway because
|
||
`web/playwright.config.ts` never asks for headed — the accepted tradeoff being that a *headed* run
|
||
inside this image would fail. Also verified on the real base rather than assumed: Chromium launches
|
||
as **root inside a container with no `--no-sandbox`/`chromiumSandbox:false` opt-out**, so the config
|
||
carries no sandbox workaround. The Dockerfile's build-time smoke test actually *launches* the browser,
|
||
so a missing system library fails the image build rather than a CI run.
|
||
|
||
⚠️ **`ARG PLAYWRIGHT_VERSION` must equal `web/package.json`'s `@playwright/test` pin**, which is
|
||
deliberately **EXACT** (no caret): Playwright ties a browser *revision* to the package version, so a
|
||
mismatch leaves no usable browser. Renovate bumps the npm pin but cannot know about this ARG — when it
|
||
does, bump the ARG, publish the new `:<sha>` (step 1 below), then update all five container pins.
|
||
`scripts/e2e-ui.sh` guards the drift by *launching* a browser up front and failing with exactly that
|
||
instruction (it probes by launch, not by path, because `chromium.executablePath()` reports the
|
||
full-chromium path that a headless-shell-only image deliberately lacks).
|
||
|
||
**How it's layered:** `FROM ersatztv-ffmpeg:8.1.2` + `COPY --from=mcr.microsoft.com/dotnet/sdk:10.0-noble-amd64
|
||
/usr/share/dotnet` — the same pattern `docker/Dockerfile` uses for the prod image. Our ffmpeg base is
|
||
`ghcr.io/linuxserver/baseimage-ubuntu:noble`, the same Ubuntu release as the SDK image, so the copied
|
||
SDK matches the base's glibc/ICU. Keep the ffmpeg tag on that `FROM` equal to the one
|
||
`docker/Dockerfile` pins, so CI's ffmpeg stays prod-identical — that fidelity is what the
|
||
ersatztv#299 seeded-media/scanner E2E follow-ups will need.
|
||
|
||
**Bumping the toolchain is a deliberate two-step.** The jobs pin an immutable `:<sha>`, never
|
||
`:latest`, so a bad toolchain push cannot break every job at once. Both steps land in the **same
|
||
PR** — a PR that changes `docker/ci/**` without moving the pin turns `ci-image-pin` red, and the
|
||
merge-consent hook reads the combined status, so it will not auto-grant:
|
||
|
||
1. **Publish.** Push the branch with the `docker/ci/Dockerfile` change as HEAD, then dispatch
|
||
`ci-image.yml` on that branch; it publishes `ersatztv-ci:<sha>`. (`:latest` moves only from
|
||
`main` — a human pointer; **jobs must never consume it**.) Since ersatztv#744 a branch *push*
|
||
does not publish; see "Publishing from a branch is a dispatch, not a push" below.
|
||
2. **Pin.** Update the pin in `docker-build.yml` — **all five jobs together** — on top. That PR's own
|
||
CI is what proves the new image works. The pin is repeated per job because
|
||
`jobs.<id>.container.image` cannot read the workflow `env` context.
|
||
|
||
**The tag is exactly 7 hex chars — get the length right, not just the commit** (ersatztv#594).
|
||
`ci-image.yml` tags with `git rev-parse --short HEAD` under `fetch-depth: 1`, and that shallow clone
|
||
holds few enough objects that git always abbreviates to **7**. A full local clone abbreviates to **8**,
|
||
so the natural command prints one character too many:
|
||
|
||
```bash
|
||
git rev-parse --short HEAD # 8 chars in a full clone — WRONG, no such registry tag
|
||
git rev-parse --short=7 HEAD # 7 chars — what ci-image.yml publishes. Use this.
|
||
```
|
||
|
||
An 8-char pin names the *right commit* but *no existing image*: it satisfies a resolve-and-compare
|
||
check, then every `container:` job dies at image-pull with `manifest unknown`, which reads like a
|
||
registry outage rather than a one-character pin error. `ci-image-pin` therefore checks the pin's
|
||
**length** as an invariant separate from its correctness, and prints the exact tag to use.
|
||
|
||
> **Caveat worth knowing before you trust the 7:** `ci-image.yml` still tags with a plain `--short`,
|
||
> whose length git *auto-scales* to the object count. 7 is therefore an empirical property of today's
|
||
> shallow clone, not an enforced invariant — if that count ever crosses git's threshold, the publisher
|
||
> emits 8, the correct pin becomes 8, and the gate's hardcoded 7 goes permanently red demanding a tag
|
||
> with no image behind it. Making the publisher emit `--short=7` is tracked as ersatztv#597.
|
||
|
||
`ci-image.yml` triggers on pushes **to `main`** touching `docker/ci/**`, `workflow_dispatch` on any
|
||
branch, and a weekly Monday 05:00 UTC cron (base-image security updates; Gitea registers `schedule`
|
||
only from `main`). It runs on
|
||
`ubuntu-latest` — it was on `small` until server-management#639, where "docker-only" was found to be
|
||
a poor proxy for "small": this is a full buildx of the .NET toolchain image, the heaviest job in that
|
||
lane. Like `docker-build.yml`, it needs BuildKit's inline `http = true` for the HTTP
|
||
registry. Renovate tracks the Dockerfile's image pins (`dockerfile` manager, see `renovate.json`).
|
||
|
||
**Three container-specific gotchas** — worth knowing if you add a job or a step:
|
||
- **`sh`, not bash, is the default shell inside a `container:`.** act_runner runs steps as
|
||
`sh -e {0}` (dash) because it can't assume bash exists in an arbitrary image — even though ours
|
||
has it. Every bashism (`set -o pipefail`, arrays, `shopt`, `mapfile`) then dies instantly with
|
||
`set: Illegal option -o pipefail`. `docker-build.yml` therefore declares a workflow-level
|
||
`defaults: run: shell: bash`. **If you add a workflow with containerized jobs, do the same** —
|
||
outside a container the shell defaults to bash, so this failure only appears once you containerize
|
||
and it looks nothing like a shell problem (it surfaced as the `migrations` job dying in 0.13s).
|
||
- `actions/checkout` clones as root into a mounted workspace, which trips git's *"detected dubious
|
||
ownership"* guard and breaks every `git` call in a step. Fixed in the Dockerfile with
|
||
`git config --global --add safe.directory '*'`.
|
||
- *(defensive, not load-bearing)* The ffmpeg base sets `ENTRYPOINT ["ffmpeg"]` because it ships as an
|
||
ffmpeg CLI, so the Dockerfile resets `ENTRYPOINT`/`CMD`. act overrides the entrypoint anyway
|
||
(`entrypoint=["/bin/sleep" "10800"]`), so this is belt-and-braces for anyone running the image by
|
||
hand — unlike the two above, which are real.
|
||
|
||
⚠️ **A REBASE invalidates the pin.** The pin must equal the short sha of the commit that touched
|
||
`docker/ci/**`, and a rebase **rewrites that commit's sha** — so `ci-image-pin` goes red on a branch
|
||
that was green before, with a pin that still resolves to a real (now-orphaned) commit and an image
|
||
that still exists in the registry.
|
||
|
||
### Publishing from a branch is a dispatch, not a push (ersatztv#744)
|
||
|
||
**A branch push of this file's committed form no longer publishes anything.** `ci-image.yml`'s
|
||
`push` trigger is `branches: [main]`, because Gitea resolves a `push` workflow's definition from the
|
||
pushed branch — so an unfiltered trigger meant any branch push touching those paths executed that
|
||
branch's own YAML, unreviewed, on a docker-capable runner holding the credential that writes
|
||
`ersatztv:prod` and the toolchain image five `container:` jobs run. Credential scoping
|
||
(`ci.actions-credential-scoping`) bounds what such a job may *do*; it never bounded whether it runs.
|
||
|
||
**Read that heading exactly as narrow as it is written.** The filter is loaded from the pushed ref
|
||
like every other line in the file, so a branch that *deletes* it re-enables the route in one edit.
|
||
What is closed is the **drive-by** case: an ordinary push of a legitimate `docker/ci` change
|
||
publishing an image nobody asked for, with no deliberate act anywhere in the loop. Against someone
|
||
who intends to run their own YAML this is not a boundary at all, and citing it as one is the mistake
|
||
to avoid — see #853, which settled its DISPATCH row; the `v*` tag-push and `pull_request:` rows are
|
||
live in #885.
|
||
|
||
**What this does not close — beginning with this file's own filter, then sorted by REF CLASS
|
||
rather than by which keyword a trigger carries.**
|
||
`docker-build.yml` has `branches: [main]`, so a keyword test passes it; it also has `tags: ['v*']`
|
||
and `pull_request:`, both head-resolved and both reaching the same registry credential. A tag push
|
||
is outside `release.main-direct-push-disabled`, so `git push origin <commit>:refs/tags/v99.9.9` runs
|
||
that commit's `docker-build.yml` and can publish `ersatztv:prod` — no dispatch needed. Separately,
|
||
`ci-image.yml`, `docker-build.yml`, `dependency-scan.yml` and `renovate.yml` all carry an
|
||
unrestricted `workflow_dispatch`, which loads its definition from the selected ref. The filter here
|
||
removes the *drive-by* route, where merely pushing a branch was enough; it is not a boundary against
|
||
a malicious writer.
|
||
|
||
**That remaining class was probed and ACCEPTED in #853 — read this before proposing a fix for it.**
|
||
Gitea 1.27.1 has no way to restrict `workflow_dispatch` by ref (the API takes `ref` as a required
|
||
free-form string) and no protected-environment concept to gate a secret behind — zero of its 308
|
||
documented API paths mention "environment", and the config file the running server names in its own
|
||
`--config` sets only `ENABLED` and `DEFAULT_ACTIONS_URL` under `[actions]`. More to the point,
|
||
restricting dispatch would close nothing: the `pull_request:` row above is cheaper than any
|
||
dispatch, because it runs **attacker-authored YAML**, which can name any secret in the repo store —
|
||
not only the ones the committed workflows reference. That is what makes it cover `renovate.yml`'s
|
||
`RENOVATE_TOKEN` without dispatching `renovate.yml` at all. (`dependency-scan.yml` references no
|
||
secrets at all.) Full reasoning and the probe: `ci.workflow-dispatch-ref-unrestricted`.
|
||
|
||
Six jobs in `docker-build.yml` used to hold `REGISTRY_PASSWORD` on that route, two of them required
|
||
contexts. Since ersatztv#885 **no job on the `pull_request` route names any stored secret**
|
||
(`ci.pr-route-carries-no-stored-credential`): the toolchain image is pulled through the registry's
|
||
anonymous bearer-token flow and the commit-status API is read unauthenticated, leaving the PAT to
|
||
`build`, which the route cannot reach. The invariant is held by
|
||
`scripts/tests/test_workflow_persist_credentials.py::test_no_PULL_REQUEST_route_job_names_a_STORED_secret`,
|
||
which derives the population from the git index by the predicate *every job of a
|
||
`pull_request`-triggered workflow that names a `secrets.*`* — never a list, and never "every
|
||
`container:` job" either, which names five of the six because `toolchain-preflight` is
|
||
container-free. The `v*` tag-push row is closed against the bot by `release.tag-protection-v-star`.
|
||
|
||
What that does **not** close, and must not be read as closing: `REGISTRY_PASSWORD` is still in the
|
||
repo's Actions store, and head-supplied YAML can still name it, `RENOVATE_TOKEN` or
|
||
`SERVERMGMT_DEPLOY_KEY` in a job of its own. "Push a branch, open a PR" still reaches every secret in
|
||
the store with **no act outside the ordinary contribution flow**. Bounding the store needs
|
||
per-environment secret scoping, which Gitea 1.27.1 does not have. What is removed is the routine
|
||
materialisation of a write-capable credential into six PR-run environments — blast radius, not the
|
||
route.
|
||
|
||
**A `v*` tag push does not fire `ci-image.yml`.** There is no `tags:` key, and a `branches:` filter
|
||
is compared against a branch ref — so a release cut no longer republishes the toolchain image as a
|
||
side effect. The exact matcher semantics are unprobed; the observable claim is the one stated.
|
||
|
||
The capability removed is real and is **not** assumed away — it is replaced by `workflow_dispatch`,
|
||
which Gitea loads from the branch you select. The constraint to plan around is that `ci-image.yml`
|
||
tags `git rev-parse --short HEAD`, i.e. **the branch HEAD at dispatch time**, not the commit that
|
||
touched `docker/ci`. So the branch-side two-step is:
|
||
|
||
1. Push the branch with the `docker/ci/**` change as **HEAD**. Anything that changes how the image is
|
||
built — a `ci-image.yml` edit to build-args, Dockerfile path or platforms — must be **in that
|
||
commit or below it**, because the dispatch builds HEAD's whole tree. Commit it afterwards
|
||
and the published image was built by the *old* recipe while the pin and `expected` both name the
|
||
`docker/ci` commit, so `ci-image-pin` is green over an image the merged recipe would not produce.
|
||
2. Dispatch `ci-image.yml` on that branch (Actions → *Build CI Toolchain Image* → *Run workflow* →
|
||
pick the branch). It publishes `:<short sha of that HEAD>`; `:latest` stays put, because only
|
||
`main` may move it.
|
||
3. Commit the pin bump in `docker-build.yml` on top. HEAD moves, but nothing needs republishing —
|
||
`expected` is the `docker/ci` commit, which the pin now names.
|
||
|
||
After a rebase, redo all three: the sha is new, so the image must be republished under it. That is the
|
||
loop ersatztv#445 paid twice when `main` moved mid-review.
|
||
|
||
> **Probed on this instance 2026-08-27**, because the whole branch-side path rests on it. Dispatched
|
||
> `ci-image.yml` on `fix/744-ci-image-branch-filter` at `43b1e45` (run 2340, `conclusion: success`).
|
||
> Gitea bound the job to `head_branch: fix/744-ci-image-branch-filter` / `head_sha: 43b1e45b5554…`,
|
||
> so it loads both the definition and the tree from the selected non-default ref, and the run
|
||
> published `ersatztv-ci:43b1e45` — it tags the branch HEAD at dispatch time, as described above.
|
||
> `:latest` still resolved to `sha256:7baf403c…`, the digest it carried before the dispatch. Note the
|
||
> limit of that last check: `docker/ci` was unchanged, so the rebuild was content-identical and the
|
||
> new `:<sha>` carries the same digest as `:latest`. It therefore shows `:latest` did not CHANGE, and
|
||
> cannot by itself distinguish "not pushed" from "pushed the same digest" — the `refs/heads/main`
|
||
> guard in the workflow is what makes that decision.
|
||
|
||
The cheapest way to avoid the rebase loop entirely is still to **land a toolchain-image change on its
|
||
own, before** the work that consumes it, so the consuming branch never carries the `docker/ci` commit
|
||
through a rebase.
|
||
|
||
> **`docker/ci/**` has no cheap-edit path, comments included.** Editing any byte there re-points
|
||
> `ci-image-pin`'s `expected` at the editing commit, so the five `container:` pins go stale and a
|
||
> blocking job fails — a one-line comment fix costs a full publish-and-repin, and a rebase charges it
|
||
> again. Ride prose fixes in that directory with the next genuine toolchain bump. The TWO stale
|
||
> publish instructions still in `docker/ci/Dockerfile` — the `PLAYWRIGHT_VERSION` bump comment, and
|
||
> the rebase warning's "re-dispatching does not help", which is now the opposite of the documented
|
||
> recovery — are #854, deferred on exactly these grounds.
|
||
|
||
**A change that lives only in `ci-image.yml` publishes nothing.** That file is no longer one of its
|
||
own trigger paths, nor one of `ci-image-pin`'s `expected` paths. Those two lists must otherwise name
|
||
the same sources, and `scripts/tests/test_ci_image_paths_pin_agreement.py` holds them to it — it
|
||
derives both from the workflow documents and compares them for set equality, so editing one alone
|
||
reddens `script-tests`. That job is not a required context, so the red does not block server-side; it
|
||
denies the merge-consent auto-grant through the combined status. **Read that guard before taking the
|
||
reversal described next**: it models a `<dir>/**`-against-`<dir>` pair only, so putting
|
||
`ci-image.yml` back into both lists is a file-against-file pair it refuses, and would need a file arm
|
||
added there first (ersatztv#855). That pairing is a **decided
|
||
tradeoff, not a necessity** — keeping the file in `expected` still works, since the branch dispatch
|
||
can publish the ci-image.yml commit itself and the pin then matches. What it costs is the reason it
|
||
went: that route charges a full ~2 GB publish and a five-pin bump for *every* edit to the file, a
|
||
comment included, and a rebase charges it again. That price reverted the first attempt (#697) and is
|
||
why this change was landed alone — also a priced decision, not a rule. The chosen side pays the
|
||
opposite risk instead, below.
|
||
The consequence to live with: if you change **how** the image is built (build-args, Dockerfile path,
|
||
platforms) without touching `docker/ci/**`, CI keeps running an image built by the previous recipe
|
||
and nothing goes red. **Make the change alongside a `docker/ci/**` edit** — a comment bump in the
|
||
Dockerfile is enough. That is not one option of two: publishing after the merge and then pinning
|
||
does not work, because `expected` is the last `docker/ci` commit, so a pin naming the workflow-only
|
||
commit fails `ci-image-pin` and leaving the pin where it is keeps consuming the old image. The
|
||
`docker/ci` touch is what gives the new recipe a commit the pin is allowed to name.
|
||
|
||
### When the pinned tag disappears
|
||
|
||
⚠️ **An immutable PIN is a promise about what we consume, not about what the registry keeps.** It
|
||
means the jobs never follow a floating tag like `:latest`, so a fresh toolchain push cannot change
|
||
what today's CI runs. It does not promise the tag will still EXIST — nor, strictly, that the tag's
|
||
content is frozen: `ci-image.yml` tags `git rev-parse --short HEAD`, so a `workflow_dispatch` or a
|
||
weekly `no-cache` run at the same HEAD republishes that same `:<sha>` from a rebuilt image. Those are
|
||
three different claims, and existence is the one that is not ours to make: the registry belongs to
|
||
server-management, and an owner-level Gitea *package cleanup rule* there (`keep_count` 15,
|
||
`remove_days` 1, `remove_pattern` `.*`, and a `keep_pattern` that no 7-hex sha can match) deletes any
|
||
sha tag once 15 newer versions of the package exist. `ci-image.yml` publishes a new `:<sha>` weekly
|
||
and on every push to `main` touching `docker/ci/**` (plus every branch dispatch), while the pin only
|
||
moves when a human bumps it — so a pin ages toward eviction on its own. That is what happened between 2026-08-11 and
|
||
2026-08-13 (ersatztv#772): the tag vanished, and every `container:` job — **both required contexts
|
||
included** — died at image pull, before running a step, with
|
||
|
||
```
|
||
Error response from daemon: failed to resolve reference ".../ersatztv-ci:<pin>": not found
|
||
```
|
||
|
||
buried in each job's log. Nothing said "your toolchain image is gone", so the natural first reading
|
||
was "my diff broke the build", and that is where the review time went. The durable fix is
|
||
registry-side and is tracked in **timothy/server-management#842**; until it lands, assume any pin
|
||
older than a couple of weeks can evaporate.
|
||
|
||
**How firm that cause is, since it decides whether you go looking further.** The rule and its nightly
|
||
execution are directly observed; the specific deletion is not, because Gitea hard-deletes package
|
||
versions with no audit row. What ties them is the same rule's fingerprint on the sibling `ersatztv`
|
||
package — every `:<sha>` older than the 15-slot window gone, every `keep_pattern` tag kept back to
|
||
`26.3.1`. Reproduce both halves on the Gitea host (LXC 119, `192.168.1.95`):
|
||
|
||
```bash
|
||
# on the Gitea host: the rule itself
|
||
sqlite3 /var/lib/gitea/data/gitea.db 'select * from package_cleanup_rule;'
|
||
# from anywhere: that the cleanup task is scheduled and has been running (schedule/prev/exec_times)
|
||
curl -s -u user:pass 'http://192.168.1.95:3000/api/v1/admin/cron?limit=50' \
|
||
| jq '.[] | select(.name == "cleanup_packages")'
|
||
```
|
||
|
||
(Do **not** reach for `journalctl -u gitea | grep ExecuteCleanupRules` — that identifier reaches the
|
||
log only via slow-query warnings, so an empty grep on a healthy host would read as "the rule never
|
||
ran", which is the inverse of what it means.)
|
||
|
||
If a pin disappears again *after* #842 changes that rule, treat this cause as refuted rather than
|
||
re-applying it — something else is deleting tags.
|
||
|
||
**Detection.** `docker-build.yml::toolchain-preflight` (`scripts/ci-toolchain-image-resolves.sh`)
|
||
resolves every pin in `docker-build.yml` against the registry on every run and fails with a message
|
||
that names the tag. It is container-free by necessity — a job consuming the missing image could not
|
||
run to report it — and deliberately **not** a `needs:` of the five jobs it diagnoses: the container
|
||
jobs already fail fast, so gating them would tax every green run to speed up a rare red one.
|
||
|
||
**Everything it cannot establish is a FAILURE, not a warning**, and the arms are worth knowing
|
||
because they send you to different places:
|
||
|
||
| Answer | Job | Message says |
|
||
|---|---|---|
|
||
| HTTP 200 with a manifest body | green | resolves |
|
||
| HTTP 404 | **red** | `IS GONE` — rebuild the tag (recovery above) |
|
||
| HTTP 200, body is not a manifest | **red** | something is answering for the registry (proxy, login page) |
|
||
| 401 / 403 that survives a bearer the run actually obtained | **red** | the registry `refused an ANONYMOUS read ... even after a Bearer token was obtained` — about the PACKAGE: check that the `ersatztv-ci` package is still PUBLIC. It is linked to no repository (measured 2026-09-05: every version reports `"repository": null`), so this is the package's own visibility and NOT `timothy/ersatztv`'s |
|
||
| 401 whose token leg ANSWERED and yielded no bearer — no `Www-Authenticate` at all, a challenge naming no realm, or a token endpoint answering with no token | **red**, asked once | `could NOT OBTAIN an anonymous pull token ... after 1 token-leg attempt(s)` — about the registry's TOKEN ENDPOINT, not the package's visibility. The endpoint said something, so asking again cannot change it |
|
||
| 401 whose token endpoint could not be REACHED, or answered 5xx | **red** after `ETV_CI_ATTEMPTS` token-leg tries | the same message, naming the attempts it actually made. An endpoint that said nothing is the same transport blip a flaky manifest read gets to survive — the two legs of one read must not have opposite flake tolerances when a red here denies a merge |
|
||
| 403 on the FIRST read, before any token leg — `probe` enters it on a `401` only | **red** | `refused an ANONYMOUS read ... and NO TOKEN WAS EVER REQUESTED` — it reports what the RUN did (never followed this answer as a challenge), not what the registry sent. About ACCESS to the registry: visibility, or something answering for it |
|
||
| anything else (5xx, unreachable, no `curl`) | **red** after `ETV_CI_ATTEMPTS` tries | `could NOT VERIFY` — check the registry's health, NOT the pin |
|
||
|
||
**The refusal rows say only what actually ran**, and that is the point of wording them apart
|
||
rather than a nicety: a message naming a step the run skipped is evidence for a diagnosis nobody
|
||
performed. So the arm branches on `token` first — a bearer was obtained — then on whether the token
|
||
leg was attempted at all, and only then reports the never-asked case; and the token-leg message
|
||
carries its own attempt count rather than leaving the reader to infer one from the retry policy.
|
||
|
||
Those refusal rows and the `could NOT VERIFY` row are the ones worth defending, because warning
|
||
on them and exiting 0 is the natural way to write this check and it is wrong: a missing `curl`, a
|
||
moved registry and a DNS change all land in the last of them, and a green-with-a-warning job is
|
||
indistinguishable from a healthy pin forever after. The
|
||
unknown arm retries first (`ETV_CI_ATTEMPTS`, default 3, `ETV_CI_RETRY_SECONDS` apart) so an ordinary
|
||
registry blip does not redden a PR — that pause is what makes failing on unknown affordable, and
|
||
shortening it silently trades this guard for flake. **An unreachable token endpoint is in that
|
||
class, not in the refusal class**, and is retried on the same budget: the split is by what the
|
||
endpoint SAID, never by which leg of the read it happened on.
|
||
|
||
**It is not a `needs:` of anything, but it is not consequence-free either.** The merge-consent hook
|
||
reads the PR's **combined** status and denies on a non-`success` combined state (a `skipped` context
|
||
counts as green, ersatztv#593; an advisory red does not, ersatztv#598), so a red preflight blocks the
|
||
merge exactly like any other red job. "Advisory" would be the wrong word for
|
||
it — what it does not do is *skip* the jobs it diagnoses.
|
||
|
||
**Recovery, without needing CI to be healthy.** The tag names a commit, and that commit still builds
|
||
the same image, so the fastest fix is to republish the *same* tag by hand — no PR, no pin bump, no
|
||
green CI required, and every open branch recovers at once. Run this on a host with docker and this
|
||
registry in `insecure-registries` (bumblebee `192.168.1.99` or jazz `192.168.1.29`):
|
||
|
||
```bash
|
||
repo=$(pwd) # keep the CURRENT checkout: the pin commit predates
|
||
# the preflight script and the verify step below
|
||
pin=$(grep -oE 'ersatztv-ci:[0-9a-f]+' .gitea/workflows/docker-build.yml | cut -d: -f2 | sort -u)
|
||
git worktree add /tmp/etv-toolchain "$pin" # the pin IS the commit's short sha
|
||
cd /tmp/etv-toolchain
|
||
|
||
# The registry is HTTP-only and BuildKit does NOT inherit the daemon's insecure-registries, so the
|
||
# `docker-container` driver (anything created by `docker buildx create`) will try HTTPS and fail.
|
||
# Either build on the default `docker` driver — `docker buildx use default` — or give the container
|
||
# driver the same inline config ci-image.yml passes it:
|
||
# [registry."192.168.1.95:3000"]
|
||
# http = true
|
||
prev_builder=$(docker buildx inspect 2>/dev/null | awk '/^Name:/{print $2; exit}')
|
||
docker buildx use default # needs the containerd image store to --push;
|
||
# both named hosts have it (checked 2026-08-22)
|
||
docker login 192.168.1.95:3000 -u timothy
|
||
docker buildx build --platform linux/amd64 --provenance=false \
|
||
-f docker/ci/Dockerfile -t "192.168.1.95:3000/timothy/ersatztv-ci:$pin" --push .
|
||
|
||
cd "$repo" && git worktree remove /tmp/etv-toolchain
|
||
[ -n "$prev_builder" ] && docker buildx use "$prev_builder" # leave the builder as you found it
|
||
```
|
||
|
||
Then confirm the tag resolves before re-running anything — the preflight script does exactly this
|
||
check and takes no arguments. Run it from the CURRENT checkout, not the pin worktree, which is why
|
||
`$repo` is kept above:
|
||
|
||
```bash
|
||
scripts/ci-toolchain-image-resolves.sh
|
||
```
|
||
|
||
**What this rebuild does and does not restore.** It restores a *working* toolchain at that tag, built
|
||
from that commit's `docker/ci` — not a bit-identical copy of what was deleted: the base image tags
|
||
and the apt/NodeSource packages the Dockerfile pulls are mutable, so a rebuild picks up whatever they
|
||
point at today. That is the same exposure the weekly `no-cache` cron has by design. Prefer this over
|
||
the two-step above whenever the pin is *missing* rather than *stale*: the two-step exists to move the
|
||
pin to a NEW image, and running it here would leave the repo pinning a different sha for no reason.
|
||
|
||
The push path was exercised against this registry on 2026-08-22 — a throwaway `docker push` of a
|
||
13 MB image to `timothy/etv-772-recovery-probe:probe1` from bumblebee, `HEAD /v2/.../manifests/probe1`
|
||
→ `200`, then `DELETE /api/v1/packages/timothy/container/etv-772-recovery-probe/probe1` → `204` and
|
||
the manifest read back `404`. Re-run that shape against a scratch package name to re-establish it;
|
||
what it establishes is the auth + HTTP-registry push path, not the toolchain build itself.
|
||
|
||
**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/**` (since
|
||
ersatztv#744 that path alone — `ci-image.yml` is deliberately not part of it), if that pin isn't
|
||
exactly 7 chars long (see above), 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 `:<sha>`** in `container.image` — so a
|
||
Renovate base bump would otherwise publish a new image, test the *old* one, and merge with the
|
||
Dockerfile disagreeing with the pin. A red `ci-image-pin` means: dispatch `ci-image.yml` on the
|
||
branch to publish the new
|
||
`:<sha>`, then update all five pins to it.
|
||
|
||
**What it is and isn't worth.** Measured honestly (ersatztv#390): the image saves ~15–40s per job
|
||
(`setup-dotnet` is 8–19s, `setup-node` 2–5s cached, the two tool installs ~9s) plus the 110s
|
||
apt-ffmpeg step — roughly 3–8% of runtime. It is **not** where CI time goes; see the lane table above
|
||
(queue wait, server-management#604) and ersatztv#398 (742s of redundant compilation). Its durable
|
||
value is prod-identical ffmpeg, a pinned/consistent toolchain, and making jobs runner-agnostic — the
|
||
last is what allowed the lane rebalance.
|
||
|
||
## Dockerfile notes (`docker/Dockerfile`)
|
||
|
||
- Base image: **`192.168.1.95:3000/timothy/ersatztv-ffmpeg:8.1.2`** (our Gitea fork of
|
||
the archived `ghcr.io/ersatztv/ersatztv-ffmpeg`). FFmpeg 8 base image work landed in
|
||
ersatztv-ffmpeg#4; app-side compatibility work landed in ersatztv#9.
|
||
- Copies `Directory.Build.props`, `Directory.Build.targets`, `Directory.Packages.props`,
|
||
`global.json`, `.editorconfig` before `dotnet restore` so the image build uses the same
|
||
MSBuild config, central package versions, SDK pin, and analyzer severities as local/CI
|
||
builds (it previously copied only `*.sln`). `Directory.Packages.props` is **required**
|
||
here: under Central Package Management the csproj carry no inline versions, so the
|
||
image's restore fails (`NU1015`) without the central manifest.
|
||
- amd64-only (the runner/build host is x86_64). No arm32/arm64, no DMG/exe artifacts, no GHCR/DockerHub.
|
||
- **openapi-generator jar layer ordering** (ersatztv#190): the `wget` for the
|
||
openapi-generator-cli jar runs *before* the `COPY` of `ErsatzTV/wwwroot/openapi/`, so the
|
||
~30MB download layer is cached independently of the openapi spec. Previously the jar was
|
||
downloaded after that `COPY`, so any PR touching the spec (e.g. `v1.json`) busted the
|
||
download layer too and re-fetched the jar on every such change. Codegen itself still runs
|
||
after the spec `COPY`, since it needs both the jar and the spec files.
|
||
|
||
## Dependency management (Central Package Management + scans)
|
||
|
||
**Central Package Management (CPM)** — package versions live in a single repo-root
|
||
`Directory.Packages.props` (`ManagePackageVersionsCentrally=true`); the per-project
|
||
csproj reference packages by name only (no `Version=`). One source of truth, atomic
|
||
one-line bumps, and cross-project version drift is structurally impossible. To add or
|
||
change a dependency, edit the `<PackageVersion>` entry centrally — never put a `Version=`
|
||
back on a `<PackageReference>` (that trips `NU1008`). The Docker build must copy this file
|
||
before restore (see Dockerfile notes). The `.mcp/` vendored tool (gitignored, not in the
|
||
solution) keeps inline versions via a **local-only** `.mcp/Directory.Packages.props`
|
||
opt-out (`ManagePackageVersionsCentrally=false`). (ersatztv#14)
|
||
|
||
**NuGet audit** — .NET 10 runs NuGet audit on restore. Several projects set
|
||
`TreatWarningsAsErrors=true`, so vulnerable transitive packages failed the build.
|
||
`Directory.Build.props` demotes low/moderate/high advisories (NU1901-1903) to warnings
|
||
and promotes NU1904 (critical) to an error in **every** project via `WarningsAsErrors`.
|
||
The advisories that prompted this were resolved in ersatztv#8 (NCalcSync→6.x; SQLitePCLRaw
|
||
bundle 3.x) and ersatztv#314 (**Microsoft.OpenApi 2.0.0→2.7.5**, GHSA-v5pm-xwqc-g5wc High —
|
||
direct-pinned in `ErsatzTV.csproj` over the 2.0.0 that `Microsoft.AspNetCore.OpenApi` +
|
||
`Scalar.AspNetCore` pull transitively; the SQLitePCLRaw override pattern; regenerates the
|
||
OpenAPI doc byte-identically). The NU1901-1903 demotion is **kept by design**: criticals (NU1904)
|
||
still hard-block, while low/moderate/high advisories surface as warnings + via the weekly scan and
|
||
Renovate security PRs, rather than breaking unrelated PRs the moment a new transitive
|
||
advisory drops.
|
||
|
||
**Scheduled vulnerability scan** — `.gitea/workflows/dependency-scan.yml` runs weekly
|
||
(cron `0 6 * * 1`) + on `workflow_dispatch`: `dotnet list package --vulnerable
|
||
--include-transitive` over the **full** solution (incl. Scanner, which the image build
|
||
strips). `dotnet list` exits 0 even with findings, so the step (`bash -euo pipefail`)
|
||
greps for the "has the following vulnerable packages" marker and fails the run if present.
|
||
Detection only — it surfaces advisories on a schedule, a Gitea-native stand-in for
|
||
Dependabot; it does **not** open update PRs (that's Renovate — server-management#484).
|
||
Gitea registers `schedule` triggers only from the default branch, so the cron starts
|
||
after merge to `main`; use `workflow_dispatch` to run on demand. It went **green** once
|
||
ersatztv#8 cleared the NCalcSync/SQLitePCLRaw advisories — a red run now means a **new**
|
||
advisory has appeared. (ersatztv#14, ersatztv#8)
|
||
|
||
**Renovate (automated update PRs)** — `.gitea/workflows/renovate.yml` runs self-hosted
|
||
[Renovate](https://docs.renovatebot.com) weekly (cron `0 3 * * 1`) + on `workflow_dispatch`,
|
||
as a `renovate/renovate:43` container job on the shared act_runner. This is the *proposing*
|
||
layer the scan above deliberately omits: it opens grouped dependency-update PRs and
|
||
OSV-driven vulnerability-fix PRs against `main`, and maintains a **Dependency Dashboard**
|
||
issue listing the full backlog. Config is the repo-root `renovate.json` — managers `nuget`
|
||
(via CPM), `github-actions`, and `dockerfile` (scoped to the built `docker/Dockerfile`; it reads the
|
||
HTTP-only Gitea registry for the `ersatztv-ffmpeg` base via a `RENOVATE_HOST_RULES` host rule —
|
||
`insecureRegistry` + registry read creds, set in the workflow env, not the committed config). The
|
||
docker-compose manager is unused (repo compose files are `build:`-only). Auth: a dedicated
|
||
**`renovate` Gitea bot** (Write
|
||
collaborator) via repo Actions secrets `RENOVATE_TOKEN` (bot PAT) + `GH_COM_TOKEN` (no-scope
|
||
github.com PAT for changelogs — named `GH_`, **not** `GITHUB_`, a prefix Gitea reserves).
|
||
**Patch** bumps to test/dev-only packages (NUnit\*, NSubstitute, Shouldly, coverlet,
|
||
`Microsoft.NET.Test.Sdk`, `Testably.Abstractions*`, threading analyzer) **auto-merge** once
|
||
the `Build & test (.NET)` check passes — branch protection on `main` requires that context;
|
||
everything else is manual review (ersatztv is prod-bearing). Range-pinned packages (e.g. EF
|
||
Core `[9.0.x,10)`) are respected — no v10 jump. PR volume is throttled (`prConcurrentLimit`
|
||
5 + `config:recommended`'s `prHourlyLimit` 2); tick a dashboard checkbox or raise the limits
|
||
to drain faster. `workflow_dispatch` defaults to a safe **dry run**. Cross-repo rollout
|
||
tracked in server-management#484. (server-management#484)
|
||
|
||
## Security scanning — black-box DAST + SAST (`scripts/security-scan.sh`, ersatztv#314)
|
||
|
||
Every other security check we run is **in-ecosystem / white-box** — SonarAnalyzer, NetArchTest, the
|
||
adversarial fork + Codex review passes, the api-docs/format/decisions CI gates, `dotnet list package
|
||
--vulnerable` — so they share our blind spots. `scripts/security-scan.sh` is the **out-of-ecosystem,
|
||
black-box** complement and a **#197 exit criterion** (HARD GATE before remote exposure): it drives the
|
||
running product from outside our C#/review stack.
|
||
|
||
- **What it does.** Boots a **throwaway** container from the image under test (fresh empty config volume;
|
||
never the deployed prod/test container — the authenticated *active* scan sends attack payloads to write
|
||
endpoints), reads the generated machine key, and runs an **authenticated OWASP ZAP** API scan
|
||
(`zap-api-scan.py`) that imports the static `/openapi/v1.json` so it exercises **every** declared
|
||
`/api/v1` operation, injecting `X-Api-Key` on every request via a ZAP replacer rule so it reaches the
|
||
`[RequiresAuthentication]` + `RequireKeyForReads` surface (not just the `/app` shell an unauthenticated
|
||
spider sees). Then a **semgrep** SAST cross-check (`p/security-audit` + `p/secrets` + `p/csharp`). The
|
||
container is torn down on exit.
|
||
- **Where/when.** Runs on the **docker host** (jazz — the Mac has no docker), like
|
||
`migration-smoke.sh`: `scripts/security-scan.sh [IMAGE] [PORT]` (defaults `…:latest` / `8411`). It is a
|
||
**manual release-gate**, deliberately **not** a per-PR CI job — it needs docker + a booted image, takes
|
||
several minutes, and is noisy (expect to tune, not take raw). The continuous layer is the per-PR
|
||
white-box gates + the weekly `dependency-scan`; this is the per-release black-box pass. Re-run it each
|
||
release and before any change to the exposure posture.
|
||
- **Exit-code contract (ersatztv#338).** `zap-api-scan.py`'s raw exit code is NOT a simple pass/fail — it
|
||
conflates a clean run with a warnings-only run unless you know its wrapper contract: **0** clean (no
|
||
FAIL or WARN alerts), **2** WARN-only (triage required, but **not** release-blocking), **1** FAIL (at
|
||
least one FAIL-level alert — release-blocking), **124** the script's own `timeout` wrapper killed a
|
||
hung post-scan cleanup (the report written before the hang is still usable — triage it), any other
|
||
code means the scanner/tool itself errored (not a scan result at all). `scripts/security-scan.sh`
|
||
encodes this in `classify_zap_exit()` and prints an unambiguous `==> ZAP result: <PASS|WARN|FAIL|
|
||
TIMEOUT|TOOL ERROR> ...` line; the script's own exit status reflects that classification (0 for
|
||
clean/WARN, 1 for FAIL/timeout/tool-error) rather than ZAP's raw code, so a warnings-only run no longer
|
||
reads as a failed scan. Found when the v26.8.0 release scan (#335) returned raw exit 2 for a report with
|
||
`FAIL-NEW: 0` and two known/expected warning classes — the shell result looked like a failure though the
|
||
release gate had actually passed. Run `scripts/security-scan.sh --selftest` for a docker-free regression
|
||
check of the classification logic.
|
||
- **Triage.** Triage each WARN/FAIL finding false-positive vs real. Real, in-scope, go-live-blocking
|
||
findings get fixed (e.g. the security headers from the #319 baseline; the Microsoft.OpenApi pin above);
|
||
LAN-expected noise (Private-IP disclosure) is revisited only for genuine remote exposure. nuclei
|
||
(template-based CVE fingerprinting) is an optional third pass — deferred while its template fetch is
|
||
blocked in the runner env (pre-seed a template volume to add it); ZAP covers the DAST baseline and
|
||
semgrep the SAST, so it is not on the critical path.
|
||
|
||
## Static analysis & formatting
|
||
|
||
**Analyzers** — `Directory.Build.props` enables the SDK analyzers at `latest-All` and turns on
|
||
`Microsoft.VisualStudio.Threading.Analyzers` for every centrally managed project.
|
||
`Directory.Build.targets` also references
|
||
**Roslynator**, **SonarAnalyzer.CSharp**, **Meziantou.Analyzer**, and **AsyncFixer** repo-wide (versions
|
||
central via CPM). All analyzer package references are guarded on `ManagePackageVersionsCentrally`, so the
|
||
gitignored `.mcp` tool—which deliberately uses inline package versions—does not inherit versionless
|
||
references.
|
||
They are introduced **incrementally** (ersatztv#15). `eng/analyzers/sdk-all-suggestion.globalconfig`
|
||
enumerates the .NET 10 SDK `All` inventory at `suggestion`; this exact-ID baseline is necessary because
|
||
the SDK's generated `latest-All` severities outrank `.editorconfig` bulk settings. `.editorconfig` keeps
|
||
the threading and curated-pack baselines at `suggestion`. Diagnostics remain visible to IDEs and
|
||
`dotnet format analyzers`, but do not create a wall of failures (a direct `latest-All` trial activated
|
||
455 existing errors in the TWAE projects).
|
||
|
||
**Promotion is the enforcement** — set a reviewed rule to `warning` in `.editorconfig` and append its ID
|
||
to the central `WarningsAsErrors` list in `Directory.Build.props`. The explicit list makes the rule block
|
||
in every project, including test projects that do not otherwise use TWAE. On a major SDK upgrade,
|
||
regenerate the checked-in SDK baseline from `analysislevel_<major>_all.globalconfig`, preserve SDK `none`
|
||
entries, and review newly introduced rules before accepting the snapshot.
|
||
|
||
Promoted rules are recorded here so the blocking subset stays intentional and reviewable:
|
||
- **Sonar `S3981` — `warning` + `WarningsAsErrors`** (ersatztv#15): rejects collection-count comparisons that are constant
|
||
regardless of collection size. Its first finding exposed `Workers.Count >= 0`, which permanently
|
||
classified scheduled memory releases as busy and skipped the intended aggressive idle collection.
|
||
|
||
- **StyleCop.Analyzers is intentionally excluded**: its latest stable (1.1.118) crashes (`AD0001`) on
|
||
C# `record` declarations, and its rules overlap the existing `.editorconfig`/Roslynator. Revisit via
|
||
the record-compatible `1.2.0-beta` only if specifically wanted.
|
||
- **The former Blazor `.razor` caveat is retired**: Blazor removal deleted the Razor sources and their
|
||
temporary Sonar `NoWarn` list. The `.razor`/`.cshtml` suggestion scopes remain in `.editorconfig` only
|
||
as a defensive default if server-rendered view code is ever reintroduced.
|
||
|
||
**Formatting** — the inherited tree still contains legacy UTF-8 BOM/whitespace debt, so the standing
|
||
policy is **format as you touch**, not a mass rewrite (ersatztv#311). The Husky pre-commit hook and
|
||
the blocking `format` CI job both run `dotnet format whitespace . --folder --verify-no-changes
|
||
--include <changed .cs>` — scoped to the files the commit/PR touches. Untouched legacy files remain
|
||
outside the gate; `.gitattributes` pins line endings. A one-time full-tree normalization remains a
|
||
separate, unmade decision.
|
||
|
||
*Why `whitespace . --folder`, not the full `dotnet format <sln>` (ersatztv#469)* — the gate only
|
||
needs to enforce `.editorconfig` **whitespace** (indent/EOL/trailing/final-newline) and **charset**
|
||
(no UTF-8 BOM). The old recipe (`dotnet format ErsatzTV.sln --no-restore --verify-no-changes
|
||
--include`) loaded the entire ~10-project MSBuild workspace and built a Roslyn compilation per
|
||
project *before* checking a single file — `--include` narrows *which* files are checked, never what
|
||
gets loaded. Measured whole-solution `dotnet format` ran **~480s locally**; folder mode runs in
|
||
**~0.5s** and needs no `dotnet restore` (the NuGet-cache + Restore steps were removed from the job).
|
||
`--folder` treats the tree as a plain folder of files, skipping MSBuild/Roslyn entirely, and still
|
||
reads `.editorconfig`. Verified **non-vacuous**: it exits non-zero on an injected trailing-whitespace
|
||
line (`error WHITESPACE`) and on a prepended UTF-8 BOM (`error CHARSET`), and exits 0 on a clean file.
|
||
**No coverage was lost**: the full `dotnet format` gate did **not** enforce the style/analyzer pass
|
||
either — a probe injecting a `warning`-severity naming violation (`local_constants` not `ALL_UPPER`)
|
||
passed the *full* solution format (exit 0): the only `.editorconfig` rule above `:suggestion`/`:none`
|
||
severity is that one naming rule, and naming violations have no `dotnet format` batch code-fixer, so
|
||
`--verify-no-changes` reports no change regardless of severity. The analyzers that **must** block
|
||
(`NU1904`, `S3981`) are enforced at *compile time* via
|
||
`WarningsAsErrors` in `Directory.Build.props`, not by this job. Devs fix a violation with `dotnet
|
||
format whitespace . --folder --include <files>` (the full `dotnet format ErsatzTV.sln --include
|
||
<files>` is a superset and also works).
|
||
|
||
## Migration integrity (EF Core, both providers)
|
||
|
||
`TvContext` (`ErsatzTV.Infrastructure/Data/TvContext.cs`) has **two** migration sets — one per
|
||
provider project: `ErsatzTV.Infrastructure.Sqlite/Migrations` and
|
||
`ErsatzTV.Infrastructure.MySql/Migrations`, each with its own `TvContextModelSnapshot`. **A model
|
||
change needs a migration in BOTH.** Add them with `scripts/add-migration.sh <Name>` (runs the EF CLI
|
||
for each provider). The EF CLI pattern (provider selected by the post-`--` arg, which `Startup`
|
||
reads as the `provider` config key):
|
||
|
||
```
|
||
dotnet ef <cmd> --context TvContext --startup-project ErsatzTV \
|
||
--project ErsatzTV.Infrastructure.{Sqlite|MySql} -- --provider {Sqlite|MySql}
|
||
```
|
||
|
||
The **`migrations` job** in `docker-build.yml` runs on every push/PR and, for **each** provider:
|
||
|
||
1. `dotnet ef migrations has-pending-model-changes` — fails if an entity changed without a matching
|
||
migration (**model drift**), so a forgotten migration can't merge.
|
||
2. `dotnet ef database update` against a **fresh empty DB** — applies *all* migrations in order and
|
||
fails on any broken/un-orderable one.
|
||
|
||
- **SQLite** (the prod provider) uses a throwaway file (`ETV_CONFIG_FOLDER=$(mktemp -d)`); no service
|
||
needed. Validated: 787 migrations → 139 tables.
|
||
- **MySql** uses `ServerVersion.AutoDetect`, which **connects at config time**, so the job needs a
|
||
reachable server — provided by a `services: mysql:8.4` container (the act_runner uses Docker
|
||
execution with an auto-created per-job network — service reachable as `mysql:3306` (the old
|
||
bumblebee runner pinned network `downloadswarm`; relocated in server-management#570)). Connection
|
||
string via `MySql__ConnectionString` (→ config key `MySql:ConnectionString`). Validated: 305
|
||
migrations → 137 tables. It's an **independent gate** (not yet a `needs:` of the image build) so
|
||
the new MySql-service dependency can't block image builds until it's proven; promote it to a
|
||
required check once stable.
|
||
|
||
**Caveat — non-transactional operations**: some migrations (e.g. SQLite `PRAGMA foreign_keys`) run
|
||
outside a transaction and warn at startup; they can't be rolled back mid-migration, so review such
|
||
migrations carefully (this is part of what motivated the apply-to-fresh check before the prod
|
||
cutover, server-management#481).
|
||
|
||
**Resilience — the MySql apply is retried (concurrent-runner contention, not a model bug)**: both
|
||
runners (`ci-runner` VM 127 + `bumblebee-runner`) serve `ubuntu-latest`, and when two migration jobs
|
||
land on the **same host at once** (common when several PRs push together), each spins its own
|
||
`mysql:8.4` service container and they starve each other — producing intermittent `Command Timeout
|
||
expired` **or** mid-replay `MySqlEndOfStreamException` (dropped connection) on the MySql
|
||
apply-to-fresh-DB step. This is pure infra flakiness — `has-pending-model-changes` (the actual model
|
||
check) still passes, and the *same commit* passes on a quieter host. The job hardens against it two
|
||
ways: the connection string sets `DefaultCommandTimeout=300` (up from MySqlConnector's 30s default),
|
||
and the apply is wrapped in a **3× retry** that resumes from `__EFMigrationsHistory` (EF commits each
|
||
migration in its own transaction, so an interrupted one rolls back and the retry continues). A *real*
|
||
migration failure fails deterministically on every attempt, so the retry never masks it. If a run
|
||
still flakes past the retry, re-trigger (Gitea has no rerun API on this version — push, or the run
|
||
drains); don't treat a lone MySql-apply red as a code problem without checking the failure mode.
|
||
|
||
### Migration-on-prod-copy smoke — release path (`scripts/migration-smoke.sh`, ersatztv#315)
|
||
|
||
The `migrations` job above only proves a migration is **well-formed against a fresh, empty DB**. It
|
||
can't prove it applies cleanly to the **accumulated prod SQLite** — real row volume, historical values,
|
||
and the post-migration data steps ErsatzTV runs on startup: `DatabaseMigratorService` (a
|
||
`BackgroundService`) applies pending migrations, then `DbInitializer.Initialize` + `PopulatePathHashes`
|
||
(an `UPDATE` over the real `MediaFile` table). A migration green on a fresh DB can still fail or corrupt
|
||
on prod, and today you'd only find out mid-deploy after the container recreates.
|
||
|
||
`scripts/migration-smoke.sh` rehearses it on a **throwaway copy** of the latest prod backup — it never
|
||
touches the live DB:
|
||
|
||
```
|
||
scripts/migration-smoke.sh --image <ref-about-to-be-promoted> [--db <backup.sqlite3>] [--timeout 180]
|
||
```
|
||
|
||
It copies the backup into a temp config dir, boots the new image against it (`ETV_CONFIG_FOLDER`), and
|
||
**gates PASS on the `Done applying database migrations` log line** — not merely on HTTP readiness, since
|
||
the migrator runs concurrently with Kestrel, so the web server can serve before/while migrations run.
|
||
FAIL = the container exits before finishing, a migration exception appears in the logs, migrations
|
||
don't finish within `--timeout`, or the app won't serve `/iptv/channels.m3u` afterwards. The smoke
|
||
container, the DB copy, and the temp dir are always torn down on exit (the ErsatzTV image runs as root,
|
||
so cleanup deletes its root-owned config files from inside a throwaway root container — otherwise each
|
||
run would leak the multi-hundred-MB copy). Exit 0 = clean, 1 = migration/boot failure, 2 = usage error.
|
||
|
||
- **`--db` default**: the newest `ersatztv.sqlite3` under `$ETV_BACKUP_DIR`
|
||
(default `~/downloadswarm/ersatztv-backups` — where the host-side pre-deploy backup hook writes
|
||
timestamped snapshots). Pass `--image` = the version tag about to be promoted.
|
||
- **Where it runs**: it's meant to run **on the docker host** as a **Komodo pre-deploy step** (which
|
||
already produces the backup — see "Cutting a release" and the #553 pre-deploy backup caveat), so a
|
||
bad migration aborts the promote before the live container recreates. Wiring it into that hook is a
|
||
server-management concern (cross-repo — this repo owns the script + docs, server-management owns the
|
||
Komodo hook). Until wired, run it by hand before cutting a migration-bearing release.
|
||
- Validated live 2026-07-12: `:latest` against a copy of the 283 MB prod backup → migrations applied
|
||
cleanly, app booted and served, temp dir removed.
|
||
|
||
## Pre-commit hooks (web/)
|
||
|
||
The repo uses **husky** git hooks (installed via `web/`'s **lint-staged** + npm) to catch
|
||
lint/format/type/API-drift errors locally, before they reach CI. Because the git root and
|
||
the npm project dir differ (monorepo: no root `package.json`, the JS/TS project lives
|
||
entirely in `web/`), the wiring is:
|
||
|
||
- `husky` + `lint-staged` are devDependencies of `web/package.json` (not a root package —
|
||
there isn't one).
|
||
- The committed hook scripts live at the repo root: `.husky/pre-commit`, `.husky/pre-push`,
|
||
`.husky/commit-msg`.
|
||
- `web/package.json`'s `prepare` script (`cd .. && husky`) runs on `npm install` inside
|
||
`web/` and points git at the repo-root `.husky` dir (`git config core.hooksPath
|
||
.husky/_` — the `_` subdir is husky's generated internal dir, gitignored via its own
|
||
`.husky/_/.gitignore`; only the hook scripts themselves are committed). This works
|
||
because npm keeps `web/node_modules/.bin` on `PATH` for the `prepare` script even after
|
||
it `cd ..`s to the repo root (which husky's init requires — it hard-checks for `.git`
|
||
in the *current* directory).
|
||
|
||
**The four hooks:**
|
||
|
||
1. **`pre-commit`** — (a) `cd web && npx lint-staged`: runs `eslint --fix` on staged
|
||
`web/src/**/*.{ts,tsx}` files, then a project-wide `npm run typecheck` (`tsc -b` isn't
|
||
file-scoped, so it runs the full check, but only when a `.ts`/`.tsx` file is staged);
|
||
(b) back at the repo root, if any **`*.cs`** files are staged, `dotnet format
|
||
ErsatzTV.sln --verify-no-changes --include <staged .cs>` — a formatting violation
|
||
blocks the commit. The .cs step is **skipped entirely when no .cs is staged**, so
|
||
web-only commits don't pay the sln-load cost; when it does run it's scoped to the staged
|
||
files (~6-7s wall in practice, dominated by the workspace load); (c) **H3** (ersatztv#303) —
|
||
refuses a staged **root-level `*.png`** (`git diff --cached --name-only | grep -E '^[^/]+\.png$'`),
|
||
belt-and-suspenders with the `.gitignore` screenshot rule so a forced `git add -f` still can't land
|
||
a review/debug screenshot at the repo root. Nested `*.png` (real assets) pass; (d) **decision
|
||
lifecycle validator** (ersatztv#521, supersedes the ersatztv#303 H9 append-only mechanic) — runs
|
||
`.claude/hooks/decisions-guard.sh` (no args; a fail-open shim around
|
||
`scripts/decisions_validate.py`), the structural checks over the working tree (metadata
|
||
well-formedness, one active record per key, reciprocal links). It has no base/head here, so the
|
||
body-diff/no-vanish checks it also knows about are skipped locally and only run in the CI
|
||
`decisions lifecycle` job, which has a PR base to diff against.
|
||
2. **`pre-push`** — CI-parity gate: `cd web && npm run check:api && npm run lint && npm run
|
||
typecheck && npm run build`. `check:api` guards generated-OpenAPI drift
|
||
(`ErsatzTV/wwwroot/openapi/v1.json` → `web/src/api/generated/v1.d.ts`); the full
|
||
lint/typecheck/build catch a staged change that breaks an *unstaged* file (lint-staged
|
||
only sees staged files). Any failure blocks the push.
|
||
3. **`commit-msg`** — enforces the CLAUDE.md protocol: the message must carry a
|
||
`Co-Authored-By:` trailer, else the commit is rejected (merge commits are exempt, detected
|
||
via `git rev-parse --verify MERGE_HEAD`). The decision-lifecycle check lives in `pre-commit`
|
||
(above), not here — the `Decisions-Edit:` trailer is read from the commit message, but only by the CI
|
||
`decisions lifecycle` job's body-diff step (`range` mode over the PR's merge-base diff), which is
|
||
the only place a base/head range exists to diff against.
|
||
|
||
- **Worktree/subdir gotcha**: git exports `GIT_DIR` (and friends) while running hooks. In a
|
||
worktree or any subdir, an explicit `GIT_DIR` makes nested `git` commands mislocate the
|
||
working tree — `pre-push`'s `check:api` (`git diff --exit-code`, run from `web/`) then
|
||
silently reports "no diff" and lets drift through. `pre-push` therefore `unset`s
|
||
`GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE` first. (`pre-commit`'s `.cs` collection uses
|
||
`git diff --cached`, index-vs-HEAD, which needs only `GIT_DIR` and is unaffected.)
|
||
- **Practical effect**: a fresh `web/` `npm install` (after cloning or pulling this change)
|
||
installs all four hooks automatically — no separate setup step. Commits that touch only
|
||
non-`web/`, non-`.cs` files skip linting/formatting (lint-staged no-ops with nothing to
|
||
run, the `.cs` step is skipped).
|
||
|
||
## Registry
|
||
|
||
Gitea Packages, HTTP-only at `192.168.1.95:3000`. the `ci-runner` VM's Docker daemon (192.168.1.127) has it as an
|
||
insecure-registry (server-management#172; runner relocated off bumblebee in #570). Images: `192.168.1.95:3000/timothy/ersatztv:<tag>`.
|
||
|
||
## Test / prod environments
|
||
|
||
Container/compose wiring lives in **server-management** (project boundary): test
|
||
`ersatztv-test` on 8410 (`:latest`), prod `ersatztv` on 8409 (`:prod`). See
|
||
server-management#481 for the full spec (registry pull on the docker host, volumes, Jellyfin
|
||
isolation for test, Watchtower/manual promotion).
|
||
|
||
## Retired upstream workflows
|
||
|
||
The upstream `.github/workflows/` (`ci.yml`, `docker.yml`, `artifacts.yml`,
|
||
`release.yml`, `pr.yml`, `issue-stale.yml`) were removed — they targeted
|
||
GHCR/DockerHub + Azure/Apple signing and called reusable workflows at dead
|
||
`ersatztv/ersatztv@main` paths, and ran as noise (incl. a daily stale-issue cron) on
|
||
the Gitea runner. Upstream is archived, so there are no future merges to preserve them
|
||
for. The dead `.github/dependabot.yml` and `FUNDING.yml` (upstream-pointed) were also
|
||
removed.
|
||
|
||
## Known follow-ups
|
||
|
||
- Pin third-party actions to commit SHAs (currently floating major tags cloned from
|
||
github.com at runtime) — low priority for a homelab; tracked informally.
|