From e48197990fde5a67a90dcb57f906c787b517d8b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 19:17:45 +0000 Subject: [PATCH 1/3] Fail release tests if Test can pass after its tests fail, or another job uploads to PyPI Publish waits on Test, so continue-on-error on Test or its steps, a condition on the test step, or anything such as || true after make test would let a release whose tests failed reach PyPI. The publishing check also read push.yaml alone, so an upload in any other workflow file would have bypassed it. Follow-ups to the review of #591 (P3 1 and 6). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LwQq6Jzt9cKvgaMoYvZDfG --- tests/core/test_release_tagging.py | 111 +++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index c364128e..37739cf7 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -339,3 +339,114 @@ def test_publish_workflow_publishes_only_after_tests_pass(): jobs["Publish"]["if"], re.IGNORECASE, ) + + +WORKFLOWS = ROOT / ".github" / "workflows" +PYPI_UPLOAD_COMMAND = re.compile(r"\b(twine\s+upload|uv\s+publish)\b") + + +def uploads_to_pypi(step: dict) -> bool: + return step.get("uses", "").startswith("pypa/gh-action-pypi-publish@") or bool( + PYPI_UPLOAD_COMMAND.search(step.get("run", "")) + ) + + +def pypi_publishing_jobs(workflows: Path) -> list[tuple[str, str]]: + return [ + (path.name, name) + for path in sorted([*workflows.glob("*.yaml"), *workflows.glob("*.yml")]) + for name, job in (yaml.safe_load(path.read_text()).get("jobs") or {}).items() + if any(uploads_to_pypi(step) for step in job.get("steps", [])) + ] + + +def test_only_push_publish_job_uploads_to_pypi(): + # The checks above cover push.yaml's Publish job alone, so an upload in + # any other job or workflow file would reach PyPI without them. + assert pypi_publishing_jobs(WORKFLOWS) == [("push.yaml", "Publish")] + + +@pytest.mark.parametrize( + "upload", + [ + {"uses": "pypa/gh-action-pypi-publish@release/v1"}, + {"run": "python -m twine upload dist/*"}, + {"run": "make build\nuv publish"}, + ], + ids=["pypi-publish-action", "twine-upload", "uv-publish"], +) +def test_pypi_upload_check_finds_upload_in_another_workflow(tmp_path, upload): + for path in WORKFLOWS.iterdir(): + shutil.copy(path, tmp_path / path.name) + pr_workflow = tmp_path / "pr.yaml" + workflow = yaml.safe_load(pr_workflow.read_text()) + workflow["jobs"]["Test"]["steps"].append(upload) + pr_workflow.write_text(yaml.safe_dump(workflow)) + + assert pypi_publishing_jobs(tmp_path) == [ + ("pr.yaml", "Test"), + ("push.yaml", "Publish"), + ] + + +def assert_test_job_fails_when_tests_fail(test_job: dict) -> None: + steps = test_job["steps"] + # continue-on-error on Test or one of its steps lets the job succeed + # after a step fails, and Publish would then run. + assert not any("continue-on-error" in item for item in [test_job, *steps]), ( + "Test continues on error" + ) + test_step = steps[ + only_step(steps, lambda step: "make test" in step.get("run", ""), "test") + ] + # A skipped step does not fail the job, so a condition on the step that + # runs the tests could let Test succeed without running them. + assert "if" not in test_step, "test step has a condition" + # Anything after the command, such as `|| true`, could mask its failure. + assert test_step["run"].strip() == "make test", "test step runs more than make test" + + +def test_test_job_cannot_pass_with_failing_tests(): + test_job = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"]["Test"] + + assert_test_job_fails_when_tests_fail(test_job) + + +def find_test_step(test_job: dict) -> dict: + return next(step for step in test_job["steps"] if step.get("run") == "make test") + + +def continue_test_job_on_error(test_job: dict) -> None: + test_job["continue-on-error"] = True + + +def continue_after_test_failure(test_job: dict) -> None: + find_test_step(test_job)["continue-on-error"] = True + + +def ignore_test_failure(test_job: dict) -> None: + find_test_step(test_job)["run"] += " || true" + + +def skip_tests(test_job: dict) -> None: + find_test_step(test_job)["if"] = False + + +@pytest.mark.parametrize( + ("mutate", "violation"), + [ + pytest.param(mutate, violation, id=mutate.__name__) + for mutate, violation in [ + (continue_test_job_on_error, "Test continues on error"), + (continue_after_test_failure, "Test continues on error"), + (ignore_test_failure, "test step runs more than make test"), + (skip_tests, "test step has a condition"), + ] + ], +) +def test_test_job_check_rejects_masked_test_failure(mutate, violation): + test_job = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"]["Test"] + mutate(test_job) + + with pytest.raises(AssertionError, match=violation): + assert_test_job_fails_when_tests_fail(test_job) From 3620e8d55db72781919638b6dc6d0524bb181f97 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 19:17:45 +0000 Subject: [PATCH 2/3] Drop the actionlint ignores for the removed Deploy documentation step #594 moved that step out of Test into the Docs job, so both ignored messages no longer occur; keeping them would hide them if they came back. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LwQq6Jzt9cKvgaMoYvZDfG --- .github/actionlint.yaml | 9 --------- changelog.d/core591-release-guards.changed.md | 1 + 2 files changed, 1 insertion(+), 9 deletions(-) create mode 100644 changelog.d/core591-release-guards.changed.md diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index 90b61a49..edc4d5de 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -1,11 +1,2 @@ # 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/changelog.d/core591-release-guards.changed.md b/changelog.d/core591-release-guards.changed.md new file mode 100644 index 00000000..41b33852 --- /dev/null +++ b/changelog.d/core591-release-guards.changed.md @@ -0,0 +1 @@ +The release tests now fail if the push workflow's Test job could pass after its tests fail, or if any workflow other than the Publish job uploads to PyPI, and the actionlint config drops two ignores for a step that no longer exists. From 38c462b1a82c80ea5da872fffc2c6c1473addfa1 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 3 Nov 2026 14:49:58 -0500 Subject: [PATCH 3/3] Name the upload forms and Test-job checks in the release note The fragment said the release tests fail if any workflow other than Publish uploads to PyPI, but the check recognizes only pypa/gh-action-pypi-publish, twine upload and uv publish, so a step that runs poetry publish passes it (review P3 1). The fragment now names the three upload forms and the three Test-job checks. Co-Authored-By: Claude Opus 5.5 --- changelog.d/core591-release-guards.changed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/core591-release-guards.changed.md b/changelog.d/core591-release-guards.changed.md index 41b33852..7828be99 100644 --- a/changelog.d/core591-release-guards.changed.md +++ b/changelog.d/core591-release-guards.changed.md @@ -1 +1 @@ -The release tests now fail if the push workflow's Test job could pass after its tests fail, or if any workflow other than the Publish job uploads to PyPI, and the actionlint config drops two ignores for a step that no longer exists. +The release tests now fail if `push.yaml`'s Test job or one of its steps sets `continue-on-error`, or if its `make test` step has an `if:` condition or runs more than that command. They also fail if any job other than `push.yaml`'s Publish uploads to PyPI with `pypa/gh-action-pypi-publish`, `twine upload` or `uv publish`. The actionlint config drops two ignores for a step that no longer exists.