fix(server): use shutil.which to resolve npm for Windows compatibility#3340
fix(server): use shutil.which to resolve npm for Windows compatibility#3340deacon-mp wants to merge 2 commits into
Conversation
On Windows, npm is a batch script (npm.cmd) that subprocess cannot find without shell=True. Instead of adding shell=True (security concern per Bandit B602), use shutil.which() to resolve the full path to npm on any platform.
There was a problem hiding this comment.
Pull request overview
Improves cross-platform support for Caldera’s server-side UI build/dev flags by resolving npm via shutil.which() instead of relying on shell=True, targeting Windows compatibility and avoiding Bandit B602 concerns.
Changes:
- Add
_resolve_npm()helper to locatenpmand provide a clearer error when missing. - Update Vue dev server startup to use the resolved npm path.
- Update
--uidev/--buildnpm subprocess calls to use the resolved npm path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| npm = shutil.which("npm") | ||
| if npm is None: | ||
| raise FileNotFoundError( | ||
| "npm is not installed or not on PATH. " | ||
| "Install Node.js (https://nodejs.org/) and ensure npm is available." | ||
| ) | ||
| return npm |
|
1 similar comment
|
…which
On Windows, shutil.which("npm") resolves to npm.cmd, and .cmd files
cannot be executed directly by subprocess without shell=True. Add
shell=(sys.platform == "win32") to subprocess.run calls and use
create_subprocess_shell on Windows for the async dev server call.
There was a problem hiding this comment.
Pull request overview
Improves cross-platform support for invoking npm from server.py (notably for Windows) when using the --build and --uidev flags, aiming to avoid shell=True by resolving the npm executable path first.
Changes:
- Added
_resolve_npm()helper that usesshutil.which("npm")and raises a clear error if npm is missing. - Updated Vue dev-server startup to use resolved npm and a Windows-specific subprocess path.
- Updated
--build/--uidevnpm invocations to use the resolved npm executable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # be executed directly by create_subprocess_exec, so use shell. | ||
| cmd = subprocess.list2cmdline([npm, "run", "dev"]) | ||
| proc = await asyncio.create_subprocess_shell( | ||
| cmd, stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH, |
| subprocess.run([npm, "run", "build"], cwd=MAGMA_PATH, check=True, | ||
| shell=(sys.platform == "win32")) |
| """Resolve the full path to the npm executable. | ||
|
|
||
| On Windows, ``npm`` is a batch script (``npm.cmd``) that | ||
| ``subprocess`` cannot find without ``shell=True``. Using | ||
| ``shutil.which`` resolves the correct path on every platform. |
| subprocess.run([npm, "install"], cwd=MAGMA_PATH, check=True, | ||
| shell=(sys.platform == "win32")) | ||
| subprocess.run([npm, "run", "build"], cwd=MAGMA_PATH, check=True, | ||
| shell=(sys.platform == "win32")) |
|
we don't officially support running caldera on windows - we still want to add this? |
Summary
--buildand--uidevflags wheresubprocess.run(["npm", ...])fails becausenpmis a batch script (npm.cmd) on Windowsshutil.which("npm")to resolve the full path instead of addingshell=True(which is a security concern per Bandit B602)_resolve_npm()helper with a clear error message if npm is not installedshell=TrueTest plan
--buildflag works on Linux (npm resolves correctly)--buildflag works on Windows (npm.cmd resolves correctly)--uidevflag works on both platforms🤖 Generated with Claude Code