Skip to content

fix(ci): require every release test to reach a gate - #377

Open
be-student wants to merge 11 commits into
lacs-project:mainfrom
be-student:codex/331-release-test-reachability
Open

be-student wants to merge 11 commits into
lacs-project:mainfrom
be-student:codex/331-release-test-reachability

Conversation

@be-student

@be-student be-student commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Add a gate that discovers every tests/release/*.test.sh and tests/e2e/*.test.sh file and requires a standalone bash <path> command in a parsed workflow step's run field in ci.yml, e2e.yml, or release.yml. Comments, artifact inputs, heredoc text, path substrings, echo commands, and compound commands do not satisfy this convention. This checks static invocation presence; it does not evaluate job conditions or guarantee runtime execution.

The check rejects empty suites, missing gate files, and a missing YAML parser. It uses PyYAML from the existing yamllint prerequisite, documented for the same Python environment that runs the gate. Wire it into GitHub CI and ci-local.sh, document the convention, and derive the repository root from the script location without an unused positional argument. Current upstream main is merged without rewriting published history.

Related Issue

Closes #331.

Validation

  • Before the parser refinement, all 29 release/E2E shell scripts and four supporting gates passed in Linux, and all 126 setup tests passed with Node 24 and real netcat available.
  • On the final parser refinement, Linux and host regression fixtures, the reachability gate, evidence checker, and ShellCheck passed.
  • Regression fixtures cover orphan tests, empty suites, missing workflows, artifact-block and heredoc text, and the distinction between filename suffixes and shell comments. The scalar and filename-suffix cases demonstrated failure before their fixes and success afterward.
  • ShellCheck passed for maintained scripts; YAML lint exited zero with four existing issue-template warnings; changed Markdown and diff whitespace checks passed.
  • Rust suites were not rerun under the repository's explicit non-Rust contribution exemption. The upstream rustls advisory remains present and the maintainer is handling that dependency fix separately.
  • Remote CI must validate the new commit before merge.

The Linux validation used task-owned temporary storage after Docker disk exhaustion; existing shared Docker resources were preserved. Tests exercised the actual shipped guard, not a rewritten substitute.

Notes for Reviewers

OpenAI Codex implemented and tested this change, with an independent Astra agent review before publication. No human manual testing or personal verification is claimed.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 5c1bf6196d92608fb70947f9db8d314468575cef.

You built the check #331 described and you got the part most people miss: the empty-suite case fails instead of passing over zero files. That explicit (( ${#tests[@]} == 0 )) is the whole reason this gate is worth having, because the defect it guards against is a check that inspected nothing, and a check with that hole reproduces the bug one level up.

What I verified

Everything ran in rootless podman with the tree mounted read-only and no network, since a fork's shell scripts execute as this user otherwise.

Your gate passes on your own tree:

$ podman run --rm --network=none -v "$PWD/trees/p377pure:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh
test-reachability: every release and E2E test is invoked by a gate.
exit=0

#331 asked for both directions, so I ran both. An orphan test, using the file PR #379 is currently adding:

$ podman run --rm --network=none -v "$PWD/trees/merged1:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh
test-reachability: test is not invoked by a gate: tests/release/mdbook-links.test.sh
exit=1

A deleted invocation, with every mention of public-claims.test.sh rewritten out of ci.yml and ci-local.sh:

$ podman run --rm --network=none -v "$PWD/trees/p377mut:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh
test-reachability: test is not invoked by a gate: tests/release/public-claims.test.sh
exit=1

An empty release suite, every *.test.sh renamed to *.test.sh.disabled:

$ podman run --rm --network=none -v "$PWD/trees/p377empty:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh
test-reachability: no tests discovered in tests/release/*.test.sh
exit=1

And the directory gone entirely, which is where a find under process substitution usually swallows the failure:

$ podman run --rm --network=none -v "$PWD/trees/p377pure:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash -c 'sed -i "s#\$repo_root/tests/\$suite#\$repo_root/tests/NOSUCHDIR#" /tmp/x 2>/dev/null; cp scripts/check_test_reachability.sh /tmp/c.sh; sed -i "s#tests/\$suite#tests/NOSUCHDIR#" /tmp/c.sh; bash /tmp/c.sh /repo'
find: '/repo/tests/NOSUCHDIR': No such file or directory
test-reachability: no tests discovered in tests/release/*.test.sh
exit=1

Four for four. Shellcheck is clean at the severity CI uses:

$ shellcheck --severity=warning /tmp/sc_*.sh
$ echo "SHELLCHECK_EXIT=$?"
SHELLCHECK_EXIT=0

I read the ci.yml hunk myself rather than trusting the summary. It adds one step and touches no trigger and no permissions block. Your whole board is green.

Blocking: the gate has no test of its own

The five runs above are the proof this check works, and all five live in my terminal. Nothing in the repository holds them. scripts/check_test_reachability.sh is a scripts/ file rather than a tests/release/*.test.sh, so its own rule does not reach it, and a future edit that turns the (( ${#tests[@]} == 0 )) branch into a warning, or drops a return 1, passes every check on the board.

That is the exact failure this gate exists to stop, and it is the one place the gate cannot look. #331 asked for the two directions as committed cases. Both of your sibling PRs ship one: #378 has tests/release/release-version-pins.test.sh, #379 has tests/release/mdbook-links.test.sh. A tests/release/test-reachability.test.sh that builds a fixture tree, plants an uninvoked *.test.sh, asserts non-zero and asserts the message names the file, plus an empty-suite case, would carry what I ran. Reuse the fixture-and-assert shape from your own #378 test; it is already the right shape.

That is the only change I am asking for.

Merge order, which is mine to manage and not a change request

#379 adds tests/release/mdbook-links.test.sh and wires it into .github/workflows/docs.yml alone, which is not in your gate_files array. Whichever of the two lands second turns docs-and-hygiene red on main. That is the second run quoted above.

I am not asking you to add docs.yml to gate_files. It fires only on push to main, so a test reachable only from there never runs on a pull request, and calling that wired would weaken the guarantee your gate is making. I have asked #379 to wire its test into ci.yml instead, which fixes both that collision and a gap in that PR. I will hold the order so neither of you gets a red board from the other's work.

One optional note

check_suite has no explicit return 0, so its status is whatever the last if produced. That is 0 today, and I checked. An explicit return 0 would stop the next edit inside the loop from changing the function's answer by accident.

You already have #346 open from me and unanswered, so I am not putting a second issue in front of you. It stays yours.

Someone who reaches for the empty-glob case before being asked owns a build system other people depend on. If you run Linux where an outage costs you something, I would like to hear how SysKnife holds up on it. npx sysknife-setup is the whole install, and sysknife doctor, sysknife history and sysknife audit verify mutate nothing. No bearing on this review either way.

@be-student

Copy link
Copy Markdown
Contributor Author

Added the requested committed gate regression in c0291a7. It proves an orphan test fails with its path, proves an empty suite fails, and is wired into both CI and ci-local; the checker also returns success explicitly. The regression, production checker, and shellcheck all pass.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at c0291a7ea19ecbac577ce09285d4402529df51a0.

You closed the gap I asked about, and the test you wrote is the real thing rather than a restatement of the checker. It copies the production script into a fixture tree and calls it, so the assertion cannot drift into a parallel copy of the logic it proves. That distinction is the reason the previous version of this concern existed at all.

The test holds the checker down

I broke the checker three ways and left your tests/release/test-reachability.test.sh untouched. All in rootless podman with the tree read-only and no network, because a fork's shell scripts otherwise run as this user. Output is captured into a variable before I read the status, since a pipe would hand me tail's exit code:

$ BASE=/tmp/skrev377.P4pYfn
$ for t in pure m1 m2 m3; do
    echo "===== $t ====="
    out=$(podman run --rm --network=none -v "$BASE/$t:/repo:ro" -w /repo \
      docker.io/library/debian:stable-slim bash tests/release/test-reachability.test.sh 2>&1); rc=$?
    printf '%s\n' "$out" | grep -v '^time='
    echo "exit=$rc"
  done
===== pure =====
test-reachability test: orphan and empty-suite failures validated.
exit=0
===== m1 =====
test-reachability test: empty suite unexpectedly passed
exit=1
===== m2 =====
test-reachability test: orphan test unexpectedly passed
exit=1
===== m3 =====
test-reachability test: empty suite unexpectedly passed
exit=1

The three mutations, each applied to scripts/check_test_reachability.sh and read back before running:

  • m1 the empty-suite branch's return 1 becomes return 0
  • m2 the orphan branch's return 1 becomes return 0
  • m3 the whole if (( ${#tests[@]} == 0 )) block deleted

Each names which case broke. The five runs that used to live only in my terminal are now in the repository, which was the whole of my last request.

Gates

Your branch merged onto current main (adab560d + c0291a7e, merge 9755d169, clean, 96 insertions and no deletions):

$ out=$(podman run --rm --network=none -v "$WT:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh 2>&1); rc=$?
$ printf '%s\n' "$out" | grep -v '^time='; echo "exit=$rc"
test-reachability: every release and E2E test is invoked by a gate.
exit=0

All 28 release and E2E test files on that merged tree resolve, so nothing that landed on main while you were working turns your gate red. Your new test file is itself wired into ci.yml and ci-local.sh, so the gate's own rule reaches it.

Shellcheck at CI's severity on the three scripts you touched came back SHELLCHECK_EXIT=0. I read the ci.yml hunk rather than its summary: two steps added, no trigger touched, no permissions block touched. Eleven checks green, none pending.

Blocking, and this one is my miss rather than yours

gate_files counts scripts/ci-local.sh as a gate, and no workflow runs that script:

$ grep -rn 'ci-local' /tmp/skmerge377.SKoaOY/.github/workflows/ || echo "(no workflow runs ci-local.sh)"
(no workflow runs ci-local.sh)

So a test named only there satisfies your checker while no CI job will ever run it. I planted one to be sure rather than reasoning about it:

$ touch "$BASE/localonly/tests/release/never-runs-in-ci.test.sh"
$ printf "    run_step 'hygiene: never-runs-in-ci.test.sh' bash \"\$repo_root/tests/release/never-runs-in-ci.test.sh\"\n" >> "$BASE/localonly/scripts/ci-local.sh"
$ out=$(podman run --rm --network=none -v "$BASE/localonly:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh 2>&1); rc=$?
$ printf '%s\n' "$out" | grep -v '^time='; echo "exit=$rc"
test-reachability: every release and E2E test is invoked by a gate.
exit=0

Your CONTRIBUTING.md paragraph says an uninvoked test "makes both local and remote CI fail". For that file it makes neither fail, so the gate reports a guarantee it is not providing, which is what this repository fails CI over everywhere else.

That array was already in the head I reviewed first, and I read it and thought about this exact question. I told you not to add docs.yml because a push-only workflow never runs on a pull request, so a test reachable only from there is not gated. ci-local.sh is the weaker case of the same argument and it was sitting in the array while I wrote that paragraph. Yours to fix, mine to have missed.

One line, and I ran it before suggesting it:

     "$repo_root/.github/workflows/release.yml"
-    "$repo_root/scripts/ci-local.sh"
 )
$ for t in fixmerged fixlocalonly; do
    echo "=== $t ==="
    out=$(podman run --rm --network=none -v "$BASE/$t:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh 2>&1); rc=$?
    printf '%s\n' "$out" | grep -v '^time='; echo "exit=$rc"
  done
=== fixmerged ===
test-reachability: every release and E2E test is invoked by a gate.
exit=0
=== fixlocalonly ===
test-reachability: test is not invoked by a gate: tests/release/never-runs-in-ci.test.sh
exit=1

Green on the real merged tree and red on the planted case. It costs no coverage, because every one of the 28 tests is named in ci.yml today; I checked each one rather than assuming. A third fixture case next to your other two would pin it.

If you would rather keep a mention in ci-local.sh meaningful, requiring at least one workflow match and treating ci-local.sh as additional does the same job. Either shape is fine by me, and I am not asking for anything else.

This costs you no calendar time. The PR touches .github/workflows/**, so it waits on me reading that diff by hand at merge whatever else happens.

Not a change request

I am not asking you to add docs.yml, for the reason above, and #379 is still the PR I asked to move its test into ci.yml. I am holding the merge order so neither of you inherits a red board from the other.

Next

#331 is yours and this closes it. #369 is the offer of mine you have not answered yet, and it stays held for you, so I am not putting a third thing in front of you tonight.

@be-student

Copy link
Copy Markdown
Contributor Author

Addressed the review in a95af9f. The reachability checker no longer treats ci-local.sh as a release gate, and the fixture proves a test referenced only there remains unreachable. The reachability tests and ShellCheck pass.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at a95af9fbbb048da074d1da41747f4d5d82cb8c6d.

You did both halves of what I asked. gate_files is down to the three workflows, and the third fixture case pins the exclusion rather than leaving it to a comment:

touch "$fixture/tests/e2e/reachable.test.sh" "$fixture/tests/release/local-only.test.sh"
printf '%s\n' 'run: bash tests/release/local-only.test.sh' >> "$fixture/scripts/ci-local.sh"
if output="$(bash "$fixture/scripts/check_test_reachability.sh" "$fixture" 2>&1)"; then
    printf 'test-reachability test: local-only test unexpectedly passed\n' >&2

That is the shape that keeps working after both of us have forgotten why the array is three entries long.

maintainer screen 377 returns DO NOT EXECUTE, so nothing ran on the host. All of the below ran in docker.io/library/bash:5 under rootless podman, --network=none, tree from git archive.

What I proved

The gate bites, and its own test holds it down:

$ out=$(bash /home/entropia/.local/state/sysknife-maint/pt.sh p377 bash -c "$(cat /tmp/sk377proof.sh)" 2>&1); rc=$?; printf '%s\n' "$out" | grep -v '^time='; echo "=== wrapper exit=$rc ==="
### A. orphan direction: add tests/release/orphan.test.sh, gate must refuse and name it
test-reachability: test is not invoked by a gate: tests/release/orphan.test.sh
exit=1

### B. the doc-following case: a new release test wired ONLY into scripts/ci-local.sh
###    (CONTRIBUTING.md:161 says ci-local.sh counts as a gate)
423:    run_step 'hygiene: local-only' bash "$repo_root/tests/release/local-only.test.sh"
test-reachability: test is not invoked by a gate: tests/release/local-only.test.sh
exit=1

### C. mutation: neuter the gate check in the script; the PR's own test must go red
27:        if false; then
test-reachability test: orphan test unexpectedly passed
exit=1
=== wrapper exit=0 ===

Your stale base is not a problem. main grew two release tests since d881d49b (release-version-pins.test.sh, test-baseline-provenance.test.sh), and neither your script nor your test carries a hardcoded list, so discovery picks them up. I merged the trees and ran the gate against the result rather than reasoning about it:

$ git merge-tree --write-tree origin/main p377
rc=0  tree=8bc99db2ffe3b0ec37bc8466df00ef1ab3cc316c

$ out=$(bash /home/entropia/.local/state/sysknife-maint/pt.sh 8bc99db2ffe3b0ec37bc8466df00ef1ab3cc316c bash -c 'cd /repo; echo "--- release tests present: $(ls tests/release/*.test.sh | wc -l) ---"; out=$(bash scripts/check_test_reachability.sh 2>&1); rc=$?; printf "%s\n" "$out"; echo "gate exit=$rc"; echo "--- and its own test ---"; out2=$(bash tests/release/test-reachability.test.sh 2>&1); rc2=$?; printf "%s\n" "$out2"; echo "test exit=$rc2"' 2>&1); printf '%s\n' "$out" | grep -v '^time='
--- release tests present: 22 ---
test-reachability: every release and E2E test is invoked by a gate.
gate exit=0
--- and its own test ---
test-reachability test: orphan, empty-suite, and local-only failures validated.
test exit=0

Twenty-two release tests on the merged tree, all reachable. I read the ci.yml hunk in full rather than its summary: two steps added under docs-and-hygiene, no trigger, no permissions, no action pin touched.

Blocking, and it is one token

CONTRIBUTING.md:161 still describes the behaviour I asked you to remove:

then requires each exact path to appear in `ci.yml`,
`e2e.yml`, `release.yml`, or `ci-local.sh`. Add the invocation in the same
change as a new test; an uninvoked test makes both local and remote CI fail.

gate_files has three entries and your own test asserts that a ci-local.sh-only mention fails the gate. Case B above is that exact scenario: a contributor who follows the written instruction gets exit=1 and a diagnostic naming their test rather than the wrong sentence.

-`e2e.yml`, `release.yml`, or `ci-local.sh`. Add the invocation in the same
+`e2e.yml`, or `release.yml`. Add the invocation in the same

The prose was correct against the code I asked you to change, so this is fallout from my own request landing in one file and not the other.

Not blocking

Nothing asserts the gate reaches remote CI. Every assertion in tests/release/test-reachability.test.sh runs against $fixture; the real ci.yml is never read. Revert the .github/workflows/ci.yml hunk and keep the script and the test, and the whole suite stays green while GitHub stops running the gate. One line greping the real ci.yml for scripts/check_test_reachability.sh would close it. Your call, and not something I want to hold the merge for.

Separately, and not yours: ci.yml runs 21 release tests while run_hygiene_group in ci-local.sh runs 14. Your gate proves test → workflow, which is the direction that matters; gate ↔ gate parity is a different check and a different issue.

The merge is armed

Approving at the head above once the doc line moves. Push the one-token change and I will merge on my next pass, on these conditions, all of which I check in the same shell as the merge:

  1. git diff a95af9fb..<new-head> --stat -- 'scripts/**' 'tests/**' '.github/**' 'crates/**' 'apps/**' is empty. A CONTRIBUTING.md-only diff passes; anything touching the script, the test or the workflow is a different PR and gets a full re-review.
  2. The mutation above still turns the test red on the new head.
  3. Board fully green, zero failing and zero pending, ci and e2e both.
  4. My approval survives the push.
  5. gh api user --jq .login is the repository owner.

If your push carries anything else, the merge stops and you get another review instead of silence.

Next

#387 is yours if you want it: the no-secrets gate stays green when you delete its Slack pattern, and its tree scan passes on an empty file list. Same failure class you have now closed twice, one gate over. You hold #331 (this PR) and an unanswered offer on #369, so I am naming it rather than assigning it; say the word and I will hold it for you.

Three PRs in and all three about whether a check can go red. If you administer Ubuntu boxes yourself, sysknife "which services failed and how full is /var" plans and previews without executing anything, and that read-only path is the honest place to judge whether the tool earns its keep. Entirely optional.

vladimirrott
vladimirrott previously approved these changes Sep 14, 2026

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 9f89c0af5a55cb61c55e22c3cf5f62d0680be3c6.

The thing I want to single out is the empty-suite check in check_test_reachability.sh:

if (( ${#tests[@]} == 0 )); then
    printf 'test-reachability: no tests discovered in tests/%s/*.test.sh\n' "$suite" >&2
    return 1
fi

A reachability gate that discovers zero tests and reports success is the exact shape this repo keeps getting bitten by, and you closed it in the same commit that created the risk. Your fixture test then proves the failing direction rather than asserting it, and it does so by copying the real script into the fixture and running that copy, so the test exercises shipped code instead of a paraphrase. That is the difference between a guard and a decoration.

I also checked the matching half, since grep in a pipeline is where this class of gate usually dies. Line 28 is grep -Fq -- "$relative_path" "${gate_files[@]}", with grep opening the files itself. No pipeline, so no pipefail masking, and a missing gate file exits 2 and trips the guard. It fails closed.

One thing that is not your fault and that I want to state plainly: your branch left main on 6 September, and a two-dot diff against today's main shows 3,507 deletions that your commits never made. I checked before believing it:

$ git diff origin/main...p377 --shortstat
 5 files changed, 106 insertions(+)
$ git diff origin/main..p377 --shortstat
 92 files changed, 641 insertions(+), 3507 deletions(-)

Zero deletions from the merge base. Your PR removes nothing. A rebase would make this PR much easier for the next person to read, but nothing is wrong with the change itself.

Two optional notes, neither blocking:

  1. repo_root="${1:-...}" is a parameter no production caller passes. The test copies the script into $fixture/scripts/, so the default dirname/.. already resolves to the fixture and every assertion still passes with the parameter removed. CLAUDE.md asks for no params "just in case", so I would drop it.
  2. CONTRIBUTING.md says the check requires each "exact path" to appear in a gate file, but grep -Fq -- matches a substring anywhere, including inside a comment or an upload-artifact glob. Either anchor on run: bash <path> or soften the prose. I would rather the code got stricter.

Separately, and affecting every open PR rather than yours: RUSTSEC-2026-0285 against rustls published today, and main itself is red on cargo audit because of it. I confirmed on main at 61b3a878:

Crate:     rustls
Version:   0.23.39
ID:        RUSTSEC-2026-0285
Solution:  Upgrade to >=0.23.45

So if you see security-audit go red, it is mine to fix and not something you introduced. I am landing the lockfile bump separately.

Approving. The rebase is worth doing for readability, and I will merge once the branch is current and the advisory is off main.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three commits since 9f89c0af are the right answers. Parsing the workflows with PyYAML and reading only the run: scalars kills the whole class of false positives I was worried about, fullmatch on the stripped string means a test named inside a longer script no longer counts as invoked, and the trailing-comment arm is a detail I would have missed. check_suite refusing when it discovers zero tests is the part I like most, because that is the shape where this kind of gate usually dies without a sound.

At 4a6e8979 on its own the gate is green:

$ bash scripts/check_test_reachability.sh
test-reachability: every release and E2E test is invoked by a gate.
$ bash tests/release/test-reachability.test.sh
test-reachability test: orphan, empty-suite, local-only, and invocation matching validated.

Blocking: it goes red the moment #429 lands

#429 is approved and green, and it adds a release test whose CI invocation needs root:

- name: Check bounded actions and removal of shell grants
  run: sudo -n bash tests/release/action-steps.test.sh

command accepts bash <path> and nothing before it, so sudo -n bash … does not match and the test reads as unreachable. I merged the two branches and ran your gate on the result:

$ git merge --no-edit origin/pull/429/head     # 22 files changed
$ bash scripts/check_test_reachability.sh
test-reachability: test is not invoked by a gate: tests/release/action-steps.test.sh
rc=1

That is not your bug, and #429 did not exist when you wrote the regex. Whichever of the two lands second turns docs-and-hygiene red on main, so I would rather fix it here than revert something later.

One line does it:

command = re.compile(r"(?:sudo(?:[ \t]+-[A-Za-z]+)*[ \t]+)?bash[ \t]+(tests/(?:release|e2e)/[A-Za-z0-9_.-]+\.test\.sh)(?:[ \t]+#.*)?[ \t]*")

I applied exactly that to the merged tree and re-ran both:

$ bash scripts/check_test_reachability.sh
test-reachability: every release and E2E test is invoked by a gate.
$ bash tests/release/test-reachability.test.sh
test-reachability test: orphan, empty-suite, local-only, and invocation matching validated.

Worth one more case in test-reachability.test.sh alongside the ones you already have: a step whose run is sudo -n bash tests/release/example.test.sh counts as an invocation, and a step whose run is echo sudo bash tests/release/example.test.sh does not. The second half is what keeps the optional prefix from turning into a hole.

Not blocking

fullmatch also means a multi-line run: block never counts, even when its only real command is the test. Nothing in the tree does that today and I checked. It is worth a sentence in CONTRIBUTING.md next to the rule, because the failure mode is a contributor adding a perfectly good gate and being told their test is unreachable.

Where this stands

Your branch already carries main through c56a613, so there is no rebase to do. The three-dot diff against main is 5 files and 106 insertions with zero deletions; I said as much last time and it still holds. security-audit is red for RUSTSEC-2026-0285 in rustls, which is on main and not on you; it clears once the lockfile bump lands.

Push the regex and the two cases and I will take another pass the same day.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote a tighter regex than the one I handed you, and your new cases prove mine was wrong. I suggested (?:sudo(?:[ \t]+-[A-Za-z]+)*[ \t]+)?, which accepts sudo -l bash tests/release/x.test.sh. sudo -l lists a sudoers entry and runs nothing, so that spelling would have let an unreachable test read as reachable. You took (?:sudo(?:[ \t]+-n)?[ \t]+)? and added six negative cases around it. I put my version into your gate and your own test caught it:

$ podman run --rm --network=none -v /tmp/mut377b:/repo:ro -w /repo localhost/sk-yamlgate:1 \
    sh -c 'out="$(bash scripts/check_test_reachability.sh 2>&1)"; rc=$?; echo "$out"; echo "gate rc=$rc"; echo "---"; out2="$(bash tests/release/test-reachability.test.sh 2>&1)"; rc2=$?; echo "$out2"; echo "test rc=$rc2"'
test-reachability: every release and E2E test is invoked by a gate.
gate rc=0
---
test-reachability test: non-invocation unexpectedly passed: run: sudo -l bash tests/release/not-executed.test.sh
test rc=1

That is the review finding the reviewer's mistake, which is the best thing a test can do for me.

Verified at 1cbd5dd9ec93145ba8d96233d36b9ba7c929b309

The merge base is b2c823e6, the current tip of main, so the three-dot diff is 5 files and 255 insertions with zero deletions. Everything below ran inside a container with --network=none and the tree mounted read-only; maintainer screen says this branch may not be executed on my host, and I did not.

Clean:

$ podman run --rm --network=none -v /tmp/wt377:/repo:ro -w /repo localhost/sk-yamlgate:1 \
    sh -c 'bash scripts/check_test_reachability.sh; echo "gate rc=$?"; bash tests/release/test-reachability.test.sh; echo "test rc=$?"'
test-reachability: every release and E2E test is invoked by a gate.
gate rc=0
test-reachability test: orphan, empty-suite, local-only, and invocation matching validated.
test rc=0

25 release tests and 6 E2E tests, 31 in all, and the gate accounts for every one.

The blocking item from last time is fixed, and I proved the fix is load-bearing

I reverted your regex hunk back to the pre-e6f73f0 form on a copy of the tree and ran both:

$ sed -i 's|^command = re.compile(r"(?:sudo(?:\[ \\t\]+-n)?\[ \\t\]+)?bash|command = re.compile(r"bash|' scripts/check_test_reachability.sh
$ podman run --rm --network=none -v /tmp/mut377:/repo:ro -w /repo localhost/sk-yamlgate:1 \
    sh -c 'out="$(bash scripts/check_test_reachability.sh 2>&1)"; rc=$?; echo "$out"; echo "gate rc=$rc"; echo "---"; out2="$(bash tests/release/test-reachability.test.sh 2>&1)"; rc2=$?; echo "$out2"; echo "test rc=$rc2"'
test-reachability: test is not invoked by a gate: tests/release/action-steps.test.sh
gate rc=1
---
test-reachability: test is not invoked by a gate: tests/release/not-executed.test.sh
test rc=1

The first line is the real one. #429 landed while you were working, and ci.yml:128 now reads run: sudo -n bash tests/release/action-steps.test.sh, so without your change the gate goes red on main the moment this merges. With it, green.

Fail-closed, both ways

A gate that inspects nothing and says so is the shape this repo keeps losing to, so I broke its inputs rather than its subject:

$ rm /tmp/mut377c/.github/workflows/e2e.yml
$ podman run --rm --network=none -v /tmp/mut377c:/repo:ro -w /repo localhost/sk-yamlgate:1 sh -c 'bash scripts/check_test_reachability.sh; echo "gate rc=$?"'
test-reachability: cannot read workflow /repo/.github/workflows/e2e.yml: [Errno 2] No such file or directory: '/repo/.github/workflows/e2e.yml'
gate rc=1

$ podman run --rm --network=none -v /tmp/wt377:/repo:ro -w /repo docker.io/library/python:3.12-slim sh -c 'bash scripts/check_test_reachability.sh; echo "gate rc=$?"'
test-reachability: PyYAML is required; install yamllint with python3 -m pip install yamllint==1.38.0
gate rc=1

Missing workflow, missing parser, empty suite: all three refuse instead of reporting a pass over nothing.

I also checked the step ordering you depend on, because the gate needs a module nothing else in that job imports. Install lint tools runs pip install yamllint==1.38.0 at ci.yml:45, five lines above your first new step at ci.yml:50, and yamllint carries PyYAML:

$ podman run --rm docker.io/library/python:3.12-slim sh -c 'pip install -q yamllint==1.38.0; python3 -c "import yaml; print(yaml.__version__)"; pip show yamllint | grep -i "^Requires"'
6.0.3
Requires: pathspec, pyyaml

The ci-local deduplication is right

run_shell_tests already globs tests/release/*.test.sh, so the explicit run_step you removed in 1cbd5dd was running test-reachability.test.sh a second time. Coverage is unchanged; ls tests/release/test-reachability.test.sh resolves, so the glob still picks it up. The sentence you added to CONTRIBUTING.md is the part I would have forgotten.

The other two gates this touches pass on your tree: shellcheck over 201 scripts at --severity=warning, rc 0, and markdownlint over the CI file list, 0 issues in 0 files.

Approved, and one thing holding the button that is not yours

I have approved this at 1cbd5dd9. The board is green on all eleven checks including security-audit, which was red for RUSTSEC-2026-0285 last time and cleared when the lockfile bump landed on main.

It edits .github/workflows/ci.yml, and I keep a human read of a workflow diff as the control that stops a fork reshaping CI, so I am not pressing merge myself. The diff is two steps in docs-and-hygiene and nothing else. It goes in today.

Separately, my own merge gate cannot issue its receipt for this one, and the reason is mine: the container it runs contributor scripts in is python:3.12 with no PyYAML, so it watched your test fail on a missing parser and reported it as the test not passing. I ran the same proof by hand in an image that has PyYAML, which is what the two mutation blocks above are. I will fix the image.

Optional, and only if you are already in the file

Line 81, inside the non-invocation loop, is a bare grep -Fq 'test is not invoked by a gate: ...' <<< "$output". It does fail the run under set -e, so the assertion bites. It exits without printing which case went wrong, unlike the || { printf ...; exit 1; } arms you wrote at lines 40 and 51. Fifteen cases share that loop now.

What I would like you to look at next

Two open issues sit on what you demonstrated here, and I will hold either for you:

  • #433: ci-local reports PASS after a root-only assertion silently skipped, and nothing compares how CI invokes a test with how ci-local does. That second half is the natural sequel to this gate: you have the invocation set from ci.yml already parsed.
  • #441: the action-pin check reads uses: with a line grep, so a flow-style mapping never gets checked. You have already shown, in this PR, why parsing the YAML beats matching its text.

Say which one and it is yours.

One last thing. This gate exists because a release test can rot out of CI unnoticed, and you run enough CI to have felt that. SysKnife's read-only side is worth pointing a shell at on a box you already own: sysknife --dry-run "show disk usage and list services that ate cpu in the last hour" plans and previews and writes nothing. No obligation, and it changes nothing about this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nothing requires that a tests/release/*.test.sh is actually invoked by a gate

2 participants