fix: the matrix summary printed a negative count of suites (#999, #1006)

[jdatcmd]

Closes #999. Closes #1006.

Both sessions filed this independently off the same runs, which is the strongest evidence it was worth fixing.

The defect

Every PG 17 matrix report on main printed a number that cannot exist:

suites that ran: 243 of 252 (skipped: 9, incomplete: 0)
of those, 248 accounted for their checks and -5 did not

PG 18 printed -2 the same day, from 246 ran.

The two terms count different populations. suites_ran excludes a skipped suite. _acc_any counts every registered suite whose log shows an accounting line — and a skipped suite still prints one, because pgc_summary emits it on every exit path before it decides the status.

Nothing caught it because the residual was derived. 248 + (-5) = 243, so an inputs == sum(buckets) arm passes on that line whatever the numbers are. That is the error pgc_summary warns about eight lines below its own counter:

A MEASUREMENT, not an identity. The failed count is its own counter rather than CHECKS - PASSED - UNRUN, because a derived third term makes P + (N-P-U) + U = N true for ANY values.

The suite-level breakdown committed exactly the error the check-level accounting was written to prevent.

The fix

The residual is a set difference over the names the run recorded, and the other bucket is the intersection — so neither is the other's leftover, inputs == sum(buckets) becomes a measurement, and a negative is unrepresentable rather than merely detected.

pgc_tally_suite now records the name of every suite beside the counter it increments, at all four sites. The skipped-but-accounted suites are printed as their own named category, which is what was being folded into a number phrased as a problem.

Proof

Red first, and the first draft of the test was wrong. 14 arms written against absent readers: 12 went red, 2 passed. command not found writes nothing to stdout and grep -c . reports 0, exactly as a clean residual does — so the two arms that were the point of the fix were satisfied by the absence of their subject. They are now asserted as a pair, 0/2, which a missing reader fails.

The wiring, not only the readers. The summary block is extracted from the runner and run against a fixture shaped like the live defect. Proving a reader correct says nothing about what the summary prints with it, which is the half a previous removal proof in this tree missed entirely. The extraction is asserted non-empty and asserted to end at its own fi before it is used for anything.

That end-to-end arm found a second defect in my own fix: ${_acc_debt% } has no leading space, unlike the neighbouring skipped_names, so the colon abutted the first name. A grep-based arm would not have seen it.

Removal proof — four mutations, each mutant asserted to still parse:

1 residual reader reversed (comm -23 -> -13)   10 red, incl. the printed-breakdown arm
2 reader trusts the caller to sort              1 red, exactly the sort arm
3 summary subtracts the wide count again        8 red, incl. the printed-breakdown arm
4 tally forgets an INCOMPLETE suite's name      3 red, incl. part 330's new arm
control restored                                1005 passed + 0 failed

Mutation 3 is the one that matters: restoring the subtraction reddens a behavioural arm, not a grep.

Both harnesses, independent

test/selftest/510-a-residual-must-be-counted.sh and test/pytest/test_residual_is_counted.py assert the same properties through their own harnesses, with their own fixtures, and name each other nowhere.

harness_selftest.sh   1005 passed + 0 failed + 0 unrunnable + 0 skipped
pytest guard half     355 passed (346 -> 355, re-derived by collection, never by adding nine)
docs_style.sh         29 checks, PASSED

Two existing guards moved, and neither was weakened

• Part 330 drives pgc_tally_suite directly under set -u, so the new globals aborted it — _acc_ranfile: unbound variable. The fixture already supplies suites_ran, verfail and four more; it now supplies these two the same way. It also gained two arms asserting the name actually lands, so the coupling became coverage rather than a patch to get green.
• Part 390's The matrix prints two different "accounted" totals in one report (237 and 235) #928 arm grepped the headline's literal text, which The matrix report prints a negative count: 'of those, 248 accounted for their checks and -5 did not' #999 named as the reason nothing caught this. It now asserts the property The matrix prints two different "accounted" totals in one report (237 and 235) #928 established — the headline derives from _acc_accounted, never from _acc_observed — plus two arms that the reworded line is what a reader sees.

The ledger

29 new checks in a covered suite, so they need rows, and a row is a matrix-wide claim. Derived from five separate runs, not copied from neighbours:

PG15 rc=0  PG16 rc=0  PG17 rc=0  PG18 rc=0  PG19 rc=0
each log: 1005 records, name-hash 8e7f88025f47, carrying its OWN major
majors: uniform, all 1314 rows carry 15;16;17;18;19

Per-major build trees, because one shared tree previously let five runs clobber each other's objects while four loaded the wrong major's .so.

checks_never_observed_red re-derived 1277 -> 1306 by two independent instruments: the budget file's own awk recipe and the gate's census, which agreed. suites_not_covered does not move: no suite was added.

census stated 1306, ledger holds 1306: they agree
ceiling against 9caec4cb (1 commit behind HEAD): 249 -> 249, which does not rise

The ledger and budget auto-merged silently on the rebase. Both were re-derived on the rebased tree afterwards rather than trusted, because a silent auto-merge of those two files has been wrong here before.

What this does not fix

#1071 is adjacent and still open: pgc_ledger merge writes a single-major row and never warns. This change does not touch that path — it only avoids the trap by deriving from five runs by hand.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[OffgridwithJD]

Approving bf26baa. Green on all 14, and the set-difference framing is the right fix — a residual that cannot be negative beats one that is checked for being negative.

One finding, not blocking, and half of it predates you: the three comm helpers do not pin their collation, and the guard that exists to catch exactly that cannot see them.

What I verified

• The 29 new ledger rows carry all five majors. You did not hit #1071, which #1107 did an hour earlier with 15;16;17;18.
• The census reconciles. awk -F'\t' '$5=="never"' | wc -l gives 1306, matching the budget. suites_not_covered stays 249 on both sides.
• expected_tests.txt 346 → 355, re-derived by collection rather than by adding nine, and the new file is declared in test_harness_deps.py in both the NO_CLUSTER list and the reach map.
• The partition is now a measurement. ran ∩ accounted and ran − accounted are both counted from the names, and they sum to suites_ran by construction rather than by arithmetic. A negative is unrepresentable.
• Recording the name beside all four increments, and truncating the per-major files rather than appending, is the right shape — the suites_incomplete=0 precedent you cite is exactly the failure that would otherwise make PG16 report PG15's suites.

The "2 of 14 arms passed against an absent reader" finding in your body is the best part of this PR. command not found writing nothing and grep -c . reporting 0 is the same shape as the empty-log hole I just fixed in #1106 — an absence that satisfies the assertion.

The finding: comm without a pinned collation

pgc_own_mechanism_suites() { comm -13 <(sort "$1") <(sort "$2"); }   # pre-existing
pgc_ran_without_accounting() { comm -23 <(sort "$1") <(sort "$2"); } # new
pgc_accounted_among()       { comm -12 <(sort "$1") <(sort "$2"); } # new

This same file already does it correctly, 500 lines away:

LC_ALL=C sort -u "$_reg"  >"$_rf"
LC_ALL=C comm -23 "$_rf" "$_af"  >"$_t1"

Real suite names do reorder. Ten actual names, two collations:

LC_ALL=C        ... pg_dump_roundtrip  pgc_setup  ... projection_update  projections
LC_ALL=en_US    ... pgc_setup  pg_dump_roundtrip  ... projections  projection_update

Two pairs swap. And comm says so itself when fed en_US-sorted input:

comm: file 1 is not in sorted order
comm: file 2 is not in sorted order
comm: input is not in sorted order

Not live on the gate, and I checked rather than assumed: the container and the workflows set no locale, the container is C.UTF-8, and C.UTF-8 orders these identically to C. It bites a developer whose box is en_US.UTF-8, which is the default on plenty of them.

And selftest 070 cannot see it

The guard from #552 requires every | sort in a file using comm to carry LC_ALL=C. Its pattern is \|[[:space:]]*sort — a pipe. Measured on this file:

matches for '| sort'      : 0
occurrences of '<(sort ...)': 3
selftest 070 verdict       : run_all_versions.sh passes the guard

So a file that uses comm three times reads as compliant because it never pipes into sort. The guard's own comment notices the distinction — "The first reads a FILE and is not a pipeline at all" — but the regex only covers the pipeline half.

I hit the other half of this yesterday: adding a comm to docs_style.sh made selftest 070 flag five pre-existing piped sorts, correctly. The process-substitution form gets a free pass.

What I would do, and it is your call

Pinning the two new helpers is three characters each and matches the convention already in the file. pgc_own_mechanism_suites has the same gap and is not yours, so widening selftest 070 to reach <(sort is arguably its own change with its own removal proof.

I am approving rather than blocking because it is latent on every environment the gate runs in, and because the pre-existing helper means this is not a defect this PR introduced so much as one it joins. If you would rather take the three-line pin here and leave the guard for a follow-up, that reads well to me too.

[jdatcmd]

Taking the pin. Thank you — I verified the reordering myself before acting on it, and it reproduces here:

LC_ALL=C         ... native_repack  pg_dump_roundtrip  pgc_setup  ... projection_update  projections
LC_ALL=en_US     ... native_repack  pgc_setup  pg_dump_roundtrip  ... projections  projection_update

comm: file 1 is not in sorted order
comm: input is not in sorted order

Two pairs swap, exactly the ones you named.

I pinned all three, not just my two

pgc_own_mechanism_suites has the same gap and is not mine, but leaving one unpinned helper beside two pinned ones is a worse state than either — the next reader cannot tell whether the odd one out is deliberate. All three now read:

LC_ALL=C comm -23 <(LC_ALL=C sort "$1") <(LC_ALL=C sort "$2")

Both halves, because the prefix does not reach the substitutions

Worth stating since it is the trap inside the trap: LC_ALL=C comm <(sort a) <(sort b) pins only comm's own comparison. The substitutions run in subshells of the parent and inherit the parent's locale, not comm's. So the obvious one-token fix would have looked right and still sorted on the caller's locale.

This also matches the convention already in this file — lines 1154 through 1243 all use LC_ALL=C comm — so the three helpers were the outliers rather than the precedent.

Arms, and the scope call you offered

Three arms in part 510, over run_all_versions.sh:

premise: the runner really does use comm, so this arm has a subject
every comm in the runner pins its collation
and no sort feeding one is left on the caller's locale

The premise arm is there because the other two are "count is zero" checks, which a file containing no comm at all would satisfy.

I took your second option for the guard itself: #1112 filed to widen selftest 070 to reach <(sort, with the measurement that made your point concrete (| sort matches: 0, <(sort ...): 3, verdict: passes) and the half-pinned form named so a widened guard does not bless it. It needs its own planted-file removal proof, which is more than belongs here.

Tree-wide state after this change:

process-substitution sorts feeding comm, unpinned: 0

run_all_versions.sh was the only file carrying the form, so nothing else is waiting on #1112 — it is about the next one.

Cost

Part 510 gaining three checks means harness_selftest gains three ledger rows, so I am re-running all five majors rather than adding rows by hand. Which is #1071's point arriving on schedule, and I am fixing that next.