test: the collation guard could not see a process-substituted sort (#1112)

CHANGELOG.md

@@ -69,6 +69,42 @@ true until the next version shipped.
   shows a generator read through a subshell losing the seed on this bash, another
   shows the assigning form keeping it. Three static arms pin `fuzz.sh`'s own form,
   and putting the printing generator back reddens two of them.
+- The collation guard could not see a process-substituted sort (#1112).
+
+  `comm` requires both inputs sorted in ITS collation and does not check. Fed a
+  mismatch it writes `input is not in sorted order` to stderr and prints a result
+  anyway, so where stderr lands in a log nobody reads, a wrong set arrives looking
+  like an answer. The inputs are not collation-insensitive: on real suite names
+  `pgc_setup`/`pg_dump_roundtrip` and `projections`/`projection_update` both swap
+  between `C` and `en_US.UTF-8`.
+
+  `test/selftest/070` required every `sort` feeding a `comm` to carry `LC_ALL=C`,
+  and matched only the PIPED form. A process-substituted sort is not a pipeline, so
+  `run_all_versions.sh` used `comm` three times through substitutions, matched the
+  pattern zero times, and read as compliant. One of the three had been there since
+  #928. Found by OffgridwithJD reviewing #1110.
+
+  PINNING THE COMM IS NOT ENOUGH, and that is the half that changes the fix rather
+  than its description. `LC_ALL=C comm <(sort a) <(sort b)` pins only comm's own
+  comparison: the substitutions run in subshells of the PARENT and inherit its
+  locale. A guard accepting `LC_ALL=C` anywhere on the line would bless exactly the
+  form a reader writes after reading the guard's name. The two halves are now
+  separate checks.
+
+  AND IT READS CODE ONLY. The guard scanned every line, prose included, so a comment
+  explaining the rule violated it: a note reading "reads only the piped form"
+  contained the literal string it grepped for and flagged its own file. A rule that
+  cannot be written down is a rule people stop writing down.
+
+  Both corpus arms report zero on this tree, measured before the change, so the
+  detector is proved by PLANTING rather than by the corpus: the substituted form,
+  the piped form, a half-pinned line, a pinned comm over unpinned sorts, and the two
+  forms that must not be flagged.
+
+  Mutation testing deleted a line from the first draft and NOTHING changed, so it
+  was dead and is gone: both patterns require `sort` immediately after the `|` or
+  the `<(`, which makes a mixed line fail without the substitution step the draft
+  performed.
 
 - Every PR with a changelog entry conflicted with every other one (#996).
 

test/check_ledger.tsv

@@ -221,12 +221,21 @@ harness_selftest	050-the-list-must-be-read-the	premise: the fixture really does
 harness_selftest	060-the-list-stays-sorted-which-is	premise: C collation puts sort_status before sorted_projection	15;16;17;18;19	never	-
 harness_selftest	060-the-list-stays-sorted-which-is	the suite list is sorted in C order, so two new suites land in different places	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	a file that uses comm pins the collation of every sort feeding it	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	a line with ONE of two sorts pinned is still caught	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	a process-substituted sort with no pin is caught, which the old pattern missed	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	an unpinned comm is caught by its own reader	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	and a fully pinned line is not flagged, so the reader can report none	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	and a pinned comm is not	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	and a prefix of a registered name is not treated as registered	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	and pins the comm itself, whose prefix does not reach its substitutions	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	and the piped form the old pattern did catch is still caught	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	every registered suite has a file	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	every suite is registered in run_all_versions.sh	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	negative control: and does not find one that is not	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	pinning only the comm does not pin its substitutions	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	positive control: the membership test finds a name that is registered	15;16;17;18;19	never	-
 harness_selftest	070-and-comm-s-two-inputs-must	premise: some suite still uses comm, or the check below is vacuous	15;16;17;18;19	never	-
+harness_selftest	070-and-comm-s-two-inputs-must	prose describing the rule does not violate it	15;16;17;18;19	never	-
 harness_selftest	080-no-suite-pipes-a-captured-string	a comment ending in a pipe is not a producer	15;16;17;18;19	never	-
 harness_selftest	080-no-suite-pipes-a-captured-string	a comment naming a TERMINATED tag still opens no heredoc, so the lines below it are seen	15;16;17;18;19	never	-
 harness_selftest	080-no-suite-pipes-a-captured-string	a comment naming a heredoc exempts nothing, so the line below it is still seen	15;16;17;18;19	never	-

test/check_ledger_budget.txt

@@ -78,4 +78,15 @@ suites_not_covered 249
 # clean union and only the COUNT had to be re-derived. Both files conflicted loudly
 # here, which is the safe half: the ledger is always right and only sometimes speaks.
 # Re-derived by the command above on the merged tree, never by adding the deltas:
-checks_never_observed_red 1340
+# 1325 -> 1334: nine arms in selftest part 070, which learned to see a
+# process-substituted sort and to read code rather than prose (#1112). Derived by the
+# command above on this tree, and by the gate's census in the same run.
+# RESEATED onto main carrying #1113 and #1119. Main states 1330, this branch stated
+# 1334, and the merged tree is neither. The ledger auto-merged SILENTLY here while the
+# budget conflicted loudly, which is the usual pairing and the reason the union was
+# checked by KEY rather than trusted: 0 main keys lost, 9 added, all in part 070.
+# Re-derived by the command above on the merged tree.
+# RESEATED onto main carrying #1114 and #1120. Re-derived by the command above on
+# the merged tree; the ledger auto-merged silently again and was checked by KEY:
+# 0 main keys lost, 9 added, all in part 070.
+checks_never_observed_red 1349

test/pytest/TESTS.md

@@ -95,6 +95,7 @@ behaviour, the source of that number is named.
 - [47. test_index_fetch_penalty_crossover.py: the correlated range must not fetch](#47-test_index_fetch_penalty_crossoverpy-the-correlated-range-must-not-fetch)
 - [48. test_parallel_scan_cost.py: a parallel custom scan must not divide I/O](#48-test_parallel_scan_costpy-a-parallel-custom-scan-must-not-divide-io)
 - [49. test_residual_is_counted.py: a residual must be counted, not subtracted](#49-test_residual_is_countedpy-a-residual-must-be-counted-not-subtracted)
+- [50. test_collation_pinned.py: comm's inputs must be sorted the same way](#50-test_collation_pinnedpy-comms-inputs-must-be-sorted-the-same-way)
 
 ## 1. How to read a test in here
 
@@ -4524,3 +4525,42 @@ and neither file names the other.
 The load-bearing assertion is that the printed breakdown takes a count a count
 can take. It is unreachable while the residual is a subtraction across two
 populations, and reachable only once it is a set difference over the names.
+## 50. test_collation_pinned.py: comm's inputs must be sorted the same way
+
+#552 established the rule and #1112 found the hole. `comm` requires both inputs
+sorted in ITS collation and does not check: fed a mismatch it writes `input is not
+in sorted order` to stderr and prints a result anyway, so where stderr lands in a
+log nobody reads, a wrong set arrives looking like an answer.
+
+The inputs are not collation-insensitive. On real suite names,
+`pgc_setup`/`pg_dump_roundtrip` and `projections`/`projection_update` both swap
+between `C` and `en_US.UTF-8`.
+
+**Two halves, and the second is the one that was missed.** `LC_ALL=C comm <(sort a)
+<(sort b)` pins only comm's own comparison: the substitutions run in subshells of
+the **parent** and inherit its locale. A guard accepting `LC_ALL=C` anywhere on the
+line would bless exactly the form a reader writes after reading the guard's name.
+
+Public seam: the shell corpus under `test/*.sh`. Read independently of
+`test/selftest/070-and-comm-s-two-inputs-must.sh` -- same corpus, own
+implementation, own planted probes, and neither file names the other.
+
+### Every arm
+
+| test | what it holds |
+| --- | --- |
+| `test_the_corpus_has_files_using_comm_so_the_sweep_is_not_vacuous` | the sweep spans more than one file, printed from the data |
+| `test_every_sort_feeding_a_comm_pins_its_collation` | no suite using `comm` leaves a sort on the caller's locale |
+| `test_every_comm_pins_its_own_comparison` | and the `comm` itself is pinned, which is a separate claim |
+| `test_the_detector_catches_the_process_substituted_form` | the hole #1112 names: not a pipeline, so a pipe pattern cannot see it |
+| `test_the_detector_still_catches_the_piped_form` | widening did not trade #552's case away |
+| `test_a_line_with_one_of_two_sorts_pinned_is_caught` | the half-pinned form, which is what a partial fix produces |
+| `test_pinning_only_the_comm_does_not_pin_its_substitutions` | a pinned `comm` over unpinned sorts is still a breach, while its comm half is satisfied |
+| `test_a_fully_pinned_line_is_not_flagged` | without which the detector could be "flag everything" and every arm above still passes |
+| `test_prose_describing_the_rule_does_not_violate_it` | a comment containing the forbidden form is not a breach -- it flagged `run_all_versions.sh` for its own text before the guard skipped comments |
+| `test_an_unpinned_comm_is_caught_and_a_word_containing_comm_is_not` | `command` and an identifier containing `comm` are not comms |
+
+Both corpus arms report zero on this tree, measured before the file was written, so
+the detector is proved by planting rather than by the corpus. The load-bearing arm
+is the process-substituted form: unreachable by a pipe pattern, and reachable only
+once the detector reads substitutions too.

test/pytest/expected_tests.txt

@@ -173,7 +173,27 @@
 # 361 here, which is a coincidence of this merge rather than a method: the deltas were
 # measured against different trees. Re-derived by collection on the merged tree, which
 # is the only resolution this number has.
-guard_tests 363
+# 355 -> 365 when test_collation_pinned.py landed (#1112): ten arms over the comm
+# collation rule -- the corpus sweep in both halves, plus seven planted probes, because
+# both corpus arms report ZERO on this tree and an arm that can only report "none" is a
+# check that cannot fail.
+#
+# NOTE FOR WHOEVER MERGES SECOND. #1115 moves this same key to 361 from 355 for six arms
+# of its own. Neither number survives and the deltas were measured against different
+# trees. Re-derive by collection on the merged tree.
+# MERGED with #1115's six arms. This branch derived 365 from a base of 355, and #1115
+# derived 361 from that same 355; neither is the merged truth. Both deltas happen to
+# land on 371 from the other's base, which is exactly the coincidence this file warns
+# about three times over -- they were measured against different trees and agreeing is
+# not evidence. Re-derived by collection on the merged tree: `371 tests collected`.
+# RESEATED onto main carrying #1113 and #1119. This branch derived 371 from a base of
+# 361 and main now states 362; neither is the merged truth. Re-derived by collection on
+# this tree: `372 tests collected`.
+# RESEATED again, onto main carrying #1114 and #1120. Main states 363, this branch had
+# 372; re-derived by collection on this tree: `373 tests collected`. This key has now
+# been re-derived four times on one branch, each time against a different base, and the
+# arithmetic would have been right by luck twice and wrong twice.
+guard_tests 373
 
 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran
 # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and

test/pytest/test_collation_pinned.py

@@ -0,0 +1,161 @@
+"""`comm`'s two inputs must be sorted in one collation, and the pin must reach them.
+
+#552 established the rule and #1112 found the hole. `comm` requires both inputs sorted
+in ITS collation and does not check: fed a mismatch it writes `input is not in sorted
+order` to stderr and prints a result anyway, so in a harness whose stderr lands in a log
+nobody reads, a wrong set arrives looking like an answer.
+
+The inputs are not collation-insensitive. Measured on real suite names,
+`pgc_setup`/`pg_dump_roundtrip` and `projections`/`projection_update` both swap between
+`C` and `en_US.UTF-8`.
+
+TWO HALVES, AND THE SECOND IS THE ONE THAT WAS MISSED. `LC_ALL=C comm <(sort a) <(sort b)`
+pins only comm's own comparison: the process substitutions run in subshells of the
+PARENT and inherit ITS locale. A guard that accepted `LC_ALL=C` anywhere on the line
+would bless exactly the form a reader writes after reading the guard's name.
+
+CODE LINES ONLY. The shell guard used to scan every line, prose included, so a comment
+explaining the rule violated it -- a note reading "reads only the piped form" contained
+the literal string the guard grepped for and flagged its own file.
+
+Read INDEPENDENTLY of `test/selftest/070-and-comm-s-two-inputs-must.sh`: same corpus,
+own implementation, own planted probes, and neither file names the other. The shell part
+is a set of `grep` pipelines over the same files; this walks them in Python.
+"""
+
+import pathlib
+import re
+
+REPO = pathlib.Path(__file__).resolve().parents[2]
+SUITES = REPO / "test"
+
+# `sort` IMMEDIATELY after a pipe or an opening process substitution. A pinned form
+# reads `| LC_ALL=C sort` or `<(LC_ALL=C sort`, which puts text between the two and so
+# does not match -- that is what makes a MIXED line fail without further work, since an
+# unpinned sort on the same line still matches.
+_UNPINNED_SORT = re.compile(r"(\|\s*sort(\s|$)|<\(\s*sort(\s|$))")
+_COMM = re.compile(r"(^|[^_\w])comm\s")
+_PINNED_COMM = re.compile(r"LC_ALL=C\s+comm\s")
+
+
+def _code_lines(text):
+    """Every line that is not a comment. Prose about the rule is not a breach of it."""
+    return [l for l in text.splitlines() if not l.lstrip().startswith("#")]
+
+
+def _unpinned_sorts(text):
+    return [l for l in _code_lines(text) if _UNPINNED_SORT.search(l)]
+
+
+def _unpinned_comms(text):
+    return [l for l in _code_lines(text)
+            if _COMM.search(l) and not _PINNED_COMM.search(l)]
+
+
+def _files_using_comm():
+    out = []
+    for p in sorted(SUITES.glob("*.sh")):
+        t = p.read_text()
+        if any(_COMM.search(l) for l in _code_lines(t)):
+            out.append(p)
+    return out
+
+
+# ---- the corpus ------------------------------------------------------------
+
+
+def test_the_corpus_has_files_using_comm_so_the_sweep_is_not_vacuous(expect):
+    """The arms below report "none". Without this they report none of nothing."""
+    names = [p.name for p in _files_using_comm()]
+    expect.at_least(len(names), 1, "at least one suite still uses comm")
+    # Printed from the data rather than retyped, so this cannot go stale.
+    expect.text("many" if len(names) >= 2 else "one", "many",
+                f"and more than one does, so the sweep spans files: {names}")
+
+
+def test_every_sort_feeding_a_comm_pins_its_collation(expect):
+    offenders = {}
+    for p in _files_using_comm():
+        bad = _unpinned_sorts(p.read_text())
+        if bad:
+            offenders[p.name] = bad
+    expect.num(len(offenders), 0,
+               f"no suite using comm leaves a sort on the caller's locale: {offenders}")
+
+
+def test_every_comm_pins_its_own_comparison(expect):
+    """Separate from the arm above, because the prefix does not reach the inputs.
+
+    Both have to hold: `comm` compares in its own locale, and each substitution sorts
+    in the parent's.
+    """
+    offenders = {}
+    for p in _files_using_comm():
+        bad = _unpinned_comms(p.read_text())
+        if bad:
+            offenders[p.name] = bad
+    expect.num(len(offenders), 0,
+               f"no comm is left on the caller's locale: {offenders}")
+
+
+# ---- the detector, proved by planting --------------------------------------
+#
+# Both corpus arms report zero today, measured before this file was written. An arm
+# that can only ever report "none" is a check that cannot fail, so every form the
+# detector must catch is planted, and the forms it must NOT catch are planted too.
+
+
+def test_the_detector_catches_the_process_substituted_form(expect):
+    """The hole #1112 names: not a pipeline, so a pipe pattern cannot see it."""
+    expect.num(len(_unpinned_sorts('\tcomm -23 <(sort "$1") <(sort "$2")\n')), 1,
+               "an unpinned process-substituted sort is caught")
+
+
+def test_the_detector_still_catches_the_piped_form(expect):
+    """The case #552 already covered. Widening must not trade one for the other."""
+    expect.num(len(_unpinned_sorts("\tcat a | sort > b\n")), 1,
+               "an unpinned piped sort is caught")
+
+
+def test_a_line_with_one_of_two_sorts_pinned_is_caught(expect):
+    """The half-pinned form, which is what a reader writes after a partial fix."""
+    expect.num(len(_unpinned_sorts('\tcomm -23 <(LC_ALL=C sort "$1") <(sort "$2")\n')), 1,
+               "pinning one of two sorts is not pinning the line")
+
+
+def test_pinning_only_the_comm_does_not_pin_its_substitutions(expect):
+    """The trap inside the trap, and the reason the two arms are separate."""
+    text = '\tLC_ALL=C comm -23 <(sort "$1") <(sort "$2")\n'
+    expect.num(len(_unpinned_sorts(text)), 1,
+               "a pinned comm over unpinned sorts is still a breach")
+    expect.num(len(_unpinned_comms(text)), 0,
+               "while the comm half of that same line is satisfied")
+
+
+def test_a_fully_pinned_line_is_not_flagged(expect):
+    """Without this the detector could be 'flag everything' and every arm above passes."""
+    text = '\tLC_ALL=C comm -23 <(LC_ALL=C sort "$1") <(LC_ALL=C sort "$2")\n'
+    expect.num(len(_unpinned_sorts(text)), 0, "a fully pinned line is clean")
+    expect.num(len(_unpinned_comms(text)), 0, "and so is its comm")
+
+
+def test_prose_describing_the_rule_does_not_violate_it(expect):
+    """A rule that cannot be written down is a rule people stop writing down.
+
+    This exact comment flagged `run_all_versions.sh` for its own text before the
+    shell guard learned to skip comments.
+    """
+    text = "# the guard reads only the `| sort` form and misses <(sort ...)\n"
+    expect.num(len(_unpinned_sorts(text)), 0,
+               "a comment containing the forbidden form is not a breach")
+    expect.num(len(_unpinned_comms("# comm needs both inputs sorted the same way\n")), 0,
+               "and neither is a comment mentioning comm")
+
+
+def test_an_unpinned_comm_is_caught_and_a_word_containing_comm_is_not(expect):
+    """`_COMM` must not fire on `_cm_unpinned_comms` or `command`."""
+    expect.num(len(_unpinned_comms("\tcomm -23 a b\n")), 1, "a bare comm is caught")
+    expect.num(len(_unpinned_comms("\tcommand -v sort\n")), 0,
+               "and `command` is not a comm")
+    expect.num(len(_unpinned_comms("\t_my_comm_helper a b\n")), 0,
+               "nor is an identifier containing it")

test/pytest/test_harness_deps.py

@@ -121,6 +121,10 @@
     # cluster, no driver. The classifier agrees, and this declaration has to say so
     # before the guard job will collect it.
     "test_residual_is_counted.py",
+    # #1112's file. An AST-free text sweep over `test/*.sh` -- no connection, no
+    # cluster, no driver -- reading the same corpus the shell part reads, with its
+    # own implementation.
+    "test_collation_pinned.py",
 ]