diff --git a/crates/nub-cli/src/cli.rs b/crates/nub-cli/src/cli.rs index 7b941f13e..1fd5030a5 100644 --- a/crates/nub-cli/src/cli.rs +++ b/crates/nub-cli/src/cli.rs @@ -10339,6 +10339,21 @@ fn run_pm_shim_install() -> Result { ProfileOutcome::AlreadyPresent(profile) => { println!(" PATH: already present in {}", profile.display()) } + // Reached on upgrade. The directory is a compile-time constant within + // one build, but the CONSTANT ITSELF moved under XDG in #752, so a + // profile written by an older nub still names `$HOME/.nub/shims` beneath + // this marker and the line is rewritten in place. Only when that legacy + // directory is already GONE — the migration above strips the block + // outright while it still exists — so the live shell carries a + // directory on PATH that holds no shims, which is why the re-source + // hint matters more here than on a fresh add. + ProfileOutcome::Rewritten(profile) => println!( + " PATH: updated the entry in {} to point at {}\n \ + restart your shell, or run: source {}", + profile.display(), + dir.display(), + profile.display() + ), // No writable profile for this shell: print the line and exit 0 (the // spec's manual fallback — the shims themselves are installed). ProfileOutcome::Manual { line } => println!( @@ -10486,6 +10501,21 @@ fn run_node_shim_install() -> Result { ProfileOutcome::AlreadyPresent(profile) => { println!(" PATH: already present in {}", profile.display()) } + // Reached on upgrade. The directory is a compile-time constant within + // one build, but the CONSTANT ITSELF moved under XDG in #752, so a + // profile written by an older nub still names `$HOME/.nub/node-shim` beneath + // this marker and the line is rewritten in place. Only when that legacy + // directory is already GONE — the migration above strips the block + // outright while it still exists — so the live shell carries a + // directory on PATH that holds no shims, which is why the re-source + // hint matters more here than on a fresh add. + ProfileOutcome::Rewritten(profile) => println!( + " PATH: updated the entry in {} to point at {}\n \ + restart your shell, or run: source {}", + profile.display(), + dir.display(), + profile.display() + ), ProfileOutcome::Manual { line } => println!( " PATH: no known shell profile to edit — add this line to your shell config:\n {line}" ), diff --git a/crates/nub-cli/src/pm_engine/info_family.rs b/crates/nub-cli/src/pm_engine/info_family.rs index 8efea8995..60c6d79be 100644 --- a/crates/nub-cli/src/pm_engine/info_family.rs +++ b/crates/nub-cli/src/pm_engine/info_family.rs @@ -10,9 +10,11 @@ //! `whoami`/`owner`). //! **Still a stub** (deliberately): `sbom` (below). //! -//! `bin -g` / `root -g` print the engine's global-install layout (the -//! `PNPM_HOME`-compatible home, packages under its `global-aube/` subdir) — -//! real on-disk paths where the already-wired `add -g` installs, preserved +//! `bin -g` / `root -g` print the engine's global-install layout, and the two +//! now resolve from different roots: `bin -g` gives the SHARED user-binary +//! directory already on PATH (`~/.local/bin` and its `XDG_BIN_HOME` +//! relatives), while the installs themselves live under `//global`. +//! Real on-disk paths where the already-wired `add -g` installs, preserved //! by the rewrite policy like the global-links residual in the install //! family. //! diff --git a/crates/nub-cli/src/pm_engine/install_family.rs b/crates/nub-cli/src/pm_engine/install_family.rs index a81c2d36d..c8674fb95 100644 --- a/crates/nub-cli/src/pm_engine/install_family.rs +++ b/crates/nub-cli/src/pm_engine/install_family.rs @@ -401,6 +401,85 @@ fn finish_code(result: miette::Result>) -> Result { // ───────────────────────── wired verbs ────────────────────────── +/// Put the global bin directory on PATH after a successful global install. +/// +/// Installing a package globally has exactly one purpose — running it by name — +/// so an install that leaves the directory unreachable has not done what was +/// asked. The engine has already warned and named the line; this wires it. +/// +/// Deliberately narrow. It runs ONLY on a successful global install, only when +/// the directory is genuinely absent from PATH (after the move to the shared +/// user-binary directory that is already true for few systems), and it writes +/// through the marker-keyed path so a profile can never collect a second copy +/// of the block. A failure here is reported and swallowed: the packages are +/// installed either way, and an unwritable profile must not fail the install. +fn wire_global_bin_path(code: i32) { + if code != 0 { + return; + } + let Ok(layout) = aube::commands::global::GlobalLayout::resolve() else { + return; + }; + if aube::commands::global::dir_is_on_path(&layout.bin_dir) { + return; + } + // Windows profile/registry editing is out of scope for v0 — the same call + // `nub pm shim` makes (cli.rs, `run_pm_shim_install`). Without this, the + // shell probe below finds no `SHELL`, falls back to the POSIX dialect, and + // writes `export PATH=…` into a `.profile` that neither cmd.exe nor + // PowerShell reads — then reports PATH configured. Printing the line is + // honest; the silent POSIX write is worse than either doing it properly or + // not doing it at all. + if cfg!(windows) { + eprintln!( + " PATH: add {} to your PATH (PATH editing isn't automated on Windows yet)", + layout.bin_dir.display() + ); + return; + } + match nub_core::pm::shim::add_global_bin_path_block(&layout.bin_dir) { + Ok(nub_core::pm::shim::ProfileOutcome::Added(p)) => { + eprintln!( + " PATH: added {} to {}", + layout.bin_dir.display(), + p.display() + ); + eprintln!(" Restart your shell, or source that file, to pick it up."); + } + Ok(nub_core::pm::shim::ProfileOutcome::Rewritten(p)) => { + eprintln!( + " PATH: updated the nub global bin entry in {} to {} — restart \ + your shell, or source that file, to pick it up.", + p.display(), + layout.bin_dir.display() + ); + } + // The line is in a profile this shell reads, but the CURRENT shell has + // not sourced it — otherwise the on-PATH check above would have + // returned. Say what to do about the session in hand. + Ok(nub_core::pm::shim::ProfileOutcome::AlreadyPresent(p)) => { + eprintln!( + " PATH: {} is already configured in {} — restart your shell, \ + or source that file, to pick it up.", + layout.bin_dir.display(), + p.display() + ); + } + // No profile this shell reads could be written. This is the ONLY path + // that asks the user to do it by hand, which is why the engine no + // longer prints a remediation of its own. + Ok(nub_core::pm::shim::ProfileOutcome::Manual { line }) => { + eprintln!( + "warning: {} is not on PATH, so the commands just installed will \ + not run.\n No shell profile could be written — add this line \ + yourself:\n {line}", + layout.bin_dir.display() + ); + } + Err(e) => eprintln!("warning: could not update your shell profile: {e}"), + } +} + fn run_add(typed: &str, args: &[String]) -> Result { let (globals, verb): (_, aube::commands::add::AddArgs) = parse_or_return!(typed, args); let session = super::engine_session(globals.dir.as_deref())?; @@ -411,6 +490,8 @@ fn run_add(typed: &str, args: &[String]) -> Result { &yarn_remedy("add", &verb.packages), )); } + // Read before the move: `run` consumes `verb`. + let is_global = verb.global; super::min_release_age::arm(); let code = finish_quieted( &globals.output, @@ -418,6 +499,9 @@ fn run_add(typed: &str, args: &[String]) -> Result { aube::commands::add::run(verb, globals.effective_filter()), )?; super::min_release_age::persist(&session.cwd, code == 0, &globals.output); + if is_global { + wire_global_bin_path(code); + } stamp_if_virgin(&session, code); crate::install_engine::record(&session.cwd, code); // `nub add vite` (or adding any dep to a vite project) changes the graph; diff --git a/crates/nub-core/src/node/shim.rs b/crates/nub-core/src/node/shim.rs index cc12da99c..d51730c56 100644 --- a/crates/nub-core/src/node/shim.rs +++ b/crates/nub-core/src/node/shim.rs @@ -38,17 +38,19 @@ use crate::pm::shim::{ /// from the PM shims' (`~/.nub/shims`, `# nub shims`) so the two install and /// uninstall independently. const NODE_SHIM_BLOCK: ShimBlock = ShimBlock { - marker: NODE_SHIM_MARKER, + marker: std::borrow::Cow::Borrowed(NODE_SHIM_MARKER), // One descriptor, matching resolve_shim_dir's single rule. The default is // spelled inline so a profile sourced without XDG_DATA_HOME still names a // real dir, and `test -n` rather than fish's `set -q` because `set -q` is // true for a DEFINED-but-empty variable (which would yield `/nub/node-shim`). - posix_line: r#"export PATH="${XDG_DATA_HOME:-$HOME/.local/share}/nub/node-shim:$PATH""#, - fish_line: concat!( + posix_line: std::borrow::Cow::Borrowed( + r#"export PATH="${XDG_DATA_HOME:-$HOME/.local/share}/nub/node-shim:$PATH""#, + ), + fish_line: std::borrow::Cow::Borrowed(concat!( "set -gx PATH (test -n \"$XDG_DATA_HOME\"; and echo $XDG_DATA_HOME; ", "or echo $HOME/.local/share)/nub/node-shim $PATH" - ), - dir_marker: NODE_SHIM_DIR_MARKER, + )), + dir_marker: std::borrow::Cow::Borrowed(NODE_SHIM_DIR_MARKER), }; const NODE_SHIM_MARKER: &str = "# nub node shim"; diff --git a/crates/nub-core/src/pm/shim.rs b/crates/nub-core/src/pm/shim.rs index 7626ea354..9a72e7fea 100644 --- a/crates/nub-core/src/pm/shim.rs +++ b/crates/nub-core/src/pm/shim.rs @@ -16,6 +16,7 @@ //! under `$XDG_CACHE_HOME/nub`: a shim is an installation the user opted into, //! and wiping a cache must never silently remove entries their PATH points at. +use std::borrow::Cow; use std::ffi::OsStr; use std::fmt; use std::io::Write as _; @@ -900,13 +901,18 @@ const BLOCK_MARKER: &str = "# nub shims"; /// persistent `node` shim (`crate::node::shim`) without duplicating it. Each /// family carries a DISTINCT marker so an unshim strips exactly its own block — /// never a sibling's, never install.sh's `# nub` block. +/// +/// Fields are [`Cow`] because not every family's directory is known at compile +/// time: the global-bin block's path is RESOLVED AT RUNTIME (it follows +/// `XDG_BIN_HOME` and friends), so its lines cannot be `&'static str` the way +/// the two fixed `~/.nub/…` families' can. pub(crate) struct ShimBlock { /// The marker comment written on its own line above the PATH line. - pub(crate) marker: &'static str, + pub(crate) marker: Cow<'static, str>, /// The POSIX (bash/zsh) `export PATH="…:$PATH"` line. - pub(crate) posix_line: &'static str, + pub(crate) posix_line: Cow<'static, str>, /// The fish `set -gx PATH … $PATH` line. - pub(crate) fish_line: &'static str, + pub(crate) fish_line: Cow<'static, str>, /// A substring the PATH line must contain for [`strip_block`]'s defensive /// "is this really our line" guard. /// @@ -916,7 +922,7 @@ pub(crate) struct ShimBlock { /// leave the PATH line behind pointing at a directory it had just deleted. /// The guard is only a defensive check on a line already identified by its /// own `marker` comment, so matching one character less costs nothing. - pub(crate) dir_marker: &'static str, + pub(crate) dir_marker: Cow<'static, str>, } /// The PM shims' block (`# nub shims`). One descriptor: the resolution rule has @@ -925,12 +931,68 @@ pub(crate) struct ShimBlock { /// (`$HOME/.nub/shims`, which contains it) and an unshim can clean up an install /// that predates the move. pub(crate) const PM_SHIM_BLOCK: ShimBlock = ShimBlock { - marker: BLOCK_MARKER, - posix_line: SHIMS_POSIX_PATH_LINE, - fish_line: SHIMS_FISH_PATH_LINE, - dir_marker: "nub/shims", + marker: Cow::Borrowed(BLOCK_MARKER), + posix_line: Cow::Borrowed(SHIMS_POSIX_PATH_LINE), + fish_line: Cow::Borrowed(SHIMS_FISH_PATH_LINE), + dir_marker: Cow::Borrowed("nub/shims"), }; +/// Build the global-bin family's block for a directory resolved at runtime. +/// +/// Unlike the two fixed families this one cannot be a `const`: the directory +/// follows `XDG_BIN_HOME` and the user's settings, so it is only known once the +/// engine has resolved it. The marker is distinct from both `# nub shims` and +/// install.sh's `# nub`, so each family adds and strips exactly its own block. +/// +/// The emitted line stays `$HOME`-relative when the directory is under the home +/// directory, matching install.sh, so a profile synced between machines keeps +/// working. Both dialects quote the path so a directory containing a space +/// survives fish's word splitting. +pub fn global_bin_block(dir: &Path, home: &Path) -> ShimBlockSpec { + let shown = match dir.strip_prefix(home) { + Ok(rel) => format!("$HOME/{}", rel.display()), + Err(_) => dir.display().to_string(), + }; + ShimBlockSpec { + inner: ShimBlock { + marker: Cow::Borrowed(GLOBAL_BIN_MARKER), + posix_line: Cow::Owned(format!(r#"export PATH="{shown}:$PATH""#)), + fish_line: Cow::Owned(format!(r#"set -gx PATH "{shown}" $PATH"#)), + dir_marker: Cow::Owned(shown), + }, + } +} + +/// The global-bin family's marker. Distinct from `# nub shims` and from +/// install.sh's `# nub` so the three never strip or rewrite each other. +const GLOBAL_BIN_MARKER: &str = "# nub global bin"; + +/// Opaque wrapper so [`ShimBlock`] stays crate-private while callers outside +/// this module can still name a block they built. +pub struct ShimBlockSpec { + inner: ShimBlock, +} + +/// Wire `dir` into every profile the current shell reads, idempotently. +/// +/// Adds the block when absent, REWRITES the line when our marker is there with +/// a different directory beneath it, and does nothing when it already matches. +/// A profile is never given a second copy of this block. +pub fn add_global_bin_path_block(dir: &Path) -> Result { + let home = dirs_next::home_dir().context("cannot locate the home directory")?; + let shell = std::env::var("SHELL").unwrap_or_default(); + let shell = Path::new(&shell) + .file_name() + .map(|s| s.to_string_lossy().into_owned()) + .filter(|s| !s.is_empty()) + .unwrap_or_else(|| "bash".to_string()); + let xdg = std::env::var_os("XDG_CONFIG_HOME") + .filter(|v| !v.is_empty()) + .map(PathBuf::from); + let spec = global_bin_block(dir, &home); + add_path_block_for(&shell, &home, xdg.as_deref(), &spec.inner) +} + /// Outcome of [`add_path_block`], for the CLI's "what changed" report. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ProfileOutcome { @@ -938,9 +1000,15 @@ pub enum ProfileOutcome { Added(PathBuf), /// The profile already carries the PATH line — adding twice is a no-op. AlreadyPresent(PathBuf), + /// The profile carried our marker with a DIFFERENT line beneath it, and the + /// line was rewritten in place; appending instead is what would accumulate a + /// stale block per relocation. Reached both by a family whose directory is + /// resolved at runtime and by a FIXED one across an upgrade that moved its + /// constant — #752 moving the shims under `XDG_DATA_HOME` is the latter. + Rewritten(PathBuf), /// No known profile exists / is writable for this shell — the CLI prints /// `line` as "add this to your shell config yourself" and exits 0. - Manual { line: &'static str }, + Manual { line: String }, } /// Append the marked PATH block to ALL of the current shell's profile files — @@ -996,16 +1064,20 @@ pub(crate) fn add_path_block_for( let targets = shell_profiles(shell, home, xdg_config, block); if targets.is_empty() { return Ok(ProfileOutcome::Manual { - line: block.posix_line, + line: block.posix_line.to_string(), }); } let mut first_added: Option = None; + let mut first_rewritten: Option = None; let mut first_present: Option = None; for target in &targets { - match append_block(target, block.marker)? { + match append_block(target, &block.marker)? { ProfileOutcome::Added(p) => { first_added.get_or_insert(p); } + ProfileOutcome::Rewritten(p) => { + first_rewritten.get_or_insert(p); + } ProfileOutcome::AlreadyPresent(p) => { first_present.get_or_insert(p); } @@ -1015,11 +1087,12 @@ pub(crate) fn add_path_block_for( ProfileOutcome::Manual { .. } => {} } } - Ok(match (first_added, first_present) { - (Some(p), _) => ProfileOutcome::Added(p), - (None, Some(p)) => ProfileOutcome::AlreadyPresent(p), - (None, None) => ProfileOutcome::Manual { - line: block.posix_line, + Ok(match (first_added, first_rewritten, first_present) { + (Some(p), _, _) => ProfileOutcome::Added(p), + (None, Some(p), _) => ProfileOutcome::Rewritten(p), + (None, None, Some(p)) => ProfileOutcome::AlreadyPresent(p), + (None, None, None) => ProfileOutcome::Manual { + line: block.posix_line.to_string(), }, }) } @@ -1028,7 +1101,7 @@ pub(crate) fn add_path_block_for( /// missing file may be created. struct ProfileTarget { path: PathBuf, - line: &'static str, + line: String, may_create: bool, } @@ -1044,7 +1117,7 @@ fn shell_profiles( ) -> Vec { let posix = |path: PathBuf, may_create: bool| ProfileTarget { path, - line: block.posix_line, + line: block.posix_line.to_string(), may_create, }; match shell { @@ -1085,7 +1158,7 @@ fn shell_profiles( .unwrap_or_else(|| home.join(".config")); vec![ProfileTarget { path: base.join("fish").join("config.fish"), - line: block.fish_line, + line: block.fish_line.to_string(), may_create: true, }] } @@ -1098,6 +1171,36 @@ fn appendable(path: &Path) -> bool { std::fs::OpenOptions::new().append(true).open(path).is_ok() } +/// Replace a profile's whole contents without ever truncating it. +/// +/// `std::fs::write` is create + TRUNCATE + `write_all`, so a crash, SIGKILL or +/// ENOSPC between those steps leaves the user's `.zshrc` empty or holding a +/// partial prefix — and everything else in it goes too. This is not a file nub +/// owns, so write beside it and rename, which is atomic. +/// +/// The rename targets the CANONICALIZED path — a `~/.zshrc` that is a symlink +/// into a dotfiles repo must stay a symlink, with the edit landing in the +/// linked-to file; renaming onto the symlink path would replace the link with a +/// regular file and orphan the dotfiles copy. Permissions are copied over so a +/// 600 profile stays 600. +fn replace_profile_atomically(path: &Path, contents: &str, tag: &str) -> Result<()> { + let target = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); + let tmp = target.with_file_name(format!( + "{}.nub-{tag}-{}", + target.file_name().unwrap_or_default().to_string_lossy(), + std::process::id() + )); + std::fs::write(&tmp, contents).with_context(|| format!("writing {}", tmp.display()))?; + if let Ok(meta) = std::fs::metadata(&target) { + let _ = std::fs::set_permissions(&tmp, meta.permissions()); + } + if let Err(e) = std::fs::rename(&tmp, &target) { + let _ = std::fs::remove_file(&tmp); + return Err(e).with_context(|| format!("replacing {}", target.display())); + } + Ok(()) +} + /// Append `\n# nub shims\n\n` — byte-for-byte what install.sh's three /// `echo`s produce for its own block. Idempotency keys on the PATH line itself /// (trimmed line equality), so a hand-added identical line also counts as @@ -1107,18 +1210,32 @@ fn append_block(target: &ProfileTarget, marker: &str) -> Result Ok(s) => s, Err(e) if e.kind() == std::io::ErrorKind::NotFound => { if !target.may_create { - return Ok(ProfileOutcome::Manual { line: target.line }); + return Ok(ProfileOutcome::Manual { + line: target.line.clone(), + }); } String::new() } Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { - return Ok(ProfileOutcome::Manual { line: target.line }); + return Ok(ProfileOutcome::Manual { + line: target.line.clone(), + }); } Err(e) => return Err(e).with_context(|| format!("reading {}", target.path.display())), }; if existing.lines().any(|l| l.trim() == target.line) { return Ok(ProfileOutcome::AlreadyPresent(target.path.clone())); } + // Our marker is here but the line under it differs, so the directory moved + // between runs (a resolved-at-runtime family: `XDG_BIN_HOME` changed, the + // setting changed, a different HOME). Replace that line rather than append + // a second block — appending is what silently accumulates one stale PATH + // entry per relocation, and the user never sees it because a shell only + // reports the winning entry. + if let Some(rewritten) = rewrite_marked_line(&existing, marker, &target.line) { + replace_profile_atomically(&target.path, &rewritten, "shim")?; + return Ok(ProfileOutcome::Rewritten(target.path.clone())); + } if target.may_create { if let Some(parent) = target.path.parent() { std::fs::create_dir_all(parent) @@ -1132,7 +1249,9 @@ fn append_block(target: &ProfileTarget, marker: &str) -> Result { Ok(f) => f, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { - return Ok(ProfileOutcome::Manual { line: target.line }); + return Ok(ProfileOutcome::Manual { + line: target.line.clone(), + }); } Err(e) => return Err(e).with_context(|| format!("opening {}", target.path.display())), }; @@ -1141,6 +1260,34 @@ fn append_block(target: &ProfileTarget, marker: &str) -> Result Ok(ProfileOutcome::Added(target.path.clone())) } +/// Replace the line directly beneath `marker` with `line`, returning the new +/// file contents — or `None` when the marker is absent, or is present but is +/// the last line, or already carries `line`. +/// +/// Anchoring on the MARKER rather than on the line text is the whole point: a +/// runtime-resolved directory spells differently between runs (`$HOME`-relative +/// against absolute, a different `HOME` under `sudo`, a relocated +/// `XDG_BIN_HOME`), so line-equality misses and appends a duplicate. The marker +/// is the stable identity. +/// +/// Only the FIRST occurrence is rewritten; a file that somehow carries two of +/// our blocks keeps the second, which a later run then reports as already +/// present rather than silently deleting a line we may not have written. +fn rewrite_marked_line(existing: &str, marker: &str, line: &str) -> Option { + let mut lines: Vec<&str> = existing.lines().collect(); + let at = lines.iter().position(|l| l.trim() == marker)?; + let target = lines.get(at + 1)?; + if target.trim() == line { + return None; + } + lines[at + 1] = line; + let mut out = lines.join("\n"); + if existing.ends_with('\n') { + out.push('\n'); + } + Some(out) +} + /// Strip the marked block from EVERY profile [`add_path_block`] may have /// written — across all shells, both the interactive rc and the /// non-interactive/login files — not just the current `$SHELL`'s, since the user @@ -1188,26 +1335,7 @@ pub(crate) fn remove_path_block_from_profiles( let Some(stripped) = strip_block(&content, block) else { continue; }; - // Temp + rename: a torn write must never truncate a shell profile. - // The rename targets the CANONICALIZED path — a `~/.zshrc` that is a - // symlink into a dotfiles repo must stay a symlink, with the edit - // landing in the linked-to file; renaming onto the symlink path would - // replace the link with a regular file and orphan the dotfiles copy. - // Permissions are copied over so a 600 profile stays 600. - let target = path.canonicalize().unwrap_or_else(|_| path.clone()); - let tmp = target.with_file_name(format!( - "{}.nub-unshim-{}", - target.file_name().unwrap_or_default().to_string_lossy(), - std::process::id() - )); - std::fs::write(&tmp, &stripped).with_context(|| format!("writing {}", tmp.display()))?; - if let Ok(meta) = std::fs::metadata(&target) { - let _ = std::fs::set_permissions(&tmp, meta.permissions()); - } - if let Err(e) = std::fs::rename(&tmp, &target) { - let _ = std::fs::remove_file(&tmp); - return Err(e).with_context(|| format!("replacing {}", target.display())); - } + replace_profile_atomically(&path, &stripped, "unshim")?; changed.push(path); } Ok(changed) @@ -1225,7 +1353,7 @@ pub(crate) fn remove_path_block_from_profiles( /// after the path line; we excise that whole span, so whatever surrounded it — /// including "the file ended right here, no newline" — is restored verbatim. fn strip_block(content: &str, block: &ShimBlock) -> Option { - let marker = block.marker; + let marker: &str = &block.marker; // The marker as it sits on its own line: find a line whose trimmed text is // exactly the block's marker. We scan line starts so an in-prose mention of // the string can't be mistaken for the marker. @@ -1260,7 +1388,7 @@ fn strip_block(content: &str, block: &ShimBlock) -> Option { .find('\n') .map(|n| block_end + n + 1) .unwrap_or(content.len()); - if content[block_end..path_line_end].contains(block.dir_marker) { + if content[block_end..path_line_end].contains(&*block.dir_marker) { block_end = path_line_end; // our PATH line + its newline } } @@ -2239,10 +2367,10 @@ mod tests { // so the test asserts the ENGINE's block-independence without depending on // `node::shim`'s constant (this is the pm layer's own test). const TEST_NODE_BLOCK: ShimBlock = ShimBlock { - marker: "# nub node shim", - posix_line: r#"export PATH="$HOME/.nub/node-shim:$PATH""#, - fish_line: "set -gx PATH $HOME/.nub/node-shim $PATH", - dir_marker: ".nub/node-shim", + marker: Cow::Borrowed("# nub node shim"), + posix_line: Cow::Borrowed(r#"export PATH="$HOME/.nub/node-shim:$PATH""#), + fish_line: Cow::Borrowed("set -gx PATH $HOME/.nub/node-shim $PATH"), + dir_marker: Cow::Borrowed(".nub/node-shim"), }; #[test] @@ -2313,7 +2441,7 @@ mod tests { assert_eq!( add_path_block_for("tcsh", &home, None, &PM_SHIM_BLOCK).unwrap(), ProfileOutcome::Manual { - line: SHIMS_POSIX_PATH_LINE + line: SHIMS_POSIX_PATH_LINE.to_string() } ); } @@ -2693,6 +2821,61 @@ mod tests { ); } + /// What temp + rename BUYS on this path is torn-write safety, and what it + /// COSTS is this: `rename` onto a symlinked `~/.zshrc` replaces the link + /// with a regular file and orphans the dotfiles copy, where the plain + /// `std::fs::write` it replaces followed the link. Canonicalizing first is + /// what keeps the old behaviour, and this test is what holds it — dropping + /// the `canonicalize` in `replace_profile_atomically` turns it red, as it + /// does the strip-path test above. + /// + /// The torn write itself has no unit test: it needs the process to die + /// between truncate and `write_all`. The atomicity is argued from `rename`, + /// not measured here. + #[cfg(unix)] + #[test] + fn rewriting_edits_through_a_symlinked_profile_without_replacing_the_link() { + let home = tmpdir("symlink-rewrite"); + let home = home.as_path(); + let dotfiles = home.join("dotfiles"); + std::fs::create_dir_all(&dotfiles).unwrap(); + let real = dotfiles.join("zshrc"); + // The pre-#752 line, so wiring the current block rewrites rather than appends. + std::fs::write( + &real, + format!("export EDITOR=vi\n\n{BLOCK_MARKER}\nexport PATH=\"$HOME/.nub/shims:$PATH\"\n"), + ) + .unwrap(); + std::os::unix::fs::symlink(&real, home.join(".zshrc")).unwrap(); + // The sibling zsh profile carries the same old line, as a pre-#752 nub + // wrote it. An empty one would be an `Added`, which outranks + // `Rewritten` in the fold and would hide the case under test. + std::fs::write( + home.join(".zshenv"), + format!("{BLOCK_MARKER}\nexport PATH=\"$HOME/.nub/shims:$PATH\"\n"), + ) + .unwrap(); + + let outcome = add_path_block_for("zsh", home, None, &PM_SHIM_BLOCK).unwrap(); + assert!( + matches!(outcome, ProfileOutcome::Rewritten(_)), + "expected a rewrite, got {outcome:?}" + ); + assert!( + home.join(".zshrc").symlink_metadata().unwrap().is_symlink(), + "the profile must still be a symlink — replacing it orphans the dotfiles copy" + ); + let after = std::fs::read_to_string(&real).unwrap(); + assert!( + after.starts_with("export EDITOR=vi\n"), + "the rest of the profile must survive the rewrite:\n{after}" + ); + assert!( + after.contains("XDG_DATA_HOME") && !after.contains(".nub/shims"), + "the line under the marker must be the new one:\n{after}" + ); + } + #[cfg(unix)] #[test] fn reachability_reports_the_first_hit_and_flags_shadowing() { @@ -2769,4 +2952,119 @@ mod tests { "a dangling bin entry must name the missing file, got: {err}" ); } + + /// The global-bin block's directory is resolved at runtime, so it is the one + /// family whose line can legitimately CHANGE between runs. Wiring it twice + /// must leave one block, and wiring a different directory must REPLACE the + /// line rather than add a second — a profile that accumulates PATH entries + /// is invisible to the user, because a shell only ever reports the winner. + #[test] + fn global_bin_block_is_written_once_and_rewritten_in_place() { + let home = tmpdir("global-bin-idem"); + let home = home.as_path(); + let first = home.join(".local/bin"); + let block = global_bin_block(&first, home); + + let added = add_path_block_for("zsh", home, None, &block.inner).unwrap(); + assert!(matches!(added, ProfileOutcome::Added(_)), "got {added:?}"); + + let again = add_path_block_for("zsh", home, None, &block.inner).unwrap(); + assert!( + matches!(again, ProfileOutcome::AlreadyPresent(_)), + "a second identical wiring must be a no-op, got {again:?}" + ); + + let zshrc = std::fs::read_to_string(home.join(".zshrc")).unwrap(); + assert_eq!( + zshrc.matches(GLOBAL_BIN_MARKER).count(), + 1, + "two wirings must leave exactly one block:\n{zshrc}" + ); + + // Relocate: same marker, different directory. + let moved = home.join("elsewhere/bin"); + let moved_block = global_bin_block(&moved, home); + let rewritten = add_path_block_for("zsh", home, None, &moved_block.inner).unwrap(); + assert!( + matches!(rewritten, ProfileOutcome::Rewritten(_)), + "a changed directory must rewrite, got {rewritten:?}" + ); + + let zshrc = std::fs::read_to_string(home.join(".zshrc")).unwrap(); + assert_eq!( + zshrc.matches(GLOBAL_BIN_MARKER).count(), + 1, + "relocating must not add a second block:\n{zshrc}" + ); + assert!( + zshrc.contains("elsewhere/bin"), + "the new directory must be present:\n{zshrc}" + ); + assert!( + !zshrc.contains("$HOME/.local/bin"), + "the stale directory must be gone, not merely outranked:\n{zshrc}" + ); + } + + /// `Rewritten` is reachable for a FIXED-directory family too, which is why + /// cli.rs handles it rather than treating it as dead. The directory is a + /// compile-time constant within one build, but the CONSTANT ITSELF changed + /// between releases (#752 moved the shims under `XDG_DATA_HOME`), so a + /// profile written by an older nub carries a different line under the same + /// marker. cli.rs strips the block instead only when the legacy DIRECTORY + /// still exists, so a synced dotfile on a machine that never had + /// `~/.nub/shims` lands here — with the live shell still pointing at the + /// old directory, which is why that arm prints the re-source hint. + #[test] + fn an_upgraded_profile_rewrites_the_pre_xdg_shims_line() { + let home = tmpdir("shims-upgrade"); + let home = home.as_path(); + // What a pre-#752 nub wrote: BOTH zsh profiles, since `Added` on any one + // target outranks `Rewritten` in the fold and would mask this. + let legacy = format!("{BLOCK_MARKER}\nexport PATH=\"$HOME/.nub/shims:$PATH\"\n"); + std::fs::write(home.join(".zshrc"), &legacy).unwrap(); + std::fs::write(home.join(".zshenv"), &legacy).unwrap(); + + let outcome = add_path_block_for("zsh", home, None, &PM_SHIM_BLOCK).unwrap(); + assert!( + matches!(outcome, ProfileOutcome::Rewritten(_)), + "a profile carrying the pre-XDG line must be rewritten, got {outcome:?}" + ); + + let zshrc = std::fs::read_to_string(home.join(".zshrc")).unwrap(); + assert!( + !zshrc.contains(".nub/shims"), + "the pre-XDG directory must be gone, not merely outranked:\n{zshrc}" + ); + assert!( + zshrc.contains("XDG_DATA_HOME"), + "the XDG line must replace it:\n{zshrc}" + ); + assert_eq!( + zshrc.matches(BLOCK_MARKER).count(), + 1, + "an upgrade must not add a second block:\n{zshrc}" + ); + } + + /// A directory under the home dir is emitted `$HOME`-relative so a profile + /// synced between machines keeps working, and both dialects quote it so a + /// path containing a space survives fish's word splitting. + #[test] + fn global_bin_block_lines_are_home_relative_and_quoted() { + let home = Path::new("/home/u"); + let block = global_bin_block(&home.join(".local/bin"), home); + assert_eq!( + &*block.inner.posix_line, + r#"export PATH="$HOME/.local/bin:$PATH""# + ); + assert_eq!( + &*block.inner.fish_line, + r#"set -gx PATH "$HOME/.local/bin" $PATH"# + ); + + // Outside the home dir there is nothing to relativize against. + let block = global_bin_block(Path::new("/opt/bin"), home); + assert_eq!(&*block.inner.posix_line, r#"export PATH="/opt/bin:$PATH""#); + } } diff --git a/crates/nub-core/src/version_management/mod.rs b/crates/nub-core/src/version_management/mod.rs index 54b3b5c73..dc51ef689 100644 --- a/crates/nub-core/src/version_management/mod.rs +++ b/crates/nub-core/src/version_management/mod.rs @@ -641,6 +641,37 @@ fn atomic_replace_file( .with_context(|| format!("set permissions on {}", tmp.display()))?; } + // Losing the race to move the stale value aside is expected and retryable — + // but it does not look the same on both platforms. Unix reports the source + // that the winner already renamed as `NotFound`. Windows reports + // `ACCESS_DENIED` instead, because the losing rename can land while the name + // is still held by the winner's in-flight rename or by a handle that has not + // closed yet. Treating that as fatal is what made a concurrent repair fail on + // Windows while the identical race retried cleanly everywhere else. + // + // Bounded, and only on Windows, because `PermissionDenied` on a rename is + // otherwise a real permission fault: unbounded retries would spin forever on + // a read-only directory, and unix keeps its exact previous semantics. + // + // The budget is REAL TIME, not attempts, because what it waits on is the + // holder finishing a `MoveFileExW` or closing a handle. An attempt count + // would not bound that: `thread::yield_now` is `SwitchToThread`, which only + // yields to a thread ready on the CURRENT processor and returns immediately + // when there is none — so on a multi-core box, with the competing repairer + // on another core, every attempt could burn in microseconds and fail for the + // reason it first failed. Sleeping is what actually gives the holder a + // window. Same shape rustup uses for Windows filesystem contention: its + // `is_retryable_dir_error` counts `PermissionDenied` as retryable and backs + // off in real time rather than yielding. + // + // The budget covers THIS rename, not a whole repair: `atomic_write_node_license` + // calls here three times per iteration of its own `0..16` loop, so a repair can + // hold up to 48 independent budgets. That is deliberate — the failure path is + // still capped at one budget, since exhausting it returns `Err` and unwinds, + // and only calls that succeed after partially spending theirs accumulate. + const CONTENTION_PAUSE: std::time::Duration = std::time::Duration::from_millis(1); + const CONTENTION_BUDGET: std::time::Duration = std::time::Duration::from_millis(500); + let mut contended = std::time::Duration::ZERO; loop { match std::fs::hard_link(&tmp, &dest) { Ok(()) => return Ok(()), @@ -657,6 +688,15 @@ fn atomic_replace_file( match std::fs::rename(&dest, &displaced) { Ok(()) => {} Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + Err(err) + if cfg!(windows) + && err.kind() == std::io::ErrorKind::PermissionDenied + && contended < CONTENTION_BUDGET => + { + std::thread::sleep(CONTENTION_PAUSE); + contended += CONTENTION_PAUSE; + continue; + } Err(err) => { return Err(err) .with_context(|| format!("moving stale {} aside", dest.display())); diff --git a/install.sh b/install.sh index a49568030..fd4e2317a 100755 --- a/install.sh +++ b/install.sh @@ -331,11 +331,33 @@ if echo "$PATH" | tr ':' '\n' | grep -qx "$bin_dir"; then exit 0 fi +# Being absent from $PATH does NOT mean the profile lacks our line: a profile +# edited by an earlier run is not reflected in the current shell until it is +# sourced. Re-running the installer from that same shell would then append the +# block again, once per run, forever. Match the line we are about to write, so +# a profile that already carries it is left alone. +profile_has_line() { + local file=$1 line=$2 + [[ -f "$file" ]] && grep -qxF "$line" "$file" +} + +already_wired() { + local file=$1 line=$2 + if profile_has_line "$file" "$line"; then + success "Already configured in $(tildify "$file"). Restart your shell, or run: nub --version" + return 0 + fi + return 1 +} + refresh_command="" case $(basename "${SHELL:-bash}") in zsh) config="$HOME/.zshrc" + if already_wired "$config" "$posix_path_line"; then + exit 0 + fi if [[ -w "$config" ]] || [[ ! -f "$config" ]]; then { echo '' @@ -352,6 +374,9 @@ bash) if [[ -w "$f" ]]; then config="$f"; break; fi done if [[ -n "$config" ]]; then + if already_wired "$config" "$posix_path_line"; then + exit 0 + fi { echo '' echo '# nub' @@ -363,6 +388,9 @@ bash) ;; fish) config="${XDG_CONFIG_HOME:-$HOME/.config}/fish/config.fish" + if already_wired "$config" "$fish_path_line"; then + exit 0 + fi if [[ -w "$config" ]] || [[ ! -f "$config" ]]; then mkdir -p "$(dirname "$config")" { diff --git a/site/content/docs/install/index.mdx b/site/content/docs/install/index.mdx index ecbb90692..27b0b0e02 100644 --- a/site/content/docs/install/index.mdx +++ b/site/content/docs/install/index.mdx @@ -437,6 +437,56 @@ The install engine is distinct from `nub pm`, the [package meta-manager](/docs/p The two compose: `nub pm shim` routes bare `npm` / `pnpm` / `yarn` through the pin while you keep using whatever installer you prefer. +## Global installs + +Passing `-g` installs a package for your user rather than a project, and links its executables somewhere your shell can find them: + +```bash +nub add -g cowsay # or: nub install -g cowsay +cowsay hello + +nub remove -g cowsay +``` + +Two directories are involved, and they are deliberately separate: + +| | Default | Purpose | +| --- | --- | --- | +| Executables | `~/.local/bin` | Shared with every tool that installs there, and already on `PATH` on most systems | +| Packages | `$XDG_DATA_HOME/nub/global` | Nub's own, beside the content store | + +Executables go to the conventional user-binary directory rather than one Nub owns, so a global install is runnable without configuring anything. Resolution order is `XDG_BIN_HOME`, then `$XDG_DATA_HOME/../bin`, then `~/.local/bin`, on every platform. Point it elsewhere with a setting: + +```bash +nub config set --global global-bin-dir ~/bin +``` + +Run `nub bin -g` to print the directory in use, and `nub root -g` for where the packages themselves live. + +### When the directory is not on your PATH + +Most Linux distributions already expose `~/.local/bin`; macOS does not. When Nub finds the directory missing from `PATH` after a global install, it adds it to your shell profile and says so: + +```console +$ nub add -g cowsay +Linked 2 bins into /Users/you/.local/bin + PATH: added /Users/you/.local/bin to /Users/you/.zshrc + Restart your shell, or source that file, to pick it up. +``` + +The block is written once, under a `# nub global bin` marker, for bash, zsh and fish. Installing again does not add a second copy, and pointing the directory somewhere new rewrites that line rather than stacking another. When no profile can be written, Nub prints the line for you to add by hand instead. + +### Name collisions + +The executable directory is shared, so a package may want a name something else already occupies. Nub links it only when it can show the existing entry is one of its own; otherwise it keeps your file and says what it skipped: + +```console +$ nub add -g cowsay +warning: not linking cowsay — /Users/you/.local/bin/cowsay already exists and was not created by this tool; remove it to link cowsay +``` + +The rest of the package's executables still link. Removing a global package likewise only unlinks entries it owns, so an executable that another install has since taken over is left alone. + ## Lifecycle scripts Some dependencies run build steps on install — `preinstall`, `install`, and `postinstall` scripts declared in their own `package.json` (across pnpm, npm, and Bun). Nub ships a **deny-by-default** posture: it does not run them indiscriminately the way npm does. You control which packages build. diff --git a/site/public/install.sh b/site/public/install.sh index a49568030..fd4e2317a 100755 --- a/site/public/install.sh +++ b/site/public/install.sh @@ -331,11 +331,33 @@ if echo "$PATH" | tr ':' '\n' | grep -qx "$bin_dir"; then exit 0 fi +# Being absent from $PATH does NOT mean the profile lacks our line: a profile +# edited by an earlier run is not reflected in the current shell until it is +# sourced. Re-running the installer from that same shell would then append the +# block again, once per run, forever. Match the line we are about to write, so +# a profile that already carries it is left alone. +profile_has_line() { + local file=$1 line=$2 + [[ -f "$file" ]] && grep -qxF "$line" "$file" +} + +already_wired() { + local file=$1 line=$2 + if profile_has_line "$file" "$line"; then + success "Already configured in $(tildify "$file"). Restart your shell, or run: nub --version" + return 0 + fi + return 1 +} + refresh_command="" case $(basename "${SHELL:-bash}") in zsh) config="$HOME/.zshrc" + if already_wired "$config" "$posix_path_line"; then + exit 0 + fi if [[ -w "$config" ]] || [[ ! -f "$config" ]]; then { echo '' @@ -352,6 +374,9 @@ bash) if [[ -w "$f" ]]; then config="$f"; break; fi done if [[ -n "$config" ]]; then + if already_wired "$config" "$posix_path_line"; then + exit 0 + fi { echo '' echo '# nub' @@ -363,6 +388,9 @@ bash) ;; fish) config="${XDG_CONFIG_HOME:-$HOME/.config}/fish/config.fish" + if already_wired "$config" "$fish_path_line"; then + exit 0 + fi if [[ -w "$config" ]] || [[ ! -f "$config" ]]; then mkdir -p "$(dirname "$config")" { diff --git a/tests/global-install/run.sh b/tests/global-install/run.sh new file mode 100755 index 000000000..824ee6d48 --- /dev/null +++ b/tests/global-install/run.sh @@ -0,0 +1,143 @@ +#!/usr/bin/env bash +# End-to-end guard for `nub install -g` / `nub remove -g` bin handling. +# +# The unit tests in `vendor/aube/.../commands/global.rs` cover the ownership +# LOGIC against constructed fixtures. This covers the INTEGRATION: a real +# registry install, the real content store, the real symlink shapes the linker +# produces, and the resolved on-disk layout. Three of the four defects below +# were invisible to unit tests because they only appear once a package has +# actually been installed. +# +# What it guards, and the defect each case exists for: +# +# 1. `remove -g` left every bin it linked behind in the global bin dir. +# Ownership was decided by testing whether the bin symlink resolved under +# the install dir, but under the isolated layout `node_modules/` is +# a symlink into the shared content store, so it resolves OUT of the +# install dir and the test never matched. +# 2. `add -g` over an overlapping alias set deleted the bin it had just +# linked. Bins are linked BEFORE priors are torn down (deliberately — a +# crash must leave a working copy), and two installs of one package at one +# version share a content-store path, so the prior's recorded target +# matched the live link. +# 3. `install -g` silently replaced a binary it did not own. The global bin +# dir is shared with every other tool that installs there. +# 4. Bins landed in a pnpm-named directory (`~/.local/share/pnpm`, +# `~/Library/pnpm`) that nothing puts on PATH, and the install reported +# success anyway. nubjs/nub#642, reported twice. +# +# HOW THIS SCRIPT LIES TO YOU, if you let it. Every check here reads "clean" +# when the command under test never ran, so a failed install prints a screen of +# passes. Five separate vacuous passes were found while writing it: +# +# - Run from a checkout with two lockfiles, `install -g` aborts with +# ERR_NUB_LOCKFILE_AMBIGUOUS before doing any global work. Hence the `cd` +# to an empty directory. +# - A second package with a native dependency (`lolcatjs` -> `sleep`) fails +# its node-gyp `install` script on current Node, the cleanup guard rolls +# the install back, and the teardown path never runs. Hence `semver`. +# - `printf > "$BIN_DIR/"` FOLLOWS an existing symlink, so on a build +# with defect 1 it writes through the link into the content store and +# corrupts the package. Every later check then reads the planted string +# back out and passes on ANY build. Hence the `rm -f` and the not-a-symlink +# assertion. +# - A relative path to the nub binary stops resolving after the `cd`. Hence +# the absolute-path resolution up top. +# - Any install failing unchecked. Hence the explicit aborts. +# +# So: RUN IT AGAINST A BUILD THAT PREDATES THE FIX TOO. If the control does not +# fail, the harness is broken, not the code. `git worktree add` at the +# merge-base and build there — never the shipped release, which trails main by +# unrelated commits. +# +# Requires network (installs cowsay + semver from the registry). +# +# Usage: tests/global-install/run.sh +set -uo pipefail + +NUB="${1:?usage: run.sh }" +NUB=$(cd "$(dirname "$NUB")" && pwd)/$(basename "$NUB") +[ -x "$NUB" ] || { echo "not executable: $NUB"; exit 2; } + +ROOT=$(mktemp -d) +HOME_DIR="$ROOT/home" +WORK="$ROOT/work" +mkdir -p "$HOME_DIR" "$WORK" +trap 'rm -rf "$ROOT"' EXIT +cd "$WORK" || exit 1 + +# PNPM_HOME is commonly exported on a developer machine and used to win over +# HOME when resolving the global root, so a run that does not clear it escapes +# the sandbox and writes into the real home. +run() { env -u PNPM_HOME -u XDG_DATA_HOME -u XDG_BIN_HOME HOME="$HOME_DIR" "$NUB" "$@"; } + +fail=0 +check() { if [ "$2" = "0" ]; then echo " PASS $1"; else echo " FAIL $1"; fail=1; fi; } +abort() { echo " ABORT $1"; tail -12 "$2" | sed 's/^/ /'; exit 2; } + +BIN_DIR=$(run bin -g 2>/dev/null | tail -1) +echo "global bin dir: $BIN_DIR" + +echo +echo "== 1. install -g then remove -g leaves nothing behind ==" +run install -g cowsay > "$ROOT/i1.log" 2>&1 || abort "install -g failed" "$ROOT/i1.log" +[ -e "$BIN_DIR/cowsay" ]; check "cowsay linked after install" $? +run remove -g cowsay >/dev/null 2>&1 +# `-e` follows symlinks and so reports false for a DANGLING link; only the +# union of a link test and an existence test catches both leftover shapes. +leftover=$(find "$BIN_DIR" -maxdepth 1 \( -type l -o -type f \) 2>/dev/null || true) +[ -z "$leftover" ]; check "no bin entry left after remove -g" $? +[ -n "$leftover" ] && echo " leftover: $leftover" + +echo +echo "== 2. add -g over an overlapping alias set keeps the live bin ==" +run install -g cowsay >/dev/null 2>&1 +run install -g cowsay semver > "$ROOT/i2.log" 2>&1 || abort "overlapping install -g failed" "$ROOT/i2.log" +[ -e "$BIN_DIR/cowsay" ]; check "cowsay still resolves after the overlapping install" $? +[ -e "$BIN_DIR/semver" ]; check "semver linked by the new install" $? + +echo +echo "== 3. a foreign binary in the bin dir survives an install ==" +run remove -g cowsay >/dev/null 2>&1 +run remove -g semver >/dev/null 2>&1 +rm -f "$BIN_DIR/cowsay" +printf '#!/bin/sh\necho FOREIGN\n' > "$BIN_DIR/cowsay" +chmod +x "$BIN_DIR/cowsay" +[ ! -L "$BIN_DIR/cowsay" ] || { echo " ABORT planted file is a symlink, not a real file"; exit 2; } +run install -g cowsay > "$ROOT/i3.log" 2>&1 || abort "install -g failed" "$ROOT/i3.log" +out=$("$BIN_DIR/cowsay" 2>/dev/null || true) +[ "$out" = "FOREIGN" ]; check "foreign cowsay not overwritten by install -g" $? +[ -e "$BIN_DIR/cowthink" ]; check "the non-colliding bin of the same package still links" $? + +echo +echo "== 4. the layout is nub-named and PATH-conventional ==" +case "$BIN_DIR" in + */pnpm|*/Library/pnpm) echo " FAIL bin dir is still pnpm-named: $BIN_DIR"; fail=1 ;; + */.local/bin) echo " PASS bin dir is the conventional ~/.local/bin" ;; + *) echo " FAIL unexpected bin dir: $BIN_DIR"; fail=1 ;; +esac +[ -d "$HOME_DIR/.local/share/nub/global" ]; check "installs live under the data namespace" $? +[ ! -e "$HOME_DIR/.local/share/pnpm" ]; check "nothing written to a pnpm-named path" $? +[ ! -e "$HOME_DIR/Library/pnpm" ]; check "nothing written to ~/Library/pnpm" $? + +echo +echo "== 5. an install wires the bin dir onto PATH, exactly once ==" +# The sandboxed HOME guarantees the dir is absent from PATH, so the wiring must +# fire. Reporting success for commands that cannot run is #642 itself. The +# SECOND install is the load-bearing half: the block must not be duplicated, +# and a profile that grows an entry per run is invisible to the user because a +# shell only ever reports the winning PATH entry. +run remove -g cowsay >/dev/null 2>&1 +rm -f "$HOME_DIR/.zshrc" +SHELL=/bin/zsh run install -g cowsay > "$ROOT/i5.log" 2>&1 +grep -q "PATH: added" "$ROOT/i5.log"; check "install reports wiring the bin dir onto PATH" $? +grep -q '^# nub global bin$' "$HOME_DIR/.zshrc" 2>/dev/null; check "the marked block landed in the profile" $? + +SHELL=/bin/zsh run install -g cowsay > "$ROOT/i5b.log" 2>&1 +blocks=$(grep -c '^# nub global bin$' "$HOME_DIR/.zshrc" 2>/dev/null || echo 0) +[ "$blocks" = "1" ]; check "a second install leaves exactly one block (got $blocks)" $? +grep -q "already configured" "$ROOT/i5b.log"; check "the second install says it is already configured" $? + +echo +if [ "$fail" = "0" ]; then echo "ALL CHECKS PASSED"; else echo "FAILURES PRESENT"; fi +exit "$fail" diff --git a/vendor/aube/crates/aube-linker/src/lib.rs b/vendor/aube/crates/aube-linker/src/lib.rs index 2d2bf3ad4..db92e6cb7 100644 --- a/vendor/aube/crates/aube-linker/src/lib.rs +++ b/vendor/aube/crates/aube-linker/src/lib.rs @@ -45,8 +45,11 @@ pub use sweep::{is_physical_importer, mkdirp, remove_dir_all_with_retry, sweep_s pub(crate) use sweep::{sweep_stale_top_level_entries, try_remove_entry}; pub use sys::{ BinShimOptions, create_bin_shim, create_dir_link, is_native_executable, normalize_path, - parse_posix_shim_target, remove_bin_shim, validate_bin_name, validate_bin_target, + parse_posix_shim_target, parse_win_shim_target, remove_bin_shim, validate_bin_name, + validate_bin_target, }; +#[cfg(windows)] +pub use sys::win_shim_paths; /// Strategy for arranging packages under `node_modules/`. /// diff --git a/vendor/aube/crates/aube-linker/src/sys.rs b/vendor/aube/crates/aube-linker/src/sys.rs index 18e4736ee..2435cb225 100644 --- a/vendor/aube/crates/aube-linker/src/sys.rs +++ b/vendor/aube/crates/aube-linker/src/sys.rs @@ -502,8 +502,12 @@ fn write_shim_file(dst: &Path, contents: &[u8]) -> io::Result<()> { /// `.ps1` stub. Index 0 is the extensionless wrapper — callers that /// already unlinked it (the unix-first branch of `remove_bin_shim`) /// can skip it with `.into_iter().skip(1)`. +/// +/// Public so an ownership check can be driven off the SAME list the writer +/// uses. A guard that hardcodes its own extensions drifts from this one +/// silently, and the drift is invisible until a foreign shim is overwritten. #[cfg(windows)] -fn win_shim_paths(bin_dir: &Path, name: &str) -> [PathBuf; 3] { +pub fn win_shim_paths(bin_dir: &Path, name: &str) -> [PathBuf; 3] { [ bin_dir.join(name), bin_dir.join(format!("{name}.cmd")), @@ -808,7 +812,52 @@ fn safe_prog(prog: &str) -> &str { } } -#[cfg(windows)] +/// Extract the `%~dp0`-relative target a Windows `.cmd` shim execs. +/// +/// Both shapes `create_bin_shim` emits: the direct-exec wrapper for a native +/// binary (`@"%~dp0\" %*`) and the node wrapper, whose IF branch names +/// `node.exe` and whose ELSE branch carries the real target. +/// +/// The SINGLE reader of this format: the link path (`bin_slot_is_writable`) and +/// the delete path (`unlink_bins`) both call it, so a change to the writer +/// cannot leave one of them behind. Reads the same on any platform, so it is +/// testable without a Windows runner. +/// +/// Recovering a target does NOT prove the shim is ours. pnpm and yarn classic +/// use `@zkochan/cmd-shim`, which emits the same `"%~dp0\"` shape this +/// crate does, so their wrappers parse here too — ownership is decided by where +/// the recovered target RESOLVES, never by the shape. npm's own `cmd-shim` is +/// structurally different: it assigns `SET dp0=%~dp0` once and builds every path +/// from `%dp0%`, so `%~dp0\` never appears and it yields `None`. +pub fn parse_win_shim_target(content: &str) -> Option { + content.lines().find_map(|line| { + let line = line.trim(); + if let Some(after) = line.strip_prefix("@\"%~dp0\\") { + let end = after.find('"')?; + return Some(after[..end].to_string()); + } + if line.contains("%~dp0\\") && !line.contains(".exe\"") { + let start = line.find("%~dp0\\")?; + let after = &line[start + 6..]; + let end = after.find('"')?; + return Some(after[..end].to_string()); + } + None + }) +} + +/// Render the `.cmd` wrapper text for a Windows bin shim. +/// +/// Deliberately NOT `#[cfg(windows)]`, and public: this is pure string +/// formatting with no platform API, and the ownership guard's parser has to be +/// testable against what this actually emits. Gating it to Windows is what +/// forced that test onto hand-written fixtures, and a parser checked only +/// against invented input is how the writer and the reader drifted apart in the +/// first place. +/// +/// `cfg(test)` keeps it out of a non-Windows release build, where nothing but +/// the round-trip test calls it. +#[cfg(any(windows, test))] fn generate_cmd_shim( launch: &BinLaunch, rel_target_backslash: &str, @@ -2822,4 +2871,82 @@ mod tests { "unsafe prog spliced into posix shim:\n{shim}" ); } + /// Round-trip the ownership parser against what the WRITER emits, rather + /// than against a hand-written fixture. + /// + /// The parser exists to tell a shim this tool wrote from one npm, pnpm or + /// yarn wrote. Checking it on invented input only confirms the invention: + /// two Windows-only defects reached review that way, because the local + /// suite compiled the Windows arms away and the fixtures encoded the same + /// assumption the code did. Runs on every platform — `generate_cmd_shim` + /// is pure formatting and is deliberately not gated to Windows. + #[test] + fn parse_win_shim_target_recovers_what_generate_cmd_shim_embeds() { + for launch in [BinLaunch::Direct, BinLaunch::Interpreter("node".to_string())] { + let rel = r"..\share\nub\global\1a-2b\node_modules\pkg\cli.js"; + let text = generate_cmd_shim(&launch, rel, None); + assert_eq!( + parse_win_shim_target(&text).as_deref(), + Some(rel), + "the parser must recover exactly the target the writer embedded \ + ({launch:?}); emitted text was:\n{text}" + ); + } + } + + /// The direction with consequences: a spurious `Some(...)` is how the guard + /// claims a slot nub does not own. + /// + /// Asserted against npm's REAL `cmd-shim` output, not an invented string. + /// A hand-written fixture is the correct instrument here and nowhere else, + /// because that writer lives in another repo: it assigns `SET dp0=%~dp0` + /// once and builds every path from `%dp0%`, so the literal `%~dp0\` this + /// parser keys on never appears. + /// + /// What it pins is narrower than it looks, and worth stating so nobody + /// relies on it for more: a fallback substring scan makes this fail, and + /// that is the one loosening it catches. Dropping the `.exe"` filter or + /// matching a bare `%~dp0` still yields `None` here — verified — because an + /// npm shim has no `%~dp0` followed by a backslash anywhere, so both + /// branches miss on shape before either condition is reached. Those two are + /// pinned by `parse_win_shim_target_recovers_what_generate_cmd_shim_embeds` + /// instead, whose node-dialect input carries `"%~dp0\node.exe"` in its IF + /// branch. + #[test] + fn parse_win_shim_target_rejects_an_npm_cmd_shim() { + let npm_shim = concat!( + "@ECHO off\r\n", + "GOTO start\r\n", + ":find_dp0\r\n", + "SET dp0=%~dp0\r\n", + "EXIT /b\r\n", + ":start\r\n", + "SETLOCAL\r\n", + "CALL :find_dp0\r\n", + "IF EXIST \"%dp0%\\node.exe\" (\r\n", + " SET \"_prog=%dp0%\\node.exe\"\r\n", + ") ELSE (\r\n", + " SET \"_prog=node\"\r\n", + " SET PATHEXT=%PATHEXT:;.JS;=;%\r\n", + ")\r\n", + "endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & ", + "\"%_prog%\" \"%dp0%\\..\\pkg\\cli.js\" %*\r\n", + ); + assert_eq!( + parse_win_shim_target(npm_shim), + None, + "an npm cmd-shim must not be claimed as ours" + ); + } + + /// The NODE_PATH line the node dialect emits is unquoted `%~dp0`, so it must + /// not be mistaken for the target — the failure mode would be silently + /// claiming somebody else's slot. + #[test] + fn parse_win_shim_target_ignores_the_node_path_line() { + let rel = r"..\pkg\cli.js"; + let text = generate_cmd_shim(&BinLaunch::Interpreter("node".to_string()), rel, Some("%~dp0\\..\\node_modules")); + assert_eq!(parse_win_shim_target(&text).as_deref(), Some(rel)); + } + } diff --git a/vendor/aube/crates/aube/src/commands/add/global.rs b/vendor/aube/crates/aube/src/commands/add/global.rs index 0bb9a8e3e..2d5af2560 100644 --- a/vendor/aube/crates/aube/src/commands/add/global.rs +++ b/vendor/aube/crates/aube/src/commands/add/global.rs @@ -307,7 +307,13 @@ async fn run_global_inner( ), hidden_modules_dir: None, }); - let linked = global::link_bins(install_dir, &layout.bin_dir, &aliases, shim_opts)?; + let linked = global::link_bins( + install_dir, + &layout.bin_dir, + &layout.pkg_dir, + &aliases, + shim_opts, + )?; // Now safe to drop priors. Errors here are non-fatal — the new // install is already live — but we still surface them so the user @@ -317,9 +323,18 @@ async fn run_global_inner( // at the *new* install dir (we overwrote it a few lines up). Deleting // the pointer in that case would break the live install, so we only // wipe the prior's physical dir + bins. + // A prior sharing a package+version with the new install resolves to the + // same content-store path, so its recorded bin targets still match the + // links `link_bins` wrote a moment ago. Excluding what we just linked is + // what stops the teardown from deleting the live install's own bins. + let linked_set: std::collections::BTreeSet<&str> = linked.iter().map(String::as_str).collect(); for prior in &priors { let res = if prior.hash == hash { - let bins = global::bin_names_for(&prior.install_dir, &prior.aliases); + let bins: Vec = + global::owned_bins(&prior.install_dir, &prior.aliases) + .into_iter() + .filter(|bin| !linked_set.contains(bin.name.as_str())) + .collect(); global::unlink_bins(&prior.install_dir, &layout.bin_dir, &bins); std::fs::remove_dir_all(&prior.install_dir) .or_else(|e| { @@ -336,7 +351,7 @@ async fn run_global_inner( ) }) } else { - global::remove_package(prior, layout) + global::remove_package(prior, layout, &linked_set) }; if let Err(e) = res { eprintln!("warning: failed to remove prior global install: {e}"); @@ -349,6 +364,11 @@ async fn run_global_inner( pluralizer::pluralize("bin", linked.len() as isize, true), layout.bin_dir.display() ); + // Whether the bin dir is on PATH — and what to do when it is not — is + // deliberately NOT reported here. A host that wires PATH itself would + // otherwise print a remediation immediately before announcing it had + // already applied it. `global::dir_is_on_path` is public so the host + // owns that decision and emits exactly one message. } Ok(()) diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index 6ef0fb797..bcd0931d8 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -1,17 +1,26 @@ //! Global install layout — `aube add -g`, `aube remove -g`, `aube list -g`. //! -//! Modeled on pnpm v11's per-install-dir layout: +//! Two roots, deliberately separate. Bins go to the SHARED user-binary +//! directory that systems already put on PATH; the installs themselves go +//! under our own namespaced data root, beside the content store: //! //! ```text -//! / # on PATH; bins symlink into here -//! ├── some-bin -> //node_modules/.bin/some-bin -//! └── global-aube/ # : one subdir per global package -//! ├── -/ # physical install dir (normal aube project) -//! │ ├── package.json -//! │ └── node_modules/ -//! └── -> - # stable pointer keyed on aliases +//! ~/.local/bin/ # : shared, on PATH, NOT ours alone +//! └── some-bin -> //node_modules// +//! +//! $XDG_DATA_HOME//global/ # : one subdir per global package +//! ├── -/ # physical install dir (normal aube project) +//! │ ├── package.json +//! │ └── node_modules/ +//! └── -> - # stable pointer keyed on aliases //! ``` //! +//! The split is the point. A tool-owned bin directory is on PATH for nobody +//! until something wires it up, which made a successful install produce +//! commands that would not run. Sharing the conventional directory fixes that +//! and costs one obligation: every write there must prove ownership first, +//! because the neighbours are other tools' binaries. +//! //! Each `aube add -g ` runs a full normal install into a fresh //! `-` directory, then: //! 1. Computes a hash of the resolved aliases. @@ -29,10 +38,10 @@ use std::path::{Path, PathBuf}; /// Where aube puts globally-installed packages and their PATH-visible bins. /// -/// `bin_dir` is the directory the user is expected to have on `$PATH` — -/// it's where bin symlinks live. `pkg_dir` is where the per-install -/// directories and hash pointers live; it's an aube-specific subdir so we -/// never step on a sibling pnpm install. +/// `bin_dir` is the shared user-binary directory the system already has on +/// `PATH` — it holds entries from every tool that installs there, so it is +/// never ours to write blindly. `pkg_dir` holds the per-install directories +/// and hash pointers, under our own data namespace where nothing else lives. #[derive(Debug, Clone)] pub struct GlobalLayout { pub bin_dir: PathBuf, @@ -43,10 +52,12 @@ impl GlobalLayout { pub fn resolve() -> miette::Result { let cwd = std::env::current_dir().unwrap_or_default(); - // `bin_dir` and `pkg_dir` are independent: `globalBinDir` controls - // where bin symlinks go (on PATH), `globalDir` controls where - // package installs live. Neither inherits from the other — both - // fall back to the default home (_HOME → PNPM_HOME → platform). + // `bin_dir` and `pkg_dir` are independent, and now resolve from + // DIFFERENT roots: bins go to the shared user-binary directory that is + // already on PATH ([`default_bin_dir`]), while package installs go + // under our own namespaced data root ([`prefix_dir`]). They used to + // share one root, which is how bins ended up in a directory named + // after another package manager and on nobody's PATH. let (setting_bin, setting_pkg) = super::with_settings_ctx(&cwd, |ctx| { let bin = aube_settings::resolved::global_bin_dir(ctx) .and_then(|raw| super::expand_setting_path(&raw, &cwd)); @@ -55,72 +66,107 @@ impl GlobalLayout { (bin, pkg) }); - let bin_dir = setting_bin.map_or_else(resolve_home, Ok)?; - // Package-install subdir named after the active embedder so we never - // step on a sibling pnpm install. Standalone aube → `global-aube`. - let pkg_subdir = format!("global-{}", aube_util::embedder().name); + let bin_dir = setting_bin.map_or_else(default_bin_dir, Ok)?; + // A `global` leaf under the resolved root, matching pnpm's + // `/global`. Under our own data namespace there is no sibling + // install to collide with, so the directory does not need the + // embedder's name in it. let pkg_dir = setting_pkg.map_or_else( - || resolve_home().map(|h| h.join(&pkg_subdir)), - |p| Ok(p.join(&pkg_subdir)), + || prefix_dir().map(|h| h.join("global")), + |p| Ok(p.join("global")), )?; Ok(Self { bin_dir, pkg_dir }) } } -/// Resolve the PATH-visible root. Honors the branded `_HOME` -/// (standalone aube → `AUBE_HOME`), then `PNPM_HOME` (so existing pnpm users -/// already have the right dir on PATH), then a platform-specific pnpm-style -/// default. An embedder with no `env_prefix` skips the branded var. -fn resolve_home() -> miette::Result { +/// Resolve the PATH-visible directory global bins link into. +/// +/// This follows the shared user-binary convention rather than owning a +/// directory of our own, in that order: +/// +/// - `XDG_BIN_HOME` +/// - `$XDG_DATA_HOME/../bin`, so a relocated XDG root is respected +/// - `~/.local/bin` +/// +/// On every platform, matching uv and pipx. The point is that most Linux +/// distributions already put `~/.local/bin` on PATH — the XDG spec asks them +/// to — so a global install is runnable without editing a shell profile. A +/// tool-owned directory is on PATH for nobody until something wires it up, +/// which is the whole defect this resolution exists to avoid. +/// +/// It also means the directory is SHARED with every other tool that installs +/// there, so nothing may be overwritten without proving we own it — see +/// [`bin_slot_is_writable`]. +fn default_bin_dir() -> miette::Result { + // A tool with its own home var keeps owning both roots when the user sets + // it — an explicit `_HOME` is a deliberate instruction, not a + // default to be second-guessed. An embedder with no `env_prefix` (nub) + // skips this and takes the conventional chain below. if let Some(prefix) = aube_util::embedder().env_prefix && let Ok(v) = std::env::var(format!("{prefix}_HOME")) && !v.is_empty() { return Ok(PathBuf::from(v)); } - if let Ok(v) = std::env::var("PNPM_HOME") + if let Ok(v) = std::env::var("XDG_BIN_HOME") && !v.is_empty() { return Ok(PathBuf::from(v)); } - platform_default() + // `parent()` rather than joining a literal `..`: the path is printed by + // `bin -g`, compared against PATH entries, and written into shell + // profiles, and a `..` component makes all three read wrong. + if let Some(parent) = aube_util::env::xdg_data_home() + .as_deref() + .and_then(Path::parent) + { + return Ok(parent.join("bin")); + } + let home = aube_util::env::home_dir() + .ok_or_else(|| miette!("HOME is not set; can't locate the global bin directory"))?; + Ok(home.join(".local/bin")) } -/// Resolve the global prefix root. This is distinct from `globalBinDir`: -/// users may point global bin symlinks somewhere else while the prefix -/// itself still comes from `AUBE_HOME` / `PNPM_HOME` / the platform default. +/// Resolve the root holding global package installs — distinct from the bin +/// directory, which is shared and lives on PATH. +/// +/// This is our own namespaced data directory, the same root and the same +/// `data_namespace` the content store already uses, so a global install lands +/// beside the store instead of inside a directory named after another package +/// manager. No `PNPM_HOME` and no pnpm-named path: a global operation does not +/// consult whatever tool a project happens to use. pub fn prefix_dir() -> miette::Result { - resolve_home() -} - -// Linux plus every other Unix (FreeBSD, Android/Termux, …): pnpm -// special-cases only macOS (`~/Library/pnpm`), while Windows has its own -// arm below. Scoped to `unix` so a non-Unix, non-Windows target doesn't -// silently inherit the XDG/HOME logic — it gets a compile error instead, -// which is the signal we'd want before shipping such a build. -#[cfg(all(unix, not(target_os = "macos")))] -fn platform_default() -> miette::Result { + let ns = aube_util::embedder().data_namespace; + if let Some(prefix) = aube_util::embedder().env_prefix + && let Ok(v) = std::env::var(format!("{prefix}_HOME")) + && !v.is_empty() + { + return Ok(PathBuf::from(v)); + } if let Some(xdg) = aube_util::env::xdg_data_home() { - return Ok(xdg.join("pnpm")); + return Ok(xdg.join(ns)); + } + #[cfg(windows)] + if let Ok(local) = std::env::var("LOCALAPPDATA") + && !local.is_empty() + { + return Ok(PathBuf::from(local).join(ns)); } let home = aube_util::env::home_dir() - .ok_or_else(|| miette!("HOME is not set; can't locate global directory"))?; - Ok(home.join(".local/share/pnpm")) + .ok_or_else(|| miette!("HOME is not set; can't locate the global directory"))?; + Ok(home.join(".local/share").join(ns)) } -#[cfg(target_os = "macos")] -fn platform_default() -> miette::Result { - let home = std::env::var("HOME") - .map_err(|_| miette!("HOME is not set; can't locate global directory"))?; - Ok(PathBuf::from(home).join("Library/pnpm")) -} - -#[cfg(target_os = "windows")] -fn platform_default() -> miette::Result { - let local = std::env::var("LOCALAPPDATA") - .map_err(|_| miette!("LOCALAPPDATA is not set; can't locate global directory"))?; - Ok(PathBuf::from(local).join("pnpm")) +/// Whether `dir` is already an entry in `PATH`. Compared canonically so a +/// symlinked home or a trailing slash does not read as absent and produce a +/// warning telling the user to add something they already have. +pub fn dir_is_on_path(dir: &Path) -> bool { + let want = std::fs::canonicalize(dir).unwrap_or_else(|_| dir.to_path_buf()); + let Some(path) = std::env::var_os("PATH") else { + return false; + }; + std::env::split_paths(&path).any(|entry| std::fs::canonicalize(&entry).unwrap_or(entry) == want) } /// Create a fresh install directory under `pkg_dir`. Matches pnpm's naming @@ -257,14 +303,137 @@ pub fn symlink_force(target: &Path, link: &Path) -> miette::Result<()> { Ok(()) } +/// Whether a global install may write `/`. +/// +/// An empty slot is free. A slot owned by some install under `pkg_dir` is +/// ours to replace — that covers this install's own prior links, which +/// `add -g` legitimately overwrites on a re-add. Anything else is FOREIGN: +/// another tool's binary, or one the user put there by hand. +/// +/// The global bin dir is shared by construction — it is whatever directory +/// the user has on `PATH`, so it holds entries from every tool that installs +/// there — which is why the mere existence of the slot can never be taken as +/// permission to overwrite it. +fn bin_slot_is_writable(bin_dir: &Path, pkg_dir: &Path, name: &str) -> bool { + // Windows writes THREE files per bin (``, `.cmd`, `.ps1`) + // and overwrites each unconditionally, so the name is occupied when ANY of + // them is. Checking only the extensionless path misses the common case + // outright: npm, pnpm and yarn install a `.cmd` with no extensionless + // sibling, so the slot reads as empty and their shim is replaced — the very + // thing this guard exists to prevent. Driven off the writer's own list so + // the two cannot drift apart. + #[cfg(windows)] + { + // The `.cmd` decides the whole slot. Of the three files the writer + // emits, only that one carries a recoverable target: the `.ps1` and the + // extensionless wrapper resolve `$basedir` at run time and stamp no + // marker, so demanding that every path prove itself rejects the shims + // this tool wrote moments earlier. `unlink_bins` keys on `{name}.cmd` + // for the same reason, so the two now agree. + // + // Getting this wrong is not a warning, it is data loss: an unwritable + // slot makes `link_bins` skip every bin, and `add -g` reads the empty + // result as "nothing was re-linked", which disarms the filter guarding + // the prior install's bins — so re-adding a package you already have + // removes it and leaves no command behind. + let cmd = bin_dir.join(format!("{name}.cmd")); + if cmd.symlink_metadata().is_ok() { + return slot_entry_is_ours(&cmd, pkg_dir); + } + // No `.cmd`, so nothing here came from this writer — it never emits a + // sibling without one. Any other occupant is somebody else's. + aube_linker::win_shim_paths(bin_dir, name) + .iter() + .all(|p| p.symlink_metadata().is_err()) + } + #[cfg(not(windows))] + { + slot_entry_is_ours(&bin_dir.join(name), pkg_dir) + } +} + +/// Whether one concrete path in the bin dir is free, or is occupied by an +/// entry this tool created. See [`bin_slot_is_writable`] for the policy. +fn slot_entry_is_ours(link: &Path, pkg_dir: &Path) -> bool { + let bin_dir = link.parent().unwrap_or(Path::new("")); + let Ok(meta) = link.symlink_metadata() else { + return true; // nothing there + }; + // Both forms of the package dir. A lexical path never matches a + // canonicalized one once any component is a symlink — macOS `/tmp` -> + // `/private/tmp`, a symlinked `$HOME` under Docker or Nix, a relocated + // `XDG_DATA_HOME` — and the dangling-link arm below compares a LEXICAL + // target, so testing only the canonical form reports a bin we created as + // somebody else's. `unlink_bins` keeps both for the same reason. + let pkg_canon = std::fs::canonicalize(pkg_dir).unwrap_or_else(|_| pkg_dir.to_path_buf()); + let pkg_lex = aube_linker::normalize_path(pkg_dir); + + if meta.file_type().is_symlink() { + let Ok(raw) = std::fs::read_link(link) else { + return false; + }; + let absolute = if raw.is_absolute() { + raw + } else { + link.parent().unwrap_or(bin_dir).join(raw) + }; + // Surface shape: the link points straight into the global pkg dir. + let lex = aube_linker::normalize_path(&absolute); + if lex.starts_with(&pkg_lex) || lex.starts_with(&pkg_canon) { + return true; + } + match std::fs::canonicalize(&absolute) { + // Store shape: the link resolves through `node_modules/` + // into the shared content store, landing outside `pkg_dir` + // entirely, so it can only be recognised by matching it against + // what the installs living there actually own. + Ok(resolved) => scan_packages(pkg_dir).iter().any(|info| { + owned_bins(&info.install_dir, &info.aliases) + .iter() + .any(|bin| bin.target.as_ref().is_some_and(|t| *t == resolved)) + }), + // Broken link into our own tree: a leftover of ours, so reclaiming + // it is right. Broken and pointing elsewhere stays untouched — we + // cannot show it is not something the user is repairing. + Err(_) => false, + } + } else { + // A regular file is one of ours only when its embedded target points + // back into the global package dir. + // + // The presence of a shim shape proves nothing about who wrote it: npm, + // pnpm and yarn all emit `%~dp0`-relative `.cmd` wrappers of the same + // form, so testing for the marker alone would adopt every one of them + // as ours and overwrite it — the precise failure this guard exists to + // stop. Only where the target RESOLVES distinguishes them. + let Ok(content) = std::fs::read_to_string(link) else { + return false; + }; + let rel = aube_linker::parse_posix_shim_target(&content) + .map(str::to_string) + .or_else(|| aube_linker::parse_win_shim_target(&content)); + let Some(rel) = rel else { + return false; + }; + let resolved = aube_linker::normalize_path(&bin_dir.join(rel.replace('\\', "/"))); + resolved.starts_with(&pkg_lex) || resolved.starts_with(&pkg_canon) + } +} + /// After a global install lands, link each resolved dependency's bins /// into ``. Bins are extracted from each package's `package.json` /// inside `/node_modules//`. Returns the list of bin /// names that were linked — callers use this list to undo the links on /// `aube remove -g`. +/// +/// A name already held by a foreign file is skipped with a warning rather +/// than overwritten, and rather than failing the whole install: the other +/// packages in the same command still have to land, and the occupant is the +/// user's to remove. pub fn link_bins( install_dir: &Path, bin_dir: &Path, + pkg_dir: &Path, aliases: &[String], shim_opts: aube_linker::BinShimOptions, ) -> miette::Result> { @@ -274,8 +443,8 @@ pub fn link_bins( let modules = super::project_modules_dir(install_dir); let mut linked = Vec::new(); for alias in aliases { - let pkg_dir = modules.join(alias); - let manifest_path = pkg_dir.join("package.json"); + let alias_dir = modules.join(alias); + let manifest_path = alias_dir.join("package.json"); let Ok(raw) = std::fs::read_to_string(&manifest_path) else { continue; }; @@ -302,7 +471,15 @@ pub fn link_bins( { continue; } - let target = pkg_dir.join(&rel); + if !bin_slot_is_writable(bin_dir, pkg_dir, &name) { + eprintln!( + "warning: not linking {name} — {} already exists and was not \ + created by this tool; remove it to link {name}", + bin_dir.join(&name).display() + ); + continue; + } + let target = alias_dir.join(&rel); aube_linker::create_bin_shim(bin_dir, &name, &target, shim_opts) .into_diagnostic() .wrap_err_with(|| format!("failed to create bin shim for {name}"))?; @@ -312,26 +489,38 @@ pub fn link_bins( Ok(linked) } -/// Remove bin symlinks we own. Only unlinks entries whose symlink target -/// points inside `install_dir` — any bin that was overwritten by a later -/// `aube add -g` is owned by that later install, so we leave it alone. +/// Remove bin entries we own. A bin that was overwritten by a later +/// `aube add -g` belongs to that later install, so we leave it alone. +/// +/// Ownership is decided two ways, and the first is what makes an isolated +/// install work at all: a symlinked bin resolves THROUGH +/// `node_modules/` into the shared content store, so it lands +/// outside `install_dir` and a containment test alone never matches — the +/// bins then leak as dangling links every `remove -g`. Comparing the +/// resolved link against the target [`owned_bins`] captured from this +/// install's own manifest is what identifies it. Containment stays as the +/// second test for the layouts that do keep the target inside the install. +/// +/// Both sides are canonicalized. On macOS, temp dirs like `/var/folders/...` +/// are symlinks to `/private/var/folders/...`; without canonicalizing both +/// the comparison always returns false and the bins leak. /// -/// Both the target and `install_dir` are canonicalized before the -/// `starts_with` check. On macOS, temp dirs like `/var/folders/...` are -/// actually symlinks to `/private/var/folders/...`; without canonicalizing -/// both sides the comparison always returns false and the bins leak. -pub fn unlink_bins(install_dir: &Path, bin_dir: &Path, bin_names: &[String]) { +/// Two installs providing the same package at the same version resolve to +/// the same store path, so target equality cannot tell their bins apart and +/// either `remove -g` reclaims the shared link. That is deliberate: the +/// links are byte-identical, and re-running `add -g` restores the survivor's. +pub fn unlink_bins(install_dir: &Path, bin_dir: &Path, bins: &[OwnedBin]) { #[cfg(unix)] { let install_canon = std::fs::canonicalize(install_dir).ok(); - // Lex-normalized `install_dir` is the fallback ownership anchor - // for regular-file shims (`preferSymlinkedExecutables=false`), - // where we can't canonicalize the shim's `$basedir/` target - // without following the project's symlinks into the shared - // virtual store. + // Lex-normalized `install_dir` is the ownership anchor for the two + // cases that cannot be canonicalized: a regular-file shim + // (`preferSymlinkedExecutables=false`), whose `$basedir/` target + // would resolve through the project's symlinks into the shared virtual + // store, and a dangling link, whose target no longer exists at all. let install_lex = aube_linker::normalize_path(install_dir); - for name in bin_names { - let link = bin_dir.join(name); + for bin in bins { + let link = bin_dir.join(&bin.name); // A scoped name (`@scope/foo`) puts the link a directory below // `bin_dir`, and `create_bin_shim` anchors both the symlink // target and the shim's `$basedir` on that deeper directory. @@ -340,21 +529,36 @@ pub fn unlink_bins(install_dir: &Path, bin_dir: &Path, bin_names: &[String]) { let link_parent = link.parent().unwrap_or(bin_dir); match std::fs::read_link(&link) { Ok(target) => { - // Symlink bin: fully resolve and check against - // `install_canon`. Matches the pre-settings behavior. let absolute = if target.is_absolute() { target } else { link_parent.join(target) }; - let Some(install_canon) = install_canon.as_ref() else { - continue; - }; - let Some(resolved) = std::fs::canonicalize(&absolute).ok() else { - continue; - }; - if resolved.starts_with(install_canon) { - let _ = std::fs::remove_file(&link); + match std::fs::canonicalize(&absolute) { + Ok(resolved) => { + let ours = bin.target.as_ref().is_some_and(|t| *t == resolved) + || install_canon + .as_ref() + .is_some_and(|canon| resolved.starts_with(canon)); + if ours { + let _ = std::fs::remove_file(&link); + } + } + // An unresolvable link is dangling — its target tree is + // already gone. Reclaim it when the literal target sits + // inside this install, which is what clears the + // ` -> global-/` strays a + // previous leak left behind. + Err(_) => { + let lex = aube_linker::normalize_path(&absolute); + if lex.starts_with(&install_lex) + || install_canon + .as_ref() + .is_some_and(|canon| lex.starts_with(canon)) + { + let _ = std::fs::remove_file(&link); + } + } } } Err(_) => { @@ -388,48 +592,25 @@ pub fn unlink_bins(install_dir: &Path, bin_dir: &Path, bin_names: &[String]) { } #[cfg(windows)] { - // On Windows, bins are cmd-shim wrapper scripts. Parse the .cmd - // shim to extract the embedded relative target path and verify - // it resolves into install_dir before removing — same ownership - // semantics as the Unix read_link check. + // On Windows, bins are cmd-shim wrapper scripts whose embedded target + // is the surface path under `install_dir` (`relative_bin_target`), not + // a store path — so containment alone still decides ownership here and + // the store-resolution problem the Unix arm handles cannot arise. let Ok(install_canon) = std::fs::canonicalize(install_dir) else { return; }; - for name in bin_names { + for bin in bins { + let name = &bin.name; let cmd_path = bin_dir.join(format!("{name}.cmd")); let Ok(content) = std::fs::read_to_string(&cmd_path) else { continue; }; - // The .cmd shim embeds the target as `"%~dp0\"`. - // Two shapes: - // - node shim: extract from the ELSE branch (`prog "%~dp0\ - // " %*`), skipping the IF-branch `"%~dp0\node.exe"`. - // - direct-exec shim for a native bin (#394): the whole file - // is `@"%~dp0\" %*` — a line that STARTS with `@"%~dp0\`, - // which the node shim never produces. Its `` typically - // ends in `.exe`, so it must be matched by shape, not by the - // `.exe"` filter below (which would drop it and skip the - // ownership check, over-removing another install's bin). - let owned = content - .lines() - .filter_map(|line| { - let line = line.trim(); - if let Some(after) = line.strip_prefix("@\"%~dp0\\") { - let end = after.find('"')?; - return Some(after[..end].to_string()); - } - // Match the fallback line: `prog "%~dp0\" %*` - // Skip lines containing `.exe"` (those are the IF branch). - if line.contains("%~dp0\\") && !line.contains(".exe\"") { - let start = line.find("%~dp0\\")?; - let after = &line[start + 6..]; // skip `%~dp0\` - let end = after.find('"')?; - Some(after[..end].to_string()) - } else { - None - } - }) - .next(); + // One parse, shared with the link path — see + // `aube_linker::parse_win_shim_target`. Inlining a second copy here + // is what let the reader drift from the writer before: this is the + // DELETE path, so a stale parse removes the wrong binary or leaves + // a live one behind, and the round-trip test would stay green. + let owned = aube_linker::parse_win_shim_target(&content); if let Some(rel) = owned { let resolved = bin_dir.join(&rel); if let Ok(resolved) = std::fs::canonicalize(&resolved) @@ -444,13 +625,30 @@ pub fn unlink_bins(install_dir: &Path, bin_dir: &Path, bin_names: &[String]) { } } -/// Enumerate bin names for every alias in an install dir. Used by the -/// remove path to know which symlinks to clean up. -pub fn bin_names_for(install_dir: &Path, aliases: &[String]) -> Vec { +/// One bin an install owns: the name it occupies in ``, plus the +/// canonical path that bin is expected to resolve to. +#[derive(Debug, Clone)] +pub struct OwnedBin { + pub name: String, + /// `None` when the target could not be canonicalized at capture time — + /// [`unlink_bins`] then falls back to its containment tests. + pub target: Option, +} + +/// Enumerate the bins every alias in an install dir owns, resolving each +/// target as it goes. +/// +/// **Call this BEFORE mutating or deleting the install.** The target is the +/// only evidence of ownership that survives an isolated layout, and it can +/// only be read while the install is intact: `aube update -g` unlinks stale +/// bins *after* re-installing, by which point the manifest no longer lists +/// them and the old files are gone. +pub fn owned_bins(install_dir: &Path, aliases: &[String]) -> Vec { let modules = super::project_modules_dir(install_dir); let mut out = Vec::new(); for alias in aliases { - let manifest_path = modules.join(alias).join("package.json"); + let pkg_dir = modules.join(alias); + let manifest_path = pkg_dir.join("package.json"); let Ok(raw) = std::fs::read_to_string(&manifest_path) else { continue; }; @@ -460,16 +658,22 @@ pub fn bin_names_for(install_dir: &Path, aliases: &[String]) -> Vec { let Some(bin_field) = json.get("bin") else { continue; }; - match bin_field { - serde_json::Value::String(_) => { - out.push(alias.rsplit('/').next().unwrap_or(alias).to_string()); - } - serde_json::Value::Object(map) => { - for name in map.keys() { - out.push(name.clone()); - } + let bins: Vec<(String, String)> = match bin_field { + serde_json::Value::String(rel) => { + let name = alias.rsplit('/').next().unwrap_or(alias).to_string(); + vec![(name, rel.clone())] } - _ => {} + serde_json::Value::Object(map) => map + .iter() + .filter_map(|(k, v)| v.as_str().map(|s| (k.clone(), s.to_string()))) + .collect(), + _ => continue, + }; + for (name, rel) in bins { + out.push(OwnedBin { + name, + target: std::fs::canonicalize(pkg_dir.join(&rel)).ok(), + }); } } out @@ -485,8 +689,23 @@ pub fn bin_names_for(install_dir: &Path, aliases: &[String]) -> Vec { /// that's actually a symlink to `/private/var/folders/...`). Without /// normalizing here, `starts_with` silently returns false and the /// physical install dir leaks. -pub fn remove_package(info: &GlobalPackageInfo, layout: &GlobalLayout) -> miette::Result<()> { - let bins = bin_names_for(&info.install_dir, &info.aliases); +/// +/// `keep_bins` names bins a CALLER has already re-linked to something it owns, +/// and it exists because `add -g` commits the new install before tearing down +/// the priors it replaces. Two installs of the same package at the same +/// version share one content-store path, so this package's recorded target +/// still matches the link the new install just wrote — without the exclusion, +/// dropping a prior would delete the live bin. Pass an empty set to remove +/// every bin this package owns. +pub fn remove_package( + info: &GlobalPackageInfo, + layout: &GlobalLayout, + keep_bins: &std::collections::BTreeSet<&str>, +) -> miette::Result<()> { + let bins: Vec = owned_bins(&info.install_dir, &info.aliases) + .into_iter() + .filter(|bin| !keep_bins.contains(bin.name.as_str())) + .collect(); unlink_bins(&info.install_dir, &layout.bin_dir, &bins); // Remove the hash pointer first. A missing pointer is fine (the @@ -580,7 +799,14 @@ mod tests { .unwrap(); } - unlink_bins(&install_dir, &bin_dir, &names); + let bins: Vec = names + .iter() + .map(|name| OwnedBin { + name: name.clone(), + target: std::fs::canonicalize(&target).ok(), + }) + .collect(); + unlink_bins(&install_dir, &bin_dir, &bins); for name in &names { assert!( @@ -591,4 +817,298 @@ mod tests { } } } + + /// The layout a real isolated install produces: `node_modules/` is a + /// symlink into a content store OUTSIDE the install dir, so the bin link + /// resolves clean out of `install_dir`. Containment alone cannot recognise + /// that bin as ours — which is what left every `remove -g` bin behind in + /// the global bin dir as a dangling link. + /// + /// Ownership must come from the target `owned_bins` captured off this + /// install's own manifest, so the assertion below is what fails when that + /// evidence is dropped. + #[cfg(unix)] + #[test] + fn unlink_bins_removes_a_bin_that_resolves_into_the_content_store() { + for prefer_symlink in [None, Some(false)] { + let dir = tempfile::tempdir().unwrap(); + let install_dir = dir.path().join("install"); + let bin_dir = dir.path().join("bin"); + let store_pkg = dir.path().join("store/pkg@1.0.0/node_modules/pkg"); + std::fs::create_dir_all(&store_pkg).unwrap(); + std::fs::create_dir_all(install_dir.join("node_modules")).unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + std::fs::write(store_pkg.join("cli.js"), b"#!/usr/bin/env node\n").unwrap(); + std::fs::write( + store_pkg.join("package.json"), + br#"{"name":"pkg","version":"1.0.0","bin":{"pkg":"cli.js"}}"#, + ) + .unwrap(); + std::os::unix::fs::symlink(&store_pkg, install_dir.join("node_modules/pkg")).unwrap(); + + let bins = owned_bins(&install_dir, &["pkg".to_string()]); + assert_eq!( + bins.len(), + 1, + "owned_bins must read the manifest through the store symlink" + ); + let install_canon = std::fs::canonicalize(&install_dir).unwrap(); + assert!( + bins[0] + .target + .as_ref() + .is_some_and(|t| !t.starts_with(&install_canon)), + "the fixture is only meaningful if the target resolves OUTSIDE \ + install_dir — otherwise containment would pass on its own" + ); + + aube_linker::create_bin_shim( + &bin_dir, + "pkg", + &install_dir.join("node_modules/pkg/cli.js"), + aube_linker::BinShimOptions { + prefer_symlinked_executables: prefer_symlink, + ..Default::default() + }, + ) + .unwrap(); + assert!( + bin_dir.join("pkg").symlink_metadata().is_ok(), + "positive control: the bin must exist before we unlink it" + ); + + unlink_bins(&install_dir, &bin_dir, &bins); + + assert!( + bin_dir.join("pkg").symlink_metadata().is_err(), + "bin resolving into the content store is still ours and must be \ + removed (prefer_symlinked_executables={prefer_symlink:?})" + ); + } + } + + /// The global bin dir is shared with every other tool that installs there, + /// so an occupied slot is only ours to take when we can show we put it + /// there. A foreign file must survive — silently replacing one is how a + /// global install eats another tool's binary. + /// + /// The store-shape case is the one that needs the package scan: the link + /// resolves out of `pkg_dir` into the content store, so it looks exactly as + /// foreign as a stranger's symlink until it is matched against what the + /// installs actually own. + #[cfg(unix)] + #[test] + fn bin_slot_is_writable_only_when_the_occupant_is_ours() { + let dir = tempfile::tempdir().unwrap(); + let pkg_dir = dir.path().join("global-aube"); + let bin_dir = dir.path().join("bin"); + let install_dir = pkg_dir.join("1234-abcd"); + let store_pkg = dir.path().join("store/pkg@1.0.0/node_modules/pkg"); + std::fs::create_dir_all(&store_pkg).unwrap(); + std::fs::create_dir_all(install_dir.join("node_modules")).unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + std::fs::write(store_pkg.join("cli.js"), b"#!/usr/bin/env node\n").unwrap(); + std::fs::write( + store_pkg.join("package.json"), + br#"{"name":"pkg","version":"1.0.0","bin":{"pkg":"cli.js"}}"#, + ) + .unwrap(); + std::os::unix::fs::symlink(&store_pkg, install_dir.join("node_modules/pkg")).unwrap(); + // `scan_packages` only sees an install through its hash pointer, and + // the manifest is where it reads the alias back. + std::fs::write( + install_dir.join("package.json"), + br#"{"name":"aube-global","dependencies":{"pkg":"1.0.0"}}"#, + ) + .unwrap(); + std::os::unix::fs::symlink(&install_dir, pkg_dir.join("deadbeef")).unwrap(); + + assert!( + bin_slot_is_writable(&bin_dir, &pkg_dir, "pkg"), + "an empty slot is free" + ); + + std::fs::write(bin_dir.join("pkg"), b"#!/bin/sh\necho not ours\n").unwrap(); + assert!( + !bin_slot_is_writable(&bin_dir, &pkg_dir, "pkg"), + "a foreign regular file must not be overwritten" + ); + + std::fs::remove_file(bin_dir.join("pkg")).unwrap(); + aube_linker::create_bin_shim( + &bin_dir, + "pkg", + &install_dir.join("node_modules/pkg/cli.js"), + aube_linker::BinShimOptions::default(), + ) + .unwrap(); + assert!( + bin_slot_is_writable(&bin_dir, &pkg_dir, "pkg"), + "our own prior link is ours to replace on a re-add" + ); + } + + /// The same policy on EVERY platform, including the one it keeps breaking on. + /// + /// The store-shape test above needs `std::os::unix::fs::symlink` for its + /// fixture, so `#[cfg(unix)]` compiles it away on the `windows-latest` leg of + /// `.github/workflows/aube-parity.yml` — leaving the Windows arm of the guard + /// with no test that composes it with a real occupant, which is how two + /// consecutive Windows-only defects reached this function. Neither case here + /// builds a symlink by hand — the writer makes whatever its platform uses — + /// so both run everywhere. + /// + /// Each assertion pins one of those defects. Consulting only the + /// extensionless path on Windows reads a foreign `pkg.cmd` slot as empty and + /// fails the second; demanding every `win_shim_paths` entry prove itself + /// rejects the shims this writer just emitted and fails the third. + #[test] + fn the_slot_policy_holds_for_plain_files_on_every_platform() { + let dir = tempfile::tempdir().unwrap(); + let pkg_dir = dir.path().join("global-aube"); + let bin_dir = dir.path().join("bin"); + let install_dir = pkg_dir.join("1234-abcd/node_modules/pkg"); + std::fs::create_dir_all(&install_dir).unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + let target = install_dir.join("cli.js"); + std::fs::write(&target, b"#!/usr/bin/env node\n").unwrap(); + + assert!( + bin_slot_is_writable(&bin_dir, &pkg_dir, "pkg"), + "an empty slot is free" + ); + + // A stranger's entry, in whichever path this platform actually consults. + let foreign = if cfg!(windows) { + bin_dir.join("pkg.cmd") + } else { + bin_dir.join("pkg") + }; + std::fs::write(&foreign, b"@echo not ours\n").unwrap(); + assert!( + !bin_slot_is_writable(&bin_dir, &pkg_dir, "pkg"), + "a foreign file must not be overwritten" + ); + std::fs::remove_file(&foreign).unwrap(); + + // Ours, written by the production writer rather than a hand-built + // string, so the guard is read against what actually gets emitted. + aube_linker::create_bin_shim( + &bin_dir, + "pkg", + &target, + aube_linker::BinShimOptions::default(), + ) + .unwrap(); + assert!( + bin_slot_is_writable(&bin_dir, &pkg_dir, "pkg"), + "a shim this writer just emitted is ours to replace on a re-add" + ); + } + + /// `add -g` links the new install's bins BEFORE tearing down the priors it + /// replaces. A prior holding the same package+version resolves to the same + /// content-store path as the new one, so its recorded target matches the + /// live link — and dropping it would delete the bin the user just + /// installed. `keep_bins` is what prevents that. + /// + /// Both `keep_bins` values run against an identical fixture, so the pair is + /// its own control: an ignored `keep_bins` fails the first case, and a + /// broken ownership check fails the second. Either way the install dir must + /// go — skipping a bin is not skipping the removal. + #[cfg(unix)] + #[test] + fn remove_package_keeps_only_the_bins_the_replacing_install_relinked() { + for keep in [ + ["pkg"] + .into_iter() + .collect::>(), + std::collections::BTreeSet::new(), + ] { + let survives = keep.contains("pkg"); + let dir = tempfile::tempdir().unwrap(); + let pkg_dir = dir.path().join("global-aube"); + let bin_dir = dir.path().join("bin"); + let install_dir = pkg_dir.join("1234-abcd"); + let store_pkg = dir.path().join("store/pkg@1.0.0/node_modules/pkg"); + std::fs::create_dir_all(&store_pkg).unwrap(); + std::fs::create_dir_all(install_dir.join("node_modules")).unwrap(); + std::fs::create_dir_all(&bin_dir).unwrap(); + std::fs::write(store_pkg.join("cli.js"), b"#!/usr/bin/env node\n").unwrap(); + std::fs::write( + store_pkg.join("package.json"), + br#"{"name":"pkg","version":"1.0.0","bin":{"pkg":"cli.js"}}"#, + ) + .unwrap(); + std::os::unix::fs::symlink(&store_pkg, install_dir.join("node_modules/pkg")).unwrap(); + + aube_linker::create_bin_shim( + &bin_dir, + "pkg", + &install_dir.join("node_modules/pkg/cli.js"), + aube_linker::BinShimOptions::default(), + ) + .unwrap(); + + let info = GlobalPackageInfo { + hash: "deadbeef".to_string(), + install_dir: std::fs::canonicalize(&install_dir).unwrap(), + aliases: vec!["pkg".to_string()], + }; + let layout = GlobalLayout { + bin_dir: bin_dir.clone(), + pkg_dir: pkg_dir.clone(), + }; + remove_package(&info, &layout, &keep).unwrap(); + + assert_eq!( + bin_dir.join("pkg").symlink_metadata().is_ok(), + survives, + "with keep_bins={keep:?} the bin should {}", + if survives { "survive" } else { "be removed" } + ); + assert!( + !install_dir.exists(), + "the physical install dir must be removed either way \ + (keep_bins={keep:?})" + ); + } + } + + /// A bin a LATER install took over keeps its new owner's target, so the + /// earlier install must leave it alone. This is the property the ownership + /// check exists to protect, and the one a blanket "remove by name" breaks. + #[cfg(unix)] + #[test] + fn unlink_bins_leaves_a_bin_another_install_took_over() { + let dir = tempfile::tempdir().unwrap(); + let ours = dir.path().join("ours"); + let theirs = dir.path().join("theirs"); + let bin_dir = dir.path().join("bin"); + for base in [&ours, &theirs] { + std::fs::create_dir_all(base.join("node_modules/pkg")).unwrap(); + std::fs::write(base.join("node_modules/pkg/cli.js"), b"#!/bin/sh\n").unwrap(); + } + std::fs::create_dir_all(&bin_dir).unwrap(); + + // The link on disk belongs to `theirs`. + aube_linker::create_bin_shim( + &bin_dir, + "pkg", + &theirs.join("node_modules/pkg/cli.js"), + aube_linker::BinShimOptions::default(), + ) + .unwrap(); + + let bins = vec![OwnedBin { + name: "pkg".to_string(), + target: std::fs::canonicalize(ours.join("node_modules/pkg/cli.js")).ok(), + }]; + unlink_bins(&ours, &bin_dir, &bins); + + assert!( + bin_dir.join("pkg").symlink_metadata().is_ok(), + "the bin belongs to a later install and must survive" + ); + } } diff --git a/vendor/aube/crates/aube/src/commands/prefix.rs b/vendor/aube/crates/aube/src/commands/prefix.rs index 6659e4469..df235da15 100644 --- a/vendor/aube/crates/aube/src/commands/prefix.rs +++ b/vendor/aube/crates/aube/src/commands/prefix.rs @@ -2,7 +2,8 @@ //! //! Mirrors `pnpm prefix`. Without flags, prints the current project root //! (or cwd when no project root is found). With `--global` / `-g`, prints -//! the global prefix directory used for PATH-visible global bins. +//! the root holding global package installs. That is NOT the bin directory: +//! bins link into the shared user-binary dir on PATH, which `bin -g` prints. use clap::Args; @@ -13,7 +14,7 @@ Examples: /home/user/project $ aube prefix -g - /home/user/.local/share/pnpm + /home/user/.local/share/aube "; #[derive(Debug, Args)] diff --git a/vendor/aube/crates/aube/src/commands/remove.rs b/vendor/aube/crates/aube/src/commands/remove.rs index bb2166c47..0756f5098 100644 --- a/vendor/aube/crates/aube/src/commands/remove.rs +++ b/vendor/aube/crates/aube/src/commands/remove.rs @@ -250,7 +250,9 @@ fn run_global(packages: &[String]) -> miette::Result<()> { for name in packages { match super::global::find_package(&layout.pkg_dir, name) { Some(info) => { - super::global::remove_package(&info, &layout)?; + // Nothing to keep: `remove -g` is not replacing this package, + // so every bin it owns should go. + super::global::remove_package(&info, &layout, &std::collections::BTreeSet::new())?; eprintln!("Removed global {name}"); any_removed = true; } diff --git a/vendor/aube/crates/aube/src/commands/update.rs b/vendor/aube/crates/aube/src/commands/update.rs index dc3e1ce3f..74935aa63 100644 --- a/vendor/aube/crates/aube/src/commands/update.rs +++ b/vendor/aube/crates/aube/src/commands/update.rs @@ -1045,7 +1045,10 @@ async fn run_global(args: UpdateArgs) -> miette::Result> { let original_cwd = crate::dirs::cwd()?; let result = async { for (info, package_args) in selected { - let old_bins = super::global::bin_names_for(&info.install_dir, &info.aliases); + // Captured before the re-install: a stale bin is gone from the + // manifest by the time we unlink it, so its target can only be + // resolved now. + let old_bins = super::global::owned_bins(&info.install_dir, &info.aliases); super::retarget_cwd(&info.install_dir)?; let mut inner = args.clone(); @@ -1074,14 +1077,15 @@ async fn run_global(args: UpdateArgs) -> miette::Result> { let linked = super::global::link_bins( &info.install_dir, &layout.bin_dir, + &layout.pkg_dir, &info.aliases, shim_opts, )?; let linked_set: std::collections::BTreeSet<&str> = linked.iter().map(String::as_str).collect(); - let stale_bins: Vec = old_bins + let stale_bins: Vec = old_bins .into_iter() - .filter(|name| !linked_set.contains(name.as_str())) + .filter(|bin| !linked_set.contains(bin.name.as_str())) .collect(); super::global::unlink_bins(&info.install_dir, &layout.bin_dir, &stale_bins); if !linked.is_empty() { diff --git a/vendor/aube/test/global_install.bats b/vendor/aube/test/global_install.bats index fa00a3f24..5124599e1 100644 --- a/vendor/aube/test/global_install.bats +++ b/vendor/aube/test/global_install.bats @@ -4,7 +4,7 @@ setup() { load 'test_helper/common_setup' _common_setup # Route global installs into the per-test temp dir so nothing escapes - # the sandbox. `bin_dir` = AUBE_HOME, `pkg_dir` = AUBE_HOME/global-aube. + # the sandbox. `bin_dir` = AUBE_HOME, `pkg_dir` = AUBE_HOME/global. # aube prints AUBE_HOME as-is (no canonicalize), so compare against the # env var verbatim, not `pwd -P`. export AUBE_HOME="$TEST_TEMP_DIR/aube-home" @@ -24,7 +24,7 @@ teardown() { @test "aube root -g prints the global package directory" { run aube root -g assert_success - assert_output "$AUBE_HOME/global-aube" + assert_output "$AUBE_HOME/global" } @test "aube prefix -g prints the global prefix directory" { @@ -45,11 +45,17 @@ teardown() { assert_output "$AUBE_HOME" } -@test "aube bin -g honors PNPM_HOME when AUBE_HOME is unset" { +# PNPM_HOME used to win here, so that an existing pnpm user already had the +# directory on PATH. It is no longer read: a global operation must not resolve +# through another package manager's configuration, and the assumption had gone +# stale anyway — pnpm 11 puts its global bins in `/bin`, one level below +# the directory this returned. +@test "aube bin -g ignores PNPM_HOME and uses the conventional bin dir" { unset AUBE_HOME - PNPM_HOME="$TEST_TEMP_DIR/pnpm-home" run aube bin -g + XDG_BIN_HOME="$TEST_TEMP_DIR/xdg-bin" PNPM_HOME="$TEST_TEMP_DIR/pnpm-home" run aube bin -g assert_success - assert_output "$TEST_TEMP_DIR/pnpm-home" + assert_output "$TEST_TEMP_DIR/xdg-bin" + refute_output --partial "pnpm-home" } @test "aube list -g reports nothing on an empty global dir" { @@ -149,7 +155,7 @@ teardown() { run aube add -g is-odd@0.1.2 assert_success - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" install_dir="$(find "$pkg_dir" -mindepth 1 -maxdepth 1 -type d -print -quit)" rm "$install_dir/aube-lock.yaml" @@ -185,7 +191,7 @@ teardown() { assert_success # At least one symlink entry (the hash) should exist in the pkg dir - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" run bash -c "find '$pkg_dir' -maxdepth 1 -type l | wc -l | tr -d ' '" assert_success assert_output "1" @@ -198,7 +204,7 @@ teardown() { assert_success # Only one install dir + one hash pointer should remain. - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" run bash -c "find '$pkg_dir' -maxdepth 1 -type l | wc -l | tr -d ' '" assert_output "1" run bash -c "find '$pkg_dir' -maxdepth 1 -type d | tail -n +2 | wc -l | tr -d ' '" @@ -269,7 +275,7 @@ teardown() { run aube add -g aube-test-builds-marker@1.0.0 assert_success - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" install_dir="$(find "$pkg_dir" -mindepth 1 -maxdepth 1 -type d -print -quit)" assert_file_not_exists "$install_dir/aube-builds-marker.txt" @@ -308,7 +314,7 @@ teardown() { refute_output --partial "must be reviewed before install" refute_output --partial "ignored build scripts" - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" install_dir="$(find "$pkg_dir" -mindepth 1 -maxdepth 1 -type d -print -quit)" # Build actually ran — the marker dep's postinstall writes the file. assert_file_exists "$install_dir/aube-builds-marker.txt" @@ -326,7 +332,7 @@ teardown() { refute_output --partial "must be reviewed before install" refute_output --partial "ignored build scripts" - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" install_dir="$(find "$pkg_dir" -mindepth 1 -maxdepth 1 -type d -print -quit)" assert_file_exists "$install_dir/aube-builds-marker.txt" } @@ -339,7 +345,7 @@ teardown() { refute_output --partial "must be reviewed before install" refute_output --partial "ignored build scripts" - pkg_dir="$AUBE_HOME/global-aube" + pkg_dir="$AUBE_HOME/global" install_dir="$(find "$pkg_dir" -mindepth 1 -maxdepth 1 -type d -print -quit)" # Denied dep stayed skipped, but strictDepBuilds accepted the # explicit review decision.