fix(update): preserve Bun install ownership#3259
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: 552fe97f9b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (usesNativeTransaction && !ownership) { | ||
| console.log(packageManagerOwnershipError()); | ||
| return { status: 'failed', currentVersion: null, latestVersion: null }; |
There was a problem hiding this comment.
When the passive startup update path uses the native dependencies and ownership cannot be resolved (for example a local checkout, an unsupported/global install, or ambiguous npm+Bun evidence), this branch returns before writeUpdateState runs. Because maybeCheckAndPromptUpdate only throttles via that state file, every subsequent launch re-enters ownership detection and logs this failure instead of waiting for CHECK_INTERVAL_MS; record the advisory check or defer ownership resolution until an install is actually needed.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| return spawnInstalledSetupRefresh(cliEntry, cwd); | ||
| return spawnInstalledSetupRefresh(cliEntry, cwd, spawnSync, ownership?.manager === 'bun' ? ownership.bunCommand : process.execPath); |
There was a problem hiding this comment.
For a Bun-owned global install whose omx bin resolves to this package’s #!/usr/bin/env node entrypoint, the running CLI is still on Node, but this line switches the setup refresh to ownership.bunCommand. The verification manifest added in this commit says Node is the only runtime contract, so a Bun stable update can install successfully and then refresh setup under an unsupported runtime; keep Bun as the package manager but invoke the updated CLI with Node for setup refreshes.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent exact-head red-team verdict for Hosted CI is green, but the current head is not merge-ready:
Repair every finding, add focused regressions, push a new head, wait for fresh CI, and obtain a fresh independent exact-head review. The GitHub API disallows a PR author from submitting a — |
|
Repair push: exact head
Local verification passed on this head: build, focused update/postinstall tests, no-unused, lint, full — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66515ab0b7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // cmd.exe can still resolve and run npm from the user's configured PATH. | ||
| return spawnProcess('cmd.exe', ['/d', '/s', '/c', 'npm', ...args], options); | ||
| function stableInstallArgs(ownership: PackageManagerOwnership, installSource: string): string[] { | ||
| return ['install', '--global', '--ignore-scripts', '--no-audit', '--no-progress', '--prefix', ownership.npmPrefix!, installSource]; |
There was a problem hiding this comment.
When the frozen owner is Bun, this helper is still passed to spawnProcess(ownership.bunCommand, ...), so stable immediate updates invoke Bun with npm-only flags (--prefix and --no-audit) while the log says bun add -g. I checked bun install --help: it supports --global, --ignore-scripts, and --no-progress, but not those npm flags; a parser check treats the --prefix value as another package and fails, so Bun-owned stable updates cannot complete. The deferred worker duplicates the same argv and needs the same fix.
Useful? React with 👍 / 👎.
| ...(existingStamp?.package_manager === "npm" || existingStamp?.package_manager === "bun" | ||
| ? { package_manager: existingStamp.package_manager } | ||
| : {}), |
There was a problem hiding this comment.
If a user switches package managers with a real global reinstall (for example, a Bun-stamped install followed by npm install -g oh-my-codex), this postinstall path writes the new version but preserves package_manager: "bun". Later resolvePackageManagerOwnership trusts that stamp as the only manager to probe, so the now npm-owned executable cannot resolve ownership and omx update keeps failing until the user manually edits the stamp. Drop stale provenance here unless the current lifecycle/install has been validated as the same manager.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES Exact review target: PR #3259 head The independent red-team report is blocking. Deduplicated transaction-boundary findings:
Do not merge. Repair by transaction boundary, manager semantics, and worker integrity; add deterministic regression coverage; then push a new head, run fresh CI, and request a fresh independent exact-head verdict. — |
|
Red-team transaction-boundary repairs are pushed at exact head The repair adds executable-backed/canonical manager authority, a minimal transaction environment, Bun configured-bin semantics without npm-prefix reuse, exclusive staged deferred-worker payload validation with pre/post ownership checks, fail-closed pnpm/Yarn diagnostics, and deterministic regression coverage. Local verification on this head passed: focused update/postinstall tests, build, no-unused, lint, full — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d75ce0b6d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const npmExecPath = process.env.npm_execpath; | ||
| if (!npmExecPath || !isAbsolute(npmExecPath)) return null; |
There was a problem hiding this comment.
For a normal npm install -g user running omx update, the package bin is launched directly by Node, not from an npm lifecycle script, so process.env.npm_execpath is usually unset. This early return leaves resolvePackageManagerOwnership() with no npm candidate, and the native update path aborts before any explicit or passive update can run for the default supported npm install; resolve or persist the npm command without requiring lifecycle-only env.
Useful? React with 👍 / 👎.
| if (result.error || result.status !== 0) throw new Error(String(result.stderr || result.error?.message || 'controller install failed')); | ||
| const cliEntry = await validateOwnership(payload.ownership); | ||
| if (!cliEntry) throw new Error('Frozen manager, package root, or bin ownership validation failed after update.'); | ||
| const setup = spawnSync(process.execPath, [cliEntry, ...payload.setupArgs], { cwd: payload.cwd, env: payload.ownership.environment, stdio: 'inherit', windowsHide: true }); |
There was a problem hiding this comment.
When the deferred worker reaches setup refresh, this replaces the worker environment with the minimal ownership snapshot. The scheduler intentionally starts the worker with OMX_SKIP_NATIVE_AGENT_REFRESH=1, and users may also rely on CODEX_HOME; both are dropped here, so background updates can refresh native agents and/or write setup output to the default Codex home instead of the active one. Merge the setup guard and user config environment into the refresh process.
Useful? React with 👍 / 👎.
| async function ownerOnlyStage(stage: string): Promise<boolean> { | ||
| try { | ||
| const stat = await lstat(stage); | ||
| return stat.isDirectory() && !stat.isSymbolicLink() && (stat.mode & 0o077) === 0 && (typeof process.getuid !== 'function' || stat.uid === process.getuid()); |
There was a problem hiding this comment.
On Windows, Node does not model Unix owner/group/other permission bits in the way this POSIX check expects, so after the scheduler chmods the temp stage to 0700, stat.mode can still expose group/other bits and the worker rejects the staged transaction before installing. Gate this owner-only bit check to POSIX or use a Windows-appropriate ACL/owner validation so deferred updates can run on Windows.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent exact-head review of Fresh CI is green, but the terminal red-team review found a blocking ownership bootstrap regression: Do not merge. The owning lane must provide a canonical, non-PATH, normal-launch executable authority source for npm and Bun, add normal-launch regression coverage, then repeat CI and independent review. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b1cd7751d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /** Only accept Bun provenance from the current runtime, lifecycle launcher, or configured install root, never PATH. */ | ||
| export function resolveBunCommand(): string | null { | ||
| const candidates = [process.execPath, process.env.npm_execpath, process.env.BUN_INSTALL && join(process.env.BUN_INSTALL, 'bin', 'bun')] |
There was a problem hiding this comment.
When OMX is launched under Node from a Bun-owned global install on Windows, neither process.execPath nor npm_execpath points at Bun, so the BUN_INSTALL fallback is the path that has to recover ownership. Bun’s Windows installer writes <BUN_INSTALL>\bin\bun.exe, but this candidate hard-codes bin/bun; realpathSync does not perform PATHEXT lookup, so Bun ownership is never established and omx update fails before the install. Add the .exe candidate on win32 here and in the installed-root fallback.
Useful? React with 👍 / 👎.
|
|
||
| function transactionEnvironment(source: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv { | ||
| const environment: NodeJS.ProcessEnv = {}; | ||
| for (const key of ['HOME', 'PATH', 'TMPDIR', 'TEMP', 'TMP', 'SystemRoot', 'USERPROFILE']) { |
There was a problem hiding this comment.
For Bun installs that set BUN_INSTALL_GLOBAL_DIR or BUN_INSTALL_BIN to move global packages/bins, ownership is resolved using the current environment but the frozen transaction drops those variables here. The later bun pm bin -g validation and bun add --global then run with Bun’s default global locations instead of the frozen bunGlobalBin/packageRoot, causing deferred updates to fail validation and immediate updates to install somewhere other than the owned install. Include Bun’s documented global install env overrides in the transaction environment.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent current-head review for Fresh CI is green, but the review remains blocking: the current transaction still has unresolved ownership/deferred-worker/Windows/script-suppression contract gaps in the 14-file diff. The normal-launch fix addressed one prior P1 only; it does not establish a terminal — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7404931a33
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return ownership?.manager === 'bun' | ||
| ? { ok: false, stderr: 'Bun dev updates are not yet supported' } | ||
| : runDevGlobalUpdate(spawnProcess, resolvedPlatform); |
There was a problem hiding this comment.
When omx update --dev runs from a validated npm-owned install, this branch drops the ownership transaction and runDevGlobalUpdate falls back to PATH-based npm commands, including a global install without the frozen --prefix or environment. On machines with a different npm earlier in PATH or a custom global prefix, the dev tarball can be installed into the wrong global root even though ownership was just validated; pass the frozen npm command/prefix into the dev install path or fail closed.
Useful? React with 👍 / 👎.
| if (result.error || result.status !== 0) throw new Error(String(result.stderr || result.error?.message || 'controller install failed')); | ||
| const cliEntry = await validateOwnership(payload.ownership); | ||
| if (!cliEntry) throw new Error('Frozen manager, package root, or bin ownership validation failed after update.'); | ||
| const setup = spawnSync(process.execPath, [cliEntry, ...payload.setupArgs], { cwd: payload.cwd, env: { ...payload.ownership.environment, [SKIP_NATIVE_AGENT_REFRESH_ENV]: '1' }, stdio: 'inherit', windowsHide: true }); |
There was a problem hiding this comment.
When the deferred worker succeeds here, the install was invoked with --ignore-scripts, so the package postinstall that normally refreshes install-state.json never runs, and this worker does not write the immediate-update stamp after setup completes. After a background update, installed_version and setup_completed_version remain on the old version, leaving later setup/version checks to operate on stale metadata despite a successful update; write the same success stamp before exiting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4ce5e8686
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const executable = dependencies.realpath(dependencies.currentExecutable); | ||
| const packageRoot = dependencies.realpath(dependencies.currentPackageRoot); | ||
| const shimName = dependencies.platform === 'win32' ? 'omx.cmd' : 'omx'; | ||
| if (dependencies.realpath(join(bunGlobalBin, shimName)) !== executable) return null; |
There was a problem hiding this comment.
For Windows Bun-owned installs where omx is launched through an omx.cmd shim, currentExecutable is the JS entry under install/global/node_modules, while join(bunGlobalBin, 'omx.cmd') is the shim file itself. This equality therefore returns null even after a Bun command is found, so resolvePackageManagerOwnership() fails and both passive and explicit Bun updates abort before install. Validate the shim target, or accept the package-root evidence, instead of requiring the .cmd file to realpath to the JS entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a15276987
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (path.basename(packageRoot) !== 'oh-my-codex' || !isPathWithin(executable, packageRoot, dependencies.platform)) return null; | ||
| if (dependencies.platform === 'win32') { | ||
| // A Windows .cmd shim is a separate file, so its canonical location is the ownership evidence. | ||
| if (!isPathWithin(shim, bunGlobalBin, dependencies.platform)) return null; |
There was a problem hiding this comment.
When BUN_INSTALL exists on Windows, this branch treats any omx.cmd located in Bun's global bin as sufficient ownership evidence; it no longer ties that shim to currentExecutable nor requires packageRoot to be under <BUN_INSTALL>\install\global\node_modules. A user running a local checkout or another install named oh-my-codex while ~\.bun\bin\omx.cmd exists will be classified as Bun-owned, so omx update updates Bun's global package but refreshes/stamps the local package. Require the package root to match the Bun global install layout or validate the .cmd target before returning ownership.
Useful? React with 👍 / 👎.
| const canonicalStage = realpathSync(stage); | ||
| const canonicalPayload = realpathSync(payloadPath); | ||
| const payloadStat = lstatSync(payloadPath); | ||
| if (canonicalPayload !== join(canonicalStage, 'transaction.json') || payloadStat.isSymbolicLink() || !payloadStat.isFile() || (payloadStat.mode & 0o077) !== 0) throw new Error('Deferred update payload is not a canonical owner-only staged file.'); |
There was a problem hiding this comment.
Separate from the worker validation, scheduling a deferred update on Windows still reaches this POSIX mode-bit check immediately after creating transaction.json. Node cannot reliably represent owner/group/other bits on Windows, so a file created with mode: 0o600 can still report group/other bits and this throws before the worker is spawned; gate the (mode & 0o077) test to POSIX or use Windows ACL validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f109a17ca4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function transactionEnvironment(source: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv { | ||
| const environment: NodeJS.ProcessEnv = {}; | ||
| for (const key of ['HOME', 'PATH', 'TMPDIR', 'TEMP', 'TMP', 'SystemRoot', 'USERPROFILE', 'BUN_INSTALL']) { |
There was a problem hiding this comment.
When npm ownership is resolved, npmPrefix is computed with the caller's full environment, but this snapshot drops env-only npm prefix settings such as PREFIX or npm_config_prefix. The deferred worker later validates with npm prefix -g using only ownership.environment, so installs that were proven under a custom env prefix fall back to npm's default prefix and the worker rejects the transaction before updating; preserve the prefix config or validate with the frozen prefix.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent exact-head review for REST CI is terminal green, but the current ownership transaction is not merge-ready. The review found remaining standard Windows npm/deferred compatibility gaps, Bun installation authorization/target ambiguity, and deferred setup/finalization boundary defects. Do not merge. Repair the remaining manager/deferred transaction semantics in the owning lane, add deterministic regressions, then run fresh CI and another current-head review. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ee7c1eb1c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const path = platformPath(dependencies.platform); | ||
| const packageRoot = dependencies.realpath(dependencies.currentPackageRoot); | ||
| if (path.basename(packageRoot) !== 'oh-my-codex') return null; | ||
| const script = dependencies.realpath(path.join(path.dirname(packageRoot), 'npm', 'bin', 'npm-cli.js')); |
There was a problem hiding this comment.
On the standard Windows npm layout, npm documents global packages under {prefix}\node_modules (npm folders), so a normal omx.cmd launch has packageRoot like %AppData%\npm\node_modules\oh-my-codex and no npm_execpath; the npm CLI that shipped with Node is under the Node installation, not the package-root sibling constructed here. The new fallback still hard-codes that sibling path, so realpath fails before npm root -g/prefix -g can run and omx update reports an ownership failure for supported Windows npm installs; derive or persist the npm CLI from the Node install instead.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent terminal review of exact head REST CI is terminal green, but the current review found three HIGH transaction/Windows defects and one MEDIUM provenance defect. Frozen commands, script suppression, Node setup refresh, deferred finalization, and Bun-dev fail-closed behavior improved materially, but the remaining defects still block merge. Preserve all findings, repair transaction/Windows/provenance behavior in the owning lane, add deterministic regressions, and repeat fresh CI plus an exact-head review. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fe6ec8e18
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!resolved.currentExecutable) return null; | ||
| const stamp = await resolved.readInstallStamp(); | ||
| const managers: PackageManager[] = stamp?.package_manager ? [stamp.package_manager] : ['npm', 'bun']; | ||
| if (!stamp?.package_manager && hasAmbiguousBunShimEvidence(resolved)) return null; |
There was a problem hiding this comment.
For a POSIX Bun global install that has no package_manager stamp yet (fresh installs, or users upgrading from older stamps), Bun's global omx shim realpaths to the current package entry, so this guard returns null before the later Bun resolver can validate BUN_INSTALL, the package root, and the Bun command. Since runPostinstall only preserves an existing manager stamp and does not infer Bun on first install, these otherwise valid Bun-owned installs can never pass ownership resolution for omx update unless the user manually edits the stamp; let the Bun candidate be evaluated and rely on the single-candidate check instead of treating matching shim evidence as an immediate ambiguity.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent terminal review of exact head REST CI is terminal green, but the current head remains non-merge-ready. The review acknowledges material improvements to command freezing, script suppression, Node-only setup refresh, deferred finalization, manifest validation, and Bun-dev fail-closed behavior, while retaining blocking transaction/provenance compatibility findings. Do not merge. Preserve all findings, repair with deterministic regressions, then repeat fresh CI and an independent exact-head review. — |
|
REQUEST_CHANGES — independent terminal review of exact head REST CI is terminal green, and the review confirms improvements to command freezing, script suppression, Node setup refresh, deferred finalization, manifest checks, and Bun-dev fail-closed behavior. Core transaction/provenance compatibility findings remain blocking. Do not merge. Preserve and repair the terminal findings with deterministic coverage, then repeat fresh CI and an independent exact-head review. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f31c698da
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function transactionEnvironment(source: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv { | ||
| const environment: NodeJS.ProcessEnv = {}; | ||
| for (const key of ['CODEX_HOME', 'HOME', 'PATH', 'TMPDIR', 'TEMP', 'TMP', 'SystemRoot', 'USERPROFILE', 'BUN_INSTALL']) { |
There was a problem hiding this comment.
When an update runs from a session that set the documented OMX_ROOT/OMX_STATE_ROOT (README.md:170-192), this allowlist drops those variables, and both immediate and deferred setup refreshes spawn the updated CLI with only ownership.environment. setup() then creates omxStateDir/omxPlansDir from omxRoot(projectRoot), so the refresh falls back to <cwd>/.omx and can create or update state in the wrong checkout instead of the active boxed root; include the boxed root variables in the frozen environment.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent terminal review for exact head The review confirms improvements to frozen commands, script suppression, Node-only setup refresh, deferred finalization, and Bun-dev fail-closed behavior. Seven P1 ownership-resolution findings remain, including supported Windows npm installs and fresh Bun installs failing ownership resolution, plus Windows deferred transaction compatibility gaps. Do not merge. Repair only concrete transaction findings with deterministic regressions, then require fresh CI and a new exact-head review. — |
|
REQUEST_CHANGES — independent terminal review of exact head REST CI is terminal green. The review confirms the branch is materially safer: commands are mostly frozen, stable/global installs suppress lifecycle scripts, Bun dev fails before effects, setup remains on Node, and the deferred worker has integrity fingerprints. Remaining ownership/transaction defects still block merge. Do not merge; preserve the terminal findings, repair with deterministic regressions, then repeat fresh CI and exact-head review. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd6708c23
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const packageRoot = dependencies.realpath(path.join(root, 'oh-my-codex')); | ||
| const currentPackageRoot = dependencies.realpath(dependencies.currentPackageRoot); | ||
| const executable = dependencies.realpath(dependencies.currentExecutable); | ||
| return pathsEqual(packageRoot, currentPackageRoot, dependencies.platform) && isPathWithin(executable, packageRoot, dependencies.platform) ? packageRoot : null; |
There was a problem hiding this comment.
When the global oh-my-codex entry is a symlink to a checkout outside npm root -g (for example npm link), this returns the checkout realpath as npm-owned because it only checks that the running executable is inside the symlink target. The later frozen validator requires the package root to be under globalInstallRoot, so deferred updates are scheduled but the worker aborts, while immediate updates run npm install -g first and then fail setup refresh against the stale outside package root. Reject package roots outside the global root during ownership resolution.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent terminal review of exact head The exact-head Actions run — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fa1ae9623
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function transactionEnvironment(source: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv { | ||
| const environment: NodeJS.ProcessEnv = {}; | ||
| for (const key of ['CODEX_HOME', 'HOME', 'PATH', 'TMPDIR', 'TEMP', 'TMP', 'SystemRoot', 'USERPROFILE', 'BUN_INSTALL']) { |
There was a problem hiding this comment.
When a user relies on env-only npm configuration such as npm_config_registry, npm_config_userconfig with token substitution, or proxy/CA settings, this allowlist strips those variables before the frozen npm install/validation because the child process env is exactly ownership.environment. The previous updater inherited process.env, so these supported setups now make omx update contact the wrong registry or fail authentication/proxy setup; preserve the selected manager's registry/network config in the frozen transaction instead of reducing it to this list.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES — independent terminal review of exact head Exact-head CI run — |
|
Repair update for exact head The deferred payload boundary now rejects malformed, incomplete, noncanonical-filename, and foreign owner-only staging inputs without deleting unrelated owner-only state. Scheduler namespace and canonical transaction filename checks are covered by deterministic regressions. Local verification passed: build, focused update ownership suite (76/76), no-unused typecheck, and lint. The prior 9fa — |
|
REQUEST_CHANGES — independent exact-head review for Three P1 findings block merge:
REST CI is green but does not override these ownership invariants. Repair all three with deterministic regressions, then require fresh CI and a fresh exact-head review. — |
|
REQUEST_CHANGES — independent exact-head red-team review for Hosted CI is terminal green and local verification is clean, but one P1 ownership regression blocks merge. Verification evidence (exact head, this worktree)
P1 — stale Bun provenance in the install stamp permanently wedges self-update after a supported manager switchContract locations (exact head):
Reproduction sequence (proven deterministically against fresh dist):
Recovery-path audit (all dead ends):
Impact: after a legitimate, supported npm↔Bun manager switch, the affected installation can never self-update again, and OMX's own recovery instruction actively reproduces the wedge. Failure being closed prevents cross-install damage but does not make permanent loss of the self-update capability acceptable — this is a core ownership regression in the exact workflow this PR exists to protect. Required fix:
Repair this finding with deterministic regressions, then require fresh CI and a fresh exact-head review. Do not merge at — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfccdb0f09
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const path = platformPath(dependencies.platform); | ||
| const bunInstallRoot = dependencies.realpath(dependencies.bunInstallRoot); | ||
| const globalInstallRoot = dependencies.realpath(path.join(bunInstallRoot, 'install', 'global', 'node_modules')); | ||
| const packageRoot = matchesCurrentInstall(globalInstallRoot, dependencies); |
There was a problem hiding this comment.
When oh-my-codex is registered with bun link, Bun creates <BUN_INSTALL>/install/global/node_modules/oh-my-codex as a symlink to the checkout and the global omx shim resolves into that outside tree; I confirmed this with bun link after checking bun link --help describes registering the current directory as a linkable package. matchesCurrentInstall follows that symlink and returns the external checkout here, but resolveBunOwnership never requires the returned packageRoot to remain under globalInstallRoot, so immediate omx update can run bun add --global against the Bun global install and only fail later during setup validation against the linked checkout. Reject Bun-linked roots during ownership resolution, like the later validator already does, before any install is spawned.
Useful? React with 👍 / 👎.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
MERGE_READY — independent exact-head red-team review for bfccdb0f09cea366e686c58bdce73fdefe5519b5 against origin/dev 299f31ad824409e5cae0015d982de41aa1b09552 (merge-base == origin/dev; 20 linear commits).
Hosted CI at exact head is terminal green (runs 29940854714, 29940851946) and is treated as evidence, not proof. Independent local verification at the exact head is also clean, and the 9ced9f0d stale-Bun-stamp wedge is verifiably fixed in both directions.
Verification evidence (exact head, this worktree)
git rev-parse HEAD=bfccdb0f09cea366e686c58bdce73fdefe5519b5; worktree clean (no tracked-file edits).- Fresh
npm ci(lockfile-preserved) +npm run build: exit 0. - Focused compiled suites:
dist/cli/__tests__/update.test.js79/79 pass,dist/scripts/__tests__/postinstall.test.js8/8 pass. npm run check:no-unused: exit 0.npm run lint(biome, 778 files): clean.- Deterministic runtime repro against freshly built
dist(dependency-injected, no repo files touched): 10/10 scenarios pass, including one real Bun 1.3.14 end-to-end lane.
Prior blocker disposition
9ced9f0d P1 (stale Bun stamp → permanent self-update wedge) — FIXED.
package-manager-ownership.ts:351-352now treats the stamp as probe priority only (bunstamp →['bun','npm'], else['npm','bun']); every candidate requires live ownership proof and resolution still returns null unless exactly one manager validates. Runtime proof: stale Bun stamp + npm-owned install resolvesnpm(including with Bun fully removed: noBUN_INSTALL, no bun binary); stale npm stamp + Bun-owned install resolvesbun. The npm probe spy records the independent live validation the9cedreview found suppressed.postinstall.tsno longer carriesexistingStamp.package_managerinto a bumped stamp, so a legitimate bumped manager replacement clears contradicting provenance instead of perpetuating it (runtime proof: npm-lifecycle bump over a Bun stamp writespackage_manager: undefinedwhile preservingsetup_completed_version).- Recovery guidance no longer re-wedges: the same-version reinstall (
noop-same-version) leaves the stamp untouched but resolution recovers on live evidence; the bumped reinstall clears the stamp and resolves. Both paths were executed to a resolvable owner in the repro.
280963da three P1s — intact at this head. npm resolves from the selected installed OMX package/root (incl. the Windows Node-layout fallback, resolveInstalledNpmCommand); Bun's configured global root/bin is frozen pre-mutation via bun pm bin -g under the frozen transaction environment; validatePackageManagerOwnership revalidates root/bin/shim after install before setup refresh and stamp publication in both the immediate path and the deferred worker (target-proof: a missing/mismatched shim fails revalidation against the live Bun layout).
Deferred payload/transaction integrity (280963da) — intact. Worker self-digest, payload digest, canonical transaction.json staging, owner-only omx-update- stage namespace, and foreign owner-only directory preservation are all covered by deterministic compiled regressions (5/5 worker tests pass) and re-read at this head.
Adversarial target results
- Fallback uniqueness: a stamped-manager mismatch falls back only when the alternate manager uniquely validates the exact current package root; both-validate stays fail-closed for unstamped/npm/bun stamps (3/3 in repro), no-owner fails closed.
- Symmetry: Bun→npm and npm→Bun manager switches both recover without stamp deletion or manual intervention.
- Windows: win32 path canonicalization (
platformPath, case-insensitivepathsEqual),.cmdshim containment semantics, and npm CLI selection from the selected root before the Node-runtime sibling are covered by deterministic tests; POSIX shim requires canonical equality with the installed CLI entry. - Bun dev: rejected before any effect with the owner-approved unsupported message; no spawn occurs.
- npm CLI selection: lifecycle
npm_execpathis canonicalized and executed throughprocess.execPath; normal-launch resolution derives npm from the selected installed root and fails closed when no npm CLI exists there. - Tests reflect implementation: the adjusted assertions (probe-priority stamp, independent live npm validation, cleared bump provenance,
runNpmCommandcontract regex) match the shipped semantics rather than the pre-fix narrowing assumption.
Residual notes (non-blocking)
- A permanently stale stamp (same-version reinstall noop) persists but is now cosmetic: it only orders probes and never authorizes or suppresses a manager. A subsequent successful update rewrites correct provenance.
- Bun ownership detection requires
BUN_INSTALL(or Bun executable provenance) in the environment; without it the path fails closed with the actionable reinstall guidance, which now actually recovers. This is the accepted fail-closed trade-off, not a cross-install risk. - PR body still says postinstall "preserves an existing Bun owner stamp"; the head now clears stale provenance on bumped replacements. Description drift only — the code behavior is the correct one.
Evidence denominator: exact-head worktree + fresh npm ci/build + 87 focused compiled tests (79 update + 8 postinstall) + no-unused + lint + 10/10 runtime manager-switch repro scenarios (1 with real Bun 1.3.14) + exact-head hosted CI green. Merge is unblocked at bfccdb0f09cea366e686c58bdce73fdefe5519b5.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
1 participant
Summary
Implements the narrow #3257 package-manager ownership path while keeping OMX on Node:
--devbefore effects with the owner-approved unsupported message;Verification
npm run buildnpm run check:no-unusednpm run lintnpm testnpm run smoke:packed-installRefs #3257
—
[repo owner's gaebal-gajae (clawdbot) 🦞]