fix(pacquet/fs): fall back to absolute target across Windows drives#11721
Conversation
`pathdiff::diff_paths` does not return `None` when the two paths sit on different Windows drive roots; it produces a path that climbs out with `..` and then re-anchors with the target's drive letter (for example, `..\..\C:\Users\...`). Windows rejects such a symlink target with `ERROR_INVALID_PARAMETER` (os error 87), which is the failure mode CI on Windows hits when the workspace lives on `D:` and the global store installed by `setup-pnpm` lives on `C:`. Node.js's `path.win32.relative` returns the absolute `to` argument when the two paths share no common root, which is the behavior upstream `symlink-dir` relies on. Mirror that by detecting unequal `Component::Prefix` values up front and returning the absolute target unchanged. Drive-letter comparison is case-insensitive to match NTFS and Node.js semantics.
|
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:
📝 WalkthroughWalkthroughrelative_target_for now simplifies paths and, on Windows, checks whether the link parent and target share the same Windows root; if not, it returns the absolute original. A new same_path_root helper and Windows-only tests were added; the dunce crate is added under Windows dependencies. ChangesWindows Symlink Cross-Root Safeguard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/fs/src/symlink_dir.rs (1)
56-90: 💤 Low valueWindows cross-drive guard implementation is correct.
The guard correctly detects different path roots and falls back to absolute paths, preventing invalid
..-prefixed targets. The case-insensitive drive-letter comparison matches NTFS and Node.js semantics.Optional future enhancement: UNC path case normalization
The current
case_normalizeimplementation handlesDiskandVerbatimDiskprefixes but leaves UNC paths (server/share names) unchanged. Windows UNC server and share names are also case-insensitive, so\\SERVER\shareand\\server\sharerefer to the same root.This is low-priority because:
- The PR specifically targets drive-letter scenarios (the documented CI failure mode)
- Same UNC root with different casing is rare
- The current behavior is safe (would use absolute path instead of relative, still correct)
If you want full Node.js parity for UNC paths in the future, you could normalize
UNCandVerbatimUNCserver/share components case-insensitively.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/fs/src/symlink_dir.rs` around lines 56 - 90, The Windows UNC server/share names are not case-normalized, so to fully match Node.js semantics update the case_normalize helper inside same_path_root to also normalize Prefix::UNC and Prefix::VerbatimUNC by lowercasing their server and share components (treat both UNC and VerbatimUNC analogous to Disk/VerbatimDisk), ensuring comparisons of UNC roots become case-insensitive while keeping existing Disk/VerbatimDisk handling and behavior of relative_target_for unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pacquet/crates/fs/src/symlink_dir.rs`:
- Around line 56-90: The Windows UNC server/share names are not case-normalized,
so to fully match Node.js semantics update the case_normalize helper inside
same_path_root to also normalize Prefix::UNC and Prefix::VerbatimUNC by
lowercasing their server and share components (treat both UNC and VerbatimUNC
analogous to Disk/VerbatimDisk), ensuring comparisons of UNC roots become
case-insensitive while keeping existing Disk/VerbatimDisk handling and behavior
of relative_target_for unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cb8020f-6367-450b-b68f-476dcd7995ab
📒 Files selected for processing (2)
pacquet/crates/fs/src/symlink_dir.rspacquet/crates/fs/src/symlink_dir/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). (10)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Analyze (javascript)
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Dylint
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions must match pnpm exactly, includingpnpm:<channel>events, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Prefer real fixtures (tempfile::TempDir, mocked registry, integration tests) for testing; use dependency-injection seams only for branches real fixtures cannot cover portably (filesystem errors, deterministic time, process-global state, external-service paths)
Declare a newtype wrapper for branded string types rather than collapsing them into plain String or &str to preserve type-safety from the TypeScript implementation
If upstream pnpm always validates before constructing a branded string type, validate in Rust too by constructing only via TryFrom and/or FromStr
If upstream pnpm never validates a branded string type, expose an infallible From constructor for type-safety without validation
If upstream pnpm occasionally constructs a branded string type without validation, expose a from_str_unchecked constructor alongside the validating constructor
For branded types that cross JSON, YAML, or INI boundaries (manifest files, lockfiles, state files, config files), wire serde validation using #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls on branded types; fall back to manual impl only for custom logic
Model upstream pnpm string literal unions (e.g., 'auto' | 'always' | 'never') as Rust enums, not newtype wrappers, since the set of valid values is closed
Treat upstream pnpm template literal types (e.g.,${string}@${string}) the same as branded string types with validation discipline applied
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-coun...
Files:
pacquet/crates/fs/src/symlink_dir/tests.rspacquet/crates/fs/src/symlink_dir.rs
🔇 Additional comments (2)
pacquet/crates/fs/src/symlink_dir.rs (1)
41-55: LGTM!pacquet/crates/fs/src/symlink_dir/tests.rs (1)
10-14: LGTM!Also applies to: 152-194
There was a problem hiding this comment.
Pull request overview
Fixes pacquet’s Windows symlink target computation so cross-drive links fall back to an absolute target (avoiding invalid ..\..\C:\...-style paths), aligning behavior with Node’s path.win32.relative and preventing ERROR_INVALID_PARAMETER failures in Windows CI.
Changes:
- Add an explicit Windows “different root” check before calling
pathdiff::diff_paths, returning the absolute target when roots differ. - Introduce
same_path_root()(Windows-only) to compare drive/UNC roots (with drive-letter case normalization). - Add Windows-gated unit tests covering cross-drive fallback and same-drive relative behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pacquet/crates/fs/src/symlink_dir.rs |
Adds Windows root comparison to avoid invalid relative targets across drives. |
pacquet/crates/fs/src/symlink_dir/tests.rs |
Adds Windows-only tests validating cross-drive fallback and same-drive relative targets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11721 +/- ##
=======================================
Coverage 89.72% 89.73%
=======================================
Files 129 129
Lines 14970 14973 +3
=======================================
+ Hits 13432 13436 +4
+ Misses 1538 1537 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`Prefix::Disk('C')` and `Prefix::VerbatimDisk('C')` name the same
physical drive, but compare unequal as `Component::Prefix` values, so
the cross-root guard added in the previous commit would unnecessarily
fall back to an absolute target whenever one input was a verbatim
`\\?\C:\...` path. Worse, `pathdiff::diff_paths` itself would emit a
re-anchored garbage path on the variant-mismatched case if the guard
ever let it through.
Run both inputs through `dunce::simplified` on Windows, which strips
the `\\?\` prefix when the non-verbatim form is still valid, so
mixed verbatim and non-verbatim views of the same drive collapse
onto a common `Prefix::Disk` before the comparison and the diff.
Tighten `case_normalize` to map `VerbatimDisk` onto `Disk` for the
residual long-path case that `dunce` declines to simplify.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.33685072514,
"stddev": 0.07573115772601717,
"median": 2.31574460614,
"user": 2.74642874,
"system": 3.5770612199999996,
"min": 2.22707558814,
"max": 2.46609302214,
"times": [
2.46609302214,
2.28140662714,
2.4106847181399997,
2.40482110614,
2.3141119031399997,
2.38373412314,
2.31737730914,
2.26550869314,
2.29769416114,
2.22707558814
]
},
{
"command": "pacquet@main",
"mean": 2.2950896353399997,
"stddev": 0.054567529649498785,
"median": 2.2893971836399998,
"user": 2.7146039399999995,
"system": 3.5631761200000005,
"min": 2.21696474314,
"max": 2.42242382814,
"times": [
2.31971937914,
2.26170465714,
2.30866765614,
2.42242382814,
2.21696474314,
2.2863822101399998,
2.31363555214,
2.27624599014,
2.29241215714,
2.25274018014
]
},
{
"command": "pnpm",
"mean": 4.587140807839999,
"stddev": 0.03964875794997593,
"median": 4.58053169714,
"user": 7.76981894,
"system": 4.0624888200000004,
"min": 4.52853245814,
"max": 4.64918940314,
"times": [
4.64918940314,
4.64165533114,
4.60281644114,
4.55706119014,
4.54906712214,
4.61514915914,
4.58261886814,
4.57844452614,
4.56687357914,
4.52853245814
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.69225413476,
"stddev": 0.02497023195222664,
"median": 0.6845262157600001,
"user": 0.37485168,
"system": 1.5725122399999998,
"min": 0.67417617826,
"max": 0.75679986326,
"times": [
0.75679986326,
0.68954214326,
0.68348676626,
0.68428750726,
0.68798820526,
0.71053164126,
0.6755072332600001,
0.67417617826,
0.68476492426,
0.67545688526
]
},
{
"command": "pacquet@main",
"mean": 0.6845512568600001,
"stddev": 0.010409413860732531,
"median": 0.68415730576,
"user": 0.37086248,
"system": 1.57610154,
"min": 0.66638348726,
"max": 0.69983481626,
"times": [
0.69983481626,
0.69981988426,
0.68483144426,
0.68029857126,
0.68710691026,
0.68348316726,
0.6728819902600001,
0.68271188926,
0.66638348726,
0.68816040826
]
},
{
"command": "pnpm",
"mean": 2.5179554629600003,
"stddev": 0.07783932812265315,
"median": 2.52197387126,
"user": 2.9938250799999997,
"system": 2.20636214,
"min": 2.38225528126,
"max": 2.6315241802600005,
"times": [
2.57575045926,
2.5215731162600004,
2.6315241802600005,
2.5223746262600004,
2.5707746192600003,
2.58194002826,
2.38225528126,
2.41150384126,
2.48174477026,
2.50011370726
]
}
]
} |
`clippy::needless_return` rejects the explicit `return` in the
Windows `{ ... }` block because the block itself is the function's
tail expression on Windows. The block-with-cfg-attribute layout
also obscured that the function body shrinks to a single tail
expression on non-Windows. Extracting two `#[cfg]`-gated
`relative_target_inner` helpers makes both targets read as a
straight tail expression and drops the clippy violation.
Collapsing `Disk` and `VerbatimDisk` (and the UNC analogues) onto a common variant in `same_path_root` is unsafe when one input is a verbatim path that `dunce::simplified` declined to strip (e.g., a path long enough that the non-verbatim form would exceed the legacy limit). `same_path_root` would declare the two paths same-root, but `pathdiff::diff_paths` would still walk `Component::Prefix` for equality and emit a re-anchored garbage path on the variant mismatch — the same `..\\..\\C:\\Users\\...` shape that triggers `ERROR_INVALID_PARAMETER` (os error 87) on Windows. Restore the variant-strict comparison so mismatched variants fall back to an absolute target, which is the correct outcome on the edge case. The common short-path mixed-form case (`\\?\C:\foo` paired with `C:\bar`) still produces a relative target because `dunce::simplified` normalizes the variants upstream of this check.
Add an explicit rule to AGENTS.md: when a behavioral scenario or edge case is already captured by a test (name, setup, assertions), do not re-narrate it in a doc comment on the implementation. The doc comment should state the contract; the test demonstrates the behavior. Apply the rule to the recently added Windows symlink fix. The doc comments on `relative_target_for` and `same_path_root` had grown into mini essays restating the `ERROR_INVALID_PARAMETER` failure mode, the CI scenario, and the `dunce::simplified` rationale, all of which is covered by the three Windows tests. Trim both to the contract plus one short note on the only non-obvious invariant (why `same_path_root` is variant-strict). The tests in turn shed their multi-paragraph preambles in favor of self-explanatory names and direct assertions, keeping only a one-line regression pointer on the CI-failure test.
The previous doc claimed "same UNC share" as a match condition, but the implementation requires identical `Prefix` after `dunce::simplified` with only drive-letter case folding. UNC server/share differences in case or `UNC`/`VerbatimUNC` variant fall back to absolute.
Pulls in dfd8fbf "chore: update pnpm-lock.yaml" (#11559), which bumps `brace-expansion` from 5.0.5 to 5.0.6 — the upgrade the Audit dependencies CI job is gating on. Also pulls in e01d2bc "fix(pacquet/fs): fall back to absolute target across Windows drives" (#11721), which doesn't touch any file this branch edits. https://claude.ai/code/session_01Btd6rp2MgfXb3ArHwSJpqA
Summary
relative_target_for(used bysymlink_dirto mirror pnpm'ssymlink-dir) computed a relative symlink target viapathdiff::diff_paths. On Windows, when the target and link live on different drives,pathdiffdoes not returnNone; it walks components and emits a re-anchored garbage path like..\..\C:\Users\..., which Windows rejects withERROR_INVALID_PARAMETER(os error 87).D:and the global store installed bysetup-pnpmlives onC:.path.win32.relative, which returns the absolutetoargument when the two paths share no common root: detect unequalComponent::Prefixvalues up front and pass the absolute target through unchanged. Drive-letter comparison is case-insensitive to match NTFS and Node.js semantics.Test plan
cargo check -p pacquet-fs --tests --lockedcargo clippy -p pacquet-fs --tests --locked -- --deny warningscargo fmt --checkcargo nextest run -p pacquet-fs(24/24 pass on macOS; the two Windows-gated unit tests exerciserelative_target_fordirectly without a real symlink syscall, and run when CI compiles for*-windows-*)Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Style