From 2fd798cccf992cc43b80a74e19cb8a899734437d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:37:56 +0200 Subject: [PATCH 1/7] fix(648,649): one shared PR-file enumeration + an explicit jq version contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #649 — the enforced review-verdict.yml guard had drifted strictly WEAKER than the advisory merge-consent hook: four rounds of #643 hardening landed on the copy whose failures produce a human prompt, and never reached the copy that writes the branch-protection-required review-verdict/h10 status. Its fail-closed behaviour on a garbage response was also incidental (an empty `n` erroring a bash conditional to false), not designed. Extract scripts/pr-changed-files.sh as the single implementation both call. Shared MECHANISM, not policy: the two docs-only allow-lists differ deliberately and stay separate. review-verdict.yml now checks out the BASE ref, never the PR head, so a PR cannot rewrite the gate that judges it. #648 — baking jq into docker/ci/Dockerfile provably cannot cover the gate that broke: review-verdict.yml is runs-on:small with no toolchain pin, so it gets the host's jq 1.6 (checked, not assumed). Add scripts/jq-preflight.sh: floor+observable everywhere, and a --expect tripwire on script-tests only — pinning the required merge check would deadlock every merge on a jq bump. Verified by mutation: six guards individually broken, each turning exactly its own test red, then restored byte-identical. fixes #648 fixes #649 --- .claude/hooks/pretooluse-merge-consent.sh | 126 ++--------- .gitea/workflows/pr-checks.yml | 24 +- .gitea/workflows/review-verdict.yml | 89 ++++---- scripts/jq-preflight.sh | 107 +++++++++ scripts/pr-changed-files.sh | 190 ++++++++++++++++ scripts/tests/test_jq_preflight.py | 144 ++++++++++++ scripts/tests/test_pr_changed_files.py | 256 ++++++++++++++++++++++ 7 files changed, 784 insertions(+), 152 deletions(-) create mode 100755 scripts/jq-preflight.sh create mode 100755 scripts/pr-changed-files.sh create mode 100644 scripts/tests/test_jq_preflight.py create mode 100644 scripts/tests/test_pr_changed_files.py 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/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 82f5b7194..73ab968aa 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -52,6 +52,23 @@ jobs: name: Set review-verdict status runs-on: small # a few API calls; keep it off the build runners steps: + # Check out the BASE ref, NEVER the PR head. This job now runs scripts/pr-changed-files.sh from + # the checkout, and checking out the head would let a PR rewrite the very gate that judges it — + # the PROTECTED list below could not save us, because the tampered script would already have + # produced the file list that list is applied to. `base.sha` is a commit already on the target + # branch, so it is trusted by construction. (ersatztv#649) + - name: Checkout the base ref (trusted; never the PR head) + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false + # Floor-only: print the jq version so a future divergence is diagnosable from the log alone, and + # refuse anything below the supported floor. Deliberately NOT pinned with `--expect` — this + # workflow writes the branch-protection-required `review-verdict/h10` status, so a hard version + # pin here would turn any jq bump on the runner into a repo-wide merge deadlock. The pin lives on + # `script-tests` in pr-checks.yml instead. See docs/ci-cd.md -> "The jq contract". (ersatztv#648) + - name: Preflight jq version + run: ./scripts/jq-preflight.sh - name: Classify the PR and post the review-verdict status env: GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }} @@ -116,50 +133,36 @@ jobs: exit 0 fi - # --- Changed files: PAGE to exhaustion, and fail CLOSED if we cannot. ---------------- - # Gitea caps this endpoint at 50 rows per page and SILENTLY IGNORES a larger `limit` - # (verified: PR #619 has 194 changed files and `?limit=100` returns exactly 50). A - # single-page read is therefore a silent false negative: a protected path sitting at - # position 51+ would simply not be seen, and a bot-authored PR that edits the gate could - # exempt itself from the gate. Page until a short page proves the end. - PAGE_SIZE=50 - MAX_PAGES=40 # 2000 files; beyond this we refuse rather than guess - files="" - page=1 - complete=no - while [ "$page" -le "$MAX_PAGES" ]; do - raw=$(gh "$BASE_URL/repos/$REPO/pulls/$PR/files?limit=${PAGE_SIZE}&page=${page}") || raw="" - # A transport/parse failure must NOT masquerade as a legitimate short final page. - # Empty output counts as zero rows, which would otherwise read as "end of list" and set - # complete=yes over a PARTIAL enumeration — failing OPEN at the exact point this guard - # exists to fail closed. - # - # Validating only the TOP-LEVEL type is not enough: a page like `[{}]` is a well-formed - # array whose rows carry no `filename`, so it contributes no paths, counts as a short - # page, and completes the enumeration from a partial list — the same failure one level - # down. Require every row to carry a non-empty string `filename`; an empty array stays - # valid, since that is what a genuine end-of-pagination looks like. - if ! printf '%s' "$raw" \ - | jq -e 'type == "array" and all(.[]; (.filename | type == "string" and length > 0) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))' \ - >/dev/null 2>&1; then - complete=no; break - fi - # Page-size termination is measured in ROWS; the path set collects BOTH sides of a - # rename. Gitea reports a `git mv` as ONE row whose `filename` is the DESTINATION, with - # the source only in `previous_filename` — so reading `filename` alone lets a PR move a - # protected file INTO docs/ and pass as docs-only (verified live: - # `.gitea/workflows/renovate.yml` -> `docs/innocuous-note.md` showed no protected path). - # One renamed row thus contributes ONE to `n` and TWO to the path set, which is why - # these two counts are deliberately computed differently. - n=$(printf '%s' "$raw" | jq -r 'length') - chunk=$(printf '%s' "$raw" | jq -r '.[] | (.filename // empty), (.previous_filename // empty)') - [ -n "$chunk" ] && files=$(printf '%s\n%s' "$files" "$chunk") - if [ "$n" -lt "$PAGE_SIZE" ]; then complete=yes; break; fi - page=$((page + 1)) - done - files=$(printf '%s\n' "$files" | grep -v '^$' || true) + # --- Changed files: enumerate exhaustively, or fail CLOSED. -------------------------- + # This was an inline copy of logic that also lives in + # .claude/hooks/pretooluse-merge-consent.sh. The two drifted, and badly: four rounds of + # ersatztv#643 hardening landed on the HOOK — the advisory copy, whose failures produce a + # human prompt — and never reached this one, the ENFORCED copy that writes the + # branch-protection-required status. The advisory copy ended up strictly stricter than the + # authoritative one. Worse, a round-4 review traced that this copy's safe behaviour on a + # garbage response was INCIDENTAL: `n` came back empty, `[ "$n" -lt 50 ]` errored to false, + # the loop ran to MAX_PAGES and left complete=no. Correct outcome, arrived at by a bash + # arithmetic error that any refactor of the loop could have flipped. + # + # Both callers now share ONE implementation whose fail-closed behaviour is intentional and + # asserted by scripts/tests/test_pr_changed_files.py (ersatztv#649). It carries the six + # guards this copy was missing: CR/LF rejection, `..` rejection, a closed `.status` + # allow-list, `previous_filename` validated on EVERY row rather than only `renamed` ones, + # termination only on a validated EMPTY page (never a merely short one), and binding the + # whole enumeration to a single head sha. + # + # A non-zero exit means "could not tell" and MUST withhold the exemption. Note the script + # is invoked WITHOUT `|| true`: `set -e` is not in play for an `if` condition, and reading + # its stdout without checking its status is the exact mistake this replaces. + if files=$(GITEA_TOKEN="$GITEA_TOKEN" GITEA_BASE_URL="$BASE_URL" \ + ./scripts/pr-changed-files.sh "${REPO%%/*}" "${REPO##*/}" "$PR" "$SHA"); then + complete=yes + else + complete=no + files="" + fi count=$(printf '%s\n' "$files" | grep -c . || true) - echo "Changed files (${count}, complete=${complete}, pages=${page}):" + echo "Changed files (${count}, complete=${complete}):" printf '%s\n' "$files" | sed 's/^/ /' exempt=no diff --git a/scripts/jq-preflight.sh b/scripts/jq-preflight.sh new file mode 100755 index 000000000..0f995a017 --- /dev/null +++ b/scripts/jq-preflight.sh @@ -0,0 +1,107 @@ +#!/usr/bin/env bash +# Make the jq version a job's shell gates run under OBSERVABLE, and any drift LOUD. +# +# ersatztv#648. Every shell gate in this repo is authored and tested on a developer Mac shipping +# jq 1.8.x. The CI runner ships jq 1.6. Nothing pinned or checked that, and until ersatztv#631 the one +# thing that could have noticed (scripts/tests/) never ran on the runner. Three independent divergences +# surfaced in a single day: +# +# ersatztv#643 `jq -e` over EMPTY input -> exit 4 on 1.8, exit 0 on 1.6 (a transport failure +# passed the docs-only pagination guard) +# ersatztv#647 contains("") -> 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) expect="${2:-}"; shift 2 ;; + *) echo "jq-preflight: unknown argument '$1'" >&2; exit 2 ;; + esac +done + +if ! command -v jq >/dev/null 2>&1; then + echo "jq-preflight: jq is not on PATH. The shell gates in scripts/ and .gitea/workflows/ shell out to jq; without it they fail as a pile of opaque assertion errors instead of one clear message." >&2 + exit 1 +fi + +raw=$(jq --version 2>&1 || true) +# `jq --version` prints e.g. `jq-1.6`, `jq-1.7.1`, or on some builds `jq-1.8.2-dirty`. +version=${raw#jq-} +major=${version%%.*} +rest=${version#*.} +minor=${rest%%.*} +# Some builds append a suffix with no further dot ("jq-1.6-dirty", "jq-1.7rc1"), which would otherwise +# leave a non-numeric minor and fail closed on a perfectly ordinary jq. Keep the leading digits only. +major=${major%%[!0-9]*} +minor=${minor%%[!0-9]*} + +# THIS LINE IS THE POINT of the no-arg mode: the jq version CI actually used is in the job log, so a +# future divergence can be diagnosed from the log alone rather than by guessing at the runner image. +echo "jq-preflight: jq version in use = ${raw} (parsed ${major}.${minor}; floor ${MIN_VERSION})" + +case "$major$minor" in + *[!0-9]*|"") echo "jq-preflight: could not parse a major.minor out of '${raw}' — failing closed" >&2; exit 1 ;; +esac + +min_major=${MIN_VERSION%%.*} +min_minor=${MIN_VERSION#*.} +if [ "$major" -lt "$min_major" ] || { [ "$major" -eq "$min_major" ] && [ "$minor" -lt "$min_minor" ]; }; then + echo "jq-preflight: jq ${major}.${minor} is BELOW the supported floor ${MIN_VERSION}. The gates in scripts/ and .gitea/workflows/ are written against ${MIN_VERSION}+ semantics and will misbehave silently on older builds." >&2 + exit 1 +fi + +if [ -n "$expect" ]; then + if [ "${major}.${minor}" != "$expect" ]; then + echo "jq-preflight: expected jq ${expect}, found ${major}.${minor}." >&2 + echo "" >&2 + echo "This is a TRIPWIRE, not a defect in your change (ersatztv#648). scripts/tests/ was pinned to" >&2 + echo "jq ${expect} because that is what this runner shipped; it now reports ${major}.${minor}. The ${expect}" >&2 + echo "coverage the suite assumed has therefore just disappeared, silently — and jq 1.7 altered NUL" >&2 + echo "handling, exit codes, @base64d and number precision, every one of which a gate here depends on." >&2 + echo "" >&2 + echo "Decide explicitly, then update the --expect value in .gitea/workflows/pr-checks.yml:" >&2 + echo " * re-pin to the new version after re-reading docs/ci-cd.md -> 'The jq contract', or" >&2 + echo " * add a real matrix leg that runs the suite under ${MIN_VERSION} as well." >&2 + exit 1 + fi + echo "jq-preflight: version matches the expected pin (${expect})." +fi diff --git a/scripts/pr-changed-files.sh b/scripts/pr-changed-files.sh new file mode 100755 index 000000000..2e393b7cf --- /dev/null +++ b/scripts/pr-changed-files.sh @@ -0,0 +1,190 @@ +#!/usr/bin/env bash +# Exhaustively enumerate a PR's changed file paths, or fail closed. +# +# ersatztv#649. This is the ONE implementation of the security-critical half of the merge gate. +# It exists because the same logic was written twice — once in `.claude/hooks/pretooluse-merge-consent.sh` +# (advisory: a failure produces a human prompt) and once in `.gitea/workflows/review-verdict.yml` +# (ENFORCED: it writes the branch-protection-required `review-verdict/h10` status). The advisory copy +# accumulated four rounds of hardening (ersatztv#643) that the enforced copy never received, leaving the +# copy with real authority strictly weaker than the copy without. Two copies of a security predicate +# drift; one cannot. +# +# SCOPE — mechanism, not policy. This script answers exactly one question: "what is the complete set of +# paths this PR touches, at one head, or can we not tell?" It deliberately does NOT classify the PR. +# The two callers' allow-lists differ ON PURPOSE and must stay separate: +# * the hook's docs-only pattern also lets .claude/ .gitea/ .husky/ through, which is safe there only +# because it falls through to a HUMAN PROMPT; +# * the workflow's is narrower, because there a match posts a green status with nobody in the loop. +# Sharing the enumeration fixes the drift; sharing the classification would erase an intended difference. +# +# CONTRACT +# Usage: pr-changed-files.sh +# 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. Without it the `renamed => previous_filename REQUIRED` + # clause is dodgeable by any other value — `"Renamed"` with a capital R, or an absent status — + # letting a `git mv ErsatzTV/Program.cs -> docs/a.md` drop its source path and read as docs-only. + # `modified` is accepted alongside `changed` deliberately: live Gitea 1.25.4 emits `changed`, but a + # closed allow-list built from the wrong vocabulary is a worse failure than the hole it closes — it + # would gate every genuine docs-only PR on any version that spells it differently. The property is + # "reject values we do not recognise", not "enumerate one version exactly". + if ! printf '%s' "$raw" \ + | jq -e 'def ok: type == "string" and length > 0 + and (test("[\\r\\n]") | not) + and (split("/") | index("..") | not); + type == "array" and all(.[]; + (.filename | ok) + and (.previous_filename == null or (.previous_filename | ok)) + and ((.status // "") as $s | ($s | type) == "string" + and (["added","deleted","changed","modified","renamed","copied"] | index($s)) != null) + and (if .status == "renamed" + then (.previous_filename | type == "string" and length > 0) + else true end))' \ + >/dev/null 2>&1; then + echo "pr-changed-files: page ${page} failed row validation" >&2 + complete=no; break + fi + + # BOTH sides of a rename: Gitea reports a `git mv` as ONE row whose `filename` is the DESTINATION, + # with the source only in `previous_filename`. Reading `filename` alone lets a PR move a protected + # file INTO docs/ and pass as docs-only (verified live: `.gitea/workflows/renovate.yml` -> + # `docs/innocuous-note.md` showed no protected path). One renamed row is therefore ONE row but TWO + # paths, which is why the two counts below are computed differently. + n=$(printf '%s' "$raw" | jq -r 'length') + chunk=$(printf '%s' "$raw" | jq -r '.[] | (.filename // empty), (.previous_filename // empty)') + [ -n "$chunk" ] && files=$(printf '%s\n%s' "$files" "$chunk") + + # Terminate ONLY on an explicitly validated EMPTY page — never on a merely SHORT one. + # "Fewer than 50 rows means last page" assumes the server's page size is the 50 we asked for, but + # Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and is free to + # return fewer. A 30-row page followed by a page of code would complete the enumeration over a + # PARTIAL list — the same fail-open, reached without any transport error. Costs one extra request; + # the MAX_PAGES cap still fails closed. + if [ "$n" -eq 0 ]; then complete=yes; break; fi + page=$((page + 1)) +done + +if [ "$complete" != yes ]; then + echo "pr-changed-files: enumeration incomplete (stopped at page ${page}) — failing closed" >&2 + exit 1 +fi + +# Bind the enumeration to ONE head. Paging is several round-trips; a force-push between them means +# page 1 came from head A and page 2 from head B, so the assembled list belongs to no single commit — +# B's code page can be skipped entirely while B's docs page reads as a clean short tail. Re-read the +# head and refuse if it moved. +prjson=$(gq "repos/$owner/$repo/pulls/$pr") +if [ -z "${prjson//[[:space:]]/}" ]; then + echo "pr-changed-files: could not re-read PR head to bind the enumeration — failing closed" >&2 + exit 1 +fi +sha_after=$(printf '%s' "$prjson" | jq -r '.head.sha // ""' 2>/dev/null || true) +if [ -z "$sha_after" ] || [ "$sha_after" != "$expected_sha" ]; then + echo "pr-changed-files: head moved during enumeration (${expected_sha:0:7} -> ${sha_after:0:7}) — failing closed" >&2 + exit 1 +fi + +printf '%s\n' "$files" | grep -v '^$' || true +exit 0 diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py new file mode 100644 index 000000000..5f9b83f53 --- /dev/null +++ b/scripts/tests/test_jq_preflight.py @@ -0,0 +1,144 @@ +"""Tests for `scripts/jq-preflight.sh` — the jq version contract (ersatztv#648). + +The axis this guards. Every shell gate in this repo is authored on a Mac shipping jq 1.8.x; the CI +runner ships jq 1.6. Nothing pinned or checked that, and three independent divergences surfaced in a +single day — `jq -e` over empty input (exit 4 vs 0), `contains("")` (false vs true for every +string), and the parse-error exit code (5 vs 4, colliding with "no output"). Each was patched with a +version-stable construct, but patching constructs one at a time leaves the AXIS untested. + +These tests shim `jq` on PATH with a fake reporting an arbitrary version, so the preflight's own +behaviour is verified by MEASUREMENT rather than by observing a green CI tick — ersatztv#648's third +Done-when box. Doing it here rather than by pushing a deliberately-red commit also keeps the proof +reproducible: it re-runs on every PR instead of living in one CI run's history. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "scripts" / "jq-preflight.sh" +WORKFLOWS = REPO_ROOT / ".gitea" / "workflows" +# Resolved BEFORE PATH is narrowed to the shim dir — the tests strip PATH down to just that +# directory, so `bash` could not be found by name from inside them. +BASH = shutil.which("bash") or "/bin/bash" + + +@pytest.fixture +def preflight(tmp_path): + bindir = tmp_path / "bin" + bindir.mkdir() + + class Handle: + def with_jq(self, version_line): + """Install a fake `jq` reporting `version_line` for --version.""" + shim = bindir / "jq" + shim.write_text("#!/bin/sh\n" + 'if [ "$1" = "--version" ]; then echo "%s"; exit 0; fi\nexit 0\n' + % version_line) + shim.chmod(0o755) + + def without_jq(self): + shim = bindir / "jq" + if shim.exists(): + shim.unlink() + + def run(self, *args): + env = dict(os.environ) + # PATH contains ONLY the shim dir. An earlier draft appended /usr/bin:/bin "for the + # basics" and the missing-jq test passed vacuously against the developer machine's real + # /usr/bin/jq — the negative case was never negative. The script needs nothing from PATH + # but jq itself (`command -v` is a builtin, and bash is invoked by absolute path), so + # there is nothing to keep. + env["PATH"] = str(bindir) + return subprocess.run([BASH, str(SCRIPT), *args], + env=env, capture_output=True, text=True) + + return Handle() + + +def test_the_version_is_printed_so_the_job_log_shows_it(preflight): + """ersatztv#648's second Done-when box: the jq version CI actually uses must be OBSERVABLE.""" + preflight.with_jq("jq-1.6") + r = preflight.run() + assert r.returncode == 0, r.stderr + assert "jq-1.6" in r.stdout + + +def test_floor_mode_accepts_the_runner_version(preflight): + preflight.with_jq("jq-1.6") + assert preflight.run().returncode == 0 + + +def test_floor_mode_accepts_a_newer_jq(preflight): + """No upper bound in floor mode — review-verdict.yml writes the REQUIRED merge check, so a jq + bump must never be able to deadlock every merge in the repo.""" + preflight.with_jq("jq-1.8.2") + assert preflight.run().returncode == 0 + + +def test_below_the_floor_is_LOUD(preflight): + preflight.with_jq("jq-1.5") + r = preflight.run() + assert r.returncode == 1 + assert "below the supported floor" in r.stderr.lower() + + +def test_missing_jq_is_loud(preflight): + preflight.without_jq() + r = preflight.run() + assert r.returncode == 1 + assert "not on PATH" in r.stderr + + +@pytest.mark.parametrize("version_line", ["jq-1.6-dirty", "jq-1.6", "jq-1.6.0"]) +def test_build_suffixes_still_parse_as_1_6(preflight, version_line): + """A packaging suffix must not fail a perfectly ordinary jq closed — that would be a tripwire + firing on noise, which is how tripwires get disabled.""" + preflight.with_jq(version_line) + assert preflight.run("--expect", "1.6").returncode == 0, version_line + + +def test_expect_mismatch_is_LOUD(preflight): + """THE TRIPWIRE. scripts/tests exercises the jq 1.6 path only because the runner ships 1.6. If + the runner were upgraded that coverage would vanish silently, so the pin must go red instead.""" + preflight.with_jq("jq-1.7.1") + r = preflight.run("--expect", "1.6") + assert r.returncode == 1 + assert "expected jq 1.6, found 1.7" in r.stderr + + +def test_expect_match_passes(preflight): + preflight.with_jq("jq-1.6") + assert preflight.run("--expect", "1.6").returncode == 0 + + +def test_unknown_argument_is_a_usage_error(preflight): + preflight.with_jq("jq-1.6") + assert preflight.run("--pin", "1.6").returncode == 2 + + +# --- Wiring guards: the preflight is worthless if a caller silently stops running it ------------ + +def test_script_tests_pins_and_review_verdict_only_floors(): + """The asymmetry is deliberate and load-bearing, so it is asserted rather than merely commented. + + `script-tests` carries `--expect` (a tripwire on a normal job). `review-verdict.yml` must NOT: + it writes the branch-protection-required `review-verdict/h10` status, so a hard version pin + there would turn a jq bump on the runner into a repo-wide merge deadlock. + """ + pr_checks = (WORKFLOWS / "pr-checks.yml").read_text() + review_verdict = (WORKFLOWS / "review-verdict.yml").read_text() + + assert "jq-preflight.sh --expect" in pr_checks, \ + "script-tests must pin the jq version — that pin is the tripwire" + assert "jq-preflight.sh" in review_verdict, \ + "review-verdict.yml must at least print/floor-check its jq version" + assert "jq-preflight.sh --expect" not in review_verdict, \ + ("review-verdict.yml must NOT pin a jq version: it writes the required review-verdict/h10 " + "status, so a pin would deadlock every merge on a jq bump (ersatztv#648)") diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py new file mode 100644 index 000000000..bd0e27c4e --- /dev/null +++ b/scripts/tests/test_pr_changed_files.py @@ -0,0 +1,256 @@ +"""Tests for `scripts/pr-changed-files.sh` — the SHARED PR file enumeration (ersatztv#649). + +Why this file exists. The enumeration used to be written twice: once in +`.claude/hooks/pretooluse-merge-consent.sh` (ADVISORY — a failure produces a human prompt) and once +inline in `.gitea/workflows/review-verdict.yml` (ENFORCED — it writes the branch-protection-required +`review-verdict/h10` status). They drifted, and in the dangerous direction: four rounds of +ersatztv#643 hardening landed on the advisory copy and never reached the enforced one, so the copy +with real authority ended up strictly weaker than the copy without. + +The specific thing this suite pins is the point of ersatztv#649's second Done-when box. A round-4 +review traced that the enforced copy's fail-closed behaviour on a garbage response was INCIDENTAL, +not designed: `n` came back empty, `[ "$n" -lt 50 ]` errored to false, the loop ran to MAX_PAGES and +left complete=no. The right answer, reached through a bash arithmetic error that any refactor of the +loop could have silently flipped. Every failure-path test below therefore asserts a NON-ZERO exit +explicitly, so the behaviour is a contract rather than a coincidence. + +Observable contract of the script: + exit 0 -> enumeration complete and bound to the expected head; stdout is the authoritative path set + exit 1 -> could not enumerate/verify; stdout meaningless, caller MUST withhold any exemption + exit 2 -> usage error +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "scripts" / "pr-changed-files.sh" +HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-merge-consent.sh" +WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml" + +SHA = "a9e3e23abf337980ca4c05854f5b1e210099d08b" +OTHER_SHA = "b71c0d4e2f8a91b3c5d7e9f1a3b5c7d9e1f3a5b7" + +# Serves paged `pulls/N/files`, plus the PR object the script re-reads to bind the enumeration. +CURL_SHIM = r'''#!/usr/bin/env python3 +import json, os, sys, pathlib, urllib.parse + +state = pathlib.Path(os.environ["STUB_DIR"]) +args = sys.argv[1:] +url = [a for a in args if a.startswith("http")][-1] + +if "/pulls/" in url and "/files" in url: + q = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) + page = int(q.get("page", ["1"])[0]) + pages = json.loads((state / "pages.json").read_text()) + if page > len(pages): + print("[]"); sys.exit(0) + entry = pages[page - 1] + if entry == "ERROR": # transport failure on this page + sys.exit(22) + if entry == "GARBAGE": # 200 with a non-array body (proxy/error page) + print('{"message":"internal error"}'); sys.exit(0) + print(json.dumps(entry)); sys.exit(0) + +if "/pulls/" in url: + # A second head sha served from the Nth PR-object read onward models a force-push landing + # between pagination round-trips. + sha = os.environ["STUB_SHA"] + alt = state / "pr_sha_after.txt" + if alt.exists(): + sha = alt.read_text().strip() + print(json.dumps({"head": {"sha": sha}})) + sys.exit(0) + +print("{}") +''' + + +def _rows(paths, status="modified"): + return [{"filename": p, "status": status} for p in paths] + + +@pytest.fixture +def enumerate_files(tmp_path): + bindir = tmp_path / "bin"; bindir.mkdir() + shim = bindir / "curl"; shim.write_text(CURL_SHIM); shim.chmod(0o755) + state = tmp_path / "state"; state.mkdir() + + env = dict(os.environ) + env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}" + env["STUB_DIR"] = str(state) + env["STUB_SHA"] = SHA + env["ETV_GITEA_TOKEN"] = "stub" + env["ETV_GITEA_URL"] = "http://gitea.example" + env.pop("ETV_GITEA_BASICAUTH", None) + env.pop("GITEA_TOKEN", None) + + class Handle: + def __init__(self, env_, state_): + self.env = env_ + self.state = state_ + + def set_pages(self, *pages): + (state / "pages.json").write_text(json.dumps(list(pages))) + + def head_moves_to(self, sha): + (state / "pr_sha_after.txt").write_text(sha) + + def run(self, expected_sha=SHA, args=("timothy", "ersatztv", "42")): + return subprocess.run( + ["bash", str(SCRIPT), *args, expected_sha], + env=self.env, capture_output=True, text=True) + + def paths(self): + """Assert success and return the enumerated path set.""" + r = self.run() + assert r.returncode == 0, f"expected success, got {r.returncode}: {r.stderr}" + return [ln for ln in r.stdout.splitlines() if ln.strip()] + + def fails_closed(self): + """The whole point: a NON-ZERO exit, asserted, not inferred.""" + r = self.run() + return r.returncode != 0 + + return Handle(env, state) + + +# --- The happy path, so the failure-path assertions below cannot pass vacuously ---------------- + +def test_complete_enumeration_returns_every_path(enumerate_files): + enumerate_files.set_pages(_rows(["docs/a.md", "ErsatzTV/Program.cs", "README.md"])) + assert enumerate_files.paths() == ["docs/a.md", "ErsatzTV/Program.cs", "README.md"] + + +def test_a_rename_contributes_BOTH_sides(enumerate_files): + """One row, two paths — the `git mv` hole. Reading `.filename` alone hides the source.""" + enumerate_files.set_pages([{"filename": "docs/innocuous-note.md", "status": "renamed", + "previous_filename": ".gitea/workflows/renovate.yml"}]) + assert sorted(enumerate_files.paths()) == [".gitea/workflows/renovate.yml", + "docs/innocuous-note.md"] + + +def test_paths_on_a_later_page_are_included(enumerate_files): + enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), + _rows(["scripts/decisions_lib.py"])) + assert "scripts/decisions_lib.py" in enumerate_files.paths() + + +# --- Fail-closed contract: each of these MUST be non-zero, by design --------------------------- + +def test_transport_failure_mid_pagination_fails_closed(enumerate_files): + """The defect that started all of this: an errored page counted as zero rows and read as + 'end of list', completing the enumeration over a PARTIAL list.""" + enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), "ERROR", + _rows(["docs/tail.md"])) + assert enumerate_files.fails_closed() + + +def test_non_array_body_fails_closed(enumerate_files): + enumerate_files.set_pages("GARBAGE") + assert enumerate_files.fails_closed() + + +def test_rows_without_filename_fail_closed(enumerate_files): + """`[{}]` is a well-formed array that yields no paths — a partial list wearing a valid shape.""" + enumerate_files.set_pages([{}, {}]) + assert enumerate_files.fails_closed() + + +def test_array_of_scalars_fails_closed(enumerate_files): + enumerate_files.set_pages(["docs/a.md", "docs/b.md"]) + assert enumerate_files.fails_closed() + + +@pytest.mark.parametrize("evil", ["safe.md\ndocs/Program.cs", "safe.md\rdocs/Program.cs"]) +def test_CRLF_in_filename_fails_closed(enumerate_files, evil): + """A newline splits one path into two lines that are each allow-list-matched separately, so + `safe.md\\ndocs/Program.cs` reads as two exempt paths while the real path ends in .cs.""" + enumerate_files.set_pages(_rows([evil])) + assert enumerate_files.fails_closed() + + +def test_CRLF_in_previous_filename_on_a_NON_renamed_row_fails_closed(enumerate_files): + """The hole one predicate wide: `previous_filename` is CONSUMED on every row, so it must be + VALIDATED on every row — not only where `.status == "renamed"` makes it semantically expected.""" + enumerate_files.set_pages([{"filename": "docs/a.md", "status": "modified", + "previous_filename": "safe.md\ndocs/Program.cs"}]) + assert enumerate_files.fails_closed() + + +def test_dotdot_path_component_fails_closed(enumerate_files): + """The callers' allow-lists anchor `^docs/`, which `docs/../ErsatzTV/Program.cs` matches.""" + enumerate_files.set_pages(_rows(["docs/../ErsatzTV/Program.cs"])) + assert enumerate_files.fails_closed() + + +@pytest.mark.parametrize("status", ["Renamed", "moved", ""]) +def test_status_outside_the_closed_allow_list_fails_closed(enumerate_files, status): + """Without a closed set, the `renamed => previous_filename REQUIRED` clause is dodgeable by any + other value, letting a `git mv` drop its source path and read as docs-only.""" + enumerate_files.set_pages([{"filename": "docs/a.md", "status": status, + "previous_filename": "ErsatzTV/Program.cs"}]) + assert enumerate_files.fails_closed() + + +def test_renamed_row_without_previous_filename_fails_closed(enumerate_files): + enumerate_files.set_pages([{"filename": "docs/a.md", "status": "renamed"}]) + assert enumerate_files.fails_closed() + + +def test_head_moving_during_enumeration_fails_closed(enumerate_files): + """A force-push between round-trips means page 1 came from head A and page 2 from head B, so + the assembled list belongs to no single commit.""" + enumerate_files.set_pages(_rows(["docs/a.md"])) + enumerate_files.head_moves_to(OTHER_SHA) + assert enumerate_files.fails_closed() + + +def test_missing_credentials_fails_closed(enumerate_files): + enumerate_files.set_pages(_rows(["docs/a.md"])) + enumerate_files.env.pop("ETV_GITEA_TOKEN", None) + assert enumerate_files.fails_closed() + + +@pytest.mark.parametrize("args", [("timothy", "ersatztv"), ("timothy", "ersatztv", "")]) +def test_usage_errors_exit_2(enumerate_files, args): + enumerate_files.set_pages(_rows(["docs/a.md"])) + r = enumerate_files.run(args=args) if len(args) == 3 else subprocess.run( + ["bash", str(SCRIPT), *args], env=enumerate_files.env, capture_output=True, text=True) + assert r.returncode == 2, r.stderr + + +def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files): + """Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS and may return fewer than asked. + 'Fewer than 50 rows means last page' would complete over a partial list without any transport + error — so termination requires a validated EMPTY page. A 30-row page followed by code must be + seen.""" + enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), + _rows(["ErsatzTV/Program.cs"])) + assert "ErsatzTV/Program.cs" in enumerate_files.paths() + + +# --- Drift guard: the reason this file is worth having at all ---------------------------------- + +def test_both_callers_use_the_shared_script_and_neither_reimplements_it(): + """ersatztv#649's third Done-when box: a test that fails if the two copies diverge again. + + Structural rather than behavioural on purpose. Behavioural equivalence tests would still pass if + someone pasted the loop back inline and kept it correct *that day* — which is exactly how the + drift happened the first time. What must be prevented is a SECOND implementation existing. + """ + for caller in (HOOK, WORKFLOW): + text = caller.read_text() + assert "scripts/pr-changed-files.sh" in text, ( + f"{caller.relative_to(REPO_ROOT)} no longer calls the shared enumeration") + # An inline `pulls//files?limit=` fetch is the signature of a re-inlined copy. + assert not re.search(r"pulls/\$?\{?\w+\}?/files\?limit=", text), ( + f"{caller.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — " + "that is the duplication ersatztv#649 removed") -- 2.47.3 From 63fa81fbb5eb320b71d395e83ef768b958efab49 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:45:05 +0200 Subject: [PATCH 2/7] docs(648,649): the jq contract + the shared PR-file enumeration record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds docs/ci-cd.md "The jq contract" (1.6 floor, the three divergent constructs, and the deliberate pin-vs-floor asymmetry with its merge-deadlock reason), plus two decision records: ci.jq-version-contract and ci.shared-pr-file-enumeration. ci.script-tests-job stops restating the three jq rules and points at the new record. Also corrects the script-tests preflight description: it is now two steps (git presence, then jq VERSION via scripts/jq-preflight.sh --expect 1.6), not one. A literal NUL byte had crept into the ci-cd.md paragraph describing jq 1.6's NUL truncation — which git treats as a binary file. Replaced with the literal text. Decisions-Edit: yes --- docs/ci-cd.md | 52 +++++++++++-- docs/decisions/README.md | 2 + .../records/ci/jq-version-contract.md | 69 +++++++++++++++++ docs/decisions/records/ci/script-tests-job.md | 17 ++--- .../records/ci/shared-pr-file-enumeration.md | 76 +++++++++++++++++++ 5 files changed, 200 insertions(+), 16 deletions(-) create mode 100644 docs/decisions/records/ci/jq-version-contract.md create mode 100644 docs/decisions/records/ci/shared-pr-file-enumeration.md diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 644107310..2ea1256a6 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -655,11 +655,53 @@ 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) + +> 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..df3cca13b --- /dev/null +++ b/docs/decisions/records/ci/jq-version-contract.md @@ -0,0 +1,69 @@ +--- +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`, which is advisory (it reddens the + run but is not one of the three required checks). `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. Failing here costs a red on a non-required job; failing the same way on + `review-verdict.yml` costs every PR its ability to merge. + +**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..97384d343 --- /dev/null +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -0,0 +1,76 @@ +--- +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`' +--- + +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`/`modified`/`removed`/`renamed`/`copied`), not an open + denylist. +- `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. + +**`review-verdict.yml` also now checks out the PR's BASE ref, never the head**, so the workflow that +judges a PR cannot be rewritten by that same PR to weaken its own judgment — a PR editing +`review-verdict.yml` executes the *pre-PR* version of the gate against itself. + +**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. -- 2.47.3 From 2c10f057b85e10a0d298f09d2c21636ff0154524 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:53:28 +0200 Subject: [PATCH 3/7] fix(648,649): stage the enforced-gate wiring behind the scripts it calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splits the review-verdict.yml rewiring out of this PR. That workflow checks out the PR's BASE ref — deliberately, so a PR cannot rewrite the gate that judges it — and the base is main, which does not yet contain scripts/pr-changed-files.sh or scripts/jq-preflight.sh. Wiring it here would make the job exit 127 on its own PR and block the merge gate through the combined status, which reads red jobs as blocking. So this PR lands the scripts, their tests, the hook rewiring and the script-tests jq pin; the follow-up points review-verdict.yml at them once they exist on main. The two tests that asserted on review-verdict.yml are scoped accordingly, each carrying the reason. test_review_verdict_never_pins_a_jq_version is asserted NOW rather than in the follow-up, so the no-pin constraint on the required check is already enforced when the wiring lands. Decisions-Edit: yes --- .gitea/workflows/review-verdict.yml | 89 +++++++++---------- docs/ci-cd.md | 7 ++ .../records/ci/shared-pr-file-enumeration.md | 5 ++ scripts/tests/test_jq_preflight.py | 25 ++++-- scripts/tests/test_pr_changed_files.py | 27 +++--- 5 files changed, 88 insertions(+), 65 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 73ab968aa..82f5b7194 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -52,23 +52,6 @@ jobs: name: Set review-verdict status runs-on: small # a few API calls; keep it off the build runners steps: - # Check out the BASE ref, NEVER the PR head. This job now runs scripts/pr-changed-files.sh from - # the checkout, and checking out the head would let a PR rewrite the very gate that judges it — - # the PROTECTED list below could not save us, because the tampered script would already have - # produced the file list that list is applied to. `base.sha` is a commit already on the target - # branch, so it is trusted by construction. (ersatztv#649) - - name: Checkout the base ref (trusted; never the PR head) - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.base.sha }} - persist-credentials: false - # Floor-only: print the jq version so a future divergence is diagnosable from the log alone, and - # refuse anything below the supported floor. Deliberately NOT pinned with `--expect` — this - # workflow writes the branch-protection-required `review-verdict/h10` status, so a hard version - # pin here would turn any jq bump on the runner into a repo-wide merge deadlock. The pin lives on - # `script-tests` in pr-checks.yml instead. See docs/ci-cd.md -> "The jq contract". (ersatztv#648) - - name: Preflight jq version - run: ./scripts/jq-preflight.sh - name: Classify the PR and post the review-verdict status env: GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }} @@ -133,36 +116,50 @@ jobs: exit 0 fi - # --- Changed files: enumerate exhaustively, or fail CLOSED. -------------------------- - # This was an inline copy of logic that also lives in - # .claude/hooks/pretooluse-merge-consent.sh. The two drifted, and badly: four rounds of - # ersatztv#643 hardening landed on the HOOK — the advisory copy, whose failures produce a - # human prompt — and never reached this one, the ENFORCED copy that writes the - # branch-protection-required status. The advisory copy ended up strictly stricter than the - # authoritative one. Worse, a round-4 review traced that this copy's safe behaviour on a - # garbage response was INCIDENTAL: `n` came back empty, `[ "$n" -lt 50 ]` errored to false, - # the loop ran to MAX_PAGES and left complete=no. Correct outcome, arrived at by a bash - # arithmetic error that any refactor of the loop could have flipped. - # - # Both callers now share ONE implementation whose fail-closed behaviour is intentional and - # asserted by scripts/tests/test_pr_changed_files.py (ersatztv#649). It carries the six - # guards this copy was missing: CR/LF rejection, `..` rejection, a closed `.status` - # allow-list, `previous_filename` validated on EVERY row rather than only `renamed` ones, - # termination only on a validated EMPTY page (never a merely short one), and binding the - # whole enumeration to a single head sha. - # - # A non-zero exit means "could not tell" and MUST withhold the exemption. Note the script - # is invoked WITHOUT `|| true`: `set -e` is not in play for an `if` condition, and reading - # its stdout without checking its status is the exact mistake this replaces. - if files=$(GITEA_TOKEN="$GITEA_TOKEN" GITEA_BASE_URL="$BASE_URL" \ - ./scripts/pr-changed-files.sh "${REPO%%/*}" "${REPO##*/}" "$PR" "$SHA"); then - complete=yes - else - complete=no - files="" - fi + # --- Changed files: PAGE to exhaustion, and fail CLOSED if we cannot. ---------------- + # Gitea caps this endpoint at 50 rows per page and SILENTLY IGNORES a larger `limit` + # (verified: PR #619 has 194 changed files and `?limit=100` returns exactly 50). A + # single-page read is therefore a silent false negative: a protected path sitting at + # position 51+ would simply not be seen, and a bot-authored PR that edits the gate could + # exempt itself from the gate. Page until a short page proves the end. + PAGE_SIZE=50 + MAX_PAGES=40 # 2000 files; beyond this we refuse rather than guess + files="" + page=1 + complete=no + while [ "$page" -le "$MAX_PAGES" ]; do + raw=$(gh "$BASE_URL/repos/$REPO/pulls/$PR/files?limit=${PAGE_SIZE}&page=${page}") || raw="" + # A transport/parse failure must NOT masquerade as a legitimate short final page. + # Empty output counts as zero rows, which would otherwise read as "end of list" and set + # complete=yes over a PARTIAL enumeration — failing OPEN at the exact point this guard + # exists to fail closed. + # + # Validating only the TOP-LEVEL type is not enough: a page like `[{}]` is a well-formed + # array whose rows carry no `filename`, so it contributes no paths, counts as a short + # page, and completes the enumeration from a partial list — the same failure one level + # down. Require every row to carry a non-empty string `filename`; an empty array stays + # valid, since that is what a genuine end-of-pagination looks like. + if ! printf '%s' "$raw" \ + | jq -e 'type == "array" and all(.[]; (.filename | type == "string" and length > 0) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))' \ + >/dev/null 2>&1; then + complete=no; break + fi + # Page-size termination is measured in ROWS; the path set collects BOTH sides of a + # rename. Gitea reports a `git mv` as ONE row whose `filename` is the DESTINATION, with + # the source only in `previous_filename` — so reading `filename` alone lets a PR move a + # protected file INTO docs/ and pass as docs-only (verified live: + # `.gitea/workflows/renovate.yml` -> `docs/innocuous-note.md` showed no protected path). + # One renamed row thus contributes ONE to `n` and TWO to the path set, which is why + # these two counts are deliberately computed differently. + n=$(printf '%s' "$raw" | jq -r 'length') + chunk=$(printf '%s' "$raw" | jq -r '.[] | (.filename // empty), (.previous_filename // empty)') + [ -n "$chunk" ] && files=$(printf '%s\n%s' "$files" "$chunk") + if [ "$n" -lt "$PAGE_SIZE" ]; then complete=yes; break; fi + page=$((page + 1)) + done + files=$(printf '%s\n' "$files" | grep -v '^$' || true) count=$(printf '%s\n' "$files" | grep -c . || true) - echo "Changed files (${count}, complete=${complete}):" + echo "Changed files (${count}, complete=${complete}, pages=${page}):" printf '%s\n' "$files" | sed 's/^/ /' exempt=no diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 2ea1256a6..2b27b03fb 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -665,6 +665,13 @@ the runner image. ### The jq contract (ersatztv#648) +> **Landing note (staged over two PRs).** The `review-verdict.yml` half of this — the floor-only +> preflight and the call into `scripts/pr-changed-files.sh` — lands in the FOLLOW-UP PR, not the one +> that introduces these scripts. That workflow checks out the PR's **base** ref, and the base is +> `main`; until these scripts exist on `main`, wiring the workflow would make the job exit 127 on its +> own PR and block the merge gate through the combined status. The scripts land first, then the +> enforced gate is pointed at them. + > Full rationale: `docs/decisions/records/ci/jq-version-contract.md`. Every shell gate in this repo — `decisions-guard`, `script-tests`'s own harness, diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index 97384d343..1cdbaa263 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -10,6 +10,11 @@ signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hoo mechanics: '`scripts/pr-changed-files.sh ` -> stdout newline-delimited paths, exit 0 only if complete and bound to the given sha; callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`' --- +> **Landing note.** This record describes the end state. It is delivered over two PRs: the shared +> script, its tests and the hook rewiring first; the `review-verdict.yml` rewiring second, once the +> script exists on `main`. The reason is the base-ref checkout below — a workflow that runs the base +> version of the gate cannot call a script the base does not yet have. + Before #649, the PR changed-file enumeration existed as two independent implementations. That would be an ordinary duplication smell anywhere else; here it was actively dangerous, because the two copies had unequal *consequence*. The advisory hook's failure mode is a human permission prompt — a diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py index 5f9b83f53..af7a4c900 100644 --- a/scripts/tests/test_jq_preflight.py +++ b/scripts/tests/test_jq_preflight.py @@ -125,20 +125,27 @@ def test_unknown_argument_is_a_usage_error(preflight): # --- Wiring guards: the preflight is worthless if a caller silently stops running it ------------ -def test_script_tests_pins_and_review_verdict_only_floors(): - """The asymmetry is deliberate and load-bearing, so it is asserted rather than merely commented. +def test_script_tests_pins_the_jq_version(): + """The pin is the tripwire, so its presence is asserted rather than merely commented. - `script-tests` carries `--expect` (a tripwire on a normal job). `review-verdict.yml` must NOT: - it writes the branch-protection-required `review-verdict/h10` status, so a hard version pin - there would turn a jq bump on the runner into a repo-wide merge deadlock. + SCOPE NOTE — the symmetric assertion about `review-verdict.yml` (that it runs the FLOOR-only + mode and must never pin, because it writes the branch-protection-required `review-verdict/h10` + status and a pin would deadlock every merge on a jq bump) lands with the follow-up PR that + wires that workflow. It cannot land here: that workflow checks out the BASE ref, and the base + is `main`, which does not yet contain `scripts/jq-preflight.sh`. """ pr_checks = (WORKFLOWS / "pr-checks.yml").read_text() - review_verdict = (WORKFLOWS / "review-verdict.yml").read_text() - assert "jq-preflight.sh --expect" in pr_checks, \ "script-tests must pin the jq version — that pin is the tripwire" - assert "jq-preflight.sh" in review_verdict, \ - "review-verdict.yml must at least print/floor-check its jq version" + + +def test_review_verdict_never_pins_a_jq_version(): + """Whatever else changes, the REQUIRED merge check must never carry a hard version pin. + + Asserted now, before the workflow is wired, so the constraint is already enforced when the + follow-up PR adds the floor-only call — rather than being a comment someone can miss. + """ + review_verdict = (WORKFLOWS / "review-verdict.yml").read_text() assert "jq-preflight.sh --expect" not in review_verdict, \ ("review-verdict.yml must NOT pin a jq version: it writes the required review-verdict/h10 " "status, so a pin would deadlock every merge on a jq bump (ersatztv#648)") diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index bd0e27c4e..9af499481 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -239,18 +239,25 @@ def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files): # --- Drift guard: the reason this file is worth having at all ---------------------------------- -def test_both_callers_use_the_shared_script_and_neither_reimplements_it(): - """ersatztv#649's third Done-when box: a test that fails if the two copies diverge again. +def test_the_hook_uses_the_shared_script_and_does_not_reimplement_it(): + """ersatztv#649's third Done-when box: a test that fails if a copy is re-inlined. Structural rather than behavioural on purpose. Behavioural equivalence tests would still pass if someone pasted the loop back inline and kept it correct *that day* — which is exactly how the drift happened the first time. What must be prevented is a SECOND implementation existing. + + SCOPE NOTE — this asserts the HOOK only, deliberately. `.gitea/workflows/review-verdict.yml` is + the other caller, but its rewiring cannot land in this PR: that workflow will check out the BASE + ref (never the PR head, so a PR cannot rewrite the gate judging it), and the base is `main`, + which does not yet contain the scripts this PR ADDS. Wiring it here would make the job exit 127 + on its own PR and block the merge gate via the combined status. The workflow half therefore + lands in the follow-up PR, once these scripts are on `main`, and that PR extends this test to + cover both callers. """ - for caller in (HOOK, WORKFLOW): - text = caller.read_text() - assert "scripts/pr-changed-files.sh" in text, ( - f"{caller.relative_to(REPO_ROOT)} no longer calls the shared enumeration") - # An inline `pulls//files?limit=` fetch is the signature of a re-inlined copy. - assert not re.search(r"pulls/\$?\{?\w+\}?/files\?limit=", text), ( - f"{caller.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — " - "that is the duplication ersatztv#649 removed") + text = HOOK.read_text() + assert "scripts/pr-changed-files.sh" in text, ( + f"{HOOK.relative_to(REPO_ROOT)} no longer calls the shared enumeration") + # An inline `pulls//files?limit=` fetch is the signature of a re-inlined copy. + assert not re.search(r"pulls/\$?\{?\w+\}?/files\?limit=", text), ( + f"{HOOK.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — " + "that is the duplication ersatztv#649 removed") -- 2.47.3 From 5e7623b8d5940a5d3d96b545d62c499118bd3ffe Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:37:48 +0200 Subject: [PATCH 4/7] =?UTF-8?q?fix(648,649):=20security-review=20round=202?= =?UTF-8?q?=20=E2=80=94=20close=20the=20version-parse=20hole=20and=20the?= =?UTF-8?q?=20untested=20caller=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real defects, and three docs claims that were simply wrong. jq-preflight.sh parsed the version by stripping around the first `-` and `.`, which assumed the format is exactly `jq-X.Y`. A build printing `jq version 1.6` left major empty; the sanity check concatenated major+minor into "6", which is non-empty and all-digits, so it PASSED. The floor comparison then ran `[ "" -lt 1 ]`, which errors — and `set -e` exempts a failing command in an `if` condition, so the conditional read false and the script exited 0 having asserted nothing, after printing a plausible "parsed" line. The silently-untested-axis failure this script exists to eliminate, reproduced inside the script itself. Now parsed by explicit regex, failing closed with a diagnosis when there is no . match. Also: `--expect` with no value exited 1 with empty output on both streams. The hook's exit-status check was pinned by nothing: mutating `if files=$(...)` into `files=$(...) || true; files_complete=yes` left the ENTIRE suite green. It survived only by redundancy — the script writes stdout once, right before exit 0, so failures also happen to yield empty stdout and `[ -n "$files" ]` catches it. Safe by accident, which is the exact criticism this branch levels at the old code. Four tests now pin it, with a stub that FAILS while emitting a docs-only list (the one case redundancy cannot absorb) plus a positive control proving the harness can see the difference. Verified: the mutation now turns exactly those tests red. Docs corrections. The record claimed the --expect pin was safe because script-tests is "advisory, not a required check" — false. The merge-consent hook reads the COMBINED status (ci.advisory-red-blocks-the-merge-gate, #598), so firing the tripwire blocks every non-docs-only merge until someone re-pins. Kept anyway, for a stated reason, but no longer described as free. The record also asserted in the present tense that review-verdict.yml checks out the base ref; it has no checkout step at all, so that is now a future-tense requirement on the follow-up. And the documented .status allow-list named GitHub's `removed`, which the code rejects. The drift-guard regex anchored on `?limit=`, so a re-inlined copy written `files?page=1&limit=50` would have walked past it. Decisions-Edit: yes --- .../records/ci/jq-version-contract.md | 29 +++-- .../records/ci/shared-pr-file-enumeration.md | 23 +++- scripts/jq-preflight.sh | 42 +++++-- scripts/pr-changed-files.sh | 12 +- scripts/tests/test_jq_preflight.py | 40 ++++++ scripts/tests/test_pr_changed_files.py | 114 +++++++++++++++++- 6 files changed, 229 insertions(+), 31 deletions(-) diff --git a/docs/decisions/records/ci/jq-version-contract.md b/docs/decisions/records/ci/jq-version-contract.md index df3cca13b..2be530047 100644 --- a/docs/decisions/records/ci/jq-version-contract.md +++ b/docs/decisions/records/ci/jq-version-contract.md @@ -46,13 +46,28 @@ assumed. So the fix has to hold at 1.6, in every gate, regardless of which lane 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`, which is advisory (it reddens the - run but is not one of the three required checks). `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. Failing here costs a red on a non-required job; failing the same way on - `review-verdict.yml` costs every PR its ability to merge. +- `--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 every non-docs-only merge in the repo until + someone re-pins. + + 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 three constructs to avoid, and their version-stable replacements:** diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index 1cdbaa263..a0b3c420c 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -59,8 +59,12 @@ different. in each caller: - CR/LF rejection and `..` rejection on every path. -- A closed `.status` allow-list (`added`/`modified`/`removed`/`renamed`/`copied`), not an open - denylist. +- 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 @@ -71,9 +75,18 @@ in each caller: list if it moved mid-enumeration, since paging is several round-trips and a force-push between them would otherwise yield a list belonging to no single commit. -**`review-verdict.yml` also now checks out the PR's BASE ref, never the head**, so the workflow that -judges a PR cannot be rewritten by that same PR to weaken its own judgment — a PR editing -`review-verdict.yml` executes the *pre-PR* version of the gate against itself. +**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 diff --git a/scripts/jq-preflight.sh b/scripts/jq-preflight.sh index 0f995a017..e3fce55bd 100755 --- a/scripts/jq-preflight.sh +++ b/scripts/jq-preflight.sh @@ -53,7 +53,15 @@ MIN_VERSION="1.6" expect="" while [ "$#" -gt 0 ]; do case "$1" in - --expect) expect="${2:-}"; shift 2 ;; + --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 @@ -65,23 +73,31 @@ fi raw=$(jq --version 2>&1 || true) # `jq --version` prints e.g. `jq-1.6`, `jq-1.7.1`, or on some builds `jq-1.8.2-dirty`. -version=${raw#jq-} -major=${version%%.*} -rest=${version#*.} -minor=${rest%%.*} -# Some builds append a suffix with no further dot ("jq-1.6-dirty", "jq-1.7rc1"), which would otherwise -# leave a non-numeric minor and fail closed on a perfectly ordinary jq. Keep the leading digits only. -major=${major%%[!0-9]*} -minor=${minor%%[!0-9]*} +# 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. +if [[ "$raw" =~ ([0-9]+)\.([0-9]+) ]]; then + major="${BASH_REMATCH[1]}" + minor="${BASH_REMATCH[2]}" +else + echo "jq-preflight: could not parse a major.minor version out of '${raw}'. 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. echo "jq-preflight: jq version in use = ${raw} (parsed ${major}.${minor}; floor ${MIN_VERSION})" -case "$major$minor" in - *[!0-9]*|"") echo "jq-preflight: could not parse a major.minor out of '${raw}' — failing closed" >&2; exit 1 ;; -esac - min_major=${MIN_VERSION%%.*} min_minor=${MIN_VERSION#*.} if [ "$major" -lt "$min_major" ] || { [ "$major" -eq "$min_major" ] && [ "$minor" -lt "$min_minor" ]; }; then diff --git a/scripts/pr-changed-files.sh b/scripts/pr-changed-files.sh index 2e393b7cf..e6be5001b 100755 --- a/scripts/pr-changed-files.sh +++ b/scripts/pr-changed-files.sh @@ -123,9 +123,15 @@ while [ "$page" -le "$MAX_PAGES" ]; do # * `..` is rejected because the callers' allow-lists anchor `^docs/`, so `docs/../ErsatzTV/Program.cs` # matches one. Git will not produce such a path; this guard's job is to fail closed on unexpected # 2xx shapes rather than assume a well-behaved peer. - # * `.status` is checked against a CLOSED set. Without it the `renamed => previous_filename REQUIRED` - # clause is dodgeable by any other value — `"Renamed"` with a capital R, or an absent status — - # letting a `git mv ErsatzTV/Program.cs -> docs/a.md` drop its source path and read as docs-only. + # * `.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 diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py index af7a4c900..b3861cc0b 100644 --- a/scripts/tests/test_jq_preflight.py +++ b/scripts/tests/test_jq_preflight.py @@ -123,6 +123,46 @@ def test_unknown_argument_is_a_usage_error(preflight): 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 + + # --- Wiring guards: the preflight is worthless if a caller silently stops running it ------------ def test_script_tests_pins_the_jq_version(): diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 9af499481..9b5079805 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -33,7 +33,7 @@ import pytest REPO_ROOT = Path(__file__).resolve().parents[2] SCRIPT = REPO_ROOT / "scripts" / "pr-changed-files.sh" HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-merge-consent.sh" -WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "review-verdict.yml" +# (the enforced caller, .gitea/workflows/review-verdict.yml, is wired in the follow-up PR) SHA = "a9e3e23abf337980ca4c05854f5b1e210099d08b" OTHER_SHA = "b71c0d4e2f8a91b3c5d7e9f1a3b5c7d9e1f3a5b7" @@ -237,6 +237,110 @@ def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files): 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(): @@ -257,7 +361,11 @@ def test_the_hook_uses_the_shared_script_and_does_not_reimplement_it(): text = HOOK.read_text() assert "scripts/pr-changed-files.sh" in text, ( f"{HOOK.relative_to(REPO_ROOT)} no longer calls the shared enumeration") - # An inline `pulls//files?limit=` fetch is the signature of a re-inlined copy. - assert not re.search(r"pulls/\$?\{?\w+\}?/files\?limit=", text), ( + # 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. A drift guard one refactor away from decorative is worse than none, because it reads as + # coverage. + assert not re.search(r"pulls/\$?\{?\w+\}?/files\?", text), ( f"{HOOK.relative_to(REPO_ROOT)} appears to enumerate PR files inline again — " "that is the duplication ersatztv#649 removed") -- 2.47.3 From 4e094637c62736f0a4c332715ef49cf04ce47810 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:47:40 +0200 Subject: [PATCH 5/7] fix(648): the version parser was fail-OPEN on a jq that cannot start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3, and it found that round 2's fix was a REGRESSION on the case that matters most. `raw=$(jq --version 2>&1 || true)` did two wrong things at once: folded stderr into the parse input and discarded the exit status. Combined with a pattern that matched the first . ANYWHERE, a jq broken by a glibc mismatch — which exits 127 and writes "version `GLIBC_2.34' not found" to stderr — parsed as version 2.34 and PASSED the floor. The strip-based parse this replaced failed CLOSED there. So the fix for a fail-open bug introduced a worse fail-open bug, in the one script whose entire purpose is to refuse to certify a version it did not parse. Same mechanism, second symptom: an unanchored match let a prefix outrank the real version. `2026.07.26 jq-1.6` parsed as 2026.07; a leading warning line carrying any number won too. Now: jq's exit status is captured explicitly (`$?` inside `if ! cmd` is the NEGATED status, so that needed care too), stderr is kept out of the parse, and the pattern is anchored to the leading `jq` token. Every legitimate form still parses — `jq-1.6`, `jq version 1.6`, `jq-1.7.1`, `jq-1.6-dirty`, `jq-1.6 (Debian 1.6-2.1)`, `jq-1.10` (numeric compare, so the two-digit minor is not read lexically). The tests could not have caught any of this: the shim always exited 0 and never wrote to stderr, so every case it could express was clean. It now takes stderr and an exit code, and the four new cases turn red under the exact mutation. Also: the drift guard now strips comment lines before matching. A future comment citing `pulls/$pr/files?limit=100` as an example of what not to do would otherwise have reddened script-tests — which, per this branch's own correction, blocks merges. And the record no longer over-corrects: the combined-status read is guarded by `if [ "$mwcs" != "true" ]`, so a red script-tests blocks the hook-mediated merge path, not literally every merge. Decisions-Edit: yes --- .../records/ci/jq-version-contract.md | 9 ++- scripts/jq-preflight.sh | 31 ++++++-- scripts/tests/test_jq_preflight.py | 71 +++++++++++++++++-- scripts/tests/test_pr_changed_files.py | 11 ++- 4 files changed, 108 insertions(+), 14 deletions(-) diff --git a/docs/decisions/records/ci/jq-version-contract.md b/docs/decisions/records/ci/jq-version-contract.md index 2be530047..98147b51b 100644 --- a/docs/decisions/records/ci/jq-version-contract.md +++ b/docs/decisions/records/ci/jq-version-contract.md @@ -58,8 +58,13 @@ assumed. So the fix has to hold at 1.6, in every gate, regardless of which lane 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 every non-docs-only merge in the repo until - someone re-pins. + 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 diff --git a/scripts/jq-preflight.sh b/scripts/jq-preflight.sh index e3fce55bd..4549d92aa 100755 --- a/scripts/jq-preflight.sh +++ b/scripts/jq-preflight.sh @@ -71,7 +71,24 @@ if ! command -v jq >/dev/null 2>&1; then exit 1 fi -raw=$(jq --version 2>&1 || true) +# 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 `.`. # @@ -86,9 +103,15 @@ raw=$(jq --version 2>&1 || true) # # 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. -if [[ "$raw" =~ ([0-9]+)\.([0-9]+) ]]; then - major="${BASH_REMATCH[1]}" - minor="${BASH_REMATCH[2]}" +# 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. +if [[ "$raw" =~ ^[[:space:]]*[Jj][Qq][[:space:]-]+(version[[:space:]]+)?v?([0-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 '${raw}'. Refusing to assert a floor against an unparsed version — that would silently pass." >&2 exit 1 diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py index b3861cc0b..2902571c6 100644 --- a/scripts/tests/test_jq_preflight.py +++ b/scripts/tests/test_jq_preflight.py @@ -29,18 +29,35 @@ WORKFLOWS = REPO_ROOT / ".gitea" / "workflows" 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): - """Install a fake `jq` reporting `version_line` for --version.""" + 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" - shim.write_text("#!/bin/sh\n" - 'if [ "$1" = "--version" ]; then echo "%s"; exit 0; fi\nexit 0\n' - % version_line) + 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): @@ -163,6 +180,50 @@ def test_unparseable_version_fails_CLOSED_rather_than_asserting_nothing(prefligh 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,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"), +]) +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(): diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 9b5079805..a47d5b351 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -361,11 +361,16 @@ def test_the_hook_uses_the_shared_script_and_does_not_reimplement_it(): 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. A drift guard one refactor away from decorative is worse than none, because it reads as - # coverage. - assert not re.search(r"pulls/\$?\{?\w+\}?/files\?", text), ( + # 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") -- 2.47.3 From 807ebbd38eec9baca2232d9c22faf05107e67865 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 22:02:00 +0200 Subject: [PATCH 6/7] =?UTF-8?q?fix(648):=20round=204=20=E2=80=94=20the=20r?= =?UTF-8?q?ound-1=20fail-open=20was=20still=20reachable,=20via=20an=20over?= =?UTF-8?q?-long=20number?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 found the round-1 MECHANISM alive in round 3's regex. The pattern guaranteed the operands were digits but not that they fit `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. That is precisely what the empty string did in round 1: same shape, third occurrence, same predicate. Bounding the runs with {1,9} was not sufficient on its own. The pattern 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 rather than an error that skips it. The trailing `([^0-9]|$)` is what actually closes it. Second hole: `[[:space:]]` matches NEWLINES, so round 3's "anchor" still scanned the whole output. `jq\n2.34: cannot load shared library` matched `jq`, crossed the newline as separator, and parsed 2.34. Now the first line only, with `[[:blank:]]`. Third: the separator class `[-[:blank:]]{1,4}` could be walked across filler — `jq -- 2.34 (real jq-1.6)` parsed as 2.34, `jq9.9` as 9.9. It is now one of the two forms real jq emits: `jq-1.6` or `jq version 1.6` (a blank separator REQUIRES the literal word `version`). Verified across a 20-case matrix: every legitimate form still parses to the right numbers (jq-1.6, jq version 1.6, jq-1.7.1, jq-1.6-dirty, jq-1.10 numerically, jq-1.6 (Debian 1.6-2.1), jq-v1.6, JQ-1.6, jq-1.6.0, CRLF), and every constructed attack fails closed. Four mutations, each reddening exactly its own tests. The real jq 1.8.2 on this machine still reports cleanly. Also: the log line now interpolates the first line, so a multi-line --version cannot split the single grep-able line the no-arg mode exists to emit. None of these are reachable from a real jq build. They are recorded and fixed because the guard's own stated invariant — never assert a floor against something it did not parse — was still violable three rounds in, and the follow-up PR moves this exact code into the branch-protection-required check. --- scripts/jq-preflight.sh | 33 +++++++++++++++++-- scripts/tests/test_jq_preflight.py | 51 ++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/scripts/jq-preflight.sh b/scripts/jq-preflight.sh index 4549d92aa..5b3751828 100755 --- a/scripts/jq-preflight.sh +++ b/scripts/jq-preflight.sh @@ -109,17 +109,44 @@ fi # 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. -if [[ "$raw" =~ ^[[:space:]]*[Jj][Qq][[:space:]-]+(version[[:space:]]+)?v?([0-9]+)\.([0-9]+) ]]; then +# 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 '${raw}'. Refusing to assert a floor against an unparsed version — that would silently pass." >&2 + 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. -echo "jq-preflight: jq version in use = ${raw} (parsed ${major}.${minor}; floor ${MIN_VERSION})" +# `$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#*.} diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py index 2902571c6..73ef6ba49 100644 --- a/scripts/tests/test_jq_preflight.py +++ b/scripts/tests/test_jq_preflight.py @@ -212,10 +212,61 @@ def test_a_number_that_is_not_the_VERSION_is_not_accepted_as_one(preflight, vers 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", [ + "jq\n2.34: cannot load shared library", # NEWLINE crossed by a [[:space:]] separator + "jq\n\n\n99.9", + "jq -- 2.34 (real jq-1.6)", # filler walked by a permissive separator class + "jq\t\t9.9", +]) +def test_a_number_AFTER_the_jq_token_is_not_reachable_across_filler(preflight, version_line): + """`[[:space:]]` matches NEWLINES, so the round-3 'anchor' still scanned the whole output — + `jq\\n2.34: cannot load` matched `jq`, crossed the newline as separator, and parsed 2.34. + Parsing the first line only, and requiring one of the two separator forms real jq emits + (`jq-1.6` or `jq version 1.6`), confines the match to a real version token.""" + preflight.with_jq(version_line) + r = preflight.run() + assert r.returncode == 1, f"{version_line!r} was accepted as a version" + + +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) -- 2.47.3 From b255b7ffdcc433f29c4655538aac897886efcfd6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 22:20:33 +0200 Subject: [PATCH 7/7] =?UTF-8?q?test(648):=20close=20the=20mutation=20gaps?= =?UTF-8?q?=20round=205=20found=20=E2=80=94=20two=20tests=20passed=20for?= =?UTF-8?q?=20the=20wrong=20reason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 could not break the predicate itself: 28,930 real runs of the script across 14,465 crafted --version strings on bash 3.2.57 and 5.3.15 produced zero fail-opens, and `{1,9}` is honoured on bash 3.2, so round 4's bound is not void on the authoring Macs. What it did find is that two of round 4's changes were unpinned, and the tests that looked like they covered them did not. Reverting BOTH the first-line slice and `[[:blank:]]`→`[[:space:]]` together left the whole suite green. The four filler cases are all killed by the SEPARATOR restriction alone, so they attributed the fix to the wrong layer. Added three cases carrying the literal word `version` (`jq\nversion\n9.9` and friends), which satisfy the separator rule and can only be stopped by confining the parse to line one with a newline-free blank class. The CR-strip test was worse: vacuous through two independent mechanisms. `str.splitlines()` also splits on `\r`, so a per-line view dropped the stray CR; and `subprocess.run(text=True)` translates `\r` to `\n` outright, so even a raw-string check on stdout was unfalsifiable. The mutant demonstrably emits `... = jq-1.6 (parsed 1.6; ...)` at the byte level while the test reported green. Added `run_bytes()` and a bytes comparison. Both gaps are now mutation-verified: reverting either change reddens exactly its own test. Also records the operational edge this parser acquires in the follow-up: it is strictly fail-closed by design, so once the floor mode gates the required check, a jq wrapper that prints a banner line would deadlock merges. The fix there is to widen the accepted forms, never to relax fail-closed. Decisions-Edit: yes --- .../records/ci/jq-version-contract.md | 14 +++++ scripts/tests/test_jq_preflight.py | 58 +++++++++++++++++-- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/docs/decisions/records/ci/jq-version-contract.md b/docs/decisions/records/ci/jq-version-contract.md index 98147b51b..707ea925a 100644 --- a/docs/decisions/records/ci/jq-version-contract.md +++ b/docs/decisions/records/ci/jq-version-contract.md @@ -74,6 +74,20 @@ assumed. So the fix has to hold at 1.6, in every gate, regardless of which lane 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. diff --git a/scripts/tests/test_jq_preflight.py b/scripts/tests/test_jq_preflight.py index 73ef6ba49..ccc1cfe32 100644 --- a/scripts/tests/test_jq_preflight.py +++ b/scripts/tests/test_jq_preflight.py @@ -76,6 +76,20 @@ def preflight(tmp_path): 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() @@ -234,21 +248,53 @@ def test_an_OUT_OF_RANGE_digit_run_fails_closed(preflight, version_line): @pytest.mark.parametrize("version_line", [ - "jq\n2.34: cannot load shared library", # NEWLINE crossed by a [[:space:]] separator + # 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)", # filler walked by a permissive separator class + "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): - """`[[:space:]]` matches NEWLINES, so the round-3 'anchor' still scanned the whole output — - `jq\\n2.34: cannot load` matched `jq`, crossed the newline as separator, and parsed 2.34. - Parsing the first line only, and requiring one of the two separator forms real jq emits - (`jq-1.6` or `jq version 1.6`), confines the match to a real version token.""" + """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.""" -- 2.47.3