This repository was archived by the owner on May 14, 2026. It is now read-only.
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pacquet-workspace crate (workspace manifest parsing, project enumeration, root finding), encodes lockfile importer versions with ImporterDepVersion (Regular vs Link), refactors symlink/bin linking to operate per-importer under a workspace root, and threads workspace-root discovery into install flows. ChangesWorkspace Support and Per-Importer Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main pnpm/pacquet#443 +/- ##
==========================================
- Coverage 87.20% 87.16% -0.04%
==========================================
Files 105 113 +8
Lines 8493 9187 +694
==========================================
+ Hits 7406 8008 +602
- Misses 1087 1179 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces Stage 1 workspace support for pacquet install --frozen-lockfile by adding a new pacquet-workspace crate (workspace root discovery + pnpm-workspace.yaml parsing + project enumeration) and updating the installer pipeline to operate per-lockfile-importer (including link: workspace deps) while aligning reporter prefix semantics with upstream pnpm.
Changes:
- Added
pacquet-workspacecrate implementing workspace root finding, workspace manifest parsing (packages:), project enumeration, and project manifest reading. - Updated frozen-lockfile install to resolve
lockfileDirvia workspace discovery and to symlink direct dependencies per importer (includingversion: link:...support). - Updated lockfile types to parse/serialize importer dependency
versionas either a regular semver-with-peer or alink:workspace target; addedwaxfor globbing.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/workspace/src/lib.rs | Exposes workspace discovery/manifest/projects APIs from the new crate. |
| crates/workspace/src/root_finder.rs | Implements find_workspace_dir parity with pnpm (walk-up + bad-name detection + env override). |
| crates/workspace/src/root_finder/tests.rs | Adds unit tests for workspace root discovery behavior. |
| crates/workspace/src/manifest.rs | Parses pnpm-workspace.yaml into a minimal WorkspaceManifest (currently packages: only). |
| crates/workspace/src/manifest/tests.rs | Tests workspace manifest parsing/validation behaviors. |
| crates/workspace/src/project_manifest.rs | Adds package.json-only project manifest reader. |
| crates/workspace/src/project_manifest/tests.rs | Tests manifest reader behavior for missing/unsupported basenames. |
| crates/workspace/src/projects.rs | Enumerates workspace projects by glob-expanding packages: (via wax) with ignores and sorting. |
| crates/workspace/src/projects/tests.rs | Tests glob expansion, root inclusion, ignores, dedupe, and defaults. |
| crates/workspace/Cargo.toml | Declares the new pacquet-workspace crate and its dependencies. |
| crates/package-manager/src/install.rs | Resolves install prefix via find_workspace_dir to match pnpm’s lockfileDir. |
| crates/package-manager/src/install_frozen_lockfile.rs | Threads workspace root into direct-dependency symlinking stage. |
| crates/package-manager/src/symlink_direct_dependencies.rs | Fans out direct-dep symlinking across all lockfile importers; adds link: handling and per-importer prefix emits. |
| crates/package-manager/src/symlink_direct_dependencies/tests.rs | Adds coverage for link: cross-importer symlinks, per-importer prefixes, and empty-importers no-op. |
| crates/package-manager/src/build_sequence.rs | Skips link: deps when collecting snapshot-root build entries. |
| crates/package-manager/src/build_sequence/tests.rs | Updates tests for new importer dep version type. |
| crates/package-manager/src/build_modules/tests.rs | Updates tests for new importer dep version type. |
| crates/package-manager/Cargo.toml | Adds pacquet-workspace dependency. |
| crates/lockfile/src/resolved_dependency.rs | Introduces ImporterDepVersion (Regular vs Link) for importer-level deps; custom serde + parsing. |
| crates/lockfile/src/resolved_dependency/tests.rs | Adds tests for parsing/serde of ImporterDepVersion and ResolvedDependencySpec. |
| crates/lockfile/src/save_lockfile/tests.rs | Adds workspace lockfile round-trip fixture with a link: dep. |
| Cargo.toml | Registers pacquet-workspace in workspace deps and adds wax = 0.7.0. |
| Cargo.lock | Locks new dependency graph for wax and the new crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/lockfile/src/resolved_dependency/tests.rs (1)
10-10: ⚡ Quick winAdd failure context to non-
assert_eq!assertions.These
assert!checks are currently opaque on failure; add assertion messages (or nearby logging) so failures are diagnosable without reruns.Proposed update
- assert!(parsed.as_link_target().is_none()); + assert!( + parsed.as_link_target().is_none(), + "expected regular version to have no link target, got: {parsed:?}" + ); - assert!(matches!(parsed, ImporterDepVersion::Regular(_))); + assert!( + matches!(parsed, ImporterDepVersion::Regular(_)), + "expected Regular variant, got: {parsed:?}" + ); - assert!(parsed.as_regular().is_none()); + assert!( + parsed.as_regular().is_none(), + "expected link version to have no regular value, got: {parsed:?}" + ); - assert!(spec.version.as_regular().is_some()); + assert!( + spec.version.as_regular().is_some(), + "expected regular importer version, got: {:?}", + spec.version + );Based on learnings: In Rust test code, add logging/context for
assert!/assert_ne!failures unless usingassert_eq!.Also applies to: 19-19, 28-28, 56-56
🤖 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 `@crates/lockfile/src/resolved_dependency/tests.rs` at line 10, Replace opaque assert! checks in the tests with assertions that include failure context: for each occurrence (e.g. the expression parsed.as_link_target().is_none(), other assert! uses at the mentioned positions) change to an assertion that prints the actual value on failure—either use assert_eq!(None, parsed.as_link_target(), "parsed.as_link_target() should be None but was: {:?}", parsed.as_link_target()) or add a message to assert!(parsed.as_link_target().is_none(), "expected no link target, got: {:?}", parsed.as_link_target()); apply the same pattern to the other assert! / assert_ne! occurrences noted so failures show the offending value.crates/package-manager/src/symlink_direct_dependencies/tests.rs (1)
282-288: ⚡ Quick winAssert the symlink destination, not just that it exists.
This check can pass even if the link points to the wrong directory. Add a canonicalized target assertion to pin the “sibling rootDir (not virtual store)” contract.
Proposed update
let symlink_path = workspace_root.join("packages/web/node_modules/shared"); assert!( is_symlink_or_junction(&symlink_path).unwrap(), "expected a symlink at {symlink_path:?}", ); + let actual_target = fs::canonicalize(&symlink_path).expect("resolve symlink target"); + let expected_target = fs::canonicalize(&shared_dir).expect("resolve shared project dir"); + assert_eq!( + actual_target, expected_target, + "workspace link should target sibling project rootDir" + );🤖 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 `@crates/package-manager/src/symlink_direct_dependencies/tests.rs` around lines 282 - 288, The test currently only checks that symlink_path exists as a symlink; update it to also verify the symlink target is the sibling package rootDir: use std::fs::read_link on symlink_path (or equivalent helper) and canonicalize that result, canonicalize the expected sibling root (e.g., workspace_root.join("packages/shared") or the variable representing the sibling rootDir), and assert they are equal so the link points at the sibling's rootDir (not the virtual store); keep the existing is_symlink_or_junction check and add this canonicalized-target assertion.crates/workspace/src/root_finder/tests.rs (1)
10-67: ⚡ Quick winAdd a focused test for
NPM_CONFIG_WORKSPACE_DIRoverride behavior.Given this PR’s new root-finding behavior, a dedicated test for env-var precedence/fallback would harden against regressions in Stage 2 follow-ups.
🤖 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 `@crates/workspace/src/root_finder/tests.rs` around lines 10 - 67, Add a focused unit test in crates/workspace/src/root_finder/tests.rs that verifies the new environment-variable precedence: set NPM_CONFIG_WORKSPACE_DIR to a path (use TempDir) and assert find_workspace_dir returns that dir even if pnpm-workspace.yaml exists elsewhere; also add sub-cases for an invalid/nonexistent NPM_CONFIG_WORKSPACE_DIR value to ensure find_workspace_dir falls back to normal discovery (e.g., finds ancestor manifest or returns None) and for an empty/unset variable; reference the find_workspace_dir function and the NPM_CONFIG_WORKSPACE_DIR symbol so the test explicitly exercises the env-var override and its fallback behavior.
🤖 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 `@crates/package-manager/src/install.rs`:
- Around line 110-111: The current assignment to the variable `prefix` uses
`workspace_root.to_str().expect(...)` which panics on non-UTF-8 paths; change
the logic that sets `prefix` (the `prefix` binding in install.rs) to use a
lossless conversion such as `to_string_lossy().into_owned()` so non-UTF-8
workspace paths don't cause a panic and logging still receives a usable string.
In `@crates/package-manager/src/symlink_direct_dependencies.rs`:
- Line 211: The code currently calls symlink_package(&target_path,
&modules_dir.join(&name_str)).expect("symlink pkg") inside a par_iter which will
panic on failure; change this to propagate errors instead: have the closure
return a Result for each item (e.g., Result<(), Error>) rather than calling
expect, call symlink_package(...) ? to propagate its error, and then collect or
try_for_each over the par_iter results to aggregate/early-return a single Result
from the parent function; reference symlink_package,
modules_dir.join(&name_str), and the par_iter closure so you replace the expect
panic with proper Result propagation (or send errors through a shared channel
and abort early) and update the parent function signature to return a Result if
needed.
---
Nitpick comments:
In `@crates/lockfile/src/resolved_dependency/tests.rs`:
- Line 10: Replace opaque assert! checks in the tests with assertions that
include failure context: for each occurrence (e.g. the expression
parsed.as_link_target().is_none(), other assert! uses at the mentioned
positions) change to an assertion that prints the actual value on failure—either
use assert_eq!(None, parsed.as_link_target(), "parsed.as_link_target() should be
None but was: {:?}", parsed.as_link_target()) or add a message to
assert!(parsed.as_link_target().is_none(), "expected no link target, got: {:?}",
parsed.as_link_target()); apply the same pattern to the other assert! /
assert_ne! occurrences noted so failures show the offending value.
In `@crates/package-manager/src/symlink_direct_dependencies/tests.rs`:
- Around line 282-288: The test currently only checks that symlink_path exists
as a symlink; update it to also verify the symlink target is the sibling package
rootDir: use std::fs::read_link on symlink_path (or equivalent helper) and
canonicalize that result, canonicalize the expected sibling root (e.g.,
workspace_root.join("packages/shared") or the variable representing the sibling
rootDir), and assert they are equal so the link points at the sibling's rootDir
(not the virtual store); keep the existing is_symlink_or_junction check and add
this canonicalized-target assertion.
In `@crates/workspace/src/root_finder/tests.rs`:
- Around line 10-67: Add a focused unit test in
crates/workspace/src/root_finder/tests.rs that verifies the new
environment-variable precedence: set NPM_CONFIG_WORKSPACE_DIR to a path (use
TempDir) and assert find_workspace_dir returns that dir even if
pnpm-workspace.yaml exists elsewhere; also add sub-cases for an
invalid/nonexistent NPM_CONFIG_WORKSPACE_DIR value to ensure find_workspace_dir
falls back to normal discovery (e.g., finds ancestor manifest or returns None)
and for an empty/unset variable; reference the find_workspace_dir function and
the NPM_CONFIG_WORKSPACE_DIR symbol so the test explicitly exercises the env-var
override and its fallback behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bb2bc9b-8bc1-4078-87cd-5414295c9633
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Cargo.tomlcrates/lockfile/src/resolved_dependency.rscrates/lockfile/src/resolved_dependency/tests.rscrates/lockfile/src/save_lockfile/tests.rscrates/package-manager/Cargo.tomlcrates/package-manager/src/build_modules/tests.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/build_sequence/tests.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/workspace/Cargo.tomlcrates/workspace/src/lib.rscrates/workspace/src/manifest.rscrates/workspace/src/manifest/tests.rscrates/workspace/src/project_manifest.rscrates/workspace/src/project_manifest/tests.rscrates/workspace/src/projects.rscrates/workspace/src/projects/tests.rscrates/workspace/src/root_finder.rscrates/workspace/src/root_finder/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). (8)
- GitHub Check: Agent
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Dylint
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/build_modules/tests.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/build_sequence/tests.rscrates/lockfile/src/save_lockfile/tests.rscrates/package-manager/src/install.rscrates/workspace/src/manifest/tests.rscrates/lockfile/src/resolved_dependency/tests.rscrates/workspace/src/lib.rscrates/workspace/src/manifest.rscrates/workspace/src/root_finder.rscrates/package-manager/src/install_frozen_lockfile.rscrates/workspace/src/project_manifest/tests.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/workspace/src/project_manifest.rscrates/workspace/src/projects/tests.rscrates/lockfile/src/resolved_dependency.rscrates/workspace/src/root_finder/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/workspace/src/projects.rs
🧠 Learnings (2)
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/build_modules/tests.rscrates/package-manager/src/build_sequence/tests.rscrates/lockfile/src/save_lockfile/tests.rscrates/workspace/src/manifest/tests.rscrates/lockfile/src/resolved_dependency/tests.rscrates/workspace/src/project_manifest/tests.rscrates/workspace/src/projects/tests.rscrates/workspace/src/root_finder/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/build_modules/tests.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/build_sequence/tests.rscrates/lockfile/src/save_lockfile/tests.rscrates/package-manager/src/install.rscrates/workspace/src/manifest/tests.rscrates/lockfile/src/resolved_dependency/tests.rscrates/workspace/src/lib.rscrates/workspace/src/manifest.rscrates/workspace/src/root_finder.rscrates/package-manager/src/install_frozen_lockfile.rscrates/workspace/src/project_manifest/tests.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/workspace/src/project_manifest.rscrates/workspace/src/projects/tests.rscrates/lockfile/src/resolved_dependency.rscrates/workspace/src/root_finder/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/workspace/src/projects.rs
🔇 Additional comments (23)
crates/lockfile/src/resolved_dependency.rs (1)
1-165: LGTM!crates/package-manager/src/build_modules/tests.rs (1)
252-255: LGTM!crates/package-manager/src/build_sequence/tests.rs (1)
45-48: LGTM!crates/package-manager/src/build_sequence.rs (1)
141-150: LGTM!crates/lockfile/src/save_lockfile/tests.rs (1)
87-162: LGTM!crates/package-manager/src/install_frozen_lockfile.rs (1)
18-22: LGTM!Also applies to: 47-54, 153-161
crates/package-manager/src/symlink_direct_dependencies.rs (6)
1-17: LGTM!
19-53: LGTM!
55-60: LGTM!
62-101: LGTM!
103-118: LGTM!
262-274: LGTM!crates/workspace/src/manifest.rs (1)
1-136: LGTM!crates/workspace/src/root_finder.rs (1)
1-137: LGTM!crates/workspace/src/project_manifest.rs (1)
1-107: LGTM!crates/workspace/src/project_manifest/tests.rs (1)
1-68: LGTM!crates/workspace/src/projects.rs (1)
1-230: LGTM!crates/workspace/src/projects/tests.rs (1)
1-118: LGTM!crates/workspace/src/lib.rs (1)
1-38: LGTM!crates/workspace/src/manifest/tests.rs (1)
1-66: LGTM!crates/package-manager/Cargo.toml (1)
28-28: LGTM!Cargo.toml (1)
34-34: LGTM!Also applies to: 89-89
crates/workspace/Cargo.toml (1)
1-24: LGTM!
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4831201633599997,
"stddev": 0.04167633079208864,
"median": 2.4689126526600003,
"user": 2.60659114,
"system": 3.4385569399999993,
"min": 2.43738637116,
"max": 2.55940021516,
"times": [
2.45252438316,
2.49372909516,
2.55940021516,
2.51041505516,
2.4588666371600003,
2.47470540216,
2.46311990316,
2.53931036416,
2.43738637116,
2.44174420716
]
},
{
"command": "pacquet@main",
"mean": 2.4764405917600003,
"stddev": 0.07324214662777033,
"median": 2.4670707701600003,
"user": 2.59762084,
"system": 3.4258058399999998,
"min": 2.3571206081600002,
"max": 2.57800792016,
"times": [
2.57800792016,
2.47191327716,
2.40613471616,
2.40923732116,
2.5502315121600003,
2.3571206081600002,
2.52968021516,
2.45164480916,
2.46222826316,
2.5482072751600002
]
},
{
"command": "pnpm",
"mean": 5.93899265436,
"stddev": 0.10579744400951284,
"median": 5.92203868116,
"user": 8.66512554,
"system": 4.30418624,
"min": 5.80863392416,
"max": 6.12215791416,
"times": [
6.0526980331599995,
6.01267010616,
5.885087728159999,
6.12215791416,
5.870042124159999,
5.817394129159999,
5.99771457716,
5.80863392416,
5.95898963416,
5.864538373159999
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.68791756452,
"stddev": 0.02754256367741308,
"median": 0.6740233957199999,
"user": 0.3590085,
"system": 1.5118548200000004,
"min": 0.66735246222,
"max": 0.75472353422,
"times": [
0.75472353422,
0.67264191422,
0.6720331442199999,
0.67224584522,
0.67540487722,
0.70329413422,
0.70916553622,
0.66948583322,
0.68282836422,
0.66735246222
]
},
{
"command": "pacquet@main",
"mean": 0.7994735266199999,
"stddev": 0.09329072902994748,
"median": 0.78081436322,
"user": 0.3531575,
"system": 1.5150295200000001,
"min": 0.66224441522,
"max": 0.95305915122,
"times": [
0.94752299122,
0.74549254422,
0.83707888022,
0.7672540772199999,
0.76379389222,
0.66224441522,
0.95305915122,
0.79437464922,
0.80944026422,
0.7144744012199999
]
},
{
"command": "pnpm",
"mean": 2.40861705902,
"stddev": 0.08604618885942437,
"median": 2.38809026472,
"user": 2.8847140999999996,
"system": 2.1824650199999995,
"min": 2.32789497722,
"max": 2.63378423722,
"times": [
2.63378423722,
2.34025797422,
2.3659717002200003,
2.4274157502200002,
2.32789497722,
2.37841632222,
2.3732966822200003,
2.4190129712200004,
2.42235576822,
2.3977642072200003
]
}
]
} |
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Doc job (blocking):
- Replace private intra-doc links to module names in
`crates/workspace/src/lib.rs` with text + linked public items.
- Drop two `[`pacquet_config::WorkspaceSettings`]` rustdoc links in
`crates/workspace/src/manifest.rs` — `pacquet-config` is not a
dependency of `pacquet-workspace`, so the links were broken.
Review feedback:
- `install.rs:111` — replace `to_str().expect(...)` with
`to_string_lossy().into_owned()` so non-UTF-8 workspace paths do
not panic the installer.
- `symlink_direct_dependencies.rs` —
- Validate every importer key (`validate_importer_id`) and reject
absolute paths, Windows drive prefixes, backslash separators, and
`..` segments with `SymlinkDirectDependenciesError::UnsafeImporterPath`.
A malformed lockfile would otherwise let `Path::join` create
`node_modules` outside the workspace root.
- Replace `symlink_package(...).expect("symlink pkg")` with a
fallible `try_for_each` path and a new
`SymlinkDirectDependenciesError::SymlinkPackage` variant so a per-
package symlink failure surfaces as a structured error instead of
panicking the rayon worker.
- Fix the struct-level doc comment to refer to
`<workspace_root>/node_modules/.pacquet` (pacquet's actual virtual
store dir) rather than `.pnpm`.
- `projects.rs:186` — restrict the manifest-read swallow to
`ENOENT`-equivalent errors (`Io(NotFound)` and
`NoImporterManifestFound`); parse / permission / other I/O errors
now propagate via `FindWorkspaceProjectsError::ReadManifest`.
- `root_finder.rs` — treat an empty `NPM_CONFIG_WORKSPACE_DIR` as
unset so the upward walk takes over, matching upstream's truthy
`if (workspaceDir)` check.
- `project_manifest.rs` — drop the misleading mention of
`package.yaml` / `package.json5` from `NoImporterManifestFound`;
pacquet only probes `package.json` today.
- `manifest.rs` — remove the unreachable
`InvalidWorkspaceManifestError::PackagesNotArray` variant; serde
already rejects non-array shapes at parse time.
- `symlink_direct_dependencies/tests.rs` — drop leftover `dbg!`.
New tests:
- `pacquet-package-manager`: `unsafe_importer_keys_error_before_filesystem_writes`
covers six unsafe importer-key shapes.
- `pacquet-workspace`: `empty_env_var_is_treated_as_unset` covers
the `NPM_CONFIG_WORKSPACE_DIR=""` fallback.
Verified locally: `just ready` (649 passed), `taplo format --check`,
`just dylint`, and `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --document-private-items`
all clean.
---
Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/workspace/src/manifest.rs (1)
4-4: ⚡ Quick winUse
pnpmblob/mainlinks instead of pinned SHA permalinks in module docs.These references will drift from the source-of-truth guidance for ports; switch both links to
https://github.com/pnpm/pnpm/blob/main/....Suggested doc-link update
-//! [`readWorkspaceManifest`](https://github.com/pnpm/pnpm/blob/94240bc046/workspace/workspace-manifest-reader/src/index.ts). +//! [`readWorkspaceManifest`](https://github.com/pnpm/pnpm/blob/main/workspace/workspace-manifest-reader/src/index.ts). ... -//! [`validateWorkspaceManifest`](https://github.com/pnpm/pnpm/blob/94240bc046/workspace/workspace-manifest-reader/src/index.ts): +//! [`validateWorkspaceManifest`](https://github.com/pnpm/pnpm/blob/main/workspace/workspace-manifest-reader/src/index.ts):As per coding guidelines: "When porting features... open files from
https://github.com/pnpm/pnpm/blob/main/...rather than from a permalinked SHA."Also applies to: 16-16
🤖 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 `@crates/workspace/src/manifest.rs` at line 4, The crate doc reference to the PNPM source uses a permalinked SHA; update the module-level doc links that reference readWorkspaceManifest (and the second occurrence at line 16) to use the canonical blob/main path (https://github.com/pnpm/pnpm/blob/main/...) instead of the pinned SHA permalink so both references point to the live main branch.crates/package-manager/src/symlink_direct_dependencies/tests.rs (1)
222-228: ⚡ Quick winUse
pnpmblob/mainURLs in porting references.These comments pin upstream links to a specific SHA. For pnpm-porting references in this repo, switch to
https://github.com/pnpm/pnpm/blob/main/...so the source-of-truth stays current.As per coding guidelines, "If reading on GitHub, open files from
https://github.com/pnpm/pnpm/blob/main/...... rather than from a permalinked SHA."Also applies to: 343-347
🤖 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 `@crates/package-manager/src/symlink_direct_dependencies/tests.rs` around lines 222 - 228, The comment currently references an upstream pnpm link with a permalined SHA (the URL in the block comment that points to the specific commit/sha); update that URL to use the canonical blob/main path (https://github.com/pnpm/pnpm/blob/main/...) so the reference tracks the main branch, and make the same replacement for the other occurrence noted around the comment block (the similar link mentioned at lines ~343-347) so both upstream links use blob/main.
🤖 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 `@crates/package-manager/src/symlink_direct_dependencies/tests.rs`:
- Around line 434-481: The test currently only asserts the error but must also
assert no filesystem writes; after verifying the result is
Err(SymlinkDirectDependenciesError::UnsafeImporterPath {..}) in
unsafe_importer_keys_error_before_filesystem_writes, add an assertion that the
modules directory was not created (e.g.,
assert!(!workspace_root.join("node_modules").exists()) or
assert!(!config.modules_dir.exists())) to ensure no node_modules was written for
each importer_id before dropping dir.
---
Nitpick comments:
In `@crates/package-manager/src/symlink_direct_dependencies/tests.rs`:
- Around line 222-228: The comment currently references an upstream pnpm link
with a permalined SHA (the URL in the block comment that points to the specific
commit/sha); update that URL to use the canonical blob/main path
(https://github.com/pnpm/pnpm/blob/main/...) so the reference tracks the main
branch, and make the same replacement for the other occurrence noted around the
comment block (the similar link mentioned at lines ~343-347) so both upstream
links use blob/main.
In `@crates/workspace/src/manifest.rs`:
- Line 4: The crate doc reference to the PNPM source uses a permalinked SHA;
update the module-level doc links that reference readWorkspaceManifest (and the
second occurrence at line 16) to use the canonical blob/main path
(https://github.com/pnpm/pnpm/blob/main/...) instead of the pinned SHA permalink
so both references point to the live main branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d697e62-010f-4069-a2f5-1ecafa305a5c
📒 Files selected for processing (9)
crates/package-manager/src/install.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/workspace/src/lib.rscrates/workspace/src/manifest.rscrates/workspace/src/project_manifest.rscrates/workspace/src/projects.rscrates/workspace/src/root_finder.rscrates/workspace/src/root_finder/tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/workspace/src/lib.rs
- crates/workspace/src/projects.rs
- crates/workspace/src/project_manifest.rs
- crates/package-manager/src/symlink_direct_dependencies.rs
- crates/workspace/src/root_finder.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). (5)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/install.rscrates/workspace/src/manifest.rscrates/workspace/src/root_finder/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
🧠 Learnings (2)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/install.rscrates/workspace/src/manifest.rscrates/workspace/src/root_finder/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/workspace/src/root_finder/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
🔇 Additional comments (4)
crates/workspace/src/manifest.rs (1)
49-57: LGTM!Also applies to: 68-94, 101-136
crates/workspace/src/root_finder/tests.rs (1)
1-95: LGTM!crates/package-manager/src/install.rs (1)
1-1: LGTM!Also applies to: 62-65, 99-117
crates/package-manager/src/symlink_direct_dependencies/tests.rs (1)
9-200: LGTM!Also applies to: 230-341, 348-433
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… keys Strengthens `unsafe_importer_keys_error_before_filesystem_writes` to match its name: in addition to the error-type check, it now asserts that no `node_modules` directory is created under `workspace_root` *or* its parent for each rejected importer key. The parent-side check guards against a regression where `Path::join` would let a `..` or absolute key land outside the workspace root. Addresses CodeRabbit feedback on PR #443. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…ring round-trip `Install::run` converts the resolved workspace root to a lossy-UTF-8 string for reporter envelopes (`prefix`) and passes that to `InstallFrozenLockfile::requester`. The previous code then reconstructed `PathBuf::from(requester)` inside `InstallFrozenLockfile::run` to feed `SymlinkDirectDependencies` and `BuildModules` — meaning real filesystem operations were driven by a path that had already passed through a lossy conversion. On hosts with non-UTF-8 filenames, the on-disk path could silently diverge from the workspace root pacquet meant to install under. Add a `workspace_root: &'a Path` field on `InstallFrozenLockfile` threaded straight from `Install::run` (which still holds the original `PathBuf` returned by `find_workspace_dir`). The lossy `requester` string remains for reporter envelopes only; filesystem-effecting code paths (`SymlinkDirectDependencies::workspace_root`, `BuildModules`'s `lockfile_dir`) now read the real path directly. Caught by Copilot in PR #443 review of crates/package-manager/src/install_frozen_lockfile.rs:158. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Doc job (blocking):
- Replace private intra-doc links to module names in
`crates/workspace/src/lib.rs` with text + linked public items.
- Drop two `[`pacquet_config::WorkspaceSettings`]` rustdoc links in
`crates/workspace/src/manifest.rs` — `pacquet-config` is not a
dependency of `pacquet-workspace`, so the links were broken.
Review feedback:
- `install.rs:111` — replace `to_str().expect(...)` with
`to_string_lossy().into_owned()` so non-UTF-8 workspace paths do
not panic the installer.
- `symlink_direct_dependencies.rs` —
- Validate every importer key (`validate_importer_id`) and reject
absolute paths, Windows drive prefixes, backslash separators, and
`..` segments with `SymlinkDirectDependenciesError::UnsafeImporterPath`.
A malformed lockfile would otherwise let `Path::join` create
`node_modules` outside the workspace root.
- Replace `symlink_package(...).expect("symlink pkg")` with a
fallible `try_for_each` path and a new
`SymlinkDirectDependenciesError::SymlinkPackage` variant so a per-
package symlink failure surfaces as a structured error instead of
panicking the rayon worker.
- Fix the struct-level doc comment to refer to
`<workspace_root>/node_modules/.pacquet` (pacquet's actual virtual
store dir) rather than `.pnpm`.
- `projects.rs:186` — restrict the manifest-read swallow to
`ENOENT`-equivalent errors (`Io(NotFound)` and
`NoImporterManifestFound`); parse / permission / other I/O errors
now propagate via `FindWorkspaceProjectsError::ReadManifest`.
- `root_finder.rs` — treat an empty `NPM_CONFIG_WORKSPACE_DIR` as
unset so the upward walk takes over, matching upstream's truthy
`if (workspaceDir)` check.
- `project_manifest.rs` — drop the misleading mention of
`package.yaml` / `package.json5` from `NoImporterManifestFound`;
pacquet only probes `package.json` today.
- `manifest.rs` — remove the unreachable
`InvalidWorkspaceManifestError::PackagesNotArray` variant; serde
already rejects non-array shapes at parse time.
- `symlink_direct_dependencies/tests.rs` — drop leftover `dbg!`.
New tests:
- `pacquet-package-manager`: `unsafe_importer_keys_error_before_filesystem_writes`
covers six unsafe importer-key shapes.
- `pacquet-workspace`: `empty_env_var_is_treated_as_unset` covers
the `NPM_CONFIG_WORKSPACE_DIR=""` fallback.
Verified locally: `just ready` (649 passed), `taplo format --check`,
`just dylint`, and `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --document-private-items`
all clean.
---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… keys Strengthens `unsafe_importer_keys_error_before_filesystem_writes` to match its name: in addition to the error-type check, it now asserts that no `node_modules` directory is created under `workspace_root` *or* its parent for each rejected importer key. The parent-side check guards against a regression where `Path::join` would let a `..` or absolute key land outside the workspace root. Addresses CodeRabbit feedback on PR #443. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…ring round-trip `Install::run` converts the resolved workspace root to a lossy-UTF-8 string for reporter envelopes (`prefix`) and passes that to `InstallFrozenLockfile::requester`. The previous code then reconstructed `PathBuf::from(requester)` inside `InstallFrozenLockfile::run` to feed `SymlinkDirectDependencies` and `BuildModules` — meaning real filesystem operations were driven by a path that had already passed through a lossy conversion. On hosts with non-UTF-8 filenames, the on-disk path could silently diverge from the workspace root pacquet meant to install under. Add a `workspace_root: &'a Path` field on `InstallFrozenLockfile` threaded straight from `Install::run` (which still holds the original `PathBuf` returned by `find_workspace_dir`). The lossy `requester` string remains for reporter envelopes only; filesystem-effecting code paths (`SymlinkDirectDependencies::workspace_root`, `BuildModules`'s `lockfile_dir`) now read the real path directly. Caught by Copilot in PR #443 review of crates/package-manager/src/install_frozen_lockfile.rs:158. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Two items from the 2026-05-13 Copilot review:
1. `validate_importer_id` previously accepted an empty importer key as
equivalent to `"."`. Pnpm only ever writes `.` for the root importer;
an empty-string key is non-standard and could mask a malformed
lockfile. Reject it with `UnsafeImporterPath` instead. The
`importer_root_dir` lookup is correspondingly tightened to the
single canonical root key; the empty-key fallback is unreachable
after validation. New test case (empty string) added to
`unsafe_importer_keys_error_before_filesystem_writes`.
2. `empty_env_var_is_treated_as_unset` was calling `unsafe { env::set_var }`,
which is documented UB when other threads access the process
environment concurrently — and Rust's default test runner is
multi-threaded. Refactor `find_workspace_dir_from_env` to accept an
injected env accessor (`find_workspace_dir_from_env_with`) and have
the production path call it with `std::env::var_os`. Tests now
exercise the fall-through, truthy-value, and lowercase-fallback
branches via pure in-memory closures with no `set_var` calls.
---
Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/workspace/src/project_manifest/tests.rs (1)
23-32: ⚡ Quick winAdd diagnostic logging before non-
assert_eq!assertions.Line 25 and Line 31 use
assert!(...is_none())without logging the evaluated result, which makes failures harder to diagnose in CI output.Proposed patch
#[test] fn try_read_returns_none_when_missing() { let tmp = TempDir::new().unwrap(); - assert!(try_read_project_manifest(tmp.path()).unwrap().is_none()); + let result = try_read_project_manifest(tmp.path()).unwrap(); + eprintln!("try_read_project_manifest({:?}) => {:?}", tmp.path(), result); + assert!(result.is_none()); } #[test] fn safe_read_returns_none_when_missing() { let tmp = TempDir::new().unwrap(); - assert!(safe_read_project_manifest_only(tmp.path()).unwrap().is_none()); + let result = safe_read_project_manifest_only(tmp.path()).unwrap(); + eprintln!( + "safe_read_project_manifest_only({:?}) => {:?}", + tmp.path(), + result + ); + assert!(result.is_none()); }Based on learnings: In Rust test code, add logging around non-
assert_eq!assertions so failures are diagnosable without rerunning.🤖 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 `@crates/workspace/src/project_manifest/tests.rs` around lines 23 - 32, Capture the result of try_read_project_manifest(tmp.path()) and safe_read_project_manifest_only(tmp.path()) into a local variable inside the tests try_read_returns_none_when_missing and safe_read_returns_none_when_missing, emit diagnostic output (e.g., with eprintln! or dbg!) showing the evaluated value, then perform the assertion (prefer assert_eq!(res, None) or assert!(res.is_none())). This ensures you log the computed value from try_read_project_manifest and safe_read_project_manifest_only before asserting so failures are diagnosable in CI.crates/workspace/src/projects/tests.rs (1)
56-77: ⚡ Quick winAdd
bower_componentsexclusion coverage in this filter test.Given this stage’s behavior includes filtering both
node_modulesandbower_components, adding a sibling assertion here would better protect against regressions.Suggested patch
fn filters_node_modules() { let tmp = TempDir::new().unwrap(); make_project(tmp.path(), ".", "root"); make_project(tmp.path(), "node_modules/foo", "foo"); + make_project(tmp.path(), "bower_components/bar", "bar"); make_project(tmp.path(), "packages/real", "real"); @@ assert!( !names.contains(&"foo".to_string()), "node_modules contents must not surface as workspace projects: {names:?}", ); + assert!( + !names.contains(&"bar".to_string()), + "bower_components contents must not surface as workspace projects: {names:?}", + ); assert!(names.contains(&"real".to_string())); }🤖 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 `@crates/workspace/src/projects/tests.rs` around lines 56 - 77, The test filters_node_modules should also create a sample project inside "bower_components" (e.g., call make_project(tmp.path(), "bower_components/bar", "bar")) and then assert the returned workspace names from find_workspace_projects (called with FindWorkspaceProjectsOpts { patterns: Some(vec!["**".to_string()]) }) do NOT contain "bar" (similar to the existing assertion for "foo"), ensuring bower_components is excluded; add that project creation and a corresponding assert!(!names.contains(&"bar".to_string())) after collecting names in the test.crates/lockfile/src/resolved_dependency/tests.rs (1)
10-10: ⚡ Quick winAdd failure context to non-
assert_eq!assertions.These
assert!checks are currently context-free, so failures won’t show the parsed value/state directly. Please add a message (or nearbyeprintln!) for quicker diagnosis.Suggested update
- assert!(parsed.as_link_target().is_none()); + assert!(parsed.as_link_target().is_none(), "parsed={parsed:?}"); - assert!(matches!(parsed, ImporterDepVersion::Regular(_))); + assert!( + matches!(parsed, ImporterDepVersion::Regular(_)), + "parsed={parsed:?}" + ); - assert!(parsed.as_regular().is_none()); + assert!(parsed.as_regular().is_none(), "parsed={parsed:?}"); - assert!(spec.version.as_regular().is_some()); + assert!( + spec.version.as_regular().is_some(), + "spec.version={:?}", + spec.version + );Based on learnings: In Rust test code, add diagnostic logging/context for non-
assert_eq!assertions so failures are debuggable without reruns.Also applies to: 19-19, 28-28, 56-56
🤖 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 `@crates/lockfile/src/resolved_dependency/tests.rs` at line 10, The assert! checks lack failure context — update each to include a diagnostic message showing the actual value so test failures are debuggable; e.g., replace assert!(parsed.as_link_target().is_none()) with assert!(parsed.as_link_target().is_none(), "expected no link target, got: {:?}", parsed.as_link_target()) or print eprintln!("parsed = {:?}", parsed) immediately before the assert; apply the same pattern to the other non-assert_eq! assertions that reference parsed (and its methods like as_link_target()) so failures surface the parsed state.
🤖 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 `@crates/package-manager/src/symlink_direct_dependencies/tests.rs`:
- Line 456: The test assumes workspace_root.parent()/node_modules which can
point to a shared system tempdir; instead create and use an explicit
subdirectory inside the tempdir for the workspace root (e.g. let workspace_root
= dir.path().join("workspace"); fs::create_dir_all(&workspace_root)?), then
update assertions that referenced workspace_root.parent().join("node_modules")
to use workspace_root.join("node_modules") (and any other checks in the same
test that use workspace_root.parent()). This ensures the test only inspects
directories created for the test and avoids shared-parent flakes; update code
that sets or asserts on workspace_root accordingly (variable workspace_root in
symlink_direct_dependencies tests).
In `@crates/workspace/src/projects/tests.rs`:
- Line 76: The assertion on the variable `names` (the line with
assert!(names.contains(&"real".to_string()))) lacks diagnostic context; update
this assertion to include a failure message that prints the expected value
"real" and the current contents of `names` (e.g., use an assert! with a
formatted message showing names and the missing value) so test failures show
actual vs expected for easier debugging.
---
Nitpick comments:
In `@crates/lockfile/src/resolved_dependency/tests.rs`:
- Line 10: The assert! checks lack failure context — update each to include a
diagnostic message showing the actual value so test failures are debuggable;
e.g., replace assert!(parsed.as_link_target().is_none()) with
assert!(parsed.as_link_target().is_none(), "expected no link target, got: {:?}",
parsed.as_link_target()) or print eprintln!("parsed = {:?}", parsed) immediately
before the assert; apply the same pattern to the other non-assert_eq! assertions
that reference parsed (and its methods like as_link_target()) so failures
surface the parsed state.
In `@crates/workspace/src/project_manifest/tests.rs`:
- Around line 23-32: Capture the result of try_read_project_manifest(tmp.path())
and safe_read_project_manifest_only(tmp.path()) into a local variable inside the
tests try_read_returns_none_when_missing and
safe_read_returns_none_when_missing, emit diagnostic output (e.g., with
eprintln! or dbg!) showing the evaluated value, then perform the assertion
(prefer assert_eq!(res, None) or assert!(res.is_none())). This ensures you log
the computed value from try_read_project_manifest and
safe_read_project_manifest_only before asserting so failures are diagnosable in
CI.
In `@crates/workspace/src/projects/tests.rs`:
- Around line 56-77: The test filters_node_modules should also create a sample
project inside "bower_components" (e.g., call make_project(tmp.path(),
"bower_components/bar", "bar")) and then assert the returned workspace names
from find_workspace_projects (called with FindWorkspaceProjectsOpts { patterns:
Some(vec!["**".to_string()]) }) do NOT contain "bar" (similar to the existing
assertion for "foo"), ensuring bower_components is excluded; add that project
creation and a corresponding assert!(!names.contains(&"bar".to_string())) after
collecting names in the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ee1ac94-9a88-4807-88df-f98d4e4c54e5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Cargo.tomlcrates/lockfile/src/resolved_dependency.rscrates/lockfile/src/resolved_dependency/tests.rscrates/lockfile/src/save_lockfile/tests.rscrates/package-manager/Cargo.tomlcrates/package-manager/src/build_modules/tests.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/build_sequence/tests.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/workspace/Cargo.tomlcrates/workspace/src/lib.rscrates/workspace/src/manifest.rscrates/workspace/src/manifest/tests.rscrates/workspace/src/project_manifest.rscrates/workspace/src/project_manifest/tests.rscrates/workspace/src/projects.rscrates/workspace/src/projects/tests.rscrates/workspace/src/root_finder.rscrates/workspace/src/root_finder/tests.rs
✅ Files skipped from review due to trivial changes (4)
- crates/package-manager/src/build_sequence/tests.rs
- crates/workspace/src/lib.rs
- crates/workspace/src/manifest/tests.rs
- crates/package-manager/src/build_modules/tests.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/workspace/Cargo.toml
- crates/package-manager/src/build_sequence.rs
- crates/lockfile/src/save_lockfile/tests.rs
- crates/package-manager/src/install_frozen_lockfile.rs
- crates/workspace/src/root_finder/tests.rs
- crates/lockfile/src/resolved_dependency.rs
- crates/workspace/src/root_finder.rs
- crates/package-manager/src/install.rs
- crates/workspace/src/projects.rs
- crates/workspace/src/manifest.rs
- crates/workspace/src/project_manifest.rs
- crates/package-manager/src/symlink_direct_dependencies.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: Run benchmark on ubuntu-latest
- GitHub Check: Agent
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/lockfile/src/resolved_dependency/tests.rscrates/workspace/src/projects/tests.rscrates/workspace/src/project_manifest/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
🧠 Learnings (2)
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/lockfile/src/resolved_dependency/tests.rscrates/workspace/src/projects/tests.rscrates/workspace/src/project_manifest/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/lockfile/src/resolved_dependency/tests.rscrates/workspace/src/projects/tests.rscrates/workspace/src/project_manifest/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rs
🔇 Additional comments (4)
Cargo.toml (1)
34-34: LGTM!Also applies to: 89-89
crates/package-manager/Cargo.toml (1)
28-28: LGTM!crates/workspace/src/project_manifest/tests.rs (1)
1-20: LGTM!Also applies to: 34-67
crates/workspace/src/projects/tests.rs (1)
1-55: LGTM!Also applies to: 79-117
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Doc job (blocking):
- Replace private intra-doc links to module names in
`crates/workspace/src/lib.rs` with text + linked public items.
- Drop two `[`pacquet_config::WorkspaceSettings`]` rustdoc links in
`crates/workspace/src/manifest.rs` — `pacquet-config` is not a
dependency of `pacquet-workspace`, so the links were broken.
Review feedback:
- `install.rs:111` — replace `to_str().expect(...)` with
`to_string_lossy().into_owned()` so non-UTF-8 workspace paths do
not panic the installer.
- `symlink_direct_dependencies.rs` —
- Validate every importer key (`validate_importer_id`) and reject
absolute paths, Windows drive prefixes, backslash separators, and
`..` segments with `SymlinkDirectDependenciesError::UnsafeImporterPath`.
A malformed lockfile would otherwise let `Path::join` create
`node_modules` outside the workspace root.
- Replace `symlink_package(...).expect("symlink pkg")` with a
fallible `try_for_each` path and a new
`SymlinkDirectDependenciesError::SymlinkPackage` variant so a per-
package symlink failure surfaces as a structured error instead of
panicking the rayon worker.
- Fix the struct-level doc comment to refer to
`<workspace_root>/node_modules/.pacquet` (pacquet's actual virtual
store dir) rather than `.pnpm`.
- `projects.rs:186` — restrict the manifest-read swallow to
`ENOENT`-equivalent errors (`Io(NotFound)` and
`NoImporterManifestFound`); parse / permission / other I/O errors
now propagate via `FindWorkspaceProjectsError::ReadManifest`.
- `root_finder.rs` — treat an empty `NPM_CONFIG_WORKSPACE_DIR` as
unset so the upward walk takes over, matching upstream's truthy
`if (workspaceDir)` check.
- `project_manifest.rs` — drop the misleading mention of
`package.yaml` / `package.json5` from `NoImporterManifestFound`;
pacquet only probes `package.json` today.
- `manifest.rs` — remove the unreachable
`InvalidWorkspaceManifestError::PackagesNotArray` variant; serde
already rejects non-array shapes at parse time.
- `symlink_direct_dependencies/tests.rs` — drop leftover `dbg!`.
New tests:
- `pacquet-package-manager`: `unsafe_importer_keys_error_before_filesystem_writes`
covers six unsafe importer-key shapes.
- `pacquet-workspace`: `empty_env_var_is_treated_as_unset` covers
the `NPM_CONFIG_WORKSPACE_DIR=""` fallback.
Verified locally: `just ready` (649 passed), `taplo format --check`,
`just dylint`, and `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --document-private-items`
all clean.
---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… keys Strengthens `unsafe_importer_keys_error_before_filesystem_writes` to match its name: in addition to the error-type check, it now asserts that no `node_modules` directory is created under `workspace_root` *or* its parent for each rejected importer key. The parent-side check guards against a regression where `Path::join` would let a `..` or absolute key land outside the workspace root. Addresses CodeRabbit feedback on PR #443. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…ring round-trip `Install::run` converts the resolved workspace root to a lossy-UTF-8 string for reporter envelopes (`prefix`) and passes that to `InstallFrozenLockfile::requester`. The previous code then reconstructed `PathBuf::from(requester)` inside `InstallFrozenLockfile::run` to feed `SymlinkDirectDependencies` and `BuildModules` — meaning real filesystem operations were driven by a path that had already passed through a lossy conversion. On hosts with non-UTF-8 filenames, the on-disk path could silently diverge from the workspace root pacquet meant to install under. Add a `workspace_root: &'a Path` field on `InstallFrozenLockfile` threaded straight from `Install::run` (which still holds the original `PathBuf` returned by `find_workspace_dir`). The lossy `requester` string remains for reporter envelopes only; filesystem-effecting code paths (`SymlinkDirectDependencies::workspace_root`, `BuildModules`'s `lockfile_dir`) now read the real path directly. Caught by Copilot in PR #443 review of crates/package-manager/src/install_frozen_lockfile.rs:158. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Two items from the 2026-05-13 Copilot review:
1. `validate_importer_id` previously accepted an empty importer key as
equivalent to `"."`. Pnpm only ever writes `.` for the root importer;
an empty-string key is non-standard and could mask a malformed
lockfile. Reject it with `UnsafeImporterPath` instead. The
`importer_root_dir` lookup is correspondingly tightened to the
single canonical root key; the empty-key fallback is unreachable
after validation. New test case (empty string) added to
`unsafe_importer_keys_error_before_filesystem_writes`.
2. `empty_env_var_is_treated_as_unset` was calling `unsafe { env::set_var }`,
which is documented UB when other threads access the process
environment concurrently — and Rust's default test runner is
multi-threaded. Refactor `find_workspace_dir_from_env` to accept an
injected env accessor (`find_workspace_dir_from_env_with`) and have
the production path call it with `std::env::var_os`. Tests now
exercise the fall-through, truthy-value, and lowercase-fallback
branches via pure in-memory closures with no `set_var` calls.
---
Written by an agent (Claude Code, claude-opus-4-7).
Stage 1 of pnpm/pacquet#299: enumerate `Lockfile.importers` and materialise per-project `node_modules/` against the shared virtual store. Ports four upstream packages into a new `pacquet-workspace` crate (`workspace/root-finder`, `workspace/workspace-manifest-reader`, `workspace/projects-reader`, `workspace/project-manifest-reader` — pinned to pnpm/pnpm@94240bc046) and refactors the installer so the "missing root importer" hard-failure becomes per-importer iteration. New crate: `pacquet-workspace` - `find_workspace_dir`: walk up to `pnpm-workspace.yaml`, honour `NPM_CONFIG_WORKSPACE_DIR`, reject misnamed variants (`pnpm-workspace.yml`, `.pnpm-workspace.yaml`, …) with the same `BAD_WORKSPACE_MANIFEST_NAME` upstream raises. - `read_workspace_manifest`: parse `packages:` (and tolerate settings-only manifests). - `find_workspace_projects` / `find_workspace_projects_no_check`: glob-expand `packages:` via `wax`, always include the workspace root, filter `**/node_modules/**` and `**/bower_components/**`, sort lex by `rootDir`. - `read_project_manifest_only` / `try_read_project_manifest` / `read_exact_project_manifest`: thin wrappers over pacquet's existing `PackageManifest::from_path`. Installer changes: - `SymlinkDirectDependencies` now takes `workspace_root` and iterates every importer (sorted for determinism). Per-importer `node_modules/` and `pnpm:root added` `prefix = rootDir` match upstream's `linkDirectDeps`. The install-wide `prefix` (pnpm:stage, pnpm:summary) stays at `lockfileDir` as upstream does — partially closes #357 by resolving that path through `find_workspace_dir`. - `Lockfile.importers[*].dependencies` now decodes `link:<path>` values into a new `ImporterDepVersion::Link` variant. The symlink stage resolves the path against the importer's `rootDir` and points `node_modules/<name>` at the sibling project rather than the virtual store. `link:` deps are also skipped when building the snapshot-key roots for `BuildModules`. - `MissingRootImporter` is gone — an empty importers map is a silent no-op now, mirroring pnpm's per-importer treatment. Adds `wax = 0.7.0` to `[workspace.dependencies]` for project globbing. Tests added: - `pacquet-workspace`: 21 unit tests covering env-var override, misnamed-variant rejection, settings-only manifests, glob expansion with always-include-root, `node_modules` exclusion, dedup across overlapping patterns, default `['.', '**']` patterns. - `pacquet-lockfile`: workspace lockfile round-trip (two importers, one `workspace:*` → `link:../shared`). - `pacquet-package-manager`: per-importer `pnpm:root` prefix, empty-importers no-op, cross-importer `link:` symlinks to sibling `rootDir`. Out of scope (Stage 2 of #299): `--filter`, projects-graph, `engines`/`os`/`cpu` installability filter at project enumeration, the `resolutions` non-root warning, real-path resolution for case-insensitive filesystems. --- Written by an agent (Claude Code, claude-opus-4-7).
Doc job (blocking):
- Replace private intra-doc links to module names in
`crates/workspace/src/lib.rs` with text + linked public items.
- Drop two `[`pacquet_config::WorkspaceSettings`]` rustdoc links in
`crates/workspace/src/manifest.rs` — `pacquet-config` is not a
dependency of `pacquet-workspace`, so the links were broken.
Review feedback:
- `install.rs:111` — replace `to_str().expect(...)` with
`to_string_lossy().into_owned()` so non-UTF-8 workspace paths do
not panic the installer.
- `symlink_direct_dependencies.rs` —
- Validate every importer key (`validate_importer_id`) and reject
absolute paths, Windows drive prefixes, backslash separators, and
`..` segments with `SymlinkDirectDependenciesError::UnsafeImporterPath`.
A malformed lockfile would otherwise let `Path::join` create
`node_modules` outside the workspace root.
- Replace `symlink_package(...).expect("symlink pkg")` with a
fallible `try_for_each` path and a new
`SymlinkDirectDependenciesError::SymlinkPackage` variant so a per-
package symlink failure surfaces as a structured error instead of
panicking the rayon worker.
- Fix the struct-level doc comment to refer to
`<workspace_root>/node_modules/.pacquet` (pacquet's actual virtual
store dir) rather than `.pnpm`.
- `projects.rs:186` — restrict the manifest-read swallow to
`ENOENT`-equivalent errors (`Io(NotFound)` and
`NoImporterManifestFound`); parse / permission / other I/O errors
now propagate via `FindWorkspaceProjectsError::ReadManifest`.
- `root_finder.rs` — treat an empty `NPM_CONFIG_WORKSPACE_DIR` as
unset so the upward walk takes over, matching upstream's truthy
`if (workspaceDir)` check.
- `project_manifest.rs` — drop the misleading mention of
`package.yaml` / `package.json5` from `NoImporterManifestFound`;
pacquet only probes `package.json` today.
- `manifest.rs` — remove the unreachable
`InvalidWorkspaceManifestError::PackagesNotArray` variant; serde
already rejects non-array shapes at parse time.
- `symlink_direct_dependencies/tests.rs` — drop leftover `dbg!`.
New tests:
- `pacquet-package-manager`: `unsafe_importer_keys_error_before_filesystem_writes`
covers six unsafe importer-key shapes.
- `pacquet-workspace`: `empty_env_var_is_treated_as_unset` covers
the `NPM_CONFIG_WORKSPACE_DIR=""` fallback.
Verified locally: `just ready` (649 passed), `taplo format --check`,
`just dylint`, and `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --document-private-items`
all clean.
---
Written by an agent (Claude Code, claude-opus-4-7).
… keys Strengthens `unsafe_importer_keys_error_before_filesystem_writes` to match its name: in addition to the error-type check, it now asserts that no `node_modules` directory is created under `workspace_root` *or* its parent for each rejected importer key. The parent-side check guards against a regression where `Path::join` would let a `..` or absolute key land outside the workspace root. Addresses CodeRabbit feedback on PR #443. --- Written by an agent (Claude Code, claude-opus-4-7).
…ring round-trip `Install::run` converts the resolved workspace root to a lossy-UTF-8 string for reporter envelopes (`prefix`) and passes that to `InstallFrozenLockfile::requester`. The previous code then reconstructed `PathBuf::from(requester)` inside `InstallFrozenLockfile::run` to feed `SymlinkDirectDependencies` and `BuildModules` — meaning real filesystem operations were driven by a path that had already passed through a lossy conversion. On hosts with non-UTF-8 filenames, the on-disk path could silently diverge from the workspace root pacquet meant to install under. Add a `workspace_root: &'a Path` field on `InstallFrozenLockfile` threaded straight from `Install::run` (which still holds the original `PathBuf` returned by `find_workspace_dir`). The lossy `requester` string remains for reporter envelopes only; filesystem-effecting code paths (`SymlinkDirectDependencies::workspace_root`, `BuildModules`'s `lockfile_dir`) now read the real path directly. Caught by Copilot in PR #443 review of crates/package-manager/src/install_frozen_lockfile.rs:158. --- Written by an agent (Claude Code, claude-opus-4-7).
Two items from the 2026-05-13 Copilot review:
1. `validate_importer_id` previously accepted an empty importer key as
equivalent to `"."`. Pnpm only ever writes `.` for the root importer;
an empty-string key is non-standard and could mask a malformed
lockfile. Reject it with `UnsafeImporterPath` instead. The
`importer_root_dir` lookup is correspondingly tightened to the
single canonical root key; the empty-key fallback is unreachable
after validation. New test case (empty string) added to
`unsafe_importer_keys_error_before_filesystem_writes`.
2. `empty_env_var_is_treated_as_unset` was calling `unsafe { env::set_var }`,
which is documented UB when other threads access the process
environment concurrently — and Rust's default test runner is
multi-threaded. Refactor `find_workspace_dir_from_env` to accept an
injected env accessor (`find_workspace_dir_from_env_with`) and have
the production path call it with `std::env::var_os`. Tests now
exercise the fall-through, truthy-value, and lowercase-fallback
branches via pure in-memory closures with no `set_var` calls.
---
Written by an agent (Claude Code, claude-opus-4-7).
Two cleanups while rebasing on top of #442 (partial frozen install): - `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml` write site referenced #431 as future work. With workspace install now landing in this PR, refresh the wording to call out that we *do* ship every importer's section unchanged, and the remaining narrowing (selected importers × engine filter) is gated on `--filter` (Stage 2 of #299). - `root_finder/tests.rs`: rustfmt collapses the if/else closures introduced in a0d4df8 to single-line `if … else …` form. Apply it once here so the diff doesn't keep returning across runs. --- Written by an agent (Claude Code, claude-opus-4-7).
…n review nits Four follow-ups from the post-rebase review: - `pacquet-config` (Copilot, real bug): when `pacquet install` runs from a workspace subdirectory, `Config::current` re-anchored `modules_dir` and `virtual_store_dir` to the CLI `--dir` (the subdir) while [`pacquet_workspace::find_workspace_dir`] still routed the per-importer `SymlinkDirectDependencies` writes under the workspace root. The two layers disagreed about where the install actually lived. Mirror pnpm v11's `pnpmConfig.dir = lockfileDir` rule: when `WorkspaceSettings::find_and_load` finds a `pnpm-workspace.yaml` ancestor, re-anchor `modules_dir` and `virtual_store_dir` to that workspace dir *before* applying settings, so an explicit `modulesDir` / `virtualStoreDir` in the yaml still wins. Two new tests pin the workspace-subdir and single-project cases. - `symlink_direct_dependencies.rs` doc comment (Copilot): hard-coded `.pacquet` and claimed it was fixed by `default_virtual_store_dir`, but pacquet's actual default is `node_modules/.pnpm`. Reword to point readers at `config.virtual_store_dir` (the source of truth) while noting the default. - `unsafe_importer_keys_error_before_filesystem_writes` (CodeRabbit): the assertion that the workspace_root's *parent* has no `node_modules` could spuriously fail when the tempdir's parent is a shared system temp directory populated by an unrelated process. Drop it; keep the workspace_root-side check. - `projects::tests::filters_node_modules` (CodeRabbit): add a diagnostic payload to the bare `assert!` so a failure tells you which projects the enumeration actually surfaced. --- Written by an agent (Claude Code, claude-opus-4-7).
… paths `Install::run` resolves the workspace root through [`pacquet_workspace::find_workspace_dir`], which honors `NPM_CONFIG_WORKSPACE_DIR` (and its lowercase spelling) as an explicit override. Until now, `Config::current` only walked up for `pnpm-workspace.yaml`, so a user setting the env var would get: - per-importer `SymlinkDirectDependencies` writes under the env-var dir, - virtual store under the CLI `--dir`'s `node_modules/.pnpm`, i.e. two `node_modules` layouts for the same install — the same inconsistency the workspace-subdir bug had before commit 5f5803b, just through a different discovery path. Mirror the env-var override in `Config::current`. When the env var is set (non-empty), use it as the workspace dir directly: load `pnpm-workspace.yaml` from there if present (silent on ENOENT, matching upstream's truthy `if (workspaceDir)` check), and re-anchor `modules_dir` / `virtual_store_dir` to that path before applying the yaml settings. The cwd-walk path stays as a fallback when the env var is unset. The env-var lookup is inlined here rather than via `pacquet_workspace::find_workspace_dir_from_env` to avoid adding a cross-crate dependency just for the four-line read — the contract is fixed by pnpm upstream, so the duplication is low-risk. Three new tests: - `npm_config_workspace_dir_re_anchors_modules` covers the override case. - `empty_npm_config_workspace_dir_falls_through` pairs with `pacquet_workspace`'s `empty_env_var_is_treated_as_unset`. - `single_project_anchors_modules_at_cwd` now snapshots the env vars via `EnvGuard` so a concurrent env-var test cannot make it observe the override path. Caught by Copilot in PR #443 review of crates/config/src/lib.rs:618. --- Written by an agent (Claude Code, claude-opus-4-7).
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a `PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to `ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps can be represented. `link:` deps don't live in the virtual store, so they have no snapshot to hoist; mirror the same skip that `pacquet-package-manager`'s `build_sequence::collect_root_dep_paths` already does and `continue` past `ImporterDepVersion::Link` values. The matching `resolved_dep` test helper takes a `&str` literal and is unit-test infrastructure, so a `.into()` from `PkgVerPeer` to `ImporterDepVersion::Regular` is enough — no functional change. --- Written by an agent (Claude Code, claude-opus-4-7).
Three follow-ups from CI and PR review:
- Windows test failure
(`npm_config_workspace_dir_re_anchors_modules`): Windows env vars
are case-insensitive, so
`set_var("NPM_CONFIG_WORKSPACE_DIR", x)` followed by
`remove_var("npm_config_workspace_dir")` was clearing the value
the test had just set. The runner observed "no env override" and
the assertion fired. Fix: in the re-anchor test set only the
uppercase spelling (the production lookup checks it first, so an
externally-set lowercase is unobservable here); in the
empty-falls-through test set both spellings to "" so the truthy
filter rejects them on both platforms.
- Copilot review (symlink_direct_dependencies.rs:133): the
per-importer modules dir was hard-coded to `node_modules`,
ignoring `Config.modules_dir` (and any `modulesDir` override in
`pnpm-workspace.yaml`). Derive the suffix from
`config.modules_dir.file_name()` and apply it under each importer
so all stages (symlink, `.modules.yaml` write, bin link) agree.
New test `custom_modules_dir_propagates_to_each_importer` pins
the contract by configuring `config.modules_dir = .../custom_modules`
and asserting the symlink lands under `<importer>/custom_modules/`,
with no `<importer>/node_modules/` materialised.
- Copilot review (symlink_direct_dependencies/tests.rs:99):
removed two leftover `eprintln!` debug calls and folded the link
paths into the `assert!` messages so failures still carry the
same context without the unconditional stderr noise.
---
Written by an agent (Claude Code, claude-opus-4-7).
…tcher Three follow-ups from Copilot's latest review: - `WorkspaceManifest.packages` was `Vec<String>` with `#[serde(default)]`, collapsing the omitted and explicit-empty cases into the same `Vec::new()`. Upstream's `opts.patterns ?? defaults` only falls back when the key is absent, so an explicit `packages: []` should enumerate only the workspace root, not recurse via the `['.', '**']` defaults. Switch to `Option<Vec<String>>` so `None` (omitted) maps to the defaults and `Some(vec![])` is preserved. - `find_workspace_projects_no_check` consequently changes from `.filter(|p| !p.is_empty()).unwrap_or(&default_patterns)` to a `match` on the `Option`, defaulting only on `None`. Two new tests pin the contract: `empty_packages_array_preserved_as_some_empty` at the manifest layer, and `empty_patterns_array_enumerates_root_only` at the projects-reader layer. - The `wax::any(IGNORE_PATTERNS...)` matcher was being rebuilt for every user-supplied `packages:` pattern. `wax::Glob` and `wax::Any` both derive `Clone`, so hoist the matcher above the loop and `.clone()` it into each `Walk::not` call instead of reparsing the constant ignore list each iteration. --- Written by an agent (Claude Code, claude-opus-4-7).
…l tests After rebasing on top of #439 (`pacquet-package-is-installable` + the `SkippedSnapshots` set), the new test sites this PR added — `per_importer_prefix_in_pnpm_root_events`, `unsafe_importer_keys_error_before_filesystem_writes`, `cross_importer_link_dep_symlinks_to_sibling_rootdir`, and `custom_modules_dir_propagates_to_each_importer` — needed the new `skipped` field on their `SymlinkDirectDependencies` builders. Pass `&SkippedSnapshots::default()` everywhere; the per-importer install behaviour these tests cover is orthogonal to the installability filter, so an empty skipped set keeps the assertion subject the same. --- Written by an agent (Claude Code, claude-opus-4-7).
Origin/main picked up #451 (`feat(git-fetcher): install git-hosted tarballs via preparePackage + packlist`) which adds a required `path: Option<String>` field to `TarballResolution`. The `synthetic_metadata` helper in `installability/tests.rs` builds the struct literally and so needs the new field. Set it to `None` — the installability check doesn't look at `path`, so an unset value preserves the test's existing assertion subject (engines / cpu / os / libc filtering). --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Workspace install (#431) landed in #443. Pacquet now installs every entry in `Lockfile.importers`, not just the root — hoist needs to follow. - `InstallFrozenLockfile::run` now passes the full `importers` map to `build_direct_deps_by_importer` instead of filtering down to the root importer. Transitives unique to a workspace package now reach the shared `<vs>/node_modules` private hoist target, matching upstream's `directDepsByImporterId` shape. - New `workspace_hoist_walks_every_importer` integration test covers the basic multi-importer hoist: root + `packages/foo`, where `foo` is the only importer pulling in `@pnpm.e2e/hello-world-js-bin-parent`. Asserts the transitive `@pnpm.e2e/hello-world-js-bin` lands at `<workspace>/node_modules/.pnpm/node_modules/@pnpm.e2e/hello-world-js-bin` even though the root has no deps. Verified by temporarily reverting the importers swap — the test fails with the expected "transitive of workspace package must be privately hoisted" message. - `known_failures` reasons updated. The original `workspace_install()` blanket reason (which pointed at #431) is replaced by three more-specific reasons that match the actual blockers: - `manifest_mutation_via_pnpm_add()` for tests that call `pnpm add`-equivalent flows mid-test - `workspace_filter_selection()` for tests that use `--filter`-style project selection (separate from #431; not yet implemented) - `hoist_workspace_packages_unsupported()` for the `hoistWorkspacePackages` config which links workspace projects themselves (separate `hoistedWorkspacePackages` shape pacquet doesn't model yet) `workspace_hoist_all_to_virtual_store_node_modules` now points at `partial_install_persists_hoisted_map()` since the basic shape is covered by the new `workspace_hoist_walks_every_importer` test — only the re-install-and-preserve assertion is still gated. - Lockfile types changed: `ResolvedDependencySpec.version` is now `ImporterDepVersion` (`Regular | Link`). `build_direct_deps_by_importer` uses `as_regular()` to skip `link:` workspace siblings — they're not snapshots and aren't hoist candidates (upstream handles them via the separate `hoistedWorkspacePackages` shape). - `SymlinkDirectDependencies` field cleanup from #443: `requester` was removed (now derived per-importer from `rootDir`), `workspace_root` was added. - Updated module docs and `plans/TEST_PORTING.md` to reflect that workspace install (#431) landed; remaining workspace-related hoist tests are gated on more-specific blockers. Tests: `just ready` 882/882 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…ort) #443 (workspace support for `--frozen-lockfile`, the #431 issue pacquet/pacquet#432 explicitly flagged as a follow-up) just landed. With it, the lockfile's `importers` map can carry multiple entries, and the install pipeline materialises a `node_modules/` for each sibling project. The project-registry write needs to follow suit: upstream pnpm registers every importer separately under `<store_dir>/projects/<short-hash>` so the prune sweep can keep every reachable consumer of `<store_dir>/links/...` alive. Changes: - `Install::run` now iterates `lockfile.importers.keys()` and registers each importer's project directory (computed via the newly-exposed `pub(crate) importer_root_dir(...)` from `symlink_direct_dependencies`). Replaces the previous one-shot registration of the workspace root, which would have left sub-importer entries missing from the registry. - The registry write moves to *after* `InstallFrozenLockfile::run` succeeds so the importer keys have already been validated by `SymlinkDirectDependencies::run`'s `validate_importer_id` — no more pre-validation entries for malformed lockfiles. - Gating unchanged: still only fires under `frozen_lockfile && config.enable_global_virtual_store`. - New test `frozen_lockfile_under_gvs_registers_each_workspace_importer` walks the multi-importer case: workspace root + `packages/web` each end up with their own symlink in `<store_dir>/projects/`, resolving back to the right directory. All 829 workspace tests pass; taplo / dylint / cargo doc clean.
4 tasks
11 tasks
zkochan
added a commit
that referenced
this pull request
May 13, 2026
## Summary Replaces the stub `nm_hoist` (landed in #448) with a working hoister and the surrounding guardrails: BFS over the result graph pulling every eligible descendant up to the root, plus upfront refusal of inputs the algorithm doesn't yet model. ### What the algorithm models - **Free hoist.** A transitive dep with no name collision at the root surfaces at the root. - **Identity dedup.** A dep reachable through multiple parents shares one `Rc` thanks to the wrapper's cache; the hoist preserves that identity and strips the duplicate reference at the other parent path. - **Parent-wins on version conflict.** When two distinct deps share an alias but resolve to different snapshot keys, the first BFS visitor takes the root slot and the other stays under its declaring parent. Visit order matches the wrapper's alias-sorted insertion order, so the outcome is deterministic. - **Deep chains flatten in one pass.** `root → a → b → c → d` becomes `root → {a, b, c, d}` — each node, once hoisted, is queued for further descent; its own children evaluate against root's slots rather than against the (now-empty) intermediate parent. - **O(1) root-slot lookup.** A side `HashMap<String, RcByPtr<HoisterResult>>` mirrors root's direct deps, so the per-edge "is this name taken?" check doesn't degrade to O(N²) `IndexSet` scans on flat graphs. ### Fail-fast guards The algorithm doesn't yet model peers, hoisting limits, or multi-importer (workspace) hoist trees. Rather than emit a layout pnpm would reject, the wrapper refuses these inputs upfront with three new `HoistError` variants: - `UnsupportedPeerDependency { ident, peers }` — fires when scanning the constructed `HoisterTree` finds any node with non-empty `peer_names` (either `peerDependencies` from the `packages:` map or `transitive_peer_dependencies` on a `snapshots:` entry). - `UnsupportedHoistingLimits { len }` — non-empty `opts.hoisting_limits`. - `UnsupportedWorkspace { extra_importers }` — any importer beyond the root `.`. Each carries enough context for an operator to identify what triggered it. The `UnsupportedWorkspace` help points at `SymlinkDirectDependencies` — workspaces *do* work in pacquet's wider install path (workspace support landed in #443 for the isolated linker); only the hoister is restricted. ### Rebase pickup `collect_importer_deps` carries the `Link`-variant skip introduced by #443 (workspace support) — `spec.version.as_regular()` extracts the snapshot key for `Regular` deps and `continue`s past `Link` deps, since workspace siblings are direct symlinks materialised by `SymlinkDirectDependencies` and have no snapshot to hoist. ### Performance Nothing in this PR is reachable from `pacquet install` today (`crates/package-manager/src/install*.rs` doesn't import `pacquet-real-hoist` — the crate is dead code from the install pipeline's perspective until the hoisted-linker wiring lands in later slices). The benchmark results bear that out: cold Frozen Lockfile is within CI variance (+3.2% mean, +3.5% median, driven by one outlier), Hot Cache is 6% *faster* (same `stat → lstat` improvement that shipped in slice 1's `import_indexed_dir`), micro is identical. No code path explains a real regression. ## What's deferred - Peer-dependency-aware hoisting (`peer_names` constraints, peer-promise satisfaction). - Multi-round convergence — the BFS handles deep chains in one pass, so the cases requiring true multi-round are limited to peer interactions. - `hoistingLimits` runtime enforcement. - `dependencyKind` distinctions for workspaces and external soft links (today only `ExternalSoftLink` placeholders are added by the wrapper and stripped post-hoist; `Workspace`-kind nodes are blocked by the `UnsupportedWorkspace` guard upfront). ## Upstream reference - [`hoist.ts` algorithm overview](https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L1-L51) — the recipe pacquet's single-pass BFS approximates. - [`hoistTo` main loop](https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L329) — the structural intent the port mirrors for the subset above.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Workspace install (#431) landed in #443. Pacquet now installs every entry in `Lockfile.importers`, not just the root — hoist needs to follow. - `InstallFrozenLockfile::run` now passes the full `importers` map to `build_direct_deps_by_importer` instead of filtering down to the root importer. Transitives unique to a workspace package now reach the shared `<vs>/node_modules` private hoist target, matching upstream's `directDepsByImporterId` shape. - New `workspace_hoist_walks_every_importer` integration test covers the basic multi-importer hoist: root + `packages/foo`, where `foo` is the only importer pulling in `@pnpm.e2e/hello-world-js-bin-parent`. Asserts the transitive `@pnpm.e2e/hello-world-js-bin` lands at `<workspace>/node_modules/.pnpm/node_modules/@pnpm.e2e/hello-world-js-bin` even though the root has no deps. Verified by temporarily reverting the importers swap — the test fails with the expected "transitive of workspace package must be privately hoisted" message. - `known_failures` reasons updated. The original `workspace_install()` blanket reason (which pointed at #431) is replaced by three more-specific reasons that match the actual blockers: - `manifest_mutation_via_pnpm_add()` for tests that call `pnpm add`-equivalent flows mid-test - `workspace_filter_selection()` for tests that use `--filter`-style project selection (separate from #431; not yet implemented) - `hoist_workspace_packages_unsupported()` for the `hoistWorkspacePackages` config which links workspace projects themselves (separate `hoistedWorkspacePackages` shape pacquet doesn't model yet) `workspace_hoist_all_to_virtual_store_node_modules` now points at `partial_install_persists_hoisted_map()` since the basic shape is covered by the new `workspace_hoist_walks_every_importer` test — only the re-install-and-preserve assertion is still gated. - Lockfile types changed: `ResolvedDependencySpec.version` is now `ImporterDepVersion` (`Regular | Link`). `build_direct_deps_by_importer` uses `as_regular()` to skip `link:` workspace siblings — they're not snapshots and aren't hoist candidates (upstream handles them via the separate `hoistedWorkspacePackages` shape). - `SymlinkDirectDependencies` field cleanup from #443: `requester` was removed (now derived per-importer from `rootDir`), `workspace_root` was added. - Updated module docs and `plans/TEST_PORTING.md` to reflect that workspace install (#431) landed; remaining workspace-related hoist tests are gated on more-specific blockers. Tests: `just ready` 882/882 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Workspace install (#431 / #443) and GVS install activation (#432 / #449) both landed since the hoist work in #435 began. The rebase needed two adjustments to keep the symlink target paths correct under GVS: - `symlink_hoisted_dependencies` now takes `&VirtualStoreLayout` instead of `virtual_store_dir: &Path`. The symlink target (the source of each hoist symlink) goes through `layout.slot_dir(key)`, which under GVS resolves to `<store_dir>/links/<scope>/<name>/<version>/<hash>/` and falls back to the legacy `<virtual_store_dir>/<key.virtual_store_name>/` flat name when GVS is off. The hoist code never has to branch on `enable_global_virtual_store` itself. - The private hoist *target dir* (where hoist symlinks live) stays `config.virtual_store_dir.join("node_modules")`. Pacquet keeps `virtual_store_dir` project-local even with GVS enabled — only `global_virtual_store_dir` carries the shared `<store_dir>/links` path (see `Config::apply_global_virtual_store_derivation`). So `<root>/node_modules/.pnpm/node_modules` is still the right placement under both modes; the GVS-rewire concern from the issue description doesn't apply to pacquet's split-field design. Updated the call site comment to record this and dropped the stale "GVS not implemented yet — tracked at #432" note. Tests: `just ready` 894/894 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. The full hoist CLI suite (36/36) and unit suite (12/12) still pass with the GVS layout pulled into the symlink path.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Workspace install (#431) landed in #443. Pacquet now installs every entry in `Lockfile.importers`, not just the root — hoist needs to follow. - `InstallFrozenLockfile::run` now passes the full `importers` map to `build_direct_deps_by_importer` instead of filtering down to the root importer. Transitives unique to a workspace package now reach the shared `<vs>/node_modules` private hoist target, matching upstream's `directDepsByImporterId` shape. - New `workspace_hoist_walks_every_importer` integration test covers the basic multi-importer hoist: root + `packages/foo`, where `foo` is the only importer pulling in `@pnpm.e2e/hello-world-js-bin-parent`. Asserts the transitive `@pnpm.e2e/hello-world-js-bin` lands at `<workspace>/node_modules/.pnpm/node_modules/@pnpm.e2e/hello-world-js-bin` even though the root has no deps. Verified by temporarily reverting the importers swap — the test fails with the expected "transitive of workspace package must be privately hoisted" message. - `known_failures` reasons updated. The original `workspace_install()` blanket reason (which pointed at #431) is replaced by three more-specific reasons that match the actual blockers: - `manifest_mutation_via_pnpm_add()` for tests that call `pnpm add`-equivalent flows mid-test - `workspace_filter_selection()` for tests that use `--filter`-style project selection (separate from #431; not yet implemented) - `hoist_workspace_packages_unsupported()` for the `hoistWorkspacePackages` config which links workspace projects themselves (separate `hoistedWorkspacePackages` shape pacquet doesn't model yet) `workspace_hoist_all_to_virtual_store_node_modules` now points at `partial_install_persists_hoisted_map()` since the basic shape is covered by the new `workspace_hoist_walks_every_importer` test — only the re-install-and-preserve assertion is still gated. - Lockfile types changed: `ResolvedDependencySpec.version` is now `ImporterDepVersion` (`Regular | Link`). `build_direct_deps_by_importer` uses `as_regular()` to skip `link:` workspace siblings — they're not snapshots and aren't hoist candidates (upstream handles them via the separate `hoistedWorkspacePackages` shape). - `SymlinkDirectDependencies` field cleanup from #443: `requester` was removed (now derived per-importer from `rootDir`), `workspace_root` was added. - Updated module docs and `plans/TEST_PORTING.md` to reflect that workspace install (#431) landed; remaining workspace-related hoist tests are gated on more-specific blockers. Tests: `just ready` 882/882 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Workspace install (#431 / #443) and GVS install activation (#432 / #449) both landed since the hoist work in #435 began. The rebase needed two adjustments to keep the symlink target paths correct under GVS: - `symlink_hoisted_dependencies` now takes `&VirtualStoreLayout` instead of `virtual_store_dir: &Path`. The symlink target (the source of each hoist symlink) goes through `layout.slot_dir(key)`, which under GVS resolves to `<store_dir>/links/<scope>/<name>/<version>/<hash>/` and falls back to the legacy `<virtual_store_dir>/<key.virtual_store_name>/` flat name when GVS is off. The hoist code never has to branch on `enable_global_virtual_store` itself. - The private hoist *target dir* (where hoist symlinks live) stays `config.virtual_store_dir.join("node_modules")`. Pacquet keeps `virtual_store_dir` project-local even with GVS enabled — only `global_virtual_store_dir` carries the shared `<store_dir>/links` path (see `Config::apply_global_virtual_store_derivation`). So `<root>/node_modules/.pnpm/node_modules` is still the right placement under both modes; the GVS-rewire concern from the issue description doesn't apply to pacquet's split-field design. Updated the call site comment to record this and dropped the stale "GVS not implemented yet — tracked at #432" note. Tests: `just ready` 894/894 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. The full hoist CLI suite (36/36) and unit suite (12/12) still pass with the GVS layout pulled into the symlink path.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Address PR #445 review feedback after the rebase pulled in #439 (installability check / `SkippedSnapshots`) + #443 (workspace install). - **Honor `SkippedSnapshots` in the hoist pass** (Copilot, real bug). `install_frozen_lockfile.rs` was passing `HashSet::new()` to `HoistInputs.skipped` even though `compute_skipped_snapshots` already produces the optional+platform-incompatible skip set earlier in the same function. Effect of the fix: hoist no longer creates symlinks to slots that were never extracted, and an alias that would have been claimed by a skipped snapshot can now be claimed by a non-skipped sibling at a deeper level. The `HoistInputs` shape stays `&HashSet<PackageKey>` so `hoist.rs` doesn't depend on `installability`; the cheap conversion happens at the call site. - **Refresh `build_direct_deps_by_importer` doc comment** (Copilot). Doc said "currently only installs the root importer" — wrong since #443 (workspace install) and the follow-up that switched the call site to pass the full `importers` map. Updated to describe the current per-importer walk and the `link:` filter inside the loop. - **Refresh `skipped_optional_deps` known-failure reason** (Copilot). The reason claimed pacquet doesn't compute skip sets — wrong since #439. The actual gap is the test fixture: upstream's `@pnpm.e2e/not-compatible-with-any-os` isn't in pacquet's mocked registry, so the end-to-end skip-then-hoist path can't be exercised yet. Reason updated to point at the fixture gap, not the (already-implemented) skip computation. Tests: `just ready` 902/902 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. The 36 CLI hoist tests + 12 unit tests still pass with the real skip set threaded through. Out of scope: Copilot's comment about `satisfies_package_manifest` only validating the root importer is about main's #447 freshness check (not this PR's hoist work). Worth a follow-up issue but not addressed here.
13 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Stage 1 of pnpm/pnpm#11633 / closes #431 (Stage 1 scope) / partially closes #357.
New crate
pacquet-workspace— ports pnpm'sworkspace/root-finder,workspace/workspace-manifest-reader,workspace/projects-reader, andworkspace/project-manifest-reader(pinned to94240bc046).find_workspace_dirhonoursNPM_CONFIG_WORKSPACE_DIR(and its lowercase spelling, with empty-value fall-through), walks up topnpm-workspace.yaml, and rejects misnamed variants (pnpm-workspace.yml,.pnpm-workspace.yaml, ...) withBAD_WORKSPACE_MANIFEST_NAME. Project enumeration glob-expandspackages:viawax, always includes the workspace root (Always include the rootpackage.jsonin a workspace pnpm#1986), filters**/node_modules/**+**/bower_components/**, sorts lex byrootDir, and preserves the omitted-vs-explicit-empty distinction sopackages: []enumerates only the root rather than falling back to the recursive['.', '**']default.Per-importer install path —
SymlinkDirectDependenciesiterates every entry inLockfile.importers, computes per-projectrootDirandnode_modules/, and usesrootDiras theprefixforpnpm:root addedevents (matching upstream'slinkDirectDeps). Install-wide events (pnpm:stage,pnpm:summary) still uselockfileDir, now resolved throughfind_workspace_dir.MissingRootImporteris gone — an empty importers map is a silent no-op. A custommodulesDirfrompnpm-workspace.yamlpropagates to every importer (the symlink stage readsconfig.modules_dir.file_name()rather than hard-codingnode_modules).Path-anchoring consistency —
Config::currentre-anchorsmodules_dir/virtual_store_dirto the workspace root whenpnpm-workspace.yamlis found in an ancestor, mirroring pnpm v11'spnpmConfig.dir = lockfileDir. Runningpacquet installfrom a workspace subdirectory no longer produces two inconsistentnode_moduleslayouts (subdir virtual store vs workspace-root per-importer symlinks).Config::currentalso honoursNPM_CONFIG_WORKSPACE_DIRso the env-var override drives bothfind_workspace_dirand the path anchors.workspace_rootis threaded throughInstallFrozenLockfileas a real&Path(no lossy-UTF-8 round-trip), so non-UTF-8 filenames survive the install layer intact.Lockfile
link:support —ResolvedDependencySpec.versionis now anImporterDepVersionenum (Regular(PkgVerPeer) | Link(String)), so a v9 workspace lockfile withversion: link:../sharedparses. The symlink stage resolves the link target against the importer'srootDirand pointsnode_modules/<name>at the sibling project rather than the virtual store;BuildModules,pacquet-real-hoist, and the side-effects-cache write path all skipLinkvariants when collecting snapshot-key roots.Importer-key validation — rejects absolute POSIX paths, Windows drive prefixes, backslash separators,
..segments, and the empty string with a typedUnsafeImporterPatherror, so a malformed (or hostile) lockfile cannot makePath::joincreatenode_modulesoutside the workspace root.Symlink error propagation — the prior
expect("symlink pkg")is replaced withtry_for_each+ aSymlinkDirectDependenciesError::SymlinkPackagevariant carrying theimporter_id,name, and underlyingSymlinkPackageError. A filesystem failure no longer crashes the rayon pool.Adds
wax = 0.7.0to[workspace.dependencies]for project globbing.Test plan
just ready— 825 tests pass (2 skipped)taplo format --check— cleanjust dylint(perfectionist) — cleanpacquet-workspacecovering discovery, env-var override (incl. empty-fall-through, lowercase spelling, in-memory injection so noset_varUB), manifest parsing (omitted vs explicit-emptypackages:), and project enumeration (overlap dedup, root-always-included, node_modules ignore, custom defaults)pacquet-configtests pin path anchoring: workspace-subdir re-anchors at workspace root, single-project keeps the cwd,NPM_CONFIG_WORKSPACE_DIRre-anchors, empty env-var falls through (EnvGuardso concurrent tests don't race)pacquet-lockfileround-trips a workspace lockfile with aworkspace:*→link:../shareddep, and pinsImporterDepVersion::{Regular, Link}parsingpacquet-package-managercovers per-importerpnpm:rootprefix, cross-importerlink:symlink targets, custommodulesDirpropagation, unsafe-importer-key rejection (7 shapes, no filesystem writes), and empty-importers no-opOut of scope (Stage 2 of pnpm/pnpm#11633)
--filter/--filter-prod/ change-based filteringprojects-graph+ topological sort across local packagesengines/os/cpuinstallability filtering at project enumeration (separate from the per-snapshot filter that already landed in feat(package-is-installable): platform + engine check, skip optional incompatibles (#434 slice 1) #439)resolutions-on-non-root warningrootDirfor case-insensitive filesystemsWritten by an agent (Claude Code, claude-opus-4-7).