From cf8fb6a1721952eeefd10e72ff5d21181fd5a818 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 6 Oct 2026 16:31:57 -0400 Subject: [PATCH 1/5] Lint GitHub Actions workflows on pull requests Add a "Lint workflows" job to the pull request workflow that runs actionlint 1.7.12 from the official rhysd/actionlint image, pinned by digest. The image bundles shellcheck and pyflakes, so `run:` scripts are checked too. Fix the two warnings it reports on master that #594 does not touch: quote the version substitution passed to `towncrier build --version` (SC2046) in push.yaml and in the Makefile `changelog` target, and rename the deprecated `skip_existing` input of pypa/gh-action-pypi-publish to `skip-existing`. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yaml | 11 +++++++++++ .github/workflows/push.yaml | 4 ++-- Makefile | 2 +- changelog.d/ci-actionlint.changed.md | 1 + 4 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 changelog.d/ci-actionlint.changed.md diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index a52d09218..4baf4e217 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -11,6 +11,17 @@ jobs: uses: astral-sh/setup-uv@v8.1.0 - name: Check formatting run: uvx ruff format --check . + lint-workflows: + name: Lint workflows + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Run actionlint + # rhysd/actionlint 1.7.12. The image bundles shellcheck and pyflakes, + # which actionlint runs on `run:` scripts. + uses: docker://rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667 + with: + args: -color check-changelog: name: Check changelog fragment runs-on: ubuntu-latest diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index 8605abf3c..0bc97cf7b 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -42,7 +42,7 @@ jobs: run: | uv pip install --system towncrier python .github/bump_version.py - towncrier build --yes --version $(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))") + towncrier build --yes --version "$(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))")" - name: Preview changelog update run: ".github/get-changelog-diff.sh" - name: Update changelog @@ -109,6 +109,6 @@ jobs: with: user: __token__ password: ${{ secrets.PYPI }} - skip_existing: true + skip-existing: true - name: Publish a git tag run: bash .github/publish-git-tag.sh diff --git a/Makefile b/Makefile index e774214b3..abf910541 100644 --- a/Makefile +++ b/Makefile @@ -27,4 +27,4 @@ build: changelog: python .github/bump_version.py - towncrier build --yes --version $$(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))") \ No newline at end of file + towncrier build --yes --version "$$(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))")" \ No newline at end of file diff --git a/changelog.d/ci-actionlint.changed.md b/changelog.d/ci-actionlint.changed.md new file mode 100644 index 000000000..fab3f8947 --- /dev/null +++ b/changelog.d/ci-actionlint.changed.md @@ -0,0 +1 @@ +Pull requests now run actionlint on the GitHub Actions workflows, and the release workflow quotes the version it passes to towncrier and uses the `skip-existing` input of the PyPI publish action. From d67d103f234007217e51d6192ca4b11b0ec6bf02 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 6 Oct 2026 17:22:05 -0400 Subject: [PATCH 2/5] Parse the Publish job in the release-ordering test test_publish_workflow_tags_only_after_pypi_succeeds sliced push.yaml's text from " Publish:" to the end of the file. With a later job holding contents: write and Publish's own permissions deleted, the slice read the later job's block and the test passed. It now asserts on yaml.safe_load(...)["jobs"]["Publish"]: contents: write, a fetch-depth 0 checkout, the tag step after the PyPI step with no condition, no continue-on-error on either step, and no `|| true` on the tag script. Mutation cases check that each assertion rejects the mistake it names. Add .github/actionlint.yaml so Lint workflows passes before #594 lands: it ignores, in push.yaml only, the two errors in the Test job's docs deploy step that #594 fixes. Delete both entries in whichever PR fixes or removes that step. Document running actionlint locally in the README. Co-Authored-By: Claude Opus 5.5 --- .github/actionlint.yaml | 11 +++ README.md | 4 + tests/core/test_release_tagging.py | 141 +++++++++++++++++++++++++++-- 3 files changed, 147 insertions(+), 9 deletions(-) create mode 100644 .github/actionlint.yaml diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml new file mode 100644 index 000000000..90b61a49e --- /dev/null +++ b/.github/actionlint.yaml @@ -0,0 +1,11 @@ +# actionlint reads this file from .github/ on every run, in CI and locally. +# Format: https://github.com/rhysd/actionlint/blob/v1.7.12/docs/config.md +paths: + .github/workflows/push.yaml: + ignore: + # Both come from the Test job's "Deploy documentation" step, which + # PolicyEngine/policyengine-core#594 moves into its own Docs job and + # fixes. Delete these two entries in whichever PR fixes or removes that + # step. + - 'property "os" is not defined in object type \{\}' + - 'input "GITHUB_TOKEN" is not defined in action "JamesIves/github-pages-deploy-action@v4"' diff --git a/README.md b/README.md index 8c6b88f9a..508ae87a2 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,10 @@ uv run pytest tests/core/test_file.py::test_name -v # Format before committing (CI enforces) make format + +# Lint GitHub Actions workflows after editing .github/ (CI enforces; +# install shellcheck so actionlint also checks run: scripts) +uvx --from actionlint-py actionlint ``` **Always use `uv run`** for Python commands; bare `python` / `pytest` / `pip` may pick up the wrong environment. diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index 64b88f3f3..520b4630d 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -1,9 +1,11 @@ +import re import shutil import subprocess import sys from pathlib import Path import pytest +import yaml ROOT = Path(__file__).resolve().parents[2] @@ -144,13 +146,134 @@ def test_release_tag_script_propagates_push_failure(tmp_path: Path): assert "origin" in result.stderr +def only_step(steps: list[dict], matches, description: str) -> int: + indices = [index for index, step in enumerate(steps) if matches(step)] + assert len(indices) == 1, f"expected one {description} step, found {indices}" + return indices[0] + + +def assert_publish_tags_only_after_pypi_succeeds(publish: dict) -> None: + steps = publish["steps"] + checkout = steps[ + only_step( + steps, + lambda step: step.get("uses", "").startswith("actions/checkout@"), + "checkout", + ) + ] + pypi_index = only_step( + steps, + lambda step: step.get("uses", "").startswith("pypa/gh-action-pypi-publish@"), + "PyPI publish", + ) + tag_index = only_step( + steps, + lambda step: ".github/publish-git-tag.sh" in step.get("run", ""), + "git tag", + ) + pypi_step = steps[pypi_index] + tag_step = steps[tag_index] + + # The tag script pushes a tag, so Publish itself needs write access. + # Permissions granted to any other job do not apply to it. + assert publish.get("permissions", {}).get("contents") == "write", ( + "Publish lacks contents: write" + ) + # The script looks for an existing tag locally, and the checkout fetches + # tags only with fetch-depth 0. + assert checkout.get("with", {}).get("fetch-depth") == 0, ( + "checkout is not fetch-depth 0" + ) + # Steps run in order and a failure skips the rest, so the tag step runs + # only after a successful upload, unless a status condition or + # continue-on-error lets it run anyway. + assert pypi_index < tag_index, "tag step precedes the PyPI step" + assert "if" not in tag_step, "tag step has a condition" + assert pypi_step.get("continue-on-error", False) is False, ( + "PyPI step continues on error" + ) + # A failed tag push must fail the job, not leave an untagged release. + assert tag_step.get("continue-on-error", False) is False, ( + "tag step continues on error" + ) + assert not re.search(r"\|\|\s*(true|:)", tag_step["run"]), ( + "tag script failure is ignored" + ) + + def test_publish_workflow_tags_only_after_pypi_succeeds(): - workflow = PUSH_WORKFLOW.read_text() - publish_job = workflow.split(" Publish:\n", maxsplit=1)[1] - publish_step = "uses: pypa/gh-action-pypi-publish@release/v1" - tag_step = "run: bash .github/publish-git-tag.sh" - - assert "permissions:\n contents: write" in publish_job - assert "fetch-depth: 0" in publish_job - assert publish_job.index(tag_step) > publish_job.index(publish_step) - assert "publish-git-tag.sh || true" not in publish_job + publish = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"]["Publish"] + + assert_publish_tags_only_after_pypi_succeeds(publish) + + +def drop_publish_permissions_and_add_later_writer(jobs: dict) -> None: + # A text slice from " Publish:" to the end of the file read the later + # job's permissions as Publish's and passed this workflow. + del jobs["Publish"]["permissions"] + jobs["Docs"] = { + "runs-on": "ubuntu-latest", + "permissions": {"contents": "write"}, + "steps": [{"run": "make documentation"}], + } + + +def find_step(jobs: dict, fragment: str) -> dict: + return next( + step + for step in jobs["Publish"]["steps"] + if fragment in step.get("uses", "") + step.get("run", "") + ) + + +def shallow_checkout(jobs: dict) -> None: + del find_step(jobs, "actions/checkout@")["with"]["fetch-depth"] + + +def tag_before_pypi(jobs: dict) -> None: + steps = jobs["Publish"]["steps"] + tag_step = find_step(jobs, "publish-git-tag.sh") + steps.remove(tag_step) + steps.insert(steps.index(find_step(jobs, "gh-action-pypi-publish")), tag_step) + + +def ignore_tag_failure(jobs: dict) -> None: + find_step(jobs, "publish-git-tag.sh")["run"] += " || true" + + +def continue_after_tag_failure(jobs: dict) -> None: + find_step(jobs, "publish-git-tag.sh")["continue-on-error"] = True + + +def continue_after_pypi_failure(jobs: dict) -> None: + find_step(jobs, "gh-action-pypi-publish")["continue-on-error"] = True + + +def tag_even_if_pypi_fails(jobs: dict) -> None: + find_step(jobs, "publish-git-tag.sh")["if"] = "always()" + + +@pytest.mark.parametrize( + ("mutate", "violation"), + [ + pytest.param(mutate, violation, id=mutate.__name__) + for mutate, violation in [ + ( + drop_publish_permissions_and_add_later_writer, + "Publish lacks contents: write", + ), + (shallow_checkout, "checkout is not fetch-depth 0"), + (tag_before_pypi, "tag step precedes the PyPI step"), + (tag_even_if_pypi_fails, "tag step has a condition"), + (continue_after_pypi_failure, "PyPI step continues on error"), + (continue_after_tag_failure, "tag step continues on error"), + (ignore_tag_failure, "tag script failure is ignored"), + ] + ], +) +def test_publish_workflow_check_rejects_unsafe_release_order(mutate, violation): + jobs = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"] + mutate(jobs) + + with pytest.raises(AssertionError, match=violation): + assert_publish_tags_only_after_pypi_succeeds(jobs["Publish"]) From cbb88b576c896f7017178d26b9ef8e7a7855f6a3 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 6 Oct 2026 17:41:25 -0400 Subject: [PATCH 3/5] Tighten the Publish-job checks from review - Accept `permissions: write-all`, which also grants contents: write, and fail with the named message (not AttributeError) on any other string. - Compare fetch-depth as a string: action inputs are strings, so 0 and "0" are the same input, and `false` no longer passes as 0. - Require the tag step to run exactly `bash .github/publish-git-tag.sh`, which also rejects `|| exit 0`, `set +e` and the like, not only `|| true`. - Say that fetch-depth 0 fetches every tag, not that only it does: checkout's fetch-tags input also fetches tags. - Pin the README's local actionlint command to the version CI runs, and mention pyflakes. Co-Authored-By: Claude Opus 5.5 --- README.md | 6 +++--- tests/core/test_release_tagging.py | 24 +++++++++++++----------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 508ae87a2..a256b3406 100644 --- a/README.md +++ b/README.md @@ -40,9 +40,9 @@ uv run pytest tests/core/test_file.py::test_name -v # Format before committing (CI enforces) make format -# Lint GitHub Actions workflows after editing .github/ (CI enforces; -# install shellcheck so actionlint also checks run: scripts) -uvx --from actionlint-py actionlint +# Lint GitHub Actions workflows after editing .github/ (CI enforces; install +# shellcheck and pyflakes so actionlint also checks run: scripts) +uvx --from actionlint-py==1.7.12.25 actionlint ``` **Always use `uv run`** for Python commands; bare `python` / `pytest` / `pip` may pick up the wrong environment. diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index 520b4630d..be43e152d 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -1,4 +1,3 @@ -import re import shutil import subprocess import sys @@ -176,12 +175,14 @@ def assert_publish_tags_only_after_pypi_succeeds(publish: dict) -> None: # The tag script pushes a tag, so Publish itself needs write access. # Permissions granted to any other job do not apply to it. - assert publish.get("permissions", {}).get("contents") == "write", ( - "Publish lacks contents: write" - ) - # The script looks for an existing tag locally, and the checkout fetches - # tags only with fetch-depth 0. - assert checkout.get("with", {}).get("fetch-depth") == 0, ( + permissions = publish.get("permissions", {}) + assert permissions == "write-all" or ( + isinstance(permissions, dict) and permissions.get("contents") == "write" + ), "Publish lacks contents: write" + # The script looks for an existing tag locally, and a checkout with + # fetch-depth 0 fetches every tag. Action inputs are strings, so 0 and "0" + # are the same input. + assert str(checkout.get("with", {}).get("fetch-depth")) == "0", ( "checkout is not fetch-depth 0" ) # Steps run in order and a failure skips the rest, so the tag step runs @@ -192,12 +193,13 @@ def assert_publish_tags_only_after_pypi_succeeds(publish: dict) -> None: assert pypi_step.get("continue-on-error", False) is False, ( "PyPI step continues on error" ) - # A failed tag push must fail the job, not leave an untagged release. + # A failed tag push must fail the job, not leave an untagged release, so + # the step runs the script alone: nothing such as `|| true` may follow it. assert tag_step.get("continue-on-error", False) is False, ( "tag step continues on error" ) - assert not re.search(r"\|\|\s*(true|:)", tag_step["run"]), ( - "tag script failure is ignored" + assert tag_step["run"].strip() == "bash .github/publish-git-tag.sh", ( + "tag step runs more than the tag script" ) @@ -267,7 +269,7 @@ def tag_even_if_pypi_fails(jobs: dict) -> None: (tag_even_if_pypi_fails, "tag step has a condition"), (continue_after_pypi_failure, "PyPI step continues on error"), (continue_after_tag_failure, "tag step continues on error"), - (ignore_tag_failure, "tag script failure is ignored"), + (ignore_tag_failure, "tag step runs more than the tag script"), ] ], ) From b7c4977fb47fba877407e7055ca3fd5beed50eaf Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 6 Oct 2026 18:14:17 -0400 Subject: [PATCH 4/5] Reject a condition on the PyPI step, and harden the lint job From the delta review of cbb88b57: - A condition on the PyPI step (for example `if: github.ref == 'refs/heads/main'` on a master-branch repo) skips the upload without failing the job, so the tag step still ran. The check now rejects it, with its own mutation case. - Test that the equivalent settings the check accepts, `write-all` and `fetch-depth: "0"`, pass. - Lint workflows runs with `contents: read`, and its checkout does not persist the token into the workspace the container mounts. - The README notes that actionlint must run from the repo root: the path-scoped ignores match paths relative to the working directory. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yaml | 4 ++++ README.md | 5 +++-- tests/core/test_release_tagging.py | 34 ++++++++++++++++++++++++++++-- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 4baf4e217..1eed1b705 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -14,8 +14,12 @@ jobs: lint-workflows: name: Lint workflows runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v6 + with: + persist-credentials: false - name: Run actionlint # rhysd/actionlint 1.7.12. The image bundles shellcheck and pyflakes, # which actionlint runs on `run:` scripts. diff --git a/README.md b/README.md index a256b3406..b92c86625 100644 --- a/README.md +++ b/README.md @@ -40,8 +40,9 @@ uv run pytest tests/core/test_file.py::test_name -v # Format before committing (CI enforces) make format -# Lint GitHub Actions workflows after editing .github/ (CI enforces; install -# shellcheck and pyflakes so actionlint also checks run: scripts) +# Lint GitHub Actions workflows after editing .github/, from the repo root +# (CI enforces; install shellcheck and pyflakes so actionlint also checks +# run: scripts) uvx --from actionlint-py==1.7.12.25 actionlint ``` diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index be43e152d..ffe31e961 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -186,9 +186,11 @@ def assert_publish_tags_only_after_pypi_succeeds(publish: dict) -> None: "checkout is not fetch-depth 0" ) # Steps run in order and a failure skips the rest, so the tag step runs - # only after a successful upload, unless a status condition or - # continue-on-error lets it run anyway. + # only after a successful upload. A condition on either step, or + # continue-on-error on the upload, would let it run anyway: a skipped + # step does not fail the job. assert pypi_index < tag_index, "tag step precedes the PyPI step" + assert "if" not in pypi_step, "PyPI step has a condition" assert "if" not in tag_step, "tag step has a condition" assert pypi_step.get("continue-on-error", False) is False, ( "PyPI step continues on error" @@ -255,6 +257,11 @@ def tag_even_if_pypi_fails(jobs: dict) -> None: find_step(jobs, "publish-git-tag.sh")["if"] = "always()" +def skip_pypi_on_this_branch(jobs: dict) -> None: + # The default branch is master, so this skips the upload on every release. + find_step(jobs, "gh-action-pypi-publish")["if"] = "github.ref == 'refs/heads/main'" + + @pytest.mark.parametrize( ("mutate", "violation"), [ @@ -266,6 +273,7 @@ def tag_even_if_pypi_fails(jobs: dict) -> None: ), (shallow_checkout, "checkout is not fetch-depth 0"), (tag_before_pypi, "tag step precedes the PyPI step"), + (skip_pypi_on_this_branch, "PyPI step has a condition"), (tag_even_if_pypi_fails, "tag step has a condition"), (continue_after_pypi_failure, "PyPI step continues on error"), (continue_after_tag_failure, "tag step continues on error"), @@ -279,3 +287,25 @@ def test_publish_workflow_check_rejects_unsafe_release_order(mutate, violation): with pytest.raises(AssertionError, match=violation): assert_publish_tags_only_after_pypi_succeeds(jobs["Publish"]) + + +def grant_write_all(jobs: dict) -> None: + jobs["Publish"]["permissions"] = "write-all" + + +def quote_fetch_depth(jobs: dict) -> None: + find_step(jobs, "actions/checkout@")["with"]["fetch-depth"] = "0" + + +@pytest.mark.parametrize( + "mutate", + [ + pytest.param(mutate, id=mutate.__name__) + for mutate in [grant_write_all, quote_fetch_depth] + ], +) +def test_publish_workflow_check_accepts_equivalent_settings(mutate): + jobs = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"] + mutate(jobs) + + assert_publish_tags_only_after_pypi_succeeds(jobs["Publish"]) From 9a30bd555b9766864f6cc0a0b834cac9605b242e Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 6 Oct 2026 18:40:14 -0400 Subject: [PATCH 5/5] Say a condition could, not would, let the tag step run An `if: success()` on either step changes nothing; the check still rejects any `if:`, since most conditions would skip a step. Co-Authored-By: Claude Opus 5.5 --- tests/core/test_release_tagging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index ffe31e961..12762cbfd 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -187,7 +187,7 @@ def assert_publish_tags_only_after_pypi_succeeds(publish: dict) -> None: ) # Steps run in order and a failure skips the rest, so the tag step runs # only after a successful upload. A condition on either step, or - # continue-on-error on the upload, would let it run anyway: a skipped + # continue-on-error on the upload, could let it run anyway: a skipped # step does not fail the job. assert pypi_index < tag_index, "tag step precedes the PyPI step" assert "if" not in pypi_step, "PyPI step has a condition"