fix: use content diff for private sync state detection in release script#12181
Merged
gomesalexandre merged 8 commits intodevelopfrom Mar 17, 2026
Merged
fix: use content diff for private sync state detection in release script#12181gomesalexandre merged 8 commits intodevelopfrom
gomesalexandre merged 8 commits intodevelopfrom
Conversation
The native QR scanner handler ships in mobile app v3.7.2 (mobile-app PR #156), but the version gate was set to 3.4.0. This caused mobile apps v3.4.0–3.7.1 to incorrectly attempt native scanning, resulting in a 60-second spinner timeout with no way to scan. Co-authored-by: Claude Opus 4.6 <[email protected]>
Contributor
📝 WalkthroughWalkthroughThe release script replaces SHA-based private/main comparisons with a boolean Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Release Script
participant Git as Git (origin/private, main)
participant GH as GitHub API
participant State as State Deriver
Script->>Git: fetch SHAs & diff origin/private vs main
Git-->>Script: returns privateContentMatchesMain (boolean) and SHAs
Script->>State: deriveReleaseState(privateContentMatchesMain, SHAs, openPRs...)
State-->>Script: returns ReleaseState
Script->>GH: createPr/backmerge if needed
GH-->>Script: returns PR URL or error ("No commits between") -> null
Script->>Script: conditional logging / exit based on PR URL and ReleaseState
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
SHA comparison for private branch breaks with squash merges - private's SHA will never equal main's SHA even when content is identical, leaving the script permanently stuck in tagged_private_stale and unable to reach idle/done. Fixes the "can't cut a new release" situation. changes: - deriveReleaseState/deriveHotfixState: replace privateSha param with privateContentMatchesMain boolean (git diff based, squash-safe) - doRegularRelease/doHotfixRelease: compute content diff before state derivation - tagged_private_stale log message: use latestTag (not nextVersion) - createPr: return null on "No commits between" instead of crashing Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
gomesalexandre
force-pushed
the
fix/release-script-no-commits-crash
branch
from
ef505bb to
acb8d81
Compare
gomesalexandre
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
scripts/release.ts (3)
548-554:⚠️ Potential issue | 🟡 MinorInconsistent null handling for
createPrreturn value.Since
createPrcan now returnnullwhen there are no commits between branches, this call site should handle that case to avoid printing "Release PR created: null". Other call sites in this file (lines 625-630, 652-656, 790-794, etc.) properly check for null before logging.While unlikely in the
idlestate flow (commits are checked beforehand), the branches could theoretically be in sync due to race conditions or edge cases.Proposed fix
const releasePrUrl = await createPr({ base: 'main', head: 'release', title: `chore: release ${nextVersion}`, body: releaseBody, }) - console.log(chalk.green(`Release PR created: ${releasePrUrl}`)) + if (releasePrUrl) { + console.log(chalk.green(`Release PR created: ${releasePrUrl}`)) + } else { + console.log(chalk.green('release branch already in sync with main - nothing to do.')) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 548 - 554, Handle the case where createPr returns null before logging: after calling createPr (the releasePrUrl variable) check for null and only call console.log(chalk.green(...)) if releasePrUrl is non-null; otherwise skip the success log (or log an appropriate message). Update the block that assigns releasePrUrl from createPr({ base: 'main', head: 'release', title: `chore: release ${nextVersion}`, body: releaseBody }) to perform a null-check and guard the console.log accordingly so the code mirrors other call sites that avoid printing "Release PR created: null".
753-761:⚠️ Potential issue | 🟡 MinorSame inconsistent null handling.
For consistency with other call sites, this should also handle the
nullcase, even though it's less likely to occur in the hotfix flow (commits are explicitly cherry-picked).Proposed fix
const prUrl = await createPr({ base: 'main', head: hotfixBranch, title: `chore: hotfix ${nextVersion}`, body: `## Hotfix ${nextVersion}\n\n${commitList}`, }) - console.log(chalk.green(`Hotfix PR created: ${prUrl}`)) + if (prUrl) { + console.log(chalk.green(`Hotfix PR created: ${prUrl}`)) + } else { + console.log(chalk.green('hotfix branch already in sync with main - nothing to do.')) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 753 - 761, The call to createPr in the hotfix flow does not handle a null return like other call sites; after invoking createPr({ base: 'main', head: hotfixBranch, title: `chore: hotfix ${nextVersion}`, body: ... }), add a null-check for prUrl and handle it the same way as elsewhere (e.g., log an error with context including hotfixBranch/nextVersion and exit with a non-zero status) before proceeding to the success console.log lines so the script fails cleanly when createPr returns null.
571-577:⚠️ Potential issue | 🟡 MinorSame inconsistent null handling.
Similar to the release PR creation above, this should handle the
nullcase for consistency.Proposed fix
const prereleasePrUrl = await createPr({ base: 'release', head: 'develop', title: `chore: prerelease ${nextVersion}`, body: `## Prerelease ${nextVersion}\n\n${commitList}`, }) - console.log(chalk.green(`Prerelease PR created: ${prereleasePrUrl}`)) + if (prereleasePrUrl) { + console.log(chalk.green(`Prerelease PR created: ${prereleasePrUrl}`)) + } else { + console.log(chalk.green('develop already in sync with release - nothing to do.')) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 571 - 577, createPr can return null so the prerelease PR branch must handle that case consistently: after calling createPr for prerelease, check if prereleasePrUrl is null and if so call processLogger.error (or console.error) with a descriptive message and exit non-zero (mirroring the release PR handling); only call console.log(chalk.green(...)) when prereleasePrUrl is non-null. Refer to createPr, prereleasePrUrl, and the console.log(chalk.green(...)) call to locate where to add the null-check and error handling.
🤖 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 548-554: Handle the case where createPr returns null before
logging: after calling createPr (the releasePrUrl variable) check for null and
only call console.log(chalk.green(...)) if releasePrUrl is non-null; otherwise
skip the success log (or log an appropriate message). Update the block that
assigns releasePrUrl from createPr({ base: 'main', head: 'release', title:
`chore: release ${nextVersion}`, body: releaseBody }) to perform a null-check
and guard the console.log accordingly so the code mirrors other call sites that
avoid printing "Release PR created: null".
- Around line 753-761: The call to createPr in the hotfix flow does not handle a
null return like other call sites; after invoking createPr({ base: 'main', head:
hotfixBranch, title: `chore: hotfix ${nextVersion}`, body: ... }), add a
null-check for prUrl and handle it the same way as elsewhere (e.g., log an error
with context including hotfixBranch/nextVersion and exit with a non-zero status)
before proceeding to the success console.log lines so the script fails cleanly
when createPr returns null.
- Around line 571-577: createPr can return null so the prerelease PR branch must
handle that case consistently: after calling createPr for prerelease, check if
prereleasePrUrl is null and if so call processLogger.error (or console.error)
with a descriptive message and exit non-zero (mirroring the release PR
handling); only call console.log(chalk.green(...)) when prereleasePrUrl is
non-null. Refer to createPr, prereleasePrUrl, and the
console.log(chalk.green(...)) call to locate where to add the null-check and
error handling.
Contributor
Author
|
@coderabbitai the title is intentional - the content diff approach is the primary fix (replaces SHA comparison, unblocks new releases after squash-synced private). The no-commits-between null handling is a secondary defensive change bundled in the same PR. Both are captured in the commit message body. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to regenerate the PR summary to better highlight the content-diff approach as the primary fix? ✏️ Learnings added
🧠 Learnings used |
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
Author
|
@coderabbitai no need to regenerate, title and existing summary are fine as-is ser |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/release.ts (3)
553-559: Minor: Missing null check for release PR URL.While this code path is only reached when commits exist (validated at line 532), if
createPrreturnsnullunexpectedly, the log would show "Release PR created: null". The same applies to prerelease PR (line 576) and hotfix PR (line 798).Since the "No commits between" error should only occur when branches are identical (which shouldn't happen here given prior validation), this is very low risk. Consider adding a defensive check for consistency with other call sites.
💡 Optional: Defensive null check
const releasePrUrl = await createPr({ base: 'main', head: 'release', title: `chore: release ${nextVersion}`, body: releaseBody, }) - console.log(chalk.green(`Release PR created: ${releasePrUrl}`)) + if (releasePrUrl) { + console.log(chalk.green(`Release PR created: ${releasePrUrl}`)) + } else { + exit(chalk.red('Failed to create release PR - no commits between branches.')) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 553 - 559, Add a defensive null check after calling createPr so we never log "Release PR created: null": after the createPr call that sets releasePrUrl, verify releasePrUrl is non-null and only call console.log(chalk.green(...)) when it is; if null, log an error or throw (consistent with other call sites). Apply the same pattern for the prerelease and hotfix createPr calls (the variables used there) so all PR creation sites consistently guard against a null return from createPr.
860-861: Minor: Redundant diff computation.
privateContentMatchesMainwas already computed at the start ofdoHotfixRelease(lines 732-734). This additional diff check (privateDiffHotfix) is defensive but redundant—if we're intagged_private_stale, we already knowprivateContentMatchesMainis false. Additionally,createPrnow returnsnullwhen there are no commits between branches, providing another safety net.This isn't a bug, but it's slightly inconsistent with the regular release flow which doesn't have this double-check. Consider removing for consistency, or keep as belt-and-suspenders.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 860 - 861, The extra diff check computing privateDiffHotfix and shouldSyncPrivateHotfix inside doHotfixRelease is redundant because privateContentMatchesMain was already determined earlier and the createPr call now returns null when there are no commits; remove the redundant computation (privateDiffHotfix and shouldSyncPrivateHotfix) and any logic that relies on it, and instead use the previously computed privateContentMatchesMain (and rely on createPr's null result) to decide whether to sync/create the private PR; update any references to shouldSyncPrivateHotfix to use the existing privateContentMatchesMain or the createPr return value, and remove the unnecessary git().diff call.
427-429: Defensive error handling looks correct.The
String(err).includes('No commits between')check handles the GitHub API response gracefully. However, consider using a more specific check if GitHub provides a structured error code, as string matching can be fragile if GitHub changes their error message wording.💡 Optional: More robust error detection
If the error object has a structured format (e.g.,
err.stderrorerr.code), matching on that would be more reliable than string matching on the full error. However, since this is a CLI tool and the current approach works, this is a low-priority refinement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 427 - 429, The catch block that currently does if (String(err).includes('No commits between')) return null should be made more robust by checking structured error properties first (e.g., err.code, err.message, err.stderr) and only falling back to the string include if those properties are absent; update the catch handling around that try/catch to test err.code or err.message for a known GitHub/CLI code or phrase, then return null when matched, otherwise rethrow the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/release.ts`:
- Around line 553-559: Add a defensive null check after calling createPr so we
never log "Release PR created: null": after the createPr call that sets
releasePrUrl, verify releasePrUrl is non-null and only call
console.log(chalk.green(...)) when it is; if null, log an error or throw
(consistent with other call sites). Apply the same pattern for the prerelease
and hotfix createPr calls (the variables used there) so all PR creation sites
consistently guard against a null return from createPr.
- Around line 860-861: The extra diff check computing privateDiffHotfix and
shouldSyncPrivateHotfix inside doHotfixRelease is redundant because
privateContentMatchesMain was already determined earlier and the createPr call
now returns null when there are no commits; remove the redundant computation
(privateDiffHotfix and shouldSyncPrivateHotfix) and any logic that relies on it,
and instead use the previously computed privateContentMatchesMain (and rely on
createPr's null result) to decide whether to sync/create the private PR; update
any references to shouldSyncPrivateHotfix to use the existing
privateContentMatchesMain or the createPr return value, and remove the
unnecessary git().diff call.
- Around line 427-429: The catch block that currently does if
(String(err).includes('No commits between')) return null should be made more
robust by checking structured error properties first (e.g., err.code,
err.message, err.stderr) and only falling back to the string include if those
properties are absent; update the catch handling around that try/catch to test
err.code or err.message for a known GitHub/CLI code or phrase, then return null
when matched, otherwise rethrow the error.
0xApotheosis
approved these changes
Mar 17, 2026
Member
0xApotheosis
left a comment
0xApotheosis
left a comment
There was a problem hiding this comment.
PR Review
Root cause fix is correct. Replacing privateSha !== mainSha with a git diff-based boolean is squash-merge safe — the right approach.
What's good
createPrreturningnullon "No commits between" is a nice defensive improvement- All callers handle
nullcorrectly - Log message fix (
latestTaginstead ofnextVersionintagged_private_stale) is correct
Minor issues
1. Inconsistency in hotfix tagged_private_stale handler
The regular release tagged_private_stale handler was cleanly refactored — removed the redundant git diff check and relies on createPr returning null. But the hotfix tagged_private_stale handler (~line 846) still uses the old pattern:
const privateDiffHotfix = await git().diff(['origin/main', 'origin/private'])
const shouldSyncPrivateHotfix = Boolean(privateDiffHotfix)This is redundant because tagged_private_stale already means !privateContentMatchesMain. Could be simplified to match the regular release path. Not a bug, just inconsistent and a wasted diff call.
2. Nit: String(err).includes('No commits between') is a bit fragile if GH CLI ever changes that message, but the fallback is just throw err so low risk.
LGTM 👍
…tale - add exit() on null releasePrUrl/prereleasePrUrl/prUrl (hotfix) - these are unexpected nulls since commits are validated beforehand, so fail loud - remove privateDiffHotfix/shouldSyncPrivateHotfix from hotfix tagged_private_stale - redundant since that state only fires when !privateContentMatchesMain. matches regular release path now. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
Author
|
done @0xApotheosis - removed the redundant |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
68-84:⚠️ Potential issue | 🟠 MajorUpdate test fixtures/callers to the renamed state input (
privateContentMatchesMain).
deriveReleaseStateandderiveHotfixStatenow requireprivateContentMatchesMain: boolean, but related tests still appear to passprivateSha: string. This will break type-checking (or invalidate test intent) until those fixtures are migrated.Suggested test updates
- privateSha: 'aaa', + privateContentMatchesMain: true, - privateSha: 'zzz', + privateContentMatchesMain: false,Also applies to: 98-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release.ts` around lines 68 - 84, The tests and any callers still pass the old privateSha string to deriveReleaseState/deriveHotfixState; update those fixtures and call sites to provide the renamed boolean parameter privateContentMatchesMain (true/false) instead of privateSha, and adjust any test assertions/fixtures to reflect the boolean semantics; search for deriveReleaseState and deriveHotfixState usages and replace the former privateSha argument with a computed boolean (e.g., privateContentMatchesMain) so TypeScript types and test intent match the new signature.
🤖 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 664-669: The PR metadata for the "tagged_private_stale" PRs uses
nextVersion but should reference the already-tagged release latestTag; update
the createPr calls that build the private branch PR (the call that assigns
privatePrUrl and the analogous call around lines 874-879) to use latestTag in
the title and body instead of nextVersion so the title/body read e.g. `chore:
sync private to ${latestTag}` and `Sync private branch to main after release
${latestTag}`; keep the same createPr invocation and variables (createPr,
privatePrUrl) but replace nextVersion with latestTag.
---
Outside diff comments:
In `@scripts/release.ts`:
- Around line 68-84: The tests and any callers still pass the old privateSha
string to deriveReleaseState/deriveHotfixState; update those fixtures and call
sites to provide the renamed boolean parameter privateContentMatchesMain
(true/false) instead of privateSha, and adjust any test assertions/fixtures to
reflect the boolean semantics; search for deriveReleaseState and
deriveHotfixState usages and replace the former privateSha argument with a
computed boolean (e.g., privateContentMatchesMain) so TypeScript types and test
intent match the new signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
This was referenced Mar 17, 2026
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
2 participants
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.
SHA comparison for
privatebranch breaks with squash merges - private's SHA will never equal main's SHA even when content is identical, so the script gets permanently stuck intagged_private_staleand can never reachidle. New release = borked.changes
deriveReleaseState/deriveHotfixState: replaceprivateShaparam withprivateContentMatchesMainboolean (computed viagit diff, squash-safe)doRegularRelease/doHotfixRelease: compute content diff before state derivationtagged_private_stalelog message: uselatestTaginstead ofnextVersion(was showing wrong version)createPr: returnnullon "No commits between" instead of crashing, callers handle gracefullyrisk
Low - release script only, no prod code.
testing
Run
GITHUB_TOKEN= pnpm releaseafter a release where private was synced via squash - should showidle(ready for next release) instead oftagged_private_stale.Summary by CodeRabbit
Bug Fixes
Refactor