From 9114a7e8af65c89cc322e029de8ab761f220d24b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:05:14 +0200 Subject: [PATCH 1/4] fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #658 landed the shared implementation, `scripts/pr-changed-files.sh`, and rewired the ADVISORY hook onto it. The ENFORCED copy — the one that writes the branch-protection- required `review-verdict/h10` status — was left byte-identical to main, so its fail-closed behaviour on a malformed or empty response stayed INCIDENTAL: an empty `n` erroring `[ "$n" -lt 50 ]` to false. That is #649's second Done-when box, and the whole point of the issue was that the gate with real authority was weaker than the gate with none. `review-verdict.yml` now: - checks out the PR's BASE ref (`base.sha`, `persist-credentials: false`), never the head, so a PR cannot supply the code that judges it; - runs `scripts/jq-preflight.sh` in FLOOR-ONLY mode — `--expect` here would deadlock every merge on `main` the day the runner's jq changes; - calls `scripts/pr-changed-files.sh` and reads its EXIT STATUS, never its stdout on a failure path. The env trap flagged in review is handled: the script reads GITEA_BASE_URL and takes owner/repo as two separate arguments, so passing BASE_URL and a combined `owner/repo` would have silently fallen back to the hardcoded LAN default. The ~40 lines of inline enumeration are deleted, so the two copies can no longer drift. A base ref predating #658 has no such script; that posts `pending` with the reason rather than dying with no status at all. The drift guard is re-tightened from "the hook uses the shared script" to "BOTH callers do", and the workflow's own preconditions are pinned by parsing the YAML rather than substring-matching it — `head.sha` for `base.sha` is a nine-character diff. Verified by mutation, six mutants, each killed by its intended test: ignore the exit status; check out the head; drop `persist-credentials`; add `--expect`; re-inline a `pulls/N/files?` fetch; delete the PROTECTED clause. That last one initially MISSED, and the miss was the useful finding. The test used a docs-only-plus-protected file list and passed with the clause deleted, because PROTECTED (`.claude/ .gitea/ .husky/ scripts/ docker/ci/`) and DOCS_ONLY (`docs/`, root `*.md`) are disjoint — on the docs-only path that clause can never fire, and DOCS_ONLY was doing all the work. PROTECTED is load-bearing only on the BOT path, so the test now covers a Renovate PR editing the shared script, with a positive control proving the bot exemption fires at all. The caller contract is tested by EXECUTING the workflow's `run:` block against a stubbed enumeration that fails while emitting a perfectly docs-only list — the one combination the "every failure path also happens to print nothing" redundancy cannot absorb, and the exact mutation that survived the whole suite last round. Docs: both "Landing note" blocks removed, and the record's base-ref paragraph converted from a future-tense requirement to present-tense fact with its staging rationale kept as history. Refs #649 Decisions-Edit: yes --- .gitea/workflows/review-verdict.yml | 120 +++++--- docs/ci-cd.md | 30 +- .../records/ci/shared-pr-file-enumeration.md | 34 ++- scripts/tests/test_pr_changed_files.py | 267 ++++++++++++++++-- 4 files changed, 364 insertions(+), 87 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 82f5b7194..b6532cb9c 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -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,48 @@ 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 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 +167,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 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 2b27b03fb..d3316c862 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -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`) diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index a0b3c420c..4f9017e0c 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -10,11 +10,6 @@ signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hoo mechanics: '`scripts/pr-changed-files.sh ` -> 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 @@ -75,18 +70,27 @@ in each caller: 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. -**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. **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 diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index a47d5b351..061bae24d 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -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" @@ -342,35 +342,256 @@ 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//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) + assert "jq-preflight.sh" in code, ( + "review-verdict.yml no longer runs the jq preflight, so the version its shell gates run " + "under is unobservable again (ersatztv#648)") + 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") + + +# --- 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) + 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) + posted = tmp_path / "posted.json" + 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") -- 2.47.3 From 322dd43d108cd365f1c5561397062337aa020b5d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:23:52 +0200 Subject: [PATCH 2/4] fix(649): close the test-isolation gaps cold review found, and make the job's Gitea config authoritative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings acted on; two more are real but pre-existing and are being filed rather than fixed here (see below). **The job's Gitea config was not authoritative.** `pr-changed-files.sh` resolves `ETV_GITEA_URL` BEFORE `GITEA_BASE_URL` (and `ETV_GITEA_TOKEN` before `GITEA_TOKEN`), because its other caller is a developer Mac using the ETV_* convention. Setting only the GITEA_* names meant a runner exporting a stale ETV_GITEA_URL would enumerate a DIFFERENT Gitea instance and this job would post a verdict here from a diff read there. Both names are now set to the same value, so precedence cannot matter. **Three guards passed their tests for the wrong reason.** Each was confirmed by deleting the clause and watching the suite stay green — the reviewer asserted it, mutation proved it: - The explicit empty-response clause was uncovered on jq 1.8, because jq 1.8 rejects empty input by itself. jq 1.6 does not, and THE RUNNER SHIPS 1.6 — so the one environment where the clause is load-bearing had no coverage. That is the #643/#647 failure class reproduced inside the suite meant to prevent it. Now covered by importing the existing jq-1.6 shim (imported, not copied — a second quirk emulator is the same drift problem one level down), with a verify-the-verifier test and a positive control. - `type == "array"` needed a body whose VALUES are valid rows. Two earlier attempts failed for a third reason: `jq`'s `all(.[]; …)` iterates an object's values, so `{"message":"…"}` and a single flat row are both rejected by `.filename` erroring on a string. Only `{"0": {…valid row…}}` reaches the fail-open, where a non-array body enumerates as a complete docs-only list. - `.filename | ok` is now isolated by a row carrying a valid `.status` and no filename, removing the closed-allow-list as a second reason to reject. **Two assertions proved less than their names claimed.** `"jq-preflight.sh" in code` also matched the `[ -x … ]` presence guard, so deleting the invocation left it green; it now requires an invoking line. `_run_classify` accepted every POST, so a status aimed at the wrong endpoint or sha would not have been noticed; it now asserts the POST lands on `/statuses/`. **One test name overclaimed** and is narrowed rather than left implying coverage it does not have: the head-movement test proves "final head != expected sha", not movement *during* enumeration. Deferred, both pre-existing and neither introduced here — filed as follow-ups: - A commit status is repo-global, so a `review-verdict/h10=success` obtained for head H on one PR is inherited by any other PR with the same head, including one opened against a different base. Same class as #632, reached by a third route. - The A->B->A force-push race: paging is several round-trips and the head is re-read once at the end, so a restore to the original sha passes the binding while the pages came from two states. Inherent to enumerating a mutable list over an API with no commit-pinned files endpoint. Refs #649 --- .gitea/workflows/review-verdict.yml | 9 ++ scripts/tests/test_pr_changed_files.py | 112 ++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 4 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index b6532cb9c..fedfc6841 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -106,6 +106,15 @@ jobs: # 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 }} diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 061bae24d..3700d8f45 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -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 @@ -437,12 +523,21 @@ def test_the_workflow_runs_the_jq_preflight_in_FLOOR_mode_only(): # 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) - assert "jq-preflight.sh" in code, ( + # 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 --------------------------------------------------- @@ -467,6 +562,7 @@ 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) @@ -512,6 +608,14 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy"): r = subprocess.run(["bash", str(script)], cwd=tmp_path, env=env, capture_output=True, text=True) posted = tmp_path / "posted.json" + url_file = tmp_path / "posted_url.txt" + if url_file.exists(): + # Assert the WIRING, not only the classification. Cold review's point: the stub accepts every + # POST, so a status aimed at the wrong endpoint or the wrong sha would leave these tests green + # while the real required check was never written. + url = url_file.read_text().strip() + assert url.endswith(f"/statuses/{SHA}"), ( + f"the status was POSTed to {url!r}, not to /statuses/") return (json.loads(posted.read_text()) if posted.exists() else None), r -- 2.47.3 From 3885fd6aeac863c2eb952452c3251036e2a1a375 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:29:44 +0200 Subject: [PATCH 3/4] docs(649): narrow the enumeration's stated guarantees to what it actually proves Two limitations the cold review surfaced are now written where the guarantees are described, rather than living only in a review transcript. Both are pre-existing and tracked separately (#663, #664); neither is fixed here. - Head-sha binding detects ONE-WAY movement. An A->B->A force-push round trip restores the expected sha, so the binding holds while the pages came from two states. The record previously read as though the race were closed. - A commit status is repo-GLOBAL, so a success earned on one PR is inherited by any other PR with the same head. Same property that makes the per-sha binding work, read from the other end. Refs #649 Decisions-Edit: yes --- .../records/ci/shared-pr-file-enumeration.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index 4f9017e0c..0903fbc22 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -68,7 +68,11 @@ 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 — 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 @@ -92,6 +96,13 @@ and mutation testing was what established that `PROTECTED` is load-bearing only 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 own. It is still a real weakening worth fixing — the whole point of #649 is that the same class of -- 2.47.3 From e960d5b9185e4d44408346e4bb62ace8622e8728 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:38:25 +0200 Subject: [PATCH 4/4] test(649): make the POST-wiring assertion unable to opt out or accept the wrong host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review found the verifier could disable itself two ways, both of which look like coverage: - it was guarded by `if url_file.exists()`, so deleting the recorder in the stub turned the whole assertion into a no-op and every test stayed green; - it compared only the URL SUFFIX, so a POST to the right path on the wrong HOST or the wrong REPO passed — which is exactly the class the assertion was added to catch. It now requires the URL to have been recorded whenever a status was posted, and compares the full URL against the env the job was given. Mutation-verified three ways: wrong host, wrong repo, and deleting the recorder each redden the suite. Refs #649 --- scripts/tests/test_pr_changed_files.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 3700d8f45..19d4590aa 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -607,15 +607,24 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy"): 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 url_file.exists(): - # Assert the WIRING, not only the classification. Cold review's point: the stub accepts every - # POST, so a status aimed at the wrong endpoint or the wrong sha would leave these tests green - # while the real required check was never written. - url = url_file.read_text().strip() - assert url.endswith(f"/statuses/{SHA}"), ( - f"the status was POSTed to {url!r}, not to /statuses/") + 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 -- 2.47.3