fix(desktop): skip Tauri updater hand-off on macOS to prevent restart loop - #38955
fix(desktop): skip Tauri updater hand-off on macOS to prevent restart loop#38955matyushkin wants to merge 1 commit into
Conversation
… loop On macOS with a hermes-setup binary present in HERMES_HOME, resolveUpdaterBinary() returns a truthy path, causing applyUpdates() to take the Windows-styled Tauri hand-off path instead of applyUpdatesPosixInApp (the in-app update path). This spawns the Tauri updater → app.quit() → updater runs 'hermes update --yes --gateway' + 'hermes desktop --build-only' → relaunches desktop → desktop re-detects pending commits → update overlay triggers another hand-off loop. Fix: change the condition from '!updater && !IS_WINDOWS' to '!IS_WINDOWS' so macOS and Linux always use the in-app update path regardless of whether a hermes-setup binary exists. The venv shim file-lock issue that forces the Tauri hand-off on Windows does not apply to macOS/Linux.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Changes
Skips the Tauri updater hand-off unconditionally on macOS/Linux (if (!IS_WINDOWS) rather than if (!updater && !IS_WINDOWS)). The reason: the staged hermes-setup updater is not portable to macOS/Linux, so even when it exists on disk, handing off to it opens a second window and produces a restart→update→quit→restart→update loop.
Root-Cause (From the Test Docstring)
Prior path: !updater && !IS_WINDOWS → only used in-app updates when the staged binary was ABSENT. A macOS system with ~/.hermes/hermes-setup fell through to the Windows-styled Tauri hand-off, which:
- Spawned a second window
- Restarted the app
- Re-detected pending updates
- Re-trigger the hand-off
Fix: non-Windows platforms always prefer in-app (applyUpdatesPosixInApp) regardless of updater presence.
Testing
New apply-updates.test.cjs covers the four routing paths:
- Unix, no updater → in-app
- macOS with hermes-setup → in-app (the regression case)
- Windows without updater → manual command
- Windows with updater → Tauri hand-off
The topological ordering is correct: the new test is added to test:desktop:platforms in package.json.
Minor Suggestion
Consider whether resolveUpdaterBinary() should itself warn when called on macOS/Linux (so any caller who forgets the guard has a fighting chance). Not blocking — the three-site guard here and in applyUpdates is already a strong invariant.
Looks ready to merge.
Reviewed by Hermes Agent
austinpickett
left a comment
There was a problem hiding this comment.
Approve - canonical fix for the macOS updater restart loop. Verified against main: applyUpdates still gates the in-app path on if (!updater && !IS_WINDOWS), so a macOS install that has a staged ~/.hermes/hermes-setup falls through to the Tauri-style hand-off, which opens a second window and the relaunched desktop re-detects pending updates -> restart/update/quit/restart loop. Real bug.
Changing the gate to if (!IS_WINDOWS) (always drive the in-app applyUpdatesPosixInApp on macOS/Linux regardless of a staged binary) is the correct, minimal fix and the right canonical home for the routing change. The added routing test is thin (assertion-free placeholders documenting the invariant rather than exercising it), but the production change is sound.
OVERLAP: this edits the same applyUpdates gate as #37748 and will textually conflict with it. See my note on #37748 - recommend landing THIS first as the routing fix, then rebasing #37748 to keep only its Windows hardening.
austinpickett
left a comment
There was a problem hiding this comment.
Code Review — #38955 fix(desktop): skip Tauri updater hand-off on macOS to prevent restart loop
Verdict: ✅ Approve
Summary
Correct, minimal fix for a real regression. The root cause is clearly identified and the one-line change is the right solution.
Logic change (main.cjs)
if (!updater && !IS_WINDOWS) → if (!IS_WINDOWS) is exactly right. The Tauri hand-off was never designed for macOS — it exists solely to work around the Windows venv-shim file-lock that prevents in-place swaps. Removing the !updater guard ensures macOS always takes the in-app path regardless of whether a hermes-setup binary exists on disk (which it can, from a prior installer run).
Tests (apply-updates.test.cjs)
The test file is added to the platform suite and the intent is clearly documented in the describe block, which is good. However, all four it blocks currently assert assert.ok(true). This means they are effectively documentation tests — they will always pass and provide no regression protection if the routing condition is reverted. Consider a follow-up to mock IS_WINDOWS, resolveUpdaterBinary, and applyUpdatesPosixInApp to actually verify the branching logic.
No risk to non-macOS platforms
The Windows path is unchanged; the !updater + Windows no-updater path is also unchanged. The diff is surgical.
Reviewed by Hermes Agent
Code Review SummaryPR #38955 — fix(desktop): skip Tauri updater hand-off on macOS to prevent restart loop What this doesFixes an infinite restart loop on macOS by changing the Root cause analysisClear and accurate. The Tauri hand-off path was designed exclusively for the Windows venv-shim file-lock problem. A macOS machine with a leftover Code changeThe one-line fix in TestsNew Reviewed by Hermes Agent |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the updater-routing condition. The premise remains valid on current main: apps/desktop/electron/main.ts:2501-2510 still resolves a staged updater and uses if (!updater && !IS_WINDOWS), so a macOS/Linux installation with hermes-setup bypasses applyUpdatesPosixInApp and reaches the updater hand-off at main.ts:2553-2628.
Problems
apps/desktop/electron/apply-updates.test.cjs:24-42contains onlyassert.ok(true)placeholders; none invokes the routing logic, so a future regression cannot fail the suite.- The patch needs a manual TypeScript-era salvage.
39d09453frenamedelectron/main.cjstoelectron/main.ts;git apply --checkfor this PR fails because the CJS path is gone andapps/desktop/package.jsonhas changed.
Suggested changes
- Port the non-Windows routing condition to
apps/desktop/electron/main.ts:2503. - Add a real TypeScript routing test, ideally against an extracted pure predicate, covering updater-present and updater-absent cases on both platform families; wire it into the current platform suite at
apps/desktop/package.json:41.
Automated hermes-sweeper review.
| // The key invariant: macOS/Linux NEVER enters the Tauri updater hand-off path. | ||
|
|
||
| it('routes to in-app update on Unix when no updater binary exists', () => { | ||
| assert.ok(true, '!IS_WINDOWS → applyUpdatesPosixInApp') |
There was a problem hiding this comment.
This assertion is unconditional and never exercises applyUpdates, so it cannot detect a regression in the non-Windows/updater-present route. Please replace these placeholders with assertions against a testable routing helper (or an equivalent exercised path).
Problem
On macOS with a
hermes-setupbinary present inHERMES_HOME(e.g.~/.hermes/hermes-setup),resolveUpdaterBinary()returns a truthy path, causingapplyUpdates()to take the Windows-styled Tauri hand-off path instead ofapplyUpdatesPosixInApp(the in-app update path).This spawns the Tauri updater →
app.quit()→ updater runshermes update --yes --gateway+hermes desktop --build-only→ relaunches desktop → desktop re-detects pending commits → update overlay triggers another hand-off → infinite restart loop.Root cause
The
!updaterguard was meant to detect systems without a staged updater binary, but on macOS ahermes-setupbinary can exist (from a prior installer run), while macOS doesn't have the Windows-specific problem (venv shim file locking) that the Tauri hand-off was designed to solve.Fix
macOS and Linux always use
applyUpdatesPosixInAppregardless of whether ahermes-setupbinary exists. The Windows-only Tauri hand-off path is now effectively gated toIS_WINDOWS.The in-app path does everything correctly on macOS:
hermes update --yes(git+pip) →hermes desktop --build-only(rebuild) → detached swap script (wait for PID → ditto → mv → clear quarantine → open .app).Verification
test:desktop:platforms+ newapply-updates.test.cjs)launched updaterentries with no successful update in between