diff --git a/src/lab/fabric/producer-isolate.ts b/src/lab/fabric/producer-isolate.ts index 55d5e851e1..3ab672b9bd 100644 --- a/src/lab/fabric/producer-isolate.ts +++ b/src/lab/fabric/producer-isolate.ts @@ -1,6 +1,7 @@ import { spawn, type ChildProcess } from "node:child_process"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; +import { ensureRestrictedDir } from "../paths"; import { FABRIC_LIMITS } from "./constants"; import { FABRIC_PRODUCER_PROTOCOL_MAX_BYTES, @@ -35,15 +36,23 @@ interface IsolateRequest { * The environment an isolated producer child runs with. * * Exported so a test that spawns `producer-child.ts` directly cannot drift from - * the environment production actually uses. The Windows-only additions below - * are load-bearing, and a test carrying its own literal copy of this object - * silently loses them. + * the environment production actually uses. The Windows loader state and the + * scratch-owned temp paths below are load-bearing, and a test carrying its own + * literal copy of this object silently loses them. */ export function minimalFabricChildEnv(scratchRoot: string): Record { + const childTempDir = join(scratchRoot, ".tmp"); + ensureRestrictedDir(childTempDir, scratchRoot); const env: Record = { TZ: "UTC", NO_COLOR: "1", OCX_FABRIC_SCRATCH_ROOT: scratchRoot, + // Executors commonly use os.tmpdir() through libraries they import. Keep + // those writes inside the same scratch boundary instead of forwarding the + // user's ambient temp directory (Windows) or falling back to /tmp (POSIX). + TEMP: childTempDir, + TMP: childTempDir, + TMPDIR: childTempDir, }; if (process.platform !== "win32") return env; // Windows has no equivalent of "run with an (almost) empty environment". A @@ -54,11 +63,9 @@ export function minimalFabricChildEnv(scratchRoot: string): Record void; toggleDefaultModeRequestUserInput?: (enabled: boolean) => void; createManagementConvergeCodex?: (config: Readonly) => ConvergeCodex; + /** Test-only destination for best-effort Claude agent-definition sync. */ + claudeAgentConfigDir?: string; /** Startup-health seam keeps route tests from launching platform probes. */ getCachedStartupHealth?: (config: Pick) => Promise; /** diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 84c8d0ab3d..da8d0b522a 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -47,6 +47,14 @@ afterAll(() => stallingFakePowerShell?.cleanup()); expect(probe.ok, `fake PowerShell fixture at ${stallingFakePowerShell.executable} did not run: ${probe.detail}`).toBe(true); }); + test("a hung PowerShell fixture probe is killed at its local deadline", async () => { + const startedAt = Date.now(); + const probe = await probeWindowsPowerShellFixture(stallingFakePowerShell, 25); + expect(probe.ok).toBe(false); + expect(probe.detail).toContain("timed out after 25ms"); + expect(Date.now() - startedAt).toBeLessThan(2_000); + }); + test("not_running when no app-server process exists", () => { const status = collectCodexAppServerCatalogState({ listSnapshots: () => [], diff --git a/tests/codex-prompt-base-variants.test.ts b/tests/codex-prompt-base-variants.test.ts index fb21053b5f..3d3bae8fd8 100644 --- a/tests/codex-prompt-base-variants.test.ts +++ b/tests/codex-prompt-base-variants.test.ts @@ -8,7 +8,6 @@ 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, @@ -61,20 +60,6 @@ describe("base variant selection", () => { }); }); - // A Windows path is the case where reading the literal verbatim and decoding it - // differ, because `encodeBasicString` escapes every backslash on the way in. - // The verbatim read returned the doubled form, so a variant this code had just - // selected came back as `external` -- the UI would show the user's own base - // prompt as replaced by a stranger's file. Asserted with a literal rather than - // a platform branch, so the POSIX lanes guard it too. - test("a Windows path survives the config round trip and is not read doubled", () => { - const paths = fixture("model_instructions_file = \"C:\\\\Users\\\\jun\\\\prompt.md\"\n"); - expect(readPromptLayers(paths).baseSelection).toEqual({ - kind: "external", - path: "C:\\Users\\jun\\prompt.md", - }); - }); - test("selecting a variant writes an absolute path, and the default removes the key", () => { const paths = fixture("model = \"x\"\n"); const created = writeBaseVariant({ id: null, title: "Terse", body: "Be brief." }, rev(paths), paths); @@ -83,15 +68,11 @@ describe("base variant selection", () => { expect(selectBaseVariant({ kind: "variant", id }, rev(paths), paths).ok).toBe(true); const withVariant = read(paths.configPath)!; - expect(withVariant).toContain("model_instructions_file = "); - // 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 }); + const selectedPath = resolve(join(paths.baseVariantDir, id + ".md")); + // The file is TOML, so Windows backslashes appear in an encoded basic-string + // literal rather than as the raw filesystem path. + expect(withVariant).toContain(`model_instructions_file = ${JSON.stringify(selectedPath)}`); + // Assert the decoded behavior separately from its on-disk representation. expect(readPromptLayers(paths).baseSelection).toEqual({ kind: "variant", id }); expect(selectBaseVariant({ kind: "default" }, rev(paths), paths).ok).toBe(true); diff --git a/tests/helpers/windows-power-shell-fixture.ts b/tests/helpers/windows-power-shell-fixture.ts index 8e98e2cab3..25e4a88826 100644 --- a/tests/helpers/windows-power-shell-fixture.ts +++ b/tests/helpers/windows-power-shell-fixture.ts @@ -19,17 +19,38 @@ export interface WindowsPowerShellFixture { */ export async function probeWindowsPowerShellFixture( fixture: WindowsPowerShellFixture, + timeoutMs = 5_000, ): Promise<{ ok: boolean; detail: string }> { try { const child = Bun.spawn([fixture.executable, "-NoProfile", "-NoLogo", "-NonInteractive", "-Command", "probe"], { stdout: "pipe", stderr: "pipe", }); - const [stdout, stderr, exitCode] = await Promise.all([ - new Response(child.stdout).text(), - new Response(child.stderr).text(), - child.exited, + const stdoutPromise = new Response(child.stdout).text(); + const stderrPromise = new Response(child.stderr).text(); + const completed = await Promise.race([ + Promise.all([stdoutPromise, stderrPromise, child.exited]) + .then(([stdout, stderr, exitCode]) => ({ stdout, stderr, exitCode })), + Bun.sleep(timeoutMs).then(() => null), ]); + if (!completed) { + try { child.kill(); } catch { /* already exited */ } + let reaped = await Promise.race([ + child.exited.then(() => true, () => true), + Bun.sleep(500).then(() => false), + ]); + if (!reaped) { + try { child.kill(9); } catch { /* already exited */ } + reaped = await Promise.race([ + child.exited.then(() => true, () => true), + Bun.sleep(500).then(() => false), + ]); + } + void stdoutPromise.catch(() => {}); + void stderrPromise.catch(() => {}); + return { ok: false, detail: `timed out after ${timeoutMs}ms; reaped=${reaped}` }; + } + const { stdout, stderr, exitCode } = completed; if (exitCode === 0 && stdout.includes("codex app-server")) { return { ok: true, detail: `exit=0 stdout=${JSON.stringify(stdout)}` }; } diff --git a/tests/lab-fabric-task.test.ts b/tests/lab-fabric-task.test.ts index e75be0ec4b..fe964a7f0f 100644 --- a/tests/lab-fabric-task.test.ts +++ b/tests/lab-fabric-task.test.ts @@ -208,6 +208,32 @@ export async function execute(_input: FabricPatchExecutorInput): Promise correctSyntheticPatch()); } +function fabricTmpdirProbeExecutor(home: string): TrustedFabricPatchExecutor { + const dir = join(home, "fabric-executors"); + mkdirSync(dir, { recursive: true }); + const modulePath = join(dir, "tmpdir-probe.ts"); + writeFileSync(modulePath, ` +import { tmpdir } from "node:os"; +import { isAbsolute, relative, resolve } from "node:path"; +import type { FabricPatchExecutorInput, SyntheticPatchV1 } from "${repoImport("src/lab/fabric/types")}"; +import { SYNTHETIC_AFTER_UTF8, SYNTHETIC_VALUE_PATH } from "${repoImport("src/lab/fabric/constants")}"; + +export async function execute(input: FabricPatchExecutorInput): Promise { + const scratchRoot = resolve(input.scratchRoot); + const observedTmpdir = resolve(tmpdir()); + const fromScratch = relative(scratchRoot, observedTmpdir); + if (fromScratch === "" || fromScratch.startsWith("..") || isAbsolute(fromScratch)) { + throw new Error(\`tmpdir escaped fabric scratch: \${observedTmpdir}\`); + } + return { + schemaVersion: 1, + operations: [{ op: "replace", path: SYNTHETIC_VALUE_PATH, contentUtf8: SYNTHETIC_AFTER_UTF8 }], + }; +} +`); + return createHostIssuedFabricPatchExecutor(modulePath, async () => correctSyntheticPatch()); +} + function fabricSymlinkSandboxExecutor(home: string): TrustedFabricPatchExecutor { const dir = join(home, "fabric-executors"); mkdirSync(dir, { recursive: true }); @@ -795,6 +821,18 @@ export async function execute() { }, { configDir: home })).toThrow(FabricTaskError); }); + test("isolated executors resolve tmpdir inside their scratch tree", async () => { + const home = tempHome(); + process.env.OPENCODEX_HOME = home; + const result = await runFabricSyntheticPatchTaskForRoute({ + routeContext: fabricMockRoute(), + destination: await fabricDestination(home), + patchExecutor: fabricTmpdirProbeExecutor(home), + configDir: home, + }); + expect(result.outcome.outcome).toBe("pass"); + }); + test("user repository cannot host the scratch root", () => { const home = tempHome(); const repo = join(home, "user-repo"); @@ -1206,9 +1244,22 @@ export async function execute() { expect(env[leaked]).toBeUndefined(); } + const childTempDir = join(home, ".tmp"); + expect(env.TEMP).toBe(childTempDir); + expect(env.TMP).toBe(childTempDir); + expect(env.TMPDIR).toBe(childTempDir); + expect(existsSync(childTempDir)).toBe(true); + if (process.platform !== "win32") { - // POSIX passes the loader an absolute interpreter path and needs nothing else. - expect(Object.keys(env).sort()).toEqual(["NO_COLOR", "OCX_FABRIC_SCRATCH_ROOT", "TZ"]); + // POSIX needs no ambient loader state; only scratch-owned temp state is added. + expect(Object.keys(env).sort()).toEqual([ + "NO_COLOR", + "OCX_FABRIC_SCRATCH_ROOT", + "TEMP", + "TMP", + "TMPDIR", + "TZ", + ]); return; } diff --git a/tests/routing-profile-management-editor.test.ts b/tests/routing-profile-management-editor.test.ts index c53c5c25cc..93eae2f944 100644 --- a/tests/routing-profile-management-editor.test.ts +++ b/tests/routing-profile-management-editor.test.ts @@ -423,7 +423,12 @@ describe("routing profile management editor API", () => { req, new URL(req.url), config, - deps(() => { saves += 1; }), + { + ...deps(() => { saves += 1; }), + // Generated Claude agent files are part of this migration side effect, + // but a route test must keep them inside its own temporary root. + claudeAgentConfigDir: join(testDir, "claude"), + }, ); expect(response?.status).toBe(200);