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/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/codex-envkey-admission-substitution.test.ts b/tests/codex-envkey-admission-substitution.test.ts index 307c5f667c..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 @@ -23,6 +24,50 @@ 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 }), + }); +} + +/** + * 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 = []; @@ -124,7 +169,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = const stored = liveJwt(); writeStoredMain(stored); - const server = startServer(0); + const server = await startSettledFixtureServer(); try { const response = await postResponses(server.url, `Bearer ${ADMISSION_SECRET}`); @@ -153,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 = startServer(0); + const server = await startSettledFixtureServer(); try { const response = await postResponses(server.url, `Bearer ${ADMISSION_SECRET}`); @@ -170,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 = startServer(0); + 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 } }; @@ -188,7 +233,7 @@ describe("#1686 env_key bearer admission reaches Direct with substitution", () = saveConfig(directConfig()); writeStoredMain(liveJwt()); - const server = startServer(0); + 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"); 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..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, @@ -60,6 +61,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); @@ -69,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/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/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); }); 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/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..c53c5c25cc 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,27 @@ beforeEach(() => { afterEach(() => { if (previousHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousHome; - if (testDir) rmSync(testDir, { recursive: true, force: true }); + // 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 { 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));