Conversation
vladimirrott
left a comment
There was a problem hiding this comment.
One change to make, and it is three lines. Everything else here is right, including the parts I went looking to break.
Head ef7712ff, reviewed on the merge of 9d02dc9 with your branch, clean at c38679e5.
The gap is real and you closed it
There is no .github/actions in the tree today, so I made one with a mutable reference and ran both gates against it:
$ out="$(podman run --rm --network=none -v /tmp/r22oldact:/repo:ro -w /repo localhost/sk-rehearsal:1 bash tests/release/release-rehearsal.test.sh 2>&1)"; echo "OLD GATE + unpinned composite rc=$? :: $(printf '%s' "$out"|grep -v level=warning|tail -2)"
OLD GATE + unpinned composite rc=0 :: SKIP: unreadable workflow fixture requires a non-root user
Release rehearsal contract passed.
$ out2="$(podman run --rm --network=none -v /tmp/r22newact:/repo:ro -w /repo localhost/sk-rehearsal:1 bash tests/release/release-rehearsal.test.sh 2>&1)"; echo "471 GATE + unpinned composite rc=$? :: $(printf '%s' "$out2"|grep -v level=warning|tail -2)"
471 GATE + unpinned composite rc=1 :: FAIL: action.yml action is not pinned to a 40-hex SHA: actions/checkout@v7
The action file was eight lines, using: composite, one step, uses: actions/checkout@v7. On main nothing objects and the gate reports the contract passed.
The Node side lands the same way, and the end-of-life date comes out of your own schedule rather than a hardcoded list:
$ out="$(podman run --rm --network=none -v /tmp/r22nodeact:/repo:ro -w /repo localhost/sk-yamlgate:2 bash tests/release/node-eol.test.sh 2>&1)"; echo "node-eol + using:node20 rc=$? :: $(printf '%s' "$out"|grep -v level=warning|tail -3)"
node-eol + using:node20 rc=1 :: FAIL action.yml:5 pins Node 20, end-of-life since 2026-04-30
An existing but empty actions root refuses rather than reporting a clean scan:
$ out2="$(podman run --rm --network=none -v /tmp/r22emptyact:/repo:ro -w /repo localhost/sk-yamlgate:2 bash tests/release/node-eol.test.sh 2>&1)"; echo "node-eol + empty actions root rc=$? :: $(printf '%s' "$out2"|grep -v level=warning|tail -2)"
node-eol + empty actions root rc=1 :: FAIL: no action metadata files matched under /repo/.github/actions
And the fixtures are load-bearing. Make discover() skip the actions root and seven of the eight go red:
$ rm -rf /tmp/r22mut471 && cp -a /tmp/r22m471 /tmp/r22mut471 && rm -f /tmp/r22mut471/.git && sed -i 's| if actions is not None and actions.exists():| if False:|' /tmp/r22mut471/scripts/github_yaml.py && grep -n 'if False' /tmp/r22mut471/scripts/github_yaml.py && out="$(podman run --rm --network=none -v /tmp/r22mut471:/repo:ro -w /repo localhost/sk-yamlgate:2 python3 tests/test_github_yaml.py 2>&1)"; echo "MUTATION-E (discovery ignores .github/actions) rc=$? :: $(printf '%s' "$out"|grep -v level=warning|tail -3)"
14: if False:
MUTATION-E (discovery ignores .github/actions) rc=1 :: Ran 8 tests in 1.221s
FAILED (failures=7)
Collecting names + directories so a directory named action.yml gets picked up and then dies in read_text is the kind of detail that only gets written by somebody who has watched a guard inspect nothing. Same for checking every path is readable before emitting any of the list.
The one change
discover() gives workflows a floor and gives templates none:
files = sorted(p for p in workflows.iterdir() if p.suffix in (".yml", ".yaml"))
if not files:
raise ValueError(f"no workflow files matched under {workflows}")
…
if templates is not None:
files.extend(sorted(p for p in templates.iterdir() if p.suffix in (".yml", ".yaml")))Delete every issue template and the new gate is happy:
$ rm -rf /tmp/r22tmpl && cp -a /tmp/r22m471 /tmp/r22tmpl && rm -f /tmp/r22tmpl/.git && rm -f /tmp/r22tmpl/.github/ISSUE_TEMPLATE/*.yml && ls -A /tmp/r22tmpl/.github/ISSUE_TEMPLATE/ ; out="$(podman run --rm --network=none -v /tmp/r22tmpl:/repo:ro -w /repo localhost/sk-yamlgate:2 bash scripts/lint-github-yaml.sh 2>&1)"; echo "EMPTY ISSUE_TEMPLATE rc=$? :: $(printf '%s' "$out"|grep -v level=warning|tail -2)"
EMPTY ISSUE_TEMPLATE rc=0 ::
The command it replaces refused:
$ out="$(podman run --rm --network=none -v /tmp/r22tmplold:/repo:ro -w /repo localhost/sk-yamlgate:2 sh -c 'cd /repo && yamllint .github/ISSUE_TEMPLATE/*.yml .github/workflows/*.yml' 2>&1)"; echo "OLD COMMAND, empty ISSUE_TEMPLATE rc=$? :: $(printf '%s' "$out"|grep -v level=warning|tail -3)"
OLD COMMAND, empty ISSUE_TEMPLATE rc=255 :: [Errno 2] No such file or directory: '.github/ISSUE_TEMPLATE/*.yml'
An unexpanded glob is a clumsy way to fail closed, and it did fail closed. Three lines restore it:
if templates is not None:
found = sorted(p for p in templates.iterdir() if p.suffix in (".yml", ".yaml"))
if not found:
raise ValueError(f"no issue templates matched under {templates}")
files.extend(found)Nothing else in the pull request needs touching, and a fixture alongside the seven you already wrote would close it for good. I am asking because this repository has merged four fixes this month for a guard reporting clean over an empty set (#407, #423, #443 and #461), and a fifth arriving inside the fix for the fourth would be a poor joke.
Everything else, on the merged tree
| Gate | Result |
|---|---|
python3 tests/test_github_yaml.py |
rc=0, Ran 8 tests, OK |
bash scripts/lint-github-yaml.sh |
rc=0, same three pre-existing line-length warnings |
tests/release/action-pin-comments.test.sh |
rc=0 |
tests/release/ci-local.test.sh |
rc=0, OK |
tests/release/shellcheck-coverage.test.sh |
rc=0, 209 tracked shell files |
| ShellCheck 0.11.0 over the repo's file list | rc=0, 209 files |
scripts/check_test_reachability.sh |
rc=0 |
One note that costs you nothing. release-rehearsal.test.sh now calls tests/test_github_yaml.py, so that test needs yamllint where it did not before. In my container without it the failure reads:
AssertionError: 127 != 0 : /repo/scripts/lint-github-yaml.sh: line 10: yamllint: command not found
CI installs yamllint in docs-and-hygiene, so CI is fine, and your CONTRIBUTING paragraph names the requirement for the fixture file. A contributor who runs only the rehearsal test locally meets it as a traceback rather than as a sentence. Mentioning yamllint where CONTRIBUTING describes the rehearsal test would close that, whenever you are next in the file.
Coordination
This overlaps two of @Humerous's branches. #469 edits tests/release/release-rehearsal.test.sh where you do, and #463 edits .github/workflows/ci.yml where you do. I merged all three onto main in the order 471, 469, 463: three clean auto-merges, working tree clean, combined tree at 6659eaa4, and crates-index-poll.test.sh, test_github_yaml.py and check_test_reachability.sh all pass on it. Nobody has to rebase around anybody, and I will re-check that as they land.
Push the templates floor and I will re-run the seven gates above at the new head. This one edits .github/workflows/ci.yml, so a human reads the workflow diff before it merges either way, which means the change costs you no time in the queue.
|
Addressed in ae9f992.
Before the implementation change, both subcases failed with CONTRIBUTING now explicitly says that |
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at ae9f992435b5ee146f75d7740eae7b1be763601d.
You closed the one blocking item the way I would have wanted: the new test deletes the only template and then runs both halves, discover() directly and lint-github-yaml.sh end to end, and it repeats the case with a stray README.md in the directory so a non-YAML file cannot rescue the scan. The CONTRIBUTING line with the pinned pip install answers my optional note too.
Everything below ran in rootless podman with --network=none against the three-way merge of your head with main at f22a6abd, which merges clean.
My repro from last time, which exited 0 on ef7712ff:
$ cd /tmp && rm -f /tmp/r23m471/.git && rm -rf /tmp/r23tmpl /tmp/r23m471mut && cp -a /tmp/r23m471 /tmp/r23tmpl && rm -f /tmp/r23tmpl/.github/ISSUE_TEMPLATE/*.yml && ls -A /tmp/r23tmpl/.github/ISSUE_TEMPLATE/; out="$(podman run --rm --network=none -v /tmp/r23tmpl:/repo:ro -w /repo localhost/sk-yamlgate:2 bash scripts/lint-github-yaml.sh 2>&1)"; echo "EMPTY ISSUE_TEMPLATE rc=$? :: $(printf '%s' "$out"|grep -v level=warning|tail -2)"
EMPTY ISSUE_TEMPLATE rc=1 :: FAIL: no issue templates matched under /repo/.github/ISSUE_TEMPLATE
The new guard bites. I replaced only the templates if not found: with if False:, left the actions guard alone, and ran the fixtures:
$ for pair in "HEAD:/tmp/r23m471" "TEMPLATE-GUARD-OFF:/tmp/r23m471mut"; do name=${pair%%:*}; dir=${pair##*:}; out="$(podman run --rm --network=none -v $dir:/repo:ro -w /repo localhost/sk-yamlgate:2 python3 tests/test_github_yaml.py 2>&1)"; rc=$?; echo "=== $name rc=$rc"; printf '%s\n' "$out" | grep -v level=warning | grep -E '^(Ran|OK|FAILED|FAIL:)' ; done
=== HEAD rc=0
Ran 9 tests in 2.995s
OK
That first loop's mutated copy was too broad (my sed hit the actions guard as well), so I rebuilt the copy with only the templates guard changed and re-ran it:
$ grep -n 'if False:\|if not found:' /tmp/r23m471mut/scripts/github_yaml.py; out="$(podman run --rm --network=none -v /tmp/r23m471mut:/repo:ro -w /repo localhost/sk-yamlgate:2 python3 tests/test_github_yaml.py 2>&1)"; rc=$?; echo "=== TEMPLATE-GUARD-ONLY-OFF rc=$rc"; printf '%s\n' "$out" | grep -v level=warning | grep -E '^(Ran|OK|FAILED|FAIL:|AssertionError)'
27: if not found:
32: if False:
=== TEMPLATE-GUARD-ONLY-OFF rc=1
FAIL: test_empty_templates_is_not_rescued_by_workflow (__main__.ActionMetadataTests.test_empty_templates_is_not_rescued_by_workflow) (unrelated_file=False)
AssertionError: ValueError not raised
FAIL: test_empty_templates_is_not_rescued_by_workflow (__main__.ActionMetadataTests.test_empty_templates_is_not_rescued_by_workflow) (unrelated_file=True)
AssertionError: ValueError not raised
Ran 9 tests in 2.494s
FAILED (failures=2)
Other gates on the merged tree: lint-github-yaml.sh rc=0, check_test_reachability.sh rc=0, node-eol.test.sh rc=0 (5 workflow Node pins, all on a supported release as of 2026-09-22). I could not get a clean local release-rehearsal.test.sh run: the one image I have with cargo lacks yamllint, and the run failed on that (yamllint: command not found), the requirement your new CONTRIBUTING line documents. CI ran that script green on this head.
I approve it. It edits .github/workflows/ci.yml, and a workflow change gets a second human read before it merges, so the merge is mine to do by hand rather than something the automation lands. Nothing more is needed from you.
Closes #459.
A mutable uses reference inside a local composite action currently bypasses the workflow gates. Discover nested
.github/actions/**/action.ymlandaction.yamlalongside workflow files, then apply SHA pinning, version-comment verification, Node EOL checks, and the same yamllint invocation locally and in CI. An absent actions directory remains valid; existing empty or unreadable inputs fail discovery instead of reporting a clean scan. When issue templates are requested, their YAML set must also be nonempty, preserving the old lint command's failure behavior.Preserves #446's YAML parser, workflow discovery floor, local references, and parser/input failure checks. Nine fixture tests exercise pinned and unpinned block/flow composites, both metadata extensions, empty roots, malformed/directory-shaped inputs, comment extraction, Node runtime/version rejection, and YAML linting. The template regression checks both an empty directory and one containing only a README; valid workflows must not hide either missing template set. Normal fixtures include a valid template. The existing release gate invokes the fixtures, and CONTRIBUTING explicitly documents its PyYAML/yamllint prerequisites. Existing action SHAs are unchanged.
Validation: the original before/after Linux run demonstrates the previously accepted unpinned composite and validates the initial eight fixtures. For the template follow-up, the new regression fails twice on
ef7712fwithValueError not raised, then passes onae9f992, including the actual lint entry point. Existing offline action-comment fixtures also pass. The latest upstream CI runs the complete release contract and all nine fixtures. No Rust files changed.The alternative Linux host lacked yamllint and could not resolve package mirrors; complete validation therefore ran on GitHub's Ubuntu runners.
Implemented with Codex assistance. This follows the coordination note to @mikevillari on #459; #460's assertion diagnostics remain separate.