From 39b0eeedaa5385925fe70cee29efa30d27193614 Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:35:57 +0900 Subject: [PATCH 1/9] devlog: open the Windows stability program unit 806/806 green locally is not the same as stable for a Windows user, and the reason is structural: platform-windows is gated on workflow_dispatch (.github/workflows/ci.yml:547-552), the aggregation job accepts skipped, and release.yml:181-201 asks for a push-event CI run that Windows never joins. Every release so far published without executing a Windows test. Three independent Pro audits were run against a zip of the v2.24.2 tree with the GitHub connector, on orthogonal briefs: platform primitives, runtime and distribution, and user-visible failure modes plus CI coverage. Every finding carried into this unit was reproduced against the working tree in the same session; the rest were dropped, including two that turned out to be already fixed. Seven verified findings, nine dependency-ordered phases. The sharpest one is src/service.ts:2361, which uses the exact PowerShell argv that src/codex/user-identity.ts:222-224 forbids under #1589 -- it survived because the regression test at tests/windows-deploy-close-regressions.test.ts:43 is bound to src/update/job.ts alone. The icacls/CIM request-path latency class is deliberately excluded: both audits rank it first, but this session measured nothing, and an unverified claim next to seven verified ones devalues all of them. It is recorded at the end of 001 so the next cycle inherits it. No production code changes. --- .../000_problem_model.md | 68 +++++++ .../001_verified_findings.md | 189 ++++++++++++++++++ .../002_sequencing.md | 50 +++++ .../010_windowstyle_argv.md | 49 +++++ .../020_wrapper_killer_dedupe.md | 47 +++++ .../030_shared_replace_retry.md | 42 ++++ .../031_retry_telemetry.md | 34 ++++ .../040_credential_acl_inventory.md | 40 ++++ .../050_wrapper_backoff.md | 52 +++++ .../051_crash_restart_ci.md | 28 +++ .../060_windows_ci_gate.md | 51 +++++ .../070_flakiness_policy.md | 34 ++++ .../080_environment_smoke.md | 39 ++++ 13 files changed, 723 insertions(+) create mode 100644 devlog/_plan/260817_windows_stability_program/000_problem_model.md create mode 100644 devlog/_plan/260817_windows_stability_program/001_verified_findings.md create mode 100644 devlog/_plan/260817_windows_stability_program/002_sequencing.md create mode 100644 devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md create mode 100644 devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md create mode 100644 devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md create mode 100644 devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md create mode 100644 devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md create mode 100644 devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md create mode 100644 devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md create mode 100644 devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md create mode 100644 devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md create mode 100644 devlog/_plan/260817_windows_stability_program/080_environment_smoke.md diff --git a/devlog/_plan/260817_windows_stability_program/000_problem_model.md b/devlog/_plan/260817_windows_stability_program/000_problem_model.md new file mode 100644 index 0000000000..9631272811 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/000_problem_model.md @@ -0,0 +1,68 @@ +# 000 — Windows stability: why "806/806 green" is not "stable" + +Unit opened 2026-08-17, after v2.24.2 shipped. + +## The gap this unit exists to close + +The Windows campaign that preceded this unit took the local Bun suite from 53+ +failures to 806/806 across 15 commits. That was real work on real defects — an +empty-string `LocalApplicationData`, unfinalized SQLite statements holding a +file open against unlink, TOML escapes doubling backslashes, per-process +identity lookups costing ~510ms each. + +None of it proves the product is stable for a Windows user, and the reason is +structural rather than rhetorical: **the suite that went green is not a gate.** + +```yaml +# .github/workflows/ci.yml:547-552 +platform-windows: + name: windows ${{ matrix.shard }}/4 + needs: select-windows-runner + if: github.event_name == 'workflow_dispatch' +``` + +Windows runs only when a maintainer asks by hand. The aggregation job at +`.github/workflows/ci.yml:747-783` accepts `skipped` as an outcome, and +`.github/workflows/release.yml:181-201` requires a successful **push-event** +CI run before publishing. Since `platform-windows` always skips on push, a +release satisfies its own gate having executed zero Windows tests. + +Issue #1059 tracks exactly this and is still open. Its stated end condition is +Windows restored as a required gate. The failure counts quoted there are now +stale in our favour; the workflow contract has not caught up. + +## Evidence base for this unit + +Three independent GPT-5 Pro audits were run on 2026-08-17 against a zip of the +v2.24.2 tree (`src/`, `tests/`, `scripts/`, `.github/`, `structure/`), each +with the GitHub connector attached and a distinct brief: + +| Chat | Perspective | Conversation | +|---|---|---| +| P1 | Platform primitives: handles, locking, atomic publication, paths, ACLs | `chatgpt.com/c/6a82ebc4-48d4-83ee-a223-a6fc5a9556e5` | +| P2 | Runtime and distribution: install, spawn, service lifecycle, update, ports | `chatgpt.com/c/6a82ec28-86b4-83e8-86e4-a5477b6a9d91` | +| P3 | User-visible failure modes, diagnostics, and CI coverage | `chatgpt.com/c/6a82ec41-6b0c-83ee-93ab-3a96010a543f` | + +Every finding carried into `001` was **re-verified against the working tree in +this session**. Claims that could not be reproduced locally were dropped rather +than recorded. That rule matters here because two of the three audits also +correctly identified defects as *already fixed* (#1843 elevation argv, #31 +passthrough segfault) — an audit that cannot tell live from historical is not +usable as a roadmap input. + +## What changed in the problem model + +The pre-campaign model was "Windows has many small filesystem bugs." The +evidence no longer supports that as the dominant class. The surviving defects +cluster into three shapes: + +1. **Synchronous Windows subprocesses on the request path.** `icacls` and + PowerShell/CIM calls that block Bun's event loop. This is invisible to a + test suite that never measures latency under concurrency. +2. **Lifecycle operations that are not transactional.** Update and native + service migration both destroy working state before proving the replacement. +3. **Invariants enforced by prose or by a single-file test, so they drift.** + The `-WindowStyle Hidden` case in `001` is the clearest example. + +None of those three are things a per-file unit test naturally catches, which is +why 806 green files and an unhappy user base are consistent with each other. diff --git a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md new file mode 100644 index 0000000000..9011f646a9 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md @@ -0,0 +1,189 @@ +# 001 — Verified findings + +Every entry below was reproduced against the working tree at `474584bcd` on +2026-08-17. Line numbers are from that tree. Findings the audits raised that +could not be reproduced are listed at the bottom under "Not carried". + +Ranked by user impact. + +--- + +## F1 — `src/service.ts:2361` uses the exact PowerShell argv the codebase forbids + +`killWindowsServiceWrapperProcesses()` in `src/service.ts` spawns: + +```ts +// src/service.ts:2360-2363 +spawnSync(resolveTrustedWindowsPowerShellExe(), [ + "-NoProfile", "-NoLogo", "-NonInteractive", "-WindowStyle", "Hidden", + "-Command", ps, +], { stdio: "ignore", timeout: 5000, windowsHide: true }); +``` + +The codebase already knows this is wrong. `src/codex/user-identity.ts:222-224`: + +> Do not add PowerShell's `-WindowStyle Hidden` here: Bun 1.3.14 can fail that +> direct CLI combination before the SID command executes (#1589); the +> process-level `windowsHide` flag is sufficient. + +**Why it survived.** The regression test is scoped to one file: + +```ts +// tests/windows-deploy-close-regressions.test.ts:43 +expect(src).not.toContain('["-NoProfile", "-NoLogo", "-NonInteractive", "-WindowStyle", "Hidden", "-Command", ps]'); +``` + +`src` there is `read("src/update/job.ts")` (line 13). `src/service.ts` is never +checked. A search of `src/` finds exactly one surviving production occurrence +of that CLI pair: `src/service.ts:2361`. + +**User-visible consequence.** `stopServiceIfInstalled()` calls this function +because `schtasks /end` can leave the `wscript.exe`/`cmd.exe` wrapper alive, +which then respawns the proxy. The call ignores `spawnSync`'s exit status and +swallows errors, so under #1589 wrapper termination silently does nothing: +`ocx stop`, restart, and update appear to succeed and do not stick. + +Severity: high. Fix cost: one line. Phase 010. + +--- + +## F2 — The wrapper killer exists twice and the copies have drifted apart + +Two implementations of the same operation: + +```ts +// src/service.ts:2330 — canonical full-path token matching, scoped to THIS home +// src/update/job.ts:1377 +"$pats = @('opencodex-service.cmd','opencodex-service-launcher.vbs');" +... +"foreach ($p in $pats) { if ($c -like ('*' + $p + '*')) { return $true } };" +``` + +The updater copy matches a bare filename anywhere in a command line. Two +OpenCodex homes under one Windows account means a dashboard update for home A +can terminate home B's scheduler wrapper. Any unrelated process whose command +line contains either filename also matches. + +The drift is already measurable and runs in both directions: `update/job.ts` +received the #1589 argv cleanup that `service.ts` missed (F1); `service.ts` +received canonical path scoping that `update/job.ts` missed. Two copies, two +different half-fixes. + +Severity: high (cross-installation process kill). Phase 020. + +--- + +## F3 — Windows is not a gate, and the release gate cannot see that + +```yaml +# .github/workflows/ci.yml:547-552 +if: github.event_name == 'workflow_dispatch' +``` + +The aggregation job (`ci.yml:747-783`) accepts `skipped`. The release preflight +(`release.yml:181-201`) demands a successful **push-event** `ci.yml` run — +deliberately narrower than "any successful run for this SHA" — but +`platform-windows` never runs on push. Every release to date has therefore +published without executing a single Windows test. + +Severity: high, and it is the multiplier on every other finding — without it, +each fix below is one careless merge away from regressing. Phases 060 and 070. + +--- + +## F4 — Durable publishers do not share the Windows retry primitive + +`src/config.ts:102-123` knows about Windows sharing violations: + +```ts +const transientWindowsError = io.platform === "win32" + && (code === "EBUSY" || code === "EPERM" || code === "EACCES"); +if (!transientWindowsError || attempt >= 2) throw error; +io.sleep(25 * (attempt + 1)); +``` + +Two retries, 25ms then 50ms: about 75ms of total tolerance. Other durable +publishers do not call it at all and use raw `renameSync`: + +- `src/codex/prompt-journal.ts` — publishes a journal holding full + `config.toml` bytes +- `src/lib/config-ownership.ts` — publishes the uninstall ownership manifest + +These are fail-safe, not corrupting: they throw rather than publish a partial +file. But under a real-time scanner or a sync client holding the target, they +turn a recoverable hiccup into a user-visible operational failure. + +The 75ms envelope is itself a watch item, not yet a defect — we have no field +telemetry showing Defender or OneDrive holding files longer. Instrument before +widening. Phase 030 makes the primitive shared; Phase 031 adds the counters. + +--- + +## F5 — `chmod` is load-bearing where it does nothing + +`src/config.ts` calls `chmodSync(target, 0o600)` at lines 221, 316, 450, 1713, +2683 and `chmodSync(dir, 0o700)` at 1704, 2632, each wrapped in +`catch { /* platform may ignore chmod */ }`. On Windows the call is a no-op: +the ACL is what protects the file, and `src/lib/windows-secret-acl.ts` is what +sets it. + +Where both run, the file is protected. The audit work needed here is an +inventory: every path that writes a credential, token, or OAuth refresh token, +and whether the Windows ACL path is reached on that specific write or only the +`chmod`. `src/service.ts:1983` is explicit that the ACL is authoritative — +which is correct, and is exactly why any writer that lacks it is a gap. + +Treated as **unproven** until the inventory is done. Phase 040. Per AGENTS.md, +if that inventory turns up a live exposure the writeup goes to scratch space, +not into this directory. + +--- + +## F6 — The service wrapper retries a deterministic crash forever + +```bat +:: src/service.ts:1556-1563 +"%OCX_BUN%" "%OCX_CLI%" start ... +if %ERRORLEVEL% NEQ 0 ( + ... restarting in 5s + ping -n 6 127.0.0.1 >nul + goto loop +) +``` + +A proxy that starts successfully and then crashes deterministically is +relaunched every five seconds indefinitely. #1877 deliberately fixed only the +missing-executable case, on the reasoning that a flat "N failures then stop" +ceiling would break recovery from intermittent faults. That reasoning is sound; +the conclusion does not have to be an unbounded fixed-interval loop. + +Capped exponential backoff with a health-reset — 5s, 15s, 30s, 60s, reset after +sustained uptime — preserves recovery and stops the log storm. Phase 050. + +--- + +## F7 — Windows CI never proves crash-restart + +`.github/workflows/service-lifecycle.yml:104-135` kills the systemd MainPID, +waits for a different PID, and asserts `/healthz`. The Windows job +(`windows-schtasks`, line 239) only covers install, health, clean `ocx stop`, +uninstall. The restart path F6 describes has no coverage on the platform where +it is implemented in batch. Phase 051. + +--- + +## Not carried + +Raised by the audits, deliberately excluded: + +- **#1843 elevated `Start-Process` argv** — already fixed; PR #1860 merged and + present in the tree. +- **#31 passthrough SSE segfault** — fixed via `body.tee()`. +- **Bun replacing its own running executable during update** — + `src/update/index.ts:152-155` documents that the plain-Node launcher handles + npm self-update before Bun starts. +- **Synchronous `icacls`/CIM on the request path (#1852, #1298; PR #1876)** — + both P1 and P3 rate this their top runtime issue and the reasoning is + persuasive, but it is a latency property this session did not measure. It + belongs to the open PR, not to this unit. Recorded here so the next cycle + starts from it rather than rediscovering it. diff --git a/devlog/_plan/260817_windows_stability_program/002_sequencing.md b/devlog/_plan/260817_windows_stability_program/002_sequencing.md new file mode 100644 index 0000000000..c9a987c188 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/002_sequencing.md @@ -0,0 +1,50 @@ +# 002 — Sequencing and what this unit deliberately does not do + +## Order + +```mermaid +graph TD + A["010 forbidden argv"] --> B["020 wrapper killer dedupe"] + B --> C["030 shared replace retry"] + C --> D["031 retry telemetry"] + B --> E["050 wrapper backoff"] + E --> F["051 crash-restart CI"] + F --> G["060 windows CI gate"] + D --> G + G --> H["070 flakiness policy"] + G --> I["080 environment smoke"] + J["040 credential ACL inventory"] -.independent.-> G +``` + +The dependencies are real, not tidiness. 010 before 020 because the fix lands in +the copy that 020 deletes. 050 before 051 because there is no point testing a +loop that is about to change. Everything before 060 because a gate armed over +known-red is a gate that gets disarmed. + +040 is independent and can run any time; it produces a document, not a patch. + +## Out of scope for this unit + +**The synchronous-subprocess latency class.** Both P1 and P3 rank +`icacls`/PowerShell-CIM on the request path as the top runtime problem +(#1852, #1298, PR #1876), and their reasoning is convincing. It is excluded here +because this session measured nothing — no latency numbers, no event-loop +traces. Carrying it in would put an unverified claim next to seven verified +ones and devalue all of them. It is recorded at the end of `001` so the next +cycle inherits it instead of rediscovering it. Its natural home is #1876. + +**Update transactionality.** #1849 is open and the design work (stage outside +the live tree, verify, switch, retire the backup) is larger than any phase here. +Separate unit. + +## Definition of done for the unit + +- 010-051 landed with their guards driven red first. +- 060 through stage 4, so a release cannot publish on a run where Windows + silently skipped. +- 070's nightly running and its quarantine list open and reviewed. +- 080 items landed individually or explicitly recorded as not achievable. +- 040's table complete, with any live exposure handled in scratch per AGENTS.md. + +Until 060 stage 4 is done, every other phase in this unit is one merge away from +regressing. That is the point of the unit. diff --git a/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md b/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md new file mode 100644 index 0000000000..4b9c980449 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md @@ -0,0 +1,49 @@ +# 010 — Remove the forbidden `-WindowStyle Hidden` argv (F1) + +**Depends on:** nothing. This is the entry point of the unit. + +## Change + +`src/service.ts:2360-2363`, delete the CLI pair only: + +```diff + spawnSync(resolveTrustedWindowsPowerShellExe(), [ +- "-NoProfile", "-NoLogo", "-NonInteractive", "-WindowStyle", "Hidden", ++ "-NoProfile", "-NoLogo", "-NonInteractive", + "-Command", ps, + ], { stdio: "ignore", timeout: 5000, windowsHide: true }); +``` + +`windowsHide: true` stays — it is the flag that actually suppresses the console +window (#1278), and it is the one `src/codex/user-identity.ts:225` relies on. + +## Widen the guard so it cannot drift back + +`tests/windows-deploy-close-regressions.test.ts:43` asserts the bad argv only +against `src/update/job.ts`. Replace the single-file assertion with a sweep over +every `src/**/*.ts` that spawns PowerShell directly, asserting none passes +`-WindowStyle` adjacent to `Hidden` in an argv array. Keep the existing +`update/job.ts` assertion; this adds a family check rather than replacing one. + +Note `src/lib/windows-elevation.ts:622,660,687,736`, `src/tray/windows.ts:489` +and `src/update/job.ts:574` use `-WindowStyle Hidden` **inside a PowerShell +script string** passed to `Start-Process`/`ProcessStartInfo`. That is a +different construct and is not affected by #1589. The guard must match the argv +array form specifically, or it will fire on six correct call sites. + +## Verify + +```powershell +bun test tests/windows-deploy-close-regressions.test.ts +bun test tests/service.test.ts +``` + +Drive it red first: restore the two array elements, confirm the new assertion +fails, then remove them again. An assertion that has never failed is not a +guard. + +## Risk + +Low. The behavioral surface is one `spawnSync` that already ignores its exit +status. The regression risk is the guard being written loosely enough to match +the six legitimate script-string sites — hence the argv-shape requirement above. diff --git a/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md new file mode 100644 index 0000000000..79b01e5691 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md @@ -0,0 +1,47 @@ +# 020 — Collapse the duplicated scheduler-wrapper killer (F2) + +**Depends on:** 010 — that fix lands in one of the two copies, and this phase +removes the copy. Doing them in the other order means writing the fix twice. + +## Change + +New shared helper, `src/lib/windows-service-wrappers.ts`: + +```ts +export function killWindowsSchedulerWrappers(opts: { + scriptPath: string; // ...\opencodex-service.cmd + launcherPath: string; // ...\opencodex-service-launcher.vbs +}): void +``` + +Take the `src/service.ts:2330` implementation as the base — it is the correct +one. It builds canonical paths for *this* OpenCodex home and requires each to +appear as a complete command-line token, checking that the characters on either +side of the match are whitespace or a quote (`src/service.ts:2351-2356`). + +Then: + +- `src/service.ts` — `killWindowsServiceWrapperProcesses()` becomes a call into + the helper with this home's paths. +- `src/update/job.ts:1373-1392` — delete the bare-substring implementation + entirely and call the helper. The updater knows its target home; pass it. + +## Verify + +```powershell +bun test tests/service.test.ts +bun test tests/windows-deploy-close-regressions.test.ts +bun test tests/update-job.test.ts +``` + +Add a case asserting that a command line containing `opencodex-service.cmd` as +a *substring of a different absolute path* does not match. That is the exact +cross-home kill F2 describes, and it fails against today's `update/job.ts`. + +## Risk + +Medium — this is the phase that can regress `ocx stop`. The updater currently +kills more broadly than it should, so anything relying on that over-broad +behavior to clean up a stale wrapper will now leave it running. Check that the +updater passes the home it is actually updating, not the home of the process +doing the updating; on the dashboard path those can differ. diff --git a/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md b/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md new file mode 100644 index 0000000000..d0c1d6899c --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md @@ -0,0 +1,42 @@ +# 030 — Make the Windows replace-with-retry a shared primitive (F4) + +**Depends on:** nothing structurally, but sequence it after 020 so the service +and update paths are settled before touching the write path. + +## Change + +Export the retry loop currently inlined at `src/config.ts:102-123` as a +filesystem primitive both sync and async publishers call. It already has the +right shape: retry only on `win32` and only for `EBUSY`/`EPERM`/`EACCES`, +never masking a real error. The async twin at `src/config.ts:287-299` folds in +with it. + +Convert the raw `renameSync` publishers to the primitive: + +- `src/codex/prompt-journal.ts` — the journal carries full `config.toml` bytes; + a failure here is what breaks journal restore. +- `src/lib/config-ownership.ts` — the uninstall ownership manifest. + +Then sweep `src/` for remaining `renameSync` calls that publish a durable file +and either convert them or leave a comment saying why the file is transient. + +**Do not change the retry envelope in this phase.** It stays at two retries / +75ms. Widening it without evidence is how a 75ms hiccup becomes a 5s stall. + +## Verify + +```powershell +bun test tests/config.test.ts +bun test tests/codex-journal.test.ts +``` + +The existing `AtomicRenameIO` injection point (`src/config.ts:105-109`) already +makes this testable without a real sharing violation: inject a `rename` that +throws `EBUSY` twice then succeeds, and assert the publisher completes. + +## Risk + +Low-medium. The primitive is behavior-preserving for callers that already used +it. The new callers gain retries they did not have, which can only convert a +throw into a success. Watch for any caller that *depends* on `renameSync` +throwing promptly to detect a lock. diff --git a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md new file mode 100644 index 0000000000..cb64655b69 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md @@ -0,0 +1,34 @@ +# 031 — Instrument the retry envelope before widening it (F4) + +**Depends on:** 030. + +## Change + +Count, do not change. Each time the primitive from 030 retries, and each time it +exhausts its attempts, increment a counter tagged with the error code and the +publisher. Surface it wherever the existing diagnostic counters live — this must +not become a new logging surface, and per AGENTS.md it must never carry a path +that could identify the user, a request body, or a credential. Code and count +only. + +## Why this phase exists separately + +Both audits flagged the 75ms envelope. Neither could show it failing in the +field, and one explicitly declined to raise its severity for that reason. The +honest move is to measure first. If the counters stay at zero across a release, +the envelope is fine and this closes as NOOP. If they do not, 032 widens it with +bounded jittered backoff and cites the numbers. + +## Verify + +```powershell +bun run typecheck +bun run privacy:scan +bun test tests/config.test.ts +``` + +`privacy:scan` is the gate that matters here. + +## Risk + +Low. No behavioral change. diff --git a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md new file mode 100644 index 0000000000..78781e1cd1 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md @@ -0,0 +1,40 @@ +# 040 — Inventory every credential writer's Windows ACL coverage (F5) + +**Depends on:** nothing. Can run parallel to 010-030; sequence it after so its +findings land against a settled tree. + +## Change + +This phase produces a document, not a patch. + +Enumerate every path that writes a credential, token, OAuth refresh token, or +session secret. Starting points: `src/config.ts` (chmod sites at 221, 316, 450, +1713, 2683; dir sites 1704, 2632), `src/oauth/store.ts`, `src/service.ts:189` +and `:386`, `src/lab/artifacts/secure-fs.ts`, +`src/adapters/google-antigravity-replay.ts:251`. + +For each, record: the file written, whether `hardenSecretPath` (or the async +twin) runs on **that specific write**, and whether the `chmod` is the only +protection. `chmodSync` is a no-op on Windows; `src/service.ts:1983` says so +outright — "required Windows ACL is authoritative". A writer with only the +`chmod` has no protection on Windows at all. + +Output: a table in this unit listing writer, ACL status, and verdict. + +## If the inventory finds a live exposure + +Stop. Per AGENTS.md, pre-disclosure security material does not go in `devlog/` +— it goes to `.tmp/` or a `mktemp -d` path, and only the shipped fix plus its +regression test come back here. This phase's deliverable in that case is the +table with the exposed rows redacted and a pointer to the scratch location. + +## Verify + +Inventory correctness is verified by reading, not by a command. Each row cites +the writing line and the hardening line (or its absence). + +## Risk + +None to the runtime. The risk is doing it carelessly and recording a false +negative — a writer that looks covered because `hardenSecretPath` appears +somewhere in the file rather than on that path. diff --git a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md new file mode 100644 index 0000000000..91aa11384c --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md @@ -0,0 +1,52 @@ +# 050 — Bounded backoff for the service wrapper restart loop (F6) + +**Depends on:** 010 and 020 — both touch `src/service.ts` wrapper behavior, and +this phase edits the batch script that file generates. + +## Change + +`src/service.ts:1556-1563` currently sleeps a flat five seconds and loops +forever: + +```bat +if %ERRORLEVEL% NEQ 0 ( + ... restarting in 5s + ping -n 6 127.0.0.1 >nul + goto loop +) +``` + +Replace with capped exponential backoff plus a health reset: + +- delay sequence 5s, 15s, 30s, 60s, then hold at 60s; +- reset the delay to 5s once the child has stayed up past a health threshold + (10-15 minutes is the range both audits converged on); +- keep retrying indefinitely at the 60s cap. + +The cap, not a retry ceiling, is the design decision. #1877 declined a flat +"N failures then stop" because it breaks recovery from intermittent faults, and +that reasoning still holds. What it did not intend to preserve is a fixed 5s +cadence for a deterministic crash. + +Implementation constraint: this is batch. Tracking elapsed uptime in `cmd.exe` +without spawning helpers is awkward — capture a timestamp before the child +starts and compare after it exits, and keep the arithmetic in `set /a`. Do not +reach for PowerShell here; the wrapper must stay dependency-free. + +## Verify + +```powershell +bun test tests/service.test.ts +``` + +The wrapper is generated by `buildWindowsServiceScript()`, so assert on the +generated text: the sequence appears, the reset threshold appears, and the exit +code 3 incomplete-install branch added by #1877 still short-circuits before any +backoff. + +## Risk + +Medium. This changes recovery timing for every Windows service install. A +transient fault that previously recovered in 5s may now take up to 60s. That is +the intended trade, but it should be stated in the release note rather than +discovered. diff --git a/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md b/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md new file mode 100644 index 0000000000..da3f069179 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md @@ -0,0 +1,28 @@ +# 051 — Windows crash-restart coverage in service CI (F7) + +**Depends on:** 050 — test the behavior after it is worth testing. + +## Change + +`.github/workflows/service-lifecycle.yml` has a `windows-schtasks` job at line +239 covering install, health, clean `ocx stop`, uninstall. The Linux job at +lines 104-135 does more: it kills the systemd MainPID, waits for a *different* +PID, and asserts `/healthz` recovers. + +Add the Windows equivalent: kill the proxy process the scheduled task launched, +wait for the wrapper to relaunch it, assert a new PID and a healthy `/healthz`. + +With 050 landed, the first retry is still 5s, so the test does not need to wait +out the backoff curve. Give it margin anyway — hosted Windows runners are slow +and a tight bound here becomes the flake this unit is trying to prevent. + +## Verify + +The workflow is the verification. Run it on a branch, confirm it passes, then +confirm it *fails* when 050's backoff is reverted to a broken loop. + +## Risk + +Medium — this is new CI on the platform we are about to make required (060). +A flaky crash-restart test would poison that gate. Land it, watch it across +several runs, and only then let 060 depend on it. diff --git a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md new file mode 100644 index 0000000000..b5bac458b2 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md @@ -0,0 +1,51 @@ +# 060 — Stage Windows back into CI as a real gate (F3) + +**Depends on:** 010-051. Arming the gate before the known defects are fixed just +turns the gate red. + +## Change + +Staged, because flipping `if: github.event_name == 'workflow_dispatch'` in one +step is how a gate gets disabled again a week later. + +**Stage 1 — run it, do not gate on it.** Let `platform-windows` run on +`pull_request` and `push` with `continue-on-error: true`. Collect real data on +duration and failure rate across at least a week of normal merges. Nothing +blocks. + +**Stage 2 — resize the shards.** The current matrix is 4 shards over ~806 files, +roughly 200 files each. The 806/806 result was achieved in batches of ~60 files +because Bun 1.3.14 panics near 3.5GB RSS on larger runs, and CI-shaped shards +have reproduced that panic. Move to a shard size near the batch size that +actually worked. This is a prerequisite for gating, not an optimization: a gate +that fails on a runtime panic rather than a test failure teaches maintainers to +ignore it. + +**Stage 3 — gate on `pull_request`.** Remove `continue-on-error`. Windows now +blocks merges to `dev`. + +**Stage 4 — close the release hole.** `.github/workflows/ci.yml:747-783` accepts +`skipped` for every job. Once Windows runs on push, that tolerance must not +apply to it: assert `platform-windows` reached `success`, not +`success || skipped`. Otherwise `release.yml:181-201` keeps accepting a +push-event run in which Windows silently did nothing — which is the current +state described in `000`. + +Runner choice: hosted `windows-latest` for the gate. The self-hosted path +(`select-windows-runner`, `ci.yml:85`, repo variable `OCX_SELF_HOSTED_WINDOWS`) +stays what its own comment says it is — an operational switch, not a security +boundary — and a persistent runner carries state between runs, which is the +opposite of what a trustworthy gate needs. + +## Verify + +```powershell +gh workflow run ci.yml --ref +``` + +Each stage is verified by its own run history, not by the next stage. + +## Risk + +High if rushed, low if staged. The failure mode is a red gate everyone learns to +override. Stage 1's data is what tells us whether stage 3 is safe. diff --git a/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md b/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md new file mode 100644 index 0000000000..c50ad14ef4 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md @@ -0,0 +1,34 @@ +# 070 — Flakiness detection, not retry (F3) + +**Depends on:** 060 stage 1, which produces the data this policy needs. + +## Change + +The standing bar for this project is that flakiness is not tolerated. The usual +CI answer — automatic reruns — directly contradicts that: a rerun converts a +flake into a pass and destroys the evidence. + +Policy: + +- **Never auto-rerun a failed Windows job to make it green.** A rerun may be + used to *investigate*, and both results are recorded. +- **Detect instead.** A nightly scheduled run of the Windows suite on `dev`, + same shards as the gate. A test that passes in the gate and fails nightly, or + vice versa, on an unchanged tree is flaky by definition. +- **Quarantine explicitly.** A test identified as flaky gets an issue and a + named skip that states why and links the issue — never a silent + `test.skip`, never a widened timeout to make red go away. The existing budget + constants in `tests/helpers/test-budget.ts` are the sanctioned way to raise a + bound, and that file documents when doing so is legitimate. +- **Quarantine is a debt, not a resolution.** Quarantined tests are listed in + this unit and reviewed at each release. + +## Verify + +The nightly workflow's own history. After a month, the quarantine list should be +short and shrinking; if it grows, stage 3 of 060 was premature. + +## Risk + +Low mechanically. The real risk is social — a quarantine list that is easier to +append to than to drain. The per-release review is what stops that. diff --git a/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md new file mode 100644 index 0000000000..3793b441fe --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md @@ -0,0 +1,39 @@ +# 080 — Windows-specific smoke coverage that does not exist at all (F3) + +**Depends on:** 060 stage 3. Add these once the basic gate is trustworthy. + +## Change + +The unit suite tests logic. These test the environment, and no amount of unit +coverage substitutes for them. Each is a small job, added one at a time: + +1. **Non-ASCII username.** A profile path like `C:\Users\김병준` exercises + encoding through every path join, config write, and PowerShell invocation. + This machine's own user is ASCII, so nothing currently covers it. +2. **Long paths.** A working directory deep enough to cross MAX_PATH (260), + with and without `LongPathsEnabled`. +3. **Non-admin user.** File symlink creation throws EPERM unelevated. The suite + already skips those cases via a `canSymlink` probe; CI should prove the + *product* degrades correctly, not just that tests skip. +4. **OneDrive-redirected profile.** Known Folder redirection puts Documents and + Desktop under a synced path with a filter driver holding handles. This is the + most common real-world source of the sharing violations 030 and 031 address. +5. **Korean locale / code page 949.** Console encoding for a non-UTF-8 default + code page, which is this maintainer's own environment. +6. **Service across a reboot.** Install, reboot the runner, assert the proxy is + healthy. The single highest-value job on this list and the hardest to + arrange on hosted runners. +7. **Self-update end to end.** Install the previous published version, update to + the candidate, assert the CLI and service both survive. + +## Verify + +Each job passes on a branch before it is added to the required set. Add them +individually — a batch of seven new Windows jobs landing together makes the +first failure impossible to attribute. + +## Risk + +Medium and mostly about time. Several of these are slow, and 6 may not be +achievable on hosted runners at all; if not, record that limitation here rather +than quietly dropping it. From 9fa762c568d4d72db3e7da200b428ed2a01d139d Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:44:59 +0900 Subject: [PATCH 2/9] devlog: fold the r1 plan audit back into the Windows unit Round r1-20260817113441 returned FAIL with six blockers and four citation defects. All verified before acting on them; the auditor was right on every one. Citations corrected in 001: the updater's bare -like match is job.ts:1381 not :1377, and the service copy's token boundaries are service.ts:2350-2355 not :2330. Two overstatements withdrawn -- "every release ran zero Windows tests" ignores that service-shaped releases separately require service-lifecycle.yml, which does include a Windows job (release.yml:224-234); and service.ts:1983 argues the ACL-is-authoritative principle for an elevation staging directory, not for credential writers generally. Sequencing (002) was mostly wrong and is rewritten. Only 030->031 and 060 stage 1->070 are structural; the rest was file-overlap. 010->020 was backwards, 050->051 was fake and had produced an impossible verification claim, and "everything before 060" was false since nothing here makes the suite red today. 060 stage 1 and 051 now start immediately. 040 no longer writes a redacted table or a scratch pointer when it finds an exposure -- per AGENTS.md that is itself disclosure. It goes silent in the tree and reports verbally. 060 stops claiming a merge gate it cannot create: dev has no branch protection (MAINTAINERS.md:121,125). Stage 3 is a convention gate, stage 4 is the real one because release.yml reads conclusions directly. The hosted-vs-self-hosted contradiction is now a decision to make before stage 1 rather than two incompatible sentences. 031 names its module, counters, retrieval path and the constraint that the publisher label can never be path-derived. 030 specifies a new neutral module because config.ts:47 already imports config-ownership, so the obvious placement would close a cycle. 050 picks 600s and spells out the %TIME% midnight-wrap and space-pad traps. 080 drops to five implementable jobs plus two marked investigate-and-record. Missing verification commands added: typecheck on the source phases, full suite on 030, prepush on the workflow phases per .github/AGENTS.md:25. --- .../001_verified_findings.md | 26 +++-- .../002_sequencing.md | 102 ++++++++++++------ .../010_windowstyle_argv.md | 1 + .../020_wrapper_killer_dedupe.md | 1 + .../030_shared_replace_retry.md | 53 +++++---- .../031_retry_telemetry.md | 51 ++++++--- .../040_credential_acl_inventory.md | 44 +++++--- .../050_wrapper_backoff.md | 23 ++-- .../051_crash_restart_ci.md | 42 +++++--- .../060_windows_ci_gate.md | 99 +++++++++++------ .../070_flakiness_policy.md | 12 ++- .../080_environment_smoke.md | 101 ++++++++++++----- 12 files changed, 381 insertions(+), 174 deletions(-) diff --git a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md index 9011f646a9..24357485d7 100644 --- a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md +++ b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md @@ -52,8 +52,9 @@ Severity: high. Fix cost: one line. Phase 010. Two implementations of the same operation: ```ts -// src/service.ts:2330 — canonical full-path token matching, scoped to THIS home -// src/update/job.ts:1377 +// src/service.ts:2337-2358 — canonical token matching scoped to THIS home +// (paths built 2340-2341; token boundaries enforced 2350-2355) +// src/update/job.ts:1377-1383 (the bare -like match is line 1381) "$pats = @('opencodex-service.cmd','opencodex-service-launcher.vbs');" ... "foreach ($p in $pats) { if ($c -like ('*' + $p + '*')) { return $true } };" @@ -64,6 +65,8 @@ OpenCodex homes under one Windows account means a dashboard update for home A can terminate home B's scheduler wrapper. Any unrelated process whose command line contains either filename also matches. +Cited precisely: the updater's bare match is `src/update/job.ts:1381`; the service copy builds canonical paths at `src/service.ts:2340-2341` and enforces token boundaries at `:2350-2355`. + The drift is already measurable and runs in both directions: `update/job.ts` received the #1589 argv cleanup that `service.ts` missed (F1); `service.ts` received canonical path scoping that `update/job.ts` missed. Two copies, two @@ -80,11 +83,18 @@ Severity: high (cross-installation process kill). Phase 020. if: github.event_name == 'workflow_dispatch' ``` -The aggregation job (`ci.yml:747-783`) accepts `skipped`. The release preflight +The aggregation job accepts `skipped` (`ci.yml:771`). The release preflight (`release.yml:181-201`) demands a successful **push-event** `ci.yml` run — deliberately narrower than "any successful run for this SHA" — but -`platform-windows` never runs on push. Every release to date has therefore -published without executing a single Windows test. +`platform-windows` never runs on push. So the general release preflight does not require `platform-windows`, and a +release can publish without it having run. + +One qualification, because the stronger claim is not true: releases that touch +`src/service.ts`, `src/cli/index.ts`, `package.json` and a few others separately +require a green `service-lifecycle.yml` (`release.yml:224-234`), and that +workflow does include a Windows job. Windows is therefore not entirely absent +from release gating - it is absent from the *suite* gate, and present only as a +lifecycle smoke test for service-shaped changes. Severity: high, and it is the multiplier on every other finding — without it, each fix below is one careless merge away from regressing. Phases 060 and 070. @@ -130,8 +140,10 @@ sets it. Where both run, the file is protected. The audit work needed here is an inventory: every path that writes a credential, token, or OAuth refresh token, and whether the Windows ACL path is reached on that specific write or only the -`chmod`. `src/service.ts:1983` is explicit that the ACL is authoritative — -which is correct, and is exactly why any writer that lacks it is a gap. +`chmod`. `src/service.ts:1983` states the ACL is authoritative, but says so about an +elevation staging directory specifically. That is evidence for the principle, +not evidence about any credential writer's coverage - each inventory row needs +its own citation. Treated as **unproven** until the inventory is done. Phase 040. Per AGENTS.md, if that inventory turns up a live exposure the writeup goes to scratch space, diff --git a/devlog/_plan/260817_windows_stability_program/002_sequencing.md b/devlog/_plan/260817_windows_stability_program/002_sequencing.md index c9a987c188..f612ab4254 100644 --- a/devlog/_plan/260817_windows_stability_program/002_sequencing.md +++ b/devlog/_plan/260817_windows_stability_program/002_sequencing.md @@ -1,50 +1,88 @@ # 002 — Sequencing and what this unit deliberately does not do -## Order +The first draft of this document claimed a long dependency chain. A plan audit +(round `r1-20260817113441`) showed most of it was file-overlap dressed up as +dependency, and one link was backwards. This is the corrected version; the +reasoning is kept because "why we thought these were dependencies" is the more +useful record. + +## Real dependencies + +Only two links are structural: ```mermaid -graph TD - A["010 forbidden argv"] --> B["020 wrapper killer dedupe"] - B --> C["030 shared replace retry"] - C --> D["031 retry telemetry"] - B --> E["050 wrapper backoff"] - E --> F["051 crash-restart CI"] - F --> G["060 windows CI gate"] - D --> G - G --> H["070 flakiness policy"] - G --> I["080 environment smoke"] - J["040 credential ACL inventory"] -.independent.-> G +graph LR + A["030 shared replace primitive"] --> B["031 retry telemetry"] + C["060 stage 1 - run non-gating"] --> D["070 flakiness policy"] ``` -The dependencies are real, not tidiness. 010 before 020 because the fix lands in -the copy that 020 deletes. 050 before 051 because there is no point testing a -loop that is about to change. Everything before 060 because a gate armed over -known-red is a gate that gets disarmed. +`030 → 031` because there is nothing to instrument until the primitive exists. +`060 stage 1 → 070` because the flakiness policy is calibrated on the failure +data stage 1 produces. + +Everything else is schedulable now. + +## Start immediately, in parallel + +- **060 stage 1** — highest priority despite its number. It only makes Windows + *run*; it blocks nothing, and every later phase wants its data. Delaying it + delays the unit. +- **010** — one line plus a widened guard. +- **051** — crash-restart already exists, so it is testable today. Landing it + before 050 gives the timing change a baseline. +- **040** — independent inventory, produces a document. -040 is independent and can run any time; it produces a document, not a patch. +## Ordering preferences that are not dependencies + +Stated so nobody mistakes them for blockers: + +- **010 before 020** was originally justified as "otherwise the fix is written + twice". That is wrong: deduplicating first moves one flawed implementation, + and 010 then fixes it once. Either order works. Prefer 010 first only because + it is trivial and unblocks nothing else. +- **020 before 030** is people-not-colliding in `service.ts` and `job.ts`. +- **010/020 before 050** is the same, all three touch `src/service.ts`. +- **050 before 051** was fake, and worse, it produced an impossible verification + claim — 051 now says plainly that it cannot verify 050's backoff. +- **"everything before 060"** was false. None of F1, F2, F4, F5 or F6 makes the + suite red today. What is true is narrower: **060 stages 3 and 4** should wait + for the fixes, because that is when a Windows failure starts costing someone + a merge or a release. +- **080** starts non-gating alongside 060 stage 1 and does not wait for stage 3. ## Out of scope for this unit **The synchronous-subprocess latency class.** Both P1 and P3 rank `icacls`/PowerShell-CIM on the request path as the top runtime problem -(#1852, #1298, PR #1876), and their reasoning is convincing. It is excluded here -because this session measured nothing — no latency numbers, no event-loop -traces. Carrying it in would put an unverified claim next to seven verified -ones and devalue all of them. It is recorded at the end of `001` so the next -cycle inherits it instead of rediscovering it. Its natural home is #1876. +(#1852, #1298, PR #1876). It is excluded because this session measured nothing — +no latency numbers, no event-loop traces. Carrying it would put an unverified +claim beside seven verified ones and devalue all of them. + +The audit accepted that exclusion as honest and then made the sharper point: +because `000` itself names this the leading runtime class, finishing this unit +**cannot** establish "Windows is stable". It establishes a reliability and CI +baseline while the highest-ranked risk stays open in #1876. That is the accurate +claim and the one to make in any release note. + +**Update transactionality.** #1849 is open and the design work — stage outside +the live tree, verify, switch, retire the backup — is larger than any phase +here. Separate unit. -**Update transactionality.** #1849 is open and the design work (stage outside -the live tree, verify, switch, retire the backup) is larger than any phase here. -Separate unit. +**Branch protection.** 060 cannot make Windows block a merge; `dev` has no +protection and `MAINTAINERS.md:121` and `:125` record that enforcing anything +that way is an unmade decision. Configuring it is a maintainer call, not a +phase. ## Definition of done for the unit -- 010-051 landed with their guards driven red first. -- 060 through stage 4, so a release cannot publish on a run where Windows - silently skipped. -- 070's nightly running and its quarantine list open and reviewed. -- 080 items landed individually or explicitly recorded as not achievable. -- 040's table complete, with any live exposure handled in scratch per AGENTS.md. +- 010, 020, 030, 031, 050, 051 landed, each guard driven red before it counts. +- 060 through stage 4, so a release preflight cannot pass on a push run where + Windows silently skipped. +- 060's runner policy explicitly resolved rather than left implicit. +- 070's nightly running, quarantine list open and reviewed each release. +- 080 items 1-5 landed; items 6 and 7 landed or documented as not achievable. +- 040's inventory complete, with any live exposure handled entirely in scratch + per AGENTS.md and nothing about it written here. -Until 060 stage 4 is done, every other phase in this unit is one merge away from +Until 060 stage 4 is done, every fix in this unit is one careless merge from regressing. That is the point of the unit. diff --git a/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md b/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md index 4b9c980449..4e21eb7873 100644 --- a/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md +++ b/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md @@ -34,6 +34,7 @@ array form specifically, or it will fire on six correct call sites. ## Verify ```powershell +bun run typecheck bun test tests/windows-deploy-close-regressions.test.ts bun test tests/service.test.ts ``` diff --git a/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md index 79b01e5691..146432696c 100644 --- a/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md +++ b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md @@ -29,6 +29,7 @@ Then: ## Verify ```powershell +bun run typecheck bun test tests/service.test.ts bun test tests/windows-deploy-close-regressions.test.ts bun test tests/update-job.test.ts diff --git a/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md b/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md index d0c1d6899c..7f80a5fa6b 100644 --- a/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md +++ b/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md @@ -1,42 +1,51 @@ # 030 — Make the Windows replace-with-retry a shared primitive (F4) -**Depends on:** nothing structurally, but sequence it after 020 so the service -and update paths are settled before touching the write path. +**Depends on:** nothing structural. Sequence after 020 only to keep two people +out of the same files at once. ## Change -Export the retry loop currently inlined at `src/config.ts:102-123` as a -filesystem primitive both sync and async publishers call. It already has the -right shape: retry only on `win32` and only for `EBUSY`/`EPERM`/`EACCES`, -never masking a real error. The async twin at `src/config.ts:287-299` folds in -with it. +New module `src/lib/windows-atomic-replace.ts`. It must be a **new neutral +module, not an export from `config.ts`**: `src/config.ts:47` already imports +`./lib/config-ownership`, so having `config-ownership.ts` import back from +`config.ts` would close a cycle. -Convert the raw `renameSync` publishers to the primitive: +Move the retry loop from `src/config.ts:102-123` into it, keeping the shape +exactly: retry only on `win32`, only for `EBUSY`/`EPERM`/`EACCES`, never +masking another error, and keeping the `AtomicRenameIO` injection point +(`src/config.ts:105-109`) that makes it testable. The async twin at +`src/config.ts:287-299` moves with it. `config.ts` then imports from the new +module. -- `src/codex/prompt-journal.ts` — the journal carries full `config.toml` bytes; - a failure here is what breaks journal restore. -- `src/lib/config-ownership.ts` — the uninstall ownership manifest. +Convert the raw `renameSync` publishers: + +- `src/codex/prompt-journal.ts` — publishes a journal carrying full + `config.toml` bytes; a failure here is what breaks journal restore. +- `src/lib/config-ownership.ts` — publishes the uninstall ownership manifest. Then sweep `src/` for remaining `renameSync` calls that publish a durable file and either convert them or leave a comment saying why the file is transient. -**Do not change the retry envelope in this phase.** It stays at two retries / -75ms. Widening it without evidence is how a 75ms hiccup becomes a 5s stall. +**Do not change the retry envelope.** It stays at two retries / 75ms. Widening +it without evidence is how a 75ms hiccup becomes a 5s stall. 031 measures first. ## Verify ```powershell -bun test tests/config.test.ts -bun test tests/codex-journal.test.ts +bun run typecheck +bun run test ``` -The existing `AtomicRenameIO` injection point (`src/config.ts:105-109`) already -makes this testable without a real sharing violation: inject a `rename` that -throws `EBUSY` twice then succeeds, and assert the publisher completes. +The full suite, not a focused run: this touches shared config and the atomic +write path, which AGENTS.md names as the case where repository-wide validation +is required. + +Test via the injected `AtomicRenameIO` — a `rename` that throws `EBUSY` twice +then succeeds — rather than trying to produce a real sharing violation. ## Risk -Low-medium. The primitive is behavior-preserving for callers that already used -it. The new callers gain retries they did not have, which can only convert a -throw into a success. Watch for any caller that *depends* on `renameSync` -throwing promptly to detect a lock. +Low-medium. Behavior-preserving for existing callers; new callers gain retries +they lacked, which can only turn a throw into a success. Watch for any caller +that depends on `renameSync` throwing promptly to detect a lock. The import +cycle is the concrete trap — hence the neutral module. diff --git a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md index cb64655b69..8d05d3e658 100644 --- a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md +++ b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md @@ -1,23 +1,43 @@ # 031 — Instrument the retry envelope before widening it (F4) -**Depends on:** 030. +**Depends on:** 030. This is a genuine dependency: there is nothing to count +until the primitive exists. ## Change -Count, do not change. Each time the primitive from 030 retries, and each time it -exhausts its attempts, increment a counter tagged with the error code and the -publisher. Surface it wherever the existing diagnostic counters live — this must -not become a new logging surface, and per AGENTS.md it must never carry a path -that could identify the user, a request body, or a credential. Code and count -only. +Count, do not change behavior. -## Why this phase exists separately +Add to `src/lib/windows-atomic-replace.ts` (the module created in 030) a +module-scope counter keyed by `(code, publisher)` where `code` is the +`ErrnoException.code` that triggered the retry and `publisher` is a caller- +supplied string literal — `"config"`, `"prompt-journal"`, +`"config-ownership"`. Two counts per key: `retried` and `exhausted`. -Both audits flagged the 75ms envelope. Neither could show it failing in the -field, and one explicitly declined to raise its severity for that reason. The -honest move is to measure first. If the counters stay at zero across a release, -the envelope is fine and this closes as NOOP. If they do not, 032 widens it with -bounded jittered backoff and cites the numbers. +Export `readWindowsReplaceRetryCounters()` returning a plain snapshot object. + +Surface it on the existing management diagnostics route rather than inventing a +transport. The counters are process-lifetime and in-memory; they reset on +restart, and that is acceptable because the question being answered is "does +this ever fire at all", not "how often per hour". + +**Naming constraint:** the `publisher` value is a fixed literal chosen at the +call site. It must never be derived from a path, because a path can contain a +username. `privacy:scan` is the gate that enforces this and it must stay green. + +## How the evidence is actually collected + +In-memory counters cannot prove anything "across a release" on their own, so +the collection path is explicit: + +- Local: run the proxy through a normal session, hit the diagnostics route, + read the snapshot. Zero across ordinary use is itself a data point. +- CI: assert the counters exist and stay zero during the Windows suite. A + non-zero `exhausted` count in CI is a defect, not telemetry. +- Field: only if a user voluntarily includes a diagnostics snapshot in a bug + report. We do not collect this, and nothing in this phase transmits anything. + +If those three sources produce no evidence within a release cycle, 032 does not +happen and this closes NOOP. That is a legitimate outcome. ## Verify @@ -27,8 +47,7 @@ bun run privacy:scan bun test tests/config.test.ts ``` -`privacy:scan` is the gate that matters here. - ## Risk -Low. No behavioral change. +Low. No behavioral change to the retry path itself. The privacy surface is the +only thing worth reviewing. diff --git a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md index 78781e1cd1..aac1083976 100644 --- a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md +++ b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md @@ -1,11 +1,11 @@ # 040 — Inventory every credential writer's Windows ACL coverage (F5) -**Depends on:** nothing. Can run parallel to 010-030; sequence it after so its -findings land against a settled tree. +**Depends on:** nothing. Independent of every other phase, including 060 — an +inventory cannot gate CI and should not be sequenced as though it could. ## Change -This phase produces a document, not a patch. +This phase produces an inventory. Where it lands depends on what it finds. Enumerate every path that writes a credential, token, OAuth refresh token, or session secret. Starting points: `src/config.ts` (chmod sites at 221, 316, 450, @@ -15,26 +15,36 @@ and `:386`, `src/lab/artifacts/secure-fs.ts`, For each, record: the file written, whether `hardenSecretPath` (or the async twin) runs on **that specific write**, and whether the `chmod` is the only -protection. `chmodSync` is a no-op on Windows; `src/service.ts:1983` says so -outright — "required Windows ACL is authoritative". A writer with only the -`chmod` has no protection on Windows at all. +protection. `chmodSync` is a no-op on Windows, so a writer with only the +`chmod` has no protection there at all. -Output: a table in this unit listing writer, ACL status, and verdict. +On the ACL-is-authoritative principle: `src/service.ts:1983` states it, but for +an elevation staging directory specifically — it is evidence for the principle, +not for any credential writer's coverage. Each row needs its own citation. -## If the inventory finds a live exposure +## Where the output goes -Stop. Per AGENTS.md, pre-disclosure security material does not go in `devlog/` -— it goes to `.tmp/` or a `mktemp -d` path, and only the shipped fix plus its -regression test come back here. This phase's deliverable in that case is the -table with the exposed rows redacted and a pointer to the scratch location. +**If every writer is covered:** the table goes in this unit as `041`. It is a +clean bill of health, discloses nothing, and is worth having on record. + +**If any writer is not covered:** nothing goes in this unit. Not a redacted +table, not a pointer to a scratch path, not a row saying a gap exists. Per +AGENTS.md, pre-disclosure material stays entirely in scratch (`.tmp/` or a +`mktemp -d` path) until the fix ships. A tracked file saying "there is an +unfixed credential exposure, details elsewhere" is itself disclosure — it tells +a reader exactly where to look and that looking is worthwhile. + +In that case this phase reports its status verbally to the maintainer and stays +otherwise silent in the tree. The record comes back afterwards, in `_fin`, once +the fix and its regression test are public. ## Verify -Inventory correctness is verified by reading, not by a command. Each row cites -the writing line and the hardening line (or its absence). +Verified by reading. Each row cites the writing line and the hardening line, or +its absence. No command proves an inventory correct. ## Risk -None to the runtime. The risk is doing it carelessly and recording a false -negative — a writer that looks covered because `hardenSecretPath` appears -somewhere in the file rather than on that path. +None to the runtime. The risk is a false negative — marking a writer covered +because `hardenSecretPath` appears somewhere in the file rather than on that +code path. diff --git a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md index 91aa11384c..6bb2d2ccfe 100644 --- a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md +++ b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md @@ -19,8 +19,9 @@ if %ERRORLEVEL% NEQ 0 ( Replace with capped exponential backoff plus a health reset: - delay sequence 5s, 15s, 30s, 60s, then hold at 60s; -- reset the delay to 5s once the child has stayed up past a health threshold - (10-15 minutes is the range both audits converged on); +- reset the delay to 5s once the child has stayed up past **600 seconds**. One + number, not a range: the wrapper cannot express a policy, and leaving it open + means whoever implements it picks a number that never gets reviewed; - keep retrying indefinitely at the 60s cap. The cap, not a retry ceiling, is the design decision. #1877 declined a flat @@ -28,14 +29,24 @@ The cap, not a retry ceiling, is the design decision. #1877 declined a flat that reasoning still holds. What it did not intend to preserve is a fixed 5s cadence for a deterministic crash. -Implementation constraint: this is batch. Tracking elapsed uptime in `cmd.exe` -without spawning helpers is awkward — capture a timestamp before the child -starts and compare after it exits, and keep the arithmetic in `set /a`. Do not -reach for PowerShell here; the wrapper must stay dependency-free. +Implementation constraint: this is batch, and it must stay dependency-free — no +PowerShell inside the wrapper. + +The timing arithmetic needs care. `%TIME%` is locale-formatted and wraps at +midnight, so subtracting two samples can produce a negative uptime and reset the +backoff on a service that has been healthy for hours. Convert each sample to +seconds-since-midnight with `set /a`, and when the difference is negative add +86400 before comparing. `%TIME%` is also space-padded before 10:00, which breaks +naive `set /a` — strip the pad first. + +If that proves fragile under review, the fallback is a small state file beside +the wrapper holding the attempt index and last start time. It trades one file +write per restart for arithmetic a reviewer can check at a glance. ## Verify ```powershell +bun run typecheck bun test tests/service.test.ts ``` diff --git a/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md b/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md index da3f069179..4221e9c4fe 100644 --- a/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md +++ b/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md @@ -1,28 +1,46 @@ # 051 — Windows crash-restart coverage in service CI (F7) -**Depends on:** 050 — test the behavior after it is worth testing. +**Depends on:** nothing. Crash-restart exists today, so it is testable now — +and testing it *before* 050 changes the timing gives the change a baseline to +be measured against. Land this first if convenient. ## Change -`.github/workflows/service-lifecycle.yml` has a `windows-schtasks` job at line -239 covering install, health, clean `ocx stop`, uninstall. The Linux job at -lines 104-135 does more: it kills the systemd MainPID, waits for a *different* +`.github/workflows/service-lifecycle.yml` covers install, health, clean +`ocx stop`, uninstall in the `windows-schtasks` job (line 239). The Linux job +at lines 104-135 does more: it kills the systemd MainPID, waits for a different PID, and asserts `/healthz` recovers. Add the Windows equivalent: kill the proxy process the scheduled task launched, wait for the wrapper to relaunch it, assert a new PID and a healthy `/healthz`. -With 050 landed, the first retry is still 5s, so the test does not need to wait -out the backoff curve. Give it margin anyway — hosted Windows runners are slow -and a tight bound here becomes the flake this unit is trying to prevent. +## What this test does and does not prove + +It proves the wrapper relaunches a killed child. It does **not** prove anything +about 050's backoff curve: reverting 050 would leave a fixed five-second loop +that still relaunches, still yields a new PID, still restores health, and this +test would still pass. Do not present it as verification for 050. + +Backoff is verified separately in 050 by asserting on the text +`buildWindowsServiceScript()` generates. That is the honest split: this job +covers the runtime behavior, the source assertion covers the timing policy. + +A second job could prove the curve by crashing the child repeatedly and timing +the relaunches, but it would be slow and timing-sensitive on hosted runners — +exactly the flake profile 070 exists to prevent. Not proposed here. ## Verify -The workflow is the verification. Run it on a branch, confirm it passes, then -confirm it *fails* when 050's backoff is reverted to a broken loop. +```powershell +bun run prepush +gh workflow run service-lifecycle.yml --ref +``` + +Then confirm the job fails when the wrapper's relaunch branch is deliberately +broken. That is the red-first check that matters, and unlike the backoff +revert, it actually fails. ## Risk -Medium — this is new CI on the platform we are about to make required (060). -A flaky crash-restart test would poison that gate. Land it, watch it across -several runs, and only then let 060 depend on it. +Medium — new CI on a platform about to carry more weight. A flaky crash-restart +job would poison 060. Land it, watch several runs, then let 060 lean on it. diff --git a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md index b5bac458b2..8313794f90 100644 --- a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md +++ b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md @@ -1,51 +1,86 @@ -# 060 — Stage Windows back into CI as a real gate (F3) +# 060 — Stage Windows back into CI (F3) -**Depends on:** 010-051. Arming the gate before the known defects are fixed just -turns the gate red. +**Depends on:** 010-051 for stages 3 and 4. **Stage 1 depends on nothing and +should start immediately** — it is the source of the data 070 and the later +stages need, and delaying it delays everything downstream. -## Change +## What "gate" can and cannot mean here -Staged, because flipping `if: github.event_name == 'workflow_dispatch'` in one -step is how a gate gets disabled again a week later. +`dev` has no branch protection. `MAINTAINERS.md:121` is explicit that CODEOWNERS +requests reviews rather than enforcing them, and line 125 records that enforcing +any of it through branch protection is a separate decision that has not been +taken. `AGENTS.md` says the same about approval policy: enforced by convention. -**Stage 1 — run it, do not gate on it.** Let `platform-windows` run on -`pull_request` and `push` with `continue-on-error: true`. Collect real data on -duration and failure rate across at least a week of normal merges. Nothing -blocks. +So this phase cannot make Windows block a merge, and claiming otherwise would be +writing a plan against a repository that does not exist. What it can do: -**Stage 2 — resize the shards.** The current matrix is 4 shards over ~806 files, -roughly 200 files each. The 806/806 result was achieved in batches of ~60 files -because Bun 1.3.14 panics near 3.5GB RSS on larger runs, and CI-shaped shards -have reproduced that panic. Move to a shard size near the batch size that -actually worked. This is a prerequisite for gating, not an optimization: a gate -that fails on a runtime panic rather than a test failure teaches maintainers to -ignore it. +- make Windows **run** on `pull_request` and `push`, so a red result is visible + before a merge rather than never; +- make Windows **required by the release preflight**, which is real enforcement + because `release.yml` reads run conclusions directly (stage 4); +- leave actual merge blocking as an explicit, separately authorized branch- + protection change — out of scope for this unit and not something to configure + without the maintainer deciding it. -**Stage 3 — gate on `pull_request`.** Remove `continue-on-error`. Windows now -blocks merges to `dev`. +Stage 3 below is therefore a convention gate. Stage 4 is a real one. -**Stage 4 — close the release hole.** `.github/workflows/ci.yml:747-783` accepts +## Stages + +**Stage 1 — run it, block nothing.** `platform-windows` runs on +`pull_request` and `push` with `continue-on-error: true`. Collect duration and +failure rate across at least a week of normal merges. Start now. + +**Stage 2 — resize the shards.** The matrix is 4 shards over ~806 files, about +200 each. The 806/806 result came from batches of ~60 files because Bun 1.3.14 +panics near 3.5GB RSS on larger runs, and CI-shaped shards have reproduced that +panic. Shard nearer the batch size that actually worked. This is a prerequisite, +not an optimization: a leg that fails on a runtime panic instead of a test +failure teaches everyone to ignore it. + +**Stage 3 — remove `continue-on-error`.** Windows failures now fail the run and +are visible on the PR. Convention, not enforcement, per above. + +**Stage 4 — close the release hole.** `.github/workflows/ci.yml:771` accepts `skipped` for every job. Once Windows runs on push, that tolerance must not -apply to it: assert `platform-windows` reached `success`, not -`success || skipped`. Otherwise `release.yml:181-201` keeps accepting a -push-event run in which Windows silently did nothing — which is the current -state described in `000`. +apply to it: assert `platform-windows` reached `success`. Without this, +`release.yml:181-201` keeps accepting a push-event run in which Windows did +nothing. + +## Runner policy — decide this before stage 1 + +`select-windows-runner` (`ci.yml:85`) routes to a persistent self-hosted runner +when the repo variable `OCX_SELF_HOSTED_WINDOWS` is set, and push events are +exactly the trusted events that routing applies to. Push runs are also exactly +what the release preflight consumes. So "gate on hosted `windows-latest`" and +"keep the self-hosted selector as-is" cannot both hold. + +Resolve it explicitly, one of: + +1. **Hosted only for the gated legs.** Constrain the selector so `push` runs + land on `windows-latest` regardless of the variable, and leave self-hosted + for `workflow_dispatch` investigation. Clean, slower, costs more. +2. **Self-hosted allowed, with hygiene.** Keep the selector, and make the + existing "Clean workspace (self-hosted only)" step (`ci.yml:571`) a hard + requirement with a verified-clean assertion, since a persistent runner + carries state between runs and that is what makes a green result untrustworthy. -Runner choice: hosted `windows-latest` for the gate. The self-hosted path -(`select-windows-runner`, `ci.yml:85`, repo variable `OCX_SELF_HOSTED_WINDOWS`) -stays what its own comment says it is — an operational switch, not a security -boundary — and a persistent runner carries state between runs, which is the -opposite of what a trustworthy gate needs. +Option 1 is the recommendation. The self-hosted comment at `ci.yml:109` already +says the variable is an operational switch and not a security boundary; a +release gate wants the boundary. ## Verify ```powershell +bun run prepush gh workflow run ci.yml --ref ``` -Each stage is verified by its own run history, not by the next stage. +`bun run prepush` is required for CI and packaging workflow changes +(`.github/AGENTS.md:25`). Workflow edits also require the security review named +in `MAINTAINERS.md` — release automation and workflow permissions are on that +list. Each stage is verified by its own run history. ## Risk -High if rushed, low if staged. The failure mode is a red gate everyone learns to -override. Stage 1's data is what tells us whether stage 3 is safe. +High if rushed, low if staged. The failure mode is a red leg everyone learns to +override. Stage 1's data is what says whether stage 3 is safe. diff --git a/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md b/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md index c50ad14ef4..99cfb0358e 100644 --- a/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md +++ b/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md @@ -25,8 +25,16 @@ Policy: ## Verify -The nightly workflow's own history. After a month, the quarantine list should be -short and shrinking; if it grows, stage 3 of 060 was premature. +```powershell +bun run prepush +gh workflow run ci.yml --ref +``` + +The nightly workflow is a CI change, so `bun run prepush` applies +(`.github/AGENTS.md:25`), and workflow edits need the security review named in +`MAINTAINERS.md`. Beyond that, the policy is verified by its own run history: +after a month the quarantine list should be short and shrinking. If it grows, +060 stage 3 was premature. ## Risk diff --git a/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md index 3793b441fe..601f3e7fbc 100644 --- a/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md +++ b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md @@ -1,39 +1,84 @@ -# 080 — Windows-specific smoke coverage that does not exist at all (F3) +# 080 — Windows environment smoke coverage (F3) -**Depends on:** 060 stage 3. Add these once the basic gate is trustworthy. +**Depends on:** 060 stage 1, so these run alongside a Windows leg that already +executes. They start **non-gating** (`continue-on-error: true`) and do not wait +for stage 3. ## Change The unit suite tests logic. These test the environment, and no amount of unit -coverage substitutes for them. Each is a small job, added one at a time: - -1. **Non-ASCII username.** A profile path like `C:\Users\김병준` exercises - encoding through every path join, config write, and PowerShell invocation. - This machine's own user is ASCII, so nothing currently covers it. -2. **Long paths.** A working directory deep enough to cross MAX_PATH (260), - with and without `LongPathsEnabled`. -3. **Non-admin user.** File symlink creation throws EPERM unelevated. The suite - already skips those cases via a `canSymlink` probe; CI should prove the - *product* degrades correctly, not just that tests skip. -4. **OneDrive-redirected profile.** Known Folder redirection puts Documents and - Desktop under a synced path with a filter driver holding handles. This is the - most common real-world source of the sharing violations 030 and 031 address. -5. **Korean locale / code page 949.** Console encoding for a non-UTF-8 default - code page, which is this maintainer's own environment. -6. **Service across a reboot.** Install, reboot the runner, assert the proxy is - healthy. The single highest-value job on this list and the hardest to - arrange on hosted runners. -7. **Self-update end to end.** Install the previous published version, update to - the candidate, assert the CLI and service both survive. +coverage substitutes for them. Each is a separate job in +`.github/workflows/ci.yml`, added one at a time, in this order — cheapest and +most certain first. + +### 1. Non-ASCII username (do first) + +A profile path like `C:\Users\김병준` exercises encoding through every path +join, config write, and PowerShell invocation. On `windows-latest`: + +```powershell +$u = "ocxtest한글" +net user $u "P@ssw0rd-ocx-ci!" /add +``` + +then run `ocx doctor` and the config-write tests as that user via +`Start-Process -Credential`. Runner admin rights make local account creation +viable; this is the cheapest high-value item on the list. + +### 2. Long paths + +Check out into a directory deep enough to cross MAX_PATH (260). Two variants +via `HKLM:\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled` set +to 1 and 0. Assert install and first request succeed in both, or fail with a +legible message in the 0 case. + +### 3. Korean locale / code page 949 + +`chcp 949` before the CLI smoke, assert output is not mojibake. Cheap, and it +is the maintainer's own environment. + +### 4. Non-admin user + +Reuse the account from job 1 without elevation. Assert the product degrades +correctly where file symlinks throw EPERM — the suite already skips those cases +via a `canSymlink` probe, and skipping is not the same as degrading well. + +### 5. Self-update end to end + +`npm i -g @bitkyc08/opencodex@`, then update to a locally packed +tarball of the candidate, assert the CLI and service both survive. Uses +`npm pack`, so it needs no pre-publication registry artifact. + +### 6. OneDrive-redirected profile — investigate, do not schedule + +Known Folder redirection with a sync filter driver holding handles is the most +common real-world source of the sharing violations 030 and 031 address, and it +is the item we most want. It is also the one with no clean hosted-runner story: +provisioning OneDrive and a signed-in account on an ephemeral runner is not a +CI step, it is a project. Redirecting Known Folders to a local path via registry +reproduces the *path shape* but not the filter driver, which is the part that +matters. Timebox an investigation; if there is no honest way to reproduce it, +record that here and rely on 031's counters instead. + +### 7. Service across a reboot — likely not achievable, record the outcome + +The highest-value item and the hardest. Hosted runners do not survive a reboot +with the job intact. A self-hosted runner could, but that reintroduces exactly +the persistent-state problem 060 is trying to avoid for gating. Investigate, and +if the answer is no, say so here rather than leaving it on a list forever. ## Verify -Each job passes on a branch before it is added to the required set. Add them -individually — a batch of seven new Windows jobs landing together makes the -first failure impossible to attribute. +```powershell +bun run prepush +gh workflow run ci.yml --ref +``` + +Each job passes on a branch before joining the set. Add them individually — a +batch of seven new Windows jobs landing together makes the first failure +impossible to attribute. ## Risk -Medium and mostly about time. Several of these are slow, and 6 may not be -achievable on hosted runners at all; if not, record that limitation here rather -than quietly dropping it. +Medium, mostly time. Items 6 and 7 may not be achievable; the plan's obligation +is to reach a documented answer, not to keep them pending indefinitely. From 39fcb1a28ed276bc7b9d8488349fa4996f77820e Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:50:50 +0900 Subject: [PATCH 3/9] devlog: close the r2 blockers on the Windows unit Round r2 returned FAIL with five findings. Verified each; all five were real. The 050 %TIME% advice was wrong in a way that would have failed at runtime. set /a reads a leading zero as octal, so a minute or second component of 08 or 09 is a hard error -- confirmed on this machine, "set /a a=08" prints Invalid number. The doc now names four traps (octal, space padding, midnight wrap, delayed expansion inside the parenthesized branch given the wrapper's plain setlocal at service.ts:1522) and promotes the state-file approach from fallback to recommendation, since four traps plus an expansion-mode change is not arithmetic worth defending in review. 080's self-update job was not testing self-update. ocx update resolves its target from the registry (update/index.ts:167) and installs a resolved version (update/index.ts:106); there is no tarball injection seam, so a locally packed candidate cannot drive the real command. Renamed to a package replacement smoke, which is still worth having because npm replacing a live global install is the step that produced #1849, and the doc now says what covering ocx update would actually require. 031 named a producer but not a consumer. It now names system-routes.ts:49, a sibling endpoint rather than appending to /api/system/memory, the response shape, and the route test. 060 no longer says stage 1 depends on nothing and then gates it on the runner decision. Option 2 is downgraded honestly: the cleanup step removes stale checkout files, not installed services or registry state, which this product writes as normal behavior. Option 1 is the only one that closes the contradiction. Phase headers on 020, 050 and 080 still claimed dependencies that 002 had already withdrawn. Synchronized, and 002 now states it is authoritative on what is structural. --- .../002_sequencing.md | 12 +++-- .../020_wrapper_killer_dedupe.md | 6 ++- .../031_retry_telemetry.md | 19 ++++++-- .../050_wrapper_backoff.md | 48 ++++++++++++++----- .../060_windows_ci_gate.md | 20 +++++--- .../080_environment_smoke.md | 27 ++++++++--- 6 files changed, 98 insertions(+), 34 deletions(-) diff --git a/devlog/_plan/260817_windows_stability_program/002_sequencing.md b/devlog/_plan/260817_windows_stability_program/002_sequencing.md index f612ab4254..8b93c997a5 100644 --- a/devlog/_plan/260817_windows_stability_program/002_sequencing.md +++ b/devlog/_plan/260817_windows_stability_program/002_sequencing.md @@ -25,8 +25,9 @@ Everything else is schedulable now. ## Start immediately, in parallel - **060 stage 1** — highest priority despite its number. It only makes Windows - *run*; it blocks nothing, and every later phase wants its data. Delaying it - delays the unit. + *run*; it blocks nothing, and every later phase wants its data. Its one + prerequisite is the runner-policy decision inside 060, which is a decision to + make rather than work to schedule. - **010** — one line plus a widened guard. - **051** — crash-restart already exists, so it is testable today. Landing it before 050 gives the timing change a baseline. @@ -48,7 +49,12 @@ Stated so nobody mistakes them for blockers: suite red today. What is true is narrower: **060 stages 3 and 4** should wait for the fixes, because that is when a Windows failure starts costing someone a merge or a release. -- **080** starts non-gating alongside 060 stage 1 and does not wait for stage 3. +- **080** is simplest to add once 060 stage 1 has a Windows leg running, but it + is not blocked by it; it starts non-gating and does not wait for stage 3. + +Each phase header states its own dependency line. Where a header says "sequence +around" another phase, that is collision avoidance in shared files — `002` is +authoritative on what is structural, and only the two links above are. ## Out of scope for this unit diff --git a/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md index 146432696c..b250682256 100644 --- a/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md +++ b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md @@ -1,7 +1,9 @@ # 020 — Collapse the duplicated scheduler-wrapper killer (F2) -**Depends on:** 010 — that fix lands in one of the two copies, and this phase -removes the copy. Doing them in the other order means writing the fix twice. +**Depends on:** nothing structural. Either order works with 010: doing 020 first +moves one flawed implementation and 010 then fixes it once. Prefer 010 first +only because it is trivial. Both touch the same files, so sequence to avoid +collisions (see `002`). ## Change diff --git a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md index 8d05d3e658..60dbbc1b14 100644 --- a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md +++ b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md @@ -15,10 +15,21 @@ supplied string literal — `"config"`, `"prompt-journal"`, Export `readWindowsReplaceRetryCounters()` returning a plain snapshot object. -Surface it on the existing management diagnostics route rather than inventing a -transport. The counters are process-lifetime and in-memory; they reset on -restart, and that is acceptable because the question being answered is "does -this ever fire at all", not "how often per hour". +Surface it through `handleSystemRoutes` in +`src/server/management/system-routes.ts:49`, which is where process-level +diagnostics already live. Add a sibling endpoint rather than extending the +existing one: `GET /api/system/windows-replace-retries` returning +`{ counters: { [key]: { retried, exhausted } } }`. `/api/system/memory` +(line 51) returns a memory-shaped payload and appending unrelated counters to it +would make both harder to consume. + +The counters are process-lifetime and in-memory; they reset on restart, and that +is acceptable because the question is "does this ever fire at all", not "how +often per hour". + +Route test: extend `tests/system-routes.test.ts` with a case asserting the +endpoint returns the snapshot shape and that a simulated retry (via the injected +`AtomicRenameIO` from 030) increments the expected key. **Naming constraint:** the `publisher` value is a fixed literal chosen at the call site. It must never be derived from a path, because a path can contain a diff --git a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md index 6bb2d2ccfe..3280f203c4 100644 --- a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md +++ b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md @@ -1,7 +1,8 @@ # 050 — Bounded backoff for the service wrapper restart loop (F6) -**Depends on:** 010 and 020 — both touch `src/service.ts` wrapper behavior, and -this phase edits the batch script that file generates. +**Depends on:** nothing structural. 010 and 020 also touch `src/service.ts`, so +sequence around them to avoid collisions — that is scheduling, not dependency +(see `002`). ## Change @@ -32,16 +33,41 @@ cadence for a deterministic crash. Implementation constraint: this is batch, and it must stay dependency-free — no PowerShell inside the wrapper. -The timing arithmetic needs care. `%TIME%` is locale-formatted and wraps at -midnight, so subtracting two samples can produce a negative uptime and reset the -backoff on a service that has been healthy for hours. Convert each sample to -seconds-since-midnight with `set /a`, and when the difference is negative add -86400 before comparing. `%TIME%` is also space-padded before 10:00, which breaks -naive `set /a` — strip the pad first. +The timing arithmetic has four separate traps, and all of them bite. -If that proves fragile under review, the fallback is a small state file beside -the wrapper holding the attempt index and last start time. It trades one file -write per restart for arithmetic a reviewer can check at a glance. +**Octal.** `set /a` reads a leading zero as octal, so a minute or second +component of `08` or `09` is a hard error. Verified on this machine: + +```text +C:\> set /a a=08 +Invalid number. Numeric constants are either decimal (17), +hexadecimal (0x11), or octal (021). +``` + +Every component extracted from `%TIME%` must be forced to decimal. The standard +idiom prefixes `1` and subtracts 100: `set /a mm=1%TIME:~3,2% - 100`. + +**Space padding.** `%TIME%` pads the hour with a space before 10:00, so +`%TIME:~0,2%` yields a leading space. The `1`-prefix idiom does not fix that; +replace the space first (`set t=%TIME: =0%`) and apply the prefix trick to each +component of `t`. + +**Midnight wrap.** Seconds-since-midnight goes backwards across midnight, which +reads as negative uptime and would reset the backoff on a service healthy for +hours. When the difference is negative, add 86400. + +**Delayed expansion.** The generated wrapper uses plain `setlocal` +(`src/service.ts:1522`). Inside the parenthesized restart branch a `%VAR%` +expands once when the block is parsed, so a counter incremented in that block +reads stale. Either add `setlocal enabledelayedexpansion` and use `!VAR!`, or +keep the state outside the block. Changing the wrapper preamble is its own +reviewable decision. + +Given four traps and an expansion-mode change, the state file is the +**recommended** implementation rather than the fallback: a small file beside the +wrapper holding the attempt index and last start time, trading one write per +restart for arithmetic a reviewer can check at a glance. Decide before writing +the batch, not during review. ## Verify diff --git a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md index 8313794f90..cd07135734 100644 --- a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md +++ b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md @@ -1,8 +1,10 @@ # 060 — Stage Windows back into CI (F3) -**Depends on:** 010-051 for stages 3 and 4. **Stage 1 depends on nothing and -should start immediately** — it is the source of the data 070 and the later -stages need, and delaying it delays everything downstream. +**Depends on:** stages 3 and 4 want 010-051 landed, because that is when a +Windows failure starts costing someone a merge or a release. Stage 1 needs only +the runner-policy decision below — which is a decision, not a phase, and should +be made today. Nothing else blocks it, and delaying it delays every phase that +wants its data. ## What "gate" can and cannot mean here @@ -46,7 +48,7 @@ apply to it: assert `platform-windows` reached `success`. Without this, `release.yml:181-201` keeps accepting a push-event run in which Windows did nothing. -## Runner policy — decide this before stage 1 +## Runner policy — the one decision stage 1 waits on `select-windows-runner` (`ci.yml:85`) routes to a persistent self-hosted runner when the repo variable `OCX_SELF_HOSTED_WINDOWS` is set, and push events are @@ -64,9 +66,13 @@ Resolve it explicitly, one of: requirement with a verified-clean assertion, since a persistent runner carries state between runs and that is what makes a green result untrustworthy. -Option 1 is the recommendation. The self-hosted comment at `ci.yml:109` already -says the variable is an operational switch and not a security boundary; a -release gate wants the boundary. +Option 1 is the recommendation, and it is the only one that closes the +contradiction outright. Option 2 narrows it rather than closing it: the existing +cleanup step removes stale checkout files, not installed services, registry +state, tool caches, or anything else a previous run left on the machine — and +this product installs services and writes registry state as its normal +behavior. The `ci.yml:109` comment already says the variable is an operational +switch and not a security boundary; a release gate wants the boundary. ## Verify diff --git a/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md index 601f3e7fbc..9e79c00d06 100644 --- a/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md +++ b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md @@ -1,8 +1,8 @@ # 080 — Windows environment smoke coverage (F3) -**Depends on:** 060 stage 1, so these run alongside a Windows leg that already -executes. They start **non-gating** (`continue-on-error: true`) and do not wait -for stage 3. +**Depends on:** nothing structural. These are simplest to add alongside 060 +stage 1, once a Windows leg is already executing, and they start **non-gating** +(`continue-on-error: true`). They do not wait for stage 3. ## Change @@ -43,11 +43,24 @@ Reuse the account from job 1 without elevation. Assert the product degrades correctly where file symlinks throw EPERM — the suite already skips those cases via a `canSymlink` probe, and skipping is not the same as degrading well. -### 5. Self-update end to end +### 5. Package replacement smoke (not `ocx update`) -`npm i -g @bitkyc08/opencodex@`, then update to a locally packed -tarball of the candidate, assert the CLI and service both survive. Uses -`npm pack`, so it needs no pre-publication registry artifact. +`npm i -g @bitkyc08/opencodex@`, then `npm i -g` a locally packed +tarball of the candidate, then assert the CLI still runs and the service still +responds. This exercises **npm replacing a live global install on Windows** — +the step that produced #1849 — and it needs no pre-publication registry +artifact. + +It is deliberately **not** an `ocx update` test, and must not be described as +one. `ocx update` resolves its target from the registry +(`src/update/index.ts:167`) and installs `@bitkyc08/opencodex@` +(`src/update/index.ts:106`). There is no seam for injecting a local tarball, so +the real command cannot be driven against an unpublished candidate. + +Covering `ocx update` itself needs one of: a published prerelease to update +*to*, or an injection seam in `updateCommand()` for a candidate target. The +second is a source change and belongs in the #1849 unit, not here. Until one +exists, this job covers the npm mechanics and says so. ### 6. OneDrive-redirected profile — investigate, do not schedule From ba7e90a1a6e49e6ae3e6625e6dc22af9b77e5888 Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:54:12 +0900 Subject: [PATCH 4/9] devlog: close the two r3 near-pass points tests/system-routes.test.ts does not exist -- confirmed. Current handleSystemRoutes coverage sits in memory-watchdog.test.ts:171 and codex-restart-route.test.ts:11. 031 now says to create the file rather than extend it, and its verify block runs it. The state file was oversold in 050. It removes the delayed-expansion problem because the counter is read fresh each iteration, but it does not remove the elapsed-time arithmetic: a stored start timestamp still has to be parsed and subtracted, so the octal, padding and midnight-wrap rules apply either way. The doc now splits it -- state file for the retry counter, documented arithmetic for the 600s uptime reset -- and names the file's own questions: location, what happens when the write fails (treat as a fresh counter, never fail the restart), and removal on uninstall alongside the wrapper and launcher. --- .../031_retry_telemetry.md | 10 ++++++--- .../050_wrapper_backoff.md | 21 ++++++++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md index 60dbbc1b14..676af51921 100644 --- a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md +++ b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md @@ -27,9 +27,12 @@ The counters are process-lifetime and in-memory; they reset on restart, and that is acceptable because the question is "does this ever fire at all", not "how often per hour". -Route test: extend `tests/system-routes.test.ts` with a case asserting the -endpoint returns the snapshot shape and that a simulated retry (via the injected -`AtomicRenameIO` from 030) increments the expected key. +Route test: `tests/system-routes.test.ts` does not exist — current +`handleSystemRoutes` coverage is spread across `tests/memory-watchdog.test.ts` +(line 171) and `tests/codex-restart-route.test.ts` (line 11). Create +`tests/system-routes.test.ts` for this endpoint: assert the snapshot shape, and +assert that a simulated retry driven through the injected `AtomicRenameIO` from +030 increments the expected key. **Naming constraint:** the `publisher` value is a fixed literal chosen at the call site. It must never be derived from a path, because a path can contain a @@ -56,6 +59,7 @@ happen and this closes NOOP. That is a legitimate outcome. bun run typecheck bun run privacy:scan bun test tests/config.test.ts +bun test tests/system-routes.test.ts ``` ## Risk diff --git a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md index 3280f203c4..4a7a6313fe 100644 --- a/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md +++ b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md @@ -63,11 +63,22 @@ reads stale. Either add `setlocal enabledelayedexpansion` and use `!VAR!`, or keep the state outside the block. Changing the wrapper preamble is its own reviewable decision. -Given four traps and an expansion-mode change, the state file is the -**recommended** implementation rather than the fallback: a small file beside the -wrapper holding the attempt index and last start time, trading one write per -restart for arithmetic a reviewer can check at a glance. Decide before writing -the batch, not during review. +Given four traps and an expansion-mode change, prefer a state file for the +**retry counter** — a small file beside the wrapper holding the attempt index, +which removes the delayed-expansion problem entirely because the value is read +fresh each iteration rather than expanded when the block is parsed. + +Be clear about what that does not solve. The 600-second uptime reset still needs +an elapsed-time comparison, so the octal, padding and midnight-wrap rules above +apply either way — a state file storing a start timestamp still has to parse and +subtract it. The file also brings its own questions: where it lives, what happens +when the write fails (treat as a fresh counter and keep going, never fail the +restart), and removal on uninstall alongside the wrapper and launcher. + +So: state file for the counter, documented arithmetic for the uptime check, and +if review prefers to avoid a file altogether, `setlocal enabledelayedexpansion` +with `!VAR!` is the in-memory equivalent. Decide before writing the batch, not +during review. ## Verify From 038fbad633b656c20c5e8b89e51f93e0de97aacf Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:06:18 +0900 Subject: [PATCH 5/9] devlog: close the r5 blockers on the Windows unit Independent final-gate review returned FAIL with three findings and three citation corrections. All verified; all correct. 031 claimed privacy:scan enforces the fixed-literal publisher label. It does not -- privacy-scan.ts:187 is a textual scanner over file content matching home paths, emails and token shapes, and it cannot see that a runtime value was path-derived. Replaced with a closed union type so a path-derived string fails typecheck instead, plus a test asserting the snapshot keys are a subset of it. privacy:scan stays in the verify block as a backstop, not as the mechanism. 031 also claimed CI would assert the counters stay zero across the Windows suite. The counters are process-local and the suite runs across four sharded runners in many short-lived processes with no endpoint to query, so that assertion needs a suite finalizer and a collection step -- a design of its own. The CI claim is withdrawn rather than left as an instruction nobody could follow, and the evidence section now says plainly that local runs and voluntary bug reports are the only sources. 040's seed list missed config.ts:3937, the invalid-config backup, which copies the whole config including any secrets in it. Added, and the phase now says to re-derive every chmodSync call rather than trust the seeds -- an incomplete seed list is exactly the false negative that phase exists to avoid. Citations: the updater's bare -like match is job.ts:1383 not :1381; the skipped allowance is the jq filter at ci.yml:769-772; release.yml service enforcement runs to :241 with the failure at 235-239. --- .../001_verified_findings.md | 15 +++++--- .../031_retry_telemetry.md | 37 +++++++++++++++---- .../040_credential_acl_inventory.md | 10 ++++- .../060_windows_ci_gate.md | 4 +- 4 files changed, 49 insertions(+), 17 deletions(-) diff --git a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md index 24357485d7..8d143d3c42 100644 --- a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md +++ b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md @@ -54,7 +54,7 @@ Two implementations of the same operation: ```ts // src/service.ts:2337-2358 — canonical token matching scoped to THIS home // (paths built 2340-2341; token boundaries enforced 2350-2355) -// src/update/job.ts:1377-1383 (the bare -like match is line 1381) +// src/update/job.ts:1377-1383 (the bare -like match is line 1383) "$pats = @('opencodex-service.cmd','opencodex-service-launcher.vbs');" ... "foreach ($p in $pats) { if ($c -like ('*' + $p + '*')) { return $true } };" @@ -65,7 +65,7 @@ OpenCodex homes under one Windows account means a dashboard update for home A can terminate home B's scheduler wrapper. Any unrelated process whose command line contains either filename also matches. -Cited precisely: the updater's bare match is `src/update/job.ts:1381`; the service copy builds canonical paths at `src/service.ts:2340-2341` and enforces token boundaries at `:2350-2355`. +Cited precisely: the updater's bare match is `src/update/job.ts:1383`; the service copy builds canonical paths at `src/service.ts:2340-2341` and enforces token boundaries at `:2350-2355`. The drift is already measurable and runs in both directions: `update/job.ts` received the #1589 argv cleanup that `service.ts` missed (F1); `service.ts` @@ -83,7 +83,8 @@ Severity: high (cross-installation process kill). Phase 020. if: github.event_name == 'workflow_dispatch' ``` -The aggregation job accepts `skipped` (`ci.yml:771`). The release preflight +The aggregation job accepts `skipped` (`ci.yml:769-772` — the jq filter keeps +only jobs that are neither `success` nor `skipped`). The release preflight (`release.yml:181-201`) demands a successful **push-event** `ci.yml` run — deliberately narrower than "any successful run for this SHA" — but `platform-windows` never runs on push. So the general release preflight does not require `platform-windows`, and a @@ -91,7 +92,8 @@ release can publish without it having run. One qualification, because the stronger claim is not true: releases that touch `src/service.ts`, `src/cli/index.ts`, `package.json` and a few others separately -require a green `service-lifecycle.yml` (`release.yml:224-234`), and that +require a green `service-lifecycle.yml` (`release.yml:224-241`, enforced at +235-239), and that workflow does include a Windows job. Windows is therefore not entirely absent from release gating - it is absent from the *suite* gate, and present only as a lifecycle smoke test for service-shaped changes. @@ -132,8 +134,9 @@ widening. Phase 030 makes the primitive shared; Phase 031 adds the counters. ## F5 — `chmod` is load-bearing where it does nothing `src/config.ts` calls `chmodSync(target, 0o600)` at lines 221, 316, 450, 1713, -2683 and `chmodSync(dir, 0o700)` at 1704, 2632, each wrapped in -`catch { /* platform may ignore chmod */ }`. On Windows the call is a no-op: +2683 and 3937, and `chmodSync(dir, 0o700)` at 1704, 2632, each wrapped in +`catch { /* platform may ignore chmod */ }`. The 3937 site is the invalid-config +backup, which copies the whole config including whatever secrets it held. On Windows the call is a no-op: the ACL is what protects the file, and `src/lib/windows-secret-acl.ts` is what sets it. diff --git a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md index 676af51921..8fa324a608 100644 --- a/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md +++ b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md @@ -34,9 +34,23 @@ Route test: `tests/system-routes.test.ts` does not exist — current assert that a simulated retry driven through the injected `AtomicRenameIO` from 030 increments the expected key. -**Naming constraint:** the `publisher` value is a fixed literal chosen at the -call site. It must never be derived from a path, because a path can contain a -username. `privacy:scan` is the gate that enforces this and it must stay green. +**Naming constraint:** the `publisher` value must be a fixed literal chosen at +the call site and never derived from a path, because a path can contain a +username. + +`privacy:scan` does **not** enforce that. It is a textual scanner over file +content (`scripts/privacy-scan.ts:187`) matching home paths, emails and token +shapes; it cannot see that a runtime value was path-derived. Enforce it in the +type system instead: declare a closed union + +```ts +type ReplacePublisher = "config" | "prompt-journal" | "config-ownership"; +``` + +and type the counter API to accept only that. A path-derived string then fails +`bun run typecheck` rather than passing a scan. Add a test asserting the +snapshot's keys are a subset of the union. Keep `privacy:scan` in the verify +block as a backstop for the endpoint's response, not as the mechanism. ## How the evidence is actually collected @@ -45,13 +59,22 @@ the collection path is explicit: - Local: run the proxy through a normal session, hit the diagnostics route, read the snapshot. Zero across ordinary use is itself a data point. -- CI: assert the counters exist and stay zero during the Windows suite. A - non-zero `exhausted` count in CI is a defect, not telemetry. +- CI: **not in this phase.** The counters are process-local, and the Windows + suite runs across four sharded runners in many short-lived processes, none of + which exposes an endpoint to query. Making "stayed zero across the suite" a CI + assertion needs a suite finalizer that aggregates per-process state and a + workflow step to collect it — a design of its own, not a line in this phase. + What CI covers here is the route test above, nothing more. - Field: only if a user voluntarily includes a diagnostics snapshot in a bug report. We do not collect this, and nothing in this phase transmits anything. -If those three sources produce no evidence within a release cycle, 032 does not -happen and this closes NOOP. That is a legitimate outcome. +So the evidence comes from local runs and voluntary bug reports, not from CI. +That is thinner than it first looked, and it is the honest description: this +phase can show the counters firing, but it cannot prove a negative at scale +without the aggregation work above. + +If no evidence appears within a release cycle, 032 does not happen and this +closes NOOP. That is a legitimate outcome. ## Verify diff --git a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md index aac1083976..d2b6a02947 100644 --- a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md +++ b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md @@ -9,10 +9,16 @@ This phase produces an inventory. Where it lands depends on what it finds. Enumerate every path that writes a credential, token, OAuth refresh token, or session secret. Starting points: `src/config.ts` (chmod sites at 221, 316, 450, -1713, 2683; dir sites 1704, 2632), `src/oauth/store.ts`, `src/service.ts:189` -and `:386`, `src/lab/artifacts/secure-fs.ts`, +1713, 2683, and **3937** — the invalid-config backup, which copies the whole +config including any secrets in it; dir sites 1704, 2632), `src/oauth/store.ts`, +`src/service.ts:189` and `:386`, `src/lab/artifacts/secure-fs.ts`, `src/adapters/google-antigravity-replay.ts:251`. +These are seeds, not the list. Start by re-deriving every `chmodSync` call in +`src/` rather than trusting this enumeration — an incomplete seed list is +exactly the false negative this phase exists to avoid, and the 3937 site was +missed on the first pass. + For each, record: the file written, whether `hardenSecretPath` (or the async twin) runs on **that specific write**, and whether the `chmod` is the only protection. `chmodSync` is a no-op on Windows, so a writer with only the diff --git a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md index cd07135734..4fd6fbd828 100644 --- a/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md +++ b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md @@ -42,8 +42,8 @@ failure teaches everyone to ignore it. **Stage 3 — remove `continue-on-error`.** Windows failures now fail the run and are visible on the PR. Convention, not enforcement, per above. -**Stage 4 — close the release hole.** `.github/workflows/ci.yml:771` accepts -`skipped` for every job. Once Windows runs on push, that tolerance must not +**Stage 4 — close the release hole.** The aggregation job accepts `skipped` for +every job (`.github/workflows/ci.yml:769-772`). Once Windows runs on push, that tolerance must not apply to it: assert `platform-windows` reached `success`. Without this, `release.yml:181-201` keeps accepting a push-event run in which Windows did nothing. From cc5ba67d2cb6b092f72f10564481424f027ba685 Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:09:38 +0900 Subject: [PATCH 6/9] devlog: correct the invalid-config backup chmod line 3937 declares backupInvalidConfig; the chmodSync(backupPath, 0o600) call is line 3942. Corrected in 001 F5 and in 040's seed list, with the declaring function named so the citation stays legible if the file shifts. --- .../001_verified_findings.md | 7 ++++--- .../040_credential_acl_inventory.md | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md index 8d143d3c42..c9d3e2b7b3 100644 --- a/devlog/_plan/260817_windows_stability_program/001_verified_findings.md +++ b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md @@ -134,9 +134,10 @@ widening. Phase 030 makes the primitive shared; Phase 031 adds the counters. ## F5 — `chmod` is load-bearing where it does nothing `src/config.ts` calls `chmodSync(target, 0o600)` at lines 221, 316, 450, 1713, -2683 and 3937, and `chmodSync(dir, 0o700)` at 1704, 2632, each wrapped in -`catch { /* platform may ignore chmod */ }`. The 3937 site is the invalid-config -backup, which copies the whole config including whatever secrets it held. On Windows the call is a no-op: +2683 and 3942, and `chmodSync(dir, 0o700)` at 1704, 2632, each wrapped in +`catch { /* platform may ignore chmod */ }`. The 3942 site sits inside +`backupInvalidConfig` (declared at 3937), which copies the whole config +including whatever secrets it held. On Windows the call is a no-op: the ACL is what protects the file, and `src/lib/windows-secret-acl.ts` is what sets it. diff --git a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md index d2b6a02947..45b0eb05cf 100644 --- a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md +++ b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md @@ -16,7 +16,7 @@ config including any secrets in it; dir sites 1704, 2632), `src/oauth/store.ts`, These are seeds, not the list. Start by re-deriving every `chmodSync` call in `src/` rather than trusting this enumeration — an incomplete seed list is -exactly the false negative this phase exists to avoid, and the 3937 site was +exactly the false negative this phase exists to avoid, and the 3942 site was missed on the first pass. For each, record: the file written, whether `hardenSecretPath` (or the async From 6a4101f3db7eadb8144a80fdc14264ce8ec8177a Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:09:50 +0900 Subject: [PATCH 7/9] devlog: fix the remaining 3937 reference in the 040 seed list --- .../040_credential_acl_inventory.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md index 45b0eb05cf..24b1d2b9d7 100644 --- a/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md +++ b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md @@ -9,8 +9,9 @@ This phase produces an inventory. Where it lands depends on what it finds. Enumerate every path that writes a credential, token, OAuth refresh token, or session secret. Starting points: `src/config.ts` (chmod sites at 221, 316, 450, -1713, 2683, and **3937** — the invalid-config backup, which copies the whole -config including any secrets in it; dir sites 1704, 2632), `src/oauth/store.ts`, +1713, 2683, and **3942** — the invalid-config backup (inside +`backupInvalidConfig`, declared at 3937), which copies the whole config +including any secrets it held; dir sites 1704, 2632), `src/oauth/store.ts`, `src/service.ts:189` and `:386`, `src/lab/artifacts/secure-fs.ts`, `src/adapters/google-antigravity-replay.ts:251`. From f9cb0fcd48bcbb75fca25a1bb172d625592957de Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:14:09 +0900 Subject: [PATCH 8/9] devlog: record the seven audit rounds and what they corrected --- .../003_audit_record.md | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 devlog/_plan/260817_windows_stability_program/003_audit_record.md diff --git a/devlog/_plan/260817_windows_stability_program/003_audit_record.md b/devlog/_plan/260817_windows_stability_program/003_audit_record.md new file mode 100644 index 0000000000..7c653b2d5a --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/003_audit_record.md @@ -0,0 +1,98 @@ +# 003 — Audit record + +Seven review rounds over this unit, two independent reviewers. Recorded because +the corrections are more instructive than the plan, and because a unit that +claims "every finding was verified" should show what verification cost. + +## Rounds + +| Round | Reviewer | Verdict | Findings | +|---|---|---|---| +| r1 | A | FAIL | 6 blockers, 4 citation defects | +| r2 | A | FAIL | 5 blockers | +| r3 | A | NEAR-PASS | 2 | +| r4 | A | (inconclusive) | verdict lost — reviewer closed before the hook recorded it | +| r5 | B (fresh) | FAIL | 3 blockers, 3 citation corrections | +| r6 | B | NEAR-PASS | 1 citation defect | +| r7 | B | PASS | none | + +Reviewer B was dispatched with no prior context and explicitly told not to +assume reviewer A had been thorough. It found three blockers A had passed over, +including one that would have shipped a false claim about a security control. + +## Corrections worth remembering + +**A verifier that could not verify.** `031` claimed `privacy:scan` enforced the +fixed-literal publisher label. It does not — `scripts/privacy-scan.ts:187` is a +textual scanner over file content and cannot see that a runtime value was +path-derived. The fix was a closed union type so the constraint fails +`typecheck` instead. This is the most valuable catch in the seven rounds: the +plan named a guard that would have passed while the invariant it claimed to +protect was violated. + +**A CI assertion nobody could implement.** `031` also said CI would assert the +counters stayed zero across the Windows suite. The counters are process-local +and the suite runs across four sharded runners in many short-lived processes. +The claim was withdrawn rather than reworded — an instruction that cannot be +followed is worse than an admitted gap. + +**A test verifying the wrong thing.** `051` claimed it could verify `050`'s +backoff by reverting `050`. Reverting would leave a fixed five-second loop that +still relaunches, still yields a new PID, still restores health — the test would +pass either way. Now stated plainly, with backoff verified separately by +asserting on generated script text. + +**Batch arithmetic that fails at runtime.** `050` advised converting `%TIME%` +with `set /a`. `set /a` reads a leading zero as octal, so `08` and `09` are hard +errors — confirmed directly: + +```text +C:\> set /a a=08 +Invalid number. Numeric constants are either decimal (17), +hexadecimal (0x11), or octal (021). +``` + +Four traps documented in the end: octal, space padding, midnight wrap, delayed +expansion. + +**A job that did not test what it claimed.** `080`'s "self-update end to end" +used a locally packed tarball, but `ocx update` resolves its target from the +registry (`src/update/index.ts:167`) and installs a resolved version (`:106`). +There is no injection seam, so the real command was never exercised. Renamed to +a package replacement smoke, which is still worth having. + +**A gate that does not exist.** `060` promised Windows would block merges. `dev` +has no branch protection (`MAINTAINERS.md:121`, `:125`). Stage 3 is now a +convention gate; stage 4 is the real one because `release.yml` reads run +conclusions directly. + +**Sequencing invented after the fact.** `002` originally claimed a long +dependency chain. Only two links were structural. One was backwards. + +**Six citation defects.** `job.ts:1381`→`:1383`, `ci.yml:771`→`:769-772`, +`release.yml:224-234`→`:224-241`, `service.ts:2330`→`:2340-2341`/`:2350-2355`, +`config.ts:3937`→`:3942`, and a missing `chmodSync` site the `040` seed list had +skipped entirely — which is why `040` now says to re-derive the list rather than +trust it. + +## Two claims withdrawn + +"Every release to date ran zero Windows tests" was false. Releases touching +`src/service.ts` and a few other paths separately require a green +`service-lifecycle.yml` (`release.yml:224-241`), which includes a Windows job. +The defensible claim is narrower: the release preflight does not require the +Windows *suite*. + +`src/service.ts:1983` was cited as evidence that ACLs are authoritative for +credential writers. It says so about an elevation staging directory. Evidence +for the principle, not for any writer's coverage. + +## What this says about the unit + +Sixteen findings against a document that had already been written carefully. +Every one was reproduced against the tree before being acted on, and two of the +reviewer's own line numbers were off in the other direction and corrected back. + +The rate at which confident-sounding planning prose turns out to be wrong is the +argument for `060`. A plan gets seven adversarial rounds; a merge to `dev` +currently gets no Windows execution at all. From 5ca4ffe2745d020c13cdae71a4987187b69860ec Mon Sep 17 00:00:00 2001 From: lidge-jun <243035832+lidge-jun@users.noreply.github.com> Date: Tue, 18 Aug 2026 10:32:26 +0900 Subject: [PATCH 9/9] devlog: record what the implementation review changed Six blockers across three rounds, all verified before acting. The two worth remembering: the counters first collapsed three error codes into one number, and the wrapper tests asserted the generated script contained IndexOf/before/after - which a broken substring matcher would also satisfy. Also records that 030's instruction to sweep for remaining renameSync calls read as complete and was not: six more publishers were left behind, two of them found only in the second review round. --- .../004_implementation_outcome.md | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 devlog/_plan/260817_windows_stability_program/004_implementation_outcome.md diff --git a/devlog/_plan/260817_windows_stability_program/004_implementation_outcome.md b/devlog/_plan/260817_windows_stability_program/004_implementation_outcome.md new file mode 100644 index 0000000000..23690231eb --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/004_implementation_outcome.md @@ -0,0 +1,83 @@ +# 004 — Implementation outcome, phases 010 / 020 / 030 / 031 + +Shipped as a stacked chain against `dev` on 2026-08-18. This records what +landed, what the code review changed, and what the plan got wrong. + +## The stack + +| PR | Phase | Base | Commit | +|---|---|---|---| +| [#1949](https://github.com/lidge-jun/opencodex/pull/1949) | this unit | `dev` | `f9cb0fcd4` | +| [#1944](https://github.com/lidge-jun/opencodex/pull/1944) | 010 | `dev` | `393d72a77` | +| [#1945](https://github.com/lidge-jun/opencodex/pull/1945) | 020 | #1944 | `a3169db77` | +| [#1946](https://github.com/lidge-jun/opencodex/pull/1946) | 030 | #1945 | `c5c6644d7` | +| [#1947](https://github.com/lidge-jun/opencodex/pull/1947) | 031 | #1946 | `fcc9e5022` | + +Each guard was driven red before its fix. 010's sweep reported +`["service.ts"]`; 020's no-private-matcher assertion failed for both files. + +## What the code review changed + +An independent reviewer took three rounds and found six blockers. Every one was +verified against the tree before acting, and every one was real. + +**The counters lost the error code.** The first implementation keyed them by +publisher alone, so EBUSY from a scanner, EACCES from a permissions problem and +EPERM from a lock collapsed into one number. That defeats the reason the +counters exist. Now keyed `publisher:CODE`. + +**Phase 031 leaked into phase 030.** The extracted module arrived carrying +`ReplacePublisher`, the counters and the read/reset API — telemetry behavior in +the PR that was supposed to be a pure move, and without its tests. Stripped back +out; 030 is now the loop and nothing else. + +**The wrapper tests proved nothing.** They asserted the generated PowerShell +*contained* `IndexOf`, `before` and `after`. A broken substring matcher would +keep all three tokens and pass. Rewritten to port the rule to JS and run real +command lines through it — this home's wrapper, another home's path, a longer +path ending with ours, an unrelated process naming the file — with a separate +test pinning the port to the shipped script so it cannot silently diverge. The +old `-like` rule kills all three negative cases; the token rule kills none. + +**The sweep was half done.** `030` said to convert every durable publisher and +converted two. Six more were left: `claude/agents-inject.ts`, both Lab +automation writers, `lab/ledger/purge.ts`, and — found only in the second round +— `storage/cleanup.ts` and `tray/windows.ts`. All eight now use the helper. The +three remaining `renameSync` calls in `storage/cleanup.ts` are directory +relocations, a different problem, and the commit says so. + +**One publisher was mislabelled.** `storage/cleanup.ts` called the helper +without a label, so its retries would have been reported as `config`. Caught +only because the reviewer read the default argument rather than the call site. + +## What the plan got wrong + +`031` claimed `privacy:scan` would enforce the fixed-literal publisher label. +The plan audit had already corrected that once — the scanner reads file text and +cannot see a runtime value — and the closed union is what actually enforces it. +Worth noting that the same claim had to be caught twice, in the plan and again +in the code. + +`030`'s instruction to "sweep `src/` for remaining `renameSync` calls" read as +complete and was not. A phase that says "sweep" should name the expected count +or the command that produces it, or the sweep silently becomes whatever the +implementer happened to notice. + +## Verification + +- `bun run typecheck` clean at every commit +- `bun run privacy:scan` passed +- Full suite in 60-file batches over 809 files: 3 residual failures, all + pre-existing or contention-only. `codex-app-server-processes` memo case + reproduces on clean `origin/dev`; `command-code-provider` and + `issue-452-empty-503` pass in isolation. `native-codex-toggle` panics Bun + 1.3.14 at teardown after all four of its tests pass, also on clean `dev`. +- CI: #1944 and #1949 fully green; the stacked children green apart from slow + macos legs still running at time of writing. + +## Not done + +Phases `040`, `050`, `051`, `060`, `070`, `080` remain open. `050` needs its +implementation shape decided (state file vs delayed expansion) and the CI phases +need the runner and gating decisions `060` names. Nothing here changes the +central point in `000`: Windows still does not gate a merge or a release.