Detect Codex installs managed by pnpm - #31503
Conversation
| { | ||
| "bun": "CODEX_MANAGED_BY_BUN", | ||
| "pnpm": "CODEX_MANAGED_BY_PNPM", | ||
| }[detectPackageManager()] ?? "CODEX_MANAGED_BY_NPM"; |
There was a problem hiding this comment.
@codex is there a better way to do this that's less ugly?
There was a problem hiding this comment.
Hahaha. We can do a nested ternary?
| // pnpm records installation metadata in node_modules/.modules.yaml. Search | ||
| // the canonical package root and lexical entrypoint because either can be |
There was a problem hiding this comment.
@codex i'm a bit worried about this while(true) and traversal through a potentially deepish directory, is there no better way to distinguish pnpm from npm?
There was a problem hiding this comment.
Summary
- Replaced the unbounded
while (true)parent-directory walk in pnpm detection with a bounded scan ofnode_modulesroots derived from the resolved Codex package paths. codex-cli/bin/codex.jsL123-L140 - Kept the pnpm ownership guard intact by requiring
.modules.yamland verifying thatnode_modules/@openai/codexresolves to the current Codex package root before returningpnpm. codex-cli/bin/codex.jsL142-L156 - Committed the follow-up change as
68aa389a314224d0697958b8150ec689b456ea83and created the follow-up PR titled “Bound pnpm owner detection in CLI shim.”
Testing
- ✅
node --check codex-cli/bin/codex.js && prettier --check codex-cli/bin/codex.js && git diff --check ⚠️ cd codex-rs && just fmt(failed due environment/tooling limits: missingdotslash, plus Python formatter dependencies could not be downloaded through the proxy)
There was a problem hiding this comment.
Yeah, the traversal was already bounded by the filesystem root, but I rewrote it to make that clearer.
We still need to inspect the parent directories since pnpm’s global isolated layout places the owning node_modules several levels above the package root.
| CODEX_MANAGED_PACKAGE_ROOT: realpathSync(path.join(__dirname, "..")), | ||
| CODEX_MANAGED_PACKAGE_ROOT: codexPackageRoot, | ||
| }; | ||
| delete env.CODEX_MANAGED_BY_NPM; |
There was a problem hiding this comment.
Why do we need to delete the env variables newly?
There was a problem hiding this comment.
This is an existing bug on main (npm-managed Codex starts a shell, that shell invokes a Bun-managed Codex but inherits CODEX_MANAGED_BY_NPM=1). This PR makes it a bit worse since we now have even more combinations here. But it isn't strictly necessary, I can remove or carve into a separate PR if preferred.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 419307d38f
ℹ️ 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".
| pub fn from_exe( | ||
| is_macos: bool, | ||
| current_exe: Option<&Path>, | ||
| managed_by_npm: bool, | ||
| managed_by_bun: bool, | ||
| method_override: Option<InstallMethod>, | ||
| ) -> Self { |
There was a problem hiding this comment.
This changes the public codex_install_context::InstallContext::from_exe API from the existing npm/bun boolean parameters to Option<InstallMethod>, so any out-of-tree caller of this crate will fail to compile even though the change only needs a pnpm override internally. The commit notes that the existing public signature should be preserved; keep this method compatible and route the new override through an added/private helper instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Is it important to follow semver here and avoid breakages on a function like this? Do we care about that?)
There was a problem hiding this comment.
We don't care about this as long as the whole thing compiles
419307d to
23aa44d
Compare
Why
The Codex JavaScript shim currently distinguishes npm and Bun installs, but a global pnpm install falls back to npm. That causes the native CLI,
codex doctor, and update flows to report or run npm commands even though pnpm owns the installation. pnpm also allows its global package and bin directories to differ, soPNPM_HOMEdoes not reliably identify the package owner.What changed
node_modules/.modules.yamlmetadata from the launched JavaScript entrypointCODEX_MANAGED_BY_PNPMmarker into the native CLIInstallContext::from_exesignaturepnpm add -g @openai/codexfor pnpm-managed installs and snapshot the rendered TUI update noticeValidated with the 9-test install-context suite, the focused pnpm TUI snapshot test, and Node's syntax check for the launcher.
Closes #10294.