fix(ci): discover local guards and report required skips - #427
Conversation
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 153998c3b8a8c6c3217cd71a32d7a7be17bef5c4.
The local gate was running 21 guards while CI ran 28. I enumerated it myself rather than taking the number from the body:
$ ls tests/release/*.test.sh tests/e2e/*.test.sh | wc -l
28
$ grep -c "run_step 'hygiene: .*\.test\.sh'" scripts/ci-local.sh
21
$ for f in tests/release/*.test.sh tests/e2e/*.test.sh; do
grep -q "$(basename "$f")" scripts/ci-local.sh || echo "MISSED: $f"; done
MISSED: tests/release/cassette-replay-parity.test.sh
MISSED: tests/release/codex-plugin-manifest.test.sh
MISSED: tests/release/grub-kargs-edit.test.sh
MISSED: tests/release/log-edit.test.sh
MISSED: tests/release/mount-edit.test.sh
MISSED: tests/release/no-secrets.test.sh
MISSED: tests/release/rmswap.test.sh
Exactly the seven you named. Four of them (grub-kargs-edit, log-edit, mount-edit, rmswap) are privileged-helper safety guards and one is the secret scanner, so the guards most worth catching before a push were the ones the pre-push hook never ran. A hand-maintained list drifting from disk is the failure, and replacing it with discovery is the correct fix rather than adding seven more lines that would drift again.
Two details I checked specifically.
An empty discovery is a hard failure, not a silent pass:
if ((count == 0)); then
record FAIL 'hygiene: no release/e2e shell tests discovered'
fiand record FAIL feeds hard_failures, which the script's final ((hard_failures == 0)) turns into a non-zero exit. Your test proves that direction by running the extracted function against an empty temp tree and asserting failures=1, then adding a file and asserting failures=0. That is proving the guard fails when it should, not asserting that it would.
You also avoided the trap I went looking for. There is no mapfile -t x < <(cmd) here; you iterate two globs with [[ -f "$test" ]] || continue, so an unmatched glob leaves the literal pattern, gets skipped, and lands in the count == 0 branch. Nothing swallows an error into an empty set.
The test harness is unusually good for a shell gate. Extracting the real production functions by regex and running them with stubbed record/run_step/have means you are exercising the shipped code rather than a paraphrase of it, and the explicit assertTrue(disk, 'empty discovery must never pass') guards the harness itself against going vacuous.
Two optional notes:
print_summary() exists partly so the test's functions() regex can extract it, and its body is unindented. That is test shape leaking into production style. Harmless, slightly odd to read.
Preferring Podman when both runtimes are present is a real behavior change that is not in the title. It is in the CHANGELOG and covered by test_podman_precedes_docker, so I am not asking you to remove it, but a contributor with both installed will see a different runtime chosen than yesterday.
One coordination note, not your fault: #423 adds a run_step line inside the same block you delete, so the two conflict. I confirmed with git merge-tree rather than guessing:
$ git merge-tree $(git merge-base p423 p427) p423 p427 | grep -c '<<<<<<<'
1
Resolution is yours: take run_shell_tests, and the glob picks up their shellcheck-coverage.test.sh automatically, which your own test then enforces. I will sequence the two.
Note that INCOMPLETE still exits 0, which you documented in the developer guide. That is the right call for the pre-push path since --fast never enters the Postgres group, but it does mean a caller reading only $? cannot tell PASS from INCOMPLETE. Worth keeping in mind if anything ever automates on top of it.
Approving. The seven recovered guards are the kind of gap that only shows up when someone counts.
One CHANGELOG conflict, both bullets kept. lacs-project#416 landed while this sat in the queue; the two entries are independent and neither replaces the other. Conflict caused by my merge order, resolved by me. Nothing of the pull request's own diff changed.
vladimirrott
left a comment
There was a problem hiding this comment.
Re-approving at 0237813b. The push that dismissed the approval was mine: I resolved the CHANGELOG.md conflict my own merge order created, rather than leaving it on your desk.
Both bullets kept, #416's first because it landed first:
### Fixed
- Align planner action descriptions with the Ubuntu execution fence, including
Ufw and the other tool families moved to shared or Debian-family support.
Guard both directions of the description/fence relationship (#416).
- Discover release/e2e shell guards in local CI instead of maintaining a stale
list. Report skipped required Postgres validation in the final summary and
prefer Podman when both container runtimes are available (#346).
Relative to main the pull request is now five files and nothing else:
.github/workflows/ci.yml | 3 ++
CHANGELOG.md | 3 ++
docs/developer-guide.md | 13 +++--
scripts/ci-local.sh | 68 +++++++++++++-----------
tests/release/ci-local.test.sh | 116 +++++++++++++++++++++++++++++++++++++
Your Cargo.lock hunk has dissolved, which is good news rather than lost work: #426 carried the identical rustls bump onto main an hour ago, so main is already at 0.23.45 and your copy is a no-op now. Four of you sent that same bump; the first one through took it.
I enumerated the seven tests the old hardcoded list missed independently before believing the count: 28 *.test.sh on main, 21 named in run_hygiene_group, seven not. Replacing a list with a glob is the right shape here because the failure mode was a list going stale in silence, and record FAIL 'no release/e2e shell tests discovered' when the glob matches nothing is what keeps the replacement from having the same problem in reverse.
One thing that is not blocking and that I would leave alone. print_summary exits 0 on INCOMPLETE, so a full local run with no container runtime prints the warning and still reports success to the caller. That is the right call for a convenience script every contributor runs, and the required checks are enforced by CI, not here. Worth a sentence in docs/developer-guide.md next to the new warning so nobody reads exit 0 as "everything ran".
Fixes #346.
The local hygiene gate now discovers all release/e2e
*.test.shfiles instead of maintaining a list that omitted seven CI guards. There are no deliberate exclusions, and an empty discovery is a failure. Missing or explicitly skipped Postgres validation now produces an INCOMPLETE final summary naming the required CI gate and both remedies. Podman is preferred when both runtimes are installed. As before, skips are nonfatal; actual check failures return nonzero.Validation on Windows/Git Bash:
bash -n scripts/ci-local.shandgit diff --check: PASS.The new regression is invoked by CI and by local discovery. CI also surfaced the newly published RUSTSEC-2026-0285 in the inherited lockfile. A separate commit updates rustls to 0.23.45 and its required TLS dependencies; the final security-audit check passes.
Codex assisted with implementation and validation.