fix(server): add shell=True to npm subprocess calls for Windows compa… #3194
Conversation
…tibility fix(server): add shell=True to npm subprocess calls for Windows compatibility
There was a problem hiding this comment.
Pull Request Overview
This PR fixes Windows compatibility issues with npm subprocess calls by adding the shell=True parameter to all npm-related subprocess invocations in the server.py file.
- Adds
shell=Trueparameter to npm subprocess calls to resolve Windows FileNotFoundError issues - Maintains backward compatibility with Unix-like systems where the parameter is harmless
- Ensures cross-platform functionality for npm command execution
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| async def start_vue_dev_server(): | ||
| await asyncio.create_subprocess_shell( | ||
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH | ||
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH, shell=True |
There was a problem hiding this comment.
Using shell=True with asyncio.create_subprocess_shell is redundant since create_subprocess_shell already executes commands through the shell by default. Consider using asyncio.create_subprocess_exec with the command split into arguments for better security and consistency with the other subprocess calls.
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH, shell=True | |
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH |
Copilot uses AI. Check for mistakes.
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| logging.info("VueJS front-end build complete.") | ||
| app_svc.application.on_response_prepare.append(enable_cors) | ||
|
|
||
| if args.build: | ||
| if len(os.listdir(MAGMA_PATH)) > 0: | ||
| logging.info("Building VueJS front-end.") | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) |
There was a problem hiding this comment.
Using shell=True with a list of arguments can lead to unexpected behavior and potential security issues. When using shell=True, the command should be passed as a string, not a list. Either change to string format or remove shell=True and handle Windows compatibility differently.
Copilot uses AI. Check for mistakes.
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| logging.info("VueJS front-end build complete.") | ||
| app_svc.application.on_response_prepare.append(enable_cors) | ||
|
|
||
| if args.build: | ||
| if len(os.listdir(MAGMA_PATH)) > 0: | ||
| logging.info("Building VueJS front-end.") | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) |
There was a problem hiding this comment.
Using shell=True with a list of arguments can lead to unexpected behavior and potential security issues. When using shell=True, the command should be passed as a string, not a list. Either change to string format or remove shell=True and handle Windows compatibility differently.
Copilot uses AI. Check for mistakes.
|
2 participants
…tibility
fix(server): add shell=True to npm subprocess calls for Windows compatibility
Description
Description:
This pull request introduces a small but essential fix to ensure that the Caldera server can successfully invoke npm commands on Windows environments.
📌 Problem
On Windows, subprocess.Popen() or subprocess.run() calls that execute npm commands without shell=True fail with a FileNotFoundError, even when npm is available in the system's PATH. This is due to how Windows handles executable resolution without a shell context.
🔧 Changes Made
Added shell=True to all subprocess calls that include npm in server.py.
No logic or behavior change on Unix-like systems (Linux/macOS), where this argument is harmless.
✅ Tested On
✅ Windows 10/11: npm commands now execute correctly
✅ Linux (Ubuntu 22.04): no side effects observed
🛡️ Impact
This change improves cross-platform compatibility, especially for Windows users who want to contribute to or extend Caldera without having to patch the code manually.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: