Conversation
Ports upstream's [`pruneGlobalVirtualStore`](https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/pruneGlobalVirtualStore.ts) and [`getRegisteredProjects`](https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/projectRegistry.ts#L37-L100) (the read half of the registry pacquet only shipped the write half of in #432 / #444). Wiring in `StoreCommand::Prune` already routes through `StoreDir::prune()`, which was a `todo!()` stub until now. With this commit, `pacquet store prune` runs the mark-and-sweep upstream ships: 1. **Mark** — walk every registered project, find every `node_modules/`, follow symlinks that land under `<store>/links/...`, record reachable slot paths (`<scope>/<name>/<version>/<hash>`), and recurse into the slot's own `node_modules/` for transitive deps. 2. **Sweep** — walk the four-level slot tree and remove every `<hash>` not in the reachable set. Empty `<version>/` and `<name>/` parents get cleaned up. `get_registered_projects` self-heals the registry on every call by unlinking entries whose project directory has been deleted. Permission / unrelated errors surface as `PROJECT_INACCESSIBLE` / `PROJECT_REGISTRY_ENTRY_INACCESSIBLE` instead of being silently dropped, mirroring upstream's strict stance. Pacquet doesn't yet have rayon plumbing in store-dir, so the port walks sequentially. Prune is one-shot (CLI), not on the hot install path; parallelism can be added later if profiling shows it matters. The function shape mirrors upstream's `Promise.all`-driven layout so a future graft is mechanical. Tests: - 5 `get_registered_projects` tests (missing dir, live, stale self-heal, mixed, dotfile skip) - 6 `prune` tests (no-links no-op, no-projects no-op, dead-project slot removal + name-dir cleanup, shared-slot survives partial unregister, orphan slot removal, transitive reachability)
|
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:
📝 WalkthroughWalkthroughPorts cross-platform directory-symlink helpers, adds a read-side project registry with stale-entry self-healing, and implements a mark-and-sweep StoreDir::prune that computes reachable store slots from registered projects and removes unreachable slots and empty parents. ChangesProject Registry and Store Pruning
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Registry
participant ProjectsDir
participant SymlinkReader
participant Filesystem
Caller->>Registry: call get_registered_projects()
Registry->>ProjectsDir: read_dir(projects/)
loop per non-dotfile entry
Registry->>SymlinkReader: read_symlink_dir(entry)
alt resolved target
SymlinkReader-->>Registry: target path
Registry->>Filesystem: metadata(target)
Filesystem-->>Registry: exists -> add to results
else ENOENT/EINVAL-like
Registry-->>Registry: silently skip entry
else target NotFound
Registry->>SymlinkReader: remove_symlink_dir(entry)
else other error
Registry-->>Caller: return error
end
end
Registry-->>Caller: Vec<PathBuf>
sequenceDiagram
participant Caller
participant Prune
participant LinksDir
participant Registry
participant NodeFinder
participant SymlinkWalker
Caller->>Prune: call prune()
Prune->>LinksDir: check links/ exists
alt links missing
Prune-->>Caller: Ok (no-op)
else links present
Prune->>Registry: get_registered_projects()
Registry-->>Prune: project paths
Prune->>NodeFinder: find_all_node_modules_dirs(projects)
NodeFinder-->>Prune: node_modules dirs
Prune->>SymlinkWalker: walk_symlinks_to_store(dirs)
SymlinkWalker->>LinksDir: follow symlink -> slot
SymlinkWalker-->>Prune: reachable slot set
Prune->>LinksDir: iterate scope/name/version/hash
Prune->>LinksDir: remove unreachable slots and cleanup parents
Prune-->>Caller: return Result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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 unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/store-dir/src/prune.rs (1)
213-218: 💤 Low valueAvoid
slot.clone()by reordering operations.
slotis inserted intoreachableand then used for the join. Performing the join first removes the need for clone.♻️ Suggested refactor
if let Some(idx) = nm_idx { let slot: PathBuf = parts[..idx].iter().collect(); - reachable.insert(slot.clone()); // Recurse into the slot's own node_modules for // transitive deps. let inner_modules = canonical_links.join(&slot).join("node_modules"); + reachable.insert(slot); walk_symlinks_to_store(&inner_modules, links_dir, reachable, visited); }🤖 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/store-dir/src/prune.rs` around lines 213 - 218, The code clones `slot` unnecessarily when inserting into `reachable` before using it to compute `inner_modules`; fix by computing `inner_modules` first (use canonical_links.join(&slot).join("node_modules")), then insert `slot` into `reachable` by moving it (reachable.insert(slot)), and finally call walk_symlinks_to_store(&inner_modules, links_dir, reachable, visited); update the block around the `slot`, `inner_modules`, and `reachable` logic in the same function so no `.clone()` is required.crates/store-dir/src/project_registry.rs (1)
294-298: 💤 Low valueUnnecessary clone when
targetis already owned.
targetis owned here. Theifbranch can move it directly; only theelsebranch needs the value.♻️ Suggested refactor to avoid clone
- let absolute_target = if target.is_absolute() { - target.clone() - } else { - link_path.parent().map(|p| p.join(&target)).unwrap_or_else(|| target.clone()) - }; + let absolute_target = if target.is_absolute() { + target + } else { + link_path.parent().map(|p| p.join(&target)).unwrap_or(target) + };🤖 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/store-dir/src/project_registry.rs` around lines 294 - 298, The code clones `target` unnecessarily; since `target` is owned you can move it in the `if` branch and only use it by reference in the `else` chain. Replace the current expression for `absolute_target` so the `if target.is_absolute()` arm returns `target` (moved) and the `else` arm uses `link_path.parent().map(|p| p.join(&target)).unwrap_or_else(|| target)` to avoid the extra clone while still moving `target` only when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/store-dir/src/project_registry.rs`:
- Around line 294-298: The code clones `target` unnecessarily; since `target` is
owned you can move it in the `if` branch and only use it by reference in the
`else` chain. Replace the current expression for `absolute_target` so the `if
target.is_absolute()` arm returns `target` (moved) and the `else` arm uses
`link_path.parent().map(|p| p.join(&target)).unwrap_or_else(|| target)` to avoid
the extra clone while still moving `target` only when needed.
In `@crates/store-dir/src/prune.rs`:
- Around line 213-218: The code clones `slot` unnecessarily when inserting into
`reachable` before using it to compute `inner_modules`; fix by computing
`inner_modules` first (use canonical_links.join(&slot).join("node_modules")),
then insert `slot` into `reachable` by moving it (reachable.insert(slot)), and
finally call walk_symlinks_to_store(&inner_modules, links_dir, reachable,
visited); update the block around the `slot`, `inner_modules`, and `reachable`
logic in the same function so no `.clone()` is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c384083e-c293-4ed9-b7c1-bcc2d633e915
📒 Files selected for processing (2)
crates/store-dir/src/project_registry.rscrates/store-dir/src/prune.rs
📜 Review details
🧰 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/store-dir/src/project_registry.rscrates/store-dir/src/prune.rs
🧠 Learnings (1)
📚 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/store-dir/src/project_registry.rscrates/store-dir/src/prune.rs
🔇 Additional comments (10)
crates/store-dir/src/project_registry.rs (4)
11-13: LGTM!
163-214: LGTM!
320-341: LGTM!
456-551: LGTM!crates/store-dir/src/prune.rs (6)
1-33: LGTM!
35-51: LGTM!
53-110: LGTM!
119-150: LGTM!
235-324: LGTM!
326-503: LGTM!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #475 +/- ##
==========================================
+ Coverage 89.18% 89.99% +0.81%
==========================================
Files 116 119 +3
Lines 10472 11529 +1057
==========================================
+ Hits 9339 10376 +1037
- Misses 1133 1153 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Pull request overview
Implements pacquet store prune for the global virtual store by porting pnpm’s mark-and-sweep (pruneGlobalVirtualStore) and adding the missing read/cleanup half of the project registry (getRegisteredProjects) so the prune command can discover live projects and remove unreachable <store>/links/... slots.
Changes:
- Add
StoreDir::prune()with mark-and-sweep traversal of registered projects’node_modulessymlinks into<store>/links. - Add
get_registered_projects()with stale-registry-entry self-healing and structured error reporting. - Add unit tests covering registry read/cleanup and prune behaviors (no-op cases, orphan removal, shared-slot retention, transitive reachability).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/store-dir/src/prune.rs | Adds the prune implementation + traversal/sweep helpers and tests. |
| crates/store-dir/src/project_registry.rs | Adds read-side project registry listing with stale-entry cleanup + tests. |
Comments suppressed due to low confidence (3)
crates/store-dir/src/prune.rs:187
walk_symlinks_to_storedrops errors fromfs::read_dirandfs::read_link(and fromentry.file_type()), which can lead to an incomplete reachable set and unsafe deletion during sweep. Sinceprune’s docs state that mark/sweep I/O errors are surfaced, consider returningResultfrom this walker and propagating errors (at least for permission/unexpected errors;NotFoundraces can be treated as benign).
let Ok(entries) = fs::read_dir(dir) else {
return;
};
for entry in entries.flatten() {
let entry_path = entry.path();
let Ok(file_type) = entry.file_type() else {
continue;
};
if file_type.is_symlink() {
let Ok(target) = fs::read_link(&entry_path) else {
continue;
};
crates/store-dir/src/prune.rs:304
list_subdirstreats anyfs::read_direrror as “no subdirs” and also drops per-entry errors viaflatten(). In the sweep phase this can cause prune to silently do nothing (or partially prune) on permission/corruption errors. It would be safer to return aResultand propagate unexpected I/O errors so failures are visible and prune doesn’t make decisions from incomplete data.
fn list_subdirs(dir: &Path) -> Vec<std::ffi::OsString> {
let Ok(entries) = fs::read_dir(dir) else {
return Vec::new();
};
crates/store-dir/src/prune.rs:324
path_existsusesfs::metadata(path).is_ok(), which converts permission and other I/O errors into “does not exist” and makes prune silently no-op. Consider distinguishingNotFound(no-op) from other errors (surface asPruneError) so operators can see misconfigured permissions rather than getting a misleading “no links/ dir” behavior.
fn path_exists(path: &Path) -> bool {
fs::metadata(path).is_ok()
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5858531640800004,
"stddev": 0.07117635972514501,
"median": 2.55533327378,
"user": 2.64520938,
"system": 3.5754606000000004,
"min": 2.51054950278,
"max": 2.75804172178,
"times": [
2.75804172178,
2.55097609278,
2.55164095678,
2.5385974727800003,
2.62139781578,
2.5958120247800003,
2.62850936478,
2.54398109778,
2.55902559078,
2.51054950278
]
},
{
"command": "pacquet@main",
"mean": 2.4896222534800003,
"stddev": 0.06777838187087971,
"median": 2.47175921878,
"user": 2.62449608,
"system": 3.5309500999999996,
"min": 2.41310300478,
"max": 2.66257779878,
"times": [
2.44745275078,
2.47228112478,
2.66257779878,
2.46466884278,
2.49204994278,
2.47123731278,
2.45157310678,
2.41310300478,
2.52063029778,
2.5006483527800003
]
},
{
"command": "pnpm",
"mean": 5.84671035658,
"stddev": 0.0644292630280967,
"median": 5.83124623678,
"user": 8.560440879999998,
"system": 4.3342277,
"min": 5.78161939378,
"max": 5.97825353078,
"times": [
5.80524717378,
5.848413089779999,
5.94023646278,
5.8317537777799995,
5.791453543779999,
5.97825353078,
5.8048014087799995,
5.85458648878,
5.78161939378,
5.83073869578
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7431751170800001,
"stddev": 0.042196015294705944,
"median": 0.73134552208,
"user": 0.37852422,
"system": 1.61862624,
"min": 0.71180385758,
"max": 0.85783687858,
"times": [
0.85783687858,
0.75158355958,
0.72744577158,
0.71243999558,
0.74378990058,
0.73000391558,
0.72500053558,
0.71180385758,
0.7326871285800001,
0.73915962758
]
},
{
"command": "pacquet@main",
"mean": 0.7563599382799999,
"stddev": 0.032147284333075093,
"median": 0.74687064558,
"user": 0.36698622,
"system": 1.64003244,
"min": 0.71569607358,
"max": 0.80608825258,
"times": [
0.80608825258,
0.78110046858,
0.73919718658,
0.73653885358,
0.77978420058,
0.71569607358,
0.7260444915800001,
0.79645979258,
0.72814595858,
0.75454410458
]
},
{
"command": "pnpm",
"mean": 2.4499240984800004,
"stddev": 0.04728306139999927,
"median": 2.4494975760799997,
"user": 2.8602033199999997,
"system": 2.20815254,
"min": 2.3585625015800002,
"max": 2.5250381235800003,
"times": [
2.49015525658,
2.47164822958,
2.42823506058,
2.4841242715800003,
2.43363631158,
2.46535884058,
2.3585625015800002,
2.43248044558,
2.41000194358,
2.5250381235800003
]
}
]
} |
Four issues raised:
1. `StoreDir::prune` doc said "Returns the count of removed slot
directories" but the function returns `Result<(), PruneError>`.
The count is intentionally reported to stderr (matching upstream's
`globalInfo("Removed N package(s) ...")` shape), not via the
return type. Doc updated to match.
2. `get_registered_projects` was silently dropping registry entries
when `file_type()` failed. A permission error there would let a
downstream prune remove slots a live project still references.
Now surfaces as `EntryInaccessible` — there's no upstream
analogue (Node's `entry.isSymbolicLink()` can't fail), so we err
on the side of strictness.
3. The sweep-phase `list_subdirs` was swallowing every `read_dir`
error and returning an empty `Vec`. That doesn't match upstream's
[`getSubdirsSafely`](https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/pruneGlobalVirtualStore.ts#L282-L299),
which only swallows `ENOENT` and propagates everything else. New
`PruneError::ReadSweepDir` variant carries the path + io::Error.
The mark-phase helpers (`find_all_node_modules_dirs`,
`walk_symlinks_to_store`) keep their bare `catch { return }` shape
— that *does* match upstream and tightening it unilaterally would
diverge from pnpm's prune behaviour on shared store directories.
Comments now flag the deliberate match.
4. Stale-entry unlink in `get_registered_projects` and the heal
branch in `register_project` both used `fs::remove_file`, which
fails on Windows junctions (created via `pacquet_fs::symlink_dir`).
Added `pacquet_fs::remove_symlink_dir` (uses `remove_dir` on
Windows, `remove_file` on Unix) and routed both call sites
through it. The `register_project` fix is incidental — same
logical operation, same pre-existing cross-platform bug.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e46de3c6-3e55-40aa-abe4-6e3d037f1ec2
📒 Files selected for processing (3)
crates/fs/src/symlink_dir.rscrates/store-dir/src/project_registry.rscrates/store-dir/src/prune.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/store-dir/src/prune.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). (6)
- GitHub Check: Lint and Test (macos-latest)
- 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/fs/src/symlink_dir.rscrates/store-dir/src/project_registry.rs
🧠 Learnings (1)
📚 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/fs/src/symlink_dir.rscrates/store-dir/src/project_registry.rs
🔇 Additional comments (1)
crates/fs/src/symlink_dir.rs (1)
13-27: LGTM!
CodeRabbit caught a critical Windows bug: pacquet's registry entries
are NTFS junctions (created via `junction::create` in
`pacquet_fs::symlink_dir`), and `std::fs::read_link` only handles
`IO_REPARSE_TAG_SYMLINK` reparse points — it returns `EINVAL`
("not a symlink") for `IO_REPARSE_TAG_MOUNT_POINT` junctions. The
EINVAL silent-skip would then drop every live registry entry on
Windows, breaking `pacquet store prune` entirely (see
[rust-lang/rust#28528](rust-lang/rust#28528),
open since 2015).
Added `pacquet_fs::read_symlink_dir` — falls back to
`junction::get_target` on Windows EINVAL. Routed two `fs::read_link`
sites through it:
- `get_registered_projects` (the new code from #475)
- `register_project::InspectExisting` (pre-existing bug — heal
branch was also broken on Windows since the original #432 / #444
landed)
Same one-helper-for-both pattern as `remove_symlink_dir` in the
previous review fix. Tests still pass; the Windows behaviour
isn't reachable from macOS / Linux CI but the platform split is
contained in one well-commented helper.
Also: `cargo fmt --all` reflow of one `EntryInaccessible` struct
literal that `just ready`'s `cargo fmt` step ran *before* the
Copilot-fix edits — CI's `cargo fmt --all -- --check` caught it.
`junction` is only declared on Windows targets, so the intra-doc link `[\`junction::get_target\`]` resolves on Windows but fails the Linux doc build (`-D rustdoc::broken-intra-doc-links` is enabled in CI via `RUSTDOCFLAGS="-D warnings"`). Switched to plain backticks with an inline note about why the link can't be there. Caught by CI Doc job after 8c7f43c — should have run `cargo doc` locally first.
…elp wording Three Copilot review comments addressed: 1. **Critical Windows bug in mark phase** — `walk_symlinks_to_store` was still calling `std::fs::read_link` directly. On Windows pacquet creates junctions for every `node_modules/<pkg>` entry, and `fs::read_link` returns `EINVAL` for junctions ([rust-lang/rust#28528](rust-lang/rust#28528)). Result: the mark walk would skip every direct dep link on Windows, leaving `reachable` incomplete, and the sweep phase would delete slots that are still in use — same shape as the registry-side bug already fixed in 8c7f43c but on the mark path. Routed through `pacquet_fs::read_symlink_dir` so both call sites share the junction fallback. 2. **Redundant canonicalize in the hot loop** — `walk_symlinks_to_store` was canonicalising `links_dir` inside the per-entry loop, which burns one `realpath` syscall per visited symlink even though the answer is invariant for the whole traversal. Moved the canonicalize to `StoreDir::prune` (called once) and renamed the parameter to `canonical_links` so the contract is visible at the signature. 3. **Misleading help text on Windows** — `EntryInaccessible` and `ProjectInaccessible` diagnostics suggested "delete the file at" / "delete the symlink at", but on Windows the registry entry is a directory junction (not a file, not a symlink in the Unix sense). Reworded both to "delete the entry at" so the guidance is platform-neutral.
Copilot's last comment: `remove_dir_if_present` was using `fs::remove_dir_all` for both slot directories AND for the empty `<version>/`, `<name>/`, `<scope>/` parents. The slot case is fine (the subtree is known unreachable), but on the parent path `remove_dir_all` would also wipe any concurrent install that materialised a fresh slot between `list_subdirs` and the parent cleanup. Split into two helpers: - `remove_slot_dir`: `fs::remove_dir_all` for the actual sweep target — unchanged behaviour, subtree is known unreferenced. - `remove_empty_dir`: `fs::remove_dir` that returns `Ok(true)` on success, `Ok(false)` for `DirectoryNotEmpty` / `NotFound` (race with a concurrent install or already gone), propagates other errors. Used for the `<version>/`, `<name>/`, `<scope>/` parents. Counts (`emptied_versions`, `emptied_pkgs`) now track *actual* removal rather than "we tried to remove" — propagating the `Ok(false)` "didn't remove" signal up the call chain so we don't attempt to remove a parent's parent based on stale counters when the race hits. This is a deliberate divergence from upstream's [`rimraf`-everywhere shape](https://github.com/pnpm/pnpm/blob/94240bc046/store/controller/src/storeController/pruneGlobalVirtualStore.ts#L210-L223). Same on-disk result in the non-race case; pacquet is strictly safer in the race case where upstream would clobber the new slot. Doc comment on `remove_empty_dir` flags the divergence so reviewers know it's intentional.
Summary
Closes #458. Ports upstream's
pruneGlobalVirtualStoreand the read half ofprojectRegistry(pacquet shipped only the write half in #432 / #444).The CLI wiring already existed —
StoreCommand::Prune→StoreDir::prune()— butStoreDir::prunewas atodo!(). With this PRpacquet store pruneactually runs.What it does
Mark phase: walk every registered project (
<store>/projects/<short-hash>→ project root). For each project, find everynode_modules/, follow symlinks that land under<store>/links/..., record reachable<scope>/<name>/<version>/<hash>slots, and recurse into the slot's ownnode_modules/for transitive deps.Sweep phase: walk
<store>/links/<scope>/<name>/<version>/<hash>and remove every<hash>not in the reachable set. Empty<version>/,<name>/, and<scope>/parents get cleaned up.Self-healing registry:
get_registered_projectsunlinks entries whose project directory has been deleted (ENOENTon stat). Permission / unrelated errors surface asPROJECT_INACCESSIBLE/PROJECT_REGISTRY_ENTRY_INACCESSIBLE— matches upstream's strict stance, since silently dropping an inaccessible registry entry could remove slots the project still references.Notes
Promise.all-driven layout so a parallelism graft is mechanical when it lands.eprintln!. Pacquet doesn't yet thread the install-time reporter through store-dir; the proper wiring is tracked under #344.Test plan
get_registered_projectstests: missing-registry-dir, live project, stale self-heal, mixed live/stale, dotfile-entry skipprunetests: no-links no-op, no-projects no-op, dead-project slot removal + empty-name-dir cleanup, shared slot survives partial unregister, orphan slot removal, transitive reachability via slot-internalnode_modulesjust ready(955 tests pass, 2 skipped)taplo format --checkjust dylintWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes
Tests