fix(648): an explicit jq version contract + one shared PR-file enumeration #658
@@ -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
|
||||
|
||||
+54
-5
@@ -655,11 +655,60 @@ which is itself a small demonstration of why the suite needed to run in CI at al
|
||||
`small`-lane Python jobs it adds `actions/setup-python@v5` first. Checkout is at default depth: every `git` call in the suite runs
|
||||
against a temp repo it creates itself, never this repository's history.
|
||||
|
||||
A **preflight step** asserts `jq` and `git` are on PATH before running the suite. Those two tests
|
||||
exec the real shell scripts, which shell out to `jq` ~26 times; the tests shim `curl` on PATH but
|
||||
not `jq`, so a runner image without it would surface as ~20 opaque assertion failures instead of one
|
||||
diagnosis. It deliberately **checks** rather than installs — ersatztv#390 removed run-time
|
||||
`apt-get` from CI; the fix for a genuine miss is to bake the tool into the runner image.
|
||||
Two **preflight steps** run before the suite. The first asserts `git` is on PATH; the second runs
|
||||
`scripts/jq-preflight.sh --expect 1.6`, which checks jq's **version**, not merely its presence (see
|
||||
"The jq contract" below). Those two tests exec the real shell scripts, which shell out to `jq` ~26
|
||||
times; the tests shim `curl` on PATH but not `jq`, so a runner image without it would surface as ~20
|
||||
opaque assertion failures instead of one diagnosis. Both deliberately **check** rather than install —
|
||||
ersatztv#390 removed run-time `apt-get` from CI; the fix for a genuine miss is to bake the tool into
|
||||
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,
|
||||
`pretooluse-merge-consent.sh`, `review-verdict.yml`, `scripts/pr-changed-files.sh` — is authored and
|
||||
tested on a developer Mac shipping **jq 1.8.x**. The CI runner ships **jq 1.6**. Author to the
|
||||
1.6-compatible subset; three concrete constructs diverge between the two and each one produced a real
|
||||
bug when it hit CI for the first time:
|
||||
|
||||
- **`jq -e` over EMPTY input.** Exits 4 on jq >= 1.7, but **0** on jq 1.6. A guard that infers
|
||||
"transport failure" from that exit status silently passes an empty/failed page on 1.6.
|
||||
- **`` contains("\u0000") `` (or any NUL literal).** The NUL escape truncates to `""` on jq 1.6, so
|
||||
the containment test is vacuously true for **every** string, not just ones containing a NUL. Use
|
||||
`explode | index(0)` instead — it is version-stable.
|
||||
- **Parse-error exit code.** `jq empty` exits 5 on jq >= 1.7 but **4** on jq 1.6 — the same code 1.6
|
||||
uses for "no output produced". Reading that exit code as a specific failure mode conflates garbage
|
||||
input with an empty-but-valid response.
|
||||
|
||||
`scripts/jq-preflight.sh` makes the running version **observable** in every gate job's log (it prints
|
||||
the parsed version and asserts a floor of 1.6) so a future divergence can be diagnosed from the log
|
||||
alone instead of guessing at the runner image.
|
||||
|
||||
**Pin vs floor is deliberately asymmetric.** `scripts/jq-preflight.sh --expect 1.6` additionally pins
|
||||
the version and fails loudly if it drifts, but that mode is used **only** by `script-tests`
|
||||
(`.gitea/workflows/pr-checks.yml`) — advisory, not a required check. `review-verdict.yml` runs the
|
||||
no-args floor-only mode and never pins, because that workflow writes `review-verdict/h10`, the
|
||||
branch-protection-**required** status check on `main`: a hard pin there would mean the day the
|
||||
runner's jq version changes (a base-image bump, a host reimage — nothing this repo controls), every
|
||||
PR on `main` stops merging until someone notices and re-pins. A required merge gate cannot fail
|
||||
because an upstream package manager did its job. The narrower pin on `script-tests` exists precisely
|
||||
because that job is the suite's only 1.6 coverage — if the runner's jq silently changed, that coverage
|
||||
would evaporate with no signal, so failing loudly there forces a human decision instead.
|
||||
|
||||
Baking a pinned jq into `docker/ci/Dockerfile` was considered and rejected: `review-verdict.yml` is
|
||||
`runs-on: small` with no toolchain-image pin, and per `ci.small-lane-git-only` the small lane is
|
||||
git-only, so it gets the **host's** jq regardless of what the toolchain image contains — a pin in the
|
||||
image provably cannot reach the gate that broke. This was checked against the running binary, not
|
||||
assumed.
|
||||
|
||||
## PR gates workflow
|
||||
|
||||
|
||||
@@ -45,6 +45,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) |
|
||||
| `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=<name>` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) |
|
||||
| `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) |
|
||||
| `ci.jq-version-contract` | Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job's log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock. | 2026-07-26 | [link](records/ci/jq-version-contract.md) |
|
||||
| `ci.killed-job-triage` | Never trust a job's `conclusion` field alone — read the log tail and require an `❌ Failure - Main …` marker before treating a red as a real failure. | 2026-07-21 | [link](records/ci/killed-job-triage.md) |
|
||||
| `ci.monitor-armed-at-pr-open` | Arm a CI monitor on the PR head sha the moment the PR opens, polling the commit-status endpoint — not at the end of the work. | 2026-07-21 | [link](records/ci/monitor-armed-at-pr-open.md) |
|
||||
| `ci.no-host-health-gating` | Push when your work is validated — never SSH to bumblebee to sample load/RAM first, and never hand-schedule around other sessions' runs. | 2026-07-21 | [link](records/ci/no-host-health-gating.md) |
|
||||
@@ -52,6 +53,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) |
|
||||
| `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) |
|
||||
| `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) |
|
||||
| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) |
|
||||
| `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) |
|
||||
| `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) |
|
||||
| `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) |
|
||||
|
||||
@@ -0,0 +1,103 @@
|
||||
---
|
||||
key: ci.jq-version-contract
|
||||
title: '2026-07-26 — jq 1.6 is the FLOOR every shell gate must run on; `scripts/jq-preflight.sh` makes the version observable, and only `script-tests` pins it (#648)'
|
||||
status: active
|
||||
since: '2026-07-26'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job''s log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock.'
|
||||
signals: 'jq version divergence, jq 1.6 vs 1.8, jq -e exit code on empty input, contains NUL false positive, jq parse-error exit code collision, jq-preflight, script-tests --expect, review-verdict jq floor, merge deadlock from a pinned dependency · paths: `scripts/jq-preflight.sh`, `.gitea/workflows/review-verdict.yml`, `.gitea/workflows/pr-checks.yml`, `docs/ci-cd.md` · issues: #643, #647, #648, #649'
|
||||
mechanics: '`scripts/jq-preflight.sh` (no args) -> floor+observability in every gate job; `scripts/jq-preflight.sh --expect 1.6` -> tripwire, `script-tests` job only; `docs/ci-cd.md` -> "The jq contract"'
|
||||
---
|
||||
|
||||
Three independent jq-version divergences hit inside a single day (#643, #647), all in gates written
|
||||
and tested on a developer Mac (jq 1.8.x) but running on the CI runner (jq 1.6):
|
||||
|
||||
- `jq -e` over EMPTY input exits 4 on jq >= 1.7, but **0** on jq 1.6 — the docs-only pagination guard
|
||||
inferred "transport failure" from that exit status, so on 1.6 a failed page silently passed and the
|
||||
loop walked past unread pages while still reporting `files_complete=yes`.
|
||||
- `contains("\u0000")` — the NUL escape truncates to `""` on jq 1.6, so the containment test is
|
||||
vacuously **true for every string**, not just ones actually containing a NUL. The H10
|
||||
review-verdict classifier that relied on this was entirely inert on the runner.
|
||||
- Parse-error exit code: `jq empty` exits 5 on jq >= 1.7 but **4** on jq 1.6 — the same code jq 1.6
|
||||
uses for "no output produced". A garbage API response and an empty-but-valid one were
|
||||
indistinguishable, and the garbage case was read as "no comments."
|
||||
|
||||
None of these are exotic jq usage — they are constructs anyone would reach for first, and each one
|
||||
was discovered only because a real gate broke, not because anyone thought to test jq 1.6. That is the
|
||||
argument for a *contract*, not three point fixes: the failures share one shape (a shell gate's
|
||||
behavior is a function of an interpreter version nobody was treating as a variable), so patching each
|
||||
construct as it's found does not converge — it just narrows the next surprise.
|
||||
|
||||
**Why 1.6 is the floor and not 1.8.** The runner is the binding constraint, not the author's machine.
|
||||
Baking a pinned jq into `docker/ci/Dockerfile` was the obvious first idea and was rejected because it
|
||||
provably cannot cover the gate that actually broke: `review-verdict.yml` is `runs-on: small` with no
|
||||
toolchain-image pin, and per `ci.small-lane-git-only` the small lane is git-only — it gets the host's
|
||||
jq 1.6 no matter what the toolchain image contains. That was checked against the running binary, not
|
||||
assumed. So the fix has to hold at 1.6, in every gate, regardless of which lane it runs in.
|
||||
|
||||
**Why the pin is asymmetric.** `scripts/jq-preflight.sh` has two modes on purpose:
|
||||
|
||||
- No arguments — print the parsed version and fail only below the 1.6 floor. This is pure
|
||||
observability: the jq version CI actually used is now in the job log, so a future divergence can be
|
||||
diagnosed from the log alone instead of by guessing at the runner image. `review-verdict.yml` runs
|
||||
this mode. It cannot run the pinned mode, because that job's output is the required
|
||||
`review-verdict/h10` status check on `main` — a hard version pin there means the day the runner's jq
|
||||
is upgraded (a base-image bump, a host reimage, anything outside this repo's control), every PR
|
||||
on `main` stops merging until someone notices and re-pins. A required merge gate cannot have a
|
||||
failure mode that is "an upstream package manager did its job."
|
||||
- `--expect 1.6` — pin and fail loudly. Used only by `script-tests`. `scripts/tests/` currently
|
||||
exercises the 1.6 code path only because the runner happens to ship 1.6; if that silently changed,
|
||||
the 1.6 coverage this whole contract depends on would evaporate with no signal. The tripwire forces
|
||||
a human decision — re-pin after re-reading this record, or add a real 1.6 matrix leg — instead of
|
||||
letting the coverage quietly disappear.
|
||||
|
||||
**Be honest about the cost: firing this tripwire DOES block merges.** An earlier draft of this
|
||||
record claimed the pin was safe because `script-tests` is "advisory, not one of the required
|
||||
checks". That reasoning is wrong, and the correction is worth recording because it is easy to make
|
||||
twice. `.claude/hooks/pretooluse-merge-consent.sh` reads the **combined** commit status and denies
|
||||
on anything that is not `success`/`skipped` — see `ci.advisory-red-blocks-the-merge-gate` (#598).
|
||||
`script-tests` is a Gitea Actions job, so its red is a context folded into that combined state.
|
||||
A jq bump therefore reddens `script-tests` and blocks non-docs-only merges until someone re-pins.
|
||||
|
||||
One qualification, so this does not over-correct in the other direction: that combined-status read
|
||||
is guarded by `if [ "$mwcs" != "true" ]`. On the `merge_when_checks_succeed` path the hook does not
|
||||
read the combined status at all and defers to Gitea, which gates on *required* checks only — and
|
||||
`script-tests` is not one. So the blast radius is the hook-mediated merge path, not literally every
|
||||
merge.
|
||||
|
||||
The pin is kept anyway, deliberately: the fix is a one-line edit to the `--expect` value in
|
||||
`pr-checks.yml`, the failure message spells that out, and the alternative — silently losing the
|
||||
only coverage of the version axis that produced three bugs in one day — is worse than a visible
|
||||
stop. What is NOT acceptable is believing it is free. The difference from `review-verdict.yml` is
|
||||
therefore one of *degree and recoverability*, not of "blocks merges vs doesn't": there the check is
|
||||
required per-sha and a jq bump would deadlock merges with no in-repo remedy at all, whereas here a
|
||||
human can unblock the repo in one commit.
|
||||
|
||||
**The parse is strictly fail-closed, and that has an operational edge once it gates merges.**
|
||||
`scripts/jq-preflight.sh` accepts only a FIRST line of the form `jq-<X>.<Y>` or `jq version <X>.<Y>`;
|
||||
anything else — a leading blank line, a wrapper that prints a warning first, a version reported only
|
||||
on stderr — exits 1 rather than guess. That is the right default for a guard whose whole purpose is
|
||||
refusing to certify a version it did not parse, and it was arrived at over four revisions in which
|
||||
every *permissive* variant turned out to be fail-OPEN.
|
||||
|
||||
But when the follow-up wires the floor-only mode into `review-verdict.yml`, that strictness sits in
|
||||
the branch-protection-**required** check. A jq wrapper that starts printing a banner line would then
|
||||
deadlock merges repo-wide — the very failure the pin/floor asymmetry exists to avoid, arriving
|
||||
through the parser instead of the pin. If that ever happens the fix is to widen the accepted forms in
|
||||
`jq-preflight.sh`, **not** to relax the fail-closed behaviour: an unparsed version must never be
|
||||
treated as satisfying the floor.
|
||||
|
||||
**The three constructs to avoid, and their version-stable replacements:**
|
||||
|
||||
- Never infer "empty input" from a jq exit status — check the string in shell before invoking jq.
|
||||
- Never use `contains("\u0000")` (or any raw NUL literal) for a control-character test — use
|
||||
`explode | index(0)`, which does not depend on jq's NUL-escape handling.
|
||||
- Never infer "parse error" from `jq`'s exit code on ambiguous input — `jq empty` is the portable
|
||||
parse-only test, but its exit code collides between "no output" (1.6) and "parse error" (1.6, same
|
||||
code as 1.8's "no output"). Validate the response shape explicitly rather than reading one exit
|
||||
code as a specific failure mode.
|
||||
|
||||
Full narrative of how these were found (inside the #631 `script-tests` rollout) is in
|
||||
`docs/decisions/records/ci/script-tests-job.md`; this record is the durable contract that came out of
|
||||
it, rather than the incident log.
|
||||
@@ -59,17 +59,12 @@ both reds were real:
|
||||
|
||||
The second one is the argument for this record in miniature. It sat in the gate that decides whether
|
||||
a PR skips the Done-when checks, it was covered by an existing test, and that test could not catch it
|
||||
on a developer Mac (jq 1.8) — only in CI, where the suite had never run. **Standing rules it leaves behind, all three the same shape — never infer a
|
||||
CONDITION from a jq exit status or a version-dependent builtin:**
|
||||
- never infer "empty input" from a jq exit status; check the string (`#643`);
|
||||
- never infer "parse error" from a jq exit status — `jq empty` is the portable test, because
|
||||
jq >= 1.7 exits 5 where 1.6 exits 4, and 4 is also "no output" (`#647`);
|
||||
- never use `contains("\u0000")` — on jq 1.6 the escape truncates to `""` and it matches every
|
||||
string (`#647`). `explode | index(0)` is version-stable.
|
||||
|
||||
The runner ships **jq 1.6**; a developer Mac ships 1.8.x. Three divergences were found in one day,
|
||||
so the durable fix is to pin or preflight the version rather than keep patching constructs —
|
||||
tracked on `#647`.
|
||||
on a developer Mac (jq 1.8) — only in CI, where the suite had never run. Two further divergences of
|
||||
the same shape (a `contains("\u0000")` false positive and a colliding parse-error exit code) turned
|
||||
up the same day; the durable contract that came out of all three — the exact constructs to avoid, and
|
||||
why `jq-preflight.sh` pins in `script-tests` but only floors the version in `review-verdict.yml` — is
|
||||
recorded once, in `ci.jq-version-contract` (`docs/decisions/records/ci/jq-version-contract.md`), and
|
||||
is not restated here.
|
||||
|
||||
An independent cross-family review of that fix then found **two further fail-opens in the same
|
||||
enumeration, both reachable with no transport error at all** (#643):
|
||||
|
||||
@@ -0,0 +1,94 @@
|
||||
---
|
||||
key: ci.shared-pr-file-enumeration
|
||||
title: '2026-07-26 — `scripts/pr-changed-files.sh` is the ONE enumeration of a PR''s changed files; the advisory hook and the enforced gate share mechanism, never policy (#649)'
|
||||
status: active
|
||||
since: '2026-07-26'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.'
|
||||
signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hook, docs-only allow-list drift, shared mechanism not shared policy, pr-changed-files.sh, checkout base ref not PR head, gate judging its own PR, exhaustiveness bug in a security predicate · paths: `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml` · issues: #643, #648, #649'
|
||||
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
|
||||
missed guard there just means a person gets asked instead of an automatic decision. The enforced
|
||||
workflow's failure mode is a `success` write to `review-verdict/h10`, the one status branch
|
||||
protection actually requires — a missed guard there merges an unreviewed PR with nobody asked at all.
|
||||
|
||||
**The drift that motivated this.** Four rounds of #643 hardening landed entirely on the copy with the
|
||||
*lower* stakes. The hook accumulated CR/LF rejection, `..` rejection, a closed `.status` allow-list,
|
||||
`previous_filename` validation on every row (not just `renamed`), termination only on a validated
|
||||
empty page, and head-sha binding — while the enforced workflow kept the original, weaker logic. Its
|
||||
fail-closed behavior on a garbage API response was incidental (an empty `n` erroring a bash
|
||||
conditional to false), not a designed property. The gate with real authority was strictly weaker than
|
||||
the gate with none, which is the wrong way around by construction, not by anyone's mistake in a
|
||||
single review — nothing in the original layout forced the two to move together.
|
||||
|
||||
**Why the fix is "one script, two callers" rather than "copy the hardening across."** Copying keeps
|
||||
the two-implementation shape; the next hardening round would only need to happen twice again, and
|
||||
there is no mechanism that would surface a second drift before it mattered. Extracting
|
||||
`scripts/pr-changed-files.sh` makes the enumeration a single artifact with a single test suite
|
||||
(`scripts/tests/test_pr_changed_files.py`), so a future guard is added once and both callers get it
|
||||
atomically.
|
||||
|
||||
**Mechanism, not policy — the two allow-lists stay separate on purpose.** The extracted script
|
||||
answers exactly one question: "what is the complete set of paths this PR touches, at one head, or can
|
||||
we not tell?" It does not decide whether that set makes the PR docs-only. Each caller keeps its own
|
||||
classification:
|
||||
|
||||
- The hook's docs-only pattern also lets `.claude/`, `.gitea/`, `.husky/` through, which is safe there
|
||||
*only* because a non-match falls through to a human prompt rather than an auto-grant.
|
||||
- The workflow's is narrower, because there a match posts a green status with nobody in the loop, and
|
||||
both docs-only and Renovate exemptions are void when the PR touches `.claude/`, `.gitea/`,
|
||||
`.husky/`, `scripts/` or `docker/ci/` — the gate must not be able to exempt itself from review by
|
||||
editing itself.
|
||||
|
||||
Merging the two allow-lists would have quietly widened the enforced exemption to match the advisory
|
||||
one, turning a difference that exists for a reason into an accident of refactoring. Sharing the
|
||||
enumeration closes the drift that actually caused harm without touching the part that was correctly
|
||||
different.
|
||||
|
||||
**What the shared script owns.** Six guards, all now exercised by one test suite instead of a subset
|
||||
in each caller:
|
||||
|
||||
- CR/LF rejection and `..` rejection on every path.
|
||||
- A closed `.status` allow-list — `added`/`deleted`/`changed`/`modified`/`renamed`/`copied`, not an
|
||||
open denylist. Note `changed` and `deleted` are the values live Gitea 1.25.4 actually emits;
|
||||
`modified` is accepted alongside `changed` because a closed list built from the wrong vocabulary
|
||||
would gate every genuine docs-only PR. GitHub's `removed` is deliberately **not** in the list — an
|
||||
earlier draft of this record said it was, which would have sent a maintainer looking for a value
|
||||
the code rejects.
|
||||
- `previous_filename` validated on **every** row the extraction consumes, not only rows whose
|
||||
`.status` is `renamed` — a `modified`/`copied` row can still carry it, and an earlier fix that
|
||||
validated only the `renamed` case was found incomplete for exactly this reason (see
|
||||
`ci.script-tests-job` for the review trail).
|
||||
- Termination only on a validated **empty** page — Gitea's paging can return fewer rows than
|
||||
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.
|
||||
|
||||
**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
|
||||
head, so the workflow that judges a PR cannot be rewritten by that same PR to weaken its own
|
||||
judgment.
|
||||
|
||||
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.
|
||||
|
||||
**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
|
||||
bug on the *enforced* copy would not have been merely a downgrade.
|
||||
Executable
+173
@@ -0,0 +1,173 @@
|
||||
#!/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)
|
||||
# `shift 2` with a missing value fails under `set -e` and exits 1 with NOTHING on either
|
||||
# stream — a CI step dying with an empty log is exactly the diagnostic hole this script exists
|
||||
# to remove. Check explicitly instead.
|
||||
if [ "$#" -lt 2 ] || [ -z "${2:-}" ]; then
|
||||
echo "jq-preflight: --expect requires a <major.minor> value" >&2
|
||||
exit 2
|
||||
fi
|
||||
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
|
||||
|
||||
# Take jq's EXIT STATUS seriously, and keep stderr OUT of the parse input.
|
||||
#
|
||||
# This was `raw=$(jq --version 2>&1 || true)`, which did neither — and that combination turned the
|
||||
# guard fail-OPEN on the case it most needs to catch. A jq that cannot start (the canonical one is a
|
||||
# glibc mismatch after a base-image change) exits 127 and writes something like
|
||||
# `jq: /lib/x86_64-linux-gnu/libc.so.6: version 'GLIBC_2.34' not found` to stderr. Folded into `raw`,
|
||||
# that string contains `2.34`, which the version pattern happily matched — so the preflight printed
|
||||
# "parsed 2.34", certified the floor, and exited 0 on a jq that cannot run at all. The strip-based
|
||||
# parse this replaced failed CLOSED there, so it was a regression introduced by the fix.
|
||||
# `$?` inside an `if ! cmd; then` block is the NEGATED status (0), not jq's, so capture it explicitly.
|
||||
set +e
|
||||
raw=$(jq --version 2>/dev/null)
|
||||
jq_rc=$?
|
||||
set -e
|
||||
if [ "$jq_rc" -ne 0 ]; then
|
||||
echo "jq-preflight: 'jq --version' failed (exit ${jq_rc}). jq is on PATH but cannot run — a broken build or a missing shared library. Failing closed rather than certifying a version it did not report." >&2
|
||||
exit 1
|
||||
fi
|
||||
# `jq --version` prints e.g. `jq-1.6`, `jq-1.7.1`, or on some builds `jq-1.8.2-dirty`.
|
||||
# Parse with an explicit regex rather than by stripping around the first `-` and `.`.
|
||||
#
|
||||
# The strip approach had a hole that defeated the whole point of this script. It assumed the format
|
||||
# is exactly `jq-X.Y`, so a build printing anything else — `jq version 1.6` (a distro wrapper),
|
||||
# `JQ-1.6`, `jq-1.-6` — left ONE of major/minor empty. The old sanity check was
|
||||
# `case "$major$minor" in *[!a-9]*|"")`, and on `jq version 1.6` that concatenation is "6": non-empty
|
||||
# and all-digits, so the guard PASSED. The floor comparison then ran `[ "" -lt 1 ]`, which exits 2
|
||||
# with "integer expression expected" — and `set -e` exempts a failing command in an `if` condition,
|
||||
# so the whole conditional read false and the script exited 0 having asserted NOTHING, after printing
|
||||
# a plausible-looking "parsed" line.
|
||||
#
|
||||
# That is the silently-untested-axis failure this script was written to eliminate, reproduced inside
|
||||
# the script itself. Require a real `<digits>.<digits>` match, and fail closed when there isn't one.
|
||||
# ANCHORED to the leading `jq` token, not "first digits.digits anywhere in the string".
|
||||
#
|
||||
# An unanchored match takes whatever number comes first, wherever it is. That accepted a leading
|
||||
# warning line or a date prefix as the version — `2026.07.26 jq-1.6` parsed as 2026.07, which sails
|
||||
# over the floor. Anchoring keeps every legitimate form (`jq-1.6`, `jq version 1.6`, `jq-1.7.1`,
|
||||
# `jq-1.6-dirty`, `jq-1.6 (Debian 1.6-2.1)`) and rejects the rest, which then fails closed below.
|
||||
# FIRST LINE ONLY, and bounded everywhere. Both bounds are load-bearing; this is the third round on
|
||||
# this one predicate and each previous version failed for a variant of the same reason.
|
||||
#
|
||||
# * First line only. `[[:space:]]` matches NEWLINES, so an "anchored" pattern still scanned the
|
||||
# whole output: `jq\n2.34: cannot load` matched `jq`, crossed the newline as separator, and
|
||||
# parsed 2.34 — fail-open, the round-2 bug narrowed but not closed. `[[:blank:]]` (space/tab
|
||||
# only) plus a first-line slice confines the match to the line that can actually carry a version.
|
||||
# * Bounded digit runs. This is the round-1 mechanism resurrected. The regex guaranteed the
|
||||
# operands were digits but not that they fit in `test`'s integer range, so a 23-digit major made
|
||||
# `[ "$major" -lt "$min_major" ]` error with "integer expression expected" — and `set -e` exempts
|
||||
# a failing command in an `if` condition, so the conditional read false and THE FLOOR WAS NEVER
|
||||
# ASSERTED, exit 0. Exactly what the empty-string case did in round 1. `{1,9}` keeps every
|
||||
# operand inside a 32-bit integer, so the comparison can no longer error.
|
||||
# * Bounded separator runs, so the pattern cannot be walked across arbitrary filler.
|
||||
first=${raw%%$'\n'*}
|
||||
first=${first%$'\r'}
|
||||
# The separator is one of the two forms real jq actually emits — `jq-1.6` or `jq version 1.6` — not
|
||||
# "any run of dashes and blanks". A permissive class let the pattern be walked across filler:
|
||||
# `jq -- 2.34 (real jq-1.6)` parsed as 2.34, and `jq<TAB><TAB>9.9` as 9.9. A blank separator now
|
||||
# REQUIRES the literal word `version`, which is the only context a real build puts one in.
|
||||
#
|
||||
# The trailing `([^0-9]|$)` is what actually bounds the digit runs. `{1,9}` alone does not: the regex
|
||||
# is unanchored at the end, so `jq-1.99999999999999999999999` simply matched the first 9 digits of
|
||||
# the minor and compared THAT — a mis-parse that passes the floor. Requiring a non-digit (or
|
||||
# end-of-string) after the minor makes an over-long run fail to match at all, so it fails closed.
|
||||
if [[ "$first" =~ ^[[:blank:]]*[Jj][Qq](-v?|[[:blank:]]+version[[:blank:]]+v?)([0-9]{1,9})\.([0-9]{1,9})([^0-9]|$) ]]; then
|
||||
major="${BASH_REMATCH[2]}"
|
||||
minor="${BASH_REMATCH[3]}"
|
||||
else
|
||||
echo "jq-preflight: could not parse a major.minor version out of '${first}'. Refusing to assert a floor against an unparsed version — that would silently pass." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# 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.
|
||||
# `$first`, not `$raw`: a multi-line `--version` would split this across lines, breaking the single
|
||||
# grep-able log line that is the entire point of the no-arg mode.
|
||||
echo "jq-preflight: jq version in use = ${first} (parsed ${major}.${minor}; floor ${MIN_VERSION})"
|
||||
|
||||
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
+196
@@ -0,0 +1,196 @@
|
||||
#!/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. Be precise about what this does and does not do:
|
||||
# the extraction below emits `(.previous_filename // empty)` UNCONDITIONALLY, so a present
|
||||
# `previous_filename` is never dropped on account of `.status`. What the closed set actually buys
|
||||
# is rejecting rows whose vocabulary we do not recognise — where a source path may be absent, or
|
||||
# carried in some other field we are not reading. Without it, `"Renamed"` with a capital R, or an
|
||||
# absent status, silently takes the `else true` branch of the clause below and skips the
|
||||
# "renamed rows MUST carry previous_filename" requirement entirely. (An earlier version of this
|
||||
# comment claimed the source path would be "dropped", which is not the mechanism; a maintainer
|
||||
# who tested that claim would find it false and might conclude the check is redundant.)
|
||||
# `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,349 @@
|
||||
"""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"
|
||||
|
||||
|
||||
def _shq(s):
|
||||
"""Single-quote a string for /bin/sh."""
|
||||
return "'" + s.replace("'", "'\\''") + "'"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def preflight(tmp_path):
|
||||
bindir = tmp_path / "bin"
|
||||
bindir.mkdir()
|
||||
|
||||
class Handle:
|
||||
def with_jq(self, version_line, stderr="", exit_code=0):
|
||||
"""Install a fake `jq` reporting `version_line` for --version.
|
||||
|
||||
`stderr` and `exit_code` exist because an earlier version of this shim ALWAYS exited 0
|
||||
and never wrote to stderr — so it structurally could not observe the worst failure this
|
||||
script has: a jq that cannot start. The preflight was folding stderr into the parse via
|
||||
`2>&1` and discarding the exit status, so a glibc-mismatch message containing `2.34`
|
||||
parsed as version 2.34 and PASSED the floor. Every case the shim could express was clean,
|
||||
so every test passed.
|
||||
"""
|
||||
shim = bindir / "jq"
|
||||
body = "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then\n"
|
||||
if version_line:
|
||||
body += ' printf "%%s\\n" %s\n' % _shq(version_line)
|
||||
if stderr:
|
||||
body += ' printf "%%s\\n" %s >&2\n' % _shq(stderr)
|
||||
body += " exit %d\nfi\nexit 0\n" % exit_code
|
||||
shim.write_text(body)
|
||||
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)
|
||||
|
||||
def run_bytes(self, *args):
|
||||
"""Same, but WITHOUT text mode.
|
||||
|
||||
`text=True` enables universal-newlines translation, which rewrites `\\r` to `\\n` in the
|
||||
captured output — so any assertion about a stray carriage return is unfalsifiable through
|
||||
`run()`. That is not hypothetical: the CR test passed identically with the strip removed
|
||||
until this was noticed, while the mutant demonstrably emits
|
||||
`... = jq-1.6\\r (parsed 1.6; ...)` at the byte level.
|
||||
"""
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = str(bindir)
|
||||
return subprocess.run([BASH, str(SCRIPT), *args],
|
||||
env=env, capture_output=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
|
||||
|
||||
|
||||
def test_expect_without_a_value_is_a_usage_error_WITH_output(preflight):
|
||||
"""`shift 2` on a missing value exits 1 under `set -e` with NOTHING on either stream. A CI step
|
||||
that dies with an empty log is the diagnostic hole this script exists to remove."""
|
||||
preflight.with_jq("jq-1.6")
|
||||
r = preflight.run("--expect")
|
||||
assert r.returncode == 2
|
||||
assert "requires a <major.minor> value" in r.stderr
|
||||
|
||||
|
||||
# --- Version parsing: the guard must never assert a floor against an unparsed version ----------
|
||||
#
|
||||
# The original strip-based parse assumed the format is exactly `jq-X.Y`. Anything else left major or
|
||||
# minor EMPTY, and the sanity check concatenated them — so `jq version 1.6` produced "6", which is
|
||||
# non-empty and all-digits, so the check PASSED. The floor comparison then ran `[ "" -lt 1 ]`, which
|
||||
# errors; `set -e` exempts a failing command in an `if` condition, so the conditional read false and
|
||||
# the script exited 0 having asserted NOTHING. That is this script's own stated failure mode,
|
||||
# reproduced inside itself, which is why these cases are pinned rather than left to inspection.
|
||||
|
||||
@pytest.mark.parametrize("version_line", [
|
||||
"jq version 1.6", # some distro wrappers print this form
|
||||
"JQ-1.6",
|
||||
"jq-1.6-dirty",
|
||||
])
|
||||
def test_unusual_but_parseable_version_forms_are_accepted(preflight, version_line):
|
||||
preflight.with_jq(version_line)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 0, f"{version_line!r}: {r.stderr}"
|
||||
assert "parsed 1.6" in r.stdout
|
||||
|
||||
|
||||
@pytest.mark.parametrize("version_line", ["jq-1.-6", "jq-.6", "not-a-version", ""])
|
||||
def test_unparseable_version_fails_CLOSED_rather_than_asserting_nothing(preflight, version_line):
|
||||
preflight.with_jq(version_line)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1, (
|
||||
f"{version_line!r} exited {r.returncode}: an unparsed version must never reach — or "
|
||||
"silently skip — the floor assertion")
|
||||
assert "could not parse" in r.stderr
|
||||
|
||||
|
||||
def test_a_jq_that_cannot_START_fails_closed(preflight):
|
||||
"""THE case the previous shim could not express, and the guard therefore got wrong.
|
||||
|
||||
A jq broken by a glibc mismatch (the canonical post-base-image-bump failure) exits 127 and writes
|
||||
`... version 'GLIBC_2.34' not found` to STDERR. The preflight was reading `jq --version 2>&1` and
|
||||
discarding the exit status, so that message became the parse input, `2.34` matched, and the floor
|
||||
was certified green on a jq that cannot run at all.
|
||||
"""
|
||||
preflight.with_jq(
|
||||
"", stderr="jq: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found",
|
||||
exit_code=127)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1
|
||||
assert "cannot run" in r.stderr
|
||||
assert "parsed 2.34" not in r.stdout, "stderr must never be parsed as a version"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("version_line", [
|
||||
"warning: something 3.14", # a noise line carrying a plausible number
|
||||
"2026.07.26 jq-1.6", # a date prefix, which outranks the real version if unanchored
|
||||
"jq-master-v0.0.0-1.6",
|
||||
])
|
||||
def test_a_number_that_is_not_the_VERSION_is_not_accepted_as_one(preflight, version_line):
|
||||
"""Matching the first `<digits>.<digits>` ANYWHERE let a prefix win over the real version.
|
||||
`2026.07.26 jq-1.6` parsed as 2026.07 and sailed over the floor. The pattern is anchored to the
|
||||
leading `jq` token, so these fail closed instead."""
|
||||
preflight.with_jq(version_line)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1, f"{version_line!r} was accepted as a version"
|
||||
assert "could not parse" in r.stderr
|
||||
|
||||
|
||||
@pytest.mark.parametrize("version_line", [
|
||||
"jq-99999999999999999999999.0",
|
||||
"jq-1.99999999999999999999999",
|
||||
])
|
||||
def test_an_OUT_OF_RANGE_digit_run_fails_closed(preflight, version_line):
|
||||
"""The round-1 fail-open mechanism, resurrected via an over-long number.
|
||||
|
||||
A regex that guarantees *digits* does not guarantee they fit `test`'s integer range. With a
|
||||
23-digit major, `[ "$major" -lt "$min_major" ]` errors with "integer expression expected" — and
|
||||
`set -e` exempts a failing command in an `if` condition, so the conditional read false and THE
|
||||
FLOOR WAS NEVER ASSERTED, exit 0. Identical in shape to the empty-string case that started this.
|
||||
|
||||
Bounding the run with `{1,9}` alone was NOT enough either: the pattern is unanchored at the end,
|
||||
so an over-long minor just matched its first 9 digits and compared that instead — a mis-parse
|
||||
that passes. The trailing non-digit requirement is what actually closes it.
|
||||
"""
|
||||
preflight.with_jq(version_line)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1, f"{version_line!r} exited 0 — the floor was not asserted"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("version_line", [
|
||||
# Killed by the SEPARATOR restriction (a blank separator must be followed by `version`).
|
||||
"jq\n2.34: cannot load shared library",
|
||||
"jq\n\n\n99.9",
|
||||
"jq -- 2.34 (real jq-1.6)",
|
||||
"jq\t\t9.9",
|
||||
# Killed ONLY by the first-line slice + `[[:blank:]]`. These carry the literal word `version`,
|
||||
# so the separator restriction is satisfied and cannot save us — the newline must be excluded
|
||||
# from the separator class AND the parse confined to line one.
|
||||
#
|
||||
# Without these, a round-5 mutation check found that reverting BOTH of those changes together
|
||||
# (`[[:blank:]]`→`[[:space:]]` and parsing `$raw` instead of `$first`) left the whole suite
|
||||
# GREEN: the four cases above are all killed by the separator alone, so they attributed the fix
|
||||
# to the wrong layer. A test that passes for the wrong reason is how the previous three rounds
|
||||
# each shipped a defect.
|
||||
"jq\nversion\n9.9",
|
||||
"jq\nversion 9.9",
|
||||
"jq \n version \n 9.9",
|
||||
])
|
||||
def test_a_number_AFTER_the_jq_token_is_not_reachable_across_filler(preflight, version_line):
|
||||
"""Two independent layers keep a stray number from being read as the version, and both are
|
||||
pinned here: the separator must be one of the forms real jq emits (`jq-1.6` / `jq version 1.6`),
|
||||
AND the match is confined to the first line with `[[:blank:]]` (which, unlike `[[:space:]]`,
|
||||
does not match a newline). Round 3's 'anchor' had neither and parsed `jq\\n2.34: cannot load`
|
||||
as 2.34."""
|
||||
preflight.with_jq(version_line)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 1, f"{version_line!r} was accepted as a version"
|
||||
|
||||
|
||||
def test_a_CRLF_version_line_parses_and_logs_without_the_carriage_return(preflight):
|
||||
"""The trailing `\\r` strip was unpinned — the commit claimed CRLF was verified, but nothing in
|
||||
the suite contained one. Harmless today (a `\\r` satisfies the trailing non-digit boundary, so
|
||||
the version still parses) but the log line would carry a stray CR."""
|
||||
preflight.with_jq("jq-1.6\r")
|
||||
r = preflight.run_bytes()
|
||||
assert r.returncode == 0, r.stderr
|
||||
assert b"parsed 1.6" in r.stdout
|
||||
# Two separate traps had to be cleared for this assertion to mean anything:
|
||||
# 1. `str.splitlines()` also splits on `\r`, so inspecting the "version in use" line would drop
|
||||
# the stray CR before the assertion could see it;
|
||||
# 2. `subprocess.run(text=True)` translates `\r` to `\n` outright, so even raw-string checks on
|
||||
# `r.stdout` were unfalsifiable.
|
||||
# Both made the test pass identically with the strip removed. Hence `run_bytes()` and a bytes
|
||||
# comparison — verified by mutation, not by reading the code.
|
||||
assert b"\r" not in r.stdout, "the carriage return leaked into the log line"
|
||||
|
||||
|
||||
def test_the_observability_line_stays_on_ONE_line(preflight):
|
||||
"""The no-arg mode exists to put a single grep-able version line in the job log; interpolating a
|
||||
multi-line `--version` would split it."""
|
||||
preflight.with_jq("jq-1.6\ntrailing noise")
|
||||
r = preflight.run()
|
||||
assert r.returncode == 0, r.stderr
|
||||
version_lines = [ln for ln in r.stdout.splitlines() if "version in use" in ln]
|
||||
assert len(version_lines) == 1
|
||||
assert "trailing noise" not in r.stdout
|
||||
|
||||
|
||||
@pytest.mark.parametrize("version_line,expected", [
|
||||
("jq-1.6 (Debian 1.6-2.1)", "1.6"), # distro packaging suffix
|
||||
("jq-1.10", "1.10"), # two-digit minor: must compare numerically, not lexically
|
||||
("jq-1.7.1", "1.7"),
|
||||
("jq-1.6.0", "1.6"),
|
||||
("jq-v1.6", "1.6"),
|
||||
("JQ-1.6", "1.6"),
|
||||
])
|
||||
def test_legitimate_forms_still_parse_to_the_right_version(preflight, version_line, expected):
|
||||
preflight.with_jq(version_line)
|
||||
r = preflight.run()
|
||||
assert r.returncode == 0, f"{version_line!r}: {r.stderr}"
|
||||
assert f"parsed {expected}" in r.stdout
|
||||
|
||||
|
||||
# --- Wiring guards: the preflight is worthless if a caller silently stops running it ------------
|
||||
|
||||
def test_script_tests_pins_the_jq_version():
|
||||
"""The pin is the tripwire, so its presence is asserted rather than merely commented.
|
||||
|
||||
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()
|
||||
assert "jq-preflight.sh --expect" in pr_checks, \
|
||||
"script-tests must pin the jq version — that pin is the tripwire"
|
||||
|
||||
|
||||
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)")
|
||||
@@ -0,0 +1,376 @@
|
||||
"""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"
|
||||
# (the enforced caller, .gitea/workflows/review-verdict.yml, is wired in the follow-up PR)
|
||||
|
||||
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()
|
||||
|
||||
|
||||
# --- 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
|
||||
# review mutated the hook's `if files=$(...)` into `files=$(...) || true; files_complete=yes` — i.e.
|
||||
# ignore the exit status entirely — and the ENTIRE suite still passed.
|
||||
#
|
||||
# It survives today only by REDUNDANCY: the script writes stdout once, immediately before `exit 0`,
|
||||
# so every failure path also happens to yield empty stdout, and the hook's independent
|
||||
# `[ -n "$files" ]` check catches it. That is exactly the shape this PR criticises elsewhere — safe
|
||||
# by accident rather than by assertion. Any future change that streams pages, or prints a partial
|
||||
# list before failing, turns it into a live false exemption.
|
||||
#
|
||||
# So the stub below FAILS *while emitting a perfectly docs-only list*, which is the one combination
|
||||
# the redundancy cannot absorb. The positive control immediately after it proves the harness can
|
||||
# actually observe the difference, rather than reporting "not exempt" for some unrelated reason.
|
||||
|
||||
HOOK_STUB_CURL = r'''#!/usr/bin/env python3
|
||||
import json, sys
|
||||
url = [a for a in sys.argv[1:] if a.startswith("http")][-1]
|
||||
if "/pulls/" in url and "/files" not in url:
|
||||
print(json.dumps({"head": {"sha": "%s"}, "body": "no linked issue"}))
|
||||
else:
|
||||
print("{}")
|
||||
''' % SHA
|
||||
|
||||
|
||||
def _mirror_tree(tmp_path, stub_body):
|
||||
"""Lay out a minimal repo mirror so the hook resolves OUR stub as the shared script.
|
||||
|
||||
The hook finds the script via `${BASH_SOURCE[0]}/../..`, so the mirror must reproduce the real
|
||||
`.claude/hooks/` + `scripts/` shape rather than just dropping the stub anywhere.
|
||||
"""
|
||||
hooks = tmp_path / ".claude" / "hooks"
|
||||
hooks.mkdir(parents=True)
|
||||
(hooks / "pretooluse-merge-consent.sh").write_text(HOOK.read_text())
|
||||
scripts = tmp_path / "scripts"
|
||||
scripts.mkdir()
|
||||
stub = scripts / "pr-changed-files.sh"
|
||||
stub.write_text(stub_body)
|
||||
stub.chmod(0o755)
|
||||
|
||||
bindir = tmp_path / "bin"; bindir.mkdir()
|
||||
curl = bindir / "curl"; curl.write_text(HOOK_STUB_CURL); curl.chmod(0o755)
|
||||
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
|
||||
env["ETV_GITEA_TOKEN"] = "stub"
|
||||
env["ETV_GITEA_URL"] = "http://gitea.example"
|
||||
env.pop("ETV_GITEA_BASICAUTH", None)
|
||||
|
||||
payload = {"tool_input": {"method": "merge", "owner": "timothy",
|
||||
"repo": "ersatztv", "pull_number": 42}}
|
||||
r = subprocess.run(["bash", str(hooks / "pretooluse-merge-consent.sh")],
|
||||
input=json.dumps(payload), env=env, capture_output=True, text=True)
|
||||
assert r.returncode == 0, r.stderr
|
||||
# Exempt == passthrough == exit 0 with no decision JSON on stdout.
|
||||
return r.stdout.strip() == ""
|
||||
|
||||
|
||||
DOCS_ONLY_OUTPUT = 'printf "docs/a.md\\ndocs/b.md\\n"\n'
|
||||
|
||||
|
||||
def test_a_FAILING_script_withholds_the_exemption_even_when_stdout_looks_docs_only(tmp_path):
|
||||
exempt = _mirror_tree(tmp_path, "#!/usr/bin/env bash\n" + DOCS_ONLY_OUTPUT + "exit 1\n")
|
||||
assert exempt is False, (
|
||||
"the hook granted a docs-only exemption from the stdout of a script that FAILED — the exit "
|
||||
"status is not being checked")
|
||||
|
||||
|
||||
def test_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."""
|
||||
exempt = _mirror_tree(tmp_path, "#!/usr/bin/env bash\n" + DOCS_ONLY_OUTPUT + "exit 0\n")
|
||||
assert exempt is True, (
|
||||
"the positive control failed, so the negative test above cannot be trusted to be measuring "
|
||||
"the exit status at all")
|
||||
|
||||
|
||||
def test_a_script_that_crashes_also_withholds_the_exemption(tmp_path):
|
||||
"""Not every failure is a clean `exit 1` — a crash must not read as success either."""
|
||||
exempt = _mirror_tree(
|
||||
tmp_path, "#!/usr/bin/env bash\n" + DOCS_ONLY_OUTPUT + "kill -TERM $$\n")
|
||||
assert exempt is False
|
||||
|
||||
|
||||
def test_a_MISSING_script_withholds_the_exemption(tmp_path):
|
||||
"""The transition window: the shared checkout has no such script until this lands."""
|
||||
hooks = tmp_path / ".claude" / "hooks"
|
||||
hooks.mkdir(parents=True)
|
||||
(hooks / "pretooluse-merge-consent.sh").write_text(HOOK.read_text())
|
||||
(tmp_path / "scripts").mkdir()
|
||||
bindir = tmp_path / "bin"; bindir.mkdir()
|
||||
curl = bindir / "curl"; curl.write_text(HOOK_STUB_CURL); curl.chmod(0o755)
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
|
||||
env["ETV_GITEA_TOKEN"] = "stub"
|
||||
env["ETV_GITEA_URL"] = "http://gitea.example"
|
||||
payload = {"tool_input": {"method": "merge", "owner": "timothy",
|
||||
"repo": "ersatztv", "pull_number": 42}}
|
||||
r = subprocess.run(["bash", str(hooks / "pretooluse-merge-consent.sh")],
|
||||
input=json.dumps(payload), env=env, capture_output=True, text=True)
|
||||
assert r.returncode == 0, r.stderr
|
||||
assert r.stdout.strip() != "", "a missing shared script must not grant an exemption"
|
||||
|
||||
|
||||
# --- Drift guard: the reason this file is worth having at all ----------------------------------
|
||||
|
||||
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.
|
||||
"""
|
||||
text = HOOK.read_text()
|
||||
assert "scripts/pr-changed-files.sh" in text, (
|
||||
f"{HOOK.relative_to(REPO_ROOT)} no longer calls the shared enumeration")
|
||||
# Search CODE only. The hook's comments legitimately discuss this endpoint, and a future comment
|
||||
# writing `pulls/$pr/files?limit=100` as an example of what NOT to do would redden this test —
|
||||
# which, since `script-tests` red blocks merges via the combined status, would block the repo over
|
||||
# a piece of prose.
|
||||
code = "\n".join(ln for ln in text.splitlines() if not ln.lstrip().startswith("#"))
|
||||
# An inline `pulls/<n>/files?` fetch is the signature of a re-inlined copy. Match on the endpoint
|
||||
# alone, NOT on `?limit=` — an earlier version anchored the query string, so a copy written as
|
||||
# `files?page=1&limit=50` would have walked straight past a guard that exists to stop exactly
|
||||
# that. Still evadable by a copy that builds the URL without a literal `?`, so this narrows the
|
||||
# gap rather than closing it.
|
||||
assert not re.search(r"pulls/\$?\{?\w+\}?/files\?", code), (
|
||||
f"{HOOK.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — "
|
||||
"that is the duplication ersatztv#649 removed")
|
||||
Reference in New Issue
Block a user