fix(update): recover Windows dashboard proxy after self-update - #838
fix(update): recover Windows dashboard proxy after self-update#838Wibias wants to merge 31 commits into
Conversation
…vice Gate post-update restart on service viability, not mere install presence, and fall through to a direct ocx start when reinstall leaves a non-viable manager so /healthz comes back after browser-triggered updates.
The pre-update PID alone left respawned bun/node holders protected, so the direct-start fallback could not bind :10100 after a non-elevated service reinstall.
…reclaim schtasks /end leaves the hidden cmd/wscript :loop alive, which mints a new bun PID mid-reclaim. Kill those wrappers and reclaim with killAllOcxOnPort so dashboard update recovery can bind :10100 after a non-elevated service reinstall.
…down Windows can keep a dead pre-update LISTEN owner listed after the cmdline probe fails; treating it as foreign blocked SetTcpEntry and left :10100 unbindable.
During npm install -g Windows can respawn from @bitkyc08/.opencodex-* which failed verifyPidIdentity and blocked port reclaim as a foreign holder.
isPortAvailable treated any listen error as busy, so a mid-npm Bun glitch blocked restart for 30s. Prefer EADDRINUSE only, resolve the live launcher path after npm rename, and pinned-start when reclaim times out with no live LISTEN owners.
After reclaim timeout with only ghost holders, poll a fresh Node listen (and drop Windows TCP rows) so the detached start is not raced against a still-busy :10100.
📝 WalkthroughWalkthroughThe PR hardens service viability checks, port reclamation, update restart recovery, Windows worker spawning, health probing, cross-platform worker teardown, and related regression tests. ChangesService and restart recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The real Windows reproduction and the recovery direction are valuable. I also ran the focused update/reclaim/service suite locally on head 7fa29759: 130 passed, and typecheck is clean. Two correctness/safety blockers prevent approval:
-
restartAfterUpdate()unconditionally setskillAnyListenPidOnPort: true(src/update/job.ts), andreclaimListenPort()then callskillProxy(pid)for every live non-OCX PID returned for that port. The new test explicitly expects a foreign listener to be killed. Capturing a port before update does not confer ownership of every process that may bind it afterward; an unrelated process can claim the port during the stop/update window, and an OCX identity probe can also fail transiently. Unknown live holders must remain fail-closed. Restrict termination to the captured old PID and strongly revalidated OCX/update descendants (for example, verified executable/package-rename path plus lineage/start-time evidence); otherwise report the holder and refuse to start. Remove the blanket foreign-PID kill test and replace it with a regression proving a foreign claimant survives. -
isPortAvailable()now resolvestruefor every listen error exceptEADDRINUSE. Errors such asEACCES,EPERM,EADDRNOTAVAIL, or an invalid bind address mean the requested bind is not available; reporting true makesfindAvailablePort()return a port the proxy still cannot bind. Keep unknown/non-transient errors unavailable/fail-closed, or recognize only the exact evidenced npm/Bun transient condition in a separate bounded retry path. Add regressions for EACCES and EADDRNOTAVAIL so neither is reported available.
The current macOS red check is unrelated to this diff: Bun 1.3.14 segfaulted after the storage-worker teardown tests with exit 133, after the assertions shown in the log had passed. That job can be rerun once the still-running workflow finishes, but it does not waive the two code blockers above. Please keep this unmerged until both are fixed and the Windows real-update walk is repeated with the narrower reclaim policy.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fa29759e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| killAllOcxOnPort: true, | ||
| // npm's in-place rename leaves respawns under `@bitkyc08/.opencodex-*` that | ||
| // fail verifyPidIdentity — still our port for post-update recovery. | ||
| killAnyListenPidOnPort: true, |
There was a problem hiding this comment.
During any post-update restart, if another application acquires the captured port after the proxy stops, enabling killAnyListenPidOnPort causes reclaimListenPort to call killProxy even when verifyPidIdentity classifies that listener as foreign. This can terminate an unrelated user process merely because it won the port race; keep this option disabled and extend the OpenCodex identity check to recognize the npm rename path instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on aa660dc0.
Removed killAnyListenPidOnPort entirely. Post-update reclaim now only terminates allowlisted teardown PIDs and revalidated ocx listeners (including @bitkyc08/.opencodex-* via isOcxStartCommandLine). Foreign claimants fail closed; regression added in tests/port-reclaim.test.ts.
| if (process.env.OCX_SERVICE === "1") { | ||
| updateJob(job, {}, "Skipping service reinstall from the non-elevated update worker; falling back to a direct proxy start."); | ||
| skipServiceInstall = true; |
There was a problem hiding this comment.
For dashboard workers on Linux and macOS, spawnGuiUpdateWorker also sets OCX_SERVICE=1, so this unconditional branch skips systemd/launchd reinstall whenever the first post-update health check reaches the explicit restart path. The fallback then starts an unmanaged process while the existing supervisor may continue respawning children, leaving the service conflicted and losing restart-on-login/reboot behavior; gate this shortcut on process.platform === "win32" and retain the normal service refresh elsewhere.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on aa660dc0.
Service-reinstall skip is now gated on process.platform === ""win32"" && process.env.OCX_SERVICE === ""1"", so Linux/macOS supervisors still refresh normally.
| server.once("error", (err) => { | ||
| // Only a real address-in-use conflict means busy. Other listen failures | ||
| // (runtime glitches while npm replaces the running Bun binary mid-update) | ||
| // must not look like a stuck port — reclaim would wait out the full timeout | ||
| // and never spawn the post-update start. | ||
| resolve(!isAddrInUse(err)); |
There was a problem hiding this comment.
When the configured hostname cannot be bound, such as an address not assigned to the machine (EADDRNOTAVAIL) or a permission-denied endpoint, this now resolves true and findAvailablePort selects an endpoint that the real server cannot open. For example, isPortAvailable(54321, "192.0.2.1") currently returns true on a host without that address; availability should only be reported from the listening event, while every error remains unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on aa660dc0.
isPortAvailable is fail-closed again: only the listening event reports available; EACCES / EADDRNOTAVAIL / other listen errors return false. Regression covers TEST-NET-1 bind failure.
| */ | ||
| async function strictRuntimePortAvailable(port: number, hostname = "127.0.0.1"): Promise<boolean> { | ||
| const script = strictBindProbeScript(port, hostname); | ||
| if (!spawnBindProbe(nodeBin(), script)) return false; |
There was a problem hiding this comment.
For a Bun-installed copy on a machine without a separate node executable, this mandatory first probe always fails before the bundled/live Bun probe is attempted. Every production pinned-start attempt therefore reports the port as unbindable and the dashboard update finishes with a failed restart even though Bun could bind it; probe the runtime that will actually execute ocx start, and make the Node comparison optional.
AGENTS.md reference: src/AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on aa660dc0.
strictRuntimePortAvailable probes live package Bun (else worker process.execPath) only — no mandatory separate node binary.
| const argList = args.map(psQuote).join(", "); | ||
| const ps = [ | ||
| `$env:OCX_SERVICE = '1'`, | ||
| `$p = Start-Process -FilePath ${psQuote(process.execPath)} -ArgumentList @(${argList}) -WindowStyle Hidden -PassThru`, |
There was a problem hiding this comment.
When process.argv[1] is under a Windows profile or install directory containing spaces, the PowerShell literal quotes are consumed before Start-Process constructs the child command line, so Bun receives the script path as multiple arguments and the update worker never runs. The Start-Process -ArgumentList documentation notes that array elements are joined with spaces and outer quotes are not retained; pass one correctly escaped argument string containing literal quotes, or use a launch mechanism that preserves the argument vector.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on aa660dc0.
Windows worker launch now uses buildWindowsElevatedArgumentList as a single -ArgumentList string plus resolveTrustedWindowsPowerShellExe(), so paths with spaces survive Start-Process.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/update/job.ts (1)
972-1003: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA fully dead proxy can now pass the stability window and be reported healthy.
Two changes in this block interact badly. Line 975 raises the per-probe cost by using
timeoutMs: 2_000, attempts: 3, and lines 994-1003 require three consecutive misses before declaring a flap. The stability window on line 995 is bounded by wall clock only, so the miss counter may never reach the limit.Cost of one failing probe with the new settings, from
proxyIdentityAtinsrc/server/proxy-liveness.tslines 104-125: three attempts at a 2s timeout plus two 100ms back-off sleeps, about 6.2s. Each stability iteration then costs roughly 6.2s plus the 500ms sleep on line 1005, about 6.7s.Trace a proxy that is down for the entire window, with
RESTART_STABILITY_WINDOW_MSat 15s:
- t≈6.7s: miss 1, loop continues.
- t≈13.4s: miss 2, loop continues.
- t≈20.1s:
now() >= stableUntil, the loop exits withmisses === 2.
missesnever reachesstabilityMissLimit, so line 1007 logs "Proxy stayed healthy" and line 1008 returns{ ok: true }.confirmRestartedProxyandconfirmNpmExplicitRestartthen mark the update job as a successful restart while/healthzis dead. Before this change a single miss failed the window, so this outcome was not reachable.Unit tests cannot catch it. They inject
sleepMsandnow, so the simulated clock advances only by the sleep amounts and never by the real probe duration.Make the verdict depend on the last probe result rather than only on reaching a consecutive-miss count:
🐛 Proposed fix: fail the window when it ends on a failed probe
const stableUntil = now() + RESTART_STABILITY_WINDOW_MS; let misses = 0; while (now() < stableUntil) { if (await probe(port, hostname)) { misses = 0; } else { misses += 1; if (misses >= stabilityMissLimit) { updateJob(job, {}, `Proxy became unhealthy on ${hostname}:${port} during the stability window.`); return { ok: false, reason: "flapped" }; } } await sleep(500); } + // The window can end before `stabilityMissLimit` is reached: one failing probe + // costs ~6s (2s timeout x 3 attempts), so a dead proxy may only register two + // misses inside a 15s window. Trust the window only when the last probe passed. + if (misses > 0) { + updateJob(job, {}, `Proxy was still unhealthy on ${hostname}:${port} when the stability window ended.`); + return { ok: false, reason: "flapped" }; + } updateJob(job, {}, `Proxy stayed healthy for ${Math.trunc(RESTART_STABILITY_WINDOW_MS / 1000)}s after restart.`);Add a regression test in
tests/update-job.test.tsthat injects aprobeProxywhich succeeds once and then always fails, and asleepMsthat advances the clock by the real probe cost, then assertsreason: "flapped".Run the following script to confirm the window constant and the resulting arithmetic:
#!/bin/bash # Description: Read the restart health/stability constants used by awaitRestartedProxyHealthy. set -uo pipefail echo "=== restart timing constants ===" rg -n --type=ts 'RESTART_STABILITY_WINDOW_MS|RESTART_HEALTH_TIMEOUT_MS|RESTART_PORT_RECLAIM_MS|RESTART_TIMEOUT_MS' src/ echo "=== proxyIdentityAt attempt/timeout handling ===" sed -n '90,130p' src/server/proxy-liveness.ts echo "=== stability-window tests ===" rg -n --type=ts -g 'tests/**' -C6 'flapped|stability window' || echo "none"As per path instructions for
tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/update/job.ts` around lines 972 - 1003, Update the stability-window logic in awaitRestartedProxyHealthy so a window ending after a failed final probe returns { ok: false, reason: "flapped" } instead of reporting success; preserve the consecutive-miss threshold while also evaluating the last probe result when the wall-clock window expires. Add a focused test in the existing update-job tests using probeProxy that succeeds once then fails, and sleepMs/clock behavior that accounts for probe duration, asserting the flapped reason.Source: Path instructions
src/server/port-reclaim.ts (1)
228-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnreachable condition in the non-OCX kill path.
At line 247,
else if (!killAnyListen && !allowlisted) foreignLive = true;sits inside the block guarded byif (mayKill && (allowlisted || killAnyListen))(line 237). That guard already guaranteesallowlisted || killAnyListenis true, so!killAnyListen && !allowlistedcan never be true. This branch never executes.Cross-checking against
tests/port-reclaim.test.ts(lines 498-524, "allowlisted PID that fails ocx verify still gets killed and does not block TCP drop") confirms this: that test hits exactly this code path (non-OCX, allowlisted, live after kill) and the test only passes becauseforeignLivestaysfalse— i.e., the dead branch was never expected to fire.Leaving this in place gives future readers false confidence that a "still-foreign-after-kill" safety net exists here, when it does not. Either remove the dead branch, or fix the condition to express the actual intended guard (e.g., if the intent was to protect against a genuinely-foreign PID that only slipped in via
killAnyListen, that check needs to live outside thisif, evaluated before the best-effort kill is attempted).🧹 Proposed fix: remove the dead branch
if (!killed.has(pid)) { try { killFn(pid); killed.add(pid); } catch { /* kill failed — still allow TCP drop for this trusted port/PID */ } } - if (!isAliveFn(pid)) killed.delete(pid); - else if (!killAnyListen && !allowlisted) foreignLive = true; + if (!isAliveFn(pid)) killed.delete(pid); continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/port-reclaim.ts` around lines 228 - 252, Remove the unreachable `else if (!killAnyListen && !allowlisted) foreignLive = true` branch from the non-OCX handling in the PID scan loop. Keep the existing best-effort kill, cleanup, TCP-drop behavior, and `foreignLive` handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/update/job.ts`:
- Around line 777-781: Wrap the killProxy(pid) call in the PID-handling block of
restartAfterUpdate with the same best-effort try/catch pattern used by the other
kill sites in this file, allowing the recovery flow to continue when termination
throws while preserving the existing updateJob call and PID guard.
- Around line 405-428: Update the Windows worker launch logic around psQuote and
Start-Process so arguments retain their individual boundaries when paths contain
spaces, especially args[0] (process.argv[1]); use correctly escaped native
command-line construction or an argument-passing mechanism that preserves
elements. Ensure startup failures remain observable where possible, and add a
Windows regression test covering a script path with spaces.
In `@tests/port-reclaim.test.ts`:
- Around line 302-330: Update the inline comments in the test around verifyOcxFn
and the subsequent scans to describe the actual mechanism: after the initial OCX
verification, later scans return null, reclassifying the allowlisted PID as
non-OCX and allowing the best-effort kill path to terminate it. Do not attribute
the kill to OCX revalidation tolerating the allowlist.
In `@tests/update-job.test.ts`:
- Around line 375-389: Clear the ambient OCX_SERVICE environment variable in a
beforeEach covering the relevant restartAfterUpdate service-branch tests,
including the cases around lines 310, 348, and 375. Preserve the existing
assertions so these tests consistently exercise service viability and direct
proxy fallback; optionally add a focused OCX_SERVICE=1 test only if coverage for
the skip branch is required.
---
Outside diff comments:
In `@src/server/port-reclaim.ts`:
- Around line 228-252: Remove the unreachable `else if (!killAnyListen &&
!allowlisted) foreignLive = true` branch from the non-OCX handling in the PID
scan loop. Keep the existing best-effort kill, cleanup, TCP-drop behavior, and
`foreignLive` handling unchanged.
In `@src/update/job.ts`:
- Around line 972-1003: Update the stability-window logic in
awaitRestartedProxyHealthy so a window ending after a failed final probe returns
{ ok: false, reason: "flapped" } instead of reporting success; preserve the
consecutive-miss threshold while also evaluating the last probe result when the
wall-clock window expires. Add a focused test in the existing update-job tests
using probeProxy that succeeds once then fails, and sleepMs/clock behavior that
accounts for probe duration, asserting the flapped reason.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d8612a2-5e8d-4597-bea2-f1987df471e3
📒 Files selected for processing (16)
bin/ocx.mjssrc/cli/index.tssrc/config.tssrc/server/management/system-restart.tssrc/server/port-reclaim.tssrc/server/ports.tssrc/server/windows-tcp-drop.tssrc/service.tssrc/update/index.tssrc/update/job.tstests/config.test.tstests/port-reclaim.test.tstests/service.test.tstests/system-restart.test.tstests/update-job.test.tstests/windows-deploy-close-regressions.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fa29759e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (process.platform === "win32") { | ||
| try { stopWindows(); } catch { /* already stopped */ } | ||
| killWindowsServiceWrapperProcesses(); |
There was a problem hiding this comment.
When the installed backend is native WinSW and the explicit restart path is reached—for example after a transient post-update health miss—this calls the Task Scheduler-only stopWindows(). The native supervisor remains active and can continually respawn the listener while reclaim/direct-start runs; src/service.ts:1675-1677 already maps the native backend to stopWinswService. Detect the installed backend or use the existing backend-aware service stop path before reclaiming the port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on 751dd8e4.
stopWindowsServiceWrappersBestEffort now routes native WinSW installs through stopWinswService() via readServiceBackend(); the scheduler path still uses stopWindows() plus wrapper kill.
| const listPids = io.listListenPidsFn ?? listListenPids; | ||
| const aliveFn = io.isAliveFn ?? isProcessAlive; | ||
| const liveListeners = listPids(captured.port) | ||
| .filter(pid => pid !== process.pid && aliveFn(pid)); | ||
| if (liveListeners.length === 0) { | ||
| updateJob(job, {}, "npm self-update did not leave a live listener; performing explicit restart..."); |
There was a problem hiding this comment.
On Linux or macOS installations without lsof/netstat, listListenPids() deliberately converts a failed scan into [] (src/server/port-reclaim.ts:169-172), so this branch incorrectly concludes that a successfully restarted service has no live listener and enters the destructive explicit-restart path without probing /healthz. Preserve the scanListenPids() success flag, or fall back to the health probe when process discovery is inconclusive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on 751dd8e4.
finishGuiUpdateRestart now uses scanListenPids (ok/fail). A failed scan falls through to /healthz instead of treating the result as no listeners. Added regression coverage in tests/update-job.test.ts.
| "$pats = @('opencodex-service.cmd','opencodex-service-launcher.vbs');", | ||
| "Get-CimInstance Win32_Process | Where-Object {", | ||
| " $c = $_.CommandLine; if (-not $c) { return $false };", | ||
| " foreach ($p in $pats) { if ($c -like ('*' + $p + '*')) { return $true } };", |
There was a problem hiding this comment.
On Windows, the spawned PowerShell process's command line contains both wrapper basenames because they are literals in its -Command argument, so this predicate selects the cleanup process itself and Stop-Process can terminate the pipeline before all wrapper processes are handled. Exclude $PID (and preferably match the exact configured wrapper paths) before applying the basename checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on 751dd8e4.
Wrapper cleanup PowerShell excludes 39520 before basename matches and launches via resolveTrustedWindowsPowerShellExe().
| lastChild = spawnDetachedStart(job, job.installer, port); | ||
| const healthDeadline = Date.now() + perAttemptHealthMs; | ||
| while (Date.now() < healthDeadline) { | ||
| if (await probe(port, hostname)) return; | ||
| await sleep(500); |
There was a problem hiding this comment.
If the third pinned-start child remains alive but never serves /healthz—for example because startup hangs after spawning—the loop ends without terminating lastChild. The subsequent confirmation marks the update failed, but the hung process and its PID/port state remain, so a manual retry or later update can be blocked by the process this job left behind. Kill the final child after exhausting all attempts, just as the retry path already does for earlier attempts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on 751dd8e4.
After three pinned-start attempts, a still-alive lastChild is best-effort killed so a hung start cannot block the next retry.
Drop blanket foreign-listener kills, restore fail-closed isPortAvailable, and gate Windows TCP drops / service-skip / pinned-start prep so recovery cannot terminate or ResetTcpEntry against unknown live holders.
Document killAllOcxOnPort in the port-reclaim module header and share one Windows service-wrapper teardown helper across restart paths.
Stop native WinSW via stopWinswService, keep listener-scan failures from skipping /healthz, exclude the cleanup PowerShell PID from wrapper kills, and terminate a hung final pinned-start child.
The streaming /v1/messages end-to-end case binds a real server and was timing out at Bun's 5s default under windows-latest contention.
Bring OS-join settle and capped isolate churn from the isolate-worker fix line, and extend the settle/churn caps to Linux after ubuntu-latest exit 132.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/update-job.test.ts (1)
140-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
killAllOcxOnPortcast so the compiler checks the option name.Lines 148 and 188 read the flag through
(opts as { killAllOcxOnPort?: boolean } | undefined)?.killAllOcxOnPort. That cast is no longer needed:waitForPortis typed astypeof reclaimListenPort(src/update/job.ts line 624), andReclaimListenPortOptionsnow declareskillAllOcxOnPorton src/server/port-reclaim.ts line 41, soopts?.killAllOcxOnPorttype-checks directly.The cast is not merely redundant. It suppresses exactly the check these tests exist to provide. If the option is renamed in
src/server/port-reclaim.ts, the cast keeps this file compiling, the capture silently recordsundefined, and the assertions on lines 160 and 194 fail with an opaque value diff instead of a type error at the rename site.♻️ Proposed change
opts: { killOcxHolders: opts?.killOcxHolders, onlyKillPids: opts?.onlyKillPids, - killAllOcxOnPort: (opts as { killAllOcxOnPort?: boolean } | undefined)?.killAllOcxOnPort, + killAllOcxOnPort: opts?.killAllOcxOnPort, },Apply the same removal on line 188.
Also applies to: 183-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/update-job.test.ts` around lines 140 - 152, Remove the unnecessary inline type cast from both option captures in the waitForPort stubs, using opts?.killAllOcxOnPort directly. Update both the blocks around the waited.push calls so waitForPort’s typeof reclaimListenPort typing validates the option name at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/port-reclaim.ts`:
- Around line 5-8: Update the module header comment near the port-reclaim
authorization description to acknowledge the allowlisted teardown exception:
when `killOcxHolders` is enabled and a PID is included in the non-empty
`onlyKillPids` allowlist, it may be killed even if `verifyOcxFn` does not
identify it as an ocx process. Keep the existing restrictions for default
behavior and `killAllOcxOnPort` intact.
In `@src/update/job.ts`:
- Around line 911-914: Update src/update/job.ts lines 911-914 in the
health-probe loop to use injected io.now when computing healthDeadline and
checking the loop condition. Update waitForGhostListenClear at src/update/job.ts
lines 179-205 to accept a now parameter defaulting to Date.now, use it for the
deadline and condition, and pass io.now from both call sites at lines 832 and
893.
In `@tests/config.test.ts`:
- Around line 1416-1419: Add a focused test near the existing
isOcxStartCommandLine cases using a command line that reaches the start port
through the `@bitkyc08/.opencodex-`* rename tree without containing
src/cli/index.ts or another recognized entrypoint marker. Ensure the assertion
exercises the new rename-tree alternative rather than an earlier matching
branch, preserving coverage for the authorization path used by verifyPidIdentity
and reclaimListenPort.
In `@tests/port-reclaim.test.ts`:
- Around line 473-498: Add a focused test alongside the existing
killAllOcxOnPort coverage that sets onlyKillPids to an empty array or omits it
while keeping killAllOcxOnPort enabled, then verifies the revalidated OCX holder
is killed and reclaimListenPort succeeds. Ensure the scenario exercises the
allowedKillPids.size === 0 && killAllOcx branch rather than the non-empty
allowlist path.
In `@tests/storage-worker-teardown-isolate.test.ts`:
- Around line 78-81: Update the teardown hook containing drainStorageWorkers()
so OPENCODEX_HOME restoration, isolatedCodexHome reset, and testDir removal
always run in a finally block. Preserve the existing non-Windows Bun.sleep
behavior, and rethrow any drainStorageWorkers() failure after cleanup so the
original error remains visible.
- Around line 43-48: Add a Darwin-safe lifecycle test near the existing storage
worker teardown tests that registers a worker via registerStorageWorker(), uses
a fake worker whose terminate() invokes the captured close listener, and asserts
concurrent termination or drain followed by liveStorageWorkerCount() === 0.
Apply skipDarwinWorkerSpawn only to tests that instantiate real workers, leaving
this fake-worker coverage runnable on Darwin.
In `@tests/update-job.test.ts`:
- Around line 344-401: Add a focused regression test near the existing
restart-after-update tests covering OCX_SERVICE=1 on Windows. Set the
environment variable, invoke restartAfterUpdateForTests with service callbacks,
and when process.platform is "win32" assert the skip log is emitted, runService
is not called, and direct pinned start occurs; restore the environment in
finally. Skip or gate the Windows-specific assertions on non-Windows platforms.
In `@tests/windows-deploy-close-regressions.test.ts`:
- Around line 32-50: Add a focused behavioral regression test in
tests/update-job.test.ts for restartAfterUpdateForTests that injects isAliveFn,
exhausts the pinned-start retries, and asserts the surviving child PID is
terminated. Keep the existing source-text assertions in
tests/windows-deploy-close-regressions.test.ts only as optional tripwires, but
do not rely on exact formatting or local names to validate preferRetryMs or
lastChild handling.
---
Outside diff comments:
In `@tests/update-job.test.ts`:
- Around line 140-152: Remove the unnecessary inline type cast from both option
captures in the waitForPort stubs, using opts?.killAllOcxOnPort directly. Update
both the blocks around the waited.push calls so waitForPort’s typeof
reclaimListenPort typing validates the option name at compile time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a4c432a-06d1-41b7-a829-95a1d80f2d1c
📒 Files selected for processing (13)
bin/ocx.mjssrc/config.tssrc/server/port-reclaim.tssrc/server/ports.tssrc/storage/worker-lifecycle.tssrc/update/job.tstests/claude-messages-endpoint.test.tstests/config.test.tstests/port-reclaim.test.tstests/ports.test.tstests/storage-worker-teardown-isolate.test.tstests/update-job.test.tstests/windows-deploy-close-regressions.test.ts
| * Killing is never the default. A process may be killed only when the caller | ||
| * sets `killOcxHolders` and either supplies a non-empty `onlyKillPids` allowlist | ||
| * or enables `killAllOcxOnPort` for revalidated ocx listeners. Foreign (non-ocx) | ||
| * processes are never killed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten the header rule so it matches the new allowlisted-teardown kill path.
Line 8 states "Foreign (non-ocx) processes are never killed". Lines 231-245 now kill a PID that fails verifyOcxFn when mayKill is true and the PID is in onlyKillPids. The inline comment on lines 228-230 and the option doc on lines 38-39 both describe that carve-out correctly, so only the module header is now imprecise. A reader who audits kill authorization from the header alone will draw the wrong conclusion about onlyKillPids.
♻️ Proposed header correction
* Killing is never the default. A process may be killed only when the caller
* sets `killOcxHolders` and either supplies a non-empty `onlyKillPids` allowlist
* or enables `killAllOcxOnPort` for revalidated ocx listeners. Foreign (non-ocx)
- * processes are never killed.
+ * processes are never killed unless the caller explicitly allowlisted that PID
+ * in `onlyKillPids` (a teardown PID whose identity probe raced or whose owner is
+ * already dead). Unknown claimants are never killed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Killing is never the default. A process may be killed only when the caller | |
| * sets `killOcxHolders` and either supplies a non-empty `onlyKillPids` allowlist | |
| * or enables `killAllOcxOnPort` for revalidated ocx listeners. Foreign (non-ocx) | |
| * processes are never killed. | |
| * Killing is never the default. A process may be killed only when the caller | |
| * sets `killOcxHolders` and either supplies a non-empty `onlyKillPids` allowlist | |
| * or enables `killAllOcxOnPort` for revalidated ocx listeners. Foreign (non-ocx) | |
| * processes are never killed unless the caller explicitly allowlisted that PID | |
| * in `onlyKillPids` (a teardown PID whose identity probe raced or whose owner is | |
| * already dead). Unknown claimants are never killed. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/port-reclaim.ts` around lines 5 - 8, Update the module header
comment near the port-reclaim authorization description to acknowledge the
allowlisted teardown exception: when `killOcxHolders` is enabled and a PID is
included in the non-empty `onlyKillPids` allowlist, it may be killed even if
`verifyOcxFn` does not identify it as an ocx process. Keep the existing
restrictions for default behavior and `killAllOcxOnPort` intact.
| while (Date.now() < healthDeadline) { | ||
| if (await probe(port, hostname)) return; | ||
| await sleep(500); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Two new time-bounded loops accept a sleep seam but read the wall clock directly. The shared root cause is an incomplete seam: both helpers let a caller replace the sleep while Date.now() stays hard-coded, so a test that advances a fake clock through sleepMs can never move either deadline. awaitRestartedProxyHealthy already establishes the correct convention on lines 1017-1027 by reading time through io.now.
Neither loop is reachable from the current tests, because line 850 returns as soon as io.spawnStart is injected and every test in tests/update-job.test.ts injects it. That is what makes this a latent trap rather than a present bug: the first test to drive the real pinned-start path with now/sleepMs fakes will busy-loop against the real clock for the full window while the fake clock alone advances, and the failure will present as a hang.
src/update/job.ts#L911-L914: computehealthDeadlineand the loop condition from the injected clock. Threadio.nowinto this scope (it is already available onRestartIoline 639) and use it in place of bothDate.now()calls.src/update/job.ts#L179-L205: add anow: () => number = Date.nowparameter towaitForGhostListenClearalongside the existingsleepparameter, use it fordeadlineon line 187 and thewhilecondition on line 189, and passio.nowfrom both call sites (lines 832 and 893).
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 1 file
src/update/job.ts#L911-L914(this comment)src/update/job.ts#L179-L205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/update/job.ts` around lines 911 - 914, Update src/update/job.ts lines
911-914 in the health-probe loop to use injected io.now when computing
healthDeadline and checking the loop condition. Update waitForGhostListenClear
at src/update/job.ts lines 179-205 to accept a now parameter defaulting to
Date.now, use it for the deadline and condition, and pass io.now from both call
sites at lines 832 and 893.
| // npm's in-place rename during `npm install -g` (Windows service wrapper respawn mid-update). | ||
| expect(isOcxStartCommandLine( | ||
| 'bun C:/nvm/node_modules/@bitkyc08/.opencodex-1JejBqbZ/src/cli/index.ts start --port 10100', | ||
| )).toBe(true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This case passes through the old src/cli/index.ts match, not the new rename-tree branch.
isOcxStartCommandLine (src/config.ts lines 2161-2172) evaluates its alternatives in order. Line 2168 tests normalized.includes("src/cli/index.ts"), and the asserted command line contains exactly that substring. The check short-circuits there, so the new alternative added on line 2170, /@bitkyc08\/\.opencodex-/, is never reached. Deleting line 2170 would leave this test green.
That matters because this predicate is the identity gate: verifyPidIdentity (src/config.ts lines 2254-2261) calls isLikelyOcxStartProcess, and reclaimListenPort uses the result to decide whether a listener may be killed under killAllOcxOnPort. The new branch is authorization-relevant and currently unverified.
Add a command line that reaches the port through the rename tree without any other entrypoint marker. bin/ocx.mjs does not match the bare-name alternative on line 2171, because the ocx token is followed by . rather than whitespace or a quote.
💚 Proposed additional case
// npm's in-place rename during `npm install -g` (Windows service wrapper respawn mid-update).
expect(isOcxStartCommandLine(
'bun C:/nvm/node_modules/@bitkyc08/.opencodex-1JejBqbZ/src/cli/index.ts start --port 10100',
)).toBe(true);
+ // Isolates the `@bitkyc08/.opencodex-*` alternative: no `src/cli/index.ts`, no
+ // `@bitkyc08/opencodex`, and `ocx.mjs` does not match the bare-name pattern.
+ expect(isOcxStartCommandLine(
+ 'bun C:/nvm/node_modules/@bitkyc08/.opencodex-1JejBqbZ/bin/ocx.mjs start --port 10100',
+ )).toBe(true);As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // npm's in-place rename during `npm install -g` (Windows service wrapper respawn mid-update). | |
| expect(isOcxStartCommandLine( | |
| 'bun C:/nvm/node_modules/@bitkyc08/.opencodex-1JejBqbZ/src/cli/index.ts start --port 10100', | |
| )).toBe(true); | |
| // npm's in-place rename during `npm install -g` (Windows service wrapper respawn mid-update). | |
| expect(isOcxStartCommandLine( | |
| 'bun C:/nvm/node_modules/@bitkyc08/.opencodex-1JejBqbZ/src/cli/index.ts start --port 10100', | |
| )).toBe(true); | |
| // Isolates the `@bitkyc08/.opencodex-*` alternative: no `src/cli/index.ts`, no | |
| // `@bitkyc08/opencodex`, and `ocx.mjs` does not match the bare-name pattern. | |
| expect(isOcxStartCommandLine( | |
| 'bun C:/nvm/node_modules/@bitkyc08/.opencodex-1JejBqbZ/bin/ocx.mjs start --port 10100', | |
| )).toBe(true); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/config.test.ts` around lines 1416 - 1419, Add a focused test near the
existing isOcxStartCommandLine cases using a command line that reaches the start
port through the `@bitkyc08/.opencodex-`* rename tree without containing
src/cli/index.ts or another recognized entrypoint marker. Ensure the assertion
exercises the new rename-tree alternative rather than an earlier matching
branch, preserving coverage for the authorization path used by verifyPidIdentity
and reclaimListenPort.
Source: Path instructions
| test("killAllOcxOnPort kills ocx listeners absent from the allowlist snapshot", async () => { | ||
| const killed: number[] = []; | ||
| let holder = 9001; | ||
| let available = false; | ||
| await expect(reclaimListenPort(10100, "127.0.0.1", { | ||
| timeoutMs: 200, | ||
| intervalMs: 20, | ||
| scanIntervalMs: 20, | ||
| dropTcpRows: false, | ||
| killOcxHolders: true, | ||
| killAllOcxOnPort: true, | ||
| onlyKillPids: [100], // pre-update PID — respawned child is 9001 | ||
| isAvailableFn: async () => available, | ||
| listListenPidsFn: () => (holder > 0 ? [holder] : []), | ||
| isAliveFn: pid => pid === holder, | ||
| verifyOcxFn: pid => pid, | ||
| killFn: pid => { | ||
| killed.push(pid); | ||
| if (pid === holder) holder = 0; | ||
| }, | ||
| sleepMs: async () => { | ||
| if (holder === 0) available = true; | ||
| }, | ||
| })).resolves.toBe(true); | ||
| expect(killed).toEqual([9001]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case where killAllOcxOnPort alone authorizes killing, with an empty onlyKillPids.
This test passes onlyKillPids: [100], so mayKill on src/server/port-reclaim.ts lines 191-192 is already true through the non-empty allowlist. The new half of that gate, allowedKillPids.size === 0 && killAllOcx, is therefore not exercised by any test in this file. A regression that reverted line 192 to allowedKillPids.size > 0 would keep every current test green while silently disabling reclaim for a wrapper-respawned listener whose PID was never captured.
Add one focused case with onlyKillPids: [] (or omitted) and killAllOcxOnPort: true, and assert the revalidated ocx holder is killed.
💚 Proposed regression test
+ test("killAllOcxOnPort authorizes killing with an empty allowlist", async () => {
+ const killed: number[] = [];
+ let holder = 9001;
+ let available = false;
+ await expect(reclaimListenPort(10100, "127.0.0.1", {
+ timeoutMs: 200,
+ intervalMs: 20,
+ scanIntervalMs: 20,
+ dropTcpRows: false,
+ killOcxHolders: true,
+ killAllOcxOnPort: true,
+ // No pre-update PID was captured — killAllOcxOnPort is the only authorization.
+ onlyKillPids: [],
+ isAvailableFn: async () => available,
+ listListenPidsFn: () => (holder > 0 ? [holder] : []),
+ isAliveFn: pid => pid === holder,
+ verifyOcxFn: pid => pid,
+ killFn: pid => {
+ killed.push(pid);
+ if (pid === holder) holder = 0;
+ },
+ sleepMs: async () => {
+ if (holder === 0) available = true;
+ },
+ })).resolves.toBe(true);
+ expect(killed).toEqual([9001]);
+ });As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("killAllOcxOnPort kills ocx listeners absent from the allowlist snapshot", async () => { | |
| const killed: number[] = []; | |
| let holder = 9001; | |
| let available = false; | |
| await expect(reclaimListenPort(10100, "127.0.0.1", { | |
| timeoutMs: 200, | |
| intervalMs: 20, | |
| scanIntervalMs: 20, | |
| dropTcpRows: false, | |
| killOcxHolders: true, | |
| killAllOcxOnPort: true, | |
| onlyKillPids: [100], // pre-update PID — respawned child is 9001 | |
| isAvailableFn: async () => available, | |
| listListenPidsFn: () => (holder > 0 ? [holder] : []), | |
| isAliveFn: pid => pid === holder, | |
| verifyOcxFn: pid => pid, | |
| killFn: pid => { | |
| killed.push(pid); | |
| if (pid === holder) holder = 0; | |
| }, | |
| sleepMs: async () => { | |
| if (holder === 0) available = true; | |
| }, | |
| })).resolves.toBe(true); | |
| expect(killed).toEqual([9001]); | |
| }); | |
| test("killAllOcxOnPort kills ocx listeners absent from the allowlist snapshot", async () => { | |
| const killed: number[] = []; | |
| let holder = 9001; | |
| let available = false; | |
| await expect(reclaimListenPort(10100, "127.0.0.1", { | |
| timeoutMs: 200, | |
| intervalMs: 20, | |
| scanIntervalMs: 20, | |
| dropTcpRows: false, | |
| killOcxHolders: true, | |
| killAllOcxOnPort: true, | |
| onlyKillPids: [100], // pre-update PID — respawned child is 9001 | |
| isAvailableFn: async () => available, | |
| listListenPidsFn: () => (holder > 0 ? [holder] : []), | |
| isAliveFn: pid => pid === holder, | |
| verifyOcxFn: pid => pid, | |
| killFn: pid => { | |
| killed.push(pid); | |
| if (pid === holder) holder = 0; | |
| }, | |
| sleepMs: async () => { | |
| if (holder === 0) available = true; | |
| }, | |
| })).resolves.toBe(true); | |
| expect(killed).toEqual([9001]); | |
| }); | |
| test("killAllOcxOnPort authorizes killing with an empty allowlist", async () => { | |
| const killed: number[] = []; | |
| let holder = 9001; | |
| let available = false; | |
| await expect(reclaimListenPort(10100, "127.0.0.1", { | |
| timeoutMs: 200, | |
| intervalMs: 20, | |
| scanIntervalMs: 20, | |
| dropTcpRows: false, | |
| killOcxHolders: true, | |
| killAllOcxOnPort: true, | |
| // No pre-update PID was captured — killAllOcxOnPort is the only authorization. | |
| onlyKillPids: [], | |
| isAvailableFn: async () => available, | |
| listListenPidsFn: () => (holder > 0 ? [holder] : []), | |
| isAliveFn: pid => pid === holder, | |
| verifyOcxFn: pid => pid, | |
| killFn: pid => { | |
| killed.push(pid); | |
| if (pid === holder) holder = 0; | |
| }, | |
| sleepMs: async () => { | |
| if (holder === 0) available = true; | |
| }, | |
| })).resolves.toBe(true); | |
| expect(killed).toEqual([9001]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/port-reclaim.test.ts` around lines 473 - 498, Add a focused test
alongside the existing killAllOcxOnPort coverage that sets onlyKillPids to an
empty array or omits it while keeping killAllOcxOnPort enabled, then verifies
the revalidated OCX holder is killed and reclaimListenPort succeeds. Ensure the
scenario exercises the allowedKillPids.size === 0 && killAllOcx branch rather
than the non-empty allowlist path.
Source: Path instructions
| /** | ||
| * Bun 1.3.14 macOS Silicon: Worker spawn in this file still segfaults the | ||
| * isolate process after green assertions (balanced counts). Skip the hammer | ||
| * cases on darwin; win32/linux keep full coverage. | ||
| */ | ||
| const skipDarwinWorkerSpawn = process.platform === "darwin"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 \
'test\.skipIf\(skipDarwinWorkerSpawn\)|new Worker|registerStorageWorker|terminateStorageWorker|drainStorageWorkers' \
tests/storage-worker-teardown-isolate.test.tsRepository: lidge-jun/opencodex
Length of output: 5709
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- worker lifecycle symbols ---'
rg -n -C 18 \
'function (terminateStorageWorker|drainStorageWorkers|needsWorkerOsJoinSettle)|const (terminateStorageWorker|drainStorageWorkers)|export .*?(terminateStorageWorker|drainStorageWorkers)|needsWorkerOsJoinSettle' \
src tests
printf '%s\n' '--- Worker registration and test doubles ---'
rg -n -C 10 \
'registerStorageWorker|Worker\b|terminate\(\)|addEventListener\(\)|liveStorageWorkerCount' \
src tests -g '*.ts' -g '*.tsx'
printf '%s\n' '--- test file outline and exact relevant sections ---'
ast-grep outline tests/storage-worker-teardown-isolate.test.ts
sed -n '1,190p' tests/storage-worker-teardown-isolate.test.tsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- tests/storage-worker-teardown-isolate.test.ts ---'
sed -n '1,180p' tests/storage-worker-teardown-isolate.test.ts
printf '%s\n' '--- src/storage/worker-lifecycle.ts ---'
sed -n '64,220p' src/storage/worker-lifecycle.ts
printf '%s\n' '--- focused registration call sites ---'
rg -n -C 8 \
'registerStorageWorker\(' \
src/storage tests/storage-worker-teardown-isolate.test.tsRepository: lidge-jun/opencodex
Length of output: 14442
Add Darwin-safe coverage for registered worker termination.
The three lifecycle tests at tests/storage-worker-teardown-isolate.test.ts:104, :122, and :149 all spawn real workers and are skipped on Darwin. The remaining Darwin test checks only the cycle cap, while the unregistered fake at line 167 exercises only the immediate fallback path. Add a separate test with registerStorageWorker() and a fake worker that triggers its captured close listener from terminate(). Assert concurrent termination or drain and a final liveStorageWorkerCount() of zero. Keep the Darwin skip only on tests that instantiate real workers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/storage-worker-teardown-isolate.test.ts` around lines 43 - 48, Add a
Darwin-safe lifecycle test near the existing storage worker teardown tests that
registers a worker via registerStorageWorker(), uses a fake worker whose
terminate() invokes the captured close listener, and asserts concurrent
termination or drain followed by liveStorageWorkerCount() === 0. Apply
skipDarwinWorkerSpawn only to tests that instantiate real workers, leaving this
fake-worker coverage runnable on Darwin.
| await drainStorageWorkers(); | ||
| // Bun 1.3.14: balanced-count segfault can hit mid-file (before afterAll) on | ||
| // macOS Silicon and ubuntu-latest. Brief settle — not a CI timeout bump. | ||
| if (process.platform !== "win32") await Bun.sleep(250); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run test-state cleanup in a finally block.
drainStorageWorkers() can reject when terminateStorageWorker() times out in src/storage/worker-lifecycle.ts, Lines 186-187. The new await at Line 78 runs before the OPENCODEX_HOME restore, isolatedCodexHome reset, and testDir removal at Lines 82-87.
When a worker times out, the hook exits before those cleanup operations. Later tests can inherit the isolated home, and the temporary directory can remain on disk.
Move the existing state cleanup into finally. Re-throw the drain error after cleanup so the original failure remains visible.
Suggested cleanup shape
- await drainStorageWorkers();
- if (process.platform !== "win32") await Bun.sleep(250);
- // existing environment and temporary-directory cleanup
+ try {
+ await drainStorageWorkers();
+ if (process.platform !== "win32") await Bun.sleep(250);
+ } finally {
+ // existing environment and temporary-directory cleanup
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/storage-worker-teardown-isolate.test.ts` around lines 78 - 81, Update
the teardown hook containing drainStorageWorkers() so OPENCODEX_HOME
restoration, isolatedCodexHome reset, and testDir removal always run in a
finally block. Preserve the existing non-Windows Bun.sleep behavior, and rethrow
any drainStorageWorkers() failure after cleanup so the original error remains
visible.
| const prevService = process.env.OCX_SERVICE; | ||
| delete process.env.OCX_SERVICE; | ||
| try { | ||
| await restartAfterUpdateForTests(job, { port: 19999, hostname: "127.0.0.1" }, { | ||
| serviceInstalledFn: () => true, | ||
| serviceViableFn: () => false, | ||
| waitForPort: async () => true, | ||
| runService: () => ({ status: 1 }), | ||
| spawnStart: (_job, _installer, port) => { | ||
| spawned.push({ port: port ?? 0 }); | ||
| }, | ||
| }); | ||
| // The fallback must fire: direct proxy start instead of throwing. | ||
| expect(spawned).toEqual([{ port: 19999 }]); | ||
| } finally { | ||
| if (prevService === undefined) delete process.env.OCX_SERVICE; | ||
| else process.env.OCX_SERVICE = prevService; | ||
| } | ||
| }); | ||
|
|
||
| test("service reinstall exit 0 with non-viable assets falls back to direct start", async () => { | ||
| const spawned: Array<{ port: number }> = []; | ||
| const job: UpdateJobState = { | ||
| id: "svc-stale-fallback", | ||
| status: "restarting", | ||
| startedAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| currentVersion: "2.7.42", | ||
| latestVersion: "2.7.43", | ||
| channel: "latest", | ||
| installer: "npm", | ||
| restart: true, | ||
| command: "", | ||
| log: [], | ||
| }; | ||
| writeFileSync(updateJobPath(job.id), JSON.stringify(job)); | ||
| const prevService = process.env.OCX_SERVICE; | ||
| delete process.env.OCX_SERVICE; | ||
| try { | ||
| await restartAfterUpdateForTests(job, { port: 19100, hostname: "127.0.0.1" }, { | ||
| serviceInstalledFn: () => true, | ||
| // Installed but stale/missing assets — the status line users see after a dead update. | ||
| serviceViableFn: () => false, | ||
| waitForPort: async () => true, | ||
| runService: () => ({ status: 0 }), | ||
| spawnStart: (_job, _installer, port) => { | ||
| spawned.push({ port: port ?? 0 }); | ||
| }, | ||
| }); | ||
| expect(spawned).toEqual([{ port: 19100 }]); | ||
| expect(readUpdateJob(job.id)?.log.some(line => | ||
| line.includes("not viable") && line.includes("direct proxy start"), | ||
| )).toBe(true); | ||
| } finally { | ||
| if (prevService === undefined) delete process.env.OCX_SERVICE; | ||
| else process.env.OCX_SERVICE = prevService; | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the inverse test for the non-elevated OCX_SERVICE=1 skip.
Both tests in this range now clear OCX_SERVICE so they reach the viability gate, which is correct. The branch they clear away is still untested. src/update/job.ts lines 735-738 set skipServiceInstall = true when process.platform === "win32" && process.env.OCX_SERVICE === "1", log "Skipping service reinstall from the non-elevated update worker", and fall through to the direct pinned start. That behavior is a stated objective of this PR, and no test asserts it.
Without coverage, a regression that drops the guard would leave every existing test green while restoring the UAC-failing schtasks /create race that this PR set out to remove.
Note the platform condition: assert the log line and runService non-invocation only when process.platform === "win32", or gate the test with a platform check so it does not fail on Linux and macOS runners.
💚 Proposed regression test
+ test.skipIf(process.platform !== "win32")(
+ "service reinstall is skipped from the non-elevated Windows update worker",
+ async () => {
+ const spawned: Array<{ port: number }> = [];
+ let serviceRuns = 0;
+ const job: UpdateJobState = {
+ id: "svc-nonelevated-skip",
+ status: "restarting",
+ startedAt: new Date().toISOString(),
+ updatedAt: new Date().toISOString(),
+ currentVersion: "2.7.42",
+ latestVersion: "2.7.43",
+ channel: "latest",
+ installer: "npm",
+ restart: true,
+ command: "",
+ log: [],
+ };
+ writeFileSync(updateJobPath(job.id), JSON.stringify(job));
+ const prevService = process.env.OCX_SERVICE;
+ process.env.OCX_SERVICE = "1";
+ try {
+ await restartAfterUpdateForTests(job, { port: 19200, hostname: "127.0.0.1" }, {
+ serviceInstalledFn: () => true,
+ serviceViableFn: () => true,
+ waitForPort: async () => true,
+ runService: () => { serviceRuns += 1; return { status: 0 }; },
+ spawnStart: (_job, _installer, port) => { spawned.push({ port: port ?? 0 }); },
+ });
+ expect(serviceRuns).toBe(0);
+ expect(spawned).toEqual([{ port: 19200 }]);
+ expect(readUpdateJob(job.id)?.log.some(line =>
+ line.includes("non-elevated update worker"),
+ )).toBe(true);
+ } finally {
+ if (prevService === undefined) delete process.env.OCX_SERVICE;
+ else process.env.OCX_SERVICE = prevService;
+ }
+ },
+ );As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/update-job.test.ts` around lines 344 - 401, Add a focused regression
test near the existing restart-after-update tests covering OCX_SERVICE=1 on
Windows. Set the environment variable, invoke restartAfterUpdateForTests with
service callbacks, and when process.platform is "win32" assert the skip log is
emitted, runService is not called, and direct pinned start occurs; restore the
environment in finally. Skip or gate the Windows-specific assertions on
non-Windows platforms.
Source: Path instructions
| expect(read("src/cli/index.ts")).toContain("preferRetryMs: hardPin ? 5_000 : 750"); | ||
| expect(read("src/cli/index.ts")).toContain("Not opening the GUI"); | ||
| expect(read("src/server/ports.ts")).toContain("allowEphemeralFallback"); | ||
| }); | ||
| test("Windows GUI update worker is launched without inheriting the proxy LISTEN socket", () => { | ||
| // Direct spawn() inherits Bun.serve's LISTEN handle → ghost LISTEN with dead parent PID. | ||
| expect(src).toContain("function spawnGuiUpdateWorker"); | ||
| expect(src).toContain("Start-Process"); | ||
| expect(src).toContain("buildWindowsElevatedArgumentList"); | ||
| expect(src).toContain("resolveTrustedWindowsPowerShellExe"); | ||
| expect(src).toContain("spawnWorkerFn: spawnGuiUpdateWorker"); | ||
| // Foreign listeners must stay fail-closed; npm rename is covered by ocx identity. | ||
| expect(src).not.toContain("killAnyListenPidOnPort"); | ||
| expect(src).toContain('process.platform === "win32" && process.env.OCX_SERVICE === "1"'); | ||
| // Native WinSW installs must stop via stopWinswService, not Task Scheduler /end only. | ||
| expect(src).toContain("readServiceBackend"); | ||
| expect(src).toContain("stopWinswService"); | ||
| expect(src).toContain("$_.ProcessId -eq $PID"); | ||
| expect(src).toContain("lastChild?.pid && aliveFn(lastChild.pid)"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Back the two behavioral claims in this block with executable assertions.
Lines 32 and 50 pin exact source expressions: preferRetryMs: hardPin ? 5_000 : 750 and lastChild?.pid && aliveFn(lastChild.pid). Both literals do exist in the current sources, so the test passes today. The cost is brittleness with no behavioral guarantee. Rewriting 5_000 as 5000, renaming the local aliveFn, or reflowing that condition breaks this test while the behavior is unchanged. The inverse is worse: a change that keeps the text but breaks the semantics still passes.
Line 50 in particular guards a real recovery behavior — src/update/job.ts lines 917-919 terminate a hung pinned-start child after the retries are exhausted. That is reachable through restartAfterUpdateForTests with an injected isAliveFn, so it can be asserted directly in tests/update-job.test.ts. Keep these text assertions as a cheap tripwire if you want, and add one behavioral test that the exhausted-retry path kills the surviving child PID.
As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/windows-deploy-close-regressions.test.ts` around lines 32 - 50, Add a
focused behavioral regression test in tests/update-job.test.ts for
restartAfterUpdateForTests that injects isAliveFn, exhausts the pinned-start
retries, and asserts the surviving child PID is terminated. Keep the existing
source-text assertions in tests/windows-deploy-close-regressions.test.ts only as
optional tripwires, but do not rely on exact formatting or local names to
validate preferRetryMs or lastChild handling.
Source: Path instructions
ubuntu-latest still segfaulted Bun 1.3.14 at the isolate file boundary with balanced worker counts; keep the platform-cap meta-test and full win32 coverage.
Keep background-shell term grace timers ref'd so Bun Windows fires them during unresolved drain, tolerate icacls timeouts on config writes so the data plane can still start, and inject test fsync for directory spill order.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.ts (1)
2191-2197: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winConstrain the npm temporary launcher matcher
At
src/config.ts:2188-2199,normalized.includes("@bitkyc08/opencodex")matches sibling packages such as@bitkyc08/opencodex-malware. The temporary-package regex also matches without path-segment boundaries. The package checks can therefore classify an arbitrary executable as an OpenCodex launcher. For example,notepad /tmp/@bitkyc08/opencodex-cache startreturnstrue.readPid()andverifyPidIdentity()use this result before destructive operations. Require complete package path segments, retain thesrc/cli.tsorsrc/cli/index.tslauncher check for package paths, and add negative tests for sibling packages and arbitrary executables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 2191 - 2197, Constrain the package checks in hasOcxEntrypoint so `@bitkyc08/opencodex` and the `@bitkyc08/.opencodex-` temporary package pattern require complete path-segment boundaries, preventing sibling packages or arbitrary executable arguments from matching. Preserve the existing src/cli.ts and src/cli/index.ts launcher checks for valid package paths, and add negative tests covering sibling packages and unrelated executables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 118-127: The atomic write flow must preserve whether ACL hardening
actually completed. Update AtomicWriteIO.harden to return a success-or-timeout
result, assign hardened from that result instead of unconditionally setting it
true, and ensure tolerated ACL timeouts return false so residual error handling
can retry hardening accurately.
- Around line 106-110: The ACL timeout signal is being lost before config
handling, and tolerated timeouts can incorrectly mark temporary files as
hardened. Update the Windows ACL helper and its callers in the config flow
around `isWindowsAclTimeoutError`, including the call sites near the
required-failure and `atomicWriteFile` handling, to return or preserve a typed
timeout outcome for both initial and memoized failures; ensure tolerated
timeouts leave `atomicWriteFile`’s `hardened` flag false. Add coverage for
initial timeout, memoized timeout, and non-timeout failure.
---
Outside diff comments:
In `@src/config.ts`:
- Around line 2191-2197: Constrain the package checks in hasOcxEntrypoint so
`@bitkyc08/opencodex` and the `@bitkyc08/.opencodex-` temporary package pattern
require complete path-segment boundaries, preventing sibling packages or
arbitrary executable arguments from matching. Preserve the existing src/cli.ts
and src/cli/index.ts launcher checks for valid package paths, and add negative
tests covering sibling packages and unrelated executables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02803b80-77d7-4945-b2b1-6351b5ed06b8
📒 Files selected for processing (5)
src/adapters/cursor/native-exec-shell.tssrc/config.tstests/responses-state.test.tstests/shutdown-drain.test.tstests/storage-worker-teardown-isolate.test.ts
| /** True when required NTFS harden failed closed on a transient icacls timeout. */ | ||
| function isWindowsAclTimeoutError(error: unknown): boolean { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| return /timed out|previous attempt timed out/i.test(message); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'isWindowsAclTimeoutError|hardenSecretPath|hardenSecretDir|timed out|previous attempt timed out' \
src/lib/windows-secret-acl.ts src/config.ts testsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- windows-secret-acl.ts: types, error creation, and harden paths ---'
sed -n '1,80p' src/lib/windows-secret-acl.ts
sed -n '170,205p' src/lib/windows-secret-acl.ts
sed -n '300,470p' src/lib/windows-secret-acl.ts
sed -n '470,515p' src/lib/windows-secret-acl.ts
printf '%s\n' '--- config.ts: atomic write implementations and cleanup state ---'
sed -n '80,235p' src/config.ts
sed -n '235,285p' src/config.ts
sed -n '1450,1580p' src/config.ts
sed -n '1580,1710p' src/config.ts
printf '%s\n' '--- residual error references and timeout tests ---'
rg -n -C 5 'AtomicWriteResidualTempError|hardened|isWindowsAclTimeoutError|ETIMEDOUT|previous attempt timed out' src testsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p;170,205p;300,470p;470,515p' src/lib/windows-secret-acl.ts
sed -n '80,285p;1450,1710p' src/config.ts
rg -n -C 5 'AtomicWriteResidualTempError|hardened|isWindowsAclTimeoutError|ETIMEDOUT|previous attempt timed out' src testsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- config ACL integration tests ---'
sed -n '1460,1605p' tests/config.test.ts
printf '%s\n' '--- exact ACL error and timeout branches ---'
sed -n '180,195p;310,330p;385,465p' src/lib/windows-secret-acl.ts
printf '%s\n' '--- all config timeout-related tests and call sites ---'
rg -n -C 8 'atomic config write ACL|config mutation directory ACL|ACL harden|atomicWriteFileAsync|isWindowsAclTimeoutError' tests src/config.ts
printf '%s\n' '--- deterministic contract probe ---'
python3 - <<'PY'
from pathlib import Path
import re
acl = Path("src/lib/windows-secret-acl.ts").read_text()
config = Path("src/config.ts").read_text()
assert 'err.code = result.timedOut ? "ETIMEDOUT" : "EICACLS";' in acl
assert 'const diagnostics = "ACL hardening skipped — previous attempt timed out";' in acl
assert 'if (opts.required) throw new Error(annotated);' in acl
assert 'if (opts.required) throw new Error(diagnostics);' in acl
assert 'return /timed out|previous attempt timed out/i.test(message);' in config
# Model the public required:true error text currently produced by both timeout paths.
public_timeout_messages = [
"ACL hardening timed out (ETIMEDOUT); no broad ACL grants detected (hardening still incomplete)",
"ACL hardening skipped — previous attempt timed out",
]
for message in public_timeout_messages:
assert re.search(r"timed out|previous attempt timed out", message, re.I)
# Model a normal required hardening failure after sanitizeDiagnostics.
ordinary_failure_messages = [
"ACL hardening failed (EICACLS) — icacls command error; filesystem may not support per-user NTFS ACLs",
"ACL hardening failed (EACCES) — permission denied running icacls",
"ACL hardening failed — filesystem may not support per-user NTFS ACLs",
]
for message in ordinary_failure_messages:
assert not re.search(r"timed out|previous attempt timed out", message, re.I)
print("current public timeout messages match the config regex")
print("current sanitized non-timeout messages do not match the config regex")
print("the ACL module already has a typed internal ETIMEDOUT predicate, but required errors discard that code")
PYRepository: lidge-jun/opencodex
Length of output: 22629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1460,1605p' tests/config.test.ts
sed -n '180,195p;310,330p;385,465p' src/lib/windows-secret-acl.ts
rg -n -C 8 'atomic config write ACL|config mutation directory ACL|ACL harden|atomicWriteFileAsync|isWindowsAclTimeoutError' tests src/config.ts
python3 - <<'PY'
from pathlib import Path
import re
acl = Path("src/lib/windows-secret-acl.ts").read_text()
config = Path("src/config.ts").read_text()
assert 'err.code = result.timedOut ? "ETIMEDOUT" : "EICACLS";' in acl
assert 'const diagnostics = "ACL hardening skipped — previous attempt timed out";' in acl
assert 'if (opts.required) throw new Error(annotated);' in acl
assert 'return /timed out|previous attempt timed out/i.test(message);' in config
for message in [
"ACL hardening timed out (ETIMEDOUT); no broad ACL grants detected",
"ACL hardening skipped — previous attempt timed out",
]:
assert re.search(r"timed out|previous attempt timed out", message, re.I)
for message in [
"ACL hardening failed (EICACLS) — icacls command error",
"ACL hardening failed (EACCES) — permission denied running icacls",
"ACL hardening failed — filesystem may not support per-user NTFS ACLs",
]:
assert not re.search(r"timed out|previous attempt timed out", message, re.I)
print("current public timeout messages match the config regex")
print("current sanitized non-timeout messages do not match the config regex")
print("required errors discard the internal ETIMEDOUT code")
PYRepository: lidge-jun/opencodex
Length of output: 22403
Expose timeout state from the ACL helper instead of parsing error text.
src/lib/windows-secret-acl.ts identifies timeouts with ETIMEDOUT, but required failures and memoized timeouts discard that code before src/config.ts:107-109 handles them. Return a typed timeout result or preserve an exported timeout predicate for both call sites at src/config.ts:122 and src/config.ts:1552. Ensure tolerated timeouts do not set atomicWriteFile’s hardened flag, or residual cleanup can report an unhardened temporary file as hardened. Add tests for initial timeout, memoized timeout, and non-timeout failure.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 106 - 110, The ACL timeout signal is being lost
before config handling, and tolerated timeouts can incorrectly mark temporary
files as hardened. Update the Windows ACL helper and its callers in the config
flow around `isWindowsAclTimeoutError`, including the call sites near the
required-failure and `atomicWriteFile` handling, to return or preserve a typed
timeout outcome for both initial and memoized failures; ensure tolerated
timeouts leave `atomicWriteFile`’s `hardened` flag false. Add coverage for
initial timeout, memoized timeout, and non-timeout failure.
| hardenSecretPath(target, { required: true }); | ||
| } catch (err) { | ||
| // Data-plane invariant: ACL timeout must not abort atomic config writes / | ||
| // server start (management-auth catches the same class of failure). | ||
| if (!isWindowsAclTimeoutError(err)) throw err; | ||
| console.warn( | ||
| "[opencodex] atomic config write ACL harden timed out:", | ||
| err instanceof Error ? err.message : err, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'AtomicWriteResidualTempError|hardened|io\.harden' \
src testsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/config.ts ---'
sed -n '1,190p' src/config.ts
printf '%s\n' '--- config hardening tests ---'
sed -n '1460,1560p' tests/config.test.ts
printf '%s\n' '--- ACL timeout implementation and tests ---'
sed -n '320,390p' src/lib/windows-secret-acl.ts
sed -n '380,450p' tests/windows-secret-acl.test.tsRepository: lidge-jun/opencodex
Length of output: 17188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/config.ts")
lines = p.read_text().splitlines()
for start, end in [(70, 180)]:
for n in range(start, min(end, len(lines)) + 1):
print(f"{n}:{lines[n-1]}")
PY
printf '%s\n' '--- targeted config tests ---'
rg -n -A75 -B8 'successive atomic temps|residual|timeout|timed out|ACL harden' tests/config.test.tsRepository: lidge-jun/opencodex
Length of output: 14384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
class Residual(Exception):
def __init__(self, hardened):
self.hardened = hardened
def atomic_write(io):
hardened = False
try:
io["write"]()
io["harden"]()
hardened = True
io["rename"]()
except Exception as cause:
scrubbed = False
try:
io["truncate"]()
scrubbed = True
except FileNotFoundError:
scrubbed = True
removed = False
try:
io["unlink"]()
removed = True
except FileNotFoundError:
removed = True
if not removed and not scrubbed:
raise RuntimeError("secret residual")
retry_count = 0
if not removed and not hardened:
retry_count += 1
try:
io["harden"]()
hardened = True
except Exception:
pass
if not removed:
raise Residual(hardened)
raise cause
calls = []
def harden():
calls.append("harden")
# Models a tolerated ACL timeout: the default callback logs and returns.
def rename():
calls.append("rename")
raise OSError("rename failed")
def truncate():
calls.append("truncate")
def unlink():
calls.append("unlink")
raise PermissionError("unlink failed")
try:
atomic_write({
"write": lambda: calls.append("write"),
"harden": harden,
"rename": rename,
"truncate": truncate,
"unlink": unlink,
})
except Residual as error:
print({"hardened": error.hardened, "calls": calls})
PYRepository: lidge-jun/opencodex
Length of output: 550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
class Residual(Exception):
def __init__(self, hardened):
self.hardened = hardened
def atomic_write(io):
hardened = False
try:
io["write"]()
io["harden"]()
hardened = True
io["rename"]()
except Exception as cause:
scrubbed = False
try:
io["truncate"]()
scrubbed = True
except FileNotFoundError:
scrubbed = True
except Exception:
try:
io["write_empty"]()
scrubbed = True
except Exception:
pass
removed = False
try:
io["unlink"]()
removed = True
except FileNotFoundError:
removed = True
except Exception:
try:
io["unlink_retry"]()
removed = True
except FileNotFoundError:
removed = True
except Exception:
pass
if not removed and not scrubbed:
raise RuntimeError("secret residual") from cause
retry_harden_calls = 0
if not removed and not hardened:
retry_harden_calls += 1
try:
io["harden"]()
hardened = True
except Exception:
pass
if not removed:
raise Residual(hardened) from cause
raise cause
calls = []
def harden():
calls.append("harden")
# Models the default callback swallowing a tolerated ACL timeout.
def rename():
calls.append("rename")
raise OSError("rename failed")
def truncate():
calls.append("truncate")
def unlink():
calls.append("unlink")
raise PermissionError("unlink failed")
def unlink_retry():
calls.append("unlink_retry")
raise PermissionError("unlink failed")
def write_empty():
calls.append("write_empty")
try:
atomic_write({
"write": lambda: calls.append("write"),
"write_empty": write_empty,
"harden": harden,
"rename": rename,
"truncate": truncate,
"unlink": unlink,
"unlink_retry": unlink_retry,
})
except Residual as error:
print({"hardened": error.hardened, "calls": calls})
PYRepository: lidge-jun/opencodex
Length of output: 254
Preserve the actual ACL state in hardened.
When io.harden tolerates an ACL timeout, atomicWriteFile sets hardened = true at src/config.ts:140 although hardening did not complete. If rename and cleanup fail, the residual error reports an inaccurate state and skips the residual hardening attempt.
Make AtomicWriteIO.harden return a success or timeout result. Assign hardened from that result, and keep tolerated timeouts as false.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 118 - 127, The atomic write flow must preserve
whether ACL hardening actually completed. Update AtomicWriteIO.harden to return
a success-or-timeout result, assign hardened from that result instead of
unconditionally setting it true, and ensure tolerated ACL timeouts return false
so residual error handling can retry hardening accurately.
Take upstream kill-grace ref and coordination-dir ACL degrade, and narrow the management-auth icacls fixtures so directory stalls do not block secret-file config persistence on real Windows.
Two bun x tsc --noEmit spawns and gated refresh CAS races exceeded Bun's 5s default on the windows leg after the suite hang was cleared.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/config.ts (1)
111-111: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse one platform-gating abstraction in production and tests.
Both sites bypass the ACL module’s shared platform override. This can produce inconsistent ACL coverage and contaminate concurrent tests.
src/config.ts#L111-L111: callwindowsSecretAclApplies()beforehardenSecretPath.tests/config.test.ts#L1627-L1628: usewindowsAcl.setPlatformForTests("win32")and reset it infinally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` at line 111, Use the ACL module’s shared platform abstraction consistently: in src/config.ts lines 111-111, guard hardenSecretPath with windowsSecretAclApplies() instead of directly checking process.platform; in tests/config.test.ts lines 1627-1628, set the simulated platform through windowsAcl.setPlatformForTests("win32") and reset it in a finally block.Source: Path instructions
tests/config.test.ts (1)
1627-1628: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the ACL platform override in
tests/config.test.ts:1627-1638. When the test forces Windows behavior, callwindowsAcl.setPlatformForTests("win32")and reset it withwindowsAcl.setPlatformForTests(null)infinally.saveConfig()useswindowsSecretAclApplies(), so this override reaches the tested path without mutating process-global state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/config.test.ts` around lines 1627 - 1628, Update the Windows-behavior test around the process.platform override to use the windowsAcl.setPlatformForTests API instead: set it to "win32" before exercising saveConfig(), and reset it to null in a finally block. Remove the process.platform mutation while preserving the test’s existing assertions and cleanup.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/config.ts`:
- Line 111: Use the ACL module’s shared platform abstraction consistently: in
src/config.ts lines 111-111, guard hardenSecretPath with
windowsSecretAclApplies() instead of directly checking process.platform; in
tests/config.test.ts lines 1627-1628, set the simulated platform through
windowsAcl.setPlatformForTests("win32") and reset it in a finally block.
In `@tests/config.test.ts`:
- Around line 1627-1628: Update the Windows-behavior test around the
process.platform override to use the windowsAcl.setPlatformForTests API instead:
set it to "win32" before exercising saveConfig(), and reset it to null in a
finally block. Remove the process.platform mutation while preserving the test’s
existing assertions and cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ab497ff-edfd-4a5a-83f5-5e2c74e2ab9a
📒 Files selected for processing (6)
src/adapters/cursor/native-exec-shell.tssrc/config.tstests/config.test.tstests/oauth-refresh-generic-lock.test.tstests/server-management-auth.test.tstests/translator-budget.test.ts
Take tip's darwin settle + isolate churn cap so this branch stays current with the storage-worker teardown fix line.
2 participants
Summary
ocx start --portso/healthzcomes back without a viable service.Start-Process.:loop, allowlisted teardown PIDs), skip non-elevated service reinstall from the update worker, and tolerate brief/healthzblips during the stability window.Verified on a real Windows walk: labeled install
2.7.0→ dashboard update to registry2.8.0→ jobsucceeded/restarted: true//healthz2.8.0.Test plan
bun test ./tests/update-job.test.ts ./tests/port-reclaim.test.ts ./tests/windows-deploy-close-regressions.test.ts/api/update/run→ job succeeded, proxy healthy on target versionSummary by CodeRabbit