Skip to content

fix: replace spawn shell option with explicit shell args to avoid Node.js DEP0190 warning#1234

Merged
DennisYu07 merged 3 commits into
QwenLM:mainfrom
afarber:1115-fix-dep0190-deprecation
Jan 21, 2026
Merged

fix: replace spawn shell option with explicit shell args to avoid Node.js DEP0190 warning#1234
DennisYu07 merged 3 commits into
QwenLM:mainfrom
afarber:1115-fix-dep0190-deprecation

Conversation

@afarber

@afarber afarber commented Dec 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

TLDR

Replaces spawn(command, { shell: true }) and spawn(command, [], { shell: true }) patterns with explicit spawn(shell, ['-c', command]) to eliminate Node.js DEP0190 deprecation warning.

Dive Deeper

Node.js 20+ introduced DEP0190 deprecation warning that triggers when passing an args array to child_process.spawn() with shell: true. The warning exists because arguments are only space-concatenated, not properly escaped, which can lead to shell injection vulnerabilities.

Before:

  spawn(command, [], { shell: true })
  spawn(command, { shell: true })

After:

  const shell = isWindows ? 'cmd.exe' : 'bash';
  const shellArgs = isWindows ? ['/c', command] : ['-c', command];
  spawn(shell, shellArgs);

This pattern was already used in shellExecutionService.ts for PTY spawning, so this PR applies the same approach consistently across all spawn calls.

Files changed:

  • packages/cli/src/utils/handleAutoUpdate.ts - auto-update process spawning
  • packages/cli/src/utils/sandbox.ts - proxy process spawning (macOS seatbelt and Docker)
  • packages/core/src/services/shellExecutionService.ts - shell command execution

Reviewer Test Plan

  1. Run with Node.js 20+ and --pending-deprecation flag to verify no DEP0190 warning appears:
    NODE_OPTIONS="--pending-deprecation --trace-warnings" npm run start
  2. Execute shell commands via the CLI and verify they work correctly
  3. On Windows, verify commands execute properly with cmd.exe /c

Testing Matrix

🍏 🪟 🐧
npm run yes ❓ yes
npx ❓ ❓ ❓
Docker ❓ ❓ ❓
Podman ❓ - -
Seatbelt ❓ - -

Linked issues / bugs

Fixes #1115

github-advanced-security[bot]
github-advanced-security AI found potential problems Dec 12, 2025
View reviewed changes
Comment thread packages/cli/src/utils/sandbox.ts Outdated
Comment on lines +800 to +803
? ['/c', proxyContainerCommand]
: ['-c', proxyContainerCommand];
// CLI tool intentionally executes user-provided proxy commands in container
// codeql-disable-next-line js/shell-command-injection-from-environment

Check warning

Code scanning / CodeQL

Shell command built from environment values Medium

This shell command depends on an uncontrolled
absolute path
Loading
.
This shell command depends on an uncontrolled
absolute path
Loading
.
This shell command depends on an uncontrolled
absolute path
Loading
.
This shell command depends on an uncontrolled
absolute path
Loading
.
This shell command depends on an uncontrolled
absolute path
Loading
.
@afarber afarber mentioned this pull request Dec 12, 2025
Closed
@afarber

afarber commented Dec 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Successfully smoke tested on Ubuntu 25.04, do not see any deprecation warnings while running commands.

Ran with NODE_OPTIONS='--pending-deprecation' npm run start to verify no warnings appear.

image

@xuewenjie123

xuewenjie123 commented Dec 16, 2025

Copy link
Copy Markdown
Collaborator

@afarber thanks for your contribution！
I've been trying to reproduce the DEP0190 warning locally on the main branch (using Node.js v24 with --pending-deprecation), but I haven't been able to trigger it yet. It seems that spawn(cmd, [], { shell: true }) (with an empty args array) doesn't trigger the warning in my environment, whereas passing non-empty args does.

Did you encounter the warning in a specific scenario that I might be missing, or is this intended as a proactive fix to align with best practices? Just want to make sure I understand the context fully. Thanks again!"

@xuewenjie123 xuewenjie123 mentioned this pull request Dec 16, 2025
Closed
@afarber

afarber commented Dec 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks for testing! I also tried to reproduce on macOS with Node v25 by using

git checkout main
npm run build
NODE_OPTIONS='--pending-deprecation --trace-deprecation' npm run start

on my Macbook and issuing the prompt "Run 'git log --oneline -10'" but no warning appeared (also tried ctrl+o):

image

The reason is that the current code uses the pattern:

  spawn(commandToExecute, [], { shell: 'bash' })

The entire command (e.g., "git log --oneline -10") is passed as a single string, always with an empty args array []. DEP0190 only triggers when the args array is non-empty, like:

  spawn("git", ["log", "--oneline"], { shell: true })  // This would warn

So the current implementation accidentally avoids the warning by putting everything in the command string.

This PR is still valuable as a proactive fix because:

  1. It follows the Node.js recommended pattern: spawn('bash', ['-c', cmd])
  2. It removes the confusing empty [] parameter
  3. It aligns with the pattern already used for PTY spawning in the same file

The original issue reporter was on Windows with Node v24 - I have not tried that environment

@afarber afarber force-pushed the 1115-fix-dep0190-deprecation branch 2 times, most recently from a1ec67b to 8264897 Compare December 16, 2025 12:57
@afarber afarber force-pushed the 1115-fix-dep0190-deprecation branch from 8264897 to fa8b5a7 Compare January 17, 2026 13:29
DennisYu07
DennisYu07 approved these changes Jan 21, 2026
View reviewed changes
Comment thread packages/cli/src/utils/handleAutoUpdate.ts
);
const updateProcess = spawnFn(updateCommand, { stdio: 'pipe', shell: true });
const isWindows = os.platform() === 'win32';
const shell = isWindows ? 'cmd.exe' : 'bash';

@DennisYu07 DennisYu07 Jan 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better to use '/bin/sh' instead of 'bash' to avoid some conner case such as in Alpine it dose not have bash?

@DennisYu07 DennisYu07 merged commit fb3a95e into QwenLM:main Jan 21, 2026
15 checks passed
@xmillogx-cmd xmillogx-cmd mentioned this pull request May 15, 2026
Merged
xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026
@DennisYu07
Merge pull request QwenLM#1234 from afarber/1115-fix-dep0190-deprecation
9126909 
fix: replace spawn shell option with explicit shell args to avoid Node.js DEP0190 warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DennisYu07 DennisYu07 DennisYu07 approved these changes

@xuewenjie123 xuewenjie123 Awaiting requested review from xuewenjie123

@tanzhenxin tanzhenxin Awaiting requested review from tanzhenxin

@gwinthis gwinthis Awaiting requested review from gwinthis

@LaZzyMan LaZzyMan Awaiting requested review from LaZzyMan

@pomelo-nwu pomelo-nwu Awaiting requested review from pomelo-nwu

@Mingholy Mingholy Awaiting requested review from Mingholy

Assignees

@xuewenjie123 xuewenjie123

@DennisYu07 DennisYu07

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

(node:9184) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true

4 participants

@afarber @xuewenjie123 @github-advanced-security @DennisYu07