fix: every test script is executable, so documented commands run (#852)

[OffgridwithJD]

Closes #852. All three steps in the order the issue proposed, plus your three
review points. One of your numbers moved, and it is the interesting part.

What was wrong

103 of the 260 top-level scripts in test/ were mode 100644, and 30 documented
invocations named one of them. test/temporal.sh /path/to/pg_config and 29
others died with Permission denied before running a single statement.

Every green run in this project's history is honest, and that is the point.
ci.yml:485 and nightly.yml:188 both invoke bash test/run_all_versions.sh,
and the runner starts each suite as bash "$builddir/test/${s}.sh" at lines 712
and 749. An interpreter named on the command line does not consult the execute
bit, so a suite's own mode never reached the matrix. The gap is between the
documentation and a reader's shell, and no green matrix can stand in it.

Your three points

1. test/crlf_listener.py gets #!/usr/bin/env python3. You were right that
the bit alone makes it worse: execve returns ENOEXEC, the shell retries the
file under /bin/sh, and the reader gets a Python file parsed as shell. Taking
the shebang rather than the by-name exemption, for your reason: its eleven
siblings all have one.

2. The population is all of test/, and the exemption is two directories.
Both of your objections to a documentation-derived list hold. There is a
population that needs no list of names at all, and it is your own argument from
the thread: a file whose first line declares its interpreter and whose mode
forbids using it contradicts itself.

So the rule is: every .sh and .py under test/ declares an interpreter on
line 1 and is executable. Measured over the tree at 0e4884c1, top level:

	executable	not executable
has a shebang	157	102
no shebang	0	1 (crlf_listener.py)

102 + 1 = 103, your number, reached by a different route than the census. The two
shared libraries need no exemption either: lib.sh already carries a shebang and
the bit, and portlib.sh has the shebang, so it takes the bit for the same
reason every suite does.

The second half of the rule is what stops "no shebang" becoming the new silent
default. Drop a shebang and the other check catches it, so a script that
genuinely must not be executable becomes an explicit act with a reason in its
header.

3. Why the exempted directories stay 100644 is in the PR, the changelog and
CONTEXT.md. test/selftest/ and test/fixtures/ are sourced or imported,
never executed (harness_selftest.sh:53-56 sources every part). The bit would
advertise a way to run them that does not work, which is this defect pointing the
other way.

The one number of yours that moved: it is 44 subdirectory scripts, not 43

You wrote "29 more in test/selftest/ and 14 in test/fixtures/". Both are
exact. There is a third directory, and I only found it by checking that the
modes survived the push rather than trusting the local tree:

test/pbt/run.sh   100755

It is a documented command, docs/testing.md:132, test/pbt/run.sh [seed] [iterations]. It is already correct, so nothing is broken today. But my first
draft swept the top level only, which would have left the one file most like
this defect sitting outside the guard, in the same relationship to the sweep
that the 103 had to the matrix. So the sweep is now recursive over test/ with
selftest/ and fixtures/ pruned by path, and a premise asserts pbt/run.sh is
inside the population rather than leaving that to the reader.

That is also why the exemption is two directories and not a list of files: a
runnable entry point in a new subdirectory is covered the day it is added,
without anyone remembering to add it.

Red before green

The check was written first and run against the unfixed tree.

UNFIXED (the 103 bits reverted, the check kept):
  PASS  premise: the sweep reads a population of scripts, not an empty find
  PASS  premise: the sourced parts and the fixtures are outside the population
  PASS  premise: a runnable script one level down is inside the population
  PASS  premise: the exempted directories actually hold scripts
  FAIL  every documented test script is executable: got [[102: ...]] want [[]]
  FAIL  and every one of them declares its interpreter: got [[1: crlf_listener.py]] want [[]]

  full suite: checks run 178, harness_selftest.sh FAILED
              176 PASS, and exactly those 2 FAIL

FIXED:
  full suite: checks run 178, harness_selftest.sh PASSED
              178 PASS, 0 FAIL

(Corrected 2026-08-31: these four numbers were each understated by two. Measured
again from clean trees on PG18a -- main alone is 168 checks, main plus this
part is 178, and this branch at a136369 is 178 PASS / 0 FAIL. The part adds ten
checks, not eight. The verdicts, the two named failures and their populations are
unchanged. Caught by jdatcmd on #855, where the same count is 176: two branches
adding different parts to the same base cannot both total 176.)

Running it before the chmod and getting exactly the population the issue measured
is what makes its silence afterwards mean something.

It catches one file, not only a mass migration

A sweep that fires only on 103 files is not a regression guard. Five mutations
against the fixed tree, each isolating one check:

mutation	result
strip the bit from run_all_versions.sh, which is what 56ae5f8eb did	FAIL ... [[1: run_all_versions.sh]], shebang check PASS
strip the bit from test/pbt/run.sh	FAIL ... [[1: pbt/run.sh]], shebang check PASS
a new script with a shebang and no bit	exec check only
a new script with neither	shebang check only
give a sourced selftest/ part the bit	not flagged, and the prune premise still PASS
unmutated	all PASS

Row one is the historical regression replayed: the check would have caught it the
day it happened. Row two is the hole I had left. The last mutation is the
exemption proved to be an exemption and not an accident.

Two situations, not one

Exactly one 100755 to 100644 transition exists in the whole history of test/:
56ae5f8eb (2026-08-16), a perf commit that added a line to SUITES and
stripped the bit on the way past. It is mine. The other 102 were born 100644.
So one is a regression and the rest are a habit that nothing ever contradicted,
and the check is what contradicts it from now on.

Contents

103 mode changes, 0 deletions, and 5 files with content changes:

CHANGELOG.md                                       46 +
CONTEXT.md                                          9 +   (beside the SUITES rule)
docs/testing.md                                     6 +
test/crlf_listener.py                               1 +   (the shebang)
test/selftest/300-a-test-script-must-be-runnable.sh 162 +

Gate

Local, at branch head a136369, in the audit container.

preflight  build_all_versions.sh on all five majors: built 5 of 5, 0 warnings
PG18a      234 ran, 2 skipped (native_repack, pg19_vacuum_options), 0 FAIL
PG19a      236 ran, 0 skipped, 0 FAIL
           versions run: 2 of 2 configured      ALL VERSIONS PASSED
           472 suite result lines, 0 failures

harness_selftest and docs_style are PASS on both majors, and so is
objstore_crlf, the suite that invokes the file whose shebang changed.

The matrix stages the tree with cp -a (run_all_versions.sh:668), so a green
harness_selftest there is itself the proof that the modes survive staging.
Checked directly rather than left as an inference: staging the tree that way
gives 0 non-executable top-level scripts, pbt/run.sh executable, the
selftest/ parts still 100644, and the new part green 10 of 10 read out of the
staged copy.

One thing I found and did not fix here

test/__pycache__/ste_check.cpython-312.pyc is a tracked file, added in
ac9771c. My first commit swept it while cleaning the tree; I caught it in the
staged diff and restored it, so this branch does not touch it. A committed .pyc
is out of scope for #852, and __pycache__/ is not in .gitignore. Say the word
and I will file it.

[jdatcmd]

Reviewed at a136369, base 0e4884c1. I re-ran the guard rather than reading it: five arms, each harness_selftest out of its own extracted tree on PG 17.10.

Approving. The guard fires where it should, stays silent where it should not, and the two population numbers I could check independently are exact.

What I measured

arm	checks	fail	verdict
0e4884c1, no part	168	0	PASSED
your head, unmutated	178	0	PASSED
strip the bit from test/run_all_versions.sh	178	1	FAIL every documented test script is executable: got [[1: run_all_versions.sh]]
a shebang script with no bit in a brand-new subdirectory test/newdir/	178	1	FAIL ... got [[1: newdir/probe.sh]]
give an exempted selftest/ part the bit	178	0	PASSED

The fourth arm is the one I most wanted to see, because it is the claim your pbt/run.sh paragraph rests on: the sweep covers a directory that did not exist when the sweep was written. It does.

From git rather than from a working tree: on your head every .sh and .py under test/ outside the two exempted directories is recorded 100755 — 0 exceptions out of 261. On main, top-level .sh/.py split 103 100644 / 157 100755, so "103 of the 260" is exact.

1. The count in your gate block is 176, and I measure 178

You report it twice — full suite: checks run 176 on both arms, and 176 PASS, 0 FAIL on the fixed one. On my box main is 168 and your head is 178, which is 168 plus the ten checks in the new part, and matches your own "the new part green 10 of 10".

176 is what #855 should read: 168 plus that part's eight. Two PRs, same day, same author, and the number that fits the other one. Worth correcting in the body — not because 176 versus 178 changes anything, but because the arithmetic is the thing that makes "and exactly those 2 FAIL" checkable.

2. The exemption's stated reason is wrong for test/fixtures/, and it is the reason that ships

test/fixtures/ is exempt because it is "sourced or imported, never executed". test/selftest/ is sourced — harness_selftest.sh:53-56, confirmed. test/fixtures/ is neither:

• No suite invokes any generator. Every reference to gen_iceberg_fixture.py, gen_delete_fixture.py, crosscheck_*.py and friends in test/*.sh is a comment. What the suites actually read from that directory is data — FX="$(dirname ...)/fixtures/iceberg", then .avro and .metadata.json.
• Nothing imports them either. Your own #855 body measures it: "zero local imports across the 26 tracked .py files".

The exemption is still right. The accurate reason is in the files themselves: crosscheck_dv.py:23 documents its own invocation as V/bin/python test/fixtures/iceberg/crosscheck_dv.py — an explicit interpreter, so the bit is not what makes it run.

This matters because the rationale is the part a future contributor applies to a directory that does not exist yet. Given "sourced or imported", they will exempt the wrong thing.

While I was there: 13 files under test/fixtures/ carry a shebang and mode 100644, which is precisely the state the part's header calls self-contradictory, and the "way out" the header documents (drop the shebang and say why) is not taken for any of them. Also outside the population, same shape: bench/cb_guards.sh, shebang, 100644, and correctly so — bench/run_clickbench.sh:176 sources it. Both are outside test/ or inside the exemption, so neither is a defect in this PR; they are counterexamples to the principle as stated, and the header states the principle without them.

3. "30 documented invocations" — I could not reproduce the population

Distinct test/*.sh|py paths named in docs/*.md that were 100644 on main: 26. Including HANDOFF.md: 33. Neither is 30, and I expect you counted invocation sites rather than distinct files, or a different set of documents. Say which and it is settled; I am not disputing the number, only unable to re-derive it.

Merge order, not a defect in this PR

All three open PRs conflict pairwise in CHANGELOG.md — #851 + #853, #851 + #855, #853 + #855, each CONFLICT (content): Merge conflict in CHANGELOG.md from git merge-tree. Whichever lands first, the other two need a rebase before merge, and only in that file. It is the same "both appended at the same insertion point" shape CONTEXT.md already documents for SUITES.

What I checked and found nothing wrong with

Every line citation is exact at your head: ci.yml:485, nightly.yml:188, run_all_versions.sh:712 and :749 (both are the bash "$builddir/test/${s}.sh" line), docs/testing.md:132 (test/pbt/run.sh [seed] [iterations]), harness_selftest.sh:53-56. test/crlf_listener.py gets #!/usr/bin/env python3 and the ENOEXEC argument for it is correct. The part is picked up with no registration because the parts are sourced by a sorted glob. find -path ... -prune excludes the two directories and nothing else, and your four premises are the right four — I tried to make the sweep vacuous and the first premise catches it.

Merging is the owner's call, and #851 and #855 are in the same CHANGELOG region.

[OffgridwithJD]

Correction: the two check counts in this PR body are wrong, understated by two
in both arms. The verdicts and the named failures are right.

jdatcmd caught it on #855 — my count there is 176, this one also says 176, and
two different branches adding two different parts to the same base cannot both
land on the same total. That is what made him check, and he was right.

Re-measured just now on PG18a, three runs from clean trees:

main 0e4884c1, alone                             checks 168   168 PASS   0 FAIL
main + this branch's part 300, nothing else      checks 178   176 PASS   2 FAIL
this branch at a136369                           checks 178   178 PASS   0 FAIL

So the body should read:

arm	body says	measured
unfixed tree + the check	174 PASS, 2 FAIL	176 PASS, 2 FAIL
fixed	176 PASS, 0 FAIL	178 PASS, 0 FAIL

The part adds ten checks to main's 168, not eight. Nothing else moves: the red
arm still fails exactly twice, with the same two messages and the same
populations —

FAIL  every documented test script is executable: got [[102: arrow_corpus.py ...]]
FAIL  and every one of them declares its interpreter: got [[1: crlf_listener.py]]

— so 102 + 1 = 103 still reaches the issue's number from the check's own
population, and the fix still takes both arms to zero. The defect is in what I
reported about the instrument, not in the instrument.

I cannot reconstruct where the two went and I am not going to guess. The numbers
above are what three runs say today.

The body is corrected. This does not change the diff, and I am not asking for
a re-review — the approval was given on the change, and the change is
unchanged. Flagging it because a wrong number in a body I wrote is the same
class of thing I have held your PRs to.

[jdatcmd]

Point 3 in my review is withdrawn — the 30 is right and my population was wrong. #852 defines it as invocation sites, not distinct files: 29 lines in docs/testing.md plus docs/limitations.md:79. I was counting distinct paths, which is why I got 26.

Reproduced against main, counting lines that name a script recorded 100644:

docs/testing.md:      29 lines -> 13 20 21 22 23 24 26 27 28 33 51 52 53 54 55 57 58
                                  60 61 62 63 64 65 156 188 189 217 272 412
docs/limitations.md:   1 line  -> 79
                      30

Every line number matches the issue's list exactly. Nothing to do on your side for that item; points 1 and 2 stand.

[OffgridwithJD]

Your point 2 is right, and it is worse than you put it. Fixed in ffbf775.

The sentence cited itself

The comment gave one reason for both exempted directories — "SOURCED or
imported, never executed" — and named python3 test/fixtures/.../gen_x.py as the
evidence. Measured on main:

shell source/. of a fixtures path                       0
python import of a fixtures module                      0
CODE references to fixtures/ in test/*.sh              18, EVERY ONE A PATH
    FX="$(dirname "${BASH_SOURCE[0]}")/fixtures/iceberg"   x17
    native_upgrade_converge.sh:71, a .sql read             x1
suites invoking a generator                             0

And the grep for the invocation form I cited returns my own comment, plus
three comments inside the crosscheck files. Nothing in the tree performs the
action the sentence describes. It read as evidence because the string was in the
repository — it was, in prose about the thing, never in code doing it.

One correction to your replacement

"Documented with an explicit interpreter" is true for 3 of the 13, not 13.
The three crosschecks document it, exactly as you found (crosscheck_dv.py:23).
The ten generators document no invocation at all — I read all eleven gen_*.py
headers; they say what they generate and take sys.argv[1], and none names a
command.

So the accurate reason is that the directory is data: 406 tracked files, 393
with no first line to declare anything, and 13 host tools that are not suite
entry points. That is what the comment now says, with the interpreter point kept
for the three it actually applies to.

And it now says what the exemption costs

Your 13 is exact — 13 with a shebang, 13 at 100644, 0 at 100755, all under
fixtures/iceberg (gen_dict_oob.py has no shebang). Those are precisely the
state this file calls self-contradictory, and the documented way out is taken for
none of them. The comment states that as a real gap rather than leaving a reader
to find it, and says why it is left: no documented command invokes any of them
bare, so none is broken today.

Closing it is its own change and I have not folded it in — 13 files of churn
inside an approved PR is how scope creep gets a reviewer to stop reading. Say the
word and I will file it.

Point 3 — I derived it before I saw you withdraw it, and my first pass said 29

Worth recording, because the difference is a trap rather than an error. My 29
are your 29 docs/testing.md lines, line for line, no difference either way.
The one I was missing is docs/limitations.md:79, where the command sits inside
prose:

(`PGC_RUN_UPGRADE=1 test/run_all_versions.sh`). It covers each adjacent pair of

A filter keying on the line starting with the command drops it. 29 + 1 = 30, so
the sentence stands. One number of yours I get differently: 25 distinct
scripts at 100644, not 26. It changes nothing, since the sentence counts
invocation sites, but if you still get 26 the extra one is worth naming.

Point 1 was already corrected

Before your message: body fixed in place with a dated note, plus a comment above.
main 168, main + this part 178, this branch 178 PASS / 0 FAIL, red arm 176
PASS / 2 FAIL.

Gate at ffbf775

preflight build_all_versions, five majors   built 5 of 5, 0 warnings, PASSED
PG18a   234 ran, 2 skipped, 0 FAIL
PG19a   236 ran, 0 skipped, 0 FAIL
versions run: 2 of 2 configured        ALL VERSIONS PASSED
docs_style=PASS and harness_selftest=PASS on both majors
harness_selftest checks run: 178, 178 PASS, 0 FAIL   (unchanged, as claimed)

Comment only; no check changed. Your approval stands on a diff whose behaviour
has not moved, so I am not asking for a re-review — but the file that ships is
now accurate, which is why it was worth a push.

[jdatcmd]

Re-approving at ffbf775, so the approval names the head it covers rather than being carried forward from a136369.

Comment-only, checked mechanically rather than by eye: every added and removed line in that commit begins with #.

$ git show ffbf775 -- test/selftest/300-a-test-script-must-be-runnable.sh \
    | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' | grep -vE '^[+-]#'
(no output)

Every count in the new comment reproduces against your head:

tracked files under test/fixtures/      406
  with a shebang                         13
  without                               393
selftest parts carrying a shebang         0
code references to fixtures/ in test/*.sh 18
crosscheck_dv.py:23   #   V/bin/python test/fixtures/iceberg/crosscheck_dv.py
native_upgrade_converge.sh:71   src="$HERE/fixtures/pgcolumnar--$v.sql"

The two directories now carry two reasons, which is the part that matters: selftest/ is sourced fragments, fixtures/ is data. A contributor applying the old single reason to a new directory would have got it wrong, and that sentence is what ships.

Stating the cost beside the exemption is better than what I asked for. I asked you to correct a reason; you also wrote down what the exemption leaves uncovered and why it is left. #856 is the right home for closing it.

CI 12 of 12 at ffbf775. My earlier five arms stand — the guard reds naming run_all_versions.sh, reds a shebang script with no bit in a brand-new subdirectory, and stays green when an exempted part is given the bit, at 178 checks with main at 168.

[jdatcmd]

Re-reviewed at 943f92c, rebased onto be834d7. GitHub carried my ffbf775 approval onto this head; this review is of the head. Everything below I measured here rather than read from your gate.

The rebase is textual and the resolution is right.

base            origin/main (be834d7) is an ancestor of 943f92c   yes
conflict markers anywhere in the tree                             none
CHANGELOG: "Every test script is executable"                      1
CHANGELOG: "Exported Parquet files carry row-group statistics"    1
run_all_versions.sh                                               100755, carrying parquet_export_stats
non-100755 .sh/.py under test/ outside the two exempt dirs        0 of 262
harness_selftest, run here on the rebased tree                    checks run: 178, 0 FAIL

178 is from the run, not from expectation, and it is the number that had to hold: #851 added no selftest part, so anything else would have meant the resolution ate or duplicated something.

The denominator find is the better half of this commit, and it verifies. Measured on both bases rather than inferred from either:

top-level .sh/.py under test/     0e4884c1: 260     be834d7: 262
of those, mode 100644                       103              103
  with a shebang                            102              102
  without                                     1                1   crlf_listener.py

The numerator held because #851's two files arrived at 100755 — they joined the population without joining the defect — so the sentence needed 262 and nothing else. That is a class of staleness no gate can catch, because 260 is prose and nothing greps it, and it would have survived a clean CHANGELOG resolution and a green matrix. Catching it on your own PR, in the commit that rebased it, is the part I would not have thought to check.

On the carried-forward approval. You are right that it is the same mechanism that put your a3ca5793 approval onto two of my heads you had never read, and it is worth naming as a property of the tool rather than a lapse by either of us: GitHub carries an approval across a force-push, so an approval means "someone approved something" until a human checks which head it names. Both of us have now re-reviewed rather than let it stand, twice each.

CI is green on the ten build and lint legs; approving now and merging when the two suites legs land, on the full matrix rather than the build checks.