Realign the release branch on its remote during checkout - #5686
Realign the release branch on its remote during checkout#5686AliSoftware wants to merge 6 commits into
Conversation
`checkout-release-branch.sh` fetched the release branch then checked it out, but never moved the local branch to the fetched commit. Buildkite cleans the working copy between jobs, yet can reuse it — so a `refs/heads/release/x.y` left behind by an earlier job on the same agent survives, and `git checkout` then just switches to that stale local ref instead of the freshly fetched remote one. Anything running afterwards, such as the version bump and the GitHub Release draft created by `finalize_release`, would target the wrong commit. Adding `git reset --hard "origin/$BRANCH_NAME"` after the checkout makes the branch match the remote unconditionally. `reset --hard` rather than `git pull`: it needs no extra network round trip and cannot produce a merge if the local and remote refs have diverged. This is the second part of AINFRA-2725, a follow-up to the WooCommerce iOS 25.1 release incident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @AliSoftware's task in 2m 42s —— View job Code review
The fix is correct and addresses a real failure mode. Two hardening points, both about the same "reused working copy" premise the PR is built on. Neither is a blocker. 1. The reset trusts a remote-tracking ref that
|
|
About the 2 points Claude raises:
|
"realign it on the remote" read as though the operation happened *on* the remote, rather than describing what the local branch is realigned against. Say plainly what the reset does instead: force the local branch to the fetched commit. Wording suggested by @mokagio in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @AliSoftware's task in 1m 3s —— View job Re-review of
|
`git fetch origin <branch>` always writes `FETCH_HEAD`, but it only updates `refs/remotes/origin/<branch>` when the remote's configured fetch refspec covers that branch. With the default `+refs/heads/*:refs/remotes/origin/*` that Buildkite sets up, the two are equivalent — but on a clone whose refspec was narrowed after `origin/<branch>` already existed, the fetch leaves that ref stale and `reset --hard "origin/$BRANCH_NAME"` silently lands on the old commit: exactly the failure this script is meant to prevent, reintroduced through the back door. Resetting to `FETCH_HEAD` removes the dependency on the refspec entirely — it is whatever the line above just fetched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The script had drifted into five different shapes across the repos: argument required via `${1?…}` or `${1:?…}`, argument plus a `BUILDKITE_BRANCH` fallback, argument with a hand-rolled usage check, the same under a different variable name, and — in one repo — no argument at all, reading `RELEASE_VERSION` from the environment. Having rolled the same one-line fix out thirteen times this week, the divergence is pure friction, so this settles on a single canonical version.
The resolution order is a superset of what every repo did before — argument, then `RELEASE_VERSION` from the environment, then the `release/*` branch the build runs on — so no call site needed changing. The `BUILDKITE_BRANCH` fallback only ever fires on a branch matching `^release/`, and it derives the branch name back from that same value, so it cannot select a branch other than the one the build was already triggered on.
It also closes two latent bugs. Under `bash -eu`, `[[ -z "${RELEASE_VERSION}" ]]` on an unset variable and a bare `RELEASE_VERSION=$1` with no arguments both abort with `unbound variable` before their intended usage message can print. And an argument that is passed but empty — which happens when a pipeline forwards an unset `$RELEASE_VERSION` — is now a hard error everywhere, rather than resolving to `release/` or silently falling through to the current branch.
The redundant `echo '--- :git: Checkout Release Branch'` in simplenote-android's pipelines is dropped, since the canonical script prints that group header itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @AliSoftware's task in 4m 10s —— View job Re-review of
|
|
@mokagio heads-up — these PRs grew since your review, so they're worth another look rather than assuming they're still the three-line change you saw. Same comment on all 13 repos. Two commits were added on top of the wording fix you suggested: 1. Reset to 2. Standardized the script across all 13 repos. Tangent to the original issue, bundled deliberately. The script had drifted into five different shapes — argument required via All 13 now share one canonical script. The resolution order is a superset of what every repo did before — argument, then It also closes two latent bugs: under The scripts are byte-identical across all 13 repos and pass Worth flagging that the release-toolkit PR also picked up a real fix from your review — the tag-based lookup was preferring a leftover draft over the release that actually owns the tag. |
Collapse the three-way `if/elif/else` into a plain assignment plus two guards. The `: # Already provided through the pipeline environment` no-op branch existed only to skip reassigning a value that was already correct, which reads oddly for anyone who has not just written it.
The one behavioural difference is that an argument that is passed but empty no longer gets its own dedicated error: it now falls through to the environment variable, then to the `release/*` branch, and finally to the same generic error as the unset case. That is a rare enough situation to not be worth a distinct branch, and when the fallback does catch it, it resolves to the branch the build is already running on, which cannot be a different branch than intended.
Note the nested guard in `${1:-${RELEASE_VERSION:-}}`: written as `${1:-$RELEASE_VERSION}`, the default expression itself dereferences an unset variable, so under `bash -eu` the script would abort with `RELEASE_VERSION: unbound variable` when neither is set — the same latent failure this standardization removed from a couple of the repos.
Also say "a different commit" rather than "an older commit" when describing the reused working copy, since a stale local ref is not necessarily behind the remote.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @AliSoftware's task in 3m 30s —— View job Re-review of
|
| input | origin/main |
this PR |
|---|---|---|
| non-empty arg | arg | arg |
empty arg, on release/X |
X |
X |
| empty arg, on trunk | error | error |
no arg, on release/X |
X |
X |
no arg, RELEASE_VERSION in env |
error | env value (new tier) |
| nothing available | error | error |
So the note I asked for in the PR description isn't needed any more — the description is accurate again as written.
The nested :- guard is necessary, and the commit message gets the reason exactly right
Worth confirming since it's the kind of thing a future reader may try to "simplify" back: bash evaluates the default word of ${1:-word} lazily, so ${1:-$RELEASE_VERSION} would not abort when an argument is passed — it would abort only when neither $1 nor RELEASE_VERSION is set, i.e. precisely the case that needs to reach the usage error on line 27. Under bash -eu the script would die with RELEASE_VERSION: unbound variable instead. The inner :- is load-bearing and the comment history explains why; nothing to change.
Also confirmed the self-reference on line 20 is safe — the RHS is fully expanded before the assignment — and that reassigning an already-exported RELEASE_VERSION preserves the export attribute, so nothing downstream sees a difference (moot here anyway, since every caller runs this as a subprocess rather than sourcing it).
Two notes on the trade-off you accepted, neither a blocker
Env now outranks the branch fallback for an empty argument. New ordering relative to origin/main, which had no env tier: empty arg + runtime RELEASE_VERSION=X + build on release/Y resolved to Y before and resolves to X now. Unreachable from any call site in this repo — nothing sets RELEASE_VERSION in these jobs' runtime env; it's interpolated into the argument at upload time — and the explicit variable is the better signal if it ever is set. Noting it only because it's the one row where the two versions disagree on a non-error outcome.
Empty argument is now a silent recovery rather than a loud failure. Your commit message justifies this on the grounds that the fallback "resolves to the branch the build is already running on, which cannot be a different branch than intended". That holds for finalize-release.yml and new-beta-release.yml. For .buildkite/release-pipelines/finalize-hotfix-release.yml:12 it rests on ReleasesV2 triggering the job on the hotfix branch (release/X.Y.Z, as created by new_hotfix_release) rather than on the base release/X.Y — if it ever triggered on the base with RELEASE_VERSION unset at upload, the script would now silently finalize the base release instead of erroring. That's origin/main's behaviour restored, not a regression this PR introduces, and I believe the trigger branch is the hotfix branch in practice. Flagging it because this PR descends from a wrong-commit-released incident, and that one call site is the only place cb5363c's stricter variant had a defensible edge. Your call, and I'd accept it as-is.
.buildkite/commands/release-build.sh is unaffected either way — its own ${1:?RELEASE_VERSION parameter missing} on line 4 fires on an empty value first, so it can never forward one.
Checked and clear
- My earlier point 1 (
FETCH_HEAD) stays closed. Lines 32-38 are untouched by this commit. The adjacency coupling between the fetch and the reset is still a maintenance note only, not a defect. - "a different commit" replaced "an older commit" on line 34, which was my suggestion — a stale local ref genuinely needn't be behind. Line 34 is now ~114 chars while 35-37 sit around half that; still nothing enforces wrapping here (
spotlessCheckdoesn't cover.buildkite/**), so still ignorable. - Header comment, line 7: "the first of these that is set and non-empty" is accurate for tiers 1-2; tier 3 additionally requires
^release/, and a branch named exactlyrelease/yields an empty version and lands on the generic error. Correct behaviour, marginally under-described comment. Not worth a commit. - Security. Unchanged surface.
$BRANCH_NAMEderives from$1/RELEASE_VERSION/BUILDKITE_BRANCH, is consistently quoted, and reachesgitas a single argv element — no injection path. All callers still run this beforeconfigure_apply, so no secrets are in play.BUILDKITE_BRANCH=release/1.2/3round-trips to the same branch name, so the strip-and-reprefix can't retarget. - Performance. No change;
reset --hardremains local-only. - Still the only git branch manipulation anywhere under
.buildkite/.
Merge gate
Gemfile.lock:169 is still fastlane-plugin-wpmreleasetoolkit (14.11.1), so the bump your description makes a merge precondition hasn't landed. Still blocked on release-toolkit#763 → gem release → bump here.
I did not empirically run the resolution matrix this time: the sandbox declined executing a scratch harness, and shellcheck isn't installed on this runner, so the table above is traced from bash parameter-expansion semantics rather than observed. Your own cross-repo test run covers the same combinations. No builds or tests run — the diff is one .buildkite shell script with no Gradle, Kotlin, or Ruby surface.
· branch ainfra-2725/reset-release-branch-checkout
The two justifications—`reset --hard` over `git pull`, and `FETCH_HEAD` over the remote-tracking ref—were run together in a prose paragraph that wrapped mid-clause, so neither stood out. Split them into bullets under the sentence stating what the reset does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @AliSoftware's task in 49s —— View job Re-review of
|
Note
Please do not merge this PR yet. It should wait until wordpress-mobile/release-toolkit#763 is merged and a new version of the
release-toolkitgem has been released, so that thefastlane-plugin-wpmreleasetoolkitversion can be bumped inGemfile.lockas part of this same PR. Both halves of AINFRA-2725 then land together.What does it do?
Part of AINFRA-2725, following the WooCommerce iOS 25.1 release incident. The same change is going out to every mobile product repo.
1. Realign the release branch on the remote (the actual fix)
.buildkite/commands/checkout-release-branch.shfetched the release branch and checked it out, but never moved the local branch onto the fetched commit:Buildkite cleans the working copy between jobs, but it can reuse it. A
refs/heads/release/x.yleft behind by an earlier job on the same agent therefore survives, andgit checkoutthen simply switches to that stale local ref rather than to what was just fetched. Whatever runs next — the version bump, or the GitHub Release draft thatfinalize_releasecreates fromHEAD— would then be based on the wrong commit.The fix adds the missing realignment.
reset --hardrather thangit pull: no extra network round trip, and no merge commit if the refs diverged.2. Reset to
FETCH_HEADrather than the remote-tracking refgit fetch origin <branch>always writesFETCH_HEAD, but it only updatesrefs/remotes/origin/<branch>when the remote's fetch refspec covers that branch. With Buildkite's default+refs/heads/*:refs/remotes/origin/*the two are equivalent — but on a clone whose refspec was narrowed afterorigin/<branch>already existed, the fetch leaves that ref stale andreset --hard "origin/$BRANCH_NAME"lands on the old commit: the very failure this script exists to prevent, reintroduced through the back door. Resetting toFETCH_HEADdrops the refspec dependency entirely.3. Standardize the script across all repos
Slightly tangent to the issue, but bundled deliberately: the script had drifted into five different shapes across the repos — argument required via
${1?…}or${1:?…}, argument plus aBUILDKITE_BRANCHfallback, argument with a hand-rolled usage check, the same under a different variable name, and (in one repo) no argument at all, readingRELEASE_VERSIONfrom the environment. Having rolled the same one-line fix out thirteen times this week, that divergence is pure friction.All repos now share one canonical script. The resolution order is a superset of what every repo did before — argument, then
RELEASE_VERSIONfrom the environment, then therelease/*branch the build runs on — so no call site needed changing. TheBUILDKITE_BRANCHfallback only fires on a branch matching^release/, and derives the branch name back from that same value, so it cannot select a branch other than the one the build was already triggered on.It also closes two latent bugs: under
bash -eu, both[[ -z "${RELEASE_VERSION}" ]]on an unset variable and a bareRELEASE_VERSION=$1with no arguments abort withunbound variablebefore their intended usage message can print. And an argument that is passed but empty — what happens when a pipeline forwards an unset$RELEASE_VERSION— no longer resolves to a barerelease/: it falls through to the environment variable, then to therelease/*branch the build runs on, and finally to a clear error if none of those yields a version.Testing instructions
No behaviour change on a fresh checkout, which is the normal case: the local branch is already at the fetched commit, so the reset is a no-op. The argument-resolution logic was exercised across all combinations (argument / empty argument / environment variable /
release/*branch / trunk / feature branch / unset), and the resulting script passesshellcheckin every repo. The next release build exercising this script is the real check.🤖 Generated with Claude Code