Skip to content

Publish a release only after its tests pass - #591

Open
MaxGhenis wants to merge 4 commits into
masterfrom
fix-publish-needs-test
Open

MaxGhenis wants to merge 4 commits into
masterfrom
fix-publish-needs-test

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

What this fixes

In .github/workflows/push.yaml, the Publish job (PyPI upload and git tag) had no needs:, so on each release commit it ran beside Test. A release could reach PyPI while its tests were still running, or after they failed. In the 2026-10-06 13:11Z release run (37468917800), Publish finished at 13:40:07 and Test at 13:44:04.

Publish now has needs: Test. Both jobs have the same if: (the canonical repository and the "Update PolicyEngine Core" commit), so Publish still runs on every release commit; it now waits for Test to pass.

Merged with master, which since added a Lint job, a Test timeout, and a Docs job with needs: [Test, Publish] (#594, #597, #604). The job graph is now Test → Publish → Docs, with no cycle; all three share the same if:, so Docs still runs on release commits. The Test comment that said Publish does not wait on it now says Publish waits on Test and Docs on both.

Gating won't hold up current releases: all 69 of the last 69 "Update PolicyEngine Core" push runs passed as of 2026-10-06, and the five most recent (2026-10-30 to 10-31) all passed too.

Tests

test_release_tagging.py::test_publish_workflow_publishes_only_after_tests_pass checks four things:

  • every job that uses pypa/gh-action-pypi-publish needs Test;
  • the only such job is Publish;
  • Publish's if: is Test's;
  • Publish's if: contains no status function (always(), failure(), cancelled(), success()), which could let it run after a failed Test.

Master's parsed check of the Publish steps (test_publish_workflow_tags_only_after_pypi_succeeds and its mutation tests, from #597) covers permissions, fetch depth and step order, not needs, so both are kept. test_docs_deployment.py::test_documentation_deploy_cannot_block_pypi_publishing checks Docs is not upstream of Publish.

After the merge with master:

before (master workflow):  1 failed, 15 passed   (AssertionError: assert 'Test' in [])
after:                     16 passed
tests/core/test_docs_deployment.py: 5 passed
actionlint 1.7.12 (with shellcheck and pyflakes): 0 errors

Impact

Impact: CI only; no package code changes. Each release now publishes once Test finishes: in the last five "Update PolicyEngine Core" push runs, Test took between 5 min 31 s and 7 min 40 s, while Publish finished 36 to 47 s after it started. Docs already waited on Test, so documentation deploys no later than before.

Noticed in passing: the Test job's "Deploy documentation" step ran if: matrix.os == 'ubuntu-latest', but the job had no matrix, so the step was skipped on every run. #594 resolved this by moving the deploy into its own Docs job.

axiom: n/a: CI workflow change, no policy change

Other audit findings, handled elsewhere

The 2026-10-06 invariants audit of master 757147c7 also confirmed defects that this PR leaves alone, because other work owns them:

Branch staleness after set_input and raw-file restore are documented behaviour, not defects.


Generated by Claude Code

push.yaml ran Test and Publish side by side on the release commit, so a
release could reach PyPI while its tests were running or after they failed.
In the 2026-10-06 13:11Z release run, Publish finished at 13:40:07 and Test
at 13:44:04. Publish now has needs: Test. Both jobs share the same if
condition, so Publish still runs on every release commit and waits for Test
first.

Adds a check in test_release_tagging.py that every PyPI-publishing job
needs Test and runs under Test's condition.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis and others added 2 commits October 6, 2026 16:59
From the independent review of 9948a8d (approved, nit): adding always()
to both jobs' shared condition kept them equal and passed the check,
although it would let Publish run after a failed Test. The check now also
requires Publish's condition to contain no status function.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the second independent review (approved, nit): Always() and ALWAYS()
passed the check. It now uses a case-insensitive regex; all five override
variants are rejected and the current workflow passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis added a commit that referenced this pull request Oct 8, 2026
* Deploy the documentation site from its own release job

The Test job lost its OS matrix in #282 (2024-09-26) but kept the
deploy step's `if: matrix.os == 'ubuntu-latest'` guard, so the step has
been skipped on every release since and gh-pages last moved on
2024-09-26.

Move the docs build and deploy into a Docs job that needs Test, has
`contents: write`, and uses the v4 action's documented inputs. Keeping
it out of Test means a failed deploy cannot hold back a PyPI release
once Publish needs Test (#591). Add sphinx.ext.githubpages so the build
writes the .nojekyll that a branch-sourced Pages site needs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Deploy docs only after the release reaches PyPI

Review follow-ups on the Docs job:

- Need Publish as well as Test, so the site never documents a version
  that failed to reach PyPI. Publish still does not depend on Docs.
- Serialize deploys with a gh-pages-deploy concurrency group; a newer
  waiting deploy replaces an older one instead of racing it.
- Loosen the structural tests to the properties that matter (branch,
  folder, contents: write, an unconditional deploy step), parse the
  Makefile recipe by its tab-indented lines, and skip jobs whose matrix
  is built by an expression.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Tighten docs deploy checks from the delta review

- Say what the concurrency group guarantees: one deploy at a time, a
  newly queued deploy replacing one still waiting. GitHub does not
  promise release order.
- Require a constant concurrency group (a per-run group would not
  serialize anything) and accept the string shorthand.
- Match `jb build docs` as a whole recipe line, so a changed output
  path or a commented-out line no longer passes.
- Treat `strategy: null` like no strategy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Picking this up for the US hub. I'll post a hand-off comment here when it's ready.


Generated by Claude Code

Keep master's push workflow (Lint, Test timeout, Docs job needing Test and
Publish, Publish environment and permissions) and add only `needs: Test` to
Publish. Reword the Test comment, which said Publish does not wait on it.
Keep master's parsed Publish-step check and this branch's needs/if/status
check; they test different things.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwQq6KoHBBVJ3ZLqpicVpS
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Ready for hub review

Head: 2424738af6b1582da08bf5116680870e0730aae0. This is a merge commit of origin/master (3c89556f2d) into fef0d21f73, pushed as a fast-forward. There was no rebase or force-push. The branch head matched fef0d21f73 at pick-up, and its last push was 2026-10-06.

RESULT: FIXED

Conflicts and how I resolved them

  • .github/workflows/push.yaml. Git's 3-way merge put this branch's needs: Test line into master's new Docs job, and left Publish with no needs. I resolved it this way:
    • Docs keeps master's needs: [Test, Publish] and its comment.
    • Publish gets needs: Test, with a one-line comment, at .github/workflows/push.yaml:115-116.
    • Everything else is master's, unchanged: Lint, versioning, Test with timeout-minutes: 45, Docs with its permissions and concurrency, and Publish with its environment, permissions, steps and comments.
  • Stale comment. The Test comment said "Docs waits on this job (Publish does not)". It now reads "Publish waits on this job, and Docs waits on both" (push.yaml:60-62). I checked every other comment in the file. The Docs comment ("Deploy only a release that reached PyPI…") and the Publish trusted-publishing comment are still accurate.
  • tests/core/test_release_tagging.py.
  • Checks on master's structure:
    • Publish's if: still equals Test's. Docs' if: equals both.
    • The status-function check is kept.
    • The job graph is Test → Publish → Docs. graphlib gives the topological order Lint, versioning, Test, Publish, Docs, so there is no cycle.
    • Docs shares the release-commit if:, so it still runs on release commits.
    • test_docs_deployment.py::test_documentation_deploy_cannot_block_pypi_publishing already asserts Docs is not upstream of Publish.

Tests (one file per process)

  • tests/core/test_release_tagging.py: 16 passed.
  • tests/core/test_docs_deployment.py: 5 passed. This is the only other test file that reads .github/workflows/.
  • Mutant: I restored master's push.yaml, without needs: Test, and ran test_release_tagging.py. Result: 1 failed (AssertionError: assert 'Test' in []), 15 passed. The code was restored afterwards and the file gives 16 passed again.
  • actionlint: pr.yaml runs the rhysd/actionlint 1.7.12 Docker image with -color. Docker has no daemon in this VM, so I ran the official 1.7.12 release binary with shellcheck and pyflakes on PATH (the image bundles both). Result: 0 errors in 2 files.
  • ruff format --check and ruff check on the test file: clean.

CI at head

Pull request run 41548423680: completed, success. All 19 jobs passed, including Lint workflows, the 8 Test jobs and the 8 country-package jobs.

PR body

Judgment calls

  • Timing claim. I did not keep "about four minutes". In the last five "Update PolicyEngine Core" push runs (41183437332, 41165687527, 41093672475, 41091798445, 41060086107), Test took 5 min 31 s to 7 min 40 s, and Publish took 36–47 s. The body now quotes those figures.
  • Test-job comment. I left master's "Slowest observed 2026-10-06..08: 4 minutes" in the Test comment. It is dated and still well under the 45-minute timeout, and this PR shouldn't rewrite Cancel superseded PR CI runs and add job timeouts #604's data.

Proposed follow-up

Remove actionlint ignores that no longer match anything. .github/actionlint.yaml ignores two push.yaml findings from the old Test-job deploy step, and its own comment says to delete them once #594 lands. actionlint 1.7.12 reports 0 errors with an empty config, so both entries are now unused.


Generated by Claude Code

This branch has not been deployed

No deployments
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.

2 participants