From dfc53d1f50297f3aca6d7422bd3a9cf4eeaf3df3 Mon Sep 17 00:00:00 2001 From: "MK (fengmk2)" Date: Tue, 13 Oct 2026 01:40:20 +0800 Subject: [PATCH] fix(install): persist Windows PATH independently of shell setup --- Cargo.lock | 1 + .../snapshots.toml | 6 + .../command_env_doctor_missing_fallback.md | 10 + .../verify.mjs | 20 ++ crates/vp_global_cli/Cargo.toml | 3 + .../vp_global_cli/src/commands/env/doctor.rs | 4 + crates/vp_global_cli/src/self_setup.rs | 39 +++- crates/vp_global_cli/src/self_setup/shell.rs | 22 -- .../src/self_setup/windows_path.rs | 203 ++++++++++++++++++ .../src/self_setup/windows_tests.rs | 132 ++++++++++++ 10 files changed, 408 insertions(+), 32 deletions(-) create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots.toml create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots/command_env_doctor_missing_fallback.md create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/verify.mjs create mode 100644 crates/vp_global_cli/src/self_setup/windows_path.rs create mode 100644 crates/vp_global_cli/src/self_setup/windows_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 090f1e993c..d4e7445fed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8374,6 +8374,7 @@ dependencies = [ "vt_pty_terminal_test_client", "vt_str", "vt_workspace", + "winreg 0.56.0", ] [[package]] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots.toml new file mode 100644 index 0000000000..084d9f2315 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots.toml @@ -0,0 +1,6 @@ +[[case]] +name = "command_env_doctor_missing_fallback" +vp = "global" +steps = [ + { argv = ["node", "verify.mjs"], comment = "Doctor names the missing fallback directory even when the main shim directory is on PATH." }, +] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots/command_env_doctor_missing_fallback.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots/command_env_doctor_missing_fallback.md new file mode 100644 index 0000000000..7888c01f73 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/snapshots/command_env_doctor_missing_fallback.md @@ -0,0 +1,10 @@ +# command_env_doctor_missing_fallback + +## `node verify.mjs` + +Doctor names the missing fallback directory even when the main shim directory is on PATH. + +``` + ✗ Fallback dir not in PATH + note: Expected: /.vite-plus/fallback-bin +``` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/verify.mjs b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/verify.mjs new file mode 100644 index 0000000000..428601e3a4 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/command_env_doctor_missing_fallback/verify.mjs @@ -0,0 +1,20 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import path from "node:path"; + +const bin = path.join(process.env.VP_HOME, "bin"); +const result = spawnSync( + path.join(bin, process.platform === "win32" ? "vp.exe" : "vp"), + ["env", "doctor", "node"], + { + encoding: "utf8", + env: { ...process.env, PATH: bin }, + }, +); +assert.equal(result.status, 1, `${result.stdout}\n${result.stderr}`); +const lines = result.stdout.replace(/\u001B\[[0-9;]*m/g, "").split(/\r?\n/); +const index = lines.findIndex((line) => line.includes("Fallback dir")); +assert.ok(index >= 0, result.stdout); +assert.match(lines[index], /not in PATH/); +assert.match(lines[index + 1], /note: Expected: .*[/\\]fallback-bin$/); +console.log(lines.slice(index, index + 2).join("\n")); diff --git a/crates/vp_global_cli/Cargo.toml b/crates/vp_global_cli/Cargo.toml index 1ea644f9f8..e67d7e5310 100644 --- a/crates/vp_global_cli/Cargo.toml +++ b/crates/vp_global_cli/Cargo.toml @@ -50,6 +50,9 @@ vt_str = { workspace = true } vt_workspace = { workspace = true } uuid = { workspace = true, features = ["v4"] } +[target.'cfg(windows)'.dependencies] +winreg = { workspace = true } + [dev-dependencies] serial_test = { workspace = true } temp-env = { workspace = true } diff --git a/crates/vp_global_cli/src/commands/env/doctor.rs b/crates/vp_global_cli/src/commands/env/doctor.rs index e19d10f40a..041290fdf7 100644 --- a/crates/vp_global_cli/src/commands/env/doctor.rs +++ b/crates/vp_global_cli/src/commands/env/doctor.rs @@ -560,6 +560,10 @@ async fn check_path(scope: EnvScope) -> bool { let fallback = vp_shared::EnvConfig::get().dirs.fallback_bin(); if !paths.iter().any(|path| path == fallback.as_path()) { print_check(&style(output::CROSS).red().to_string(), "Fallback dir", "not in PATH"); + print_hint(&format!( + "Expected: {}", + abbreviate_home(&fallback.as_path().display().to_string()) + )); print_path_fix(&vp_shared::EnvConfig::get().dirs.config); return false; } diff --git a/crates/vp_global_cli/src/self_setup.rs b/crates/vp_global_cli/src/self_setup.rs index c73293189c..378e4ee09c 100644 --- a/crates/vp_global_cli/src/self_setup.rs +++ b/crates/vp_global_cli/src/self_setup.rs @@ -2,6 +2,10 @@ mod external; mod shell; +#[cfg(windows)] +mod windows_path; +#[cfg(all(test, windows))] +mod windows_tests; use std::{path::Path, process::ExitCode}; @@ -274,14 +278,7 @@ async fn run(source: &Path, bundled: bool) -> Result { if deploy { remove_previous_install(previous_install.as_deref()).await?; } - if std::env::var(env_vars::VP_SELF_SETUP_NO_MODIFY_PATH).as_deref() != Ok("1") { - if let Err(error) = shell::configure().await { - output::warn(&format!( - "Could not configure shell profiles: {error}. Add {} to PATH manually.", - dirs.bin.as_path().display() - )); - } - } + configure_shells().await?; let mut settings = config::load_config().await?; if let Some(mode) = node_override.or(default_mode) { settings.node_shim_mode = mode; @@ -302,8 +299,8 @@ async fn run(source: &Path, bundled: bool) -> Result { // Existing Windows installs also need the fallback directory in their persistent PATH. #[cfg(windows)] - if in_place && std::env::var(env_vars::VP_SELF_SETUP_NO_MODIFY_PATH).as_deref() != Ok("1") { - shell::configure().await?; + if in_place { + configure_shells().await?; } // 2. Activate a standalone download; an upgrade hook must not overwrite rollback history. @@ -345,6 +342,28 @@ async fn run(source: &Path, bundled: bool) -> Result { Ok(binary) } +async fn configure_shells() -> Result<(), Error> { + if std::env::var(env_vars::VP_SELF_SETUP_NO_MODIFY_PATH).as_deref() == Ok("1") { + return Ok(()); + } + let dirs = &EnvConfig::get().dirs; + // Windows PATH is required even when an optional shell (such as an older + // Nushell) cannot be configured. Do not mark setup complete if this fails. + #[cfg(windows)] + windows_path::configure(&dirs.bin, &dirs.fallback_bin()) + .map_err(|error| Error::Other(format!("Could not configure user PATH: {error}").into()))?; + if let Err(error) = shell::configure().await { + #[cfg(windows)] + output::warn(&format!("Could not configure shell profiles: {error}.")); + #[cfg(not(windows))] + output::warn(&format!( + "Could not configure shell profiles: {error}. Add {} to PATH manually.", + dirs.bin.as_path().display() + )); + } + Ok(()) +} + fn interactive() -> bool { std::env::var_os("CI").is_none() && vp_shared::is_stderr_terminal() } diff --git a/crates/vp_global_cli/src/self_setup/shell.rs b/crates/vp_global_cli/src/self_setup/shell.rs index 23db53d582..821530dd82 100644 --- a/crates/vp_global_cli/src/self_setup/shell.rs +++ b/crates/vp_global_cli/src/self_setup/shell.rs @@ -90,27 +90,5 @@ pub(super) async fn configure() -> Result<(), Error> { } } } - #[cfg(windows)] - { - let bin = setup::escape_powershell_single_quoted_string(&config.dirs.bin.to_string()); - let fallback = - setup::escape_powershell_single_quoted_string(&config.dirs.fallback_bin().to_string()); - let script = format!( - "$bin = '{bin}'; $fallback = '{fallback}'; $path = @([Environment]::GetEnvironmentVariable('Path', 'User') -split ';' | Where-Object {{ $_ -and $_ -ne $bin -and $_ -ne $fallback }}); [Environment]::SetEnvironmentVariable('Path', ((@($bin) + $path + @($fallback)) -join ';'), 'User')" - ); - let result = tokio::process::Command::new("powershell") - .args(["-NoProfile", "-NonInteractive", "-Command", &script]) - .output() - .await?; - if !result.status.success() { - return Err(Error::Other( - format!( - "Could not configure user PATH: {}", - String::from_utf8_lossy(&result.stderr) - ) - .into(), - )); - } - } Ok(()) } diff --git a/crates/vp_global_cli/src/self_setup/windows_path.rs b/crates/vp_global_cli/src/self_setup/windows_path.rs new file mode 100644 index 0000000000..7cc0c5b666 --- /dev/null +++ b/crates/vp_global_cli/src/self_setup/windows_path.rs @@ -0,0 +1,203 @@ +//! Persist Windows PATH without depending on an installed shell. + +use std::io; + +use vt_path::AbsolutePath; +use winreg::{ + RegKey, RegValue, + enums::{HKEY_CURRENT_USER, KEY_READ, KEY_WRITE, REG_EXPAND_SZ, REG_SZ}, +}; + +pub(super) fn configure(bin: &AbsolutePath, fallback: &AbsolutePath) -> io::Result<()> { + let hkcu = RegKey::predef(HKEY_CURRENT_USER); + let (environment, _) = hkcu.create_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)?; + if update_path(&environment, &bin.to_string(), &fallback.to_string())? { + broadcast_settings_change(); + } + Ok(()) +} + +// Accept an open key so tests never need to change the real user environment. +fn update_path(environment: &RegKey, bin: &str, fallback: &str) -> io::Result { + let original = match environment.get_raw_value("Path") { + Ok(value) => Some(value), + Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Err(error) => return Err(error), + }; + let (current, kind) = match &original { + Some(value) => (decode_path(value)?, value.vtype.clone()), + None => (String::new(), REG_EXPAND_SZ), + }; + let matches = |entry: &str, directory: &str| { + entry + .trim_end_matches(['\\', '/']) + .eq_ignore_ascii_case(directory.trim_end_matches(['\\', '/'])) + }; + let mut entries = vec![bin]; + if !current.is_empty() { + entries.extend( + current.split(';').filter(|entry| !matches(entry, bin) && !matches(entry, fallback)), + ); + } + entries.push(fallback); + let updated = RegValue { + vtype: kind, + bytes: entries.join(";").encode_utf16().chain([0]).flat_map(u16::to_le_bytes).collect(), + }; + if original.as_ref() == Some(&updated) { + return Ok(false); + } + environment.set_raw_value("Path", &updated)?; + Ok(true) +} + +fn decode_path(value: &RegValue) -> io::Result { + if !matches!(value.vtype, REG_SZ | REG_EXPAND_SZ) || !value.bytes.len().is_multiple_of(2) { + return Err(io::Error::new(io::ErrorKind::InvalidData, "Invalid user PATH registry value")); + } + let mut units: Vec<_> = + value.bytes.as_chunks::<2>().0.iter().copied().map(u16::from_le_bytes).collect(); + while units.last() == Some(&0) { + units.pop(); + } + if units.contains(&0) { + return Err(io::Error::new(io::ErrorKind::InvalidData, "User PATH contains a NUL")); + } + String::from_utf16(&units).map_err(|error| io::Error::new(io::ErrorKind::InvalidData, error)) +} + +/// Notify Explorer and other applications that future processes need the new PATH. +fn broadcast_settings_change() { + #[link(name = "user32")] + unsafe extern "system" { + fn SendMessageTimeoutW( + window: isize, + message: u32, + wparam: usize, + lparam: isize, + flags: u32, + timeout: u32, + result: *mut usize, + ) -> isize; + } + let environment: Vec = "Environment".encode_utf16().chain([0]).collect(); + // HWND_BROADCAST, WM_SETTINGCHANGE, SMTO_ABORTIFHUNG. + // The string remains live until this synchronous call returns. + unsafe { + SendMessageTimeoutW( + 0xffff, + 0x001a, + 0, + environment.as_ptr() as isize, + 0x0002, + 5000, + std::ptr::null_mut(), + ); + } +} + +#[cfg(test)] +mod tests { + use winreg::enums::{REG_BINARY, REG_MULTI_SZ}; + + use super::*; + + struct TestKey { + key: RegKey, + path: String, + } + + impl TestKey { + fn new() -> Self { + let path = format!("Software\\VitePlusTests\\{}", uuid::Uuid::new_v4()); + let (key, _) = RegKey::predef(HKEY_CURRENT_USER).create_subkey(&path).unwrap(); + Self { key, path } + } + } + + impl Drop for TestKey { + fn drop(&mut self) { + RegKey::predef(HKEY_CURRENT_USER).delete_subkey_all(&self.path).unwrap(); + } + } + + fn value(path: &str, kind: winreg::enums::RegType) -> RegValue<'static> { + RegValue { + vtype: kind, + bytes: path.encode_utf16().chain([0]).flat_map(u16::to_le_bytes).collect(), + } + } + + #[test] + fn preserves_variables_empty_entries_and_registry_type() { + let key = TestKey::new(); + for kind in [REG_EXPAND_SZ, REG_SZ] { + let before = "%PNPM_HOME%;;C:\\工具\\bin"; + key.key.set_raw_value("Path", &value(before, kind.clone())).unwrap(); + assert!(update_path(&key.key, r"C:\Vite+\bin", r"C:\Vite+\fallback-bin").unwrap()); + assert_eq!( + key.key.get_raw_value("Path").unwrap(), + value(&format!("C:\\Vite+\\bin;{before};C:\\Vite+\\fallback-bin"), kind) + ); + assert!(!update_path(&key.key, r"C:\Vite+\bin", r"C:\Vite+\fallback-bin").unwrap()); + } + } + + #[test] + fn creates_missing_path_and_handles_empty_path() { + let key = TestKey::new(); + for initial in [None, Some(value("", REG_EXPAND_SZ))] { + if let Some(initial) = initial { + key.key.set_raw_value("Path", &initial).unwrap(); + } + update_path(&key.key, r"C:\bin", r"C:\fallback").unwrap(); + assert_eq!( + key.key.get_raw_value("Path").unwrap(), + value(r"C:\bin;C:\fallback", REG_EXPAND_SZ) + ); + } + } + + #[test] + fn moves_existing_entries_to_opposite_ends_without_duplicates() { + let key = TestKey::new(); + key.key.set_value("Path", &r"C:\tools;c:\BIN\;C:\fallback;C:\bin;C:\other").unwrap(); + update_path(&key.key, r"C:\bin", r"C:\fallback").unwrap(); + assert_eq!( + key.key.get_value::("Path").unwrap(), + r"C:\bin;C:\tools;C:\other;C:\fallback" + ); + } + + #[test] + fn refuses_invalid_values_without_overwriting_them() { + let key = TestKey::new(); + for original in [ + RegValue { vtype: REG_BINARY, bytes: vec![1, 2].into() }, + value("first\0second", REG_MULTI_SZ), + RegValue { vtype: REG_SZ, bytes: vec![1].into() }, + RegValue { vtype: REG_SZ, bytes: vec![0, 0xd8, 0, 0].into() }, + value("first\0second", REG_SZ), + ] { + key.key.set_raw_value("Path", &original).unwrap(); + assert_eq!( + update_path(&key.key, r"C:\bin", r"C:\fallback").unwrap_err().kind(), + io::ErrorKind::InvalidData + ); + assert_eq!(key.key.get_raw_value("Path").unwrap(), original); + } + } + + #[test] + fn propagates_write_errors() { + let key = TestKey::new(); + key.key.set_value("Path", &r"C:\tools").unwrap(); + let readonly = + RegKey::predef(HKEY_CURRENT_USER).open_subkey_with_flags(&key.path, KEY_READ).unwrap(); + assert_eq!( + update_path(&readonly, r"C:\bin", r"C:\fallback").unwrap_err().kind(), + io::ErrorKind::PermissionDenied + ); + assert_eq!(key.key.get_value::("Path").unwrap(), r"C:\tools"); + } +} diff --git a/crates/vp_global_cli/src/self_setup/windows_tests.rs b/crates/vp_global_cli/src/self_setup/windows_tests.rs new file mode 100644 index 0000000000..2439d4c601 --- /dev/null +++ b/crates/vp_global_cli/src/self_setup/windows_tests.rs @@ -0,0 +1,132 @@ +//! Run registry redirection in a dedicated test process, never alongside other tests. + +use std::process::Command; + +use vp_shared::{EnvConfig, env_vars}; +use winreg::{RegKey, enums::HKEY_CURRENT_USER}; + +struct RegistryOverride { + root: RegKey, + path: String, +} + +#[link(name = "advapi32")] +unsafe extern "system" { + fn RegOverridePredefKey(key: isize, replacement: isize) -> i32; +} + +impl RegistryOverride { + fn new() -> Self { + let path = format!("Software\\VitePlusTests\\{}", uuid::Uuid::new_v4()); + let (root, _) = RegKey::predef(HKEY_CURRENT_USER).create_subkey(&path).unwrap(); + // This process runs only the ignored helper test below. Keep the root + // handle alive and restore HKCU before removing the temporary subtree. + assert_eq!( + unsafe { RegOverridePredefKey(HKEY_CURRENT_USER as isize, root.raw_handle() as isize) }, + 0 + ); + Self { root, path } + } +} + +impl Drop for RegistryOverride { + fn drop(&mut self) { + assert_eq!(unsafe { RegOverridePredefKey(HKEY_CURRENT_USER as isize, 0) }, 0); + RegKey::predef(HKEY_CURRENT_USER).delete_subkey_all(&self.path).unwrap(); + } +} + +#[test] +fn windows_self_setup_handles_path_failures_and_optional_shells() { + let output = Command::new(std::env::current_exe().unwrap()) + .args(["--ignored", "--exact", "self_setup::windows_tests::isolated_setup", "--nocapture"]) + .env("VP_TEST_ISOLATED_REGISTRY", "1") + .output() + .unwrap(); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[tokio::test] +#[ignore = "invoked in a dedicated process by windows_self_setup_handles_path_failures_and_optional_shells"] +async fn isolated_setup() { + assert_eq!(std::env::var("VP_TEST_ISOLATED_REGISTRY").as_deref(), Ok("1")); + let registry = RegistryOverride::new(); + let (environment, _) = registry.root.create_subkey("Environment").unwrap(); + let temporary = tempfile::tempdir().unwrap(); + let source_dir = temporary.path().join("source"); + std::fs::create_dir(&source_dir).unwrap(); + let source = source_dir.join("vp.exe"); + // run() only copies these payloads and creates shims; it never executes them. + std::fs::write(&source, b"vp test payload").unwrap(); + std::fs::write(source_dir.join("vp-shim.exe"), b"test trampoline").unwrap(); + let nu_dir = temporary.path().join("old-nu"); + std::fs::create_dir(&nu_dir).unwrap(); + // The test harness rejects "-c", like Nushell versions without the queried field. + std::fs::copy(std::env::current_exe().unwrap(), nu_dir.join("nu.exe")).unwrap(); + + for (case, path, skip) in [ + ("no-powershell", "", None), + ("broken-nushell", nu_dir.to_str().unwrap(), None), + ("path-error", "", None), + ("opt-out", "", Some("1")), + ] { + let home = temporary.path().join(case); + let home_str = home.to_str().unwrap(); + let environment = &environment; + let source = &source; + Box::pin(EnvConfig::with_vars_async( + [ + (env_vars::VP_HOME, Some(home_str)), + ("PATH", Some(path)), + ("VP_SKIP_DEPS_INSTALL", Some("1")), + ("VP_VERSION", Some("1.1.0")), + ("VP_NODE_MANAGER", Some("no")), + ("VP_PM_MANAGER", Some("no")), + (env_vars::VP_SELF_SETUP_NO_MODIFY_PATH, skip), + ("CI", Some("true")), + ], + |config| async move { + let before = "%PNPM_HOME%;;C:\\tools"; + environment.set_value("Path", &before).unwrap(); + let marker = config.dirs.data.join("1.1.0/bin").join(vp_setup::SELF_SETUP_MARKER); + if case == "path-error" { + environment.set_value("Path", &42_u32).unwrap(); + let error = super::run(source, false).await.unwrap_err(); + assert!(error.to_string().contains("Could not configure user PATH"), "{error}"); + assert!(!marker.as_path().is_file(), "failed PATH setup must remain retryable"); + assert_eq!(environment.get_value::("Path").unwrap(), 42); + // Retrying after the registry value is repaired finishes setup. + environment.set_value("Path", &before).unwrap(); + } + let binary = super::run(source, false).await.unwrap(); + assert!(marker.as_path().is_file()); + let persisted: String = environment.get_value("Path").unwrap(); + if skip.is_some() { + assert_eq!(persisted, before); + } else { + assert_eq!( + persisted, + format!( + "{};{before};{}", + config.dirs.bin.as_path().display(), + config.dirs.fallback_bin().as_path().display() + ) + ); + } + // An upgrade invokes the binary already in current/bin after + // clearing its marker. It must configure PATH with the same rules. + std::fs::remove_file(marker.as_path()).unwrap(); + environment.set_value("Path", &before).unwrap(); + super::run(binary.as_path(), false).await.unwrap(); + assert!(marker.as_path().is_file()); + assert_eq!(environment.get_value::("Path").unwrap(), persisted); + }, + )) + .await; + } +}