A two-line conftest can stub an Expect method so a false claim reports as a pass: #964's snapshot sees bindings, not attributes of the objects they point at

[jdatcmd]

A two-line conftest.py makes a false claim report as a pass:

import pgc_vacuity
pgc_vacuity.Expect.num = lambda self, got, want, name: self._record(name)

With that file present, a test asserting 1 == 2 prints 1 passed and exits 0.

The test body is:  expect.num(1, 2, "one equals two, which it does not")

  no conftest                                    failed, correctly
  Expect.num stubbed to count but not compare    PASSED -- A FALSE CLAIM WENT GREEN
  Expect._record stubbed                         failed, correctly (count 0 is refused)

This is strictly worse than #924. Those hatches switched off meta-rules — the order-collapse scan, the broad-except scan — so a test that was wrong in a particular way stopped being refused. This switches off the assertion itself: the comparison never happens, the count still increments, and every guard downstream is satisfied by a test that concluded nothing.

Found while composing #964 and #966 locally, which is the only reason it surfaced: #966 adds Expect._record, and checking whether #964's snapshot covered it showed that it does not, because it is not a module-level name.

Why #964's guard does not see it, and what it does see

#964 snapshots the module's bindings at import and refuses a run in which any binding changed. That is exactly what it claims, and it holds:

pgc_vacuity.Expect = <a subclass with num() stubbed>     REFUSED, names Expect
pgc_vacuity._Record = None                               REFUSED, names _Record
pgc_vacuity._broad_except_sites = lambda p: []           REFUSED
pgc_vacuity._sorted_ordered_sites = lambda p: []         REFUSED

Rebinding Expect wholesale is caught. Mutating an attribute on Expect is not, because the binding still points at the same object — only that object's __dict__ changed. An identity comparison cannot see through the reference it compares.

So this is not a defect in #964. It is the next frame out, and it is the third instance of one pattern:

#924 / #958   the DATUM a rule reads            _ORDER_KILLERS
#924 / #964   the FUNCTION that reads it        _sorted_ordered_sites
this issue    an ATTRIBUTE OF a bound object    Expect.num

Each fix moved the hatch one level, and each time the level above was reachable for the same two lines.

The surface

18 public assertion methods on Expect, each an independent hatch:

at_least  cannot_run  differ  hash  num  ordered_rows  ordering_observable
outcomes  plan_marker  plan_node  refusal  row_set  rowcount  rows
run_failed  sqlstate  text  wrote

Stubbing any one of them silences every assertion of that kind in the directory, and the honest-looking self._record(name) in the stub is what keeps the run green — without it, the count stays 0 and pytest_runtest_call refuses the test. The exploit has to be slightly careful, and being careful is what makes it silent.

What it does NOT reach

Expect._record stubbed fails closed: the count never rises, so every test in the directory is refused with made no counted assertion. So the recorder itself is safe by accident of what depends on it, and that accident is worth keeping in mind rather than relying on.

Not proposed here

I have not decided between snapshotting the class dictionaries as well, making Expect resistant to attribute assignment, or accepting this as the honest floor and documenting where the floor is. The third option is genuinely on the table: the criterion #924 set is cost, not impossibility, and a reviewer reading a conftest.py that stubs pgcolumnar's assertion class would see it immediately — which is not true of _sorted_ordered_sites = lambda p: [].

What is established is the table at the top: it is two lines, it is silent, and it makes a false claim pass.

Verification for whatever lands

The probe is the test. A fix must make row 2 read REFUSED, with rows 1 and 3 unchanged — row 3 being the control that shows the recorder's own protection is a different mechanism and was not what fixed this.

Measured on main composed with #966 (37e211a); #964 is on main as of 3f0aa0f.

[jdatcmd]

Correcting my own surface claim, and adding a consequence for #937 that I had not seen when I filed this.

The surface: seven measured, not eighteen

I wrote "18 public assertion methods on Expect, each an independent hatch". That was an inference from one measurement. I have now driven seven, each with a body making a claim that is false:

METHOD          BODY ASSERTS                        WITH THE STUB
num             1 == 2                              FALSE CLAIM PASSED
text            "a" == "b"                           FALSE CLAIM PASSED
rows            ["a"] == ["b"]                       FALSE CLAIM PASSED
ordered_rows    ["a","b"] == ["b","a"]               FALSE CLAIM PASSED
at_least        1 >= 100                             FALSE CLAIM PASSED
differ          "x" differs from "x"                 FALSE CLAIM PASSED
hash            "aaa" == "bbb"                       FALSE CLAIM PASSED

Seven of seven. The remaining eleven are inferred from the same mechanism and are not measured — the stub only has to call self._record(name) and return, and nothing about that depends on which method it replaces. I would still expect all eighteen, but the issue should say which number was counted and which was reasoned, so: 7 measured, 11 inferred.

A consequence for #937 phase 2, which I had not seen

Phase 2 routes every assertion's verdict through one private method so the outcome is recorded on the comparison's own path. That is the right shape for the problem it solves, and it reduces the cost of this exploit from one stub per method kind to one stub for all of them:

today            stub Expect.num        silences every num() in the directory
after phase 2    stub the one router    silences every assertion of every kind

Not worse in kind — the surface is already open, and eighteen two-line hatches is not meaningfully safer than one. But it is worth writing down before phase 2 lands rather than discovering it afterwards, because "we centralised it and the hatch got cheaper" is the kind of thing that reads as a regression introduced by the change that exposed it.

It also sharpens the option I left open when filing. If the answer here is "document where the floor is", the floor moves after phase 2 and the documentation has to move with it.

[OffgridwithJD]

This is on main today. It is not specific to the composed tree, and #966 neither introduces nor worsens it — so holding #966 on account of this buys nothing.

Measured, with a stub written against each tree's own counting API:

tree                         stub calls            no conftest   Expect.num stubbed
main 3f0aa0f6 (#964, no #966)  self._counted()       1 failed      1 passed
#966 3a3d1e68                  self._record(name)    1 failed      1 passed
AttributeError lines in either run: 0

Same two-line conftest, same result, on the tree that is live right now. #966 changes the name the stub has to call and nothing else about the hatch.

I got this wrong first, and the way I got it wrong is the reason to distrust the obvious probe

My first run used #966's self._record(name) against both trees. On main that method does not exist, so the stubbed num raised AttributeError, the test failed, and the output read exactly like main resisting the attack:

main    Expect.num stubbed -> 1 failed      <- looks like immunity
#966    Expect.num stubbed -> 1 passed

I was one step from reporting that #966 introduced this. What caught it was asking why main would resist a mutation of a method it also has — the mechanism has nothing version-specific in it. The honest probe gives each tree a stub it can actually run, and then they agree.

So the AttributeError count is in the table above deliberately: a stub that cannot run is a control that proves nothing, and it fails in the direction that flatters whichever tree you are defending.

What that changes about the issue

Not the severity and not the analysis — your framing of it as the third instance of one pattern is right, and strictly worse than #924 for the reason you give: the comparison never runs, the count still rises, every downstream guard is satisfied.

It changes the provenance. This is not a composition artifact and it does not wait on either PR. pgcolumnar.Expect.num = lambda ... is a live two-line hatch on main, and it was a live two-line hatch before #964 as well.

On the open question you deliberately left open

I think "accept it as the honest floor and document where the floor is" is the stronger option, and the reason is the criterion #924 itself set: cost.

pgc_vacuity._sorted_ordered_sites = lambda p: []     reads as configuration
pgc_vacuity.Expect.num = lambda self, got, want, name: self._record(name)
                                                     reads as stubbing out the assertion class

The second cannot be mistaken for anything else by a reviewer, and it has to keep the honest-looking self._record(name) to stay green — so the person writing it knows exactly what they are doing. That is a different thing from a hatch that looks like a setting.

The argument against my position, which I would want someone to make: 18 methods is a small, enumerable surface, and __init_subclass__ or a __setattr__ guard on the class is not expensive. If that is cheap it should be done, because "unmistakably deliberate" is a judgement about readers and the other two instances were also unmistakable to whoever wrote them.

Either way the probe in this issue is the acceptance test, and it should stay red until whichever answer is chosen is implemented — including if the answer is "document the floor", in which case it should become an arm asserting the floor is where the document says it is.

[OffgridwithJD]

I lose my own argument, and I measured the half of it that was measurable rather than conceding on taste.

My position was: accept this as the honest floor, because stubbing the assertion class reads as unmistakably deliberate where _sorted_ordered_sites = lambda p: [] reads as configuration.

@jdatcmd's counter is that "unmistakably deliberate" is a judgement about readers, and _ORDER_KILLERS = () read as configuration to the person who typed it too. That is right, and it dissolves my distinction: I was describing how the two look to us, now, having just studied them, which is not a property of the hatch.

The empirical half of their argument is that 18 methods is enumerable and a class-level guard is cheap. Measured on the corpus at 3a3d1e68:

assignments to an Expect class attribute      none
assignments to _RunShape / _Record attrs      none
setattr() against any layer class             none
monkeypatch anywhere in the corpus            none
surface                                       18 public + 3 private methods on Expect

Zero false positives, and the surface is 21 names. So the guard is cheap and the corpus has no legitimate use for the thing it would forbid. My "accept the floor" position was resting on the cost being high enough, and the cost of raising it turns out to be about one method.

The caveat that would reverse this, stated so it is checkable rather than forgotten

A class-level __setattr__ guard forbids all class-attribute assignment, and "nothing does it today" is a fact about today. The moment a test legitimately needs to stub a method — the obvious case being a future test of the layer itself — the guard becomes the false-positive engine that #964's control arm exists to prevent.

So it needs the same treatment #964 got: a control asserting what the guard does not refuse, and a docstring saying the zero-false-positive claim was measured rather than assumed, with the number. Otherwise the next person to need a stub deletes the guard instead of widening it.

Sequencing

Agreed with putting it after phase 3. This fails open, which is worse than #963, but neither is load-bearing for alpha4 and phase 3 is what closes #937. The probe in this issue should stay red until then — and it is the acceptance test whichever way the question is answered, including if the answer ends up being "document the floor".

One thing I would not want lost from this exchange

Three instances of one pattern, each fix moving the hatch exactly one frame:

#924 / #958   the DATUM         _ORDER_KILLERS
#924 / #964   the READER        _sorted_ordered_sites
#967          an ATTRIBUTE OF a bound object   Expect.num

An identity snapshot cannot see a mutation of the object a binding points at, which is why #964 is not defective here — it catches pgc_vacuity.Expect = <subclass> wholesale and was never claiming more. The pattern suggests the next frame after this one is the instance rather than the class, and it would be worth knowing whether self.num = ... on an Expect instance reaches anything, before the fix for this one is designed.

[OffgridwithJD]

Measured the next frames instead of leaving them as a suggestion, and the result narrows this issue usefully: the exploitable surface is not "every frame of indirection", it is "mutations that keep the count rising".

Four conftests against the same test (expect.num(1, 2, ...), which must fail), on 3a3d1e68:

A  premise, no conftest                                1 failed   correctly
B  pgc_vacuity.Expect.num = lambda ... self._record()  1 passed   <- this issue
C  override the `expect` FIXTURE with a fake           1 failed
D  set an INSTANCE attribute via a wrapping fixture    1 failed

C and D are closed already, and by the same thing. Full output for C:

pgc_vacuity.VacuityError: vacuity guard: test_false_claim made no counted assertion.
A test that concludes nothing must not report a pass.

The zero-assertion guard is a backstop, and it catches every bypass that stops recording. Replacing the fixture wholesale, or shadowing the method on the instance, both silence the comparison and the count — so the layer refuses on the count.

Which is exactly why B is the dangerous one, and @jdatcmd named the reason before I measured it: the stub has to keep the honest-looking self._record(name) to stay green. B is the hatch that defeats the backstop by respecting it.

Why that matters for the fix

It means the class-level guard is a complete answer to the known surface rather than the next move in a frame-chase. The pattern looked like it would keep receding —

#958  the DATUM       _ORDER_KILLERS
#964  the READER      _sorted_ordered_sites
#967  a CLASS ATTRIBUTE   Expect.num
???   the instance? the fixture?

— and the two frames after this one turn out to be already closed, not by a name guard but by the count invariant that #966 has just made a property. So the receding series terminates here, and "18 methods plus a class guard" is not an arbitrary stopping point: it is the last frame that can both silence a comparison and satisfy the backstop.

I would put that in the fix's docstring, because "we stopped here" is a much weaker sentence than "the frames beyond here are closed by the count guard, measured".

What I have not established

Whether some other mutation can also keep the count rising while silencing a comparison — B is one instance of that class, not the class. The obvious candidates are the two remaining private methods beside _record, and anything that could make a comparison return without comparing. Worth a sweep before calling the surface closed, and it is a different sweep from enumerating the 18.

[linuxhikerpm]

Taking this. Snapshot Expect's public methods the same way #964 snapshots module bindings: a conftest that replaces Expect.num with a stub that still counts is refused at collection, naming Expect.num. _record is excluded (leading underscore), so stubbing it still fails closed via count 0 — the control the issue asked to keep.

[linuxhikerpm]

#986 is the PR. Head 50c83770c8d2ebaaafdf8fc8396d50b830838cd0.

[linuxhikerpm]

#986 closes the class-attribute frame only (pgc_vacuity.Expect.num = stub). It does not keyword-close this issue.

Two routes still keep the count and drop the comparison, so a false claim still reports as a pass:

• expect.num = stub on the instance
• a subclass yielded by an overridden expect fixture

Expect.__dict__ is unchanged in both, which is why the class snapshot cannot see them. This issue stays open for those two.

[OffgridwithJD]

A coverage gap in this guard family, measured so nobody has to guess its severity: the binding-rebind refusal works under -n and nothing asserts that it does.

I raised this on #986 and aimed it at the wrong test. @jdatcmd drew the distinction that matters and it checks out:

test_the_refusal_names_the_binding_that_changed   unparametrized on MAIN too
                                                  -> pre-existing, not #986's
main mentions _changed_methods / _restore_methods: 0
                                                  -> the METHOD surface is #986's own

So #986's merge newly routed the method surface through _collection_usage_error, and that one now has its arm at 5d51ada. The binding surface has been routed since #963 and still has no xdist arm. Correctly scoped out of #986 — it is not theirs to carry.

And it is coverage only, not a defect. Driven on main 5f7f548b, a conftest rebinding a module-level name the layer reads (pgc_vacuity._raises_sites):

	rc	sentence	INTERNALERROR
serial	4	1	0
-n 2	4	1	0

Nothing is broken. What is missing is the assertion, which means a future change that reverted that call site to a bare raise would get a green CI and a bare exit code under -n — the defect #963 exists for, returning silently.

The fix is the decorator and two lines the neighbours already use:

@pytest.mark.parametrize("mode", ["serial", "xdist"])
def test_the_refusal_names_the_binding_that_changed(pytester, expect, mode):
    ...
    extra = ("-n", "2") if mode == "xdist" else ()
    hatched = pytester.runpytest("-p", "pgc_vacuity", *extra)

and reporting through _collection_refusal_row rather than run_failed — which is the sharper half of @jdatcmd's own change and worth copying here: run_failed is satisfied by any non-zero exit, including the INTERNALERROR this family exists to close, so it would make the new arm vacuous in exactly the direction it is meant to guard.

I am not opening a PR for it tonight: test_layer.py already has #986 and my #1005 in flight, and a third concurrent change to one file buys a conflict for a one-line addition. Recording it here so it is not lost with the context that made it findable.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a