From d7584b425c1e2667fe01ddd55e689a763458b07c Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:50:14 -0700 Subject: [PATCH 01/19] pm: stop global remove from leaking every bin it linked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remove -g` reported success, deleted the install directory, and left the bin symlinks behind as dangling entries in the global bin directory. Ownership was decided by canonicalizing the bin symlink and requiring the result to sit under the install dir. Under the isolated layout `node_modules/` is itself a symlink into the shared content store, so the bin resolves out of the install dir entirely and the test never matched. Nothing was ever unlinked. Replace `bin_names_for` with `owned_bins`, which records each bin's canonical target alongside its name. The target has to be captured before the caller mutates the install: `update -g` unlinks stale bins after re-installing, by which point the manifest no longer lists them. `unlink_bins` now accepts a bin whose resolved link matches that recorded target, keeping containment as the second test for layouts that do hold the target inside the install. A dangling link pointing into the install is reclaimed too, which clears entries the old behaviour stranded. Two installs of one package at one version share a content-store path, so recorded targets cannot tell their bins apart. That is harmless on remove but not on replace: `add -g` links the new install before tearing down the priors it supersedes, so a prior's recorded target matches the link just written. `remove_package` therefore takes the set of bins the caller has already re-linked and skips them; `remove -g` passes an empty set. Windows is unaffected — its cmd shims embed the surface path under the install dir, so containment alone still decides ownership there. --- .../crates/aube/src/commands/add/global.rs | 13 +- .../aube/crates/aube/src/commands/global.rs | 340 +++++++++++++++--- .../aube/crates/aube/src/commands/remove.rs | 4 +- .../aube/crates/aube/src/commands/update.rs | 9 +- 4 files changed, 314 insertions(+), 52 deletions(-) diff --git a/vendor/aube/crates/aube/src/commands/add/global.rs b/vendor/aube/crates/aube/src/commands/add/global.rs index 0bb9a8e3e..a242d69eb 100644 --- a/vendor/aube/crates/aube/src/commands/add/global.rs +++ b/vendor/aube/crates/aube/src/commands/add/global.rs @@ -317,9 +317,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 +345,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}"); diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index 6ef0fb797..cfc686ed2 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -312,26 +312,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. /// -/// 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]) { +/// 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. +/// +/// 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 +352,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,14 +415,15 @@ 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; @@ -444,13 +472,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 +505,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 +536,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 +646,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 +664,179 @@ 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:?})" + ); + } + } + + /// `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/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 1c9ae75a7..92d01c89b 100644 --- a/vendor/aube/crates/aube/src/commands/update.rs +++ b/vendor/aube/crates/aube/src/commands/update.rs @@ -984,7 +984,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(); @@ -1018,9 +1021,9 @@ async fn run_global(args: UpdateArgs) -> miette::Result> { )?; 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() { From 78e88f20873be8ffbb9aa0ea01a246fd413f2a2c Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:21:45 -0700 Subject: [PATCH 02/19] pm: stop a global install from replacing a binary it does not own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `add -g` overwrote whatever already occupied a bin name, with no warning and a zero exit. The global bin directory is shared by construction — it is whatever directory the user keeps on PATH — so the occupant is routinely another tool's binary or one the user put there by hand. Link only into a slot we can show is ours: empty, a symlink into the global package directory, a symlink resolving to a target one of the installs there owns, or a regular file carrying the shim marker `create_bin_shim` writes. Anything else is left alone and reported. The store-shape case is why this consults the installed packages rather than testing containment. Under the isolated layout a bin link resolves through `node_modules/` into the shared content store, landing outside the package directory, so it is indistinguishable from a stranger's symlink until matched against what the installs record. A collision skips that one name rather than failing the command: the other packages in the same install still have to land, and the occupant belongs to the user. `add -g` re-adding its own bins is unaffected, since a prior install's links are recognised as ours. This is stricter than pnpm, which guards only against another global package it manages and overwrites an unrecognised file. --- .../crates/aube/src/commands/add/global.rs | 8 +- .../aube/crates/aube/src/commands/global.rs | 138 +++++++++++++++++- .../aube/crates/aube/src/commands/update.rs | 1 + 3 files changed, 143 insertions(+), 4 deletions(-) diff --git a/vendor/aube/crates/aube/src/commands/add/global.rs b/vendor/aube/crates/aube/src/commands/add/global.rs index a242d69eb..720ad46f4 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 diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index cfc686ed2..354884a5a 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -257,14 +257,77 @@ 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 { + let link = bin_dir.join(name); + let Ok(meta) = link.symlink_metadata() else { + return true; // nothing there + }; + let pkg_canon = std::fs::canonicalize(pkg_dir).unwrap_or_else(|_| pkg_dir.to_path_buf()); + + 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. + if aube_linker::normalize_path(&absolute).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 our shims only when it carries the marker + // `create_bin_shim` writes. Any other script in the slot belongs to + // somebody else. + match std::fs::read_to_string(&link) { + Ok(content) => aube_linker::parse_posix_shim_target(&content).is_some(), + Err(_) => false, + } + } +} + /// 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 +337,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 +365,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}"))?; @@ -734,6 +805,67 @@ mod tests { } } + /// 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" + ); + } + /// `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 diff --git a/vendor/aube/crates/aube/src/commands/update.rs b/vendor/aube/crates/aube/src/commands/update.rs index 92d01c89b..8f23023df 100644 --- a/vendor/aube/crates/aube/src/commands/update.rs +++ b/vendor/aube/crates/aube/src/commands/update.rs @@ -1016,6 +1016,7 @@ 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, )?; From ec6d0ec1cc1351f6c5952f86cd70f4a02ab8ffe6 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:35:19 -0700 Subject: [PATCH 03/19] pm: put global bins on a directory that is already on PATH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A global install reported success and printed the directory it linked into, but the commands could not run: bins went to a pnpm-named path (`~/.local/share/pnpm`, `~/Library/pnpm`) that nothing puts on PATH. Reported twice. The resolution was a verbatim copy of pnpm's own data-directory logic, leaf name included, while every other path helper in the engine reads the embedder's `data_namespace`. Its stated reason — that a pnpm user already has the directory on PATH — held only for someone who had run `pnpm setup`, and pnpm 11 moved its global bins to `/bin`, one level below what this returned. Split the two roots. Bins resolve to the shared user-binary directory the system already exposes: `_HOME`, `XDG_BIN_HOME`, `$XDG_DATA_HOME/../bin`, then `~/.local/bin`, on every platform, matching uv and pipx. Package installs move under the engine's own data namespace at `//global`, beside the content store. `PNPM_HOME` is no longer read: a global operation must not resolve through another package manager's configuration. The XDG-relative entry is derived with `parent()` rather than joining a literal `..`, because that path is printed by `bin -g`, compared against PATH entries, and belongs in a shell profile. When the resolved directory is absent from PATH the install now says so and names the line to add, for bash/zsh and fish. Both pnpm majors refuse the install instead; a warning is enough here because the packages are installed and reachable by absolute path, and the new default is a directory most systems already expose, so an error would be a false alarm in the common case. Existing installs under the old directory are left where they are. --- crates/nub-cli/src/pm_engine/info_family.rs | 8 +- .../crates/aube/src/commands/add/global.rs | 14 ++ .../aube/crates/aube/src/commands/global.rs | 160 +++++++++++------- .../aube/crates/aube/src/commands/prefix.rs | 5 +- vendor/aube/test/global_install.bats | 30 ++-- 5 files changed, 143 insertions(+), 74 deletions(-) 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/vendor/aube/crates/aube/src/commands/add/global.rs b/vendor/aube/crates/aube/src/commands/add/global.rs index 720ad46f4..4579406c8 100644 --- a/vendor/aube/crates/aube/src/commands/add/global.rs +++ b/vendor/aube/crates/aube/src/commands/add/global.rs @@ -364,6 +364,20 @@ async fn run_global_inner( pluralizer::pluralize("bin", linked.len() as isize, true), layout.bin_dir.display() ); + // Reporting success for an install whose commands cannot run is the + // defect behind nubjs/nub#642 and #708. Both pnpm majors refuse the + // install outright instead; a warning is chosen over an error because + // the packages ARE installed and usable by absolute path, and because + // the default bin dir is one most systems already have on PATH — so an + // error would be a false alarm for the common case. + if !global::dir_is_on_path(&layout.bin_dir) { + let dir = layout.bin_dir.display(); + eprintln!( + "warning: {dir} is not on PATH, so the commands just installed will not run.\n \ + bash/zsh: export PATH=\"{dir}:$PATH\"\n \ + fish: set -gx PATH \"{dir}\" $PATH" + ); + } } Ok(()) diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index 354884a5a..fe668300b 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 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/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. From 060581f50acbd84d76e89a261a6ac60e29f3c81f Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:37:11 -0700 Subject: [PATCH 04/19] install: stop re-adding the PATH block on every run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The installer decided whether to edit a shell profile by testing whether the bin directory was already in `$PATH`, and never read the profile at all. A profile edited by an earlier run is not reflected in the current shell until it is sourced, so re-running the installer from that shell appended another `# nub` block — once per run, without limit. Match the exact line before writing, and report the profile as already configured when it is there. Verified against the pre-fix script with the same harness: three runs produced three blocks, and now produce one. --- install.sh | 28 ++++++++++++++++++++++++++++ site/public/install.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/install.sh b/install.sh index a517d45f4..5075092d4 100755 --- a/install.sh +++ b/install.sh @@ -312,11 +312,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 '' @@ -333,6 +355,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' @@ -344,6 +369,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/public/install.sh b/site/public/install.sh index a517d45f4..5075092d4 100755 --- a/site/public/install.sh +++ b/site/public/install.sh @@ -312,11 +312,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 '' @@ -333,6 +355,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' @@ -344,6 +369,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")" { From 99c9660093c793fa7344627da706e9af9d0372d0 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:42:11 -0700 Subject: [PATCH 05/19] tests: add an end-to-end guard for global bin handling The unit tests cover the ownership logic against constructed fixtures. This covers what only appears once a package is really installed: the content-store symlink shape the linker produces, the resolved on-disk layout, and the PATH warning. Three of the four defects it guards were invisible to unit tests for that reason. The header records how the script lies when it is wrong. Every check 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, including shell redirection following a leftover symlink and corrupting the content store instead of planting a foreign file. Hence the explicit aborts, and hence the instruction to run it against a build that predates the fix: if the control does not fail, the harness is broken rather than the code. Verified both ways. Against this branch, 12/12 pass. Against a pre-fix build, six fail, one per defect, and the invariants still pass. --- tests/global-install/run.sh | 134 ++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100755 tests/global-install/run.sh diff --git a/tests/global-install/run.sh b/tests/global-install/run.sh new file mode 100755 index 000000000..94dfe42c9 --- /dev/null +++ b/tests/global-install/run.sh @@ -0,0 +1,134 @@ +#!/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 warns when the bin dir is not on PATH ==" +# The sandboxed HOME guarantees this dir is absent from PATH, so the warning +# must fire. Reporting success for commands that cannot run is #642 itself. +run remove -g cowsay >/dev/null 2>&1 +run install -g cowsay > "$ROOT/i5.log" 2>&1 +grep -q "is not on PATH" "$ROOT/i5.log"; check "install warns that the bin dir is not on PATH" $? +grep -q "export PATH=" "$ROOT/i5.log"; check "the warning names the line to add" $? + +echo +if [ "$fail" = "0" ]; then echo "ALL CHECKS PASSED"; else echo "FAILURES PRESENT"; fi +exit "$fail" From 9ccb7a5e5bf9afbaeeaa6c24a753fef20a825a43 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:43:41 -0700 Subject: [PATCH 06/19] agents: aube tests DO run in CI, via aube-parity The build-discipline section said a test added under vendor/aube "neither runs nor protects anything". True of the root gates only: aube-parity.yml runs `cargo test --workspace` inside vendor/aube on ubuntu and windows, path-filtered to vendor/aube/**, gating pull requests. As written it reads as a reason not to write the test. --- wiki/agents.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wiki/agents.md b/wiki/agents.md index 670887bea..3d737e20f 100644 --- a/wiki/agents.md +++ b/wiki/agents.md @@ -331,7 +331,7 @@ Feature-specific harnesses live under `tests//` — e.g. `tests/pnp/` b ``` A dev-dependency counts: adding one to a crate the root workspace owns moves the ROOT lock too, and it moves it again later than you expect — after whatever build last regenerated it. - **`vendor/aube` is the same story, and its failure mode is quieter.** It is a path DEPENDENCY, not a workspace member, so `cargo test -p aube-scripts` from the repo root refuses outright — `package 'aube-scripts' cannot be tested because it requires dev-dependencies and is not a member of the workspace`. Run aube's tests from inside it, with its own target dir: `cd vendor/aube && CARGO_TARGET_DIR= cargo test -p `. The consequence worth internalising is that **a test you add under `vendor/aube/` is invisible to every nub-side gate** — `--all-targets` from the root never builds a path dependency's test targets — so it neither runs nor protects anything until that gate exists (tracked as the aube-workspace gate). Verified 2026-08-04 by adding a test there and watching the root invocation refuse to run it. + **`vendor/aube` is the same story, and its failure mode is quieter.** It is a path DEPENDENCY, not a workspace member, so `cargo test -p aube-scripts` from the repo root refuses outright — `package 'aube-scripts' cannot be tested because it requires dev-dependencies and is not a member of the workspace`. Run aube's tests from inside it, with its own target dir: `cd vendor/aube && CARGO_TARGET_DIR= cargo test -p `. The consequence worth internalising is that **a test you add under `vendor/aube/` is invisible to the ROOT gates** — `--all-targets` from the root never builds a path dependency's test targets. Verified 2026-08-04 by adding a test there and watching the root invocation refuse to run it. **It DOES still run in CI, so write it:** `.github/workflows/aube-parity.yml` runs `cargo test --workspace` with `working-directory: vendor/aube`, on ubuntu AND windows, path-filtered to `vendor/aube/**`, and it gates pull requests. Corrected 2026-08-16 — this previously said such a test "neither runs nor protects anything", which reads as a reason not to write one at all. 3. **Prefer running the heavy gates remotely.** `cargo clippy --all-targets --all-features` and a full `cargo test` are what saturate the dev host when many worktrees build at once. Run them on an ephemeral GCE spot VM: `nub scripts/remote-build.ts --job clippy --detach`, then `nub scripts/remote-build.ts --attach ` to collect (the `remote-build` skill). Byte-identical CI invocation, a few cents each. **Use `--detach`/`--attach`, never the plain foreground form** — a foreground run is SIGKILLed at the agent harness's timeout, which no handler can catch, so cleanup is skipped and the builder leaks until its server-side TTL; `--attach` exits 75 meaning "still running, call again". The `--profile fast` inner loop stays local. 4. **End-to-end test the specific functionality with a tmp fixture** — build the binary and exercise the actual feature against a throwaway fixture in `/tmp`, diffing against the reference tool where parity is claimed. "Tests pass" is not "the feature works." 5. **Use Docker for behavior touching the global cache / config / a clean machine** — global `~/.npmrc`, config homes, the CAS store, first-run install, a Node floor — in an ephemeral `docker run --rm` container so host state can't mask or pollute the result. Linux only; Windows rides CI. From ab76aadececba9b1d7d33c33cea66e0ddde9e72b Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:00:52 -0700 Subject: [PATCH 07/19] pm: put the global bin directory on PATH after a global install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installing a package globally has one purpose — running it by name — so an install that leaves the directory unreachable has not done what was asked. Wire it into every profile the current shell reads, on success, and only when the directory is genuinely absent from PATH. Detection anchors on the marker, never on the line beneath it. This family's directory is resolved at run time, so it legitimately spells differently between runs: `$HOME`-relative against absolute, a different HOME under sudo, a relocated XDG_BIN_HOME. Matching the line text would miss those and append a second block, and a profile that grows one stale PATH entry per relocation is invisible to the user, because a shell only ever reports the winning entry. When the marker is present with a different line under it, that line is rewritten in place. `ShimBlock` therefore takes `Cow<'static, str>`; the PM-shim and node-shim families keep compile-time constants and cannot reach the new `Rewritten` outcome, which is still handled at both call sites so that giving either a runtime directory later cannot panic during shell startup. The engine no longer reports PATH state itself. With both in place an install printed a remediation immediately before announcing it had applied it; `dir_is_on_path` stays public so the host decides and emits one message per outcome. Asking the user to edit a profile by hand is now reached only when no profile could be written. A failure to write is reported and swallowed. The packages are installed either way, and an unwritable profile must not fail an install that already succeeded. --- crates/nub-cli/src/cli.rs | 14 + .../nub-cli/src/pm_engine/install_family.rs | 69 +++++ crates/nub-core/src/node/shim.rs | 8 +- crates/nub-core/src/pm/shim.rs | 249 ++++++++++++++++-- tests/global-install/run.sh | 21 +- .../crates/aube/src/commands/add/global.rs | 19 +- 6 files changed, 327 insertions(+), 53 deletions(-) diff --git a/crates/nub-cli/src/cli.rs b/crates/nub-cli/src/cli.rs index 74d8709cc..a231a2ee6 100644 --- a/crates/nub-cli/src/cli.rs +++ b/crates/nub-cli/src/cli.rs @@ -10030,6 +10030,13 @@ fn run_pm_shim_install() -> Result { ProfileOutcome::AlreadyPresent(profile) => { println!(" PATH: already present in {}", profile.display()) } + // Not reachable for these two families — their directories are + // compile-time constants, so the line beneath the marker never differs. + // Handled rather than left to `unreachable!` so that giving either one a + // runtime-resolved directory later cannot panic in a user's shell setup. + ProfileOutcome::Rewritten(profile) => { + println!(" PATH: updated the entry in {}", 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!( @@ -10148,6 +10155,13 @@ fn run_node_shim_install() -> Result { ProfileOutcome::AlreadyPresent(profile) => { println!(" PATH: already present in {}", profile.display()) } + // Not reachable for these two families — their directories are + // compile-time constants, so the line beneath the marker never differs. + // Handled rather than left to `unreachable!` so that giving either one a + // runtime-resolved directory later cannot panic in a user's shell setup. + ProfileOutcome::Rewritten(profile) => { + println!(" PATH: updated the entry in {}", 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/install_family.rs b/crates/nub-cli/src/pm_engine/install_family.rs index c5a9a6754..682533bd2 100644 --- a/crates/nub-cli/src/pm_engine/install_family.rs +++ b/crates/nub-cli/src/pm_engine/install_family.rs @@ -398,6 +398,70 @@ 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; + } + 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 {}", + 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())?; @@ -408,6 +472,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, @@ -415,6 +481,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 a5b66e424..d536d908a 100644 --- a/crates/nub-core/src/node/shim.rs +++ b/crates/nub-core/src/node/shim.rs @@ -37,10 +37,10 @@ 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: "# 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: std::borrow::Cow::Borrowed("# nub node shim"), + posix_line: std::borrow::Cow::Borrowed(r#"export PATH="$HOME/.nub/node-shim:$PATH""#), + fish_line: std::borrow::Cow::Borrowed("set -gx PATH $HOME/.nub/node-shim $PATH"), + dir_marker: std::borrow::Cow::Borrowed(".nub/node-shim"), }; /// The name the shim intercepts. diff --git a/crates/nub-core/src/pm/shim.rs b/crates/nub-core/src/pm/shim.rs index 6b66f8e27..e677dd024 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 _; @@ -711,26 +712,87 @@ 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 — the `$HOME`-relative dir (`.nub/shims`). - pub(crate) dir_marker: &'static str, + pub(crate) dir_marker: Cow<'static, str>, } /// The PM shims' block (`~/.nub/shims`, `# nub shims`). 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 { @@ -738,9 +800,14 @@ 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. Only reachable for a family whose directory + /// is resolved at runtime and can therefore change between runs; appending + /// instead is what would accumulate a stale block per relocation. + 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 — @@ -796,16 +863,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); } @@ -815,11 +886,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(), }, }) } @@ -828,7 +900,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, } @@ -844,7 +916,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 { @@ -885,7 +957,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, }] } @@ -907,18 +979,33 @@ 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) { + std::fs::write(&target.path, rewritten) + .with_context(|| format!("rewriting {}", target.path.display()))?; + return Ok(ProfileOutcome::Rewritten(target.path.clone())); + } if target.may_create { if let Some(parent) = target.path.parent() { std::fs::create_dir_all(parent) @@ -932,7 +1019,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())), }; @@ -941,6 +1030,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 @@ -1021,7 +1138,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. @@ -1056,7 +1173,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 } } @@ -2006,10 +2123,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] @@ -2080,7 +2197,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() } ); } @@ -2424,4 +2541,78 @@ 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}" + ); + } + + /// 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/tests/global-install/run.sh b/tests/global-install/run.sh index 94dfe42c9..824ee6d48 100755 --- a/tests/global-install/run.sh +++ b/tests/global-install/run.sh @@ -121,13 +121,22 @@ esac [ ! -e "$HOME_DIR/Library/pnpm" ]; check "nothing written to ~/Library/pnpm" $? echo -echo "== 5. an install warns when the bin dir is not on PATH ==" -# The sandboxed HOME guarantees this dir is absent from PATH, so the warning -# must fire. Reporting success for commands that cannot run is #642 itself. +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 -run install -g cowsay > "$ROOT/i5.log" 2>&1 -grep -q "is not on PATH" "$ROOT/i5.log"; check "install warns that the bin dir is not on PATH" $? -grep -q "export PATH=" "$ROOT/i5.log"; check "the warning names the line to add" $? +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 diff --git a/vendor/aube/crates/aube/src/commands/add/global.rs b/vendor/aube/crates/aube/src/commands/add/global.rs index 4579406c8..2d5af2560 100644 --- a/vendor/aube/crates/aube/src/commands/add/global.rs +++ b/vendor/aube/crates/aube/src/commands/add/global.rs @@ -364,20 +364,11 @@ async fn run_global_inner( pluralizer::pluralize("bin", linked.len() as isize, true), layout.bin_dir.display() ); - // Reporting success for an install whose commands cannot run is the - // defect behind nubjs/nub#642 and #708. Both pnpm majors refuse the - // install outright instead; a warning is chosen over an error because - // the packages ARE installed and usable by absolute path, and because - // the default bin dir is one most systems already have on PATH — so an - // error would be a false alarm for the common case. - if !global::dir_is_on_path(&layout.bin_dir) { - let dir = layout.bin_dir.display(); - eprintln!( - "warning: {dir} is not on PATH, so the commands just installed will not run.\n \ - bash/zsh: export PATH=\"{dir}:$PATH\"\n \ - fish: set -gx PATH \"{dir}\" $PATH" - ); - } + // 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(()) From 18afd403970e59188e21315a5bb3cf22517b966a Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:09:30 -0700 Subject: [PATCH 08/19] docs: document global installs `-g` appeared only as a flag in two code blocks, with nothing on where a global package goes or why its executables are reachable. That is the surface nubjs/nub#642 reported against. Covers the two directories and why they are separate, the resolution order for the executable directory, how to point it elsewhere, what the install does when the directory is absent from PATH, and what happens when a name is already taken by something else. Every console block is captured output, not illustration. --- site/content/docs/install/index.mdx | 50 +++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/site/content/docs/install/index.mdx b/site/content/docs/install/index.mdx index b1864f464..ae046bd3e 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. From e46c57b0aa2f9cd2832f018d7756aed9c43eefa2 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:10:17 -0700 Subject: [PATCH 09/19] pm: fix three defects in the global bin ownership guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From review on #773. Windows had no guard on the PATH wiring. `SHELL` is unset there, so the shell probe fell back to the POSIX dialect and wrote `export PATH=…` into a `.profile` that neither cmd.exe nor PowerShell reads, then reported PATH configured. Print the line instead, as `nub pm shim` already does — the silent POSIX write was worse than either automating it or declining to. The ownership check inspected only the extensionless path, while the Windows writer emits three files and overwrites each. npm, pnpm and yarn install a `.cmd` with no extensionless sibling, so the slot read as empty and their shim was replaced — the case this guard exists to stop. Drive the check off `win_shim_paths`, the writer's own list, so the two cannot drift apart again. A regular file is now claimed only when its embedded target resolves back into the global package directory. Testing for the `%~dp0` shim shape alone would adopt every foreign wrapper, since npm's use it too. The dangling-link arm compared a lexically normalized target against a canonicalized package dir. Those never match once any component is a symlink — macOS `/tmp`, a symlinked `$HOME`, every `tempdir()` on macOS — so a link this tool created was reported as somebody else's. Accept either form, which is what `unlink_bins` already did. The `.cmd` parse is exercised on every platform rather than behind `cfg(windows)`: it is the whole of Windows ownership, and a Windows-only test goes unrun in the local loop. --- .../nub-cli/src/pm_engine/install_family.rs | 14 +++ vendor/aube/crates/aube-linker/src/lib.rs | 2 + vendor/aube/crates/aube-linker/src/sys.rs | 6 +- .../aube/crates/aube/src/commands/global.rs | 118 ++++++++++++++++-- 4 files changed, 129 insertions(+), 11 deletions(-) diff --git a/crates/nub-cli/src/pm_engine/install_family.rs b/crates/nub-cli/src/pm_engine/install_family.rs index d208e957f..166cd2e34 100644 --- a/crates/nub-cli/src/pm_engine/install_family.rs +++ b/crates/nub-cli/src/pm_engine/install_family.rs @@ -423,6 +423,20 @@ fn wire_global_bin_path(code: i32) { 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!( diff --git a/vendor/aube/crates/aube-linker/src/lib.rs b/vendor/aube/crates/aube-linker/src/lib.rs index 2d2bf3ad4..6946d57a3 100644 --- a/vendor/aube/crates/aube-linker/src/lib.rs +++ b/vendor/aube/crates/aube-linker/src/lib.rs @@ -47,6 +47,8 @@ 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, }; +#[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..2615329e5 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")), diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index fe668300b..a08461f7e 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -315,14 +315,65 @@ pub fn symlink_force(target: &Path, link: &Path) -> miette::Result<()> { /// 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 { - let link = bin_dir.join(name); + // 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)] + { + return aube_linker::win_shim_paths(bin_dir, name) + .iter() + .all(|p| slot_entry_is_ours(p, pkg_dir)); + } + #[cfg(not(windows))] + slot_entry_is_ours(&bin_dir.join(name), pkg_dir) +} + +/// 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. Mirrors the parse +/// `unlink_bins` performs, and reads the same on any platform so the logic can +/// be unit-tested without a Windows runner. +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 + }) +} + +/// 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 { + let Ok(raw) = std::fs::read_link(link) else { return false; }; let absolute = if raw.is_absolute() { @@ -331,7 +382,8 @@ fn bin_slot_is_writable(bin_dir: &Path, pkg_dir: &Path, name: &str) -> bool { link.parent().unwrap_or(bin_dir).join(raw) }; // Surface shape: the link points straight into the global pkg dir. - if aube_linker::normalize_path(&absolute).starts_with(&pkg_canon) { + 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) { @@ -350,13 +402,25 @@ fn bin_slot_is_writable(bin_dir: &Path, pkg_dir: &Path, name: &str) -> bool { Err(_) => false, } } else { - // A regular file is one of our shims only when it carries the marker - // `create_bin_shim` writes. Any other script in the slot belongs to - // somebody else. - match std::fs::read_to_string(&link) { - Ok(content) => aube_linker::parse_posix_shim_target(&content).is_some(), - Err(_) => false, - } + // 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(|| 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) } } @@ -912,6 +976,40 @@ mod tests { ); } + /// The `.cmd` parse is the whole of Windows ownership, so it is written to + /// run on any platform: a Windows-only test would go unexercised in the + /// local loop and only speak up on CI. + /// + /// The second case is the one that matters. npm, pnpm and yarn all emit + /// `%~dp0`-relative wrappers of the same shape, so a check for the marker + /// alone adopts every foreign shim as ours — the target is what tells them + /// apart, and this asserts the parse recovers it rather than the shape. + #[test] + fn win_shim_target_is_recovered_from_both_wrapper_shapes() { + // Direct-exec wrapper for a native bin. + assert_eq!( + parse_win_shim_target("@\"%~dp0\\..\\global\\1-2\\node_modules\\p\\p.exe\" %*\n") + .as_deref(), + Some("..\\global\\1-2\\node_modules\\p\\p.exe") + ); + // Node wrapper: the IF branch names node.exe, the ELSE branch carries + // the real target, and only the latter may be returned. + let node_shim = concat!( + "@IF EXIST \"%~dp0\\node.exe\" (\r\n", + " \"%~dp0\\node.exe\" \"%~dp0\\..\\global\\1-2\\node_modules\\p\\cli.js\" %*\r\n", + ") ELSE (\r\n", + " node \"%~dp0\\..\\global\\1-2\\node_modules\\p\\cli.js\" %*\r\n", + ")\r\n" + ); + assert_eq!( + parse_win_shim_target(node_shim).as_deref(), + Some("..\\global\\1-2\\node_modules\\p\\cli.js"), + "the node.exe IF branch must never be mistaken for the target" + ); + // A wrapper with no embedded target is not ours to claim. + assert_eq!(parse_win_shim_target("@echo off\r\necho hi\r\n"), None); + } + /// `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 From 40b74cff347d537843697446450477058ec297fc Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:09:25 -0700 Subject: [PATCH 10/19] pm: key Windows bin ownership on the .cmd shim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring every path `win_shim_paths` names to prove itself rejected the shims this tool had just written. Of the three files the Windows writer emits, only `.cmd` carries a recoverable target: the `.ps1` and the extensionless wrapper resolve their base directory at run time and stamp no marker, and the generator that does stamp one is `#[cfg(unix)]`. The consequence was not a spurious warning. An unwritable slot makes `link_bins` skip every bin, `add -g` reads the empty result as "nothing was re-linked", and that disarms the filter protecting the prior install's bins — so re-adding a package already installed removed it and left no command behind. Decide from the `.cmd` alone, which is the file `unlink_bins` already keys on, so the two agree. With no `.cmd` present nothing in the slot came from this writer, which never emits a sibling without one, so any occupant is somebody else's. --- .../aube/crates/aube/src/commands/global.rs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index a08461f7e..e800116ac 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -324,12 +324,32 @@ fn bin_slot_is_writable(bin_dir: &Path, pkg_dir: &Path, name: &str) -> bool { // the two cannot drift apart. #[cfg(windows)] { - return aube_linker::win_shim_paths(bin_dir, name) + // 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| slot_entry_is_ours(p, pkg_dir)); + .all(|p| p.symlink_metadata().is_err()) } #[cfg(not(windows))] - slot_entry_is_ours(&bin_dir.join(name), pkg_dir) + { + slot_entry_is_ours(&bin_dir.join(name), pkg_dir) + } } /// Extract the `%~dp0`-relative target a Windows `.cmd` shim execs. From ea5775b2a5e83751cd92e0519280e86d7f2ed7e1 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:19:07 -0700 Subject: [PATCH 11/19] linker: bind the Windows shim parser to the writer it must agree with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership guard's parser lived in `aube` while the generator it reads lives here, and the only test composing the guard with an occupant was `#[cfg(unix)]` — compiled away on the one platform where that guard has now been rewritten twice. Its parser test fed hand-written strings, so nothing checked the reader against what the writer emits, which is the disagreement that produced both defects. Move `parse_win_shim_target` next to `generate_cmd_shim` and round-trip them: generate the wrapper, parse it back, assert the recovered target is the one embedded. The second case covers the node dialect's unquoted `@SET NODE_PATH=%~dp0…` line, which must not be mistaken for the target. Both run on every platform. `generate_cmd_shim` is pure formatting, and gating it to Windows is what forced the fixtures to be invented in the first place; it is now `cfg(any(windows, test))` so a non-Windows release build still leaves it out. The three fixture-based tests this replaces are removed rather than kept alongside. Keeping them preserves the thing that failed: assertions built from the same assumption as the code they check. --- vendor/aube/crates/aube-linker/src/lib.rs | 3 +- vendor/aube/crates/aube-linker/src/sys.rs | 70 ++++++++++++++++++- .../aube/crates/aube/src/commands/global.rs | 60 +--------------- 3 files changed, 72 insertions(+), 61 deletions(-) diff --git a/vendor/aube/crates/aube-linker/src/lib.rs b/vendor/aube/crates/aube-linker/src/lib.rs index 6946d57a3..db92e6cb7 100644 --- a/vendor/aube/crates/aube-linker/src/lib.rs +++ b/vendor/aube/crates/aube-linker/src/lib.rs @@ -45,7 +45,8 @@ 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; diff --git a/vendor/aube/crates/aube-linker/src/sys.rs b/vendor/aube/crates/aube-linker/src/sys.rs index 2615329e5..a3ea82191 100644 --- a/vendor/aube/crates/aube-linker/src/sys.rs +++ b/vendor/aube/crates/aube-linker/src/sys.rs @@ -812,7 +812,42 @@ 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. Mirrors the parse +/// `unlink_bins` performs, and reads the same on any platform so the logic can +/// be unit-tested without a Windows runner. +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, @@ -2826,4 +2861,37 @@ 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 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/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index e800116ac..994a7e225 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -352,30 +352,6 @@ fn bin_slot_is_writable(bin_dir: &Path, pkg_dir: &Path, name: &str) -> bool { } } -/// 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. Mirrors the parse -/// `unlink_bins` performs, and reads the same on any platform so the logic can -/// be unit-tested without a Windows runner. -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 - }) -} - /// 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 { @@ -435,7 +411,7 @@ fn slot_entry_is_ours(link: &Path, pkg_dir: &Path) -> bool { }; let rel = aube_linker::parse_posix_shim_target(&content) .map(str::to_string) - .or_else(|| parse_win_shim_target(&content)); + .or_else(|| aube_linker::parse_win_shim_target(&content)); let Some(rel) = rel else { return false; }; @@ -996,40 +972,6 @@ mod tests { ); } - /// The `.cmd` parse is the whole of Windows ownership, so it is written to - /// run on any platform: a Windows-only test would go unexercised in the - /// local loop and only speak up on CI. - /// - /// The second case is the one that matters. npm, pnpm and yarn all emit - /// `%~dp0`-relative wrappers of the same shape, so a check for the marker - /// alone adopts every foreign shim as ours — the target is what tells them - /// apart, and this asserts the parse recovers it rather than the shape. - #[test] - fn win_shim_target_is_recovered_from_both_wrapper_shapes() { - // Direct-exec wrapper for a native bin. - assert_eq!( - parse_win_shim_target("@\"%~dp0\\..\\global\\1-2\\node_modules\\p\\p.exe\" %*\n") - .as_deref(), - Some("..\\global\\1-2\\node_modules\\p\\p.exe") - ); - // Node wrapper: the IF branch names node.exe, the ELSE branch carries - // the real target, and only the latter may be returned. - let node_shim = concat!( - "@IF EXIST \"%~dp0\\node.exe\" (\r\n", - " \"%~dp0\\node.exe\" \"%~dp0\\..\\global\\1-2\\node_modules\\p\\cli.js\" %*\r\n", - ") ELSE (\r\n", - " node \"%~dp0\\..\\global\\1-2\\node_modules\\p\\cli.js\" %*\r\n", - ")\r\n" - ); - assert_eq!( - parse_win_shim_target(node_shim).as_deref(), - Some("..\\global\\1-2\\node_modules\\p\\cli.js"), - "the node.exe IF branch must never be mistaken for the target" - ); - // A wrapper with no embedded target is not ours to claim. - assert_eq!(parse_win_shim_target("@echo off\r\necho hi\r\n"), None); - } - /// `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 From 1cf6a0efb05b1d9ff1f06482383cb4bc536d8186 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:38:24 -0700 Subject: [PATCH 12/19] linker: one reader of the .cmd format, and a negative case for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unlink_bins` still carried a byte-equivalent copy of the parse, and the doc comment recorded that duplication as intent. Extracting a shared parser while leaving a second copy behind keeps exactly the drift the extraction was for — and that copy is the DELETE path, so a writer change that updates only the shared function leaves the remove path claiming the wrong bins while the round-trip test stays green. Collapsed onto the one function. Both round-trips drove input through this crate's own writer, so together they proved only that nub agrees with itself. A spurious `Some(..)` is how the guard claims a slot nub does not own, and that direction had no assertion after the fixture test was removed. Assert `None` against npm's real `cmd-shim` body: it assigns `SET dp0=%~dp0` once and builds paths from `%dp0%`, so the `%~dp0\` this parser keys on never appears. A hand-written string is the right instrument there and only there, because that writer lives in another repo. Also record what the parse does NOT establish: pnpm and yarn classic use `@zkochan/cmd-shim`, which emits the same `"%~dp0\"` shape this crate does, so their wrappers parse here too. Only where the recovered target resolves decides ownership. --- vendor/aube/crates/aube-linker/src/sys.rs | 54 +++++++++++++++++-- .../aube/crates/aube/src/commands/global.rs | 36 +++---------- 2 files changed, 57 insertions(+), 33 deletions(-) diff --git a/vendor/aube/crates/aube-linker/src/sys.rs b/vendor/aube/crates/aube-linker/src/sys.rs index a3ea82191..d2554fab8 100644 --- a/vendor/aube/crates/aube-linker/src/sys.rs +++ b/vendor/aube/crates/aube-linker/src/sys.rs @@ -816,9 +816,19 @@ fn safe_prog(prog: &str) -> &str { /// /// 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. Mirrors the parse -/// `unlink_bins` performs, and reads the same on any platform so the logic can -/// be unit-tested without a Windows runner. +/// `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(); @@ -2884,6 +2894,44 @@ mod tests { } } + /// 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. Loosening either branch — dropping the + /// `.exe"` filter, matching a bare `%~dp0`, falling back to a substring + /// scan — makes this fail instead of silently widening what nub calls its + /// own. + #[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. diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index 994a7e225..3d29c2aff 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -605,36 +605,12 @@ pub fn unlink_bins(install_dir: &Path, bin_dir: &Path, bins: &[OwnedBin]) { 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) From 9a7082a1f3173d9eea816d2327df745a73bd77d3 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:51:07 -0700 Subject: [PATCH 13/19] linker: say what the npm-shim test actually pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment claimed three loosenings would fail it. Only one does. Dropping the `.exe"` filter or matching a bare `%~dp0` both still yield `None` against an npm shim, because it carries no `%~dp0` followed by a backslash anywhere — so each branch misses on shape long before either condition is reached. A comment that promises coverage the test does not provide is worse than no comment: it tells the next reader their change is guarded when it is not. Narrowed to the substring-scan case, with the other two attributed to the round-trip test that does pin them, whose node-dialect input carries `"%~dp0\node.exe"`. --- vendor/aube/crates/aube-linker/src/sys.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/vendor/aube/crates/aube-linker/src/sys.rs b/vendor/aube/crates/aube-linker/src/sys.rs index d2554fab8..2435cb225 100644 --- a/vendor/aube/crates/aube-linker/src/sys.rs +++ b/vendor/aube/crates/aube-linker/src/sys.rs @@ -2901,10 +2901,17 @@ mod tests { /// 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. Loosening either branch — dropping the - /// `.exe"` filter, matching a bare `%~dp0`, falling back to a substring - /// scan — makes this fail instead of silently widening what nub calls its - /// own. + /// 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!( From 01beb152a3cb36911b13c036a7295e54c25ac7d7 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 20 Aug 2026 06:46:23 -0700 Subject: [PATCH 14/19] pm/node: print the re-source hint when an upgrade rewrites the PATH line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `ProfileOutcome::Rewritten` arms carried a comment claiming they were unreachable for the two fixed-directory shim families, "their directories are compile-time constants, so the line beneath the marker never differs." A constant is fixed within one build but can move between releases, and #752 moved the shims from `$HOME/.nub/shims` to the `XDG_DATA_HOME` form. A profile written by an older nub therefore carries a different line under the same marker, and the arm now runs on the upgrade path. Only when the legacy DIRECTORY is already gone: while it still exists the migration above strips the block outright and the outcome is `Added`. So this is reached by a synced dotfile on a machine that never had the old directory — where the live shell has a directory on PATH holding no shims. That makes the source hint matter more here than on a fresh add, and it was the one outcome not printing it. The same stale claim was on `ProfileOutcome`'s own doc; corrected there too. The new test seeds both `.zshrc` and `.zshenv`, because `add_path_block_for` ranks a per-file `Added` above `Rewritten` when folding the outcome, so seeding one profile would mask the rewrite behind the other file's `Added`. --- crates/nub-cli/src/cli.rs | 44 +++++++++++++++++++++---------- crates/nub-core/src/pm/shim.rs | 48 +++++++++++++++++++++++++++++++--- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/crates/nub-cli/src/cli.rs b/crates/nub-cli/src/cli.rs index 769922820..4118d0126 100644 --- a/crates/nub-cli/src/cli.rs +++ b/crates/nub-cli/src/cli.rs @@ -10322,13 +10322,21 @@ fn run_pm_shim_install() -> Result { ProfileOutcome::AlreadyPresent(profile) => { println!(" PATH: already present in {}", profile.display()) } - // Not reachable for these two families — their directories are - // compile-time constants, so the line beneath the marker never differs. - // Handled rather than left to `unreachable!` so that giving either one a - // runtime-resolved directory later cannot panic in a user's shell setup. - ProfileOutcome::Rewritten(profile) => { - println!(" PATH: updated the entry 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!( @@ -10476,13 +10484,21 @@ fn run_node_shim_install() -> Result { ProfileOutcome::AlreadyPresent(profile) => { println!(" PATH: already present in {}", profile.display()) } - // Not reachable for these two families — their directories are - // compile-time constants, so the line beneath the marker never differs. - // Handled rather than left to `unreachable!` so that giving either one a - // runtime-resolved directory later cannot panic in a user's shell setup. - ProfileOutcome::Rewritten(profile) => { - println!(" PATH: updated the entry 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-core/src/pm/shim.rs b/crates/nub-core/src/pm/shim.rs index cc171e88b..3b90be59d 100644 --- a/crates/nub-core/src/pm/shim.rs +++ b/crates/nub-core/src/pm/shim.rs @@ -1001,9 +1001,10 @@ pub enum ProfileOutcome { /// 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. Only reachable for a family whose directory - /// is resolved at runtime and can therefore change between runs; appending - /// instead is what would accumulate a stale block per relocation. + /// 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. @@ -2940,6 +2941,47 @@ mod tests { ); } + /// `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. From ad326c7996de3338d5dd904ca6ba7c8fe7535fd2 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 20 Aug 2026 07:04:56 -0700 Subject: [PATCH 15/19] global: test the bin-slot policy on every platform, not just unix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bin_slot_is_writable_only_when_the_occupant_is_ours` needs `std::os::unix::fs::symlink` to build its store-shape fixture, so `#[cfg(unix)]` compiles it away on the windows-latest leg of aube-parity.yml. That left the guard's Windows arm with no test composing it against a real occupant, on the one platform where it had been rewritten twice. The regular-file cases need no hand-built symlink: create the occupant with the production writer and let each platform place it where that platform looks. Both defects are pinned. Consulting only the extensionless path reads a foreign `pkg.cmd` slot as empty, failing the foreign-file assertion; requiring every `win_shim_paths` entry to prove itself rejects the shims the writer just emitted, failing the last one. The store-shape test keeps its gate — that fixture genuinely needs a symlink. --- .../aube/crates/aube/src/commands/global.rs | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/vendor/aube/crates/aube/src/commands/global.rs b/vendor/aube/crates/aube/src/commands/global.rs index 3d29c2aff..bcd0931d8 100644 --- a/vendor/aube/crates/aube/src/commands/global.rs +++ b/vendor/aube/crates/aube/src/commands/global.rs @@ -948,6 +948,64 @@ mod tests { ); } + /// 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 From 3e60620f05e283cd5e7ab84c20d740af62ec728c Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Thu, 20 Aug 2026 07:14:48 -0700 Subject: [PATCH 16/19] pm: never truncate a shell profile, and hint the re-source on add -g MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two consequences of the rewrite branch becoming a live upgrade path. `wire_global_bin_path` is the third site handling `Rewritten`, and the one where a missing hint costs most: it returns early when the bin dir is already on PATH, so every arm below runs only when it is NOT, and the user has just run `nub add -g` expecting a runnable command. `Added` and `AlreadyPresent` both say to restart the shell; `Rewritten` was silent about the session in hand. The rewrite itself wrote with `std::fs::write` — create, TRUNCATE, write_all — so an interruption between those steps leaves the user's `.zshrc` empty or holding a partial prefix, taking everything else in it along. The other branch of `append_block` appends and so can never shorten a profile; this branch was the first that could. `remove_path_block_from_profiles` already wrote through a temp + rename for exactly this reason, so that is now shared as `replace_profile_atomically` and both callers use it. Temp + rename brings its own hazard, which is what the new test pins: renaming onto a symlinked `~/.zshrc` would replace the link and orphan a dotfiles copy, where the plain write followed it. Canonicalizing first is what preserves the old behavior — dropping it turns both symlink tests red. The torn write has no unit test; it needs the process to die between truncate and write_all, so the atomicity is argued from rename. --- .../nub-cli/src/pm_engine/install_family.rs | 3 +- crates/nub-core/src/pm/shim.rs | 109 ++++++++++++++---- 2 files changed, 89 insertions(+), 23 deletions(-) diff --git a/crates/nub-cli/src/pm_engine/install_family.rs b/crates/nub-cli/src/pm_engine/install_family.rs index 166cd2e34..c8674fb95 100644 --- a/crates/nub-cli/src/pm_engine/install_family.rs +++ b/crates/nub-cli/src/pm_engine/install_family.rs @@ -448,7 +448,8 @@ fn wire_global_bin_path(code: i32) { } Ok(nub_core::pm::shim::ProfileOutcome::Rewritten(p)) => { eprintln!( - " PATH: updated the nub global bin entry in {} to {}", + " 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() ); diff --git a/crates/nub-core/src/pm/shim.rs b/crates/nub-core/src/pm/shim.rs index 3b90be59d..9a72e7fea 100644 --- a/crates/nub-core/src/pm/shim.rs +++ b/crates/nub-core/src/pm/shim.rs @@ -1171,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 @@ -1203,8 +1233,7 @@ fn append_block(target: &ProfileTarget, marker: &str) -> Result // 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) { - std::fs::write(&target.path, rewritten) - .with_context(|| format!("rewriting {}", target.path.display()))?; + replace_profile_atomically(&target.path, &rewritten, "shim")?; return Ok(ProfileOutcome::Rewritten(target.path.clone())); } if target.may_create { @@ -1306,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) @@ -2811,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() { From 38cccc56f4c418448df3482305b40d5b177b73e6 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Mon, 24 Aug 2026 09:55:19 -0700 Subject: [PATCH 17/19] version: retry the license-repair rename that Windows loses differently Two concurrent repairs race to move a stale LICENSE aside, and exactly one wins. The loser is expected and retryable, but it does not report the same way on both platforms: unix sees the source the winner already renamed and returns NotFound, which this loop retried. Windows returns 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. That fell to the fatal arm, so a repair that every other platform completed failed on Windows. The retry is bounded and Windows-only. PermissionDenied on a rename is otherwise a real permission fault, so retrying it without a bound would spin forever on a read-only directory, and gating on the platform leaves unix semantics exactly as they were. Surfaced by `concurrent_exact_license_repairs_leave_one_attested_notice` failing the windows-latest legs of ci.yml while passing everywhere else. --- crates/nub-core/src/version_management/mod.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/nub-core/src/version_management/mod.rs b/crates/nub-core/src/version_management/mod.rs index 54b3b5c73..b25f09702 100644 --- a/crates/nub-core/src/version_management/mod.rs +++ b/crates/nub-core/src/version_management/mod.rs @@ -641,6 +641,18 @@ 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. + let mut contended = 0u32; loop { match std::fs::hard_link(&tmp, &dest) { Ok(()) => return Ok(()), @@ -657,6 +669,17 @@ 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 < 64 => + { + contended += 1; + // Yield rather than spin: the holder only has to finish + // its own rename or drop its handle. + std::thread::yield_now(); + continue; + } Err(err) => { return Err(err) .with_context(|| format!("moving stale {} aside", dest.display())); From a6c4fba9b916a6e48fbeb9fb7aab7ac746d265a2 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:15:57 -0700 Subject: [PATCH 18/19] version: budget the contended-rename retry in time, not attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry waits on a holder finishing a MoveFileExW or closing a handle, which is a duration, but the bound counted attempts and paused with `thread::yield_now`. On Windows that is `SwitchToThread`, which yields only to a thread ready on the CURRENT processor and returns immediately when there is none — std's own comment says so. With the competing repairer on another core, every attempt could burn in microseconds and fail for exactly the reason it first failed, so the budget bounded nothing that mattered. Sleep 1ms per attempt against a 500ms budget instead. rustup takes the same position for Windows filesystem contention: `is_retryable_dir_error` counts PermissionDenied as retryable and backs off in real time. Unix is unaffected — the arm stays `cfg!(windows)`-gated and the NotFound arm keeps its unbounded retry, which is correct because NotFound means the winner already moved the file. --- crates/nub-core/src/version_management/mod.rs | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/crates/nub-core/src/version_management/mod.rs b/crates/nub-core/src/version_management/mod.rs index b25f09702..a75b74cdf 100644 --- a/crates/nub-core/src/version_management/mod.rs +++ b/crates/nub-core/src/version_management/mod.rs @@ -652,7 +652,20 @@ fn atomic_replace_file( // 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. - let mut contended = 0u32; + // + // 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. + 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(()), @@ -672,12 +685,10 @@ fn atomic_replace_file( Err(err) if cfg!(windows) && err.kind() == std::io::ErrorKind::PermissionDenied - && contended < 64 => + && contended < CONTENTION_BUDGET => { - contended += 1; - // Yield rather than spin: the holder only has to finish - // its own rename or drop its handle. - std::thread::yield_now(); + std::thread::sleep(CONTENTION_PAUSE); + contended += CONTENTION_PAUSE; continue; } Err(err) => { From 05ff3e5dd1883f4a09e05b63f7561a42dc4409d5 Mon Sep 17 00:00:00 2001 From: Colin McDonnell <3084745+colinhacks@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:27:02 -0700 Subject: [PATCH 19/19] version: say that the rename budget covers one call, not a whole repair atomic_write_node_license calls atomic_replace_file three times per iteration of its own 0..16 loop, so a repair can hold up to 48 independent budgets. The comment read as if 500ms bounded the repair. Left as is rather than threaded through a shared deadline: exhausting a budget returns Err and unwinds, so the failure path is still capped at one, and only calls that succeed after partially spending theirs add up. --- crates/nub-core/src/version_management/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/nub-core/src/version_management/mod.rs b/crates/nub-core/src/version_management/mod.rs index a75b74cdf..dc51ef689 100644 --- a/crates/nub-core/src/version_management/mod.rs +++ b/crates/nub-core/src/version_management/mod.rs @@ -663,6 +663,12 @@ fn atomic_replace_file( // 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;