fix(patches): align Computer Use settings resolution and quit-guard insertion#451
fix(patches): align Computer Use settings resolution and quit-guard insertion#451ryan-mt wants to merge 1 commit into
Conversation
…nsertion Two core patch fixes: 1. computerUseUiSettingsAppId resolved CODEX_APP_ID before CODEX_LINUX_APP_ID - the inverse of every other resolver in the repo, including the injected codexLinuxSettingsAppId runtime that persists the codex-linux-computer-use-ui-enabled flag. With side-by-side app identities (e.g. make build-dev-app) the build-time reader checked a different settings.json than the one the running app writes, so the persisted Computer Use UI opt-in was silently ignored. Also honour CODEX_LINUX_SETTINGS_FILE like the runtime settings path resolver. 2. applyLinuxQuitGuardPatch's last-resort fallback prepended the quit guard at byte 0 of the main bundle. The .vite/build bundles start with "use strict"; prepending a statement before the directive silently demotes the whole bundle to sloppy mode. Insert after the directive instead (same technique the other helper-injection patches use). The "could not find insertion point" warning was also dead code behind an unreachable condition; it now actually fires when no insertion anchor exists.
| const previousXdg = process.env.XDG_CONFIG_HOME; | ||
| const previousAppId = process.env.CODEX_APP_ID; | ||
| const previousLinuxAppId = process.env.CODEX_LINUX_APP_ID; | ||
| const previousSettingsFile = process.env.CODEX_LINUX_SETTINGS_FILE; |
There was a problem hiding this comment.
This is an auto review done by revuto.
This saves the prior CODEX_LINUX_SETTINGS_FILE, but the finally block never restores it. The new test below sets this env var to a file under tempHome, and line 4104 then removes that directory, leaving process.env.CODEX_LINUX_SETTINGS_FILE pointing at a deleted file until another helper call clears it; if the caller had the variable set before the suite, the first withIsolatedHome call also permanently drops it. Please restore/delete it in the same way as HOME, XDG_CONFIG_HOME, and the app-id vars.
|
Blocking issue: withIsolatedHome() saves and deletes CODEX_LINUX_SETTINGS_FILE but does not restore it in the finally block. The new test can leave process.env.CODEX_LINUX_SETTINGS_FILE pointing at a removed temp file, or lose a value that existed before the test suite. Please restore/delete CODEX_LINUX_SETTINGS_FILE the same way HOME, XDG_CONFIG_HOME, CODEX_APP_ID, CODEX_LINUX_APP_ID, and the Computer Use flag are restored. |
|
Closing this for now. If you'd like to come back to it, please feel free to reopen. |
3 participants
What changed
Two core ASAR patch fixes.
computer-use.jsresolved the settings identity inversely to the runtime that writes the flag.computerUseUiSettingsAppIdusedCODEX_APP_ID || CODEX_LINUX_APP_ID, while every other resolver in the repo — including the injectedcodexLinuxSettingsAppId()runtime that persistscodex-linux-computer-use-ui-enabled, pluscodexLinuxWrapAppId,codexLinuxReadAloudSettingsAppId,bootstrap-wizard.sh,apply-pending.sh, and the updater's documented resolution order (updater/src/config.rs) — usesCODEX_LINUX_APP_ID || CODEX_APP_ID. With side-by-side app identities (e.g.make build-dev-app, where the repo's own tests set the two vars to different values) the build-time reader checked a differentsettings.jsonthan the one the running app writes, silently ignoring the persisted Computer Use UI opt-in. The reader now also honoursCODEX_LINUX_SETTINGS_FILE, like the runtimecodexLinuxSettingsPath()it mirrors.applyLinuxQuitGuardPatch's last-resort fallback de-stricted the main bundle, and its drift warning was dead code. The fallback prepended the quit-guard helper at byte 0; the.vite/buildbundles start with"use strict";, and a statement before the directive demotes it to a plain expression, switching the whole bundle to sloppy mode. The helper is now inserted after a leading directive (the same technique the other helper-injection patches use). The "could not find insertion point" warning was guarded by a condition that the precedingreturnmade unreachable; it now actually fires when no anchor exists.Source-of-truth files
scripts/patches/computer-use.jsscripts/patches/main-process.jsscripts/patch-linux-window-ui.test.js(new coverage)Validation (Ubuntu/WSL, node 22)
node --test scripts/patch-linux-window-ui.test.js: 222/222 pass (218 baseline + 4 new: LINUX-first app-id precedence,CODEX_LINUX_SETTINGS_FILEoverride, use-strict-preserving fallback insertion, warning on missing anchor)node --test linux-features/*/test.js: all passmain: 218/218 pass, so no regressionsNotes / limitations