fix(executor): run lifecycle scripts from long global virtual store slots on Windows - #15147

Description

The changeset sentence combines the prior lifecycle-script failure with the claim that it is fixed
instead of stating the corrected behavior separately. Readers encountering this Windows bug-fix
entry must infer what now succeeds, and the previous failure is not documented in its own past-tense
sentence.

Code

.changeset/fix-gvs-lifecycle-script-cwd-windows.md[5]

+Fixed lifecycle scripts failing to spawn under `enableGlobalVirtualStore` on Windows when running scoped dependencies [pnpm/pnpm#15111](https://github.com/pnpm/pnpm/issues/15111).

Relevance

●●● Strong

Recent changeset reviews accepted separating prior failures from corrected behavior, matching this
exact repository rule.

PR-#15029
PR-#15121

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 3058498 requires previous buggy behavior to appear in a separate past-tense
sentence. The changed changeset instead uses one sentence, `Fixed lifecycle scripts failing to
spawn...`, to express both the defect and its resolution.

Rule 3058498: Describe previous buggy behavior in a separate past-tense sentence
.changeset/fix-gvs-lifecycle-script-cwd-windows.md[5-5]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The changeset combines the previous failure and its resolution in one sentence rather than describing the current behavior and then the old behavior separately.

## Fix Focus Areas
- .changeset/fix-gvs-lifecycle-script-cwd-windows.md[5-5]

## Recommended Fix
Replace the entry with two sentences. State first that lifecycle scripts for scoped dependencies now spawn successfully on Windows when `enableGlobalVirtualStore` is enabled, then state in a separate past-tense sentence that they previously failed to spawn.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The doc comment on push_slash_separated_path merely says that slash-separated segments are pushed
onto path, repeating the function name and its one-line body. It provides no additional rationale
or contract concerning empty segments, traversal components, or the expected relative-path invariant
for later callers.

Code

pnpm/crates/fs/src/relative_path.rs[71]

+/// Push each `/`-separated segment of `rel` onto `path`.

Relevance

●●● Strong

Recent Rust reviews accepted removing or rewriting comments that merely restate helper behavior or
names.

PR-#15119
PR-#15132

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 713467 rejects comments that only mirror an identifier or simple expression. The
comment directly restates both push_slash_separated_path and path.extend(rel.split('/')) without
adding intent or constraints.

Rule 713467: Avoid redundant comments that restate the code
pnpm/crates/fs/src/relative_path.rs[71-73]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new helper's doc comment repeats its name and implementation without documenting useful constraints or rationale.

## Fix Focus Areas
- pnpm/crates/fs/src/relative_path.rs[71-73]

## Recommended Fix
Rewrite the comment to document the expected relative input and behavior for empty or traversal segments, or remove it if the repository does not require public-item documentation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

.changeset/fix-gvs-lifecycle-script-cwd-windows.md says dependencies with build scripts now
install on Windows under enableGlobalVirtualStore, but shorten_to_working_dir_limit returns an
over-limit path unchanged when normalization and the 8.3 conversion cannot shorten it. Stores on
volumes without short-name generation and long network-share paths therefore still reach process
spawning unchanged, so the unqualified release-note outcome extends beyond the implemented cases.

Code

.changeset/fix-gvs-lifecycle-script-cwd-windows.md[5]

+Dependencies that run build scripts now install on Windows under `enableGlobalVirtualStore`. A global virtual store slot can be longer than the working directory Windows allows a process. The install used to fail there with `The directory name is invalid. (os error 267)` [#15111](https://github.com/pnpm/pnpm/issues/15111).

Relevance

●●● Strong

Recent precedents accept release-note qualifications when implementation supports only narrower
conditions than the claim.

PR-#15073
PR-#14963
PR-#15132

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 3365343 requires changeset claims to match the implemented user-visible behavior. The changeset
makes an unconditional Windows success claim, while the implementation documents and returns the
original over-limit path when no shortening method succeeds and excludes network-share paths from
8.3 conversion.

.changeset/fix-gvs-lifecycle-script-cwd-windows.md[5-5]
pnpm/crates/executor/src/script_working_dir.rs[39-51]
pnpm/crates/executor/src/script_working_dir.rs[76-82]
pnpm/crates/executor/src/script_working_dir.rs[98-107]
Skill: review-code

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The changeset promises that global-virtual-store build scripts now install on Windows without disclosing that overlong paths still fail when they cannot be normalized or converted to an 8.3 path.

## Fix Focus Areas
- .changeset/fix-gvs-lifecycle-script-cwd-windows.md[5-5]

## Recommended Fix
Revise the release note to state that the fix applies when pnpm can shorten the working directory, without claiming success for every Windows global virtual store path.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

shorten_to_working_dir_limit applies pnpm_fs::lexical_normalize to over-limit paths, removing
.. without preserving symlink traversal semantics and potentially selecting a different package
root. On Windows, a repository-relative storeDir containing symlink/.. triggers this once the
slot exceeds 259 UTF-16 units, so the manifest, lifecycle environment, and PATH can refer to the
original root while shell and emulated lifecycle commands execute from the rewritten root.

Code

pnpm/crates/executor/src/script_working_dir.rs[R47-49]

+    let normalized = pnpm_fs::lexical_normalize(path);
+    if utf16_len(&normalized) <= MAX_WORKING_DIR_CHARS {
+        return Cow::Owned(normalized);

Relevance

●● Moderate

Potential symlink-sensitive path mismatch is plausible, but history lacks a closely matching
accepted or rejected precedent.

PR-#13403
PR-#13445

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The shortening function uses filesystem-free lexical normalization for every over-limit path when
the normalized spelling fits, and that helper removes preceding components without inspecting
symlinks. Repository-controlled relative store paths can contain .., so a symlink can make the
normalized destination differ from the installed package root; manifest selection remains based on
the original root, while the changed shell and emulator call sites use the rewritten root as their
working directory, demonstrating the mismatched execution path and the failure to keep path
resolution within its intended destination.

pnpm/crates/executor/src/script_working_dir.rs[26-51]
pnpm/crates/executor/src/lifecycle.rs[399-410]
pnpm/crates/executor/src/lifecycle.rs[460-466]
pnpm/crates/config/src/workspace_yaml/apply.rs[130-154]
pnpm/crates/fs/src/lexical_normalize.rs[6-17]
pnpm/crates/fs/src/lexical_normalize.rs[90-101]
pnpm/crates/executor/src/lifecycle.rs[176-184]
pnpm/crates/executor/src/lifecycle.rs[402-410]
pnpm/crates/executor/src/lifecycle.rs[463-466]
Skill: review-code

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Long Windows lifecycle working directories are lexically normalized before execution, even though removing `..` after a symlink can resolve to a different directory. This can run a lifecycle script from a working directory other than the package root whose manifest supplied the script, while the lifecycle environment and `PATH` still reference the original root.

## Fix Focus Areas
- pnpm/crates/executor/src/script_working_dir.rs[43-51]
- pnpm/crates/executor/src/lifecycle.rs[402-410]
- pnpm/crates/executor/src/lifecycle.rs[463-466]
- pnpm/crates/executor/src/script_working_dir/tests.rs[23-46]

## Recommended Fix
Replace filesystem-free lexical normalization with filesystem-aware resolution that preserves the original path's symlink semantics before obtaining a short path. Use the resolved working directory consistently for process cwd, lifecycle environment, and PATH construction; if equivalent resolution cannot be completed, retain the original path and fail rather than executing in a potentially different directory. Add a Windows regression test using an over-limit path containing a symlink followed by `..`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

a_slot_windows_refuses_still_spawns_by_its_short_name unconditionally unwraps the result of
shorter_working_dirs, although that function explicitly returns no candidate when the volume does
not generate 8.3 names. Running the Windows suite on such a volume reaches the .expect(...) after
the long-path refusal and fails the test rather than treating the unavailable fallback as an unmet
precondition.

Code

pnpm/crates/executor/src/script_working_dir/tests.rs[66]

+    let shortest = shorter_working_dirs(&slot).pop().expect("a shorter spelling of the slot");

Relevance

●●● Strong

Test unconditionally unwraps a documented optional fallback; handling absent 8.3 names is a
deterministic reliability fix.

PR-#13445
PR-#14853

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test requires .pop() to produce a path, but the production helper documents and implements an
empty result when 8.3 generation is disabled. Because the constructed test path has no lexical
normalization opportunity, a volume without short names leaves the vector empty and triggers the
added expectation.

pnpm/crates/executor/src/script_working_dir/tests.rs[63-72]
pnpm/crates/executor/src/script_working_dir.rs[48-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Windows regression test assumes every test volume generates 8.3 short names, while the production helper explicitly supports volumes where no short spelling exists. This makes the suite fail due to host filesystem configuration rather than a product regression.

## Fix Focus Areas
- pnpm/crates/executor/src/script_working_dir/tests.rs[63-72]
- pnpm/crates/executor/src/script_working_dir.rs[48-61]

## Recommended Fix
Handle an empty `shorter_working_dirs` result as an unsupported test precondition, returning from the test with a diagnostic message. Keep the canonical-path and successful-spawn assertions when a short spelling is available.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

every_spelling_offered_for_a_refused_slot_is_shorter only checks that fallback spellings are
shorter and resolve to the same directory, without spawning a lifecycle process through the fallback
path. The test can pass while every candidate still fails as a working directory, so the reported
Windows lifecycle-script fix is not directly regression-tested.

Code

pnpm/crates/executor/src/script_working_dir/tests.rs[R58-80]

+fn every_spelling_offered_for_a_refused_slot_is_shorter() {
+    let root = tempfile::Builder::new()
+        .prefix("pnpm-wd-")
+        .tempdir()
+        .expect("create temporary directory");
+    let slot = slot_dir(root.path());
+    std::fs::create_dir_all(&slot).expect("create the deep slot");
+
+    let refusal = spawn_in(&slot).expect_err("Windows must refuse a working directory this long");
+    assert!(is_refused_directory(&refusal), "expected ERROR_DIRECTORY, got {refusal:?}");
+
+    for spelling in shorter_working_dirs(&slot) {
+        assert_eq!(
+            std::fs::canonicalize(&spelling).expect("canonicalize a shorter spelling"),
+            std::fs::canonicalize(&slot).expect("canonicalize the slot"),
+            "{spelling:?} must name the same directory as the slot",
+        );
+        assert!(
+            spelling.as_os_str().len() < slot.as_os_str().len(),
+            "{spelling:?} is not shorter than the {}-character slot it stands in for",
+            slot.as_os_str().len(),
+        );
+    }

Relevance

●●● Strong

Recent history accepts strengthening regression tests to exercise the actual failure mode, not
merely indirect invariants.

PR-#15182
PR-#15153

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added Windows test creates a refused long directory and asserts only that each candidate
canonicalizes to the same directory and is shorter. No assertion starts a process with a candidate
working directory, while the production behavior being fixed is process creation from the lifecycle
directory.

pnpm/crates/executor/src/script_working_dir/tests.rs[58-80]
Skill: review-code

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Windows regression test verifies only path length and directory identity. It does not prove that the fallback working directory can actually start a child process, so the lifecycle-script failure can remain undetected.

## Fix Focus Areas
- pnpm/crates/executor/src/script_working_dir/tests.rs[58-80]

## Recommended Fix
After generating each shorter spelling, invoke a child process with that spelling as its current directory and assert that spawning succeeds. Prefer exercising the same spawn helper or lifecycle spawn path used by production so the test fails when the fallback does not solve the original working-directory refusal.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

shorter_working_dirs relies on names_the_same_dir to reject lexical normalization when ..
crosses a symlink, but the added tests only construct ordinary directories where both spellings are
equivalent. Removing the new filter would therefore leave the tests green while allowing lifecycle
scripts to execute from a different directory again.

Code

pnpm/crates/executor/src/script_working_dir.rs[67]

+    spellings.retain(|spelling| names_the_same_dir(spelling, pkg_root));

Relevance

●● Moderate

Coverage-strengthening findings are often accepted, but a recent symlink-specific test request was
rejected in similar normalization code.

PR-#14853
PR-#15140

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The production comments explicitly identify symlink traversal as the reason every candidate must be
checked, and line 67 implements that check. The revised normalization tests create only normal
directories through workspace/.., so they cannot distinguish safe lexical normalization from the
symlink case this change is intended to reject.

pnpm/crates/executor/src/script_working_dir.rs[46-52]
pnpm/crates/executor/src/script_working_dir.rs[67-78]
pnpm/crates/executor/src/script_working_dir/tests.rs[7-30]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new directory-identity check protects a security-sensitive invariant, but no regression test exercises a `..` path whose lexical normalization changes meaning because an ancestor is a symlink or junction.

## Fix Focus Areas
- pnpm/crates/executor/src/script_working_dir.rs[46-52]
- pnpm/crates/executor/src/script_working_dir/tests.rs[7-24]

## Recommended Fix
Add a platform-appropriate symlink or junction test that constructs a package path containing `symlink/..` where the normalized spelling resolves to a different directory, then assert that `shorter_working_dirs` does not offer that spelling. Ensure the test fails if the `names_the_same_dir` filtering is removed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools