fix: persist resolved Windows pnpm bin path - #33
amishabenramani wants to merge 1 commit into
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? |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe Windows path extender now writes the resolved directory directly to ChangesWindows PATH entries
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Merge Risk: ⚪ Minimal · up to The Windows PATH entry now uses the resolved pnpm directory, with tests and documentation reflecting that behavior. No actionable merge-blocking risk is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 checks the Windows trail Comment |
|
Thanks for digging into pnpm/pnpm#5283. I'm closing this together with its counterpart pnpm/pnpm#16206, because we want to keep the
If Written by an agent (Claude Code, claude-opus-5-5). |
Summary
On Windows, persist the resolved pnpm directory in the user
Pathinstead of a nested%PNPM_HOME%reference.Windows does not reliably expand one user environment variable from another user
Pathentry. With current pnpm 11.28.0 and 12.7.0,pnpm setupwrites%PNPM_HOME%\binto the userPath. A fresh Windows environment keeps that entry literal, so pnpm's global-bin validation does not see the configured bin directory.This component still creates the
PNPM_HOMEenvironment variable. It only changes the persisted WindowsPathentry to the normalized directory itself.Refs pnpm/pnpm#5283.
Native Windows evidence
On Windows 11 x64:
pnpm setupwith pnpm 11.28.0 wrotePNPM_HOME=E:\pnpm-5283-repro\v11\homeandPath=%PNPM_HOME%\bin;....%PNPM_HOME%\binentry rather than the resolved directory.Regression evidence
A focused test against upstream
pnpm/componentsmain failed as expected:The same focused test against this branch passes.
The existing path-extender specs were updated for first-time setup, subdirectory setup, an already-set
PNPM_HOME, and forced setup. The component docs now show the resolved path behavior.Related work
#32 fixes a different Windows registry issue in the same source and spec files. This PR is intentionally separate because it addresses pnpm/pnpm#5283. If #32 merges first, this branch may need a trivial rebase due to the shared lines.
Written by an agent (ChatGPT, GPT-5.6 Sol).
Summary by CodeRabbit
Pathentries now use the resolvedPNPM_HOMEdirectory directly, including when a subdirectory is configured orPNPM_HOMEis already set.Path.