ci: require stable releases from main#6
Conversation
jwfing
left a comment
There was a problem hiding this comment.
Review: ci: require stable releases from main
Summary: A small, correct hardening of the PyPI publish workflow that blocks stable releases whose tag does not resolve to a commit reachable from main, while leaving prerelease tags untouched.
Requirements context
No matching spec/plan found under docs/superpowers/ — the only docs there (specs/2026-03-28-python-sdk-design.md, plans/2026-03-28-python-sdk.md) cover the SDK design and contain no release/CI/publishing requirements (grep for publish|release|main|ancestry|prerelease|pypi returned nothing). Assessed against the PR description alone.
I verified the crux of correctness — that origin/main is actually available at check time — against the actions/checkout docs via context7: with fetch-depth: 0, getRefSpecForAllHistory() always fetches +refs/heads/*:refs/remotes/origin/*, so the origin/main remote-tracking ref is populated even on a tag-push checkout. The added fetch-depth: 0 is therefore necessary and sufficient for the merge-base --is-ancestor … origin/main check to work. ✅
Findings
Critical
(none)
Suggestion
- Functionality / diagnostics —
.github/workflows/publish.yml:44-47.if ! git merge-base --is-ancestor "$release_commit" origin/maintreats every non-zero exit as "not an ancestor".merge-base --is-ancestorreturns1for "not ancestor" but128for an error (e.g. an unresolvable ref). Givenfetch-depth: 0this shouldn't happen, but iforigin/mainwere ever missing the run would fail with the misleading "must point to a commit contained in main" message rather than a fetch error. Optional: explicitly assertgit rev-parse --verify origin/mainfirst, or branch on the exit code, so a genuine ref/fetch failure is distinguishable from a real ancestry rejection.
Information
- Security —
.github/workflows/publish.yml:43-47. No injection concern: this branch is only reachable after line 36-39 has assertedRELEASE_TAG == "v$package_version"andpackage_versionhas matched^[0-9]+\.[0-9]+\.[0-9]+$, soRELEASE_TAGis constrained tov<digits.digits.digits>and is additionally double-quoted in every use. No secrets/PII touched;permissions: contents: readon thebuildjob is unchanged. No security-relevant regressions. - Performance —
.github/workflows/publish.yml:23-24.fetch-depth: 0pulls full history for all branches/tags, marginally slower than the default shallow clone, but it is required for ancestry validation and negligible for a release-only workflow. No concern. - Software engineering — testing. Workflow-logic changes aren't unit-testable here, and the PR lists appropriate verification (
actionlint,compileall, unit tests,python -m build,twine check). The gating placement is correct: the check lives in thebuildjob's version step, and bothpublish(needs: build) andrelease(needs: [build, publish]) are skipped if it exits non-zero, so a rejected tag never reaches PyPI. The stable/prerelease split matches the PR's stated intent — prerelease versions fail the^X.Y.Z$regex, setstable=false, and skip the ancestry check entirely.
Verdict
approved (informational — no Critical findings; human approval remains a separate GitHub action). The change is correct and well-scoped; the single Suggestion is optional robustness polish, not blocking.
Summary
mainVerification
actionlintpython -m compileall -q insforge testspython -m buildpython -m twine check dist/*