Conversation
POSIX echo can interpret backslash escapes and corrupt Windows-form paths in the generated shim header (pnpm/pnpm#14867). Print $link with printf '%s\n' so sed still converts backslashes. The Rust/pacquet copy of this header was fixed in pnpm/pnpm#14878. Co-authored-by: Bennett Payoyo <Yahiro025@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: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
Update sh shim generation src/index.ts |
The generated sh shim uses printf to pass $link to sed for basedir conversion. |
Validate generated shims test/e2e.test.js, test/test.js, test/*.snapshot |
POSIX tests verify the command and normalize C:\node_modules\.bin\tsc to C:/node_modules/.bin/tsc. Shim snapshots record the updated command across cases. |
Priority: ⬇️ Low
Estimated code review effort: 2 (Simple) | ~10 minutes
Suggested reviewers: zkochan
Merge Risk: ⚪ Minimal · up to 6f43d
This change addresses the reported POSIX path-handling issue without introducing a concrete merge-blocking risk.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: replacing echo with printf for POSIX path normalization. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (2 skipped: 2 … |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 checks the shim at night
printf keeps each backslash right
Sed turns the path with care
Snapshots show the change laid bare
The little paths now hop just fine
Comment @coderabbitai help to get the list of available commands.
Summary
Fixes the POSIX bin shim header so Windows-form paths keep their backslashes until
sedconverts them.Related: pnpm/pnpm#14867. POSIX
echocan interpret\n/\t/ etc. beforesedruns, so a path likeC:\node_modules\.bin\tscbecomes corrupted on dash and macOS/bin/sh. pnpm 11 generates these shims via@zkochan/cmd-shim.The generated header now uses:
basedir=$(printf '%s\n' "$link" | command -p sed -e 's,\\,/,g')instead of
echo "$link". The commented documentation template above the live JS string is updated to match.The Rust/pacquet copy of this header was already fixed in pnpm/pnpm#14878. This is the remaining
@zkochan/cmd-shim/ pnpm 11 path.Test plan
npx tsc --build&&node --test test/test.js test/e2e.test.js(68 passed on Linux;/bin/shis dash)printf '%s\n' "$link"and notecho "$link"/bin/shturnsC:\node_modules\.bin\tscintoC:/node_modules/.bin/tscwithout injecting a newline or tabAI disclosure
I used Cursor to help draft the fix and tests. I reviewed the diff and verified the regression coverage before opening this PR.
Summary by CodeRabbit
Bug Fixes
\nand\tfrom being interpreted incorrectly.Tests