fix(update): preserve proxy on npm cache failures

[WZBbiao]

Summary

• Check the effective npm cache ownership on Unix before either npm update path stops the proxy. Foreign-owned entries, missing cache roots, inspection failures, and traversal-budget exhaustion now abort while the existing service remains available.
• Run the blocking cache walk in a separate Node/Bun process. The parent enforces the 10-second deadline with SIGKILL, and persisted failure text contains neither cache/entry paths nor Unix account identifiers.
• When a GUI-launched installer exits nonzero after stopping a previously active proxy, retain identity-checked PID/runtime evidence across transient health misses, re-probe for a concurrent replacement immediately before recovery, and refuse service/direct restart when a different identity-checked PID appears, including after port reclaim. Recovery can use a validated current or npm-retired launcher. The update job remains failed so availability recovery is not reported as an upgrade success.
• Run npm installs in an isolated process tree. POSIX cleanup ignores zombie-only groups and protects against reused process-group IDs; Windows failed-root exits remain fail-closed when descendant shutdown cannot be proven. Recovery and tray restoration are skipped whenever cleanup is unconfirmed.
• Revalidate PID identity at update and recovery boundaries so a reused PID cannot inherit stale command-line cache evidence.
• Validate recovery-package ownership and probe at most two current/newest npm-retired launchers with a side-effect-free node launcher --version check before attempting fallback.

Incident

A real GUI update from 2.7.33 to 2.7.41 reproduced the failure. The updater stopped the proxy, then npm install -g failed with EACCES because the configured ~/.npm cache contained hundreds of entries owned by a different Unix account. npm had already moved the previous package into a hidden transaction directory, leaving a partial install, and the failed update path exited without restoring the service.

The preserved incident cache is rejected by this preflight for an ownership mismatch, while the current user-owned cache passes.

Verification

• Focused update/recovery suites covering cache preflight, failed-install recovery, PID replacement safety, descendant-process termination, and launcher fallback (135 pass, 0 fail)
• bun run typecheck
• bun run prepush and the repository pre-push hook (5059 pass, 0 fail, 382 files; typecheck, GUI lint, and privacy scan passed)
• npm pack --dry-run --ignore-scripts --json confirmed the preflight and installer-process runtime/type files are included in the 337-entry package
• Deadline regression verifies a scanner that ignores SIGTERM is terminated by the parent deadline
• Real-cache regression: the current cache passes in about 502ms; the preserved incident cache is rejected in about 23ms without persisting cache paths, account names, or numeric UIDs

Security Review

This change touches the dependency-installation boundary. The preflight runs the configured npm executable with fixed config get cache arguments, then runs the packaged scanner through the current Node/Bun executable with fixed arguments, shell: false, bounded stdin, a 10-second timeout, and SIGKILL. The scanner canonicalizes only the configured cache-root symlink, performs bounded read-only lstat/directory traversal, and never follows nested symlinks or mutates cache contents. Failed-install recovery considers at most two current/newest packages and accepts them only when the package root, manifest, and launcher are owned by the current UID or root, are not symlinks, and the launcher is contained in a regular package directory whose manifest has the expected package name and pre-update version. Maintainer security review is explicitly requested for the process boundary, path handling, retired-package validation, PID identity checks, and fail-closed behavior.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

• New Features
  • Added a Unix npm cache ownership pre-check to ocx update, aborting before the proxy is stopped when ownership can’t be verified.
  • Added safer, trusted recovery-tree scanning to gate automated recovery.
• Bug Fixes
  • Hardened the installer execution with process-tree cleanup confirmation, bounded output, and improved interruption/timeout handling.
  • Strengthened restart/recovery eligibility using fresh identity and liveness evidence.
• Documentation
  • Updated ocx update CLI docs (including translations) to describe the new pre-check.
• Tests
  • Expanded coverage for npm cache pre-flight, installer process isolation, and recovery/restart scenarios.

[WZBbiao]

Addressed the CodeRabbit outside-diff cache-traversal finding in commit 1490ca9. The preflight now fails closed after 50,000 discovered entries or 10 seconds, with injectable budgets and checks around stat, directory reads, and entry iteration. Regressions cover both limits; the current 15,718-entry cache passes in about 317ms, and the retained 36,202-entry incident cache still rejects promptly on ownership mismatch.

[WZBbiao]

CodeRabbit follow-up is in 314eb53. I also adopted the outside-diff PID-helper consolidation because it keeps all replacement-process safety decisions on one implementation while preserving the direct path PID for killProxy. I did not add the optional errorIssue factory: that suggestion is presentation-only, the current error variants are already covered by the structured result contract, and avoiding an unrelated refactor keeps this incident fix focused. Verification: 55 focused tests passed; the full pre-push gate passed 4,964 tests, typecheck, GUI lint, and privacy scan.

[WZBbiao]

@Ingwannu Follow-up head a28d6f8 is pushed after the latest Codex and CodeRabbit findings. The exact staged diff passed the local guard, 74 focused tests, and two full pre-push gates with 5,074 tests / 24,933 assertions, typecheck, GUI lint, and privacy scan. The new fork workflow runs are awaiting maintainer approval: Cross-platform CI https://github.com/lidge-jun/opencodex/actions/runs/30262870035 and React Doctor https://github.com/lidge-jun/opencodex/actions/runs/30262869987. Please approve these exact-head runs when convenient. This requests CI execution only; the PR remains open and unmerged while the fresh automated reviews complete.

[WZBbiao]

@Ingwannu The follow-up head 2f21501e is ready for cross-platform verification. Both fork workflows are awaiting maintainer approval:

• Cross-platform CI: https://github.com/lidge-jun/opencodex/actions/runs/30265995365
• React Doctor: https://github.com/lidge-jun/opencodex/actions/runs/30265995218

The local pre-push gate passed 5,078 tests with typecheck, GUI lint, and privacy scan. I will keep monitoring the exact-head reviews and CI after approval.

[Wibias]

Review — bugs, edge cases, leaks & security

Updated verdict: changes requested. The original review found the security posture deliberately fail-closed and broadly sound, with one residual availability concern. A follow-up review of the current head found two additional real blockers that should be fixed before merge.

Security

• Preflight before stop — Unix cache ownership scan in bin/ocx.mjs and src/update/index.ts runs before proxy unload; nested symlinks and traversal-budget exhaustion fail closed; dedicated preflight errors omit paths and UIDs.
• PID reuse — verifyPidIdentityFresh and sameGeneration are checked before killing recovery processes; replacement PIDs are left untouched; health alone is not treated as restart success.
• Installer cleanup — POSIX refuses leaderless or reused PGIDs; Windows fails closed when tree exit cannot be proven, and exit code 75 blocks unsafe recovery.
• Recovery trust — Recovery candidates are checked for trusted ownership and permissions, non-symlink roots, manifest name/version, path containment, bounded tree scanning, and a side-effect-free node launcher --version probe.
• Command injection — --tag is allowlisted; spawns use argv arrays; worker processes use shell: false.
• Accepted residuals — Windows skips Unix cache-ownership checks; integrity preflight may return { ok: "skipped" }.

Bugs and edge cases

Severity	Location	Issue
High	src/update/npm-cache-preflight.mjs	The preflight validates UID ownership, but not effective writability. A same-UID cache that is mode 0500, blocked by ACLs, or mounted read-only can pass. The updater may then stop the proxy and hit EACCES during npm install, recreating the availability failure this PR is intended to prevent. Require effective read/search/write access and fail before stopping the proxy when access cannot be proven. Add a same-UID 0500 regression proving the proxy and tray are not stopped.
High	src/update/job.ts / persisted update-job.json	Raw installer stdout and stderr are appended to the persisted job log. npm errors commonly contain absolute home/cache paths, usernames, numeric UIDs, and transaction paths. The dedicated preflight formatter sanitizes its own errors, but installer output is stored without equivalent redaction and exposed through the management API. Persist only sanitized, path-free summaries. Add a regression where npm emits /home/alice/.npm/_cacache/... and uid=1001, then assert neither appears in job.command, job.log, or job.error.
Medium	src/update/install-process.mjs + src/update/job.ts (installerFailureAllowsRecovery)	Nonzero installer exits do not yield treeExited: true on the conservative paths. POSIX requires status === 0; Windows treats failed root exit as unknown. As a result, after a stopped proxy and failed npm install, validated .opencodex-* recovery launchers cannot run. This is security-conservative but conflicts with the ADR's best-effort restore goal. The primary foreign-cache incident is still prevented by preflight.
Low	bin/ocx.mjs bake-port capture	The CLI path trusts runtime-port.json using process existence rather than a fresh OpenCodex identity check. Under PID reuse this can bake the wrong port, though it is not a kill hazard.
By design	POSIX orphan descendants	A failed installer root with surviving in-group children remains fail-closed with treeExited: false.

Leaks and resource hygiene

• Cache and recovery scans run in bounded worker processes with hard deadlines.
• Captured installer output is bounded in memory.
• Timers and signal handlers are removed in runProcessTreeCommand.
• Detached restart and recovery processes call unref().
• Unhealthy recovery candidates are killed only after both generation and fresh identity validation.

Non-blocking notes

1. recovery-tree-scan.mjs does not pin argv[1] to its own module path the way the cache worker does.
2. The fail-closed recovery behavior should be documented clearly if nonzero installer exits intentionally require manual package repair or ocx start.
3. Full Linux, macOS, Windows, and npm-global CI should pass on the current head before merge.

Recommendation

Fix the two blocking issues first:

1. Make the Unix npm-cache preflight validate effective access, not only ownership.
2. Sanitize all installer-derived data before persisting it to update-job.json.

Then either document that clean nonzero installer failures require manual recovery, or add a narrowly proven clean failed exit + confirmed tree gone path that enables validated recovery without reopening PID-reuse or escaped-descendant races.

[lidge-jun]

Maintainer takeover follow-up opened as #557 against dev.

It addresses the two high blockers from the latest review on this PR:

• effective npm cache access check before proxy stop
• sanitization of persisted update-job command/log/error fields

I am leaving this PR open/draft and not merging the takeover PR automatically because the remaining nonzero-installer recovery policy is updater/security-adjacent and needs maintainer review.

[lidge-jun]

Follow-up on the takeover PR #557.

Its first CI run failed on windows-latest only, in a test that comes from this PR's stack (1fdc443d2), not from the takeover patch:

(fail) GUI update execution decisions > service restart waits on the captured port and clears OCX_BAKE_PORT after install

Cause: tests/update-job.test.ts asserted the spawned launcher path with a hardcoded POSIX separator:

expect(launchersDuringInstall[0]?.endsWith("/bin/ocx.mjs")).toBe(true);

The value under test comes from join(canonicalRoot, "bin", "ocx.mjs") in src/update/job.ts, which yields ...\bin\ocx.mjs on Windows, so the assertion can never hold there. Fixed in #557 by asserting with the platform separator (join("bin", "ocx.mjs")); the production path was not changed.

windows-latest now passes on #557 head b0434ea584c4b51d60facd6a40ba34414ab12c16, with the full matrix green.

If you prefer to keep this PR as the merge vehicle, please cherry-pick the same assertion fix here — otherwise Windows CI will keep failing on this branch.

[Wibias]

@lidge-jun When there is no near reply i would suggest we just maintainer-takeover this one so we can fix it for the users.

[WZBbiao]

Thanks for the follow-up. I confirm that #557 should be the sole merge vehicle, so I will not cherry-pick the Windows assertion fix back into #533 or continue parallel changes here.

I have reviewed #557 and support keeping nonzero installer failures fail-closed with a documented manual recovery path. Please proceed with the maintainer takeover. GitHub currently reports #557 as conflicting with the latest dev, so it will need a maintainer rebase followed by fresh CI and review before being marked ready.

Once #557 is merged, #533 can be closed as superseded. Please retain attribution to #533 and @WZBbiao; the attribution request in #557 still stands.

[lidge-jun]

CLOSE-RECOMMENDED once #557 lands — superseded, not rejected. Leaving this open for now.

I verified the relationship rather than inferring it from the descriptions. git range-diff against the correct merge bases pairs all 14 commits on this branch with the first 14 on #557 and marks only two as new there: d215cbeb9 (effective cache access plus persisted-field sanitization) and b0434ea58 (the Windows-incompatible "/bin/ocx.mjs" assertion). #557 is a strict superset of this work, which matches what this thread already converged on.

So the design here is not in question. The pre-stop cache preflight, the bounded process-tree installer wrapper, the PID and generation checks before recovery, the validated current and retired launcher probing, and the captured-port restoration all carry forward intact. What this branch lacks are the two blockers accepted at the end of the review: no effective accessSync, so a same-UID but unwritable cache still passes, and no sanitizeUpdateJobText, so raw installer output can reach persisted update-job fields.

Both of those are #557's job now, and I have flagged there that its sanitizer does not yet close the second one — its path regexes stop at whitespace, so a Windows profile containing a space survives unredacted. That is a #557 fix, not something to reopen here.

To be explicit about two things this thread asked for. Attribution stays: #557 carries your commits, and the merge will say so. And nobody is asking you to rebase 3,000 lines in parallel — that was the point of the takeover.

What happens next: this stays open until #557 is corrected and merged, then closes as superseded. If you want to help, the highest-value thing is reviewing the sanitizer fix on #557 when it appears, since you know that code better than anyone.

Thanks for carrying this as far as you did. 91 reviews is a lot of patience.

[lidge-jun]

Status update so this is not left ambiguous: staying open, with a stated closing condition.

I am not closing this yet even though #557 supersedes it, because #557 still carries an unfixed blocker — its log sanitizer terminates every path regex at whitespace, so a Windows profile with a space in the name survives unredacted into persisted update-job state. I ran its regexes to confirm. Closing this now would leave the original closed and the successor blocked, which is the worst of both.

The condition is simple: this closes as superseded when #557 lands. Not when #557 is corrected — when it merges. Your commits go in with it and attribution stays.

Nothing is being asked of you here. If you want to help, reviewing the sanitizer fix on #557 when it appears would be worth more than anything on this branch, since you wrote the surrounding code.

[lidge-jun]

Closing in favour of #557, which supersedes this branch.

Comparing by patch-id, all 14 update-recovery commits here appear in #557, and #557 adds two further hardening commits on top. Keeping both open means reviewing the same series twice, so this one closes and #557 remains the live vehicle.

To be explicit about one thing that could have been misread: #737 (tolerate late proxy readiness, already on dev) does not replace this work. It changes src/update/job.ts and its test — 86 lines — and contains none of the cache-ownership preflight, recovery-tree validation, or process-tree cleanup. That design is still wanted; it needs a rebase and the security review that dependency-install and path-trust changes require under MAINTAINERS.md.