fix: spawn pnpm without a shell to silence Node DEP0190 deprecation w… - #52
fix: spawn pnpm without a shell to silence Node DEP0190 deprecation w…#52sebdanielsson wants to merge 1 commit into
Conversation
…arning
pnpm install and pnpm store prune were spawned with an args array and
`shell: true`, which makes Node emit a DEP0190 deprecation warning
("Passing args to a child process with shell option true can lead to
security vulnerabilities") on every run.
Spawn the pnpm this action installed by its absolute path instead —
path.join(dest, 'pnpm.exe' | 'pnpm'). Running the .exe directly on Windows
runs the actual binary rather than the .cmd batch file, which needs no
shell, so the warning is gone on every platform. `dest` is normalized to
an absolute path in getInputs so the spawned path is not affected by the
`cwd` the install runs with.
Co-authored-by: sebdanielsson <20663065+sebdanielsson@users.noreply.github.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe action now resolves ChangesDirect pnpm execution
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The action now runs the installed pnpm executable from an absolute destination path, avoiding shell spawning and the associated deprecation warning. No current merge-blocking risk is identified. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, consistent across call sites, and correctly avoid shell: true by executing the installed pnpm binary via a stable absolute path.
Pull request overview
This pull request removes shell: true from pnpm invocations (addressing Node’s DEP0190 warning) by executing the pnpm binary via an absolute path derived from the action’s configured dest directory.
Changes:
- Spawn pnpm via
path.join(inputs.dest, 'pnpm(.exe)')instead of relying onshell: true+ PATH resolution. - Normalize
destto an absolute path ingetInputs()so spawning remains correct regardless ofcwd. - Apply the same approach to both
pnpm installandpnpm store prune.
File summaries
| File | Description |
|---|---|
src/pnpm-store-prune/index.ts |
Runs pnpm store prune by absolute executable path and drops shell: true. |
src/pnpm-install/index.ts |
Runs pnpm install by absolute executable path and drops shell: true. |
src/inputs/index.ts |
Resolves dest to an absolute path to keep spawned executable paths stable. |
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking command-selection inconsistency for workflows that self-update pnpm before post-job cache pruning. Direct spawning is valid for the installed platform binaries and fixed arguments, but post-job pruning now bypasses the PATH precedence deliberately established for a self-updated pnpm. Files Needing Attention: src/pnpm-store-prune/index.ts Reviews (1): Last reviewed commit: "fix: spawn pnpm without a shell to silen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const pnpmBin = path.join(inputs.dest, process.platform === 'win32' ? 'pnpm.exe' : 'pnpm') | ||
| const { error, status } = spawnSync(pnpmBin, ['store', 'prune'], { |
There was a problem hiding this comment.
If a later workflow step runs pnpm self-update, the replacement in dest/bin takes precedence on PATH. This absolute path instead runs the original dest/pnpm during the post action, so cache cleanup may use different store-pruning behavior than the pnpm version used by the rest of the job. Preserve the prepared dest/bin precedence without reintroducing shell execution.
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Fixes #18
Every run of the action logged
(node) [DEP0190] DeprecationWarning: Passing args to a child process with shell option truebecause two call sites spawned pnpm with an args array andshell: true. All args are fixed literals, so the shell added no value — it only existed to resolve thepnpm.cmdbatch file on Windows.Changes
src/pnpm-install/index.ts— droppedshell: truefrom thepnpm installspawnsrc/pnpm-store-prune/index.ts— droppedshell: truefrom thepnpm store prunespawndist/index.js— rebuilt bundleThe executable name is now resolved by platform instead, matching how
install-runtimealready spawns pnpm shell-free:Windows needs the explicit
.cmdname because a CMD batch file cannot be spawned without a shell; on POSIX, barepnpmresolves throughPATH, whichinstall-pnpmhas already pointed at the pnpm this action installed.Summary by CodeRabbit