fix: allow exotic dependencies under workspace packages (pnpm/pnpm#13… - #13719
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughWorkspace provenance now flows through recursive dependency resolution. Exotic dependencies under workspace-resolved packages bypass ChangesWorkspace exotic dependency handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change is localized to allowing explicitly declared exotic dependencies for workspace packages, with coverage in both resolver implementations; no actionable merge-blocking risk remains after normal checks and review. Assessment against linked issues
Suggested labels: Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Confidence Score: 4/5The resolver fix appears functionally safe, but the required release changeset must be added before merging so the published behavior change enters the release plan. The cross-stack behavior change affects published packages, while the current HEAD still has no changeset recording the affected packages, version intent, or user-facing release note. Files Needing Attention: pnpm11/installing/deps-resolver/src/resolveDependencies.ts and the missing .changeset entry Reviews (3): Last reviewed commit: "fix: allow exotic dependencies under wor..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pnpm11/installing/deps-installer/test/install/blockExoticSubdeps.ts (1)
122-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove comments that restate the test code.
Lines 122 and 144 repeat the dependency declaration and the test expectation. The test name and setup already state both facts.
Proposed cleanup
version: '1.0.0', dependencies: { - // Direct git dependency in workspace project 'is-negative': 'github:kevva/is-negative#1.0.0', }, }, @@ - // This should not fail even when blockExoticSubdeps is true await mutateModules(importers, testDefaults({As per coding guidelines, comments must document non-obvious contracts or invariants and must not restate code.
Also applies to: 144-144
🤖 Prompt for 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. In `@pnpm11/installing/deps-installer/test/install/blockExoticSubdeps.ts` around lines 122 - 123, Remove the comments immediately above the dependency declaration and the corresponding test expectation in the blockExoticSubdeps test; keep the setup and assertions unchanged, since the comments only restate what the code already expresses.Source: Coding guidelines
🤖 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 `@pnpm11/installing/deps-installer/test/install/blockExoticSubdeps.ts`:
- Around line 145-149: Extend the test around mutateModules with assertions on
the resulting dependency tree: verify that project-1 links project-2 and that
project-2 installs is-negative. Keep the existing blockExoticSubdeps setup, but
inspect the installed module state rather than only relying on mutateModules
completing without throwing.
---
Nitpick comments:
In `@pnpm11/installing/deps-installer/test/install/blockExoticSubdeps.ts`:
- Around line 122-123: Remove the comments immediately above the dependency
declaration and the corresponding test expectation in the blockExoticSubdeps
test; keep the setup and assertions unchanged, since the comments only restate
what the code already expresses.
🪄 Autofix
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: 73437890-a21c-4109-996d-7f6ec1cd2f5e
📒 Files selected for processing (4)
pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspnpm/crates/resolving-deps-resolver/src/tests.rspnpm11/installing/deps-installer/test/install/blockExoticSubdeps.tspnpm11/installing/deps-resolver/src/resolveDependencies.ts
1b24180 to
caebfa4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13719 +/- ##
=======================================
Coverage 88.31% 88.32%
=======================================
Files 629 629
Lines 104352 104357 +5
=======================================
+ Hits 92163 92175 +12
+ Misses 12189 12182 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Commit: Regular scenarios report direct and pnpr installs. The peer-heavy resolver scenario compares current Rust, main Rust, and TypeScript pnpm. Bencher consumes pacquet@HEAD and pnpr@HEAD. The tables below show mean ± σ; Bencher thresholds on the minimum latency, which is far less perturbed by shared-runner contention (noise only adds time). Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.71053186314,
"stddev": 0.17298129060939935,
"median": 2.7527560699399998,
"user": 2.02553424,
"system": 1.5973476,
"min": 2.4087969019399997,
"max": 2.91252952494
},
{
"command": "pacquet@main",
"mean": 2.58125870814,
"stddev": 0.12535435441858936,
"median": 2.56649584894,
"user": 1.96974534,
"system": 1.4639248000000002,
"min": 2.44045010894,
"max": 2.8391293429399997
},
{
"command": "pnpr@HEAD",
"mean": 1.5446909085400002,
"stddev": 0.12213521370590584,
"median": 1.55664179544,
"user": 1.2209152399999998,
"system": 1.2693366000000004,
"min": 1.38651443694,
"max": 1.70409562194
},
{
"command": "pnpr@main",
"mean": 1.5504314579399998,
"stddev": 0.149486416452129,
"median": 1.5608737429400001,
"user": 1.2601550400000001,
"system": 1.2811315,
"min": 1.36265548394,
"max": 1.76356987294
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.27105353916,
"stddev": 0.00905891471683406,
"median": 0.27258904916000004,
"user": 0.2442572,
"system": 0.5278225799999999,
"min": 0.25442370716,
"max": 0.28215499016
},
{
"command": "pacquet@main",
"mean": 0.27975375906,
"stddev": 0.017134694087588984,
"median": 0.27794459766,
"user": 0.25788289999999997,
"system": 0.55127558,
"min": 0.24902965216000003,
"max": 0.31352574616
},
{
"command": "pnpr@HEAD",
"mean": 0.27658586556000003,
"stddev": 0.011015752785528053,
"median": 0.27692946366000004,
"user": 0.22768249999999995,
"system": 0.54644788,
"min": 0.25989851516,
"max": 0.29671719716
},
{
"command": "pnpr@main",
"mean": 0.2911953125600001,
"stddev": 0.008193652529002575,
"median": 0.29011633116,
"user": 0.2612368,
"system": 0.57262388,
"min": 0.27443619316,
"max": 0.30274937416000003
}
]
}Scenario: Isolated linker: repeat install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.008579706026589584,
"stddev": 0.00232215552459428,
"median": 0.0078379752,
"user": 0.00528228011560693,
"system": 0.0033445142196531787,
"min": 0.006247786700000001,
"max": 0.0157673777
},
{
"command": "pacquet@main",
"mean": 0.007034306003621172,
"stddev": 0.0007336612651885444,
"median": 0.006767304700000001,
"user": 0.004627790194986076,
"system": 0.0024522191643454043,
"min": 0.006047549700000001,
"max": 0.0108294827
},
{
"command": "pnpr@HEAD",
"mean": 0.007330287583419691,
"stddev": 0.0007397344018838529,
"median": 0.007206991700000001,
"user": 0.004823942279792743,
"system": 0.0025556145077720214,
"min": 0.0061822537,
"max": 0.010661445700000001
},
{
"command": "pnpr@main",
"mean": 0.006941376026219511,
"stddev": 0.0006919267062614224,
"median": 0.006720897200000001,
"user": 0.004694352317073169,
"system": 0.0022935175609756103,
"min": 0.0059913137000000005,
"max": 0.0101356957
}
]
}Scenario: Isolated linker: repeat install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.00731996269047619,
"stddev": 0.0008076026909576944,
"median": 0.0073972595,
"user": 0.004735881904761905,
"system": 0.0026531154285714294,
"min": 0.005842247,
"max": 0.010501607000000001
},
{
"command": "pacquet@main",
"mean": 0.006567663900355871,
"stddev": 0.00047944640745560947,
"median": 0.006460612,
"user": 0.004438366263345198,
"system": 0.0022041621352313157,
"min": 0.005747889,
"max": 0.008312427
},
{
"command": "pnpr@HEAD",
"mean": 0.006785427547619046,
"stddev": 0.000686355849353222,
"median": 0.006578052,
"user": 0.004289439047619045,
"system": 0.002570089404761906,
"min": 0.00577466,
"max": 0.010507680000000002
},
{
"command": "pnpr@main",
"mean": 0.007196217857575759,
"stddev": 0.0007881927281714735,
"median": 0.006972304,
"user": 0.004720710909090909,
"system": 0.002548081333333333,
"min": 0.006053575,
"max": 0.010666027000000002
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.79045722662,
"stddev": 0.017026860629833725,
"median": 2.79531938702,
"user": 1.7630821400000003,
"system": 1.4302237599999996,
"min": 2.7587040125200004,
"max": 2.81504911352
},
{
"command": "pacquet@main",
"mean": 2.7660675167200006,
"stddev": 0.02550982953856564,
"median": 2.7605945920200003,
"user": 1.6823856399999997,
"system": 1.4014198599999999,
"min": 2.7278470245200004,
"max": 2.8128001195200003
},
{
"command": "pnpr@HEAD",
"mean": 1.5774255089199998,
"stddev": 0.1354496085936675,
"median": 1.5282384925199999,
"user": 1.1670935400000002,
"system": 1.2690088599999998,
"min": 1.4584638965199999,
"max": 1.85361710652
},
{
"command": "pnpr@main",
"mean": 1.6977593536199997,
"stddev": 0.13252068786378796,
"median": 1.71781162652,
"user": 1.0895224399999999,
"system": 1.3071435599999996,
"min": 1.4755213975199999,
"max": 1.8505682475199998
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.44637550338,
"stddev": 0.022313147066666168,
"median": 0.43741514038000007,
"user": 0.55243008,
"system": 0.6965709999999999,
"min": 0.41768211338000005,
"max": 0.48454684638000006
},
{
"command": "pacquet@main",
"mean": 0.4418695248800001,
"stddev": 0.02103867898195737,
"median": 0.4365722443800001,
"user": 0.5241433799999999,
"system": 0.7333392000000001,
"min": 0.41590351438000006,
"max": 0.4841498993800001
},
{
"command": "pnpr@HEAD",
"mean": 0.3371227771800001,
"stddev": 0.021313112477076984,
"median": 0.33414897088000006,
"user": 0.24142747999999994,
"system": 0.5565945,
"min": 0.30354814438000005,
"max": 0.36392453338
},
{
"command": "pnpr@main",
"mean": 0.31611448778000006,
"stddev": 0.010799642048721775,
"median": 0.31471855288000006,
"user": 0.21495628,
"system": 0.5542701,
"min": 0.30119739938000006,
"max": 0.33473863238000007
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.9167983542000002,
"stddev": 0.015758484679137263,
"median": 1.9089385954,
"user": 0.8273055600000001,
"system": 0.7861488999999999,
"min": 1.9001299664000002,
"max": 1.9497294574000001
},
{
"command": "pacquet@main",
"mean": 1.8941823219,
"stddev": 0.02092980580025804,
"median": 1.8961828794000002,
"user": 0.7853721599999999,
"system": 0.7772702,
"min": 1.8636163844000002,
"max": 1.9234172974000001
},
{
"command": "pnpr@HEAD",
"mean": 0.3182008794000001,
"stddev": 0.011511790729591952,
"median": 0.3149160409,
"user": 0.21261955999999996,
"system": 0.5464417,
"min": 0.2958560874,
"max": 0.33745037640000003
},
{
"command": "pnpr@main",
"mean": 0.3206356105,
"stddev": 0.014212844902351477,
"median": 0.3239808449,
"user": 0.21607275999999995,
"system": 0.5573197,
"min": 0.3004237954,
"max": 0.34320201240000003
}
]
}Scenario: Isolated linker: fresh resolve, hot cache, offline
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.27039500399666666,
"stddev": 0.052952091000669534,
"median": 0.25304528458000003,
"user": 0.2185985066666667,
"system": 0.07994263666666666,
"min": 0.21592818908000003,
"max": 0.36847547208000003
},
{
"command": "pacquet@main",
"mean": 0.24263971162545453,
"stddev": 0.01938930160242902,
"median": 0.24775514108000002,
"user": 0.19443084000000005,
"system": 0.0637008109090909,
"min": 0.20192570008000002,
"max": 0.27408642808
},
{
"command": "pnpr@HEAD",
"mean": 0.09949333984666667,
"stddev": 0.0029535201660212834,
"median": 0.09891761658,
"user": 0.028937673333333334,
"system": 0.013211886666666665,
"min": 0.09490961608000001,
"max": 0.10648150008
},
{
"command": "pnpr@main",
"mean": 0.09788041350857143,
"stddev": 0.0026787846953456266,
"median": 0.09720798358,
"user": 0.02841266142857143,
"system": 0.011809918571428569,
"min": 0.09251303508,
"max": 0.10494464708000001
}
]
}Scenario: Isolated linker: peer-heavy resolve, hot cache, offline
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.5248766428955555,
"stddev": 0.05212618643637823,
"median": 0.51127712434,
"user": 0.5053519422222221,
"system": 0.10536531111111112,
"min": 0.45821845334,
"max": 0.60025205634
},
{
"command": "pacquet@main",
"mean": 0.5776607767844445,
"stddev": 0.13040519091500208,
"median": 0.53631401434,
"user": 0.5554047199999999,
"system": 0.12812853333333335,
"min": 0.46934610234,
"max": 0.86956361234
},
{
"command": "pnpm@HEAD",
"mean": 3.247350528784445,
"stddev": 0.14843779462135123,
"median": 3.22506322534,
"user": 4.971555164444444,
"system": 0.30689208888888886,
"min": 3.05253375334,
"max": 3.51044223334
}
]
}Scenario: Isolated linker: fresh restore, cold cache + cold store + cold pnpr
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.646590645120001,
"stddev": 0.19605782356382637,
"median": 4.56180196872,
"user": 2.16006554,
"system": 1.6238068599999997,
"min": 4.47334168322,
"max": 5.03982171622
},
{
"command": "pacquet@main",
"mean": 4.66750195492,
"stddev": 0.18586113333167625,
"median": 4.60718992522,
"user": 2.11412254,
"system": 1.57572236,
"min": 4.47187609122,
"max": 5.01421816522
},
{
"command": "pnpr@HEAD",
"mean": 3.61393170802,
"stddev": 0.12315264715010855,
"median": 3.58380009572,
"user": 1.36499954,
"system": 1.3093929599999998,
"min": 3.4754141882200003,
"max": 3.83991097122
},
{
"command": "pnpr@main",
"mean": 3.5709802434199993,
"stddev": 0.11634601926639275,
"median": 3.5480094717200004,
"user": 1.31769574,
"system": 1.28958766,
"min": 3.45447083022,
"max": 3.80024852322
}
]
} |
|
| Project | pnpm's project |
| Branch | pr/13719 |
| 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 | 1,458.46 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 295.86 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 303.55 ms |
| isolated-linker.fresh-resolve.hot-cache.offline | 📈 view plot | 94.91 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 1,386.51 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store.cold-pnpr | 📈 view plot | 3,475.41 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 259.90 ms |
| isolated-linker.repeat-install.cold-cache.hot-store | 📈 view plot | 5.77 ms |
| isolated-linker.repeat-install.hot-cache.hot-store | 📈 view plot | 6.18 ms |
caebfa4 to
db50557
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Summary
When
blockExoticSubdepsis enabled, pnpm prevents resolving transitive subdependencies via exotic protocols (likegit,url, orfile). However, direct dependencies of workspace packages are explicitly declared by the user in the repository. They should not be blocked as "subdependencies" even when they are resolved at a depth > 0 (e.g., when a workspace app depends on a workspace package, which in turn depends on an exotic catalog dependency).This PR:
blockExoticSubdepscheck if the parent package was resolved via'workspace'(indicating it is a workspace package).pacquet) dependency resolvers.Closes #13682
Squash Commit Body
Checklist
already linked to it solves it.
pnpm/port.pnpm changeset) if this PR changes any publishedpackage. Keep it short and written for pnpm users — it becomes a release note.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit