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."""