From 4dfc2d69bc488b28c8f79dd15a9cb40a8e4b4822 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 03:54:28 +0900 Subject: [PATCH 01/11] fix(log-guard): accept the OS's own canonical spelling on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI reported 22 Log Guard failures, all resolving to unsafe_path. The feature (#1729) is new in this release range and has never shipped, so this is a release blocker rather than a pre-existing break. 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. databasePathIsSafe read that disagreement as an ancestor-symlink redirection and refused every mutation: protect, unprotect, repair, reclaim, compact. macOS had the same class of problem with /var and /tmp and was given an explicit alias normalizer. Windows was not. The fix keeps the check identity-based rather than string-based: when the spellings differ, the requested path is re-canonicalized through the same call and the two canonical forms must agree. A genuine symlink or junction resolves somewhere else and still fails, so the guard refuses exactly what it was built to refuse. It is scoped to win32 and cannot change any POSIX verdict. Regression tests cover all three directions: an OS-canonicalized spelling is the same file, a symlinked database is not, and an unrelated sibling is not. Refs #1729 Found by reading the Windows shard logs rather than assuming the leg was broken. It sat behind 3 failures of my own — POSIX mode assertions that do not apply on Windows, fixed separately in #2139 — which is why it took a second pass to see. --- src/codex/log-guard/path-safety.ts | 28 ++++++++++++- tests/codex-log-guard-coderabbit.test.ts | 50 +++++++++++++++++++++++- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/codex/log-guard/path-safety.ts b/src/codex/log-guard/path-safety.ts index e1f0ee0736..471bca6824 100644 --- a/src/codex/log-guard/path-safety.ts +++ b/src/codex/log-guard/path-safety.ts @@ -35,5 +35,31 @@ 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 comparison is still identity-based, not string-based: it re-canonicalizes the + * REQUESTED path through the same call and requires the two canonical forms to agree. A + * genuine symlink or junction resolves somewhere else and still fails, so the guard keeps + * refusing exactly what it was built to refuse. + */ +function sameWindowsCanonicalPath(realPath: string, requestedPath: string): boolean { + if (process.platform !== "win32") return false; + try { + return samePathIdentity(realPath, realpathSync.native(requestedPath)); + } catch { + return false; + } } 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); + }); +}); From d2f1dcc1e380fa8817f34f8e7aeb9c331f4bd9ed Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 04:18:43 +0900 Subject: [PATCH 02/11] test(composed): show the child's output when a start never publishes its port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six of the WP13 acceptance tests fail on the Windows shards with "timed out waiting for runtime-port record". That message is the symptom and never the cause: the child process writes runtime-port.json only after a successful start, so the interesting information — a throw, a bind refusal, a missing artifact — is in its stderr, which the fixture piped and then discarded. Two CI rounds read as "Windows is flaky" for exactly this reason. This makes the next round say what actually happened. The streams are captured once at spawn and carried on StartedServer, because a Bun subprocess stream cannot be read twice — the Grok E2E case already reads stdout after start() and would otherwise fail with "ReadableStream has already been used". Caught locally before pushing. Diagnostic only: no production code, and the failure message is the only behavior that changes. Refs #2108 --- tests/codex-composed-acceptance.test.ts | 31 +++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 857c27b1a8..6949c99333 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -41,7 +41,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 { @@ -176,6 +182,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 +209,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,7 +220,7 @@ 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 { @@ -570,7 +593,7 @@ 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 { From 512d1c7597935971f591f3fb85a0106424632abd Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 04:41:03 +0900 Subject: [PATCH 03/11] test(composed): scale the WP13 start watchdogs to the CI floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six WP13 acceptance cases fail on the Windows shards with "timed out waiting for runtime-port record". The diagnostic added in the previous commit answered what that meant: child exit=null with both streams still open. The child was alive and still working — a slow start, not a crash. These waits sit on a real `ocx start`: spawn a Bun runtime, load the CLI, read config, bind a port, publish runtime-port.json. 10s is generous locally and is not on a Windows shard running a quarter of the suite. The repository already has the answer to exactly this class of flake — watchdogMs() with a 30s CI floor, added when the macOS lane's 10s-floor watchdog fired at 10.16s — and this suite never adopted it. All three watchdogs move together, because they bound one lifecycle: the runtime-port wait, the CLI watchdog, and the shutdown watchdog. The per-case budget moves with them. A case can start a server twice and stop it, so 45s would kill the case before a 30s watchdog inside it could report anything — the budget has to exceed the sum of what it contains. 150s on CI, unchanged locally. Worth stating why the number matters here and not on the macOS lane: that lane passes --timeout 60000, which would pre-empt any larger per-case value. The Windows shards pass no --timeout at all, so Bun's 5s default applies and the explicit per-case budget is the only ceiling that exists. Refs #2108 Verification: 8 pass / 0 fail locally, ci-workflows 132 pass / 0 fail (it pins the lane's timeout contract), tsc --noEmit exit 0. The Windows shard is the only check that can prove the change, since the failure exists nowhere else. --- tests/codex-composed-acceptance.test.ts | 43 +++++++++++++++++++------ 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 6949c99333..add4e8eb88 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"; @@ -73,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(); @@ -169,7 +187,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]), @@ -227,7 +250,7 @@ class Fixture { 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. @@ -317,7 +340,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 @@ -372,7 +395,7 @@ describe("WP13 composed toggle acceptance", () => { } 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 () => { @@ -434,7 +457,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 () => { @@ -492,7 +515,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(); @@ -537,7 +560,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 () => { @@ -600,7 +623,7 @@ describe("WP13 composed toggle acceptance", () => { 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 () => { @@ -662,5 +685,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); }); From fe8fc6fe8f4cc332fa80442157638be718e445dc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 04:59:54 +0900 Subject: [PATCH 04/11] ci(windows): give the Windows shards the timeout every other leg already has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the remaining Windows failures were Bun's default 5s per-test ceiling firing on tests that had not hung — "this test timed out after 5000ms" on the synchronous restore body, a Log Guard reclaim case, and a pool-retry case. The Linux batches pass --timeout 60000 through run-bun-test-batches.sh and the macOS control passes it inline. The Windows leg was the only one still on the default, and it is the slowest hardware on the board. That is not a Windows quirk, it is a gap in the workflow. tests/ci-workflows.test.ts pins the exact command string for this leg, so the flag is now part of that contract rather than something a future edit can drop silently. Separately, the B-reduced composed-acceptance case failed with a 500 where it expected 200, and the log said why: "[Bun.serve]: request timed out after 10 seconds." That is the test's OWN provider fixture, which holds a /models response open on purpose — the hold is the instrument that keeps a discovery request in flight while the toggle flips. Bun's default request idleTimeout cancelled the request the test was holding. It now sets idleTimeout: 255, the same value the production server uses; the hold stays bounded by its release promise, not by the socket. Refs #2108 Verification: composed-acceptance 8 pass / 0 fail, ci-workflows 132 pass / 0 fail, tsc --noEmit exit 0. The Windows shards are the only check that can prove either half. Not claimed as fixed: the A-reduced case fails on `back.exitCode` being 1, a real CLI failure rather than a timeout, and shard 2 is a Bun runtime panic. Those are separate and still open. --- .github/workflows/ci.yml | 6 +++++- tests/ci-workflows.test.ts | 6 +++++- tests/codex-composed-acceptance.test.ts | 6 ++++++ 3 files changed, 16 insertions(+), 2 deletions(-) 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/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 add4e8eb88..51f4bc20e4 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -407,6 +407,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) { From cf5f91e468f5b29500a938607face7fa83b7b0f3 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 05:00:51 +0900 Subject: [PATCH 05/11] test(composed): put the CLI output in the restore-back assertion message A-reduced fails on Windows with exitCode 1 and the assertion said only "expected 0, got 1". runCli already captures stdout and stderr; the message now carries them, so the next Windows round names the CLI failure instead of leaving it to be guessed at. Diagnostic only. --- tests/codex-composed-acceptance.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 51f4bc20e4..7460378944 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -388,7 +388,9 @@ 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 }); From b7b34a9ff4c4b6de570939031a9e6bee2118101b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 05:20:17 +0900 Subject: [PATCH 06/11] fix(windows): widen the identity-lookup budget on CI runners only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last four Windows composed-acceptance failures all resolve to one cause, which only became visible after the previous commit put the CLI's own output in the assertion message: CodexUserIdentityRefusal: Windows effective-account lookup timed out 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 child was still starting, not hung — and bounding a hung child is the only thing that budget exists to do. Gated on CI alone, so a user's machine keeps the 8s ceiling exactly as before and the recoverable-refusal contract is unchanged where it matters. This is NOT a regression from this release range: src/codex/user-identity.ts has zero commits in main..dev. It is a pre-existing CI-only limit that was invisible until the diagnostics landed. The contract test now pins BOTH values rather than loosening to a range. Its comment says the point is to stop a silent re-tune, and a range would permit exactly that; two exact assertions keep the guard while admitting the second number. Ablated to confirm it fails without the change. Refs #2108 --- src/codex/user-identity.ts | 22 +++++++++++++++++++++- tests/windows-popup-fix.test.ts | 17 ++++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) 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/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", () => { From 9e11d4ff29be4ed27e375f3897f1f83d06d215dc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 05:38:26 +0900 Subject: [PATCH 07/11] test(composed): pass CI through to the CLI children The previous commit widened the Windows identity-lookup budget on CI, and the Windows shard still refused with "effective-account lookup timed out". The reason is in this fixture: env() is a deliberate whitelist, so CI never reached the child and the CLI kept the 8s desktop ceiling. Named explicitly rather than inheriting process.env, which is what the whitelist is for. --- tests/codex-composed-acceptance.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 7460378944..796c052bb2 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -151,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, }; } From d19a2c9504eed0159af74d52e773ddef0a33c4a5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 05:55:46 +0900 Subject: [PATCH 08/11] fix(log-guard): make the Windows path widening link-aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows shard caught a real hole in my own fix: "a symlinked database is still refused" went from pass to FAIL. The widening let through exactly what the guard exists to refuse. The bug was self-referential. databasePathIsSafe calls sameLogGuardPathIdentity(realpathSync.native(path), path) — so realPath is ALREADY the resolved form. Re-resolving the requested path produced the same value on both sides, and a symlinked database compared equal to itself. The check is now link-aware. A short-name expansion rewrites the spelling of components that are all still directories on one chain, so requiring that no component of the request is a link is sufficient: with none present, any remaining difference is the OS's own canonical spelling. A symlink or junction anywhere in the chain fails closed, and an unreadable component fails closed too. Refs #1729 Verification: 21 pass / 0 fail across the two Log Guard suites, tsc --noEmit exit 0. The symlink verdict was also checked directly against the exact call shape the caller uses — realpathSync.native(link) versus link — which is the shape that made the first version wrong and which the POSIX suites do not exercise. Worth recording plainly: this is the second time in this branch that a Windows fix of mine created a defect the platform legs then caught. The tests are doing their job; my first cut of a fail-closed boundary is not to be trusted without them. --- src/codex/log-guard/path-safety.ts | 35 +++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/codex/log-guard/path-safety.ts b/src/codex/log-guard/path-safety.ts index 471bca6824..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"; @@ -50,16 +50,39 @@ export function sameLogGuardPathIdentity(realPath: string, requestedPath: string * every Log Guard mutation refuse with `unsafe_path` on Windows, which is what the CI shards * were reporting. * - * The comparison is still identity-based, not string-based: it re-canonicalizes the - * REQUESTED path through the same call and requires the two canonical forms to agree. A - * genuine symlink or junction resolves somewhere else and still fails, so the guard keeps - * refusing exactly what it was built to refuse. + * 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; + } +} From c055608bed4a4651cf6cc2d3aaa9cd3ab7d728c0 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 06:15:43 +0900 Subject: [PATCH 09/11] test(composed): stop a timed-out case from killing the next one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows shard showed a second failure mode behind the first: a case failing with "Expected: 0, Received: 143" on a child it had just spawned, immediately after Bun printed "killed 1 dangling process". That is a cascade, not six independent failures. A case that times out leaves a live `ocx start`. Teardown was supposed to reap it, but the wait threw on the first child that did not exit inside 10s, so the rest of the loop — including every remaining child and the lock-file cleanup — never ran. The survivor was then killed by Bun's between-file sweep, and the next case's child died with it. Two fixes, both in teardown: - cleanup() now SIGTERMs every child, waits for each independently rather than aborting the loop, and SIGKILLs whatever is still alive. A survivor is strictly worse than an ungraceful exit; the case is already over. - afterEach drains every fixture before reporting, so one fixture's teardown failure cannot strand another fixture's children. This does not make the underlying case faster. It stops one slow case from being charged to unrelated ones, which is what made the Windows failures look like a moving target across runs. Refs #2108 Verification: 8 pass / 0 fail locally, tsc --noEmit exit 0. The Windows shard is the only place the cascade reproduces. Context worth recording: WP13 has never passed on Windows. It has zero commits in main..dev, and the 2026-08-18 run I originally compared against had shard 4/4 CANCELLED, so those cases never executed there. "Pre-existing on dev" was true; "already known to pass" was not, and I stated the second when I only had evidence for the first. --- tests/codex-composed-acceptance.test.ts | 39 +++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 796c052bb2..ad508f273e 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -281,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 @@ -308,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", () => { From 2b2190ff8f050b5c7660012a9f86214d9ec7510b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 06:34:40 +0900 Subject: [PATCH 10/11] test(composed): route the last two hardcoded budgets through the CI scale The Windows shard said "this test timed out after 30000ms" on two cases while the file's per-case budget is 150s on CI. Both were hardcoded 30_000 values the earlier commit missed: the E lock case's own per-test budget, and the restore watchdog inside the Restore-truth case. So those two were never given the headroom the rest of the file got, and their failures were being read as slow-runner evidence when they were a stale constant. Both now use the same scale as everything around them: CASE_TIMEOUT_MS for the per-case budget, watchdogMs() for the in-test watchdog. No new numbers. Refs #2108 Verification: 8 pass / 0 fail locally, tsc --noEmit exit 0, and a grep confirms no bare 30_000 remains in the file. --- tests/codex-composed-acceptance.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index ad508f273e..ded130e17e 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -642,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 () => { @@ -717,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" } } }); From 3fa40535676ee33cb6e2f70c535aa29f2443777a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 06:56:09 +0900 Subject: [PATCH 11/11] docs(devlog): record the Windows leg, and the pre-existing verdict that was wrong --- .../180_windows_leg.md | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/180_windows_leg.md 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. +