refactor: simplify release script around merge-commit-based releases#12235
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace SHA-driven state logic and tests with boolean/ancestry/content flags; add git-cherry parsing and a safe/unaccounted classifier; refactor release/hotfix flows to use resolved private-sync/backmerge branches and step handlers; disable automated GH merges for sync/backmerge; change Changes
Sequence Diagram(s)sequenceDiagram
actor Script as ReleaseScript
participant Git as Git (local/origin)
participant GH as GitHub API
participant PR as PR Service
Script->>Git: inspect branches, tags, ancestry (is-ancestor/merge-base)
Git-->>Script: commit lists, ancestry result
Script->>GH: query open PRs, tags
GH-->>Script: PRs, tags
Script->>Script: deriveReleaseState(releaseIsAheadOfMain, privateContentMatchesMain, mainIsAncestorOfDevelop)
alt requires backmerge/private-sync check
Script->>Git: git cherry origin/main...origin/private
Git-->>Script: cherry output
Script->>Script: parseGitCherryOutput(output) -> {safe, unaccounted}
alt safe
Script->>Git: create chore/sync-private-<v>-resolve (branch, merge --strategy=ours)
Git-->>Script: push branch
Script->>PR: createResolvedPrivateSyncPr / createResolvedBackmergePr
PR->>GH: create PR
GH-->>PR: PR created
else unsafe
Script->>PR: create manual PR for backmerge/private-sync
PR->>GH: create PR
GH-->>PR: PR created
end
end
Script->>GH: check PR mergeability/approval
GH-->>Script: mergeable?
alt mergeable & auto-merge enabled
Script->>GH: gh pr merge (merge/squash) [sync/backmerge auto-merge currently disabled]
GH-->>Script: merged
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/release.ts (1)
391-402:⚠️ Potential issue | 🔴 CriticalFix
findOpenPrto normalize GitHub CLI'snulloutput asundefined.When
gh pr listreturns no results, the--jq '.[0]'filter outputs the literal string"null". The current code checksif (!trimmed)which skips empty strings but allows"null"to pass through. Parsing"null"yields JavaScript'snullvalue (notundefined), and the downstream checkopenBackmergePr !== undefinedat line 99 treatsnullas a valid PR, incorrectly routing the release state to'tagged_private_stale'even when no backmerge PR exists.Suggested fix
const stdout = typeof result === 'string' ? result : (result as { stdout: string }).stdout const trimmed = stdout.trim() - if (!trimmed) return undefined + if (!trimmed || trimmed === 'null') return undefined try { - return JSON.parse(trimmed) as GitHubPr + const parsed = JSON.parse(trimmed) as GitHubPr | null + return parsed ?? undefined } catch { return undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 391 - 402, The findOpenPr function currently treats the literal string "null" as valid because trimmed is truthy; update findOpenPr so that when the gh CLI returns "null" it normalizes to undefined—either by checking if trimmed === 'null' and returning undefined before JSON.parse, or by parsing and returning undefined when the parsed value is null; ensure the function (findOpenPr) always returns undefined for no-result cases so downstream checks like openBackmergePr !== undefined behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/release.ts`:
- Around line 574-583: The current doTagAndCreatePostReleasePrs(tags HEAD after
pulling main) can tag the wrong commit; change its signature to accept a stable
commit SHA (e.g., commitSha: string) and require callers to pass the fetched
origin/main SHA captured right after the merge/fetch step. Inside
doTagAndCreatePostReleasePrs, avoid relying on the current checkout HEAD for the
tag: still checkout/pull if needed, but create the annotated tag pointing at the
provided commit SHA (use git tag -a <tag> <commitSha> -m <msg>), push the tags,
then call createPostReleasePrs(nextVersion) as before. Ensure all callers that
previously invoked doTagAndCreatePostReleasePrs(nextVersion) are updated to
supply the captured origin/main SHA.
- Around line 405-423: The getPrMergeability function currently only queries
reviewDecision and mergeable; update the gh pr view invocation inside
getPrMergeability to also request mergeStateStatus, parse mergeStateStatus from
the JSON, and change the returned mergeable boolean to require both
data.mergeable === 'MERGEABLE' and data.mergeStateStatus being 'CLEAN' or
'HAS_HOOKS' (treat null/undefined as non-clean). Keep the existing approved
logic (reviewDecision === 'APPROVED' || reviewDecision === null) and return url
unchanged.
---
Outside diff comments:
In `@scripts/release.ts`:
- Around line 391-402: The findOpenPr function currently treats the literal
string "null" as valid because trimmed is truthy; update findOpenPr so that when
the gh CLI returns "null" it normalizes to undefined—either by checking if
trimmed === 'null' and returning undefined before JSON.parse, or by parsing and
returning undefined when the parsed value is null; ensure the function
(findOpenPr) always returns undefined for no-result cases so downstream checks
like openBackmergePr !== undefined behave correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1289ed5-6b45-45e5-af85-7ffb6687159f
📒 Files selected for processing (2)
scripts/release.test.tsscripts/release.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/utils.ts (1)
7-10: Exit code is always 0, even for error scenarios.The
exitfunction uses exit code 0 (success) unconditionally, but callers use it for error conditions (e.g., Line 15 with invalid semver tag, and inrelease.tsfor various failures). This makes it impossible for CI/scripts to distinguish successful exits from failures.♻️ Suggested fix to support error exit codes
-export const exit = (reason?: string): never => { - if (reason) console.log(reason) - process.exit(0) +export const exit = (reason?: string, code = 0): never => { + if (reason) console.log(reason) + process.exit(code) }Then callers can pass
exit(chalk.red('error message'), 1)for error conditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/utils.ts` around lines 7 - 10, The exit helper always calls process.exit(0) so failures are reported as success; change the exit signature (exit) to accept an optional numeric exit code (e.g., exit(reason?: string, code?: number): never) and call process.exit(code ?? 0) so callers can set non-zero codes for errors; update all call sites that represent failures (e.g., the invalid semver tag usage and the various failure calls from release.ts) to pass a non-zero code (typically 1) when reporting errors and keep success paths using the default or code 0.scripts/release.ts (2)
479-483: Potential command injection if drift filenames contain special characters.Line 481 constructs a shell command by interpolating filenames wrapped in double quotes. If any filename contains
",$, or backticks, this could break the command or cause unintended execution.While drift files come from
git diffoutput (not direct user input), defensive escaping is safer:♻️ Safer approach using execFile with array args
if (drift.length > 0) { console.log(chalk.yellow(`Removing ${drift.length} private drift file(s) not in main...`)) - await pify(exec)(`git rm -f ${drift.map(f => `"${f}"`).join(' ')}`) + await pify(execFile)('git', ['rm', '-f', '--', ...drift]) await pify(exec)(`git commit -m "chore: remove private drift files not in main"`) }Using
execFilewith an array avoids shell interpretation entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 479 - 483, The current code builds shell commands by interpolating drift filenames into pify(exec)(`git rm -f ${...}`) and pify(exec)(`git commit -m "chore: remove private drift files not in main"`), which can be vulnerable if filenames contain special chars; replace the exec-based string interpolation with a shell-avoiding call such as pify(execFile) or child_process.spawn that takes args array: call git with args ["rm", "-f", ...drift] to remove files and then call git with args ["commit", "-m", "chore: remove private drift files not in main"] (or use an API that accepts arg arrays) so filenames are passed as literal args and not interpreted by the shell, keeping the rest of the logic (drift.length check and messages) the same.
399-448: Function callsexit()but declares return typePromise<string | null>.Lines 418, 431, and 441 call
exit()which has return typeneverand terminates the process. This makes thePromise<string | null>return type misleading — the function can either return a string/null or never return at all. While TypeScript handlesnevercorrectly in control flow, the signature doesn't communicate this to readers.Consider either:
- Changing the return type to
Promise<string | null | never>(thoughneveris absorbed in unions)- Throwing an error instead of calling
exit()and letting the caller decide how to handle cancellation- Documenting in a comment that the function may exit the process
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 399 - 448, The function createResolvedBackmergePr currently calls exit() which terminates the process and contradicts the declared Promise<string | null> return type; replace those exit() calls with throwing a clear cancellaton error (e.g., throw new Error('Sync cancelled') or a custom CancelledError) so the function either resolves to string|null or throws and callers can handle it, and ensure all three places that currently do return exit('Sync cancelled.') (the branches after inquireConfirm checks) are updated accordingly; alternatively, if you prefer process termination, add a short comment on createResolvedBackmergePr documenting that it may call process.exit and change the signature to reflect that behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/release.ts`:
- Around line 705-715: handleNeedsTag currently checks out and pulls main then
tags HEAD, which can race if main advances; change handleNeedsTag to accept an
explicit tagSha parameter (e.g., handleNeedsTag(nextVersion: string, tagSha:
string)), remove the checkout/pull/tag-HEAD sequence and instead create the
annotated tag for the exact SHA using git tag -a <nextVersion> <tagSha> -m
<nextVersion> and then push the tags, then call handleSyncPending(nextVersion);
update callers to capture origin/main SHA immediately after confirming the merge
and pass that SHA into handleNeedsTag.
---
Nitpick comments:
In `@scripts/release.ts`:
- Around line 479-483: The current code builds shell commands by interpolating
drift filenames into pify(exec)(`git rm -f ${...}`) and pify(exec)(`git commit
-m "chore: remove private drift files not in main"`), which can be vulnerable if
filenames contain special chars; replace the exec-based string interpolation
with a shell-avoiding call such as pify(execFile) or child_process.spawn that
takes args array: call git with args ["rm", "-f", ...drift] to remove files and
then call git with args ["commit", "-m", "chore: remove private drift files not
in main"] (or use an API that accepts arg arrays) so filenames are passed as
literal args and not interpreted by the shell, keeping the rest of the logic
(drift.length check and messages) the same.
- Around line 399-448: The function createResolvedBackmergePr currently calls
exit() which terminates the process and contradicts the declared Promise<string
| null> return type; replace those exit() calls with throwing a clear
cancellaton error (e.g., throw new Error('Sync cancelled') or a custom
CancelledError) so the function either resolves to string|null or throws and
callers can handle it, and ensure all three places that currently do return
exit('Sync cancelled.') (the branches after inquireConfirm checks) are updated
accordingly; alternatively, if you prefer process termination, add a short
comment on createResolvedBackmergePr documenting that it may call process.exit
and change the signature to reflect that behavior.
In `@scripts/utils.ts`:
- Around line 7-10: The exit helper always calls process.exit(0) so failures are
reported as success; change the exit signature (exit) to accept an optional
numeric exit code (e.g., exit(reason?: string, code?: number): never) and call
process.exit(code ?? 0) so callers can set non-zero codes for errors; update all
call sites that represent failures (e.g., the invalid semver tag usage and the
various failure calls from release.ts) to pass a non-zero code (typically 1)
when reporting errors and keep success paths using the default or code 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 557eff73-c4eb-4a94-9be6-5392d67085d3
📒 Files selected for processing (3)
scripts/release.test.tsscripts/release.tsscripts/utils.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/release.ts (2)
642-649:⚠️ Potential issue | 🟡 MinorRace condition: tag may land on wrong commit if main advances during execution.
The checkout/pull/tag-HEAD pattern is vulnerable to a race: if another PR merges to main between the release PR merge and running this script, the tag will be placed on that unrelated commit instead of the release commit.
Capture
origin/mainSHA immediately afterfetchOrigin()completes (before any interactive prompts) and tag that specific SHA:🐛 Suggested fix
case 'merged_untagged': { console.log(chalk.green(`Release merged to main. Tagging ${nextVersion}...`)) - await git().checkout(['main']) - await git().pull() - await git().tag(['-a', nextVersion, '-m', nextVersion]) + const mainShaToTag = await getSha('origin/main') + await git().tag(['-a', nextVersion, '-m', nextVersion, mainShaToTag]) console.log(chalk.green('Pushing tag...'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 642 - 649, The current checkout/pull/tag sequence in the merged_untagged branch risks tagging whatever HEAD is at tag time; after fetchOrigin() completes, immediately capture the origin/main commit SHA (e.g., by calling git().revparse(['origin/main']) or equivalent) and then use that SHA when creating the tag (pass the SHA to git().tag instead of relying on the checked-out HEAD), leaving the rest of the flow (checkout main, push tags) intact; update the code paths around git().checkout, git().tag and where fetchOrigin() is called to read and use the saved origin/main SHA for the annotated tag.
855-862:⚠️ Potential issue | 🟡 MinorSame tagging race condition as regular release.
Apply the same fix here: tag a specific SHA instead of HEAD after pull.
🐛 Suggested fix
case 'merged_untagged': { console.log(chalk.green(`Hotfix merged to main. Tagging ${nextVersion}...`)) - await git().checkout(['main']) - await git().pull() - await git().tag(['-a', nextVersion, '-m', nextVersion]) + const mainShaToTag = await getSha('origin/main') + await git().tag(['-a', nextVersion, '-m', nextVersion, mainShaToTag]) console.log(chalk.green('Pushing tag...'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 855 - 862, In the 'merged_untagged' case the script tags HEAD which can race after the pull; instead capture the resolved commit SHA and tag that SHA explicitly. After calling git().checkout(['main']) and await git().pull(), run git().revparse(['HEAD']) (or git().raw(['rev-parse', 'HEAD'])) to get the commit SHA, then call git().tag(['-a', nextVersion, '-m', nextVersion, <sha>]) and finally push tags with git().push(['origin', '--tags']) so the tag references a specific commit rather than a moving HEAD.
🧹 Nitpick comments (1)
scripts/release.ts (1)
398-411: Push may fail on re-runs if branch already exists on remote.The push on line 404 will fail if
chore/sync-private-${version}-resolvealready exists on remote with divergent history (e.g., from a previous partial run). Since this helper is meant to support idempotent re-runs, consider using--force-with-leaseto allow overwriting the remote branch safely:Proposed fix
await git().checkout(['-B', resolveBranch, 'origin/main']) await pify(exec)(`git merge --strategy=ours --no-edit origin/private`) - await git().push(['-u', 'origin', resolveBranch]) + await git().push(['-u', '--force-with-lease', 'origin', resolveBranch])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 398 - 411, The push in createPrivateSyncBranch can fail if the remote branch chore/sync-private-{version}-resolve already exists; update the push invocation in createPrivateSyncBranch so it uses a safe force option (e.g., --force-with-lease) when pushing the local resolveBranch to origin (preserving the -u behavior) to allow idempotent re-runs while avoiding blind force pushes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/release.ts`:
- Around line 642-649: The current checkout/pull/tag sequence in the
merged_untagged branch risks tagging whatever HEAD is at tag time; after
fetchOrigin() completes, immediately capture the origin/main commit SHA (e.g.,
by calling git().revparse(['origin/main']) or equivalent) and then use that SHA
when creating the tag (pass the SHA to git().tag instead of relying on the
checked-out HEAD), leaving the rest of the flow (checkout main, push tags)
intact; update the code paths around git().checkout, git().tag and where
fetchOrigin() is called to read and use the saved origin/main SHA for the
annotated tag.
- Around line 855-862: In the 'merged_untagged' case the script tags HEAD which
can race after the pull; instead capture the resolved commit SHA and tag that
SHA explicitly. After calling git().checkout(['main']) and await git().pull(),
run git().revparse(['HEAD']) (or git().raw(['rev-parse', 'HEAD'])) to get the
commit SHA, then call git().tag(['-a', nextVersion, '-m', nextVersion, <sha>])
and finally push tags with git().push(['origin', '--tags']) so the tag
references a specific commit rather than a moving HEAD.
---
Nitpick comments:
In `@scripts/release.ts`:
- Around line 398-411: The push in createPrivateSyncBranch can fail if the
remote branch chore/sync-private-{version}-resolve already exists; update the
push invocation in createPrivateSyncBranch so it uses a safe force option (e.g.,
--force-with-lease) when pushing the local resolveBranch to origin (preserving
the -u behavior) to allow idempotent re-runs while avoiding blind force pushes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/release.ts (1)
704-711:⚠️ Potential issue | 🔴 CriticalTag the merged SHA explicitly.
This still tags
HEADafter a freshpull. If another PR lands onmainbetween the release/hotfix merge and this step, the semver tag moves onto that unrelated commit. Capture the targetorigin/mainSHA right after the merge/fetch step and pass it intohandleNeedsTag().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 704 - 711, handleNeedsTag currently tags whatever HEAD points to after a pull, risking tagging a different commit; change handleNeedsTag(nextVersion: string) to accept the target SHA (e.g., handleNeedsTag(nextVersion: string, targetSha: string)), capture the SHA immediately after syncing with remote using a rev-parse of origin/main (call git().revparse(['origin/main']) or equivalent) and pass that SHA into handleNeedsTag from the caller, then create the annotated tag against that SHA (use the SHA as the final argument to git().tag) instead of tagging HEAD; keep the subsequent call to handleSyncPending(nextVersion) as-is or update it only if it needs the SHA too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/release.ts`:
- Around line 746-755: handlePrereleaseOpen currently merges the PR then calls
handlePrereleaseMerged while origin/release may still point to stale refs;
before calling handlePrereleaseMerged you should refresh the remote release ref
so the subsequent origin/main..origin/release inspection sees the new squash
commit. Add a fetch of the release branch (e.g., run a git fetch for origin
release using the same exec helper, e.g., via pify(exec)) in
handlePrereleaseOpen right after the merge and before calling
handlePrereleaseMerged to ensure origin/release is up-to-date.
- Around line 656-676: The message shown when an existing backmerge PR is found
(in the block using findOpenBackmergePr and checkMainSafeToAutoBackmerge) should
explicitly require a merge-commit-only merge (not a squash) because
mainIsAncestorOfDevelop only clears with a merge commit; update both console.log
branches (the “Resolve conflicts manually before merging.” and the “Auto-merge
temporarily disabled — merge it on GitHub.” messages) to state “Merge using a
merge commit (disable squash/rebase)” or similar, and optionally instruct to use
the GitHub UI option to disable squash (or run gh pr merge --merge) so callers
know to avoid squash merges. Ensure references to existingBackmergePr.number
remain in the messages.
- Around line 67-93: The check using releaseIsAheadOfMain is not squash-safe;
update deriveReleaseState to accept and use a content-equivalence boolean (e.g.,
releaseContentMatchesMain or releaseIsSquashEquivalent) instead of (or in
addition to) releaseIsAheadOfMain and change the logic so you detect whether the
release branch is functionally identical to main by content, not SHA/history;
specifically, modify the function signature to add the new boolean, replace the
releaseIsAheadOfMain-based branch that returns 'prerelease_merged' to rely on
the content-equivalence flag (and keep releaseIsAheadOfMain only if you still
need history-based semantics), and apply the same change in deriveHotfixState so
squash-merged branches don’t cause recreation of already-shipped PRs.
---
Duplicate comments:
In `@scripts/release.ts`:
- Around line 704-711: handleNeedsTag currently tags whatever HEAD points to
after a pull, risking tagging a different commit; change
handleNeedsTag(nextVersion: string) to accept the target SHA (e.g.,
handleNeedsTag(nextVersion: string, targetSha: string)), capture the SHA
immediately after syncing with remote using a rev-parse of origin/main (call
git().revparse(['origin/main']) or equivalent) and pass that SHA into
handleNeedsTag from the caller, then create the annotated tag against that SHA
(use the SHA as the final argument to git().tag) instead of tagging HEAD; keep
the subsequent call to handleSyncPending(nextVersion) as-is or update it only if
it needs the SHA too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 725186ea-21a3-4835-8466-21523313b8c2
📒 Files selected for processing (2)
scripts/release.test.tsscripts/release.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/release.test.ts
kaladinlight
force-pushed
the
fix/release-script-validation
branch
from
af39dee to
0632f6a
Compare
kaladinlight
changed the title
kaladinlight
force-pushed
the
fix/release-script-validation
branch
from
0632f6a to
47a67a5
Compare
The release script had grown a lot of bookkeeping — develop-side tags, merge marker scanning, cherry-pick trailer parsing — all of it existed solely to compensate for the fact that release PRs landed on main as squash commits, which destroys the link between develop's individual commits and main's history. This switches to merging release PRs as true merge commits. Once develop SHAs are reachable from main as ancestors, "what's unreleased?" reduces to a single git command: git log --cherry-pick --right-only --no-merges main...develop Patch-id matching naturally handles hotfixes (same patch, different SHA on main). Merge-base advances on every release. No tags, no markers, no trailer parsing — the state machine self-heals. Net: ~190 lines smaller, much easier to reason about.
kaladinlight
force-pushed
the
fix/release-script-validation
branch
from
47a67a5 to
cb28757
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Simplifies the release script so it just works with merge-commit-based release PRs. The previous flow had grown a lot of clever bookkeeping to compensate for squash merges destroying develop SHAs on main; switching to merge commits removes the need for all of it.
Why
The old flow had a bunch of moving parts that were hard to reason about:
develop-v*tags as anchors for "what was last released"(cherry picked from commit X)trailer parsing to filter out hotfixesAll of that existed for one reason: squash merges to main destroy the link between develop's individual commits and main's history. There's no way to ask git "what's already shipped?" because the squash is opaque.
Switching to merge commits for the release PR fixes this at the root. Develop SHAs become reachable from main as ancestors, and
git log --cherry-pickpatch-id matching just works for both regular releases and hotfixes.What's actually different
getUnreleasedCommitsOne git command:
Returns develop commits whose patches aren't on main. Hotfixes (cherry-picked onto main with different SHAs) are filtered automatically by patch-id matching.
handleReleaseReadynotesSame pattern, against
main...release— so release notes never include commits already shipped by an earlier hotfix.State machine
releaseIsAheadOfMainnow uses commit count instead of tree diff. Previously, after any hotfix, the script would deadlock inrelease_readyforever because main and release trees would differ in the wrong direction.handleNeedsTagjust tags main and pushes the specific tag — no more develop-tag dance, no more--tagsblast.handleHotfixIdlecherry-picks once (onto the hotfix branch only) and reverses to chronological order so commits with ordering dependencies apply correctly. Pre-cleans any leftover local hotfix branch from a prior failed run so retries are idempotent.Removed
develop-v*tag trackingRELEASE_MERGE_MESSAGE_PATTERN,getLatestReleaseMergeSha,findReleaseMergeBySecondParent)parseCherryPickTrailers,getCherryPickedShasSince)assertIsCleanRepo(dead code; the script never touches the working tree)_nextVersionparameters onhandleReleaseOpen/handleHotfixOpenNet: ~190 lines smaller.
The squash → merge transition
v1.1020.0was squash-merged the old way. Switching to merge-commit detection mid-flight doesn't work cleanly because the squash commit on main has no patch-id link to develop's individuals.The transition was bridged manually with a one-time bootstrap: a merge commit on main containing develop's content (parent1 = v1.1020.0 squash, parent2 = develop tip), tagged as
v1.1020.1, withreleaseandprivatebrought in line. After that, every future release runs through the script automatically — confirmed by dry-runs of both regular and hotfix flows against the current repo state.What everyone needs to know going forward
Merge release PRs with "Create a merge commit", not "Squash and merge". This is the one human responsibility that keeps the script working. Worth changing the GitHub repo's PR merge default if it's currently set to squash, so this can never accidentally regress.
Testing
pnpm vitest run scripts/release.test.ts— 24 tests passhandleReleaseReadynotes contain exactly the new commitgetUnreleasedCommitsreturns empty, merge-base advances to develop tipgetUnreleasedCommitsreturns empty (patch-id filter caught the develop original),releaseIsAheadOfMaincorrectly reports 0 → state derives toidle, NOTrelease_ready(no deadlock)v1.1020.1bootstrap executed end-to-end on the live repo: main, release, private, tag all in expected post-state