fix(ci): only deploy from main, and never re-release a shipped version - #1014
fix(ci): only deploy from main, and never re-release a shipped version#1014simonoppowa wants to merge 3 commits into
Conversation
The five deploy guards pinned the ref on the `push` arm only:
((github.event_name == 'push' && github.ref == 'refs/heads/main') ||
github.event_name == 'workflow_dispatch') && ...
`workflow_dispatch:` accepts no branch filter — GitHub does not offer
one — so the dispatch arm accepted any branch or tag in the repository.
A manual run from a topic branch got the full release: a TestFlight
upload, a Play versionCode consumed, and a `v*` tag cut at that branch's
head.
Not hypothetical. Run 26086317616 was a `workflow_dispatch` on
`integrate-cicd-deployment` and carried ios-package, android-package,
ios-deploy, android-deploy and github-release all to success — a
release published from a feature branch. We got away with it because
that head was later merged and every tag this repo has cut happens to
point at a commit reachable from `main`. Nothing in the guard required
that.
Testing the ref first, for every event, is a pure narrowing: the push
case is unchanged and dispatch is restricted to `main`. Dispatch stays
available there, so the manual recovery path is intact.
Restricted to `main` rather than also allowing `release/**`: every
release this repo has cut carries `target_commitish: main`, and 2.2.0
shipped from the push to `main` after #988 merged, not from the release
branch. Both release branches that have existed only ever ran
`pull_request` checks.
Verified: dispatch from a feature branch, from `release/2.2.0` and from
a tag are all refused; dispatch on `main`, push to `main` and the
pull-request case are unchanged.
Refs #1011
Fixing the deploy-chain skip (a8a7899) exposed this. Until then the five deploy jobs silently skipped on every push to `main`, so it did not matter that they are keyed on nothing but the branch. They fire for real now, and `push: main` fires on every merge — docs-only ones included. With `pubspec.yaml` still at `2.2.0+63` the morning after 2.2.0 shipped, the next merge to `main` re-runs the whole chain against an already-published version. Today that goes red rather than destructive: `ios-deploy` re-uploads build 63, Apple answers ITMS-90189, pilot raises, and because `github-release` requires `ios-deploy` to have succeeded it never runs. Ordering is the only thing protecting the release — and it costs ~41 macOS runner minutes to discover. Ordering is not a guarantee. Bump only the build number, `2.2.0+64`, and iOS passes because 64 is new to TestFlight, `TAG_NAME` is still `v2.2.0` because the build suffix is stripped, and `github-release` runs against an existing tag. softprops/action-gh-release does not fail there: it updates the release in place, and `overwrite_files` defaults to true, so it deletes the published IPA, AAB and APK and re-uploads binaries built from a different commit while the tag stays where it was. GitHub does not version release assets, so that deletion is permanent. The result would be a published release whose binaries do not match its tag, from a green run, with no warning. `release-gate` reads the ledger the pipeline itself writes — the `v<version>` tag that `github-release` creates in the last step of the last job. Tag present means a release carrying that version name ran to completion, so there is nothing to ship; the five deploy jobs skip and the run stays green with a notice. Tag absent means ship. Gating on the tag rather than on "did pubspec change in this push" survives re-runs, which RELEASING.md tells releasers to do, and does not depend on `github.event.before`, which is unusable after a force-push and absent for `workflow_dispatch`. `git ls-remote --exit-code` rather than a REST call, because it separates the three answers into distinct exit codes: 0 released, 2 not, anything else the question went unanswered. "Could not reach the remote" must never read as "no tag, go ahead and release", so that case fails the job and the chain stays shut. `overwrite_files: false` goes on the release step as a backstop, so that if a tag ever does slip through, a silent corruption becomes a failed step. Consequence, chosen deliberately: a build-only bump can no longer deploy. `2.2.0+64` derives `v2.2.0` and is refused. That is the exact input that arms the overwrite above, and RELEASING.md already asks for both halves of the version to move. Verified against the real remote: 2.2.0+63 and 2.2.0+64 both resolve to an existing v2.2.0 and are refused; 2.2.1 and 2.3.0 are accepted; the exact ref path does not false-positive on v1.3.2+53-build.629. Full outcome matrix passes — a real release ships from a push or a dispatch on main, while an already-released version, a skipped or failed gate, a dispatch from a feature branch, a release branch or a tag, a pull request, a push to develop, a failed integration suite and a cancelled run all stay shut. Refs #1011, #1012
There was a problem hiding this comment.
🟢 Approval recommended
The workflow logic changes are coherent and narrowly scoped to preventing unintended releases, with only a minor doc-comment nit identified.
Pull request overview
This PR hardens the release pipeline in .github/workflows/default_workflow.yml by (1) ensuring deploy jobs can only run from refs/heads/main (including workflow_dispatch), and (2) adding a release-gate job that prevents re-releasing a version that already has a corresponding v<version> tag (plus a safety backstop to prevent overwriting existing GitHub release assets).
Changes:
- Add a
release-gatejob that checks whetherv<pubspec version>already exists on the remote and exposes ashipoutput used by deploy jobs. - Update all deploy job
if:guards to requiregithub.ref == 'refs/heads/main'for bothpushandworkflow_dispatch, and to requirerelease-gatesuccess +ship == 'true'. - Add
overwrite_files: falseto the GitHub Release step to prevent destructive asset replacement if a tag collision ever occurs.
File summaries
| File | Description |
|---|---|
| .github/workflows/default_workflow.yml | Adds a remote-tag-based release gate, tightens deploy guards to main for push/dispatch, and prevents GitHub release asset overwrite. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # The literal string 'true' and nothing else. Every gate below compares | ||
| # against it, so the empty string a skipped or failed job leaves behind | ||
| # reads as "do not ship" without any extra handling. |
Reworks the gate this branch added. The first version answered one
question — does `v<version-name>` already exist — and gated all five
deploy jobs on it. That refuses a build-only bump: 2.2.0+63 to 2.2.0+64
is a new binary under an existing version name, and both derive
`v2.2.0`. A build-only bump is a legitimate TestFlight release and must
ship.
Two questions were being conflated, with different ledgers and different
stakes:
deploy — is there a NEW BUILD for the stores? Keyed on the build
number. Read by the four packaging and deploy jobs. Getting
it wrong costs a red run and two store rejections.
publish — is `v<version-name>` still unclaimed? Keyed on that tag.
Read by `github-release` alone, because that is the only
destructive path: on an existing tag the action does not
fail, it updates in place, and `overwrite_files` defaults to
true, so it would delete the published IPA, AAB and APK and
re-upload binaries from another commit. Permanent — GitHub
does not version release assets.
The build ledger is a `deployed/<n>` tag pushed by both deploy jobs on
success. Keyed on the build number alone: a Play versionCode is spent
forever regardless of which version name carried it, and a
version-qualified key would wave through the commonest slip — bumping
the name and forgetting the build.
The ledger starts empty, so it carries a hardcoded floor,
LAST_BUILD_BEFORE_LEDGER=63, rather than a tag seeded by hand. A
pre-merge step done exactly once is the step that gets forgotten, and
`release-gate` has no `needs:` — it reaches the question thirty seconds
into the run, so there is no window to win afterwards. Without the floor
the very first run reads build 63 as new and re-pushes it to both
stores, which now fails on Play as well since 63 was uploaded by hand.
The floor also catches what a marker ledger cannot: build numbers are
not monotonic here. 1.3.1+51 and 1.4.0+51 both used versionCode 51, and
1.4.0+51 follows 1.3.2+55.
Unreachable remote, malformed version, non-integer build number, or a
name bumped with the build forgotten all fail the job loudly rather than
guessing. No `needs:` entry changes anywhere.
The runbook is updated in the same change, because this alters what a
releaser has to know and a stale runbook is what nearly ran 2.2.0.
Verified against the real remote: 2.2.0+63 gives deploy=false
publish=false and a green run — which is exactly what this PR's own
merge commit is; 2.2.0+64 ships to both stores without touching the
v2.2.0 release; 2.3.0+64 does both; 2.3.0+63, 2.2.0+abc and an
unreachable remote exit 1; 1.4.0+51 is caught by the floor. Outcome
matrix passes and actionlint reports no new findings.
Refs #1011, #1012
There was a problem hiding this comment.
🟡 Changes recommended
The new “record deployed/ tag” fallback treats any existing tag as success without verifying it points at the current SHA, which can mask build-number collisions/concurrency issues (plus a couple of misleading/incorrect messages in the new gate logic).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/default_workflow.yml:1479
- Same as the iOS deploy ledger step: after a failed
git push, the fallback exits 0 as long as the tag exists, without checking whether it points at the current commit. To avoid masking a build-number collision (tag exists at a different SHA), compare the remote SHA to the local SHA and fail if they differ.
if git ls-remote --exit-code --tags origin "refs/tags/$BUILD_TAG" >/dev/null 2>&1; then
echo "::notice::$BUILD_TAG was already recorded (the sibling deploy job, or a concurrent run, won the race). The build number is spent either way, which is all the ledger claims."
exit 0
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
| if git ls-remote --exit-code --tags origin "refs/tags/$BUILD_TAG" >/dev/null 2>&1; then | ||
| echo "::notice::$BUILD_TAG was already recorded (the sibling deploy job, or a concurrent run, won the race). The build number is spent either way, which is all the ledger claims." | ||
| exit 0 | ||
| fi |
| # Both are the literal string 'true' and nothing else, so the empty | ||
| # string a skipped or failed job leaves behind reads as "do not ship" | ||
| # without any extra handling. |
| 0) REF_EXISTS=yes ;; | ||
| 2) REF_EXISTS=no ;; | ||
| *) | ||
| echo "::error::Could not read tags from origin (git ls-remote exit $ls_status) while asking whether refs/tags/$1 exists. Refusing to guess whether this build has already shipped." |
| **Bumping the build number alone is a supported release.** `2.2.0+63` → `2.2.0+64` ships to | ||
| TestFlight and Play under the same version name. On that path `github-release` is **skipped | ||
| on purpose**, because `v2.2.0` already exists and re-running it would delete the published | ||
| release's assets and replace them with binaries from another commit. A skipped | ||
| `github-release` there is expected, not the #1008 skip bug — the run is green and | ||
| `release-gate`'s step summary says which of the two happened. The consequence: a build-only | ||
| bump publishes no GitHub release, so its IPA and AAB exist only as run artifacts, for 90 | ||
| days. Bump the name as well when the build should have a durable public download. |
The
develophalf. Merging intodeveloptriggers no run at all (on.push.branchesis[main]only), but this is what makes every future branch born safe — a dispatch runs the workflow file from the selected ref, so the guard only protects branches that carry it.1 · Only ever deploy from
main— #1011The five deploy guards pinned the ref on the
pusharm only, so theworkflow_dispatcharm accepted any branch or tag — GitHub offers no branch filter for that trigger. Run 26086317616 was a dispatch onintegrate-cicd-deploymentand carried all five jobs to success: a release published from a feature branch.The ref is now tested first, for every event. Pure narrowing; dispatch on
mainstill works, so the manual recovery path stays. Restricted tomainalone, notrelease/**— every release carriestarget_commitish: main, and 2.2.0 shipped from the push tomainafter #988.2 · A gate that answers two questions, not one
Fixing the deploy-chain skip (#1008) means
push: mainnow really deploys, on every merge — docs-only ones included. Withpubspec.yamlat2.2.0+63the morning after 2.2.0 shipped, the next merge re-runs the chain against an already-published version.Two questions were being conflated:
deploypublishv<name>still unclaimed?github-releaseonlyGating both on the tag — the first version of this branch — refuses a build-only bump, because
2.2.0+63and2.2.0+64both derivev2.2.0. A build-only bump is a legitimate TestFlight release and now ships, whilegithub-releaseskips so the publishedv2.2.0is never touched.That destructive path is real:
softprops/action-gh-releasedoes not fail on an existing tag. It updates in place, andoverwrite_filesdefaults to true — deleting the published IPA, AAB and APK and re-uploading binaries from another commit while the tag stays put. GitHub does not version release assets.overwrite_files: falseis kept as a backstop.The build ledger, and why there is nothing to run before merging
A
deployed/<n>tag pushed by both deploy jobs on success, keyed on the build number alone — a Play versionCode is spent forever regardless of which version name carried it, and a version-qualified key would wave through the commonest slip: bumping the name and forgetting the build.The ledger starts empty, so it carries a constant floor,
LAST_BUILD_BEFORE_LEDGER=63, rather than a tag seeded by hand. A pre-merge step performed exactly once is the step that gets forgotten, andrelease-gatehas noneeds:— it reaches the question ~30 seconds in, so there is no window to win afterwards. Without the floor the first run reads build 63 as new and re-pushes it to both stores, which now fails on Play too since 63 was uploaded manually.The floor also catches what a marker ledger cannot: build numbers are not monotonic in this repo.
1.3.1+51and1.4.0+51both used versionCode 51, and1.4.0+51follows1.3.2+55.This PR's own merge is safe
A push runs the workflow at the pushed commit, so the merge commit's run includes the gate. It reads
2.2.0+63, hits the floor, emitsdeploy=false publish=false, and all five jobs skip. Green, no store touched, no macOS minutes burned.Verified — the gate script executed against the live remote
Outcome matrix passes: ships on a push or dispatch on
mainwith a new build; does not ship for a docs merge, a skipped or failed gate, a dispatch from a feature branch or tag, a pull request, a push todevelop, a failed integration suite, or a cancelled run.actionlintreports no new findings, all five guards still mirror theirneeds:exactly, and noneeds:entry changes anywhere.Runbook updated in the same change
docs/RELEASING.mdnow covers: a build-only bump as a supported release; that a skippedgithub-releasethere is expected, not the #1008 bug; that such a build publishes no GitHub release, so its artifacts expire in 90 days; that "Re-run failed jobs" is correct and "Re-run all jobs" strands the release; and thatdeployed/<n>records what a store consumed, not what it published — with the #942 tolerance, a build nobody hand-uploaded is still spent. It also corrects two places, one of themandroid-deploy's own step summary, that told the releaser to take the AAB from a GitHub release that a build-only bump never creates.Known limits, stated rather than hidden
v2.2.0keeps describing build 63. That is the price of never overwriting a published release.concurrency:group. Two release-bearing pushes inside the same window could both see no marker; the second then fails red on a duplicate build number. Never destructive. Say the word and I'll addcancel-in-progress: false.Refs #1011