From b255b7ffdcc433f29c4655538aac897886efcfd6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 22:20:33 +0200 Subject: [PATCH] =?UTF-8?q?test(648):=20close=20the=20mutation=20gaps=20ro?= =?UTF-8?q?und=205=20found=20=E2=80=94=20two=20tests=20passed=20for=20the?= =?UTF-8?q?=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."""