ci(release): gate every publishing job on a deployment environment (#641) - #673
Conversation
) `create-release.yml` reached PyPI and Anaconda with nothing a human had to approve. The `pypi` job's `environment: release` was commented out while the `id-token: write` from the same upstream snippet had been re-added live below it, so the block read as disabled as a unit when only its gate still was; the `conda` job never had an `environment:` at all. A scheduled vendor crawl that bumps the version is enough to enter the workflow, so a cron was sufficient to publish to two public indexes. Declare a deployment `environment:` on each of the four jobs with an effect outside the run, one per credential and blast radius rather than a single shared `release` that one approval would release everything through: * `github` -> `github-release`: the public Release, and the `v*` tag with it * `tag` -> `conda-tag`: a commit pushed to `main`, and the `conda-*` tag * `pypi` -> `pypi`: the PyPI and TestPyPI uploads * `conda` -> `anaconda`: the anaconda.org upload Also drop the stale commented block the gate came from, folding its `IMPORTANT` note onto the live `id-token: write` it describes. `tests/project/test_release_gates.py` asserts the general rule -- no job here runs a publishing action or a `git push` without naming an environment -- so a publishing job added later without a gate fails rather than ships. The gate is inert until each environment exists in repository settings with a required reviewer, which is the owner's action and cannot be done from here. YAML parses; 13 new tests pass and 3 of them fail without the workflow edit; `tests/project/` is 109 passed, 471 subtests, exit 0. Fixes #641
f282594 to
e9fc31a
Compare
GOOD TO GOCross-review by an independent agent on a different model (Sonnet) from the one that authored the change (Opus), briefed to falsify rather than bless, running read-only. Verdict verbatim: GOOD TO GO — no load-bearing claim failed verification. It reproduced every check itself rather than taking this description as a starting point, and it found one real overstatement in my prose, which I have now fixed (see the last section). Independently confirmedNo publishing path left ungated — the finding that mattered most, since a partial gate would make this PR actively harmful. It read the whole file and enumerated the outward-facing sites itself: 3 ×
The reusable-workflow claim is true, and this is the one I was least certain of. It fetched GitHub's own SchemaStore workflow schema: the The auto-approve premise holds — the other claim I could not verify locally, and the one the whole The b5 protection holds — its own Credentials unchanged — it extracted every The test genuinely fails without the fix — reproduced independently by sha256-tracked file copy rather than by touching git state: 3 failed / 10 passed / exit 1 without, 13 passed / exit 0 with, The environments read-only check — it re-ran What it disputed, and what I changedOne finding, and it was right. An earlier draft of this description called the test's rule "the general form" and claimed it catches "a new publishing job added later without a gate" without qualification. The reviewer's objection: Fixed in It also checked for vacuous assertions and found none — noting that the one theoretically vacuous-risk assertion ( Agreed as out of scope, with a caveat
ProvenanceNo model substitution was needed: Sonnet was the intended reviewing model and the one that ran. One thing this verdict cannot do is record itself anywhere GitHub tracks, unlike a check run — hence posting it here next to the change rather than leaving it in a session transcript. |
Fixes #641.
Merging this PR changes nothing on its own. A job naming an
environment:that does not exist gets that environment created implicitly, with no protection rules, and runs unapproved — so on merge the release path behaves exactly as it does today, while every reader of the file believes it is gated. That is strictly worse than no gate, because nobody looks twice at a protection they think they have.I cannot configure this; it is repository settings, not repository content. For each of the four names below:
The four names:
github-release,conda-tag,pypi,anaconda.Leave "Deployment branches and tags" as "No restriction" on all four. This workflow's
pushtrigger istags: v*, and a branch policy that does not list that tag pattern does not pause the job for approval — it fails it outright withBranch is not allowed to deploy to <environment>. That would break the release path rather than gate it.Half-configured is the failure mode that matters, since the ungated ones stay silently automatic. This repository already ships the worked example:
GET /repos/JarryShaw/PyPCAPKit/environmentsreturnsgithub-pages, created 2021-01-10, with"protection_rules": []— an environment that has existed for five years and gates nothing.Environments that exist today (read-only check, nothing created or modified)
So none of the four exist, and neither existing one has any protection rule.
copilotandgithub-pagesare untouched by this PR.Every job in the file with an outward-facing effect, and what gates it now
I read the whole workflow rather than only the job carrying the commented line. Six jobs; four reach outside the run, and all four are now gated.
githubv*tag with it —action-gh-releasecreates the tag itstag_namenames, and every job below checks that tag outGITHUB_TOKENgithub-releasetagmain(git push origin HEAD:main) and theconda-*tagPYPCAPKITPAT +PYPCAPKIT_ACTIONS_DEPLOY_KEYconda-tagpypipypicondaANACONDA_TOKENanacondaunit-testsenvironment:version_checkcontents: readtagis the one worth calling out: itspermissions: {}makes it read as inert, but the write goes through the deploy key and PAT rather than the run's token, so it pushes to the default branch with no gate at all today.No docs deploy in this file. There is one in
deploy-pages.yml(JamesIves/github-pages-deploy-action@v4) and it is also ungated, but that file is outside this PR's scope and outside #641 — flagging it rather than touching it, and happy to file it separately if you want it.Why four environments rather than one
releaseI agree with your inclination, and went one step further on the same reasoning: one environment per distinct credential and blast radius.
The load-bearing fact is that approval is granted to an environment, not to a job. A single
releaseapproved once releases PyPI and Anaconda together, which renames the problem rather than fixing it. PyPI in particular has to be answerable alone: a version number it has accepted cannot be reused, andskip-existing: truemeans the re-upload then succeeds having published nothing — so an unintended publish permanently consumes the number the intended release wanted.githubandtagare split from each other on the same principle:githubspends the run's own ephemeralGITHUB_TOKEN, whereastagspends a long-lived PAT and a deploy key to write tomain. Different credential, different blast radius, so a separate answer. It also means you can approve a Release without approving a push to the default branch.The cost is two approval waves, not four.
githubandtagneed onlyversion_check, so they arrive together;pypiandcondathen arrive together too. Each wave is one visit to "Review pending deployments" with two checkboxes.One thing I could not verify and am not claiming: whether GitHub requests approval once per environment per run or once per matrix leg.
pypiis a 7-leg matrix andcondaa 10-leg one. The pending-deployments API is keyed by environment, which is why I expect one approval each, but that is a property of the runner and only a real run shows it. The b5 run will settle it.Credentials untouched
No secret invented, renamed, or moved; no change to how any credential is obtained. Trusted publishing still uses OIDC (
# password: ${{ secrets.PYPI_API_TOKEN }}stays commented exactly as it was),condastill usessecrets.ANACONDA_TOKEN,tagstill uses its PAT and deploy key. The onlypermissions:change is cosmetic: theIMPORTANT: this permission is mandatory for trusted publishingnote moved from the dead commented block onto the liveid-token: writeit actually describes.I did delete the stale commented block (
## Specifying a GitHub environment is optional.../#environment: release/ the commentedpermissions:). Leaving#environment: releasesitting beside a liveenvironment: pypiwould be worse than either. That block is the subject of #641, and its duplicate commentedpermissions:above a live one is precisely what made the omission read as deliberate.The b5 validation run
Deliberately untouched, since they are the thing under test: the
Select release bodystep, its absent-file branch, its version-mismatch branch that exits 0 with nopath, its match branch, and thebody_path/generate_release_notes: truewiring. Verified byte-for-byte, not by eye — both regions fromHEADare exact substrings of the new file:and the diff's hunks on the old side are lines 33, 34, 88, 176, 233–237 and 239, 319 — none inside 121–147 or 158–164. The single deletion is 233–237, the commented block.
Expect
v1.5.0b5to pause for approval. That is the feature working, not a regression: once you have configured the environments, the run will stop atgithub/tagand wait for you, then again atpypi/conda. The release-body selection under test runs insidegithub, so it runs after you approve wave 1 — the test still happens, it just happens on your click. If you would rather validate b5 unimpeded, merge this after b5 rather than before; the ordering is yours and nothing here depends on it.YAML verified to parse
actionlintis not installed on this host (actionlint not found), so this is a YAML parse plus a structural read-back:Test:
tests/project/test_release_gates.pyThe rule asserted is not a copy of the current file: any job in this workflow that runs a publishing action or a
git pushmust declare anenvironment:. So it also catches a new publishing job added later without a gate, and a job renamed or reordered.How far that generalises, stated precisely, because it is easy to oversell — and the cross-review caught me overselling it in an earlier draft of this description. The detector is a finite, hand-maintained allowlist of four substrings, so the rule generalises across job identity and not across publishing mechanism. A job uploading by a route none of the four names —
twine upload,gh release upload, a fork of a pinned action under another name, acurlstraight at an index — matches no marker and would pass this test while genuinely ungated. The marker-liveness test only keeps the existing four from going blind; it cannot invent a fifth. Adding a publishing route to this workflow means adding its marker, and that is a maintenance obligation rather than something the test enforces. Both the module docstring and thePUBLISHING_MARKERScomment now say so.Unit tier, and
yamlis not in thetestextra, so the scanner is hand-rolled and dependency-free; a separate test cross-checks it againstyaml.safe_loadwhen PyYAML happens to be installed, which is what stops the substitute from drifting. Same split astest_the_rewritten_date_is_a_string_and_not_a_yaml_dateintest_bump_version.py.Also pinned, against fixtures rather than the repo so they outlive this file: that
#environment: releasereads as ungated (the exact #641 shape), that a comment naming a publishing action is not evidence a job publishes, and that theenvironment:/name:mapping form is read.Passing, with the fix — exit code read from the file, and the subtest tally is explicit rather than inferred from a
PASSEDline:Failing, without it. I restored
HEAD's workflow by file copy (not by touching git state), re-ran, then restored the fix and confirmed the file was byte-identical again:with the message naming all four, and why each needed a gate:
Scoped suite, unaffected:
No CI verification claimed — the queue is deep and this may not report for a while.
Labels
cionly — the change is workflow configuration, matching theci:subject prefix, as #658 (ci(release): ...) was labelled.Not
breaking, and the label's own description is what decides it: "Alters public API or wire output". This alters neither — nopcapkitAPI, CLI, or wire behaviour moves, and an installing user sees no difference. Two further reasons, in case the intent is broader than the description:breakingon this commit would point at the wrong commit.I'd still call out the real behavioural consequence plainly, since it is the point of the change rather than a side effect: once configured, a tag push no longer publishes on its own. The
bump → tag → PyPI in a median three minuteschain becomesbump → tag → wait for a human. If you would rather that were flagged asbreakinganyway, say so and I'll add it — it is additive.Not
testdespite adding a test module: the type label tracks the subject prefix, and this isci:.Not in scope
No changelog entry on this branch —
CHANGELOG.mdanddocs/source/changelog/are untouched here, and the bullet goes to #657 (docs/changelog-1.5.0) as its own commit..github/release.ymluntouched. Nothing underpcapkit/touched; this is workflow-only plus its test, as expected.