From f5459fc7185b311ff72c5a4f2d1460efef489e80 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Mon, 31 Aug 2026 22:24:33 +0000 Subject: [PATCH 1/4] fix: a shebang and the execute bit go together, in every directory that documents a command (#856) #852 gave every documented test script its execute bit. Two things were left, and the second is why the first had no clean answer. THE RULE FLAGGED BOTH STATES, SO A SOURCED FRAGMENT COULD NOT BE CORRECT. The guard reddened a file that declared an interpreter without the bit, and it also reddened a file with no interpreter line at all. Its header documented the way out as "drop its shebang and say why", and that way out did not exist: dropping the shebang moved the file from one red to the other. bench/cb_guards.sh is the file that proved it -- sourced by bench/run_clickbench.sh:176 and test/bench_guards.sh:43, its header saying "Sourced, not executed" since it was written, and unfixable under the old rule. The rule is now a biconditional. A file that declares an interpreter must be executable, and a file that is executable must declare one. A file with neither is a fragment meant to be sourced, and passes. AND THAT REMOVED THE EXEMPTIONS. test/selftest/ and test/fixtures/ were pruned by path because the old rule would have reddened them wholesale. Measured under the new one before the prune came out: selftest/ is 31 scripts and every one is already correct, fixtures/ is 14 .sh/.py of which 13 are the host tools this commit gives the bit. Nothing is excluded now, so nothing is concealed -- the thirteen the exemption used to hide are inside the population and the suite reddens if one loses its bit. bench/ JOINS THE SWEPT POPULATION. docs/benchmarks.md names five bench/ scripts as bare commands -- run_bench.sh, run_bench_fsst.sh, run_bench_readstream.sh, run_bench_join.sh and run_clickbench.sh. All five are executable today, so all five work: correct by habit with nothing checking it, which is exactly what test/ was before #852. A THIRD CHECK, ANCHORED ON THE DOCUMENTS, CLOSES THE HOLE THE BICONDITIONAL OPENS. Delete a documented command's shebang AND its bit and the file is internally consistent and still broken for the reader, so neither of the first two rules can see it. The third reads what the documents tell a reader to type and requires those files to be executable. It is deliberately over-inclusive: the cost of a false positive is one execute bit on a file that is an entry point anyway. Red before green, both arms run. Unfixed, the guard names all fourteen files; fixed, 190 of 190. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9 --- bench/cb_guards.sh | 1 - test/fixtures/iceberg/crosscheck_dv.py | 0 test/fixtures/iceberg/crosscheck_eq_duckdb.py | 0 test/fixtures/iceberg/crosscheck_nm.py | 0 test/fixtures/iceberg/gen_delete_fixture.py | 0 test/fixtures/iceberg/gen_iceberg_bucket.py | 0 test/fixtures/iceberg/gen_iceberg_datepart.py | 0 test/fixtures/iceberg/gen_iceberg_day.py | 0 test/fixtures/iceberg/gen_iceberg_fixture.py | 0 test/fixtures/iceberg/gen_iceberg_temporal.py | 0 test/fixtures/iceberg/gen_iceberg_trunc.py | 0 .../fixtures/iceberg/gen_iceberg_warehouse.py | 0 .../fixtures/iceberg/gen_malformed_fixture.py | 0 .../iceberg/gen_name_mapping_fixture.py | 0 .../300-a-test-script-must-be-runnable.sh | 294 ++++++++++-------- 15 files changed, 159 insertions(+), 136 deletions(-) mode change 100644 => 100755 test/fixtures/iceberg/crosscheck_dv.py mode change 100644 => 100755 test/fixtures/iceberg/crosscheck_eq_duckdb.py mode change 100644 => 100755 test/fixtures/iceberg/crosscheck_nm.py mode change 100644 => 100755 test/fixtures/iceberg/gen_delete_fixture.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_bucket.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_datepart.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_day.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_fixture.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_temporal.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_trunc.py mode change 100644 => 100755 test/fixtures/iceberg/gen_iceberg_warehouse.py mode change 100644 => 100755 test/fixtures/iceberg/gen_malformed_fixture.py mode change 100644 => 100755 test/fixtures/iceberg/gen_name_mapping_fixture.py diff --git a/bench/cb_guards.sh b/bench/cb_guards.sh index 28074009..9db640d2 100644 --- a/bench/cb_guards.sh +++ b/bench/cb_guards.sh @@ -1,4 +1,3 @@ -#!/usr/bin/env bash # # Guards for the ClickBench harness (#465). # diff --git a/test/fixtures/iceberg/crosscheck_dv.py b/test/fixtures/iceberg/crosscheck_dv.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/crosscheck_eq_duckdb.py b/test/fixtures/iceberg/crosscheck_eq_duckdb.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/crosscheck_nm.py b/test/fixtures/iceberg/crosscheck_nm.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_delete_fixture.py b/test/fixtures/iceberg/gen_delete_fixture.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_bucket.py b/test/fixtures/iceberg/gen_iceberg_bucket.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_datepart.py b/test/fixtures/iceberg/gen_iceberg_datepart.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_day.py b/test/fixtures/iceberg/gen_iceberg_day.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_fixture.py b/test/fixtures/iceberg/gen_iceberg_fixture.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_temporal.py b/test/fixtures/iceberg/gen_iceberg_temporal.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_trunc.py b/test/fixtures/iceberg/gen_iceberg_trunc.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_iceberg_warehouse.py b/test/fixtures/iceberg/gen_iceberg_warehouse.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_malformed_fixture.py b/test/fixtures/iceberg/gen_malformed_fixture.py old mode 100644 new mode 100755 diff --git a/test/fixtures/iceberg/gen_name_mapping_fixture.py b/test/fixtures/iceberg/gen_name_mapping_fixture.py old mode 100644 new mode 100755 diff --git a/test/selftest/300-a-test-script-must-be-runnable.sh b/test/selftest/300-a-test-script-must-be-runnable.sh index dfb9420f..04a2b6f4 100644 --- a/test/selftest/300-a-test-script-must-be-runnable.sh +++ b/test/selftest/300-a-test-script-must-be-runnable.sh @@ -1,192 +1,216 @@ -# ---- a test script must be runnable the way it is documented (#852) --------- +# ---- a test script must be runnable the way it is documented (#852, #856) ---- # # Every document that tells a reader how to run a script spells it as a command: # `test/temporal.sh /path/to/pg_config`, `PGC_RUN_UPGRADE=1 -# test/run_all_versions.sh`, `test/pbt/run.sh [seed] [iterations]`. 103 of the -# 262 top-level scripts were mode 100644 at the base this lands on, so 30 of -# those documented invocations died with `Permission denied` before running a -# single statement. +# test/run_all_versions.sh`, `bench/run_bench_fsst.sh /path/to/pg_config`. 103 of +# the 262 top-level scripts under test/ were mode 100644 at the base #853 landed +# on, so 30 of those documented invocations died with `Permission denied` before +# running a single statement. # -# THE DENOMINATOR MOVED UNDER THIS BRANCH AND THE NUMERATOR DID NOT. It was 260 -# when this was written; #851 landed first and added test/parquet_export_stats.sh -# and test/parquet_stats.py, both 100755, so the population grew by two and the -# defect did not. Re-measured on the rebased base rather than carried over. +# THE MATRIX NEVER SAW IT, AND THAT IS THE POINT. ci.yml and nightly.yml both +# call `bash test/run_all_versions.sh`, and the runner launches each suite as +# `bash "$builddir/test/${s}.sh"`. An interpreter named on the command line does +# not consult the execute bit, so a suite's own mode is invisible to every green +# run in this project's history. Those runs are honest. The gap is between the +# documentation and a shell, and only a check that reads the MODE can stand in it. # -# THE MATRIX NEVER SAW IT, AND THAT IS THE POINT. ci.yml:485 and nightly.yml:188 -# both call `bash test/run_all_versions.sh`, and the runner launches each suite -# as `bash "$builddir/test/${s}.sh"` (lines 712 and 749). An interpreter named on -# the command line does not consult the execute bit, so a suite's own mode is -# invisible to every green run in this project's history. Those runs are honest. -# The gap is between the documentation and a shell, and only a check that reads -# the MODE can stand in it. +# THE RULE IS A BICONDITIONAL: A SHEBANG AND THE EXECUTE BIT GO TOGETHER. # -# TWO SITUATIONS, NOT ONE. Exactly one 100755 -> 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. The other 102 files were -# born 100644. So one is a regression that a check like this one would have -# caught the day it happened, and the rest are a habit that nothing ever -# contradicted. +# A. A file that declares an interpreter must be executable. It has said it is +# meant to be run, and a mode that forbids running it contradicts the file's +# own first line. This is #852's defect. +# B. A file that is executable must declare an interpreter. Otherwise the mode +# promises something the file cannot deliver: what would `./foo.py` even run +# under? +# C. And a script a document names as a bare command must be executable, +# whatever its first line says. This is the only rule anchored on prose, and +# it is here to close the hole B leaves: delete a documented command's +# shebang AND its bit and A and B both fall silent, because the file is then +# internally consistent and still broken for the reader. # -# WHY THE RULE IS ANCHORED ON THE SHEBANG. The alternative populations both -# fail. A documentation-derived list ("every script docs/testing.md names") makes -# the gate a hostage to prose: delete a line from a document and the check -# silently narrows, which is the same class of accident as the one being fixed. -# A hand-maintained allowlist of names drifts, and 102 files born wrong is what -# drift looks like. The file's own first line is the honest population: a script -# that opens `#!/usr/bin/env bash` has declared that it is meant to be run, and a -# mode that forbids running it contradicts what the file already says about -# itself. So the rule needs no name list, and the two shared libraries need no -# exemption -- lib.sh carries a shebang and the bit, and portlib.sh gets the bit -# here for the same reason every suite does. +# A file with NEITHER a shebang nor the bit passes, and that is deliberate. It is +# a fragment meant to be sourced, and it is the only self-consistent way to say +# so. #856 is why this is spelled out: an earlier revision of this part flagged +# every file without a shebang, which left a sourced fragment no way to be +# correct -- dropping the shebang moved it from one red to another. The header +# then documented "drop its shebang and say why" as the way out, and that way out +# did not exist. bench/cb_guards.sh is the file that proved it: sourced by +# bench/run_clickbench.sh and test/bench_guards.sh, its header saying "Sourced, +# not executed" since the day it was written, and unfixable under the old rule. # -# THE EXEMPTION IS TWO DIRECTORIES, NOT A LIST OF FILES, AND THE TWO ARE EXEMPT -# FOR DIFFERENT REASONS. An earlier version of this comment gave one reason for -# both -- "SOURCED or imported, never executed" -- and it was false for -# test/fixtures/ in both halves. +# WHY THE POPULATION IS "EVERY DIRECTORY THAT HOLDS A DOCUMENTED ENTRY POINT". +# Today that is test/ and bench/. #852 was found in test/ and fixed there, and +# bench/ was left out -- while docs/benchmarks.md names five bench/ scripts as +# bare commands. All five are 100755 today, so all five work: correct by habit, +# with nothing checking it, which is exactly what test/ was before #852. A +# directory that documents an entry point earns the same guard as the one that +# already had it. # -# test/selftest/ holds SOURCED fragments. harness_selftest.sh:53-56 sources every -# part, and they carry no shebang precisely because nothing executes them. Giving -# them the bit would advertise a way to run them that does not work, which is -# this defect pointing the other way. +# AND THE BICONDITIONAL REMOVED THE NEED FOR EXEMPTIONS, WHICH IS THE REAL +# SIMPLIFICATION HERE. The old rule flagged every file without a shebang, so the +# two directories full of sourced fragments and data had to be pruned by path or +# they would have reddened the sweep wholesale. Under A and B a fragment with +# neither a shebang nor the bit is CORRECT, so nothing needs excusing: # -# test/fixtures/ is DATA. Nothing in test/ sources, imports or runs any of it: -# every reference in test/*.sh is a PATH -- 18 of them, all of the shape -# `FX="$(dirname "${BASH_SOURCE[0]}")/fixtures/iceberg"`, plus one .sql read in -# native_upgrade_converge.sh:71 -- and no suite invokes a generator. Measured, on -# main, rather than assumed. Of 406 tracked files there, 393 have no first line -# to declare anything. The 13 that do are host tools rather than suite entry -# points: three crosschecks, each documenting an explicit interpreter -# (crosscheck_dv.py:23, `V/bin/python test/fixtures/iceberg/crosscheck_dv.py`), -# and ten generators that record how a committed fixture was made, run by hand -# when one is regenerated. An interpreter named on the command line does not -# consult the execute bit, which is the same mechanism this file relies on for -# the matrix above. +# test/selftest/ 31 scripts, every one already ok -- no shebang, no bit +# test/fixtures/ 14 .sh/.py, one ok and thirteen that #856 gave the bit # -# AND WHAT THE EXEMPTION COSTS, said rather than left for a reader to find. Those -# 13 are shebang-without-the-bit -- the exact state this file calls -# self-contradictory -- and the way out below is taken for none of them. It is a -# real gap in the rule's coverage, left deliberately because it is not #852's -# defect: no documented command invokes any of them bare, so none of them is -# broken today. Closing it is its own change. -# -# Every OTHER directory under test/ is swept, so a runnable entry -# point in a new subdirectory is covered the day it is added. test/pbt/run.sh is -# why that matters: it is a documented command (docs/testing.md:132) that lives -# one level down, it is already correct, and a top-level-only sweep would have -# left the one file most like the defect outside the guard. -# -# THE WAY OUT, IF A FUTURE SCRIPT MUST NOT BE EXECUTABLE, IS TO DROP ITS SHEBANG -# AND SAY WHY IN ITS HEADER -- not to leave a 100644 file whose first line still -# claims otherwise. The second check below is what makes that an explicit act: a -# file with neither is caught too, so "no shebang" cannot become the new silent -# default. +# Measured, both directories, before the prune was removed. Nothing is excluded +# now, so nothing is concealed: the thirteen fixture host tools that the old +# exemption hid in a state this rule calls wrong are inside the population and +# the suite would redden if any of them lost its bit again. -# ---- controls --------------------------------------------------------------- -# -# A mode sweep that has never fired is indistinguishable from a tree that is -# already clean, and this one is added to a tree where it fires 103 times. The -# fixtures pin all three verdicts, so the sweep below is known to separate them -# before it is believed about the real tree. +_tsm_root="$(cd "$PGC_TESTDIR/.." && pwd)" _tsm_has_shebang() { # _tsm_has_shebang FILE head -1 "$1" 2>/dev/null | grep -q '^#!' } -_tsm_verdict() { # _tsm_verdict FILE -> ok | noexec | noshebang - if ! _tsm_has_shebang "$1"; then echo noshebang - elif [ ! -x "$1" ]; then echo noexec - else echo ok +# ok -- shebang and bit, or neither: internally consistent +# noexec -- declares an interpreter and cannot be run (rule A) +# nodecl -- can be run and declares no interpreter (rule B) +_tsm_verdict() { # _tsm_verdict FILE -> ok | noexec | nodecl + if _tsm_has_shebang "$1"; then + [ -x "$1" ] && echo ok || echo noexec + else + [ -x "$1" ] && echo nodecl || echo ok fi } -_tsm_fix="$PGC_WORKDIR/tsm_shebang_noexec.sh" -printf '#!/usr/bin/env bash\nexit 0\n' > "$_tsm_fix"; chmod 644 "$_tsm_fix" -check "control: a script that declares an interpreter without the bit is caught" \ - "$(_tsm_verdict "$_tsm_fix")" "noexec" - -_tsm_ok="$PGC_WORKDIR/tsm_shebang_exec.sh" -printf '#!/usr/bin/env bash\nexit 0\n' > "$_tsm_ok"; chmod 755 "$_tsm_ok" -check "control: and the same script with the bit is not" \ - "$(_tsm_verdict "$_tsm_ok")" "ok" - -_tsm_none="$PGC_WORKDIR/tsm_no_shebang.py" -printf 'import sys\n' > "$_tsm_none"; chmod 644 "$_tsm_none" -check "control: and a script with no interpreter line is caught separately" \ - "$(_tsm_verdict "$_tsm_none")" "noshebang" - -# The bit has to survive the copy, or this check reads a mode the repository -# does not have. run_all_versions.sh:668 stages the tree with `cp -a`, and the -# matrix runs these parts out of that staged copy. Pinned as a property of the -# copy rather than trusted from the flag. -_tsm_cp_src="$PGC_WORKDIR/tsm_copy_src.sh" -printf '#!/usr/bin/env bash\nexit 0\n' > "$_tsm_cp_src"; chmod 755 "$_tsm_cp_src" -cp -a "$_tsm_cp_src" "$PGC_WORKDIR/tsm_copy_dst.sh" +# ---- controls: all four corners of the biconditional ------------------------ +# +# A sweep that has never fired is indistinguishable from a tree that is already +# clean. These pin every verdict before the sweep below is believed. + +_tsm_c="$PGC_WORKDIR/tsm_ctl"; mkdir -p "$_tsm_c" +printf '#!/usr/bin/env bash\nexit 0\n' > "$_tsm_c/sh_noexec.sh"; chmod 644 "$_tsm_c/sh_noexec.sh" +printf '#!/usr/bin/env bash\nexit 0\n' > "$_tsm_c/sh_exec.sh"; chmod 755 "$_tsm_c/sh_exec.sh" +printf 'import sys\n' > "$_tsm_c/bare_exec.py"; chmod 755 "$_tsm_c/bare_exec.py" +printf 'import sys\n' > "$_tsm_c/bare_noexec.py"; chmod 644 "$_tsm_c/bare_noexec.py" + +check "control: an interpreter declared without the bit is caught" \ + "$(_tsm_verdict "$_tsm_c/sh_noexec.sh")" "noexec" +check "control: and the same file with the bit is not" \ + "$(_tsm_verdict "$_tsm_c/sh_exec.sh")" "ok" +check "control: the bit without an interpreter is caught too" \ + "$(_tsm_verdict "$_tsm_c/bare_exec.py")" "nodecl" +check "control: and a sourced fragment, with neither, is correct" \ + "$(_tsm_verdict "$_tsm_c/bare_noexec.py")" "ok" + +# The bit has to survive the copy, or this reads a mode the repository does not +# have: run_all_versions.sh stages the tree with `cp -a` and the matrix runs +# these parts out of that staged copy. +cp -a "$_tsm_c/sh_exec.sh" "$_tsm_c/copied.sh" check "control: cp -a preserves the execute bit, so a staged tree reads the same" \ - "$(_tsm_verdict "$PGC_WORKDIR/tsm_copy_dst.sh")" "ok" + "$(_tsm_verdict "$_tsm_c/copied.sh")" "ok" # ---- the population --------------------------------------------------------- # -# Every .sh and .py under test/, at any depth, MINUS the two sourced -# directories. Pruned with -path so a file is excluded by the directory it is -# in, not by matching its name against a list. +# Every .sh and .py under a directory that holds a documented entry point, at +# any depth, and nothing is excluded. There is no prune and no name list. + +_tsm_dirs=("$_tsm_root/test" "$_tsm_root/bench") _tsm_scripts=() while IFS= read -r _tsm_f; do _tsm_scripts+=("$_tsm_f") -done < <(find "$PGC_TESTDIR" \ - \( -path "$PGC_TESTDIR/selftest" -o -path "$PGC_TESTDIR/fixtures" \) -prune -o \ - -type f \( -name '*.sh' -o -name '*.py' \) -print | sort) +done < <(find "${_tsm_dirs[@]}" -type f \( -name '*.sh' -o -name '*.py' \) \ + -print 2>/dev/null | sort) # ---- premises --------------------------------------------------------------- -# A sweep over an empty population passes vacuously, and a find that matched -# nothing looks exactly like a clean tree. check "premise: the sweep reads a population of scripts, not an empty find" \ "$([ "${#_tsm_scripts[@]}" -ge 200 ] && echo enough || echo "${#_tsm_scripts[@]}")" "enough" -# The prune must exclude those two directories and nothing else. Both halves -# matter: an over-broad prune would silently empty the sweep, and a prune that -# missed would redden every sourced part. -check "premise: the sourced parts and the fixtures are outside the population" \ - "$(printf '%s\n' "${_tsm_scripts[@]}" | grep -cE '/(selftest|fixtures)/')" "0" +# The reverse of what this premise used to assert. Both directories are swept +# now, and saying so out loud is what stops the prune quietly coming back. +check "premise: the sourced parts are inside the population, not pruned" \ + "$([ "$(printf '%s\n' "${_tsm_scripts[@]}" | grep -c '/test/selftest/')" -ge 25 ] \ + && echo yes || echo no)" "yes" + +check "premise: and so are the fixture host tools" \ + "$([ "$(printf '%s\n' "${_tsm_scripts[@]}" | grep -c '/test/fixtures/')" -ge 10 ] \ + && echo yes || echo no)" "yes" -# and the prune did not also swallow a subdirectory that IS swept check "premise: a runnable script one level down is inside the population" \ "$(printf '%s\n' "${_tsm_scripts[@]}" | grep -c '/pbt/run\.sh$')" "1" -# The two exempted directories are non-empty, so the exemption is a real -# decision about real files rather than a prune of nothing. -check "premise: the exempted directories actually hold scripts" \ - "$([ "$(find "$PGC_TESTDIR/selftest" "$PGC_TESTDIR/fixtures" -type f \ - \( -name '*.sh' -o -name '*.py' \) | wc -l)" -ge 40 ] && echo yes || echo no)" "yes" +# #856's whole point: bench/ is swept now, and the premise says so by naming a +# file that only a bench/ sweep can reach. +check "premise: and bench/ is in the population" \ + "$(printf '%s\n' "${_tsm_scripts[@]}" | grep -c '/bench/run_bench\.sh$')" "1" -# ---- the sweep -------------------------------------------------------------- +# ---- the sweep: rules A and B ---------------------------------------------- _tsm_noexec=""; _tsm_noexec_n=0 -_tsm_noshebang=""; _tsm_noshebang_n=0 +_tsm_nodecl=""; _tsm_nodecl_n=0 for _tsm_f in "${_tsm_scripts[@]}"; do case "$(_tsm_verdict "$_tsm_f")" in noexec) _tsm_noexec_n=$((_tsm_noexec_n + 1)) - [ "$_tsm_noexec_n" -le 5 ] && _tsm_noexec="$_tsm_noexec ${_tsm_f#"$PGC_TESTDIR"/}" + [ "$_tsm_noexec_n" -le 5 ] && _tsm_noexec="$_tsm_noexec ${_tsm_f#"$_tsm_root"/}" ;; - noshebang) - _tsm_noshebang_n=$((_tsm_noshebang_n + 1)) - [ "$_tsm_noshebang_n" -le 5 ] && _tsm_noshebang="$_tsm_noshebang ${_tsm_f#"$PGC_TESTDIR"/}" + nodecl) + _tsm_nodecl_n=$((_tsm_nodecl_n + 1)) + [ "$_tsm_nodecl_n" -le 5 ] && _tsm_nodecl="$_tsm_nodecl ${_tsm_f#"$_tsm_root"/}" ;; esac done -# The count leads the message and at most five names follow it: a real failure -# is one or two files and wants naming, while the 103 this was written against -# would otherwise print an unreadable line. +# The count leads the message and at most five names follow it: a real failure is +# one or two files and wants naming, while the 103 this was written against would +# otherwise print an unreadable line. _tsm_fmt() { # _tsm_fmt N NAMES [ "$1" -eq 0 ] && { echo "[]"; return; } echo "[$1:$2]" } -check "every documented test script is executable" \ +check "every script that declares an interpreter is executable" \ "$(_tsm_fmt "$_tsm_noexec_n" "$_tsm_noexec")" "[]" -check "and every one of them declares its interpreter" \ - "$(_tsm_fmt "$_tsm_noshebang_n" "$_tsm_noshebang")" "[]" +check "and every executable script declares one" \ + "$(_tsm_fmt "$_tsm_nodecl_n" "$_tsm_nodecl")" "[]" + +# ---- rule C: what the documents actually tell a reader to type -------------- +# +# Anchored on prose, and that is a deliberate exception rather than a lapse. A +# documentation-derived list is the wrong POPULATION -- delete a line and the +# sweep silently narrows -- which is why A and B are anchored on the files. As an +# additional check it cannot create a false green for either of them, and it is +# the only one that states #852's defect directly: the command in the manual +# does not run. +# +# Over-inclusive on purpose. Any `test/x.sh` or `bench/x.sh` in a document that +# is not directly preceded by an interpreter counts, prose or fenced, because a +# reader who sees a path may well type it. The cost of a false positive is one +# execute bit on a file that is an entry point anyway. + +_tsm_docs=() +while IFS= read -r _tsm_d; do + _tsm_docs+=("$_tsm_d") +done < <(find "$_tsm_root/docs" -maxdepth 1 -name '*.md' -print 2>/dev/null | sort) +[ -f "$_tsm_root/README.md" ] && _tsm_docs+=("$_tsm_root/README.md") + +_tsm_named="$(grep -hoE '(^|[^/[:alnum:]_.-])(test|bench)/[A-Za-z0-9_./-]+\.(sh|py)' \ + "${_tsm_docs[@]}" 2>/dev/null \ + | grep -oE '(test|bench)/[A-Za-z0-9_./-]+\.(sh|py)' | sort -u)" + +check_num "premise: the documents name a population of commands, not none" \ + "$([ "$(printf '%s\n' "$_tsm_named" | grep -c .)" -ge 40 ] && echo 1 || echo 0)" "1" + +_tsm_docbad=""; _tsm_docbad_n=0 +while IFS= read -r _tsm_s; do + [ -n "$_tsm_s" ] || continue + [ -f "$_tsm_root/$_tsm_s" ] || continue # a document may name a path that moved + if [ ! -x "$_tsm_root/$_tsm_s" ]; then + _tsm_docbad_n=$((_tsm_docbad_n + 1)) + [ "$_tsm_docbad_n" -le 5 ] && _tsm_docbad="$_tsm_docbad $_tsm_s" + fi +done <<< "$_tsm_named" + +check "and every script a document names is executable" \ + "$(_tsm_fmt "$_tsm_docbad_n" "$_tsm_docbad")" "[]" + +unset _tsm_root _tsm_scripts _tsm_dirs _tsm_docs _tsm_named _tsm_f _tsm_d _tsm_s +unset _tsm_noexec _tsm_noexec_n _tsm_nodecl _tsm_nodecl_n _tsm_docbad _tsm_docbad_n _tsm_c +unset -f _tsm_has_shebang _tsm_verdict _tsm_fmt From 6d8997cfffaa7d11fe0cf5b499b94bb6549dd50a Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Mon, 31 Aug 2026 22:28:20 +0000 Subject: [PATCH 2/4] docs: state the biconditional and the new population (#856) CONTEXT.md had it right before the code did: it already described the rule as failing "if one has either without the other", which is the biconditional this branch implements. The code was stricter than the documented rule, and that gap is what left a sourced fragment no way to be correct. The bullet now also says the population is every directory that documents an entry point -- today test/ and bench/ -- that nothing is excluded, and that a script a document names must be executable whatever its first line says. No docs/ change: nothing user-facing moves and no documented command changes. This is a contributor rule and it lives in CONTEXT.md beside the others. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9 --- CHANGELOG.md | 35 +++++++++++++++++++++++++++++++++++ CONTEXT.md | 27 ++++++++++++++++++--------- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33365057..f9d2e43c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,41 @@ true until the next version shipped. ### Fixed +- A shebang and the execute bit go together, and every directory that documents + a command is swept (#856). Two things were left over from #852. + + **The rule flagged both states, so a sourced fragment could not be correct.** + It reddened a file that declared an interpreter without the bit, and it also + reddened a file with no interpreter line at all. Its header documented the way + out as "drop its shebang and say why", and that way out did not exist: dropping + the shebang moved the file from one red to the other. `bench/cb_guards.sh` is + the file that proved it -- sourced by `bench/run_clickbench.sh` and + `test/bench_guards.sh`, its header saying "Sourced, not executed" since it was + written, and unfixable under the old rule. `CONTEXT.md` had described the rule + correctly all along, as failing "if one has either without the other"; it was + the code that was stricter than the documented rule. + + The rule is now that biconditional, and a file with neither a shebang nor the + bit passes. + + **That removed the exemptions.** `test/selftest/` and `test/fixtures/` were + pruned by path because the old rule would have reddened them wholesale. + Measured under the new one before the prune came out: `selftest/` is 31 scripts + and every one is already correct, `fixtures/` is 14 `.sh`/`.py` of which 13 are + the host tools this change gives the bit. Nothing is excluded now, so nothing + is concealed. + + **`bench/` joins the population.** `docs/benchmarks.md` names five `bench/` + scripts as bare commands. All five are executable today, so all five work: + correct by habit with nothing checking it, which is what `test/` was before + #852. + + **A third check, anchored on the documents, closes the hole the biconditional + opens.** Delete a documented command's shebang and its bit and the file is + internally consistent and still broken for a reader, so neither of the first + two rules can see it. The third requires every script a document names to be + executable. + - No compiled Python artifact is tracked, and the tree ignores the ones the interpreter writes (#854). `test/__pycache__/ste_check.cpython-312.pyc` was tracked. Its source, `test/ste_check.py`, was renamed to diff --git a/CONTEXT.md b/CONTEXT.md index 1b648e9c..e83d4944 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -166,15 +166,24 @@ and ends with `pgc_summary`. - Register every suite in `SUITES` in `test/run_all_versions.sh`. That array is **one name per line and sorted**; insert in sorted position, never at the end. `harness_selftest` fails if the order decays. -- **A script is executable and declares its interpreter on line 1.** - `harness_selftest` sweeps every `.sh` and `.py` under `test/`, at any depth, - and fails if one has either without the other. Both halves are needed. The - matrix starts a suite as `bash test/.sh`, which never reads the mode, so - only the documentation and a reader's shell ever see it: 103 scripts were - 100644 when this rule was written, and 30 documented commands died with - `Permission denied` (#852). `test/selftest/` and `test/fixtures/` are exempt, - and only those two. Their contents are sourced or imported rather than run, so - the bit would advertise a way to run them that does not work. +- **A script is executable and declares its interpreter on line 1, or it has + neither.** `harness_selftest` sweeps every `.sh` and `.py` under `test/` and + `bench/`, at any depth, with nothing excluded, and fails if one has either + without the other. A file with neither is a fragment meant to be sourced, and + that is the only self-consistent way to say so. The matrix starts a suite as + `bash test/.sh`, which never reads the mode, so only the documentation + and a reader's shell ever see it: 103 scripts were 100644 when this rule was + written, and 30 documented commands died with `Permission denied` (#852). +- **And a script a document names as a bare command must be executable**, + whatever its first line says. That is the one check anchored on prose, and it + is deliberately over-inclusive: it exists because a file that has lost both its + shebang and its bit is internally consistent and still broken for the reader, + so the sweep above cannot see it (#856). +- The population is every directory that holds a documented entry point, which + today is `test/` and `bench/`. `docs/benchmarks.md` names five `bench/` scripts + as bare commands; before #856 nothing checked them, which is exactly what + `test/` was before #852. A new directory that documents a command belongs in + the sweep the day it is added. - Count suites by asking the runner, never by parsing the source: `bash test/run_all_versions.sh --list-suites | wc -l`. A text parser over the array disagrees with bash on exactly the mistake this invites, and the From dff9ef68b6de849de94b05b57a2fd75313204321 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Mon, 31 Aug 2026 23:11:14 +0000 Subject: [PATCH 3/4] fix: check C's premise is per-directory, because a total hides the narrowing (#856) jdatcmd said in review that he would look at whether check C's population can silently narrow, since that is the objection #853's own header raises against a prose-derived population. It can, and the measurement is worse than the objection: the documents name 83 scripts, 76 under test/ and 7 under bench/. Delete docs/benchmarks.md and the bench/ half goes to ZERO while the total is still 75 -- over any floor, so the premise stayed green and C silently stopped covering the directory #856 added it for. A total is the wrong premise for this check. It is now per-directory, and it is the population's own definition turned into an assertion: a directory is swept BECAUSE it holds documented entry points, so the documents must still name one in it. Two arms, both red where the old premise was green: delete docs/benchmarks.md premise reds, got [[ bench]] add a swept directory documenting nothing (tools/) premise reds, got [[ tools]] The second is the better half. It means a directory cannot be added to the sweep on somebody's assumption that it holds documented commands: the premise checks the reason the directory is in the population at all. A single deleted line still narrows C by one script and stays green, and that is accepted rather than overlooked. C is an additional check over A and B, not the population, so a one-script narrowing weakens C alone and cannot make either sweep pass falsely. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9 --- .../300-a-test-script-must-be-runnable.sh | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/selftest/300-a-test-script-must-be-runnable.sh b/test/selftest/300-a-test-script-must-be-runnable.sh index 04a2b6f4..d80f4df5 100644 --- a/test/selftest/300-a-test-script-must-be-runnable.sh +++ b/test/selftest/300-a-test-script-must-be-runnable.sh @@ -198,6 +198,27 @@ _tsm_named="$(grep -hoE '(^|[^/[:alnum:]_.-])(test|bench)/[A-Za-z0-9_./-]+\.(sh| check_num "premise: the documents name a population of commands, not none" \ "$([ "$(printf '%s\n' "$_tsm_named" | grep -c .)" -ge 40 ] && echo 1 || echo 0)" "1" +# A TOTAL IS THE WRONG PREMISE FOR THIS CHECK, and a floor over it hides the +# narrowing that matters. Measured on this tree: the documents name 83 scripts, +# 76 under test/ and 7 under bench/. Delete docs/benchmarks.md and the bench/ +# half goes to ZERO while the total is still 75 -- comfortably over any floor, +# so the premise stays green and C silently stops covering the directory #856 +# added it for. That is #853's own objection to a prose-derived population, +# arriving where it actually bites. +# +# So the premise is per-directory, and it is the population's definition turned +# into an assertion: a directory is swept BECAUSE it holds documented entry +# points, so the documents must still name one. A directory added to the sweep +# with nothing documented in it reddens here and says so. +_tsm_uncovered="" +for _tsm_d in "${_tsm_dirs[@]}"; do + _tsm_b="$(basename "$_tsm_d")" + printf '%s\n' "$_tsm_named" | grep -q "^$_tsm_b/" \ + || _tsm_uncovered="$_tsm_uncovered $_tsm_b" +done +check "premise: and they name at least one command in every swept directory" \ + "[${_tsm_uncovered}]" "[]" + _tsm_docbad=""; _tsm_docbad_n=0 while IFS= read -r _tsm_s; do [ -n "$_tsm_s" ] || continue @@ -212,5 +233,6 @@ check "and every script a document names is executable" \ "$(_tsm_fmt "$_tsm_docbad_n" "$_tsm_docbad")" "[]" unset _tsm_root _tsm_scripts _tsm_dirs _tsm_docs _tsm_named _tsm_f _tsm_d _tsm_s +unset _tsm_uncovered _tsm_b unset _tsm_noexec _tsm_noexec_n _tsm_nodecl _tsm_nodecl_n _tsm_docbad _tsm_docbad_n _tsm_c unset -f _tsm_has_shebang _tsm_verdict _tsm_fmt From 9794d52b7aa32ad384501d63828c5b0c5fdd2632 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Mon, 31 Aug 2026 23:18:35 +0000 Subject: [PATCH 4/4] fix: a documented script must exist, and the comment must match the extraction (#856) Two findings from jdatcmd's review, both measured on the branch rather than read off it. A DOCUMENTED PATH THAT DOES NOT EXIST WAS SKIPPED, SILENTLY. Check C did `[ -f ] || continue`, commented "a document may name a path that moved". He appended a fenced line naming test/does_not_exist.sh to docs/testing.md and the suite stayed green. A reader who types that command gets "No such file or directory" instead of "Permission denied", which is #852's defect wearing a different error message -- and C is the check whose whole justification is that the command in the manual does not run. The skip is now a check. All 83 named paths resolve today, so this is a guard rather than a fix, and C is two assertions instead of one: a named script must exist, and it must be executable. CONTEXT.md says both halves now. AND THE COMMENT DESCRIBED A FILTER THE CODE DOES NOT IMPLEMENT. It said the extraction counts any path "not directly preceded by an interpreter". There is no such exclusion: the leading character class admits a space, so `bash test/smoke.sh` in docs/testing.md is counted like any other, which is the one occurrence of that shape today. The behaviour is right -- over-inclusive is the point -- and the sentence was wrong, which is the same class as the fixtures rationale on #853 and worth the same correction. The accepted slack is now stated in the file with its number rather than only in a pull request body: 43 over the floor, so deleting one line narrows C by one script and stays green. C is additive over A and B, so that weakens C alone and cannot make either sweep pass falsely. The narrowing that matters, a whole document going and taking a directory's coverage with it, is what the per-directory premise catches. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9 --- CONTEXT.md | 6 ++- .../300-a-test-script-must-be-runnable.sh | 42 ++++++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index e83d4944..60515373 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -174,8 +174,10 @@ and ends with `pgc_summary`. `bash test/.sh`, which never reads the mode, so only the documentation and a reader's shell ever see it: 103 scripts were 100644 when this rule was written, and 30 documented commands died with `Permission denied` (#852). -- **And a script a document names as a bare command must be executable**, - whatever its first line says. That is the one check anchored on prose, and it +- **And a script a document names as a bare command must exist and be + executable**, whatever its first line says. Both halves: a named path that has + been deleted or moved gives a reader `No such file or directory`, which is the + same defect as `Permission denied` in a different coat. That is the one check anchored on prose, and it is deliberately over-inclusive: it exists because a file that has lost both its shebang and its bit is internally consistent and still broken for the reader, so the sweep above cannot see it (#856). diff --git a/test/selftest/300-a-test-script-must-be-runnable.sh b/test/selftest/300-a-test-script-must-be-runnable.sh index d80f4df5..d8e6e438 100644 --- a/test/selftest/300-a-test-script-must-be-runnable.sh +++ b/test/selftest/300-a-test-script-must-be-runnable.sh @@ -180,10 +180,33 @@ check "and every executable script declares one" \ # the only one that states #852's defect directly: the command in the manual # does not run. # -# Over-inclusive on purpose. Any `test/x.sh` or `bench/x.sh` in a document that -# is not directly preceded by an interpreter counts, prose or fenced, because a -# reader who sees a path may well type it. The cost of a false positive is one -# execute bit on a file that is an entry point anyway. +# Over-inclusive on purpose, and more so than an earlier draft of this comment +# claimed. EVERY `test/x.sh` or `bench/x.sh` in a document counts -- prose or +# fenced, and including one an interpreter already precedes. The leading +# character class admits a space, so `bash test/smoke.sh` in docs/testing.md is +# counted like any other, which is the one occurrence of that shape today. That +# is fine and it is deliberate: a reader who sees a path may well type it +# without the interpreter, and the cost of a false positive is one execute bit +# on a file that is an entry point anyway. The draft said the extraction skipped +# interpreter-prefixed paths; it never did, and a comment describing a filter +# the code does not implement is the defect this file exists to prevent. +# +# TWO THINGS ARE ASSERTED, NOT ONE. A document may name a script that does not +# exist, and a reader who types it gets "No such file or directory" rather than +# "Permission denied" -- #852's defect wearing a different error message, and C +# is the check whose entire justification is that the command in the manual does +# not run. So a named path must EXIST as well as be executable. An earlier draft +# skipped a missing path with `[ -f ] || continue`, commented "a document may +# name a path that moved", and that skip was a silent hole rather than a +# decision. All 83 named paths resolve today, so this is a guard rather than a +# fix. +# +# WHAT IS STILL ACCEPTED, with the number rather than a shrug: deleting one line +# that names one script narrows C by one and stays green, because the count +# premise below has 43 of slack over its floor. C is additive over A and B, so a +# one-script narrowing weakens C alone and cannot make either sweep pass +# falsely. The narrowing that WOULD matter -- a whole document going, taking a +# directory's coverage with it -- is what the per-directory premise catches. _tsm_docs=() while IFS= read -r _tsm_d; do @@ -220,19 +243,26 @@ check "premise: and they name at least one command in every swept directory" \ "[${_tsm_uncovered}]" "[]" _tsm_docbad=""; _tsm_docbad_n=0 +_tsm_docgone=""; _tsm_docgone_n=0 while IFS= read -r _tsm_s; do [ -n "$_tsm_s" ] || continue - [ -f "$_tsm_root/$_tsm_s" ] || continue # a document may name a path that moved - if [ ! -x "$_tsm_root/$_tsm_s" ]; then + if [ ! -e "$_tsm_root/$_tsm_s" ]; then + _tsm_docgone_n=$((_tsm_docgone_n + 1)) + [ "$_tsm_docgone_n" -le 5 ] && _tsm_docgone="$_tsm_docgone $_tsm_s" + elif [ ! -x "$_tsm_root/$_tsm_s" ]; then _tsm_docbad_n=$((_tsm_docbad_n + 1)) [ "$_tsm_docbad_n" -le 5 ] && _tsm_docbad="$_tsm_docbad $_tsm_s" fi done <<< "$_tsm_named" +check "and every script a document names exists" \ + "$(_tsm_fmt "$_tsm_docgone_n" "$_tsm_docgone")" "[]" + check "and every script a document names is executable" \ "$(_tsm_fmt "$_tsm_docbad_n" "$_tsm_docbad")" "[]" unset _tsm_root _tsm_scripts _tsm_dirs _tsm_docs _tsm_named _tsm_f _tsm_d _tsm_s unset _tsm_uncovered _tsm_b unset _tsm_noexec _tsm_noexec_n _tsm_nodecl _tsm_nodecl_n _tsm_docbad _tsm_docbad_n _tsm_c +unset _tsm_docgone _tsm_docgone_n unset -f _tsm_has_shebang _tsm_verdict _tsm_fmt