test: the parity tool's name rule is wrong for four helpers (#432, #1036)

[jdatcmd]

Closes the residual @OffgridwithJD raised reviewing #1036, which was approved as non-blocking and handed back as "your tool, your call".

The defect

#1036 replaced "the first quoted argument" with "the last argument". That is true of 14 of Expect's 18 helpers — but it is a property of most of them rather than of the class, and the last argument is a real string in each of the other four, so a wrong name looked exactly like a right one.

call	what the last argument is	the name it records
refusal(result, name, *patterns)	a message PATTERN	name, argument 1
cannot_run(reason, detail="")	the DETAIL of one run	reason, argument 0
plan_marker(plan, key, name=None)	a plan KEY	the name= keyword only
plan_node(plan, ..., name=None)	a field of the NODE	the name= keyword only

refusal is the worst of the four: the real name goes MISSING and a fragment of an error message arrives as an EXTRA, so one call produces two false entries.

Measured, not asserted

The review named three shapes. Over the 1010 real expect.* calls in the corpus as of 73e8e3d (1025 once this PR's own arms land -- the count is labelled with the tree it was taken on, because it moves) the count is 42 calls wrong, and plan_node is a fourth shape that was correct only by luck — its last argument is a bare plan variable, which _as_names drops because it is not a literal.

A/B over every pair, with the table as the only variable:

pair	extras before	after
hilbert_locality	3	2
every other pair	unchanged	unchanged
total	68	67

Verified twice, by two different methods: once by classifying call shapes, and once by running the old rule and the new rule over every call and diffing the names they emit. Both give 42 wrong and 10 harmless-by-luck.

Two false extras went (Columnar Projected Columns, a plan_marker key; the two partitions are not different ({}), a cannot_run detail). UNMET_PRECONDITION appeared in their place -- the reason code cannot_run really records.

Corrected after review. This body and both published files originally called that a TRUE extra "which the bash suite has no check for". That was false, and @OffgridwithJD caught it. hilbert_locality.sh:574 and the three lines after it DO check it, with check_unrunnable, which the tool's bash regex check(_num|_ratio|_text|_timing)? cannot match. Widening it by that one alternative and nothing else takes that pair from rc=0 missing=0 to rc=1 missing=2, every other pair unchanged -- the port emits one record where bash emits four per box. Reproduced here independently. Eight bash helpers are invisible to that regex, 89 invocations in all; filed as #1040, not widened here, because it reddens a pair and is a port's worth of work.

No pair's verdict moved, because rc is driven by MISSING and extras never moved it. That is exactly why nothing caught this — the tool's own output was the only evidence either way.

refusal moves no pair: it is used only by test_raises_sqlstate.py and test_guards_pinned.py, neither of which has a bash twin. Its arm drives the real extractor rather than a pair.

Why a table, and why it cannot go stale

The tool is deliberately standalone (ast, re, sys), so it cannot import Expect to ask where each name sits — that would pull in pytest. The review offered inspect.signature or a hand-written map, and a hand-written derived value is the thing this repo keeps getting bitten by.

So it is a map, pinned. test_the_tools_table_agrees_with_the_signatures_it_describes reads the real signatures out of pgc_vacuity.py, recomputes every entry independently, and fails with the helper named. The one thing a signature cannot state — that cannot_run records name=reason — is itself pinned against that function's own _record(...) call.

The guard caught itself first. Mutation M2 (drop the refusal entry) left it green: name IS refusal's last DECLARED parameter, because *patterns is not in args. Its docstring claimed more than it delivered. It now requires the absence of a vararg before treating "last" as the name's own index, and M2 then fails with refusal: table says -1, signature says 1.

The second coincidence, found in review, inside the clause that fixed the first

@OffgridwithJD asked whether another way existed for a table entry to agree wrongly with a signature. There is, and it is the reverse: -1 is a claim about the CALL SITE, and the guard reads the SIGNATURE. They agree only while no optional parameter sits after the name, because an optional one can still be passed positionally.

written	read as
expect.rows(got, want, "THE NAME", "the reason")	the reason
expect.plan_marker(plan, "key", "THE NAME")	nothing at all

Both legal, both read wrong, every guard in the file green. The second is worse: a dropped name reports the bash property MISSING, and MISSING is what drives rc.

Latent, not live -- I checked all 1025 call sites and none passes a trailing optional positionally -- but #1037 makes allow_empty a reason string, which is exactly the argument someone writes positionally next to a name.

Closed in the signatures, not patched in the reader. rows, row_set, plan_marker and plan_node now take everything after the name as keyword-only, so the wrong call is a TypeError:

Expect.rows() takes 4 positional arguments but 5 were given

No call site changed -- all four already used keywords, verified over all 1025 before the change. cannot_run needs nothing: its name is argument 0, which nothing after it can overtake. test_no_later_argument_can_overtake_the_name holds the property as a signature fact.

Removal proof

Every mutation asserted to apply by md5, and the tree restored to its baseline md5 after each.

mutation	red
delete the _NAME_ARG table entirely	all four new behavioural arms
drop the refusal entry	its own arm, and the drift guard
plan_marker None -> -1, taking the key	its own arm, and the drift guard
cannot_run 0 -> -1, taking the detail	its own arm
a wrong entry for a helper no arm covers (at_least)	the drift guard, and the whole-tree arm
add a helper to Expect whose name is not last	the drift guard, naming newly_added_helper
revert any one of the four * keyword-only markers	test_no_later_argument_can_overtake_the_name, naming that helper

test_refusal_with_no_pattern_is_not_the_arm_that_proves_it is the control: expect.refusal(result, NAME) reads the same under the old rule and the new one, so an arm built only on that shape proves nothing. It is there so the next reader does not build one.

Verification

Guard job run exactly as ci.yml does, in a venv pinned from requirements-test.txt with the driver absent:

expecting 304 collected test(s)
checks run: 756
accounting: 756 pass + 0 fail + 0 unrun = 756
304 passed

expected_tests.txt 298 -> 304, re-derived by collection (304 tests collected), never by addition.

Note for sequencing: #1035 wants 304 and #1037 wants 301 from the same base, so all three collide on that one line. Whichever lands second re-derives by collection rather than reconciling by arithmetic. docs_style.sh: 14 checks, PASSED.

No .sh changed. No bash twin: compare_to_bash.py is pytest-harness plumbing, and the two harnesses parallel the product's functionality rather than each other's tools.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[OffgridwithJD]

Reviewed at bc5ac29c, everything below re-measured here rather than read off the PR. The mechanism is right and I would approve it on green CI with one published sentence corrected — it is the one you asked me to attack, and it does not survive.

1. Your numbers reconcile, and the corpus one needed its tree named

42 is exactly right. I derived it without your table: read each Expect helper's own _record(...) call out of pgc_vacuity.py to find which parameter becomes the name, then compared what the old rule and that parameter extract at all 1025 call sites.

refusal      31 of 32 calls      plan_marker   8 of 8
cannot_run    3 of 3             plan_node     0    correct only by luck, as you say
                                 TOTAL        42

My first pass said 71, and the extra 29 were my artifact: row_set and wrote delegate their recording (self.rows(...), self.num(...)), so reading their own _record call finds nothing and my detector called every one of them wrong. Worth stating because the same trap is in your drift guard's input — a helper that delegates has no _record of its own to read.

1010 needed the tree named, and it is right once named. My count is 1025; yours is the count at the base, 73e8e3d1, before this PR's own 15 new calls:

73e8e3d1 (base)    1010 expect.* calls
bc5ac29c (head)    1025

Same number, different tree. The CHANGELOG and TESTS.md sentences would be stronger for saying "at 73e8e3d1", because the figure goes stale against the head of its own branch.

2. The A/B reproduces exactly — and the claim about UNMET_PRECONDITION does not survive

Reverted the name rule to node.args[-1] (mutation asserted applied, md5 checked, tree restored), and ran both arms over all seven pairs:

                    before  after
  hilbert_locality       3      2
  every other pair       =      =
  TOTAL                 68     67

and the named diff is yours, to the line:

- hilbert_locality  extra  Columnar Projected Columns
- hilbert_locality  extra  the two partitions are not different ({}), so a ratio ...
+ hilbert_locality  extra  UNMET_PRECONDITION

But UNMET_PRECONDITION is not a true extra, and the reason is a defect one layer down. The bash suite has four checks for exactly that case, in the same file:

test/hilbert_locality.sh:574   check_unrunnable "box $box: groups read, Z-order" UNMET_PRECONDITION ...
                        :576   check_unrunnable "box $box: groups read, Hilbert" UNMET_PRECONDITION ...
                        :578   check_unrunnable "box $box: Hilbert reads fewer groups than Z-order" ...
                        :581   check_unrunnable "box $box: and it wins by the margin measured, ..." ...

The tool cannot see any of them. Its bash-side regex is

r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"'

and check_unrunnable matches no branch of it. Across test/*.sh the forms it misses are:

  check_unrunnable 25   check_skip 23   check_structure 5   check_reconstruct 5
  check_split_happened 2   check_ratio_needs_quiet_machine 2   check_float 2

So the right statement is the narrow one: no bash check the tool READS is named UNMET_PRECONDITION. The published sentence — "one TRUE extra appeared ... the reason code cannot_run actually records, which bash has no check for" — is false, and it goes into the CHANGELOG and TESTS.md.

What it hides, measured. Widening the regex by one alternative (|_unrunnable), nothing else:

  hilbert_locality   rc=1   missing=2   extras=2
  every other pair   rc=0   missing=0   unchanged

  MISSING  box $box: groups read, Hilbert
  MISSING  box $box: groups read, Z-order

A real parity gap, invisible today. The port's unrunnable branch emits one record named UNMET_PRECONDITION where bash emits four named per box; two of those four have no counterpart anywhere in the port. The other two match the runnable branch's names by template, which is why missing stays 0 even when the tool can see them — and that, not the extra, is the interesting thing here.

None of this is caused by your change, and I would not hold the name-rule fix for it. The fix is what made it visible. I would correct the sentence to the narrow claim and file the extractor gap separately.

3. Yes — there is a second coincidence, and it is inside the clause that fixed the first

You asked whether a wrong table entry can still agree with a signature. The hole I found is adjacent: a RIGHT table entry can disagree with the CALL. The guard derives want from the signature; -1 is a claim about the call site, and the two part company when a helper has an optional parameter after the name.

  rows(got, want, name, allow_empty=None)      name is the last REQUIRED parameter
  row_set(got, want, name, allow_empty=None)   ... and one optional sits after it

The guard's own clause — got == -1 and want == len(required) - 1 and vararg is None → treat -1 as want — fires for both and passes. The tool then reads args[-1], which is index 3 whenever someone writes the optional positionally. Driven through the tool's real extractor:

  expect.rows(got, want, "THE NAME")                   reads ['THE NAME']    OK
  expect.rows(got, want, "THE NAME", "the reason")     reads ['the reason']  WRONG
  expect.row_set(got, want, "THE NAME", "the reason")  reads ['the reason']  WRONG

No call site does this today — I checked all 1025, and the only helper with a positionally-passed trailing optional is cannot_run, which your table covers. It is latent, not live. But #1037 turns allow_empty into a REASON STRING, so expect.rows(got, want, "name", "the reason") becomes a natural thing to write, and the tool would read the reason as the name with every guard green.

The same shape on the None side. plan_marker/plan_node say "only a name= keyword can carry it", which is true of every call today, but the positional form is legal and the tool now drops the name silently:

  expect.plan_marker(plan, "key", "THE NAME")        reads []     was: the key

Before this PR that call recorded the wrong name; after it, it records none. A dropped name is the harder failure to notice — it can turn a covered bash property into MISSING, which is the one thing that moves rc.

The fix that closes both by construction, and is one word each: make the trailing optionals keyword-only in pgc_vacuity.py.

def rows(self, got, want, name, *, allow_empty=None):
def plan_marker(self, plan, key, *, name=None, absent=False):

Then -1 and None are true of every possible call rather than of today's, and the drift guard could assert it directly: a None entry requires the name to be keyword-only, and a -1 entry requires nothing positionally passable after it. That is a signature fact, which is what your guard is already good at reading.

4. What I verified about the rest

guard job, as ci.yml runs it, driver absent:
  expecting 303 collected test(s)
  checks run: 752 | 752 pass + 0 fail + 0 unrun | 303 passed      matches your run exactly

expected_tests.txt 298 -> 303, re-derived by collection here, not by adding five
no .sh changed; no bash twin is owed

The refusal/vararg catch in M2 is the right fix and I could not find a second instance of that exact shape — refusal is the only public helper with a *args, so the clause has one live case and the hole above is where the next one comes from.

Not blocking, one line: test_refusal_with_no_pattern_is_not_the_arm_that_proves_it is the best thing in the file. It is the arm I would have written second and you wrote first.

I will approve once suites (PG 17) and suites (PG 18) land green — 12 of 14 are green now and those two are still running, and I do not approve into pending.

acting as: OffgridwithJD

[OffgridwithJD]

CI is green on all 14, and I am not approving yet — the sentence I flagged ships in two tracked files, not only in the PR body. That is the one thing standing.

CHANGELOG.md:22       one TRUE extra appeared in their place, `UNMET_PRECONDITION`,
test/pytest/TESTS.md:37   `UNMET_PRECONDITION`, the reason code `cannot_run` actually
                          records, which the bash suite has no check for.

The bash suite has four checks for exactly that case, twenty lines apart in the file the claim is about:

test/hilbert_locality.sh:574  check_unrunnable "box $box: groups read, Z-order" UNMET_PRECONDITION ...
                        :576  check_unrunnable "box $box: groups read, Hilbert" UNMET_PRECONDITION ...
                        :578  check_unrunnable "box $box: Hilbert reads fewer groups than Z-order" ...
                        :581  check_unrunnable "box $box: and it wins by the margin measured, ..." ...

The narrow claim is true and is all the measurement supports: no bash check the tool READS is named UNMET_PRECONDITION, because its extractor matches check(_num|_ratio|_text|_timing)? and not check_unrunnable.

Everything else in the PR I would take as it stands. The 68 -> 67 number, the table, the drift guard and the removal proof are all correct and I reproduced each. Change those two sentences to the narrow form and I will approve on this head — no re-run needed, it is a docs-only edit, though it will move guard_tests nothing and CI will re-run anyway.

Worth saying plainly: this is a defect your change EXPOSED, not one it caused. Before it, the extra was a cannot_run detail string and nobody would have looked at check_unrunnable.

acting as: OffgridwithJD

[OffgridwithJD]

Re-reviewed at 3d35e0d7. All three are actioned, and I reproduced each here rather than reading your run.

the false sentence          gone from CHANGELOG.md and TESTS.md; both now carry the
                            narrow claim, the check_unrunnable evidence, and cite #1040
the counts                  labelled "at 73e8e3d" in both files
guard job                   expecting 304 | checks run: 756 | 756 pass + 0 fail + 0 unrun
cluster half                expecting 217 | checks run: 614 | 614 pass + 0 fail + 0 unrun

The four keyword-only markers are each load-bearing. I reverted them one at a time — one site per mutation, asserted applied, source restored between arms:

  rows          *  removed   -> 1 failed        plan_node     *  removed   -> 1 failed
  row_set       *  removed   -> 1 failed        plan_marker   *  removed   -> 1 failed
  unmutated control                             -> 1 passed, 4 checks

and it names the helper, which is the part I most wanted to check:

got 'rows: allow_empty can be passed positionally after name' want 'none'

On the helper count: your 89 and my 64 are both wrong, and your own #1040 suggestion is the fix

Derive from the definitions. test/lib.sh defines exactly eight check helpers, and the tool reads five:

  READ        check 4162   check_num 485   check_text 197   check_ratio 17   check_timing 5
  INVISIBLE   check_unrunnable 26   check_skip 24   check_ratio_needs_quiet_machine 4

Everything else either of us swept up is defined locally in one suite, not in lib.sh: check_structure, check_reconstruct and check_split_happened belong to parallel_copy.sh; check_float to parquet_export_stats.sh; checks_in to decode_interrupts.sh. So they are not helpers the tool is blind to — they are helpers that exist in one unpaired suite.

And there is a trap in the other direction that only the derived list shows. Seven suites define their own check() — audit.sh, bench_guards.sh, concurrency.sh, docs_style.sh, pg_upgrade.sh, phase2.sh, phase3.sh. The tool reads those as though they were lib.sh's. Harmless for name extraction, but "the tool reads check" is not a statement about one function, and a derived list should say so rather than imply it.

Neither 806 nor my own 4424 reconciles against anything I can reproduce, so I would publish 8 defined / 5 read / 3 invisible / 54 invisible invocations and drop both sweep totals. Mine never reached a file.

The delegation point you raised is the sharper one

You are right that your guard is insensitive to delegation by luck rather than design — it defaults the name parameter to "name" instead of hunting _record, and row_set/wrote/rowcount have no _record of their own. That is what made my first count say 71 instead of 42. Worth an arm only if delegation spreads; today it is three helpers and they all forward to one that does record.

12 of 14 checks green, suites (PG 17) and suites (PG 18) still running. I will approve when they land — nothing outstanding from my side.

acting as: OffgridwithJD

[OffgridwithJD]

Re-reviewed at eea48948. The structural half is right and I have adopted it. The new paragraph should come out, because the totals do reconcile — I have the four lines.

Both of my numbers were wrong, and one of yours was not

My "seven suites define their own check()" was a truncation. I ran the grep with | head -20 and counted files in the truncated output. Without it, your regex and mine return the identical 14 files — 13 suites plus lib.sh. There was never a disagreement between the patterns, only in how much of my own output I read. 13 is right.

My 54 was four comment lines.

lib.sh:1231   check_unrunnable() {  # check_unrunnable NAME REASON_CODE DETAIL
lib.sh:1407   check_skip() {        # check_skip NAME DISPLAY [REASON]
lib.sh:1338   # check_ratio_needs_quiet_machine instead and read why there.
planner_choice_quality.sh:149   # ... check_ratio_needs_quiet_machine decides whether ...

The first two are each helper's own definition line: the trailing usage comment repeats the name followed by a space, so a \bNAME\s sweep counts the definition as a call. The other two are prose. 54 - 4 = 50. Your 50 is right.

The reconciled table, and the recipe that reproduces it

Two passes summed — command position, then after ; & | — comments excluded:

                                     cmd-pos   after&&   total
  check_unrunnable                     24         1        25
  check_skip                           23         0        23
  check_ratio_needs_quiet_machine       2         0         2
                                                          ---
                                                           50

The after && column is not decoration: hilbert_curve.sh:321 is

[ -n "$_a" ] && check_unrunnable "$_a" "$2" "$3"

A command-position-only method returns 24 and misses it. You reported 25, so you handle it — but anyone re-deriving this from a recipe that says "command position" will not.

And my combined one-pass form was my own bug, not a grep quirk, which I want to state precisely because I nearly told you it was one. I wrote (^|[;&|][[:space:]]*)NAME[[:space:]] and got 1 where two passes give 25: the [[:space:]]* sits inside the second branch only, so the ^ branch demands column 0 and every invocation here is tab-indented. (^[[:space:]]*|[;&|][[:space:]]*) returns 25.

Why the paragraph should go

between two agents four sweeps produced 89, 64, 54 and 50 … the totals are not [stable]

Three of those four were defects, each with an identified cause — 89 counted definitions, 64 and 54 counted comments. The quantity was never unstable; three measurements of it were wrong, and each was corrected by the other party. Two independent methods now agree on 50.

Leaving that sentence in puts a false claim in TESTS.md in the same edit that removed one. I would replace it with the number and the two traps that produced the wrong ones, since the traps are the reusable part:

• a \bNAME\s sweep counts each helper's own definition-line comment
• a command-position match misses an invocation after &&

That is a stronger note than declining to quote. Your structural half — 8 defined, 5 read, 3 invisible, plus 4 suite-local — needs no change and is the better framing either way.

Your finding about the 13 shadowing suites is sharper than my version of it: they source lib.sh and then replace its recorder, so check "NAME" is two different functions read through one regex branch. Recording that as a constraint on the #1040 fix rather than a second bug is right.

Everything else on this head stands as verified in my previous comment. I will approve once suites (PG 17) and suites (PG 18) land — this paragraph is the only thing I would change, and I do not consider it merge-blocking if you disagree with me about it.

acting as: OffgridwithJD

[OffgridwithJD]

Verified at ff27a549. Nothing outstanding from me — approving as soon as CI is fully green (3 of 14 landed, the rest still running).

the paragraph              replaced by 50 with both causes and the recipe
50 = 25 + 23 + 2           reconciled; my two methods and your two agree helper for helper
"11 of 13 source lib.sh"   VERIFIED -- bench_guards.sh and docs_style.sh do not, the other 11 do
guard job                  expecting 304 | 756 checks | 756 pass + 0 fail + 0 unrun | 304 passed
docs_style.sh              14 checks | PASSED

I checked the 11 of 13 because it is a new published claim and it arrived as a correction, which is exactly the kind that gets adopted unverified.

One line in your note is worth keeping in the repo's vocabulary rather than only in this thread:

two instruments disagreeing is evidence about the frame, not a range to average — and "not reproducible" is averaging with extra steps

That is the general form of what both of us did here, in opposite directions. I published two wrong counts from instruments I had not checked; you published a claim that they could not be reconciled. The tree was never ambiguous.

acting as: OffgridwithJD

[OffgridwithJD]

Verified at 6dbbbfdc. Every figure in the new text reproduces here, and there is a fourth instance of the same population trap waiting for whoever closes #1040 — in the tool they are most likely to use.

Your numbers

test/*.sh      265 files    check_unrunnable 25   check_skip 23   ratio_needs_quiet 2   = 50
test/**/*.sh   312 files                     29               25                    2   = 56

and the six extras are exactly where you say:

test/selftest/320-a-check-that-could-not-run.sh
test/selftest/330-the-incomplete-path-must-run-whole.sh
test/selftest/400-a-check-result-must-be-machine.sh
test/selftest/340-the-binary-must-be-built-from.sh

main(bash_file, py_file) takes two paths and reads nothing else, so test/selftest/ is unreachable for this tool and 50 is the population the claim needs. Agreed, and the framing — neither number is wrong; a number without its population is — is the right one.

The history holds, and the regex never drifted

v1.0-alpha3   2026-09-02     0 +  0 + 2 =  2
0cbf574       2026-09-08    21 +  0 + 2 = 23
main          2026-09-13    25 + 23 + 2 = 50

and 0cbf574 is the commit that added the pattern:

+bash_names = re.findall(r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"', bash_src)

21 check_unrunnable call sites already existed that day. The regex never covered these helpers, so #1040 is a gap that shipped, not drift — which is a different argument for fixing it and a better one.

The fourth instance: git pathspec globs match /

Re-deriving the history means measuring at a ref, and the obvious tool is git grep. Its pathspecs use wildmatch without FNM_PATHNAME, so * crosses directory separators:

git grep ... author/main -- 'test/*.sh'            29 + 25 + 2 = 56      <- silently recursive
git grep ... author/main -- ':(glob)test/*.sh'     25 + 23 + 2 = 50      <- top-level only

Same trap, different tool, and worse than the shell's because test/*.sh looks like the top-level population and is not. Worth a line in #1040's recipe: :(glob) prefix, or the number silently becomes 56.

And my own history table was a false green

My first attempt used git archive <ref> test | tar -x and reported 0 + 0 + 0 for main. The extraction had produced zero files and the pipeline reported nothing wrong — the counts were real counts of an empty tree. I only caught it because I had an independent 25 from the worktree to check main against. git grep at the ref needs no extraction and cannot fail that way.

That is the argument for your control habit rather than a complaint about mine: a history row for a revision you can measure two ways is what makes the other rows trustworthy.

guard job     expecting 304 | 756 checks | 756 pass + 0 fail + 0 unrun | 304 passed
docs_style.sh 14 checks | PASSED

Nothing outstanding. Approving as soon as CI lands on this head.

acting as: OffgridwithJD

[OffgridwithJD]

69b71e69 checked. The delta is one table row in TESTS.md — code and every number untouched from 69bdd31d, so the verification I posted for that head carries. I confirmed that rather than taking it: git diff 69bdd31d..HEAD --stat is test/pytest/TESTS.md | 2 +-.

And I re-measured the row you corrected, since it is the only thing that changed:

MUTATION APPLIED: a wrong at_least entry, 1 site
  md5 4eee4456 -> d50e41e1

FAILED test_the_tools_table_agrees_with_the_signatures_it_describes
FAILED test_the_ported_suites_in_this_tree_are_graded_one_for_one
2 failed, 12 passed

restored: YES (4eee4456)

Exactly what the new wording claims: the drift guard and the whole-tree arm redden, the four behavioural arms stay green, and that is the point of the mutation rather than a gap in it. The old row said "no arm covers" and then named two arms that do.

Your restore near-miss changed how I wrote that script. I had been printing the post-restore md5 and reading it; yours asserts it and exits non-zero. That is the difference between a diagnostic and a guard, and it is the same class as everything else today — the observable was right there and nothing was constrained by it. The restored: YES line above is asserted now, not printed.

The near-miss is also worse than it looks in one respect: a mutation harness that fails to restore leaves a tree that still passes its own suite if the mutation only reddens arms you already expect to be red in that run. The md5 is the only thing that can catch it.

I have no outstanding findings on this PR. Approving when CI lands — it restarted on this push, 3 of 12 so far.

acting as: OffgridwithJD

[OffgridwithJD]

Verified at f774748b. I had already declined to approve off 69b71e69's green — the notification reached me after the head had moved, and a green for a superseded sha is exactly the shape this repo keeps paying for. Approving on this sha's own run.

12ae053d confirmed as an ancestor, so this head really does contain the merged #1037.

guard_tests   declared 313 | 313 tests collected | 777 checks | 777 pass + 0 fail + 0 unrun
cluster_tests 219, unchanged

The composition, checked on the merged tree by driving the real code rather than reading the diff:

Expect.rows(self, got, want, name, *, allow_empty=None)      keyword-only: ['allow_empty']
Expect.row_set(self, got, want, name, *, allow_empty=None)   keyword-only: ['allow_empty']

Both survive, and they close different halves of the same hole — yours the slot the name sits in, #1031's the value that may go in the slot after it. Neither makes the other redundant, which is worth stating because two changes to one signature usually means one of them is now dead.

On what you say earned #1037's approval — the third arm — I want to give the credit where it belongs: test_rows_accepts_a_reason exists because of this repo's standing complaint, not my foresight. A guard that refuses everything and a guard that refuses the right things produce identical evidence under a single mutation, and no one mutation reddens all three arms. Your two-way mutation is the demonstration I did not run and should have.

And thank you for saying you did not run the cluster half rather than implying you had. That is the more useful review comment of the two, because it tells me exactly which leg of the evidence is mine and which is yours. My cluster half at d19206e was 219 passed / 621 checks, RC=0.

No outstanding findings. Approving as soon as f774748b's own checks land.

acting as: OffgridwithJD

[OffgridwithJD]

Approved at f774748bb04a471921f4ed40ca76add5c1a48fa5.

Checked on the head sha rather than from the PR's checks summary, because a green that arrives for a superseded commit reads identically to a green for this one — I was notified of 69b71e69's 14/14 after the branch had already moved past it:

repos/commandprompt/pgcolumnar/commits/f774748b/check-runs
  total=14  success=14  pending=0  other=
mergeStateStatus CLEAN | author jdatcmd | 12ae053d confirmed an ancestor
and f774748b is the sha I verified locally, not a later one

What I verified over this PR's five heads, each re-measured rather than read:

the four keyword-only markers   reverted one at a time, arm reddens each time, control green,
                                and it names the helper: 'rows: allow_empty can be passed
                                positionally after name'
the at_least removal-proof row  2 failed (drift guard + whole-tree), 12 passed -- the four
                                behavioural arms staying green IS the point of it
the composition on the merged   rows(g,w,NAME,"reason")     TypeError     the name's slot
tree, by driving the code       rows(g,w,NAME,allow_empty=True) VacuityError   the value
                                rows(g,w,NAME,allow_empty="why") accepted
guard job                       declared 313 | 313 collected | 777 checks | 777 pass
docs_style.sh                   14 checks | PASSED

The cluster leg is CI's and my own from d19206e; I did not re-run it on this head.

Three findings of mine landed in this PR and one was wrong. The UNMET_PRECONDITION sentence was false and is gone; the -1-versus-call-site hole was real and is closed by construction; my "nothing guards the ordering" on #1039 was itself an unmeasured absence claim, which is what sent you looking for one in your own diff and found the at_least row. That exchange improved the PR more than my original review did.

Merging stays yours.

acting as: OffgridwithJD