From e01d2bc0a11347da85eea01029ccfc6bc270f028 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 18 May 2026 18:54:14 +0200 Subject: [PATCH] fix(pacquet/fs): fall back to absolute target across Windows drives (#11721) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(pacquet/fs): fall back to absolute target across Windows drives `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. * fix(pacquet/fs): normalize verbatim Windows paths before relative diff `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. * style(pacquet/fs): split relative_target_inner per platform `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. * fix(pacquet/fs): make same_path_root variant-strict 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. * docs(pacquet): tests are documentation; do not narrate them in code 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. * docs(pacquet/fs): describe same_path_root match shape accurately 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. --- Cargo.lock | 1 + pacquet/AGENTS.md | 1 + pacquet/crates/fs/Cargo.toml | 1 + pacquet/crates/fs/src/symlink_dir.rs | 55 +++++++++++++++++++--- pacquet/crates/fs/src/symlink_dir/tests.rs | 35 ++++++++++++++ 5 files changed, 87 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 89b2eb1f74..fc81b0477c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2087,6 +2087,7 @@ name = "pacquet-fs" version = "0.0.1" dependencies = [ "derive_more", + "dunce", "junction", "miette 7.6.0", "pathdiff", diff --git a/pacquet/AGENTS.md b/pacquet/AGENTS.md index badabab09a..af0f88f2fe 100644 --- a/pacquet/AGENTS.md +++ b/pacquet/AGENTS.md @@ -295,6 +295,7 @@ Rust-specific defaults: - **Doc comments (`///`, `//!`) document the contract.** Preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body. - **Do not restate at call sites what the callee's doc comment already says.** If `///` on the function says "no-op when …", the caller should not repeat that. Update the doc once; let every call site benefit. +- **Tests are documentation. Do not duplicate them in prose.** If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test (its name, its setup, its assertions), do not also narrate it in the doc comment on the implementation. The doc comment should state the contract once; the test demonstrates the behavior. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the *why* if it is not obvious. - **`// SAFETY:`, `// TODO:`, and similar prefixes are the exception.** They signal hidden invariants or known follow-ups that a reader cannot recover from the code alone. Prefer renaming, restructuring, or extracting a helper over leaving a comment. Reach for prose only when the right names and types genuinely cannot carry the information. diff --git a/pacquet/crates/fs/Cargo.toml b/pacquet/crates/fs/Cargo.toml index 591ed0601b..3cccc01008 100644 --- a/pacquet/crates/fs/Cargo.toml +++ b/pacquet/crates/fs/Cargo.toml @@ -19,6 +19,7 @@ pathdiff = { workspace = true } tempfile = { workspace = true } [target.'cfg(windows)'.dependencies] +dunce = { workspace = true } junction = { workspace = true } [lints] diff --git a/pacquet/crates/fs/src/symlink_dir.rs b/pacquet/crates/fs/src/symlink_dir.rs index 17e7bf39ab..4a636db9eb 100644 --- a/pacquet/crates/fs/src/symlink_dir.rs +++ b/pacquet/crates/fs/src/symlink_dir.rs @@ -36,17 +36,60 @@ pub fn symlink_dir(original: &Path, link: &Path) -> io::Result<()> { /// link's parent directory to `original`. Mirrors upstream's /// `resolveSrcOnTrueSymlink(src, dest) = path.relative(path.dirname(dest), src)`. /// -/// Callers in pacquet always pass absolute paths; if `pathdiff` cannot -/// produce a relative form (one side is relative, or they live on -/// different roots — only possible on Windows), fall back to the -/// absolute `original` so the kernel still gets a valid path. This -/// matches upstream's behavior when `path.relative` returns an -/// absolute path because the two arguments share no common root. +/// Returns an absolute path when no relative form exists between the +/// two arguments. This matches Node.js's `path.win32.relative`, which +/// returns the absolute `to` argument when the paths share no common +/// root (different drives, different UNC shares). fn relative_target_for(original: &Path, link: &Path) -> PathBuf { let parent = link.parent().unwrap_or_else(|| Path::new("")); + relative_target_inner(original, parent) +} + +#[cfg(windows)] +fn relative_target_inner(original: &Path, parent: &Path) -> PathBuf { + let original = dunce::simplified(original); + let parent = dunce::simplified(parent); + if !same_path_root(original, parent) { + return original.to_path_buf(); + } pathdiff::diff_paths(original, parent).unwrap_or_else(|| original.to_path_buf()) } +#[cfg(not(windows))] +fn relative_target_inner(original: &Path, parent: &Path) -> PathBuf { + pathdiff::diff_paths(original, parent).unwrap_or_else(|| original.to_path_buf()) +} + +/// Whether `a` and `b` have an identical `Component::Prefix` after +/// `dunce::simplified`, with drive letters case-folded. UNC shares +/// only match when their server/share are written with identical +/// casing and variant — the check has to stay in lockstep with what +/// `pathdiff::diff_paths` will tolerate, since a variant-tolerant or +/// case-tolerant comparison here would let the downstream diff emit +/// a re-anchored garbage path on a `Prefix` mismatch it cannot relate. +#[cfg(windows)] +fn same_path_root(a: &Path, b: &Path) -> bool { + fn first_prefix(path: &Path) -> Option> { + match path.components().next()? { + std::path::Component::Prefix(p) => Some(p.kind()), + _ => None, + } + } + fn case_normalize(prefix: std::path::Prefix<'_>) -> std::path::Prefix<'_> { + use std::path::Prefix::{Disk, VerbatimDisk}; + match prefix { + Disk(d) => Disk(d.to_ascii_uppercase()), + VerbatimDisk(d) => VerbatimDisk(d.to_ascii_uppercase()), + other => other, + } + } + match (first_prefix(a), first_prefix(b)) { + (Some(pa), Some(pb)) => case_normalize(pa) == case_normalize(pb), + (None, None) => true, + _ => false, + } +} + /// Remove a symlink (or junction on Windows) previously created with /// [`symlink_dir`]. /// diff --git a/pacquet/crates/fs/src/symlink_dir/tests.rs b/pacquet/crates/fs/src/symlink_dir/tests.rs index 9b3a959984..caf44ea5ef 100644 --- a/pacquet/crates/fs/src/symlink_dir/tests.rs +++ b/pacquet/crates/fs/src/symlink_dir/tests.rs @@ -7,7 +7,11 @@ //! idempotent re-symlink, retargeting a stale link, and renaming a //! non-symlink occupant out of the way. +#[cfg(windows)] +use super::relative_target_for; use super::{ForceSymlinkOutcome, force_symlink_dir, read_symlink_dir, symlink_dir}; +#[cfg(windows)] +use std::path::Path; use std::{fs, path::PathBuf}; use tempfile::tempdir; @@ -145,6 +149,37 @@ fn force_symlink_dir_creates_missing_parent_directories() { assert_eq!(resolved_link, resolved_target); } +/// Regression for the Windows CI failure where the workspace lives +/// on `D:` and the global store (installed by `setup-pnpm`) lives on +/// `C:`: `pathdiff::diff_paths` produced a re-anchored garbage path +/// that Windows rejected with `ERROR_INVALID_PARAMETER` (os error 87). +#[cfg(windows)] +#[test] +fn windows_cross_drive_symlink_target_falls_back_to_absolute() { + let target = Path::new(r"C:\Users\runneradmin\setup-pnpm\store\@babel\plugin-x"); + let link = Path::new(r"D:\a\pnpm\pnpm\node_modules\@babel\plugin-x"); + + assert_eq!(relative_target_for(target, link), target); +} + +#[cfg(windows)] +#[test] +fn windows_same_drive_symlink_target_stays_relative() { + let target = Path::new(r"C:\workspace\packages\pkg-a"); + let link = Path::new(r"C:\workspace\app\node_modules\pkg-a"); + + assert!(relative_target_for(target, link).is_relative()); +} + +#[cfg(windows)] +#[test] +fn windows_verbatim_and_plain_disk_resolve_to_same_root() { + let target = Path::new(r"\\?\C:\workspace\packages\pkg-a"); + let link = Path::new(r"C:\workspace\app\node_modules\pkg-a"); + + assert!(relative_target_for(target, link).is_relative()); +} + /// After [`force_symlink_dir`] places a symlink, [`read_symlink_dir`] /// returns the same contents. The combination covers the round-trip /// pacquet's prune sweep needs (write a slot link with