Skip to content

fix(codex): keep npm Node cache within inspection lifetime - #7562

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/cli-update-npm-cache-20261023
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/cli-update-npm-cache-20261023

Conversation

@luvs01

@luvs01 luvs01 commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Keep Node's compile cache inside the temporary directory owned by Codex CLI update metadata inspection. npm can enable this cache itself after the ambient NODE_COMPILE_CACHE value has been removed, leaving files in the user's temporary directory after inspection finishes.
  • Assign an absolute cache path inside the existing isolation directory and remove it with the rest of the inspection state. The public API and update/install behavior stay unchanged.
  • Add a regression that starts real npm through the existing resolver spawn seam and checks the cache lifetime, empty temporary/home directories, and cleanup. Update the isolation contract documentation.

Verification

  • Bun 1.4.3 on Windows: the two focused regressions failed before the fix and passed afterward.
  • bun test tests/codex-integration/codex-cli-update-plan.test.ts: 54 passed, 1 POSIX-only test skipped, 0 failed.
  • TypeScript --noEmit, privacy scan, structure check, file-size ratchet, and git diff --check: passed.
  • The regression uses real npm startup with fixed --version --offline arguments; registry replies remain synthetic. It verifies cache output and cleanup, not registry authenticity or a complete installation.
  • The full suite was not repeated locally because several isolated worktrees share this host. Focused behavior and static checks passed. No installed CLI, active proxy, credentials, or user configuration were changed.
  • Upstream CI completed successfully at c129c1e164a9b97030f127182d084befda2525c7: all four Ubuntu test shards and the aggregate check passed (14 successful jobs, 9 conditional skips). The macOS and Windows full test matrices were not run.

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

  • Bug Fixes
    • The CLI update process now keeps Node’s compile cache within its isolated temporary workspace, including when an external cache location is configured. This helps prevent update-related cache files from being left in other directories.
    • The update process continues to clean up its temporary files on a best-effort basis.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between ec8b8cf and c129c1e.

📒 Files selected for processing: 3
  • src/codex/cli-update-plan.ts
  • structure/codex-cli-update.md
  • tests/codex-integration/codex-cli-update-plan.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The npm update environment now sets NODE_COMPILE_CACHE to a directory inside its owned temporary root. Integration tests check this setting and verify that npm startup does not leave cache files in the ambient cache path, temporary directory, or home directory.

Changes: npm cache isolation

Layer / File(s) Summary
Redirect and verify Node compile cache
src/codex/cli-update-plan.ts, tests/codex-integration/codex-cli-update-plan.test.ts, structure/codex-cli-update.md
At lines 318–322, createNpmConfigIsolation sets NODE_COMPILE_CACHE to node-compile-cache inside the isolated root. Tests check the environment setting and run npm with an ambient compile-cache path to verify cleanup. The documentation describes the npm and Node cache isolation.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c129c

No actionable merge-blocking risk is identified; normal checks can proceed.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: keeping npm's Node compile cache within the Codex inspection lifetime. It matches the implementation, tests, documentation, and stated object…
Docstring Coverage Passed 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 Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title clearly and concisely describes the main change: keeping npm's Node compile cache within the Codex inspection lifetime. It matches the implementation, tests, documentation, and stated objectives.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@luvs01
luvs01 marked this pull request as ready for review October 22, 2026 22:25
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

luvs01 commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #7585, the upstream-hosted parent for the CLI-update stack. All three-file changes from c129c1e164a9b97030f127182d084befda2525c7 are preserved at d5e5b4158a713c4bf805f50826b24cf2698a942b (55 additions, 5 deletions; identical stable patch ID 699d364878085768767b6302b6c0ad7c2b813b27). CI for the new head passed. The original branch and PR history are retained.

@luvs01 luvs01 closed this Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant