timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration #666

Merged
timothy merged 4 commits from fix/649-enforced-verdict-guard into main 2026-07-27 00:04:07 +02:00
Conversation 1 Commits 4 Files Changed 4
+501 -91
4 changed files with 501 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitea/workflows/review-verdict.yml
+88 -41
View File
@@ -38,6 +38,22 @@ name: Review verdict
# NOT the required check and is not what gates merges — `review-verdict/h10`, the status it POSTS,
# is. Keeping them distinct is deliberate: a workflow cannot be allowed to satisfy the gate merely
# by running successfully.
#
# THE CHANGED-FILE ENUMERATION IS NOT INLINE HERE (ersatztv#649). It lives in
# `scripts/pr-changed-files.sh`, the single implementation this job and the advisory hook
# `.claude/hooks/pretooluse-merge-consent.sh` both call. It used to be written twice, and drifted in
# the dangerous direction: four rounds of ersatztv#643 hardening landed on the ADVISORY copy (whose
# failure mode is a human prompt) and never reached THIS one (whose failure mode is a `success`
# write to a required status with nobody in the loop). See
# `docs/decisions/records/ci/shared-pr-file-enumeration.md`.
#
# WHY THE CHECKOUT TAKES THE PR'S **BASE** REF, NEVER THE HEAD. This job judges the PR, so the PR
# must not be able to supply the code that judges it. Checking out `head.sha` would let a PR edit
# `scripts/pr-changed-files.sh` to return an empty list and exempt itself — the `PROTECTED` list
# below would flag the edit, but only if the enumeration that feeds it were trustworthy, and it
# would be the PR's own. `base.sha` is the commit the PR merges INTO: already on `main`, already
# reviewed. `persist-credentials: false` because nothing here pushes, and a checkout that leaves a
# token in `.git/config` hands it to every script the job runs.
on:
pull_request:
@@ -52,13 +68,57 @@ jobs:
name: Set review-verdict status
runs-on: small # a few API calls; keep it off the build runners
steps:
# BASE, not head — see the header. `fetch-depth: 1` is enough: nothing here reads history,
# only the working tree's `scripts/`.
- name: Checkout the PR's BASE ref
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.base.sha }}
fetch-depth: 1
persist-credentials: false
# FLOOR ONLY — never `--expect` in this workflow. `--expect` pins an exact version and fails
# when it drifts, which is right for `script-tests` (advisory) and catastrophic here: this job
# writes `review-verdict/h10`, a REQUIRED check on `main`, so a pin would turn any jq bump on
# the runner into a repo-wide merge deadlock. Asserting the 1.6 floor is what the gates below
# are written against; see docs/ci-cd.md -> "The jq contract".
#
# A hard failure here is correct and fails CLOSED: the job dies, no `review-verdict/h10` is
# posted, and an absent required check blocks the merge. Guarded on presence because a PR
# whose BASE predates ersatztv#658 has no such script, and "the base is old" is not a jq
# problem — that case is handled as an enumeration failure below, with an actionable status.
- name: jq preflight (floor only)
run: |
set -euo pipefail
if [ -x ./scripts/jq-preflight.sh ]; then
./scripts/jq-preflight.sh
else
echo "::warning::The PR's base ref has no scripts/jq-preflight.sh; skipping the version assertion. The enumeration step below will fail closed on its own."
fi
- name: Classify the PR and post the review-verdict status
env:
GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
BASE_URL: ${{ github.server_url }}/api/v1
# `scripts/pr-changed-files.sh` reads GITEA_BASE_URL (not BASE_URL) and takes owner/repo as
# two SEPARATE arguments (not one `owner/repo` string). Getting either wrong is silent, not
# loud: the script would fall back to its hardcoded LAN default and enumerate the wrong
# repo, or a wrong host that answers, rather than erroring. A value already ending in
# /api/v1 is used as-is by the script.
GITEA_BASE_URL: ${{ github.server_url }}/api/v1
# BOTH names, same value, on purpose. The script's precedence is
# `ETV_GITEA_URL` > `GITEA_BASE_URL` > a hardcoded LAN default (and `ETV_GITEA_TOKEN` >
# `GITEA_TOKEN`), because its other caller is a developer Mac using the ETV_* convention.
# Setting only the GITEA_* names would leave this job's explicit configuration NON-
# authoritative: a runner that happened to export a stale ETV_GITEA_URL would silently
# enumerate a different Gitea instance and post the verdict here from a diff read there.
# Cheap to make deterministic; leave both set even though only one is read.
ETV_GITEA_URL: ${{ github.server_url }}/api/v1
ETV_GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
REPO: ${{ github.repository }}
PR: ${{ github.event.pull_request.number }}
SHA: ${{ github.event.pull_request.head.sha }}
BASE_SHA: ${{ github.event.pull_request.base.sha }}
AUTHOR: ${{ github.event.pull_request.user.login }}
PR_URL: ${{ github.event.pull_request.html_url }}
run: |
@@ -116,56 +176,43 @@ jobs:
exit 0
fi
# --- Changed files: PAGE to exhaustion, and fail CLOSED if we cannot. ----------------
# Gitea caps this endpoint at 50 rows per page and SILENTLY IGNORES a larger `limit`
# (verified: PR #619 has 194 changed files and `?limit=100` returns exactly 50). A
# single-page read is therefore a silent false negative: a protected path sitting at
# position 51+ would simply not be seen, and a bot-authored PR that edits the gate could
# exempt itself from the gate. Page until a short page proves the end.
PAGE_SIZE=50
MAX_PAGES=40 # 2000 files; beyond this we refuse rather than guess
# --- Changed files: the SHARED enumeration, or no exemption. -------------------------
# `scripts/pr-changed-files.sh` (from the BASE checkout) owns every guard this job used to
# carry inline and six it did not: CR/LF rejection, `..` rejection, a closed `.status`
# allow-list, `previous_filename` validated on EVERY row rather than only `renamed` ones,
# termination only on a validated EMPTY page rather than a merely short one, and head-sha
# binding across the paging round-trips. ersatztv#649.
#
# READ THE EXIT STATUS, NEVER THE STDOUT OF A FAILED RUN. exit 0 means "complete and bound
# to $SHA"; anything else means "could not tell" and stdout is meaningless. That the
# script happens to print nothing on its failure paths is redundancy, not contract —
# `files` is therefore cleared explicitly rather than trusted to be empty. stderr is left
# attached to the job log on purpose: its diagnostic is the only thing that distinguishes
# a force-push mid-enumeration from a dead API.
ENUM=./scripts/pr-changed-files.sh
files=""
page=1
complete=no
while [ "$page" -le "$MAX_PAGES" ]; do
raw=$(gh "$BASE_URL/repos/$REPO/pulls/$PR/files?limit=${PAGE_SIZE}&page=${page}") || raw=""
# A transport/parse failure must NOT masquerade as a legitimate short final page.
# Empty output counts as zero rows, which would otherwise read as "end of list" and set
# complete=yes over a PARTIAL enumeration — failing OPEN at the exact point this guard
# exists to fail closed.
#
# Validating only the TOP-LEVEL type is not enough: a page like `[{}]` is a well-formed
# array whose rows carry no `filename`, so it contributes no paths, counts as a short
# page, and completes the enumeration from a partial list — the same failure one level
# down. Require every row to carry a non-empty string `filename`; an empty array stays
# valid, since that is what a genuine end-of-pagination looks like.
if ! printf '%s' "$raw" \
| jq -e 'type == "array" and all(.[]; (.filename | type == "string" and length > 0) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))' \
>/dev/null 2>&1; then
complete=no; break
fi
# Page-size termination is measured in ROWS; the path set collects BOTH sides of a
# rename. Gitea reports a `git mv` as ONE row whose `filename` is the DESTINATION, with
# the source only in `previous_filename` — so reading `filename` alone lets a PR move a
# protected file INTO docs/ and pass as docs-only (verified live:
# `.gitea/workflows/renovate.yml` -> `docs/innocuous-note.md` showed no protected path).
# One renamed row thus contributes ONE to `n` and TWO to the path set, which is why
# these two counts are deliberately computed differently.
n=$(printf '%s' "$raw" | jq -r 'length')
chunk=$(printf '%s' "$raw" | jq -r '.[] | (.filename // empty), (.previous_filename // empty)')
[ -n "$chunk" ] && files=$(printf '%s\n%s' "$files" "$chunk")
if [ "$n" -lt "$PAGE_SIZE" ]; then complete=yes; break; fi
page=$((page + 1))
done
enum_error=""
if [ ! -x "$ENUM" ]; then
# Only reachable for a PR whose BASE predates ersatztv#658. Fail closed with a readable
# status rather than an absent one, so the PR shows why instead of stalling silently.
enum_error="the PR's base ref (${BASE_SHA:0:7}) has no executable ${ENUM}"
elif files=$("$ENUM" "${REPO%%/*}" "${REPO#*/}" "$PR" "$SHA"); then
complete=yes
else
files=""
enum_error="scripts/pr-changed-files.sh could not enumerate PR #${PR} at ${SHA:0:7} exhaustively (see the step log)"
fi
files=$(printf '%s\n' "$files" | grep -v '^$' || true)
count=$(printf '%s\n' "$files" | grep -c . || true)
echo "Changed files (${count}, complete=${complete}, pages=${page}):"
echo "Changed files (${count}, complete=${complete}):"
printf '%s\n' "$files" | sed 's/^/ /'
exempt=no
reason=""
if [ "$complete" != yes ]; then
reason="could not enumerate the changed files exhaustively (stopped at ${count}) — no exemption"
reason="${enum_error} — no exemption"
elif [ "${count:-0}" -eq 0 ]; then
reason="no changed files could be read from the API — no exemption"
elif printf '%s\n' "$files" | grep -qE "$PROTECTED"; then
docs/ci-cd.md
+22 -8
View File
@@ -665,13 +665,6 @@ the runner image.
### The jq contract (ersatztv#648)
> **Landing note (staged over two PRs).** The `review-verdict.yml` half of this — the floor-only
> preflight and the call into `scripts/pr-changed-files.sh` — lands in the FOLLOW-UP PR, not the one
> that introduces these scripts. That workflow checks out the PR's **base** ref, and the base is
> `main`; until these scripts exist on `main`, wiring the workflow would make the job exit 127 on its
> own PR and block the merge gate through the combined status. The scripts land first, then the
> enforced gate is pointed at them.
> Full rationale: `docs/decisions/records/ci/jq-version-contract.md`.
Every shell gate in this repo — `decisions-guard`, `script-tests`'s own harness,
@@ -793,12 +786,33 @@ PR touches `.claude/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`** — a
gate must not be able to exempt itself from the gate. That includes Renovate's `docker/ci` base
bumps, which already need the manual publish-then-pin two-step anyway.
Deciding either exemption requires the PR's **complete** changed-file list, which the workflow does
not compute itself: it calls `scripts/pr-changed-files.sh`, the single shared implementation also
used by the advisory hook `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv#649). The workflow
reads that script's **exit status** — a non-zero exit means "could not tell" and withholds the
exemption; its stdout is meaningless on any failure path and is never consumed.
Two properties of that call are security-relevant and are pinned by tests in
`scripts/tests/test_pr_changed_files.py`:
- **The checkout takes the PR's BASE ref**, `ref: ${{ github.event.pull_request.base.sha }}` with
`persist-credentials: false` — never the head. This job judges the PR, so the PR must not supply
the code that judges it; a head checkout would let a PR edit the enumeration to return an empty
list and exempt itself.
- **`scripts/jq-preflight.sh` runs in floor-only mode**, never `--expect`. This job writes a
branch-protection-**required** status, so an exact version pin would turn any jq upgrade on the
runner into a repo-wide merge deadlock.
A PR whose base predates ersatztv#658 has no such script on its base ref; that case posts `pending`
with the reason rather than dying with no status at all.
It lives in its **own workflow file** on purpose: `pr-checks.yml` sets `cancel-in-progress: true`,
and a cancelled run there would leave an exempt PR with no status and no further push to
re-trigger it. Its own job context (`Review verdict / Set review-verdict status`) is **not** the
required check — a workflow must not satisfy the gate merely by running successfully.
Full rationale: `docs/decisions/records/release/verdict-status-check.md`.
Full rationale: `docs/decisions/records/release/verdict-status-check.md` and
`docs/decisions/records/ci/shared-pr-file-enumeration.md`.
## CI toolchain image (`docker/ci/Dockerfile`, `.gitea/workflows/ci-image.yml`)
docs/decisions/records/ci/shared-pr-file-enumeration.md
+31 -16
View File
@@ -10,11 +10,6 @@ signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hoo
mechanics: '`scripts/pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha>` -> stdout newline-delimited paths, exit 0 only if complete and bound to the given sha; callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`'
---
> **Landing note.** This record describes the end state. It is delivered over two PRs: the shared
> script, its tests and the hook rewiring first; the `review-verdict.yml` rewiring second, once the
> script exists on `main`. The reason is the base-ref checkout below — a workflow that runs the base
> version of the gate cannot call a script the base does not yet have.
Before #649, the PR changed-file enumeration existed as two independent implementations. That would
be an ordinary duplication smell anywhere else; here it was actively dangerous, because the two
copies had unequal *consequence*. The advisory hook's failure mode is a human permission prompt — a
@@ -73,20 +68,40 @@ in each caller:
requested well before the real end of the list, so "short page" is not a valid termination signal.
- Head-sha binding: the head is re-read after enumeration, and the caller must refuse to trust the
list if it moved mid-enumeration, since paging is several round-trips and a force-push between them
would otherwise yield a list belonging to no single commit.
would otherwise yield a list belonging to no single commit. **This detects ONE-WAY movement only.**
An A→B→A force-push round trip restores the expected sha, so the binding holds while the pages came
from two different states — see #664. Closing that needs a commit-pinned files endpoint (Gitea has
none) or a local diff, not a tighter check here; the guarantee is stated narrowly rather than left
to read as complete.
**Base-ref checkout — a REQUIREMENT on the follow-up, not yet a property of the tree.** When
`review-verdict.yml` is wired to this script it MUST check out the PR's BASE ref
(`ref: ${{ github.event.pull_request.base.sha }}`, plus `persist-credentials: false`), never the
**Base-ref checkout — now a property of the tree.** `review-verdict.yml` checks out the PR's BASE
ref (`ref: ${{ github.event.pull_request.base.sha }}`, `persist-credentials: false`), never the
head, so the workflow that judges a PR cannot be rewritten by that same PR to weaken its own
judgment.
judgment. The checkout and its `ref` are the security-relevant parts — a bare `run:` calling the
script would not have been sufficient, since the script would then have come from wherever the
runner happened to be.
Stated in the future tense deliberately. Today `review-verdict.yml` has **no `actions/checkout` step
at all** — it is a pure inline `run:` — so nothing about base-vs-head holds of it right now, and the
`PROTECTED` path list is the only thing standing between a PR and the gate. An earlier draft asserted
this in the present tense; a reader resolving this record from the catalog would have concluded the
self-rewrite hole was already closed. Adding a bare `run:` line in the follow-up is **not**
sufficient — the checkout and its `ref` are the security-relevant parts.
An earlier revision of this record stated the requirement in the future tense, because the wiring
was staged over two PRs: the workflow runs the BASE version of the gate, and until the shared script
existed on `main` a wired workflow would have exited 127 on its own PR and blocked the merge gate
through the combined status. That staging is complete. Both halves are asserted by
`scripts/tests/test_pr_changed_files.py`, which parses the workflow YAML rather than substring-
matching it — `head.sha` for `base.sha` is a nine-character diff, and a text-level check would still
pass if a second checkout step took the head afterwards and won.
**What is deliberately NOT claimed.** The base-ref checkout closes the self-rewrite route through
the *scripts*; it does not make the workflow immune to everything a PR controls. The `PROTECTED`
path list remains the guard that stops a bot-authored PR from editing the gate and exempting itself,
and mutation testing was what established that `PROTECTED` is load-bearing only on the BOT path —
it and `DOCS_ONLY` are disjoint patterns, so on the docs-only path that clause can never fire. A
test written against a docs-only-plus-protected file list passed with the clause deleted.
**A commit status is repo-global, which this record does not fix either.** `review-verdict/h10` is
attached to a sha in the repository, not to a pull request, so a success earned on one PR is
inherited by any other PR with the same head — including one opened against a different base after
the first is closed (#663). That is the same property that makes #622's per-sha binding work, read
from the other end. Out of scope here; noted so the enumeration's guarantees are not mistaken for a
guarantee about *which PR* a verdict belongs to.
**Severity, stated honestly.** Every enumeration defect found in this area (#643) downgraded a
mechanical deny/ask to a human prompt on the hook side; none produced a silent self-merge on their
scripts/tests/test_pr_changed_files.py
+360 -26
View File
@@ -33,7 +33,7 @@ import pytest
REPO_ROOT = Path(__file__).resolve().parents[2]
SCRIPT = REPO_ROOT / "scripts" / "pr-changed-files.sh"
HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-merge-consent.sh"
# (the enforced caller, .gitea/workflows/review-verdict.yml, is wired in the follow-up PR)
WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml"
SHA = "a9e3e23abf337980ca4c05854f5b1e210099d08b"
OTHER_SHA = "b71c0d4e2f8a91b3c5d7e9f1a3b5c7d9e1f3a5b7"
@@ -158,12 +158,41 @@ def test_non_array_body_fails_closed(enumerate_files):
assert enumerate_files.fails_closed()
def test_an_OBJECT_OF_VALID_ROWS_isolates_the_top_level_array_check(enumerate_files):
"""Cold review found `test_non_array_body_fails_closed` passing for the wrong reason, and the
first attempt to fix it failed for a THIRD reason — worth recording, because both near-misses
look like coverage.
`jq`'s `all(.[]; …)` iterates an object's VALUES, so the top-level `type == "array"` check is
only load-bearing when those values would themselves validate:
* `{"message":"internal error"}` — values are strings, `.filename` on a string errors. Rejected
with the clause deleted, so it never isolated it.
* `{"filename":"docs/a.md","status":"modified"}` — a single row, but its values are still
strings. Same non-isolation, one level less obvious.
* `{"0": {"filename":"docs/a.md","status":"modified"}}` — values ARE valid rows. With the clause
deleted this validates, `length` is 1, the path is collected, and the next page ends the
enumeration cleanly: a non-array body enumerated as a complete docs-only list. That is the
fail-open, and only this shape exposes it.
"""
enumerate_files.set_pages({"0": {"filename": "docs/a.md", "status": "modified"}})
assert enumerate_files.fails_closed()
def test_rows_without_filename_fail_closed(enumerate_files):
"""`[{}]` is a well-formed array that yields no paths — a partial list wearing a valid shape."""
enumerate_files.set_pages([{}, {}])
assert enumerate_files.fails_closed()
def test_a_row_with_a_VALID_status_but_no_filename_isolates_the_filename_check(enumerate_files):
"""Same wrong-reason problem: `[{}]` is also rejected by the closed `.status` allow-list, since
an absent status is not in it. A row carrying a legitimate `status` and no `filename` removes
that second reason, leaving only the guard under test."""
enumerate_files.set_pages([{"status": "modified"}])
assert enumerate_files.fails_closed()
def test_array_of_scalars_fails_closed(enumerate_files):
enumerate_files.set_pages(["docs/a.md", "docs/b.md"])
assert enumerate_files.fails_closed()
@@ -205,9 +234,17 @@ def test_renamed_row_without_previous_filename_fails_closed(enumerate_files):
assert enumerate_files.fails_closed()
def test_head_moving_during_enumeration_fails_closed(enumerate_files):
"""A force-push between round-trips means page 1 came from head A and page 2 from head B, so
the assembled list belongs to no single commit."""
def test_head_differing_from_the_expected_sha_fails_closed(enumerate_files):
"""Narrowed to what this actually proves, per cold review.
The stub serves the alternate sha from the FIRST PR-object read, and the script reads that
object only once, after paging — so this exercises "the final head does not equal the expected
sha", not movement *during* enumeration. The distinction matters: an A->B->A force-push round
trip would restore the expected sha and pass this check while the pages came from two different
states. That race is inherent to enumerating a mutable list over several round-trips against an
API with no commit-pinned files endpoint, and is tracked separately rather than papered over
with a test name that implies it is covered.
"""
enumerate_files.set_pages(_rows(["docs/a.md"]))
enumerate_files.head_moves_to(OTHER_SHA)
assert enumerate_files.fails_closed()
@@ -237,6 +274,55 @@ def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files):
assert "ErsatzTV/Program.cs" in enumerate_files.paths()
# --- The same guard, under the runner's jq 1.6 ------------------------------------------------
#
# `test_transport_failure_mid_pagination_fails_closed` above does NOT isolate the explicit
# `if [ -z "${raw//[[:space:]]/}" ]` clause — cold review claimed this and mutation confirmed it:
# deleting that clause leaves the whole suite green on a developer Mac, because jq 1.8 rejects empty
# input on its own. jq 1.6 does not, and the runner ships 1.6 — so the one environment where the
# clause is load-bearing was the one environment with no coverage. That is the #643/#647 failure
# class exactly, reproduced in the test suite meant to prevent it.
#
# The shim is IMPORTED, not copied. A second copy of a version-quirk emulator is the same
# two-implementations-drift problem this whole issue is about, one level down.
from scripts.tests.test_merge_consent_exemption import _JQ16_SHIM # noqa: E402
@pytest.fixture
def enumerate_files_jq16(enumerate_files, tmp_path):
"""The same harness, plus a jq shim reproducing jq 1.6's empty-input `-e` exit status."""
jq = tmp_path / "bin" / "jq"
jq.write_text(_JQ16_SHIM)
jq.chmod(0o755)
return enumerate_files
def test_jq16_shim_actually_reproduces_the_quirk(enumerate_files_jq16, tmp_path):
"""Verify the verifier. A shim that failed to install would make the test below pass vacuously,
reporting the guard safe on jq 1.6 without ever exercising the quirk."""
assert enumerate_files_jq16 is not None # the fixture is what installs the shim
jq = str(tmp_path / "bin" / "jq")
empty = subprocess.run([jq, "-e", "."], input="", capture_output=True, text=True)
assert empty.returncode == 0, "the shim does not reproduce jq 1.6's empty-input exit 0"
real = subprocess.run([jq, "-e", ".a"], input='{"a":1}', capture_output=True, text=True)
assert real.returncode == 0 and real.stdout.strip() == "1", "the shim broke ordinary jq"
false = subprocess.run([jq, "-e", ".a"], input='{"a":false}', capture_output=True, text=True)
assert false.returncode == 1, "the shim broke jq's real -e semantics for a false result"
def test_transport_failure_mid_pagination_fails_closed_on_jq16(enumerate_files_jq16):
"""The property, asserted on the interpreter that actually runs it in CI."""
enumerate_files_jq16.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), "ERROR",
_rows(["docs/tail.md"]))
assert enumerate_files_jq16.fails_closed()
def test_a_docs_only_pr_still_enumerates_cleanly_on_jq16(enumerate_files_jq16):
"""Positive control: the shim must not make everything fail, or the test above proves nothing."""
enumerate_files_jq16.set_pages(_rows(["docs/a.md", "docs/b.md"]))
assert enumerate_files_jq16.paths() == ["docs/a.md", "docs/b.md"]
# --- The CALLER contract: a non-zero exit must withhold the exemption, on its own -------------
#
# This is the single line the whole extraction rests on, and it was the one guard nothing pinned. A
@@ -342,35 +428,283 @@ def test_a_MISSING_script_withholds_the_exemption(tmp_path):
# --- Drift guard: the reason this file is worth having at all ----------------------------------
#
# Scope is BOTH callers. Until the enforced workflow was rewired (the follow-up half of ersatztv#649)
# this guard could only assert the hook, which left the copy with real authority — the one that
# writes the branch-protection-required `review-verdict/h10` status — unpinned. That asymmetry was
# the entire subject of the issue, so a guard that covered only the advisory side would have been
# the same mistake one level up.
def test_the_hook_uses_the_shared_script_and_does_not_reimplement_it():
"""ersatztv#649's third Done-when box: a test that fails if a copy is re-inlined.
CALLERS = pytest.mark.parametrize(
"caller", [HOOK, WORKFLOW], ids=["advisory-hook", "enforced-workflow"])
Structural rather than behavioural on purpose. Behavioural equivalence tests would still pass if
someone pasted the loop back inline and kept it correct *that day* — which is exactly how the
drift happened the first time. What must be prevented is a SECOND implementation existing.
SCOPE NOTE — this asserts the HOOK only, deliberately. `.gitea/workflows/review-verdict.yml` is
the other caller, but its rewiring cannot land in this PR: that workflow will check out the BASE
ref (never the PR head, so a PR cannot rewrite the gate judging it), and the base is `main`,
which does not yet contain the scripts this PR ADDS. Wiring it here would make the job exit 127
on its own PR and block the merge gate via the combined status. The workflow half therefore
lands in the follow-up PR, once these scripts are on `main`, and that PR extends this test to
cover both callers.
def _code_lines(path: Path) -> str:
"""Strip comment lines.
Both callers' comments legitimately discuss the `pulls/N/files` endpoint, and a future comment
writing `pulls/$pr/files?limit=100` as an example of what NOT to do would redden these tests —
which, since `script-tests` red blocks merges via the combined status, would block the repo over
a piece of prose. `#`-prefixed works for both files: YAML comments and the shell comments inside
the workflow's `run:` block share the marker.
"""
return "\n".join(
ln for ln in path.read_text().splitlines() if not ln.lstrip().startswith("#"))
@CALLERS
def test_the_caller_uses_the_shared_script(caller):
assert "scripts/pr-changed-files.sh" in _code_lines(caller), (
f"{caller.relative_to(REPO_ROOT)} no longer calls the shared enumeration")
@CALLERS
def test_the_caller_does_not_reimplement_the_enumeration(caller):
"""Structural rather than behavioural on purpose. Behavioural equivalence tests would still pass
if someone pasted the loop back inline and kept it correct *that day* — which is exactly how the
drift happened the first time. What must be prevented is a SECOND implementation existing.
"""
text = HOOK.read_text()
assert "scripts/pr-changed-files.sh" in text, (
f"{HOOK.relative_to(REPO_ROOT)} no longer calls the shared enumeration")
# Search CODE only. The hook's comments legitimately discuss this endpoint, and a future comment
# writing `pulls/$pr/files?limit=100` as an example of what NOT to do would redden this test —
# which, since `script-tests` red blocks merges via the combined status, would block the repo over
# a piece of prose.
code = "\n".join(ln for ln in text.splitlines() if not ln.lstrip().startswith("#"))
# An inline `pulls/<n>/files?` fetch is the signature of a re-inlined copy. Match on the endpoint
# alone, NOT on `?limit=` — an earlier version anchored the query string, so a copy written as
# `files?page=1&limit=50` would have walked straight past a guard that exists to stop exactly
# that. Still evadable by a copy that builds the URL without a literal `?`, so this narrows the
# gap rather than closing it.
assert not re.search(r"pulls/\$?\{?\w+\}?/files\?", code), (
f"{HOOK.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — "
assert not re.search(r"pulls/\$?\{?\w+\}?/files\?", _code_lines(caller)), (
f"{caller.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — "
"that is the duplication ersatztv#649 removed")
# --- The ENFORCED caller's own preconditions ---------------------------------------------------
def _workflow_steps():
import yaml
wf = yaml.safe_load(WORKFLOW.read_text())
return wf["jobs"]["set-verdict-status"]["steps"]
def _classify_step():
for step in _workflow_steps():
if "review-verdict/h10" in (step.get("run") or ""):
return step
raise AssertionError("no step in review-verdict.yml posts review-verdict/h10")
def test_the_workflow_checks_out_the_BASE_ref_never_the_head():
"""The self-rewrite hole. This workflow judges the PR, so the PR must not supply the code that
judges it: checking out `head.sha` would let a PR rewrite `scripts/pr-changed-files.sh` to
return an empty list and exempt itself.
Asserted through a YAML parse rather than a substring search, because the dangerous edit is
subtle — `head.sha` for `base.sha` is a nine-character diff in a `with:` block — and a
text-level `"base.sha" in text` check would still pass if a second checkout step took the head
afterwards and won.
"""
checkouts = [s for s in _workflow_steps() if "actions/checkout" in (s.get("uses") or "")]
assert len(checkouts) == 1, (
f"expected exactly one checkout step, found {len(checkouts)} — a second checkout can "
"silently overwrite the base ref with the PR head")
with_ = checkouts[0].get("with") or {}
ref = str(with_.get("ref", ""))
assert "pull_request.base.sha" in ref, (
f"the checkout ref is {ref!r}; it must be the PR's BASE sha, so a PR cannot rewrite the "
"gate that judges it")
assert "head" not in ref, f"the checkout ref {ref!r} references the PR head"
assert with_.get("persist-credentials") is False, (
"persist-credentials must be false — nothing here pushes, and a token left in .git/config "
"is handed to every script the job runs")
def test_the_workflow_runs_the_jq_preflight_in_FLOOR_mode_only():
"""`--expect` pins an exact jq version and fails when it drifts. That is right for the advisory
`script-tests` job and catastrophic here: this workflow writes `review-verdict/h10`, a REQUIRED
check on `main`, so a pin would turn any jq upgrade on the runner into a repo-wide merge
deadlock — a required gate failing because an upstream package manager did its job.
"""
# CODE only, for the reason `_code_lines` documents: the first draft of this assertion read the
# raw text and went red on the workflow's own comment explaining why `--expect` is banned here.
code = _code_lines(WORKFLOW)
# A bare `"jq-preflight.sh" in code` is NOT enough, and cold review was right to say so: the
# path also appears in the `if [ -x ./scripts/jq-preflight.sh ]` presence guard, so deleting the
# actual invocation would leave that substring behind and the assertion green. Require a line
# that INVOKES it.
steps = [s for s in _workflow_steps() if "jq-preflight.sh" in (s.get("run") or "")]
assert steps, (
"review-verdict.yml no longer runs the jq preflight, so the version its shell gates run "
"under is unobservable again (ersatztv#648)")
invocations = [ln.strip() for ln in steps[0]["run"].splitlines()
if re.match(r"^\s*(\./)?scripts/jq-preflight\.sh(\s|$)", ln)]
assert invocations, "the jq preflight is referenced but never actually invoked"
assert "--expect" not in code, (
"review-verdict.yml must run jq-preflight.sh in floor-only mode; --expect here deadlocks "
"every merge on `main` the day the runner's jq changes")
assert all("--expect" not in ln for ln in invocations)
# --- The ENFORCED caller's contract, EXECUTED ---------------------------------------------------
#
# The structural guards above prove the workflow *calls* the shared script. They cannot prove it
# reacts correctly when the script FAILS — and that is precisely the mutation that survived the last
# round on the hook side: making the caller ignore the exit status left the entire suite green,
# because every failure path also happened to produce empty stdout. So the stub below FAILS while
# emitting a perfectly docs-only list, the one combination that redundancy cannot absorb.
#
# The step's `run:` block is extracted from the YAML and executed directly. That is a real
# behavioural test of the shipped text — not a paraphrase of it — at the cost of not exercising the
# runner's step wiring, which no local test can reach anyway.
WORKFLOW_STUB_CURL = r'''#!/usr/bin/env python3
import json, os, pathlib, sys
args = sys.argv[1:]
url = [a for a in args if a.startswith("http")][-1]
out = pathlib.Path(os.environ["STUB_DIR"])
if "-X" in args and args[args.index("-X") + 1] == "POST":
payload = args[args.index("-d") + 1]
(out / "posted.json").write_text(payload)
(out / "posted_url.txt").write_text(url)
print("{}")
sys.exit(0)
if "/status" in url:
# No verdict yet for this head — the case where the job goes on to classify.
print(json.dumps({"statuses": []}))
sys.exit(0)
print("{}")
'''
def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy"):
"""Execute the workflow's classify `run:` block with a stubbed enumeration script.
Returns the status payload the job POSTed, or None if it posted nothing.
"""
bindir = tmp_path / "bin"; bindir.mkdir()
curl = bindir / "curl"; curl.write_text(WORKFLOW_STUB_CURL); curl.chmod(0o755)
scripts = tmp_path / "scripts"; scripts.mkdir()
if enum_stub is not None:
enum = scripts / "pr-changed-files.sh"
enum.write_text(enum_stub)
enum.chmod(0o755)
env = dict(os.environ)
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
env["STUB_DIR"] = str(tmp_path)
env.update({
"GITEA_TOKEN": "stub",
"BASE_URL": "http://gitea.example/api/v1",
"GITEA_BASE_URL": "http://gitea.example/api/v1",
"REPO": "timothy/ersatztv",
"PR": "42",
"SHA": SHA,
"BASE_SHA": OTHER_SHA,
"AUTHOR": author,
"PR_URL": "http://gitea.example/timothy/ersatztv/pulls/42",
})
script = tmp_path / "step.sh"
script.write_text(_classify_step()["run"])
r = subprocess.run(["bash", str(script)], cwd=tmp_path, env=env,
capture_output=True, text=True)
# Assert the WIRING, not only the classification. The stub accepts every POST, so a status aimed
# at the wrong endpoint, sha, host or repo would otherwise leave these tests green while the real
# required check was never written.
#
# Two ways this check could disable itself, both found by cold review of an earlier draft:
# * it was guarded by `if url_file.exists()`, so deleting the recorder in the stub turned it
# into a no-op and every test stayed green — a verifier that silently opts out;
# * it compared only the URL SUFFIX, so a POST to the right path on the WRONG HOST OR REPO
# passed. Compare the whole URL against the env this job was given.
posted = tmp_path / "posted.json"
url_file = tmp_path / "posted_url.txt"
if posted.exists():
assert url_file.exists(), (
"a status was POSTed but its URL was not recorded — the wiring assertion below would "
"have silently skipped")
expected = f"{env['BASE_URL']}/repos/{env['REPO']}/statuses/{SHA}"
assert url_file.read_text().strip() == expected, (
f"the status was POSTed to {url_file.read_text().strip()!r}, expected {expected!r}")
return (json.loads(posted.read_text()) if posted.exists() else None), r
DOCS_ONLY = 'printf "docs/a.md\\ndocs/b.md\\n"\n'
def test_a_FAILING_enumeration_withholds_the_exemption_even_when_stdout_looks_docs_only(tmp_path):
"""The mutation that previously survived: ignore the exit status, trust stdout."""
posted, r = _run_classify(tmp_path, "#!/usr/bin/env bash\n" + DOCS_ONLY + "exit 1\n")
assert posted is not None, f"the job posted no status at all: {r.stderr}"
assert posted["state"] == "pending", (
f"a docs-only exemption was granted from the stdout of a script that FAILED "
f"(state={posted['state']}, desc={posted['description']!r}) — the exit status is not being "
"checked")
def test_workflow_positive_control_the_same_output_with_exit_0_DOES_exempt(tmp_path):
"""Without this, the test above could pass for any unrelated reason and prove nothing."""
posted, r = _run_classify(tmp_path, "#!/usr/bin/env bash\n" + DOCS_ONLY + "exit 0\n")
assert posted is not None, f"the job posted no status at all: {r.stderr}"
assert posted["state"] == "success", (
"the positive control failed, so the negative test above cannot be trusted to be measuring "
f"the exit status at all (state={posted['state']}, desc={posted['description']!r})")
def test_a_crashing_enumeration_also_withholds_the_exemption(tmp_path):
posted, _ = _run_classify(tmp_path, "#!/usr/bin/env bash\n" + DOCS_ONLY + "kill -TERM $$\n")
assert posted is not None
assert posted["state"] == "pending"
def test_a_MISSING_enumeration_script_withholds_the_exemption(tmp_path):
"""A PR whose BASE predates the script's introduction. It must post an actionable `pending`
rather than dying with no status — an absent required check blocks the merge either way, but a
stalled PR with no explanation is how a gate acquires a reputation for being flaky.
"""
posted, r = _run_classify(tmp_path, None)
assert posted is not None, f"the job posted no status at all: {r.stderr}"
assert posted["state"] == "pending"
def _emitting(*paths):
body = "".join(f'printf "%s\\n" "{p}"\n' for p in paths)
return "#!/usr/bin/env bash\n" + body + "exit 0\n"
def test_a_code_file_defeats_the_docs_only_exemption(tmp_path):
"""The workflow's own classification — deliberately NOT shared with the hook, whose allow-list
is wider because a match there falls through to a human prompt rather than a green status.
"""
posted, _ = _run_classify(tmp_path, _emitting("docs/a.md", "ErsatzTV/Program.cs"))
assert posted is not None
assert posted["state"] == "pending"
def test_a_BOT_pr_touching_a_protected_path_is_NOT_exempt(tmp_path):
"""`PROTECTED` guards the BOT exemption specifically, and mutation testing is how that got
stated correctly. The first version of this test used a docs-only+protected file list and
passed even with the `PROTECTED` clause deleted — `PROTECTED` (`.claude/ .gitea/ .husky/
scripts/ docker/ci/`) and `DOCS_ONLY` (`docs/`, root `*.md`) are DISJOINT, so on the docs-only
path that clause can never fire and the `DOCS_ONLY` check was doing all the work. The test
looked like it covered the self-exemption hole and covered nothing.
Renovate lands patch bumps unattended via Gitea's own auto-merge, so a bot PR that edits the
gate, CI, the hooks, or the scripts they call is the one path where an unreviewed change to the
merge gate could actually merge itself.
"""
posted, _ = _run_classify(
tmp_path, _emitting("ErsatzTV/Program.cs", "scripts/pr-changed-files.sh"),
author="renovate")
assert posted is not None
assert posted["state"] == "pending", (
"a bot PR editing the shared enumeration was auto-exempted — a PR that weakens the merge "
"gate must never be able to exempt itself from the merge gate")
def test_bot_positive_control_a_plain_bot_pr_IS_exempt(tmp_path):
"""Proves the test above measures `PROTECTED` and not merely 'bot PRs are never exempt'."""
posted, _ = _run_classify(
tmp_path, _emitting("Directory.Packages.props"), author="renovate")
assert posted is not None
assert posted["state"] == "success", (
"the bot exemption never fires at all, so the protected-path test above proves nothing")