rushstack icon indicating copy to clipboard operation
rushstack copied to clipboard

[rush] `rush-pnpm patch` puts absolute paths into pnpm-config.json

Open Toxaris opened this issue 4 months ago • 2 comments

Summary

We already have patches configured in pnpm-config.json. When adding another patch using rush-pnpm, the new entry gets added correctly with a relative path, but the old entries in the pnpm-config.json file get rewritten to use an absolute path.

Repro steps

We already have pnpm patches configured in pnpm-config.json.

$ awk -e '/"globalPatchedDependencies": {/, /}/ { print }' < common/config/rush/pnpm-config.json 

  "globalPatchedDependencies": {
    "$package1@$version1": "patches/$package1@$version1.patch",
    "@$scope2/$package2@$version2": "patches/$scope2__$package2@$version2.patch"
  },

We started adding another patch:

$ rush-pnpm patch $package3

Patch: You can now edit the package at:

  /home/$user/$project/common/temp/node_modules/.pnpm_patches/$package3@$version3

To commit your changes, run:

  rush-pnpm --subspace default patch-commit '/home/$user/$project/common/temp/node_modules/.pnpm_patches/$project3-$version3'

After modifying files in '/home/$user/$project/common/temp/node_modules/.pnpm_patches/$project3-$version3, we proceeded as instructed:

$ rush-pnpm --subspace default patch-commit '/home/$user/$project/common/temp/node_modules/.pnpm_patches/$package3@$version3'

Copying /home/$user/$project/common/temp/patches
  --> /home/$user/$project/common/pnpm-patches

Running "rush update"

...

Running "pnpm install" in /home/$user/$project/common/temp

...

Copying "/home/$user/$project/common/temp/pnpm-lock.yaml"
  --> "/home/$user/$project/common/config/rush/pnpm-lock.yaml"

Rush refreshed the pnpm-config.json, shrinkwrap file and patch files under the "/home/$user/$project/common/temp/patches" folder.
  Please commit this change to Git.

(Side observation: I think the message should say patch files under the "/home/$user/$project/common/pnpm-patches" folder)

Expected result:

The old patches should be left alone. The new patch should use a relative path and be specific to a version. For example, we would expect:

diff --git a/common/config/rush/pnpm-config.json b/common/config/rush/pnpm-config.json
index $hash1..$hash2 100644
--- a/common/config/rush/pnpm-config.json
+++ b/common/config/rush/pnpm-config.json
@@ -$line,1 +$line,2 @@
    * PNPM documentation: https://pnpm.io/package_json#pnpmpatcheddependencies
    */
   "globalPatchedDependencies": {
     "$package1@$version1": "patches/$package1@$version1.patch",
-    "@$scope2/$package2@$version2": "patches/$scope2__$package2@$version2.patch"
+    "@$scope2/$package2@$version2": "patches/$scope2__$package2@$version2.patch",
+    "$package3@$version3": "patches/$package3@$version3.patch"
   },
 
   /**

Actual result:

The old patches are rewritten to use the absolute path of the common/temp folder, leaking the directory of the checkout into the configuration. The new patch is added as a relative path, but not specific to a version. For example, we see:

diff --git a/common/config/rush/pnpm-config.json b/common/config/rush/pnpm-config.json
index $hash1..$hash2 100644
--- a/common/config/rush/pnpm-config.json
+++ b/common/config/rush/pnpm-config.json
@@ -$line-1,2 +$line-1,3 @@
    * PNPM documentation: https://pnpm.io/package_json#pnpmpatcheddependencies
    */
   "globalPatchedDependencies": {
-    "$package1@$version1": "patches/$package1@$version1.patch",
-    "@$scope2/$package2@$version2": "patches/$scope2__$package2@$version2.patch"
+    "$package1@$version1": "/home/$user/$project/common/temp/patches/$package1@$version1.patch",
+    "@$scope2/$package2@$version2": "/home/$user/$project/common/temp/patches/$scope2__$package2@$version2.patch",
+    "$package3": "patches/$package3.patch"
   },
 
   /**

Details

Looks like the mangling of paths in the interop between rush and pnpm is missing some steps?

Standard questions

Question Answer
@microsoft/rush globally installed version? 5.148.0
rushVersion from rush.json? 5.148.0
useWorkspaces from rush.json? true (in pnpm-config.json)
Operating system? Linux
Would you consider contributing a PR? maybe
Node.js version (node -v)? 22.19.0

rush-pnpm -v is 10.7.1. We can also reproduce this with [email protected] globally and in the rush.json.

Toxaris avatar Sep 02 '25 12:09 Toxaris

This may be a compatibility issue that needs fixing for pnpm >= 9.

dmichon-msft avatar Sep 03 '25 18:09 dmichon-msft

Hey @dmichon-msft Proposed fix plan for this issue

Approach

  • Add a small normalizer used only by rush-pnpm after patch-commit/patch-remove, before persisting to pnpm-config.json.
  • Transformation rules for each [key, value] in pnpm.patchedDependencies:
    • Key normalization:
      • If key lacks a version, infer from the patch filename <encodedName>@<version>.patch.
      • Decode scoped names from @scope__name@scope/name.
      • Final form: package@exactVersion.
    • Path normalization:
      • If value is absolute or under the temp patches folder, replace with patches/<filename>.patch.
      • Normalize separators to /.
  • Merge policy:
    • patch-commit: shallow-merge normalized entries into existing globalPatchedDependencies; preserve untouched entries.
    • patch-remove: use the normalized new map as authoritative so deletions are respected.
  • Copy behavior unchanged (temp → committed pnpm-patches). Update the post-update log to reference the committed pnpm-patches path.

Non-goals

  • No schema or config format changes.
  • No changes to non-patch flows.
  • No rewriting of existing entries unless the same key is being updated.

Validation to add with PR

  • Unit tests covering: absolute→relative path, unversioned→versioned key, scoped names, preservation of existing entries, and patch-remove deletions.

Questions

  • Is the merge/replace policy (commit vs remove) acceptable?
  • Any additional edge cases you want included (e.g., multiple __ in scoped names, Windows path variants)?

If this approach looks good, I’ll open a PR with the change and tests.

AryanBagade avatar Sep 04 '25 00:09 AryanBagade