test(649): cold-review fixes — the never-overwrite test skipped the case its docstring called sharpest

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.
2026-07-27 08:01:36 +02:00
parent 2a2dcacd58
commit 7ed0a59c56
1 changed files with 58 additions and 23 deletions
@@ -578,8 +578,15 @@ if "/status" in url:
        print("<html>502 Bad Gateway</html>")
        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"