Conversation
…bare-semver deps Pacquet's npm resolver only consulted the workspace map for `workspace:`-prefixed wanted deps; bare-semver ranges always went straight to the registry. When the workspace package isn't on npm (e.g. babylon's `@dev/build-tools`), the install errored out with 404; when a same-named package existed on the registry, pacquet silently linked the wrong copy. Mirror pnpm's three workspace branches around `pickPackage`: * registry pick succeeded + workspace shadow (exact `name@version` match, higher local version, or `preferWorkspacePackages`), * registry pick returned `null` → workspace fallback, * registry fetch errored → workspace fallback (swallow workspace errors and re-raise the registry error). Gated by `link-workspace-packages` (true / false / "deep") which is now parsed from `pnpm-workspace.yaml`, flows through `Config`, and is encoded into `ResolveOptions::always_try_workspace_packages` at the install layer. Tri-state semantics are preserved on the config side; pacquet's single-`base_opts` deps-resolver collapses `true` and `"deep"` onto the same per-call flag until depth threading lands. Closes #11929.
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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a tri-state LinkWorkspacePackages config, wires it through workspace settings and persisted workspace state, and implements workspace fallback/shadowing in the npm resolver with new tests. ChangesWorkspace Package Linking
Sequence DiagramsequenceDiagram
participant WorkspaceYAML as pnpm-workspace.yaml
participant Install as install_with_fresh_lockfile
participant Resolver as NpmResolver::resolve_impl
participant Registry as Registry
participant Workspace as workspace_packages
WorkspaceYAML->>Install: linkWorkspacePackages: true
Install->>Resolver: ResolveOptions { always_try_workspace_packages: true }
Resolver->>Registry: pick_package(spec)
alt Registry returns Ok(Some(version))
Registry-->>Resolver: Some(version)
Resolver->>Workspace: try_workspace_shadow (exact match / higher version / prefer flag)
Workspace-->>Resolver: workspace_version or None
Resolver-->>Install: workspace_version or registry_version
else Registry returns Ok(None) or Err
Registry-->>Resolver: Ok(None) / Err
Resolver->>Workspace: try_workspace_fallback
Workspace-->>Resolver: workspace_version or Err
Resolver-->>Install: workspace_version or Err
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Micro-Benchmark ResultsLinux |
…triple-dot to satisfy Dylint
…kages tests from pnpm Cover the six pnpm tests the initial port skipped: * `injected_workspace_match_emits_file_resolution` — workspace shadow branch with `injected: true` emits a `file:` resolution. * `workspace_fallback_picks_highest_version_for_latest_tag` — 404 fallback into a multi-version workspace via the Tag branch of `pick_matching_local_version_or_null`. * `workspace_fallback_picks_local_prerelease_for_latest_tag` — 404 fallback into a prerelease-only workspace, exercising `resolve_workspace_range`'s `includePrerelease` arm. * `workspace_fallback_resolves_specific_version_request` — 404 fallback against a pinned version-spec lookup. * `workspace_fallback_kicks_in_when_registry_lacks_requested_version` — `Ok(None)` fallback path (registry serves the packument but no version matches), distinct from the `Err` 404 path. * `registry_error_propagates_when_workspace_has_no_matching_version` — negative test verifying the original 404 surfaces when the workspace can't satisfy the request. * `registry_pick_wins_when_workspace_version_does_not_match` — workspace shadow no-op when the workspace carries a different version than the registry pick.
…ne assert! to satisfy Dylint
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs (1)
718-722: ⚡ Quick winTrim verbose test doc comments that duplicate the test body.
Most
///blocks here restate scenario/setup/assertion details already encoded by the tests. Prefer a short purpose note (or a single module-level note) and keep non-obvious rationale only.As per coding guidelines, "Tests are documentation. Do not duplicate them in prose... If a behavioral scenario... is already captured by a test, do not also narrate it in the doc comment."
Also applies to: 739-743, 778-782, 809-813, 834-838, 865-869, 901-905, 927-930
🤖 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 `@pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs` around lines 718 - 722, These test doc comments duplicate the test body; replace each verbose `///` block (e.g., the workspace-map comment above the `build_workspace_packages` helper and the similar docblocks at the other listed ranges) with a short purpose note or remove them entirely so the test itself serves as the documentation; keep only non-obvious rationale or intent (one brief sentence) and avoid restating setup/assertions already encoded by the tests, updating the docblocks near `build_workspace_packages` and the other nearby test blocks accordingly.
🤖 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.
Nitpick comments:
In `@pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs`:
- Around line 718-722: These test doc comments duplicate the test body; replace
each verbose `///` block (e.g., the workspace-map comment above the
`build_workspace_packages` helper and the similar docblocks at the other listed
ranges) with a short purpose note or remove them entirely so the test itself
serves as the documentation; keep only non-obvious rationale or intent (one
brief sentence) and avoid restating setup/assertions already encoded by the
tests, updating the docblocks near `build_workspace_packages` and the other
nearby test blocks accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9c9b772-4405-437c-ae09-88b86b6c3931
📒 Files selected for processing (1)
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
🧬 Code graph analysis (1)
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs (1)
pacquet/crates/resolving-resolver-base/src/resolve.rs (5)
WorkspacePackages(175-175)WorkspacePackagesByVersion(171-171)WorkspacePackage(164-167)WantedDependency(82-102)as_str(38-40)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11930 +/- ##
==========================================
+ Coverage 87.67% 87.77% +0.10%
==========================================
Files 220 224 +4
Lines 26800 27295 +495
==========================================
+ Hits 23497 23959 +462
- Misses 3303 3336 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.9160571812600002,
"stddev": 0.44613895289447253,
"median": 2.8353298237600004,
"user": 2.16437372,
"system": 2.5120189999999996,
"min": 2.24956521926,
"max": 3.70236078926,
"times": [
2.7807484542600003,
2.6877338672600004,
2.51888748026,
2.88991119326,
3.18180026826,
3.07391659626,
3.47072934926,
3.70236078926,
2.24956521926,
2.60491859526
]
},
{
"command": "pacquet@main",
"mean": 2.88054608886,
"stddev": 0.5006534277445681,
"median": 2.79352854376,
"user": 2.1616188199999997,
"system": 2.5006565000000003,
"min": 2.28002756426,
"max": 3.73192662926,
"times": [
2.40823908726,
2.96696604226,
2.34964860126,
2.28002756426,
2.66698209426,
3.73192662926,
2.7976661642600003,
3.2619158612600003,
2.78939092326,
3.55269792126
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.91363729154,
"stddev": 0.05632467726844137,
"median": 0.92428626434,
"user": 0.28914193999999993,
"system": 1.04850082,
"min": 0.8505567658400001,
"max": 1.0182092278400001,
"times": [
0.9487154978400001,
0.93263610884,
0.8638016148400001,
0.91741573584,
0.8505567658400001,
1.0182092278400001,
0.9630149278400001,
0.85169552384,
0.9311567928400001,
0.85917071984
]
},
{
"command": "pacquet@main",
"mean": 1.37131064634,
"stddev": 0.30992541055631967,
"median": 1.23023543534,
"user": 0.28758523999999996,
"system": 1.0543704200000001,
"min": 0.95190318084,
"max": 1.96478052884,
"times": [
1.16532768484,
1.69374206184,
0.95190318084,
1.14368465784,
1.2480993148400001,
1.2123715558400001,
1.21227804384,
1.96478052884,
1.58145199184,
1.53946744284
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.94926280236,
"stddev": 0.3666784807487576,
"median": 2.87895286556,
"user": 3.5083229400000002,
"system": 2.5568506599999994,
"min": 2.64735486206,
"max": 3.90701147306,
"times": [
2.96589209606,
2.79855130806,
2.74466443406,
3.07680207706,
2.95935442306,
2.97658897906,
3.90701147306,
2.66659449606,
2.74981387506,
2.64735486206
]
},
{
"command": "pacquet@main",
"mean": 2.9692967575599996,
"stddev": 0.14199256560510357,
"median": 2.93779582256,
"user": 3.5365151399999997,
"system": 2.5878009599999996,
"min": 2.79182139706,
"max": 3.29069119406,
"times": [
3.06188372706,
2.8714433280600002,
2.8940253770599997,
3.29069119406,
2.8599847660599997,
2.92324304506,
3.0595487870599998,
2.98797735406,
2.79182139706,
2.95234860006
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.8019833166800001,
"stddev": 0.14910467241306774,
"median": 1.7408192903800002,
"user": 1.6340099799999996,
"system": 1.60122296,
"min": 1.69678704988,
"max": 2.1006880948799997,
"times": [
2.1006880948799997,
1.7367082608800002,
2.06374602888,
1.7567918878800002,
1.7562765078800002,
1.7202528668800001,
1.7146907668800002,
1.7449303198800001,
1.7289613828800001,
1.69678704988
]
},
{
"command": "pacquet@main",
"mean": 2.0643945593799997,
"stddev": 0.5746877496040788,
"median": 1.83152034588,
"user": 1.6495202799999997,
"system": 1.60357976,
"min": 1.7051538028800002,
"max": 3.5782038528799998,
"times": [
1.7051538028800002,
2.00440652688,
3.5782038528799998,
2.4248780218799997,
2.02999928788,
1.79183171988,
1.7329243148800002,
1.71350737488,
1.8564805428800002,
1.80656014888
]
}
]
} |
|
Bencher Report| Branch | pr/11930 |
| Testbed | pacquet |
🚨 2 Alerts
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| isolated-linker.fresh-restore.cold-cache.cold-store | Latency seconds (s) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 2.92 s(+30.22%)Baseline: 2.24 s | 2.69 s (108.52%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | Latency milliseconds (ms) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 913.64 ms(+39.38%)Baseline: 655.52 ms | 786.62 ms (116.15%) |
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 | 2,949.26 ms(-26.23%)Baseline: 3,997.92 ms | 4,797.50 ms (61.47%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,801.98 ms(-40.43%)Baseline: 3,024.96 ms | 3,629.95 ms (49.64%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 2,916.06 ms(+30.22%)Baseline: 2,239.36 ms | 2,687.23 ms (108.52%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 913.64 ms(+39.38%)Baseline: 655.52 ms | 786.62 ms (116.15%) |
…kWorkspacePackages tests Per pacquet/CLAUDE.md "tests are documentation" — the new test docblocks restated what the test name plus body already say. Keep only the upstream-link citation (required by the porting rule) and drop the trailing narrative. Two ports keep one extra sentence because the distinction they exercise (the `Ok(None)` vs `Err` fallback split, the `includePrerelease` arm) is not recoverable from the test body alone. Also drop the inline `lockfile_dir` / "Registry returns a packument..." comments that narrate setup the helper code already encodes; keep the `latest`-back-stamp comment because it explains why a workspace-resolved result still carries that field.
Summary
pickPackagein pacquet's npm resolver, so a bare-semver dependency on a workspace package can resolve to the local copy instead of going to (or 404-ing on) the registry.linkWorkspacePackages(true | false | "deep") parsing topnpm-workspace.yaml, threads it throughConfig, and encodes it intoResolveOptions::always_try_workspace_packagesat the install layer..pnpm-workspace-state-v1.jsonso a subsequentpnpminvocation can detect a setting change.Closes #11929.
Why
Pacquet's
NpmResolver::resolve_implonly routedworkspace:-prefixed specs throughtry_resolve_from_workspace. For bare-semver specs (e.g.\"@dev/build-tools\": \"^1.0.0\"where@dev/build-toolsis a workspace package), it fell straight through to the registry picker. The vlt.sh babylon fixture hits this path, gets a 404 from npm for@dev/build-tools, and reports DNF on every variation.For users whose workspace package shares a name with a published npm package, the same gap silently links the wrong copy.
How
In
resolving-npm-resolver/src/npm_resolver.rs::resolve_impl:workspace_packages_active = always_try_workspace_packages ? workspace_packages : None. Matches pnpm'sopts.alwaysTryWorkspacePackages !== falsegate atindex.ts#L435.pick_packagereturningOk(None)orErr(_), trytry_resolve_from_workspace_packages(already used by theworkspace:path) before reporting "no matching version" or surfacing the original fetch error. Workspace errors are swallowed.Ok(Some(picked)), run the registry-pick + workspace-shadow logic fromindex.ts#L550-L582: exactname@versionmatch wins, a higher workspace version wins,preferWorkspacePackageswins.The existing
try_resolve_from_workspace_packages,pick_matching_local_version_or_null, andresolve_from_local_packagehelpers inresolve_from_workspace.rsare promoted topub(crate)so the npm resolver can call them directly (matching the way upstream factors the same helpers).In
pacquet-config:LinkWorkspacePackagestri-state enum with a customDeserializeacceptingbool | "deep", mirroringConfig.linkWorkspacePackages.Config(defaultOff) and toWorkspaceSettings::apply_to. Listed inclear_workspace_only_fieldsto match the global config filter atconfigFileKey.ts#L113.In
pacquet-package-manager:install_with_fresh_lockfile.rssetsalways_try_workspace_packagesfromconfig.link_workspace_packages != Off.install.rsserializes the value intoWorkspaceStateSettings.link_workspace_packagesas the same tri-state JSON shape pnpm writes.Behavioral note
Pacquet's deps-resolver currently passes a single
base_optsto every resolve call, with no per-depth differentiation. Pnpm gates the workspace lookup bylinkWorkspacePackagesDepth >= currentDepthsotrueapplies only to depth 0 and"deep"applies to every depth. With pacquet's current shape,trueand"deep"both collapse onto a single flag that applies at every depth. The (rare) result is that a transitive registry dep whose name and version match a workspace package may resolve to the workspace instead of the registry underlinkWorkspacePackages: true. Threading per-call depth into the resolver is a follow-up.Test plan
cargo nextest run -p pacquet-config -p pacquet-resolving-npm-resolver -p pacquet-package-manager— 743 passedcargo clippy --locked --workspace --all-targets -- --deny warningscargo fmt -- --checkresolving/npm-resolver/test/index.tscovering branches 2 and 3 (falls_back_to_workspace_when_registry_returns_404,workspace_shadows_registry_when_name_and_version_match,always_try_workspace_packages_false_skips_workspace_match,registry_version_higher_than_workspace_keeps_registry_pick,prefer_workspace_packages_keeps_workspace_over_newer_registry,workspace_higher_version_shadows_registry_pick).Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests