test: a conftest cannot rebind the order-killer list (#924)

[linuxhikerpm]

Fixes #924.

The order-collapse scan looked up _ORDER_KILLERS on each call. A conftest.py is imported before collection, so two lines switched the guard off for every test in that directory, with no reason recorded:

import pgc_vacuity
pgc_vacuity._ORDER_KILLERS = ()

That is less to type than the honest form, which is the hatch the layer's own false-positive budget forbids. Reproduced on this tree: the same collapse test is uncollectable with no extra file, and reports 1 passed with only that rebind. After the fix both runs are refused.

The killer names are bound at definition time, in a default argument, the same way query_error already binds its prefix. There is no module-level name left to rebind. _RECORDERS is a registry the layer writes, not rule data, and is out of scope here.

Red first: the inner run exited 0, so nothing refused it. Putting _ORDER_KILLERS back as the name the scan reads reddens the same arm for the same reason.

No shell twin. The subject is the scan in pgc_vacuity.py. A shell part that greps or inspects that module is the coupling selftest 350 and 360 deleted; the behaviour is pinned where it can be observed, in test_ordered.py.

No ledger row and no census move.

Residual

_sorted_ordered_sites is still a module-level name the collection hook looks up at call time, so pgc_vacuity._sorted_ordered_sites = lambda path: [] would still silence the scan. That replaces the scanner rather than its data, and is a different hatch. Not this PR.

Do not merge from this PR until reviewed.

Made with Cursor

[OffgridwithJD]

Reviewed at 69c28989. The fix works and I reproduced both halves. One thing to change before merge, and it is about Fixes #924 rather than about the code.

This closes an issue I filed, so my only interest is in it being right.

Verified, with the premise asserted

A subdirectory of the real corpus, so the parent conftest supplies expect and the hooks — the #924 scenario exactly:

arm                                    main           #958
no conftest (premise)                  VACUITY        VACUITY
conftest: _ORDER_KILLERS = ()          1 passed       VACUITY     <- closed
conftest: _sorted_ordered_sites = ...  --             1 passed    <- the residual

The middle row is the fix and it is real: the same two lines that silenced the guard on main are refused here. My first rig got fixture 'expect' not found on every arm, including the premise — a broken instrument, not your code — and I rebuilt it inside the corpus before believing anything.

The residual you disclosed is exploitable at exactly the same price

You named it precisely and I am not adding anything you did not already say, except a number:

conftest to silence via _ORDER_KILLERS        2 lines   (now refused)
conftest to silence via _sorted_ordered_sites 2 lines   (still works)

#924's own criterion was the cost of the hatch — "Two lines of conftest.py is less to type than the honest form, which is the hatch the layer's own false-positive budget forbids" — and it also said the point was the shape rather than the count: "Any rule whose data lives in a module-level name is switchable from a conftest."

Your change protects the data. The scanner is still a module-level name the hook looks up at call time, at pgc_vacuity.py:1647. So the hatch costs the same two lines, which means Fixes #924 would close an issue whose stated criterion is not yet met.

It closes in one line, with your own technique, and I verified that rather than asking

Bind the scanner in the hook's signature the way you bound the tuple in the factory's:

def pytest_collection_modifyitems(config, items, _sites=_sorted_ordered_sites):
    ...
    offenders.extend(_sites(f))

Measured on a copy of your branch:

premise, no conftest:            VACUITY
_ORDER_KILLERS rebind:           VACUITY
_sorted_ordered_sites rebind:    VACUITY     <- now closed too
full corpus:                     313 passed  <- no breakage

A verified negative that bounds the class, and is worth recording either way

Rebinding the hook itself does not work:

conftest: pgc_vacuity.pytest_collection_modifyitems = lambda *a, **k: None
  -> VACUITY

pytest collects the hook when it registers the plugin, so a later rebind of the module attribute does not reach the registered callable. That avenue was never open, which is useful: it means the exposed surface is the names the hook body looks up at call time, not the hook, and that surface is enumerable rather than endless.

The structural note, for whoever takes the rest

Def-time binding is the right local move and it is still whack-a-mole in principle — Python always allows monkeypatching, so a determined conftest.py can reach anything. The class does not close by hiding names; it closes by noticing: snapshot the rule data at import and refuse when the live value differs, so switching a rule off appears in the run's output rather than in nobody's. That is the third option #924 listed and it is the only one that makes a deliberate override cost what a hatch should cost.

Not this PR either way.

What I am asking for

Either take the one-line bind above — cheap, your technique, verified green — or change Fixes #924 to a plain reference so the issue stays open with the residual recorded. I would prefer the first, but the second is perfectly honest and I will not argue for scope.

Gate

CHANGELOG and TESTS.md both present. CI is 11 of 13 with the two suites jobs running; I will post a formal approve once all 13 report, and I will re-check the head first. The code is right as it stands.

[OffgridwithJD]

Approving the code at 69c28989 on full green CI (13 of 13), the head I reviewed and reproduced against.

The code is right. Verified with the premise asserted, in a subdirectory of the real corpus so the parent conftest supplies the fixtures:

arm                                    main        #958
no conftest (premise)                  VACUITY     VACUITY
conftest: _ORDER_KILLERS = ()          1 passed    VACUITY    <- closed

Binding the tuple in the factory's default argument is the same shape the layer already used for query_error, so it is consistent rather than novel, and it leaves no module-level name holding that rule's data.

One thing for whoever merges, which my approval does not settle

Fixes #924 will auto-close an issue whose own criterion is not yet met. The residual @linuxhikerpm disclosed is exploitable at the same price:

silence via _ORDER_KILLERS         2 lines   now refused
silence via _sorted_ordered_sites  2 lines   still works

#924 asked about the cost of the hatch — "two lines is less to type than the honest form" — and said the shape mattered more than the count. The data is protected; the scanner is still a module-level name the hook looks up at pgc_vacuity.py:1647.

So please either take the one-line bind I verified on a copy of this branch —

def pytest_collection_modifyitems(config, items, _sites=_sorted_ordered_sites):
    ...
    offenders.extend(_sites(f))

_sorted_ordered_sites rebind -> VACUITY      (closed)
full corpus                  -> 313 passed   (no breakage)

— or change Fixes #924 to a plain reference, so the issue stays open carrying the residual. I am not blocking on which, and I would rather this land than argue scope; but the one outcome to avoid is #924 closing while a two-line conftest still switches the guard off.

A verified negative that bounds what is left: rebinding the hook itself does not work, because pytest collects it when it registers the plugin. So the exposed surface is the names the hook body looks up at call time — enumerable, not endless.

And the structural note, for whoever takes the remainder rather than for this PR: def-time binding is the right local move and whack-a-mole in principle, since Python always allows monkeypatching. The class closes by noticing — snapshot the rule data at import and refuse when the live value differs — which is the third option #924 listed and the only one that makes a deliberate override cost what a hatch should cost.

[OffgridwithJD]

The head moved 69c28989 -> 5c6d0325 for the CHANGELOG union, and this repo does not dismiss stale reviews, so my APPROVED carried to a head I had not reviewed. I have verified it rather than leaving that assumed.

what #958 adds on top of main 3bfa175d:
  CHANGELOG.md                 13 ++++
  test/pytest/TESTS.md          1 +
  test/pytest/pgc_vacuity.py  171 ++++++----
  test/pytest/test_ordered.py  37 ++++
derived files among them: NONE

the files I approved:
  pgc_vacuity.py   99598ad6fd -> 99598ad6fd   IDENTICAL
  test_ordered.py  bcf0774937 -> bcf0774937   IDENTICAL

Gated on the resolved head: docs_style 0 FAIL, corpus 317 passed, driver-free 219 passed.

So the approval stands on its merits, not by inheritance — the code is the same bytes and the resolved tree is green.

The one thing still outstanding, and it is not the code

You did not take the one-line bind, which is exactly what I said I would not block on. But Fixes #924 is still in the body (1 occurrence), so merging will auto-close an issue whose own criterion is unmet:

silence via _ORDER_KILLERS         2 lines   refused after this PR
silence via _sorted_ordered_sites  2 lines   still works
the one-line remedy                verified: that rebind -> VACUITY, corpus 313 green

@jdatcmd has said they will merge as authorised and then reopen #924 with the reason, which I think is a better repair than editing your body or letting it close quietly: the merge is what jd approved, the auto-close is a keyword side effect, and a reopened issue carrying the measurement is visible where a closed one is not. I am content with that and not asking for anything further here.

For the reopen note, the negative is what bounds the remaining work: rebinding the hook itself does not work, because pytest collects it at plugin registration. So the exposed surface is the names the hook body looks up at call time — a list, not an open question.

[OffgridwithJD]

Head moved again for the rebase onto e1c86742 (#957 landed). Re-verified; my approval still stands on evidence rather than inheritance.

what #958 adds on top of main e1c86742:   4 files, NO derived file
pgc_vacuity.py   99598ad6fd -> 99598ad6fd   IDENTICAL to what I approved
test_ordered.py  bcf0774937 -> bcf0774937   IDENTICAL

The residual, re-measured at THIS head so the number is current

You did not take the one-line bind — _sites=_sorted_ordered_sites occurrences in the file: 0 — which is exactly what I said I would not block on. Measured at 87b238cd:

premise, no conftest:          VACUITY
_ORDER_KILLERS rebind:         VACUITY     <- this PR closed it
_sorted_ordered_sites rebind:  1 passed    <- still open, same 2 lines

@jdatcmd, for the reopen note: that is the head-accurate measurement, taken on the tree that will land rather than on the one I first reviewed. Fixes #924 is still present (1 occurrence), so the auto-close will happen and the reopen is the repair.

Nothing further from me. Good PR — the part I would single out is that you disclosed the residual yourself, precisely, in the body. I measured it because it was worth a number, not because it was hidden.