[codex] merge upstream warm-start, zypper, and updater lock updates#8
[codex] merge upstream warm-start, zypper, and updater lock updates#8
Conversation
|
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:
📝 WalkthroughWalkthroughLauncher now supports warm-start reuse/adoption of verified webview servers with stricter PID/process checks; prelaunch runs updater staleness checks asynchronously; updater gains non-blocking file-lock and freshness gating with a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Launcher as Launcher\n(start.sh)
participant ProcCheck as Process\nDetection
participant WebServer as Webview\nServer
participant Electron as Electron
participant CLI as CLI\nPreflight
User->>Launcher: Start package
Launcher->>Launcher: Check WARM_START
alt Warm-start path
Launcher->>ProcCheck: Inspect existing Electron PID & exe
ProcCheck-->>Launcher: PID valid / invalid
alt Valid electron process
Launcher->>WebServer: Validate webview on 5175 in app dir
alt Verified webview exists
WebServer-->>Launcher: Adopt existing server
Launcher->>CLI: Trigger preflight (background)
else Not verified / unowned
Launcher->>WebServer: Stop unverified, start new server
end
else No valid electron
Launcher->>WebServer: Start new webview server
Launcher->>Electron: Spawn Electron
end
else Cold-start
Launcher->>WebServer: Start webview server
Launcher->>CLI: Run preflight (sync/async per flag)
CLI-->>Launcher: Preflight done
Launcher->>Electron: Spawn Electron
end
Electron-->>User: UI ready
sequenceDiagram
participant Launcher as Package\nLauncher
participant Background as Prelaunch\nBackground
participant Updater as Updater
participant Lock as check.lock\n(fs4)
participant Config as Staleness\nConfig
Launcher->>Background: Start async prelaunch
Background->>Background: Setup systemd/env (background)
Background->>Updater: Run `check-now --if-stale`
Updater->>Lock: Try exclusive lock
alt Lock acquired
Updater->>Config: Read last_successful_check_at
Updater->>Config: Compare with check_interval_hours
alt Fresh (within interval)
Updater-->>Background: Skip upstream check, reconcile pending installs
else Stale
Updater->>Updater: Perform upstream check
Updater->>Config: Update last_successful_check_at
end
Updater->>Lock: Release lock
else Lock held
Updater-->>Background: Skip due to overlapping check
end
Background-->>Launcher: Continue startup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Pull request overview
Merges upstream codex-desktop-linux changes into this fork while preserving local codex-app naming and behaviors, including updated launcher/runtime flow, updater CLI behavior, and Linux distro support.
Changes:
- Adds openSUSE support (zypper) for dependency installation and RPM installation paths.
- Extends updater CLI with
check-now --if-staleand adds an upstream-check lock to avoid overlapping checks. - Updates the generated launcher to Electron
41.3.0, adds warm-start detection, and reuses a verified webview server when possible.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
updater/src/install.rs |
Adds zypper-backed RPM installation command + tests. |
updater/src/cli.rs |
Extends check-now command with --if-stale flag. |
updater/src/app.rs |
Wires --if-stale, adds upstream freshness check and kernel-backed check lock. |
updater/Cargo.toml |
Adds fs4 dependency for locking. |
Cargo.lock |
Locks new transitive dependencies (notably fs4). |
packaging/linux/packaged-runtime.sh |
Triggers codex-app-updater check-now --if-stale during launch. |
install.sh |
Updates Electron version, adds warm-start + webview-server reuse, adjusts CLI preflight flow. |
tests/scripts_smoke.sh |
Updates launcher template and lifecycle assertions to match new launcher behavior. |
scripts/patch-linux-window-ui.js |
Brings upstream Linux UI/tray/single-instance patches. |
scripts/install-deps.sh |
Adds openSUSE dependency installation via zypper. |
Makefile |
Adds zypper install support and updates RPM help text. |
flake.nix |
Updates DMG hash to match upstream. |
docs/maintainers/package-runtime-maintenance.md |
Documents new launcher + updater behavior and zypper RPM install order. |
CHANGELOG.md |
Notes the merged upstream behavior changes. |
scripts/build-rpm.sh |
Minor cleanup (removes stray blank line). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: d0d634a0f4
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/maintainers/package-runtime-maintenance.md`:
- Around line 183-185: The RPM install fallback order is inconsistent: update
the earlier mention of the RPM install sequence (currently "dnf/dnf5 → rpm
-Uvh") to match the later text by including "zypper" so both places read
"dnf/dnf5 → zypper → rpm -Uvh"; locate the paragraph that lists package manager
fallbacks and change the RPM fallback sequence string accordingly so the
document consistently uses "dnf/dnf5 → zypper → rpm -Uvh".
In `@install.sh`:
- Around line 884-900: The adopted webview server is being treated as this
launcher's child by assigning its PID to STARTED_WEBVIEW_PID in
adopt_existing_webview_server(), which leads the cleanup path to kill it; change
adopt_existing_webview_server() to set a separate ADOPTED_WEBVIEW_PID
(initialize this variable with other launcher state) when you detect an existing
server via owned_webview_server_pid or discover_webview_server_pid and write
WEBVIEW_PID_FILE, and leave STARTED_WEBVIEW_PID only for processes this launcher
actually starts (e.g., start_webview_server()). Also remove or relax the
immediate post-adoption verification that can race a server still starting (do
not convert an adoption into a failure that triggers stopping a server); ensure
the exit/cleanup logic only attempts to kill STARTED_WEBVIEW_PID and never
ADOPTED_WEBVIEW_PID.
In `@packaging/linux/packaged-runtime.sh`:
- Around line 52-58: The current conditional runs systemd-run and then
unconditionally returns 0, which swallows systemd-run failures and prevents the
safe direct fallback; modify the block around the systemd-run invocation so you
only return on success (e.g., capture the exit status of the systemd-run command
and if it succeeds return 0), otherwise allow execution to continue to the
direct fallback invocation of /usr/bin/codex-app-updater check-now --if-stale;
reference the systemd-run call and the codex-app-updater check-now command when
making the change.
In `@scripts/install-deps.sh`:
- Around line 74-80: Update README.md to document openSUSE support and reference
the new install path: add a short section (or extend the existing "Supported
distributions" block) stating that openSUSE is supported and that dependencies
can be installed via the install-deps script's install_zypper function; mention
the package names (nodejs-default, npm-default, python3, p7zip-full, curl,
unzip) and the devel_basis pattern and note that install-deps.sh runs zypper
with --non-interactive for unattended installs so maintainers/users know how to
install on openSUSE and where to look in the repository.
In `@scripts/patch-linux-window-ui.js`:
- Around line 300-308: The current short needle trayStartupNeedle ("E&&oe();")
is too generic and risks false matches; update the patch logic to use a longer,
more specific needle string (or a precise regex) that includes surrounding
context from the bundle (for example additional surrounding tokens/characters
around "E&&oe();" such as the preceding expression and following
semicolon/paren) and then replace that exact longer pattern with
trayStartupPatch ("(E||process.platform===`linux`)&&oe();"); modify the code
that sets trayStartupNeedle/trayStartupPatch and the
patchedSource.includes/replace checks (referencing trayStartupNeedle,
trayStartupPatch and patchedSource) so the match is robust against other
minified occurrences and only replaces the intended tray startup call.
In `@updater/Cargo.toml`:
- Line 12: The Cargo.toml currently pins the fs4 crate to "0.13.1"; update that
dependency to the new stable version "1.0.2" by replacing the fs4 version
specifier (the dependency entry named fs4) with 1.0.2 and run cargo update /
cargo build to ensure the lockfile is refreshed and that there are no
API-breaking changes to address in code that imports fs4.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2e5a643f-4c53-4631-8992-b686d3d8f011
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
CHANGELOG.mdMakefiledocs/maintainers/package-runtime-maintenance.mdflake.nixinstall.shpackaging/linux/packaged-runtime.shscripts/build-rpm.shscripts/install-deps.shscripts/patch-linux-window-ui.jstests/scripts_smoke.shupdater/Cargo.tomlupdater/src/app.rsupdater/src/cli.rsupdater/src/install.rs
💤 Files with no reviewable changes (1)
- scripts/build-rpm.sh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
install.sh (1)
906-913:⚠️ Potential issue | 🟠 MajorWait for an adopted webview server to become ready before failing.
A second launcher can discover the first launcher's
python3 -m http.serverin/procbefore it is actually accepting connections. This branch converts that transient state into a hard startup failure instead of reusing the server a few hundred milliseconds later.Suggested fix
if adopt_existing_webview_server; then - if verify_webview_origin "http://127.0.0.1:5175/index.html" >/dev/null 2>&1; then + if wait_for_webview_server \ + && verify_webview_origin "http://127.0.0.1:5175/index.html" >/dev/null 2>&1; then echo "Reusing existing verified webview server on :5175" log_phase "webview_reused" return 0 fi notify_error "Existing Codex webview server on port 5175 did not verify. Stop that server and try again." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 906 - 913, The current branch returns failure immediately when adopt_existing_webview_server finds a process but verify_webview_origin fails; instead implement a short retry-with-timeout loop in the installer: after adopt_existing_webview_server returns true, poll verify_webview_origin (call it) for a configurable interval (e.g., total timeout ~5–10s with short sleeps) before treating the server as non-responsive; if verify_webview_origin succeeds within the timeout, log "webview_reused" and return 0, otherwise call notify_error and exit 1. Ensure you only modify the logic around adopt_existing_webview_server and verify_webview_origin invocations and preserve existing log_phase/notify_error 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 `@Makefile`:
- Around line 121-127: The zypper branch is unconditionally adding
--allow-unsigned-rpm to installs using the picked $$rpm (derived from RPM or
RPM_GLOB), which lets users bypass signature checks for arbitrary RPM values;
change the logic in the Makefile branch that sets rpm and calls zypper so the
--allow-unsigned-rpm flag is only appended when the selected $$rpm is the
locally built artifact (i.e., matches the dist/ artifact naming coming from
RPM_GLOB or the default selection logic), by adding a conditional around the
zypper invocation that checks the $$rpm source (e.g., whether it came from
RPM_GLOB/default or starts with "dist/") and only then includes
--allow-unsigned-rpm; refer to the variables RPM, RPM_GLOB and the zypper
install command in the Makefile to locate and update the code.
---
Duplicate comments:
In `@install.sh`:
- Around line 906-913: The current branch returns failure immediately when
adopt_existing_webview_server finds a process but verify_webview_origin fails;
instead implement a short retry-with-timeout loop in the installer: after
adopt_existing_webview_server returns true, poll verify_webview_origin (call it)
for a configurable interval (e.g., total timeout ~5–10s with short sleeps)
before treating the server as non-responsive; if verify_webview_origin succeeds
within the timeout, log "webview_reused" and return 0, otherwise call
notify_error and exit 1. Ensure you only modify the logic around
adopt_existing_webview_server and verify_webview_origin invocations and preserve
existing log_phase/notify_error behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 132e138a-3563-4c8a-b1be-3656b7f3d334
📒 Files selected for processing (4)
Makefileinstall.shtests/scripts_smoke.shupdater/src/install.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nisavid
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12f6fe1ffb
ℹ️ 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".
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)
packaging/linux/packaged-runtime.sh (1)
7-18:⚠️ Potential issue | 🟠 MajorDon’t gate
check-now --if-staleon user-systemd availability.These early returns bypass
codex_packaged_runtime_trigger_update_check()entirely, so package launches stop nudging the updater wheneversystemctl --useris unavailable or unhealthy, even though the helper below already has a direct fallback for exactly that case.Proposed fix
codex_packaged_runtime_prelaunch_background() { - if ! command -v systemctl >/dev/null 2>&1; then - return 0 - fi - - if [ -z "${XDG_RUNTIME_DIR:-}" ] || [ ! -d "$XDG_RUNTIME_DIR" ]; then - return 0 - fi - - if ! systemctl --user show-environment >/dev/null 2>&1; then - return 0 + if command -v systemctl >/dev/null 2>&1 \ + && [ -n "${XDG_RUNTIME_DIR:-}" ] \ + && [ -d "$XDG_RUNTIME_DIR" ] \ + && systemctl --user show-environment >/dev/null 2>&1; then + systemctl --user import-environment \ + DISPLAY \ + WAYLAND_DISPLAY \ + DBUS_SESSION_BUS_ADDRESS \ + XAUTHORITY \ + XDG_RUNTIME_DIR >/dev/null 2>&1 || true + + if command -v dbus-update-activation-environment >/dev/null 2>&1; then + dbus-update-activation-environment --systemd \ + DISPLAY \ + WAYLAND_DISPLAY \ + DBUS_SESSION_BUS_ADDRESS \ + XAUTHORITY \ + XDG_RUNTIME_DIR >/dev/null 2>&1 || true + fi + + systemctl --user disable --now codex-update-manager.service >/dev/null 2>&1 || true + + if systemctl --user is-enabled codex-app-updater.service >/dev/null 2>&1; then + systemctl --user restart codex-app-updater.service >/dev/null 2>&1 || true + else + systemctl --user enable --now codex-app-updater.service >/dev/null 2>&1 || true + fi fi - - systemctl --user import-environment \ - DISPLAY \ - WAYLAND_DISPLAY \ - DBUS_SESSION_BUS_ADDRESS \ - XAUTHORITY \ - XDG_RUNTIME_DIR >/dev/null 2>&1 || true - - if command -v dbus-update-activation-environment >/dev/null 2>&1; then - dbus-update-activation-environment --systemd \ - DISPLAY \ - WAYLAND_DISPLAY \ - DBUS_SESSION_BUS_ADDRESS \ - XAUTHORITY \ - XDG_RUNTIME_DIR >/dev/null 2>&1 || true - fi - - systemctl --user disable --now codex-update-manager.service >/dev/null 2>&1 || true - - if systemctl --user is-enabled codex-app-updater.service >/dev/null 2>&1; then - systemctl --user restart codex-app-updater.service >/dev/null 2>&1 || true - else - systemctl --user enable --now codex-app-updater.service >/dev/null 2>&1 || true - fi codex_packaged_runtime_trigger_update_check }Also applies to: 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packaging/linux/packaged-runtime.sh` around lines 7 - 18, The early-return checks inside codex_packaged_runtime_prelaunch_background prevent codex_packaged_runtime_trigger_update_check from running when user-systemd or XDG_RUNTIME_DIR is missing, so remove or relax those early returns and always call codex_packaged_runtime_trigger_update_check (which contains its own fallback to run check-now --if-stale directly when systemctl --user is unavailable); specifically, ensure codex_packaged_runtime_prelaunch_background no longer returns before invoking codex_packaged_runtime_trigger_update_check and apply the same change for the analogous checks at the later block (lines around the second occurrence) so the updater nudges still run without relying on systemd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packaging/linux/packaged-runtime.sh`:
- Around line 47-62: In codex_packaged_runtime_trigger_update_check: resolve the
packaged updater to an absolute path once (e.g. check for a packaged binary at
/usr/bin/codex-app-updater and prefer that over PATH or use command -v with a
fallback) and use that resolved path in the initial guard and in both branches
(the systemd-run --unit invocation and the direct fallback call) instead of
mixing command -v and a hardcoded /usr/bin path; also ensure the guard verifies
the resolved file is executable before proceeding so the same updater binary is
used consistently.
In `@scripts/patch-linux-window-ui.js`:
- Around line 181-313: The applyLinuxTrayPatch function currently applies
multiple independent replacements which can leave the tray feature half-broken;
change it to perform an all-or-nothing check before mutating patchedSource: scan
for all required anchors (at minimum trayGuardNeedle, trayIconNeedle,
closeToTrayNeedle, trayContextMethodNeedle, trayClickNeedle or existing
setLinuxTrayContextMenu presence, trayMenuBuildNeedle, trayContextMenuNeedle,
trayStartupNeedle, trayMenuThreadsNeedle) and if any required needle is missing,
skip all Linux-specific replacements and emit a single warning; only when the
full set is present (or already patched) proceed to apply the individual
replaces (including creating setLinuxTrayContextMenu, altering click handlers,
menu build, popup guards, startup and threads patches) so dependent changes like
setLinuxTrayContextMenu are never left out.
---
Outside diff comments:
In `@packaging/linux/packaged-runtime.sh`:
- Around line 7-18: The early-return checks inside
codex_packaged_runtime_prelaunch_background prevent
codex_packaged_runtime_trigger_update_check from running when user-systemd or
XDG_RUNTIME_DIR is missing, so remove or relax those early returns and always
call codex_packaged_runtime_trigger_update_check (which contains its own
fallback to run check-now --if-stale directly when systemctl --user is
unavailable); specifically, ensure codex_packaged_runtime_prelaunch_background
no longer returns before invoking codex_packaged_runtime_trigger_update_check
and apply the same change for the analogous checks at the later block (lines
around the second occurrence) so the updater nudges still run without relying on
systemd.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 30e9df2c-be07-4149-9a25-480d5440f9e8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
MakefileREADME.mddocs/maintainers/package-runtime-maintenance.mdinstall.shpackaging/linux/packaged-runtime.shscripts/patch-linux-window-ui.jstests/scripts_smoke.shupdater/Cargo.tomlupdater/src/app.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e21fd48f25
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbe27319a3
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
install.sh (1)
914-921:⚠️ Potential issue | 🟠 MajorWait for an adopted server to come up before failing reuse.
This path verifies an adopted server immediately. On two near-simultaneous cold starts, the second launcher can discover the first launcher's
http.serverbefore it is listening, failverify_webview_origin, and abort even though the server would become healthy a moment later. Reuse should go throughwait_for_webview_serverhere too.Proposed fix
if adopt_existing_webview_server; then - if verify_webview_origin "http://127.0.0.1:5175/index.html" >/dev/null 2>&1; then + if wait_for_webview_server && verify_webview_origin "http://127.0.0.1:5175/index.html" >/dev/null 2>&1; then echo "Reusing existing verified webview server on :5175" log_phase "webview_reused" return 0 fi notify_error "Existing Codex webview server on port 5175 did not verify. Stop that server and try again." exit 1Based on learnings: For webview changes, verify the local server still serves expected Codex webview startup assets before Electron launch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 914 - 921, The reuse branch currently calls verify_webview_origin immediately after adopt_existing_webview_server and fails fast; change it to wait for the adopted server to become healthy by calling wait_for_webview_server (or calling wait_for_webview_server before verify_webview_origin) so the script retries until the local HTTP server is listening/serving, then run verify_webview_origin and only log_phase "webview_reused" or call notify_error/exit on final verification failure; refer to adopt_existing_webview_server, wait_for_webview_server, verify_webview_origin, notify_error and log_phase to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 823-834: Replace the brittle substring test in
pid_is_webview_server with a strict argv token check: read /proc/$pid/cmdline as
NUL-separated arguments (e.g., loop with read -r -d '' arg) and require that one
of the args equals exactly "5175" (not just contains " 5175"), keeping the other
checks (pid validity, ownership via pid_is_current_user, cwd compare with
canonical_path "$WEBVIEW_DIR") and returning non-zero if no exact token match is
found; update the cmdline handling in pid_is_webview_server accordingly.
In `@updater/src/install.rs`:
- Around line 470-487: The zypper_install_command function currently adds the
unsafe flag "--allow-unsigned-rpm" by default; remove that flag from the args in
zypper_install_command (and from any other use of ZYPPER_CANDIDATES/"zypper")
and make unsigned-RPM installation opt-in by adding an explicit allow_unsigned
parameter to zypper_install_command (or read a clearly named env/config flag
like ALLOW_UNSIGNED_RPM) that, when true, appends "--allow-unsigned-rpm" to the
Command; update all callers of zypper_install_command to pass the new parameter
(or ensure tests and call sites use the env/config to opt-in) so unsigned
installs are never permitted implicitly.
---
Duplicate comments:
In `@install.sh`:
- Around line 914-921: The reuse branch currently calls verify_webview_origin
immediately after adopt_existing_webview_server and fails fast; change it to
wait for the adopted server to become healthy by calling wait_for_webview_server
(or calling wait_for_webview_server before verify_webview_origin) so the script
retries until the local HTTP server is listening/serving, then run
verify_webview_origin and only log_phase "webview_reused" or call
notify_error/exit on final verification failure; refer to
adopt_existing_webview_server, wait_for_webview_server, verify_webview_origin,
notify_error and log_phase to locate and update the logic.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e28f93f9-daa5-4aae-9dee-e59799e38321
📒 Files selected for processing (3)
install.shtests/scripts_smoke.shupdater/src/install.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
nisavid
force-pushed
the
chore/merge-upstream-2026-04-28
branch
from
07efb7c to
c81b013
Compare
nisavid
force-pushed
the
chore/merge-upstream-2026-04-28
branch
from
c81b013 to
8f01875
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge upstream Linux app updates and review fixes for warm-start webview reuse, updater staleness checks, openSUSE/zypper handling, Electron 41.3.0, and Linux tray behavior. Validation: local shell syntax checks, node --check, cargo test -p codex-app-updater, tests/scripts_smoke.sh, and PR checks. Co-authored-by: Codex <noreply@openai.com>
nisavid
force-pushed
the
chore/merge-upstream-2026-04-28
branch
from
8f01875 to
684327b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 684327bc4e
ℹ️ 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".
| if if_stale && state.status != UpdateStatus::Failed && upstream_check_is_fresh(config, state) { | ||
| info!("skipping check-now because the last successful upstream check is still fresh"); | ||
| return reconcile_pending_install(config, state, paths).await; |
There was a problem hiding this comment.
The new --if-stale early return applies to any status except Failed, which includes transient states like CheckingUpstream. If a previous updater process is interrupted after persisting an in-progress status and a fresh last_successful_check_at, this branch returns before run_check_cycle, and reconcile_pending_install does not recover CheckingUpstream; launch-time checks can be suppressed until the full freshness window expires. Restrict this shortcut to stable terminal statuses (for example Idle/Installed) or force a retry when state is in an in-progress check status.
Useful? React with 👍 / 👎.
Summary
Merges upstream
ilysenko/codex-desktop-linuxhead40fd7a8into this fork while preserving the localcodex-apppackage/runtime naming, protected-main workflow, updater boundaries, and updater-first CLI discovery behavior.What changed
41.3.0, warm-start detection, and verified webview server reuse.codex-app-updater check-now --if-stalehandling through the packaged runtime helper.fs4.Validation
bash -n install.sh scripts/build-deb.sh scripts/build-rpm.sh scripts/build-pacman.sh scripts/install-deps.sh packaging/linux/packaged-runtime.shcargo check -p codex-app-updatercargo test -p codex-app-updaterbash tests/scripts_smoke.shSummary by CodeRabbit
New Features
Chores
Documentation
Tests
Greptile Summary
This PR merges upstream warm-start, zypper/openSUSE, and updater lock changes into the fork while preserving local naming and workflow conventions. The implementation is solid overall — the fs4-backed check lock, idempotent JS patches, warm-start PID tracking, and background preflight are all well-structured.
updater/src/install.rs:zypper_install_commandomits--allow-unsigned-rpm. On default openSUSE (gpgcheck=on), zypper will refuse to install the locally-built, unsigned RPM that the updater produces, silently breaking the privileged install path for all openSUSE users. TheMakefilealready adds this flag for packages indist/— the same flag needs to be added to the Rust command.Confidence Score: 4/5
Safe to merge for Debian/Fedora/Arch users; openSUSE updater installs will silently fail until the missing --allow-unsigned-rpm flag is added.
One P1 defect: the zypper install command in the Rust updater omits --allow-unsigned-rpm, which the Makefile correctly includes. On default openSUSE this breaks the privileged install path entirely. All other changes are well-implemented and tested.
updater/src/install.rs — zypper_install_command and its unit test need --allow-unsigned-rpm
Important Files Changed
Comments Outside Diff (4)
install.sh, line 491-493 (link)nohupremoved from webview server startThe previous invocation used
nohup python3 -m http.server --bind 127.0.0.1 5175 &, which protected the background process from SIGHUP when its controlling terminal closed. The new formpython3 -m http.server 5175 --bind 127.0.0.1 &removes that protection. In practice the EXIT trap will kill the server on a clean shell exit anyway, but if the controlling terminal is killed (e.g. SIGHUP from an SSH disconnect), the webview server will also receive SIGHUP and die before the EXIT trap runs — potentially leaving Electron without its webview backend for the remainder of the session.Prompt To Fix With AI
updater/src/install.rs, line 1402-1418 (link)--no-gpg-checksskips RPM signature verification in zypperzypper --no-gpg-checks installsuppresses GPG/RPM signature validation entirely. The staged-copy and identity-check logic ininstall_rpmreduces source-replacement risk, but the package itself is never signature-verified — unlikerpm -Uvhwhich will verify signatures when they exist. Consider whether--no-gpg-checksis necessary here, or whether dropping that flag (accepting a possible "unsigned package" prompt) gives better defence-in-depth. This is consistent with the Makefile's zypper invocation, so the behaviour is at least predictable, but it is worth a conscious decision.Prompt To Fix With AI
updater/src/install.rs, line 1676-1680 (link)--allow-unsigned-rpmfor local zypper installsThe updater builds RPMs locally without GPG signing. When zypper runs with
--non-interactiveand encounters an unsigned local package it cannot prompt the user, so it will reject the package and the auto-install fails silently on openSUSE. TheMakefilealready handles this correctly with--allow-unsigned-rpmfor packages from thedist/directory; the same flag is needed here.Prompt To Fix With AI
updater/src/install.rs, line 1687-1703 (link)--allow-unsigned-rpmfor locally-built packagesOn default openSUSE installations (
gpgcheck=on), zypper refuses to install unsigned RPMs without--allow-unsigned-rpm. The updater builds its own RPM from the extracted DMG and that package is not GPG-signed, so this command will fail at the privileged install step on openSUSE.The
Makefilealready handles this correctly by conditionally adding--allow-unsigned-rpmfor packages indist/. The same flag is needed here, and the corresponding unit test assertion would also need"--allow-unsigned-rpm"added to the expected args vec.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (8): Last reviewed commit: "chore: merge upstream warm-start updates" | Re-trigger Greptile