fix(vscode): use shell mode for spawn on Windows to detect npm correctly

[aalhadxx]

Problem

On Windows, npm is shipped as npm.cmd. Node's spawn without shell: true fails to execute batch files, causing the VS Code extension to falsely report 'npm not detected' even when npm is installed and works in the terminal.

Reproduction

On Windows 11 with Node.js and npm installed, open the Open Code Review configuration page in VS Code. Node.js is detected but npm is marked as missing.

Fix

Apply shell: process.platform === 'win32' to probeCommand (used for environment detection: node, npm, ocr version checks).

The install() method already used this flag. runRaw was intentionally excluded because its args may include user-controlled values (opts.from, opts.to, opts.customPrompt), and shell: true with user input carries command-injection risk.

Test

Added CliService probe shell option tests that verify:

• On Windows (win32), spawn receives shell: true
• On non-Windows (linux), spawn receives shell: false

Fixes #453

[CLAassistant]

All committers have signed the CLA.

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

[github-actions]

[security · medium]
Security consideration: Using shell: true on Windows causes Node.js to concatenate the command and arguments into a single string passed to cmd.exe. In runRaw, arguments may include user-controlled values (e.g., opts.customPrompt from buildReviewArgs). While Node.js applies basic quoting for array arguments, cmd.exe metacharacters like %, ^, or ! can still potentially cause unexpected behavior or injection.

Consider sanitizing user-supplied argument values before passing them to spawn with shell: true, or alternatively use cross-spawn / execa which handle Windows .cmd resolution without requiring shell: true.

[lizhengfeng101]

Code Review

Overview

This PR fixes a real Windows issue where npm.cmd can't be spawned without shell: true, causing the VS Code extension to falsely report "npm not detected." The fix is straightforward and the test intent is correct, but there are two blocking issues.

---

Blocking Issues

1. jest.mock('child_process') breaks existing tests in the same file

The added jest.mock('child_process') is a module-level mock that affects the entire test file. The existing CliService.isAvailable, CliService.runRaw, and CliService.testConnection tests all rely on real spawn to execute node commands. With this mock in place, those tests will all fail.

Fix options:

• Move the new tests to a separate file (e.g. CliService.shell.test.ts) where child_process can be mocked in isolation
• Or use jest.spyOn with per-test setup/teardown instead of a module-level mock

2. PR description claims runRaw is fixed, but the diff only changes probeCommand

The PR body states:

Apply shell: process.platform === 'win32' to the spawn calls in:

• probeCommand
• runRaw

But the diff only modifies probeCommand (line 35). runRaw (line 111) is untouched. Either add the missing fix to runRaw or correct the PR description.

---

Minor Issues

3. headRefName is main

The PR is submitted from the fork's main branch rather than a feature branch. Not blocking, but it prevents parallel development on the same fork.

4. Non-Windows assertion could be more precise

expect(spawn).toHaveBeenCalledWith(
  'npm', ['--version'],
  expect.not.objectContaining({ shell: true }),
);

Since shell: process.platform === 'win32' evaluates to { shell: false } on Linux, asserting expect.objectContaining({ shell: false }) would be more explicit about the expected behavior.

5. Security note on shell: true

shell: true can introduce command injection if args contain unescaped shell metacharacters. In this case the risk is low (args is hardcoded ['--version']), but a brief inline comment explaining why it's safe would help future readers.

---

Summary

Aspect	Verdict
Correctness	jest.mock breaks existing tests; runRaw fix is missing
Scope	Focused, solves a real Windows problem
Tests	Logic is correct but mock placement is wrong
Security	Low risk — args are hardcoded

Recommendation: Fix the two blocking issues (isolate the mock, address runRaw discrepancy), then this is good to merge.

[aalhadxx]

Hi @lizhengfeng101, thanks for the detailed review. All blocking issues have been addressed in the latest push:

1. Mock isolation — Replaced the module-level with per-test that is restored in . Existing / / tests now continue to use the real spawn.

2. PR description — Updated to clarify that only is fixed. was intentionally excluded after security feedback (args may contain user-controlled values); the original bug (Windows 环境下vs code插件检测误报"未检测到 npm"（spawn 未使用 shell 模式） #453) was specifically about environment detection failing on Windows.

Minor fixes also applied:

• Non-Windows assertion now uses for precision.
• Added an inline comment above explaining why it's safe (hardcoded args, no user input).

Please take another look when you have a moment.

[aalhadxx]

Hi @lizhengfeng101, thanks for the detailed review. All blocking issues have been addressed in the latest push:

1. Mock isolation -- Replaced the module-level jest.mock('child_process') with per-test jest.spyOn(require('child_process'), 'spawn') that is restored in afterEach. Existing isAvailable, runRaw, and testConnection tests now continue to use the real spawn.

2. PR description -- Updated to clarify that only probeCommand is fixed. runRaw was intentionally excluded after security feedback (args may contain user-controlled values); the original bug (Windows 环境下vs code插件检测误报"未检测到 npm"（spawn 未使用 shell 模式） #453) was specifically about environment detection failing on Windows.

Minor fixes also applied:

• Non-Windows assertion now uses expect.objectContaining({ shell: false }) for precision.
• Added an inline comment above shell: true explaining why it is safe (hardcoded --version args, no user input).

Please take another look when you have a moment.