fix: prepend node directory to PATH for sudo OpenClaw install/uninstall#13356
fix: prepend node directory to PATH for sudo OpenClaw install/uninstall#13356
Conversation
When retrying npm install/uninstall with sudo via @expo/sudo-prompt on macOS/Linux, the root user's PATH doesn't include nvm or homebrew directories where node lives. Since npm's shebang is #!/usr/bin/env node, this causes `env: node: No such file or directory` (exit code 127). Fix by prepending npm's parent directory (which typically also contains the node binary) to PATH in the sudo command string. This is only needed on macOS/Linux; Windows npm uses .cmd shims without shebangs. Fixes #13333 Co-Authored-By: Claude Opus 4.6 <[email protected]>
GeorgeDong32
left a comment
GeorgeDong32
left a comment
There was a problem hiding this comment.
Code Review Summary
PR #13356: fix: prepend node directory to PATH for sudo OpenClaw install/uninstall
Overall: ✅ Approved
A clean, well-targeted fix for a platform-specific issue.
Changes Reviewed
- Modified
src/main/services/OpenClawService.ts - Added PATH export for sudo commands on macOS/Linux
- Properly distinguished Windows behavior (unaffected)
Quality Checks
- ✅ Lint passes
- ✅ TypeScript typecheck passes
- ✅ All 3471 tests pass
- ✅ Logic correctness verified
- ✅ Security: No command injection risks
- ✅ Code style matches project conventions
Notes
The fix correctly addresses the root cause: when sudo runs in a clean environment, it doesn't have access to the user's PATH where nvm/homebrew node installations reside. By prepending npm's parent directory to PATH, the shebang #!/usr/bin/env node can now resolve correctly.
Minor optional suggestion: Consider adding quotes around ${nodeDir} for paths with spaces, though current risk is low since npmPath is already processed.
Recommendation: Ready to merge.
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
Clean, focused fix that correctly addresses the root cause. The approach of prepending nodeDir to PATH in the sudo command string is the right call — minimal surface area, platform-aware, and doesn't touch global config.
One minor nit about the path.dirname('npm') === '.' edge case, but not blocking.
LGTM ✅
| // clean environment without user PATH) can resolve `node` via npm's shebang | ||
| // (#!/usr/bin/env node). | ||
| const nodeDir = path.dirname(npmPath) | ||
| const npmCommand = isWin |
There was a problem hiding this comment.
[nit] Edge case: when findExecutableInEnv('npm') returns null, npmPath falls back to 'npm', and path.dirname('npm') returns '.'. This would make the sudo command become:
export PATH=".:$PATH" && "npm" install -g ...Prepending . to root's PATH is a minor security anti-pattern (current directory in PATH can lead to unintended binary execution). Though practically harmless since it's a one-shot sudo command, consider adding a guard:
const nodeDir = path.dirname(npmPath)
const pathPrefix = nodeDir !== '.' ? `export PATH="${nodeDir}:$PATH" && ` : ''Not blocking — just a robustness note.
Replace `export PATH=... && command` with `PATH=... command` syntax. This scopes the PATH change to only the npm child process without polluting the shell session, and eliminates the `&&` chain that could mask errors. Co-Authored-By: Claude Opus 4.6 <[email protected]>
SiinXu
left a comment
SiinXu
left a comment
There was a problem hiding this comment.
Self-review note (non-blocking nit):
const npmPath = (await findExecutableInEnv('npm')) || 'npm'If findExecutableInEnv('npm') falls back to the literal 'npm', then path.dirname(npmPath) becomes '.', and the sudo retry command becomes:
PATH=".:$PATH" "npm" install -g ...This doesn't meaningfully help in the clean-env scenario we're trying to fix. Consider guarding the PATH prepend — only apply it when npmPath is absolute:
const nodeDir = path.dirname(npmPath)
const needsPathFix = path.isAbsolute(npmPath)
const npmCommand = isWin
? `"${npmPath}" install -g ${packageName} ${registryArg}`.trim()
: needsPathFix
? `PATH="${nodeDir}:$PATH" "${npmPath}" install -g ${packageName} ${registryArg}`.trim()
: `"${npmPath}" install -g ${packageName} ${registryArg}`.trim()This is non-blocking — when npmPath falls back to 'npm', it implies npm is already discoverable on the system PATH (since findExecutableInEnv found nothing better), so sudo would likely resolve it too. The current fix already covers the vast majority of real-world cases (nvm/Homebrew users with absolute npm paths).
Will address in a follow-up if needed.
When findExecutableInEnv('npm') falls back to the literal string 'npm',
path.dirname('npm') returns '.', making the sudo retry command
`PATH=".:$PATH" "npm" install ...` which doesn't help in a clean
sudo environment.
Now only prepend nodeDir to PATH when npmPath is an absolute path.
When it's a bare 'npm' fallback, skip the PATH fix since npm must
already be discoverable on the system PATH.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…k case" This reverts commit 1001d42.
DeJeune
added
the
ready-to-merge
5 participants
What this PR does
Before this PR:
When OpenClaw one-click install fails and retries with
sudovia@expo/sudo-prompt, the root user's PATH doesn't include nvm/homebrew directories wherenodelives. Since npm's shebang is#!/usr/bin/env node, this causesenv: node: No such file or directory(exit code 127).After this PR:
Prepend npm's parent directory to PATH in the sudo command string on macOS/Linux. Windows is unaffected (npm uses
.cmdshims without shebangs).Fixes #13333
Why we need it and why it was done in this way
N/A
Breaking changes
N/A
Special notes for your reviewer
install(): PrependnodeDirto PATH in sudo retry commanduninstall(): Same fix for sudo retry commandChecklist
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note