test: a conftest cannot switch a vacuity rule off by rebinding the layer's own names (#924)

CHANGELOG.md

@@ -425,6 +425,45 @@ true until the next version shipped.
 
 ### Fixed
 
+- A conftest can no longer switch a vacuity rule off by rebinding a name the
+  layer reads (#924).
+
+  pytest imports `conftest.py` from the directory it is policing, into the
+  policing interpreter, before collection. Every module-level name in
+  `pgc_vacuity.py` is therefore writable by the code it judges.
+
+  #958 closed the datum one exploit used. The three scans that read such data
+  are module-level names one frame further out, and each was a two-line conftest
+  away from being a no-op. Measured with the pinned runner:
+
+  ```
+  GUARD               no conftest    with the scan rebound to a no-op
+  order collapse      REFUSED rc=4   PASSED rc=0
+  broad except        REFUSED rc=4   PASSED rc=0
+  raises not pinned   REFUSED rc=4   PASSED rc=0
+  ```
+
+  Plugging a fourth name would reopen this again: the transitive closure from
+  the eight hooks is 31 of the module's 47 names, `ast` among them. So the layer
+  snapshots its own bindings at import, holds the snapshot in a closure, and
+  refuses a run in which any of them changed. Names added later are covered
+  without being listed anywhere.
+
+  No allowlist is needed: the module contains no `global` statement, so every
+  module-level binding is constant after import.
+
+  The bindings are restored before the refusal is raised. `pytester` runs its
+  inner session in-process on the same module object, so without that an inner
+  conftest's rebind stays made for every test that follows.
+
+  This is a cost guard, not a lock. Reaching into the hook's `__defaults__` still
+  reaches the closure. The criterion is that silencing a rule must cost more than
+  stating a reason, which `expect.cannot_run(REASON, detail)` does.
+
+  The shell harness needs no equivalent, and not because bash is simpler: its
+  policing runs in a different process from the code it polices. `selftest/260`
+  reads `lib.sh` with grep and awk and never sources the file it judges.
+
 - The sentinel sweep no longer excludes an assertion by accident of naming (#938).
 
   `_comparisons()` selected on the first two parameter names, so `wrote(cur, want,

test/pytest/TESTS.md

@@ -261,6 +261,32 @@ the general case was hand-rolled.
 | `test_the_inequality_scan_finds_a_planted_offence` | the AST scan fires on both spellings |
 | `test_the_inequality_scan_does_not_flag_honest_code` | five shapes it must not flag |
 | `test_no_test_in_this_corpus_hand_rolls_an_inequality` | the population is zero, across 17 files |
+| `test_a_conftest_cannot_switch_off_the_order_collapse_scan` | #924, the route still open after #958 |
+| `test_a_conftest_cannot_switch_off_the_broad_except_scan` | the same hatch, a second scan |
+| `test_a_conftest_cannot_switch_off_the_raises_scan` | and a third |
+| `test_the_refusal_names_the_binding_that_changed` | an honest run, refused, naming the conftest's name |
+| `test_a_new_attribute_on_the_layer_is_not_a_rebind` | the control: a check that fires on anything is not a check |
+| `test_the_rebind_does_not_leak_into_this_session` | the binding is restored, so `pytester` does not poison the outer run |
+
+**A conftest is loaded from the directory being policed, into the policing
+interpreter, before collection.** So every module-level name in `pgc_vacuity` is
+writable by the code it judges. #958 closed the datum one exploit used; the three
+scans that read such data are module-level names one frame further out, and each was
+a two-line conftest away from being a no-op:
+
+```
+GUARD               no conftest    with `pgc_vacuity.<scan> = lambda p: []`
+order collapse      REFUSED rc=4   PASSED rc=0
+broad except        REFUSED rc=4   PASSED rc=0
+raises not pinned   REFUSED rc=4   PASSED rc=0
+```
+
+The layer now snapshots its own bindings at import and refuses a run in which any of
+them changed, which covers names added after it was written. It is a cost guard, not
+a lock: the criterion #924 set is that silencing a rule must cost more than stating a
+reason. **The shell harness needs no equivalent** — `selftest/260` greps `lib.sh` from
+a separate process and never sources the file it judges.
+
 
 **Two failed queries are not two observable arms.** `query_error()` produces a value
 unique per occurrence precisely so two failures cannot compare **equal** and pass an

test/pytest/pgc_vacuity.py

@@ -1622,12 +1622,102 @@ def _raises_sites(path):
     return out
 
 
-def pytest_collection_modifyitems(config, items):
+# THE LAYER'S OWN BINDINGS, AND WHY THEY ARE CHECKED RATHER THAN HIDDEN (#924).
+#
+# pytest imports `conftest.py` FROM THE DIRECTORY IT IS POLICING into the policing
+# interpreter, before collection, with no opt-out. Every module-level name here is
+# therefore writable by the code this layer judges. That is what conftest is for; it
+# is a defect here only because this layer's job is to refuse, and a refusal that can
+# be deleted in two lines is a suggestion.
+#
+# THE SHELL HARNESS HAS NO EQUIVALENT, and not because bash is simpler: its policing
+# runs in a DIFFERENT PROCESS from the code it polices. `selftest/260` reads
+# `$TESTDIR/lib.sh` with grep and awk and never sources the file it judges, so
+# nothing a policed suite writes can reach the judge's namespace.
+#
+# #958 closed the datum one exploit used, by binding the killer list in a default
+# argument. The three scans that READ such data are module-level names themselves,
+# one frame further out. Measured on main 226f805 with the pinned runner, each was a
+# two-line conftest away from being a no-op:
+#
+#     GUARD               no conftest    with `pgc_vacuity.<scan> = lambda p: []`
+#     order collapse      REFUSED rc=4   PASSED rc=0
+#     broad except        REFUSED rc=4   PASSED rc=0
+#     raises not pinned   REFUSED rc=4   PASSED rc=0
+#
+# SO THE FIX IS NOT A FOURTH NAME MOVED OUT OF REACH. Plugging names one at a time is
+# what reopened #924 after #958, and the transitive closure from the eight hooks is
+# 31 of this module's 47 names -- `ast` among them. The layer notices instead that one
+# of its own bindings CHANGED, which covers the names added after this was written.
+#
+# THE SNAPSHOT IS EXACT WITH NO ALLOWLIST because this module contains no `global`
+# statement anywhere: after import, every module-level binding here is constant.
+# Verified rather than assumed, and it is the property that makes a bare identity
+# comparison correct.
+#
+# A NEW ATTRIBUTE IS NOT A REBIND. `pgc_vacuity._something_new = 1` changes no
+# behaviour, and refusing it would make this a tripwire on the mere act of importing
+# the module -- the false-positive engine this layer's own budget forbids. The arm
+# for that is a control in test_layer.py.
+#
+# WHAT IT STILL DOES NOT STOP, stated because a guard's blind spots are part of its
+# meaning. Anything sharing the interpreter can eventually win: reaching into
+# `pytest_collection_modifyitems.__defaults__` reaches the closure below. The
+# criterion #924 set is COST, not impossibility -- the hatch must cost more than
+# stating a reason, and that shape is unmistakably deliberate where two lines of
+# assignment are not. It also checks at COLLECTION only, so a test that rebinds a
+# name inside its own body and restores it is untouched; two arms in
+# test_failed_query_sentinel.py do exactly that, legitimately.
+def _binding_guard():
+    snapshot = {}
+    missing = object()
+
+    def arm(ns):
+        snapshot.update({k: v for k, v in ns.items() if not k.startswith("__")})
+
+    def changed(ns):
+        return sorted(k for k, v in snapshot.items() if ns.get(k, missing) is not v)
+
+    # RESTORED BEFORE THE REFUSAL, not after it, and this is load-bearing rather than
+    # tidiness. `pytester` runs its inner session IN-PROCESS on this same module
+    # object, so a rebind made by an inner conftest stays made for every test that
+    # follows in the outer run -- the hazard `_RunShape` already paid for once. The
+    # arms that prove this refusal would otherwise poison the rest of their own file.
+    def restore(ns):
+        ns.update(snapshot)
+
+    return arm, changed, restore
+
+
+_arm_bindings, _changed_bindings, _restore_bindings = _binding_guard()
+
+
+def pytest_collection_modifyitems(config, items,
+                                  _changed=_changed_bindings,
+                                  _restore=_restore_bindings):
     """Refuse a bare skip, which exits 0 and reads as success.
 
     Measured: two skipped tests report `2 skipped` and exit 0. A skip is allowed
     only through expect.cannot_run(), which names a reason from a closed list.
     """
+    # FIRST, because every rule below is read through a name a conftest can write.
+    # Captured in this signature at DEFINITION time, so rebinding `_changed_bindings`
+    # or `_restore_bindings` on the module does not reach what runs here.
+    _rebound = _changed(globals())
+    if _rebound:
+        _restore(globals())
+        raise pytest.UsageError(
+            "the pgColumnar vacuity layer refuses this run: a conftest or plugin "
+            "rebound the layer's own "
+            + ("names " if len(_rebound) > 1 else "name ")
+            + ", ".join(_rebound)
+            + " -- a rule this layer enforces is read through that binding, so the "
+            "run would have reported on rules that were switched off. The bindings "
+            "have been restored. If a check is wrong for your case, say so where the "
+            "run records it: expect.cannot_run(REASON, detail), which names a reason "
+            "from a closed list, or fix the test the rule is objecting to."
+        )
+
     offenders = []
     seen_files = set()
     for item in items:
@@ -1725,3 +1815,10 @@ def pytest_collection_modifyitems(config, items):
         raise pytest.UsageError(
             "the pgColumnar vacuity layer refuses this run: " + ". ".join(parts) + "."
         )
+
+
+# ARMED HERE, AT THE BOTTOM, because a snapshot taken earlier would miss every name
+# defined after it -- including this hook. Import order is what makes this safe: the
+# module is fully executed before pytest imports any conftest, so nothing the policed
+# tree writes can be in the snapshot.
+_arm_bindings(globals())

test/pytest/test_layer.py

@@ -540,3 +540,196 @@ def test_no_test_in_this_corpus_hand_rolls_an_inequality(expect):
     for f in files:
         offences += _hand_rolled_inequalities(f.read_text(encoding="utf-8"), f.name)
     expect.text(repr(offences), "[]", "no test hand-rolls an inequality")
+
+
+# ---- the layer's own names are reachable from the tree it polices (#924) ----
+#
+# pytest imports `conftest.py` FROM THE POLICED DIRECTORY into the policing
+# interpreter, before collection, with no opt-out. So every module-level name in
+# pgc_vacuity is writable by the code it judges. That is not a bug in pytest; it
+# is what conftest is for. It is a defect here because this layer's whole job is
+# to refuse, and a refusal that can be deleted in two lines is a suggestion.
+#
+# #958 closed the datum one exploit used (`_ORDER_KILLERS`) by binding it in a
+# default argument. The three scans that READ such data are module-level names
+# themselves, one frame further out, and each is a two-line conftest away from
+# being a no-op. Measured on main 226f805 with the pinned runner:
+#
+#     GUARD               no conftest    with the rebind
+#     order collapse      REFUSED rc=4   PASSED rc=0
+#     broad except        REFUSED rc=4   PASSED rc=0
+#     raises not pinned   REFUSED rc=4   PASSED rc=0
+#
+# The fix is not another name moved out of reach. It is the layer noticing that
+# one of its own bindings changed, which covers the names added after this was
+# written as well as the 47 present when it was.
+
+
+def _rebind(name):
+    """A conftest that switches one scan off, and nothing else. Two lines."""
+    return (
+        "import pgc_vacuity\n"
+        f"pgc_vacuity.{name} = lambda path: []\n"
+    )
+
+
+def test_a_conftest_cannot_switch_off_the_order_collapse_scan(pytester, expect):
+    """#924, the route still open after #958."""
+    pytester.makepyfile(
+        """
+        def test_order_collapsed(expect):
+            got = ["b", "a"]
+            g = sorted(got)
+            expect.ordered_rows(g, ["a", "b"], "rows in order")
+        """
+    )
+    plain = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(plain, "premise: the collapse is refused when nobody rebinds")
+    plain.stderr.fnmatch_lines(["*order-killed*"])
+
+    pytester.makeconftest(_rebind("_sorted_ordered_sites"))
+    hatched = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(hatched, "and it is still refused after the scan is rebound")
+    hatched.stderr.fnmatch_lines(["*_sorted_ordered_sites*"])
+
+
+def test_a_conftest_cannot_switch_off_the_broad_except_scan(pytester, expect):
+    """The same hatch, a different scan. Named separately because a fix that
+    closed only the scan #924 happens to name would leave this one open, which
+    is how #924 came back after #958."""
+    pytester.makepyfile(
+        """
+        def test_broad(expect):
+            try:
+                x = 1
+            except Exception:
+                pass
+            expect.num(x, 1, "x is one")
+        """
+    )
+    plain = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(plain, "premise: the broad except is refused")
+    plain.stderr.fnmatch_lines(["*catches Exception broadly*"])
+
+    pytester.makeconftest(_rebind("_broad_except_sites"))
+    hatched = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(hatched, "and it is still refused after the scan is rebound")
+    hatched.stderr.fnmatch_lines(["*_broad_except_sites*"])
+
+
+def test_a_conftest_cannot_switch_off_the_raises_scan(pytester, expect):
+    """The third. The body holds a compound statement rather than a broad
+    exception class, because that arm needs no driver installed and this file
+    runs in the database-free job.
+
+    My first version of this premise used `pytest.raises(ValueError)` around one
+    statement, which the rule does not refuse -- so the PREMISE printed `1 passed`
+    and the arm could not have shown anything being switched off.
+    """
+    pytester.makepyfile(
+        """
+        import pytest
+
+        def test_raises_compound(expect):
+            with pytest.raises(ValueError):
+                for i in [1]:
+                    raise ValueError("boom")
+            expect.num(1, 1, "ran")
+        """
+    )
+    plain = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(plain, "premise: an unpinned compound raises block is refused")
+    plain.stderr.fnmatch_lines(["*not pinned*"])
+
+    pytester.makeconftest(_rebind("_raises_sites"))
+    hatched = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(hatched, "and it is still refused after the scan is rebound")
+    hatched.stderr.fnmatch_lines(["*_raises_sites*"])
+
+
+def test_the_refusal_names_the_binding_that_changed(pytester, expect):
+    """A refusal that does not say what was rebound sends the reader to the
+    wrong file. The offending test here is HONEST -- the only thing wrong with
+    the run is the conftest -- so nothing but the tamper check can refuse it."""
+    pytester.makepyfile(
+        """
+        def test_honest(expect):
+            expect.num(1, 1, "one is one")
+        """
+    )
+    plain = pytester.runpytest("-p", "pgc_vacuity")
+    expect.outcomes(plain, "premise: an honest test passes", passed=1, failed=0)
+
+    pytester.makeconftest(_rebind("_broad_except_sites"))
+    hatched = pytester.runpytest("-p", "pgc_vacuity")
+    expect.run_failed(hatched, "a rebound layer name refuses a run of honest tests")
+    hatched.stderr.fnmatch_lines(["*_broad_except_sites*"])
+
+
+def test_a_new_attribute_on_the_layer_is_not_a_rebind(pytester, expect):
+    """The control, and it is LOAD-BEARING rather than hygiene (@OffgridwithJD).
+
+    A guard that fired on anything touching the module would be the
+    false-positive engine this layer's own budget forbids -- but it would also
+    turn four EXISTING tests red, because each writes a name that has never been
+    a module attribute:
+
+        _ORDER_KILLERS    test_ordered.py:148, :169     (removed by #958)
+        _BROAD_RAISES     test_raises_sqlstate.py:584
+        broad_families    test_raises_sqlstate.py:585
+        BROAD_RAISES      test_raises_sqlstate.py:586
+
+    A snapshot comparison cannot flag any of them, because there is no prior
+    binding to differ from. So this arm is what keeps an ADD from being tightened
+    into tampering by someone who reads the check as incomplete. Verified: only
+    `QUERY_ERROR` (test_failed_query_sentinel.py:321, :352) rebinds a name that
+    exists, and it restores it in a `finally` without driving an inner collection.
+    """
+    pytester.makepyfile(
+        """
+        def test_honest(expect):
+            expect.num(1, 1, "one is one")
+        """
+    )
+    pytester.makeconftest(
+        "import pgc_vacuity\n"
+        "pgc_vacuity._a_name_the_layer_never_had = 1\n"
+    )
+    result = pytester.runpytest("-p", "pgc_vacuity")
+    expect.outcomes(result, "a NEW attribute is not a changed binding",
+                    passed=1, failed=0)
+
+
+def test_the_rebind_does_not_leak_into_this_session(pytester, expect):
+    """pytester runs the inner session IN-PROCESS, on the same module object, so
+    a conftest that rebinds a scan leaves it rebound for every test that follows
+    -- including the ones in this file. The layer already paid for that once
+    (see `_RunShape`: 'AN INSTANCE PER CONFIG, NOT MODULE GLOBALS').
+
+    So the refusal restores the binding before raising, and this asserts it by
+    USING the scan afterwards rather than by comparing identities: a no-op lambda
+    returns [] for everything, and the real scan finds the planted offence.
+    """
+    pytester.makepyfile(
+        """
+        def test_honest(expect):
+            expect.num(1, 1, "one is one")
+        """
+    )
+    pytester.makeconftest(_rebind("_broad_except_sites"))
+    expect.run_failed(pytester.runpytest("-p", "pgc_vacuity"),
+                      "the inner run is refused")
+
+    import pgc_vacuity
+    planted = pytester.makepyfile(
+        planted="""
+        def test_planted(expect):
+            try:
+                x = 1
+            except Exception:
+                pass
+            expect.num(x, 1, "x is one")
+        """
+    )
+    expect.num(len(pgc_vacuity._broad_except_sites(str(planted))), 1,
+               "the real scan is back and still finds the offence")