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..c9d3e2b7b3 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/001_verified_findings.md @@ -0,0 +1,205 @@ +# 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: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 1383) +"$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. + +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` +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 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 +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-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. + +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 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. + +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` 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, +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..8b93c997a5 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/002_sequencing.md @@ -0,0 +1,94 @@ +# 002 — Sequencing and what this unit deliberately does not do + +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 LR + A["030 shared replace primitive"] --> B["031 retry telemetry"] + C["060 stage 1 - run non-gating"] --> D["070 flakiness policy"] +``` + +`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. 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. +- **040** — independent inventory, produces a document. + +## 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** 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 + +**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). 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. + +**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, 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 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/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. 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. 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..4e21eb7873 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/010_windowstyle_argv.md @@ -0,0 +1,50 @@ +# 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 run typecheck +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..b250682256 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/020_wrapper_killer_dedupe.md @@ -0,0 +1,50 @@ +# 020 — Collapse the duplicated scheduler-wrapper killer (F2) + +**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 + +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 run typecheck +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..7f80a5fa6b --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/030_shared_replace_retry.md @@ -0,0 +1,51 @@ +# 030 — Make the Windows replace-with-retry a shared primitive (F4) + +**Depends on:** nothing structural. Sequence after 020 only to keep two people +out of the same files at once. + +## Change + +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. + +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. + +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.** 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 run typecheck +bun run test +``` + +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. 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 new file mode 100644 index 0000000000..8fa324a608 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/031_retry_telemetry.md @@ -0,0 +1,91 @@ +# 031 — Instrument the retry envelope before widening it (F4) + +**Depends on:** 030. This is a genuine dependency: there is nothing to count +until the primitive exists. + +## Change + +Count, do not change behavior. + +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`. + +Export `readWindowsReplaceRetryCounters()` returning a plain snapshot object. + +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: `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 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 + +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: **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. + +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 + +```powershell +bun run typecheck +bun run privacy:scan +bun test tests/config.test.ts +bun test tests/system-routes.test.ts +``` + +## Risk + +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 new file mode 100644 index 0000000000..24b1d2b9d7 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/040_credential_acl_inventory.md @@ -0,0 +1,57 @@ +# 040 — Inventory every credential writer's Windows ACL coverage (F5) + +**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 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 **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`. + +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 3942 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 +`chmod` has no protection there at all. + +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. + +## Where the output goes + +**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 + +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 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 new file mode 100644 index 0000000000..4a7a6313fe --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/050_wrapper_backoff.md @@ -0,0 +1,100 @@ +# 050 — Bounded backoff for the service wrapper restart loop (F6) + +**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 + +`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 **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 +"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, and it must stay dependency-free — no +PowerShell inside the wrapper. + +The timing arithmetic has four separate traps, and all of them bite. + +**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, 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 + +```powershell +bun run typecheck +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..4221e9c4fe --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/051_crash_restart_ci.md @@ -0,0 +1,46 @@ +# 051 — Windows crash-restart coverage in service CI (F7) + +**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` 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`. + +## 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 + +```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 — 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 new file mode 100644 index 0000000000..4fd6fbd828 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/060_windows_ci_gate.md @@ -0,0 +1,92 @@ +# 060 — Stage Windows back into CI (F3) + +**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 + +`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. + +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: + +- 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 below is therefore a convention gate. Stage 4 is a real one. + +## 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.** 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. + +## 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 +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. + +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 + +```powershell +bun run prepush +gh workflow run ci.yml --ref +``` + +`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 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 new file mode 100644 index 0000000000..99cfb0358e --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/070_flakiness_policy.md @@ -0,0 +1,42 @@ +# 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 + +```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 + +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..9e79c00d06 --- /dev/null +++ b/devlog/_plan/260817_windows_stability_program/080_environment_smoke.md @@ -0,0 +1,97 @@ +# 080 — Windows environment smoke coverage (F3) + +**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 + +The unit suite tests logic. These test the environment, and no amount of unit +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. Package replacement smoke (not `ocx update`) + +`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 + +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 + +```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, 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.