CI: run Windows pnpm e2e tests instead of skipping (#1054)

.github/workflows/ci.yml

@@ -1700,8 +1700,7 @@ jobs:
           # the matching matrix key, which the suites treat as unset.
           SOCKET_PATCH_NPM_E2E_REQUIRED: ${{ matrix.npm_required || '' }}
           # `pnpm_required` does the same for the pnpm-family suites
-          # (corepack pnpm / pnpm on PATH). Set on the Linux rows only; the
-          # Windows and macOS rows have not been checked against it yet.
+          # (corepack pnpm / pnpm on PATH), on every OS that runs them.
           SOCKET_PATCH_PNPM_E2E_REQUIRED: ${{ matrix.pnpm_required || '' }}
           SOCKET_PATCH_CARGO_E2E_REQUIRED: ${{ matrix.suite == 'e2e_safety_cargo_build' && '1' || '' }}
           SOCKET_PATCH_UV_E2E_REQUIRED: ${{ matrix.uv != '' && '1' || '' }}
@@ -1942,11 +1941,13 @@ jobs:
         include:
           - os: windows-latest
             suite: e2e_safety_pnpm
+            pnpm_required: '1'
           - os: windows-latest
             suite: e2e_redirect_npm_build
             npm_required: '1'
           - os: windows-latest
             suite: e2e_redirect_rush_sim
+            pnpm_required: '1'
           - os: windows-latest
             suite: e2e_redirect_bun_build e2e_vendor_bun_build mode_migration_bun
             bun: '1.4.2'
@@ -1959,7 +1960,7 @@ jobs:
           - {os: windows-latest, suite: e2e_safety_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix, vlt_store_linker: 'hardlink'}
           - {os: windows-latest, suite: e2e_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix}
           - {os: windows-latest, suite: e2e_vlt, vlt: '1.0.0-rc.14', test_filter: --include-ignored vlt_pinned_matrix}
-          - {os: windows-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix'}
+          - {os: windows-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix', pnpm_required: '1'}
           - {os: windows-latest, suite: e2e_vendor_jvm_build, jvm_tool: maven, maven: '3.9.16', test_filter: '--ignored maven_reactor'}
           - {os: windows-latest, suite: e2e_vendor_jvm_build, jvm_tool: gradle, gradle: '8.14.3', java: '17', test_filter: '--ignored gradle_multi_project'}
     runs-on: ${{ vars.DISABLE_DEPOT_RUNNERS != 'true' && (matrix.os == 'ubuntu-latest' && 'depot-ubuntu-24.04-4' || matrix.os == 'ubuntu-22.04' && 'depot-ubuntu-22.04-4') || matrix.os }}
@@ -1979,11 +1980,13 @@ jobs:
         include:
           - os: macos-latest
             suite: e2e_safety_pnpm
+            pnpm_required: '1'
           - os: macos-latest
             suite: e2e_redirect_npm_build
             npm_required: '1'
           - os: macos-latest
             suite: e2e_redirect_rush_sim
+            pnpm_required: '1'
           - os: macos-latest
             suite: e2e_redirect_bun_build
             bun: '1.4.2'
@@ -2001,7 +2004,7 @@ jobs:
           - {os: macos-latest, suite: mode_migration_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix}
           - {os: macos-latest, suite: e2e_safety_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix, vlt_store_linker: 'hardlink'}
           - {os: macos-latest, suite: e2e_vlt, vlt: '1.2.0', test_filter: --include-ignored vlt_pinned_matrix}
-          - {os: macos-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix'}
+          - {os: macos-latest, suite: e2e_redirect_pnpm_build, test_filter: '--ignored --skip pnpm_pinned_matrix', pnpm_required: '1'}
           - {os: macos-latest, suite: e2e_redirect_uv_build, uv: '0.12.17'}
           - {os: macos-latest, suite: e2e_vendor_pypi_build, uv: '0.12.17', test_filter: --include-ignored}
           - {os: macos-latest, suite: e2e_vex_build, test_filter: 'poetry:: --ignored', poetry: '2.4.3'}

crates/socket-patch-cli/tests/common/hermetic.rs

@@ -88,6 +88,19 @@ pub fn binary_command() -> Command {
     command(Path::new(env!("CARGO_BIN_EXE_socket-patch")))
 }
 
+/// A `Command` for the package-manager tool `name` on `PATH`, resolved with
+/// the CLI's `resolve_tool` (every `PATHEXT` extension on Windows). npm and Node install `pnpm`, `corepack` and `npm` as `.cmd`
+/// shims there, and a bare `Command::new("corepack")` only finds
+/// `corepack.exe`, so the pnpm suites' probes reported the tool missing and
+/// every Windows test skipped. A tool that isn't on `PATH` keeps the bare
+/// name, so spawning it still fails the way the probes expect.
+pub fn tool_command(name: &str) -> Command {
+    Command::new(
+        socket_patch_core::utils::process::resolve_tool(name)
+            .map_or_else(|| name.into(), std::path::PathBuf::into_os_string),
+    )
+}
+
 /// Remove every ambient `SOCKET_*` var from `cmd`, except the telemetry
 /// opt-outs (an opted-out developer stays opted out), `SOCKET_NO_CONFIG`
 /// and `SOCKET_NO_UPDATE_CHECK`. [`command`] runs this; package-manager

crates/socket-patch-cli/tests/common/mod.rs

@@ -279,7 +279,7 @@ pub fn vlt_run(cwd: &Path, vlt_js: &Path, args: &[&str], extra_env: &[(&str, &st
 }
 
 fn run_toolchain(cwd: &Path, exe: &str, args: &[&str], extra_env: &[(&str, &str)]) {
-    let mut cmd = Command::new(exe);
+    let mut cmd = hermetic::tool_command(exe);
     cmd.args(args).current_dir(cwd);
     // Sandbox the caches first so `extra_env` can still override any
     // individual one (`Command` env ops are last-write-wins per name).

crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs

@@ -124,7 +124,7 @@ fn pnpm_command(pm: &str) -> Command {
     if let Some(bin) = std::env::var_os("SOCKET_PATCH_PNPM_E2E_BIN") {
         Command::new(bin)
     } else {
-        let mut cmd = Command::new("corepack");
+        let mut cmd = hermetic::tool_command("corepack");
         cmd.arg(pm);
         cmd
     }

crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs

@@ -29,7 +29,7 @@ mod common;
 use common::binary;
 
 use std::path::Path;
-use std::process::{Command, Output, Stdio};
+use std::process::{Output, Stdio};
 
 use sha2::{Digest, Sha512};
 use wiremock::matchers::{method, path, path_regex};
@@ -82,7 +82,7 @@ fn has_corepack_pm(pm: &str) -> bool {
     };
     // Isolated too: this probe is what actually downloads the package manager
     // the first time, and corepack stores it under `COREPACK_HOME`.
-    let mut cmd = Command::new("corepack");
+    let mut cmd = hermetic::tool_command("corepack");
     cmd.args([pm, "--version"])
         .current_dir(probe.path())
         .env("COREPACK_ENABLE_DOWNLOAD_PROMPT", "0");
@@ -95,7 +95,7 @@ fn has_corepack_pm(pm: &str) -> bool {
 }
 
 fn has_command(cmd: &str) -> bool {
-    let mut probe = Command::new(cmd);
+    let mut probe = hermetic::tool_command(cmd);
     probe.arg("--version");
     cache_env::isolate(&mut probe);
     probe
@@ -107,7 +107,7 @@ fn has_command(cmd: &str) -> bool {
 }
 
 fn corepack(cwd: &Path, pm: &str, args: &[&str], extra_env: &[(&str, &str)]) -> Output {
-    let mut cmd = Command::new("corepack");
+    let mut cmd = hermetic::tool_command("corepack");
     cmd.arg(pm).args(args).current_dir(cwd);
     hermetic::scrub_socket_vars(&mut cmd);
     hermetic::scrub_extra(&mut cmd, &[hermetic::Extra::Venv]);
@@ -652,7 +652,7 @@ async fn rush_hosted_real_rush_update_install() {
     let rush = |args: &[&str]| -> Output {
         let mut full = vec!["x", "-y", rush_pkg.as_str()];
         full.extend_from_slice(args);
-        let mut cmd = Command::new("npm");
+        let mut cmd = hermetic::tool_command("npm");
         cmd.args(&full).current_dir(root);
         hermetic::scrub_socket_vars(&mut cmd);
         hermetic::scrub_extra(&mut cmd, &[hermetic::Extra::Venv]);

crates/socket-patch-cli/tests/e2e_safety_pnpm.rs

@@ -28,7 +28,7 @@ use std::path::{Path, PathBuf};
 mod common;
 
 use common::{
-    assert_run_ok, git_sha256, git_sha256_file, has_command, pnpm_run, write_blob,
+    assert_run_ok, git_sha256, git_sha256_file, hermetic, pnpm_run, write_blob,
     write_minimal_manifest, write_package_json, PatchEntry,
 };
 
@@ -45,6 +45,22 @@ fn pnpm_required() -> bool {
     std::env::var_os("SOCKET_PATCH_PNPM_E2E_REQUIRED").is_some_and(|v| !v.is_empty())
 }
 
+/// `pnpm --version` succeeds. Probed through [`hermetic::tool_command`]: on
+/// Windows pnpm is the `pnpm.cmd` shim `npm install -g` writes, which
+/// `common::has_command`'s bare `Command::new` never finds. The exit status
+/// counts too, since a resolved `.cmd` spawns through `cmd.exe` even when
+/// the shim itself is broken.
+fn has_pnpm() -> bool {
+    let mut probe = hermetic::tool_command("pnpm");
+    probe.arg("--version");
+    common::cache_env::isolate(&mut probe);
+    probe
+        .stdout(std::process::Stdio::null())
+        .stderr(std::process::Stdio::null())
+        .status()
+        .is_ok_and(|s| s.success())
+}
+
 /// Print the SKIP line for a missing pnpm, or panic under [`pnpm_required`].
 fn skip_missing_pnpm() {
     assert!(
@@ -249,7 +265,7 @@ fn file_identity(path: &Path) -> (u64, u64) {
 #[test]
 #[ignore]
 fn pnpm_install_produces_symlinked_layout() {
-    if !has_command("pnpm") {
+    if !has_pnpm() {
         skip_missing_pnpm();
         return;
     }
@@ -313,7 +329,7 @@ fn pnpm_install_produces_symlinked_layout() {
 #[test]
 #[ignore]
 fn apply_in_a_does_not_mutate_b_or_store() {
-    if !has_command("pnpm") {
+    if !has_pnpm() {
         skip_missing_pnpm();
         return;
     }
@@ -417,7 +433,7 @@ fn apply_in_a_does_not_mutate_b_or_store() {
 #[test]
 #[ignore]
 fn pnpm_install_in_b_does_not_revert_a() {
-    if !has_command("pnpm") {
+    if !has_pnpm() {
         skip_missing_pnpm();
         return;
     }
@@ -532,7 +548,7 @@ fn pnpm_install_in_b_does_not_revert_a() {
 #[test]
 #[ignore]
 fn apply_in_pnpm_project_emits_layout_note() {
-    if !has_command("pnpm") {
+    if !has_pnpm() {
         skip_missing_pnpm();
         return;
     }

scripts/tests/test_ci_e2e_tiers.py

@@ -273,8 +273,8 @@ def test_compat_workflow_builds_its_own_binaries(self):
 
 class PnpmRequiredGate(unittest.TestCase):
     """The pnpm-family suites soft-skip when pnpm is missing, and libtest
-    counts a skip as `ok`, so their Linux rows must set the fail-instead-of-
-    skip gate (#1054)."""
+    counts a skip as `ok`, so every row that runs them, on every OS, must set
+    the fail-instead-of-skip gate (#1054)."""
 
     suites = {"e2e_redirect_pnpm_build", "e2e_safety_pnpm", "e2e_redirect_rush_sim"}
     jobs = ("e2e", "e2e-full", "e2e-extended", "e2e-windows", "e2e-macos")
@@ -283,20 +283,42 @@ def test_run_step_reads_the_row_key(self):
         run = dict(rows_mod.steps(JOBS["e2e"]))["Run e2e tests"]
         self.assertIn("SOCKET_PATCH_PNPM_E2E_REQUIRED: ${{ matrix.pnpm_required || '' }}", run)
 
-    def test_every_linux_pnpm_row_sets_the_gate(self):
-        gated = set()
+    def test_every_pnpm_row_sets_the_gate(self):
+        gated = {}
         for job in self.jobs:
             for row in rows(job):
-                if self.suites & set(row["suite"].split()) and row["os"].startswith("ubuntu"):
+                if self.suites & set(row["suite"].split()):
                     with self.subTest(job=job, row=row):
                         self.assertEqual(row.get("pnpm_required"), "1")
-                    gated |= self.suites & set(row["suite"].split())
+                    for suite in self.suites & set(row["suite"].split()):
+                        gated.setdefault(suite, set()).add(row["os"].split("-")[0])
                 elif "pnpm_required" in row:
                     self.fail(f"{job} row outside the checked set sets pnpm_required: {row}")
-        self.assertEqual(gated, self.suites)
+        self.assertEqual(gated, {s: {"ubuntu", "windows", "macos"} for s in self.suites})
         self.assertIn("e2e_redirect_pnpm_build", {r["suite"] for r in rows("e2e") if r.get("pnpm_required")},
                       "the PR tier must keep its gated pnpm row")
 
+    def test_suites_resolve_windows_shims(self):
+        # A bare `Command::new("pnpm")` / `Command::new("corepack")` can't
+        # spawn the `.cmd` shims npm and Node install on Windows, so every
+        # Windows test skipped (passed in 0.02 s) until the gate made that a
+        # failure. `hermetic::tool_command` resolves them through PATHEXT.
+        for suite in sorted(self.suites):
+            with self.subTest(suite=suite):
+                src = (TESTS / f"{suite}.rs").read_text(encoding="utf-8")
+                self.assertNotRegex(src, r'Command::new\("(pnpm|corepack|npm)(\.cmd)?"\)')
+                if "fn has_command(" in src:
+                    # A suite-local probe must resolve shims itself...
+                    local = src[src.index("fn has_command("):]
+                    self.assertIn("hermetic::tool_command(cmd)", local[:local.index("\n}\n")])
+                else:
+                    # ...and `common::has_command` does not.
+                    self.assertNotRegex(src, r'has_command\("(pnpm|corepack|npm)"\)')
+        # `pnpm_run` goes through the shared runner.
+        common = (TESTS / "common" / "mod.rs").read_text(encoding="utf-8")
+        runner = common[common.index("fn run_toolchain("):]
+        self.assertIn("hermetic::tool_command(exe)", runner[:runner.index("\n}\n")])
+
     def test_every_gated_suite_honours_the_gate(self):
         for suite in sorted(self.suites):
             with self.subTest(suite=suite):