From 509e5ea91631171362311a4f496803e66ebcaedb Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 03:14:19 +0900 Subject: [PATCH 1/4] docs(devlog): plan the Windows suite stabilization unit Four-shard baseline on the pinned runtime (bun 1.4.0) on a self-hosted Windows box: 17783 pass, 25 fail, three test-harness defects. An earlier baseline used the 1.3.14 on PATH and reported 179+ failures across three shards with the fourth wedged; 001 records that mistake and the controlled A/B that exposed it. Research is 001-006, one decade doc per implementation phase (010/020/030) plus a dependent follow-up (040). --- .../000_plan.md | 81 ++++++ .../001_runtime_fault.md | 88 +++++++ .../002_v140_baseline.md | 112 ++++++++ .../003_void_preload_analysis.md | 120 +++++++++ .../004_void_singles_analysis.md | 56 ++++ .../005_wedge_resolution.md | 80 ++++++ .../006_void_inventory_1314.md | 136 ++++++++++ .../010_defect_acl_seam.md | 243 ++++++++++++++++++ .../020_defect_launcher_argv.md | 194 ++++++++++++++ .../030_defect_unlinked_cwd.md | 75 ++++++ .../040_acl_stub_hygiene.md | 93 +++++++ 11 files changed, 1278 insertions(+) create mode 100644 devlog/_plan/260905_windows_suite_stabilization/000_plan.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/001_runtime_fault.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/002_v140_baseline.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/003_void_preload_analysis.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/004_void_singles_analysis.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/005_wedge_resolution.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/006_void_inventory_1314.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/020_defect_launcher_argv.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/030_defect_unlinked_cwd.md create mode 100644 devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md diff --git a/devlog/_plan/260905_windows_suite_stabilization/000_plan.md b/devlog/_plan/260905_windows_suite_stabilization/000_plan.md new file mode 100644 index 0000000000..0fefe7541a --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/000_plan.md @@ -0,0 +1,81 @@ +# 000 — Plan: stabilize the Windows suite + +Unit: get the Windows test suite to zero failures on the runtime this repository +pins, and keep it there. Base `dev` at `00834d710`, 2026-09-05. + +Runner: the user's own Windows box `desktop-c795oh4` (Windows 10.0.26200.9168, +16 cores, Git-bash), checkout at `C:\ocxwin\repo`, reached over SSH. Single +machine, so suite runs are **strictly serial** under `/c/ocxwin/.suite.lock` and +never overlapped. + +**Always pin the runtime explicitly:** + +```bash +cd /c/ocxwin/repo && B=./node_modules/bun/bin/bun.exe && "$B" --version # 1.4.0 +``` + +A bare `bun` on that box is 1.3.14 and produces a fictional failure list. That +mistake was made once, cost ~70 minutes, and is recorded in `001`. + +## Baseline + +| shard | pass | skip | fail | wall | +|---|---|---|---|---| +| 1/4 | 4459 | 39 | 2 | 971s | +| 2/4 | 4606 | 16 | 22 | 1147s | +| 3/4 | 4305 | 11 | 1 | 1274s | +| 4/4 | 4413 | 12 | 0 | 888s | +| total | **17783** | 78 | **25** | 4280s | + +25 failures, three defects, all in test-harness code. No product defect +identified. + +## Work phases + +Three, **independent** — disjoint write sets, no shared API. Any stacking is for +review convenience, ordered by size. + +| phase | doc | defect | failures | write set | +|---|---|---|---|---| +| wp-acl | `010_defect_acl_seam.md` | the ACL stub seam can be installed half-way, so a real `icacls.exe` outlives teardown | 22 | `tests/helpers/windows-secret-acl-stubs.ts` (new), `tests/oauth-store-multi.test.ts` | +| wp-argv | `020_defect_launcher_argv.md` | a test reads the `cmd.exe` launcher's argument grammar as its mock API | 2 | `tests/multi-agent-keep-native-v1.test.ts` | +| wp-cwd | `030_defect_unlinked_cwd.md` | the test needs a POSIX unlinked cwd, which Windows cannot produce | 1 | `tests/update-notify.test.ts` | + +One follow-up phase, which unlike the three above is **dependent**: + +| phase | doc | purpose | depends on | +|---|---|---|---| +| wp-hygiene | `040_acl_stub_hygiene.md` | make the half-installed seam unrepresentable: a repo-hygiene rule plus 17 fixture migrations | `010` (enforces adoption of the helper it introduces) | + +It is separate because it is a mechanical 18-file change that would swamp the +review of a 22-failure bug fix. It fixes no failing test; it stops the next one. + +## Research + +`001`-`006` are analysis and are not implemented from: + +| doc | what it is | +|---|---| +| `001_runtime_fault.md` | the 1.3.14-vs-1.4.0 A/B, and the method correction | +| `002_v140_baseline.md` | the corrected baseline and root-cause roll-up | +| `003_void_preload_analysis.md` | VOID — a 1.3.14-only mechanism; records a latent hazard at `tests/preload.ts:41` | +| `004_void_singles_analysis.md` | VOID — four of six "singles" do not exist on 1.4.0 | +| `005_wedge_resolution.md` | RESOLVED — the shard-3 wedge was the runtime; no code target | +| `006_void_inventory_1314.md` | VOID — the first inventory, kept as the record of the mistake | + +## Acceptance for the unit + +1. Four shards, pinned runtime, **0 fail, twice consecutively**, with logs. +2. Every fix is a root-cause change: no assertion weakened, no timeout inflated + without naming the intrinsic operation it covers. +3. macOS unchanged for every touched file, verified by running it. +4. `bun run typecheck` clean. +5. Published as pull requests against `dev`, each filling the template. +6. Any Windows landmine not already in the `fuck-powershell` corpus is added + there and passes `lint-cases` + `validate-graph`. + +## Out of scope + +Product changes (none are indicated), release promotion, npm publish, and the +repository-wide local suite on macOS — the user prohibited the last one; focused +files and `typecheck` only. diff --git a/devlog/_plan/260905_windows_suite_stabilization/001_runtime_fault.md b/devlog/_plan/260905_windows_suite_stabilization/001_runtime_fault.md new file mode 100644 index 0000000000..34ed269b0b --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/001_runtime_fault.md @@ -0,0 +1,88 @@ +# 001 — The first baseline used the wrong Bun. Everything it concluded is void. + +Research doc. Written after the plan audit at `A` returned FAIL and its first +blocker turned out to be correct. + +## What happened + +The 2026-09-05 baseline in `000` was run with the Bun on the Windows box's PATH, +`~/.bun/bin/bun` = **1.3.14**. The repository pins **1.4.0** +(`package.json:68`, `dependencies.bun`), and `.github/actions/setup-project-bun` +installs exactly that version, keeping "the runtime SOT in one place". The +checkout already carried it at `node_modules/bun/bin/bun.exe`. + +So the baseline measured a runtime that neither CI nor a correct local run uses. + +## The controlled comparison + +Same box, same checkout, same two files, same flags — only the binary differs: + +``` +$ ./node_modules/bun/bin/bun.exe test --isolate --timeout 60000 \ + tests/abort-idle-deadline.test.ts tests/codex-reset-credit-operation-ledger.test.ts + 50 pass · 0 fail · 211 expect() calls · [8.63s] + +$ ~/.bun/bin/bun test --isolate --timeout 60000 \ + tests/abort-idle-deadline.test.ts tests/codex-reset-credit-operation-ledger.test.ts + 6 pass · 44 fail · 203 expect() calls · [8.75s] +``` + +The 44-failure guard defect exists only on 1.3.14. + +The wedge behaves the same way: + +``` +$ bun 1.3.14 test --isolate tests/client-hub-relay.test.ts tests/cline-pass-reasoning-efforts.test.ts + → killed at the 240s deadline; the second file never printed a line (exit 124) + +$ bun 1.4.0 test --isolate (identical command) + 12 pass · 0 fail · [1.60s] +``` + +## What this invalidates + +- `000` — every shard count. The four-shard baseline must be re-measured. +- `010` — the preload run-id provenance analysis. The mechanism it describes is + real in the source (the auditor verified (a)-(e) line by line, correcting one + citation: the non-win32 early return is `scripts/test-run-lock.ts:164`, not + `:162`). What is NOT established is that this mechanism fires on the runtime + the project actually uses. On 1.4.0 the guard arms and the same files pass. +- `020` — S1 and S6 were argued as ambient-`CODEX_HOME` defects. The auditor + showed the preload assigns a per-file `CODEX_HOME` when it completes, so both + may simply be downstream of the guard fault and disappear with the runtime. +- `030` — the wedge, and with it the attribution to + `tests/cline-pass-reasoning-efforts.test.ts`. + +## What survives + +The ordered-pair experiment the auditor asked for was run, and it settles the +wedge boundary that adjacency alone could not: + +| run | 1.3.14 | 1.4.0 | +|---|---|---| +| `cline-pass` alone | 6 pass, 0.95s | — | +| `client-hub-relay` → `cline-pass` | **wedged, exit 124** | 12 pass, 1.6s | +| `cline-pass` → `client-hub-relay` | 12 pass | — | +| pair without `--isolate` | 12 pass | — | + +Order-dependent, `--isolate`-dependent, and runtime-dependent. That is an isolate +realm-transition fault in 1.3.14, not a defect in either test file — which is why +no code change was made against it. + +## Method correction + +Pin the runtime explicitly in every command against the box: + +```bash +cd /c/ocxwin/repo && B=./node_modules/bun/bin/bun.exe && "$B" --version +``` + +A bare `bun` on that machine is 1.3.14 and must not be used for any measurement +that a conclusion depends on. `.github/workflows/ci.yml` never had this problem: +it calls `./.github/actions/setup-project-bun` before every test step. + +## Cost of the mistake + +Roughly 70 minutes of shard time and four documents' worth of analysis, caught by +the `A` gate before a single line of product code was changed. That is the gate +working. The re-measured baseline replaces `000` in `002`. diff --git a/devlog/_plan/260905_windows_suite_stabilization/002_v140_baseline.md b/devlog/_plan/260905_windows_suite_stabilization/002_v140_baseline.md new file mode 100644 index 0000000000..f1e09e4213 --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/002_v140_baseline.md @@ -0,0 +1,112 @@ +# 002 — Corrected baseline on the pinned runtime (`bun 1.4.0`) + +Same box, same checkout, same serial lock. The only change from `000` is the +binary: `./node_modules/bun/bin/bun.exe` (1.4.0, the version `package.json:68` +pins) instead of the 1.3.14 on `PATH`. + +## Shard 1/4 + +``` +4459 pass · 39 skip · 2 fail · 128135 expect() calls · [971.45s] +``` + +**52 → 2.** The 50 that disappeared were the 1.3.14 isolate fault (`001`), not +defects in this repository. Both survivors are in one file: + +``` +(fail) ocx v2 keep-native-v1 > enabling the native-v1 pin disables the global V2 override before catalog sync +(fail) ocx v2 keep-native-v1 > mode v2 honors a pre-existing native-v1 pin instead of enabling the global override +``` + +Shards 2-4 are running and land in the table below as they finish. + +| shard | pass | skip | fail | wall | +|---|---|---|---|---| +| 1/4 | 4459 | 39 | **2** | 971s | +| 2/4 | 4606 | 16 | **22** | 1147s | +| 3/4 | 4305 | 11 | **1** | 1274s — **past the 1.3.14 wedge** | +| 4/4 | 4413 | 12 | **0** | 888s | +| **total** | **17783** | **78** | **25** | 4280s | + +## What the corrected baseline says + +| | first attempt (1.3.14) | corrected (1.4.0) | +|---|---|---| +| shard 1 | 52 | 2 | +| shard 2 | 122 | 22 | +| shard 3 | no verdict (wedged) | 1 | +| shard 4 | 5 | 0 | +| **defects** | unknowable | **3** | + +25 failures, three root causes, and one of them is a single file. Shard 4 — +which `000` reported as five failures including two that looked like a +containment breach at `tests/service.test.ts:1283` — is **completely green**. +That alleged breach was the 1.3.14 guard fault, not a real hole in the armed-test +refusal. + +### The three defects + +| # | failures | file | mechanism | doc section | +|---|---|---|---|---| +| 1 | 2 | `tests/multi-agent-keep-native-v1.test.ts` | `.cmd` shim argv used as a mock API | "The one real defect so far" | +| 2 | 22 | `tests/oauth-store-multi.test.ts` | async `icacls` seam left unstubbed (+8 exposed siblings) | "The second real defect" | +| 3 | 1 | `tests/update-notify.test.ts` | POSIX unlinked-cwd is unreachable on Windows | "The third real defect" | + +All three are **test-harness defects**. **No product defect was identified**, and +that phrasing is deliberate: `src/lib/win-exec.ts` is verifiably correct and is +what defect 1 trips over, `src/lib/windows-secret-acl.ts` offers the async seam +defect 2 forgot to use, and defect 3 asks the filesystem for something Windows +does not provide. What the evidence supports is "no product defect identified; +the failures point at harness teardown" — not the stronger claim that none can +exist. `010` carries the red/green A/B that would upgrade or refute that for +defect 2. + +So the unit is "three fixtures encode POSIX assumptions", not "Windows is +broken". + +### Sequencing: the three phases are INDEPENDENT + +An earlier draft called this a dependency chain (2 → 1 → 3). It is not, and +describing risk ordering as dependency was wrong: defect 1 and 3 consume nothing +from the ACL helper, and defect 2 touches neither `src/cli/v2.ts` nor +`tests/update-notify.test.ts`. Disjoint write sets, no shared API. + +They may be built and reviewed in parallel. If they are published as a stack it +is for review convenience only, and the order is then by size — `010` (22 +failures), `020` (2), `030` (1) — which is a presentation choice, not a +constraint. + +Shard 2's 22 are one file, `tests/oauth-store-multi.test.ts`, and one mechanism. +The 122 failures `000` recorded for this shard are gone: the 68 recovery and 49 +fabric guard failures do not exist on the pinned runtime. + +### Where the implementation plans live + +This document is research: baseline and root-cause analysis only. One diff-level +document per surviving phase, each independently landable: + +| doc | defect | failures | files touched | +|---|---|---|---| +| `010_defect_acl_seam.md` | half-installed ACL stub seam | 22 | `tests/helpers/windows-secret-acl-stubs.ts` (new), `tests/oauth-store-multi.test.ts` | +| `020_defect_launcher_argv.md` | launcher argv used as a mock API | 2 | `tests/multi-agent-keep-native-v1.test.ts` | +| `030_defect_unlinked_cwd.md` | POSIX unlinked cwd unreachable | 1 | `tests/update-notify.test.ts` | + +No product source file appears in that table. + +## Evidence + +Shard logs are retained at `.tmp/win/v140-{1,2,3,4}.log` (gitignored; 603KB, +661KB, 597KB, 646KB). `grep -c '^(fail)'` over them gives 4, 44, 2, 0 — twice +the reported per-shard counts for 1-3 because Bun prints each failure once +inline and once in the trailing summary, and 0 for shard 4 either way. + +**Shard 3 clears the wedge.** `030` predicted this from the pair experiment; +the full shard confirms it in situ: + +``` +1227:tests\client-hub-relay.test.ts: +1235:tests\cline-pass-reasoning-efforts.test.ts: +``` + +Eight log lines apart. On 1.3.14 that boundary consumed 14 minutes and never +produced a second file. No code changed in between — only the runtime. diff --git a/devlog/_plan/260905_windows_suite_stabilization/003_void_preload_analysis.md b/devlog/_plan/260905_windows_suite_stabilization/003_void_preload_analysis.md new file mode 100644 index 0000000000..8d29391372 --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/003_void_preload_analysis.md @@ -0,0 +1,120 @@ +# 003 — VOID: the preload run-id analysis (1.3.14 only) + +> **This phase does not ship.** The defect it describes does not exist on +> `bun 1.4.0`: the same files are 50/50 green there (`001`, `002`). Renumbered +> into the research range because it is now a record of a mechanism, not a plan. +> +> **The latent hazard it found is still real and still unfixed**, and that is why +> the document stays: `tests/preload.ts:41` republishes its own bare run id into +> `OCX_TEST_RUN_ID`, which makes a bare run indistinguishable from a wrapper +> handoff. On 1.3.14 that ambiguity was load-bearing. On 1.4.0 nothing currently +> reaches it. If a future runtime re-evaluates preload the same way, this is +> where to start — and `tests/preload.ts:41` is the line to delete, not the +> `OCX_TEST_RUN_KIND` marker the original draft proposed (the audit showed that +> marker is unsound: `??= "bare"` preserves an ambient `wrapped`, and +> `OCX_TEST_NO_QUEUE=1` already separates a marker from its capability). + +> **Status: HELD, pending the `002` baseline.** The source mechanism below was +> verified line by line by the plan audit — with one citation corrected: the +> non-win32 early return is `scripts/test-run-lock.ts:164`, not `:162` +> (`:162` is the no-run-id return). What is NOT established is that it fires on +> `bun 1.4.0`, the runtime this project actually pins. On 1.4.0 the same files +> pass 50/50. See `001_runtime_fault.md`. +> +> If `002` shows the guard armed on 1.4.0, this phase does not ship a fix for a +> defect nobody has. What survives either way is the latent hazard: a preload +> that republishes its own bare id is indistinguishable from a wrapper handoff, +> and `tests/preload.ts:41` is the line that makes them indistinguishable. +> +> **Audit findings to fold in before this phase can be attested, whatever `002` +> says:** +> +> 1. The proposed `OCX_TEST_RUN_KIND` is unsound as written. +> `process.env[TEST_RUN_KIND_ENV] ??= "bare"` preserves an ambient +> `wrapped`, so an environment that already carries `wrapped` with no id +> reproduces the original fault. A fix must be a total, fail-closed state +> machine over: undefined, invalid, bare, wrapped, wrapped-without-id, +> partial capability, `OCX_TEST_NO_QUEUE=1`, non-Windows, and mutation +> between isolate files. +> 2. The claim that the marker and its capability "can never separate" is +> **false**: `OCX_TEST_NO_QUEUE=1` produces a wrapper run with a kind and an +> id but deliberately no path/token. +> 3. A simpler candidate was not evaluated and must be: **delete the write-back +> at `tests/preload.ts:41`.** Bare identity is already stable per process or +> parallel controller (`scripts/test-run-lock.ts:365-371`), and a repository +> search found no consumer that needs a bare preload to publish +> `OCX_TEST_RUN_ID`. If nested bare invocations do need it, that requires a +> reproducer, not an assumption. +> 4. The proposed regression test says "with win32 semantics" without saying how +> `process.platform` becomes Windows in a cross-platform test. Either inject +> the platform through the existing seam or run the regression only on the +> Windows box and say so. + +One defect. It accounts for **161 of the 174 failures** seen so far +(44 in shard 1, 117 in shard 2) and it is Windows-only by construction. + +## What fails + +Three unrelated test files die in `beforeEach`/`afterEach`, each on a different +guarded seam, each with the same underlying condition +`process.env.OCX_TEST_HOME_GUARD !== "1"`: + +| file | seam | src | count | +|---|---|---|---| +| `tests/codex-reset-credit-operation-ledger.test.ts:296` | `setResetCreditOperationMigrationFaultForTests` | `src/codex/reset-credit-operation-ledger.ts:488` | 44 | +| `tests/codex-reset-credit-recovery.test.ts:90,94` | `resetCodexResetCreditRecoveryProcessStateForTests` | `src/codex/reset-credit-recovery.ts:638` | 68 | +| `tests/lab-fabric-task.test.ts:304` | `setFabricProducerIsolationLimitsForTests` | `src/lab/fabric/producer-isolate.ts:287` | 49 | + +Plus the one that names the defect outright: +`tests/test-home-guard.test.ts:274` — *"the preload sandboxes this very process"* — +asserts `isTestHomeGuardArmed()` is true and receives false. That test exists +precisely to catch this state, and on Windows it is red. + +## Mechanism + +`bunfig.toml` preloads `tests/preload.ts`, and `--isolate` re-evaluates it per +test file. The preload's own bookkeeping is what breaks the next evaluation: + +``` +preload.ts:30 const wrappedRunId = process.env[TEST_RUN_ID_ENV]?.trim(); +preload.ts:36 const inheritedLock = resolveInheritedTestRunLock({ wrappedRunId, env: process.env }); +preload.ts:41 process.env[TEST_RUN_ID_ENV] = runId; // <- writes back a BARE id +``` + +File 1 has no `OCX_TEST_RUN_ID`, so `wrappedRunId` is undefined, the bare +identity is used, and line 41 publishes `bare-` into the environment. +File 2's preload reads that value as `wrappedRunId` — it cannot tell who wrote +it. On Windows `resolveInheritedTestRunLock` then demands the rest of the +wrapper capability: + +``` +test-run-lock.ts:162 if (platform !== "win32") return undefined; // macOS exits here +test-run-lock.ts:166 const candidate = env[TEST_RUN_LOCK_PATH_ENV] +test-run-lock.ts:167 const ownerToken = env[TEST_RUN_LOCK_TOKEN_ENV] +test-run-lock.ts:169 throw new Error("The wrapped Bun test lock capability is incomplete; ...") +``` + +A bare run never sets those two variables, so the preload throws at line 36 — +**before** the sandbox is installed and before `OCX_TEST_HOME_GUARD = "1"` at +line 64. Every guarded seam in that file's realm then refuses. + +macOS never reaches the check: line 162 returns early on any non-win32 platform. +That is why the same three files are green locally at the same SHA. + +### Why the failure count differs per shard + +It is a function of how many guarded files land in the shard, not of flakiness. + +## Fix (NOT IMPLEMENTED) + +The original draft proposed an OCX_TEST_RUN_KIND provenance marker plus edits to +scripts/test-run-lock.ts, scripts/test.ts and tests/preload.ts. **That proposal is +withdrawn and deliberately not reproduced here**: the audit showed it unsound +(`??= "bare"` preserves an ambient `wrapped`, and OCX_TEST_NO_QUEUE=1 already +separates a marker from its capability), and the defect it targeted does not exist +on the pinned runtime. + +If this class ever returns, the candidate to evaluate FIRST is deleting the +write-back at tests/preload.ts:41 — bare identity is already stable per process or +parallel controller (scripts/test-run-lock.ts:365-371), and no consumer was found +that needs a bare preload to publish OCX_TEST_RUN_ID. diff --git a/devlog/_plan/260905_windows_suite_stabilization/004_void_singles_analysis.md b/devlog/_plan/260905_windows_suite_stabilization/004_void_singles_analysis.md new file mode 100644 index 0000000000..e84a940719 --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/004_void_singles_analysis.md @@ -0,0 +1,56 @@ +# 004 — VOID: the six 1.3.14 "singles" + +> **This phase does not ship.** Four of the six do not fail on `bun 1.4.0` +> (`002`). The two that survive are defect 1, replanned in `020` — and NOT with +> the `V2CliDeps.featureAction` seam sketched below, which the audit rejected +> for removing the `cmdV2 → codexFeaturesInvocation → commandInvocation` +> integration from exactly the tests that should keep it. +> +> Kept as a record of what the wrong runtime made look like six defects. + +> **Status: HELD, pending the `002` baseline** (`bun 1.4.0`). Every failure +> below was observed on `bun 1.3.14`; see `001_runtime_fault.md`. +> +> The audit landed two corrections that apply regardless of runtime: +> +> - **S1 and S6 are probably not independent.** They were argued as ambient +> `CODEX_HOME` defects, but a preload that COMPLETES already assigns a +> per-file `CODEX_HOME` (`scripts/test.ts:22-26,62` → `tests/preload.ts:57-64`). +> On 1.3.14 the preload did not complete, so "ambient" was a symptom of the +> guard fault, not a cause. Re-measure both after `002`; if they vanish, they +> leave this phase. +> - **S6's proposed assertion is ordered wrong.** Checking +> `getEffectiveActiveCodexAccountId(config) === "pool-a"` before +> `recordCodexUpstreamOutcome` (`tests/routing-profile.test.ts:494`) cannot +> observe the cursor movement at `src/codex/routing.ts:2422-2438`. It has to +> run after. +> - **S3/S4's semantic seam must not silently drop launcher coverage.** The +> Windows `.cmd` path is covered by `tests/codex-v2-gate.test.ts:1692-1726` +> and `tests/win-exec.test.ts:92-125`; both stay unchanged, and one +> `cmdV2 → feature action` integration assertion must survive the refactor. +> - **S2's budget cannot come from `isolationBudgetMs()` alone.** That helper +> only scales under `CI=true` or `OCX_TEST_FULL_SUITE=1` +> (`tests/helpers/ci-watchdog.ts:49-51`), and the self-hosted run sets +> neither. Use explicit case-local budgets and scale `totalTimeoutMs` with +> them. The production default is 30s (`src/lab/live/executor.ts:110`); the +> 30ms in this test is a fixture artifact, and the dedicated first-byte and +> inactivity cases at `:76-103` keep that behaviour covered. +> +> **S5 moves out of this phase.** It is an investigation with no known failing +> line, and mixing it here breaks the research/implementation split. It returns +> as its own phase once `002` says whether it still fails. + +Six failures that are not the preload defect. Each passes on macOS at the same +SHA (verified per file). Grouped by mechanism, because the fixes pair up. + + +## The six, and where they went + +Four do not fail on bun 1.4.0 and have no successor phase. The two that survive +are the launcher-argv defect, replanned from scratch in 020_defect_launcher_argv.md. + +The per-defect fix sketches that used to live here are removed rather than kept: +they were written against a fictional failure list, and two of them (the +V2CliDeps.featureAction seam, the CODEX_HOME isolation for S1/S6) were +subsequently rejected on audit. Reading them as guidance would be worse than +having nothing. diff --git a/devlog/_plan/260905_windows_suite_stabilization/005_wedge_resolution.md b/devlog/_plan/260905_windows_suite_stabilization/005_wedge_resolution.md new file mode 100644 index 0000000000..9cc0ed3da1 --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/005_wedge_resolution.md @@ -0,0 +1,80 @@ +# 005 — RESOLVED: the shard-3 wedge was the runtime, not a test + +> **Outcome: this phase ships nothing, and that is the correct result.** +> +> The audit was right that adjacency is not attribution, and demanded an +> alone/alone/ordered-pair matrix before naming a culprit. That matrix was run +> on the box and it exonerates both files: +> +> | run | `bun 1.3.14` | `bun 1.4.0` | +> |---|---|---| +> | `cline-pass` alone | 6 pass, 0.95s | — | +> | `client-hub-relay` → `cline-pass`, `--isolate` | **wedged, exit 124** | 12 pass, 1.6s | +> | `cline-pass` → `client-hub-relay`, `--isolate` | 12 pass | — | +> | same pair WITHOUT `--isolate` | 12 pass | — | +> +> Order-dependent, `--isolate`-dependent, runtime-dependent, and absent on the +> version the repository pins. That is an isolate realm-transition fault in Bun +> 1.3.14, not a defect in either test. `client-hub-relay.test.ts` opens no +> socket and no process — it calls pure functions — so there is nothing in it to +> fix. +> +> The section below is the original reasoning, kept because its CI observation +> still stands on its own: a 25-minute shard ceiling reports a wedge as a +> timeout, so this class of fault has never been nameable from CI logs alone. + +Not a failing test. A **stopped shard** — which is worse, because it produces no +verdict at all for the ~215 files behind it. + +## Observation + +Shard 3/4 printed its last line at 00:53 and produced nothing for the next 14 +minutes: `base-3.log` stayed at exactly 96929 bytes while the Bun process +(PID 1382, started 00:50:43) remained alive under the run's shell. + +Last file to report: + +``` +1228:tests\client-hub-relay.test.ts: +… +(pass) fixed-target hub management relay > rejects traversal, authority, encoded + separator, and caller-host variants before outbound I/O [0.40ms] +``` + +The shard's file list is Bun's sorted round-robin (`NR%4==3` over +`ls tests/*.test.ts | sort`), which puts **`tests/cline-pass-reasoning-efforts.test.ts`** +immediately after `client-hub-relay`. Nothing from it ever printed, so the wedge +is at that file's load or first test. + +## Why `--timeout 60000` did not save it + +Bun's per-test timeout bounds a test body. It does not bound module evaluation, +and it does not bound a worker that never reports. Fourteen minutes with a live +process and a byte-stable log is neither a slow test nor a crash: the runner is +not making progress and nothing in the harness notices. + +## Why CI never showed this + +`.github/workflows/ci.yml:653` caps each Windows shard at 25 minutes. A shard +that wedges here is CANCELLED at the ceiling, which is recorded as a timeout, not +as a wedge on a named file — the same truncation `260902_windows_ci_release/070` +hit repeatedly and attributed to a "native-main-refresh microtask spin". The +self-hosted box has no ceiling, which is exactly why the file is nameable now. + +## What is not yet known + +The file itself is 152 lines and looks inert: it imports the registry, the +adapter and `routeModel`, builds a static config, and asserts on +`PROVIDER_REGISTRY`. Nothing in it opens a socket. So the suspicion falls on +module-graph evaluation under Bun 1.3.14 on Windows — `../src/router` and +`../src/providers/registry` pull in a large graph — rather than on the test +bodies. **That is a hypothesis, not a finding.** The next step is to run this one +file alone on the Windows box, with the suite lock held, and watch whether it +completes, wedges, or wedges only after a preceding file. + +## Sequencing note + +Because the box is single-tenant and runs are serial, a wedged shard blocks the +whole inventory. The baseline therefore stops shard 3 after a bounded wait and +records the wedge rather than waiting it out; shard 4 runs next so the inventory +is complete, and this file gets a dedicated isolated run afterwards. diff --git a/devlog/_plan/260905_windows_suite_stabilization/006_void_inventory_1314.md b/devlog/_plan/260905_windows_suite_stabilization/006_void_inventory_1314.md new file mode 100644 index 0000000000..53010e74bd --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/006_void_inventory_1314.md @@ -0,0 +1,136 @@ +# 000 — Windows suite failure inventory, FIRST ATTEMPT (VOID) + +> **This document is superseded and its numbers must not be used.** It was +> measured with `bun 1.3.14` while the repository pins `1.4.0` +> (`package.json:68`). A controlled A/B on the same box showed the headline +> defect exists only on 1.3.14: the same two files give 44 fail on 1.3.14 and +> 50 pass on 1.4.0. See `001_runtime_fault.md`. The corrected baseline is `002`. +> +> It is kept, not deleted: the shard timings, the log layout, the serial-lock +> protocol and the reasoning that the audit overturned are all real, and a +> deleted mistake is one the next person repeats. + +Unit: stabilize the Windows test suite until four shards run clean, twice. +Base: `dev` at `00834d710`. Runner: **not** GitHub Actions — the user's own +Windows box `desktop-c795oh4` (Windows 10.0.26200.9168, 16 cores, Git-bash, +`bun 1.3.14`, checkout at `C:\ocxwin\repo`), reached over SSH. + +## Why a self-hosted baseline instead of a CI dispatch + +The Windows leg is `workflow_dispatch`-only (`.github/workflows/ci.yml:565`) and +each shard carries a 25-minute ceiling. A CI round therefore costs ~25 minutes and +returns a log that is already truncated when a shard is slow. The self-hosted box +has no ceiling, so a shard runs to completion and every failure is readable. The +box is a single machine: **suite runs are strictly serial**, tracked by +`/c/ocxwin/.suite.lock`, and never overlapped. + +## Baseline, all four shards + +| shard | pass | skip | fail | wall | verdict | +|---|---|---|---|---|---| +| 1/4 | 4385 | 39 | **52** | 1083s | complete | +| 2/4 | 4507 | 15 | **122** | 1125s | complete | +| 3/4 | — | — | — | — | **WEDGED** — no verdict (see `030`) | +| 4/4 | 4405 | 12 | **5** | 922s | complete | + +179 failures across the three shards that finished, plus one shard that never +reported. Shard 3 stopped producing output after `client-hub-relay` and stayed +byte-stable for 14 minutes with a live process; it was killed after a bounded +wait so shard 4 could run. Its ~215 remaining files are UNMEASURED, so this +inventory is a floor, not a total. + +## Shard 1/4 detail + +`bun test --isolate --timeout 60000 tests --shard=1/4`, 265 files, 1083.33s. + + 4385 pass · 39 skip · 52 fail · 4 errors · 191329 expect() calls + +Every failure below passes on macOS at the same SHA (verified per file, not +assumed): the ledger file is 44/44 green locally, and so are the six singles. + +| # | Signature | Count | Owner | Class | +|---|---|---|---|---| +| L | `reset-credit operation migration faults require the repository test preload` | 44 | `src/codex/reset-credit-operation-ledger.ts:488` guard; raised from the `afterEach` at `tests/codex-reset-credit-operation-ledger.test.ts:296` | preload/env — one root, 44 cascaded cases | +| S1 | `adapter-event OAuth failover > Codex and Anthropic remain excluded` — expects 401, gets 400 | 1 | `tests/adapter-event-oauth-failover.test.ts:190` | TBD | +| S2 | `CL-03 pinned live transport … output_byte_limit` — gets `pinned provider first byte timed out` | 1 | `tests/lab-live-pinned-timeouts.test.ts:112` | timing race, slower Windows I/O | +| S3 | `ocx v2 keep-native-v1 > enabling the native-v1 pin disables the global V2 override before catalog sync` | 1 | `tests/multi-agent-keep-native-v1.test.ts` | TBD | +| S4 | `ocx v2 keep-native-v1 > mode v2 honors a pre-existing native-v1 pin …` | 1 | `tests/multi-agent-keep-native-v1.test.ts` | TBD | +| S5 | `OpenAI provider-option integration spine > keeps Pool, Direct, and API ownership stable …` | 1 | TBD | TBD | +| S6 | `routing profiles (RI-04) > API dry-run mirrors live codex cooldown for openai candidates` | 1 | `tests/routing-profile.test.ts:479` | ambient `CODEX_HOME` | + +## Shard 2/4 detail — the same root, three more files + +| Signature | Count | Guarded seam | src | +|---|---|---|---| +| `resetProcessStateForTests is available only under the repository test preload` | 68 | `resetCodexResetCreditRecoveryProcessStateForTests` | `src/codex/reset-credit-recovery.ts:638` | +| `fabric isolation limits can only be overridden by the test harness` | 49 | `setFabricProducerIsolationLimitsForTests` | `src/lab/fabric/producer-isolate.ts:287` | +| `real-home write guard > the preload sandboxes this very process` | 1 | — | `tests/test-home-guard.test.ts:274` | +| `runWindowsElevated spawn contract > an armed test cannot launch the live Windows elevation boundary` | 1 | armed-process refusal | — | +| `multi-account auth store > OAuth 30 second wait timeout …` | 1 | — | — | +| `Grok orphan adoption (#511)`, `020 coverage completions` | 2 | — | — | + +The third row is the one that names the defect: the test whose whole job is to +assert `isTestHomeGuardArmed()` receives `false`. The guard is genuinely not +armed — this is not 161 separate assertions disagreeing, it is one process-level +fault observed 161 times. + +## Shard 4/4 detail + +| Signature | Count | Class | +|---|---|---| +| `service lifecycle cleanup ordering > an armed test cannot fall through to a live Task Scheduler mutation` | 1 | same guard root | +| `service lifecycle cleanup ordering > an armed partial install cannot fall through to live native-service removal` | 1 | same guard root | +| `Windows tray packaging and command safety > launches the detached tray host without retaining the proxy listen socket` | 1 | Windows-specific, own investigation | +| `health-aware scoring (RI-06) > execution path applies live codex account cooldown to openai candidates` | 1 | sibling of S6 — ambient `CODEX_HOME` | +| (1 more) | 1 | — | + +The two service failures are the guard defect wearing a different coat, and they +are the dangerous shape of it: `tests/service.test.ts:1283` expects the armed +process to REFUSE a machine-global Task Scheduler mutation and instead gets +*"Task Scheduler reported success, but the new registration is absent"*. The +refusal did not fire, so an unarmed test process reached a live scheduler call on +the user's machine. That elevates the preload defect from "many red tests" to a +containment failure, and it is why wp2 leads the queue. + +## Roll-up by root cause + +| root | failures | phase | +|---|---|---| +| preload run-id provenance (guard never arms) | **163** | wp2 (`010`) | +| six shard-1 singles | 6 | wp3 (`020`) | +| shard-3 wedge | (blocks ~215 files) | wp4 (`030`) | +| shard-2/4 stragglers not yet classified | ~10 | wp5, after the above clears | + +Fixing one defect is expected to clear roughly 90% of the red. The remainder is +small enough to work case by case — but the count only becomes trustworthy after +shard 3 reports, which is why the wedge is a first-class phase and not a footnote. + +## Shard 1/4 signature table + +44 of 52 failures are one defect. The headline number is six independent defects +plus one env fault, not fifty-two. + +## L — the preload guard + +``` +488 | if (process.env.OCX_TEST_HOME_GUARD !== "1") { +489 | throw new Error("reset-credit operation migration faults require the repository test preload"); +``` + +`tests/preload.ts:64` is the only writer of that variable, and `bunfig.toml` +preloads it for every invocation. The first occurrence in the Windows log is +**before any test body**, at the file's `afterEach`, and the run also prints +`[opencodex] Reset-credit operation ledger is unavailable.` So either the preload +never reached line 64 on this file's worker, or its effect was not visible there. +That distinction is what the wp2 experiment has to settle; it is not yet settled +and nothing below assumes an answer. + +## Shards 2-4 + +Running serially after shard 1. Recorded in `001` when complete. + +## Evidence + +Shard logs live on the Windows box at `/c/ocxwin/logs/base-.log` and are +copied into `.tmp/win/` (gitignored) for reading. They are not committed: a +single shard log is 650KB of pass lines. diff --git a/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md b/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md new file mode 100644 index 0000000000..a3f5b57c9b --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md @@ -0,0 +1,243 @@ +# 010 — Defect 2: the ACL stub seam can be installed half-way + +Implementation phase. Independent of `020` and `030` — no shared file, no +shared API. 22 of the 25 failures. + +## Failure + +``` +error: EPERM: operation not permitted, rm 'C:\ocxwin\repo\tests\.tmp-oauth-store-multi-test' + at removeTreeWithRetry (tests/helpers/remove-tree.ts:28:83) + at tests/oauth-store-multi.test.ts:44 (beforeEach) / :61 (afterEach) +(fail) multi-account auth store > … [5274.25ms] +``` + +Evidence: `.tmp/win/v140-2.log` (gitignored), 22 cases, all this signature. + +## Reachability, verified rather than assumed + +`getCredential()` / `saveCredential()` → `loadAuthStoreInternal()` +(`src/oauth/store.ts:338`) or `persist()` → `hardenConfigDir()` +(`src/config/paths.ts:31`) → fire-and-forget `hardenSecretDirAsync()` → +`asyncIcaclsRunner` (`src/lib/windows-secret-acl.ts:373`). `store.ts` calls +`hardenConfigDir` at seven sites. The fixture creates the directory before +exercising this, so the harden is not short-circuited by the `existsSync` guard +at `paths.ts:35`. + +`tests/oauth-store-multi.test.ts:48` stubs `setIcaclsRunnerForTests` and there is +no `setAsyncIcaclsRunnerForTests` and no `flushConfigDirHardeningForTests` in the +file. So the async runner is the real one. + +## What is NOT yet proven + +That the `icacls.exe` child is the specific handle causing each EPERM. The code +path is proven reachable; the holder is inferred. The 5.2s case duration is +consistent with 50×50ms of retries plus work, and `removeTreeWithRetry` only +retries `EPERM`/`EBUSY`/`ENOTEMPTY`, but neither fact identifies the handle. + +**Therefore this phase's first step is a Windows red/green A/B**, before any +committed change: + +1. Instrument a scratch copy to log each `asyncIcaclsRunner` invocation with its + target path. Confirm it fires for `.tmp-oauth-store-multi-test`. +2. Confirm the failure reproduces (red). +3. Apply dual stubbing + flight flush in the scratch copy. Confirm green. +4. Revert one of the two (stub only / flush only) and confirm it is still red, + so the fix is not over-determined. + +If step 1 shows no invocation for that path, this analysis is wrong and the +phase restarts from the log rather than from the patch. + +## Why pairing the setters is not sufficient + +Teardown that restores the real runner while a flight is still awaiting +principal resolution hands the continuation the real `icacls.exe`. Ordering is +part of the contract, not an implementation detail. + +## NEW `tests/helpers/windows-secret-acl-stubs.ts` + +`IcaclsRunner` and `AsyncIcaclsRunner` are NOT exported +(`src/lib/windows-secret-acl.ts:285-286`), so the helper derives its parameter +types from the setters rather than importing names that do not exist: + +```ts +import { + resetHardenedStateForTests, + setAsyncIcaclsRunnerForTests, + setIcaclsRunnerForTests, +} from "../../src/lib/windows-secret-acl"; +import { flushConfigDirHardeningForTests } from "../../src/config/paths"; + +type SyncRunner = NonNullable[0]>; +type AsyncRunner = NonNullable[0]>; + +/** Same shape the already-correct fixtures use (tests/codex-account-store.test.ts:18). */ +const ICACLS_OK = { success: true, exitCode: 0, timedOut: false, stdout: "" } as const; + +export function installWindowsSecretAclStubs( + runners: { sync?: SyncRunner; async?: AsyncRunner } = {}, +): { restore(): Promise } { + setIcaclsRunnerForTests(runners.sync ?? (() => ICACLS_OK)); + setAsyncIcaclsRunnerForTests(runners.async ?? (async () => ICACLS_OK)); + let restored = false; + return { + async restore() { + if (restored) return; // idempotent + restored = true; + await flushConfigDirHardeningForTests(); // 1. settle WHILE stubbed + setIcaclsRunnerForTests(null); // 2. restore both + setAsyncIcaclsRunnerForTests(null); + resetHardenedStateForTests(); // 3. clear memo + }, // 4. only now may the caller + }; // delete its temp home +} +``` + +`resetHardenedStateForTests` is exported at `src/lib/windows-secret-acl.ts:415`. + +The helper lives in `tests/helpers/`, not in `src/lib/windows-secret-acl.ts`: +`src/config/paths.ts` already imports the ACL module, so having the ACL module +import the flight flusher inverts the dependency. + +## MODIFY `tests/oauth-store-multi.test.ts` + +`beforeEach` calls `installWindowsSecretAclStubs()`; `afterEach` awaits +`restore()` BEFORE restoring `OPENCODEX_HOME` and before the final +`removeTreeWithRetry`. The `afterEach` becomes `async`. + +## Migration scope — narrowed by the audit + +The earlier claim of "nine loaded guns" was wrong at call-site level. Several of +those files stub the sync runner deliberately, to test synchronous +`hardenSecretPath` directly, and never start an async config harden: +`tests/config.test.ts:2797`, `tests/lab-public-security-regressions.test.ts:145`, +`tests/windows-tray.test.ts:61`. Forcing an async stub there would add noise, not +safety. + +Migration is therefore limited to fixture-lifecycle sites that actually reach +`hardenConfigDir`. `oauth-store-multi` is confirmed. Each other candidate must +show a reaching call path before it is migrated; a file that only exercises the +sync API keeps the individual setter. + +## Hygiene rule — replaced + +The proposed file-level "every sync setter needs an async setter somewhere in the +file" grep is deleted. It false-passes (one paired call anywhere in a file hides +an unpaired one, which is exactly `tests/windows-secret-acl.test.ts`) and it +false-fails legitimate synchronous unit tests. + +### The rule that was proposed here first, and why it was wrong + +The previous draft skipped any file that does not mention +`setAsyncIcaclsRunnerForTests`, on the theory that stubbing the async runner +marks a fixture that can start a flight. **It is exactly inverted**, and running +it proved so: + +``` +BOTH (sync + async): 13 files +SYNC-ONLY: 9 files — including tests/oauth-store-multi.test.ts +``` + +The rule would have skipped all nine sync-only files — the defect population, +containing the very file whose 22 failures started this phase — and flagged the +13 already-correct ones. It fails RED on the wrong set and green on the bug. + +Recorded rather than quietly replaced: the earlier deleted rule and this one +failed the same way, by grepping for a proxy that felt like the property instead +of measuring the property. + +### MODIFY `tests/repo-hygiene.test.ts` + +The population is "every fixture that stubs the ACL runners at all", and the +exception set is audited by hand, once, with a reason per entry: + +```ts +test("an ACL-stubbing fixture installs both runners through the atomic helper", async () => { + // Any file that stubs the ACL runners for FIXTURE ISOLATION must take both + // through installWindowsSecretAclStubs, so the flush-before-restore ordering + // cannot be skipped. Files that drive the setters as their SUBJECT are listed + // below with the reason each is exempt. + const EXEMPT = new Map([ + ["tests/windows-secret-acl.test.ts", + "drives the runners directly; they are the unit under test"], + ["tests/config.test.ts", + "injects failing/timing-out sync runners to assert ACL error classification"], + ["tests/lab-public-security-regressions.test.ts", + "asserts synchronous hardenSecretPath refusals; starts no config-dir flight"], + ["tests/windows-tray.test.ts", + "asserts synchronous tray-directory hardening only"], + ]); + const offenders: string[] = []; + for (const file of await Array.fromAsync(new Bun.Glob("tests/**/*.test.ts").scan())) { + if (EXEMPT.has(file)) continue; + const source = await Bun.file(file).text(); + const stubs = source.includes("setIcaclsRunnerForTests") + || source.includes("setAsyncIcaclsRunnerForTests"); + if (!stubs) continue; + if (!source.includes("installWindowsSecretAclStubs")) offenders.push(file); + } + expect(offenders.sort()).toEqual([]); +}); +``` + +### Measured, not asserted + +The scan above was RUN before being written down. Result: + +``` +offenders: 18 +oauth-store-multi RED? true +``` + +It fails on the bug it exists to prevent, which is the property both earlier +drafts lacked. + +### The cost, and how this phase pays it + +18 files. That is a mechanical migration far larger than the defect, and folding +it into this phase would produce a PR nobody can review against a 22-failure fix. + +**So the rule does NOT ship in this phase.** It splits: + +| phase | contents | +|---|---| +| `010` (this one) | the helper + `tests/oauth-store-multi.test.ts` — the fix for the 22 failures | +| `040` (follow-up) | the hygiene rule + the 17 remaining migrations, as its own reviewable unit | + +The write set for `010` is therefore back to two files: + +| file | change | +|---|---| +| `tests/helpers/windows-secret-acl-stubs.ts` | NEW | +| `tests/oauth-store-multi.test.ts` | MODIFY — adopt the helper, async `afterEach` | + +which is what `002` recorded, so that contradiction is gone too. + +The alternative — shipping a rule that is green on `oauth-store-multi` so the +diff stays small — is what both previous drafts did, in different ways. A guard +that passes on the defect is worse than no guard, because it is claimed as +enforcement afterwards. + +The 18-file list and the provisional four-entry exemption set move to `040`, +where each exemption is re-read at implementation time rather than inherited from +this classification. + +### Write set for this phase + +| file | change | +|---|---| +| `tests/helpers/windows-secret-acl-stubs.ts` | NEW | +| `tests/oauth-store-multi.test.ts` | MODIFY — adopt the helper, async `afterEach` | +| `tests/repo-hygiene.test.ts` | MODIFY — add the assertion above | +| further fixtures | MODIFY only if the red rule names them | + +`002` records the two-file write set from before this rule existed; this table +supersedes it for phase `010`. + +## Acceptance + +1. The A/B above: red before, green after, and still red with either half alone. +2. `tests/oauth-store-multi.test.ts` green on Windows with the pinned runtime; + per-case duration back under a second (5.2s today). +3. `bun test tests/oauth-store-multi.test.ts` still 22 pass on macOS. +4. `bun run typecheck` clean. diff --git a/devlog/_plan/260905_windows_suite_stabilization/020_defect_launcher_argv.md b/devlog/_plan/260905_windows_suite_stabilization/020_defect_launcher_argv.md new file mode 100644 index 0000000000..601278d8ba --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/020_defect_launcher_argv.md @@ -0,0 +1,194 @@ +# 020 — Defect 1: a test asserts on the Windows launcher's argument grammar + +Implementation phase. Independent of `010` and `030`. 2 of the 25 failures. + +## Failure + +``` + [ +- "disable", ++ "/s", + ] + at tests/multi-agent-keep-native-v1.test.ts:223:21 +(fail) ocx v2 keep-native-v1 > mode v2 honors a pre-existing native-v1 pin … +(fail) ocx v2 keep-native-v1 > enabling the native-v1 pin disables the global V2 override … +``` + +Evidence: `.tmp/win/v140-1.log`. + +## The product is correct + +`commandInvocation` (`src/lib/win-exec.ts:79-96`) routes a `.cmd`/`.bat` target +through `ComSpec` as `["/d","/s","/c", ""]`, preserving +`features disable multi_agent_v2` inside the quoted line. A shell-less `.cmd` +spawn is rejected by post-CVE Node/Bun, so the wrapper is required, and +`args[1] === "/s"` is its correct output. Corpus: `cmd-shim-reparses-argv`. + +The defect is that `tests/multi-agent-keep-native-v1.test.ts` reads `args[1]` +(`:215`) and compares the joined argv to a POSIX string (`:185`), i.e. it uses the +OS launcher's grammar as its mock API. + +## Fix — normalize in the test, do NOT add a product seam + +An earlier draft proposed adding `V2CliDeps.featureAction`. **Rejected on audit, +and the audit is right:** it would move all three keep-native call sites outside +`codexFeaturesInvocation`, so those tests would no longer exercise +`cmdV2 → codexFeaturesInvocation → commandInvocation` at all, and a future state +test could bypass launcher construction without anyone noticing. Adding a +product-visible seam to make a test simpler is the wrong trade when the test can +simply read the value correctly. + +The repository already has the right pattern, in a test that hit this first +(`tests/codex-v2-gate.test.ts:1736`): + +```ts +// POSIX: ["features", "enable|disable", ...]; win32 .cmd: ["/d","/s","/c","...enable..."] +const joined = args.join(" "); +const enabled = args[1] === "enable" || /\benable\b/.test(joined); +``` + +### MODIFY `tests/multi-agent-keep-native-v1.test.ts` + +`:175` — record the semantic action rather than the raw joined argv: + +```ts +- events.push(args.join(" ")); ++ events.push(featureActionOf(args)); // "features disable multi_agent_v2" +``` + +`:214` — same, replacing the `args[1]` index: + +```ts +- actions.push(args[1]!); ++ actions.push(featureActionOf(args).split(" ")[1]!); +``` + +with one local helper in the test file. **It parses one of the two supported argv +SHAPES; it does not search for a phrase.** An unanchored search would accept +`["/d","/s","/c","echo features disable multi_agent_v2"]` — a bypassed +invocation that never runs `codex` — and report success: + +```ts +/** + * The semantic `features ` triple, parsed from exactly the two + * argv shapes `commandInvocation` produces (src/lib/win-exec.ts:85-95): + * POSIX / .exe : ["features", "", ""] + * win32 .cmd : ["/d", "/s", "/c", '" ^"features^" ^"^" ^"^""'] + * Anything else throws, so a bypassed or malformed invocation fails the test + * instead of silently matching. + */ +function featureActionOf(args: readonly string[]): string { + const ACTION = /^(?:enable|disable)$/; + const FEATURE = /^[a-z0-9_]+$/; + + // Direct spawn: the exact three-element argv, nothing before or after. + if (args.length === 3 && args[0] === "features") { + const [, action, feature] = args; + if (!ACTION.test(action!) || !FEATURE.test(feature!)) { + throw new Error(`malformed features argv: ${JSON.stringify(args)}`); + } + return `features ${action} ${feature}`; + } + + // cmd.exe wrapper: fixed prefix, single quoted line, and the command line must + // BEGIN with the codex target — "echo features disable x" is rejected here. + if (args.length === 4 && args[0] === "/d" && args[1] === "/s" && args[2] === "/c") { + const line = args[3]!; + if (!line.startsWith('"') || !line.endsWith('"')) { + throw new Error(`unquoted cmd line: ${line}`); + } + const inner = line.slice(1, -1); + // Split on unescaped spaces only: escapeCmdCommand rewrites a space in the + // target path as "^ ", so "C:\Program Files\..." is ONE token. Then strip the + // argument quoting, which is "^\"" for a normal target and "^^^\"" for a + // node_modules/.bin shim (IS_CMD_SHIM double-escapes, win-exec.ts:89). + const tokens = inner + .split(/(? t.replace(/\^+"/g, "").replace(/\^ /g, " ")); + const [target, keyword, action, feature, ...rest] = tokens; + if ( + rest.length > 0 + || !/\.(cmd|bat)$/i.test(target ?? "") + || keyword !== "features" + || !ACTION.test(action ?? "") + || !FEATURE.test(feature ?? "") + ) { + throw new Error(`unrecognized cmd invocation: ${inner}`); + } + return `features ${action} ${feature}`; + } + + throw new Error(`unrecognized features invocation: ${JSON.stringify(args)}`); +} +``` + +Three properties matter, and each closes a specific silent-pass hole: + +1. **Exact arity and prefix.** Extra leading or trailing tokens are rejected, so + a wrapper that grew an argument is a failure, not a match. +2. **The cmd line must start with the `.cmd`/`.bat` target.** This is what + rejects `echo features disable multi_agent_v2`: `echo` is not a batch target. +3. **It throws instead of defaulting.** A future change that stops invoking + `codex features` fails loudly rather than recording `""` and passing. + +### Negative cases the phase must add + +The helper is itself test logic, so it gets tested. Each of these must throw: + +```ts +["/d","/s","/c",'"echo ^"features^" ^"disable^" ^"multi_agent_v2^""'] // bypassed target +["features","disable"] // truncated +["features","restart","multi_agent_v2"] // unknown action +["/d","/s","/c","features disable multi_agent_v2"] // unquoted line +["-c","features disable multi_agent_v2"] // wrong shape +``` + +### Verified against real `commandInvocation` output + +The parser was not written from the source and hoped at — it was run against +what `commandInvocation` actually emits for three target shapes: + +``` +C:\npm\codex.cmd + "C:\npm\codex.cmd ^"features^" ^"disable^" ^"multi_agent_v2^"" +C:\Program Files\npm\codex.cmd + "C:\Program^ Files\npm\codex.cmd ^"features^" ^"disable^" ^"multi_agent_v2^"" +C:\proj\node_modules\.bin\codex.cmd + "C:\proj\node_modules\.bin\codex.cmd ^^^"features^^^" ^^^"disable^^^" ^^^"multi_agent_v2^^^"" +``` + +Two traps a naive parser walks into, both real: + +1. **A space in the target path is escaped as `^ `, not quoted.** Splitting on + `/\s+/` would break `C:\Program^ Files\...` into two tokens and reject a + perfectly valid invocation. Hence the `(? interactiveGuardOk safely evaluates without throwing when cwd is unlinked [2611.94ms] +``` + +Evidence: `.tmp/win/v140-3.log`. + +## Mechanism + +```ts +// tests/update-notify.test.ts:139-143 +const tempDir = mkdtempSync(join(tmpdir(), "ocx-unlinked-cwd-")); +process.chdir(tempDir); +removeTreeWithRetry(tempDir); // delete the directory this process stands in +``` + +POSIX allows unlinking a directory that a process holds as cwd; the process keeps +a valid but nameless working directory. Windows locks the cwd — no process may +delete it. The delete cannot succeed while the test stands there, so +`removeTreeWithRetry` exhausts all 50 attempts (2.6s) and rethrows. The retry +helper is behaving correctly; the precondition is unreachable. + +## What the test actually protects + +`interactiveGuardOk` (`src/update/notify.ts:126-133`) does **not** call +`process.cwd()` — it reads `OCX_SERVICE` and calls `isatty(0)`/`isatty(1)` +inside a try/catch. The regression it guards is that evaluating the TTY gate +from an unlinked cwd must not throw while initializing a stream. The cwd healing +itself lives elsewhere, at `src/cli/index.ts:7-12`, which catches a throwing +`process.cwd()` and `chdir`s to `homedir()`. + +So this is an integration case over a real filesystem state, not a unit test of +a catch branch. + +## Fix + +```ts +// Windows locks a process's cwd: it cannot be unlinked, so the state under test +// cannot exist there. src/cli/index.ts:7 heals a throwing cwd; this case covers +// the POSIX variant where the directory is gone but the cwd handle survives. +test.skipIf(process.platform === "win32")( + "interactiveGuardOk safely evaluates without throwing when cwd is unlinked", + () => { /* body unchanged */ }, +); +``` + +### Rejected alternatives + +- **`chdir` back before deleting.** Makes the delete succeed and destroys the + test: `interactiveGuardOk()` would run with a valid cwd, asserting nothing. +- **Replace it with an injected `isatty` throw.** That exercises the catch + branch, not the deleted-cwd regression — a different test, and the audit is + right that it must not REPLACE this one. It may be added later as its own + case; it is out of scope here. +- **Invent a Windows-reachable "bad cwd"** (revoked ACL, dropped drive mapping). + A different failure mode dressed up to keep a green checkmark. + +A skip is honest here: the platform cannot enter the state, so there is no +coverage to lose. The comment says which platform property makes it so, so it +reads as a boundary rather than a muted failure. + +## Acceptance + +1. Shard 3 green on Windows with the pinned runtime. +2. `bun test tests/update-notify.test.ts` on macOS → 21 pass, with this case + still RUNNING (verified by output, not by reading the predicate). +3. `bun run typecheck` clean. diff --git a/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md b/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md new file mode 100644 index 0000000000..eb7223eff6 --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md @@ -0,0 +1,93 @@ +# 040 — Follow-up: make the half-installed ACL seam unrepresentable + +Implementation phase, split out of `010` because it is a mechanical migration of +18 files and does not belong in the same review as a 22-failure bug fix. + +Depends on `010` — it enforces adoption of the helper `010` introduces. This is +a real dependency, unlike the independence of `010`/`020`/`030`. + +## Why the rule exists + +`010` fixes one fixture. Nothing stops the eleventh one from stubbing the sync +runner alone and rediscovering the same 2.5-second EPERM. Two earlier attempts at +a guard both failed, in instructive ways: + +1. *"Every file with a sync setter must also have an async setter."* False-passes + (one paired call anywhere in a file hides an unpaired one) and false-fails + legitimate sync-only ACL tests. +2. *"Skip files that do not stub the async runner."* Exactly inverted: measured, + it skipped all 9 sync-only files — the defect population, including + `oauth-store-multi` — and flagged the 13 already-correct ones. + +Both grepped a proxy that felt like the property. The rule below measures the +property: a fixture that stubs the ACL runners at all must take them from the +helper, so the flush-before-restore ordering cannot be skipped. + +## MODIFY `tests/repo-hygiene.test.ts` + +```ts +test("an ACL-stubbing fixture installs both runners through the atomic helper", async () => { + const EXEMPT = new Map([ + ["tests/windows-secret-acl.test.ts", + "drives the runners directly; they are the unit under test"], + ["tests/config.test.ts", + "injects failing/timing-out sync runners to assert ACL error classification"], + ["tests/lab-public-security-regressions.test.ts", + "asserts synchronous hardenSecretPath refusals; starts no config-dir flight"], + ["tests/windows-tray.test.ts", + "asserts synchronous tray-directory hardening only"], + ]); + const offenders: string[] = []; + for (const file of await Array.fromAsync(new Bun.Glob("tests/**/*.test.ts").scan())) { + if (EXEMPT.has(file)) continue; + const source = await Bun.file(file).text(); + const stubs = source.includes("setIcaclsRunnerForTests") + || source.includes("setAsyncIcaclsRunnerForTests"); + if (!stubs) continue; + if (!source.includes("installWindowsSecretAclStubs")) offenders.push(file); + } + expect(offenders.sort()).toEqual([]); +}); +``` + +Measured before being written down — 18 offenders, and `oauth-store-multi` among +them, so the guard is red on the defect it prevents. + +## Migration + +17 files after `010` lands (`oauth-store-multi` migrates there): + +``` +codex-account-store codex-auth-api codex-auth-context +codex-prompt-journal google-antigravity-replay google-signature-history-roundtrip +oauth-account-id-collision oauth-manual-code oauth-public-surface +oauth-reauth-bind oauth-status-privacy openai-provider-option-e2e +openai-provider-option-startup responses-state server-management-auth +service thought-signature-credential-scope +``` + +Each replaces its paired `setIcaclsRunnerForTests` / `setAsyncIcaclsRunnerForTests` +calls with `installWindowsSecretAclStubs(...)` and awaits `restore()` before its +home teardown. Files passing custom runners keep them: the helper takes +`{ sync?, async? }`. + +`tests/responses-state.test.ts` is the hard one — 12 sync and 18 async sites, +several with bespoke gating runners — and if it does not migrate cleanly it gets +an exemption with a written reason rather than a forced rewrite. + +## The exemption list is provisional + +The four entries above were classified from their call sites and an auditor's +inventory, not from reading each test's intent end to end. **Each is re-read at +implementation time**; an entry that turns out to start a config-directory +flight loses its exemption and migrates instead. + +## Acceptance + +1. The rule is added FIRST and observed failing with 17 offenders. A guard never + seen red is not known to be a guard. +2. After migration: 0 offenders, and `bun test tests/repo-hygiene.test.ts` green. +3. Every migrated file still passes on macOS, and the Windows shards stay at 0. +4. `bun run typecheck` clean. +5. Every exemption carries a one-line reason in the map — a bare path is not an + exemption. From 5afd0aab90c7e32913f59539324b715d0157e578 Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 03:32:12 +0900 Subject: [PATCH 2/4] docs(devlog): retract the ACL-seam defect after its falsification probe fired 22 of the 25 baseline failures were contamination, not a defect. Stubbing both icacls runners left them failing and logged zero runner invocations, so the mechanism 010 described never executed. The holder was a Windows handle kept by the wedged 1.3.14 shard-3 process after kill -9; with the leftover directory removed the fixture is 22 pass in 1.4s. Retracts 010 and 040 - a helper plus an 18-file migration for a defect that does not exist. The Windows suite has three real failures, not 25. --- .../000_plan.md | 46 ++++---- .../007_acl_defect_retracted.md | 105 ++++++++++++++++++ .../010_defect_acl_seam.md | 6 + .../040_acl_stub_hygiene.md | 6 + 4 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 devlog/_plan/260905_windows_suite_stabilization/007_acl_defect_retracted.md diff --git a/devlog/_plan/260905_windows_suite_stabilization/000_plan.md b/devlog/_plan/260905_windows_suite_stabilization/000_plan.md index 0fefe7541a..6a56e38bfd 100644 --- a/devlog/_plan/260905_windows_suite_stabilization/000_plan.md +++ b/devlog/_plan/260905_windows_suite_stabilization/000_plan.md @@ -19,36 +19,42 @@ mistake was made once, cost ~70 minutes, and is recorded in `001`. ## Baseline -| shard | pass | skip | fail | wall | -|---|---|---|---|---| -| 1/4 | 4459 | 39 | 2 | 971s | -| 2/4 | 4606 | 16 | 22 | 1147s | -| 3/4 | 4305 | 11 | 1 | 1274s | -| 4/4 | 4413 | 12 | 0 | 888s | -| total | **17783** | 78 | **25** | 4280s | +| shard | pass | skip | fail | wall | note | +|---|---|---|---|---|---| +| 1/4 | 4459 | 39 | 2 | 971s | | +| 2/4 | 4606 | 16 | 22 | 1147s | **contaminated** — 22 → 0 on a clean tree, see `007` | +| 3/4 | 4305 | 11 | 1 | 1274s | | +| 4/4 | 4413 | 12 | 0 | 888s | | + +**Three real failures, two defects**, both in test-harness code. No product +defect identified. + +Shard 2's 22 were contamination I created: a `kill -9` on the wedged 1.3.14 +shard left a Windows handle on `tests/.tmp-oauth-store-multi-test`, so every +later teardown in that fixture hit EPERM. Clean, the file is 22 pass in 1.4s. +`007_acl_defect_retracted.md` has the falsification probe and the diagnosis it +destroyed. That count was measured after the kill, so the confirmation run +re-measures it. + +**Before any measurement a conclusion depends on**, clear what a killed run +leaves behind: -25 failures, three defects, all in test-harness code. No product defect -identified. +```bash +cd /c/ocxwin/repo && ls -d tests/.tmp-* 2>/dev/null; ps | grep bun +``` ## Work phases -Three, **independent** — disjoint write sets, no shared API. Any stacking is for -review convenience, ordered by size. +Two, **independent** — disjoint write sets, no shared API. | phase | doc | defect | failures | write set | |---|---|---|---|---| -| wp-acl | `010_defect_acl_seam.md` | the ACL stub seam can be installed half-way, so a real `icacls.exe` outlives teardown | 22 | `tests/helpers/windows-secret-acl-stubs.ts` (new), `tests/oauth-store-multi.test.ts` | | wp-argv | `020_defect_launcher_argv.md` | a test reads the `cmd.exe` launcher's argument grammar as its mock API | 2 | `tests/multi-agent-keep-native-v1.test.ts` | | wp-cwd | `030_defect_unlinked_cwd.md` | the test needs a POSIX unlinked cwd, which Windows cannot produce | 1 | `tests/update-notify.test.ts` | -One follow-up phase, which unlike the three above is **dependent**: - -| phase | doc | purpose | depends on | -|---|---|---|---| -| wp-hygiene | `040_acl_stub_hygiene.md` | make the half-installed seam unrepresentable: a repo-hygiene rule plus 17 fixture migrations | `010` (enforces adoption of the helper it introduces) | - -It is separate because it is a mechanical 18-file change that would swamp the -review of a 22-failure bug fix. It fixes no failing test; it stops the next one. +`010_defect_acl_seam.md` and `040_acl_stub_hygiene.md` are **RETRACTED** (`007`). +Between them they would have added a test helper and rewritten 18 test files to +prevent a defect that does not exist. ## Research diff --git a/devlog/_plan/260905_windows_suite_stabilization/007_acl_defect_retracted.md b/devlog/_plan/260905_windows_suite_stabilization/007_acl_defect_retracted.md new file mode 100644 index 0000000000..14cd1f41c0 --- /dev/null +++ b/devlog/_plan/260905_windows_suite_stabilization/007_acl_defect_retracted.md @@ -0,0 +1,105 @@ +# 007 — RETRACTED: the "ACL seam" defect never existed + +22 of the 25 baseline failures were not a defect in this repository. They were +contamination I created, and the diagnosis I built on them was wrong. + +## The claim + +`002` and `010` said: `tests/oauth-store-multi.test.ts` stubs only the +synchronous `icacls` runner, so a real `icacls.exe` holds the fixture directory +and `removeTreeWithRetry` exhausts its 50 retries with EPERM. It matched the +corpus case `async-child-holds-dir-after-stop` exactly, and the code path was +verified reachable: `getCredential` → `hardenConfigDir` (`src/config/paths.ts:31`) +→ `hardenSecretDirAsync` → `asyncIcaclsRunner`. + +Reachable is not the same as reached. `010` said so, and made the first +implementation step a falsification test. That test fired. + +## The measurements that killed it + +On the box, pinned runtime, one file at a time: + +| probe | result | +|---|---| +| stub BOTH icacls runners via `--preload` | still 22 fail | +| log every runner invocation to a file | **0 lines** — `icacls` never ran | +| also stub both `windows-user-principal` runners | still 22 fail, still 0 invocations | + +A mechanism that never executes cannot be the cause. Every claim in `010` about +`icacls.exe` holding the directory was false. + +## What actually held it + +`tests/.tmp-oauth-store-multi-test/auth.json`, timestamped **00:44** — from the +1.3.14 baseline, hours earlier. At 01:08 I sent `kill -9` to the wedged shard-3 +process (PID 1382, `001`/`005`). On Windows that leaves the handle held: the +process is gone but its open file keeps the directory undeletable, and every +later `beforeEach` in that fixture hit EPERM. + +After deleting the leftover: + +``` +$ ./node_modules/bun/bin/bun.exe test --isolate --timeout 60000 tests/oauth-store-multi.test.ts + 22 pass · 0 fail · 71 expect() calls · [1404.00ms] +``` + +1.4 seconds, from 115 seconds and 22 failures. And recreating the leftover +directory and file WITHOUT a holder still passes — so the debris was never the +problem either. The dead process's handle was. + +## Corrected failure count + +| defect | failures | status | +|---|---|---| +| `.cmd` launcher argv as a mock API | 2 | REAL — reproduced clean | +| POSIX unlinked cwd on Windows | 1 | REAL — reproduced clean | +| ~~ACL stub seam~~ | ~~22~~ | **RETRACTED — self-inflicted** | + +Verified together on a clean tree: + +``` +$ bun.exe test --isolate --timeout 60000 \ + tests/multi-agent-keep-native-v1.test.ts tests/update-notify.test.ts + 29 pass · 3 fail · [8.08s] +``` + +**The Windows suite has three real failures, not 25.** + +## What this retracts + +- `010_defect_acl_seam.md` — the whole phase. No helper, no fixture change. +- `040_acl_stub_hygiene.md` — the hygiene rule and its 18-file migration. It + would have rewritten 18 test files to prevent a defect that does not exist. + +## Why I believed it + +The symptom matched a corpus case precisely, and the corpus is good, so I +classified instead of measuring. I checked that the ACL path *could* run and +treated that as proof it *did*. Three audit rounds did not catch it either — +the reviewers challenged the fix's shape, the evidence retention, and the +wording of the conclusion, and all of that was useful, but none of it could +substitute for running the thing. + +The one thing that did catch it was a falsification condition written into the +plan before implementation, with the instruction to restart from the log rather +than patch. It cost one probe to find out, after roughly three hours of planning +built on top of it. + +## Operational lesson, now a rule for this unit + +**A killed suite process contaminates the next run.** `kill -9` on a Bun test +process leaves Windows handles held. Before any measurement that a conclusion +depends on: + +```bash +cd /c/ocxwin/repo && git status --short # leftover tests/.tmp-* dirs? +ls -d tests/.tmp-* 2>/dev/null # remove before measuring +ps | grep bun # no survivors from a prior run +``` + +Two more leftovers exist right now (`tests/.tmp-api-catalog-route-9092`, +`tests/.tmp-issue-914-test`) and must be cleared before the confirmation runs. + +The shard-2 count in `000_plan.md` and `002` is therefore also suspect: it was +measured after the kill. The confirmation baseline re-measures it on a clean +tree. diff --git a/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md b/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md index a3f5b57c9b..9aa22ce3de 100644 --- a/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md +++ b/devlog/_plan/260905_windows_suite_stabilization/010_defect_acl_seam.md @@ -1,3 +1,9 @@ +> **RETRACTED — see 007_acl_defect_retracted.md.** The defect described here does +> not exist. The icacls runners are never invoked by this fixture (measured: 0 +> invocations with both runners stubbed), and the 22 failures came from a Windows +> handle held by a process I killed. Kept as the record of a diagnosis that matched +> a corpus case exactly and was still wrong. + # 010 — Defect 2: the ACL stub seam can be installed half-way Implementation phase. Independent of `020` and `030` — no shared file, no diff --git a/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md b/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md index eb7223eff6..025dac96f7 100644 --- a/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md +++ b/devlog/_plan/260905_windows_suite_stabilization/040_acl_stub_hygiene.md @@ -1,3 +1,9 @@ +> **RETRACTED — see 007_acl_defect_retracted.md.** The defect described here does +> not exist. The icacls runners are never invoked by this fixture (measured: 0 +> invocations with both runners stubbed), and the 22 failures came from a Windows +> handle held by a process I killed. Kept as the record of a diagnosis that matched +> a corpus case exactly and was still wrong. + # 040 — Follow-up: make the half-installed ACL seam unrepresentable Implementation phase, split out of `010` because it is a mechanical migration of From d9e350de545c81de1df4239393815ab4b48fdba1 Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 03:33:24 +0900 Subject: [PATCH 3/4] test(v2): read the feature transition, not the Windows launcher argv On Windows an npm-installed codex is a .cmd shim, and a shell-less .cmd spawn is rejected by post-CVE Node/Bun, so commandInvocation wraps the call as cmd.exe /d /s /c "". That makes args[1] equal /s, and these two tests asserted args[1] === 'disable' and a POSIX-joined argv string, so they were asserting the OS launcher's argument grammar rather than the state transition. featureActionOf() parses the two shapes commandInvocation actually emits and throws on anything else, so a bypassed or malformed invocation still fails. Its own test pins both shapes - including an escaped space in the target path and the node_modules/.bin double-escaped form - and five rejections. No product change: the tests still run through cmdV2 -> codexFeaturesInvocation -> commandInvocation, and the launcher contract stays pinned by codex-v2-gate.test.ts and win-exec.test.ts. --- .../multi-agent-keep-native-v1.test.ts | 84 ++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/tests/codex-integration/multi-agent-keep-native-v1.test.ts b/tests/codex-integration/multi-agent-keep-native-v1.test.ts index d774c6e8e7..33c021f68f 100644 --- a/tests/codex-integration/multi-agent-keep-native-v1.test.ts +++ b/tests/codex-integration/multi-agent-keep-native-v1.test.ts @@ -89,6 +89,61 @@ function isolateHomes(): void { process.env.CODEX_HOME = mkdtempSync(join(tmpdir(), "codex-keep-native-")); } +/** + * The semantic `features ` triple, parsed from the two argv + * shapes `commandInvocation` produces (src/lib/win-exec.ts:85-95): + * + * POSIX / .exe : ["features", "", ""] + * win32 .cmd : ["/d", "/s", "/c", '" ^"features^" ^"^" ^"^""'] + * + * A Windows npm install exposes `codex` as a `.cmd` shim, and a shell-less + * `.cmd` spawn is rejected by post-CVE Node/Bun, so the launcher must wrap it — + * which means `args[1]` is `/s`, not the action. Reading the index directly made + * these tests assert the OS launcher's argument grammar instead of the state + * transition they exist to check. + * + * This THROWS on anything else rather than falling back, so a bypassed or + * malformed invocation fails the test instead of silently matching. + */ +function featureActionOf(args: readonly string[]): string { + const ACTION = /^(?:enable|disable)$/; + const FEATURE = /^[a-z0-9_]+$/; + + if (args.length === 3 && args[0] === "features") { + const [, action, feature] = args; + if (!ACTION.test(action!) || !FEATURE.test(feature!)) { + throw new Error(`malformed features argv: ${JSON.stringify(args)}`); + } + return `features ${action} ${feature}`; + } + + if (args.length === 4 && args[0] === "/d" && args[1] === "/s" && args[2] === "/c") { + const line = args[3]!; + if (!line.startsWith('"') || !line.endsWith('"')) { + throw new Error(`unquoted cmd line: ${line}`); + } + // Split on unescaped spaces only: escapeCmdCommand rewrites a space inside the + // target path as "^ ", so "C:\Program Files\..." stays one token. Then strip the + // argument quoting, which is ^" normally and ^^^" for a node_modules/.bin shim + // (IS_CMD_SHIM double-escapes, src/lib/win-exec.ts:17,89). + const inner = line.slice(1, -1); + const tokens = inner.split(/(? t.replace(/\^+"/g, "").replace(/\^ /g, " ")); + const [target, keyword, action, feature, ...rest] = tokens; + if ( + rest.length > 0 + || !/\.(cmd|bat)$/i.test(target ?? "") + || keyword !== "features" + || !ACTION.test(action ?? "") + || !FEATURE.test(feature ?? "") + ) { + throw new Error(`unrecognized cmd invocation: ${inner}`); + } + return `features ${action} ${feature}`; + } + + throw new Error(`unrecognized features invocation: ${JSON.stringify(args)}`); +} + function captureLog(): { logs: string[]; errors: string[]; log: { log: (m?: unknown) => void; error: (m?: unknown) => void } } { const logs: string[] = []; const errors: string[] = []; @@ -164,6 +219,31 @@ describe("keep-native-v1 restamp path", () => { }); describe("ocx v2 keep-native-v1", () => { + test("featureActionOf parses both launcher shapes and rejects everything else", () => { + // The exact strings commandInvocation emits, captured from a real run against + // three target shapes: plain path, a path containing a space, and a + // node_modules/.bin shim (double-escaped). + expect(featureActionOf(["features", "disable", "multi_agent_v2"])) + .toBe("features disable multi_agent_v2"); + expect(featureActionOf(["/d", "/s", "/c", + String.raw`"C:\npm\codex.cmd ^"features^" ^"disable^" ^"multi_agent_v2^""`])) + .toBe("features disable multi_agent_v2"); + expect(featureActionOf(["/d", "/s", "/c", + String.raw`"C:\Program^ Files\npm\codex.cmd ^"features^" ^"disable^" ^"multi_agent_v2^""`])) + .toBe("features disable multi_agent_v2"); + expect(featureActionOf(["/d", "/s", "/c", + String.raw`"C:\p\node_modules\.bin\codex.cmd ^^^"features^^^" ^^^"enable^^^" ^^^"multi_agent_v2^^^""`])) + .toBe("features enable multi_agent_v2"); + + // A bypassed target must not match merely because the phrase is present. + expect(() => featureActionOf(["/d", "/s", "/c", + String.raw`"echo ^"features^" ^"disable^" ^"multi_agent_v2^""`])).toThrow(); + expect(() => featureActionOf(["features", "disable"])).toThrow(); + expect(() => featureActionOf(["features", "restart", "multi_agent_v2"])).toThrow(); + expect(() => featureActionOf(["/d", "/s", "/c", "features disable multi_agent_v2"])).toThrow(); + expect(() => featureActionOf(["-c", "features disable multi_agent_v2"])).toThrow(); + }); + test("enabling the native-v1 pin disables the global V2 override before catalog sync", async () => { isolateHomes(); saveConfig({ ...loadConfig(), multiAgentMode: "v2" }); @@ -173,7 +253,7 @@ describe("ocx v2 keep-native-v1", () => { const code = await cmdV2(["keep-native-v1", "on"], { execFile: (_file, args) => { - events.push(args.join(" ")); + events.push(featureActionOf(args)); writeFileSync(codexConfig, readFileSync(codexConfig, "utf8").replace("enabled = true", "enabled = false")); }, sync: async () => { events.push("sync"); }, @@ -212,7 +292,7 @@ describe("ocx v2 keep-native-v1", () => { expect(await cmdV2(["mode", "v2"], { execFile: (_file, args) => { - actions.push(args[1]!); + actions.push(featureActionOf(args).split(" ")[1]!); writeFileSync(codexConfig, readFileSync(codexConfig, "utf8").replace("enabled = true", "enabled = false")); }, sync: async () => {}, From ba0379553044ee3ef685c12482374da4dafb5dca Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 03:33:24 +0900 Subject: [PATCH 4/4] test(update): skip the unlinked-cwd case on Windows Windows locks a process's current directory, so a live process cannot stand in a deleted one. The fixture chdir()s into a temp dir and deletes it, which POSIX allows; on Windows the delete can never succeed and removeTreeWithRetry spends all 50 attempts before rethrowing EBUSY (2.6s). Skipped rather than reworked: chdir-ing back first would make the delete succeed and leave interactiveGuardOk() running with a valid cwd, asserting nothing. The platform cannot enter the state, so there is no coverage to lose - src/cli/index.ts:7 heals a throwing cwd, and that path deserves its own test rather than a Windows impersonation of this one. --- tests/update/update-notify.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/update/update-notify.test.ts b/tests/update/update-notify.test.ts index 5f1509f716..b3863c0ca1 100644 --- a/tests/update/update-notify.test.ts +++ b/tests/update/update-notify.test.ts @@ -138,7 +138,14 @@ describe("cli wiring", () => { expect(promptIndex).toBeLessThan(serverIndex); }); - test("interactiveGuardOk safely evaluates without throwing when cwd is unlinked", () => { + // POSIX-only by construction. Windows locks a process's current directory, so + // the state under test — a live process standing in a deleted directory — + // cannot exist there: the rmSync below can never succeed while this test holds + // the cwd, and removeTreeWithRetry burns all 50 attempts before rethrowing + // EBUSY. src/cli/index.ts:7 heals a cwd that throws; this case covers the + // POSIX variant where the directory is gone but the cwd handle survives. + test.skipIf(process.platform === "win32")( + "interactiveGuardOk safely evaluates without throwing when cwd is unlinked", () => { const origCwd = process.cwd(); const tempDir = mkdtempSync(join(tmpdir(), "ocx-unlinked-cwd-")); process.chdir(tempDir);