Repository navigation
[NoQA] Fix three ways a version sync can fail or push a bad version to main - #97599
Conversation
|
@codex @MelvinBot review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Review — LGTM ✅The fix is correct and minimal. I verified:
One non-blocking observation (pre-existing, out of scope)The full-sync push-failure/rebase path ( No changes requested — this is good to merge (after the stacked base #97598, per the PR note). |
Only the App side is rewritten, so a target that isn't the Mobile-Expensify version can never pass verification. It used to be caught after the version commits were already pushed to main.
Verification refetches Mobile-Expensify main, which can move while the sync runs, so a sync that recorded exactly what the check step pinned it to was reported as a failure.
# Conflicts: # tests/tooling/SyncVersions.test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. |
# Conflicts: # tests/tooling/SyncVersions.test.ts
# Conflicts: # .github/scripts/syncVersions.sh
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69aa94de68
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Mobile-Expensify main can advance mid-run, which is likely because a deployer triggers this | ||
| # during a deploy. Recording what `check` pinned is still a successful sync, so report it and | ||
| # let the next run collect the newer commit. | ||
| if [[ "$recorded" == "$EXPECTED_SUBMODULE_SHA" ]]; then |
There was a problem hiding this comment.
Do not claim the stale gitlink matches current main
When Mobile-Expensify advances after check, this branch deliberately returns success while recorded is still the older SHA. The workflow then runs its existing “Announce sync in Slack” step, whose success message says that the submodule matches Mobile-Expensify main, even though this branch just established that it does not. Deployer notifications will therefore report a completed, fully synchronized version when a rerun is still required; expose this stale-but-accepted state to the workflow or change the success announcement accordingly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Vit's agent)
Good catch, and correct — fixed in 7a6f54c8d62. verify_sync now sets a SUBMODULE_MATCHES_REMOTE output, and the announce step's text branches on it, so the stale-but-accepted case says "Mobile-Expensify main has advanced since, so run this again to record it" instead of claiming a match. Both directions are asserted in the suite.
Letting a mid-run Mobile-Expensify advance pass verification left the announce step still saying the submodule matches main. verify_sync now reports which of the two it was.
|
Minor comment |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@madmax330 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@ZhenjaHorbach sorry this did not require C+ review, its just a workflow change |
Don't worry |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.5.0-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.0-5 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Note
Extract syncVersions workflow shell into a script, which this was stacked on, has merged. This now targets
maindirectly and its diff is only the hunks described below.The full version sync path in
.github/scripts/syncVersions.shrecords the Mobile-Expensify submodule pointer unconditionally:git add Mobile-Expensify git commit -m "Update Mobile-Expensify submodule version to $target (sync recovery)"When the versions differ but the App gitlink already points at Mobile-Expensify
main, nothing is staged, sogit commitexits 1. Underset -ethat kills the job after the local version commit and before any push — so the sync silently accomplishes nothing and has to be re-run.That state is reachable in practice: it is exactly what a previously half-completed sync leaves behind, where the submodule pointer got bumped but the versions did not.
The fix applies the same guard the submodule-only path already uses:
This was deliberately left out of #97598 so that PR stayed a provably behavior-neutral refactor.
Two more failure modes in the same script are fixed here, for the same reason — they are behavior changes, so they do not belong in the refactor.
A
TARGET_VERSIONthat isn't the Mobile-Expensify version reachedmainbefore failing. Only the App side is rewritten, so any other target fails the final verification. It did so aftersync_full_versionhad committed and pushed that version to Appmain, leavingmainclaiming a version Mobile-Expensify never had.resolve_target_versionnow rejects it before anything is written.Mobile-Expensify moving mid-run was reported as a failed sync. Verification refetches Mobile-Expensify
mainand compares it to the gitlink App now records. A deployer triggers this during a deploy, so Mobile-Expensifymaincan easily gain a commit while the sync runs — and a sync that recorded exactly whatcheckpinned it to was then announced as a failure in#deployer. That one case is now a::warning::telling you to re-run; a gitlink this run never pinned is still a hard::error::.Accepting that case means the run reaches the
Announce sync in Slackstep, whose text claimed the submodule matches Mobile-Expensifymain— which is exactly what this branch just established is not true.verify_synctherefore reports aSUBMODULE_MATCHES_REMOTEoutput and the announcement branches on it, so#deployeris told to run it again rather than that everything is aligned. That is the one hunk in.github/workflows/syncVersions.yml.Fixed Issues
$ #88233
PROPOSAL: N/A
Tests
Check out this branch and run the suite on macOS. It lives in
tests/tooling/and runs underbun test, not Jest:TZ=utc bun test --parallel --preload ./scripts/stubReactNative.js --preload ./tests/tooling/setup.ts ./tests/tooling/SyncVersions.test.tsVerify all 19 tests pass. Four of them cover the three fixes:
still syncs when the submodule pointer is already currentrejects a target version that does not match Mobile-Expensify, without touching App main— a rewrite offails verification when the target version does not match Mobile-Expensify, which passed for the wrong reason: it asserted the failure that happened after the bad version was pushedwarns instead of failing when Mobile-Expensify main advances mid-syncfails verification when App main records a submodule commit this run never pinned— the other side of that branch, so downgrading it to a warning cannot swallow a real mismatchVerify they are genuine regression tests. The fixes are committed, so put
main's script back rather than stashing:Re-run the command above and confirm exactly those four fail and the other 15 pass, then restore with
git checkout HEAD -- .github/scripts/syncVersions.shand confirm all 19 pass again.Verify each failure in its unfixed state:
nothing to commit, working tree cleanand a non-zero exit, withorigin/mainleft without the version commit. Fixed,origin/mainends onUpdate version to 9.3.11-48 (sync recovery)andPOST_SYNC_APP_VERSIONis set.TARGET_VERSION:origin/mainreceivesUpdate version to 9.9.9-9 (sync recovery)before verification fails. Fixed,origin/mainis untouched and the error names both versions.::error::Submodule on App main ... still differs. Fixed, it exits 0 with::warning::Mobile-Expensify main advanced to .... A gitlink this run never pinned still exits 1 on that same::error::.Run
npm run shellcheckand verify it passes.Read the
Announce sync in Slackpayload in.github/workflows/syncVersions.ymland confirm the success text sayssubmodule matches Mobile-Expensify main.only whenSUBMODULE_MATCHES_REMOTEis'true', and otherwise tells the deployer to run it again. The suite asserts that output is'true'after a clean submodule-only sync and'false'after one where Mobile-Expensify moved mid-run.Deployer validation: trigger the
Sync E/App and Mobile-Expensify versionsworkflow manually from this branch (theResolve the script to runstep, now onmain, makes that work before merge) and verify a normal sync — versions differ and the submodule pointer is behind — still produces both commits:Update version to <version> (sync recovery)followed byUpdate Mobile-Expensify submodule version to <version> (sync recovery).Verify the
TARGET_VERSIONguard from the deployer side: trigger the same workflow withTARGET_VERSIONset to something Mobile-Expensify is not on, and verify the run fails on::error::TARGET_VERSION (...) must match the Mobile-Expensify version (...)with nothing pushed tomain.Offline tests
Not applicable — this changes a GitHub Actions workflow script. There is no client-side behavior and no network state to vary.
QA Steps
Not applicable — no user-facing behavior changes. The workflow is a manual
workflow_dispatchrestricted to mobile deployers and is validated by triggering it directly (see step 5 inTests), not on staging or production.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — CI workflow change only.
Android: mWeb Chrome
N/A — CI workflow change only.
iOS: Native
N/A — CI workflow change only.
iOS: mWeb Safari
N/A — CI workflow change only.
MacOS: Chrome / Safari
N/A — CI workflow change only.