fix(vscode): use shell mode for spawn on Windows to detect npm correctly - #626
fix(vscode): use shell mode for spawn on Windows to detect npm correctly#626aalhadxx wants to merge 5 commits into
Conversation
On Windows, npm is shipped as npm.cmd. Without shell: true, Node's spawn fails to execute batch files, causing the VS Code extension to falsely report 'npm not detected' even when it is installed. The install() method already used this flag; apply the same fix to probeCommand (environment detection) and runRaw (CLI execution). Fixes alibaba#453
Reverts shell: true in runRaw to address security feedback: runRaw args may include user-controlled values (opts.from, opts.to, opts.customPrompt), and shell mode could interpret cmd.exe metacharacters. Keep shell: true only in probeCommand where args are known-safe (--version). The original bug (alibaba#453) was specifically about environment detection failing on Windows, so this targeted fix is sufficient.
- Replace module-level jest.mock('child_process') with per-test jest.spyOn
so existing isAvailable / runRaw / testConnection tests continue to
use the real spawn.
- Add inline comment explaining why shell: true is safe in probeCommand
(hardcoded --version args, no user input).
- Use explicit expect.objectContaining({ shell: false }) for non-Windows
assertion instead of expect.not.objectContaining({ shell: true }).
- Remove unused import of spawn from child_process - Add eslint-disable comment for require() in test (jest.spyOn on non-configurable CommonJS export) - Replace Function type with explicit (code: number) => void callback type
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| - name: Create GitHub Release | ||
| uses: softprops/action-gh-release@v3 | ||
| uses: softprops/action-gh-release@v2 |
There was a problem hiding this comment.
[security · medium]
Third-party actions should be pinned to a full commit SHA rather than a mutable tag. Tags can be moved or hijacked, posing a supply-chain risk. Consider pinning to the specific commit SHA for softprops/action-gh-release@v2 (e.g., uses: softprops/action-gh-release@<full-sha>). First-party actions/* pinned to major version tags like @v4 / @v2 are acceptable.
| # Content-validation guardrails, kept out of ci.yml (which is build/test/lint): | ||
| # - blocking: all README*.md translations share the same ## section structure. | ||
| # - non-blocking: warn when a docs/en page changes without its zh/ja/ru counterpart. | ||
| # - non-blocking: warn when a docs/en page changes without its zh/ja counterpart. |
There was a problem hiding this comment.
[documentation · medium]
The comments here and on line 43 were updated to say "zh/ja" (removing Russian), but the underlying script scripts/github-actions/check-translation-sync.js was not updated in this PR. It still defines DOCS_LOCALES = ["zh", "ja", "ru"] (line 46) and references "zh/ja/ru" in its own comments (lines 19, 44, 166, 179). This creates a mismatch between what the workflow comments claim and what the check actually does.
Either:
- Also update the script to remove
"ru"fromDOCS_LOCALESand update its comments, or - Revert these comment changes to keep them consistent with the current script behavior.
| // shell: true is safe here because args are hardcoded ['--version'] — no user input. | ||
| const proc = spawn(resolveBin(bin), args, { env: getShellEnv(), shell: process.platform === 'win32' }); |
There was a problem hiding this comment.
[maintainability · low]
Minor: The comment states "args are hardcoded ['--version']" to justify shell: true, but args is actually a parameter of probeCommand. While all current callers do pass ['--version'], a future caller could pass user-controlled args, making the security justification stale. Consider either:
- Removing the
argsparameter and hardcoding['--version']inside the method (since it's only used for version probing), or - Adjusting the comment to say "all current callers pass hardcoded args" so future developers know to verify safety when adding new call sites.
…y justification Address OpenCodeReview bot feedback: the previous implementation passed args as a parameter while claiming they were hardcoded. Now probeCommand only probes --version, making the shell: true usage unambiguously safe. Updates call sites in checkEnvironment and test assertions accordingly.
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
@aalhadxx The core fix (adding shell: process.platform === 'win32' to probeCommand) is correct and well-scoped — no security concerns, good test coverage. However, this PR has significant unrelated changes mixed in that block merging:
1. GitHub Actions version downgrades (breaking)
All workflow files downgrade actions from their current versions on main (e.g. checkout@v7 → @v4, cache@v6 → @v4, action-gh-release@v3 → @v2, attest-build-provenance@v4 → @v2). These are regressions, likely because your branch is based on a stale commit.
2. package-lock.json added (+10,241 lines)
This project uses yarn. A package-lock.json should not be committed.
3. translation-sync.yml comment changes
Removing "ru" from translation sync comments is unrelated to this fix.
4. yarn.lock changes
Unrelated dependency churn.
Requested action: Please rebase onto the latest main, drop all unrelated changes, and keep only the CliService fix + tests. A clean way to do this:
git fetch origin main
git checkout -b fix/vscode-windows-shell origin/main
git cherry-pick <your-commit-with-CliService-changes>The actual fix is good — just needs a clean branch. Thanks!
|
Closing to replace with a cleaner branch that drops all unrelated lockfile and workflow changes. New PR incoming. |
2 participants
Fixes #453
Problem
On Windows, child_process.spawn cannot find npm.cmd / npx.cmd without shell: true, causing the VS Code extension to falsely report npm not detected and disable CLI features.
Fix
Add shell: process.platform === 'win32' only to probeCommand (used for environment detection with hardcoded --version args). runRaw is intentionally left unchanged — it receives user-controlled args and must not use shell: true for security.
Tests
Prior art
Supersedes #584 (closed due to merge conflicts in lock files).