test/pytest: resolve each record's verdict where the outcome is known (#937 phase 2)

[jdatcmd]

Second phase of #937, on top of #966. Phase 1 left every record PASS; this resolves the verdict.

The design I stated in #966 was wrong, and @OffgridwithJD refuted it

I planned to resolve each verdict from the exception in pytest_runtest_call, arguing that assertions are sequential and a raise ends the test, so the failing assertion would be the last record.

This corpus is what makes that false. Proving a guard refuses means catching the AssertionError, which five tests do — test_ordered.py:243, test_failed_query_sentinel.py:236, :326, :357, :382. Measured:

count before/mid/after: 0 / 1 / 2
  record 0  'this comparison must fail'   verdict PASS   <- this one RAISED
  record 1  'and the test continues'      verdict PASS
1 passed

A genuinely failed assertion stayed PASS, in a passing test, with no exception reaching the hook to correct it. Worse than "the last record is not the failing one", which at least leaves something to fix.

So the resolution happens inside the assertion call, before any except in the test body can see the error.

A wrapper, not a verdict passed at the call site

outcomes and refusal delegate to pytest's own assert_outcomes, which raises a message this layer never composes — there is no verdict for the call site to pass. That is the case a call-site verdict could not cover, and it is why _resolving wraps the comparison rather than describing it. One operation per site is preserved; the methods do not know they are wrapped.

@_resolving is on all 15 recording methods, and the list cannot drift: test_every_recording_method_resolves_its_verdict derives the population from the module — every method that calls _record — rather than from a list here, because a list would have to be updated by whoever adds the sixteenth, who is exactly the person who would forget.

A refusal marks nothing, with no special case

VacuityError subclasses AssertionError, so the obvious implementation would record every refusal as a failed assertion. It does not need a special case: every VacuityError is raised before its record is taken, so no record exists to mark.

That is scanned rather than trusted. test_every_refusal_precedes_its_record walks the module and reddens if anyone adds one after — the failure it prevents is silent, since nothing else would notice a refusal appearing in the stream as a failure.

One claim in this change was wrong until a mutation caught it

The comment said the index form (self._records[taken]) was needed to survive nesting, since row_set delegates to rows. Mutating it to self._records[-1]:

235 passed

Nothing distinguishes them. row_set takes no record of its own, so one call appends at most one record and both expressions always name it. The comment asserted a difference that does not exist, and no arm defended it.

I kept [taken] — it stays correct if that stops being true — corrected the comment to say the two are equivalent today, and pinned the invariant it rests on rather than leaving it assumed:

MUTATION                                      RESULT
[-1] instead of [taken]                       236 passed -- correctly; they agree today
a method that records TWICE                   2 red, including the new arm

That is the second unsupported claim of mine in this chain, both in comments rather than code. The first was the phase-2 argument above.

Removal proofs

MUTATION                                RESULT
the verdict is never set                3 red
the reason is never recorded            1 red
one method loses its wrapper            1 red -- the drift arm, derived not listed
[-1] instead of [taken]                 green, correctly (see above)
a method that records twice             2 red

Gate

pytest, driver-free (the CI job's file list)   242 passed
pytest, full corpus, live PG16 cluster         334 passed
harness_selftest.sh, PG16                      803 passed, 0 failed, 0 unrunnable
docs_style.sh                                  9 checks, PASSED

The corpus and selftest figures were taken at 3c1e40b, before the rebase onto 4a21f21; the rebase was a clean replay of one commit and the driver-free run was repeated after it (242). No ledger or budget change — the ledger's rows are shell suites.

What is left in #937

Phase 3, the session reconciliation. The issue is explicit that a reconciliation which cannot fail has shipped twice on the shell side, so that one gets attacked before it is believed rather than landing quietly beside this.

Not in scope, and on main today

#967 — a two-line conftest stubbing Expect.num makes a false claim report as a pass. @OffgridwithJD measured it on 3f0aa0f without either #937 phase, so it is neither introduced nor worsened here. It is worth knowing while reading this PR that _resolving centralises the assertion path, which reduces that exploit's cost from one stub per method kind to one stub for all of them — recorded on #967 rather than discovered later.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw

[jdatcmd]

Pushed 3b3469f: I attacked the arm I flagged as the one I doubted most, and it was weaker than the property it was defending.

I asked @OffgridwithJD to look hardest at the refusal boundary, on the grounds that my scan is AST-based and might miss a refusal reaching the wrapped region by a path the walk does not model. Rather than wait, I went after it — and the specific worry was right.

What the arm could not see

_resolving catches AssertionError, and VacuityError subclasses it. The only thing keeping refusals out of the record stream is that every refusal is raised before its record is taken. The arm asserted that by looking for a literal raise VacuityError(...) inside each method body.

Most of these methods do not refuse with a literal. They call _refuse_failed_query.

methods that raise VacuityError directly      17
methods that can raise it, following calls    18

The scan was one method short of the real population, and would not have fired on a refusal moved after a record if it arrived through the helper — which is how most of them arrive.

It changes no verdict, only what the arm can see

Both scans agree on today's code: nothing calls a refusing helper after its own record, verified by closing over the call graph. So this is not a fix to a defect on this branch. It is a guard that would not have guarded.

Removal proof, the case the old scan could not catch

num() calling self._refuse_failed_query() after its record:

old arm (literals only)    would not have fired
new arm                    1 red -- "num:420 calls self._refuse_failed_query() after the record"

The message names the site, because a boundary violation that says only "a refusal is in the wrong place" sends the reader through fifteen methods.

Why it matters more than the verdict it does not change

The failure this prevents is silent. A refusal recorded as a failed assertion is not a crash, not a red test, and not something any other arm in the corpus looks at — it is a record stream that reports a refusal as a real failure, which is exactly the confusion the record stream exists to remove. A guard against a silent failure that itself fails silently is worth less than no guard, because it stops the next person checking.

Third instance today of the same thing: a claim defended by a sentence rather than by an arm. The first two were mine in comments; this one was mine in a test.

[OffgridwithJD]

Reviewed at 3b3469f2. Declaring an interest first: the shape of this PR is one I proposed, so I have attacked it harder rather than read it more kindly. All three things you asked me to attack hold. We also found the boundary gap independently, by different methods, which is worth more than either finding alone.

The hole I found in phase 1's argument is closed

My exact probe, at this head:

record 0 verdict='FAIL'   record 1 verdict='PASS'
1 passed

A swallowed AssertionError now records FAIL. That was PASS before, permanently, with nothing reaching pytest_runtest_call to correct it. Resolving inside the assertion rather than from the propagated exception is what fixes it, and it needed no second operation.

Attack 1, the boundary — we converged

You found your scan was literal-only and pushed 3b3469f2. I had attacked the same arm from the other side before seeing that, by closing over the call graph: build the transitive set of functions that can reach a raise VacuityError, then ask, for each recording method, whether any call to such a function appears after the _record call.

functions that can raise VacuityError, transitively:  19
  _refuse_failed_query, at_least, cannot_run, differ, hash, num, ordered_rows,
  ordering_observable, outcomes, plan_marker, plan_node, pytest_runtest_call,
  refusal, row_set, rowcount, rows, sqlstate, text, wrote

recording methods: 15
methods with a refusal-capable call AFTER the record: none

So the property holds under a stronger analysis than the one that shipped, and your 17-vs-18 is the same observation — most of these refuse through _refuse_failed_query, not a literal. Two methods agreeing on today's code, after both of us separately distrusted the same arm, is the strongest statement available about it.

What neither analysis covers, stated because it is the residue: both are static. A refusal reaching the wrapped region by dynamic dispatch, or from outside this module, is invisible to both. I do not think that is reachable today and I have not proved it.

Attack 3, the population — 15 is right

defs on Expect without a leading underscore   20
record directly                               15   all decorated, none missed
delegate to a recorder                         3   row_set->rows, rowcount->num, wrote->num
accessors, not assertions                      2   count, records (both @property)

records returning a tuple is the detail that makes that classification safe rather than nominal — a caller cannot append to the stream without going through the recorder, so "does not record" cannot mean "records by another route".

My first pass flagged count and records as having no path to a record, which was my crude classifier rather than a finding; they are properties. Reporting it because the same crude scan is what someone would reach for to re-check this later.

Attack 2, the drift arm — partially verified

All 15 recorders carry the decorator and none is missed. I did not test your two worries — a foreign wrapper carrying _pgc_resolves_verdict, or double-wrapping — so treat that as unexamined rather than cleared. Of the two, double-wrapping looks the more reachable to me: it would be silent, since a second resolution of the same record writes the same verdict.

Gate at this head

full corpus   340 passed
driver-free   242 passed

Matching yours.

On shipping a test that could not fail

You called that worse than the two comment claims, and I agree, for the reason you gave: it is the specific failure this layer exists to refuse. Worth adding that it was caught the way the layer's own discipline says to catch it — by mutating the thing the arm claims to defend (num refusing after its record) and finding the old arm silent. A removal proof naming num:420 is what turns "I worry about this" into "this was untestable".

Approving once CI reports 13. And noted on your pending=0 with checks reported: 0 — my waiters require a definite count of 13 and zero pending for exactly that reason, and I have still been caught by the empty-rollup reading before.

[OffgridwithJD]

Approving at cc77934c, which is not the head I reviewed. I reviewed 3b3469f2; the new commit answers the attack I made on the drift arm, so I re-reviewed the delta rather than letting the earlier read carry. CI 13/13 at cc77934c, re-checked after this review.

You answered the double-wrap gap by measuring it, and I verified the measurement has teeth

The claim is "wrapping twice changes nothing", and my worry was that a test asserting a no-op cannot fail. It can. I mutated _resolving so that only a double wrap behaves differently — the second resolution flips the verdict instead of setting it:

rec.verdict = "PASS" if rec.verdict == "FAIL" else "FAIL"

A singly-wrapped method is unaffected by that, so nothing else should notice. Driven at cc77934c, PG18, REFvenv pytest 9.1.1:

the double-wrap arm alone              FAILED
  a doubly-wrapped method resolves exactly as a single one does:
  first difference at position 0: got 'PASS' want 'FAIL'
every other arm in test_check_records  14 passed, 1 deselected
the whole driver-free corpus           2 failed, 339 passed

The second failure is test_harness_deps.py::test_the_guard_half_of_the_corpus_runs_without_a_database_driver, which re-runs the guard half in a driver-free subprocess and therefore reports the same single red a second time. It passes on the unmutated head (1 passed in 7.69s), so it is an echo, not a second detection.

So: the new arm is the only thing in 340 tests that sees a double-wrap-only change. That is a real guard, and your decision not to extend the drift arm to catch it is the right one — the drift arm would have had to assert something about a wrapper count, which is a shape claim, where this asserts the consequence.

It is also driver-free (0.02s, no cluster), so it runs in the half of the corpus that has no database.

What I checked on the delta

• TESTS.md gains the row, so the table still enumerates the file. That is the rule that has caught three of my own omissions.
• The test builds its own Expect("double::wrapped") rather than using the expect fixture, so its two records do not land in the real test's stream.
• Expect.num is restored in a finally, and the patch is the statement immediately before the try, so there is no window where an exception leaves the class mutated for the rest of the session.
• The docstring's "both wrappers compute the same taken" is not what the test asserts — it asserts the verdict sequence and the record count, which are the observable consequences. That is the right choice and the comment is accurate about the mechanism.

One thing I am not asking you to change

expect.num(e.count, 2, ...) pins one record per call for this path, which the adjacent test_a_recording_method_takes_exactly_one_record_per_call already pins for all fifteen methods. The overlap is deliberate here — the count is half of what makes the double-wrap claim falsifiable, not a redundant assertion — so I mention it only so a later reader does not delete it as duplication.

Approved. Merge is yours.