diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a30a237812..02344a0aa2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -607,7 +607,11 @@ jobs: bun run build - name: Test - run: bun test --isolate tests --shard=${{ matrix.shard }}/4 + # --timeout: the Linux batches and the macOS control both pass 60000; this leg was + # the only one left on Bun's 5s default, and it is the slowest hardware on the board. + # Three of its failures were the default firing on tests that had not hung — the + # composed-acceptance cases spawn a real `ocx start` and were still working at 41s. + run: bun test --isolate --timeout 60000 tests --shard=${{ matrix.shard }}/4 - name: CLI help smoke run: bun run src/cli/index.ts help diff --git a/devlog/_plan/260819_unclaimed_bug_selection/180_windows_leg.md b/devlog/_plan/260819_unclaimed_bug_selection/180_windows_leg.md new file mode 100644 index 0000000000..ea7b42a5e4 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/180_windows_leg.md @@ -0,0 +1,91 @@ +# 180 — the Windows leg: five defects, and one I created + +PR #2143. The Windows shards were red before this work and are less red after +it; this records what was actually wrong, because "Windows is flaky" was the +wrong answer four separate times. + +## The correction that matters most + +I told the user twice that the Windows failures predated this release range. +The first half was true — the leg was red on `e446607c8` (2026-08-18). The +second half was not, and I stated it anyway. + +**Log Guard is new in `main...dev`.** It has never worked on Windows. Every +mutation — protect, unprotect, repair, reclaim, compact — refused with +`unsafe_path`. Shipping this range without looking would have released a +feature that is broken on one of three platforms, and my own "pre-existing" +verdict is what nearly let it through. + +The lesson is narrow and worth keeping: **"red before my change" and "not my +release's problem" are different claims.** A feature that landed on `dev` two +days earlier is still in the release. + +Compounding it: the run I compared against had shard 4/4 **cancelled**, so the +WP13 cases never executed there at all. I read "no failures listed" as "passed". + +## What was actually wrong + +| # | Defect | Where | Effect | +|---|---|---|---| +| 1 | `realpathSync.native` expands 8.3 short names (`RUNNER~1`), read as a symlink redirection | `log-guard/path-safety.ts` | 22 failures; Log Guard unusable on Windows | +| 2 | Windows shards ran on Bun's 5s default | `.github/workflows/ci.yml` | 3 failures on tests that had not hung | +| 3 | Test fixture's own `Bun.serve` used the default 10s idleTimeout | `codex-composed-acceptance.test.ts` | it cancelled the request the test was deliberately holding | +| 4 | 8s PowerShell identity budget, unreachable on a contended runner | `codex/user-identity.ts` | `effective-account lookup timed out` | +| 5 | Teardown aborted on the first child that would not exit | `codex-composed-acceptance.test.ts` | survivors killed by Bun's between-file sweep → the NEXT case failed with 143 | + +Number 5 is why the failures looked like a moving target: one slow case was +being charged to unrelated ones. + +## The defect I introduced + +My first fix for #1 re-canonicalized the requested path and compared the two +canonical forms. The caller already passes `realpathSync.native(requested)`, so +that compared a symlink against itself and **let through exactly what the guard +exists to refuse**. The Windows shard caught it as +`a symlinked database is still refused` flipping to fail. + +Second time in this branch that my fix to a fail-closed boundary created a +hole. Both were caught by the platform leg rather than by me. + +The shipped version is link-aware: a short-name expansion rewrites the spelling +of components that are all still directories, so requiring that no component of +the request is a link is sufficient, and any link fails closed. + +## Diagnostics were the actual unlock + +Two rounds produced only `timed out waiting for runtime-port record`. That is +the symptom. The fixture piped the child's streams and discarded them, so a +start that failed for a concrete reason reported nothing. + +Once the child's stderr reached the assertion message, the next round said +`CodexUserIdentityRefusal: Windows effective-account lookup timed out` and +defect 4 was obvious. Before that I was tuning timeouts against a message that +could not distinguish "slow" from "refused". + +Worth noting the budget fix then needed a second commit anyway: `env()` in the +fixture is a deliberate whitelist, so `CI` never reached the child and it kept +the 8s desktop ceiling. + +## Result + +| | before | after | +|---|---|---| +| shard 3/4 | failure | **success** | +| Log Guard | 22 fail | **0** | +| 5s-default | 3 fail | **0** | +| identity lookup | 4 fail | **0** | +| WP13 | 6 fail | 3 fail | + +## What is still red, and why it is not this range + +- **WP13 (3)** — zero commits in `main..dev`, and no Windows run has ever + executed them to completion. Each case starts a real server more than once; + the remaining failures are runner cost, now that the cascade is gone. +- **npm cache preflight (3)** — zero commits in `main..dev`. Symlink-creation + tests on a runner where an unprivileged user cannot create symlinks. +- **shard 2 Bun panic** — `Internal assertion failure`, a runtime crash, not a + test result. + +Fixing those means redesigning a test harness that predates this release. That +is a real piece of work and it is not this one. + diff --git a/src/codex/log-guard/path-safety.ts b/src/codex/log-guard/path-safety.ts index e1f0ee0736..6dabd1dd7d 100644 --- a/src/codex/log-guard/path-safety.ts +++ b/src/codex/log-guard/path-safety.ts @@ -1,5 +1,5 @@ -import { realpathSync } from "node:fs"; -import { resolve, sep } from "node:path"; +import { lstatSync, realpathSync } from "node:fs"; +import { dirname, resolve, sep } from "node:path"; import { samePathIdentity } from "../user-identity"; @@ -35,5 +35,54 @@ export function normalizeTrustedDarwinSystemAlias(path: string): string { * Arbitrary ancestor symlinks remain refused. */ export function sameLogGuardPathIdentity(realPath: string, requestedPath: string): boolean { - return samePathIdentity(realPath, normalizeTrustedDarwinSystemAlias(requestedPath)); + const requested = normalizeTrustedDarwinSystemAlias(requestedPath); + if (samePathIdentity(realPath, requested)) return true; + return sameWindowsCanonicalPath(realPath, requested); +} + +/** + * On Windows, is the difference between these two spellings the OS canonicalizing the + * request rather than a redirection? + * + * `realpathSync.native` expands 8.3 short components — the `RUNNER~1` form that appears + * throughout `%TEMP%` — so the canonical path and the requested path can disagree as + * strings while naming the same file. Reading that as an ancestor-symlink redirection made + * every Log Guard mutation refuse with `unsafe_path` on Windows, which is what the CI shards + * were reporting. + * + * The first version of this re-canonicalized the requested path and compared the two + * canonical forms. That was wrong, and the Windows shard proved it: the caller already + * passes `realpathSync.native(requested)` as `realPath`, so re-resolving the request + * produced the same value on BOTH sides and a symlinked database compared equal. The + * widening let through exactly what the guard exists to refuse. + * + * The comparison is therefore link-aware. A short-name expansion rewrites the spelling of + * components that are all still directories on the same chain, so it is enough to require + * that no component of the request is a link: with none present, any remaining difference + * is the OS's own canonical spelling. A symlink or junction anywhere in the chain fails + * closed as before. + */ +function sameWindowsCanonicalPath(realPath: string, requestedPath: string): boolean { + if (process.platform !== "win32") return false; + try { + if (pathChainContainsLink(requestedPath)) return false; + return samePathIdentity(realPath, realpathSync.native(requestedPath)); + } catch { + return false; + } +} + +/** Is any component of this path a symlink or junction? Fails closed on an unreadable one. */ +function pathChainContainsLink(path: string): boolean { + let current = resolve(path); + for (;;) { + try { + if (lstatSync(current).isSymbolicLink()) return true; + } catch { + return true; + } + const parent = dirname(current); + if (parent === current) return false; + current = parent; + } } diff --git a/src/codex/user-identity.ts b/src/codex/user-identity.ts index 716d06b4cb..05d1a5bc10 100644 --- a/src/codex/user-identity.ts +++ b/src/codex/user-identity.ts @@ -39,6 +39,26 @@ const SID_PATTERN = /^S-1-(?:\d+-)+\d+$/i; */ const WINDOWS_POWERSHELL_LOOKUP_TIMEOUT_MS = 8_000; +/** + * The same budget, widened for a contended CI runner. + * + * 8s is a generous ceiling for `powershell.exe -Command` on a real desktop and is not one + * on a GitHub Windows runner executing a quarter of this suite: the composed-acceptance + * cases fail there with "Windows effective-account lookup timed out" while the child is + * still starting. That is runner contention, not a hung lookup, and the budget exists to + * bound the latter. + * + * Gated on `CI` alone. A user's machine keeps the 8s ceiling exactly as before, so the + * recoverable-refusal contract this budget protects is unchanged where it matters. + */ +const WINDOWS_POWERSHELL_LOOKUP_TIMEOUT_CI_MS = 30_000; + +function windowsIdentityLookupTimeoutMs(): number { + return process.env.CI === "true" + ? WINDOWS_POWERSHELL_LOOKUP_TIMEOUT_CI_MS + : WINDOWS_POWERSHELL_LOOKUP_TIMEOUT_MS; +} + /** * FOLDERID_LocalAppData, and the flag that makes the lookup ignore the caller's * environment. @@ -108,7 +128,7 @@ function windowsIdentityPowerShellSpawnOptions(): { stdin: "ignore", stdout: "pipe", stderr: "pipe", - timeout: WINDOWS_POWERSHELL_LOOKUP_TIMEOUT_MS, + timeout: windowsIdentityLookupTimeoutMs(), windowsHide: true, }; } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index cabff33a78..cdcf0d9119 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -219,7 +219,11 @@ describe("GitHub Actions hardening", () => { // the runner's disk and the suite passes against a tree that no longer // exists in git. const winSteps = (ci.jobs?.["platform-windows"] as { steps?: { if?: string; run?: string }[] })?.steps ?? []; - const windowsTestCommand = `bun test --isolate tests --shard=\${{ matrix.shard }}/${windowsShards.length}`; + // --timeout is part of the contract, not incidental: this leg ran on Bun's 5s default + // while Linux and macOS both pass 60000, and it is the slowest hardware on the board. + // Three composed-acceptance failures were that default firing on tests still working + // at 41s. Pin the flag so the leg cannot silently drift back to the default. + const windowsTestCommand = `bun test --isolate --timeout 60000 tests --shard=\${{ matrix.shard }}/${windowsShards.length}`; expect(hasExactShellCommand(`echo ${windowsTestCommand}`, windowsTestCommand)).toBe(false); // Binding the assertion to an executable line is only half the guarantee: a // step carrying the exact command still runs nothing under `if: false`, and diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 857c27b1a8..ded130e17e 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -24,6 +24,15 @@ import { join, relative, resolve } from "node:path"; import { createHash } from "node:crypto"; import { Database } from "bun:sqlite"; +import { watchdogMs } from "./helpers/ci-watchdog"; + +/** + * Per-case budget. A case can start a server twice and stop it, so it must exceed the sum of + * the watchdogs inside it or the case dies before the watchdog it was meant to bound can + * report anything useful. On CI those watchdogs take the 30s floor, so this scales with them. + */ +const CASE_TIMEOUT_MS = process.env.CI === "true" ? 150_000 : 45_000; + import { canonicalizeCodexHome, } from "../src/codex/codex-write-lock"; @@ -41,7 +50,13 @@ const roots: Fixture[] = []; type CliResult = { exitCode: number; stdout: string; stderr: string }; type RuntimeRecord = { pid: number; port: number; hostname?: string }; -type StartedServer = { process: ReturnType; runtime: RuntimeRecord }; +type StartedServer = { + process: ReturnType; + runtime: RuntimeRecord; + /** Captured during start(): the child's streams can only be read once. */ + stdout: Promise; + stderr: Promise; +}; /** A byte manifest: paths plus bytes, not mtimes or parsed JSON. */ function manifest(root: string): Record { @@ -67,7 +82,16 @@ function manifestWithoutCatalogArtifacts(entries: Record): Recor ); } -async function waitFor(read: () => T | null | Promise, label: string, timeoutMs = 10_000): Promise { +async function waitFor( + read: () => T | null | Promise, + label: string, + // These wait on a REAL `ocx start` child: spawn a Bun runtime, load the CLI, read config, + // bind a port, then publish runtime-port.json. On the Windows shards that exceeded 10s + // while the child was still alive and still working — `child exit=null` with both streams + // open, which is a slow start, not a crash. The watchdog exists to bound a hung test, not + // to assert startup latency, so it takes the repository's CI floor. + timeoutMs = watchdogMs(10_000), +): Promise { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { const value = await read(); @@ -127,6 +151,11 @@ class Fixture { // A fixed fixture value avoids reading the generated credential file. OPENCODEX_ADMIN_AUTH_TOKEN: this.managementToken, NO_PROXY: "127.0.0.1,localhost", + // The env is a whitelist, so CI does not reach the child unless it is named. It must: + // the CLI's Windows identity lookup keeps an 8s budget locally and widens on CI, and + // without this the child spawned by a CI runner refuses with "Windows effective-account + // lookup timed out" while powershell.exe is still starting. + ...(process.env.CI === "true" ? { CI: "true" } : {}), ...this.serviceManagerEnv, }; } @@ -163,7 +192,12 @@ class Fixture { return child; } - async runCli(argv: string[], home = this.homeA, userprofile = this.userprofileA, timeoutMs = 15_000): Promise { + async runCli( + argv: string[], + home = this.homeA, + userprofile = this.userprofileA, + timeoutMs = watchdogMs(15_000), + ): Promise { const child = this.spawnCli(argv, home, userprofile); const completed = await Promise.race([ Promise.all([new Response(child.stdout).text(), new Response(child.stderr).text(), child.exited]), @@ -176,6 +210,23 @@ class Fixture { async start(): Promise { const child = this.spawnCli(["start"]); const runtimePath = join(this.ocx, "runtime-port.json"); + // Capture the child's streams while we wait. Without this, a start that dies for a + // concrete reason — a throw, a port bind refusal, a missing artifact — surfaces only as + // "timed out waiting for runtime-port record", which is the symptom and never the cause. + // That is exactly how the Windows failures read for two CI rounds. + const stderr = new Response(child.stderr).text(); + const stdout = new Response(child.stdout).text(); + const diagnose = async (label: string): Promise => { + const exited = child.exitCode ?? (await Promise.race([ + child.exited, + new Promise(resolve => setTimeout(() => resolve(null), 500)), + ])); + const [err, out] = await Promise.all([ + Promise.race([stderr, new Promise(resolve => setTimeout(() => resolve(""), 500))]), + Promise.race([stdout, new Promise(resolve => setTimeout(() => resolve(""), 500))]), + ]); + throw new Error(`${label}; child exit=${String(exited)}\n--- stderr ---\n${err.slice(-4000)}\n--- stdout ---\n${out.slice(-2000)}`); + }; const runtime = await waitFor(() => { if (!existsSync(runtimePath)) return null; try { @@ -186,7 +237,7 @@ class Fixture { } catch { return null; } - }, "runtime-port record"); + }, "runtime-port record").catch(() => diagnose("timed out waiting for runtime-port record")); const health = await waitFor(async () => { try { const response = await fetch(`http://127.0.0.1:${runtime.port}/healthz`, { signal: AbortSignal.timeout(500) }); @@ -197,14 +248,14 @@ class Fixture { } }, "child /healthz"); expect(health).toMatchObject({ pid: child.pid, port: runtime.port }); - return { process: child, runtime }; + return { process: child, runtime, stdout, stderr }; } async stop(server: StartedServer): Promise { if (server.process.exitCode === null) server.process.kill("SIGTERM"); const exitCode = await Promise.race([ server.process.exited, - new Promise((_, reject) => setTimeout(() => reject(new Error("server shutdown watchdog")), 10_000)), + new Promise((_, reject) => setTimeout(() => reject(new Error("server shutdown watchdog")), watchdogMs(10_000))), ]); // Bun reports a forced SIGTERM as 128 + 15 on Windows; POSIX children may // run the CLI shutdown handler and exit cleanly instead. @@ -230,13 +281,34 @@ class Fixture { } async cleanup(): Promise { + // Teardown must not be able to leave a child behind. A case that timed out has a live + // `ocx start`, and if the wait below throws — or an earlier child refuses SIGTERM — the + // rest of this loop never runs. The survivor is then killed by Bun's between-file + // "killed N dangling process" sweep, which on the Windows shard surfaced as the NEXT + // case failing with exit 143: one slow case cascading into unrelated ones. + // + // So: SIGTERM every child, wait for each independently, then SIGKILL whatever is still + // alive. Errors are collected rather than thrown mid-loop. for (const child of this.children) { if (child.exitCode === null) child.kill("SIGTERM"); } + const stubborn: Array> = []; for (const child of this.children) { - if (child.exitCode === null) await Promise.race([ + if (child.exitCode === null) { + const exited = await Promise.race([ + child.exited.then(() => true), + new Promise(resolve => setTimeout(() => resolve(false), 10_000)), + ]); + if (!exited) stubborn.push(child); + } + } + for (const child of stubborn) { + // SIGKILL is not graceful and does not need to be: the case is already over, and a + // survivor is strictly worse than an ungraceful exit. + try { child.kill("SIGKILL"); } catch { /* already gone */ } + await Promise.race([ child.exited, - new Promise((_, reject) => setTimeout(() => reject(new Error(`child ${child.pid} did not exit`)), 10_000)), + new Promise(resolve => setTimeout(resolve, 2_000)), ]); } // Re-resolve before the limited four-name removal: never glob or inspect a @@ -257,7 +329,19 @@ function fixture(): Fixture { } afterEach(async () => { - while (roots.length) await roots.pop()!.cleanup(); + // One fixture's teardown failure must not strand the next fixture's children. Drain every + // fixture, then report. Without this, a throw here leaves live `ocx start` processes for + // Bun's between-file sweep to kill, and the next case fails with exit 143 for a reason + // that has nothing to do with it. + const failures: unknown[] = []; + while (roots.length) { + try { + await roots.pop()!.cleanup(); + } catch (error) { + failures.push(error); + } + } + if (failures.length > 0) throw failures[0]; }); describe("WP13 composed toggle acceptance", () => { @@ -294,7 +378,7 @@ describe("WP13 composed toggle acceptance", () => { } finally { await fx.stop(server); } - }, 45_000); + }, CASE_TIMEOUT_MS); /** * RED: remove shouldSyncCodexOnStart or the under-lock desired-state read; an @@ -342,14 +426,16 @@ describe("WP13 composed toggle acceptance", () => { // The fixture records itself as the active service install, so the // production ownership preflight admits this home and P08 completes the // enable transition through the real CLI. - expect(back.exitCode).toBe(0); + // The CLI's own output is the assertion message: a bare "expected 0, got 1" sent two + // Windows CI rounds chasing a timeout that was never the cause. + expect(`exit=${back.exitCode}\nstderr: ${back.stderr}\nstdout: ${back.stdout}`).toContain("exit=0"); expect((await fx.request(server.runtime, "/api/native-integrations/codex", { method: "PUT", body: JSON.stringify({ enabled: false }), })).body).toMatchObject({ desiredEnabled: false }); } finally { await fx.stop(server); } - }, 45_000); + }, CASE_TIMEOUT_MS); /** RED: bypass the persisted OFF mutation or the under-lock re-read; stale P19 writes its candidate after gather. */ test("B-reduced: a held local provider cannot commit after the HTTP route persists OFF", async () => { @@ -361,6 +447,12 @@ describe("WP13 composed toggle acceptance", () => { const enteredGather = new Promise(resolveEntered => { entered = resolveEntered; }); const provider = Bun.serve({ port: 0, + // This fixture HOLDS the /models response open on purpose — that hold is the test's + // instrument for keeping a provider-discovery request in flight while the toggle flips. + // Bun's default request idleTimeout is 10s, so on a loaded Windows shard the runtime + // cancelled the very request the test was holding and the assertion saw a 500 instead + // of the 200 it was waiting for. The hold is bounded by `released`, not by this value. + idleTimeout: 255, fetch: async request => { if (new URL(request.url).pathname.endsWith("/models")) { if (hold) { @@ -411,7 +503,7 @@ describe("WP13 composed toggle acceptance", () => { } finally { provider.stop(true); } - }, 45_000); + }, CASE_TIMEOUT_MS); /** RED: omit `admitCodexWrite` ownership refusal; start/ensure/P19 create a coordinator or native artifact. */ test("D-reduced: foreign service-home evidence refuses real CLI and HTTP writers before artifacts", async () => { @@ -469,7 +561,7 @@ describe("WP13 composed toggle acceptance", () => { } finally { await fx.stop(server); } - }, 45_000); + }, CASE_TIMEOUT_MS); test("D-unknown: unprovable service-home ownership refuses native reads and cache writes", async () => { const fx = fixture(); @@ -514,7 +606,7 @@ describe("WP13 composed toggle acceptance", () => { } finally { await fx.stop(server); } - }, 45_000); + }, CASE_TIMEOUT_MS); /** RED: key N by HOME/USERPROFILE instead of effective uid plus canonical CODEX_HOME; both children acquire. */ test("E: separate fake homes share the effective-user Codex lock", async () => { @@ -550,7 +642,7 @@ describe("WP13 composed toggle acceptance", () => { expect(existsSync(join(fx.homeB, "native-write-locks"))).toBe(false); writeFileSync(release, "release"); expect(await holder.exited).toBe(0); - }, 30_000); + }, CASE_TIMEOUT_MS); /** RED: delete the durable Grok intent or bypass `shouldSyncGrokOnStart`; startup recreates the fence. */ test("Grok E2E: route-disabled Grok stays absent across a real startup", async () => { @@ -570,14 +662,14 @@ describe("WP13 composed toggle acceptance", () => { await fx.stop(first); } const second = await fx.start(); - const secondOutput = new Response(second.process.stdout).text(); + const secondOutput = second.stdout; try { expect(readFileSync(join(grokHome, "config.toml"), "utf8")).not.toContain("opencodex managed block"); } finally { await fx.stop(second); } expect(await secondOutput).not.toContain("Grok Build config updated"); - }, 45_000); + }, CASE_TIMEOUT_MS); /** RED: report restore success after a blocked history worker; config recovery must not hide history contention. */ test("Restore truth: JSON distinguishes a busy history restore from native artifact recovery", async () => { @@ -625,7 +717,7 @@ describe("WP13 composed toggle acceptance", () => { // A 15 s watchdog left almost no margin and fired on a loaded macOS runner // (dev CI run 31105071651). Give the wait its budget plus real headroom; // the case's own 45 s test timeout still bounds it. - const blocked = await fx.runCli(["restore", "--json"], fx.homeA, fx.userprofileA, 30_000); + const blocked = await fx.runCli(["restore", "--json"], fx.homeA, fx.userprofileA, watchdogMs(30_000)); expect(blocked.exitCode).toBe(1); const envelope = JSON.parse(blocked.stdout) as { success: boolean; artifacts: { history: { state: string; reason?: string } } }; expect(envelope).toMatchObject({ success: false, artifacts: { history: { state: "failed", reason: "busy" } } }); @@ -639,5 +731,5 @@ describe("WP13 composed toggle acceptance", () => { const after = new Database(stateDb, { readonly: true }); expect(after.query<{ model_provider: string }, []>("SELECT model_provider FROM threads WHERE id = 'restore-1'").get()?.model_provider).toBe("openai"); after.close(); - }, 45_000); + }, CASE_TIMEOUT_MS); }); diff --git a/tests/codex-log-guard-coderabbit.test.ts b/tests/codex-log-guard-coderabbit.test.ts index d510323057..4fa57a6c78 100644 --- a/tests/codex-log-guard-coderabbit.test.ts +++ b/tests/codex-log-guard-coderabbit.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, test } from "bun:test"; import { Database } from "bun:sqlite"; -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -205,3 +205,51 @@ describe("CodeRabbit protection regressions", () => { expect(reservedTriggers(databasePath)).toEqual(before); }); }); + +/* + * Windows CI reported 22 Log Guard failures as `unsafe_path`, on a feature (#1729) that is + * new in this release range and has therefore never shipped. + * + * `realpathSync.native` on Windows expands 8.3 short components — the `RUNNER~1` form that + * appears throughout %TEMP% — so the canonical realpath and the requested path disagree as + * strings while naming the same file. The safety check read that as an ancestor-symlink + * redirection and refused every mutation. macOS had the same class of problem with /var and + * /tmp and was given an explicit alias normalizer; Windows was not. + */ +describe("log guard path identity survives OS canonicalization", () => { + test("a path that only differs by the OS's own canonical spelling is the same file", () => { + const root = mkdtempSync(join(tmpdir(), "ocx-log-guard-identity-")); + roots.push(root); + const path = join(root, "logs_2.sqlite"); + writeFileSync(path, ""); + + // realpathSync.native is exactly what databasePathIsSafe compares against. + expect(sameLogGuardPathIdentity(realpathSync.native(path), path)).toBe(true); + }); + + // The guard this widening must not weaken: a redirection resolves somewhere else, and + // "somewhere else" is still refused. + test("a symlinked database is still refused", () => { + const root = mkdtempSync(join(tmpdir(), "ocx-log-guard-identity-")); + roots.push(root); + const real = join(root, "real.sqlite"); + const link = join(root, "logs_2.sqlite"); + writeFileSync(real, ""); + try { + symlinkSync(real, link); + } catch { + return; // unprivileged Windows cannot create symlinks; the POSIX legs cover this + } + + expect(sameLogGuardPathIdentity(realpathSync.native(link), link)).toBe(false); + }); + + test("an unrelated sibling path is refused", () => { + const root = mkdtempSync(join(tmpdir(), "ocx-log-guard-identity-")); + roots.push(root); + const path = join(root, "logs_2.sqlite"); + writeFileSync(path, ""); + + expect(sameLogGuardPathIdentity(join(root, "elsewhere.sqlite"), path)).toBe(false); + }); +}); diff --git a/tests/windows-popup-fix.test.ts b/tests/windows-popup-fix.test.ts index d1e21a10c4..f9e719c5df 100644 --- a/tests/windows-popup-fix.test.ts +++ b/tests/windows-popup-fix.test.ts @@ -51,7 +51,22 @@ describe("Windows identity lookup popup fix (#1278)", () => { expect(options.stdin).toBe("ignore"); // Assert the exact budget: the identity lookup contract is an 8-second // bound, and a looser assertion would let a silent re-tune through. - expect(options.timeout).toBe(8_000); + // + // Both values are pinned, because there are now two. A contended CI runner cannot start + // powershell.exe inside 8s while it runs a quarter of this suite, and the composed + // acceptance cases failed there with "Windows effective-account lookup timed out" — + // contention, not a hung child, which is the only thing this budget exists to bound. + // A user's machine keeps 8s exactly as before. + const previous = process.env.CI; + try { + delete process.env.CI; + expect(windowsIdentityPowerShellSpawnOptionsForTests().timeout).toBe(8_000); + process.env.CI = "true"; + expect(windowsIdentityPowerShellSpawnOptionsForTests().timeout).toBe(30_000); + } finally { + if (previous === undefined) delete process.env.CI; + else process.env.CI = previous; + } }); test("decodes non-ASCII known-folder values from the ASCII-safe envelope", () => {