[codex] fix desktop update packaging integrity#96
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 763a748745
ℹ️ 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".
| return new Promise((resolve, reject) => { | ||
| const request = NodeHttp.get( | ||
| { | ||
| hostname: "127.0.0.1", |
There was a problem hiding this comment.
In environments where Node resolves localhost to IPv6 (for example Node 24 on this runner binds server.listen(..., "localhost") to ::1), this smoke test never reaches the mock update server: scripts/mock-update-server.ts listens on localhost, but the new readiness probe always dials 127.0.0.1, so waitForUpdateServer times out before the update flow starts. Please make the mock server and probe use the same explicit host, or probe localhost here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8cbcb3544
ℹ️ 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".
| rootPackageNames: [ | ||
| ...Object.keys(resolvedServerDependencies), | ||
| ...Object.keys(platformFffNativeDependencies), | ||
| ...Object.keys(linuxWslFffNativeDependencies), |
There was a problem hiding this comment.
When packaging Windows/macOS builds, stageClerkPasskeyNativeBinaries copies Clerk passkey .node files into the desktop runtime package, but this manifest seed only walks server/fff dependencies. If an update leaves one of those smart-unpacked desktop native files missing from app.asar.unpacked, the new bootstrap integrity check still passes and the app can fail later while initializing createClerkBridge({ passkeys: true }), which defeats the incomplete-install diagnostic this change is adding. Please include the desktop native unpack roots (at least @clerk/electron-passkeys) in the required-file manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 473ccf14d1
ℹ️ 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".
| return { | ||
| ...state, | ||
| status: "installing", | ||
| message: "Installing update. T3 Code will restart when installation is ready.", |
There was a problem hiding this comment.
When installUpdate() succeeds, DesktopUpdates.install returns the current state after setting this installing message, with { accepted: true, completed: false }. Both the sidebar and settings flows interpret any accepted/non-completed action with a non-empty state.message as an error toast, so a normal install handoff can briefly show “Could not install update” with this installing text before the app restarts. Please keep the installing status message out of the action-error path or special-case installing as non-error.
Useful? React with 👍 / 👎.
wizzoapp
Bot
force-pushed
the
wizzo/ada-55-desktop-update-integrity
branch
from
473ccf1 to
9c0ba5c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c0ba5c3bd
ℹ️ 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".
| } | ||
|
|
||
| const missingFiles = manifest.requiredFiles.filter( | ||
| (relativePath) => !NodeFS.existsSync(NodePath.join(unpackedRoot, relativePath)), |
There was a problem hiding this comment.
When an update or over-existing install leaves app.asar from the new version but app.asar.unpacked from the previous version, the same runtime paths still exist, so this existence-only check passes and the app continues into a mixed-version install. That is exactly the class of partial desktop update this bootstrap is meant to diagnose; include and verify at least size or hash metadata from the generated manifest so stale unpacked files are treated as integrity failures rather than surfacing later as runtime crashes.
Useful? React with 👍 / 👎.
|
|
||
| yield* Ref.set(desktopState.quitting, true); | ||
| yield* Ref.set(updateInstallInFlightRef, true); | ||
| yield* setState(reduceDesktopUpdateStateOnInstallStart(state)); |
There was a problem hiding this comment.
When the install effect is interrupted after this transition but before quitAndInstall either succeeds or reports an error, the onInterrupt path below only clears the in-flight/quitting refs and leaves the update state as installing. Because checks and UI controls now treat installing as non-actionable, an app that remains alive after that cancellation can be stuck with update controls disabled and no retry path until restart; restore the prior downloaded state or mark an install failure in the interrupt finalizer.
Useful? React with 👍 / 👎.
wizzoapp
Bot
force-pushed
the
wizzo/ada-55-desktop-update-integrity
branch
from
fac5777 to
7b3d358
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b3d358172
ℹ️ 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".
| @@ -0,0 +1,565 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
This adds the N-1 → update → relaunch smoke test, but it is never invoked: rg desktop-update-smoke only finds this new file, and the touched workflow steps still only run scripts/desktop-launch-smoke.mjs. As a result, the Windows updater path this change is meant to protect can still regress without CI noticing; please add this script to the Windows artifact workflow or to a package script that CI runs.
Useful? React with 👍 / 👎.
| } | ||
| if ($outputText -notmatch "(?i)(apps/server/dist/bin\.mjs|missing server entry|backend|timed out)") { | ||
| throw "Launch smoke failed against the broken packaged artifact, but did not report the expected missing backend entry." | ||
| if ($outputText -notmatch "(?i)(installation is incomplete|apps/server/dist/bin\.mjs)") { |
There was a problem hiding this comment.
This negative fixture is supposed to prove the new bootstrap integrity check reports the incomplete-installation diagnostic, but the regex also accepts any output containing apps/server/dist/bin.mjs (the same pattern is duplicated in reusable-build-release-artifacts.yml). If bootstrap gets bypassed and the app later dies with a raw missing-module error for the removed file, this step still passes, so the gate no longer verifies the diagnostic it claims to protect.
Useful? React with 👍 / 👎.
| async function collectRelativeFiles(input) { | ||
| const { root, current, logicalRoot, files, visited } = input; | ||
| const realCurrent = await NodeFSP.realpath(current).catch(() => current); | ||
| const visitedKey = `${realCurrent}\0${logicalRoot}`; |
There was a problem hiding this comment.
When app.asar.unpacked contains a symlinked directory cycle, including logicalRoot in the visited key makes each traversal of the same real directory look new (root, loop, loop/loop, ...). The manifest walk can then emit many duplicate required paths or hit symlink-depth/path limits during afterPack, blocking desktop artifact builds for pnpm-style dependency symlink cycles; track visited directories by real path before following symlink directories.
Useful? React with 👍 / 👎.
wizzoapp
Bot
force-pushed
the
wizzo/ada-55-desktop-update-integrity
branch
from
7b3d358 to
13fe6ba
Compare
|
Closing per the starved Codex-review recycle recipe. The rebased branch is green on CI at 13fe6ba, including Desktop Linux Package Smoke and Desktop Windows Package Smoke; I will open the fresh replacement PR from the same branch and link it here next. |
Summary
Fixes the desktop update corruption path by shrinking the Windows unpacked payload, disabling differential updater downloads, adding a launch-time packaged integrity check, and adding CI coverage for a real install N-1 -> update -> relaunch path.
Root cause
The desktop package unpacked the entire production node_modules tree into app.asar.unpacked. Windows in-app updates then had to install thousands of loose files, making silent install slow and increasing the chance of a partial app.asar.unpacked payload after update.
Changes
Validation
Note: the true Windows update-path proof is the new CI job; this PR should not be considered fixed until that job passes on GitHub.