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

fix: repair PNPM_HOME registry type - #32

Merged
zkochan merged 1 commit into
pnpm:mainfrom
amishabenramani:fix/pnpm-home-registry-type
Sep 26, 2026
Merged

zkochan merged 1 commit into
pnpm:mainfrom
amishabenramani:fix/pnpm-home-registry-type

Conversation

@amishabenramani

@amishabenramani amishabenramani commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Repair legacy Windows PNPM_HOME registry entries when the stored path is already correct but the registry value type is not.

Older pnpm setup versions could leave PNPM_HOME as REG_EXPAND_SZ. Current setup compared only the stored data, so a matching path was treated as already correct and never rewritten as the non-expandable REG_SZ value that %PNPM_HOME% references in Path require.

The Windows path extender now:

  • reads both registry value data and type;
  • preserves the existing error when PNPM_HOME points somewhere else;
  • skips a matching value only when its registry type is also correct;
  • rewrites a matching value when only the registry type is wrong.

This is the TypeScript/v11 half of pnpm/pnpm#5696. The companion pacquet/v12 fix is pnpm/pnpm#15746.

Regression evidence

On native Windows 11 x64:

  • Before the implementation change, the focused regression failed because a matching REG_EXPAND_SZ PNPM_HOME was reported as skipped rather than updated.
  • After the change, the current path-extender-windows.spec.ts suite passes: 12/12 tests.
  • The suite covers repair of the legacy type, idempotency when PNPM_HOME is already REG_SZ, different-value protection, forced overwrite, Path behavior, and failure handling.
  • A strict TypeScript no-emit check of the changed source passes.
  • git diff --check passes.

The repository-documented bit test pnpm.os/env/path-extender-windows command could not reach the tests locally because this checkout's Bit workspace fails to resolve pnpm.env/envs/pnpm-env@3.1.0, including after bit install. The standalone Jest evidence above runs the component's actual source/spec with its declared test dependencies.


Written by an agent (ChatGPT, GPT-5.6 Sol).

Summary by CodeRabbit

  • Bug Fixes
    • Windows environment setup now corrects an existing variable when its value matches the requested directory but its registry type does not.
    • Existing variables with both the matching value and registry type are left unchanged, and an already configured Path entry is not rewritten.

@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 25, 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: c1e204be-c085-4b35-8c31-a372fd0f8e42

📥 Commits

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

📒 Files selected for processing (2)
  • 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 context used
🪛 ast-grep (0.45.3)
os/env/path-extender-windows/path-extender-windows.ts

[warning] 148-148: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^ {4}(?<name>${envVarName}) {4}(?<type>\\w+) {4}(?<data>.*)$, 'gim')
Note: [CWE-1333] Inefficient Regular Expression Complexity

(regexp-from-variable)

🔇 Additional comments (2)
os/env/path-extender-windows/path-extender-windows.ts (1)

93-105: LGTM!

Also applies to: 142-151

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

328-328: LGTM!

Also applies to: 340-344, 346-386


📝 Walkthrough

Walkthrough

The Windows path extender now checks the registry value type as well as its data. It skips a matching value only when its type also matches the requested type. Tests cover updates for type mismatches and skips for matching values and types.

Changes

Windows registry type matching

Layer / File(s) Summary
Typed registry lookup and update
os/env/path-extender-windows/path-extender-windows.ts, os/env/path-extender-windows/path-extender-windows.spec.ts
The registry lookup returns the value name, type, and data. When an existing value matches but its type differs from the requested type, the update logic writes the requested value. Tests verify that mismatched types trigger writes and matching values and types are skipped.

Priority: ⬇️ Low

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

Merge Risk: ⚪ Minimal · up to 355fe

The Windows path extender now updates matching registry values with the wrong type, while retaining the skip behavior for matching values and types. No actionable merge-blocking risk is identified.

Architecture Summary

Architecture risk: 🟡 Medium · up to 355fe

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; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.spec.ts: The existing-PNPM_HOME test now expects the variable’s action to be updated rather than skipped.
  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.spec.ts: The test now expects the existing PNPM_HOME value and Path to be written to the registry, followed by environment refresh and registry cleanup commands.
  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.spec.ts: Adds a test where PNPM_HOME has the correct REG_SZ type and value, and Path already includes %PNPM_HOME%. It expects both variables to be skipped with unchanged values and asserts that no registry add command is called.
  • observed — Modified behavior in os/env/path-extender-windows/path-extender-windows.ts: updateEnvVariable now obtains registry data and type. With overwrite disabled, a differing existing value still throws; a matching value is skipped only when its type matches the requested REG_EXPAND_SZ or REG_SZ type. Otherwise it writes the requested value and reports an update. Previously, any matching existing value was skipped regardless of type.

Reliability and maintainability

  • inferred — Risk-relevant change factors for os: blast_radius_2; direct_dependents_2
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: repairing the PNPM_HOME registry type handling on Windows.
  • 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 each registry key
For matching type and data, too
If types differ, the value is set
If both match, no write is due
Then off I hop beneath the moon

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

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