Skip to content
This repository was archived by the owner on Sep 26, 2026. It is now read-only.

fix: persist resolved Windows pnpm bin path - #33

Closed
amishabenramani wants to merge 1 commit into
pnpm:mainfrom
amishabenramani:fix/windows-user-pnpm-home-path
Closed

amishabenramani wants to merge 1 commit into
pnpm:mainfrom
amishabenramani:fix/windows-user-pnpm-home-path

Conversation

@amishabenramani

@amishabenramani amishabenramani commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

On Windows, persist the resolved pnpm directory in the user Path instead of a nested %PNPM_HOME% reference.

Windows does not reliably expand one user environment variable from another user Path entry. With current pnpm 11.28.0 and 12.7.0, pnpm setup writes %PNPM_HOME%\bin to the user Path. 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_HOME environment variable. It only changes the persisted Windows Path entry to the normalized directory itself.

Refs pnpm/pnpm#5283.

Native Windows evidence

On Windows 11 x64:

  1. pnpm setup with pnpm 11.28.0 wrote PNPM_HOME=E:\pnpm-5283-repro\v11\home and Path=%PNPM_HOME%\bin;....
  2. A fresh Windows environment contained the literal %PNPM_HOME%\bin entry rather than the resolved directory.
  3. The same persistence behavior reproduces with pnpm 12.7.0.

Regression evidence

A focused test against upstream pnpm/components main failed as expected:

Expected: C:\pnpm\bin;C:\Windows
Received: %PNPM_HOME%\bin;C:\Windows

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

  • Bug Fixes
    • Windows Path entries now use the resolved PNPM_HOME directory directly, including when a subdirectory is configured or PNPM_HOME is already set.
  • Documentation
    • Updated the Windows environment-path example to show the resolved directory in Path.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: bd27f537-99b9-42a6-a280-d50d1990c913

📥 Commits

Reviewing files that changed from the base of the PR and between a9b32aa and f888602.

📒 Files selected for processing (3)
  • os/env/path-extender-windows/path-extender-windows.docs.mdx
  • os/env/path-extender-windows/path-extender-windows.spec.ts
  • os/env/path-extender-windows/path-extender-windows.ts

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)
os/env/path-extender-windows/path-extender-windows.ts (1)

74-75: LGTM!

os/env/path-extender-windows/path-extender-windows.spec.ts (1)

141-141: LGTM!

Also applies to: 146-146, 195-195, 198-198, 336-336, 340-340, 421-421

os/env/path-extender-windows/path-extender-windows.docs.mdx (1)

8-8: LGTM!

Also applies to: 24-24


📝 Walkthrough

Walkthrough

The Windows path extender now writes the resolved directory directly to Path, joining the configured subdirectory when present. Tests and usage documentation reflect the direct path.

Changes

Windows PATH entries

Layer / File(s) Summary
Write the resolved directory to Path
os/env/path-extender-windows/path-extender-windows.ts, os/env/path-extender-windows/path-extender-windows.spec.ts, os/env/path-extender-windows/path-extender-windows.docs.mdx
The path extender uses the resolved directory for the PATH entry, with the configured subdirectory joined when present. Tests and documentation expect the direct path.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Merge Risk: ⚪ Minimal · up to f8886

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 Summary

Architecture risk: 🔵 Low · up to f8886

The change affects 1 system.

Changed systems: os

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — os (service) was modified; 3 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.docs.mdx: The usage description now says the resolved directory is prepended to Path, replacing the claim that PNPM_HOME’s value is prepended.
  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.docs.mdx: The example Path output now contains C:\pnpm directly instead of %PNPM_HOME%.
  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.spec.ts: The first-installation test now expects the normalized PNPM_HOME directory directly in the updated Path value and registry write, replacing %PNPM_HOME%.
  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.spec.ts: The proxyVarSubDir test now expects the normalized home directory joined with bin directly in Path and the registry write, replacing %PNPM_HOME%\bin.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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: persisting the resolved Windows pnpm bin path instead of a proxy-variable reference.
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.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

A rabbit checks the Windows trail
The resolved path leads without a veil
A bin joins in when set to go
Tests confirm the paths below
Then documentation gets the show

Comment @coderabbitai help to get the list of available commands.

@zkochan

zkochan commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Thanks for digging into pnpm/pnpm#5283. I'm closing this together with its counterpart pnpm/pnpm#16206, because we want to keep the %PNPM_HOME% reference in the Windows user Path. The reasons:

  1. The reference expands when PNPM_HOME is REG_SZ. Setup stores PNPM_HOME as REG_SZ and Path as REG_EXPAND_SZ. Windows sets plain-string user variables before it expands the expandable ones, so %PNPM_HOME%\bin resolves in new logon sessions. The known way it stays literal is PNPM_HOME itself being REG_EXPAND_SZ, which pnpm 7.9.1 to 7.17.0 wrote. fix(setup): repair PNPM_HOME registry type pnpm#15746 (v12) and fix: repair PNPM_HOME registry type #32 (v11) repair that type on the next pnpm setup. fix(config): name an unexpanded PATH entry when the global bin dir is not in PATH pnpm#15886 makes the ERR_PNPM_GLOBAL_BIN_DIR_NOT_IN_PATH error name the unexpanded entry. Your comment on pnpm add -g not support user's environment path on windows pnpm#5283 reports PNPM_HOME as REG_SZ and Path as REG_EXPAND_SZ, which is the expected state, so the open question is how the new environment was obtained. A process whose environment was built without Windows expanding it, or a read through [Environment]::GetEnvironmentVariable('Path', 'User') or reg query, returns the raw registry text by design. A new logon session, or a cmd window opened from the Start menu, shows the environment Windows actually builds.
  2. Existing users would get a duplicate entry. add_to_path / addToPath skip only an exact match, so rerunning pnpm setup with %PNPM_HOME%\bin already in Path prepends the resolved directory as a second entry.
  3. The entry would stop following PNPM_HOME. After pnpm setup --force moves PNPM_HOME, the old resolved directory stays in Path and goes stale.

If echo %PATH% in such a window still shows %PNPM_HOME%\bin literally while reg query HKCU\Environment shows PNPM_HOME as REG_SZ with a full path (no %...% inside), please open an issue with that output. That would be a new bug, and a fix would also need to migrate existing entries in both pnpm versions.


Written by an agent (Claude Code, claude-opus-5-5).

@zkochan zkochan closed this Sep 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants