fix(launcher): harden override handling

luvs01 · luvs01 · commit 68e6d5c354fc · 2026-07-30T17:04:13.000+09:00
diff --git a/bin/ocx.mjs b/bin/ocx.mjs
@@ -290,7 +290,9 @@ function bunBinDir() {
 // The `bun` package ships a tiny ASCII placeholder at bin/bun.exe until its
 // postinstall downloads the real ~60MB binary. --ignore-scripts / pnpm leave
 // the ~450-byte stub in place, which is NOT executable (ENOEXEC). A size gate
-// cleanly distinguishes the stub from a real binary on every platform.
+// cleanly distinguishes the stub from a real binary on every platform. Keep
+// this Node-safe copy aligned with src/lib/bun-runtime.ts: the launcher cannot
+// import Bun-native TypeScript until after it has resolved a Bun executable.
 const REAL_BUN_MIN_BYTES = 1_000_000;
 const BUN_OVERRIDE_ENV = "OPENCODEX_BUN_PATH";
 
@@ -329,7 +331,13 @@ function resolveBun() {
   // Keep direct npm-launcher starts aligned with durable service/shim installs:
   // a valid explicit runtime must win even when the bundled dependency exists.
   const override = process.env[BUN_OVERRIDE_ENV]?.trim();
-  if (override && isRealBunBinary(override)) return override;
+  if (override) {
+    const overridePath = resolve(override);
+    if (isRealBunBinary(overridePath)) return overridePath;
+    console.error(
+      `opencodex: ${BUN_OVERRIDE_ENV} is missing, unreadable, or not a complete Bun binary; falling back to the bundled runtime.`,
+    );
+  }
 
   let bunDir;
   try {
diff --git a/tests/bun-runtime.test.ts b/tests/bun-runtime.test.ts
@@ -69,11 +69,18 @@ describe("bundledBunPath / durableBunPath", () => {
   });
 
   it("durableBunPath returns the bundled path when present, else process.execPath", () => {
-    const bundled = bundledBunPath();
-    const durable = durableBunPath();
-    expect(typeof durable).toBe("string");
-    expect(durable.length).toBeGreaterThan(0);
-    if (bundled) expect(durable).toBe(bundled);
-    else expect(durable).toBe(process.execPath);
+    const inheritedOverride = process.env.OPENCODEX_BUN_PATH;
+    delete process.env.OPENCODEX_BUN_PATH;
+    try {
+      const bundled = bundledBunPath();
+      const durable = durableBunPath();
+      expect(typeof durable).toBe("string");
+      expect(durable.length).toBeGreaterThan(0);
+      if (bundled) expect(durable).toBe(bundled);
+      else expect(durable).toBe(process.execPath);
+    } finally {
+      if (inheritedOverride === undefined) delete process.env.OPENCODEX_BUN_PATH;
+      else process.env.OPENCODEX_BUN_PATH = inheritedOverride;
+    }
   });
 });
diff --git a/tests/ocx-launcher-runtime.test.ts b/tests/ocx-launcher-runtime.test.ts
@@ -1,6 +1,6 @@
 import { describe, expect, test } from "bun:test";
 import { spawn, spawnSync, type ChildProcess } from "node:child_process";
-import { copyFileSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs";
+import { chmodSync, copyFileSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs";
 import { createServer } from "node:net";
 import { tmpdir } from "node:os";
 import { join, resolve } from "node:path";
@@ -128,6 +128,33 @@ function sameProcess(actual: WindowsProcessIdentity | null, expected: WindowsPro
     && sameWindowsPath(actual.executablePath, expected.executablePath);
 }
 
+function captureWindowsProcessIdentity(pid: number): WindowsProcessIdentity {
+  let lastError: unknown;
+  for (let attempt = 0; attempt < 20; attempt += 1) {
+    try {
+      const identity = windowsProcessIdentity(pid);
+      if (identity) return identity;
+    } catch (error) {
+      lastError = error;
+    }
+    Bun.sleepSync(100);
+  }
+  throw new Error(`could not capture process identity for PID ${pid}: ${String(lastError ?? "process not found")}`);
+}
+
+function inspectWindowsProcessIdentity(pid: number): WindowsProcessIdentity | null {
+  let lastError: unknown;
+  for (let attempt = 0; attempt < 5; attempt += 1) {
+    try {
+      return windowsProcessIdentity(pid);
+    } catch (error) {
+      lastError = error;
+      Bun.sleepSync(100);
+    }
+  }
+  throw lastError;
+}
+
 function removeTree(path: string): void {
   // Windows can retain the copied executable's image handle briefly after
   // taskkill returns. Retry only transient fixture-cleanup errors, with a cap.
@@ -153,15 +180,19 @@ async function effectiveRuntime(override: string): Promise<string> {
   const opencodexHome = join(root, "opencodex");
   const codexHome = join(root, "codex");
   const grokHome = join(root, "grok");
-  mkdirSync(opencodexHome, { recursive: true });
-  mkdirSync(codexHome, { recursive: true });
-  mkdirSync(grokHome, { recursive: true });
-
-  const port = await freePort();
+  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,
@@ -176,8 +207,8 @@ async function effectiveRuntime(override: string): Promise<string> {
       },
     });
     if (!launcher.pid) throw new Error("Node launcher has no process id");
-    ownedLauncher = windowsProcessIdentity(launcher.pid);
-    if (!ownedLauncher) throw new Error("could not capture Node launcher process identity");
+    launcherPid = launcher.pid;
+    ownedLauncher = captureWindowsProcessIdentity(launcherPid);
 
     const health = await waitForHealth(port, 25_000, launcher);
     if (!health) throw new Error("proxy did not become healthy");
@@ -186,34 +217,150 @@ async function effectiveRuntime(override: string): Promise<string> {
       throw new Error("health PID is not the spawned Node launcher's direct Bun child");
     }
     ownedProxy = identity;
-    return identity.executablePath;
-  } finally {
-    // Both identities were captured from processes this test spawned. Re-query
-    // before each kill so a reused PID can never become a cleanup target.
-    const cleanupErrors: string[] = [];
-    for (const [label, identity] of [
-      ["launcher", ownedLauncher],
-      ["proxy", ownedProxy],
-    ] as const) {
-      if (!identity || !sameProcess(windowsProcessIdentity(identity.pid), identity)) continue;
-      try {
-        killProxy(identity.pid);
-      } catch (error) {
-        cleanupErrors.push(`${label} cleanup failed: ${String(error)}`);
+    runtimePath = identity.executablePath;
+  } catch (error) {
+    hasPrimaryError = true;
+    primaryError = error;
+  }
+
+  const cleanupErrors: string[] = [];
+  let launcherTreeStopped = false;
+
+  // Prefer the creation-time/path identity. If the initial CIM capture failed,
+  // the live ChildProcess handle and its PID are still positive ownership of
+  // this test's launcher, so terminate that exact process tree as a fallback.
+  if (ownedLauncher) {
+    try {
+      if (sameProcess(inspectWindowsProcessIdentity(ownedLauncher.pid), ownedLauncher)) {
+        killProxy(ownedLauncher.pid);
+        launcherTreeStopped = true;
       }
-      if (sameProcess(windowsProcessIdentity(identity.pid), identity)) {
-        cleanupErrors.push(`owned ${label} PID ${identity.pid} remained after bounded cleanup`);
+    } catch (error) {
+      cleanupErrors.push(`launcher cleanup failed: ${String(error)}`);
+    }
+  }
+  if (!launcherTreeStopped && launcher && launcherPid && launcher.exitCode === null && launcher.signalCode === null) {
+    try {
+      killProxy(launcherPid);
+      launcherTreeStopped = true;
+    } catch (error) {
+      cleanupErrors.push(`launcher tree fallback failed: ${String(error)}`);
+    }
+  }
+
+  // The launcher tree kill normally removes the Bun child. Retain the
+  // identity-verified proxy fallback in case the launcher exited first.
+  if (ownedProxy) {
+    try {
+      if (sameProcess(inspectWindowsProcessIdentity(ownedProxy.pid), ownedProxy)) {
+        killProxy(ownedProxy.pid);
       }
+    } catch (error) {
+      cleanupErrors.push(`proxy cleanup failed: ${String(error)}`);
     }
+  }
+
+  // Inspection failures are reported, but never prevent the remaining
+  // process checks or fixture cleanup from running.
+  for (const [label, identity] of [
+    ["launcher", ownedLauncher],
+    ["proxy", ownedProxy],
+  ] as const) {
+    if (!identity) continue;
     try {
-      removeTree(root);
+      if (sameProcess(inspectWindowsProcessIdentity(identity.pid), identity)) {
+        cleanupErrors.push(`owned ${label} PID ${identity.pid} remained after bounded cleanup`);
+      }
     } catch (error) {
-      cleanupErrors.push(`fixture cleanup failed: ${String(error)}`);
+      cleanupErrors.push(`${label} cleanup verification failed: ${String(error)}`);
     }
-    if (cleanupErrors.length > 0) throw new Error(cleanupErrors.join("; "));
   }
+  if (port !== null) {
+    const lingeringProxy = await healthAt(port);
+    if (lingeringProxy) {
+      cleanupErrors.push(`OpenCodex proxy PID ${lingeringProxy.pid} remained on owned port ${port}`);
+    }
+  }
+  try {
+    removeTree(root);
+  } catch (error) {
+    cleanupErrors.push(`fixture cleanup failed: ${String(error)}`);
+  }
+
+  if (hasPrimaryError) {
+    if (cleanupErrors.length > 0) console.error(`additional cleanup errors: ${cleanupErrors.join("; ")}`);
+    throw primaryError;
+  }
+  if (cleanupErrors.length > 0) throw new Error(cleanupErrors.join("; "));
+  if (!runtimePath) throw new Error("proxy runtime path was not captured");
+  return runtimePath;
+}
+
+function isolatedLauncherEnv(root: string, override: string): NodeJS.ProcessEnv {
+  const opencodexHome = join(root, "opencodex");
+  const codexHome = join(root, "codex");
+  const grokHome = join(root, "grok");
+  mkdirSync(opencodexHome, { recursive: true });
+  mkdirSync(codexHome, { recursive: true });
+  mkdirSync(grokHome, { recursive: true });
+  return {
+    ...process.env,
+    HOME: root,
+    USERPROFILE: root,
+    OPENCODEX_HOME: opencodexHome,
+    CODEX_HOME: codexHome,
+    GROK_HOME: grokHome,
+    OPENCODEX_BUN_PATH: override,
+  };
 }
 
+describe.skipIf(!nodeAvailable)("ocx npm launcher relative Bun override", () => {
+  test("resolves a valid bare relative override before spawning", () => {
+    const root = mkdtempSync(join(tmpdir(), "ocx-launcher-relative-"));
+    try {
+      const overrideName = `custom-bun${process.platform === "win32" ? ".exe" : ""}`;
+      const override = join(root, overrideName);
+      copyFileSync(process.execPath, override);
+      chmodSync(override, 0o755);
+
+      const result = spawnSync("node", [BIN_OCX, "--version"], {
+        cwd: root,
+        encoding: "utf8",
+        timeout: 30_000,
+        windowsHide: true,
+        env: isolatedLauncherEnv(root, overrideName),
+      });
+
+      expect(result.status).toBe(0);
+      expect(result.stderr).not.toContain("OPENCODEX_BUN_PATH is missing");
+    } finally {
+      removeTree(root);
+    }
+  }, 60_000);
+
+  test("warns without exposing the rejected override path before bundled fallback", () => {
+    const root = mkdtempSync(join(tmpdir(), "ocx-launcher-invalid-"));
+    try {
+      const overrideName = `stub-bun${process.platform === "win32" ? ".exe" : ""}`;
+      writeFileSync(join(root, overrideName), "not a Bun executable", "utf8");
+
+      const result = spawnSync("node", [BIN_OCX, "--version"], {
+        cwd: root,
+        encoding: "utf8",
+        timeout: 30_000,
+        windowsHide: true,
+        env: isolatedLauncherEnv(root, overrideName),
+      });
+
+      expect(result.status).toBe(0);
+      expect(result.stderr).toContain("OPENCODEX_BUN_PATH is missing, unreadable, or not a complete Bun binary");
+      expect(result.stderr).not.toContain(root);
+    } finally {
+      removeTree(root);
+    }
+  }, 60_000);
+});
+
 describe.skipIf(!runnable)("ocx npm launcher effective Bun runtime", () => {
   test("uses a valid OPENCODEX_BUN_PATH for the actual proxy process", async () => {
     const root = mkdtempSync(join(tmpdir(), "ocx-launcher-runtime-copy-"));
diff --git a/tests/ocx-launcher-source.test.ts b/tests/ocx-launcher-source.test.ts
@@ -25,18 +25,28 @@ describe("ocx.mjs npm launcher (source invariants)", () => {
 
   test("valid Bun overrides are selected before the bundled runtime", () => {
     expect(source).toContain('const BUN_OVERRIDE_ENV = "OPENCODEX_BUN_PATH";');
-    expect(source).toContain("if (override && isRealBunBinary(override)) return override;");
+    expect(source).toContain("const overridePath = resolve(override);");
+    expect(source).toContain("if (isRealBunBinary(overridePath)) return overridePath;");
 
     const resolveStart = source.indexOf("function resolveBun() {");
     const overrideCheck = source.indexOf("process.env[BUN_OVERRIDE_ENV]?.trim()", resolveStart);
+    const overrideResolve = source.indexOf("resolve(override)", overrideCheck);
     const bundledLookup = source.indexOf("bunDir = bunBinDir()", resolveStart);
     expect(resolveStart).toBeGreaterThanOrEqual(0);
     expect(overrideCheck).toBeGreaterThan(resolveStart);
-    expect(bundledLookup).toBeGreaterThan(overrideCheck);
+    expect(overrideResolve).toBeGreaterThan(overrideCheck);
+    expect(bundledLookup).toBeGreaterThan(overrideResolve);
   });
 
-  test("invalid Bun overrides fall back without throwing", () => {
+  test("invalid Bun overrides warn safely and fall back without throwing", () => {
     expect(source).toContain("function isRealBunBinary(path) {");
     expect(source).toMatch(/function isRealBunBinary\(path\) \{[\s\S]*?try \{[\s\S]*?statSync\(path\)[\s\S]*?catch \{[\s\S]*?return false;/);
+    expect(source).toContain("is missing, unreadable, or not a complete Bun binary; falling back to the bundled runtime.");
+    expect(source).not.toContain('${override} is missing, unreadable');
+  });
+
+  test("documents why the Node launcher keeps a synchronized validator copy", () => {
+    expect(source).toContain("this Node-safe copy aligned with src/lib/bun-runtime.ts");
+    expect(source).toContain("cannot\n// import Bun-native TypeScript until after it has resolved a Bun executable");
   });
 });
