fix(648): an explicit jq version contract + one shared PR-file enumeration #658
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#658
Reference in New Issue
Block a user
fixes #648
Lands the jq version contract, and the shared PR-file enumeration that #649 needs. #649 stays open: its enforced half is a deliberate follow-up, explained below.
#648 — the decision, with the small-lane constraint checked rather than assumed
The issue offered three options and declined to pick. Option 1 (bake a pinned jq into
docker/ci/Dockerfile) is ruled out by measurement:.gitea/workflows/review-verdict.ymlisruns-on: small, carries no toolchain-image pin, and perci.small-lane-git-onlythe small lane is git-only — so it gets the host's jq 1.6 regardless of what the toolchain image contains. That is the exact workflow where the #647 bug was live, so the pin would have missed it entirely.So the contract is inverted: jq 1.6 is the floor, and it is the runner's own jq that provides the 1.6 coverage
scripts/tests/runs under.scripts/jq-preflight.shhas two deliberately asymmetric modes:--expect 1.6— additionally pinscript-testsonlyThe asymmetry is load-bearing.
review-verdict.ymlwritesreview-verdict/h10, the branch-protection-required status. A hard pin there would turn any jq bump on the runner — a base-image bump, a host reimage, nothing this repo controls — into a repo-wide merge deadlock. A required merge gate must not fail because an upstream package manager did its job.Conversely the pin on
script-testsmatters 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.#649 (partial) — one enumeration instead of two
The enforced
review-verdict.ymlguard 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 a required status. Its fail-closed behaviour on a garbage response was also incidental —ncame back empty,[ "$n" -lt 50 ]errored to false, the loop ran toMAX_PAGES. The right answer, reached through a bash arithmetic error any refactor could have flipped.scripts/pr-changed-files.shis now the single implementation. Shared mechanism, not policy — the two docs-only allow-lists differ on purpose (the hook's is broader because it falls through to a human prompt; the workflow's is narrower because 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.Note the issue's asymmetry table was already partly stale when filed:
bb1809fbhad since ported two of the eight guards. The real gap was six, and all six are in the shared script.Why the enforced half is a separate PR
review-verdict.ymlwill check out the PR's base ref, never the head, so a PR cannot rewrite the gate that judges it. The base ismain— which does not yet contain these scripts. Wiring it in this PR would make the job exit 127 on its own PR and block the merge gate through the combined status. Scripts land first; the follow-up points the enforced gate at them.Verification
Mutation-tested, not just green. Each of the six guards was individually broken and the suite re-run; every mutation turned exactly its own test red, and the file restored byte-identical:
..rejection removed.statusallow-list removedprevious_filenamechecked only onrenamedscripts/tests: 259 passed.Five rounds on one predicate
The version parser in
scripts/jq-preflight.shtook four revisions, and rounds 2, 3 and 4 each found a defect in the previous round's fix. They share one signature, which is worth naming because it is the thing to look for next time:jq version 1.6left the major empty; the sanity check saw"6"and passed;[ "" -lt 1 ]errored → conditional false → floor never asserted, exit 0jq --version 2>&1 || truefolded stderr in and dropped the exit status, so a glibc error containing2.34passed the floor — a regression; round 1 failed closed here[[:space:]]matches newlines, so the "anchor" still scanned the whole output (jq\n2.34: cannot load→ 2.34); and an over-long digit run resurrected round 1's exact mechanism{1,9}alone didn't bound anything — unanchored at the end, an over-long minor just matched its first 9 digits and compared that| 5 | (predicate clean) — but two of round 4's changes were unpinned: reverting the first-line slice and
[[:blank:]]together left the suite green, and the CR test was vacuous twice over | The 4 filler cases were all killed by the separator alone; andsplitlines()splits on\rwhilesubprocess(text=True)rewrites\r→\n, so the CR assertion was unfalsifiable |Round 5 could not break the predicate: 28,930 real runs across 14,465 crafted
--versionstrings on bash 3.2.57 and 5.3.15, zero fail-opens. It also settled the highest-risk portability question —{1,9}interval quantifiers are honoured inside[[ =~ ]]on bash 3.2, so round 4's bound is not void on the authoring Macs.Round 4's fix: first line only,
[[:blank:]]not[[:space:]], bounded digit runs with a trailing non-digit boundary, and a separator restricted to the two forms real jq emits (jq-1.6/jq version 1.6).Verified against a 20-case matrix — every legitimate form parses to the right numbers (
jq-1.6,jq version 1.6,jq-1.7.1,jq-1.6-dirty,jq-1.10compared 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 reports cleanly end-to-end.None of these are reachable from a real jq build. They are fixed anyway because the guard's own stated invariant — never assert a floor against something it did not parse — was still violable four revisions in, and the follow-up PR moves this code into the branch-protection-required check, where "unreachable in practice" stops being a good enough reason.
Deviation from a Done-when box, stated rather than glossed: the box asked to prove loudness by temporarily asserting a version the runner lacks and watching the job go red. I did it as a unit test instead (
test_jq_preflight.pyshimsjqon PATH), which is strictly better — it re-runs on every PR rather than living in one CI run's history — but it is not literally what the box says. The floor/pin logic is proven; what a real CI run still adds is confirming the version line actually appears in the job log, which is observable on this PR's own run.A vacuous test was found and fixed during development:
test_missing_jq_is_loudpassed whilePATHstill exposed the Mac's real/usr/bin/jq, so the negative case was never negative. PATH is now narrowed to the shim dir alone.Transition window is safe: the hook resolves the script relative to its own location and runs from the shared checkout, which will not have it until this merges. Verified by hiding the script and re-running: the exemption is withheld (fail-closed) and the gate falls through to a prompt. Costs an extra prompt, never a silent pass.
Docs
docs/ci-cd.md→ "The jq contract"; new recordsci.jq-version-contractandci.shared-pr-file-enumeration;ci.script-tests-jobnow points at the contract instead of restating it. Both records carry a landing note about the two-PR split, removed by the follow-up.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, `jq<TAB><TAB>9.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.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<CR> (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: yesReview-verdict: MERGEABLE @
b255b7fFive adversarial review rounds on the version predicate; rounds 2-4 each broke the previous fix, round 5 could not break it (28,930 runs, bash 3.2 + 5.3) and its two test-quality gaps are closed and mutation-verified. Runner log confirms jq-1.6 and the observable version line. 292 script tests; 16 guards individually mutation-tested.