chore: enable release-plz trusted publishing - #65
Conversation
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The workflow change is correct and agrees with docs/research/release-plz.md. The publish job is separate from the release-PR job. Only the publish job has id-token: write. It has no concurrency group and no registry token. All actions are pinned by SHA, and the CLI version is pinned. The invariant script in test.yml is a useful guard.
Blocker (repository setting, not the diff): the PR description says the job uses the protected release environment. The live environment is not protected. GET /repos/cipherstash/envelopers/environments shows release with protection_rules: [] and deployment_branch_policy: null. Thus any branch can deploy to it, and no reviewer must approve.
This is important because crates.io Trusted Publishing matches the owner, repository, workflow filename and environment, but not the git ref. The job-level if: guard is in the workflow file, so a person who can push a branch can change it. Thus, a person with write access (or a leaked token with contents/workflows write) can push a non-main branch that runs a modified release-plz.yml with environment: release, and then publish to crates.io. Only the deployment branch policy of the environment can stop this on the server side.
Before this merges (and before #64 merges), please set these on the release environment:
- Deployment branches: Selected branches →
mainonly. - Required reviewers, as the research doc checklist says ("Create and protect the
releaseGitHub Environment"). Also think about whether admins can bypass (can_admins_bypassis currentlytrue).
When the environment is protected, I will approve. The two inline comments about app-id and the checker script are optional improvements and do not block this PR.
create-github-app-token v3 deprecates app-id in favour of client-id. The workflow invariant script now also asserts the release environment binding required by crates.io trusted publishing.
|
All review feedback is addressed:
|
Requested changes have been made and verified by another engineer
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Verdict: request changes
The workflow itself is well constructed — SHA pins, persist-credentials: false, fetch-depth: 0, least-privilege top-level permissions, publish-job-only id-token: write, and a deliberate absence of a concurrency group on the publish job. It matches docs/research/release-plz.md closely.
Two things need resolving before merge:
- Out-of-band prerequisites. This PR switches on unattended crates.io publishing, but the repo's own checklist still has "Create and protect the
releaseGitHub Environment" and "Configure crates.io Trusted Publishing" unticked. GitHub auto-creates a referenced environment with zero protection rules, so the approval gate the design promises may not actually exist. Same class of risk for theRELEASE_PLZ_APP_ID->RELEASE_PLZ_APP_CLIENT_IDrename, which breaks the existing release-PR job if the new variable is not already set. - The guard script cannot prove the invariant it advertises.
assert_countcounts line occurrences, so it cannot express "environment: releaseandid-token: writeare on the same job" — and it does not check the fork/event guard at all. A structuralyqassertion is both stronger and immune to reformatting.
Review stats
| Source | Model | Type | Raw | Kept |
|---|---|---|---|---|
| claude | claude-opus-5 | test-gap | 3 | 2 |
| claude | claude-opus-5 | infracode | 5 | 5 |
| codex | gpt-5.6-terra | test-gap | 1 | 1 (merged) |
| codex | gpt-5.6-terra | infracode | 1 | 1 |
| Total | 10 | 8 inline + 4 overflow |
Cross-model overlap: 1 kept finding was corroborated by 2+ models (the missing if: guard assertion, raised independently by claude's test-gap pass and codex's test-gap pass). The remaining 7 are single-source but verified against the diff. Two claude findings were merged into one (the script's hardcoded relative path appeared as both a test-gap and an infracode note).
Dropped: 1. Claude's test-gap proposal to add scripts/check-release-workflow-test.sh — a self-test for a 35-line grep lint, with a parameterised workflow="${1:-...}" path purely to make it mutable. Real in principle, but the structural yq rewrite recommended inline removes most of the fragility it was protecting against, at lower cost.
Additional findings not posted inline
docs/research/release-plz.md:272is now stale. The checklist still reads "Add App ID/private-key credentials to GitHub Actions" while the workflow and line 130 of the same doc have moved to Client ID. Same file, same PR — worth fixing here.- The implementation checklist was not updated. Step 5 of the rollout plan is exactly what this PR does, and several boxes (SHA-pinned release-PR job, pin and record action SHA + CLI version,
release-plz.tomlwithrelease_always = false) are genuinely done. Tick them so the doc stays a usable record of what is and is not configured. - CI step placement.
scripts/check-release-workflow.shis a pure shell/grep check but sits in the Rustqualityjob afteractions/checkout, toolchain install, andcargo fmt. Moving it to the first step of that job puts the ~1 min toolchain setup off the failure path. assert_countcomparesgrep -coutput to the expected value as strings, soassert_count 01 ...would never match. Not a live problem with the current call sites; noted so it does not surprise someone later.
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
@tobyhede thanks for this.
Approving, in anticipation of the @cipherstash-bot review findings being addressed.
Rewrite check-release-workflow.sh to assert the workflow's structure with yq instead of counting exact lines. It now verifies that the release job's push-to-main guard, release environment, and id-token permission all sit on the publish job, that no other job gets them, and that harmless reformatting or SHA bumps still pass. The publish job's default GITHUB_TOKEN drops to contents: read, since release-plz uses the GitHub App token. Both jobs now use the same repository guard, the workflow_dispatch and concurrency choices are commented, and the research doc checklist is updated.
|
@cipherstash-bot findings are addressed in 612951e, and CI is green:
|
The release job is bound to the protected release environment, so every push to main created a deployment waiting for approval even when there was nothing to publish. A read-only release-gate job now looks up the PR behind the pushed commit and lets the release job run only when it is a merged release-plz PR opened by the release App.
4 participants
Closes #66.
releasejobreleaseenvironment (deployments limited tomain,rustteam approval required)id-token: writemainincipherstash/envelopersThe existing
workflow_dispatchpath skips publishing and runs only the release-PR job.Important: generated release PR #64 remains open. Merging #64 after this change lands will publish
0.8.4; that remains a separate maintainer decision.Validation:
actionlint .github/workflows/release-plz.ymlRUSTC_WRAPPER= cargo check --all-targets --all-featuresRUSTC_WRAPPER= cargo test(28 unit tests, 3 doc tests)