fix(648,649): one shared PR-file enumeration + an explicit jq version contract
#649 — the enforced review-verdict.yml guard had drifted strictly WEAKER than the advisory merge-consent hook: four rounds of #643 hardening landed on the copy whose failures produce a human prompt, and never reached the copy that writes the branch-protection-required review-verdict/h10 status. Its fail-closed behaviour on a garbage response was also incidental (an empty `n` erroring a bash conditional to false), not designed. Extract scripts/pr-changed-files.sh as the single implementation both call. Shared MECHANISM, not policy: the two docs-only allow-lists differ deliberately and stay separate. review-verdict.yml now checks out the BASE ref, never the PR head, so a PR cannot rewrite the gate that judges it. #648 — baking jq into docker/ci/Dockerfile provably cannot cover the gate that broke: review-verdict.yml is runs-on:small with no toolchain pin, so it gets the host's jq 1.6 (checked, not assumed). Add scripts/jq-preflight.sh: floor+observable everywhere, and a --expect tripwire on script-tests only — pinning the required merge check would deadlock every merge on a jq bump. Verified by mutation: six guards individually broken, each turning exactly its own test red, then restored byte-identical. fixes #648 fixes #649
This commit is contained in:
@@ -85,109 +85,31 @@ sha=$(printf '%s' "$prjson" | jq -r '.head.sha // ""' 2>/dev/null || true)
|
||||
body=$(printf '%s' "$prjson" | jq -r '.body // ""' 2>/dev/null || true)
|
||||
|
||||
# --- Docs-only exemption: if every changed file is docs/process, skip the gate. ---
|
||||
# The file list must be enumerated EXHAUSTIVELY or the exemption is unsafe. 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), so the previous single-page read could see 50
|
||||
# docs files, miss the code in positions 51+, and exempt a PR that is not remotely docs-only.
|
||||
# Page until a short page proves the end; anything else leaves `files_complete=no`, which withholds
|
||||
# the exemption and falls through to the full gate (ersatztv#622).
|
||||
files=""; files_complete=no; page=1
|
||||
while [ "$page" -le 40 ]; do
|
||||
raw=$(gq "repos/$owner/$repo/pulls/$pr/files?limit=50&page=$page")
|
||||
# A transport/parse failure must not look like a legitimate short final page: `gq` returns empty
|
||||
# on any error, which counts as zero rows and would set files_complete=yes over a PARTIAL list —
|
||||
# failing OPEN into the exemption.
|
||||
#
|
||||
# Checking only the top-level type leaves the same hole one level down: `[{}]` is a valid array
|
||||
# whose rows carry no `filename`, so it yields no paths, looks like a short page, and completes
|
||||
# the enumeration from a partial list. Require every row to carry a non-empty string `filename`
|
||||
# (an empty array is still valid — that is a genuine end-of-pagination). This also rejects arrays
|
||||
# of scalars, which would otherwise make the `.filename` extraction below fail under `set -e`.
|
||||
#
|
||||
# An EMPTY body is rejected EXPLICITLY here rather than left to jq's exit status, because that
|
||||
# status is not portable: `jq -e` over empty input exits 4 on jq >= 1.7 but **0 on jq 1.6**
|
||||
# (verified against both binaries — ersatztv#631). Relying on it made this guard fail OPEN on any
|
||||
# host with the older jq, including the CI runner, which ships jq 1.6. The chain: a transport
|
||||
# failure makes `gq` return empty, the jq guard wrongly passes, `n` is empty so `[ "$n" -lt 50 ]`
|
||||
# errors into false, the loop walks PAST the failed page, the NEXT page legitimately returns `[]`,
|
||||
# and `files_complete=yes` is set over a PARTIAL list — exempting a PR whose unread pages may be
|
||||
# pure code. That is the very defect the paragraph above describes, reintroduced one layer down.
|
||||
if [ -z "${raw//[[:space:]]/}" ]; then
|
||||
files_complete=no; break
|
||||
fi
|
||||
#
|
||||
# CR/LF in a path is REJECTED outright (ersatztv#643 review). `chunk` below flattens paths into
|
||||
# newline-delimited text, so a filename containing a newline splits into TWO lines that are each
|
||||
# matched against the allow-list separately: `"safe.md\ndocs/Program.cs"` yields `safe.md` and
|
||||
# `docs/Program.cs`, both of which pass, while the actual single path ends in `.cs`. Git permits
|
||||
# newlines in filenames, so this is reachable, and it was reproduced against this hook. Failing
|
||||
# closed on control characters is the cheap fix; no decision/docs path ever contains one.
|
||||
#
|
||||
# VALIDATE EVERY FIELD THE EXTRACTION BELOW CONSUMES. `chunk` emits `(.previous_filename //
|
||||
# empty)` for EVERY row regardless of `.status`, so validating that field only on `renamed` rows
|
||||
# left a hole one predicate wide: a row with `status: "modified"` (or Gitea's distinct `copied`)
|
||||
# carrying a newline in `previous_filename` was reproducibly exempted. The rule this encodes:
|
||||
# the validation domain must match the CONSUMPTION domain, not the domain the field is
|
||||
# semantically "supposed to" appear in. The `renamed` => REQUIRED clause is kept on top of the
|
||||
# unconditional if-present check.
|
||||
#
|
||||
# `..` is rejected for the same reason: the allow-list anchors `^docs/`, so
|
||||
# `docs/../ErsatzTV/Program.cs` matches it. Git will not produce such a path, but this guard's
|
||||
# whole job is to fail closed on unexpected 2xx shapes rather than to assume a well-behaved peer.
|
||||
#
|
||||
# `.status` is checked against a CLOSED set, verified against live Gitea 1.25.4 output:
|
||||
# added|deleted|changed|renamed|copied. Without it, the `renamed => previous_filename REQUIRED`
|
||||
# clause could be dodged by any other value — `"Renamed"` with a capital R, or an absent status —
|
||||
# letting a `git mv ErsatzTV/Program.cs -> docs/a.md` drop its source path and read as docs-only.
|
||||
# An unknown status now fails closed rather than silently taking the `else true` branch.
|
||||
#
|
||||
# `modified` is accepted ALONGSIDE `changed` deliberately. Live Gitea 1.25.4 emits `changed`, but
|
||||
# a closed allow-list built from the wrong vocabulary is a worse failure than the hole it closes:
|
||||
# it would gate every genuine docs-only PR, on every version that spells it differently. The
|
||||
# security property here is "reject values we do not recognise", not "enumerate one version
|
||||
# exactly", so the set errs toward accepting plausible synonyms.
|
||||
if ! printf '%s' "$raw" \
|
||||
| jq -e 'def ok: type == "string" and length > 0
|
||||
and (test("[\\r\\n]") | not)
|
||||
and (split("/") | index("..") | not);
|
||||
type == "array" and all(.[];
|
||||
(.filename | ok)
|
||||
and (.previous_filename == null or (.previous_filename | ok))
|
||||
and ((.status // "") as $s | ($s | type) == "string"
|
||||
and (["added","deleted","changed","modified","renamed","copied"] | index($s)) != null)
|
||||
and (if .status == "renamed"
|
||||
then (.previous_filename | type == "string" and length > 0)
|
||||
else true end))' \
|
||||
>/dev/null 2>&1; then
|
||||
files_complete=no; break
|
||||
fi
|
||||
# BOTH sides of a rename: Gitea reports a `git mv` as ONE row whose `filename` is the DESTINATION,
|
||||
# with the source in `previous_filename`. Reading only `filename` would let a PR move code into
|
||||
# docs/ and claim the docs-only exemption. Page size is measured in ROWS, not paths — one renamed
|
||||
# row is one row but two paths.
|
||||
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")
|
||||
# Terminate ONLY on an explicitly validated EMPTY page — never on a merely SHORT one
|
||||
# (ersatztv#643 review). "Fewer than 50 rows means last page" assumes the server's page size is
|
||||
# the 50 we asked for, but Gitea caps `limit` at the server-wide `MAX_RESPONSE_ITEMS` (default 50,
|
||||
# configurable) and is free to return fewer. A 30-row page followed by a page of code would set
|
||||
# files_complete=yes over a PARTIAL list — the same fail-open, reached without any transport error.
|
||||
# Costs one extra request per enumeration; the `page <= 40` cap still fails closed.
|
||||
if [ "$n" -eq 0 ]; then files_complete=yes; break; fi
|
||||
page=$((page + 1))
|
||||
done
|
||||
files=$(printf '%s\n' "$files" | grep -v '^$' || true)
|
||||
# Bind the enumeration to ONE head (ersatztv#643 review). Paging is several round-trips; a
|
||||
# force-push between them means page 1 came from head A and page 2 from head B, so the assembled
|
||||
# list belongs to no single commit — B's code page can be skipped entirely while B's docs page
|
||||
# reads as a clean short tail. Re-read the head and refuse the exemption if it moved.
|
||||
if [ "$files_complete" = yes ]; then
|
||||
sha_after=$(printf '%s' "$(gq "repos/$owner/$repo/pulls/$pr")" | jq -r '.head.sha // ""' 2>/dev/null || true)
|
||||
if [ -z "$sha_after" ] || [ "$sha_after" != "$sha" ]; then
|
||||
files_complete=no
|
||||
fi
|
||||
# The file list must be enumerated EXHAUSTIVELY, validated row by row, and bound to ONE head, or the
|
||||
# exemption is unsafe. ALL of that now lives in scripts/pr-changed-files.sh — the single shared
|
||||
# implementation, also called by .gitea/workflows/review-verdict.yml (ersatztv#649).
|
||||
#
|
||||
# Why it moved: this logic was written twice. This copy is ADVISORY (a failure produces a human
|
||||
# prompt); the workflow's copy is ENFORCED (it writes the branch-protection-required
|
||||
# `review-verdict/h10` status). Four rounds of ersatztv#643 hardening landed here and never reached
|
||||
# there, leaving the copy with real authority strictly weaker than the copy without — and its safe
|
||||
# behaviour resting on a bash arithmetic error rather than an intentional guard. Two copies of a
|
||||
# security predicate drift; one cannot.
|
||||
#
|
||||
# What is NOT shared, deliberately: the docs-only allow-list below. This one also lets .claude/,
|
||||
# .gitea/ and .husky/ through, which is safe HERE only because a match falls through to a human
|
||||
# prompt rather than auto-granting. The workflow's list is narrower for exactly that reason. Sharing
|
||||
# the enumeration fixes the drift; sharing the classification would erase an intended difference.
|
||||
#
|
||||
# A non-zero exit means "could not tell" and MUST withhold the exemption — never read stdout without
|
||||
# checking the status. An empty `$sha` (unparseable PR JSON) reaches the script as an empty argument
|
||||
# and is rejected there, so that path also fails closed.
|
||||
repo_root=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)
|
||||
files=""; files_complete=no
|
||||
if files=$("$repo_root/scripts/pr-changed-files.sh" "$owner" "$repo" "$pr" "$sha" 2>/dev/null); then
|
||||
files_complete=yes
|
||||
fi
|
||||
|
||||
if [ "$files_complete" = yes ] && [ -n "$files" ] && ! printf '%s\n' "$files" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then
|
||||
# Docs/process-only PR: the Done-when + review-verdict gate doesn't apply — but this exemption is a
|
||||
# file-TYPE bypass, NOT the a+b+c "provably reviewed & ready" proof, so it does NOT auto-grant. It
|
||||
|
||||
@@ -239,14 +239,24 @@ jobs:
|
||||
# as ~20 opaque assertion errors — this turns that into one actionable line.
|
||||
- name: Preflight external tools
|
||||
run: |
|
||||
missing=()
|
||||
for t in jq git; do command -v "$t" >/dev/null 2>&1 || missing+=("$t"); done
|
||||
if [ ${#missing[@]} -gt 0 ]; then
|
||||
echo "::error::script-tests needs these on PATH but they are absent: ${missing[*]}." \
|
||||
"The suite execs real shell scripts that use them. Bake them into the runner" \
|
||||
"image rather than apt-get installing here (see ersatztv#390)."
|
||||
if ! command -v git >/dev/null 2>&1; then
|
||||
echo "::error::script-tests needs git on PATH but it is absent. The suite execs real" \
|
||||
"shell scripts that use it. Bake it into the runner image rather than apt-get" \
|
||||
"installing here (see ersatztv#390)."
|
||||
exit 1
|
||||
fi
|
||||
echo "Preflight OK: $(jq --version), $(git --version)"
|
||||
echo "Preflight OK: $(git --version)"
|
||||
# jq gets its OWN step because its VERSION, not merely its presence, is load-bearing
|
||||
# (ersatztv#648). `--expect` makes this a TRIPWIRE: scripts/tests exercises the jq 1.6 code path
|
||||
# only because this runner ships 1.6, so an upgrade would silently delete that coverage — and
|
||||
# the three divergences found in ersatztv#643/#647 all lived exactly there. Going red forces an
|
||||
# explicit human decision instead of letting the coverage evaporate.
|
||||
#
|
||||
# The pin lives HERE and deliberately NOT in review-verdict.yml: that workflow writes the
|
||||
# branch-protection-required `review-verdict/h10` status, so pinning a version there would turn
|
||||
# any jq bump on the runner into a repo-wide merge deadlock. It gets the floor-only mode.
|
||||
# See docs/ci-cd.md -> "The jq contract".
|
||||
- name: Preflight jq version
|
||||
run: ./scripts/jq-preflight.sh --expect 1.6
|
||||
- name: Run scripts/tests
|
||||
run: PYTHONPATH=. python3 -m pytest scripts/tests -q
|
||||
|
||||
@@ -52,6 +52,23 @@ 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 }}
|
||||
@@ -116,50 +133,36 @@ 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
|
||||
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)
|
||||
# --- 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
|
||||
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
|
||||
|
||||
Executable
+107
@@ -0,0 +1,107 @@
|
||||
#!/usr/bin/env bash
|
||||
# Make the jq version a job's shell gates run under OBSERVABLE, and any drift LOUD.
|
||||
#
|
||||
# ersatztv#648. Every shell gate in this repo is authored and tested on a developer Mac shipping
|
||||
# jq 1.8.x. The CI runner ships jq 1.6. Nothing pinned or checked that, and until ersatztv#631 the one
|
||||
# thing that could have noticed (scripts/tests/) never ran on the runner. Three independent divergences
|
||||
# surfaced in a single day:
|
||||
#
|
||||
# ersatztv#643 `jq -e` over EMPTY input -> exit 4 on 1.8, exit 0 on 1.6 (a transport failure
|
||||
# passed the docs-only pagination guard)
|
||||
# ersatztv#647 contains("<NUL>") -> false on 1.8, TRUE for every string on 1.6
|
||||
# (the H10 verdict classifier was entirely inert)
|
||||
# ersatztv#647 parse-error exit code -> 5 on 1.8, 4 on 1.6 — same as "no output"
|
||||
# (garbage API response read as "no comments")
|
||||
#
|
||||
# All three are fixed with version-stable constructs, but patching constructs one at a time does not
|
||||
# scale: the failures share one shape — a shell gate's behaviour is a function of its interpreter's
|
||||
# version, and that version was an UNTESTED AXIS. This script makes the axis explicit.
|
||||
#
|
||||
# WHY A FLOOR AND NOT A PIN EVERYWHERE. The obvious fix — bake a pinned jq into the CI toolchain image
|
||||
# (docker/ci/Dockerfile) — provably does NOT cover the gate that actually broke. `.gitea/workflows/
|
||||
# review-verdict.yml` is `runs-on: small`, carries no toolchain-image pin, and per `ci.small-lane-git-only`
|
||||
# the small lane is git-only. It therefore gets the HOST's jq 1.6 no matter what the image contains.
|
||||
# That was checked, not assumed (ersatztv#648's first Done-when box).
|
||||
#
|
||||
# So the contract is the other way round: 1.6 is the FLOOR every gate must work on, and it is the
|
||||
# runner's own jq that provides the 1.6 coverage `scripts/tests/` runs under.
|
||||
#
|
||||
# TWO MODES, deliberately asymmetric:
|
||||
#
|
||||
# (no --expect) Print the version and assert it is >= MIN_VERSION. Used by jobs on the merge
|
||||
# path, including review-verdict.yml. There is NO upper bound here on purpose:
|
||||
# review-verdict.yml writes `review-verdict/h10`, a REQUIRED status check on
|
||||
# `main`, so a hard pin there would turn any jq upgrade on the runner into a
|
||||
# repo-wide merge deadlock. Observability without a deadlock risk.
|
||||
#
|
||||
# --expect X.Y Additionally assert the version is exactly X.Y, and FAIL if not. Used by the
|
||||
# `script-tests` job. This is the tripwire: `scripts/tests/` currently exercises
|
||||
# the 1.6 path only because the runner happens to ship 1.6. If the runner were
|
||||
# upgraded, that coverage would vanish SILENTLY and the whole class of bug above
|
||||
# would go untested again. Going red forces a human to decide — re-pin, or add a
|
||||
# real 1.6 matrix leg — rather than letting the coverage evaporate unnoticed.
|
||||
#
|
||||
# Usage: jq-preflight.sh [--expect <major.minor>]
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
# The lowest jq every shell gate in this repo must run correctly on. Do not raise this without
|
||||
# confirming the CI runner has actually been upgraded first — the runner, not the dev Mac, is the
|
||||
# binding constraint.
|
||||
MIN_VERSION="1.6"
|
||||
|
||||
expect=""
|
||||
while [ "$#" -gt 0 ]; do
|
||||
case "$1" in
|
||||
--expect) expect="${2:-}"; shift 2 ;;
|
||||
*) echo "jq-preflight: unknown argument '$1'" >&2; exit 2 ;;
|
||||
esac
|
||||
done
|
||||
|
||||
if ! command -v jq >/dev/null 2>&1; then
|
||||
echo "jq-preflight: jq is not on PATH. The shell gates in scripts/ and .gitea/workflows/ shell out to jq; without it they fail as a pile of opaque assertion errors instead of one clear message." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
raw=$(jq --version 2>&1 || true)
|
||||
# `jq --version` prints e.g. `jq-1.6`, `jq-1.7.1`, or on some builds `jq-1.8.2-dirty`.
|
||||
version=${raw#jq-}
|
||||
major=${version%%.*}
|
||||
rest=${version#*.}
|
||||
minor=${rest%%.*}
|
||||
# Some builds append a suffix with no further dot ("jq-1.6-dirty", "jq-1.7rc1"), which would otherwise
|
||||
# leave a non-numeric minor and fail closed on a perfectly ordinary jq. Keep the leading digits only.
|
||||
major=${major%%[!0-9]*}
|
||||
minor=${minor%%[!0-9]*}
|
||||
|
||||
# THIS LINE IS THE POINT of the no-arg mode: the jq version CI actually used is in the job log, so a
|
||||
# future divergence can be diagnosed from the log alone rather than by guessing at the runner image.
|
||||
echo "jq-preflight: jq version in use = ${raw} (parsed ${major}.${minor}; floor ${MIN_VERSION})"
|
||||
|
||||
case "$major$minor" in
|
||||
*[!0-9]*|"") echo "jq-preflight: could not parse a major.minor out of '${raw}' — failing closed" >&2; exit 1 ;;
|
||||
esac
|
||||
|
||||
min_major=${MIN_VERSION%%.*}
|
||||
min_minor=${MIN_VERSION#*.}
|
||||
if [ "$major" -lt "$min_major" ] || { [ "$major" -eq "$min_major" ] && [ "$minor" -lt "$min_minor" ]; }; then
|
||||
echo "jq-preflight: jq ${major}.${minor} is BELOW the supported floor ${MIN_VERSION}. The gates in scripts/ and .gitea/workflows/ are written against ${MIN_VERSION}+ semantics and will misbehave silently on older builds." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if [ -n "$expect" ]; then
|
||||
if [ "${major}.${minor}" != "$expect" ]; then
|
||||
echo "jq-preflight: expected jq ${expect}, found ${major}.${minor}." >&2
|
||||
echo "" >&2
|
||||
echo "This is a TRIPWIRE, not a defect in your change (ersatztv#648). scripts/tests/ was pinned to" >&2
|
||||
echo "jq ${expect} because that is what this runner shipped; it now reports ${major}.${minor}. The ${expect}" >&2
|
||||
echo "coverage the suite assumed has therefore just disappeared, silently — and jq 1.7 altered NUL" >&2
|
||||
echo "handling, exit codes, @base64d and number precision, every one of which a gate here depends on." >&2
|
||||
echo "" >&2
|
||||
echo "Decide explicitly, then update the --expect value in .gitea/workflows/pr-checks.yml:" >&2
|
||||
echo " * re-pin to the new version after re-reading docs/ci-cd.md -> 'The jq contract', or" >&2
|
||||
echo " * add a real matrix leg that runs the suite under ${MIN_VERSION} as well." >&2
|
||||
exit 1
|
||||
fi
|
||||
echo "jq-preflight: version matches the expected pin (${expect})."
|
||||
fi
|
||||
Executable
+190
@@ -0,0 +1,190 @@
|
||||
#!/usr/bin/env bash
|
||||
# Exhaustively enumerate a PR's changed file paths, or fail closed.
|
||||
#
|
||||
# ersatztv#649. This is the ONE implementation of the security-critical half of the merge gate.
|
||||
# It exists because the same logic was written twice — once in `.claude/hooks/pretooluse-merge-consent.sh`
|
||||
# (advisory: a failure produces a human prompt) and once in `.gitea/workflows/review-verdict.yml`
|
||||
# (ENFORCED: it writes the branch-protection-required `review-verdict/h10` status). The advisory copy
|
||||
# accumulated four rounds of hardening (ersatztv#643) that the enforced copy never received, leaving the
|
||||
# copy with real authority strictly weaker than the copy without. Two copies of a security predicate
|
||||
# drift; one cannot.
|
||||
#
|
||||
# SCOPE — mechanism, not policy. This script answers exactly one question: "what is the complete set of
|
||||
# paths this PR touches, at one head, or can we not tell?" It deliberately does NOT classify the PR.
|
||||
# The two callers' allow-lists differ ON PURPOSE and must stay separate:
|
||||
# * the hook's docs-only pattern also lets .claude/ .gitea/ .husky/ through, which is safe there only
|
||||
# because it falls through to a HUMAN PROMPT;
|
||||
# * the workflow's is narrower, because there a match posts a green status with nobody in the loop.
|
||||
# Sharing the enumeration fixes the drift; sharing the classification would erase an intended difference.
|
||||
#
|
||||
# CONTRACT
|
||||
# Usage: pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha>
|
||||
# stdout: newline-delimited paths, BOTH sides of every rename, no blank lines. May be empty.
|
||||
# exit 0 the enumeration is COMPLETE and bound to <expected-head-sha>. stdout is authoritative.
|
||||
# exit 1 the enumeration could NOT be completed or verified. stdout is meaningless — the caller
|
||||
# MUST fail closed (withhold any exemption). A diagnostic goes to stderr.
|
||||
# exit 2 usage error.
|
||||
# Callers must treat any non-zero exit as "no exemption". Never read stdout without checking the status.
|
||||
#
|
||||
# AUTH/TRANSPORT is caller-supplied via env, because the two callers authenticate differently:
|
||||
# ETV_GITEA_TOKEN | GITEA_TOKEN -> `Authorization: token`
|
||||
# ETV_GITEA_BASICAUTH -> curl -u user:pass
|
||||
# ETV_GITEA_URL | GITEA_BASE_URL -> API base; defaults to the homelab Gitea. A value ending in
|
||||
# /api/v1 is used as-is, otherwise /api/v1 is appended.
|
||||
#
|
||||
# jq COMPATIBILITY (ersatztv#648). This runs on the CI runner, which ships **jq 1.6**, while it is
|
||||
# authored on Macs shipping 1.8.x. It is therefore written to the 1.6-compatible subset:
|
||||
# * never rely on `jq -e`'s exit status over EMPTY input — 1.6 exits 0 where >=1.7 exits 4, which is
|
||||
# precisely the fail-open that ersatztv#647 found live in the enforced gate. Emptiness is always
|
||||
# checked explicitly in shell FIRST.
|
||||
# * never use `contains()` for substring tests — on 1.6 `contains("<NUL>")` is true for every string.
|
||||
# * never distinguish a parse error from "no output" by exit code — 1.6 returns 4 for both.
|
||||
# See docs/ci-cd.md -> "The jq contract".
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
if [ "$#" -ne 4 ]; then
|
||||
echo "usage: pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha>" >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
owner=$1
|
||||
repo=$2
|
||||
pr=$3
|
||||
expected_sha=$4
|
||||
|
||||
if [ -z "$owner" ] || [ -z "$repo" ] || [ -z "$pr" ] || [ -z "$expected_sha" ]; then
|
||||
echo "pr-changed-files: empty owner/repo/pr/sha argument" >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
base_url="${ETV_GITEA_URL:-${GITEA_BASE_URL:-http://192.168.1.95:3000}}"
|
||||
case "$base_url" in
|
||||
*/api/v1) : ;;
|
||||
*/) base_url="${base_url}api/v1" ;;
|
||||
*) base_url="${base_url}/api/v1" ;;
|
||||
esac
|
||||
|
||||
# Empty output on ANY failure, so every caller path treats a transport error the same way. The
|
||||
# emptiness is then rejected explicitly below — never inferred from a jq exit code.
|
||||
gq() {
|
||||
local path="$1"
|
||||
if [ -n "${ETV_GITEA_TOKEN:-}" ]; then
|
||||
curl -sf -H "Authorization: token $ETV_GITEA_TOKEN" "$base_url/$path" 2>/dev/null || true
|
||||
elif [ -n "${GITEA_TOKEN:-}" ]; then
|
||||
curl -sf -H "Authorization: token $GITEA_TOKEN" "$base_url/$path" 2>/dev/null || true
|
||||
elif [ -n "${ETV_GITEA_BASICAUTH:-}" ]; then
|
||||
curl -sf -u "$ETV_GITEA_BASICAUTH" "$base_url/$path" 2>/dev/null || true
|
||||
else
|
||||
printf ''
|
||||
fi
|
||||
}
|
||||
|
||||
if [ -z "${ETV_GITEA_TOKEN:-}" ] && [ -z "${GITEA_TOKEN:-}" ] && [ -z "${ETV_GITEA_BASICAUTH:-}" ]; then
|
||||
echo "pr-changed-files: no Gitea credentials in env — cannot enumerate, failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
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=$(gq "repos/$owner/$repo/pulls/$pr/files?limit=${PAGE_SIZE}&page=${page}")
|
||||
|
||||
# An EMPTY body is rejected in SHELL, before jq sees it. `jq -e` over empty input exits 4 on
|
||||
# jq >= 1.7 but 0 on jq 1.6, and the runner ships 1.6 — leaving this to jq's exit status is the
|
||||
# exact fail-open ersatztv#647 found in the enforced copy. A transport failure must never
|
||||
# masquerade as a legitimate short final page.
|
||||
if [ -z "${raw//[[:space:]]/}" ]; then
|
||||
echo "pr-changed-files: empty/unreadable response for page ${page}" >&2
|
||||
complete=no; break
|
||||
fi
|
||||
|
||||
# VALIDATE EVERY FIELD THE EXTRACTION BELOW CONSUMES, on EVERY row.
|
||||
#
|
||||
# * Top-level type alone is not enough: `[{}]` is a well-formed array whose rows carry no
|
||||
# `filename`, so it contributes no paths, looks like a short page, and would complete the
|
||||
# enumeration from a PARTIAL list — the same failure one level down. It also rejects arrays of
|
||||
# scalars, which would otherwise make the extraction fail under `set -e`.
|
||||
# * CR/LF in a path is rejected outright. `chunk` flattens paths into newline-delimited text, so a
|
||||
# filename containing a newline splits into TWO lines matched against the allow-list separately:
|
||||
# "safe.md\ndocs/Program.cs" yields `safe.md` and `docs/Program.cs`, both of which pass, while the
|
||||
# real single path ends in `.cs`. Git permits newlines in filenames, so this is reachable and was
|
||||
# reproduced against the hook.
|
||||
# * `previous_filename` is validated on EVERY row, not only `renamed` ones, because `chunk` emits it
|
||||
# for every row regardless of `.status`. Validating it only where it is semantically "supposed to"
|
||||
# appear left a hole one predicate wide: a `status: "modified"` row carrying a newline in
|
||||
# `previous_filename` was reproducibly exempted. The validation domain must match the CONSUMPTION
|
||||
# domain.
|
||||
# * `..` is rejected because the callers' allow-lists anchor `^docs/`, so `docs/../ErsatzTV/Program.cs`
|
||||
# matches one. Git will not produce such a path; this guard's job is to fail closed on unexpected
|
||||
# 2xx shapes rather than assume a well-behaved peer.
|
||||
# * `.status` is checked against a CLOSED set. Without it the `renamed => previous_filename REQUIRED`
|
||||
# clause is dodgeable by any other value — `"Renamed"` with a capital R, or an absent status —
|
||||
# letting a `git mv ErsatzTV/Program.cs -> docs/a.md` drop its source path and read as docs-only.
|
||||
# `modified` is accepted alongside `changed` deliberately: live Gitea 1.25.4 emits `changed`, but a
|
||||
# closed allow-list built from the wrong vocabulary is a worse failure than the hole it closes — it
|
||||
# would gate every genuine docs-only PR on any version that spells it differently. The property is
|
||||
# "reject values we do not recognise", not "enumerate one version exactly".
|
||||
if ! printf '%s' "$raw" \
|
||||
| jq -e 'def ok: type == "string" and length > 0
|
||||
and (test("[\\r\\n]") | not)
|
||||
and (split("/") | index("..") | not);
|
||||
type == "array" and all(.[];
|
||||
(.filename | ok)
|
||||
and (.previous_filename == null or (.previous_filename | ok))
|
||||
and ((.status // "") as $s | ($s | type) == "string"
|
||||
and (["added","deleted","changed","modified","renamed","copied"] | index($s)) != null)
|
||||
and (if .status == "renamed"
|
||||
then (.previous_filename | type == "string" and length > 0)
|
||||
else true end))' \
|
||||
>/dev/null 2>&1; then
|
||||
echo "pr-changed-files: page ${page} failed row validation" >&2
|
||||
complete=no; break
|
||||
fi
|
||||
|
||||
# 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`. 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 is therefore ONE row but TWO
|
||||
# paths, which is why the two counts below are 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")
|
||||
|
||||
# Terminate ONLY on an explicitly validated EMPTY page — never on a merely SHORT one.
|
||||
# "Fewer than 50 rows means last page" assumes the server's page size is the 50 we asked for, but
|
||||
# Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and is free to
|
||||
# return fewer. A 30-row page followed by a page of code would complete the enumeration over a
|
||||
# PARTIAL list — the same fail-open, reached without any transport error. Costs one extra request;
|
||||
# the MAX_PAGES cap still fails closed.
|
||||
if [ "$n" -eq 0 ]; then complete=yes; break; fi
|
||||
page=$((page + 1))
|
||||
done
|
||||
|
||||
if [ "$complete" != yes ]; then
|
||||
echo "pr-changed-files: enumeration incomplete (stopped at page ${page}) — failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Bind the enumeration to ONE head. Paging is several round-trips; a force-push between them means
|
||||
# page 1 came from head A and page 2 from head B, so the assembled list belongs to no single commit —
|
||||
# B's code page can be skipped entirely while B's docs page reads as a clean short tail. Re-read the
|
||||
# head and refuse if it moved.
|
||||
prjson=$(gq "repos/$owner/$repo/pulls/$pr")
|
||||
if [ -z "${prjson//[[:space:]]/}" ]; then
|
||||
echo "pr-changed-files: could not re-read PR head to bind the enumeration — failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
sha_after=$(printf '%s' "$prjson" | jq -r '.head.sha // ""' 2>/dev/null || true)
|
||||
if [ -z "$sha_after" ] || [ "$sha_after" != "$expected_sha" ]; then
|
||||
echo "pr-changed-files: head moved during enumeration (${expected_sha:0:7} -> ${sha_after:0:7}) — failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
printf '%s\n' "$files" | grep -v '^$' || true
|
||||
exit 0
|
||||
@@ -0,0 +1,144 @@
|
||||
"""Tests for `scripts/jq-preflight.sh` — the jq version contract (ersatztv#648).
|
||||
|
||||
The axis this guards. Every shell gate in this repo is authored on a Mac shipping jq 1.8.x; the CI
|
||||
runner ships jq 1.6. Nothing pinned or checked that, and three independent divergences surfaced in a
|
||||
single day — `jq -e` over empty input (exit 4 vs 0), `contains("<NUL>")` (false vs true for every
|
||||
string), and the parse-error exit code (5 vs 4, colliding with "no output"). Each was patched with a
|
||||
version-stable construct, but patching constructs one at a time leaves the AXIS untested.
|
||||
|
||||
These tests shim `jq` on PATH with a fake reporting an arbitrary version, so the preflight's own
|
||||
behaviour is verified by MEASUREMENT rather than by observing a green CI tick — ersatztv#648's third
|
||||
Done-when box. Doing it here rather than by pushing a deliberately-red commit also keeps the proof
|
||||
reproducible: it re-runs on every PR instead of living in one CI run's history.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
SCRIPT = REPO_ROOT / "scripts" / "jq-preflight.sh"
|
||||
WORKFLOWS = REPO_ROOT / ".gitea" / "workflows"
|
||||
# Resolved BEFORE PATH is narrowed to the shim dir — the tests strip PATH down to just that
|
||||
# directory, so `bash` could not be found by name from inside them.
|
||||
BASH = shutil.which("bash") or "/bin/bash"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def preflight(tmp_path):
|
||||
bindir = tmp_path / "bin"
|
||||
bindir.mkdir()
|
||||
|
||||
class Handle:
|
||||
def with_jq(self, version_line):
|
||||
"""Install a fake `jq` reporting `version_line` for --version."""
|
||||
shim = bindir / "jq"
|
||||
shim.write_text("#!/bin/sh\n"
|
||||
'if [ "$1" = "--version" ]; then echo "%s"; exit 0; fi\nexit 0\n'
|
||||
% version_line)
|
||||
shim.chmod(0o755)
|
||||
|
||||
def without_jq(self):
|
||||
shim = bindir / "jq"
|
||||
if shim.exists():
|
||||
shim.unlink()
|
||||
|
||||
def run(self, *args):
|
||||
env = dict(os.environ)
|
||||
# PATH contains ONLY the shim dir. An earlier draft appended /usr/bin:/bin "for the
|
||||
# basics" and the missing-jq test passed vacuously against the developer machine's real
|
||||
# /usr/bin/jq — the negative case was never negative. The script needs nothing from PATH
|
||||
# but jq itself (`command -v` is a builtin, and bash is invoked by absolute path), so
|
||||
# there is nothing to keep.
|
||||
env["PATH"] = str(bindir)
|
||||
return subprocess.run([BASH, str(SCRIPT), *args],
|
||||
env=env, capture_output=True, text=True)
|
||||
|
||||
return Handle()
|
||||
|
||||
|
||||
def test_the_version_is_printed_so_the_job_log_shows_it(preflight):
|
||||
"""ersatztv#648's second Done-when box: the jq version CI actually uses must be OBSERVABLE."""
|
||||
preflight.with_jq("jq-1.6")
|
||||
r = preflight.run()
|
||||
assert r.returncode == 0, r.stderr
|
||||
assert "jq-1.6" in r.stdout
|
||||
|
||||
|
||||
def test_floor_mode_accepts_the_runner_version(preflight):
|
||||
preflight.with_jq("jq-1.6")
|
||||
assert preflight.run().returncode == 0
|
||||
|
||||
|
||||
def test_floor_mode_accepts_a_newer_jq(preflight):
|
||||
"""No upper bound in floor mode — review-verdict.yml writes the REQUIRED merge check, so a jq
|
||||
bump must never be able to deadlock every merge in the repo."""
|
||||
preflight.with_jq("jq-1.8.2")
|
||||
assert preflight.run().returncode == 0
|
||||
|
||||
|
||||
def test_below_the_floor_is_LOUD(preflight):
|
||||
preflight.with_jq("jq-1.5")
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1
|
||||
assert "below the supported floor" in r.stderr.lower()
|
||||
|
||||
|
||||
def test_missing_jq_is_loud(preflight):
|
||||
preflight.without_jq()
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1
|
||||
assert "not on PATH" in r.stderr
|
||||
|
||||
|
||||
@pytest.mark.parametrize("version_line", ["jq-1.6-dirty", "jq-1.6", "jq-1.6.0"])
|
||||
def test_build_suffixes_still_parse_as_1_6(preflight, version_line):
|
||||
"""A packaging suffix must not fail a perfectly ordinary jq closed — that would be a tripwire
|
||||
firing on noise, which is how tripwires get disabled."""
|
||||
preflight.with_jq(version_line)
|
||||
assert preflight.run("--expect", "1.6").returncode == 0, version_line
|
||||
|
||||
|
||||
def test_expect_mismatch_is_LOUD(preflight):
|
||||
"""THE TRIPWIRE. scripts/tests exercises the jq 1.6 path only because the runner ships 1.6. If
|
||||
the runner were upgraded that coverage would vanish silently, so the pin must go red instead."""
|
||||
preflight.with_jq("jq-1.7.1")
|
||||
r = preflight.run("--expect", "1.6")
|
||||
assert r.returncode == 1
|
||||
assert "expected jq 1.6, found 1.7" in r.stderr
|
||||
|
||||
|
||||
def test_expect_match_passes(preflight):
|
||||
preflight.with_jq("jq-1.6")
|
||||
assert preflight.run("--expect", "1.6").returncode == 0
|
||||
|
||||
|
||||
def test_unknown_argument_is_a_usage_error(preflight):
|
||||
preflight.with_jq("jq-1.6")
|
||||
assert preflight.run("--pin", "1.6").returncode == 2
|
||||
|
||||
|
||||
# --- 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.
|
||||
|
||||
`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.
|
||||
"""
|
||||
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"
|
||||
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)")
|
||||
@@ -0,0 +1,256 @@
|
||||
"""Tests for `scripts/pr-changed-files.sh` — the SHARED PR file enumeration (ersatztv#649).
|
||||
|
||||
Why this file exists. The enumeration used to be written twice: once in
|
||||
`.claude/hooks/pretooluse-merge-consent.sh` (ADVISORY — a failure produces a human prompt) and once
|
||||
inline in `.gitea/workflows/review-verdict.yml` (ENFORCED — it writes the branch-protection-required
|
||||
`review-verdict/h10` status). They drifted, and in the dangerous direction: four rounds of
|
||||
ersatztv#643 hardening landed on the advisory copy and never reached the enforced one, so the copy
|
||||
with real authority ended up strictly weaker than the copy without.
|
||||
|
||||
The specific thing this suite pins is the point of ersatztv#649's second Done-when box. A round-4
|
||||
review traced that the enforced copy's fail-closed behaviour on a garbage response was INCIDENTAL,
|
||||
not designed: `n` came back empty, `[ "$n" -lt 50 ]` errored to false, the loop ran to MAX_PAGES and
|
||||
left complete=no. The right answer, reached through a bash arithmetic error that any refactor of the
|
||||
loop could have silently flipped. Every failure-path test below therefore asserts a NON-ZERO exit
|
||||
explicitly, so the behaviour is a contract rather than a coincidence.
|
||||
|
||||
Observable contract of the script:
|
||||
exit 0 -> enumeration complete and bound to the expected head; stdout is the authoritative path set
|
||||
exit 1 -> could not enumerate/verify; stdout meaningless, caller MUST withhold any exemption
|
||||
exit 2 -> usage error
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
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"
|
||||
WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml"
|
||||
|
||||
SHA = "a9e3e23abf337980ca4c05854f5b1e210099d08b"
|
||||
OTHER_SHA = "b71c0d4e2f8a91b3c5d7e9f1a3b5c7d9e1f3a5b7"
|
||||
|
||||
# Serves paged `pulls/N/files`, plus the PR object the script re-reads to bind the enumeration.
|
||||
CURL_SHIM = r'''#!/usr/bin/env python3
|
||||
import json, os, sys, pathlib, urllib.parse
|
||||
|
||||
state = pathlib.Path(os.environ["STUB_DIR"])
|
||||
args = sys.argv[1:]
|
||||
url = [a for a in args if a.startswith("http")][-1]
|
||||
|
||||
if "/pulls/" in url and "/files" in url:
|
||||
q = urllib.parse.parse_qs(urllib.parse.urlparse(url).query)
|
||||
page = int(q.get("page", ["1"])[0])
|
||||
pages = json.loads((state / "pages.json").read_text())
|
||||
if page > len(pages):
|
||||
print("[]"); sys.exit(0)
|
||||
entry = pages[page - 1]
|
||||
if entry == "ERROR": # transport failure on this page
|
||||
sys.exit(22)
|
||||
if entry == "GARBAGE": # 200 with a non-array body (proxy/error page)
|
||||
print('{"message":"internal error"}'); sys.exit(0)
|
||||
print(json.dumps(entry)); sys.exit(0)
|
||||
|
||||
if "/pulls/" in url:
|
||||
# A second head sha served from the Nth PR-object read onward models a force-push landing
|
||||
# between pagination round-trips.
|
||||
sha = os.environ["STUB_SHA"]
|
||||
alt = state / "pr_sha_after.txt"
|
||||
if alt.exists():
|
||||
sha = alt.read_text().strip()
|
||||
print(json.dumps({"head": {"sha": sha}}))
|
||||
sys.exit(0)
|
||||
|
||||
print("{}")
|
||||
'''
|
||||
|
||||
|
||||
def _rows(paths, status="modified"):
|
||||
return [{"filename": p, "status": status} for p in paths]
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def enumerate_files(tmp_path):
|
||||
bindir = tmp_path / "bin"; bindir.mkdir()
|
||||
shim = bindir / "curl"; shim.write_text(CURL_SHIM); shim.chmod(0o755)
|
||||
state = tmp_path / "state"; state.mkdir()
|
||||
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
|
||||
env["STUB_DIR"] = str(state)
|
||||
env["STUB_SHA"] = SHA
|
||||
env["ETV_GITEA_TOKEN"] = "stub"
|
||||
env["ETV_GITEA_URL"] = "http://gitea.example"
|
||||
env.pop("ETV_GITEA_BASICAUTH", None)
|
||||
env.pop("GITEA_TOKEN", None)
|
||||
|
||||
class Handle:
|
||||
def __init__(self, env_, state_):
|
||||
self.env = env_
|
||||
self.state = state_
|
||||
|
||||
def set_pages(self, *pages):
|
||||
(state / "pages.json").write_text(json.dumps(list(pages)))
|
||||
|
||||
def head_moves_to(self, sha):
|
||||
(state / "pr_sha_after.txt").write_text(sha)
|
||||
|
||||
def run(self, expected_sha=SHA, args=("timothy", "ersatztv", "42")):
|
||||
return subprocess.run(
|
||||
["bash", str(SCRIPT), *args, expected_sha],
|
||||
env=self.env, capture_output=True, text=True)
|
||||
|
||||
def paths(self):
|
||||
"""Assert success and return the enumerated path set."""
|
||||
r = self.run()
|
||||
assert r.returncode == 0, f"expected success, got {r.returncode}: {r.stderr}"
|
||||
return [ln for ln in r.stdout.splitlines() if ln.strip()]
|
||||
|
||||
def fails_closed(self):
|
||||
"""The whole point: a NON-ZERO exit, asserted, not inferred."""
|
||||
r = self.run()
|
||||
return r.returncode != 0
|
||||
|
||||
return Handle(env, state)
|
||||
|
||||
|
||||
# --- The happy path, so the failure-path assertions below cannot pass vacuously ----------------
|
||||
|
||||
def test_complete_enumeration_returns_every_path(enumerate_files):
|
||||
enumerate_files.set_pages(_rows(["docs/a.md", "ErsatzTV/Program.cs", "README.md"]))
|
||||
assert enumerate_files.paths() == ["docs/a.md", "ErsatzTV/Program.cs", "README.md"]
|
||||
|
||||
|
||||
def test_a_rename_contributes_BOTH_sides(enumerate_files):
|
||||
"""One row, two paths — the `git mv` hole. Reading `.filename` alone hides the source."""
|
||||
enumerate_files.set_pages([{"filename": "docs/innocuous-note.md", "status": "renamed",
|
||||
"previous_filename": ".gitea/workflows/renovate.yml"}])
|
||||
assert sorted(enumerate_files.paths()) == [".gitea/workflows/renovate.yml",
|
||||
"docs/innocuous-note.md"]
|
||||
|
||||
|
||||
def test_paths_on_a_later_page_are_included(enumerate_files):
|
||||
enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]),
|
||||
_rows(["scripts/decisions_lib.py"]))
|
||||
assert "scripts/decisions_lib.py" in enumerate_files.paths()
|
||||
|
||||
|
||||
# --- Fail-closed contract: each of these MUST be non-zero, by design ---------------------------
|
||||
|
||||
def test_transport_failure_mid_pagination_fails_closed(enumerate_files):
|
||||
"""The defect that started all of this: an errored page counted as zero rows and read as
|
||||
'end of list', completing the enumeration over a PARTIAL list."""
|
||||
enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), "ERROR",
|
||||
_rows(["docs/tail.md"]))
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_non_array_body_fails_closed(enumerate_files):
|
||||
enumerate_files.set_pages("GARBAGE")
|
||||
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_array_of_scalars_fails_closed(enumerate_files):
|
||||
enumerate_files.set_pages(["docs/a.md", "docs/b.md"])
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("evil", ["safe.md\ndocs/Program.cs", "safe.md\rdocs/Program.cs"])
|
||||
def test_CRLF_in_filename_fails_closed(enumerate_files, evil):
|
||||
"""A newline splits one path into two lines that are each allow-list-matched separately, so
|
||||
`safe.md\\ndocs/Program.cs` reads as two exempt paths while the real path ends in .cs."""
|
||||
enumerate_files.set_pages(_rows([evil]))
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_CRLF_in_previous_filename_on_a_NON_renamed_row_fails_closed(enumerate_files):
|
||||
"""The hole one predicate wide: `previous_filename` is CONSUMED on every row, so it must be
|
||||
VALIDATED on every row — not only where `.status == "renamed"` makes it semantically expected."""
|
||||
enumerate_files.set_pages([{"filename": "docs/a.md", "status": "modified",
|
||||
"previous_filename": "safe.md\ndocs/Program.cs"}])
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_dotdot_path_component_fails_closed(enumerate_files):
|
||||
"""The callers' allow-lists anchor `^docs/`, which `docs/../ErsatzTV/Program.cs` matches."""
|
||||
enumerate_files.set_pages(_rows(["docs/../ErsatzTV/Program.cs"]))
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("status", ["Renamed", "moved", ""])
|
||||
def test_status_outside_the_closed_allow_list_fails_closed(enumerate_files, status):
|
||||
"""Without a closed set, the `renamed => previous_filename REQUIRED` clause is dodgeable by any
|
||||
other value, letting a `git mv` drop its source path and read as docs-only."""
|
||||
enumerate_files.set_pages([{"filename": "docs/a.md", "status": status,
|
||||
"previous_filename": "ErsatzTV/Program.cs"}])
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_renamed_row_without_previous_filename_fails_closed(enumerate_files):
|
||||
enumerate_files.set_pages([{"filename": "docs/a.md", "status": "renamed"}])
|
||||
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."""
|
||||
enumerate_files.set_pages(_rows(["docs/a.md"]))
|
||||
enumerate_files.head_moves_to(OTHER_SHA)
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_missing_credentials_fails_closed(enumerate_files):
|
||||
enumerate_files.set_pages(_rows(["docs/a.md"]))
|
||||
enumerate_files.env.pop("ETV_GITEA_TOKEN", None)
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("args", [("timothy", "ersatztv"), ("timothy", "ersatztv", "")])
|
||||
def test_usage_errors_exit_2(enumerate_files, args):
|
||||
enumerate_files.set_pages(_rows(["docs/a.md"]))
|
||||
r = enumerate_files.run(args=args) if len(args) == 3 else subprocess.run(
|
||||
["bash", str(SCRIPT), *args], env=enumerate_files.env, capture_output=True, text=True)
|
||||
assert r.returncode == 2, r.stderr
|
||||
|
||||
|
||||
def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files):
|
||||
"""Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS and may return fewer than asked.
|
||||
'Fewer than 50 rows means last page' would complete over a partial list without any transport
|
||||
error — so termination requires a validated EMPTY page. A 30-row page followed by code must be
|
||||
seen."""
|
||||
enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]),
|
||||
_rows(["ErsatzTV/Program.cs"]))
|
||||
assert "ErsatzTV/Program.cs" in enumerate_files.paths()
|
||||
|
||||
|
||||
# --- 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.
|
||||
|
||||
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.
|
||||
"""
|
||||
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")
|
||||
Reference in New Issue
Block a user