fix(772,792): name the missing toolchain image; stop a refusal leaving a verdict comment (#825)
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 13s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 14s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 11s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 12s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 4m11s
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 13s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 14s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 11s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 12s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 4m11s
This commit was merged in pull request #825.
This commit is contained in:
@@ -114,6 +114,27 @@ env:
|
||||
MSBUILDDISABLENODEREUSE: "1" # MSBuild worker nodes exit with the build instead of lingering
|
||||
|
||||
jobs:
|
||||
# Answers "is the toolchain image still there?" in ONE place, so a deleted pin does not read as
|
||||
# five broken jobs and a broken diff (ersatztv#772). Deliberately container-free and deliberately
|
||||
# NOT a `needs:` of the jobs it diagnoses — see scripts/ci-toolchain-image-resolves.sh for both
|
||||
# decisions and for the cleanup-rule root cause it cannot fix from this repo.
|
||||
toolchain-preflight:
|
||||
name: CI toolchain image resolves
|
||||
runs-on: small
|
||||
steps:
|
||||
- name: Checkout
|
||||
uses: actions/checkout@v4
|
||||
- name: Resolve the pinned toolchain tag in the registry
|
||||
env:
|
||||
ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }}
|
||||
run: |
|
||||
"${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark resolve
|
||||
scripts/ci-toolchain-image-resolves.sh
|
||||
- name: Assert every expected step executed (ersatztv#756)
|
||||
run: >-
|
||||
scripts/ci-step-ran.sh assert
|
||||
--always resolve
|
||||
|
||||
test:
|
||||
name: Build & test (.NET)
|
||||
runs-on: ubuntu-latest
|
||||
|
||||
+135
-4
@@ -206,10 +206,14 @@ so a PR claiming many proofs costs proportionally more than the rest of the lane
|
||||
`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. `scan` is the lane member to think hardest about before changing anything
|
||||
here: it is the only one that lives in `docker-build.yml` rather than `pr-checks.yml`, so the only
|
||||
one that runs on a **tag push**, and the only one whose failure does not merely redden a status but
|
||||
**skips `build`** — an OOM or a wedge there yields no release image at all.
|
||||
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
|
||||
@@ -1576,6 +1580,133 @@ a follow-up commit. That is the same two-step below, just re-run after the rebas
|
||||
avoid it entirely is 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.
|
||||
|
||||
### 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 touching `docker/ci/**` or the workflow file, 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 after 1–2s 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 | **red** | the credentials were rejected — fix the secrets |
|
||||
| anything else (5xx, unreachable, no `curl`) | **red** after `ETV_CI_ATTEMPTS` tries | `could NOT VERIFY` — check the registry's health, NOT the pin |
|
||||
| `ETV_REGISTRY_AUTH` unset, malformed, or either half empty | **red**, before any query | an absent secret interpolates to `":"`, which is not a credential |
|
||||
|
||||
The last two rows 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
|
||||
there, 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.
|
||||
|
||||
**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
|
||||
ETV_REGISTRY_AUTH=user:pass 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/**` or
|
||||
|
||||
@@ -136,6 +136,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:<version>` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) |
|
||||
| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) |
|
||||
| `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) |
|
||||
| `release.verdict-writes-status-before-comment` | `scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant. | 2026-08-22 | [link](records/release/verdict-writes-status-before-comment.md) |
|
||||
| `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `"<n> day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) |
|
||||
| `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) |
|
||||
| `scan.getoraddfolder-db-lookup` | `ILibraryRepository.GetOrAddFolder` resolves the existing folder via a DB query on `(LibraryPathId, Path)`, not the caller's `LibraryPath.LibraryFolders` in-memory navigation, since that navigation is only eager-loaded on the local scan path and is null on remote (Jellyfin) callers. | 2026-07-20 | [link](records/scan/getoraddfolder-db-lookup.md) |
|
||||
|
||||
@@ -7,7 +7,7 @@ supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark <key>` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always <keys> --gated <keys>`, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step''s `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships. The ban is enforced on the RELEASE PATH itself, not only in review (#767): a `scan` job runs the PyYAML-based ban test and `build` lists it in `needs:`, so a delimiter means `build` never runs and no image is published. A guard STEP inside `build` was tried first and is wrong — a step cannot protect the job it publishes from, and "my body has no opener so I cannot be dropped" is circular when only the PR-only test enforces that. The pytest in `script-tests` remains, but it is `on: pull_request` and not a required context, so it alone left the tag path unchecked.'
|
||||
signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py`, `scripts/tests/test_ci_release_path_scan_job.py` · issues: #756, #751, #684, #767'
|
||||
mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`. All three REFUSE rather than falling back to a reusable name. The third was warn-and-default until its presence was measured: grepping a log for the variable NAME proves nothing, and inferring it from the absence of a stderr warning proves nothing either (stderr capture was itself unestablished), so `assert` was made to print `Marker identity: job=… run=… attempt=… (from the runner)` on STDOUT and the answer was read off run 1916 for both required jobs. That line is retained as standing evidence. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn). The release-path `scan` job (#767) runs the existing PyYAML-based ban test rather than a second implementation, so there is no drift surface; `scripts/tests/test_ci_release_path_scan_job.py` holds the WIRING instead — that `build` needs it AND that `build`''s own `if:` carries no `always()`/`!cancelled()`/`failure()` (which would downgrade the `needs:` edge to mere ordering), that `scan` carries no job-level `if:` (one excluding the tag push restores the hole, one skipping the job skips `build` too) and is not advisory at STEP or JOB level, and that its own run bodies are delimiter-free. Its load-bearing test is an EXECUTION PROBE, not a shape assertion: it runs the scan step''s real `run:` body with the full env the runner would give it (workflow, job AND step `env:` tiers) 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 and lost repeatedly — from `echo`ing the command to `PYTEST_ADDOPTS` one env tier up — so do NOT replace the probe with cheaper checks about the command. Two tiers cannot be reached from inside pytest at all and are handled differently: a step writing to `$GITHUB_ENV` is BANNED by test, and repo-root pytest configuration (`pytest.ini` `addopts`, `pytest_collection_modifyitems`) can deselect any test including the guards, so the positive control is a SHELL step — `scripts/ci-prove-ban-detects.sh` poisons the checked-out workflow in the REAL checkout, re-runs the ban test, and vouches ONLY for the ban test''s `build` parametrisation failing — any other outcome (exit 5 from a total deselect, exit 2 from a collection error, an unrelated test failing) is a REFUSAL, not a pass, because each weaker reading was a live bug in an earlier draft and the deselection disarm it exists to catch exits 5 rather than 1. That script is itself positively controlled — `test_the_PROOF_SCRIPT_itself_refuses_when_the_ban_is_deselected` and `test_the_PROOF_SCRIPT_refuses_when_the_WRONG_test_fails` cover the two refusal branches a disarm actually lands on, each verified by making that branch alone unreachable — since it was for a while the one guard exercised only on the happy path. The third branch (pytest passing outright) has no control and does not need one: neutering it falls through to the exit-code branch, which still refuses. A copy-based proof is not equivalent: it does not inherit the repo-root config a disarm would live in. Its steps carry markers and a trailing assert of their own, verified by the same drop-each-key-in-turn behavioural pattern. CARVE-OUT: the "fresh container is the primary protection, keying is defence in depth" reasoning above does NOT cover `scan` — it has no `container:` and runs on `small`, where RUNNER_TEMP is the shared host /tmp, so for that job the run-id/attempt keying is the ONLY protection. Residual: a single-job re-run that does not increment GITHUB_RUN_ATTEMPT would find the prior attempt''s marker file and the assert would pass even with the pytest step dropped. Identity on that lane was measured, not assumed — run 1929 printed `Marker identity: job=scan run=1929 attempt=1 (from the runner)`.'
|
||||
mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`. All three REFUSE rather than falling back to a reusable name. The third was warn-and-default until its presence was measured: grepping a log for the variable NAME proves nothing, and inferring it from the absence of a stderr warning proves nothing either (stderr capture was itself unestablished), so `assert` was made to print `Marker identity: job=… run=… attempt=… (from the runner)` on STDOUT and the answer was read off run 1916 for both required jobs. That line is retained as standing evidence. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn). The release-path `scan` job (#767) runs the existing PyYAML-based ban test rather than a second implementation, so there is no drift surface; `scripts/tests/test_ci_release_path_scan_job.py` holds the WIRING instead — that `build` needs it AND that `build`''s own `if:` carries no `always()`/`!cancelled()`/`failure()` (which would downgrade the `needs:` edge to mere ordering), that `scan` carries no job-level `if:` (one excluding the tag push restores the hole, one skipping the job skips `build` too) and is not advisory at STEP or JOB level, and that its own run bodies are delimiter-free. Its load-bearing test is an EXECUTION PROBE, not a shape assertion: it runs the scan step''s real `run:` body with the full env the runner would give it (workflow, job AND step `env:` tiers) 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 and lost repeatedly — from `echo`ing the command to `PYTEST_ADDOPTS` one env tier up — so do NOT replace the probe with cheaper checks about the command. Two tiers cannot be reached from inside pytest at all and are handled differently: a step writing to `$GITHUB_ENV` is BANNED by test, and repo-root pytest configuration (`pytest.ini` `addopts`, `pytest_collection_modifyitems`) can deselect any test including the guards, so the positive control is a SHELL step — `scripts/ci-prove-ban-detects.sh` poisons the checked-out workflow in the REAL checkout, re-runs the ban test, and vouches ONLY for the ban test''s `build` parametrisation failing — any other outcome (exit 5 from a total deselect, exit 2 from a collection error, an unrelated test failing) is a REFUSAL, not a pass, because each weaker reading was a live bug in an earlier draft and the deselection disarm it exists to catch exits 5 rather than 1. That script is itself positively controlled — `test_the_PROOF_SCRIPT_itself_refuses_when_the_ban_is_deselected` and `test_the_PROOF_SCRIPT_refuses_when_the_WRONG_test_fails` cover the two refusal branches a disarm actually lands on, each verified by making that branch alone unreachable — since it was for a while the one guard exercised only on the happy path. The third branch (pytest passing outright) has no control and does not need one: neutering it falls through to the exit-code branch, which still refuses. A copy-based proof is not equivalent: it does not inherit the repo-root config a disarm would live in. Its steps carry markers and a trailing assert of their own, verified by the same drop-each-key-in-turn behavioural pattern. CARVE-OUT: the "fresh container is the primary protection, keying is defence in depth" reasoning above does NOT cover the container-free jobs — `scan`, and `toolchain-preflight` since #772. Neither has a `container:` and both run on `small`, where RUNNER_TEMP is the shared host /tmp, so for those jobs the run-id/attempt keying is the ONLY protection. The MEMBERSHIP is the part that rots: a further container-free job added without joining this list silently inherits a reassurance nobody checked for it. Residual: a single-job re-run that does not increment GITHUB_RUN_ATTEMPT would find the prior attempt''s marker file and the assert would pass even with the pytest step dropped. Identity on that lane was measured, not assumed — run 1929 printed `Marker identity: job=scan run=1929 attempt=1 (from the runner)`.'
|
||||
---
|
||||
|
||||
**Why per step, when #756 proposed per job.** A job-start marker answers "did this job begin", which
|
||||
|
||||
@@ -0,0 +1,48 @@
|
||||
---
|
||||
key: release.verdict-writes-status-before-comment
|
||||
title: '2026-08-22 — the verdict STATUS is written before the verdict COMMENT, so the only reachable half-state is the safe one (#792)'
|
||||
status: active
|
||||
since: '2026-08-22'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: '`scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant.'
|
||||
signals: 'verdict comment without a status, half-posted verdict, orphaned Review-verdict comment, exit code lies, post-review-verdict exits 0 · paths: `scripts/post-review-verdict.sh`, `scripts/tests/test_post_review_verdict.py`, `.claude/hooks/pretooluse-merge-consent.sh` · issues: #792, #622, #632, #778'
|
||||
mechanics: '`scripts/tests/test_post_review_verdict.py::test_the_status_is_written_BEFORE_the_comment`, `::test_no_refusal_leaves_a_VERDICT_COMMENT_standing_in_for_the_status` (every refusal mode), `::test_every_path_that_writes_NO_STATUS_exits_non_zero`, `::test_a_failed_COMMENT_after_a_written_status_is_still_an_error`'
|
||||
---
|
||||
|
||||
The script has two writes and they are not equal. The **status** is the gate — a required context on
|
||||
`main`, bound to one sha. The **comment** is the artifact a human reads, and the merge hook's
|
||||
condition (c). Writing the comment first meant that every refusal between the two writes left a PR
|
||||
carrying `Review-verdict: MERGEABLE @ <head>` with no status behind it: an artifact that reads as
|
||||
granted consent, produced by the very run that refused to grant it. The refusals are correct and are
|
||||
not what changed (`ci.verdict-write-retarget-fence` — the fence must keep refusing when it cannot
|
||||
bind safely); what changed is which write survives a partial failure.
|
||||
|
||||
Ordering settles it without a rollback, and rollback is the option not taken: deleting or annotating
|
||||
the orphaned comment needs a Gitea call, and the refusals it would compensate for are frequently
|
||||
*caused* by Gitea being unreachable, so the compensating write is unavailable exactly when it is
|
||||
needed. Ordering costs nothing and cannot fail to apply.
|
||||
|
||||
The two surviving half-states are asymmetric, and that asymmetry is the whole justification:
|
||||
|
||||
- comment, no status → the hook's condition (c) classifies a positive verdict, the operator sees
|
||||
consent, and only the required check stands between that and a merge. Fail-open in appearance.
|
||||
- status, no comment → the hook has no verdict for this head to classify, which it resolves as
|
||||
**ask**. Fail-closed, visible, and cured by re-running the command. The qualification, because
|
||||
the hook reads the whole comment history rather than this run's write: if a positive verdict for
|
||||
the SAME head already exists, condition (c) is satisfied by it and the hook may grant — which is
|
||||
correct, since that comment covers this exact sha. A stale or negative verdict yields deny. So
|
||||
`ask` is the outcome when the current head has no verdict comment, and nothing here can produce a
|
||||
grant over a head no one reviewed.
|
||||
|
||||
**#792's premise about the exit code was wrong, and is corrected rather than repeated.** The issue
|
||||
reported the script printing its refusal and exiting 0. Re-measured on the tree that carries #632's
|
||||
fence, every no-status path exits NON-ZERO (`die` exits 1, the usage path 2, a failing `jq` its own
|
||||
status, a signal 128+n) — eight refusal modes are now driven through the real entry point
|
||||
and asserted, and those assertions pass against the predecessor as well, which is how we know the
|
||||
defect was never in the script. The observed 0 came from the invocation around it (a pipeline reports
|
||||
its last command's status, not the script's). The exit-code contract is asserted anyway: it was true
|
||||
by convention, held by one shared `die` helper, and nothing had ever executed it.
|
||||
|
||||
Read with `release.verdict-status-check` (why the status, not the comment, is the gate) and
|
||||
`release.review-verdict-gate` (the comment convention itself).
|
||||
@@ -178,6 +178,7 @@ recorded as unexamined rather than as cleared.
|
||||
| `scripts/ci-peak-anon.sh` | nothing (samples container memory) | TOOLING | NONE | — |
|
||||
| `scripts/ci-prove-ban-detects.sh` | the release path, if the delimiter ban is disarmed | GUARD | NONE | — |
|
||||
| `scripts/ci-step-ran.sh` | the two required contexts, on a dropped step | GUARD | MUTATION | `test_ci_dropped_step_guard.py::test_dropping_ANY_single_step_FAILS_the_guard` |
|
||||
| `scripts/ci-toolchain-image-resolves.sh` | the `toolchain-preflight` job, when the pinned CI toolchain image has been deleted from the registry | GUARD | MUTATION | `test_ci_toolchain_image_resolves.py::test_MUTATION_a_deleted_tag_is_reported_as_a_failure` |
|
||||
| `scripts/decisions_validate.py` | the `decisions-guard` job, on a lifecycle fault | GUARD | MUTATION | `test_decisions_validate.py::test_main_actually_CALLS_the_wing_scan` |
|
||||
| `scripts/e2e-functional.sh` | the Functional E2E job, on a failed HTTP contract assertion | GUARD | NONE | — |
|
||||
| `scripts/e2e-local.sh` | nothing (boots a local instance) | TOOLING | NONE | — |
|
||||
@@ -196,6 +197,7 @@ recorded as unexamined rather than as cleared.
|
||||
| `scripts/tests/test_ci_dropped_step_guard.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_ci_image_pin_population.py` | the `script-tests` job, when a container job loses its pin | GUARD | MUTATION | `test_ci_image_pin_population.py::test_a_single_job_losing_its_pin_is_DETECTED` |
|
||||
| `scripts/tests/test_ci_release_path_scan_job.py` | the `script-tests` job, on a weakened release-path scan job | GUARD | NONE | — |
|
||||
| `scripts/tests/test_ci_toolchain_image_resolves.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_decisions_lib.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_decisions_validate.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_guard_inventory.py` | the `script-tests` job, on an unclassified guard or a stale proof ref | GUARD | MUTATION | `test_guard_inventory.py::test_the_inventory_covers_exactly_the_guards_that_exist` |
|
||||
@@ -235,7 +237,7 @@ clause-provable and the entry is regraded.
|
||||
|
||||
## What the numbers say
|
||||
|
||||
36 guards, 6 tooling scripts, 19 proof files. **14 guards carry a mutation proof; 6 are
|
||||
37 guards, 6 tooling scripts, 20 proof files. **15 guards carry a mutation proof; 6 are
|
||||
behaviour-only; 16 have none.** These figures are asserted against the table by
|
||||
`test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against
|
||||
a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it,
|
||||
|
||||
@@ -95,7 +95,7 @@ classifications differ; otherwise the strictest applies and the Note names the e
|
||||
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is new: both comparisons were guarded by `[ -n "$x" ] &&`, so a well-formed 2xx body that merely omitted the field made the check a no-op and the status was posted having confirmed nothing — found by cold review on #778 and regression-tested against the real predecessor, since the redundant `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual: the **comment** is posted *before* the re-read, so a head that moves in between leaves a verdict comment with no status — the comment is not the gate, but the mismatch is confusing and is tracked in **#792**. |
|
||||
| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is new: both comparisons were guarded by `[ -n "$x" ] &&`, so a well-formed 2xx body that merely omitted the field made the check a no-op and the status was posted having confirmed nothing — found by cold review on #778 and regression-tested against the real predecessor, since the redundant `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). |
|
||||
| `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix, graded honestly after cold review: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases; this row previously said "any movement fails", which was stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias is not covered by anything — a force-push `H1 -> H2 -> H1` during pagination leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and no counter moves. That residual is real, unfenced, and stated here rather than papered over; closing it needs a monotonic head-mutation fence or enumeration bound to an immutable tree, neither of which exists today — tracked in **#803**, which also carries the three older contracts that still assert more than this row does. |
|
||||
| `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. |
|
||||
| `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. |
|
||||
@@ -128,6 +128,7 @@ classifications differ; otherwise the strictest applies and the Note names the e
|
||||
| `scripts/ci-peak-anon.sh` | `N/A` | Reads no live remote state — samples the runner's local cgroup `memory.stat`/`memory.peak`. |
|
||||
| `scripts/ci-prove-ban-detects.sh` | `N/A` | Reads no live remote state — mutates a local workflow copy and runs pytest against the local checkout. |
|
||||
| `scripts/ci-step-ran.sh` | `N/A` | Reads no live remote state — reads runner-supplied env vars and local marker files it wrote itself. |
|
||||
| `scripts/ci-toolchain-image-resolves.sh` — registry manifest read for the pinned toolchain tag | `UNSAFE-KNOWN` | Reads a MUTABLE identifier (a registry tag) with nothing re-checking it before the `container:` jobs pull, so a tag deleted between the preflight and the pull is reported as present. Graded `UNSAFE-KNOWN` rather than `N/A` deliberately: nothing proceeds on the strength of the read — it can only turn its own job red, which is not nothing (the merge-consent hook denies on the COMBINED status, ersatztv#598) but is not authorization either — while a stale PASS is read by a human as "the image is fine", which is an assertion about remote state this file exists to grade. The residual is bounded by what it degrades to: a stale pass leaves exactly the pre-#772 behaviour (five jobs failing at pull), never anything that proceeds on the strength of the read. The opposite error is closed by the EXIT CODE rather than by wording: an unusable credential, an unverifiable answer (after retries) and an HTTP 200 whose body is not a manifest all FAIL the job. THE TRAP, since warning on those and exiting 0 is the natural way to write this check: a missing `curl`, a moved registry or a DNS change all land there, and a green-with-a-warning job is indistinguishable from a healthy pin forever after — "the check could not run" presenting as "the pin is fine", which is precisely what this row would then be asserting falsely. |
|
||||
| `scripts/set-provider.sh` | `N/A` | Reads no live remote state — sets local `dotnet user-secrets` values. |
|
||||
| `scripts/__init__.py` | `N/A` | Empty package marker — executes nothing. |
|
||||
| `scripts/scripted-schedules/entrypoint.py` — `ScriptedScheduleApi.get_context(build_id)`, then `define_content` / `reset_playout` / `build_playout` against the same live server | `UNSAFE-KNOWN` | A genuine read-then-act over live ErsatzTV state, and the row cold review found missing when the population was still non-recursive. The context is fetched, handed to user-supplied script functions that mutate the playout, and re-fetched after a reset with nothing pinning either read — a concurrent build or edit between them is invisible. Accepted because it runs inside a single scripted-schedule build the server itself serialises per playout, and because the API exposes no version or ETag on the context to compare against; the honest bound is that the blast radius is one playout's content, reversible by rebuilding. |
|
||||
|
||||
@@ -101,12 +101,14 @@ EOF
|
||||
# message naming the variable; that is loud, instantly diagnosable, and the correct direction for a
|
||||
# required check.
|
||||
#
|
||||
# EXCEPT ON A LANE WITH NO `container:` (ersatztv#767). The `scan` job runs on `small` with no
|
||||
# container, so RUNNER_TEMP is the shared host /tmp and the keying below is the ONLY thing separating
|
||||
# runs, not defence in depth on top of a fresh filesystem. There the rerun residual DOES still exist:
|
||||
# a single-job rerun that does not increment GITHUB_RUN_ATTEMPT would find the previous attempt's
|
||||
# marker file. See the carve-out in `ci.required-job-step-execution-markers`; do not read the
|
||||
# paragraph above as covering that job.
|
||||
# EXCEPT ON A LANE WITH NO `container:` (ersatztv#767, extended by #772). `scan` and
|
||||
# `toolchain-preflight` both run on `small` with no container, so RUNNER_TEMP is the shared host /tmp
|
||||
# and the keying below is the ONLY thing separating runs, not defence in depth on top of a fresh
|
||||
# filesystem. There the rerun residual DOES still exist: a single-job rerun that does not increment
|
||||
# GITHUB_RUN_ATTEMPT would find the previous attempt's marker file. See the carve-out in
|
||||
# `ci.required-job-step-execution-markers`; do not read the paragraph above as covering those jobs.
|
||||
# The list is the property to keep current — a THIRD container-free job added without appearing here
|
||||
# inherits a reassurance that was never checked for it.
|
||||
marker_path() {
|
||||
local missing=""
|
||||
[ -n "${GITHUB_JOB:-}" ] || missing="$missing GITHUB_JOB"
|
||||
|
||||
Executable
+149
@@ -0,0 +1,149 @@
|
||||
#!/usr/bin/env bash
|
||||
# Preflight: does the PINNED CI toolchain image still exist in the registry? (ersatztv#772)
|
||||
#
|
||||
# WHY THIS EXISTS. `docker-build.yml` pins its five `container:` jobs to an immutable
|
||||
# `ersatztv-ci:<sha>`. Between 2026-08-11 and 2026-08-13 that tag was deleted from the Gitea
|
||||
# registry and every one of those jobs — including BOTH required contexts — died after 1-2s with
|
||||
#
|
||||
# Error response from daemon: failed to resolve reference "…/ersatztv-ci:<the pinned sha>": 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". This job says it in one line, in a job whose NAME says it.
|
||||
#
|
||||
# "Immutable" was taken to mean "will always exist", and those are different claims. The cause was
|
||||
# an owner-level Gitea package cleanup rule (keep_count 15, remove_days 1, remove_pattern `.*`, and
|
||||
# a keep_pattern no 7-hex sha can match), so a pinned tag is deleted once 15 newer versions of the
|
||||
# package exist. The rule lives in the registry's repo — the durable fix is
|
||||
# timothy/server-management#842 — and THIS script does not fix it. It converts a five-job pull
|
||||
# failure into one actionable message, which is all a consumer of someone else's registry can do.
|
||||
#
|
||||
# WHY IT DOES NOT GATE THE CONTAINER JOBS with `needs:`. Serialising five jobs behind a checkout +
|
||||
# one curl would tax every green run to speed up the rare red one, and the container jobs already
|
||||
# fail fast (1-2s) when the pull fails. This runs in PARALLEL: the diagnosis is present the moment
|
||||
# anyone looks, and the happy path pays nothing.
|
||||
#
|
||||
# UNKNOWN IS NOT A PASS, and this is where the first draft was wrong. It warned and exited 0 on
|
||||
# every answer that was not 200 or 404, which makes "curl is missing from this runner", "the
|
||||
# registry moved", and "DNS changed" all indistinguishable from a healthy pin — a job that is green
|
||||
# forever having checked nothing, in a file whose header claims the opposite. Unknown answers are
|
||||
# RETRIED (they are usually transient) and then FAIL. The message stays distinct from the deleted
|
||||
# case: "could not verify" and "IS GONE" send an operator to different places.
|
||||
#
|
||||
# Env (all optional except the credential; the defaults are the live values):
|
||||
# ETV_CI_REGISTRY registry host:port (default 192.168.1.95:3000)
|
||||
# ETV_CI_IMAGE_REPO package path inside the registry (default timothy/ersatztv-ci)
|
||||
# ETV_CI_WORKFLOW workflow file to read the pin from (default .gitea/workflows/docker-build.yml)
|
||||
# ETV_CI_ATTEMPTS tries per pin before an unknown becomes a failure (default 3)
|
||||
# ETV_CI_RETRY_SECONDS pause between those tries (default 5)
|
||||
# ETV_REGISTRY_AUTH user:pass — REQUIRED; the registry rejects anonymous reads with 401
|
||||
set -euo pipefail
|
||||
|
||||
registry="${ETV_CI_REGISTRY:-192.168.1.95:3000}"
|
||||
image_repo="${ETV_CI_IMAGE_REPO:-timothy/ersatztv-ci}"
|
||||
workflow="${ETV_CI_WORKFLOW:-.gitea/workflows/docker-build.yml}"
|
||||
|
||||
fail() { printf '::error::ci-toolchain-image-resolves: %s\n' "$*" >&2; exit 1; }
|
||||
|
||||
[ -f "$workflow" ] || fail "cannot read $workflow to find the toolchain pin"
|
||||
|
||||
# The same expression `pr-checks.yml::ci-image-pin` greps with, so the two cannot disagree about
|
||||
# what "the pin" is. Note it is written so THIS line cannot match itself: the character after the
|
||||
# colon here is `[`, which is not in [0-9a-f].
|
||||
pins=$(grep -oE 'ersatztv-ci:[0-9a-f]+' "$workflow" | cut -d: -f2 | sort -u || true)
|
||||
[ -n "$pins" ] || fail "no ersatztv-ci pin found in $workflow — if the grep pattern stopped matching, fix it here and in pr-checks.yml::ci-image-pin together"
|
||||
|
||||
# No credentials is NOT a pass. An unauthenticated read of this registry is a 401 for every tag,
|
||||
# present or deleted, so a run without them would report "cannot tell" for a live pin and for a
|
||||
# deleted one alike — the shape where a guard reports green having checked nothing.
|
||||
#
|
||||
# The EMPTY-halves check is the one that matters in CI and is easy to miss: an absent secret does
|
||||
# not arrive here as an unset variable. `ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ ... }}`
|
||||
# interpolates a missing secret to the empty string, so the job passes the non-empty string ":".
|
||||
# Testing only the unset case would leave the production shape uncovered.
|
||||
auth="${ETV_REGISTRY_AUTH:-}"
|
||||
[ -n "$auth" ] || fail "ETV_REGISTRY_AUTH (user:pass) is unset, so the registry cannot be queried — this check refuses to report a pass it did not establish"
|
||||
case "$auth" in
|
||||
*:*) ;;
|
||||
*) fail "ETV_REGISTRY_AUTH must be user:pass, got a value with no ':' — the registry cannot be queried and this check refuses to report a pass it did not establish" ;;
|
||||
esac
|
||||
[ -n "${auth%%:*}" ] && [ -n "${auth#*:}" ] \
|
||||
|| fail "ETV_REGISTRY_AUTH has an empty half (user or password) — this is what an ABSENT REGISTRY_USER/REGISTRY_PASSWORD secret interpolates to, not a credential. Fix the secrets rather than reading an unauthenticated 401 as could-not-tell."
|
||||
|
||||
accept='application/vnd.oci.image.manifest.v1+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json,application/vnd.docker.distribution.manifest.list.v2+json'
|
||||
attempts="${ETV_CI_ATTEMPTS:-3}"
|
||||
retry_seconds="${ETV_CI_RETRY_SECONDS:-5}"
|
||||
rc=0
|
||||
|
||||
# One GET, returning "<code> <is-a-manifest>". The body is fetched rather than a HEAD sent, because
|
||||
# HTTP 200 alone does not mean "the manifest is there": a proxy, a captive login page or an error
|
||||
# document all answer 200 with a body that is not a manifest, and a check that reads only the status
|
||||
# line reports those as "resolves". A manifest always carries `schemaVersion`, so the body is matched
|
||||
# for it — with a shell `case`, so nothing depends on jq being installed and no pipeline can invert
|
||||
# the result on a large body.
|
||||
probe() {
|
||||
local url="$1" resp code body
|
||||
# `-u` puts the credential in argv, visible to `ps` for the length of the call — and this job has
|
||||
# no `container:`, so that is the shared host. Kept because it is the shape every other curl caller
|
||||
# in scripts/ already uses (`ci-detect-already-validated.sh`, `pr-changed-files.sh`,
|
||||
# `select-queue.sh`, `issue-qualification-audit.sh`): fixing one site would leave the class intact
|
||||
# and the codebase inconsistent. The class is tracked in ersatztv#821.
|
||||
resp=$(curl -s -w '\n%{http_code}' -u "$auth" -H "Accept: $accept" "$url") || resp=""
|
||||
[ -n "$resp" ] || { printf '000 no\n'; return 0; }
|
||||
code=${resp##*$'\n'}
|
||||
body=${resp%$'\n'*}
|
||||
case "$body" in
|
||||
*'"schemaVersion"'*) printf '%s yes\n' "$code" ;;
|
||||
*) printf '%s no\n' "$code" ;;
|
||||
esac
|
||||
}
|
||||
|
||||
for pin in $pins; do
|
||||
url="http://$registry/v2/$image_repo/manifests/$pin"
|
||||
attempt=1
|
||||
while : ; do
|
||||
read -r code is_manifest <<EOF
|
||||
$(probe "$url")
|
||||
EOF
|
||||
case "$code" in
|
||||
200|404|401|403) break ;;
|
||||
esac
|
||||
# Only the unknown answers are retried: 200/404 are answers, and an auth failure will not cure
|
||||
# itself. A transient registry is the common case for the rest, and absorbing it here is what
|
||||
# lets the unknown be a FAILURE at the end rather than a warning nobody reads.
|
||||
[ "$attempt" -lt "$attempts" ] || break
|
||||
attempt=$((attempt + 1))
|
||||
sleep "$retry_seconds"
|
||||
done
|
||||
|
||||
case "$code" in
|
||||
200)
|
||||
if [ "$is_manifest" = "yes" ]; then
|
||||
printf 'ci-toolchain-image-resolves: %s/%s:%s resolves (HTTP 200, manifest present)\n' "$registry" "$image_repo" "$pin"
|
||||
else
|
||||
printf '::error::ci-toolchain-image-resolves: %s/%s:%s answered HTTP 200 with a body that is not a manifest (no schemaVersion). Something is answering for the registry — a proxy, a login page, or an error document. The pin was NOT verified.\n' \
|
||||
"$registry" "$image_repo" "$pin" >&2
|
||||
rc=1
|
||||
fi
|
||||
;;
|
||||
404)
|
||||
# The one unambiguous answer, and the outage this exists for.
|
||||
printf '::error::ci-toolchain-image-resolves: the pinned CI toolchain image %s/%s:%s IS GONE from the registry (HTTP 404). Every container: job in docker-build.yml will fail at image pull, including both required contexts, and NO diff caused it. Recovery does not need CI: rebuild that exact tag from the commit it names and push it — see docs/ci-cd.md -> "CI toolchain image" -> "When the pinned tag disappears". Root cause + the durable fix: timothy/server-management#842.\n' \
|
||||
"$registry" "$image_repo" "$pin" >&2
|
||||
rc=1
|
||||
;;
|
||||
401|403)
|
||||
# `fail` rather than `rc=1`: unlike a 404, this says nothing about the pin, and it will say
|
||||
# the same thing about every remaining one. Abandoning the loop keeps the log to one cause.
|
||||
fail "the registry rejected these credentials (HTTP $code) for $registry/$image_repo:$pin, so the pin could not be checked. Fix REGISTRY_USER/REGISTRY_PASSWORD rather than reading this as a pass."
|
||||
;;
|
||||
*)
|
||||
# NOT gone, and NOT a pass either. Deliberately worded apart from the 404 message: this sends
|
||||
# an operator to the registry's health, not to a rebuild of a tag that may be sitting there.
|
||||
printf '::error::ci-toolchain-image-resolves: could NOT VERIFY %s/%s:%s after %s attempt(s) (last answer: HTTP %s). This is not evidence the image is gone — it is evidence the check could not run, which fails rather than passing so the preflight cannot quietly become a no-op.\n' \
|
||||
"$registry" "$image_repo" "$pin" "$attempts" "$code" >&2
|
||||
rc=1
|
||||
;;
|
||||
esac
|
||||
done
|
||||
|
||||
exit "$rc"
|
||||
@@ -119,21 +119,20 @@ short=${sha:0:7}
|
||||
base_ref=$(printf '%s' "$prjson" | jq -r '.base.ref // ""')
|
||||
[ -n "$base_ref" ] || die "PR #$pr has no resolvable base branch (.base.ref) — refusing to post a verdict that cannot record what it was formed against"
|
||||
|
||||
# --- The comment (human-readable artifact + the hook's condition-(c) input). --------------------
|
||||
# The verdict line MUST start the line: the hook anchors its parser to line-start precisely so a
|
||||
# comment that merely QUOTES the template mid-sentence cannot self-approve a merge.
|
||||
body="Review-verdict: $verdict @ $short"
|
||||
[ -n "$note" ] && body="$body"$'\n\n'"$note"
|
||||
comment_payload=$(jq -n --arg b "$body" '{body:$b}')
|
||||
api_post "repos/$owner/$repo/issues/$pr/comments" "$comment_payload" >/dev/null \
|
||||
|| die "failed to post the verdict comment on PR #$pr"
|
||||
printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short"
|
||||
|
||||
# --- TOCTOU guard: refuse to green a head that stopped being head while we were posting. --------
|
||||
# --- TOCTOU guard: refuse to green a head that stopped being head since we read it. -------------
|
||||
# Without this, a commit pushed between the head read above and the status write below would inherit
|
||||
# a verdict written for its parent — reintroducing ersatztv#622 at a smaller time scale. We do NOT
|
||||
# retry against the new head: the new commit is genuinely unreviewed, and silently re-targeting the
|
||||
# verdict at it is exactly the failure this script exists to prevent.
|
||||
#
|
||||
# THE WINDOW THIS FENCES USED TO BE MUCH WIDER, and that is why the comment is now written AFTER the
|
||||
# status rather than before it (ersatztv#792). Posting the comment first meant every refusal below
|
||||
# left a PR carrying `Review-verdict: MERGEABLE @ <head>` with NO `review-verdict/h10` status — and
|
||||
# the comment is not the gate. The half-state was read by an operator as consent that had not been
|
||||
# granted. Ordering the two writes status-first makes the surviving half the SAFE half: a status
|
||||
# with no comment leaves the hook at condition (c) with nothing to classify, which is an `ask`, not
|
||||
# a grant. The refusals themselves are unchanged and must stay — see
|
||||
# `release.verdict-writes-status-before-comment`.
|
||||
# Fail CLOSED if the re-read itself fails. This used to be `sha_now=$(api_get ... | jq ...)`, where
|
||||
# `set -e` + `pipefail` aborted the script on a failed GET — implicitly, but before any status was
|
||||
# written. Folding the two reads into one variable with `|| true` would have swallowed that: both
|
||||
@@ -187,6 +186,17 @@ api_post "repos/$owner/$repo/statuses/$sha" "$status_payload" >/dev/null \
|
||||
|| die "failed to post the '$STATUS_CONTEXT' commit status on $short"
|
||||
printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short"
|
||||
|
||||
# --- The comment (human-readable artifact + the hook's condition-(c) input). --------------------
|
||||
# Written LAST, after the gating status exists (ersatztv#792). The verdict line MUST start the line:
|
||||
# the hook anchors its parser to line-start precisely so a comment that merely QUOTES the template
|
||||
# mid-sentence cannot self-approve a merge.
|
||||
body="Review-verdict: $verdict @ $short"
|
||||
[ -n "$note" ] && body="$body"$'\n\n'"$note"
|
||||
comment_payload=$(jq -n --arg b "$body" '{body:$b}')
|
||||
api_post "repos/$owner/$repo/issues/$pr/comments" "$comment_payload" >/dev/null \
|
||||
|| die "the '$STATUS_CONTEXT' status was written on $short, but the verdict COMMENT could not be posted. The merge gate needs both: it reads the comment for condition (c) and will ASK rather than auto-grant until one exists. Re-run this command once Gitea is reachable."
|
||||
printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short"
|
||||
|
||||
if [ "$state" = "failure" ]; then
|
||||
printf '\nPR #%s stays BLOCKED: %s is failing on head %s.\n' "$pr" "$STATUS_CONTEXT" "$short"
|
||||
else
|
||||
|
||||
@@ -213,6 +213,20 @@ MUTATIONS: tuple[Mutation, ...] = (
|
||||
"`test_MUTATION_disarming_the_EXIT_STATUS_gate_accepts_a_run_that_NEVER_RAN_A_TEST`; the "
|
||||
"inventory holds one ref per row, so this entry names the stronger of the two.",
|
||||
),
|
||||
Mutation(
|
||||
guard="scripts/ci-toolchain-image-resolves.sh",
|
||||
target="scripts/ci-toolchain-image-resolves.sh",
|
||||
clause=" 404)",
|
||||
replacement=" 4040)",
|
||||
proof="test_ci_toolchain_image_resolves.py::test_MUTATION_a_deleted_tag_is_reported_as_a_failure",
|
||||
granularity=CLAUSE,
|
||||
expect="a deleted tag was not reported as GONE",
|
||||
why="404 is the ONE answer that establishes the pinned toolchain image is gone; every other "
|
||||
"code means the check could not run. Both fail the job, so the EXIT CODE does not separate "
|
||||
"them and the mutation is caught by the DIAGNOSTIC instead: retargeting the arm sends the "
|
||||
"real outage down the could-not-verify path, which sends an operator to the registry's "
|
||||
"health rather than to the rebuild that fixes it.",
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -82,10 +82,12 @@ _DOC = yaml.safe_load(WORKFLOW.read_text())
|
||||
# reviewable act; a job silently losing its container block is not.
|
||||
TOOLCHAIN_JOBS = frozenset({"test", "migrations", "functional-e2e", "api-docs", "format"})
|
||||
|
||||
# `scan` and `build` deliberately run on the bare runner: `scan` is `runs-on: small` and needs only
|
||||
# python, and `build` drives docker/buildx on the host. Listed here so their ABSENCE above reads as
|
||||
# a decision rather than an oversight.
|
||||
BARE_RUNNER_JOBS = frozenset({"scan", "build"})
|
||||
# `scan`, `build` and `toolchain-preflight` deliberately run on the bare runner: `scan` is
|
||||
# `runs-on: small` and needs only python, `build` drives docker/buildx on the host, and
|
||||
# `toolchain-preflight` exists to report that the pinned toolchain image is GONE — a job that
|
||||
# consumed that image could not run to say so (ersatztv#772). Listed here so their ABSENCE above
|
||||
# reads as a decision rather than an oversight.
|
||||
BARE_RUNNER_JOBS = frozenset({"scan", "build", "toolchain-preflight"})
|
||||
|
||||
|
||||
def _jobs(doc) -> dict:
|
||||
|
||||
@@ -0,0 +1,303 @@
|
||||
"""Tests for `scripts/ci-toolchain-image-resolves.sh` (ersatztv#772).
|
||||
|
||||
The script answers one question — does the tag `docker-build.yml` pins still exist? — and the whole
|
||||
value is in *which answers it refuses to round off*. A registry read has three outcomes, not two:
|
||||
present, gone, and could-not-tell. Collapsing the third into either of the others is how a preflight
|
||||
becomes decoration, so each is driven here through the real entry point with a stubbed `curl`.
|
||||
|
||||
`test_MUTATION_a_deleted_tag_is_reported_as_a_failure` is the load-bearing one and is declared in
|
||||
`scripts/tests/mutation_manifest.py`. Note what it can and cannot turn on: since an unverifiable
|
||||
answer fails the job too, disarming the `404` arm still exits non-zero, so the EXIT CODE separates
|
||||
nothing. What the disarm destroys is the DIAGNOSTIC — the outage is reported as "could not verify",
|
||||
which sends an operator to the registry's health instead of to the rebuild that fixes it.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import time
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
SCRIPT = REPO_ROOT / "scripts" / "ci-toolchain-image-resolves.sh"
|
||||
|
||||
# Stands in for `curl -s -w '\n%{http_code}' -u <auth> -H Accept <url>`: prints a scripted body,
|
||||
# a newline and the HTTP code, and logs the call. It VALIDATES `-u` rather than ignoring it — a stub
|
||||
# that answers 200 whether or not the script authenticates would stay green if the real `-u` were
|
||||
# deleted, which is the fidelity gap that lets a test double certify a script the live registry
|
||||
# would reject on every request.
|
||||
CURL_SHIM = r"""#!/usr/bin/env python3
|
||||
import os, pathlib, sys
|
||||
|
||||
state = pathlib.Path(os.environ["STUB_DIR"])
|
||||
args = sys.argv[1:]
|
||||
url = [a for a in args if a.startswith("http")][-1]
|
||||
tag = url.rsplit("/", 1)[-1]
|
||||
auth = args[args.index("-u") + 1] if "-u" in args else ""
|
||||
with (state / "calls").open("a") as fh:
|
||||
fh.write(f"{url} auth={auth}\n")
|
||||
|
||||
# The live registry answers 401 to an anonymous read of ANY tag, present or deleted.
|
||||
user, _, password = auth.partition(":")
|
||||
if not user or not password:
|
||||
print("{}\n401", end="")
|
||||
sys.exit(0)
|
||||
|
||||
codes = dict(pair.split("=", 1) for pair in (state / "codes").read_text().split() if pair)
|
||||
code = codes.get(tag, codes.get("*", "200"))
|
||||
if code == "TRANSPORT":
|
||||
# Only the EXIT STATUS is observable: the script's `|| resp=""` discards whatever curl printed,
|
||||
# so what this reproduces is the non-zero exit, not the `\n000` real curl also emits.
|
||||
sys.exit(7)
|
||||
body = '{"schemaVersion": 2, "mediaType": "application/vnd.oci.image.manifest.v1+json"}'
|
||||
if code == "200-NOT-A-MANIFEST":
|
||||
code, body = "200", "<html><title>Sign in</title></html>"
|
||||
print(f"{body}\n{code}", end="")
|
||||
"""
|
||||
|
||||
WORKFLOW_TEMPLATE = """jobs:
|
||||
test:
|
||||
container:
|
||||
image: 192.168.1.95:3000/timothy/ersatztv-ci:{pin}
|
||||
"""
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def preflight(tmp_path):
|
||||
bindir = tmp_path / "bin"
|
||||
bindir.mkdir()
|
||||
shim = bindir / "curl"
|
||||
shim.write_text(CURL_SHIM)
|
||||
shim.chmod(0o755)
|
||||
|
||||
state = tmp_path / "state"
|
||||
state.mkdir()
|
||||
(state / "codes").write_text("*=200")
|
||||
|
||||
workflow = tmp_path / "docker-build.yml"
|
||||
workflow.write_text(WORKFLOW_TEMPLATE.format(pin="32747a0"))
|
||||
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
|
||||
env["STUB_DIR"] = str(state)
|
||||
env["ETV_CI_WORKFLOW"] = str(workflow)
|
||||
env["ETV_REGISTRY_AUTH"] = "stub-user:stub-pass"
|
||||
# The retry PAUSE is what makes failing on an unknown affordable in CI and unaffordable in a
|
||||
# test suite; the retry COUNT is behaviour, so it is kept and only the wait is removed.
|
||||
env["ETV_CI_ATTEMPTS"] = "2"
|
||||
env["ETV_CI_RETRY_SECONDS"] = "0"
|
||||
|
||||
class Handle:
|
||||
def __init__(self):
|
||||
self.env = env
|
||||
self.state = state
|
||||
self.workflow = workflow
|
||||
self.script = SCRIPT
|
||||
|
||||
def set_codes(self, mapping: dict[str, str]):
|
||||
(state / "codes").write_text(" ".join(f"{k}={v}" for k, v in mapping.items()))
|
||||
|
||||
def set_workflow_text(self, text: str):
|
||||
workflow.write_text(text)
|
||||
|
||||
def calls(self):
|
||||
log = state / "calls"
|
||||
return log.read_text().splitlines() if log.exists() else []
|
||||
|
||||
def run(self, script: Path | None = None):
|
||||
return subprocess.run(
|
||||
["bash", str(script or SCRIPT)],
|
||||
env=env,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
cwd=REPO_ROOT,
|
||||
)
|
||||
|
||||
return Handle()
|
||||
|
||||
|
||||
def test_a_pin_that_resolves_passes(preflight):
|
||||
preflight.set_codes({"*": "200"})
|
||||
result = preflight.run()
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert "32747a0 resolves" in result.stdout
|
||||
assert preflight.calls(), "the registry was never queried, so nothing was established"
|
||||
|
||||
|
||||
def test_MUTATION_a_deleted_tag_is_reported_as_a_failure(preflight):
|
||||
"""The outage of 2026-08-11..13, in one assertion.
|
||||
|
||||
Declared in `mutation_manifest.py`: replacing the `404` arm sends a deleted tag down the
|
||||
could-not-verify path, which fails the job with the wrong story — a preflight that runs, reddens,
|
||||
and still misses the only thing it was built to name.
|
||||
"""
|
||||
preflight.set_codes({"32747a0": "404"})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0, (
|
||||
"a deleted tag did not fail the preflight — the 404 arm is not load-bearing:\n"
|
||||
f"stdout={result.stdout}\nstderr={result.stderr}"
|
||||
)
|
||||
assert "IS GONE" in result.stderr, (
|
||||
"a deleted tag was not reported as GONE — the 404 arm is not load-bearing. Since an "
|
||||
"unverifiable answer now fails too, exiting non-zero no longer distinguishes 'the image is "
|
||||
"deleted' from 'the check could not run', and only this message does:\n"
|
||||
f"stderr={result.stderr}"
|
||||
)
|
||||
assert "32747a0" in result.stderr, "the message must name the tag the operator has to restore"
|
||||
assert "server-management#842" in result.stderr, "and where the durable fix lives"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("code", ["TRANSPORT", "503"])
|
||||
def test_an_unknown_answer_FAILS_and_is_not_reported_as_gone(preflight, code):
|
||||
"""The first draft warned and exited 0 here, which is how a preflight becomes a no-op.
|
||||
|
||||
A missing `curl`, a moved registry or a DNS change all land in this arm, and each would have
|
||||
been green forever. It fails — but with its own wording, because "could not verify" and "IS
|
||||
GONE" send an operator to entirely different places.
|
||||
"""
|
||||
preflight.set_codes({"32747a0": code})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0, "an unestablished check must not report success"
|
||||
assert "could NOT VERIFY" in result.stderr
|
||||
assert "IS GONE" not in result.stderr, "could-not-tell must never be reported as gone"
|
||||
|
||||
|
||||
def test_an_unknown_is_RETRIED_before_it_fails(preflight):
|
||||
"""Retries are what make failing on unknown affordable rather than flaky."""
|
||||
preflight.env["ETV_CI_ATTEMPTS"] = "3"
|
||||
preflight.set_codes({"32747a0": "503"})
|
||||
assert preflight.run().returncode != 0
|
||||
assert len(preflight.calls()) == 3, f"expected 3 attempts, got {preflight.calls()}"
|
||||
|
||||
|
||||
def test_an_ANSWER_is_not_retried(preflight):
|
||||
"""404 and 200 are answers; retrying them would only slow the job down."""
|
||||
preflight.set_codes({"32747a0": "404"})
|
||||
assert preflight.run().returncode != 0
|
||||
assert len(preflight.calls()) == 1, f"a 404 must not be retried, got {preflight.calls()}"
|
||||
|
||||
|
||||
def test_HTTP_200_with_a_body_that_is_not_a_manifest_is_not_a_pass(preflight):
|
||||
"""A proxy or a login page answers 200 too; the status line alone establishes nothing."""
|
||||
preflight.set_codes({"32747a0": "200-NOT-A-MANIFEST"})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "not a manifest" in result.stderr
|
||||
assert "IS GONE" not in result.stderr
|
||||
|
||||
|
||||
@pytest.mark.parametrize("code", ["401", "403"])
|
||||
def test_rejected_credentials_refuse_rather_than_pass(preflight, code):
|
||||
"""The failure mode that would otherwise make this job green forever.
|
||||
|
||||
An anonymous read of this registry is 401 for a live tag and a deleted one alike, so treating
|
||||
an auth failure as "could not tell, carry on" would turn a broken secret into a permanent,
|
||||
silent pass.
|
||||
"""
|
||||
preflight.set_codes({"32747a0": code})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "rejected these credentials" in result.stderr
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("value", "shape"),
|
||||
[
|
||||
(None, "unset"),
|
||||
(":", "both secrets absent — WHAT THE WORKFLOW ACTUALLY PASSES"),
|
||||
("user:", "password secret absent"),
|
||||
(":pass", "user secret absent"),
|
||||
("no-colon", "malformed"),
|
||||
],
|
||||
)
|
||||
def test_unusable_credentials_refuse_BEFORE_querying_anything(preflight, value, shape):
|
||||
"""The empty-halves cases are the ones that happen, and testing only `unset` misses them.
|
||||
|
||||
`ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }}` interpolates
|
||||
a missing secret to the empty string, so a job with no secrets configured passes the non-empty
|
||||
string ":" — which is a perfectly good non-empty value and a useless credential. The registry
|
||||
answers 401 to it for a live tag and a deleted one alike.
|
||||
"""
|
||||
if value is None:
|
||||
del preflight.env["ETV_REGISTRY_AUTH"]
|
||||
else:
|
||||
preflight.env["ETV_REGISTRY_AUTH"] = value
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0, f"{shape}: reported success on a credential it cannot use"
|
||||
assert "ETV_REGISTRY_AUTH" in result.stderr
|
||||
assert preflight.calls() == [], "it must not query the registry it cannot authenticate to"
|
||||
|
||||
|
||||
def test_the_credential_actually_REACHES_the_registry(preflight):
|
||||
"""Anti-vacuity for every test above: the stub 401s an unauthenticated read, as the live
|
||||
registry does, so a script that stopped passing `-u` would redden the whole file rather than
|
||||
sailing through on a stub that answers 200 regardless."""
|
||||
preflight.set_codes({"*": "200"})
|
||||
assert preflight.run().returncode == 0
|
||||
assert preflight.calls() == [
|
||||
"http://192.168.1.95:3000/v2/timothy/ersatztv-ci/manifests/32747a0 auth=stub-user:stub-pass"
|
||||
]
|
||||
|
||||
|
||||
def test_a_workflow_with_no_pin_at_all_is_a_failure(preflight):
|
||||
"""If the grep stops matching, the honest report is 'I found nothing', not 'all clear'."""
|
||||
preflight.set_workflow_text("jobs:\n test:\n runs-on: ubuntu-latest\n")
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "no ersatztv-ci pin found" in result.stderr
|
||||
|
||||
|
||||
def test_every_distinct_pin_is_checked_and_one_gone_fails_the_job(preflight):
|
||||
"""`ci-image-pin` bans a second pin; this must not silently check only the first one anyway."""
|
||||
preflight.set_workflow_text(
|
||||
WORKFLOW_TEMPLATE.format(pin="32747a0") + " image: 192.168.1.95:3000/timothy/ersatztv-ci:15d2439\n"
|
||||
)
|
||||
preflight.set_codes({"32747a0": "200", "15d2439": "404"})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "15d2439" in result.stderr
|
||||
assert len(preflight.calls()) == 2, f"both pins must be queried, got {preflight.calls()}"
|
||||
|
||||
|
||||
def test_the_grep_line_cannot_match_ITSELF(preflight):
|
||||
"""The pin is found with the same expression `pr-checks.yml::ci-image-pin` uses.
|
||||
|
||||
That expression is written into this script's own source, so a careless pattern would find its
|
||||
own text and 'check' a pin nobody wrote — and the same hazard sits in `pr-checks.yml`, whose
|
||||
pin-count check greps the file this script's job now lives in. Feed the real script its own
|
||||
source as the workflow file: the answer must be 'no pin found', not a query for `[0-9a-f]+`.
|
||||
This also pins the second half of the property — the source carries no literal pin of its own,
|
||||
so the file cannot go stale against a pin bump it does not participate in.
|
||||
"""
|
||||
preflight.set_workflow_text(SCRIPT.read_text())
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "no ersatztv-ci pin found" in result.stderr
|
||||
assert preflight.calls() == []
|
||||
|
||||
|
||||
def test_the_PRODUCTION_retry_defaults_are_the_ones_that_run(preflight):
|
||||
"""Every other test overrides the retry knobs, so nothing evaluated `${VAR:-default}` itself.
|
||||
|
||||
That matters because the defaults are the argument: "unknown fails" is only affordable if an
|
||||
ordinary registry blip is absorbed first. Edited to 1 attempt / 0 seconds, this file would stay
|
||||
green while a single transient 503 reddened every PR. So this one drops both overrides and
|
||||
measures the real thing — three attempts, and a pause long enough to have actually happened.
|
||||
"""
|
||||
del preflight.env["ETV_CI_ATTEMPTS"]
|
||||
del preflight.env["ETV_CI_RETRY_SECONDS"]
|
||||
preflight.set_codes({"32747a0": "503"})
|
||||
|
||||
started = time.monotonic()
|
||||
result = preflight.run()
|
||||
elapsed = time.monotonic() - started
|
||||
|
||||
assert result.returncode != 0
|
||||
assert len(preflight.calls()) == 3, f"the default attempt count is not 3 — got {len(preflight.calls())} call(s)"
|
||||
assert elapsed >= 8, (
|
||||
f"two pauses at the default 5s should clear the 8s floor; took {elapsed:.1f}s, so the pause "
|
||||
"has been shortened out from under the 'a blip does not redden a PR' argument"
|
||||
)
|
||||
@@ -50,6 +50,11 @@ if "-d" in args:
|
||||
payload = args[args.index("-d") + 1]
|
||||
|
||||
if is_post:
|
||||
# A POST can be scripted to fail (`post_fail` holds a URL substring), so the two write paths
|
||||
# can be broken independently — the shape ersatztv#792 is about.
|
||||
fail_on = (state / "post_fail").read_text().strip() if (state / "post_fail").exists() else ""
|
||||
if fail_on and fail_on in url:
|
||||
sys.exit(22)
|
||||
with (state / "posts.jsonl").open("a") as fh:
|
||||
fh.write(json.dumps({"url": url, "payload": json.loads(payload)}) + "\n")
|
||||
print("{}")
|
||||
@@ -120,6 +125,10 @@ def gitea(tmp_path):
|
||||
def set_pr_state(self, value):
|
||||
(state / "pr_state").write_text(value)
|
||||
|
||||
def fail_posts_to(self, url_substring):
|
||||
"""Make POSTs whose URL contains this substring fail, as curl -f does on a 4xx/5xx."""
|
||||
(state / "post_fail").write_text(url_substring)
|
||||
|
||||
def set_base_sequence(self, *refs):
|
||||
"""Base branch per PR GET. 'MISSING' omits `.base` from the response entirely."""
|
||||
(state / "pr_bases").write_text(" ".join(refs))
|
||||
@@ -177,8 +186,10 @@ def test_refuses_when_head_moves_mid_flight(gitea):
|
||||
assert result.returncode != 0
|
||||
assert "UNREVIEWED" in result.stderr
|
||||
assert gitea.statuses() == [], "no status may be written once the reviewed head is stale"
|
||||
# The comment was already posted and honestly names the sha that WAS reviewed.
|
||||
assert SHA_A[:7] in gitea.comments()[0]["payload"]["body"]
|
||||
# And no comment either, since ersatztv#792. This assertion used to say the opposite — the
|
||||
# comment went first, so a refusal left `Review-verdict: MERGEABLE @ <sha>` on the PR with no
|
||||
# status behind it, which reads to an operator as consent that was never granted.
|
||||
assert gitea.comments() == [], "a refusal must leave no verdict comment standing in for a status"
|
||||
|
||||
|
||||
def test_never_retargets_the_verdict_at_the_new_head(gitea):
|
||||
@@ -412,3 +423,110 @@ def test_a_reread_that_LOSES_a_field_refuses_instead_of_posting(head_seq, base_s
|
||||
assert not gitea.statuses(), (
|
||||
f"a status was written despite the re-read carrying no {field} — this is the fail-open the -n conjunct created"
|
||||
)
|
||||
|
||||
|
||||
# --- ersatztv#792: no path may write no status and report success ------------------------------
|
||||
#
|
||||
# The issue was filed on an observed "printed the refusal AND exited 0". Re-measured on the tree
|
||||
# that fixed the re-read fence: every refusal already exits NON-ZERO, and the exit-0 came from the
|
||||
# caller's pipeline, not from the script. That is worth an executed contract rather than a second
|
||||
# reading of the source — `die` is one line away from being edited into a `return`, and this file is
|
||||
# where that would be caught. The parametrisation covers each refusal REASON, not one
|
||||
# representative, because those paths were added at four different times and only the shared helper
|
||||
# makes them agree today.
|
||||
#
|
||||
# SCOPE, since "every path" would overclaim: these are the eight refusal MODES reachable through the
|
||||
# real entry point. The source also exits non-zero for a usage error (2), a failing `jq` (its own
|
||||
# status, under `set -e` + `pipefail`), and a signal (128+n); none of those is a refusal DECISION,
|
||||
# and only the non-zero-ness is common to all of them.
|
||||
|
||||
|
||||
def _drive(gitea, mode):
|
||||
if mode == "head-moved":
|
||||
gitea.set_head_sequence(SHA_A, SHA_B)
|
||||
elif mode == "reread-failed":
|
||||
gitea.set_head_sequence(SHA_A, "GONE")
|
||||
elif mode == "reread-lost-head":
|
||||
gitea.set_head_sequence(SHA_A, "NOHEAD")
|
||||
elif mode == "base-retargeted":
|
||||
gitea.set_base_sequence("main", "some-feature-branch")
|
||||
elif mode == "reread-lost-base":
|
||||
gitea.set_base_sequence("main", "MISSING")
|
||||
elif mode == "first-read-failed":
|
||||
gitea.set_head_sequence("GONE")
|
||||
elif mode == "pr-closed":
|
||||
gitea.set_pr_state("closed")
|
||||
elif mode == "status-post-failed":
|
||||
gitea.fail_posts_to("/statuses/")
|
||||
else: # pragma: no cover - a typo in the parametrisation must not pass silently
|
||||
raise AssertionError(f"unknown mode {mode}")
|
||||
return gitea.run("42", "MERGEABLE")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mode",
|
||||
[
|
||||
"head-moved",
|
||||
"reread-failed",
|
||||
"reread-lost-head",
|
||||
"base-retargeted",
|
||||
"reread-lost-base",
|
||||
"first-read-failed",
|
||||
"pr-closed",
|
||||
"status-post-failed",
|
||||
],
|
||||
)
|
||||
def test_every_REFUSAL_MODE_that_writes_no_status_exits_non_zero(gitea, mode):
|
||||
result = _drive(gitea, mode)
|
||||
assert gitea.statuses() == [], f"{mode} wrote a status it had no business writing"
|
||||
assert result.returncode != 0, (
|
||||
f"{mode} wrote no status and reported SUCCESS — anything checking $? concludes the verdict "
|
||||
f"posted. stdout={result.stdout!r} stderr={result.stderr!r}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mode",
|
||||
[
|
||||
"head-moved",
|
||||
"reread-failed",
|
||||
"reread-lost-head",
|
||||
"base-retargeted",
|
||||
"reread-lost-base",
|
||||
"status-post-failed",
|
||||
],
|
||||
)
|
||||
def test_no_refusal_leaves_a_VERDICT_COMMENT_standing_in_for_the_status(gitea, mode):
|
||||
"""The half-state, which is the part of #792 that was really broken.
|
||||
|
||||
The comment is not the gate — `review-verdict/h10` is — but `Review-verdict: MERGEABLE @ <head>`
|
||||
sitting on a PR reads exactly like consent. Every mode here is one where the status is refused
|
||||
after the head has been resolved, i.e. every mode that could once have left that comment behind.
|
||||
"""
|
||||
_drive(gitea, mode)
|
||||
assert gitea.comments() == [], f"{mode} left an orphaned verdict comment: {gitea.comments()}"
|
||||
|
||||
|
||||
def test_the_status_is_written_BEFORE_the_comment(gitea):
|
||||
"""Ordering is the mechanism, so it is asserted rather than described.
|
||||
|
||||
Status-then-comment makes the only reachable half-state the safe one: a status with no comment
|
||||
leaves the merge hook's condition (c) with nothing to classify, which is an `ask`. The reverse
|
||||
order manufactures the appearance of a granted verdict.
|
||||
"""
|
||||
assert gitea.run("42", "MERGEABLE").returncode == 0
|
||||
urls = [p["url"] for p in gitea.posts()]
|
||||
assert len(urls) == 2, urls
|
||||
assert "/statuses/" in urls[0], f"the status must be written first, got {urls}"
|
||||
assert "/comments" in urls[1], f"the comment must be written second, got {urls}"
|
||||
|
||||
|
||||
def test_a_failed_COMMENT_after_a_written_status_is_still_an_error(gitea):
|
||||
"""The surviving half-state is safe, not silent: the operator is told to re-run."""
|
||||
gitea.fail_posts_to("/comments")
|
||||
result = gitea.run("42", "MERGEABLE")
|
||||
assert result.returncode != 0
|
||||
assert len(gitea.statuses()) == 1, "the status was already written and must not be rolled back"
|
||||
assert gitea.comments() == []
|
||||
assert "COMMENT could not be posted" in result.stderr
|
||||
assert "ask" in result.stderr.lower(), "it must say what the gate will do, not just that a call failed"
|
||||
|
||||
Reference in New Issue
Block a user