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