diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 19d4590aa..526e505e2 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -567,7 +567,27 @@ 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:"): + # 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": "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) @@ -575,13 +595,19 @@ 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", 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" @@ -591,6 +617,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 +735,124 @@ 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 + + +# 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"]) +@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. + + `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(*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 with {paths} as {author}") + + +# --- 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_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. + + 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. + + 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. + + 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. + """ + 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"