From b0f1739742bfadeedbefdf6eb981b2efdc5b1036 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 18 Aug 2026 11:40:15 +0900 Subject: [PATCH] test(codex): centralize shim recursion-guard sanitization The previous fix deleted both guard variables at one spawn site. A reviewer pointed out the class of bug survives: the nearby re-entry tests assert status 126, so an inherited depth offset leaves them passing for entirely the wrong reason, and the next test to spawn a shim starts the cycle again. shimChildEnv strips PID, DEPTH and PROBE_ACTIVE in one place, mirroring what probeUnixShimInstall already does before spawning its probe. The second spawn site at line 291 had the same gap - it deleted only the pid - so it was quietly depth-sensitive too. What this buys is that a green run means what it says. Before, these tests measured whatever shim ancestry the developer's shell happened to carry, and were green in CI only because CI has no shimmed ancestor. --- tests/codex-shim.test.ts | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/tests/codex-shim.test.ts b/tests/codex-shim.test.ts index 177964e987..b9ed2a5663 100644 --- a/tests/codex-shim.test.ts +++ b/tests/codex-shim.test.ts @@ -7,6 +7,29 @@ import { autoRestoreCodexShim, buildUnixCodexShim, buildWindowsCodexShim, buildW const SHIM_MARKER = "opencodex codex autostart shim"; const UNIX_SHIM_REVISION_MARKER = "opencodex unix codex shim revision 2"; + +/** + * A child environment with the shim's recursion-guard state stripped. + * + * Every one of these tests reasons about a shim invocation starting from depth 0, but a developer + * whose shell was itself launched through an installed Codex shim exports + * `OCX_SHIM_ACTIVE_DEPTH=1` — so the guard fires a level early and the test measures whatever + * ancestry the machine happened to have. CI has no shimmed ancestor, which is what let that bleed + * hide: green there, red on a real developer's machine. + * + * The re-entry tests are the subtle case. They assert `status === 126`, so an inherited +1 offset + * leaves them passing for entirely the wrong reason. Sanitizing centrally is what makes their + * green mean what it says. + * + * Mirrors `probeUnixShimInstall`, which already clears the same three before spawning its probe. + */ +function shimChildEnv(overrides: Record = {}): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env, ...overrides }; + delete env.OCX_SHIM_ACTIVE_PID; + delete env.OCX_SHIM_ACTIVE_DEPTH; + delete env.OCX_SHIM_PROBE_ACTIVE; + return env; +} const skipStabilityWait = () => {}; const python3Path = process.platform === "win32" ? "" @@ -288,8 +311,7 @@ exit 64 chmodSync(misePath, 0o755); chmodSync(realCodexPath, 0o755); chmodSync(shimPath, 0o755); - const env = { ...process.env, PATH: prependPath(dir, process.env.PATH), OCX_SHIM_BYPASS: "1" }; - delete env.OCX_SHIM_ACTIVE_PID; + const env = shimChildEnv({ PATH: prependPath(dir, process.env.PATH) ?? "", OCX_SHIM_BYPASS: "1" }); const result = spawnSync(shimPath, ["--help"], { encoding: "utf8", @@ -1042,15 +1064,7 @@ printf '%s\\n' child-codex chmodSync(bunPath, 0o755); chmodSync(realCodexPath, 0o755); chmodSync(shimPath, 0o755); - const env = { ...process.env, OCX_SHIM_BYPASS: "1" }; - // Both recursion-guard variables, not just the pid. A developer running this suite from a - // shell that was itself launched through an installed shim inherits - // OCX_SHIM_ACTIVE_DEPTH=1, so the outer shim starts at depth 1, the child re-entry lands on - // depth 2, and the guard exits 126 with the launcher-loop message — the shim behaving - // exactly as designed, on a test that meant to start from a clean slate. CI never sees it - // because CI has no shimmed ancestor, which is what made this look environmental. - delete env.OCX_SHIM_ACTIVE_PID; - delete env.OCX_SHIM_ACTIVE_DEPTH; + const env = shimChildEnv({ OCX_SHIM_BYPASS: "1" }); const result = spawnSync(shimPath, ["--help"], { encoding: "utf8",