fix(update): recover Windows dashboard proxy after self-update - #838
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe update and restart paths now distinguish viable services from stale installations, reclaim eligible OpenCodeX listeners, retry hard-pinned ports, use Windows-safe worker spawning, and validate replacement processes with identity-aware health checks. ChangesService and restart recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateJob
participant PortReclaimer
participant ServiceDiagnostics
participant Proxy
UpdateJob->>PortReclaimer: reclaim captured port and inspect holders
PortReclaimer-->>UpdateJob: return port availability and holder details
UpdateJob->>ServiceDiagnostics: reinstall service and check viability
ServiceDiagnostics-->>UpdateJob: return viable or non-viable status
UpdateJob->>Proxy: start pinned proxy when recovery is required
Proxy-->>UpdateJob: return identity-aware health result
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".
![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".
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
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
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.
Gate live-package Bun bind probes with isRealBunBinary so a postinstall stub cannot block pinned start, and clear the reclaim killed-set when a LISTEN owner is dead so a respawn that reuses the PID is not skipped.
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 (1)
src/server/port-reclaim.ts (1)
226-250: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not authorize teardown with a PID alone after identity verification fails.
Line 229 removes the PID from
killed, butallowedKillPidsretains it. If the original process exits and Windows reuses that PID for a foreign process that binds this port, Line 238 enters the allowlisted failed-identity path and Line 241 kills the foreign process.Store a stable process identity with each teardown target, such as the Windows process creation time plus PID. Require that identity to match immediately before termination. If identity verification fails, preserve the listener and do not kill it. Add regression coverage for a reused allowlisted PID whose replacement fails OCX verification.
🤖 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 226 - 250, Update the teardown tracking around allowedKillPids and the failed-identity branch in the port-reclaim flow so allowlisting stores a stable process identity alongside each PID, such as creation time and PID. Immediately before killFn, require the current process identity to match the recorded teardown target; if verification fails or the identity differs, preserve the listener and do not kill it. Add regression coverage for an allowlisted PID reused by a foreign process that fails verifyOcxFn.
🤖 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/server/port-reclaim.ts`:
- Around line 226-250: Update the teardown tracking around allowedKillPids and
the failed-identity branch in the port-reclaim flow so allowlisting stores a
stable process identity alongside each PID, such as creation time and PID.
Immediately before killFn, require the current process identity to match the
recorded teardown target; if verification fails or the identity differs,
preserve the listener and do not kill it. Add regression coverage for an
allowlisted PID reused by a foreign process that fails verifyOcxFn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3cee383f-ab76-4c77-afa4-5886c3fe2598
📒 Files selected for processing (3)
src/server/port-reclaim.tssrc/update/job.tstests/port-reclaim.test.ts
|
[shipping-github] Addressed feedback |
|
Merging this. Why it helps: Windows dashboard npm self-update can leave the proxy dead when Background Service reinstall cannot elevate. This recovers with a direct Ship it. |
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
Bug Fixes
Reliability