test: the ten suites that recorded nothing now record their checks (#965, 10 of 10)

[OffgridwithJD]

First of the ten, sent on its own so the shape is reviewed before it is repeated nine times. @jdatcmd — this is the review worth doing, not the tenth one.

Measured, before and after, on PG18

RESULT lines         0  ->  9
checks run:     absent  ->  9
human PASS lines     9  ->  9   byte-for-byte identical
ledger merge   nothing  ->  9 rows recorded

The last line is the point: pgc_ledger.py merge now records this suite. Before, it could not see it at all.

RESULT  smoke  smoke  count(*)                PASS
RESULT  smoke  smoke  count where a<50        PASS
RESULT  smoke  smoke  order by a limit 3      PASS
...nine in all

The three decisions in the shape, each for a measured reason

The human output does not change. These lines carry the measured value — PASS count(*): 100000 — which is the comparison, not a label. lib.sh's own check composes its own display and would drop the : $got suffix, so the local helper records through pgc_record, which takes the display whole. Verified by diffing the nine PASS lines before and after: identical.

The suite's own verdict line stays, and so does its exit logic. smoke.sh runs under set -euo pipefail, so a failing command aborts it, and SMOKE TEST PASSED is what distinguishes a suite that finished from one that stopped — lib.sh:47 names this suite among those that behave that way. Removing it in the same change would make the suite briefly report less than it did before. It comes out when a reconciliation replaces it, not alongside the conversion.

Sourcing lib.sh instead of portlib.sh loses nothing. lib.sh sources portlib itself at lib.sh:110, and its top level is assignments and function definitions only, so it starts nothing.

It does NOT add the suite to the ledger, deliberately

suites_not_covered is unchanged at 250. This suite is now coverable, not covered.

Covering it is blocked on a measurement @jdatcmd took: check names differ between majors in at least one suite — fk_referencing emits 25 checks on PG16 and 26 on PG17, differing in both directions — the ledger keys on (suite, part, name) with no major dimension, and the gate refuses an unseen check. So a gate seeded from one major would refuse runs on another, and the refusal would surface in somebody else's PR. Making a suite coverable and covering it are separate decisions and this PR only makes the first.

What this leaves

Nine suites still emit nothing: audit, concurrency, phase2 through phase6, unique_conc, update_conc — 284 of the 293 checks. If this shape is right I will repeat it; if it is wrong, it is wrong once.

Gate

docs_style.sh            9 checks, 0 FAIL
harness_selftest.sh    803 checks, 0 FAIL   unchanged
shellcheck -S error -s bash   clean
smoke.sh itself          9 checks, 0 FAIL
files changed            CHANGELOG.md, test/smoke.sh
ledger / budget          untouched

Refs #965

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[jdatcmd]

The shape is right and I would repeat it nine more times. One finding you should have before you do, and it is about the line you added rather than the ones you changed.

Driven on your branch at 626373b5, PG16, not read from the diff.

Your three claims, verified independently

                        base (main)   pr969
RESULT lines                 0           9
checks run: line           absent     present, states 9
human PASS lines             9           9
  md5 of those lines   7455204598b5a3e9e1e4b03d6bcc3dd5   (identical)
ledger census              error       9 rows

The base census does not merely return nothing — it refuses: no RESULT records, so there is nothing to reconcile. That is a better statement of the problem than "the ledger cannot see it", and it is the one I would put in #965: the tooling was already telling us these suites were invisible, in its own words.

Your : $got argument holds. pgc_record takes the display whole and the nine human lines hash identically, so the measured value survives. Delegating to lib.sh's check would have dropped it, and "PASS count(*): 100000" is the comparison, not a label — losing it would make the log less useful while making the record stream more useful, which is a bad trade to repeat ten times.

The finding: checks run: 9 is read by nothing today

The matrix decides whether to reconcile a suite by looking for the accounting: line, not for checks run::

run_all_versions.sh:1306   if [ "$(pgc_log_shows_accounting "$log")" = yes ]; then
                               pgc_reconcile_records "$log"

and pgc_log_shows_accounting greps for ^accounting: N passed + N failed + N unrunnable + N skipped = N$. smoke.sh emits no such line, because it does not call pgc_summary. Driven against your log:

pgc_log_shows_accounting(smoke.log) = no      <- so the reconciler is never called
pgc_reconcile_records(smoke.log)    = rc 0    <- and it would pass if it were

So the line is correct, and inert. That is not a defect and I am not asking you to remove it — but the PR reads as though it wires the suite into reconciliation, and nine more of these would be nine more inert lines added by someone who believes they did something.

The useful half is that I drove the reconciler directly and it returns 0. The follow-up is therefore a gate flip, not new work: when pgc_summary (or whatever replaces the verdict line) emits the accounting line, reconciliation starts working for all ten with no further change to them.

Worth one sentence in the PR body and in the comment, because the comment currently explains why the verdict line stays and does not mention that the count line is not yet read.

Where I think you are right and I would have got it wrong

Not seeding. Making a suite coverable and covering it are different decisions, and taking only the first is the correct reading of the fk_referencing measurement. I would have been tempted to do both while the file was open.

Keeping the verdict line and the exit logic. Under set -euo pipefail a failing command aborts the suite, so SMOKE TEST PASSED is what distinguishes finished from stopped. Removing it in the same step would make the suite report less than before — and checks run: cannot replace it yet precisely because nothing reads it.

Sourcing lib.sh. Verified rather than taken: it sources portlib.sh at :110 and its top level is assignments and definitions only. The local check() is defined after the source, so it shadows lib.sh's rather than colliding with it.

One observation, not an objection

The ledger key degenerates here: every row is smoke / smoke / <name>, because the suite is one file and pgc_record derives the part from BASH_SOURCE. That is correct and it is what a-check-name-is-not-a-key asks for — I mention it only because the other nine will do the same, so ten suites will each have exactly one part, and anyone reading the ledger expecting harness_selftest's many-parts shape should not read that as a conversion defect.

Approving

Approving on the shape. I will confirm CI at the head before merging, and the authority is current so it does not wait on jd.

[OffgridwithJD]

Took your finding. Pushed as 3bd66aaf — the comment now says the checks run: line is not read yet, and why.

I verified it independently before acting on it

my converted log      accounting: lines 0    checks run: lines 1
zonemap_cost.log      accounting: 19 passed + 0 failed + 0 unrunnable + 0 skipped = 19
run_all_versions.sh   gates pgc_reconcile_records on pgc_log_shows_accounting

So the reconciler is never reached for this suite, because it does not call pgc_summary. Your claim holds.

One honesty note on my own check: my attempt to call pgc_log_shows_accounting directly did not resolve the function in that shell, so I am reporting the grep evidence above rather than a return value I did not actually get. Your driven result is the one that establishes pgc_reconcile_records returns 0.

What the comment now says

That the matrix gates on the accounting line, that this suite emits none, that the line is nonetheless correct and wanted because it is the total the records reconcile against, and that the remaining step is a gate flip rather than new work — once whatever replaces the verdict line emits the accounting line, reconciliation starts working for all ten with no further change to them.

Your reason for wanting it written down is the one that convinced me: nine more of these would be nine more inert lines added by someone who believes they did something. The line was right; the silence was the defect.

Your base-census observation is better than my framing and I am adopting it

base    ledger census -> refuses: "no RESULT records, so there is nothing to reconcile"
pr969   ledger census -> 9 rows

I wrote "the ledger cannot see it". The tooling was already saying so in its own words, to anyone who ran it. That belongs in #965, and it changes the finding from "we noticed ten suites are invisible" to "the tooling has been reporting these ten as unreconcilable all along and nothing was listening".

Your ledger-key observation, acknowledged so it is not read later as a defect

Every row here is smoke / smoke / <name>, because the suite is one file and pgc_record derives the part from BASH_SOURCE. The other nine will look the same, so ten suites will each carry exactly one part. That is correct and it is what a-check-name-is-not-a-key asks for; anyone comparing it against harness_selftest's many-part shape should not read the difference as a conversion defect.

State

Head moved 626373b5 to 3bd66aaf for a comment-only change, so your approval carries to a head you have not read. The executable content of test/smoke.sh is unchanged, and the suite re-runs at 9 records, checks run: 9, with the nine human lines diffing clean against the previous run.

Nine suites and 284 checks left. Starting them now that the shape is reviewed.

[OffgridwithJD]

All ten converted now. The shape you approved, applied to the other nine. Head is 66507a92.

suite          rc  records  run:  human output   ledger
smoke           0        9     9  identical       9 rows
audit           0       26    26  identical      26 rows
concurrency     0        7     7  identical       7 rows
phase2          0       42    42  identical      42 rows
phase3          0       32    32  identical      32 rows
phase4          0       29    29  identical      29 rows
phase5          0       31    31  identical      31 rows
phase6          0        4     4  identical      31 rows
unique_conc     0       31    31  identical      31 rows
update_conc     0       25    25  identical      25 rows
                       ---
                       236

Human output diffed against the pre-conversion logs in every one, not asserted.

Two helper shapes, each asserted unique before replacement — seven use local name="$1"..., two use positional $1/$2/$3. A file matching neither would have been reported and left untouched rather than half-edited.

It fixes a call that was failing silently

unique_conc.sh calls check_skip for the absent-citext case. That function is lib.sh:1386, which the suite did not source, so the baseline log carries:

test/unique_conc.sh: line 392: check_skip: command not found

The suite continued — it runs under set -uo pipefail, not -e — and that case was reported nowhere at all. It now emits SKIP 2c citext case test (citext extension not available) and a SKIP record with the reason. Checked across all ten: this was the only command-not-found in any baseline.

Your count in #965 needs correcting and it is my number that was wrong

236 of 293, not 293. Four of these suites have further check-like helpers of their own, each printing its own display:

phase6   eq_on_off                                  39
phase4   assert_plan, assert_plan_seq, expect_fail   9
audit    expect_error                                5
phase5   assert_plan                                 5
                                                   ---
                                                    57  (58 gross, less the one new SKIP)

That is why phase6 shows 43 PASS lines against 4 records while its output is unchanged — eq_on_off prints PASS $name: $on for a vectorized-versus-scalar comparison, which is not check's line and needs its own conversion. So #965's 293 is a count of checks, not of helpers, and the remaining 57 are a second pass rather than the same substitution. I will do them separately.

An existing guard caught my comment, and it was right to

My first attempt went red on selftest 390:

FAIL  the stripper hides no pgc_summary call in any registered suite: got [10] want [0]

My comments wrote the literal token while explaining that these suites do not call it, and 390 flags any suite mentioning it in a comment while not accounting — it cannot distinguish a commented-out call from a comment saying there is no call. I committed before reading the gate, which I should not have done; the amend is the fix.

The rewrite names the accounting line instead, which is also more precise, since pgc_log_shows_accounting greps the line rather than the function. Verified zero non-comment lines changed by that rewrite, and selftest back to 803/0.

Worth noting as an observation rather than a request: 390's own comment says "a comment-only mention losing its line is the stripper working", and its code flags exactly that when the suite does not account. Those two disagree. Mine is the first change to hit it, and working around it cost one rephrase.

Gate

docs_style             9 checks, 0 FAIL
harness_selftest     803 checks, 0 FAIL
shellcheck -S error -s bash   clean
each of the ten suites  green, recording, human output identical
ledger / budget        untouched

Refs #965

[OffgridwithJD]

Correction to my table above: the phase6 ledger column should read 4 rows, not 31. It has 4 records, so it merges 4 rows — 31 was a copy error from the unique_conc row two lines down. The records and run: columns were right, and the 236 total is right.

Flagging it rather than editing the comment in place, because a silently corrected number in a table someone is reviewing is worse than a visible one.

[jdatcmd]

Re-reviewed from scratch, because my approval does not cover this. I approved 626373b5 — two files, one suite, "review the shape, not the tenth one". The head is now 66507a92: eleven files, 271 insertions, all ten suites. That is a materially different change, and an approval that rides through a 5x scope expansion is not a review.

Everything checks out. Driven on PG16, both trees, all ten suites.

All ten, base against PR

SUITE          base rec   PR rec   PR run:   rc   human output
smoke                 0        9         9    0   identical
audit                 0       23        23    0   identical
concurrency           0        7         7    0   identical
phase2                0       42        42    0   identical
phase3                0       32        32    0   identical
phase4                0       29        29    0   identical
phase5                0       31        31    0   identical
phase6                0        4         4    0   identical
unique_conc           0       31        31    0   DIFFERS -- see below
update_conc           0       25        25    0   identical

checks run: equals the record count in every suite, so each of the ten would reconcile if the gate called the reconciler — same state you and I established for smoke.sh.

The one that differs is the silent failure you found, and it is an addition

10a11
> SKIP  2c citext case (citext extension not available)

base:  test/unique_conc.sh: line 392: check_skip: command not found     1 occurrence
PR:                                                                     0 occurrences

The human output does not change, it gains a line that should always have been there. Before, check_skip was undefined, the suite continued under set -uo pipefail, and the citext case was reported nowhere — not as a pass, not as a skip, not as an error. It now emits SKIP the 2c citext case / citext extension not available.

That is the most valuable thing in this PR and it is not the thing the PR is about. A suite calling a function that does not exist, surviving it, and reporting a clean pass is the exact shape #965 exists to make impossible, found by sourcing lib.sh for an unrelated reason.

Your residual claim, verified exactly

SUITE          human   recorded   UNRECORDED
audit             27         23            4
phase4            38         29            9
phase5            36         31            5
phase6            43          4           39
                                        ----
                                          57

57, and phase6's 39 is eq_on_off as you said — 40 call sites in the file. Your correction of the phase6 row to 4 was right too.

So #965's 293 is not fully addressed and the PR says so. No closing keyword on any of the three commits, which is the right call — #965 must stay open at 57, and it now has a precise remainder rather than a claim of completion.

One thing to fix before I merge

The title is stale, and it is the line that goes into main:

test: smoke.sh records its checks, so the ledger can see them (#965, 1 of 10)

Anyone later asking when the other nine landed will not find it. Retitle to something like "test: the ten suites that recorded nothing now record their checks (#965, 10 of 10)" — your CHANGELOG heading already says exactly that — and I will merge on the retitle. I would rather you name your own change than have me rename it.

A measurement for #965 and #432, not a blocker here

The check counts differ by major, in these very suites:

your PG18 total (the nine)   227      my PG16 total (all ten)   233 recorded + 57 unrecorded = 290
your PG18 census (#965)      293

Same suites, different majors, different totals. That is fk_referencing again, and it means the 293 in #965 is a PG18 number rather than a property of the suites. Worth a line in #965 so the remainder is not chased against the wrong denominator on a different box.

[OffgridwithJD]

Retitled: "the ten suites that recorded nothing now record their checks (#965, 10 of 10)". You were right to hold on it and right to ask me to do it rather than doing it for me — the title is what lands in main, and "1 of 10" would have sent anyone looking for the other nine to a commit that contains them.

And you were right to throw your approval away. I expanded the scope 5x under an approval granted for "review the shape, not the tenth one", and I should have said so explicitly in the push rather than leaving you to notice that eleven files had arrived under a two-file approval. An approval that rides through that is not a review.

Your PG16 numbers differ from my PG18 ones, and that is the finding

              my PG18    your PG16
audit              26           23
total recorded    236          233
total counted     293          290

So audit alone differs by three checks between majors, and the 293 in #965 is a PG18 number rather than a property of the suites. That is fk_referencing again, in the very suites being converted, and it matters for the remainder: whoever chases the last 57 needs to know the denominator moves.

I am putting that in #965 now rather than leaving it in a message.

It also strengthens the case for not seeding these, which neither of us had connected: these ten are not merely uncovered, they are uncovered and major-divergent, so they would have been among the worst candidates to seed from a single major.

On the citext find

Agreed it is worth more than what the PR is about, and I want to be accurate about how I found it: not by looking. I was reconciling a one-off discrepancy — unique_conc reported 31 records against 30 PASS lines — and chased the extra row because the arithmetic did not close. The command not found in the baseline was the answer. If the numbers had happened to match I would not have looked, which is an argument for reconciling every count that does not close rather than for my diligence.

Your hedging correction

"Over-caution reading as honesty is its own failure mode" is the sharpest line of the exchange, and it applies to me: I labelled the tab-and-newline arm as unable to fail and accepted your label without testing it either. You mutated _Record to carry a tab-joined line and reddened it directly. I should have done that when I first read the label instead of agreeing that it was decorative.

[OffgridwithJD]

I measured the per-major question instead of doing the arithmetic, and the answer is better than the one I gave you an hour ago. I had said "audit differs by three checks because some cases are conditional" — that was inference from your table and mine. Here it is driven, test/audit.sh at 66507a92 on both majors, each in its own build tree:

                                      PG18 (pg18a)   PG16 (pg16a)
checks run: / RESULT records                   26            23
human PASS lines                               31            27
  of those, from expect_error                   5             4
the server_version_num >= 170000 arm        taken     not taken
rc                                              0             0

One gate explains all of it. audit.sh:293 gates the partitioned-parent arm, and that block holds exactly 3 check calls and 1 expect_error. 3 + 1 = the 3-record and 1-unrecorded difference, nothing left over. Your measurement that audit is the only one of the ten that moves is what made that findable — nine suites agreeing across majors is what turns "the totals differ" into "this one block".

Two corrections to my own numbers

The PG18 remainder is 58, not 57. I took your 57 — a PG16 figure — and applied it to my PG18 census. The audit row is 5 on PG18 and 4 on PG16. a-borrowed-count-is-still-my-claim, for the second time this week.

The comparable PG16 census is 289, not 290. Your 290 is 233 recorded + 57, and the 233 includes the citext SKIP that has no PASS line in the base census. Not a consequential difference, but the whole point of the section is the denominator, so it should close exactly: 289 human lines, 232 of them recorded, plus 1 new SKIP = 233, remainder 57.

What the measurement found that neither of us was looking for

The two version gates in these ten suites treat their skipped arm differently, and one of them is the class you just praised me for finding by accident:

unique_conc.sh:546   else  check_skip "the NULLS NOT DISTINCT case" ...   -> a SKIP record
audit.sh:313         else  echo "-- PG16 refuses PARTITION BY ..."        -> nothing at all

So on PG16 the ledger gets 23 rows for audit with nothing saying an arm was gated out. The count just differs, and anyone reconciling it against a PG18 figure sees three checks that look lost. audit.sh:313 should call check_skip exactly as unique_conc.sh already does — same shape as the citext bug, found the same way, by a count that would not close.

That is queued into the remainder pass, not added to this PR. #965 now carries all of it: the per-major table, the gate as the mechanism, both corrections, and audit.sh:313 in the remainder list.

One more thing #969 fixed that I did not claim: unique_conc.sh:546's check_skip was also undefined before this PR. It never fired because nothing here runs PG14, so that one was a latent silent failure rather than an observed one — two of them in that file, not one.

Where this leaves the merge

Retitled as you asked. 13/13 at 66507a92. Nothing above changes a line of the PR.

[OffgridwithJD]

I moved the head you were about to merge, and you should know exactly what by before you re-check it. New head 19ddb4a3, one commit on top of 66507a92, CHANGELOG prose only — four lines, no code, no test, no behaviour.

 CHANGELOG.md | 7 ++++---

The paragraph said "Fifty-seven checks across four of the suites are still not recorded". Fifty-seven is your PG16 figure, not a PG18 one, and this entry's other numbers are all PG18. On PG18 it is fifty-eight. It now says both and names the reason.

I would normally not touch a head you have verified and are holding only on a title. I did it because the CHANGELOG is the permanent record: correcting it in a later entry leaves a reader of this entry with a wrong number and nothing pointing at the correction. A ten-minute CI re-run costs less than that.

The difference is the gate, measured on both majors

                                      PG18 (pg18a)   PG16 (pg16a)
checks run: / RESULT records                   26            23
human PASS lines                               31            27
  of those, from expect_error                   5             4
the server_version_num >= 170000 arm        taken     not taken

audit.sh:293 gates the partitioned-parent arm; that block holds exactly 3 check calls and 1 expect_error. The expect_error is the fifty-eighth check, and it does not exist on your box. Each arm ran in its own build tree — my first attempt reused one tree for both majors, installed a PG18-built .so into pg16a, and the postmaster would not start. I read that empty log as a PG16 result for a moment.

Re-checking this should be a diff read, not a re-run

git diff 66507a92..19ddb4a3 is four lines of prose in one paragraph. Everything you verified on PG16 — the ten suites, the 233 records, the citext SKIP, the checks run: equality — is untouched by it, and the files you drove are byte-identical between the two heads:

git diff --stat 66507a92..19ddb4a3 -- test/     (empty)

Still yours to merge, and still on your judgement whether the correction was worth the head move. If you would rather merge 66507a92 and take the number fix separately, say so and I will open it as its own PR instead — I will not push to this branch again either way.

[OffgridwithJD]

13/13 green at 19ddb4a3, the head with the CHANGELOG correction. git diff --stat 66507a92..19ddb4a3 -- test/ is empty, so nothing you drove on PG16 moved.

Ready when you are. The follow-up is open as #970 — the remaining 58, which takes all ten suites to recording every check they run.

[jdatcmd]

Approving 19ddb4a3, which is the head I am merging — a fresh approval rather than the stale one.

My first approval was at 626373b5 (one suite). GitHub still showed reviewDecision: APPROVED after the PR grew to eleven files, which is precisely why an approval state is not evidence that anyone read the thing being merged.

What changed since the head I verified

66507a92 -> 19ddb4a3   CHANGELOG.md only, 4 insertions, 3 deletions

No shell file moved, so the ten-suite run I posted above still describes this head. The change takes the per-major point:

fifty-eight on PG18 and fifty-seven on PG16, because one of them sits behind a version gate

That is the right correction and it matches my measurement from the other side — I counted 57 on PG16 against your 58 on PG18, and a difference of one behind a version gate is exactly what closes that gap. It also means #965's remainder is now stated as a per-major number rather than a bare one, which is the thing that would otherwise be chased against the wrong denominator on a different box.

Merging

Title now reads "the ten suites that recorded nothing now record their checks (#965, 10 of 10)", so the line landing in main describes the change. CI 13/13, ms=CLEAN, and the merge is gated on 19ddb4a3.

#965 stays open at 58, with no closing keyword on any of the four commits.