fix(launcher): honor OPENCODEX_BUN_PATH - #734
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:
📝 WalkthroughWalkthroughThe npm launcher now honors valid ChangesBun override launcher
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeTest
participant OcxLauncher
participant BunValidator
participant ProxyProcess
RuntimeTest->>OcxLauncher: Spawn with OPENCODEX_BUN_PATH
OcxLauncher->>BunValidator: Validate override
BunValidator-->>OcxLauncher: Select override or bundled Bun
OcxLauncher->>ProxyProcess: Start proxy with selected runtime
ProxyProcess-->>RuntimeTest: Report health and process identity
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3837865 to
fa455f7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa455f7798
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/ocx.mjs`:
- Around line 328-333: Update resolveBun so a non-empty OPENCODEX_BUN_PATH
override that fails isRealBunBinary emits a diagnostic identifying the rejected
path and reason before falling back to the bundled runtime; preserve the
existing behavior for valid overrides and unset values.
- Around line 295-313: Unify the Bun binary validation used by findBunBinary and
the corresponding validator in bun-runtime.ts by extracting the shared
existsSync/statSync size-check logic into a reusable helper, or add an explicit
cross-reference if sharing across these launch paths is impractical. Preserve
REAL_BUN_MIN_BYTES and the current false-on-error behavior, and update both
callers to use the single source of truth.
In `@tests/ocx-launcher-runtime.test.ts`:
- Around line 160-189: Ensure the spawned launcher is cleaned up even when
windowsProcessIdentity(launcher.pid) returns null or throws before ownedLauncher
is assigned. Add a fallback cleanup path keyed by the trusted launcher.pid,
using the existing killProxy process-tree cleanup mechanism, and preserve
ownedLauncher/ownedProxy cleanup for successfully identified processes.
- Around line 190-214: Update the cleanup logic in the test’s finally block so
cleanupErrors never throws over an exception from the main try block. Preserve
the cleanup attempts and ensure cleanup failures are reported without replacing
the primary test failure, using the surrounding test error-handling flow or
deferred reporting mechanism.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6726e791-aadd-4691-b359-e77acc7b5df8
📒 Files selected for processing (3)
bin/ocx.mjstests/ocx-launcher-runtime.test.tstests/ocx-launcher-source.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ocx-launcher-runtime.test.ts (1)
178-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isolatedLauncherEnv()duplicates the env/dir setup already inlined ineffectiveRuntime().
effectiveRuntime()(Lines 180-182, 192-194, 199-207) andisolatedLauncherEnv()(Lines 299-315) build the identicalopencodexHome/codexHome/grokHomedirectories and the identicalHOME/USERPROFILE/OPENCODEX_HOME/CODEX_HOME/GROK_HOME/OPENCODEX_BUN_PATHenv shape, in the same file, in the same diff. SinceisolatedLauncherEnvis defined right aftereffectiveRuntimebut never called from it, this reads like an extraction that should have replaced the inline block.♻️ Proposed fix: have `effectiveRuntime` reuse `isolatedLauncherEnv`
async function effectiveRuntime(override: string): Promise<string> { const root = mkdtempSync(join(tmpdir(), "ocx-launcher-runtime-")); - const opencodexHome = join(root, "opencodex"); - const codexHome = join(root, "codex"); - const grokHome = join(root, "grok"); let port: number | null = null; let launcher: ChildProcess | null = null; let launcherPid: number | null = null; let ownedLauncher: WindowsProcessIdentity | null = null; let ownedProxy: WindowsProcessIdentity | null = null; let runtimePath: string | undefined; let hasPrimaryError = false; let primaryError: unknown; try { - mkdirSync(opencodexHome, { recursive: true }); - mkdirSync(codexHome, { recursive: true }); - mkdirSync(grokHome, { recursive: true }); port = await freePort(); launcher = spawn("node", [BIN_OCX, "start", "--port", String(port)], { stdio: "ignore", windowsHide: true, - env: { - ...process.env, - HOME: root, - USERPROFILE: root, - OPENCODEX_HOME: opencodexHome, - CODEX_HOME: codexHome, - GROK_HOME: grokHome, - OPENCODEX_BUN_PATH: override, - }, + env: isolatedLauncherEnv(root, override), });(Move the
isolatedLauncherEnvfunction definition aboveeffectiveRuntimeor hoist it, since function declarations are hoisted in TS/JS this is safe either way.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ocx-launcher-runtime.test.ts` around lines 178 - 315, Update effectiveRuntime to reuse isolatedLauncherEnv for creating the home directories and constructing the launcher environment, passing the temporary root and override values instead of maintaining the duplicated inline setup. Preserve the existing launcher behavior and remove only the redundant directory and environment construction.
♻️ Duplicate comments (1)
bin/ocx.mjs (1)
293-305: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the Bun-binary validator into one shared plain-JS module instead of documenting a manual sync.
bin/ocx.mjs'sisRealBunBinary()/REAL_BUN_MIN_BYTESis a byte-for-byte copy ofsrc/lib/bun-runtime.ts's, and the only guard against drift is a test that checks a comment string exists — neither actually verifies the two thresholds/logic stay identical.
bin/ocx.mjs#L293-L305: extractisRealBunBinaryandREAL_BUN_MIN_BYTESinto a new plain.mjsfile (e.g.src/lib/bun-binary-validator.mjs) that uses onlynode:fs(existsSync/statSync) — no Bun-specific APIs — so it can be imported unmodified from this Node-executed launcher before any Bun binary is resolved; replace the local copy with an import from that module.tests/ocx-launcher-source.test.ts#L48-L50: once both files import the shared module, replace (or supplement) the comment-text assertion with a real parity check — e.g. import the shared module directly and assertbin/ocx.mjs/src/lib/bun-runtime.tsboth reference it, or assert the exportedREAL_BUN_MIN_BYTESvalue matches between call sites — so an actual threshold/logic change in one place is caught rather than just a missing comment.♻️ Proposed shared module sketch
// src/lib/bun-binary-validator.mjs import { existsSync, statSync } from "node:fs"; export const REAL_BUN_MIN_BYTES = 1_000_000; export function isRealBunBinary(path) { try { return existsSync(path) && statSync(path).size >= REAL_BUN_MIN_BYTES; } catch { return false; } }-function isRealBunBinary(path) { - try { - return existsSync(path) && statSync(path).size >= REAL_BUN_MIN_BYTES; - } catch { - return false; - } -} +import { isRealBunBinary } from "../src/lib/bun-binary-validator.mjs";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/ocx.mjs` around lines 293 - 305, Extract REAL_BUN_MIN_BYTES and isRealBunBinary into a shared Node-safe plain-JS module, update bin/ocx.mjs and src/lib/bun-runtime.ts to import it, and remove the duplicated implementations. In tests/ocx-launcher-source.test.ts lines 48-50, replace the comment-only assertion with a parity check that verifies both call sites use the shared validator or exported threshold, catching future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/ocx-launcher-runtime.test.ts`:
- Around line 178-315: Update effectiveRuntime to reuse isolatedLauncherEnv for
creating the home directories and constructing the launcher environment, passing
the temporary root and override values instead of maintaining the duplicated
inline setup. Preserve the existing launcher behavior and remove only the
redundant directory and environment construction.
---
Duplicate comments:
In `@bin/ocx.mjs`:
- Around line 293-305: Extract REAL_BUN_MIN_BYTES and isRealBunBinary into a
shared Node-safe plain-JS module, update bin/ocx.mjs and src/lib/bun-runtime.ts
to import it, and remove the duplicated implementations. In
tests/ocx-launcher-source.test.ts lines 48-50, replace the comment-only
assertion with a parity check that verifies both call sites use the shared
validator or exported threshold, catching future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f2cbc11-468b-4683-a4a6-38077860c5be
📒 Files selected for processing (4)
bin/ocx.mjstests/bun-runtime.test.tstests/ocx-launcher-runtime.test.tstests/ocx-launcher-source.test.ts
|
Addressed the follow-up review in 191e7e2. The launcher and Bun runtime now share one Node-safe validator (with a packaged |
191e7e2 to
72db422
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/ocx.mjs`:
- Around line 317-327: The overrideBunPath() logic in src/lib/bun-runtime.ts
must normalize OPENCODEX_BUN_PATH before validation. Trim the environment value,
resolve it against the current working directory, and pass the resolved path to
isRealBunBinary(), matching bin/ocx.mjs behavior; add a regression test covering
a relative override from a different launcher cwd.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56e6cd3a-9e1d-4cfc-8f02-3f400653d2d7
📒 Files selected for processing (7)
bin/ocx.mjssrc/lib/bun-binary-validator.d.mtssrc/lib/bun-binary-validator.mjssrc/lib/bun-runtime.tstests/bun-runtime.test.tstests/ocx-launcher-runtime.test.tstests/ocx-launcher-source.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/bun-runtime.ts`:
- Around line 51-52: Update isRealBunBinary, used by the override selection in
bun-runtime.ts, to reject oversized files unless they are regular files that are
readable, executable where required by the platform, and valid Bun binaries.
Ensure durableBunRuntime() falls back to the bundled runtime for invalid
overrides, and add a regression fixture covering large text, non-Bun executable,
and unreadable files.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f151c25-4ee1-4f92-9b1d-153272107629
📒 Files selected for processing (2)
src/lib/bun-runtime.tstests/bun-runtime.test.ts
| const resolved = resolve(value); | ||
| return isRealBunBinary(resolved) ? resolved : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reject large non-Bun or non-executable overrides before selecting them.
The shared validator in src/lib/bun-binary-validator.mjs only checks existence and file size. A large text file, non-Bun executable, or unreadable file can therefore pass Line 52; durableBunRuntime() will select it as the override and skip the bundled fallback. Add regular-file plus platform-appropriate executable/Bun validation, and cover this with a regression fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/bun-runtime.ts` around lines 51 - 52, Update isRealBunBinary, used by
the override selection in bun-runtime.ts, to reject oversized files unless they
are regular files that are readable, executable where required by the platform,
and valid Bun binaries. Ensure durableBunRuntime() falls back to the bundled
runtime for invalid overrides, and add a regression fixture covering large text,
non-Bun executable, and unreadable files.
|
LAND-AFTER — correcting an earlier triage call. This PR was previously slated as NEEDS-CHANGES on the grounds that it conflicted with On the substance, this resolves #721 the way it should be resolved. What happens next: nothing required from you. This lands after the release blockers, alongside the other queued PRs. CI context (shared across the current review round). |
|
Found a test failure while trial-merging this against current
This reproduces with the PR alone on top of The fix is to compare realpaths rather than the raw temp path. Either run the temp root through const real = realpathSync(join(launcherCwd, "relative-bun.exe"));Resolving at the Everything else looks good — the rest of the file passes, the merge is clean against current |
PR #734's launcher change is correct, but its test built the expected path from mkdtemp and then asserted against a value resolved through process.cwd(). On macOS /var is a symlink to /private/var, so the two forms of the same file compared unequal and the case failed on macos-latest only. Fixed at the source rather than at the assertion: realpathSync wraps the mkdtemp root, so every case in the file resolves consistently instead of just the one that happened to break. 7 pass / 1 fail before, 8 pass / 0 fail after.
|
Closing — this is already merged. The macOS test failure that held it up is also fixed: #721 is closed with that evidence. |
Summary
OPENCODEX_BUN_PATHto an absolute path and prefer it before the npm launcher probes bundled Bun..mjsmodule used by both the npm launcher and Bun runtime code.Fixes #721.
Why
Durable service and Codex-shim artifacts already use
durableBunRuntime(), which gives a valid explicit override priority. The npm launcher maintained a separate resolver that only considered bundled Bun. Coldocx gui/ direct starts—and the detached direct-start fallback after a service refresh failure—could therefore downgrade the effective proxy runtime to the bundled version.The launcher now applies the same validation contract as the shared resolver before it touches the bundled dependency. Both launch paths import one Node-safe binary validator, avoiding threshold or error-handling drift.
Behavioral regression
Cross-platform launcher tests invoke
node bin/ocx.mjs --versionwith bare relative overrides and verify both absolute resolution and a path-redacted invalid-override warning. The Windows test launchesnode bin/ocx.mjs starton isolated homes and free ports, then verifies the actual health PID through CIM:ExecutablePathis the overrideExecutablePathis bundled BunCleanup records the spawned launcher PID immediately, retries CIM identity capture, uses the still-live owned child as a bounded tree-cleanup fallback, preserves the primary test failure, probes the owned port for a lingering OpenCodex proxy, and confirms owned processes and fixtures are gone. The runtime and relative-path tests share one isolated launcher environment builder.
Verification
node --check bin/ocx.mjsnode --check src/lib/bun-binary-validator.mjs1.3.14 (0d9b296a)1.4.0-canary.1 (b22e0e6d0)tests/ocx-launcher-source.test.ts,tests/ocx-launcher-runtime.test.ts,tests/bun-runtime.test.ts,tests/install-scripts.test.tsbun x tsc --noEmit: passed on both runtimesbun scripts/privacy-scan.ts: passed on both runtimesgit diff --checknpm pack --dry-run --ignore-scripts --json: shared validator implementation, declaration, and launcher are includedgui/files changedFull-suite context
A CI-like monolithic Windows run was attempted with the repository's Bun 1.3.14 and a 20-minute command budget. Bun itself hit an internal assertion and crashed after 83.4 seconds while the suite was still in
api-storage-policy.test.ts, before reaching the changed launcher tests. This is recorded as a runtime crash, not a passing run or a test assertion failure. A post-crash audit found no worktree-linked Node/Bun survivors and no recent launcher fixtures.Current
devhas a green cross-platform CI baseline. Cross-platform CI for this fork PR still requires repository approval.Checklist
Summary by CodeRabbit
New Features
OPENCODEX_BUN_PATH, including support for relative paths.Bug Fixes
Tests