diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 6487efeae..2b41c37e0 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -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 diff --git a/.gitea/workflows/pr-checks.yml b/.gitea/workflows/pr-checks.yml index 0fe73e668..315cdc6c2 100644 --- a/.gitea/workflows/pr-checks.yml +++ b/.gitea/workflows/pr-checks.yml @@ -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 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 644107310..2b27b03fb 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -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 diff --git a/docs/decisions/README.md b/docs/decisions/README.md index c4461fe24..901b99aaf 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -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=` 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) | diff --git a/docs/decisions/records/ci/jq-version-contract.md b/docs/decisions/records/ci/jq-version-contract.md new file mode 100644 index 000000000..707ea925a --- /dev/null +++ b/docs/decisions/records/ci/jq-version-contract.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-.` or `jq version .`; +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. diff --git a/docs/decisions/records/ci/script-tests-job.md b/docs/decisions/records/ci/script-tests-job.md index 476816b71..8b5636fce 100644 --- a/docs/decisions/records/ci/script-tests-job.md +++ b/docs/decisions/records/ci/script-tests-job.md @@ -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): diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md new file mode 100644 index 000000000..a0b3c420c --- /dev/null +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -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 ` -> 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. diff --git a/scripts/jq-preflight.sh b/scripts/jq-preflight.sh new file mode 100755 index 000000000..5b3751828 --- /dev/null +++ b/scripts/jq-preflight.sh @@ -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("") -> 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 ] + +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 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 `.` 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 `jq9.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 diff --git a/scripts/pr-changed-files.sh b/scripts/pr-changed-files.sh new file mode 100755 index 000000000..e6be5001b --- /dev/null +++ b/scripts/pr-changed-files.sh @@ -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 +# stdout: newline-delimited paths, BOTH sides of every rename, no blank lines. May be empty. +# exit 0 the enumeration is COMPLETE and bound to . 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("")` 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 " >&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 diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py new file mode 100644 index 000000000..ccc1cfe32 --- /dev/null +++ b/scripts/tests/test_jq_preflight.py @@ -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("")` (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 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 `.` 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)") diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py new file mode 100644 index 000000000..a47d5b351 --- /dev/null +++ b/scripts/tests/test_pr_changed_files.py @@ -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//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")