From 2a2dcacd587cd60c48bd3fd9fe365dfda40c3c48 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 27 Jul 2026 00:26:09 +0200 Subject: [PATCH 1/2] test(649): cover the review-verdict status read, and the guards that only fire on the bot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Additive tests for properties #666 shipped correctly but left unguarded. No behaviour change. The stub's status read was hardcoded to "no verdict yet", so two whole branches of the classify step were unreachable from any test. Four mutations survived the full suite because of it — including re-introducing the literal ersatztv#647 fail-open, and overwriting an existing human verdict. The stub now models a transport error, a garbage body, and an existing verdict. `test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT` needs the bot author to test anything: with a non-bot, the blank line an empty list produces already fails DOCS_ONLY, so the `count -eq 0` guard never decides the outcome. On the bot path it is the ONLY thing between an enumeration that read nothing and an unattended success. Verified by mutation — `grep -c .` -> `grep -c ''` grants a bot PR success while every other test stays green. Same short-circuit shape as the PROTECTED/DOCS_ONLY disjointness this file already documents. Two anchors were also unguarded: `grep -qxF` (author `ova` is a substring of `renovate`) and DOCS_ONLY's `$` (`evil.mdx` reads as docs-only). Five of the six mutations are caught behaviourally. The sixth — dropping the shell emptiness check — cannot be caught locally: `jq -e` over empty input exits 4 on jq 1.8 so the guard still fires on a dev Mac, and 0 on the runner's 1.6 where it is the actual bug. A structural assertion closes that gap, with comments stripped first, since a raw substring search is satisfiable by moving the guard into a comment while deleting the real one — verified. refs #649, #672 --- scripts/tests/test_pr_changed_files.py | 117 ++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 2 deletions(-) diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 19d4590aa..b6b49c75f 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -567,7 +567,20 @@ if "-X" in args and args[args.index("-X") + 1] == "POST": sys.exit(0) if "/status" in url: - # No verdict yet for this head — the case where the job goes on to classify. + # Configurable. Hardcoding "no verdict yet" left the ersatztv#647 emptiness guard and the + # never-overwrite short-circuit unreachable: neither could be made to fire, so mutations + # deleting them survived the whole suite. + mode = os.environ.get("STUB_STATUS_MODE", "none") + if mode == "transport-error": + # Real `gh()` is `curl -sf`: an HTTP error exits 22 with EMPTY stdout. + sys.exit(22) + if mode == "garbage": + print("502 Bad Gateway") + sys.exit(0) + if mode.startswith("existing:"): + print(json.dumps({"statuses": [ + {"context": "review-verdict/h10", "status": mode.split(":", 1)[1]}]})) + sys.exit(0) print(json.dumps({"statuses": []})) sys.exit(0) @@ -575,7 +588,8 @@ print("{}") ''' -def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy"): +def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", + status_mode: str = "none"): """Execute the workflow's classify `run:` block with a stubbed enumeration script. Returns the status payload the job POSTed, or None if it posted nothing. @@ -591,6 +605,7 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy"): env = dict(os.environ) env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}" env["STUB_DIR"] = str(tmp_path) + env["STUB_STATUS_MODE"] = status_mode env.update({ "GITEA_TOKEN": "stub", "BASE_URL": "http://gitea.example/api/v1", @@ -708,3 +723,101 @@ def test_bot_positive_control_a_plain_bot_pr_IS_exempt(tmp_path): assert posted is not None assert posted["state"] == "success", ( "the bot exemption never fires at all, so the protected-path test above proves nothing") + + +# --- The STATUS READ: the ersatztv#647 guard and the never-overwrite short-circuit ------------- +# +# These were unreachable until the stub's status response became configurable. Four mutations +# survived the full suite without them, including re-introducing the literal ersatztv#647 fail-open. + +def test_a_transport_failure_on_the_STATUS_READ_posts_NOTHING(tmp_path): + """`gh()` is `curl -sf`, so an HTTP error yields exit 22 and EMPTY stdout. Reading that as "no + verdict exists" would let the job post over a real human verdict. It must fail WITHOUT posting. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="transport-error") + assert r.returncode != 0, "an unreadable status read must fail the job" + assert posted is None, "nothing may be posted when the existing verdict state is unknown" + + +def test_a_GARBAGE_status_response_posts_NOTHING(tmp_path): + """A proxy error page is a 200 with a non-JSON body — not an absent verdict.""" + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="garbage") + assert r.returncode != 0 + assert posted is None + + +@pytest.mark.parametrize("existing", ["success", "failure"]) +def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existing): + """A human verdict for this exact head may already exist — the reviewer ran + post-review-verdict.sh before this job finished, or the job re-ran. Re-posting would un-approve + a reviewed head, or (worse) approve one a human marked BLOCKED. + + `failure` is the sharp case: that is a human saying NO, and an exemption posted over it would + turn a rejection into a merge. + """ + posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode=f"existing:{existing}") + assert r.returncode == 0, r.stderr + assert posted is None, f"overwrote an existing '{existing}' verdict on this head" + + +# --- The `count -eq 0` guard, on the path where it is the ONLY guard --------------------------- + +def test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT(tmp_path): + """The author must be a BOT for this to test anything. + + With a non-bot author the blank line an empty list produces already fails `DOCS_ONLY`, so the + `[ "${count:-0}" -eq 0 ]` guard never decides the outcome — the same short-circuit that made an + earlier `PROTECTED` test vacuous. On the bot path that guard is the ONLY thing between an + unreadable-but-successful enumeration and an unattended `success`. + + Verified by mutation: changing `grep -c .` to `grep -c ''` (counting the blank line, so + `count=1`) grants a bot PR `success` here while every other test stays green. + """ + posted, r = _run_classify(tmp_path, "#!/usr/bin/env bash\nexit 0\n", author="renovate") + assert posted is not None, f"the job posted no status at all: {r.stderr}" + assert posted["state"] == "pending", ( + "an empty file list was treated as a bot exemption — the enumeration returning nothing is " + "not evidence that nothing was changed") + + +# --- Anchors in the classifier predicates ------------------------------------------------------ + +def test_the_BOT_match_is_whole_line_not_substring(tmp_path): + """`grep -qxF` is anchored; plain `grep -qF` would exempt any author whose name CONTAINS a bot + name. `ova` is a substring of `renovate`.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), author="ova") + assert posted is not None + assert posted["state"] == "pending", "a substring of a bot name was granted the bot exemption" + + +def test_DOCS_ONLY_anchors_the_markdown_extension(tmp_path): + r"""`[^/]*\.md$` must match only a top-level file ENDING in .md. Losing the `$` exempts + `evil.mdx`, and the docs-only exemption posts a green status with nobody in the loop.""" + posted, _ = _run_classify(tmp_path, _emitting("evil.mdx")) + assert posted is not None + assert posted["state"] == "pending", "a .mdx file was accepted as docs-only" + + +def test_the_status_read_checks_EMPTINESS_in_SHELL_before_invoking_jq(): + """Structural, and deliberately so — no behavioural test can catch this on a dev machine. + + Deleting the `[ -z "${statusjson//[[:space:]]/}" ]` half of the guard leaves the empty case to + `jq -e`'s exit status, which is 4 on jq >= 1.7 (guard still fires, and + `test_a_transport_failure_on_the_STATUS_READ_posts_NOTHING` still passes on a Mac with jq 1.8) + but **0 on jq 1.6, which is what the runner ships**. There the job sails past the guard and + posts over a possibly-existing human verdict. That is literally ersatztv#647, and the reason + `ci.jq-version-contract` exists. + + So the behavioural test catches this mutation only when the suite runs under 1.6 — in CI, not + locally. This closes the local gap by pinning the construct. + + COMMENTS ARE STRIPPED FIRST. A raw substring search over the step text is satisfiable by moving + the guard into a comment while deleting the real one — the same "a comment mentioning it is not + a call" vacuity this file already fixed for the enumeration drift guard. + """ + body = _classify_step()["run"] + code = "\n".join(ln for ln in body.splitlines() if not ln.lstrip().startswith("#")) + assert '[ -z "${statusjson//[[:space:]]/}" ]' in code, ( + "the status read must check emptiness in SHELL before jq; leaving it to `jq -e`'s exit " + "status is fail-open on the runner's jq 1.6 (ersatztv#647)") -- 2.47.3 From 7ed0a59c56aa44d907bcf3e15ec9d11bc50ac9ea Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 27 Jul 2026 08:01:36 +0200 Subject: [PATCH 2/2] =?UTF-8?q?test(649):=20cold-review=20fixes=20?= =?UTF-8?q?=E2=80=94=20the=20never-overwrite=20test=20skipped=20the=20case?= =?UTF-8?q?=20its=20docstring=20called=20sharpest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four gaps, all found by mutation rather than reading. The never-overwrite test used only a NON-EXEMPT file list, so "an exemption posted over a human BLOCKED verdict" — the scenario its own docstring named as the sharp one — was asserted nowhere. Moving the short-circuit to after classification, gated on non-exempt, survived the whole suite while turning a human rejection into a green required status for both a docs-only PR and a bot PR. Now parametrised over non-exempt, docs-only-exempt and bot-exempt file lists. The structural emptiness pin is REPLACED by a real jq-1.6 behavioural test. Its stated justification — "no behavioural test can catch this on a dev machine" — was simply false: this file already imports _JQ16_SHIM for pr-changed-files.sh, so the runner's quirk is reproducible here. The structural version was also weaker than it looked, stripping only FULL-LINE comments, so leaving the literal as a trailing comment on the surviving guard satisfied it while the real check was gone. The behavioural test catches that mutant and needs no comment-stripping. The status-read stub now returns DECOY contexts either side of the verdict row, so dropping `select(.context == $c)` is caught. First attempt gave the decoys `status: success`, which triggers the same short-circuit as a real verdict — the mutation still produced an identical outcome and survived. `pending` decoys make mis-selection observable. DOCS_ONLY's `^` anchor is now covered alongside its `$`: losing it exempts ErsatzTV/docs/Evil.cs, a C# file, and is fail-OPEN. Two remaining survivors are documented in-file as behaviourally equivalent, not gaps: `first` -> `last` (the combined endpoint returns one row per context by contract, so a two-row fixture would test a fiction), and the garbage-response test defending the type guard only by redundancy. --- scripts/tests/test_pr_changed_files.py | 81 ++++++++++++++++++-------- 1 file changed, 58 insertions(+), 23 deletions(-) diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index b6b49c75f..526e505e2 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -578,8 +578,15 @@ if "/status" in url: print("502 Bad Gateway") sys.exit(0) if mode.startswith("existing:"): + # DECOY contexts either side, because real heads carry several (a live head showed 7 rows, + # the first an unrelated CI context). Their status is deliberately `pending`, NOT `success`: + # a `success` decoy triggers the very same short-circuit as a real verdict, so dropping + # `select(.context == $c)` produced an identical outcome and the mutation survived. With + # `pending` decoys, mis-selecting means no short-circuit — the job posts, and the test sees it. print(json.dumps({"statuses": [ - {"context": "review-verdict/h10", "status": mode.split(":", 1)[1]}]})) + {"context": "Build & test (.NET)", "status": "pending"}, + {"context": "review-verdict/h10", "status": mode.split(":", 1)[1]}, + {"context": "Functional E2E", "status": "pending"}]})) sys.exit(0) print(json.dumps({"statuses": []})) sys.exit(0) @@ -589,13 +596,18 @@ print("{}") def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", - status_mode: str = "none"): + status_mode: str = "none", jq16: bool = False): """Execute the workflow's classify `run:` block with a stubbed enumeration script. Returns the status payload the job POSTed, or None if it posted nothing. """ bindir = tmp_path / "bin"; bindir.mkdir() curl = bindir / "curl"; curl.write_text(WORKFLOW_STUB_CURL); curl.chmod(0o755) + if jq16: + # Reproduce the runner's jq 1.6 (`-e` over EMPTY input exits 0, not 4). The shim is the one + # already used for pr-changed-files.sh in this file, so its fidelity is covered by that + # suite's own verify-the-verifier test. + jq = bindir / "jq"; jq.write_text(_JQ16_SHIM); jq.chmod(0o755) scripts = tmp_path / "scripts"; scripts.mkdir() if enum_stub is not None: enum = scripts / "pr-changed-files.sh" @@ -746,8 +758,22 @@ def test_a_GARBAGE_status_response_posts_NOTHING(tmp_path): assert posted is None +# Two mutations to this read are NOT covered, and both are behaviourally equivalent rather than gaps: +# * `first` -> `last`: `select(.context == $c)` yields exactly ONE row, because the job reads the +# COMBINED status endpoint, which returns latest-per-context by contract. A fixture with two +# `review-verdict/h10` rows would be testing something the API does not produce. +# * `test_a_GARBAGE_status_response_posts_NOTHING` does not, on its own, defend the type guard: +# with the guard gone, the following `jq -r` fails on non-JSON and `set -e` kills the job anyway. +# That is fail-closed by REDUNDANCY. The guard's own behaviour is pinned by the jq-1.6 test below, +# which is where it actually matters. + @pytest.mark.parametrize("existing", ["success", "failure"]) -def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existing): +@pytest.mark.parametrize("paths,author", [ + (("ErsatzTV/Program.cs",), "timothy"), # non-exempt: only the short-circuit can stop it + (("docs/a.md",), "timothy"), # docs-only EXEMPT + (("Directory.Packages.props",), "renovate"), # bot EXEMPT +]) +def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existing, paths, author): """A human verdict for this exact head may already exist — the reviewer ran post-review-verdict.sh before this job finished, or the job re-ran. Re-posting would un-approve a reviewed head, or (worse) approve one a human marked BLOCKED. @@ -755,10 +781,11 @@ def test_an_existing_verdict_on_this_head_is_NEVER_overwritten(tmp_path, existin `failure` is the sharp case: that is a human saying NO, and an exemption posted over it would turn a rejection into a merge. """ - posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + posted, r = _run_classify(tmp_path, _emitting(*paths), author=author, status_mode=f"existing:{existing}") assert r.returncode == 0, r.stderr - assert posted is None, f"overwrote an existing '{existing}' verdict on this head" + assert posted is None, ( + f"overwrote an existing '{existing}' verdict on this head with {paths} as {author}") # --- The `count -eq 0` guard, on the path where it is the ONLY guard --------------------------- @@ -799,25 +826,33 @@ def test_DOCS_ONLY_anchors_the_markdown_extension(tmp_path): assert posted["state"] == "pending", "a .mdx file was accepted as docs-only" -def test_the_status_read_checks_EMPTINESS_in_SHELL_before_invoking_jq(): - """Structural, and deliberately so — no behavioural test can catch this on a dev machine. +def test_a_transport_failure_under_jq_1_6_STILL_posts_nothing(tmp_path): + """The ersatztv#647 fail-open, tested where it actually lives: jq 1.6. - Deleting the `[ -z "${statusjson//[[:space:]]/}" ]` half of the guard leaves the empty case to - `jq -e`'s exit status, which is 4 on jq >= 1.7 (guard still fires, and - `test_a_transport_failure_on_the_STATUS_READ_posts_NOTHING` still passes on a Mac with jq 1.8) - but **0 on jq 1.6, which is what the runner ships**. There the job sails past the guard and - posts over a possibly-existing human verdict. That is literally ersatztv#647, and the reason - `ci.jq-version-contract` exists. + The shell emptiness check exists because `jq -e` over EMPTY input exits 4 on jq >= 1.7 but **0 on + jq 1.6, which is what the runner ships**. Remove that check and, on a dev Mac's 1.8, the guard + still fires and every test stays green — the bug is invisible locally. - So the behavioural test catches this mutation only when the suite runs under 1.6 — in CI, not - locally. This closes the local gap by pinning the construct. + An earlier version of this pinned the construct STRUCTURALLY instead, on the stated grounds that + "no behavioural test can catch this on a dev machine". That was wrong: this file already imports + `_JQ16_SHIM` for `pr-changed-files.sh`, so the runner's quirk is reproducible here. The structural + version was also weaker than it looked — it stripped only FULL-LINE comments, so leaving the + literal as a trailing comment on the surviving `if` satisfied it while the real guard was gone. - COMMENTS ARE STRIPPED FIRST. A raw substring search over the step text is satisfiable by moving - the guard into a comment while deleting the real one — the same "a comment mentioning it is not - a call" vacuity this file already fixed for the enumeration drift guard. + This test is strictly stronger: it catches that mutant, needs no comment-stripping, and fails for + the right reason. Verified by mutation under both jq versions. """ - body = _classify_step()["run"] - code = "\n".join(ln for ln in body.splitlines() if not ln.lstrip().startswith("#")) - assert '[ -z "${statusjson//[[:space:]]/}" ]' in code, ( - "the status read must check emptiness in SHELL before jq; leaving it to `jq -e`'s exit " - "status is fail-open on the runner's jq 1.6 (ersatztv#647)") + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), + status_mode="transport-error", jq16=True) + assert r.returncode != 0, ( + "under jq 1.6 an unreadable status read must still fail the job — this is the exact " + "ersatztv#647 fail-open") + assert posted is None, "nothing may be posted when the existing verdict state is unknown" + + +def test_DOCS_ONLY_anchors_the_START_of_the_path_too(tmp_path): + r"""`^(docs/|...)` must match only at the start. Losing the `^` exempts `ErsatzTV/docs/Evil.cs`, + which is a C# file — fail-OPEN, and the sibling of the `$` case above.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/docs/Evil.cs")) + assert posted is not None + assert posted["state"] == "pending", "a path merely CONTAINING docs/ was accepted as docs-only" -- 2.47.3