Repository navigation
fix(codex): refuse uncertain prefix-local Node observations - #7577
Merged
lidge-jun merged 1 commit intoOct 23, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: refusing uncertain prefix-local Node observations instead of using PATH fallback. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
luvs01
marked this pull request as ready for review
October 23, 2026 02:00
Ingwannu
approved these changes
Oct 23, 2026
Ingwannu
left a comment
Ingwannu
left a comment
Owner
There was a problem hiding this comment.
Reviewed the complete three-file diff at 62d12d1ab7f18ebbbb21836e54c01aade034d5c7. Only definite absence now permits the prefix-local Node-to-PATH fallback. Refused/unavailable observations fail without probing a different Node, matching the npm shim's selection contract. The new controls exclude the npm prefix from PATH, so they exercise the formerly incorrect fallback rather than a same-file result. The owning documentation is synchronized.
Independent capped Linux/Bun 1.4.3: bun test tests/codex-integration/codex-cli-installation-targets.test.ts — 19 pass / 1 platform skip / 0 fail / 27 assertions. These seam/derivation tests do not certify the native Windows reader or update execution. Current-head hosted CI 39885641950, including gates/Typecheck, succeeded; no unresolved thread or standing change request was present.
Full local suite/typecheck were not repeated under this shared host's limits; completed hosted Ubuntu checks are the broader evidence. Approval is for this head/diff, with later integration-union validation still required. No real installation, account or service was touched.
lidge-jun
added a commit
that referenced
this pull request
Oct 28, 2026
* fix(codex): keep npm Node cache within inspection lifetime * feat(codex): add confirmed CLI update apply candidate (cherry picked from commit ad1bb33) * fix(codex): preserve canonical staging and complete install preimages (cherry picked from commit a38c1a0) * fix(codex): await archive descriptor release before readback (cherry picked from commit 25b698e) * feat(codex): stage durable CLI update job management (cherry picked from commit be9df8a) * fix(codex): bind management launches and preserve uncertain job lifetimes (cherry picked from commit f33757a) * test(codex): align POSIX CLI job consumer contracts Carry only the request-id, POSIX job-store and status usage hunks from 86ad5a0; Windows read-only plan tests remain separate. * fix(codex): initialize confirmed CLI update jobs safely (cherry picked from commit 7db5a1e) * test(codex): preserve job DTO evidence during matching (cherry picked from commit 1c61f31) * test(codex): isolate npm wrapper module scope (cherry picked from commit 951e594) * docs(codex): describe POSIX update apply and job consumers * fix(cli): document Codex updater resource parity (cherry picked from commit b1dc856) * fix(codex): preserve update job results across store failures * fix(api): advertise retry delay for CLI job admission contention * fix(codex): recover owned job locks and signal apply failures * test(codex): isolate admission lock replacement identity guards * test(codex): align unapplied CLI consumer exit expectation * feat(codex): prove explicit Windows npm installations for read-only plans Port the Windows-plan hunks of 44353b9, 86ad5a0 and c291ff8 onto the 7586 POSIX stack head 4c22308. A proof-captured absolute standard npm codex.cmd shim, its global-prefix package, prefix-local Node/npm and the prefix-local optional backing are bound by held-handle identity re-verification; the plan then reports applicable: false with refusal windows_apply_disabled. PATH-only selection and separate-prefix Node stay refused; Windows apply remains disabled. The independent cli-installation-targets refused-probe fix (#7577) is intentionally not included. * fix(codex): preserve CLI results after persistence failure * fix(test): promote Bun 1.4.4 with independent runner role verification * docs: align Docker Bun comments with unified role defaults * test: use a draining TLS origin for early upload replies * test: confine draining origin to buffered HTTP/1 upload * fix(codex): run cli-update registry queries and session scans async PR #7586 review P1: planning ran three sequential npm view spawnSync calls (12s deadline each) plus the synchronous process-table scan in the server process, parking the proxy event loop for unrelated requests and streaming; background apply repeats the same planning. - spawnCodexCliUpdateMetadata: async child runner preserving the deadline, 1 MiB stdout bound, SIGKILL + bounded reap, and the status-null refusal lanes of spawnSync - resolveCodexCliUpdateTarget/resolveCodexCliUpdateArtifactTarget are async; createCodexCliUpdatePlan awaits them; CodexCliUpdateSpawn keeps sync test injections assignable - scanCodexSessionProcessesAsync plus async darwin/Windows enumeration twins; plan and apply prefer the injected async seam, then sync dep, then the async default - gated-child regressions: a real npm-view-shaped child held on a release file proves timers and a concurrent management request stay responsive while planning waits; wall-clock bounds discriminate a parked loop and the child self-cap keeps the harness hang-free * fix(codex): keep bare Windows check content-observation opt-in A bare check now requests the entrypoint, npm CLI, Node and native files metadata-only: the same handles stay open and identity-bound, but zero content bytes are streamed and installContentDigest stays null. The plan path still opts in, so the plan-bound digest and evidence are unchanged. CodeRabbit review on #7596 comment 4365760303. * docs(cli): align localized update-check wording with Windows x64 support The seven localized CLI pages still claimed Windows defers inspection without reading candidates. Align each check paragraph with the English contract: supported x64 npm installs are inspected under reparse-refusing held handles, and a supported Windows install can produce a deterministic read-only plan refused with windows_apply_disabled. CodeRabbit review on #7596 comment 4365760298. * fix(codex): settle cli-update metadata on the child's terminal event Independent review found real defects the first async revision carried over from spawnSync semantics: - spawnCodexCliUpdateMetadata is now an execFile callback runner: the result resolves only after the child terminated and its piped stdio was consumed, so the caller's isolation cleanup can never outrun a live child; encoding utf8 decodes the buffered output once so a multibyte character split across OS pipe chunks stays intact; maxBuffer bounds both captured streams in bytes; the per-query deadline is latched so a late code-0 close after the kill still refuses, and the arbitrary 1 s reap that settled without close evidence is gone. - applyCodexCliUpdate: the awaited final process scan is the last yield before install, and a synchronous guard in the same event-loop turn as the spawn re-checks the signal, prefix identity, the byte-level preimage taken before the downloads and the launcher's live canonical selection, so an earlier await can no longer hide a mutation between the final check and install. Regressions: real-child lanes prove status null on deadline kill, maxBuffer overflow and spawn ENOENT, exact exit codes, split-multibyte decode, and settlement following the child's own terminal event; an injected pending final scan refuses a signal abort, a preimage drift and a selection change with zero install calls (POSIX). * fix(codex): report windows_apply_disabled for direct Windows apply refusal A direct applyCodexCliUpdate on Windows misreported windows_inspection_deferred, the check-time code for layouts that cannot be inspected, while the plan path refuses with windows_apply_disabled. Align the guard and add the code to CODEX_CLI_JOB_REASONS so a persisted refusal is not rewritten to job_result_unknown. CodeRabbit review on #7596 comment 4365760310. * test(codex): hold Windows toolchain through async metadata resolution * test(codex): pin gated registry fixtures to x64 * ci: record measured Codex CLI apply batch weight * fix(codex): release apply lease when version verifier setup fails before spawn * test(codex): cover terminal pre-child verifier setup on every platform * fix(codex): run CLI update plan/apply hashing in a dedicated worker OpenCodex #7586: the management plan/apply paths ran synchronous full-preimage hashing, file-set walks and process scans on the HTTP event loop. Each admitted job now runs in a fresh Worker realm (cli-update-worker in WORKER_ENTRIES) while admission and the generation-fenced durable ledger stay in the parent. - per-job SAB fence PREPARING->CANCELLED/COMMITTED plus a real AbortSignal driven by a cancel message; the commit CAS is the last guard before the installer spawn, and a post-COMMITTED cancel defers - one process-wide shared selection gate covers the short commit window: the five mutable selection writers, the remote-workspace spawn and the desktop restart ladder report busy instead of racing - the run message carries the allowlisted frozen launcher snapshot; the worker rebuilds inspection deps and derives fresh selection evidence in its own realm - settlement requires a valid reply AND the worker close; the host never terminates a worker, so post-delivery loss settles ambiguous with the lease retained and live workers tracked Base: b0e2d8d * fix(codex): drain CLI update workers on Bun 1.4.4 Guard worker entry installation with Bun.isMainThread, remove message handlers for natural drain, and preserve explicit spawnWorker coverage. Add fresh-main import and real Bun child-drain regressions without forced worker termination. * test(codex): prove worker imports preserve fresh main handler * test(codex): align route planning fixture with worker dispatch * test(codex): isolate selector fixtures from runtime memo expiry * Refine Windows CLI installation checks * fix(codex): validate update paths and retain installation admission * fix(codex): recover update admission after terminal evidence * fix(codex): preserve recovery across admission and storage failures * fix(codex): preserve recovery admission across publication and home aliases * fix(codex): recognize interrupted terminal recovery writes * test(codex): use canonical homes for recovery fixtures * fix(codex): spawn no Windows npm metadata child until the toolchain closure is verified Holding node.exe and npm-cli.js does not cover npm's module closure or Node's executable dependencies. The default Windows plan now reports target_unresolved instead of running npm. The regression uses an npm entrypoint that requires a separate module and asserts it never runs. --------- Co-authored-by: JUN <243035832+lidge-jun@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
node.exeprobe is refused or its volume cannot be inspected. Those outcomes do not prove the file is absent; selecting a PATH Node could report a different toolchain from the npm command shim.Verification
bun test tests/codex-integration/codex-cli-installation-targets.test.ts— 20 passed, 0 failed, 28 assertions. On the unchanged base, the two new cases fail and 18 existing cases pass.bun run typecheck,bun run structure:check,bun run privacy:scan, andgit diff --checkpassed.bun test tests/ci-workflows/file-size-ratchet.test.ts -t repository --timeout 30000. Missing sparse-checkout files were restored from the exact HEAD first. The default 5-second run timed out locally; no test assertion or baseline was changed.62d12d1ab7f18ebbbb21836e54c01aade034d5c7, tested as merge2e24620a0b8f0623e6557cbd035eacd6df7f110cwith base815af90464e49c98f806afbe489cc5b0e753555f(14 successful jobs, 9 conditional skips). The native Windows focused suite above covers refused, unavailable, definite-not-found, and normal local-Node selection; CI does not establish native Windows execution, Windows update/apply behavior, or validation against a laterdevtip.Checklist
Summary by CodeRabbit