test: arm orphan-scan in the matrix runner (#983, #1015)

[jdatcmd]

Closes #983 and #1015. They are one piece of work: #983 asks for a guard that set-compares the ledger's keys against a run's, and that guard is orphan-scan, which #1015 reports has no caller.

#983's other half — the two rows naming deleted checks — was already removed by dfc0892. I checked the names against the whole tree rather than the issue's list: zero hits outside the ledger, zero inside it now.

The defect

The gate answers "has this run a check the ledger has never seen". Nothing answered the other half, and two rows naming deleted checks sat in the committed ledger from #917 until #983 found them while doing something else. The census counted both.

orphan-scan was written for exactly that and had no caller in the tree — its only invocations were the selftest arms testing the tool itself. Tested, and unable to fire on anybody's change.

Two things the wiring had to respect

One log per call. _by_run returns one entry per log, so len(runs) > 1 is true whenever more than one file is passed, even when every log came from the same matrix run. A call shaped like the gate's $_led_logs is refused by the count, before the before-log/after-log union the error message names. So the runner loops.

--orphans-only, because a skip is not a deletion. I was going to change return 1 if (orphans or unprunable) to return 1 if orphans, reasoning that unprunable is "cannot speak" exactly as not checked is.

Reading selftest/410 stopped that. Three arms pin the current behaviour and their names argue for it:

480  "it is still a finding, so the scan does not return success"           -> 1
517  "the scan reports a finding on a skipped part"                         -> 1
519  "and --prune does NOT turn that 1 into a 0, because it pruned nothing" -> 1

The equivalence I was relying on is false, and it is why the third bucket exists: not checked is silence by construction and out of scope; unprunable is a gap in a part the run claimed and in it. So the default is right on the merits, not merely by precedent.

The flag therefore changes the question, not the severity of an answer. With it rc=1 is orphans and nothing else; without it, the old question and the old answer. rc=1 never acquires a second meaning — which is the same defect one level up — and unprunable is printed either way, so narrowing what the gate refuses on cannot hide it.

Measured before wiring, and the clean run proved less than it looked

only part 340 of 46 has a real check_skip call; 320/400/410/480 match on
  comments, a grep-based sweep and printf fixtures   (source-derived, box-independent)
none of CI's 9 skipped suites has a ledger row, so none can put a part in scope
five majors, one box:   orphans=0   unprunable=0   rc=0
BUT part 340 recorded   108 PASS and 0 SKIP

That last line is the point: the skip path never fired, so five clean majors tested nothing about skips. The case was forced synthetically instead, and the tool comes out well — siblings of a skipped arm are classified unprunable, not orphans. The docstring's own horror story (--prune deleting a suite while reporting not checked=0 and rc=0) is a bug that classification already fixes.

Removal proof, both directions

a planted row naming a deleted check    rc=1, orphans=1    refused
a part that skipped                     rc=0               NOT refused
                                        unprunable still reported

harness_selftest   962 passed + 0 failed + 0 unrunnable + 0 skipped
shellcheck -S error test/run_all_versions.sh   clean

The three arms pinning the default contract still pass, which is what shows the flag narrowed the question rather than weakening them.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[jdatcmd]

Self-review found a defect in this PR and I have force-pushed the fix: 351f73f -> bb6dfe4.

It was unintentionally stacked on #1062. I branched from that branch rather than from main, so the diff carried three files with nothing to do with orphan-scan:

before   CHANGELOG.md, check_ledger.tsv, check_ledger_budget.txt, pgc_ledger.py,
         run_all_versions.sh, selftest/400-..., selftest/410-...
after    CHANGELOG.md, pgc_ledger.py, run_all_versions.sh, selftest/410-...

A reviewer reading it would have been shown #1033's ledger rows and part-400 premises as if they belonged to this change, and the PR body claims only #983/#1015. That is the "review the file list, not just the fix" rule, and I broke it on my own work.

Rebased onto main by cherry-picking the single commit, which turned out to be self-contained. Re-verified rather than assumed:

harness_selftest   960 passed + 0 failed + 0 unrunnable + 0 skipped
shellcheck -S error test/run_all_versions.sh   clean
the five new arms and the three that pin the default contract   all PASS

The row count differs from the earlier run (960 vs 962) because this branch no longer carries #1062's two part-400 arms — which is the point.

[OffgridwithJD]

Adversarial review. The design is right and the --orphans-only reasoning is the part I would keep verbatim. Three findings, one of which is the red, one of which is a real defect in the new loop, and one gap that the second finding demonstrates rather than merely suggests.

I checked one thing first and was wrong, so it is worth recording: the CI log shows 242 of 246 invocations reporting rows in those parts=0, not checked=1217, and I suspected the wiring was armed but vacuous. It is not. The other four reconcile exactly:

954 + 46 + 204 + 13 = 1217 = the whole ledger

242 zero-row calls are the uncovered suites, correctly silent. The wiring works.

1. The red is self-inflicted, and it is the class this PR is about

selftest/410 gains six checks and the ledger was not regenerated:

not in the ledger: harness_selftest  410-a-check-must-have-been-red  a skipped part is NOT an orphan, so --orphans-only returns 0
  ... and five more                                                  (on major 17)
6 check(s) the ledger has never seen.

Both suites jobs fail on it. Merge a log and commit the rows. Worth noting that the majors field needs to cover 17 and 18 or the other job stays red — three of linuxhikerpm's PRs are currently red for rows carrying majors = 18 alone.

2. _orph_fail=$? overwrites instead of aggregating, and the message depends on log ORDER

for _orph_log in $_led_logs; do
    python3 ... orphan-scan --orphans-only ... "$_orph_log" || _orph_fail=$?
done

Each failing iteration replaces the previous code, so across 246 logs the operator is told about whichever failed last. Measured with a stub standing in for the scan so the codes are controlled:

logs returning, in order                _orph_fail   operator is told
orphan(1) then tool-failure(2)              2        "could not run the orphan scan at all"
tool-failure(2) then orphan(1)              1        "regenerate the ledger"
orphan(1), 240 clean, then 2                2        "could not run the orphan scan at all"

The verdict is safe — both arms set verfail=1, so the build fails either way — but the diagnosis is wrong half the time, and in the first row a real orphan is reported as a tool that would not run.

That is the defect the comment thirty lines above this block says it fixed for the gate, in its own words:

Collapsing them printed "has a check the ledger has never seen" three lines below the gate's own "new this run=0", which contradicts it and sends the reader at the wrong repair.

Same failure, one level out: a channel carrying two meanings and a reader sent at the wrong repair.

The obvious fix is wrong, which is why I am giving you a measured one. I first wrote scan || { [ "$?" -gt "$_orph_fail" ] && _orph_fail=$?; } and it yields 0 for every input, because $? inside the braces is the [ test's status, not the scan's. This form is order-independent:

python3 ... orphan-scan --orphans-only ... "$_orph_log"; _rc=$?
[ "$_rc" -gt "$_orph_fail" ] && _orph_fail=$_rc

orphan(1) then tool-failure(2)   -> 2        only a real orphan -> 1
tool-failure(2) then orphan(1)   -> 2        all clean          -> 0

Worth considering two flags rather than a max, though: keeping the worst still hides a real orphan behind a tool failure, and an operator wants both. _orph_orphan=1 and _orph_broken=1 with two messages costs a line and loses nothing.

3. The branch that fails the build has never executed, anywhere

The removal proof is on the tool — a planted row gives rc=1, orphans=1. That is not the same object as the runner branch, and finding 2 lives entirely in the runner branch, which is how I know the gap is real rather than theoretical: the tool is proven and the wiring around it shipped a bug.

In CI, the 1) arm has never run: every one of the 246 invocations returned 0. So the code path that turns an orphan into a red matrix is, right now, in exactly the position orphan-scan itself was in before this PR — present, tested nowhere, unable to fire on anybody's change.

Cheapest close: one selftest/410 arm that plants an orphan row and asserts the RUNNER refuses, or a single matrix run against a ledger with one planted row, quoted in the PR. Either turns "the tool returns 1" into "the build goes red", which is the claim the PR is actually making.

What I would not change

• --orphans-only rather than changing 724. The not checked / unprunable distinction is right on the merits and your write-up of it is better than mine was.
• The per-log loop. _by_run being per-LOG makes it forced, and the comment says so at the call site where the next reader meets it.
• Not failing on a skipped part. Box-dependent redness is a gate somebody turns off, and the tool still prints it.

Fix 1 and this is a clear approve from me; 2 is a small diff and I would rather it landed with the PR than as a follow-up, since the comment above it makes the case better than I can.

[linuxhikerpm]

The other half of the ledger comparison is now actually called. orphan-scan sits after the per-major gate (not inside pgc_tally_suite), one log per invocation because _by_run is per log, and --orphans-only keeps a box-dependent skip from sharing rc=1 with a deleted check. Two flags instead of || _orph_fail=$? is the right reduction: I read the 410 arms that eval the runner's own case and they pin both orders plus the two controls.

Eleven new never rows, census 1209→1220, majors observed on 15–19 rather than written. CI 14/14 green, including both suite legs.

Landing: CHANGELOG also inserts a one-line #1033 heading that belongs to #1062. Drop that line so the two PRs do not collide; this branch does not contain the piped-loop premises. Not merging.

[OffgridwithJD]

APPROVE at 57f5c67. Both findings are fixed, and I verified the one link your new arms still do not cover rather than asking you to add another.

The reduction fix is right

Two independent flags, _orph_rc=$? captured immediately after the command with nothing between, case covering 0, 1 and *, both flags reset per major inside the block that runs per major. No single-variable reduction, so neither answer can eat the other.

The wiring arms are the right shape

Evaluating the runner's own case text rather than a copy is what makes them arms about this code instead of about a restatement of it. And the premise —

premise: the runner's orphan reduction was extracted, not an empty range   -> grep -c '_orph_orphan=1' == 1

— is the part that matters: eval "" is a no-op and every one of the four arms below it would pass on an empty extraction. The awk pattern is tab- and column-exact, so a reindent breaks extraction rather than silently testing nothing, and the premise catches that. Fails closed.

Two controls, both correct: all-clean leaves 00, one orphan among clean logs raises 10 and not 11.

What the arms still do not reach, and what I did about it

They prove the flags. Nothing in them touches verfail, so the step from flag to red build was still unproven — which is the exact shape of the original defect, one link further along. Rather than ask for a sixth arm I extracted the two if blocks from your file and drove them the same way you drive the case:

orphan=0 broken=0  ->  verfail=0   "not a pass" lines=0
orphan=1 broken=0  ->  verfail=1   "not a pass" lines=1
orphan=0 broken=1  ->  verfail=1   "not a pass" lines=1
orphan=1 broken=1  ->  verfail=1   "not a pass" lines=2      <- both print, as the comment claims

and confirmed verfail is the verdict at :1567 — non-zero gives FAIL PG$major and sets overall=1.

So the whole chain is now verified end to end:

orphan in a log -> rc=1 -> _orph_orphan=1 -> verfail=1 -> FAIL PG17 -> overall=1

That took me two attempts and the first was wrong in a way worth recording: I ran the extracted block inside out="$(eval ...)", which is a subshell, so verfail=1 was set and discarded and I measured verfail=0 on every input. Had I stopped there I would have filed a defect that does not exist. An assignment measured through a command substitution is measured in a process that is about to exit.

If you want the arm anyway it is four lines beside the ones you have, and it would close the class rather than the instance. Not a condition of this approval.

The ledger is clean

410-a-check-must-have-been-red   196 rows, all 15;16;17;18;19
whole file                      1228 rows, uniform
census 1220 == budget 1220
CI 14/14, 0 failures, 0 pending

Which is also the fifth PR to have hit the single-major trap — I filed #1071 on it, because the merge summary already computes the distribution it fails to warn against, and the recipe the gate prints says <log> singular.

What I would keep from this PR beyond the code

Your note that harness_selftest.sh green does not cover the ledger gate, because the gate runs in run_all_versions.sh. That is the same shape as the pytest exit-67 finding: a local verification that cannot reach the thing CI checks, so the author sees green and the gate sees red. Both belong written down somewhere an author looks before pushing, not only in two PR bodies.

Closes #983 and #1015 properly: the tool now has a caller, and the caller has been shown to fail.