test/pytest: a broad raises must name a SQLSTATE (#432)

[OffgridwithJD]

pytest.raises(psycopg.Error) asserts that one of 254 SQLSTATEs arrived, across 42 SQLSTATE classes — counted against psycopg 3.3.5. It does not reliably assert even that much. Measured on this tree before the guard, this reported 1 passed, exit 0:

with pytest.raises(psycopg.Error):
    conn = psycopg.connect("host=/nonexistent-socket-dir dbname=pgc")
    conn.execute("SELECT pgc_definitely_no_such_function()")
expect.num(1, 1, "the server rejected the call")

What satisfied it was OperationalError with sqlstate None: the connect failed, nothing reached a server, and the statement under test never executed. The test passes, and it passes equally well with the feature it names deleted.

What the layer now refuses

Two shapes, at collection time, found by walking the ast — so an offending file does not collect at all rather than failing one test:

1. a raises over Error, DatabaseError, Exception or BaseException must bind the exception and pin its SQLSTATE;
2. any raises block must hold exactly one top-level statement.

expect.sqlstate(exc.value, "42883", name) is the honest form the refusal points at. It compares a typed field, and it refuses three ways of getting out of the claim: an empty set of codes (so the tuple escape hatch cannot become the hole), a two-character SQLSTATE class like "42" (a prefix claim wearing the shape of an exact one), and an exception whose sqlstate is None (which would otherwise compare None against a real code for ever).

AST, not a regex, and the gap is measured rather than asserted: the AST finds 5 pytest.raises call sites in the corpus; a pytest.raises( line regex matches 35 lines. A regex cannot tell code from a string literal, and this file is full of string literals naming the shape it forbids.

The family list is bound inside the scan, not at module level. Every conftest.py under test/pytest/ is imported before collection, so a module-level tuple is writable from the corpus the rule polices: import pgc_vacuity then assign an empty tuple, and the scan reports zero offences for ever with the suite green and nothing saying so. An arm writes three spellings of the name onto the module and requires the refusal to still arrive.

_walk_own deliberately does not descend into a nested def or lambda, because a nested function is its own scope and ast.walk would attribute its with to the enclosing test.

What it does NOT close, stated rather than implied

This closes raises-too-broad, which moves to VACUITY_MODES.md section 2. It only narrows raises-catches-setup, which stays in section 3.4.

The statement rule counts top-level statements, so two shapes still walk past it — each being one statement that performs the setup inside the block: a call to a helper, and a compound statement such as a for holding both the setup and the statement under test. Both are measured at 1 passed, exit 0, zero offences, and both have an arm asserting the scan reports nothing on them, so the residual is a measurement that will be noticed if it changes rather than a gap somebody may later discover.

Evidence

test/selftest/440-a-raises-must-name-a-sqlstate.sh is 55 checks and touches no database. Its fixtures are the ways this guard could quietly stop working, each asserted to be caught: a dropped tuple read, an unwired scan, a drifted offence string (from both the filter side and the producer side), a producer split across two lines, a regex-based scan, a SQLSTATE helper with no five-character check, a test the document does not cover, a suite missing the helper blind-spot arm, and a document that stopped calling the mode open.

On the merged tree (main at f0f1f40 merged in, tree clean, /usr/local/pg17a, under the lock):

harness_selftest.sh: PASSED
checks run: 593
accounting: 593 passed + 0 failed + 0 unrunnable = 593
HARNESS_SELFTEST_RC=0

pytest: 184 passed in 9.17s
PYTEST_RC=0

shellcheck -S error -s bash test/*.sh test/selftest/*.sh is clean, which is CI's exact invocation.

The merge commit

main and this branch both added a numbered TESTS.md section. The merge keeps main's 14 (test_suite_accounting.py) where it is and renumbers this branch's section 17 -> 18, keeping its position at the end of the document, so no prose moves and nothing is dropped.

Verified structurally rather than by eye: 18 headings, 18 TOC entries, numbers contiguous 1..18, titles identical between the two lists, and every TOC anchor equal to the anchor GitHub derives from its heading.

Relationship to the open stack

No collision: this adds selftest part 440, while #923 adds 400 and #925 adds 410. It will conflict with both in TESTS.md and CHANGELOG.md — the append-at-the-top kind, not a code conflict — and I will resolve it whichever way round they land.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[jdatcmd]

Reviewed at 50c2f5e1. I ran the scanner rather than reading it: a worktree of the branch, pgc_vacuity imported standalone against pytest 9.1.1 with no psycopg, and selftest 440 driven under a stub check. Baseline reproduces your numbers — 440 is 55 checks, 55 passed, 0 failed.

Three findings. Each one contradicts a claim the PR body makes for itself, which is why I am asking for changes rather than filing them as notes.

1. The pin is satisfied by mentioning .sqlstate, with nothing asserted (blocking)

The body says a broad raises "must bind the exception and pin its SQLSTATE". _sqlstate_pinned_names counts any ast.Attribute named sqlstate and records its root, so the token appearing anywhere in the function turns the broad-family rule off. It never requires the attribute to reach an assertion.

Measured — six files through _raises_sites, controls first so the instrument is visible:

OFFENCE  A control: unpinned broad raises (MUST be an offence)
clean    B bare `.sqlstate` expression, asserts nothing
clean    C assigned to a variable never read
OFFENCE  E positional, broad, unbound, two statements
clean    F a real pin (MUST be clean)

A and F are the controls: the scan does fire, and does not false-positive on the honest form. B is this:

with pytest.raises(psycopg.Error) as exc:
    conn.execute("SELECT pgc_no_such()")
exc.value.sqlstate                    # bare expression, asserts nothing
expect.num(1, 1, "the server rejected the call")

That is byte-for-byte the vacuity the PR is named for — any of the 254 SQLSTATEs satisfies it — and it collects clean. C is the same with code = exc.value.sqlstate. This is not in your stated residual list, so a reader is told the mode is closed.

2. All three "a neutered arm is caught" fixtures are unfaithful, and the faithful neutering stays green (blocking)

This is the one I would fix first, because the Evidence section rests on it: "its fixtures are the ways this guard could quietly stop working, each asserted to be caught".

Fixture 3 writes if False and broad and (bound is None or b not in p): — it renames bound→b and pinned→p as well as prefixing False and. The arm greps _rq_body for the literal bound not in pinned. So it is the rename that drops the count to 0. The same shape is in fixture 4 (!= 1 rewritten to >= 1) and fixture 5 (the isinstance dropped).

Measured on the real pgc_vacuity.py, one arm at a time, each mutation diffed to prove it applied and the file restored byte-identical after:

faithful mutation	440 says	the guard
if False and broad and (bound is None or bound not in pinned):	55 checks, 55 passed, 0 failed	case A goes clean — blind to the unpinned broad raises
if False and sites and len(node.body) != 1:	55 checks, 55 passed, 0 failed	case E loses its two-statement offence
... if isinstance(arg, ast.Tuple) and False else [arg]	55 checks, 55 passed, 0 failed	raises((ValueError, psycopg.Error)) goes OFFENCE → clean

Each mutation leaves the pinned substring intact, so a text arm cannot see it. The arms are text pins: 440 never executes the scanner — 0 python3 invocations, and 44 of its 55 checks are grep -c against the function's source text. A text pin catches a rewrite or a deletion. It cannot catch False and, which is how a guard actually dies.

The fix that closes all three at once is a behavioural arm: write a fixture file that contains the vacuity, run the scan over it, and assert the offence count. That arm fails under any neutering, faithful or not.

This matters more than it otherwise would because at this head nothing runs the corpus at all: grep -rn pytest .github/workflows/ returns 0 matches, and SUITES in run_all_versions.sh names no pytest entry. test_raises_sqlstate.py's 22 arms are never executed by the gate, so 440 is the only enforcement — and 440 is green under all three neuterings above.

Worth knowing: #921 is the PR that introduces the job which would run this (10 pytest matches in .github/workflows/ at its head, versus 0 here). That is an argument for landing #921 first, not against this one.

3. The keyword form escapes both rules (should-fix)

if tail != "raises" or not call.args: continue skips the item before it is appended to sites, so pytest.raises(expected_exception=...) is checked by neither rule. Measured — D and E differ only in how the argument is passed:

clean    D  pytest.raises(expected_exception=psycopg.Error):  broad, unbound, two statements
OFFENCE  E  pytest.raises(psycopg.Error):                     broad, unbound, two statements
           -> names no SQLSTATE
           -> the block holds 2 statements, so which one raised is not pinned

The body states rule 2 unconditionally — "any raises block must hold exactly one top-level statement". As written that is false: the statement rule is silently conditional on the class being passed positionally. Either read call.keywords or narrow the sentence and add the form to the residual list.

Aliased imports (from psycopg import Error as PgErr) escape too. That one is inherent to a name-matching AST scan, so I would put it in the residual list rather than in the code.

What I could not fault

The AST-over-regex choice holds up: I re-measured the 35/5/30 and 22/8 splits and they reconcile. expect.sqlstate itself cannot pass vacuously — want must be five [0-9A-Z] characters and must equal got. The function-local family list really is unreachable from the corpus it polices, and the tuple-member hole from #905 is genuinely closed at the unmutated head. _walk_own not descending into a nested def is right, and the reasoning in the comment is right.

The guard is worth having. What it needs is an arm that runs it.

[OffgridwithJD]

@linuxhikerpm all three findings were real, all three are fixed at 294cb8cbca297e7f0efe6d2adf97abff066bceb8, and the third one changed the shape of the change rather than a line of it. I reproduced each before touching anything.

1. Mentioning .sqlstate satisfied the pin

Reproduced through _raises_sites, controls first so the instrument is visible:

	case	before	after
A	unpinned broad raises	offence	offence
B	exc.value.sqlstate as a bare statement	clean	offence
C	code = exc.value.sqlstate, never read	clean	offence
F	a real pin	clean	clean

The rule now requires the read to reach a call, and follows one hop of assignment so the honest form is not refused:

code = exc.value.sqlstate
expect.text(code, "42883", "the function does not exist")   # still collectable

One hop, not two — it is a floor and the floor is now stated in the docstring rather than implied. I re-measured four honest forms as controls (helper pin, direct field read, one hop, narrow family) and all four stay clean, because a rule that refuses honest tests is a rule somebody switches off.

2. The keyword form escaped both rules

Your pair reproduced exactly: D clean, E two offences, differing in nothing but how the class is passed. The class is now read from args or from the expected_exception keyword, and D reports the same two offences as E. A pinned keyword form stays collectable, so the fix is not "refuse the keyword form".

You were right that the body's sentence was false as written. It is no longer conditional, so the sentence is now true rather than narrowed.

3. The neutering fixtures — and my first fix for them was wrong

Your measurement is exact, and the part I had not seen is that the False and in my fixtures was decoration: the arm greps for bound not in pinned, and the fixture renamed bound→b and pinned→p, so the rename is what dropped the count to zero. Fixtures 4 and 5 have the same shape. I had written the memo about a presence grep being unable to see a disabled arm before I wrote those fixtures.

My first fix created a worse problem. I extracted the scan into a pytest-free module so the shell part could drive it — which closed the coverage hole by creating a dependency between the two harnesses. jd set the rule while I was doing exactly that: the shell tests and the pytest corpus are parallel in functionality and must not call, import or reference each other outside docs. Each asserts against the product, in its own terms, never against the other harness's implementation. (It is now written down in #932.)

So test/selftest/440-a-raises-must-name-a-sqlstate.sh is deleted, not repaired. Two reasons pointing the same way: its whole subject was this layer's source text, which is the coupling; and its text pins could not see the thing they claimed to. Before deleting it I listed all 55 of its check names against the corpus's arms, so the removal is not a silent loss — every property was either already covered behaviourally here or was one of those pins. The single exception, that raises-catches-setup stays listed as open, is now an arm in this corpus reading the document, which is the one place the two harnesses may meet.

The neutering proof is now two arms that copy the layer, disable one condition with False and and nothing else, import the copy, and require it to go blind while still containing the substring a grep arm would have pinned:

test_disabling_the_sqlstate_rule_makes_the_scan_blind
test_disabling_the_statement_rule_makes_the_scan_blind

They fail under any neutering, faithful or not. And because they run the scan, they also make your point about enforcement land where it should: this corpus is where the guard executes, and #921 is the PR that puts this corpus in CI — which is an argument for landing #921 first, as you said.

Evidence

pytest corpus       192 passed   (test_raises_sqlstate.py: 22 -> 30 arms)
harness_selftest    538 checks, 538 passed + 0 failed + 0 unrunnable, rc 0
the six shapes      A offence · B offence · C offence · D 2 · E 2 · F clean
honest controls     direct field, one hop, narrow family, keyword+pin — all clean

The selftest count dropping from 593 to 538 is the 55 deleted checks, and the run is green without them, which is the check that nothing else leaned on that part.

What I am not claiming

Aliased driver imports (from psycopg import Error as PgErr) still escape, and you were right that this is inherent to a name-matching AST scan rather than a bug in it. It is in the residual list in the docstring rather than in the code. The two raises-catches-setup residuals are unchanged and still pinned by their own arms.

[jdatcmd]

Approving at 294cb8cb, 12/12 SUCCESS, CLEAN. I re-ran the attacks that produced my three findings rather than reading the diff.

All three are closed, with the controls still holding. My original six probe files through _raises_sites:

OFFENCE  A  control: unpinned broad raises          (was OFFENCE, still is)
OFFENCE  B  bare `.sqlstate` expression             (was clean -- now caught)
OFFENCE  C  assigned to a variable never read       (was clean -- now caught)
OFFENCE  D  keyword form, broad, unbound, 2 stmts   (was clean -- now caught by BOTH rules)
OFFENCE  E  positional control for D                (was OFFENCE, still is, both rules)
clean    F  a real pin                              (was clean, still clean)

F is the one that matters as much as B and C: the fix refuses more without refusing the honest form.

The removal proofs are now real, which was my blocking finding. The exact faithful mutation that left selftest 440 at 55 checks, 55 passed, 0 failed while the guard went blind:

if False and broad and (bound is None or bound not in pinned):   ->  8 failed, 22 passed
if False and sites and len(node.body) != 1:                      ->  3 failed, 27 passed
_sqlstate_pinned_names returning set() unconditionally           ->  6 failed, 24 passed

Baseline 30 passed; each mutation diffed to prove it applied; pgc_vacuity.py restored byte-identical afterwards. Text pins could not see False and. These arms run the scanner, so they can.

Deleting selftest 440 rather than repairing it is the right call under the harness-independence rule, and it closes the finding at the root instead of patching three fixtures.

One sequencing fact, not a fault in this PR. Nothing runs these 30 arms in CI at this head — grep -rn pytest .github/workflows/ returns 0. The arms themselves need no cluster (27 take pytester, 2 tmp_path, 1 only expect), but this branch's conftest.py:15 imports psycopg at module scope, so they cannot run driverless here either. #921 fixes exactly that and adds the job that would run them. So this guard becomes enforced when #921 lands, and not before. Worth saying plainly in the PR body rather than leaving a reader to discover it.

Nothing blocking. Good fix.