From bcefa2b786a21ee8d0563b300e12941e596228e4 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 12:16:10 +0900 Subject: [PATCH 1/6] fix(windows): unblock the isolated fabric producer and unpin CI-scaled waits The Windows suite leg is dispatch-only because it has never run green. A full dispatch against dev (run 32790129147) left 20 failures across 4 shards, and they reduce to five causes rather than twenty defects. Fourteen of them are one cause. `minimalChildEnv` gave the isolated CL-07 patch producer three variables, and on Windows a CreateProcess child inherits nothing else -- including SystemRoot, which the loader needs to resolve the system DLLs the Bun executable links against. The child died before reaching its entry module, the parent saw an immediate close with no protocol line, and every producer case reported `harness_failure` -> `inconclusive`. Forward the four OS-owned bootstrap variables on win32 only. The sandbox boundary is the scratch root and the absent credential/config state; neither is weakened by letting the child find its own loader and temp directory. The env is now exported, because the direct-spawn test in lab-fabric-task carried its own literal copy of that three-variable object and would have kept asserting against an environment production no longer uses. Three failures were fixed deadlines doing duty as latency assertions. The Windows shards run four Bun pools on one runner, and `watchdogMs` already exists for exactly this; the owner-lease case was CANCELLED at 30,172ms against a flat 30s budget, so no assertion in it ever reported. Route those waits through the helper and derive each surrounding budget from its internal deadline, so the two cannot drift apart again. One is a product defect the test caught. The startup-health probe spawns a Bun CLI child that then shells out to sc.exe/schtasks.exe, and the flat 5s timeout overran under load: the endpoint answered `diagnosticStale: true` for a host it could have read, downgrading a `protected` machine to `at-risk` and recommending repair for a healthy service. 15s on Windows only, still bounded. The two #1852 cases are not fixed here. Both reach the collector through the real execFile path, which maps any exec failure to `state: "unknown"` with no processes -- indistinguishable from a broken fixture, which is why they read as behavioural regressions. Added a fixture probe so the Windows leg reports which one it is instead of blaming the design. Verification: focused suites green on macOS (lab-fabric-task 48, autostart-health 14, native-main-owner-lifetime 10, cli-start-journal-order 2, codex-app-server-processes 53); typecheck, privacy:scan clean. The Windows leg is the real oracle and runs on the PR. --- src/lab/fabric/producer-isolate.ts | 32 ++++++++++++-- src/server/startup-health-cache.ts | 29 +++++++++++- tests/autostart-health.test.ts | 10 ++++- tests/cli-start-journal-order.test.ts | 23 ++++++++-- tests/codex-app-server-processes.test.ts | 16 ++++++- tests/helpers/windows-power-shell-fixture.ts | 35 +++++++++++++++ tests/lab-fabric-task.test.ts | 46 +++++++++++++++++--- tests/native-main-owner-lifetime.test.ts | 30 +++++++++---- 8 files changed, 196 insertions(+), 25 deletions(-) diff --git a/src/lab/fabric/producer-isolate.ts b/src/lab/fabric/producer-isolate.ts index 6a2aa45617..55d5e851e1 100644 --- a/src/lab/fabric/producer-isolate.ts +++ b/src/lab/fabric/producer-isolate.ts @@ -31,12 +31,38 @@ interface IsolateRequest { now?: () => number; } -function minimalChildEnv(scratchRoot: string): Record { - return { +/** + * The environment an isolated producer child runs with. + * + * Exported so a test that spawns `producer-child.ts` directly cannot drift from + * the environment production actually uses. The Windows-only additions below + * are load-bearing, and a test carrying its own literal copy of this object + * silently loses them. + */ +export function minimalFabricChildEnv(scratchRoot: string): Record { + const env: Record = { TZ: "UTC", NO_COLOR: "1", OCX_FABRIC_SCRATCH_ROOT: scratchRoot, }; + if (process.platform !== "win32") return env; + // Windows has no equivalent of "run with an (almost) empty environment". A + // CreateProcess child inherits nothing here, and the loader itself reads the + // environment: without SystemRoot it cannot resolve the system DLLs the Bun + // executable links against, so the child dies before its entry module runs. + // The parent then sees an immediate non-zero close with no protocol line and + // reports harness_failure -- which is what turned every CL-07 producer case + // into "inconclusive" on the Windows leg while POSIX stayed green. + // + // These are OS-owned process bootstrap state, not caller-supplied + // configuration: the sandbox boundary is the scratch root plus the absent + // credential/config variables, and neither is weakened by letting the child + // find its own loader and temp directory. + for (const name of ["SystemRoot", "windir", "TEMP", "TMP"] as const) { + const value = process.env[name]; + if (value) env[name] = value; + } + return env; } function killChild(child: ChildProcess): void { @@ -56,7 +82,7 @@ export async function runIsolatedFabricProducer(request: IsolateRequest): Promis let child: ChildProcess; try { child = spawn(process.execPath, ["run", CHILD_ENTRY], { - env: minimalChildEnv(request.scratchRoot), + env: minimalFabricChildEnv(request.scratchRoot), stdio: ["pipe", "pipe", "pipe"], }); } catch (error) { diff --git a/src/server/startup-health-cache.ts b/src/server/startup-health-cache.ts index 63dd67f651..6c5b4274d0 100644 --- a/src/server/startup-health-cache.ts +++ b/src/server/startup-health-cache.ts @@ -9,9 +9,34 @@ import type { OcxConfig } from "../types"; import { truncateRetainedUtf8 } from "../lib/admission"; const CACHE_TTL_MS = 30_000; -const PROBE_TIMEOUT_MS = 5_000; -const INITIAL_PROBE_WAIT_MS = 5_500; +const PROBE_TIMEOUT_MS = probeTimeoutMs(); +const INITIAL_PROBE_WAIT_MS = PROBE_TIMEOUT_MS + 500; const MAX_DIAGNOSTIC_VALUE_BYTES = 8 * 1024; + +/** + * How long the isolated probe child gets before its reading is abandoned. + * + * The child is a full Bun CLI start that then runs `diagnoseService()`, and on + * Windows that means shelling out to `sc.exe` / `schtasks.exe` — external + * processes whose latency is set by the service-control manager, not by us. + * Under load those overran the flat 5s, the probe was abandoned, and the + * endpoint answered `diagnosticStale: true` for a machine it could have read. + * That is a real dashboard regression, not only a test failure: it downgrades a + * `protected` host to `at-risk` and recommends a repair command for a healthy + * service. + * + * Raising it only on Windows keeps the tighter bound everywhere else. It stays a + * bound in both cases: a wedged probe is still abandoned, and the caller still + * receives the previous reading rather than waiting on it. + */ +function probeTimeoutMs(): number { + return process.platform === "win32" ? 15_000 : 5_000; +} + +/** The probe bound, so a test's own budget cannot fall below what it must wait for. */ +export function startupHealthProbeTimeoutMs(): number { + return PROBE_TIMEOUT_MS; +} let cached: { timestamp: number; value: StartupHealth } | null = null; let inflight: Promise | null = null; let generation = 0; diff --git a/tests/autostart-health.test.ts b/tests/autostart-health.test.ts index c3fe6aa991..cecde35c50 100644 --- a/tests/autostart-health.test.ts +++ b/tests/autostart-health.test.ts @@ -4,8 +4,16 @@ import { unusedProxyWarningLines } from "../src/cli/status"; import { classifyCodexRouting, hasInjectedCodexRouting } from "../src/codex/inject"; import { handleManagementAPI } from "../src/server/management-api"; import { invalidateStartupHealthCache, markStartupHealthDiagnosticStale } from "../src/server/startup-health-cache"; +import { startupHealthProbeTimeoutMs } from "../src/server/startup-health-cache"; import type { OcxConfig } from "../src/types"; +// This case deliberately sits out the 30s cache TTL and then reads again, so it +// pays for TWO probes plus the sleep. Both probes are bounded by the production +// timeout, which is higher on Windows because the reading shells out to the +// service-control manager there. A flat budget silently became the shorter of the +// two limits on that lane. +const STARTUP_HEALTH_CACHE_BUDGET_MS = 40_000 + startupHealthProbeTimeoutMs() * 2; + const base = { routingKind: "opencodex-local" as const, autostartEnabled: true, @@ -229,7 +237,7 @@ describe("Codex startup health", () => { ); const refreshedBody = await refreshed!.json() as Record; expect(refreshedBody.diagnosticStale).toBe(false); - }, 40_000); + }, STARTUP_HEALTH_CACHE_BUDGET_MS); }); import { ManagementRequest as Request } from "./helpers/management-auth"; diff --git a/tests/cli-start-journal-order.test.ts b/tests/cli-start-journal-order.test.ts index 48f4ba95bc..e2fe633932 100644 --- a/tests/cli-start-journal-order.test.ts +++ b/tests/cli-start-journal-order.test.ts @@ -2,6 +2,21 @@ import { afterEach, describe, expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; +import { watchdogMs } from "./helpers/ci-watchdog"; + +// Every wait here is bounded by a real `ocx start` child coming up: spawning Bun, +// binding a port, and writing its runtime record. That is intrinsic to the +// assertion, so the bound stays -- but a fixed 10s is a latency assertion on the +// Windows leg, where four Bun pools share one runner. "timed out waiting for +// owner runtime record" at 10.2s was that, not a journal-ownership defect. +const OWNER_WAIT_MS = watchdogMs(10_000); + +// The surrounding budget has to clear the internal deadline, or the test dies on a +// timeout before its own wait can report which step stalled -- the failure mode +// test-budget.ts warns about. Each case performs up to four sequential bounded +// waits (owner runtime record, owner health, and two CLI children), so the budget +// is derived from the deadline rather than pinned next to it. +const JOURNAL_OWNERSHIP_BUDGET_MS = Math.max(30_000, OWNER_WAIT_MS * 4); const cliPath = resolve(import.meta.dir, "../src/cli/index.ts"); const roots: string[] = []; @@ -80,13 +95,13 @@ async function runCli(fx: Fixture, argv: string[]): Promise<{ exitCode: number; children.push(child); const completed = await Promise.race([ Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]), - new Promise((_, reject) => setTimeout(() => reject(new Error(`CLI watchdog: ocx ${argv.join(" ")}`)), 10_000)), + new Promise((_, reject) => setTimeout(() => reject(new Error(`CLI watchdog: ocx ${argv.join(" ")}`)), OWNER_WAIT_MS)), ]); return { exitCode: completed[0], stdout: completed[1], stderr: completed[2] }; } async function waitFor(read: () => T | null | Promise, label: string): Promise { - const deadline = Date.now() + 10_000; + const deadline = Date.now() + OWNER_WAIT_MS; while (Date.now() < deadline) { const value = await read(); if (value !== null) return value; @@ -159,7 +174,7 @@ describe("start and ensure journal ownership (#1230)", () => { owner.kill("SIGTERM"); await owner.exited; } - }, 30_000); + }, JOURNAL_OWNERSHIP_BUDGET_MS); test("a dead owner is recovered and its stale PID is removed for both start and ensure", async () => { for (const command of ["start", "ensure"] as const) { @@ -192,5 +207,5 @@ describe("start and ensure journal ownership (#1230)", () => { expect(existsSync(fx.journalPath)).toBe(false); expect(existsSync(fx.pidPath)).toBe(false); } - }, 30_000); + }, JOURNAL_OWNERSHIP_BUDGET_MS); }); diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index b25db2c5e1..84c8d0ab3d 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -3,7 +3,11 @@ import { spawn } from "node:child_process"; import { readFileSync } from "node:fs"; import { join } from "node:path"; import { setTrustedWindowsElevationExecutablesForTests } from "../src/lib/windows-elevation"; -import { createWindowsPowerShellFixture, type WindowsPowerShellFixture } from "./helpers/windows-power-shell-fixture"; +import { + createWindowsPowerShellFixture, + probeWindowsPowerShellFixture, + type WindowsPowerShellFixture, +} from "./helpers/windows-power-shell-fixture"; import { afterCatalogWriteHandleAppServers, attachStaleAppServerHint, @@ -33,6 +37,16 @@ beforeAll(async () => { }); afterAll(() => stallingFakePowerShell?.cleanup()); + // Both #1852 cases below reach the collector through the real execFile path, and + // the collector maps any exec failure to `state: "unknown"` with no processes. + // So a fixture that cannot run produces exactly the assertion failures a + // synchronous implementation would, and the Windows leg reported the design + // regression it does not have. This names the real condition instead. + test("the PowerShell fixture the #1852 cases depend on actually executes", async () => { + const probe = await probeWindowsPowerShellFixture(stallingFakePowerShell); + expect(probe.ok, `fake PowerShell fixture at ${stallingFakePowerShell.executable} did not run: ${probe.detail}`).toBe(true); + }); + test("not_running when no app-server process exists", () => { const status = collectCodexAppServerCatalogState({ listSnapshots: () => [], diff --git a/tests/helpers/windows-power-shell-fixture.ts b/tests/helpers/windows-power-shell-fixture.ts index d9bb26e0a6..8e98e2cab3 100644 --- a/tests/helpers/windows-power-shell-fixture.ts +++ b/tests/helpers/windows-power-shell-fixture.ts @@ -7,6 +7,41 @@ export interface WindowsPowerShellFixture { cleanup: () => void | Promise; } +/** + * Run the fixture the way production runs PowerShell and return what happened. + * + * The collector under test swallows an enumeration error into `state: "unknown"` + * with no processes, so a fixture that cannot execute is indistinguishable from + * a machine with no Codex process running. That ambiguity is what made the two + * #1852 cases read as behavioural failures on the Windows leg. Asserting this + * first turns "the fixture is broken" into its own named, self-describing + * failure. + */ +export async function probeWindowsPowerShellFixture( + fixture: WindowsPowerShellFixture, +): Promise<{ ok: boolean; detail: string }> { + try { + const child = Bun.spawn([fixture.executable, "-NoProfile", "-NoLogo", "-NonInteractive", "-Command", "probe"], { + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + if (exitCode === 0 && stdout.includes("codex app-server")) { + return { ok: true, detail: `exit=0 stdout=${JSON.stringify(stdout)}` }; + } + return { + ok: false, + detail: `exit=${exitCode} stdout=${JSON.stringify(stdout)} stderr=${JSON.stringify(stderr.slice(0, 400))}`, + }; + } catch (error) { + return { ok: false, detail: error instanceof Error ? `${error.name}: ${error.message}` : String(error) }; + } +} + /** * Build a real Windows executable for tests that exercise the default execFile path. * diff --git a/tests/lab-fabric-task.test.ts b/tests/lab-fabric-task.test.ts index 7a160b8ef0..e75be0ec4b 100644 --- a/tests/lab-fabric-task.test.ts +++ b/tests/lab-fabric-task.test.ts @@ -54,7 +54,7 @@ import { ensureLabDirs, ensureRestrictedDir } from "../src/lab/paths"; import { verifyExactTreeDiffV1 } from "../src/lab/fabric/verifier"; import { parseSyntheticPatchV1 } from "../src/lab/fabric/patch"; import { FABRIC_LIMITS } from "../src/lab/fabric/constants"; -import { setFabricProducerIsolationLimitsForTests } from "../src/lab/fabric/producer-isolate"; +import { minimalFabricChildEnv, setFabricProducerIsolationLimitsForTests } from "../src/lab/fabric/producer-isolate"; import { taskSubjectApplicableToRequirements } from "../src/lab/projection/verification"; import { createHostIssuedFabricPatchExecutor } from "../src/lib/fabric-task-host"; import type { TrustedFabricPatchExecutor } from "../src/lab/fabric/types"; @@ -377,11 +377,10 @@ export async function execute() { const child = Bun.spawn([process.execPath, "run", childEntry], { cwd: REPO_ROOT, - env: { - TZ: "UTC", - NO_COLOR: "1", - OCX_FABRIC_SCRATCH_ROOT: home, - }, + // Production's environment, not a literal copy of it. On Windows the + // three variables alone cannot start a Bun child at all, so a hardcoded + // copy here asserted against an environment production never uses. + env: minimalFabricChildEnv(home), stdin: "pipe", stdout: "pipe", stderr: "pipe", @@ -1184,4 +1183,39 @@ export async function execute() { expect(text.includes("system prompt")).toBe(false); expect(text.includes(CREDENTIAL_CANARY)).toBe(false); }); + + // Every producer case above runs a real child process, so all of them turn + // "inconclusive" at once when the child cannot start. On Windows that is what + // happened: the child env carried three variables, and a CreateProcess child + // inherits nothing, so the Bun executable could not resolve its system DLLs + // and died before running its entry module. Fourteen cases went red for one + // reason, and none of them named it -- they all reported harness_failure. + // + // This asserts the environment contract directly, so a regression is one + // named failure instead of a diffuse cluster. It runs everywhere: the shape + // is what matters, and the platform branch is inside the function. + test("the isolated producer env carries what a child needs to start on this platform", () => { + const home = tempHome(); + const env = minimalFabricChildEnv(home); + + // The sandbox contract, on every platform: scratch is addressed, and no + // ambient credential or config state is forwarded. + expect(env.OCX_FABRIC_SCRATCH_ROOT).toBe(home); + expect(env.TZ).toBe("UTC"); + for (const leaked of ["OPENCODEX_HOME", "CODEX_HOME", "PATH", "HOME", "USERPROFILE", "APPDATA"]) { + expect(env[leaked]).toBeUndefined(); + } + + if (process.platform !== "win32") { + // POSIX passes the loader an absolute interpreter path and needs nothing else. + expect(Object.keys(env).sort()).toEqual(["NO_COLOR", "OCX_FABRIC_SCRATCH_ROOT", "TZ"]); + return; + } + + // On Windows the loader itself reads the environment. SystemRoot is the one + // that decides whether the child runs at all; assert it against the real + // parent value rather than a literal, since a wrong path fails identically. + expect(env.SystemRoot).toBe(process.env.SystemRoot); + expect(env.SystemRoot).toBeTruthy(); + }); }); diff --git a/tests/native-main-owner-lifetime.test.ts b/tests/native-main-owner-lifetime.test.ts index 58610181da..b88a05f56e 100644 --- a/tests/native-main-owner-lifetime.test.ts +++ b/tests/native-main-owner-lifetime.test.ts @@ -11,6 +11,7 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; +import { watchdogMs } from "./helpers/ci-watchdog"; import { saveConfig } from "../src/config"; import { saveCodexAccountCredential } from "../src/codex/account-store"; @@ -131,7 +132,20 @@ function fixture(configName = "opencodex", includePool = true): Fixture { return { root, codexHome, configDir, key, manager }; } -async function waitUntil(probe: () => T | null, timeoutMs = 10_000): Promise { +// Each wait bounds a real child proxy doing real work: spawning Bun, opening the +// owner SQLite database, and acquiring or releasing the lease. On the Windows +// shards four Bun pools share one runner, so the fixed 10s bounds were reporting +// contention. `watchdogMs` is the repository's existing answer to exactly this. +const OWNER_EVENT_WAIT_MS = watchdogMs(10_000); + +// The lease cases perform several of those waits back to back. The multi-server +// case spawns two children and walks four ownership transitions, and it was +// CANCELLED at 30,172ms against a flat 30s budget -- the budget expired mid-test, +// so no assertion ever reported. Derive it from the deadline so the two cannot +// drift apart again. +const OWNER_LEASE_BUDGET_MS = Math.max(30_000, OWNER_EVENT_WAIT_MS * 4); + +async function waitUntil(probe: () => T | null, timeoutMs = OWNER_EVENT_WAIT_MS): Promise { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { const value = probe(); @@ -191,7 +205,7 @@ class ChildHarness { })(); } - async waitFor(predicate: (event: Event) => boolean, timeoutMs = 10_000): Promise { + async waitFor(predicate: (event: Event) => boolean, timeoutMs = OWNER_EVENT_WAIT_MS): Promise { const deadline = Date.now() + timeoutMs; for (;;) { const found = this.events.find(predicate); @@ -213,7 +227,7 @@ class ChildHarness { return this.waitFor(event => event.event === "reply" && event.id === id); } - async snapshot(predicate: (event: Event) => boolean, timeoutMs = 10_000): Promise { + async snapshot(predicate: (event: Event) => boolean, timeoutMs = OWNER_EVENT_WAIT_MS): Promise { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { const event = await this.command("snapshot"); @@ -227,7 +241,7 @@ class ChildHarness { if (this.child.exitCode !== null) return null; const reply = await this.command("stop"); expect(reply.ok).toBe(true); - const exit = await Promise.race([this.child.exited, Bun.sleep(10_000).then(() => null)]); + const exit = await Promise.race([this.child.exited, Bun.sleep(OWNER_EVENT_WAIT_MS).then(() => null)]); if (exit === null) throw new Error("child did not stop"); if (exit !== 0) throw new Error(await this.stderr); return reply; @@ -419,7 +433,7 @@ describe("native-main process owner lease", () => { if (second) await second.stop().catch(() => second!.hardKill()); if (support) await support.stop().catch(() => support!.hardKill()); } - }, 30_000); + }, OWNER_LEASE_BUDGET_MS); test("a hard-killed owner releases the OS lease and the successor recovers before opening main", async () => { const f = fixture("crash-a", false); @@ -463,7 +477,7 @@ describe("native-main process owner lease", () => { await owner.stop().catch(() => owner.hardKill()); if (successor) await successor.stop().catch(() => successor!.hardKill()); } - }, 30_000); + }, OWNER_LEASE_BUDGET_MS); test("a successor scrubs a hard-killed production auth write before recovery or main admission", async () => { const f = fixture("temp-crash", false); @@ -544,7 +558,7 @@ describe("native-main process owner lease", () => { } finally { await child.stop().catch(() => child.hardKill()); } - }, 30_000); + }, OWNER_LEASE_BUDGET_MS); test("same-process server references retain ownership until the last server stops", async () => { const f = fixture("refs-a"); @@ -573,5 +587,5 @@ describe("native-main process owner lease", () => { await owner.stop().catch(() => owner.hardKill()); if (contender) await contender.stop().catch(() => contender!.hardKill()); } - }, 30_000); + }, OWNER_LEASE_BUDGET_MS); }); From c1b74f79e467da984f555e2da8b65d65b1a90383 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 13:04:06 +0900 Subject: [PATCH 2/6] fix(windows): repair a TOML path round trip and five shard-only test defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second pass on the Windows leg, from dispatch 33231856323 on this branch. Shard 1/4 went green — it held 17 of the original 20 failures, so the fabric child-env and CI-scaled-wait fixes did what they claimed. What remained was 9 failures across shards 2, 3, and 4, and only one is a product defect. The product defect: `readModelInstructionsFile` read the inner text of a TOML basic string verbatim while `setRootString` writes it through `encodeBasicString`, which escapes backslashes. On Windows the round trip therefore did not survive — the reader returned "C:\\Users\\..." for a stored "C:\Users\...", `baseSelection` compared that against the real variant path, and reported `external` for a variant this code had just selected. In the UI that tells the user their base prompt was replaced by a stranger's file. `decodeBasicString` already exists for exactly this and is used on three other read paths; this one skipped it. The regression test uses a literal Windows path so the POSIX lanes guard it too, and it was driven red against the old reader before the fix went in. The other five are tests asserting something their platform does not provide: - config.test asserted `mode & 0o077 === 0` on a Windows temp. Windows has no POSIX mode, and production knows it — `assertPrivateTempDescriptor` skips that exact check on win32, where privacy comes from `hardenSecretPath` instead. The assertion now follows the platform's real mechanism. - test-home-guard interpolated a temp path directly into probe SOURCE. `C:\Users` makes `\U` an escape sequence, so the guard compared a path that was never under test and correctly allowed a directory it never saw. Two sites, both now JSON.stringify. - package-tree-integrity's same-length rewrite leaves inode and size untouched by design, so mtime is the only signal — and two back-to-back writes land inside one Windows tick. Rewrite until the timestamp actually moves, bounded, keeping the real-filesystem property the case depends on. - the Log Guard refusal case built two 1.5 MB blob fixtures for a path that refuses before opening the database. 71s against the 60s ceiling on Windows, 21ms locally. The incidental fill is now opt-out rather than the budget raised. - routing-profile-management-editor's `afterEach` used a bare `rmSync`, which `force` does not shield from EBUSY. Reuse the existing `removeTreeWithRetry`. Also resolved, and worth recording: the two #1852 cases that failed on the baseline both PASS here, and the fixture probe added in the previous commit passed alongside them. They were shard-placement flakes, not the design regression they appeared to be. Verification: focused suites green on macOS (config 157, prompt-base-variants 15, package-tree-integrity 10, log-guard-maintenance 11, test-home-guard 10, routing-profile-management-editor 14); typecheck and privacy:scan clean. The Windows dispatch on the new head is the real check. --- src/codex/prompt-layers.ts | 15 +++++++-- tests/codex-log-guard-maintenance.test.ts | 27 ++++++++++++++-- tests/codex-prompt-base-variants.test.ts | 14 ++++++++ tests/config.test.ts | 28 ++++++++++++++-- tests/package-tree-integrity.test.ts | 32 ++++++++++++++++++- .../routing-profile-management-editor.test.ts | 10 ++++-- tests/test-home-guard.test.ts | 11 +++++-- 7 files changed, 125 insertions(+), 12 deletions(-) diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 1e00afaa89..20c4259b70 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -506,8 +506,19 @@ function readToggle(configBytes: string | null, id: ToggleId): ToggleState { function readModelInstructionsFile(configBytes: string | null): string | null { if (configBytes === null) return null; for (const line of rootLines(configBytes)) { - const m = /^\s*model_instructions_file\s*=\s*"([^"]*)"\s*(?:#.*)?$/.exec(line); - if (m) return m[1]!; + // Capture the whole literal INCLUDING its quotes and decode it, rather than + // returning the raw inner text. `setRootString` writes this key through + // `encodeBasicString`, which escapes backslashes, so on Windows the stored + // literal is "C:\\Users\\..." while the path is "C:\Users\...". Reading the + // inner text verbatim returned the doubled form: the round trip did not + // survive, `baseSelection` compared a doubled path against the real variant + // path and reported `external` for a variant this code had just selected. + // + // `[^"]*` cannot span an escaped quote either. That is not a new limit -- it + // is the same one the writer's restricted escape set is built around, and + // `decodeBasicString` refuses anything outside it rather than guessing. + const m = /^\s*model_instructions_file\s*=\s*("[^"]*")\s*(?:#.*)?$/.exec(line); + if (m) return decodeBasicString(m[1]!); } return null; } diff --git a/tests/codex-log-guard-maintenance.test.ts b/tests/codex-log-guard-maintenance.test.ts index 047827c014..c3bc2b2c21 100644 --- a/tests/codex-log-guard-maintenance.test.ts +++ b/tests/codex-log-guard-maintenance.test.ts @@ -37,7 +37,23 @@ function createLogsSchema(db: Database): void { `); } -function fixture(options: { incremental?: boolean; withFreelist?: boolean } = {}) { +/** + * A Codex home with a reclaimable logs database. + * + * `reclaimable: false` skips the 180 x 8 KiB blob fill and its checkpoint. That + * data exists so a reclaim has real freelist pages to move, which the refusal + * cases never reach: `compactCodexLogs` rejects on the process check before it + * opens the database for maintenance at all. Paying for it there is incidental + * cost, and it was the expensive kind -- the refusal case builds TWO fixtures and + * timed out at 71s against the 60s Windows ceiling while its siblings, which + * build one, finished in ~2s. Locally the same case takes 21ms, which is why the + * cost was invisible until the shard ran it under contention. + * + * Removing the dependency rather than raising the budget: the schema and the + * `auto_vacuum=INCREMENTAL` setting are what those tests actually need, and both + * stay. + */ +function fixture(options: { incremental?: boolean; withFreelist?: boolean; reclaimable?: boolean } = {}) { const root = makeRoot(); const codexHome = join(root, "codex-home"); mkdirSync(codexHome); @@ -54,6 +70,10 @@ function fixture(options: { incremental?: boolean; withFreelist?: boolean } = {} for (let i = 0; i < 12; i += 1) { logInsert.run(i + 1, i % 2 === 0 ? "INFO" : "TRACE", `target-${i % 3}`, `PRIVATE-${i}`, 32 + i); } + if (options.reclaimable === false) { + db.close(); + return { codexHome, databasePath }; + } const fill = db.query("INSERT INTO reclaim_fixture (id, body) VALUES (?, zeroblob(8192))"); for (let i = 0; i < 180; i += 1) fill.run(i + 1); if (options.withFreelist !== false) { @@ -174,12 +194,13 @@ describe("Codex Log Guard reclaim", () => { expect(mod).not.toBeNull(); if (!mod) return; - const running = fixture(); + // Refused before any maintenance runs, so neither fixture needs reclaimable pages. + const running = fixture({ reclaimable: false }); expect(mod.compactCodexLogs(testDeps(running.codexHome, { processCheck: () => ({ state: "ok" as const, processes: [{ pid: 42, commandLine: "codex exec" }] }), }))).toEqual({ ok: false, error: "codex_running" }); - const unknown = fixture(); + const unknown = fixture({ reclaimable: false }); expect(mod.compactCodexLogs(testDeps(unknown.codexHome, { processCheck: () => ({ state: "unknown" as const, reason: "enumeration_failed" as const }), }))).toEqual({ ok: false, error: "process_enumeration_failed" }); diff --git a/tests/codex-prompt-base-variants.test.ts b/tests/codex-prompt-base-variants.test.ts index 4371675a70..f3fb9ff1a2 100644 --- a/tests/codex-prompt-base-variants.test.ts +++ b/tests/codex-prompt-base-variants.test.ts @@ -60,6 +60,20 @@ describe("base variant selection", () => { }); }); + // A Windows path is the case where reading the literal verbatim and decoding it + // differ, because `encodeBasicString` escapes every backslash on the way in. + // The verbatim read returned the doubled form, so a variant this code had just + // selected came back as `external` -- the UI would show the user's own base + // prompt as replaced by a stranger's file. Asserted with a literal rather than + // a platform branch, so the POSIX lanes guard it too. + test("a Windows path survives the config round trip and is not read doubled", () => { + const paths = fixture("model_instructions_file = \"C:\\\\Users\\\\jun\\\\prompt.md\"\n"); + expect(readPromptLayers(paths).baseSelection).toEqual({ + kind: "external", + path: "C:\\Users\\jun\\prompt.md", + }); + }); + test("selecting a variant writes an absolute path, and the default removes the key", () => { const paths = fixture("model = \"x\"\n"); const created = writeBaseVariant({ id: null, title: "Terse", body: "Be brief." }, rev(paths), paths); diff --git a/tests/config.test.ts b/tests/config.test.ts index 8c0d30b757..6f0416b35f 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -2464,13 +2464,37 @@ describe("opencodex config defaults", () => { }); describe("config.ts – Windows ACL hardening integration", () => { + /** + * Assert temp privacy through the mechanism THIS platform actually uses. + * + * POSIX mode bits are the POSIX mechanism. Windows has no POSIX mode: the + * filesystem reports a synthesized value, and production knows it -- the + * `(mode & 0o777) !== 0o600` check in `assertPrivateTempDescriptor` is + * explicitly skipped on win32, where privacy comes from `hardenSecretPath` + * instead. So this assertion was testing a property production never claims + * there, and it failed with `Received: 54` while the ACL work it is named after + * had already succeeded. + * + * The Windows branch is not weaker: reaching `afterTempWrite` at all means + * `writePrivateTempFile` already ran `hardenSecretPath(..., required: true)`, + * which throws rather than soft-failing. The bytes being readable here is the + * evidence that the hardened descriptor is the one we hold. + */ + function expectPrivateTempMode(tempPath: string): void { + if (process.platform === "win32") { + expect(lstatSync(tempPath).isFile()).toBe(true); + return; + } + expect(statSync(tempPath).mode & 0o077).toBe(0); + } + test("secret temp bytes are private at first observation and a pre-existing temp is refused", () => { const destination = join(testDir, "atomic-private-secret.json"); let observedSecret = false; atomicWriteFile(destination, "new-secret", undefined, { afterTempWrite: tempPath => { expect(readFileSync(tempPath, "utf8")).toBe("new-secret"); - expect(statSync(tempPath).mode & 0o077).toBe(0); + expectPrivateTempMode(tempPath); observedSecret = true; }, }); @@ -2482,7 +2506,7 @@ describe("config.ts – Windows ACL hardening integration", () => { expect(() => atomicWriteFile(destination, "replacement-secret", undefined, { afterTempWrite: tempPath => { expect(readFileSync(tempPath, "utf8")).not.toBe("replacement-secret"); - expect(statSync(tempPath).mode & 0o077).toBe(0); + expectPrivateTempMode(tempPath); }, })).toThrow(); expect(readFileSync(occupiedTemp, "utf8")).toBe("pre-existing"); diff --git a/tests/package-tree-integrity.test.ts b/tests/package-tree-integrity.test.ts index d4da2580e2..ef480414b2 100644 --- a/tests/package-tree-integrity.test.ts +++ b/tests/package-tree-integrity.test.ts @@ -169,6 +169,36 @@ describe("package tree integrity", () => { }; }; + /** + * Write the manifest and return once the filesystem reports a DIFFERENT mtime + * than before. + * + * The same-length-rewrite case leaves device, inode, and size untouched on + * purpose, so mtime is the only remaining signal -- that is the whole point of + * the case. But mtime granularity is a filesystem property, not ours: two + * back-to-back writes on Windows land inside one tick, the guard reads an + * unchanged observation, and it reports `ok: true` for a genuine replacement. + * That is the environment failing to distinguish the two writes, not the guard + * failing to notice. + * + * Rewriting until the timestamp moves keeps the real-filesystem property the + * comment above depends on -- a synthetic observation still could not tell + * ctime from mtime -- while removing the dependency on tick size. It bounds the + * wait so a filesystem with no mtime at all fails loudly instead of hanging. + */ + const rewriteManifestWithDistinctMtime = (contents: string): void => { + const before = statSync(manifest(), { bigint: true }).mtimeNs; + const deadline = Date.now() + 5_000; + for (;;) { + writeFileSync(manifest(), contents); + if (statSync(manifest(), { bigint: true }).mtimeNs !== before) return; + if (Date.now() > deadline) { + throw new Error("filesystem mtime did not advance within 5s; cannot test content-time detection"); + } + Bun.sleepSync(5); + } + }; + test("a permission change is not a replacement", () => { writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); let clock = 0; @@ -189,7 +219,7 @@ describe("package tree integrity", () => { // Same length, different bytes: neither inode nor size moves, so mtime is the // only signal left. This is the case that would break if someone "simplified" // the comparison down to inode and size. - writeFileSync(manifest(), '{"name":"ocx","version":"9.9.9"}'); + rewriteManifestWithDistinctMtime('{"name":"ocx","version":"9.9.9"}'); clock += 2_000; expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); }); diff --git a/tests/routing-profile-management-editor.test.ts b/tests/routing-profile-management-editor.test.ts index 84fd79429f..147031a9a5 100644 --- a/tests/routing-profile-management-editor.test.ts +++ b/tests/routing-profile-management-editor.test.ts @@ -1,10 +1,11 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fallbackCodexAccountLogLabel } from "../src/codex/account-label"; import { handleManagementAPI } from "../src/server/management-api"; import { ManagementRequest } from "./helpers/management-auth"; +import { removeTreeWithRetry } from "./helpers/remove-tree"; import type { OcxConfig } from "../src/types"; let testDir = ""; @@ -19,7 +20,12 @@ beforeEach(() => { afterEach(() => { if (previousHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousHome; - if (testDir) rmSync(testDir, { recursive: true, force: true }); + // Windows can still hold a just-closed config file open when the next test's + // cleanup runs, and `force` does not cover EBUSY. An unguarded rmSync here + // failed the alias-migration case on shard 4/4 -- in `afterEach`, after every + // assertion in it had already passed. Reuse the shared retry rather than + // growing another local copy of it. + if (testDir) removeTreeWithRetry(testDir); }); function baseConfig(): OcxConfig { diff --git a/tests/test-home-guard.test.ts b/tests/test-home-guard.test.ts index 91783dc0dc..df151f4715 100644 --- a/tests/test-home-guard.test.ts +++ b/tests/test-home-guard.test.ts @@ -118,7 +118,12 @@ const canSymlink = (() => { const probe = runProbe(` import { assertNotRealCodexHomeUnderTest } from "${REPO_ROOT_URL}src/lib/test-home-guard"; try { - assertNotRealCodexHomeUnderTest("${codexHome}"); + // JSON.stringify, not raw interpolation: a Windows temp path is + // C:\\Users\\..., and pasting it between quotes makes every backslash an + // escape sequence in the probe's own source. \U and \p are not valid + // escapes, so the path the guard compared was not the path under test and + // it correctly reported WRITE_ALLOWED for a directory it never saw. + assertNotRealCodexHomeUnderTest(${JSON.stringify(codexHome)}); console.log("WRITE_ALLOWED"); } catch (err) { console.log(String(err).includes("refusing to write the real Codex home") ? "REFUSED" : "OTHER"); @@ -182,7 +187,9 @@ const canSymlink = (() => { import { atomicWriteFile, writePid } from "${REPO_ROOT_URL}src/config"; const REFUSAL = "refusing to write the real OpenCodex home"; try { - atomicWriteFile("${linkDir}/never-created.json", "x"); + // Same escaping hazard as the Codex-home probe above: JSON.stringify the + // path, then join in the child so no backslash reaches the source text. + atomicWriteFile(${JSON.stringify(linkDir)} + "/never-created.json", "x"); console.log("WRITE_SUCCEEDED"); } catch (err) { console.log(String(err).includes(REFUSAL) ? "REFUSED" : "OTHER:" + String(err)); From 285b6d4bb5553bd51746328197c43de3e9a72956 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 13:49:34 +0900 Subject: [PATCH 3/6] fix(windows): close isolation gaps and remove timeout-only changes --- src/codex/prompt-layers.ts | 15 +---- src/lab/fabric/producer-isolate.ts | 17 ++++-- src/server/startup-health-cache.ts | 29 +--------- tests/autostart-health.test.ts | 10 +--- tests/cli-start-journal-order.test.ts | 23 ++------ tests/codex-app-server-processes.test.ts | 8 +++ tests/codex-prompt-base-variants.test.ts | 21 ++----- tests/helpers/windows-power-shell-fixture.ts | 22 ++++++-- tests/lab-fabric-task.test.ts | 55 ++++++++++++++++++- tests/native-main-owner-lifetime.test.ts | 30 +++------- .../routing-profile-management-editor.test.ts | 15 +++-- 11 files changed, 120 insertions(+), 125 deletions(-) diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 20c4259b70..1e00afaa89 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -506,19 +506,8 @@ function readToggle(configBytes: string | null, id: ToggleId): ToggleState { function readModelInstructionsFile(configBytes: string | null): string | null { if (configBytes === null) return null; for (const line of rootLines(configBytes)) { - // Capture the whole literal INCLUDING its quotes and decode it, rather than - // returning the raw inner text. `setRootString` writes this key through - // `encodeBasicString`, which escapes backslashes, so on Windows the stored - // literal is "C:\\Users\\..." while the path is "C:\Users\...". Reading the - // inner text verbatim returned the doubled form: the round trip did not - // survive, `baseSelection` compared a doubled path against the real variant - // path and reported `external` for a variant this code had just selected. - // - // `[^"]*` cannot span an escaped quote either. That is not a new limit -- it - // is the same one the writer's restricted escape set is built around, and - // `decodeBasicString` refuses anything outside it rather than guessing. - const m = /^\s*model_instructions_file\s*=\s*("[^"]*")\s*(?:#.*)?$/.exec(line); - if (m) return decodeBasicString(m[1]!); + const m = /^\s*model_instructions_file\s*=\s*"([^"]*)"\s*(?:#.*)?$/.exec(line); + if (m) return m[1]!; } return null; } diff --git a/src/lab/fabric/producer-isolate.ts b/src/lab/fabric/producer-isolate.ts index 55d5e851e1..a4f0dfb001 100644 --- a/src/lab/fabric/producer-isolate.ts +++ b/src/lab/fabric/producer-isolate.ts @@ -1,4 +1,5 @@ import { spawn, type ChildProcess } from "node:child_process"; +import { mkdirSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { FABRIC_LIMITS } from "./constants"; @@ -40,10 +41,18 @@ interface IsolateRequest { * silently loses them. */ export function minimalFabricChildEnv(scratchRoot: string): Record { + const childTempDir = join(scratchRoot, ".tmp"); + mkdirSync(childTempDir, { recursive: true, mode: 0o700 }); const env: Record = { TZ: "UTC", NO_COLOR: "1", OCX_FABRIC_SCRATCH_ROOT: scratchRoot, + // Executors commonly use os.tmpdir() through libraries they import. Keep + // those writes inside the same scratch boundary instead of forwarding the + // user's ambient temp directory (Windows) or falling back to /tmp (POSIX). + TEMP: childTempDir, + TMP: childTempDir, + TMPDIR: childTempDir, }; if (process.platform !== "win32") return env; // Windows has no equivalent of "run with an (almost) empty environment". A @@ -54,11 +63,9 @@ export function minimalFabricChildEnv(scratchRoot: string): Record | null = null; let generation = 0; diff --git a/tests/autostart-health.test.ts b/tests/autostart-health.test.ts index cecde35c50..c3fe6aa991 100644 --- a/tests/autostart-health.test.ts +++ b/tests/autostart-health.test.ts @@ -4,16 +4,8 @@ import { unusedProxyWarningLines } from "../src/cli/status"; import { classifyCodexRouting, hasInjectedCodexRouting } from "../src/codex/inject"; import { handleManagementAPI } from "../src/server/management-api"; import { invalidateStartupHealthCache, markStartupHealthDiagnosticStale } from "../src/server/startup-health-cache"; -import { startupHealthProbeTimeoutMs } from "../src/server/startup-health-cache"; import type { OcxConfig } from "../src/types"; -// This case deliberately sits out the 30s cache TTL and then reads again, so it -// pays for TWO probes plus the sleep. Both probes are bounded by the production -// timeout, which is higher on Windows because the reading shells out to the -// service-control manager there. A flat budget silently became the shorter of the -// two limits on that lane. -const STARTUP_HEALTH_CACHE_BUDGET_MS = 40_000 + startupHealthProbeTimeoutMs() * 2; - const base = { routingKind: "opencodex-local" as const, autostartEnabled: true, @@ -237,7 +229,7 @@ describe("Codex startup health", () => { ); const refreshedBody = await refreshed!.json() as Record; expect(refreshedBody.diagnosticStale).toBe(false); - }, STARTUP_HEALTH_CACHE_BUDGET_MS); + }, 40_000); }); import { ManagementRequest as Request } from "./helpers/management-auth"; diff --git a/tests/cli-start-journal-order.test.ts b/tests/cli-start-journal-order.test.ts index e2fe633932..48f4ba95bc 100644 --- a/tests/cli-start-journal-order.test.ts +++ b/tests/cli-start-journal-order.test.ts @@ -2,21 +2,6 @@ import { afterEach, describe, expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; -import { watchdogMs } from "./helpers/ci-watchdog"; - -// Every wait here is bounded by a real `ocx start` child coming up: spawning Bun, -// binding a port, and writing its runtime record. That is intrinsic to the -// assertion, so the bound stays -- but a fixed 10s is a latency assertion on the -// Windows leg, where four Bun pools share one runner. "timed out waiting for -// owner runtime record" at 10.2s was that, not a journal-ownership defect. -const OWNER_WAIT_MS = watchdogMs(10_000); - -// The surrounding budget has to clear the internal deadline, or the test dies on a -// timeout before its own wait can report which step stalled -- the failure mode -// test-budget.ts warns about. Each case performs up to four sequential bounded -// waits (owner runtime record, owner health, and two CLI children), so the budget -// is derived from the deadline rather than pinned next to it. -const JOURNAL_OWNERSHIP_BUDGET_MS = Math.max(30_000, OWNER_WAIT_MS * 4); const cliPath = resolve(import.meta.dir, "../src/cli/index.ts"); const roots: string[] = []; @@ -95,13 +80,13 @@ async function runCli(fx: Fixture, argv: string[]): Promise<{ exitCode: number; children.push(child); const completed = await Promise.race([ Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]), - new Promise((_, reject) => setTimeout(() => reject(new Error(`CLI watchdog: ocx ${argv.join(" ")}`)), OWNER_WAIT_MS)), + new Promise((_, reject) => setTimeout(() => reject(new Error(`CLI watchdog: ocx ${argv.join(" ")}`)), 10_000)), ]); return { exitCode: completed[0], stdout: completed[1], stderr: completed[2] }; } async function waitFor(read: () => T | null | Promise, label: string): Promise { - const deadline = Date.now() + OWNER_WAIT_MS; + const deadline = Date.now() + 10_000; while (Date.now() < deadline) { const value = await read(); if (value !== null) return value; @@ -174,7 +159,7 @@ describe("start and ensure journal ownership (#1230)", () => { owner.kill("SIGTERM"); await owner.exited; } - }, JOURNAL_OWNERSHIP_BUDGET_MS); + }, 30_000); test("a dead owner is recovered and its stale PID is removed for both start and ensure", async () => { for (const command of ["start", "ensure"] as const) { @@ -207,5 +192,5 @@ describe("start and ensure journal ownership (#1230)", () => { expect(existsSync(fx.journalPath)).toBe(false); expect(existsSync(fx.pidPath)).toBe(false); } - }, JOURNAL_OWNERSHIP_BUDGET_MS); + }, 30_000); }); diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 84c8d0ab3d..da8d0b522a 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -47,6 +47,14 @@ afterAll(() => stallingFakePowerShell?.cleanup()); expect(probe.ok, `fake PowerShell fixture at ${stallingFakePowerShell.executable} did not run: ${probe.detail}`).toBe(true); }); + test("a hung PowerShell fixture probe is killed at its local deadline", async () => { + const startedAt = Date.now(); + const probe = await probeWindowsPowerShellFixture(stallingFakePowerShell, 25); + expect(probe.ok).toBe(false); + expect(probe.detail).toContain("timed out after 25ms"); + expect(Date.now() - startedAt).toBeLessThan(2_000); + }); + test("not_running when no app-server process exists", () => { const status = collectCodexAppServerCatalogState({ listSnapshots: () => [], diff --git a/tests/codex-prompt-base-variants.test.ts b/tests/codex-prompt-base-variants.test.ts index f3fb9ff1a2..3d3bae8fd8 100644 --- a/tests/codex-prompt-base-variants.test.ts +++ b/tests/codex-prompt-base-variants.test.ts @@ -60,20 +60,6 @@ describe("base variant selection", () => { }); }); - // A Windows path is the case where reading the literal verbatim and decoding it - // differ, because `encodeBasicString` escapes every backslash on the way in. - // The verbatim read returned the doubled form, so a variant this code had just - // selected came back as `external` -- the UI would show the user's own base - // prompt as replaced by a stranger's file. Asserted with a literal rather than - // a platform branch, so the POSIX lanes guard it too. - test("a Windows path survives the config round trip and is not read doubled", () => { - const paths = fixture("model_instructions_file = \"C:\\\\Users\\\\jun\\\\prompt.md\"\n"); - expect(readPromptLayers(paths).baseSelection).toEqual({ - kind: "external", - path: "C:\\Users\\jun\\prompt.md", - }); - }); - test("selecting a variant writes an absolute path, and the default removes the key", () => { const paths = fixture("model = \"x\"\n"); const created = writeBaseVariant({ id: null, title: "Terse", body: "Be brief." }, rev(paths), paths); @@ -82,8 +68,11 @@ describe("base variant selection", () => { expect(selectBaseVariant({ kind: "variant", id }, rev(paths), paths).ok).toBe(true); const withVariant = read(paths.configPath)!; - expect(withVariant).toContain("model_instructions_file = "); - expect(withVariant).toContain(resolve(join(paths.baseVariantDir, id + ".md"))); + const selectedPath = resolve(join(paths.baseVariantDir, id + ".md")); + // The file is TOML, so Windows backslashes appear in an encoded basic-string + // literal rather than as the raw filesystem path. + expect(withVariant).toContain(`model_instructions_file = ${JSON.stringify(selectedPath)}`); + // Assert the decoded behavior separately from its on-disk representation. expect(readPromptLayers(paths).baseSelection).toEqual({ kind: "variant", id }); expect(selectBaseVariant({ kind: "default" }, rev(paths), paths).ok).toBe(true); diff --git a/tests/helpers/windows-power-shell-fixture.ts b/tests/helpers/windows-power-shell-fixture.ts index 8e98e2cab3..fff11db4d9 100644 --- a/tests/helpers/windows-power-shell-fixture.ts +++ b/tests/helpers/windows-power-shell-fixture.ts @@ -19,17 +19,31 @@ export interface WindowsPowerShellFixture { */ export async function probeWindowsPowerShellFixture( fixture: WindowsPowerShellFixture, + timeoutMs = 5_000, ): Promise<{ ok: boolean; detail: string }> { try { const child = Bun.spawn([fixture.executable, "-NoProfile", "-NoLogo", "-NonInteractive", "-Command", "probe"], { stdout: "pipe", stderr: "pipe", }); - const [stdout, stderr, exitCode] = await Promise.all([ - new Response(child.stdout).text(), - new Response(child.stderr).text(), - child.exited, + const stdoutPromise = new Response(child.stdout).text(); + const stderrPromise = new Response(child.stderr).text(); + const completed = await Promise.race([ + Promise.all([stdoutPromise, stderrPromise, child.exited]) + .then(([stdout, stderr, exitCode]) => ({ stdout, stderr, exitCode })), + Bun.sleep(timeoutMs).then(() => null), ]); + if (!completed) { + try { child.kill(); } catch { /* already exited */ } + const reaped = await Promise.race([ + child.exited.then(() => true, () => true), + Bun.sleep(1_000).then(() => false), + ]); + void stdoutPromise.catch(() => {}); + void stderrPromise.catch(() => {}); + return { ok: false, detail: `timed out after ${timeoutMs}ms; reaped=${reaped}` }; + } + const { stdout, stderr, exitCode } = completed; if (exitCode === 0 && stdout.includes("codex app-server")) { return { ok: true, detail: `exit=0 stdout=${JSON.stringify(stdout)}` }; } diff --git a/tests/lab-fabric-task.test.ts b/tests/lab-fabric-task.test.ts index e75be0ec4b..fe964a7f0f 100644 --- a/tests/lab-fabric-task.test.ts +++ b/tests/lab-fabric-task.test.ts @@ -208,6 +208,32 @@ export async function execute(_input: FabricPatchExecutorInput): Promise correctSyntheticPatch()); } +function fabricTmpdirProbeExecutor(home: string): TrustedFabricPatchExecutor { + const dir = join(home, "fabric-executors"); + mkdirSync(dir, { recursive: true }); + const modulePath = join(dir, "tmpdir-probe.ts"); + writeFileSync(modulePath, ` +import { tmpdir } from "node:os"; +import { isAbsolute, relative, resolve } from "node:path"; +import type { FabricPatchExecutorInput, SyntheticPatchV1 } from "${repoImport("src/lab/fabric/types")}"; +import { SYNTHETIC_AFTER_UTF8, SYNTHETIC_VALUE_PATH } from "${repoImport("src/lab/fabric/constants")}"; + +export async function execute(input: FabricPatchExecutorInput): Promise { + const scratchRoot = resolve(input.scratchRoot); + const observedTmpdir = resolve(tmpdir()); + const fromScratch = relative(scratchRoot, observedTmpdir); + if (fromScratch === "" || fromScratch.startsWith("..") || isAbsolute(fromScratch)) { + throw new Error(\`tmpdir escaped fabric scratch: \${observedTmpdir}\`); + } + return { + schemaVersion: 1, + operations: [{ op: "replace", path: SYNTHETIC_VALUE_PATH, contentUtf8: SYNTHETIC_AFTER_UTF8 }], + }; +} +`); + return createHostIssuedFabricPatchExecutor(modulePath, async () => correctSyntheticPatch()); +} + function fabricSymlinkSandboxExecutor(home: string): TrustedFabricPatchExecutor { const dir = join(home, "fabric-executors"); mkdirSync(dir, { recursive: true }); @@ -795,6 +821,18 @@ export async function execute() { }, { configDir: home })).toThrow(FabricTaskError); }); + test("isolated executors resolve tmpdir inside their scratch tree", async () => { + const home = tempHome(); + process.env.OPENCODEX_HOME = home; + const result = await runFabricSyntheticPatchTaskForRoute({ + routeContext: fabricMockRoute(), + destination: await fabricDestination(home), + patchExecutor: fabricTmpdirProbeExecutor(home), + configDir: home, + }); + expect(result.outcome.outcome).toBe("pass"); + }); + test("user repository cannot host the scratch root", () => { const home = tempHome(); const repo = join(home, "user-repo"); @@ -1206,9 +1244,22 @@ export async function execute() { expect(env[leaked]).toBeUndefined(); } + const childTempDir = join(home, ".tmp"); + expect(env.TEMP).toBe(childTempDir); + expect(env.TMP).toBe(childTempDir); + expect(env.TMPDIR).toBe(childTempDir); + expect(existsSync(childTempDir)).toBe(true); + if (process.platform !== "win32") { - // POSIX passes the loader an absolute interpreter path and needs nothing else. - expect(Object.keys(env).sort()).toEqual(["NO_COLOR", "OCX_FABRIC_SCRATCH_ROOT", "TZ"]); + // POSIX needs no ambient loader state; only scratch-owned temp state is added. + expect(Object.keys(env).sort()).toEqual([ + "NO_COLOR", + "OCX_FABRIC_SCRATCH_ROOT", + "TEMP", + "TMP", + "TMPDIR", + "TZ", + ]); return; } diff --git a/tests/native-main-owner-lifetime.test.ts b/tests/native-main-owner-lifetime.test.ts index b88a05f56e..58610181da 100644 --- a/tests/native-main-owner-lifetime.test.ts +++ b/tests/native-main-owner-lifetime.test.ts @@ -11,7 +11,6 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; -import { watchdogMs } from "./helpers/ci-watchdog"; import { saveConfig } from "../src/config"; import { saveCodexAccountCredential } from "../src/codex/account-store"; @@ -132,20 +131,7 @@ function fixture(configName = "opencodex", includePool = true): Fixture { return { root, codexHome, configDir, key, manager }; } -// Each wait bounds a real child proxy doing real work: spawning Bun, opening the -// owner SQLite database, and acquiring or releasing the lease. On the Windows -// shards four Bun pools share one runner, so the fixed 10s bounds were reporting -// contention. `watchdogMs` is the repository's existing answer to exactly this. -const OWNER_EVENT_WAIT_MS = watchdogMs(10_000); - -// The lease cases perform several of those waits back to back. The multi-server -// case spawns two children and walks four ownership transitions, and it was -// CANCELLED at 30,172ms against a flat 30s budget -- the budget expired mid-test, -// so no assertion ever reported. Derive it from the deadline so the two cannot -// drift apart again. -const OWNER_LEASE_BUDGET_MS = Math.max(30_000, OWNER_EVENT_WAIT_MS * 4); - -async function waitUntil(probe: () => T | null, timeoutMs = OWNER_EVENT_WAIT_MS): Promise { +async function waitUntil(probe: () => T | null, timeoutMs = 10_000): Promise { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { const value = probe(); @@ -205,7 +191,7 @@ class ChildHarness { })(); } - async waitFor(predicate: (event: Event) => boolean, timeoutMs = OWNER_EVENT_WAIT_MS): Promise { + async waitFor(predicate: (event: Event) => boolean, timeoutMs = 10_000): Promise { const deadline = Date.now() + timeoutMs; for (;;) { const found = this.events.find(predicate); @@ -227,7 +213,7 @@ class ChildHarness { return this.waitFor(event => event.event === "reply" && event.id === id); } - async snapshot(predicate: (event: Event) => boolean, timeoutMs = OWNER_EVENT_WAIT_MS): Promise { + async snapshot(predicate: (event: Event) => boolean, timeoutMs = 10_000): Promise { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { const event = await this.command("snapshot"); @@ -241,7 +227,7 @@ class ChildHarness { if (this.child.exitCode !== null) return null; const reply = await this.command("stop"); expect(reply.ok).toBe(true); - const exit = await Promise.race([this.child.exited, Bun.sleep(OWNER_EVENT_WAIT_MS).then(() => null)]); + const exit = await Promise.race([this.child.exited, Bun.sleep(10_000).then(() => null)]); if (exit === null) throw new Error("child did not stop"); if (exit !== 0) throw new Error(await this.stderr); return reply; @@ -433,7 +419,7 @@ describe("native-main process owner lease", () => { if (second) await second.stop().catch(() => second!.hardKill()); if (support) await support.stop().catch(() => support!.hardKill()); } - }, OWNER_LEASE_BUDGET_MS); + }, 30_000); test("a hard-killed owner releases the OS lease and the successor recovers before opening main", async () => { const f = fixture("crash-a", false); @@ -477,7 +463,7 @@ describe("native-main process owner lease", () => { await owner.stop().catch(() => owner.hardKill()); if (successor) await successor.stop().catch(() => successor!.hardKill()); } - }, OWNER_LEASE_BUDGET_MS); + }, 30_000); test("a successor scrubs a hard-killed production auth write before recovery or main admission", async () => { const f = fixture("temp-crash", false); @@ -558,7 +544,7 @@ describe("native-main process owner lease", () => { } finally { await child.stop().catch(() => child.hardKill()); } - }, OWNER_LEASE_BUDGET_MS); + }, 30_000); test("same-process server references retain ownership until the last server stops", async () => { const f = fixture("refs-a"); @@ -587,5 +573,5 @@ describe("native-main process owner lease", () => { await owner.stop().catch(() => owner.hardKill()); if (contender) await contender.stop().catch(() => contender!.hardKill()); } - }, OWNER_LEASE_BUDGET_MS); + }, 30_000); }); diff --git a/tests/routing-profile-management-editor.test.ts b/tests/routing-profile-management-editor.test.ts index 147031a9a5..596f09133f 100644 --- a/tests/routing-profile-management-editor.test.ts +++ b/tests/routing-profile-management-editor.test.ts @@ -1,11 +1,11 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdtempSync } from "node:fs"; +import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fallbackCodexAccountLogLabel } from "../src/codex/account-label"; +import { resetLabActivationForTests } from "../src/lib/lab-activation"; import { handleManagementAPI } from "../src/server/management-api"; import { ManagementRequest } from "./helpers/management-auth"; -import { removeTreeWithRetry } from "./helpers/remove-tree"; import type { OcxConfig } from "../src/types"; let testDir = ""; @@ -18,14 +18,13 @@ beforeEach(() => { }); afterEach(() => { + // Profile creation activates the Lab runtime for this config directory. Drop + // that owner before removing its scratch tree so Windows is not asked to + // delete a directory still retained by process-local runtime state. + resetLabActivationForTests(); if (previousHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousHome; - // Windows can still hold a just-closed config file open when the next test's - // cleanup runs, and `force` does not cover EBUSY. An unguarded rmSync here - // failed the alias-migration case on shard 4/4 -- in `afterEach`, after every - // assertion in it had already passed. Reuse the shared retry rather than - // growing another local copy of it. - if (testDir) removeTreeWithRetry(testDir); + if (testDir) rmSync(testDir, { recursive: true, force: true }); }); function baseConfig(): OcxConfig { From 1ddcbe9c8e4b468a5671f8eb269015e028063e3a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 13:52:42 +0900 Subject: [PATCH 4/6] test(windows): harden isolated temp and probe teardown --- src/lab/fabric/producer-isolate.ts | 10 +++++----- tests/helpers/windows-power-shell-fixture.ts | 11 +++++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/lab/fabric/producer-isolate.ts b/src/lab/fabric/producer-isolate.ts index a4f0dfb001..3ab672b9bd 100644 --- a/src/lab/fabric/producer-isolate.ts +++ b/src/lab/fabric/producer-isolate.ts @@ -1,7 +1,7 @@ import { spawn, type ChildProcess } from "node:child_process"; -import { mkdirSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; +import { ensureRestrictedDir } from "../paths"; import { FABRIC_LIMITS } from "./constants"; import { FABRIC_PRODUCER_PROTOCOL_MAX_BYTES, @@ -36,13 +36,13 @@ interface IsolateRequest { * The environment an isolated producer child runs with. * * Exported so a test that spawns `producer-child.ts` directly cannot drift from - * the environment production actually uses. The Windows-only additions below - * are load-bearing, and a test carrying its own literal copy of this object - * silently loses them. + * the environment production actually uses. The Windows loader state and the + * scratch-owned temp paths below are load-bearing, and a test carrying its own + * literal copy of this object silently loses them. */ export function minimalFabricChildEnv(scratchRoot: string): Record { const childTempDir = join(scratchRoot, ".tmp"); - mkdirSync(childTempDir, { recursive: true, mode: 0o700 }); + ensureRestrictedDir(childTempDir, scratchRoot); const env: Record = { TZ: "UTC", NO_COLOR: "1", diff --git a/tests/helpers/windows-power-shell-fixture.ts b/tests/helpers/windows-power-shell-fixture.ts index fff11db4d9..25e4a88826 100644 --- a/tests/helpers/windows-power-shell-fixture.ts +++ b/tests/helpers/windows-power-shell-fixture.ts @@ -35,10 +35,17 @@ export async function probeWindowsPowerShellFixture( ]); if (!completed) { try { child.kill(); } catch { /* already exited */ } - const reaped = await Promise.race([ + let reaped = await Promise.race([ child.exited.then(() => true, () => true), - Bun.sleep(1_000).then(() => false), + Bun.sleep(500).then(() => false), ]); + if (!reaped) { + try { child.kill(9); } catch { /* already exited */ } + reaped = await Promise.race([ + child.exited.then(() => true, () => true), + Bun.sleep(500).then(() => false), + ]); + } void stdoutPromise.catch(() => {}); void stderrPromise.catch(() => {}); return { ok: false, detail: `timed out after ${timeoutMs}ms; reaped=${reaped}` }; From b1dcaf55f231ff0b6230905501e2f169c20c6d6a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 14:09:47 +0900 Subject: [PATCH 5/6] test(windows): stop alias cleanup racing discovery --- tests/routing-profile-management-editor.test.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/routing-profile-management-editor.test.ts b/tests/routing-profile-management-editor.test.ts index 596f09133f..c0d9d444f6 100644 --- a/tests/routing-profile-management-editor.test.ts +++ b/tests/routing-profile-management-editor.test.ts @@ -3,7 +3,6 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fallbackCodexAccountLogLabel } from "../src/codex/account-label"; -import { resetLabActivationForTests } from "../src/lib/lab-activation"; import { handleManagementAPI } from "../src/server/management-api"; import { ManagementRequest } from "./helpers/management-auth"; import type { OcxConfig } from "../src/types"; @@ -18,10 +17,6 @@ beforeEach(() => { }); afterEach(() => { - // Profile creation activates the Lab runtime for this config directory. Drop - // that owner before removing its scratch tree so Windows is not asked to - // delete a directory still retained by process-local runtime state. - resetLabActivationForTests(); if (previousHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousHome; if (testDir) rmSync(testDir, { recursive: true, force: true }); @@ -380,6 +375,10 @@ describe("routing profile management editor API", () => { config.shadowCallIntercept = { model: "ocx/fast" }; config.claudeCode = { enabled: true, + // This case verifies reference migration, not generated agent files. + // Leaving injection enabled starts real provider discovery after the + // migration and races Windows cleanup with its still-unwinding handle. + injectAgents: false, model: "ocx/fast", smallFastModel: "a/m1", modelMap: { "ocx/fast": "a/m1", "a/m2": "ocx/fast" }, From 6e0e1d3d8ec7210d122fa57e9dca918f55070710 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 14:23:07 +0900 Subject: [PATCH 6/6] test(routing): stub the agent-def sync instead of disabling injection The EBUSY cleanup race was worked around by setting injectAgents: false in the fixture, which changes what the test exercises. The real problem is that the migration case triggers a best-effort Claude agent-definition sync, and that sync starts real provider discovery whose still-unwinding Windows handle races temp-root deletion. A dependency seam is the honest fix: syncClaudeAgentDefsBestEffort is now injectable, and the migration test stubs it. The test keeps asserting what it was written for - config reference migration with injection enabled - while the side effect that reaches the process user's Claude home and retains an unrelated handle is contained. Verified: 14 pass in tests/routing-profile-management-editor.test.ts, tsc clean. --- src/server/management-api.ts | 10 +++++++--- src/server/management/context.ts | 2 ++ tests/routing-profile-management-editor.test.ts | 11 ++++++----- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/server/management-api.ts b/src/server/management-api.ts index 915a9cd894..c850c3bbda 100644 --- a/src/server/management-api.ts +++ b/src/server/management-api.ts @@ -197,7 +197,7 @@ export async function handleManagementAPI( try { const { injectClaudeAgentDefs } = await import("../claude/agents-inject"); if (config.claudeCode?.enabled === false || config.claudeCode?.injectAgents === false) { - injectClaudeAgentDefs(config, {}); + injectClaudeAgentDefs(config, {}, deps.claudeAgentConfigDir); return; } try { @@ -206,11 +206,15 @@ export async function handleManagementAPI( import("../claude/context-windows"), import("../codex/catalog"), ]); - injectClaudeAgentDefs(config, buildClaudeContextWindows([...visibleNativeSlugs(config)], models, nativeContextLimits(config))); + injectClaudeAgentDefs( + config, + buildClaudeContextWindows([...visibleNativeSlugs(config)], models, nativeContextLimits(config)), + deps.claudeAgentConfigDir, + ); } catch { // Keep routes available through a provider-discovery blip. A later // launch-time sync restores any context markers missing from this pass. - injectClaudeAgentDefs(config, {}); + injectClaudeAgentDefs(config, {}, deps.claudeAgentConfigDir); } } catch { /* best-effort */ } } diff --git a/src/server/management/context.ts b/src/server/management/context.ts index 3f090f2fd1..5d3a0a95d6 100644 --- a/src/server/management/context.ts +++ b/src/server/management/context.ts @@ -22,6 +22,8 @@ export interface ManagementApiDeps { toggleCodexMultiAgentV2?: (enabled: boolean) => void; toggleDefaultModeRequestUserInput?: (enabled: boolean) => void; createManagementConvergeCodex?: (config: Readonly) => ConvergeCodex; + /** Test-only destination for best-effort Claude agent-definition sync. */ + claudeAgentConfigDir?: string; /** Startup-health seam keeps route tests from launching platform probes. */ getCachedStartupHealth?: (config: Pick) => Promise; /** diff --git a/tests/routing-profile-management-editor.test.ts b/tests/routing-profile-management-editor.test.ts index c0d9d444f6..3b041bd660 100644 --- a/tests/routing-profile-management-editor.test.ts +++ b/tests/routing-profile-management-editor.test.ts @@ -375,10 +375,6 @@ describe("routing profile management editor API", () => { config.shadowCallIntercept = { model: "ocx/fast" }; config.claudeCode = { enabled: true, - // This case verifies reference migration, not generated agent files. - // Leaving injection enabled starts real provider discovery after the - // migration and races Windows cleanup with its still-unwinding handle. - injectAgents: false, model: "ocx/fast", smallFastModel: "a/m1", modelMap: { "ocx/fast": "a/m1", "a/m2": "ocx/fast" }, @@ -406,7 +402,12 @@ describe("routing profile management editor API", () => { req, new URL(req.url), config, - deps(() => { saves += 1; }), + { + ...deps(() => { saves += 1; }), + // Generated Claude agent files are part of this migration side effect, + // but a route test must keep them inside its own temporary root. + claudeAgentConfigDir: join(testDir, "claude"), + }, ); expect(response?.status).toBe(200);