diff --git a/src/cli/restart-scope.ts b/src/cli/restart-scope.ts index 4f8660b2d95..2dd14c94214 100644 --- a/src/cli/restart-scope.ts +++ b/src/cli/restart-scope.ts @@ -6,7 +6,7 @@ */ import { afterCatalogWriteHandleAppServers } from "../codex/app-server-processes"; import type { AfterCatalogWriteAppServerResult } from "../codex/app-server-processes"; -import type { DesktopAppRestartResult } from "../codex/desktop-app-restart"; +import type { DesktopAppRestartIo, DesktopAppRestartResult } from "../codex/desktop-app-restart"; /** * Which restart a command was asked for. @@ -102,10 +102,12 @@ export async function handleRestartScopeAfterWrite( */ export async function handleDesktopAppRestart( log: Pick, + io: DesktopAppRestartIo = {}, ): Promise { const { restartCodexDesktopApp } = await import("../codex/desktop-app-restart"); const { startDesktopRestartHandoff } = await import("../codex/desktop-app/handoff"); const result = restartCodexDesktopApp({ + ...io, // The CLI is the one caller whose exit is exactly the signal the helper waits for, // so it is the one caller allowed to hand off. The management service is not (it // runs in a proxy that never exits) and the helper itself is not (recursion). @@ -123,6 +125,12 @@ export async function handleDesktopAppRestart( + "nothing was stopped.", ); return result; + case "test_environment": + log.error( + "Skipped the Codex desktop app restart: this is an armed opencodex test process " + + "(OCX_TEST_HOME_GUARD=1), so the real app was not touched.", + ); + return result; case "restart_in_flight": log.error( "Another Codex desktop-app restart is already running; this one did nothing. " diff --git a/src/codex/desktop-app-restart.ts b/src/codex/desktop-app-restart.ts index 17bf83ed7de..6de8220ad0b 100644 --- a/src/codex/desktop-app-restart.ts +++ b/src/codex/desktop-app-restart.ts @@ -40,6 +40,7 @@ import { rootShells, type DesktopAppAdapter, type DesktopExec, type DesktopProce import { darwinDesktopAppAdapter, darwinDefaultExec } from "./desktop-app/darwin"; import { linuxDesktopAppAdapter, linuxDefaultExec } from "./desktop-app/linux"; import { windowsDesktopAppAdapter, windowsDefaultExec } from "./desktop-app/windows"; +import { isTestHomeGuardArmed } from "../lib/test-home-guard"; export type { DesktopAppExecOptions } from "./desktop-app/types"; @@ -81,6 +82,7 @@ export interface DesktopAppRestartIo { export type DesktopAppRestartReason = | "unsupported_platform" + | "test_environment" | "package_discovery_failed" | "process_probe_failed" | "no_targets" @@ -213,6 +215,12 @@ export function restartCodexDesktopApp(io: DesktopAppRestartIo = {}): DesktopApp const adapter = io.adapter ?? selected?.adapter; const exec = io.execFile ?? selected?.exec; if (!adapter || !exec) return skipped("unsupported_platform"); + // In an armed test process a call without an injected exec would reach the real OS (an injected + // adapter still execs through the platform default): on a developer Mac, `performCodexRestart` + // tests quit the user's ChatGPT (Codex) app and relaunched it through `/usr/bin/open` with the + // runner's sandbox HOME, logged out. Armed means the test preload's flag, not NODE_ENV, for the + // reason test-home-guard gives: a real `NODE_ENV=test ocx ...` must still restart the app. + if (!io.execFile && isTestHomeGuardArmed()) return skipped("test_environment"); // Step 0. Two restarts at once are destructive rather than merely wasteful: the // first quits and relaunches, the second sees the freshly started shell as a target diff --git a/tests/clients/desktop-app-restart-posix.test.ts b/tests/clients/desktop-app-restart-posix.test.ts index 585c86dcb78..660d2775f5a 100644 --- a/tests/clients/desktop-app-restart-posix.test.ts +++ b/tests/clients/desktop-app-restart-posix.test.ts @@ -1,8 +1,9 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { mkdirSync, mkdtempSync, realpathSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { restartCodexDesktopApp, type DesktopAppRestartIo } from "../../src/codex/desktop-app-restart"; +import { setDarwinKillForTests } from "../../src/codex/desktop-app/darwin"; import { isUnderRoot } from "../../src/codex/desktop-app/types"; import { acquireDesktopRestartLock, @@ -52,6 +53,15 @@ function psRows(rows: Array<[number, number, string]>): string { return rows.map(([pid, ppid, exe]) => `${pid} ${ppid} ${WHEN} ${process.getuid?.() ?? 0} ${exe}`).join("\n"); } +/** + * Every signal the darwin adapter sends. The adapter signals through `process.kill`, which the + * exec seam cannot intercept, so without this recorder the synthetic pids below (15901 …) were + * signalled for real on whatever machine ran the suite. + */ +const kills: Array<[number, string]> = []; +beforeEach(() => { setDarwinKillForTests((pid, signal) => { kills.push([pid, signal]); }); }); +afterEach(() => { setDarwinKillForTests(null); }); + function darwinIo(options: { calls: Call[]; rows?: Array<[number, number, string]>; @@ -279,6 +289,26 @@ describe.skipIf(process.platform === "win32")("a stop is only ever claimed when }); }); +describe.skipIf(process.platform === "win32")("darwin signals stay inside the suite", () => { + test("a forced stop signals the recorder, never a real pid", () => { + kills.length = 0; + restartCodexDesktopApp({ + platform: "darwin", + lock: isolatedLock(), + ancestryPids: () => [99_999], + isAlive: () => true, + sleep: () => {}, + now: (() => { let t = 0; return () => (t += 500); })(), + execFile: (file) => { + if (file === "/bin/ps") return psRows([[15901, 1, SHELL]]); + if (file === "/usr/libexec/PlistBuddy") return "com.openai.codex"; + return ""; + }, + }); + expect(kills.some(([pid]) => pid === 15901)).toBe(true); + }); +}); + describe("the restart singleton lock", () => { const alive = new Set([1001, 1002, 2001]); const io = (lockPath: string, pid: number) => ({ diff --git a/tests/clients/desktop-app-restart.test.ts b/tests/clients/desktop-app-restart.test.ts index fd7ae597dcd..1fd3de3c26e 100644 --- a/tests/clients/desktop-app-restart.test.ts +++ b/tests/clients/desktop-app-restart.test.ts @@ -1,10 +1,11 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, test } from "bun:test"; import { execFileSync } from "node:child_process"; import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { restartCodexDesktopApp, type DesktopAppRestartIo } from "../../src/codex/desktop-app-restart"; import { windowsDesktopAppAdapter } from "../../src/codex/desktop-app/windows"; +import { handleDesktopAppRestart } from "../../src/cli/restart-scope"; import { setTrustedWindowsElevationExecutablesForTests } from "../../src/lib/windows-elevation"; /** @@ -140,6 +141,35 @@ function scriptedIo(options: { const DISCOVERY = [AUMID.replace("!App", ""), INSTALL, AUMID].join("\n"); +// The guard follows the test preload's OCX_TEST_HOME_GUARD, as the home guard does, rather than +// NODE_ENV: Bun's test runner keeps an inherited NODE_ENV, and a real `NODE_ENV=test ocx ...` +// must still restart the app. +describe("the test-runner guard follows the test preload, not NODE_ENV", () => { + const originalNodeEnv = process.env.NODE_ENV; + const originalGuard = process.env.OCX_TEST_HOME_GUARD; + afterEach(() => { + if (originalNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = originalNodeEnv; + if (originalGuard === undefined) delete process.env.OCX_TEST_HOME_GUARD; + else process.env.OCX_TEST_HOME_GUARD = originalGuard; + }); + + test("an armed test process is guarded even when NODE_ENV was inherited as something else", () => { + process.env.NODE_ENV = "development"; + const result = restartCodexDesktopApp({ lock: isolatedLock(), platform: "win32" }); + expect(result.reason).toBe("test_environment"); + }); + + test("a process the test preload did not arm restarts as usual even with NODE_ENV=test", () => { + process.env.NODE_ENV = "test"; + process.env.OCX_TEST_HOME_GUARD = "0"; + // discover() answers without exec, so this never reaches the OS. + const adapter = { ...windowsDesktopAppAdapter, discover: () => null }; + const result = restartCodexDesktopApp({ lock: isolatedLock(), platform: "win32", adapter }); + expect(result.reason).toBe("package_discovery_failed"); + }); +}); + describe("Codex desktop app restart (#2292)", () => { // macOS and Linux are no longer no-ops: they have real adapters. What survives from the // original assertion is that a platform with NO adapter still refuses without execing @@ -158,6 +188,33 @@ describe("Codex desktop app restart (#2292)", () => { expect(calls).toEqual([]); }); + // A test that reaches the restart without injecting an adapter or exec used to drive the real + // OS adapter: on a developer Mac `performCodexRestart` tests quit the user's ChatGPT (Codex) + // app and relaunched it through `/usr/bin/open` with the runner's sandbox HOME, logged out. + // win32 keeps the pre-fix run harmless off Windows (no PowerShell to discover anything with). + test("under the test runner, the real OS adapter is never used without an injected one", () => { + const result = restartCodexDesktopApp({ lock: isolatedLock(), platform: "win32" }); + expect(result).toEqual({ + attempted: false, stopped: [], surviving: [], relaunch: "skipped", reason: "test_environment", + }); + }); + + // An injected adapter still execs through the platform default when no exec is injected, so + // only an injected exec proves the caller is simulating the OS. + test("an injected adapter without an injected exec still never reaches the OS", () => { + const result = restartCodexDesktopApp({ lock: isolatedLock(), platform: "win32", adapter: windowsDesktopAppAdapter }); + expect(result.reason).toBe("test_environment"); + }); + + // win32 keeps this call away from the OS on macOS and Linux even if the guard regressed. + test("the CLI says why nothing was restarted under the test runner", async () => { + const out: string[] = []; + const log = { log: (...a: unknown[]) => { out.push(a.join(" ")); }, error: (...a: unknown[]) => { out.push(a.join(" ")); } }; + const result = await handleDesktopAppRestart(log, { platform: "win32", lock: isolatedLock() }); + expect(result.reason).toBe("test_environment"); + expect(out.join("\n")).toContain("OCX_TEST_HOME_GUARD"); + }); + test("fails closed when the package cannot be identified, killing nothing", () => { const calls: Call[] = []; const result = withTrustedExes(() => restartCodexDesktopApp(scriptedIo({ discovery: "MISS", calls })));