fix(772,792): name the missing toolchain image, and stop a refusal leaving a verdict comment
#772 — the pinned CI toolchain image can be deleted out from under us, and when it was (2026-08-11..13) all five `container:` jobs died at image pull, both required contexts included, with the cause buried in each job's log. Root cause is registry-side and is now established rather than guessed: an owner-level Gitea package cleanup rule (keep_count 15, remove_days 1, remove_pattern `.*`, keep_pattern no 7-hex sha can match) deletes a sha tag once 15 newer versions exist, and `ExecuteCleanupRules` ran nightly through the window. The `ersatztv` package carries the same rule's fingerprint exactly — every sha tag older than the 15-slot window is gone, every keep_pattern tag back to 26.3.1 survives. Version deletes leave no audit row, so the specific run cannot be replayed; that limit is stated where the claim is made. The durable fix belongs to the registry's repo: server-management#842. What lands here is what a consumer of someone else's registry can do: * `toolchain-preflight`, a container-free job (a job consuming the image could not run to report it missing) resolving every pin against the registry and failing with a message that names the tag and the recovery. Not a `needs:` of the jobs it diagnoses — gating five jobs behind a checkout and one curl taxes every green run to speed up a rare red one, and they already fail fast. * Only HTTP 404 means gone. Everything else is could-not-tell, and rejected credentials fail rather than pass as unknown — "the check could not run" must never present as "the pin is fine". * A recovery path that does not need CI: rebuild the SAME tag from the commit it names and push it. The push half was verified against this registry on 2026-08-22 with a throwaway package (created, resolved 200, deleted). #792 — the reported defect was the exit code, and re-measuring says that premise is false: every no-status path already exits 1, and eight refusal modes now assert it against the real predecessor, where they pass. The observed 0 came from the invocation, not the script. What WAS broken is the half-state the issue describes second: the comment was written before the status, so every refusal left `Review-verdict: MERGEABLE @ <head>` on a PR with no gating status behind it. The two writes are now ordered status-then-comment, which 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 refusals themselves are untouched. Ordering rather than compensating deletion: an orphaned-comment cleanup needs a Gitea call, and these refusals are usually caused by Gitea being unreachable. Proof for the ordering is the split against origin/main's script: the 8 orphan/ordering tests go red there, the 8 exit-code tests stay green. fixes #772 fixes #792 Refs: server-management#842 Decisions-Edit: yes
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
|
||||
|
||||
@@ -1576,6 +1576,62 @@ 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
|
||||
|
||||
⚠️ **"Immutable" means the tag never MOVES. It does not mean the tag will still EXIST.** Those are
|
||||
different claims, and the second one 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.
|
||||
|
||||
**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. Only
|
||||
HTTP 404 is treated as "gone"; anything else (a 5xx, an unreachable host) is reported as
|
||||
could-not-tell, and rejected credentials fail the job rather than passing as unknown.
|
||||
|
||||
**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
|
||||
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
|
||||
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 .
|
||||
```
|
||||
|
||||
Then confirm the tag resolves before re-running anything — the preflight script does exactly this
|
||||
check and takes no arguments:
|
||||
|
||||
```bash
|
||||
ETV_REGISTRY_AUTH=user:pass scripts/ci-toolchain-image-resolves.sh
|
||||
```
|
||||
|
||||
The push half of that recipe was verified against this registry on 2026-08-22 (tag created, resolved
|
||||
`200`, then deleted) with a throwaway package; the build half is `ci-image.yml`'s own build line with
|
||||
its cache flags dropped. 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.
|
||||
|
||||
**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) |
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
---
|
||||
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 → condition (c) has nothing to classify, which the hook resolves as **ask**.
|
||||
Fail-closed, visible, and cured by re-running the command.
|
||||
|
||||
**#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 1 — 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: it authorizes nothing — it can only turn its own non-required job red — but 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 rather than accepted — every answer that is not HTTP 200 or 404 is reported as could-not-tell, and an auth failure exits non-zero, so "the check could not run" can never present as "the pin is fine". |
|
||||
| `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. |
|
||||
|
||||
Executable
+81
@@ -0,0 +1,81 @@
|
||||
#!/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.
|
||||
#
|
||||
# Env (all optional; 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_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.
|
||||
[ -n "${ETV_REGISTRY_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"
|
||||
|
||||
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'
|
||||
rc=0
|
||||
|
||||
for pin in $pins; do
|
||||
url="http://$registry/v2/$image_repo/manifests/$pin"
|
||||
code=$(curl -s -o /dev/null -w '%{http_code}' -I -u "$ETV_REGISTRY_AUTH" -H "Accept: $accept" "$url" || printf '000')
|
||||
case "$code" in
|
||||
200)
|
||||
printf 'ci-toolchain-image-resolves: %s/%s:%s resolves (HTTP 200)\n' "$registry" "$image_repo" "$pin"
|
||||
;;
|
||||
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 "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."
|
||||
;;
|
||||
*)
|
||||
# Deliberately NOT a failure. Only 404 answers the question this job asks; a transient
|
||||
# registry (000/5xx) leaves it UNKNOWN, and the container jobs may still be running off a
|
||||
# runner-local image cache. Reddening every run on a registry hiccup would buy a false
|
||||
# signal with real flake, so the unknown is reported as an unknown.
|
||||
printf '::warning::ci-toolchain-image-resolves: could not determine whether %s/%s:%s exists (HTTP %s). This is NOT a pass — the pin was not verified.\n' \
|
||||
"$registry" "$image_repo" "$pin" "$code" >&2
|
||||
;;
|
||||
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,19 @@ 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 did not fail the preflight",
|
||||
why="404 is the ONE answer that establishes the pinned toolchain image is gone; every other "
|
||||
"code is could-not-tell and exits 0 by design. Retargeting the arm sends the real outage "
|
||||
"down the warn path, where the script still runs and still prints — the shape this preflight "
|
||||
"exists to replace, reproduced inside the preflight itself.",
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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,201 @@
|
||||
"""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`: disarming the `404` arm leaves a script that still runs, still
|
||||
prints, still exits 0 — and never reports the outage it exists for.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
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 -o /dev/null -w '%{http_code}' -I -u … <url>`: prints the scripted HTTP
|
||||
# code for the tag in the URL and logs the call, so a test can also assert curl was NOT reached.
|
||||
CURL_SHIM = r"""#!/usr/bin/env python3
|
||||
import os, pathlib, sys
|
||||
|
||||
state = pathlib.Path(os.environ["STUB_DIR"])
|
||||
url = [a for a in sys.argv[1:] if a.startswith("http")][-1]
|
||||
tag = url.rsplit("/", 1)[-1]
|
||||
with (state / "calls").open("a") as fh:
|
||||
fh.write(url + "\n")
|
||||
|
||||
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": # curl itself fails (unreachable host): no body, non-zero exit
|
||||
sys.exit(7)
|
||||
print(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"
|
||||
|
||||
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-tell path, where it warns and exits 0 — a preflight that runs, prints, and misses the
|
||||
only thing it was built to catch.
|
||||
"""
|
||||
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
|
||||
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"
|
||||
|
||||
|
||||
def test_an_unknown_answer_warns_and_does_NOT_claim_a_pass(preflight):
|
||||
"""A transient registry is not evidence either way, and is not treated as either."""
|
||||
preflight.set_codes({"32747a0": "TRANSPORT"})
|
||||
result = preflight.run()
|
||||
assert result.returncode == 0, "a registry hiccup must not redden every run"
|
||||
assert "::warning::" in result.stderr
|
||||
assert "NOT a pass" in result.stderr
|
||||
assert "IS GONE" not in result.stderr, "could-not-tell must never be reported as gone"
|
||||
|
||||
|
||||
def test_a_500_is_also_unknown_rather_than_gone(preflight):
|
||||
preflight.set_codes({"32747a0": "503"})
|
||||
result = preflight.run()
|
||||
assert result.returncode == 0
|
||||
assert "::warning::" 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
|
||||
|
||||
|
||||
def test_missing_credentials_refuse_BEFORE_querying_anything(preflight):
|
||||
del preflight.env["ETV_REGISTRY_AUTH"]
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "ETV_REGISTRY_AUTH" in result.stderr
|
||||
assert preflight.calls() == [], "it must not query the registry it cannot authenticate to"
|
||||
|
||||
|
||||
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() == []
|
||||
@@ -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,104 @@ 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 1, 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
|
||||
# the paths were added at four different times and only the shared helper makes them agree today.
|
||||
|
||||
|
||||
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_path_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, "it must say what the gate will do, not just that a call failed"
|
||||
|
||||
Reference in New Issue
Block a user