fix: avoid deprecated shell spawning for pnpm commands - #52
Conversation
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 Suggested reviewers: 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. 🚥 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. A rabbit found a path made bright 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.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@zkochan hi! Sorry for pinging, just wanted to check in if you've seen this and if the changes look sound. |
…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>
zkochan
changed the title
8dd17f0 to
0b7e77a
Compare
|
|
||
| // A native executable named pnpm proves direct spawning works even when the | ||
| // destination contains spaces or shell metacharacters. Node consumes the | ||
| // fixture named "install" as its script, then passes through the install flags. |
There was a problem hiding this comment.
This comment restates how the native executable fixture and its arguments work instead of letting the test structure express that intent. The same pattern appears in the PATH setup at line 84 and in src/pnpm-store-prune/index.ts at lines 13–15. This violates the repository directive that comments must not narrate code, so the narration must be removed or the surrounding names and structure made self-explanatory before merging.
Context Used: Comments and docs in code are suspicious. Is test coverage not sufficient the reason why a comment was added? Comments should not replace tests. Comments should also not narrate code. Is the code hard to understand? Then it should be refactored to ma... (source)
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!
Fixes #18.
Automatic installs and post-job store pruning emitted Node's DEP0190 warning because they passed arguments with
shell: true.Installs now invoke the action's downloaded native
pnpm/pnpm.exedirectly. The destination is resolved to an absolute path so relativedestvalues keep working when the project is in a different working directory.Pruning uses
@actions/execto preserve PATH precedence, including the shim created by a laterpnpm self-update, and to support Windows.cmdshims without the deprecated spawn options. The post action awaits pruning before saving the cache; pruning errors remain warnings.Validation:
--throw-deprecation.