fix: resolve Node outside minimal hook PATH - #737
fscfede-beep wants to merge 13 commits into
Conversation
![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: 8f09814f4b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a17c3dafe4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc0ffadc0e
ℹ️ 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".
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
There is a version-manager edge here when node is absent from PATH and several nvm/fnm/asdf/mise installations exist. The glob loop selects the first supported executable in lexical path order, not the version manager's active/default runtime. That selected directory is then prepended to PATH, so a codex shim found later in another installation can still resolve #!/usr/bin/env node to the earlier, unrelated Node version. A machine with nvm v20 and v24 installed can therefore run the companion and Codex under a different runtime than the user's configured default. Resolving the manager's default/current alias first, or selecting the highest supported semantic version, would avoid that mismatch. A regression with two supported nvm installs and codex present only in the newer one would make the behavior explicit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef58bf1597
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1338edef04
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6855f3992
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64dcef9272
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a420fc0dfb
ℹ️ 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".
|
Review reconciliation update for the multi-version/toolchain concern: addressed in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ef6b735bc
ℹ️ 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".
|
@codex review Please re-review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11007e405a
ℹ️ 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".
|
@codex review Current head |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@openai/developer-experience — contributor-side work on this PR is complete at exact head |
Upstream PR openai#737 (fscfede-beep), one conflict resolved. Hooks ran `node` directly, so a Node installed through nvm, fnm, asdf, mise, Volta or Homebrew was invisible under the minimal PATH hooks get, and the plugin looked broken for anyone who does not have a system-wide node. The hooks and the node-invoking commands now go through scripts/run-node.sh, which finds a supported (>=18.18) toolchain, prefers one that ships codex next to it, and restores the npm global prefix. Conflict: plugins/codex/hooks/hooks.json, SessionEnd. The launcher command is taken from the PR; the timeout stays at this fork's 30s rather than reverting to upstream's 5s, since the bounded teardown imported earlier needs it. One fix beyond the PR: find_managed_node() listed /home/linuxbrew, /opt/homebrew, /usr/local/bin and /opt/local/bin BEFORE the version-manager roots, so on any machine carrying /usr/local/bin/node (Debian and Ubuntu packages, CI images, this repo's own container) the system install shadowed every managed toolchain — the precise failure the PR set out to fix. Four of its own tests caught it here and passed only where no system node exists. The managed roots now come first; HOMEBREW_PREFIX keeps its place ahead of them because it is configured rather than guessed. Verified: bash -n on run-node.sh; the new tests/node-launcher.test.mjs 14/14; full npm test 254/254; npm run check-version passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Adds openai#731, openai#737, openai#747 and openai#763 to "Differences From Upstream", and notes in Requirements that Node no longer has to be on the system PATH now that the hooks go through scripts/run-node.sh (including CODEX_COMPANION_NODE for pinning one). Verified: full npm test 254/254. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
|
Tried this out on a fork and hit something the PR's own tests catch: for candidate in \
"$homebrew_node" /home/linuxbrew/.linuxbrew/bin/node /opt/homebrew/bin/node /usr/local/bin/node /opt/local/bin/node \
"$home/.volta/bin/node" \
"$nvm_dir"/versions/node/*/bin/node \
...
Worth noting why those four and not the rest: the first pass runs with for candidate in \
- "$homebrew_node" /home/linuxbrew/.linuxbrew/bin/node /opt/homebrew/bin/node /usr/local/bin/node /opt/local/bin/node \
+ "$homebrew_node" \
"$home/.volta/bin/node" \
"$nvm_dir"/versions/node/*/bin/node \
"$fnm_dir"/node-versions/*/installation/bin/node \
"$asdf_dir"/installs/nodejs/*/bin/node \
- "$mise_dir"/installs/node/*/bin/node; do
+ "$mise_dir"/installs/node/*/bin/node \
+ /home/linuxbrew/.linuxbrew/bin/node /opt/homebrew/bin/node /usr/local/bin/node /opt/local/bin/node; do
|
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked current e116d57b. The fallback still selects managed Node installations in a fixed filesystem order rather than resolving the active version manager/runtime. With more than one manager installed and a minimal hook PATH, find_managed_node can choose Homebrew/Volta/nvm/asdf/mise based on this hard-coded order rather than the project/user-selected runtime, changing the Node version used by the hook. Please make the fallback honor the active manager/version (or an explicit configured Node) and add a multi-manager regression that proves the selected runtime is preserved.
The review on openai#737 asks for a multi-manager regression alongside the ordering fix. Two tests, with every manager root populated at once (nvm, fnm, asdf, mise): - "prefers a managed toolchain over a system install with several managers present" is the regression for the precedence bug itself. It discriminates where a system node exists: putting /usr/local/bin and the Homebrew paths back ahead of the manager roots fails it, along with the four upstream tests that caught the bug originally. It deliberately does not pin *which* manager wins — the launcher cannot tell which one the project or user selected, since it reads no .nvmrc, .tool-versions or mise config, so asserting one would freeze an arbitrary order as if it were a guarantee. - "lets CODEX_COMPANION_NODE override every installed manager" pins the one way a specific runtime can be selected today, which is the honest answer to the reviewer's "preserve the selected runtime" until the launcher learns to read the active manager. Verified: the first test fails with the pre-fix ordering restored and passes with it in place; full npm test 304/304; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
3 participants
Summary
Fixes #105.
Claude Code hooks and deterministic
!-pattern commands can run with a minimal shellPATHthat does not expose a normally installed Node or Codex executable. This PR adds a portable POSIX launcher and routes the affected plugin surfaces through it.The current branch covers the compatibility issues found during review:
PATH;/codex:transferthrough the same portable launcher as the other deterministic commands;NVM_SYMLINK,VOLTA_HOME, andLOCALAPPDATA;NPM_CONFIG_PREFIXornpm prefix -g;NVM_DIR,FNM_DIR,ASDF_DATA_DIR, andMISE_DATA_DIRroots for both Node discovery and PATH enrichment;HOMEBREW_PREFIXand the standard Linuxbrew/home/linuxbrew/.linuxbrewprefix, in addition to macOS/Homebrew fallback locations;CODEX_COMPANION_NODEbefore returning it, so downstream directory derivation exposes the selected toolchain's adjacent npm/Codex executables instead of falling back to.;Validation
Fresh validation on Windows 11 / Git Bash at head
e116d57b1d5fa61a3bfa02ab6aa9595335b84416:ASDF_DATA_DIR/MISE_DATA_DIRregression: RED before fix, GREEN after;PATHfell back to.), GREEN after;HOMEBREW_PREFIXregression: RED before fix with exit 127, GREEN after;bash -n plugins/codex/scripts/run-node.sh: PASS;git diff --check: PASS.Upstream Pull Request CI is checked per head. If GitHub reports
action_requiredwith zero jobs, that is treated as an external workflow-approval/action gate, not a CI test failure.Current scope
The implementation remains limited to launcher/routing compatibility and regression coverage; it does not alter Codex task/review semantics.