Skip to content

fix(pacquet/fs): fall back to absolute target across Windows drives#11721

Merged
zkochan merged 6 commits into
mainfrom
fix-pacquet-symlinks-windows
May 18, 2026
Merged

fix(pacquet/fs): fall back to absolute target across Windows drives#11721
zkochan merged 6 commits into
mainfrom
fix-pacquet-symlinks-windows

Conversation

@zkochan

@zkochan zkochan commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

  • pacquet's relative_target_for (used by symlink_dir to mirror pnpm's symlink-dir) computed a relative symlink target via pathdiff::diff_paths. On Windows, when the target and link live on different drives, pathdiff does not return None; it walks components and emits a re-anchored garbage path like ..\..\C:\Users\..., which Windows rejects with ERROR_INVALID_PARAMETER (os error 87).
  • This is the failure mode the Windows CI hit (e.g. run 26037300862, job 76557020777), where the workspace lives on D: and the global store installed by setup-pnpm lives on C:.
  • Mirror Node.js's path.win32.relative, which returns the absolute to argument when the two paths share no common root: detect unequal Component::Prefix values 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 --locked
  • cargo clippy -p pacquet-fs --tests --locked -- --deny warnings
  • cargo fmt --check
  • cargo nextest run -p pacquet-fs (24/24 pass on macOS; the two Windows-gated unit tests exercise relative_target_for directly without a real symlink syscall, and run when CI compiles for *-windows-*)
  • Windows CI passes on this branch

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

  • Bug Fixes

    • Improved Windows symlink handling so targets on different roots (e.g., cross-drive/UNC mismatches) return a safe absolute path instead of an invalid relative path.
  • Tests

    • Added Windows-specific unit tests for cross-drive handling, same-drive relative targets, and verbatim vs plain path equivalence.
  • Chores

    • Added a Windows-only build dependency to support path-root handling.
  • Style

    • Clarified guidance to avoid duplicating tests inside implementation doc comments.

Review Change Stack

`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.
Copilot AI review requested due to automatic review settings May 18, 2026 15:42
@coderabbitai

coderabbitai Bot commented May 18, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

relative_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.

Changes

Windows Symlink Cross-Root Safeguard

Layer / File(s) Summary
relative_target_for docs and implementation
pacquet/crates/fs/src/symlink_dir.rs
Updated documentation and logic: route through relative_target_inner, simplify inputs, and on Windows use a same-root check to decide between a pathdiff relative target or falling back to the absolute original.
relative_target_inner variants
pacquet/crates/fs/src/symlink_dir.rs
Introduces #[cfg(windows)] and #[cfg(not(windows))] variants: Windows simplifies paths, gates on same_path_root, then uses pathdiff::diff_paths with absolute fallback; non-Windows keeps pathdiff::diff_paths with the same fallback.
same_path_root helper
pacquet/crates/fs/src/symlink_dir.rs
New Windows-only helper extracts and compares first path prefixes, normalizes Disk/VerbatimDisk drive letters case-insensitively, treats rootless paths as matching, and returns whether prefixes are equal.
Windows-only dependency
pacquet/crates/fs/Cargo.toml
Adds dunce under target.'cfg(windows)'.dependencies to support path simplification on Windows.
Windows-only tests for relative_target_for
pacquet/crates/fs/src/symlink_dir/tests.rs
Adds three #[cfg(windows)] unit tests: cross-drive returns absolute target, same-drive yields a relative path, and verbatim vs plain drive paths on the same drive produce a relative target.
AGENTS.md guidance
pacquet/AGENTS.md
Adds a bullet that tests should serve as documentation and doc comments should not duplicate test assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • pnpm/pnpm#11684: Related symlink target computation changes in the same symlink_dir module.

Poem

🐰 I hopped across a Windows drive,
Where relative paths wouldn't survive.
I checked the roots with careful art,
And gave the absolute its part.
Now symlinks find their proper way.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main fix: preventing invalid relative symlink targets when crossing Windows drives by falling back to absolute paths.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-pacquet-symlinks-windows

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pacquet/crates/fs/src/symlink_dir.rs (1)

56-90: 💤 Low value

Windows 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_normalize implementation handles Disk and VerbatimDisk prefixes but leaves UNC paths (server/share names) unchanged. Windows UNC server and share names are also case-insensitive, so \\SERVER\share and \\server\share refer 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 UNC and VerbatimUNC server/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

📥 Commits

Reviewing files that changed from the base of the PR and between bfa861f and 6507516.

📒 Files selected for processing (2)
  • pacquet/crates/fs/src/symlink_dir.rs
  • pacquet/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, including pnpm:<channel> events, payload, and ordering so @pnpm/cli.default-reporter parses 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.rs
  • pacquet/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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pacquet/crates/fs/src/symlink_dir.rs
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      8.1±0.36ms   535.8 KB/sec    1.01      8.2±0.54ms   532.0 KB/sec

@codecov-commenter

codecov-commenter commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.73%. Comparing base (bfa861f) to head (89acd30).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`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.
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.337 ± 0.076 2.227 2.466 1.02 ± 0.04
pacquet@main 2.295 ± 0.055 2.217 2.422 1.00
pnpm 4.587 ± 0.040 4.529 4.649 2.00 ± 0.05
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 692.3 ± 25.0 674.2 756.8 1.01 ± 0.04
pacquet@main 684.6 ± 10.4 666.4 699.8 1.00
pnpm 2518.0 ± 77.8 2382.3 2631.5 3.68 ± 0.13
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.
Copilot AI review requested due to automatic review settings May 18, 2026 16:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Comment thread pacquet/crates/fs/src/symlink_dir.rs
zkochan added 2 commits May 18, 2026 18:25
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.
Copilot AI review requested due to automatic review settings May 18, 2026 16:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Comment thread pacquet/crates/fs/src/symlink_dir.rs Outdated
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.
@zkochan zkochan merged commit e01d2bc into main May 18, 2026
26 of 28 checks passed
@zkochan zkochan deleted the fix-pacquet-symlinks-windows branch May 18, 2026 16:54
KSXGitHub pushed a commit that referenced this pull request May 18, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants