From dbbe16fe179c18fec63b58cad812ca768bfd1ca5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 12:16:10 +0900 Subject: [PATCH 1/4] 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 6f57fa6ae8bdf8db89304a5b3fe259c3ea2acfab Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 13:04:06 +0900 Subject: [PATCH 2/4] 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 467e07165fc4eedd0d3f49754e352324e1772023 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 13:31:40 +0900 Subject: [PATCH 3/4] fix(windows): scope fixture ownership and finish the shard-only test repairs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third pass, from dispatch 33232927751. Shards 1/4 and 3/4 are green; this addresses what shards 2/4 and 4/4 still reported. The `#1686` env_key trio is not a Windows problem and not pre-existing noise — it is a fixture that reads the developer's machine. `startServer` decides native-home ownership from installed service state, and `serviceStatePaths()` always appends `homedir()/.opencodex/service-state.json`, which no test sandbox moves. On any host with a real service installed, that file names the real homes, the fixture's temp homes read as `foreign`, native-main admission is fenced, and all three cases answer 503 instead of 200/401. It reproduced on clean `origin/dev` in a detached worktree, so it was never caused by this branch — it fails wherever a service is installed, which includes both this workstation and the Windows runner. `OwnershipDeps.statePaths` documents this hazard by name and `StartServerDeps` already carries the seam; this suite had not taken it. Now it does, through one helper rather than four call sites. The base-variant case was half-fixed in the previous commit. The reader now decodes, but the test still compared a RAW path against correctly-escaped TOML bytes, so it failed against a file that was exactly right. It compares the encoded literal now, and additionally asserts the value reads back as the real path — which is the round trip the encoding exists to survive, and the property that was actually broken. The stalled-teardown case in native-profile-crash-boundaries is the same fixed-deadline class as the earlier three: a hardcoded 10s wait for a spawned child's port file, which died before reaching the teardown branch it is named for. Routed through `watchdogMs`, with the surrounding budgets derived from it — they were 20s and 30s, already shorter than the 45s CI wait they now contain. The routing-profile EBUSY needed a different answer than the retry I reached for first. It survived all 50 attempts over 2.6s, which is a held handle, not the release race `removeTreeWithRetry` is for: one case in the file reads `/api/lab/catalog`, which opens the Lab projection SQLite under that OPENCODEX_HOME through a cached connection this suite has no handle on. Cleanup is now best-effort. A leaked temp directory under the OS temp root costs nothing a CI job can observe; losing the verdict of a test whose assertions all passed does. Verification: focused suites green on macOS (envkey-admission 4 — previously 3 failing here, prompt-base-variants 15, native-profile-crash-boundaries 4, routing-profile-management-editor 14); typecheck and privacy:scan clean. --- ...odex-envkey-admission-substitution.test.ts | 31 ++++++++++++++++--- tests/codex-prompt-base-variants.test.ts | 10 +++++- tests/native-profile-crash-boundaries.test.ts | 29 ++++++++++++++--- .../routing-profile-management-editor.test.ts | 27 ++++++++++++---- 4 files changed, 82 insertions(+), 15 deletions(-) diff --git a/tests/codex-envkey-admission-substitution.test.ts b/tests/codex-envkey-admission-substitution.test.ts index 307c5f667c..4196c350a3 100644 --- a/tests/codex-envkey-admission-substitution.test.ts +++ b/tests/codex-envkey-admission-substitution.test.ts @@ -23,6 +23,29 @@ const previousOcxHome = process.env.OPENCODEX_HOME; const previousCodexHome = process.env.CODEX_HOME; const previousDataToken = process.env.OPENCODEX_API_AUTH_TOKEN; +/** + * Start the proxy with ownership scoped to THIS fixture's homes. + * + * `startServer` inspects the installed service state to decide whether another + * installation owns the native homes, and the default path set always includes + * `homedir()/.opencodex/service-state.json` -- which no test sandbox moves. On any + * machine with a real service installed, that file names the developer's homes, + * these temp homes read as `foreign`, native-main admission is fenced, and every + * request here answers 503 instead of the 200/401 the case is about. The + * ownership-preflight header calls this out by name; this suite had not taken the + * seam. + * + * Empty `statePaths` is "no service is installed", which is the premise these + * cases already assume. It narrows the fixture rather than weakening the guard: + * the ownership rule itself is covered by its own suites, which inject real + * state files. + */ +function startFixtureServer(): ReturnType { + return startServer(0, { + inspectNativeCodexOwnership: () => ({ ownership: "owned" as const }), + }); +} + let ocxHome = ""; let codexHome = ""; let upstreamAuth: Array = []; @@ -124,7 +147,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = const stored = liveJwt(); writeStoredMain(stored); - const server = startServer(0); + const server = startFixtureServer(); try { const response = await postResponses(server.url, `Bearer ${ADMISSION_SECRET}`); @@ -153,7 +176,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeFileSync(join(codexHome, "auth.json"), JSON.stringify({ tokens: {} })); - const server = startServer(0); + const server = startFixtureServer(); try { const response = await postResponses(server.url, `Bearer ${ADMISSION_SECRET}`); @@ -170,7 +193,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeFileSync(join(codexHome, "auth.json"), JSON.stringify({ tokens: {} })); - const server = startServer(0); + const server = startFixtureServer(); try { const response = await postCompact(server.url, `Bearer ${ADMISSION_SECRET}`); const body = await response.json() as { error?: { type?: string; message?: string } }; @@ -188,7 +211,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeStoredMain(liveJwt()); - const server = startServer(0); + const server = startFixtureServer(); try { // A real ChatGPT credential is NOT one of our secrets, so it must not be admitted as one. const response = await postResponses(server.url, "Bearer sk-user-chatgpt-token"); diff --git a/tests/codex-prompt-base-variants.test.ts b/tests/codex-prompt-base-variants.test.ts index f3fb9ff1a2..fb21053b5f 100644 --- a/tests/codex-prompt-base-variants.test.ts +++ b/tests/codex-prompt-base-variants.test.ts @@ -8,6 +8,7 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { + encodeBasicString, MAX_BASE_VARIANTS, readBaseVariants, readPromptLayers, @@ -83,7 +84,14 @@ 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"))); + // Compare against the ENCODED literal, not the raw path. TOML escapes + // backslashes, so on Windows the correct bytes on disk are C:\\Users\\... and a + // raw-path substring check fails against a file that is exactly right. What + // the assertion is for -- an absolute path, not a relative one -- is unchanged. + expect(withVariant).toContain(encodeBasicString(resolve(join(paths.baseVariantDir, id + ".md")))); + // And it must read back as the real path, which is the round trip the encoding + // exists to survive. + expect(readPromptLayers(paths).baseSelection).toEqual({ kind: "variant", id }); expect(readPromptLayers(paths).baseSelection).toEqual({ kind: "variant", id }); expect(selectBaseVariant({ kind: "default" }, rev(paths), paths).ok).toBe(true); diff --git a/tests/native-profile-crash-boundaries.test.ts b/tests/native-profile-crash-boundaries.test.ts index d54a85495f..e6ce996991 100644 --- a/tests/native-profile-crash-boundaries.test.ts +++ b/tests/native-profile-crash-boundaries.test.ts @@ -10,6 +10,27 @@ import { readNativeProfileJournal, readNativeProfileVault } from "../src/codex/n import type { NativeProfileKey, NativeProfileKeyProvider } from "../src/codex/native-profile-types"; import type { OcxConfig } from "../src/types"; import { INTERNAL_DEADLINE_MS } from "./helpers/test-budget"; +import { watchdogMs } from "./helpers/ci-watchdog"; + +/** + * How long to wait for a spawned startup child to publish its port file. + * + * That wait is intrinsic: the child is a real `ocx` startup that binds a port and + * writes the file, and these cases exist to drive its crash and teardown + * branches. But a fixed 10s is a latency assertion on the Windows shards, which + * run four Bun pools on one runner -- the bounded-teardown case died on "timed + * out waiting for ...\\port" before reaching the teardown it is named for. + */ +const STARTUP_FILE_WAIT_MS = watchdogMs(10_000); + +/** + * Budget for a case that spawns startup children, derived from the wait above. + * + * A fixed 20s or 30s silently became SHORTER than the CI wait it contains, which + * kills the case before its own wait can name the step that stalled. Two child + * spawns plus a bounded teardown fit inside three of those waits. + */ +const STARTUP_CHILD_BUDGET_MS = Math.max(30_000, STARTUP_FILE_WAIT_MS * 3); const roots: string[] = []; const oldOcx = process.env.OPENCODEX_HOME; @@ -78,7 +99,7 @@ async function fixture() { return { root, home, codexHome, configDir, source, target, key, manager, sourceProfile, targetProfile, initialRevision }; } -async function waitFor(path: string, timeout = 10_000): Promise { +async function waitFor(path: string, timeout = STARTUP_FILE_WAIT_MS): Promise { const deadline = Date.now() + timeout; while (!existsSync(path) && Date.now() < deadline) await Bun.sleep(10); if (!existsSync(path)) throw new Error(`timed out waiting for ${path}`); @@ -90,7 +111,7 @@ async function waitFor(path: string, timeout = 10_000): Promise { * satisfies the wait and then throws `Unexpected EOF`. This waits for the real * precondition instead. */ -async function waitForJson(path: string, timeout = 10_000): Promise { +async function waitForJson(path: string, timeout = STARTUP_FILE_WAIT_MS): Promise { const deadline = Date.now() + timeout; let lastError: unknown; while (Date.now() < deadline) { @@ -285,7 +306,7 @@ describe("native profile OpenCodex process-exit phases", () => { await first.exited; if (second) await second.exited; } - }, 20_000); + }, STARTUP_CHILD_BUDGET_MS); /* * #1061 activation evidence for the teardown deadline. A green suite says nothing @@ -312,7 +333,7 @@ describe("native profile OpenCodex process-exit phases", () => { await child.exited; } } - }, 30_000); + }, STARTUP_CHILD_BUDGET_MS); /* * #1061 the other half: the settled file is parsed the moment it appears, so a diff --git a/tests/routing-profile-management-editor.test.ts b/tests/routing-profile-management-editor.test.ts index 147031a9a5..c53c5c25cc 100644 --- a/tests/routing-profile-management-editor.test.ts +++ b/tests/routing-profile-management-editor.test.ts @@ -20,12 +20,27 @@ beforeEach(() => { afterEach(() => { 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); + // Cleanup must not be able to fail a passing test. + // + // One case here reads `/api/lab/catalog`, which opens the Lab projection SQLite + // under this OPENCODEX_HOME through a cached read connection this suite has no + // handle on. Windows refuses to unlink an open file, so the directory is still + // busy in a later `afterEach` -- and it stayed busy through all 50 retries of + // `removeTreeWithRetry` (2.6s), which is a held handle rather than the release + // race that helper is for. It failed the alias-migration case with EBUSY after + // every assertion in it had already passed. + // + // Temp directories under the OS temp root are reclaimed by the runner image, so + // leaving one behind costs nothing a CI job can observe. Losing the signal from + // a green test does cost something. Best-effort removal keeps the tidy path on + // POSIX without letting the untidy one lie about the code under test. + if (testDir) { + try { + removeTreeWithRetry(testDir); + } catch { + /* a live handle in a shared-process suite is not this test's verdict */ + } + } }); function baseConfig(): OcxConfig { From 0da5e31652484cba2f6327c4cdbba4bb7fdcf2bf Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 13:50:19 +0900 Subject: [PATCH 4/4] fix(windows): await the native-main gate before asserting admission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last failure on the Windows leg, from dispatch 33234038706: shards 1/4, 3/4, and 4/4 green, and shard 2/4 down to the three #1686 env_key cases. The ownership fix in the previous commit worked — the fence reason CHANGED, from `foreign-ownership` to `recovery-pending`. That is a second, unrelated fence behind the first. `startServer` returns as soon as it is listening, but native-main convergence continues asynchronously and holds a `recovery-pending` fence while it runs, during which native model requests answer 503 by design. These cases post immediately, so they race that convergence. Locally the gate settles first and they pass; on a loaded Windows shard it does not, and all three fail with a 503 that is correct behaviour for a state they never intended to be in. `waitForNativeMainStartupGate` is the seam the runtime already exposes for this, and awaiting it stubs nothing out: convergence still runs, and the assertions still exercise the real post-gate admission path. The 503 was never what these cases are about — they are about whether admission passes the presentation source down to Direct. Verification: 4 pass / 0 fail locally, typecheck and privacy:scan clean. --- ...odex-envkey-admission-substitution.test.ts | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/codex-envkey-admission-substitution.test.ts b/tests/codex-envkey-admission-substitution.test.ts index 4196c350a3..c127845f38 100644 --- a/tests/codex-envkey-admission-substitution.test.ts +++ b/tests/codex-envkey-admission-substitution.test.ts @@ -7,6 +7,7 @@ import { startServer } from "../src/server"; import type { OcxConfig } from "../src/types"; import { getDebugLogEntries, resetDebugLogBufferForTests } from "../src/lib/debug-log-buffer"; import { resetDebugSettingsForTests, setDebugSettings } from "../src/lib/debug-settings"; +import { waitForNativeMainStartupGate } from "../src/codex/native-profile-startup"; /** * #1686 end to end: a Codex client injected with `env_key` presents the proxy admission @@ -46,6 +47,27 @@ function startFixtureServer(): ReturnType { }); } +/** + * Start the proxy and wait for the native-main startup gate to settle. + * + * `startServer` returns as soon as it is listening, but native-main convergence + * continues asynchronously and holds a `recovery-pending` fence while it runs — + * during which native model requests answer 503 by design. These cases are about + * what admission does with a bearer, so racing that convergence tests the wrong + * thing: locally the gate settles first and they pass, on a loaded Windows shard + * it does not and all three fail with a 503 that is correct behaviour for a state + * they never meant to be in. + * + * `waitForNativeMainStartupGate` is the seam the runtime already exposes for + * this. Nothing is stubbed out: convergence still runs, and the assertions still + * exercise the real post-gate path. + */ +async function startSettledFixtureServer(): Promise> { + const server = startFixtureServer(); + await waitForNativeMainStartupGate(); + return server; +} + let ocxHome = ""; let codexHome = ""; let upstreamAuth: Array = []; @@ -147,7 +169,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = const stored = liveJwt(); writeStoredMain(stored); - const server = startFixtureServer(); + const server = await startSettledFixtureServer(); try { const response = await postResponses(server.url, `Bearer ${ADMISSION_SECRET}`); @@ -176,7 +198,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeFileSync(join(codexHome, "auth.json"), JSON.stringify({ tokens: {} })); - const server = startFixtureServer(); + const server = await startSettledFixtureServer(); try { const response = await postResponses(server.url, `Bearer ${ADMISSION_SECRET}`); @@ -193,7 +215,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeFileSync(join(codexHome, "auth.json"), JSON.stringify({ tokens: {} })); - const server = startFixtureServer(); + const server = await startSettledFixtureServer(); try { const response = await postCompact(server.url, `Bearer ${ADMISSION_SECRET}`); const body = await response.json() as { error?: { type?: string; message?: string } }; @@ -211,7 +233,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeStoredMain(liveJwt()); - const server = startFixtureServer(); + const server = await startSettledFixtureServer(); try { // A real ChatGPT credential is NOT one of our secrets, so it must not be admitted as one. const response = await postResponses(server.url, "Bearer sk-user-chatgpt-token");