fix: dedupe workspace deps with children in single-project mode#11448
Merged
zkochan merged 6 commits intoJun 17, 2026
Conversation
|
💖 Thanks for opening this pull request! 💖 |
📝 WalkthroughWalkthroughThis PR extends a prior deduplication fix for injected workspace packages: when a workspace dependency has its own dependencies and a single-project ChangesWorkspace Dependency Deduplication with Children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a regression where, with injectWorkspacePackages: true, single-project operations (notably pnpm rm run from inside a workspace package) can cause workspace dependency entries to flip from link: to file: in pnpm-lock.yaml when the target workspace package has its own dependencies.
Changes:
- Adjust
dedupeInjectedDepsto dedupe injected workspace deps even when the target workspace project wasn’t part of the current resolution (dependenciesByProjectIdmissing). - Add a regression test that reproduces the single-project uninstall path when the target workspace package has its own dependencies.
- Add a changeset for patch releases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| installing/deps-resolver/src/dedupeInjectedDeps.ts | Changes dedupe behavior when the target workspace project isn’t in dependenciesByProjectId, aiming to preserve link: protocol in single-project operations. |
| installing/deps-installer/test/install/injectWorkspacePackagesPersistence.test.ts | Adds a regression test for pnpm rm-equivalent single-project mutation when the target workspace dep has children. |
| .changeset/dedupe-injected-deps-with-children.md | Declares patch releases and documents the behavioral fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DASPRiD
added a commit
to DASPRiD/pnpm
that referenced
this pull request
May 4, 2026
The previous fast-path unconditionally deduped any injected workspace dep when the target project wasn't part of the resolution. For peer-resolved injected deps (depPaths containing `(`), the resolution depends on the importer's peer context, and a plain `link:` would lose that. Skip dedupe whenever the depPath has a peer suffix. Adds a third test case that exercises the peer-resolved single-project rm path. Spotted by Copilot review on pnpm#11448. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressing @copilot's review feedback (#11448 (comment)): The fast-path now skips dedupe when Added a third test in |
bd8600c to
e91f3c3
Compare
zkochan
pushed a commit
to DASPRiD/pnpm
that referenced
this pull request
Jun 17, 2026
The previous fast-path unconditionally deduped any injected workspace dep when the target project wasn't part of the resolution. For peer-resolved injected deps (depPaths containing `(`), the resolution depends on the importer's peer context, and a plain `link:` would lose that. Skip dedupe whenever the depPath has a peer suffix. Adds a third test case that exercises the peer-resolved single-project rm path. Spotted by Copilot review on pnpm#11448. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review by Qodo
1.
|
|
Code review by qodo was updated up to the latest commit 94234ff |
94234ff to
924ccc3
Compare
zkochan
approved these changes
Jun 17, 2026
|
Code review by qodo was updated up to the latest commit 924ccc3 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/cli/tests/dedupe_injected_deps.rs`:
- Around line 89-188: The test
injected_workspace_dep_with_children_stays_link_after_remove currently verifies
that injected workspace dependencies stay as link:../b after remove, but it
doesn't cover the case where peer-suffixed depPaths (like file:b(peer@...))
should be preserved with their peer context intact. Extend the test to create a
scenario with peer-suffixed dependencies, then add assertions after the remove
operation to verify that peer-suffix data in dependency paths is maintained in
the pnpm-lock.yaml and hasn't been stripped or simplified, ensuring lockfile
identity integrity is preserved through the remove operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a0bc522-837b-4620-993d-2cd202a3922b
📒 Files selected for processing (2)
installing/deps-resolver/src/dedupeInjectedDeps.tspacquet/crates/cli/tests/dedupe_injected_deps.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- installing/deps-resolver/src/dedupeInjectedDeps.ts
Removes the defensive guard added in pnpm#10575 that skipped deduplication whenever the target workspace project's children weren't available in the current resolution. In single-project operations the injected dep is resolved against the same workspace package source, so dedupe is safe and the link: protocol is preserved. Adds a test case where the workspace dep target has its own children, reproducing the bug the original PR was meant to fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous fast-path unconditionally deduped any injected workspace dep when the target project wasn't part of the resolution. For peer-resolved injected deps (depPaths containing `(`), the resolution depends on the importer's peer context, and a plain `link:` would lose that. Skip dedupe whenever the depPath has a peer suffix. Adds a third test case that exercises the peer-resolved single-project rm path. Spotted by Copilot review on pnpm#11448. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without the explicit type, TypeScript inferred the workspacePackages Map's value type from project a's manifest (which has dependencies), making project b's manifest (which has peerDependencies but no dependencies) fail to unify. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…jectedDeps
The single-project fast-path must skip dedupe only for peer-suffixed
depPaths. A raw `includes('(')` check misfires when a workspace
project's directory path itself contains a parenthesis. A depPath is
built as `${pkgIdWithPatchHash}${peerDepGraphHash}`, so it carries a
peer suffix exactly when it differs from its peer-free
`pkgIdWithPatchHash` — an exact comparison of already-resolved data,
with no string heuristic.
…emove Pacquet's dedupeInjectedDeps had no end-to-end coverage for an injected workspace dep that has its own dependencies (the children-subset path); the existing test only covered the childless case. Add a CLI test with injectWorkspacePackages and a workspace dep that depends on a registry package, asserting it is deduped to link: on install and stays link: after a remove re-resolves the workspace.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11448 +/- ##
=======================================
Coverage 88.13% 88.13%
=======================================
Files 310 310
Lines 41864 41861 -3
=======================================
- Hits 36896 36894 -2
+ Misses 4968 4967 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
924ccc3 to
81fda9d
Compare
…emove The existing pacquet dedupe coverage asserted that a childless and a with-children injected workspace dep collapse to link: and stay link: after a remove, but nothing pinned the peer-suffixed case. A peer-resolved injected dep's identity depends on the importer's peer context, so it must stay in its `file:packages/b(peer@ver)` form rather than collapse to a plain `link:../b` that would strip the peer suffix. Add a CLI test where workspace dep `b` declares a peer dependency that `a` provides, asserting the peer-suffixed file: entry is produced on install and preserved byte-for-byte through a remove that re-resolves the whole workspace. Mirrors the TS peer-suffixed single-project rm test on the pacquet side. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit 24cb56e |
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.16922529872,
"stddev": 0.1585788386673922,
"median": 4.08543503532,
"user": 3.8952464399999998,
"system": 3.4723049399999995,
"min": 4.01063320232,
"max": 4.4123248333200005,
"times": [
4.4123248333200005,
4.10455205532,
4.06631801532,
4.38151114332,
4.26574094732,
4.0569983373200005,
4.01063320232,
4.05874686932,
4.32282401832,
4.01260356532
]
},
{
"command": "pacquet@main",
"mean": 4.12118126052,
"stddev": 0.12976790165426558,
"median": 4.0755062653200005,
"user": 3.8949780399999994,
"system": 3.4674872399999996,
"min": 3.94426177332,
"max": 4.34006984432,
"times": [
3.94426177332,
4.0661534253200005,
4.34006984432,
4.0848591053200005,
4.2704918883200005,
4.030954293320001,
4.277190147320001,
4.0355153473200005,
4.04454114632,
4.11777563432
]
},
{
"command": "pnpr@HEAD",
"mean": 2.05283350242,
"stddev": 0.08813716442379484,
"median": 2.02651524682,
"user": 2.59722874,
"system": 2.9938379399999997,
"min": 1.95539420232,
"max": 2.26865762832,
"times": [
2.04698003132,
2.00329477832,
2.03464321232,
2.01838728132,
2.26865762832,
2.00475273932,
2.00144684932,
1.95539420232,
2.11872492632,
2.07605337532
]
},
{
"command": "pnpr@main",
"mean": 2.0495164252199998,
"stddev": 0.10375375113172854,
"median": 2.01103503132,
"user": 2.6006448399999997,
"system": 2.9865364399999996,
"min": 1.9553005083200001,
"max": 2.30531309032,
"times": [
2.00732526432,
2.00297510732,
2.01987858832,
2.05364503532,
2.14998370232,
2.30531309032,
1.99759250732,
1.98840565032,
1.9553005083200001,
2.01474479832
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6487752078,
"stddev": 0.008463196998539431,
"median": 0.6467481648,
"user": 0.3829406799999999,
"system": 1.3388325999999997,
"min": 0.6378976748,
"max": 0.6606972778,
"times": [
0.6488702298,
0.6606972778,
0.6378976748,
0.6441537848000001,
0.6438432628,
0.6526780898,
0.6446260998000001,
0.6579102548,
0.6379907898,
0.6590846138
]
},
{
"command": "pacquet@main",
"mean": 0.6237826773000001,
"stddev": 0.00558478476149773,
"median": 0.6218904043,
"user": 0.38594098,
"system": 1.3055861,
"min": 0.6171006548,
"max": 0.6329533088,
"times": [
0.6310264398000001,
0.6212874208,
0.6199556378000001,
0.6171006548,
0.6224933878000001,
0.6180031278,
0.6211597558,
0.6240132888000001,
0.6329533088,
0.6298337508
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7743046479,
"stddev": 0.12809796955559816,
"median": 0.7375015738,
"user": 0.40837428,
"system": 1.4058275,
"min": 0.7078489868000001,
"max": 1.1360276548,
"times": [
0.7578343868,
0.7342737498,
0.7374780158,
0.7084254508000001,
0.7375251318,
0.7327143268,
0.7527161848,
0.7382025908000001,
0.7078489868000001,
1.1360276548
]
},
{
"command": "pnpr@main",
"mean": 0.7031111992000001,
"stddev": 0.08015111687005719,
"median": 0.6766265078,
"user": 0.39660828,
"system": 1.3444207,
"min": 0.6590566648,
"max": 0.9255818148,
"times": [
0.6824575268,
0.6590566648,
0.7001152258000001,
0.6836794318,
0.6662554888000001,
0.6707954888000001,
0.6605641458,
0.6680767078000001,
0.9255818148,
0.7145294968
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.201751134660001,
"stddev": 0.051003698701321314,
"median": 4.19555348266,
"user": 3.7192203400000006,
"system": 3.3394490400000003,
"min": 4.13947034566,
"max": 4.28396231266,
"times": [
4.18242093266,
4.28396231266,
4.15446685066,
4.198347296660001,
4.25690924266,
4.19799376966,
4.26455673566,
4.1931131956600005,
4.14627066466,
4.13947034566
]
},
{
"command": "pacquet@main",
"mean": 4.162003422760001,
"stddev": 0.020195745954781325,
"median": 4.165145256660001,
"user": 3.6977772399999993,
"system": 3.3388716400000007,
"min": 4.132862304660001,
"max": 4.18959035666,
"times": [
4.1894916676600005,
4.18959035666,
4.164787915660001,
4.166084151660001,
4.14345908566,
4.179106919660001,
4.1480651256600005,
4.132862304660001,
4.165502597660001,
4.141084102660001
]
},
{
"command": "pnpr@HEAD",
"mean": 2.16427511866,
"stddev": 0.12034283881067602,
"median": 2.16216224316,
"user": 2.4428411399999996,
"system": 2.88978134,
"min": 2.00871753766,
"max": 2.40156427366,
"times": [
2.01013626666,
2.10751152266,
2.20737855266,
2.00871753766,
2.16115252966,
2.29780925066,
2.17598081366,
2.40156427366,
2.16317195666,
2.10932848266
]
},
{
"command": "pnpr@main",
"mean": 2.1769394569600005,
"stddev": 0.13607841267897341,
"median": 2.15867335616,
"user": 2.42456344,
"system": 2.8954117399999992,
"min": 2.01942481066,
"max": 2.4349982466599998,
"times": [
2.16038896966,
2.15695774266,
2.20831308766,
2.34511336866,
2.12872992066,
2.01942481066,
2.0355832176599997,
2.24060930466,
2.03927590066,
2.4349982466599998
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.33281732632,
"stddev": 0.011911888686680888,
"median": 1.3376011600200002,
"user": 1.35126634,
"system": 1.7098449600000003,
"min": 1.31613531402,
"max": 1.3479376820200002,
"times": [
1.33807671702,
1.3176415650200002,
1.31613531402,
1.3449487910200002,
1.3230877840200002,
1.3431606630200001,
1.32198242702,
1.3479376820200002,
1.33730790202,
1.33789441802
]
},
{
"command": "pacquet@main",
"mean": 1.3561373836200001,
"stddev": 0.08373907621769579,
"median": 1.3304655855200003,
"user": 1.36765554,
"system": 1.6940052599999997,
"min": 1.3101795100200002,
"max": 1.5915342150200003,
"times": [
1.33822435602,
1.3333040200200001,
1.3273670400200002,
1.5915342150200003,
1.3417980790200001,
1.3238615690200002,
1.35438807402,
1.3276271510200002,
1.3130898220200002,
1.3101795100200002
]
},
{
"command": "pnpr@HEAD",
"mean": 0.69023597932,
"stddev": 0.07627880179080472,
"median": 0.66638292702,
"user": 0.34436624,
"system": 1.3009123599999999,
"min": 0.65724964202,
"max": 0.90629389802,
"times": [
0.65724964202,
0.66454248102,
0.66822337302,
0.90629389802,
0.67880861602,
0.65872195602,
0.67586941402,
0.67098589702,
0.6574835840200001,
0.6641809320200001
]
},
{
"command": "pnpr@main",
"mean": 0.66829295362,
"stddev": 0.011308884961379138,
"median": 0.66528196552,
"user": 0.34326133999999997,
"system": 1.2893644599999998,
"min": 0.65768845602,
"max": 0.69464989202,
"times": [
0.66221055302,
0.65768845602,
0.6590763550200001,
0.66835337802,
0.69464989202,
0.65922992302,
0.67548969202,
0.66883491502,
0.67530786402,
0.6620885080200001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.9723380720200003,
"stddev": 0.03557807625757142,
"median": 2.96081738672,
"user": 1.80323778,
"system": 1.93088406,
"min": 2.93698002072,
"max": 3.05896933572,
"times": [
2.97304185472,
2.98695638972,
2.94795190172,
2.94918658772,
2.9519726637200003,
2.95772877972,
2.99668719272,
3.05896933572,
2.96390599372,
2.93698002072
]
},
{
"command": "pacquet@main",
"mean": 3.00253723432,
"stddev": 0.059992916939952366,
"median": 2.98770363472,
"user": 1.8091009799999997,
"system": 1.9525032599999996,
"min": 2.9609553337200003,
"max": 3.16789950672,
"times": [
2.97506612472,
2.9896739507200003,
2.99582563572,
2.9609553337200003,
2.98573331872,
2.97757353872,
3.01239618172,
2.96697114972,
3.16789950672,
2.99327760272
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6702866164200001,
"stddev": 0.00874681829646051,
"median": 0.67330525722,
"user": 0.34298928,
"system": 1.30718116,
"min": 0.65354951472,
"max": 0.68127265272,
"times": [
0.68127265272,
0.6750473757200001,
0.6737447977200001,
0.65354951472,
0.6728657167200001,
0.6743740907200001,
0.6611898447200001,
0.67959409372,
0.66272366672,
0.66850441072
]
},
{
"command": "pnpr@main",
"mean": 0.64347197862,
"stddev": 0.0055268283199200385,
"median": 0.64352287972,
"user": 0.33866107999999995,
"system": 1.27763046,
"min": 0.63374759772,
"max": 0.6506434507200001,
"times": [
0.64867341972,
0.64132057472,
0.65053284272,
0.64366699872,
0.63374759772,
0.6451665667200001,
0.6381994027200001,
0.6506434507200001,
0.64337876072,
0.63939017172
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/11448 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,201.75 ms(-0.12%)Baseline: 4,206.88 ms | 5,048.25 ms (83.23%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 2,972.34 ms(-1.36%)Baseline: 3,013.43 ms | 3,616.12 ms (82.20%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,332.82 ms(+0.35%)Baseline: 1,328.19 ms | 1,593.83 ms (83.62%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,169.23 ms(+0.01%)Baseline: 4,168.86 ms | 5,002.63 ms (83.34%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 648.78 ms(+5.67%)Baseline: 613.94 ms | 736.72 ms (88.06%) |
Contributor
|
| Branch | pr/11448 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,164.28 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 670.29 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 690.24 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,052.83 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 774.30 ms |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
When
injectWorkspacePackages: trueis set and a workspace package depends on another workspace package that has its own dependencies, runningpnpm rmfrom inside the dependent package's directory switches the lockfile protocol fromlink:tofile:.Reproduction (workspace where
adepends on workspaceb, andbhas any dependency of its own):Root Cause
The fix in #10575 added a defensive guard in
dedupeInjectedDepsthat skipped deduplication whenever the target workspace project's children weren't independenciesByProjectId:In single-project operations (
mutateModulesInSingleProject, used bypnpm rmfrom inside a package directory) only the operated-on project is resolved.dependenciesByProjectIdthen only has that one project, so the guard fires for any workspace dependency whose target has children, and the protocol staysfile:.Solution
In single-project mode the injected dep is resolved against the same workspace package source, so dedupe is safe — except for peer-suffixed depPaths, whose resolution depends on the importer's peer context (a plain
link:would lose it). The new code dedupes whenevertargetProjectDepsis missing for a known workspace project and the depPath has no peer suffix. The peer-suffix check compares the depPath against its peer-freepkgIdWithPatchHash(depPaths are built as${pkgIdWithPatchHash}${peerDepGraphHash}), so it's exact rather than a(-substring heuristic.Test
Two tests were added alongside the existing one in
injectWorkspacePackagesPersistence.test.ts: one where workspace packagebdepends onis-negative, run throughmutateModulesInSingleProjectwithmutation: 'uninstallSome'to reproduce thepnpm rmpath (fails onmain, passes here); and one wherebhas a peer dependency, so the injected entry resolves tofile:b(is-positive@1.0.0)and must stay in that peer-suffixed form rather than collapsing tolink:.On the pacquet side, a behavioral CLI test (
pacquet/crates/cli/tests/dedupe_injected_deps.rs) covers an injected workspace dep that has its own dependency being deduped tolink:and surviving aremove.Summary by CodeRabbit
Bug Fixes
link:workspace protocol when removing workspace packages withinjectWorkspacePackagesenabled, preventinglink:→file:switching—also when the affected workspace dependency has its own dependencies.Tests
link:resolutions.🤖 Description updated by Claude (Opus 4.8) via Claude Code.