fix(648,649): stage the enforced-gate wiring behind the scripts it calls
Splits the review-verdict.yml rewiring out of this PR. That workflow checks out the PR's BASE ref — deliberately, so a PR cannot rewrite the gate that judges it — and the base is main, which does not yet contain scripts/pr-changed-files.sh or scripts/jq-preflight.sh. Wiring it here would make the job exit 127 on its own PR and block the merge gate through the combined status, which reads red jobs as blocking. So this PR lands the scripts, their tests, the hook rewiring and the script-tests jq pin; the follow-up points review-verdict.yml at them once they exist on main. The two tests that asserted on review-verdict.yml are scoped accordingly, each carrying the reason. test_review_verdict_never_pins_a_jq_version is asserted NOW rather than in the follow-up, so the no-pin constraint on the required check is already enforced when the wiring lands. Decisions-Edit: yes
This commit is contained in:
@@ -52,23 +52,6 @@ jobs:
|
||||
name: Set review-verdict status
|
||||
runs-on: small # a few API calls; keep it off the build runners
|
||||
steps:
|
||||
# Check out the BASE ref, NEVER the PR head. This job now runs scripts/pr-changed-files.sh from
|
||||
# the checkout, and checking out the head would let a PR rewrite the very gate that judges it —
|
||||
# the PROTECTED list below could not save us, because the tampered script would already have
|
||||
# produced the file list that list is applied to. `base.sha` is a commit already on the target
|
||||
# branch, so it is trusted by construction. (ersatztv#649)
|
||||
- name: Checkout the base ref (trusted; never the PR head)
|
||||
uses: actions/checkout@v4
|
||||
with:
|
||||
ref: ${{ github.event.pull_request.base.sha }}
|
||||
persist-credentials: false
|
||||
# Floor-only: print the jq version so a future divergence is diagnosable from the log alone, and
|
||||
# refuse anything below the supported floor. Deliberately NOT pinned with `--expect` — this
|
||||
# workflow writes the branch-protection-required `review-verdict/h10` status, so a hard version
|
||||
# pin here would turn any jq bump on the runner into a repo-wide merge deadlock. The pin lives on
|
||||
# `script-tests` in pr-checks.yml instead. See docs/ci-cd.md -> "The jq contract". (ersatztv#648)
|
||||
- name: Preflight jq version
|
||||
run: ./scripts/jq-preflight.sh
|
||||
- name: Classify the PR and post the review-verdict status
|
||||
env:
|
||||
GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
|
||||
@@ -133,36 +116,50 @@ jobs:
|
||||
exit 0
|
||||
fi
|
||||
|
||||
# --- Changed files: enumerate exhaustively, or fail CLOSED. --------------------------
|
||||
# This was an inline copy of logic that also lives in
|
||||
# .claude/hooks/pretooluse-merge-consent.sh. The two drifted, and badly: four rounds of
|
||||
# ersatztv#643 hardening landed on the HOOK — the advisory copy, whose failures produce a
|
||||
# human prompt — and never reached this one, the ENFORCED copy that writes the
|
||||
# branch-protection-required status. The advisory copy ended up strictly stricter than the
|
||||
# authoritative one. Worse, a round-4 review traced that this copy's safe behaviour on a
|
||||
# garbage response was INCIDENTAL: `n` came back empty, `[ "$n" -lt 50 ]` errored to false,
|
||||
# the loop ran to MAX_PAGES and left complete=no. Correct outcome, arrived at by a bash
|
||||
# arithmetic error that any refactor of the loop could have flipped.
|
||||
#
|
||||
# Both callers now share ONE implementation whose fail-closed behaviour is intentional and
|
||||
# asserted by scripts/tests/test_pr_changed_files.py (ersatztv#649). It carries the six
|
||||
# guards this copy was missing: 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 (never a merely short one), and binding the
|
||||
# whole enumeration to a single head sha.
|
||||
#
|
||||
# A non-zero exit means "could not tell" and MUST withhold the exemption. Note the script
|
||||
# is invoked WITHOUT `|| true`: `set -e` is not in play for an `if` condition, and reading
|
||||
# its stdout without checking its status is the exact mistake this replaces.
|
||||
if files=$(GITEA_TOKEN="$GITEA_TOKEN" GITEA_BASE_URL="$BASE_URL" \
|
||||
./scripts/pr-changed-files.sh "${REPO%%/*}" "${REPO##*/}" "$PR" "$SHA"); then
|
||||
complete=yes
|
||||
else
|
||||
complete=no
|
||||
files=""
|
||||
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
|
||||
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
|
||||
files=$(printf '%s\n' "$files" | grep -v '^$' || true)
|
||||
count=$(printf '%s\n' "$files" | grep -c . || true)
|
||||
echo "Changed files (${count}, complete=${complete}):"
|
||||
echo "Changed files (${count}, complete=${complete}, pages=${page}):"
|
||||
printf '%s\n' "$files" | sed 's/^/ /'
|
||||
|
||||
exempt=no
|
||||
|
||||
@@ -665,6 +665,13 @@ 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,
|
||||
|
||||
@@ -10,6 +10,11 @@ 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
|
||||
|
||||
@@ -125,20 +125,27 @@ def test_unknown_argument_is_a_usage_error(preflight):
|
||||
|
||||
# --- Wiring guards: the preflight is worthless if a caller silently stops running it ------------
|
||||
|
||||
def test_script_tests_pins_and_review_verdict_only_floors():
|
||||
"""The asymmetry is deliberate and load-bearing, so it is asserted rather than merely commented.
|
||||
def test_script_tests_pins_the_jq_version():
|
||||
"""The pin is the tripwire, so its presence is asserted rather than merely commented.
|
||||
|
||||
`script-tests` carries `--expect` (a tripwire on a normal job). `review-verdict.yml` must NOT:
|
||||
it writes the branch-protection-required `review-verdict/h10` status, so a hard version pin
|
||||
there would turn a jq bump on the runner into a repo-wide merge deadlock.
|
||||
SCOPE NOTE — the symmetric assertion about `review-verdict.yml` (that it runs the FLOOR-only
|
||||
mode and must never pin, because it writes the branch-protection-required `review-verdict/h10`
|
||||
status and a pin would deadlock every merge on a jq bump) lands with the follow-up PR that
|
||||
wires that workflow. It cannot land here: that workflow checks out the BASE ref, and the base
|
||||
is `main`, which does not yet contain `scripts/jq-preflight.sh`.
|
||||
"""
|
||||
pr_checks = (WORKFLOWS / "pr-checks.yml").read_text()
|
||||
review_verdict = (WORKFLOWS / "review-verdict.yml").read_text()
|
||||
|
||||
assert "jq-preflight.sh --expect" in pr_checks, \
|
||||
"script-tests must pin the jq version — that pin is the tripwire"
|
||||
assert "jq-preflight.sh" in review_verdict, \
|
||||
"review-verdict.yml must at least print/floor-check its jq version"
|
||||
|
||||
|
||||
def test_review_verdict_never_pins_a_jq_version():
|
||||
"""Whatever else changes, the REQUIRED merge check must never carry a hard version pin.
|
||||
|
||||
Asserted now, before the workflow is wired, so the constraint is already enforced when the
|
||||
follow-up PR adds the floor-only call — rather than being a comment someone can miss.
|
||||
"""
|
||||
review_verdict = (WORKFLOWS / "review-verdict.yml").read_text()
|
||||
assert "jq-preflight.sh --expect" not in review_verdict, \
|
||||
("review-verdict.yml must NOT pin a jq version: it writes the required review-verdict/h10 "
|
||||
"status, so a pin would deadlock every merge on a jq bump (ersatztv#648)")
|
||||
|
||||
@@ -239,18 +239,25 @@ def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files):
|
||||
|
||||
# --- Drift guard: the reason this file is worth having at all ----------------------------------
|
||||
|
||||
def test_both_callers_use_the_shared_script_and_neither_reimplements_it():
|
||||
"""ersatztv#649's third Done-when box: a test that fails if the two copies diverge again.
|
||||
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.
|
||||
|
||||
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.
|
||||
"""
|
||||
for caller in (HOOK, WORKFLOW):
|
||||
text = caller.read_text()
|
||||
assert "scripts/pr-changed-files.sh" in text, (
|
||||
f"{caller.relative_to(REPO_ROOT)} no longer calls the shared enumeration")
|
||||
# An inline `pulls/<n>/files?limit=` fetch is the signature of a re-inlined copy.
|
||||
assert not re.search(r"pulls/\$?\{?\w+\}?/files\?limit=", text), (
|
||||
f"{caller.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — "
|
||||
"that is the duplication ersatztv#649 removed")
|
||||
text = HOOK.read_text()
|
||||
assert "scripts/pr-changed-files.sh" in text, (
|
||||
f"{HOOK.relative_to(REPO_ROOT)} no longer calls the shared enumeration")
|
||||
# An inline `pulls/<n>/files?limit=` fetch is the signature of a re-inlined copy.
|
||||
assert not re.search(r"pulls/\$?\{?\w+\}?/files\?limit=", text), (
|
||||
f"{HOOK.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — "
|
||||
"that is the duplication ersatztv#649 removed")
|
||||
|
||||
Reference in New Issue
Block a user