From 93575c62c046d397fef8907cc1271ffb73745a25 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:53:53 +0000 Subject: [PATCH 1/4] Start fix for #1054 Assisted-by: Claude Code:claude-opus-5-5 From 837c302d1298c631c53b155186cc0e148e909506 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 01:16:08 +0000 Subject: [PATCH 2/4] Run Windows pnpm e2e tests instead of skipping On windows-latest the pnpm-family e2e suites started pnpm and corepack with a bare Command::new, which only finds .exe files. npm and Node install both tools as .cmd shims there, so every availability probe said the tool was missing and every test skipped. libtest counts a skip as a pass, so the nightly Windows legs went green in 0.02 s while testing nothing. Add hermetic::tool_command, which resolves a tool through PATHEXT the same way the CLI does, and use it for every pnpm, corepack and npm spawn in e2e_safety_pnpm, e2e_redirect_pnpm_build and e2e_redirect_rush_sim, and in common::run_toolchain. Refs #1054 Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/common/hermetic.rs | 14 +++++++++++ crates/socket-patch-cli/tests/common/mod.rs | 2 +- .../tests/e2e_redirect_pnpm_build.rs | 2 +- .../tests/e2e_redirect_rush_sim.rs | 10 ++++---- .../socket-patch-cli/tests/e2e_safety_pnpm.rs | 24 +++++++++++++++---- 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 8c7859040..165c1b289 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -88,6 +88,20 @@ 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 the +/// way the CLI resolves its own tools (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 diff --git a/crates/socket-patch-cli/tests/common/mod.rs b/crates/socket-patch-cli/tests/common/mod.rs index 3eeb95327..05fabad6c 100644 --- a/crates/socket-patch-cli/tests/common/mod.rs +++ b/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). diff --git a/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs index c6cd5f28a..4bbba48f1 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_pnpm_build.rs +++ b/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 } diff --git a/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs b/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs index cf614c0f3..bd812e2f4 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_rush_sim.rs +++ b/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]); diff --git a/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs b/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs index 3884bf944..dfbf71740 100644 --- a/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs +++ b/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,20 @@ fn pnpm_required() -> bool { std::env::var_os("SOCKET_PATCH_PNPM_E2E_REQUIRED").is_some_and(|v| !v.is_empty()) } +/// pnpm is on `PATH`. 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. +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() +} + /// Print the SKIP line for a missing pnpm, or panic under [`pnpm_required`]. fn skip_missing_pnpm() { assert!( @@ -249,7 +263,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 +327,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 +431,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 +546,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; } From c8f514ebc919d2fa330e4ba7cd6a8f7d37dd2d83 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 01:16:08 +0000 Subject: [PATCH 3/4] Fail Windows and macOS pnpm legs on a skip Set pnpm_required on the Windows and macOS rows of e2e_safety_pnpm, e2e_redirect_pnpm_build and e2e_redirect_rush_sim. A missing pnpm or corepack now fails the leg instead of letting it pass with every test skipped. These rows only run in the full tier (nightly and dispatch), so PR and merge-queue runs are unchanged. The workflow contract test now checks the gate on every OS, and a new check stops these suites from spawning bare pnpm, corepack or npm again. Refs #1054 Assisted-by: Claude Code:claude-opus-5-5 --- .github/workflows/ci.yml | 11 +++++++---- scripts/tests/test_ci_e2e_tiers.py | 26 +++++++++++++++++++------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a099edc44..b876f7910 100644 --- a/.github/workflows/ci.yml +++ b/.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'} diff --git a/scripts/tests/test_ci_e2e_tiers.py b/scripts/tests/test_ci_e2e_tiers.py index 81886440b..1832e2025 100644 --- a/scripts/tests/test_ci_e2e_tiers.py +++ b/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,32 @@ 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)"\)') + self.assertNotIn('has_command("pnpm")', src) + def test_every_gated_suite_honours_the_gate(self): for suite in sorted(self.suites): with self.subTest(suite=suite): From c28558aa6036d3b1d87d1d1aa1fd2e39f2c48990 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 01:23:12 +0000 Subject: [PATCH 4/4] Tighten pnpm probe and shim regression guard has_pnpm now needs `pnpm --version` to succeed, not just to spawn: a resolved .cmd shim starts through cmd.exe even when the shim is broken. The shim guard now also covers `.cmd` spellings, a suite-local has_command, and the shared run_toolchain that pnpm_run uses. Refs #1054 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/tests/common/hermetic.rs | 5 ++--- crates/socket-patch-cli/tests/e2e_safety_pnpm.rs | 10 ++++++---- scripts/tests/test_ci_e2e_tiers.py | 14 ++++++++++++-- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/hermetic.rs b/crates/socket-patch-cli/tests/common/hermetic.rs index 165c1b289..7cab6e7c1 100644 --- a/crates/socket-patch-cli/tests/common/hermetic.rs +++ b/crates/socket-patch-cli/tests/common/hermetic.rs @@ -88,9 +88,8 @@ 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 the -/// way the CLI resolves its own tools (every `PATHEXT` extension on -/// Windows). npm and Node install `pnpm`, `corepack` and `npm` as `.cmd` +/// 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 diff --git a/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs b/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs index dfbf71740..2a6e48941 100644 --- a/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs +++ b/crates/socket-patch-cli/tests/e2e_safety_pnpm.rs @@ -45,9 +45,11 @@ fn pnpm_required() -> bool { std::env::var_os("SOCKET_PATCH_PNPM_E2E_REQUIRED").is_some_and(|v| !v.is_empty()) } -/// pnpm is on `PATH`. 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. +/// `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"); @@ -56,7 +58,7 @@ fn has_pnpm() -> bool { .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .status() - .is_ok() + .is_ok_and(|s| s.success()) } /// Print the SKIP line for a missing pnpm, or panic under [`pnpm_required`]. diff --git a/scripts/tests/test_ci_e2e_tiers.py b/scripts/tests/test_ci_e2e_tiers.py index 1832e2025..6ea29bad9 100644 --- a/scripts/tests/test_ci_e2e_tiers.py +++ b/scripts/tests/test_ci_e2e_tiers.py @@ -306,8 +306,18 @@ def test_suites_resolve_windows_shims(self): 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)"\)') - self.assertNotIn('has_command("pnpm")', src) + 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):